Re: [PATCH v4 00/29] tree-walk: mostly replace "mode" with "enum object_type"

Ævar Arnfjörð Bjarmason  [off-list ref] writes:

A re-roll of v3 of this series[1] and on top of (and requires) my
just-submitted v5 re-roll of the read_tree() refactoring series[2].

There was a regression in 1/32 of the old series. Removing the
canon_mode() call in diff.c didn't account for us needing to
canonicalize "diff --no-index" modes. There were no tests for this,
and it failed or not depending on the FS modes in the git.git checkout
being tested. This fixes the CI smoke coming from this series.

Sorry, but quite honestly, I am not quite sure what value this
entire code churn is trying to add to the codebase.

The function signature of read_tree_fn_t callback function gets
changed from the mode bits (which is capable to express differences
between regular files, executable files and symbolic links) to "enum
object_type" (which can only say "this is a blob"), which is a
regression, no?  

A callback can no longer do things like this, for example:

static int add_path_to_index(const struct object_id *oid,
			     struct strbuf *base, const char *path,
			     unsigned int mode, void *context)
{
	struct index_state *istate = (struct index_state *)context;
	struct cache_entry *ce;
	size_t len = base->len;

	if (S_ISDIR(mode))
		return READ_TREE_RECURSIVE;

	strbuf_addstr(base, path);

	ce = make_cache_entry(istate, mode, oid, base->buf, 0, 0);
	ce->ce_flags |= CE_SKIP_WORKTREE;
	set_index_entry(istate, istate->cache_nr++, ce);

	strbuf_setlen(base, len);
	return 0;
}

where executableness or symlinkshood is lost.

This probably is the third time I caught similar "let's lose
information passed through the call chain as nobody seems to need
it" mistakes in the iterations of this series, and that is two times
too many.  We should learn from our earlier mistakes---tweaking of
the API that happens to be still OK with the current codebase can be
either a needless churn that loses useful expressiveness from the
API, or a useful clean-up to kill dead parameter or excess
flexibility.

And these three incidents we have seen so far are the former.

Thanks.