Re: [PATCH v8 08/14] merge-resolve: rewrite in C

> 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.

> > merge_strategies_resolve() locks the index only once, at the beginning
> > of the merge, and releases it when the merge has been completed.
> >
> > Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
> > ---
> > diff --git a/builtin/merge-resolve.c b/builtin/merge-resolve.c
> > new file mode 100644
> > index 0000000000..a51158ebf8
> > --- /dev/null
> > +++ b/builtin/merge-resolve.c
> > @@ -0,0 +1,63 @@
> > +/*
> > + * Builtin "git merge-resolve"
> > + *
> > + * Copyright (c) 2020 Alban Gruin
> > + *
> > + * Based on git-merge-resolve.sh, written by Linus Torvalds and Junio C
> > + * Hamano.
> > + *
> > + * Resolve two trees, using enhanced multi-base read-tree.
> > + */
> > +
> > +#include "cache.h"
> > +#include "builtin.h"
> > +#include "merge-strategies.h"
> > +
> > +static const char builtin_merge_resolve_usage[] =
> > +	"git merge-resolve <bases>... -- <head> <remote>";
> > +
> > +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.

> > +
> > +	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.