tummychow / git-absorb

git commit --fixup, but automatic
https://crates.io/crates/git-absorb
BSD 3-Clause "New" or "Revised" License
3.35k stars 59 forks source link

autostage: automatically add everything to index if index was empty #98

Closed kiprasmel closed 6 months ago

kiprasmel commented 6 months ago

often times, i just want to try and absorb everything.

right now, this means an extra step of git add . before git absorb, and potentially a git reset @ later if not everything was fixed up.

in this patch we remove both of the extra step for convenience: if nothing was staged, we stage everything, try to create fixups, and whatever's left uncommitted - we unstage.

tummychow commented 6 months ago

sorry but i'm not merging this unless it's behind a flag. the implemented behavior is surprising and counterintuitive when compared to official git commands, eg git commit with an empty index is a noop unless you pass --all.

tummychow commented 6 months ago

by the way, if you're going to implement a git absorb --all, i have to warn you that the correct behavior is not obvious to me. unlike git commit, git absorb's behavior is conditional on the other commits it's absorbing into. for instance, let's say i have an empty index and i run git absorb --all, and some of my unstaged changes get absorbed but some do not. do i expect those changes to remain staged after the command finishes, or should they go back to being unstaged? what if i have some staged changes and some unstaged changes, and i run git absorb --all? if some of my staged changes get absorbed and some do not, should the unabsorbed ones remain staged? what about the unstaged changes that didn't get absorbed, do those go back to being unstaged? you'd have to track which hunks you staged or didn't stage to determine which unabsorbed ones should be re-unstaged

kiprasmel commented 6 months ago

hey @tummychow, sorry took so long.

re: counter-intuitive to git's behavior -- i was trying to match behavior of mercurial's absorb, i believe they've achieved a good ux. but fair enough, we can add a flag.

or rather, for starters, i'd prefer a git config option, first because that's the behavior i'd want to enable for myself by default, and second, as you mentioned, a flag like --all is not trivial to implement.

i did think for a while about the behavior of --all, i think it's doable, but i don't have bandwidth for it right now.

would you be willing to accept a git config option? something like absorb.autoAddIfNothingAdded or so? as for the behavior: a) if anything's staged by the user, it's a noop. b) if nothing's staged by the user, then we stage everything, try to absorb, and whatever's left - we unstage, i.e. back to original state. it's simple this way, unlike with --all, because there's no (c) case where there's a mix of user vs our staged changes.

cheers

tummychow commented 6 months ago

yeah config option i'll accept. you can put it on this branch

kiprasmel commented 6 months ago

nice. here we go. commit-by-commit should be easier to review

Konfekt commented 5 months ago

How about a command line parameter --auto-stage instead ?

kiprasmel commented 5 months ago

@Konfekt you can provide a temporary git config value, e.g.

git -c absorb.autoStageIfNothingStaged=true absorb -r

but at this point you might as well do a git add yourself

do you have a use-case where you want to toggle the behavior often? it's not like it's intrusive - if you have staged anything by yourself, absorb won't stage anything extra.

Konfekt commented 5 months ago

Just a git alias such as aa for absorb --auto-stage instead of a for absorb. Thank you for the hint. True, this is easily achieved by a temporary git config value as well. The thinking was that in practice, I'd expect this to be rather an occasional opt-in rather than opt-out. Adding would skip the final git reset @.

kiprasmel commented 5 months ago

no worries. i still don't see why not to default to this always, but anyhow, for an alias you can just do

[alias]
    aa = -c absorb.autoStageIfNothingStaged=true absorb

though, debugging it right now, it seems that rust's libgit wrapper, git2, doesn't recognize this variable somehow? i.e. running git -c absorb.autoStageIfNothingStaged=true absorb, absorb doesn't take in this variable..

cc @tummychow this looks like an issue for all config variables

tummychow commented 5 months ago

-c looks like an upstream issue: https://github.com/libgit2/libgit2/issues/3854

i'll accept a pr to also configure this via a cli flag since, while it's redundant, it's also pretty common in actual git commands

teto commented 4 months ago

thanks for this x100 ! is it possible to set it on the command line , e.g., git absorb -u ? git absorb --help doesn't list it.

Also I dont think the README wording is what you meant:

git-absorb will only consider files that you've staged to the index via git add. However, sometimes one wants to try and absorb from all changes, which would require to stage them first via git add .. ...

git add . adds everything not git-ignored. I believe you meant git add -u ?

Konfekt commented 4 months ago

is it possible to set it on the command line , e.g., git absorb -u

Are you suggesting -u instead of --auto-stage in https://github.com/tummychow/git-absorb/pull/98#issuecomment-1986740666 ?

teto commented 4 months ago

--auto-stage doesn't work for me and doesn't appear in help git absorb --version git-absorb 0.6.12 ? if it's already there I am happy, even though a short version would be nice, why not -u indeed.

Konfekt commented 4 months ago

Okay, so we are proposing the same change under different names. I'd be of course fine with -u. Often there's an expanded, more telling, equivalent command-line parameter.

teto commented 4 months ago

exactly I fully agree. I must have answered a bit fast without reading the full conv otherwise I would have made it clearer it was the same proposition srry

ccoVeille commented 4 months ago

Hi @Konfekt and @teto

I followed your discussion and suggestion because I was following the initial ticket.

May I suggest you to open a separate ticket, so you could reference this ticket and your request for improvement.

I'm not a maintainer of this project but you are commenting a merged ticket. And you are talking about adding a feature.

Please note, I like your idea to add -u and --auto-stage, that's why I would like to see it implemented.