[PATCHv1 0/2] git-p4 patches

[PATCHv1 0/2] git-p4 patches

[Luke Diamand]

This is a couple of small patches for git-p4.

The first just teaches git-p4 about git workspaces, so that you can
do "git p4 submit" from within a workspace (P.S. workspaces
are completely *awesome*).

The second follows on from the work by Vinicius for shelving
changelists, by letting you update an existing shelved changelist.

This is a bit like using "git commit --amend" only far more painful....

Luke Diamand (2):
  git-p4: support git-workspaces
  git-p4: support updating an existing shelved changelist

 Documentation/git-p4.txt |  4 ++++
 git-p4.py                | 39 +++++++++++++++++++++++++++++++++++----
 t/t9807-git-p4-submit.sh | 38 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 77 insertions(+), 4 deletions(-)

-- 
2.11.0.274.g0ea315c

[PATCHv1 1/2] git-p4: support git-workspaces

[Luke Diamand]

Teach git-p4 about git-workspaces.

Signed-off-by: Luke Diamand <luke@diamand.org>
---
 git-p4.py | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/git-p4.py b/git-p4.py
index 0c4f2afd2..5e2db1919 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -566,6 +566,12 @@ def isValidGitDir(path):
     if (os.path.exists(path + "/HEAD")
         and os.path.exists(path + "/refs") and os.path.exists(path + "/objects")):
         return True;
+
+    # git workspace directory?
+    if (os.path.exists(path + "/HEAD")
+        and os.path.exists(path + "/gitdir")):
+        return True
+
     return False
 
 def parseRevision(ref):
-- 
2.11.0.274.g0ea315c

Re: [PATCHv1 1/2] git-p4: support git-workspaces

[Junio C Hamano]

Luke Diamand <luke@diamand.org> writes:

> Teach git-p4 about git-workspaces.

Is this what we call "git worktree", or something else?

>
> Signed-off-by: Luke Diamand <luke@diamand.org>
> ---
>  git-p4.py | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/git-p4.py b/git-p4.py
> index 0c4f2afd2..5e2db1919 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -566,6 +566,12 @@ def isValidGitDir(path):
>      if (os.path.exists(path + "/HEAD")
>          and os.path.exists(path + "/refs") and os.path.exists(path + "/objects")):
>          return True;
> +
> +    # git workspace directory?
> +    if (os.path.exists(path + "/HEAD")
> +        and os.path.exists(path + "/gitdir")):
> +        return True
> +
>      return False
>  
>  def parseRevision(ref):

Re: [PATCHv1 1/2] git-p4: support git-workspaces

[Luke Diamand]

On 5 December 2016 at 20:53, Junio C Hamano <gitster@pobox.com> wrote:
> Luke Diamand <luke@diamand.org> writes:
>
>> Teach git-p4 about git-workspaces.
>
> Is this what we call "git worktree", or something else?

Ah, I think you're right!

>
>>
>> Signed-off-by: Luke Diamand <luke@diamand.org>
>> ---
>>  git-p4.py | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/git-p4.py b/git-p4.py
>> index 0c4f2afd2..5e2db1919 100755
>> --- a/git-p4.py
>> +++ b/git-p4.py
>> @@ -566,6 +566,12 @@ def isValidGitDir(path):
>>      if (os.path.exists(path + "/HEAD")
>>          and os.path.exists(path + "/refs") and os.path.exists(path + "/objects")):
>>          return True;
>> +
>> +    # git workspace directory?
>> +    if (os.path.exists(path + "/HEAD")
>> +        and os.path.exists(path + "/gitdir")):
>> +        return True
>> +
>>      return False
>>
>>  def parseRevision(ref):

Re: [PATCHv1 1/2] git-p4: support git-workspaces

[Junio C Hamano]

Luke Diamand <luke@diamand.org> writes:

> On 5 December 2016 at 20:53, Junio C Hamano <gitster@pobox.com> wrote:
>> Luke Diamand <luke@diamand.org> writes:
>>
>>> Teach git-p4 about git-workspaces.
>>
>> Is this what we call "git worktree", or something else?
>
> Ah, I think you're right!

Then I'll queue it like the attached.

HOWEVER.

How fast does isValidGitDir() function need to be?  The primary one
seems to check HEAD (but it does not notice a non-repository that
has a directory with that name), refs and objects (but it does not
notice a non-repository that has a non-directory with these names),
and this new one uses a test that is even more sloppy.

What I am trying to get at is if we want to use a single command
that can be given a path and answer "Yes, that is a repository"
here, and that single command should know how the repository should
look like.  I offhand do not know we already have such a command we
can use, e.g. "git rev-parse --is-git-dir $path", but if there isn't
perhaps we would want one, so that not just "git p4" but other
scripted Porcelains can make use of it?

-- >8 --
From: Luke Diamand <luke@diamand.org>
Date: Fri, 2 Dec 2016 22:43:18 +0000
Subject: [PATCH] git-p4: support secondary working trees managed by "git worktree"

Teach git-p4 about them.

Signed-off-by: Luke Diamand <luke@diamand.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 git-p4.py | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/git-p4.py b/git-p4.py
index fd5ca52462..b3c50ae7e5 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -566,6 +566,12 @@ def isValidGitDir(path):
     if (os.path.exists(path + "/HEAD")
         and os.path.exists(path + "/refs") and os.path.exists(path + "/objects")):
         return True;
+
+    # secondary working tree managed by "git worktree"?
+    if (os.path.exists(path + "/HEAD")
+        and os.path.exists(path + "/gitdir")):
+        return True
+
     return False
 
 def parseRevision(ref):
-- 
2.11.0-222-g22b1346184

Re: [PATCHv1 1/2] git-p4: support git-workspaces

[Junio C Hamano]

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

> Luke Diamand <luke@diamand.org> writes:
> ...
>      if (os.path.exists(path + "/HEAD")
>          and os.path.exists(path + "/refs") and os.path.exists(path + "/objects")):
>          return True;
> +
> +    # secondary working tree managed by "git worktree"?
> +    if (os.path.exists(path + "/HEAD")
> +        and os.path.exists(path + "/gitdir")):
> +        return True
> +
>      return False

Wait a bit.  How is this expected to work?

I assume "path" is something like "$somewhere/.git" (either
absolute or relative), and by concatenating HEAD, refs and objects
under it the original checks form paths to expected directories,
because "$somewhere/.git" is a directory.

A worktree created with "git worktree add" gets ".git" that is a
file, and the contents of the file records the path to a
subdirectory "worktrees/$name" of the primary repository the
worktree is borrowing from, i.e. "$somewhere/.git/worktrees/$name".

So for this patch to work correctly, the caller, when in a "git
worktree" managed secondary working tree, MUST have found the ".git"
file at the root level of the working tree, MUST have read its
contents to learn that "$somewhere/.git/worktrees/$name" path, and
then feeding it to this function.  But doesn't such a caller already
know that it is a valid repository?  After all, it trusted the
contents of that ".git" file at the root level.

    ... goes and looks at the callers ...

Between the two call sites of isValidGitDir() in main(), the first
one (i.e. if ".git" in the current directory, made into abspath,
does not look like a repository, ask "rev-parse --git-dir" for one)
looks correct and I think it would grab the correct $GIT_DIR just
fine [*1*].  Isn't the real problem the second one, i.e. the one
that feeds a correct cmd.gitdir that we just obtained by asking
"rev-parse --git-dir" to the sloppy isValidGitDir() again.  The
latter second guesses "rev-parse --git-dir" and botches.

I have a feeling that adding more to isValidGitDir() like this patch
does is a step in the wrong direction.  The first fix would be not
to do the "if isValidGitDir() says no, then do something else" step
if you already asked "rev-parse --git-dir" and got a valid $GIT_DIR,
perhaps?

Stepping back even more.  

I wonder why the script needs to do os.environ.get("GIT_DIR") in the
first place to initialize cmd.gitdir field.  If it instead asked
"rev-parse --gitdir", the script would get the right answer that
already takes GIT_DIR environment into account.


[Footnote]

*1* This ends up asking "rev-parse --git-dir" only because
    isValidGitDir() is sloppy.  If you are in a secondary working
    tree whose ".git" is a file, the function would say "no", and
    that is the only thing that allows you to make a call to
    "rev-parse --git-dir" to obtain the correct result.

Re: [PATCHv1 1/2] git-p4: support git-workspaces

[Luke Diamand]

On 5 December 2016 at 21:24, Junio C Hamano <gitster@pobox.com> wrote:
> Luke Diamand <luke@diamand.org> writes:
>
>> On 5 December 2016 at 20:53, Junio C Hamano <gitster@pobox.com> wrote:
>>> Luke Diamand <luke@diamand.org> writes:
>>>
>>>> Teach git-p4 about git-workspaces.
>>>
>>> Is this what we call "git worktree", or something else?
>>
>> Ah, I think you're right!
>
> Then I'll queue it like the attached.
>
> HOWEVER.
>
> How fast does isValidGitDir() function need to be?

It doesn't need to be fast.

> The primary one
> seems to check HEAD (but it does not notice a non-repository that
> has a directory with that name), refs and objects (but it does not
> notice a non-repository that has a non-directory with these names),
> and this new one uses a test that is even more sloppy.
>
> What I am trying to get at is if we want to use a single command
> that can be given a path and answer "Yes, that is a repository"
> here, and that single command should know how the repository should
> look like.  I offhand do not know we already have such a command we
> can use, e.g. "git rev-parse --is-git-dir $path", but if there isn't
> perhaps we would want one, so that not just "git p4" but other
> scripted Porcelains can make use of it?

That would be nicer than random ad-hoc rules, certainly. I couldn't
find any obvious combination that would work.

Luke

Re: [PATCHv1 1/2] git-p4: support git-workspaces

[Junio C Hamano]

Luke Diamand <luke@diamand.org> writes:

>> What I am trying to get at is if we want to use a single command
>> that can be given a path and answer "Yes, that is a repository"
>> here, and that single command should know how the repository should
>> look like.  I offhand do not know we already have such a command we
>> can use, e.g. "git rev-parse --is-git-dir $path", but if there isn't
>> perhaps we would want one, so that not just "git p4" but other
>> scripted Porcelains can make use of it?
>
> That would be nicer than random ad-hoc rules, certainly. I couldn't
> find any obvious combination that would work.

I've already sent an update but my reading of this code is that
there is no need for the program to be able to, given a random
directory path, ask isValidGitDir() "is this a Git repository?" at
all.  

What the program wanted to know, IIUC, is "Where is the 'Git
repository directory', if there is one?".  This is needed primarily
because the program wants to error out before doing any operation
that requires a Git repository to work on, when the answer is "there
is none".

It also wants to know it because (for whatever reason) it wants to
export it in GIT_DIR [*1*].

And isValidGitDir() is a helper function used by the handcrafted
logic in main() to answer that question, but as far as I can tell,
you'd get a more correct answer by asking "rev-parse --git-dir"
unconditionally (even when the user of the program exported GIT_DIR
to you).

I just was reading Lars's LFS changes, but that one has hardcoded
"Is there a '.git/lfs/objects/' directory directly inside the
current working directory?" and similar, which I do not think would
work well in a secondary working tree where ".git" is merely a file
[*2*].  In a "secondary working tree"-aware world, I think you would
need to ask locations of --git-dir and --git-common-dir to rev-parse
upfront and choose which ones are ought to be shared entities across
the family of worktrees and which ones are to be private per
worktree.  I suspect that LFS objects want to be shared across
working trees that share the object store, for example, which would
mean that "--git-dir" is not what Lars would want to use.


[Footnote]

*1* I do not think this is necessary.  Git tools know how to find
the repository by first checking GIT_DIR environment and if it is
not set then by walking up the directory hierarchy just fine without
the program exporting GIT_DIR for them.

*2* The part that bases this path relative to getcwd() is OK, as the
start-up sequence in main() does a cdup to the top before everything
else.

[PATCHv1 2/2] git-p4: support updating an existing shelved changelist

[Luke Diamand]

Adds new option "--update-shelve CHANGELIST" which updates
an existing shelved changelist.

The original changelist must have been created by the current user.

This allows workflow something like:

   hack hack hack
   git commit
   git p4 submit --shelve
   $mail interested parties about shelved changelist
   make corrections
   git commit --amend
   git p4 submit --update-shelve $CHANGELIST
   $mail interested parties about shelved changelist
   etc

Signed-off-by: Luke Diamand <luke@diamand.org>
---
 Documentation/git-p4.txt |  4 ++++
 git-p4.py                | 33 +++++++++++++++++++++++++++++----
 t/t9807-git-p4-submit.sh | 38 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 71 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
index 1bbf43d15..ce40b9a54 100644
--- a/Documentation/git-p4.txt
+++ b/Documentation/git-p4.txt
@@ -308,6 +308,10 @@ These options can be used to modify 'git p4 submit' behavior.
 	After creating each shelve, the relevant files are reverted/deleted.
 	If you have multiple commits pending multiple shelves will be created.
 
+--update-shelve CHANGELIST::
+	Update an existing shelved changelist with this commit. Implies
+	--shelve.
+
 --conflict=(ask|skip|quit)::
 	Conflicts can occur when applying a commit to p4.  When this
 	happens, the default behavior ("ask") is to prompt whether to
diff --git a/git-p4.py b/git-p4.py
index 5e2db1919..36242d384 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -262,6 +262,10 @@ def p4_revert(f):
 def p4_reopen(type, f):
     p4_system(["reopen", "-t", type, wildcard_encode(f)])
 
+def p4_reopen_in_change(changelist, files):
+    cmd = ["reopen", "-c", str(changelist)] + files
+    p4_system(cmd)
+
 def p4_move(src, dest):
     p4_system(["move", "-k", wildcard_encode(src), wildcard_encode(dest)])
 
@@ -1298,6 +1302,9 @@ class P4Submit(Command, P4UserMap):
                 optparse.make_option("--shelve", dest="shelve", action="store_true",
                                      help="Shelve instead of submit. Shelved files are reverted, "
                                      "restoring the workspace to the state before the shelve"),
+                optparse.make_option("--update-shelve", dest="update_shelve", action="store", type="int",
+                                     metavar="CHANGELIST",
+                                     help="update an existing shelved changelist, implies --shelve")
         ]
         self.description = "Submit changes from git to the perforce depot."
         self.usage += " [name of git branch to submit into perforce depot]"
@@ -1306,6 +1313,7 @@ class P4Submit(Command, P4UserMap):
         self.preserveUser = gitConfigBool("git-p4.preserveUser")
         self.dry_run = False
         self.shelve = False
+        self.update_shelve = None
         self.prepare_p4_only = False
         self.conflict_behavior = None
         self.isWindows = (platform.system() == "Windows")
@@ -1474,7 +1482,7 @@ class P4Submit(Command, P4UserMap):
                     return 1
         return 0
 
-    def prepareSubmitTemplate(self):
+    def prepareSubmitTemplate(self, changelist=None):
         """Run "p4 change -o" to grab a change specification template.
            This does not use "p4 -G", as it is nice to keep the submission
            template in original order, since a human might edit it.
@@ -1486,7 +1494,11 @@ class P4Submit(Command, P4UserMap):
 
         template = ""
         inFilesSection = False
-        for line in p4_read_pipe_lines(['change', '-o']):
+        args = ['change', '-o']
+        if changelist:
+            args.append(str(changelist))
+
+        for line in p4_read_pipe_lines(args):
             if line.endswith("\r\n"):
                 line = line[:-2] + "\n"
             if inFilesSection:
@@ -1585,11 +1597,14 @@ class P4Submit(Command, P4UserMap):
         editedFiles = set()
         pureRenameCopy = set()
         filesToChangeExecBit = {}
+        all_files = list()
 
         for line in diff:
             diff = parseDiffTreeEntry(line)
             modifier = diff['status']
             path = diff['src']
+            all_files.append(path)
+
             if modifier == "M":
                 p4_edit(path)
                 if isModeExecChanged(diff['src_mode'], diff['dst_mode']):
@@ -1715,6 +1730,10 @@ class P4Submit(Command, P4UserMap):
             mode = filesToChangeExecBit[f]
             setP4ExecBit(f, mode)
 
+        if self.update_shelve:
+            print("all_files = %s" % str(all_files))
+            p4_reopen_in_change(self.update_shelve, all_files)
+
         #
         # Build p4 change description, starting with the contents
         # of the git commit message.
@@ -1723,7 +1742,7 @@ class P4Submit(Command, P4UserMap):
         logMessage = logMessage.strip()
         (logMessage, jobs) = self.separate_jobs_from_description(logMessage)
 
-        template = self.prepareSubmitTemplate()
+        template = self.prepareSubmitTemplate(self.update_shelve)
         submitTemplate = self.prepareLogMessage(template, logMessage, jobs)
 
         if self.preserveUser:
@@ -1795,7 +1814,10 @@ class P4Submit(Command, P4UserMap):
                 if self.isWindows:
                     message = message.replace("\r\n", "\n")
                 submitTemplate = message[:message.index(separatorLine)]
-                if self.shelve:
+
+                if self.update_shelve:
+                    p4_write_pipe(['shelve', '-r', '-i'], submitTemplate)
+                elif self.shelve:
                     p4_write_pipe(['shelve', '-i'], submitTemplate)
                 else:
                     p4_write_pipe(['submit', '-i'], submitTemplate)
@@ -1921,6 +1943,9 @@ class P4Submit(Command, P4UserMap):
         if len(self.origin) == 0:
             self.origin = upstream
 
+        if self.update_shelve:
+            self.shelve = True
+
         if self.preserveUser:
             if not self.canChangeChangelists():
                 die("Cannot preserve user names without p4 super-user or admin permissions")
diff --git a/t/t9807-git-p4-submit.sh b/t/t9807-git-p4-submit.sh
index 42a5fada5..e37239e65 100755
--- a/t/t9807-git-p4-submit.sh
+++ b/t/t9807-git-p4-submit.sh
@@ -444,6 +444,44 @@ test_expect_success 'submit --shelve' '
 	)
 '
 
+# Update an existing shelved changelist
+
+test_expect_success 'submit --update-shelve' '
+	test_when_finished cleanup_git &&
+	git p4 clone --dest="$git" //depot &&
+	(
+		cd "$cli" &&
+		p4 revert ... &&
+		cd "$git" &&
+		git config git-p4.skipSubmitEdit true &&
+		test_commit "test-update-shelved-change" &&
+		git p4 submit --origin=HEAD^ --shelve &&
+
+		shelf_cl=$(p4 -G changes -s shelved -m 1 |\
+			marshal_dump change) &&
+		test -n $shelf_cl &&
+		echo "updating shelved change list $shelf_cl" &&
+
+		echo "updated-line" >>shelf.t &&
+		echo added-file.t >added-file.t &&
+		git add shelf.t added-file.t &&
+		git rm -f test-update-shelved-change.t &&
+		git commit --amend -C HEAD &&
+		git show --stat HEAD &&
+		git p4 submit -v --origin HEAD^ --update-shelve $shelf_cl &&
+		echo "done git p4 submit"
+	) &&
+	(
+		cd "$cli" &&
+		change=$(p4 -G changes -s shelved -m 1 //depot/... | \
+			 marshal_dump change) &&
+		p4 unshelve -c $change -s $change &&
+		grep -q updated-line shelf.t &&
+		p4 describe -S $change | grep added-file.t &&
+		test_path_is_missing test-update-shelved-change.t
+	)
+'
+
 test_expect_success 'kill p4d' '
 	kill_p4d
 '
-- 
2.11.0.274.g0ea315c