Re: [PATCH v7 03/12] for-each-ref: change comment in ref_sort

Christian Couder <christian.couder@gmail.com> writes:

Now I am confused. used_atom[] is the mechanism we use to give a small integer to each atom used in the %(placeholder) string, so that we do not have to refer to them as "placeholder" string and we do not have to call !strcmp() to compare for equality. How can a field in ref_sort refer to it internally if used_atom[] is not visible?

Indeed, ref-filter.c after the series does have used_atom[] and get_ref_atom_value() does use atom to index into it. So these two lines do not make much sense to me. I am puzzled.

If by "moved" you are referring to the fact that the structure and its fields are exposed to the callers of the API while the implementation detail of the mechanism to give short integer to each used atom is not exposed to them, then I do agree that the comment to the structure field as the external API definition should not talk about used_atom[] array.

Perhaps in 03/12, you can stop talking about the implementation (i.e. the value is used to index into used_atom[] to get to the original string) and instead start talking about what the value means to the callers (that are still internal to for-each-ref implementation), to make things better.

Having said that, I am not sure if there is a sensible description for that field if you avoid exposing the implementation detail.

You would probably want to start by asking what that value means. For (evantual) external callers, the number is what they get from parse_ref_filter_atom(); calling that function is the only way they can get an appropriate value to stuff in the field, and parse_opt_ref_sorting() is the most likely function external callers use to make that happen.

"The number internally used to represent an attribute of a ref used when sorting the set of refs" would be one way to say what it is without exposing the implementation detail to the readers.

But does that help anybody? I doubt it. It is mouthful for external users, and it is not concrete enough for implementors.

So either

 - treat ref-filter.h as information for API users, and not talk
   about what it means at all, or

 - treat ref-filter.h as information for both API users and API
   implementors, and describe it as an index into used_atom[],
   i.e. do not change anything.

I'd say the latter is a more sensible way to go. I think it is also OK to change this comment to "index into used_atom array (internal)" when ref-filter.h is created as an external API definition.