Re: [PATCH 1/2] doc: fixup/squash: clarify use of <oid-hash> in subject line

[Junio C Hamano <gitster@pobox.com>]

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.