| Message ID | 20190726155258.28561-8-newren@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Cleanup merge API | expand |
Elijah Newren <newren@gmail.com> writes: > diff --git a/cache-tree.c b/cache-tree.c > index 706ffcf188..99144b1704 100644 > --- a/cache-tree.c > +++ b/cache-tree.c > @@ -613,14 +613,19 @@ int write_index_as_tree(struct object_id *oid, struct index_state *index_state, > int entries, was_valid; > struct lock_file lock_file = LOCK_INIT; > int ret = 0; > + int access_disk = !(flags & WRITE_TREE_FROM_MEMORY); Shouldn't we go one step futher and make the bulk of in-core index processing into a new helper function, while making write_index_as_tree() a thin-wrapper around it, i.e. write_index_as_tree() { lock the index for update; read the on-disk index; call that new helper function to write a tree; update the on-disk index; } and reuse the helper from merge-recursive.c::write_tree_from_memory() while keeping the call to the latter in merge_trees_internal()? Wouldn't that approach let you do this without adding an extra flag bit? Also, there used to be a check to ensure that the in-core index fed to write_tree_from_memory() is fully merged and otherwise dump the unmerged entries with BUG(). Can we simply lose it? I know you return with "error building trees" from merge_trees_internal() but it does not BUG().
On Fri, Jul 26, 2019 at 1:11 PM Junio C Hamano <gitster@pobox.com> wrote: > > Elijah Newren <newren@gmail.com> writes: > > > diff --git a/cache-tree.c b/cache-tree.c > > index 706ffcf188..99144b1704 100644 > > --- a/cache-tree.c > > +++ b/cache-tree.c > > @@ -613,14 +613,19 @@ int write_index_as_tree(struct object_id *oid, struct index_state *index_state, > > int entries, was_valid; > > struct lock_file lock_file = LOCK_INIT; > > int ret = 0; > > + int access_disk = !(flags & WRITE_TREE_FROM_MEMORY); > > Shouldn't we go one step futher and make the bulk of in-core index > processing into a new helper function, while making > write_index_as_tree() a thin-wrapper around it, i.e. > > write_index_as_tree() > { > lock the index for update; > read the on-disk index; > call that new helper function to write a tree; > update the on-disk index; > } > > and reuse the helper from > merge-recursive.c::write_tree_from_memory() while keeping the call > to the latter in merge_trees_internal()? Wouldn't that approach > let you do this without adding an extra flag bit? I thought about that briefly yesterday, but the fact that the write_locked_index() call only happens if !cache_tree_fully_valid() meant refactoring slightly more to get the helper to also return that boolean value, and since I was a little unsure of myself with cache-tree stuff in general I wanted to propose what looked like the minimally invasive changes first (by which I mean smallest patch). I'll take a closer look at this path. > Also, there used to be a check to ensure that the in-core index fed > to write_tree_from_memory() is fully merged and otherwise dump the > unmerged entries with BUG(). Can we simply lose it? I know you > return with "error building trees" from merge_trees_internal() but > it does not BUG(). I thought about that yesterday and decided, "Nah, it's a developer only debug message used during development. I've _never_ seen anyone report those messages, and I only saw them when I was making bad changes during development when I first started a decade ago." Then Emily posts a report today showing that exact BUG message being hit with git-2.22.0, and how she wouldn't have been able to get all that extra information in her analysis without that bit of information being reported. So, yeah, I need to put something from those BUG() messages back in; they clearly helped with that issue, and might help again in the future.
Elijah Newren <newren@gmail.com> writes: > I thought about that briefly yesterday, but the fact that the > write_locked_index() call only happens if !cache_tree_fully_valid() > meant refactoring slightly more to get the helper to also return that > boolean value, and since I was a little unsure of myself with > cache-tree stuff in general I wanted to propose what looked like the > minimally invasive changes first (by which I mean smallest patch). Or have the caller check if cache-tree is fully valid, which is the only case that you can build a tree (and a fully merged index would be fully valid after you do cache_tree_update()). > I'll take a closer look at this path. > ... > So, yeah, I need to put something from those BUG() messages back in; > they clearly helped with that issue, and might help again in the > future. Thanks.
diff --git a/builtin/checkout.c b/builtin/checkout.c index 20e38c5edc..a7b6454061 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -709,6 +709,7 @@ static int merge_working_tree(const struct checkout_opts *opts, * whether the merge flag was used. */ struct tree *work; + struct object_id work_oid; struct tree *old_tree; struct merge_options o; struct strbuf sb = STRBUF_INIT; @@ -759,7 +760,10 @@ static int merge_working_tree(const struct checkout_opts *opts, */ init_merge_options(&o, the_repository); o.verbosity = 0; - work = write_tree_from_memory(&o); + if (write_index_as_tree(&work_oid, &the_index, NULL, + WRITE_TREE_FROM_MEMORY, NULL) || + !(work = lookup_tree(the_repository, &work_oid))) + die(_("error building trees")); ret = reset_tree(new_tree, opts, 1, diff --git a/cache-tree.c b/cache-tree.c index 706ffcf188..99144b1704 100644 --- a/cache-tree.c +++ b/cache-tree.c @@ -613,14 +613,19 @@ int write_index_as_tree(struct object_id *oid, struct index_state *index_state, int entries, was_valid; struct lock_file lock_file = LOCK_INIT; int ret = 0; + int access_disk = !(flags & WRITE_TREE_FROM_MEMORY); - hold_lock_file_for_update(&lock_file, index_path, LOCK_DIE_ON_ERROR); + if (access_disk) { + hold_lock_file_for_update(&lock_file, index_path, + LOCK_DIE_ON_ERROR); - entries = read_index_from(index_state, index_path, get_git_dir()); - if (entries < 0) { - ret = WRITE_TREE_UNREADABLE_INDEX; - goto out; + entries = read_index_from(index_state, index_path, get_git_dir()); + if (entries < 0) { + ret = WRITE_TREE_UNREADABLE_INDEX; + goto out; + } } + if (flags & WRITE_TREE_IGNORE_CACHE_TREE) cache_tree_free(&index_state->cache_tree); @@ -633,13 +638,16 @@ int write_index_as_tree(struct object_id *oid, struct index_state *index_state, ret = WRITE_TREE_UNMERGED_INDEX; goto out; } - write_locked_index(index_state, &lock_file, COMMIT_LOCK); - /* Not being able to write is fine -- we are only interested - * in updating the cache-tree part, and if the next caller - * ends up using the old index with unupdated cache-tree part - * it misses the work we did here, but that is just a - * performance penalty and not a big deal. - */ + if (access_disk) { + write_locked_index(index_state, &lock_file, COMMIT_LOCK); + /* Not being able to write is fine -- we are only + * interested in updating the cache-tree part, and if + * the next caller ends up using the old index with + * unupdated cache-tree part it misses the work we + * did here, but that is just a performance penalty + * and not a big deal. + */ + } } if (prefix) { @@ -655,7 +663,9 @@ int write_index_as_tree(struct object_id *oid, struct index_state *index_state, oidcpy(oid, &index_state->cache_tree->oid); out: - rollback_lock_file(&lock_file); + if (access_disk) + rollback_lock_file(&lock_file); + return ret; } diff --git a/cache-tree.h b/cache-tree.h index 757bbc48bc..481ec45dfa 100644 --- a/cache-tree.h +++ b/cache-tree.h @@ -34,12 +34,13 @@ int cache_tree_fully_valid(struct cache_tree *); int cache_tree_update(struct index_state *, int); void cache_tree_verify(struct repository *, struct index_state *); -/* bitmasks to write_cache_as_tree flags */ +/* bitmasks to write_index_as_tree flags */ #define WRITE_TREE_MISSING_OK 1 #define WRITE_TREE_IGNORE_CACHE_TREE 2 #define WRITE_TREE_DRY_RUN 4 #define WRITE_TREE_SILENT 8 #define WRITE_TREE_REPAIR 16 +#define WRITE_TREE_FROM_MEMORY 32 /* error return codes */ #define WRITE_TREE_UNREADABLE_INDEX (-1) diff --git a/merge-recursive.c b/merge-recursive.c index 308e350ff1..6c38c02e3f 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -412,37 +412,6 @@ static void unpack_trees_finish(struct merge_options *opt) clear_unpack_trees_porcelain(&opt->unpack_opts); } -struct tree *write_tree_from_memory(struct merge_options *opt) -{ - struct tree *result = NULL; - struct index_state *istate = opt->repo->index; - - if (unmerged_index(istate)) { - int i; - fprintf(stderr, "BUG: There are unmerged index entries:\n"); - for (i = 0; i < istate->cache_nr; i++) { - const struct cache_entry *ce = istate->cache[i]; - if (ce_stage(ce)) - fprintf(stderr, "BUG: %d %.*s\n", ce_stage(ce), - (int)ce_namelen(ce), ce->name); - } - BUG("unmerged index entries in merge-recursive.c"); - } - - if (!istate->cache_tree) - istate->cache_tree = cache_tree(); - - if (!cache_tree_fully_valid(istate->cache_tree) && - cache_tree_update(istate, 0) < 0) { - err(opt, _("error building trees")); - return NULL; - } - - result = lookup_tree(opt->repo, &istate->cache_tree->oid); - - return result; -} - static int save_files_dirs(const struct object_id *oid, struct strbuf *base, const char *path, unsigned int mode, int stage, void *context) @@ -3471,8 +3440,15 @@ static int merge_trees_internal(struct merge_options *opt, unpack_trees_finish(opt); - if (opt->call_depth && !(*result = write_tree_from_memory(opt))) - return -1; + if (opt->call_depth) { + struct object_id tree_id; + if (write_index_as_tree(&tree_id, opt->repo->index, NULL, + WRITE_TREE_FROM_MEMORY, NULL) || + !(*result = lookup_tree(opt->repo, &tree_id))) { + err(opt, _("error building trees")); + return -1; + } + } return clean; } diff --git a/merge-recursive.h b/merge-recursive.h index 812c456f1b..b3394502c7 100644 --- a/merge-recursive.h +++ b/merge-recursive.h @@ -109,7 +109,6 @@ int merge_recursive_generic(struct merge_options *o, void init_merge_options(struct merge_options *o, struct repository *repo); -struct tree *write_tree_from_memory(struct merge_options *o); int parse_merge_opt(struct merge_options *out, const char *s);
write_tree_from_memory() appeared to be a merge-recursive special that basically duplicated write_index_as_tree(). The two had a slightly different call structure but the big difference was just that write_index_as_tree() would always unconditionally read the index off of disk instead of working on the current in-memory index. Add a flag to allow using the in-memory index, and then switch over to write_index_as_tree(). Signed-off-by: Elijah Newren <newren@gmail.com> --- builtin/checkout.c | 6 +++++- cache-tree.c | 36 +++++++++++++++++++++++------------- cache-tree.h | 3 ++- merge-recursive.c | 42 +++++++++--------------------------------- merge-recursive.h | 1 - 5 files changed, 39 insertions(+), 49 deletions(-)