Junio C Hamano writes: > Don't we need some comment that explains what the function does, > what its return value means, etc.? > > It seems that even from its returned value, the caller cannot tell > if *retval was set by the function or not. Perhaps it makes a much > cleaner API to assign 0 to *retval at the beginning of this function, > just like the original did so anyway? ... Oh, sorry for that, I did not notice the "retval" before because the naming is unimpressive and the tests were passed, though... I just looked at it, actually, it's important, not as what it is named, it affects the result. The "retval" actually determine whether to CONTINUE reading the current "tree" or BREAK into the next one [1] . So, I think this commit should be modified despite the tests are passed, firstly, I want to rename "retval" to another name that makes sense, then just make the relevant "if" and "return" logic more clearly with the newname, finally, it'll be consistent with the definitions in "read_tree_at()" at "tree.c" [1]. [1] https://github.com/dyrone/git/blob/master/tree.c#L40 Thanks. Junio C Hamano <gitster@xxxxxxxxx> 于2022年1月4日周二 10:06写道： > > Teng Long <dyroneteng@xxxxxxxxx> writes: > > > > -static int show_tree(const struct object_id *oid, struct strbuf *base, > > - const char *pathname, unsigned mode, void *context) > > +static int show_tree_init(enum object_type *type, struct strbuf *base, > > + const char *pathname, unsigned mode, int *retval) > > Don't we need some comment that explains what the function does, > what its return value means, etc.? > > > { > > - int retval = 0; > > - size_t baselen; > > - enum object_type type = OBJ_BLOB; > > - > > if (S_ISGITLINK(mode)) { > > - type = OBJ_COMMIT; > > + *type = OBJ_COMMIT; > > } else if (S_ISDIR(mode)) { > > if (show_recursive(base->buf, base->len, pathname)) { > > - retval = READ_TREE_RECURSIVE; > > + *retval = READ_TREE_RECURSIVE; > > if (!(ls_options & LS_SHOW_TREES)) > > - return retval; > > + return 1; > > } > > - type = OBJ_TREE; > > + *type = OBJ_TREE; > > } > > else if (ls_options & LS_TREE_ONLY) > > - return 0; > > + return 1; > > + return 0; > > +} > > It seems that even from its returned value, the caller cannot tell > if *retval was set by the function or not. Perhaps it makes a much > cleaner API to assign 0 to *retval at the beginning of this function, > just like the original did so anyway? ... > > > +static int show_tree(const struct object_id *oid, struct strbuf *base, > > + const char *pathname, unsigned mode, void *context) > > +{ > > + int retval = 0; > > ... It would mean we can lose this initialization. > > > + size_t baselen; > > + enum object_type type = OBJ_BLOB; > > + > > + if (show_tree_init(&type, base, pathname, mode, &retval)) > > + return retval; > > > > > if (!(ls_options & LS_NAME_ONLY)) { > > if (ls_options & LS_SHOW_SIZE) {
