From: Andy Koppe Date: Sat, 11 Nov 2023 14:08:05 GMT Subject: Re: [PATCH v3 1/2] rebase: support non-interactive autosquash Message-ID: <8dba0a09-d5bf-46b8-835b-9855f4f4326a@gmail.com> In-Reply-To: <8c2bb219-127c-4128-99ed-158bc64b1dab@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