| Message ID | 20200525213632.1626-2-philipoakley@iee.email (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Clarify some of the fixup! documenation | expand |
Philip Oakley <philipoakley@iee.email> writes: > The use of ellision `...` isn't great, as it gives no hint or clue, > leaving the subsequent test with a difficult explanation. True. If you are planning to correct it in 2/2, then I think it makes more sense to squash that in to have a single patch. > Clarify if a full oid has is required, or a unique abbreviation within > the respository, or just uniques within the rebase instruction? Puzzled. You must know the answer to "do we need a full object name, or is it sufficient to have anything that gives us a unique commit object name?" so why not write it in the patch instead of asking the question here? Or do you not know the answer and this is a RFC/WIP patch???? > This is a minimal change that sidesteps the chance to rewrite/clarify > the potential wider confusions over specifying the <commit> being > referred to in the fixup/squash process. Hmph. So this step cannot be reviewed to judge if it is a good change by itself? Let me locally recreate a squashed single patch and review _that_ instead. > Documentation/git-rebase.txt | 18 ++++++++++-------- > 1 file changed, 10 insertions(+), 8 deletions(-) > > diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt > index 4624cfd288..462cb4c52c 100644 > --- a/Documentation/git-rebase.txt > +++ b/Documentation/git-rebase.txt > @@ -571,16 +571,18 @@ See also INCOMPATIBLE OPTIONS below. > > --autosquash:: > --no-autosquash:: > - When the commit log message begins with "squash! ..." (or > - "fixup! ..."), and there is already a commit in the todo list that > - matches the same `...`, automatically modify the todo list of rebase > + When the commit log message begins with "squash! <line>" (or > + "fixup! <line>"), and there is already a commit in the todo list that > + matches the same `<line>`, automatically modify the todo list of rebase > -i so that the commit marked for squashing comes right after the > commit to be modified, and change the action of the moved commit > + from `pick` to `squash` (or `fixup`). > ++ > +A commit matches the `<line>` if > +the commit subject matches, or if the `<line>` refers to the commit's > +hash. As a fall-back, partial matches of the commit subject work, > +too. The recommended way to create fixup/squash commits is by using > +the `--fixup`/`--squash` options of linkgit:git-commit[1]. > + Overall it looks much better than the original. The original did not even attempt to define what is a "match" for the purpose of this option, so the ellipses may have been OK, but once we need to refer to what is there, we need a name to refer to it and ellipses no longer are sufficient, and using the step 1/2 alone would not make any sense. We definitely should take the step 2/2 together with it. "A commit matches the <line> if the commit subject matches" is not a great definition of what a "match" is, though. The readers are left in the same darkness about what constitutes a "match" of <line> against "the commit subject". If you define this "subject matches" as a substring match, for example, you do not even have to say "as a fall-back"---it is by (the updated version of your) definition that how the commit subject and <line> matches so there is no need to allow any fall-back involved.
On 27/05/2020 18:35, Junio C Hamano wrote: > Philip Oakley <philipoakley@iee.email> writes: > >> The use of ellision `...` isn't great, as it gives no hint or clue, >> leaving the subsequent test with a difficult explanation. > True. If you are planning to correct it in 2/2, then I think it > makes more sense to squash that in to have a single patch. OK >> Clarify if a full oid has is required, or a unique abbreviation within >> the respository, or just uniques within the rebase instruction? > Puzzled. You must know the answer to "do we need a full object > name, or is it sufficient to have anything that gives us a unique > commit object name?" so why not write it in the patch instead of > asking the question here? Or do you not know the answer and this is > a RFC/WIP patch???? This was a left over note about deeper questions outside of this patch series. > >> This is a minimal change that sidesteps the chance to rewrite/clarify >> the potential wider confusions over specifying the <commit> being >> referred to in the fixup/squash process. > Hmph. So this step cannot be reviewed to judge if it is a good > change by itself? I was working on 'small incremental steps' here. > > Let me locally recreate a squashed single patch and review _that_ > instead. > >> Documentation/git-rebase.txt | 18 ++++++++++-------- >> 1 file changed, 10 insertions(+), 8 deletions(-) >> >> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt >> index 4624cfd288..462cb4c52c 100644 >> --- a/Documentation/git-rebase.txt >> +++ b/Documentation/git-rebase.txt >> @@ -571,16 +571,18 @@ See also INCOMPATIBLE OPTIONS below. >> >> --autosquash:: >> --no-autosquash:: >> - When the commit log message begins with "squash! ..." (or >> - "fixup! ..."), and there is already a commit in the todo list that >> - matches the same `...`, automatically modify the todo list of rebase >> + When the commit log message begins with "squash! <line>" (or >> + "fixup! <line>"), and there is already a commit in the todo list that >> + matches the same `<line>`, automatically modify the todo list of rebase >> -i so that the commit marked for squashing comes right after the >> commit to be modified, and change the action of the moved commit >> + from `pick` to `squash` (or `fixup`). >> ++ >> +A commit matches the `<line>` if >> +the commit subject matches, or if the `<line>` refers to the commit's >> +hash. As a fall-back, partial matches of the commit subject work, >> +too. The recommended way to create fixup/squash commits is by using >> +the `--fixup`/`--squash` options of linkgit:git-commit[1]. >> + > Overall it looks much better than the original. > > The original did not even attempt to define what is a "match" for > the purpose of this option, so the ellipses may have been OK, but > once we need to refer to what is there, we need a name to refer to > it and ellipses no longer are sufficient, and using the step 1/2 > alone would not make any sense. We definitely should take the step > 2/2 together with it. I'd taken the idea of being able to name the thing as step 1, to get past the Newspeak problem. > > "A commit matches the <line> if the commit subject matches" is not a > great definition of what a "match" is, though. The readers are left > in the same darkness about what constitutes a "match" of <line> > against "the commit subject". If you define this "subject matches" > as a substring match, for example, you do not even have to say "as a > fall-back"---it is by (the updated version of your) definition that > how the commit subject and <line> matches so there is no need to > allow any fall-back involved. The fall back does include the commit hash, and (not yet in this series) is the extra information that Dscho provided at [1], so it's not a simple substring match, nor partial string match. Part of this reader confusion comes out of the `commit --fixup` option that effectively directs the reader away from using a hash, to using the target's full commit message for the fixup! line. At this stage, the aim is to make the option for the use of the commit hash a bit more visible within the text. Even after many years of reading, it still didn't stand out in the old 'wall of text', hence the all important paragraph break I'll combine the two patches at this stage. Philip [1] https://public-inbox.org/git/nycvar.QRO.7.76.6.2005180522230.55@tvgsbejvaqbjf.bet/ "It's even worse"
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt index f7a6033607..dfd3d6d0ef 100644 --- a/Documentation/git-rebase.txt +++ b/Documentation/git-rebase.txt @@ -539,11 +539,13 @@ See also INCOMPATIBLE OPTIONS below. matches the same `...`, automatically modify the todo list of rebase -i so that the commit marked for squashing comes right after the commit to be modified, and change the action of the moved commit - from `pick` to `squash` (or `fixup`). A commit matches the `...` if - the commit subject matches, or if the `...` refers to the commit's - hash. As a fall-back, partial matches of the commit subject work, - too. The recommended way to create fixup/squash commits is by using - the `--fixup`/`--squash` options of linkgit:git-commit[1]. + from `pick` to `squash` (or `fixup`). ++ +A commit matches the `...` if +the commit subject matches, or if the `...` refers to the commit's +hash. As a fall-back, partial matches of the commit subject work, +too. The recommended way to create fixup/squash commits is by using +the `--fixup`/`--squash` options of linkgit:git-commit[1]. + If the `--autosquash` option is enabled by default using the configuration variable `rebase.autoSquash`, this option can be
The option to use the oid hash is buried deep within the fixup/squash documenation. Split the paragraph so that the option choice is more obvious. Signed-off-by: Philip Oakley <philipoakley@iee.email> --- The use of ellision `...` isn't great, as it gives no hint or clue, leaving the subsequent test with a difficult explanation. Clarify if a full oid has is required, or a unique abbreviation within the respository, or just uniques within the rebase instruction? This is a minimal change that sidesteps the chance to rewrite/clarify the potential wider confusions over specifying the <commit> being referred to in the fixup/squash process. --- Documentation/git-rebase.txt | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)