diff mbox series

[v5,03/18] cache.h: have base_name_compare() take "is tree?", not "mode"

Message ID patch-03.19-44c3fdd0085-20210331T190531Z-avarab@gmail.com (mailing list archive)
State New, archived
Headers show
Series tree-walk.h: slimmed down | expand

Commit Message

Ævar Arnfjörð Bjarmason March 31, 2021, 7:09 p.m. UTC 
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 introduces no functional change, but makes later (not part of
this series) changes to abstract away "object_type" from "mode" more
readable.

The API being modified here was originally added way back in
958ba6c96eb (Introduce "base_name_compare()" helper function,
2005-05-20).

None of these comparison functions used to have tests, but with
preceding commits some of them now do. I thought the remainder was
trivial enough to review without tests, and didn't want to spend more
time on them.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/fast-import.c | 12 ++++++++----
 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, 47 insertions(+), 35 deletions(-)

Comments

Junio C Hamano March 31, 2021, 10:55 p.m. UTC | #1 
Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> 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 introduces no functional change, but makes later (not part of
> this series) changes to abstract away "object_type" from "mode" more
> readable.
>
> The API being modified here was originally added way back in
> 958ba6c96eb (Introduce "base_name_compare()" helper function,
> 2005-05-20).
>
> None of these comparison functions used to have tests, but with
> preceding commits some of them now do. I thought the remainder was
> trivial enough to review without tests, and didn't want to spend more
> time on them.

Puzzled.  preceeding commits?

> diff --git a/cache.h b/cache.h
> index 41e99bd9c63..3bcea022ad2 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);

Hopefully there won't be any new callers to these comparison
functions introduced while this topic is in flight.

Without seeing real users (not yet at this 3rd step in the 18 patch
series, anyway), this step feels to be a needless code churn whose
only effect is to increase risks of silent semantic conflicts that
compilers will not be able to help detecting.  It might make it
slightly less error prone to add

	if (istree1 != 0 && istree1 != 1)
		BUG("%o?  unconverted caller?", istree1);
	if (istree2 != 0 && istree2 != 1)
		BUG("%o?  unconverted caller?", istree2);

in the callee while the topic is still in flight.  Or do this in a
renamed function so that such a semantic conflict will be noticed by
the linker (although that would increase the busywork workload on
the integrator).

I know steps like this in a long series (not limited to this series)
means well, and we do encourage people to move necessary preliminary
clean-up to the early part of a series, but they make me wonder "can
we get to the point without 'clean-up while we are at it' steps that
may turn out to be more-or-less irrelevant?"

In other words, we do encourage people to do necessary preliminary
clean-up before the main part of a series, but it sometimes is
unclear if an early "clean-up" patch in a long series is "necessary"
or merely "while at it" that can be omitted.

But let me hold my judgment until I reach the end of the series to
know enough to say if this step is or is not "necessary" preliminary
clean-up.

> diff --git a/unpack-trees.c b/unpack-trees.c
> index 29029f34ed6..23c1640ae9a 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)

Here, the "istree" boolean is expressed as unsigned, but in cache.h,
it is expressed as int?  Why the discrepancy?
diff mbox series

Patch

diff --git a/builtin/fast-import.c b/builtin/fast-import.c
index 3afa81cf9ac..2fb56d6e9b7 100644
--- a/builtin/fast-import.c
+++ b/builtin/fast-import.c
@@ -1287,18 +1287,22 @@  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);
+	int istree_a = S_ISDIR(a->versions[0].mode);
+	int istree_b = S_ISDIR(b->versions[0].mode);
 	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, istree_a,
+		b->name->str_dat, b->name->str_len, istree_b);
 }
 
 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);
+	int istree_a = S_ISDIR(a->versions[1].mode);
+	int istree_b = S_ISDIR(b->versions[1].mode);
 	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, istree_a,
+		b->name->str_dat, b->name->str_len, istree_b);
 }
 
 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 41e99bd9c63..3bcea022ad2 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 06635f91bc2..155d52971e6 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 f6c194c1cca..48f3e0ed526 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 = S_ISDIR(a->mode);
+	int istree_b = S_ISDIR(b->mode);
+	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 dd1f38e3c32..97b8670c0a5 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -2257,8 +2257,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 ed31f9496cb..97520a88646 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 5a907af2fb5..6e0b41ed175 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..644b4656421 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 = S_ISDIR(e1->mode);
 	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 = S_ISDIR(e2->mode);
+	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 29029f34ed6..23c1640ae9a 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 = S_ISDIR(n->mode);
+	int cmp = do_compare_entry(ce, info, n->path, n->pathlen, istree);
 	if (cmp)
 		return cmp;