On Tue, Feb 2, 2021 at 10:29 AM Charvi Mendiratta <charvi077@xxxxxxxxx> wrote: > On Tue, 2 Feb 2021 at 06:17, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> 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.
