Re: [PATCH] revisions --stdin: accept CRLF line terminators

Junio C Hamano <gitster@pobox.com> writes:

We have other places that take long list of things via --stdin option. It somehow feels incomplete to patch only rev-list and not others, doesn't it?

I looked at hits from 'grep -e --stdin Documentation/'. Here are the findings.

1. These use strbuf_getline() to get one line at a time into a
   strbuf and expects the line termination stripped off (i.e. these
   callers do not want to worry about having LF at the end):

        check-attr --stdin
        check-ingore --stdin
        check-mailmap --stdin
        checkout-index --stdin
        hash-object --stdin-paths
        http-fetch --stdin
        notes --stdin
        send-pack --stdin
        update-index --index-info

2. Any command in the "log" family uses strbuf_getwholeline(), so it
   needs to know about the LF at the end explicitly:

        rev-list --stdin
        show --stdin
        cherry-pick --stdin
        ...

3. This uses fgets() into a fixed buffer; it calls get_sha1_hex() on
   it, and the expected input is one 40-hex per line, so it does not
   matter if there is an extra CR at the end immediately before LF.

        diff-tree --stdin

4. This slurps everything in-core, instead of going line-by-line.

        update-ref --stdin

Now, I am wondering if it makes sense to do these two things:

 * Teach revision.c::read_revisions_from_stdin() to use
   strbuf_getline() instead of strbuf_getwholeline().

 * Teach strbuf_getline() to remove CR at the end when stripping the
   LF at the end, only if "term" parameter is set to LF.

Doing so would solve 1. and 2., but we obviously need to audit all the other uses of strbuf_getline() to see if they can benefit (or if some of them may be broken because they _always_ need LF terminated lines, i.e. CRLF terminated input is illegal to them).

As to 3., I think it is OK. The code structure of 4. is too ugly and needs to be revamped to go one line at a time first before even thinking about how to proceed, I would think.

Thoughts?