[PATCH] branch: clarify <oldbranch> and <newbranch> terms further

[PATCH] branch: clarify <oldbranch> and <newbranch> terms further

[Dragan Simic]

Clarify further the uses for <oldbranch> and describe the additional use
for <newbranch>.  Mentioning both renaming and copying in these places might
seem a bit redundant, but it should actually make understanding these terms
easier to the readers of the git-branch(1) man page.

Signed-off-by: Dragan Simic <dsimic@manjaro.org>
---
 Documentation/git-branch.txt | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index 0b0844293235..7392c2f0797d 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -312,12 +312,14 @@ superproject's "origin/main", but tracks the submodule's "origin/main".
 	option is omitted, the current HEAD will be used instead.
 
 <oldbranch>::
-	The name of an existing branch.  If this option is omitted,
-	the name of the current branch will be used instead.
+	The name of an existing branch to be renamed or copied.
+	If this option is omitted, the name of the current branch
+	will be used instead.
 
 <newbranch>::
-	The new name for an existing branch. The same restrictions as for
-	<branchname> apply.
+	The new name for an existing branch, when renaming a branch,
+	or the name for a new branch, when copying a branch.  The same
+	naming restrictions apply as for <branchname>.
 
 --sort=<key>::
 	Sort based on the key given. Prefix `-` to sort in descending

Re: [PATCH] branch: clarify <oldbranch> and <newbranch> terms further

[Kyle Lippincott]

On Mon, Feb 5, 2024 at 4:45 AM Dragan Simic <dsimic@manjaro.org> wrote:
>
> Clarify further the uses for <oldbranch> and describe the additional use
> for <newbranch>.  Mentioning both renaming and copying in these places might
> seem a bit redundant, but it should actually make understanding these terms
> easier to the readers of the git-branch(1) man page.
>
> Signed-off-by: Dragan Simic <dsimic@manjaro.org>
> ---
>  Documentation/git-branch.txt | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
> index 0b0844293235..7392c2f0797d 100644
> --- a/Documentation/git-branch.txt
> +++ b/Documentation/git-branch.txt
> @@ -312,12 +312,14 @@ superproject's "origin/main", but tracks the submodule's "origin/main".
>         option is omitted, the current HEAD will be used instead.
>
>  <oldbranch>::
> -       The name of an existing branch.  If this option is omitted,
> -       the name of the current branch will be used instead.
> +       The name of an existing branch to be renamed or copied.
> +       If this option is omitted, the name of the current branch
> +       will be used instead.
>
>  <newbranch>::
> -       The new name for an existing branch. The same restrictions as for
> -       <branchname> apply.
> +       The new name for an existing branch, when renaming a branch,
> +       or the name for a new branch, when copying a branch.  The same
> +       naming restrictions apply as for <branchname>.

The precision here makes me worry that I'm potentially missing
something when reading this, and has made me re-read it multiple times
to try to figure out what it is.

I think this would be cleaner:

The name to give the branch created by the rename or copy operation.
The operation fails if <newbranch> already exists, use --force to ignore
this error. The same naming restrictions apply as for <branchname>.

I'm not super pleased with that second sentence, and maybe we
shouldn't include it here. Maybe it belongs on the documentation for
--move and --copy instead? It's sort of mentioned in the text at the
top describing the -m/-M and -c/-C options, though it's not clear from
that text what actually happens to the existing copy of <newbranch> if
one uses --force. If we could include a better description of what
happens to the existing branch when one uses --force, that'd be nice.

>
>  --sort=<key>::
>         Sort based on the key given. Prefix `-` to sort in descending
>

Re: [PATCH] branch: clarify <oldbranch> and <newbranch> terms further

[Junio C Hamano]

Kyle Lippincott <spectral@google.com> writes:

> I'm not super pleased with that second sentence, and maybe we
> shouldn't include it here. Maybe it belongs on the documentation for
> --move and --copy instead? It's sort of mentioned in the text at the
> top describing the -m/-M and -c/-C options, though it's not clear from
> that text what actually happens to the existing copy of <newbranch> if
> one uses --force. If we could include a better description of what
> happens to the existing branch when one uses --force, that'd be nice.

My preference is to limit the "OPTIONS" section to dashed options.
If "--move" takes one or two arguments, update its description to
talk about how these one or two arguments are used, perhaps like

	-m [<oldbranch>] <newbranch>::
	--move [<oldbranch>] <newbranch>::

		Rename an existing branch <oldbranch>, which
                defaults to the current branch, to <newbranch>.  The
                configuration variables about and the reflog of
                <oldbranch> are also renamed appropriately to be
                used with <newbranch>. It is an error if <newbranch>
                exists (you can use `--force` to clobber an existing
                <newbranch>).

or something like that.

Listing non-options in the list may have been a misguided attempt to
"save" description on arguments that are common to multiple options,
but it is not working.  We can see the bad effect of that approach
only by looking at the current description of the above option,
which reads:

	-m::
	--move::
		Move/rename a branch, together with its config and reflog.

It does not mentioning what arguments "--move" takes, and does not
even refer the readers to the entries for <newbranch> and
<oldbranch>, so the only plausible way the users can learn what they
want about this single option is by reading the page from top to
bottom.

And trim the DESCRIPTION part. A lot. Because things are explained
redundantly between there and the OPTIONS part, and their details
are waiting to drift apart unless we are careful.

I think I laid all this out and more in a separate message.

https://lore.kernel.org/git/xmqq8r4zln08.fsf@gitster.g/

Re: [PATCH] branch: clarify <oldbranch> and <newbranch> terms further

[Dragan Simic]

Hello Junio,

On 2024-02-06 01:09, Junio C Hamano wrote:
> Kyle Lippincott <spectral@google.com> writes:
> 
>> I'm not super pleased with that second sentence, and maybe we
>> shouldn't include it here. Maybe it belongs on the documentation for
>> --move and --copy instead? It's sort of mentioned in the text at the
>> top describing the -m/-M and -c/-C options, though it's not clear from
>> that text what actually happens to the existing copy of <newbranch> if
>> one uses --force. If we could include a better description of what
>> happens to the existing branch when one uses --force, that'd be nice.
> 
> My preference is to limit the "OPTIONS" section to dashed options.
> If "--move" takes one or two arguments, update its description to
> talk about how these one or two arguments are used, perhaps like
> 
> 	-m [<oldbranch>] <newbranch>::
> 	--move [<oldbranch>] <newbranch>::
> 
> 		Rename an existing branch <oldbranch>, which
>                 defaults to the current branch, to <newbranch>.  The
>                 configuration variables about and the reflog of
>                 <oldbranch> are also renamed appropriately to be
>                 used with <newbranch>. It is an error if <newbranch>
>                 exists (you can use `--force` to clobber an existing
>                 <newbranch>).
> 
> or something like that.

Thank you for your detailed feedback!

I like it and I fully agree that describing the operation arguments
fits and flows much better in the descriptions of their respective
operations.  Describing the outcome of forced operations is also
needed for completeness, and for safety.

I'll prepare and send a v2 that takes that approach.

> Listing non-options in the list may have been a misguided attempt to
> "save" description on arguments that are common to multiple options,
> but it is not working.  We can see the bad effect of that approach
> only by looking at the current description of the above option,
> which reads:
> 
> 	-m::
> 	--move::
> 		Move/rename a branch, together with its config and reflog.
> 
> It does not mentioning what arguments "--move" takes, and does not
> even refer the readers to the entries for <newbranch> and
> <oldbranch>, so the only plausible way the users can learn what they
> want about this single option is by reading the page from top to
> bottom.

... or the users can perhaps learn by simply experimenting a bit
and observing what happens, after getting a bit disappointed by the
current descriptions of the operations and resorting to the rather
usual "tl;dr" approach.

Avoiding such "tl;dr" scenarios is the way to move forward with the
improvements to the git man pages, if you agree.

> And trim the DESCRIPTION part. A lot. Because things are explained
> redundantly between there and the OPTIONS part, and their details
> are waiting to drift apart unless we are careful.
> 
> I think I laid all this out and more in a separate message.
> 
> https://lore.kernel.org/git/xmqq8r4zln08.fsf@gitster.g/

I agree about this as well, but that will perhaps be handled in some
separate patch for the git-branch(1) man page.

Re: [PATCH] branch: clarify <oldbranch> and <newbranch> terms further

[Junio C Hamano]

Dragan Simic <dsimic@manjaro.org> writes:

>> I think I laid all this out and more in a separate message.
>> https://lore.kernel.org/git/xmqq8r4zln08.fsf@gitster.g/
>
> I agree about this as well, but that will perhaps be handled in some
> separate patch for the git-branch(1) man page.

If you truly agree with the longer term plan, which includes removal
of the bullet points your patch updates, then reviewing further on
that patch becomes a waste of time in the larger picture, doesn't
it?

"Will be deferred to some later time, let's take this small update
as-is" has already been said back then.  Let's not do that again
this time.

Thanks.

Re: [PATCH] branch: clarify <oldbranch> and <newbranch> terms further

[Dragan Simic]

On 2024-02-06 19:28, Junio C Hamano wrote:
> Dragan Simic <dsimic@manjaro.org> writes:
>>> I think I laid all this out and more in a separate message.
>>> https://lore.kernel.org/git/xmqq8r4zln08.fsf@gitster.g/
>> 
>> I agree about this as well, but that will perhaps be handled in some
>> separate patch for the git-branch(1) man page.
> 
> If you truly agree with the longer term plan, which includes removal
> of the bullet points your patch updates, then reviewing further on
> that patch becomes a waste of time in the larger picture, doesn't
> it?

Hmm, it doesn't seem like a waste of time and effort to me, because
the first patch would move/add the descriptions of the <oldbranch>
and <newbranch> arguments to the descriptions of the branch copy and
rename operations in the "Options" section.

This needs to be done anyway, if you agree.  The following patch would
either dissolve as many sentences from the "Description" section into
the "Options" section, or have those sentences converted into some
kind of more readable prose.

I hope you agree.

> "Will be deferred to some later time, let's take this small update
> as-is" has already been said back then.  Let's not do that again
> this time.

Oh, I've also heard that too many times, and I also know where such
an approach usually leads.  Nowhere. :)

Re: [PATCH] branch: clarify <oldbranch> and <newbranch> terms further

[Dragan Simic]

Hello Kyle,

On 2024-02-06 00:53, Kyle Lippincott wrote:
> On Mon, Feb 5, 2024 at 4:45 AM Dragan Simic <dsimic@manjaro.org> wrote:
>> 
>> Clarify further the uses for <oldbranch> and describe the additional 
>> use
>> for <newbranch>.  Mentioning both renaming and copying in these places 
>> might
>> seem a bit redundant, but it should actually make understanding these 
>> terms
>> easier to the readers of the git-branch(1) man page.
>> 
>> Signed-off-by: Dragan Simic <dsimic@manjaro.org>
>> ---
>>  Documentation/git-branch.txt | 10 ++++++----
>>  1 file changed, 6 insertions(+), 4 deletions(-)
>> 
>> diff --git a/Documentation/git-branch.txt 
>> b/Documentation/git-branch.txt
>> index 0b0844293235..7392c2f0797d 100644
>> --- a/Documentation/git-branch.txt
>> +++ b/Documentation/git-branch.txt
>> @@ -312,12 +312,14 @@ superproject's "origin/main", but tracks the 
>> submodule's "origin/main".
>>         option is omitted, the current HEAD will be used instead.
>> 
>>  <oldbranch>::
>> -       The name of an existing branch.  If this option is omitted,
>> -       the name of the current branch will be used instead.
>> +       The name of an existing branch to be renamed or copied.
>> +       If this option is omitted, the name of the current branch
>> +       will be used instead.
>> 
>>  <newbranch>::
>> -       The new name for an existing branch. The same restrictions as 
>> for
>> -       <branchname> apply.
>> +       The new name for an existing branch, when renaming a branch,
>> +       or the name for a new branch, when copying a branch.  The same
>> +       naming restrictions apply as for <branchname>.
> 
> The precision here makes me worry that I'm potentially missing
> something when reading this, and has made me re-read it multiple times
> to try to figure out what it is.

Thank you for your feedback!

I'd agree that the first sentence I sent isn't exactly a textbook
example of clarity, :) but it also isn't that bad;  it tries to say
something like "one thing when this, other thing when that".

> I think this would be cleaner:
> 
> The name to give the branch created by the rename or copy operation.
> The operation fails if <newbranch> already exists, use --force to 
> ignore
> this error. The same naming restrictions apply as for <branchname>.
> 
> I'm not super pleased with that second sentence, and maybe we
> shouldn't include it here. Maybe it belongs on the documentation for
> --move and --copy instead? It's sort of mentioned in the text at the
> top describing the -m/-M and -c/-C options, though it's not clear from
> that text what actually happens to the existing copy of <newbranch> if
> one uses --force. If we could include a better description of what
> happens to the existing branch when one uses --force, that'd be nice.

I agree that moving everything to the descriptions of the move and
copy operations, as Junio described further in his message, is a much
better way moving forward.  Describing the results of forced operation
is also needed for completeness.

I'll prepare and send a v2 that takes that approach.

>> 
>>  --sort=<key>::
>>         Sort based on the key given. Prefix `-` to sort in descending
>>