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

[Junio C Hamano <gitster@pobox.com>]

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

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

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

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