[topgit] tg update error

[topgit] tg update error

[Aneesh Kumar]

doing a tg update with latest git gives the below error

[extent_validate@linux-2.6]$ tg update
fatal: Refusing to point HEAD outside of refs/heads/
[extent_validate@linux-2.6]$

-aneesh

Re: [topgit] tg update error

[martin f krafft]

[-- Attachment #1: Type: text/plain, Size: 918 bytes --]

also sprach Aneesh Kumar <aneesh.kumar@gmail.com> [2009.02.12.0909 +0100]:
> doing a tg update with latest git gives the below error
> 
> [extent_validate@linux-2.6]$ tg update
> fatal: Refusing to point HEAD outside of refs/heads/
> [extent_validate@linux-2.6]$

Which version? And could you please provide (a lot) more information
about your repository or make it available?

-- 
 .''`.   martin f. krafft <madduck@d.o>      Related projects:
: :'  :  proud Debian developer               http://debiansystem.info
`. `'`   http://people.debian.org/~madduck    http://vcs-pkg.org
  `-  Debian - when you have better things to do than fixing systems
 
"to me, vi is zen. to use vi is to practice zen. every command is
 a koan. profound to the user, unintelligible to the uninitiated.
 you discover truth everytime you use it."
                                       -- reddy ät lion.austin.ibm.com

[-- Attachment #2: Digital signature (see http://martin-krafft.net/gpg/) --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [topgit] tg update error

[Aneesh Kumar K.V]

On Thu, Feb 12, 2009 at 09:48:11AM +0100, martin f krafft wrote:
> also sprach Aneesh Kumar <aneesh.kumar@gmail.com> [2009.02.12.0909 +0100]:
> > doing a tg update with latest git gives the below error
> > 
> > [extent_validate@linux-2.6]$ tg update
> > fatal: Refusing to point HEAD outside of refs/heads/
> > [extent_validate@linux-2.6]$
> 
> Which version? And could you please provide (a lot) more information
> about your repository or make it available?
> 

Latest git and topgit. Moving to git version v1.6.1.3 fixed the issue.
I can reproduce the problem on any test repo. Just do a tg update after
committing something in the dependent branch.

-aneesh

Re: [topgit] tg update error

[martin f krafft]

[-- Attachment #1: Type: text/plain, Size: 2720 bytes --]

also sprach Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com> [2009.02.12.1025 +0100]:
> Latest git and topgit. Moving to git version v1.6.1.3 fixed the issue.
> I can reproduce the problem on any test repo. Just do a tg update after
> committing something in the dependent branch.

This is not helpful. Please provide a complete transcript of
a session reproducing the problem.

I can't:

piper:~|master|.tmp/cdt.GydvBgiR% echo foo > bar                                                 #10002
piper:~|master|.tmp/cdt.GydvBgiR% giti                                                           #10003
Initialized empty Git repository in /home/madduck/.tmp/cdt.GydvBgiR/.git/
Created initial commit 0f189f3: initial checkin
 1 files changed, 1 insertions(+), 0 deletions(-)
 create mode 100644 bar
piper:~/.tmp/cdt.GydvBgiR|master|% tg create test                                                #10004
tg: Automatically marking dependency on master
tg: Creating test base from master...
Switched to a new branch "test"
tg: Topic branch test set up. Please fill .topmsg now and make initial commit.
tg: To abort: git rm -f .top* && git checkout master && tg delete test
cached/staged changes:
 .topdeps |    1 +
 .topmsg  |    6 ++++++
piper:~/.tmp/cdt.GydvBgiR|master|% git commit -minit                                             #10005
Created commit d49ea41: init
 2 files changed, 7 insertions(+), 0 deletions(-)
 create mode 100644 .topdeps
 create mode 100644 .topmsg
piper:~/.tmp/cdt.GydvBgiR|test|% echo bar >> bar                                                 #10006
changes on filesystem:
 bar |    1 +
piper:~/.tmp/cdt.GydvBgiR|test|% git add bar                                                     #10007
cached/staged changes:
 bar |    1 +
piper:~/.tmp/cdt.GydvBgiR|test|% git commit -m'append'                                           #10008
Created commit e85457e: append
 1 files changed, 1 insertions(+), 0 deletions(-)
piper:~/.tmp/cdt.GydvBgiR|test|% tg update                                                       #10009
tg: The base is up-to-date.
tg: The test head is up-to-date wrt. the base.
piper:~/.tmp/cdt.GydvBgiR|test|% git --version                                                   #10010
git version 1.6.0.2
piper:~/.tmp/cdt.GydvBgiR|test|% tg --version                                                    #10011
Unknown subcommand: --version
TopGit v0.5 - A different patch queue manager
Usage: tg [-r REMOTE] (create|delete|depend|export|import|info|mail|patch|remote|summary|update|help) ...

-- 
martin | http://madduck.net/ | http://two.sentenc.es/
 
this space intentionally left blank.
 
spamtraps: madduck.bogus@madduck.net

[-- Attachment #2: Digital signature (see http://martin-krafft.net/gpg/) --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [topgit] tg update error

[Aneesh Kumar K.V]

On Thu, Feb 12, 2009 at 10:32:27AM +0100, martin f krafft wrote:
> also sprach Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com> [2009.02.12.1025 +0100]:
> > Latest git and topgit. Moving to git version v1.6.1.3 fixed the issue.
> > I can reproduce the problem on any test repo. Just do a tg update after
> > committing something in the dependent branch.
> 
> This is not helpful. Please provide a complete transcript of
> a session reproducing the problem.
> 
> I can't:
> 
> piper:~/.tmp/cdt.GydvBgiR|test|% git --version                                                   #10010
> git version 1.6.0.2

The git version that failed for me is the latest git. As I mentioned
above git version 1.6.1.3 works fine.

Can you test with
$git --version
git version 1.6.2.rc0.55.g30aa4f

-aneesh

Re: [topgit] tg update error

[Bert Wesarg]

On Thu, Feb 12, 2009 at 10:32, martin f krafft <madduck@madduck.net> wrote:
> also sprach Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com> [2009.02.12.1025 +0100]:
>> Latest git and topgit. Moving to git version v1.6.1.3 fixed the issue.
>> I can reproduce the problem on any test repo. Just do a tg update after
>> committing something in the dependent branch.
>
> This is not helpful. Please provide a complete transcript of
> a session reproducing the problem.
>
> I can't:
>
> piper:~|master|.tmp/cdt.GydvBgiR% echo foo > bar                                                 #10002
> piper:~|master|.tmp/cdt.GydvBgiR% giti                                                           #10003
> Initialized empty Git repository in /home/madduck/.tmp/cdt.GydvBgiR/.git/
> Created initial commit 0f189f3: initial checkin
>  1 files changed, 1 insertions(+), 0 deletions(-)
>  create mode 100644 bar
> piper:~/.tmp/cdt.GydvBgiR|master|% tg create test                                                #10004
> tg: Automatically marking dependency on master
> tg: Creating test base from master...
> Switched to a new branch "test"
> tg: Topic branch test set up. Please fill .topmsg now and make initial commit.
> tg: To abort: git rm -f .top* && git checkout master && tg delete test
> cached/staged changes:
>  .topdeps |    1 +
>  .topmsg  |    6 ++++++
> piper:~/.tmp/cdt.GydvBgiR|master|% git commit -minit                                             #10005
> Created commit d49ea41: init
>  2 files changed, 7 insertions(+), 0 deletions(-)
>  create mode 100644 .topdeps
>  create mode 100644 .topmsg
If I interpret the 'how to reproduce' right, you have to switch to the
master branch here.

> piper:~/.tmp/cdt.GydvBgiR|test|% echo bar >> bar                                                 #10006
> changes on filesystem:
>  bar |    1 +
> piper:~/.tmp/cdt.GydvBgiR|test|% git add bar                                                     #10007
> cached/staged changes:
>  bar |    1 +
> piper:~/.tmp/cdt.GydvBgiR|test|% git commit -m'append'                                           #10008
> Created commit e85457e: append
>  1 files changed, 1 insertions(+), 0 deletions(-)
And switch back to branch test here.

> piper:~/.tmp/cdt.GydvBgiR|test|% tg update                                                       #10009
> tg: The base is up-to-date.
> tg: The test head is up-to-date wrt. the base.
> piper:~/.tmp/cdt.GydvBgiR|test|% git --version                                                   #10010
> git version 1.6.0.2
> piper:~/.tmp/cdt.GydvBgiR|test|% tg --version                                                    #10011
> Unknown subcommand: --version
> TopGit v0.5 - A different patch queue manager
> Usage: tg [-r REMOTE] (create|delete|depend|export|import|info|mail|patch|remote|summary|update|help) ...
>
Bert

Re: [topgit] tg update error

[Jeff King]

On Thu, Feb 12, 2009 at 02:55:58PM +0530, Aneesh Kumar K.V wrote:

> On Thu, Feb 12, 2009 at 09:48:11AM +0100, martin f krafft wrote:
> > also sprach Aneesh Kumar <aneesh.kumar@gmail.com> [2009.02.12.0909 +0100]:
> > > doing a tg update with latest git gives the below error
> > >
> > > [extent_validate@linux-2.6]$ tg update
> > > fatal: Refusing to point HEAD outside of refs/heads/
> > > [extent_validate@linux-2.6]$
> >
> > Which version? And could you please provide (a lot) more information
> > about your repository or make it available?
> >
>
> Latest git and topgit. Moving to git version v1.6.1.3 fixed the issue.
> I can reproduce the problem on any test repo. Just do a tg update after
> committing something in the dependent branch.

This error message and safety valve are not in any released version of
git yet. So by moving back to 1.6.1.3, you are just predating the
addition of that message. :)

I think I know what is going on. A safety valve was added in afe5d3d to
disallow setting HEAD to anything that would violate git's "is this a
git directory" detector:

    symbolic ref: refuse non-ref targets in HEAD

    When calling "git symbolic-ref" it is easy to forget that
    the target must be a fully qualified ref. E.g., you might
    accidentally do:

      $ git symbolic-ref HEAD master

    Unfortunately, this is very difficult to recover from,
    because the bogus contents of HEAD make git believe we are
    no longer in a git repository (as is_git_dir explicitly
    checks for "^refs/heads/" in the HEAD target). So
    immediately trying to fix the situation doesn't work:

      $ git symbolic-ref HEAD refs/heads/master
      fatal: Not a git repository

    and one is left editing the .git/HEAD file manually.

Released versions of git just check "refs/" in HEAD. _But_ as part of
this patch series, b229d18 also tightened the "refs/" check to
"refs/heads/".

So what I suspect is happening is that topgit is trying to set HEAD to
"refs/top-bases/whatever". Aneesh, can you confirm by running your test
with GIT_TRACE=1?  I suspect you will see a call like "git symbolic-ref
HEAD refs/top-bases/foo".

Junio, I think we should probably revert b229d18 (and loosen
symbolic-ref's check to just "refs/"). Even if you want to argue that
topgit should be changed to handle this differently, we are still
breaking existing topgit installations, and who knows what other scripts
which might have relied on doing something like this.

-Peff

Re: [topgit] tg update error

[Jeff King]

On Thu, Feb 12, 2009 at 07:56:21AM -0500, Jeff King wrote:

> So what I suspect is happening is that topgit is trying to set HEAD to
> "refs/top-bases/whatever". Aneesh, can you confirm by running your test
> with GIT_TRACE=1?  I suspect you will see a call like "git symbolic-ref
> HEAD refs/top-bases/foo".

Actually, I was able to reproduce with the recipe from Martin and Bert
elsewhere in the thread. And that is indeed what is happening:

  trace: built-in: git 'symbolic-ref' 'HEAD' 'refs/top-bases/test'

-Peff

Re: [topgit] tg update error

[martin f krafft]

[-- Attachment #1: Type: text/plain, Size: 1204 bytes --]

also sprach Jeff King <peff@peff.net> [2009.02.12.1359 +0100]:
> > So what I suspect is happening is that topgit is trying to set HEAD to
> > "refs/top-bases/whatever". Aneesh, can you confirm by running your test
> > with GIT_TRACE=1?  I suspect you will see a call like "git symbolic-ref
> > HEAD refs/top-bases/foo".
> 
> Actually, I was able to reproduce with the recipe from Martin and Bert
> elsewhere in the thread. And that is indeed what is happening:
> 
>   trace: built-in: git 'symbolic-ref' 'HEAD'
>   'refs/top-bases/test'\

Thanks, Jeff, for the accurate analysis. Since I do not see a way
for topgit to do things differently -- top-bases are *not* heads and
thus warrant a different namespace -- can I assume that this is to
be fixed in Git and not in TopGit?

-- 
 .''`.   martin f. krafft <madduck@d.o>      Related projects:
: :'  :  proud Debian developer               http://debiansystem.info
`. `'`   http://people.debian.org/~madduck    http://vcs-pkg.org
  `-  Debian - when you have better things to do than fixing systems
 
"it is only the modern that ever becomes old-fashioned."
                                                        -- oscar wilde

[-- Attachment #2: Digital signature (see http://martin-krafft.net/gpg/) --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [topgit] tg update error

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Junio, I think we should probably revert b229d18 (and loosen
> symbolic-ref's check to just "refs/"). Even if you want to argue that
> topgit should be changed to handle this differently, we are still
> breaking existing topgit installations, and who knows what other scripts
> which might have relied on doing something like this.

I'm Ok with the revert (and I agree it is absolutely the right thing to do
at least for the short term).

But I still do agree with the reasoning for the change stated in its
commit log message:

    commit b229d18a809c169314b7f0d048dc5a7632e8f916
    Author: Jeff King <peff@peff.net>
    Date:   Thu Jan 29 03:30:16 2009 -0500

        validate_headref: tighten ref-matching to just branches

        When we are trying to determine whether a directory contains
        a git repository, one of the tests we do is to check whether
        HEAD is either a symlink or a symref into the "refs/"
        hierarchy, or a detached HEAD.

        We can tighten this a little more, though: a non-detached
        HEAD should always point to a branch (since checking out
        anything else should result in detachment), so it is safe to
        check for "refs/heads/".

        Signed-off-by: Jeff King <peff@peff.net>
        Signed-off-by: Junio C Hamano <gitster@pobox.com>

It would be nice to hear TopGit people defend why setting HEAD to outside
refs/heads/ is justified, why doing so should not break other things, and
why it was needed.

The last one is particularly important to avoid this kind of issue in the
future.  Perhaps they _knew_ some things refuse to work on refs outside
refs/heads/ and wanted to take advantage of that fact to protect their own
refs from vanilla git tools, but if that really is the case, the rules
they want have to be spelled out.

"git checkout" would refuse to switch to "refs/top-bases/frotz", because
it currently considers HEAD pointing outside refs/heads/ is insane, for
example.  But the revert of the above commit *means* that it is not
insane, and somebody may add an option to switch to any refs inside refs/
hierarchy.  If the reason TopGit points HEAD outside refs/heads hierarchy
were because they assume "git checkout" would never do so, such a change
would break them again (I am not seriously suggesting to add such an
option to "git checkout", but I am just using it to illustrate the point.
We would not know what other assumption, warranted or unwarranted, it is
making).

Re: [topgit] tg update error

[martin f krafft]

[-- Attachment #1: Type: text/plain, Size: 2825 bytes --]

also sprach Junio C Hamano <gitster@pobox.com> [2009.02.12.2201 +0100]:
> It would be nice to hear TopGit people defend why setting HEAD to outside
> refs/heads/ is justified, why doing so should not break other things, and
> why it was needed.

As far as I understand it, TopGit does not /need/ to set HEAD to
refs/top-bases/foo, but it currently does so as part of its
algorithm:

When tg-update updates a depending branch, it first merges the
dependent branch into the base of the topic branch, which is pointed
to by the corresponding top-base (refs/top-bases/foo). It then
merges the top-base into the topic branch, "foo" in this case.

The result is the same as if the base branch had been merged into
"foo", and refs/top-bases/foo updated to point to the head of the
base branch.

This stops working, however, as soon as you have a topic branch
depending on more than one base branches. Since you need to track
the base of a topic branch (e.g. in order to be able to get the diff
represented by the TopGit branch), you now have a problem: which of
the base branches is the base to diff against?

TopGit addresses this requirement by creating a "virtual" branch
into which it merges all the bases (into the top-base) first, and
then merging this "virtual" branch into the topic branch. The result
is a merge commit combining all bases, which is a parent of the
merge commit into the topic branch, and can thus serve as the origin
of a diff calculation.

TopGit right now does all of this while HEAD is detached: it points
into the refs/top-bases/* namespace -- the "virtual" branch. Here,
it does the merges of the bases, and then checks out the topic
branch to merge this combined ("virtual") base.

To work around the new restriction in Git, TopGit would need to make
a proper branch, merge the bases into it, merge that branch into the
topic branch, and the probably delete the branch pointer, as it's no
longer needed and would only pollute the refs/heads/* namespace. It
could certainly do this (with a minor performance impact), but it
seems like jumping through hoops and around Git's restrictions,
without any real benefit.

Point being: I understand the reason behind the restriction, and
I wouldn't mind if it were default, but maybe there could be
a controlled way to circumvent it for cases like the one described
above, where it is safe to assume that the user^W^W the tool "knows"
what it is doing.

-- 
 .''`.   martin f. krafft <madduck@d.o>      Related projects:
: :'  :  proud Debian developer               http://debiansystem.info
`. `'`   http://people.debian.org/~madduck    http://vcs-pkg.org
  `-  Debian - when you have better things to do than fixing systems
 
in the beginning was the word,
and the word was content-type: text/plain

[-- Attachment #2: Digital signature (see http://martin-krafft.net/gpg/) --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [topgit] tg update error

[Junio C Hamano]

martin f krafft <madduck@debian.org> writes:

> TopGit would need to make
> a proper branch, merge the bases into it, merge that branch into the
> topic branch, and the probably delete the branch pointer, as it's no
> longer needed and would only pollute the refs/heads/* namespace.

So it happens purely inside TopGit and the end user never sees a state
that HEAD points outside refs/heads/, right?

Why can't the base flipping operation you descibed be done on detached
HEAD?  Perhaps with a shell variable or two that hold commit object names
you need to keep track of while it is doing is work?

> Point being: I understand the reason behind the restriction, and
> I wouldn't mind if it were default, but maybe there could be
> a controlled way to circumvent it for cases like the one described
> above, where it is safe to assume that the user^W^W the tool "knows"
> what it is doing.

Sure, the tool would know what it is doing, I wouldn't doubt that.

But the end users don't.  If TopGit dies (or killed) during the base
flipping operation, doesn't the end user left in a funny state (granted, a
detached HEAD is also a funny state, but it is already a known funny state
they are familiar with.  HEAD that is a symref but points outside
refs/heads/ is a lot funnier).

You did not actually answer a larger question.  What other undocumented
features/restrictions does the code depend on, that tightening them to
help normal git users inadvertently may cause breakages similar to this
one in TopGit?

Re: [topgit] tg update error

[martin f krafft]

[-- Attachment #1: Type: text/plain, Size: 3133 bytes --]

also sprach Junio C Hamano <gitster@pobox.com> [2009.02.13.0014 +0100]:
> > TopGit would need to make a proper branch, merge the bases into
> > it, merge that branch into the topic branch, and the probably
> > delete the branch pointer, as it's no longer needed and would
> > only pollute the refs/heads/* namespace.
> 
> So it happens purely inside TopGit and the end user never sees
> a state that HEAD points outside refs/heads/, right?

Yes.

> Why can't the base flipping operation you descibed be done on
> detached HEAD?  Perhaps with a shell variable or two that hold
> commit object names you need to keep track of while it is doing is
> work?

I am not sure I understand. Isn't that what's currently happening?

Have a look at line 110 of tg-update.sh:

  http://git.debian.org/?p=collab-maint/topgit.git;a=blob;f=tg-update.sh;hb=HEAD#l110

> But the end users don't.  If TopGit dies (or killed) during the base
> flipping operation, doesn't the end user left in a funny state (granted, a
> detached HEAD is also a funny state, but it is already a known funny state
> they are familiar with.  HEAD that is a symref but points outside
> refs/heads/ is a lot funnier).

If topgit is killed, yes, then the repo could be left in a funny
state. I suppose this could be addressed by putting proper traps in
place.

If the merge fails, however, then the user is advised what to do;
see lines 114ff.

> You did not actually answer a larger question.

It wasn't asked to me before... ;)

> What other undocumented features/restrictions does the code depend
> on, that tightening them to help normal git users inadvertently
> may cause breakages similar to this one in TopGit?

I think Petr would need to help out answering this.

I agree that it would be good to address each such occurrence in
turn and replace it with a method that only makes use of the public
API. Up until now, however,

  git checkout -q "refs/top-bases/$name"

was not really something undocmented or restricted. I find it rather
difficult to separate
public-as-in-every-user-can-and-should-use-this features from
restricted-better-be-left-alone-unless-you-really-know-what-you-are-doing
features with Git. This has gotten *a lot* better, but the fact that
I can still call e.g. git update-ref (as opposed to e.g. git
_update-ref)  and potentially turn my repository upside down
exemplifies this.

Maybe Petr remembers all the instances when he sneakily used tricks
to make things work, and then we can look at each of them in turn.

Maybe some of you could go through the code (which isn't /that/
much), looking for instances of not-so-public API abuse and help us
identify them too.

Cheers,

-- 
 .''`.   martin f. krafft <madduck@d.o>      Related projects:
: :'  :  proud Debian developer               http://debiansystem.info
`. `'`   http://people.debian.org/~madduck    http://vcs-pkg.org
  `-  Debian - when you have better things to do than fixing systems
 
"without music, life would be a mistake."
                                                 - friedrich nietzsche

[-- Attachment #2: Digital signature (see http://martin-krafft.net/gpg/) --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [topgit] tg update error

[Junio C Hamano]

martin f krafft <madduck@debian.org> writes:

> also sprach Junio C Hamano <gitster@pobox.com> [2009.02.13.0014 +0100]:
>> > TopGit would need to make a proper branch, merge the bases into
>> > it, merge that branch into the topic branch, and the probably
>> > delete the branch pointer, as it's no longer needed and would
>> > only pollute the refs/heads/* namespace.
>> 
>> So it happens purely inside TopGit and the end user never sees
>> a state that HEAD points outside refs/heads/, right?
>
> Yes.

Now I am confused by your answers.  This "Yes" means that setting HEAD
outside refs/heads/ happens purely as an intermediate state to avoid
setting HEAD to some branch ref.  After the operation finishes correctly,
HEAD will never be left outside refs/heads/.  But this contradicts
directly with what you say next.

>> Why can't the base flipping operation you descibed be done on
>> detached HEAD?  Perhaps with a shell variable or two that hold
>> commit object names you need to keep track of while it is doing is
>> work?
>
> I am not sure I understand. Isn't that what's currently happening?

If you *are* setting HEAD to some ref that is outside refs/heads (or even
inside refs/heads for that matter), at that point the HEAD is *not*
detached, so no, it obviously is *not* what is happening.

I am asking why you need to use a ref to do that, *if* it is a tentative
state while the program is running.  You are probably calling a git
plumbing or Porcelain command that updates HEAD, and the reason why you
point HEAD outside refs/heads/ is beause you would want the command you
call to update one of the refs/top-bases/ ref through HEAD.  I am asking
why you are not running these commands on a normal detached HEAD, and then
use update-ref (not symbolic-ref) plumbing to update the refs/top-bases/
ref you would want to update when it is done.

>> You did not actually answer a larger question.
>
> It wasn't asked to me before... ;)

Go back to the original message and read it again.

> Up until now, however,
>
>   git checkout -q "refs/top-bases/$name"
>
> was not really something undocmented or restricted.

Giving checkout anything that is not "a branch name" meant detaching HEAD
ever since detached HEAD was introduced, and that is a documented feature.
git checkout "refs/heads/master" would behave the same way --- it won't
check out the 'master' branch.

> I can still call e.g. git update-ref (as opposed to e.g. git
> _update-ref)  and potentially turn my repository upside down
> exemplifies this.

The distinction between Porcelain and plumbing is unfortunately not very
clear at places.  The change we reverted was probably a bad one.  The
stricter check was not done at the Porcelain level but was done at the
plumbing level.

Re: [topgit] tg update error

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> If you *are* setting HEAD to some ref that is outside refs/heads (or even
> inside refs/heads for that matter), at that point the HEAD is *not*
> detached, so no, it obviously is *not* what is happening.
>
> I am asking why you need to use a ref to do that, *if* it is a tentative
> state while the program is running.  You are probably calling a git
> plumbing or Porcelain command that updates HEAD, and the reason why you
> point HEAD outside refs/heads/ is beause you would want the command you
> call to update one of the refs/top-bases/ ref through HEAD.  I am asking
> why you are not running these commands on a normal detached HEAD, and then
> use update-ref (not symbolic-ref) plumbing to update the refs/top-bases/
> ref you would want to update when it is done.

Ok, I did read the script (yuck).  You do break out of TopGit process when
a merge conflict prevents the update operation to complete and do give
control back to the end user, so you can leave HEAD in a state that points
at a non-branch, and you do use the fact that the HEAD is pointing at
something funny as a sign that you are in the middle of conflicted merge
resolution.

It is just like how vanilla git uses MERGE_HEAD as the marker to signal
that it is in a funny state.

While I think it is a cute idea to use which funny hierarchy HEAD points
at to indicate what funny/intermediate state your interrupted operation is
in, and it may seem to be cleaner than using a marker file like MERGE_HEAD
at first sight, I do not think it is a wise thing to do in the long run.

You can only express two pieces of information (the overall "category of
state" by which funny ref/ hierarchy HEAD points at, and one object name
by storing it in the ref pointed at by HEAD), and if you need more (such
as MERGE_MSG that stores pre-packaged log message pieces is used during a
merge, in addition to MERGE_HEAD), you would need to use more than just
the "cute HEAD" trick to store them *anyway*.  Which means that it is a
bad tradeoff to use "cute HEAD" --- it closes the possibility to detect
user error to point HEAD at an incorrect place and I do not see the
benefit of "cute HEAD" outweigh the downside.

Re: [topgit] tg update error

[Jeff King]

On Thu, Feb 12, 2009 at 01:01:42PM -0800, Junio C Hamano wrote:

> > Junio, I think we should probably revert b229d18 (and loosen
> > symbolic-ref's check to just "refs/"). Even if you want to argue that
> > topgit should be changed to handle this differently, we are still
> > breaking existing topgit installations, and who knows what other scripts
> > which might have relied on doing something like this.
> 
> I'm Ok with the revert (and I agree it is absolutely the right thing to do
> at least for the short term).

It looks like you have already pushed out the revert. But I think we
need this on top to make topgit work correctly.

-- >8 --
Subject: [PATCH] symbolic-ref: allow refs/<whatever> in HEAD

Commit afe5d3d5 introduced a safety valve to symbolic-ref to
disallow installing an invalid HEAD. It was accompanied by
b229d18a, which changed validate_headref to require that
HEAD contain a pointer to refs/heads/ instead of just refs/.
Therefore, the safety valve also checked for refs/heads/.

As it turns out, topgit is using refs/top-bases/ in HEAD,
leading us to re-loosen (at least temporarily) the
validate_headref check made in b229d18a. This patch does the
corresponding loosening for the symbolic-ref safety valve,
so that the two are in agreement once more.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin-symbolic-ref.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin-symbolic-ref.c b/builtin-symbolic-ref.c
index cafc4eb..6ae6bcc 100644
--- a/builtin-symbolic-ref.c
+++ b/builtin-symbolic-ref.c
@@ -45,8 +45,8 @@ int cmd_symbolic_ref(int argc, const char **argv, const char *prefix)
 		break;
 	case 2:
 		if (!strcmp(argv[0], "HEAD") &&
-		    prefixcmp(argv[1], "refs/heads/"))
-			die("Refusing to point HEAD outside of refs/heads/");
+		    prefixcmp(argv[1], "refs/"))
+			die("Refusing to point HEAD outside of refs/");
 		create_symref(argv[0], argv[1], msg);
 		break;
 	default:
-- 
1.6.2.rc0.241.g088a

Re: [topgit] tg update error

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Feb 12, 2009 at 01:01:42PM -0800, Junio C Hamano wrote:
>
>> > Junio, I think we should probably revert b229d18 (and loosen
>> > symbolic-ref's check to just "refs/"). Even if you want to argue that
>> > topgit should be changed to handle this differently, we are still
>> > breaking existing topgit installations, and who knows what other scripts
>> > which might have relied on doing something like this.
>> 
>> I'm Ok with the revert (and I agree it is absolutely the right thing to do
>> at least for the short term).
>
> It looks like you have already pushed out the revert. But I think we
> need this on top to make topgit work correctly.

>
> -- >8 --
> Subject: [PATCH] symbolic-ref: allow refs/<whatever> in HEAD
>
> Commit afe5d3d5 introduced a safety valve to symbolic-ref to
> disallow installing an invalid HEAD. It was accompanied by
> b229d18a, which changed validate_headref to require that
> HEAD contain a pointer to refs/heads/ instead of just refs/.
> Therefore, the safety valve also checked for refs/heads/.
>
> As it turns out, topgit is using refs/top-bases/ in HEAD,
> leading us to re-loosen (at least temporarily) the
> validate_headref check made in b229d18a. This patch does the
> corresponding loosening for the symbolic-ref safety valve,
> so that the two are in agreement once more.
>
> Signed-off-by: Jeff King <peff@peff.net>

Actually we should simply revert afe5d3d5 altogether with the above
message, as it introduced a test that expects the tightened behaviour.

Re: [topgit] tg update error

[Jeff King]

On Fri, Feb 13, 2009 at 06:02:56PM -0800, Junio C Hamano wrote:

> > As it turns out, topgit is using refs/top-bases/ in HEAD,
> > leading us to re-loosen (at least temporarily) the
> > validate_headref check made in b229d18a. This patch does the
> > corresponding loosening for the symbolic-ref safety valve,
> > so that the two are in agreement once more.
> >
> > Signed-off-by: Jeff King <peff@peff.net>
> 
> Actually we should simply revert afe5d3d5 altogether with the above
> message, as it introduced a test that expects the tightened behaviour.

Is there any reason to throw away the "must be in refs/" safety valve,
though? That was the actual patch I started with and solved my problem,
and the "tighten to refs/heads/" bit came from discussion. That is, I
think having a safety valve in symbolic-ref that matches
validate_headref is orthogonal to how tightly validate_headref matches.

But yes, I obviously failed to run the test suite on the follow-up patch
I sent. The final test in t1401 would need to be reverted, as well.

-Peff

Re: [topgit] tg update error

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Feb 13, 2009 at 06:02:56PM -0800, Junio C Hamano wrote:
>
>> > As it turns out, topgit is using refs/top-bases/ in HEAD,
>> > leading us to re-loosen (at least temporarily) the
>> > validate_headref check made in b229d18a. This patch does the
>> > corresponding loosening for the symbolic-ref safety valve,
>> > so that the two are in agreement once more.
>> >
>> > Signed-off-by: Jeff King <peff@peff.net>
>> 
>> Actually we should simply revert afe5d3d5 altogether with the above
>> message, as it introduced a test that expects the tightened behaviour.
>
> Is there any reason to throw away the "must be in refs/" safety valve,
> though? That was the actual patch I started with and solved my problem,
> and the "tighten to refs/heads/" bit came from discussion. That is, I
> think having a safety valve in symbolic-ref that matches
> validate_headref is orthogonal to how tightly validate_headref matches.
>
> But yes, I obviously failed to run the test suite on the follow-up patch
> I sent. The final test in t1401 would need to be reverted, as well.

Sure.

Re: [topgit] tg update error

[Jeff King]

On Fri, Feb 13, 2009 at 06:16:23PM -0800, Junio C Hamano wrote:

> > Is there any reason to throw away the "must be in refs/" safety valve,
> > though? That was the actual patch I started with and solved my problem,
> > and the "tighten to refs/heads/" bit came from discussion. That is, I
> > think having a safety valve in symbolic-ref that matches
> > validate_headref is orthogonal to how tightly validate_headref matches.
> >
> > But yes, I obviously failed to run the test suite on the follow-up patch
> > I sent. The final test in t1401 would need to be reverted, as well.
> 
> Sure.

OK, here is the updated patch (that actually passes the test suite).

-- >8 --
Subject: [PATCH] symbolic-ref: allow refs/<whatever> in HEAD

Commit afe5d3d5 introduced a safety valve to symbolic-ref to
disallow installing an invalid HEAD. It was accompanied by
b229d18a, which changed validate_headref to require that
HEAD contain a pointer to refs/heads/ instead of just refs/.

As it turns out, topgit is using refs/top-bases/ in HEAD,
leading us to re-loosen (at least temporarily) the
validate_headref check made in b229d18a. This patch does the
corresponding loosening for the symbolic-ref safety check,
so that the two are in agreement once more.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin-symbolic-ref.c  |    4 ++--
 t/t1401-symbolic-ref.sh |    5 -----
 2 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/builtin-symbolic-ref.c b/builtin-symbolic-ref.c
index cafc4eb..6ae6bcc 100644
--- a/builtin-symbolic-ref.c
+++ b/builtin-symbolic-ref.c
@@ -45,8 +45,8 @@ int cmd_symbolic_ref(int argc, const char **argv, const char *prefix)
 		break;
 	case 2:
 		if (!strcmp(argv[0], "HEAD") &&
-		    prefixcmp(argv[1], "refs/heads/"))
-			die("Refusing to point HEAD outside of refs/heads/");
+		    prefixcmp(argv[1], "refs/"))
+			die("Refusing to point HEAD outside of refs/");
 		create_symref(argv[0], argv[1], msg);
 		break;
 	default:
diff --git a/t/t1401-symbolic-ref.sh b/t/t1401-symbolic-ref.sh
index 569f341..7fa5f5b 100755
--- a/t/t1401-symbolic-ref.sh
+++ b/t/t1401-symbolic-ref.sh
@@ -27,11 +27,6 @@ test_expect_success 'symbolic-ref refuses non-ref for HEAD' '
 '
 reset_to_sane
 
-test_expect_success 'symbolic-ref refuses non-branch for HEAD' '
-	test_must_fail git symbolic-ref HEAD refs/foo
-'
-reset_to_sane
-
 test_expect_success 'symbolic-ref refuses bare sha1' '
 	echo content >file && git add file && git commit -m one
 	test_must_fail git symbolic-ref HEAD `git rev-parse HEAD`
-- 
1.6.2.rc0.241.g088a