Re: [PATCHv1 2/2] git-p4: work with a detached head
On 28/10/15 17:44, Junio C Hamano wrote: Luke Diamand writes: On 9 September 2015 at 22:52, Junio C Hamano wrote: Luke Diamand writes: ... 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. No need to rush, but should I expect a reroll of this sometime, or have things around this topic changed to make this topic no longer necessary? I am only asking so that I can decide to either keep or drop ld/p4-detached-head topic that is listed in the [Stalled] section for quite some time [*1*]. I was waiting for the other git-p4 changes to go through before starting this up again. It definitely needs fixing - it was annoying me a lot today, as I kept on having to invent temporary branch names to needlessly keep git-p4 happy. After getting to "for-p4-9", I'm now onto "x". I'll see if I can sort something out in the next few days. Luke Thanks. [Footnote] *1* Not that my dropping a topic from 'pu' means very much; a dropped topic can still be submitted and requeued after all. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv1 2/2] git-p4: work with a detached head
Luke Diamand writes: > On 9 September 2015 at 22:52, Junio C Hamano wrote: >> Luke Diamand writes: >> ... >> 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. No need to rush, but should I expect a reroll of this sometime, or have things around this topic changed to make this topic no longer necessary? I am only asking so that I can decide to either keep or drop ld/p4-detached-head topic that is listed in the [Stalled] section for quite some time [*1*]. Thanks. [Footnote] *1* Not that my dropping a topic from 'pu' means very much; a dropped topic can still be submitted and requeued after all. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv1 2/2] git-p4: work with a detached head
Luke Diamand writes: > On 9 September 2015 at 22:52, Junio C Hamano wrote: >> Luke Diamand 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv1 2/2] git-p4: work with a detached head
On 9 September 2015 at 22:52, Junio C Hamano wrote: > Luke Diamand 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. Thanks, Luke -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv1 2/2] git-p4: work with a detached head
Luke Diamand 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? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv1 2/2] git-p4: work with a detached head
When submitting, git-p4 finds the current branch in order to know if it is allowed to submit (configuration "git-p4.allowSubmit"). On a detached head, detecting the branch would fail, and git-p4 would report a cryptic error. This change teaches git-p4 to recognise a detached head and submit successfully. Signed-off-by: Luke Diamand --- git-p4.py | 18 -- t/t9800-git-p4-basic.sh | 2 +- 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/git-p4.py b/git-p4.py index 2677c89..a22ae01 100755 --- a/git-p4.py +++ b/git-p4.py @@ -1651,8 +1651,8 @@ class P4Submit(Command, P4UserMap): 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 elif len(args) == 1: self.master = args[0] if not branchExists(self.master): @@ -1660,9 +1660,10 @@ class P4Submit(Command, P4UserMap): else: return False -allowSubmit = gitConfig("git-p4.allowSubmit") -if len(allowSubmit) > 0 and not self.master in allowSubmit.split(","): -die("%s is not in git-p4.allowSubmit" % self.master) +if self.master: +allowSubmit = gitConfig("git-p4.allowSubmit") +if len(allowSubmit) > 0 and not self.master in allowSubmit.split(","): +die("%s is not in git-p4.allowSubmit" % self.master) [upstream, settings] = findUpstreamBranchPoint() self.depotPath = settings['depot-paths'][0] @@ -1730,7 +1731,12 @@ class P4Submit(Command, P4UserMap): self.check() commits = [] -for line in read_pipe_lines(["git", "rev-list", "--no-merges", "%s..%s" % (self.origin, self.master)]): +if self.master: +commitish = self.master +else: +commitish = 'HEAD' + +for line in read_pipe_lines(["git", "rev-list", "--no-merges", "%s..%s" % (self.origin, commitish)]): commits.append(line.strip()) commits.reverse() diff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh index 114b19f..0730f18 100755 --- a/t/t9800-git-p4-basic.sh +++ b/t/t9800-git-p4-basic.sh @@ -241,7 +241,7 @@ test_expect_success 'unresolvable host in P4PORT should display error' ' ) ' -test_expect_failure 'submit from detached head' ' +test_expect_success 'submit from detached head' ' test_when_finished cleanup_git && git p4 clone --dest="$git" //depot && ( -- 2.6.0.rc0.133.ga438a11.dirty -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html