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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Mar 30, 2022, 21:09 UTC
Message-ID: <220330.86o81nta6k.gmgdl@evledraar.gmail.com>
In-Reply-To: <xmqqmth76use.fsf@gitster.g>

On Wed, Mar 30 2022, Junio C Hamano wrote:

Show 27 quoted lines

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> 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.

I know, and I didn't mean to imply that you were standing in the way of C99 progress or anything like that.

Personally, I much prefer not to have //-style comments introduced. I merely mentioned the portability concern because I thought it helped give a relatively new contributor some context.

I.e. that despite any new developments on the C99 front they might discover this particular thing is stylistic and not about portability (although that may also have been a reason in the past).

Show 25 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.
>
>>> +				 "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.

I'm referring to the "80 characters per line", but omitted the \n, but yeah, I should have just said 80.