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@xxxxxxxxxxxxxxxxxxxxxxxxx/
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
>
