Re: [PATCH 1/2] commit: add message options for rebase --autosquash
From: Stephen Boyd <hidden>
Date: 2016-06-15 22:49:33
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 <redacted> ---
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?
quoted hunk ↗ jump to hunk
@@ -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?
quoted hunk ↗ jump to hunk
@@ -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?
quoted hunk ↗ jump to hunk
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?
+ 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.