On Thu, Sep 23, 2010 at 11:56 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> "Pat Notz" <patnotz@xxxxxxxxx> 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?
>
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.
>
--
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