On Thu, Mar 16, 2023 at 4:39 PM Glen Choo [off-list ref] wrote:

Alex Henrie [off-list ref] writes:

quoted

quoted

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.

quoted

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 [off-list ref]' 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