Re: [PATCH v5] tracking branches: add advice to ambiguous refspec error
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Mar 30, 2022, 22:51 UTC
- Message-ID
- <220331.86y20rrqzg.gmgdl@evledraar.gmail.com>
- In-Reply-To
- <xmqqzgl75c35.fsf@gitster.g>
On Wed, Mar 30 2022, Junio C Hamano wrote:
Show 43 quoted lines
> Æ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.