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

> 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.
>
> > +     init_recursive(base, pathname, &recursive);
>
> 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.
>
> > +     if (type == OBJ_TREE && recursive && !(ls_options & LS_SHOW_TREES))
> > +             return recursive;
>
> 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.
>
>
> > +     if (type == OBJ_BLOB && (ls_options & LS_TREE_ONLY))
> > +             return !READ_TREE_RECURSIVE;
>
> 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.