Creating remote branch called HEAD corrupts remote clones

Hi,

On Friday we had an issue where a developer pushed a branch called HEAD to the remote server. The result was that other developers could not pull or push. I have not been able to reproduce the exact issue locally, but this script shows that the bob clone behaves oddly on each pull. That is a symptom we saw on Friday. However, bob is still able to push, which we were not able to. That point could be something to do with how the kde git infrastructure is configured.

mkdir remote cd remote/ git init --bare cd ../ git clone remote/ alice cd alice/ echo test >> file git add file git commit -am w git push origin master echo test >> file git commit -am w git branch HEAD git push origin HEAD git push cd .. git clone remote bob cd bob/ git pull --rebase echo test >> file git commit -am w git push git pull git pull git pull

There were also messages like this:

$ git pull
remote: Counting objects: 5, done.
remote: Total 3 (delta 0), reused 0 (delta 0)
Unpacking objects: 100% (3/3), done.
From /home/kde-devel/dev/src/playground/git/tmp/remote
 + 1434cd2...dd30974 HEAD       -> origin/HEAD  (forced update)
error: Ref refs/remotes/origin/master is at 
dd3097498a6c1c5bc73ad1f2ff3b7969a6f6d059 but expected 
1434cd2bb9823d2d2b1548c75fdd4ff8b1feddc1
 ! 1434cd2..2fb560d  master     -> origin/master  (unable to update local 
ref)

The HEAD branch was created accidentally and the issue was resolved by doing a git push origin -f :refs/heads/HEAD. Again though, git push -f is not something all developers are allowed to do on the kde git infrastructure, so until that was done, the repo was corrupt for everyone.

Shouldn't git forbit the creation of a branch called HEAD? Hopefully the provided script can lead to the actual issue that caused the corruption of our repo.

Thanks,

Steve.

_______________________________________________ KDE PIM mailing list kde-pim@kde.org https://mail.kde.org/mailman/listinfo/kde-pim KDE PIM home page at http://pim.kde.org/

Stephen Kelly wrote:

Does anyone have any thoughts/response on this?

Why does git not have a bug tracker?

Steve.

Stephen Kelly wrote:

http://thread.gmane.org/gmane.comp.version-control.git/136500

Ok so there was some movement with the result that no one uses what was set up.

Presumably because git developers don't want a bug tracker.

So how can I ensure that this particular issue doesn't get lost? Is there no way except hope that people get involved in fixing it straight away, and fix it straight away before they forget about it?

We worked around this on the KDE side by forbidding pushing any ref with the name HEAD to the remote, but it's still a git bug.

On 1/20/11, Thomas Rast <trast@student.ethz.ch> wrote:

On Thu, Jan 20, 2011 at 4:05 PM, Stephen Kelly <steveire@gmail.com> wrote:

You could always fix it yourself, and submit a patch.

On Thu, Jan 20, 2011 at 4:41 PM, Erik Faye-Lund <kusmabite@gmail.com> wrote:

Correct. That would be a big rampup though and I might give up, move on or forget and then no one would do it because everyone has forgotten about it. I mean that in the general sense of any bug that gets posted to this mailing list.

But I'm not trying to change how git people work. I'm just trying to discover what the states of a bug in git should be understood to be after it is emailed to this list and before it's in master. So far it seems there is only one intermediate state: "Limbo, maybe forgotten. You should email the list again"

Thanks,

Steve.

Hi,

On Thu, Jan 20, 2011 at 1:14 PM, Stephen Kelly <steveire@gmail.com> wrote:

Can you list a series of steps to reproduce this?

Because it's not needed. If you have an issue, post the issue as you
would file a bug:
 Which version of git?
 Which kind of network transport was used?
 Is this reproducible?

Chances are, if this is reproducible in the latest version, someone would fix it soon enough. If not, and it's important to you, you would ping back. If other people find this issue, they would send another email.

In fact, if you really want to help, you could clone the latest 'master' to see if this still happening, narrow down the steps needed to reproduce this, write a test to trigger it and send a patch. Certainly, a test case that constantly fails would be a constant remainder that there is a bug.

Cheers.

On Thursday, January 20, 2011 10:32:40 Felipe Contreras wrote:

[...]

FWIW, here is a quick demonstration of at least one problem with having a branch called HEAD. You can make it and push it fine, but when cloning, you don't get it.

#!/bin/bash git init --bare origin.git git clone origin.git wc1 cd wc1 git commit --allow-empty -m "Initial rev" git checkout -b HEAD git commit --allow-empty -m "Make HEAD branch" git push --all cd .. git clone origin.git wc2 diff -u <(cd wc1; git branch -a) <(cd wc2; git branch -a) diff -u <(cd wc1; git log --all) <(cd wc2; git log --all)

If I do the following from wc2 I can get the branch manually:

git pull origin refs/heads/HEAD:HEAD

I haven't played with it enough to see what other problems might arise.

Stephen Kelly <steveire@gmail.com> writes:

Stephen, thanks, I think you have found a bug, not in this step but one step before this 'git pull', where Bob pushes to the remote for the first time after making a commit.

This issue is inherent in the way how the 'separate remotes' layout introduced in 1.6.0 arranges the remote tracking mappings.

The refs/remotes/origin/HEAD in Bob's repository is supposed to be a symbolic ref that points at the primary branch of the 'origin' remote (typically its master), e.g. "ref: refs/remotes/origin/master". But in general, local 'refs/remotes/origin/X' for any value of X is to copy 'refs/heads/X' from the 'origin'.

Oops. If the origin repository has 'refs/heads/HEAD', these rules obviously conflict with each other.

In this particular case, Bob pushes the change to his refs/heads/master to the remote to update its refs/heads/master. The push at the same time tries to pretend that it fetched from the remote to update Bob's tracking branches, so refs/remotes/origin/master in Bob's repository is also updated to point at this commit.

However, because Bob's refs/remotes/origin/HEAD is a symbolic ref that points at his refs/remotes/origin/master, its value is also updated by this push.

Bob's next fetch from remote will then notice that remotes/origin/HEAD he has is different from refs/heads/HEAD the remote has, and tries to update it, which would obviously a non-fast-forward.

I personally think it is reasonable to forbid HEAD or anything all caps that ends with "_HEAD" as branch names. Opinions?

On Thu, Jan 20, 2011 at 11:53:16AM -0800, Junio C Hamano wrote:

Hmm. It seems like the symbolic ref is the culprit, not just HEAD. The HEAD thing is the most likely, of course, but I could do something like:

  git symbolic-ref refs/remotes/origin/convenient-alias \
                   refs/remotes/origin/some-name-you-dont-like

which is basically the same as the HEAD case (except that the "convenient alias" for HEAD is "origin" and not "origin/convenient-alias" due to the lookup table in dwim_ref).

Now imagine the remote creates a branch called convenient-alias. When I fetch, am I corrupting my local tracking branches by falsely equating the two? And/or when I push, am I then corrupting the remote?

So I wonder if the safety valve here should be about symbolic refs, and not about the special name HEAD. Maybe we should not follow symbolic refs during fetch. So if we are fetching the refspec "foo:bar", and the RHS "bar" is a symref, we should _not_ follow it, but instead just overwrite the symref with a regular ref.

For pushing, one rule could be to allow pushing from a named symref, but not allow the matching rules to use a symref as a source. So I could do:

  git push origin convenient-alias:new-name

but

  git push origin

would never overwrite upstream's convenient-alias.

I dunno. That's just off the top of my head, so maybe I'm missing some corner cases. I would be tempted to put the push rule into receive-pack, so it could look at the local refs, but I don't think receive-pack has any way of knowing what is a symref and what is not on the pushing end.

-Peff

Jeff King <peff@peff.net> writes:

Isn't it already wrong to do the above locally, in the sense that it is equally wrong to do this?

    git update-ref refs/remotes/origin/no-such-thing-exists-over-there 
    	refs/heads/master

I agree that the symbolic ref is the real source of confusion in Stephen's case (I admit that I was scratching my head chasing a non-existent bug in transport_update_tracking_ref() before I realized what was happening), but the usual (and the only) way for refs/remotes hierarchy to get a symbolic ref is via a normal clone, and HEAD is the only name that can cause this conflict. Creating a branch called HEAD (or FETCH_HEAD for that matter) is infinitely more likely to be a mistake than being clever, I think.

I personally like this line of thought, especially as a thought experiment to see what corner cases we could find, but I doubt I will be able to say we covered all the corner cases with confidence without thinking long and very hard. For now, I do not find this issue worth spending that kind of deep thinking, especially when a lot simpler and easier to explain workaround is available, but others may disagree and perfect your idea.

On Thu, Jan 20, 2011 at 01:43:17PM -0800, Junio C Hamano wrote:

Probably. My argument in favor would be "that is what we are doing with remotes/*/HEAD", but I think the logic is somewhat circular there.

Thinking on it more, yeah, if you want "convenient-alias", you are probably better off to do:

  git symbolic-ref refs/convenient-alias refs/remotes/origin/whatever

Actually, don't we end up with that in the case of upstream deleting a ref? Which isn't to say it isn't a stupid thing to do, but that we can and do get into that state.

Thinking on it even more, I don't think we can cover all of the weird "you have a ref named $foo and they suddenly created a ref named $foo" push corner cases. There is no substitution for actual communication and organization of ref names if you are going to be pushing along with other people into a central repo. Because fundamentally the ref name is the unique identifier, and matching refs during push tries to reconcile matches based on those identifiers.

Yeah, after reading your response and considering a bit, I think the simple "don't make HEAD" thing (or at least "don't pull or push HEAD") is a sane workaround.

-Peff

Hi,

On Thu, Jan 20, 2011 at 11:54 PM, Jeff King <peff@peff.net> wrote:

I don't fully understand the issue, so excuse me if this is totally wrong, but wouldn't a rule like 'you can't create a branch for which there's already a symbolic ref' do the trick?

Felipe Contreras <felipe.contreras@gmail.com> writes:

But whose symbolic ref are you checking against? Your own, or ones in somebody else's repository that you haven't recently updated from?

On Fri, Jan 21, 2011 at 7:37 PM, Junio C Hamano <gitster@pobox.com> wrote:

The local ones. That means that somebody can't create a 'HEAD' branch locally, and can't push a 'HEAD' branch either, as the remote server would already have a 'HEAD' symbolic link. And actually, if for some reason I have a FOO_HEAD, and I fetch a branch called bob/FOO_HEAD, obviously the local symbolic ref without namespace should take precedence.

bump.

I don't think this issue was fixed, was it?

(no need to put kdepim back in the cc list)

On Sat, Jan 22, 2011 at 1:46 PM, Felipe Contreras <felipe.contreras@gmail.com> wrote:

Can git have a bug tracker please?

This is another reminder to fix this bug which is otherwise untrackable.

Thanks,

Steve.

On Sun, Feb 20, 2011 at 2:17 PM, Stephen Kelly <steveire@gmail.com> wrote:

On Tue, Apr 26, 2011 at 3:09 PM, Stephen Kelly <steveire@gmail.com> wrote:

So that you would feel comfortable that there would be a bug report gathering dust? Or that it's closed as invalid for lack of information?

Let's imagine you are posting this to bugzilla: first question? How do you reproduce this?

But I already asked you this[1], and you didn't reply. What should one assume but that you don't care enough to help get this fixed.

[1] http://article.gmane.org/gmane.comp.version-control.git/165320

On Tue, Apr 26, 2011 at 8:18 PM, Felipe Contreras <felipe.contreras@gmail.com> wrote:

If you believe that it is a foregone conclusion that that is the fate of all bug trackers, and that that's a reasonable reason for git not to have one, then you have had very different experiences to me.

I don't think there's more I can say than that.

My initial mail illustrated the problem as best I could:

http://thread.gmane.org/gmane.comp.kde.devel.pim/29534

Someone else replied. Isn't that enough?

Other git developers confirmed it's probably an issue. Isn't that enough?

http://thread.gmane.org/gmane.comp.kde.devel.pim/29534/focus=165326

Anyway, we've had a work around in place since January. From the git POV, this just falls through the cracks. Consider the bug marked as can not reproduce/needs info/whatever you prefer. I'm outie.

Steve.

On Wed, Apr 27, 2011 at 12:18 PM, Stephen Kelly <steveire@gmail.com> wrote:

No problems here:

Initialized empty Git repository in /tmp/remote/
Cloning into alice...
done.
warning: You appear to have cloned an empty repository.
[master (root-commit) 6983153] w
 1 files changed, 1 insertions(+), 0 deletions(-)
 create mode 100644 file
Counting objects: 3, done.
Writing objects: 100% (3/3), 213 bytes, done.
Total 3 (delta 0), reused 0 (delta 0)
Unpacking objects: 100% (3/3), done.
To /tmp/remote/
 * [new branch]      master -> master
[master 116a225] w
 1 files changed, 1 insertions(+), 0 deletions(-)
Counting objects: 5, done.
Writing objects: 100% (3/3), 242 bytes, done.
Total 3 (delta 0), reused 0 (delta 0)
Unpacking objects: 100% (3/3), done.
To /tmp/remote/
 * [new branch]      HEAD -> HEAD
Everything up-to-date
Cloning into bob...
done.
From /tmp/remote
   6983153..116a225  HEAD       -> origin/HEAD
Current branch master is up to date.
[master 0951422] w
 1 files changed, 1 insertions(+), 0 deletions(-)
Counting objects: 5, done.
Writing objects: 100% (3/3), 242 bytes, done.
Total 3 (delta 0), reused 0 (delta 0)
Unpacking objects: 100% (3/3), done.
To /tmp/remote
   6983153..0951422  HEAD -> master
From /tmp/remote
 + 0951422...116a225 HEAD       -> origin/HEAD  (forced update)
Already up-to-date.
From /tmp/remote
 + 116a225...0951422 master     -> origin/master  (forced update)
Already up-to-date.
From /tmp/remote
 + 0951422...116a225 HEAD       -> origin/HEAD  (forced update)
Already up-to-date.

With a different issue that's not really important.

_probably_, many things have happened since then.

Is it still an issue? Doesn't seem so.

On Wed, Apr 27, 2011 at 11:48 AM, Felipe Contreras <felipe.contreras@gmail.com> wrote:

I had another go.

mkdir remote cd remote/ git init --bare cd ../ git clone remote/ alice cd alice/ echo test >> file git add file git commit -am w git push origin master echo test >> file git commit -am w git branch HEAD git push origin HEAD git push cd .. git clone remote bob cd bob/ git branch git pull --rebase echo test >> file git commit -am w git push git pull git pull git pull echo test >> file git commit -am w git push cd ../alice git branch git status echo test >> file git commit -am w git push echo test2 >> file git commit -am w git push git pull echo test3 >> file git commit -am w git status git push gitk

stephen@bishop:/tmp/git$ mkdir remote

stephen@bishop:/tmp/git$ cd remote/

stephen@bishop:/tmp/git/remote$ git init --bare Initialized empty Git repository in /tmp/git/remote/

stephen@bishop:/tmp/git/remote$ cd ../

stephen@bishop:/tmp/git$ git clone remote/ alice Cloning into alice... done. warning: You appear to have cloned an empty repository.

stephen@bishop:/tmp/git$ cd alice/

stephen@bishop:/tmp/git/alice$ echo test >> file

stephen@bishop:/tmp/git/alice$ git add file

stephen@bishop:/tmp/git/alice$ git commit -am w
[master (root-commit) 072df32] w
 1 files changed, 1 insertions(+), 0 deletions(-)
 create mode 100644 file

stephen@bishop:/tmp/git/alice{master}$ git push origin master
Counting objects: 3, done.
Writing objects: 100% (3/3), 210 bytes, done.
Total 3 (delta 0), reused 0 (delta 0)
Unpacking objects: 100% (3/3), done.
To /tmp/git/remote/
 * [new branch]      master -> master

stephen@bishop:/tmp/git/alice{master}$ echo test >> file

stephen@bishop:/tmp/git/alice{master}$ git commit -am w
[master b39d099] w
 1 files changed, 1 insertions(+), 0 deletions(-)

stephen@bishop:/tmp/git/alice{master}$ git branch HEAD

stephen@bishop:/tmp/git/alice{master}$ git push origin HEAD
Counting objects: 5, done.
Writing objects: 100% (3/3), 242 bytes, done.
Total 3 (delta 0), reused 0 (delta 0)
Unpacking objects: 100% (3/3), done.
To /tmp/git/remote/
 * [new branch]      HEAD -> HEAD

stephen@bishop:/tmp/git/alice{master}$ git push Everything up-to-date

stephen@bishop:/tmp/git/alice{master}$ cd ..

stephen@bishop:/tmp/git$ git clone remote bob Cloning into bob... done.

stephen@bishop:/tmp/git$ cd bob/

stephen@bishop:/tmp/git/bob{master}$ git pull --rebase
From /tmp/git/remote
   072df32..b39d099  HEAD       -> origin/HEAD
Current branch master is up to date.

stephen@bishop:/tmp/git/bob{master}$ echo test >> file

stephen@bishop:/tmp/git/bob{master}$ git commit -am w
[master b39d099] w
 1 files changed, 1 insertions(+), 0 deletions(-)

stephen@bishop:/tmp/git/bob{master}$ git push
Total 0 (delta 0), reused 0 (delta 0)
To /tmp/git/remote
   072df32..b39d099  HEAD -> master

stephen@bishop:/tmp/git/bob{master}$ git pull Current branch master is up to date.

stephen@bishop:/tmp/git/bob{master}$ git pull Current branch master is up to date.

stephen@bishop:/tmp/git/bob{master}$ git pull Current branch master is up to date.

stephen@bishop:/tmp/git/bob{master}$ echo test >> file

stephen@bishop:/tmp/git/bob{master}$ git commit -am w
[master 47699a9] w
 1 files changed, 1 insertions(+), 0 deletions(-)

stephen@bishop:/tmp/git/bob{master}$ git push
Counting objects: 5, done.
Writing objects: 100% (3/3), 240 bytes, done.
Total 3 (delta 0), reused 0 (delta 0)
Unpacking objects: 100% (3/3), done.
To /tmp/git/remote
   b39d099..47699a9  HEAD -> master

stephen@bishop:/tmp/git/bob{master}$ cd ../alice

stephen@bishop:/tmp/git/alice{master}$ git branch
  HEAD
* master

stephen@bishop:/tmp/git/alice{master}$ git status warning: refname 'HEAD' is ambiguous. warning: refname 'HEAD' is ambiguous. # On branch master # Your branch is ahead of 'origin/master' by 1 commit. # nothing to commit (working directory clean)

stephen@bishop:/tmp/git/alice{master}$ echo test >> file

stephen@bishop:/tmp/git/alice{master}$ git commit -am w
warning: refname 'HEAD' is ambiguous.
warning: refname 'HEAD' is ambiguous.
warning: refname 'HEAD' is ambiguous.
[master 7e83bed] w
 1 files changed, 1 insertions(+), 0 deletions(-)

stephen@bishop:/tmp/git/alice{master}$ git push Everything up-to-date

stephen@bishop:/tmp/git/alice{master}$ echo test2 >> file

stephen@bishop:/tmp/git/alice{master}$ git commit -am w
warning: refname 'HEAD' is ambiguous.
warning: refname 'HEAD' is ambiguous.
warning: refname 'HEAD' is ambiguous.
[master b4f5b5b] w
 1 files changed, 1 insertions(+), 0 deletions(-)

stephen@bishop:/tmp/git/alice{master}$ git push Everything up-to-date

stephen@bishop:/tmp/git/alice{master}$ git pull
warning: refname 'HEAD' is ambiguous.
warning: refname 'HEAD' is ambiguous.
warning: refname 'HEAD' is ambiguous.
warning: refname 'HEAD' is ambiguous.
remote: Counting objects: 5, done.
remote: Total 3 (delta 0), reused 0 (delta 0)
Unpacking objects: 100% (3/3), done.
From /tmp/git/remote
   072df32..47699a9  master     -> origin/master
warning: refname 'HEAD' is ambiguous.
warning: refname 'HEAD' is ambiguous.
warning: refname 'HEAD' is ambiguous.
First, rewinding head to replay your work on top of it...
warning: refname 'HEAD' is ambiguous.
warning: refname 'HEAD' is ambiguous.
Applying: w
warning: refname 'HEAD' is ambiguous.
warning: refname 'HEAD' is ambiguous.

stephen@bishop:/tmp/git/alice{master}$ echo test3 >> file

stephen@bishop:/tmp/git/alice{master}$ git commit -am w
warning: refname 'HEAD' is ambiguous.
warning: refname 'HEAD' is ambiguous.
warning: refname 'HEAD' is ambiguous.
[master cc2088e] w
 1 files changed, 1 insertions(+), 0 deletions(-)

stephen@bishop:/tmp/git/alice{master}$ git status warning: refname 'HEAD' is ambiguous. warning: refname 'HEAD' is ambiguous. # On branch master # Your branch is ahead of 'origin/master' by 2 commits. # nothing to commit (working directory clean)

stephen@bishop:/tmp/git/alice{master}$ git push Everything up-to-date

stephen@bishop:/tmp/git/alice{master}$

stephen@bishop:/tmp/git/alice{master}$ git --version git version 1.7.4.1

On Wed, Apr 27, 2011 at 2:29 PM, Stephen Kelly <steveire@gmail.com> wrote:

And is that the expected behavior or not? BTW. I used 1.7.5.

It is not expected.

Alices repo is fubar'd. gitk doesn't work. The info about master being ahead of remote etc is wrong or git push tells me it worked, though it doesn't seem to.

stephen@bishop:/tmp/git/alice{master}$ git status warning: refname 'HEAD' is ambiguous. warning: refname 'HEAD' is ambiguous. # On branch master # Your branch is ahead of 'origin/master' by 2 commits. # nothing to commit (working directory clean)

stephen@bishop:/tmp/git/alice{master}$ git push Everything up-to-date

stephen@bishop:/tmp/git/alice{master}$ git status warning: refname 'HEAD' is ambiguous. warning: refname 'HEAD' is ambiguous. # On branch master # Your branch is ahead of 'origin/master' by 2 commits. # nothing to commit (working directory clean)

On Wed, Apr 27, 2011 at 1:32 PM, Felipe Contreras <felipe.contreras@gmail.com> wrote:

On Wed, Apr 27, 2011 at 2:37 PM, Stephen Kelly <steveire@gmail.com> wrote:

gitk --all works fine, and gitk show a precise warning explaining the problem.

Also, the 'git push' worked fine. Perhaps what you didn't expect is that when push.default=current, instead of pushing the current branch, the 'HEAD' branch is being pushed.

So the test can be simplified to:

mkdir remote cd remote/ git init --bare cd ../ git clone remote/ alice cd alice/ echo test >> file git add file git commit -am w git push origin master echo test >> file git commit -am w git branch HEAD git push origin HEAD git -c push.default=current push git diff master origin/master

And the diff should be empty. With that in mind, it should be easy to create a test script that does something similar, and add it to the suite.

On Wed, Apr 27, 2011 at 1:29 PM, Stephen Kelly <steveire@gmail.com> wrote:

I'll stop you here. You reproduce the issue a lot simpler:

git init foo && cd foo && echo "foo" > bar && git add bar && git commit -m. && git branch HEAD && gitk

No need to involve remote branches. While remote branches makes the issue worse, because you can get in a situation where gitk doesn't when someone else made a nasty branch, and you fetched it.

The real problem is that "git rev-parse HEAD" outputs "warning: refname 'HEAD' is ambiguous." to stderr (even if stderr is a non-tty), and gitk does not like that.

This can be fixed by either doing "git -c core.warnambiguousrefs=0 rev-parse HEAD", which strikes me as ugly, or by making sure that we don't issue this warning when not attached to a tty:

On Wed, Apr 27, 2011 at 2:21 PM, Erik Faye-Lund <kusmabite@gmail.com> wrote:

Of course, a third (and probably even better) option is to make gitk warn about the ambiguous refname (like other commands will), but not treat it as a fatal problem. But I'm not motivated enough to give that solution a stab myself.

Not outputting that warning might be a regression for other users of rev-parse (and/or the underlying mechanics).

On Wed, Apr 27, 2011 at 2:49 PM, Erik Faye-Lund <kusmabite@gmail.com> wrote:

Ok, if you can't see in the code why a branch called HEAD might corrupt the remote and I can't demonstrate it with a testcase, maybe it's not an issue anymore, I don't know.

Hopefully the relevant people saw the side issues brought up such as this ambiguous ref issue. After all, there's no other way to track those issues.

Thanks for the investigation and help,

Steve.

On Mon, May 2, 2011 at 9:26 PM, Stephen Kelly <steveire@gmail.com> wrote:

No, it's still an issue, and I believe I pin-pointed it in my first mail. You can try out the patch I sent, and see if that helps in your case. If it does, I think it'd make sense to do something (preferably a bit more robust) with it.

On Mon, May 2, 2011 at 10:43 PM, Erik Faye-Lund <kusmabite@gmail.com> wrote:

Yes, I think your patch should be applied regardless, as that solves _one_ issue.

But there are other issues.

On Tue, May 3, 2011 at 7:54 PM, Felipe Contreras <felipe.contreras@gmail.com> wrote:

I don't have a build of git at the moment to test it as I'm using distro packages again. The only test case I have is the alice and bob stuff already posted, so if your patch fixes that for you that's good enough from my POV.

All the best,

Steve.

On Tue, May 3, 2011 at 9:08 PM, Stephen Kelly <steveire@gmail.com> wrote:

As I said, 'gitk --all' works fine, the patch would fix 'gitk'.

On Tue, May 3, 2011 at 7:54 PM, Felipe Contreras <felipe.contreras@gmail.com> wrote:

OK, I'll send out an RFC with some discussion on the alternatives a bit later.

I guess the root of the problem(s) is that there's no way to disambiguate 'HEAD'. One solution could be to say that 'HEAD' never is ambiguous, but it feels a little inconsistent... Thoughts, anyone?

If there's a branch (either local or remote) called 'HEAD' commands that take a ref currently emits a warning, no matter if the output is going to a TTY or not.

Fix this by making sure we only output this warning when stderr is a TTY. Other git commands or scripts should not care about this ambiguity.

This fix prevents gitk from barfing when given no arguments and there's a branch called 'HEAD'.

Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>

---

In 2f8acdb ('core.warnambiguousrefs: warns when "name" is used and both "name" branch and tag exists.'), a check for collisions of refs was introduced. It does not seem to me like the intention was to check HEAD for ambiguty (because the commit talks about branches and tags), but it does.

Because HEAD cannot be disambiguated like branches and tags can, this can lead to an annoying warning, or even an error in the case of gitk.

A branch called HEAD can be 'injected' into another user's repo through remotes, and this can cause annoyance (and in the case of gitk, brokenness) just by pulling the wrong remote. Yuck.

The particular problem of gitk can be fixed by making gitk able to parse the warning, and probably forwarding it to the user. This strikes me as The Right Thing To Do(tm), but is outside of my gitk and TCL/TK skills.

Alternatively, gitk could state that it doesn't care about ambiguous refs, by calling 'git -c core.warnambiguousrefs=0 show-ref <ref>'.

One question is if ANY warnings should be output to stderr if it's not a TTY. My guess is that there probably are some classes of warnings that should, but the vast majority should probably not.

Perhaps it's better to make warning() filter the output if stderr is not a tty instead, and make the places that needs to warn just do fprintf(stderr, ...) instead? That's one huge hammer, though.

Another question is if we should come up with a way of disambiguating HEAD. Perhaps having something like 'refs/HEAD' will do?

So, to recap: The way I see it, these are our options:

 1) Discard this specific warning when stderr isn't a TTY (i.e
    what this patch does)
 2) Discard all warnings when stderr isn't a TTY
 3) Make gitk understand and forward warnings to the user
 4) Have gitk explicitly ignore ambiuous refs
 5) Come up with a way to disambiguate HEAD, and use that instead
    by default
 6) Force HEAD to never be ambiguous
 7) Leave things as they are

I think 3) + 5) might be the most sane solution. That way we inform the user that there's an ambiguity if he or she runs 'gitk HEAD' (so he or she has a chance the chance to correct it), but the correct HEAD is chosen (without any annoying warnings) if the user didn't specify a ref.

This combination also relies on us NOT doing 1), 2) or 4); i.e the warning must still be output to reach the user.

Thoughs?

 sha1_name.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

On Mon, May 09, 2011 at 09:51:18AM +0200, Erik Faye-Lund wrote:

This feels wrong. Gitk should not care about messages on stderr, for exactly the reason that they may be harmless warnings (if anything, it should show them to the user in a dialog).

My understanding is that this is a tcl thing, but I just think it's insane.

This is a separate issue, isn't it? Gitk should probably handle ambiguous ref warnings better, no matter what the name. And if ambiguous HEAD warnings are considered too annoying, they should be squelched for everyone. I don't personally have an opinion on the latter, though.

Can you give an example? If I am fetching your refs into refs/remotes/$remote/*, how does that create an ambiguity?

Agreed. And also outside my tcl skills. :)

I disagree. If I do:

  git foo 2>errors

I would certainly expect any relevant errors to end up in that file. As for why I would do that, two cases I can think of offhand are:

  1. Test scripts, which use this extensively.

  2. Sometimes cron jobs will capture chatty output in a file and show
     it only in the case of some error condition.

Yeah, if we disambiguate, I would be tempted to say that "HEAD" always unambiguously refers to "HEAD". And "refs/HEAD" should already work, no?

Agreed.

I think I have made it clear that I am not in favor of this approach, but if we were to do it, it is too late to be calling isatty(2) here. You need to also check pager_in_use(), as we may have redirected stderr into the pager's pipe.

-Peff

On Mon, May 9, 2011 at 10:03 AM, Jeff King <peff@peff.net> wrote:

I agree.

I agree; it's possible to squelch them already with the core.warnambiguousrefs config, so people who want to live with branched called 'HEAD' can already get around it.

Actually, this is just something I read out of Stephen's report and was too lazy to double check. It's not possible to do, because refs/remotes/* does not seem to be checked for ambiguity. Thanks for setting me straight :)

I was talking about warnings, not errors. But I can also see that one would sometimes want warnings even when not connected to a tty, but perhaps only when -v is specified?

While that would touch less code, my gut tells me it's a bit more fragile. But perhaps you're right; I can't come up with any real arguments (i.e use cases that I care about) on top of my head.

No:
$ git init foo
$ cd foo/
$ echo "foo" > bar
$ git add bar
$ git commit -m.
[master (root-commit) fc0cbef] .
warning: LF will be replaced by CRLF in bar.
The file will have its original line endings in your working directory.
 1 files changed, 1 insertions(+), 0 deletions(-)
 create mode 100644 bar
$ git show refs/HEAD
fatal: ambiguous argument 'refs/HEAD': unknown revision or path not in
the working tree.
Use '--' to separate paths from revisions

Good point. I doubt I'll update the patch in this direction though, since I agree it's not the right approach.

On Mon, May 09, 2011 at 10:41:02AM +0200, Erik Faye-Lund wrote:

I know. I meant a script like this:

  cat >>foo.sh <<'EOF'
  # go to branch in question
  git checkout "$1"

  # note some point of interest
  sha1=`git rev-parse "$2"`

  # do some script-specific inspection of $sha1, and
  # merge if it looks OK
  if test -z "$(git log ..$sha1 -- some-path)"; then
    git merge $sha1 || exit 1
  fi
  EOF

It may produce some chatty output (like "switched to branch..."). So I redirect it to a file, and if everything is successful, that output is uninteresting. But if it fails, then I want to see everything. So I do something like:

  if ! foo.sh master topic >output.tmp 2>&1; then
    cat output.tmp
    exit 1
  fi

If the merge fails, it will produce an error message. But I _also_ want to see any warnings that were generated by it and earlier commands, like rev-parse (e.g., an ambiguous ref warning might help us understand why the merge failed).

Obviously this is a pretty trivial example that I cooked up for this email. But the concept of stash-stderr-and-report-on-error is a pretty common pattern for cron jobs.

Honestly, I'm kind of surprised it's not that way already. It would make sense to me that "upper" levels would take precedence over lower levels, but that ambiguity would occur within a level. So if I say "foo", we would look for:

  1. $GIT_DIR/foo, with no ambiguity

  2. $GIT_DIR/refs/foo, with no ambiguity

  3. $GIT_DIR/refs/tags/foo
     $GIT_DIR/refs/heads/foo
     $GIT_DIR/refs/remotes/foo

     And note any ambiguity between those three.

Which is not very different than what we do today, except that things like HEAD and FETCH_HEAD would always be unambiguously about the top-level.

Of course, because there is no refs/HEAD at all. I meant "if you have ambiguity between $GIT_DIR/HEAD and $GIT_DIR/refs/HEAD", then saying "refs/HEAD" should disambiguate already. In your example, there is no ambiguity.

What I failed to notice is that the likely disambiguator is actually "refs/heads/HEAD" if you erroneously made a branch.

Try this:

  # A repo with two commits
  git init repo && cd repo &&
  echo content >file &&
  git add file &&
  git commit -m one &&
  echo content >>file &&
  git commit -a -m two &&

  # And an ambiguously named ref called HEAD, pointing to "one";
  # our real HEAD is still pointing to "two"
  git branch HEAD HEAD^ &&

  # This should warn of ambiguity, but show "two"
  git log -1 --oneline HEAD

  # And this should not be ambiguous at all, and show "one"
  git log -1 --oneline refs/heads/HEAD

  # You can even do the same thing with refs/HEAD if you want, but
  # you have to use plumbing to get such a ref.
  git branch -d HEAD
  git update-ref refs/HEAD HEAD^

  # same as before, ambiguous "two"
  git log -1 --oneline HEAD

  # or we can use refs/HEAD to get "one"
  git log -1 --oneline refs/HEAD

So most of that makes sense to me. We choose $GIT_DIR/HEAD over other options, and you can specifically refer to something further down by its fully-qualified name.

The only thing that I think we might want to change is that "HEAD" is considered ambiguous with "refs/heads/HEAD". On the other hand, it seems a little insane to name your branch that, given that it has a well-established meaning in git. I admit I haven't been following this thread too closely. What is the reason not to tell the user "sorry, that is an insane branch name. Accept the ambiguity warning, or choose a different name"?

-Peff

On Mon, May 9, 2011 at 12:32 PM, Jeff King <peff@peff.net> wrote:

Yeah, I understood that part. My point was that once the output is wanted for diagnostics, you probably also want verbose output. And warnings should probably always be output if we're verbose.

But I have no strong feelings about this, so it's probably better to leave it alone.

I think that would make sense.

I meant that "refs/HEAD" could be an non-ambiguous alias for HEAD, but it's probably easier to just say that 'HEAD' isn't ambiguous. Your suggestion of only checking for ambiguousness on the same level is IMO an elegant way of doing this.

I agree. There could be a remote chance that you can get a branch called 'HEAD' from some foreign vcs or something, though. But I don't think it's very likely, and the problem will also go away if we go with your approach mentioned above.

I think having the ambiguity warning in itself isn't the problem, it's gitk not swallowing it that is.

The reporter also had some problems pushing with a branch named 'HEAD' in his repo, but I didn't look into that part at all.

On Mon, May 09, 2011 at 02:37:48PM +0200, Erik Faye-Lund wrote:

Ah, I see. I think my main concern is that the behavior you proposed would simply be surprising to people used to normal unix conventions. But it sounds like we both agree that isn't the right direction anyway.

OK, I see what you meant. But "refs/HEAD" cannot be a shortcut for "HEAD", as it means something totally different. You can have "HEAD", "refs/HEAD", "refs/heads/HEAD" all co-existing.

Thinking on it more, I think warning is probably the only sane thing to do there. Having a branch with that name is just going to be confusing in the long run, and the sooner we start making the user aware of the situation, the better.

Agreed.

I expect that would be a separate issue entirely (if it were fetching, I wouldn't be surprised if it was the "fake" refs/remotes/*/HEAD symref we create getting in the way).

-Peff

Jeff King <peff@peff.net> writes:

I agree with both of the above. It seems that the only thing we would need is to do (3) and nothing else in Erik's original list?

On Mon, May 09, 2011 at 09:33:07AM -0700, Junio C Hamano wrote:

Yeah, I think so. Now we just need a volunteer who wants to write some tcl. :)

-Peff