* [PATCHv1 0/2] git-p4: work on a detached head
@ 2015-09-05 14:02 Luke Diamand
2015-09-05 14:02 ` [PATCHv1 1/2] git-p4: add failing test for submit from " Luke Diamand
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Luke Diamand @ 2015-09-05 14:02 UTC (permalink / raw
To: git; +Cc: Luke Diamand
git-p4 won't submit from a detached head. If you do, it gives a
cryptic error message, and instead you have to create an endless
series of throw-away branches, which can get very trying.
The first patch in this series demonstrates the problem, and the
second patch fixes it. I've been using it for the last few days.
Luke Diamand (2):
git-p4: add failing test for submit from detached head
git-p4: work with a detached head
git-p4.py | 18 ++++++++++++------
t/t9800-git-p4-basic.sh | 16 ++++++++++++++++
2 files changed, 28 insertions(+), 6 deletions(-)
--
2.6.0.rc0.133.ga438a11.dirty
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCHv1 1/2] git-p4: add failing test for submit from detached head
2015-09-05 14:02 [PATCHv1 0/2] git-p4: work on a detached head Luke Diamand
@ 2015-09-05 14:02 ` Luke Diamand
2015-09-05 14:02 ` [PATCHv1 2/2] git-p4: work with a " Luke Diamand
2015-09-09 12:03 ` [PATCHv1 0/2] git-p4: work on " Lars Schneider
2 siblings, 0 replies; 12+ messages in thread
From: Luke Diamand @ 2015-09-05 14:02 UTC (permalink / raw
To: git; +Cc: Luke Diamand
git-p4 can't submit from a detached head. This test case
demonstrates the problem.
Signed-off-by: Luke Diamand <luke@diamand.org>
---
t/t9800-git-p4-basic.sh | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh
index 90d41ed..114b19f 100755
--- a/t/t9800-git-p4-basic.sh
+++ b/t/t9800-git-p4-basic.sh
@@ -241,6 +241,22 @@ test_expect_success 'unresolvable host in P4PORT should display error' '
)
'
+test_expect_failure 'submit from detached head' '
+ test_when_finished cleanup_git &&
+ git p4 clone --dest="$git" //depot &&
+ (
+ cd "$git" &&
+ git checkout p4/master &&
+ >detached_head_test &&
+ git add detached_head_test &&
+ git commit -m "add detached_head" &&
+ git config git-p4.skipSubmitEdit true &&
+ git p4 submit &&
+ git p4 rebase &&
+ git log p4/master | grep detached_head
+ )
+'
+
test_expect_success 'kill p4d' '
kill_p4d
'
--
2.6.0.rc0.133.ga438a11.dirty
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCHv1 2/2] git-p4: work with a detached head
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 ` Luke Diamand
2015-09-09 21:52 ` Junio C Hamano
2015-09-09 12:03 ` [PATCHv1 0/2] git-p4: work on " Lars Schneider
2 siblings, 1 reply; 12+ messages in thread
From: Luke Diamand @ 2015-09-05 14:02 UTC (permalink / raw
To: git; +Cc: Luke Diamand
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 <luke@diamand.org>
---
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCHv1 0/2] git-p4: work on a detached head
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 12:03 ` Lars Schneider
2015-09-10 1:57 ` Jacob Keller
2 siblings, 1 reply; 12+ messages in thread
From: Lars Schneider @ 2015-09-09 12:03 UTC (permalink / raw
To: Luke Diamand; +Cc: git
I wanted to play with the patch and apply it to my source but the process is really complicated for me. I wonder if you can give me a few recommendations how to work efficiently with email patches. I don’t want to start a flame-war about what email client is “right", I am just curious how you work and what clients, scripts, or tricks you use :-)
Usually I use Apple Mail. I experimented with mutt but it was not dramatically more convenient.
Thanks,
Lars
On 05 Sep 2015, at 16:02, Luke Diamand <luke@diamand.org> wrote:
> git-p4 won't submit from a detached head. If you do, it gives a
> cryptic error message, and instead you have to create an endless
> series of throw-away branches, which can get very trying.
>
> The first patch in this series demonstrates the problem, and the
> second patch fixes it. I've been using it for the last few days.
>
> Luke Diamand (2):
> git-p4: add failing test for submit from detached head
> git-p4: work with a detached head
>
> git-p4.py | 18 ++++++++++++------
> t/t9800-git-p4-basic.sh | 16 ++++++++++++++++
> 2 files changed, 28 insertions(+), 6 deletions(-)
>
> --
> 2.6.0.rc0.133.ga438a11.dirty
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCHv1 2/2] git-p4: work with a detached head
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
0 siblings, 1 reply; 12+ messages in thread
From: Junio C Hamano @ 2015-09-09 21:52 UTC (permalink / raw
To: Luke Diamand; +Cc: git
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?
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCHv1 0/2] git-p4: work on a detached head
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
0 siblings, 1 reply; 12+ messages in thread
From: Jacob Keller @ 2015-09-10 1:57 UTC (permalink / raw
To: Lars Schneider; +Cc: Luke Diamand, Git List
On Wed, Sep 9, 2015 at 5:03 AM, Lars Schneider <larsxschneider@gmail.com> wrote:
> I wanted to play with the patch and apply it to my source but the process is really complicated for me. I wonder if you can give me a few recommendations how to work efficiently with email patches. I don’t want to start a flame-war about what email client is “right", I am just curious how you work and what clients, scripts, or tricks you use :-)
>
> Usually I use Apple Mail. I experimented with mutt but it was not dramatically more convenient.
>
> Thanks,
> Lars
Generally, any client which lets you save patches as mbox format. Then
you can feed the file via git-am
I don't know what clients make this easy. I normally use Evolution,
which has as "save as mbox" option. It's possible Apple Mail does too.
Regards,
Jake
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCHv1 0/2] git-p4: work on a detached head
2015-09-10 1:57 ` Jacob Keller
@ 2015-09-10 1:59 ` Jacob Keller
2015-09-10 12:23 ` Luke Diamand
0 siblings, 1 reply; 12+ messages in thread
From: Jacob Keller @ 2015-09-10 1:59 UTC (permalink / raw
To: Lars Schneider; +Cc: Luke Diamand, Git List
On Wed, Sep 9, 2015 at 6:57 PM, Jacob Keller <jacob.keller@gmail.com> wrote:
> On Wed, Sep 9, 2015 at 5:03 AM, Lars Schneider <larsxschneider@gmail.com> wrote:
>> I wanted to play with the patch and apply it to my source but the process is really complicated for me. I wonder if you can give me a few recommendations how to work efficiently with email patches. I don’t want to start a flame-war about what email client is “right", I am just curious how you work and what clients, scripts, or tricks you use :-)
>>
>> Usually I use Apple Mail. I experimented with mutt but it was not dramatically more convenient.
>>
>> Thanks,
>> Lars
>
> Generally, any client which lets you save patches as mbox format. Then
> you can feed the file via git-am
>
> I don't know what clients make this easy. I normally use Evolution,
> which has as "save as mbox" option. It's possible Apple Mail does too.
>
> Regards,
> Jake
For the record, you can use "show original" from GMail, and save that
file as well, I believe. I don't know about Apple Mail specifically,
however.
Regards,
Jake
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCHv1 2/2] git-p4: work with a detached head
2015-09-09 21:52 ` Junio C Hamano
@ 2015-09-10 7:29 ` Luke Diamand
2015-09-10 16:20 ` Junio C Hamano
2015-10-28 17:44 ` Junio C Hamano
0 siblings, 2 replies; 12+ messages in thread
From: Luke Diamand @ 2015-09-10 7:29 UTC (permalink / raw
To: Junio C Hamano; +Cc: Git Users
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.
Thanks,
Luke
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCHv1 0/2] git-p4: work on a detached head
2015-09-10 1:59 ` Jacob Keller
@ 2015-09-10 12:23 ` Luke Diamand
0 siblings, 0 replies; 12+ messages in thread
From: Luke Diamand @ 2015-09-10 12:23 UTC (permalink / raw
To: Jacob Keller; +Cc: Lars Schneider, Git List
On 10 September 2015 at 02:59, Jacob Keller <jacob.keller@gmail.com> wrote:
> On Wed, Sep 9, 2015 at 6:57 PM, Jacob Keller <jacob.keller@gmail.com> wrote:
>> On Wed, Sep 9, 2015 at 5:03 AM, Lars Schneider <larsxschneider@gmail.com> wrote:
>>> I wanted to play with the patch and apply it to my source but the process is really complicated for me. I wonder if you can give me a few recommendations how to work efficiently with email patches. I don’t want to start a flame-war about what email client is “right", I am just curious how you work and what clients, scripts, or tricks you use :-)
>>>
>>> Usually I use Apple Mail. I experimented with mutt but it was not dramatically more convenient.
>>>
>>> Thanks,
>>> Lars
>>
>> Generally, any client which lets you save patches as mbox format. Then
>> you can feed the file via git-am
>>
>> I don't know what clients make this easy. I normally use Evolution,
>> which has as "save as mbox" option. It's possible Apple Mail does too.
>>
>> Regards,
>> Jake
>
> For the record, you can use "show original" from GMail, and save that
> file as well, I believe. I don't know about Apple Mail specifically,
> however.
I also don't use Apple Mail, but it must surely have a way to get the
original email text. Save that and then do:
$ git am /path/to/saved-file
At least, that's what I do.
Luke
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCHv1 2/2] git-p4: work with a detached head
2015-09-10 7:29 ` Luke Diamand
@ 2015-09-10 16:20 ` Junio C Hamano
2015-10-28 17:44 ` Junio C Hamano
1 sibling, 0 replies; 12+ messages in thread
From: Junio C Hamano @ 2015-09-10 16:20 UTC (permalink / raw
To: Luke Diamand; +Cc: Git Users
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCHv1 2/2] git-p4: work with a detached head
2015-09-10 7:29 ` Luke Diamand
2015-09-10 16:20 ` Junio C Hamano
@ 2015-10-28 17:44 ` Junio C Hamano
2015-10-28 19:03 ` Luke Diamand
1 sibling, 1 reply; 12+ messages in thread
From: Junio C Hamano @ 2015-10-28 17:44 UTC (permalink / raw
To: Luke Diamand; +Cc: Git Users
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 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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCHv1 2/2] git-p4: work with a detached head
2015-10-28 17:44 ` Junio C Hamano
@ 2015-10-28 19:03 ` Luke Diamand
0 siblings, 0 replies; 12+ messages in thread
From: Luke Diamand @ 2015-10-28 19:03 UTC (permalink / raw
To: Junio C Hamano; +Cc: Git Users
On 28/10/15 17:44, Junio C Hamano wrote:
> 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 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 "xyyyy". 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.
>
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2015-10-28 19:03 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
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
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
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).