Re: [PATCHv4 1/4] commit: --fixup option for use with rebase --autosquash

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 23, 2010, 17:56 UTC
Message-ID: <7vsk10qr7p.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1285262076-20134-2-git-send-email-patnotz@gmail.com>

"Pat Notz" <patnotz@gmail.com> writes:

Show 11 quoted lines

> +	} else if (fixup_message) {
> +		unsigned char sha1[20];
> +		struct commit *commit;
> +		struct pretty_print_context ctx = {0};
> +		if (get_sha1(fixup_message, sha1))
> +			die("could not lookup commit %s", fixup_message);
> +		commit = lookup_commit_reference(sha1);
> +		if (!commit || parse_commit(commit))
> +			die("could not parse commit %s", fixup_message);
> +		format_commit_message(commit, "fixup! %s\n\n", &sb, &ctx);
> +		hook_arg1 = "message";

I notice that the above is a half-copy-and-paste from "if (use_message)" codepath that handles -c/-C. A few issues to think about (i.e. not complaints; I haven't thought about them myself):

 (1) Is it worth refactoring the original instead of copying;

 (2) What happens/should happen when the original commit is encoded
     differently from the current commit encoding?  -c/-C codepath takes
     pains to re-encode.  Should we do so somewhere in this codepath, too?

 (3) If the answer to (2) is "Yes", notice that format_commit_message()
     does not re-encode the commit log message ("log" output codepath uses
     pretty.c::pretty_print_commit(), which reencodes for log output
     encoding).  Maybe we need an option to tell format_commit_message()
     to do so?

The last is not exactly an issue this patch alone should address, but I thought I'd better mention it anyway.

My knee-jerk answers to the above are:

 (1) The first handful of lines in this new "if (fixup_message)" codeblock
     up to "die" might want to use a helper function shared with the
     existing "if (use_message)" codepath;

 (2) We probably want to re-encode to the log output encoding the string
     we receive from format_commit_message() in this codepath.

 (3) No need yet.