From: Philip Oakley <philipoakley@iee.email>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH 1/2] doc: fixup/squash: clarify use of <oid-hash> in subject line
Date: Fri, 29 May 2020 12:41:01 +0100 [thread overview]
Message-ID: <a2ef3923-f3d5-d0cc-61b2-326adaf4ef6b@iee.email> (raw)
In-Reply-To: <xmqqmu5tl1qa.fsf@gitster.c.googlers.com>
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"
next prev parent reply other threads:[~2020-05-29 11:41 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-04 20:40 [PATCH] rebase --autosquash: fix a potential segfault Johannes Schindelin via GitGitGadget
2020-05-04 21:19 ` Junio C Hamano
2020-05-04 21:33 ` Jeff King
2020-05-04 22:09 ` Taylor Blau
2020-05-05 20:30 ` Junio C Hamano
2020-05-06 21:35 ` Johannes Schindelin
2020-05-07 19:17 ` Jeff King
2020-05-08 23:45 ` Johannes Schindelin
2020-05-05 22:33 ` [PATCH v2] " Johannes Schindelin via GitGitGadget
2020-05-09 19:23 ` [PATCH v3] " Johannes Schindelin via GitGitGadget
2020-05-06 15:12 ` [PATCH] " Andrei Rybak
2020-05-07 14:27 ` Johannes Schindelin
2020-05-08 16:43 ` Philip Oakley
2020-05-08 16:57 ` Andrei Rybak
2020-05-08 17:21 ` Philip Oakley
2020-05-18 16:47 ` Philip Oakley
2020-05-18 3:27 ` Johannes Schindelin
2020-05-25 17:29 ` Philip Oakley
2020-05-25 21:36 ` [PATCH 0/2] Clarify some of the fixup! documenation Philip Oakley
2020-05-25 21:36 ` [PATCH 1/2] doc: fixup/squash: clarify use of <oid-hash> in subject line Philip Oakley
2020-05-27 17:35 ` Junio C Hamano
2020-05-29 11:41 ` Philip Oakley [this message]
2020-05-25 21:36 ` [PATCH 2/2] doc: fixup/squash: remove ellipsis marks, use <line> for clarify Philip Oakley
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=a2ef3923-f3d5-d0cc-61b2-326adaf4ef6b@iee.email \
--to=philipoakley@iee.email \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).