Re: [PATCH v3 1/2] rebase: support non-interactive autosquash

[Andy Koppe <andy.koppe@gmail.com>]

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@gmail.com/
> 
>> 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