Re: [PATCH v5] tracking branches: add advice to ambiguous refspec error

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 30, 2022, 20:37 UTC
Message-ID: <xmqqmth76use.fsf@gitster.g>
In-Reply-To: <220330.864k3fpo32.gmgdl@evledraar.gmail.com>

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

Show 8 quoted lines

> On Wed, Mar 30 2022, Tao Klerks via GitGitGadget wrote:
>
>> From: Tao Klerks <tao@klerks.biz>
>
>> +		case 2:
>> +			// there are at least two remotes; backfill the first one
>
> Nit: I think it's been Junio's preference not to introduce C99 comments,

I often mention the rule to new contributors, simply because it has been that way in our code base, regardless of what my personal preference might be, and sticking to the style will be more consistent. It's not like I am forcing my personal preference on developers. Do not mislead new people into thinking so. It is especially irritating to see ...

> despite other C99 features now being used (and I think it should work in
> practice as far as portability goes, see
> https://lore.kernel.org/git/87wnmwpwyf.fsf@evledraar.gmail.com/)

... a mention like this, when you know that it is not about portability but is about consistency, and also you know I've mentioned more than once on the list if we want to loosen some written CodingGuidelines rules, especially those that tools do not necessarily catch.

Show 7 quoted lines

>> +	if (tracking.matches > 1) {
>> +		int status = die_message(_("not tracking: ambiguous information for ref %s"),
>> +					    orig_ref);
>
> This isn't per-se new, but I wonder if while we're at it we shold just
> quote '%s' here, which we'd usually do. I.e. this message isn't new, but
> referring again to "ref %s" (and not "ref '%s'") below is.

Good.

Show 11 quoted lines

>> +				 "To support setting up tracking branches, ensure that\n"
>> +				 "different remotes' fetch refspecs map into different\n"
>> +				 "tracking namespaces."),
>> +			       orig_ref,
>> +			       remotes_advice.buf
>> +			       );
>
> Nit: The usual style for multi-line arguments is to "fill" lines until
> you're at 79 characters, so these last three lines (including the ");")
> can all go on the "tracking namespaces" line (until they're at 79, then
> wrap)>

I didn't know about the magic "79" number. It makes the resulting source code extremely hard to read, though, while making it easier to grep for specific messages.