Skip to content

cu apply#177

Merged
aluzzardi merged 8 commits intomainfrom
cu-merge-squash
Jul 10, 2025
Merged

cu apply#177
aluzzardi merged 8 commits intomainfrom
cu-merge-squash

Conversation

@aluzzardi
Copy link
Contributor

@aluzzardi aluzzardi commented Jul 2, 2025

No description provided.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if you pass a commit message here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing, unfortunately

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah you can in .git/SQUASH_MSG

Copy link
Contributor

@cwlbraa cwlbraa Jul 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but then it still needs a git commit after 🤔... not an ideal solution. i guess what i want is maybe closer to a git rebase -i --autosquash but non interactive and rewriting the final commit message.

@aluzzardi aluzzardi force-pushed the cu-merge-squash branch 2 times, most recently from 3a7d187 to f58a545 Compare July 3, 2025 19:28
@aluzzardi aluzzardi marked this pull request as draft July 3, 2025 19:29
@aluzzardi aluzzardi force-pushed the cu-merge-squash branch 3 times, most recently from 4870ca1 to 6a57aa3 Compare July 7, 2025 17:36
@aluzzardi aluzzardi changed the title cu merge: --squash cu apply Jul 7, 2025
@aluzzardi aluzzardi marked this pull request as ready for review July 7, 2025 17:49
@aluzzardi aluzzardi requested a review from cwlbraa July 7, 2025 19:46
@aluzzardi aluzzardi requested a review from eunomie July 7, 2025 19:47
@aluzzardi
Copy link
Contributor Author

@eunomie Do you mind taking this out for a spin and see if it replaces your manual cu diff | git apply?

There's an alternate implementation (commented) in repository.go that actually does a diff | apply instead of a merge --squash, in case this is not working fine

@tiborvass
Copy link
Contributor

tiborvass commented Jul 8, 2025

I tried both merge and apply in 46ce377.

For both cu merge and cu apply:

  • when i have unstashed changes, it's possible that i did it on purpose. It's possible that i have conflicts that i understandably need to resolve. But it's also possible that i simply forgot to stash. And in that case I wish i had a way to revert. I don't think it is even possible. For this reason, I'm tempted to err on the safe side and error out if there are unstashed changes. That will always do the right thing and we can always revisit for optimizing UX if it becomes a major pain.

For cu merge:

  • I want --no-ff: this may be a matter of taste, but I only dislike merge commits in repos where there is commit hygiene. Which is to say, I love merge commits. Especially with agents, I'm pretty sure I (personally) don't want to see a single linear history of human commits (whether nice or shitty) intertwined with shitty agent commits. I either want a linear git history with squashed AI commits, or detailed AI commits with merge commits.
  • I'm torn between advocating for default --no-ff and realizing that fast-forward is the default in git merge.
  • The very least, we need to add the --no-ff option. It's possible that this can be done as part of an overhaul of most git commands if we decide to do a pass thru of git flags (not sure that's a good idea).

I'm not familiar enough with merge strategies to try to find differences without the last commit, that is at 1df29b5.

My gut feeling is that cu merge mirrors easily the git merge command and its flags.

Other than that, this is a VERY welcome change, thank you!

@eunomie
Copy link
Member

eunomie commented Jul 8, 2025

I think to stop and raise an error if the working repo is dirty is a good idea. That's what I have regularly while using stgit, and it let the user decide what they want to do. And maybe it's possible to still add a flag like --stash so that container-use knows it has to stash before.

Regarding --no-ff. I think I'm on the same side. I really like to have merge commits, even while working alone on something. This makes clear distinction on logs, and in our case, with agents, this helps to group all commits if the user doesn't want to squash them.
I think I used to have a config in my .gitconfig to force --no-ff by default.

@eunomie
Copy link
Member

eunomie commented Jul 8, 2025

@eunomie Do you mind taking this out for a spin and see if it replaces your manual cu diff | git apply?

I tried on some (basic) changes, and it works just fine. Better than the cu diff | git apply in term of dx.

That sounds pretty good to me :-)

@aluzzardi aluzzardi force-pushed the cu-merge-squash branch 2 times, most recently from df7f512 to fd49df7 Compare July 8, 2025 21:41
@cwlbraa
Copy link
Contributor

cwlbraa commented Jul 9, 2025

i am also pro --no-ff for this application, i've not been using cu merge and part of that is because i generally prefer git merge --no-ff container-use/env-id

@aluzzardi
Copy link
Contributor Author

i am also pro --no-ff for this application, i've not been using cu merge and part of that is because i generally prefer git merge --no-ff container-use/env-id

Add a --no-ff flag to mirror git or do a --no-ff?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the answer is "maybe, if it helps with UX when combo'd with some additional commit metadata" lol, but i don't want to couple that with a bunch of other changes.

handling conflicts is nicer with merge --squash for right now, correct? like users have the ability to abort?

if yes, there's 2 desirable UXs that are mutually exclusive to the 2 impls:

  1. conflict-abortion
  2. repeated-squash-merges

it might be possible to get both via some clever behind the scenes rebasing, but that sounds like a huge faff. we may have to pick one of these 2 desirable behaviors.

@cwlbraa
Copy link
Contributor

cwlbraa commented Jul 9, 2025

Add a --no-ff flag to mirror git or do a --no-ff?

@aluzzardi lol that's a bigger question... i'd mirror for now to keep open the possibility of passing unknown args through to git.

Copy link
Contributor

@cwlbraa cwlbraa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think we go with this and iterate later.

  1. separating out merge and apply seems like good UX
  2. everything else is minutia

the only thing is that imo we should make the commit when applying.

}

return RunInteractiveGitCommand(ctx, r.userRepoPath, w, "merge", "-m", "Merge environment "+envInfo.ID, "--", "container-use/"+envInfo.ID)
return RunInteractiveGitCommand(ctx, r.userRepoPath, w, "merge", "--autostash", "--squash", "--", "container-use/"+envInfo.ID)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the beauty of "cu apply" imo is that it can make a commit without "overriding" the meaning of git merge flags, so if we're doing --squash, let's make the commit if there's no conflict?

later, we can add logic to edit .git/SQUASH_MSG with the llm-updated summary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this flag was applied to cu merge -- it replaces the old broken logic of the stash + defer stash pop. Doesn't apply to cu apply, unrelated improvement

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think we're miscommunicating here, one of both of us is misunderstanding lol --- what i'm asking isn't related to autostash, i'm assuming that with this PR, cu apply will leave the user with a bunch of staged files but no commit message, right? so what i'm trying to suggest is that cu apply should just go ahead and make a commit if this merge --squash succeeds.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh nm, i see your response below, indeed miscommunication (i'm the one misunderstanding XD)

@aluzzardi
Copy link
Contributor Author

the only thing is that imo we should make the commit when applying.

I'm unsure about that because:

  1. it's going to be another magic thing (like if you staged some other changes, they'll come along)
  2. Feels a bit redundant with cu merge (as in, this is what I'd expect cu merge --squash to do, except it doesn't)
  3. I think we need something that gives you the ability to tweak stuff before committing, otherwise you'd end up still doingI diff | apply (or soft reset)
  4. Messages (even once we have fancier ones) will still need to be reworded by the human. apply + commit is dumb proof compared to apply + commit --amend

IMHO there should either be a apply --commit flag or --no-commit flag, but the "just grab the stuff and put it here" functionality should still exist somehow.

@aluzzardi aluzzardi force-pushed the cu-merge-squash branch 2 times, most recently from 555adce to 3e6e50b Compare July 9, 2025 21:56
Andrea Luzzardi added 7 commits July 9, 2025 15:25
Signed-off-by: Andrea Luzzardi <al@dagger.io>
Signed-off-by: Andrea Luzzardi <al@dagger.io>
Signed-off-by: Andrea Luzzardi <al@dagger.io>
Signed-off-by: Andrea Luzzardi <al@dagger.io>
Signed-off-by: Andrea Luzzardi <al@dagger.io>
Signed-off-by: Andrea Luzzardi <al@dagger.io>
Signed-off-by: Andrea Luzzardi <al@dagger.io>
Signed-off-by: Andrea Luzzardi <al@dagger.io>
@cwlbraa
Copy link
Contributor

cwlbraa commented Jul 10, 2025

@aluzzardi lgtm based on your commment:

it's going to be another magic thing (like if you staged some other changes, they'll come along)

ugh, good point, didn't think of that edge case, just thought our shit was handled by --autostash but i imagine that doesn't stash staged files?

IMHO there should either be a apply --commit flag or --no-commit flag, but the "just grab the stuff and put it here" functionality should still exist somehow.

works for me.

@vito
Copy link
Contributor

vito commented Jul 10, 2025

(@aluzzardi asked for my 2c in Discord, so this may be a bit of a non sequitur:)

IMO if we have commands that match git command names they should match the core behavior of those git commands for the most part.

But more importantly I never want the raw commits from the environment branch to enter my pristine main history, even if we put a bow on them with a descriptive merge commit. I also wouldn't want any of that to show up in the merge commit message (like GitHub does with 'squash and merge').

Following those two principles, I get:

  • cu apply doesn't make a commit, since git apply doesn't.
  • cu merge is more like a cursed command that I would almost never use -- and I wouldn't really want to change its behavior, since I think it makes sense to align with git merge. It seems inherent that it would bring along the agent's fine-grained commits. Maybe that's useful sometimes? Maybe you want to merge to rebase + squash and drop detours that the agent made?
  • cu commit could maybe be a thing? It would be a shortcut for cu apply && git commit -m <agent authored message>. I know this diverges a bit from my first point since it magically brings the changes out from the aether, unlike git commit which simply commits the staged changes. But it's hard to imagine a command called cu commit doing anything else (like as a substitue for plain git commit).
  • Any git-sounding command should error out early in any scenario where the corresponding git command would have, too - i.e. when merging/rebasing/applying on top of a dirty tree.

I think that would cover all the scenarios I care about:

  • The agent is done, but I want to tweak/test things or write the commit myself:
    • cu apply; ...; git commit
    • or cu commit and just amend
    • CAVEAT: The only awkward thing is I might be tempted to do cu apply; ...; cu commit which would just error as described so far. Maybe we could magically detect that and have cu commit commit what you applied + any changes, and author the commit?
  • The agent is done, it did a good job, I want to commit the work so far and move on to the next task:
    • cu commit
    • CAVEAT: how do I "continue on" now that my history diverges from the agent's state? Maybe we reset --hard the environment branch to the commit? Maybe it works by squashing the agent's branch (from last cu commit) and merging?

I dunno, feels half baked but maybe there are some nuggets of wisdom in there.

@aluzzardi aluzzardi merged commit 0fe4df3 into main Jul 10, 2025
4 checks passed
@aluzzardi aluzzardi deleted the cu-merge-squash branch July 10, 2025 21:09
gsnaiper pushed a commit to gsnaiper/container-use that referenced this pull request Nov 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

Comments