threads / rfc / 49783

RFC patch, 5 partsref-filter: add new formatting options

Subject: [RFC PATCH 0/5] ref-filter: add new formatting options

## tl;dr

10 messages between Nov 9, 2018 and Jan 10, 2019. Diffs are folded; open one to read it.

replies: 9people: 2as markdown or json

Оля Тележная· Nov 9, 2018, 07:37 UTC · lore

Add formatting options %(objectsize:disk) and %(deltabase), as in cat-file command.

I can not test %(deltabase) properly (I mean, I want to have test with meaningful deltabase in the result - now we have only with zeros). I tested it manually on my git repo, and I have not-null deltabases there. We have "t/t1006-cat-file.sh" with similar case, but it is about blobs. ref-filter does not work with blobs, I need to write test about refs, and I feel that I can't catch the idea (and it is hard for me to write in Shell).

Finally, I want to remove formatting logic in cat-file and use functions from ref-filter (we are almost there, so many work was done for this). I had an idea to make this migration in this patch (and stop worrying about bad tests about deltabase: we already have such test for cat-file and hopefully that could be enough). But I have another question there. cat-file has one more formatting option: "rest" [1]. Do we want such formatting option in ref-filter? It's easier for me to support that in ref-filter than to leave it only specifically for cat-file.

Thank you!
[1] https://git-scm.com/docs/git-cat-file#git-cat-file-coderestcode
Оля Тележная· Dec 24, 2018, 13:16 UTC · re: Оля Тележная · lore

[PATCH v2 0/5] ref-filter: add new formatting options

пт, 9 нояб. 2018 г. в 10:37, Оля Тележная <olyatelezhnaya@gmail.com>:
Show 21 quoted lines
>
> Add formatting options %(objectsize:disk) and %(deltabase), as in
> cat-file command.
>
> I can not test %(deltabase) properly (I mean, I want to have test with
> meaningful deltabase in the result - now we have only with zeros). I
> tested it manually on my git repo, and I have not-null deltabases
> there. We have "t/t1006-cat-file.sh" with similar case, but it is
> about blobs. ref-filter does not work with blobs, I need to write test
> about refs, and I feel that I can't catch the idea (and it is hard for
> me to write in Shell).
>
> Finally, I want to remove formatting logic in cat-file and use
> functions from ref-filter (we are almost there, so many work was done
> for this). I had an idea to make this migration in this patch (and
> stop worrying about bad tests about deltabase: we already have such
> test for cat-file and hopefully that could be enough). But I have
> another question there. cat-file has one more formatting option:
> "rest" [1]. Do we want such formatting option in ref-filter? It's
> easier for me to support that in ref-filter than to leave it only
> specifically for cat-file.
Updates since previous version:
1. Fix type cast not to generate warnings/errors in other system
platforms (travis CI says that everything is OK now)
2. Add check for negative object size (BUG if it is negative)
3. Update documentation (thanks to Junio for better wording)
>
> Thank you!
>
> [1] https://git-scm.com/docs/git-cat-file#git-cat-file-coderestcode
Оля Тележная· Jan 10, 2019, 06:25 UTC · re: Оля Тележная · lore

Re: [PATCH v2 0/5] ref-filter: add new formatting options

пн, 24 дек. 2018 г. в 16:16, Оля Тележная <olyatelezhnaya@gmail.com>:
Show 29 quoted lines
>
> пт, 9 нояб. 2018 г. в 10:37, Оля Тележная <olyatelezhnaya@gmail.com>:
> >
> > Add formatting options %(objectsize:disk) and %(deltabase), as in
> > cat-file command.
> >
> > I can not test %(deltabase) properly (I mean, I want to have test with
> > meaningful deltabase in the result - now we have only with zeros). I
> > tested it manually on my git repo, and I have not-null deltabases
> > there. We have "t/t1006-cat-file.sh" with similar case, but it is
> > about blobs. ref-filter does not work with blobs, I need to write test
> > about refs, and I feel that I can't catch the idea (and it is hard for
> > me to write in Shell).
> >
> > Finally, I want to remove formatting logic in cat-file and use
> > functions from ref-filter (we are almost there, so many work was done
> > for this). I had an idea to make this migration in this patch (and
> > stop worrying about bad tests about deltabase: we already have such
> > test for cat-file and hopefully that could be enough). But I have
> > another question there. cat-file has one more formatting option:
> > "rest" [1]. Do we want such formatting option in ref-filter? It's
> > easier for me to support that in ref-filter than to leave it only
> > specifically for cat-file.
>
> Updates since previous version:
> 1. Fix type cast not to generate warnings/errors in other system
> platforms (travis CI says that everything is OK now)
> 2. Add check for negative object size (BUG if it is negative)
> 3. Update documentation (thanks to Junio for better wording)
Just fixed 1 cast from (intmax_t) to (uintmax_t).
Show 5 quoted lines
>
> >
> > Thank you!
> >
> > [1] https://git-scm.com/docs/git-cat-file#git-cat-file-coderestcode
Olga Telezhnaya· Jan 10, 2019, 06:32 UTC · re: Оля Тележная · lore

[PATCH v3 1/6] ref-filter: add objectsize:disk option

Add new formatting option objectsize:disk to know exact size that object takes up on disk.

Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
---
 ref-filter.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)
Show changes to ref-filter.c +16 −8
diff --git a/ref-filter.c b/ref-filter.c
index 61d75d5c86c64..ecef4b47c751c 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -231,12 +231,18 @@ static int objecttype_atom_parser(const struct ref_format *format, struct used_a
 static int objectsize_atom_parser(const struct ref_format *format, struct used_atom *atom,
 				  const char *arg, struct strbuf *err)
 {
-	if (arg)
-		return strbuf_addf_ret(err, -1, _("%%(objectsize) does not take arguments"));
-	if (*atom->name == '*')
-		oi_deref.info.sizep = &oi_deref.size;
-	else
-		oi.info.sizep = &oi.size;
+	if (!arg) {
+		if (*atom->name == '*')
+			oi_deref.info.sizep = &oi_deref.size;
+		else
+			oi.info.sizep = &oi.size;
+	} else if (!strcmp(arg, "disk")) {
+		if (*atom->name == '*')
+			oi_deref.info.disk_sizep = &oi_deref.disk_size;
+		else
+			oi.info.disk_sizep = &oi.disk_size;
+	} else
+		return strbuf_addf_ret(err, -1, _("unrecognized %%(objectsize) argument: %s"), arg);
 	return 0;
 }
 
@@ -880,7 +886,10 @@ static void grab_common_values(struct atom_value *val, int deref, struct expand_
 			name++;
 		if (!strcmp(name, "objecttype"))
 			v->s = xstrdup(type_name(oi->type));
-		else if (!strcmp(name, "objectsize")) {
+		else if (!strcmp(name, "objectsize:disk")) {
+			v->value = oi->disk_size;
+			v->s = xstrfmt("%"PRIuMAX, (uintmax_t)oi->disk_size);
+		} else if (!strcmp(name, "objectsize")) {
 			v->value = oi->size;
 			v->s = xstrfmt("%"PRIuMAX , (uintmax_t)oi->size);
 		}

--
https://github.com/git/git/pull/552
Olga Telezhnaya· Jan 10, 2019, 06:32 UTC · re: Olga Telezhnaya · lore

[PATCH v3 5/6] ref-filter: add tests for deltabase

Test new formatting option deltabase.
Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
---
 t/t6300-for-each-ref.sh | 3 +++
 1 file changed, 3 insertions(+)
Show changes to t/t6300-for-each-ref.sh +3 −1
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 097fdf21fe196..0ffd63071392e 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -84,6 +84,7 @@ test_atom head push:strip=-1 master
 test_atom head objecttype commit
 test_atom head objectsize 171
 test_atom head objectsize:disk 138
+test_atom head deltabase 0000000000000000000000000000000000000000
 test_atom head objectname $(git rev-parse refs/heads/master)
 test_atom head objectname:short $(git rev-parse --short refs/heads/master)
 test_atom head objectname:short=1 $(git rev-parse --short=1 refs/heads/master)
@@ -127,6 +128,8 @@ test_atom tag objecttype tag
 test_atom tag objectsize 154
 test_atom tag objectsize:disk 138
 test_atom tag '*objectsize:disk' 138
+test_atom tag deltabase 0000000000000000000000000000000000000000
+test_atom tag '*deltabase' 0000000000000000000000000000000000000000
 test_atom tag objectname $(git rev-parse refs/tags/testtag)
 test_atom tag objectname:short $(git rev-parse --short refs/tags/testtag)
 test_atom head objectname:short=1 $(git rev-parse --short=1 refs/heads/master)

--
https://github.com/git/git/pull/552
Olga Telezhnaya· Jan 10, 2019, 06:32 UTC · re: Olga Telezhnaya · lore

[PATCH v3 4/6] ref-filter: add deltabase option

Add new formatting option: deltabase. If the object is stored as a delta on-disk, this expands to the 40-hex sha1 of the delta base object. Otherwise, expands to the null sha1 (40 zeroes). We have same option in cat-file command. Hopefully, in the end I will remove formatting code from cat-file and reuse formatting parts from ref-filter.

Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
---
 ref-filter.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)
Show changes to ref-filter.c +15 −2
diff --git a/ref-filter.c b/ref-filter.c
index 57f3789d1040d..422a9c9ae3fd2 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -246,6 +246,18 @@ static int objectsize_atom_parser(const struct ref_format *format, struct used_a
 	return 0;
 }
 
+static int deltabase_atom_parser(const struct ref_format *format, struct used_atom *atom,
+				 const char *arg, struct strbuf *err)
+{
+	if (arg)
+		return strbuf_addf_ret(err, -1, _("%%(deltabase) does not take arguments"));
+	if (*atom->name == '*')
+		oi_deref.info.delta_base_sha1 = oi_deref.delta_base_oid.hash;
+	else
+		oi.info.delta_base_sha1 = oi.delta_base_oid.hash;
+	return 0;
+}
+
 static int body_atom_parser(const struct ref_format *format, struct used_atom *atom,
 			    const char *arg, struct strbuf *err)
 {
@@ -437,6 +449,7 @@ static struct {
 	{ "objecttype", SOURCE_OTHER, FIELD_STR, objecttype_atom_parser },
 	{ "objectsize", SOURCE_OTHER, FIELD_ULONG, objectsize_atom_parser },
 	{ "objectname", SOURCE_OTHER, FIELD_STR, objectname_atom_parser },
+	{ "deltabase", SOURCE_OTHER, FIELD_STR, deltabase_atom_parser },
 	{ "tree", SOURCE_OBJ },
 	{ "parent", SOURCE_OBJ },
 	{ "numparent", SOURCE_OBJ, FIELD_ULONG },
@@ -892,7 +905,8 @@ static void grab_common_values(struct atom_value *val, int deref, struct expand_
 		} else if (!strcmp(name, "objectsize")) {
 			v->value = oi->size;
 			v->s = xstrfmt("%"PRIuMAX , (uintmax_t)oi->size);
-		}
+		} else if (!strcmp(name, "deltabase"))
+			v->s = xstrdup(oid_to_hex(&oi->delta_base_oid));
 		else if (deref)
 			grab_objectname(name, &oi->oid, v, &used_atom[i]);
 	}

--
https://github.com/git/git/pull/552
Olga Telezhnaya· Jan 10, 2019, 06:32 UTC · re: Olga Telezhnaya · lore

[PATCH v3 2/6] ref-filter: add check for negative file size

If we have negative file size, we are doing something wrong.
Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
---
 ref-filter.c | 2 ++
 1 file changed, 2 insertions(+)
Show changes to ref-filter.c +2 −1
diff --git a/ref-filter.c b/ref-filter.c
index ecef4b47c751c..57f3789d1040d 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1491,6 +1491,8 @@ static int get_object(struct ref_array_item *ref, int deref, struct object **obj
 				     OBJECT_INFO_LOOKUP_REPLACE))
 		return strbuf_addf_ret(err, -1, _("missing object %s for %s"),
 				       oid_to_hex(&oi->oid), ref->refname);
+	if (oi->info.disk_sizep && oi->disk_size < 0)
+		BUG("Object size is less than zero.");
 
 	if (oi->info.contentp) {
 		*obj = parse_object_buffer(the_repository, &oi->oid, oi->type, oi->size, oi->content, &eaten);

--
https://github.com/git/git/pull/552
Olga Telezhnaya· Jan 10, 2019, 06:32 UTC · re: Olga Telezhnaya · lore

[PATCH v3 6/6] ref-filter: add docs for new options

Add documentation for formatting options objectsize:disk and deltabase.

Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
---
 Documentation/git-for-each-ref.txt | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)
Show changes to Documentation/git-for-each-ref.txt +20 −2
diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 901faef1bfdce..774cecc7ede78 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -128,13 +128,18 @@ objecttype::
 
 objectsize::
 	The size of the object (the same as 'git cat-file -s' reports).
-
+	Append `:disk` to get the size, in bytes, that the object takes up on
+	disk. See the note about on-disk sizes in the `CAVEATS` section below.
 objectname::
 	The object name (aka SHA-1).
 	For a non-ambiguous abbreviation of the object name append `:short`.
 	For an abbreviation of the object name with desired length append
 	`:short=<length>`, where the minimum length is MINIMUM_ABBREV. The
 	length may be exceeded to ensure unique object names.
+deltabase::
+	This expands to the object name of the delta base for the
+	given object, if it is stored as a delta.  Otherwise it
+	expands to the null object name (all zeroes).
 
 upstream::
 	The name of a local ref which can be considered ``upstream''
@@ -361,6 +366,20 @@ This prints the authorname, if present.
 git for-each-ref --format="%(refname)%(if)%(authorname)%(then) Authored by: %(authorname)%(end)"
 ------------
 
+CAVEATS
+-------
+
+Note that the sizes of objects on disk are reported accurately, but care
+should be taken in drawing conclusions about which refs or objects are
+responsible for disk usage. The size of a packed non-delta object may be
+much larger than the size of objects which delta against it, but the
+choice of which object is the base and which is the delta is arbitrary
+and is subject to change during a repack.
+
+Note also that multiple copies of an object may be present in the object
+database; in this case, it is undefined which copy's size or delta base
+will be reported.
+
 SEE ALSO
 --------
 linkgit:git-show-ref[1]

--
https://github.com/git/git/pull/552
Olga Telezhnaya· Jan 10, 2019, 06:32 UTC · re: Olga Telezhnaya · lore

[PATCH v3 3/6] ref-filter: add tests for objectsize:disk

Test new formatting atom.
Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
---
 t/t6300-for-each-ref.sh | 3 +++
 1 file changed, 3 insertions(+)
Show changes to t/t6300-for-each-ref.sh +3 −1
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 97bfbee6e8d69..097fdf21fe196 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -83,6 +83,7 @@ test_atom head push:strip=1 remotes/myfork/master
 test_atom head push:strip=-1 master
 test_atom head objecttype commit
 test_atom head objectsize 171
+test_atom head objectsize:disk 138
 test_atom head objectname $(git rev-parse refs/heads/master)
 test_atom head objectname:short $(git rev-parse --short refs/heads/master)
 test_atom head objectname:short=1 $(git rev-parse --short=1 refs/heads/master)
@@ -124,6 +125,8 @@ test_atom tag upstream ''
 test_atom tag push ''
 test_atom tag objecttype tag
 test_atom tag objectsize 154
+test_atom tag objectsize:disk 138
+test_atom tag '*objectsize:disk' 138
 test_atom tag objectname $(git rev-parse refs/tags/testtag)
 test_atom tag objectname:short $(git rev-parse --short refs/tags/testtag)
 test_atom head objectname:short=1 $(git rev-parse --short=1 refs/heads/master)

--
https://github.com/git/git/pull/552
Junio C Hamano· Jan 10, 2019, 18:17 UTC · re: Оля Тележная · lore

Re: [PATCH v2 0/5] ref-filter: add new formatting options

Оля Тележная  <olyatelezhnaya@gmail.com> writes:
> Just fixed 1 cast from (intmax_t) to (uintmax_t).
Thanks.

As the previous one already is in 'next', let's queue this on top of it instead.

-- >8 --
Subject: [PATCH] ref-filter: give uintmax_t to format with %PRIuMAX

As long as we are casting to a wider type, we should cast to the one with the correct signed-ness.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 ref-filter.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to ref-filter.c +1 −1
diff --git a/ref-filter.c b/ref-filter.c
index d8d3718abb..b22cab133e 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -897,7 +897,7 @@ static void grab_common_values(struct atom_value *val, int deref, struct expand_
 			v->s = xstrdup(type_name(oi->type));
 		else if (!strcmp(name, "objectsize:disk")) {
 			v->value = oi->disk_size;
-			v->s = xstrfmt("%"PRIuMAX, (intmax_t)oi->disk_size);
+			v->s = xstrfmt("%"PRIuMAX, (uintmax_t)oi->disk_size);
 		} else if (!strcmp(name, "objectsize")) {
 			v->value = oi->size;
 			v->s = xstrfmt("%lu", oi->size);
-- 
2.20.1-98-gecbdaf0899

← back to recent threads