ls-files: introduce "--format" and "--object-only" options

From: ZheNing Hu <[email protected]>

Add a new option --format that output index enties informations with custom format, taking inspiration from the option with the same name in the git ls-tree command.

--format cannot used with -s, -o, -k, --resolve-undo, --deduplicate, --debug.

Signed-off-by ZheNing Hu <[email protected]>

---
 Documentation/git-ls-files.txt |  51 +++++++++++-
 builtin/ls-files.c             | 126 ++++++++++++++++++++++++++++-
 t/t3013-ls-files-format.sh     | 142 +++++++++++++++++++++++++++++++++
 3 files changed, 315 insertions(+), 4 deletions(-)
 create mode 100755 t/t3013-ls-files-format.sh

Documentation/git-ls-files.txt +1 -1

index 0dabf3f0ddc..b22860ec8c0 100644
@@ -20,7 +20,7 @@ SYNOPSIS
 		[--exclude-standard]
 		[--error-unmatch] [--with-tree=<tree-ish>]
 		[--full-name] [--recurse-submodules]
-		[--abbrev[=<n>]] [--] [<file>...]
+		[--abbrev[=<n>]] [--format=<format>] [--] [<file>...]
 
 DESCRIPTION
 -----------

Documentation/git-ls-files.txt +43 -0

+	`--debug`.
 \--::
 	Do not interpret any more arguments as options.
 

@@ -223,6 +230,48 @@ quoted as explained for the configuration variable `core.quotePath`
 (see linkgit:git-config[1]).  Using `-z` the filename is output
 verbatim and the line is terminated by a NUL byte.
 
+It is possible to print in a custom format by using the `--format`
+option, which is able to interpolate different fields using
+a `%(fieldname)` notation. For example, if you only care about the
+"objectname" and "path" fields, you can execute with a specific
+"--format" like
+
+	git ls-files --format='%(objectname) %(path)'
+
+FIELD NAMES
+-----------
+Various values from structured fields can be used to interpolate
+into the resulting output. For each outputting line, the following
+names can be used:
+
+tag::
+	The tag of file status.
+objectmode::
+	The mode of the object.
+objectname::
+	The name of the object.
+stage::
+	The stage of the file.
+eol::
+	The line endings of files.
+path::
+	The pathname of the object.
+ctime::
+	The create time of file.
+mtime::
+	The modify time of file.
+dev::
+	The ID of device containing file.
+ino::
+	The inode number of file.
+uid::
+	The user id of file owner.
+gid::
+	The group id of file owner.
+size::
+	The size of the file.
+flags::
+	The flags of the file.
 
 EXCLUDE PATTERNS
 ----------------

builtin/ls-files.c +20 -3

index e791b65e7e9..9dd6c55eeb9 100644
@@ -11,6 +11,7 @@
 #include "quote.h"
 #include "dir.h"
 #include "builtin.h"
+#include "strbuf.h"
 #include "tree.h"
 #include "cache-tree.h"
 #include "parse-options.h"

@@ -48,6 +49,7 @@ static char *ps_matched;
 static const char *with_tree;
 static int exc_given;
 static int exclude_args;
+static const char *format;
 
 static const char *tag_cached = "";
 static const char *tag_unmerged = "";

@@ -58,8 +60,8 @@ static const char *tag_modified = "";
 static const char *tag_skip_worktree = "";
 static const char *tag_resolve_undo = "";
 
-static void write_eolinfo(struct index_state *istate,
-			  const struct cache_entry *ce, const char *path)
+static void write_eolinfo_internal(struct strbuf *sb, struct index_state *istate,
+				   const struct cache_entry *ce, const char *path)
 {
 	if (show_eol) {
 		struct stat st;

@@ -71,10 +73,25 @@ static void write_eolinfo(struct index_state *istate,
 							       ce->name);
 		if (!lstat(path, &st) && S_ISREG(st.st_mode))
 			w_txt = get_wt_convert_stats_ascii(path);
-		printf("i/%-5s w/%-5s attr/%-17s\t", i_txt, w_txt, a_txt);
+		if (sb)
+			strbuf_addf(sb, "i/%-5s w/%-5s attr/%-17s\t", i_txt, w_txt, a_txt);
+		else
+			printf("i/%-5s w/%-5s attr/%-17s\t", i_txt, w_txt, a_txt);
 	}
 }
 
+static void write_eolinfo(struct index_state *istate,
+			  const struct cache_entry *ce, const char *path)
+{
+	write_eolinfo_internal(NULL, istate, ce, path);
+}
+
+static void write_eolinfo_to_buf(struct strbuf *sb, struct index_state *istate,
+				 const struct cache_entry *ce, const char *path)
+{
+	write_eolinfo_internal(sb, istate, ce, path);
+}
+
 static void write_name(const char *name)
 {
 	/*

builtin/ls-files.c +19 -0

+}
+
 static const char *get_tag(const struct cache_entry *ce, const char *tag)
 {
 	static char alttag[4];

@@ -222,6 +249,86 @@ static void show_submodule(struct repository *superproject,
 	repo_clear(&subrepo);
 }
 
+struct show_index_data {
+	const char *tag;
+	const char *pathname;
+	struct index_state *istate;
+	const struct cache_entry *ce;
+};
+
+static size_t expand_show_index(struct strbuf *sb, const char *start,
+			       void *context)
+{
+	struct show_index_data *data = context;
+	const char *end;
+	const char *p;
+	unsigned int errlen;
+	const struct stat_data *sd = &data->ce->ce_stat_data;
+	size_t len = strbuf_expand_literal_cb(sb, start, NULL);
+	if (len)

builtin/ls-files.c +5 -0

+		strbuf_addstr(sb, get_tag(data->ce, data->tag));
+	} else 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);

builtin/ls-files.c +5 -0

+	} 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)) {

builtin/ls-files.c +28 -0

+	}
+
+	return len;
+}
+
+static void show_ce_fmt(struct repository *repo, const struct cache_entry *ce,
+			const char *format, const char *fullname, const char *tag) {
+
+	struct show_index_data data = {
+		.tag = tag,
+		.pathname = fullname,
+		.istate = repo->index,
+		.ce = ce,
+	};
+
+	struct strbuf sb = STRBUF_INIT;
+	strbuf_expand(&sb, format, expand_show_index, &data);
+	strbuf_addch(&sb, line_terminator);
+	fwrite(sb.buf, sb.len, 1, stdout);
+	strbuf_release(&sb);
+	return;
+}
+
 static void show_ce(struct repository *repo, struct dir_struct *dir,
 		    const struct cache_entry *ce, const char *fullname,
 		    const char *tag)

@@ -236,6 +343,11 @@ static void show_ce(struct repository *repo, struct dir_struct *dir,
 				  max_prefix_len, ps_matched,
 				  S_ISDIR(ce->ce_mode) ||
 				  S_ISGITLINK(ce->ce_mode))) {
+		if (format) {
+			show_ce_fmt(repo, ce, format, fullname, tag);
+			return;
+		}
+
 		tag = get_tag(ce, tag);
 
 		if (!show_stage) {

builtin/ls-files.c +1 -0

+
 	if (show_tag || show_valid_bit || show_fsmonitor_bit) {
 		tag_cached = "H ";
 		tag_unmerged = "M ";

t/t3013-ls-files-format.sh new file +48 -0

new file mode 100755
index 00000000000..61a2e68713a
@@ -0,0 +1,142 @@
+#!/bin/sh
+
+test_description='git ls-files --format test'
+
+. ./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
+'
+
+test_expect_success 'git ls-files --format tag' '
+	printf "H \nH \n" >expect &&
+	git ls-files --format="%(tag)" -t >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'git ls-files --format objectmode' '
+	cat >expect <<-EOF &&
+	100755
+	100644
+	EOF
+	git ls-files --format="%(objectmode)" -t >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'git ls-files --format objectname' '
+	oid1=$(git hash-object o1) &&
+	oid2=$(git hash-object o2) &&
+	cat >expect <<-EOF &&
+	$oid1
+	$oid2
+	EOF
+	git ls-files --format="%(objectname)" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'git ls-files --format eol' '
+	printf "i/lf    w/lf    attr/                 \t\n" >expect &&
+	printf "i/lf    w/lf    attr/                 \t\n" >>expect &&
+	git ls-files --format="%(eol)" --eol >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'git ls-files --format path' '

t/t3013-ls-files-format.sh new file +9 -0

+	git ls-files --format="  %(mtime)" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'git ls-files --format dev and ino' '
+	git ls-files --debug | grep dev >expect &&
+	git ls-files --format="  %(dev)%x09%(ino)" >actual &&
+	test_cmp expect actual
+'

t/t3013-ls-files-format.sh new file +15 -0

+	test_when_finished "git restore o1" &&
+	cat >expect <<-EOF &&
+	o1
+	EOF
+	git ls-files --format="%(path)" -d >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'git ls-files --format size and flags' '
+	git ls-files --debug | grep size >expect &&
+	git ls-files --format="  %(size)%x09%(flags)" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'git ls-files --format imitate --stage' '

t/t3013-ls-files-format.sh new file +13 -0

+test_expect_success 'git ls-files --format with -s must fail' '
+	test_must_fail git ls-files --format="%(objectname)" -s
+'
+
+test_expect_success 'git ls-files --format with -o must fail' '
+	test_must_fail git ls-files --format="%(objectname)" -o
+'
+
+test_expect_success 'git ls-files --format with -k must fail' '
+	test_must_fail git ls-files --format="%(objectname)" -k
+'
+
+test_expect_success 'git ls-files --format with --resolve-undo must fail' '

t/t3013-ls-files-format.sh new file +1 -0

+test_done

builtin/ls-files.c static void write_name(const char *name) · line 107

Quoted 5 lines

builtin/ls-files.c +3 -0

+static void write_name_to_buf(struct strbuf *sb, const char *name)
+{
+     name = relative_path(name, prefix_len ? prefix : NULL, sb);

FWIW I'd find this a bit less "huh?" if we declared another variable here, so just:

const char *rel = relative_path(name, ...).

t/t3013-ls-files-format.sh line 91

Quoted 8 lines

t/t3013-ls-files-format.sh new file +8 -0

+     o1
+     EOF
+     git ls-files --format="%(path)" -m >actual &&
+     test_cmp expect actual
+'
+
+test_expect_success 'git ls-files --format with -d' '
+     rm o1 &&

Don't "rm o1" here, rather have the test that creates it do:

test_when_finished "rm o1" &&
[the command that creates o1]

t/t3013-ls-files-format.sh line 99

Quoted 8 lines

t/t3013-ls-files-format.sh new file +8 -0

+     test_when_finished "git restore o1" &&
+     cat >expect <<-EOF &&
+     o1
+     EOF
+     git ls-files --format="%(path)" -d >actual &&
+     test_cmp expect actual
+'
+

These tests...:

Quoted 13 lines

t/t3013-ls-files-format.sh new file +13 -0

+test_expect_success 'git ls-files --format with -s must fail' '
+     test_must_fail git ls-files --format="%(objectname)" -s
+'
+
+test_expect_success 'git ls-files --format with -o must fail' '
+     test_must_fail git ls-files --format="%(objectname)" -o
+'
+
+test_expect_success 'git ls-files --format with -k must fail' '
+     test_must_fail git ls-files --format="%(objectname)" -k
+'
+
+test_expect_success 'git ls-files --format with --resolve-undo must fail' '

t/t3013-ls-files-format.sh line 140

Quoted 10 lines

t/t3013-ls-files-format.sh new file +10 -0

+     test_must_fail git ls-files --format="%(objectname)" --resolve-undo
+'
+
+test_expect_success 'git ls-files --format with --deduplicate must fail' '
+     test_must_fail git ls-files --format="%(objectname)" --deduplicate
+'
+
+test_expect_success 'git ls-files --format with --debug must fail' '
+     test_must_fail git ls-files --format="%(objectname)" --debug
+'

...would be better done with a for-loop, so:

        for flag in -s -o -k --resolve-undo [...]
        do
                test_expect_success "git ls-files --format is incompatible with $flag" '
                        test_must_fail git ls-files --format="%(objectname)" $flag
                '
        done

Note the '' on the second argument, that's intentional, as we eval it you don't need "".

Quoted 1 line

+test_done

From: ZheNing Hu <[email protected]>

--object-only is an alias for --format=%(objectname), which output objectname of index entries, taking inspiration from the option with the same name in the git ls-tree command.

--object-only cannot be used with --format, and -s, -o, -k, --resolve-undo, --deduplicate, --debug.

Signed-off-by ZheNing Hu <[email protected]>

---
 Documentation/git-ls-files.txt |  8 +++++++-
 builtin/ls-files.c             | 36 +++++++++++++++++++++++++++++++++-
 t/t3013-ls-files-format.sh     | 34 ++++++++++++++++++++++++++++++++
 3 files changed, 76 insertions(+), 2 deletions(-)

Documentation/git-ls-files.txt +7 -1

index b22860ec8c0..c3f46bb821b 100644
@@ -13,7 +13,7 @@ SYNOPSIS
 		[-c|--cached] [-d|--deleted] [-o|--others] [-i|--|ignored]
 		[-s|--stage] [-u|--unmerged] [-k|--|killed] [-m|--modified]
 		[--directory [--no-empty-directory]] [--eol]
-		[--deduplicate]
+		[--deduplicate] [--object-only]
 		[-x <pattern>|--exclude=<pattern>]
 		[-X <file>|--exclude-from=<file>]
 		[--exclude-per-directory=<file>]

@@ -199,6 +199,12 @@ followed by the  ("attr/<eolattr>").
 	interpolates to `\0` (NUL), `%09` to `\t` (TAB) and %0a to `\n` (LF).
 	--format cannot be combined with `-s`, `-o`, `-k`, `--resolve-undo`,
 	`--debug`.
+
+--object-only::
+	List only names of the objects, one per line. This is equivalent
+	to specifying `--format='%(objectname)'`. Cannot be combined with
+	`--format=<format>`.
+
 \--::
 	Do not interpret any more arguments as options.
 

builtin/ls-files.c +10 -0

index 9dd6c55eeb9..4ac8f34baac 100644
@@ -60,6 +60,27 @@ static const char *tag_modified = "";
 static const char *tag_skip_worktree = "";
 static const char *tag_resolve_undo = "";
 
+static enum ls_files_cmdmode {
+	MODE_DEFAULT = 0,
+	MODE_OBJECT_ONLY,
+} ls_files_cmdmode;
+
+struct ls_files_cmdmodee_to_fmt {
+	enum ls_files_cmdmode mode;
+	const char *const fmt;
+};
+

Quoted 61 lines · ZheNing Hu via GitGitGadget

From: ZheNing Hu <[email protected]>

--object-only is an alias for --format=%(objectname), which output objectname of index entries, taking inspiration from the option with the same name in the git ls-tree command.

--object-only cannot be used with --format, and -s, -o, -k, --resolve-undo, --deduplicate, --debug.

Signed-off-by ZheNing Hu <[email protected]>

---
 Documentation/git-ls-files.txt |  8 +++++++-
 builtin/ls-files.c             | 36 +++++++++++++++++++++++++++++++++-
 t/t3013-ls-files-format.sh     | 34 ++++++++++++++++++++++++++++++++
 3 files changed, 76 insertions(+), 2 deletions(-)

Documentation/git-ls-files.txt +7 -1

index b22860ec8c0..c3f46bb821b 100644
@@ -13,7 +13,7 @@ SYNOPSIS
              [-c|--cached] [-d|--deleted] [-o|--others] [-i|--|ignored]
              [-s|--stage] [-u|--unmerged] [-k|--|killed] [-m|--modified]
              [--directory [--no-empty-directory]] [--eol]
-             [--deduplicate]
+             [--deduplicate] [--object-only]
              [-x <pattern>|--exclude=<pattern>]
              [-X <file>|--exclude-from=<file>]
              [--exclude-per-directory=<file>]

@@ -199,6 +199,12 @@ followed by the  ("attr/<eolattr>").
      interpolates to `\0` (NUL), `%09` to `\t` (TAB) and %0a to `\n` (LF).
      --format cannot be combined with `-s`, `-o`, `-k`, `--resolve-undo`,
      `--debug`.
+
+--object-only::
+     List only names of the objects, one per line. This is equivalent
+     to specifying `--format='%(objectname)'`. Cannot be combined with
+     `--format=<format>`.
+
 \--::
      Do not interpret any more arguments as options.
 

builtin/ls-files.c +10 -0

index 9dd6c55eeb9..4ac8f34baac 100644
@@ -60,6 +60,27 @@ static const char *tag_modified = "";
 static const char *tag_skip_worktree = "";
 static const char *tag_resolve_undo = "";
 
+static enum ls_files_cmdmode {
+     MODE_DEFAULT = 0,
+     MODE_OBJECT_ONLY,
+} ls_files_cmdmode;
+
+struct ls_files_cmdmodee_to_fmt {
+     enum ls_files_cmdmode mode;
+     const char *const fmt;
+};
+

builtin/ls-files.c static const char *tag_modified = ""; · line 82

Quoted 10 lines

builtin/ls-files.c +10 -0

+static struct ls_files_cmdmodee_to_fmt ls_files_cmdmode_format[] = {
+     {
+             .mode = MODE_DEFAULT,
+             .fmt = NULL,
+     },
+     {
+             .mode = MODE_OBJECT_ONLY,
+             .fmt = "%(objectname)",
+     },
+};

[...snip...]

This code all looks OK from skimming it, and is substantially copied from builtin/ls-tree.c (which is good).

But I wonder as in that case whether having such an alias is worth it at all, especially since in the case of ls-files (unlike ls-tree) we don't start out with various --just-the-X-field type options, this is the first one.

So I *really* like that you took my suggestion of "why not a --format" from a previous round, but given the above for ls-files in particular is it really worth it to have this extra code just to type:

--object-only

Instead of:

    --format="%(objectname)"

So, maybe, and I'm not set against it, but I think it's worth re-evaluating in this case.

In particular because the part of ls-tree's code is missing here where we "format optimize", i.e. we take a form like:

    --format="%(objectname)"

And dispatch it to the more optimized special function, instead of the generic strbuf_expand(), whereas in this case it's the other way around, the option is just an alias for --format.

From: ZheNing Hu <[email protected]>

Add a new option --format that output index enties informations with custom format, taking inspiration from the option with the same name in the git ls-tree command.

--format cannot used with -s, -o, -k, --resolve-undo, --deduplicate and --debug.

Signed-off-by ZheNing Hu <[email protected]>

  Documentation/git-ls-files.txt |  51 ++++++++++++-
  builtin/ls-files.c             | 130 ++++++++++++++++++++++++++++++++-
  t/t3013-ls-files-format.sh     | 130 +++++++++++++++++++++++++++++++++
  3 files changed, 307 insertions(+), 4 deletions(-)

create mode 100755 t/t3013-ls-files-format.sh

Documentation/git-ls-files.txt +14 -1

index 0dabf3f0ddc..9a88c92f1ad 100644
@@ -20,7 +20,7 @@ SYNOPSIS
  		[--exclude-standard]
  		[--error-unmatch] [--with-tree=<tree-ish>]
  		[--full-name] [--recurse-submodules]
-		[--abbrev[=<n>]] [--] [<file>...]
+		[--abbrev[=<n>]] [--format=<format>] [--] [<file>...]
 
  DESCRIPTION
  -----------

@@ -192,6 +192,13 @@ followed by the  ("attr/<eolattr>").
  	to the contained files. Sparse directories will be shown with a
  	trailing slash, such as "x/" for a sparse directory "x".
 
+--format=<format>::
+	A string that interpolates `%(fieldname)` from the result being shown.
+	It also interpolates `%%` to `%`, and `%xx` where `xx` are hex digits
+	interpolates to character with hex code `xx`; for example `%00`
+	interpolates to `\0` (NUL), `%09` to `\t` (TAB) and %0a to `\n` (LF).
+	--format cannot be combined with `-s`, `-o`, `-k`, `--resolve-undo` and
+	`--debug`.
  \--::
  	Do not interpret any more arguments as options.
 

@@ -223,6 +230,48 @@ quoted as explained for the configuration variable `core.quotePath`
  (see linkgit:git-config[1]).  Using `-z` the filename is output
  verbatim and the line is terminated by a NUL byte.
 
+It is possible to print in a custom format by using the `--format`
+option, which is able to interpolate different fields using
+a `%(fieldname)` notation. For example, if you only care about the
+"objectname" and "path" fields, you can execute with a specific
+"--format" like
+

Documentation/git-ls-files.txt +4 -0

+mtime::
+	The modify time of file.
+dev::
+	The ID of device containing file.

[...]

Documentation/git-ls-files.txt +3 -0

+static size_t expand_show_index(struct strbuf *sb, const char *start,
+			       void *context)
+{

Hi ZheNing

Quoted 7 lines · ZheNing Hu via GitGitGadget

From: ZheNing Hu <[email protected]>

Add a new option --format that output index enties informations with custom format, taking inspiration from the option with the same name in the git ls-tree command.

Quoted 2 lines

--format cannot used with -s, -o, -k, --resolve-undo, --deduplicate and --debug.

I think this is an interesting feature that provides functionality that is not available by feeding index entries into cat-file.

Quoted 49 lines

Signed-off-by ZheNing Hu <[email protected]>

  Documentation/git-ls-files.txt |  51 ++++++++++++-
  builtin/ls-files.c             | 130 ++++++++++++++++++++++++++++++++-
  t/t3013-ls-files-format.sh     | 130 +++++++++++++++++++++++++++++++++
  3 files changed, 307 insertions(+), 4 deletions(-)

create mode 100755 t/t3013-ls-files-format.sh

Documentation/git-ls-files.txt +19 -1

index 0dabf3f0ddc..9a88c92f1ad 100644
@@ -20,7 +20,7 @@ SYNOPSIS
              [--exclude-standard]
              [--error-unmatch] [--with-tree=<tree-ish>]
              [--full-name] [--recurse-submodules]
-             [--abbrev[=<n>]] [--] [<file>...]
+             [--abbrev[=<n>]] [--format=<format>] [--] [<file>...]
 
  DESCRIPTION
  -----------

@@ -192,6 +192,13 @@ followed by the  ("attr/<eolattr>").
      to the contained files. Sparse directories will be shown with a
      trailing slash, such as "x/" for a sparse directory "x".
 
+--format=<format>::
+     A string that interpolates `%(fieldname)` from the result being shown.
+     It also interpolates `%%` to `%`, and `%xx` where `xx` are hex digits
+     interpolates to character with hex code `xx`; for example `%00`
+     interpolates to `\0` (NUL), `%09` to `\t` (TAB) and %0a to `\n` (LF).
+     --format cannot be combined with `-s`, `-o`, `-k`, `--resolve-undo` and
+     `--debug`.
  \--::
      Do not interpret any more arguments as options.
 

@@ -223,6 +230,48 @@ quoted as explained for the configuration variable `core.quotePath`
  (see linkgit:git-config[1]).  Using `-z` the filename is output
  verbatim and the line is terminated by a NUL byte.
 
+It is possible to print in a custom format by using the `--format`
+option, which is able to interpolate different fields using
+a `%(fieldname)` notation. For example, if you only care about the
+"objectname" and "path" fields, you can execute with a specific
+"--format" like
+
+     git ls-files --format='%(objectname) %(path)'
+
+FIELD NAMES
+-----------
+Various values from structured fields can be used to interpolate

Documentation/git-ls-files.txt quoted as explained for the configuration variable `core.quotePath` · line 248

Quoted 5 lines

Documentation/git-ls-files.txt +5 -0

+into the resulting output. For each outputting line, the following
+names can be used:
+
+tag::
+     The tag of file status.

The documentation for -t strong discourages its use, so I wonder if we really want to expose it here.

Quoted 5 lines

Documentation/git-ls-files.txt +5 -0

+objectmode::
+     The mode of the object.
+objectname::
+     The name of the object.
+stage::

Documentation/git-ls-files.txt quoted as explained for the configuration variable `core.quotePath` · line 256

Quoted 3 lines

Documentation/git-ls-files.txt +3 -0

+     The stage of the file.
+eol::
+     The line endings of files.

Every other option refers to either a "file" or "object" but here we have "files". Looking at the implementation below this will print the line ending from both the index and the worktree, it would be useful to clarify that here.

Documentation/git-ls-files.txt quoted as explained for the configuration variable `core.quotePath` · line 260

Quoted 4 lines

Documentation/git-ls-files.txt +4 -0

+path::
+     The pathname of the object.
+ctime::
+     The create time of file.

It is not clear from this whether this (and all the file attributes below) are coming from the worktree or the index or both like eol?

Quoted 8 lines

Documentation/git-ls-files.txt +8 -0

+mtime::
+     The modify time of file.
+dev::
+     The ID of device containing file.
+ino::
+     The inode number of file.
+uid::
+     The user id of file owner.

Documentation/git-ls-files.txt quoted as explained for the configuration variable `core.quotePath` · line 274

Quoted 6 lines

Documentation/git-ls-files.txt +6 -0

+gid::
+     The group id of file owner.
+size::
+     The size of the file.
+flags::
+     The flags of the file.

What are the flags?

Quoted 4 lines

[...]

Documentation/git-ls-files.txt +3 -0

+static size_t expand_show_index(struct strbuf *sb, const char *start,
+                            void *context)
+{

Documentation/git-ls-files.txt

Quoted 10 lines

Documentation/git-ls-files.txt +9 -0

+     struct show_index_data *data = context;
+     const char *end;
+     const char *p;
+     unsigned int errlen;
[...]
+     else if (skip_prefix(start, "(flags)", &p))
+             strbuf_addf(sb, "flags: %x", data->ce->ce_flags);
+     else {
+             errlen = (unsigned long)len;
+             die(_("bad ls-files format: %%%.*s"), errlen, start);

errlen is declared as an unsigned int, but you cast len which is a size_t to unsigned long when assigning to errlen. Then errlen is used where a signed int is required by die. There is also a style violation as if any branch of an if needs braces then they should all be braced. I think that the best solution would be to drop errlen and just write

        else
                die(_("bad ls-files format: %%%.*s"), (int)len, start);

It would be interesting to check the performance of this implementation on a large repository as it is doing a lot of branching inside a loop. I don't think we should change it unless it turns out to be a problem. Then we could try switching on the first character of the format specifier or some other optimization.

Best Wishes

Phillip

From: ZheNing Hu <[email protected]>

Add a new option --format that output index enties informations with custom format, taking inspiration from the option with the same name in the git ls-tree command.

--format cannot used with -s, -o, -k, --resolve-undo, --deduplicate and --debug.

Signed-off-by ZheNing Hu <[email protected]>

---
ls-files: introduce "--format" options

v2->v3:

1. remove %(tag) because -t is deprecated, suggested by Phillip.
2. fix some description of atoms in document, suggested by Phillip..

[...]

Documentation/git-ls-files.txt +13 -0

+It is possible to print in a custom format by using the `--format`
+option, which is able to interpolate different fields using
+a `%(fieldname)` notation. For example, if you only care about the
+"objectname" and "path" fields, you can execute with a specific
+"--format" like
+
+	git ls-files --format='%(objectname) %(path)'
+
+FIELD NAMES
+-----------
+Various values from structured fields can be used to interpolate
+into the resulting output. For each outputting line, the following
+names can be used:

Documentation/git-ls-files.txt +5 -0

+mtime::
+	The modified time of file which is in the index.
+dev::
+	The ID of device containing file which is in the index.
+ino::

Thanks for re-rolling, having taken a look a closer look at the tests I'm concerned about the output format for some of the specifiers, see below.

Documentation/git-ls-files.txt quoted as explained for the configuration variable `core.quotePath` · line 257

Quoted 2 lines

Documentation/git-ls-files.txt +2 -0

+path::
+	The pathname of the file which is in the index.

I think that for all these it might be clearer to say "recorded in the index" rather than "of the file which is in the index"

Documentation/git-ls-files.txt quoted as explained for the configuration variable `core.quotePath` · line 259

Quoted 2 lines

Documentation/git-ls-files.txt +2 -0

+ctime::
+	The create time of file which is in the index.

This is printed with a prefix 'ctime:' (the same applies to the format specifiers below) I think we should omit that and just print the data so the user can choose the format they want.

Documentation/git-ls-files.txt quoted as explained for the configuration variable `core.quotePath` · line 261

Quoted 2 lines

Documentation/git-ls-files.txt +2 -0

+mtime::
+	The modified time of file which is in the index.

Documentation/git-ls-files.txt quoted as explained for the configuration variable `core.quotePath` · line 269

Quoted 8 lines

Documentation/git-ls-files.txt +8 -0

+dev::
+	The ID of device containing file which is in the index.
+ino::
+	The inode number of file which is in the index.
+uid::
+	The user id of file owner which is in the index.
+gid::
+	The group id of file owner which is in the index.

Documentation/git-ls-files.txt quoted as explained for the configuration variable `core.quotePath` · line 271

Quoted 2 lines

Documentation/git-ls-files.txt +2 -0

+size::
+	The size of the file which is in the index.

Documentation/git-ls-files.txt quoted as explained for the configuration variable `core.quotePath` · line 274

Quoted 3 lines

Documentation/git-ls-files.txt +3 -0

+flags::
+	The flags of the file in the index which include
+	in-memory only flags and some extended on-disk flags.

If %(flags) is going to be useful then I think we need to think about how they are printed and document that. At the moment they are printed as a hexadecimal number which is fine for debugging but probably not going to be useful for something like --format. I think printing documented symbolic names with some kind of separator (a comma maybe) between them is probably more useful

t/t3013-ls-files-format.sh line 38

Quoted 5 lines

[...]

t/t3013-ls-files-format.sh new file +4 -0

+test_expect_success 'git ls-files --format eol' '
+	printf "i/lf    w/lf    attr/                 \t\n" >expect &&
+	printf "i/lf    w/lf    attr/                 \t\n" >>expect &&
+	git ls-files --format="%(eol)" --eol >actual &&

I'm not sure why this is passing --eol as well as --format='%(eol)' - shouldn't that combination of flags be an error?

Quoted 3 lines · Phillip Wood

Thanks for re-rolling, having taken a look a closer look at the tests I'm concerned about the output format for some of the specifiers, see below.

Thanks for raising these issues. I agree with you on many of them. In addition to what you covered ....

Quoted 4 lines

Documentation/git-ls-files.txt quoted as explained for the configuration variable `core.quotePath` · line 257

Quoted 2 lines

Documentation/git-ls-files.txt +2 -0

+path::
+	The pathname of the file which is in the index.

I think that for all these it might be clearer to say "recorded in the index" rather than "of the file which is in the index"

I think we would call this "name". The name of the existing option that controls how they are shown is "--full-name", not "--full-path", for example.

Quoted 9 lines

Documentation/git-ls-files.txt quoted as explained for the configuration variable `core.quotePath` · line 259

Quoted 2 lines

Documentation/git-ls-files.txt +2 -0

+ctime::
+	The create time of file which is in the index.

This is printed with a prefix 'ctime:' (the same applies to the format specifiers below) I think we should omit that and just print the data so the user can choose the format they want.

Documentation/git-ls-files.txt quoted as explained for the configuration variable `core.quotePath` · line 261

Quoted 2 lines

Documentation/git-ls-files.txt +2 -0

+mtime::
+	The modified time of file which is in the index.

These are only the low-bits of the full timestamp, not ctime/mtime themselves.

But stepping back a bit, why do we need to include them in the output? What workflow and use case are we trying to help? Dump output from "stat <path>" equivalent from ls-files and compare with "stat ." output to see which ones are stale? Or is there any value to see the value of, say, ctime as an individual data item?

Quoted 8 lines

Documentation/git-ls-files.txt quoted as explained for the configuration variable `core.quotePath` · line 269

Quoted 8 lines

Documentation/git-ls-files.txt +8 -0

+dev::
+	The ID of device containing file which is in the index.
+ino::
+	The inode number of file which is in the index.
+uid::
+	The user id of file owner which is in the index.
+gid::
+	The group id of file owner which is in the index.

Again, why do we need to include these in the output?

Wouldn't it be sufficient, as well as a lot more useful, to show a single bit "the cached stat info matches what is in the working tree (yes/no)"?

Quoted 10 lines

Documentation/git-ls-files.txt quoted as explained for the configuration variable `core.quotePath` · line 274

Quoted 3 lines

Documentation/git-ls-files.txt +3 -0

+flags::
+	The flags of the file in the index which include
+	in-memory only flags and some extended on-disk flags.

If %(flags) is going to be useful then I think we need to think about how they are printed and document that. At the moment they are printed as a hexadecimal number which is fine for debugging but probably not going to be useful for something like --format. I think printing documented symbolic names with some kind of separator (a comma maybe) between them is probably more useful

I am guessing that most of the above are only useful for curious geeks and those who are debugging their new tweak to the code that touches the index, i.e. a debugging feature. But these folks can run "git" under a debugger, and they probably have to do so when they are seeing an unexpected value in the flags member of a cache entry anyway. So I am not sure whom this field is intended to help.

Quoted 30 lines

Quoted 9 lines

Documentation/git-ls-files.txt quoted as explained for the configuration variable `core.quotePath` · line 259

Quoted 2 lines

Documentation/git-ls-files.txt +2 -0

+ctime::
+   The create time of file which is in the index.

This is printed with a prefix 'ctime:' (the same applies to the format specifiers below) I think we should omit that and just print the data so the user can choose the format they want.

Quoted 2 lines

Documentation/git-ls-files.txt +2 -0

+mtime::
+   The modified time of file which is in the index.

These are only the low-bits of the full timestamp, not ctime/mtime themselves.

But stepping back a bit, why do we need to include them in the output? What workflow and use case are we trying to help? Dump output from "stat <path>" equivalent from ls-files and compare with "stat ." output to see which ones are stale? Or is there any value to see the value of, say, ctime as an individual data item?

Quoted 8 lines

Documentation/git-ls-files.txt quoted as explained for the configuration variable `core.quotePath` · line 269

Quoted 8 lines

Documentation/git-ls-files.txt +8 -0

+dev::
+   The ID of device containing file which is in the index.
+ino::
+   The inode number of file which is in the index.
+uid::
+   The user id of file owner which is in the index.
+gid::
+   The group id of file owner which is in the index.

Again, why do we need to include these in the output?

Quoted 3 lines

Wouldn't it be sufficient, as well as a lot more useful, to show a single bit "the cached stat info matches what is in the working tree (yes/no)"?

That does sound useful

Quoted 11 lines

Quoted 10 lines

Documentation/git-ls-files.txt quoted as explained for the configuration variable `core.quotePath` · line 274

Quoted 3 lines

Documentation/git-ls-files.txt +3 -0

+flags::
+   The flags of the file in the index which include
+   in-memory only flags and some extended on-disk flags.

If %(flags) is going to be useful then I think we need to think about how they are printed and document that. At the moment they are printed as a hexadecimal number which is fine for debugging but probably not going to be useful for something like --format. I think printing documented symbolic names with some kind of separator (a comma maybe) between them is probably more useful

Quoted 6 lines

I am guessing that most of the above are only useful for curious geeks and those who are debugging their new tweak to the code that touches the index, i.e. a debugging feature. But these folks can run "git" under a debugger, and they probably have to do so when they are seeing an unexpected value in the flags member of a cache entry anyway. So I am not sure whom this field is intended to help.

I wondered about that as well, but thought there might be a plausible use if someone wants to check if an entry is marked intent-to-add, or has the skip-worktree/spare-index bits set (are there other ways to inspect those?)

Best Wishes

Phillip

Quoted 3 lines · Phillip Wood

Thanks for re-rolling, having taken a look a closer look at the tests I'm concerned about the output format for some of the specifiers, see below.

Thanks for raising these issues. I agree with you on many of them. In addition to what you covered ....

Quoted 4 lines

Documentation/git-ls-files.txt quoted as explained for the configuration variable `core.quotePath` · line 257

Quoted 2 lines

Documentation/git-ls-files.txt +2 -0

+path::
+	The pathname of the file which is in the index.

I think that for all these it might be clearer to say "recorded in the index" rather than "of the file which is in the index"

I think we would call this "name". The name of the existing option that controls how they are shown is "--full-name", not "--full-path", for example.

We also say that we will "show the full path names" in that documentation.

Hi ZheNing

Quoted 22 lines

[...]

Documentation/git-ls-files.txt +21 -0

+It is possible to print in a custom format by using the `--format`
+option, which is able to interpolate different fields using
+a `%(fieldname)` notation. For example, if you only care about the
+"objectname" and "path" fields, you can execute with a specific
+"--format" like
+
+     git ls-files --format='%(objectname) %(path)'
+
+FIELD NAMES
+-----------
+Various values from structured fields can be used to interpolate
+into the resulting output. For each outputting line, the following
+names can be used:
+
+objectmode::
+     The mode of the file which is in the index.
+objectname::
+     The name of the file which is in the index.
+stage::
+     The stage of the file which is in the index.
+eol::

Documentation/git-ls-files.txt quoted as explained for the configuration variable `core.quotePath` · line 255

Quoted 2 lines

Documentation/git-ls-files.txt +2 -0

+     The <eolinfo> and <eolattr> of files both in the
+     index and the work-tree.

Looking at the test for this option I think it needs more work, why should --format arbitrarily append a tab to the end of the output? - the user should be able to specify a separator if they want one as part of the format string. Also I'm not sure why there is so much whitespace in the output.

If %(flags) is going to be useful then I think we need to think about how they are printed and document that. At the moment they are printed as a hexadecimal number which is fine for debugging but probably not going to be useful for something like --format. I think printing documented symbolic names with some kind of separator (a comma maybe) between them is probably more useful

t/t3013-ls-files-format.sh line 38

Quoted 5 lines

[...]

t/t3013-ls-files-format.sh new file +4 -0

+test_expect_success 'git ls-files --format eol' '
+     printf "i/lf    w/lf    attr/                 \t\n" >expect &&
+     printf "i/lf    w/lf    attr/                 \t\n" >>expect &&
+     git ls-files --format="%(eol)" --eol >actual &&

I'm not sure why this is passing --eol as well as --format='%(eol)' - shouldn't that combination of flags be an error?

Best Wishes

Phillip

From: ZheNing Hu <[email protected]>
[...]

builtin/ls-files.c +19 -0

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

builtin/ls-files.c +1 -0

+
 	if (show_tag || show_valid_bit || show_fsmonitor_bit) {
 		tag_cached = "H ";
 		tag_unmerged = "M ";

t/t3013-ls-files-format.sh new file +0 -0

new file mode 100755
index 00000000000..8c3ef2df138

t/t3013-ls-files-format.sh new file +8 -0

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

+test_done

base-commit: ab336e8f1c8009c8b1aab8deb592148e69217085

builtin/ls-files.c static void show_submodule(struct repository *superproject, · line 288

Quoted 8 lines · ZheNing Hu via GitGitGadget

builtin/ls-files.c +8 -0

+	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, "(path)", &p))
+		write_name_to_buf(sb, data->pathname);

Quoted 10 lines

builtin/ls-files.c +10 -0

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

builtin/ls-files.c static void show_submodule(struct repository *superproject, · line 306

Quoted 8 lines

builtin/ls-files.c +8 -0

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

Quoted 9 lines · Ævar Arnfjörð Bjarmason

builtin/ls-files.c static void show_submodule(struct repository *superproject, · line 288

Quoted 8 lines · ZheNing Hu via GitGitGadget

builtin/ls-files.c +8 -0

+    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, "(path)", &p))
+            write_name_to_buf(sb, data->pathname);

These are just "values".

Quoted 10 lines

Quoted 10 lines

builtin/ls-files.c +10 -0

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

Quoted 8 lines

builtin/ls-files.c static void show_submodule(struct repository *superproject, · line 306

Quoted 8 lines

builtin/ls-files.c +8 -0

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

These are not.

Quoted 10 lines

Quoted 2 lines

builtin/ls-files.c static void show_submodule(struct repository *superproject, · line 288

Quoted 2 lines

builtin/ls-files.c +2 -0

+    else if (skip_prefix(start, "(path)", &p))
+            write_name_to_buf(sb, data->pathname);

These are just "values".
...

Quoted 4 lines

Quoted 4 lines

builtin/ls-files.c +4 -0

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

Quoted 2 lines

These are not.

... If someone else
need them for some reason, we can add them back.

Quoted 10 lines · ZheNing Hu

Quoted 10 lines

Quoted 2 lines

builtin/ls-files.c static void show_submodule(struct repository *superproject, · line 288

Quoted 2 lines

builtin/ls-files.c +2 -0

+    else if (skip_prefix(start, "(path)", &p))
+            write_name_to_buf(sb, data->pathname);

These are just "values".
...

Quoted 4 lines

Quoted 4 lines

builtin/ls-files.c +4 -0

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

Quoted 4 lines

Quoted 2 lines

These are not.

... If someone else
need them for some reason, we can add them back.

If someone else needs to see "size:" printed in front of the value of sd_size member, we DO NOT HAVE TO DO ANYTHING! That someone else can write "--format=size: %(size)" to do so themselves.

Quoted 18 lines · Junio C Hamano <[email protected]> 于2022年6月27日周一 16:22写道：

Quoted 14 lines · ZheNing Hu

Quoted 12 lines

Quoted 2 lines

builtin/ls-files.c static void show_submodule(struct repository *superproject, · line 288

Quoted 2 lines

builtin/ls-files.c +2 -0

+    else if (skip_prefix(start, "(path)", &p))
+            write_name_to_buf(sb, data->pathname);

These are just "values".
...

Quoted 4 lines

builtin/ls-files.c static void show_submodule(struct repository *superproject, · line 306

Quoted 4 lines

builtin/ls-files.c +4 -0

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

These are not.

... If someone else
need them for some reason, we can add them back.

Quoted 3 lines

If someone else needs to see "size:" printed in front of the value of sd_size member, we DO NOT HAVE TO DO ANYTHING! That someone else can write "--format=size: %(size)" to do so themselves.

Oh, sorry, I mean if someone need some atoms from %(size) to %(flags), we can add them back.

Quoted 2 lines · ZheNing Hu

Oh, sorry, I mean if someone need some atoms from %(size) to %(flags), we can add them back.

Ah, I see. I am not sure about the %(flags) to help the debugging mode, but giving a single bit "is it dirty?" would be more useful than giving the cached stat info, I would think.

Thanks.

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

Quoted 2 lines

From: ZheNing Hu <[email protected]>
[...]

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.

builtin/ls-files.c int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix) · line 818

Quoted 4 lines

builtin/ls-files.c +4 -0

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

Quoted 10 lines

builtin/ls-files.c +1 -0

+
      if (show_tag || show_valid_bit || show_fsmonitor_bit) {
              tag_cached = "H ";
              tag_unmerged = "M ";

t/t3013-ls-files-format.sh new file +0 -0

new file mode 100755
index 00000000000..8c3ef2df138

t/t3013-ls-files-format.sh line 4

Quoted 4 lines

t/t3013-ls-files-format.sh new file +4 -0

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

Quoted 11 lines

t/t3013-ls-files-format.sh new file +10 -0

+. ./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
+'
+
[...]

t/t3013-ls-files-format.sh line 123

Quoted 6 lines

t/t3013-ls-files-format.sh new file +6 -0

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

Quoted 3 lines

+test_done

base-commit: ab336e8f1c8009c8b1aab8deb592148e69217085

Range-diff vs v3:
...
+test_done

Documentation/git-ls-files.txt +8 -0

+FIELD NAMES
+-----------
+Various values from structured fields can be used to interpolate
+into the resulting output. For each outputting line, the following
+names can be used:
+
+objectmode::
+	The mode of the file which is recorded in the index.

Quoted 3 lines · ZheNing Hu via GitGitGadget

Range-diff vs v3:
...
+test_done

I omitted 300 lines of range-diff, which is not exactly illuminating in this case. I wonder if there is a way to turn it off when it is not helping automatically...

Quoted 8 lines

Documentation/git-ls-files.txt +8 -0

+FIELD NAMES
+-----------
+Various values from structured fields can be used to interpolate
+into the resulting output. For each outputting line, the following
+names can be used:
+
+objectmode::
+     The mode of the file which is recorded in the index.

Documentation/git-ls-files.txt quoted as explained for the configuration variable `core.quotePath`

Quoted 10 lines

Documentation/git-ls-files.txt +8 -0

+objectname::
+     The name of the file which is recorded in the index.
+stage::
+     The stage of the file which is recorded in the index.
 
 
+eolinfo:index::
+     The <eolinfo> of the file which is recorded in the index.
+eolinfo:worktree::
+     The <eolinfo> of the file which is recorded in the working tree.

These sound somewhat strange, as the above makes it sound as if we are recording eolinfo for something (we never record eolinfo of anything anywhere).

eolinfo:index::
eolinfo:worktree::
The <eolinfo> (see the description of the --eol option) of the contents in the index or in the worktree for the path

perhaps? I dunno.

Documentation/git-ls-files.txt quoted as explained for the configuration variable `core.quotePath` · line 258

Quoted 2 lines

Documentation/git-ls-files.txt +2 -0

+eolattr::
+     The <eolattr> of the file which is recorded in the index.

Likewise, eolattr comes from the attribute subsystem and not recorded in the index. It is more like

eolattr:
The <eolattr> (see the description of the --eol option) that applies to the path.

Because attribute applies to the path, it applies equally to both what is in the index and what is in the working tree.

Documentation/git-ls-files.txt quoted as explained for the configuration variable `core.quotePath` · line 260

Quoted 2 lines

Documentation/git-ls-files.txt +2 -0

+path::
+     The pathname of the file which is recorded in the index.

As ls-tree already uses %(path) for it, this is probably OK (otherwise we would probably have called it %(pathname)).

From: ZheNing Hu <[email protected]>

Add a new option --format that output index enties informations with custom format, taking inspiration from the option with the same name in the git ls-tree command.

--format cannot used with -s, -o, -k, --resolve-undo, --deduplicate and --eol.

Signed-off-by ZheNing Hu <[email protected]>

---
[...]

Quoted 13 lines · ZheNing Hu via GitGitGadget

From: ZheNing Hu <[email protected]>

Add a new option --format that output index enties informations with custom format, taking inspiration from the option with the same name in the git ls-tree command.

--format cannot used with -s, -o, -k, --resolve-undo, --deduplicate and --eol.

Signed-off-by ZheNing Hu <[email protected]>

---
[...]

t/t3013-ls-files-format.sh line 108

Quoted 7 lines

t/t3013-ls-files-format.sh new file +7 -0

+test_expect_success 'git ls-files --format with --debug' '
+     git ls-files --debug >expect &&
+     git ls-files --format="%(path)" --debug >actual &&
+     test_cmp expect actual
+'
+
+test_done

I'm not sure what to make of this.

In some ways I think this makes more sense than what I suggested in
https://lore.kernel.org/git/[email protected]/;
but I had to think for a second about what's going on here.

In my version I suggested having this work with --debug, but not in this way, in my version you'd always emit the debug output, and the format output.

But here e.g.:

    git ls-files -t --debug

Will emit "H tag.c" or whatever, but if you add --format the -t option is silently discarded.

So the test is relying on "%(path)" being the default format.

I think extending this to e.g. test what happens with "-t" would be a good thing, but also in general does combining --format with -t make sense, and are there other such options where the combination might not make sense?

So I'm not 100% sure, but I think I'd prefer my version, but I see how it would get hairy to support, e.g.:

    git ls-files -s --debug --format=...

Should work, but you'd have to special-case the logic for erroring if -s is combined with --format.

Anyway, I think it would be fine to leave this in whatever state is easy, the --debug option "just for debugging".

But re
https://lore.kernel.org/git/CAOLTT8Tc95-aUE+uN2d8QjTJpGpGw6cBJfG+bpmyE55OcXTSRA@mail.gmail.com/
I think it might be interesting to get --format to a state where we can remove --debug entirely.

I.e. in c2a29405105 (t1091/t3705: remove 'test-tool read-cache --table', 2021-12-22) we could replace some similar test-only code with "git ls-files". I for one wouldn't mind --debug going away entirely, and have the t3705-add-sparse-checkout.sh tests use --format instead.

Or we could keep --debug, but just have it powerful enough to do what print_debug() is doing now, possibly without "truly internal" stuff like "ce_flags".

From: ZheNing Hu <[email protected]>

Documentation/git-ls-files.txt +10 -0

+FIELD NAMES
+-----------
+Various values from structured fields can be used to interpolate
+into the resulting output. For each outputting line, the following
+names can be used:
+
+objectmode::
+	The mode of the file which is recorded in the index.
+objectname::
+	The name of the file which is recorded in the index.

Hi ZheNing

This looks good, I don't have much to add beyond the comments others have left.

Quoted 16 lines · ZheNing Hu via GitGitGadget

From: ZheNing Hu <[email protected]>

Documentation/git-ls-files.txt +15 -0

+FIELD NAMES
+-----------
+Various values from structured fields can be used to interpolate
+into the resulting output. For each outputting line, the following
+names can be used:
+
+objectmode::
+     The mode of the file which is recorded in the index.
+objectname::
+     The name of the file which is recorded in the index.
+stage::
+     The stage of the file which is recorded in the index.
+eolinfo:index::
+     The <eolinfo> of the file which is recorded in the index.
+eolinfo:worktree::

Documentation/git-ls-files.txt quoted as explained for the configuration variable `core.quotePath` · line 260

Quoted 5 lines

Documentation/git-ls-files.txt +5 -0

+     The <eolinfo> of the file which is recorded in the working tree.
+eolattr::
+     The <eolattr> of the file which is recorded in the index.
+path::
+     The pathname of the file which is recorded in the index.

I think starting with this shorter list of field names is a good idea, we can always add more fields later if there is a demand for %(flags) etc.

t/t3013-ls-files-format.sh line 106

Quoted 5 lines

t/t3013-ls-files-format.sh new file +5 -0

+test_expect_success 'git ls-files --format with --debug' '
+     git ls-files --debug >expect &&
+     git ls-files --format="%(path)" --debug >actual &&
+     test_cmp expect actual
+'

What's the motivation for being able to combine --format with --debug?

Best Wishes

Phillip

From: ZheNing Hu <[email protected]>

Add a new option --format that output index enties informations with custom format, taking inspiration from the option with the same name in the git ls-tree command.

--format cannot used with -s, -o, -k, -t, --resolve-undo, --deduplicate and --eol.

Signed-off-by ZheNing Hu <[email protected]>

---
ls-files: introduce "--format" options

v4->v5:

1. Let --format incompatible with -t.
2. Fix %(eolinfo) and %(eolattr) docs suggested by Junio.

Looking forward to Ævar's reviewing.

Quoted 14 lines · ZheNing Hu via GitGitGadget

From: ZheNing Hu <[email protected]>

Add a new option --format that output index enties informations with custom format, taking inspiration from the option with the same name in the git ls-tree command.

--format cannot used with -s, -o, -k, -t, --resolve-undo, --deduplicate and --eol.

Signed-off-by ZheNing Hu <[email protected]>

---
ls-files: introduce "--format" options

Quoted 6 lines

v4->v5:

1. Let --format incompatible with -t.
2. Fix %(eolinfo) and %(eolattr) docs suggested by Junio.

Looking forward to Ævar's reviewing.

Thanks again, I took a look at this and it looks good to me as-is.

If you do want to further twiddle with it at this point I applied these changes to it locally while poking around, changes:

* Some trivial whitespace between variable decl.

* Removed a "return;" at the end of a function

* I found the new write_*() helpers to be uneccesary, what I did spot after seeing if they could be factored out is the existing write_eolinfo() function.

I see you just copied some of the code from there, but e.g. initializing to "" and doing an unconditional strbuf_addstr() looks odd IMO compared to just doing it inline as below.

I think if helpers are to be introduced here I'd think it would make more sense to split out the small bits of behavior from write_eolinfo() so you can call it picemeal and share the code, but since it's calling trivial external functions I think just calling those directly probably makes more sense...

* Likewise for the test I wondered if you were adding a bug by not reporting when lstat() failed, but then found that this is the same thing we do on --eol.

So for the tests I think it's better just to demonstrate that we can emit the exact same thing that --eol does with --format.

* We've gone back & forth a bit on whether this would combine with --debug, while it's an internal-only feature it would be nice to have a test for it combined with --format, noting that the behavior might change...

* There is one subtle behavior change here in that I deleted the "ce &&" part from write_index_eolinfo_to_buf() when moving the code over. I'm 99% sure this is the right thing to do, as other code in expand_show_index() unconditionally dereferences it.

So perhaps we don't need that guard in write_eolinfo() either? In any case copy/pasting it over when we're already assuming a non-NULL "ce" in the same "if/elseif/else" chain looks a bit odd.

Ah, I see it's because in show_dir_entry() we explicitly pass it as NULL, but that doesn't apply to our new codepath, so as long as we're not sharing that helper with write_eolinfo() it makes sense to not do that check.

Even then the helper should probably assume "ce", and write_eolinfo() itself should do the "is ce NULL?" check which is specific to its use-case.

builtin/ls-files.c +13 -31

index 79ecdce2c9c..cc3cece3830 100644
@@ -77,30 +77,6 @@ static void write_eolinfo(struct index_state *istate,
        }
 }
 
-static void write_index_eolinfo_to_buf(struct strbuf *sb, struct index_state *istate,
-                                      const struct cache_entry *ce)
-{
-       const char *i_txt = "";
-       if (ce && S_ISREG(ce->ce_mode))
-               i_txt = get_cached_convert_stats_ascii(istate, ce->name);
-       strbuf_addstr(sb, i_txt);
-}
-
-static void write_worktree_eolinfo_to_buf(struct strbuf *sb, const char *path)
-{
-       struct stat st;
-       const char *w_txt = "";
-       if (!lstat(path, &st) && S_ISREG(st.st_mode))
-               w_txt = get_wt_convert_stats_ascii(path);
-       strbuf_addstr(sb, w_txt);
-}
-
-static void write_eolattr_to_buf(struct strbuf *sb, struct index_state *istate,
-                                const char *path)
-{
-       strbuf_addstr(sb, get_convert_attr_ascii(istate, path));
-}
-
 static void write_name(const char *name)
 {
        /*

@@ -114,6 +90,7 @@ static void write_name(const char *name)
 static void write_name_to_buf(struct strbuf *sb, const char *name)
 {
        const char *rel = relative_path(name, prefix_len ? prefix : NULL, sb);
+
        if (line_terminator)
                quote_c_style(rel, sb, NULL, 0);
        else

@@ -270,6 +247,8 @@ static size_t expand_show_index(struct strbuf *sb, const char *start,
        const char *end;
        const char *p;
        size_t len = strbuf_expand_literal_cb(sb, start, NULL);
+       struct stat st;
+
        if (len)
                return len;
        if (*start != '(')

@@ -288,12 +267,16 @@ static size_t expand_show_index(struct strbuf *sb, const char *start,
                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, "(eolinfo:index)", &p))
-               write_index_eolinfo_to_buf(sb, data->istate, data->ce);
-       else if (skip_prefix(start, "(eolinfo:worktree)", &p))
-               write_worktree_eolinfo_to_buf(sb, data->pathname);
+       else if (skip_prefix(start, "(eolinfo:index)", &p) &&
+                S_ISREG(data->ce->ce_mode))
+               strbuf_addstr(sb, get_cached_convert_stats_ascii(data->istate,
+                                                                data->ce->name));
+       else if (skip_prefix(start, "(eolinfo:worktree)", &p) &&
+                !lstat(data->pathname, &st) && S_ISREG(st.st_mode))
+               strbuf_addstr(sb, get_wt_convert_stats_ascii(data->pathname));
        else if (skip_prefix(start, "(eolattr)", &p))
-               write_eolattr_to_buf(sb, data->istate, data->pathname);
+               strbuf_addstr(sb, get_convert_attr_ascii(data->istate,
+                                                        data->pathname));
        else if (skip_prefix(start, "(path)", &p))
                write_name_to_buf(sb, data->pathname);
        else

@@ -310,13 +293,12 @@ static void show_ce_fmt(struct repository *repo, const struct cache_entry *ce,
                .istate = repo->index,
                .ce = ce,
        };
-
        struct strbuf sb = STRBUF_INIT;
+
        strbuf_expand(&sb, format, expand_show_index, &data);
        strbuf_addch(&sb, line_terminator);
        fwrite(sb.buf, sb.len, 1, stdout);
        strbuf_release(&sb);
-       return;
 }
 
 static void show_ce(struct repository *repo, struct dir_struct *dir,

t/t3013-ls-files-format.sh +0 -21

index 60c415aafd6..baf03f9096e 100755
@@ -40,27 +40,13 @@ test_expect_success 'git ls-files --format objectname' '
        test_cmp expect actual
 '
 
-test_expect_success 'git ls-files --format eolinfo:index' '
-       cat >expect <<-\EOF &&
-       lf
-       lf
-       EOF
-       git ls-files --format="%(eolinfo:index)" >actual &&
-       test_cmp expect actual
-'
-
-test_expect_success 'git ls-files --format eolinfo:worktree' '
-       cat >expect <<-\EOF &&
-       lf
-       lf
-       EOF
-       git ls-files --format="%(eolinfo:worktree)" >actual &&
-       test_cmp expect actual
-'
-
-test_expect_success 'git ls-files --format eolattr' '
-       printf "\n\n" >expect &&
-       git ls-files --format="%(eolattr)" >actual &&

From: ZheNing Hu <[email protected]>

Add a new option --format that output index enties informations with custom format, taking inspiration from the option with the same name in the git ls-tree command.

+FIELD NAMES

+-----------

Documentation/git-ls-files.txt +7 -0

+Various values from structured fields can be used to interpolate
+into the resulting output. For each outputting line, the following
+names can be used:
+
+objectmode::
+	The mode of the file which is recorded in the index.
+objectname::

t/t3013-ls-files-format.sh new file +14 -0

+test_expect_success 'git ls-files --format eolinfo:index' '
+	cat >expect <<-\EOF &&
+	lf
+	lf
+	EOF
+	git ls-files --format="%(eolinfo:index)" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'git ls-files --format eolinfo:worktree' '
+	cat >expect <<-\EOF &&
+	lf
+	lf
+	EOF

Quoted 6 lines · ZheNing Hu via GitGitGadget

From: ZheNing Hu <[email protected]>

Add a new option --format that output index enties informations with custom format, taking inspiration from the option with the same name in the git ls-tree command.

[]

Quoted 1 line

+FIELD NAMES

Nice

Quoted 8 lines

+-----------

Documentation/git-ls-files.txt +7 -0

+Various values from structured fields can be used to interpolate
+into the resulting output. For each outputting line, the following
+names can be used:
+
+objectmode::
+     The mode of the file which is recorded in the index.
+objectname::

Documentation/git-ls-files.txt quoted as explained for the configuration variable `core.quotePath` · line 259

Quoted 10 lines

Documentation/git-ls-files.txt +10 -0

+     The name of the file which is recorded in the index.
+stage::
+     The stage of the file which is recorded in the index.
+eolinfo:index::
+eolinfo:worktree::
+     The <eolinfo> (see the description of the `--eol` option) of
+     the contents in the index or in the worktree for the path.
+eolattr::
+     The <eolattr> (see the description of the `--eol` option)
+     that applies to the path.

This may be a matter of taste, looking at the eol-stuff:
Should the ':' be dropped and we have 3 fieldnames like this:

eolindex
eolworktree
eolattr

Quoted 14 lines

t/t3013-ls-files-format.sh new file +14 -0

+test_expect_success 'git ls-files --format eolinfo:index' '
+     cat >expect <<-\EOF &&
+     lf
+     lf
+     EOF
+     git ls-files --format="%(eolinfo:index)" >actual &&
+     test_cmp expect actual
+'
+
+test_expect_success 'git ls-files --format eolinfo:worktree' '
+     cat >expect <<-\EOF &&
+     lf
+     lf
+     EOF

t/t3013-ls-files-format.sh line 66

Quoted 10 lines

t/t3013-ls-files-format.sh new file +10 -0

+     git ls-files --format="%(eolinfo:worktree)" >actual &&
+     test_cmp expect actual
+'
+
+test_expect_success 'git ls-files --format eolattr' '
+     printf "\n\n" >expect &&
+     git ls-files --format="%(eolattr)" >actual &&
+     test_cmp expect actual
+'
+

What exactly should this testcases test ?
Does it make sense to set up a combination of index, worktree, attr, which are happening in real live ?

There are some tests in t0025, t0027 and t0028 that do more realistic tests of different combinations.

From: ZheNing Hu <[email protected]>

Add a new option --format that output index enties ...

Documentation/git-ls-files.txt +7 -0

+
+objectmode::
+	The mode of the file which is recorded in the index.
+objectname::
+	The name of the file which is recorded in the index.
+stage::
+	The stage of the file which is recorded in the index.

builtin/ls-files.c +1 -0

@@ -48,6 +49,7 @@ static char *ps_matched;
 static const char *with_tree;
 static int exc_given;
 static int exclude_args;
+static const char *format;
 
 static const char *tag_cached = "";
 static const char *tag_unmerged = "";

builtin/ls-files.c +10 -0

+{
+	struct show_index_data *data = context;
+	const char *end;
+	const char *p;
+	size_t len = strbuf_expand_literal_cb(sb, start, NULL);
+	struct stat st;
+
+	if (len)
+		return len;
+	if (*start != '(')

Quoted 3 lines · ZheNing Hu via GitGitGadget

From: ZheNing Hu <[email protected]>

Add a new option --format that output index enties ...

Let's quote the options and use the Oxford comma.

ls-files: introduce "--format" option

Add a new option "--format" that outputs index entries in a custom format, taking inspiration from the option with the same name in the git ls-tree command.

"--format" cannot used with "-s", "-o", "-k", "-t", "--resolve-undo", "--deduplicate", and "--eol".

Documentation/git-ls-files.txt quoted as explained for the configuration variable `core.quotePath` · line 234

Quoted 2 lines

Documentation/git-ls-files.txt +2 -0

+It is possible to print in a custom format by using the `--format`
+option, which is able to interpolate different fields using

So we use the term "field" to mean different piece of information we can present. The definition of what fields are available come later and the presentation order is a bit awkward, but hopefully the text is understandable as-is.

Documentation/git-ls-files.txt quoted as explained for the configuration variable `core.quotePath` · line 239

Quoted 5 lines

Documentation/git-ls-files.txt +5 -0

+a `%(fieldname)` notation. For example, if you only care about the
+"objectname" and "path" fields, you can execute with a specific
+"--format" like
+
+     git ls-files --format='%(objectname) %(path)'

And the example makes it pretty clear. OK.

Documentation/git-ls-files.txt quoted as explained for the configuration variable `core.quotePath` · line 243

Quoted 3 lines

Documentation/git-ls-files.txt +3 -0

+FIELD NAMES
+-----------
+Various values from structured fields can be used to interpolate

Are we dealing with unstructured fields, too? If not, let's drop "structured".

Documentation/git-ls-files.txt quoted as explained for the configuration variable `core.quotePath` · line 245

Quoted 2 lines

Documentation/git-ls-files.txt +2 -0

+into the resulting output. For each outputting line, the following
+names can be used:

"outputting line" sounds like a non language.

The way each path is shown can be customized by using the --format=<format> option, where the %(fieldname) in the <format> string for various aspects of the index entry are interpolated. The following "fieldname" are understood:

perhaps?

Quoted 18 lines

builtin/ls-files.c +18 -0

+{
+     struct show_index_data *data = context;
+     const char *end;
+     const char *p;
+     size_t len = strbuf_expand_literal_cb(sb, start, NULL);
+     struct stat st;
+
+     if (len)
+             return len;
+     if (*start != '(')
+             die(_("bad ls-files format: element '%s' "
+                   "does not start with '('"), start);
+
+     end = strchr(start + 1, ')');
+     if (!end)
+             die(_("bad ls-files format: element '%s'"
+                   "does not end in ')'"), start);
+

builtin/ls-files.c static void show_submodule(struct repository *superproject, · line 263

Quoted 2 lines

builtin/ls-files.c +2 -0

+     len = end - start + 1;
+     if (skip_prefix(start, "(objectmode)", &p))

Using skip_prefix() not for the purpose of skipping (notice that nobody uses p at all) is ugly. We already computed start and end (hence the length), so we should be able to do much better than this.

But let's let it pass, as it was copy-pasted from existing code in ls-tree.c::expand_show_tree().

builtin/ls-files.c static void show_submodule(struct repository *superproject, · line 272

Quoted 4 lines

builtin/ls-files.c +4 -0

+     else if (skip_prefix(start, "(eolinfo:index)", &p) &&
+              S_ISREG(data->ce->ce_mode))
+             strbuf_addstr(sb, get_cached_convert_stats_ascii(data->istate,
+                                                              data->ce->name));

This is outright wrong, isn't it?

It is unlikely to see such a trivial error in the 6th round of a series after other reviewers looked at it many times, so perhaps I am missing something? Or perhaps this is a new code added in this round.

If you ask for %(eolinfo:index) for an index entry that is not a regular file, this "else if" will not trigger, and the control will eventually fall through to hit "bad ls-files format" but what you detected is not a bad format at all. Once the skip_prefix() hits, you should be committed to handle that "field" and never let the other choices in this if/elif/ cascade to see it.

It is OK to interpolate %(eolinfo:index) to an empty string for a gitlink and a symbolic link, but the right way to do so would probably be:

        else if (skip_prefix(start, "(eolinfo:index)", &p) {
                if (S_ISREG(data->ce->ce_mode))
                        strbuf_addstr(...);
        } else ...

builtin/ls-files.c static void show_submodule(struct repository *superproject, · line 275

Quoted 3 lines

builtin/ls-files.c +3 -0

+     else if (skip_prefix(start, "(eolinfo:worktree)", &p) &&
+              !lstat(data->pathname, &st) && S_ISREG(st.st_mode))
+             strbuf_addstr(sb, get_wt_convert_stats_ascii(data->pathname));

Likewise.

t/t3013-ls-files-format.sh line 21

Quoted 7 lines

t/t3013-ls-files-format.sh new file +7 -0

+test_expect_success 'setup' '
+     echo o1 >o1 &&
+     echo o2 >o2 &&
+     git add o1 o2 &&
+     git add --chmod +x o1 &&
+     git commit -m base
+'

Apparently, this set-up is too trivial to uncover the above bug that can be spotted in 10 seconds of staring at the code. Perhaps add a symbolic link (use "git update-index --cacheinfo" and you do not have to worry about Windows), a subdirectory and a submodule?

Quoted 8 lines

Quoted 7 lines

t/t3013-ls-files-format.sh new file +7 -0

+test_expect_success 'setup' '
+     echo o1 >o1 &&
+     echo o2 >o2 &&
+     git add o1 o2 &&
+     git add --chmod +x o1 &&
+     git commit -m base
+'

Quoted 4 lines

Apparently, this set-up is too trivial to uncover the above bug that can be spotted in 10 seconds of staring at the code. Perhaps add a symbolic link (use "git update-index --cacheinfo" and you do not have to worry about Windows), a subdirectory and a submodule?

Ah, Just looking at the c code, I took a long time (more than 10 minutes) to find out where the mistake was.

But yeah, use a subdirectory can quickly meet the error, so I need to add more cases here.

From: ZheNing Hu <[email protected]>

Add a new option "--format" that outputs index entries informations in a custom format, taking inspiration from the option with the same name in the git ls-tree command.

"--format" cannot used with "-s", "-o", "-k", "-t", " --resolve-undo","--deduplicate" and "--eol".

Signed-off-by ZheNing Hu <[email protected]>

---
ls-files: introduce "--format" options

v6->v7:

1. Change documents helped by Junio.
2. Fix bug of parsing format.
3. Add more test cases for other mode index entries (120000 and 160000).

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1262%2Fadlternative%2Fzh%2Fls-file-format-v7
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1262/adlternative/zh/ls-file-format-v7
Pull-Request: https://github.com/gitgitgadget/git/pull/1262

Range-diff vs v6:

1: 57ed2c15728 ! 1: 9ca22edba94 ls-files: introduce "--format" option

     @@ Metadata
       ## Commit message ##
          ls-files: introduce "--format" option
      
     -    Add a new option --format that output index enties
     -    informations with custom format, taking inspiration
     +    Add a new option "--format" that outputs index entries
     +    informations in a custom format, taking inspiration
          from the option with the same name in the `git ls-tree`
          command.
      
     -    --format cannot used with -s, -o, -k, -t, --resolve-undo,
     -    --deduplicate and --eol.
     +    "--format" cannot used with "-s", "-o", "-k", "-t",
     +    " --resolve-undo","--deduplicate" and "--eol".
      
          Signed-off-by: ZheNing Hu <[email protected]>
      
     @@ Documentation/git-ls-files.txt: quoted as explained for the configuration variab
      +
      +FIELD NAMES
      +-----------
     -+Various values from structured fields can be used to interpolate
     -+into the resulting output. For each outputting line, the following
     -+names can be used:
     ++The way each path is shown can be customized by using the
     ++`--format=<format>` option, where the %(fieldname) in the
     ++<format> string for various aspects of the index entry are
     ++interpolated.  The following "fieldname" are understood:
      +
      +objectmode::
      +	The mode of the file which is recorded in the index.
     @@ builtin/ls-files.c: static void write_name(const char *name)
      +static void write_name_to_buf(struct strbuf *sb, const char *name)
      +{
      +	const char *rel = relative_path(name, prefix_len ? prefix : NULL, sb);
     ++
      +	if (line_terminator)
      +		quote_c_style(rel, sb, NULL, 0);
      +	else
     -+		strbuf_add(sb, rel, strlen(rel));
     ++		strbuf_addstr(sb, rel);
      +}
      +
       static const char *get_tag(const struct cache_entry *ce, const char *tag)
     @@ builtin/ls-files.c: static void show_submodule(struct repository *superproject,
      +};
      +
      +static size_t expand_show_index(struct strbuf *sb, const char *start,
     -+			       void *context)
     ++				void *context)
      +{
      +	struct show_index_data *data = context;
      +	const char *end;
     @@ builtin/ls-files.c: static void show_submodule(struct repository *superproject,
      +		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, "(eolinfo:index)", &p) &&
     -+		 S_ISREG(data->ce->ce_mode))
     -+		strbuf_addstr(sb, get_cached_convert_stats_ascii(data->istate,
     -+								 data->ce->name));
     -+	else if (skip_prefix(start, "(eolinfo:worktree)", &p) &&
     -+		 !lstat(data->pathname, &st) && S_ISREG(st.st_mode))
     -+		strbuf_addstr(sb, get_wt_convert_stats_ascii(data->pathname));
     ++	else if (skip_prefix(start, "(eolinfo:index)", &p))
     ++		strbuf_addstr(sb, S_ISREG(data->ce->ce_mode) ?
     ++			      get_cached_convert_stats_ascii(data->istate,
     ++			      data->ce->name) : "");
     ++	else if (skip_prefix(start, "(eolinfo:worktree)", &p))
     ++		strbuf_addstr(sb, !lstat(data->pathname, &st) &&
     ++			      S_ISREG(st.st_mode) ?
     ++			      get_wt_convert_stats_ascii(data->pathname) : "");
      +	else if (skip_prefix(start, "(eolattr)", &p))
      +		strbuf_addstr(sb, get_convert_attr_ascii(data->istate,
      +			      data->pathname));
     @@ t/t3013-ls-files-format.sh (new)
      +done
      +
      +test_expect_success 'setup' '
     -+	echo o1 >o1 &&
     -+	echo o2 >o2 &&
     -+	git add o1 o2 &&
     -+	git add --chmod +x o1 &&
     ++	printf "LINEONE\nLINETWO\nLINETHREE\n" >o1.txt &&
     ++	printf "LINEONE\r\nLINETWO\r\nLINETHREE\r\n" >o2.txt &&
     ++	printf "LINEONE\r\nLINETWO\nLINETHREE\n" >o3.txt &&
     ++	ln -s o3.txt o4.txt &&
     ++	git add "*.txt" &&
     ++	git add --chmod +x o1.txt &&
     ++	git update-index --add --cacheinfo 160000 $(git hash-object o1.txt) o5.txt &&
      +	git commit -m base
      +'
      +
     -+test_expect_success 'git ls-files --format objectmode' '
     -+	cat >expect <<-\EOF &&
     -+	100755
     -+	100644
     -+	EOF
     ++test_expect_success 'git ls-files --format objectmode v.s. -s' '
     ++	git ls-files -s | awk "{print \$1}" >expect &&
      +	git ls-files --format="%(objectmode)" >actual &&
      +	test_cmp expect actual
      +'
      +
     -+test_expect_success 'git ls-files --format objectname' '
     -+	oid1=$(git hash-object o1) &&
     -+	oid2=$(git hash-object o2) &&
     -+	cat >expect <<-EOF &&
     -+	$oid1
     -+	$oid2
     -+	EOF
     ++test_expect_success 'git ls-files --format objectname v.s. -s' '
     ++	git ls-files -s | awk "{print \$2}" >expect &&
      +	git ls-files --format="%(objectname)" >actual &&
      +	test_cmp expect actual
      +'
      +
     -+HT='	'
     -+WS='    '
      +test_expect_success 'git ls-files --format v.s. --eol' '
     -+	git ls-files --eol >expect 2>err &&
     ++	git ls-files --eol >tmp &&
     ++	sed -e "s/	/ /g" -e "s/  */ /g" tmp >expect 2>err &&
      +	test_must_be_empty err &&
     -+	git ls-files --format="i/%(eolinfo:index)${WS}w/%(eolinfo:worktree)${WS}attr/${WS}${WS}${WS}${WS} ${HT}%(path)" >actual 2>err &&
     ++	git ls-files --format="i/%(eolinfo:index) w/%(eolinfo:worktree) attr/%(eolattr) %(path)" >actual 2>err &&
      +	test_must_be_empty err &&
      +	test_cmp expect actual
      +'
      +
     -+test_expect_success 'git ls-files --format path' '
     -+	cat >expect <<-\EOF &&
     -+	o1
     -+	o2
     -+	EOF
     ++test_expect_success 'git ls-files --format path v.s. -s' '
     ++	git ls-files -s | awk "{print \$4}" >expect &&
      +	git ls-files --format="%(path)" >actual &&
      +	test_cmp expect actual
      +'
      +
      +test_expect_success 'git ls-files --format with -m' '
     -+	echo change >o1 &&
     ++	echo change >o1.txt &&
      +	cat >expect <<-\EOF &&
     -+	o1
     ++	o1.txt
     ++	o5.txt
      +	EOF
      +	git ls-files --format="%(path)" -m >actual &&
      +	test_cmp expect actual
      +'
      +
      +test_expect_success 'git ls-files --format with -d' '
     -+	echo o3 >o3 &&
     -+	git add o3 &&
     -+	rm o3 &&
     ++	echo o6 >o6.txt &&
     ++	git add o6.txt &&
     ++	rm o6.txt &&
      +	cat >expect <<-\EOF &&
     -+	o3
     ++	o5.txt
     ++	o6.txt
      +	EOF
      +	git ls-files --format="%(path)" -d >actual &&
      +	test_cmp expect actual

 Documentation/git-ls-files.txt | 39 +++++++++++++-
 builtin/ls-files.c             | 95 ++++++++++++++++++++++++++++++++++
 t/t3013-ls-files-format.sh     | 87 +++++++++++++++++++++++++++++++
 3 files changed, 220 insertions(+), 1 deletion(-)
 create mode 100755 t/t3013-ls-files-format.sh

Documentation/git-ls-files.txt +38 -1

index 0dabf3f0ddc..d7986419c25 100644
@@ -20,7 +20,7 @@ SYNOPSIS
 		[--exclude-standard]
 		[--error-unmatch] [--with-tree=<tree-ish>]
 		[--full-name] [--recurse-submodules]
-		[--abbrev[=<n>]] [--] [<file>...]
+		[--abbrev[=<n>]] [--format=<format>] [--] [<file>...]
 
 DESCRIPTION
 -----------

@@ -192,6 +192,13 @@ followed by the  ("attr/<eolattr>").
 	to the contained files. Sparse directories will be shown with a
 	trailing slash, such as "x/" for a sparse directory "x".
 
+--format=<format>::
+	A string that interpolates `%(fieldname)` from the result being shown.
+	It also interpolates `%%` to `%`, and `%xx` where `xx` are hex digits
+	interpolates to character with hex code `xx`; for example `%00`
+	interpolates to `\0` (NUL), `%09` to `\t` (TAB) and %0a to `\n` (LF).
+	--format cannot be combined with `-s`, `-o`, `-k`, `-t`, `--resolve-undo`
+	and `--eol`.
 \--::
 	Do not interpret any more arguments as options.
 

@@ -223,6 +230,36 @@ quoted as explained for the configuration variable `core.quotePath`
 (see linkgit:git-config[1]).  Using `-z` the filename is output
 verbatim and the line is terminated by a NUL byte.
 
+It is possible to print in a custom format by using the `--format`
+option, which is able to interpolate different fields using
+a `%(fieldname)` notation. For example, if you only care about the
+"objectname" and "path" fields, you can execute with a specific
+"--format" like
+
+	git ls-files --format='%(objectname) %(path)'
+
+FIELD NAMES
+-----------
+The way each path is shown can be customized by using the
+`--format=<format>` option, where the %(fieldname) in the
+<format> string for various aspects of the index entry are
+interpolated.  The following "fieldname" are understood:
+
+objectmode::
+	The mode of the file which is recorded in the index.
+objectname::
+	The name of the file which is recorded in the index.
+stage::
+	The stage of the file which is recorded in the index.
+eolinfo:index::
+eolinfo:worktree::
+	The <eolinfo> (see the description of the `--eol` option) of
+	the contents in the index or in the worktree for the path.
+eolattr::
+	The <eolattr> (see the description of the `--eol` option)
+	that applies to the path.
+path::
+	The pathname of the file which is recorded in the index.
 
 EXCLUDE PATTERNS
 ----------------

builtin/ls-files.c +88 -0

index e791b65e7e9..6f3ebcaaff7 100644
@@ -11,6 +11,7 @@
 #include "quote.h"
 #include "dir.h"
 #include "builtin.h"
+#include "strbuf.h"
 #include "tree.h"
 #include "cache-tree.h"
 #include "parse-options.h"

@@ -48,6 +49,7 @@ static char *ps_matched;
 static const char *with_tree;
 static int exc_given;
 static int exclude_args;
+static const char *format;
 
 static const char *tag_cached = "";
 static const char *tag_unmerged = "";

@@ -85,6 +87,16 @@ static void write_name(const char *name)
 				   stdout, line_terminator);
 }
 
+static void write_name_to_buf(struct strbuf *sb, const char *name)
+{
+	const char *rel = relative_path(name, prefix_len ? prefix : NULL, sb);
+
+	if (line_terminator)
+		quote_c_style(rel, sb, NULL, 0);
+	else
+		strbuf_addstr(sb, rel);
+}
+
 static const char *get_tag(const struct cache_entry *ce, const char *tag)
 {
 	static char alttag[4];

@@ -222,6 +234,73 @@ static void show_submodule(struct repository *superproject,
 	repo_clear(&subrepo);
 }
 
+struct show_index_data {
+	const char *pathname;
+	struct index_state *istate;
+	const struct cache_entry *ce;
+};
+
+static size_t expand_show_index(struct strbuf *sb, const char *start,
+				void *context)
+{
+	struct show_index_data *data = context;
+	const char *end;
+	const char *p;
+	size_t len = strbuf_expand_literal_cb(sb, start, NULL);
+	struct stat st;
+
+	if (len)
+		return len;
+	if (*start != '(')
+		die(_("bad ls-files format: element '%s' "
+		      "does not start with '('"), start);
+
+	end = strchr(start + 1, ')');
+	if (!end)
+		die(_("bad ls-files format: element '%s'"
+		      "does not end in ')'"), start);
+
+	len = end - start + 1;
+	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, "(eolinfo:index)", &p))
+		strbuf_addstr(sb, S_ISREG(data->ce->ce_mode) ?
+			      get_cached_convert_stats_ascii(data->istate,
+			      data->ce->name) : "");
+	else if (skip_prefix(start, "(eolinfo:worktree)", &p))
+		strbuf_addstr(sb, !lstat(data->pathname, &st) &&
+			      S_ISREG(st.st_mode) ?
+			      get_wt_convert_stats_ascii(data->pathname) : "");
+	else if (skip_prefix(start, "(eolattr)", &p))
+		strbuf_addstr(sb, get_convert_attr_ascii(data->istate,
+			      data->pathname));
+	else if (skip_prefix(start, "(path)", &p))
+		write_name_to_buf(sb, data->pathname);
+	else
+		die(_("bad ls-files format: %%%.*s"), (int)len, start);
+
+	return len;
+}
+
+static void show_ce_fmt(struct repository *repo, const struct cache_entry *ce,
+			const char *format, const char *fullname) {
+	struct show_index_data data = {
+		.pathname = fullname,
+		.istate = repo->index,
+		.ce = ce,
+	};
+	struct strbuf sb = STRBUF_INIT;
+
+	strbuf_expand(&sb, format, expand_show_index, &data);
+	strbuf_addch(&sb, line_terminator);
+	fwrite(sb.buf, sb.len, 1, stdout);
+	strbuf_release(&sb);
+}
+
 static void show_ce(struct repository *repo, struct dir_struct *dir,
 		    const struct cache_entry *ce, const char *fullname,
 		    const char *tag)

@@ -236,6 +315,12 @@ static void show_ce(struct repository *repo, struct dir_struct *dir,
 				  max_prefix_len, ps_matched,
 				  S_ISDIR(ce->ce_mode) ||
 				  S_ISGITLINK(ce->ce_mode))) {
+		if (format) {
+			show_ce_fmt(repo, ce, format, fullname);
+			print_debug(ce);
+			return;
+		}
+
 		tag = get_tag(ce, tag);
 
 		if (!show_stage) {

@@ -675,6 +760,9 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 			 N_("suppress duplicate entries")),
 		OPT_BOOL(0, "sparse", &show_sparse_dirs,
 			 N_("show sparse directories in the presence of a sparse index")),
+		OPT_STRING_F(0, "format", &format, N_("format"),
+			     N_("format to use for the output"),
+			     PARSE_OPT_NONEG),
 		OPT_END()
 	};
 	int ret = 0;