git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
* [RFC PATCH 0/5] ref-filter: add new formatting options
@ 2018-11-09  7:37 Оля Тележная
  2018-11-09  7:44 ` [RFC PATCH 1/5] ref-filter: add objectsize:disk option Olga Telezhnaya
  0 siblings, 1 reply; 14+ messages in thread
From: Оля Тележная @ 2018-11-09  7:37 UTC (permalink / raw)
  To: git, Christian Couder, Jeff King

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [RFC PATCH 3/5] ref-filter: add deltabase option
  2018-11-09  7:44 ` [RFC PATCH 1/5] ref-filter: add objectsize:disk option Olga Telezhnaya
  2018-11-09  7:44   ` [RFC PATCH 2/5] ref-filter: add tests for objectsize:disk Olga Telezhnaya
@ 2018-11-09  7:44   ` Olga Telezhnaya
  2018-11-09  7:44   ` [RFC PATCH 5/5] ref-filter: add docs for new options Olga Telezhnaya
                     ` (2 subsequent siblings)
  4 siblings, 0 replies; 14+ messages in thread
From: Olga Telezhnaya @ 2018-11-09  7:44 UTC (permalink / raw)
  To: git

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 | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/ref-filter.c b/ref-filter.c
index 8ba1a4e72f2c3..3cfe01a039f8b 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 },
@@ -888,7 +901,9 @@ 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("%lu", oi->size);
-		} else if (deref)
+		} 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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [RFC PATCH 5/5] ref-filter: add docs for new options
  2018-11-09  7:44 ` [RFC PATCH 1/5] ref-filter: add objectsize:disk option Olga Telezhnaya
  2018-11-09  7:44   ` [RFC PATCH 2/5] ref-filter: add tests for objectsize:disk Olga Telezhnaya
  2018-11-09  7:44   ` [RFC PATCH 3/5] ref-filter: add deltabase option Olga Telezhnaya
@ 2018-11-09  7:44   ` Olga Telezhnaya
  2018-11-12  5:24     ` Junio C Hamano
  2018-11-09  7:44   ` [RFC PATCH 4/5] ref-filter: add tests for deltabase Olga Telezhnaya
  2018-11-12  5:03   ` [RFC PATCH 1/5] ref-filter: add objectsize:disk option Junio C Hamano
  4 siblings, 1 reply; 14+ messages in thread
From: Olga Telezhnaya @ 2018-11-09  7:44 UTC (permalink / raw)
  To: git

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(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 901faef1bfdce..22d2ff88110cd 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::
+	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). See `CAVEATS` section below.
 
 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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [RFC PATCH 2/5] ref-filter: add tests for objectsize:disk
  2018-11-09  7:44 ` [RFC PATCH 1/5] ref-filter: add objectsize:disk option Olga Telezhnaya
@ 2018-11-09  7:44   ` Olga Telezhnaya
  2018-11-09  7:44   ` [RFC PATCH 3/5] ref-filter: add deltabase option Olga Telezhnaya
                     ` (3 subsequent siblings)
  4 siblings, 0 replies; 14+ messages in thread
From: Olga Telezhnaya @ 2018-11-09  7:44 UTC (permalink / raw)
  To: git

Test new formatting atom.

Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
---
 t/t6300-for-each-ref.sh | 3 +++
 1 file changed, 3 insertions(+)

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [RFC PATCH 4/5] ref-filter: add tests for deltabase
  2018-11-09  7:44 ` [RFC PATCH 1/5] ref-filter: add objectsize:disk option Olga Telezhnaya
                     ` (2 preceding siblings ...)
  2018-11-09  7:44   ` [RFC PATCH 5/5] ref-filter: add docs for new options Olga Telezhnaya
@ 2018-11-09  7:44   ` Olga Telezhnaya
  2018-11-12  5:03   ` [RFC PATCH 1/5] ref-filter: add objectsize:disk option Junio C Hamano
  4 siblings, 0 replies; 14+ messages in thread
From: Olga Telezhnaya @ 2018-11-09  7:44 UTC (permalink / raw)
  To: git

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(+)

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [RFC PATCH 1/5] ref-filter: add objectsize:disk option
  2018-11-09  7:37 [RFC PATCH 0/5] ref-filter: add new formatting options Оля Тележная
@ 2018-11-09  7:44 ` Olga Telezhnaya
  2018-11-09  7:44   ` [RFC PATCH 2/5] ref-filter: add tests for objectsize:disk Olga Telezhnaya
                     ` (4 more replies)
  0 siblings, 5 replies; 14+ messages in thread
From: Olga Telezhnaya @ 2018-11-09  7:44 UTC (permalink / raw)
  To: git

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 | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 0c45ed9d94a4b..8ba1a4e72f2c3 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;
 }
 
@@ -876,11 +882,13 @@ 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("%lld", (long long)oi->disk_size);
+		} else if (!strcmp(name, "objectsize")) {
 			v->value = oi->size;
 			v->s = xstrfmt("%lu", oi->size);
-		}
-		else if (deref)
+		} else if (deref)
 			grab_objectname(name, &oi->oid, v, &used_atom[i]);
 	}
 }

--
https://github.com/git/git/pull/552

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC PATCH 1/5] ref-filter: add objectsize:disk option
  2018-11-09  7:44 ` [RFC PATCH 1/5] ref-filter: add objectsize:disk option Olga Telezhnaya
                     ` (3 preceding siblings ...)
  2018-11-09  7:44   ` [RFC PATCH 4/5] ref-filter: add tests for deltabase Olga Telezhnaya
@ 2018-11-12  5:03   ` Junio C Hamano
  2018-11-12 12:03     ` Johannes Schindelin
  2018-11-12 13:05     ` Jeff King
  4 siblings, 2 replies; 14+ messages in thread
From: Junio C Hamano @ 2018-11-12  5:03 UTC (permalink / raw)
  To: Olga Telezhnaya; +Cc: git

Olga Telezhnaya <olyatelezhnaya@gmail.com> writes:

> @@ -876,11 +882,13 @@ 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("%lld", (long long)oi->disk_size);

oi.disk_size is off_t; do we know "long long" 

   (1) is available widely enough (I think it is from c99)?

   (2) is sufficiently wide so that we can safely cast off_t to?

   (3) will stay to be sufficiently wide as machines get larger
       together with standard types like off_t in the future?

I'd rather use intmax_t (as off_t is a signed integral type) so that
we do not have to worry about these questions in the first place.

> +		} else if (!strcmp(name, "objectsize")) {
>  			v->value = oi->size;
>  			v->s = xstrfmt("%lu", oi->size);

This is not a suggestion but is a genuine question, but I wonder if
two years down the road somebody who meets this API for the first
time find the asymmetry between "objectsize" and "objectsize:disk" a
tad strange and suggest the former to have "objectsize:real" or some
synonym.  Or we can consider "objectsize" the primary thing (hence
needing no colon-plus-modifier to clarify what kind of size we are
asking) and leave these two deliberatly asymmetric.  I am leaning
towards the latter myself.

> -		}
> -		else if (deref)
> +		} else if (deref)
>  			grab_objectname(name, &oi->oid, v, &used_atom[i]);
>  	}
>  }
>
> --
> https://github.com/git/git/pull/552

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC PATCH 5/5] ref-filter: add docs for new options
  2018-11-09  7:44   ` [RFC PATCH 5/5] ref-filter: add docs for new options Olga Telezhnaya
@ 2018-11-12  5:24     ` Junio C Hamano
  0 siblings, 0 replies; 14+ messages in thread
From: Junio C Hamano @ 2018-11-12  5:24 UTC (permalink / raw)
  To: Olga Telezhnaya; +Cc: git

Olga Telezhnaya <olyatelezhnaya@gmail.com> writes:

>  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::
> +	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). See `CAVEATS` section below.

I know existing description for other things nearby still talk about
SHA-1, but we can prepare ourselves better with something like:

	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).

> +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.

OK.


>  SEE ALSO
>  --------
>  linkgit:git-show-ref[1]
>
> --
> https://github.com/git/git/pull/552

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC PATCH 1/5] ref-filter: add objectsize:disk option
  2018-11-12  5:03   ` [RFC PATCH 1/5] ref-filter: add objectsize:disk option Junio C Hamano
@ 2018-11-12 12:03     ` Johannes Schindelin
  2018-11-12 13:12       ` Jeff King
  2018-11-12 13:05     ` Jeff King
  1 sibling, 1 reply; 14+ messages in thread
From: Johannes Schindelin @ 2018-11-12 12:03 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Olga Telezhnaya, git

Hi,

On Mon, 12 Nov 2018, Junio C Hamano wrote:

> Olga Telezhnaya <olyatelezhnaya@gmail.com> writes:
> 
> > @@ -876,11 +882,13 @@ 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("%lld", (long long)oi->disk_size);
> 
> oi.disk_size is off_t; do we know "long long" 
> 
>    (1) is available widely enough (I think it is from c99)?
> 
>    (2) is sufficiently wide so that we can safely cast off_t to?
> 
>    (3) will stay to be sufficiently wide as machines get larger
>        together with standard types like off_t in the future?
> 
> I'd rather use intmax_t (as off_t is a signed integral type) so that
> we do not have to worry about these questions in the first place.

You mean something like

			v->s = xstrfmt("%"PRIdMAX, (intmax_t)oi->disk_size);


If so, I agree.

Ciao,
Dscho

P.S.: I wondered whether we have precedent for PRIdMAX, as we used to use
only PRIuMAX, but yes: JeffH's json-writer uses PRIdMAX.

> 
> > +		} else if (!strcmp(name, "objectsize")) {
> >  			v->value = oi->size;
> >  			v->s = xstrfmt("%lu", oi->size);
> 
> This is not a suggestion but is a genuine question, but I wonder if
> two years down the road somebody who meets this API for the first
> time find the asymmetry between "objectsize" and "objectsize:disk" a
> tad strange and suggest the former to have "objectsize:real" or some
> synonym.  Or we can consider "objectsize" the primary thing (hence
> needing no colon-plus-modifier to clarify what kind of size we are
> asking) and leave these two deliberatly asymmetric.  I am leaning
> towards the latter myself.
> 
> > -		}
> > -		else if (deref)
> > +		} else if (deref)
> >  			grab_objectname(name, &oi->oid, v, &used_atom[i]);
> >  	}
> >  }
> >
> > --
> > https://github.com/git/git/pull/552
> 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC PATCH 1/5] ref-filter: add objectsize:disk option
  2018-11-12  5:03   ` [RFC PATCH 1/5] ref-filter: add objectsize:disk option Junio C Hamano
  2018-11-12 12:03     ` Johannes Schindelin
@ 2018-11-12 13:05     ` Jeff King
  1 sibling, 0 replies; 14+ messages in thread
From: Jeff King @ 2018-11-12 13:05 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Olga Telezhnaya, git

On Mon, Nov 12, 2018 at 02:03:20PM +0900, Junio C Hamano wrote:

> > +		} else if (!strcmp(name, "objectsize")) {
> >  			v->value = oi->size;
> >  			v->s = xstrfmt("%lu", oi->size);
> 
> This is not a suggestion but is a genuine question, but I wonder if
> two years down the road somebody who meets this API for the first
> time find the asymmetry between "objectsize" and "objectsize:disk" a
> tad strange and suggest the former to have "objectsize:real" or some
> synonym.  Or we can consider "objectsize" the primary thing (hence
> needing no colon-plus-modifier to clarify what kind of size we are
> asking) and leave these two deliberatly asymmetric.  I am leaning
> towards the latter myself.

I think to some degree that ship has already sailed (and is my fault!).

The ulterior motive here is to eventually unify the cat-file formatter
with the ref-filter formatter.  So for that we'll have to support
%(objectsize) anyway.

That still leaves the option of having %(objectsize:real) later and
marking a bare %(objectsize) as a deprecated synonym. But I don't think
there's any advantage to trying to deal with it at this stage.

-Peff

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC PATCH 1/5] ref-filter: add objectsize:disk option
  2018-11-12 12:03     ` Johannes Schindelin
@ 2018-11-12 13:12       ` Jeff King
  2018-11-13  1:52         ` Junio C Hamano
  0 siblings, 1 reply; 14+ messages in thread
From: Jeff King @ 2018-11-12 13:12 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Junio C Hamano, Olga Telezhnaya, git

On Mon, Nov 12, 2018 at 01:03:25PM +0100, Johannes Schindelin wrote:

> > oi.disk_size is off_t; do we know "long long" 
> > 
> >    (1) is available widely enough (I think it is from c99)?
> > 
> >    (2) is sufficiently wide so that we can safely cast off_t to?
> > 
> >    (3) will stay to be sufficiently wide as machines get larger
> >        together with standard types like off_t in the future?
> > 
> > I'd rather use intmax_t (as off_t is a signed integral type) so that
> > we do not have to worry about these questions in the first place.
> 
> You mean something like
> 
> 			v->s = xstrfmt("%"PRIdMAX, (intmax_t)oi->disk_size);

I think elsewhere we simply use PRIuMAX for printing large sizes via
off_t; we know this value isn't going to be negative.

I'm not opposed to PRIdMAX, which _is_ more accurate, but...

> P.S.: I wondered whether we have precedent for PRIdMAX, as we used to use
> only PRIuMAX, but yes: JeffH's json-writer uses PRIdMAX.

That's pretty recent. I won't be surprised if we have to do some
preprocessor trickery to handle that at some point. We have a PRIuMAX
fallback already. That comes from c4001d92be (Use off_t when we really
mean a file offset., 2007-03-06), but it's not clear to me if that was
motivated by a real platform or an over-abundance of caution.

I'm OK with just using PRIdMAX as appropriate for now. It will serve as
a weather-balloon, and we can #define our way out of it later if need
be.

-Peff

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC PATCH 1/5] ref-filter: add objectsize:disk option
  2018-11-12 13:12       ` Jeff King
@ 2018-11-13  1:52         ` Junio C Hamano
  2018-11-20  9:17           ` Оля Тележная
  0 siblings, 1 reply; 14+ messages in thread
From: Junio C Hamano @ 2018-11-13  1:52 UTC (permalink / raw)
  To: Jeff King; +Cc: Johannes Schindelin, Olga Telezhnaya, git

Jeff King <peff@peff.net> writes:

>> You mean something like
>> 
>> 			v->s = xstrfmt("%"PRIdMAX, (intmax_t)oi->disk_size);
>
> I think elsewhere we simply use PRIuMAX for printing large sizes via
> off_t; we know this value isn't going to be negative.
>
> I'm not opposed to PRIdMAX, which _is_ more accurate, but...
>
>> P.S.: I wondered whether we have precedent for PRIdMAX, as we used to use
>> only PRIuMAX, but yes: JeffH's json-writer uses PRIdMAX.
>
> That's pretty recent. I won't be surprised if we have to do some
> preprocessor trickery to handle that at some point. We have a PRIuMAX
> fallback already. That comes from c4001d92be (Use off_t when we really
> mean a file offset., 2007-03-06), but it's not clear to me if that was
> motivated by a real platform or an over-abundance of caution.
>
> I'm OK with just using PRIdMAX as appropriate for now. It will serve as
> a weather-balloon, and we can #define our way out of it later if need
> be.

I am OK if we avoid PRIdMAX and use PRIuMAX instead with a cast to
the corresponding size in this codepath, as long as we properly
handle negative oi.disk_size field, which may be telling some
"unusual" condition to us.



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC PATCH 1/5] ref-filter: add objectsize:disk option
  2018-11-13  1:52         ` Junio C Hamano
@ 2018-11-20  9:17           ` Оля Тележная
  2018-11-21  0:27             ` Junio C Hamano
  0 siblings, 1 reply; 14+ messages in thread
From: Оля Тележная @ 2018-11-20  9:17 UTC (permalink / raw)
  To: Junio C Hamano, Jeff King, Christian Couder, SZEDER Gábor
  Cc: Johannes Schindelin, git

вт, 13 нояб. 2018 г. в 04:52, Junio C Hamano <gitster@pobox.com>:
>
> Jeff King <peff@peff.net> writes:
>
> >> You mean something like
> >>
> >>                      v->s = xstrfmt("%"PRIdMAX, (intmax_t)oi->disk_size);
> >
> > I think elsewhere we simply use PRIuMAX for printing large sizes via
> > off_t; we know this value isn't going to be negative.
> >
> > I'm not opposed to PRIdMAX, which _is_ more accurate, but...
> >
> >> P.S.: I wondered whether we have precedent for PRIdMAX, as we used to use
> >> only PRIuMAX, but yes: JeffH's json-writer uses PRIdMAX.
> >
> > That's pretty recent. I won't be surprised if we have to do some
> > preprocessor trickery to handle that at some point. We have a PRIuMAX
> > fallback already. That comes from c4001d92be (Use off_t when we really
> > mean a file offset., 2007-03-06), but it's not clear to me if that was
> > motivated by a real platform or an over-abundance of caution.
> >
> > I'm OK with just using PRIdMAX as appropriate for now. It will serve as
> > a weather-balloon, and we can #define our way out of it later if need
> > be.
>
> I am OK if we avoid PRIdMAX and use PRIuMAX instead with a cast to
> the corresponding size in this codepath, as long as we properly
> handle negative oi.disk_size field, which may be telling some
> "unusual" condition to us.

Maybe we want to change the type (from off_t to unsigned) directly in
struct object_info? That will help us not to make additional
checkings. Or, at least, I suggest to add check to
oid_object_info_extended() so that this function will give a guarantee
that the size is non-negative. That will make code cleaner (otherwise
we need to add checks everywhere after oid_object_info_extended()
usage).

Please, look at this one also [1]. Thanks a lot!

[1] https://public-inbox.org/git/CAL21BmnoZuRih3Ky66_Tk0PweD36eZ6=fbY3jGumRcSJ=Bc_pQ@mail.gmail.com/

>
>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC PATCH 1/5] ref-filter: add objectsize:disk option
  2018-11-20  9:17           ` Оля Тележная
@ 2018-11-21  0:27             ` Junio C Hamano
  0 siblings, 0 replies; 14+ messages in thread
From: Junio C Hamano @ 2018-11-21  0:27 UTC (permalink / raw)
  To: Оля
	Тележная
  Cc: Jeff King, Christian Couder, SZEDER Gábor, Johannes Schindelin, git

Оля Тележная  <olyatelezhnaya@gmail.com> writes:

>> I am OK if we avoid PRIdMAX and use PRIuMAX instead with a cast to
>> the corresponding size in this codepath, as long as we properly
>> handle negative oi.disk_size field, which may be telling some
>> "unusual" condition to us.
>
> Maybe we want to change the type (from off_t to unsigned) directly in
> struct object_info? That will help us not to make additional
> checkings. Or, at least, I suggest to add check to
> oid_object_info_extended() so that this function will give a guarantee
> that the size is non-negative.

I am fine with the approach.  The potential gain of allowing
oi.disk_size is it would allow the code to say "I'll give these
pieces of info about the object, but the on-disk size is unknown"
without failing the whole object_info_extended() request.  And I
personally do not think such an ability is not all that important
or useful.


^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, back to index

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-09  7:37 [RFC PATCH 0/5] ref-filter: add new formatting options Оля Тележная
2018-11-09  7:44 ` [RFC PATCH 1/5] ref-filter: add objectsize:disk option Olga Telezhnaya
2018-11-09  7:44   ` [RFC PATCH 2/5] ref-filter: add tests for objectsize:disk Olga Telezhnaya
2018-11-09  7:44   ` [RFC PATCH 3/5] ref-filter: add deltabase option Olga Telezhnaya
2018-11-09  7:44   ` [RFC PATCH 5/5] ref-filter: add docs for new options Olga Telezhnaya
2018-11-12  5:24     ` Junio C Hamano
2018-11-09  7:44   ` [RFC PATCH 4/5] ref-filter: add tests for deltabase Olga Telezhnaya
2018-11-12  5:03   ` [RFC PATCH 1/5] ref-filter: add objectsize:disk option Junio C Hamano
2018-11-12 12:03     ` Johannes Schindelin
2018-11-12 13:12       ` Jeff King
2018-11-13  1:52         ` Junio C Hamano
2018-11-20  9:17           ` Оля Тележная
2018-11-21  0:27             ` Junio C Hamano
2018-11-12 13:05     ` Jeff King

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox