* some git confusion (where git's advice didn't help) @ 2019-05-14 9:54 Ulrich Windl 2019-05-14 11:21 ` Jeff King 0 siblings, 1 reply; 7+ messages in thread From: Ulrich Windl @ 2019-05-14 9:54 UTC (permalink / raw) To: git Hi! I was fighting with a remote branch being missing: > git branch f-systemd integration master * next > git pull Already up-to-date. > git pull --all Fetching origin Already up-to-date. > git fetch f-gcc-4.8 fatal: 'f-gcc-4.8' does not appear to be a git repository fatal: Could not read from remote repository. Please make sure you have the correct access rights and the repository exists. > git fetch origin > git branch f-systemd integration master * next > git branch -r origin/HEAD -> origin/f-systemd origin/backport-0.0 origin/backport-0.1 origin/f-gcc-4.8 origin/f-manual-peak-reset origin/f-read-failure origin/f-spec-RA origin/f-start-notice origin/f-status-dir origin/f-systemd origin/f-systemd-generator origin/f-usage origin/master origin/next > git branch --track origin/f-gcc-4.8 Branch origin/f-gcc-4.8 set up to track local branch next. > git fetch > git branch f-systemd integration master * next origin/f-gcc-4.8 > git merge f-gcc-4.8 merge: f-gcc-4.8 - not something we can merge Did you mean this? origin/f-gcc-4.8 > git merge origin/f-gcc-4.8 warning: refname 'origin/f-gcc-4.8' is ambiguous. Already up-to-date. ### 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. Actually this was just an addition to my previous message about missing remote branches after clone... (Seen with git 2.12.3) My local "solution" was: git branch -d origin/f-gcc-4.8 Regards, Ulrich ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: some git confusion (where git's advice didn't help) 2019-05-14 9:54 some git confusion (where git's advice didn't help) Ulrich Windl @ 2019-05-14 11:21 ` Jeff King 2019-05-14 11:29 ` Duy Nguyen 2019-05-14 12:02 ` [PATCH 0/2] some fixes for help_unknown_ref() Jeff King 0 siblings, 2 replies; 7+ messages in thread From: Jeff King @ 2019-05-14 11:21 UTC (permalink / raw) To: Ulrich Windl; +Cc: git On Tue, May 14, 2019 at 11:54:43AM +0200, Ulrich Windl wrote: > > git branch --track origin/f-gcc-4.8 > Branch origin/f-gcc-4.8 set up to track local branch next. > > git fetch > > git branch > f-systemd > integration > master > * next > origin/f-gcc-4.8 > > git merge f-gcc-4.8 > merge: f-gcc-4.8 - not something we can merge > > Did you mean this? > origin/f-gcc-4.8 > > git merge origin/f-gcc-4.8 > warning: refname 'origin/f-gcc-4.8' is ambiguous. > Already up-to-date. > > ### 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. Right, that was the source of the problem. Having both "refs/heads/origin/f-gcc-4.8" and "refs/remotes/origin/f-gcc-4.8" is going to lead to confusion, and you're best off deleting the mistaken branch as soon as possible. But I agree we could be more helpful in the messages. The "did you mean?" advice just blindly says "oh, you asked for X and refs/remotes/ABC/X exists, so let's suggest ABC/X", without checking for ambiguities. It should probably do this: diff --git a/help.c b/help.c index a9e451f2ee..108ca54af3 100644 --- a/help.c +++ b/help.c @@ -759,7 +759,8 @@ static int append_similar_ref(const char *refname, const struct object_id *oid, /* A remote branch of the same name is deemed similar */ if (skip_prefix(refname, "refs/remotes/", &remote) && !strcmp(branch, cb->base_ref)) - string_list_append(cb->similar_refs, remote); + string_list_append(cb->similar_refs, + shorten_unambiguous_ref(refname, 1)); return 0; } which would print "ABC/X" in most cases, but "remotes/ABC/X" for your ambiguous case. Incidentally, the existing code also has a memory problem! It blindly skips past "refs/remotes/" in the refname and saves the pointer away in a NODUP string-list. But that refname pointer isn't ours, and isn't guaranteed to last past our for_each_ref() callback. The hunk above fixes it because shorten_unambiguous_ref() always returns a newly allocated string. :) I also think the "warning: refname ... is ambiguous" message would probably be a bit more helpful if it showed _which_ candidates it found (and which one it chose!). -Peff ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: some git confusion (where git's advice didn't help) 2019-05-14 11:21 ` Jeff King @ 2019-05-14 11:29 ` Duy Nguyen 2019-05-14 12:16 ` Jeff King 2019-05-14 12:02 ` [PATCH 0/2] some fixes for help_unknown_ref() Jeff King 1 sibling, 1 reply; 7+ messages in thread From: Duy Nguyen @ 2019-05-14 11:29 UTC (permalink / raw) To: Jeff King; +Cc: Ulrich Windl, git On Tue, May 14, 2019 at 07:21:15AM -0400, Jeff King wrote: > I also think the "warning: refname ... is ambiguous" message would > probably be a bit more helpful if it showed _which_ candidates it found > (and which one it chose!). Alternatively, just refuse to resolve ambiguous refs. It's not always printed in a short output that stands out to you. Something like this perhaps. It could probably use some improvements, suggesting the ambiguous candidates too. It's just what I've been using for years. -- 8< -- Subject: [PATCH] sha1_name.c: add an option to abort on ambiguous refs There are cases when a warning on ambiguous ref may go unnoticed (e.g. git-log filling up the whole screen). There are also cases when people want to catch ambiguation early (e.g. it happens deep in some script). In either case, aborting the program would accomplish it. Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> --- Documentation/config/core.txt | 3 ++- config.c | 5 ++++- sha1-name.c | 10 ++++++++-- 3 files changed, 14 insertions(+), 4 deletions(-) diff --git a/Documentation/config/core.txt b/Documentation/config/core.txt index 7e9b6c8f4c..f81634b642 100644 --- a/Documentation/config/core.txt +++ b/Documentation/config/core.txt @@ -340,7 +340,8 @@ core.sharedRepository:: core.warnAmbiguousRefs:: If true, Git will warn you if the ref name you passed it is ambiguous - and might match multiple refs in the repository. True by default. + and might match multiple refs in the repository. If set to "fatal", + the program will abort on ambiguous refs. True by default. core.compression:: An integer -1..9, indicating a default compression level. diff --git a/config.c b/config.c index 0f0cdd8c0f..f314caeb87 100644 --- a/config.c +++ b/config.c @@ -1158,7 +1158,10 @@ static int git_default_core_config(const char *var, const char *value, void *cb) } if (!strcmp(var, "core.warnambiguousrefs")) { - warn_ambiguous_refs = git_config_bool(var, value); + if (!strcasecmp(value, "fatal")) + warn_ambiguous_refs = 2; + else + warn_ambiguous_refs = git_config_bool(var, value); return 0; } diff --git a/sha1-name.c b/sha1-name.c index 6dda2c16df..e613c955d7 100644 --- a/sha1-name.c +++ b/sha1-name.c @@ -760,6 +760,8 @@ static int get_oid_basic(const char *str, int len, struct object_id *oid, warning(warn_msg, len, str); if (advice_object_name_warning) fprintf(stderr, "%s\n", _(object_name_msg)); + if (warn_ambiguous_refs > 1) + die(_("cannot continue with ambiguous refs")); } free(real_ref); } @@ -817,8 +819,12 @@ static int get_oid_basic(const char *str, int len, struct object_id *oid, if (warn_ambiguous_refs && !(flags & GET_OID_QUIETLY) && (refs_found > 1 || - !get_short_oid(str, len, &tmp_oid, GET_OID_QUIETLY))) - warning(warn_msg, len, str); + !get_short_oid(str, len, &tmp_oid, GET_OID_QUIETLY))) { + if (warn_ambiguous_refs > 1) + die(warn_msg, len, str); + else + warning(warn_msg, len, str); + } if (reflog_len) { int nth, i; -- 2.21.0.1141.gd54ac2cb17 -- 8< -- ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: some git confusion (where git's advice didn't help) 2019-05-14 11:29 ` Duy Nguyen @ 2019-05-14 12:16 ` Jeff King 0 siblings, 0 replies; 7+ messages in thread From: Jeff King @ 2019-05-14 12:16 UTC (permalink / raw) To: Duy Nguyen; +Cc: Ulrich Windl, git On Tue, May 14, 2019 at 06:29:41PM +0700, Duy Nguyen wrote: > On Tue, May 14, 2019 at 07:21:15AM -0400, Jeff King wrote: > > I also think the "warning: refname ... is ambiguous" message would > > probably be a bit more helpful if it showed _which_ candidates it found > > (and which one it chose!). > > Alternatively, just refuse to resolve ambiguous refs. It's not always > printed in a short output that stands out to you. Something like this > perhaps. Yeah, I actually think that would be much better. I'm not really sure if there's a reason this is only a warning, except for historical inertia. In most other cases where the meaning is not perfectly clear, we try to guide the user into disambiguating for us. > It could probably use some improvements, suggesting the ambiguous > candidates too. It's just what I've been using for years. Yes, I still think it would be nice to do that on top, so the user knows how to reissue their command with the correct name. > core.warnAmbiguousRefs:: > If true, Git will warn you if the ref name you passed it is ambiguous > - and might match multiple refs in the repository. True by default. > + and might match multiple refs in the repository. If set to "fatal", > + the program will abort on ambiguous refs. True by default. I actually wonder if we should switch it to "fatal" by default, but that can be a follow-on. :) > diff --git a/config.c b/config.c > index 0f0cdd8c0f..f314caeb87 100644 > --- a/config.c > +++ b/config.c > @@ -1158,7 +1158,10 @@ static int git_default_core_config(const char *var, const char *value, void *cb) > } > > if (!strcmp(var, "core.warnambiguousrefs")) { > - warn_ambiguous_refs = git_config_bool(var, value); > + if (!strcasecmp(value, "fatal")) > + warn_ambiguous_refs = 2; > + else > + warn_ambiguous_refs = git_config_bool(var, value); > return 0; > } I know you mentioned wanting more cleanup, but I think an enum instead of a magic 2 would be nice here. > @@ -817,8 +819,12 @@ static int get_oid_basic(const char *str, int len, struct object_id *oid, > > if (warn_ambiguous_refs && !(flags & GET_OID_QUIETLY) && > (refs_found > 1 || > - !get_short_oid(str, len, &tmp_oid, GET_OID_QUIETLY))) > - warning(warn_msg, len, str); > + !get_short_oid(str, len, &tmp_oid, GET_OID_QUIETLY))) { > + if (warn_ambiguous_refs > 1) > + die(warn_msg, len, str); > + else > + warning(warn_msg, len, str); > + } This case makes sense. They say "foo", and we say "nope, ambiguous", and they come back and say "heads/foo" or whatever. But... > @@ -760,6 +760,8 @@ static int get_oid_basic(const char *str, int len, struct object_id *oid, > warning(warn_msg, len, str); > if (advice_object_name_warning) > fprintf(stderr, "%s\n", _(object_name_msg)); > + if (warn_ambiguous_refs > 1) > + die(_("cannot continue with ambiguous refs")); > } > free(real_ref); > } This one is trickier. Here they said "1234abcd..." and we say "by the way, there is also a refs/heads/1234abcd...". If they really did mean the object id, there's no syntax to say that. -Peff ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 0/2] some fixes for help_unknown_ref() 2019-05-14 11:21 ` Jeff King 2019-05-14 11:29 ` Duy Nguyen @ 2019-05-14 12:02 ` Jeff King 2019-05-14 12:04 ` [PATCH 1/2] help_unknown_ref(): duplicate collected refnames Jeff King 2019-05-14 12:05 ` [PATCH 2/2] help_unknown_ref(): check for refname ambiguity Jeff King 1 sibling, 2 replies; 7+ messages in thread From: Jeff King @ 2019-05-14 12:02 UTC (permalink / raw) To: Ulrich Windl; +Cc: git On Tue, May 14, 2019 at 07:21:15AM -0400, Jeff King wrote: > But I agree we could be more helpful in the messages. > > The "did you mean?" advice just blindly says "oh, you asked for X and > refs/remotes/ABC/X exists, so let's suggest ABC/X", without checking for > ambiguities. It should probably do this: Here's a patch series to do that. > I also think the "warning: refname ... is ambiguous" message would > probably be a bit more helpful if it showed _which_ candidates it found > (and which one it chose!). This series doesn't do anything for this problem, which I think is a bit more involved. I'm not planning to work on it immediately, if somebody else wants to pick it up (and I see Duy already has a response :) ). [1/2]: help_unknown_ref(): duplicate collected refnames [2/2]: help_unknown_ref(): check for refname ambiguity help.c | 8 ++++---- t/t7600-merge.sh | 20 ++++++++++++++++++++ 2 files changed, 24 insertions(+), 4 deletions(-) -Peff ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2] help_unknown_ref(): duplicate collected refnames 2019-05-14 12:02 ` [PATCH 0/2] some fixes for help_unknown_ref() Jeff King @ 2019-05-14 12:04 ` Jeff King 2019-05-14 12:05 ` [PATCH 2/2] help_unknown_ref(): check for refname ambiguity Jeff King 1 sibling, 0 replies; 7+ messages in thread From: Jeff King @ 2019-05-14 12:04 UTC (permalink / raw) To: Ulrich Windl; +Cc: git When "git merge" sees an unknown refname, we iterate through the refs to try to suggest some possible alternates. We do so with for_each_ref(), and in the callback we add some of the refnames we get to a string_list that is declared with NODUP, directly adding a pointer into the refname string our callback received. But the for_each_ref() machinery does not promise that the refname string will remain valid, and as a result we may print garbage memory. The code in question dates back to its inception in e56181060e (help: add help_unknown_ref(), 2013-05-04). But back then, the refname strings generally did remain stable, at least immediately after the for_each_ref() call. Later, in d1cf15516f (packed_ref_iterator_begin(): iterate using `mmapped_ref_iterator`, 2017-09-25), we started consistently re-using a separate buffer for packed refs. The fix is simple: duplicate the strings we intend to collect. We already call string_list_clear(), so the memory is correctly freed. Signed-off-by: Jeff King <peff@peff.net> --- help.c | 2 +- t/t7600-merge.sh | 14 ++++++++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/help.c b/help.c index a9e451f2ee..d3b3f64e3c 100644 --- a/help.c +++ b/help.c @@ -766,7 +766,7 @@ static int append_similar_ref(const char *refname, const struct object_id *oid, static struct string_list guess_refs(const char *ref) { struct similar_ref_cb ref_cb; - struct string_list similar_refs = STRING_LIST_INIT_NODUP; + struct string_list similar_refs = STRING_LIST_INIT_DUP; ref_cb.base_ref = ref; ref_cb.similar_refs = &similar_refs; diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh index 7f9c68cbe7..7551ae3488 100755 --- a/t/t7600-merge.sh +++ b/t/t7600-merge.sh @@ -867,4 +867,18 @@ test_expect_success EXECKEEPSPID 'killed merge can be completed with --continue' verify_parents $c0 $c1 ' +test_expect_success 'merge suggests matching remote refname' ' + git commit --allow-empty -m not-local && + git update-ref refs/remotes/origin/not-local HEAD && + git reset --hard HEAD^ && + + # This is white-box testing hackery; we happen to know + # that reading packed refs is more picky about the memory + # ownership of strings we pass to for_each_ref() callbacks. + git pack-refs --all --prune && + + test_must_fail git merge not-local 2>stderr && + grep origin/not-local stderr +' + test_done -- 2.21.0.1388.g2b1efd806f ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] help_unknown_ref(): check for refname ambiguity 2019-05-14 12:02 ` [PATCH 0/2] some fixes for help_unknown_ref() Jeff King 2019-05-14 12:04 ` [PATCH 1/2] help_unknown_ref(): duplicate collected refnames Jeff King @ 2019-05-14 12:05 ` Jeff King 1 sibling, 0 replies; 7+ messages in thread From: Jeff King @ 2019-05-14 12:05 UTC (permalink / raw) To: Ulrich Windl; +Cc: git When the user asks to merge "foo" and we suggest "origin/foo" instead, we do so by simply chopping off "refs/remotes/" from the front of the suggested ref. This is usually fine, but it's possible that the resulting name is ambiguous (e.g., you have "refs/heads/origin/foo", too). Let's use shorten_unambiguous_ref() to do this the right way, which should usually yield the same "origin/foo", but "remotes/origin/foo" if necessary. Note that in this situation there may be other options (e.g., we could suggest "heads/origin/foo" as well). I'll leave that up for debate; the focus here is just to avoid giving advice that does not actually do what we expect. Signed-off-by: Jeff King <peff@peff.net> --- help.c | 6 +++--- t/t7600-merge.sh | 6 ++++++ 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/help.c b/help.c index d3b3f64e3c..5261d83ecf 100644 --- a/help.c +++ b/help.c @@ -754,12 +754,12 @@ static int append_similar_ref(const char *refname, const struct object_id *oid, { struct similar_ref_cb *cb = (struct similar_ref_cb *)(cb_data); char *branch = strrchr(refname, '/') + 1; - const char *remote; /* A remote branch of the same name is deemed similar */ - if (skip_prefix(refname, "refs/remotes/", &remote) && + if (starts_with(refname, "refs/remotes/") && !strcmp(branch, cb->base_ref)) - string_list_append(cb->similar_refs, remote); + string_list_append_nodup(cb->similar_refs, + shorten_unambiguous_ref(refname, 1)); return 0; } diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh index 7551ae3488..2286b0799d 100755 --- a/t/t7600-merge.sh +++ b/t/t7600-merge.sh @@ -881,4 +881,10 @@ test_expect_success 'merge suggests matching remote refname' ' grep origin/not-local stderr ' +test_expect_success 'suggested names are not ambiguous' ' + git update-ref refs/heads/origin/not-local HEAD && + test_must_fail git merge not-local 2>stderr && + grep remotes/origin/not-local stderr +' + test_done -- 2.21.0.1388.g2b1efd806f ^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-05-14 12:16 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-05-14 9:54 some git confusion (where git's advice didn't help) Ulrich Windl 2019-05-14 11:21 ` Jeff King 2019-05-14 11:29 ` Duy Nguyen 2019-05-14 12:16 ` Jeff King 2019-05-14 12:02 ` [PATCH 0/2] some fixes for help_unknown_ref() Jeff King 2019-05-14 12:04 ` [PATCH 1/2] help_unknown_ref(): duplicate collected refnames Jeff King 2019-05-14 12:05 ` [PATCH 2/2] help_unknown_ref(): check for refname ambiguity Jeff King
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).