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?
> + 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;
> }
> 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.
> 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
