| Message ID | 20231024195655.2413191-1-sandals@crustytoothpaste.net (mailing list archive) |
|---|---|
| Headers | show |
| Series | Object ID support for git merge-file | expand |
On Tue, Oct 24, 2023 at 12:58 PM brian m. carlson <sandals@crustytoothpaste.net> wrote: > > This series introduces an --object-id option to git merge-file such > that, instead of reading and writing from files on the system, it reads > from and writes to the object store using blobs. This seems like a reasonable capability to want from such a plumbing command. > This is in use at > GitHub to produce conflict diffs when a merge fails, and it seems > generally useful, so I'm sending it here. But...wouldn't you already have the conflicts generated when doing the merge and learning that it fails? Why would you need to generate them again? (Also, generating them again may risk getting names munged for conflict markers in edge cases involving renames.) That said, even if I have questions about your particular usecase, I think the feature you are submitting here makes sense independently. I left a few minor questions on the patch itself, but overall it looks good to me.
On 29/10/2023 06:24, Elijah Newren wrote: > On Tue, Oct 24, 2023 at 12:58 PM brian m. carlson >> This is in use at >> GitHub to produce conflict diffs when a merge fails, and it seems >> generally useful, so I'm sending it here. > > But...wouldn't you already have the conflicts generated when doing the > merge and learning that it fails? Why would you need to generate them > again? I was surprised by this as well, but as you say this seems like a useful addition independent of any specific use at GitHub. Best Wishes Phillip
On Sat, Oct 28, 2023 at 11:24:06PM -0700, Elijah Newren wrote: > On Tue, Oct 24, 2023 at 12:58 PM brian m. carlson > <sandals@crustytoothpaste.net> wrote: > > > > This series introduces an --object-id option to git merge-file such > > that, instead of reading and writing from files on the system, it reads > > from and writes to the object store using blobs. > > This seems like a reasonable capability to want from such a plumbing command. Agreed. > > This is in use at > > GitHub to produce conflict diffs when a merge fails, and it seems > > generally useful, so I'm sending it here. > > But...wouldn't you already have the conflicts generated when doing the > merge and learning that it fails? Why would you need to generate them > again? brian would know better than I do, but I believe the reason is because the "attempt this merge" RPC is handled separately from the "show me the merge conflict(s) at xyz path". Those probably could be combined (obviating the need for this patch), but doing so is probably rather complicated. Since this feature is generally useful for callers that haven't already completed a tree-level merge and really just care about the result of merging a single path, I don't have any objections here. Thanks, Taylor
On 2023-10-30 at 15:54:14, Taylor Blau wrote: > On Sat, Oct 28, 2023 at 11:24:06PM -0700, Elijah Newren wrote: > > But...wouldn't you already have the conflicts generated when doing the > > merge and learning that it fails? Why would you need to generate them > > again? > > brian would know better than I do, but I believe the reason is because > the "attempt this merge" RPC is handled separately from the "show me the > merge conflict(s) at xyz path". Those probably could be combined > (obviating the need for this patch), but doing so is probably rather > complicated. That's correct. They could in theory happen at different times, which is why they're not linked.
On Mon, Oct 30, 2023 at 9:24 AM brian m. carlson <sandals@crustytoothpaste.net> wrote: > > On 2023-10-30 at 15:54:14, Taylor Blau wrote: > > On Sat, Oct 28, 2023 at 11:24:06PM -0700, Elijah Newren wrote: > > > But...wouldn't you already have the conflicts generated when doing the > > > merge and learning that it fails? Why would you need to generate them > > > again? > > > > brian would know better than I do, but I believe the reason is because > > the "attempt this merge" RPC is handled separately from the "show me the > > merge conflict(s) at xyz path". Those probably could be combined > > (obviating the need for this patch), but doing so is probably rather > > complicated. > > That's correct. They could in theory happen at different times, which > is why they're not linked. Maybe this is digging a little into "historical reasons" too much, but this still seems a little funny. If they happen at different times, you still need multiple pieces of information remembered from the merge operation in order for git-merge-file to be able to regenerate the conflict correctly in general. In particular, you need the OIDs and the filenames. Trying to regenerate a conflict without remembering those from the merge step would only work for common cases, but would be problematic in the face of either renames being involved or recursive merges or both. And if you need to remember information from the merge step, then why not remember the actual conflicts (or at least the tree OID generated by the merge operation, which has the conflicts embedded within it)? I know, I know, there's probably just historical cruft that needs cleaning up, and I don't think any of this matters to the patch at hand since it's independently useful. It just sounds like a system has been set up that has some rough edge cases caused by a poor splitting. > -- > brian m. carlson (he/him or they/them) > Toronto, Ontario, CA