{"thread":{"id":"51092","subject":"some git confusion (where git's advice didn't help)","startedAt":"2019-05-14T09:54:49Z","lastAt":"2019-05-14T12:16:55Z","messageCount":7,"participants":["Ulrich Windl","Jeff King","Duy Nguyen"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"375493","messageId":"5CDA9063020000A10003131D@gwsmtp.uni-regensburg.de","threadId":"51092","inReplyTo":null,"subject":"some git confusion (where git's advice didn't help)","fromName":"Ulrich Windl","fromEmail":"ulrich.windl@rz.uni-regensburg.de","sentAt":"2019-05-14T09:54:43Z","receivedAt":"2019-05-14T09:54:49Z","isPatch":false,"sender":{"key":"ulrich.windl@rz.uni-regensburg.de","avatar":null},"body":"Hi!\n\nI was fighting with a remote branch being missing:\n> git branch\n  f-systemd\n  integration\n  master\n* next\n> git pull\nAlready up-to-date.\n> git pull --all\nFetching origin\nAlready up-to-date.\n> git fetch f-gcc-4.8\nfatal: 'f-gcc-4.8' does not appear to be a git repository\nfatal: Could not read from remote repository.\n\nPlease make sure you have the correct access rights\nand the repository exists.\n> git fetch origin\n> git branch\n  f-systemd\n  integration\n  master\n* next\n> git branch -r\n  origin/HEAD -> origin/f-systemd\n  origin/backport-0.0\n  origin/backport-0.1\n  origin/f-gcc-4.8\n  origin/f-manual-peak-reset\n  origin/f-read-failure\n  origin/f-spec-RA\n  origin/f-start-notice\n  origin/f-status-dir\n  origin/f-systemd\n  origin/f-systemd-generator\n  origin/f-usage\n  origin/master\n  origin/next\n> git branch --track origin/f-gcc-4.8\nBranch origin/f-gcc-4.8 set up to track local branch next.\n> git fetch\n> git branch\n  f-systemd\n  integration\n  master\n* next\n  origin/f-gcc-4.8\n> git merge f-gcc-4.8\nmerge: f-gcc-4.8 - not something we can merge\n\nDid you mean this?\n        origin/f-gcc-4.8\n> git merge origin/f-gcc-4.8\nwarning: refname 'origin/f-gcc-4.8' is ambiguous.\nAlready up-to-date.\n\n### So actually this advice wasn't helpful at all. Cause of the problem most likely was \"git branch --track origin/f-gcc-4.8\" that \"imported\" the branch under the same name as the remote branch is referenced.\n\nActually this was just an addition to my previous message about missing remote branches after clone...\n(Seen with git 2.12.3)\n\nMy local \"solution\" was: git branch -d origin/f-gcc-4.8\n\nRegards,\nUlrich\n\n\n\n"},{"id":"375505","messageId":"20190514112115.GA26957@sigill.intra.peff.net","threadId":"51092","inReplyTo":"5CDA9063020000A10003131D@gwsmtp.uni-regensburg.de","subject":"Re: some git confusion (where git's advice didn't help)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-14T11:21:15Z","receivedAt":"2019-05-14T11:21:18Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 14, 2019 at 11:54:43AM +0200, Ulrich Windl wrote:\n\n> > git branch --track origin/f-gcc-4.8\n> Branch origin/f-gcc-4.8 set up to track local branch next.\n> > git fetch\n> > git branch\n>   f-systemd\n>   integration\n>   master\n> * next\n>   origin/f-gcc-4.8\n> > git merge f-gcc-4.8\n> merge: f-gcc-4.8 - not something we can merge\n> \n> Did you mean this?\n>         origin/f-gcc-4.8\n> > git merge origin/f-gcc-4.8\n> warning: refname 'origin/f-gcc-4.8' is ambiguous.\n> Already up-to-date.\n> \n> ### So actually this advice wasn't helpful at all. Cause of the\n> problem most likely was \"git branch --track origin/f-gcc-4.8\" that\n> \"imported\" the branch under the same name as the remote branch is\n> referenced.\n\nRight, that was the source of the problem. Having both\n\"refs/heads/origin/f-gcc-4.8\" and \"refs/remotes/origin/f-gcc-4.8\" is\ngoing to lead to confusion, and you're best off deleting the mistaken\nbranch as soon as possible.\n\nBut I agree we could be more helpful in the messages.\n\nThe \"did you mean?\" advice just blindly says \"oh, you asked for X and\nrefs/remotes/ABC/X exists, so let's suggest ABC/X\", without checking for\nambiguities. It should probably do this:\n\ndiff --git a/help.c b/help.c\nindex a9e451f2ee..108ca54af3 100644\n--- a/help.c\n+++ b/help.c\n@@ -759,7 +759,8 @@ static int append_similar_ref(const char *refname, const struct object_id *oid,\n \t/* A remote branch of the same name is deemed similar */\n \tif (skip_prefix(refname, \"refs/remotes/\", &remote) &&\n \t    !strcmp(branch, cb->base_ref))\n-\t\tstring_list_append(cb->similar_refs, remote);\n+\t\tstring_list_append(cb->similar_refs,\n+\t\t\t\t   shorten_unambiguous_ref(refname, 1));\n \treturn 0;\n }\n \n\nwhich would print \"ABC/X\" in most cases, but \"remotes/ABC/X\" for your\nambiguous case. Incidentally, the existing code also has a memory\nproblem! It blindly skips past \"refs/remotes/\" in the refname and saves\nthe pointer away in a NODUP string-list. But that refname pointer isn't\nours, and isn't guaranteed to last past our for_each_ref() callback. The\nhunk above fixes it because shorten_unambiguous_ref() always returns a\nnewly allocated string. :)\n\nI also think the \"warning: refname ... is ambiguous\" message would\nprobably be a bit more helpful if it showed _which_ candidates it found\n(and which one it chose!).\n\n-Peff\n"},{"id":"375513","messageId":"20190514112941.GA23603@ash","threadId":"51092","inReplyTo":"20190514112115.GA26957@sigill.intra.peff.net","subject":"Re: some git confusion (where git's advice didn't help)","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-05-14T11:29:41Z","receivedAt":"2019-05-14T11:29:50Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, May 14, 2019 at 07:21:15AM -0400, Jeff King wrote:\n> I also think the \"warning: refname ... is ambiguous\" message would\n> probably be a bit more helpful if it showed _which_ candidates it found\n> (and which one it chose!).\n\nAlternatively, just refuse to resolve ambiguous refs. It's not always\nprinted in a short output that stands out to you. Something like this\nperhaps.\n\nIt could probably use some improvements, suggesting the ambiguous\ncandidates too. It's just what I've been using for years.\n\n-- 8< --\nSubject: [PATCH] sha1_name.c: add an option to abort on ambiguous refs\n\nThere are cases when a warning on ambiguous ref may go unnoticed\n(e.g. git-log filling up the whole screen). There are also cases when\npeople want to catch ambiguation early (e.g. it happens deep in some\nscript). In either case, aborting the program would accomplish it.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n Documentation/config/core.txt |  3 ++-\n config.c                      |  5 ++++-\n sha1-name.c                   | 10 ++++++++--\n 3 files changed, 14 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/config/core.txt b/Documentation/config/core.txt\nindex 7e9b6c8f4c..f81634b642 100644\n--- a/Documentation/config/core.txt\n+++ b/Documentation/config/core.txt\n@@ -340,7 +340,8 @@ core.sharedRepository::\n \n core.warnAmbiguousRefs::\n \tIf true, Git will warn you if the ref name you passed it is ambiguous\n-\tand might match multiple refs in the repository. True by default.\n+\tand might match multiple refs in the repository. If set to \"fatal\",\n+\tthe program will abort on ambiguous refs. True by default.\n \n core.compression::\n \tAn integer -1..9, indicating a default compression level.\ndiff --git a/config.c b/config.c\nindex 0f0cdd8c0f..f314caeb87 100644\n--- a/config.c\n+++ b/config.c\n@@ -1158,7 +1158,10 @@ static int git_default_core_config(const char *var, const char *value, void *cb)\n \t}\n \n \tif (!strcmp(var, \"core.warnambiguousrefs\")) {\n-\t\twarn_ambiguous_refs = git_config_bool(var, value);\n+\t\tif (!strcasecmp(value, \"fatal\"))\n+\t\t\twarn_ambiguous_refs = 2;\n+\t\telse\n+\t\t\twarn_ambiguous_refs = git_config_bool(var, value);\n \t\treturn 0;\n \t}\n \ndiff --git a/sha1-name.c b/sha1-name.c\nindex 6dda2c16df..e613c955d7 100644\n--- a/sha1-name.c\n+++ b/sha1-name.c\n@@ -760,6 +760,8 @@ static int get_oid_basic(const char *str, int len, struct object_id *oid,\n \t\t\t\twarning(warn_msg, len, str);\n \t\t\t\tif (advice_object_name_warning)\n \t\t\t\t\tfprintf(stderr, \"%s\\n\", _(object_name_msg));\n+\t\t\t\tif (warn_ambiguous_refs > 1)\n+\t\t\t\t\tdie(_(\"cannot continue with ambiguous refs\"));\n \t\t\t}\n \t\t\tfree(real_ref);\n \t\t}\n@@ -817,8 +819,12 @@ static int get_oid_basic(const char *str, int len, struct object_id *oid,\n \n \tif (warn_ambiguous_refs && !(flags & GET_OID_QUIETLY) &&\n \t    (refs_found > 1 ||\n-\t     !get_short_oid(str, len, &tmp_oid, GET_OID_QUIETLY)))\n-\t\twarning(warn_msg, len, str);\n+\t     !get_short_oid(str, len, &tmp_oid, GET_OID_QUIETLY))) {\n+\t\tif (warn_ambiguous_refs > 1)\n+\t\t\tdie(warn_msg, len, str);\n+\t\telse\n+\t\t\twarning(warn_msg, len, str);\n+\t}\n \n \tif (reflog_len) {\n \t\tint nth, i;\n-- \n2.21.0.1141.gd54ac2cb17\n\n-- 8< --\n"},{"id":"375517","messageId":"20190514120220.GA7714@sigill.intra.peff.net","threadId":"51092","inReplyTo":"20190514112115.GA26957@sigill.intra.peff.net","subject":"[PATCH 0/2] some fixes for help_unknown_ref()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-14T12:02:20Z","receivedAt":"2019-05-14T12:02:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 14, 2019 at 07:21:15AM -0400, Jeff King wrote:\n\n> But I agree we could be more helpful in the messages.\n> \n> The \"did you mean?\" advice just blindly says \"oh, you asked for X and\n> refs/remotes/ABC/X exists, so let's suggest ABC/X\", without checking for\n> ambiguities. It should probably do this:\n\nHere's a patch series to do that.\n\n> I also think the \"warning: refname ... is ambiguous\" message would\n> probably be a bit more helpful if it showed _which_ candidates it found\n> (and which one it chose!).\n\nThis series doesn't do anything for this problem, which I think is a bit\nmore involved. I'm not planning to work on it immediately, if somebody\nelse wants to pick it up (and I see Duy already has a response :) ).\n\n  [1/2]: help_unknown_ref(): duplicate collected refnames\n  [2/2]: help_unknown_ref(): check for refname ambiguity\n\n help.c           |  8 ++++----\n t/t7600-merge.sh | 20 ++++++++++++++++++++\n 2 files changed, 24 insertions(+), 4 deletions(-)\n\n-Peff\n"},{"id":"375518","messageId":"20190514120430.GA27229@sigill.intra.peff.net","threadId":"51092","inReplyTo":"20190514120220.GA7714@sigill.intra.peff.net","subject":"[PATCH 1/2] help_unknown_ref(): duplicate collected refnames","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-14T12:04:31Z","receivedAt":"2019-05-14T12:04:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When \"git merge\" sees an unknown refname, we iterate through the refs to\ntry to suggest some possible alternates. We do so with for_each_ref(),\nand in the callback we add some of the refnames we get to a\nstring_list that is declared with NODUP, directly adding a pointer into\nthe refname string our callback received.\n\nBut the for_each_ref() machinery does not promise that the refname\nstring will remain valid, and as a result we may print garbage memory.\n\nThe code in question dates back to its inception in e56181060e (help:\nadd help_unknown_ref(), 2013-05-04). But back then, the refname strings\ngenerally did remain stable, at least immediately after the\nfor_each_ref() call. Later, in d1cf15516f (packed_ref_iterator_begin():\niterate using `mmapped_ref_iterator`, 2017-09-25), we started\nconsistently re-using a separate buffer for packed refs.\n\nThe fix is simple: duplicate the strings we intend to collect. We\nalready call string_list_clear(), so the memory is correctly freed.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n help.c           |  2 +-\n t/t7600-merge.sh | 14 ++++++++++++++\n 2 files changed, 15 insertions(+), 1 deletion(-)\n\ndiff --git a/help.c b/help.c\nindex a9e451f2ee..d3b3f64e3c 100644\n--- a/help.c\n+++ b/help.c\n@@ -766,7 +766,7 @@ static int append_similar_ref(const char *refname, const struct object_id *oid,\n static struct string_list guess_refs(const char *ref)\n {\n \tstruct similar_ref_cb ref_cb;\n-\tstruct string_list similar_refs = STRING_LIST_INIT_NODUP;\n+\tstruct string_list similar_refs = STRING_LIST_INIT_DUP;\n \n \tref_cb.base_ref = ref;\n \tref_cb.similar_refs = &similar_refs;\ndiff --git a/t/t7600-merge.sh b/t/t7600-merge.sh\nindex 7f9c68cbe7..7551ae3488 100755\n--- a/t/t7600-merge.sh\n+++ b/t/t7600-merge.sh\n@@ -867,4 +867,18 @@ test_expect_success EXECKEEPSPID 'killed merge can be completed with --continue'\n \tverify_parents $c0 $c1\n '\n \n+test_expect_success 'merge suggests matching remote refname' '\n+\tgit commit --allow-empty -m not-local &&\n+\tgit update-ref refs/remotes/origin/not-local HEAD &&\n+\tgit reset --hard HEAD^ &&\n+\n+\t# This is white-box testing hackery; we happen to know\n+\t# that reading packed refs is more picky about the memory\n+\t# ownership of strings we pass to for_each_ref() callbacks.\n+\tgit pack-refs --all --prune &&\n+\n+\ttest_must_fail git merge not-local 2>stderr &&\n+\tgrep origin/not-local stderr\n+'\n+\n test_done\n-- \n2.21.0.1388.g2b1efd806f\n\n"},{"id":"375519","messageId":"20190514120505.GB27229@sigill.intra.peff.net","threadId":"51092","inReplyTo":"20190514120220.GA7714@sigill.intra.peff.net","subject":"[PATCH 2/2] help_unknown_ref(): check for refname ambiguity","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-14T12:05:05Z","receivedAt":"2019-05-14T12:05:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When the user asks to merge \"foo\" and we suggest \"origin/foo\" instead,\nwe do so by simply chopping off \"refs/remotes/\" from the front of the\nsuggested ref. This is usually fine, but it's possible that the\nresulting name is ambiguous (e.g., you have \"refs/heads/origin/foo\",\ntoo).\n\nLet's use shorten_unambiguous_ref() to do this the right way, which\nshould usually yield the same \"origin/foo\", but \"remotes/origin/foo\" if\nnecessary.\n\nNote that in this situation there may be other options (e.g., we could\nsuggest \"heads/origin/foo\" as well). I'll leave that up for debate; the\nfocus here is just to avoid giving advice that does not actually do what\nwe expect.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n help.c           | 6 +++---\n t/t7600-merge.sh | 6 ++++++\n 2 files changed, 9 insertions(+), 3 deletions(-)\n\ndiff --git a/help.c b/help.c\nindex d3b3f64e3c..5261d83ecf 100644\n--- a/help.c\n+++ b/help.c\n@@ -754,12 +754,12 @@ static int append_similar_ref(const char *refname, const struct object_id *oid,\n {\n \tstruct similar_ref_cb *cb = (struct similar_ref_cb *)(cb_data);\n \tchar *branch = strrchr(refname, '/') + 1;\n-\tconst char *remote;\n \n \t/* A remote branch of the same name is deemed similar */\n-\tif (skip_prefix(refname, \"refs/remotes/\", &remote) &&\n+\tif (starts_with(refname, \"refs/remotes/\") &&\n \t    !strcmp(branch, cb->base_ref))\n-\t\tstring_list_append(cb->similar_refs, remote);\n+\t\tstring_list_append_nodup(cb->similar_refs,\n+\t\t\t\t\t shorten_unambiguous_ref(refname, 1));\n \treturn 0;\n }\n \ndiff --git a/t/t7600-merge.sh b/t/t7600-merge.sh\nindex 7551ae3488..2286b0799d 100755\n--- a/t/t7600-merge.sh\n+++ b/t/t7600-merge.sh\n@@ -881,4 +881,10 @@ test_expect_success 'merge suggests matching remote refname' '\n \tgrep origin/not-local stderr\n '\n \n+test_expect_success 'suggested names are not ambiguous' '\n+\tgit update-ref refs/heads/origin/not-local HEAD &&\n+\ttest_must_fail git merge not-local 2>stderr &&\n+\tgrep remotes/origin/not-local stderr\n+'\n+\n test_done\n-- \n2.21.0.1388.g2b1efd806f\n"},{"id":"375521","messageId":"20190514121651.GA27276@sigill.intra.peff.net","threadId":"51092","inReplyTo":"20190514112941.GA23603@ash","subject":"Re: some git confusion (where git's advice didn't help)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-14T12:16:52Z","receivedAt":"2019-05-14T12:16:55Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 14, 2019 at 06:29:41PM +0700, Duy Nguyen wrote:\n\n> On Tue, May 14, 2019 at 07:21:15AM -0400, Jeff King wrote:\n> > I also think the \"warning: refname ... is ambiguous\" message would\n> > probably be a bit more helpful if it showed _which_ candidates it found\n> > (and which one it chose!).\n> \n> Alternatively, just refuse to resolve ambiguous refs. It's not always\n> printed in a short output that stands out to you. Something like this\n> perhaps.\n\nYeah, I actually think that would be much better. I'm not really sure if\nthere's a reason this is only a warning, except for historical inertia.\nIn most other cases where the meaning is not perfectly clear, we try to\nguide the user into disambiguating for us.\n\n> It could probably use some improvements, suggesting the ambiguous\n> candidates too. It's just what I've been using for years.\n\nYes, I still think it would be nice to do that on top, so the user knows\nhow to reissue their command with the correct name.\n\n>  core.warnAmbiguousRefs::\n>  \tIf true, Git will warn you if the ref name you passed it is ambiguous\n> -\tand might match multiple refs in the repository. True by default.\n> +\tand might match multiple refs in the repository. If set to \"fatal\",\n> +\tthe program will abort on ambiguous refs. True by default.\n\nI actually wonder if we should switch it to \"fatal\" by default, but that\ncan be a follow-on. :)\n\n> diff --git a/config.c b/config.c\n> index 0f0cdd8c0f..f314caeb87 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -1158,7 +1158,10 @@ static int git_default_core_config(const char *var, const char *value, void *cb)\n>  \t}\n>  \n>  \tif (!strcmp(var, \"core.warnambiguousrefs\")) {\n> -\t\twarn_ambiguous_refs = git_config_bool(var, value);\n> +\t\tif (!strcasecmp(value, \"fatal\"))\n> +\t\t\twarn_ambiguous_refs = 2;\n> +\t\telse\n> +\t\t\twarn_ambiguous_refs = git_config_bool(var, value);\n>  \t\treturn 0;\n>  \t}\n\nI know you mentioned wanting more cleanup, but I think an enum instead\nof a magic 2 would be nice here.\n\n> @@ -817,8 +819,12 @@ static int get_oid_basic(const char *str, int len, struct object_id *oid,\n>  \n>  \tif (warn_ambiguous_refs && !(flags & GET_OID_QUIETLY) &&\n>  \t    (refs_found > 1 ||\n> -\t     !get_short_oid(str, len, &tmp_oid, GET_OID_QUIETLY)))\n> -\t\twarning(warn_msg, len, str);\n> +\t     !get_short_oid(str, len, &tmp_oid, GET_OID_QUIETLY))) {\n> +\t\tif (warn_ambiguous_refs > 1)\n> +\t\t\tdie(warn_msg, len, str);\n> +\t\telse\n> +\t\t\twarning(warn_msg, len, str);\n> +\t}\n\nThis case makes sense. They say \"foo\", and we say \"nope, ambiguous\", and\nthey come back and say \"heads/foo\" or whatever.\n\nBut...\n\n> @@ -760,6 +760,8 @@ static int get_oid_basic(const char *str, int len, struct object_id *oid,\n>  \t\t\t\twarning(warn_msg, len, str);\n>  \t\t\t\tif (advice_object_name_warning)\n>  \t\t\t\t\tfprintf(stderr, \"%s\\n\", _(object_name_msg));\n> +\t\t\t\tif (warn_ambiguous_refs > 1)\n> +\t\t\t\t\tdie(_(\"cannot continue with ambiguous refs\"));\n>  \t\t\t}\n>  \t\t\tfree(real_ref);\n>  \t\t}\n\nThis one is trickier. Here they said \"1234abcd...\" and we say \"by the\nway, there is also a refs/heads/1234abcd...\". If they really did mean\nthe object id, there's no syntax to say that.\n\n-Peff\n"}]}