Re: [PATCH v11 08/13] ls-tree: slightly refactor `show_tree()`

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Tue, Feb 08 2022, Teng Long wrote:

> This is a non-functional change, we use a new int "shown_fields" to mark
> which columns to output, and `parse_shown_fields()` to calculate the
> value of "shown_fields".
>
> This has the advantage of making the show_tree logic simpler and more
> readable, as well as making it easier to extend new options (for example,
> if we want to add a "--object-only" option, we just need to add a similar
> "if (shown_fields == FIELD_OBJECT_NAME)" short-circuit logic in
> "show_tree()").

I think this and the 09/13 really don't make sense in combination.

Now, I clearly prefer to put options for the command into its own little
struct to pass around, I think it makes for easier reading than the
globals you end up with.

But tastes differ, and some built-ins use one, and some the other
pattern.

But this is really the worst of both worlds, let's just pick one or the
other, not effectively some some ptions in that struct in 09/13, and
some in globals here...

> +static unsigned int shown_fields;
> +#define FIELD_PATH_NAME 1
> +#define FIELD_SIZE (1 << 1)
> +#define FIELD_OBJECT_NAME (1 << 2)
> +#define FIELD_TYPE (1 << 3)
> +#define FIELD_MODE (1 << 4)
> +#define FIELD_DEFAULT 29 /* 11101 size is not shown to output by default */

Why do we need some FIELD_DEFAULT here as opposed to just having it by
an enum field with a valu of 0?

> +enum {
> +	MODE_UNSPECIFIED = 0,
> +	MODE_NAME_ONLY,
> +	MODE_LONG,
> +};
> +
> +static int cmdmode = MODE_UNSPECIFIED;

let's name this enum type and use it, see e.g. builtin/help.c's "static
enum help_action" for an example.

> +
> +static int parse_shown_fields(void)
> +{
> +	if (cmdmode == MODE_NAME_ONLY) {
> +		shown_fields = FIELD_PATH_NAME;
> +		return 0;
> +	}
> +
> +	if (!ls_options || (ls_options & LS_RECURSIVE)
> +	    || (ls_options & LS_SHOW_TREES)
> +	    || (ls_options & LS_TREE_ONLY))
> +		shown_fields = FIELD_DEFAULT;
> +	if (cmdmode == MODE_LONG)
> +		shown_fields = FIELD_LONG_DEFAULT;
> +	return 1;
> +}

I still don't really get why we can't just use the one MODE_*
here. E.g. doesn't MODE_LONG map to FIELD_LONG_DEFAULT, MODE_NAME_ONLY
to FIELD_PATH_NAME etc?

Is this all so we can do "shown_fields & FIELD_SIZE" in show_default()
as opposed to e.g. checking "default format or long format?" ?