Hi Junio,
Le 09/10/2020 à 06:48, Junio C Hamano a écrit :
> Alban Gruin <alban.gruin@xxxxxxxxx> writes:
>
>> diff --git a/merge-strategies.c b/merge-strategies.c
>> index bbe6f48698..f0e30f5624 100644
>> --- a/merge-strategies.c
>> +++ b/merge-strategies.c
>> @@ -2,6 +2,7 @@
>> #include "dir.h"
>> #include "ll-merge.h"
>> #include "merge-strategies.h"
>> +#include "run-command.h"
>> #include "xdiff-interface.h"
>>
>> static int add_to_index_cacheinfo(struct index_state *istate,
>> @@ -212,3 +213,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][GIT_MAX_HEXSZ] = {{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);
>
> oid_to_hex() uses 4-slot rotating buffer, no? Relying on the fact
> that three would be available here without getting reused (or,
> rather, our caller didn't make its own calls and/or does not mind
> us invalidating all but one slot for them) feels a bit iffy.
>
> Extending ownbuf[] to 6 elements and using oid_to_hex_r() would be a
> trivial way to clarify the code.
>
>> + 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);
>
> And these mode bits would not need GIT_MAX_HEXSZ to begin with.
> This smells like a WIP that hasn't been carefullly proofread.
>
> char oidbuf[3][GIT_MAX_HEXSZ] = { 0 };
> char modebuf[3][8] = { 0 };
So here I picked GIT_MAX_HEXSZ + 1 and 10 for those buffers, they are
already used by builtin/diff.c.
> char *args[] = {
> data, oidbuf[0], oidbuf[1], oidbuf[2], path,
> modebuf[0], modebuf[1], modebuf[2], NULL,
> };
>
> if (orig_blob)
> oid_to_hex_r(oidbuf[0], orig_blob);
> ...
> xsnprintf(modebuf[0], ...);
>
>
> Eh, wait. Is this meant to be able to drive "git-merge-one-file",
> i.e. a missing common/ours/theirs is indicated by an empty string
> in both oiod and mode? If so, an unconditional xsnprintf() would
> either give garbage or "0" at best, neither of which is an empty
> string. So the body would be more like
>
> if (orig_blob) {
> oid_to_hex_r(oidbuf[0], orig_blob);
> xsnprintf(modebuf[0], "%o", orig_mode);
> }
> if (our_blob) {
> oid_to_hex_r(oidbuf[1], our_blob);
> xsnprintf(modebuf[1], "%o", our_mode);
> }
> ...
>
> wouldn't it?
>
Yes, especially since you suggested to error out if an empty oid has a
non-empty mode in the second patch.
>> + 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)
>
> When we use an identifier "cb", it typically means callback data,
> not a callback function which is often called "fn". So, name the
> type "merge_fn" (or "merge_func"), and call the parameter "fn".
>
>> +{
>> + 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;
>> +}
>
> This copies from builtin/merge-index.c::merge_entry().
>
>> +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;
>> +}
>
> Likewise from the same function in that file.
>
> Are we removing the "git merge-index" program? Reusing the same
> identifier for these copied-and-pasted pairs of functions bothers
> me for two reasons.
>
> - An indentifier that was clear and unique enough in the original
> context as a file-scope static may not be a good name as a global
> identifier.
>
> - Having two similar-looking functions with the same name makes
> reading and learning the codebase starting at "git grep" hits
> more difficult than necessary.
>
I don't plan to remove `git merge-index' -- nor any other program, for
that matter. Why not renaming merge_one_path() and merge_all(),
merge_index_path() and merge_all_index()?
>> +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;
>> +}
>
> Likewise.
>
>> 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);
>
> Call it "merge_one_file_func", probably.
>
>> +int merge_program_cb(const struct object_id *orig_blob,
>
> Call it spawn_merge_one_file() perhaps?
>
>> + 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 */
Ack for the rest, the two function names are the only thing I am still
missing on this patch right now.
Alban
