Re: updating merge request

[Lukas-Fabian Moser]

Am 20.01.23 um 07:38 schrieb Aaron Hill:
> On 2023-01-19 8:54 pm, David Zelinsky wrote:
> > Am I misunderstanding something?  Or is what it says in 3.2 a mistake?
>
> If GitLab works similar to GitHub (which I have more experience with), 
> any new commits to your branch should automatically be picked up as 
> part of the open merge/pull request.  When accepting the request, 
> there is an option to rebase or squash, although some project owners 
> will ask that the original submitter manually do this before 
> accepting, but again this is all being done in the same branch of the 
> pending request.
>
> I think 3.2 might only be advising against creating a *new* branch and 
> submitting another request.  But the --amend instruction seems odd as 
> that is usually about correcting an incomplete commit, for instance 
> when a file that was supposed to be staged was missed. It looks clunky 
> to have a second commit when you can often get away with fixing up the 
> original commit to be complete. Otherwise, you are basically rewriting 
> history, and that is a general no-no.  One should let all commits sit 
> clearly in the open during review and then at the end squash or rebase 
> or whatever before merging based on how one prefers the main line to 
> look.

Accepted practice in lilypond development is different. We all rewrite 
history in our local brands for the merge requests frequently and use 
push --force(-with-lease). This way, the merge request is always "up to 
date" without needing squashing. Gitlab itself has the feature of 
keeping track of the various versions of a merge requests and letting us 
browse them conveniently. This way, we never have a state of a merge 
request that contains commits of the form
- feature X
- correct typo in feature X
- fix stupid mistake in feature X
- revert to previous state
etc., but (rewriting history) different versions of a merge request that 
always contains
- feature X [version 1, 2, 3, ...]

So, David: git commit --amend and force-pushing are fine.

(Personally, I use two local branches for developing a feature: one in 
which I keep track of my progress by adding commit after commit, and one 
which is the "squashed" version that I update with amend/squash and use 
for pushing. git's interactive rebase feature is my best friend here. 
But there might be better methods.)

Note that it's not strictly necessary that a merge request consists of 
one commit only: Logically independent "smallish" commits are fine 
(possible example: one commit for the feature, one for the larger 
documentation revision the feature necessitates). But always make sure 
that each single commit gives a "working" version of LilyPond in order 
to allow for bisecting later, which necessarily steps to "arbitrary" 
commits in the history. Example for that:

commit 23cfed4c16770d85f4709275f1c78ff138c37262
Author: Jean Abou Samra <jean@abou-samra.fr>
Date:   Wed Jan 4 22:10:26 2023 +0100

    Assume all colors have a transparency value in output backends

    The previous commit makes (color ...) stencil expressions always 
contain
    colors with a transparency value (4 elements, not 3). Therefore,
    interpret_stencil_expression can stop differentiating between the
    3-element and 4-element cases, as well as all three output backends.

commit 4e3ad954d0a20d302045afe8fee856f77a75667d
Author: Jean Abou Samra <jean@abou-samra.fr>
Date:   Wed Jan 4 22:03:26 2023 +0100

    Refactor colors

    Add a normalize-color Scheme function that takes a color in any style
    (color list, CSS) and returns an (r g b a) list.  Reimplement
    ly:stencil-in-color and stencil-with-color in terms of normalize-color.

    Having normalize-color will enable background handling for PNG 
images in
    the PS backend, where the 'pngfile stencil expression will need to
    include a color.

    The code is moved to Scheme because I got tired of doing complicated
    type checks in C++.

(I didn't check if both commits actually come from the same merge 
request, but they absolutely might.)

Lukas