Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> 于2022年6月24日周五 21:46写道：
>
>
> On Tue, Jun 21 2022, ZheNing Hu via GitGitGadget wrote:
>
> > From: ZheNing Hu <adlternative@xxxxxxxxx>
> > [...]
>
> 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
