Re: [PATCH v3] ls-files: introduce "--format" option
- From
ZheNing Hu <adlternative@gmail.com>
- Date
- Jun 26, 2022, 13:34 UTC
- Message-ID
- <CAOLTT8RPrtgcsrrdPwNTcD2WUozThaN_QRYu+JOO2er0F8a8-w@mail.gmail.com>
- In-Reply-To
- <220624.86letmi383.gmgdl@evledraar.gmail.com>
Ævar Arnfjörð Bjarmason <avarab@gmail.com> 于2022年6月24日周五 21:46写道：
Show 12 quoted lines
> > > 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.
Show 22 quoted lines
> 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.
Show 14 quoted lines
> > +
> > + 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?
Show 54 quoted lines
> 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