Create branch fails silently
Reported by Stoney | May 15th, 2009 @ 07:58 PM
If I click on the Create Branch button, and type in a branch name containing a space, the branch is not created, and no error is reported.
Comments and changes to this ticket
I've created a commit to fix this problem:
- State changed from new to small
Cool thing. Two things I'd like to see before I think we could incorporate this patch:
- A return-code != 0 doesn't just mean "invalid branch name" but
could also mean "branch already exists". Unfortunately the
return-code for both operations is 128, so you'll have to check the
output in case the return-code is !=0
- The diff of the XIB-file reveals a file "public.png", but your commit does not. Since the XIB-files have become quite readable IMHO, we should also just stage the changes we intended to.
Wow, that comment was just unnecessary. I just read your code more closely and saw that you're using check-ref-format. Ok. But what you could also do (while you're at it) is to check the return-value of update-ref and use your new error-message field to indicate failure (because of already existing refs). This would happen after line 206 in your patch.
While I agree that we need to check for valid names and existing refs,
I don't like this approach. This way we need three calls to git before
creating a ref.
The check-ref-format call might be OK, but for what it does (as is
described in the manpage), we might just as well recreate it
ourselves. The existing branch thing should be handled as I mentioned
in the other ticket.
I only count one call to git before creating the ref - the call to check-ref-format. What advantage is there to recreating it when check-ref-format does it for us? I'd think using that means that if they change the rules down the line, we inherit the new rules by virtue of using the same function that git's tools are using.
Oh, I see, you're referring to the call to show-ref as the second call. OK - here is another commit (merging both tickets #151 and 155) and using update-ref with the 40 "0"'s rather than show-ref, along with cleaning up the commit.
- State changed from small to resolved
Thanks, this looks good and I've cherry-picked it into my stable
branch. This is probably important enough for the stable release.
One thing I'd like to see in a future release is that the sheet(s) get
their own .xib and controller. The logic is starting to become complex
enough to be allow them to be separated. That'd also allow us to
create a new branch from the commit view, for example.