Hi Alban
On 25/06/2020 13:19, Alban Gruin wrote:
> The "resolve" and "octopus" merge strategies do not call directly `git
> merge-one-file', they delegate the work to another git command, `git
> merge-index', that will loop over files in the index and call the
> specified command. Unfortunately, these functions are not part of
> libgit.a, which means that once rewritten, the strategies would still
> have to invoke `merge-one-file' by spawning a new process first.
>
> To avoid this, this moves merge_one_path(), merge_all(), and their
> helpers to merge-strategies.c. They also take a callback to dictate
> what they should do for each file. For now, only one launching a new
> process is defined to preserve the behaviour of the builtin version.
>
> Signed-off-by: Alban Gruin <alban.gruin@xxxxxxxxx>
> ---
>
> Notes:
> This patch is best viewed with `--color-moved'.
>
> builtin/merge-index.c | 77 +++------------------------------
> merge-strategies.c | 99 +++++++++++++++++++++++++++++++++++++++++++
> merge-strategies.h | 17 ++++++++
> 3 files changed, 123 insertions(+), 70 deletions(-)
>
> diff --git a/builtin/merge-index.c b/builtin/merge-index.c
> index 38ea6ad6ca..6cb666cc78 100644
> --- a/builtin/merge-index.c
> +++ b/builtin/merge-index.c
> @@ -1,74 +1,11 @@
> #define USE_THE_INDEX_COMPATIBILITY_MACROS
> #include "builtin.h"
> -#include "run-command.h"
> -
> -static const char *pgm;
> -static int one_shot, quiet;
> -static int err;
> -
> -static int merge_entry(int pos, const char *path)
> -{
> - int found;
> - const char *arguments[] = { pgm, "", "", "", path, "", "", "", NULL };
> - char hexbuf[4][GIT_MAX_HEXSZ + 1];
> - char ownbuf[4][60];
> -
> - if (pos >= active_nr)
> - die("git merge-index: %s not in the cache", path);
> - found = 0;
> - do {
> - const struct cache_entry *ce = active_cache[pos];
> - int stage = ce_stage(ce);
> -
> - if (strcmp(ce->name, path))
> - break;
> - found++;
> - oid_to_hex_r(hexbuf[stage], &ce->oid);
> - xsnprintf(ownbuf[stage], sizeof(ownbuf[stage]), "%o", ce->ce_mode);
> - arguments[stage] = hexbuf[stage];
> - arguments[stage + 4] = ownbuf[stage];
> - } while (++pos < active_nr);
> - if (!found)
> - die("git merge-index: %s not in the cache", path);
> -
> - if (run_command_v_opt(arguments, 0)) {
> - if (one_shot)
> - err++;
> - else {
> - if (!quiet)
> - die("merge program failed");
> - exit(1);
> - }
> - }
> - return found;
> -}
> -
> -static void merge_one_path(const char *path)
> -{
> - int pos = cache_name_pos(path, strlen(path));
> -
> - /*
> - * If it already exists in the cache as stage0, it's
> - * already merged and there is nothing to do.
> - */
> - if (pos < 0)
> - merge_entry(-pos-1, path);
> -}
> -
> -static void merge_all(void)
> -{
> - int i;
> - for (i = 0; i < active_nr; i++) {
> - const struct cache_entry *ce = active_cache[i];
> - if (!ce_stage(ce))
> - continue;
> - i += merge_entry(i, ce->name)-1;
> - }
> -}
> +#include "merge-strategies.h"
>
> int cmd_merge_index(int argc, const char **argv, const char *prefix)
> {
> - int i, force_file = 0;
> + int i, force_file = 0, err = 0, one_shot = 0, quiet = 0;
> + const char *pgm;
>
> /* Without this we cannot rely on waitpid() to tell
> * what happened to our children.
> @@ -98,14 +35,14 @@ int cmd_merge_index(int argc, const char **argv, const char *prefix)
> continue;
> }
> if (!strcmp(arg, "-a")) {
> - merge_all();
> + err |= merge_all(&the_index, one_shot, quiet,
> + merge_program_cb, (void *)pgm);
> continue;
> }
> die("git merge-index: unknown option %s", arg);
> }
> - merge_one_path(arg);
> + err |= merge_one_path(&the_index, one_shot, quiet, arg,
> + merge_program_cb, (void *)pgm);
> }
> - if (err && !quiet)
> - die("merge program failed");
> return err;
> }
> diff --git a/merge-strategies.c b/merge-strategies.c
> index 3a9fce9f22..f4c0b4acd6 100644
> --- a/merge-strategies.c
> +++ b/merge-strategies.c
> @@ -1,6 +1,7 @@
> #include "cache.h"
> #include "dir.h"
> #include "merge-strategies.h"
> +#include "run-command.h"
> #include "xdiff-interface.h"
>
> static int add_to_index_cacheinfo(struct index_state *istate,
> @@ -189,3 +190,101 @@ int merge_strategies_one_file(struct repository *r,
>
> return 0;
> }
> +
> +int merge_program_cb(const struct object_id *orig_blob,
> + const struct object_id *our_blob,
> + const struct object_id *their_blob, const char *path,
> + unsigned int orig_mode, unsigned int our_mode, unsigned int their_mode,
> + void *data)
Using void* is slightly unfortunate but it's needed later.
It would be nice to check if the program to run is git-merge-one-file
and call the appropriate function instead in that case so all users of
merge-index get the benefit of it being builtin. That probably wants to
be done in cmd_merge_index() rather than here though.
> +{
> + char ownbuf[3][60] = {{0}};
I know this is copied from above but it would be better to use
GIT_MAX_HEXSZ rather than 60
> + const char *arguments[] = { (char *)data, "", "", "", path,
> + ownbuf[0], ownbuf[1], ownbuf[2],
> + NULL };
> +
> + if (orig_blob)
> + arguments[1] = oid_to_hex(orig_blob);
> + if (our_blob)
> + arguments[2] = oid_to_hex(our_blob);
> + if (their_blob)
> + arguments[3] = oid_to_hex(their_blob);
> +
> + xsnprintf(ownbuf[0], sizeof(ownbuf[0]), "%o", orig_mode);
> + xsnprintf(ownbuf[1], sizeof(ownbuf[1]), "%o", our_mode);
> + xsnprintf(ownbuf[2], sizeof(ownbuf[2]), "%o", their_mode);
These are leaked. Also are you sure we want to fill out the mode if the
corresponding blob is missing - I guess it doesn't matter but it would
be good to check that - i think the original passed "". It also passed
"" rather than "0000..." for the blobs that were missing I think.
Best Wishes
Phillip
> +
> + return run_command_v_opt(arguments, 0);
> +}
> +
> +static int merge_entry(struct index_state *istate, int quiet, int pos,
> + const char *path, merge_cb cb, void *data)
> +{
> + int found = 0;
> + const struct object_id *oids[3] = {NULL};
> + unsigned int modes[3] = {0};
> +
> + do {
> + const struct cache_entry *ce = istate->cache[pos];
> + int stage = ce_stage(ce);
> +
> + if (strcmp(ce->name, path))
> + break;
> + found++;
> + oids[stage - 1] = &ce->oid;
> + modes[stage - 1] = ce->ce_mode;
> + } while (++pos < istate->cache_nr);
> + if (!found)
> + return error(_("%s is not in the cache"), path);
> +
> + if (cb(oids[0], oids[1], oids[2], path, modes[0], modes[1], modes[2], data)) {
> + if (!quiet)
> + error(_("Merge program failed"));
> + return -2;
> + }
> +
> + return found;
> +}
> +
> +int merge_one_path(struct index_state *istate, int oneshot, int quiet,
> + const char *path, merge_cb cb, void *data)
> +{
> + int pos = index_name_pos(istate, path, strlen(path)), ret;
> +
> + /*
> + * If it already exists in the cache as stage0, it's
> + * already merged and there is nothing to do.
> + */
> + if (pos < 0) {
> + ret = merge_entry(istate, quiet, -pos - 1, path, cb, data);
> + if (ret == -1)
> + return -1;
> + else if (ret == -2)
> + return 1;
> + }
> + return 0;
> +}
> +
> +int merge_all(struct index_state *istate, int oneshot, int quiet,
> + merge_cb cb, void *data)
> +{
> + int err = 0, i, ret;
> + for (i = 0; i < istate->cache_nr; i++) {
> + const struct cache_entry *ce = istate->cache[i];
> + if (!ce_stage(ce))
> + continue;
> +
> + ret = merge_entry(istate, quiet, i, ce->name, cb, data);
> + if (ret > 0)
> + i += ret - 1;
> + else if (ret == -1)
> + return -1;
> + else if (ret == -2) {
> + if (oneshot)
> + err++;
> + else
> + return 1;
> + }
> + }
> +
> + return err;
> +}
> diff --git a/merge-strategies.h b/merge-strategies.h
> index b527d145c7..cf78d7eaf4 100644
> --- a/merge-strategies.h
> +++ b/merge-strategies.h
> @@ -10,4 +10,21 @@ int merge_strategies_one_file(struct repository *r,
> unsigned int orig_mode, unsigned int our_mode,
> unsigned int their_mode);
>
> +typedef int (*merge_cb)(const struct object_id *orig_blob,
> + const struct object_id *our_blob,
> + const struct object_id *their_blob, const char *path,
> + unsigned int orig_mode, unsigned int our_mode, unsigned int their_mode,
> + void *data);
> +
> +int merge_program_cb(const struct object_id *orig_blob,
> + const struct object_id *our_blob,
> + const struct object_id *their_blob, const char *path,
> + unsigned int orig_mode, unsigned int our_mode, unsigned int their_mode,
> + void *data);
> +
> +int merge_one_path(struct index_state *istate, int oneshot, int quiet,
> + const char *path, merge_cb cb, void *data);
> +int merge_all(struct index_state *istate, int oneshot, int quiet,
> + merge_cb cb, void *data);
> +
> #endif /* MERGE_STRATEGIES_H */
>
