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

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

On Mon, Feb 28 2022, Teng Long wrote:

[Since this had HTML parts it didn't make it to the git ML, quoting it
here in full & replying on-list].

> On Sat, Feb 19, 2022 at 2:04 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>
>  > 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...
>
> I'm not 100 percent sure about it, but I agree with we can just pick one or
> the other.
>
> So, how about: 
>      1. add "unsigned shown_fields"  in "struct show_tree_data"
>      2.  move global "show_fields" to "struct show_tree_data"
>      3. move "parse_show_fields()" to "show_tree()"
>
> diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
> index 293b8f9dfb..92add01ecc 100644
> --- a/builtin/ls-tree.c
> +++ b/builtin/ls-tree.c
> @@ -25,7 +25,6 @@ static int ls_options;
>  static struct pathspec pathspec;
>  static int chomp_prefix;
>  static const char *ls_tree_prefix;
> -static unsigned int shown_fields;
>  #define FIELD_PATH_NAME 1
>  #define FIELD_SIZE (1 << 1)
>  #define FIELD_OBJECT_NAME (1 << 2)
> @@ -40,6 +39,7 @@ struct show_tree_data {
>         const struct object_id *oid;
>         const char *pathname;
>         struct strbuf *base;
> +       unsigned int shown_fields;
>  };
>  
>  static const  char * const ls_tree_usage[] = {
> @@ -55,19 +55,19 @@ enum {
>  
>  static int cmdmode = MODE_UNSPECIFIED;
>  
> -static int parse_shown_fields(void)
> +static int parse_shown_fields(unsigned int *shown_fields)
>  {
>         if (cmdmode == MODE_NAME_ONLY) {
> -               shown_fields = FIELD_PATH_NAME;
> +               *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;
> +               *shown_fields = FIELD_DEFAULT;
>         if (cmdmode == MODE_LONG)
> -               shown_fields = FIELD_LONG_DEFAULT;
> +               *shown_fields = FIELD_LONG_DEFAULT;
>         return 1;
>  }
>  
> @@ -105,7 +105,7 @@ static int show_default(struct show_tree_data *data)
>  {
>         size_t baselen = data->base->len;
>  
> -       if (shown_fields & FIELD_SIZE) {
> +       if (data->shown_fields & FIELD_SIZE) {
>                 char size_text[24];
>                 if (data->type == OBJ_BLOB) {
>                         unsigned long size;
> @@ -137,15 +137,19 @@ static int show_tree(const struct object_id *oid, struct strbuf *base,
>  {
>         int recurse = 0;
>         size_t baselen;
> +       unsigned int shown_fields = 0;
>         enum object_type type = object_type(mode);
> -       struct show_tree_data data = {
> +               struct show_tree_data data = {
>                 .mode = mode,
>                 .type = type,
>                 .oid = oid,
>                 .pathname = pathname,
>                 .base = base,
> +               .shown_fields = shown_fields,
>         };
>  
> +       parse_shown_fields(&shown_fields);
> +
>         if (type == OBJ_TREE && show_recursive(base->buf, base->len, pathname))
>                 recurse = READ_TREE_RECURSIVE;
>         if (type == OBJ_TREE && recurse && !(ls_options & LS_SHOW_TREES))
> @@ -219,8 +223,6 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
>         if (get_oid(argv[0], &oid))
>                 die("Not a valid object name %s", argv[0]);
>  
> -       parse_shown_fields();
> -
>         /*
>          * show_recursive() rolls its own matching code and is
>          * generally ignorant of 'struct pathspec'. The magic mask
>

Yes, this looks good, i.e. these are (I think, I didn't look in much
detail) now all store one place instead of two.

>  > >+#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?
>
>  
> You mean to use "cmdmode" or set "FIELD_DEFAULT" to 0, if the former  I think is a
> similar situation to your last replied paragraph so I will reply to that. Or if the 
> latter, we want them as a bitmask not only a flag,  so  I think "0" here means no fields
> will be shown and "29" is the default bitmask value (also some context metioned in
> https://public-inbox.org/git/xmqqmtlu7bb0.fsf@gitster.g/).

I meant (elaborated on below) that it seemed like this was conflating
"default mode" with "wants to emit size" in the state machine.

I.e. I didn't try rewriting it (or re-visited it now), but it seemed at
the time with some minor changes it could be eliminated, but maybe
not...

>  > let's name this enum type and use it, see e.g. builtin/help.c's "static
>  > enum help_action" for an example.
>
> Totally agree with that. 
> I want to name it "mutx_option", I'm not sure whether we need a better one. 

Sure, I don't think the name matters much since it's now, so whatever
you think is OK.

I just wanted to point out that some existing code makes the same
pattern simpler by relying on the 0-init and having fields starting at
0.

>  > 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?
>
>  
> For current, the answer is YES I think.
>
>  > 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?" ?
>
> Actually, in v3 patch I use "cmdmode" to determine the output fields and the next patches Junio
> suggest using bitmasks for the work (https://public-inbox.org/git/xmqqmtlu7bb0.fsf@gitster.g/).
>
> I didn't understand it at first,  and I thought it might make sense. The cmd_mode is used here to indicate that options are mutually exclusive, which may have the same effect in certain
> cases, such as some short-circuited options (-- name-only).
>
> However, cmd_mode is not a complete representation of what fields we want to output, because some options may not be mutually exclusive but can also be used to change the output (for
> example, we want to add -format or others). If there is a bitmask associated with the output field, we can quickly and explicitly know what to output in the show_tree() phase, without
> thinking about the relationship to cmd_mode or doing a translation of meaning， and at the same time  this will make it easier to adapt to change I think.

*nod*

I'm not quite sure I get what you're aiming for, but I'll look at it
again in any future re-roll & see if I can submit a patch-on-top next
time if I have any comments (if at all worth it). Thanks!