Re: [PATCH v2 07/34] sequencer (rebase -i): add support for the 'fixup' and 'squash' commands
From: Junio C Hamano <hidden>
Date: 2016-12-15 18:43:41
Johannes Schindelin [off-list ref] writes:
quoted hunk
diff --git a/sequencer.c b/sequencer.c index f6e20b142a..271c21581d 100644 --- a/sequencer.c +++ b/sequencer.c@@ -45,6 +45,35 @@ static GIT_PATH_FUNC(rebase_path_todo, "rebase-merge/git-rebase-todo") */ static GIT_PATH_FUNC(rebase_path_done, "rebase-merge/done") /* + * The commit message that is planned to be used for any changes that + * need to be committed following a user interaction. + */ +static GIT_PATH_FUNC(rebase_path_message, "rebase-merge/message") +/* + * The file into which is accumulated the suggested commit message for + * squash/fixup commands. When the first of a series of squash/fixups
The same comment as 03/34 applies here, regarding blank line to separate logical unit.
+static int update_squash_messages(enum todo_command command,
+ struct commit *commit, struct replay_opts *opts)
+{
+ struct strbuf buf = STRBUF_INIT;
+ int count, res;
+ const char *message, *body;
+
+ if (file_exists(rebase_path_squash_msg())) {
+ char *p, *p2;
+
+ if (strbuf_read_file(&buf, rebase_path_squash_msg(), 2048) <= 0)
+ return error(_("could not read '%s'"),
+ rebase_path_squash_msg());
+
+ if (buf.buf[0] != comment_line_char ||
+ !skip_prefix(buf.buf + 1, " This is a combination of ",
+ (const char **)&p))
+ return error(_("unexpected 1st line of squash message:"
+ "\n\n\t%.*s"),
+ (int)(strchrnul(buf.buf, '\n') - buf.buf),
+ buf.buf);
+ count = strtol(p, &p2, 10);
+
+ if (count < 1 || *p2 != ' ')
+ return error(_("invalid 1st line of squash message:\n"
+ "\n\t%.*s"),
+ (int)(strchrnul(buf.buf, '\n') - buf.buf),
+ buf.buf);
+
+ sprintf((char *)p, "%d", ++count);Do we know the area pointed at p (which is inside buf) long enough not to overflow? If the original were 9 and you incremented to get 10, you would need one extra byte.
+ if (!*p2)
+ *p2 = ' ';
+ else {
+ *(++p2) = 'c';p2 points into buf; do we know this increment does not step beyond its end? What is the meaning of a letter 'c' here (I do not see a corresponding one in the scripted update_squash_messages)?
+ strbuf_insert(&buf, p2 - buf.buf, " ", 1);
+ }
+ }
+ else {
Style: "} else {" (I won't repeat this, as it will become too noisy).
+ unsigned char head[20];
+ struct commit *head_commit;
+ const char *head_message, *body;
+
+ if (get_sha1("HEAD", head))
+ return error(_("need a HEAD to fixup"));
+ if (!(head_commit = lookup_commit_reference(head)))
+ return error(_("could not read HEAD"));
+ if (!(head_message = get_commit_buffer(head_commit, NULL)))
+ return error(_("could not read HEAD's commit message"));
+
+ body = strstr(head_message, "\n\n");
+ if (!body)
+ body = "";
+ else
+ body = skip_blank_lines(body + 2);I think I saw you used a helper function find_commit_subject() to do the above in an earlier patch for "edit" in this series. Would it make this part (and another one for "commit" we have after this if/else) shorter?
quoted hunk
static int do_pick_commit(enum todo_command command, struct commit *commit, - struct replay_opts *opts) + struct replay_opts *opts, int final_fixup) { + int edit = opts->edit, cleanup_commit_message = 0; + const char *msg_file = edit ? NULL : git_path_merge_msg(); unsigned char head[20]; struct commit *base, *next, *parent; const char *base_label, *next_label; struct commit_message msg = { NULL, NULL, NULL, NULL }; struct strbuf msgbuf = STRBUF_INIT; - int res, unborn = 0, allow; + int res, unborn = 0, amend = 0, allow; if (opts->no_commit) { /*@@ -749,7 +885,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit, else parent = commit->parents->item; - if (opts->allow_ff && + if (opts->allow_ff && !is_fixup(command) && ((parent && !hashcmp(parent->object.oid.hash, head)) || (!parent && unborn))) return fast_forward_to(commit->object.oid.hash, head, unborn, opts);@@ -813,6 +949,28 @@ static int do_pick_commit(enum todo_command command, struct commit *commit, } } + if (is_fixup(command)) { + if (update_squash_messages(command, commit, opts)) + return -1; + amend = 1; + if (!final_fixup) + msg_file = rebase_path_squash_msg(); + else if (file_exists(rebase_path_fixup_msg())) { + cleanup_commit_message = 1; + msg_file = rebase_path_fixup_msg(); + } + else { + const char *dest = git_path("SQUASH_MSG"); + unlink(dest); + if (copy_file(dest, rebase_path_squash_msg(), 0666)) + return error(_("could not rename '%s' to '%s'"), + rebase_path_squash_msg(), dest);
Perhaps an error from unlink(dest) before copy_file() should also result in an error return? After all, that unlink() was added to mimick the "cp" in the scripted one and copy_file() does not want to overwrite an existing destination.
+ unlink(git_path("MERGE_MSG"));Errors from this and other unlink() that emulates "rm -f" were unchecked in the scripted original, so not checking for errors is not a regression. I would check for an error if I were writing this, however, because I know I would forget updating these after I am done with the series.
quoted hunk
+ msg_file = dest; + edit = 1; + } + } + if (!opts->strategy || !strcmp(opts->strategy, "recursive") || command == TODO_REVERT) { res = do_recursive_merge(base, next, base_label, next_label, head, &msgbuf, opts);@@ -868,8 +1026,13 @@ static int do_pick_commit(enum todo_command command, struct commit *commit, goto leave; } if (!opts->no_commit) - res = run_git_commit(opts->edit ? NULL : git_path_merge_msg(), - opts, allow, opts->edit, 0, 0); + res = run_git_commit(msg_file, opts, allow, edit, amend, + cleanup_commit_message);
The introduction of msg_file variable made this call (actually, the logic to decide which file is affected) much nicer to understand.
quoted hunk
+ if (!res && final_fixup) { + unlink(rebase_path_fixup_msg()); + unlink(rebase_path_squash_msg()); + }@@ -1475,6 +1660,21 @@ static int do_exec(const char *command_line) return status; } +static int is_final_fixup(struct todo_list *todo_list) +{ + int i = todo_list->current; + + if (!is_fixup(todo_list->items[i].command)) + return 0; + + while (++i < todo_list->nr) + if (is_fixup(todo_list->items[i].command)) + return 0; + else if (todo_list->items[i].command < TODO_NOOP) + break;
What follows NOOP are comment and "drop" which is another comment in disguise, so this one is excluding all the no-op commands in various shapes, which makes sense but is clear only to a reader who bothered to go back to "enum todo_command" and checked that fact. If a check for "is it one of the no-op commands?" appears only here, a single liner comment may be sufficient (but necessary) to help readers. Otherwise a single-liner helper function (similar to is_fixup() you have) with a descriptive name would be better than a single liner comment.