top | item 40962076

(no title)

zeotroph | 1 year ago

How do you envision pushing/reviewing a branch of 3 cleanly separated commits (say: write failing test, fix code, refactor) into a repo with a fast-forward/no-merge policy? The end result should of course be 3 commits, but review history should not be lost.

1) push 3 commits (author)

2) push 3 commits + 1 review commit (reviewer)

or even 2a), + 3 interleaved review commits, might be needed if the final refactor removes something.

then

3a) force-push 3 commits with the review comments applied (you mentioned --force is supported)

3b) or push 3 + 1 + (worst case 3) fixup commits, then a squash later?

If 3a), does pico have the concept of patchsets like gerrit, so the state 2) can be retrieved later?

PS: ... or even 2c, if the line I am commenting on is removed in patch 2 I would need to push a whole "review tree" (or fix conflicts in b and c):

   a -- b -- c -- review-c
    \   \
     \   `-review-b
      `- review-a

discuss

order

qudat|1 year ago

Right now, when force pushing, we preserve event logs for every server mutation, but the review themselves are lost.

We do see this as a problem and are thinking about ways to address it. We do support the idea of a patchset (that's what the patch request managed). Right now we only support a single patchset for a patch request, but we are leaning toward introducing patch revisions where both contrib and owner submit entire versions of the patchset. This could provide better tracability since reviews would be in a separate patchset "channel" and not lost to force pushing.

We also plan on supporting a patch request "cover letter" which includes all the events and comments during review. This cover letter is just an empty patch which could be `git am --empty=keep` or `git am --empty=drop` depending on preference.

zeotroph|1 year ago

Good to hear, Gerrit does a lot of things right, see "stacked diffs" (or the realization that GitHub lacks these) now becoming more popular. And the patchset model is one important aspect of that, especially being able to see the difference just between patchsets - which git-pr can then do via `git range-diff` (which you already mentioned).

And even if everything git-pr does could be done by talking to the Gerrit REST-API, a simple straight forward and lightweight implementation of the "essence" of this model is very useful.

nektro|1 year ago

they should be done as separate patches