Re: [PATCH v3 1/1] ls-tree.c: support `--oid-only` option for "git-ls-tree"

Junio C Hamano [off-list ref] writes:

Ævar Arnfjörð Bjarmason [off-list ref] writes:

quoted

quoted

That is what I would call over-engineering that I would rather not
to have in low level plumbing.
...

We've got --format for for-each-ref and family (also branch etc.), and
for the "log" family.

But I didn't comment on them. ls-tree is a lot lower-level plumbing
where --format does not belong in my mind.

There is a lot more practical reason why I'd prefer a less flexible
and good enough interface.

I can see, without coding it myself but from mere memory of how the
code looked like, how such a "we allow you to choose which field to
include, but you do not get to choose the order of fields or any
other string in the output" can be done with minimum disruption to
the existing code and without introducing a bug.  On the other hand,
I am fairly certain that anything more flexible than that will risk
new bugs involved in any large shuffling of the code, which I am
getting tired of.

So there.

On Mon, Nov 22 2021, Junio C Hamano wrote:

Junio C Hamano [off-list ref] writes:

quoted

Ævar Arnfjörð Bjarmason [off-list ref] writes:

quoted

quoted

That is what I would call over-engineering that I would rather not
to have in low level plumbing.
...

We've got --format for for-each-ref and family (also branch etc.), and
for the "log" family.

But I didn't comment on them. ls-tree is a lot lower-level plumbing
where --format does not belong in my mind.

Yes, you're just talking about ls-tree here. I'm just trying to
understand what you meant with:

    I am not interested in eliminating the _output_ by scripts.
    They should capture and format the pieces we output in any way they
    want.

Which reads to me like "we provide the data, you pretty-fy it". In this
case the proposed feature doesn't need a patch to git, but can also be
done as:

    git ls-tree HEAD | cut -d$'\t' -f1 | cut -d ' ' -f3

I think it's useful. I'm just trying to understand what you think about
such plumbing output.

There is a lot more practical reason why I'd prefer a less flexible
and good enough interface.

I can see, without coding it myself but from mere memory of how the
code looked like, how such a "we allow you to choose which field to
include, but you do not get to choose the order of fields or any
other string in the output" can be done with minimum disruption to
the existing code and without introducing a bug.  On the other hand,
I am fairly certain that anything more flexible than that will risk
new bugs involved in any large shuffling of the code, which I am
getting tired of.

To be clear, I wasn't talking about running with the WIP patch I had in
[off-list ref] here, but that the
interface wolud leave the door open to it. So something like the below.

This works to do what --oid-only does without adding that switch,
instead we add it tou our list of 4 supported hardcoded formats, all of
which map to one of the MODE_* flags.

We could just document that we support that limited list for now, and
that we might add more in the future.

So it's just a way of adding a new MODE_* without supporting an ever
growing list of flags, --oid-only, --objectmode-only, --objectsize-only
etc.

Then if we'd ever want to generalize this in the future we can pick up
someting like my WIP patch and we'd have support for any arbitrary
format.

diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
index 1e4a82e669a..e1e746ae02a 100644
--- a/builtin/ls-tree.c
+++ b/builtin/ls-tree.c

@@ -30,10 +30,11 @@ static const char * const ls_tree_usage[] = {
 	NULL
 };
 
-enum {
+enum ls_tree_cmdmode {
 	MODE_UNSPECIFIED = 0,
 	MODE_NAME_ONLY,
-	MODE_OID_ONLY
+	MODE_OID_ONLY,
+	MODE_LONG,
 };
 
 static int cmdmode = MODE_UNSPECIFIED;

@@ -131,11 +132,22 @@ static int show_tree(const struct object_id *oid, struct strbuf *base,
 	return retval;
 }
 
+static struct {
+	enum ls_tree_cmdmode cmdmode;
+	const char *fmt;
+} allowed_formats[] = {
+	{ MODE_UNSPECIFIED,	"%(objectmode) %(objecttype) %(objectname)%09%(path)" },
+	{ MODE_NAME_ONLY,	"%(path)" },
+	{ MODE_OID_ONLY,	"%(objectname)" },
+	{ MODE_LONG,		"%(objectmode) %(objecttype) %(objectsize) %(objectname)%09%(path)" },
+};
+
 int cmd_ls_tree(int argc, const char **argv, const char *prefix)
 {
 	struct object_id oid;
 	struct tree *tree;
 	int i, full_tree = 0;
+	const char *format = NULL;
 	const struct option ls_tree_options[] = {
 		OPT_BIT('d', NULL, &ls_options, N_("only show trees"),
 			LS_TREE_ONLY),

@@ -149,7 +161,8 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
 			LS_SHOW_SIZE),
 		OPT_CMDMODE('n', "name-only", &cmdmode, N_("list only filenames"), MODE_NAME_ONLY),
 		OPT_CMDMODE('s', "name-status", &cmdmode, N_("list only filenames"), MODE_NAME_ONLY),
-		OPT_CMDMODE('o', "oid-only", &cmdmode, N_("list only oids"), MODE_OID_ONLY),
+		OPT_STRING(0 , "format", &format, N_("format"),
+			   N_("(limited) format to use for the output")),
 		OPT_SET_INT(0, "full-name", &chomp_prefix,
 			    N_("use full path names"), 0),
 		OPT_BOOL(0, "full-tree", &full_tree,

@@ -170,6 +183,22 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
 		ls_tree_prefix = prefix = NULL;
 		chomp_prefix = 0;
 	}
+
+	if (format && cmdmode)
+		die(_("--format and --name-only, --long etc. are incompatible"));
+	if (format) {
+		size_t i;
+
+		for (i = 0; i <= ARRAY_SIZE(allowed_formats); i++) {
+			if (i == ARRAY_SIZE(allowed_formats))
+				die(_("your --format=%s is not on the whitelist of supported formats"), format);
+			if (!strcmp(format, allowed_formats[i].fmt)) {
+				cmdmode = allowed_formats[i].cmdmode;
+				break;
+			}
+		}
+	}
+
 	/* -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))