| Message ID | 20210129182050.26143-7-charvi077@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Commit | 9e3cebd97cbd47909e683e617d5ffa2781f0adaa |
| Headers | show |
| Series | rebase -i: add options to fixup command | expand |
On Fri, Jan 29, 2021 at 1:24 PM Charvi Mendiratta <charvi077@gmail.com> wrote: > Add options to `fixup` command to fixup both the commit contents and > message. `fixup -C` command is used to replace the original commit > message and `fixup -c`, additionally allows to edit the commit message. In the cover letter for this series, you had this additional information: This convention is similar to the existing `merge` command in the interactive rebase, that also supports `-c` and `-C` options with similar meanings. which helps to explain the choice of -c and -C. It might be nice to include that explanation in this commit message, as well (but not itself worth a re-roll). > Signed-off-by: Charvi Mendiratta <charvi077@gmail.com> > --- > diff --git a/rebase-interactive.c b/rebase-interactive.c > @@ -44,7 +44,9 @@ void append_todo_help(int command_count, > "s, squash <commit> = use commit, but meld into previous commit\n" > -"f, fixup <commit> = like \"squash\", but discard this commit's log message\n" > +"f, fixup [-C | -c] <commit> = like \"squash\", but discard this\n" > +" commit's log message. Use -C to replace with this\n" > +" commit message or -c to edit the commit message\n" This change jarringly introduces the first and only use of a period and capitalized word in the to-do help text. Perhaps instead say: ... like \"squash\", but discard this commit's log message; use -C to replace with this commit message or -c to edit the commit message When `-c` says "edit the commit message" it's not clear what will be edited. The original's commit message? The replacement's message? A combination of the two? If you can come up with a succinct way to word it such that it states more precisely what exactly will be edited, it would be nice, but not necessarily worth a re-roll. > diff --git a/sequencer.c b/sequencer.c > @@ -1718,6 +1718,12 @@ static int is_pick_or_similar(enum todo_command command) > +enum todo_item_flags { > + TODO_EDIT_MERGE_MSG = (1 << 0), > + TODO_REPLACE_FIXUP_MSG = (1 << 1), > + TODO_EDIT_FIXUP_MSG = (1 << 2), > +}; I'm confused. These enumeration items are defined as bit flags, implying that they may be combined, however... > @@ -1734,32 +1740,176 @@ static size_t subject_length(const char *body) > +static int check_fixup_flag(enum todo_command command, > + enum todo_item_flags flag) ...here and elsewhere, you declare the argument as a strict `todo_item_flags` enum item rather than `unsigned` which is the typical declaration when combining bit flag values. So, the picture thus far is confusing. Are the `todo_item_flags` values distinct unique values which will never be combined, or are they meant to be combined? > +{ > + return command == TODO_FIXUP && ((flag & TODO_REPLACE_FIXUP_MSG) || > + (flag & TODO_EDIT_FIXUP_MSG)); > +} This code adds to the confusion. In the function argument list, `flag` has been declared as a single enum item, yet this code is treating `flag` as if it is a combination of bits. So, it's not clear what the intention is here. Is `flag` always going to be a specific enum item in this context or is it going to be a combination of bits? If it is only ever going to be a distinct enum item, then one would expect this code to be written like this: return command == TODO_FIXUP && (flag == TODO_REPLACE_FIXUP_MSG || flag == TODO_EDIT_FIXUP_MSG); Otherwise, if `flag` will actually be a bag of bits, then the argument should be declared as such: static int check_fixup_flag(enum todo_command command, unsigned flag) By the way, the function name check_fixup_flag() doesn't necessarily do a good job conveying the purpose of this function. Based upon the implementation, I gather that it is checking whether the command is a "fixup" command, so perhaps the name could reflect that. Perhaps is_fixup() or something? > +static int append_squash_message(struct strbuf *buf, const char *body, > + enum todo_command command, struct replay_opts *opts, > + enum todo_item_flags flag) > +{ > + /* > + * amend is non-interactive and not normally used with fixup! > + * or squash! commits, so only comment out those subjects when > + * squashing commit messages. > + */ > + if (starts_with(body, "amend!") || > + ((command == TODO_SQUASH || seen_squash(opts)) && > + (starts_with(body, "squash!") || starts_with(body, "fixup!")))) > commented_len = subject_length(body); I understand from the cover letter that "amend!" is being added by this series, however, no patch up to this point, nor this patch itself, adds "amend!" functionality, so it's surprising to see it being tested here. As a reader, I would expect code/comments related to "amend!" to be added in the patch which actually introduces "amend!" rather than doing it here. > + /* fixup -C after squash behaves like squash */ > + if (check_fixup_flag(command, flag) && !seen_squash(opts)) { > + if (opts->signoff) > + append_signoff(buf, 0, 0); > + > + if ((command == TODO_FIXUP) && > + (flag & TODO_REPLACE_FIXUP_MSG) && > + (file_exists(rebase_path_fixup_msg()) || > + !file_exists(rebase_path_squash_msg()))) { Is the expression `command == TODO_FIXUP` redundant here considering that the only way we got this far is if check_fixup_flag() returned true, in which case we know that command is TODO_FIXUP? Or am I missing something? > @@ -2281,6 +2436,18 @@ static int parse_insn_line(struct repository *r, struct todo_item *item, > + if (item->command == TODO_FIXUP) { > + if (skip_prefix(bol, "-C", &bol) && > + (*bol == ' ' || *bol == '\t')) { > + bol += strspn(bol, " \t"); > + item->flags |= TODO_REPLACE_FIXUP_MSG; > + } else if (skip_prefix(bol, "-c", &bol) && > + (*bol == ' ' || *bol == '\t')) { > + bol += strspn(bol, " \t"); > + item->flags |= TODO_EDIT_FIXUP_MSG; > + } > + } I was wondering if the above could be rephrased like this to avoid the repetition: if (bol[0] == '-' && tolower(bol[1]) == 'c' && (bol[2] == ' ' || bol[2] == '\t') { item->flags |= bol[1] == 'C' ? TODO_REPLACE_FIXUP_MSG : TODO_EDIT_FIXUP_MSG; bol += 2 + strspn(bol + 2, " \t"); } but perhaps it's too magical and ugly.
Hi Eric, On Tue, 2 Feb 2021 at 06:17, Eric Sunshine <sunshine@sunshineco.com> wrote: > > On Fri, Jan 29, 2021 at 1:24 PM Charvi Mendiratta <charvi077@gmail.com> wrote: > > Add options to `fixup` command to fixup both the commit contents and > > message. `fixup -C` command is used to replace the original commit > > message and `fixup -c`, additionally allows to edit the commit message. > > In the cover letter for this series, you had this additional information: > > This convention is similar to the existing `merge` command in the > interactive rebase, that also supports `-c` and `-C` options with > similar meanings. > > which helps to explain the choice of -c and -C. It might be nice to > include that explanation in this commit message, as well (but not > itself worth a re-roll). > Agree, I will include this in the commit message. > > Signed-off-by: Charvi Mendiratta <charvi077@gmail.com> > > --- > > diff --git a/rebase-interactive.c b/rebase-interactive.c > > @@ -44,7 +44,9 @@ void append_todo_help(int command_count, > > "s, squash <commit> = use commit, but meld into previous commit\n" > > -"f, fixup <commit> = like \"squash\", but discard this commit's log message\n" > > +"f, fixup [-C | -c] <commit> = like \"squash\", but discard this\n" > > +" commit's log message. Use -C to replace with this\n" > > +" commit message or -c to edit the commit message\n" > > This change jarringly introduces the first and only use of a period > and capitalized word in the to-do help text. Perhaps instead say: > > ... like \"squash\", but discard this > commit's log message; use -C to replace with this > commit message or -c to edit the commit message > Okay, I will change it. > When `-c` says "edit the commit message" it's not clear what will be > edited. The original's commit message? The replacement's message? A > combination of the two? If you can come up with a succinct way to word > it such that it states more precisely what exactly will be edited, it > would be nice, but not necessarily worth a re-roll. > Here the editor shows the commented out commit message of original commit and the replacement commit message (of fixup -c commit) is not commented out. So maybe s/edit the commit message/edit this commit message is better. > > diff --git a/sequencer.c b/sequencer.c > > @@ -1718,6 +1718,12 @@ static int is_pick_or_similar(enum todo_command command) > > +enum todo_item_flags { > > + TODO_EDIT_MERGE_MSG = (1 << 0), > > + TODO_REPLACE_FIXUP_MSG = (1 << 1), > > + TODO_EDIT_FIXUP_MSG = (1 << 2), > > +}; > > I'm confused. These enumeration items are defined as bit flags, > implying that they may be combined, however... > > > @@ -1734,32 +1740,176 @@ static size_t subject_length(const char *body) > > +static int check_fixup_flag(enum todo_command command, > > + enum todo_item_flags flag) > > ...here and elsewhere, you declare the argument as a strict > `todo_item_flags` enum item rather than `unsigned` which is the > typical declaration when combining bit flag values. So, the picture > thus far is confusing. Are the `todo_item_flags` values distinct > unique values which will never be combined, or are they meant to be > combined? > > > +{ > > + return command == TODO_FIXUP && ((flag & TODO_REPLACE_FIXUP_MSG) || > > + (flag & TODO_EDIT_FIXUP_MSG)); > > +} > > This code adds to the confusion. In the function argument list, `flag` > has been declared as a single enum item, yet this code is treating > `flag` as if it is a combination of bits. So, it's not clear what the > intention is here. Is `flag` always going to be a specific enum item > in this context or is it going to be a combination of bits? If it is > only ever going to be a distinct enum item, then one would expect this > code to be written like this: > > return command == TODO_FIXUP && > (flag == TODO_REPLACE_FIXUP_MSG || > flag == TODO_EDIT_FIXUP_MSG); > > Otherwise, if `flag` will actually be a bag of bits, then the argument > should be declared as such: > > static int check_fixup_flag(enum todo_command command, > unsigned flag) > I admit it resulted in a bit of confusion. Here, its true that flag is always going to be specific enum item( as command can be merge -c, fixup -c, or fixup -C ) and I combined the bag of bits to denote the specific enum item. So, maybe we can go with the first method? > By the way, the function name check_fixup_flag() doesn't necessarily > do a good job conveying the purpose of this function. Based upon the > implementation, I gather that it is checking whether the command is a > "fixup" command, so perhaps the name could reflect that. Perhaps > is_fixup() or something? > Agree, here it's checking if the command is fixup and the flag value ( which implies either user has given command fixup -c or fixup -C ) So, I wonder if we can write is_fixup_flag() ? > > +static int append_squash_message(struct strbuf *buf, const char *body, > > + enum todo_command command, struct replay_opts *opts, > > + enum todo_item_flags flag) > > +{ > > + /* > > + * amend is non-interactive and not normally used with fixup! > > + * or squash! commits, so only comment out those subjects when > > + * squashing commit messages. > > + */ > > + if (starts_with(body, "amend!") || > > + ((command == TODO_SQUASH || seen_squash(opts)) && > > + (starts_with(body, "squash!") || starts_with(body, "fixup!")))) > > commented_len = subject_length(body); > > I understand from the cover letter that "amend!" is being added by > this series, however, no patch up to this point, nor this patch > itself, adds "amend!" functionality, so it's surprising to see it > being tested here. As a reader, I would expect code/comments related > to "amend!" to be added in the patch which actually introduces > "amend!" rather than doing it here. > This patch series does not include the implementation of amend! commit. I think to avoid the confusion I will remove this part from this patch series and add it in the next patch series for amend! commit. > > + /* fixup -C after squash behaves like squash */ > > + if (check_fixup_flag(command, flag) && !seen_squash(opts)) { > > + if (opts->signoff) > > + append_signoff(buf, 0, 0); > > + > > + if ((command == TODO_FIXUP) && > > + (flag & TODO_REPLACE_FIXUP_MSG) && > > + (file_exists(rebase_path_fixup_msg()) || > > + !file_exists(rebase_path_squash_msg()))) { > > Is the expression `command == TODO_FIXUP` redundant here considering > that the only way we got this far is if check_fixup_flag() returned > true, in which case we know that command is TODO_FIXUP? Or am I > missing something? Yes, it implies the same. > > > @@ -2281,6 +2436,18 @@ static int parse_insn_line(struct repository *r, struct todo_item *item, > > + if (item->command == TODO_FIXUP) { > > + if (skip_prefix(bol, "-C", &bol) && > > + (*bol == ' ' || *bol == '\t')) { > > + bol += strspn(bol, " \t"); > > + item->flags |= TODO_REPLACE_FIXUP_MSG; > > + } else if (skip_prefix(bol, "-c", &bol) && > > + (*bol == ' ' || *bol == '\t')) { > > + bol += strspn(bol, " \t"); > > + item->flags |= TODO_EDIT_FIXUP_MSG; > > + } > > + } > > I was wondering if the above could be rephrased like this to avoid the > repetition: > > if (bol[0] == '-' && tolower(bol[1]) == 'c' && > (bol[2] == ' ' || bol[2] == '\t') { > item->flags |= bol[1] == 'C' ? > TODO_REPLACE_FIXUP_MSG : > TODO_EDIT_FIXUP_MSG; > bol += 2 + strspn(bol + 2, " \t"); > } > > but perhaps it's too magical and ugly. I agree, this [tolower(bol[1]) == 'c'] is actually doing all the magic, but I am not sure if we should change it or not ? As in the source code just after this code we are checking in a similar way for the 'merge' command. So, maybe implementing in a similar way is easier to read ? Thanks and Regards, Charvi
On Tue, Feb 2, 2021 at 10:29 AM Charvi Mendiratta <charvi077@gmail.com> wrote: > On Tue, 2 Feb 2021 at 06:17, Eric Sunshine <sunshine@sunshineco.com> wrote: > > When `-c` says "edit the commit message" it's not clear what will be > > edited. The original's commit message? The replacement's message? A > > combination of the two? If you can come up with a succinct way to word > > it such that it states more precisely what exactly will be edited, it > > would be nice, but not necessarily worth a re-roll. > > Here the editor shows the commented out commit message of original commit and > the replacement commit message (of fixup -c commit) is not commented out. > > So maybe s/edit the commit message/edit this commit message is better. Yes, that would be clearer and is just as succinct. > > This code adds to the confusion. In the function argument list, `flag` > > has been declared as a single enum item, yet this code is treating > > `flag` as if it is a combination of bits. So, it's not clear what the > > intention is here. Is `flag` always going to be a specific enum item > > in this context or is it going to be a combination of bits? If it is > > only ever going to be a distinct enum item, then one would expect this > > code to be written like this: > > > > return command == TODO_FIXUP && > > (flag == TODO_REPLACE_FIXUP_MSG || > > flag == TODO_EDIT_FIXUP_MSG); > > I admit it resulted in a bit of confusion. Here, its true that flag is always > going to be specific enum item( as command can be merge -c, fixup -c, or > fixup -C ) and I combined the bag of bits to denote > the specific enum item. So, maybe we can go with the first method? Sounds fine. It would clarify the intent. > > By the way, the function name check_fixup_flag() doesn't necessarily > > do a good job conveying the purpose of this function. Based upon the > > implementation, I gather that it is checking whether the command is a > > "fixup" command, so perhaps the name could reflect that. Perhaps > > is_fixup() or something? > > Agree, here it's checking if the command is fixup and the flag value ( > which implies either user has given command fixup -c or fixup -C ) > So, I wonder if we can write is_fixup_flag() ? Reasonable. > > I was wondering if the above could be rephrased like this to avoid the > > repetition: > > [...] > > but perhaps it's too magical and ugly. > > I agree, this [tolower(bol[1]) == 'c'] is actually doing all the > magic, but I am not > sure if we should change it or not ? As in the source code just after > this code we > are checking in a similar way for the 'merge' command. So, maybe implementing > in a similar way is easier to read ? Keeping it similar to nearby code makes sense.
On Wed, 3 Feb 2021 at 10:35, Eric Sunshine <sunshine@sunshineco.com> wrote: > > > return command == TODO_FIXUP && > > > (flag == TODO_REPLACE_FIXUP_MSG || > > > flag == TODO_EDIT_FIXUP_MSG); > > > > I admit it resulted in a bit of confusion. Here, its true that flag is always > > going to be specific enum item( as command can be merge -c, fixup -c, or > > fixup -C ) and I combined the bag of bits to denote > > the specific enum item. So, maybe we can go with the first method? > > Sounds fine. It would clarify the intent. > (Apology for confusion) After, looking again at the source code, as we are using the flag element of the structure todo_item of in sequencer.h. So, I think right way is to let it be in binary only and change type from 'enum todo_item_flag' to 'unsigned' , as you suggested below (better than first method) : Otherwise, if `flag` will actually be a bag of bits, then the argument should be declared as such: static int check_fixup_flag(enum todo_command command, unsigned flag) > > Agree, here it's checking if the command is fixup and the flag value ( > > which implies either user has given command fixup -c or fixup -C ) > > So, I wonder if we can write is_fixup_flag() ? > > Reasonable. > [...] > > I agree, this [tolower(bol[1]) == 'c'] is actually doing all the > > magic, but I am not > > sure if we should change it or not ? As in the source code just after > > this code we > > are checking in a similar way for the 'merge' command. So, maybe implementing > > in a similar way is easier to read ? > > Keeping it similar to nearby code makes sense. Thanks for confirming! Thanks and Regards, Charvi
On Wed, Feb 3, 2021 at 7:01 PM Charvi Mendiratta <charvi077@gmail.com> wrote: > > > I admit it resulted in a bit of confusion. Here, its true that flag is always > > > going to be specific enum item( as command can be merge -c, fixup -c, or > > > fixup -C ) and I combined the bag of bits to denote > > > the specific enum item. So, maybe we can go with the first method? > > > > Sounds fine. It would clarify the intent. > > (Apology for confusion) After, looking again at the source code, as we are using > the flag element of the structure todo_item of in sequencer.h. So, I > think right > way is to let it be in binary only and change type from 'enum todo_item_flag' to > 'unsigned' , as you suggested below (better than first method) : That would be fine, as well. Thanks.
diff --git a/rebase-interactive.c b/rebase-interactive.c index 762853bc7e..c3bd02adee 100644 --- a/rebase-interactive.c +++ b/rebase-interactive.c @@ -44,7 +44,9 @@ void append_todo_help(int command_count, "r, reword <commit> = use commit, but edit the commit message\n" "e, edit <commit> = use commit, but stop for amending\n" "s, squash <commit> = use commit, but meld into previous commit\n" -"f, fixup <commit> = like \"squash\", but discard this commit's log message\n" +"f, fixup [-C | -c] <commit> = like \"squash\", but discard this\n" +" commit's log message. Use -C to replace with this\n" +" commit message or -c to edit the commit message\n" "x, exec <command> = run command (the rest of the line) using shell\n" "b, break = stop here (continue rebase later with 'git rebase --continue')\n" "d, drop <commit> = remove commit\n" diff --git a/sequencer.c b/sequencer.c index 6d9a10afcf..46e11d20e8 100644 --- a/sequencer.c +++ b/sequencer.c @@ -1718,6 +1718,12 @@ static int is_pick_or_similar(enum todo_command command) } } +enum todo_item_flags { + TODO_EDIT_MERGE_MSG = (1 << 0), + TODO_REPLACE_FIXUP_MSG = (1 << 1), + TODO_EDIT_FIXUP_MSG = (1 << 2), +}; + static size_t subject_length(const char *body) { const char *p = body; @@ -1734,32 +1740,176 @@ static size_t subject_length(const char *body) static const char first_commit_msg_str[] = N_("This is the 1st commit message:"); static const char nth_commit_msg_fmt[] = N_("This is the commit message #%d:"); +static const char skip_first_commit_msg_str[] = N_("The 1st commit message will be skipped:"); static const char skip_nth_commit_msg_fmt[] = N_("The commit message #%d will be skipped:"); static const char combined_commit_msg_fmt[] = N_("This is a combination of %d commits."); -static void append_squash_message(struct strbuf *buf, const char *body, - struct replay_opts *opts) +static int check_fixup_flag(enum todo_command command, + enum todo_item_flags flag) +{ + return command == TODO_FIXUP && ((flag & TODO_REPLACE_FIXUP_MSG) || + (flag & TODO_EDIT_FIXUP_MSG)); +} + +/* + * Wrapper around strbuf_add_commented_lines() which avoids double + * commenting commit subjects. + */ +static void add_commented_lines(struct strbuf *buf, const void *str, size_t len) +{ + const char *s = str; + while (len > 0 && s[0] == comment_line_char) { + size_t count; + const char *n = memchr(s, '\n', len); + if (!n) + count = len; + else + count = n - s + 1; + strbuf_add(buf, s, count); + s += count; + len -= count; + } + strbuf_add_commented_lines(buf, s, len); +} + +/* Does the current fixup chain contain a squash command? */ +static int seen_squash(struct replay_opts *opts) +{ + return starts_with(opts->current_fixups.buf, "squash") || + strstr(opts->current_fixups.buf, "\nsquash"); +} + +static void update_comment_bufs(struct strbuf *buf1, struct strbuf *buf2, int n) { - size_t commented_len = 0; + strbuf_setlen(buf1, 2); + strbuf_addf(buf1, _(nth_commit_msg_fmt), n); + strbuf_addch(buf1, '\n'); + strbuf_setlen(buf2, 2); + strbuf_addf(buf2, _(skip_nth_commit_msg_fmt), n); + strbuf_addch(buf2, '\n'); +} - unlink(rebase_path_fixup_msg()); - if (starts_with(body, "squash!") || starts_with(body, "fixup!")) +/* + * Comment out any un-commented commit messages, updating the message comments + * to say they will be skipped but do not comment out the empty lines that + * surround commit messages and their comments. + */ +static void update_squash_message_for_fixup(struct strbuf *msg) +{ + void (*copy_lines)(struct strbuf *, const void *, size_t) = strbuf_add; + struct strbuf buf1 = STRBUF_INIT, buf2 = STRBUF_INIT; + const char *s, *start; + char *orig_msg; + size_t orig_msg_len; + int i = 1; + + strbuf_addf(&buf1, "# %s\n", _(first_commit_msg_str)); + strbuf_addf(&buf2, "# %s\n", _(skip_first_commit_msg_str)); + s = start = orig_msg = strbuf_detach(msg, &orig_msg_len); + while (s) { + const char *next; + size_t off; + if (skip_prefix(s, buf1.buf, &next)) { + /* + * Copy the last message, preserving the blank line + * preceding the current line + */ + off = (s > start + 1 && s[-2] == '\n') ? 1 : 0; + copy_lines(msg, start, s - start - off); + if (off) + strbuf_addch(msg, '\n'); + /* + * The next message needs to be commented out but the + * message header is already commented out so just copy + * it and the blank line that follows it. + */ + strbuf_addbuf(msg, &buf2); + if (*next == '\n') + strbuf_addch(msg, *next++); + start = s = next; + copy_lines = add_commented_lines; + update_comment_bufs(&buf1, &buf2, ++i); + } else if (skip_prefix(s, buf2.buf, &next)) { + off = (s > start + 1 && s[-2] == '\n') ? 1 : 0; + copy_lines(msg, start, s - start - off); + start = s - off; + s = next; + copy_lines = strbuf_add; + update_comment_bufs(&buf1, &buf2, ++i); + } else { + s = strchr(s, '\n'); + if (s) + s++; + } + } + copy_lines(msg, start, orig_msg_len - (start - orig_msg)); + free(orig_msg); + strbuf_release(&buf1); + strbuf_release(&buf2); +} + +static int append_squash_message(struct strbuf *buf, const char *body, + enum todo_command command, struct replay_opts *opts, + enum todo_item_flags flag) +{ + const char *fixup_msg; + size_t commented_len = 0, fixup_off; + /* + * amend is non-interactive and not normally used with fixup! + * or squash! commits, so only comment out those subjects when + * squashing commit messages. + */ + if (starts_with(body, "amend!") || + ((command == TODO_SQUASH || seen_squash(opts)) && + (starts_with(body, "squash!") || starts_with(body, "fixup!")))) commented_len = subject_length(body); + strbuf_addf(buf, "\n%c ", comment_line_char); strbuf_addf(buf, _(nth_commit_msg_fmt), ++opts->current_fixup_count + 1); strbuf_addstr(buf, "\n\n"); strbuf_add_commented_lines(buf, body, commented_len); + /* buf->buf may be reallocated so store an offset into the buffer */ + fixup_off = buf->len; strbuf_addstr(buf, body + commented_len); + + /* fixup -C after squash behaves like squash */ + if (check_fixup_flag(command, flag) && !seen_squash(opts)) { + /* + * We're replacing the commit message so we need to + * append the Signed-off-by: trailer if the user + * requested '--signoff'. + */ + if (opts->signoff) + append_signoff(buf, 0, 0); + + if ((command == TODO_FIXUP) && + (flag & TODO_REPLACE_FIXUP_MSG) && + (file_exists(rebase_path_fixup_msg()) || + !file_exists(rebase_path_squash_msg()))) { + fixup_msg = skip_blank_lines(buf->buf + fixup_off); + if (write_message(fixup_msg, strlen(fixup_msg), + rebase_path_fixup_msg(), 0) < 0) + return error(_("cannot write '%s'"), + rebase_path_fixup_msg()); + } else { + unlink(rebase_path_fixup_msg()); + } + } else { + unlink(rebase_path_fixup_msg()); + } + + return 0; } static int update_squash_messages(struct repository *r, enum todo_command command, struct commit *commit, - struct replay_opts *opts) + struct replay_opts *opts, + enum todo_item_flags flag) { struct strbuf buf = STRBUF_INIT; - int res; + int res = 0; const char *message, *body; const char *encoding = get_commit_output_encoding(); @@ -1779,6 +1929,8 @@ static int update_squash_messages(struct repository *r, opts->current_fixup_count + 2); strbuf_splice(&buf, 0, eol - buf.buf, header.buf, header.len); strbuf_release(&header); + if (check_fixup_flag(command, flag) && !seen_squash(opts)) + update_squash_message_for_fixup(&buf); } else { struct object_id head; struct commit *head_commit; @@ -1792,18 +1944,22 @@ static int update_squash_messages(struct repository *r, return error(_("could not read HEAD's commit message")); find_commit_subject(head_message, &body); - if (command == TODO_FIXUP && write_message(body, strlen(body), + if (command == TODO_FIXUP && !flag && write_message(body, strlen(body), rebase_path_fixup_msg(), 0) < 0) { unuse_commit_buffer(head_commit, head_message); return error(_("cannot write '%s'"), rebase_path_fixup_msg()); } - strbuf_addf(&buf, "%c ", comment_line_char); strbuf_addf(&buf, _(combined_commit_msg_fmt), 2); strbuf_addf(&buf, "\n%c ", comment_line_char); - strbuf_addstr(&buf, _(first_commit_msg_str)); + strbuf_addstr(&buf, check_fixup_flag(command, flag) ? + _(skip_first_commit_msg_str) : + _(first_commit_msg_str)); strbuf_addstr(&buf, "\n\n"); - strbuf_addstr(&buf, body); + if (check_fixup_flag(command, flag)) + strbuf_add_commented_lines(&buf, body, strlen(body)); + else + strbuf_addstr(&buf, body); unuse_commit_buffer(head_commit, head_message); } @@ -1813,8 +1969,8 @@ static int update_squash_messages(struct repository *r, oid_to_hex(&commit->object.oid)); find_commit_subject(message, &body); - if (command == TODO_SQUASH) { - append_squash_message(&buf, body, opts); + if (command == TODO_SQUASH || check_fixup_flag(command, flag)) { + res = append_squash_message(&buf, body, command, opts, flag); } else if (command == TODO_FIXUP) { strbuf_addf(&buf, "\n%c ", comment_line_char); strbuf_addf(&buf, _(skip_nth_commit_msg_fmt), @@ -1825,7 +1981,9 @@ static int update_squash_messages(struct repository *r, return error(_("unknown command: %d"), command); unuse_commit_buffer(commit, message); - res = write_message(buf.buf, buf.len, rebase_path_squash_msg(), 0); + if (!res) + res = write_message(buf.buf, buf.len, rebase_path_squash_msg(), + 0); strbuf_release(&buf); if (!res) { @@ -2026,7 +2184,8 @@ static int do_pick_commit(struct repository *r, if (command == TODO_REWORD) reword = 1; else if (is_fixup(command)) { - if (update_squash_messages(r, command, commit, opts)) + if (update_squash_messages(r, command, commit, + opts, item->flags)) return -1; flags |= AMEND_MSG; if (!final_fixup) @@ -2191,10 +2350,6 @@ static int read_and_refresh_cache(struct repository *r, return 0; } -enum todo_item_flags { - TODO_EDIT_MERGE_MSG = 1 -}; - void todo_list_release(struct todo_list *todo_list) { strbuf_release(&todo_list->buf); @@ -2281,6 +2436,18 @@ static int parse_insn_line(struct repository *r, struct todo_item *item, return 0; } + if (item->command == TODO_FIXUP) { + if (skip_prefix(bol, "-C", &bol) && + (*bol == ' ' || *bol == '\t')) { + bol += strspn(bol, " \t"); + item->flags |= TODO_REPLACE_FIXUP_MSG; + } else if (skip_prefix(bol, "-c", &bol) && + (*bol == ' ' || *bol == '\t')) { + bol += strspn(bol, " \t"); + item->flags |= TODO_EDIT_FIXUP_MSG; + } + } + if (item->command == TODO_MERGE) { if (skip_prefix(bol, "-C", &bol)) bol += strspn(bol, " \t"); @@ -5287,6 +5454,14 @@ static void todo_list_to_strbuf(struct repository *r, struct todo_list *todo_lis short_commit_name(item->commit) : oid_to_hex(&item->commit->object.oid); + if (item->command == TODO_FIXUP) { + if (item->flags & TODO_EDIT_FIXUP_MSG) + strbuf_addstr(buf, " -c"); + else if (item->flags & TODO_REPLACE_FIXUP_MSG) { + strbuf_addstr(buf, " -C"); + } + } + if (item->command == TODO_MERGE) { if (item->flags & TODO_EDIT_MERGE_MSG) strbuf_addstr(buf, " -c");
Add options to `fixup` command to fixup both the commit contents and message. `fixup -C` command is used to replace the original commit message and `fixup -c`, additionally allows to edit the commit message. Original-patch-by: Phillip Wood <phillip.wood@dunelm.org.uk> Mentored-by: Christian Couder <chriscool@tuxfamily.org> Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk> Signed-off-by: Charvi Mendiratta <charvi077@gmail.com> --- rebase-interactive.c | 4 +- sequencer.c | 213 +++++++++++++++++++++++++++++++++++++++---- 2 files changed, 197 insertions(+), 20 deletions(-)