Re: What's cooking in git.git (Jan 2016, #02; Mon, 11)

Jeff King [off-list ref] writes:

On Tue, Jan 12, 2016 at 10:47:25AM -0800, Junio C Hamano wrote:

quoted

I think strbuf_getline() that handles the payload as "text" without
having _crlf() suffix is an ideal endgame in the longer term, but I
do not think it is a good idea to do that as a flag-day change.  So
I am inclined not to change the function names around that feature
in this series.  Others can do the wholesale rename as a separate
follow-up topic when the tree is quiescent.

Yeah, I think we would want to catch topics in flight. Should the end of
this series then be to _remove_ strbuf_getline()? Callers should be
using strbuf_getline_crlf() if they want text lines, and
strbuf_getdelim() if they do not.

Topics in flight will need fixed up, but that's OK; the breakage (and
the fix) will be obvious.

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.

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().

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.

On Wed, Jan 13, 2016 at 03:07:13PM -0800, Junio C Hamano wrote:

quoted

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