Re: [PATCH Outreachy] mru: use double-linked list from list.h

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

I added "v2" after "PATCH", but it does not appeared. Actually it was
written automatically and it was "PATCH Outreachy v2". I rearranged it
in the middle of the phrase.

>> I forgot about leak. I also need to add checking in mru_clear. That's
>> not beautiful solution but it works reliably.
>
> I'm not sure what you mean about checking in mru_clear(). It may make
> sense to just send your v2 patch and I can see there what you do.

I realized that I said something strange before. I solved the problem
with leak by deleting INIT in prepare_packed_git and adding init as
you suggested here:
https://github.com/telezhnaya/git/commit/7f8995835949f83e54efdfd88445feb6d54cb3e9#commitcomment-24576103

By the way, I asked to edit FAQ for Linux kernel newbies about linked
list that confused me a week ago, and that funny picture was deleted.
https://kernelnewbies.org/FAQ/LinkedLists
Maybe it will help to someone else :)

Re: [PATCH v2 Outreachy] mru: use double-linked list from list.h

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

>> Simplify mru.[ch] and related code by reusing the double-linked list
>> implementation from list.h instead of a custom one.
>> This commit is an intermediate step. Our final goal is to get rid of
>> mru.[ch] at all and inline all logic.
>
> Thanks, this version looks correct to me.

Great! What is better - to complete my application now (and say only
about this patch) or to complete it in last days (and say about
everything that I've done. Maybe there would be something new).

> I do think there are a few ugly bits in the result (like that
> initializer for packed_git_mru :) ), so I'd prefer not to merge this
> down until we do that final step.
>
> So the big question is: who wants to do it?
>
> I think you've done a good job here, and this would count for your
> Outreachy application's contribution. But if you'd like to do that next
> step, you are welcome to.

I was thinking about starting my small research about main task of the
internship. I could postpone it and end with this task, there's no
problem for me.
Or, if someone from the newbies wants to have a small simple task -
it's a great opportunity for him/her. By the way, I am ready to help
if he/she will have questions or difficulties.
So please make a decision on your own, I will be happy in any scenario.

Re: [PATCH Outreachy] mru: use double-linked list from list.h

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

Hi everyone,
Many thanks to all of you, I am interested in every opinion. Sorry
that I wasn't in the discussion, unfortunately I got sick, that's why
I skipped all the process.
I want to reply to the main moments and also ask some questions.

>> Simplify mru.c, mru.h and related code by reusing the double-linked list 
>> implementation from list.h instead of a custom one.
> An overlong line (I can locally wrap it, so the patch does not have
> to be re-sent only to fix this alone).
I've read only about 50 characters max in commit head (and
highlighting repeats it), but there's nothing about max length of line
in commit message. Sorry, next time I will make it shorter.

About many different opinions how to improve the code: I agree with
the idea that my commit is a middle step to get rid of MRU at all. If
we really need to add initializer/mru_for_each/smth_else - it's
absolutely not a problem, but as it was said, not sure that we need
it.
It really looks that using list implementation from list.h directly
won't be worse.

> I had envisioned leaving mru_mark() as a wrapper for "move to the front"
> that could operate on any list. But seeing how Olga's patch takes it
> down to two trivial lines, I'd also be fine with an endgame that just
> eliminates it.
Let's add needed function to list.h directly? I also wanted to add
list_for_each_entry function to list.h as it's in Linux kernel.
https://www.kernel.org/doc/htmldocs/kernel-api/API-list-for-each-entry.html
It will simplify the code even more, guess that not only in MRU
related code. Maybe we need to do that in separate patch.

About minor issues ( "tmp" vs "p2", variable scope, space indentation)
- fully agree, I will fix it.

So finally I think that I need to fix that minor issues and that's
all. I have plans to rewrite (with --amend) my current commit (I think
so because I will add no new features, so it's better to have single
commit for all changes).
As I understand, Submitgit will send an update in a new thread. And I
need to say there [PATCH v2].
Please correct me if I am wrong in any of the moments mentioned earlier.

By the way, other contributors write smth like "[PATCH v6 0/3]". What
does mean "0/3"? It's about editing separate commits in a single
patch, am I right?

Thank you one more time!
Olga

Re: [PATCH Outreachy] mru: use double-linked list from list.h

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

> About minor issues ( "tmp" vs "p2", variable scope, space indentation)
> - fully agree, I will fix it.
> ...
> So finally I think that I need to fix that minor issues and that's
> all.

I forgot about leak. I also need to add checking in mru_clear. That's
not beautiful solution but it works reliably.
Question remains the same - do you see something more to fix in this patch?

Thank you!
Olga

[PATCH] [Outreachy] cleanup: use list.h in mru.h and mru.c

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

Remove implementation of double-linked list in mru.c and mru.h and use
implementation from list.h.

Signed-off-by: Olga Telezhnaia 
Mentored-by: Christian Couder , Jeff King

---
 builtin/pack-objects.c |  5 +++--
 mru.c  | 51 +++---
 mru.h  | 31 +-
 packfile.c |  6 --
 4 files changed, 35 insertions(+), 58 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index f721137ea..fb4c9be89 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -995,8 +995,8 @@ static int want_object_in_pack(const unsigned char *sha1,
 struct packed_git **found_pack,
 off_t *found_offset)
 {
- struct mru_entry *entry;
  int want;
+struct list_head *pos;

  if (!exclude && local && has_loose_object_nonlocal(sha1))
  return 0;
@@ -1012,7 +1012,8 @@ static int want_object_in_pack(const unsigned char *sha1,
  return want;
  }

- for (entry = packed_git_mru.head; entry; entry = entry->next) {
+ list_for_each(pos, _git_mru.list) {
+ struct mru *entry = list_entry(pos, struct mru, list);
  struct packed_git *p = entry->item;
  off_t offset;

diff --git a/mru.c b/mru.c
index 9dedae028..8b6ba3d9b 100644
--- a/mru.c
+++ b/mru.c
@@ -1,50 +1,29 @@
 #include "cache.h"
 #include "mru.h"

-void mru_append(struct mru *mru, void *item)
+void mru_append(struct mru *head, void *item)
 {
- struct mru_entry *cur = xmalloc(sizeof(*cur));
+ struct mru *cur = xmalloc(sizeof(*cur));
  cur->item = item;
- cur->prev = mru->tail;
- cur->next = NULL;
-
- if (mru->tail)
- mru->tail->next = cur;
- else
- mru->head = cur;
- mru->tail = cur;
+ list_add_tail(>list, >list);
 }

-void mru_mark(struct mru *mru, struct mru_entry *entry)
+void mru_mark(struct mru *head, struct mru *entry)
 {
- /* If we're already at the front of the list, nothing to do */
- if (mru->head == entry)
- return;
-
- /* Otherwise, remove us from our current slot... */
- if (entry->prev)
- entry->prev->next = entry->next;
- if (entry->next)
- entry->next->prev = entry->prev;
- else
- mru->tail = entry->prev;
-
- /* And insert us at the beginning. */
- entry->prev = NULL;
- entry->next = mru->head;
- if (mru->head)
- mru->head->prev = entry;
- mru->head = entry;
+ /* To mark means to put at the front of the list. */
+ list_del(>list);
+ list_add(>list, >list);
 }

-void mru_clear(struct mru *mru)
+void mru_clear(struct mru *head)
 {
- struct mru_entry *p = mru->head;
-
- while (p) {
- struct mru_entry *to_free = p;
- p = p->next;
+ struct list_head *p1;
+ struct list_head *p2;
+ struct mru *to_free;
+
+ list_for_each_safe(p1, p2, >list) {
+ to_free = list_entry(p1, struct mru, list);
  free(to_free);
  }
- mru->head = mru->tail = NULL;
+ INIT_LIST_HEAD(>list);
 }
diff --git a/mru.h b/mru.h
index 42e4aeaa1..36a332af0 100644
--- a/mru.h
+++ b/mru.h
@@ -1,6 +1,8 @@
 #ifndef MRU_H
 #define MRU_H

+#include "list.h"
+
 /**
  * A simple most-recently-used cache, backed by a doubly-linked list.
  *
@@ -8,18 +10,15 @@
  *
  *   // Create a list.  Zero-initialization is required.
  *   static struct mru cache;
- *   mru_append(, item);
- *   ...
+ *   INIT_LIST_HEAD();
  *
- *   // Iterate in MRU order.
- *   struct mru_entry *p;
- *   for (p = cache.head; p; p = p->next) {
- * if (matches(p->item))
- * break;
- *   }
+ *   // Add new item to the end of the list.
+ *   void *item;
+ *   ...
+ *   mru_append(, item);
  *
  *   // Mark an item as used, moving it to the front of the list.
- *   mru_mark(, p);
+ *   mru_mark(, item);
  *
  *   // Reset the list to empty, cleaning up all resources.
  *   mru_clear();
@@ -29,17 +28,13 @@
  * you will begin traversing the whole list again.
  */

-struct mru_entry {
- void *item;
- struct mru_entry *prev, *next;
-};
-
 struct mru {
- struct mru_entry *head, *tail;
+ struct list_head list;
+void *item;
 };

-void mru_append(struct mru *mru, void *item);
-void mru_mark(struct mru *mru, struct mru_entry *entry);
-void mru_clear(struct mru *mru);
+void mru_append(struct mru *head, void *item);
+void mru_mark(struct mru *head, struct mru *entry);
+void mru_clear(struct mru *head);

 #endif /* MRU_H */
diff --git a/packfile.c b/packfile.c
index f69a5c8d6..ae3b0b2e9 100644
--- a/packfile.c
+++ b/packfile.c
@@ -876,6 +876,7 @@ void prepare_packed_git(void)
  for (alt = alt_odb_list; alt; alt = alt->next)
  prepare_packed_git_one(alt->path, 0);
  rearrange_packed_git();
+ INIT_LIST_HEAD(_git_mru.list);
  prepare_packed_git_mru();
  prepare_packed_git_run_once = 1;
 }
@@ -1824,13 +1825,14 @@ static int fill_pack_entry(const unsigned char *sha1,
  */
 int find_pack_entry(const unsigned char *sha1, struct pack_entry *e)
 {
- struct mru_entry *p;
+ struct list_head *pos;

  prepare_packed_git();
  if (!packed_git)
  return 0;

- for (p = packed_git_mru.head; p; p = p->next) {
+ list_for_each(pos, _git_mru.list) {
+ 

Re: [PATCH v2 Outreachy] mru: use double-linked list from list.h

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

> We hung back on it to leave it as low-hanging fruit for other Outreachy
> applicants. Perhaps Olga would like to pick it up now that the
> application period is over.

It's absolutely not a problem for me, I can do that as one more
warm-up exercise in the beginning of the internship.

Thanks!

Re: [PATCH Outreachy 1/2] format: create pretty.h file

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

> I see you've "standardized" to drop "extern" from the declarations
> in the header; I have an impression that our preference however is
> to go in the other direction.

OK, absolutely not a problem, I will return them. Do I need to write
"extern" further in function declarations? And why did everyone choose
writing "extern" every time? It looks obvious for me that declaration
of function is extern, that's why I decided to throw them away.


> The choice of bits that are moved to the new header looks quite
> sensible to me.

I'm very happy and satisfied with it :-)


> s/futher/further/

It was a typo that I missed. Thank you! Will fix it also.


> This has a toll on topics in flight that expect the symbols for
> pretty are available in "commit.h"; they are forced to include
> this new file they did not even know about.
>
> I notice that "commit.h" is included in "builtin.h"; perhaps adding
> a new include for "pretty.h" there would be of lessor impact?  I
> dunno.
>

It's a middle point, as I said. I have plans to create unifying
format.h then (for all formatting issues). I guess that pretty.h and
ref-filter.h will be deleted later. But, I really need to create now
that pretty.h because it is much easier to work with existing
interface. If you have another ideas how to achieve the main goal -
please share them with me, I would appreciate that so much. I am not
sure that my solution is the best, but I can't come up with something
better for now.

Re: [PATCH Outreachy 1/2] format: create pretty.h file

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

Is it true that I need to fix only one commit message? (a typo
s/futher/further/)

Do you have any other advises what do I need to change?

Thanks!

Re: [PATCH] mru: Replace mru.[ch] with list.h implementation

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

Gargi,
If you have some difficulties - please feel free to ask questions
(here or you can write me directly). I will be happy to help you.
As I understand, you are super close to finish your first patch.

Olga

[PATCH RFC 0/1] ref-filter: start using oid_object_info

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

Hello everyone,
Some context at first: I am trying to migrate cat-file formatting part
(I mean batch option) from its own implementation to ref-filter one.
That's why I want to use oid_object_info_extended() in ref-filter, it
will simplify my further work. Additionally, it will give us 2 new
options (objectsize:disk and deltabase), and hopefully some commands
could work faster because we will not get and parse whole object if we
don't need it (invocation of oid_object_info_extended() is cheaper).

I am not sure if I need to add same logic for refs (I mean when we
need to deref the object) - please share your opinion.

I also see the problem with sorting - that's the part of logic that I
am trying not to touch at all. Data (from oid_object_info_extended())
is initialized in format_ref_array_item(), and we use it later in
populate value(). We have get_ref_atom_value(), it uses
populate_value(). The problem is that get_ref_atom_value() is used not
only by format_ref_array_item(), but also by cmp_ref_sorting(). So, it
means that data could be uninitialized in populate_value(). Now I do
nothing with it, and it works fine because cmp_ref_sorting() works
only with refs, and my code do not change refs logic for now. But it's
bad, and I know about it.

I see 2 ways of fixing that: I need to support refs somehow; or, I
need to add additional checks whether the data is initialized before.
I will be happy to hear your opinion.

If you see other issues/ideas - please share your opinion, I
appreciate it so much.

Thanks a lot!
Olga

[PATCH RFC v2 0/4] ref-filter: start using oid_object_info

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

Hello,
Discussion starts here: [1], [2].

Updates:

I fixed memory leaks, now we use data from oid_object_info() OR from
grab_commom_values() - not both as it was before.

I added support for "%(objectsize:disk)" atom (with tests) and for
"%(deltabase)" atom (without tests yet).
I didn't support "%(*objectsize:disk)" and "%(*deltabase)" because I
can't see the possibility to fill these fields at the same time with
other processes.
I really want to discuss this moment with someone. The best idea that
I have is to fully change grab_common_values() function so that it
would collect and fill all info from oid_object_info_extended(), and
we will invoke it before we actually get and parse full object.

I am not sure with this proposed solution at all: we would still have
the scenario when we call both oid_object_info_extended() and
get_object(), it means the performance will be worse in some cases. I
don't know if it's crucial.

By the way, do we have an official way to measure performance?

I didn't add tests for %(deltabase) atom because I am not sure that it
has sense for for-each-ref at all: I have a guess that I added this
atom only for future using in cat-file command, and also for deref
option %(*deltabase).

I was also thinking about adding new parameter to atom, something like
enum "need oid_object_info", "need get_object" etc, so that we could
collect all needed data in a shortest way. The problem is that the
code will be even more complicated, and I am not sure our goal worth
it. And, anyway, there would be the scenario when we have to call both
oid_object_info_extended() and get_object(): for example, if we have
format="%(objectsize:disk) %(author)".

Thanks a lot,
Waiting for any ideas/comments!

[1] 
https://public-inbox.org/git/CAL21Bm=6Z54-zsUq0DJqmqhSciHCDLUNXR8inDMAd-b-=qj...@mail.gmail.com/
[2] https://public-inbox.org/git/xmqq8t8la81a@gitster-ct.c.googlers.com/

Re: Rewrite cat-file.c : need help to find a bug

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

2017-12-29 16:22 GMT+03:00 Jeff King <p...@peff.net>:
> On Fri, Dec 29, 2017 at 01:05:50PM +0300, Оля Тележная wrote:
>
>> Hi everyone,
>> I am trying to reuse formatting logic from ref-filter in cat-file
>> command. Now cat-file uses its own formatting code.
>> I am trying to achieve that step-by-step, now I want to invoke
>> populate_value function, and I have a bug somewhere.
>> My code is here.
>> https://github.com/telezhnaya/git/commits/catfile
>> All commits that starts from "cat-file: " are successfully passing all tests.
>> So for now my last commit fails, and I am tired of searching for the
>> error. Actually, I just copied some values to another variable and
>> move some code to other place. All "intelligent" work will go further,
>> but now I need to fix current situation.
>
> The problem is that "cat_file_info" is NULL when you get to
> populate_value(), so the moved code doesn't trigger.
>

 Thanks a lot!

Re: Rewrite cat-file.c : need help to find a bug

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

2017-12-29 17:04 GMT+03:00 Оля Тележная <olyatelezhn...@gmail.com>:
> 2017-12-29 16:22 GMT+03:00 Jeff King <p...@peff.net>:
>> On Fri, Dec 29, 2017 at 01:05:50PM +0300, Оля Тележная wrote:
>>
>>> Hi everyone,
>>> I am trying to reuse formatting logic from ref-filter in cat-file
>>> command. Now cat-file uses its own formatting code.
>>> I am trying to achieve that step-by-step,

Happy to say that my previous bug is fixed, and I am struggling with
the next one.
Now I want to get rid of using expand_data structure (I took it from
cat-file.c to simplify migrating process and now it's time to delete
it).

>>> My code is here.
>>> https://github.com/telezhnaya/git/commits/catfile
>>> All commits that starts from "cat-file: " are successfully passing all 
>>> tests.

So for now 2 of my last commits fail, and I am tired of searching for the error.
I was also trying to leave cat_file_info variable and fill in both new
and old variables and then compare resulting values by printing them
into file. Everything is OK, but I find it dudpicious that the
resulting file is too small (fprintf was invoked only 3 times, it was
here: 
https://github.com/telezhnaya/git/commit/54a5b5a0167ad634c26e4fd7df234a46286ede0a#diff-2846189963e8aec1bcb559b69b7f20d0R1489)

I have left few comments in github to simplify your understanding what
I was trying to achieve. Feel free to ask any questions if you find
the code strange, unclear or suspicious.
Thanks!

Olga

Re: [PATCH v2 03/18] ref-filter: make valid_atom as function parameter

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

2018-01-16 0:42 GMT+03:00 Jeff King :
> On Wed, Jan 10, 2018 at 09:36:41AM +, Olga Telezhnaya wrote:
>
>> Make valid_atom as a function parameter,
>> there could be another variable further.
>> Need that for further reusing of formatting logic in cat-file.c.
>>
>> We do not need to allow users to pass their own valid_atom variable in
>> global functions like verify_ref_format because in the end we want to
>> have same set of valid atoms for all commands. But, as a first step
>> of migrating, I create further another version of valid_atom
>> for cat-file.
>
> I agree in the end we'd want a single valid_atom list. It doesn't look
> like we hit that end state in this series, though.
>
> I guess I'm not quite clear on why we're not adding these new atoms to
> ref-filter (and for-each-ref) right away, though. We already have the
> first three (name, type, and size), and we'd just need to support
> %(rest) and %(deltabase).
>
> I think %(rest) doesn't really make sense for for-each-ref (we're not
> reading any input), but it could expand to the empty string by default
> (or even throw an error if the caller asks us not to support it).
>
> IOW, the progression I'd expect in a series like this is:
>
>   1. Teach ref-filter.c to support everything that cat-file can do.
>
>   2. Convert cat-file to use ref-filter.c.
>
> -Peff

I agree, I even made this and it's working fine:
https://github.com/git/git/pull/450/commits/1b74f1047f07434dccb207534d1ad45a143e3f2b
But I decided not to add that to patch because I expand the
functionality of several commands (not only cat-file and
for-each-ref), and I need to support all new functionality in a proper
way, make these error messages, test everything and - the hardest one
- support many new commands for cat-file. As I understand, it is not
possible unless we finally move to ref-filter and print results also
there. Oh, and I also need to rewrite docs in that case. And I decided
to apply this in another patch. But, please, say your opinion, maybe
we could do that here in some way.

Olga

Re: [PATCH v2 04/18] cat-file: move struct expand_data into ref-filter

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

2018-01-16 0:44 GMT+03:00 Jeff King :
> On Wed, Jan 10, 2018 at 09:36:41AM +, Olga Telezhnaya wrote:
>
>> Need that for further reusing of formatting logic in cat-file.
>> Have plans to get rid of using expand_data in cat-file at all,
>> and use it only in ref-filter for collecting, formatting and printing
>> needed data.
>
> I think some of these will want to remain in cat-file.c. For instance,
> split_on_whitespace is not something that ref-filter.c itself would care
> about. I'd expect in the endgame for cat-file to keep its own
> split_on_whitespace flag, and to set it based on the presence of the
> %(rest) flag, which it can check by seeing if the "rest" atom is in the
> "used_atom" list after parsing the format.
>
> -Peff

Yes, maybe we will need to leave some part of expand_data variables.
But, if it would be only "split_on_whitespace", it's better to make
just one flag without any other stuff. Actually, I thought about
moving related logic to ref-filter also. Anyway, it's hard to say
exact things before we code everything. Do I need to fix commit
message and make it more soft?

Olga

Re: [PATCH v2 01/18] cat-file: split expand_atom into 2 functions

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

2018-01-16 1:09 GMT+03:00 Jeff King :
> On Mon, Jan 15, 2018 at 04:33:35PM -0500, Jeff King wrote:
>
>> That works, but I don't think it's where we want to end up in the long
>> run.

I absolutely agree, I want to merge current edits and then continue
migrating process. And hopefully second part of the function will also
be removed.

>> Because:
>>
>>   1. We still have the set of formats duplicated between expand_atom()
>>  and the "preparation" step. It's just that the preparation is now
>>  in ref-filter.c. What happens if ref-filter.c learns new formatting
>>  placeholders (or options for those placeholders) that cat-file.c
>>  doesn't, or vice versa? The two have to be kept in sync.
>>
>>   2. We're missing out on all of the other placeholders that ref-filter
>>  knows about. Not all of them are meaningful (e.g., %(refname)
>>  wouldn't make sense here), but part of our goal is to support the
>>  same set of placeholders as much as possible. Some obvious ones
>>  that ought to work for cat-file: %(objectname:short), %(if), and
>>  things like %(subject) when the appropriate object type is used.
>>
>> In other words, I think the endgame is that expand_atom() isn't there at
>> all, and we're calling the equivalent of format_ref_item() for each
>> object (except that in a unified formatting world, it probably doesn't
>> have the word "ref" in it, since that's just one of the items a caller
>> might pass in).

Agree! I want to merge current edits, then create format.h file and
make some renames, then finish migrating process to new format.h and
support all new meaningful tags.

>
> I read carefully up through about patch 6, and then skimmed through the
> rest. I think we really want to push this in the direction of more
> unification, as above. I know that the patches here may be a step on the
> way, but I had a hard time evaluating each patch to see if it was
> leading us in the right direction.
>
> I think what would help for reviewing this is:
>
>   1. Start with a cover letter that makes it clear what the end state of
>  the series is, and why that's the right place to end up.
>
>   2. Include a rough roadmap of the patches in the cover letter.
>  Hopefully they should group into a few obvious steps (like "in
>  patches 1-5, we're teaching ref-filter to support everything that
>  cat-file can do, then in 6-10 we're preparing cat-file for the
>  conversion, and then in 11 we do the conversion"). If it doesn't
>  form a coherent narrative, then it may be worth stepping back and
>  thinking about combining or reordering the patches in a different
>  way, so that the progression becomes more plain.
>
>  I think one of the things that makes the progression here hard to
>  understand (for me, anyway) is that it's "inside out" of what I'd
>  expect. There's a lot of code movement happening first, and then
>  refactoring and simplifying after that. So between those two steps,
>  there's a lot of interim ugliness (e.g., having to reach across
>  module boundaries to look at expand_data). It's hard to tell when
>  looking at each individual patch how necessary the ugliness is, and
>  whether and when it's going to go away later in the series.
>
> There's a lot of good work here, and you've figured out a lot about how
> the two systems function. I think we just need to rearrange things a bit
> to make sure each step is moving in the right direction.
>
> -Peff

As I said, I am sure that I will continue working on that, so this is
not the end state. So I am not sure that I am able to write finalizing
messages for now. But, if we merge current edits, it will be much
easier for me to continue working on that (next patch would be about
creating format.h and I am afraid of some merge conflicts if I will
develop absolutely all process from start to finish in my branch, it
takes time. It's not a big problem, but, if we find final goal
worthwhile, so maybe we could go to it step-by-step).

Re: [PATCH v2 02/18] cat-file: reuse struct ref_format

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

2018-01-16 0:37 GMT+03:00 Jeff King :
> On Wed, Jan 10, 2018 at 09:36:41AM +, Olga Telezhnaya wrote:
>
>> Start using ref_format struct instead of simple char*.
>> Need that for further reusing of formatting logic from ref-filter.
>
> OK, this makes sense (though again, at some point we want this to stop
> being a "ref_format" and just be a "format").
>
>>  struct batch_options {
>> + struct ref_format *format;
>
> Does this need to be a pointer? We can just store the ref_format inside
> the struct, right? And then...
>
>> @@ -557,7 +558,8 @@ int cmd_cat_file(int argc, const char **argv, const char 
>> *prefix)
>>  {
>>   int opt = 0;
>>   const char *exp_type = NULL, *obj_name = NULL;
>> - struct batch_options batch = {0};
>> + struct ref_format format = REF_FORMAT_INIT;
>> + struct batch_options batch = {};
>>   int unknown_type = 0;
>
> ...here you would not need the extra local variable. You can initialize
> it like:
>
>   struct batch_options batch = { REF_FORMAT_INIT };
>
> -Peff

Thanks a lot!
Fixed, please check new version here: https://github.com/git/git/pull/450
If everything else is OK, I will send it to the mailing list.
As I said in other email threads, not sure that we need to include
last commit ("make valid_atom general again") into a new patch.

Thanks again,
Olga

Re: [PATCH v2 03/18] ref-filter: make valid_atom as function parameter

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

2018-01-18 17:23 GMT+03:00 Christian Couder <christian.cou...@gmail.com>:
> On Thu, Jan 18, 2018 at 12:49 PM, Оля Тележная <olyatelezhn...@gmail.com> 
> wrote:
>> 2018-01-18 9:20 GMT+03:00 Оля Тележная <olyatelezhn...@gmail.com>:
>>>
>>> I think it's important to finish migrating process at first. I mean,
>>> now we are preparing and collecting everything in ref-filter, but we
>>> make resulting string and print still in cat-file. And I am not sure,
>>> but maybe it will not be possible to start using new atoms in cat-file
>>> while some part of logic still differs.
>>
>> I tried to make that part here:
>> https://github.com/telezhnaya/git/commit/19a148614f1d4db1f8e628eb4e6d7c819d2da875
>> I know that the code is disgusting and there is a memory leak :) I
>> just try to reuse ref-filter logic, I will cleanup everything later.
>> At first, I try to make it work.
>> The problem is that I have segfault, and if I use gdb, I get:
>>
>> Program received signal SIGSEGV, Segmentation fault.
>> 0x in ?? ()
>
> Make sure that you compile with debug options like -g3. For example I use:
>
> $ make -j 4 DEVELOPER=1 CFLAGS="-g3"

Is it OK that I get different test results with simple make and with
make with all that flags?
Have a code: https://github.com/telezhnaya/git/commits/catfile
I do:

olya@ubuntu17-vm:~/git$ make install
olya@ubuntu17-vm:~/git$ cd t
olya@ubuntu17-vm:~/git/t$ ./t1006-cat-file.sh

And I have 17 tests broken.
Then, without any changes in code, I do:

olya@ubuntu17-vm:~/git$ make -j 4 DEVELOPER=1 CFLAGS="-g3" install
olya@ubuntu17-vm:~/git$ cd t
olya@ubuntu17-vm:~/git/t$ ./t1006-cat-file.sh

And there is 42 tests broken.
And it's really hard to search for errors in such situation.

>
>> I tried to google it, it's my first time when I get that strange
>> message, and unfortunately find nothing. So please explain me the
>> reason, why I can't find a place of segfault that way.
>
> I get the following:
>
> (gdb) run cat-file --batch < myarg.txt
> Starting program: /home/ubuntu/bin/git cat-file --batch < myarg.txt
> [Thread debugging using libthread_db enabled]
> Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
>
> Program received signal SIGSEGV, Segmentation fault.
> 0x556ea7cf in format_ref_array_item (info=0x7fffd460,
> format=0x7fffd6e0,
> final_buf=0x7fffd410) at ref-filter.c:2234
> 2234atomv->handler(atomv, );
> (gdb) bt
> #0  0x556ea7cf in format_ref_array_item (info=0x7fffd460,
> format=0x7fffd6e0, final_buf=0x7fffd410) at ref-filter.c:2234
> #1  0x556ea91c in show_ref_array_item (info=0x7fffd460,
> format=0x7fffd6e0)
> at ref-filter.c:2256
> #2  0x55577ef7 in batch_object_write (
> obj_name=0x55a66770 "5e1c309dae7f45e0f39b1bf3ac3cd9db12e7d689",
> opt=0x7fffd6e0, data=0x7fffd5e0) at builtin/cat-file.c:298

Re: [PATCH v2 03/18] ref-filter: make valid_atom as function parameter

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

2018-01-19 20:14 GMT+03:00 Christian Couder <christian.cou...@gmail.com>:
> On Thu, Jan 18, 2018 at 7:20 AM, Оля Тележная <olyatelezhn...@gmail.com> 
> wrote:
>> 2018-01-18 1:39 GMT+03:00 Christian Couder <christian.cou...@gmail.com>:
>>> On Wed, Jan 17, 2018 at 10:43 PM, Jeff King <p...@peff.net> wrote:
>>>> On Tue, Jan 16, 2018 at 09:55:22AM +0300, Оля Тележная wrote:
>>>>
>>>>> > IOW, the progression I'd expect in a series like this is:
>>>>> >
>>>>> >   1. Teach ref-filter.c to support everything that cat-file can do.
>>>>> >
>>>>> >   2. Convert cat-file to use ref-filter.c.
>>>>>
>>>>> I agree, I even made this and it's working fine:
>>>>> https://github.com/git/git/pull/450/commits/1b74f1047f07434dccb207534d1ad45a143e3f2b
>>>
>>> (Nit: it looks like the above link does not work any more, but it
>>> seems that you are talking about the last patch on the catfile
>>> branch.)
>>>
>>>>> But I decided not to add that to patch because I expand the
>>>>> functionality of several commands (not only cat-file and
>>>>> for-each-ref), and I need to support all new functionality in a proper
>>>>> way, make these error messages, test everything and - the hardest one
>>>>> - support many new commands for cat-file. As I understand, it is not
>>>>> possible unless we finally move to ref-filter and print results also
>>>>> there. Oh, and I also need to rewrite docs in that case. And I decided
>>>>> to apply this in another patch. But, please, say your opinion, maybe
>>>>> we could do that here in some way.
>>>>
>>>> Yeah, I agree that it will cause changes to other users of ref-filter,
>>>> and you'd need to deal with documentation and tests there. But I think
>>>> we're going to have to do those things eventually (since supporting
>>>> those extra fields everywhere is part of the point of the project). And
>>>> by doing them now, I think it can make the transition for cat-file a lot
>>>> simpler, because we never have to puzzle over this intermediate state
>>>> where only some of the atoms are valid for cat-file.
>>>
>>> I agree that you will have to deal with documentation and tests at one
>>> point and that it could be a good idea to do that now.
>>>
>>> I wonder if it is possible to add atoms one by one into ref-filter and
>>> to add tests and documentation at the same time, instead of merging
>>> cat-file atoms with ref-filter atoms in one big step.
>>>
>>> When all the cat-file atoms have been added to ref-filter's
>>> valid_atom, maybe you could add ref-filter's atoms to cat-file's
>>> valid_cat_file_atom one by one and add tests and documentation at the
>>> same time.
>>>
>>> And then when ref-filter's valid_atom and cat-file's
>>> valid_cat_file_atom are identical you can remove cat-file's
>>> valid_cat_file_atom and maybe after that rename "ref-filter" to
>>> "format".
>>
>> I think it's important to finish migrating process at first. I mean,
>> now we are preparing and collecting everything in ref-filter, but we
>> make resulting string and print still in cat-file. And I am not sure,
>> but maybe it will not be possible to start using new atoms in cat-file
>> while some part of logic still differs.
>
> Ok, you can finish the migration process then.
>
>> And another thoughts here - we were thinking about creating format.h
>> but decided not to move forward with it, and now we are suffering
>> because of it. Can I create it right now or the history of commits
>> would be too dirty because of it?
>
> It would also make it difficult to refactor your patch series if there
> is a big move or renaming in the middle.
>
>> Also, do you mean just renaming of
>> ref-filter? I was thinking that I need to put formatting-related logic
>> to another file and leave all other stuff in ref-filter.
>
> Yeah, you can do both a move and a renaming.

Thanks for a response! That thought is not clear enough for me. Do you
want me to split ref-filter into 2 files (one is for formatting only
called format and other one is for anything else still called
ref-filter) - here is a second question by the way, do I need to
create only format.h (and leave all realizations in ref-filter.c), or
I also need to create format.c. Or, just to rename ref-filter into
format and that's all.

>
>&

Re: Please review my code

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

2018-01-25 23:22 GMT+03:00 Christian Couder <christian.cou...@gmail.com>:
> Hi Olga,
>
> On Thu, Jan 25, 2018 at 6:20 PM, Оля Тележная <olyatelezhn...@gmail.com> 
> wrote:
>> Hi everyone,
>> I haven't sent the code by mailing lists because 25 commits (every
>> commit in separate message) look like a spam.
>
> Yeah, so now that you added tests, it might be interesting to see if
> the patch series can be refactored to be shorter or to be clearer.
>
>> Please look at my code:
>> https://github.com/telezhnaya/git/commits/catfile
>> You could send me any ideas here or in Github.
>
> I left some comments on GitHub. My main suggestion is to try to get
> rid of the is_cat global and if possible to remove the "struct
> expand_data *cat_file_info" global.

Thanks for your comments, I find them very useful. Most of issues are
fixed except the main one, that you mentioned here :)
I left the comment on GitHub.
https://github.com/telezhnaya/git/commit/403ab584fbf543acef1ecdf279ce31f4fc2ced3f

Thanks again!
Olga

>
>> The main idea of the patch is to get rid of using custom formatting in
>> cat-file and start using general one from ref-filter.
>> Additional bonus is that cat-file becomes to support many new
>> formatting commands like %(if), %(color), %(committername) etc.
>
> Yeah, that is a really nice result.
>
>> I remember I need to rewrite docs, I will do that in the near future.
>
> Great, thanks,
> Christian.

Re: Please review my code

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

2018-01-26 19:42 GMT+03:00 Christian Couder <christian.cou...@gmail.com>:
> On Fri, Jan 26, 2018 at 11:32 AM, Оля Тележная <olyatelezhn...@gmail.com> 
> wrote:
>> 2018-01-25 23:22 GMT+03:00 Christian Couder <christian.cou...@gmail.com>:
>>> On Thu, Jan 25, 2018 at 6:20 PM, Оля Тележная <olyatelezhn...@gmail.com> 
>>> wrote:
>>>> Please look at my code:
>>>> https://github.com/telezhnaya/git/commits/catfile
>>>> You could send me any ideas here or in Github.
>>>
>>> I left some comments on GitHub. My main suggestion is to try to get
>>> rid of the is_cat global and if possible to remove the "struct
>>> expand_data *cat_file_info" global.
>>
>> Thanks for your comments, I find them very useful. Most of issues are
>> fixed except the main one, that you mentioned here :)
>
> Ok, no problem, we will see what happens on the mailing list.
>
> It looks like the for-each-ref documentation has not been changed though.

Have you seen last commit? I updated cat-file documentation and I
mentioned why I decided not to touch for-each-ref docs. Please share
with me any ideas how can I make that place better.

>
> Otherwise it looks good to me and perhaps you could send your series
> to the mailing list even if it's long. For the first version, you may
> want to add "RFC" in the subject of the patch emails you send.

Great, thanks, I will send it now.
Olga

>
> Thanks,

Re: [PATCH v2 01/18] cat-file: split expand_atom into 2 functions

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

2018-01-18 2:04 GMT+03:00 Christian Couder <christian.cou...@gmail.com>:
> On Wed, Jan 17, 2018 at 10:49 PM, Jeff King <p...@peff.net> wrote:
>> On Tue, Jan 16, 2018 at 10:22:23AM +0300, Оля Тележная wrote:
>>
>>> >> In other words, I think the endgame is that expand_atom() isn't there at
>>> >> all, and we're calling the equivalent of format_ref_item() for each
>>> >> object (except that in a unified formatting world, it probably doesn't
>>> >> have the word "ref" in it, since that's just one of the items a caller
>>> >> might pass in).
>>>
>>> Agree! I want to merge current edits, then create format.h file and
>>> make some renames, then finish migrating process to new format.h and
>>> support all new meaningful tags.
>>
>> I think we have a little bit of chicken and egg there, though. I'm
>> having trouble reviewing the current work, because it's hard to evaluate
>> whether it's doing the right thing without seeing the end state.
>
> Yeah, to me it feels like you are at a middle point and there are many
> ways to go forward.

OK. Maybe I misunderstood you and Jeff in our call, I thought that was
your idea to make a merge now, sorry. I will continue my work here.

>
> As I wrote in another email though, I think it might be a good time to
> consolidate new functionality by adding tests (and perhaps
> documentation at the same time) for each new atom that is added to
> ref-filter or cat-file. It will help you refactor the code and your
> patch series later without breaking the new functionality.
>
>> So what
>> I was suggesting in my earlier mails was that we actually _not_ try to
>> merge this series, but use its components and ideas to build a new
>> series that does things in a bit different order.
>
> Yeah, I think you will have to do that, but the tests that you can add
> now for the new features will help you when you will build the new
> series.
>
> And hopefully it will not be too much work to create this new series
> as you will perhaps be able to just use the interactive rebase to
> build it.
>
> I also don't think it's a big problem if the current patch series gets
> quite long before you start creating a new series.

Re: [PATCH v2 04/18] cat-file: move struct expand_data into ref-filter

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

2018-01-18 0:45 GMT+03:00 Jeff King <p...@peff.net>:
> On Tue, Jan 16, 2018 at 10:00:42AM +0300, Оля Тележная wrote:
>
>> > I think some of these will want to remain in cat-file.c. For instance,
>> > split_on_whitespace is not something that ref-filter.c itself would care
>> > about. I'd expect in the endgame for cat-file to keep its own
>> > split_on_whitespace flag, and to set it based on the presence of the
>> > %(rest) flag, which it can check by seeing if the "rest" atom is in the
>> > "used_atom" list after parsing the format.
>>
>> Yes, maybe we will need to leave some part of expand_data variables.
>> But, if it would be only "split_on_whitespace", it's better to make
>> just one flag without any other stuff. Actually, I thought about
>> moving related logic to ref-filter also. Anyway, it's hard to say
>> exact things before we code everything. Do I need to fix commit
>> message and make it more soft?
>
> Yeah, I think it might just be split_on_whitespace, and that could live
> on as a single variable in cat-file.c.
>
> I don't think it makes sense to move that part to ref-filter, since it's
> not about formatting at all. It's about how cat-file parses its input,
> which is going to be unlike other ref-filter users (which don't
> generally parse input at all).
>
> -Peff

OK, I will leave split_on_whitespace in cat-file for now. :) There are
so many places that are more important now, in my opinion, so I am
planning just to leave this variable and do other stuff, and I will
return to this question later.

Re: [PATCH v2 03/18] ref-filter: make valid_atom as function parameter

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

2018-01-18 1:39 GMT+03:00 Christian Couder <christian.cou...@gmail.com>:
> On Wed, Jan 17, 2018 at 10:43 PM, Jeff King <p...@peff.net> wrote:
>> On Tue, Jan 16, 2018 at 09:55:22AM +0300, Оля Тележная wrote:
>>
>>> > IOW, the progression I'd expect in a series like this is:
>>> >
>>> >   1. Teach ref-filter.c to support everything that cat-file can do.
>>> >
>>> >   2. Convert cat-file to use ref-filter.c.
>>>
>>> I agree, I even made this and it's working fine:
>>> https://github.com/git/git/pull/450/commits/1b74f1047f07434dccb207534d1ad45a143e3f2b
>
> (Nit: it looks like the above link does not work any more, but it
> seems that you are talking about the last patch on the catfile
> branch.)
>
>>> But I decided not to add that to patch because I expand the
>>> functionality of several commands (not only cat-file and
>>> for-each-ref), and I need to support all new functionality in a proper
>>> way, make these error messages, test everything and - the hardest one
>>> - support many new commands for cat-file. As I understand, it is not
>>> possible unless we finally move to ref-filter and print results also
>>> there. Oh, and I also need to rewrite docs in that case. And I decided
>>> to apply this in another patch. But, please, say your opinion, maybe
>>> we could do that here in some way.
>>
>> Yeah, I agree that it will cause changes to other users of ref-filter,
>> and you'd need to deal with documentation and tests there. But I think
>> we're going to have to do those things eventually (since supporting
>> those extra fields everywhere is part of the point of the project). And
>> by doing them now, I think it can make the transition for cat-file a lot
>> simpler, because we never have to puzzle over this intermediate state
>> where only some of the atoms are valid for cat-file.
>
> I agree that you will have to deal with documentation and tests at one
> point and that it could be a good idea to do that now.
>
> I wonder if it is possible to add atoms one by one into ref-filter and
> to add tests and documentation at the same time, instead of merging
> cat-file atoms with ref-filter atoms in one big step.
>
> When all the cat-file atoms have been added to ref-filter's
> valid_atom, maybe you could add ref-filter's atoms to cat-file's
> valid_cat_file_atom one by one and add tests and documentation at the
> same time.
>
> And then when ref-filter's valid_atom and cat-file's
> valid_cat_file_atom are identical you can remove cat-file's
> valid_cat_file_atom and maybe after that rename "ref-filter" to
> "format".

I think it's important to finish migrating process at first. I mean,
now we are preparing and collecting everything in ref-filter, but we
make resulting string and print still in cat-file. And I am not sure,
but maybe it will not be possible to start using new atoms in cat-file
while some part of logic still differs.
And another thoughts here - we were thinking about creating format.h
but decided not to move forward with it, and now we are suffering
because of it. Can I create it right now or the history of commits
would be too dirty because of it? Also, do you mean just renaming of
ref-filter? I was thinking that I need to put formatting-related logic
to another file and leave all other stuff in ref-filter.

Anyway, your suggested steps looks good, and I am planning to
implement them later. Let's discuss, what behavior we are waiting for
when atom seems useless for the command. Die or ignore? And, which
atoms are useless (as I understand, "rest" and "deltabase" from
cat-file are useless for all ref-filter users, so the question is
about - am I right in it, and about ref-filter atoms for cat-file).

I have never written any tests and docs for Git, I will try to explore
by myself how to do that, but if you have any special links/materials
about it - please send them to me :)

Olga

Please review my code

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

Hi everyone,
I haven't sent the code by mailing lists because 25 commits (every
commit in separate message) look like a spam.

Please look at my code:
https://github.com/telezhnaya/git/commits/catfile
You could send me any ideas here or in Github.

The main idea of the patch is to get rid of using custom formatting in
cat-file and start using general one from ref-filter.
Additional bonus is that cat-file becomes to support many new
formatting commands like %(if), %(color), %(committername) etc.

I remember I need to rewrite docs, I will do that in the near future.

I would be happy to get any ideas from you.
Thanks!
Olga

Re: [PATCH 03/20] cat-file: rename variables in ref-filter

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

2018-01-10 1:04 GMT+03:00 Junio C Hamano :
> Olga Telezhnaya  writes:
>
>> Rename some variables for easier reading.
>> They point not to values, but to arrays.
>
> Once the code is written and people start to build on top, a change
> like this is not worth the code churn, especially because there are
> two equally valid schools of naming convention.
>
>  - When you have an array, each of whose 20 slots holds a single
>dosh, I would prefer to call the array dosh[20], not doshes[20],
>so that I can refer to the seventh dosh as "dosh[7]".
>
>  - If you more often refer to the array as a whole (than you refer
>to individual elements) and want to stress the fact that the
>array holds multiple elements in it, I can understand that you
>may be tempted to call the whole array "doshes[]".
>
> So please drop this and other "rename variables" patches from the
> series.
>
>

OK, I will revert that. I have done this because it's hard for me to
keep in mind that it's not just a simple pointer to a single value,
and I tried to make the code more intuitive.

Re: [PATCH RFC 01/24] ref-filter: get rid of goto

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

2018-01-30 23:49 GMT+03:00 Junio C Hamano <gits...@pobox.com>:
> Оля Тележная  <olyatelezhn...@gmail.com> writes:
>
>>> one place improves readability.  If better readability is the
>>> purpose, I would even say
>>>
>>>  for (i = 0; i < used_atom_cnt; i++) {
>>> if (...)
>>> -   goto need_obj;
>>> +   break;
>>> }
>>> -   return;
>>> +   if (used_atom_cnt <= i)
>>> return;
>>>
>>> -need_obj:
>>>
>>> would make the result easier to follow with a much less impact.
>>
>> It's hard for me to read the code with goto, and as I know, it's not
>> only my problem,...
>
> That sounds as if you are complaining "I wanted to get rid of goto
> and you tell me not to do so???", but read what I showed above again
> and notice that it is also getting rid of "goto".

No, I am not complaining. I tried to explain why I did everything that
way. Sorry if it was not clear enough.

>
> The main difference from your version is that the original function
> is still kept as a single unit of work, instead of two.

And I am not sure that it is good, the function is too big and it
actually does so many different separate pieces. If it is possible to
shorten long function by getting some separate logic (that's our case,
we do not request object until that final goto statement), I think
it's good idea and we need to do so and simplify future reading. But,
if you do not agree with this fact, please explain your position in
detail, and I will change that place as you want.

Thanks.

Re: [PATCH RFC 03/24] cat-file: split expand_atom into 2 functions

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

2018-01-27 0:46 GMT+03:00 Junio C Hamano :
> Olga Telezhnaya  writes:
>
>> Split expand_atom function into 2 different functions,
>> expand_atom_into_fields prepares variable for further filling,
>> (new) expand_atom creates resulting string.
>> Need that for further reusing of formatting logic from ref-filter.
>>
>> Signed-off-by: Olga Telezhnaia 
>> Mentored-by: Christian Couder 
>> Mentored by: Jeff King 
>> ---
>>  builtin/cat-file.c | 73 
>> +-
>>  1 file changed, 39 insertions(+), 34 deletions(-)
>
> As expand_atom() is file-scope static and its callers are well
> isolated, it is OK to change its meaning while restructuring the
> code like this patch does (as opposed to a public function to which
> new callers may be added on other topics in flight).
>
> The split itself looks sensible, but expand_atom_into_fields() is a
> questionable name.  expand_atom() does fill the data in sb, but
> calling expand_atom_into_fields() does not fill any data into
> separated fields---it merely prepares somebody else to do so.
>
> Helped by this comment:
>
> /*
>  * If mark_query is true, we do not expand anything, but rather
>  * just mark the object_info with items we wish to query.
>  */
> int mark_query;
>
> we can guess that a better name would mention or hint "object_info",
> "query" and probably "prepare" (because we would do so before
> actually querying).

OK, I will rename that function. Actually, not sure that we really
need to have ideal name here because both functions will be deleted by
the end of this patch.
"mark_atom_in_object_info"?

>
> I am not sure if separating the logic into these two functions is a
> good way to organize things.  When a new %(atom) is introduced, it
> is more likely that a programmer adds it to one but forgets to make
> a matching change to the other, no?  (here, "I am not sure" is just
> that.  It is very different from "I am sure this is wrong").

In the end of the patch we don't have both of those functions, so
there would not be such problem.
But, I need that split, it helps me to go further and apply new
changes step-by-step.

>
> Thanks.

Re: [PATCH RFC 01/24] ref-filter: get rid of goto

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

2018-01-26 23:19 GMT+03:00 Junio C Hamano :
> Olga Telezhnaya  writes:
>
>> Get rid of goto command in ref-filter for better readability.
>>
>> Signed-off-by: Olga Telezhnaia 
>> Mentored-by: Christian Couder 
>> Mentored by: Jeff King 
>> ---
>
> How was this patch "mentored by" these two folks?  Have they already
> reviewed and gave you OK, or are you asking them to also help reviewing
> with this message?  Mostly just being curious.

Christian and Jeff help me when I have different sort of difficulties.
Not sure that they were helping me with that commit separately.
Both of them reviewed my code and said that it's ready for a final
review (actually, Christian said, but it's usual situation when I ask
for help/review and one of them helps me. The other one could add
something, but, as I understand, if he totally agree, he will keep
silence, and I find that behavior logical).
Do I need to delete these lines from some of commits where I do not
remember help from them?

>
> It is not convincning that this splitting the last part of a single
> function into a separate helper function that is called from only
> one place improves readability.  If better readability is the
> purpose, I would even say
>
>  for (i = 0; i < used_atom_cnt; i++) {
> if (...)
> -   goto need_obj;
> +   break;
> }
> -   return;
> +   if (used_atom_cnt <= i)
> return;
>
> -need_obj:
>
> would make the result easier to follow with a much less impact.

It's hard for me to read the code with goto, and as I know, it's not
only my problem, it's usual situation to try to get rid of gotos. I
always need to re-check whether we use that piece of code somewhere
else or not, and how we do that. I also think that it's good that most
of variables in the beginning of the function populate_value go to new
function.

>
> If we later in the series will use this new helper function from
> other places, it certainly makes sense to create a new helper like
> this patch does, but then "get rid of goto for readability" is not
> the justification for such a change.

We don't use that new function anywhere else further. So, I can delete
this commit or I can change commit message (if so, please give me some
ideas what I need to mention there).

>
>>  ref-filter.c | 103 
>> ++-
>>  1 file changed, 53 insertions(+), 50 deletions(-)
>>
>> diff --git a/ref-filter.c b/ref-filter.c
>> index f9e25aea7a97e..37337b57aacf4 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -1354,16 +1354,60 @@ static const char *get_refname(struct used_atom 
>> *atom, struct ref_array_item *re
>>   return show_ref(>u.refname, ref->refname);
>>  }
>>
>> +static void need_object(struct ref_array_item *ref) {
>
> Style.  The opening brace at the beginning of the function sits on
> its own line alone.

Thanks, I will fix that when we decide how to finally improve that commit.

Olga

Re: [PATCH RFC 02/24] ref-filter: add return value to some functions

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

2018-01-27 0:05 GMT+03:00 Junio C Hamano :
> Olga Telezhnaya  writes:
>
>> Add return flag to format_ref_array_item, show_ref_array_item,
>> get_ref_array_info and populate_value for further using.
>> Need it to handle situations when item is broken but we can not invoke
>> die() because we are in batch mode and all items need to be processed.
>>
>> Signed-off-by: Olga Telezhnaia 
>> Mentored-by: Christian Couder 
>> Mentored by: Jeff King 
>> ---
>>  ref-filter.c | 21 +
>>  ref-filter.h |  4 ++--
>>  2 files changed, 15 insertions(+), 10 deletions(-)
>
> Makes sense as a preparatory step to pass the return status
> throughout the call chain.  The functions that actually will detect
> issues should probably gain
>
> /*
>  * a comment before the function
>  * that documents what it does and what values it returns
>  * to signal the failure.
>  */
>
> before them; those that only pass the errorcode through to the
> caller do not necessarily have to, though.

I will add comments, agree, thank you.

>
>> -void show_ref_array_item(struct ref_array_item *info,
>> +int show_ref_array_item(struct ref_array_item *info,
>>const struct ref_format *format)
>>  {
>>   struct strbuf final_buf = STRBUF_INIT;
>> + int retval = format_ref_array_item(info, format, _buf);
>>
>> - format_ref_array_item(info, format, _buf);
>>   fwrite(final_buf.buf, 1, final_buf.len, stdout);
>>   strbuf_release(_buf);
>> - putchar('\n');
>> + if (!retval)
>> + putchar('\n');
>> + return retval;
>
> This is questionable.  Why does it write final_buf out regardless of
> the return value, even though it refrains from writing terminating LF
> upon non-zero return from the same function that prepared final_buf?
>
> "Because final_buf is left unmodified when formatter returns an
> error, and calling fwrite on an empty buffer ends up being a no-op"
> is a wrong answer---it relies on having too intimate knowledge on
> how the callee happens to work currently.

I will change that piece of code by putting fwrite into if statement
also, thank you. We really do not put anything to buffer if retval is
not equal to zero (checked that).

Thank you for all your comments, waiting for further review.
Olga

[PATCH v3 0/23] cat-file: reuse formatting logic from ref-filter

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

The main idea of the patch is to get rid of using custom formatting in
cat-file and start using general one from ref-filter.
Additional bonus is that cat-file becomes to support many new
formatting commands like %(if), %(color), %(committername) etc.

Updates since last review:
In [PATCH v3 16/23] ref-filter: make cat_file_info independent
is_cat flag is hidden into global cat_file_info variable

Also make some minor refactoring.

Re: [PATCH v3 14/23] ref_filter: add is_atom_used function

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

2018-02-15 8:49 GMT+03:00 Jeff King :
> On Mon, Feb 12, 2018 at 08:08:54AM +, Olga Telezhnaya wrote:
>
>> Delete all items related to split_on_whitespace from ref-filter
>> and add new function for handling the logic.
>> Now cat-file could invoke that function to implementing its logic.
>
> OK, this is a good direction. I think in a more compact series we'd
> avoid moving the split-on-whitespace bits over to ref-filter in the
> first place, and have two commits:
>
>  - one early in the series adding is_atom_used()
>
>  - one late in the series switching cat-file over to is_atom_used() as
>part of the conversion to ref-filter
>
>> diff --git a/builtin/cat-file.c b/builtin/cat-file.c
>> index 6db57e3533806..3a49b55a1cc2e 100644
>> --- a/builtin/cat-file.c
>> +++ b/builtin/cat-file.c
>> @@ -382,8 +382,7 @@ static int batch_objects(struct batch_options *opt)
>>  {
>>   struct strbuf buf = STRBUF_INIT;
>>   struct expand_data data;
>> - int save_warning;
>> - int retval = 0;
>> + int save_warning, is_rest, retval = 0;
>
> Try to avoid reformatting existing code that you're not otherwise
> touching, as it makes the diff noisier. Just adding "int is_rest" would
> make this easier to review.
>
> I also think the variable name should probably still be
> "split_on_whitespace". It's set based on whether we saw a "%(rest)"
> atom, but ultimately we'll use it to decide whether to split.

OK, I will fix that.

>
> -Peff

Re: [PATCH v3 09/23] cat-file: start use ref_array_item struct

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

2018-02-15 8:40 GMT+03:00 Jeff King :
> On Mon, Feb 12, 2018 at 08:08:54AM +, Olga Telezhnaya wrote:
>
>> Moving from using expand_data to ref_array_item structure.
>> That helps us to reuse functions from ref-filter easier.
>
> This one feels weird. The point of a ref_array_item is for the caller to
> feed data into the ref-filter formatting code (usually that data comes
> from an earlier call to filter_refs(), but in the new cat-file we'd
> presumably feed single items).
>
> But here we're adding a bunch of fields for items that we'd expect the
> format code to compute itself.

It would be changed later, it's just the addition of new structure
that we have never used in cat-file before.

>
> -Peff

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 08/23] ref-filter: reuse parse_ref_filter_atom()

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

2018-02-15 8:37 GMT+03:00 Jeff King :
> On Mon, Feb 12, 2018 at 08:08:54AM +, Olga Telezhnaya wrote:
>
>> Continue migrating formatting logic from cat-file to ref-filter.
>> Reuse parse_ref_filter_atom() for unifying all processes in ref-filter
>> and further removing of mark_atom_in_object_info().
>
> OK, now it looks we're moving in a good direction.
>
> One thing that puzzles me:
>
>> @@ -401,20 +420,14 @@ static int is_atom(const char *atom, const char *s, 
>> int slen)
>>  static void mark_atom_in_object_info(const char *atom, int len,
>>   struct expand_data *data)
>>  {
>> - if (is_atom("objectname", atom, len))
>> - ; /* do nothing */
>> - else if (is_atom("objecttype", atom, len))
>> + if (is_atom("objecttype", atom, len))
>>   data->info.typep = >type;
>>   else if (is_atom("objectsize", atom, len))
>>   data->info.sizep = >size;
>> - else if (is_atom("objectsize:disk", atom, len))
>> - data->info.disk_sizep = >disk_size;
>>   else if (is_atom("rest", atom, len))
>>   data->split_on_whitespace = 1;
>>   else if (is_atom("deltabase", atom, len))
>>   data->info.delta_base_sha1 = data->delta_base_oid.hash;
>> - else
>> - die("unknown format element: %.*s", len, atom);
>>  }
>
> Why do some of these atoms go away and not others?

I deleted "objectname" because we were doing nothing there;
"objectsize:disk" because we have its own parser function;
"die" because ref-filter has its own checker whether the atom is valid or not.
I left all others because I haven't supported them at that point. This
whole function will be removed later.

> It seems like we're
> now relying on ref-filter to parse some of the common ones using its
> existing atom-parser. But wouldn't it have objecttype and objectsize
> already, then?

We haven't migrated enough to ref-filter at this point and we can't
reuse general ref-filter logic about filling the fields. So, we still
need to have our own function for doing that. Anyway, as I said
earlier, we will reach that status in the end of the patch: this
function would be deleted and we will use general ref-filter logic.

>
> -Peff

Re: [PATCH v3 15/23] cat-file: move skip_object_info into ref-filter

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

2018-02-15 8:51 GMT+03:00 Jeff King :
> On Mon, Feb 12, 2018 at 08:08:54AM +, Olga Telezhnaya wrote:
>
>> Move logic related to skip_object_info into ref-filter,
>> so that cat-file does not use that field at all.
>
> I think this is going the wrong way. ref-filter should always do as
> little work as possible to fulfill the request. So it should skip the
> object_info call whenever it can. And then any callers who want to make
> sure that the object exists can do so (as long as the formatting code
> tells them whether it looked up the object or not).
>
> And then ref-filter doesn't have to know about this skip_object_info
> flag at all.

Your message looks contradictory to me.
I agree that ref-filter should do as least as it's possible, and that
is the main reason why I put this code there. Moreover, I think that
it's a good idea to implement that variable not only for cat-file, but
for all ref-filter callers. And I think that it's a task of ref-filter
to check whether the object exists or not (or - whether the ref is
valid or not). But I am not sure that I need to solve that moment in
current patch. It sounds like another separate task.

>
> -Peff

Re: [PATCH v3 02/23] ref-filter: add return value to some functions

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

2018-02-15 8:16 GMT+03:00 Jeff King :
> On Mon, Feb 12, 2018 at 08:08:54AM +, Olga Telezhnaya wrote:
>
>> Add return flag to format_ref_array_item(), show_ref_array_item(),
>> get_ref_array_info() and populate_value() for further using.
>> Need it to handle situations when item is broken but we can not invoke
>> die() because we are in batch mode and all items need to be processed.
>
> OK. The source of these errors would eventually be calls in
> populate_value(), but we don't flag any errors there yet (well, we do,
> but they all end up in die() for now). So I'd expect to see later in the
> series those die() calls converted to errors (I haven't looked further
> yet; just making a note to myself).
>
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -1356,8 +1356,9 @@ static const char *get_refname(struct used_atom *atom, 
>> struct ref_array_item *re
>>
>>  /*
>>   * Parse the object referred by ref, and grab needed value.
>> + * Return 0 if everything was successful, -1 otherwise.
>>   */
>
> We discussed off-list the concept that the caller may want to know one
> of three outcomes:
>
>   - we completed the request, having accessed the object
>   - we completed the request, but it didn't require accessing any
> objects
>   - an error occurred accessing the object
>
> Since callers like "cat-file" would need to check has_sha1_file()
> manually in the second case. Should this return value actually be an
> enum, which would make it easier to convert later to a tri-state?

I decided not to implement this particular scenario because all other
callers are waiting that everything will be printed inside ref-filter.
We just add support for cat-file there. I don't think that I need to
re-think all printing process and move printing logic to all other
callers so that cat-file will behave fine. In my opinion, in the final
version cat-file must accept all ref-filter logic parts and adapt to
them.

>
>> -static void populate_value(struct ref_array_item *ref)
>> +static int populate_value(struct ref_array_item *ref)
>>  {
>>   void *buf;
>>   struct object *obj;
>> @@ -1482,7 +1483,7 @@ static void populate_value(struct ref_array_item *ref)
>>   }
>>   }
>>   if (used_atom_cnt <= i)
>> - return;
>> + return 0;
>
> Most of these conversions are obviously correct, because they just turn
> a void return into one with a value. But this one is trickier:
>
>> @@ -2138,9 +2144,10 @@ void format_ref_array_item(struct ref_array_item 
>> *info,
>>   ep = strchr(sp, ')');
>>   if (cp < sp)
>>   append_literal(cp, sp, );
>> - get_ref_atom_value(info,
>> -parse_ref_filter_atom(format, sp + 2, ep),
>> -);
>> + if (get_ref_atom_value(info,
>> +parse_ref_filter_atom(format, sp + 2, 
>> ep),
>> +))
>> + return -1;
>>   atomv->handler(atomv, );
>>   }
>
> since it affects the control flow. Might we be skipping any necessary
> cleanup in the function if we see an error?
>
> It looks like we may have called push_stack_element(), but we'd never
> get to the end of the function where we call pop_stack_element(),
> causing us to leak.

Agree,  I will fix this.

>
> -Peff

Re: [PATCH v3 13/23] ref-filter: get rid of mark_atom_in_object_info()

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

2018-02-15 8:45 GMT+03:00 Jeff King :
> On Mon, Feb 12, 2018 at 08:08:54AM +, Olga Telezhnaya wrote:
>
>> Remove mark_atom_in_object_info() and create same logic
>> in terms of ref-filter style.
>
> This one is definitely a step in the right direction. In fact, this is
> what I would have expected to see in the beginning. It seems like this
> first half of the series really would benefit from being squashed into a
> few commits. I.e., I'd have expected to see the whole series looking
> something like:
>
>   - preparatory cleanup of ref-filter
>
>   - teach ref-filter any new atoms (or atom options) that cat-file knows
> about but it doesn't
>
>   - convert cat-file to use ref-filter
>
> Most of what I've seen so far is basically that second step, but strung
> out along a bunch of commits. Can we compact it into a few commits that
> all make clear forward progress (by using "rebase -i" with "squash")?

I am afraid that I don't really see any 100% improvements by squashing
some of the commits. I tried to do that several times and I squashed
some of them, but now for me it looks like nothing else could be
squashed so that commits still look close to making atomic changes (I
am sure that it's super important).

>
> -Peff

Re: [PATCH v3 16/23] ref-filter: make cat_file_info independent

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

2018-02-15 8:53 GMT+03:00 Jeff King :
> On Mon, Feb 12, 2018 at 08:08:54AM +, Olga Telezhnaya wrote:
>
>> Remove connection between expand_data variable
>> in cat-file and in ref-filter.
>> It will help further to get rid of using expand_data in cat-file.
>
> I have to admit I'm confused at this point about what is_cat_file is
> for, or even why we need cat_file_data. Shouldn't these items be handled
> by their matching ref-filter atoms at this point?

We discussed that earlier outside of mailing list, and I even tried to
implement that idea and spent a couple of days to prove that it's not
possible.
The problem is that the list of atoms is made dynamically, and we
can't store pointers to any values in each atom. That's why we need
separate cat_file_info variable that is outside of main atom list.
We also need is_cat_file because we still have some part of logic that
is different for cat-file and for all other commands, and sometimes we
need to know that information.

>
> -Peff

Re: [PATCH v3 21/23] for-each-ref: tests for new atoms added

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

2018-02-15 8:57 GMT+03:00 Jeff King :
> On Mon, Feb 12, 2018 at 08:08:54AM +, Olga Telezhnaya wrote:
>
>> Add tests for new formatting atoms: rest, deltabase, objectsize:disk.
>> rest means nothing and we expand it into empty string.
>> We need this atom for cat-file command.
>> Have plans to support deltabase and objectsize:disk further
>> (as it is done in cat-file), now also expand it to empty string.
>
> I'm glad that you're adding tests, but I'm not sure it's a good idea to
> add tests checking for the thing we know to be wrong. If anything, you
> could be adding test_expect_failure looking for the _right_ thing, and
> accept that it does not yet work.

OK, I will try to fix that when other parts of the patch will look
good enough. I am not sure at this point that we will add my code to
the main version of the project, so these tests were written actually
for checking current functionality better before sending the code for
the review.

>
> -Peff

Re: [PATCH v3 04/23] ref-filter: make valid_atom as function parameter

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

2018-02-15 8:23 GMT+03:00 Jeff King :
> On Mon, Feb 12, 2018 at 08:08:54AM +, Olga Telezhnaya wrote:
>
>> Make valid_atom as a function parameter,
>> there could be another variable further.
>> Need that for further reusing of formatting logic in cat-file.c.
>>
>> We do not need to allow users to pass their own valid_atom variable in
>> global functions like verify_ref_format() because in the end we want to
>> have same set of valid atoms for all commands. But, as a first step
>> of migrating, I create further another version of valid_atom
>> for cat-file.
>
> Hmm. So I see where you're going here, but I think in the end we'd want
> to have a single valid_atom list again, and we wouldn't need this.
>
> And indeed, it looks like this goes away in patch 17. Can we
> reorganize/rebase the series so that we avoid dead-end directions like
> this?

I tried to do that, but in my opinion it's easier to understand
everything in current version. I need to squash too many items so that
commits do not look atomic, and it's really hard to understand what is
going on. Now we have that helper commit that will be cancelled later,
and other logic is more clear for readers.

>
> -Peff

Re: [PATCH 2/2] ref-filter: get rid of goto

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

2018-02-21 20:41 GMT+03:00 Junio C Hamano :
> Olga Telezhnaya  writes:
>
>> Get rid of goto command in ref-filter for better readability.
>>
>> Signed-off-by: Olga Telezhnaia 
>> Mentored-by: Christian Couder 
>> Mentored by: Jeff King 
>> ---
>>  ref-filter.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> It looks like this is the same change as the bottom-most change on
> your "cat-file --batch" series (and is obviously correct).
>
> I am puzzled by your intention---are you re-organizing and rebooting
> the series?  Either 'Yes' or 'No' is an acceptable answer, and so is
> anything else.  I just want to know what you want to happen to the
> merge conflicts if I queued this while still keeping your "cat-file
> --batch" thing I have on 'pu').

Thanks for the question, I needed to mention that.
We (with Peff) decided that it's better and easier to remake whole
previous patch. Before that, I want to make some refactorings in
ref-filter so after that migrating should go much easier. I want to do
that by small separate patches, so this is first in the series. I hope
it would be both easier to review for you and easier to fix for me.
So, if everything is fine, you could merge it to master. I will
rewrite most parts of previous patch anyway.

Thanks!

>
>>
>> diff --git a/ref-filter.c b/ref-filter.c
>> index 83ffd84affe52..28df6e21fb996 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -1494,11 +1494,11 @@ static void populate_value(struct ref_array_item 
>> *ref)
>>   for (i = 0; i < used_atom_cnt; i++) {
>>   struct atom_value *v = >value[i];
>>   if (v->s == NULL)
>> - goto need_obj;
>> + break;
>>   }
>> - return;
>> + if (used_atom_cnt <= i)
>> + return;
>>
>> - need_obj:
>>   get_object(ref, >objectname, 0, );
>>
>>   /*
>>
>> --
>> https://github.com/git/git/pull/460

ref-filter: how to improve the code

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

Hi everyone,
I am trying to remove cat-file formatting part and reuse same
functionality from ref-filter.
I have a problem that cat-file sometimes needs to continue running
even if the request is broken, while in ref-filter we invoke die() in
many places everywhere during formatting process. I write this email
because I want to discuss how to implement the solution better.

ref-filter has 2 functions which could be interesting for us:
format_ref_array_item() and show_ref_array_item(). I guess it's a good
idea to print everything in show_ref_array_item(), including all
errors. In that case all current users of ref-filter will invoke
show_ref_array_item() (as they did it before), and cat-file could use
format_ref_array_item() and work with the result in its own logic.

I tried to replace all die("...") with `return error("...")` and
finally exit(), but actual problem is that we print "error:..."
instead of "fatal:...", and it looks funny.
The draft of the code is here: https://github.com/telezhnaya/git/commits/p2

One of the easiest solutions is to add strbuf parameter for errors to
all functions that we use during formatting process, fill it in and
print in show_ref_array_item() if necessary. What do you think about
this idea?

Another way is to change the resulting error message, print current
message with "error" prefix and then print something like "fatal:
could not format the output". It is easier but I am not sure that it's
a good idea to change the interface.

If you have another ideas - please share them with me. It is really
important step to make formatting logic more general and easier to
reuse.

Thanks!
Olga

Rewrite cat-file.c : need help to find a bug

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

Hi everyone,
I am trying to reuse formatting logic from ref-filter in cat-file
command. Now cat-file uses its own formatting code.
I am trying to achieve that step-by-step, now I want to invoke
populate_value function, and I have a bug somewhere.
My code is here.
https://github.com/telezhnaya/git/commits/catfile
All commits that starts from "cat-file: " are successfully passing all tests.
So for now my last commit fails, and I am tired of searching for the
error. Actually, I just copied some values to another variable and
move some code to other place. All "intelligent" work will go further,
but now I need to fix current situation.

You could write here or leave comments in github if you have any ideas.

Thank you in advance for your help!
Olga

Re: What's cooking in git.git (Jul 2018, #03; Wed, 25)

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

2018-07-26 1:13 GMT+03:00 Junio C Hamano :
>
> * ot/ref-filter-object-info (2018-07-17) 5 commits
>  - ref-filter: use oid_object_info() to get object
>  - ref-filter: merge get_obj and get_object
>  - ref-filter: initialize eaten variable
>  - ref-filter: fill empty fields with empty values
>  - ref-filter: add info_source to valid_atom
>
>  A few atoms like %(objecttype) and %(objectsize) in the format
>  specifier of "for-each-ref --format=" can be filled without
>  getting the full contents of the object, but just with the object
>  header.  These cases have been optimzied by calling
>  oid_object_info() API.
>
>  What's the doneness of this one?
>

It is ready. Thanks.

Re: [PATCH v2 4/4] ref-filter: use oid_object_info() to get object

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

2018-07-16 23:53 GMT+03:00 Junio C Hamano :
> Olga Telezhnaya  writes:
>
>> -static int get_object(struct ref_array_item *ref, const struct object_id 
>> *oid,
>> -   int deref, struct object **obj, struct strbuf *err)
>> +static int get_object(struct ref_array_item *ref, int deref, struct object 
>> **obj,
>> +   struct expand_data *oi, struct strbuf *err)
>>  {
>> - int eaten;
>> - int ret = 0;
>> - unsigned long size;
>> - enum object_type type;
>> - void *buf = read_object_file(oid, , );
>> - if (!buf)
>> - ret = strbuf_addf_ret(err, -1, _("missing object %s for %s"),
>> -   oid_to_hex(oid), ref->refname);
>> - else {
>> - *obj = parse_object_buffer(oid, type, size, buf, );
>> - if (!*obj)
>> - ret = strbuf_addf_ret(err, -1, _("parse_object_buffer 
>> failed on %s for %s"),
>> -   oid_to_hex(oid), ref->refname);
>> - else
>> - grab_values(ref->value, deref, *obj, buf, size);
>> + /* parse_object_buffer() will set eaten to 0 if free() will be needed 
>> */
>> + int eaten = 1;
>
> Hmph, doesn't this belong to the previous step?  In other words,
> isn't the result of applying 1-3/4 has a bug that can leave eaten
> uninitialized (and base decision to free(buf) later on it), and
> isn't this change a fix for it?

Oh. I was thinking that it was new bug created by me. Now I see that
previously we had the same problem. I guess it's better to make
separate commit to fix it. Hope I can do it in this patch (I don't
want to create separate patch because of so minor update).
Thank you! I will fix it soon.

>
>> + if (oi->info.contentp) {
>> + /* We need to know that to use parse_object_buffer properly */
>> + oi->info.sizep = >size;
>> + oi->info.typep = >type;
>>   }
>> + if (oid_object_info_extended(the_repository, >oid, >info,
>> +  OBJECT_INFO_LOOKUP_REPLACE))
>> + return strbuf_addf_ret(err, -1, _("missing object %s for %s"),
>> +oid_to_hex(>oid), ref->refname);
>> +
>> + if (oi->info.contentp) {
>> + *obj = parse_object_buffer(>oid, oi->type, oi->size, 
>> oi->content, );
>> + if (!obj) {
>> + if (!eaten)
>> + free(oi->content);
>> + return strbuf_addf_ret(err, -1, _("parse_object_buffer 
>> failed on %s for %s"),
>> +oid_to_hex(>oid), 
>> ref->refname);
>> + }
>> + grab_values(ref->value, deref, *obj, oi->content, oi->size);
>> + }
>> +
>> + grab_common_values(ref->value, deref, oi);
>>   if (!eaten)
>> - free(buf);
>> - return ret;
>> + free(oi->content);
>> + return 0;
>>  }
>

Re: What's cooking in git.git (Jul 2018, #02; Wed, 18)

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

> * ot/ref-filter-object-info (2018-07-17) 5 commits
>  - ref-filter: use oid_object_info() to get object
>  - ref-filter: merge get_obj and get_object
>  - ref-filter: initialize eaten variable
>  - ref-filter: fill empty fields with empty values
>  - ref-filter: add info_source to valid_atom
>
>  A few atoms like %(objecttype) and %(objectsize) in the format
>  specifier of "for-each-ref --format=" can be filled without
>  getting the full contents of the object, but just with the object
>  header.  These cases have been optimzied by calling
>  oid_object_info() API.
>
>  What's the doneness of this one?
>

I fixed all known issues, I hope we can move forward with it.
Thank you!

Re: Git in Outreachy Dec-Mar?

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

Hi everyone,

I was Outreachy intern last winter. I guess I need to speak up: I will
be happy if my feedback helps you.
At first, I want to repeat all thanks to Outreachy organizers and Git
mentors. That was unique experience and I am so proud of being a part
of this project. But, I need to say that my internship wasn't ideal.
Mentors, please do not feel guilty: I just want to improve the quality
of future internships and give some advises.

I guess some of the problems aren't related to Git, and it's Outreachy
weak points. Please forward this email to Outreachy organizers if you
want.

1. The main problem of Outreachy internship is positioning. I mean, I
had strong confidence that it's an internship for newbies in
programming. All my friends had the same confidence, and that's the
reason why 2 my friends failed in the middle of the Outreachy
internship. Load was so big for them, noone explained this fact in the
beginning, noone helped with this situation during the internship. I
was thinking I could be overqualified and I took someone's place (I
had 2 other SWE internships before Outreachy). The truth is that my
skills were barely enough.

2. Please tell more about minimal requirements: write it down on a
landing page in the beginning and maybe repeat them in every task. I
guess it would be the same this year: good knowledge of C, gdb, Git
(as a user: intern needs to know how to work with forks, git remote,
git rebase -i, etc), Shell, base understanding of Linux terminal,
being ready to work remotely. It's good idea to mention that it's not
100% requirement, but anyway at least 60% from the list must be
familiar.

3. If you decide to be a mentor - at first, thanks a lot. Please be
ready to spend A LOT OF time on it. You need to explain not only the
task to your intern, but also how to split the task into subtasks, how
to look for solutions, how to work with the terminal, how to debug
better and many other questions. It's not only about solving
internship task. It's about learning something new. And I did not
mention code reviews: there would be many stupid errors and it's a
talent not to be angry about that.

4. I fully sure that you need to talk with your intern by the voice. I
mean regular calls, at least once a week. It's good idea to share the
desktop and show how you are working, what are you using, etc.
Ask your intern to share the desktop: you need to feel confident that
they understand how to work with the task. Help them with the
shortcuts.
Remote work is so hard at the beginning, I feel alone with all my
problems, feel ashamed to ask questions (because they are not "smart
enough"), sometimes I didn't know what to ask. I need to mention that
I had almost 1 year of remote work experience, and that helped me a
lot. But other interns do not have such experience.
Actually, I am sure that the only reason why I successfully finished
the internship is that my mentors believed in me and did not fire me
in the middle. I personally think that I failed first half of the
internship, and only in the end I had almost clear understanding
what's going on. (My friend was fired in the same situation.)

5. In the ideal world, I want to force all mentors to get special
courses (it will solve problems 2-3-4). Great developer is not equal
to great mentor. And, if you work with really newbie, it becomes so
necessary.

I hope that was useful.

In the end I want to say that there's no special requirements to
involve people from unrepresented groups. I see no racism or sexism in
mailing lists, my mentors were polite and friendly, I can't say
anything bad here. Please keep this safe environment and explain your
colleagues if you see something bad.
In my opinion, the problem is that Git is not friendly with newbies in
general. We do not have task tracker, regular mentors (without any
special programs: just some developers that are ready to help with
first patch). The code is not structured properly, this is additional
difficulty for newbie. This system with mailing lists and patches... I
understand that it's not possible to make all processes perfect in one
moment, but at least we need to help all newbies to solve all these
problems in the beginning.
I guess that there are only 2 scenarios how to become Git developer.
First one is internship. Second is to ask your colleague (who is Git
developer) to help you.
I don't want to speak on behalf of all women, but I guess many girls
feel not confident enough to ask for such help. For me the only
possibility to start was the internship.

Some personal info: I am in the process of changing jobs. I wish I
could help you with mentoring (not as a main mentor, maybe as a second
or third one - my experience as an intern could be useful, I could
help other interns to start), but I can't predict my load. If you are
interested in my help, please write me. And, by the way, please delete
my task from list of internship tasks, I will finish it by myself just
when I have some free time :)

Re: [PATCH 3/4] ref-filter: merge get_obj and get_object

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

Fully agree, thank you so much.
I have fixed it. Waiting for other issues that need to be fixed, then
I will re-send the patch.

Thank you!

Re: [PATCH 2/4] ref-filter: add empty values to technical fields

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

2018-07-10 1:39 GMT+03:00 Junio C Hamano :
> Olga Telezhnaya  writes:
>
>> Atoms like "align" or "end" do not have string representation.
>> Earlier we had to go and parse whole object with a hope that we
>> could fill their string representations. It's easier to fill them
>> with an empty string before we start to work with whole object.
>>
>> Signed-off-by: Olga Telezhnaia 
>> ---
>
> Just being curious, but is there any meaningful relationship between
> what was labelled as SOURCE_NONE in the previous step and what this
> step calls "technical fields"?  Things like "upstream" (which is not
> affected by the contents of the object, but is affected by the ref
> in question) and "if" (which merely exists to construct the language
> syntax) would fall into quite different category, so one might be
> subset/superset of the other, but I am wondering if we can take
> advantage of more table-driven approach taken by the previous step.

Sorry that it was not clear enough.
SOURCE_NONE fields are the fields that do not require object info.
By "technical fields" in this particular commit I mean fields that
will never be filled. So, it's a good idea to fill such fields with an
empty string and do not take them into account later (when we are
thinking whether we need to get and parse an object or not). I guess
we do not need to mark these "technical fields" in a special way: we
don't need this information. Moreover, some of the fields that I
filled with an empty string here, sometimes have non-empty
representation, and I changed nothing for such cases.

>
>
>>  ref-filter.c | 8 +++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/ref-filter.c b/ref-filter.c
>> index 8611c24fd57d1..27733ef013bed 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -1497,6 +1497,7 @@ static int populate_value(struct ref_array_item *ref, 
>> struct strbuf *err)
>>   refname = get_symref(atom, ref);
>>   else if (starts_with(name, "upstream")) {
>>   const char *branch_name;
>> + v->s = "";
>>   /* only local branches may have an upstream */
>>   if (!skip_prefix(ref->refname, "refs/heads/",
>>_name))
>> @@ -1509,6 +1510,7 @@ static int populate_value(struct ref_array_item *ref, 
>> struct strbuf *err)
>>   continue;
>>   } else if (atom->u.remote_ref.push) {
>>   const char *branch_name;
>> + v->s = "";
>>   if (!skip_prefix(ref->refname, "refs/heads/",
>>_name))
>>   continue;
>> @@ -1549,22 +1551,26 @@ static int populate_value(struct ref_array_item 
>> *ref, struct strbuf *err)
>>   continue;
>>   } else if (starts_with(name, "align")) {
>>   v->handler = align_atom_handler;
>> + v->s = "";
>>   continue;
>>   } else if (!strcmp(name, "end")) {
>>   v->handler = end_atom_handler;
>> + v->s = "";
>>   continue;
>>   } else if (starts_with(name, "if")) {
>>   const char *s;
>> -
>> + v->s = "";
>>   if (skip_prefix(name, "if:", ))
>>   v->s = xstrdup(s);
>>   v->handler = if_atom_handler;
>>   continue;
>>   } else if (!strcmp(name, "then")) {
>>   v->handler = then_atom_handler;
>> + v->s = "";
>>   continue;
>>   } else if (!strcmp(name, "else")) {
>>   v->handler = else_atom_handler;
>> + v->s = "";
>>   continue;
>>   } else
>>   continue;
>>
>> --
>> https://github.com/git/git/pull/520

Re: [PATCH 0/4] Use oid_object_info() instead of read_object_file()

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

2018-07-09 11:27 GMT+03:00 Оля Тележная :
> Hello everyone,
> This is my new attempt to start using oid_object_info_extended() in
> ref-filter. You could look at previous one [1] [2] but it is not
> necessary.
>
> The goal (still) is to improve performance by avoiding calling expensive
> functions when we don't need the information they provide
> or when we could get it by using a cheaper function.
>
> This patch is a middle step. In the end, I want to add new atoms
> ("objectsize:disk" and "deltabase") and reuse ref-filter logic in
> cat-file command.
>
> I also know about problems with memory leaks in ref-filter: that would
> be my next task that I will work on. Since I did not generate any new
> leaks in this patch (just use existing ones), I decided to put this
> part on a review and fix leaks as a separate task.

UPDATES since v1:
add init to eaten variable (thanks to Szeder Gabor, Johannes Schindelin)
improve second commit message (thanks to Junio C Hamano)
add static keyword (thanks to Ramsay Jones)

>
> Thank you!
>
> [1] https://github.com/git/git/pull/493
> [2] 
> https://public-inbox.org/git/010201637254c969-a346030e-0b75-41ad-8ef3-2ac7e04ba4fb-000...@eu-west-1.amazonses.com/

Re: [PATCH] ref-filter: mark some file-local symbols as static

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

2018-07-12 18:57 GMT+03:00 Ramsay Jones :
>
> Signed-off-by: Ramsay Jones 
> ---
>
> Hi Olga,
>
> If you need to re-roll your 'ot/ref-filter-object-info' branch,
> could you please squash this into the relevant patch (commit c5d9a471d6,
> "ref-filter: use oid_object_info() to get object", 2018-07-09).
>
> [Both sparse and my static-check.pl script are complaining about
> the 'oi' and 'oi_deref' symbols.]

You are right. Thanks a lot!

>
> Thanks!
>
> ATB,
> Ramsay Jones
>
>  ref-filter.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index 9aedf29e0..70fb15619 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -63,7 +63,7 @@ struct refname_atom {
> int lstrip, rstrip;
>  };
>
> -struct expand_data {
> +static struct expand_data {
> struct object_id oid;
> enum object_type type;
> unsigned long size;
> --
> 2.18.0

[PATCH 0/4] Use oid_object_info() instead of read_object_file()

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

Hello everyone,
This is my new attempt to start using oid_object_info_extended() in
ref-filter. You could look at previous one [1] [2] but it is not
necessary.

The goal (still) is to improve performance by avoiding calling expensive
functions when we don't need the information they provide
or when we could get it by using a cheaper function.

This patch is a middle step. In the end, I want to add new atoms
("objectsize:disk" and "deltabase") and reuse ref-filter logic in
cat-file command.

I also know about problems with memory leaks in ref-filter: that would
be my next task that I will work on. Since I did not generate any new
leaks in this patch (just use existing ones), I decided to put this
part on a review and fix leaks as a separate task.

Thank you!

[1] https://github.com/git/git/pull/493
[2] 
https://public-inbox.org/git/010201637254c969-a346030e-0b75-41ad-8ef3-2ac7e04ba4fb-000...@eu-west-1.amazonses.com/

Re: [PATCH v2 03/18] ref-filter: make valid_atom as function parameter

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

2018-01-18 9:20 GMT+03:00 Оля Тележная <olyatelezhn...@gmail.com>:
> 2018-01-18 1:39 GMT+03:00 Christian Couder <christian.cou...@gmail.com>:
>> On Wed, Jan 17, 2018 at 10:43 PM, Jeff King <p...@peff.net> wrote:
>>> On Tue, Jan 16, 2018 at 09:55:22AM +0300, Оля Тележная wrote:
>>>
>>>> > IOW, the progression I'd expect in a series like this is:
>>>> >
>>>> >   1. Teach ref-filter.c to support everything that cat-file can do.
>>>> >
>>>> >   2. Convert cat-file to use ref-filter.c.
>>>>
>>>> I agree, I even made this and it's working fine:
>>>> https://github.com/git/git/pull/450/commits/1b74f1047f07434dccb207534d1ad45a143e3f2b
>>
>> (Nit: it looks like the above link does not work any more, but it
>> seems that you are talking about the last patch on the catfile
>> branch.)
>>
>>>> But I decided not to add that to patch because I expand the
>>>> functionality of several commands (not only cat-file and
>>>> for-each-ref), and I need to support all new functionality in a proper
>>>> way, make these error messages, test everything and - the hardest one
>>>> - support many new commands for cat-file. As I understand, it is not
>>>> possible unless we finally move to ref-filter and print results also
>>>> there. Oh, and I also need to rewrite docs in that case. And I decided
>>>> to apply this in another patch. But, please, say your opinion, maybe
>>>> we could do that here in some way.
>>>
>>> Yeah, I agree that it will cause changes to other users of ref-filter,
>>> and you'd need to deal with documentation and tests there. But I think
>>> we're going to have to do those things eventually (since supporting
>>> those extra fields everywhere is part of the point of the project). And
>>> by doing them now, I think it can make the transition for cat-file a lot
>>> simpler, because we never have to puzzle over this intermediate state
>>> where only some of the atoms are valid for cat-file.
>>
>> I agree that you will have to deal with documentation and tests at one
>> point and that it could be a good idea to do that now.
>>
>> I wonder if it is possible to add atoms one by one into ref-filter and
>> to add tests and documentation at the same time, instead of merging
>> cat-file atoms with ref-filter atoms in one big step.
>>
>> When all the cat-file atoms have been added to ref-filter's
>> valid_atom, maybe you could add ref-filter's atoms to cat-file's
>> valid_cat_file_atom one by one and add tests and documentation at the
>> same time.
>>
>> And then when ref-filter's valid_atom and cat-file's
>> valid_cat_file_atom are identical you can remove cat-file's
>> valid_cat_file_atom and maybe after that rename "ref-filter" to
>> "format".
>
> I think it's important to finish migrating process at first. I mean,
> now we are preparing and collecting everything in ref-filter, but we
> make resulting string and print still in cat-file. And I am not sure,
> but maybe it will not be possible to start using new atoms in cat-file
> while some part of logic still differs.

I tried to make that part here:
https://github.com/telezhnaya/git/commit/19a148614f1d4db1f8e628eb4e6d7c819d2da875
I know that the code is disgusting and there is a memory leak :) I
just try to reuse ref-filter logic, I will cleanup everything later.
At first, I try to make it work.
The problem is that I have segfault, and if I use gdb, I get:

Program received signal SIGSEGV, Segmentation fault.
0x in ?? ()

I tried to google it, it's my first time when I get that strange
message, and unfortunately find nothing. So please explain me the
reason, why I can't find a place of segfault that way.
Thanks!

> And another thoughts here - we were thinking about creating format.h
> but decided not to move forward with it, and now we are suffering
> because of it. Can I create it right now or the history of commits
> would be too dirty because of it? Also, do you mean just renaming of
> ref-filter? I was thinking that I need to put formatting-related logic
> to another file and leave all other stuff in ref-filter.

By the way, I can create format.h in absolutely another branch, we
could merge it, and I will deal with all merge conflicts with my
current work. It sounds tediously, but actually it's not such a big
problem, I can do that.
But I still need to fully understand, what do you find more proper -
just rename ref-filter, or create new file and move formatting-related
logic.

>
> Anyway, your suggested steps looks good, and I a

Re: ref-filter: how to improve the code

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

2018-02-28 16:25 GMT+03:00 Jeff King <p...@peff.net>:
> On Sun, Feb 25, 2018 at 09:28:25PM +0300, Оля Тележная wrote:
>
>> I am trying to remove cat-file formatting part and reuse same
>> functionality from ref-filter.
>> I have a problem that cat-file sometimes needs to continue running
>> even if the request is broken, while in ref-filter we invoke die() in
>> many places everywhere during formatting process. I write this email
>> because I want to discuss how to implement the solution better.
>>
>> ref-filter has 2 functions which could be interesting for us:
>> format_ref_array_item() and show_ref_array_item(). I guess it's a good
>> idea to print everything in show_ref_array_item(), including all
>> errors. In that case all current users of ref-filter will invoke
>> show_ref_array_item() (as they did it before), and cat-file could use
>> format_ref_array_item() and work with the result in its own logic.
>
> Yes, that arrangement makes sense to me.
>
>> I tried to replace all die("...") with `return error("...")` and
>> finally exit(), but actual problem is that we print "error:..."
>> instead of "fatal:...", and it looks funny.
>
> If you do that, then format_ref_array_item() is still going to print
> things, even if it doesn't die(). But for "cat-file --batch", we usually
> do not print errors at all, but instead just say "... missing" (although
> it depends on the error; syntactic errors in the format string would
> still cause us to write to stderr).

Not sure if you catch my idea. format_ref_array_item() will not print
anything, it will just return an error code. And if there was an error
- we will print it in show_ref_array_item() (or go back to cat-file
and print what we want).

>
>> One of the easiest solutions is to add strbuf parameter for errors to
>> all functions that we use during formatting process, fill it in and
>> print in show_ref_array_item() if necessary. What do you think about
>> this idea?
>>
>> Another way is to change the resulting error message, print current
>> message with "error" prefix and then print something like "fatal:
>> could not format the output". It is easier but I am not sure that it's
>> a good idea to change the interface.
>
> For reference, the first one is what we've been switching to in the refs
> code. And I think it's been fairly successful there.
>
> -Peff

[RFC 0/4] ref-filter: remove printing from formatting logic

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

The main idea of the patch is, if you want to format the output by
ref-filter, you should have an ability to work with errors by yourself
if you want to.
So I decided not to touch signature of show_ref_array_item(), but to
move all printing (I mean errors) to it. So that we could invoke
format_ref_array_item() and be sure that we cold handle errors by
ourselves.

The patch is not finished, but I decided to show it to you. There are
still many places where we could die in the middle of formatting
process. But, if you like the general idea, I will finish and re-send
it.

Another question is about verify_ref_format(). Do we need to allow its
users also to manage errors by themselves? I left the old scenario,
printing everything in verify_ref_format() and die. If you have better
ideas, please share them.

Thanks a lot!
Olga

Re: [PATCH v2 5/5] ref-filter: get_ref_atom_value() error handling

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

2018-03-15 23:47 GMT+03:00 Martin Ågren :
> I skimmed the first four patches of this v2. It seems that patches 1 and
> 4 are identical to v2. Patches 2 and 3 have very straightforward changes
> based on my earlier comments. Let's see what this patch is about. :-)

Yes, you are right.

>
> On 14 March 2018 at 20:04, Olga Telezhnaya  wrote:
>> Finish removing any printing from ref-filter formatting logic,
>> so that it could be more general.
>>
>> Change the signature of get_ref_atom_value() and underlying functions
>> by adding return value and strbuf parameter for error message.
>>
>> It's important to mention that grab_objectname() returned 1 if
>> it gets objectname atom and 0 otherwise. Now this logic changed:
>> we return 0 if we have no error, -1 otherwise. If someone needs to
>> know whether it's objectname atom or not, he/she could use
>> starts_with() function. It duplicates this checking but it does not
>> sound like a really big overhead.
>>
>> Signed-off-by: Olga Telezhnaia 
>> ---
>>  ref-filter.c | 109 
>> +--
>>  1 file changed, 69 insertions(+), 40 deletions(-)
>>
>> diff --git a/ref-filter.c b/ref-filter.c
>> index 62ea4adcd0ff1..3f0c3924273d5 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -831,26 +831,27 @@ static void *get_obj(const struct object_id *oid, 
>> struct object **obj, unsigned
>>  }
>>
>>  static int grab_objectname(const char *name, const unsigned char *sha1,
>> -  struct atom_value *v, struct used_atom *atom)
>> +  struct atom_value *v, struct used_atom *atom,
>> +  struct strbuf *err)
>>  {
>> if (starts_with(name, "objectname")) {
>> if (atom->u.objectname.option == O_SHORT) {
>> v->s = xstrdup(find_unique_abbrev(sha1, 
>> DEFAULT_ABBREV));
>> -   return 1;
>> } else if (atom->u.objectname.option == O_FULL) {
>> v->s = xstrdup(sha1_to_hex(sha1));
>> -   return 1;
>> } else if (atom->u.objectname.option == O_LENGTH) {
>> v->s = xstrdup(find_unique_abbrev(sha1, 
>> atom->u.objectname.length));
>> -   return 1;
>> -   } else
>> -   die("BUG: unknown %%(objectname) option");
>> +   } else {
>> +   strbuf_addstr(err, "BUG: unknown %(objectname) 
>> option");
>> +   return -1;
>> +   }
>> }
>> return 0;
>>  }
>
> This is interesting. This die() is never ever supposed to actually
> trigger, except to allow a developer adding some new O_xxx-value to
> quickly notice that they have forgotten to add code here.

Oh, cool, so I can revert this change, OK.

>
>>  /* See grab_values */
>> -static void grab_common_values(struct atom_value *val, int deref, struct 
>> object *obj, void *buf, unsigned long sz)
>> +static int grab_common_values(struct atom_value *val, int deref, struct 
>> object *obj,
>> + void *buf, unsigned long sz, struct strbuf 
>> *err)
>>  {
>> int i;
>>
>> @@ -868,8 +869,10 @@ static void grab_common_values(struct atom_value *val, 
>> int deref, struct object
>> v->s = xstrfmt("%lu", sz);
>> }
>> else if (deref)
>> -   grab_objectname(name, obj->oid.hash, v, 
>> _atom[i]);
>> +   if (grab_objectname(name, obj->oid.hash, v, 
>> _atom[i], err))
>> +   return -1;
>> }
>> +   return 0;
>>  }
>
> So if that conversion I commented on above had not happened, this would
> not have been necessary. Let's read on...

Of course, I will check and also revert functions that were touched
only because of other functions.

>
>>  /* See grab_values */
>> @@ -1225,9 +1228,11 @@ static void fill_missing_values(struct atom_value 
>> *val)
>>   * pointed at by the ref itself; otherwise it is the object the
>>   * ref (which is a tag) refers to.
>>   */
>> -static void grab_values(struct atom_value *val, int deref, struct object 
>> *obj, void *buf, unsigned long sz)
>> +static int grab_values(struct atom_value *val, int deref, struct object 
>> *obj,
>> +  void *buf, unsigned long sz, struct strbuf *err)
>>  {
>> -   grab_common_values(val, deref, obj, buf, sz);
>> +   if (grab_common_values(val, deref, obj, buf, sz, err))
>> +   return -1;
>> switch (obj->type) {
>> case OBJ_TAG:
>> grab_tag_values(val, deref, obj, buf, sz);
>> @@ -1247,8 +1252,10 @@ static void grab_values(struct atom_value *val, int 
>> deref, struct object *obj, v
>> /* grab_blob_values(val, deref, obj, buf, sz); */
>> break;
>> 

Re: [PATCH v2 5/5] ref-filter: get_ref_atom_value() error handling

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

2018-03-16 0:01 GMT+03:00 Eric Sunshine :
> On Thu, Mar 15, 2018 at 4:47 PM, Martin Ågren  wrote:
>> These are "real" errors and yield several more changes in the remainder.
>> Ignoring those BUG-type messages at the beginning of this patch would
>> give a patch like the one below.
>>
>> +static int get_object(struct ref_array_item *ref, const struct object_id 
>> *oid,
>> +  int deref, struct object **obj, struct strbuf *err)
>>  {
>> void *buf = get_obj(oid, obj, , );
>> -   if (!buf)
>> -   die(_("missing object %s for %s"),
>> -   oid_to_hex(oid), ref->refname);
>> -   if (!*obj)
>> -   die(_("parse_object_buffer failed on %s for %s"),
>> -   oid_to_hex(oid), ref->refname);
>> -
>> +   if (!buf) {
>> +   strbuf_addf(err, _("missing object %s for %s"), 
>> oid_to_hex(oid),
>> +   ref->refname);
>> +   return -1;
>> +   }
>> +   if (!*obj) {
>> +   strbuf_addf(err, _("parse_object_buffer failed on %s for 
>> %s"),
>> +   oid_to_hex(oid), ref->refname);
>> +   return -1;
>
> Doesn't this leak 'buf'?

Yes. Thanks a lot.
>
>> +   }
>> grab_values(ref->value, deref, *obj, buf, size);
>> if (!eaten)
>> free(buf);
>> +   return 0;
>>  }

Re: [PATCH v2 3/5] ref-filter: change parsing function error handling

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

2018-03-16 1:48 GMT+03:00 Junio C Hamano :
> Olga Telezhnaya  writes:
>
>> Continue removing any printing from ref-filter formatting logic,
>> so that it could be more general.
>
> Hmm.
>
>> Change the signature of parse_ref_filter_atom() by changing return value,
>> adding previous return value to function parameter and also adding
>> strbuf parameter for error message.
>
> This says what the patch changes, but it does not explain why it is
> a good idea to return something with different meaning to the
> callers (which of course forces us to update all callers so that
> they pass  pointer and check the value returned in the
> variable), or more importantly what meaning the return value has and
> how the callers are expected to use it.  While at it, it probably is
> a good idea to explain what the original return value means.
>
> The return value from parse_ref_filter_atom() used to be the
> position the atom is found in the used_atom[] array; that
> information is now returned in an integer pointed at by the
> *res parameter.  The function now returns 0 for success and
> -1 for failure.
>
> or something like that.
>
> Having said that, I wonder if a calling convention that does not
> force callers to pass in a pointer-to-int may make more sense.
> Because the original return value is an index into an array, we know
> the normal return values are not negative.  An updated caller could
> become like this instead:
>
> pos = parse_ref_filter_atom(format, atom, ep, );
> if (pos < 0)
> die("%s", err.buf);
> ... original code that used 'pos' can stay as before ...
>

Martin also mentioned that, but I was not sure which solution is
better. Great, thanks, I will fix that.

Re: [RFC 0/4] ref-filter: remove printing from formatting logic

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

2018-03-13 22:26 GMT+03:00 Martin Ågren <martin.ag...@gmail.com>:
> Hi Olga
>
> On 13 March 2018 at 11:25, Оля Тележная <olyatelezhn...@gmail.com> wrote:
>> The main idea of the patch is, if you want to format the output by
>> ref-filter, you should have an ability to work with errors by yourself
>> if you want to.
>> So I decided not to touch signature of show_ref_array_item(), but to
>> move all printing (I mean errors) to it. So that we could invoke
>> format_ref_array_item() and be sure that we cold handle errors by
>> ourselves.
>>
>> The patch is not finished, but I decided to show it to you. There are
>> still many places where we could die in the middle of formatting
>> process. But, if you like the general idea, I will finish and re-send
>> it.
>>
>> Another question is about verify_ref_format(). Do we need to allow its
>> users also to manage errors by themselves? I left the old scenario,
>> printing everything in verify_ref_format() and die. If you have better
>> ideas, please share them.
>
> I think it is a good idea to stop die-ing in "libgit". This seems like a
> good way of achieving that, or isolating the issue. Do you have any
> particular use-case for this, i.e., are you setting up the stage for a
> patch "5" where you add a new user of one of these?

Yes, I want to reuse formatting part in cat-file command. In cat-file
sometimes we have an error but we want to continue our work and check
all other sha1s. But, anyway, I find these changes useful not only for
cat-file.

>
> I do wonder whether a helper function to call strbuf_addstr() and return
> -1 would be a good idea though. I mentioned it in patch 2, then with
> patches 3 and 4, it started to seem like a reasonably good idea. It
> would be a shame if this sort of "boilerplate" for handling errors could
> have an impact on code clarity / obviousness.

I am also not sure if the code will be intuitive enough.

>
> Another issue is whether passing NULL for an error-strbuf should be a
> way of saying "I don't care; die() so I do not have to". Well, right now
> I guess passing NULL would indeed terminate the program. ;-) Such a
> construct might be another reason for providing error_strbuf_addstr()...
> Of course, it also means we keep die-ing in libgit..
>
> I feel I'm just talking out loud. Maybe you find my input useful.

I do so! Thanks a lot.
I fixed all that you mentioned, you could find new code here if you want:
https://github.com/telezhnaya/git/commits/prepare
I will not re-send it to the mailing list now. I want to finish the
patch at first, there are still some die() calls.
If anyone has other thoughts or ideas - please share them.

>
> Martin

Re: [RFC 1/4] ref-filter: start adding strbufs with errors

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

2018-03-13 22:12 GMT+03:00 Martin Ågren :
> On 13 March 2018 at 11:16, Olga Telezhnaya  wrote:
>> This is a first step in removing any printing from
>> ref-filter formatting logic, so that it could be more general.
>> Everything would be the same for show_ref_array_item() users.
>> But, if you want to deal with errors by your own, you could invoke
>> format_ref_array_item(). It means that you need to print everything
>> (the result and errors) on your side.
>>
>> This commit changes signature of format_ref_array_item() by adding
>> return value and strbuf parameter for errors, and fixes
>> its callers.
>
> Minor nit: Maybe s/fixes its callers/adjusts its callers/. They are not
> broken or need to be fixed. They were simply playing the game according
> to the old rules, and now they need to learn the new ways. :-)

Agree.

>
>> Signed-off-by: Olga Telezhnaia 
>> ---
>>  builtin/branch.c |  7 +--
>>  ref-filter.c | 17 -
>>  ref-filter.h |  7 ---
>>  3 files changed, 21 insertions(+), 10 deletions(-)
>>
>> diff --git a/builtin/branch.c b/builtin/branch.c
>> index 8dcc2ed058be6..f86709ca42d5e 100644
>> --- a/builtin/branch.c
>> +++ b/builtin/branch.c
>> @@ -391,7 +391,6 @@ static void print_ref_list(struct ref_filter *filter, 
>> struct ref_sorting *sortin
>> struct ref_array array;
>> int maxwidth = 0;
>> const char *remote_prefix = "";
>> -   struct strbuf out = STRBUF_INIT;
>
> You move this variable into the loop to reduce its scope. At first I
> suspected that this might mean we now start allocating+releasing in each
> run of the loop, which might be a performance-regression. But it turns
> out, we already did that, so this tightening of the scope has no such
> downsides. :-) From the commit message, I wasn't expecting this move,
> though. Maybe "While at it, reduce the scope of the out-variable."

Added this to commit message. I just wanted to unify code style. I
added another strbuf and tried to reduce its scope (not sure that it
will cause a performance regression, I guess compiler is smart enough
- but, who knows). I saw another strbuf, and I just decided to put
them together. In my opinion, it's easier to read the code where
variables are created in the smallest possible scope.
But, anyway, you are right, I needed to mention that in the commit message.

>
>> char *to_free = NULL;
>>
>> /*
>> @@ -419,7 +418,10 @@ static void print_ref_list(struct ref_filter *filter, 
>> struct ref_sorting *sortin
>> ref_array_sort(sorting, );
>>
>> for (i = 0; i < array.nr; i++) {
>> -   format_ref_array_item(array.items[i], format, );
>> +   struct strbuf out = STRBUF_INIT;
>> +   struct strbuf err = STRBUF_INIT;
>> +   if (format_ref_array_item(array.items[i], format, , 
>> ))
>> +   die("%s", err.buf);
>
> Using "%s", good.
>
>> if (column_active(colopts)) {
>> assert(!filter->verbose && "--column and --verbose 
>> are incompatible");
>>  /* format to a string_list to let print_columns() 
>> do its job */
>> @@ -428,6 +430,7 @@ static void print_ref_list(struct ref_filter *filter, 
>> struct ref_sorting *sortin
>> fwrite(out.buf, 1, out.len, stdout);
>> putchar('\n');
>> }
>> +   strbuf_release();
>> strbuf_release();
>> }
>>
>> diff --git a/ref-filter.c b/ref-filter.c
>> index 45fc56216aaa8..54fae00bdd410 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -2118,9 +2118,10 @@ static void append_literal(const char *cp, const char 
>> *ep, struct ref_formatting
>> }
>>  }
>>
>> -void format_ref_array_item(struct ref_array_item *info,
>> +int format_ref_array_item(struct ref_array_item *info,
>>const struct ref_format *format,
>> -  struct strbuf *final_buf)
>> +  struct strbuf *final_buf,
>> +  struct strbuf *error_buf)
>>  {
>> const char *cp, *sp, *ep;
>> struct ref_formatting_state state = REF_FORMATTING_STATE_INIT;
>> @@ -2148,19 +2149,25 @@ void format_ref_array_item(struct ref_array_item 
>> *info,
>> resetv.s = GIT_COLOR_RESET;
>> append_atom(, );
>> }
>> -   if (state.stack->prev)
>> -   die(_("format: %%(end) atom missing"));
>> +   if (state.stack->prev) {
>> +   strbuf_addstr(error_buf, _("format: %(end) atom missing"));
>> +   return -1;
>> +   }
>> strbuf_addbuf(final_buf, >output);
>> pop_stack_element();
>> +   return 0;
>>  }
>>
>>  void show_ref_array_item(struct ref_array_item *info,
>>  const struct ref_format *format)
>>  {
>> struct 

Re: [RFC 3/4] ref-filter: change parsing function error handling

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

2018-03-13 22:18 GMT+03:00 Martin Ågren :
> On 13 March 2018 at 11:16, Olga Telezhnaya  wrote:
>> Continue removing any printing from ref-filter formatting logic,
>> so that it could be more general.
>>
>> Change the signature of parse_ref_filter_atom() by changing return value,
>> adding previous return value to function parameter and also adding
>> strbuf parameter for error message.
>
> I think the current return value is always non-negative. Maybe it would
> be easier to leave the return value as-is, except return negative on
> error? Unless I am missing something?

That's interesting. I like your idea, but let's see what other people think.
If others agree with us, I am ready to implement your solution.

>
>>
>> Signed-off-by: Olga Telezhnaia 
>> ---
>>  ref-filter.c | 45 -
>>  1 file changed, 32 insertions(+), 13 deletions(-)
>>
>> diff --git a/ref-filter.c b/ref-filter.c
>> index 07bedc636398c..e146215bf1e64 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -397,7 +397,8 @@ struct atom_value {
>>   * Used to parse format string and sort specifiers
>>   */
>>  static int parse_ref_filter_atom(const struct ref_format *format,
>> -const char *atom, const char *ep)
>> +const char *atom, const char *ep, int *res,
>> +struct strbuf *err)
>>  {
>> const char *sp;
>> const char *arg;
>> @@ -406,14 +407,18 @@ static int parse_ref_filter_atom(const struct 
>> ref_format *format,
>> sp = atom;
>> if (*sp == '*' && sp < ep)
>> sp++; /* deref */
>> -   if (ep <= sp)
>> -   die(_("malformed field name: %.*s"), (int)(ep-atom), atom);
>> +   if (ep <= sp) {
>> +   strbuf_addf(err, _("malformed field name: %.*s"), 
>> (int)(ep-atom), atom);
>> +   return -1;
>> +   }
>>
>> /* Do we have the atom already used elsewhere? */
>> for (i = 0; i < used_atom_cnt; i++) {
>> int len = strlen(used_atom[i].name);
>> -   if (len == ep - atom && !memcmp(used_atom[i].name, atom, 
>> len))
>> -   return i;
>> +   if (len == ep - atom && !memcmp(used_atom[i].name, atom, 
>> len)) {
>> +   *res = i;
>> +   return 0;
>> +   }
>> }
>
> If you did so, this hunk above would not need to be changed ...
>
>> @@ -458,7 +465,8 @@ static int parse_ref_filter_atom(const struct ref_format 
>> *format,
>> need_tagged = 1;
>> if (!strcmp(valid_atom[i].name, "symref"))
>> need_symref = 1;
>> -   return at;
>> +   *res = at;
>> +   return 0;
>>  }
>
> ... nor this one above ...
>
>> if (!ep)
>> return error(_("malformed format string %s"), sp);
>> /* sp points at "%(" and ep points at the closing ")" */
>> -   at = parse_ref_filter_atom(format, sp + 2, ep);
>> +   if (parse_ref_filter_atom(format, sp + 2, ep, , ))
>> +   die("%s", err.buf);
>
> And this would be more like "if (at < 0) die(...)".
>
>> for (cp = format->format; *cp && (sp = find_next(cp)); cp = ep + 1) {
>> struct atom_value *atomv;
>> +   struct strbuf err = STRBUF_INIT;
>> +   int pos;
>>
>> ep = strchr(sp, ')');
>> if (cp < sp)
>> append_literal(cp, sp, );
>> -   get_ref_atom_value(info,
>> -  parse_ref_filter_atom(format, sp + 2, ep),
>> -  );
>> +   if (parse_ref_filter_atom(format, sp + 2, ep, , ))
>> +   return -1;
>> +   get_ref_atom_value(info, pos, );
>> if (atomv->handler(atomv, , error_buf))
>> return -1;
>> +   strbuf_release();
>
> This looks leaky: if we get an error, we've got something in the buffer
> but we do not release it because we return early. Stepping back a bit, I
> wonder why we do not do anything at all with "err". Stepping back a bit
> more :-) I wonder if you could get rid of "err" and pass "error_buf" to
> parse_ref_filter_atom() instead. Our caller would like to have access to
> the error string?

Fully agree, I don't know why I decided to create one more buffer. Fixed.

>
> This ties back to my comment on the first patch -- "return negative if
> and only if you add some error string to the buffer" might be a useful
> rule?
>
> Martin

Re: [PATCH v6 0/6] ref-filter: remove die() calls from formatting logic

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

2018-03-29 17:41 GMT+03:00 Christian Couder <christian.cou...@gmail.com>:
> On Thu, Mar 29, 2018 at 2:52 PM, Оля Тележная <olyatelezhn...@gmail.com> 
> wrote:
>> Move helper function from strbuf to ref-filter.
>> Get rid of some memory leaks.
>
> The above seems to be the changes since v5. Usually in a cover letter
> (patch 0/X) there is both information about the goal of the patch
> series and the changes since last version.
>
> Repeating the goal in each version is useful for reviewers who might
> not have time to look at the patch series before, or who might have
> forgotten about it.

Thank you, I wasn't thinking about it that way. I agree, it's important.

Description:
The main idea of the patch is, if you want to format the output by
ref-filter, you should have an ability to work with errors and final
message by yourself if you want to.
So I decided not to touch signature of show_ref_array_item(), but to
move all die() invocations to it. So that we could invoke
format_ref_array_item() and be sure that we could handle errors by
ourselves, and we also get formatted message so we could continue
working with it if we want to.

Thank you,
Olga

[PATCH v6 0/6] ref-filter: remove die() calls from formatting logic

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

Move helper function from strbuf to ref-filter.
Get rid of some memory leaks.

Thanks to everyone!
Olga

[PATCH v4 0/5] ref-filter: remove die() calls from formatting logic

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

Only commit messages were updated since last review (there was no
comments about the code).

Thank you!
Olga

[PATCH v5 0/6] ref-filter: remove die() calls from formatting logic

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

Add strbuf_error() as a first commit and use it in all other commits.
Good line reduction, -67 lines compare to previous version.
Eric, thanks a lot, new code looks much better!

Thank all of you,
Olga

Re: [PATCH v5 1/6] strbuf: add shortcut to work with error messages

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

2018-03-21 23:20 GMT+03:00 Junio C Hamano :
> Olga Telezhnaya  writes:
>
>> Add function strbuf_error() that helps to save few lines of code.
>> Function expands fmt with placeholders, append resulting error message
>> to strbuf *err, and return error code ret.
>>
>> Signed-off-by: Olga Telezhnaia 
>> ---
>>  strbuf.h | 13 +
>>  1 file changed, 13 insertions(+)
>>
>> diff --git a/strbuf.h b/strbuf.h
>> index e6cae5f4398c8..fa66d4835f1a7 100644
>> --- a/strbuf.h
>> +++ b/strbuf.h
>> @@ -620,4 +620,17 @@ char *xstrvfmt(const char *fmt, va_list ap);
>>  __attribute__((format (printf, 1, 2)))
>>  char *xstrfmt(const char *fmt, ...);
>>
>> +/*
>> + * Expand error message, append it to strbuf *err, then return error code 
>> ret.
>> + * Allow to save few lines of code.
>> + */
>> +static inline int strbuf_error(struct strbuf *err, int ret, const char 
>> *fmt, ...)
>> +{
>
> With this function, err does not have to be an error message, and
> ret does not have to be negative.  Hence strbuf_error() is a wrong
> name for the wrapper.
>
> It somewhat is bothersome to see that this is inlined; if it is
> meant for error codepath, it probably shouldn't have to be.
>
>> + va_list ap;
>> + va_start(ap, fmt);
>> + strbuf_vaddf(err, fmt, ap);
>> + va_end(ap);
>> + return ret;
>> +}
>> +
>>  #endif /* STRBUF_H */
>
> Quite honestly, I am not sure if it is worth to be in strbuf.h; it
> feels a bit too specific to the immediate need for these five
> patches and nowhere else.  Are there many existing calls to
> strbuf_addf() immediately followed by an "return" of an integer,
> that can be simplified by using this helper?  I see quite a many
> instances of addf() soon followed by "return -1" in
> refs/files-backend.c, but they are not immediately adjacent to each
> other, and won't be helped.

Summarizing all that we discussed: I have 2 options how to continue
this patch. I can revert to v4, or I can replace new function in
strbuf.h with similar macro in ref-filter.c (I mean, there would be no
changes in strbuf.h and one new macro in ref-filter.c). What do you
like more?

Thanks!
Olga

Re: [PATCH v5 4/6] ref-filter: change parsing function error handling

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

2018-03-21 23:36 GMT+03:00 Junio C Hamano :
> Olga Telezhnaya  writes:
>
>> @@ -2144,13 +2151,15 @@ int format_ref_array_item(struct ref_array_item 
>> *info,
>>
>>   for (cp = format->format; *cp && (sp = find_next(cp)); cp = ep + 1) {
>>   struct atom_value *atomv;
>> + int pos;
>>
>>   ep = strchr(sp, ')');
>>   if (cp < sp)
>>   append_literal(cp, sp, );
>> - get_ref_atom_value(info,
>> -parse_ref_filter_atom(format, sp + 2, ep),
>> -);
>> + pos = parse_ref_filter_atom(format, sp + 2, ep, error_buf);
>> + if (pos < 0)
>> + return -1;
>> + get_ref_atom_value(info, pos, );
>>   if (atomv->handler(atomv, , error_buf))
>>   return -1;
>>   }
>
> These error returns leave the formatting state "state" on the stack
> holding onto its resources, no?

Yes, you are right, thanks a lot!

>
> The only thing the caller of format_ref_array_item() that notices an
> error return does is to die even after this series, so in that sense
> it does not matter (yet), but it still feels somewhat wrong.
>

I am sure I need to fix it anyway, thanks.

Re: [PATCH 1/3] ref-filter: free memory from used_atom

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

чт, 18 окт. 2018 г. в 9:51, Junio C Hamano :
>
> Jeff King  writes:
>
> > Presumably it came from the manual comment-style fixup.
>
> Wow, that was embarrassing.  Thanks for catching it.

Jeff, thanks a lot!
I just sent new version where I fixed all known issues including that comment.
>
> >
> > With that fix, the tests run fine for me under ASan/UBSan (with the
> > exception of t5310, but that's fixed already in a parallel topic).
> >
> > -Peff

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

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

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

[PATCH 0/3] ref-filter: reduce memory leaks

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

Reduce memory leaks in ref-filter.c.
We still have leaks, but at least not so much.

I use command valgrind --leak-check=full -v ./git for-each-ref to check results.

Before:
==24727== LEAK SUMMARY:
==24727==definitely lost: 69,424 bytes in 724 blocks
==24727==indirectly lost: 29,643 bytes in 723 blocks
==24727==  possibly lost: 120 bytes in 3 blocks
==24727==still reachable: 16,535 bytes in 185 blocks
==24727== suppressed: 0 bytes in 0 blocks
==24727== Reachable blocks (those to which a pointer was found) are not shown.
==24727== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==24727==
==24727== ERROR SUMMARY: 107 errors from 107 contexts (suppressed: 0 from 0)
==24727== ERROR SUMMARY: 107 errors from 107 contexts (suppressed: 0 from 0)

After:
==16419== LEAK SUMMARY:
==16419==definitely lost: 17,144 bytes in 1,447 blocks
==16419==indirectly lost: 0 bytes in 0 blocks
==16419==  possibly lost: 120 bytes in 3 blocks
==16419==still reachable: 16,217 bytes in 181 blocks
==16419== suppressed: 0 bytes in 0 blocks
==16419== Reachable blocks (those to which a pointer was found) are not shown.
==16419== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==16419==
==16419== ERROR SUMMARY: 82 errors from 82 contexts (suppressed: 0 from 0)
==16419== ERROR SUMMARY: 82 errors from 82 contexts (suppressed: 0 from 0)

Travis build failed, but it also failed in master, so I guess it's not my fault.
https://github.com/git/git/pull/538

Thank you!
Olga

Re: [RFC PATCH 1/5] ref-filter: add objectsize:disk option

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

вт, 13 нояб. 2018 г. в 04:52, Junio C Hamano :
>
> Jeff King  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...@mail.gmail.com/

>
>