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) {
