From: Pat Notz <patnotz@gmail.com>
Date: Thu, 23 Sep 2010 18:27:46 GMT
Subject: Re: [PATCHv4 1/4] commit: --fixup option for use with rebase --autosquash
Message-ID: <AANLkTikUonhDU-_mq+ON2nRvHEnJszR5jW7siyeXDFLn@mail.gmail.com>
In-Reply-To: <7vsk10qr7p.fsf@alter.siamese.dyndns.org>

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