| Message ID | 20210316021312.13927-1-avarab@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | tree-walk: mostly replace "mode" with "enum object_type" | expand |
On Mon, Mar 15, 2021 at 7:13 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > > A v2 of the big tree-walk.[ch] refactoring series, goals etc. at the > v1 at: > https://lore.kernel.org/git/20210308150650.18626-1-avarab@gmail.com/ > > It is based on my just-re-rolled v3 read_tree_recursive() series: > https://lore.kernel.org/git/20210315234344.28427-1-avarab@gmail.com/ > > This version should address all the feedback on v1 for patces 1-27/30, > thanks to Elijah for very valuable comments on v1. > > It's mostly small nits here and there, except: > > - I found that the change I'd made to update-index.c was buggy at the > point it was introduced in the series, the object_type would be > uninitialized. There were/are no tests for that, but I've moved > things around so we don't have that bug anymore. > > - Elijah had a comment on whether we needed oid_object_info() in > blame.c. As it turns out we don't need a "is blob?" check at all > there. There's a new 28/29 to refactor that small part of blame.c, > along with a test. > > What this re-roll *does not* include is the final 28-30/30 part of v1 > to s/mode/raw_mode/g and move canonical_mode() out of tree-walk.h. > > So a follow-up series will still be needed to fix the fsck.c check for > bad modes, but I wanted to split that tricker change off from this > rather big initial refactoring. Splitting those trickier bits off makes sense. This series looks really good and addresses most of my feedback from v1. There were two commit message wording comments in patches 21 and 27 leftover from the previous round, and a nasty bug introduced in patch 6 still left from the previous round. I also had a question on the new testcase in patch 28. Other than that, though, the series looks good to me. > > Ævar Arnfjörð Bjarmason (29): > diff.c: remove redundant canon_mode() call > notes & match-trees: use name_entry's "pathlen" member > cache.h: add a comment to object_type() > tree-walk.h: add object_type member to name_entry > tree-walk.c: migrate to using new "object_type" field when possible > cache.h: have base_name_compare() take "is tree?", not "mode" > tree-walk.h users: switch object_type(...) to new .object_type > tree.h: format argument lists of read_tree_recursive() users > tree.h users: format argument lists in archive.c > archive: get rid of 'stage' parameter > tree.h API: make read_tree_fn_t take an "enum object_type" > tree-walk.h users: migrate "p->mode &&" pattern > tree-walk.h users: refactor chained "mode" if/else into switch > tree-walk.h users: migrate miscellaneous "mode" to "object_type" > merge-tree tests: test for the mode comparison in same_entry() > merge-ort: correct reference to test in 62fdec17a11 > fsck.c: switch on "object_type" in fsck_walk_tree() > tree-walk.h users: use temporary variable(s) for "mode" > tree-walk.h API: formatting changes for subsequent commit > tree-walk.h API: rename get_tree_entry() to get_tree_entry_mode() > tree-walk.h API users: use "tmp" for mode in shift_tree_by() > tree-walk.h API: add get_tree_entry_type() > tree-walk.h API: document and format tree_entry_extract() > tree-entry.h API: rename tree_entry_extract() to > tree_entry_extract_mode() > tree-walk.h API: add a tree_entry_extract_all() function > tree-walk.h API: add get_tree_entry_all() > tree-walk.h API: add a get_tree_entry_path() function > blame: emit a better error on 'git blame directory' > tree-walk.h API: add a tree_entry_extract_type() function > > archive.c | 50 +++++++++--------- > blame.c | 9 ++-- > builtin/checkout.c | 6 ++- > builtin/fast-import.c | 8 +-- > builtin/grep.c | 6 +-- > builtin/log.c | 7 +-- > builtin/ls-files.c | 6 ++- > builtin/ls-tree.c | 14 +++--- > builtin/merge-tree.c | 30 +++++++---- > builtin/mktree.c | 4 +- > builtin/pack-objects.c | 6 +-- > builtin/reflog.c | 3 +- > builtin/rm.c | 2 +- > builtin/update-index.c | 6 ++- > cache-tree.c | 2 +- > cache.h | 11 ++-- > combine-diff.c | 8 +-- > delta-islands.c | 2 +- > diff.c | 2 +- > fsck.c | 23 ++++----- > http-push.c | 6 ++- > line-log.c | 2 +- > list-objects.c | 20 +++++--- > match-trees.c | 52 +++++++++---------- > merge-ort.c | 13 ++--- > merge-recursive.c | 33 ++++++------ > notes.c | 14 +++--- > object-name.c | 7 ++- > pack-bitmap-write.c | 8 +-- > read-cache.c | 16 +++--- > revision.c | 12 +++-- > t/t4300-merge-tree.sh | 44 ++++++++++++++++ > t/t8004-blame-with-conflicts.sh | 20 ++++++++ > tree-diff.c | 30 +++++++---- > tree-walk.c | 89 ++++++++++++++++++++++++--------- > tree-walk.h | 63 ++++++++++++++++++++--- > tree.c | 19 ++++--- > tree.h | 5 +- > unpack-trees.c | 24 +++++---- > walker.c | 22 ++++---- > 40 files changed, 460 insertions(+), 244 deletions(-) > > Range-diff: > 1: e5df57c3440 = 1: f9bbc30f69f diff.c: remove redundant canon_mode() call > 2: 8c2500bbf35 = 2: 187fc2c3e64 notes & match-trees: use name_entry's "pathlen" member > 3: 3d98e0c132f ! 3: 311637c5583 cache.h: add a comment to object_type() > @@ Commit message > cache.h: add a comment to object_type() > > Add a comment to the object_type() function to explain what it > - returns, and whet the "mode" is in the "else" case. > + returns, and what the "mode" is in the "else" case. > > The object_type() function dates back to 4d1012c3709 (Fix rev-list > when showing objects involving submodules, 2007-11-11). It's not > 4: ce5808b317c = 4: fecfe3d462c tree-walk.h: add object_type member to name_entry > 5: 18f26531acf = 5: db961ab5e8d tree-walk.c: migrate to using new "object_type" field when possible > 6: 55e2640b815 ! 6: df2fc76161d cache.h: have base_name_compare() take "is tree?", not "mode" > @@ cache.h: int repo_interpret_branch_name(struct repository *r, > > -int base_name_compare(const char *name1, int len1, int mode1, const char *name2, int len2, int mode2); > -int df_name_compare(const char *name1, int len1, int mode1, const char *name2, int len2, int mode2); > -+int base_name_compare(const char *name1, int len1, int isdir1, const char *name2, int len2, int isdir2); > -+int df_name_compare(const char *name1, int len1, int isdir1, const char *name2, int len2, int isdir2); > ++int base_name_compare(const char *name1, int len1, int istree1, const char *name2, int len2, int istree2); > ++int df_name_compare(const char *name1, int len1, int istree1, const char *name2, int len2, int istree2); > 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); > > @@ combine-diff.c > const struct diff_filespec *two) > { > - if (!S_ISDIR(one->mode) && !S_ISDIR(two->mode)) > -+ int isdir_one = S_ISDIR(one->mode); > -+ int isdir_two = S_ISDIR(two->mode); > -+ if (!isdir_one && !isdir_two) > ++ int istree_one = S_ISDIR(one->mode); > ++ int istree_two = S_ISDIR(two->mode); > ++ if (!istree_one && !istree_two) > return strcmp(one->path, two->path); > > - return base_name_compare(one->path, strlen(one->path), one->mode, > - two->path, strlen(two->path), two->mode); > -+ return base_name_compare(one->path, strlen(one->path), isdir_one, > -+ two->path, strlen(two->path), isdir_two); > ++ return base_name_compare(one->path, strlen(one->path), istree_one, > ++ two->path, strlen(two->path), istree_two); > } > > static int filename_changed(char status) > @@ match-trees.c: static void *fill_tree_desc_strict(struct tree_desc *desc, > { > - return base_name_compare(a->path, tree_entry_len(a), a->mode, > - b->path, tree_entry_len(b), b->mode); > -+ int isdira = a->object_type == OBJ_TREE; > -+ int isdirb = b->object_type == OBJ_TREE; > -+ return base_name_compare(a->path, tree_entry_len(a), isdira, > -+ b->path, tree_entry_len(b), isdirb); > ++ int istree_a = (a->object_type == OBJ_TREE); > ++ int istree_b = (b->object_type == OBJ_TREE); > ++ return base_name_compare(a->path, tree_entry_len(a), istree_a, > ++ b->path, tree_entry_len(b), istree_b); > } > > /* > @@ read-cache.c: int ie_modified(struct index_state *istate, > > -int base_name_compare(const char *name1, int len1, int mode1, > - const char *name2, int len2, int mode2) > -+int base_name_compare(const char *name1, int len1, int isdir1, > -+ const char *name2, int len2, int isdir2) > ++int base_name_compare(const char *name1, int len1, int istree1, > ++ const char *name2, int len2, int istree2) > { > unsigned char c1, c2; > int len = len1 < len2 ? len1 : len2; > @@ read-cache.c: int base_name_compare(const char *name1, int len1, int mode1, > c1 = name1[len]; > c2 = name2[len]; > - if (!c1 && S_ISDIR(mode1)) > -+ if (!c1 && isdir1) > ++ if (!c1 && istree1) > c1 = '/'; > - if (!c2 && S_ISDIR(mode2)) > -+ if (!c2 && isdir2) > ++ if (!c2 && istree2) > c2 = '/'; > return (c1 < c2) ? -1 : (c1 > c2) ? 1 : 0; > } > @@ read-cache.c: int base_name_compare(const char *name1, int len1, int mode1, > */ > -int df_name_compare(const char *name1, int len1, int mode1, > - const char *name2, int len2, int mode2) > -+int df_name_compare(const char *name1, int len1, int isdir1, > -+ const char *name2, int len2, int isdir2) > ++int df_name_compare(const char *name1, int len1, int istree1, > ++ const char *name2, int len2, int istree2) > { > int len = len1 < len2 ? len1 : len2, cmp; > unsigned char c1, c2; > @@ read-cache.c: int df_name_compare(const char *name1, int len1, int mode1, > return 0; > c1 = name1[len]; > - if (!c1 && S_ISDIR(mode1)) > -+ if (!c1 && isdir1) > ++ if (!c1 && istree1) > c1 = '/'; > c2 = name2[len]; > - if (!c2 && S_ISDIR(mode2)) > -+ if (!c2 && isdir2) > ++ if (!c2 && istree2) > c2 = '/'; > if (c1 == '/' && !c2) > return 0; > @@ tree-diff.c: static int tree_entry_pathcmp(struct tree_desc *t1, struct tree_des > { > struct name_entry *e1, *e2; > int cmp; > -+ int e1_is_tree, e2_is_tree; > ++ int istree_e1, istree_e2; > > /* empty descriptors sort after valid tree entries */ > if (!t1->size) > @@ tree-diff.c: static int tree_entry_pathcmp(struct tree_desc *t1, struct tree_des > return -1; > > e1 = &t1->entry; > -+ e1_is_tree = e1->object_type == OBJ_TREE; > ++ istree_e1 = (e1->object_type == OBJ_TREE); > e2 = &t2->entry; > - cmp = base_name_compare(e1->path, tree_entry_len(e1), e1->mode, > - e2->path, tree_entry_len(e2), e2->mode); > -+ e2_is_tree = e2->object_type == OBJ_TREE; > -+ cmp = base_name_compare(e1->path, tree_entry_len(e1), e1_is_tree, > -+ e2->path, tree_entry_len(e2), e2_is_tree); > ++ istree_e2 = (e2->object_type == OBJ_TREE); > ++ cmp = base_name_compare(e1->path, tree_entry_len(e1), istree_e1, > ++ e2->path, tree_entry_len(e2), istree_e2); > return cmp; > } > > @@ unpack-trees.c: static int traverse_trees_recursive(int n, unsigned long dirmask > const struct traverse_info *info, > const char *name, size_t namelen, > - unsigned mode) > -+ unsigned is_tree) > ++ unsigned istree) > { > int pathlen, ce_len; > const char *ce_name; > @@ unpack-trees.c: static int do_compare_entry_piecewise(const struct cache_entry * > ce_name = ce->name + pathlen; > > - return df_name_compare(ce_name, ce_len, S_IFREG, name, namelen, mode); > -+ return df_name_compare(ce_name, ce_len, 0, name, namelen, is_tree); > ++ return df_name_compare(ce_name, ce_len, 0, name, namelen, istree); > } > > static int do_compare_entry(const struct cache_entry *ce, > const struct traverse_info *info, > const char *name, size_t namelen, > - unsigned mode) > -+ unsigned is_tree) > ++ unsigned istree) > { > int pathlen, ce_len; > const char *ce_name; > @@ unpack-trees.c: static int do_compare_entry(const struct cache_entry *ce, > */ > if (!info->traverse_path) > - return do_compare_entry_piecewise(ce, info, name, namelen, mode); > -+ return do_compare_entry_piecewise(ce, info, name, namelen, is_tree); > ++ return do_compare_entry_piecewise(ce, info, name, namelen, istree); > > cmp = strncmp(ce->name, info->traverse_path, info->pathlen); > if (cmp) > @@ unpack-trees.c: static int do_compare_entry(const struct cache_entry *ce, > ce_name = ce->name + pathlen; > > - return df_name_compare(ce_name, ce_len, S_IFREG, name, namelen, mode); > -+ return df_name_compare(ce_name, ce_len, 0, name, namelen, is_tree); > ++ return df_name_compare(ce_name, ce_len, 0, name, namelen, istree); > } > > static int compare_entry(const struct cache_entry *ce, const struct traverse_info *info, const struct name_entry *n) > { > - int cmp = do_compare_entry(ce, info, n->path, n->pathlen, n->mode); > -+ int is_tree = n->object_type == OBJ_TREE; > -+ int cmp = do_compare_entry(ce, info, n->path, n->pathlen, is_tree); > ++ int istree = (n->object_type == OBJ_TREE); > ++ int cmp = do_compare_entry(ce, info, n->path, n->pathlen, istree); > if (cmp) > return cmp; > > 7: abc128f6cb9 = 7: 49d5da8c086 tree-walk.h users: switch object_type(...) to new .object_type > 8: dcf13faf3cd ! 8: c9d209d496a tree.h: format argument lists of read_tree_recursive() users > @@ archive.c: static int check_attr_export_subst(const struct attr_check *check) > - void *context) > + int baselen, const char *filename, > + unsigned mode, > -+ int stage, void *context) > ++ int stage, > ++ void *context) > { > static struct strbuf path = STRBUF_INIT; > struct archiver_context *c = context; > @@ tree.h: struct tree *parse_tree_indirect(const struct object_id *oid); > + unsigned int, > + void *); > > - int read_tree_recursive(struct repository *r, > - struct tree *tree, > + int read_tree_at(struct repository *r, > + struct tree *tree, > 9: b33fcf82349 ! 9: a6d2660fe14 tree.h users: format argument lists in archive.c > @@ Commit message > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > > ## archive.c ## > -@@ archive.c: static int check_attr_export_subst(const struct attr_check *check) > - static int write_archive_entry(const struct object_id *oid, const char *base, > - int baselen, const char *filename, > - unsigned mode, > -- int stage, void *context) > -+ int stage, > -+ void *context) > - { > - static struct strbuf path = STRBUF_INIT; > - struct archiver_context *c = context; > @@ archive.c: static int write_archive_entry(const struct object_id *oid, const char *base, > } > > 10: 6a20d3c058f = 10: 15f7f89acca archive: get rid of 'stage' parameter > 11: a7f7444917c ! 11: 7a71404ea3f tree.h API: make read_tree_fn_t take an "enum object_type" > @@ merge-recursive.c: static int save_files_dirs(const struct object_id *oid, > static void get_files_dirs(struct merge_options *opt, struct tree *tree) > > ## tree.c ## > -@@ tree.c: static int read_tree_1(struct repository *r, > +@@ tree.c: int read_tree_at(struct repository *r, > init_tree_desc(&desc, tree->buffer, tree->size); > > while (tree_entry(&desc, &entry)) { > @@ tree.c: static int read_tree_1(struct repository *r, > if (retval != all_entries_interesting) { > retval = tree_entry_interesting(r->index, &entry, > base, 0, pathspec); > -@@ tree.c: static int read_tree_1(struct repository *r, > +@@ tree.c: int read_tree_at(struct repository *r, > } > > switch (fn(&entry.oid, base, > @@ tree.c: static int read_tree_1(struct repository *r, > case 0: > continue; > case READ_TREE_RECURSIVE: > -@@ tree.c: static int read_tree_1(struct repository *r, > +@@ tree.c: int read_tree_at(struct repository *r, > return -1; > } > > @@ tree.c: static int read_tree_1(struct repository *r, > commit = lookup_commit(r, &entry.oid); > if (!commit) > die("Commit %s in submodule path %s%s not found", > -@@ tree.c: static int read_tree_1(struct repository *r, > +@@ tree.c: int read_tree_at(struct repository *r, > base->buf, entry.path); > > oidcpy(&oid, get_commit_tree_oid(commit)); > @@ tree.h: int cmp_cache_name_compare(const void *a_, const void *b_); > + enum object_type, unsigned int, > void *); > > - int read_tree_recursive(struct repository *r, > + int read_tree_at(struct repository *r, > 12: 625c643513d ! 12: 64dc9364bae tree-walk.h users: migrate "p->mode &&" pattern > @@ Metadata > ## Commit message ## > tree-walk.h users: migrate "p->mode &&" pattern > > - Change code that dpends on "p->mode" either being a valid mode or zero > - to use a p->object_type comparison to "OBJ_NONE". > + Change code that depends on "p->mode" either being a valid mode or > + zero to use a p->object_type comparison to "OBJ_NONE". > > - The object_type() function in cache.h will not return OBJ_NONE, but in > - this these API users are implicitly relying on the memzero() that > - happens in setup_traverse_info(). > + The object_type() function in cache.h will not return OBJ_NONE, but > + these API users are implicitly relying on the memzero() that happens > + in setup_traverse_info(). > > Since OBJ_NONE is "0" we can also rely on that being zero'd out here, > along with the rest of the structure. I think this is slightly less > 13: 37b28c7feff = 13: 93ed3edbbd5 tree-walk.h users: refactor chained "mode" if/else into switch > 14: e0b8ec6e291 = 14: 7aa48aa34c3 tree-walk.h users: migrate miscellaneous "mode" to "object_type" > 15: 0cd162c43d7 = 15: 3ae81621dcf merge-tree tests: test for the mode comparison in same_entry() > 16: f8ce666d4a7 = 16: 4249ad5c4de merge-ort: correct reference to test in 62fdec17a11 > 17: 4963902ba97 = 17: e5e17505dde fsck.c: switch on "object_type" in fsck_walk_tree() > 18: d74e6778009 = 18: 3f0b884f1fd tree-walk.h users: use temporary variable(s) for "mode" > 19: d39db486d4e = 19: 174167613bb tree-walk.h API: formatting changes for subsequent commit > 20: 69eb956b1ab = 20: ec76db613f2 tree-walk.h API: rename get_tree_entry() to get_tree_entry_mode() > 21: cc56453e600 = 21: 11e34941729 tree-walk.h API users: use "tmp" for mode in shift_tree_by() > 22: ca9e3b3ad00 ! 22: b31c106557f tree-walk.h API: Add get_tree_entry_type() > @@ Metadata > Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > > ## Commit message ## > - tree-walk.h API: Add get_tree_entry_type() > + tree-walk.h API: add get_tree_entry_type() > > Add a get_tree_entry_type() helper function to compliment the existing > - get_tree_entry(). Move those users of get_tree_entry_type() who didn't > - care about the mode specifically, but just want to know whether the > - tree entry is one of OBJ_{BLOB,COMMIT,TREE} over to it. > + get_tree_entry(), and a static get_tree_entry_all() which it uses internally. > + > + Move those users of get_tree_entry_type() who didn't care about the > + mode specifically, but just want to know whether the tree entry is one > + of OBJ_{BLOB,COMMIT,TREE} over to the new get_tree_entry_type(). > + > + The get_tree_entry_all() function itself will be made non-static in a > + subsequent commit. I'm leaving its argument list indented accordingly > + to reduce churn when I do so. > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > > @@ archive.c: static void parse_treeish_arg(const char **argv, > > tree = parse_tree_indirect(&tree_oid); > > - ## blame.c ## > -@@ blame.c: static void verify_working_tree_path(struct repository *r, > - for (parents = work_tree->parents; parents; parents = parents->next) { > - const struct object_id *commit_oid = &parents->item->object.oid; > - struct object_id blob_oid; > -- unsigned short mode; > -- int ret = get_tree_entry_mode(r, commit_oid, path, &blob_oid, > -- &mode); > -+ enum object_type object_type; > -+ int ret = get_tree_entry_type(r, commit_oid, path, &blob_oid, > -+ &object_type); > - > -- if (!ret && oid_object_info(r, &blob_oid, NULL) == OBJ_BLOB) > -+ if (!ret && object_type == OBJ_BLOB) > - return; > - } > - > - > ## match-trees.c ## > @@ match-trees.c: void shift_tree_by(struct repository *r, > const char *shift_prefix) > @@ match-trees.c: void shift_tree_by(struct repository *r, > > ## tree-walk.c ## > @@ tree-walk.c: struct dir_state { > + struct object_id oid; > + }; > > ++static int get_tree_entry_all(struct repository *r, > ++ const struct object_id *tree_oid, > ++ const char *name, > ++ struct object_id *oid, > ++ unsigned short *mode, > ++ enum object_type *object_type); > ++ > static int find_tree_entry(struct repository *r, struct tree_desc *t, > const char *name, struct object_id *result, > - unsigned short *mode) > @@ tree-walk.c: static int find_tree_entry(struct repository *r, struct tree_desc * > - const char *name, > - struct object_id *oid, > - unsigned short *mode) > -+int get_tree_entry_all(struct repository *r, > ++static int get_tree_entry_all(struct repository *r, > + const struct object_id *tree_oid, > + const char *name, > + struct object_id *oid, > @@ tree-walk.h: struct traverse_info { > - * The third and fourth parameters are set to the entry's sha1 and > - * mode respectively. > + * There are variants of this function depending on what fields in the > -+ * "struct name_entry" you'd like. You always need to pointer to an > ++ * "struct name_entry" you'd like. You always need a pointer to an > + * appropriate variable to fill in (NULL won't do!): > + * > + * get_tree_entry_mode(): unsigned int mode > + * get_tree_entry_type(): enum object_type > -+ * get_tree_entry_all(): unsigned int mode, enum object_type > */ > int get_tree_entry_mode(struct repository *, const struct object_id *, const char *, > struct object_id *, > @@ tree-walk.h: struct traverse_info { > +int get_tree_entry_type(struct repository *, const struct object_id *, const char *, > + struct object_id *, > + enum object_type *); > -+int get_tree_entry_all(struct repository *, const struct object_id *, const char *, > -+ struct object_id *, > -+ unsigned short *, enum object_type *); > > /** > * Generate the full pathname of a tree entry based from the root of the > 24: 5986f494aa1 ! 23: 304d5d4d1af tree-walk.h API: document and format tree_entry_extract() > @@ tree-walk.h: struct tree_desc { > - * `pathp` and `modep` arguments are set to the entry's pathname and mode > - * respectively. > + * `tree_desc's` `entry` member) and return the OID of the entry. > -+ > ++ * > + * There are variants of this function depending on what fields in the > -+ * "struct name_entry" you'd like. You always need to pointer to an > ++ * "struct name_entry" you'd like. You always need a pointer to an > + * appropriate variable to fill in (NULL won't do!): > + * > + * tree_entry_extract_mode(): const char *path, unsigned int mode > 25: 9b604a193b8 ! 24: 346453df356 tree-entry.h API: rename tree_entry_extract() to tree_entry_extract_mode() > @@ tree-diff.c: static struct combine_diff_path *emit_path(struct combine_diff_path > > isdir = S_ISDIR(mode); > > - ## tree-walk.c ## > -@@ tree-walk.c: static int find_tree_entry(struct repository *r, struct tree_desc *t, > - struct object_id oid; > - int entrylen, cmp; > - > -- oidcpy(&oid, tree_entry_extract(t, &entry, mode)); > -+ oidcpy(&oid, tree_entry_extract_mode(t, &entry, mode)); > - entrylen = tree_entry_len(&t->entry); > - update_tree_entry(t); > - if (entrylen > namelen) > - > ## tree-walk.h ## > @@ tree-walk.h: struct tree_desc { > * > 26: 40878d04550 ! 25: dd012b661e5 tree-walk.h API: add a tree_entry_extract_all() function > @@ Commit message > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > > - ## builtin/update-index.c ## > -@@ builtin/update-index.c: static struct cache_entry *read_one_ent(const char *which, > - struct object_id *ent, const char *path, > - int namelen, int stage) > - { > -+ enum object_type object_type; > - unsigned short mode; > - struct object_id oid; > - struct cache_entry *ce; > - > -- if (get_tree_entry_mode(the_repository, ent, path, &oid, &mode)) { > -+ if (get_tree_entry_all(the_repository, ent, path, &oid, > -+ &mode, &object_type)) { > - if (which) > - error("%s: not in %s branch.", path, which); > - return NULL; > - } > -- if (mode == S_IFDIR) { > -+ if (object_type == OBJ_TREE) { > - if (which) > - error("%s: not a blob in %s branch.", path, which); > - return NULL; > - > ## tree-diff.c ## > @@ tree-diff.c: static struct combine_diff_path *emit_path(struct combine_diff_path *p, > assert(t || tp); > @@ tree-diff.c: static struct combine_diff_path *emit_path(struct combine_diff_path > + oid = tree_entry_extract_all(t, &path, &mode, &object_type); > pathlen = tree_entry_len(&t->entry); > - isdir = S_ISDIR(mode); > -+ isdir = object_type == OBJ_TREE; > ++ isdir = (object_type == OBJ_TREE); > } else { > /* > * a path was removed - take path from imin parent. Also take > @@ tree-walk.c: static int find_tree_entry(struct repository *r, struct tree_desc * > struct object_id oid; > int entrylen, cmp; > > -- oidcpy(&oid, tree_entry_extract_mode(t, &entry, mode)); > +- oidcpy(&oid, tree_entry_extract(t, &entry, mode)); > + oidcpy(&oid, tree_entry_extract_all(t, &entry, mode, object_type)); > -+ > entrylen = tree_entry_len(&t->entry); > update_tree_entry(t); > if (entrylen > namelen) > -: ----------- > 26: b6ee8410e38 tree-walk.h API: add get_tree_entry_all() > 23: 6b864e066d9 ! 27: 5c98afd9e7a tree-walk.h API: add a get_tree_entry_path() function > @@ tree-walk.c: int get_tree_entry_all(struct repository *r, > > ## tree-walk.h ## > @@ tree-walk.h: struct traverse_info { > - * "struct name_entry" you'd like. You always need to pointer to an > + * "struct name_entry" you'd like. You always need a pointer to an > * appropriate variable to fill in (NULL won't do!): > * > + * get_tree_entry_path(): <no extra argument, just get the common 'path'> > -: ----------- > 28: 3e7e0f7eb85 blame: emit a better error on 'git blame directory' > 27: e4a6fae1ae0 = 29: ac1ccf13570 tree-walk.h API: add a tree_entry_extract_type() function > 28: 766b4460a95 < -: ----------- tree-walk.h API users: rename "struct name_entry"'s "mode" to "raw_mode" > 29: 4bdf94ae5c1 < -: ----------- tree.h API users: rename read_tree_fn_t's "mode" to "raw_mode" > 30: 9d049fdbd00 < -: ----------- tree-walk.h API: move canon_mode() back out of decode_tree_entry() > -- > 2.31.0.rc2.211.g1d0b8788b3 >
On Tue, Mar 16 2021, Elijah Newren wrote: > On Mon, Mar 15, 2021 at 7:13 PM Ævar Arnfjörð Bjarmason > <avarab@gmail.com> wrote: >> >> A v2 of the big tree-walk.[ch] refactoring series, goals etc. at the >> v1 at: >> https://lore.kernel.org/git/20210308150650.18626-1-avarab@gmail.com/ >> >> It is based on my just-re-rolled v3 read_tree_recursive() series: >> https://lore.kernel.org/git/20210315234344.28427-1-avarab@gmail.com/ >> >> This version should address all the feedback on v1 for patces 1-27/30, >> thanks to Elijah for very valuable comments on v1. >> >> It's mostly small nits here and there, except: >> >> - I found that the change I'd made to update-index.c was buggy at the >> point it was introduced in the series, the object_type would be >> uninitialized. There were/are no tests for that, but I've moved >> things around so we don't have that bug anymore. >> >> - Elijah had a comment on whether we needed oid_object_info() in >> blame.c. As it turns out we don't need a "is blob?" check at all >> there. There's a new 28/29 to refactor that small part of blame.c, >> along with a test. >> >> What this re-roll *does not* include is the final 28-30/30 part of v1 >> to s/mode/raw_mode/g and move canonical_mode() out of tree-walk.h. >> >> So a follow-up series will still be needed to fix the fsck.c check for >> bad modes, but I wanted to split that tricker change off from this >> rather big initial refactoring. > > Splitting those trickier bits off makes sense. > > This series looks really good and addresses most of my feedback from > v1. There were two commit message wording comments in patches 21 and > 27 leftover from the previous round, and a nasty bug introduced in > patch 6 still left from the previous round. > > I also had a question on the new testcase in patch 28. > > Other than that, though, the series looks good to me. Sorry about that, and thanks for spotting those. For what it's worth I *did* actually address all your feedback from the last round and would have prominently noted if I'd left the issue you noted in v1's 6/30 in v2. The problem is that version never made it out of my local tree. I was rebasing this in multiple passes, and I think the immediate culprit is that I've recently aliased "git commit --amend" to "git ca" and "git rebase --continue" as "git rc" as a UI experiment. I then managed to mix the two up, so I aborted during the middle of one of my rebase passes, then when I came back to the computer managed to forget what state I should be in, knowing I'd dealt with 06/30 already etc., and happily continued. And then in trying to re-remember how to massage git-format-patch to apply the correct range-diff invocation option I managed to miss that before sending it out. Anyway. I'm just noting the details of that local screwup I think it's important to accurately summarize changes from the last iteration in CL or patches when getting feedback like that. I'll go over your v1+v2 feedback again and submit a v3 soon. It's taking me a bit longer than I'd have wanted because of course Murphy's law dictates that the first thing I got in my git.git this morning was a warning about too many loose objects, and before having read my E-Mail I thought "do I have anything outstanding but un-referenced?", "no!", and ran "git gc --prune=now". >> >> Ævar Arnfjörð Bjarmason (29): >> diff.c: remove redundant canon_mode() call >> notes & match-trees: use name_entry's "pathlen" member >> cache.h: add a comment to object_type() >> tree-walk.h: add object_type member to name_entry >> tree-walk.c: migrate to using new "object_type" field when possible >> cache.h: have base_name_compare() take "is tree?", not "mode" >> tree-walk.h users: switch object_type(...) to new .object_type >> tree.h: format argument lists of read_tree_recursive() users >> tree.h users: format argument lists in archive.c >> archive: get rid of 'stage' parameter >> tree.h API: make read_tree_fn_t take an "enum object_type" >> tree-walk.h users: migrate "p->mode &&" pattern >> tree-walk.h users: refactor chained "mode" if/else into switch >> tree-walk.h users: migrate miscellaneous "mode" to "object_type" >> merge-tree tests: test for the mode comparison in same_entry() >> merge-ort: correct reference to test in 62fdec17a11 >> fsck.c: switch on "object_type" in fsck_walk_tree() >> tree-walk.h users: use temporary variable(s) for "mode" >> tree-walk.h API: formatting changes for subsequent commit >> tree-walk.h API: rename get_tree_entry() to get_tree_entry_mode() >> tree-walk.h API users: use "tmp" for mode in shift_tree_by() >> tree-walk.h API: add get_tree_entry_type() >> tree-walk.h API: document and format tree_entry_extract() >> tree-entry.h API: rename tree_entry_extract() to >> tree_entry_extract_mode() >> tree-walk.h API: add a tree_entry_extract_all() function >> tree-walk.h API: add get_tree_entry_all() >> tree-walk.h API: add a get_tree_entry_path() function >> blame: emit a better error on 'git blame directory' >> tree-walk.h API: add a tree_entry_extract_type() function >> >> archive.c | 50 +++++++++--------- >> blame.c | 9 ++-- >> builtin/checkout.c | 6 ++- >> builtin/fast-import.c | 8 +-- >> builtin/grep.c | 6 +-- >> builtin/log.c | 7 +-- >> builtin/ls-files.c | 6 ++- >> builtin/ls-tree.c | 14 +++--- >> builtin/merge-tree.c | 30 +++++++---- >> builtin/mktree.c | 4 +- >> builtin/pack-objects.c | 6 +-- >> builtin/reflog.c | 3 +- >> builtin/rm.c | 2 +- >> builtin/update-index.c | 6 ++- >> cache-tree.c | 2 +- >> cache.h | 11 ++-- >> combine-diff.c | 8 +-- >> delta-islands.c | 2 +- >> diff.c | 2 +- >> fsck.c | 23 ++++----- >> http-push.c | 6 ++- >> line-log.c | 2 +- >> list-objects.c | 20 +++++--- >> match-trees.c | 52 +++++++++---------- >> merge-ort.c | 13 ++--- >> merge-recursive.c | 33 ++++++------ >> notes.c | 14 +++--- >> object-name.c | 7 ++- >> pack-bitmap-write.c | 8 +-- >> read-cache.c | 16 +++--- >> revision.c | 12 +++-- >> t/t4300-merge-tree.sh | 44 ++++++++++++++++ >> t/t8004-blame-with-conflicts.sh | 20 ++++++++ >> tree-diff.c | 30 +++++++---- >> tree-walk.c | 89 ++++++++++++++++++++++++--------- >> tree-walk.h | 63 ++++++++++++++++++++--- >> tree.c | 19 ++++--- >> tree.h | 5 +- >> unpack-trees.c | 24 +++++---- >> walker.c | 22 ++++---- >> 40 files changed, 460 insertions(+), 244 deletions(-) >> >> Range-diff: >> 1: e5df57c3440 = 1: f9bbc30f69f diff.c: remove redundant canon_mode() call >> 2: 8c2500bbf35 = 2: 187fc2c3e64 notes & match-trees: use name_entry's "pathlen" member >> 3: 3d98e0c132f ! 3: 311637c5583 cache.h: add a comment to object_type() >> @@ Commit message >> cache.h: add a comment to object_type() >> >> Add a comment to the object_type() function to explain what it >> - returns, and whet the "mode" is in the "else" case. >> + returns, and what the "mode" is in the "else" case. >> >> The object_type() function dates back to 4d1012c3709 (Fix rev-list >> when showing objects involving submodules, 2007-11-11). It's not >> 4: ce5808b317c = 4: fecfe3d462c tree-walk.h: add object_type member to name_entry >> 5: 18f26531acf = 5: db961ab5e8d tree-walk.c: migrate to using new "object_type" field when possible >> 6: 55e2640b815 ! 6: df2fc76161d cache.h: have base_name_compare() take "is tree?", not "mode" >> @@ cache.h: int repo_interpret_branch_name(struct repository *r, >> >> -int base_name_compare(const char *name1, int len1, int mode1, const char *name2, int len2, int mode2); >> -int df_name_compare(const char *name1, int len1, int mode1, const char *name2, int len2, int mode2); >> -+int base_name_compare(const char *name1, int len1, int isdir1, const char *name2, int len2, int isdir2); >> -+int df_name_compare(const char *name1, int len1, int isdir1, const char *name2, int len2, int isdir2); >> ++int base_name_compare(const char *name1, int len1, int istree1, const char *name2, int len2, int istree2); >> ++int df_name_compare(const char *name1, int len1, int istree1, const char *name2, int len2, int istree2); >> 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); >> >> @@ combine-diff.c >> const struct diff_filespec *two) >> { >> - if (!S_ISDIR(one->mode) && !S_ISDIR(two->mode)) >> -+ int isdir_one = S_ISDIR(one->mode); >> -+ int isdir_two = S_ISDIR(two->mode); >> -+ if (!isdir_one && !isdir_two) >> ++ int istree_one = S_ISDIR(one->mode); >> ++ int istree_two = S_ISDIR(two->mode); >> ++ if (!istree_one && !istree_two) >> return strcmp(one->path, two->path); >> >> - return base_name_compare(one->path, strlen(one->path), one->mode, >> - two->path, strlen(two->path), two->mode); >> -+ return base_name_compare(one->path, strlen(one->path), isdir_one, >> -+ two->path, strlen(two->path), isdir_two); >> ++ return base_name_compare(one->path, strlen(one->path), istree_one, >> ++ two->path, strlen(two->path), istree_two); >> } >> >> static int filename_changed(char status) >> @@ match-trees.c: static void *fill_tree_desc_strict(struct tree_desc *desc, >> { >> - return base_name_compare(a->path, tree_entry_len(a), a->mode, >> - b->path, tree_entry_len(b), b->mode); >> -+ int isdira = a->object_type == OBJ_TREE; >> -+ int isdirb = b->object_type == OBJ_TREE; >> -+ return base_name_compare(a->path, tree_entry_len(a), isdira, >> -+ b->path, tree_entry_len(b), isdirb); >> ++ int istree_a = (a->object_type == OBJ_TREE); >> ++ int istree_b = (b->object_type == OBJ_TREE); >> ++ return base_name_compare(a->path, tree_entry_len(a), istree_a, >> ++ b->path, tree_entry_len(b), istree_b); >> } >> >> /* >> @@ read-cache.c: int ie_modified(struct index_state *istate, >> >> -int base_name_compare(const char *name1, int len1, int mode1, >> - const char *name2, int len2, int mode2) >> -+int base_name_compare(const char *name1, int len1, int isdir1, >> -+ const char *name2, int len2, int isdir2) >> ++int base_name_compare(const char *name1, int len1, int istree1, >> ++ const char *name2, int len2, int istree2) >> { >> unsigned char c1, c2; >> int len = len1 < len2 ? len1 : len2; >> @@ read-cache.c: int base_name_compare(const char *name1, int len1, int mode1, >> c1 = name1[len]; >> c2 = name2[len]; >> - if (!c1 && S_ISDIR(mode1)) >> -+ if (!c1 && isdir1) >> ++ if (!c1 && istree1) >> c1 = '/'; >> - if (!c2 && S_ISDIR(mode2)) >> -+ if (!c2 && isdir2) >> ++ if (!c2 && istree2) >> c2 = '/'; >> return (c1 < c2) ? -1 : (c1 > c2) ? 1 : 0; >> } >> @@ read-cache.c: int base_name_compare(const char *name1, int len1, int mode1, >> */ >> -int df_name_compare(const char *name1, int len1, int mode1, >> - const char *name2, int len2, int mode2) >> -+int df_name_compare(const char *name1, int len1, int isdir1, >> -+ const char *name2, int len2, int isdir2) >> ++int df_name_compare(const char *name1, int len1, int istree1, >> ++ const char *name2, int len2, int istree2) >> { >> int len = len1 < len2 ? len1 : len2, cmp; >> unsigned char c1, c2; >> @@ read-cache.c: int df_name_compare(const char *name1, int len1, int mode1, >> return 0; >> c1 = name1[len]; >> - if (!c1 && S_ISDIR(mode1)) >> -+ if (!c1 && isdir1) >> ++ if (!c1 && istree1) >> c1 = '/'; >> c2 = name2[len]; >> - if (!c2 && S_ISDIR(mode2)) >> -+ if (!c2 && isdir2) >> ++ if (!c2 && istree2) >> c2 = '/'; >> if (c1 == '/' && !c2) >> return 0; >> @@ tree-diff.c: static int tree_entry_pathcmp(struct tree_desc *t1, struct tree_des >> { >> struct name_entry *e1, *e2; >> int cmp; >> -+ int e1_is_tree, e2_is_tree; >> ++ int istree_e1, istree_e2; >> >> /* empty descriptors sort after valid tree entries */ >> if (!t1->size) >> @@ tree-diff.c: static int tree_entry_pathcmp(struct tree_desc *t1, struct tree_des >> return -1; >> >> e1 = &t1->entry; >> -+ e1_is_tree = e1->object_type == OBJ_TREE; >> ++ istree_e1 = (e1->object_type == OBJ_TREE); >> e2 = &t2->entry; >> - cmp = base_name_compare(e1->path, tree_entry_len(e1), e1->mode, >> - e2->path, tree_entry_len(e2), e2->mode); >> -+ e2_is_tree = e2->object_type == OBJ_TREE; >> -+ cmp = base_name_compare(e1->path, tree_entry_len(e1), e1_is_tree, >> -+ e2->path, tree_entry_len(e2), e2_is_tree); >> ++ istree_e2 = (e2->object_type == OBJ_TREE); >> ++ cmp = base_name_compare(e1->path, tree_entry_len(e1), istree_e1, >> ++ e2->path, tree_entry_len(e2), istree_e2); >> return cmp; >> } >> >> @@ unpack-trees.c: static int traverse_trees_recursive(int n, unsigned long dirmask >> const struct traverse_info *info, >> const char *name, size_t namelen, >> - unsigned mode) >> -+ unsigned is_tree) >> ++ unsigned istree) >> { >> int pathlen, ce_len; >> const char *ce_name; >> @@ unpack-trees.c: static int do_compare_entry_piecewise(const struct cache_entry * >> ce_name = ce->name + pathlen; >> >> - return df_name_compare(ce_name, ce_len, S_IFREG, name, namelen, mode); >> -+ return df_name_compare(ce_name, ce_len, 0, name, namelen, is_tree); >> ++ return df_name_compare(ce_name, ce_len, 0, name, namelen, istree); >> } >> >> static int do_compare_entry(const struct cache_entry *ce, >> const struct traverse_info *info, >> const char *name, size_t namelen, >> - unsigned mode) >> -+ unsigned is_tree) >> ++ unsigned istree) >> { >> int pathlen, ce_len; >> const char *ce_name; >> @@ unpack-trees.c: static int do_compare_entry(const struct cache_entry *ce, >> */ >> if (!info->traverse_path) >> - return do_compare_entry_piecewise(ce, info, name, namelen, mode); >> -+ return do_compare_entry_piecewise(ce, info, name, namelen, is_tree); >> ++ return do_compare_entry_piecewise(ce, info, name, namelen, istree); >> >> cmp = strncmp(ce->name, info->traverse_path, info->pathlen); >> if (cmp) >> @@ unpack-trees.c: static int do_compare_entry(const struct cache_entry *ce, >> ce_name = ce->name + pathlen; >> >> - return df_name_compare(ce_name, ce_len, S_IFREG, name, namelen, mode); >> -+ return df_name_compare(ce_name, ce_len, 0, name, namelen, is_tree); >> ++ return df_name_compare(ce_name, ce_len, 0, name, namelen, istree); >> } >> >> static int compare_entry(const struct cache_entry *ce, const struct traverse_info *info, const struct name_entry *n) >> { >> - int cmp = do_compare_entry(ce, info, n->path, n->pathlen, n->mode); >> -+ int is_tree = n->object_type == OBJ_TREE; >> -+ int cmp = do_compare_entry(ce, info, n->path, n->pathlen, is_tree); >> ++ int istree = (n->object_type == OBJ_TREE); >> ++ int cmp = do_compare_entry(ce, info, n->path, n->pathlen, istree); >> if (cmp) >> return cmp; >> >> 7: abc128f6cb9 = 7: 49d5da8c086 tree-walk.h users: switch object_type(...) to new .object_type >> 8: dcf13faf3cd ! 8: c9d209d496a tree.h: format argument lists of read_tree_recursive() users >> @@ archive.c: static int check_attr_export_subst(const struct attr_check *check) >> - void *context) >> + int baselen, const char *filename, >> + unsigned mode, >> -+ int stage, void *context) >> ++ int stage, >> ++ void *context) >> { >> static struct strbuf path = STRBUF_INIT; >> struct archiver_context *c = context; >> @@ tree.h: struct tree *parse_tree_indirect(const struct object_id *oid); >> + unsigned int, >> + void *); >> >> - int read_tree_recursive(struct repository *r, >> - struct tree *tree, >> + int read_tree_at(struct repository *r, >> + struct tree *tree, >> 9: b33fcf82349 ! 9: a6d2660fe14 tree.h users: format argument lists in archive.c >> @@ Commit message >> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> >> >> ## archive.c ## >> -@@ archive.c: static int check_attr_export_subst(const struct attr_check *check) >> - static int write_archive_entry(const struct object_id *oid, const char *base, >> - int baselen, const char *filename, >> - unsigned mode, >> -- int stage, void *context) >> -+ int stage, >> -+ void *context) >> - { >> - static struct strbuf path = STRBUF_INIT; >> - struct archiver_context *c = context; >> @@ archive.c: static int write_archive_entry(const struct object_id *oid, const char *base, >> } >> >> 10: 6a20d3c058f = 10: 15f7f89acca archive: get rid of 'stage' parameter >> 11: a7f7444917c ! 11: 7a71404ea3f tree.h API: make read_tree_fn_t take an "enum object_type" >> @@ merge-recursive.c: static int save_files_dirs(const struct object_id *oid, >> static void get_files_dirs(struct merge_options *opt, struct tree *tree) >> >> ## tree.c ## >> -@@ tree.c: static int read_tree_1(struct repository *r, >> +@@ tree.c: int read_tree_at(struct repository *r, >> init_tree_desc(&desc, tree->buffer, tree->size); >> >> while (tree_entry(&desc, &entry)) { >> @@ tree.c: static int read_tree_1(struct repository *r, >> if (retval != all_entries_interesting) { >> retval = tree_entry_interesting(r->index, &entry, >> base, 0, pathspec); >> -@@ tree.c: static int read_tree_1(struct repository *r, >> +@@ tree.c: int read_tree_at(struct repository *r, >> } >> >> switch (fn(&entry.oid, base, >> @@ tree.c: static int read_tree_1(struct repository *r, >> case 0: >> continue; >> case READ_TREE_RECURSIVE: >> -@@ tree.c: static int read_tree_1(struct repository *r, >> +@@ tree.c: int read_tree_at(struct repository *r, >> return -1; >> } >> >> @@ tree.c: static int read_tree_1(struct repository *r, >> commit = lookup_commit(r, &entry.oid); >> if (!commit) >> die("Commit %s in submodule path %s%s not found", >> -@@ tree.c: static int read_tree_1(struct repository *r, >> +@@ tree.c: int read_tree_at(struct repository *r, >> base->buf, entry.path); >> >> oidcpy(&oid, get_commit_tree_oid(commit)); >> @@ tree.h: int cmp_cache_name_compare(const void *a_, const void *b_); >> + enum object_type, unsigned int, >> void *); >> >> - int read_tree_recursive(struct repository *r, >> + int read_tree_at(struct repository *r, >> 12: 625c643513d ! 12: 64dc9364bae tree-walk.h users: migrate "p->mode &&" pattern >> @@ Metadata >> ## Commit message ## >> tree-walk.h users: migrate "p->mode &&" pattern >> >> - Change code that dpends on "p->mode" either being a valid mode or zero >> - to use a p->object_type comparison to "OBJ_NONE". >> + Change code that depends on "p->mode" either being a valid mode or >> + zero to use a p->object_type comparison to "OBJ_NONE". >> >> - The object_type() function in cache.h will not return OBJ_NONE, but in >> - this these API users are implicitly relying on the memzero() that >> - happens in setup_traverse_info(). >> + The object_type() function in cache.h will not return OBJ_NONE, but >> + these API users are implicitly relying on the memzero() that happens >> + in setup_traverse_info(). >> >> Since OBJ_NONE is "0" we can also rely on that being zero'd out here, >> along with the rest of the structure. I think this is slightly less >> 13: 37b28c7feff = 13: 93ed3edbbd5 tree-walk.h users: refactor chained "mode" if/else into switch >> 14: e0b8ec6e291 = 14: 7aa48aa34c3 tree-walk.h users: migrate miscellaneous "mode" to "object_type" >> 15: 0cd162c43d7 = 15: 3ae81621dcf merge-tree tests: test for the mode comparison in same_entry() >> 16: f8ce666d4a7 = 16: 4249ad5c4de merge-ort: correct reference to test in 62fdec17a11 >> 17: 4963902ba97 = 17: e5e17505dde fsck.c: switch on "object_type" in fsck_walk_tree() >> 18: d74e6778009 = 18: 3f0b884f1fd tree-walk.h users: use temporary variable(s) for "mode" >> 19: d39db486d4e = 19: 174167613bb tree-walk.h API: formatting changes for subsequent commit >> 20: 69eb956b1ab = 20: ec76db613f2 tree-walk.h API: rename get_tree_entry() to get_tree_entry_mode() >> 21: cc56453e600 = 21: 11e34941729 tree-walk.h API users: use "tmp" for mode in shift_tree_by() >> 22: ca9e3b3ad00 ! 22: b31c106557f tree-walk.h API: Add get_tree_entry_type() >> @@ Metadata >> Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com> >> >> ## Commit message ## >> - tree-walk.h API: Add get_tree_entry_type() >> + tree-walk.h API: add get_tree_entry_type() >> >> Add a get_tree_entry_type() helper function to compliment the existing >> - get_tree_entry(). Move those users of get_tree_entry_type() who didn't >> - care about the mode specifically, but just want to know whether the >> - tree entry is one of OBJ_{BLOB,COMMIT,TREE} over to it. >> + get_tree_entry(), and a static get_tree_entry_all() which it uses internally. >> + >> + Move those users of get_tree_entry_type() who didn't care about the >> + mode specifically, but just want to know whether the tree entry is one >> + of OBJ_{BLOB,COMMIT,TREE} over to the new get_tree_entry_type(). >> + >> + The get_tree_entry_all() function itself will be made non-static in a >> + subsequent commit. I'm leaving its argument list indented accordingly >> + to reduce churn when I do so. >> >> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> >> >> @@ archive.c: static void parse_treeish_arg(const char **argv, >> >> tree = parse_tree_indirect(&tree_oid); >> >> - ## blame.c ## >> -@@ blame.c: static void verify_working_tree_path(struct repository *r, >> - for (parents = work_tree->parents; parents; parents = parents->next) { >> - const struct object_id *commit_oid = &parents->item->object.oid; >> - struct object_id blob_oid; >> -- unsigned short mode; >> -- int ret = get_tree_entry_mode(r, commit_oid, path, &blob_oid, >> -- &mode); >> -+ enum object_type object_type; >> -+ int ret = get_tree_entry_type(r, commit_oid, path, &blob_oid, >> -+ &object_type); >> - >> -- if (!ret && oid_object_info(r, &blob_oid, NULL) == OBJ_BLOB) >> -+ if (!ret && object_type == OBJ_BLOB) >> - return; >> - } >> - >> - >> ## match-trees.c ## >> @@ match-trees.c: void shift_tree_by(struct repository *r, >> const char *shift_prefix) >> @@ match-trees.c: void shift_tree_by(struct repository *r, >> >> ## tree-walk.c ## >> @@ tree-walk.c: struct dir_state { >> + struct object_id oid; >> + }; >> >> ++static int get_tree_entry_all(struct repository *r, >> ++ const struct object_id *tree_oid, >> ++ const char *name, >> ++ struct object_id *oid, >> ++ unsigned short *mode, >> ++ enum object_type *object_type); >> ++ >> static int find_tree_entry(struct repository *r, struct tree_desc *t, >> const char *name, struct object_id *result, >> - unsigned short *mode) >> @@ tree-walk.c: static int find_tree_entry(struct repository *r, struct tree_desc * >> - const char *name, >> - struct object_id *oid, >> - unsigned short *mode) >> -+int get_tree_entry_all(struct repository *r, >> ++static int get_tree_entry_all(struct repository *r, >> + const struct object_id *tree_oid, >> + const char *name, >> + struct object_id *oid, >> @@ tree-walk.h: struct traverse_info { >> - * The third and fourth parameters are set to the entry's sha1 and >> - * mode respectively. >> + * There are variants of this function depending on what fields in the >> -+ * "struct name_entry" you'd like. You always need to pointer to an >> ++ * "struct name_entry" you'd like. You always need a pointer to an >> + * appropriate variable to fill in (NULL won't do!): >> + * >> + * get_tree_entry_mode(): unsigned int mode >> + * get_tree_entry_type(): enum object_type >> -+ * get_tree_entry_all(): unsigned int mode, enum object_type >> */ >> int get_tree_entry_mode(struct repository *, const struct object_id *, const char *, >> struct object_id *, >> @@ tree-walk.h: struct traverse_info { >> +int get_tree_entry_type(struct repository *, const struct object_id *, const char *, >> + struct object_id *, >> + enum object_type *); >> -+int get_tree_entry_all(struct repository *, const struct object_id *, const char *, >> -+ struct object_id *, >> -+ unsigned short *, enum object_type *); >> >> /** >> * Generate the full pathname of a tree entry based from the root of the >> 24: 5986f494aa1 ! 23: 304d5d4d1af tree-walk.h API: document and format tree_entry_extract() >> @@ tree-walk.h: struct tree_desc { >> - * `pathp` and `modep` arguments are set to the entry's pathname and mode >> - * respectively. >> + * `tree_desc's` `entry` member) and return the OID of the entry. >> -+ >> ++ * >> + * There are variants of this function depending on what fields in the >> -+ * "struct name_entry" you'd like. You always need to pointer to an >> ++ * "struct name_entry" you'd like. You always need a pointer to an >> + * appropriate variable to fill in (NULL won't do!): >> + * >> + * tree_entry_extract_mode(): const char *path, unsigned int mode >> 25: 9b604a193b8 ! 24: 346453df356 tree-entry.h API: rename tree_entry_extract() to tree_entry_extract_mode() >> @@ tree-diff.c: static struct combine_diff_path *emit_path(struct combine_diff_path >> >> isdir = S_ISDIR(mode); >> >> - ## tree-walk.c ## >> -@@ tree-walk.c: static int find_tree_entry(struct repository *r, struct tree_desc *t, >> - struct object_id oid; >> - int entrylen, cmp; >> - >> -- oidcpy(&oid, tree_entry_extract(t, &entry, mode)); >> -+ oidcpy(&oid, tree_entry_extract_mode(t, &entry, mode)); >> - entrylen = tree_entry_len(&t->entry); >> - update_tree_entry(t); >> - if (entrylen > namelen) >> - >> ## tree-walk.h ## >> @@ tree-walk.h: struct tree_desc { >> * >> 26: 40878d04550 ! 25: dd012b661e5 tree-walk.h API: add a tree_entry_extract_all() function >> @@ Commit message >> >> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> >> >> - ## builtin/update-index.c ## >> -@@ builtin/update-index.c: static struct cache_entry *read_one_ent(const char *which, >> - struct object_id *ent, const char *path, >> - int namelen, int stage) >> - { >> -+ enum object_type object_type; >> - unsigned short mode; >> - struct object_id oid; >> - struct cache_entry *ce; >> - >> -- if (get_tree_entry_mode(the_repository, ent, path, &oid, &mode)) { >> -+ if (get_tree_entry_all(the_repository, ent, path, &oid, >> -+ &mode, &object_type)) { >> - if (which) >> - error("%s: not in %s branch.", path, which); >> - return NULL; >> - } >> -- if (mode == S_IFDIR) { >> -+ if (object_type == OBJ_TREE) { >> - if (which) >> - error("%s: not a blob in %s branch.", path, which); >> - return NULL; >> - >> ## tree-diff.c ## >> @@ tree-diff.c: static struct combine_diff_path *emit_path(struct combine_diff_path *p, >> assert(t || tp); >> @@ tree-diff.c: static struct combine_diff_path *emit_path(struct combine_diff_path >> + oid = tree_entry_extract_all(t, &path, &mode, &object_type); >> pathlen = tree_entry_len(&t->entry); >> - isdir = S_ISDIR(mode); >> -+ isdir = object_type == OBJ_TREE; >> ++ isdir = (object_type == OBJ_TREE); >> } else { >> /* >> * a path was removed - take path from imin parent. Also take >> @@ tree-walk.c: static int find_tree_entry(struct repository *r, struct tree_desc * >> struct object_id oid; >> int entrylen, cmp; >> >> -- oidcpy(&oid, tree_entry_extract_mode(t, &entry, mode)); >> +- oidcpy(&oid, tree_entry_extract(t, &entry, mode)); >> + oidcpy(&oid, tree_entry_extract_all(t, &entry, mode, object_type)); >> -+ >> entrylen = tree_entry_len(&t->entry); >> update_tree_entry(t); >> if (entrylen > namelen) >> -: ----------- > 26: b6ee8410e38 tree-walk.h API: add get_tree_entry_all() >> 23: 6b864e066d9 ! 27: 5c98afd9e7a tree-walk.h API: add a get_tree_entry_path() function >> @@ tree-walk.c: int get_tree_entry_all(struct repository *r, >> >> ## tree-walk.h ## >> @@ tree-walk.h: struct traverse_info { >> - * "struct name_entry" you'd like. You always need to pointer to an >> + * "struct name_entry" you'd like. You always need a pointer to an >> * appropriate variable to fill in (NULL won't do!): >> * >> + * get_tree_entry_path(): <no extra argument, just get the common 'path'> >> -: ----------- > 28: 3e7e0f7eb85 blame: emit a better error on 'git blame directory' >> 27: e4a6fae1ae0 = 29: ac1ccf13570 tree-walk.h API: add a tree_entry_extract_type() function >> 28: 766b4460a95 < -: ----------- tree-walk.h API users: rename "struct name_entry"'s "mode" to "raw_mode" >> 29: 4bdf94ae5c1 < -: ----------- tree.h API users: rename read_tree_fn_t's "mode" to "raw_mode" >> 30: 9d049fdbd00 < -: ----------- tree-walk.h API: move canon_mode() back out of decode_tree_entry() >> -- >> 2.31.0.rc2.211.g1d0b8788b3 >>
A v2 of the big tree-walk.[ch] refactoring series, goals etc. at the v1 at: https://lore.kernel.org/git/20210308150650.18626-1-avarab@gmail.com/ It is based on my just-re-rolled v3 read_tree_recursive() series: https://lore.kernel.org/git/20210315234344.28427-1-avarab@gmail.com/ This version should address all the feedback on v1 for patces 1-27/30, thanks to Elijah for very valuable comments on v1. It's mostly small nits here and there, except: - I found that the change I'd made to update-index.c was buggy at the point it was introduced in the series, the object_type would be uninitialized. There were/are no tests for that, but I've moved things around so we don't have that bug anymore. - Elijah had a comment on whether we needed oid_object_info() in blame.c. As it turns out we don't need a "is blob?" check at all there. There's a new 28/29 to refactor that small part of blame.c, along with a test. What this re-roll *does not* include is the final 28-30/30 part of v1 to s/mode/raw_mode/g and move canonical_mode() out of tree-walk.h. So a follow-up series will still be needed to fix the fsck.c check for bad modes, but I wanted to split that tricker change off from this rather big initial refactoring. Ævar Arnfjörð Bjarmason (29): diff.c: remove redundant canon_mode() call notes & match-trees: use name_entry's "pathlen" member cache.h: add a comment to object_type() tree-walk.h: add object_type member to name_entry tree-walk.c: migrate to using new "object_type" field when possible cache.h: have base_name_compare() take "is tree?", not "mode" tree-walk.h users: switch object_type(...) to new .object_type tree.h: format argument lists of read_tree_recursive() users tree.h users: format argument lists in archive.c archive: get rid of 'stage' parameter tree.h API: make read_tree_fn_t take an "enum object_type" tree-walk.h users: migrate "p->mode &&" pattern tree-walk.h users: refactor chained "mode" if/else into switch tree-walk.h users: migrate miscellaneous "mode" to "object_type" merge-tree tests: test for the mode comparison in same_entry() merge-ort: correct reference to test in 62fdec17a11 fsck.c: switch on "object_type" in fsck_walk_tree() tree-walk.h users: use temporary variable(s) for "mode" tree-walk.h API: formatting changes for subsequent commit tree-walk.h API: rename get_tree_entry() to get_tree_entry_mode() tree-walk.h API users: use "tmp" for mode in shift_tree_by() tree-walk.h API: add get_tree_entry_type() tree-walk.h API: document and format tree_entry_extract() tree-entry.h API: rename tree_entry_extract() to tree_entry_extract_mode() tree-walk.h API: add a tree_entry_extract_all() function tree-walk.h API: add get_tree_entry_all() tree-walk.h API: add a get_tree_entry_path() function blame: emit a better error on 'git blame directory' tree-walk.h API: add a tree_entry_extract_type() function archive.c | 50 +++++++++--------- blame.c | 9 ++-- builtin/checkout.c | 6 ++- builtin/fast-import.c | 8 +-- builtin/grep.c | 6 +-- builtin/log.c | 7 +-- builtin/ls-files.c | 6 ++- builtin/ls-tree.c | 14 +++--- builtin/merge-tree.c | 30 +++++++---- builtin/mktree.c | 4 +- builtin/pack-objects.c | 6 +-- builtin/reflog.c | 3 +- builtin/rm.c | 2 +- builtin/update-index.c | 6 ++- cache-tree.c | 2 +- cache.h | 11 ++-- combine-diff.c | 8 +-- delta-islands.c | 2 +- diff.c | 2 +- fsck.c | 23 ++++----- http-push.c | 6 ++- line-log.c | 2 +- list-objects.c | 20 +++++--- match-trees.c | 52 +++++++++---------- merge-ort.c | 13 ++--- merge-recursive.c | 33 ++++++------ notes.c | 14 +++--- object-name.c | 7 ++- pack-bitmap-write.c | 8 +-- read-cache.c | 16 +++--- revision.c | 12 +++-- t/t4300-merge-tree.sh | 44 ++++++++++++++++ t/t8004-blame-with-conflicts.sh | 20 ++++++++ tree-diff.c | 30 +++++++---- tree-walk.c | 89 ++++++++++++++++++++++++--------- tree-walk.h | 63 ++++++++++++++++++++--- tree.c | 19 ++++--- tree.h | 5 +- unpack-trees.c | 24 +++++---- walker.c | 22 ++++---- 40 files changed, 460 insertions(+), 244 deletions(-) Range-diff: 1: e5df57c3440 = 1: f9bbc30f69f diff.c: remove redundant canon_mode() call 2: 8c2500bbf35 = 2: 187fc2c3e64 notes & match-trees: use name_entry's "pathlen" member 3: 3d98e0c132f ! 3: 311637c5583 cache.h: add a comment to object_type() @@ Commit message cache.h: add a comment to object_type() Add a comment to the object_type() function to explain what it - returns, and whet the "mode" is in the "else" case. + returns, and what the "mode" is in the "else" case. The object_type() function dates back to 4d1012c3709 (Fix rev-list when showing objects involving submodules, 2007-11-11). It's not 4: ce5808b317c = 4: fecfe3d462c tree-walk.h: add object_type member to name_entry 5: 18f26531acf = 5: db961ab5e8d tree-walk.c: migrate to using new "object_type" field when possible 6: 55e2640b815 ! 6: df2fc76161d cache.h: have base_name_compare() take "is tree?", not "mode" @@ cache.h: int repo_interpret_branch_name(struct repository *r, -int base_name_compare(const char *name1, int len1, int mode1, const char *name2, int len2, int mode2); -int df_name_compare(const char *name1, int len1, int mode1, const char *name2, int len2, int mode2); -+int base_name_compare(const char *name1, int len1, int isdir1, const char *name2, int len2, int isdir2); -+int df_name_compare(const char *name1, int len1, int isdir1, const char *name2, int len2, int isdir2); ++int base_name_compare(const char *name1, int len1, int istree1, const char *name2, int len2, int istree2); ++int df_name_compare(const char *name1, int len1, int istree1, const char *name2, int len2, int istree2); 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); @@ combine-diff.c const struct diff_filespec *two) { - if (!S_ISDIR(one->mode) && !S_ISDIR(two->mode)) -+ int isdir_one = S_ISDIR(one->mode); -+ int isdir_two = S_ISDIR(two->mode); -+ if (!isdir_one && !isdir_two) ++ int istree_one = S_ISDIR(one->mode); ++ int istree_two = S_ISDIR(two->mode); ++ if (!istree_one && !istree_two) return strcmp(one->path, two->path); - return base_name_compare(one->path, strlen(one->path), one->mode, - two->path, strlen(two->path), two->mode); -+ return base_name_compare(one->path, strlen(one->path), isdir_one, -+ two->path, strlen(two->path), isdir_two); ++ return base_name_compare(one->path, strlen(one->path), istree_one, ++ two->path, strlen(two->path), istree_two); } static int filename_changed(char status) @@ match-trees.c: static void *fill_tree_desc_strict(struct tree_desc *desc, { - return base_name_compare(a->path, tree_entry_len(a), a->mode, - b->path, tree_entry_len(b), b->mode); -+ int isdira = a->object_type == OBJ_TREE; -+ int isdirb = b->object_type == OBJ_TREE; -+ return base_name_compare(a->path, tree_entry_len(a), isdira, -+ b->path, tree_entry_len(b), isdirb); ++ int istree_a = (a->object_type == OBJ_TREE); ++ int istree_b = (b->object_type == OBJ_TREE); ++ return base_name_compare(a->path, tree_entry_len(a), istree_a, ++ b->path, tree_entry_len(b), istree_b); } /* @@ read-cache.c: int ie_modified(struct index_state *istate, -int base_name_compare(const char *name1, int len1, int mode1, - const char *name2, int len2, int mode2) -+int base_name_compare(const char *name1, int len1, int isdir1, -+ const char *name2, int len2, int isdir2) ++int base_name_compare(const char *name1, int len1, int istree1, ++ const char *name2, int len2, int istree2) { unsigned char c1, c2; int len = len1 < len2 ? len1 : len2; @@ read-cache.c: int base_name_compare(const char *name1, int len1, int mode1, c1 = name1[len]; c2 = name2[len]; - if (!c1 && S_ISDIR(mode1)) -+ if (!c1 && isdir1) ++ if (!c1 && istree1) c1 = '/'; - if (!c2 && S_ISDIR(mode2)) -+ if (!c2 && isdir2) ++ if (!c2 && istree2) c2 = '/'; return (c1 < c2) ? -1 : (c1 > c2) ? 1 : 0; } @@ read-cache.c: int base_name_compare(const char *name1, int len1, int mode1, */ -int df_name_compare(const char *name1, int len1, int mode1, - const char *name2, int len2, int mode2) -+int df_name_compare(const char *name1, int len1, int isdir1, -+ const char *name2, int len2, int isdir2) ++int df_name_compare(const char *name1, int len1, int istree1, ++ const char *name2, int len2, int istree2) { int len = len1 < len2 ? len1 : len2, cmp; unsigned char c1, c2; @@ read-cache.c: int df_name_compare(const char *name1, int len1, int mode1, return 0; c1 = name1[len]; - if (!c1 && S_ISDIR(mode1)) -+ if (!c1 && isdir1) ++ if (!c1 && istree1) c1 = '/'; c2 = name2[len]; - if (!c2 && S_ISDIR(mode2)) -+ if (!c2 && isdir2) ++ if (!c2 && istree2) c2 = '/'; if (c1 == '/' && !c2) return 0; @@ tree-diff.c: static int tree_entry_pathcmp(struct tree_desc *t1, struct tree_des { struct name_entry *e1, *e2; int cmp; -+ int e1_is_tree, e2_is_tree; ++ int istree_e1, istree_e2; /* empty descriptors sort after valid tree entries */ if (!t1->size) @@ tree-diff.c: static int tree_entry_pathcmp(struct tree_desc *t1, struct tree_des return -1; e1 = &t1->entry; -+ e1_is_tree = e1->object_type == OBJ_TREE; ++ istree_e1 = (e1->object_type == OBJ_TREE); e2 = &t2->entry; - cmp = base_name_compare(e1->path, tree_entry_len(e1), e1->mode, - e2->path, tree_entry_len(e2), e2->mode); -+ e2_is_tree = e2->object_type == OBJ_TREE; -+ cmp = base_name_compare(e1->path, tree_entry_len(e1), e1_is_tree, -+ e2->path, tree_entry_len(e2), e2_is_tree); ++ istree_e2 = (e2->object_type == OBJ_TREE); ++ cmp = base_name_compare(e1->path, tree_entry_len(e1), istree_e1, ++ e2->path, tree_entry_len(e2), istree_e2); return cmp; } @@ unpack-trees.c: static int traverse_trees_recursive(int n, unsigned long dirmask const struct traverse_info *info, const char *name, size_t namelen, - unsigned mode) -+ unsigned is_tree) ++ unsigned istree) { int pathlen, ce_len; const char *ce_name; @@ unpack-trees.c: static int do_compare_entry_piecewise(const struct cache_entry * ce_name = ce->name + pathlen; - return df_name_compare(ce_name, ce_len, S_IFREG, name, namelen, mode); -+ return df_name_compare(ce_name, ce_len, 0, name, namelen, is_tree); ++ return df_name_compare(ce_name, ce_len, 0, name, namelen, istree); } static int do_compare_entry(const struct cache_entry *ce, const struct traverse_info *info, const char *name, size_t namelen, - unsigned mode) -+ unsigned is_tree) ++ unsigned istree) { int pathlen, ce_len; const char *ce_name; @@ unpack-trees.c: static int do_compare_entry(const struct cache_entry *ce, */ if (!info->traverse_path) - return do_compare_entry_piecewise(ce, info, name, namelen, mode); -+ return do_compare_entry_piecewise(ce, info, name, namelen, is_tree); ++ return do_compare_entry_piecewise(ce, info, name, namelen, istree); cmp = strncmp(ce->name, info->traverse_path, info->pathlen); if (cmp) @@ unpack-trees.c: static int do_compare_entry(const struct cache_entry *ce, ce_name = ce->name + pathlen; - return df_name_compare(ce_name, ce_len, S_IFREG, name, namelen, mode); -+ return df_name_compare(ce_name, ce_len, 0, name, namelen, is_tree); ++ return df_name_compare(ce_name, ce_len, 0, name, namelen, istree); } static int compare_entry(const struct cache_entry *ce, const struct traverse_info *info, const struct name_entry *n) { - int cmp = do_compare_entry(ce, info, n->path, n->pathlen, n->mode); -+ int is_tree = n->object_type == OBJ_TREE; -+ int cmp = do_compare_entry(ce, info, n->path, n->pathlen, is_tree); ++ int istree = (n->object_type == OBJ_TREE); ++ int cmp = do_compare_entry(ce, info, n->path, n->pathlen, istree); if (cmp) return cmp; 7: abc128f6cb9 = 7: 49d5da8c086 tree-walk.h users: switch object_type(...) to new .object_type 8: dcf13faf3cd ! 8: c9d209d496a tree.h: format argument lists of read_tree_recursive() users @@ archive.c: static int check_attr_export_subst(const struct attr_check *check) - void *context) + int baselen, const char *filename, + unsigned mode, -+ int stage, void *context) ++ int stage, ++ void *context) { static struct strbuf path = STRBUF_INIT; struct archiver_context *c = context; @@ tree.h: struct tree *parse_tree_indirect(const struct object_id *oid); + unsigned int, + void *); - int read_tree_recursive(struct repository *r, - struct tree *tree, + int read_tree_at(struct repository *r, + struct tree *tree, 9: b33fcf82349 ! 9: a6d2660fe14 tree.h users: format argument lists in archive.c @@ Commit message Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> ## archive.c ## -@@ archive.c: static int check_attr_export_subst(const struct attr_check *check) - static int write_archive_entry(const struct object_id *oid, const char *base, - int baselen, const char *filename, - unsigned mode, -- int stage, void *context) -+ int stage, -+ void *context) - { - static struct strbuf path = STRBUF_INIT; - struct archiver_context *c = context; @@ archive.c: static int write_archive_entry(const struct object_id *oid, const char *base, } 10: 6a20d3c058f = 10: 15f7f89acca archive: get rid of 'stage' parameter 11: a7f7444917c ! 11: 7a71404ea3f tree.h API: make read_tree_fn_t take an "enum object_type" @@ merge-recursive.c: static int save_files_dirs(const struct object_id *oid, static void get_files_dirs(struct merge_options *opt, struct tree *tree) ## tree.c ## -@@ tree.c: static int read_tree_1(struct repository *r, +@@ tree.c: int read_tree_at(struct repository *r, init_tree_desc(&desc, tree->buffer, tree->size); while (tree_entry(&desc, &entry)) { @@ tree.c: static int read_tree_1(struct repository *r, if (retval != all_entries_interesting) { retval = tree_entry_interesting(r->index, &entry, base, 0, pathspec); -@@ tree.c: static int read_tree_1(struct repository *r, +@@ tree.c: int read_tree_at(struct repository *r, } switch (fn(&entry.oid, base, @@ tree.c: static int read_tree_1(struct repository *r, case 0: continue; case READ_TREE_RECURSIVE: -@@ tree.c: static int read_tree_1(struct repository *r, +@@ tree.c: int read_tree_at(struct repository *r, return -1; } @@ tree.c: static int read_tree_1(struct repository *r, commit = lookup_commit(r, &entry.oid); if (!commit) die("Commit %s in submodule path %s%s not found", -@@ tree.c: static int read_tree_1(struct repository *r, +@@ tree.c: int read_tree_at(struct repository *r, base->buf, entry.path); oidcpy(&oid, get_commit_tree_oid(commit)); @@ tree.h: int cmp_cache_name_compare(const void *a_, const void *b_); + enum object_type, unsigned int, void *); - int read_tree_recursive(struct repository *r, + int read_tree_at(struct repository *r, 12: 625c643513d ! 12: 64dc9364bae tree-walk.h users: migrate "p->mode &&" pattern @@ Metadata ## Commit message ## tree-walk.h users: migrate "p->mode &&" pattern - Change code that dpends on "p->mode" either being a valid mode or zero - to use a p->object_type comparison to "OBJ_NONE". + Change code that depends on "p->mode" either being a valid mode or + zero to use a p->object_type comparison to "OBJ_NONE". - The object_type() function in cache.h will not return OBJ_NONE, but in - this these API users are implicitly relying on the memzero() that - happens in setup_traverse_info(). + The object_type() function in cache.h will not return OBJ_NONE, but + these API users are implicitly relying on the memzero() that happens + in setup_traverse_info(). Since OBJ_NONE is "0" we can also rely on that being zero'd out here, along with the rest of the structure. I think this is slightly less 13: 37b28c7feff = 13: 93ed3edbbd5 tree-walk.h users: refactor chained "mode" if/else into switch 14: e0b8ec6e291 = 14: 7aa48aa34c3 tree-walk.h users: migrate miscellaneous "mode" to "object_type" 15: 0cd162c43d7 = 15: 3ae81621dcf merge-tree tests: test for the mode comparison in same_entry() 16: f8ce666d4a7 = 16: 4249ad5c4de merge-ort: correct reference to test in 62fdec17a11 17: 4963902ba97 = 17: e5e17505dde fsck.c: switch on "object_type" in fsck_walk_tree() 18: d74e6778009 = 18: 3f0b884f1fd tree-walk.h users: use temporary variable(s) for "mode" 19: d39db486d4e = 19: 174167613bb tree-walk.h API: formatting changes for subsequent commit 20: 69eb956b1ab = 20: ec76db613f2 tree-walk.h API: rename get_tree_entry() to get_tree_entry_mode() 21: cc56453e600 = 21: 11e34941729 tree-walk.h API users: use "tmp" for mode in shift_tree_by() 22: ca9e3b3ad00 ! 22: b31c106557f tree-walk.h API: Add get_tree_entry_type() @@ Metadata Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com> ## Commit message ## - tree-walk.h API: Add get_tree_entry_type() + tree-walk.h API: add get_tree_entry_type() Add a get_tree_entry_type() helper function to compliment the existing - get_tree_entry(). Move those users of get_tree_entry_type() who didn't - care about the mode specifically, but just want to know whether the - tree entry is one of OBJ_{BLOB,COMMIT,TREE} over to it. + get_tree_entry(), and a static get_tree_entry_all() which it uses internally. + + Move those users of get_tree_entry_type() who didn't care about the + mode specifically, but just want to know whether the tree entry is one + of OBJ_{BLOB,COMMIT,TREE} over to the new get_tree_entry_type(). + + The get_tree_entry_all() function itself will be made non-static in a + subsequent commit. I'm leaving its argument list indented accordingly + to reduce churn when I do so. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> @@ archive.c: static void parse_treeish_arg(const char **argv, tree = parse_tree_indirect(&tree_oid); - ## blame.c ## -@@ blame.c: static void verify_working_tree_path(struct repository *r, - for (parents = work_tree->parents; parents; parents = parents->next) { - const struct object_id *commit_oid = &parents->item->object.oid; - struct object_id blob_oid; -- unsigned short mode; -- int ret = get_tree_entry_mode(r, commit_oid, path, &blob_oid, -- &mode); -+ enum object_type object_type; -+ int ret = get_tree_entry_type(r, commit_oid, path, &blob_oid, -+ &object_type); - -- if (!ret && oid_object_info(r, &blob_oid, NULL) == OBJ_BLOB) -+ if (!ret && object_type == OBJ_BLOB) - return; - } - - ## match-trees.c ## @@ match-trees.c: void shift_tree_by(struct repository *r, const char *shift_prefix) @@ match-trees.c: void shift_tree_by(struct repository *r, ## tree-walk.c ## @@ tree-walk.c: struct dir_state { + struct object_id oid; + }; ++static int get_tree_entry_all(struct repository *r, ++ const struct object_id *tree_oid, ++ const char *name, ++ struct object_id *oid, ++ unsigned short *mode, ++ enum object_type *object_type); ++ static int find_tree_entry(struct repository *r, struct tree_desc *t, const char *name, struct object_id *result, - unsigned short *mode) @@ tree-walk.c: static int find_tree_entry(struct repository *r, struct tree_desc * - const char *name, - struct object_id *oid, - unsigned short *mode) -+int get_tree_entry_all(struct repository *r, ++static int get_tree_entry_all(struct repository *r, + const struct object_id *tree_oid, + const char *name, + struct object_id *oid, @@ tree-walk.h: struct traverse_info { - * The third and fourth parameters are set to the entry's sha1 and - * mode respectively. + * There are variants of this function depending on what fields in the -+ * "struct name_entry" you'd like. You always need to pointer to an ++ * "struct name_entry" you'd like. You always need a pointer to an + * appropriate variable to fill in (NULL won't do!): + * + * get_tree_entry_mode(): unsigned int mode + * get_tree_entry_type(): enum object_type -+ * get_tree_entry_all(): unsigned int mode, enum object_type */ int get_tree_entry_mode(struct repository *, const struct object_id *, const char *, struct object_id *, @@ tree-walk.h: struct traverse_info { +int get_tree_entry_type(struct repository *, const struct object_id *, const char *, + struct object_id *, + enum object_type *); -+int get_tree_entry_all(struct repository *, const struct object_id *, const char *, -+ struct object_id *, -+ unsigned short *, enum object_type *); /** * Generate the full pathname of a tree entry based from the root of the 24: 5986f494aa1 ! 23: 304d5d4d1af tree-walk.h API: document and format tree_entry_extract() @@ tree-walk.h: struct tree_desc { - * `pathp` and `modep` arguments are set to the entry's pathname and mode - * respectively. + * `tree_desc's` `entry` member) and return the OID of the entry. -+ ++ * + * There are variants of this function depending on what fields in the -+ * "struct name_entry" you'd like. You always need to pointer to an ++ * "struct name_entry" you'd like. You always need a pointer to an + * appropriate variable to fill in (NULL won't do!): + * + * tree_entry_extract_mode(): const char *path, unsigned int mode 25: 9b604a193b8 ! 24: 346453df356 tree-entry.h API: rename tree_entry_extract() to tree_entry_extract_mode() @@ tree-diff.c: static struct combine_diff_path *emit_path(struct combine_diff_path isdir = S_ISDIR(mode); - ## tree-walk.c ## -@@ tree-walk.c: static int find_tree_entry(struct repository *r, struct tree_desc *t, - struct object_id oid; - int entrylen, cmp; - -- oidcpy(&oid, tree_entry_extract(t, &entry, mode)); -+ oidcpy(&oid, tree_entry_extract_mode(t, &entry, mode)); - entrylen = tree_entry_len(&t->entry); - update_tree_entry(t); - if (entrylen > namelen) - ## tree-walk.h ## @@ tree-walk.h: struct tree_desc { * 26: 40878d04550 ! 25: dd012b661e5 tree-walk.h API: add a tree_entry_extract_all() function @@ Commit message Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> - ## builtin/update-index.c ## -@@ builtin/update-index.c: static struct cache_entry *read_one_ent(const char *which, - struct object_id *ent, const char *path, - int namelen, int stage) - { -+ enum object_type object_type; - unsigned short mode; - struct object_id oid; - struct cache_entry *ce; - -- if (get_tree_entry_mode(the_repository, ent, path, &oid, &mode)) { -+ if (get_tree_entry_all(the_repository, ent, path, &oid, -+ &mode, &object_type)) { - if (which) - error("%s: not in %s branch.", path, which); - return NULL; - } -- if (mode == S_IFDIR) { -+ if (object_type == OBJ_TREE) { - if (which) - error("%s: not a blob in %s branch.", path, which); - return NULL; - ## tree-diff.c ## @@ tree-diff.c: static struct combine_diff_path *emit_path(struct combine_diff_path *p, assert(t || tp); @@ tree-diff.c: static struct combine_diff_path *emit_path(struct combine_diff_path + oid = tree_entry_extract_all(t, &path, &mode, &object_type); pathlen = tree_entry_len(&t->entry); - isdir = S_ISDIR(mode); -+ isdir = object_type == OBJ_TREE; ++ isdir = (object_type == OBJ_TREE); } else { /* * a path was removed - take path from imin parent. Also take @@ tree-walk.c: static int find_tree_entry(struct repository *r, struct tree_desc * struct object_id oid; int entrylen, cmp; -- oidcpy(&oid, tree_entry_extract_mode(t, &entry, mode)); +- oidcpy(&oid, tree_entry_extract(t, &entry, mode)); + oidcpy(&oid, tree_entry_extract_all(t, &entry, mode, object_type)); -+ entrylen = tree_entry_len(&t->entry); update_tree_entry(t); if (entrylen > namelen) -: ----------- > 26: b6ee8410e38 tree-walk.h API: add get_tree_entry_all() 23: 6b864e066d9 ! 27: 5c98afd9e7a tree-walk.h API: add a get_tree_entry_path() function @@ tree-walk.c: int get_tree_entry_all(struct repository *r, ## tree-walk.h ## @@ tree-walk.h: struct traverse_info { - * "struct name_entry" you'd like. You always need to pointer to an + * "struct name_entry" you'd like. You always need a pointer to an * appropriate variable to fill in (NULL won't do!): * + * get_tree_entry_path(): <no extra argument, just get the common 'path'> -: ----------- > 28: 3e7e0f7eb85 blame: emit a better error on 'git blame directory' 27: e4a6fae1ae0 = 29: ac1ccf13570 tree-walk.h API: add a tree_entry_extract_type() function 28: 766b4460a95 < -: ----------- tree-walk.h API users: rename "struct name_entry"'s "mode" to "raw_mode" 29: 4bdf94ae5c1 < -: ----------- tree.h API users: rename read_tree_fn_t's "mode" to "raw_mode" 30: 9d049fdbd00 < -: ----------- tree-walk.h API: move canon_mode() back out of decode_tree_entry()