Re: [PATCH 1/2] commit: add message options for rebase --autosquash
- From
Stephen Boyd <bebarino@gmail.com>
- Date
- Sep 17, 2010, 08:36 UTC
- Message-ID
- <4C93288B.7000908@gmail.com>
- In-Reply-To
- <1284687596-236-2-git-send-email-patnotz@gmail.com>
On 09/16/2010 06:39 PM, Pat Notz wrote:
Show 11 quoted lines
> 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?
Show 9 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?
Show 16 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?
Show 16 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.
I read this and became confused. fixup_message? target_message? Perhaps it should be renamed to fixup_commit, squash_commit, target_commit?
Show 13 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.