Re: [PATCH 1/2] commit: add message options for rebase --autosquash
- From
Pat Notz <patnotz@gmail.com>
- Date
- Sep 17, 2010, 15:34 UTC
- Message-ID
- <AANLkTikZTSiG6anuRR0h499JeTzRcdeE-jaYMu7Gqr8W@mail.gmail.com>
- In-Reply-To
- <4C93288B.7000908@gmail.com>
On Fri, Sep 17, 2010 at 2:36 AM, Stephen Boyd <bebarino@gmail.com> wrote:
Show 16 quoted lines
> On 09/16/2010 06:39 PM, Pat Notz wrote: >> These options make it convenient to construct commit messages for use >> with 'rebase --autosquash'. The resulting commit message will be >> "fixup! ..." or "squash! ..." where "..." is the subject line of the >> specified commit message. >> >> Example usage: >> $ git commit --fixup HEAD~2 >> $ git commit --squash HEAD~5 >> >> Signed-off-by: Pat Notz <patnotz@gmail.com> >> --- > > So far I've been using an alias for these, but I suppose making them > real features of git could be worthwhile. What are the benefits with > this approach vs. an alias?
Mainly it's convenience. The rebase --autosquash feature seems too hard to use without this or an alias and making everyone code their own alias seems a lot to ask.
Still, I admit that I was concerned with adding yet another option to git-commit. If enough people object, I can live with that.
Show 13 quoted lines
>> @@ -863,7 +871,7 @@ static int parse_and_validate_options(int argc, const char *argv[],
>> if (force_author && renew_authorship)
>> die("Using both --reset-author and --author does not make sense");
>>
>> - if (logfile || message.len || use_message)
>> + if (logfile || message.len || use_message || fixup_message || squash_message)
>> use_editor = 0;
>> if (edit_flag)
>> use_editor = 1;
>
> The whole point of squash is to combine two commit texts, right?
> Otherwise wouldn't you use --fixup where you throw away the text
> eventually and thus don't want to open an editor?Good point. Admittedly, I was focusing on the 'fixup' case but squash needs to open the editor with the first line pre-filled.
Show 23 quoted lines
>
>> @@ -883,15 +891,19 @@ static int parse_and_validate_options(int argc, const char *argv[],
>> f++;
>> if (edit_message)
>> f++;
>> + if (fixup_message)
>> + f++;
>> + if (squash_message)
>> + f++;
>> if (logfile)
>> f++;
>> if (f > 1)
>> - die("Only one of -c/-C/-F can be used.");
>> + die("Only one of -c/-C/-F/--fixup/--squash can be used.");
>> if (message.len && f > 0)
>> - die("Option -m cannot be combined with -c/-C/-F.");
>> + die("Option -m cannot be combined with -c/-C/-F/--fixup/--squash.");
>
>
> Furthering that point, perhaps I want to squash this commit into another
> commit using the commit text from yet another commit or just with an
> extra note from the command line (-m). Perhaps this is where the benefit
> over an alias comes in?That's a good use-case. I'll re-work the --squash option.
Show 20 quoted lines
>
>> if (edit_message)
>> use_message = edit_message;
>> - if (amend && !use_message)
>> + if (amend && (!use_message && !fixup_message && !squash_message))
>> use_message = "HEAD";
>> if (!use_message && renew_authorship)
>> die("--reset-author can be used only with -C, -c or --amend.");
>> @@ -932,6 +944,23 @@ static int parse_and_validate_options(int argc, const char *argv[],
>> if (enc != utf8)
>> free(enc);
>> }
>> + if (fixup_message || squash_message) {
>> + unsigned char sha1[20];
>> + struct commit *commit;
>> + const char * target_message = fixup_message ? fixup_message : squash_message;
>> + const char * msg_fmt = fixup_message ? "fixup! %s" : "squash! %s";
>
> Style nit: stick the * to the variable.
>Oops, thanks.
> I read this and became confused. fixup_message? target_message? Perhaps > it should be renamed to fixup_commit, squash_commit, target_commit? >
I was mostly trying to reduce duplicate code for the two cases... but, I bet when I re-work --squash this will go away.
Show 18 quoted lines
>> + struct strbuf buf = STRBUF_INIT;
>> + struct pretty_print_context ctx = {0};
>> +
>> + if (get_sha1(target_message, sha1))
>> + die("could not lookup commit %s", target_message);
>> + commit = lookup_commit_reference(sha1);
>> + if (!commit || parse_commit(commit))
>> + die("could not parse commit %s", target_message);
>> +
>> + format_commit_message(commit, msg_fmt, &buf, &ctx);
>> + fixup_message_buffer = strbuf_detach(&buf, NULL);
>> + }
>>
>
> Is it necessary to do this block of code here? Couldn't you lookup and
> format the commit in prepare_to_commit()? Then we wouldn't have to
> allocate another strbuf and the "message" code would be more centralized.
>Probably not, I was mostly trying to follow the example from the use_message (-C/-c) feature. It *would* be nice to avoid the extra memory (de)alloc.
Thanks for the great feedback!