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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 1, 2007, 23:51 UTC
Message-ID: <7vr6j97ce6.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <1193944163-22892-1-git-send-email-krh@redhat.com>

Kristian Høgsberg <krh@redhat.com> writes:

Show 8 quoted lines

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

Show 18 quoted lines

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

Show 18 quoted lines

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

Show 29 quoted lines

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

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