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

[Junio C Hamano <gitster@pobox.com>]

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

> I think it is needed later when "struct ref_sort" is moved into
> ref-filter.h, because then the used_atom[] array is not moved.

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.