Re: [RFC PATCH v1 06/17] merge-index: libify merge_one_path() and merge_all()

[Phillip Wood <phillip.wood123@gmail.com>]

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 */
>>
>