| Message ID | 20200625121953.16991-7-alban.gruin@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Rewrite the remaining merge strategies from shell to C | expand |
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@gmail.com> > --- > > 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 */ >
Hi Alban On 26/06/2020 11:13, Phillip Wood wrote: > 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@gmail.com> >> --- >> >> 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); Sorry ignore all the comments below, they are nonsense Best Wishes Phillip > 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 */ >> >
Hi Phillip, Phillip Wood (phillip.wood123@gmail.com) a écrit : > Hi Alban > > On 25/06/2020 13:19, Alban Gruin wrote: > -%<- > > 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. > Dunno, I am not completely comfortable with changing a parameter that specifically describe a program, to a parameter that may be a program, except in one case where `merge-index' should lock the index, setup the worktree, and call a function instead. Well, I say that, but implementing that behaviour is not that hard: -- snip -- diff --git a/builtin/merge-index.c b/builtin/merge-index.c index 6cb666cc78..19fff9a113 100644 --- a/builtin/merge-index.c +++ b/builtin/merge-index.c @@ -1,11 +1,15 @@ #define USE_THE_INDEX_COMPATIBILITY_MACROS #include "builtin.h" +#include "lockfile.h" #include "merge-strategies.h" int cmd_merge_index(int argc, const char **argv, const char *prefix) { int i, force_file = 0, err = 0, one_shot = 0, quiet = 0; const char *pgm; + void *data; + merge_cb merge_action; + struct lock_file lock = LOCK_INIT; /* Without this we cannot rely on waitpid() to tell * what happened to our children. @@ -26,7 +30,19 @@ int cmd_merge_index(int argc, const char **argv, const char *prefix) quiet = 1; i++; } + pgm = argv[i++]; + if (!strcmp(pgm, "git-merge-one-file")) { + merge_action = merge_one_file_cb; + data = (void *)the_repository; + + setup_work_tree(); + hold_locked_index(&lock, LOCK_DIE_ON_ERROR); + } else { + merge_action = merge_program_cb; + data = (void *)pgm; + } + for (; i < argc; i++) { const char *arg = argv[i]; if (!force_file && *arg == '-') { @@ -36,13 +52,22 @@ int cmd_merge_index(int argc, const char **argv, const char *prefix) } if (!strcmp(arg, "-a")) { err |= merge_all(&the_index, one_shot, quiet, - merge_program_cb, (void *)pgm); + merge_action, data); continue; } die("git merge-index: unknown option %s", arg); } err |= merge_one_path(&the_index, one_shot, quiet, arg, - merge_program_cb, (void *)pgm); + merge_action, data); + } + + if (merge_action == merge_one_file_cb) { + if (err) { + rollback_lock_file(&lock); + return err; + } + + return write_locked_index(&the_index, &lock, COMMIT_LOCK); } return err; } -- snap -- > > +{ > > + 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 > Cheers, Alban
Hi Alban On 12/07/2020 12:36, Alban Gruin wrote: > Hi Phillip, > > Phillip Wood (phillip.wood123@gmail.com) a écrit : > >> Hi Alban >> >> On 25/06/2020 13:19, Alban Gruin wrote: >> -%<- >>> 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. >> > > Dunno, I am not completely comfortable with changing a parameter that > specifically describe a program, to a parameter that may be a program, > except in one case where `merge-index' should lock the index, setup the > worktree, and call a function instead. There is some previous discussion about this at https://lore.kernel.org/git/xmqqblv5kr9u.fsf@gitster-ct.c.googlers.com/ I'll try and have a proper look at your comments towards the end of the week (or maybe the week after the way things are at the moment...) Best Wishes Phillip > Well, I say that, but implementing that behaviour is not that hard: > > -- snip -- > diff --git a/builtin/merge-index.c b/builtin/merge-index.c > index 6cb666cc78..19fff9a113 100644 > --- a/builtin/merge-index.c > +++ b/builtin/merge-index.c > @@ -1,11 +1,15 @@ > #define USE_THE_INDEX_COMPATIBILITY_MACROS > #include "builtin.h" > +#include "lockfile.h" > #include "merge-strategies.h" > > int cmd_merge_index(int argc, const char **argv, const char *prefix) > { > int i, force_file = 0, err = 0, one_shot = 0, quiet = 0; > const char *pgm; > + void *data; > + merge_cb merge_action; > + struct lock_file lock = LOCK_INIT; > > /* Without this we cannot rely on waitpid() to tell > * what happened to our children. > @@ -26,7 +30,19 @@ int cmd_merge_index(int argc, const char **argv, const char *prefix) > quiet = 1; > i++; > } > + > pgm = argv[i++]; > + if (!strcmp(pgm, "git-merge-one-file")) { > + merge_action = merge_one_file_cb; > + data = (void *)the_repository; > + > + setup_work_tree(); > + hold_locked_index(&lock, LOCK_DIE_ON_ERROR); > + } else { > + merge_action = merge_program_cb; > + data = (void *)pgm; > + } > + > for (; i < argc; i++) { > const char *arg = argv[i]; > if (!force_file && *arg == '-') { > @@ -36,13 +52,22 @@ int cmd_merge_index(int argc, const char **argv, const char *prefix) > } > if (!strcmp(arg, "-a")) { > err |= merge_all(&the_index, one_shot, quiet, > - merge_program_cb, (void *)pgm); > + merge_action, data); > continue; > } > die("git merge-index: unknown option %s", arg); > } > err |= merge_one_path(&the_index, one_shot, quiet, arg, > - merge_program_cb, (void *)pgm); > + merge_action, data); > + } > + > + if (merge_action == merge_one_file_cb) { > + if (err) { > + rollback_lock_file(&lock); > + return err; > + } > + > + return write_locked_index(&the_index, &lock, COMMIT_LOCK); > } > return err; > } > -- snap -- > >>> +{ >>> + 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 >> > > Cheers, > Alban >
Hi Phillip, Phillip Wood (phillip.wood123@gmail.com) a écrit : > Hi Alban > > On 12/07/2020 12:36, Alban Gruin wrote: > > Hi Phillip, > > > > Phillip Wood (phillip.wood123@gmail.com) a écrit : > > > >> Hi Alban > >> > >> On 25/06/2020 13:19, Alban Gruin wrote: > >> -%<- > >>> 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. > >> > > > > Dunno, I am not completely comfortable with changing a parameter that > > specifically describe a program, to a parameter that may be a program, > > except in one case where `merge-index' should lock the index, setup the > > worktree, and call a function instead. > > There is some previous discussion about this at > https://lore.kernel.org/git/xmqqblv5kr9u.fsf@gitster-ct.c.googlers.com/ > Thanks. If no-one seems really against doing that, I'll include the patch below in the v2, with an additional note in the man page. > I'll try and have a proper look at your comments towards the end of the > week (or maybe the week after the way things are at the moment...) > > Best Wishes > > Phillip > Cheers, Alban
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) +{ + char ownbuf[3][60] = {{0}}; + 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); + + 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 */
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@gmail.com> --- 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(-)