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

"Pat Notz" [off-list ref] writes:

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

On Thu, Sep 23, 2010 at 11:56 AM, Junio C Hamano [off-list ref] wrote:

"Pat Notz" [off-list ref] writes:

quoted

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

Yes, this was the concern I expressed in the v1 series patch.  I'm
getting more comfortable with the code so I'll look into re-encoding
appropriately.

 (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;

Agreed.  I'll factor out the duplication.

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

Will do.

 (3) No need yet.