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

[Junio C Hamano <gitster@pobox.com>]

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

> Johannes Sixt <j6t@kdbg.org> writes:
>
>> On Windows, 'git rebase -i' with rebase.missingCommitsCheck set to
>> warn or error reports:
>>
>>    Dropped commits (newer to older):
>>    'atal: bad revision '410dee56...
>>
>> The error comes from the git rev-list --stdin invocation in
>> git-rebase--interactive.sh (function check_todo_list)....

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?

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "Git for Windows" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.