* [PATCH 0/6] Kill the_repository in tree-walk.c
@ 2019-06-24 9:55 Nguyễn Thái Ngọc Duy
2019-06-24 9:55 ` [PATCH 1/6] sha1-file.c: remove the_repo from read_object_with_reference() Nguyễn Thái Ngọc Duy
` (7 more replies)
0 siblings, 8 replies; 31+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2019-06-24 9:55 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy
This is the continuation of nd/sha1-name-c-wo-the-repository. In that
series I sealed off one place in sha1-name.c that cannot walk trees
from arbitrary repositories. With tree-walk.c taking 'struct
repository *' directly, that check in there can now be removed.
Nguyễn Thái Ngọc Duy (6):
sha1-file.c: remove the_repo from read_object_with_reference()
tree-walk.c: remove the_repo from fill_tree_descriptor()
tree-walk.c: remove the_repo from get_tree_entry()
tree-walk.c: remove the_repo from get_tree_entry_follow_symlinks()
match-trees.c: remove the_repo from shift_tree*()
Use the right 'struct repository' instead of the_repository
archive.c | 4 +++-
blame.c | 4 ++--
builtin/cat-file.c | 3 ++-
builtin/grep.c | 6 ++++--
builtin/merge-tree.c | 22 +++++++++++--------
builtin/pack-objects.c | 3 ++-
builtin/rebase.c | 4 ++--
builtin/reset.c | 4 ++--
builtin/rm.c | 2 +-
builtin/update-index.c | 2 +-
cache.h | 7 +++---
fast-import.c | 9 +++++---
line-log.c | 7 +++---
match-trees.c | 12 ++++++-----
merge-recursive.c | 43 +++++++++++++++++++++----------------
notes.c | 4 ++--
sequencer.c | 6 +++---
sha1-file.c | 5 +++--
sha1-name.c | 25 +++++++--------------
shallow.c | 3 ++-
t/helper/test-match-trees.c | 2 +-
tree-diff.c | 4 ++--
tree-walk.c | 35 ++++++++++++++++++++----------
tree-walk.h | 8 ++++---
unpack-trees.c | 2 +-
25 files changed, 129 insertions(+), 97 deletions(-)
--
2.22.0.rc0.322.g2b0371e29a
^ permalink raw reply [flat|nested] 31+ messages in thread* [PATCH 1/6] sha1-file.c: remove the_repo from read_object_with_reference() 2019-06-24 9:55 [PATCH 0/6] Kill the_repository in tree-walk.c Nguyễn Thái Ngọc Duy @ 2019-06-24 9:55 ` Nguyễn Thái Ngọc Duy 2019-06-27 12:54 ` Johannes Schindelin 2019-06-24 9:55 ` [PATCH 2/6] tree-walk.c: remove the_repo from fill_tree_descriptor() Nguyễn Thái Ngọc Duy ` (6 subsequent siblings) 7 siblings, 1 reply; 31+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2019-06-24 9:55 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> --- builtin/cat-file.c | 3 ++- builtin/grep.c | 6 ++++-- builtin/pack-objects.c | 3 ++- cache.h | 3 ++- fast-import.c | 9 ++++++--- sha1-file.c | 5 +++-- tree-walk.c | 7 ++++--- 7 files changed, 23 insertions(+), 13 deletions(-) diff --git a/builtin/cat-file.c b/builtin/cat-file.c index 0f092382e1..995d47c85a 100644 --- a/builtin/cat-file.c +++ b/builtin/cat-file.c @@ -172,7 +172,8 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name, * fall-back to the usual case. */ } - buf = read_object_with_reference(&oid, exp_type, &size, NULL); + buf = read_object_with_reference(the_repository, + &oid, exp_type, &size, NULL); break; default: diff --git a/builtin/grep.c b/builtin/grep.c index 580fd38f41..85da7ee542 100644 --- a/builtin/grep.c +++ b/builtin/grep.c @@ -458,7 +458,8 @@ static int grep_submodule(struct grep_opt *opt, object = parse_object_or_die(oid, oid_to_hex(oid)); grep_read_lock(); - data = read_object_with_reference(&object->oid, tree_type, + data = read_object_with_reference(opt->repo, + &object->oid, tree_type, &size, NULL); grep_read_unlock(); @@ -623,7 +624,8 @@ static int grep_object(struct grep_opt *opt, const struct pathspec *pathspec, int hit, len; grep_read_lock(); - data = read_object_with_reference(&obj->oid, tree_type, + data = read_object_with_reference(opt->repo, + &obj->oid, tree_type, &size, NULL); grep_read_unlock(); diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index b2be8869c2..a030c24a4a 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -1428,7 +1428,8 @@ static void add_preferred_base(struct object_id *oid) if (window <= num_preferred_base++) return; - data = read_object_with_reference(oid, tree_type, &size, &tree_oid); + data = read_object_with_reference(the_repository, oid, + tree_type, &size, &tree_oid); if (!data) return; diff --git a/cache.h b/cache.h index bf20337ef4..cd84cc9bbe 100644 --- a/cache.h +++ b/cache.h @@ -1500,7 +1500,8 @@ int df_name_compare(const char *name1, int len1, int mode1, const char *name2, i int name_compare(const char *name1, size_t len1, const char *name2, size_t len2); int cache_name_stage_compare(const char *name1, int len1, int stage1, const char *name2, int len2, int stage2); -void *read_object_with_reference(const struct object_id *oid, +void *read_object_with_reference(struct repository *r, + const struct object_id *oid, const char *required_type, unsigned long *size, struct object_id *oid_ret); diff --git a/fast-import.c b/fast-import.c index 76a7bd3699..3970b50acc 100644 --- a/fast-import.c +++ b/fast-import.c @@ -2410,7 +2410,8 @@ static void note_change_n(const char *p, struct branch *b, unsigned char *old_fa oidcpy(&commit_oid, &commit_oe->idx.oid); } else if (!get_oid(p, &commit_oid)) { unsigned long size; - char *buf = read_object_with_reference(&commit_oid, + char *buf = read_object_with_reference(the_repository, + &commit_oid, commit_type, &size, &commit_oid); if (!buf || size < the_hash_algo->hexsz + 6) @@ -2482,7 +2483,8 @@ static void parse_from_existing(struct branch *b) unsigned long size; char *buf; - buf = read_object_with_reference(&b->oid, commit_type, &size, + buf = read_object_with_reference(the_repository, + &b->oid, commit_type, &size, &b->oid); parse_from_commit(b, buf, size); free(buf); @@ -2560,7 +2562,8 @@ static struct hash_list *parse_merge(unsigned int *count) oidcpy(&n->oid, &oe->idx.oid); } else if (!get_oid(from, &n->oid)) { unsigned long size; - char *buf = read_object_with_reference(&n->oid, + char *buf = read_object_with_reference(the_repository, + &n->oid, commit_type, &size, &n->oid); if (!buf || size < the_hash_algo->hexsz + 6) diff --git a/sha1-file.c b/sha1-file.c index 888b6024d5..59b2e40cf3 100644 --- a/sha1-file.c +++ b/sha1-file.c @@ -1505,7 +1505,8 @@ void *read_object_file_extended(struct repository *r, return NULL; } -void *read_object_with_reference(const struct object_id *oid, +void *read_object_with_reference(struct repository *r, + const struct object_id *oid, const char *required_type_name, unsigned long *size, struct object_id *actual_oid_return) @@ -1521,7 +1522,7 @@ void *read_object_with_reference(const struct object_id *oid, int ref_length = -1; const char *ref_type = NULL; - buffer = read_object_file(&actual_oid, &type, &isize); + buffer = repo_read_object_file(r, &actual_oid, &type, &isize); if (!buffer) return NULL; if (type == required_type) { diff --git a/tree-walk.c b/tree-walk.c index ec32a47b2e..0c7722b220 100644 --- a/tree-walk.c +++ b/tree-walk.c @@ -87,7 +87,7 @@ void *fill_tree_descriptor(struct tree_desc *desc, const struct object_id *oid) void *buf = NULL; if (oid) { - buf = read_object_with_reference(oid, tree_type, &size, NULL); + buf = read_object_with_reference(the_repository, oid, tree_type, &size, NULL); if (!buf) die("unable to read tree %s", oid_to_hex(oid)); } @@ -542,7 +542,7 @@ int get_tree_entry(const struct object_id *tree_oid, const char *name, struct ob unsigned long size; struct object_id root; - tree = read_object_with_reference(tree_oid, tree_type, &size, &root); + tree = read_object_with_reference(the_repository, tree_oid, tree_type, &size, &root); if (!tree) return -1; @@ -609,7 +609,8 @@ enum get_oid_result get_tree_entry_follow_symlinks(struct object_id *tree_oid, c void *tree; struct object_id root; unsigned long size; - tree = read_object_with_reference(¤t_tree_oid, + tree = read_object_with_reference(the_repository, + ¤t_tree_oid, tree_type, &size, &root); if (!tree) -- 2.22.0.rc0.322.g2b0371e29a ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH 1/6] sha1-file.c: remove the_repo from read_object_with_reference() 2019-06-24 9:55 ` [PATCH 1/6] sha1-file.c: remove the_repo from read_object_with_reference() Nguyễn Thái Ngọc Duy @ 2019-06-27 12:54 ` Johannes Schindelin 2019-06-27 13:12 ` Duy Nguyen 0 siblings, 1 reply; 31+ messages in thread From: Johannes Schindelin @ 2019-06-27 12:54 UTC (permalink / raw) To: Nguyễn Thái Ngọc Duy; +Cc: git, Junio C Hamano [-- Attachment #1: Type: text/plain, Size: 1607 bytes --] Hi Duy, On Mon, 24 Jun 2019, Nguyễn Thái Ngọc Duy wrote: > diff --git a/builtin/grep.c b/builtin/grep.c > index 580fd38f41..85da7ee542 100644 > --- a/builtin/grep.c > +++ b/builtin/grep.c > @@ -458,7 +458,8 @@ static int grep_submodule(struct grep_opt *opt, > object = parse_object_or_die(oid, oid_to_hex(oid)); > > grep_read_lock(); > - data = read_object_with_reference(&object->oid, tree_type, > + data = read_object_with_reference(opt->repo, > + &object->oid, tree_type, Junio's hunch was absolutely spot on. This conversion is incorrect. If you replace this `opt->repo` and... > &size, NULL); > grep_read_unlock(); > > @@ -623,7 +624,8 @@ static int grep_object(struct grep_opt *opt, const struct pathspec *pathspec, > int hit, len; > > grep_read_lock(); > - data = read_object_with_reference(&obj->oid, tree_type, > + data = read_object_with_reference(opt->repo, ... this one with `the_repository`, t7814 starts passing again. It makes me very wary of this patch series that this bug has only been caught by a CI build. You probably did not run the test suite before sending this patch series. I also wonder what the rationale was to deviate from the strategy used in the remainder of the call sites, where no attempt was made to use an already-available repository pointer that might, or might not, be the correct one. It strikes me as a pretty important goal of this patch series to _not_ change any behavior, and this bug makes me dubious that all diligence has been done to assure that. Ciao, Johannes ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 1/6] sha1-file.c: remove the_repo from read_object_with_reference() 2019-06-27 12:54 ` Johannes Schindelin @ 2019-06-27 13:12 ` Duy Nguyen 0 siblings, 0 replies; 31+ messages in thread From: Duy Nguyen @ 2019-06-27 13:12 UTC (permalink / raw) To: Johannes Schindelin; +Cc: Git Mailing List, Junio C Hamano On Thu, Jun 27, 2019 at 7:54 PM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote: > > Hi Duy, > > On Mon, 24 Jun 2019, Nguyễn Thái Ngọc Duy wrote: > > > diff --git a/builtin/grep.c b/builtin/grep.c > > index 580fd38f41..85da7ee542 100644 > > --- a/builtin/grep.c > > +++ b/builtin/grep.c > > @@ -458,7 +458,8 @@ static int grep_submodule(struct grep_opt *opt, > > object = parse_object_or_die(oid, oid_to_hex(oid)); > > > > grep_read_lock(); > > - data = read_object_with_reference(&object->oid, tree_type, > > + data = read_object_with_reference(opt->repo, > > + &object->oid, tree_type, > > Junio's hunch was absolutely spot on. This conversion is incorrect. If you > replace this `opt->repo` and... > > > &size, NULL); > > grep_read_unlock(); > > > > @@ -623,7 +624,8 @@ static int grep_object(struct grep_opt *opt, const struct pathspec *pathspec, > > int hit, len; > > > > grep_read_lock(); > > - data = read_object_with_reference(&obj->oid, tree_type, > > + data = read_object_with_reference(opt->repo, > > ... this one with `the_repository`, t7814 starts passing again. > > It makes me very wary of this patch series that this bug has only been > caught by a CI build. You probably did not run the test suite before > sending this patch series. I did. After Junio reported, I've ran a lot more and had the same pass/fail-sometimes behavior. > I also wonder what the rationale was to deviate from the strategy used in > the remainder of the call sites, where no attempt was made to use an > already-available repository pointer that might, or might not, be the > correct one. My strategy has always been "use the right repo if available, fall back to the_repo otherwise". This code path has struct repo, my mistake was not realize soon enough that there are two repos, not once (Ironically I made the conversion to add subrepo here). > It strikes me as a pretty important goal of this patch series to _not_ > change any behavior, and this bug makes me dubious that all diligence has > been done to assure that. Sooner or later all the_repo must be converted, what makes _this_ series different from other conversion series? Yes I slipped, I should have been more careful to the parts related to submodule. -- Duy ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 2/6] tree-walk.c: remove the_repo from fill_tree_descriptor() 2019-06-24 9:55 [PATCH 0/6] Kill the_repository in tree-walk.c Nguyễn Thái Ngọc Duy 2019-06-24 9:55 ` [PATCH 1/6] sha1-file.c: remove the_repo from read_object_with_reference() Nguyễn Thái Ngọc Duy @ 2019-06-24 9:55 ` Nguyễn Thái Ngọc Duy 2019-06-24 13:30 ` Derrick Stolee 2019-06-24 9:55 ` [PATCH 3/6] tree-walk.c: remove the_repo from get_tree_entry() Nguyễn Thái Ngọc Duy ` (5 subsequent siblings) 7 siblings, 1 reply; 31+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2019-06-24 9:55 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy While at there, clean up the_repo usage in builtin/merge-tree.c a tiny bit. Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> --- builtin/merge-tree.c | 22 +++++++++++++--------- builtin/rebase.c | 4 ++-- builtin/reset.c | 4 ++-- notes.c | 2 +- sequencer.c | 2 +- tree-diff.c | 4 ++-- tree-walk.c | 6 ++++-- tree-walk.h | 4 +++- unpack-trees.c | 2 +- 9 files changed, 29 insertions(+), 21 deletions(-) diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c index 34ca0258b1..97b54caeb9 100644 --- a/builtin/merge-tree.c +++ b/builtin/merge-tree.c @@ -205,6 +205,7 @@ static void resolve(const struct traverse_info *info, struct name_entry *ours, s static void unresolved_directory(const struct traverse_info *info, struct name_entry n[3]) { + struct repository *r = the_repository; char *newbase; struct name_entry *p; struct tree_desc t[3]; @@ -220,9 +221,9 @@ static void unresolved_directory(const struct traverse_info *info, newbase = traverse_path(info, p); #define ENTRY_OID(e) (((e)->mode && S_ISDIR((e)->mode)) ? &(e)->oid : NULL) - buf0 = fill_tree_descriptor(t + 0, ENTRY_OID(n + 0)); - buf1 = fill_tree_descriptor(t + 1, ENTRY_OID(n + 1)); - buf2 = fill_tree_descriptor(t + 2, ENTRY_OID(n + 2)); + buf0 = fill_tree_descriptor(r, t + 0, ENTRY_OID(n + 0)); + buf1 = fill_tree_descriptor(r, t + 1, ENTRY_OID(n + 1)); + buf2 = fill_tree_descriptor(r, t + 2, ENTRY_OID(n + 2)); #undef ENTRY_OID merge_trees(t, newbase); @@ -351,14 +352,16 @@ static void merge_trees(struct tree_desc t[3], const char *base) traverse_trees(&the_index, 3, t, &info); } -static void *get_tree_descriptor(struct tree_desc *desc, const char *rev) +static void *get_tree_descriptor(struct repository *r, + struct tree_desc *desc, + const char *rev) { struct object_id oid; void *buf; - if (get_oid(rev, &oid)) + if (repo_get_oid(r, rev, &oid)) die("unknown rev %s", rev); - buf = fill_tree_descriptor(desc, &oid); + buf = fill_tree_descriptor(r, desc, &oid); if (!buf) die("%s is not a tree", rev); return buf; @@ -366,15 +369,16 @@ static void *get_tree_descriptor(struct tree_desc *desc, const char *rev) int cmd_merge_tree(int argc, const char **argv, const char *prefix) { + struct repository *r = the_repository; struct tree_desc t[3]; void *buf1, *buf2, *buf3; if (argc != 4) usage(merge_tree_usage); - buf1 = get_tree_descriptor(t+0, argv[1]); - buf2 = get_tree_descriptor(t+1, argv[2]); - buf3 = get_tree_descriptor(t+2, argv[3]); + buf1 = get_tree_descriptor(r, t+0, argv[1]); + buf2 = get_tree_descriptor(r, t+1, argv[2]); + buf3 = get_tree_descriptor(r, t+2, argv[3]); merge_trees(t, ""); free(buf1); free(buf2); diff --git a/builtin/rebase.c b/builtin/rebase.c index b8116db487..28490f5f88 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -840,13 +840,13 @@ static int reset_head(struct object_id *oid, const char *action, goto leave_reset_head; } - if (!reset_hard && !fill_tree_descriptor(&desc[nr++], &head_oid)) { + if (!reset_hard && !fill_tree_descriptor(the_repository, &desc[nr++], &head_oid)) { ret = error(_("failed to find tree of %s"), oid_to_hex(&head_oid)); goto leave_reset_head; } - if (!fill_tree_descriptor(&desc[nr++], oid)) { + if (!fill_tree_descriptor(the_repository, &desc[nr++], oid)) { ret = error(_("failed to find tree of %s"), oid_to_hex(oid)); goto leave_reset_head; } diff --git a/builtin/reset.c b/builtin/reset.c index 26ef9a7bd0..77c38f28c2 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -79,13 +79,13 @@ static int reset_index(const struct object_id *oid, int reset_type, int quiet) struct object_id head_oid; if (get_oid("HEAD", &head_oid)) return error(_("You do not have a valid HEAD.")); - if (!fill_tree_descriptor(desc + nr, &head_oid)) + if (!fill_tree_descriptor(the_repository, desc + nr, &head_oid)) return error(_("Failed to find tree of HEAD.")); nr++; opts.fn = twoway_merge; } - if (!fill_tree_descriptor(desc + nr, oid)) { + if (!fill_tree_descriptor(the_repository, desc + nr, oid)) { error(_("Failed to find tree of %s."), oid_to_hex(oid)); goto out; } diff --git a/notes.c b/notes.c index 532ec37865..2522b87d77 100644 --- a/notes.c +++ b/notes.c @@ -397,7 +397,7 @@ static void load_subtree(struct notes_tree *t, struct leaf_node *subtree, struct name_entry entry; const unsigned hashsz = the_hash_algo->rawsz; - buf = fill_tree_descriptor(&desc, &subtree->val_oid); + buf = fill_tree_descriptor(the_repository, &desc, &subtree->val_oid); if (!buf) die("Could not read %s for notes-index", oid_to_hex(&subtree->val_oid)); diff --git a/sequencer.c b/sequencer.c index ab74b6baf1..d565fcf2b1 100644 --- a/sequencer.c +++ b/sequencer.c @@ -3194,7 +3194,7 @@ static int do_reset(struct repository *r, return error_resolve_conflict(_(action_name(opts))); } - if (!fill_tree_descriptor(&desc, &oid)) { + if (!fill_tree_descriptor(r, &desc, &oid)) { error(_("failed to find tree of %s"), oid_to_hex(&oid)); rollback_lock_file(&lock); free((void *)desc.buffer); diff --git a/tree-diff.c b/tree-diff.c index f1f641eb6a..33ded7f8b3 100644 --- a/tree-diff.c +++ b/tree-diff.c @@ -422,8 +422,8 @@ static struct combine_diff_path *ll_diff_tree_paths( * diff_tree_oid(parent, commit) ) */ for (i = 0; i < nparent; ++i) - tptree[i] = fill_tree_descriptor(&tp[i], parents_oid[i]); - ttree = fill_tree_descriptor(&t, oid); + tptree[i] = fill_tree_descriptor(opt->repo, &tp[i], parents_oid[i]); + ttree = fill_tree_descriptor(opt->repo, &t, oid); /* Enable recursion indefinitely */ opt->pathspec.recursive = opt->flags.recursive; diff --git a/tree-walk.c b/tree-walk.c index 0c7722b220..c5569b3e9f 100644 --- a/tree-walk.c +++ b/tree-walk.c @@ -81,13 +81,15 @@ int init_tree_desc_gently(struct tree_desc *desc, const void *buffer, unsigned l return result; } -void *fill_tree_descriptor(struct tree_desc *desc, const struct object_id *oid) +void *fill_tree_descriptor(struct repository *r, + struct tree_desc *desc, + const struct object_id *oid) { unsigned long size = 0; void *buf = NULL; if (oid) { - buf = read_object_with_reference(the_repository, oid, tree_type, &size, NULL); + buf = read_object_with_reference(r, oid, tree_type, &size, NULL); if (!buf) die("unable to read tree %s", oid_to_hex(oid)); } diff --git a/tree-walk.h b/tree-walk.h index 161e2400f4..9aa1042642 100644 --- a/tree-walk.h +++ b/tree-walk.h @@ -45,7 +45,9 @@ int init_tree_desc_gently(struct tree_desc *desc, const void *buf, unsigned long int tree_entry(struct tree_desc *, struct name_entry *); int tree_entry_gently(struct tree_desc *, struct name_entry *); -void *fill_tree_descriptor(struct tree_desc *desc, const struct object_id *oid); +void *fill_tree_descriptor(struct repository *r, + struct tree_desc *desc, + const struct object_id *oid); struct traverse_info; typedef int (*traverse_callback_t)(int n, unsigned long mask, unsigned long dirmask, struct name_entry *entry, struct traverse_info *); diff --git a/unpack-trees.c b/unpack-trees.c index 50189909b8..cfe1c5ec6f 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -840,7 +840,7 @@ static int traverse_trees_recursive(int n, unsigned long dirmask, const struct object_id *oid = NULL; if (dirmask & 1) oid = &names[i].oid; - buf[nr_buf++] = fill_tree_descriptor(t + i, oid); + buf[nr_buf++] = fill_tree_descriptor(the_repository, t + i, oid); } } -- 2.22.0.rc0.322.g2b0371e29a ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH 2/6] tree-walk.c: remove the_repo from fill_tree_descriptor() 2019-06-24 9:55 ` [PATCH 2/6] tree-walk.c: remove the_repo from fill_tree_descriptor() Nguyễn Thái Ngọc Duy @ 2019-06-24 13:30 ` Derrick Stolee 2019-06-26 16:27 ` Junio C Hamano 0 siblings, 1 reply; 31+ messages in thread From: Derrick Stolee @ 2019-06-24 13:30 UTC (permalink / raw) To: Nguyễn Thái Ngọc Duy, git; +Cc: Junio C Hamano On 6/24/2019 5:55 AM, Nguyễn Thái Ngọc Duy wrote: > While at there, clean up the_repo usage in builtin/merge-tree.c a tiny > bit. > > Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> > --- > builtin/merge-tree.c | 22 +++++++++++++--------- > builtin/rebase.c | 4 ++-- > builtin/reset.c | 4 ++-- > notes.c | 2 +- > sequencer.c | 2 +- > tree-diff.c | 4 ++-- > tree-walk.c | 6 ++++-- > tree-walk.h | 4 +++- > unpack-trees.c | 2 +- > 9 files changed, 29 insertions(+), 21 deletions(-) > > diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c > index 34ca0258b1..97b54caeb9 100644 > --- a/builtin/merge-tree.c > +++ b/builtin/merge-tree.c > @@ -205,6 +205,7 @@ static void resolve(const struct traverse_info *info, struct name_entry *ours, s > static void unresolved_directory(const struct traverse_info *info, > struct name_entry n[3]) > { > + struct repository *r = the_repository; I like this trick to make the change below minimal: > + buf0 = fill_tree_descriptor(r, t + 0, ENTRY_OID(n + 0)); > + buf1 = fill_tree_descriptor(r, t + 1, ENTRY_OID(n + 1)); > + buf2 = fill_tree_descriptor(r, t + 2, ENTRY_OID(n + 2)); I wonder if _every_ conversion should include this trick, so when we move to change that method we simply move the definition from the method block to the prototype. (No need to adjust what you've done already, just an idea for future conversions.) Thanks, -Stolee ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 2/6] tree-walk.c: remove the_repo from fill_tree_descriptor() 2019-06-24 13:30 ` Derrick Stolee @ 2019-06-26 16:27 ` Junio C Hamano 2019-06-26 16:47 ` Derrick Stolee 0 siblings, 1 reply; 31+ messages in thread From: Junio C Hamano @ 2019-06-26 16:27 UTC (permalink / raw) To: Derrick Stolee; +Cc: Nguyễn Thái Ngọc Duy, git Derrick Stolee <stolee@gmail.com> writes: >> diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c >> index 34ca0258b1..97b54caeb9 100644 >> --- a/builtin/merge-tree.c >> +++ b/builtin/merge-tree.c >> @@ -205,6 +205,7 @@ static void resolve(const struct traverse_info *info, struct name_entry *ours, s >> static void unresolved_directory(const struct traverse_info *info, >> struct name_entry n[3]) >> { >> + struct repository *r = the_repository; > > I like this trick to make the change below minimal: >> + buf0 = fill_tree_descriptor(r, t + 0, ENTRY_OID(n + 0)); >> + buf1 = fill_tree_descriptor(r, t + 1, ENTRY_OID(n + 1)); >> + buf2 = fill_tree_descriptor(r, t + 2, ENTRY_OID(n + 2)); > > I wonder if _every_ conversion should include this trick, > so when we move to change that method we simply move the definition > from the method block to the prototype. (No need to adjust what you've > done already, just an idea for future conversions.) Hmm, interesting. So those callers in builtin/rebase.c::reset_head() and other places that adds the_repository as the new first parameter can take a local variable "r" (or perhaps a bit more descriptive, e.g. "repo") that is initialized to "the_repository" (and never reassigned at least at this step) in this same patch? ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 2/6] tree-walk.c: remove the_repo from fill_tree_descriptor() 2019-06-26 16:27 ` Junio C Hamano @ 2019-06-26 16:47 ` Derrick Stolee 0 siblings, 0 replies; 31+ messages in thread From: Derrick Stolee @ 2019-06-26 16:47 UTC (permalink / raw) To: Junio C Hamano; +Cc: Nguyễn Thái Ngọc Duy, git On 6/26/2019 12:27 PM, Junio C Hamano wrote: > Derrick Stolee <stolee@gmail.com> writes: > >>> diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c >>> index 34ca0258b1..97b54caeb9 100644 >>> --- a/builtin/merge-tree.c >>> +++ b/builtin/merge-tree.c >>> @@ -205,6 +205,7 @@ static void resolve(const struct traverse_info *info, struct name_entry *ours, s >>> static void unresolved_directory(const struct traverse_info *info, >>> struct name_entry n[3]) >>> { >>> + struct repository *r = the_repository; >> >> I like this trick to make the change below minimal: >>> + buf0 = fill_tree_descriptor(r, t + 0, ENTRY_OID(n + 0)); >>> + buf1 = fill_tree_descriptor(r, t + 1, ENTRY_OID(n + 1)); >>> + buf2 = fill_tree_descriptor(r, t + 2, ENTRY_OID(n + 2)); >> >> I wonder if _every_ conversion should include this trick, >> so when we move to change that method we simply move the definition >> from the method block to the prototype. (No need to adjust what you've >> done already, just an idea for future conversions.) > > Hmm, interesting. So those callers in builtin/rebase.c::reset_head() > and other places that adds the_repository as the new first parameter > can take a local variable "r" (or perhaps a bit more descriptive, > e.g. "repo") that is initialized to "the_repository" (and never > reassigned at least at this step) in this same patch? Yes, that is what I was thinking. It means that we can stop munging the function calls for the method that is converted. When the caller is updated, the call site already uses "r" but we change how "r" is initialized (as a parameter instead of a local variable). Thanks, -Stolee ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 3/6] tree-walk.c: remove the_repo from get_tree_entry() 2019-06-24 9:55 [PATCH 0/6] Kill the_repository in tree-walk.c Nguyễn Thái Ngọc Duy 2019-06-24 9:55 ` [PATCH 1/6] sha1-file.c: remove the_repo from read_object_with_reference() Nguyễn Thái Ngọc Duy 2019-06-24 9:55 ` [PATCH 2/6] tree-walk.c: remove the_repo from fill_tree_descriptor() Nguyễn Thái Ngọc Duy @ 2019-06-24 9:55 ` Nguyễn Thái Ngọc Duy 2019-06-24 14:20 ` Derrick Stolee 2019-06-24 9:55 ` [PATCH 4/6] tree-walk.c: remove the_repo from get_tree_entry_follow_symlinks() Nguyễn Thái Ngọc Duy ` (4 subsequent siblings) 7 siblings, 1 reply; 31+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2019-06-24 9:55 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> --- archive.c | 4 +++- blame.c | 4 ++-- builtin/rm.c | 2 +- builtin/update-index.c | 2 +- line-log.c | 7 ++++--- match-trees.c | 6 +++--- merge-recursive.c | 8 +++++--- notes.c | 2 +- sha1-name.c | 9 +++++---- tree-walk.c | 18 ++++++++++++------ tree-walk.h | 2 +- 11 files changed, 38 insertions(+), 26 deletions(-) diff --git a/archive.c b/archive.c index 53141c1f0e..a8da0fcc4f 100644 --- a/archive.c +++ b/archive.c @@ -418,7 +418,9 @@ static void parse_treeish_arg(const char **argv, unsigned short mode; int err; - err = get_tree_entry(&tree->object.oid, prefix, &tree_oid, + err = get_tree_entry(ar_args->repo, + &tree->object.oid, + prefix, &tree_oid, &mode); if (err || !S_ISDIR(mode)) die(_("current working directory is untracked")); diff --git a/blame.c b/blame.c index 145eaf2faf..ef022809e9 100644 --- a/blame.c +++ b/blame.c @@ -101,7 +101,7 @@ static void verify_working_tree_path(struct repository *r, struct object_id blob_oid; unsigned short mode; - if (!get_tree_entry(commit_oid, path, &blob_oid, &mode) && + if (!get_tree_entry(r, commit_oid, path, &blob_oid, &mode) && oid_object_info(r, &blob_oid, NULL) == OBJ_BLOB) return; } @@ -532,7 +532,7 @@ static int fill_blob_sha1_and_mode(struct repository *r, { if (!is_null_oid(&origin->blob_oid)) return 0; - if (get_tree_entry(&origin->commit->object.oid, origin->path, &origin->blob_oid, &origin->mode)) + if (get_tree_entry(r, &origin->commit->object.oid, origin->path, &origin->blob_oid, &origin->mode)) goto error_out; if (oid_object_info(r, &origin->blob_oid, NULL) != OBJ_BLOB) goto error_out; diff --git a/builtin/rm.c b/builtin/rm.c index be8edc6d1e..2eacda42b4 100644 --- a/builtin/rm.c +++ b/builtin/rm.c @@ -179,7 +179,7 @@ static int check_local_mod(struct object_id *head, int index_only) * way as changed from the HEAD. */ if (no_head - || get_tree_entry(head, name, &oid, &mode) + || get_tree_entry(the_repository, head, name, &oid, &mode) || ce->ce_mode != create_ce_mode(mode) || !oideq(&ce->oid, &oid)) staged_changes = 1; diff --git a/builtin/update-index.c b/builtin/update-index.c index 3f8cc6ccb4..dff2f4b837 100644 --- a/builtin/update-index.c +++ b/builtin/update-index.c @@ -601,7 +601,7 @@ static struct cache_entry *read_one_ent(const char *which, struct object_id oid; struct cache_entry *ce; - if (get_tree_entry(ent, path, &oid, &mode)) { + if (get_tree_entry(the_repository, ent, path, &oid, &mode)) { if (which) error("%s: not in %s branch.", path, which); return NULL; diff --git a/line-log.c b/line-log.c index 0a17b21187..3aff1849e7 100644 --- a/line-log.c +++ b/line-log.c @@ -496,12 +496,13 @@ static struct commit *check_single_commit(struct rev_info *revs) return (struct commit *) commit; } -static void fill_blob_sha1(struct commit *commit, struct diff_filespec *spec) +static void fill_blob_sha1(struct repository *r, struct commit *commit, + struct diff_filespec *spec) { unsigned short mode; struct object_id oid; - if (get_tree_entry(&commit->object.oid, spec->path, &oid, &mode)) + if (get_tree_entry(r, &commit->object.oid, spec->path, &oid, &mode)) die("There is no path %s in the commit", spec->path); fill_filespec(spec, &oid, 1, mode); @@ -585,7 +586,7 @@ parse_lines(struct repository *r, struct commit *commit, name_part); spec = alloc_filespec(full_name); - fill_blob_sha1(commit, spec); + fill_blob_sha1(r, commit, spec); fill_line_ends(r, spec, &lines, &ends); cb_data.spec = spec; cb_data.lines = lines; diff --git a/match-trees.c b/match-trees.c index 9d1ec8d6b0..de7e8a6783 100644 --- a/match-trees.c +++ b/match-trees.c @@ -290,7 +290,7 @@ void shift_tree(const struct object_id *hash1, if (!*del_prefix) return; - if (get_tree_entry(hash2, del_prefix, shifted, &mode)) + if (get_tree_entry(the_repository, hash2, del_prefix, shifted, &mode)) die("cannot find path %s in tree %s", del_prefix, oid_to_hex(hash2)); return; @@ -317,12 +317,12 @@ void shift_tree_by(const struct object_id *hash1, unsigned candidate = 0; /* Can hash2 be a tree at shift_prefix in tree hash1? */ - if (!get_tree_entry(hash1, shift_prefix, &sub1, &mode1) && + if (!get_tree_entry(the_repository, hash1, shift_prefix, &sub1, &mode1) && S_ISDIR(mode1)) candidate |= 1; /* Can hash1 be a tree at shift_prefix in tree hash2? */ - if (!get_tree_entry(hash2, shift_prefix, &sub2, &mode2) && + if (!get_tree_entry(the_repository, hash2, shift_prefix, &sub2, &mode2) && S_ISDIR(mode2)) candidate |= 2; diff --git a/merge-recursive.c b/merge-recursive.c index d2e380b7ed..b051066795 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -475,7 +475,7 @@ static int get_tree_entry_if_blob(const struct object_id *tree, { int ret; - ret = get_tree_entry(tree, path, &dfs->oid, &dfs->mode); + ret = get_tree_entry(the_repository, tree, path, &dfs->oid, &dfs->mode); if (S_ISDIR(dfs->mode)) { oidcpy(&dfs->oid, &null_oid); dfs->mode = 0; @@ -1905,7 +1905,8 @@ static int tree_has_path(struct tree *tree, const char *path) struct object_id hashy; unsigned short mode_o; - return !get_tree_entry(&tree->object.oid, path, + return !get_tree_entry(the_repository, + &tree->object.oid, path, &hashy, &mode_o); } @@ -2500,7 +2501,8 @@ static void apply_directory_rename_modifications(struct merge_options *opt, * the various handle_rename_*() functions update the index * explicitly rather than relying on unpack_trees() to have done it. */ - get_tree_entry(&tree->object.oid, + get_tree_entry(opt->repo, + &tree->object.oid, pair->two->path, &re->dst_entry->stages[stage].oid, &re->dst_entry->stages[stage].mode); diff --git a/notes.c b/notes.c index 2522b87d77..75c028b300 100644 --- a/notes.c +++ b/notes.c @@ -1015,7 +1015,7 @@ void init_notes(struct notes_tree *t, const char *notes_ref, return; if (flags & NOTES_INIT_WRITABLE && read_ref(notes_ref, &object_oid)) die("Cannot use notes ref %s", notes_ref); - if (get_tree_entry(&object_oid, "", &oid, &mode)) + if (get_tree_entry(the_repository, &object_oid, "", &oid, &mode)) die("Failed to read notes tree referenced by %s (%s)", notes_ref, oid_to_hex(&object_oid)); diff --git a/sha1-name.c b/sha1-name.c index 728e6f1f61..e8fb215e5c 100644 --- a/sha1-name.c +++ b/sha1-name.c @@ -1677,7 +1677,8 @@ int repo_get_oid_blob(struct repository *r, } /* Must be called only when object_name:filename doesn't exist. */ -static void diagnose_invalid_oid_path(const char *prefix, +static void diagnose_invalid_oid_path(struct repository *r, + const char *prefix, const char *filename, const struct object_id *tree_oid, const char *object_name, @@ -1695,7 +1696,7 @@ static void diagnose_invalid_oid_path(const char *prefix, if (is_missing_file_error(errno)) { char *fullname = xstrfmt("%s%s", prefix, filename); - if (!get_tree_entry(tree_oid, fullname, &oid, &mode)) { + if (!get_tree_entry(r, tree_oid, fullname, &oid, &mode)) { die("Path '%s' exists, but not '%s'.\n" "Did you mean '%.*s:%s' aka '%.*s:./%s'?", fullname, @@ -1902,10 +1903,10 @@ static enum get_oid_result get_oid_with_context_1(struct repository *repo, filename, oid, &oc->symlink_path, &oc->mode); } else { - ret = get_tree_entry(&tree_oid, filename, oid, + ret = get_tree_entry(repo, &tree_oid, filename, oid, &oc->mode); if (ret && only_to_die) { - diagnose_invalid_oid_path(prefix, + diagnose_invalid_oid_path(repo, prefix, filename, &tree_oid, name, len); diff --git a/tree-walk.c b/tree-walk.c index c5569b3e9f..506e12a031 100644 --- a/tree-walk.c +++ b/tree-walk.c @@ -502,7 +502,9 @@ struct dir_state { struct object_id oid; }; -static int find_tree_entry(struct tree_desc *t, const char *name, struct object_id *result, unsigned short *mode) +static int find_tree_entry(struct repository *r, struct tree_desc *t, + const char *name, struct object_id *result, + unsigned short *mode) { int namelen = strlen(name); while (t->size) { @@ -532,19 +534,23 @@ static int find_tree_entry(struct tree_desc *t, const char *name, struct object_ oidcpy(result, &oid); return 0; } - return get_tree_entry(&oid, name + entrylen, result, mode); + return get_tree_entry(r, &oid, name + entrylen, result, mode); } return -1; } -int get_tree_entry(const struct object_id *tree_oid, const char *name, struct object_id *oid, unsigned short *mode) +int get_tree_entry(struct repository *r, + const struct object_id *tree_oid, + const char *name, + struct object_id *oid, + unsigned short *mode) { int retval; void *tree; unsigned long size; struct object_id root; - tree = read_object_with_reference(the_repository, tree_oid, tree_type, &size, &root); + tree = read_object_with_reference(r, tree_oid, tree_type, &size, &root); if (!tree) return -1; @@ -559,7 +565,7 @@ int get_tree_entry(const struct object_id *tree_oid, const char *name, struct ob } else { struct tree_desc t; init_tree_desc(&t, tree, size); - retval = find_tree_entry(&t, name, oid, mode); + retval = find_tree_entry(r, &t, name, oid, mode); } free(tree); return retval; @@ -681,7 +687,7 @@ enum get_oid_result get_tree_entry_follow_symlinks(struct object_id *tree_oid, c } /* Look up the first (or only) path component in the tree. */ - find_result = find_tree_entry(&t, namebuf.buf, + find_result = find_tree_entry(the_repository, &t, namebuf.buf, ¤t_tree_oid, mode); if (find_result) { goto done; diff --git a/tree-walk.h b/tree-walk.h index 9aa1042642..639f79187f 100644 --- a/tree-walk.h +++ b/tree-walk.h @@ -68,7 +68,7 @@ struct traverse_info { int show_all_errors; }; -int get_tree_entry(const struct object_id *, const char *, struct object_id *, unsigned short *); +int get_tree_entry(struct repository *, const struct object_id *, const char *, struct object_id *, unsigned short *); char *make_traverse_path(char *path, const struct traverse_info *info, const struct name_entry *n); void setup_traverse_info(struct traverse_info *info, const char *base); -- 2.22.0.rc0.322.g2b0371e29a ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH 3/6] tree-walk.c: remove the_repo from get_tree_entry() 2019-06-24 9:55 ` [PATCH 3/6] tree-walk.c: remove the_repo from get_tree_entry() Nguyễn Thái Ngọc Duy @ 2019-06-24 14:20 ` Derrick Stolee 2019-06-24 14:55 ` Duy Nguyen 0 siblings, 1 reply; 31+ messages in thread From: Derrick Stolee @ 2019-06-24 14:20 UTC (permalink / raw) To: Nguyễn Thái Ngọc Duy, git; +Cc: Junio C Hamano On 6/24/2019 5:55 AM, Nguyễn Thái Ngọc Duy wrote: > Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> > --- > archive.c | 4 +++- > blame.c | 4 ++-- > builtin/rm.c | 2 +- > builtin/update-index.c | 2 +- > line-log.c | 7 ++++--- > match-trees.c | 6 +++--- > merge-recursive.c | 8 +++++--- > notes.c | 2 +- > sha1-name.c | 9 +++++---- > tree-walk.c | 18 ++++++++++++------ > tree-walk.h | 2 +- > 11 files changed, 38 insertions(+), 26 deletions(-) > > diff --git a/archive.c b/archive.c > index 53141c1f0e..a8da0fcc4f 100644 > --- a/archive.c > +++ b/archive.c > @@ -418,7 +418,9 @@ static void parse_treeish_arg(const char **argv, > unsigned short mode; > int err; > > - err = get_tree_entry(&tree->object.oid, prefix, &tree_oid, > + err = get_tree_entry(ar_args->repo, If I'm reading this correctly, this is a place where we previously converted to using a custom repository pointer but this function boundary reverted us to the_repository anyway. I know we have some tests around the commit-graph that ensures it works with an arbitrary repository (and I frequently stumble over them when I add new dependencies). How can we add more testing around these new conversions? The rest looks straight-forward. Thanks, -Stolee ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 3/6] tree-walk.c: remove the_repo from get_tree_entry() 2019-06-24 14:20 ` Derrick Stolee @ 2019-06-24 14:55 ` Duy Nguyen 0 siblings, 0 replies; 31+ messages in thread From: Duy Nguyen @ 2019-06-24 14:55 UTC (permalink / raw) To: Derrick Stolee; +Cc: Git Mailing List, Junio C Hamano On Mon, Jun 24, 2019 at 9:20 PM Derrick Stolee <stolee@gmail.com> wrote: > > On 6/24/2019 5:55 AM, Nguyễn Thái Ngọc Duy wrote: > > Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> > > --- > > archive.c | 4 +++- > > blame.c | 4 ++-- > > builtin/rm.c | 2 +- > > builtin/update-index.c | 2 +- > > line-log.c | 7 ++++--- > > match-trees.c | 6 +++--- > > merge-recursive.c | 8 +++++--- > > notes.c | 2 +- > > sha1-name.c | 9 +++++---- > > tree-walk.c | 18 ++++++++++++------ > > tree-walk.h | 2 +- > > 11 files changed, 38 insertions(+), 26 deletions(-) > > > > diff --git a/archive.c b/archive.c > > index 53141c1f0e..a8da0fcc4f 100644 > > --- a/archive.c > > +++ b/archive.c > > @@ -418,7 +418,9 @@ static void parse_treeish_arg(const char **argv, > > unsigned short mode; > > int err; > > > > - err = get_tree_entry(&tree->object.oid, prefix, &tree_oid, > > + err = get_tree_entry(ar_args->repo, > > If I'm reading this correctly, this is a place where we previously converted > to using a custom repository pointer but this function boundary reverted us > to the_repository anyway. I know we have some tests around the commit-graph > that ensures it works with an arbitrary repository (and I frequently stumble > over them when I add new dependencies). How can we add more testing around > these new conversions? Right now it's really patchy. There's no guarantee that the_repo is not used somwhere in the callchain (or will not be in the future). My main aim is _not_ break it when used with the_repo. These new conversions hopefully will get more used outside the default the_repo setting (e.g. new developments in git-submodule, or git-worktree). Eventually the_repo should be gone (or referenced in very few places), then the conversion will get more coverage. Really mixing repos though will not be as well tested until actually used (by submodule and friends). -- Duy ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 4/6] tree-walk.c: remove the_repo from get_tree_entry_follow_symlinks() 2019-06-24 9:55 [PATCH 0/6] Kill the_repository in tree-walk.c Nguyễn Thái Ngọc Duy ` (2 preceding siblings ...) 2019-06-24 9:55 ` [PATCH 3/6] tree-walk.c: remove the_repo from get_tree_entry() Nguyễn Thái Ngọc Duy @ 2019-06-24 9:55 ` Nguyễn Thái Ngọc Duy 2019-06-24 9:55 ` [PATCH 5/6] match-trees.c: remove the_repo from shift_tree*() Nguyễn Thái Ngọc Duy ` (3 subsequent siblings) 7 siblings, 0 replies; 31+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2019-06-24 9:55 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> --- sha1-name.c | 10 +--------- tree-walk.c | 12 ++++++++---- tree-walk.h | 2 +- 3 files changed, 10 insertions(+), 14 deletions(-) diff --git a/sha1-name.c b/sha1-name.c index e8fb215e5c..3c9fa10af8 100644 --- a/sha1-name.c +++ b/sha1-name.c @@ -1890,16 +1890,8 @@ static enum get_oid_result get_oid_with_context_1(struct repository *repo, new_filename = resolve_relative_path(repo, filename); if (new_filename) filename = new_filename; - /* - * NEEDSWORK: Eventually get_tree_entry*() should - * learn to take struct repository directly and we - * would not need to inject submodule odb to the - * in-core odb. - */ - if (repo != the_repository) - add_to_alternates_memory(repo->objects->odb->path); if (flags & GET_OID_FOLLOW_SYMLINKS) { - ret = get_tree_entry_follow_symlinks(&tree_oid, + ret = get_tree_entry_follow_symlinks(repo, &tree_oid, filename, oid, &oc->symlink_path, &oc->mode); } else { diff --git a/tree-walk.c b/tree-walk.c index 506e12a031..c20b62f49e 100644 --- a/tree-walk.c +++ b/tree-walk.c @@ -593,7 +593,10 @@ int get_tree_entry(struct repository *r, * See the code for enum get_oid_result for a description of * the return values. */ -enum get_oid_result get_tree_entry_follow_symlinks(struct object_id *tree_oid, const char *name, struct object_id *result, struct strbuf *result_path, unsigned short *mode) +enum get_oid_result get_tree_entry_follow_symlinks(struct repository *r, + struct object_id *tree_oid, const char *name, + struct object_id *result, struct strbuf *result_path, + unsigned short *mode) { int retval = MISSING_OBJECT; struct dir_state *parents = NULL; @@ -617,7 +620,7 @@ enum get_oid_result get_tree_entry_follow_symlinks(struct object_id *tree_oid, c void *tree; struct object_id root; unsigned long size; - tree = read_object_with_reference(the_repository, + tree = read_object_with_reference(r, ¤t_tree_oid, tree_type, &size, &root); @@ -687,7 +690,7 @@ enum get_oid_result get_tree_entry_follow_symlinks(struct object_id *tree_oid, c } /* Look up the first (or only) path component in the tree. */ - find_result = find_tree_entry(the_repository, &t, namebuf.buf, + find_result = find_tree_entry(r, &t, namebuf.buf, ¤t_tree_oid, mode); if (find_result) { goto done; @@ -731,7 +734,8 @@ enum get_oid_result get_tree_entry_follow_symlinks(struct object_id *tree_oid, c */ retval = DANGLING_SYMLINK; - contents = read_object_file(¤t_tree_oid, &type, + contents = repo_read_object_file(r, + ¤t_tree_oid, &type, &link_len); if (!contents) diff --git a/tree-walk.h b/tree-walk.h index 639f79187f..2a5db29e8f 100644 --- a/tree-walk.h +++ b/tree-walk.h @@ -53,7 +53,7 @@ struct traverse_info; typedef int (*traverse_callback_t)(int n, unsigned long mask, unsigned long dirmask, struct name_entry *entry, struct traverse_info *); int traverse_trees(struct index_state *istate, int n, struct tree_desc *t, struct traverse_info *info); -enum get_oid_result get_tree_entry_follow_symlinks(struct object_id *tree_oid, const char *name, struct object_id *result, struct strbuf *result_path, unsigned short *mode); +enum get_oid_result get_tree_entry_follow_symlinks(struct repository *r, struct object_id *tree_oid, const char *name, struct object_id *result, struct strbuf *result_path, unsigned short *mode); struct traverse_info { const char *traverse_path; -- 2.22.0.rc0.322.g2b0371e29a ^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH 5/6] match-trees.c: remove the_repo from shift_tree*() 2019-06-24 9:55 [PATCH 0/6] Kill the_repository in tree-walk.c Nguyễn Thái Ngọc Duy ` (3 preceding siblings ...) 2019-06-24 9:55 ` [PATCH 4/6] tree-walk.c: remove the_repo from get_tree_entry_follow_symlinks() Nguyễn Thái Ngọc Duy @ 2019-06-24 9:55 ` Nguyễn Thái Ngọc Duy 2019-06-24 9:55 ` [PATCH 6/6] Use the right 'struct repository' instead of the_repository Nguyễn Thái Ngọc Duy ` (2 subsequent siblings) 7 siblings, 0 replies; 31+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2019-06-24 9:55 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> --- cache.h | 4 ++-- match-trees.c | 12 +++++++----- merge-recursive.c | 4 ++-- t/helper/test-match-trees.c | 2 +- 4 files changed, 12 insertions(+), 10 deletions(-) diff --git a/cache.h b/cache.h index cd84cc9bbe..ddefda2bb6 100644 --- a/cache.h +++ b/cache.h @@ -1786,8 +1786,8 @@ int add_files_to_cache(const char *prefix, const struct pathspec *pathspec, int extern int diff_auto_refresh_index; /* match-trees.c */ -void shift_tree(const struct object_id *, const struct object_id *, struct object_id *, int); -void shift_tree_by(const struct object_id *, const struct object_id *, struct object_id *, const char *); +void shift_tree(struct repository *, const struct object_id *, const struct object_id *, struct object_id *, int); +void shift_tree_by(struct repository *, const struct object_id *, const struct object_id *, struct object_id *, const char *); /* * whitespace rules. diff --git a/match-trees.c b/match-trees.c index de7e8a6783..f6c194c1cc 100644 --- a/match-trees.c +++ b/match-trees.c @@ -248,7 +248,8 @@ static int splice_tree(const struct object_id *oid1, const char *prefix, * other hand, it could cover tree one and we might need to pick a * subtree of it. */ -void shift_tree(const struct object_id *hash1, +void shift_tree(struct repository *r, + const struct object_id *hash1, const struct object_id *hash2, struct object_id *shifted, int depth_limit) @@ -290,7 +291,7 @@ void shift_tree(const struct object_id *hash1, if (!*del_prefix) return; - if (get_tree_entry(the_repository, hash2, del_prefix, shifted, &mode)) + if (get_tree_entry(r, hash2, del_prefix, shifted, &mode)) die("cannot find path %s in tree %s", del_prefix, oid_to_hex(hash2)); return; @@ -307,7 +308,8 @@ void shift_tree(const struct object_id *hash1, * Unfortunately we cannot fundamentally tell which one to * be prefixed, as recursive merge can work in either direction. */ -void shift_tree_by(const struct object_id *hash1, +void shift_tree_by(struct repository *r, + const struct object_id *hash1, const struct object_id *hash2, struct object_id *shifted, const char *shift_prefix) @@ -317,12 +319,12 @@ void shift_tree_by(const struct object_id *hash1, unsigned candidate = 0; /* Can hash2 be a tree at shift_prefix in tree hash1? */ - if (!get_tree_entry(the_repository, hash1, shift_prefix, &sub1, &mode1) && + if (!get_tree_entry(r, hash1, shift_prefix, &sub1, &mode1) && S_ISDIR(mode1)) candidate |= 1; /* Can hash1 be a tree at shift_prefix in tree hash2? */ - if (!get_tree_entry(the_repository, hash2, shift_prefix, &sub2, &mode2) && + if (!get_tree_entry(r, hash2, shift_prefix, &sub2, &mode2) && S_ISDIR(mode2)) candidate |= 2; diff --git a/merge-recursive.c b/merge-recursive.c index b051066795..6d772eb0eb 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -153,9 +153,9 @@ static struct tree *shift_tree_object(struct repository *repo, struct object_id shifted; if (!*subtree_shift) { - shift_tree(&one->object.oid, &two->object.oid, &shifted, 0); + shift_tree(repo, &one->object.oid, &two->object.oid, &shifted, 0); } else { - shift_tree_by(&one->object.oid, &two->object.oid, &shifted, + shift_tree_by(repo, &one->object.oid, &two->object.oid, &shifted, subtree_shift); } if (oideq(&two->object.oid, &shifted)) diff --git a/t/helper/test-match-trees.c b/t/helper/test-match-trees.c index 96857f26ac..b9fd427571 100644 --- a/t/helper/test-match-trees.c +++ b/t/helper/test-match-trees.c @@ -20,7 +20,7 @@ int cmd__match_trees(int ac, const char **av) if (!two) die("not a tree-ish %s", av[2]); - shift_tree(&one->object.oid, &two->object.oid, &shifted, -1); + shift_tree(the_repository, &one->object.oid, &two->object.oid, &shifted, -1); printf("shifted: %s\n", oid_to_hex(&shifted)); exit(0); -- 2.22.0.rc0.322.g2b0371e29a ^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH 6/6] Use the right 'struct repository' instead of the_repository 2019-06-24 9:55 [PATCH 0/6] Kill the_repository in tree-walk.c Nguyễn Thái Ngọc Duy ` (4 preceding siblings ...) 2019-06-24 9:55 ` [PATCH 5/6] match-trees.c: remove the_repo from shift_tree*() Nguyễn Thái Ngọc Duy @ 2019-06-24 9:55 ` Nguyễn Thái Ngọc Duy 2019-06-24 14:24 ` Derrick Stolee 2019-06-27 9:06 ` Johannes Schindelin 2019-06-26 17:20 ` [PATCH 0/6] Kill the_repository in tree-walk.c Junio C Hamano 2019-06-27 9:28 ` [PATCH v2 " Nguyễn Thái Ngọc Duy 7 siblings, 2 replies; 31+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2019-06-24 9:55 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy There are a couple of places where 'struct repository' is already passed around, but the_repository is still used. Use the right repo. Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> --- merge-recursive.c | 35 ++++++++++++++++++++--------------- sequencer.c | 4 ++-- sha1-name.c | 6 ++---- shallow.c | 3 ++- 4 files changed, 26 insertions(+), 22 deletions(-) diff --git a/merge-recursive.c b/merge-recursive.c index 6d772eb0eb..12300131fc 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -465,17 +465,18 @@ static void get_files_dirs(struct merge_options *opt, struct tree *tree) { struct pathspec match_all; memset(&match_all, 0, sizeof(match_all)); - read_tree_recursive(the_repository, tree, "", 0, 0, + read_tree_recursive(opt->repo, tree, "", 0, 0, &match_all, save_files_dirs, opt); } -static int get_tree_entry_if_blob(const struct object_id *tree, +static int get_tree_entry_if_blob(struct repository *r, + const struct object_id *tree, const char *path, struct diff_filespec *dfs) { int ret; - ret = get_tree_entry(the_repository, tree, path, &dfs->oid, &dfs->mode); + ret = get_tree_entry(r, tree, path, &dfs->oid, &dfs->mode); if (S_ISDIR(dfs->mode)) { oidcpy(&dfs->oid, &null_oid); dfs->mode = 0; @@ -487,15 +488,16 @@ static int get_tree_entry_if_blob(const struct object_id *tree, * Returns an index_entry instance which doesn't have to correspond to * a real cache entry in Git's index. */ -static struct stage_data *insert_stage_data(const char *path, +static struct stage_data *insert_stage_data(struct repository *r, + const char *path, struct tree *o, struct tree *a, struct tree *b, struct string_list *entries) { struct string_list_item *item; struct stage_data *e = xcalloc(1, sizeof(struct stage_data)); - get_tree_entry_if_blob(&o->object.oid, path, &e->stages[1]); - get_tree_entry_if_blob(&a->object.oid, path, &e->stages[2]); - get_tree_entry_if_blob(&b->object.oid, path, &e->stages[3]); + get_tree_entry_if_blob(r, &o->object.oid, path, &e->stages[1]); + get_tree_entry_if_blob(r, &a->object.oid, path, &e->stages[2]); + get_tree_entry_if_blob(r, &b->object.oid, path, &e->stages[3]); item = string_list_insert(entries, path); item->util = e; return e; @@ -1900,12 +1902,13 @@ static struct diff_queue_struct *get_diffpairs(struct merge_options *opt, return ret; } -static int tree_has_path(struct tree *tree, const char *path) +static int tree_has_path(struct repository *r, struct tree *tree, + const char *path) { struct object_id hashy; unsigned short mode_o; - return !get_tree_entry(the_repository, + return !get_tree_entry(r, &tree->object.oid, path, &hashy, &mode_o); } @@ -2057,7 +2060,7 @@ static char *handle_path_level_conflicts(struct merge_options *opt, */ if (collision_ent->reported_already) { clean = 0; - } else if (tree_has_path(tree, new_path)) { + } else if (tree_has_path(opt->repo, tree, new_path)) { collision_ent->reported_already = 1; strbuf_add_separated_string_list(&collision_paths, ", ", &collision_ent->source_files); @@ -2135,7 +2138,7 @@ static void handle_directory_level_conflicts(struct merge_options *opt, string_list_append(&remove_from_merge, merge_ent->dir)->util = merge_ent; strbuf_release(&merge_ent->new_dir); - } else if (tree_has_path(head, head_ent->dir)) { + } else if (tree_has_path(opt->repo, head, head_ent->dir)) { /* 2. This wasn't a directory rename after all */ string_list_append(&remove_from_head, head_ent->dir)->util = head_ent; @@ -2149,7 +2152,7 @@ static void handle_directory_level_conflicts(struct merge_options *opt, hashmap_iter_init(dir_re_merge, &iter); while ((merge_ent = hashmap_iter_next(&iter))) { head_ent = dir_rename_find_entry(dir_re_head, merge_ent->dir); - if (tree_has_path(merge, merge_ent->dir)) { + if (tree_has_path(opt->repo, merge, merge_ent->dir)) { /* 2. This wasn't a directory rename after all */ string_list_append(&remove_from_merge, merge_ent->dir)->util = merge_ent; @@ -2478,7 +2481,7 @@ static void apply_directory_rename_modifications(struct merge_options *opt, if (pair->status == 'R') re->dst_entry->processed = 1; - re->dst_entry = insert_stage_data(new_path, + re->dst_entry = insert_stage_data(opt->repo, new_path, o_tree, a_tree, b_tree, entries); item = string_list_insert(entries, new_path); @@ -2587,14 +2590,16 @@ static struct string_list *get_renames(struct merge_options *opt, re->dir_rename_original_dest = NULL; item = string_list_lookup(entries, re->pair->one->path); if (!item) - re->src_entry = insert_stage_data(re->pair->one->path, + re->src_entry = insert_stage_data(opt->repo, + re->pair->one->path, o_tree, a_tree, b_tree, entries); else re->src_entry = item->util; item = string_list_lookup(entries, re->pair->two->path); if (!item) - re->dst_entry = insert_stage_data(re->pair->two->path, + re->dst_entry = insert_stage_data(opt->repo, + re->pair->two->path, o_tree, a_tree, b_tree, entries); else re->dst_entry = item->util; diff --git a/sequencer.c b/sequencer.c index d565fcf2b1..64428ac28f 100644 --- a/sequencer.c +++ b/sequencer.c @@ -3733,7 +3733,7 @@ static int pick_commits(struct repository *r, unlink(rebase_path_author_script()); unlink(rebase_path_stopped_sha()); unlink(rebase_path_amend()); - unlink(git_path_merge_head(the_repository)); + unlink(git_path_merge_head(r)); delete_ref(NULL, "REBASE_HEAD", NULL, REF_NO_DEREF); if (item->command == TODO_BREAK) @@ -4107,7 +4107,7 @@ static int commit_staged_changes(struct repository *r, opts, flags)) return error(_("could not commit staged changes.")); unlink(rebase_path_amend()); - unlink(git_path_merge_head(the_repository)); + unlink(git_path_merge_head(r)); if (final_fixup) { unlink(rebase_path_fixup_msg()); unlink(rebase_path_squash_msg()); diff --git a/sha1-name.c b/sha1-name.c index 3c9fa10af8..6069fe006b 100644 --- a/sha1-name.c +++ b/sha1-name.c @@ -478,7 +478,7 @@ static enum get_oid_result get_short_oid(struct repository *r, * or migrated from loose to packed. */ if (status == MISSING_OBJECT) { - reprepare_packed_git(the_repository); + reprepare_packed_git(r); find_short_object_filename(&ds); find_short_packed_object(&ds); status = finish_object_disambiguation(&ds, oid); @@ -1389,9 +1389,7 @@ int repo_get_oid_mb(struct repository *r, two = lookup_commit_reference_gently(r, &oid_tmp, 0); if (!two) return -1; - if (r != the_repository) - BUG("sorry get_merge_bases() can't take struct repository yet"); - mbs = get_merge_bases(one, two); + mbs = repo_get_merge_bases(r, one, two); if (!mbs || mbs->next) st = -1; else { diff --git a/shallow.c b/shallow.c index ce45297940..5fa2b15d37 100644 --- a/shallow.c +++ b/shallow.c @@ -248,7 +248,8 @@ static void check_shallow_file_for_update(struct repository *r) if (r->parsed_objects->is_shallow == -1) BUG("shallow must be initialized by now"); - if (!stat_validity_check(r->parsed_objects->shallow_stat, git_path_shallow(the_repository))) + if (!stat_validity_check(r->parsed_objects->shallow_stat, + git_path_shallow(r))) die("shallow file has changed since we read it"); } -- 2.22.0.rc0.322.g2b0371e29a ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH 6/6] Use the right 'struct repository' instead of the_repository 2019-06-24 9:55 ` [PATCH 6/6] Use the right 'struct repository' instead of the_repository Nguyễn Thái Ngọc Duy @ 2019-06-24 14:24 ` Derrick Stolee 2019-06-24 14:45 ` Duy Nguyen 2019-06-27 9:06 ` Johannes Schindelin 1 sibling, 1 reply; 31+ messages in thread From: Derrick Stolee @ 2019-06-24 14:24 UTC (permalink / raw) To: Nguyễn Thái Ngọc Duy, git; +Cc: Junio C Hamano On 6/24/2019 5:55 AM, Nguyễn Thái Ngọc Duy wrote: > There are a couple of places where 'struct repository' is already passed > around, but the_repository is still used. Use the right repo. > > Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> nit: the subject line doesn't use the standard "area: topic" format (including the capitalization of the first word). Perhaps: treewide: use the right 'struct repository' instead of the_repository The changes here are straight-forward, but how do we check if we are done? Thanks, -Stolee ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 6/6] Use the right 'struct repository' instead of the_repository 2019-06-24 14:24 ` Derrick Stolee @ 2019-06-24 14:45 ` Duy Nguyen 0 siblings, 0 replies; 31+ messages in thread From: Duy Nguyen @ 2019-06-24 14:45 UTC (permalink / raw) To: Derrick Stolee; +Cc: Git Mailing List, Junio C Hamano On Mon, Jun 24, 2019 at 9:24 PM Derrick Stolee <stolee@gmail.com> wrote: > > On 6/24/2019 5:55 AM, Nguyễn Thái Ngọc Duy wrote: > > There are a couple of places where 'struct repository' is already passed > > around, but the_repository is still used. Use the right repo. > > > > Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> > > nit: the subject line doesn't use the standard "area: topic" format because there's no specific area to this patch. I don't think sticking to a fixed convention for the sake of it is really sensible. > (including > the capitalization of the first word). Perhaps: > > treewide: use the right 'struct repository' instead of the_repository > > The changes here are straight-forward, but how do we check if we are done? At this point, you can't. At some point we should be able to optionally disable the_repository, at least per file [2]. But even then some function calls inside could still hide the_repository and you would need something like [1] to reveal them. The problem with [2] is it will cause a lot of problems when adding new code until most of the code is converted. I will bring that up when the number of the_repository (outside builtin/) goes down below ~50. With all my patches, I think we're at 300. [1] https://gitlab.com/pclouds/git/commit/902a4dbdef6829ca06e12dbf74b0690456733351 [2] https://gitlab.com/pclouds/git/commit/f03f915294210baf038ee72d76ee998d9387028b -- Duy ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 6/6] Use the right 'struct repository' instead of the_repository 2019-06-24 9:55 ` [PATCH 6/6] Use the right 'struct repository' instead of the_repository Nguyễn Thái Ngọc Duy 2019-06-24 14:24 ` Derrick Stolee @ 2019-06-27 9:06 ` Johannes Schindelin 1 sibling, 0 replies; 31+ messages in thread From: Johannes Schindelin @ 2019-06-27 9:06 UTC (permalink / raw) To: Nguyễn Thái Ngọc Duy; +Cc: git, Junio C Hamano [-- Attachment #1: Type: text/plain, Size: 994 bytes --] Hi Duy, On Mon, 24 Jun 2019, Nguyễn Thái Ngọc Duy wrote: > There are a couple of places where 'struct repository' is already passed > around, but the_repository is still used. Use the right repo. This patch series breaks t7814 on Linux, macOS and Windows, with GCC and clang (read: _every_ single job in the CI except for documentation and coccinelle). For details, see https://dev.azure.com/gitgitgadget/git/_build/results?buildId=11536&view=ms.vss-test-web.build-test-results-tab The first test case to break is t7814.9 basic grep tree: -- snipsnap -- expecting success: cat >expect <<-\EOF && HEAD:a:(1|2)d(3|4) HEAD:b/b:(3|4) HEAD:submodule/a:(1|2)d(3|4) HEAD:submodule/sub/a:(1|2)d(3|4) EOF git grep -e "(3|4)" --recurse-submodules HEAD >actual && test_cmp expect actual ++ cat ++ git grep -e '(3|4)' --recurse-submodules HEAD fatal: unable to read tree (e6d32f554b2f8e48c3b8feece1653e933facb34a) error: last command exited with $?=128 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 0/6] Kill the_repository in tree-walk.c 2019-06-24 9:55 [PATCH 0/6] Kill the_repository in tree-walk.c Nguyễn Thái Ngọc Duy ` (5 preceding siblings ...) 2019-06-24 9:55 ` [PATCH 6/6] Use the right 'struct repository' instead of the_repository Nguyễn Thái Ngọc Duy @ 2019-06-26 17:20 ` Junio C Hamano 2019-06-27 13:04 ` Johannes Schindelin 2019-06-27 9:28 ` [PATCH v2 " Nguyễn Thái Ngọc Duy 7 siblings, 1 reply; 31+ messages in thread From: Junio C Hamano @ 2019-06-26 17:20 UTC (permalink / raw) To: Nguyễn Thái Ngọc Duy; +Cc: git Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes: > This is the continuation of nd/sha1-name-c-wo-the-repository. In that > series I sealed off one place in sha1-name.c that cannot walk trees > from arbitrary repositories. With tree-walk.c taking 'struct > repository *' directly, that check in there can now be removed. Thanks. With these queued on 'master', t7814 seems to become flaky (tried running it with --stress, with and without these patches). Are we touching a wrong index file in some codepaths or something? ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 0/6] Kill the_repository in tree-walk.c 2019-06-26 17:20 ` [PATCH 0/6] Kill the_repository in tree-walk.c Junio C Hamano @ 2019-06-27 13:04 ` Johannes Schindelin 2019-06-27 17:09 ` Derrick Stolee 0 siblings, 1 reply; 31+ messages in thread From: Johannes Schindelin @ 2019-06-27 13:04 UTC (permalink / raw) To: Junio C Hamano; +Cc: Nguyễn Thái Ngọc Duy, git [-- Attachment #1: Type: text/plain, Size: 1072 bytes --] Hi Junio, On Wed, 26 Jun 2019, Junio C Hamano wrote: > Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes: > > > This is the continuation of nd/sha1-name-c-wo-the-repository. In that > > series I sealed off one place in sha1-name.c that cannot walk trees > > from arbitrary repositories. With tree-walk.c taking 'struct > > repository *' directly, that check in there can now be removed. > > Thanks. > > With these queued on 'master', t7814 seems to become flaky (tried > running it with --stress, with and without these patches). Are we > touching a wrong index file in some codepaths or something? It's not flaky, as it fails consistently, and yes, we're touching the wrong repository in at least this one code path. I think I would have wished for a more careful conversion in this patch series, as it does touch critical code paths. Given that this bug was only caught by a failing CI build, it does make me wonder what other bugs are hidden and would slip into our code base just because of gaps in the code coverage. Ciao, Dscho ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 0/6] Kill the_repository in tree-walk.c 2019-06-27 13:04 ` Johannes Schindelin @ 2019-06-27 17:09 ` Derrick Stolee 0 siblings, 0 replies; 31+ messages in thread From: Derrick Stolee @ 2019-06-27 17:09 UTC (permalink / raw) To: Johannes Schindelin, Junio C Hamano Cc: Nguyễn Thái Ngọc Duy, git On 6/27/2019 9:04 AM, Johannes Schindelin wrote: > Given that this bug was only caught by a failing CI build, it does make me > wonder what other bugs are hidden and would slip into our code base just > because of gaps in the code coverage. Here are the lines introduced by this series that are not covered by the test suite. I'm not asking you to write tests to cover these lines, but please re-examine the lines to be sure the correct conversion was made. Thanks, -Stolee > Uncovered code in 'pu' not in 'jch' > -------------------------------------------------------- > > archive.c > 47f956bd 421) err = get_tree_entry(ar_args->repo, > 47f956bd 422) &tree->object.oid, > > fast-import.c > 35d7cdbe 2565) char *buf = read_object_with_reference(the_repository, > 35d7cdbe 2566) &n->oid, > > match-trees.c > 3fe87a7f 294) if (get_tree_entry(r, hash2, del_prefix, shifted, &mode)) > > t/helper/test-match-trees.c > 3fe87a7f 23) shift_tree(the_repository, &one->object.oid, &two->object.oid, &shifted, -1); > > Nguyễn Thái Ngọc Duy 35d7cdbe sha1-file.c: remove the_repo from read_object_with_reference() > Nguyễn Thái Ngọc Duy 3fe87a7f match-trees.c: remove the_repo from shift_tree*() > Nguyễn Thái Ngọc Duy 47f956bd tree-walk.c: remove the_repo from get_tree_entry() ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v2 0/6] Kill the_repository in tree-walk.c 2019-06-24 9:55 [PATCH 0/6] Kill the_repository in tree-walk.c Nguyễn Thái Ngọc Duy ` (6 preceding siblings ...) 2019-06-26 17:20 ` [PATCH 0/6] Kill the_repository in tree-walk.c Junio C Hamano @ 2019-06-27 9:28 ` Nguyễn Thái Ngọc Duy 2019-06-27 9:28 ` [PATCH v2 1/6] sha1-file.c: remove the_repo from read_object_with_reference() Nguyễn Thái Ngọc Duy ` (7 more replies) 7 siblings, 8 replies; 31+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2019-06-27 9:28 UTC (permalink / raw) To: pclouds; +Cc: git, gitster, Derrick Stolee, Johannes Schindelin v2 fixes t7814 flakiness. The problem is git-grep can operate on multiple repos and I read objects from the wrong repo (supermodule) instead of the submodule one. There are still the_repository hidden in git-grep code paths, and the hack of asborbing submodule's object db to the_repo's in order to make it work. And I can't quite understand how t7814 sometimes passed. I'll revisit this after this series is done and will try to get rid of add_to_alternates_memory() in git-grep. Nguyễn Thái Ngọc Duy (6): sha1-file.c: remove the_repo from read_object_with_reference() tree-walk.c: remove the_repo from fill_tree_descriptor() tree-walk.c: remove the_repo from get_tree_entry() tree-walk.c: remove the_repo from get_tree_entry_follow_symlinks() match-trees.c: remove the_repo from shift_tree*() Use the right 'struct repository' instead of the_repository archive.c | 4 +++- blame.c | 4 ++-- builtin/cat-file.c | 3 ++- builtin/grep.c | 6 ++++-- builtin/merge-tree.c | 22 +++++++++++-------- builtin/pack-objects.c | 3 ++- builtin/rebase.c | 4 ++-- builtin/reset.c | 4 ++-- builtin/rm.c | 2 +- builtin/update-index.c | 2 +- cache.h | 7 +++--- fast-import.c | 9 +++++--- line-log.c | 7 +++--- match-trees.c | 12 ++++++----- merge-recursive.c | 43 +++++++++++++++++++++---------------- notes.c | 4 ++-- sequencer.c | 6 +++--- sha1-file.c | 5 +++-- sha1-name.c | 25 +++++++-------------- shallow.c | 3 ++- t/helper/test-match-trees.c | 2 +- tree-diff.c | 4 ++-- tree-walk.c | 35 ++++++++++++++++++++---------- tree-walk.h | 8 ++++--- unpack-trees.c | 2 +- 25 files changed, 129 insertions(+), 97 deletions(-) Range-diff dựa trên v1: 1: 35d7cdbe6a ! 1: 9e73c39f9a sha1-file.c: remove the_repo from read_object_with_reference() @@ -3,7 +3,6 @@ sha1-file.c: remove the_repo from read_object_with_reference() Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> - Signed-off-by: Junio C Hamano <gitster@pobox.com> diff --git a/builtin/cat-file.c b/builtin/cat-file.c --- a/builtin/cat-file.c @@ -27,7 +26,7 @@ grep_read_lock(); - data = read_object_with_reference(&object->oid, tree_type, -+ data = read_object_with_reference(opt->repo, ++ data = read_object_with_reference(&subrepo, + &object->oid, tree_type, &size, NULL); grep_read_unlock(); 2: 4ff146fb64 = 2: b9107f7503 tree-walk.c: remove the_repo from fill_tree_descriptor() 3: 47f956bd0f = 3: 87ed67bde5 tree-walk.c: remove the_repo from get_tree_entry() 4: e19c4b9ce6 = 4: 557b61f2ba tree-walk.c: remove the_repo from get_tree_entry_follow_symlinks() 5: 3fe87a7fde = 5: 53f09e0437 match-trees.c: remove the_repo from shift_tree*() 6: 6d0449f1a7 = 6: d5d4d2ba65 Use the right 'struct repository' instead of the_repository -- 2.22.0.rc0.322.g2b0371e29a ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v2 1/6] sha1-file.c: remove the_repo from read_object_with_reference() 2019-06-27 9:28 ` [PATCH v2 " Nguyễn Thái Ngọc Duy @ 2019-06-27 9:28 ` Nguyễn Thái Ngọc Duy 2019-06-28 12:46 ` Johannes Schindelin 2019-06-27 9:28 ` [PATCH v2 2/6] tree-walk.c: remove the_repo from fill_tree_descriptor() Nguyễn Thái Ngọc Duy ` (6 subsequent siblings) 7 siblings, 1 reply; 31+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2019-06-27 9:28 UTC (permalink / raw) To: pclouds; +Cc: git, gitster, Derrick Stolee, Johannes Schindelin Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> --- builtin/cat-file.c | 3 ++- builtin/grep.c | 6 ++++-- builtin/pack-objects.c | 3 ++- cache.h | 3 ++- fast-import.c | 9 ++++++--- sha1-file.c | 5 +++-- tree-walk.c | 7 ++++--- 7 files changed, 23 insertions(+), 13 deletions(-) diff --git a/builtin/cat-file.c b/builtin/cat-file.c index 0f092382e1..995d47c85a 100644 --- a/builtin/cat-file.c +++ b/builtin/cat-file.c @@ -172,7 +172,8 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name, * fall-back to the usual case. */ } - buf = read_object_with_reference(&oid, exp_type, &size, NULL); + buf = read_object_with_reference(the_repository, + &oid, exp_type, &size, NULL); break; default: diff --git a/builtin/grep.c b/builtin/grep.c index 580fd38f41..560051784e 100644 --- a/builtin/grep.c +++ b/builtin/grep.c @@ -458,7 +458,8 @@ static int grep_submodule(struct grep_opt *opt, object = parse_object_or_die(oid, oid_to_hex(oid)); grep_read_lock(); - data = read_object_with_reference(&object->oid, tree_type, + data = read_object_with_reference(&subrepo, + &object->oid, tree_type, &size, NULL); grep_read_unlock(); @@ -623,7 +624,8 @@ static int grep_object(struct grep_opt *opt, const struct pathspec *pathspec, int hit, len; grep_read_lock(); - data = read_object_with_reference(&obj->oid, tree_type, + data = read_object_with_reference(opt->repo, + &obj->oid, tree_type, &size, NULL); grep_read_unlock(); diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index b2be8869c2..a030c24a4a 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -1428,7 +1428,8 @@ static void add_preferred_base(struct object_id *oid) if (window <= num_preferred_base++) return; - data = read_object_with_reference(oid, tree_type, &size, &tree_oid); + data = read_object_with_reference(the_repository, oid, + tree_type, &size, &tree_oid); if (!data) return; diff --git a/cache.h b/cache.h index bf20337ef4..cd84cc9bbe 100644 --- a/cache.h +++ b/cache.h @@ -1500,7 +1500,8 @@ int df_name_compare(const char *name1, int len1, int mode1, const char *name2, i int name_compare(const char *name1, size_t len1, const char *name2, size_t len2); int cache_name_stage_compare(const char *name1, int len1, int stage1, const char *name2, int len2, int stage2); -void *read_object_with_reference(const struct object_id *oid, +void *read_object_with_reference(struct repository *r, + const struct object_id *oid, const char *required_type, unsigned long *size, struct object_id *oid_ret); diff --git a/fast-import.c b/fast-import.c index 76a7bd3699..3970b50acc 100644 --- a/fast-import.c +++ b/fast-import.c @@ -2410,7 +2410,8 @@ static void note_change_n(const char *p, struct branch *b, unsigned char *old_fa oidcpy(&commit_oid, &commit_oe->idx.oid); } else if (!get_oid(p, &commit_oid)) { unsigned long size; - char *buf = read_object_with_reference(&commit_oid, + char *buf = read_object_with_reference(the_repository, + &commit_oid, commit_type, &size, &commit_oid); if (!buf || size < the_hash_algo->hexsz + 6) @@ -2482,7 +2483,8 @@ static void parse_from_existing(struct branch *b) unsigned long size; char *buf; - buf = read_object_with_reference(&b->oid, commit_type, &size, + buf = read_object_with_reference(the_repository, + &b->oid, commit_type, &size, &b->oid); parse_from_commit(b, buf, size); free(buf); @@ -2560,7 +2562,8 @@ static struct hash_list *parse_merge(unsigned int *count) oidcpy(&n->oid, &oe->idx.oid); } else if (!get_oid(from, &n->oid)) { unsigned long size; - char *buf = read_object_with_reference(&n->oid, + char *buf = read_object_with_reference(the_repository, + &n->oid, commit_type, &size, &n->oid); if (!buf || size < the_hash_algo->hexsz + 6) diff --git a/sha1-file.c b/sha1-file.c index 888b6024d5..59b2e40cf3 100644 --- a/sha1-file.c +++ b/sha1-file.c @@ -1505,7 +1505,8 @@ void *read_object_file_extended(struct repository *r, return NULL; } -void *read_object_with_reference(const struct object_id *oid, +void *read_object_with_reference(struct repository *r, + const struct object_id *oid, const char *required_type_name, unsigned long *size, struct object_id *actual_oid_return) @@ -1521,7 +1522,7 @@ void *read_object_with_reference(const struct object_id *oid, int ref_length = -1; const char *ref_type = NULL; - buffer = read_object_file(&actual_oid, &type, &isize); + buffer = repo_read_object_file(r, &actual_oid, &type, &isize); if (!buffer) return NULL; if (type == required_type) { diff --git a/tree-walk.c b/tree-walk.c index ec32a47b2e..0c7722b220 100644 --- a/tree-walk.c +++ b/tree-walk.c @@ -87,7 +87,7 @@ void *fill_tree_descriptor(struct tree_desc *desc, const struct object_id *oid) void *buf = NULL; if (oid) { - buf = read_object_with_reference(oid, tree_type, &size, NULL); + buf = read_object_with_reference(the_repository, oid, tree_type, &size, NULL); if (!buf) die("unable to read tree %s", oid_to_hex(oid)); } @@ -542,7 +542,7 @@ int get_tree_entry(const struct object_id *tree_oid, const char *name, struct ob unsigned long size; struct object_id root; - tree = read_object_with_reference(tree_oid, tree_type, &size, &root); + tree = read_object_with_reference(the_repository, tree_oid, tree_type, &size, &root); if (!tree) return -1; @@ -609,7 +609,8 @@ enum get_oid_result get_tree_entry_follow_symlinks(struct object_id *tree_oid, c void *tree; struct object_id root; unsigned long size; - tree = read_object_with_reference(¤t_tree_oid, + tree = read_object_with_reference(the_repository, + ¤t_tree_oid, tree_type, &size, &root); if (!tree) -- 2.22.0.rc0.322.g2b0371e29a ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH v2 1/6] sha1-file.c: remove the_repo from read_object_with_reference() 2019-06-27 9:28 ` [PATCH v2 1/6] sha1-file.c: remove the_repo from read_object_with_reference() Nguyễn Thái Ngọc Duy @ 2019-06-28 12:46 ` Johannes Schindelin 0 siblings, 0 replies; 31+ messages in thread From: Johannes Schindelin @ 2019-06-28 12:46 UTC (permalink / raw) To: Nguyễn Thái Ngọc Duy; +Cc: git, gitster, Derrick Stolee [-- Attachment #1: Type: text/plain, Size: 1818 bytes --] Hi Duy, On Thu, 27 Jun 2019, Nguyễn Thái Ngọc Duy wrote: > Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> > --- This commit is _awfully_ short given that... > diff --git a/builtin/grep.c b/builtin/grep.c > index 580fd38f41..560051784e 100644 > --- a/builtin/grep.c > +++ b/builtin/grep.c > @@ -458,7 +458,8 @@ static int grep_submodule(struct grep_opt *opt, > object = parse_object_or_die(oid, oid_to_hex(oid)); > > grep_read_lock(); > - data = read_object_with_reference(&object->oid, tree_type, > + data = read_object_with_reference(&subrepo, > + &object->oid, tree_type, ... this change and... > &size, NULL); > grep_read_unlock(); > > @@ -623,7 +624,8 @@ static int grep_object(struct grep_opt *opt, const struct pathspec *pathspec, > int hit, len; > > grep_read_lock(); > - data = read_object_with_reference(&obj->oid, tree_type, > + data = read_object_with_reference(opt->repo, > + &obj->oid, tree_type, ... this change is totally not what would be intuitively the easiest: to use `the_repository` in all built-ins. It might take quite a lot of convincing that these changes are correct, in particular in light of the regressions introduced by the first iteration (to paraphrase Warren Buffet [*1*]: one slip in a patch series touching as central parts as this one will need a lot of time to restore trust in subsequent iterations' correctness.) In short: with such an empty commit message, this patch is no good. It's as if it was optimized to pass the test suite on Linux instead of a best effort to make the conversion as correct as you can make it. Ciao, Johannes Footnote *1*: https://www.forbes.com/sites/jamesberman/2014/04/20/the-three-essential-warren-buffett-quotes-to-live-by/ ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v2 2/6] tree-walk.c: remove the_repo from fill_tree_descriptor() 2019-06-27 9:28 ` [PATCH v2 " Nguyễn Thái Ngọc Duy 2019-06-27 9:28 ` [PATCH v2 1/6] sha1-file.c: remove the_repo from read_object_with_reference() Nguyễn Thái Ngọc Duy @ 2019-06-27 9:28 ` Nguyễn Thái Ngọc Duy 2019-06-27 9:28 ` [PATCH v2 3/6] tree-walk.c: remove the_repo from get_tree_entry() Nguyễn Thái Ngọc Duy ` (5 subsequent siblings) 7 siblings, 0 replies; 31+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2019-06-27 9:28 UTC (permalink / raw) To: pclouds; +Cc: git, gitster, Derrick Stolee, Johannes Schindelin While at there, clean up the_repo usage in builtin/merge-tree.c a tiny bit. Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> --- builtin/merge-tree.c | 22 +++++++++++++--------- builtin/rebase.c | 4 ++-- builtin/reset.c | 4 ++-- notes.c | 2 +- sequencer.c | 2 +- tree-diff.c | 4 ++-- tree-walk.c | 6 ++++-- tree-walk.h | 4 +++- unpack-trees.c | 2 +- 9 files changed, 29 insertions(+), 21 deletions(-) diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c index 34ca0258b1..97b54caeb9 100644 --- a/builtin/merge-tree.c +++ b/builtin/merge-tree.c @@ -205,6 +205,7 @@ static void resolve(const struct traverse_info *info, struct name_entry *ours, s static void unresolved_directory(const struct traverse_info *info, struct name_entry n[3]) { + struct repository *r = the_repository; char *newbase; struct name_entry *p; struct tree_desc t[3]; @@ -220,9 +221,9 @@ static void unresolved_directory(const struct traverse_info *info, newbase = traverse_path(info, p); #define ENTRY_OID(e) (((e)->mode && S_ISDIR((e)->mode)) ? &(e)->oid : NULL) - buf0 = fill_tree_descriptor(t + 0, ENTRY_OID(n + 0)); - buf1 = fill_tree_descriptor(t + 1, ENTRY_OID(n + 1)); - buf2 = fill_tree_descriptor(t + 2, ENTRY_OID(n + 2)); + buf0 = fill_tree_descriptor(r, t + 0, ENTRY_OID(n + 0)); + buf1 = fill_tree_descriptor(r, t + 1, ENTRY_OID(n + 1)); + buf2 = fill_tree_descriptor(r, t + 2, ENTRY_OID(n + 2)); #undef ENTRY_OID merge_trees(t, newbase); @@ -351,14 +352,16 @@ static void merge_trees(struct tree_desc t[3], const char *base) traverse_trees(&the_index, 3, t, &info); } -static void *get_tree_descriptor(struct tree_desc *desc, const char *rev) +static void *get_tree_descriptor(struct repository *r, + struct tree_desc *desc, + const char *rev) { struct object_id oid; void *buf; - if (get_oid(rev, &oid)) + if (repo_get_oid(r, rev, &oid)) die("unknown rev %s", rev); - buf = fill_tree_descriptor(desc, &oid); + buf = fill_tree_descriptor(r, desc, &oid); if (!buf) die("%s is not a tree", rev); return buf; @@ -366,15 +369,16 @@ static void *get_tree_descriptor(struct tree_desc *desc, const char *rev) int cmd_merge_tree(int argc, const char **argv, const char *prefix) { + struct repository *r = the_repository; struct tree_desc t[3]; void *buf1, *buf2, *buf3; if (argc != 4) usage(merge_tree_usage); - buf1 = get_tree_descriptor(t+0, argv[1]); - buf2 = get_tree_descriptor(t+1, argv[2]); - buf3 = get_tree_descriptor(t+2, argv[3]); + buf1 = get_tree_descriptor(r, t+0, argv[1]); + buf2 = get_tree_descriptor(r, t+1, argv[2]); + buf3 = get_tree_descriptor(r, t+2, argv[3]); merge_trees(t, ""); free(buf1); free(buf2); diff --git a/builtin/rebase.c b/builtin/rebase.c index b8116db487..28490f5f88 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -840,13 +840,13 @@ static int reset_head(struct object_id *oid, const char *action, goto leave_reset_head; } - if (!reset_hard && !fill_tree_descriptor(&desc[nr++], &head_oid)) { + if (!reset_hard && !fill_tree_descriptor(the_repository, &desc[nr++], &head_oid)) { ret = error(_("failed to find tree of %s"), oid_to_hex(&head_oid)); goto leave_reset_head; } - if (!fill_tree_descriptor(&desc[nr++], oid)) { + if (!fill_tree_descriptor(the_repository, &desc[nr++], oid)) { ret = error(_("failed to find tree of %s"), oid_to_hex(oid)); goto leave_reset_head; } diff --git a/builtin/reset.c b/builtin/reset.c index 26ef9a7bd0..77c38f28c2 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -79,13 +79,13 @@ static int reset_index(const struct object_id *oid, int reset_type, int quiet) struct object_id head_oid; if (get_oid("HEAD", &head_oid)) return error(_("You do not have a valid HEAD.")); - if (!fill_tree_descriptor(desc + nr, &head_oid)) + if (!fill_tree_descriptor(the_repository, desc + nr, &head_oid)) return error(_("Failed to find tree of HEAD.")); nr++; opts.fn = twoway_merge; } - if (!fill_tree_descriptor(desc + nr, oid)) { + if (!fill_tree_descriptor(the_repository, desc + nr, oid)) { error(_("Failed to find tree of %s."), oid_to_hex(oid)); goto out; } diff --git a/notes.c b/notes.c index 532ec37865..2522b87d77 100644 --- a/notes.c +++ b/notes.c @@ -397,7 +397,7 @@ static void load_subtree(struct notes_tree *t, struct leaf_node *subtree, struct name_entry entry; const unsigned hashsz = the_hash_algo->rawsz; - buf = fill_tree_descriptor(&desc, &subtree->val_oid); + buf = fill_tree_descriptor(the_repository, &desc, &subtree->val_oid); if (!buf) die("Could not read %s for notes-index", oid_to_hex(&subtree->val_oid)); diff --git a/sequencer.c b/sequencer.c index ab74b6baf1..d565fcf2b1 100644 --- a/sequencer.c +++ b/sequencer.c @@ -3194,7 +3194,7 @@ static int do_reset(struct repository *r, return error_resolve_conflict(_(action_name(opts))); } - if (!fill_tree_descriptor(&desc, &oid)) { + if (!fill_tree_descriptor(r, &desc, &oid)) { error(_("failed to find tree of %s"), oid_to_hex(&oid)); rollback_lock_file(&lock); free((void *)desc.buffer); diff --git a/tree-diff.c b/tree-diff.c index f1f641eb6a..33ded7f8b3 100644 --- a/tree-diff.c +++ b/tree-diff.c @@ -422,8 +422,8 @@ static struct combine_diff_path *ll_diff_tree_paths( * diff_tree_oid(parent, commit) ) */ for (i = 0; i < nparent; ++i) - tptree[i] = fill_tree_descriptor(&tp[i], parents_oid[i]); - ttree = fill_tree_descriptor(&t, oid); + tptree[i] = fill_tree_descriptor(opt->repo, &tp[i], parents_oid[i]); + ttree = fill_tree_descriptor(opt->repo, &t, oid); /* Enable recursion indefinitely */ opt->pathspec.recursive = opt->flags.recursive; diff --git a/tree-walk.c b/tree-walk.c index 0c7722b220..c5569b3e9f 100644 --- a/tree-walk.c +++ b/tree-walk.c @@ -81,13 +81,15 @@ int init_tree_desc_gently(struct tree_desc *desc, const void *buffer, unsigned l return result; } -void *fill_tree_descriptor(struct tree_desc *desc, const struct object_id *oid) +void *fill_tree_descriptor(struct repository *r, + struct tree_desc *desc, + const struct object_id *oid) { unsigned long size = 0; void *buf = NULL; if (oid) { - buf = read_object_with_reference(the_repository, oid, tree_type, &size, NULL); + buf = read_object_with_reference(r, oid, tree_type, &size, NULL); if (!buf) die("unable to read tree %s", oid_to_hex(oid)); } diff --git a/tree-walk.h b/tree-walk.h index 161e2400f4..9aa1042642 100644 --- a/tree-walk.h +++ b/tree-walk.h @@ -45,7 +45,9 @@ int init_tree_desc_gently(struct tree_desc *desc, const void *buf, unsigned long int tree_entry(struct tree_desc *, struct name_entry *); int tree_entry_gently(struct tree_desc *, struct name_entry *); -void *fill_tree_descriptor(struct tree_desc *desc, const struct object_id *oid); +void *fill_tree_descriptor(struct repository *r, + struct tree_desc *desc, + const struct object_id *oid); struct traverse_info; typedef int (*traverse_callback_t)(int n, unsigned long mask, unsigned long dirmask, struct name_entry *entry, struct traverse_info *); diff --git a/unpack-trees.c b/unpack-trees.c index 50189909b8..cfe1c5ec6f 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -840,7 +840,7 @@ static int traverse_trees_recursive(int n, unsigned long dirmask, const struct object_id *oid = NULL; if (dirmask & 1) oid = &names[i].oid; - buf[nr_buf++] = fill_tree_descriptor(t + i, oid); + buf[nr_buf++] = fill_tree_descriptor(the_repository, t + i, oid); } } -- 2.22.0.rc0.322.g2b0371e29a ^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v2 3/6] tree-walk.c: remove the_repo from get_tree_entry() 2019-06-27 9:28 ` [PATCH v2 " Nguyễn Thái Ngọc Duy 2019-06-27 9:28 ` [PATCH v2 1/6] sha1-file.c: remove the_repo from read_object_with_reference() Nguyễn Thái Ngọc Duy 2019-06-27 9:28 ` [PATCH v2 2/6] tree-walk.c: remove the_repo from fill_tree_descriptor() Nguyễn Thái Ngọc Duy @ 2019-06-27 9:28 ` Nguyễn Thái Ngọc Duy 2019-06-27 9:28 ` [PATCH v2 4/6] tree-walk.c: remove the_repo from get_tree_entry_follow_symlinks() Nguyễn Thái Ngọc Duy ` (4 subsequent siblings) 7 siblings, 0 replies; 31+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2019-06-27 9:28 UTC (permalink / raw) To: pclouds; +Cc: git, gitster, Derrick Stolee, Johannes Schindelin Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> --- archive.c | 4 +++- blame.c | 4 ++-- builtin/rm.c | 2 +- builtin/update-index.c | 2 +- line-log.c | 7 ++++--- match-trees.c | 6 +++--- merge-recursive.c | 8 +++++--- notes.c | 2 +- sha1-name.c | 9 +++++---- tree-walk.c | 18 ++++++++++++------ tree-walk.h | 2 +- 11 files changed, 38 insertions(+), 26 deletions(-) diff --git a/archive.c b/archive.c index 53141c1f0e..a8da0fcc4f 100644 --- a/archive.c +++ b/archive.c @@ -418,7 +418,9 @@ static void parse_treeish_arg(const char **argv, unsigned short mode; int err; - err = get_tree_entry(&tree->object.oid, prefix, &tree_oid, + err = get_tree_entry(ar_args->repo, + &tree->object.oid, + prefix, &tree_oid, &mode); if (err || !S_ISDIR(mode)) die(_("current working directory is untracked")); diff --git a/blame.c b/blame.c index 145eaf2faf..ef022809e9 100644 --- a/blame.c +++ b/blame.c @@ -101,7 +101,7 @@ static void verify_working_tree_path(struct repository *r, struct object_id blob_oid; unsigned short mode; - if (!get_tree_entry(commit_oid, path, &blob_oid, &mode) && + if (!get_tree_entry(r, commit_oid, path, &blob_oid, &mode) && oid_object_info(r, &blob_oid, NULL) == OBJ_BLOB) return; } @@ -532,7 +532,7 @@ static int fill_blob_sha1_and_mode(struct repository *r, { if (!is_null_oid(&origin->blob_oid)) return 0; - if (get_tree_entry(&origin->commit->object.oid, origin->path, &origin->blob_oid, &origin->mode)) + if (get_tree_entry(r, &origin->commit->object.oid, origin->path, &origin->blob_oid, &origin->mode)) goto error_out; if (oid_object_info(r, &origin->blob_oid, NULL) != OBJ_BLOB) goto error_out; diff --git a/builtin/rm.c b/builtin/rm.c index be8edc6d1e..2eacda42b4 100644 --- a/builtin/rm.c +++ b/builtin/rm.c @@ -179,7 +179,7 @@ static int check_local_mod(struct object_id *head, int index_only) * way as changed from the HEAD. */ if (no_head - || get_tree_entry(head, name, &oid, &mode) + || get_tree_entry(the_repository, head, name, &oid, &mode) || ce->ce_mode != create_ce_mode(mode) || !oideq(&ce->oid, &oid)) staged_changes = 1; diff --git a/builtin/update-index.c b/builtin/update-index.c index 3f8cc6ccb4..dff2f4b837 100644 --- a/builtin/update-index.c +++ b/builtin/update-index.c @@ -601,7 +601,7 @@ static struct cache_entry *read_one_ent(const char *which, struct object_id oid; struct cache_entry *ce; - if (get_tree_entry(ent, path, &oid, &mode)) { + if (get_tree_entry(the_repository, ent, path, &oid, &mode)) { if (which) error("%s: not in %s branch.", path, which); return NULL; diff --git a/line-log.c b/line-log.c index 0a17b21187..3aff1849e7 100644 --- a/line-log.c +++ b/line-log.c @@ -496,12 +496,13 @@ static struct commit *check_single_commit(struct rev_info *revs) return (struct commit *) commit; } -static void fill_blob_sha1(struct commit *commit, struct diff_filespec *spec) +static void fill_blob_sha1(struct repository *r, struct commit *commit, + struct diff_filespec *spec) { unsigned short mode; struct object_id oid; - if (get_tree_entry(&commit->object.oid, spec->path, &oid, &mode)) + if (get_tree_entry(r, &commit->object.oid, spec->path, &oid, &mode)) die("There is no path %s in the commit", spec->path); fill_filespec(spec, &oid, 1, mode); @@ -585,7 +586,7 @@ parse_lines(struct repository *r, struct commit *commit, name_part); spec = alloc_filespec(full_name); - fill_blob_sha1(commit, spec); + fill_blob_sha1(r, commit, spec); fill_line_ends(r, spec, &lines, &ends); cb_data.spec = spec; cb_data.lines = lines; diff --git a/match-trees.c b/match-trees.c index 9d1ec8d6b0..de7e8a6783 100644 --- a/match-trees.c +++ b/match-trees.c @@ -290,7 +290,7 @@ void shift_tree(const struct object_id *hash1, if (!*del_prefix) return; - if (get_tree_entry(hash2, del_prefix, shifted, &mode)) + if (get_tree_entry(the_repository, hash2, del_prefix, shifted, &mode)) die("cannot find path %s in tree %s", del_prefix, oid_to_hex(hash2)); return; @@ -317,12 +317,12 @@ void shift_tree_by(const struct object_id *hash1, unsigned candidate = 0; /* Can hash2 be a tree at shift_prefix in tree hash1? */ - if (!get_tree_entry(hash1, shift_prefix, &sub1, &mode1) && + if (!get_tree_entry(the_repository, hash1, shift_prefix, &sub1, &mode1) && S_ISDIR(mode1)) candidate |= 1; /* Can hash1 be a tree at shift_prefix in tree hash2? */ - if (!get_tree_entry(hash2, shift_prefix, &sub2, &mode2) && + if (!get_tree_entry(the_repository, hash2, shift_prefix, &sub2, &mode2) && S_ISDIR(mode2)) candidate |= 2; diff --git a/merge-recursive.c b/merge-recursive.c index d2e380b7ed..b051066795 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -475,7 +475,7 @@ static int get_tree_entry_if_blob(const struct object_id *tree, { int ret; - ret = get_tree_entry(tree, path, &dfs->oid, &dfs->mode); + ret = get_tree_entry(the_repository, tree, path, &dfs->oid, &dfs->mode); if (S_ISDIR(dfs->mode)) { oidcpy(&dfs->oid, &null_oid); dfs->mode = 0; @@ -1905,7 +1905,8 @@ static int tree_has_path(struct tree *tree, const char *path) struct object_id hashy; unsigned short mode_o; - return !get_tree_entry(&tree->object.oid, path, + return !get_tree_entry(the_repository, + &tree->object.oid, path, &hashy, &mode_o); } @@ -2500,7 +2501,8 @@ static void apply_directory_rename_modifications(struct merge_options *opt, * the various handle_rename_*() functions update the index * explicitly rather than relying on unpack_trees() to have done it. */ - get_tree_entry(&tree->object.oid, + get_tree_entry(opt->repo, + &tree->object.oid, pair->two->path, &re->dst_entry->stages[stage].oid, &re->dst_entry->stages[stage].mode); diff --git a/notes.c b/notes.c index 2522b87d77..75c028b300 100644 --- a/notes.c +++ b/notes.c @@ -1015,7 +1015,7 @@ void init_notes(struct notes_tree *t, const char *notes_ref, return; if (flags & NOTES_INIT_WRITABLE && read_ref(notes_ref, &object_oid)) die("Cannot use notes ref %s", notes_ref); - if (get_tree_entry(&object_oid, "", &oid, &mode)) + if (get_tree_entry(the_repository, &object_oid, "", &oid, &mode)) die("Failed to read notes tree referenced by %s (%s)", notes_ref, oid_to_hex(&object_oid)); diff --git a/sha1-name.c b/sha1-name.c index 728e6f1f61..e8fb215e5c 100644 --- a/sha1-name.c +++ b/sha1-name.c @@ -1677,7 +1677,8 @@ int repo_get_oid_blob(struct repository *r, } /* Must be called only when object_name:filename doesn't exist. */ -static void diagnose_invalid_oid_path(const char *prefix, +static void diagnose_invalid_oid_path(struct repository *r, + const char *prefix, const char *filename, const struct object_id *tree_oid, const char *object_name, @@ -1695,7 +1696,7 @@ static void diagnose_invalid_oid_path(const char *prefix, if (is_missing_file_error(errno)) { char *fullname = xstrfmt("%s%s", prefix, filename); - if (!get_tree_entry(tree_oid, fullname, &oid, &mode)) { + if (!get_tree_entry(r, tree_oid, fullname, &oid, &mode)) { die("Path '%s' exists, but not '%s'.\n" "Did you mean '%.*s:%s' aka '%.*s:./%s'?", fullname, @@ -1902,10 +1903,10 @@ static enum get_oid_result get_oid_with_context_1(struct repository *repo, filename, oid, &oc->symlink_path, &oc->mode); } else { - ret = get_tree_entry(&tree_oid, filename, oid, + ret = get_tree_entry(repo, &tree_oid, filename, oid, &oc->mode); if (ret && only_to_die) { - diagnose_invalid_oid_path(prefix, + diagnose_invalid_oid_path(repo, prefix, filename, &tree_oid, name, len); diff --git a/tree-walk.c b/tree-walk.c index c5569b3e9f..506e12a031 100644 --- a/tree-walk.c +++ b/tree-walk.c @@ -502,7 +502,9 @@ struct dir_state { struct object_id oid; }; -static int find_tree_entry(struct tree_desc *t, const char *name, struct object_id *result, unsigned short *mode) +static int find_tree_entry(struct repository *r, struct tree_desc *t, + const char *name, struct object_id *result, + unsigned short *mode) { int namelen = strlen(name); while (t->size) { @@ -532,19 +534,23 @@ static int find_tree_entry(struct tree_desc *t, const char *name, struct object_ oidcpy(result, &oid); return 0; } - return get_tree_entry(&oid, name + entrylen, result, mode); + return get_tree_entry(r, &oid, name + entrylen, result, mode); } return -1; } -int get_tree_entry(const struct object_id *tree_oid, const char *name, struct object_id *oid, unsigned short *mode) +int get_tree_entry(struct repository *r, + const struct object_id *tree_oid, + const char *name, + struct object_id *oid, + unsigned short *mode) { int retval; void *tree; unsigned long size; struct object_id root; - tree = read_object_with_reference(the_repository, tree_oid, tree_type, &size, &root); + tree = read_object_with_reference(r, tree_oid, tree_type, &size, &root); if (!tree) return -1; @@ -559,7 +565,7 @@ int get_tree_entry(const struct object_id *tree_oid, const char *name, struct ob } else { struct tree_desc t; init_tree_desc(&t, tree, size); - retval = find_tree_entry(&t, name, oid, mode); + retval = find_tree_entry(r, &t, name, oid, mode); } free(tree); return retval; @@ -681,7 +687,7 @@ enum get_oid_result get_tree_entry_follow_symlinks(struct object_id *tree_oid, c } /* Look up the first (or only) path component in the tree. */ - find_result = find_tree_entry(&t, namebuf.buf, + find_result = find_tree_entry(the_repository, &t, namebuf.buf, ¤t_tree_oid, mode); if (find_result) { goto done; diff --git a/tree-walk.h b/tree-walk.h index 9aa1042642..639f79187f 100644 --- a/tree-walk.h +++ b/tree-walk.h @@ -68,7 +68,7 @@ struct traverse_info { int show_all_errors; }; -int get_tree_entry(const struct object_id *, const char *, struct object_id *, unsigned short *); +int get_tree_entry(struct repository *, const struct object_id *, const char *, struct object_id *, unsigned short *); char *make_traverse_path(char *path, const struct traverse_info *info, const struct name_entry *n); void setup_traverse_info(struct traverse_info *info, const char *base); -- 2.22.0.rc0.322.g2b0371e29a ^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v2 4/6] tree-walk.c: remove the_repo from get_tree_entry_follow_symlinks() 2019-06-27 9:28 ` [PATCH v2 " Nguyễn Thái Ngọc Duy ` (2 preceding siblings ...) 2019-06-27 9:28 ` [PATCH v2 3/6] tree-walk.c: remove the_repo from get_tree_entry() Nguyễn Thái Ngọc Duy @ 2019-06-27 9:28 ` Nguyễn Thái Ngọc Duy 2019-06-27 9:28 ` [PATCH v2 5/6] match-trees.c: remove the_repo from shift_tree*() Nguyễn Thái Ngọc Duy ` (3 subsequent siblings) 7 siblings, 0 replies; 31+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2019-06-27 9:28 UTC (permalink / raw) To: pclouds; +Cc: git, gitster, Derrick Stolee, Johannes Schindelin Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> --- sha1-name.c | 10 +--------- tree-walk.c | 12 ++++++++---- tree-walk.h | 2 +- 3 files changed, 10 insertions(+), 14 deletions(-) diff --git a/sha1-name.c b/sha1-name.c index e8fb215e5c..3c9fa10af8 100644 --- a/sha1-name.c +++ b/sha1-name.c @@ -1890,16 +1890,8 @@ static enum get_oid_result get_oid_with_context_1(struct repository *repo, new_filename = resolve_relative_path(repo, filename); if (new_filename) filename = new_filename; - /* - * NEEDSWORK: Eventually get_tree_entry*() should - * learn to take struct repository directly and we - * would not need to inject submodule odb to the - * in-core odb. - */ - if (repo != the_repository) - add_to_alternates_memory(repo->objects->odb->path); if (flags & GET_OID_FOLLOW_SYMLINKS) { - ret = get_tree_entry_follow_symlinks(&tree_oid, + ret = get_tree_entry_follow_symlinks(repo, &tree_oid, filename, oid, &oc->symlink_path, &oc->mode); } else { diff --git a/tree-walk.c b/tree-walk.c index 506e12a031..c20b62f49e 100644 --- a/tree-walk.c +++ b/tree-walk.c @@ -593,7 +593,10 @@ int get_tree_entry(struct repository *r, * See the code for enum get_oid_result for a description of * the return values. */ -enum get_oid_result get_tree_entry_follow_symlinks(struct object_id *tree_oid, const char *name, struct object_id *result, struct strbuf *result_path, unsigned short *mode) +enum get_oid_result get_tree_entry_follow_symlinks(struct repository *r, + struct object_id *tree_oid, const char *name, + struct object_id *result, struct strbuf *result_path, + unsigned short *mode) { int retval = MISSING_OBJECT; struct dir_state *parents = NULL; @@ -617,7 +620,7 @@ enum get_oid_result get_tree_entry_follow_symlinks(struct object_id *tree_oid, c void *tree; struct object_id root; unsigned long size; - tree = read_object_with_reference(the_repository, + tree = read_object_with_reference(r, ¤t_tree_oid, tree_type, &size, &root); @@ -687,7 +690,7 @@ enum get_oid_result get_tree_entry_follow_symlinks(struct object_id *tree_oid, c } /* Look up the first (or only) path component in the tree. */ - find_result = find_tree_entry(the_repository, &t, namebuf.buf, + find_result = find_tree_entry(r, &t, namebuf.buf, ¤t_tree_oid, mode); if (find_result) { goto done; @@ -731,7 +734,8 @@ enum get_oid_result get_tree_entry_follow_symlinks(struct object_id *tree_oid, c */ retval = DANGLING_SYMLINK; - contents = read_object_file(¤t_tree_oid, &type, + contents = repo_read_object_file(r, + ¤t_tree_oid, &type, &link_len); if (!contents) diff --git a/tree-walk.h b/tree-walk.h index 639f79187f..2a5db29e8f 100644 --- a/tree-walk.h +++ b/tree-walk.h @@ -53,7 +53,7 @@ struct traverse_info; typedef int (*traverse_callback_t)(int n, unsigned long mask, unsigned long dirmask, struct name_entry *entry, struct traverse_info *); int traverse_trees(struct index_state *istate, int n, struct tree_desc *t, struct traverse_info *info); -enum get_oid_result get_tree_entry_follow_symlinks(struct object_id *tree_oid, const char *name, struct object_id *result, struct strbuf *result_path, unsigned short *mode); +enum get_oid_result get_tree_entry_follow_symlinks(struct repository *r, struct object_id *tree_oid, const char *name, struct object_id *result, struct strbuf *result_path, unsigned short *mode); struct traverse_info { const char *traverse_path; -- 2.22.0.rc0.322.g2b0371e29a ^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v2 5/6] match-trees.c: remove the_repo from shift_tree*() 2019-06-27 9:28 ` [PATCH v2 " Nguyễn Thái Ngọc Duy ` (3 preceding siblings ...) 2019-06-27 9:28 ` [PATCH v2 4/6] tree-walk.c: remove the_repo from get_tree_entry_follow_symlinks() Nguyễn Thái Ngọc Duy @ 2019-06-27 9:28 ` Nguyễn Thái Ngọc Duy 2019-06-27 9:28 ` [PATCH v2 6/6] Use the right 'struct repository' instead of the_repository Nguyễn Thái Ngọc Duy ` (2 subsequent siblings) 7 siblings, 0 replies; 31+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2019-06-27 9:28 UTC (permalink / raw) To: pclouds; +Cc: git, gitster, Derrick Stolee, Johannes Schindelin Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> --- cache.h | 4 ++-- match-trees.c | 12 +++++++----- merge-recursive.c | 4 ++-- t/helper/test-match-trees.c | 2 +- 4 files changed, 12 insertions(+), 10 deletions(-) diff --git a/cache.h b/cache.h index cd84cc9bbe..ddefda2bb6 100644 --- a/cache.h +++ b/cache.h @@ -1786,8 +1786,8 @@ int add_files_to_cache(const char *prefix, const struct pathspec *pathspec, int extern int diff_auto_refresh_index; /* match-trees.c */ -void shift_tree(const struct object_id *, const struct object_id *, struct object_id *, int); -void shift_tree_by(const struct object_id *, const struct object_id *, struct object_id *, const char *); +void shift_tree(struct repository *, const struct object_id *, const struct object_id *, struct object_id *, int); +void shift_tree_by(struct repository *, const struct object_id *, const struct object_id *, struct object_id *, const char *); /* * whitespace rules. diff --git a/match-trees.c b/match-trees.c index de7e8a6783..f6c194c1cc 100644 --- a/match-trees.c +++ b/match-trees.c @@ -248,7 +248,8 @@ static int splice_tree(const struct object_id *oid1, const char *prefix, * other hand, it could cover tree one and we might need to pick a * subtree of it. */ -void shift_tree(const struct object_id *hash1, +void shift_tree(struct repository *r, + const struct object_id *hash1, const struct object_id *hash2, struct object_id *shifted, int depth_limit) @@ -290,7 +291,7 @@ void shift_tree(const struct object_id *hash1, if (!*del_prefix) return; - if (get_tree_entry(the_repository, hash2, del_prefix, shifted, &mode)) + if (get_tree_entry(r, hash2, del_prefix, shifted, &mode)) die("cannot find path %s in tree %s", del_prefix, oid_to_hex(hash2)); return; @@ -307,7 +308,8 @@ void shift_tree(const struct object_id *hash1, * Unfortunately we cannot fundamentally tell which one to * be prefixed, as recursive merge can work in either direction. */ -void shift_tree_by(const struct object_id *hash1, +void shift_tree_by(struct repository *r, + const struct object_id *hash1, const struct object_id *hash2, struct object_id *shifted, const char *shift_prefix) @@ -317,12 +319,12 @@ void shift_tree_by(const struct object_id *hash1, unsigned candidate = 0; /* Can hash2 be a tree at shift_prefix in tree hash1? */ - if (!get_tree_entry(the_repository, hash1, shift_prefix, &sub1, &mode1) && + if (!get_tree_entry(r, hash1, shift_prefix, &sub1, &mode1) && S_ISDIR(mode1)) candidate |= 1; /* Can hash1 be a tree at shift_prefix in tree hash2? */ - if (!get_tree_entry(the_repository, hash2, shift_prefix, &sub2, &mode2) && + if (!get_tree_entry(r, hash2, shift_prefix, &sub2, &mode2) && S_ISDIR(mode2)) candidate |= 2; diff --git a/merge-recursive.c b/merge-recursive.c index b051066795..6d772eb0eb 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -153,9 +153,9 @@ static struct tree *shift_tree_object(struct repository *repo, struct object_id shifted; if (!*subtree_shift) { - shift_tree(&one->object.oid, &two->object.oid, &shifted, 0); + shift_tree(repo, &one->object.oid, &two->object.oid, &shifted, 0); } else { - shift_tree_by(&one->object.oid, &two->object.oid, &shifted, + shift_tree_by(repo, &one->object.oid, &two->object.oid, &shifted, subtree_shift); } if (oideq(&two->object.oid, &shifted)) diff --git a/t/helper/test-match-trees.c b/t/helper/test-match-trees.c index 96857f26ac..b9fd427571 100644 --- a/t/helper/test-match-trees.c +++ b/t/helper/test-match-trees.c @@ -20,7 +20,7 @@ int cmd__match_trees(int ac, const char **av) if (!two) die("not a tree-ish %s", av[2]); - shift_tree(&one->object.oid, &two->object.oid, &shifted, -1); + shift_tree(the_repository, &one->object.oid, &two->object.oid, &shifted, -1); printf("shifted: %s\n", oid_to_hex(&shifted)); exit(0); -- 2.22.0.rc0.322.g2b0371e29a ^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v2 6/6] Use the right 'struct repository' instead of the_repository 2019-06-27 9:28 ` [PATCH v2 " Nguyễn Thái Ngọc Duy ` (4 preceding siblings ...) 2019-06-27 9:28 ` [PATCH v2 5/6] match-trees.c: remove the_repo from shift_tree*() Nguyễn Thái Ngọc Duy @ 2019-06-27 9:28 ` Nguyễn Thái Ngọc Duy 2019-06-27 19:44 ` [PATCH v2 0/6] Kill the_repository in tree-walk.c Junio C Hamano 2019-06-28 9:35 ` [PATCH v2 7/6] t7814: do not generate same commits in different repos Nguyễn Thái Ngọc Duy 7 siblings, 0 replies; 31+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2019-06-27 9:28 UTC (permalink / raw) To: pclouds; +Cc: git, gitster, Derrick Stolee, Johannes Schindelin There are a couple of places where 'struct repository' is already passed around, but the_repository is still used. Use the right repo. Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> --- merge-recursive.c | 35 ++++++++++++++++++++--------------- sequencer.c | 4 ++-- sha1-name.c | 6 ++---- shallow.c | 3 ++- 4 files changed, 26 insertions(+), 22 deletions(-) diff --git a/merge-recursive.c b/merge-recursive.c index 6d772eb0eb..12300131fc 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -465,17 +465,18 @@ static void get_files_dirs(struct merge_options *opt, struct tree *tree) { struct pathspec match_all; memset(&match_all, 0, sizeof(match_all)); - read_tree_recursive(the_repository, tree, "", 0, 0, + read_tree_recursive(opt->repo, tree, "", 0, 0, &match_all, save_files_dirs, opt); } -static int get_tree_entry_if_blob(const struct object_id *tree, +static int get_tree_entry_if_blob(struct repository *r, + const struct object_id *tree, const char *path, struct diff_filespec *dfs) { int ret; - ret = get_tree_entry(the_repository, tree, path, &dfs->oid, &dfs->mode); + ret = get_tree_entry(r, tree, path, &dfs->oid, &dfs->mode); if (S_ISDIR(dfs->mode)) { oidcpy(&dfs->oid, &null_oid); dfs->mode = 0; @@ -487,15 +488,16 @@ static int get_tree_entry_if_blob(const struct object_id *tree, * Returns an index_entry instance which doesn't have to correspond to * a real cache entry in Git's index. */ -static struct stage_data *insert_stage_data(const char *path, +static struct stage_data *insert_stage_data(struct repository *r, + const char *path, struct tree *o, struct tree *a, struct tree *b, struct string_list *entries) { struct string_list_item *item; struct stage_data *e = xcalloc(1, sizeof(struct stage_data)); - get_tree_entry_if_blob(&o->object.oid, path, &e->stages[1]); - get_tree_entry_if_blob(&a->object.oid, path, &e->stages[2]); - get_tree_entry_if_blob(&b->object.oid, path, &e->stages[3]); + get_tree_entry_if_blob(r, &o->object.oid, path, &e->stages[1]); + get_tree_entry_if_blob(r, &a->object.oid, path, &e->stages[2]); + get_tree_entry_if_blob(r, &b->object.oid, path, &e->stages[3]); item = string_list_insert(entries, path); item->util = e; return e; @@ -1900,12 +1902,13 @@ static struct diff_queue_struct *get_diffpairs(struct merge_options *opt, return ret; } -static int tree_has_path(struct tree *tree, const char *path) +static int tree_has_path(struct repository *r, struct tree *tree, + const char *path) { struct object_id hashy; unsigned short mode_o; - return !get_tree_entry(the_repository, + return !get_tree_entry(r, &tree->object.oid, path, &hashy, &mode_o); } @@ -2057,7 +2060,7 @@ static char *handle_path_level_conflicts(struct merge_options *opt, */ if (collision_ent->reported_already) { clean = 0; - } else if (tree_has_path(tree, new_path)) { + } else if (tree_has_path(opt->repo, tree, new_path)) { collision_ent->reported_already = 1; strbuf_add_separated_string_list(&collision_paths, ", ", &collision_ent->source_files); @@ -2135,7 +2138,7 @@ static void handle_directory_level_conflicts(struct merge_options *opt, string_list_append(&remove_from_merge, merge_ent->dir)->util = merge_ent; strbuf_release(&merge_ent->new_dir); - } else if (tree_has_path(head, head_ent->dir)) { + } else if (tree_has_path(opt->repo, head, head_ent->dir)) { /* 2. This wasn't a directory rename after all */ string_list_append(&remove_from_head, head_ent->dir)->util = head_ent; @@ -2149,7 +2152,7 @@ static void handle_directory_level_conflicts(struct merge_options *opt, hashmap_iter_init(dir_re_merge, &iter); while ((merge_ent = hashmap_iter_next(&iter))) { head_ent = dir_rename_find_entry(dir_re_head, merge_ent->dir); - if (tree_has_path(merge, merge_ent->dir)) { + if (tree_has_path(opt->repo, merge, merge_ent->dir)) { /* 2. This wasn't a directory rename after all */ string_list_append(&remove_from_merge, merge_ent->dir)->util = merge_ent; @@ -2478,7 +2481,7 @@ static void apply_directory_rename_modifications(struct merge_options *opt, if (pair->status == 'R') re->dst_entry->processed = 1; - re->dst_entry = insert_stage_data(new_path, + re->dst_entry = insert_stage_data(opt->repo, new_path, o_tree, a_tree, b_tree, entries); item = string_list_insert(entries, new_path); @@ -2587,14 +2590,16 @@ static struct string_list *get_renames(struct merge_options *opt, re->dir_rename_original_dest = NULL; item = string_list_lookup(entries, re->pair->one->path); if (!item) - re->src_entry = insert_stage_data(re->pair->one->path, + re->src_entry = insert_stage_data(opt->repo, + re->pair->one->path, o_tree, a_tree, b_tree, entries); else re->src_entry = item->util; item = string_list_lookup(entries, re->pair->two->path); if (!item) - re->dst_entry = insert_stage_data(re->pair->two->path, + re->dst_entry = insert_stage_data(opt->repo, + re->pair->two->path, o_tree, a_tree, b_tree, entries); else re->dst_entry = item->util; diff --git a/sequencer.c b/sequencer.c index d565fcf2b1..64428ac28f 100644 --- a/sequencer.c +++ b/sequencer.c @@ -3733,7 +3733,7 @@ static int pick_commits(struct repository *r, unlink(rebase_path_author_script()); unlink(rebase_path_stopped_sha()); unlink(rebase_path_amend()); - unlink(git_path_merge_head(the_repository)); + unlink(git_path_merge_head(r)); delete_ref(NULL, "REBASE_HEAD", NULL, REF_NO_DEREF); if (item->command == TODO_BREAK) @@ -4107,7 +4107,7 @@ static int commit_staged_changes(struct repository *r, opts, flags)) return error(_("could not commit staged changes.")); unlink(rebase_path_amend()); - unlink(git_path_merge_head(the_repository)); + unlink(git_path_merge_head(r)); if (final_fixup) { unlink(rebase_path_fixup_msg()); unlink(rebase_path_squash_msg()); diff --git a/sha1-name.c b/sha1-name.c index 3c9fa10af8..6069fe006b 100644 --- a/sha1-name.c +++ b/sha1-name.c @@ -478,7 +478,7 @@ static enum get_oid_result get_short_oid(struct repository *r, * or migrated from loose to packed. */ if (status == MISSING_OBJECT) { - reprepare_packed_git(the_repository); + reprepare_packed_git(r); find_short_object_filename(&ds); find_short_packed_object(&ds); status = finish_object_disambiguation(&ds, oid); @@ -1389,9 +1389,7 @@ int repo_get_oid_mb(struct repository *r, two = lookup_commit_reference_gently(r, &oid_tmp, 0); if (!two) return -1; - if (r != the_repository) - BUG("sorry get_merge_bases() can't take struct repository yet"); - mbs = get_merge_bases(one, two); + mbs = repo_get_merge_bases(r, one, two); if (!mbs || mbs->next) st = -1; else { diff --git a/shallow.c b/shallow.c index ce45297940..5fa2b15d37 100644 --- a/shallow.c +++ b/shallow.c @@ -248,7 +248,8 @@ static void check_shallow_file_for_update(struct repository *r) if (r->parsed_objects->is_shallow == -1) BUG("shallow must be initialized by now"); - if (!stat_validity_check(r->parsed_objects->shallow_stat, git_path_shallow(the_repository))) + if (!stat_validity_check(r->parsed_objects->shallow_stat, + git_path_shallow(r))) die("shallow file has changed since we read it"); } -- 2.22.0.rc0.322.g2b0371e29a ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH v2 0/6] Kill the_repository in tree-walk.c 2019-06-27 9:28 ` [PATCH v2 " Nguyễn Thái Ngọc Duy ` (5 preceding siblings ...) 2019-06-27 9:28 ` [PATCH v2 6/6] Use the right 'struct repository' instead of the_repository Nguyễn Thái Ngọc Duy @ 2019-06-27 19:44 ` Junio C Hamano 2019-06-28 9:35 ` [PATCH v2 7/6] t7814: do not generate same commits in different repos Nguyễn Thái Ngọc Duy 7 siblings, 0 replies; 31+ messages in thread From: Junio C Hamano @ 2019-06-27 19:44 UTC (permalink / raw) To: Nguyễn Thái Ngọc Duy Cc: git, Derrick Stolee, Johannes Schindelin Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes: > tree-walk.h | 8 ++++--- > unpack-trees.c | 2 +- > 25 files changed, 129 insertions(+), 97 deletions(-) > > Range-diff dựa trên v1: > 1: 35d7cdbe6a ! 1: 9e73c39f9a sha1-file.c: remove the_repo from read_object_with_reference() I see inconsistent l10n here. I see merit in both (1) forcing the C locale when preparing project-wide communications like patch e-mails, even when end-user's UI is usually showing in another locale, for maximum reach, or (2) honoring the locale end-user's even when preparing project-wide communications, to help with mono-culture projects. Whichever stance we take, we should be consistent ;-) ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v2 7/6] t7814: do not generate same commits in different repos 2019-06-27 9:28 ` [PATCH v2 " Nguyễn Thái Ngọc Duy ` (6 preceding siblings ...) 2019-06-27 19:44 ` [PATCH v2 0/6] Kill the_repository in tree-walk.c Junio C Hamano @ 2019-06-28 9:35 ` Nguyễn Thái Ngọc Duy 2019-06-28 16:17 ` Junio C Hamano 7 siblings, 1 reply; 31+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2019-06-28 9:35 UTC (permalink / raw) To: pclouds; +Cc: Johannes.Schindelin, git, gitster, stolee t7814 has repo tree like this initial-repo submodule sub In each repo 'submodule' and 'sub', a commit is made to add the same initial file 'a' with the same message 'add a'. If tests run fast enough, the two commits are made in the same second, resulting identical commits. There is nothing wrong with that per-se. But it could make the test flaky. Currently all submodule odbs are merged back in the main one (because we can't, or couldn't, access separate submodule repos otherwise). But eventually we need to access objects from the right repo. Because the same commit could sometimes be present in both 'submodule' and 'sub', if there is a bug looking up objects in the wrong repo, sometimes it will go unnoticed because it finds the needed object in the wrong repo anyway. Fix this by changing commit time after every commit. This makes all commits unique. Of course there are still identical blobs in different repos, but because we often lookup commit first, then tree and blob, unique commits are already quite safe. Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> --- > And I can't quite understand how t7814 sometimes passed. I do now. This patch makes it fail consistently for me. This patch technically has nothing to do with this series, but I'll try to sneak it in because it was started from there. t/t7814-grep-recurse-submodules.sh | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/t/t7814-grep-recurse-submodules.sh b/t/t7814-grep-recurse-submodules.sh index 134a694516..a11366b4ce 100755 --- a/t/t7814-grep-recurse-submodules.sh +++ b/t/t7814-grep-recurse-submodules.sh @@ -14,12 +14,14 @@ test_expect_success 'setup directory structure and submodule' ' echo "(3|4)" >b/b && git add a b && git commit -m "add a and b" && + test_tick && git init submodule && echo "(1|2)d(3|4)" >submodule/a && git -C submodule add a && git -C submodule commit -m "add a" && git submodule add ./submodule && - git commit -m "added submodule" + git commit -m "added submodule" && + test_tick ' test_expect_success 'grep correctly finds patterns in a submodule' ' @@ -65,11 +67,14 @@ test_expect_success 'grep and nested submodules' ' echo "(1|2)d(3|4)" >submodule/sub/a && git -C submodule/sub add a && git -C submodule/sub commit -m "add a" && + test_tick && git -C submodule submodule add ./sub && git -C submodule add sub && git -C submodule commit -m "added sub" && + test_tick && git add submodule && git commit -m "updated submodule" && + test_tick && cat >expect <<-\EOF && a:(1|2)d(3|4) @@ -179,15 +184,18 @@ test_expect_success !MINGW 'grep recurse submodule colon in name' ' echo "(1|2)d(3|4)" >"parent/fi:le" && git -C parent add "fi:le" && git -C parent commit -m "add fi:le" && + test_tick && git init "su:b" && test_when_finished "rm -rf su:b" && echo "(1|2)d(3|4)" >"su:b/fi:le" && git -C "su:b" add "fi:le" && git -C "su:b" commit -m "add fi:le" && + test_tick && git -C parent submodule add "../su:b" "su:b" && git -C parent commit -m "add submodule" && + test_tick && cat >expect <<-\EOF && fi:le:(1|2)d(3|4) @@ -210,15 +218,18 @@ test_expect_success 'grep history with moved submoules' ' echo "(1|2)d(3|4)" >parent/file && git -C parent add file && git -C parent commit -m "add file" && + test_tick && git init sub && test_when_finished "rm -rf sub" && echo "(1|2)d(3|4)" >sub/file && git -C sub add file && git -C sub commit -m "add file" && + test_tick && git -C parent submodule add ../sub dir/sub && git -C parent commit -m "add submodule" && + test_tick && cat >expect <<-\EOF && dir/sub/file:(1|2)d(3|4) @@ -229,6 +240,7 @@ test_expect_success 'grep history with moved submoules' ' git -C parent mv dir/sub sub-moved && git -C parent commit -m "moved submodule" && + test_tick && cat >expect <<-\EOF && file:(1|2)d(3|4) @@ -251,6 +263,7 @@ test_expect_success 'grep using relative path' ' echo "(1|2)d(3|4)" >sub/file && git -C sub add file && git -C sub commit -m "add file" && + test_tick && git init parent && echo "(1|2)d(3|4)" >parent/file && @@ -260,6 +273,7 @@ test_expect_success 'grep using relative path' ' git -C parent add src/file2 && git -C parent submodule add ../sub && git -C parent commit -m "add files and submodule" && + test_tick && # From top works cat >expect <<-\EOF && @@ -293,6 +307,7 @@ test_expect_success 'grep from a subdir' ' echo "(1|2)d(3|4)" >sub/file && git -C sub add file && git -C sub commit -m "add file" && + test_tick && git init parent && mkdir parent/src && @@ -301,6 +316,7 @@ test_expect_success 'grep from a subdir' ' git -C parent submodule add ../sub src/sub && git -C parent submodule add ../sub sub && git -C parent commit -m "add files and submodules" && + test_tick && # Verify grep from root works cat >expect <<-\EOF && -- 2.22.0.rc0.322.g2b0371e29a ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH v2 7/6] t7814: do not generate same commits in different repos 2019-06-28 9:35 ` [PATCH v2 7/6] t7814: do not generate same commits in different repos Nguyễn Thái Ngọc Duy @ 2019-06-28 16:17 ` Junio C Hamano 0 siblings, 0 replies; 31+ messages in thread From: Junio C Hamano @ 2019-06-28 16:17 UTC (permalink / raw) To: Nguyễn Thái Ngọc Duy; +Cc: Johannes.Schindelin, git, stolee Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes: > t7814 has repo tree like this > > initial-repo > submodule > sub > > In each repo 'submodule' and 'sub', a commit is made to add the same > initial file 'a' with the same message 'add a'. If tests run fast > enough, the two commits are made in the same second, resulting > identical commits. > > There is nothing wrong with that per-se. But it could make the test > flaky. Currently all submodule odbs are merged back in the main > one (because we can't, or couldn't, access separate submodule repos > otherwise). But eventually we need to access objects from the right > repo. > > Because the same commit could sometimes be present in both 'submodule' > and 'sub', if there is a bug looking up objects in the wrong repo, > sometimes it will go unnoticed because it finds the needed object in the > wrong repo anyway. > > Fix this by changing commit time after every commit. This makes all > commits unique. Of course there are still identical blobs in different > repos, but because we often lookup commit first, then tree and blob, > unique commits are already quite safe. > > Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> > --- > > And I can't quite understand how t7814 sometimes passed. > > I do now. This patch makes it fail consistently for me. Well analysed. Very well done. Thanks. ^ permalink raw reply [flat|nested] 31+ messages in thread
end of thread, other threads:[~2019-06-28 16:17 UTC | newest] Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-06-24 9:55 [PATCH 0/6] Kill the_repository in tree-walk.c Nguyễn Thái Ngọc Duy 2019-06-24 9:55 ` [PATCH 1/6] sha1-file.c: remove the_repo from read_object_with_reference() Nguyễn Thái Ngọc Duy 2019-06-27 12:54 ` Johannes Schindelin 2019-06-27 13:12 ` Duy Nguyen 2019-06-24 9:55 ` [PATCH 2/6] tree-walk.c: remove the_repo from fill_tree_descriptor() Nguyễn Thái Ngọc Duy 2019-06-24 13:30 ` Derrick Stolee 2019-06-26 16:27 ` Junio C Hamano 2019-06-26 16:47 ` Derrick Stolee 2019-06-24 9:55 ` [PATCH 3/6] tree-walk.c: remove the_repo from get_tree_entry() Nguyễn Thái Ngọc Duy 2019-06-24 14:20 ` Derrick Stolee 2019-06-24 14:55 ` Duy Nguyen 2019-06-24 9:55 ` [PATCH 4/6] tree-walk.c: remove the_repo from get_tree_entry_follow_symlinks() Nguyễn Thái Ngọc Duy 2019-06-24 9:55 ` [PATCH 5/6] match-trees.c: remove the_repo from shift_tree*() Nguyễn Thái Ngọc Duy 2019-06-24 9:55 ` [PATCH 6/6] Use the right 'struct repository' instead of the_repository Nguyễn Thái Ngọc Duy 2019-06-24 14:24 ` Derrick Stolee 2019-06-24 14:45 ` Duy Nguyen 2019-06-27 9:06 ` Johannes Schindelin 2019-06-26 17:20 ` [PATCH 0/6] Kill the_repository in tree-walk.c Junio C Hamano 2019-06-27 13:04 ` Johannes Schindelin 2019-06-27 17:09 ` Derrick Stolee 2019-06-27 9:28 ` [PATCH v2 " Nguyễn Thái Ngọc Duy 2019-06-27 9:28 ` [PATCH v2 1/6] sha1-file.c: remove the_repo from read_object_with_reference() Nguyễn Thái Ngọc Duy 2019-06-28 12:46 ` Johannes Schindelin 2019-06-27 9:28 ` [PATCH v2 2/6] tree-walk.c: remove the_repo from fill_tree_descriptor() Nguyễn Thái Ngọc Duy 2019-06-27 9:28 ` [PATCH v2 3/6] tree-walk.c: remove the_repo from get_tree_entry() Nguyễn Thái Ngọc Duy 2019-06-27 9:28 ` [PATCH v2 4/6] tree-walk.c: remove the_repo from get_tree_entry_follow_symlinks() Nguyễn Thái Ngọc Duy 2019-06-27 9:28 ` [PATCH v2 5/6] match-trees.c: remove the_repo from shift_tree*() Nguyễn Thái Ngọc Duy 2019-06-27 9:28 ` [PATCH v2 6/6] Use the right 'struct repository' instead of the_repository Nguyễn Thái Ngọc Duy 2019-06-27 19:44 ` [PATCH v2 0/6] Kill the_repository in tree-walk.c Junio C Hamano 2019-06-28 9:35 ` [PATCH v2 7/6] t7814: do not generate same commits in different repos Nguyễn Thái Ngọc Duy 2019-06-28 16:17 ` Junio C Hamano
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).