From: Vitor Antunes Date: Mon, 23 Jan 2012 14:01:14 GMT Subject: Re: [PATCH v2 3/3] git-p4: Add test case for complex branch import Message-ID: In-Reply-To: <20120121171130.GA6235@padd.com> On Sat, Jan 21, 2012 at 5:11 PM, Pete Wyckoff 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