Sensitive content Clicca per aprire/chiudere

@dalias hmm. I think there are really two separate questions there: (a) the risk of the linked-to repo vanishing _while_ you're still reviewing and considering the contribution, and (b) the risk of it vanishing long afterward so that the long-term record goes away.

I've never considered (b) particularly important, although I'm open to being persuaded otherwise. To my way of thinking, once I've accepted a patch, the _final_ version of the patch is important to preserve, because it reflects the history of how my actual code evolved. But all the review drafts are just scaffolding – important during construction, but after you're done you can safely take them down and throw them away. I apply this equally to other people's patches sent to me, and my own patches I send to other people.

I've never yet had the problem of the repo containing the PR branch vanishing in mid-review. I would have guessed that it wouldn't be common because it's in the submitter's interest to keep it running! But I suppose I'm thinking on the timescale of weeks at most, however long it takes me to either accept or reject.

If PRs were public, then it seems more plausible you'd want to keep even the rejected ones forever. A patch I didn't think was worthy of getting into _my_ version of the code might still be useful to someone else to apply downstream of me.

on the part of "can get force-pushed between starting to review and merging it": I guess that might mean you like to review patches by pointing a browser at the remote site?

I can see that if someone points me at a clone of my repo on Github, and I look at the patch in a browser and like it, then there's a risk that the version I 'git fetch' and merge into my repo might not be the same one.

But that's not how I do it. I 'git fetch' _before_ the review, downloading a copy of the candidate branch to a checkout on my own machine. Then I review the patch in gitk, probably compile and test it too (after at least one eyeball review pass), and if I'm happy, merge it. And all three of those steps refer to the same version of the patch, because I only ran 'git fetch' once (unless the submitter notified me to go and check back for more changes).

@nrbray I've only heard about jj in a few recent Mastodon posts, and haven't got round to actually looking at it. So I don't (yet?) have an answer to that one!

Someone in email mentioned "Fossil SCM" to me as another possibility. I don't know anything about that one either, except that my correspondent said that it was not based on git, so it would be a large change on that score.

(I vaguely understand that jj has at least _some_ compatibility with git? But that really is about all I know.)

except format-patch gets applied with git am and so doesn’t automatically give you the right parent commit (without manual fudging around)

no, the reason I want format-patch in .tgz when sent at all is that MUAs and MLs trash at least line endings, if not whitespace or even linewrap; this is not in favour or against format-patch, just saying that format-patch either inline (including send-email) or as plain attachment, is not robust enough in practice

@mirabilos indeed, I didn't mean to say that format-patch + tar + gzip has _all_ the same advantages as a git bundle. Only some of them. I agree that identifying the parent commit is one of the best things about the bundle strategy. (And the "just send a link to a clone repo" approach.)

Identifying the right revision to apply a patch against was even more annoying in the SVN days. I never quite did it, but I always wondered about writing a script that would check out lots of different revisions of my repo, test-apply the patch against each one, and report which one had the fewest conflicts!

yes, pretty much, except often I don't even need to do the diff, because it's fresh enough in my mind anyway.

And I always like to re-read the final version of the whole patch _without_ the context of all the older versions, to make sure it makes sense on its own merits. That helps me spot a missing part of a last-minute revision, like we changed a line of the code but accidentally left in a comment referring to the original version of that line.

This email-based review flow makes it harder to do line-by-line nitpicks in the style that Github and the like make easy, attaching every comment to a specific part of the patch. But that suits me, because I don't really like to be the kind of code reviewer who leaves 73 comments demanding tiny edits. It's quicker to make all the same changes myself than type out sentences saying why I wanted them, and avoids another round trip to the submitter anyway. So I'll normally only ask for _big_ changes, especially if they need re-testing in the submitter's environment, and then I'll sweep up the tiny nitpicks myself.

The real reason why I like an email per patch, though, is because I feel it works really well for review and design discussions - just reply and keep the quote for the relevant parts of the patch for context (in whatever order makes the most sense for making the point). This is how email was meant to be used, so it's no surprise that it works well.

I suppose for both your and my approach, using a mailing list would be helpful, and solve the problems with missing public visibility?

@caoimhin I'm not sure I see the connection to one email per patch. If I receive a submission of a whole patch series, I might very well want to quote parts of more than one of the commits in a review comment, to point out how they don't match each other. "In patch 2 you document the new function as follows: [quoted diff] But in patch 5 you call it with these arguments here: [diff] which don't match what the docs say you should do."

I think it's just as easy to paste fragments of patch from gitk into my email composer, and doesn't limit me to replying to only one patch at a time!

But I take your point about handling git-send-email messages being easier if you set up some automation for it. I don't know if my MUA would make that easy to set up (but then I probably ought to get round to replacing it anyway).

True, you already get many of the advantages even with a single email containing all patches. So that part is mostly an argument for plain-text patches in email.

I like individual mails because that keeps things in more managable pieces for me. I can reply to one patch and leave the other one marked as unread for another time. It also keeps the discussion more focussed on the individual incremental steps - after all, the theory is that each logical change is one patch.