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

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

[Vitor Antunes]

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

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

[Vitor Antunes]

Signed-off-by: Vitor Antunes <vitor.hda@gmail.com>
---
 contrib/fast-import/git-p4 |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

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

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

[Vitor Antunes]

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(-)

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

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

[Vitor Antunes]

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(-)

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

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

[Junio C Hamano]

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).

> +# 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.

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

[Junio C Hamano]

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.

> @@ -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.

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

[Luke Diamand]

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.


>
> Thanks.

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

[Pete Wyckoff]

luke@diamand.org wrote on Sat, 21 Jan 2012 10:51 +0000:
> 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

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

[Vitor Antunes]

Hi Junio,

On Sat, Jan 21, 2012 at 4:55 AM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

>> @@ -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

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

[Vitor Antunes]

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.

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

Thanks,
Vitor

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

[Pete Wyckoff]

vitor.hda@gmail.com wrote on Mon, 23 Jan 2012 14:01 +0000:
> 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(-)

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

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

[Vitor Antunes]

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))

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

[Vitor Antunes]

On Mon, Jan 23, 2012 at 1:53 PM, Vitor Antunes <vitor.hda@gmail.com> wrote:
> 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?

>>> +# 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

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

[Junio C Hamano]

Vitor Antunes <vitor.hda@gmail.com> writes:

>>>> +             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.

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

[Pete Wyckoff]

vitor.hda@gmail.com wrote on Wed, 25 Jan 2012 01:23 +0000:
> 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