From: Kaartic Sivaraam Date: Sat, 16 Dec 2017 05:38:26 GMT Subject: Re: [PATCH v4 2/2] Doc/check-ref-format: clarify information about @{-N} syntax Message-ID: <641984aa-40b1-fde1-cafd-af109fa486bb@gmail.com> In-Reply-To: On Thursday 14 December 2017 11:32 PM, Junio C Hamano wrote: > Kaartic Sivaraam 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. > > 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). > 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.