Re: [PATCH v6 06/13] merge-index: don't fork if the requested program is `git-merge-one-file'
From: Derrick Stolee <hidden>
Date: 2021-01-05 16:12:22
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;
quoted hunk ↗ jump to hunk
}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.
quoted hunk ↗ jump to hunk
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