Re: [PATCH v2 3/3] git-p4: Add test case for complex branch import
- From
- Pete Wyckoff <pw@padd.com>
- Date
- Jan 23, 2012, 22:40 UTC
- Message-ID
- <20120123224012.GA10626@padd.com>
- In-Reply-To
- <CAOpHH-W1LY3Q50otrcNJTYWN67k_pCZHEOkgbKy7kPgfUbGeQw@mail.gmail.com>
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(-)
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