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@xxxxxxxxx> 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!
