Regression in 'git branch -m'?

Regression in 'git branch -m'?

[Andreas Krey]

Hi everybody,

I got something that looks like a regression somewhere since 2.11.
This script

  set -xe
  rm -rf repo
  git init repo
  cd repo
  git commit -m nix --allow-empty
  git branch -m master/master
  git rev-parse HEAD
  git branch
  git status

causes .git/HEAD to still contain 'ref: refs/heads/master' and to fail
in the rev-parse step with

  + git rev-parse HEAD
  HEAD
  fatal: ambiguous argument 'HEAD': unknown revision or path not in the working tree.
  Use '--' to separate paths from revisions, like this:
  'git <command> [<revision>...] -- [<file>...]'

This is with 2.15.0.rc0; with 2.11.0 (and 2.11.0.356.gffac48d09) it still works.

I'm going to do a bisect on this as battery permits.

- Andreas

-- 
"Totally trivial. Famous last words."
From: Linus Torvalds <torvalds@*.org>
Date: Fri, 22 Jan 2010 07:29:21 -0800

Re: Regression in 'git branch -m'?

[Jeff King]

On Thu, Oct 05, 2017 at 07:25:52PM +0200, Andreas Krey wrote:

> I got something that looks like a regression somewhere since 2.11.
> This script
> 
>   set -xe
>   rm -rf repo
>   git init repo
>   cd repo
>   git commit -m nix --allow-empty
>   git branch -m master/master
>   git rev-parse HEAD
>   git branch
>   git status
> 
> causes .git/HEAD to still contain 'ref: refs/heads/master' and to fail
> in the rev-parse step with
> 
>   + git rev-parse HEAD
>   HEAD
>   fatal: ambiguous argument 'HEAD': unknown revision or path not in the working tree.
>   Use '--' to separate paths from revisions, like this:
>   'git <command> [<revision>...] -- [<file>...]'
> 
> This is with 2.15.0.rc0; with 2.11.0 (and 2.11.0.356.gffac48d09) it still works.
> 
> I'm going to do a bisect on this as battery permits.

Looks like 31824d180d (branch: fix branch renaming not updating HEADs
correctly, 2017-08-24). This is in v2.15.0-rc0, so we should figure it
out before the upcoming release.

I didn't dig very far, but it looks like the branch name is important
"foo" doesn't trigger the problem but "master/master" does. "master/foo"
also does, but "foo/master" does not. So I suspect it's something about
how resolve_ref handles the failure when it would not be able to create
the ref because of the d/f conflict. So it's probably related to losing
the RESOLVE_REF_READING in the final hunk of that patch. That's just a
guess for now, though.

-Peff

Re: Regression in 'git branch -m'?

[Jeff King]

On Thu, Oct 05, 2017 at 02:33:03PM -0400, Jeff King wrote:

> Looks like 31824d180d (branch: fix branch renaming not updating HEADs
> correctly, 2017-08-24). This is in v2.15.0-rc0, so we should figure it
> out before the upcoming release.
> 
> I didn't dig very far, but it looks like the branch name is important
> "foo" doesn't trigger the problem but "master/master" does. "master/foo"
> also does, but "foo/master" does not. So I suspect it's something about
> how resolve_ref handles the failure when it would not be able to create
> the ref because of the d/f conflict. So it's probably related to losing
> the RESOLVE_REF_READING in the final hunk of that patch. That's just a
> guess for now, though.

I got a chance to look at this again. I think the root of the problem is
that resolve_ref() as it is implemented now is just totally unsuitable
for asking the question "what does this symbolic link point to?".

Because you end up with either:

  1. If we pass RESOLVE_REF_READING, then we do not return the target
     refname for orphaned commits (which is why 31824d180d dropped it).

  2. If not, then we do not return the target refname for commits with
     names that are not available for writing. The d/f conflict here is
     one example, but there may be others.

So I think we need to teach resolve_ref() a new mode that's like
"reading", but just follows the symref chain.

In the meantime, here's a test which shows off the regression.

diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 3ac7ebf85f..503a88d029 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -117,6 +117,16 @@ test_expect_success 'git branch -m bbb should rename checked out branch' '
 	test_cmp expect actual
 '
 
+test_expect_success 'renaming checked out branch works with d/f conflict' '
+	test_when_finished "git branch -D foo/bar || git branch -D foo" &&
+	test_when_finished git checkout master &&
+	git checkout -b foo &&
+	git branch -m foo/bar &&
+	git symbolic-ref HEAD >actual &&
+	echo refs/heads/foo/bar >expect &&
+	test_cmp expect actual
+'
+
 test_expect_success 'git branch -m o/o o should fail when o/p exists' '
 	git branch o/o &&
 	git branch o/p &&

-Peff

Re: Regression in 'git branch -m'?

[Jeff King]

On Fri, Oct 06, 2017 at 03:39:13AM -0400, Jeff King wrote:

> I got a chance to look at this again. I think the root of the problem is
> that resolve_ref() as it is implemented now is just totally unsuitable
> for asking the question "what does this symbolic link point to?".
> 
> Because you end up with either:
> 
>   1. If we pass RESOLVE_REF_READING, then we do not return the target
>      refname for orphaned commits (which is why 31824d180d dropped it).
> 
>   2. If not, then we do not return the target refname for commits with
>      names that are not available for writing. The d/f conflict here is
>      one example, but there may be others.
> 
> So I think we need to teach resolve_ref() a new mode that's like
> "reading", but just follows the symref chain.

This analysis is not _quite_ right. The "not available for writing"
thing actually isn't intentionally enforced by the resolve_ref. It's
just that it's not careful enough about checking errno. We see EISDIR
instead of ENOENT when there's a d/f situation, but both have the same
practical effect: that ref doesn't exist.

I.e., this lookup has _always_ been broken, even in the "reading" case.
It's just that the fix from 31824d180d (correctly) made git-branch more
careful about handling the cases where we couldn't resolve a HEAD.

So this patch fixes the problem:

diff --git a/refs.c b/refs.c
index df075fcd06..2ba74720c8 100644
--- a/refs.c
+++ b/refs.c
@@ -1435,7 +1435,8 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs,
 		if (refs_read_raw_ref(refs, refname,
 				      sha1, &sb_refname, &read_flags)) {
 			*flags |= read_flags;
-			if (errno != ENOENT || (resolve_flags & RESOLVE_REF_READING))
+			if ((errno != ENOENT && errno != EISDIR) ||
+			    (resolve_flags & RESOLVE_REF_READING))
 				return NULL;
 			hashclr(sha1);
 			if (*flags & REF_BAD_NAME)

but seems to stimulate a test failure in t3308. I have a suspicion that
I've just uncovered another bug, but I'll dig in that. In the meantime I
wanted to post this update in case anybody else was looking into it.

-Peff

Re: Regression in 'git branch -m'?

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> So this patch fixes the problem:
>
> diff --git a/refs.c b/refs.c
> index df075fcd06..2ba74720c8 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1435,7 +1435,8 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs,
>  		if (refs_read_raw_ref(refs, refname,
>  				      sha1, &sb_refname, &read_flags)) {
>  			*flags |= read_flags;
> -			if (errno != ENOENT || (resolve_flags & RESOLVE_REF_READING))
> +			if ((errno != ENOENT && errno != EISDIR) ||
> +			    (resolve_flags & RESOLVE_REF_READING))

Ooo, good find--is_missing_file_error() strikes back...

>  				return NULL;
>  			hashclr(sha1);
>  			if (*flags & REF_BAD_NAME)
>
> but seems to stimulate a test failure in t3308. I have a suspicion that
> I've just uncovered another bug, but I'll dig in that. In the meantime I
> wanted to post this update in case anybody else was looking into it.
>
> -Peff

Re: Regression in 'git branch -m'?

[Jeff King]

On Fri, Oct 06, 2017 at 06:45:08PM +0900, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > So this patch fixes the problem:
> >
> > diff --git a/refs.c b/refs.c
> > index df075fcd06..2ba74720c8 100644
> > --- a/refs.c
> > +++ b/refs.c
> > @@ -1435,7 +1435,8 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs,
> >  		if (refs_read_raw_ref(refs, refname,
> >  				      sha1, &sb_refname, &read_flags)) {
> >  			*flags |= read_flags;
> > -			if (errno != ENOENT || (resolve_flags & RESOLVE_REF_READING))
> > +			if ((errno != ENOENT && errno != EISDIR) ||
> > +			    (resolve_flags & RESOLVE_REF_READING))
> 
> Ooo, good find--is_missing_file_error() strikes back...

Almost. That uses ENOTDIR, so that looking for "foo/bar" handles the
case where "foo" is a regular file.

But this is the opposite: we ask about "foo", but "foo/bar" exists. The
answer isn't "it's not there" in the general case, but "it's not the
thing you were expecting".

But in the case of refs, the filesystem is just a representation of the
abstract namespace. In asking for "refs/heads/foo", if "refs/heads/foo/bar"
exists, then answer is still "no, it's not a ref".

So EISDIR is needed for this case, though I suspect the opposite case
would need ENOTDIR. I actually wonder if the files-backend read_raw_ref
ought to be normalizing all of those to ENOENT.

> >  				return NULL;
> >  			hashclr(sha1);
> >  			if (*flags & REF_BAD_NAME)
> >
> > but seems to stimulate a test failure in t3308. I have a suspicion that
> > I've just uncovered another bug, but I'll dig in that. In the meantime I
> > wanted to post this update in case anybody else was looking into it.

That failure indeed turned out to be a red herring. So I think I'm
definitely onto the right track.

I want to play with the ENOTDIR case, and then I'll write up the whole
thing and send it in later today.

-Peff

Re: Regression in 'git branch -m'?

[Jeff King]

On Thu, Oct 05, 2017 at 07:25:52PM +0200, Andreas Krey wrote:

> I got something that looks like a regression somewhere since 2.11.
> This script
> 
>   set -xe
>   rm -rf repo
>   git init repo
>   cd repo
>   git commit -m nix --allow-empty
>   git branch -m master/master
>   git rev-parse HEAD
>   git branch
>   git status
> 
> causes .git/HEAD to still contain 'ref: refs/heads/master' and to fail
> in the rev-parse step with
> 
>   + git rev-parse HEAD
>   HEAD
>   fatal: ambiguous argument 'HEAD': unknown revision or path not in the working tree.
>   Use '--' to separate paths from revisions, like this:
>   'git <command> [<revision>...] -- [<file>...]'
> 
> This is with 2.15.0.rc0; with 2.11.0 (and 2.11.0.356.gffac48d09) it still works.

So this turned out to be quite an interesting bug to explore. I think
the solution I ended up with in the second patch is the right thing. I'm
adding Michael to the cc for wisdom on the ref code, though I think the
bug I'm fixing actually goes back to the early days of Git.

Earlier I blamed Duy's 31824d180d. And that is the start of the
regression in v2.15, but only because it fixed another bug which was
papering over the one I'm fixing here. :)

  [v1 1/2]: t3308: create a real ref directory/file conflict
  [v1 2/2]: refs_resolve_ref_unsafe: handle d/f conflicts for writes

 refs.c                  | 15 ++++++++++++++-
 t/t1401-symbolic-ref.sh | 26 +++++++++++++++++++++++++-
 t/t3200-branch.sh       | 10 ++++++++++
 t/t3308-notes-merge.sh  |  2 +-
 4 files changed, 50 insertions(+), 3 deletions(-)

-Peff

[PATCH 1/2] t3308: create a real ref directory/file conflict

[Jeff King]

A test in t3308 wants to make sure that we don't
accidentally merge into "refs/notes/dir" when it exists as a
directory, so it does:

  mkdir .git/refs/notes/dir
  git -c core.notesRef=refs/notes/dir merge ...

and expects the second command to fail. But that
understimates the refs code, which is smart enough to remove
useless directories in the refs hierarchy. The test
succeeded only because of a bug which prevented resolving
refs/notes/dir for writing, even though an actual ref update
would succeed.

In preparation for fixing that bug, let's switch to creating
a real ref in refs/notes/dir, which is a more realistic
situation.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t3308-notes-merge.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t3308-notes-merge.sh b/t/t3308-notes-merge.sh
index 19aed7ec95..ab946a5153 100755
--- a/t/t3308-notes-merge.sh
+++ b/t/t3308-notes-merge.sh
@@ -79,7 +79,7 @@ test_expect_success 'fail to merge empty notes ref into empty notes ref (z => y)
 test_expect_success 'fail to merge into various non-notes refs' '
 	test_must_fail git -c "core.notesRef=refs/notes" notes merge x &&
 	test_must_fail git -c "core.notesRef=refs/notes/" notes merge x &&
-	mkdir -p .git/refs/notes/dir &&
+	git update-ref refs/notes/dir/foo HEAD &&
 	test_must_fail git -c "core.notesRef=refs/notes/dir" notes merge x &&
 	test_must_fail git -c "core.notesRef=refs/notes/dir/" notes merge x &&
 	test_must_fail git -c "core.notesRef=refs/heads/master" notes merge x &&
-- 
2.15.0.rc0.413.g9bb4ac64e2

[PATCH 2/2] refs_resolve_ref_unsafe: handle d/f conflicts for writes

[Jeff King]

If our call to refs_read_raw_ref() fails, we check errno to
see if the ref is simply missing, or if we encountered a
more serious error. If it's just missing, then in "write"
mode (i.e., when RESOLVE_REFS_READING is not set), this is
perfectly fine.

However, checking for ENOENT isn't sufficient to catch all
missing-ref cases. In the filesystem backend, we may also
see EISDIR when we try to resolve "a" and "a/b" exists.
Likewise, we may see ENOTDIR if we try to resolve "a/b" and
"a" exists. In both of those cases, we know that our
resolved ref doesn't exist, but we return an error (rather
than reporting the refname and returning a null sha1).

This has been broken for a long time, but nobody really
noticed because the next step after resolving without the
READING flag is usually to lock the ref and write it. But in
both of those cases, the write will fail with the same
errno due to the directory/file conflict.

There are two cases where we can notice this, though:

  1. If we try to write "a" and there's a leftover directory
     already at "a", even though there is no ref "a/b". The
     actual write is smart enough to move the empty "a" out
     of the way.

     This is reasonably rare, if only because the writing
     code has to do an independent resolution before trying
     its write (because the actual update_ref() code handles
     this case fine). The notes-merge code does this, and
     before the fix in the prior commit t3308 erroneously
     expected this case to fail.

  2. When resolving symbolic refs, we typically do not use
     the READING flag because we want to resolve even
     symrefs that point to unborn refs. Even if those unborn
     refs could not actually be written because of d/f
     conflicts with existing refs.

     You can see this by asking "git symbolic-ref" to report
     the target of a symref pointing past a d/f conflict.

We can fix the problem by recognizing the other "missing"
errnos and treating them like ENOENT. This should be safe to
do even for callers who are then going to actually write the
ref, because the actual writing process will fail if the d/f
conflict is a real one (and t1404 checks these cases).

Arguably this should be the responsibility of the
files-backend to normalize all "missing ref" errors into
ENOENT (since something like EISDIR may not be meaningful at
all to a database backend). However other callers of
refs_read_raw_ref() may actually care about the distinction;
putting this into resolve_ref() is the minimal fix for now.

The new tests in t1401 use git-symbolic-ref, which is the
most direct way to check the resolution by itself.
Interestingly we actually had a test that setup this case
already, but we only used it to verify that the funny state
could be overwritten, not that it could be resolved.

We also add a new test in t3200, as "branch -m" was the
original motivation for looking into this. What happens is
this:

  0. HEAD is pointing to branch "a"

  1. The user asks to rename "a" to "a/b".

  2. We create "a/b" and delete "a".

  3. We then try to update any worktree HEADs that point to
     the renamed ref (including the main repo HEAD). To do
     that, we have to resolve each HEAD. But now our HEAD is
     pointing at "a", and we get EISDIR due to the loose
     "a/b". As a result, we think there is no HEAD, and we
     do not update it. It now points to the bogus "a".

Interestingly this case used to work, but only accidentally.
Before 31824d180d (branch: fix branch renaming not updating
HEADs correctly, 2017-08-24), we'd update any HEAD which we
couldn't resolve. That was wrong, but it papered over the
fact that we were incorrectly failing to resolve HEAD.

So while the bug demonstrated by the git-symbolic-ref is
quite old, the regression to "branch -m" is recent.

Signed-off-by: Jeff King <peff@peff.net>
---
 refs.c                  | 15 ++++++++++++++-
 t/t1401-symbolic-ref.sh | 26 +++++++++++++++++++++++++-
 t/t3200-branch.sh       | 10 ++++++++++
 3 files changed, 49 insertions(+), 2 deletions(-)

diff --git a/refs.c b/refs.c
index df075fcd06..c590a992fb 100644
--- a/refs.c
+++ b/refs.c
@@ -1435,8 +1435,21 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs,
 		if (refs_read_raw_ref(refs, refname,
 				      sha1, &sb_refname, &read_flags)) {
 			*flags |= read_flags;
-			if (errno != ENOENT || (resolve_flags & RESOLVE_REF_READING))
+
+			/* In reading mode, refs must eventually resolve */
+			if (resolve_flags & RESOLVE_REF_READING)
+				return NULL;
+
+			/*
+			 * Otherwise a missing ref is OK. But the files backend
+			 * may show errors besides ENOENT if there are
+			 * similarly-named refs.
+			 */
+			if (errno != ENOENT &&
+			    errno != EISDIR &&
+			    errno != ENOTDIR)
 				return NULL;
+
 			hashclr(sha1);
 			if (*flags & REF_BAD_NAME)
 				*flags |= REF_ISBROKEN;
diff --git a/t/t1401-symbolic-ref.sh b/t/t1401-symbolic-ref.sh
index eec3e90f9c..9e782a8122 100755
--- a/t/t1401-symbolic-ref.sh
+++ b/t/t1401-symbolic-ref.sh
@@ -129,11 +129,35 @@ test_expect_success 'symbolic-ref does not create ref d/f conflicts' '
 	test_must_fail git symbolic-ref refs/heads/df/conflict refs/heads/df
 '
 
-test_expect_success 'symbolic-ref handles existing pointer to invalid name' '
+test_expect_success 'symbolic-ref can overwrite pointer to invalid name' '
+	test_when_finished reset_to_sane &&
 	head=$(git rev-parse HEAD) &&
 	git symbolic-ref HEAD refs/heads/outer &&
+	test_when_finished "git update-ref -d refs/heads/outer/inner" &&
 	git update-ref refs/heads/outer/inner $head &&
 	git symbolic-ref HEAD refs/heads/unrelated
 '
 
+test_expect_success 'symbolic-ref can resolve d/f name (EISDIR)' '
+	test_when_finished reset_to_sane &&
+	head=$(git rev-parse HEAD) &&
+	git symbolic-ref HEAD refs/heads/outer/inner &&
+	test_when_finished "git update-ref -d refs/heads/outer" &&
+	git update-ref refs/heads/outer $head &&
+	echo refs/heads/outer/inner >expect &&
+	git symbolic-ref HEAD >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'symbolic-ref can resolve d/f name (ENOTDIR)' '
+	test_when_finished reset_to_sane &&
+	head=$(git rev-parse HEAD) &&
+	git symbolic-ref HEAD refs/heads/outer &&
+	test_when_finished "git update-ref -d refs/heads/outer/inner" &&
+	git update-ref refs/heads/outer/inner $head &&
+	echo refs/heads/outer >expect &&
+	git symbolic-ref HEAD >actual &&
+	test_cmp expect actual
+'
+
 test_done
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 3ac7ebf85f..503a88d029 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -117,6 +117,16 @@ test_expect_success 'git branch -m bbb should rename checked out branch' '
 	test_cmp expect actual
 '
 
+test_expect_success 'renaming checked out branch works with d/f conflict' '
+	test_when_finished "git branch -D foo/bar || git branch -D foo" &&
+	test_when_finished git checkout master &&
+	git checkout -b foo &&
+	git branch -m foo/bar &&
+	git symbolic-ref HEAD >actual &&
+	echo refs/heads/foo/bar >expect &&
+	test_cmp expect actual
+'
+
 test_expect_success 'git branch -m o/o o should fail when o/p exists' '
 	git branch o/o &&
 	git branch o/p &&
-- 
2.15.0.rc0.413.g9bb4ac64e2

Re: [PATCH 2/2] refs_resolve_ref_unsafe: handle d/f conflicts for writes

[Michael Haggerty]

On 10/06/2017 04:42 PM, Jeff King wrote:
> If our call to refs_read_raw_ref() fails, we check errno to
> see if the ref is simply missing, or if we encountered a
> more serious error. If it's just missing, then in "write"
> mode (i.e., when RESOLVE_REFS_READING is not set), this is
> perfectly fine.
> 
> However, checking for ENOENT isn't sufficient to catch all
> missing-ref cases. In the filesystem backend, we may also
> see EISDIR when we try to resolve "a" and "a/b" exists.
> Likewise, we may see ENOTDIR if we try to resolve "a/b" and
> "a" exists. In both of those cases, we know that our
> resolved ref doesn't exist, but we return an error (rather
> than reporting the refname and returning a null sha1).
> 
> This has been broken for a long time, but nobody really
> noticed because the next step after resolving without the
> READING flag is usually to lock the ref and write it. But in
> both of those cases, the write will fail with the same
> errno due to the directory/file conflict.
> 
> There are two cases where we can notice this, though:
> 
>   1. If we try to write "a" and there's a leftover directory
>      already at "a", even though there is no ref "a/b". The
>      actual write is smart enough to move the empty "a" out
>      of the way.
> 
>      This is reasonably rare, if only because the writing
>      code has to do an independent resolution before trying
>      its write (because the actual update_ref() code handles
>      this case fine). The notes-merge code does this, and
>      before the fix in the prior commit t3308 erroneously
>      expected this case to fail.
> 
>   2. When resolving symbolic refs, we typically do not use
>      the READING flag because we want to resolve even
>      symrefs that point to unborn refs. Even if those unborn
>      refs could not actually be written because of d/f
>      conflicts with existing refs.
> 
>      You can see this by asking "git symbolic-ref" to report
>      the target of a symref pointing past a d/f conflict.
> 
> We can fix the problem by recognizing the other "missing"
> errnos and treating them like ENOENT. This should be safe to
> do even for callers who are then going to actually write the
> ref, because the actual writing process will fail if the d/f
> conflict is a real one (and t1404 checks these cases).
> 
> Arguably this should be the responsibility of the
> files-backend to normalize all "missing ref" errors into
> ENOENT (since something like EISDIR may not be meaningful at
> all to a database backend). However other callers of
> refs_read_raw_ref() may actually care about the distinction;
> putting this into resolve_ref() is the minimal fix for now.
> 
> The new tests in t1401 use git-symbolic-ref, which is the
> most direct way to check the resolution by itself.
> Interestingly we actually had a test that setup this case
> already, but we only used it to verify that the funny state
> could be overwritten, not that it could be resolved.
> 
> We also add a new test in t3200, as "branch -m" was the
> original motivation for looking into this. What happens is
> this:
> 
>   0. HEAD is pointing to branch "a"
> 
>   1. The user asks to rename "a" to "a/b".
> 
>   2. We create "a/b" and delete "a".
> 
>   3. We then try to update any worktree HEADs that point to
>      the renamed ref (including the main repo HEAD). To do
>      that, we have to resolve each HEAD. But now our HEAD is
>      pointing at "a", and we get EISDIR due to the loose
>      "a/b". As a result, we think there is no HEAD, and we
>      do not update it. It now points to the bogus "a".
> 
> Interestingly this case used to work, but only accidentally.
> Before 31824d180d (branch: fix branch renaming not updating
> HEADs correctly, 2017-08-24), we'd update any HEAD which we
> couldn't resolve. That was wrong, but it papered over the
> fact that we were incorrectly failing to resolve HEAD.
> 
> So while the bug demonstrated by the git-symbolic-ref is
> quite old, the regression to "branch -m" is recent.

Thanks for your detailed investigation and analysis and for the new tests.

This makes sense to me at the level of fixing the bug.

I do have one twinge of uneasiness at a deeper level, that I haven't had
time to check...

Does this patch make it easier to *set* HEAD to an unborn branch that
d/f conflicts with an existing reference? If so, that might be a
slightly worse UI for users. I'd rather learn about such a problem when
setting HEAD (when I am thinking about the new branch name and am in the
frame of mind to solve the problem) rather than later, when I try to
commit to the new branch.

Even if so, that wouldn't be a problem with this patch per se, but
rather a possible accidental side-effect of fixing the bug.

Michael

> [...]

Re: [PATCH 2/2] refs_resolve_ref_unsafe: handle d/f conflicts for writes

[Jeff King]

On Fri, Oct 06, 2017 at 07:09:10PM +0200, Michael Haggerty wrote:

> I do have one twinge of uneasiness at a deeper level, that I haven't had
> time to check...
> 
> Does this patch make it easier to *set* HEAD to an unborn branch that
> d/f conflicts with an existing reference? If so, that might be a
> slightly worse UI for users. I'd rather learn about such a problem when
> setting HEAD (when I am thinking about the new branch name and am in the
> frame of mind to solve the problem) rather than later, when I try to
> commit to the new branch.

Good question. The answer is no, it's allowed both before and after my
patch. At least via git-symbolic-ref.

I agree it would be nice to know earlier for such a case. For
symbolic-ref, we probably should allow it, because it's plumbing that
may be used for tricky things. For things like "checkout -b", you'd
generally get a timely warning as we try to create the ref.

The odd man out is "checkout --orphan", which leaves the branch unborn.
It might be nice if it did a manual check that the ref is available (and
also that it's syntactically acceptable, though I think we may do that
already).

But all of that is orthogonal to this fix, I think.

-Peff

Re: Regression in 'git branch -m'?

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Earlier I blamed Duy's 31824d180d. And that is the start of the
> regression in v2.15, but only because it fixed another bug which was
> papering over the one I'm fixing here. :)

I haven't read Michael's reply, but 2/2 is very well explained and
looks correct.

Thanks for digging this through to the root.  

>   [v1 1/2]: t3308: create a real ref directory/file conflict
>   [v1 2/2]: refs_resolve_ref_unsafe: handle d/f conflicts for writes
>
>  refs.c                  | 15 ++++++++++++++-
>  t/t1401-symbolic-ref.sh | 26 +++++++++++++++++++++++++-
>  t/t3200-branch.sh       | 10 ++++++++++
>  t/t3308-notes-merge.sh  |  2 +-
>  4 files changed, 50 insertions(+), 3 deletions(-)
>
> -Peff

Re: [PATCH 2/2] refs_resolve_ref_unsafe: handle d/f conflicts for writes

[Michael Haggerty]

On 10/06/2017 07:16 PM, Jeff King wrote:
> On Fri, Oct 06, 2017 at 07:09:10PM +0200, Michael Haggerty wrote:
> 
>> I do have one twinge of uneasiness at a deeper level, that I haven't had
>> time to check...
>>
>> Does this patch make it easier to *set* HEAD to an unborn branch that
>> d/f conflicts with an existing reference? If so, that might be a
>> slightly worse UI for users. I'd rather learn about such a problem when
>> setting HEAD (when I am thinking about the new branch name and am in the
>> frame of mind to solve the problem) rather than later, when I try to
>> commit to the new branch.
> 
> Good question. The answer is no, it's allowed both before and after my
> patch. At least via git-symbolic-ref.
> 
> I agree it would be nice to know earlier for such a case. For
> symbolic-ref, we probably should allow it, because it's plumbing that
> may be used for tricky things. For things like "checkout -b", you'd
> generally get a timely warning as we try to create the ref.
> 
> The odd man out is "checkout --orphan", which leaves the branch unborn.
> It might be nice if it did a manual check that the ref is available (and
> also that it's syntactically acceptable, though I think we may do that
> already).
> 
> But all of that is orthogonal to this fix, I think.

Thanks for checking. Yes, I totally agree that this is orthogonal.

Michael

Re: [PATCH 2/2] refs_resolve_ref_unsafe: handle d/f conflicts for writes

[Michael Haggerty]

On 10/07/2017 06:36 AM, Michael Haggerty wrote:
> On 10/06/2017 07:16 PM, Jeff King wrote:
>> On Fri, Oct 06, 2017 at 07:09:10PM +0200, Michael Haggerty wrote:
>>
>>> I do have one twinge of uneasiness at a deeper level, that I haven't had
>>> time to check...
>>>
>>> Does this patch make it easier to *set* HEAD to an unborn branch that
>>> d/f conflicts with an existing reference? If so, that might be a
>>> slightly worse UI for users. I'd rather learn about such a problem when
>>> setting HEAD (when I am thinking about the new branch name and am in the
>>> frame of mind to solve the problem) rather than later, when I try to
>>> commit to the new branch.
>>
>> Good question. The answer is no, it's allowed both before and after my
>> patch. At least via git-symbolic-ref.
>>
>> I agree it would be nice to know earlier for such a case. For
>> symbolic-ref, we probably should allow it, because it's plumbing that
>> may be used for tricky things. For things like "checkout -b", you'd
>> generally get a timely warning as we try to create the ref.
>>
>> The odd man out is "checkout --orphan", which leaves the branch unborn.
>> It might be nice if it did a manual check that the ref is available (and
>> also that it's syntactically acceptable, though I think we may do that
>> already).
>>
>> But all of that is orthogonal to this fix, I think.
> 
> Thanks for checking. Yes, I totally agree that this is orthogonal.

I also just checked but there don't seem to be any docstrings that need
updating.

Reviewed-by: Michael Haggerty <mhagger@alum.mit.edu>

(both patches in this series).

Michael