Re: [PATCH v2 06/29] cache.h: have base_name_compare() take "is tree?", not "mode"

[Elijah Newren <newren@gmail.com>]

On Mon, Mar 15, 2021 at 7:13 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
> Change the base_name_compare() API and the related df_name_compare()
> function to take a boolean argument indicating whether the entry is a
> tree or not, instead of having them call S_ISDIR(mode) on their own.
>
> This makes use of the new "object_type" field in the "name_entry".
>
> The API being modified here was originally added way back in
> 958ba6c96eb (Introduce "base_name_compare()" helper function,
> 2005-05-20).
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  builtin/fast-import.c |  8 ++++----
>  builtin/mktree.c      |  4 ++--
>  cache.h               |  4 ++--
>  combine-diff.c        |  8 +++++---
>  match-trees.c         |  6 ++++--
>  merge-ort.c           |  4 ++--
>  merge-recursive.c     |  6 +++---
>  read-cache.c          | 16 ++++++++--------
>  tree-diff.c           |  7 +++++--
>  unpack-trees.c        | 15 ++++++++-------
>  10 files changed, 43 insertions(+), 35 deletions(-)
>
> diff --git a/builtin/fast-import.c b/builtin/fast-import.c
> index dd4d09ceceb..ce4613c1595 100644
> --- a/builtin/fast-import.c
> +++ b/builtin/fast-import.c
> @@ -1288,8 +1288,8 @@ static int tecmp0 (const void *_a, const void *_b)
>         struct tree_entry *a = *((struct tree_entry**)_a);
>         struct tree_entry *b = *((struct tree_entry**)_b);
>         return base_name_compare(
> -               a->name->str_dat, a->name->str_len, a->versions[0].mode,
> -               b->name->str_dat, b->name->str_len, b->versions[0].mode);
> +               a->name->str_dat, a->name->str_len, 1,
> +               b->name->str_dat, b->name->str_len, 1);
>  }
>
>  static int tecmp1 (const void *_a, const void *_b)
> @@ -1297,8 +1297,8 @@ static int tecmp1 (const void *_a, const void *_b)
>         struct tree_entry *a = *((struct tree_entry**)_a);
>         struct tree_entry *b = *((struct tree_entry**)_b);
>         return base_name_compare(
> -               a->name->str_dat, a->name->str_len, a->versions[1].mode,
> -               b->name->str_dat, b->name->str_len, b->versions[1].mode);
> +               a->name->str_dat, a->name->str_len, 1,
> +               b->name->str_dat, b->name->str_len, 1);
>  }

Um...these last two hunks have not changed since the last round.  They
introduce a nasty bug, as mentioned in my review last time and
highlighted in my response on the cover letter.  They need to be
fixed, and more testcases verifying their correct operation should be
added.

This is the kind of thing that makes me really scared about these
refactorings; it's too easy to introduce changes that will write
semi-broken objects in the wild, which we will then have to deal with
for the rest of forever.

>  static void mktree(struct tree_content *t, int v, struct strbuf *b)
> diff --git a/builtin/mktree.c b/builtin/mktree.c
> index 891991b00d6..2c1973229ac 100644
> --- a/builtin/mktree.c
> +++ b/builtin/mktree.c
> @@ -37,8 +37,8 @@ static int ent_compare(const void *a_, const void *b_)
>  {
>         struct treeent *a = *(struct treeent **)a_;
>         struct treeent *b = *(struct treeent **)b_;
> -       return base_name_compare(a->name, a->len, a->mode,
> -                                b->name, b->len, b->mode);
> +       return base_name_compare(a->name, a->len, S_ISDIR(a->mode),
> +                                b->name, b->len, S_ISDIR(b->mode));
>  }
>
>  static void write_tree(struct object_id *oid)
> diff --git a/cache.h b/cache.h
> index ae0c0bef5c2..e38b1e1688c 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1506,8 +1506,8 @@ int repo_interpret_branch_name(struct repository *r,
>
>  int validate_headref(const char *ref);
>
> -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 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);
>
> diff --git a/combine-diff.c b/combine-diff.c
> index 9228aebc16b..ad7058a6f5d 100644
> --- a/combine-diff.c
> +++ b/combine-diff.c
> @@ -16,11 +16,13 @@
>  static int compare_paths(const struct combine_diff_path *one,
>                           const struct diff_filespec *two)
>  {
> -       if (!S_ISDIR(one->mode) && !S_ISDIR(two->mode))
> +       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), istree_one,
> +                                two->path, strlen(two->path), istree_two);
>  }
>
>  static int filename_changed(char status)
> diff --git a/match-trees.c b/match-trees.c
> index 1011357ad0c..a28c19a62a5 100644
> --- a/match-trees.c
> +++ b/match-trees.c
> @@ -67,8 +67,10 @@ static void *fill_tree_desc_strict(struct tree_desc *desc,
>  static int base_name_entries_compare(const struct name_entry *a,
>                                      const struct name_entry *b)
>  {
> -       return base_name_compare(a->path, tree_entry_len(a), a->mode,
> -                                b->path, tree_entry_len(b), b->mode);
> +       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);
>  }
>
>  /*
> diff --git a/merge-ort.c b/merge-ort.c
> index 603d30c5217..4075d13aaab 100644
> --- a/merge-ort.c
> +++ b/merge-ort.c
> @@ -2390,8 +2390,8 @@ static int tree_entry_order(const void *a_, const void *b_)
>
>         const struct merged_info *ami = a->util;
>         const struct merged_info *bmi = b->util;
> -       return base_name_compare(a->string, strlen(a->string), ami->result.mode,
> -                                b->string, strlen(b->string), bmi->result.mode);
> +       return base_name_compare(a->string, strlen(a->string), S_ISDIR(ami->result.mode),
> +                                b->string, strlen(b->string), S_ISDIR(bmi->result.mode));
>  }
>
>  static void write_tree(struct object_id *result_oid,
> diff --git a/merge-recursive.c b/merge-recursive.c
> index 3d9207455b3..1c9b08695a3 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -554,12 +554,12 @@ static int string_list_df_name_compare(const char *one, const char *two)
>          *
>          * To achieve this, we sort with df_name_compare and provide
>          * the mode S_IFDIR so that D/F conflicts will sort correctly.
> -        * We use the mode S_IFDIR for everything else for simplicity,
> +        * We say we have a directory for everything else for simplicity,
>          * since in other cases any changes in their order due to
>          * sorting cause no problems for us.
>          */
> -       int cmp = df_name_compare(one, onelen, S_IFDIR,
> -                                 two, twolen, S_IFDIR);
> +       int cmp = df_name_compare(one, onelen, 1, two, twolen, 1);
> +
>         /*
>          * Now that 'foo' and 'foo/bar' compare equal, we have to make sure
>          * that 'foo' comes before 'foo/bar'.
> diff --git a/read-cache.c b/read-cache.c
> index 1e9a50c6c73..4257fbd8a08 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -462,8 +462,8 @@ int ie_modified(struct index_state *istate,
>         return 0;
>  }
>
> -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 istree1,
> +                     const char *name2, int len2, int istree2)
>  {
>         unsigned char c1, c2;
>         int len = len1 < len2 ? len1 : len2;
> @@ -474,9 +474,9 @@ int base_name_compare(const char *name1, int len1, int mode1,
>                 return cmp;
>         c1 = name1[len];
>         c2 = name2[len];
> -       if (!c1 && S_ISDIR(mode1))
> +       if (!c1 && istree1)
>                 c1 = '/';
> -       if (!c2 && S_ISDIR(mode2))
> +       if (!c2 && istree2)
>                 c2 = '/';
>         return (c1 < c2) ? -1 : (c1 > c2) ? 1 : 0;
>  }
> @@ -491,8 +491,8 @@ int base_name_compare(const char *name1, int len1, int mode1,
>   * This is used by routines that want to traverse the git namespace
>   * but then handle conflicting entries together when possible.
>   */
> -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 istree1,
> +                   const char *name2, int len2, int istree2)
>  {
>         int len = len1 < len2 ? len1 : len2, cmp;
>         unsigned char c1, c2;
> @@ -504,10 +504,10 @@ int df_name_compare(const char *name1, int len1, int mode1,
>         if (len1 == len2)
>                 return 0;
>         c1 = name1[len];
> -       if (!c1 && S_ISDIR(mode1))
> +       if (!c1 && istree1)
>                 c1 = '/';
>         c2 = name2[len];
> -       if (!c2 && S_ISDIR(mode2))
> +       if (!c2 && istree2)
>                 c2 = '/';
>         if (c1 == '/' && !c2)
>                 return 0;
> diff --git a/tree-diff.c b/tree-diff.c
> index 7cebbb327e2..6ec180331fb 100644
> --- a/tree-diff.c
> +++ b/tree-diff.c
> @@ -50,6 +50,7 @@ static int tree_entry_pathcmp(struct tree_desc *t1, struct tree_desc *t2)
>  {
>         struct name_entry *e1, *e2;
>         int cmp;
> +       int istree_e1, istree_e2;
>
>         /* empty descriptors sort after valid tree entries */
>         if (!t1->size)
> @@ -58,9 +59,11 @@ static int tree_entry_pathcmp(struct tree_desc *t1, struct tree_desc *t2)
>                 return -1;
>
>         e1 = &t1->entry;
> +       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);
> +       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;
>  }
>
> diff --git a/unpack-trees.c b/unpack-trees.c
> index eb8fcda31ba..d9d573df986 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -925,7 +925,7 @@ static int traverse_trees_recursive(int n, unsigned long dirmask,
>  static int do_compare_entry_piecewise(const struct cache_entry *ce,
>                                       const struct traverse_info *info,
>                                       const char *name, size_t namelen,
> -                                     unsigned mode)
> +                                     unsigned istree)
>  {
>         int pathlen, ce_len;
>         const char *ce_name;
> @@ -933,7 +933,7 @@ static int do_compare_entry_piecewise(const struct cache_entry *ce,
>         if (info->prev) {
>                 int cmp = do_compare_entry_piecewise(ce, info->prev,
>                                                      info->name, info->namelen,
> -                                                    info->mode);
> +                                                    S_ISDIR(info->mode));
>                 if (cmp)
>                         return cmp;
>         }
> @@ -947,13 +947,13 @@ static int do_compare_entry_piecewise(const struct cache_entry *ce,
>         ce_len -= pathlen;
>         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, 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 istree)
>  {
>         int pathlen, ce_len;
>         const char *ce_name;
> @@ -965,7 +965,7 @@ static int do_compare_entry(const struct cache_entry *ce,
>          * it is quicker to use the precomputed version.
>          */
>         if (!info->traverse_path)
> -               return do_compare_entry_piecewise(ce, info, name, namelen, mode);
> +               return do_compare_entry_piecewise(ce, info, name, namelen, istree);
>
>         cmp = strncmp(ce->name, info->traverse_path, info->pathlen);
>         if (cmp)
> @@ -980,12 +980,13 @@ static int do_compare_entry(const struct cache_entry *ce,
>         ce_len -= pathlen;
>         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, 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 istree = (n->object_type == OBJ_TREE);
> +       int cmp = do_compare_entry(ce, info, n->path, n->pathlen, istree);
>         if (cmp)
>                 return cmp;
>
> --
> 2.31.0.rc2.211.g1d0b8788b3

The isdir -> istree updates are nice; thanks.