Re: [PATCH v6 06/13] merge-index: don't fork if the requested program is `git-merge-one-file'
- From
Alban Gruin <alban.gruin@gmail.com>
- Date
- Jan 5, 2021, 23:20 UTC
- Message-ID
- <411b68ad-dee5-5a19-ae94-c2b6a249161a@gmail.com>
- In-Reply-To
- <44c9189d-9d2f-c437-d0d6-9529708d2c99@gmail.com>
Le 05/01/2021 à 17:11, Derrick Stolee a écrit :
Show 14 quoted lines
> 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.
Show 41 quoted lines
>> + 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. :)
Show 26 quoted lines
>
>> }
>> 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().
Show 37 quoted lines
>> 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
>