threads / patch / 29414

v2, 3 partsgit-p4: Add checkpoint() task

Subject: [PATCH v2 1/3] git-p4: Add checkpoint() task

## tl;dr

15 messages between Jan 21, 2012 and Jan 25, 2012. Diffs are folded; open one to read it.

replies: 14people: 4as markdown or json

Vitor Antunes· Jan 21, 2012, 00:21 UTC · lore

[PATCH v2 0/3] git-p4: Search for parent commit on branch creation

The new version of these patches includes the improvement suggestions from Pete Wyckoff:

 - Use os.path.join().
 - Use git update-ref -d to remove refs.
 - Read $GIT_DIR environment variable.
 - Optimize comparison cycle to avoid excessive use of checkpoint
   command.
Vitor Antunes (3):
  git-p4: Add checkpoint() task
  git-p4: Search for parent commit on branch creation
  git-p4: Add test case for complex branch import
 contrib/fast-import/git-p4 |   38 ++++++++++++++++++++-
 t/t9801-git-p4-branch.sh   |   80 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 117 insertions(+), 1 deletions(-)
-- 
1.7.7.rc2.14.g5e044.dirty
Vitor Antunes· Jan 21, 2012, 00:21 UTC · re: Vitor Antunes · lore
Signed-off-by: Vitor Antunes <vitor.hda@gmail.com>
---
 contrib/fast-import/git-p4 |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)
Show changes to contrib/fast-import/git-p4 +8 −0
diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index 3e1aa27..417d119 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -1450,6 +1450,14 @@ class P4Sync(Command, P4UserMap):
                    .replace("%25", "%")
         return path
 
+    # Force a checkpoint in fast-import and wait for it to finish
+    def checkpoint(self):
+        self.gitStream.write("checkpoint\n\n")
+        self.gitStream.write("progress checkpoint\n\n")
+        out = self.gitOutput.readline()
+        if self.verbose:
+            print "checkpoint finished: " + out
+
     def extractFilesFromCommit(self, commit):
         self.cloneExclude = [re.sub(r"\.\.\.$", "", path)
                              for path in self.cloneExclude]

-- 
1.7.7.rc2.14.g5e044.dirty
Vitor Antunes· Jan 21, 2012, 00:21 UTC · re: Vitor Antunes · lore

[PATCH v2 2/3] git-p4: Search for parent commit on branch creation

To find out which is its parent the commit of the new branch is applied sequentially to each blob of the parent branch from the newest to the oldest. The first blob which results in a zero diff is considered the parent commit. If none is found, then the commit is applied to the top of the parent branch.

A fast-import "checkpoint" call is required for each comparison because diff-tree is only able to work with blobs on disk. But most of these commits will not be part of the final imported tree, making fast-import fail. To avoid this, the temporary branches are tracked and then removed at the end of the import process.

Signed-off-by: Vitor Antunes <vitor.hda@gmail.com>
---
 contrib/fast-import/git-p4 |   30 +++++++++++++++++++++++++++++-
 1 files changed, 29 insertions(+), 1 deletions(-)
Show changes to contrib/fast-import/git-p4 +29 −1
diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index 417d119..2e3b741 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -1429,6 +1429,8 @@ class P4Sync(Command, P4UserMap):
         self.cloneExclude = []
         self.useClientSpec = False
         self.clientSpecDirs = None
+        self.tempBranches = []
+        self.tempBranchLocation = "git-p4-tmp"
 
         if gitConfig("git-p4.syncFromOrigin") == "false":
             self.syncWithOrigin = False
@@ -2012,7 +2014,27 @@ class P4Sync(Command, P4UserMap):
                             parent = self.initialParents[branch]
                             del self.initialParents[branch]
 
-                        self.commit(description, filesForCommit, branch, [branchPrefix], parent)
+                        parentFound = False
+                        if len(parent) > 0:
+                            tempBranch = os.path.join(self.tempBranchLocation, "%d" % (change))
+                            if self.verbose:
+                                print "Creating temporary branch: " + tempBranch
+                            self.commit(description, filesForCommit, tempBranch, [branchPrefix])
+                            self.tempBranches.append(tempBranch)
+                            self.checkpoint()
+                            for blob in read_pipe_lines("git rev-list --reverse --no-merges %s" % parent):
+                                blob = blob.strip()
+                                if len( read_pipe("git diff-tree %s %s" % (blob, tempBranch)) ) == 0:
+                                    parentFound = True
+                                    if self.verbose:
+                                        print "Found parent of %s in commit %s" % (branch, blob)
+                                    break
+                        if parentFound:
+                            self.commit(description, filesForCommit, branch, [branchPrefix], blob)
+                        else:
+                            if self.verbose:
+                                print "Parent of %s not found. Committing into head of %s" % (branch, parent)
+                            self.commit(description, filesForCommit, branch, [branchPrefix], parent)
                 else:
                     files = self.extractFilesFromCommit(description)
                     self.commit(description, files, self.branch, self.depotPaths,
@@ -2347,6 +2369,12 @@ class P4Sync(Command, P4UserMap):
         self.gitOutput.close()
         self.gitError.close()
 
+        # Cleanup temporary branches created during import
+        if self.tempBranches != []:
+            for branch in self.tempBranches:
+                read_pipe("git update-ref -d %s" % branch)
+            os.rmdir(os.path.join(os.environ.get("GIT_DIR", ".git"), self.tempBranchLocation))
+
         return True
 
 class P4Rebase(Command):

-- 
1.7.7.rc2.14.g5e044.dirty
Junio C Hamano· Jan 21, 2012, 04:55 UTC · re: Vitor Antunes · lore

Re: [PATCH v2 2/3] git-p4: Search for parent commit on branch creation

Vitor Antunes <vitor.hda@gmail.com> writes:
Show 8 quoted lines
> A fast-import "checkpoint" call is required for each comparison because
> diff-tree is only able to work with blobs on disk. But most of these
> commits will not be part of the final imported tree, making fast-import
> fail. To avoid this, the temporary branches are tracked and then removed
> at the end of the import process.
>
> Signed-off-by: Vitor Antunes <vitor.hda@gmail.com>
> ---

It might make sense to squash 1/3 with this patch; the definition alone without any actual user would not be very useful and won't help hunting for breakages, if exists, in the future.

Show 19 quoted lines
> @@ -2012,7 +2014,27 @@ class P4Sync(Command, P4UserMap):
>                              parent = self.initialParents[branch]
>                              del self.initialParents[branch]
>  
> -                        self.commit(description, filesForCommit, branch, [branchPrefix], parent)
> +                        parentFound = False
> +                        if len(parent) > 0:
> +                            tempBranch = os.path.join(self.tempBranchLocation, "%d" % (change))
> +                            if self.verbose:
> +                                print "Creating temporary branch: " + tempBranch
> +                            self.commit(description, filesForCommit, tempBranch, [branchPrefix])
> +                            self.tempBranches.append(tempBranch)
> +                            self.checkpoint()
> +                            for blob in read_pipe_lines("git rev-list --reverse --no-merges %s" % parent):
> +                                blob = blob.strip()
> +                                if len( read_pipe("git diff-tree %s %s" % (blob, tempBranch)) ) == 0:
> +                                    parentFound = True
> +                                    if self.verbose:
> +                                        print "Found parent of %s in commit %s" % (branch, blob)

... also this looks excessively deeply nested, which is a sign that it might be a good idea to refactor this part into a separate helper function or something.

Vitor Antunes· Jan 23, 2012, 13:49 UTC · re: Junio C Hamano · lore

Re: [PATCH v2 2/3] git-p4: Search for parent commit on branch creation

Hi Junio,
On Sat, Jan 21, 2012 at 4:55 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 14 quoted lines
> Vitor Antunes <vitor.hda@gmail.com> writes:
>
>> A fast-import "checkpoint" call is required for each comparison because
>> diff-tree is only able to work with blobs on disk. But most of these
>> commits will not be part of the final imported tree, making fast-import
>> fail. To avoid this, the temporary branches are tracked and then removed
>> at the end of the import process.
>>
>> Signed-off-by: Vitor Antunes <vitor.hda@gmail.com>
>> ---
>
> It might make sense to squash 1/3 with this patch; the definition alone
> without any actual user would not be very useful and won't help hunting
> for breakages, if exists, in the future.
Makes sense. WIll correct as you suggest in v3.
Show 23 quoted lines
>> @@ -2012,7 +2014,27 @@ class P4Sync(Command, P4UserMap):
>>                              parent = self.initialParents[branch]
>>                              del self.initialParents[branch]
>>
>> -                        self.commit(description, filesForCommit, branch, [branchPrefix], parent)
>> +                        parentFound = False
>> +                        if len(parent) > 0:
>> +                            tempBranch = os.path.join(self.tempBranchLocation, "%d" % (change))
>> +                            if self.verbose:
>> +                                print "Creating temporary branch: " + tempBranch
>> +                            self.commit(description, filesForCommit, tempBranch, [branchPrefix])
>> +                            self.tempBranches.append(tempBranch)
>> +                            self.checkpoint()
>> +                            for blob in read_pipe_lines("git rev-list --reverse --no-merges %s" % parent):
>> +                                blob = blob.strip()
>> +                                if len( read_pipe("git diff-tree %s %s" % (blob, tempBranch)) ) == 0:
>> +                                    parentFound = True
>> +                                    if self.verbose:
>> +                                        print "Found parent of %s in commit %s" % (branch, blob)
>
> ... also this looks excessively deeply nested, which is a sign that it
> might be a good idea to refactor this part into a separate helper function
> or something.

I have to agree with you. Since v3 is required, I'll also include this fix along.

Thanks, Vitor

Vitor Antunes· Jan 21, 2012, 00:21 UTC · re: Vitor Antunes · lore

[PATCH v2 3/3] git-p4: Add test case for complex branch import

Check if branches created from old changelists are correctly imported.
Signed-off-by: Vitor Antunes <vitor.hda@gmail.com>
---
 t/t9801-git-p4-branch.sh |   80 ++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 80 insertions(+), 0 deletions(-)
Show changes to t/t9801-git-p4-branch.sh +80 −0
diff --git a/t/t9801-git-p4-branch.sh b/t/t9801-git-p4-branch.sh
index a25f18d..ed4e48c 100755
--- a/t/t9801-git-p4-branch.sh
+++ b/t/t9801-git-p4-branch.sh
@@ -226,6 +226,86 @@ test_expect_success 'git-p4 clone simple branches' '
 	)
 '
 
+# Create a complex branch structure in P4 depot to check if they are correctly
+# cloned. The branches are created from older changelists to check if git-p4 is
+# able to correctly detect them.
+# The final expected structure is:
+# `branch1
+# | `- file1
+# | `- file2 (updated)
+# | `- file3
+# `branch2
+# | `- file1
+# | `- file2
+# `branch3
+# | `- file1
+# | `- file2 (updated)
+# | `- file3
+# `branch4
+# | `- file1
+# | `- file2
+# `branch5
+#   `- file1
+#   `- file2
+#   `- file3
+test_expect_success 'git-p4 add complex branches' '
+	test_when_finished cleanup_git &&
+	test_create_repo "$git" &&
+	(
+		cd "$cli" &&
+		changelist=$(p4 changes -m1 //depot/... | cut -d" " -f2) &&
+		changelist=$(($changelist - 5)) &&
+		p4 integrate //depot/branch1/...@$changelist //depot/branch4/... &&
+		p4 submit -d "branch4" &&
+		changelist=$(($changelist + 2)) &&
+		p4 integrate //depot/branch1/...@$changelist //depot/branch5/... &&
+		p4 submit -d "branch5"
+	)
+'
+
+# Configure branches through git-config and clone them. git-p4 will only be able
+# to clone the original structure if it is able to detect the origin changelist
+# of each branch.
+test_expect_success 'git-p4 clone complex branches' '
+	test_when_finished cleanup_git &&
+	test_create_repo "$git" &&
+	(
+		cd "$git" &&
+		git config git-p4.branchList branch1:branch2 &&
+		git config --add git-p4.branchList branch1:branch3 &&
+		git config --add git-p4.branchList branch1:branch4 &&
+		git config --add git-p4.branchList branch1:branch5 &&
+		"$GITP4" clone --dest=. --detect-branches //depot@all &&
+		git log --all --graph --decorate --stat &&
+		git reset --hard p4/depot/branch1 &&
+		test_path_is_file file1 &&
+		test_path_is_file file2 &&
+		test_path_is_file file3 &&
+		grep -q update file2 &&
+		git reset --hard p4/depot/branch2 &&
+		test_path_is_file file1 &&
+		test_path_is_file file2 &&
+		test_path_is_missing file3 &&
+		! grep -q update file2 &&
+		git reset --hard p4/depot/branch3 &&
+		test_path_is_file file1 &&
+		test_path_is_file file2 &&
+		test_path_is_file file3 &&
+		grep -q update file2 &&
+		git reset --hard p4/depot/branch4 &&
+		test_path_is_file file1 &&
+		test_path_is_file file2 &&
+		test_path_is_missing file3 &&
+		! grep -q update file2 &&
+		git reset --hard p4/depot/branch5 &&
+		test_path_is_file file1 &&
+		test_path_is_file file2 &&
+		test_path_is_file file3 &&
+		! grep -q update file2 &&
+		test_path_is_missing .git/git-p4-tmp
+	)
+'
+
 test_expect_success 'kill p4d' '
 	kill_p4d
 '

-- 
1.7.7.rc2.14.g5e044.dirty
Junio C Hamano· Jan 21, 2012, 04:54 UTC · re: Vitor Antunes · lore

Re: [PATCH v2 3/3] git-p4: Add test case for complex branch import

Vitor Antunes <vitor.hda@gmail.com> writes:
Show 12 quoted lines
> +test_expect_success 'git-p4 add complex branches' '
> +	test_when_finished cleanup_git &&
> +	test_create_repo "$git" &&
> +	(
> +		cd "$cli" &&
> +		changelist=$(p4 changes -m1 //depot/... | cut -d" " -f2) &&
> +		changelist=$(($changelist - 5)) &&
> +		p4 integrate //depot/branch1/...@$changelist //depot/branch4/... &&
> +		p4 submit -d "branch4" &&
> +		changelist=$(($changelist + 2)) &&
> +		p4 integrate //depot/branch1/...@$changelist //depot/branch5/... &&
> +		p4 submit -d "branch5"

That's a strange quoting convention. Why are "branch4" and "branch5" enclosed in double quotes while "integrate" and "submit" aren't? (rhetorical: do not quote these branch names without a good reason).

Show 19 quoted lines
> +# Configure branches through git-config and clone them. git-p4 will only be able
> +# to clone the original structure if it is able to detect the origin changelist
> +# of each branch.
> +test_expect_success 'git-p4 clone complex branches' '
> +	test_when_finished cleanup_git &&
> +	test_create_repo "$git" &&
> +	(
> +		cd "$git" &&
> +		git config git-p4.branchList branch1:branch2 &&
> +		git config --add git-p4.branchList branch1:branch3 &&
> +		git config --add git-p4.branchList branch1:branch4 &&
> +		git config --add git-p4.branchList branch1:branch5 &&
> +		"$GITP4" clone --dest=. --detect-branches //depot@all &&
> +		git log --all --graph --decorate --stat &&
> +		git reset --hard p4/depot/branch1 &&
> +		test_path_is_file file1 &&
> +		test_path_is_file file2 &&
> +		test_path_is_file file3 &&
> +		grep -q update file2 &&

Do you really need to use "-q" here? Wouldn't it help if you wrote it without it while debugging tests with "sh ./t9801-*.sh -v"?

Also how does this series interact with the series Luke posted earlier on branches and labels?

Thanks.
Luke Diamand· Jan 21, 2012, 10:51 UTC · re: Junio C Hamano · lore

Re: [PATCH v2 3/3] git-p4: Add test case for complex branch import

On 21/01/12 04:54, Junio C Hamano wrote:
Show 9 quoted lines
> Vitor Antunes<vitor.hda@gmail.com>  writes:
>
>> +		grep -q update file2&&
>
> Do you really need to use "-q" here?  Wouldn't it help if you wrote it
> without it while debugging tests with "sh ./t9801-*.sh -v"?
>
> Also how does this series interact with the series Luke posted earlier on
> branches and labels?
Vitor's series applies cleanly to my changes.

However, one thing I noticed in reading through is that it will break if you end up importing a P4 branch that has spaces (or other shell chars) in its name. A quick test confirms this.

- the code doesn't handle the names properly
- git and p4 have different ideas about valid branch names

But before rejecting Vitor's changes because of that it would be worth considering whether we care (much). My own opinion is that if you have developers who are daft enough to put spaces or dollars in their branch names then their project is already doomed anyway....

Perhaps it would be enough just to issue a warning ("your project is doomed; start working on your CV") and skip such branch names rather than falling over with inexplicable error messages.

>
> Thanks.
Pete Wyckoff· Jan 21, 2012, 17:11 UTC · re: Luke Diamand · lore

Re: [PATCH v2 3/3] git-p4: Add test case for complex branch import

luke@diamand.org wrote on Sat, 21 Jan 2012 10:51 +0000:
Show 28 quoted lines
> On 21/01/12 04:54, Junio C Hamano wrote:
> >Vitor Antunes<vitor.hda@gmail.com>  writes:
> >
> >>+		grep -q update file2&&
> >
> >Do you really need to use "-q" here?  Wouldn't it help if you wrote it
> >without it while debugging tests with "sh ./t9801-*.sh -v"?
> >
> >Also how does this series interact with the series Luke posted earlier on
> >branches and labels?
> 
> Vitor's series applies cleanly to my changes.
> 
> However, one thing I noticed in reading through is that it will
> break if you end up importing a P4 branch that has spaces (or other
> shell chars) in its name. A quick test confirms this.
> 
> - the code doesn't handle the names properly
> - git and p4 have different ideas about valid branch names
> 
> But before rejecting Vitor's changes because of that it would be
> worth considering whether we care (much). My own opinion is that if
> you have developers who are daft enough to put spaces or dollars in
> their branch names then their project is already doomed anyway....
> 
> Perhaps it would be enough just to issue a warning ("your project is
> doomed; start working on your CV") and skip such branch names rather
> than falling over with inexplicable error messages.

This doesn't seem like a big deal. The read_pipe and read_pipe_lines calls shoud be list-ified. That gets rid of the problem with shell interactions.

For git branch name reserved characters, a little function to replace the bogus characters with "_" would avoid needing to go work on the resume. Anything in bad_ref_char() and check_refname_component(). I agree this doesn't have to be perfect.

This could be a new patch unrelated to Vitor's series, which verifies branch names anywhere a new commit is made.

		-- Pete
Vitor Antunes· Jan 23, 2012, 14:01 UTC · re: Pete Wyckoff · lore

Re: [PATCH v2 3/3] git-p4: Add test case for complex branch import

On Sat, Jan 21, 2012 at 5:11 PM, Pete Wyckoff <pw@padd.com> wrote:
Show 29 quoted lines
> luke@diamand.org wrote on Sat, 21 Jan 2012 10:51 +0000:
>> However, one thing I noticed in reading through is that it will
>> break if you end up importing a P4 branch that has spaces (or other
>> shell chars) in its name. A quick test confirms this.
>>
>> - the code doesn't handle the names properly
>> - git and p4 have different ideas about valid branch names
>>
>> But before rejecting Vitor's changes because of that it would be
>> worth considering whether we care (much). My own opinion is that if
>> you have developers who are daft enough to put spaces or dollars in
>> their branch names then their project is already doomed anyway....
>>
>> Perhaps it would be enough just to issue a warning ("your project is
>> doomed; start working on your CV") and skip such branch names rather
>> than falling over with inexplicable error messages.
>
> This doesn't seem like a big deal.  The read_pipe and
> read_pipe_lines calls shoud be list-ified.  That gets rid
> of the problem with shell interactions.
>
> For git branch name reserved characters, a little function
> to replace the bogus characters with "_" would avoid needing
> to go work on the resume.  Anything in bad_ref_char() and
> check_refname_component().  I agree this doesn't have to be
> perfect.
>
> This could be a new patch unrelated to Vitor's series, which
> verifies branch names anywhere a new commit is made.

I would also prefer to include that fix on a separate patch series that would include the test case Luke already prepared. In my opinion, updating read_pipe and read_pipe_lines is out of scope for the current patch series.

BTW, and on an unrelated topic, are any test cases failing on your side?

Thanks, Vitor

Pete Wyckoff· Jan 23, 2012, 22:40 UTC · re: Vitor Antunes · lore

Re: [PATCH v2 3/3] git-p4: Add test case for complex branch import

vitor.hda@gmail.com wrote on Mon, 23 Jan 2012 14:01 +0000:
Show 35 quoted lines
> On Sat, Jan 21, 2012 at 5:11 PM, Pete Wyckoff <pw@padd.com> wrote:
> > luke@diamand.org wrote on Sat, 21 Jan 2012 10:51 +0000:
> >> However, one thing I noticed in reading through is that it will
> >> break if you end up importing a P4 branch that has spaces (or other
> >> shell chars) in its name. A quick test confirms this.
> >>
> >> - the code doesn't handle the names properly
> >> - git and p4 have different ideas about valid branch names
> >>
> >> But before rejecting Vitor's changes because of that it would be
> >> worth considering whether we care (much). My own opinion is that if
> >> you have developers who are daft enough to put spaces or dollars in
> >> their branch names then their project is already doomed anyway....
> >>
> >> Perhaps it would be enough just to issue a warning ("your project is
> >> doomed; start working on your CV") and skip such branch names rather
> >> than falling over with inexplicable error messages.
> >
> > This doesn't seem like a big deal.  The read_pipe and
> > read_pipe_lines calls shoud be list-ified.  That gets rid
> > of the problem with shell interactions.
> >
> > For git branch name reserved characters, a little function
> > to replace the bogus characters with "_" would avoid needing
> > to go work on the resume.  Anything in bad_ref_char() and
> > check_refname_component().  I agree this doesn't have to be
> > perfect.
> >
> > This could be a new patch unrelated to Vitor's series, which
> > verifies branch names anywhere a new commit is made.
> 
> I would also prefer to include that fix on a separate patch series that
> would include the test case Luke already prepared. In my opinion,
> updating read_pipe and read_pipe_lines is out of scope for the current
> patch series.

How about taking what's below and just squashing it in. It's incremental on your changes and would go well with Luke's series that fixes a bunch of scattered quoting issues similarly.

The change to "describe %s" is unnecessary, but makes all the invocations look similar. You can leave it out.

This may conflict if you've already factored out the big "if self.detectBranches" chunk into a separate function as Junio recommended.

> BTW, and on an unrelated topic, are any test cases failing on your side?

I do run the tests regularly, and your series is good. There's the 'clone --use-client-spec' one that is broken until my 2ea09b5 (git-p4: adjust test to adhere to stricter useClientSpec, 2012-01-11) is merged. It's on pu.

		-- Pete
-----------8<-------------------
From f1cfb3836f5150dca86238225da56fe0bd577df8 Mon Sep 17 00:00:00 2001
From: Pete Wyckoff <pw@padd.com>
Date: Thu, 10 Nov 2011 07:40:14 -0500
Subject: [PATCH] git-p4: use list invoctaions to avoid shell mangling

Change git and p4 command invocations to avoid going through the shell. This allows branch names with spaces and wildcards to work.

Signed-off-by: Pete Wyckoff <pw@padd.com>
---
 contrib/fast-import/git-p4 |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)
Show changes to contrib/fast-import/git-p4 +3 −3
diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index 2e3b741..b440966 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -1961,7 +1961,7 @@ class P4Sync(Command, P4UserMap):
     def importChanges(self, changes):
         cnt = 1
         for change in changes:
-            description = p4Cmd("describe %s" % change)
+            description = p4Cmd(["describe", str(change)])
             self.updateOptionDict(description)
 
             if not self.silent:
@@ -2022,9 +2022,9 @@ class P4Sync(Command, P4UserMap):
                             self.commit(description, filesForCommit, tempBranch, [branchPrefix])
                             self.tempBranches.append(tempBranch)
                             self.checkpoint()
-                            for blob in read_pipe_lines("git rev-list --reverse --no-merges %s" % parent):
+                            for blob in read_pipe_lines(["git", "rev-list", "--reverse", "--no-merges", parent]):
                                 blob = blob.strip()
-                                if len( read_pipe("git diff-tree %s %s" % (blob, tempBranch)) ) == 0:
+                                if len(read_pipe(["git", "diff-tree", blob, tempBranch])) == 0:
                                     parentFound = True
                                     if self.verbose:
                                         print "Found parent of %s in commit %s" % (branch, blob)

-- 
1.7.9.rc2.33.g492ae
Vitor Antunes· Jan 25, 2012, 01:23 UTC · re: Pete Wyckoff · lore

Re: [PATCH v2 3/3] git-p4: Add test case for complex branch import

On Mon, Jan 23, 2012 at 10:40 PM, Pete Wyckoff <pw@padd.com> wrote:
Show 6 quoted lines
> How about taking what's below and just squashing it in.  It's
> incremental on your changes and would go well with Luke's series
> that fixes a bunch of scattered quoting issues similarly.
>
> The change to "describe %s" is unnecessary, but makes all the
> invocations look similar.  You can leave it out.

I've squashed your patch, but kept the "describe %s" fix in a separate commit.

Show 6 quoted lines
>> BTW, and on an unrelated topic, are any test cases failing on your side?
>
> I do run the tests regularly, and your series is good.  There's
> the 'clone --use-client-spec' one that is broken until my
> 2ea09b5 (git-p4: adjust test to adhere to stricter useClientSpec,
> 2012-01-11) is merged.  It's on pu.

Tests in t9809-git-p4-client-view.sh were failing for me because I'm using dash instead of bash. Please check patch below for a fix.

Test 15 of t9800-git-p4-basic.sh is still failing and I've not been able to pinpoint the problem. I can send you the logs off-list, if you want.

Thanks, Vitor

Show changes to t/t9809-git-p4-client-view.sh +1 −1
diff --git a/t/t9809-git-p4-client-view.sh b/t/t9809-git-p4-client-view.sh
index c9471d5..5b0ad99 100755
--- a/t/t9809-git-p4-client-view.sh
+++ b/t/t9809-git-p4-client-view.sh
@@ -31,7 +31,7 @@ client_view() {
 #
 check_files_exist() {
        ok=0 &&
-       num=${#@} &&
+       num=$# &&
        for arg ; do
                test_path_is_file "$arg" &&
                ok=$(($ok + 1))
Pete Wyckoff· Jan 25, 2012, 12:34 UTC · re: Vitor Antunes · lore

Re: [PATCH v2 3/3] git-p4: Add test case for complex branch import

vitor.hda@gmail.com wrote on Wed, 25 Jan 2012 01:23 +0000:
Show 43 quoted lines
> On Mon, Jan 23, 2012 at 10:40 PM, Pete Wyckoff <pw@padd.com> wrote:
> > How about taking what's below and just squashing it in.  It's
> > incremental on your changes and would go well with Luke's series
> > that fixes a bunch of scattered quoting issues similarly.
> >
> > The change to "describe %s" is unnecessary, but makes all the
> > invocations look similar.  You can leave it out.
> 
> I've squashed your patch, but kept the "describe %s" fix in a separate
> commit.
> 
> >> BTW, and on an unrelated topic, are any test cases failing on your side?
> >
> > I do run the tests regularly, and your series is good.  There's
> > the 'clone --use-client-spec' one that is broken until my
> > 2ea09b5 (git-p4: adjust test to adhere to stricter useClientSpec,
> > 2012-01-11) is merged.  It's on pu.
> 
> Tests in t9809-git-p4-client-view.sh were failing for me because I'm
> using dash instead of bash. Please check patch below for a fix.
> 
> Test 15 of t9800-git-p4-basic.sh is still failing and I've not been able
> to pinpoint the problem. I can send you the logs off-list, if you want.
> 
> Thanks,
> Vitor
> 
> 
> 
> diff --git a/t/t9809-git-p4-client-view.sh b/t/t9809-git-p4-client-view.sh
> index c9471d5..5b0ad99 100755
> --- a/t/t9809-git-p4-client-view.sh
> +++ b/t/t9809-git-p4-client-view.sh
> @@ -31,7 +31,7 @@ client_view() {
>  #
>  check_files_exist() {
>         ok=0 &&
> -       num=${#@} &&
> +       num=$# &&
>         for arg ; do
>                 test_path_is_file "$arg" &&
>                 ok=$(($ok + 1))
>

Yes, thanks. Plain old $# works fine, even if the arguments have spaces. I'll hang onto this with some other work in the same area and submit it eventually.

		-- Pete
Vitor Antunes· Jan 25, 2012, 01:39 UTC · lore

Re: [PATCH v2 3/3] git-p4: Add test case for complex branch import

On Mon, Jan 23, 2012 at 1:53 PM, Vitor Antunes <vitor.hda@gmail.com> wrote:
Show 23 quoted lines
> On Sat, Jan 21, 2012 at 4:54 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Vitor Antunes <vitor.hda@gmail.com> writes:
>>
>>> +test_expect_success 'git-p4 add complex branches' '
>>> +     test_when_finished cleanup_git &&
>>> +     test_create_repo "$git" &&
>>> +     (
>>> +             cd "$cli" &&
>>> +             changelist=$(p4 changes -m1 //depot/... | cut -d" " -f2) &&
>>> +             changelist=$(($changelist - 5)) &&
>>> +             p4 integrate //depot/branch1/...@$changelist //depot/branch4/... &&
>>> +             p4 submit -d "branch4" &&
>>> +             changelist=$(($changelist + 2)) &&
>>> +             p4 integrate //depot/branch1/...@$changelist //depot/branch5/... &&
>>> +             p4 submit -d "branch5"
>>
>> That's a strange quoting convention. Why are "branch4" and "branch5"
>> enclosed in double quotes while "integrate" and "submit" aren't?
>> (rhetorical: do not quote these branch names without a good reason).
>
> There is no reason that I can remember to have those enclosed in double
> quotes. Will double check in my local branches at home tonight. Anyway,
> expect a fix for this in v3.

I now see why I added the quotes. The -d option is used to input the description of the commit, which can contain spaces and other special characters. Admittedly they are not required in this case, but from a consistency point of view I would prefer to keep them. Is this acceptable?

Show 27 quoted lines
>>> +# Configure branches through git-config and clone them. git-p4 will only be able
>>> +# to clone the original structure if it is able to detect the origin changelist
>>> +# of each branch.
>>> +test_expect_success 'git-p4 clone complex branches' '
>>> +     test_when_finished cleanup_git &&
>>> +     test_create_repo "$git" &&
>>> +     (
>>> +             cd "$git" &&
>>> +             git config git-p4.branchList branch1:branch2 &&
>>> +             git config --add git-p4.branchList branch1:branch3 &&
>>> +             git config --add git-p4.branchList branch1:branch4 &&
>>> +             git config --add git-p4.branchList branch1:branch5 &&
>>> +             "$GITP4" clone --dest=. --detect-branches //depot@all &&
>>> +             git log --all --graph --decorate --stat &&
>>> +             git reset --hard p4/depot/branch1 &&
>>> +             test_path_is_file file1 &&
>>> +             test_path_is_file file2 &&
>>> +             test_path_is_file file3 &&
>>> +             grep -q update file2 &&
>>
>> Do you really need to use "-q" here?  Wouldn't it help if you wrote it
>> without it while debugging tests with "sh ./t9801-*.sh -v"?
>
> Makes sense.
>
> Thank you for the helpful comments.
> Vitor

Apparently I've hit "Reply" instead of "Reply all" before, so I'll keep the full quote of my previous email for future reference.

Vitor
Junio C Hamano· Jan 25, 2012, 04:02 UTC · re: Vitor Antunes · lore

Re: [PATCH v2 3/3] git-p4: Add test case for complex branch import

Vitor Antunes <vitor.hda@gmail.com> writes:
Show 14 quoted lines
>>>> +             p4 submit -d "branch5"
>>>
>>> That's a strange quoting convention. Why are "branch4" and "branch5"
>>> enclosed in double quotes while "integrate" and "submit" aren't?
>>> (rhetorical: do not quote these branch names without a good reason).
>>
>> There is no reason that I can remember to have those enclosed in double
>> quotes. Will double check in my local branches at home tonight. Anyway,
>> expect a fix for this in v3.
>
> I now see why I added the quotes. The -d option is used to input the
> description of the commit, which can contain spaces and other special
> characters. Admittedly they are not required in this case, but from a
> consistency point of view I would prefer to keep them.

Hmm, the argument "branch5" made it look like it is the name of the branch you are giving here. If it is supposed to be human-readable free-form text description, I would prefer to see it as such, e.g.

	p4 submit -d "integrate changes on branch #1 to branch #5"
or something like that.

← back to recent threads