Re: [PATCH] Add an option to git-ls-tree to display also the size of object

Jakub Narebski [off-list ref] writes:

Junio C Hamano wrote:

quoted

Jakub Narebski [off-list ref] writes:

quoted

Add -l/--long/--size option to git-ls-tree command, which displays
object size of an entry after object id (left-justified with minimum
width of 7 characters).

Not a NAK at all (but not an ACK either yet), but just asking
questions on some design considerations.

I guess I should use [PATCH/RFC] for this patch...

I do not see any need for that.  As far as I am concerned, all
the [PATCH] are RFCs ;-)

quoted

* Do these options do different things?  If not, why have more
  than one (or two, --long and its shorthand -l)?

The idea was to have output similar (if possible by git-ls-tree
machinery) to 'ls -l' output, hence -l/--long, but actually it is
about --size.

"ls -l" is about long (it is not "long to show everything the
system knows", but "longer than usual), so I think it is Ok to
say "ls-tree -l" and people would understand.

quoted

* Why pad to 7 places?  Do we have a similar padding elsewhere?
  Will this ever used by non-scripts?  How does this padding
  affect parsers other than Perl that read this information?

Padding is added here to make output more human-readable. And I guess
padding of 7 places is default for 'ls -l'.

Ok, "it is to make the output also consumable more easily by
humans" is a very reasonable answer.

quoted

  Also I suspect that having to show the size of a tree object,
  expressed in terms of the canonical representation, might
  force packv4 aware ls-tree to convert its traversal efficient
  representation to the canonical one only to get its size.

It still will be accessible, but perhaps it would be less efficient
with v4 pack. It is I think acceptable that -l needs more CPU (and I/O) 
time...

Shawn answered this better than I could.  I am moderately
negative on the size of tree objects part.

But modulo these details, I agree that being able to get the
size of each blob would be useful.

Junio C Hamano wrote:

Jakub Narebski [off-list ref] writes:

quoted

Junio C Hamano wrote:

quoted

quoted

  Also I suspect that having to show the size of a tree object,
  expressed in terms of the canonical representation, might
  force packv4 aware ls-tree to convert its traversal efficient
  representation to the canonical one only to get its size.

It still will be accessible, but perhaps it would be less efficient
with v4 pack. It is I think acceptable that -l needs more CPU (and I/O) 
time...

Shawn answered this better than I could.  I am moderately
negative on the size of tree objects part.

But modulo these details, I agree that being able to get the
size of each blob would be useful.

We can always return ' ', '-', or '0' as size for tree entries.
I wonder what to do about commits/gitlinks/subprojects...

-- 
Jakub Narebski
Poland

Add -l/--long option to git-ls-tree command, which displays
object size of a blob entry.  Object size is placed after
object id (left-justified with minimum width of 7 characters).
For non-blob entries `-' is used.

Rationale: for non-blob entries size of an object has no much
meaning, and is not very interesting.  Moreover, in planned
pack v4 tree objects would be constructed on demand, so tree
size would need to be calculated... although isn't object size
stored in the header?

While at it, cleanup whitespace: tabs are for indent, spaces are
for align.

Signed-off-by: Jakub Narebski <redacted>
---
I hope this addresses concerns mentioned in this thread: the
alternate name --size for -l/--long option, and showing size
for tree (and commit/submodule) objects.

 Documentation/git-ls-tree.txt |   14 +++++++++++++-
 builtin-ls-tree.c             |   39 +++++++++++++++++++++++++++++++--------
 2 files changed, 44 insertions(+), 9 deletions(-)

diff --git a/Documentation/git-ls-tree.txt b/Documentation/git-ls-tree.txt
index 7899394..ad7f1b9 100644
--- a/Documentation/git-ls-tree.txt
+++ b/Documentation/git-ls-tree.txt

@@ -9,7 +9,7 @@ git-ls-tree - List the contents of a tree object
 SYNOPSIS
 --------
 [verse]
-'git-ls-tree' [-d] [-r] [-t] [-z]
+'git-ls-tree' [-d] [-r] [-t] [-l] [-z]
 	    [--name-only] [--name-status] [--full-name] [--abbrev=[<n>]]
 	    <tree-ish> [paths...]
 

@@ -36,6 +36,10 @@ OPTIONS
 	Show tree entries even when going to recurse them. Has no effect
 	if '-r' was not passed. '-d' implies '-t'.
 
+-l::
+--long::
+	Show object size of blob (file) entries.
+
 -z::
 	\0 line termination on output.
 

@@ -65,6 +69,14 @@ Output Format
 When the `-z` option is not used, TAB, LF, and backslash characters
 in pathnames are represented as `\t`, `\n`, and `\\`, respectively.
 
+When the `-l` option is used, format changes to
+
+        <mode> SP <type> SP <object> SP <object size> TAB <file>
+
+Object size identified by <object> is given in bytes, and right-justified
+with minimum width of 7 characters.  Object size is given only for blobs
+(file) entries; for other entries `-` character is used in place of size.
+
 
 Author
 ------

diff --git a/builtin-ls-tree.c b/builtin-ls-tree.c
index 1cb4dca..1d2dc40 100644
--- a/builtin-ls-tree.c
+++ b/builtin-ls-tree.c

@@ -15,6 +15,7 @@ static int line_termination = '\n';
 #define LS_TREE_ONLY 2
 #define LS_SHOW_TREES 4
 #define LS_NAME_ONLY 8
+#define LS_SHOW_SIZE 16
 static int abbrev;
 static int ls_options;
 static const char **pathspec;

@@ -22,7 +23,7 @@ static int chomp_prefix;
 static const char *ls_tree_prefix;
 
 static const char ls_tree_usage[] =
-	"git-ls-tree [-d] [-r] [-t] [-z] [--name-only] [--name-status] [--full-name] [--abbrev[=<n>]] <tree-ish> [path...]";
+	"git-ls-tree [-d] [-r] [-t] [-l] [-z] [--name-only] [--name-status] [--full-name] [--abbrev[=<n>]] <tree-ish> [path...]";
 
 static int show_recursive(const char *base, int baselen, const char *pathname)
 {

@@ -55,10 +56,11 @@ static int show_recursive(const char *base, int baselen, const char *pathname)
 }
 
 static int show_tree(const unsigned char *sha1, const char *base, int baselen,
-		     const char *pathname, unsigned mode, int stage)
+                     const char *pathname, unsigned mode, int stage)
 {
 	int retval = 0;
 	const char *type = blob_type;
+	unsigned long size;
 
 	if (S_ISDIRLNK(mode)) {
 		/*

@@ -92,13 +94,27 @@ static int show_tree(const unsigned char *sha1, const char *base, int baselen,
 	    (baselen < chomp_prefix || memcmp(ls_tree_prefix, base, chomp_prefix)))
 		return 0;
 
-	if (!(ls_options & LS_NAME_ONLY))
-		printf("%06o %s %s\t", mode, type,
-				abbrev ? find_unique_abbrev(sha1,abbrev)
-					: sha1_to_hex(sha1));
+	if (!(ls_options & LS_NAME_ONLY)) {
+		if (ls_options & LS_SHOW_SIZE) {
+			if (!strcmp(type, blob_type)) {
+				sha1_object_info(sha1, &size);
+				printf("%06o %s %s %7lu\t", mode, type,
+				       abbrev ? find_unique_abbrev(sha1, abbrev)
+				              : sha1_to_hex(sha1),
+				       size);
+			} else
+				printf("%06o %s %s %7c\t", mode, type,
+				       abbrev ? find_unique_abbrev(sha1, abbrev)
+				              : sha1_to_hex(sha1),
+				       '-');
+		} else
+			printf("%06o %s %s\t", mode, type,
+			       abbrev ? find_unique_abbrev(sha1, abbrev)
+			              : sha1_to_hex(sha1));
+	}
 	write_name_quoted(base + chomp_prefix, baselen - chomp_prefix,
-			  pathname,
-			  line_termination, stdout);
+	                  pathname,
+	                  line_termination, stdout);
 	putchar(line_termination);
 	return retval;
 }

@@ -126,12 +142,19 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
 		case 't':
 			ls_options |= LS_SHOW_TREES;
 			break;
+		case 'l':
+			ls_options |= LS_SHOW_SIZE;
+			break;
 		case '-':
 			if (!strcmp(argv[1]+2, "name-only") ||
 			    !strcmp(argv[1]+2, "name-status")) {
 				ls_options |= LS_NAME_ONLY;
 				break;
 			}
+			if (!strcmp(argv[1]+2, "long")) {
+				ls_options |= LS_SHOW_SIZE;
+				break;
+			}
 			if (!strcmp(argv[1]+2, "full-name")) {
 				chomp_prefix = 0;
 				break;

-- 
1.5.1.4

Jakub Narebski [off-list ref] wrote:

Rationale: for non-blob entries size of an object has no much
meaning, and is not very interesting.  Moreover, in planned
pack v4 tree objects would be constructed on demand, so tree
size would need to be calculated... although isn't object size
stored in the header?

Yes and no.  In pack v4 the object sizes stored within the packfile
are more about what we need to know in order to efficiently unpack
the object than about supplying the canonical format length.  If a
value is redundant, we don't store it.  The canonical format length
is almost always redundant.

For blobs we still need the raw data length to unpack efficiently,
and hence we have the blob's canonical size readily available.
For trees its actually more the tree entry record count, as that
is all we need to know in order to recover the tree.  For commits
we don't even need a length, but we instead have the number of
parent commits.

Since pack v4 is really about faster runtime decoding our ability
to reproduce canonical encoding of objects is reduced slightly.
I think that's OK as we actually only need the canonical encoding
infrequently (index-pack during network transfer, cat-file) and
we're not really considering pack v4 for network transfer.  Yet.
 

I hope this addresses concerns mentioned in this thread: the
alternate name --size for -l/--long option, and showing size
for tree (and commit/submodule) objects.

Yes, thanks!

-- 
Shawn.