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@xxxxxxxxxx> 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@xxxxxxxxxx>
> > + * 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@xxxxxxxxxxx>'
>
> 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
-
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
