Re: [PATCH v4 2/2] Doc/check-ref-format: clarify information about @{-N} syntax

From: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
Date: Dec 16, 2017, 05:38 UTC
Message-ID: <641984aa-40b1-fde1-cafd-af109fa486bb@gmail.com>
In-Reply-To: <xmqqa7ylur1i.fsf@gitster.mtv.corp.google.com>

On Thursday 14 December 2017 11:32 PM, Junio C Hamano wrote:

Show 7 quoted lines

> Kaartic Sivaraam <kaartic.sivaraam@gmail.com> writes:
> 
> Looks alright.
> 
> It was made unnecessarily harder to review because it was marked as
> 2/2, even though this no longer applies on top of the copy of 1/2
> that was merged some time ago.

Sorry about that but I don't remember doing anything that made it not to apply on top of 1/2. (I just amended my changes to my topic branch. It can be found at [1])

> I needed to find that it was rebased
> on top of 'master';

I don't remember doing any rebase on top of 'master'. My topic was (and still is) based on the 'master' when it was pointing at 89ea799ff (Sync with maint, 2017-11-15). Anyways, it's my mistake as I didn't specify the branch on which I based this. Sorry about that.

Show 5 quoted lines

> 
> Also re-wrapping the lines only to squeeze in "but be cautious..."
> and replace s/branch/checkout/ in a few places did not help to make
> it easy to spot what's changed.
>

I expected this would happen but I thought the line shouldn't grew too much so that they have to re-wrapped. Seems it would have been better if I did the re-wrapping as a follow-up commit (didn't strike me then).

Show 8 quoted lines

> This part looked a bit strange.
> 
>> +it can be used as a valid branch name e.g. when creating a new branch
>> +(but be cautious when using the previous checkout syntax; it may refer
>> +to a detached HEAD state). The rule `git check-ref-format --branch
> 
> I think "e.g. when creating a new branch" is a parenthetical remark,
> so it should be inside parenthesis.

It was. I brought them out to introduce the parenthetical warning. I'll send a v5 by putting the remark back inside parantheses and bringing the warning out. If it's not ok, let me know. I'll also try to do the re-wrapping as a separate cleanup patch.

>  As the last three lines in the
> new text (quoted above) already warns that it may not be a branch name,
> I am not sure if the "but be cautious" adds much value, though.
>

That warning was for the impatient readers, who might want to find quick answers as to why they saw an odd behaviour (check-ref-froamt --branch not failing for a commit object name) (or) those who would want to use 'check-ref-format --branch' but do not find time to read the whole paragraph.