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

[Pete Wyckoff <pw@padd.com>]

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