PR final steps (to squash or not to squash)

[René J.V. Bertin]

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.