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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Tue, Jun 21 2022, ZheNing Hu via GitGitGadget wrote:

> From: ZheNing Hu <adlternative@gmail.com>
> [...]
> +	if (skip_prefix(start, "(objectmode)", &p))
> +		strbuf_addf(sb, "%06o", data->ce->ce_mode);
> +	else if (skip_prefix(start, "(objectname)", &p))
> +		strbuf_add_unique_abbrev(sb, &data->ce->oid, abbrev);
> +	else if (skip_prefix(start, "(stage)", &p))
> +		strbuf_addf(sb, "%d", ce_stage(data->ce));
> +	else if (skip_prefix(start, "(eol)", &p))
> +		write_eolinfo_to_buf(sb, data->istate,
> +				     data->ce, data->pathname);
> +	else if (skip_prefix(start, "(path)", &p))
> +		write_name_to_buf(sb, data->pathname);
> +	else if (skip_prefix(start, "(ctime)", &p))
> +		strbuf_addf(sb, "ctime: %u:%u",
> +			    sd->sd_ctime.sec, sd->sd_ctime.nsec);
> +	else if (skip_prefix(start, "(mtime)", &p))
> +		strbuf_addf(sb, "mtime: %u:%u",
> +			    sd->sd_mtime.sec, sd->sd_mtime.nsec);
> +	else if (skip_prefix(start, "(dev)", &p))
> +		strbuf_addf(sb, "dev: %u", sd->sd_dev);
> +	else if (skip_prefix(start, "(ino)", &p))
> +		strbuf_addf(sb, "ino: %u", sd->sd_ino);
> +	else if (skip_prefix(start, "(uid)", &p))
> +		strbuf_addf(sb, "uid: %u", sd->sd_uid);
> +	else if (skip_prefix(start, "(gid)", &p))
> +		strbuf_addf(sb, "gid: %u", sd->sd_gid);
> +	else if (skip_prefix(start, "(size)", &p))
> +		strbuf_addf(sb, "size: %u", sd->sd_size);
> +	else if (skip_prefix(start, "(flags)", &p))
> +		strbuf_addf(sb, "flags: %x", data->ce->ce_flags);


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", 

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.

> +
> +	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");

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.

> +
>  	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.

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