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

[Vitor Antunes <vitor.hda@gmail.com>]

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