[PATCHv1,2/3] git-p4: unshelve into refs/remotes/p4-unshelved, not refs/remotes/p4/unshelved
diff mbox series

Message ID 20181012052833.6945-3-luke@diamand.org
State New
Headers show
Series
  • git-p4: improved unshelving
Related show

Commit Message

Luke Diamand Oct. 12, 2018, 5:28 a.m. UTC
The branch detection code looks for branches under refs/remotes/p4/...
and can end up getting confused if there are unshelved changes in
there as well. This happens in the function p4BranchesInGit().

Instead, put the unshelved changes into refs/remotes/p4-unshelved/<N>.

Signed-off-by: Luke Diamand <luke@diamand.org>
---
 Documentation/git-p4.txt | 6 +++---
 git-p4.py                | 3 ++-
 t/t9832-unshelve.sh      | 6 +++---
 3 files changed, 8 insertions(+), 7 deletions(-)

Comments

Junio C Hamano Oct. 12, 2018, 1:45 p.m. UTC | #1
Luke Diamand <luke@diamand.org> writes:

> The branch detection code looks for branches under refs/remotes/p4/...
> and can end up getting confused if there are unshelved changes in
> there as well. This happens in the function p4BranchesInGit().
>
> Instead, put the unshelved changes into refs/remotes/p4-unshelved/<N>.

I am not a p4 user (and not a git-p4 user), so it is a bit hard for
me to assess if this is a backward incompatibile change and if so
how serious potential breakage to existing users would be.

>  
> -If the target branch in refs/remotes/p4/unshelved already exists, the old one will
> +If the target branch in refs/remotes/p4-unshelved already exists, the old one will
>  be renamed.
>  
>  ----
>  $ git p4 sync
>  $ git p4 unshelve 12345
> -$ git show refs/remotes/p4/unshelved/12345
> +$ git show p4/unshelved/12345

Isn't this "p4-unshelved/12345" now?
Luke Diamand Oct. 12, 2018, 6:19 p.m. UTC | #2
On Fri, 12 Oct 2018 at 14:45, Junio C Hamano <gitster@pobox.com> wrote:
>
> Luke Diamand <luke@diamand.org> writes:
>
> > The branch detection code looks for branches under refs/remotes/p4/...
> > and can end up getting confused if there are unshelved changes in
> > there as well. This happens in the function p4BranchesInGit().
> >
> > Instead, put the unshelved changes into refs/remotes/p4-unshelved/<N>.
>
> I am not a p4 user (and not a git-p4 user), so it is a bit hard for
> me to assess if this is a backward incompatibile change and if so
> how serious potential breakage to existing users would be.

I don't think it's a particularly serious breakage - it reports the
branch it unshelves to, so it should be fairly obvious.

However, maybe it would make sense to pull this into a separate commit
to make it more obvious? I should have thought of that before
submitting.

>
> >
> > -If the target branch in refs/remotes/p4/unshelved already exists, the old one will
> > +If the target branch in refs/remotes/p4-unshelved already exists, the old one will
> >  be renamed.
> >
> >  ----
> >  $ git p4 sync
> >  $ git p4 unshelve 12345
> > -$ git show refs/remotes/p4/unshelved/12345
> > +$ git show p4/unshelved/12345
>
> Isn't this "p4-unshelved/12345" now?

Yes, I think another reason to pull into a separate commit.

Luke

>
Luke Diamand Oct. 15, 2018, 11:03 a.m. UTC | #3
On Fri, 12 Oct 2018 at 19:19, Luke Diamand <luke@diamand.org> wrote:
>
> On Fri, 12 Oct 2018 at 14:45, Junio C Hamano <gitster@pobox.com> wrote:
> >
> > Luke Diamand <luke@diamand.org> writes:
> >
> > > The branch detection code looks for branches under refs/remotes/p4/...
> > > and can end up getting confused if there are unshelved changes in
> > > there as well. This happens in the function p4BranchesInGit().
> > >
> > > Instead, put the unshelved changes into refs/remotes/p4-unshelved/<N>.
> >
> > I am not a p4 user (and not a git-p4 user), so it is a bit hard for
> > me to assess if this is a backward incompatibile change and if so
> > how serious potential breakage to existing users would be.
>
> I don't think it's a particularly serious breakage - it reports the
> branch it unshelves to, so it should be fairly obvious.
>
> However, maybe it would make sense to pull this into a separate commit
> to make it more obvious? I should have thought of that before
> submitting.
>
> >
> > >
> > > -If the target branch in refs/remotes/p4/unshelved already exists, the old one will
> > > +If the target branch in refs/remotes/p4-unshelved already exists, the old one will
> > >  be renamed.
> > >
> > >  ----
> > >  $ git p4 sync
> > >  $ git p4 unshelve 12345
> > > -$ git show refs/remotes/p4/unshelved/12345
> > > +$ git show p4/unshelved/12345
> >
> > Isn't this "p4-unshelved/12345" now?
>
> Yes, I think another reason to pull into a separate commit.

D'oh. It's already in a separate commit.
I'll re-roll fixing that documentation.

I think it will be fine to change the branch that the unshelving
happens into - I think it's very unlikely anyone is basing some
automated scripts on this, because of the way that unshelving is used
anyway.

Luke

Patch
diff mbox series

diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
index 41780a5aa9..c7705ae6e7 100644
--- a/Documentation/git-p4.txt
+++ b/Documentation/git-p4.txt
@@ -174,7 +174,7 @@  $ git p4 submit --update-shelve 1234 --update-shelve 2345
 Unshelve
 ~~~~~~~~
 Unshelving will take a shelved P4 changelist, and produce the equivalent git commit
-in the branch refs/remotes/p4/unshelved/<changelist>.
+in the branch refs/remotes/p4-unshelved/<changelist>.
 
 The git commit is created relative to the current origin revision (HEAD by default).
 If the shelved changelist's parent revisions differ, git-p4 will refuse to unshelve;
@@ -182,13 +182,13 @@  you need to be unshelving onto an equivalent tree.
 
 The origin revision can be changed with the "--origin" option.
 
-If the target branch in refs/remotes/p4/unshelved already exists, the old one will
+If the target branch in refs/remotes/p4-unshelved already exists, the old one will
 be renamed.
 
 ----
 $ git p4 sync
 $ git p4 unshelve 12345
-$ git show refs/remotes/p4/unshelved/12345
+$ git show p4/unshelved/12345
 <submit more changes via p4 to the same files>
 $ git p4 unshelve 12345
 <refuses to unshelve until git is in sync with p4 again>
diff --git a/git-p4.py b/git-p4.py
index 5701bad06a..76c18a22e9 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -3956,7 +3956,8 @@  def __init__(self):
         ]
         self.verbose = False
         self.noCommit = False
-        self.destbranch = "refs/remotes/p4/unshelved"
+        self.destbranch = "refs/remotes/p4-unshelved"
+        self.origin = "p4/master"
 
     def renameBranch(self, branch_name):
         """ Rename the existing branch to branch_name.N
diff --git a/t/t9832-unshelve.sh b/t/t9832-unshelve.sh
index 48ec7679b8..c3d15ceea8 100755
--- a/t/t9832-unshelve.sh
+++ b/t/t9832-unshelve.sh
@@ -54,8 +54,8 @@  EOF
 		cd "$git" &&
 		change=$(last_shelved_change) &&
 		git p4 unshelve $change &&
-		git show refs/remotes/p4/unshelved/$change | grep -q "Further description" &&
-		git cherry-pick refs/remotes/p4/unshelved/$change &&
+		git show refs/remotes/p4-unshelved/$change | grep -q "Further description" &&
+		git cherry-pick refs/remotes/p4-unshelved/$change &&
 		test_path_is_file file2 &&
 		test_cmp file1 "$cli"/file1 &&
 		test_cmp file2 "$cli"/file2 &&
@@ -88,7 +88,7 @@  EOF
 		cd "$git" &&
 		change=$(last_shelved_change) &&
 		git p4 unshelve $change &&
-		git diff refs/remotes/p4/unshelved/$change.0 refs/remotes/p4/unshelved/$change | grep -q file3
+		git diff refs/remotes/p4-unshelved/$change.0 refs/remotes/p4-unshelved/$change | grep -q file3
 	)
 '