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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>>>>> +				 "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.
>
> No, what I meant was that you do not want the rule to be to cut *AT*
> exactly the column whatever random rule specifies, which would
> result in funny wrapping in the middle of the word, e.g.
>
>         "To support setting up tracking branches, ensure that diff"
>         "erent remotes' fetch refspecs map into different tracking"
>         " namespaces."
>
> and "at 79, then wrap" somehow sounded to me like that.  I do not
> think you meant to imply that (instead, I think you meant to suggest
> "wrap the line so that the string constant is not longer than 79
> columns"), but it risks to be mistaken by new contributors.
>
> FWIW, I'd actually prefer to see both the sources to be readable by
> wrapping to keep the source code line length under certain limit
> (the current guideline being 70-something), counting the leading
> indentation, and at the same time keep it possible and easy to grep
> messages in the source.
>
> That requires us to notice when our code has too deeply nested,
> resulting in overly indented lines, and maintain the readability
> (refatoring the code may be a way to help in such cases).

Yes, I didn't mean to say it was a hard rule. In particular as this code
has the strings themselves over 80 characters. It can be good to break
that guideline when it doesn't help readability.

I just meant that this made sense as a fix-up, in this case:

diff --git a/branch.c b/branch.c
index 5c28d432103..4ccf5f79e83 100644
--- a/branch.c
+++ b/branch.c
@@ -282,10 +282,8 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,
 				 "\n"
 				 "To support setting up tracking branches, ensure that\n"
 				 "different remotes' fetch refspecs map into different\n"
-				 "tracking namespaces."),
-			       orig_ref,
-			       ftb_cb.remotes_advice.buf
-			       );
+				 "tracking namespaces."), orig_ref,
+			       ftb_cb.remotes_advice.buf);
 		exit(status);
 	}
 

Which I'd also be tempted to do as:

diff --git a/branch.c b/branch.c
index 5c28d432103..b9f6fda980b 100644
--- a/branch.c
+++ b/branch.c
@@ -283,9 +283,7 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,
 				 "To support setting up tracking branches, ensure that\n"
 				 "different remotes' fetch refspecs map into different\n"
 				 "tracking namespaces."),
-			       orig_ref,
-			       ftb_cb.remotes_advice.buf
-			       );
+			       orig_ref, ftb_cb.remotes_advice.buf);
 		exit(status);
 	}
 
But I find it generally helpful to do it consistently when possible, as
when running into merge conflicts it makes things easier.