Re: [PATCH v9 5/9] ls-tree: optimize naming and handling of "return" in show_tree()

Teng Long <dyroneteng@gmail.com> writes:

What this one is doing sounds more like setting the type variable based on the mode bits, and doing only half a job at it. The name "init" does not sound like a good match to what it does.

If we make it a separate function, we probably should add the "else" clause to set *type to OBJ_BLOB there, so that the caller does not say "we'd assume it is BLOB initially, but tweak it based on mode bits".

I.e.

	type = get_type(mode);

where

	static enum object_type get_type(unsigned int mode)
	{
		return (S_ISGITLINK(mode) 
		        ? OBJ_COMMIT
			: S_ISDIR(mode)
			? OBJ_TREE
			: OBJ_BLOB);
	}

or something like that, perhaps? But I think open-coding the whole thing, after losing the "We assume BLOB" initialization, would be much easier to follow, i.e.

	if (S_ISGITLINK(mode))
		type = OBJ_COMMIT;
	else if (S_ISDIR(mode))
		type = OBJ_TREE;
	else
		type = OBJ_BLOB;

without adding init_type() helper function.

This is even less readable. In the original, it was clear that we only call show_recursive() on a path that is a true directory; we now seem to unconditionally make a call to it. Is that intended?

	Side note.  show_recursive() has a confusing name; it does
	not show anything---it only decides if we want to go
	recursive.

At least, losing the "we assume recursive is 0" upfront in the variable declaration and writing

	if (type == OBJ_TREE && show_recursive(...))
		recursive = READ_TREE_RECURSIVE;
	else
		recursive = 0;

here, without introducing init_recursive(), would make it easier to follow. If we really want to add a new function, perhaps

	recursive = get_recursive(type, base, pathname);
 
where

	static int get_recursive(enum object_type type,
				 struct strbuf *base, const char *pathname)
	{
		if (type == OBJ_TREE && show_recursive(...))
			return READ_TREE_RECURSIVE;
		else
			return 0;
	}

but I fail to see the point of doing so; open-coded 4 lines here would make the flow of thought much better to me.

In any case, I think your splitting the original into "this is about type" and "this is about the recursive bit" is a good idea to help making the resulting code easier to follow.

We are looking at an entry that is a directory. We are running in recursive mode. And we are not told to show the directory itself in the output. We skip the rest of the function, which is about to show this single entry. Makes sense.

Negation of a non-zero integer constant is 0, so it is the same as the original that returned 0, but I am not sure if it is enhancing or hurting readability of the code. The user of the value, in tree.c::read_tree_at(), knows that the possible and valid values are 0 and READ_TREE_RECURSIVE, so returning 0 would probably be a better idea. After all, the initializer in the original for the variable definition of "retval" used "0", not "!READ_TREE_RECURSIVE".

The name "recursive" is much more specific than the overly generic "retval". Its value is to be consumed by read_tree_at(), i.e. our caller, to decide if we want it to recurse into the contents of the directory. I would have called it "recurse" (or even "to_recurse"), if I were doing this change, though.