Hi Eric,
On Tue, 2 Feb 2021 at 06:17, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
>
> On Fri, Jan 29, 2021 at 1:24 PM Charvi Mendiratta <charvi077@xxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > 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
