Re: [PATCH v7 08/15] merge-one-file: rewrite in C

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

Hi Johannes,

Le 22/03/2021 à 23:20, Johannes Schindelin a écrit :
> Hi Alban,
> 
> On Wed, 17 Mar 2021, Alban Gruin wrote:
> 

>> @@ -69,10 +69,13 @@ int cmd_merge_index(int argc, const char **argv, const char *prefix)
>>
>>  	if (skip_prefix(pgm, "--use=", &use_internal)) {
>>  		if (!strcmp(use_internal, "merge-one-file"))
>> -			pgm = "git-merge-one-file";
>> +			merge_action = merge_one_file_func;
>>  		else
>>  			die(_("git merge-index: unknown internal program %s"), use_internal);
>> -	}
>> +
>> +		repo_hold_locked_index(r, &lock, LOCK_DIE_ON_ERROR);
>> +	} else
>> +		merge_action = merge_one_file_spawn;
> 
> I would have a slight preference to keep the default initializer, because
> that makes it easer to reason about. But if you _want_ to keep this patch
> as-is, I won't object.
> 

Yeah, not sure why I did this.  I'll change this.

> It is a bit sad that the conversion cannot be done more incrementally, as
> there is a lot to unpack in the many different cases that are handled. It
> looks correct, though.
> 
> Just one thing:
> 
>>
>>  	for (; i < argc; i++) {
>>  		const char *arg = argv[i];
>> diff --git a/builtin/merge-one-file.c b/builtin/merge-one-file.c
>> new file mode 100644
>> index 0000000000..ad99c6dbd4
>> --- /dev/null
>> +++ b/builtin/merge-one-file.c
>> @@ -0,0 +1,94 @@
>> +/*
>> + * Builtin "git merge-one-file"
>> + *
>> + * Copyright (c) 2020 Alban Gruin
>> + *
>> + * Based on git-merge-one-file.sh, written by Linus Torvalds.
>> + *
>> + * This is the git per-file merge utility, called with
>> + *
>> + *   argv[1] - original file object name (or empty)
>> + *   argv[2] - file in branch1 object name (or empty)
>> + *   argv[3] - file in branch2 object name (or empty)
>> + *   argv[4] - pathname in repository
>> + *   argv[5] - original file mode (or empty)
>> + *   argv[6] - file in branch1 mode (or empty)
>> + *   argv[7] - file in branch2 mode (or empty)
>> + *
>> + * Handle some trivial cases. The _really_ trivial cases have been
>> + * handled already by git read-tree, but that one doesn't do any merges
>> + * that might change the tree layout.
>> + */
>> +
>> +#include "cache.h"
>> +#include "builtin.h"
>> +#include "lockfile.h"
>> +#include "merge-strategies.h"
>> +
>> +static const char builtin_merge_one_file_usage[] =
>> +	"git merge-one-file <orig blob> <our blob> <their blob> <path> "
>> +	"<orig mode> <our mode> <their mode>\n\n"
>> +	"Blob ids and modes should be empty for missing files.";
>> +
>> +static int read_mode(const char *name, const char *arg, unsigned int *mode)
>> +{
>> +	char *last;
>> +	int ret = 0;
>> +
>> +	*mode = strtol(arg, &last, 8);
>> +
>> +	if (*last)
>> +		ret = error(_("invalid '%s' mode: expected nothing, got '%c'"), name, *last);
>> +	else if (!(S_ISREG(*mode) || S_ISDIR(*mode) || S_ISLNK(*mode)))
>> +		ret = error(_("invalid '%s' mode: %o"), name, *mode);
>> +
>> +	return ret;
>> +}
>> +
>> +int cmd_merge_one_file(int argc, const char **argv, const char *prefix)
>> +{
>> +	struct object_id orig_blob, our_blob, their_blob,
>> +		*p_orig_blob = NULL, *p_our_blob = NULL, *p_their_blob = NULL;
>> +	unsigned int orig_mode = 0, our_mode = 0, their_mode = 0, ret = 0;
>> +	struct lock_file lock = LOCK_INIT;
>> +	struct repository *r = the_repository;
>> +
>> +	if (argc != 8)
>> +		usage(builtin_merge_one_file_usage);
>> +
>> +	if (repo_read_index(r) < 0)
>> +		die("invalid index");
>> +
>> +	repo_hold_locked_index(r, &lock, LOCK_DIE_ON_ERROR);
>> +
>> +	if (!get_oid_hex(argv[1], &orig_blob)) {
>> +		p_orig_blob = &orig_blob;
>> +		ret = read_mode("orig", argv[5], &orig_mode);
>> +	} else if (!*argv[1] && *argv[5])
>> +		ret = error(_("no 'orig' object id given, but a mode was still given."));
> 
> Here, it looks as if the case of an empty `argv[1]` is not handled
> _explicitly_, but we rely on `get_oid_hex()` to return non-zero, and then
> we rely on the second arm _also_ not re-assigning `orig_blob`.
> 
> I wonder whether this could be checked, and whether it would make sense to
> fold this, along with most of these 5 lines, into the `read_mode()` helper
> function (DRYing up the code even further).
> 

Do you mean rewriting the first condition to read like this:

    if (*argv[1] && !get_oid_hex(argv[1], &orig_blob)) {

?

In which case yes, I can do that.

BTW the two lasts calls to read_mode() should be like

    err |= read_mode(…);

Cheers,
Alban

> As for the rest of the patch, it is totally possible that I missed a bug,
> but it looks correct to me, and the added regression tests give me a good
> feeling about the patch, too.
> 
> Thanks,
> Dscho
>