On Fri, Jan 20, 2023 at 7:27 AM Junio C Hamano <gitster@xxxxxxxxx> wrote: > > Derrick Stolee <derrickstolee@xxxxxxxxxx> writes: > > >> + if (options.update_refs) > >> + imply_merge(&options, "--update-refs"); > >> + > > > > This solution is very elegant. The only downside is the lack of warning > > if --update-refs was implied by rebase.updateRefs=true, but I'm happy to > > delay implementing that warning in favor of your complete solution here. > > If features A and B are incompatible and both can be specified from > both command line and configuration, ideally I would expect the > system to operate in one of two ways. I agree that one of the two ways you highlight below would be ideal. Should this series be held up on extending the checks to implement this ideal, or are you just commenting for whoever later circles back to implement this? > I haven't thought it through > to decide which one I prefer between the two. > > * Take "command line trumps configuration" one step further, so > that A that is configured but not asked for from the command > line is defeated by B that is asked for from the command line. > > This way, only when A and B are both requested via the > configuration, of via the command line, we'd fail the operation > by saying A and B are incompatible. Otherwise, the one that is > configured but overridden is turned off (either silently or with > a warning). > > * Declare "command line trumps configuration" is only among the > same feature. Regardless of how features A and B that are > incompatible are requested, the command will error out, citing > incompatibility. It would be very nice if the warning mentioned > where the requests for features A and B came from (e.g. "You > asked for -B from the command line, but you have A configured, > and both cannot be active at the same time---disable A from the > command line, or do not ask for B") > > When A is configured and B is requested from the command line, > the command will error out, and the user must defeat A from the > command line before the user can use B, e.g. "git cmd --no-A -B". > > A knee-jerk reaction to the situation is that the latter feels > somewhat safer than the former, but when I imagine the actual end > user who saw the error message, especially the suggested solution > "disable A from the command line or do not ask for B from the > command line", may say "well, I asked for B for this invocation > explicitly with -B from the command line, and you(Git) should be > able to make it imply --no-A", which amounts to the same thing as > the former choice. If it is clear to the user that A and B preclude each other, then I agree with this sentiment that the former choice (silently ignoring the config) would avoid a minor frustration for some users and thus would be better. But I don't think that's applicable here. There is no reason that --whitespace=fix shouldn't be available from the merge backend other than that we haven't implemented it yet, and it's likely feasible to implement --update-refs for the apply backend with enough effort if we thought it was worth it. So, if a user sets rebase.updateRefs=true in their config because they always want included branches updated, but one time they run `git rebase --whitespace=fix`, they will likely have a negative experience like the one that inspired this patch. Perhaps we're forced to choose between possible frustration by different end users, but if so, I think trying to debug and figure out "Wait, I switched to this branch and started tweaking it but it appears to not have some relevant changes I'm sure I made to it yesterday. What happened?" is a much worse frustration than "I have to manually specify --no-A in this special case". So, when it's not at all obvious that A and B preclude each other, I think we're better off giving the error.
