Re: [PATCH] branch: rework the descriptions of rename and copy operations

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 15, 2024, 20:41 UTC
Message-ID: <xmqqedddo3ym.fsf@gitster.g>
In-Reply-To: <f9ec6d31-7158-4381-9701-06fcb33f2e83@app.fastmail.com>

"Kristoffer Haugsbakk" <code@khaugsbakk.name> writes:

Show 16 quoted lines

> (If I read this part correctly.)
>
>> --m::
>> ---move::
>> -	Move/rename a branch, together with its config and reflog.
>> +-m [<oldbranch>] <newbranch>::
>> +--move [<oldbranch>] <newbranch>::
>> +	Rename an existing branch <oldbranch>, which if not specified defaults
>> +	to the current branch, to <newbranch>.  The configuration variables
>
> I had to read the first sentence a few times in order to understand what
> the “which” part was saying (which seems to come from [1] by Junio). How
> about letting it trail the sentence?
>
>   « Rename an existing branch `<oldbranch>` to `<newbranch>`, with
>     `<oldbranch>` defaulting to the current branch if not specified.

Yeah, insertion of "if not specified" in the middle made the resulting text unnecessarily hard to follow, and moving it to the end is a fine fix. But I think we can just drop it (in other words, we already said "defaulting to" and that should be sufficient).

Show 9 quoted lines

>> +	for the <oldbranch> branch and its reflog are also renamed appropriately
>> +	to be used with <newbranch>.  Renaming fails if branch <newbranch>
>> +	already exists, but you can use `-M` or `--move --force` to overwrite
>> +	the files in existing branch <newbranch> while renaming.
>
> “the files” refers to the branch configuration and the reflog? People
> who read the man pages might not know that the reflogs are implemented
> as files and get tripped up. Maybe “to overwrite” could be left
> unstated?

Yeah, "overwrite the files in existing branch" can be mislead in other ways, including "in a similar way that 'checkout -f <other>' can overwrite the files with local modifications in the working tree with the <other> branch we are switching to". We'd be better off not mentioning files at all [*].

    ... but you can use ... to clobber an existing `<newbranch>`.

would be sufficient. If the verb "clobber" is unfamiliar to some readers, "overwrite" is also fine. The most important part from the end-user's point of view is that whatever data associated with the name <newbranch> is now gone, and replaced by what was associated with the name <oldbranch>. How we stored such data associated with each branch is immaterial.

	Side note: Especially because we are not married to the
	files backend that stores one file per branch under
	.git/logs/refs/heads/ and .git/refs/heads/ directories
	forever.  With reftable backend, there is no such files
	specific to <newbranch> to be overwritten.

Another thing. The existence of a <newbranch> is not the only case we fail "git branch -m [<oldbranch>] <newbranch>", but the mention of this particular safety valve is to tell readers that forcing with `-M` is how to override the safety. We want to be absolutely clear that we are not trying to exhaustively enumerate failure modes [*] to our readers, and I am not sure if we succeeded in the proposed text.

	Side note: In other words, there are other ways the command
	can fail, and `-M` may not be a way to "fix" these failures.

Show 6 quoted lines

> This patch also drops this part. Shouldn’t this be noted?
>
>   « , and a reflog entry is created to remember the branch renaming.
>
> Right now it looks like (reads like) the reflog is moved and an entry is
> not made about it.

True. This was missing in my "or something like that" illustration and the patch copies without checking the original. It should be resurrected.

Thanks, both for writing and carefully reading. Removal of <newbranch> and <oldbranch> from the OPTIONS enumaration is really good, too.