On Sat, Jun 13, 2015 at 1:57 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote: > Christian Couder <christian.couder@xxxxxxxxx> 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. Like you said, the comment is still relevent to the code. So I guess of the two options suggested by you the option of keeping the comment and just adding "(internal)" while the code is moved to ref-filter.h seems to be the best solution. This would eliminate the need for PATCH 03. -- Regards, Karthik -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html