On Fri, Jan 7, 2022 at 4:44 AM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> Teng Long <dyroneteng@xxxxxxxxx> 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.
>
> > + 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.
Thanks, will apply in the next patch.
