| Message ID | 41e8ed50476a5afd6009db1a69c7a39b04e038b0.1644319434.git.dyroneteng@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ls-tree: "--object-only" and "--format" opts | expand |
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?" ?
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!
diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c index 32147e75e6..8baab7c83e 100644 --- a/builtin/ls-tree.c +++ b/builtin/ls-tree.c @@ -16,21 +16,53 @@ static int line_termination = '\n'; #define LS_RECURSIVE 1 -#define LS_TREE_ONLY 2 -#define LS_SHOW_TREES 4 -#define LS_NAME_ONLY 8 -#define LS_SHOW_SIZE 16 +#define LS_TREE_ONLY (1 << 1) +#define LS_SHOW_TREES (1 << 2) +#define LS_NAME_ONLY (1 << 3) +#define LS_SHOW_SIZE (1 << 4) static int abbrev; 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) +#define FIELD_TYPE (1 << 3) +#define FIELD_MODE (1 << 4) +#define FIELD_DEFAULT 29 /* 11101 size is not shown to output by default */ +#define FIELD_LONG_DEFAULT (FIELD_DEFAULT | FIELD_SIZE) static const char * const ls_tree_usage[] = { N_("git ls-tree [<options>] <tree-ish> [<path>...]"), NULL }; +enum { + MODE_UNSPECIFIED = 0, + MODE_NAME_ONLY, + MODE_LONG, +}; + +static int cmdmode = MODE_UNSPECIFIED; + +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; +} + static int show_recursive(const char *base, size_t baselen, const char *pathname) { int i; @@ -61,6 +93,39 @@ static int show_recursive(const char *base, size_t baselen, const char *pathname return 0; } +static int show_default(const struct object_id *oid, enum object_type type, + const char *pathname, unsigned mode, + struct strbuf *base) +{ + size_t baselen = base->len; + + if (shown_fields & FIELD_SIZE) { + char size_text[24]; + if (type == OBJ_BLOB) { + unsigned long size; + if (oid_object_info(the_repository, oid, &size) == OBJ_BAD) + xsnprintf(size_text, sizeof(size_text), "BAD"); + else + xsnprintf(size_text, sizeof(size_text), + "%" PRIuMAX, (uintmax_t)size); + } else { + xsnprintf(size_text, sizeof(size_text), "-"); + } + printf("%06o %s %s %7s\t", mode, type_name(type), + find_unique_abbrev(oid, abbrev), size_text); + } else { + printf("%06o %s %s\t", mode, type_name(type), + find_unique_abbrev(oid, abbrev)); + } + baselen = base->len; + strbuf_addstr(base, pathname); + write_name_quoted_relative(base->buf, + chomp_prefix ? ls_tree_prefix : NULL, stdout, + line_termination); + strbuf_setlen(base, baselen); + return 1; +} + static int show_tree(const struct object_id *oid, struct strbuf *base, const char *pathname, unsigned mode, void *context) { @@ -75,34 +140,19 @@ static int show_tree(const struct object_id *oid, struct strbuf *base, if (type == OBJ_BLOB && (ls_options & LS_TREE_ONLY)) return 0; - if (!(ls_options & LS_NAME_ONLY)) { - if (ls_options & LS_SHOW_SIZE) { - char size_text[24]; - if (type == OBJ_BLOB) { - unsigned long size; - if (oid_object_info(the_repository, oid, &size) == OBJ_BAD) - xsnprintf(size_text, sizeof(size_text), - "BAD"); - else - xsnprintf(size_text, sizeof(size_text), - "%"PRIuMAX, (uintmax_t)size); - } else { - xsnprintf(size_text, sizeof(size_text), "-"); - } - printf("%06o %s %s %7s\t", mode, type_name(type), - find_unique_abbrev(oid, abbrev), - size_text); - } else { - printf("%06o %s %s\t", mode, type_name(type), - find_unique_abbrev(oid, abbrev)); - } + if (shown_fields == FIELD_PATH_NAME) { + baselen = base->len; + strbuf_addstr(base, pathname); + write_name_quoted_relative(base->buf, + chomp_prefix ? ls_tree_prefix : NULL, + stdout, line_termination); + strbuf_setlen(base, baselen); + return recurse; } - baselen = base->len; - strbuf_addstr(base, pathname); - write_name_quoted_relative(base->buf, - chomp_prefix ? ls_tree_prefix : NULL, - stdout, line_termination); - strbuf_setlen(base, baselen); + + if (shown_fields >= FIELD_DEFAULT) + show_default(oid, type, pathname, mode, base); + return recurse; } @@ -120,12 +170,12 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix) LS_SHOW_TREES), OPT_SET_INT('z', NULL, &line_termination, N_("terminate entries with NUL byte"), 0), - OPT_CMDMODE('l', "long", &ls_options, N_("include object size"), - LS_SHOW_SIZE), - OPT_CMDMODE(0, "name-only", &ls_options, N_("list only filenames"), - LS_NAME_ONLY), - OPT_CMDMODE(0, "name-status", &ls_options, N_("list only filenames"), - LS_NAME_ONLY), + OPT_CMDMODE('l', "long", &cmdmode, N_("include object size"), + MODE_LONG), + OPT_CMDMODE(0, "name-only", &cmdmode, N_("list only filenames"), + MODE_NAME_ONLY), + OPT_CMDMODE(0, "name-status", &cmdmode, N_("list only filenames"), + MODE_NAME_ONLY), OPT_SET_INT(0, "full-name", &chomp_prefix, N_("use full path names"), 0), OPT_BOOL(0, "full-tree", &full_tree, @@ -156,6 +206,8 @@ 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
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()"). Signed-off-by: Teng Long <dyroneteng@gmail.com> --- builtin/ls-tree.c | 126 ++++++++++++++++++++++++++++++++-------------- 1 file changed, 89 insertions(+), 37 deletions(-)