Re: [PATCH v6 3/3] rebase: add a config option for --rebase-merges

[Alex Henrie <alexhenrie24@gmail.com>]

On Thu, Mar 16, 2023 at 4:39 PM Glen Choo <chooglen@google.com> wrote:
>
> Alex Henrie <alexhenrie24@gmail.com> writes:
>
> >> Your doc patch explains the rules pretty clearly, but perhaps it doesn't
> >> explain this mental model clearly enough, hence the confusion. If we
> >> don't find a good way to communicate this (I think it is clear, but
> >> other reviewers seem yet unconvinced), I wouldn't mind taking Phillip's
> >> suggestion to have "--rebase-merges" override
> >> "rebase.rebaseMerges='specific-mode'".
> >
> > I got the impression that everyone, including Phillip,[1] already
> > agrees that the proposed documentation is clear about the interaction
> > between the config option and the command line option. However, it is
> > a little weird when you consider that other flags with optional
> > arguments, like `git branch --track`, unconditionally override their
> > corresponding config options.[2]
>
> Ah, I didn't consider other options like `git branch --track`. I haven't
> looked into what is the norm, but I think we should follow it (whatever
> it is).
>
> If other reviewers have a strong idea of what this norm is, I am happy
> to defer to them. If not, I can look into it given some time.
>
> > Let me ask a different but related question: If we add a
> > rebase-evil-merges mode, do you think that would be orthogonal to the
> > rebase-cousins mode?
>
> I am not an expert on this, so perhaps my opinion isn't that important
> ;)
>
> My understanding is that `[no-]rebase-cousins` affects which commits get
> rebased and onto where, whereas `rebase-evil-merges` would affect how
> the merge commits are generated (by rebasing the evil or by simply
> recreating the merges). From that perspective, it seems like yes, the
> two would be orthogonal.
>
> Hm. Maybe this means that we'd be introducing a new option, and that my
> hunch that we would change the default to `rebase-evil-merges` is more
> wrong than I expected.
>
> Though I guess this doesn't really affects what we do with the CLI
> options _now_, since the discussion is about what we do about defaults,
> and what the default is is quite immaterial.

I looked through the code and documentation and found 12 good examples
of command line flags with an optional argument that always override
their corresponding config options whether or not the optional
argument is given:

git -c branch.autoSetupMerge=inherit branch --track mynewbranch

git -c commit.gpgSign=mykey commit --gpg-sign

git -c core.sharedRepository=0666 init --shared .

git -c diff.ignoreSubmodules=untracked diff --ignore-submodules

git -c diff.submodule=diff diff --submodule

git -c format.attach=myboundary format-patch --attach HEAD^

git -c format.from='Dang Git <dang@example.org>' format-patch --from HEAD^

git -c format.thread=deep format-patch --thread HEAD~3

git -c gc.pruneExpire=1.week.ago gc --prune

git -c log.decorate=full log --decorate

git -c merge.log=1 merge --log mytopicbranch

git -c pull.rebase=interactive pull --rebase

I found 6 other similar examples where the config option is a
yes/no/auto trilean, but I don't think those are good examples because
it makes total sense for a command line flag without an argument to
override a nonspecific "auto" value in the configuration:

git -c color.diff=auto diff --color | less

git -c color.grep=auto grep --color . | less

git -c color.showbranch=auto show-branch --color master mytopicbranch | less

git -c color.ui=auto log --color | less

git -c fetch.recurseSubmodules=on-demand fetch --recurse-submodules

git -c push.gpgSign=if-asked push --signed

I found 1 good example where the config option does make a difference
if the optional argument is omitted:

git -c diff.colorMoved=plain diff --color-moved

And I found 1 other similar example, but it's not good a example
because the config option doesn't do anything unless the command line
flag is specified:

git -c diff.dirstat=lines diff --dirstat

Given 12 good examples versus 1 good counterexample, I would say that
the established convention is for the command line flag to always
override the config option. Please let me know if there are other
examples that I missed.

It's worth noting that the documentation for `git format-patch
--thread` explicitly says that format.thread takes precedence over
--thread without an argument, but that is not correct: When I tested
it, the command line argument always took precedence.

Another problem is that the documentation for `git pack-objects` does
not even mention that --unpack-unreachable has an optional argument,
and it says that the argument to --cruft-expiration is required when
it is not.

I'm not going to fix all the documentation problems, at least not
right now. However, in order to follow the established convention for
flags with optional arguments, and because we probably aren't going to
use rebase.rebaseMerges to facilitate a future change of defaults, I
will redo the patch so that --rebase-merges without an argument takes
precedence over rebase.rebaseMerges.

Thanks for the feedback. I feel more confident about the way forward now.

-Alex