From: Phillip Wood Date: Tue, 16 Aug 2022 14:02:20 GMT Subject: Re: [PATCH v8 08/14] merge-resolve: rewrite in C Message-ID: In-Reply-To: <128n8n08-23ss-pnsr-n910-o39nr32q42n5@tzk.qr> On 16/08/2022 13:17, Johannes Schindelin wrote: > Hi Phillip, > > On Wed, 10 Aug 2022, Phillip Wood wrote: > >> On 09/08/2022 19:54, Alban Gruin wrote: >>> This rewrites `git merge-resolve' from shell to C. As for `git >>> merge-one-file', this port is not completely straightforward and removes >>> calls to external processes to avoid reading and writing the index over >>> and over again. >>> >>> - The call to `update-index -q --refresh' is replaced by a call to >>> refresh_index(). >>> >>> - The call to `read-tree' is replaced by a call to unpack_trees() (and >>> all the setup needed). >>> >>> - The call to `write-tree' is replaced by a call to >>> cache_tree_update(). This call is wrapped in a new function, >>> write_tree(). It is made to mimick write_index_as_tree() with >>> WRITE_TREE_SILENT flag, but without locking the index; this is taken >>> care directly in merge_strategies_resolve(). >>> >>> - The call to `diff-index ...' is replaced by a call to >>> repo_index_has_changes(). >>> >>> - The call to `merge-index', needed to invoke `git merge-one-file', is >>> replaced by a call to the new merge_all_index() function. >>> >>> The index is read in cmd_merge_resolve(), and is wrote back by >>> merge_strategies_resolve(). This is to accomodate future applications: >>> in `git-merge', the index has already been read when the merge strategy >>> is called, so it would be redundant to read it again when the builtin >>> will be able to use merge_strategies_resolve() directly. >>> >>> The parameters of merge_strategies_resolve() will be surprising at first >>> glance: why using a commit list for `bases' and `remote', where we could >>> use an oid array, and a pointer to an oid? Because, in a later commit, >>> try_merge_strategy() will be able to call merge_strategies_resolve() >>> directly, and it already uses a commit list for `bases' (`common') and >>> `remote' (`remoteheads'), and a string for `head_arg'. To reduce >>> frictions later, merge_strategies_resolve() takes the same types of >>> parameters. >> >> git-merge-resolve will happily merge three trees, unfortunately using >> lists of commits will break that. > > But isn't `merge-resolve` specifically implemented as a merge strategy? I > do not see any contract in Git's documentation that commits to supporting > direct calls to the implementation detail that is `git merge-resolve`: > > $ man git-merge-resolve > No manual entry for git-merge-resolve I've certainly got scripts that call "git merge-recursive" with a mixture of commits and trees (it's kind of doing an cherry-pick), it wouldn't surprise me if someone was doing something weird with merge-resolve. >>> +int cmd_merge_resolve(int argc, const char **argv, const char *prefix) >>> +{ >>> + int i, sep_seen = 0; >>> + const char *head = NULL; >>> + struct commit_list *bases = NULL, *remote = NULL; >>> + struct commit_list **next_base = &bases; >>> + struct repository *r = the_repository; >>> + >>> + if (argc < 5) >>> + usage(builtin_merge_resolve_usage); >> >> I think it would be better to call parse_options() and then check argc. That >> would give better error messages for unknown options and supports '-h' for >> free. > > Again, we are talking about a merge strategy, a program that is not meant > to be called directly by the user. Why should we complicate the code by > using the `parse_options` machinery? I thought it would simplify the implementation of '-h' below. However as the script does not support '-h' we should perhaps drop support for that and the usage() call if we want a strictly equivalent conversion. >> I think we also need to call git_config(). I see that read-tree respects >> submodule.recurse so I think we need the same here. I suspect we should >> also be reading the merge config to respect merge.conflictStyle. > > Valid concerns. Extra brownie points if you can provide a simple test case > that demonstrates the current behavior. I'll add it to my todo list. >>> + >>> + if (repo_index_has_changes(r, head_tree, &sb)) { >>> + error(_("Your local changes to the following files " >>> + "would be overwritten by merge:\n %s"), >>> + sb.buf); >> >> This matches the script but I wonder why that did not check for unstaged >> changes. > > Any deviations from the scripted behavior should be done on top of this > patch series, unless the deviations make the conversion substantially > cleaner. I agree. Having thought some more I suspect it is relying on unpack_trees() to error out if there are unstaged changes. Best Wishes Phillip > Thanks, > Dscho