Re: [PATCH 7/7] Implement git commit as a builtin command.

On Tue, 2007-09-18 at 14:58 +0100, Johannes Schindelin wrote:

Hi,

very nice!

Four nits, though, and a half:

- it would be nicer to put the option parsing it option.[ch] (you would 
  also need to pass the usage line then, instead of hardwiring it to 
  "git_commit_usage"),

Yes, good point.

- it seems more logical to me to call it "parse_option()" than 
  "scan_options()", since that is what it does,

Yup.

- you might want to rename OPTION_NONE to OPTION_BOOLEAN, and maybe even 
  allow "--no-<option>" in that case for free,

Agree.

- wt_status_prepare() could take a parameter "index_file", which would 
  default to git_path("index") when passed as NULL, and

Yeah, the way I did it, I preserved the API, but that's not really a
concern, I guess.

- launch_editor() is defined in builtin-tag.c, which is not part of the 
  library, and therefore it would be technically more correct to either 
  move the function to editor.c (my preferred solution), or declare it in 
  builtin.h instead of strbuf.h.

Yeah, and we should move the stripspace code there too.  Or maybe we
should rename that strbuf_stripspace and put it in strbuf.c.

As you can see, my nits are really minor, which means that I am pretty 
happy with your work!

Great, I hope we can get it in soon :)

Kristian