PR final steps (to squash or not to squash)
Hi, This has come up on the QtCurve PR and I cannot seem to find an explicit answer in the wiki (https://trac.macports.org/wiki/WorkingWithGit#pr) If a pull request has seen some evolution and thus commits due to the review process and/or somehow related reasons (for instance, because the process dragged on), should the commit history be squashed or not? I would expect not, certainly not if squashing to a single atomic commit means that whatever remains of the PR ticket after merging will no longer allow to see the evolution of the commit, and if that evolution is deemed to be of interest. Most other projects I know that accept patches through review use a separate review mechanism or simply a bug tracker, and then include a reference to the review request and/or bug ticket in the final, atomic commit to the repository. BTW, does a PR remain accessible via github.com after the source branch from which the pull was made has been deleted? R.
Re: PR final steps (to squash or not to squash)
Hi, On 5 December 2016 at 09:46, René J.V. Bertin wrote: > Hi, > > This has come up on the QtCurve PR and I cannot seem to find an explicit > answer in the wiki (https://trac.macports.org/wiki/WorkingWithGit#pr) > > If a pull request has seen some evolution and thus commits due to the review > process and/or somehow related reasons (for instance, because the process > dragged on), should the commit history be squashed or not? We discussed this quite a bit (I'm not sure where) and the conclusion was that: - we want a linear history (therefore we do rebasing) - we want to avoid "weird commits" (so yes, we want to squash the commits) BUT: multiple commits are still sometimes desired / OK. Usually "larryv" is the one who takes most care to split commits into "reasonable atomic commits". A typical example would be when you fix whitespaces in the first commit, fix typos in the second commit and add a variant in the third commit. Or when you fix a port in the first commit and then revbump dependent ports in the second commit. Why do we want to squash? Imagine a complete newbie that submits a pull request and accidentally modifies two other ports and makes some other stupid mistakes which might even break MacPorts (if the code of a Portfile doesn't compile, portindex fails etc.). Then those changes are gradually reversed and the ports gets improved in 20 commits (many of those commits with screwed up commit messages) until an acceptable state is reached. When maintainers of those ports that were accidentally modified would later browse through the history and potentially do a bisection to find when some bug was introduced, those additional unrelated commits would be pretty annoying. With the old workflow we would also keep cleaning up the code until it was ready and then a committer would make a single commit or two to represent potential months of brainstorming. I'm not saying that GitHub makes it easy to do the squashing (it has to be done manually in the command line), but well ... > I would expect not, certainly not if squashing to a single atomic commit > means that whatever remains of the PR ticket after merging will no longer > allow to see the evolution of the commit, and if that evolution is deemed to > be of interest. Most other projects I know that accept patches through review > use a separate review mechanism or simply a bug tracker, and then include a > reference to the review request and/or bug ticket in the final, atomic commit > to the repository. You may rewrite history to include *some* evolution, but not all of it and certainly not all the accidental errors that had to be fixed later. You may include addition of new subports or introduction of new variants as separate commits. The PR still keeps quite some history (I'm unable to tell you how much exactly). I guess that "larryv" is our biggest expert in this topic. > BTW, does a PR remain accessible via github.com after the source branch from > which the pull was made has been deleted? I don't have a scientific answer yet, but it does to quite some extent (probably not 100% of it). In many PRs you can see code blocks that say "outdated" where you can click to see the source code of old / rewritten commits. (You can probably try to open a PR to your own repository and test.) Mojca
Re: PR final steps (to squash or not to squash)
Mojca Miklavec wrote on 20161205::11:14:46 re: "Re: PR final steps (to squash or not to squash)" This was meant to be a public reply, sending again. >We discussed this quite a bit (I'm not sure where) and the conclusion was that: >- we want a linear history (therefore we do rebasing) >- we want to avoid "weird commits" (so yes, we want to squash the commits) I guess time and practice will tell, but that does sound like having a different patch review mechanism might be a good idea at some point in the future. Preferably something that makes it a bit easier to test a patch before it is committed. Did the discussion include consideration of the fact that each commit can send the build bots on a rebuilding spree (AFAIK it does, at least), something one would probably prefer to avoid? > Usually "larryv" is the one who takes most care to split commits into > "reasonable atomic commits". All due respect and all that, but this is error-prone, opens the door to "re-interpretation" of the intended changes ... and I guess we all have better things to do than this kind of task. > A typical example would be when you fix > whitespaces in the first commit, fix typos in the second commit and > add a variant in the third commit. A typical example indeed, but how justified is it to be this "pedantic" (for lack of a better term!) here? I can see some justification for it in (very) complex software where you may be patching multiple files at once, but Portfiles are by definition single-file programs that are rarely of true complexity (rather, much complexity is hidden in "base"). OTOH it *is* true that whitespace changes can have more side-effects in Tcl than they have in C, but that's also a possible source of error when a 3rd party starts splitting up more elaborate change sets. I know I'm very prone myself to address multiple aspect at once esp. when I'm taking over an abandoned port that's important for me (opencv, qca). You work on such a project, something that typically sees multiple commits in a personal repo, and without commit rights to the official tree you end up presenting a (more or less) finished version that can look very complicated if you only look at it as a patch w.r.t. the current verrsion. IOW, it has more of a submission of a new port than a change to an existing one, and is much easier to evaluate as such. Even if one can still describe the evolution in a chronological sequence of things one has changed it can be really untrivial to start over and recreate the corresponding patches (which one would have to test, which means introducing local regressions, etc). R.
Re: PR final steps (to squash or not to squash)
On 2016-12-05 13:37, René J.V. Bertin wrote: > Mojca Miklavec wrote on 20161205::11:14:46 re: "Re: PR final steps > (to squash or not to squash)" >> We discussed this quite a bit (I'm not sure where) and the >> conclusion was that: - we want a linear history (therefore we do >> rebasing) - we want to avoid "weird commits" (so yes, we want to >> squash the commits) > > I guess time and practice will tell, but that does sound like having > a different patch review mechanism might be a good idea at some point > in the future. Preferably something that makes it a bit easier to > test a patch before it is committed. What would be easier than just checking out the updated Portfile? You can also just download the patch and apply it. Open for suggestions. We migrated to GitHub because people were specifically asking for the ability to open pull requests instead of attaching patches in Trac. > Did the discussion include consideration of the fact that each commit > can send the build bots on a rebuilding spree (AFAIK it does, at > least), something one would probably prefer to avoid? Just needs someone to work on it, the discussion on this took place on this mailing list. However, our mailing list archives are still broken after the move off of macOS forge, so I cannot link to this directly. From: Eric A. Borisch Subject: Another workflow (pull requests) question. Date: Mon, 14 Nov 2016 10:51:21 -0600 Message-Id: >> Usually "larryv" is the one who takes most care to split commits >> into "reasonable atomic commits". > > All due respect and all that, but this is error-prone, opens the door > to "re-interpretation" of the intended changes ... and I guess we all > have better things to do than this kind of task. Well, it gets easier if the pull request author takes care of this. >> A typical example would be when you fix whitespaces in the first >> commit, fix typos in the second commit and add a variant in the >> third commit. > > A typical example indeed, but how justified is it to be this > "pedantic" (for lack of a better term!) here? I can see some > justification for it in (very) complex software where you may be > patching multiple files at once, but Portfiles are by definition > single-file programs that are rarely of true complexity (rather, much > complexity is hidden in "base"). OTOH it *is* true that whitespace > changes can have more side-effects in Tcl than they have in C, but > that's also a possible source of error when a 3rd party starts > splitting up more elaborate change sets. Is this supposed to be an argument for squashing or against? In my opinion, it does not make sense to keep every little change that was raised in reviews in a separate commit. That would be a lot of noise in the history and is usually not relevant. In any case, if you want to retain history, every commit has to follow our commit message guidelines. Rainer
tk not building on 10.6.8: #ifdef question in ticket 52090
Dear all, could anyone please have a look at https://trac.macports.org/ticket/52090 ? There's a question about "an appropriate guard for a #ifdef" in order to solve the problem. Thanks, and best wishes! Davide
Re: PR final steps (to squash or not to squash)
On Monday December 05 2016 14:58:08 Rainer Müller wrote: > > What would be easier than just checking out the updated Portfile? You > can also just download the patch and apply it. Open for suggestions. In this case that would probably rather be downloading the patch since checking out the portfile means fetching the whole fork with its full history. > > We migrated to GitHub because people were specifically asking for the > ability to open pull requests instead of attaching patches in Trac. I didn't know this, and you're right that the principle as considered by github is easy enough. It certainly makes it a lot easier to emit targeted critiques to specific bits of code, and to react to those comments point by point. I'm less convinced that the mechanism is the most suitable of the available alternatives for a procedure that requires possibly extensive or drawn-out reviewing if you don't want to commit the full review history and also don't want to use github's squash feature. As I said, time will tell (and I should have a new look at the current SourceTree to see what kind of bells and whistles it has that could prove useful). Since suggestions appear to be welcome here: I've mentioned before that I've grown quite fond of how ReviewBoard works, esp. the fact it works exclusively with patches without requiring a single commit (a nice example: https://git.reviewboard.kde.org/r/128880/). I can rave more about it on demand; I do think it could be suitable as an additional mechanism where a PR patch judged too complex or immature can be massaged into shape before being committed on the PR branch and from there merged into the main repository. > Just needs someone to work on it, the discussion on this took place on > this mailing list. Right. My fault I missed it, I am following from a distance because I don't want to be tempted to jump in discussions that hardly concern me, if at all. > However, our mailing list archives are still broken after the move off > of macOS forge, so I cannot link to this directly. > Subject: Another workflow (pull requests) question. I can probably find that thread via gmane, that's become my gateway for discussions on which I'm not CC'ed. > Well, it gets easier if the pull request author takes care of this. It certainly does, though not always by much, and only as long as the author has the exact same approach in splitting things up. And it's probably not trivial if possible at all to rewrite the history to correspond to a series of well-defined steps if those steps were never made as such at all, correct? > Is this supposed to be an argument for squashing or against? Mostly for (cf. below). Side-ways related: maybe the Git/PR wiki topic could say a word or two about when a rebase request might be made before a PR can be merged. > In my opinion, it does not make sense to keep every little change that > was raised in reviews in a separate commit. That would be a lot of noise > in the history and is usually not relevant. I agree and think it can be the rule as long as you can assume that PRs and ensuing reviews aim to introduce a coherent set of changes which can be summarised with a single commit message of reasonable length. The review history can be of interest, but doesn't have to be an integral part of the port history in order to be useful (a pointer to it is enough). > In any case, if you want to retain history, every commit has to follow > our commit message guidelines. Evidently. Not always completely: how do you describe the final commits I made to the qtcurve PR, for instance... To expand a bit on other thoughts in my previous message: with actual applications I've become used to keep patches organised by feature as that maps nicely to `patchfiles` and the principle of getting appropriate changes incorporated upstream. Not always easy when different patches start touching the same file, but it pays off when you start submitting the changes. Somehow this never occurs to me when I start working on a port, partly because there is no integrated mechanism to apply a series of patches to the Portfile. R.
Re: PR final steps (to squash or not to squash)
I'm going to throw in my 2c again asking for the 'squash and commit' button to be activated. I'm much more likely to wander through and commit some cut-and-dried PRs if it is something I can do drive-by, or even from my phone. And by cut-and-dried, I mean PRs from a prior contributor updating checksum and version numbers on a port, for example. - Eric
Re: PR final steps (to squash or not to squash)
On 2016-12-6 11:49 , Eric A. Borisch wrote: I'm going to throw in my 2c again asking for the 'squash and commit' button to be activated. I'm much more likely to wander through and commit some cut-and-dried PRs if it is something I can do drive-by, or even from my phone. And by cut-and-dried, I mean PRs from a prior contributor updating checksum and version numbers on a port, for example. If it's that cut-and-dried, what's wrong with rebase and merge? - Josh
Re: PR final steps (to squash or not to squash)
On Mon, Dec 5, 2016 at 6:56 PM, Joshua Root wrote: > On 2016-12-6 11:49 , Eric A. Borisch wrote: > >> I'm going to throw in my 2c again asking for the 'squash and commit' >> button to be activated. I'm much more likely to wander through and >> commit some cut-and-dried PRs if it is something I can do drive-by, or >> even from my phone. >> >> And by cut-and-dried, I mean PRs from a prior contributor updating >> checksum and version numbers on a port, for example. >> > > If it's that cut-and-dried, what's wrong with rebase and merge? > > - Josh > If there have been comments and an updated PR (multiple commits),should I rebase-and-merge from the web?. I thought the answer was no (non-linear commit history?) This was the particular case when I first brought it up. Just trying to reduce the "friction" to contributing to MP. - Eric
Re: PR final steps (to squash or not to squash)
On 2016-12-6 12:13 , Eric A. Borisch wrote: On Mon, Dec 5, 2016 at 6:56 PM, Joshua Root mailto:j...@macports.org>> wrote: On 2016-12-6 11:49 , Eric A. Borisch wrote: I'm going to throw in my 2c again asking for the 'squash and commit' button to be activated. I'm much more likely to wander through and commit some cut-and-dried PRs if it is something I can do drive-by, or even from my phone. And by cut-and-dried, I mean PRs from a prior contributor updating checksum and version numbers on a port, for example. If it's that cut-and-dried, what's wrong with rebase and merge? - Josh If there have been comments and an updated PR (multiple commits),should I rebase-and-merge from the web?. I thought the answer was no (non-linear commit history?) This was the particular case when I first brought it up. Just trying to reduce the "friction" to contributing to MP. Well, I wouldn't call that a cut-and-dried case, but: In the case of a PR branch with multiple commits, some of which are fixing problems with earlier commits rather than just being logically separate changes, squashing is preferable. But the nonlinear history problem is the result of *not* rebasing. From what I understand, what we'd really like for that case is a "squash, rebase and merge" option. Unless we've misunderstood what "squash and merge" does and it doesn't actually create a merge commit? - Josh
Re: PR final steps (to squash or not to squash)
From what I understand, what we'd really like for that case is a "squash, rebase and merge" option. Unless we've misunderstood what "squash and merge" does and it doesn't actually create a merge commit? - Josh No, it doesn't. See https://github.com/blog/2141-squash-your-commits. -- Zero King
Re: PR final steps (to squash or not to squash)
> On Dec 5, 2016, at 5:14 AM, Mojca Miklavec wrote: > > Usually "larryv" is the one who takes most care to split commits Hey now, why the scare quotes? :) vq
Re: PR final steps (to squash or not to squash)
> On Dec 5, 2016, at 8:57 PM, Zero King wrote: > >> From what I understand, what we'd really like for that case is >> a "squash, rebase and merge" option. Unless we've misunderstood what >> "squash and merge" does and it doesn't actually create a merge >> commit? > > No, it doesn't. See https://github.com/blog/2141-squash-your-commits. Squash merging is also described in git-merge(1): --squash, --no-squash Produce the working tree and index state as if a real merge happened (except for the merge information), but do not actually make a commit, move the HEAD, or record $GIT_DIR/MERGE_HEAD (to cause the next git commit command to create a merge commit). This allows you to create a single commit on top of the current branch whose effect is the same as merging another branch (or more in case of an octopus). With --no-squash perform the merge and commit the result. This option can be used to override --squash. The reason we are leery of "squash and merge" is not nonlinearity but that it would be the default action on the GitHub website and could make it too easy to mash a whole PR into one ungainly commit. vq
Re: PR final steps (to squash or not to squash)
> On Dec 5, 2016, at 10:38 AM, René J.V. Bertin wrote: > >> On Monday December 05 2016 14:58:08 Rainer Müller wrote: >> >> What would be easier than just checking out the updated Portfile? You >> can also just download the patch and apply it. Open for suggestions. > > In this case that would probably rather be downloading the patch since > checking out the portfile means fetching the whole fork with its full > history. Fetching from a fork does not download the full history. Only the objects that are not in your repository are fetched. >> Well, it gets easier if the pull request author takes care of this. > > It certainly does, though not always by much, and only as long as the > author has the exact same approach in splitting things up. I daresay I'm the fussiest committer here. Most of us would probably not nitpick a submitters' well-edited set of commits. > And it's probably not trivial if possible at all to rewrite the > history to correspond to a series of well-defined steps if those steps > were never made as such at all, correct? It's generally not trivial, no. Editing and revising prose to be clear and concise isn't trivial, either. > Side-ways related: maybe the Git/PR wiki topic could say a word or two > about when a rebase request might be made before a PR can be merged. Huh? vq
Re: PR final steps (to squash or not to squash)
> On Dec 5, 2016, at 7:37 AM, René J.V. Bertin wrote: > > I guess we all have better things to do than this kind of task. Our commit history is essentially communication with future committers about what we did and why. Like any other communication, it should be clear and useful. Much like writing documentation, polishing the commit history is not pleasant in the moment but is nevertheless time well-spent. > I can see some justification for it in (very) complex software where > you may be patching multiple files at once, but Portfiles are by > definition single-file programs that are rarely of true complexity > (rather, much complexity is hidden in "base"). They are also correspondingly easier to revise. > OTOH it *is* true that whitespace changes can have more side-effects > in Tcl than they have in C, but that's also a possible source of error > when a 3rd party starts splitting up more elaborate change sets. If you are worried about someone else editing your changes and botching them, I encourage you to make them presentable yourself. Failing that, committers reserve the right to modify submissions as they see fit before merging. > Even if one can still describe the evolution in a chronological > sequence of things one has changed it can be really untrivial to start > over and recreate the corresponding patches (which one would have to > test, which means introducing local regressions, etc). It doesn't have to be chronological, and each step need not be "correct" on its own. We expect users to always have the latest ports tree, so it's only important that the final result be correct. For example, in a sequence of commits that each changes a port's installed files, only the final commit need actually perform a revbump. vq
Re: [macports-ports] branch master updated: Add myself back as maintainer - I was incorrectly removed in 2007 and just noticed.
> On Dec 5, 2016, at 11:00 PM, toby wrote: > > tobypeterson pushed a commit to branch master > in repository macports-ports. > > > https://github.com/macports/macports-ports/commit/b79d2c1ac01df3629781a311219c01e0b3d61bcb > > The following commit(s) were added to refs/heads/master by this push: > > new b79d2c1 Add myself back as maintainer - I was incorrectly removed > in 2007 and just noticed. During spring cleaning in 2007, some time after OpenDarwin shut down and MacPorts moved to macOS forge in 2006, all @opendarwin.org addresses were replaced with nomaintainer, on the assumption that those people were no longer around. Before that was committed, maintainers were given some time to update their email addresses in their ports: http://www.mail-archive.com/macports-dev@lists.macosforge.org/msg00309.html https://github.com/macports/macports-ports/commit/055ddf508a3b1701f2af5d2b405ec3dc332f9600 If you would like to reclaim any other ports, here is the list of ports from which your old email address was removed (but many of these ports have probably since been claimed by other maintainers, so you may want to confer with them before making any major changes): 54321 9e Xaw3d a52dec adium adns advancecomp aee aget alac_decoder amphetamine aolserver arch archimedes audiofile audioslicer autobench awemud barcode bc bcpp beecrypt bf2c binclocken bison bluemoon bnbt bool boost-jam bs bsdiff bsflite byacc c-ares c-hey c_count calc ccache cheops cmdftp coreutils cssc ctags cupl cursive cvsync dap diction diffball dnsa dominion dsniff dsniff-devel dtach ebnf2yacc ee elinks epic5 epm ettercap fbopenssl ferite file fnv forkbomb fortune fprobe freeradius fsh galaxis gforth ghostview giflib gimp-print global glpk gnats gnucap gob1 gob2 greed groff gss gst gtime gunits gv gvpe gwhich hello httperf icecast2 ices0 ices2 ici id3tool insub iozone ipsvd irssi irssistats iverilog jabberd lame launch lcms ldmud libao libarchive libcaca libcddb libcdio libdvbpsi libedit libevent libexif libggi libggigcp libggimisc libggiwmh libgii libgiigic libident libmodplug libmsn libnet11 libnids libogg libopendaap libopennet libosip2 libotr libshout2 libsigcxx1 libtar libtheora libtomcrypt libtool-devel libtorrent libungif libusb libvorbis lightning lincity links1 localizer lostirc lws lynx macstl mathopd mkconsole monotone moscow_ml most mp3_check mp3cue mpg321 nail naim nano nano-devel nawk nbench-byte ncftp ndiff ne nedit nemesis netwalker-ircc ng-spice ngircd nzbget otrproxy outguess p5-dbi p5-mac-applescript p7zip packit pavuk pbzip2 pcre pgpdump physfs pike pipebench ploticus poc pork postal prcs prothon ptunnel q recode retawq rfcdiff rootsh scala scale2x scriptix scrollz shorten shttpd siege ski slang2 smartmontools smlnj snarf snort source-highlight sox speex speex-devel sudosh taglib tclcurl tcpdump tcpflow tcpick texi2html tf tor tor-devel ubench ucblogo unrtf vbpp vbs vms-empire vorbis-tools webfs webpublish webredirect wordplay wumpus xaos xbill xcircuit xephem xwpe ytalk yudit zile zsync
