On 06/11/2023 11:06, Phillip Wood wrote:
On 05/11/2023 00:08, Andy Koppe wrote:
So far, the rebase --autosquash option and rebase.autoSquash=true
config setting are quietly ignored when used without --interactive,
Thanks for working on this, I agree that "--autosquash" being ignored
without "--interactive" is something that we should address. I think
there are several possible solutions
1 - make "--autosquash" imply "--interactive". This has the advantage
that the user gets to check the commits are going to be reordered as
they expect when they edit the todo list. It is hard to see how to
accommodate the config setting though - I don't think we want
"rebase.autosquash=true" to imply "--interactive".
2 - make "--autosquash" without "--interactive" an error. This would
prevent the user being surprised that their commits are not squashed
by a non-interactive rebase. Users who have set
"rebase.autosquash=true" would have to pass "--no-autosquash" to
perform any form of non-interactive rebase. This is similar to the
current behavior where the user has to pass "--no-autosquash" if
they want to use the apply backend with "rebase.autosquash=true".
3 - make "--autosquash" rearrange and squash commits without
"--interactive". This is convenient but there is a risk in that the
user does not get a chance to check the todo list before the commits
are reordered and squashed. I think that risk is fairly small with
an explicit "--autosquash" on the commandline. This is the approach
taken by this patch. I do have some concerns about extending the
config setting to non-interactive rebases though. If the user has
commits that look like
fixup! foo (HEAD)
foo bar
foo
and runs "git -c rebase.autosquash=non-interactive rebase HEAD~2"
then we'll silently squash the fixup into the wrong commit due to a
prefix subject match.
Good analysis. My order of preference is 3 (obviously), 1, 2.
except that they prevent fast-forward and that they trigger conflicts
with --apply and relatives, which is less than helpful particularly for
the config setting.
The behavior to make the config setting incompatible with the apply
backend was implemented to avoid users being surprised that their
commits are not squashed by that backend even when they have set
"rebase.autosquash=true"[1]. I think one could consider "--autosquash"
being silently ignored without "--interactive" to be an oversight in
796abac7e1 (rebase: add coverage of other incompatible options,
2023-01-25) that introduced that change.
[1]
https://lore.kernel.org/git/pull.1466.v5.git.1674619434.gitgitgadget@xxxxxxxxx/
Since the "merge" backend used for interactive rebase also is the
default for non-interactive rebase, there doesn't appear to be a
reason not to do --autosquash without --interactive, so support that.
I think making "--autosquash" on the commandline work for
non-interactive rebases is reasonable but I would be open to the
argument that it would be better to make it an error and require
"--interactive" to allow the user to check that the commits are going to
be reordered as they expect.
I found that once I got used to and started trusting the feature,
particularly in connection with the corresponding git-commit support, I
no longer felt the need to check the todo list as I'd inspect the log
afterwards anyway. And of course there's always resetting to ORIG_HEAD
when things do go wrong.
So I think users should be trusted with this, especially as it's not a
particularly dangerous feature, given it requires the squash markers to
be present in the first place to do anything.
Turn rebase.autoSquash into a comma-separated list of flags, with
"interactive" or "i" enabling auto-squashing with --interactive, and
"no-interactive" or "no-i" enabling it without. Make boolean true mean
"interactive" for backward compatibility.
Please, please, please don't introduce abbreviated config settings, it
just makes the interface more complicated. The user only has to set this
once so I think the short names just add confusion.
Duly noted.
I also think
"non-interactive" would be a better name for the config setting
corresponding to non-interactive rebases. Does this mean the user can
request that commits are only rearranged when they do not pass
"--interactive"?
Yes. That doesn't seem useful.
As I said above I do have some concerns that the
"rebase.autosquash=non-interactive" setting will catch people out.
I think you're right, so I've gone back to interpreting it as a boolean,
but officially make it affect interactive mode only.
Having said that ignoring "rebase.autosquash=true" without
"--interactive" as we do now is inconsistent with the behavior of
"rebase.autosquash=true" with "--apply". One possibility would be to
introduce "rebase.autosquash=interactive" which would not cause an error
with "--apply" and always require an explicit "--autosquash" on the
commandline to squash fixups without "--interactive"
I don't think different error behaviour is worth a separate setting, as
we can't change rebase.autosquash=true to do auto-squashing without
--interactive without surprising people.
Don't prevent fast-forwards or report conflicts with --apply options
when auto-squashing is not active.
I think this change deserves to be in a separate commit (which probably
means separating out the config changes into that commit) as it is not
directly related to fixing "--autosquash" without "--interactive" on the
commandline.
Done in v4.
In summary I like "--autosquash" working without "--interactive" but I'm
unsure about the config changes.
Thanks very much for the thoughtful review!
Regards,
Andy
