Re: [PATCH v3 03/11] merge-index: libify merge_one_path() and merge_all()

[Alban Gruin <alban.gruin@gmail.com>]

Hi Junio,

Le 09/10/2020 à 06:48, Junio C Hamano a écrit :
> Alban Gruin <alban.gruin@gmail.com> 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