Gitx GitX

Comments and changes to this ticket

• 

  Charles O'Rourke May 25th, 2009 @ 06:25 PM

  I've created a commit to fix this problem:

  http://github.com/shadesofblue/gitx/commit/3427ec91a9359961696ae52a...

• 

  Johannes Gilger May 27th, 2009 @ 06:06 PM

  • State changed from “new” to “small”

  Cool thing. Two things I'd like to see before I think we could incorporate this patch:

  1. 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
     
  2. 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.
• 

  Johannes Gilger May 27th, 2009 @ 06:16 PM

  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.

• 

  Johannes Gilger May 27th, 2009 @ 06:27 PM

  Again, sorry, I didn't see you head two commits which address this problem. This calls for speedy upstream inclusion.

• 

  Pieter de Bie May 27th, 2009 @ 06:32 PM

  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.

• 

  Charles O'Rourke May 28th, 2009 @ 02:59 AM

  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.

• 

  Charles O'Rourke May 28th, 2009 @ 03:20 AM

  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.

  http://github.com/shadesofblue/gitx/commit/81841e854805befcb10d510f...

• 

  Pieter de Bie May 28th, 2009 @ 02:23 PM

  • 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.

  [state:resolved]