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

["Kristian Høgsberg" <krh@redhat.com>]

On Thu, 2007-11-01 at 16:51 -0700, Junio C Hamano wrote:

Hi Junio,

Thanks for the review (again).  I've split the patch up in a couple of
test suite patches (a patch to accept ':' as a no-op editor, fixing
hard-coding of editor, and a test case for the commit --amend --author
case), exporting launch_editor() and a reworked version of the commit
patch that addresses your comments.

> Kristian Høgsberg <krh@redhat.com> writes:
> 
> > @@ -364,7 +365,6 @@ BUILTIN_OBJS = \
> >  	builtin-rev-parse.o \
> >  	builtin-revert.o \
> >  	builtin-rm.o \
> > -	builtin-runstatus.o \
> >  	builtin-shortlog.o \
> >  	builtin-show-branch.o \
> >  	builtin-stripspace.o \
> 
> I did not read in the commit log that runstatus is gone...

True, that should be documented.  Added in the following patch.

> > diff --git a/builtin-commit.c b/builtin-commit.c
> > new file mode 100644
> > index 0000000..56c7427
> > --- /dev/null
> > +++ b/builtin-commit.c
> > @@ -0,0 +1,608 @@
> > +/*
> > + * Builtin "git commit"
> > + *
> > + * Copyright (c) 2007 Kristian Høgsberg <krh@redhat.com>
> > + * Based on git-commit.sh by Junio C Hamano and Linus Torvalds
> > + */
> > +
> > +#include <sys/types.h>
> > +#include <sys/stat.h>
> > +#include <unistd.h>
> > +
> > +#include "cache.h"
> 
> 
> The system header files on some systems have a nasty habit of
> changing the behaviour depending on the order they are included.
> Since 85023577a8f4b540aa64aa37f6f44578c0c305a3 (simplify
> inclusion of system header files) in late 2006, we have a few
> rules for system-header inclusion to avoid the portability
> issues:
> 
>  (1) sources under compat/, platform sha-1 implementations, and
>      xdelta code are exempt from the following rules;
> 
>  (2) the first #include must be "git-compat-util.h" or one of
>      our own header file that includes it first (e.g. config.h,
>      builtin.h, pkt-line.h);
> 
>  (3) system headers that are included in "git-compat-util.h"
>      need not be included in individual C source files.
> 
>  (4) "git-compat-util.h" does not have to include subsystem
>      specific header files (e.g. expat.h).

Ah, yes, this has been pointed out to me before, sorry.  Patch updated.

> > +static void determine_author_info(struct strbuf *sb)
> > +{
> > +	char *p, *eol;
> > +	char *name = NULL, *email = NULL;
> > +
> > +	if (use_message) {
> > +		p = strstr(use_message_buffer, "\nauthor");
> > +		if (!p)
> > +			die("invalid commit: %s\n", use_message);
> > +		p++;
> > +		eol = strchr(p, '\n');
> > +		if (!eol)
> > +			die("invalid commit: %s\n", use_message);
> > +
> > +		strbuf_add(sb, p, eol + 1 - p);
> > +	} else if (force_author) {
> > +		const char *eoname = strstr(force_author, " <");
> > +		const char *eomail = strchr(force_author, '>');
> 
> Doesn't this break:
> 
>     $ git commit --amend --author='A U Thor <author@example.com>'
> 
> to fix a misattribution?

Dang, good catch.  I've fixed it by swapping the (use_message) and
(force_author) clauses and the new patch adds a test case for this.

> > +static int parse_and_validate_options(int argc, const char *argv[])
> > +{
> > ...
> > +	if (use_message) {
> > +		unsigned char sha1[20];
> > +		static char utf8[] = "UTF-8";
> > +		const char *out_enc;
> > +		char *enc, *end;
> > +		struct commit *commit;
> > +
> > +		if (get_sha1(use_message, sha1))
> > +			die("could not lookup commit %s", use_message);
> > +		commit = lookup_commit(sha1);
> > +		if (!commit || parse_commit(commit))
> > +			die("could not parse commit %s", use_message);
> > +
> > +		enc = strstr(commit->buffer, "\nencoding");
> > +		if (enc) {
> > +			end = strchr(enc + 10, '\n');
> > +			enc = xstrndup(enc + 10, end - (enc + 10));
> > +		} else {
> > +			enc = utf8;
> > +		}
> > +		out_enc = git_commit_encoding ? git_commit_encoding : utf8;
> > +
> > +		use_message_buffer =
> > +			reencode_string(commit->buffer, out_enc, enc);
> > +		if (enc != utf8)
> > +			free(enc);
> 
> A few issues.
> 
>  * When use_message is set because of --amend that wanted to
>    amend a commit message that was recorded in a corrupt
>    encoding, and iconv() in reencode_string() fails, saying "I
>    cannot convert that completely", most of the message can
>    still be salvageable.  This part should allow looser
>    reencoding if the message will be eyeballed and edited by the
>    user.

So in this case we just want to copy the old message byte for byte,
right?  That's what I have in the updated patch.

>  * We would want to avoid reencoding when !strcmp(out_enc, enc).
>    Both builtin-mailinfo.c and commit.c skip the call to the
>    function at the calling site, but it might make sense to
>    teach reencode_string() about such an optimization.

Right.  I just added the call-site optimization for builtin-commit for
now, but I would expect iconv() to be smart in case input and output
encodings are the same.

cheers,
Kristian