From: Junio C Hamano <gitster@pobox•com>
To: Luke Diamand <luke@diamand•org>
Cc: Git Users <git@vger•kernel.org>
Subject: Re: [PATCHv1 2/2] git-p4: work with a detached head
Date: Thu, 10 Sep 2015 09:20:08 -0700 [thread overview]
Message-ID: <xmqq7fny5jrb.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <CAE5ih7-_zhxf_daVxNB-hxFfbakMbJp9rk0vP+k46ErrJ6Qxiw@mail.gmail.com> (Luke Diamand's message of "Thu, 10 Sep 2015 08:29:07 +0100")
Luke Diamand <luke@diamand•org> writes:
> On 9 September 2015 at 22:52, Junio C Hamano <gitster@pobox•com> wrote:
>> Luke Diamand <luke@diamand•org> writes:
>>
>>> def run(self, args):
>>> if len(args) == 0:
>>> self.master = currentGitBranch()
>>> - if len(self.master) == 0 or not gitBranchExists("refs/heads/%s" % self.master):
>>> - die("Detecting current git branch failed!")
>>> + if self.master == "undefined":
>>> + self.master = None
>>
>> The comparison with textual "undefined" smelled fishy and I ended up
>> looking at the implementation of currentGitBranch().
>>
>> def currentGitBranch():
>> return read_pipe("git name-rev HEAD").split(" ")[1].strip()
>>
>> Yuck. I know it is not entirely the fault of this patch, but
>> shouldn't it be reading from
>>
>> $ git symbolic-ref HEAD
>>
>> and catch the error "fatal: ref HEAD is not a symbolic ref" and use
>> it as a signal to tell that the HEAD is detached?
>
> That sounds much nicer. I'll redo the patch accordingly.
While "symbolic-ref" _is_ the right way to learn what the currently
checked out branch is, I think you'd need to be a bit careful while
analysing the ramifications of that fix.
Notice:
$ git checkout ld/p4-detached-head
$ git symbolic-ref -q HEAD; echo $?
refs/heads/ld/p4-detached-head
0
$ git checkout HEAD^0
$ git symbolic-ref -q HEAD; echo $?
1
$ git name-rev HEAD
HEAD ld/p4-detached-head
A few implications of the above observation:
* The fact that the code used to use 'name-rev HEAD' means that it
behaved as if you are on some branch when you are in a detached
HEAD state, if your current commit happened to be at the tip of
some branch. Users could be relying on this behaviour, i.e.
you can detach (perhaps because you do not want to accidentally
advance the history of the real branch) at the tip of a branch,
and have "git p4" still apply the configuration based on the name
of the original branch.
* If there were multiple branches that point at your current
commit, then what is returned by currentGitBranch based on
name-rev is unpredictable. So in that sense, the workflow that
relies on the existing "use the configuration based on the branch
name returned by name-rev" behaviour is already broken, but not
many people have two or more branches pointing at the same commit
very often, so they may perceive existing breakage of their
workflow a non-issue. To them, fixing the implementation of
currentGitBranch may appear to be a regression.
* Of course, if the user happens to have a branch whose name is
"undefined", you may have detached the HEAD at a totally
unrelated commit, and the existing code in run() would first set
self.master to "undefined", notices "refs/heads/undefined"
exists, and without even noticing that these two are not related
to each other at all, happily goes ahead and uses "undefined"
branch. I don't know what happens in that case---perhaps it is
the same as the second case where configuration for an unrelated
branch is applied and no other damage is done. Perhaps the code
gets confused and sometimes updates HEAD and sometimes updates
the tip of self.master branch. In either case, the existing
behaviour cannot be something the users have sensibly relied on.
A good write-up of the bug in the externally visible behaviour that
is corrected by fixing currentGitBranch implementation in the
Release Notes (when this fix hits a release) should be sufficient, I
think.
Thanks.
next prev parent reply other threads:[~2015-09-10 16:20 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-09-05 14:02 [PATCHv1 0/2] git-p4: work on a detached head Luke Diamand
2015-09-05 14:02 ` [PATCHv1 1/2] git-p4: add failing test for submit from " Luke Diamand
2015-09-05 14:02 ` [PATCHv1 2/2] git-p4: work with a " Luke Diamand
2015-09-09 21:52 ` Junio C Hamano
2015-09-10 7:29 ` Luke Diamand
2015-09-10 16:20 ` Junio C Hamano [this message]
2015-10-28 17:44 ` Junio C Hamano
2015-10-28 19:03 ` Luke Diamand
2015-09-09 12:03 ` [PATCHv1 0/2] git-p4: work on " Lars Schneider
2015-09-10 1:57 ` Jacob Keller
2015-09-10 1:59 ` Jacob Keller
2015-09-10 12:23 ` Luke Diamand
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqq7fny5jrb.fsf@gitster.mtv.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=luke@diamand$(echo .)org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox