git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Andy Koppe <andy.koppe@gmail.com>
Date
Nov 11, 2023, 14:08 UTC
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:
Show 38 quoted lines
> 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.
Show 24 quoted lines
>> 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.

Show 8 quoted lines
>> 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.
Show 5 quoted lines
> 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.

Show 6 quoted lines
> 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.

Show 7 quoted lines
>> 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

Previous: Phillip WoodNext: Andy Koppe
Message 11 of 32 in “rebase: support non-interactive autosquash”
  1. 1/2 rebase: support non-interactive autosquashAndy Koppe, Nov 3, 2023
  2. 2/2 docs: rewrite rebase --(no-)autosquash descriptionAndy Koppe, Nov 3, 2023
  3. Junio C HamanoNov 4, 2023
  4. Andy KoppeNov 4, 2023
  5. 1/2 rebase: support non-interactive autosquashAndy Koppe, Nov 4, 2023
  6. 2/2 docs: rewrite rebase --(no-)autosquash descriptionAndy Koppe, Nov 4, 2023
  7. 1/2 rebase: support non-interactive autosquashAndy Koppe, Nov 5, 2023
  8. 2/2 docs: rewrite rebase --(no-)autosquash descriptionAndy Koppe, Nov 5, 2023
  9. Phillip WoodNov 6, 2023
  10. Phillip WoodNov 6, 2023
  11. Andy KoppeNov 11, 2023
  12. 0/4 rebase: support --autosquash without -iAndy Koppe, Nov 11, 2023
  13. 2/4 rebase: support --autosquash without -iAndy Koppe, Nov 11, 2023
  14. Phillip WoodNov 13, 2023
  15. 1/4 rebase: fully ignore rebase.autoSquash without -iAndy Koppe, Nov 11, 2023
  16. Phillip WoodNov 13, 2023
  17. 4/4 docs: rewrite rebase --(no-)autosquash descriptionAndy Koppe, Nov 11, 2023
  18. Andy KoppeNov 11, 2023
  19. 3/4 rebase: test autosquash with and without -iAndy Koppe, Nov 11, 2023
  20. Junio C HamanoNov 13, 2023
  21. Phillip WoodNov 13, 2023
  22. 4/4 rebase: rewrite --(no-)autosquash documentationAndy Koppe, Nov 11, 2023
  23. Junio C HamanoNov 13, 2023
  24. Phillip WoodNov 13, 2023
  25. 0/3 rebase: support --autosquash without -iAndy Koppe, Nov 14, 2023
  26. 1/3 rebase: fully ignore rebase.autoSquash without -iAndy Koppe, Nov 14, 2023
  27. 2/3 rebase: support --autosquash without -iAndy Koppe, Nov 14, 2023
  28. 3/3 rebase: rewrite --(no-)autosquash documentationAndy Koppe, Nov 14, 2023
  29. Phillip WoodNov 15, 2023
  30. Junio C HamanoNov 16, 2023
  31. Junio C HamanoNov 6, 2023
  32. Andy KoppeNov 11, 2023

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.