Re: [PATCH v3 20/23] ref-filter: unifying formatting of cat-file opts

[Junio C Hamano]

Оля Тележная   writes:

>>> - else if (deref)
>>> + } else if (!strcmp(name, "objectsize:disk")) {
>>> + if (cat_file_info.is_cat_file) {
>>> + v->value = cat_file_info.disk_size;
>>> + v->s = xstrfmt("%"PRIuMAX, 
>>> (uintmax_t)v->value);
>>> + }
>>> + } else if (deref)
>>
>> Why do we care about is_cat_file here. Shouldn't:
>>
>>   git for-each-ref --format='%(objectsize:disk)'
>>
>> work? I.e., shouldn't the cat_file_info.disk_size variable be held
>> somewhere in a used_atom struct?
>
> At that point - no.
> I think it sounds like other separate task to add this functionality
> and to test it properly.

What does "that point" refer to?  This point at 20th patch in the
23-patch series it is not premature, but it will become feasible in
later steps?

As Peff already said in his review on earlier steps like 4/23 and
7/23, I too found the series quite confusing and felt as if I was
watching somebody stumbling in all directions in the dark in the
earlier steps in the series before deciding to go in one direction.

Re: [PATCH v3 20/23] ref-filter: unifying formatting of cat-file opts

[Оля Тележная]

2018-02-15 8:56 GMT+03:00 Jeff King :
> On Mon, Feb 12, 2018 at 08:08:54AM +, Olga Telezhnaya wrote:
>
>> cat-file options are now filled by general logic.
>
> Yay.
>
> One puzzling thing:
>
>> diff --git a/ref-filter.c b/ref-filter.c
>> index 8d104b567eb7c..5781416cf9126 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -824,8 +824,12 @@ static void grab_common_values(struct atom_value *val, 
>> int deref, struct object
>>   else if (!strcmp(name, "objectsize")) {
>>   v->value = sz;
>>   v->s = xstrfmt("%lu", sz);
>> - }
>> - else if (deref)
>> + } else if (!strcmp(name, "objectsize:disk")) {
>> + if (cat_file_info.is_cat_file) {
>> + v->value = cat_file_info.disk_size;
>> + v->s = xstrfmt("%"PRIuMAX, 
>> (uintmax_t)v->value);
>> + }
>> + } else if (deref)
>
> Why do we care about is_cat_file here. Shouldn't:
>
>   git for-each-ref --format='%(objectsize:disk)'
>
> work? I.e., shouldn't the cat_file_info.disk_size variable be held
> somewhere in a used_atom struct?

At that point - no.
I think it sounds like other separate task to add this functionality
and to test it properly.

>
> -Peff

Re: [PATCH v3 20/23] ref-filter: unifying formatting of cat-file opts

[Jeff King]

On Mon, Feb 12, 2018 at 08:08:54AM +, Olga Telezhnaya wrote:

> cat-file options are now filled by general logic.

Yay.

One puzzling thing:

> diff --git a/ref-filter.c b/ref-filter.c
> index 8d104b567eb7c..5781416cf9126 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -824,8 +824,12 @@ static void grab_common_values(struct atom_value *val, 
> int deref, struct object
>   else if (!strcmp(name, "objectsize")) {
>   v->value = sz;
>   v->s = xstrfmt("%lu", sz);
> - }
> - else if (deref)
> + } else if (!strcmp(name, "objectsize:disk")) {
> + if (cat_file_info.is_cat_file) {
> + v->value = cat_file_info.disk_size;
> + v->s = xstrfmt("%"PRIuMAX, (uintmax_t)v->value);
> + }
> + } else if (deref)

Why do we care about is_cat_file here. Shouldn't:

  git for-each-ref --format='%(objectsize:disk)'

work? I.e., shouldn't the cat_file_info.disk_size variable be held
somewhere in a used_atom struct?

-Peff

[PATCH v3 20/23] ref-filter: unifying formatting of cat-file opts

[Olga Telezhnaya]

cat-file options are now filled by general logic.

Signed-off-by: Olga Telezhnaia 
Mentored-by: Christian Couder 
Mentored by: Jeff King 
---
 ref-filter.c | 33 -
 1 file changed, 16 insertions(+), 17 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 8d104b567eb7c..5781416cf9126 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -824,8 +824,12 @@ static void grab_common_values(struct atom_value *val, int 
deref, struct object
else if (!strcmp(name, "objectsize")) {
v->value = sz;
v->s = xstrfmt("%lu", sz);
-   }
-   else if (deref)
+   } else if (!strcmp(name, "objectsize:disk")) {
+   if (cat_file_info.is_cat_file) {
+   v->value = cat_file_info.disk_size;
+   v->s = xstrfmt("%"PRIuMAX, (uintmax_t)v->value);
+   }
+   } else if (deref)
grab_objectname(name, obj->oid.hash, v, _atom[i]);
}
 }
@@ -1465,21 +1469,7 @@ static int populate_value(struct ref_array_item *ref)
name++;
}
 
-   if (cat_file_info.is_cat_file) {
-   if (starts_with(name, "objectname"))
-   v->s = oid_to_hex(>oid);
-   else if (starts_with(name, "objecttype"))
-   v->s = typename(ref->type);
-   else if (starts_with(name, "objectsize")) {
-   v->s = xstrfmt("%lu", ref->size);
-   } else if (starts_with(name, "objectsize:disk")) {
-   v->s = xstrfmt("%"PRIuMAX, 
(uintmax_t)ref->disk_size);
-   } else if (starts_with(name, "rest"))
-   v->s = ref->rest;
-   else if (starts_with(name, "deltabase"))
-   v->s = xstrdup(oid_to_hex(ref->delta_base_oid));
-   continue;
-   } else if (starts_with(name, "refname"))
+   if (starts_with(name, "refname"))
refname = get_refname(atom, ref);
else if (starts_with(name, "symref"))
refname = get_symref(atom, ref);
@@ -1535,6 +1525,15 @@ static int populate_value(struct ref_array_item *ref)
else
v->s = " ";
continue;
+   } else if (starts_with(name, "rest")) {
+   v->s = ref->rest ? ref->rest : "";
+   continue;
+   } else if (starts_with(name, "deltabase")) {
+   if (ref->delta_base_oid)
+   v->s = xstrdup(oid_to_hex(ref->delta_base_oid));
+   else
+   v->s = "";
+   continue;
} else if (starts_with(name, "align")) {
v->handler = align_atom_handler;
continue;

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