{"thread":{"id":"57591","subject":"[PATCH] tracking branches: add advice to ambiguous refspec error","startedAt":"2022-03-21T10:24:07Z","lastAt":"2022-04-01T19:57:42Z","messageCount":36,"participants":["Tao Klerks via GitGitGadget","Ævar Arnfjörð Bjarmason","Tao Klerks","Junio C Hamano","Glen Choo"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"451722","messageId":"pull.1183.git.1647858238144.gitgitgadget@gmail.com","threadId":"57591","inReplyTo":null,"subject":"[PATCH] tracking branches: add advice to ambiguous refspec error","fromName":"Tao Klerks via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-21T10:23:57Z","receivedAt":"2022-03-21T10:24:07Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"From: Tao Klerks <tao@klerks.biz>\n\nThe error \"not tracking: ambiguous information for ref\" is raised\nwhen we are evaluating what tracking information to set on a branch,\nand find that the ref to be added as tracking branch is mapped\nunder multiple remotes' fetch refspecs.\n\nThis can easily happen when a user copy-pastes a remote definition\nin their git config, and forgets to change the tracking path.\n\nAdd advice in this situation, explicitly highlighting which remotes\nare involved and suggesting how to correct the situation.\n\nSigned-off-by: Tao Klerks <tao@klerks.biz>\n---\n    RFC: tracking branches: add advice to ambiguous refspec error\n    \n    I am submitting this as an RFC because of a couple of things I'm not\n    sure about:\n    \n     1. In this proposed patch the advice is emitted before the existing\n        die(), in order to avoid changing the exact error behavior and only\n        add extra/new hint lines, but in other advise() calls I see the\n        error being emitted before the advise() hint. Given that error() and\n        die() display different message prefixes, I'm not sure whether it's\n        possible to follow the existing pattern and avoid changing the error\n        itself. Should I just accept that with the new advice, the error\n        message can and should change?\n     2. In order to include the names of the conflicting remotes I am\n        calling advise() multiple times - this is not a pattern I see\n        elsewhere - should I be building a single message and calling\n        advise() only once?\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1183%2FTaoK%2Fadvise-ambiguous-tracking-refspec-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1183/TaoK/advise-ambiguous-tracking-refspec-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1183\n\n advice.c |  1 +\n advice.h |  1 +\n branch.c | 27 ++++++++++++++++++++++++++-\n 3 files changed, 28 insertions(+), 1 deletion(-)\n\ndiff --git a/advice.c b/advice.c\nindex 1dfc91d1767..686612590ec 100644\n--- a/advice.c\n+++ b/advice.c\n@@ -39,6 +39,7 @@ static struct {\n \t[ADVICE_ADD_EMPTY_PATHSPEC]\t\t\t= { \"addEmptyPathspec\", 1 },\n \t[ADVICE_ADD_IGNORED_FILE]\t\t\t= { \"addIgnoredFile\", 1 },\n \t[ADVICE_AM_WORK_DIR] \t\t\t\t= { \"amWorkDir\", 1 },\n+\t[ADVICE_AMBIGUOUS_FETCH_REFSPEC]\t\t= { \"ambiguousFetchRefspec\", 1 },\n \t[ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME] \t= { \"checkoutAmbiguousRemoteBranchName\", 1 },\n \t[ADVICE_COMMIT_BEFORE_MERGE]\t\t\t= { \"commitBeforeMerge\", 1 },\n \t[ADVICE_DETACHED_HEAD]\t\t\t\t= { \"detachedHead\", 1 },\ndiff --git a/advice.h b/advice.h\nindex 601265fd107..3d68c1a6cb4 100644\n--- a/advice.h\n+++ b/advice.h\n@@ -17,6 +17,7 @@ struct string_list;\n \tADVICE_ADD_EMPTY_PATHSPEC,\n \tADVICE_ADD_IGNORED_FILE,\n \tADVICE_AM_WORK_DIR,\n+\tADVICE_AMBIGUOUS_FETCH_REFSPEC,\n \tADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME,\n \tADVICE_COMMIT_BEFORE_MERGE,\n \tADVICE_DETACHED_HEAD,\ndiff --git a/branch.c b/branch.c\nindex 5d20a2e8484..243f6d8b362 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -12,6 +12,7 @@\n struct tracking {\n \tstruct refspec_item spec;\n \tstruct string_list *srcs;\n+\tstruct string_list *remotes;\n \tconst char *remote;\n \tint matches;\n };\n@@ -28,6 +29,7 @@ static int find_tracked_branch(struct remote *remote, void *priv)\n \t\t\tfree(tracking->spec.src);\n \t\t\tstring_list_clear(tracking->srcs, 0);\n \t\t}\n+\t\tstring_list_append(tracking->remotes, remote->name);\n \t\ttracking->spec.src = NULL;\n \t}\n \n@@ -217,6 +219,18 @@ static int inherit_tracking(struct tracking *tracking, const char *orig_ref)\n \treturn 0;\n }\n \n+\n+static const char ambiguous_refspec_advice_pre[] =\n+N_(\"\\n\"\n+\"There are multiple remotes with fetch refspecs mapping to\\n\"\n+\"the tracking ref %s:\\n\";)\n+static const char ambiguous_refspec_advice_post[] =\n+N_(\"This is typically a configuration error.\\n\"\n+\"\\n\"\n+\"To support setting up tracking branches, ensure that\\n\"\n+\"different remotes' fetch refspecs map into different\\n\"\n+\"tracking namespaces.\\n\");\n+\n /*\n  * This is called when new_ref is branched off of orig_ref, and tries\n  * to infer the settings for branch.<new_ref>.{remote,merge} from the\n@@ -227,11 +241,14 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,\n {\n \tstruct tracking tracking;\n \tstruct string_list tracking_srcs = STRING_LIST_INIT_DUP;\n+\tstruct string_list tracking_remotes = STRING_LIST_INIT_DUP;\n \tint config_flags = quiet ? 0 : BRANCH_CONFIG_VERBOSE;\n+\tstruct string_list_item *item;\n \n \tmemset(&tracking, 0, sizeof(tracking));\n \ttracking.spec.dst = (char *)orig_ref;\n \ttracking.srcs = &tracking_srcs;\n+\ttracking.remotes = &tracking_remotes;\n \tif (track != BRANCH_TRACK_INHERIT)\n \t\tfor_each_remote(find_tracked_branch, &tracking);\n \telse if (inherit_tracking(&tracking, orig_ref))\n@@ -248,9 +265,17 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,\n \t\t\treturn;\n \t\t}\n \n-\tif (tracking.matches > 1)\n+\tif (tracking.matches > 1) {\n+\t\tif (advice_enabled(ADVICE_AMBIGUOUS_FETCH_REFSPEC)) {\n+\t\t\tadvise(_(ambiguous_refspec_advice_pre), orig_ref);\n+\t\t\tfor_each_string_list_item(item, &tracking_remotes) {\n+\t\t\t\tadvise(\"  %s\", item->string);\n+\t\t\t}\n+\t\t\tadvise(_(ambiguous_refspec_advice_post));\n+\t\t}\n \t\tdie(_(\"not tracking: ambiguous information for ref %s\"),\n \t\t    orig_ref);\n+\t}\n \n \tif (tracking.srcs->nr < 1)\n \t\tstring_list_append(tracking.srcs, orig_ref);\n\nbase-commit: 4c53a8c20f8984adb226293a3ffd7b88c3f4ac1a\n-- \ngitgitgadget\n"},{"id":"451727","messageId":"220321.86ee2v9xzd.gmgdl@evledraar.gmail.com","threadId":"57591","inReplyTo":"pull.1183.git.1647858238144.gitgitgadget@gmail.com","subject":"Re: [PATCH] tracking branches: add advice to ambiguous refspec error","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-03-21T14:11:46Z","receivedAt":"2022-03-21T14:34:14Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Mar 21 2022, Tao Klerks via GitGitGadget wrote:\n\nRe $subject (and you've got another outstanding patch on list that's the\nsame), please add \"RFC PATCH\" to the subject for RFC patches, doesn't\nGGG have some way to do that?\n\n>      1. In this proposed patch the advice is emitted before the existing\n>         die(), in order to avoid changing the exact error behavior and only\n>         add extra/new hint lines, but in other advise() calls I see the\n>         error being emitted before the advise() hint. Given that error() and\n>         die() display different message prefixes, I'm not sure whether it's\n>         possible to follow the existing pattern and avoid changing the error\n>         itself. Should I just accept that with the new advice, the error\n>         message can and should change?\n\nYou can and should use die_message() in this case, it's exactly what\nit's intended for:\n\n    int code = die_message(...);\n    /* maybe advice */\n    return code;\n\n>      2. In order to include the names of the conflicting remotes I am\n>         calling advise() multiple times - this is not a pattern I see\n>         elsewhere - should I be building a single message and calling\n>         advise() only once?\n\nThat would make me very happy, yes :)\n\nI have some not-yet-sent patches to make a lot of this advice API less\nsucky, mainly to ensure that we always have a 1=1=1 mapping between\nconfig=docs=code, and to have the API itself emit the \"and you can turn\nthis off with XYZ config\".\n\nI recently fixed the only in-tree message where we incrementally\nconstructed advice() because of that, so not having another one sneak in\nwould be good :)\n\n> diff --git a/advice.c b/advice.c\n> index 1dfc91d1767..686612590ec 100644\n> --- a/advice.c\n> +++ b/advice.c\n> @@ -39,6 +39,7 @@ static struct {\n>  \t[ADVICE_ADD_EMPTY_PATHSPEC]\t\t\t= { \"addEmptyPathspec\", 1 },\n>  \t[ADVICE_ADD_IGNORED_FILE]\t\t\t= { \"addIgnoredFile\", 1 },\n>  \t[ADVICE_AM_WORK_DIR] \t\t\t\t= { \"amWorkDir\", 1 },\n> +\t[ADVICE_AMBIGUOUS_FETCH_REFSPEC]\t\t= { \"ambiguousFetchRefspec\", 1 },\n>  \t[ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME] \t= { \"checkoutAmbiguousRemoteBranchName\", 1 },\n>  \t[ADVICE_COMMIT_BEFORE_MERGE]\t\t\t= { \"commitBeforeMerge\", 1 },\n>  \t[ADVICE_DETACHED_HEAD]\t\t\t\t= { \"detachedHead\", 1 },\n\nThis is missing the relevant Documentation/config/advice.txt update\n\n> diff --git a/advice.h b/advice.h\n> index 601265fd107..3d68c1a6cb4 100644\n> --- a/advice.h\n> +++ b/advice.h\n> @@ -17,6 +17,7 @@ struct string_list;\n>  \tADVICE_ADD_EMPTY_PATHSPEC,\n>  \tADVICE_ADD_IGNORED_FILE,\n>  \tADVICE_AM_WORK_DIR,\n> +\tADVICE_AMBIGUOUS_FETCH_REFSPEC,\n>  \tADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME,\n>  \tADVICE_COMMIT_BEFORE_MERGE,\n>  \tADVICE_DETACHED_HEAD,\n> diff --git a/branch.c b/branch.c\n> index 5d20a2e8484..243f6d8b362 100644\n> --- a/branch.c\n> +++ b/branch.c\n> @@ -12,6 +12,7 @@\n>  struct tracking {\n>  \tstruct refspec_item spec;\n>  \tstruct string_list *srcs;\n> +\tstruct string_list *remotes;\n>\n> +\"There are multiple remotes with fetch refspecs mapping to\\n\"\n> +\"the tracking ref %s:\\n\";)\n\n\"with\" and \"mapping to\" is a bit confusing, should this say:\n\n    There are multiple remotes whole fetch refspecs map to the remote\n    tracking ref '%s'?\n\n(Should also have '' quotes for that in any case)\n\n>  /*\n>   * This is called when new_ref is branched off of orig_ref, and tries\n>   * to infer the settings for branch.<new_ref>.{remote,merge} from the\n> @@ -227,11 +241,14 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,\n>  {\n>  \tstruct tracking tracking;\n>  \tstruct string_list tracking_srcs = STRING_LIST_INIT_DUP;\n> +\tstruct string_list tracking_remotes = STRING_LIST_INIT_DUP;\n>  \tint config_flags = quiet ? 0 : BRANCH_CONFIG_VERBOSE;\n> +\tstruct string_list_item *item;\n>  \n>  \tmemset(&tracking, 0, sizeof(tracking));\n>  \ttracking.spec.dst = (char *)orig_ref;\n>  \ttracking.srcs = &tracking_srcs;\n> +\ttracking.remotes = &tracking_remotes;\n>  \tif (track != BRANCH_TRACK_INHERIT)\n>  \t\tfor_each_remote(find_tracked_branch, &tracking);\n>  \telse if (inherit_tracking(&tracking, orig_ref))\n\nFWIW I find the flow with something like this a lot clearer, i.e. not\nadding the new thing to a widely-used struct, just have a CB struct for\nthe one thing that needs it:\n\n\tdiff --git a/branch.c b/branch.c\n\tindex 7958a2cb08f..55520eec6bd 100644\n\t--- a/branch.c\n\t+++ b/branch.c\n\t@@ -14,14 +14,19 @@\n\t struct tracking {\n\t \tstruct refspec_item spec;\n\t \tstruct string_list *srcs;\n\t-\tstruct string_list *remotes;\n\t \tconst char *remote;\n\t \tint matches;\n\t };\n\t \n\t+struct find_tracked_branch_cb {\n\t+\tstruct tracking *tracking;\n\t+\tstruct string_list remotes;\n\t+};\n\t+\n\t static int find_tracked_branch(struct remote *remote, void *priv)\n\t {\n\t-\tstruct tracking *tracking = priv;\n\t+\tstruct find_tracked_branch_cb *ftb = priv;\n\t+\tstruct tracking *tracking = ftb->tracking;\n\t \n\t \tif (!remote_find_tracking(remote, &tracking->spec)) {\n\t \t\tif (++tracking->matches == 1) {\n\t@@ -31,7 +36,7 @@ static int find_tracked_branch(struct remote *remote, void *priv)\n\t \t\t\tfree(tracking->spec.src);\n\t \t\t\tstring_list_clear(tracking->srcs, 0);\n\t \t\t}\n\t-\t\tstring_list_append(tracking->remotes, remote->name);\n\t+\t\tstring_list_append(&ftb->remotes, remote->name);\n\t \t\ttracking->spec.src = NULL;\n\t \t}\n\t \n\t@@ -245,16 +250,18 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,\n\t {\n\t \tstruct tracking tracking;\n\t \tstruct string_list tracking_srcs = STRING_LIST_INIT_DUP;\n\t-\tstruct string_list tracking_remotes = STRING_LIST_INIT_DUP;\n\t \tint config_flags = quiet ? 0 : BRANCH_CONFIG_VERBOSE;\n\t \tstruct string_list_item *item;\n\t+\tstruct find_tracked_branch_cb ftb_cb = {\n\t+\t\t.tracking = &tracking,\n\t+\t\t.remotes = STRING_LIST_INIT_DUP,\n\t+\t};\n\t \n\t \tmemset(&tracking, 0, sizeof(tracking));\n\t \ttracking.spec.dst = (char *)orig_ref;\n\t \ttracking.srcs = &tracking_srcs;\n\t-\ttracking.remotes = &tracking_remotes;\n\t \tif (track != BRANCH_TRACK_INHERIT)\n\t-\t\tfor_each_remote(find_tracked_branch, &tracking);\n\t+\t\tfor_each_remote(find_tracked_branch, &ftb_cb);\n\t \telse if (inherit_tracking(&tracking, orig_ref))\n\t \t\tgoto cleanup;\n\t \n\t@@ -272,7 +279,7 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,\n\t \tif (tracking.matches > 1) {\n\t \t\tif (advice_enabled(ADVICE_AMBIGUOUS_FETCH_REFSPEC)) {\n\t \t\t\tadvise(_(ambiguous_refspec_advice_pre), orig_ref);\n\t-\t\t\tfor_each_string_list_item(item, &tracking_remotes) {\n\t+\t\t\tfor_each_string_list_item(item, &ftb_cb.remotes) {\n\t \t\t\t\tadvise(\"  %s\", item->string);\n\t \t\t\t}\n\t \t\t\tadvise(_(ambiguous_refspec_advice_post));\n\nAlso missing a string_list_clear() before/after this...\n\n> @@ -248,9 +265,17 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,\n>  \t\t\treturn;\n>  \t\t}\n>  \n> -\tif (tracking.matches > 1)\n> +\tif (tracking.matches > 1) {\n> +\t\tif (advice_enabled(ADVICE_AMBIGUOUS_FETCH_REFSPEC)) {\n> +\t\t\tadvise(_(ambiguous_refspec_advice_pre), orig_ref);\n> +\t\t\tfor_each_string_list_item(item, &tracking_remotes) {\n> +\t\t\t\tadvise(\"  %s\", item->string);\n> +\t\t\t}\n\nSee:\n\n     git show --first-parent 268e6b8d4d9\n\nFor how you can avoid incrementally constructing the message. I.e. we\ncould just add a strbuf to the callback struct, have the callback add to\nit.\n\nThen on second thought we get rid of the string_list entirely don't we?\nSince we just need the \\n-delimited list of remotes te inject into the\nmessage.\n\n> +\t\t\tadvise(_(ambiguous_refspec_advice_post));\n> +\t\t}\n>  \t\tdie(_(\"not tracking: ambiguous information for ref %s\"),\n>  \t\t    orig_ref);\n> +\t}\n>  \n>  \tif (tracking.srcs->nr < 1)\n>  \t\tstring_list_append(tracking.srcs, orig_ref);\n>\n> base-commit: 4c53a8c20f8984adb226293a3ffd7b88c3f4ac1a\n\n"},{"id":"451834","messageId":"CAPMMpohn59X5CWOrhGD8gGiMumUCVcTWL=4dXE92AUsHJaRvoQ@mail.gmail.com","threadId":"57591","inReplyTo":"220321.86ee2v9xzd.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] tracking branches: add advice to ambiguous refspec error","fromName":"Tao Klerks","fromEmail":"tao@klerks.biz","sentAt":"2022-03-22T09:09:56Z","receivedAt":"2022-03-22T09:10:13Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"On Mon, Mar 21, 2022 at 3:33 PM Ævar Arnfjörð Bjarmason\n<avarab@gmail.com> wrote:\n>\n>\n> On Mon, Mar 21 2022, Tao Klerks via GitGitGadget wrote:\n>\n> Re $subject (and you've got another outstanding patch on list that's the\n> same), please add \"RFC PATCH\" to the subject for RFC patches, doesn't\n> GGG have some way to do that?\n>\n\nNot as far as I can tell, not exactly. What I *could* have done, and\nwill do next time, is add the \"RFC: \" prefix to the commit subject,\nafter the \"[PATCH]\" prefix. The email subject lines are a bit\nsurprising (to me): When it is a single commit, the email gets the\nsubject line of that commit, and the subject of the \"pull request\"\ngets buried under the triple dash, whereas a series of several commits\nhas the leading 0/N email with the pull request subject as email\nsubject.\n\n> >      1. In this proposed patch the advice is emitted before the existing\n> >         die(), in order to avoid changing the exact error behavior and only\n> >         add extra/new hint lines, but in other advise() calls I see the\n> >         error being emitted before the advise() hint. Given that error() and\n> >         die() display different message prefixes, I'm not sure whether it's\n> >         possible to follow the existing pattern and avoid changing the error\n> >         itself. Should I just accept that with the new advice, the error\n> >         message can and should change?\n>\n> You can and should use die_message() in this case, it's exactly what\n> it's intended for:\n>\n>     int code = die_message(...);\n>     /* maybe advice */\n>     return code;\n>\n\nGot it, thx.\n\n> >      2. In order to include the names of the conflicting remotes I am\n> >         calling advise() multiple times - this is not a pattern I see\n> >         elsewhere - should I be building a single message and calling\n> >         advise() only once?\n>\n> That would make me very happy, yes :)\n>\n> I have some not-yet-sent patches to make a lot of this advice API less\n> sucky, mainly to ensure that we always have a 1=1=1 mapping between\n> config=docs=code, and to have the API itself emit the \"and you can turn\n> this off with XYZ config\".\n>\n> I recently fixed the only in-tree message where we incrementally\n> constructed advice() because of that, so not having another one sneak in\n> would be good :)\n>\n\nThis is more or less sorted, although the result (the bit I did!) is\nstill a bit ugly, I suspect there is a cleaner way.\n\n> > diff --git a/advice.c b/advice.c\n> > index 1dfc91d1767..686612590ec 100644\n> > --- a/advice.c\n> > +++ b/advice.c\n> > @@ -39,6 +39,7 @@ static struct {\n> >       [ADVICE_ADD_EMPTY_PATHSPEC]                     = { \"addEmptyPathspec\", 1 },\n> >       [ADVICE_ADD_IGNORED_FILE]                       = { \"addIgnoredFile\", 1 },\n> >       [ADVICE_AM_WORK_DIR]                            = { \"amWorkDir\", 1 },\n> > +     [ADVICE_AMBIGUOUS_FETCH_REFSPEC]                = { \"ambiguousFetchRefspec\", 1 },\n> >       [ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME]  = { \"checkoutAmbiguousRemoteBranchName\", 1 },\n> >       [ADVICE_COMMIT_BEFORE_MERGE]                    = { \"commitBeforeMerge\", 1 },\n> >       [ADVICE_DETACHED_HEAD]                          = { \"detachedHead\", 1 },\n>\n> This is missing the relevant Documentation/config/advice.txt update\n>\n\nArgh, thx.\n\n> > diff --git a/advice.h b/advice.h\n> > index 601265fd107..3d68c1a6cb4 100644\n> > --- a/advice.h\n> > +++ b/advice.h\n> > @@ -17,6 +17,7 @@ struct string_list;\n> >       ADVICE_ADD_EMPTY_PATHSPEC,\n> >       ADVICE_ADD_IGNORED_FILE,\n> >       ADVICE_AM_WORK_DIR,\n> > +     ADVICE_AMBIGUOUS_FETCH_REFSPEC,\n> >       ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME,\n> >       ADVICE_COMMIT_BEFORE_MERGE,\n> >       ADVICE_DETACHED_HEAD,\n> > diff --git a/branch.c b/branch.c\n> > index 5d20a2e8484..243f6d8b362 100644\n> > --- a/branch.c\n> > +++ b/branch.c\n> > @@ -12,6 +12,7 @@\n> >  struct tracking {\n> >       struct refspec_item spec;\n> >       struct string_list *srcs;\n> > +     struct string_list *remotes;\n> >\n> > +\"There are multiple remotes with fetch refspecs mapping to\\n\"\n> > +\"the tracking ref %s:\\n\";)\n>\n> \"with\" and \"mapping to\" is a bit confusing, should this say:\n>\n>     There are multiple remotes whole fetch refspecs map to the remote\n>     tracking ref '%s'?\n\nWorks for me.\n\n>\n> (Should also have '' quotes for that in any case)\n>\n> >  /*\n> >   * This is called when new_ref is branched off of orig_ref, and tries\n> >   * to infer the settings for branch.<new_ref>.{remote,merge} from the\n> > @@ -227,11 +241,14 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,\n> >  {\n> >       struct tracking tracking;\n> >       struct string_list tracking_srcs = STRING_LIST_INIT_DUP;\n> > +     struct string_list tracking_remotes = STRING_LIST_INIT_DUP;\n> >       int config_flags = quiet ? 0 : BRANCH_CONFIG_VERBOSE;\n> > +     struct string_list_item *item;\n> >\n> >       memset(&tracking, 0, sizeof(tracking));\n> >       tracking.spec.dst = (char *)orig_ref;\n> >       tracking.srcs = &tracking_srcs;\n> > +     tracking.remotes = &tracking_remotes;\n> >       if (track != BRANCH_TRACK_INHERIT)\n> >               for_each_remote(find_tracked_branch, &tracking);\n> >       else if (inherit_tracking(&tracking, orig_ref))\n>\n> FWIW I find the flow with something like this a lot clearer, i.e. not\n> adding the new thing to a widely-used struct, just have a CB struct for\n> the one thing that needs it:\n>\n>         diff --git a/branch.c b/branch.c\n>         index 7958a2cb08f..55520eec6bd 100644\n>         --- a/branch.c\n>         +++ b/branch.c\n>         @@ -14,14 +14,19 @@\n>          struct tracking {\n>                 struct refspec_item spec;\n>                 struct string_list *srcs;\n>         -       struct string_list *remotes;\n>                 const char *remote;\n>                 int matches;\n>          };\n>\n>         +struct find_tracked_branch_cb {\n>         +       struct tracking *tracking;\n>         +       struct string_list remotes;\n>         +};\n>         +\n>          static int find_tracked_branch(struct remote *remote, void *priv)\n>          {\n>         -       struct tracking *tracking = priv;\n>         +       struct find_tracked_branch_cb *ftb = priv;\n>         +       struct tracking *tracking = ftb->tracking;\n>\n>                 if (!remote_find_tracking(remote, &tracking->spec)) {\n>                         if (++tracking->matches == 1) {\n>         @@ -31,7 +36,7 @@ static int find_tracked_branch(struct remote *remote, void *priv)\n>                                 free(tracking->spec.src);\n>                                 string_list_clear(tracking->srcs, 0);\n>                         }\n>         -               string_list_append(tracking->remotes, remote->name);\n>         +               string_list_append(&ftb->remotes, remote->name);\n>                         tracking->spec.src = NULL;\n>                 }\n>\n>         @@ -245,16 +250,18 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,\n>          {\n>                 struct tracking tracking;\n>                 struct string_list tracking_srcs = STRING_LIST_INIT_DUP;\n>         -       struct string_list tracking_remotes = STRING_LIST_INIT_DUP;\n>                 int config_flags = quiet ? 0 : BRANCH_CONFIG_VERBOSE;\n>                 struct string_list_item *item;\n>         +       struct find_tracked_branch_cb ftb_cb = {\n>         +               .tracking = &tracking,\n>         +               .remotes = STRING_LIST_INIT_DUP,\n>         +       };\n>\n>                 memset(&tracking, 0, sizeof(tracking));\n>                 tracking.spec.dst = (char *)orig_ref;\n>                 tracking.srcs = &tracking_srcs;\n>         -       tracking.remotes = &tracking_remotes;\n>                 if (track != BRANCH_TRACK_INHERIT)\n>         -               for_each_remote(find_tracked_branch, &tracking);\n>         +               for_each_remote(find_tracked_branch, &ftb_cb);\n>                 else if (inherit_tracking(&tracking, orig_ref))\n>                         goto cleanup;\n>\n>         @@ -272,7 +279,7 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,\n>                 if (tracking.matches > 1) {\n>                         if (advice_enabled(ADVICE_AMBIGUOUS_FETCH_REFSPEC)) {\n>                                 advise(_(ambiguous_refspec_advice_pre), orig_ref);\n>         -                       for_each_string_list_item(item, &tracking_remotes) {\n>         +                       for_each_string_list_item(item, &ftb_cb.remotes) {\n>                                         advise(\"  %s\", item->string);\n>                                 }\n>                                 advise(_(ambiguous_refspec_advice_post));\n\nLovely, applied, thx.\n\n>\n> Also missing a string_list_clear() before/after this...\n\nYes, sorry, I looked for \"tracking_srcs\" because I suspected some\ncleanup was needed, but did not think to look for \"tracking.srcs\", or\neven just scroll to the end. C takes some getting used to, for me at\nleast.\n\n>\n> > @@ -248,9 +265,17 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,\n> >                       return;\n> >               }\n> >\n> > -     if (tracking.matches > 1)\n> > +     if (tracking.matches > 1) {\n> > +             if (advice_enabled(ADVICE_AMBIGUOUS_FETCH_REFSPEC)) {\n> > +                     advise(_(ambiguous_refspec_advice_pre), orig_ref);\n> > +                     for_each_string_list_item(item, &tracking_remotes) {\n> > +                             advise(\"  %s\", item->string);\n> > +                     }\n>\n> See:\n>\n>      git show --first-parent 268e6b8d4d9\n>\n> For how you can avoid incrementally constructing the message. I.e. we\n> could just add a strbuf to the callback struct, have the callback add to\n> it.\n>\n> Then on second thought we get rid of the string_list entirely don't we?\n> Since we just need the \\n-delimited list of remotes te inject into the\n> message.\n\nYep, applied (with some icky argument awkwardness in the final advise() call)\n\nReroll on the way.\n"},{"id":"451836","messageId":"pull.1183.v2.git.1647940686394.gitgitgadget@gmail.com","threadId":"57591","inReplyTo":"pull.1183.git.1647858238144.gitgitgadget@gmail.com","subject":"[PATCH v2] RFC: tracking branches: add advice to ambiguous refspec error","fromName":"Tao Klerks via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-22T09:18:06Z","receivedAt":"2022-03-22T09:18:14Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"From: Tao Klerks <tao@klerks.biz>\n\nThe error \"not tracking: ambiguous information for ref\" is raised\nwhen we are evaluating what tracking information to set on a branch,\nand find that the ref to be added as tracking branch is mapped\nunder multiple remotes' fetch refspecs.\n\nThis can easily happen when a user copy-pastes a remote definition\nin their git config, and forgets to change the tracking path.\n\nAdd advice in this situation, explicitly highlighting which remotes\nare involved and suggesting how to correct the situation.\n\nSigned-off-by: Tao Klerks <tao@klerks.biz>\n---\n    RFC: tracking branches: add advice to ambiguous refspec error\n    \n    Second go at adding some advice around ambiguous remote refspecs,\n    incorporating Ævar's suggestions.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1183%2FTaoK%2Fadvise-ambiguous-tracking-refspec-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1183/TaoK/advise-ambiguous-tracking-refspec-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1183\n\nRange-diff vs v1:\n\n 1:  9ec19e1c48e ! 1:  6c1cd885dda tracking branches: add advice to ambiguous refspec error\n     @@ Metadata\n      Author: Tao Klerks <tao@klerks.biz>\n      \n       ## Commit message ##\n     -    tracking branches: add advice to ambiguous refspec error\n     +    RFC: tracking branches: add advice to ambiguous refspec error\n      \n          The error \"not tracking: ambiguous information for ref\" is raised\n          when we are evaluating what tracking information to set on a branch,\n     @@ Commit message\n      \n          Signed-off-by: Tao Klerks <tao@klerks.biz>\n      \n     + ## Documentation/config/advice.txt ##\n     +@@ Documentation/config/advice.txt: advice.*::\n     + \t\tAdvice shown when either linkgit:git-add[1] or linkgit:git-rm[1]\n     + \t\tis asked to update index entries outside the current sparse\n     + \t\tcheckout.\n     ++\tambiguousFetchRefspec::\n     ++\t\tAdvice shown when branch tracking relationship setup fails due\n     ++\t\tto multiple remotes' refspecs mapping to the same remote\n     ++\t\ttracking namespace in the repo.\n     + --\n     +\n       ## advice.c ##\n      @@ advice.c: static struct {\n       \t[ADVICE_ADD_EMPTY_PATHSPEC]\t\t\t= { \"addEmptyPathspec\", 1 },\n     @@ advice.h: struct string_list;\n       \tADVICE_DETACHED_HEAD,\n      \n       ## branch.c ##\n     -@@\n     - struct tracking {\n     - \tstruct refspec_item spec;\n     - \tstruct string_list *srcs;\n     -+\tstruct string_list *remotes;\n     - \tconst char *remote;\n     +@@ branch.c: struct tracking {\n       \tint matches;\n       };\n     + \n     ++struct find_tracked_branch_cb {\n     ++\tstruct tracking *tracking;\n     ++\tstruct strbuf remotes_advice;\n     ++};\n     ++\n     + static int find_tracked_branch(struct remote *remote, void *priv)\n     + {\n     +-\tstruct tracking *tracking = priv;\n     ++\tstruct find_tracked_branch_cb *ftb = priv;\n     ++\tstruct tracking *tracking = ftb->tracking;\n     + \n     + \tif (!remote_find_tracking(remote, &tracking->spec)) {\n     + \t\tif (++tracking->matches == 1) {\n      @@ branch.c: static int find_tracked_branch(struct remote *remote, void *priv)\n       \t\t\tfree(tracking->spec.src);\n       \t\t\tstring_list_clear(tracking->srcs, 0);\n       \t\t}\n     -+\t\tstring_list_append(tracking->remotes, remote->name);\n     ++\t\tstrbuf_addf(&ftb->remotes_advice, \"  %s\\n\", remote->name);\n       \t\ttracking->spec.src = NULL;\n       \t}\n       \n     @@ branch.c: static int inherit_tracking(struct tracking *tracking, const char *ori\n      +\n      +static const char ambiguous_refspec_advice_pre[] =\n      +N_(\"\\n\"\n     -+\"There are multiple remotes with fetch refspecs mapping to\\n\"\n     -+\"the tracking ref %s:\\n\";)\n     ++\"There are multiple remotes whose fetch refspecs map to the remote\\n\"\n     ++\"tracking ref\";)\n      +static const char ambiguous_refspec_advice_post[] =\n      +N_(\"This is typically a configuration error.\\n\"\n      +\"\\n\"\n     @@ branch.c: static int inherit_tracking(struct tracking *tracking, const char *ori\n        * This is called when new_ref is branched off of orig_ref, and tries\n        * to infer the settings for branch.<new_ref>.{remote,merge} from the\n      @@ branch.c: static void setup_tracking(const char *new_ref, const char *orig_ref,\n     - {\n       \tstruct tracking tracking;\n       \tstruct string_list tracking_srcs = STRING_LIST_INIT_DUP;\n     -+\tstruct string_list tracking_remotes = STRING_LIST_INIT_DUP;\n       \tint config_flags = quiet ? 0 : BRANCH_CONFIG_VERBOSE;\n     -+\tstruct string_list_item *item;\n     ++\tstruct find_tracked_branch_cb ftb_cb = {\n     ++\t\t.tracking = &tracking,\n     ++\t\t.remotes_advice = STRBUF_INIT,\n     ++\t};\n       \n       \tmemset(&tracking, 0, sizeof(tracking));\n       \ttracking.spec.dst = (char *)orig_ref;\n       \ttracking.srcs = &tracking_srcs;\n     -+\ttracking.remotes = &tracking_remotes;\n       \tif (track != BRANCH_TRACK_INHERIT)\n     - \t\tfor_each_remote(find_tracked_branch, &tracking);\n     +-\t\tfor_each_remote(find_tracked_branch, &tracking);\n     ++\t\tfor_each_remote(find_tracked_branch, &ftb_cb);\n       \telse if (inherit_tracking(&tracking, orig_ref))\n     + \t\treturn;\n     + \n      @@ branch.c: static void setup_tracking(const char *new_ref, const char *orig_ref,\n       \t\t\treturn;\n       \t\t}\n       \n      -\tif (tracking.matches > 1)\n     +-\t\tdie(_(\"not tracking: ambiguous information for ref %s\"),\n     +-\t\t    orig_ref);\n      +\tif (tracking.matches > 1) {\n     -+\t\tif (advice_enabled(ADVICE_AMBIGUOUS_FETCH_REFSPEC)) {\n     -+\t\t\tadvise(_(ambiguous_refspec_advice_pre), orig_ref);\n     -+\t\t\tfor_each_string_list_item(item, &tracking_remotes) {\n     -+\t\t\t\tadvise(\"  %s\", item->string);\n     -+\t\t\t}\n     -+\t\t\tadvise(_(ambiguous_refspec_advice_post));\n     -+\t\t}\n     - \t\tdie(_(\"not tracking: ambiguous information for ref %s\"),\n     - \t\t    orig_ref);\n     ++\t\tint status = die_message(_(\"not tracking: ambiguous information for ref %s\"),\n     ++\t\t\t\t\t    orig_ref);\n     ++\t\tif (advice_enabled(ADVICE_AMBIGUOUS_FETCH_REFSPEC))\n     ++\t\t\tadvise(\"%s %s:\\n%s\\n%s\",\n     ++\t\t\t       _(ambiguous_refspec_advice_pre),\n     ++\t\t\t       orig_ref,\n     ++\t\t\t       ftb_cb.remotes_advice.buf,\n     ++\t\t\t       _(ambiguous_refspec_advice_post)\n     ++\t\t\t       );\n     ++\t\texit(status);\n      +\t}\n       \n       \tif (tracking.srcs->nr < 1)\n\n\n Documentation/config/advice.txt |  4 ++++\n advice.c                        |  1 +\n advice.h                        |  1 +\n branch.c                        | 42 +++++++++++++++++++++++++++++----\n 4 files changed, 43 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt\nindex 063eec2511d..abfac4f664b 100644\n--- a/Documentation/config/advice.txt\n+++ b/Documentation/config/advice.txt\n@@ -126,4 +126,8 @@ advice.*::\n \t\tAdvice shown when either linkgit:git-add[1] or linkgit:git-rm[1]\n \t\tis asked to update index entries outside the current sparse\n \t\tcheckout.\n+\tambiguousFetchRefspec::\n+\t\tAdvice shown when branch tracking relationship setup fails due\n+\t\tto multiple remotes' refspecs mapping to the same remote\n+\t\ttracking namespace in the repo.\n --\ndiff --git a/advice.c b/advice.c\nindex 1dfc91d1767..686612590ec 100644\n--- a/advice.c\n+++ b/advice.c\n@@ -39,6 +39,7 @@ static struct {\n \t[ADVICE_ADD_EMPTY_PATHSPEC]\t\t\t= { \"addEmptyPathspec\", 1 },\n \t[ADVICE_ADD_IGNORED_FILE]\t\t\t= { \"addIgnoredFile\", 1 },\n \t[ADVICE_AM_WORK_DIR] \t\t\t\t= { \"amWorkDir\", 1 },\n+\t[ADVICE_AMBIGUOUS_FETCH_REFSPEC]\t\t= { \"ambiguousFetchRefspec\", 1 },\n \t[ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME] \t= { \"checkoutAmbiguousRemoteBranchName\", 1 },\n \t[ADVICE_COMMIT_BEFORE_MERGE]\t\t\t= { \"commitBeforeMerge\", 1 },\n \t[ADVICE_DETACHED_HEAD]\t\t\t\t= { \"detachedHead\", 1 },\ndiff --git a/advice.h b/advice.h\nindex 601265fd107..3d68c1a6cb4 100644\n--- a/advice.h\n+++ b/advice.h\n@@ -17,6 +17,7 @@ struct string_list;\n \tADVICE_ADD_EMPTY_PATHSPEC,\n \tADVICE_ADD_IGNORED_FILE,\n \tADVICE_AM_WORK_DIR,\n+\tADVICE_AMBIGUOUS_FETCH_REFSPEC,\n \tADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME,\n \tADVICE_COMMIT_BEFORE_MERGE,\n \tADVICE_DETACHED_HEAD,\ndiff --git a/branch.c b/branch.c\nindex 5d20a2e8484..4a3a77aa338 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -16,9 +16,15 @@ struct tracking {\n \tint matches;\n };\n \n+struct find_tracked_branch_cb {\n+\tstruct tracking *tracking;\n+\tstruct strbuf remotes_advice;\n+};\n+\n static int find_tracked_branch(struct remote *remote, void *priv)\n {\n-\tstruct tracking *tracking = priv;\n+\tstruct find_tracked_branch_cb *ftb = priv;\n+\tstruct tracking *tracking = ftb->tracking;\n \n \tif (!remote_find_tracking(remote, &tracking->spec)) {\n \t\tif (++tracking->matches == 1) {\n@@ -28,6 +34,7 @@ static int find_tracked_branch(struct remote *remote, void *priv)\n \t\t\tfree(tracking->spec.src);\n \t\t\tstring_list_clear(tracking->srcs, 0);\n \t\t}\n+\t\tstrbuf_addf(&ftb->remotes_advice, \"  %s\\n\", remote->name);\n \t\ttracking->spec.src = NULL;\n \t}\n \n@@ -217,6 +224,18 @@ static int inherit_tracking(struct tracking *tracking, const char *orig_ref)\n \treturn 0;\n }\n \n+\n+static const char ambiguous_refspec_advice_pre[] =\n+N_(\"\\n\"\n+\"There are multiple remotes whose fetch refspecs map to the remote\\n\"\n+\"tracking ref\";)\n+static const char ambiguous_refspec_advice_post[] =\n+N_(\"This is typically a configuration error.\\n\"\n+\"\\n\"\n+\"To support setting up tracking branches, ensure that\\n\"\n+\"different remotes' fetch refspecs map into different\\n\"\n+\"tracking namespaces.\\n\");\n+\n /*\n  * This is called when new_ref is branched off of orig_ref, and tries\n  * to infer the settings for branch.<new_ref>.{remote,merge} from the\n@@ -228,12 +247,16 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,\n \tstruct tracking tracking;\n \tstruct string_list tracking_srcs = STRING_LIST_INIT_DUP;\n \tint config_flags = quiet ? 0 : BRANCH_CONFIG_VERBOSE;\n+\tstruct find_tracked_branch_cb ftb_cb = {\n+\t\t.tracking = &tracking,\n+\t\t.remotes_advice = STRBUF_INIT,\n+\t};\n \n \tmemset(&tracking, 0, sizeof(tracking));\n \ttracking.spec.dst = (char *)orig_ref;\n \ttracking.srcs = &tracking_srcs;\n \tif (track != BRANCH_TRACK_INHERIT)\n-\t\tfor_each_remote(find_tracked_branch, &tracking);\n+\t\tfor_each_remote(find_tracked_branch, &ftb_cb);\n \telse if (inherit_tracking(&tracking, orig_ref))\n \t\treturn;\n \n@@ -248,9 +271,18 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,\n \t\t\treturn;\n \t\t}\n \n-\tif (tracking.matches > 1)\n-\t\tdie(_(\"not tracking: ambiguous information for ref %s\"),\n-\t\t    orig_ref);\n+\tif (tracking.matches > 1) {\n+\t\tint status = die_message(_(\"not tracking: ambiguous information for ref %s\"),\n+\t\t\t\t\t    orig_ref);\n+\t\tif (advice_enabled(ADVICE_AMBIGUOUS_FETCH_REFSPEC))\n+\t\t\tadvise(\"%s %s:\\n%s\\n%s\",\n+\t\t\t       _(ambiguous_refspec_advice_pre),\n+\t\t\t       orig_ref,\n+\t\t\t       ftb_cb.remotes_advice.buf,\n+\t\t\t       _(ambiguous_refspec_advice_post)\n+\t\t\t       );\n+\t\texit(status);\n+\t}\n \n \tif (tracking.srcs->nr < 1)\n \t\tstring_list_append(tracking.srcs, orig_ref);\n\nbase-commit: 4c53a8c20f8984adb226293a3ffd7b88c3f4ac1a\n-- \ngitgitgadget\n"},{"id":"451838","messageId":"220322.86ils6nvog.gmgdl@evledraar.gmail.com","threadId":"57591","inReplyTo":"pull.1183.v2.git.1647940686394.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] RFC: tracking branches: add advice to ambiguous refspec error","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-03-22T10:04:32Z","receivedAt":"2022-03-22T10:11:50Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Mar 22 2022, Tao Klerks via GitGitGadget wrote:\n\n> From: Tao Klerks <tao@klerks.biz>\n> [...]\nLooking much better!\n\n> diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt\n> index 063eec2511d..abfac4f664b 100644\n> --- a/Documentation/config/advice.txt\n> +++ b/Documentation/config/advice.txt\n> @@ -126,4 +126,8 @@ advice.*::\n>  \t\tAdvice shown when either linkgit:git-add[1] or linkgit:git-rm[1]\n>  \t\tis asked to update index entries outside the current sparse\n>  \t\tcheckout.\n> +\tambiguousFetchRefspec::\n> +\t\tAdvice shown when branch tracking relationship setup fails due\n> +\t\tto multiple remotes' refspecs mapping to the same remote\n> +\t\ttracking namespace in the repo.\n\nLet's add this in alphabetical section order. It's *somewhat* true of\nthe order now, but for some, but in any case I've got a WIP patch to\nsort these, so since it's new adding it at the top would save us the\nchurn :)\n\n> +struct find_tracked_branch_cb {\n> +\tstruct tracking *tracking;\n> +\tstruct strbuf remotes_advice;\n> +};\n\nNice, thanks!\n\n>  static int find_tracked_branch(struct remote *remote, void *priv)\n>  {\n> -\tstruct tracking *tracking = priv;\n> +\tstruct find_tracked_branch_cb *ftb = priv;\n> +\tstruct tracking *tracking = ftb->tracking;\n>  \n>  \tif (!remote_find_tracking(remote, &tracking->spec)) {\n>  \t\tif (++tracking->matches == 1) {\n> @@ -28,6 +34,7 @@ static int find_tracked_branch(struct remote *remote, void *priv)\n>  \t\t\tfree(tracking->spec.src);\n>  \t\t\tstring_list_clear(tracking->srcs, 0);\n>  \t\t}\n> +\t\tstrbuf_addf(&ftb->remotes_advice, \"  %s\\n\", remote->name);\n\nDo this as:\n\n    strbuf_addf([...], _(\"  %s\\n\"), [...]);\n\nAnd add a TRANSLATORS comment similar to show_ambiguous_object() in\nobject-name.c. I.e. the alignment needs to be translatable for RTL\nlanguages.\n\n> +static const char ambiguous_refspec_advice_pre[] =\n> +N_(\"\\n\"\n> +\"There are multiple remotes whose fetch refspecs map to the remote\\n\"\n> +\"tracking ref\";)\n> +static const char ambiguous_refspec_advice_post[] =\n> +N_(\"This is typically a configuration error.\\n\"\n> +\"\\n\"\n> +\"To support setting up tracking branches, ensure that\\n\"\n> +\"different remotes' fetch refspecs map into different\\n\"\n> +\"tracking namespaces.\\n\");\n\nThese were split up before since we incrementally built the message....\n\nAlso, is the \\n at the beginning/end really needed?\n\n>  /*\n>   * This is called when new_ref is branched off of orig_ref, and tries\n>   * to infer the settings for branch.<new_ref>.{remote,merge} from the\n> @@ -228,12 +247,16 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,\n>  \tstruct tracking tracking;\n>  \tstruct string_list tracking_srcs = STRING_LIST_INIT_DUP;\n>  \tint config_flags = quiet ? 0 : BRANCH_CONFIG_VERBOSE;\n> +\tstruct find_tracked_branch_cb ftb_cb = {\n> +\t\t.tracking = &tracking,\n> +\t\t.remotes_advice = STRBUF_INIT,\n> +\t};\n>  \n>  \tmemset(&tracking, 0, sizeof(tracking));\n>  \ttracking.spec.dst = (char *)orig_ref;\n>  \ttracking.srcs = &tracking_srcs;\n>  \tif (track != BRANCH_TRACK_INHERIT)\n> -\t\tfor_each_remote(find_tracked_branch, &tracking);\n> +\t\tfor_each_remote(find_tracked_branch, &ftb_cb);\n>  \telse if (inherit_tracking(&tracking, orig_ref))\n>  \t\treturn;\n>  \n> @@ -248,9 +271,18 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,\n>  \t\t\treturn;\n>  \t\t}\n>  \n> -\tif (tracking.matches > 1)\n> -\t\tdie(_(\"not tracking: ambiguous information for ref %s\"),\n> -\t\t    orig_ref);\n> +\tif (tracking.matches > 1) {\n> +\t\tint status = die_message(_(\"not tracking: ambiguous information for ref %s\"),\n> +\t\t\t\t\t    orig_ref);\n> +\t\tif (advice_enabled(ADVICE_AMBIGUOUS_FETCH_REFSPEC))\n> +\t\t\tadvise(\"%s %s:\\n%s\\n%s\",\n> +\t\t\t       _(ambiguous_refspec_advice_pre),\n> +\t\t\t       orig_ref,\n> +\t\t\t       ftb_cb.remotes_advice.buf,\n> +\t\t\t       _(ambiguous_refspec_advice_post)\n> +\t\t\t       );\n\n...but now we don't, so this should just be inlined here. I.e. made it\none big translatable _() message, the only parameters need to be the\norig_ref and the \"remote_advice\" buf.\n\nWe're also missing a strbuf_release() here. You can just add it to the\n\"cleanup\" omitted from the context:\n\n\tstrbuf_release(&ftb_cb.remotes_advice);\n"},{"id":"452458","messageId":"pull.1183.v3.git.1648450268285.gitgitgadget@gmail.com","threadId":"57591","inReplyTo":"pull.1183.v2.git.1647940686394.gitgitgadget@gmail.com","subject":"[PATCH v3] tracking branches: add advice to ambiguous refspec error","fromName":"Tao Klerks via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-28T06:51:08Z","receivedAt":"2022-03-28T06:51:27Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"From: Tao Klerks <tao@klerks.biz>\n\nThe error \"not tracking: ambiguous information for ref\" is raised\nwhen we are evaluating what tracking information to set on a branch,\nand find that the ref to be added as tracking branch is mapped\nunder multiple remotes' fetch refspecs.\n\nThis can easily happen when a user copy-pastes a remote definition\nin their git config, and forgets to change the tracking path.\n\nAdd advice in this situation, explicitly highlighting which remotes\nare involved and suggesting how to correct the situation.\n\nSigned-off-by: Tao Klerks <tao@klerks.biz>\n---\n    tracking branches: add advice to ambiguous refspec error\n    \n    I believe this third version incorporates all Ævar's suggestions, and\n    might be usable. Removed \"RFC\" prefix.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1183%2FTaoK%2Fadvise-ambiguous-tracking-refspec-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1183/TaoK/advise-ambiguous-tracking-refspec-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/1183\n\nRange-diff vs v2:\n\n 1:  6c1cd885dda ! 1:  22ffe81ac26 RFC: tracking branches: add advice to ambiguous refspec error\n     @@ Metadata\n      Author: Tao Klerks <tao@klerks.biz>\n      \n       ## Commit message ##\n     -    RFC: tracking branches: add advice to ambiguous refspec error\n     +    tracking branches: add advice to ambiguous refspec error\n      \n          The error \"not tracking: ambiguous information for ref\" is raised\n          when we are evaluating what tracking information to set on a branch,\n     @@ Commit message\n      \n       ## Documentation/config/advice.txt ##\n      @@ Documentation/config/advice.txt: advice.*::\n     - \t\tAdvice shown when either linkgit:git-add[1] or linkgit:git-rm[1]\n     - \t\tis asked to update index entries outside the current sparse\n     - \t\tcheckout.\n     + \tcan tell Git that you do not need help by setting these to 'false':\n     + +\n     + --\n      +\tambiguousFetchRefspec::\n      +\t\tAdvice shown when branch tracking relationship setup fails due\n      +\t\tto multiple remotes' refspecs mapping to the same remote\n      +\t\ttracking namespace in the repo.\n     - --\n     + \tfetchShowForcedUpdates::\n     + \t\tAdvice shown when linkgit:git-fetch[1] takes a long time\n     + \t\tto calculate forced updates after ref updates, or to warn\n      \n       ## advice.c ##\n      @@ advice.c: static struct {\n     @@ branch.c: static int find_tracked_branch(struct remote *remote, void *priv)\n       \t\t\tfree(tracking->spec.src);\n       \t\t\tstring_list_clear(tracking->srcs, 0);\n       \t\t}\n     -+\t\tstrbuf_addf(&ftb->remotes_advice, \"  %s\\n\", remote->name);\n     ++\t\t/*\n     ++\t\t * TRANSLATORS: This is a line listing a remote with duplicate\n     ++\t\t * refspecs, to be later included in advice message\n     ++\t\t * ambiguousFetchRefspec. For RTL languages you'll probably want\n     ++\t\t * to swap the \"%s\" and leading \"  \" space around.\n     ++\t\t */\n     ++\t\tstrbuf_addf(&ftb->remotes_advice, _(\"  %s\\n\"), remote->name);\n       \t\ttracking->spec.src = NULL;\n       \t}\n       \n     @@ branch.c: static int inherit_tracking(struct tracking *tracking, const char *ori\n       \treturn 0;\n       }\n       \n     -+\n     -+static const char ambiguous_refspec_advice_pre[] =\n     -+N_(\"\\n\"\n     -+\"There are multiple remotes whose fetch refspecs map to the remote\\n\"\n     -+\"tracking ref\";)\n     -+static const char ambiguous_refspec_advice_post[] =\n     -+N_(\"This is typically a configuration error.\\n\"\n     -+\"\\n\"\n     -+\"To support setting up tracking branches, ensure that\\n\"\n     -+\"different remotes' fetch refspecs map into different\\n\"\n     -+\"tracking namespaces.\\n\");\n      +\n       /*\n     -  * This is called when new_ref is branched off of orig_ref, and tries\n     -  * to infer the settings for branch.<new_ref>.{remote,merge} from the\n     +  * Used internally to set the branch.<new_ref>.{remote,merge} config\n     +  * settings so that branch 'new_ref' tracks 'orig_ref'. Unlike\n      @@ branch.c: static void setup_tracking(const char *new_ref, const char *orig_ref,\n       \tstruct tracking tracking;\n       \tstruct string_list tracking_srcs = STRING_LIST_INIT_DUP;\n     @@ branch.c: static void setup_tracking(const char *new_ref, const char *orig_ref,\n      -\t\tfor_each_remote(find_tracked_branch, &tracking);\n      +\t\tfor_each_remote(find_tracked_branch, &ftb_cb);\n       \telse if (inherit_tracking(&tracking, orig_ref))\n     - \t\treturn;\n     + \t\tgoto cleanup;\n       \n      @@ branch.c: static void setup_tracking(const char *new_ref, const char *orig_ref,\n     - \t\t\treturn;\n     + \t\t\tgoto cleanup;\n       \t\t}\n       \n      -\tif (tracking.matches > 1)\n     @@ branch.c: static void setup_tracking(const char *new_ref, const char *orig_ref,\n      +\t\tint status = die_message(_(\"not tracking: ambiguous information for ref %s\"),\n      +\t\t\t\t\t    orig_ref);\n      +\t\tif (advice_enabled(ADVICE_AMBIGUOUS_FETCH_REFSPEC))\n     -+\t\t\tadvise(\"%s %s:\\n%s\\n%s\",\n     -+\t\t\t       _(ambiguous_refspec_advice_pre),\n     ++\t\t\tadvise(_(\"There are multiple remotes whose fetch refspecs map to the remote\\n\"\n     ++\t\t\t\t \"tracking ref %s:\\n\"\n     ++\t\t\t\t \"%s\"\n     ++\t\t\t\t \"\\n\"\n     ++\t\t\t\t \"This is typically a configuration error.\\n\"\n     ++\t\t\t\t \"\\n\"\n     ++\t\t\t\t \"To support setting up tracking branches, ensure that\\n\"\n     ++\t\t\t\t \"different remotes' fetch refspecs map into different\\n\"\n     ++\t\t\t\t \"tracking namespaces.\"),\n      +\t\t\t       orig_ref,\n     -+\t\t\t       ftb_cb.remotes_advice.buf,\n     -+\t\t\t       _(ambiguous_refspec_advice_post)\n     ++\t\t\t       ftb_cb.remotes_advice.buf\n      +\t\t\t       );\n      +\t\texit(status);\n      +\t}\n       \n       \tif (tracking.srcs->nr < 1)\n       \t\tstring_list_append(tracking.srcs, orig_ref);\n     +@@ branch.c: static void setup_tracking(const char *new_ref, const char *orig_ref,\n     + \t\texit(-1);\n     + \n     + cleanup:\n     ++\tstrbuf_release(&ftb_cb.remotes_advice);\n     + \tstring_list_clear(&tracking_srcs, 0);\n     + }\n     + \n\n\n Documentation/config/advice.txt |  4 +++\n advice.c                        |  1 +\n advice.h                        |  1 +\n branch.c                        | 44 +++++++++++++++++++++++++++++----\n 4 files changed, 45 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt\nindex c40eb09cb7e..90f7dbd03aa 100644\n--- a/Documentation/config/advice.txt\n+++ b/Documentation/config/advice.txt\n@@ -4,6 +4,10 @@ advice.*::\n \tcan tell Git that you do not need help by setting these to 'false':\n +\n --\n+\tambiguousFetchRefspec::\n+\t\tAdvice shown when branch tracking relationship setup fails due\n+\t\tto multiple remotes' refspecs mapping to the same remote\n+\t\ttracking namespace in the repo.\n \tfetchShowForcedUpdates::\n \t\tAdvice shown when linkgit:git-fetch[1] takes a long time\n \t\tto calculate forced updates after ref updates, or to warn\ndiff --git a/advice.c b/advice.c\nindex 2e1fd483040..18ac8739519 100644\n--- a/advice.c\n+++ b/advice.c\n@@ -39,6 +39,7 @@ static struct {\n \t[ADVICE_ADD_EMPTY_PATHSPEC]\t\t\t= { \"addEmptyPathspec\", 1 },\n \t[ADVICE_ADD_IGNORED_FILE]\t\t\t= { \"addIgnoredFile\", 1 },\n \t[ADVICE_AM_WORK_DIR] \t\t\t\t= { \"amWorkDir\", 1 },\n+\t[ADVICE_AMBIGUOUS_FETCH_REFSPEC]\t\t= { \"ambiguousFetchRefspec\", 1 },\n \t[ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME] \t= { \"checkoutAmbiguousRemoteBranchName\", 1 },\n \t[ADVICE_COMMIT_BEFORE_MERGE]\t\t\t= { \"commitBeforeMerge\", 1 },\n \t[ADVICE_DETACHED_HEAD]\t\t\t\t= { \"detachedHead\", 1 },\ndiff --git a/advice.h b/advice.h\nindex a3957123a16..2d4c94f38eb 100644\n--- a/advice.h\n+++ b/advice.h\n@@ -17,6 +17,7 @@ struct string_list;\n \tADVICE_ADD_EMPTY_PATHSPEC,\n \tADVICE_ADD_IGNORED_FILE,\n \tADVICE_AM_WORK_DIR,\n+\tADVICE_AMBIGUOUS_FETCH_REFSPEC,\n \tADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME,\n \tADVICE_COMMIT_BEFORE_MERGE,\n \tADVICE_DETACHED_HEAD,\ndiff --git a/branch.c b/branch.c\nindex 6b31df539a5..5c28d432103 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -18,9 +18,15 @@ struct tracking {\n \tint matches;\n };\n \n+struct find_tracked_branch_cb {\n+\tstruct tracking *tracking;\n+\tstruct strbuf remotes_advice;\n+};\n+\n static int find_tracked_branch(struct remote *remote, void *priv)\n {\n-\tstruct tracking *tracking = priv;\n+\tstruct find_tracked_branch_cb *ftb = priv;\n+\tstruct tracking *tracking = ftb->tracking;\n \n \tif (!remote_find_tracking(remote, &tracking->spec)) {\n \t\tif (++tracking->matches == 1) {\n@@ -30,6 +36,13 @@ static int find_tracked_branch(struct remote *remote, void *priv)\n \t\t\tfree(tracking->spec.src);\n \t\t\tstring_list_clear(tracking->srcs, 0);\n \t\t}\n+\t\t/*\n+\t\t * TRANSLATORS: This is a line listing a remote with duplicate\n+\t\t * refspecs, to be later included in advice message\n+\t\t * ambiguousFetchRefspec. For RTL languages you'll probably want\n+\t\t * to swap the \"%s\" and leading \"  \" space around.\n+\t\t */\n+\t\tstrbuf_addf(&ftb->remotes_advice, _(\"  %s\\n\"), remote->name);\n \t\ttracking->spec.src = NULL;\n \t}\n \n@@ -219,6 +232,7 @@ static int inherit_tracking(struct tracking *tracking, const char *orig_ref)\n \treturn 0;\n }\n \n+\n /*\n  * Used internally to set the branch.<new_ref>.{remote,merge} config\n  * settings so that branch 'new_ref' tracks 'orig_ref'. Unlike\n@@ -232,12 +246,16 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,\n \tstruct tracking tracking;\n \tstruct string_list tracking_srcs = STRING_LIST_INIT_DUP;\n \tint config_flags = quiet ? 0 : BRANCH_CONFIG_VERBOSE;\n+\tstruct find_tracked_branch_cb ftb_cb = {\n+\t\t.tracking = &tracking,\n+\t\t.remotes_advice = STRBUF_INIT,\n+\t};\n \n \tmemset(&tracking, 0, sizeof(tracking));\n \ttracking.spec.dst = (char *)orig_ref;\n \ttracking.srcs = &tracking_srcs;\n \tif (track != BRANCH_TRACK_INHERIT)\n-\t\tfor_each_remote(find_tracked_branch, &tracking);\n+\t\tfor_each_remote(find_tracked_branch, &ftb_cb);\n \telse if (inherit_tracking(&tracking, orig_ref))\n \t\tgoto cleanup;\n \n@@ -252,9 +270,24 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,\n \t\t\tgoto cleanup;\n \t\t}\n \n-\tif (tracking.matches > 1)\n-\t\tdie(_(\"not tracking: ambiguous information for ref %s\"),\n-\t\t    orig_ref);\n+\tif (tracking.matches > 1) {\n+\t\tint status = die_message(_(\"not tracking: ambiguous information for ref %s\"),\n+\t\t\t\t\t    orig_ref);\n+\t\tif (advice_enabled(ADVICE_AMBIGUOUS_FETCH_REFSPEC))\n+\t\t\tadvise(_(\"There are multiple remotes whose fetch refspecs map to the remote\\n\"\n+\t\t\t\t \"tracking ref %s:\\n\"\n+\t\t\t\t \"%s\"\n+\t\t\t\t \"\\n\"\n+\t\t\t\t \"This is typically a configuration error.\\n\"\n+\t\t\t\t \"\\n\"\n+\t\t\t\t \"To support setting up tracking branches, ensure that\\n\"\n+\t\t\t\t \"different remotes' fetch refspecs map into different\\n\"\n+\t\t\t\t \"tracking namespaces.\"),\n+\t\t\t       orig_ref,\n+\t\t\t       ftb_cb.remotes_advice.buf\n+\t\t\t       );\n+\t\texit(status);\n+\t}\n \n \tif (tracking.srcs->nr < 1)\n \t\tstring_list_append(tracking.srcs, orig_ref);\n@@ -263,6 +296,7 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,\n \t\texit(-1);\n \n cleanup:\n+\tstrbuf_release(&ftb_cb.remotes_advice);\n \tstring_list_clear(&tracking_srcs, 0);\n }\n \n\nbase-commit: abf474a5dd901f28013c52155411a48fd4c09922\n-- \ngitgitgadget\n"},{"id":"452482","messageId":"xmqqwnge2ghe.fsf@gitster.g","threadId":"57591","inReplyTo":"pull.1183.v3.git.1648450268285.gitgitgadget@gmail.com","subject":"Re: [PATCH v3] tracking branches: add advice to ambiguous refspec error","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-28T16:23:41Z","receivedAt":"2022-03-28T16:23:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Tao Klerks via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Tao Klerks <tao@klerks.biz>\n>\n> The error \"not tracking: ambiguous information for ref\" is raised\n> when we are evaluating what tracking information to set on a branch,\n> and find that the ref to be added as tracking branch is mapped\n> under multiple remotes' fetch refspecs.\n\nOK.\n\n>  Documentation/config/advice.txt |  4 +++\n>  advice.c                        |  1 +\n>  advice.h                        |  1 +\n>  branch.c                        | 44 +++++++++++++++++++++++++++++----\n>  4 files changed, 45 insertions(+), 5 deletions(-)\n>\n> diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt\n> index c40eb09cb7e..90f7dbd03aa 100644\n> --- a/Documentation/config/advice.txt\n> +++ b/Documentation/config/advice.txt\n> @@ -4,6 +4,10 @@ advice.*::\n>  \tcan tell Git that you do not need help by setting these to 'false':\n>  +\n>  --\n> +\tambiguousFetchRefspec::\n> +\t\tAdvice shown when branch tracking relationship setup fails due\n> +\t\tto multiple remotes' refspecs mapping to the same remote\n> +\t\ttracking namespace in the repo.\n\n\tAdvice shown when fetch refspec for multiple remotes map to\n\tthe same remote-tracking branch namespace and causes branch\n\ttracking set-up to fail.\n\n\"remote-tracking\" should be spelled as a single word.  There are\nsome existing mistakes in the code, but let's not make it worse.\n\n> diff --git a/branch.c b/branch.c\n> index 6b31df539a5..5c28d432103 100644\n> --- a/branch.c\n> +++ b/branch.c\n> @@ -18,9 +18,15 @@ struct tracking {\n>  \tint matches;\n>  };\n>  \n> +struct find_tracked_branch_cb {\n> +\tstruct tracking *tracking;\n> +\tstruct strbuf remotes_advice;\n> +};\n> +\n>  static int find_tracked_branch(struct remote *remote, void *priv)\n>  {\n> -\tstruct tracking *tracking = priv;\n> +\tstruct find_tracked_branch_cb *ftb = priv;\n> +\tstruct tracking *tracking = ftb->tracking;\n>  \n>  \tif (!remote_find_tracking(remote, &tracking->spec)) {\n>  \t\tif (++tracking->matches == 1) {\n>  \t\t\tstring_list_append(tracking->srcs, tracking->spec.src);\n>  \t\t\ttracking->remote = remote->name;\n>  \t\t} else {\n>  \t\t\tfree(tracking->spec.src);\n>  \t\t\tstring_list_clear(tracking->srcs, 0);\n>  \t\t}\n> +\t\t/*\n> +\t\t * TRANSLATORS: This is a line listing a remote with duplicate\n> +\t\t * refspecs, to be later included in advice message\n> +\t\t * ambiguousFetchRefspec. For RTL languages you'll probably want\n> +\t\t * to swap the \"%s\" and leading \"  \" space around.\n> +\t\t */\n> +\t\tstrbuf_addf(&ftb->remotes_advice, _(\"  %s\\n\"), remote->name);\n>  \t\ttracking->spec.src = NULL;\n>  \t}\n\nThis is wasteful.  remotes_advice does not have to be filled when we\nare not going to give advice, i.e. there is only one matching remote\nand no second or subsequent ones, which should be the majority case.\nAnd we should not make majority of users who do not make a mistake\nthat needs the advice message pay the cost of giving advice to those\nwho screw up, if we can avoid it.\n\nInstead, only when we are looking at the second one, we can stuff\ntracking->remote (i.e. the first one) to remotes_advice, and then\nappend remote->name there.  Perhaps we can do it like so?\n\n\tstruct strbuf *names = &ftb->remotes_advice;\n\tconst char *namefmt = N_(\"  %s\\n\");\n\n\tswitch (++tracking->matches) {\n\tcase 1:\n\t\tstring_list_append(tracking->srcs, tracking->spec.src);\n\t\ttracking->remote = remote->name;\n\t\tbreak;\n\tcase 2:\n\t\tstrbuf_addf(names, _(namefmt), tracking->remote);\n\t\t/* fall through */\n\tdefault:\n\t\tstrbuf_addf(names, _(namefmt), remote->name);\n                free(tracking->spec.src);\n                string_list_clear(tracking->srcs, 0);\n                break;\n\t}\n\ttracking->spec.src = NULL;\n\nFor a bonus point, remotes_advice member can be left empty without\npaying the cost to strbuf_addf() when the advice configuration says\nwe are not going to show the message.\n\nThanks.\n"},{"id":"452486","messageId":"kl6l7d8et314.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"57591","inReplyTo":"xmqqwnge2ghe.fsf@gitster.g","subject":"Re: [PATCH v3] tracking branches: add advice to ambiguous refspec error","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-03-28T17:12:07Z","receivedAt":"2022-03-28T17:12:14Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n>> diff --git a/branch.c b/branch.c\n>> index 6b31df539a5..5c28d432103 100644\n>> --- a/branch.c\n>> +++ b/branch.c\n>> @@ -18,9 +18,15 @@ struct tracking {\n>>  \tint matches;\n>>  };\n>>  \n>> +struct find_tracked_branch_cb {\n>> +\tstruct tracking *tracking;\n>> +\tstruct strbuf remotes_advice;\n>> +};\n>> +\n>>  static int find_tracked_branch(struct remote *remote, void *priv)\n>>  {\n>> -\tstruct tracking *tracking = priv;\n>> +\tstruct find_tracked_branch_cb *ftb = priv;\n>> +\tstruct tracking *tracking = ftb->tracking;\n>>  \n>>  \tif (!remote_find_tracking(remote, &tracking->spec)) {\n>>  \t\tif (++tracking->matches == 1) {\n>>  \t\t\tstring_list_append(tracking->srcs, tracking->spec.src);\n>>  \t\t\ttracking->remote = remote->name;\n>>  \t\t} else {\n>>  \t\t\tfree(tracking->spec.src);\n>>  \t\t\tstring_list_clear(tracking->srcs, 0);\n>>  \t\t}\n>> +\t\t/*\n>> +\t\t * TRANSLATORS: This is a line listing a remote with duplicate\n>> +\t\t * refspecs, to be later included in advice message\n>> +\t\t * ambiguousFetchRefspec. For RTL languages you'll probably want\n>> +\t\t * to swap the \"%s\" and leading \"  \" space around.\n>> +\t\t */\n>> +\t\tstrbuf_addf(&ftb->remotes_advice, _(\"  %s\\n\"), remote->name);\n>>  \t\ttracking->spec.src = NULL;\n>>  \t}\n>\n> This is wasteful.  remotes_advice does not have to be filled when we\n> are not going to give advice, i.e. there is only one matching remote\n> and no second or subsequent ones, which should be the majority case.\n> And we should not make majority of users who do not make a mistake\n> that needs the advice message pay the cost of giving advice to those\n> who screw up, if we can avoid it.\n>\n> Instead, only when we are looking at the second one, we can stuff\n> tracking->remote (i.e. the first one) to remotes_advice, and then\n> append remote->name there.  Perhaps we can do it like so?\n>\n> \tstruct strbuf *names = &ftb->remotes_advice;\n> \tconst char *namefmt = N_(\"  %s\\n\");\n>\n> \tswitch (++tracking->matches) {\n> \tcase 1:\n> \t\tstring_list_append(tracking->srcs, tracking->spec.src);\n> \t\ttracking->remote = remote->name;\n> \t\tbreak;\n> \tcase 2:\n> \t\tstrbuf_addf(names, _(namefmt), tracking->remote);\n> \t\t/* fall through */\n> \tdefault:\n> \t\tstrbuf_addf(names, _(namefmt), remote->name);\n>                 free(tracking->spec.src);\n>                 string_list_clear(tracking->srcs, 0);\n>                 break;\n> \t}\n> \ttracking->spec.src = NULL;\n>\n> For a bonus point, remotes_advice member can be left empty without\n> paying the cost to strbuf_addf() when the advice configuration says\n> we are not going to show the message.\n>\n> Thanks.\n\nHm, what do you think of an alternate approach of storing of the\nmatching remotes in a string_list, something like:\n\n  struct find_tracked_branch_cb {\n  \tstruct tracking *tracking;\n  \tstruct string_list matching_remotes;\n  };\n  \n  static int find_tracked_branch(struct remote *remote, void *priv)\n  {\n  \tstruct tracking *tracking = priv;\n  \tstruct find_tracked_branch_cb *ftb = priv;\n  \tstruct tracking *tracking = ftb->tracking;\n  \n  \tif (!remote_find_tracking(remote, &tracking->spec)) {\n  \t\tif (++tracking->matches == 1) {\n  \t\t\tstring_list_append(tracking->srcs, tracking->spec.src);\n  \t\t\ttracking->remote = remote->name;\n  \t\t} else {\n  \t\t\tfree(tracking->spec.src);\n  \t\t\tstring_list_clear(tracking->srcs, 0);\n  \t\t}\n  \t\tstring_list_append(&ftb->matching_remotes, remote->name);\n  \t\ttracking->spec.src = NULL;\n  \t}\n\nthen construct the advice message in setup_tracking()? To my untrained\neye, \"case 2\" requires a bit of extra work to understand.\n"},{"id":"452487","messageId":"xmqqilry2dq7.fsf@gitster.g","threadId":"57591","inReplyTo":"kl6l7d8et314.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: [PATCH v3] tracking branches: add advice to ambiguous refspec error","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-28T17:23:12Z","receivedAt":"2022-03-28T17:23:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Glen Choo <chooglen@google.com> writes:\n\n> Hm, what do you think of an alternate approach of storing of the\n> matching remotes in a string_list, something like:\n>\n>   struct find_tracked_branch_cb {\n>   \tstruct tracking *tracking;\n>   \tstruct string_list matching_remotes;\n>   };\n>   \n>   static int find_tracked_branch(struct remote *remote, void *priv)\n>   {\n>   \tstruct tracking *tracking = priv;\n>   \tstruct find_tracked_branch_cb *ftb = priv;\n>   \tstruct tracking *tracking = ftb->tracking;\n>   \n>   \tif (!remote_find_tracking(remote, &tracking->spec)) {\n>   \t\tif (++tracking->matches == 1) {\n>   \t\t\tstring_list_append(tracking->srcs, tracking->spec.src);\n>   \t\t\ttracking->remote = remote->name;\n>   \t\t} else {\n>   \t\t\tfree(tracking->spec.src);\n>   \t\t\tstring_list_clear(tracking->srcs, 0);\n>   \t\t}\n>   \t\tstring_list_append(&ftb->matching_remotes, remote->name);\n>   \t\ttracking->spec.src = NULL;\n>   \t}\n>\n> then construct the advice message in setup_tracking()? To my untrained\n> eye, \"case 2\" requires a bit of extra work to understand.\n\nIf I were writing this code from scratch, I would have probably\ncollected the names first in this callback, and assembled them at\nthe end into a single string when we need a single string to show.\n\nHaving said that, as long as you do that lazily not to penalize\nthose who have sane setting without the need for advice/error to\ntrigger, I do not particularly care how the list of matching remote\nnames are kept.  Having string_list_append() unconditionally like\nthe above patch has, even for folks with just a single match without\nneed for the advice/error message is suboptimal, I would think.\n\n"},{"id":"452490","messageId":"CAPMMpojYJ9sB7nbAAt1b_yH0Um1O-+TpSRYXTkZ6aDHobhS59A@mail.gmail.com","threadId":"57591","inReplyTo":"xmqqilry2dq7.fsf@gitster.g","subject":"Re: [PATCH v3] tracking branches: add advice to ambiguous refspec error","fromName":"Tao Klerks","fromEmail":"tao@klerks.biz","sentAt":"2022-03-28T18:02:10Z","receivedAt":"2022-03-28T18:02:29Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"On Mon, Mar 28, 2022 at 7:23 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Glen Choo <chooglen@google.com> writes:\n>\n> > Hm, what do you think of an alternate approach of storing of the\n> > matching remotes in a string_list, something like:\n[...]\n> > then construct the advice message in setup_tracking()? To my untrained\n> > eye, \"case 2\" requires a bit of extra work to understand.\n\nInterestingly, that was what I had in the original RFC. I started using\nthe strbuf later, after Ævar confirmed that a single \"advise()\" call is\nthe way to go. I understood building the string as we go to lead to\nsimpler code, as it meant one less loop. On the other hand I\nunderstand Junio is more concerned about performance than the\nexistence of a second loop that we should almost never hit.\n\nI'm very happy to switch from strbuf-building to string_list-appending,\nbut I'm curious to understand how/why the performance of\nstrbuf_addf() would be notably worse than that of\nstring_list_append().\n\nIs there public doc about this somewhere?\n\n> Having said that, as long as you do that lazily not to penalize\n> those who have sane setting without the need for advice/error to\n> trigger, I do not particularly care how the list of matching remote\n> names are kept.  Having string_list_append() unconditionally like\n> the above patch has, even for folks with just a single match without\n> need for the advice/error message is suboptimal, I would think.\n\nAgain, I'm new here, and not a great coder to start with, but I'm\nhaving a hard time understanding why the single extra/gratuitous\nstrbuf_addf() or string_list_append() call that we stand to optimize\n(I haven't understood whether there is a significant difference\nbetween them) would ever be noticeable in the context of creating\na branch.\n\nI of course completely understand optimizing anything that will\nend up looping, but this is a max of 1 iteration's savings; I would\nhave thought that at these levels, readability/maintainability (and\nsuccinctness) of the code would trump any marginal performance\nsavings.\n\nTo that end, I'd understand going back to string_list_append() as\nGlen proposes, and building a formatted string in a single place\n(setup_tracking()) only when required - both for readability, and\nin case some aspect of strbuf_addf() is non-trivially expensive -\nbut is the \"only append to the string_list() on the rare second\npass\" optimization really worth the increase in amount of code?\n\nIs \"performance over succinctness\" a general principle that\ncould or should be noted somewhere?\n"},{"id":"452492","messageId":"220328.86y20t3o5s.gmgdl@evledraar.gmail.com","threadId":"57591","inReplyTo":"CAPMMpojYJ9sB7nbAAt1b_yH0Um1O-+TpSRYXTkZ6aDHobhS59A@mail.gmail.com","subject":"Re: [PATCH v3] tracking branches: add advice to ambiguous refspec error","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-03-28T18:50:43Z","receivedAt":"2022-03-28T18:52:38Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Mar 28 2022, Tao Klerks wrote:\n\n> On Mon, Mar 28, 2022 at 7:23 PM Junio C Hamano <gitster@pobox.com> wrote:\n>>\n>> Glen Choo <chooglen@google.com> writes:\n>>\n>> > Hm, what do you think of an alternate approach of storing of the\n>> > matching remotes in a string_list, something like:\n> [...]\n>> > then construct the advice message in setup_tracking()? To my untrained\n>> > eye, \"case 2\" requires a bit of extra work to understand.\n>\n> Interestingly, that was what I had in the original RFC. I started using\n> the strbuf later, after Ævar confirmed that a single \"advise()\" call is\n> the way to go. I understood building the string as we go to lead to\n> simpler code, as it meant one less loop. On the other hand I\n> understand Junio is more concerned about performance than the\n> existence of a second loop that we should almost never hit.\n>\n> I'm very happy to switch from strbuf-building to string_list-appending,\n> but I'm curious to understand how/why the performance of\n> strbuf_addf() would be notably worse than that of\n> string_list_append().\n>\n> Is there public doc about this somewhere?\n\nWe could do a string_list as in your v1 and concat it as we're\nformatting it, but pushing new strings to a string_list v.s. appending\nto a strbuf is actually more efficient in favor of the strbuf.\n\nBut as to not penalizing those who don't have the advice enabled,\nsomething like this (untested)?:\n\ndiff --git a/branch.c b/branch.c\nindex 5c28d432103..83545456c36 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -20,6 +20,7 @@ struct tracking {\n \n struct find_tracked_branch_cb {\n \tstruct tracking *tracking;\n+\tunsigned int do_advice:1;\n \tstruct strbuf remotes_advice;\n };\n \n@@ -36,6 +37,9 @@ static int find_tracked_branch(struct remote *remote, void *priv)\n \t\t\tfree(tracking->spec.src);\n \t\t\tstring_list_clear(tracking->srcs, 0);\n \t\t}\n+\t\ttracking->spec.src = NULL;\n+\t\tif (!ftb->do_advice)\n+\t\t\treturn 0;\n \t\t/*\n \t\t * TRANSLATORS: This is a line listing a remote with duplicate\n \t\t * refspecs, to be later included in advice message\n@@ -43,7 +47,6 @@ static int find_tracked_branch(struct remote *remote, void *priv)\n \t\t * to swap the \"%s\" and leading \"  \" space around.\n \t\t */\n \t\tstrbuf_addf(&ftb->remotes_advice, _(\"  %s\\n\"), remote->name);\n-\t\ttracking->spec.src = NULL;\n \t}\n \n \treturn 0;\n@@ -249,6 +252,7 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,\n \tstruct find_tracked_branch_cb ftb_cb = {\n \t\t.tracking = &tracking,\n \t\t.remotes_advice = STRBUF_INIT,\n+\t\t.do_advice = advice_enabled(ADVICE_AMBIGUOUS_FETCH_REFSPEC),\n \t};\n \n \tmemset(&tracking, 0, sizeof(tracking));\n@@ -273,7 +277,7 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,\n \tif (tracking.matches > 1) {\n \t\tint status = die_message(_(\"not tracking: ambiguous information for ref %s\"),\n \t\t\t\t\t    orig_ref);\n-\t\tif (advice_enabled(ADVICE_AMBIGUOUS_FETCH_REFSPEC))\n+\t\tif (ftb_cb.do_advice)\n \t\t\tadvise(_(\"There are multiple remotes whose fetch refspecs map to the remote\\n\"\n \t\t\t\t \"tracking ref %s:\\n\"\n \t\t\t\t \"%s\"\n"},{"id":"452507","messageId":"xmqqo81p256z.fsf@gitster.g","threadId":"57591","inReplyTo":"CAPMMpojYJ9sB7nbAAt1b_yH0Um1O-+TpSRYXTkZ6aDHobhS59A@mail.gmail.com","subject":"Re: [PATCH v3] tracking branches: add advice to ambiguous refspec error","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-28T20:27:32Z","receivedAt":"2022-03-28T20:27:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tao Klerks <tao@klerks.biz> writes:\n\n>> Having said that, as long as you do that lazily not to penalize\n>> those who have sane setting without the need for advice/error to\n>> trigger, I do not particularly care how the list of matching remote\n>> names are kept.  Having string_list_append() unconditionally like\n>> the above patch has, even for folks with just a single match without\n>> need for the advice/error message is suboptimal, I would think.\n>\n> Again, I'm new here, and not a great coder to start with, but I'm\n> having a hard time understanding why the single extra/gratuitous\n> strbuf_addf() or string_list_append() call that we stand to optimize\n> (I haven't understood whether there is a significant difference\n> between them) would ever be noticeable in the context of creating\n> a branch.\n\nSmall things accumulate.\n\nUnless you have to bend over backwards to do so, avoid computing\nunconditionally what you only need in an error case.  People do not\nmake mistake all the time, and they shouldn't be forced to pay for\nthe possibility that other people may make mistakes and may want to\nget help.\n\nWhen you see that you are wasting cycles \"just in case I may see an\nerror\", you may stop, take a deep breath, and think if you can push\nthe extra work to error code path with equally simple and easy code.\nAnd in this case, I think it is just as equally easy and simple as\nthe unconditional code in your patch to optimize for the case where\nthere is *no* duplicate.\n\nIt is a good discipline to learn, especially if you are new, as it\nis something that stays with you even after you move on to different\nprojects.\n\n> I of course completely understand optimizing anything that will\n> end up looping, but this is a max of 1 iteration's savings; I would\n\nWhen there is no error, you do not even need to keep the \"names that\nwill become necessary for advice\" at all, so you are comparing 0\nwith 1.  And if you read the no-error case of the suggested rewrite,\nyou'd realize that it is much easier to reason about.  You do not\nhave to wonder what the remotes_advice strbuf (or string_list) is\ndoing in the no-error code path at all.  This is not about\nperformance, it is about clarity of the code (not doing unnecessary\nthing means doing only essential thing, after all).\n\nBetween strbuf appending and string_list appending, I do not\npersonally care.  As long as the code can produce the same output,\nit is OK.  Using string_list _may_ have an advantage that string\nformatting logic will be concentrated in a single place, as opposed\nto strbuf appending, where part of formatting decisions need to be\nmade in the callback and other part is left in the advise-string.\nAnd because this should all happen only in error code path, the\nperformance between two APIs would not matter at all (and for that\nto truly hold, you need to stick to \"do not unconditionally compute \nwhat you need only in an error case\" discipline).\n\n\n\n\n\n"},{"id":"452508","messageId":"xmqqk0cd24yn.fsf@gitster.g","threadId":"57591","inReplyTo":"220328.86y20t3o5s.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v3] tracking branches: add advice to ambiguous refspec error","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-28T20:32:32Z","receivedAt":"2022-03-28T20:34:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n> But as to not penalizing those who don't have the advice enabled,\n> something like this (untested)?:\n\nUnconditionally computing what you need only in the error path is\nthe primary sin in the patch, and that should be addressed first.\n\nIf we need to do new things (like adding the do_advice member), it\nbecomes questionable if it is worth doing.  In this case, I think\nthe update is simple enough and the control flow makes it clear what\nis and what isn't needed only for advice-message generation, so it\nmight be an overall win.\n\nThanks.\n"},{"id":"452554","messageId":"CAPMMpogK_e8Z3QobtJTnrzY=MynF0kqbZJEv=cZgWzwUodQx=A@mail.gmail.com","threadId":"57591","inReplyTo":"xmqqo81p256z.fsf@gitster.g","subject":"Re: [PATCH v3] tracking branches: add advice to ambiguous refspec error","fromName":"Tao Klerks","fromEmail":"tao@klerks.biz","sentAt":"2022-03-29T11:26:04Z","receivedAt":"2022-03-29T11:26:23Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"OK, I\n\nOn Mon, Mar 28, 2022 at 10:27 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Small things accumulate.\n>\n\nOK. I've tried combining Junio's \"only do something when needed\" case\nstatement with Glen's \"co-locate advice-related string manipulations\"\nproposal, and I believe this disqualifies and obviates the need for passing\naround any new \"are we going to be issuing advice\" flag.\n"},{"id":"452555","messageId":"pull.1183.v4.git.1648553209157.gitgitgadget@gmail.com","threadId":"57591","inReplyTo":"pull.1183.v3.git.1648450268285.gitgitgadget@gmail.com","subject":"[PATCH v4] tracking branches: add advice to ambiguous refspec error","fromName":"Tao Klerks via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-29T11:26:48Z","receivedAt":"2022-03-29T11:26:57Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"From: Tao Klerks <tao@klerks.biz>\n\nThe error \"not tracking: ambiguous information for ref\" is raised\nwhen we are evaluating what tracking information to set on a branch,\nand find that the ref to be added as tracking branch is mapped\nunder multiple remotes' fetch refspecs.\n\nThis can easily happen when a user copy-pastes a remote definition\nin their git config, and forgets to change the tracking path.\n\nAdd advice in this situation, explicitly highlighting which remotes\nare involved and suggesting how to correct the situation.\n\nSigned-off-by: Tao Klerks <tao@klerks.biz>\n---\n    tracking branches: add advice to ambiguous refspec error\n    \n    I believe this third version incorporates all Ævar's suggestions, and\n    might be usable. Removed \"RFC\" prefix.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1183%2FTaoK%2Fadvise-ambiguous-tracking-refspec-v4\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1183/TaoK/advise-ambiguous-tracking-refspec-v4\nPull-Request: https://github.com/gitgitgadget/git/pull/1183\n\nRange-diff vs v3:\n\n 1:  22ffe81ac26 = 1:  ac6c782f566 tracking branches: add advice to ambiguous refspec error\n\n\n Documentation/config/advice.txt |  4 +++\n advice.c                        |  1 +\n advice.h                        |  1 +\n branch.c                        | 44 +++++++++++++++++++++++++++++----\n 4 files changed, 45 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt\nindex c40eb09cb7e..90f7dbd03aa 100644\n--- a/Documentation/config/advice.txt\n+++ b/Documentation/config/advice.txt\n@@ -4,6 +4,10 @@ advice.*::\n \tcan tell Git that you do not need help by setting these to 'false':\n +\n --\n+\tambiguousFetchRefspec::\n+\t\tAdvice shown when branch tracking relationship setup fails due\n+\t\tto multiple remotes' refspecs mapping to the same remote\n+\t\ttracking namespace in the repo.\n \tfetchShowForcedUpdates::\n \t\tAdvice shown when linkgit:git-fetch[1] takes a long time\n \t\tto calculate forced updates after ref updates, or to warn\ndiff --git a/advice.c b/advice.c\nindex 2e1fd483040..18ac8739519 100644\n--- a/advice.c\n+++ b/advice.c\n@@ -39,6 +39,7 @@ static struct {\n \t[ADVICE_ADD_EMPTY_PATHSPEC]\t\t\t= { \"addEmptyPathspec\", 1 },\n \t[ADVICE_ADD_IGNORED_FILE]\t\t\t= { \"addIgnoredFile\", 1 },\n \t[ADVICE_AM_WORK_DIR] \t\t\t\t= { \"amWorkDir\", 1 },\n+\t[ADVICE_AMBIGUOUS_FETCH_REFSPEC]\t\t= { \"ambiguousFetchRefspec\", 1 },\n \t[ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME] \t= { \"checkoutAmbiguousRemoteBranchName\", 1 },\n \t[ADVICE_COMMIT_BEFORE_MERGE]\t\t\t= { \"commitBeforeMerge\", 1 },\n \t[ADVICE_DETACHED_HEAD]\t\t\t\t= { \"detachedHead\", 1 },\ndiff --git a/advice.h b/advice.h\nindex a3957123a16..2d4c94f38eb 100644\n--- a/advice.h\n+++ b/advice.h\n@@ -17,6 +17,7 @@ struct string_list;\n \tADVICE_ADD_EMPTY_PATHSPEC,\n \tADVICE_ADD_IGNORED_FILE,\n \tADVICE_AM_WORK_DIR,\n+\tADVICE_AMBIGUOUS_FETCH_REFSPEC,\n \tADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME,\n \tADVICE_COMMIT_BEFORE_MERGE,\n \tADVICE_DETACHED_HEAD,\ndiff --git a/branch.c b/branch.c\nindex 6b31df539a5..5c28d432103 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -18,9 +18,15 @@ struct tracking {\n \tint matches;\n };\n \n+struct find_tracked_branch_cb {\n+\tstruct tracking *tracking;\n+\tstruct strbuf remotes_advice;\n+};\n+\n static int find_tracked_branch(struct remote *remote, void *priv)\n {\n-\tstruct tracking *tracking = priv;\n+\tstruct find_tracked_branch_cb *ftb = priv;\n+\tstruct tracking *tracking = ftb->tracking;\n \n \tif (!remote_find_tracking(remote, &tracking->spec)) {\n \t\tif (++tracking->matches == 1) {\n@@ -30,6 +36,13 @@ static int find_tracked_branch(struct remote *remote, void *priv)\n \t\t\tfree(tracking->spec.src);\n \t\t\tstring_list_clear(tracking->srcs, 0);\n \t\t}\n+\t\t/*\n+\t\t * TRANSLATORS: This is a line listing a remote with duplicate\n+\t\t * refspecs, to be later included in advice message\n+\t\t * ambiguousFetchRefspec. For RTL languages you'll probably want\n+\t\t * to swap the \"%s\" and leading \"  \" space around.\n+\t\t */\n+\t\tstrbuf_addf(&ftb->remotes_advice, _(\"  %s\\n\"), remote->name);\n \t\ttracking->spec.src = NULL;\n \t}\n \n@@ -219,6 +232,7 @@ static int inherit_tracking(struct tracking *tracking, const char *orig_ref)\n \treturn 0;\n }\n \n+\n /*\n  * Used internally to set the branch.<new_ref>.{remote,merge} config\n  * settings so that branch 'new_ref' tracks 'orig_ref'. Unlike\n@@ -232,12 +246,16 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,\n \tstruct tracking tracking;\n \tstruct string_list tracking_srcs = STRING_LIST_INIT_DUP;\n \tint config_flags = quiet ? 0 : BRANCH_CONFIG_VERBOSE;\n+\tstruct find_tracked_branch_cb ftb_cb = {\n+\t\t.tracking = &tracking,\n+\t\t.remotes_advice = STRBUF_INIT,\n+\t};\n \n \tmemset(&tracking, 0, sizeof(tracking));\n \ttracking.spec.dst = (char *)orig_ref;\n \ttracking.srcs = &tracking_srcs;\n \tif (track != BRANCH_TRACK_INHERIT)\n-\t\tfor_each_remote(find_tracked_branch, &tracking);\n+\t\tfor_each_remote(find_tracked_branch, &ftb_cb);\n \telse if (inherit_tracking(&tracking, orig_ref))\n \t\tgoto cleanup;\n \n@@ -252,9 +270,24 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,\n \t\t\tgoto cleanup;\n \t\t}\n \n-\tif (tracking.matches > 1)\n-\t\tdie(_(\"not tracking: ambiguous information for ref %s\"),\n-\t\t    orig_ref);\n+\tif (tracking.matches > 1) {\n+\t\tint status = die_message(_(\"not tracking: ambiguous information for ref %s\"),\n+\t\t\t\t\t    orig_ref);\n+\t\tif (advice_enabled(ADVICE_AMBIGUOUS_FETCH_REFSPEC))\n+\t\t\tadvise(_(\"There are multiple remotes whose fetch refspecs map to the remote\\n\"\n+\t\t\t\t \"tracking ref %s:\\n\"\n+\t\t\t\t \"%s\"\n+\t\t\t\t \"\\n\"\n+\t\t\t\t \"This is typically a configuration error.\\n\"\n+\t\t\t\t \"\\n\"\n+\t\t\t\t \"To support setting up tracking branches, ensure that\\n\"\n+\t\t\t\t \"different remotes' fetch refspecs map into different\\n\"\n+\t\t\t\t \"tracking namespaces.\"),\n+\t\t\t       orig_ref,\n+\t\t\t       ftb_cb.remotes_advice.buf\n+\t\t\t       );\n+\t\texit(status);\n+\t}\n \n \tif (tracking.srcs->nr < 1)\n \t\tstring_list_append(tracking.srcs, orig_ref);\n@@ -263,6 +296,7 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,\n \t\texit(-1);\n \n cleanup:\n+\tstrbuf_release(&ftb_cb.remotes_advice);\n \tstring_list_clear(&tracking_srcs, 0);\n }\n \n\nbase-commit: abf474a5dd901f28013c52155411a48fd4c09922\n-- \ngitgitgadget\n"},{"id":"452556","messageId":"CAPMMpojj81UY8-q2Oh6VeTdreoszfcTmLm6Hh+O5hoObaYw8Zg@mail.gmail.com","threadId":"57591","inReplyTo":"pull.1183.v4.git.1648553209157.gitgitgadget@gmail.com","subject":"Re: [PATCH v4] tracking branches: add advice to ambiguous refspec error","fromName":"Tao Klerks","fromEmail":"tao@klerks.biz","sentAt":"2022-03-29T11:31:40Z","receivedAt":"2022-03-29T11:31:58Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"On Tue, Mar 29, 2022 at 1:26 PM Tao Klerks via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n>\n>     I believe this third version incorporates all Ævar's suggestions, and\n>     might be usable. Removed \"RFC\" prefix.\n\nArgh, sorry about the broken cover letter. This *fourth* version\nattempts to address Junio's concerns over unnecessary extra\nerror-preparation work being done in the very-vastly-more-common\n\"normal\" case.\n"},{"id":"452575","messageId":"xmqqh77gyd0y.fsf@gitster.g","threadId":"57591","inReplyTo":"pull.1183.v4.git.1648553209157.gitgitgadget@gmail.com","subject":"Re: [PATCH v4] tracking branches: add advice to ambiguous refspec error","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-29T15:49:33Z","receivedAt":"2022-03-29T15:49:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Tao Klerks via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Tao Klerks <tao@klerks.biz>\n>\n> The error \"not tracking: ambiguous information for ref\" is raised\n> when we are evaluating what tracking information to set on a branch,\n> and find that the ref to be added as tracking branch is mapped\n> under multiple remotes' fetch refspecs.\n>\n> This can easily happen when a user copy-pastes a remote definition\n> in their git config, and forgets to change the tracking path.\n>\n> Add advice in this situation, explicitly highlighting which remotes\n> are involved and suggesting how to correct the situation.\n>\n> Signed-off-by: Tao Klerks <tao@klerks.biz>\n> ---\n>     tracking branches: add advice to ambiguous refspec error\n>     \n>     I believe this third version incorporates all Ævar's suggestions, and\n>     might be usable. Removed \"RFC\" prefix.\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1183%2FTaoK%2Fadvise-ambiguous-tracking-refspec-v4\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1183/TaoK/advise-ambiguous-tracking-refspec-v4\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1183\n>\n> Range-diff vs v3:\n>\n>  1:  22ffe81ac26 = 1:  ac6c782f566 tracking branches: add advice to ambiguous refspec error\n>\n>\n>  Documentation/config/advice.txt |  4 +++\n>  advice.c                        |  1 +\n>  advice.h                        |  1 +\n>  branch.c                        | 44 +++++++++++++++++++++++++++++----\n>  4 files changed, 45 insertions(+), 5 deletions(-)\n\nSent a wrong version?\n\nThe patch text seems to be identical to that of v3 message archived at\nhttps://lore.kernel.org/git/pull.1183.v3.git.1648450268285.gitgitgadget@gmail.com/\n"},{"id":"452639","messageId":"CAPMMpojsepWTf23Tm7uEGK54UabJUz43Yj1UfbpipVniPHScbA@mail.gmail.com","threadId":"57591","inReplyTo":"xmqqh77gyd0y.fsf@gitster.g","subject":"Re: [PATCH v4] tracking branches: add advice to ambiguous refspec error","fromName":"Tao Klerks","fromEmail":"tao@klerks.biz","sentAt":"2022-03-30T04:17:23Z","receivedAt":"2022-03-30T04:19:49Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"On Tue, Mar 29, 2022 at 5:49 PM Junio C Hamano <gitster@pobox.com> wrote:\n[...]\n>\n> Sent a wrong version?\n[...]\n\nYes indeed. My apologies once again. V5 coming as soon as the test run\ncompletes.\n"},{"id":"452658","messageId":"pull.1183.v5.git.1648624810866.gitgitgadget@gmail.com","threadId":"57591","inReplyTo":"pull.1183.v4.git.1648553209157.gitgitgadget@gmail.com","subject":"[PATCH v5] tracking branches: add advice to ambiguous refspec error","fromName":"Tao Klerks via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-30T07:20:10Z","receivedAt":"2022-03-30T07:20:27Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"From: Tao Klerks <tao@klerks.biz>\n\nThe error \"not tracking: ambiguous information for ref\" is raised\nwhen we are evaluating what tracking information to set on a branch,\nand find that the ref to be added as tracking branch is mapped\nunder multiple remotes' fetch refspecs.\n\nThis can easily happen when a user copy-pastes a remote definition\nin their git config, and forgets to change the tracking path.\n\nAdd advice in this situation, explicitly highlighting which remotes\nare involved and suggesting how to correct the situation.\n\nSigned-off-by: Tao Klerks <tao@klerks.biz>\n---\n    tracking branches: add advice to ambiguous refspec error\n    \n    This fifth version attempts to address Junio's concerns over unnecessary\n    extra error-preparation work being done in the very-vastly-more-common\n    \"normal\" case.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1183%2FTaoK%2Fadvise-ambiguous-tracking-refspec-v5\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1183/TaoK/advise-ambiguous-tracking-refspec-v5\nPull-Request: https://github.com/gitgitgadget/git/pull/1183\n\nRange-diff vs v4:\n\n 1:  ac6c782f566 ! 1:  4478eaed6df tracking branches: add advice to ambiguous refspec error\n     @@ Documentation/config/advice.txt: advice.*::\n       +\n       --\n      +\tambiguousFetchRefspec::\n     -+\t\tAdvice shown when branch tracking relationship setup fails due\n     -+\t\tto multiple remotes' refspecs mapping to the same remote\n     -+\t\ttracking namespace in the repo.\n     ++\t\tAdvice shown when fetch refspec for multiple remotes map to\n     ++\t\tthe same remote-tracking branch namespace and causes branch\n     ++\t\ttracking set-up to fail.\n       \tfetchShowForcedUpdates::\n       \t\tAdvice shown when linkgit:git-fetch[1] takes a long time\n       \t\tto calculate forced updates after ref updates, or to warn\n     @@ branch.c: struct tracking {\n       \n      +struct find_tracked_branch_cb {\n      +\tstruct tracking *tracking;\n     -+\tstruct strbuf remotes_advice;\n     ++\tstruct string_list ambiguous_remotes;\n      +};\n      +\n       static int find_tracked_branch(struct remote *remote, void *priv)\n     @@ branch.c: struct tracking {\n      +\tstruct tracking *tracking = ftb->tracking;\n       \n       \tif (!remote_find_tracking(remote, &tracking->spec)) {\n     - \t\tif (++tracking->matches == 1) {\n     -@@ branch.c: static int find_tracked_branch(struct remote *remote, void *priv)\n     +-\t\tif (++tracking->matches == 1) {\n     ++\t\tswitch (++tracking->matches) {\n     ++\t\tcase 1:\n     + \t\t\tstring_list_append(tracking->srcs, tracking->spec.src);\n     + \t\t\ttracking->remote = remote->name;\n     +-\t\t} else {\n     ++\t\t\tbreak;\n     ++\t\tcase 2:\n     ++\t\t\t// there are at least two remotes; backfill the first one\n     ++\t\t\tstring_list_append(&ftb->ambiguous_remotes, tracking->spec.src);\n     ++\t\t\t/* fall through */\n     ++\t\tdefault:\n     ++\t\t\tstring_list_append(&ftb->ambiguous_remotes, remote->name);\n       \t\t\tfree(tracking->spec.src);\n       \t\t\tstring_list_clear(tracking->srcs, 0);\n     ++\t\tbreak;\n       \t\t}\n     -+\t\t/*\n     -+\t\t * TRANSLATORS: This is a line listing a remote with duplicate\n     -+\t\t * refspecs, to be later included in advice message\n     -+\t\t * ambiguousFetchRefspec. For RTL languages you'll probably want\n     -+\t\t * to swap the \"%s\" and leading \"  \" space around.\n     -+\t\t */\n     -+\t\tstrbuf_addf(&ftb->remotes_advice, _(\"  %s\\n\"), remote->name);\n       \t\ttracking->spec.src = NULL;\n       \t}\n     - \n      @@ branch.c: static int inherit_tracking(struct tracking *tracking, const char *orig_ref)\n       \treturn 0;\n       }\n     @@ branch.c: static void setup_tracking(const char *new_ref, const char *orig_ref,\n       \tint config_flags = quiet ? 0 : BRANCH_CONFIG_VERBOSE;\n      +\tstruct find_tracked_branch_cb ftb_cb = {\n      +\t\t.tracking = &tracking,\n     -+\t\t.remotes_advice = STRBUF_INIT,\n     ++\t\t.ambiguous_remotes = STRING_LIST_INIT_DUP,\n      +\t};\n       \n       \tmemset(&tracking, 0, sizeof(tracking));\n     @@ branch.c: static void setup_tracking(const char *new_ref, const char *orig_ref,\n      +\tif (tracking.matches > 1) {\n      +\t\tint status = die_message(_(\"not tracking: ambiguous information for ref %s\"),\n      +\t\t\t\t\t    orig_ref);\n     -+\t\tif (advice_enabled(ADVICE_AMBIGUOUS_FETCH_REFSPEC))\n     ++\t\tif (advice_enabled(ADVICE_AMBIGUOUS_FETCH_REFSPEC)) {\n     ++\t\t\tstruct strbuf remotes_advice = STRBUF_INIT;\n     ++\t\t\tstruct string_list_item *item;\n     ++\n     ++\t\t\tfor_each_string_list_item(item, &ftb_cb.ambiguous_remotes) {\n     ++\t\t\t\t/*\n     ++\t\t\t\t * TRANSLATORS: This is a line listing a remote with duplicate\n     ++\t\t\t\t * refspecs in the advice message below. For RTL languages you'll\n     ++\t\t\t\t * probably want to swap the \"%s\" and leading \"  \" space around.\n     ++\t\t\t\t */\n     ++\t\t\t\tstrbuf_addf(&remotes_advice, _(\"  %s\\n\"), item->string);\n     ++\t\t\t}\n     ++\n      +\t\t\tadvise(_(\"There are multiple remotes whose fetch refspecs map to the remote\\n\"\n      +\t\t\t\t \"tracking ref %s:\\n\"\n      +\t\t\t\t \"%s\"\n     @@ branch.c: static void setup_tracking(const char *new_ref, const char *orig_ref,\n      +\t\t\t\t \"different remotes' fetch refspecs map into different\\n\"\n      +\t\t\t\t \"tracking namespaces.\"),\n      +\t\t\t       orig_ref,\n     -+\t\t\t       ftb_cb.remotes_advice.buf\n     ++\t\t\t       remotes_advice.buf\n      +\t\t\t       );\n     ++\t\t\tstrbuf_release(&remotes_advice);\n     ++\t\t}\n      +\t\texit(status);\n      +\t}\n       \n       \tif (tracking.srcs->nr < 1)\n       \t\tstring_list_append(tracking.srcs, orig_ref);\n      @@ branch.c: static void setup_tracking(const char *new_ref, const char *orig_ref,\n     - \t\texit(-1);\n       \n       cleanup:\n     -+\tstrbuf_release(&ftb_cb.remotes_advice);\n       \tstring_list_clear(&tracking_srcs, 0);\n     ++\tstring_list_clear(&ftb_cb.ambiguous_remotes, 0);\n       }\n       \n     + int read_branch_desc(struct strbuf *buf, const char *branch_name)\n\n\n Documentation/config/advice.txt |  4 +++\n advice.c                        |  1 +\n advice.h                        |  1 +\n branch.c                        | 63 +++++++++++++++++++++++++++++----\n 4 files changed, 62 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt\nindex c40eb09cb7e..343d271c707 100644\n--- a/Documentation/config/advice.txt\n+++ b/Documentation/config/advice.txt\n@@ -4,6 +4,10 @@ advice.*::\n \tcan tell Git that you do not need help by setting these to 'false':\n +\n --\n+\tambiguousFetchRefspec::\n+\t\tAdvice shown when fetch refspec for multiple remotes map to\n+\t\tthe same remote-tracking branch namespace and causes branch\n+\t\ttracking set-up to fail.\n \tfetchShowForcedUpdates::\n \t\tAdvice shown when linkgit:git-fetch[1] takes a long time\n \t\tto calculate forced updates after ref updates, or to warn\ndiff --git a/advice.c b/advice.c\nindex 2e1fd483040..18ac8739519 100644\n--- a/advice.c\n+++ b/advice.c\n@@ -39,6 +39,7 @@ static struct {\n \t[ADVICE_ADD_EMPTY_PATHSPEC]\t\t\t= { \"addEmptyPathspec\", 1 },\n \t[ADVICE_ADD_IGNORED_FILE]\t\t\t= { \"addIgnoredFile\", 1 },\n \t[ADVICE_AM_WORK_DIR] \t\t\t\t= { \"amWorkDir\", 1 },\n+\t[ADVICE_AMBIGUOUS_FETCH_REFSPEC]\t\t= { \"ambiguousFetchRefspec\", 1 },\n \t[ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME] \t= { \"checkoutAmbiguousRemoteBranchName\", 1 },\n \t[ADVICE_COMMIT_BEFORE_MERGE]\t\t\t= { \"commitBeforeMerge\", 1 },\n \t[ADVICE_DETACHED_HEAD]\t\t\t\t= { \"detachedHead\", 1 },\ndiff --git a/advice.h b/advice.h\nindex a3957123a16..2d4c94f38eb 100644\n--- a/advice.h\n+++ b/advice.h\n@@ -17,6 +17,7 @@ struct string_list;\n \tADVICE_ADD_EMPTY_PATHSPEC,\n \tADVICE_ADD_IGNORED_FILE,\n \tADVICE_AM_WORK_DIR,\n+\tADVICE_AMBIGUOUS_FETCH_REFSPEC,\n \tADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME,\n \tADVICE_COMMIT_BEFORE_MERGE,\n \tADVICE_DETACHED_HEAD,\ndiff --git a/branch.c b/branch.c\nindex 6b31df539a5..e6ab50fff41 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -18,17 +18,31 @@ struct tracking {\n \tint matches;\n };\n \n+struct find_tracked_branch_cb {\n+\tstruct tracking *tracking;\n+\tstruct string_list ambiguous_remotes;\n+};\n+\n static int find_tracked_branch(struct remote *remote, void *priv)\n {\n-\tstruct tracking *tracking = priv;\n+\tstruct find_tracked_branch_cb *ftb = priv;\n+\tstruct tracking *tracking = ftb->tracking;\n \n \tif (!remote_find_tracking(remote, &tracking->spec)) {\n-\t\tif (++tracking->matches == 1) {\n+\t\tswitch (++tracking->matches) {\n+\t\tcase 1:\n \t\t\tstring_list_append(tracking->srcs, tracking->spec.src);\n \t\t\ttracking->remote = remote->name;\n-\t\t} else {\n+\t\t\tbreak;\n+\t\tcase 2:\n+\t\t\t// there are at least two remotes; backfill the first one\n+\t\t\tstring_list_append(&ftb->ambiguous_remotes, tracking->spec.src);\n+\t\t\t/* fall through */\n+\t\tdefault:\n+\t\t\tstring_list_append(&ftb->ambiguous_remotes, remote->name);\n \t\t\tfree(tracking->spec.src);\n \t\t\tstring_list_clear(tracking->srcs, 0);\n+\t\tbreak;\n \t\t}\n \t\ttracking->spec.src = NULL;\n \t}\n@@ -219,6 +233,7 @@ static int inherit_tracking(struct tracking *tracking, const char *orig_ref)\n \treturn 0;\n }\n \n+\n /*\n  * Used internally to set the branch.<new_ref>.{remote,merge} config\n  * settings so that branch 'new_ref' tracks 'orig_ref'. Unlike\n@@ -232,12 +247,16 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,\n \tstruct tracking tracking;\n \tstruct string_list tracking_srcs = STRING_LIST_INIT_DUP;\n \tint config_flags = quiet ? 0 : BRANCH_CONFIG_VERBOSE;\n+\tstruct find_tracked_branch_cb ftb_cb = {\n+\t\t.tracking = &tracking,\n+\t\t.ambiguous_remotes = STRING_LIST_INIT_DUP,\n+\t};\n \n \tmemset(&tracking, 0, sizeof(tracking));\n \ttracking.spec.dst = (char *)orig_ref;\n \ttracking.srcs = &tracking_srcs;\n \tif (track != BRANCH_TRACK_INHERIT)\n-\t\tfor_each_remote(find_tracked_branch, &tracking);\n+\t\tfor_each_remote(find_tracked_branch, &ftb_cb);\n \telse if (inherit_tracking(&tracking, orig_ref))\n \t\tgoto cleanup;\n \n@@ -252,9 +271,38 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,\n \t\t\tgoto cleanup;\n \t\t}\n \n-\tif (tracking.matches > 1)\n-\t\tdie(_(\"not tracking: ambiguous information for ref %s\"),\n-\t\t    orig_ref);\n+\tif (tracking.matches > 1) {\n+\t\tint status = die_message(_(\"not tracking: ambiguous information for ref %s\"),\n+\t\t\t\t\t    orig_ref);\n+\t\tif (advice_enabled(ADVICE_AMBIGUOUS_FETCH_REFSPEC)) {\n+\t\t\tstruct strbuf remotes_advice = STRBUF_INIT;\n+\t\t\tstruct string_list_item *item;\n+\n+\t\t\tfor_each_string_list_item(item, &ftb_cb.ambiguous_remotes) {\n+\t\t\t\t/*\n+\t\t\t\t * TRANSLATORS: This is a line listing a remote with duplicate\n+\t\t\t\t * refspecs in the advice message below. For RTL languages you'll\n+\t\t\t\t * probably want to swap the \"%s\" and leading \"  \" space around.\n+\t\t\t\t */\n+\t\t\t\tstrbuf_addf(&remotes_advice, _(\"  %s\\n\"), item->string);\n+\t\t\t}\n+\n+\t\t\tadvise(_(\"There are multiple remotes whose fetch refspecs map to the remote\\n\"\n+\t\t\t\t \"tracking ref %s:\\n\"\n+\t\t\t\t \"%s\"\n+\t\t\t\t \"\\n\"\n+\t\t\t\t \"This is typically a configuration error.\\n\"\n+\t\t\t\t \"\\n\"\n+\t\t\t\t \"To support setting up tracking branches, ensure that\\n\"\n+\t\t\t\t \"different remotes' fetch refspecs map into different\\n\"\n+\t\t\t\t \"tracking namespaces.\"),\n+\t\t\t       orig_ref,\n+\t\t\t       remotes_advice.buf\n+\t\t\t       );\n+\t\t\tstrbuf_release(&remotes_advice);\n+\t\t}\n+\t\texit(status);\n+\t}\n \n \tif (tracking.srcs->nr < 1)\n \t\tstring_list_append(tracking.srcs, orig_ref);\n@@ -264,6 +312,7 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,\n \n cleanup:\n \tstring_list_clear(&tracking_srcs, 0);\n+\tstring_list_clear(&ftb_cb.ambiguous_remotes, 0);\n }\n \n int read_branch_desc(struct strbuf *buf, const char *branch_name)\n\nbase-commit: abf474a5dd901f28013c52155411a48fd4c09922\n-- \ngitgitgadget\n"},{"id":"452663","messageId":"220330.864k3fpo32.gmgdl@evledraar.gmail.com","threadId":"57591","inReplyTo":"pull.1183.v5.git.1648624810866.gitgitgadget@gmail.com","subject":"Re: [PATCH v5] tracking branches: add advice to ambiguous refspec error","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-03-30T13:19:47Z","receivedAt":"2022-03-30T13:27:52Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Mar 30 2022, Tao Klerks via GitGitGadget wrote:\n\n> From: Tao Klerks <tao@klerks.biz>\n\n> +\t\tcase 2:\n> +\t\t\t// there are at least two remotes; backfill the first one\n\nNit: I think it's been Junio's preference not to introduce C99 comments,\ndespite other C99 features now being used (and I think it should work in\npractice as far as portability goes, see\nhttps://lore.kernel.org/git/87wnmwpwyf.fsf@evledraar.gmail.com/)\n\n> @@ -219,6 +233,7 @@ static int inherit_tracking(struct tracking *tracking, const char *orig_ref)\n>  \treturn 0;\n>  }\n>  \n> +\n\nStray whitespace?\n\n>  /*\n>   * Used internally to set the branch.<new_ref>.{remote,merge} config\n>   * settings so that branch 'new_ref' tracks 'orig_ref'. Unlike\n> @@ -232,12 +247,16 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,\n>  \tstruct tracking tracking;\n>  \tstruct string_list tracking_srcs = STRING_LIST_INIT_DUP;\n>  \tint config_flags = quiet ? 0 : BRANCH_CONFIG_VERBOSE;\n> +\tstruct find_tracked_branch_cb ftb_cb = {\n> +\t\t.tracking = &tracking,\n> +\t\t.ambiguous_remotes = STRING_LIST_INIT_DUP,\n> +\t};\n>  \n>  \tmemset(&tracking, 0, sizeof(tracking));\n>  \ttracking.spec.dst = (char *)orig_ref;\n>  \ttracking.srcs = &tracking_srcs;\n>  \tif (track != BRANCH_TRACK_INHERIT)\n> -\t\tfor_each_remote(find_tracked_branch, &tracking);\n> +\t\tfor_each_remote(find_tracked_branch, &ftb_cb);\n>  \telse if (inherit_tracking(&tracking, orig_ref))\n>  \t\tgoto cleanup;\n>  \n> @@ -252,9 +271,38 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,\n>  \t\t\tgoto cleanup;\n>  \t\t}\n>  \n> -\tif (tracking.matches > 1)\n> -\t\tdie(_(\"not tracking: ambiguous information for ref %s\"),\n> -\t\t    orig_ref);\n> +\tif (tracking.matches > 1) {\n> +\t\tint status = die_message(_(\"not tracking: ambiguous information for ref %s\"),\n> +\t\t\t\t\t    orig_ref);\n\nThis isn't per-se new, but I wonder if while we're at it we shold just\nquote '%s' here, which we'd usually do. I.e. this message isn't new, but\nreferring again to \"ref %s\" (and not \"ref '%s'\") below is.\n\n> +\t\tif (advice_enabled(ADVICE_AMBIGUOUS_FETCH_REFSPEC)) {\n> +\t\t\tstruct strbuf remotes_advice = STRBUF_INIT;\n> +\t\t\tstruct string_list_item *item;\n> +\n> +\t\t\tfor_each_string_list_item(item, &ftb_cb.ambiguous_remotes) {\n\nNit: drop braces, not needed.\n\n> +\t\t\t\t/*\n> +\t\t\t\t * TRANSLATORS: This is a line listing a remote with duplicate\n> +\t\t\t\t * refspecs in the advice message below. For RTL languages you'll\n> +\t\t\t\t * probably want to swap the \"%s\" and leading \"  \" space around.\n> +\t\t\t\t */\n> +\t\t\t\tstrbuf_addf(&remotes_advice, _(\"  %s\\n\"), item->string);\n> +\t\t\t}\n> +\n\nPer the TRANSLATORS comments in get_short_oid(), it's also good to have\none here in front of advice(). E.g.:\n\n\t/*\n\t * TRANSLATORS: The second argument is a \\n-delimited list of\n\t * duplicate refspecs, composed above.\n\t*/\n\n> +\t\t\tadvise(_(\"There are multiple remotes whose fetch refspecs map to the remote\\n\"\n> +\t\t\t\t \"tracking ref %s:\\n\"\n> +\t\t\t\t \"%s\"\n> +\t\t\t\t \"\\n\"\n> +\t\t\t\t \"This is typically a configuration error.\\n\"\n> +\t\t\t\t \"\\n\"\n> +\t\t\t\t \"To support setting up tracking branches, ensure that\\n\"\n> +\t\t\t\t \"different remotes' fetch refspecs map into different\\n\"\n> +\t\t\t\t \"tracking namespaces.\"),\n> +\t\t\t       orig_ref,\n> +\t\t\t       remotes_advice.buf\n> +\t\t\t       );\n\nNit: The usual style for multi-line arguments is to \"fill\" lines until\nyou're at 79 characters, so these last three lines (including the \");\")\ncan all go on the \"tracking namespaces\" line (until they're at 79, then\nwrap)>\n\n> +\t\t\tstrbuf_release(&remotes_advice);\n> +\t\t}\n> +\t\texit(status);\n> +\t}\n\nOther than the minor nits noted above this version looks good to me, and\nI think addresses both the translation concerns I brought up, and the\n\"let's not do advice() work if we don't need it\" concern by Junio et al.\n\n>  \tif (tracking.srcs->nr < 1)\n>  \t\tstring_list_append(tracking.srcs, orig_ref);\n> @@ -264,6 +312,7 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,\n>  \n>  cleanup:\n>  \tstring_list_clear(&tracking_srcs, 0);\n> +\tstring_list_clear(&ftb_cb.ambiguous_remotes, 0);\n>  }\n>  \n>  int read_branch_desc(struct strbuf *buf, const char *branch_name)\n>\n> base-commit: abf474a5dd901f28013c52155411a48fd4c09922\n\n"},{"id":"452666","messageId":"CAPMMpoiNZ8pvrZmMqJsRV2x+Fvq-=hpih1GGSLMe=KX785Dk0A@mail.gmail.com","threadId":"57591","inReplyTo":"220330.864k3fpo32.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v5] tracking branches: add advice to ambiguous refspec error","fromName":"Tao Klerks","fromEmail":"tao@klerks.biz","sentAt":"2022-03-30T14:23:07Z","receivedAt":"2022-03-30T14:23:23Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"On Wed, Mar 30, 2022 at 3:27 PM Ævar Arnfjörð Bjarmason\n<avarab@gmail.com> wrote:\n>\n>\n> On Wed, Mar 30 2022, Tao Klerks via GitGitGadget wrote:\n>\n> Nit: I think it's been Junio's preference not to introduce C99 comments,\n\nArgh, my apologies, bad habits.\n\nI wonder whether anyone has any tooling to help catch this kind of\nthing - I'll ask in GitGitGadget, anyway.\n\n> > @@ -219,6 +233,7 @@ static int inherit_tracking(struct tracking *tracking, const char *orig_ref)\n> >       return 0;\n> >  }\n> >\n> > +\n>\n> Stray whitespace?\n\nThx, I will check more carefully next time.\n\n>\n> >  /*\n> >   * Used internally to set the branch.<new_ref>.{remote,merge} config\n> >   * settings so that branch 'new_ref' tracks 'orig_ref'. Unlike\n> > @@ -232,12 +247,16 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,\n> >       struct tracking tracking;\n> >       struct string_list tracking_srcs = STRING_LIST_INIT_DUP;\n> >       int config_flags = quiet ? 0 : BRANCH_CONFIG_VERBOSE;\n> > +     struct find_tracked_branch_cb ftb_cb = {\n> > +             .tracking = &tracking,\n> > +             .ambiguous_remotes = STRING_LIST_INIT_DUP,\n> > +     };\n> >\n> >       memset(&tracking, 0, sizeof(tracking));\n> >       tracking.spec.dst = (char *)orig_ref;\n> >       tracking.srcs = &tracking_srcs;\n> >       if (track != BRANCH_TRACK_INHERIT)\n> > -             for_each_remote(find_tracked_branch, &tracking);\n> > +             for_each_remote(find_tracked_branch, &ftb_cb);\n> >       else if (inherit_tracking(&tracking, orig_ref))\n> >               goto cleanup;\n> >\n> > @@ -252,9 +271,38 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,\n> >                       goto cleanup;\n> >               }\n> >\n> > -     if (tracking.matches > 1)\n> > -             die(_(\"not tracking: ambiguous information for ref %s\"),\n> > -                 orig_ref);\n> > +     if (tracking.matches > 1) {\n> > +             int status = die_message(_(\"not tracking: ambiguous information for ref %s\"),\n> > +                                         orig_ref);\n>\n> This isn't per-se new, but I wonder if while we're at it we shold just\n> quote '%s' here, which we'd usually do. I.e. this message isn't new, but\n> referring again to \"ref %s\" (and not \"ref '%s'\") below is.\n>\n\nI will fix the below, but I would default to not changing the above\nunless someone thinks we should (not sure what the expectations are\naround changing error messages unnecessarily)\n\n> > +             if (advice_enabled(ADVICE_AMBIGUOUS_FETCH_REFSPEC)) {\n> > +                     struct strbuf remotes_advice = STRBUF_INIT;\n> > +                     struct string_list_item *item;\n> > +\n> > +                     for_each_string_list_item(item, &ftb_cb.ambiguous_remotes) {\n>\n> Nit: drop braces, not needed.\n\nThx, will do.\n\n>\n> > +                             /*\n> > +                              * TRANSLATORS: This is a line listing a remote with duplicate\n> > +                              * refspecs in the advice message below. For RTL languages you'll\n> > +                              * probably want to swap the \"%s\" and leading \"  \" space around.\n> > +                              */\n> > +                             strbuf_addf(&remotes_advice, _(\"  %s\\n\"), item->string);\n> > +                     }\n> > +\n>\n> Per the TRANSLATORS comments in get_short_oid(), it's also good to have\n> one here in front of advice(). E.g.:\n>\n>         /*\n>          * TRANSLATORS: The second argument is a \\n-delimited list of\n>          * duplicate refspecs, composed above.\n>         */\n>\n\nWill do, thx.\n\n> > +                     advise(_(\"There are multiple remotes whose fetch refspecs map to the remote\\n\"\n> > +                              \"tracking ref %s:\\n\"\n> > +                              \"%s\"\n> > +                              \"\\n\"\n> > +                              \"This is typically a configuration error.\\n\"\n> > +                              \"\\n\"\n> > +                              \"To support setting up tracking branches, ensure that\\n\"\n> > +                              \"different remotes' fetch refspecs map into different\\n\"\n> > +                              \"tracking namespaces.\"),\n> > +                            orig_ref,\n> > +                            remotes_advice.buf\n> > +                            );\n>\n> Nit: The usual style for multi-line arguments is to \"fill\" lines until\n> you're at 79 characters, so these last three lines (including the \");\")\n> can all go on the \"tracking namespaces\" line (until they're at 79, then\n> wrap)>\n>\n\nYep, another oversight, thx.\n\n> > +                     strbuf_release(&remotes_advice);\n> > +             }\n> > +             exit(status);\n> > +     }\n>\n> Other than the minor nits noted above this version looks good to me, and\n> I think addresses both the translation concerns I brought up, and the\n> \"let's not do advice() work if we don't need it\" concern by Junio et al.\n>\n\nAwesome, thx!\n"},{"id":"452667","messageId":"CAPMMpohJEbfA0_0qR8jqOAXgBuwo+Nf=BkVjr0UkjeMa+STHkw@mail.gmail.com","threadId":"57591","inReplyTo":"CAPMMpoiNZ8pvrZmMqJsRV2x+Fvq-=hpih1GGSLMe=KX785Dk0A@mail.gmail.com","subject":"Re: [PATCH v5] tracking branches: add advice to ambiguous refspec error","fromName":"Tao Klerks","fromEmail":"tao@klerks.biz","sentAt":"2022-03-30T15:18:31Z","receivedAt":"2022-03-30T15:18:49Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"On Wed, Mar 30, 2022 at 4:23 PM Tao Klerks <tao@klerks.biz> wrote:\n>\n> On Wed, Mar 30, 2022 at 3:27 PM Ævar Arnfjörð Bjarmason\n> <avarab@gmail.com> wrote:\n> >\n> >\n> > On Wed, Mar 30 2022, Tao Klerks via GitGitGadget wrote:\n> >\n> > > +     if (tracking.matches > 1) {\n> > > +             int status = die_message(_(\"not tracking: ambiguous information for ref %s\"),\n> > > +                                         orig_ref);\n> >\n> > This isn't per-se new, but I wonder if while we're at it we shold just\n> > quote '%s' here, which we'd usually do. I.e. this message isn't new, but\n> > referring again to \"ref %s\" (and not \"ref '%s'\") below is.\n> >\n>\n> I will fix the below, but I would default to not changing the above\n> unless someone thinks we should (not sure what the expectations are\n> around changing error messages unnecessarily)\n\nI take this back. I will update both - the change is in a \"variable\"\npart of the message anyway, and it's hard to imagine any tool\nactively/purposefully parsing the ref out of the message and being\ncaught out by the new quotes. Any coordinating tool would already know\nwhat ref was being branched / having its tracking remote info updated.\n\n>\n> > > +                              * TRANSLATORS: This is a line listing a remote with duplicate\n> > > +                              * refspecs in the advice message below. For RTL languages you'll\n> > > +                              * probably want to swap the \"%s\" and leading \"  \" space around.\n> > > +                              */\n> > > +                             strbuf_addf(&remotes_advice, _(\"  %s\\n\"), item->string);\n> > > +                     }\n> > > +\n> >\n"},{"id":"452672","messageId":"220330.86v8vvnyyv.gmgdl@evledraar.gmail.com","threadId":"57591","inReplyTo":"CAPMMpohJEbfA0_0qR8jqOAXgBuwo+Nf=BkVjr0UkjeMa+STHkw@mail.gmail.com","subject":"Re: [PATCH v5] tracking branches: add advice to ambiguous refspec error","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-03-30T17:14:58Z","receivedAt":"2022-03-30T17:15:43Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Mar 30 2022, Tao Klerks wrote:\n\n> On Wed, Mar 30, 2022 at 4:23 PM Tao Klerks <tao@klerks.biz> wrote:\n>>\n>> On Wed, Mar 30, 2022 at 3:27 PM Ævar Arnfjörð Bjarmason\n>> <avarab@gmail.com> wrote:\n>> >\n>> >\n>> > On Wed, Mar 30 2022, Tao Klerks via GitGitGadget wrote:\n>> >\n>> > > +     if (tracking.matches > 1) {\n>> > > +             int status = die_message(_(\"not tracking: ambiguous information for ref %s\"),\n>> > > +                                         orig_ref);\n>> >\n>> > This isn't per-se new, but I wonder if while we're at it we shold just\n>> > quote '%s' here, which we'd usually do. I.e. this message isn't new, but\n>> > referring again to \"ref %s\" (and not \"ref '%s'\") below is.\n>> >\n>>\n>> I will fix the below, but I would default to not changing the above\n>> unless someone thinks we should (not sure what the expectations are\n>> around changing error messages unnecessarily)\n>\n> I take this back. I will update both - the change is in a \"variable\"\n> part of the message anyway, and it's hard to imagine any tool\n> actively/purposefully parsing the ref out of the message and being\n> caught out by the new quotes. Any coordinating tool would already know\n> what ref was being branched / having its tracking remote info updated.\n\nThanks, I'd be fine either way, it was just a suggestion.\n\nAside from what we do here we don't support third-party tooling that's\ngrepping our specific human-readable output, so changing any such\nmessaging is OK.\n"},{"id":"452702","messageId":"xmqqmth76use.fsf@gitster.g","threadId":"57591","inReplyTo":"220330.864k3fpo32.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v5] tracking branches: add advice to ambiguous refspec error","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-30T20:37:53Z","receivedAt":"2022-03-30T20:38:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n> On Wed, Mar 30 2022, Tao Klerks via GitGitGadget wrote:\n>\n>> From: Tao Klerks <tao@klerks.biz>\n>\n>> +\t\tcase 2:\n>> +\t\t\t// there are at least two remotes; backfill the first one\n>\n> Nit: I think it's been Junio's preference not to introduce C99 comments,\n\nI often mention the rule to new contributors, simply because it has\nbeen that way in our code base, regardless of what my personal\npreference might be, and sticking to the style will be more\nconsistent.  It's not like I am forcing my personal preference on\ndevelopers.  Do not mislead new people into thinking so.  It is\nespecially irritating to see ...\n\n> despite other C99 features now being used (and I think it should work in\n> practice as far as portability goes, see\n> https://lore.kernel.org/git/87wnmwpwyf.fsf@evledraar.gmail.com/)\n\n... a mention like this, when you know that it is not about\nportability but is about consistency, and also you know I've\nmentioned more than once on the list if we want to loosen some\nwritten CodingGuidelines rules, especially those that tools do not\nnecessarily catch.\n\n>> +\tif (tracking.matches > 1) {\n>> +\t\tint status = die_message(_(\"not tracking: ambiguous information for ref %s\"),\n>> +\t\t\t\t\t    orig_ref);\n>\n> This isn't per-se new, but I wonder if while we're at it we shold just\n> quote '%s' here, which we'd usually do. I.e. this message isn't new, but\n> referring again to \"ref %s\" (and not \"ref '%s'\") below is.\n\nGood.\n\n>> +\t\t\t\t \"To support setting up tracking branches, ensure that\\n\"\n>> +\t\t\t\t \"different remotes' fetch refspecs map into different\\n\"\n>> +\t\t\t\t \"tracking namespaces.\"),\n>> +\t\t\t       orig_ref,\n>> +\t\t\t       remotes_advice.buf\n>> +\t\t\t       );\n>\n> Nit: The usual style for multi-line arguments is to \"fill\" lines until\n> you're at 79 characters, so these last three lines (including the \");\")\n> can all go on the \"tracking namespaces\" line (until they're at 79, then\n> wrap)>\n\nI didn't know about the magic \"79\" number.  It makes the resulting\nsource code extremely hard to read, though, while making it easier\nto grep for specific messages.\n\n"},{"id":"452704","messageId":"220330.86o81nta6k.gmgdl@evledraar.gmail.com","threadId":"57591","inReplyTo":"xmqqmth76use.fsf@gitster.g","subject":"Re: [PATCH v5] tracking branches: add advice to ambiguous refspec error","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-03-30T21:09:49Z","receivedAt":"2022-03-30T21:14:40Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Mar 30 2022, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n>\n>> On Wed, Mar 30 2022, Tao Klerks via GitGitGadget wrote:\n>>\n>>> From: Tao Klerks <tao@klerks.biz>\n>>\n>>> +\t\tcase 2:\n>>> +\t\t\t// there are at least two remotes; backfill the first one\n>>\n>> Nit: I think it's been Junio's preference not to introduce C99 comments,\n>\n> I often mention the rule to new contributors, simply because it has\n> been that way in our code base, regardless of what my personal\n> preference might be, and sticking to the style will be more\n> consistent.  It's not like I am forcing my personal preference on\n> developers.  Do not mislead new people into thinking so.  It is\n> especially irritating to see ...\n>\n>> despite other C99 features now being used (and I think it should work in\n>> practice as far as portability goes, see\n>> https://lore.kernel.org/git/87wnmwpwyf.fsf@evledraar.gmail.com/)\n>\n> ... a mention like this, when you know that it is not about\n> portability but is about consistency, and also you know I've\n> mentioned more than once on the list if we want to loosen some\n> written CodingGuidelines rules, especially those that tools do not\n> necessarily catch.\n\nI know, and I didn't mean to imply that you were standing in the way of\nC99 progress or anything like that.\n\nPersonally, I much prefer not to have //-style comments introduced. I\nmerely mentioned the portability concern because I thought it helped\ngive a relatively new contributor some context.\n\nI.e. that despite any new developments on the C99 front they might\ndiscover this particular thing is stylistic and not about portability\n(although that may also have been a reason in the past).\n\n>>> +\tif (tracking.matches > 1) {\n>>> +\t\tint status = die_message(_(\"not tracking: ambiguous information for ref %s\"),\n>>> +\t\t\t\t\t    orig_ref);\n>>\n>> This isn't per-se new, but I wonder if while we're at it we shold just\n>> quote '%s' here, which we'd usually do. I.e. this message isn't new, but\n>> referring again to \"ref %s\" (and not \"ref '%s'\") below is.\n>\n> Good.\n>\n>>> +\t\t\t\t \"To support setting up tracking branches, ensure that\\n\"\n>>> +\t\t\t\t \"different remotes' fetch refspecs map into different\\n\"\n>>> +\t\t\t\t \"tracking namespaces.\"),\n>>> +\t\t\t       orig_ref,\n>>> +\t\t\t       remotes_advice.buf\n>>> +\t\t\t       );\n>>\n>> Nit: The usual style for multi-line arguments is to \"fill\" lines until\n>> you're at 79 characters, so these last three lines (including the \");\")\n>> can all go on the \"tracking namespaces\" line (until they're at 79, then\n>> wrap)>\n>\n> I didn't know about the magic \"79\" number.  It makes the resulting\n> source code extremely hard to read, though, while making it easier\n> to grep for specific messages.\n\nI'm referring to the \"80 characters per line\", but omitted the \\n, but\nyeah, I should have just said 80.\n\n"},{"id":"452711","messageId":"xmqqzgl75c35.fsf@gitster.g","threadId":"57591","inReplyTo":"220330.86o81nta6k.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v5] tracking branches: add advice to ambiguous refspec error","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-30T22:07:10Z","receivedAt":"2022-03-30T22:07:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n>>>> +\t\t\t\t \"To support setting up tracking branches, ensure that\\n\"\n>>>> +\t\t\t\t \"different remotes' fetch refspecs map into different\\n\"\n>>>> +\t\t\t\t \"tracking namespaces.\"),\n>>>> +\t\t\t       orig_ref,\n>>>> +\t\t\t       remotes_advice.buf\n>>>> +\t\t\t       );\n>>>\n>>> Nit: The usual style for multi-line arguments is to \"fill\" lines until\n>>> you're at 79 characters, so these last three lines (including the \");\")\n>>> can all go on the \"tracking namespaces\" line (until they're at 79, then\n>>> wrap)>\n>>\n>> I didn't know about the magic \"79\" number.  It makes the resulting\n>> source code extremely hard to read, though, while making it easier\n>> to grep for specific messages.\n>\n> I'm referring to the \"80 characters per line\", but omitted the \\n, but\n> yeah, I should have just said 80.\n\nNo, what I meant was that you do not want the rule to be to cut *AT*\nexactly the column whatever random rule specifies, which would\nresult in funny wrapping in the middle of the word, e.g.\n\n        \"To support setting up tracking branches, ensure that diff\"\n        \"erent remotes' fetch refspecs map into different tracking\"\n        \" namespaces.\"\n\nand \"at 79, then wrap\" somehow sounded to me like that.  I do not\nthink you meant to imply that (instead, I think you meant to suggest\n\"wrap the line so that the string constant is not longer than 79\ncolumns\"), but it risks to be mistaken by new contributors.\n\nFWIW, I'd actually prefer to see both the sources to be readable by\nwrapping to keep the source code line length under certain limit\n(the current guideline being 70-something), counting the leading\nindentation, and at the same time keep it possible and easy to grep\nmessages in the source.\n\nThat requires us to notice when our code has too deeply nested,\nresulting in overly indented lines, and maintain the readability\n(refatoring the code may be a way to help in such cases).\n"},{"id":"452720","messageId":"220331.86y20rrqzg.gmgdl@evledraar.gmail.com","threadId":"57591","inReplyTo":"xmqqzgl75c35.fsf@gitster.g","subject":"Re: [PATCH v5] tracking branches: add advice to ambiguous refspec error","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-03-30T22:51:50Z","receivedAt":"2022-03-30T22:54:36Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Mar 30 2022, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n>\n>>>>> +\t\t\t\t \"To support setting up tracking branches, ensure that\\n\"\n>>>>> +\t\t\t\t \"different remotes' fetch refspecs map into different\\n\"\n>>>>> +\t\t\t\t \"tracking namespaces.\"),\n>>>>> +\t\t\t       orig_ref,\n>>>>> +\t\t\t       remotes_advice.buf\n>>>>> +\t\t\t       );\n>>>>\n>>>> Nit: The usual style for multi-line arguments is to \"fill\" lines until\n>>>> you're at 79 characters, so these last three lines (including the \");\")\n>>>> can all go on the \"tracking namespaces\" line (until they're at 79, then\n>>>> wrap)>\n>>>\n>>> I didn't know about the magic \"79\" number.  It makes the resulting\n>>> source code extremely hard to read, though, while making it easier\n>>> to grep for specific messages.\n>>\n>> I'm referring to the \"80 characters per line\", but omitted the \\n, but\n>> yeah, I should have just said 80.\n>\n> No, what I meant was that you do not want the rule to be to cut *AT*\n> exactly the column whatever random rule specifies, which would\n> result in funny wrapping in the middle of the word, e.g.\n>\n>         \"To support setting up tracking branches, ensure that diff\"\n>         \"erent remotes' fetch refspecs map into different tracking\"\n>         \" namespaces.\"\n>\n> and \"at 79, then wrap\" somehow sounded to me like that.  I do not\n> think you meant to imply that (instead, I think you meant to suggest\n> \"wrap the line so that the string constant is not longer than 79\n> columns\"), but it risks to be mistaken by new contributors.\n>\n> FWIW, I'd actually prefer to see both the sources to be readable by\n> wrapping to keep the source code line length under certain limit\n> (the current guideline being 70-something), counting the leading\n> indentation, and at the same time keep it possible and easy to grep\n> messages in the source.\n>\n> That requires us to notice when our code has too deeply nested,\n> resulting in overly indented lines, and maintain the readability\n> (refatoring the code may be a way to help in such cases).\n\nYes, I didn't mean to say it was a hard rule. In particular as this code\nhas the strings themselves over 80 characters. It can be good to break\nthat guideline when it doesn't help readability.\n\nI just meant that this made sense as a fix-up, in this case:\n\ndiff --git a/branch.c b/branch.c\nindex 5c28d432103..4ccf5f79e83 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -282,10 +282,8 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,\n \t\t\t\t \"\\n\"\n \t\t\t\t \"To support setting up tracking branches, ensure that\\n\"\n \t\t\t\t \"different remotes' fetch refspecs map into different\\n\"\n-\t\t\t\t \"tracking namespaces.\"),\n-\t\t\t       orig_ref,\n-\t\t\t       ftb_cb.remotes_advice.buf\n-\t\t\t       );\n+\t\t\t\t \"tracking namespaces.\"), orig_ref,\n+\t\t\t       ftb_cb.remotes_advice.buf);\n \t\texit(status);\n \t}\n \n\nWhich I'd also be tempted to do as:\n\ndiff --git a/branch.c b/branch.c\nindex 5c28d432103..b9f6fda980b 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -283,9 +283,7 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,\n \t\t\t\t \"To support setting up tracking branches, ensure that\\n\"\n \t\t\t\t \"different remotes' fetch refspecs map into different\\n\"\n \t\t\t\t \"tracking namespaces.\"),\n-\t\t\t       orig_ref,\n-\t\t\t       ftb_cb.remotes_advice.buf\n-\t\t\t       );\n+\t\t\t       orig_ref, ftb_cb.remotes_advice.buf);\n \t\texit(status);\n \t}\n \nBut I find it generally helpful to do it consistently when possible, as\nwhen running into merge conflicts it makes things easier.\n"},{"id":"452799","messageId":"pull.1183.v6.git.1648742485887.gitgitgadget@gmail.com","threadId":"57591","inReplyTo":"pull.1183.v5.git.1648624810866.gitgitgadget@gmail.com","subject":"[PATCH v6] tracking branches: add advice to ambiguous refspec error","fromName":"Tao Klerks via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-31T16:01:25Z","receivedAt":"2022-03-31T16:01:35Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"From: Tao Klerks <tao@klerks.biz>\n\nThe error \"not tracking: ambiguous information for ref\" is raised\nwhen we are evaluating what tracking information to set on a branch,\nand find that the ref to be added as tracking branch is mapped\nunder multiple remotes' fetch refspecs.\n\nThis can easily happen when a user copy-pastes a remote definition\nin their git config, and forgets to change the tracking path.\n\nAdd advice in this situation, explicitly highlighting which remotes\nare involved and suggesting how to correct the situation.\n\nSigned-off-by: Tao Klerks <tao@klerks.biz>\n---\n    tracking branches: add advice to ambiguous refspec error\n    \n    v6 addresses some formatting and commenting issues Ævar noticed.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1183%2FTaoK%2Fadvise-ambiguous-tracking-refspec-v6\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1183/TaoK/advise-ambiguous-tracking-refspec-v6\nPull-Request: https://github.com/gitgitgadget/git/pull/1183\n\nRange-diff vs v5:\n\n 1:  4478eaed6df ! 1:  2408ab0ccb3 tracking branches: add advice to ambiguous refspec error\n     @@ branch.c: struct tracking {\n      -\t\t} else {\n      +\t\t\tbreak;\n      +\t\tcase 2:\n     -+\t\t\t// there are at least two remotes; backfill the first one\n     ++\t\t\t/* there are at least two remotes; backfill the first one */\n      +\t\t\tstring_list_append(&ftb->ambiguous_remotes, tracking->spec.src);\n      +\t\t\t/* fall through */\n      +\t\tdefault:\n     @@ branch.c: struct tracking {\n       \t\t}\n       \t\ttracking->spec.src = NULL;\n       \t}\n     -@@ branch.c: static int inherit_tracking(struct tracking *tracking, const char *orig_ref)\n     - \treturn 0;\n     - }\n     - \n     -+\n     - /*\n     -  * Used internally to set the branch.<new_ref>.{remote,merge} config\n     -  * settings so that branch 'new_ref' tracks 'orig_ref'. Unlike\n      @@ branch.c: static void setup_tracking(const char *new_ref, const char *orig_ref,\n       \tstruct tracking tracking;\n       \tstruct string_list tracking_srcs = STRING_LIST_INIT_DUP;\n     @@ branch.c: static void setup_tracking(const char *new_ref, const char *orig_ref,\n      -\t\tdie(_(\"not tracking: ambiguous information for ref %s\"),\n      -\t\t    orig_ref);\n      +\tif (tracking.matches > 1) {\n     -+\t\tint status = die_message(_(\"not tracking: ambiguous information for ref %s\"),\n     ++\t\tint status = die_message(_(\"not tracking: ambiguous information for ref '%s'\"),\n      +\t\t\t\t\t    orig_ref);\n      +\t\tif (advice_enabled(ADVICE_AMBIGUOUS_FETCH_REFSPEC)) {\n      +\t\t\tstruct strbuf remotes_advice = STRBUF_INIT;\n      +\t\t\tstruct string_list_item *item;\n      +\n     -+\t\t\tfor_each_string_list_item(item, &ftb_cb.ambiguous_remotes) {\n     ++\t\t\tfor_each_string_list_item(item, &ftb_cb.ambiguous_remotes)\n      +\t\t\t\t/*\n      +\t\t\t\t * TRANSLATORS: This is a line listing a remote with duplicate\n      +\t\t\t\t * refspecs in the advice message below. For RTL languages you'll\n      +\t\t\t\t * probably want to swap the \"%s\" and leading \"  \" space around.\n      +\t\t\t\t */\n      +\t\t\t\tstrbuf_addf(&remotes_advice, _(\"  %s\\n\"), item->string);\n     -+\t\t\t}\n      +\n     ++\t\t\t/*\n     ++\t\t\t * TRANSLATORS: The second argument is a \\n-delimited list of\n     ++\t\t\t * duplicate refspecs, composed above.\n     ++\t\t\t */\n      +\t\t\tadvise(_(\"There are multiple remotes whose fetch refspecs map to the remote\\n\"\n     -+\t\t\t\t \"tracking ref %s:\\n\"\n     ++\t\t\t\t \"tracking ref '%s':\\n\"\n      +\t\t\t\t \"%s\"\n      +\t\t\t\t \"\\n\"\n      +\t\t\t\t \"This is typically a configuration error.\\n\"\n      +\t\t\t\t \"\\n\"\n      +\t\t\t\t \"To support setting up tracking branches, ensure that\\n\"\n      +\t\t\t\t \"different remotes' fetch refspecs map into different\\n\"\n     -+\t\t\t\t \"tracking namespaces.\"),\n     -+\t\t\t       orig_ref,\n     -+\t\t\t       remotes_advice.buf\n     -+\t\t\t       );\n     ++\t\t\t\t \"tracking namespaces.\"), orig_ref,\n     ++\t\t\t       remotes_advice.buf);\n      +\t\t\tstrbuf_release(&remotes_advice);\n      +\t\t}\n      +\t\texit(status);\n\n\n Documentation/config/advice.txt |  4 +++\n advice.c                        |  1 +\n advice.h                        |  1 +\n branch.c                        | 63 +++++++++++++++++++++++++++++----\n 4 files changed, 62 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt\nindex c40eb09cb7e..343d271c707 100644\n--- a/Documentation/config/advice.txt\n+++ b/Documentation/config/advice.txt\n@@ -4,6 +4,10 @@ advice.*::\n \tcan tell Git that you do not need help by setting these to 'false':\n +\n --\n+\tambiguousFetchRefspec::\n+\t\tAdvice shown when fetch refspec for multiple remotes map to\n+\t\tthe same remote-tracking branch namespace and causes branch\n+\t\ttracking set-up to fail.\n \tfetchShowForcedUpdates::\n \t\tAdvice shown when linkgit:git-fetch[1] takes a long time\n \t\tto calculate forced updates after ref updates, or to warn\ndiff --git a/advice.c b/advice.c\nindex 2e1fd483040..18ac8739519 100644\n--- a/advice.c\n+++ b/advice.c\n@@ -39,6 +39,7 @@ static struct {\n \t[ADVICE_ADD_EMPTY_PATHSPEC]\t\t\t= { \"addEmptyPathspec\", 1 },\n \t[ADVICE_ADD_IGNORED_FILE]\t\t\t= { \"addIgnoredFile\", 1 },\n \t[ADVICE_AM_WORK_DIR] \t\t\t\t= { \"amWorkDir\", 1 },\n+\t[ADVICE_AMBIGUOUS_FETCH_REFSPEC]\t\t= { \"ambiguousFetchRefspec\", 1 },\n \t[ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME] \t= { \"checkoutAmbiguousRemoteBranchName\", 1 },\n \t[ADVICE_COMMIT_BEFORE_MERGE]\t\t\t= { \"commitBeforeMerge\", 1 },\n \t[ADVICE_DETACHED_HEAD]\t\t\t\t= { \"detachedHead\", 1 },\ndiff --git a/advice.h b/advice.h\nindex a3957123a16..2d4c94f38eb 100644\n--- a/advice.h\n+++ b/advice.h\n@@ -17,6 +17,7 @@ struct string_list;\n \tADVICE_ADD_EMPTY_PATHSPEC,\n \tADVICE_ADD_IGNORED_FILE,\n \tADVICE_AM_WORK_DIR,\n+\tADVICE_AMBIGUOUS_FETCH_REFSPEC,\n \tADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME,\n \tADVICE_COMMIT_BEFORE_MERGE,\n \tADVICE_DETACHED_HEAD,\ndiff --git a/branch.c b/branch.c\nindex 6b31df539a5..812ceae2d0f 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -18,17 +18,31 @@ struct tracking {\n \tint matches;\n };\n \n+struct find_tracked_branch_cb {\n+\tstruct tracking *tracking;\n+\tstruct string_list ambiguous_remotes;\n+};\n+\n static int find_tracked_branch(struct remote *remote, void *priv)\n {\n-\tstruct tracking *tracking = priv;\n+\tstruct find_tracked_branch_cb *ftb = priv;\n+\tstruct tracking *tracking = ftb->tracking;\n \n \tif (!remote_find_tracking(remote, &tracking->spec)) {\n-\t\tif (++tracking->matches == 1) {\n+\t\tswitch (++tracking->matches) {\n+\t\tcase 1:\n \t\t\tstring_list_append(tracking->srcs, tracking->spec.src);\n \t\t\ttracking->remote = remote->name;\n-\t\t} else {\n+\t\t\tbreak;\n+\t\tcase 2:\n+\t\t\t/* there are at least two remotes; backfill the first one */\n+\t\t\tstring_list_append(&ftb->ambiguous_remotes, tracking->spec.src);\n+\t\t\t/* fall through */\n+\t\tdefault:\n+\t\t\tstring_list_append(&ftb->ambiguous_remotes, remote->name);\n \t\t\tfree(tracking->spec.src);\n \t\t\tstring_list_clear(tracking->srcs, 0);\n+\t\tbreak;\n \t\t}\n \t\ttracking->spec.src = NULL;\n \t}\n@@ -232,12 +246,16 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,\n \tstruct tracking tracking;\n \tstruct string_list tracking_srcs = STRING_LIST_INIT_DUP;\n \tint config_flags = quiet ? 0 : BRANCH_CONFIG_VERBOSE;\n+\tstruct find_tracked_branch_cb ftb_cb = {\n+\t\t.tracking = &tracking,\n+\t\t.ambiguous_remotes = STRING_LIST_INIT_DUP,\n+\t};\n \n \tmemset(&tracking, 0, sizeof(tracking));\n \ttracking.spec.dst = (char *)orig_ref;\n \ttracking.srcs = &tracking_srcs;\n \tif (track != BRANCH_TRACK_INHERIT)\n-\t\tfor_each_remote(find_tracked_branch, &tracking);\n+\t\tfor_each_remote(find_tracked_branch, &ftb_cb);\n \telse if (inherit_tracking(&tracking, orig_ref))\n \t\tgoto cleanup;\n \n@@ -252,9 +270,39 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,\n \t\t\tgoto cleanup;\n \t\t}\n \n-\tif (tracking.matches > 1)\n-\t\tdie(_(\"not tracking: ambiguous information for ref %s\"),\n-\t\t    orig_ref);\n+\tif (tracking.matches > 1) {\n+\t\tint status = die_message(_(\"not tracking: ambiguous information for ref '%s'\"),\n+\t\t\t\t\t    orig_ref);\n+\t\tif (advice_enabled(ADVICE_AMBIGUOUS_FETCH_REFSPEC)) {\n+\t\t\tstruct strbuf remotes_advice = STRBUF_INIT;\n+\t\t\tstruct string_list_item *item;\n+\n+\t\t\tfor_each_string_list_item(item, &ftb_cb.ambiguous_remotes)\n+\t\t\t\t/*\n+\t\t\t\t * TRANSLATORS: This is a line listing a remote with duplicate\n+\t\t\t\t * refspecs in the advice message below. For RTL languages you'll\n+\t\t\t\t * probably want to swap the \"%s\" and leading \"  \" space around.\n+\t\t\t\t */\n+\t\t\t\tstrbuf_addf(&remotes_advice, _(\"  %s\\n\"), item->string);\n+\n+\t\t\t/*\n+\t\t\t * TRANSLATORS: The second argument is a \\n-delimited list of\n+\t\t\t * duplicate refspecs, composed above.\n+\t\t\t */\n+\t\t\tadvise(_(\"There are multiple remotes whose fetch refspecs map to the remote\\n\"\n+\t\t\t\t \"tracking ref '%s':\\n\"\n+\t\t\t\t \"%s\"\n+\t\t\t\t \"\\n\"\n+\t\t\t\t \"This is typically a configuration error.\\n\"\n+\t\t\t\t \"\\n\"\n+\t\t\t\t \"To support setting up tracking branches, ensure that\\n\"\n+\t\t\t\t \"different remotes' fetch refspecs map into different\\n\"\n+\t\t\t\t \"tracking namespaces.\"), orig_ref,\n+\t\t\t       remotes_advice.buf);\n+\t\t\tstrbuf_release(&remotes_advice);\n+\t\t}\n+\t\texit(status);\n+\t}\n \n \tif (tracking.srcs->nr < 1)\n \t\tstring_list_append(tracking.srcs, orig_ref);\n@@ -264,6 +312,7 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,\n \n cleanup:\n \tstring_list_clear(&tracking_srcs, 0);\n+\tstring_list_clear(&ftb_cb.ambiguous_remotes, 0);\n }\n \n int read_branch_desc(struct strbuf *buf, const char *branch_name)\n\nbase-commit: abf474a5dd901f28013c52155411a48fd4c09922\n-- \ngitgitgadget\n"},{"id":"452825","messageId":"xmqqpmm13okb.fsf@gitster.g","threadId":"57591","inReplyTo":"pull.1183.v6.git.1648742485887.gitgitgadget@gmail.com","subject":"Re: [PATCH v6] tracking branches: add advice to ambiguous refspec error","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-31T19:32:52Z","receivedAt":"2022-03-31T19:32:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Tao Klerks via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n>  \tif (!remote_find_tracking(remote, &tracking->spec)) {\n> -\t\tif (++tracking->matches == 1) {\n> +\t\tswitch (++tracking->matches) {\n> +\t\tcase 1:\n>  \t\t\tstring_list_append(tracking->srcs, tracking->spec.src);\n>  \t\t\ttracking->remote = remote->name;\n> -\t\t} else {\n> +\t\t\tbreak;\n> +\t\tcase 2:\n> +\t\t\t/* there are at least two remotes; backfill the first one */\n> +\t\t\tstring_list_append(&ftb->ambiguous_remotes, tracking->spec.src);\n> +\t\t\t/* fall through */\n> +\t\tdefault:\n> +\t\t\tstring_list_append(&ftb->ambiguous_remotes, remote->name);\n>  \t\t\tfree(tracking->spec.src);\n>  \t\t\tstring_list_clear(tracking->srcs, 0);\n> +\t\tbreak;\n\nJust to sanity check this part,\n\n - During the first iteration, we append tracking->spec.src to\n   tracking->srcs, and set tracking->remote to remote->name;\n\n - In later iterations, we do not want to touch tracking->srcs, and\n   want to collect remote->name.\n\nAnd \"backfill\" assumes that tracking->spec.src at that point in the\nsecond iteration is the same as what we got in remote->name in the\nfirst round.  If that were a correct assumption, then it is curious\nthat the first iteration uses tracking->spec.src and remote->name\nseparately for different purposes, which makes me want to double\ncheck if the assumption is indeed correct.\n\nIf it were tracking->remote (which was assigned the value of\nremote->name during the first iteration) that is used to backfill\nbefore we append remote->name in the second iteration, I wouldn't\nfind it \"curious\", but the use of tracking->spec.src there makes me\nfeel confused.\n\nThanks.\n\n\n\n\n\n"},{"id":"452827","messageId":"xmqqlewp3oio.fsf@gitster.g","threadId":"57591","inReplyTo":"pull.1183.v6.git.1648742485887.gitgitgadget@gmail.com","subject":"Re: [PATCH v6] tracking branches: add advice to ambiguous refspec error","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-31T19:33:51Z","receivedAt":"2022-03-31T19:33:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Tao Klerks via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Tao Klerks <tao@klerks.biz>\n>\n> The error \"not tracking: ambiguous information for ref\" is raised\n> when we are evaluating what tracking information to set on a branch,\n> and find that the ref to be added as tracking branch is mapped\n> under multiple remotes' fetch refspecs.\n>\n> This can easily happen when a user copy-pastes a remote definition\n> in their git config, and forgets to change the tracking path.\n>\n> Add advice in this situation, explicitly highlighting which remotes\n> are involved and suggesting how to correct the situation.\n> ...\n>  Documentation/config/advice.txt |  4 +++\n>  advice.c                        |  1 +\n>  advice.h                        |  1 +\n>  branch.c                        | 63 +++++++++++++++++++++++++++++----\n>  4 files changed, 62 insertions(+), 7 deletions(-)\n\nCan we add a test case for the new \"advice\" output?\n\nThanks.\n"},{"id":"452848","messageId":"kl6lsfqxsmj3.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"57591","inReplyTo":"xmqqpmm13okb.fsf@gitster.g","subject":"Re: [PATCH v6] tracking branches: add advice to ambiguous refspec error","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-03-31T23:57:36Z","receivedAt":"2022-03-31T23:57:41Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> \"Tao Klerks via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n>\n>>  \tif (!remote_find_tracking(remote, &tracking->spec)) {\n>> -\t\tif (++tracking->matches == 1) {\n>> +\t\tswitch (++tracking->matches) {\n>> +\t\tcase 1:\n>>  \t\t\tstring_list_append(tracking->srcs, tracking->spec.src);\n>>  \t\t\ttracking->remote = remote->name;\n>> -\t\t} else {\n>> +\t\t\tbreak;\n>> +\t\tcase 2:\n>> +\t\t\t/* there are at least two remotes; backfill the first one */\n>> +\t\t\tstring_list_append(&ftb->ambiguous_remotes, tracking->spec.src);\n>> +\t\t\t/* fall through */\n>> +\t\tdefault:\n>> +\t\t\tstring_list_append(&ftb->ambiguous_remotes, remote->name);\n>>  \t\t\tfree(tracking->spec.src);\n>>  \t\t\tstring_list_clear(tracking->srcs, 0);\n>> +\t\tbreak;\n>\n> Just to sanity check this part,\n>\n>  - During the first iteration, we append tracking->spec.src to\n>    tracking->srcs, and set tracking->remote to remote->name;\n>\n>  - In later iterations, we do not want to touch tracking->srcs, and\n>    want to collect remote->name.\n>\n> And \"backfill\" assumes that tracking->spec.src at that point in the\n> second iteration is the same as what we got in remote->name in the\n> first round.  If that were a correct assumption, then it is curious\n> that the first iteration uses tracking->spec.src and remote->name\n> separately for different purposes, which makes me want to double\n> check if the assumption is indeed correct.\n>\n> If it were tracking->remote (which was assigned the value of\n> remote->name during the first iteration) that is used to backfill\n> before we append remote->name in the second iteration, I wouldn't\n> find it \"curious\", but the use of tracking->spec.src there makes me\n> feel confused.\n>\n> Thanks.\n\nThanks for bringing this up, I also found this unusual when I was\nreading v5.\n"},{"id":"452851","messageId":"CAPMMpojb7-tD2kFNZPiaC0MpB8GNXtYqpJs796pRcM7pE=ksmg@mail.gmail.com","threadId":"57591","inReplyTo":"kl6lsfqxsmj3.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: [PATCH v6] tracking branches: add advice to ambiguous refspec error","fromName":"Tao Klerks","fromEmail":"tao@klerks.biz","sentAt":"2022-04-01T04:30:34Z","receivedAt":"2022-04-01T04:31:00Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"On Fri, Apr 1, 2022 at 1:57 AM Glen Choo <chooglen@google.com> wrote:\n>\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n> > \"Tao Klerks via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n> >\n> >>      if (!remote_find_tracking(remote, &tracking->spec)) {\n> >> -            if (++tracking->matches == 1) {\n> >> +            switch (++tracking->matches) {\n> >> +            case 1:\n> >>                      string_list_append(tracking->srcs, tracking->spec.src);\n> >>                      tracking->remote = remote->name;\n> >> -            } else {\n> >> +                    break;\n> >> +            case 2:\n> >> +                    /* there are at least two remotes; backfill the first one */\n> >> +                    string_list_append(&ftb->ambiguous_remotes, tracking->spec.src);\n> >> +                    /* fall through */\n> >> +            default:\n> >> +                    string_list_append(&ftb->ambiguous_remotes, remote->name);\n> >>                      free(tracking->spec.src);\n> >>                      string_list_clear(tracking->srcs, 0);\n> >> +            break;\n> >\n> > Just to sanity check this part,\n> >\n> >  - During the first iteration, we append tracking->spec.src to\n> >    tracking->srcs, and set tracking->remote to remote->name;\n> >\n> >  - In later iterations, we do not want to touch tracking->srcs, and\n> >    want to collect remote->name.\n> >\n> > And \"backfill\" assumes that tracking->spec.src at that point in the\n> > second iteration is the same as what we got in remote->name in the\n> > first round.  If that were a correct assumption, then it is curious\n> > that the first iteration uses tracking->spec.src and remote->name\n> > separately for different purposes, which makes me want to double\n> > check if the assumption is indeed correct.\n> >\n> > If it were tracking->remote (which was assigned the value of\n> > remote->name during the first iteration) that is used to backfill\n> > before we append remote->name in the second iteration, I wouldn't\n> > find it \"curious\", but the use of tracking->spec.src there makes me\n> > feel confused.\n> >\n> > Thanks.\n>\n> Thanks for bringing this up, I also found this unusual when I was\n> reading v5.\n\nIf you never hear from me again, please know it's because I crawled\nback under the rock I had crawled out from. This is clearly a bug from\na silly typo, and I've managed to look at the resulting output twice\nwithout noticing the wrong thing was printed. I'm guessing the use of\nthe word \"unusual\" here is a polite euphemism for \"you numskull, what\nyou wrote makes no sense!\" :)\n\nI did not think adding an automated test for advise() output made\nsense, but I guess I have proved myself wrong.\n"},{"id":"452852","messageId":"pull.1183.v7.git.1648793113943.gitgitgadget@gmail.com","threadId":"57591","inReplyTo":"pull.1183.v6.git.1648742485887.gitgitgadget@gmail.com","subject":"[PATCH v7] tracking branches: add advice to ambiguous refspec error","fromName":"Tao Klerks via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-04-01T06:05:13Z","receivedAt":"2022-04-01T06:05:22Z","isPatch":true,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"From: Tao Klerks <tao@klerks.biz>\n\nThe error \"not tracking: ambiguous information for ref\" is raised\nwhen we are evaluating what tracking information to set on a branch,\nand find that the ref to be added as tracking branch is mapped\nunder multiple remotes' fetch refspecs.\n\nThis can easily happen when a user copy-pastes a remote definition\nin their git config, and forgets to change the tracking path.\n\nAdd advice in this situation, explicitly highlighting which remotes\nare involved and suggesting how to correct the situation. Also\nupdate a test to explicitly expect that advice.\n\nSigned-off-by: Tao Klerks <tao@klerks.biz>\n---\n    tracking branches: add advice to ambiguous refspec error\n    \n    In v7 address a wrong-value-printed bug that Junio and Glen noticed, and\n    add testing for the emitted advice.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1183%2FTaoK%2Fadvise-ambiguous-tracking-refspec-v7\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1183/TaoK/advise-ambiguous-tracking-refspec-v7\nPull-Request: https://github.com/gitgitgadget/git/pull/1183\n\nRange-diff vs v6:\n\n 1:  2408ab0ccb3 ! 1:  4fc5d2b6d13 tracking branches: add advice to ambiguous refspec error\n     @@ Commit message\n          in their git config, and forgets to change the tracking path.\n      \n          Add advice in this situation, explicitly highlighting which remotes\n     -    are involved and suggesting how to correct the situation.\n     +    are involved and suggesting how to correct the situation. Also\n     +    update a test to explicitly expect that advice.\n      \n          Signed-off-by: Tao Klerks <tao@klerks.biz>\n      \n     @@ branch.c: struct tracking {\n      +\t\t\tbreak;\n      +\t\tcase 2:\n      +\t\t\t/* there are at least two remotes; backfill the first one */\n     -+\t\t\tstring_list_append(&ftb->ambiguous_remotes, tracking->spec.src);\n     ++\t\t\tstring_list_append(&ftb->ambiguous_remotes, tracking->remote);\n      +\t\t\t/* fall through */\n      +\t\tdefault:\n      +\t\t\tstring_list_append(&ftb->ambiguous_remotes, remote->name);\n     @@ branch.c: static void setup_tracking(const char *new_ref, const char *orig_ref,\n       }\n       \n       int read_branch_desc(struct strbuf *buf, const char *branch_name)\n     +\n     + ## t/t3200-branch.sh ##\n     +@@ t/t3200-branch.sh: test_expect_success 'checkout -b with -l makes reflog when core.logAllRefUpdates\n     + \tgit rev-parse --verify gamma@{0}\n     + '\n     + \n     +-test_expect_success 'avoid ambiguous track' '\n     ++test_expect_success 'avoid ambiguous track and advise' '\n     + \tgit config branch.autosetupmerge true &&\n     + \tgit config remote.ambi1.url lalala &&\n     + \tgit config remote.ambi1.fetch refs/heads/lalala:refs/heads/main &&\n     + \tgit config remote.ambi2.url lilili &&\n     + \tgit config remote.ambi2.fetch refs/heads/lilili:refs/heads/main &&\n     +-\ttest_must_fail git branch all1 main &&\n     ++\tcat <<-EOF >expected &&\n     ++\tfatal: not tracking: ambiguous information for ref '\\''refs/heads/main'\\''\n     ++\thint: There are multiple remotes whose fetch refspecs map to the remote\n     ++\thint: tracking ref '\\''refs/heads/main'\\'':\n     ++\thint:   ambi1\n     ++\thint:   ambi2\n     ++\thint: ''\n     ++\thint: This is typically a configuration error.\n     ++\thint: ''\n     ++\thint: To support setting up tracking branches, ensure that\n     ++\thint: different remotes'\\'' fetch refspecs map into different\n     ++\thint: tracking namespaces.\n     ++\tEOF\n     ++\ttest_must_fail git branch all1 main 2>actual &&\n     ++\ttest_cmp expected actual &&\n     + \ttest -z \"$(git config branch.all1.merge)\"\n     + '\n     + \n\n\n Documentation/config/advice.txt |  4 +++\n advice.c                        |  1 +\n advice.h                        |  1 +\n branch.c                        | 63 +++++++++++++++++++++++++++++----\n t/t3200-branch.sh               | 18 ++++++++--\n 5 files changed, 78 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt\nindex c40eb09cb7e..343d271c707 100644\n--- a/Documentation/config/advice.txt\n+++ b/Documentation/config/advice.txt\n@@ -4,6 +4,10 @@ advice.*::\n \tcan tell Git that you do not need help by setting these to 'false':\n +\n --\n+\tambiguousFetchRefspec::\n+\t\tAdvice shown when fetch refspec for multiple remotes map to\n+\t\tthe same remote-tracking branch namespace and causes branch\n+\t\ttracking set-up to fail.\n \tfetchShowForcedUpdates::\n \t\tAdvice shown when linkgit:git-fetch[1] takes a long time\n \t\tto calculate forced updates after ref updates, or to warn\ndiff --git a/advice.c b/advice.c\nindex 2e1fd483040..18ac8739519 100644\n--- a/advice.c\n+++ b/advice.c\n@@ -39,6 +39,7 @@ static struct {\n \t[ADVICE_ADD_EMPTY_PATHSPEC]\t\t\t= { \"addEmptyPathspec\", 1 },\n \t[ADVICE_ADD_IGNORED_FILE]\t\t\t= { \"addIgnoredFile\", 1 },\n \t[ADVICE_AM_WORK_DIR] \t\t\t\t= { \"amWorkDir\", 1 },\n+\t[ADVICE_AMBIGUOUS_FETCH_REFSPEC]\t\t= { \"ambiguousFetchRefspec\", 1 },\n \t[ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME] \t= { \"checkoutAmbiguousRemoteBranchName\", 1 },\n \t[ADVICE_COMMIT_BEFORE_MERGE]\t\t\t= { \"commitBeforeMerge\", 1 },\n \t[ADVICE_DETACHED_HEAD]\t\t\t\t= { \"detachedHead\", 1 },\ndiff --git a/advice.h b/advice.h\nindex a3957123a16..2d4c94f38eb 100644\n--- a/advice.h\n+++ b/advice.h\n@@ -17,6 +17,7 @@ struct string_list;\n \tADVICE_ADD_EMPTY_PATHSPEC,\n \tADVICE_ADD_IGNORED_FILE,\n \tADVICE_AM_WORK_DIR,\n+\tADVICE_AMBIGUOUS_FETCH_REFSPEC,\n \tADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME,\n \tADVICE_COMMIT_BEFORE_MERGE,\n \tADVICE_DETACHED_HEAD,\ndiff --git a/branch.c b/branch.c\nindex 6b31df539a5..182f1c5a556 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -18,17 +18,31 @@ struct tracking {\n \tint matches;\n };\n \n+struct find_tracked_branch_cb {\n+\tstruct tracking *tracking;\n+\tstruct string_list ambiguous_remotes;\n+};\n+\n static int find_tracked_branch(struct remote *remote, void *priv)\n {\n-\tstruct tracking *tracking = priv;\n+\tstruct find_tracked_branch_cb *ftb = priv;\n+\tstruct tracking *tracking = ftb->tracking;\n \n \tif (!remote_find_tracking(remote, &tracking->spec)) {\n-\t\tif (++tracking->matches == 1) {\n+\t\tswitch (++tracking->matches) {\n+\t\tcase 1:\n \t\t\tstring_list_append(tracking->srcs, tracking->spec.src);\n \t\t\ttracking->remote = remote->name;\n-\t\t} else {\n+\t\t\tbreak;\n+\t\tcase 2:\n+\t\t\t/* there are at least two remotes; backfill the first one */\n+\t\t\tstring_list_append(&ftb->ambiguous_remotes, tracking->remote);\n+\t\t\t/* fall through */\n+\t\tdefault:\n+\t\t\tstring_list_append(&ftb->ambiguous_remotes, remote->name);\n \t\t\tfree(tracking->spec.src);\n \t\t\tstring_list_clear(tracking->srcs, 0);\n+\t\tbreak;\n \t\t}\n \t\ttracking->spec.src = NULL;\n \t}\n@@ -232,12 +246,16 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,\n \tstruct tracking tracking;\n \tstruct string_list tracking_srcs = STRING_LIST_INIT_DUP;\n \tint config_flags = quiet ? 0 : BRANCH_CONFIG_VERBOSE;\n+\tstruct find_tracked_branch_cb ftb_cb = {\n+\t\t.tracking = &tracking,\n+\t\t.ambiguous_remotes = STRING_LIST_INIT_DUP,\n+\t};\n \n \tmemset(&tracking, 0, sizeof(tracking));\n \ttracking.spec.dst = (char *)orig_ref;\n \ttracking.srcs = &tracking_srcs;\n \tif (track != BRANCH_TRACK_INHERIT)\n-\t\tfor_each_remote(find_tracked_branch, &tracking);\n+\t\tfor_each_remote(find_tracked_branch, &ftb_cb);\n \telse if (inherit_tracking(&tracking, orig_ref))\n \t\tgoto cleanup;\n \n@@ -252,9 +270,39 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,\n \t\t\tgoto cleanup;\n \t\t}\n \n-\tif (tracking.matches > 1)\n-\t\tdie(_(\"not tracking: ambiguous information for ref %s\"),\n-\t\t    orig_ref);\n+\tif (tracking.matches > 1) {\n+\t\tint status = die_message(_(\"not tracking: ambiguous information for ref '%s'\"),\n+\t\t\t\t\t    orig_ref);\n+\t\tif (advice_enabled(ADVICE_AMBIGUOUS_FETCH_REFSPEC)) {\n+\t\t\tstruct strbuf remotes_advice = STRBUF_INIT;\n+\t\t\tstruct string_list_item *item;\n+\n+\t\t\tfor_each_string_list_item(item, &ftb_cb.ambiguous_remotes)\n+\t\t\t\t/*\n+\t\t\t\t * TRANSLATORS: This is a line listing a remote with duplicate\n+\t\t\t\t * refspecs in the advice message below. For RTL languages you'll\n+\t\t\t\t * probably want to swap the \"%s\" and leading \"  \" space around.\n+\t\t\t\t */\n+\t\t\t\tstrbuf_addf(&remotes_advice, _(\"  %s\\n\"), item->string);\n+\n+\t\t\t/*\n+\t\t\t * TRANSLATORS: The second argument is a \\n-delimited list of\n+\t\t\t * duplicate refspecs, composed above.\n+\t\t\t */\n+\t\t\tadvise(_(\"There are multiple remotes whose fetch refspecs map to the remote\\n\"\n+\t\t\t\t \"tracking ref '%s':\\n\"\n+\t\t\t\t \"%s\"\n+\t\t\t\t \"\\n\"\n+\t\t\t\t \"This is typically a configuration error.\\n\"\n+\t\t\t\t \"\\n\"\n+\t\t\t\t \"To support setting up tracking branches, ensure that\\n\"\n+\t\t\t\t \"different remotes' fetch refspecs map into different\\n\"\n+\t\t\t\t \"tracking namespaces.\"), orig_ref,\n+\t\t\t       remotes_advice.buf);\n+\t\t\tstrbuf_release(&remotes_advice);\n+\t\t}\n+\t\texit(status);\n+\t}\n \n \tif (tracking.srcs->nr < 1)\n \t\tstring_list_append(tracking.srcs, orig_ref);\n@@ -264,6 +312,7 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,\n \n cleanup:\n \tstring_list_clear(&tracking_srcs, 0);\n+\tstring_list_clear(&ftb_cb.ambiguous_remotes, 0);\n }\n \n int read_branch_desc(struct strbuf *buf, const char *branch_name)\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex 7a0ff75ba86..e12db593615 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -1039,13 +1039,27 @@ test_expect_success 'checkout -b with -l makes reflog when core.logAllRefUpdates\n \tgit rev-parse --verify gamma@{0}\n '\n \n-test_expect_success 'avoid ambiguous track' '\n+test_expect_success 'avoid ambiguous track and advise' '\n \tgit config branch.autosetupmerge true &&\n \tgit config remote.ambi1.url lalala &&\n \tgit config remote.ambi1.fetch refs/heads/lalala:refs/heads/main &&\n \tgit config remote.ambi2.url lilili &&\n \tgit config remote.ambi2.fetch refs/heads/lilili:refs/heads/main &&\n-\ttest_must_fail git branch all1 main &&\n+\tcat <<-EOF >expected &&\n+\tfatal: not tracking: ambiguous information for ref '\\''refs/heads/main'\\''\n+\thint: There are multiple remotes whose fetch refspecs map to the remote\n+\thint: tracking ref '\\''refs/heads/main'\\'':\n+\thint:   ambi1\n+\thint:   ambi2\n+\thint: ''\n+\thint: This is typically a configuration error.\n+\thint: ''\n+\thint: To support setting up tracking branches, ensure that\n+\thint: different remotes'\\'' fetch refspecs map into different\n+\thint: tracking namespaces.\n+\tEOF\n+\ttest_must_fail git branch all1 main 2>actual &&\n+\ttest_cmp expected actual &&\n \ttest -z \"$(git config branch.all1.merge)\"\n '\n \n\nbase-commit: 805e0a68082a217f0112db9ee86a022227a9c81b\n-- \ngitgitgadget\n"},{"id":"452906","messageId":"kl6lpmm0sqlw.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"57591","inReplyTo":"CAPMMpojb7-tD2kFNZPiaC0MpB8GNXtYqpJs796pRcM7pE=ksmg@mail.gmail.com","subject":"Re: [PATCH v6] tracking branches: add advice to ambiguous refspec error","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-04-01T16:41:47Z","receivedAt":"2022-04-01T16:50:55Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Tao Klerks <tao@klerks.biz> writes:\n\n> On Fri, Apr 1, 2022 at 1:57 AM Glen Choo <chooglen@google.com> wrote:\n>>\n>> Junio C Hamano <gitster@pobox.com> writes:\n>>\n>> > \"Tao Klerks via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n>> >\n>> >>      if (!remote_find_tracking(remote, &tracking->spec)) {\n>> >> -            if (++tracking->matches == 1) {\n>> >> +            switch (++tracking->matches) {\n>> >> +            case 1:\n>> >>                      string_list_append(tracking->srcs, tracking->spec.src);\n>> >>                      tracking->remote = remote->name;\n>> >> -            } else {\n>> >> +                    break;\n>> >> +            case 2:\n>> >> +                    /* there are at least two remotes; backfill the first one */\n>> >> +                    string_list_append(&ftb->ambiguous_remotes, tracking->spec.src);\n>> >> +                    /* fall through */\n>> >> +            default:\n>> >> +                    string_list_append(&ftb->ambiguous_remotes, remote->name);\n>> >>                      free(tracking->spec.src);\n>> >>                      string_list_clear(tracking->srcs, 0);\n>> >> +            break;\n>> >\n>> > Just to sanity check this part,\n>> >\n>> >  - During the first iteration, we append tracking->spec.src to\n>> >    tracking->srcs, and set tracking->remote to remote->name;\n>> >\n>> >  - In later iterations, we do not want to touch tracking->srcs, and\n>> >    want to collect remote->name.\n>> >\n>> > And \"backfill\" assumes that tracking->spec.src at that point in the\n>> > second iteration is the same as what we got in remote->name in the\n>> > first round.  If that were a correct assumption, then it is curious\n>> > that the first iteration uses tracking->spec.src and remote->name\n>> > separately for different purposes, which makes me want to double\n>> > check if the assumption is indeed correct.\n>> >\n>> > If it were tracking->remote (which was assigned the value of\n>> > remote->name during the first iteration) that is used to backfill\n>> > before we append remote->name in the second iteration, I wouldn't\n>> > find it \"curious\", but the use of tracking->spec.src there makes me\n>> > feel confused.\n>> >\n>> > Thanks.\n>>\n>> Thanks for bringing this up, I also found this unusual when I was\n>> reading v5.\n>\n> If you never hear from me again, please know it's because I crawled\n> back under the rock I had crawled out from. This is clearly a bug from\n> a silly typo, and I've managed to look at the resulting output twice\n> without noticing the wrong thing was printed. I'm guessing the use of\n> the word \"unusual\" here is a polite euphemism for \"you numskull, what\n> you wrote makes no sense!\" :)\n\nPlease don't take it that way! I use it the way I think most others use\nit, which is a more charitable \"Hm, I trust the author, but this looks\nconfusing to me and let me ask just to be sure that all our bases are\ncovered.\"\n\nAfter all, even the best of us make mistakes, so I don't think it's a\nbig deal. Plus, if the mistake managed to sneak past review, the fault\nis on all of us :)\n\n> I did not think adding an automated test for advise() output made\n> sense, but I guess I have proved myself wrong.\n\nHeh, nearly every time I think that a test isn't necessary, I find a way\nto prove myself wrong too.\n"},{"id":"452907","messageId":"kl6lmth4sq1m.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"57591","inReplyTo":"pull.1183.v7.git.1648793113943.gitgitgadget@gmail.com","subject":"Re: [PATCH v7] tracking branches: add advice to ambiguous refspec error","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-04-01T16:53:57Z","receivedAt":"2022-04-01T16:54:17Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"\"Tao Klerks via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> diff --git a/branch.c b/branch.c\n> index 6b31df539a5..182f1c5a556 100644\n> --- a/branch.c\n> +++ b/branch.c\n> @@ -18,17 +18,31 @@ struct tracking {\n>  \tint matches;\n>  };\n>  \n> +struct find_tracked_branch_cb {\n> +\tstruct tracking *tracking;\n> +\tstruct string_list ambiguous_remotes;\n> +};\n> +\n>  static int find_tracked_branch(struct remote *remote, void *priv)\n>  {\n> -\tstruct tracking *tracking = priv;\n> +\tstruct find_tracked_branch_cb *ftb = priv;\n> +\tstruct tracking *tracking = ftb->tracking;\n>  \n>  \tif (!remote_find_tracking(remote, &tracking->spec)) {\n> -\t\tif (++tracking->matches == 1) {\n> +\t\tswitch (++tracking->matches) {\n> +\t\tcase 1:\n>  \t\t\tstring_list_append(tracking->srcs, tracking->spec.src);\n>  \t\t\ttracking->remote = remote->name;\n> -\t\t} else {\n> +\t\t\tbreak;\n> +\t\tcase 2:\n> +\t\t\t/* there are at least two remotes; backfill the first one */\n> +\t\t\tstring_list_append(&ftb->ambiguous_remotes, tracking->remote);\n> +\t\t\t/* fall through */\n> +\t\tdefault:\n> +\t\t\tstring_list_append(&ftb->ambiguous_remotes, remote->name);\n>  \t\t\tfree(tracking->spec.src);\n>  \t\t\tstring_list_clear(tracking->srcs, 0);\n> +\t\tbreak;\n>  \t\t}\n>  \t\ttracking->spec.src = NULL;\n>  \t}\n\nAh I see, on the first iteration, we set the first remote's name to\ntracking->remote, and on the second iteration, we copy that value to the\nlist before copying the _current_ remote's name to the list.\n\n> -test_expect_success 'avoid ambiguous track' '\n> +test_expect_success 'avoid ambiguous track and advise' '\n>  \tgit config branch.autosetupmerge true &&\n>  \tgit config remote.ambi1.url lalala &&\n>  \tgit config remote.ambi1.fetch refs/heads/lalala:refs/heads/main &&\n>  \tgit config remote.ambi2.url lilili &&\n>  \tgit config remote.ambi2.fetch refs/heads/lilili:refs/heads/main &&\n> -\ttest_must_fail git branch all1 main &&\n> +\tcat <<-EOF >expected &&\n> +\tfatal: not tracking: ambiguous information for ref '\\''refs/heads/main'\\''\n> +\thint: There are multiple remotes whose fetch refspecs map to the remote\n> +\thint: tracking ref '\\''refs/heads/main'\\'':\n> +\thint:   ambi1\n> +\thint:   ambi2\n> +\thint: ''\n> +\thint: This is typically a configuration error.\n> +\thint: ''\n> +\thint: To support setting up tracking branches, ensure that\n> +\thint: different remotes'\\'' fetch refspecs map into different\n> +\thint: tracking namespaces.\n> +\tEOF\n> +\ttest_must_fail git branch all1 main 2>actual &&\n> +\ttest_cmp expected actual &&\n>  \ttest -z \"$(git config branch.all1.merge)\"\n>  '\n\nAnd this test shows that this indeed does what we think it does. Nicely\ndone.\n\nI notice that we there is an instance of test -z \"$(some git command)\",\nwhich IIRC is discouraged because it would mask a failure in the git\ncommand, but that's not new and I don't think it needs to be fixed in\nthis series anyway.\n\nReviewed-by: Glen Choo <chooglen@google.com>\n"},{"id":"452917","messageId":"xmqqy20ovaoe.fsf@gitster.g","threadId":"57591","inReplyTo":"kl6lmth4sq1m.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: [PATCH v7] tracking branches: add advice to ambiguous refspec error","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-01T19:57:37Z","receivedAt":"2022-04-01T19:57:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Glen Choo <chooglen@google.com> writes:\n\n> \"Tao Klerks via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n>\n>> diff --git a/branch.c b/branch.c\n>> index 6b31df539a5..182f1c5a556 100644\n>> --- a/branch.c\n>> +++ b/branch.c\n>> @@ -18,17 +18,31 @@ struct tracking {\n>>  \tint matches;\n>>  };\n>>  \n>> +struct find_tracked_branch_cb {\n>> +\tstruct tracking *tracking;\n>> +\tstruct string_list ambiguous_remotes;\n>> +};\n>> +\n>>  static int find_tracked_branch(struct remote *remote, void *priv)\n>>  {\n>> -\tstruct tracking *tracking = priv;\n>> +\tstruct find_tracked_branch_cb *ftb = priv;\n>> +\tstruct tracking *tracking = ftb->tracking;\n>>  \n>>  \tif (!remote_find_tracking(remote, &tracking->spec)) {\n>> -\t\tif (++tracking->matches == 1) {\n>> +\t\tswitch (++tracking->matches) {\n>> +\t\tcase 1:\n>>  \t\t\tstring_list_append(tracking->srcs, tracking->spec.src);\n>>  \t\t\ttracking->remote = remote->name;\n>> -\t\t} else {\n>> +\t\t\tbreak;\n>> +\t\tcase 2:\n>> +\t\t\t/* there are at least two remotes; backfill the first one */\n>> +\t\t\tstring_list_append(&ftb->ambiguous_remotes, tracking->remote);\n>> +\t\t\t/* fall through */\n>> +\t\tdefault:\n>> +\t\t\tstring_list_append(&ftb->ambiguous_remotes, remote->name);\n>>  \t\t\tfree(tracking->spec.src);\n>>  \t\t\tstring_list_clear(tracking->srcs, 0);\n>> +\t\tbreak;\n>>  \t\t}\n>>  \t\ttracking->spec.src = NULL;\n>>  \t}\n>\n> Ah I see, on the first iteration, we set the first remote's name to\n> tracking->remote, and on the second iteration, we copy that value to the\n> list before copying the _current_ remote's name to the list.\n>\n>> -test_expect_success 'avoid ambiguous track' '\n>> +test_expect_success 'avoid ambiguous track and advise' '\n>>  \tgit config branch.autosetupmerge true &&\n>>  \tgit config remote.ambi1.url lalala &&\n>>  \tgit config remote.ambi1.fetch refs/heads/lalala:refs/heads/main &&\n>>  \tgit config remote.ambi2.url lilili &&\n>>  \tgit config remote.ambi2.fetch refs/heads/lilili:refs/heads/main &&\n>> -\ttest_must_fail git branch all1 main &&\n>> +\tcat <<-EOF >expected &&\n>> +\tfatal: not tracking: ambiguous information for ref '\\''refs/heads/main'\\''\n>> +\thint: There are multiple remotes whose fetch refspecs map to the remote\n>> +\thint: tracking ref '\\''refs/heads/main'\\'':\n>> +\thint:   ambi1\n>> +\thint:   ambi2\n>> +\thint: ''\n>> +\thint: This is typically a configuration error.\n>> +\thint: ''\n>> +\thint: To support setting up tracking branches, ensure that\n>> +\thint: different remotes'\\'' fetch refspecs map into different\n>> +\thint: tracking namespaces.\n>> +\tEOF\n>> +\ttest_must_fail git branch all1 main 2>actual &&\n>> +\ttest_cmp expected actual &&\n>>  \ttest -z \"$(git config branch.all1.merge)\"\n>>  '\n>\n> And this test shows that this indeed does what we think it does. Nicely\n> done.\n>\n> I notice that we there is an instance of test -z \"$(some git command)\",\n> which IIRC is discouraged because it would mask a failure in the git\n> command, but that's not new and I don't think it needs to be fixed in\n> this series anyway.\n>\n> Reviewed-by: Glen Choo <chooglen@google.com>\n\nThanks for giving the patch a careful read.\n\nWill queue.\n\nThanks.\n"}]}