Re: Regression in 'git branch -m'?
Jeff King 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: Regression in 'git branch -m'?
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 [...] -- [...]' > > 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
Re: Regression in 'git branch -m'?
On Fri, Oct 06, 2017 at 06:45:08PM +0900, Junio C Hamano wrote:
> Jeff King 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 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'?
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'?
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'?
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 [...] -- [...]' > > 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
Regression in 'git branch -m'?
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 [...] -- [...]' 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 Date: Fri, 22 Jan 2010 07:29:21 -0800
