vitor.hda@xxxxxxxxx wrote on Mon, 23 Jan 2012 14:01 +0000:
> On Sat, Jan 21, 2012 at 5:11 PM, Pete Wyckoff <pw@xxxxxxxx> wrote:
> > luke@xxxxxxxxxxx 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@xxxxxxxx>
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@xxxxxxxx>
---
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html