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.
BTW, and on an unrelated topic, are any test cases failing on your side?
Thanks,
Vitor
--
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