From: Phillip Wood Date: Tue, 24 Jan 2023 10:27:53 GMT Subject: Re: [PATCH v4 5/9] rebase: add coverage of other incompatible options Message-ID: <83d27162-59d4-d8c0-fde3-f522630d024d@dunelm.org.uk> In-Reply-To: Hi Elijah On 24/01/2023 02:36, Elijah Newren wrote: > Hi Phillip, > > On Mon, Jan 23, 2023 at 12:08 PM Phillip Wood wrote: >> >> Hi Elijah >> >> On 22/01/2023 06:12, Elijah Newren via GitGitGadget wrote: >>> From: Elijah Newren >>> >>> The git-rebase manual noted several sets of incompatible options, but >>> we were missing tests for a few of these. Further, we were missing >>> code checks for some of these, which could result in command line >>> options being silently ignored. >>> >>> Also, note that adding a check for autosquash means that using >>> --whitespace=fix together with the config setting rebase.autosquash=true >>> will trigger an error. A subsequent commit will improve the error >>> message. >> >> Thanks for updating the commit message and for the new commits at the >> end of the series. >> >>> Signed-off-by: Elijah Newren >>> --- a/builtin/rebase.c >>> +++ b/builtin/rebase.c >>> @@ -1224,6 +1224,26 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) >>> if (options.fork_point < 0) >>> options.fork_point = 0; >>> } >>> + /* >>> + * The apply backend does not support --[no-]reapply-cherry-picks. >>> + * The behavior it implements by default is equivalent to >>> + * --no-reapply-cherry-picks (due to passing --cherry-picks to >>> + * format-patch), but --keep-base alters the upstream such that no >>> + * cherry-picks can be found (effectively making it act like >>> + * --reapply-cherry-picks). >>> + * >>> + * Now, if the user does specify --[no-]reapply-cherry-picks, but >>> + * does so in such a way that options.reapply_cherry_picks == >>> + * keep_base, then the behavior they get will match what they >>> + * expect despite options.reapply_cherry_picks being ignored. We >>> + * could just allow the flag in that case, but it seems better to >>> + * just alert the user that they've specified a flag that the >>> + * backend ignores. >>> + */ >> >> I'm a bit confused by this. --keep-base works with either >> --reapply-cherry-picks (which is the default if --keep-base is given) or >> --no-reapply-cherry-picks. Just below this hunk we have >> >> if (options.reapply_cherry_picks < 0) >> options.reapply_cherry_picks = keep_base; >> >> So we only set options.reapply_cherry_picks to match keep_base if the >> user did not specify -[-no]-reapply-cherry-picks on the commandline. > > options.reapply_cherry_picks is totally ignored by the apply backend, > regardless of whether it's set by the user or the setup code in > builtin/rebase.c. And if we have an option which is ignored, isn't it > nicer to provide an error message to the user if they tried to set it? > > Said another way, while users could start with these command lines: > > (Y) git rebase --whitespace=fix > (Z) git rebase --whitespace=fix --keep-base > > and modify them to include flags that would be ignored, we could allow: > > (A) git rebase --whitespace=fix --no-reapply-cherry-picks > (B) git rebase --whitespace=fix --keep-base --reapply-cherry-picks > > But we could not allow commands like > > (C) git rebase --whitespace=fix --reapply-cherry-picks > (D) git rebase --whitespace=fix --keep-base --no-reapply-cherry-picks (C) is already an error (D) is currently allowed and I think works as expected (--keep-base only implies --reapply-cherry-picks, the user is free to override that with --no-reapply-cherry-picks) so I don't see why we'd want to make it an error. > For all four cases (A)-(D), the apply backend will ignore whatever > --[no-]reapply-cherry-picks flag is provided. For (D) the flag is respected, (C) errors out, the other cases correspond to the default so it's like saying git rebase --merge --no-reapply-cherry-picks ignores the flag. Arguably it is confusing that the apply backend only supports -[-no]-reapply-cherry-picks if --keep-base is given but I'm not sure that is a good reason to reject a combination that currently works as expected. Best Wishes Phillip > For (A) and (B), the > behavior the apply backend provides happens to match what the user > is requesting, while for (C) and (D) the behavior does not match. > So we should at least reject (C) and (D). But, although we could > technically allow (A) and (B), what advantage would it provide? I > think the results of allowing those two commands would be: > > 1) Confusion by end users -- why should (C) & (D) throw errors if > (A) and (B) are accepted? That's not an easy rule to understand. > > 2) More confusion by end users -- the documentation for years has > stated that --reapply-cherry-picks is incompatible with the apply > backend, suggesting users would be surprised if at least (B) and > probably (A) didn't throw error messages. > > 3) Confusing documentation -- If we don't want to throw errors for > (A) and (B), how do we modify the "INCOMPATIBLE OPTIONS" section > of Documentation/git-rebase.txt to explain the relevant details > of when these flags are (or are not) incompatible with the apply > backend? I think it'd end up with a very verbose explanation > that likely confuses more than it helps. > > 4) Excessively complicated code -- The previous attempts to > implement this got it wrong. Prior to ce5238a690 ("rebase > --keep-base: imply --reapply-cherry-picks", 2022-10-17), the code > would error out on (B) and (C). After that commit, it would only > error out on (C). Both solutions are incorrect since they miss > (D), and I think the code just becomes hard to hard to follow in > order to only error out on both (C) and (D) without (A) and (B). > > (#2 and #3 might just be a repeat of the same issue, documentation, > but it seemed easier to write separately.) > > I think it's simpler for the code, for the documentation, and for end > users to just error out on all of (A), (B), (C), and (D). > --[no-]reapply-cherry-picks is not supported by the apply backend. > > But, given this lengthy email, perhaps I should split out the handling > of --[no-]reapply-cherry-picks into its own commit and copy some or > all of the description above into the commit message?