Re: updating merge request

[Jean Abou Samra]

Le 20/01/2023 à 15:03, Aaron Hill a écrit :
> On 2023-01-20 3:22 am, Jean Abou Samra wrote:
> > Rewriting history is just something that one might not be used to coming
> > from other projects, but perfectly fine for our purposes.
>
> It was impressed upon me to treat rewriting history as fraught with 
> peril, potentially even Bad(tm) in the Ghostbusters sense:
>
>     "Try to imagine all life as you know it stopping instantaneously and
>      every molecule in your body exploding at the speed of light." -- 
> Egon Spengler
>
> One might argue that anything in your development branch is not really 
> part of "history" yet as it has not been accepted.  (The situation 
> gets murkier when people are forking forks.)  As such, it *should* be 
> safe to mess about with commits to clean up typos or catch missing 
> files, what have you.  And I would agree this results in a cleaner 
> submission that is easier to review for correctness.  (I probably 
> should clarify my earlier comments that I do not intentionally make my 
> commits messy, just that I usually do not stress about them being so 
> absolutely pristine; again, I am used to work being squashed, so any 
> niceness I put in there gets lost.)



Indeed, overwriting history is Very Bad™ for shared branches, but not really
a problem for development branches as long as a single developer edits
them.



> So I can see --amend being useful for the little things.  But let's 
> say during a review, it is determined that the scope could increase to 
> cover more items than originally planned but that still feels part of 
> the same submission.  (Anything too distinct really should be an 
> independent request.)  Now, you might not necessarily want to force 
> all of the new development work into the existing commit. Reviewers 
> might even appreciate seeing the individual slices of the task more 
> cleanly delineated.  In a sense, there is some "history" to the 
> process that might be worth preserving during this stage.




Yes, exactly! And that is what our process is designed to facilitate :)

If there are two commits in the MR

Commit A: add XYZ
Commit B: take advantage of XYZ to better reimplement QRST

and you want to modify XYZ to address reviewers' suggestions, the more 
"usual"
workflow is adding a new commit on top, yielding

Commit A: add XYZ
Commit B: take advantage of XYZ to better reimplement QRST
Commit C: fix typo in XYZ

whereas in our process, you "rebase -i" to get

Commit A: add XYZ   [amended version with a different commit ID]
Commit B: take advantage of XYZ to better reimplement QRST  [also a new 
commit ID]

and

* the next reviewer who looks at the MR sees only the logical structure,
  not cluttered by the fixes,

* everything can be cleanly integrated into master as-is, without squashing
  A and B (which you would need if you wanted C not to be separate from
  A, due to the ordering).


IOW: it may seem counterintuitive, but our workflow that involves editing
commits in-place instead of adding more commits is precisely aimed at
allowing more structure between the commits, by making the history
of the branch reflect what you eventually want to end up in the repo,
a *logical* structure, rather than the temporal structure driven by
"I just happened to fix this before that".

Not that I am an out-and-out fan of this workflow, but its tradeoffs
have served us well so far, I would say.

While this may not be the most usual workflow with GitHub/GitLab,
it's not unheard of at all. E.g., the more traditional email-based
workflow (in Git itself, the Linux kernel, many GNU projects, etc.)
has the same logical separation of commits where you amend patches
even after making more patches on top of them.

https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html#separate-your-changes

OpenPGP_signature
Description: OpenPGP digital signature