Re: [PATCH v3] ls-files: introduce "--format" option

[ZheNing Hu <adlternative@gmail.com>]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> 于2022年6月24日周五 21:46写道：
>
>
> On Tue, Jun 21 2022, ZheNing Hu via GitGitGadget wrote:
>
> > From: ZheNing Hu <adlternative@gmail.com>
> > [...]
>
> In my mind almost the entire point of a --format is that you can
> e.g. \0-delimit it, and don't need to do other parsing games.
>
> So this really should be adding just e.g. "%x", not "flags: %x",
>

Yeah, I admit that there really shouldn't use extra formatting here.

> Similarly, let's no have :-delimited fields. First, for a formatted
> number "1656077225:850723245" is just bizarre for %(ctime), let's use
> ".", not ":", so: "1656077225.850723245".
>
> And let's call that %(ctime), then have (which is trivial to add) a
> %(ctime:sec) and %(ctime:nsec), so someone who wants to format this can
> parse it as they please, ditto for mtime.
>
> Looking at your tests it seemed you went down the route of aligning the
> output with the --debug output, which is already pre-formatted. I.e. to
> make what you have here match:
>
>                 printf("  ctime: %u:%u\n", sd->sd_ctime.sec, sd->sd_ctime.nsec);
>                 printf("  mtime: %u:%u\n", sd->sd_mtime.sec, sd->sd_mtime.nsec);
>                 printf("  dev: %u\tino: %u\n", sd->sd_dev, sd->sd_ino);
>                 printf("  uid: %u\tgid: %u\n", sd->sd_uid, sd->sd_gid);
>                 printf("  size: %u\tflags: %x\n", sd->sd_size, ce->ce_flags);
>
> I think that's a mistake, we should be able to emit those individual
> %-specifiers instead, not that line as-is without the " " prefix and
> "\n" suffix.
>

Yeah, agree. But now I just want to delete all atoms from %(ctime) to %(flags),
and let --debug can work with --format.

> > +
> > +     if (format && (show_stage || show_others || show_killed ||
> > +             show_resolve_undo || skipping_duplicates || debug_mode))
> > +                     die(_("ls-files --format cannot used with -s, -o, -k, --resolve-undo, --deduplicate, --debug"));
>
> Use usage_msg_opt() or usage_msg_optf() here instead of die(), and no
> need to include "ls-files " in the message.
>
> See die_for_incompatible_opt4, maybe you can just use that instead? A
> bit painful, but:
>
>     die_for_incompatible_opt4(format, "--format", show_stage, "-s", show_others, "-o", show_killed, "-k");
>     die_for_incompatible_opt4(format, "--format", show_resolve_undo, "--resolve-undo", skipping_duplicates, "--deduplicate", debug_mode, "--debug");
>

Good suggestion. I am curious about why there is no function like
die_for_incompatible_opt4() with variable parameters?

> But urgh, that helper really should use usage_msg_opt() instead, but
> using it for now as-is probably sucks less.
>
> I also think we should not forbid combining this wtih --debug, it's
> helpful to construct a format. This seems to work:
>
>         diff --git a/builtin/ls-files.c b/builtin/ls-files.c
>         index 387641b32df..82f13edef7e 100644
>         --- a/builtin/ls-files.c
>         +++ b/builtin/ls-files.c
>         @@ -343,12 +343,17 @@ static void show_ce(struct repository *repo, struct dir_struct *dir,
>                                           S_ISGITLINK(ce->ce_mode))) {
>                         if (format) {
>                                 show_ce_fmt(repo, ce, format, fullname);
>         -                       return;
>         +                       if (!debug_mode)
>         +                               return;
>                         }
>
>                         tag = get_tag(ce, tag);
>
>         -               if (!show_stage) {
>         +               if (format) {
>         +                       if (!debug_mode)
>         +                               BUG("unreachable");
>         +                       ; /* for --debug */
>         +               } else if (!show_stage) {
>                                 fputs(tag, stdout);
>                         } else {
>                                 printf("%s%06o %s %d\t",
>         @@ -814,7 +819,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
>                 }
>
>                 if (format && (show_stage || show_others || show_killed ||
>         -               show_resolve_undo || skipping_duplicates || debug_mode))
>         +               show_resolve_undo || skipping_duplicates))
>                                 die(_("ls-files --format cannot used with -s, -o, -k, --resolve-undo, --deduplicate, --debug"));
>
>                 if (show_tag || show_valid_bit || show_fsmonitor_bit) {
>
> I.e. we'll get:
>
>         $ ./git ls-files --debug --format='<%(flags) %(path)>'  -- po/is.po
>         <flags: 0 po/is.po>
>         po/is.po
>           ctime: 1654300098:369653868
>           mtime: 1654300098:369653868
>           dev: 2306     ino: 10487322
>           uid: 1001     gid: 1001
>           size: 3370    flags: 0
>
> Which I think is quite useful when poking around in this an coming up
> with a format.
>

Maybe something like this will be easier?


@@ -343,6 +335,7 @@ static void show_ce(struct repository *repo,
struct dir_struct *dir,
                                  S_ISGITLINK(ce->ce_mode))) {
                if (format) {
                        show_ce_fmt(repo, ce, format, fullname);
+                       print_debug(ce);
                        return;
                }


> > +
> >       if (show_tag || show_valid_bit || show_fsmonitor_bit) {
> >               tag_cached = "H ";
> >               tag_unmerged = "M ";
> > diff --git a/t/t3013-ls-files-format.sh b/t/t3013-ls-files-format.sh
> > new file mode 100755
> > index 00000000000..8c3ef2df138
> > --- /dev/null
> > +++ b/t/t3013-ls-files-format.sh
> > @@ -0,0 +1,124 @@
> > +#!/bin/sh
> > +
> > +test_description='git ls-files --format test'
> > +
>
> Add this line here:
>
> TEST_PASSES_SANITIZE_LEAK=true
>
> I.e. just before test-lib.sh, see other test examples. Then we'll test
> this under SANITIZE=leak in CI, to ensure it doesn't leak memory.
>
> > +. ./test-lib.sh
> > +
> > +test_expect_success 'setup' '
> > +     echo o1 >o1 &&
> > +     echo o2 >o2 &&
> > +     git add o1 o2 &&
> > +     git add --chmod +x o1 &&
> > +     git commit -m base
> > +'
> > +
> > [...]
>
> > +for flag in -s -o -k --resolve-undo --deduplicate --debug
> > +do
> > +     test_expect_success "git ls-files --format is incompatible with $flag" '
> > +             test_must_fail git ls-files --format="%(objectname)" $flag
> > +     '
> > +done
>
> Nit: I think it's good to move these sotrs of tests before "setup", and
> give them a "usage: " prefix, see some other existing examples.
>

Agree.

> We usually use test_expect_code 129 for those, depending on if you'll
> end up with die() or not...
>
> nit: missing \n before this line:
>
> > +test_done
> >
> > base-commit: ab336e8f1c8009c8b1aab8deb592148e69217085
>

ZheNing Hu