Re: [PATCH v6 06/13] merge-index: don't fork if the requested program is `git-merge-one-file'

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

Le 05/01/2021 à 17:11, Derrick Stolee a écrit :
> On 11/24/2020 6:53 AM, Alban Gruin wrote:
>> +
>>  	pgm = argv[i++];
>> +	setup_work_tree();
>> +
>> +	if (!strcmp(pgm, "git-merge-one-file")) {
> 
> This stood out to me as possibly fragile. What if we call the
> non-dashed form "git merge-one-file"? Shouldn't we be doing so?
> 
> Or, is this something that is handled higher in the builtin
> machinery to take the non-dashed version and change it to the
> dashed version for historical reasons?
> 

We had the same discussion with Phillip, who pointed out this previous
discussion about this topic:
https://lore.kernel.org/git/xmqqblv5kr9u.fsf@gitster-ct.c.googlers.com/

So, it's probably OK to do that.

>> +		merge_action = merge_one_file_func;
>> +		hold_locked_index(&lock, LOCK_DIE_ON_ERROR);
>> +	} else {
>> +		merge_action = merge_one_file_spawn;
>> +		data = (void *)pgm;
>> +	}
>> +
> 
> ...
> 
>> +	if (merge_action == merge_one_file_func) {
> 
> nit: This made me think it would be better to check the 'lock'
> itself to see if it was initialized or not. Perhaps
> 
> 	if (lock.tempfile) {
> 
> would be the appropriate way to check this?
> 
> For now, this is equivalent behavior, but it might be helpful if
> we add more cases that take the lock in the future.
> 
>> +		if (err) {
>> +			rollback_lock_file(&lock);
>> +			return err;
>> +		}
>> +
>> +		return write_locked_index(&the_index, &lock, COMMIT_LOCK);
>>  	}
>>  	return err;
> 
> nit: this could be simplified. In total, I recommend:
> 
> 	if (lock.tempfile) {
> 		if (err)
> 			rollback_lock_file(&lock);
> 		else
> 			return write_locked_index(&the_index, &lock, COMMIT_LOCK);
> 	}
> 	return err;
> 

Sure, looks better than mine.  :)

> 
>>  }
>> diff --git a/merge-strategies.c b/merge-strategies.c
>> index 6f27e66dfe..542cefcf3d 100644
>> --- a/merge-strategies.c
>> +++ b/merge-strategies.c
>> @@ -178,6 +178,18 @@ int merge_three_way(struct repository *r,
>>  	return 0;
>>  }
>>  
>> +int merge_one_file_func(struct repository *r,
>> +			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)
>> +{
>> +	return merge_three_way(r,
>> +			       orig_blob, our_blob, their_blob, path,
>> +			       orig_mode, our_mode, their_mode);
>> +}
>> +
> 
> Again, I don't recommend making this callback in the library. Instead, keep
> it in the builtin and then use merge_three_way() which is in the library.
> 

This is not possible with this callback, as it will be used later by
merge_strategies_resolve() and indirectly by merge_strategies_octopus().

>>  int merge_one_file_spawn(struct repository *r,
>>  			 const struct object_id *orig_blob,
>>  			 const struct object_id *our_blob,
>> @@ -261,17 +273,22 @@ int merge_all_index(struct repository *r, int oneshot, int quiet,
>>  		    merge_fn fn, void *data)
>>  {
>>  	int err = 0, ret;
>> -	unsigned int i;
>> +	unsigned int i, prev_nr;
>>  
>>  	for (i = 0; i < r->index->cache_nr; i++) {
>>  		const struct cache_entry *ce = r->index->cache[i];
>>  		if (!ce_stage(ce))
>>  			continue;
>>  
>> +		prev_nr = r->index->cache_nr;
>>  		ret = merge_entry(r, quiet || oneshot, i, ce->name, &err, fn, data);
>> -		if (ret > 0)
>> -			i += ret - 1;
>> -		else if (ret == -1)
>> +		if (ret > 0) {
>> +			/* Don't bother handling an index that has
>> +			   grown, since merge_one_file_func() can't grow
>> +			   it, and merge_one_file_spawn() can't change
>> +			   it. */
> 
> multi-line comment style is as follows:
> 
> 	/*
> 	 * Don't bother handling an index that has
> 	 * grown, since merge_one_file_func() can't grow
> 	 * it, and merge_one_file_spawn() can't change it.
> 	 */
> 
> Thanks,
> -Stolee
>