On Wed, Jan 13, 2016 at 03:07:13PM -0800, Junio C Hamano wrote:
> > And then after a quiet period we can drop the "_crlf()" and have
> > strbuf_getline() back.
>
> Actually, I think a patch that
>
> - renames strbuf_getline() to strbuf_getdelim(); and
> - renames strbuf_getline_crlf() to strbuf_getline()
>
> on top of the series we already have is sufficient to bring the
> endgame state to us. The new strbuf_getline() has a different
> function signature from the traditional one, so any topic in flight
> that is unaware of this series can easily be caught, and we can do
> this without a quiet period.
Ah, right, I forgot about the changed function signature.
> A more interesting question is if strbuf_getdelim() should take an
> arbitrary byte as its third parameter. As I said elsewhere, the
> only reason why it is not a "do we use LF or do we use NUL?"
> boolean is because I wrote these codepaths anticipating that there
> might be a value other than NUL and LF that could be useful when I
> introduced line_termination long time ago, but no useful caller that
> uses other useful value has emerged, so I think the interface was
> too broad and too general for its own good.
>
> It becomes very tempting not to do strbuf_getdelim() at all, but
> instead rewrite the current calls to strbuf_getline() to call one of
> two functions, i.e. strbuf_getline_lf() and strbuf_getline_nul(),
> when we rename strbuf_getline_crlf() to strbuf_getline().
I think you'll end up with some of the callers being a bit uglier. I.e.,
where we say:
strbuf_getline(&buf, in, delim);
and "delim" is set elsewhere. These will become:
if (delim == '\n') /* or maybe even "if (nul_terminate)" */
strbuf_getline_lf(&buf, in);
else
strbuf_getline_nul(&buf, in);
which is a bit less nice. But I guess these cases already need to
become uglier if we want them to handle CRLF. Unless we want to wrap the
idiom as:
int strbuf_get_record(struct strbuf *buf,
FILE *in,
enum { STRBUF_RECORD_LINE,
STRBUF_RECORD_NUL
} delim);
and then "-z" option parsers use STRBUF_RECORD_NUL instead of setting a
char to '\0'.
> By going that route, those who want to help CRLF situation further
> can then concentrate on output from "git grep strbuf_getline_lf()",
> identify the ones that can be safely turned into strbuf_getline(),
> and do the conversion.
I'm not sure that is any easier than just grepping for strbuf_delim()
that takes '\n').
-Peff
--
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