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