git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 0/3] support `--oid-only` in `ls-tree`

From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date
Nov 15, 2021, 21:50 UTC
Message-ID
<211115.86o86lqe3c.gmgdl@evledraar.gmail.com>
In-Reply-To
<YZKwduZgH4BEpFzd@coredump.intra.peff.net>
On Mon, Nov 15 2021, Jeff King wrote:
Show 10 quoted lines
> On Mon, Nov 15, 2021 at 04:13:24PM +0100, Ævar Arnfjörð Bjarmason wrote:
>
>> But I'd much rather see this be done with adding strbuf_expand() to
>> ls-tree. I.e. its docs say that it can emit:
>
> I had a similar thought, but that's a much bigger task. I think it would
> be reasonable to add --oid-only to match the existing --name-only, etc.
> If we later add a custom --format option, then it can easily be folded
> in and explained as "this is an alias for --format=%(objectname)", just
> like --name-only would become "this is an alias for --format=%(path)".

A quick patch to do it below, seems to work, passes all tests, but I don't know how much I'd trust it. It's also quite an add use of strbuf_expa(). We print to stdout directly since write_name_quoted_relative() really wants to write to stdout, and not give you a buffer. But I guess it makes sense in a way.

The hardcoded %7s for %(objectsize) is a bit nasty, but I don't know if we've got anything existing that handles format specifiers with strbuf_expand() that we could steal.

I really wouldn't trust this code much, I found it when writing it that our tests for ls-tree are really lacking, e.g. we may not have a single test for "-l" anywhere (or maybe I didn't look enough, I was just running t/*ls*tree* while hacking it.

I do thin that we should consider just going with --format in either case if we agree that this is a good direction. I.e. could just support 3-4 hardcoded formats now and die if anything else is specified.

Then we'd be future-proof with the same interface expanding later, and wouldn't need to support options that we're only carrying because we didn't implement the more generic format support.

(Assume my Signed-off-by, if there's any interest...)
diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
index 3a442631c71..e89daad4229 100644
--- a/builtin/ls-tree.c
+++ b/builtin/ls-tree.c
@@ -31,6 +31,20 @@ static const  char * const ls_tree_usage[] = {
 	NULL
 };
 
+static const char *ls_tree_format_d = "%(objectmode) %(objecttype) %(objectname)	%(path)";
+static const char *ls_tree_format_l = "%(objectmode) %(objecttype) %(objectname) %(objectsize)	%(path)";
+static const char *ls_tree_format_n = "%(path)";
+
+struct expand_ls_tree_data {
+	const char *format;
+	unsigned mode;
+	const char *type;
+	const struct object_id *oid;
+	int abbrev;
+	const char *pathname;
+	const char *basebuf;
+};
+
 static int show_recursive(const char *base, int baselen, const char *pathname)
 {
 	int i;
@@ -61,9 +75,69 @@ static int show_recursive(const char *base, int baselen, const char *pathname)
 	return 0;
 }
 
+static size_t expand_show_tree(struct strbuf *sb,
+			       const char *start,
+			       void *context)
+{
+	struct expand_ls_tree_data *data = context;
+	const char *end;
+	const char *p;
+	size_t len;
+	const char *type = blob_type;
+
+	if (sb->len) {
+		fputs(sb->buf, stdout);
+		strbuf_reset(sb);
+	}
+
+	if (*start != '(')
+		die(_("bad format as of '%s'"), start);
+	end = strchr(start + 1, ')');
+	if (!end)
+		die(_("ls-tree format element '%s' does not end in ')'"),
+		    start);
+	len = end - start + 1;
+
+	if (skip_prefix(start, "(objectmode)", &p)) {
+		printf("%06o", data->mode);
+	} else if (skip_prefix(start, "(objecttype)", &p)) {
+		fputs(data->type, stdout);
+	} else if (skip_prefix(start, "(objectsize)", &p)) {
+		char size_text[24];
+		const struct object_id *oid = data->oid;
+
+		if (!strcmp(type, blob_type)) {
+			unsigned long size;
+			if (oid_object_info(the_repository, oid, &size) == OBJ_BAD)
+				xsnprintf(size_text, sizeof(size_text),
+					  "BAD");
+			else
+				xsnprintf(size_text, sizeof(size_text),
+					  "%"PRIuMAX, (uintmax_t)size);
+		} else {
+			xsnprintf(size_text, sizeof(size_text), "-");
+		}
+		printf("%7s", size_text);
+	} else if (skip_prefix(start, "(objectname)", &p)) {
+		fputs(find_unique_abbrev(data->oid, data->abbrev), stdout);
+	} else if (skip_prefix(start, "(path)", &p)) {
+		write_name_quoted_relative(data->basebuf,
+					   chomp_prefix ? ls_tree_prefix : NULL,
+					   stdout, line_termination);
+
+	} else {
+		unsigned int errlen = (unsigned long)len;
+		die(_("bad ls-tree format specifiec %%%.*s"), errlen, start);	
+	}
+
+	return len;
+}
+
 static int show_tree(const struct object_id *oid, struct strbuf *base,
 		const char *pathname, unsigned mode, void *context)
 {
+	struct expand_ls_tree_data *data = context;
+	struct strbuf sb = STRBUF_INIT;
 	int retval = 0;
 	int baselen;
 	const char *type = blob_type;
@@ -90,31 +164,18 @@ static int show_tree(const struct object_id *oid, struct strbuf *base,
 	else if (ls_options & LS_TREE_ONLY)
 		return 0;
 
-	if (!(ls_options & LS_NAME_ONLY)) {
-		if (ls_options & LS_SHOW_SIZE) {
-			char size_text[24];
-			if (!strcmp(type, blob_type)) {
-				unsigned long size;
-				if (oid_object_info(the_repository, oid, &size) == OBJ_BAD)
-					xsnprintf(size_text, sizeof(size_text),
-						  "BAD");
-				else
-					xsnprintf(size_text, sizeof(size_text),
-						  "%"PRIuMAX, (uintmax_t)size);
-			} else
-				xsnprintf(size_text, sizeof(size_text), "-");
-			printf("%06o %s %s %7s\t", mode, type,
-			       find_unique_abbrev(oid, abbrev),
-			       size_text);
-		} else
-			printf("%06o %s %s\t", mode, type,
-			       find_unique_abbrev(oid, abbrev));
-	}
 	baselen = base->len;
 	strbuf_addstr(base, pathname);
-	write_name_quoted_relative(base->buf,
-				   chomp_prefix ? ls_tree_prefix : NULL,
-				   stdout, line_termination);
+
+	strbuf_reset(&sb);
+	data->mode = mode;
+	data->type = type;
+	data->oid = oid;
+	data->abbrev = abbrev;
+	data->pathname = pathname;
+	data->basebuf = base->buf;
+	strbuf_expand(&sb, data->format, expand_show_tree, data);
+
 	strbuf_setlen(base, baselen);
 	return retval;
 }
@@ -147,6 +208,9 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
 		OPT__ABBREV(&abbrev),
 		OPT_END()
 	};
+	struct expand_ls_tree_data ls_tree_cb_data = {
+		.format = ls_tree_format_d,
+	};
 
 	git_config(git_default_config, NULL);
 	ls_tree_prefix = prefix;
@@ -161,8 +225,14 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
 	}
 	/* -d -r should imply -t, but -d by itself should not have to. */
 	if ( (LS_TREE_ONLY|LS_RECURSIVE) ==
-	    ((LS_TREE_ONLY|LS_RECURSIVE) & ls_options))
+	    ((LS_TREE_ONLY|LS_RECURSIVE) & ls_options)) {
 		ls_options |= LS_SHOW_TREES;
+	}
+	if (ls_options & LS_NAME_ONLY)
+		ls_tree_cb_data.format = ls_tree_format_n;
+
+	if (ls_options & LS_SHOW_SIZE)
+		ls_tree_cb_data.format = ls_tree_format_l;
 
 	if (argc < 1)
 		usage_with_options(ls_tree_usage, ls_tree_options);
@@ -185,6 +255,7 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
 	tree = parse_tree_indirect(&oid);
 	if (!tree)
 		die("not a tree object");
+
 	return !!read_tree(the_repository, tree,
-			   &pathspec, show_tree, NULL);
+			   &pathspec, show_tree, &ls_tree_cb_data);
 }
Previous: Jeff KingNext: Teng Long
Message 15 of 55 in “support `--oid-only` in `ls-tree`”
  1. 0/3 support `--oid-only` in `ls-tree`Teng Long, Nov 15, 2021
  2. 1/3 ls-tree.c: support `--oid-only` option for "git-ls-tree"Teng Long, Nov 15, 2021
  3. Ævar Arnfjörð BjarmasonNov 15, 2021
  4. Teng LongNov 18, 2021
  5. Ævar Arnfjörð BjarmasonNov 18, 2021
  6. Jeff KingNov 15, 2021
  7. Jeff KingNov 15, 2021
  8. Teng LongNov 18, 2021
  9. 2/3 t3104: add related tests for `--oid-only` optionTeng Long, Nov 15, 2021
  10. Đoàn Trần Công DanhNov 15, 2021
  11. Teng LongNov 18, 2021
  12. 3/3 git-ls-tree.txt: description of the 'oid-only' optionTeng Long, Nov 15, 2021
  13. Ævar Arnfjörð BjarmasonNov 15, 2021
  14. Jeff KingNov 15, 2021
  15. Ævar Arnfjörð BjarmasonNov 15, 2021
  16. Teng LongNov 19, 2021
  17. Jeff KingNov 15, 2021
  18. 0/1 support `--oid-only` in `ls-tree`Teng Long, Nov 19, 2021
  19. 1/1 ls-tree.c: support `--oid-only` option for "git-ls-tree"Teng Long, Nov 19, 2021
  20. Ævar Arnfjörð BjarmasonNov 19, 2021
  21. Junio C HamanoNov 19, 2021
  22. Teng LongNov 22, 2021
  23. Ævar Arnfjörð BjarmasonNov 22, 2021
  24. 0/1 ls-tree.c: support `--oid-only` optionTeng Long, Nov 22, 2021
  25. 1/1 ls-tree.c: support `--oid-only` option for "git-ls-tree"Teng Long, Nov 22, 2021
  26. Peter BaumannNov 22, 2021
  27. Junio C HamanoNov 22, 2021
  28. Ævar Arnfjörð BjarmasonNov 23, 2021
  29. Junio C HamanoNov 23, 2021
  30. Ævar Arnfjörð BjarmasonNov 23, 2021
  31. Junio C HamanoNov 23, 2021
  32. Junio C HamanoNov 23, 2021
  33. Ævar Arnfjörð BjarmasonNov 23, 2021
  34. Đoàn Trần Công DanhNov 23, 2021
  35. 0/1 ls-tree.c: support `--oid-only` optionTeng Long, Nov 23, 2021
  36. 1/1 ls-tree.c: support `--oid-only` option for "git-ls-tree"Teng Long, Nov 23, 2021
  37. Junio C HamanoNov 23, 2021
  38. Teng LongDec 6, 2021
  39. 0/1 support `--object-only` option for "git-ls-tree"Teng Long, Dec 8, 2021
  40. 1/1 ls-tree.c: support `--object-only` option for "git-ls-tree"Teng Long, Dec 8, 2021
  41. Junio C HamanoDec 15, 2021
  42. Teng LongDec 16, 2021
  43. Junio C HamanoDec 16, 2021
  44. Ævar Arnfjörð BjarmasonDec 16, 2021
  45. 0/1 support `--object-only` option for "git-ls-tree"Teng Long, Dec 17, 2021
  46. 1/1 ls-tree.c: support `--object-only` option for "git-ls-tree"Teng Long, Dec 17, 2021
  47. Ævar Arnfjörð BjarmasonDec 17, 2021
  48. 0/7 ls-tree --formatÆvar Arnfjörð Bjarmason, Dec 17, 2021
  49. 1/7 ls-tree: remove commented-out codeÆvar Arnfjörð Bjarmason, Dec 17, 2021
  50. 2/7 ls-tree: add missing braces to "else" armsÆvar Arnfjörð Bjarmason, Dec 17, 2021
  51. 4/7 ls-tree: use "size_t", not "int" for "struct strbuf"'s "len"Ævar Arnfjörð Bjarmason, Dec 17, 2021
  52. 3/7 ls-tree: use "enum object_type", not {blob,tree,commit}_typeÆvar Arnfjörð Bjarmason, Dec 17, 2021
  53. 6/7 ls-tree: add a --format=<fmt> optionÆvar Arnfjörð Bjarmason, Dec 17, 2021
  54. 5/7 ls-tree: split up the "init" part of show_tree()Ævar Arnfjörð Bjarmason, Dec 17, 2021
  55. 7/7 ls-tree.c: support `--object-only` option for "git-ls-tree"Ævar Arnfjörð Bjarmason, Dec 17, 2021

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.