Subject: Re: [PATCH v3 01/19] convert: make convert_attrs() and convert structs public

From: Junio C Hamano <[email-masked]>
Date: Fri, 30 Oct 2020 10:38:37 -0700

Matheus Tavares Bernardino <[email-masked]> writes:

>> > +enum crlf_action {
>> > +     CRLF_UNDEFINED,
>> > +     CRLF_BINARY,
>> > +     CRLF_TEXT,
>> > +     CRLF_TEXT_INPUT,
>> > +     CRLF_TEXT_CRLF,
>> > +     CRLF_AUTO,
>> > +     CRLF_AUTO_INPUT,
>> > +     CRLF_AUTO_CRLF
>> > +};
>> > +
>> > +struct convert_driver;
>> > +
>> > +struct conv_attrs {
>> > +     struct convert_driver *drv;
>> ...
>> > +void convert_attrs(const struct index_state *istate,
>> > +                struct conv_attrs *ca, const char *path);
>> > +
>> >  /*
>> >   * Initialize the checkout metadata with the given values.  Any argument may be
>> >   * NULL if it is not applicable.  The treeish should be a commit if that is
>>
>> The new global symbols are reasonable, I would think, with a
>> possible exception of "crlf_action", which may want to also have
>> "conv" or "convert" somewhere in its name.
>
> OK. Maybe `enum crlf_conv_action`? In this case, should I also change

Either that, or "conv_crlf_action" (or even use the fully spelled
"convert_" as the prefix common to the global symbols from the
subsystem).

> In this case, should I also change
> the prefix of the enum values? I'm not sure if it's worth it, though,
> since there are about 52 occurrences of them.

At the use sites, these constants will be passed to, or compared
with values returned by, the API functions whose names make it clear
that they are from the "convert_" family, so I think it is OK to
leave the values as-is, as long as there is no unrelated symbol
whose name starts with "CRLF_" (and "git grep '^CRLF_'" tells me
that there is not any).

Thanks.