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

[Junio C Hamano <gitster@pobox.com>]

Æ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?