some git confusion (where git's advice didn't help)

some git confusion (where git's advice didn't help)

[Ulrich Windl]

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

Re: some git confusion (where git's advice didn't help)

[Jeff King]

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

Re: some git confusion (where git's advice didn't help)

[Duy Nguyen]

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< --

Re: some git confusion (where git's advice didn't help)

[Jeff King]

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

[PATCH 0/2] some fixes for help_unknown_ref()

[Jeff King]

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

[PATCH 1/2] help_unknown_ref(): duplicate collected refnames

[Jeff King]

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

[PATCH 2/2] help_unknown_ref(): check for refname ambiguity

[Jeff King]

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