changesets merge

[flat belfry]

IMO we never want file-level granularity - once we have chunk-level granularity, PREFER_OURS and PREFER_THEIRS just indicate how conflicts are resolved, not which changed file we select, and if there are no conflicts that's perfectly valid

Side question: is creating an empty git repo necessary? I remember playing with git apply and git diff and they also work just fine out-of-repo

Not 100% sure I interpreted the original message right so elaborating: I guess what I'm saying is onConflict to me means individual change conflicts, not file conflicts, which is I guess what it meant before. Basically it feels wrong to have an API that will lose changes from the inputs - they should either be merged cleanly, or leave merge conflicts, or the call should error (if no other option provided)

[mighty patio]

I quite like the file-level granularity, especially when applied on generators. The thing is you can have two changes on a same file that can be applied without conflict but the resulting file might be bad. So for generators I like to fail if a file is touched by multiple changesets. But I can be wrong.
One solution we just discussed is to remove the file level granularity except for fail, so to have the following strategies:

• leave conflict markers
• prefer ours / prefer theirs
• fail
• fail early (that's the one that will fail if a file is modified in both changesets)

--ours and --theirs require --3way that requires to have access to the blobs, so a git repo. At least that's my understanding

that would look like this:

  | Strategy               | Pre-check         | Git flags | Modify/Delete                                       | Binary conflict |
  |------------------------|-------------------|-----------|-----------------------------------------------------|-----------------|
  | FAIL_EARLY             | Fail if conflicts | N/A       | N/A                                                 | N/A             |
  | FAIL                   | No                | None      | Fail                                                | Fail            |
  | LEAVE_CONFLICT_MARKERS | No                | None      | Keep modified                                       | Fail            |
  | PREFER_OURS            | No                | -X ours   | Delete (our removal) or keep (our modification)     | Auto-resolved   |
  | PREFER_THEIRS          | No                | -X theirs | Delete (their removal) or keep (their modification) | Auto-resolved   |

[flat belfry]

ahhh ok that makes sense, figured it might be something obscure like that. oh well

[flat belfry]

Ah that makes sense - I only meant something more narrow, that if two changesets both modify the same file, you almost never want to choose one changeset's file and discard the other changeset's changes, so PREFER_OURS/PREFER_THEIRS would always be hunk-based, and not file-based

thinking on FAIL_EARLY - I don't have a good handle on how often that'll come up, but it seems like a reasonable knob

like you're specifically saying even if there are no conflicts, because you're worried about "semantic" conflicts etc?

[mighty patio]

side question: does that make sense to use gitutil.GitCLI to run git commands instead of exec.Command?

[mighty patio]

Yes. For instance you install two toolchains with generator functions, both of them will touch the same file, that could be an issue showing the responsibilities are not clear enough.
But maybe it's only a generator thing (or maybe just me 😅 )
At least the option could be available (especially as this option already works, that's the easiest one)

[flat belfry]

ehh worth a quick look but i think if we're just running a handful of git plumbing commands, it's probably not worth it. there's extra complexity in gitutil to support "real" repo cloning and such, which we don't need here, but if it makes life easier in other ways, sure!

[mighty patio]

I refreshed https://github.com/dagger/dagger/pull/11577
It now uses git way more, in two possible ways, withChangeset that does classical stuff, with ours/theirs, and withChangesets that tries an octopus merge

[hot vapor]

will take a look later today 👀, checked a bit yesterday night

[mighty patio]

If you have some cycle I'd love to have your feedback on the actual version @flat belfry
I think it covers a lot of different cases, but it ended up effectively with something more complex than I expected at the beginning

[flat belfry]

I did have one question - https://github.com/dagger/dagger/pull/11577#discussion_r2695044656

will review more in depth tomorrow!