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

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

On Tue, Jul 05 2022, ZheNing Hu via GitGitGadget wrote:

> From: ZheNing Hu <adlternative@gmail.com>
>
> 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 <adlternative@gmail.com>
> ---
>     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.

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.

diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 79ecdce2c9c..cc3cece3830 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -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,
diff --git a/t/t3013-ls-files-format.sh b/t/t3013-ls-files-format.sh
index 60c415aafd6..baf03f9096e 100755
--- a/t/t3013-ls-files-format.sh
+++ b/t/t3013-ls-files-format.sh
@@ -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 &&
+HT='	'
+WS='    '
+test_expect_success 'git ls-files --format v.s. --eol' '
+	git ls-files --eol >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 &&
+	test_must_be_empty err &&
 	test_cmp expect actual
 '