revisions --stdin: accept CRLF line terminators
PATCH 1/1 revisions --stdin: accept CRLF line terminators
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). It is caused by CRs that end up in the file "$todo".miss, because many tools of the MSYS toolset force LF to CRLF conversion when regular files are written via stdout.
To fix the error, permit CRLF line terminators when revisions and pathspec are read using the --stdin option.
---
This fixes a new failure in the test suite (t3404.8[67]) on Windows, but I got around to debug it only now.
revision.c | 4 ++++ t/t6017-rev-list-stdin.sh | 16 ++++++++++++++++ 2 files changed, 20 insertions(+)
revision.c +4 -0
@@ -1641,6 +1641,8 @@ static void read_pathspec_from_stdin(struct rev_info *revs, struct strbuf *sb,int len = sb->len;if (len && sb->buf[len -1 ] =='\n' ) sb->buf[--len] ='\0' ; +if (len && sb->buf[len -1 ] =='\r' ) + sb->buf[--len] ='\0' ;ALLOC_GROW (prune->path, prune->nr +1 , prune->alloc); prune->path[prune->nr++] =xstrdup (sb->buf); }
@@ -1661,6 +1663,8 @@ static void read_revisions_from_stdin(struct rev_info *revs,int len = sb.len;if (len && sb.buf[len -1 ] =='\n' ) sb.buf[--len] ='\0' ; +if (len && sb.buf[len -1 ] =='\r' ) + sb.buf[--len] ='\0' ;if (!len)break ;if (sb.buf[0 ] =='-' ) {
t/t6017-rev-list-stdin.sh +16 -0
@@ -75,4 +75,20 @@ test_expect_success 'not only --stdin' ' test_cmp expect actual ' +test_expect_success 'accept CRLF line terminators' ' + cat >expect <<-\EOF && + 7 + + file-2 + EOF + q_to_cr >input <<-\EOF && + masterQ + ^master^Q + --Q + file-2Q + EOF + git log --pretty=tformat:%s --name-only --stdin <input >actual && + test_cmp expect actual +' + test_done
Signature · 21 lines
2.3.2.245.gb5bf9d3 -- -- *** 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 [email protected] To unsubscribe from this group, send email to [email protected] 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 [email protected]. For more options, visit https://groups.google.com/d/optout.
Quoted 38 lines · Johannes Sixt
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(functioncheck_todo_list). It is caused by CRs that end up in the file "$todo".miss, because many tools of the MSYS toolset force LF to CRLF conversion when regular files are written via stdout.To fix the error, permit CRLF line terminators when revisions and pathspec are read using the --stdin option.
Signed-off-by Johannes Sixt <[email protected]>---
This fixes a new failure in the test suite (t3404.8[67]) on Windows, but I got around to debug it only now.revision.c | 4 ++++ t/t6017-rev-list-stdin.sh | 16 ++++++++++++++++ 2 files changed, 20 insertions(+)revision.c +2
-0@@ -1641,6 +1641,8 @@ static void read_pathspec_from_stdin(struct rev_info *revs, struct strbuf *sb,int len = sb->len;if (len && sb->buf[len -1 ] =='\n' ) sb->buf[--len] ='\0' ; +if (len && sb->buf[len -1 ] =='\r' ) + sb->buf[--len] ='\0' ;ALLOC_GROW (prune->path, prune->nr +1 , prune->alloc); prune->path[prune->nr++] =xstrdup (sb->buf); }
revision.c static void read_revisions_from_stdin(struct rev_info *revs, · line 1667
Quoted 5 lines
revision.c +2
-0@@ -1661,6 +1663,8 @@ static void read_revisions_from_stdin(struct rev_info *revs,int len = sb.len;if (len && sb.buf[len -1 ] =='\n' ) sb.buf[--len] ='\0' ; +if (len && sb.buf[len -1 ] =='\r' ) + sb.buf[--len] ='\0' ;
This will strip lone CR at the end of line, in addition to CRLF at the end of line (which you want to handle) and LF at the end of line (we always have removed), making it work even on ancient MacOS.
If we really cared, I guess we could write it like this:
if (len && sb.buf[len - 1] == '\n') {
len--;
if (len && sb.buf[len - 1] == '\r')
len--;
sb.buf[len] = '\0';
}but your version should be fine as-is.
Thanks.
Quoted 29 lines
t/t6017-rev-list-stdin.sh +16
-0@@ -75,4 +75,20 @@ test_expect_success 'not only --stdin' ' test_cmp expect actual ' +test_expect_success 'accept CRLF line terminators' ' + cat >expect <<-\EOF && + 7 + + file-2 + EOF + q_to_cr >input <<-\EOF && + masterQ + ^master^Q + --Q + file-2Q + EOF + git log --pretty=tformat:%s --name-only --stdin <input >actual && + test_cmp expect actual +' + test_doneSignature · 3 lines
2.3.2.245.gb5bf9d3 --
Signature · 18 lines
-- *** 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 [email protected] To unsubscribe from this group, send email to [email protected] 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 [email protected]. For more options, visit https://groups.google.com/d/optout.
Quoted 10 lines · Junio C Hamano
Quoted 8 lines · Johannes Sixt
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(functioncheck_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-info2. 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?
Signature · 18 lines
-- *** 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 [email protected] To unsubscribe from this group, send email to [email protected] 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 [email protected]. For more options, visit https://groups.google.com/d/optout.
Am 12.08.2015 um 00:14 schrieb Junio C Hamano:
Quoted 5 lines
Now, I am wondering if it makes sense to do these two things:
* Teach
revision.c::read_revisions_from_stdin()to usestrbuf_getline()instead ofstrbuf_getwholeline().
Quoted 7 lines
* 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).
I can see what I can do with these. Don't hold your breath, though.
Quoted 3 lines
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.
Regarding update-ref --stdin (your 4.), I notice that the input format is very strict, so the solution is to allow an optional CR before the LF. I alread have a patch, but it skips all trailing space, which is probably too lenient. (I only needed the patch once for a debug sesssion, but there is no obvious breakage without the patch.)
-- Hannes
cover Peace with CRLF
In reply to Junio C HamanoWe have too many topics titled "War on something"; let's try to make peace for a change.
This is a continuation to $gmane/275735, which is filed in the archive under another mailing list:
http://thread.gmane.org/gmane.comp.version-control.msysgit/21773
We read "text" files from filesystem (or from the standard input) in various places in the code. Some of them are written by us, but others are often prepared by an editor driven by human user. Even though we write (and expect) lines in these text files to be terminated with LF (not CRLF), the end-user's editor can be told to use CRLF termination, and on some platforms that may be the default.
Because many codepaths (e.g. commit log message) first pass the contents of such a file through stripspace(), and as a side effect of discarding the whitespace at the end of each line, lines terminated with CRLF are normalized to LF terminated lines (but we do not lose a lone CR in the middle of a non-blank string), this is not a problem in many codepaths. But not all of the codepaths are safe.
Typically, we use strbuf_getline() to read these "text" files, which reads a single line up to the first LF into a buffer, discard that LF at the end, and returns (an incomplete last line gets returned as-is). In theory, these places that expect to read "text", we should be able to update the lotic to read a line up to the first LF, discard that LF, together with a CR if one appears immediately before that LF, without breaking anything.
I inspected all the callsites of this function to see if it is safe to use such an updated logic at these callsites, and did not find anything problematic. I could update strbuf_getline() in place, but just to be extra careful, this series instead introduces another helper, strbuf_gets(), that is aware of this CRLF business, and convert the ones that are safe to update as we verify.
At the end of this message, you will find my notes while inspecting the current codebase as of 37023ba3 (Seventh batch for 2.7, 2015-10-26).
* This series converts only the callers of strbuf_getline() in the category [A], i.e. the current code would misbehave when fed a file with CRLF-terminated lines and use the data with an unwanted CR appended at the end, and with the update the code should work as intended with such a file, without breaking the expected behaviour when working on a file with LF-terminated lines.
* Callers that expect to read from our own output do not have to accomodate CRLF-terminated lines, but they can be updated to do so safely if we do not rely on the ability to express a payload that has CR at the end. For example, the insn sheet "rebase -i" uses typically end with commit log summary that we ourselves do not read or use, so even if the end-user on a platform with LF lines deliberately does insert \r at the end of the line and strbuf_gets() removed that \r from the payload, no unexpected behaviour should happen. They are categorized as [B] in the attached notes (and this series does not touch them).
* Some callers just strbuf_trim() or otherwise have logic that tolerates whitespaces at the end of the line. For them, it does not make any difference whether strbuf_getline() or strbuf_gets() is used to read the lines to be processed. They are caregorized as [C] in the attached notes (and this series does not touch them).
* I haven't found a caller that wants to only see LF-terminated lines (in other words, "ABC\r\n" must be treated as a line with 4 bytes payload on it, A B C and CR), but the survey reserves category name [X] for this empty set ;-).
Double-checking my classification for callers in [B] and [C] and making them all use strbuf_gets() would be a good microproject for GSoC in coming years ;-) If all callers with strbuf_getline() that uses '\n' as the terminator disappears at the end, that would be ideal.
Junio C Hamano (17):
strbuf: add strbuf_gets()
check-attr, check-ignore, checkout-index: read paths with strbuf_gets()
update-index: read --index-info with strbuf_gets()
update-index: read list of paths with strbuf_gets() under --stdin
mktree: read textual tree representation with strbuf_gets()
hash-object: read --stdin-paths with strbuf_gets()
revision: read --stdin with strbuf_gets()
rev-parse: read parseopt spec with strbuf_gets()
ident.c: read /etc/mailname with strbuf_gets()
remote.c: read $GIT_DIR/remotes/* with strbuf_gets() clone/sha1_file: read info/alternates with strbuf_gets()
transport-helper: read helper response with strbuf_gets()
cat-file: read batch stream with strbuf_gets()
column: read lines with strbuf_gets()
send-pack: read list of refs with strbuf_gets()
grep: read -f file with strbuf_gets()
test-sha1-array: read command stream with strbuf_gets()
builtin/am.c | 23 ++++------------------- builtin/cat-file.c | 2 +- builtin/check-attr.c | 4 +++- builtin/check-ignore.c | 5 ++++- builtin/checkout-index.c | 4 +++- builtin/clone.c | 2 +- builtin/column.c | 2 +- builtin/grep.c | 2 +- builtin/hash-object.c | 2 +- builtin/mktree.c | 4 +++- builtin/rev-parse.c | 4 ++-- builtin/send-pack.c | 2 +- builtin/update-index.c | 9 +++++++-- ident.c | 2 +- remote.c | 2 +- revision.c | 9 ++------- sha1_file.c | 2 +- strbuf.c | 16 ++++++++++++++-- strbuf.h | 7 +++++++ test-sha1-array.c | 2 +- transport-helper.c | 2 +- 21 files changed, 60 insertions(+), 47 deletions(-)
-- >8 -- survey of current strbuf_getline() callers -- >8 --
updating == "teaching strbuf_getline() to strip CR that comes
immediately before the LF"
[A] may be problematic on CRLF systems, "updating" will help fixing.
[B] CRLF line ending may or may not appear, but "updating" does not hurt.
[C] CRLF line ending may appear, but the code is already safe.
[X] The caller cares about distinction between CRLF and LF, and "updating" introduce breakage.
bisect.c uses it to read from BISECT_NAMES, which is a text file written by redirecting "rev-parse --sq-quote" into it. [B]
bisect.c uses it to read from BISECT_EXPECTED_REV, which is a "ref-like" file written by update_ref(). [B]
*** This should be cleaned up to use read_ref() or something.
ident.c reads from /etc/mailname, which is a text file. [A] *** mailnamebuf may want to be strbuf_strip()'ed, and doing so is another way to fix this.
credential-cache--daemon.c uses it to read requests, which is a text channel. [B]
credential-store.c uses it to read from the credential file, which is a text file. However, it is parsed by credential_from_url() that is not affected by an extra CR at the end. [C]
credential-store.c parrots the original in CRLF as CRLF with "store", while writing a new one with LF. This codepath will start normalizing the resulting file to LF, which may not be a bad thing. [C]
credential.c uses it to read from the helper. [B]
daemon.c uses it to read from a child process's output in order to save it to the log. The current code may have sent string with CR at the end to logerror(). [A]
fast-import.c uses it to read the command stream, which is text. [B]
remote-curl.c uses it as a transport helper to read commadn stream, which is text [B]
remote-testsvn.c uses it as a transport helper to read commadn stream, which is text [B]
remote.c uses it to read $GIT_DIR/remotes/* files, which is text. [A]
sequencer.c uses it to read $GIT_DIR/sequencer/head that it itself writes. [B]
builtin/clone.c and sha1_file.c uses it to read from an alternates file, which is text. [A]
shell.c does end-user interactions ;-) [B]
test-sha1-array.c reads command stream, which is text [A].
transport-helper.c reads helper output, which is text [A].
walker.c reads from the standard input a <target> HT <ref> per line, which is text [A].
wt-status.c reads from rebase-i insn, which is text, but it uses strbuf_trim() on it [C].
wt-status.c also reads from various "ref-like" things, all of which are internally generated by us [B].
builtin/am.c does a lot to process mail input. [A]
builtin/cat-file.c uses it to read --batch input, which is text [A].
builtin/checkout-index.c, builtin/check-attr.c and builtin/check-ignore.c use it to read --stdin, which is text [A].
builtin/check-mailmap.c uses it to read --stdin, but the tail end of the input line is not used [C].
builtin/clean.c does end-user interactions; it trims so it is safe [C].
builtin/column.c reads from standard input text. [A]
builtin/commit.c reads from MERGE_HEAD which is internally generated [B].
builtin/fetch-pack.c reads list of refs with --stdin, which is internally generated [B].
builtin/grep.c uses it to read patterns from a file with -f [A].
builtin/hash-object.c uses it with --stdin-paths [A].
mailinfo uses it to read from its input [B].
builtin/mktree.c reads etxt-formatted tree representation [A].
builtin/notes.c reads from the standard input upon "git notes copy"; the input is trimmed [C].
builtin/pull.c reads FETCH_HEAD which is internally generated [B].
builtin/repack.c reads the packname output by pack-objects which is internally generated [B].
builtin/rev-parse.c uses it for its parseopt mode [A].
builtin/send-pack.c uses it to read the refs from --stdin [A].
builtin/update-index.c reads "--index-info" [A].
builtin/update-index.c reads paths from "--stdin" [A].
PATCH 1/17 strbuf: add strbuf_gets()
Often we read "text" files that are supplied by the end user (e.g. commit log message that was edited with $GIT_EDITOR upon 'git commit -e'), and in some environments lines in a text file are terminated with CRLF. Existing strbuf_getline() knows to read a single line and then strip the terminating byte from the result, but it is handy to have a version that is more tailored for a "text" input that takes both '\n' and '\r\n' as line terminator (aka <newline> in POSIX lingo) and returns the body of the line after stripping <newline>.
Recently reimplemented "git am" already uses such a function implemented privately; move it to strbuf.[ch] and make it available for others.
--- builtin/am.c | 23 ++++------------------- strbuf.c | 16 ++++++++++++++-- strbuf.h | 7 +++++++ 3 files changed, 25 insertions(+), 21 deletions(-)
builtin/am.c +4 -19
@@ -45,21 +45,6 @@ static int is_empty_file(const char *filename) }/** -* Like strbuf_getline(), but treats both '\n' and "\r\n" as line terminators. -*/ -static int strbuf_getline_crlf (struct strbuf *sb,FILE *fp) -{ -if (strbuf_getwholeline (sb, fp,'\n' )) -return EOF ; -if (sb->buf[sb->len -1 ] =='\n' ) { -strbuf_setlen (sb, sb->len -1 ); -if (sb->len >0 && sb->buf[sb->len -1 ] =='\r' ) -strbuf_setlen (sb, sb->len -1 ); - } -return 0 ; -} - -/** * Returns the length of the first line of msg. */ static int linelen (const char *msg)
@@ -627,7 +612,7 @@ static int is_mail(FILE *fp)if (regcomp (®ex, header_regex,REG_NOSUB |REG_EXTENDED ))die ("invalid pattern: %s" , header_regex); -while (!strbuf_getline_crlf (&sb, fp)) { +while (!strbuf_gets (&sb, fp)) {if (!sb.len)break ;/* End of header */
@@ -674,7 +659,7 @@ static int detect_patch_format(const char **paths) fp =xfopen (*paths,"r" ); -while (!strbuf_getline_crlf (&l1, fp)) { +while (!strbuf_gets (&l1, fp)) {if (l1.len)break ; }
@@ -695,9 +680,9 @@ static int detect_patch_format(const char **paths) }strbuf_reset (&l2); -strbuf_getline_crlf (&l2, fp); +strbuf_gets (&l2, fp);strbuf_reset (&l3); -strbuf_getline_crlf (&l3, fp); +strbuf_gets (&l3, fp);/* * If the second line is empty and the third is a From, Author or Date
strbuf.c +14 -2
@@ -505,8 +505,20 @@ int strbuf_getline(struct strbuf *sb, FILE *fp, int term) {if (strbuf_getwholeline (sb, fp, term))return EOF ; -if (sb->buf[sb->len-1 ] == term) -strbuf_setlen (sb, sb->len-1 ); +if (sb->buf[sb->len -1 ] == term) +strbuf_setlen (sb, sb->len -1 ); +return 0 ; +} + +int strbuf_gets (struct strbuf *sb,FILE *fp) +{ +if (strbuf_getwholeline (sb, fp,'\n' )) +return EOF ; +if (sb->buf[sb->len -1 ] =='\n' ) { +strbuf_setlen (sb, sb->len -1 ); +if (sb->len && sb->buf[sb->len -1 ] =='\r' ) +strbuf_setlen (sb, sb->len -1 ); + }return 0 ; }
strbuf.h +7 -0
@@ -388,6 +388,13 @@ extern int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint);*/ extern int strbuf_getline (struct strbuf *,FILE *,int ); +/* +* Similar to strbuf_getline(), but uses '\n' as the terminator, +* and additionally treats a '\r' that comes immediately before '\n' +* as part of the terminator. +*/ +extern int strbuf_gets (struct strbuf *,FILE *); +/** * Like `strbuf_getline`, but keeps the trailing terminator (if * any) in the buffer.
Signature · 1 line
2.6.2-423-g5314b62
PATCH 2/17 check-attr, check-ignore, checkout-index: read paths with strbuf_gets()
In reply to Junio C HamanoThese commands read list of paths from their standard input under the --stdin option (in order to avoid busting limit on the length of the command line).
When they are using text input mode (i.e. line_termination is set to '\n'), we should try to be more friendly to our DOSsy friends and accept lines with CRLF endings.
It is tempting to lift this logic to strbuf_getline() and not introduce a separate strbuf_gets(), but that can lead to silent misconversion.
--- builtin/check-attr.c | 4 +++- builtin/check-ignore.c | 5 ++++- builtin/checkout-index.c | 4 +++- 3 files changed, 10 insertions(+), 3 deletions(-)
builtin/check-attr.c +3 -1
@@ -77,7 +77,9 @@ static void check_attr_stdin_paths(const char *prefix, int cnt,strbuf_init (&buf,0 );strbuf_init (&nbuf,0 ); -while (strbuf_getline (&buf, stdin, line_termination) !=EOF ) { +while ((line_termination + ?strbuf_gets (&buf, stdin) + :strbuf_getline (&buf, stdin,'\0' )) !=EOF ) {if (line_termination && buf.buf[0 ] =='"' ) {strbuf_reset (&nbuf);if (unquote_c_style (&nbuf, buf.buf,NULL ))
builtin/check-ignore.c +4 -1
@@ -122,7 +122,10 @@ static int check_ignore_stdin_paths(struct dir_struct *dir, const char *prefix)strbuf_init (&buf,0 );strbuf_init (&nbuf,0 ); -while (strbuf_getline (&buf, stdin, line_termination) !=EOF ) { + +while ((line_termination + ?strbuf_gets (&buf, stdin) + :strbuf_getline (&buf, stdin,'\0' )) !=EOF ) {if (line_termination && buf.buf[0 ] =='"' ) {strbuf_reset (&nbuf);if (unquote_c_style (&nbuf, buf.buf,NULL ))
builtin/checkout-index.c +3 -1
@@ -258,7 +258,9 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)if (all)die ("git checkout-index: don't mix '--all' and '--stdin'" ); -while (strbuf_getline (&buf, stdin, line_termination) !=EOF ) { +while ((line_termination + ?strbuf_gets (&buf, stdin) + :strbuf_getline (&buf, stdin,'\0' )) !=EOF ) {char *p;if (line_termination && buf.buf[0 ] =='"' ) {strbuf_reset (&nbuf);
Signature · 1 line
2.6.2-423-g5314b62
PATCH 3/17 update-index: read --index-info with strbuf_gets()
In reply to Junio C Hamano--- builtin/update-index.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
builtin/update-index.c +3 -1
@@ -473,7 +473,9 @@ static void read_index_info(int line_termination)struct strbuf buf =STRBUF_INIT ;struct strbuf uq =STRBUF_INIT ; -while (strbuf_getline (&buf, stdin, line_termination) !=EOF ) { +while ((line_termination + ?strbuf_gets (&buf, stdin) + :strbuf_getline (&buf, stdin,'\0' )) !=EOF ) {char *ptr, *tab;char *path_name;unsigned char sha1[20 ];
Signature · 1 line
2.6.2-423-g5314b62
PATCH 4/17 update-index: read list of paths with strbuf_gets() under --stdin
In reply to Junio C Hamano--- builtin/update-index.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
builtin/update-index.c +4 -1
@@ -1075,7 +1075,10 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)struct strbuf buf =STRBUF_INIT , nbuf =STRBUF_INIT ;setup_work_tree (); -while (strbuf_getline (&buf, stdin, line_termination) !=EOF ) { + +while ((line_termination + ?strbuf_gets (&buf, stdin) + :strbuf_getline (&buf, stdin,'\0' )) !=EOF ) {char *p;if (line_termination && buf.buf[0 ] =='"' ) {strbuf_reset (&nbuf);
Signature · 1 line
2.6.2-423-g5314b62
PATCH 5/17 mktree: read textual tree representation with strbuf_gets()
In reply to Junio C HamanoThe input can come from a DOS editor.
--- builtin/mktree.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
builtin/mktree.c +3 -1
@@ -157,7 +157,9 @@ int cmd_mktree(int ac, const char **av, const char *prefix)while (!got_eof) {while (1 ) { -if (strbuf_getline (&sb, stdin, line_termination) ==EOF ) { +if ((line_termination + ?strbuf_gets (&sb, stdin) + :strbuf_getline (&sb, stdin,'\0' )) ==EOF ) { got_eof =1 ;break ; }
Signature · 1 line
2.6.2-423-g5314b62
PATCH 6/17 hash-object: read --stdin-paths with strbuf_gets()
In reply to Junio C HamanoThe list of paths could have been written with a DOS editor.
--- builtin/hash-object.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
builtin/hash-object.c +1 -1
@@ -60,7 +60,7 @@ static void hash_stdin_paths(const char *type, int no_filters, unsigned flags, {struct strbuf buf =STRBUF_INIT , nbuf =STRBUF_INIT ; -while (strbuf_getline (&buf, stdin,'\n' ) !=EOF ) { +while (strbuf_gets (&buf, stdin) !=EOF ) {if (buf.buf[0 ] =='"' ) {strbuf_reset (&nbuf);if (unquote_c_style (&nbuf, buf.buf,NULL ))
Signature · 1 line
2.6.2-423-g5314b62
PATCH 7/17 revision: read --stdin with strbuf_gets()
In reply to Junio C HamanoReading with getwholeline() and manually stripping the terminating '\n' would leave CR at the end of the line if the input comes from a DOS editor.
Constrasting this with the previous few changes, one may realize that the way "log" family of commands read the paths with --stdin looks inconsistent and sloppy. It does not allow us to C-quote a textual input, and it does not accept NUL-terminated records. These are unfortunately way too late to fix X-<.
--- revision.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-)
revision.c +2 -7
@@ -1641,10 +1641,7 @@ static void append_prune_data(struct cmdline_pathspec *prune, const char **av)static void read_pathspec_from_stdin (struct rev_info *revs,struct strbuf *sb,struct cmdline_pathspec *prune) { -while (strbuf_getwholeline (sb, stdin,'\n' ) !=EOF ) { -int len = sb->len; -if (len && sb->buf[len -1 ] =='\n' ) - sb->buf[--len] ='\0' ; +while (strbuf_gets (sb, stdin) !=EOF ) {ALLOC_GROW (prune->path, prune->nr +1 , prune->alloc); prune->path[prune->nr++] =xstrdup (sb->buf); }
@@ -1661,10 +1658,8 @@ static void read_revisions_from_stdin(struct rev_info *revs, warn_on_object_refname_ambiguity =0 ;strbuf_init (&sb,1000 ); -while (strbuf_getwholeline (&sb, stdin,'\n' ) !=EOF ) { +while (strbuf_gets (&sb, stdin) !=EOF ) {int len = sb.len; -if (len && sb.buf[len -1 ] =='\n' ) - sb.buf[--len] ='\0' ;if (!len)break ;if (sb.buf[0 ] =='-' ) {
Signature · 1 line
2.6.2-423-g5314b62
PATCH 8/17 rev-parse: read parseopt spec with strbuf_gets()
In reply to Junio C Hamano"rev-parse --parseopt" specification is clearly text and we should anticipate that we may be fed CRLF lines.
--- builtin/rev-parse.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
builtin/rev-parse.c +2 -2
@@ -386,7 +386,7 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)/* get the usage up to the first line with a -- on it */ for (;;) { -if (strbuf_getline (&sb, stdin,'\n' ) ==EOF ) +if (strbuf_gets (&sb, stdin) ==EOF )die ("premature end of input" );ALLOC_GROW (usage, unb +1 , usz);if (!strcmp ("--" , sb.buf)) {
@@ -399,7 +399,7 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix) }/* parse: (<short>|<short>,<long>|<long>)[*=?!]*<arghint>? SP+ <help> */ -while (strbuf_getline (&sb, stdin,'\n' ) !=EOF ) { +while (strbuf_gets (&sb, stdin) !=EOF ) {const char *s;const char *help;struct option *o;
Signature · 1 line
2.6.2-423-g5314b62
PATCH 9/17 ident.c: read /etc/mailname with strbuf_gets()
In reply to Junio C HamanoJust in case /etc/mailname file was edited with a DOS editor, read it with strbuf_gets() so that a stray CR is not included as the last character of the mail hostname.
We _might_ want to more aggressively discard whitespace characters around the line with strbuf_trim(), but that is a bit outside the scope of this series.
--- ident.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
ident.c +1 -1
@@ -55,7 +55,7 @@ static int add_mailname_host(struct strbuf *buf) strerror(errno));return -1 ; } -if (strbuf_getline (&mailnamebuf, mailname,'\n' ) ==EOF ) { +if (strbuf_gets (&mailnamebuf, mailname) ==EOF ) {if (ferror (mailname))warning ("cannot read /etc/mailname: %s" ,strerror (errno));
Signature · 1 line
2.6.2-423-g5314b62
PATCH 10/17 remote.c: read $GIT_DIR/remotes/* with strbuf_gets()
In reply to Junio C HamanoThese files can be edited with a DOS editor, leaving CR at the end of the line if read with strbuf_getline().
--- remote.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
remote.c +1 -1
@@ -256,7 +256,7 @@ static void read_remotes_file(struct remote *remote)if (!f)return ; remote->origin =REMOTE_REMOTES ; -while (strbuf_getline (&buf, f,'\n' ) !=EOF ) { +while (strbuf_gets (&buf, f) !=EOF ) {const char *v;strbuf_rtrim (&buf);
Signature · 1 line
2.6.2-423-g5314b62
PATCH 11/17 clone/sha1_file: read info/alternates with strbuf_gets()
In reply to Junio C Hamano$GIT_OBJECT_DIRECTORY/info/alternates is a text file that can be edited with a DOS editor. We do not want to use the real path with CR appeneded at the end.
--- builtin/clone.c | 2 +- sha1_file.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
builtin/clone.c +1 -1
@@ -339,7 +339,7 @@ static void copy_alternates(struct strbuf *src, struct strbuf *dst,FILE *in =fopen (src->buf,"r" );struct strbuf line =STRBUF_INIT ; -while (strbuf_getline (&line, in,'\n' ) !=EOF ) { +while (strbuf_gets (&line, in) !=EOF ) {char *abs_path;if (!line.len || line.buf[0 ] =='#' )continue ;
sha1_file.c +1 -1
@@ -395,7 +395,7 @@ void add_to_alternates_file(const char *reference)struct strbuf line =STRBUF_INIT ;int found =0 ; -while (strbuf_getline (&line, in,'\n' ) !=EOF ) { +while (strbuf_gets (&line, in) !=EOF ) {if (!strcmp (reference, line.buf)) { found =1 ;break ;
Signature · 1 line
2.6.2-423-g5314b62
PATCH 12/17 transport-helper: read helper response with strbuf_gets()
In reply to Junio C HamanoOur implementation of helpers never use CRLF line endings, and they do not depend on the ability to place a CR as payload at the end of the line, so this is essentially a no-op for in-tree users. However, this allows third-party implementation of helpers to give us their line with CRLF line ending (they cannot expect us to feed CRLF to them, though).
--- transport-helper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
transport-helper.c +1 -1
@@ -54,7 +54,7 @@ static int recvline_fh(FILE *helper, struct strbuf *buffer, const char *name)strbuf_reset (buffer);if (debug)fprintf (stderr,"Debug: Remote helper: Waiting...\n" ); -if (strbuf_getline (buffer, helper,'\n' ) ==EOF ) { +if (strbuf_gets (buffer, helper) ==EOF ) {if (debug)fprintf (stderr,"Debug: Remote helper quit.\n" );return 1 ;
Signature · 1 line
2.6.2-423-g5314b62
PATCH 13/17 cat-file: read batch stream with strbuf_gets()
In reply to Junio C HamanoIt is possible to prepare a text file with a DOS editor and feed it as a batch command stream to the command.
--- builtin/cat-file.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
builtin/cat-file.c +1 -1
@@ -401,7 +401,7 @@ static int batch_objects(struct batch_options *opt) save_warning = warn_on_object_refname_ambiguity; warn_on_object_refname_ambiguity =0 ; -while (strbuf_getline (&buf, stdin,'\n' ) !=EOF ) { +while (strbuf_gets (&buf, stdin) !=EOF ) {if (data.split_on_whitespace) {/* * Split at first whitespace, tying off the beginning
Signature · 1 line
2.6.2-423-g5314b62
PATCH 14/17 column: read lines with strbuf_gets()
In reply to Junio C HamanoMultiple lines read here are concatenated on a single line to form a multi-column output line. We do not want to have a CR at the end, even if the input file consists of CRLF terminated lines.
--- builtin/column.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
builtin/column.c +1 -1
@@ -51,7 +51,7 @@ int cmd_column(int argc, const char **argv, const char *prefix)die (_ ("--command must be the first argument" )); }finalize_colopts (&colopts,-1 ); -while (!strbuf_getline (&sb, stdin,'\n' )) +while (!strbuf_gets (&sb, stdin))string_list_append (&list, sb.buf);print_columns (&list, colopts, &copts);
Signature · 1 line
2.6.2-423-g5314b62
PATCH 15/17 send-pack: read list of refs with strbuf_gets()
In reply to Junio C Hamano--- builtin/send-pack.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
builtin/send-pack.c +1 -1
@@ -212,7 +212,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)argv_array_push (&all_refspecs, buf); }else {struct strbuf line =STRBUF_INIT ; -while (strbuf_getline (&line, stdin,'\n' ) !=EOF ) +while (strbuf_gets (&line, stdin) !=EOF )argv_array_push (&all_refspecs, line.buf);strbuf_release (&line); }
Signature · 1 line
2.6.2-423-g5314b62
PATCH 16/17 grep: read -f file with strbuf_gets()
In reply to Junio C HamanoList of patterns file could come from a DOS editor.
This is iffy; you may actually be trying to find a line with ^M in it on a system whose line ending is LF. You can of course work it around by having a line that has "^M^M^J", let the strbuf_gets() eat the last "^M^J", leaving just the single "^M" as the pattern.
--- builtin/grep.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
builtin/grep.c +1 -1
@@ -562,7 +562,7 @@ static int file_callback(const struct option *opt, const char *arg, int unset) patterns = from_stdin ? stdin :fopen (arg,"r" );if (!patterns)die_errno (_ ("cannot open '%s'" ), arg); -while (strbuf_getline (&sb, patterns,'\n' ) ==0 ) { +while (strbuf_gets (&sb, patterns) ==0 ) {/* ignore empty line like grep does */ if (sb.len ==0 )continue ;
Signature · 1 line
2.6.2-423-g5314b62
PATCH 17/17 test-sha1-array: read command stream with strbuf_gets()
In reply to Junio C HamanoThe input to this command comes from a pipeline in t0064, whose upstream has bunch of "echo"s. It is not unreasonable to expect that it may be fed CRLF lines on DOSsy systems.
--- test-sha1-array.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
test-sha1-array.c +1 -1
@@ -11,7 +11,7 @@ int main(int argc, char **argv)struct sha1_array array =SHA1_ARRAY_INIT ;struct strbuf line =STRBUF_INIT ; -while (strbuf_getline (&line, stdin,'\n' ) !=EOF ) { +while (strbuf_gets (&line, stdin) !=EOF ) {const char *arg;unsigned char sha1[20 ];
Signature · 1 line
2.6.2-423-g5314b62
v2 cover Peace with CRLF
In reply to Junio C HamanoWe have too many topics titled "War on something"; let's try to make peace for a change.
This is a reroll with small changes of the previous one that appears here:
http://thread.gmane.org/gmane.comp.version-control.git/280401
which is a continuation to $gmane/275735, which is filed in the archive under another mailing list:
http://thread.gmane.org/gmane.comp.version-control.msysgit/21773
We read "text" files from filesystem (or from the standard input) in various places in the code. Some of them are written by us, but others are often prepared by an editor driven by human user. Even though we write (and expect) lines in these text files to be terminated with LF (not CRLF), the end-user's editor can be told to use CRLF termination, and on some platforms that may be the default.
Because many codepaths (e.g. commit log message) first pass the contents of such a file through stripspace(), and as a side effect of discarding the whitespace at the end of each line, lines terminated with CRLF are normalized to LF terminated lines (but we do not lose a lone CR in the middle of a non-blank string), this is not a problem in many codepaths. But not all of the codepaths are safe.
Typically, we use strbuf_getline() to read these "text" files, which reads a single line up to the first LF into a buffer, discard that LF at the end, and returns (an incomplete last line gets returned as-is). In theory, these places that expect to read "text", we should be able to update the logic to read a line up to the first LF, discard that LF, together with a CR if one appears immediately before that LF, without breaking anything.
I inspected all the callsites of this function to see if it is safe to use such an updated logic at these callsites, and did not find anything problematic. I could update strbuf_getline() in place, but just to be extra careful, this series instead introduces another helper, strbuf_getline_crlf(), that is aware of this CRLF business, and convert the ones that are safe to update as we verify.
* This series converts only the callers of strbuf_getline() that would misbehave when fed a file with CRLF-terminated lines and use the data with an unwanted CR appended at the end. With the update the code should work as intended with such a file, without breaking the expected behaviour when working on a file with LF-terminated lines.
* Callers of strbuf_getline() that expect to only read from our own output do not have to accommodate CRLF-terminated lines, but they can be updated to do so safely if we do not rely on the ability to express a payload that has CR at the end. For example, the insn sheet "rebase -i" uses typically ends each line with a commit log summary that we ourselves do not read or use, so even if the end-user on a platform with LF lines deliberately does insert \r at the end of the line and strbuf_gets() removed that \r from the payload, no unexpected behaviour should happen.
This series does not touch them, but it may be a good GSoC microproject to convert them to use strbuf_getline_crlf().
* Callers of strbuf_getline() that call strbuf_trim() immediately on the result before doing anything, or otherwise have logic that tolerates whitespaces at the end of the line, can continue using strbuf_getline() and will not misbehave on CRLF-terminated lines.
This series does not touch them; converting them to use strbuf_getline_crlf() would a good way to document them as dealing with "text". While doing so, they can also lose their custom logic that happens to make CRLF-terminated lines work.
* A caller of strbuf_getline() that wants to only see LF-terminated lines (in other words, "ABC\r\n" must be treated as a line with 4 bytes payload on it, A B C and CR), would be broken if we replace it with strbuf_getline_crlf(). This series does not touch them, and no follow-up series should, either.
An ideal endgame would be to retire the current strbuf_getline() and use strbuf_getline_crlf() everywhere. For that to happen, follow-up topics to cover the second and the third class of callers above need to show that there is no remaining callers to strbuf_getline() that falls into the fourth category.
Even better, once you say "line", as in strbuf_getline(), you ought to be talking about "text", so we could rename strbuf_getline_crlf() back to strbuf_getline() after all of the above happens.
Junio C Hamano (17):
strbuf: make strbuf_getline_crlf() global
check-attr, check-ignore, checkout-index: read paths with
strbuf_getline_crlf()
update-index: read --index-info with strbuf_getline_crlf()
update-index: read list of paths with strbuf_getline_crlf() under
--stdin
mktree: read textual tree representation with strbuf_getline_crlf()
hash-object: read --stdin-paths with strbuf_getline_crlf()
revision: read --stdin with strbuf_getline_crlf()
rev-parse: read parseopt spec with strbuf_getline_crlf() ident.c: read /etc/mailname with strbuf_getline_crlf() remote.c: read $GIT_DIR/remotes/* with strbuf_getline_crlf() clone/sha1_file: read info/alternates with strbuf_getline_crlf()
transport-helper: read helper response with strbuf_getline_crlf()
cat-file: read batch stream with strbuf_getline_crlf()
column: read lines with strbuf_getline_crlf()
send-pack: read list of refs with strbuf_getline_crlf()
grep: read -f file with strbuf_getline_crlf()
test-sha1-array: read command stream with strbuf_getline_crlf()
builtin/am.c | 15 --------------- builtin/cat-file.c | 2 +- builtin/check-attr.c | 4 +++- builtin/check-ignore.c | 5 ++++- builtin/checkout-index.c | 4 +++- builtin/clone.c | 2 +- builtin/column.c | 2 +- builtin/grep.c | 2 +- builtin/hash-object.c | 2 +- builtin/mktree.c | 4 +++- builtin/rev-parse.c | 4 ++-- builtin/send-pack.c | 2 +- builtin/update-index.c | 9 +++++++-- ident.c | 2 +- remote.c | 2 +- revision.c | 9 ++------- sha1_file.c | 2 +- strbuf.c | 16 ++++++++++++++-- strbuf.h | 7 +++++++ test-sha1-array.c | 2 +- transport-helper.c | 2 +- 21 files changed, 56 insertions(+), 43 deletions(-)
Signature · 4 lines
2.7.0-rc1-83-ga8b6b9e The interdiff between this round and v1 follows.
builtin/am.c +4 -4
@@ -612,7 +612,7 @@ static int is_mail(FILE *fp)if (regcomp (®ex, header_regex,REG_NOSUB |REG_EXTENDED ))die ("invalid pattern: %s" , header_regex); -while (!strbuf_gets (&sb, fp)) { +while (!strbuf_getline_crlf (&sb, fp)) {if (!sb.len)break ;/* End of header */
@@ -659,7 +659,7 @@ static int detect_patch_format(const char **paths) fp =xfopen (*paths,"r" ); -while (!strbuf_gets (&l1, fp)) { +while (!strbuf_getline_crlf (&l1, fp)) {if (l1.len)break ; }
@@ -680,9 +680,9 @@ static int detect_patch_format(const char **paths) }strbuf_reset (&l2); -strbuf_gets (&l2, fp); +strbuf_getline_crlf (&l2, fp);strbuf_reset (&l3); -strbuf_gets (&l3, fp); +strbuf_getline_crlf (&l3, fp);/* * If the second line is empty and the third is a From, Author or Date
builtin/cat-file.c +1 -1
@@ -401,7 +401,7 @@ static int batch_objects(struct batch_options *opt) save_warning = warn_on_object_refname_ambiguity; warn_on_object_refname_ambiguity =0 ; -while (strbuf_gets (&buf, stdin) !=EOF ) { +while (strbuf_getline_crlf (&buf, stdin) !=EOF ) {if (data.split_on_whitespace) {/* * Split at first whitespace, tying off the beginning
builtin/check-attr.c +1 -1
@@ -78,7 +78,7 @@ static void check_attr_stdin_paths(const char *prefix, int cnt,strbuf_init (&buf,0 );strbuf_init (&nbuf,0 );while ((line_termination - ?strbuf_gets (&buf, stdin) + ?strbuf_getline_crlf (&buf, stdin) :strbuf_getline (&buf, stdin,'\0' )) !=EOF ) {if (line_termination && buf.buf[0 ] =='"' ) {strbuf_reset (&nbuf);
builtin/check-ignore.c +1 -1
@@ -124,7 +124,7 @@ static int check_ignore_stdin_paths(struct dir_struct *dir, const char *prefix)strbuf_init (&nbuf,0 );while ((line_termination - ?strbuf_gets (&buf, stdin) + ?strbuf_getline_crlf (&buf, stdin) :strbuf_getline (&buf, stdin,'\0' )) !=EOF ) {if (line_termination && buf.buf[0 ] =='"' ) {strbuf_reset (&nbuf);
builtin/checkout-index.c +1 -1
@@ -259,7 +259,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)die ("git checkout-index: don't mix '--all' and '--stdin'" );while ((line_termination - ?strbuf_gets (&buf, stdin) + ?strbuf_getline_crlf (&buf, stdin) :strbuf_getline (&buf, stdin,'\0' )) !=EOF ) {char *p;if (line_termination && buf.buf[0 ] =='"' ) {
builtin/clone.c +1 -1
@@ -339,7 +339,7 @@ static void copy_alternates(struct strbuf *src, struct strbuf *dst,FILE *in =fopen (src->buf,"r" );struct strbuf line =STRBUF_INIT ; -while (strbuf_gets (&line, in) !=EOF ) { +while (strbuf_getline_crlf (&line, in) !=EOF ) {char *abs_path;if (!line.len || line.buf[0 ] =='#' )continue ;
builtin/column.c +1 -1
@@ -51,7 +51,7 @@ int cmd_column(int argc, const char **argv, const char *prefix)die (_ ("--command must be the first argument" )); }finalize_colopts (&colopts,-1 ); -while (!strbuf_gets (&sb, stdin)) +while (!strbuf_getline_crlf (&sb, stdin))string_list_append (&list, sb.buf);print_columns (&list, colopts, &copts);
builtin/grep.c +1 -1
@@ -562,7 +562,7 @@ static int file_callback(const struct option *opt, const char *arg, int unset) patterns = from_stdin ? stdin :fopen (arg,"r" );if (!patterns)die_errno (_ ("cannot open '%s'" ), arg); -while (strbuf_gets (&sb, patterns) ==0 ) { +while (strbuf_getline_crlf (&sb, patterns) ==0 ) {/* ignore empty line like grep does */ if (sb.len ==0 )continue ;
builtin/hash-object.c +1 -1
@@ -60,7 +60,7 @@ static void hash_stdin_paths(const char *type, int no_filters, unsigned flags, {struct strbuf buf =STRBUF_INIT , nbuf =STRBUF_INIT ; -while (strbuf_gets (&buf, stdin) !=EOF ) { +while (strbuf_getline_crlf (&buf, stdin) !=EOF ) {if (buf.buf[0 ] =='"' ) {strbuf_reset (&nbuf);if (unquote_c_style (&nbuf, buf.buf,NULL ))
builtin/mktree.c +1 -1
@@ -158,7 +158,7 @@ int cmd_mktree(int ac, const char **av, const char *prefix)while (!got_eof) {while (1 ) {if ((line_termination - ?strbuf_gets (&sb, stdin) + ?strbuf_getline_crlf (&sb, stdin) :strbuf_getline (&sb, stdin,'\0' )) ==EOF ) { got_eof =1 ;break ;
builtin/rev-parse.c +2 -2
@@ -386,7 +386,7 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)/* get the usage up to the first line with a -- on it */ for (;;) { -if (strbuf_gets (&sb, stdin) ==EOF ) +if (strbuf_getline_crlf (&sb, stdin) ==EOF )die ("premature end of input" );ALLOC_GROW (usage, unb +1 , usz);if (!strcmp ("--" , sb.buf)) {
@@ -399,7 +399,7 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix) }/* parse: (<short>|<short>,<long>|<long>)[*=?!]*<arghint>? SP+ <help> */ -while (strbuf_gets (&sb, stdin) !=EOF ) { +while (strbuf_getline_crlf (&sb, stdin) !=EOF ) {const char *s;const char *help;struct option *o;
builtin/send-pack.c +1 -1
@@ -212,7 +212,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)argv_array_push (&all_refspecs, buf); }else {struct strbuf line =STRBUF_INIT ; -while (strbuf_gets (&line, stdin) !=EOF ) +while (strbuf_getline_crlf (&line, stdin) !=EOF )argv_array_push (&all_refspecs, line.buf);strbuf_release (&line); }
builtin/update-index.c +2 -2
@@ -474,7 +474,7 @@ static void read_index_info(int line_termination)struct strbuf uq =STRBUF_INIT ;while ((line_termination - ?strbuf_gets (&buf, stdin) + ?strbuf_getline_crlf (&buf, stdin) :strbuf_getline (&buf, stdin,'\0' )) !=EOF ) {char *ptr, *tab;char *path_name;
@@ -1077,7 +1077,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)setup_work_tree ();while ((line_termination - ?strbuf_gets (&buf, stdin) + ?strbuf_getline_crlf (&buf, stdin) :strbuf_getline (&buf, stdin,'\0' )) !=EOF ) {char *p;if (line_termination && buf.buf[0 ] =='"' ) {
ident.c +1 -1
@@ -55,7 +55,7 @@ static int add_mailname_host(struct strbuf *buf) strerror(errno));return -1 ; } -if (strbuf_gets (&mailnamebuf, mailname) ==EOF ) { +if (strbuf_getline_crlf (&mailnamebuf, mailname) ==EOF ) {if (ferror (mailname))warning ("cannot read /etc/mailname: %s" ,strerror (errno));
remote.c +1 -1
@@ -256,7 +256,7 @@ static void read_remotes_file(struct remote *remote)if (!f)return ; remote->origin =REMOTE_REMOTES ; -while (strbuf_gets (&buf, f) !=EOF ) { +while (strbuf_getline_crlf (&buf, f) !=EOF ) {const char *v;strbuf_rtrim (&buf);
revision.c +2 -2
@@ -1641,7 +1641,7 @@ static void append_prune_data(struct cmdline_pathspec *prune, const char **av)static void read_pathspec_from_stdin (struct rev_info *revs,struct strbuf *sb,struct cmdline_pathspec *prune) { -while (strbuf_gets (sb, stdin) !=EOF ) { +while (strbuf_getline_crlf (sb, stdin) !=EOF ) {ALLOC_GROW (prune->path, prune->nr +1 , prune->alloc); prune->path[prune->nr++] =xstrdup (sb->buf); }
@@ -1658,7 +1658,7 @@ static void read_revisions_from_stdin(struct rev_info *revs, warn_on_object_refname_ambiguity =0 ;strbuf_init (&sb,1000 ); -while (strbuf_gets (&sb, stdin) !=EOF ) { +while (strbuf_getline_crlf (&sb, stdin) !=EOF ) {int len = sb.len;if (!len)break ;
sha1_file.c +1 -1
@@ -395,7 +395,7 @@ void add_to_alternates_file(const char *reference)struct strbuf line =STRBUF_INIT ;int found =0 ; -while (strbuf_gets (&line, in) !=EOF ) { +while (strbuf_getline_crlf (&line, in) !=EOF ) {if (!strcmp (reference, line.buf)) { found =1 ;break ;
strbuf.c +1 -1
@@ -510,7 +510,7 @@ int strbuf_getline(struct strbuf *sb, FILE *fp, int term)return 0 ; } -int strbuf_gets (struct strbuf *sb,FILE *fp) +int strbuf_getline_crlf (struct strbuf *sb,FILE *fp) {if (strbuf_getwholeline (sb, fp,'\n' ))return EOF ;
strbuf.h +1 -1
@@ -393,7 +393,7 @@ extern int strbuf_getline(struct strbuf *, FILE *, int);* and additionally treats a '\r' that comes immediately before '\n' * as part of the terminator. */ -extern int strbuf_gets (struct strbuf *,FILE *); +extern int strbuf_getline_crlf (struct strbuf *,FILE *);/** * Like `strbuf_getline`, but keeps the trailing terminator (if
test-sha1-array.c +1 -1
@@ -11,7 +11,7 @@ int main(int argc, char **argv)struct sha1_array array =SHA1_ARRAY_INIT ;struct strbuf line =STRBUF_INIT ; -while (strbuf_gets (&line, stdin) !=EOF ) { +while (strbuf_getline_crlf (&line, stdin) !=EOF ) {const char *arg;unsigned char sha1[20 ];
transport-helper.c +1 -1
@@ -54,7 +54,7 @@ static int recvline_fh(FILE *helper, struct strbuf *buffer, const char *name)strbuf_reset (buffer);if (debug)fprintf (stderr,"Debug: Remote helper: Waiting...\n" ); -if (strbuf_gets (buffer, helper) ==EOF ) { +if (strbuf_getline_crlf (buffer, helper) ==EOF ) {if (debug)fprintf (stderr,"Debug: Remote helper quit.\n" );return 1 ;
PATCH v2 1/17 strbuf: make strbuf_getline_crlf() global
Often we read "text" files that are supplied by the end user (e.g. commit log message that was edited with $GIT_EDITOR upon 'git commit -e'), and in some environments lines in a text file are terminated with CRLF. Existing strbuf_getline() knows to read a single line and then strip the terminating byte from the result, but it is handy to have a version that is more tailored for a "text" input that takes both '\n' and '\r\n' as line terminator (aka <newline> in POSIX lingo) and returns the body of the line after stripping <newline>.
Recently reimplemented "git am" uses such a function implemented privately; move it to strbuf.[ch] and make it available for others.
--- builtin/am.c | 15 --------------- strbuf.c | 16 ++++++++++++++-- strbuf.h | 7 +++++++ 3 files changed, 21 insertions(+), 17 deletions(-)
builtin/am.c +0 -15
@@ -45,21 +45,6 @@ static int is_empty_file(const char *filename) }/** -* Like strbuf_getline(), but treats both '\n' and "\r\n" as line terminators. -*/ -static int strbuf_getline_crlf (struct strbuf *sb,FILE *fp) -{ -if (strbuf_getwholeline (sb, fp,'\n' )) -return EOF ; -if (sb->buf[sb->len -1 ] =='\n' ) { -strbuf_setlen (sb, sb->len -1 ); -if (sb->len >0 && sb->buf[sb->len -1 ] =='\r' ) -strbuf_setlen (sb, sb->len -1 ); - } -return 0 ; -} - -/** * Returns the length of the first line of msg. */ static int linelen (const char *msg)
strbuf.c +14 -2
@@ -505,8 +505,20 @@ int strbuf_getline(struct strbuf *sb, FILE *fp, int term) {if (strbuf_getwholeline (sb, fp, term))return EOF ; -if (sb->buf[sb->len-1 ] == term) -strbuf_setlen (sb, sb->len-1 ); +if (sb->buf[sb->len -1 ] == term) +strbuf_setlen (sb, sb->len -1 ); +return 0 ; +} + +int strbuf_getline_crlf (struct strbuf *sb,FILE *fp) +{ +if (strbuf_getwholeline (sb, fp,'\n' )) +return EOF ; +if (sb->buf[sb->len -1 ] =='\n' ) { +strbuf_setlen (sb, sb->len -1 ); +if (sb->len && sb->buf[sb->len -1 ] =='\r' ) +strbuf_setlen (sb, sb->len -1 ); + }return 0 ; }
strbuf.h +7 -0
@@ -388,6 +388,13 @@ extern int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint);*/ extern int strbuf_getline (struct strbuf *,FILE *,int ); +/* +* Similar to strbuf_getline(), but uses '\n' as the terminator, +* and additionally treats a '\r' that comes immediately before '\n' +* as part of the terminator. +*/ +extern int strbuf_getline_crlf (struct strbuf *,FILE *); +/** * Like `strbuf_getline`, but keeps the trailing terminator (if * any) in the buffer.
Signature · 1 line
2.7.0-rc1-83-ga8b6b9e
Re: [PATCH v2 01/17] strbuf: make strbuf_getline_crlf() global
Hi Junio,
Quoted 10 lines · Junio C Hamano
Often we read "text" files that are supplied by the end user (e.g. commit log message that was edited with $GIT_EDITOR upon 'git commit -e'), and in some environments lines in a text file are terminated with CRLF. Existing
strbuf_getline()knows to read a single line and then strip the terminating byte from the result, but it is handy to have a version that is more tailored for a "text" input that takes both '\n' and '\r\n' as line terminator (aka <newline> in POSIX lingo) and returns the body of the line after stripping <newline>.
Quoted 2 lines
Recently reimplemented "git am" uses such a function implemented privately; move it to strbuf.[ch] and make it available for others.
... While at it, we fix the formatting of the strbuf_getline() function.
Ciao,
Dscho
Re: [PATCH v2 01/17] strbuf: make strbuf_getline_crlf() global
Quoted 14 lines · Johannes Schindelin
Hi Junio,
Quoted 10 lines · Junio C Hamano
Often we read "text" files that are supplied by the end user (e.g. commit log message that was edited with $GIT_EDITOR upon 'git commit -e'), and in some environments lines in a text file are terminated with CRLF. Existing
strbuf_getline()knows to read a single line and then strip the terminating byte from the result, but it is handy to have a version that is more tailored for a "text" input that takes both '\n' and '\r\n' as line terminator (aka <newline> in POSIX lingo) and returns the body of the line after stripping <newline>.
Quoted 4 lines
Quoted 2 lines
Recently reimplemented "git am" uses such a function implemented privately; move it to strbuf.[ch] and make it available for others.
... While at it, we fix the formatting of the
strbuf_getline()function.
Yeah right; I think it is obvious from the patch text, but while at it, I'll amend the log message ;-)
Thanks.
PATCH v2 2/17 check-attr, check-ignore, checkout-index: read paths with strbuf_getline_crlf()
In reply to Junio C HamanoThese commands read list of paths from their standard input under the --stdin option (in order to avoid busting limit on the length of the command line).
When they are using text input mode (i.e. line_termination is set to '\n'), we should try to be more friendly to our DOSsy friends and accept lines with CRLF endings.
It is tempting to lift this logic to strbuf_getline() and not introduce a separate strbuf_getline_crlf(), but that can lead to silent misconversion.
--- builtin/check-attr.c | 4 +++- builtin/check-ignore.c | 5 ++++- builtin/checkout-index.c | 4 +++- 3 files changed, 10 insertions(+), 3 deletions(-)
builtin/check-attr.c +3 -1
@@ -77,7 +77,9 @@ static void check_attr_stdin_paths(const char *prefix, int cnt,strbuf_init (&buf,0 );strbuf_init (&nbuf,0 ); -while (strbuf_getline (&buf, stdin, line_termination) !=EOF ) { +while ((line_termination + ?strbuf_getline_crlf (&buf, stdin) + :strbuf_getline (&buf, stdin,'\0' )) !=EOF ) {if (line_termination && buf.buf[0 ] =='"' ) {strbuf_reset (&nbuf);if (unquote_c_style (&nbuf, buf.buf,NULL ))
builtin/check-ignore.c +4 -1
@@ -122,7 +122,10 @@ static int check_ignore_stdin_paths(struct dir_struct *dir, const char *prefix)strbuf_init (&buf,0 );strbuf_init (&nbuf,0 ); -while (strbuf_getline (&buf, stdin, line_termination) !=EOF ) { + +while ((line_termination + ?strbuf_getline_crlf (&buf, stdin) + :strbuf_getline (&buf, stdin,'\0' )) !=EOF ) {if (line_termination && buf.buf[0 ] =='"' ) {strbuf_reset (&nbuf);if (unquote_c_style (&nbuf, buf.buf,NULL ))
builtin/checkout-index.c +3 -1
@@ -258,7 +258,9 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)if (all)die ("git checkout-index: don't mix '--all' and '--stdin'" ); -while (strbuf_getline (&buf, stdin, line_termination) !=EOF ) { +while ((line_termination + ?strbuf_getline_crlf (&buf, stdin) + :strbuf_getline (&buf, stdin,'\0' )) !=EOF ) {char *p;if (line_termination && buf.buf[0 ] =='"' ) {strbuf_reset (&nbuf);
Signature · 1 line
2.7.0-rc1-83-ga8b6b9e
Re: [PATCH v2 02/17] check-attr, check-ignore, checkout-index: read paths with strbuf_getline_crlf()
Hi Junio,
Quoted 11 lines · Junio C Hamano
These commands read list of paths from their standard input under the --stdin option (in order to avoid busting limit on the length of the command line).
When they are using text input mode (i.e.
line_terminationis set to '\n'), we should try to be more friendly to our DOSsy friends and accept lines with CRLF endings.It is tempting to lift this logic to
strbuf_getline()and not introduce a separatestrbuf_getline_crlf(), but that can lead to silent misconversion.
This paragraph would make more sense in 01/17.
Quoted 5 lines
builtin/check-attr.c +0
-0
builtin/check-attr.c static void check_attr_stdin_paths(const char *prefix, int cnt, · line 85
Quoted 10 lines
builtin/check-attr.c +3
-1@@ -77,7 +77,9 @@ static void check_attr_stdin_paths(const char *prefix, int cnt,strbuf_init (&buf,0 );strbuf_init (&nbuf,0 ); -while (strbuf_getline (&buf, stdin, line_termination) !=EOF ) { +while ((line_termination + ?strbuf_getline_crlf (&buf, stdin) + :strbuf_getline (&buf, stdin,'\0' )) !=EOF ) {if (line_termination && buf.buf[0 ] =='"' ) {strbuf_reset (&nbuf);if (unquote_c_style (&nbuf, buf.buf,NULL ))
Hrm. With this much context, it is unclear that line_termination can only ever be '\n' or '\0'. In fact, with the intention of this hunk, the line_termination variable makes no sense any longer because its value is not even used any longer. So something like this instead (and likewise in check-ignore.c)?
-- snipsnap --
builtin/check-attr.c +0 -0
@@ -73,12 +73,13 @@ static void check_attr_stdin_paths(const char *prefix,
int cnt,
struct git_attr_check *check)
{
struct strbuf buf, nbuf;
- int line_termination = nul_term_line ? 0 : '\n';
strbuf_init(&buf, 0);
strbuf_init(&nbuf, 0);
- while (strbuf_getline(&buf, stdin, line_termination) != EOF) {
- if (line_termination && buf.buf[0] == '"') {
+ while ((nul_term_line
+ ? strbuf_getline(&buf, stdin, '\0')
+ : strbuf_getline_crlf(&buf, stdin)) != EOF) {
+ if (!nul_term_line && buf.buf[0] == '"') {
strbuf_reset(&nbuf);
if (unquote_c_style(&nbuf, buf.buf, NULL))
die("line is badly quoted");
Re: [PATCH v2 02/17] check-attr, check-ignore, checkout-index: read paths with strbuf_getline_crlf()
Hi Junio,
Review
Quoted 10 lines · Johannes Schindelin
(quoted diff) +4
-2strbuf_init(&buf, 0); strbuf_init(&nbuf, 0); - while (strbuf_getline(&buf, stdin, line_termination) != EOF) { - if (line_termination && buf.buf[0] == '"') { + while ((nul_term_line + ? strbuf_getline(&buf, stdin, '\0') + : strbuf_getline_crlf(&buf, stdin)) != EOF) { + if (!nul_term_line && buf.buf[0] == '"') { strbuf_reset(&nbuf); if (unquote_c_style(&nbuf, buf.buf, NULL))
FWIW this is an example of that "abuse" I referred to earlier: the call to strbuf_getline(..., '\0') is actually *not* interested in a line at all. So I guess I would suggest to change the name "strbuf_getline" to "strbuf_getdelim" first, and then re-introduce a different "strbuf_getline" which is actually your "strbuf_getline_crlf". Because let's face it, if we are really reading a line of text, CR/LF should be handled automatically.
Ciao,
Dscho
Re: [PATCH v2 02/17] check-attr, check-ignore, checkout-index: read paths with strbuf_getline_crlf()
Quoted 3 lines · Johannes Schindelin
So I guess I would suggest to change the name "
strbuf_getline" to "strbuf_getdelim" first, and then re-introduce a different "strbuf_getline" which is actually your "strbuf_getline_crlf".
Makes sense. I really hated that name "getline_crlf".
PATCH v2 3/17 update-index: read --index-info with strbuf_getline_crlf()
In reply to Junio C Hamano--- builtin/update-index.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
builtin/update-index.c +3 -1
@@ -473,7 +473,9 @@ static void read_index_info(int line_termination)struct strbuf buf =STRBUF_INIT ;struct strbuf uq =STRBUF_INIT ; -while (strbuf_getline (&buf, stdin, line_termination) !=EOF ) { +while ((line_termination + ?strbuf_getline_crlf (&buf, stdin) + :strbuf_getline (&buf, stdin,'\0' )) !=EOF ) {char *ptr, *tab;char *path_name;unsigned char sha1[20 ];
Signature · 1 line
2.7.0-rc1-83-ga8b6b9e
Re: [PATCH v2 03/17] update-index: read --index-info with strbuf_getline_crlf()
Hi Junio,
builtin/update-index.c static void read_index_info(int line_termination) · line 479
Quoted 13 lines · Junio C Hamano
builtin/update-index.c +3
-1@@ -473,7 +473,9 @@ static void read_index_info(int line_termination)struct strbuf buf =STRBUF_INIT ;struct strbuf uq =STRBUF_INIT ; -while (strbuf_getline (&buf, stdin, line_termination) !=EOF ) { +while ((line_termination + ?strbuf_getline_crlf (&buf, stdin) + :strbuf_getline (&buf, stdin,'\0' )) !=EOF ) {char *ptr, *tab;
This is a problematic change because it does not safeguard for future introduction of a line_termination value other than LF or NUL. I believe the safest would be to change read_index_info() to take a nul_delimited parameter instead of the line_termination parameter first, and then introduce that change to use strbuf_getline_crlf if !nul_delimited.
Ciao,
Dscho
Re: [PATCH v2 03/17] update-index: read --index-info with strbuf_getline_crlf()
Quoted 8 lines · Johannes Schindelin
Quoted 8 lines
builtin/update-index.c +0
-0@@ -473,7 +473,9 @@ static void read_index_info(int line_termination)struct strbuf buf =STRBUF_INIT ;struct strbuf uq =STRBUF_INIT ;
Quoted 11 lines
builtin/update-index.c static void read_index_info(int line_termination) · line 479
Quoted 5 lines
builtin/update-index.c +3
-1-while (strbuf_getline (&buf, stdin, line_termination) !=EOF ) { +while ((line_termination + ?strbuf_getline_crlf (&buf, stdin) + :strbuf_getline (&buf, stdin,'\0' )) !=EOF ) {char *ptr, *tab;This is a problematic change because it does not safeguard for future introduction of a
line_terminationvalue other than LF or NUL. I believe the safest would be to changeread_index_info()to take anul_delimitedparameter instead of theline_terminationparameter first, and then introduce that change to usestrbuf_getline_crlfif !nul_delimited.
When I introduced line_termination long time ago, I wrote these codepaths anticipating that there might be a value other than NUL and LF that could be useful, but no useful caller that uses other useful value has emerged, so I agree that the interface was too broad and general for its own good.
I agree with your comments for all the changes in this series that switch between getline and getline_delim('\0') based on the value of line_termination; its generality is useless and we should declare "the line termination is either NUL or LF, no other choices"; using nul_delimited = 0/1 is a very good way to express it clearly.
Thanks.
PATCH v2 4/17 update-index: read list of paths with strbuf_getline_crlf() under --stdin
In reply to Junio C Hamano--- builtin/update-index.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
builtin/update-index.c +4 -1
@@ -1075,7 +1075,10 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)struct strbuf buf =STRBUF_INIT , nbuf =STRBUF_INIT ;setup_work_tree (); -while (strbuf_getline (&buf, stdin, line_termination) !=EOF ) { + +while ((line_termination + ?strbuf_getline_crlf (&buf, stdin) + :strbuf_getline (&buf, stdin,'\0' )) !=EOF ) {char *p;if (line_termination && buf.buf[0 ] =='"' ) {strbuf_reset (&nbuf);
Signature · 1 line
2.7.0-rc1-83-ga8b6b9e
Re: [PATCH v2 04/17] update-index: read list of paths with strbuf_getline_crlf() under --stdin
Hi Junio,
Quoted 5 lines · Junio C Hamano
builtin/update-index.c +0
-0
builtin/update-index.c int cmd_update_index(int argc, const char **argv, const char *prefix) · line 1083
Quoted 10 lines
builtin/update-index.c +4
-1@@ -1075,7 +1075,10 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)struct strbuf buf =STRBUF_INIT , nbuf =STRBUF_INIT ;setup_work_tree (); -while (strbuf_getline (&buf, stdin, line_termination) !=EOF ) { + +while ((line_termination + ?strbuf_getline_crlf (&buf, stdin) + :strbuf_getline (&buf, stdin,'\0' )) !=EOF ) {char *p;if (line_termination && buf.buf[0 ] =='"' ) {
Again, I think it would make things clearer and more future proof if we changed the logic to talk about nul_delimited instead of line_termination.
Ciao,
Dscho
PATCH v2 5/17 mktree: read textual tree representation with strbuf_getline_crlf()
In reply to Junio C HamanoThe input can come from a DOS editor.
--- builtin/mktree.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
builtin/mktree.c +3 -1
@@ -157,7 +157,9 @@ int cmd_mktree(int ac, const char **av, const char *prefix)while (!got_eof) {while (1 ) { -if (strbuf_getline (&sb, stdin, line_termination) ==EOF ) { +if ((line_termination + ?strbuf_getline_crlf (&sb, stdin) + :strbuf_getline (&sb, stdin,'\0' )) ==EOF ) { got_eof =1 ;break ; }
Signature · 1 line
2.7.0-rc1-83-ga8b6b9e
Re: [PATCH v2 05/17] mktree: read textual tree representation with strbuf_getline_crlf()
Hi Junio,
Quoted 5 lines · Junio C Hamano
builtin/mktree.c +0
-0
builtin/mktree.c int cmd_mktree(int ac, const char **av, const char *prefix) · line 165
Quoted 10 lines
builtin/mktree.c +3
-1@@ -157,7 +157,9 @@ int cmd_mktree(int ac, const char **av, const char *prefix)while (!got_eof) {while (1 ) { -if (strbuf_getline (&sb, stdin, line_termination) ==EOF ) { +if ((line_termination + ?strbuf_getline_crlf (&sb, stdin) + :strbuf_getline (&sb, stdin,'\0' )) ==EOF ) { got_eof =1 ;break ; }
Here also, I would recommend to change the logic to talk about nul_delimited instead of line_termination first, and only then introduce the change to call two different strbuf_* functions.
Ciao,
Dscho
PATCH v2 6/17 hash-object: read --stdin-paths with strbuf_getline_crlf()
In reply to Junio C HamanoThe list of paths could have been written with a DOS editor.
--- builtin/hash-object.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
builtin/hash-object.c +1 -1
@@ -60,7 +60,7 @@ static void hash_stdin_paths(const char *type, int no_filters, unsigned flags, {struct strbuf buf =STRBUF_INIT , nbuf =STRBUF_INIT ; -while (strbuf_getline (&buf, stdin,'\n' ) !=EOF ) { +while (strbuf_getline_crlf (&buf, stdin) !=EOF ) {if (buf.buf[0 ] =='"' ) {strbuf_reset (&nbuf);if (unquote_c_style (&nbuf, buf.buf,NULL ))
Signature · 1 line
2.7.0-rc1-83-ga8b6b9e
PATCH v2 7/17 revision: read --stdin with strbuf_getline_crlf()
In reply to Junio C HamanoReading with getwholeline() and manually stripping the terminating '\n' would leave CR at the end of the line if the input comes from a DOS editor.
Contrasting this with the previous few changes, one may realize that the way "log" family of commands read the paths with --stdin looks inconsistent and sloppy. It does not allow us to C-quote a textual input, and it does not accept NUL-terminated records. These are unfortunately way too late to fix X-<.
--- revision.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-)
revision.c +2 -7
@@ -1641,10 +1641,7 @@ static void append_prune_data(struct cmdline_pathspec *prune, const char **av)static void read_pathspec_from_stdin (struct rev_info *revs,struct strbuf *sb,struct cmdline_pathspec *prune) { -while (strbuf_getwholeline (sb, stdin,'\n' ) !=EOF ) { -int len = sb->len; -if (len && sb->buf[len -1 ] =='\n' ) - sb->buf[--len] ='\0' ; +while (strbuf_getline_crlf (sb, stdin) !=EOF ) {ALLOC_GROW (prune->path, prune->nr +1 , prune->alloc); prune->path[prune->nr++] =xstrdup (sb->buf); }
@@ -1661,10 +1658,8 @@ static void read_revisions_from_stdin(struct rev_info *revs, warn_on_object_refname_ambiguity =0 ;strbuf_init (&sb,1000 ); -while (strbuf_getwholeline (&sb, stdin,'\n' ) !=EOF ) { +while (strbuf_getline_crlf (&sb, stdin) !=EOF ) {int len = sb.len; -if (len && sb.buf[len -1 ] =='\n' ) - sb.buf[--len] ='\0' ;if (!len)break ;if (sb.buf[0 ] =='-' ) {
Signature · 1 line
2.7.0-rc1-83-ga8b6b9e
PATCH v2 8/17 rev-parse: read parseopt spec with strbuf_getline_crlf()
In reply to Junio C Hamano"rev-parse --parseopt" specification is clearly text and we should anticipate that we may be fed CRLF lines.
--- builtin/rev-parse.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
builtin/rev-parse.c +2 -2
@@ -386,7 +386,7 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)/* get the usage up to the first line with a -- on it */ for (;;) { -if (strbuf_getline (&sb, stdin,'\n' ) ==EOF ) +if (strbuf_getline_crlf (&sb, stdin) ==EOF )die ("premature end of input" );ALLOC_GROW (usage, unb +1 , usz);if (!strcmp ("--" , sb.buf)) {
@@ -399,7 +399,7 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix) }/* parse: (<short>|<short>,<long>|<long>)[*=?!]*<arghint>? SP+ <help> */ -while (strbuf_getline (&sb, stdin,'\n' ) !=EOF ) { +while (strbuf_getline_crlf (&sb, stdin) !=EOF ) {const char *s;const char *help;struct option *o;
Signature · 1 line
2.7.0-rc1-83-ga8b6b9e
PATCH v2 9/17 ident.c: read /etc/mailname with strbuf_getline_crlf()
In reply to Junio C HamanoJust in case /etc/mailname file was edited with a DOS editor, read it with strbuf_getline_crlf() so that a stray CR is not included as the last character of the mail hostname.
We _might_ want to more aggressively discard whitespace characters around the line with strbuf_trim(), but that is a bit outside the scope of this series.
--- ident.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
ident.c +1 -1
@@ -55,7 +55,7 @@ static int add_mailname_host(struct strbuf *buf) strerror(errno));return -1 ; } -if (strbuf_getline (&mailnamebuf, mailname,'\n' ) ==EOF ) { +if (strbuf_getline_crlf (&mailnamebuf, mailname) ==EOF ) {if (ferror (mailname))warning ("cannot read /etc/mailname: %s" ,strerror (errno));
Signature · 1 line
2.7.0-rc1-83-ga8b6b9e
PATCH v2 10/17 remote.c: read $GIT_DIR/remotes/* with strbuf_getline_crlf()
In reply to Junio C HamanoThese files can be edited with a DOS editor, leaving CR at the end of the line if read with strbuf_getline().
--- remote.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
remote.c +1 -1
@@ -256,7 +256,7 @@ static void read_remotes_file(struct remote *remote)if (!f)return ; remote->origin =REMOTE_REMOTES ; -while (strbuf_getline (&buf, f,'\n' ) !=EOF ) { +while (strbuf_getline_crlf (&buf, f) !=EOF ) {const char *v;strbuf_rtrim (&buf);
Signature · 1 line
2.7.0-rc1-83-ga8b6b9e
PATCH v2 11/17 clone/sha1_file: read info/alternates with strbuf_getline_crlf()
In reply to Junio C Hamano$GIT_OBJECT_DIRECTORY/info/alternates is a text file that can be edited with a DOS editor. We do not want to use the real path with CR appended at the end.
--- builtin/clone.c | 2 +- sha1_file.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
builtin/clone.c +1 -1
@@ -339,7 +339,7 @@ static void copy_alternates(struct strbuf *src, struct strbuf *dst,FILE *in =fopen (src->buf,"r" );struct strbuf line =STRBUF_INIT ; -while (strbuf_getline (&line, in,'\n' ) !=EOF ) { +while (strbuf_getline_crlf (&line, in) !=EOF ) {char *abs_path;if (!line.len || line.buf[0 ] =='#' )continue ;
sha1_file.c +1 -1
@@ -395,7 +395,7 @@ void add_to_alternates_file(const char *reference)struct strbuf line =STRBUF_INIT ;int found =0 ; -while (strbuf_getline (&line, in,'\n' ) !=EOF ) { +while (strbuf_getline_crlf (&line, in) !=EOF ) {if (!strcmp (reference, line.buf)) { found =1 ;break ;
Signature · 1 line
2.7.0-rc1-83-ga8b6b9e
PATCH v2 12/17 transport-helper: read helper response with strbuf_getline_crlf()
In reply to Junio C HamanoOur implementation of helpers never use CRLF line endings, and they do not depend on the ability to place a CR as payload at the end of the line, so this is essentially a no-op for in-tree users. However, this allows third-party implementation of helpers to give us their line with CRLF line ending (they cannot expect us to feed CRLF to them, though).
--- transport-helper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
transport-helper.c +1 -1
@@ -54,7 +54,7 @@ static int recvline_fh(FILE *helper, struct strbuf *buffer, const char *name)strbuf_reset (buffer);if (debug)fprintf (stderr,"Debug: Remote helper: Waiting...\n" ); -if (strbuf_getline (buffer, helper,'\n' ) ==EOF ) { +if (strbuf_getline_crlf (buffer, helper) ==EOF ) {if (debug)fprintf (stderr,"Debug: Remote helper quit.\n" );return 1 ;
Signature · 1 line
2.7.0-rc1-83-ga8b6b9e
PATCH v2 13/17 cat-file: read batch stream with strbuf_getline_crlf()
In reply to Junio C HamanoIt is possible to prepare a text file with a DOS editor and feed it as a batch command stream to the command.
--- builtin/cat-file.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
builtin/cat-file.c +1 -1
@@ -401,7 +401,7 @@ static int batch_objects(struct batch_options *opt) save_warning = warn_on_object_refname_ambiguity; warn_on_object_refname_ambiguity =0 ; -while (strbuf_getline (&buf, stdin,'\n' ) !=EOF ) { +while (strbuf_getline_crlf (&buf, stdin) !=EOF ) {if (data.split_on_whitespace) {/* * Split at first whitespace, tying off the beginning
Signature · 1 line
2.7.0-rc1-83-ga8b6b9e
PATCH v2 14/17 column: read lines with strbuf_getline_crlf()
In reply to Junio C HamanoMultiple lines read here are concatenated on a single line to form a multi-column output line. We do not want to have a CR at the end, even if the input file consists of CRLF terminated lines.
--- builtin/column.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
builtin/column.c +1 -1
@@ -51,7 +51,7 @@ int cmd_column(int argc, const char **argv, const char *prefix)die (_ ("--command must be the first argument" )); }finalize_colopts (&colopts,-1 ); -while (!strbuf_getline (&sb, stdin,'\n' )) +while (!strbuf_getline_crlf (&sb, stdin))string_list_append (&list, sb.buf);print_columns (&list, colopts, &copts);
Signature · 1 line
2.7.0-rc1-83-ga8b6b9e
PATCH v2 15/17 send-pack: read list of refs with strbuf_getline_crlf()
In reply to Junio C Hamano--- builtin/send-pack.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
builtin/send-pack.c +1 -1
@@ -212,7 +212,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)argv_array_push (&all_refspecs, buf); }else {struct strbuf line =STRBUF_INIT ; -while (strbuf_getline (&line, stdin,'\n' ) !=EOF ) +while (strbuf_getline_crlf (&line, stdin) !=EOF )argv_array_push (&all_refspecs, line.buf);strbuf_release (&line); }
Signature · 1 line
2.7.0-rc1-83-ga8b6b9e
PATCH v2 16/17 grep: read -f file with strbuf_getline_crlf()
In reply to Junio C HamanoList of patterns file could come from a DOS editor.
This is iffy; you may actually be trying to find a line with ^M in it on a system whose line ending is LF. You can of course work it around by having a line that has "^M^M^J", let the strbuf_getline_crlf() eat the last "^M^J", leaving just the single "^M" as the pattern.
--- builtin/grep.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
builtin/grep.c +1 -1
@@ -562,7 +562,7 @@ static int file_callback(const struct option *opt, const char *arg, int unset) patterns = from_stdin ? stdin :fopen (arg,"r" );if (!patterns)die_errno (_ ("cannot open '%s'" ), arg); -while (strbuf_getline (&sb, patterns,'\n' ) ==0 ) { +while (strbuf_getline_crlf (&sb, patterns) ==0 ) {/* ignore empty line like grep does */ if (sb.len ==0 )continue ;
Signature · 1 line
2.7.0-rc1-83-ga8b6b9e
Re: [PATCH v2 16/17] grep: read -f file with strbuf_getline_crlf()
Hi Junio,
Quoted 4 lines · Junio C Hamano
This is iffy; you may actually be trying to find a line with ^M in it on a system whose line ending is LF. You can of course work it around by having a line that has "^M^M^J", let the
strbuf_getline_crlf()eat the last "^M^J", leaving just the single "^M" as the pattern.
Thanks for being careful.
Having said that, grep operates on lines of text, and CR is established as a non-text byte, so I would argue that the previous behavior was actually incorrect: if you searched for, say, abc$ in a CR/LF delimited file, the outcome would not have met my expectation of correct behavior.
Ciao,
Dscho
Re: [PATCH v2 16/17] grep: read -f file with strbuf_getline_crlf()
Quoted 1 line · Johannes Schindelin
Having said that,
grepoperates on lines of text,
Correct.
Quoted 1 line
and CR is established as a non-text byte,
Correct but only if you are a pedant.
With this patch, you can no longer find "A<CR>" on a "line" that is "A<CR><LF>", which is a regression on a system whose native line ermination convention is to use a single LF at the end.
The thing is, users know "grep -e '<a non-text byte>' finds hits with a line with the non-text byte, and telling them that they shouldn't rely on the feature would not make the world a better place.
This patch limits the damage by restricting the special casing of CR only at the end of the line just before LF, so we can still find "A<CR>B" on a "line" that has a non-text byte, e.g. "A<CR>B<CR><LF>" or "A<CR>B<LF>". It is just a <CR> at the end of the line (with or without any other string before it) for which we see a regression, and I suspect that it would be a small enough price to accept to help those who would need to work with <CRLF> systems.
PATCH v2 17/17 test-sha1-array: read command stream with strbuf_getline_crlf()
In reply to Junio C HamanoThe input to this command comes from a pipeline in t0064, whose upstream has bunch of "echo"s. It is not unreasonable to expect that it may be fed CRLF lines on DOSsy systems.
--- test-sha1-array.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
test-sha1-array.c +1 -1
@@ -11,7 +11,7 @@ int main(int argc, char **argv)struct sha1_array array =SHA1_ARRAY_INIT ;struct strbuf line =STRBUF_INIT ; -while (strbuf_getline (&line, stdin,'\n' ) !=EOF ) { +while (strbuf_getline_crlf (&line, stdin) !=EOF ) {const char *arg;unsigned char sha1[20 ];
Signature · 1 line
2.7.0-rc1-83-ga8b6b9e
Re: [PATCH v2 17/17] test-sha1-array: read command stream with strbuf_getline_crlf()
Hi Junio,
I read through the entire series. I hope you find my comments helpful.
Happy new year,
Dscho
Re: [PATCH v2 00/17] Peace with CRLF
In reply to Junio C HamanoHi Junio,
Quoted 37 lines · Junio C Hamano
I inspected all the callsites of this function to see if it is safe to use such an updated logic at these callsites, and did not find anything problematic. I could update
strbuf_getline()in place, but just to be extra careful, this series instead introduces another helper,strbuf_getline_crlf(), that is aware of this CRLF business, and convert the ones that are safe to update as we verify.* This series converts only the callers of
strbuf_getline()that would misbehave when fed a file with CRLF-terminated lines and use the data with an unwanted CR appended at the end. With the update the code should work as intended with such a file, without breaking the expected behaviour when working on a file with LF-terminated lines.* Callers of
strbuf_getline()that expect to only read from our own output do not have to accommodate CRLF-terminated lines, but they can be updated to do so safely if we do not rely on the ability to express a payload that has CR at the end. For example, the insn sheet "rebase -i" uses typically ends each line with a commit log summary that we ourselves do not read or use, so even if the end-user on a platform with LF lines deliberately does insert \r at the end of the line andstrbuf_gets()removed that \r from the payload, no unexpected behaviour should happen.This series does not touch them, but it may be a good GSoC microproject to convert them to use
strbuf_getline_crlf().* Callers of
strbuf_getline()that callstrbuf_trim()immediately on the result before doing anything, or otherwise have logic that tolerates whitespaces at the end of the line, can continue usingstrbuf_getline()and will not misbehave on CRLF-terminated lines.This series does not touch them; converting them to use
strbuf_getline_crlf()would a good way to document them as dealing with "text". While doing so, they can also lose their custom logic that happens to make CRLF-terminated lines work.
Quoted 5 lines
* A caller of
strbuf_getline()that wants to only see LF-terminated lines (in other words, "ABC\r\n" must be treated as a line with 4 bytes payload on it, A B C and CR), would be broken if we replace it withstrbuf_getline_crlf(). This series does not touch them, and no follow-up series should, either.
Thanks for the detailed explanation. I totally agree that (2) would make for a good micro-project.
While I am pretty convinced that strbuf_getline() that retains a CR makes no sense, it is obviously the correct thing to prove that rather than assume it (after all, some caller might exploit strbuf_getline() for its auto-growing capabilities even on a binary stream when it is known that it cannot contain a 0x0a).
And please accept my gratitude for tackling this project.
Ciao,
Dscho
v3 cover Preview of "Peace with CRLF" rerolled
In reply to Junio C HamanoContinued from
http://thread.gmane.org/gmane.comp.version-control.git/282614
Here is only the preparatory part that
- introduces strbuf_getline_lf() and strbuf_getine_nul() and converts existing callers to strbut_getline() to use them;
- retires strbuf_getline() that takes an arbitrary line terminator and demotes it to strbuf_getdelim() that is merely an implementation detail inside strbuf.c; and
- gives the short-and-sweet name strbuf_getline() to the most "text friendly" strbuf_getline_crlf().
I am sending this out as preview now before implementing the main part of the series that audits callers of strbuf_getline_lf() and selectively turns them into strbuf_getline(). The work I did for v2 that corresponds to that main part will have to be redone because of the changes to function names made by these preparatory steps, and it would be rather boring mechanical and time-consuming work that is prone to stupid mistakes, which I do not want to start late in the day.
Junio C Hamano (9):
strbuf: miniscule style fix
strbuf: make strbuf_getline_crlf() global
strbuf: introduce strbuf_getline_{lf,nul}()
mktree: there are only two line terminators
check-attr: there are only two line terminators
check-ignore: there are only two line terminators
update-index: there are only two line terminators
checkout-index: there are only two line terminators
strbuf: give strbuf_getline() to the "most text friendly" variant
bisect.c | 8 ++++---- builtin/am.c | 37 +++++++++++-------------------------- builtin/cat-file.c | 2 +- builtin/check-attr.c | 7 ++++--- builtin/check-ignore.c | 7 ++++--- builtin/check-mailmap.c | 2 +- builtin/checkout-index.c | 10 +++++++--- builtin/clean.c | 6 +++--- builtin/clone.c | 2 +- builtin/column.c | 2 +- builtin/commit.c | 2 +- builtin/fetch-pack.c | 2 +- builtin/grep.c | 2 +- builtin/hash-object.c | 2 +- builtin/mailinfo.c | 8 ++++---- builtin/mktree.c | 14 ++++++++------ builtin/notes.c | 2 +- builtin/pull.c | 2 +- builtin/repack.c | 2 +- builtin/rev-parse.c | 4 ++-- builtin/send-pack.c | 2 +- builtin/update-index.c | 27 ++++++++++++++++----------- compat/terminal.c | 2 +- credential-cache--daemon.c | 4 ++-- credential-store.c | 2 +- credential.c | 2 +- daemon.c | 2 +- fast-import.c | 4 ++-- ident.c | 2 +- remote-curl.c | 6 +++--- remote-testsvn.c | 4 ++-- remote.c | 4 ++-- sequencer.c | 2 +- sha1_file.c | 2 +- shell.c | 2 +- strbuf.c | 28 +++++++++++++++++++++++++--- strbuf.h | 27 ++++++++++++++++++++++----- test-sha1-array.c | 2 +- transport-helper.c | 5 +++-- walker.c | 2 +- wt-status.c | 4 ++-- 41 files changed, 148 insertions(+), 110 deletions(-)
Signature · 1 line
2.7.0-242-gdd583c7
PATCH v3 1/9 strbuf: miniscule style fix
We write one SP on each side of an operator, even inside an [] pair that computes the array index.
--- strbuf.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
strbuf.c +2 -2
@@ -505,8 +505,8 @@ int strbuf_getline(struct strbuf *sb, FILE *fp, int term) {if (strbuf_getwholeline (sb, fp, term))return EOF ; -if (sb->buf[sb->len-1 ] == term) -strbuf_setlen (sb, sb->len-1 ); +if (sb->buf[sb->len -1 ] == term) +strbuf_setlen (sb, sb->len -1 );return 0 ; }
Signature · 1 line
2.7.0-242-gdd583c7
PATCH v3 2/9 strbuf: make strbuf_getline_crlf() global
In reply to Junio C HamanoOften we read "text" files that are supplied by the end user (e.g. commit log message that was edited with $GIT_EDITOR upon 'git commit -e'), and in some environments lines in a text file are terminated with CRLF. Existing strbuf_getline() knows to read a single line and then strip the terminating byte from the result, but it is handy to have a version that is more tailored for a "text" input that takes both '\n' and '\r\n' as line terminator (aka <newline> in POSIX lingo) and returns the body of the line after stripping <newline>.
Recently reimplemented "git am" uses such a function implemented privately; move it to strbuf.[ch] and make it available for others.
Note that we do not blindly replace calls to strbuf_getline() that uses LF as the line terminator with calls to strbuf_getline_crlf() and this is very much deliberate. Some callers may want to treat an incoming line that ends with CR (and terminated with LF) to have a payload that includes the final CR, and such a blind replacement will result in misconversion when done without code audit.
--- builtin/am.c | 15 --------------- strbuf.c | 12 ++++++++++++ strbuf.h | 7 +++++++ 3 files changed, 19 insertions(+), 15 deletions(-)
builtin/am.c +0 -15
@@ -45,21 +45,6 @@ static int is_empty_file(const char *filename) }/** -* Like strbuf_getline(), but treats both '\n' and "\r\n" as line terminators. -*/ -static int strbuf_getline_crlf (struct strbuf *sb,FILE *fp) -{ -if (strbuf_getwholeline (sb, fp,'\n' )) -return EOF ; -if (sb->buf[sb->len -1 ] =='\n' ) { -strbuf_setlen (sb, sb->len -1 ); -if (sb->len >0 && sb->buf[sb->len -1 ] =='\r' ) -strbuf_setlen (sb, sb->len -1 ); - } -return 0 ; -} - -/** * Returns the length of the first line of msg. */ static int linelen (const char *msg)
strbuf.c +12 -0
@@ -510,6 +510,18 @@ int strbuf_getline(struct strbuf *sb, FILE *fp, int term)return 0 ; } +int strbuf_getline_crlf (struct strbuf *sb,FILE *fp) +{ +if (strbuf_getwholeline (sb, fp,'\n' )) +return EOF ; +if (sb->buf[sb->len -1 ] =='\n' ) { +strbuf_setlen (sb, sb->len -1 ); +if (sb->len && sb->buf[sb->len -1 ] =='\r' ) +strbuf_setlen (sb, sb->len -1 ); + } +return 0 ; +} +int strbuf_getwholeline_fd (struct strbuf *sb,int fd,int term) {strbuf_reset (sb);
strbuf.h +7 -0
@@ -388,6 +388,13 @@ extern int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint);*/ extern int strbuf_getline (struct strbuf *,FILE *,int ); +/* +* Similar to strbuf_getline(), but uses '\n' as the terminator, +* and additionally treats a '\r' that comes immediately before '\n' +* as part of the terminator. +*/ +extern int strbuf_getline_crlf (struct strbuf *,FILE *); +/** * Like `strbuf_getline`, but keeps the trailing terminator (if * any) in the buffer.
Signature · 1 line
2.7.0-242-gdd583c7
PATCH v3 3/9 strbuf: introduce strbuf_getline_{lf,nul}()
In reply to Junio C HamanoThe strbuf_getline() interface allows a byte other than LF or NUL as the line terminator, but this is only 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. No useful caller that uses other value has emerged.
By now, it is clear that the interface is overly broad without a good reason. Many codepaths have hardcoded preference to read either LF terminated or NUL terminated records from their input, and then call strbuf_getline() with LF or NUL as the third parameter.
This step introduces two thin wrappers around strbuf_getline(), namely, strbuf_getline_lf() and strbuf_getline_nul(), and mechanically rewrites these call sites to call either one of them. The changes contained in this patch are:
* introduction of these two functions in strbuf.[ch]
* mechanical conversion of all callers to strbuf_getline() with either '\n' or '\0' as the third parameter to instead call the respective thin wrapper.
After this step, output from "git grep 'strbuf_getline('" would become a lot smaller. An interim goal of this series is to make this an empty set, so that we can have strbuf_getline_crlf() take over the shorter name strbuf_getline().
--- bisect.c | 8 ++++---- builtin/am.c | 14 +++++++------- builtin/cat-file.c | 2 +- builtin/check-mailmap.c | 2 +- builtin/clean.c | 6 +++--- builtin/clone.c | 2 +- builtin/column.c | 2 +- builtin/commit.c | 2 +- builtin/fetch-pack.c | 2 +- builtin/grep.c | 2 +- builtin/hash-object.c | 2 +- builtin/mailinfo.c | 8 ++++---- builtin/notes.c | 2 +- builtin/pull.c | 2 +- builtin/repack.c | 2 +- builtin/rev-parse.c | 4 ++-- builtin/send-pack.c | 2 +- compat/terminal.c | 2 +- credential-cache--daemon.c | 4 ++-- credential-store.c | 2 +- credential.c | 2 +- daemon.c | 2 +- fast-import.c | 4 ++-- ident.c | 2 +- remote-curl.c | 6 +++--- remote-testsvn.c | 4 ++-- remote.c | 4 ++-- sequencer.c | 2 +- sha1_file.c | 2 +- shell.c | 2 +- strbuf.c | 10 ++++++++++ strbuf.h | 7 +++++++ test-sha1-array.c | 2 +- transport-helper.c | 2 +- walker.c | 2 +- wt-status.c | 4 ++-- 36 files changed, 73 insertions(+), 56 deletions(-)
bisect.c +4 -4
@@ -440,7 +440,7 @@ static void read_bisect_paths(struct argv_array *array)if (!fp)die_errno ("Could not open file '%s'" , filename); -while (strbuf_getline (&str, fp,'\n' ) !=EOF ) { +while (strbuf_getline_lf (&str, fp) !=EOF ) {strbuf_trim (&str);if (sq_dequote_to_argv_array (str.buf, array)) die("Badly quoted content in file '%s': %s",
@@ -668,7 +668,7 @@ static int is_expected_rev(const struct object_id *oid)if (!fp)return 0 ; -if (strbuf_getline (&str, fp,'\n' ) !=EOF ) +if (strbuf_getline_lf (&str, fp) !=EOF ) res = !strcmp (str.buf,oid_to_hex (oid));strbuf_release (&str);
@@ -914,9 +914,9 @@ void read_bisect_terms(const char **read_bad, const char **read_good) strerror(errno)); } }else { -strbuf_getline (&str, fp,'\n' ); +strbuf_getline_lf (&str, fp); *read_bad =strbuf_detach (&str,NULL ); -strbuf_getline (&str, fp,'\n' ); +strbuf_getline_lf (&str, fp); *read_good =strbuf_detach (&str,NULL ); }strbuf_release (&str);
builtin/am.c +7 -7
@@ -268,7 +268,7 @@ static char *read_shell_var(FILE *fp, const char *key)struct strbuf sb =STRBUF_INIT ;const char *str; -if (strbuf_getline (&sb, fp,'\n' )) +if (strbuf_getline_lf (&sb, fp)) goto fail;if (!skip_prefix (sb.buf, key, &str))
@@ -557,7 +557,7 @@ static int copy_notes_for_rebase(const struct am_state *state) fp =xfopen (am_path (state,"rewritten" ),"r" ); -while (!strbuf_getline (&sb, fp,'\n' )) { +while (!strbuf_getline_lf (&sb, fp)) {unsigned char from_obj[GIT_SHA1_RAWSZ ], to_obj[GIT_SHA1_RAWSZ ];if (sb.len !=GIT_SHA1_HEXSZ *2 +1 ) {
@@ -801,7 +801,7 @@ static int stgit_patch_to_mail(FILE *out, FILE *in, int keep_cr)struct strbuf sb =STRBUF_INIT ;int subject_printed =0 ; -while (!strbuf_getline (&sb, in,'\n' )) { +while (!strbuf_getline_lf (&sb, in)) {const char *str;if (str_isspace (sb.buf))
@@ -859,7 +859,7 @@ static int split_mail_stgit_series(struct am_state *state, const char **paths,return error (_ ("could not open '%s' for reading: %s" ), *paths,strerror (errno)); -while (!strbuf_getline (&sb, fp,'\n' )) { +while (!strbuf_getline_lf (&sb, fp)) {if (*sb.buf =='#' )continue ;/* skip comment lines */
@@ -884,7 +884,7 @@ static int hg_patch_to_mail(FILE *out, FILE *in, int keep_cr) {struct strbuf sb =STRBUF_INIT ; -while (!strbuf_getline (&sb, in,'\n' )) { +while (!strbuf_getline_lf (&sb, in)) {const char *str;if (skip_prefix (sb.buf,"# User " , &str))
@@ -1298,7 +1298,7 @@ static int parse_mail(struct am_state *state, const char *mail)/* Extract message and author information */ fp =xfopen (am_path (state,"info" ),"r" ); -while (!strbuf_getline (&sb, fp,'\n' )) { +while (!strbuf_getline_lf (&sb, fp)) {const char *x;if (skip_prefix (sb.buf,"Subject: " , &x)) {
@@ -1364,7 +1364,7 @@ static int get_mail_commit_sha1(unsigned char *commit_id, const char *mail)FILE *fp =xfopen (mail,"r" );const char *x; -if (strbuf_getline (&sb, fp,'\n' )) +if (strbuf_getline_lf (&sb, fp))return -1 ;if (!skip_prefix (sb.buf,"From " , &x))
builtin/cat-file.c +1 -1
@@ -401,7 +401,7 @@ static int batch_objects(struct batch_options *opt) save_warning = warn_on_object_refname_ambiguity; warn_on_object_refname_ambiguity =0 ; -while (strbuf_getline (&buf, stdin,'\n' ) !=EOF ) { +while (strbuf_getline_lf (&buf, stdin) !=EOF ) {if (data.split_on_whitespace) {/* * Split at first whitespace, tying off the beginning
builtin/check-mailmap.c +1 -1
@@ -54,7 +54,7 @@ int cmd_check_mailmap(int argc, const char **argv, const char *prefix)if (use_stdin) {struct strbuf buf =STRBUF_INIT ; -while (strbuf_getline (&buf, stdin,'\n' ) !=EOF ) { +while (strbuf_getline_lf (&buf, stdin) !=EOF ) {check_mailmap (&mailmap, buf.buf);maybe_flush_or_die (stdout,"stdout" ); }
builtin/clean.c +3 -3
@@ -594,7 +594,7 @@ static int *list_and_choose(struct menu_opts *opts, struct menu_stuff *stuff) clean_get_color(CLEAN_COLOR_RESET)); } -if (strbuf_getline (&choice, stdin,'\n' ) !=EOF ) { +if (strbuf_getline_lf (&choice, stdin) !=EOF ) {strbuf_trim (&choice); }else { eof =1 ;
@@ -676,7 +676,7 @@ static int filter_by_patterns_cmd(void)clean_print_color (CLEAN_COLOR_PROMPT );printf (_ ("Input ignore patterns>> " ));clean_print_color (CLEAN_COLOR_RESET ); -if (strbuf_getline (&confirm, stdin,'\n' ) !=EOF ) +if (strbuf_getline_lf (&confirm, stdin) !=EOF )strbuf_trim (&confirm);else putchar ('\n' );
@@ -774,7 +774,7 @@ static int ask_each_cmd(void) qname =quote_path_relative (item->string,NULL , &buf);/* TRANSLATORS: Make sure to keep [y/N] as is */ printf (_ ("Remove %s [y/N]? " ), qname); -if (strbuf_getline (&confirm, stdin,'\n' ) !=EOF ) { +if (strbuf_getline_lf (&confirm, stdin) !=EOF ) {strbuf_trim (&confirm); }else {putchar ('\n' );
builtin/clone.c +1 -1
@@ -339,7 +339,7 @@ static void copy_alternates(struct strbuf *src, struct strbuf *dst,FILE *in =fopen (src->buf,"r" );struct strbuf line =STRBUF_INIT ; -while (strbuf_getline (&line, in,'\n' ) !=EOF ) { +while (strbuf_getline_lf (&line, in) !=EOF ) {char *abs_path;if (!line.len || line.buf[0 ] =='#' )continue ;
builtin/column.c +1 -1
@@ -51,7 +51,7 @@ int cmd_column(int argc, const char **argv, const char *prefix)die (_ ("--command must be the first argument" )); }finalize_colopts (&colopts,-1 ); -while (!strbuf_getline (&sb, stdin,'\n' )) +while (!strbuf_getline_lf (&sb, stdin))string_list_append (&list, sb.buf);print_columns (&list, colopts, &copts);
builtin/commit.c +1 -1
@@ -1687,7 +1687,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)if (fp ==NULL )die_errno (_ ("could not open '%s' for reading" ),git_path_merge_head ()); -while (strbuf_getline (&m, fp,'\n' ) !=EOF ) { +while (strbuf_getline_lf (&m, fp) !=EOF ) {struct commit *parent; parent =get_merge_parent (m.buf);
builtin/fetch-pack.c +1 -1
@@ -156,7 +156,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)else {/* read from stdin one ref per line, until EOF */ struct strbuf line =STRBUF_INIT ; -while (strbuf_getline (&line, stdin,'\n' ) !=EOF ) +while (strbuf_getline_lf (&line, stdin) !=EOF )add_sought_entry (&sought, &nr_sought, &alloc_sought, line.buf);strbuf_release (&line); }
builtin/grep.c +1 -1
@@ -562,7 +562,7 @@ static int file_callback(const struct option *opt, const char *arg, int unset) patterns = from_stdin ? stdin :fopen (arg,"r" );if (!patterns)die_errno (_ ("cannot open '%s'" ), arg); -while (strbuf_getline (&sb, patterns,'\n' ) ==0 ) { +while (strbuf_getline_lf (&sb, patterns) ==0 ) {/* ignore empty line like grep does */ if (sb.len ==0 )continue ;
builtin/hash-object.c +1 -1
@@ -60,7 +60,7 @@ static void hash_stdin_paths(const char *type, int no_filters, unsigned flags, {struct strbuf buf =STRBUF_INIT , nbuf =STRBUF_INIT ; -while (strbuf_getline (&buf, stdin,'\n' ) !=EOF ) { +while (strbuf_getline_lf (&buf, stdin) !=EOF ) {if (buf.buf[0 ] =='"' ) {strbuf_reset (&nbuf);if (unquote_c_style (&nbuf, buf.buf,NULL ))
builtin/mailinfo.c +4 -4
@@ -410,7 +410,7 @@ static int is_rfc2822_header(const struct strbuf *line)static int read_one_header_line (struct strbuf *line,FILE *in) {/* Get the first part of the line. */ -if (strbuf_getline (line, in,'\n' )) +if (strbuf_getline_lf (line, in))return 0 ;/*
@@ -435,7 +435,7 @@ static int read_one_header_line(struct strbuf *line, FILE *in) peek =fgetc (in);ungetc (peek, in);if (peek !=' ' && peek !='\t' )break ; -if (strbuf_getline (&continuation, in,'\n' )) +if (strbuf_getline_lf (&continuation, in))break ; continuation.buf[0 ] =' ' ;strbuf_rtrim (&continuation);
@@ -644,7 +644,7 @@ static void handle_filter(struct strbuf *line);static int find_boundary (void ) { -while (!strbuf_getline (&line, fin,'\n' )) { +while (!strbuf_getline_lf (&line, fin)) {if (*content_top &&is_multipart_boundary (&line))return 1 ; }
@@ -692,7 +692,7 @@ again:strbuf_release (&newline);/* replenish line */ -if (strbuf_getline (&line, fin,'\n' )) +if (strbuf_getline_lf (&line, fin))return 0 ;strbuf_addch (&line,'\n' );return 1 ;
builtin/notes.c +1 -1
@@ -290,7 +290,7 @@ static int notes_copy_from_stdin(int force, const char *rewrite_cmd) t = &default_notes_tree; } -while (strbuf_getline (&buf, stdin,'\n' ) !=EOF ) { +while (strbuf_getline_lf (&buf, stdin) !=EOF ) {unsigned char from_obj[20 ], to_obj[20 ];struct strbuf **split;int err;
builtin/pull.c +1 -1
@@ -378,7 +378,7 @@ static void get_merge_heads(struct sha1_array *merge_heads)if (!(fp =fopen (filename,"r" )))die_errno (_ ("could not open '%s' for reading" ), filename); -while (strbuf_getline (&sb, fp,'\n' ) !=EOF ) { +while (strbuf_getline_lf (&sb, fp) !=EOF ) {if (get_sha1_hex (sb.buf, sha1))continue ;/* invalid line: does not start with SHA1 */ if (starts_with (sb.buf +GIT_SHA1_HEXSZ ,"\tnot-for-merge\t" ))
builtin/repack.c +1 -1
@@ -266,7 +266,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)return ret; out =xfdopen (cmd.out,"r" ); -while (strbuf_getline (&line, out,'\n' ) !=EOF ) { +while (strbuf_getline_lf (&line, out) !=EOF ) {if (line.len !=40 )die ("repack: Expecting 40 character sha1 lines only from pack-objects." );string_list_append (&names, line.buf);
builtin/rev-parse.c +2 -2
@@ -386,7 +386,7 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)/* get the usage up to the first line with a -- on it */ for (;;) { -if (strbuf_getline (&sb, stdin,'\n' ) ==EOF ) +if (strbuf_getline_lf (&sb, stdin) ==EOF )die ("premature end of input" );ALLOC_GROW (usage, unb +1 , usz);if (!strcmp ("--" , sb.buf)) {
@@ -399,7 +399,7 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix) }/* parse: (<short>|<short>,<long>|<long>)[*=?!]*<arghint>? SP+ <help> */ -while (strbuf_getline (&sb, stdin,'\n' ) !=EOF ) { +while (strbuf_getline_lf (&sb, stdin) !=EOF ) {const char *s;const char *help;struct option *o;
builtin/send-pack.c +1 -1
@@ -212,7 +212,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)argv_array_push (&all_refspecs, buf); }else {struct strbuf line =STRBUF_INIT ; -while (strbuf_getline (&line, stdin,'\n' ) !=EOF ) +while (strbuf_getline_lf (&line, stdin) !=EOF )argv_array_push (&all_refspecs, line.buf);strbuf_release (&line); }
compat/terminal.c +1 -1
@@ -122,7 +122,7 @@ char *git_terminal_prompt(const char *prompt, int echo)fputs (prompt, output_fh);fflush (output_fh); - r =strbuf_getline (&buf, input_fh,'\n' ); + r =strbuf_getline_lf (&buf, input_fh);if (!echo) {putc ('\n' , output_fh);fflush (output_fh);
credential-cache--daemon.c +2 -2
@@ -97,12 +97,12 @@ static int read_request(FILE *fh, struct credential *c,static struct strbuf item =STRBUF_INIT ;const char *p; -strbuf_getline (&item, fh,'\n' ); +strbuf_getline_lf (&item, fh);if (!skip_prefix (item.buf,"action=" , &p))return error ("client sent bogus action line: %s" , item.buf);strbuf_addstr (action, p); -strbuf_getline (&item, fh,'\n' ); +strbuf_getline_lf (&item, fh);if (!skip_prefix (item.buf,"timeout=" , &p))return error ("client sent bogus timeout line: %s" , item.buf); *timeout =atoi (p);
credential-store.c +1 -1
@@ -23,7 +23,7 @@ static int parse_credential_file(const char *fn,return found_credential; } -while (strbuf_getline (&line, fh,'\n' ) !=EOF ) { +while (strbuf_getline_lf (&line, fh) !=EOF ) {credential_from_url (&entry, line.buf);if (entry.username && entry.password &&credential_match (c, &entry)) {
credential.c +1 -1
@@ -142,7 +142,7 @@ int credential_read(struct credential *c, FILE *fp) {struct strbuf line =STRBUF_INIT ; -while (strbuf_getline (&line, fp,'\n' ) !=EOF ) { +while (strbuf_getline_lf (&line, fp) !=EOF ) {char *key = line.buf;char *value =strchr (key,'=' );
daemon.c +1 -1
@@ -424,7 +424,7 @@ static void copy_to_log(int fd)return ; } -while (strbuf_getline (&line, fp,'\n' ) !=EOF ) { +while (strbuf_getline_lf (&line, fp) !=EOF ) {logerror ("%s" , line.buf);strbuf_setlen (&line,0 ); }
fast-import.c +2 -2
@@ -1888,7 +1888,7 @@ static int read_next_command(void)struct recent_command *rc;strbuf_detach (&command_buf,NULL ); - stdin_eof =strbuf_getline (&command_buf, stdin,'\n' ); + stdin_eof =strbuf_getline_lf (&command_buf, stdin);if (stdin_eof)return EOF ;
@@ -1960,7 +1960,7 @@ static int parse_data(struct strbuf *sb, uintmax_t limit, uintmax_t *len_res)strbuf_detach (&command_buf,NULL );for (;;) { -if (strbuf_getline (&command_buf, stdin,'\n' ) ==EOF ) +if (strbuf_getline_lf (&command_buf, stdin) ==EOF )die ("EOF in data (terminator '%s' not found)" , term);if (term_len == command_buf.len && !strcmp (term, command_buf.buf))
ident.c +1 -1
@@ -55,7 +55,7 @@ static int add_mailname_host(struct strbuf *buf) strerror(errno));return -1 ; } -if (strbuf_getline (&mailnamebuf, mailname,'\n' ) ==EOF ) { +if (strbuf_getline_lf (&mailnamebuf, mailname) ==EOF ) {if (ferror (mailname))warning ("cannot read /etc/mailname: %s" ,strerror (errno));
remote-curl.c +3 -3
@@ -827,7 +827,7 @@ static void parse_fetch(struct strbuf *buf)die ("http transport does not support %s" , buf->buf);strbuf_reset (buf); -if (strbuf_getline (buf, stdin,'\n' ) ==EOF ) +if (strbuf_getline_lf (buf, stdin) ==EOF )return ;if (!*buf->buf)break ;
@@ -940,7 +940,7 @@ static void parse_push(struct strbuf *buf)die ("http transport does not support %s" , buf->buf);strbuf_reset (buf); -if (strbuf_getline (buf, stdin,'\n' ) ==EOF ) +if (strbuf_getline_lf (buf, stdin) ==EOF ) goto free_specs;if (!*buf->buf)break ;
@@ -990,7 +990,7 @@ int main(int argc, const char **argv) do { const char *arg; - if (strbuf_getline(&buf, stdin, '\n') == EOF) { + if (strbuf_getline_lf(&buf, stdin) == EOF) { if (ferror(stdin)) error("remote-curl: error reading command stream from git"); return 1;
remote-testsvn.c +2 -2
@@ -154,7 +154,7 @@ static void check_or_regenerate_marks(int latestrev)fclose (marksfile); }else {strbuf_addf (&sb,":%d " , latestrev); -while (strbuf_getline (&line, marksfile,'\n' ) !=EOF ) { +while (strbuf_getline_lf (&line, marksfile) !=EOF ) {if (starts_with (line.buf, sb.buf)) { found++;break ;
@@ -322,7 +322,7 @@ int main(int argc, char **argv) marksfilename = marksfilename_sb.buf;while (1 ) { -if (strbuf_getline (&buf, stdin,'\n' ) ==EOF ) { +if (strbuf_getline_lf (&buf, stdin) ==EOF ) {if (ferror (stdin))die ("Error reading command stream" );else
remote.c +2 -2
@@ -256,7 +256,7 @@ static void read_remotes_file(struct remote *remote)if (!f)return ; remote->origin =REMOTE_REMOTES ; -while (strbuf_getline (&buf, f,'\n' ) !=EOF ) { +while (strbuf_getline_lf (&buf, f) !=EOF ) {const char *v;strbuf_rtrim (&buf);
@@ -281,7 +281,7 @@ static void read_branches_file(struct remote *remote)if (!f)return ; -strbuf_getline (&buf, f,'\n' ); +strbuf_getline_lf (&buf, f);strbuf_trim (&buf);if (!buf.len) {strbuf_release (&buf);
sequencer.c +1 -1
@@ -886,7 +886,7 @@ static int sequencer_rollback(struct replay_opts *opts)if (!f)return error (_ ("cannot open %s: %s" ),git_path_head_file (),strerror (errno)); -if (strbuf_getline (&buf, f,'\n' )) { +if (strbuf_getline_lf (&buf, f)) {error (_ ("cannot read %s: %s" ),git_path_head_file (),ferror (f) ?strerror (errno) :_ ("unexpected end of file" ));fclose (f);
sha1_file.c +1 -1
@@ -395,7 +395,7 @@ void add_to_alternates_file(const char *reference)struct strbuf line =STRBUF_INIT ;int found =0 ; -while (strbuf_getline (&line, in,'\n' ) !=EOF ) { +while (strbuf_getline_lf (&line, in) !=EOF ) {if (!strcmp (reference, line.buf)) { found =1 ;break ;
shell.c +1 -1
@@ -88,7 +88,7 @@ static void run_shell(void)int count;fprintf (stderr,"git> " ); -if (strbuf_getline (&line, stdin,'\n' ) ==EOF ) { +if (strbuf_getline_lf (&line, stdin) ==EOF ) {fprintf (stderr,"\n" );strbuf_release (&line);break ;
strbuf.c +10 -0
@@ -522,6 +522,16 @@ int strbuf_getline_crlf(struct strbuf *sb, FILE *fp)return 0 ; } +int strbuf_getline_lf (struct strbuf *sb,FILE *fp) +{ +return strbuf_getline (sb, fp,'\n' ); +} + +int strbuf_getline_nul (struct strbuf *sb,FILE *fp) +{ +return strbuf_getline (sb, fp,'\0' ); +} +int strbuf_getwholeline_fd (struct strbuf *sb,int fd,int term) {strbuf_reset (sb);
strbuf.h +7 -0
@@ -388,6 +388,12 @@ extern int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint);*/ extern int strbuf_getline (struct strbuf *,FILE *,int ); + +typedef int (*strbuf_getline_fn )(struct strbuf *,FILE *); + +extern int strbuf_getline_lf (struct strbuf *sb,FILE *fp); +extern int strbuf_getline_nul (struct strbuf *sb,FILE *fp); +/* * Similar to strbuf_getline(), but uses '\n' as the terminator, * and additionally treats a '\r' that comes immediately before '\n'
@@ -395,6 +401,7 @@ extern int strbuf_getline(struct strbuf *, FILE *, int);*/ extern int strbuf_getline_crlf (struct strbuf *,FILE *); +/** * Like `strbuf_getline`, but keeps the trailing terminator (if * any) in the buffer.
test-sha1-array.c +1 -1
@@ -11,7 +11,7 @@ int main(int argc, char **argv)struct sha1_array array =SHA1_ARRAY_INIT ;struct strbuf line =STRBUF_INIT ; -while (strbuf_getline (&line, stdin,'\n' ) !=EOF ) { +while (strbuf_getline_lf (&line, stdin) !=EOF ) {const char *arg;unsigned char sha1[20 ];
transport-helper.c +1 -1
@@ -54,7 +54,7 @@ static int recvline_fh(FILE *helper, struct strbuf *buffer, const char *name)strbuf_reset (buffer);if (debug)fprintf (stderr,"Debug: Remote helper: Waiting...\n" ); -if (strbuf_getline (buffer, helper,'\n' ) ==EOF ) { +if (strbuf_getline_lf (buffer, helper) ==EOF ) {if (debug)fprintf (stderr,"Debug: Remote helper quit.\n" );return 1 ;
walker.c +1 -1
@@ -220,7 +220,7 @@ int walker_targets_stdin(char ***target, const char ***write_ref)char *rf_one =NULL ;char *tg_one; -if (strbuf_getline (&buf, stdin,'\n' ) ==EOF ) +if (strbuf_getline_lf (&buf, stdin) ==EOF )break ; tg_one = buf.buf; rf_one =strchr (tg_one,'\t' );
wt-status.c +2 -2
@@ -988,7 +988,7 @@ static char *read_line_from_git_path(const char *filename)strbuf_release (&buf);return NULL ; } -strbuf_getline (&buf, fp,'\n' ); +strbuf_getline_lf (&buf, fp);if (!fclose (fp)) {return strbuf_detach (&buf,NULL ); }else {
@@ -1076,7 +1076,7 @@ static void read_rebase_todolist(const char *fname, struct string_list *lines)if (!f)die_errno ("Could not open file %s for reading" ,git_path ("%s" , fname)); -while (!strbuf_getline (&line, f,'\n' )) { +while (!strbuf_getline_lf (&line, f)) {if (line.len && line.buf[0 ] == comment_line_char)continue ;strbuf_trim (&line);
Signature · 1 line
2.7.0-242-gdd583c7
PATCH v3 4/9 mktree: there are only two line terminators
In reply to Junio C HamanoThe program by default works on LF terminated lines, with an option to use NUL terminated records. Instead of pretending that there can be other useful values that can be stuffed into line_termination, make it a boolean "lf_lines", and switch between strbuf_getline_{lf,nul} based on it.
--- builtin/mktree.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)
builtin/mktree.c +8 -6
@@ -65,7 +65,7 @@ static const char *mktree_usage[] = { NULL }; -static void mktree_line (char *buf,size_t len,int line_termination,int allow_missing) +static void mktree_line (char *buf,size_t len,int lf_lines,int allow_missing) {char *ptr, *ntr;unsigned mode;
@@ -97,7 +97,7 @@ static void mktree_line(char *buf, size_t len, int line_termination, int allow_m *ntr++ =0 ;/* now at the beginning of SHA1 */ path = ntr +41 ;/* at the beginning of name */ -if (line_termination && path[0 ] =='"' ) { +if (lf_lines && path[0 ] =='"' ) {struct strbuf p_uq =STRBUF_INIT ;if (unquote_c_style (&p_uq, path,NULL ))die ("invalid quoting" );
@@ -141,23 +141,25 @@ int cmd_mktree(int ac, const char **av, const char *prefix) {struct strbuf sb =STRBUF_INIT ;unsigned char sha1[20 ]; -int line_termination ='\n' ; +int lf_lines =1 ;int allow_missing =0 ;int is_batch_mode =0 ;int got_eof =0 ; +strbuf_getline_fn getline_fn;const struct option option[] = { -OPT_SET_INT ('z' ,NULL , &line_termination,N_ ("input is NUL terminated" ),'\0' ), +OPT_SET_INT ('z' ,NULL , &lf_lines,N_ ("input is NUL terminated" ),'\0' ),OPT_SET_INT (0 ,"missing" , &allow_missing,N_ ("allow missing objects" ),1 ),OPT_SET_INT (0 ,"batch" , &is_batch_mode,N_ ("allow creation of more than one tree" ),1 ),OPT_END () }; ac =parse_options (ac, av, prefix, option, mktree_usage,0 ); + getline_fn = lf_lines ? strbuf_getline_lf : strbuf_getline_nul;while (!got_eof) {while (1 ) { -if (strbuf_getline (&sb, stdin, line_termination) ==EOF ) { +if (getline_fn (&sb, stdin) ==EOF ) { got_eof =1 ;break ; }
@@ -167,7 +169,7 @@ int cmd_mktree(int ac, const char **av, const char *prefix)break ;die ("input format error: (blank line only valid in batch mode)" ); } -mktree_line (sb.buf, sb.len, line_termination, allow_missing); +mktree_line (sb.buf, sb.len, lf_lines, allow_missing); }if (is_batch_mode && got_eof && used <1 ) {/*
Signature · 1 line
2.7.0-242-gdd583c7
Re: [PREVIEW v3 4/9] mktree: there are only two line terminators
Quoted 4 lines · Junio C Hamano
builtin/mktree.c +0
-0@@ -141,23 +141,25 @@ int cmd_mktree(int ac, const char **av, const char *prefix) {struct strbuf sb =STRBUF_INIT ;unsigned char sha1[20 ];
builtin/mktree.c int cmd_mktree(int ac, const char **av, const char *prefix) · line 151
Quoted 10 lines
builtin/mktree.c +3
-2-int line_termination ='\n' ; +int lf_lines =1 ;int allow_missing =0 ;int is_batch_mode =0 ;int got_eof =0 ; +strbuf_getline_fn getline_fn;const struct option option[] = { -OPT_SET_INT ('z' ,NULL , &line_termination,N_ ("input is NUL terminated" ),'\0' ), +OPT_SET_INT ('z' ,NULL , &lf_lines,N_ ("input is NUL terminated" ),'\0' ),
Using '\0' isn't wrong here, but should it now just be "0", since it's no longer meant to be a "char"?
Also, I notice that other patches in the series flip the logic (instead of "lf_lines", we get its inverse, "nul_term_line"). That lets us use the more obvious "OPT_BOOL" here.
-Peff
PATCH v3 5/9 check-attr: there are only two line terminators
In reply to Junio C HamanoThe program by default works on LF terminated lines, with an option to use NUL terminated records. Instead of using line_termination that happens to take LF or NUL, use the value of nul_term_line and switch between strbuf_getline_{lf,nul} based on it.
--- builtin/check-attr.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
builtin/check-attr.c +4 -3
@@ -73,12 +73,13 @@ static void check_attr_stdin_paths(const char *prefix, int cnt, struct git_attr_check *check) {struct strbuf buf, nbuf; -int line_termination = nul_term_line ?0 :'\n' ; +strbuf_getline_fn getline_fn; + getline_fn = nul_term_line ? strbuf_getline_nul : strbuf_getline_lf;strbuf_init (&buf,0 );strbuf_init (&nbuf,0 ); -while (strbuf_getline (&buf, stdin, line_termination) !=EOF ) { -if (line_termination && buf.buf[0 ] =='"' ) { +while (getline_fn (&buf, stdin) !=EOF ) { +if (!nul_term_line && buf.buf[0 ] =='"' ) {strbuf_reset (&nbuf);if (unquote_c_style (&nbuf, buf.buf,NULL ))die ("line is badly quoted" );
Signature · 1 line
2.7.0-242-gdd583c7
PATCH v3 6/9 check-ignore: there are only two line terminators
In reply to Junio C HamanoThe program by default works on LF terminated lines, with an option to use NUL terminated records. Instead of using line_termination that happens to take LF or NUL, use the value of nul_term_line and switch between strbuf_getline_{lf,nul} based on it.
--- builtin/check-ignore.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
builtin/check-ignore.c +4 -3
@@ -117,13 +117,14 @@ static int check_ignore_stdin_paths(struct dir_struct *dir, const char *prefix) {struct strbuf buf, nbuf;char *pathspec[2 ] = {NULL ,NULL }; -int line_termination = nul_term_line ?0 :'\n' ; +strbuf_getline_fn getline_fn;int num_ignored =0 ; + getline_fn = nul_term_line ? strbuf_getline_nul : strbuf_getline_lf;strbuf_init (&buf,0 );strbuf_init (&nbuf,0 ); -while (strbuf_getline (&buf, stdin, line_termination) !=EOF ) { -if (line_termination && buf.buf[0 ] =='"' ) { +while (getline_fn (&buf, stdin) !=EOF ) { +if (!nul_term_line && buf.buf[0 ] =='"' ) {strbuf_reset (&nbuf);if (unquote_c_style (&nbuf, buf.buf,NULL ))die ("line is badly quoted" );
Signature · 1 line
2.7.0-242-gdd583c7
PATCH v3 7/9 update-index: there are only two line terminators
In reply to Junio C HamanoThe program by default works on LF terminated lines, with an option to use NUL terminated records, when reading the index-info and cacheinfo. Instead of using line_termination that happens to take LF or NUL, use the value of nul_term_line and switch between strbuf_getline_{lf,nul} based on it.
--- builtin/update-index.c | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-)
builtin/update-index.c +16 -11
@@ -468,12 +468,14 @@ static void update_one(const char *path)report ("add '%s'" , path); } -static void read_index_info (int line_termination) +static void read_index_info (int nul_term_line) {struct strbuf buf =STRBUF_INIT ;struct strbuf uq =STRBUF_INIT ; +strbuf_getline_fn getline_fn; -while (strbuf_getline (&buf, stdin, line_termination) !=EOF ) { + getline_fn = nul_term_line ? strbuf_getline_nul : strbuf_getline_lf; +while (getline_fn (&buf, stdin) !=EOF ) {char *ptr, *tab;char *path_name;unsigned char sha1[20 ];
@@ -522,7 +524,7 @@ static void read_index_info(int line_termination) goto bad_line; path_name = ptr; -if (line_termination && path_name[0 ] =='"' ) { +if (!nul_term_line && path_name[0 ] =='"' ) {strbuf_reset (&uq);if (unquote_c_style (&uq, path_name,NULL )) {die ("git update-index: bad quoting of path name" );
@@ -844,12 +846,12 @@ static int cacheinfo_callback(struct parse_opt_ctx_t *ctx,static int stdin_cacheinfo_callback (struct parse_opt_ctx_t *ctx,const struct option *opt,int unset) { -int *line_termination = opt->value; +int *nul_term_line = opt->value;if (ctx->argc !=1 )return error ("option '%s' must be the last argument" , opt->long_name); allow_add = allow_replace = allow_remove =1 ; -read_index_info (*line_termination); +read_index_info (*nul_term_line);return 0 ; }
@@ -901,7 +903,7 @@ static int reupdate_callback(struct parse_opt_ctx_t *ctx,int cmd_update_index (int argc,const char **argv,const char *prefix) { -int newfd, entries, has_errors =0 , line_termination ='\n' ; +int newfd, entries, has_errors =0 , nul_term_line =0 ;int untracked_cache =-1 ;int read_from_stdin =0 ;int prefix_length = prefix ?strlen (prefix) :0 ;
@@ -912,6 +914,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)int split_index =-1 ;struct lock_file *lock_file;struct parse_opt_ctx_t ctx; +strbuf_getline_fn getline_fn;int parseopt_state =PARSE_OPT_UNKNOWN ;struct option options[] = { OPT_BIT('q', NULL, &refresh_args.flags,
@@ -963,13 +966,13 @@ int cmd_update_index(int argc, const char **argv, const char *prefix) N_("add to index only; do not add content to object database"), 1),OPT_SET_INT (0 ,"force-remove" , &force_remove,N_ ("remove named paths even if present in worktree" ),1 ), -OPT_SET_INT ('z' ,NULL , &line_termination, -N_ ("with --stdin: input lines are terminated by null bytes" ),'\0' ), +OPT_SET_INT ('z' ,NULL , &nul_term_line, +N_ ("with --stdin: input lines are terminated by null bytes" ),1 ), {OPTION_LOWLEVEL_CALLBACK, 0, "stdin", &read_from_stdin, NULL,N_ ("read list of paths to be updated from standard input" ),PARSE_OPT_NONEG |PARSE_OPT_NOARG , (parse_opt_cb *) stdin_callback}, - {OPTION_LOWLEVEL_CALLBACK, 0, "index-info", &line_termination, NULL, + {OPTION_LOWLEVEL_CALLBACK, 0, "index-info", &nul_term_line, NULL,N_ ("add entries from standard input to the index" ),PARSE_OPT_NONEG |PARSE_OPT_NOARG , (parse_opt_cb *) stdin_cacheinfo_callback},
@@ -1057,6 +1060,8 @@ int cmd_update_index(int argc, const char **argv, const char *prefix) } } argc =parse_options_end (&ctx); + + getline_fn = nul_term_line ? strbuf_getline_nul : strbuf_getline_lf;if (preferred_index_format) {if (preferred_index_format <INDEX_FORMAT_LB ||INDEX_FORMAT_UB < preferred_index_format)
@@ -1073,9 +1078,9 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)struct strbuf buf =STRBUF_INIT , nbuf =STRBUF_INIT ;setup_work_tree (); -while (strbuf_getline (&buf, stdin, line_termination) !=EOF ) { +while (getline_fn (&buf, stdin) !=EOF ) {char *p; -if (line_termination && buf.buf[0 ] =='"' ) { +if (!nul_term_line && buf.buf[0 ] =='"' ) {strbuf_reset (&nbuf);if (unquote_c_style (&nbuf, buf.buf,NULL ))die ("line is badly quoted" );
Signature · 1 line
2.7.0-242-gdd583c7
Re: [PREVIEW v3 7/9] update-index: there are only two line terminators
Quoted 14 lines · Junio C Hamano
builtin/update-index.c +4
-2-static void read_index_info (int line_termination) +static void read_index_info (int nul_term_line) {struct strbuf buf =STRBUF_INIT ;struct strbuf uq =STRBUF_INIT ; +strbuf_getline_fn getline_fn; -while (strbuf_getline (&buf, stdin, line_termination) !=EOF ) { + getline_fn = nul_term_line ? strbuf_getline_nul : strbuf_getline_lf; +while (getline_fn (&buf, stdin) !=EOF ) {char *ptr, *tab;char *path_name;unsigned char sha1[20 ];
builtin/update-index.c static void read_index_info(int line_termination) · line 527
Quoted 5 lines
builtin/update-index.c +1
-1@@ -522,7 +524,7 @@ static void read_index_info(int line_termination) goto bad_line; path_name = ptr; -if (line_termination && path_name[0 ] =='"' ) { +if (!nul_term_line && path_name[0 ] =='"' ) {
Yikes. The original used "line_termination" both as a character and as a boolean. I think the postimage is much more obvious here.
builtin/update-index.c int cmd_update_index(int argc, const char **argv, const char *prefix) · line 970
Quoted 4 lines
builtin/update-index.c +2
-2-OPT_SET_INT ('z' ,NULL , &line_termination, - N_("with --stdin: input lines are terminated by null bytes"), '\0'), +OPT_SET_INT ('z' ,NULL , &nul_term_line, + N_("with --stdin: input lines are terminated by null bytes"), 1),
Should this just become OPT_BOOL now?
-Peff
PATCH v3 8/9 checkout-index: there are only two line terminators
In reply to Junio C HamanoThe program by default works on LF terminated lines, with an option to use NUL terminated records. Instead of using line_termination that happens to take LF or NUL to call strbuf_getline(), switch between strbuf_getline_{lf,nul} based on the value of '-z' option.
Note that this still leaves the option open to use NUL-terminated input mixed with LF-terminated output (and vice versa), and even HT-terminated output is still left as a possibility, because this series is only interested in tightening the overly broad interface on the input side.
--- builtin/checkout-index.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)
builtin/checkout-index.c +7 -3
@@ -12,6 +12,7 @@#define CHECKOUT_ALL 4static int line_termination ='\n' ; +static strbuf_getline_fn getline_fn = strbuf_getline_lf;static int checkout_stage;/* default to checkout stage0 */ static int to_tempfile;static char topath[4 ][TEMPORARY_FILENAME_LENGTH +1 ];
@@ -144,10 +145,13 @@ static int option_parse_u(const struct option *opt,static int option_parse_z (const struct option *opt,const char *arg,int unset) { -if (unset) +if (unset) { line_termination ='\n' ; -else + getline_fn = strbuf_getline_lf; + }else { line_termination =0 ; + getline_fn = strbuf_getline_nul; + }return 0 ; }
@@ -258,7 +262,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)if (all)die ("git checkout-index: don't mix '--all' and '--stdin'" ); -while (strbuf_getline (&buf, stdin, line_termination) !=EOF ) { +while (getline_fn (&buf, stdin) !=EOF ) {char *p;if (line_termination && buf.buf[0 ] =='"' ) {strbuf_reset (&nbuf);
Signature · 1 line
2.7.0-242-gdd583c7
Re: [PREVIEW v3 8/9] checkout-index: there are only two line terminators
Quoted 10 lines · Junio C Hamano
The program by default works on LF terminated lines, with an option to use NUL terminated records. Instead of using
line_terminationthat happens to take LF or NUL to callstrbuf_getline(), switch between strbuf_getline_{lf,nul} based on the value of '-z' option.Note that this still leaves the option open to use NUL-terminated input mixed with LF-terminated output (and vice versa), and even HT-terminated output is still left as a possibility, because this series is only interested in tightening the overly broad interface on the input side.
I see that we switch the line termination on the fly in option_parse_z. But I'm having trouble seeing how we could actually have mixed inputs. We don't actually look at the line-terminator until after all of the options are parsed.
IOW, could this OPT_CALLBACK for 'z' be simplified to a simple OPT_BOOL, or am I missing some case?
Quoted 3 lines
#define
CHECKOUT_ALL4
static intline_termination= '\n';
+staticstrbuf_getline_fngetline_fn=strbuf_getline_lf;
This "line_termination" can become a boolean "1" now, right?
builtin/checkout-index.c static int option_parse_u(const struct option *opt, · line 154
Quoted 13 lines
builtin/checkout-index.c +5
-2@@ -144,10 +145,13 @@ static int option_parse_u(const struct option *opt,static int option_parse_z (const struct option *opt,const char *arg,int unset) { -if (unset) +if (unset) { line_termination ='\n' ; -else + getline_fn = strbuf_getline_lf; + }else { line_termination =0 ; + getline_fn = strbuf_getline_nul; + }
Ditto here (though as before, I think I prefer it as the inverse bool:
did somebody turn on "-z").
I'm also not sure how "unset" would trigger here. If we have a long option, we can use "--no-foo". But there isn't a long option for "-z". Is there a way to negate short options?
-Peff
Re: [PREVIEW v3 8/9] checkout-index: there are only two line terminators
Quoted 4 lines · Jeff King
I see that we switch the line termination on the fly in
option_parse_z. But I'm having trouble seeing how we could actually have mixed inputs. We don't actually look at the line-terminator until after all of the options are parsed.
I do not think we are aiming for mixed inputs. What I meant by "mixed" in my description was a possible future option to allow input and output using different line termination (e.g. via "-Z" you would control output line termination, while "-z" only affects the input line termination).
After this step, there is still one call that is more natural to have line_termination that is either '\0' or '\n' and that is on the output side, which calls write_name_quoted_relative() to report the names of the temporary files that received the file contents to the standard output.
Of course there is nothing wrong to do
write_name_quoted_relative(name, prefix, stdout,
nul_term_lines ? '\0' : '\n');
though.
And I tend to think that it might even be a good idea to do so. If somebody in the future really wants to introduce '-Z', then they can add a separate "output_line_termination" variable back (and it would likely be set to '\0' or '\n' via "-z" when "-Z" is not given), but until then, I agree that a simple bool for "-z" would be better.
Quoted 3 lines
I'm also not sure how "unset" would trigger here. If we have a long option, we can use "--no-foo". But there isn't a long option for "-z". Is there a way to negate short options?
I do not think there is. It merely future-proofs against those who try to add "--nul" as a longer synonym. They would complain after they add "--nul" in builtin_checkout_index_options[] as a synonym if they see "--no-nul" does not negate the effect of an earlier "-z".
Re: [PREVIEW v3 8/9] checkout-index: there are only two line terminators
Quoted 7 lines · Junio C Hamano
Quoted 4 lines · Jeff King
I see that we switch the line termination on the fly in
option_parse_z. But I'm having trouble seeing how we could actually have mixed inputs. We don't actually look at the line-terminator until after all of the options are parsed.
Quoted 5 lines
I do not think we are aiming for mixed inputs. What I meant by "mixed" in my description was a possible future option to allow input and output using different line termination (e.g. via "-Z" you would control output line termination, while "-z" only affects the input line termination).
Ah, I see.
Quoted 11 lines
After this step, there is still one call that is more natural to have
line_terminationthat is either '\0' or '\n' and that is on the output side, which callswrite_name_quoted_relative()to report the names of the temporary files that received the file contents to the standard output.Of course there is nothing wrong to do
write_name_quoted_relative(name, prefix, stdout,
nul_term_lines? '\0' : '\n');
Quoted 7 lines
though.
And I tend to think that it might even be a good idea to do so. If somebody in the future really wants to introduce '-Z', then they can add a separate "
output_line_termination" variable back (and it would likely be set to '\0' or '\n' via "-z" when "-Z" is not given), but until then, I agree that a simple bool for "-z" would be better.
Yeah, I agree with all of that.
Quoted 8 lines
Quoted 3 lines
I'm also not sure how "unset" would trigger here. If we have a long option, we can use "--no-foo". But there isn't a long option for "-z". Is there a way to negate short options?
I do not think there is. It merely future-proofs against those who try to add "--nul" as a longer synonym. They would complain after they add "--nul" in builtin_checkout_index_options[] as a synonym if they see "--no-nul" does not negate the effect of an earlier "-z".
Makes sense. Better still if we can turn it into OPT_BOOL, though, and then it would Just Work. :)
-Peff
PATCH v3 9/9 strbuf: give strbuf_getline() to the "most text friendly" variant
In reply to Junio C HamanoNow there is no direct caller to strbuf_getline(), we can demote it to file-scope static private to strbuf.c implementation and rename it to strbuf_getdelim(). Rename strbuf_getline_crlf(), which is designed to be the most "text friendly" variant, and allow it to take over this simplest name, strbuf_getline(), so we can add more uses of it without having to type _crlf over and over again in the coming steps.
--- builtin/am.c | 8 ++++---- strbuf.c | 8 ++++---- strbuf.h | 25 ++++++++++++++----------- transport-helper.c | 3 ++- 4 files changed, 24 insertions(+), 20 deletions(-)
builtin/am.c +4 -4
@@ -612,7 +612,7 @@ static int is_mail(FILE *fp)if (regcomp (®ex, header_regex,REG_NOSUB |REG_EXTENDED ))die ("invalid pattern: %s" , header_regex); -while (!strbuf_getline_crlf (&sb, fp)) { +while (!strbuf_getline (&sb, fp)) {if (!sb.len)break ;/* End of header */
@@ -659,7 +659,7 @@ static int detect_patch_format(const char **paths) fp =xfopen (*paths,"r" ); -while (!strbuf_getline_crlf (&l1, fp)) { +while (!strbuf_getline (&l1, fp)) {if (l1.len)break ; }
@@ -680,9 +680,9 @@ static int detect_patch_format(const char **paths) }strbuf_reset (&l2); -strbuf_getline_crlf (&l2, fp); +strbuf_getline (&l2, fp);strbuf_reset (&l3); -strbuf_getline_crlf (&l3, fp); +strbuf_getline (&l3, fp);/* * If the second line is empty and the third is a From, Author or Date
strbuf.c +4 -4
@@ -501,7 +501,7 @@ int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term) }#endif -int strbuf_getline (struct strbuf *sb,FILE *fp,int term) +static int strbuf_getdelim (struct strbuf *sb,FILE *fp,int term) {if (strbuf_getwholeline (sb, fp, term))return EOF ;
@@ -510,7 +510,7 @@ int strbuf_getline(struct strbuf *sb, FILE *fp, int term)return 0 ; } -int strbuf_getline_crlf (struct strbuf *sb,FILE *fp) +int strbuf_getline (struct strbuf *sb,FILE *fp) {if (strbuf_getwholeline (sb, fp,'\n' ))return EOF ;
@@ -524,12 +524,12 @@ int strbuf_getline_crlf(struct strbuf *sb, FILE *fp)int strbuf_getline_lf (struct strbuf *sb,FILE *fp) { -return strbuf_getline (sb, fp,'\n' ); +return strbuf_getdelim (sb, fp,'\n' ); }int strbuf_getline_nul (struct strbuf *sb,FILE *fp) { -return strbuf_getline (sb, fp,'\0' ); +return strbuf_getdelim (sb, fp,'\0' ); }int strbuf_getwholeline_fd (struct strbuf *sb,int fd,int term)
strbuf.h +14 -11
@@ -354,8 +354,8 @@ extern void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm* * NOTE: The buffer is rewound if the read fails. If -1 is returned, * `errno` must be consulted, like you would do for `read(3)`. -* `strbuf_read()`, `strbuf_read_file()` and `strbuf_getline()` has the -* same behaviour as well. +* `strbuf_read()`, `strbuf_read_file()` and `strbuf_getline_*()` +* family of functions have the same behaviour as well. */ extern size_t strbuf_fread (struct strbuf *,size_t ,FILE *);
@@ -380,26 +380,29 @@ extern int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint);/** * Read a line from a FILE *, overwriting the existing contents -* of the strbuf. The second argument specifies the line -* terminator character, typically `'\n'`. +* of the strbuf. There are three public functions with this +* function signature, with different line termination convention. * Reading stops after the terminator or at EOF. The terminator * is removed from the buffer before returning. Returns 0 unless * there was nothing left before EOF, in which case it returns `EOF`. */ -extern int strbuf_getline (struct strbuf *,FILE *,int ); -typedef int (*strbuf_getline_fn )(struct strbuf *,FILE *); +/* Uses LF as the line terminator */ extern int strbuf_getline_lf (struct strbuf *sb,FILE *fp); + +/* Uses NUL as the line terminator */ extern int strbuf_getline_nul (struct strbuf *sb,FILE *fp); -/* -* Similar to strbuf_getline(), but uses '\n' as the terminator, -* and additionally treats a '\r' that comes immediately before '\n' -* as part of the terminator. +/** +* Similar to strbuf_getline_lf(), but additionally treats +* a '\r' that comes immediately before '\n' as part of the +* terminator. This is the most friendly version to be used +* to read "text" files that can come from platforms whose +* native text format is CRLF terminated. */ -extern int strbuf_getline_crlf (struct strbuf *,FILE *); +extern int strbuf_getline (struct strbuf *,FILE *);/**
transport-helper.c +2 -1
@@ -137,7 +137,8 @@ static struct child_process *get_helper(struct transport *transport) data->no_disconnect_req =0 ;/* -* Open the output as FILE* so strbuf_getline() can be used. +* Open the output as FILE* so strbuf_getline_*() family of +* functions can be used. * Do this with duped fd because fclose() will close the fd, * and stuff like taking over will require the fd to remain. */
Signature · 1 line
2.7.0-242-gdd583c7
PATCH v3 9/9 strbuf: retire strbuf_getline() for now
In reply to Junio C HamanoNow there is no direct caller to strbuf_getline(), so demote it to file-scope static private to strbuf.c implementation and rename it to strbuf_getdelim().
Eventually, we may want to make this short and clean name a synonym to strbuf_getline_crlf(), but not now.
--- strbuf.c | 6 +++--- strbuf.h | 21 +++++++++++---------- transport-helper.c | 3 ++- 3 files changed, 16 insertions(+), 14 deletions(-)
strbuf.c +3 -3
@@ -501,7 +501,7 @@ int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term) }#endif -int strbuf_getline (struct strbuf *sb,FILE *fp,int term) +static int strbuf_getdelim (struct strbuf *sb,FILE *fp,int term) {if (strbuf_getwholeline (sb, fp, term))return EOF ;
@@ -524,12 +524,12 @@ int strbuf_getline_crlf(struct strbuf *sb, FILE *fp)int strbuf_getline_lf (struct strbuf *sb,FILE *fp) { -return strbuf_getline (sb, fp,'\n' ); +return strbuf_getdelim (sb, fp,'\n' ); }int strbuf_getline_nul (struct strbuf *sb,FILE *fp) { -return strbuf_getline (sb, fp,'\0' ); +return strbuf_getdelim (sb, fp,'\0' ); }int strbuf_getwholeline_fd (struct strbuf *sb,int fd,int term)
strbuf.h +11 -10
@@ -354,8 +354,8 @@ extern void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm* * NOTE: The buffer is rewound if the read fails. If -1 is returned, * `errno` must be consulted, like you would do for `read(3)`. -* `strbuf_read()`, `strbuf_read_file()` and `strbuf_getline()` has the -* same behaviour as well. +* `strbuf_read()`, `strbuf_read_file()` and `strbuf_getline_*()` +* family of functions have the same behaviour as well. */ extern size_t strbuf_fread (struct strbuf *,size_t ,FILE *);
@@ -380,24 +380,25 @@ extern int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint);/** * Read a line from a FILE *, overwriting the existing contents -* of the strbuf. The second argument specifies the line -* terminator character, typically `'\n'`. +* of the strbuf. There are three public functions with this +* function signature, with different line termination convention. * Reading stops after the terminator or at EOF. The terminator * is removed from the buffer before returning. Returns 0 unless * there was nothing left before EOF, in which case it returns `EOF`. */ -extern int strbuf_getline (struct strbuf *,FILE *,int ); -typedef int (*strbuf_getline_fn )(struct strbuf *,FILE *); +/* Uses LF as the line terminator */ extern int strbuf_getline_lf (struct strbuf *sb,FILE *fp); + +/* Uses NUL as the line terminator */ extern int strbuf_getline_nul (struct strbuf *sb,FILE *fp); -/* -* Similar to strbuf_getline(), but uses '\n' as the terminator, -* and additionally treats a '\r' that comes immediately before '\n' -* as part of the terminator. +/** +* Similar to strbuf_getline_lf(), but additionally treats +* a '\r' that comes immediately before '\n' as part of the +* terminator. */ extern int strbuf_getline_crlf (struct strbuf *,FILE *);
transport-helper.c +2 -1
@@ -137,7 +137,8 @@ static struct child_process *get_helper(struct transport *transport) data->no_disconnect_req =0 ;/* -* Open the output as FILE* so strbuf_getline() can be used. +* Open the output as FILE* so strbuf_getline_*() family of +* functions can be used. * Do this with duped fd because fclose() will close the fd, * and stuff like taking over will require the fd to remain. */
Signature · 1 line
2.7.0-242-gdd583c7
Re: [PREVIEW v3 9/9] strbuf: retire strbuf_getline() for now
Quoted 8 lines · Junio C Hamano
Now there is no direct caller to
strbuf_getline(), so demote it to file-scope static private tostrbuf.cimplementation and rename it tostrbuf_getdelim().Eventually, we may want to make this short and clean name a synonym to
strbuf_getline_crlf(), but not now.Signed-off-by Junio C Hamano <[email protected]>
This is not part of the series but sent out by mistake.
strbuf: give strbuf_getline() to the "most text friendly" variant
Message-ID: <[email protected]>
Xref: news.gmane.org gmane.comp.version-control.git:284008
is the correct 9/9.
v4 cover Peace with CRLF
In reply to Junio C HamanoWe have too many topics titled "War on something"; let's try to make peace for a change.
This is a reroll of
http://thread.gmane.org/gmane.comp.version-control.git/282614
which is a continuation to $gmane/275735, which is filed in the archive under another mailing list:
http://thread.gmane.org/gmane.comp.version-control.msysgit/21773
but with a larger clean-up whose early part was sent earlier, marked with the "PREVIEW v3" label.
http://thread.gmane.org/gmane.comp.version-control.git/284001
We read "text" files from filesystem (or from the standard input) in various places in the code. Some of them are written by us, but others are often prepared by an editor driven by human user. Even though we write (and expect) lines in these text files to be terminated with LF (not CRLF), the end-user's editor can be told to use CRLF termination, and on some platforms that may be the default.
Typically, we use strbuf_getline() to read these "text" files, which reads a single line up to the first LF into a buffer, discard that LF at the end, and returns (an incomplete last line gets returned as-is). Lines from a file edited on Dossy systems hence may have a CR at the end that the end-user did not intend to be a part of it.
Because many codepaths (e.g. commit log message) first pass the contents of such a file through stripspace(), and as a side effect of discarding the whitespace at the end of each line, lines terminated with CRLF are normalized to LF terminated lines (but we do not lose a lone CR in the middle of a non-blank string), when we process what we read from these files, this is not a problem for them, but not all of the codepaths are safe.
In these places that expect to read "text", we should be able to update the logic to read a line up to the first LF, discard that LF, together with a CR if one appears immediately before that LF, without breaking anything. That includes cases where the program might want to treat a CR in the middle of a line as proper payload the user intended to give us. Such a variant of strbuf_getline() already exists as strbuf_getline_crlf() and used in the "git am" that was reimplemented in C recently.
The strbuf_getline() function takes a "line-termination" as its third argument. Its interface is overly broad in that it allows an arbitrary byte to be used to mark the end of a line, but in practice, no codepaths pass any byte other than LF or NUL. In theory, the callers that use LF as the line-termination are expecting "text" input, so we could blindly replace them with strbuf_getline_crlf() and not many things should break. To be conservative, however, this series converts only chosen ones after auditing that such a conversion is safe.
To make the tree-wide change manageable, the early part of this series first changes all callsites of strbuf_getline() to call one of the two new thin wrapper functions, strbuf_getline_lf() and strbuf_getline_nul(), depending on the value of "line-termination" used by the caller. This is done with Patches 1-9, which is essentially the same as "PREVIEW v3" series posted earlier, with small changes:
- The update to mktree used "lf_lines" in the preview, but this version flips it to the same "nul_term_line" to match other two, even though I think the latter is a terrible variable name with bad taste.
- The update to checkout-index kept "line_termination" in the preview, but this version no longer uses it and instead uses nul_term_line.
- The definition of nul_term_line for the option parser uses OPT_BOOL(), as the possible values in it are true or false, not '\0' or '\n'.
- The topic was rebased on v2.7.0; the bulk of builtin/mailinfo.c moved to mailinfo.c between the old base of the topic and v2.7.0, so the mechanical renaming done in Patch 3 was redone.
After Patch 9, all the original callers of strbuf_getline() that used LF line ending call strbuf_getline_lf(). No caller calls strbuf_getline() at that point, so strbuf_getline_crlf() takes over the short and nice name strbuf_getline().
The callers of strbuf_getline_lf() are inspected by the remainder of the series and updated to call strbuf_getline() instead when it makes sense to do so. They all had to be updated relative to the previous round (v2), but the callsites that are updated haven't changed.
This series converts only the callers of strbuf_getline() that would misbehave when fed a file with CRLF-terminated lines and use the data with an unwanted CR appended at the end. With the update the code should work as intended with such a file, without breaking the expected behaviour when working on a file with LF-terminated lines.
Notes on the remaining strbuf_getline_lf() callers:
* Those that expect to only read from our own output do not have to accommodate CRLF-terminated lines, but they can be updated to do so safely if we do not rely on the ability to express a payload that has CR at the end. For example, the insn sheet "rebase -i" uses typically ends each line with a commit log summary that we ourselves do not read or use, so even if the end-user on a platform with LF lines deliberately does insert CR at the end of the line and strbuf_getline() removed that CR from the payload, no unexpected behaviour should happen.
It may be a good GSoC microproject to convert them to use strbuf_getline().
* Those that call strbuf_trim() immediately on the result before doing anything, or otherwise have logic that tolerates whitespaces at the end of the line, can continue using strbuf_getline_lf() and will not misbehave on CRLF-terminated lines.
This series does not touch them; converting them to use strbuf_getline() would a good way to document them as dealing with "text". While doing so, they can also lose their custom logic that happens to make CRLF-terminated lines work.
It is a good GSoC microproject to convert them to use strbuf_getline().
* Those that want to only ever see LF-terminated lines (in other words, "ABC\r\n" must be treated as a line with 4 bytes payload on it, A B C and CR), would be broken if we replace it with strbuf_getline(). This series does not touch them, and no follow-up series should, either.
Junio C Hamano (21):
strbuf: miniscule style fix
strbuf: make strbuf_getline_crlf() global
strbuf: introduce strbuf_getline_{lf,nul}()
mktree: there are only two possible line terminations
check-attr: there are only two possible line terminations
check-ignore: there are only two possible line terminations
update-index: there are only two possible line terminations
checkout-index: there are only two possible line terminations
strbuf: give strbuf_getline() to the "most text friendly" variant
hash-object: read --stdin-paths with strbuf_getline()
revision: read --stdin with strbuf_getline()
rev-parse: read parseopt spec with strbuf_getline() ident.c: read /etc/mailname with strbuf_getline() remote.c: read $GIT_DIR/remotes/* with strbuf_getline() clone/sha1_file: read info/alternates with strbuf_getline()
transport-helper: read helper response with strbuf_getline()
cat-file: read batch stream with strbuf_getline()
column: read lines with strbuf_getline()
send-pack: read list of refs with strbuf_getline()
grep: read -f file with strbuf_getline()
test-sha1-array: read command stream with strbuf_getline()
bisect.c | 8 ++++---- builtin/am.c | 37 +++++++++++-------------------------- builtin/cat-file.c | 2 +- builtin/check-attr.c | 7 ++++--- builtin/check-ignore.c | 7 ++++--- builtin/check-mailmap.c | 2 +- builtin/checkout-index.c | 16 ++++++++-------- builtin/clean.c | 6 +++--- builtin/clone.c | 2 +- builtin/column.c | 2 +- builtin/commit.c | 2 +- builtin/fetch-pack.c | 2 +- builtin/grep.c | 2 +- builtin/hash-object.c | 2 +- builtin/mktree.c | 14 ++++++++------ builtin/notes.c | 2 +- builtin/pull.c | 2 +- builtin/repack.c | 2 +- builtin/rev-parse.c | 4 ++-- builtin/send-pack.c | 2 +- builtin/update-index.c | 27 ++++++++++++++++----------- compat/terminal.c | 2 +- credential-cache--daemon.c | 4 ++-- credential-store.c | 2 +- credential.c | 2 +- daemon.c | 2 +- fast-import.c | 4 ++-- ident.c | 2 +- mailinfo.c | 8 ++++---- remote-curl.c | 6 +++--- remote-testsvn.c | 4 ++-- remote.c | 4 ++-- revision.c | 9 ++------- sequencer.c | 2 +- sha1_file.c | 2 +- shell.c | 2 +- strbuf.c | 28 +++++++++++++++++++++++++--- strbuf.h | 27 ++++++++++++++++++++++----- test-sha1-array.c | 2 +- transport-helper.c | 5 +++-- walker.c | 2 +- wt-status.c | 4 ++-- 42 files changed, 151 insertions(+), 122 deletions(-)
Signature · 1 line
2.7.0-250-ge1b5ba3
PATCH v4 1/21 strbuf: miniscule style fix
We write one SP on each side of an operator, even inside an [] pair that computes the array index.
--- strbuf.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
strbuf.c +2 -2
@@ -505,8 +505,8 @@ int strbuf_getline(struct strbuf *sb, FILE *fp, int term) {if (strbuf_getwholeline (sb, fp, term))return EOF ; -if (sb->buf[sb->len-1 ] == term) -strbuf_setlen (sb, sb->len-1 ); +if (sb->buf[sb->len -1 ] == term) +strbuf_setlen (sb, sb->len -1 );return 0 ; }
Signature · 1 line
2.7.0-250-ge1b5ba3
PATCH v4 2/21 strbuf: make strbuf_getline_crlf() global
In reply to Junio C HamanoOften we read "text" files that are supplied by the end user (e.g. commit log message that was edited with $GIT_EDITOR upon 'git commit -e'), and in some environments lines in a text file are terminated with CRLF. Existing strbuf_getline() knows to read a single line and then strip the terminating byte from the result, but it is handy to have a version that is more tailored for a "text" input that takes both '\n' and '\r\n' as line terminator (aka <newline> in POSIX lingo) and returns the body of the line after stripping <newline>.
Recently reimplemented "git am" uses such a function implemented privately; move it to strbuf.[ch] and make it available for others.
Note that we do not blindly replace calls to strbuf_getline() that uses LF as the line terminator with calls to strbuf_getline_crlf() and this is very much deliberate. Some callers may want to treat an incoming line that ends with CR (and terminated with LF) to have a payload that includes the final CR, and such a blind replacement will result in misconversion when done without code audit.
--- builtin/am.c | 15 --------------- strbuf.c | 12 ++++++++++++ strbuf.h | 7 +++++++ 3 files changed, 19 insertions(+), 15 deletions(-)
builtin/am.c +0 -15
@@ -46,21 +46,6 @@ static int is_empty_file(const char *filename) }/** -* Like strbuf_getline(), but treats both '\n' and "\r\n" as line terminators. -*/ -static int strbuf_getline_crlf (struct strbuf *sb,FILE *fp) -{ -if (strbuf_getwholeline (sb, fp,'\n' )) -return EOF ; -if (sb->buf[sb->len -1 ] =='\n' ) { -strbuf_setlen (sb, sb->len -1 ); -if (sb->len >0 && sb->buf[sb->len -1 ] =='\r' ) -strbuf_setlen (sb, sb->len -1 ); - } -return 0 ; -} - -/** * Returns the length of the first line of msg. */ static int linelen (const char *msg)
strbuf.c +12 -0
@@ -510,6 +510,18 @@ int strbuf_getline(struct strbuf *sb, FILE *fp, int term)return 0 ; } +int strbuf_getline_crlf (struct strbuf *sb,FILE *fp) +{ +if (strbuf_getwholeline (sb, fp,'\n' )) +return EOF ; +if (sb->buf[sb->len -1 ] =='\n' ) { +strbuf_setlen (sb, sb->len -1 ); +if (sb->len && sb->buf[sb->len -1 ] =='\r' ) +strbuf_setlen (sb, sb->len -1 ); + } +return 0 ; +} +int strbuf_getwholeline_fd (struct strbuf *sb,int fd,int term) {strbuf_reset (sb);
strbuf.h +7 -0
@@ -388,6 +388,13 @@ extern int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint);*/ extern int strbuf_getline (struct strbuf *,FILE *,int ); +/* +* Similar to strbuf_getline(), but uses '\n' as the terminator, +* and additionally treats a '\r' that comes immediately before '\n' +* as part of the terminator. +*/ +extern int strbuf_getline_crlf (struct strbuf *,FILE *); +/** * Like `strbuf_getline`, but keeps the trailing terminator (if * any) in the buffer.
Signature · 1 line
2.7.0-250-ge1b5ba3
PATCH v4 3/21 strbuf: introduce strbuf_getline_{lf,nul}()
In reply to Junio C HamanoThe strbuf_getline() interface allows a byte other than LF or NUL as the line terminator, but this is only 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. No useful caller that uses other value has emerged.
By now, it is clear that the interface is overly broad without a good reason. Many codepaths have hardcoded preference to read either LF terminated or NUL terminated records from their input, and then call strbuf_getline() with LF or NUL as the third parameter.
This step introduces two thin wrappers around strbuf_getline(), namely, strbuf_getline_lf() and strbuf_getline_nul(), and mechanically rewrites these call sites to call either one of them. The changes contained in this patch are:
* introduction of these two functions in strbuf.[ch]
* mechanical conversion of all callers to strbuf_getline() with either '\n' or '\0' as the third parameter to instead call the respective thin wrapper.
After this step, output from "git grep 'strbuf_getline('" would become a lot smaller. An interim goal of this series is to make this an empty set, so that we can have strbuf_getline_crlf() take over the shorter name strbuf_getline().
--- bisect.c | 8 ++++---- builtin/am.c | 14 +++++++------- builtin/cat-file.c | 2 +- builtin/check-mailmap.c | 2 +- builtin/clean.c | 6 +++--- builtin/clone.c | 2 +- builtin/column.c | 2 +- builtin/commit.c | 2 +- builtin/fetch-pack.c | 2 +- builtin/grep.c | 2 +- builtin/hash-object.c | 2 +- builtin/notes.c | 2 +- builtin/pull.c | 2 +- builtin/repack.c | 2 +- builtin/rev-parse.c | 4 ++-- builtin/send-pack.c | 2 +- compat/terminal.c | 2 +- credential-cache--daemon.c | 4 ++-- credential-store.c | 2 +- credential.c | 2 +- daemon.c | 2 +- fast-import.c | 4 ++-- ident.c | 2 +- mailinfo.c | 8 ++++---- remote-curl.c | 6 +++--- remote-testsvn.c | 4 ++-- remote.c | 4 ++-- sequencer.c | 2 +- sha1_file.c | 2 +- shell.c | 2 +- strbuf.c | 10 ++++++++++ strbuf.h | 7 +++++++ test-sha1-array.c | 2 +- transport-helper.c | 2 +- walker.c | 2 +- wt-status.c | 4 ++-- 36 files changed, 73 insertions(+), 56 deletions(-)
bisect.c +4 -4
@@ -440,7 +440,7 @@ static void read_bisect_paths(struct argv_array *array)if (!fp)die_errno ("Could not open file '%s'" , filename); -while (strbuf_getline (&str, fp,'\n' ) !=EOF ) { +while (strbuf_getline_lf (&str, fp) !=EOF ) {strbuf_trim (&str);if (sq_dequote_to_argv_array (str.buf, array)) die("Badly quoted content in file '%s': %s",
@@ -668,7 +668,7 @@ static int is_expected_rev(const struct object_id *oid)if (!fp)return 0 ; -if (strbuf_getline (&str, fp,'\n' ) !=EOF ) +if (strbuf_getline_lf (&str, fp) !=EOF ) res = !strcmp (str.buf,oid_to_hex (oid));strbuf_release (&str);
@@ -914,9 +914,9 @@ void read_bisect_terms(const char **read_bad, const char **read_good) strerror(errno)); } }else { -strbuf_getline (&str, fp,'\n' ); +strbuf_getline_lf (&str, fp); *read_bad =strbuf_detach (&str,NULL ); -strbuf_getline (&str, fp,'\n' ); +strbuf_getline_lf (&str, fp); *read_good =strbuf_detach (&str,NULL ); }strbuf_release (&str);
builtin/am.c +7 -7
@@ -269,7 +269,7 @@ static char *read_shell_var(FILE *fp, const char *key)struct strbuf sb =STRBUF_INIT ;const char *str; -if (strbuf_getline (&sb, fp,'\n' )) +if (strbuf_getline_lf (&sb, fp)) goto fail;if (!skip_prefix (sb.buf, key, &str))
@@ -558,7 +558,7 @@ static int copy_notes_for_rebase(const struct am_state *state) fp =xfopen (am_path (state,"rewritten" ),"r" ); -while (!strbuf_getline (&sb, fp,'\n' )) { +while (!strbuf_getline_lf (&sb, fp)) {unsigned char from_obj[GIT_SHA1_RAWSZ ], to_obj[GIT_SHA1_RAWSZ ];if (sb.len !=GIT_SHA1_HEXSZ *2 +1 ) {
@@ -802,7 +802,7 @@ static int stgit_patch_to_mail(FILE *out, FILE *in, int keep_cr)struct strbuf sb =STRBUF_INIT ;int subject_printed =0 ; -while (!strbuf_getline (&sb, in,'\n' )) { +while (!strbuf_getline_lf (&sb, in)) {const char *str;if (str_isspace (sb.buf))
@@ -860,7 +860,7 @@ static int split_mail_stgit_series(struct am_state *state, const char **paths,return error (_ ("could not open '%s' for reading: %s" ), *paths,strerror (errno)); -while (!strbuf_getline (&sb, fp,'\n' )) { +while (!strbuf_getline_lf (&sb, fp)) {if (*sb.buf =='#' )continue ;/* skip comment lines */
@@ -885,7 +885,7 @@ static int hg_patch_to_mail(FILE *out, FILE *in, int keep_cr) {struct strbuf sb =STRBUF_INIT ; -while (!strbuf_getline (&sb, in,'\n' )) { +while (!strbuf_getline_lf (&sb, in)) {const char *str;if (skip_prefix (sb.buf,"# User " , &str))
@@ -1302,7 +1302,7 @@ static int parse_mail(struct am_state *state, const char *mail)/* Extract message and author information */ fp =xfopen (am_path (state,"info" ),"r" ); -while (!strbuf_getline (&sb, fp,'\n' )) { +while (!strbuf_getline_lf (&sb, fp)) {const char *x;if (skip_prefix (sb.buf,"Subject: " , &x)) {
@@ -1368,7 +1368,7 @@ static int get_mail_commit_sha1(unsigned char *commit_id, const char *mail)FILE *fp =xfopen (mail,"r" );const char *x; -if (strbuf_getline (&sb, fp,'\n' )) +if (strbuf_getline_lf (&sb, fp))return -1 ;if (!skip_prefix (sb.buf,"From " , &x))
builtin/cat-file.c +1 -1
@@ -401,7 +401,7 @@ static int batch_objects(struct batch_options *opt) save_warning = warn_on_object_refname_ambiguity; warn_on_object_refname_ambiguity =0 ; -while (strbuf_getline (&buf, stdin,'\n' ) !=EOF ) { +while (strbuf_getline_lf (&buf, stdin) !=EOF ) {if (data.split_on_whitespace) {/* * Split at first whitespace, tying off the beginning
builtin/check-mailmap.c +1 -1
@@ -54,7 +54,7 @@ int cmd_check_mailmap(int argc, const char **argv, const char *prefix)if (use_stdin) {struct strbuf buf =STRBUF_INIT ; -while (strbuf_getline (&buf, stdin,'\n' ) !=EOF ) { +while (strbuf_getline_lf (&buf, stdin) !=EOF ) {check_mailmap (&mailmap, buf.buf);maybe_flush_or_die (stdout,"stdout" ); }
builtin/clean.c +3 -3
@@ -594,7 +594,7 @@ static int *list_and_choose(struct menu_opts *opts, struct menu_stuff *stuff) clean_get_color(CLEAN_COLOR_RESET)); } -if (strbuf_getline (&choice, stdin,'\n' ) !=EOF ) { +if (strbuf_getline_lf (&choice, stdin) !=EOF ) {strbuf_trim (&choice); }else { eof =1 ;
@@ -676,7 +676,7 @@ static int filter_by_patterns_cmd(void)clean_print_color (CLEAN_COLOR_PROMPT );printf (_ ("Input ignore patterns>> " ));clean_print_color (CLEAN_COLOR_RESET ); -if (strbuf_getline (&confirm, stdin,'\n' ) !=EOF ) +if (strbuf_getline_lf (&confirm, stdin) !=EOF )strbuf_trim (&confirm);else putchar ('\n' );
@@ -774,7 +774,7 @@ static int ask_each_cmd(void) qname =quote_path_relative (item->string,NULL , &buf);/* TRANSLATORS: Make sure to keep [y/N] as is */ printf (_ ("Remove %s [y/N]? " ), qname); -if (strbuf_getline (&confirm, stdin,'\n' ) !=EOF ) { +if (strbuf_getline_lf (&confirm, stdin) !=EOF ) {strbuf_trim (&confirm); }else {putchar ('\n' );
builtin/clone.c +1 -1
@@ -339,7 +339,7 @@ static void copy_alternates(struct strbuf *src, struct strbuf *dst,FILE *in =fopen (src->buf,"r" );struct strbuf line =STRBUF_INIT ; -while (strbuf_getline (&line, in,'\n' ) !=EOF ) { +while (strbuf_getline_lf (&line, in) !=EOF ) {char *abs_path;if (!line.len || line.buf[0 ] =='#' )continue ;
builtin/column.c +1 -1
@@ -51,7 +51,7 @@ int cmd_column(int argc, const char **argv, const char *prefix)die (_ ("--command must be the first argument" )); }finalize_colopts (&colopts,-1 ); -while (!strbuf_getline (&sb, stdin,'\n' )) +while (!strbuf_getline_lf (&sb, stdin))string_list_append (&list, sb.buf);print_columns (&list, colopts, &copts);
builtin/commit.c +1 -1
@@ -1690,7 +1690,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)if (fp ==NULL )die_errno (_ ("could not open '%s' for reading" ),git_path_merge_head ()); -while (strbuf_getline (&m, fp,'\n' ) !=EOF ) { +while (strbuf_getline_lf (&m, fp) !=EOF ) {struct commit *parent; parent =get_merge_parent (m.buf);
builtin/fetch-pack.c +1 -1
@@ -158,7 +158,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)else {/* read from stdin one ref per line, until EOF */ struct strbuf line =STRBUF_INIT ; -while (strbuf_getline (&line, stdin,'\n' ) !=EOF ) +while (strbuf_getline_lf (&line, stdin) !=EOF )add_sought_entry (&sought, &nr_sought, &alloc_sought, line.buf);strbuf_release (&line); }
builtin/grep.c +1 -1
@@ -562,7 +562,7 @@ static int file_callback(const struct option *opt, const char *arg, int unset) patterns = from_stdin ? stdin :fopen (arg,"r" );if (!patterns)die_errno (_ ("cannot open '%s'" ), arg); -while (strbuf_getline (&sb, patterns,'\n' ) ==0 ) { +while (strbuf_getline_lf (&sb, patterns) ==0 ) {/* ignore empty line like grep does */ if (sb.len ==0 )continue ;
builtin/hash-object.c +1 -1
@@ -60,7 +60,7 @@ static void hash_stdin_paths(const char *type, int no_filters, unsigned flags, {struct strbuf buf =STRBUF_INIT , nbuf =STRBUF_INIT ; -while (strbuf_getline (&buf, stdin,'\n' ) !=EOF ) { +while (strbuf_getline_lf (&buf, stdin) !=EOF ) {if (buf.buf[0 ] =='"' ) {strbuf_reset (&nbuf);if (unquote_c_style (&nbuf, buf.buf,NULL ))
builtin/notes.c +1 -1
@@ -290,7 +290,7 @@ static int notes_copy_from_stdin(int force, const char *rewrite_cmd) t = &default_notes_tree; } -while (strbuf_getline (&buf, stdin,'\n' ) !=EOF ) { +while (strbuf_getline_lf (&buf, stdin) !=EOF ) {unsigned char from_obj[20 ], to_obj[20 ];struct strbuf **split;int err;
builtin/pull.c +1 -1
@@ -378,7 +378,7 @@ static void get_merge_heads(struct sha1_array *merge_heads)if (!(fp =fopen (filename,"r" )))die_errno (_ ("could not open '%s' for reading" ), filename); -while (strbuf_getline (&sb, fp,'\n' ) !=EOF ) { +while (strbuf_getline_lf (&sb, fp) !=EOF ) {if (get_sha1_hex (sb.buf, sha1))continue ;/* invalid line: does not start with SHA1 */ if (starts_with (sb.buf +GIT_SHA1_HEXSZ ,"\tnot-for-merge\t" ))
builtin/repack.c +1 -1
@@ -266,7 +266,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)return ret; out =xfdopen (cmd.out,"r" ); -while (strbuf_getline (&line, out,'\n' ) !=EOF ) { +while (strbuf_getline_lf (&line, out) !=EOF ) {if (line.len !=40 )die ("repack: Expecting 40 character sha1 lines only from pack-objects." );string_list_append (&names, line.buf);
builtin/rev-parse.c +2 -2
@@ -383,7 +383,7 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)/* get the usage up to the first line with a -- on it */ for (;;) { -if (strbuf_getline (&sb, stdin,'\n' ) ==EOF ) +if (strbuf_getline_lf (&sb, stdin) ==EOF )die ("premature end of input" );ALLOC_GROW (usage, unb +1 , usz);if (!strcmp ("--" , sb.buf)) {
@@ -396,7 +396,7 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix) }/* parse: (<short>|<short>,<long>|<long>)[*=?!]*<arghint>? SP+ <help> */ -while (strbuf_getline (&sb, stdin,'\n' ) !=EOF ) { +while (strbuf_getline_lf (&sb, stdin) !=EOF ) {const char *s;const char *help;struct option *o;
builtin/send-pack.c +1 -1
@@ -212,7 +212,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)argv_array_push (&all_refspecs, buf); }else {struct strbuf line =STRBUF_INIT ; -while (strbuf_getline (&line, stdin,'\n' ) !=EOF ) +while (strbuf_getline_lf (&line, stdin) !=EOF )argv_array_push (&all_refspecs, line.buf);strbuf_release (&line); }
compat/terminal.c +1 -1
@@ -122,7 +122,7 @@ char *git_terminal_prompt(const char *prompt, int echo)fputs (prompt, output_fh);fflush (output_fh); - r =strbuf_getline (&buf, input_fh,'\n' ); + r =strbuf_getline_lf (&buf, input_fh);if (!echo) {putc ('\n' , output_fh);fflush (output_fh);
credential-cache--daemon.c +2 -2
@@ -96,12 +96,12 @@ static int read_request(FILE *fh, struct credential *c,static struct strbuf item =STRBUF_INIT ;const char *p; -strbuf_getline (&item, fh,'\n' ); +strbuf_getline_lf (&item, fh);if (!skip_prefix (item.buf,"action=" , &p))return error ("client sent bogus action line: %s" , item.buf);strbuf_addstr (action, p); -strbuf_getline (&item, fh,'\n' ); +strbuf_getline_lf (&item, fh);if (!skip_prefix (item.buf,"timeout=" , &p))return error ("client sent bogus timeout line: %s" , item.buf); *timeout =atoi (p);
credential-store.c +1 -1
@@ -23,7 +23,7 @@ static int parse_credential_file(const char *fn,return found_credential; } -while (strbuf_getline (&line, fh,'\n' ) !=EOF ) { +while (strbuf_getline_lf (&line, fh) !=EOF ) {credential_from_url (&entry, line.buf);if (entry.username && entry.password &&credential_match (c, &entry)) {
credential.c +1 -1
@@ -142,7 +142,7 @@ int credential_read(struct credential *c, FILE *fp) {struct strbuf line =STRBUF_INIT ; -while (strbuf_getline (&line, fp,'\n' ) !=EOF ) { +while (strbuf_getline_lf (&line, fp) !=EOF ) {char *key = line.buf;char *value =strchr (key,'=' );
daemon.c +1 -1
@@ -424,7 +424,7 @@ static void copy_to_log(int fd)return ; } -while (strbuf_getline (&line, fp,'\n' ) !=EOF ) { +while (strbuf_getline_lf (&line, fp) !=EOF ) {logerror ("%s" , line.buf);strbuf_setlen (&line,0 ); }
fast-import.c +2 -2
@@ -1888,7 +1888,7 @@ static int read_next_command(void)struct recent_command *rc;strbuf_detach (&command_buf,NULL ); - stdin_eof =strbuf_getline (&command_buf, stdin,'\n' ); + stdin_eof =strbuf_getline_lf (&command_buf, stdin);if (stdin_eof)return EOF ;
@@ -1960,7 +1960,7 @@ static int parse_data(struct strbuf *sb, uintmax_t limit, uintmax_t *len_res)strbuf_detach (&command_buf,NULL );for (;;) { -if (strbuf_getline (&command_buf, stdin,'\n' ) ==EOF ) +if (strbuf_getline_lf (&command_buf, stdin) ==EOF )die ("EOF in data (terminator '%s' not found)" , term);if (term_len == command_buf.len && !strcmp (term, command_buf.buf))
ident.c +1 -1
@@ -76,7 +76,7 @@ static int add_mailname_host(struct strbuf *buf) strerror(errno));return -1 ; } -if (strbuf_getline (&mailnamebuf, mailname,'\n' ) ==EOF ) { +if (strbuf_getline_lf (&mailnamebuf, mailname) ==EOF ) {if (ferror (mailname))warning ("cannot read /etc/mailname: %s" ,strerror (errno));
mailinfo.c +4 -4
@@ -732,7 +732,7 @@ static int read_one_header_line(struct strbuf *line, FILE *in)struct strbuf continuation =STRBUF_INIT ;/* Get the first part of the line. */ -if (strbuf_getline (line, in,'\n' )) +if (strbuf_getline_lf (line, in))return 0 ;/*
@@ -756,7 +756,7 @@ static int read_one_header_line(struct strbuf *line, FILE *in) peek =fgetc (in);ungetc (peek, in);if (peek !=' ' && peek !='\t' )break ; -if (strbuf_getline (&continuation, in,'\n' )) +if (strbuf_getline_lf (&continuation, in))break ; continuation.buf[0 ] =' ' ;strbuf_rtrim (&continuation);
@@ -769,7 +769,7 @@ static int read_one_header_line(struct strbuf *line, FILE *in)static int find_boundary (struct mailinfo *mi,struct strbuf *line) { -while (!strbuf_getline (line, mi->input,'\n' )) { +while (!strbuf_getline_lf (line, mi->input)) {if (*(mi->content_top) &&is_multipart_boundary (mi, line))return 1 ; }
@@ -820,7 +820,7 @@ again:strbuf_release (&newline);/* replenish line */ -if (strbuf_getline (line, mi->input,'\n' )) +if (strbuf_getline_lf (line, mi->input))return 0 ;strbuf_addch (line,'\n' );return 1 ;
remote-curl.c +3 -3
@@ -827,7 +827,7 @@ static void parse_fetch(struct strbuf *buf)die ("http transport does not support %s" , buf->buf);strbuf_reset (buf); -if (strbuf_getline (buf, stdin,'\n' ) ==EOF ) +if (strbuf_getline_lf (buf, stdin) ==EOF )return ;if (!*buf->buf)break ;
@@ -940,7 +940,7 @@ static void parse_push(struct strbuf *buf)die ("http transport does not support %s" , buf->buf);strbuf_reset (buf); -if (strbuf_getline (buf, stdin,'\n' ) ==EOF ) +if (strbuf_getline_lf (buf, stdin) ==EOF ) goto free_specs;if (!*buf->buf)break ;
@@ -990,7 +990,7 @@ int main(int argc, const char **argv) do { const char *arg; - if (strbuf_getline(&buf, stdin, '\n') == EOF) { + if (strbuf_getline_lf(&buf, stdin) == EOF) { if (ferror(stdin)) error("remote-curl: error reading command stream from git"); return 1;
remote-testsvn.c +2 -2
@@ -154,7 +154,7 @@ static void check_or_regenerate_marks(int latestrev)fclose (marksfile); }else {strbuf_addf (&sb,":%d " , latestrev); -while (strbuf_getline (&line, marksfile,'\n' ) !=EOF ) { +while (strbuf_getline_lf (&line, marksfile) !=EOF ) {if (starts_with (line.buf, sb.buf)) { found++;break ;
@@ -322,7 +322,7 @@ int main(int argc, char **argv) marksfilename = marksfilename_sb.buf;while (1 ) { -if (strbuf_getline (&buf, stdin,'\n' ) ==EOF ) { +if (strbuf_getline_lf (&buf, stdin) ==EOF ) {if (ferror (stdin))die ("Error reading command stream" );else
remote.c +2 -2
@@ -256,7 +256,7 @@ static void read_remotes_file(struct remote *remote)if (!f)return ; remote->origin =REMOTE_REMOTES ; -while (strbuf_getline (&buf, f,'\n' ) !=EOF ) { +while (strbuf_getline_lf (&buf, f) !=EOF ) {const char *v;strbuf_rtrim (&buf);
@@ -281,7 +281,7 @@ static void read_branches_file(struct remote *remote)if (!f)return ; -strbuf_getline (&buf, f,'\n' ); +strbuf_getline_lf (&buf, f);fclose (f);strbuf_trim (&buf);if (!buf.len) {
sequencer.c +1 -1
@@ -886,7 +886,7 @@ static int sequencer_rollback(struct replay_opts *opts)if (!f)return error (_ ("cannot open %s: %s" ),git_path_head_file (),strerror (errno)); -if (strbuf_getline (&buf, f,'\n' )) { +if (strbuf_getline_lf (&buf, f)) {error (_ ("cannot read %s: %s" ),git_path_head_file (),ferror (f) ?strerror (errno) :_ ("unexpected end of file" ));fclose (f);
sha1_file.c +1 -1
@@ -396,7 +396,7 @@ void add_to_alternates_file(const char *reference)struct strbuf line =STRBUF_INIT ;int found =0 ; -while (strbuf_getline (&line, in,'\n' ) !=EOF ) { +while (strbuf_getline_lf (&line, in) !=EOF ) {if (!strcmp (reference, line.buf)) { found =1 ;break ;
shell.c +1 -1
@@ -88,7 +88,7 @@ static void run_shell(void)int count;fprintf (stderr,"git> " ); -if (strbuf_getline (&line, stdin,'\n' ) ==EOF ) { +if (strbuf_getline_lf (&line, stdin) ==EOF ) {fprintf (stderr,"\n" );strbuf_release (&line);break ;
strbuf.c +10 -0
@@ -522,6 +522,16 @@ int strbuf_getline_crlf(struct strbuf *sb, FILE *fp)return 0 ; } +int strbuf_getline_lf (struct strbuf *sb,FILE *fp) +{ +return strbuf_getline (sb, fp,'\n' ); +} + +int strbuf_getline_nul (struct strbuf *sb,FILE *fp) +{ +return strbuf_getline (sb, fp,'\0' ); +} +int strbuf_getwholeline_fd (struct strbuf *sb,int fd,int term) {strbuf_reset (sb);
strbuf.h +7 -0
@@ -388,6 +388,12 @@ extern int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint);*/ extern int strbuf_getline (struct strbuf *,FILE *,int ); + +typedef int (*strbuf_getline_fn )(struct strbuf *,FILE *); + +extern int strbuf_getline_lf (struct strbuf *sb,FILE *fp); +extern int strbuf_getline_nul (struct strbuf *sb,FILE *fp); +/* * Similar to strbuf_getline(), but uses '\n' as the terminator, * and additionally treats a '\r' that comes immediately before '\n'
@@ -395,6 +401,7 @@ extern int strbuf_getline(struct strbuf *, FILE *, int);*/ extern int strbuf_getline_crlf (struct strbuf *,FILE *); +/** * Like `strbuf_getline`, but keeps the trailing terminator (if * any) in the buffer.
test-sha1-array.c +1 -1
@@ -11,7 +11,7 @@ int main(int argc, char **argv)struct sha1_array array =SHA1_ARRAY_INIT ;struct strbuf line =STRBUF_INIT ; -while (strbuf_getline (&line, stdin,'\n' ) !=EOF ) { +while (strbuf_getline_lf (&line, stdin) !=EOF ) {const char *arg;unsigned char sha1[20 ];
transport-helper.c +1 -1
@@ -54,7 +54,7 @@ static int recvline_fh(FILE *helper, struct strbuf *buffer, const char *name)strbuf_reset (buffer);if (debug)fprintf (stderr,"Debug: Remote helper: Waiting...\n" ); -if (strbuf_getline (buffer, helper,'\n' ) ==EOF ) { +if (strbuf_getline_lf (buffer, helper) ==EOF ) {if (debug)fprintf (stderr,"Debug: Remote helper quit.\n" );return 1 ;
walker.c +1 -1
@@ -220,7 +220,7 @@ int walker_targets_stdin(char ***target, const char ***write_ref)char *rf_one =NULL ;char *tg_one; -if (strbuf_getline (&buf, stdin,'\n' ) ==EOF ) +if (strbuf_getline_lf (&buf, stdin) ==EOF )break ; tg_one = buf.buf; rf_one =strchr (tg_one,'\t' );
wt-status.c +2 -2
@@ -988,7 +988,7 @@ static char *read_line_from_git_path(const char *filename)strbuf_release (&buf);return NULL ; } -strbuf_getline (&buf, fp,'\n' ); +strbuf_getline_lf (&buf, fp);if (!fclose (fp)) {return strbuf_detach (&buf,NULL ); }else {
@@ -1076,7 +1076,7 @@ static void read_rebase_todolist(const char *fname, struct string_list *lines)if (!f)die_errno ("Could not open file %s for reading" ,git_path ("%s" , fname)); -while (!strbuf_getline (&line, f,'\n' )) { +while (!strbuf_getline_lf (&line, f)) {if (line.len && line.buf[0 ] == comment_line_char)continue ;strbuf_trim (&line);
Signature · 1 line
2.7.0-250-ge1b5ba3
PATCH v4 4/21 mktree: there are only two possible line terminations
In reply to Junio C HamanoThe program by default reads LF terminated lines, with an option to use NUL terminated records. Instead of pretending that there can be other useful values for line_termination, use a boolean variable, nul_term_line, to tell if NUL terminated records are used, and switch between strbuf_getline_{lf,nul} based on it.
--- builtin/mktree.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)
builtin/mktree.c +8 -6
@@ -65,7 +65,7 @@ static const char *mktree_usage[] = { NULL }; -static void mktree_line (char *buf,size_t len,int line_termination,int allow_missing) +static void mktree_line (char *buf,size_t len,int nul_term_line,int allow_missing) {char *ptr, *ntr;unsigned mode;
@@ -97,7 +97,7 @@ static void mktree_line(char *buf, size_t len, int line_termination, int allow_m *ntr++ =0 ;/* now at the beginning of SHA1 */ path = ntr +41 ;/* at the beginning of name */ -if (line_termination && path[0 ] =='"' ) { +if (!nul_term_line && path[0 ] =='"' ) {struct strbuf p_uq =STRBUF_INIT ;if (unquote_c_style (&p_uq, path,NULL ))die ("invalid quoting" );
@@ -141,23 +141,25 @@ int cmd_mktree(int ac, const char **av, const char *prefix) {struct strbuf sb =STRBUF_INIT ;unsigned char sha1[20 ]; -int line_termination ='\n' ; +int nul_term_line =0 ;int allow_missing =0 ;int is_batch_mode =0 ;int got_eof =0 ; +strbuf_getline_fn getline_fn;const struct option option[] = { -OPT_SET_INT ('z' ,NULL , &line_termination,N_ ("input is NUL terminated" ),'\0' ), +OPT_BOOL ('z' ,NULL , &nul_term_line,N_ ("input is NUL terminated" )),OPT_SET_INT (0 ,"missing" , &allow_missing,N_ ("allow missing objects" ),1 ),OPT_SET_INT (0 ,"batch" , &is_batch_mode,N_ ("allow creation of more than one tree" ),1 ),OPT_END () }; ac =parse_options (ac, av, prefix, option, mktree_usage,0 ); + getline_fn = nul_term_line ? strbuf_getline_nul : strbuf_getline_lf;while (!got_eof) {while (1 ) { -if (strbuf_getline (&sb, stdin, line_termination) ==EOF ) { +if (getline_fn (&sb, stdin) ==EOF ) { got_eof =1 ;break ; }
@@ -167,7 +169,7 @@ int cmd_mktree(int ac, const char **av, const char *prefix)break ;die ("input format error: (blank line only valid in batch mode)" ); } -mktree_line (sb.buf, sb.len, line_termination, allow_missing); +mktree_line (sb.buf, sb.len, nul_term_line, allow_missing); }if (is_batch_mode && got_eof && used <1 ) {/*
Signature · 1 line
2.7.0-250-ge1b5ba3
PATCH v4 5/21 check-attr: there are only two possible line terminations
In reply to Junio C HamanoThe program by default reads LF terminated lines, with an option to use NUL terminated records. Instead of pretending that there can be other useful values for line_termination, use a boolean variable, nul_term_line, to tell if NUL terminated records are used, and switch between strbuf_getline_{lf,nul} based on it.
--- builtin/check-attr.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
builtin/check-attr.c +4 -3
@@ -73,12 +73,13 @@ static void check_attr_stdin_paths(const char *prefix, int cnt, struct git_attr_check *check) {struct strbuf buf, nbuf; -int line_termination = nul_term_line ?0 :'\n' ; +strbuf_getline_fn getline_fn; + getline_fn = nul_term_line ? strbuf_getline_nul : strbuf_getline_lf;strbuf_init (&buf,0 );strbuf_init (&nbuf,0 ); -while (strbuf_getline (&buf, stdin, line_termination) !=EOF ) { -if (line_termination && buf.buf[0 ] =='"' ) { +while (getline_fn (&buf, stdin) !=EOF ) { +if (!nul_term_line && buf.buf[0 ] =='"' ) {strbuf_reset (&nbuf);if (unquote_c_style (&nbuf, buf.buf,NULL ))die ("line is badly quoted" );
Signature · 1 line
2.7.0-250-ge1b5ba3
Re: [PATCH v4 05/21] check-attr: there are only two possible line terminations
Quoted 16 lines · Junio C Hamano
The program by default reads LF terminated lines, with an option to use NUL terminated records. Instead of pretending that there can be other useful values for
line_termination, use a boolean variable,nul_term_line, to tell if NUL terminated records are used, and switch between strbuf_getline_{lf,nul} based on it.Signed-off-by Junio C Hamano <[email protected]>--- builtin/check-attr.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)builtin/check-attr.c +0
-0
builtin/check-attr.c static void check_attr_stdin_paths(const char *prefix, int cnt, · line 80
Quoted 9 lines
builtin/check-attr.c +2
-1@@ -73,12 +73,13 @@ static void check_attr_stdin_paths(const char *prefix, int cnt, struct git_attr_check *check) {struct strbuf buf, nbuf; -int line_termination = nul_term_line ?0 :'\n' ; +strbuf_getline_fn getline_fn; + getline_fn = nul_term_line ? strbuf_getline_nul : strbuf_getline_lf;strbuf_init (&buf,0 );strbuf_init (&nbuf,0 );
Not really relevant to your series, but I find it slightly confusing when strbuf_init is used instead of a static initializer (it makes me wonder _why_, and whether there are any code paths that miss initialization).
Maybe worth doing:
struct strbuf buf = STRBUF_INIT; struct strbuf nbuf = STRBUF_INIT;
at the top while we are in the area (and also maybe giving the latter a more meaningful name!). But I don't want to derail the main point of your series.
-Peff
Re: [PATCH v4 05/21] check-attr: there are only two possible line terminations
Quoted 5 lines · Jeff King
Not really relevant to your series, but I find it slightly confusing when
strbuf_initis used instead of a static initializer (it makes me wonder _why_, and whether there are any code paths that miss initialization).
Quoted 8 lines
Maybe worth doing:
struct strbuf buf = STRBUF_INIT; struct strbuf nbuf = STRBUF_INIT;at the top while we are in the area (and also maybe giving the latter a more meaningful name!). But I don't want to derail the main point of your series.
Apparently we use this pattern in quite a few places (or they all copied from each other). Here's a possible fixup that could go after your series (it could go before, too, but it creates a lot of textual conflicts with your stuff).
-- >8 --
Subject: [PATCH] give "nbuf" strbuf a more meaningful name
It's a common pattern in our code to read paths from stdin, separated either by newlines or NULs, and unquote as necessary. In each of these five cases we use "nbuf" to temporarily store the unquoted value. Let's give it the more meaningful name "unquoted", which makes it easier to understand the purpose of the variable.
While we're at it, let's also static-initialize all of our strbufs. It's not wrong to call strbuf_init, but it increases the cognitive load on the reader (who might wonder "do we sometimes avoid initializing them? why?").
---
It's a shame that we can't just factor out this common code, but I don't think it's quite long enough to merit the boilerplate. The interesting part of each function happens inside the loop. If C had lambdas, we could do something like:
foreach_path_from(stdin, nul_term_line) {
/* now do something interesting with "buf"
and some other local variables */}
but using function pointers for this kind of iteration is pretty awful (define the two-line loop body as a separate function, stuff any local variables into a struct and pass it as "void *", etc). You can overcome that with macros and make the above code work if you don't mind creating an undebuggable mess. :)
builtin/check-attr.c | 13 ++++++------- builtin/check-ignore.c | 13 ++++++------- builtin/checkout-index.c | 11 ++++++----- builtin/hash-object.c | 11 ++++++----- builtin/update-index.c | 11 ++++++----- 5 files changed, 30 insertions(+), 29 deletions(-)
builtin/check-attr.c +6 -7
@@ -72,24 +72,23 @@ static void check_attr(const char *prefix, int cnt,static void check_attr_stdin_paths (const char *prefix,int cnt,struct git_attr_check *check) { -struct strbuf buf, nbuf; +struct strbuf buf =STRBUF_INIT ; +struct strbuf unquoted =STRBUF_INIT ;strbuf_getline_fn getline_fn; getline_fn = nul_term_line ? strbuf_getline_nul : strbuf_getline_lf; -strbuf_init (&buf,0 ); -strbuf_init (&nbuf,0 );while (getline_fn (&buf, stdin) !=EOF ) {if (!nul_term_line && buf.buf[0 ] =='"' ) { -strbuf_reset (&nbuf); -if (unquote_c_style (&nbuf, buf.buf,NULL )) +strbuf_reset (&unquoted); +if (unquote_c_style (&unquoted, buf.buf,NULL ))die ("line is badly quoted" ); -strbuf_swap (&buf, &nbuf); +strbuf_swap (&buf, &unquoted); }check_attr (prefix, cnt, check, buf.buf);maybe_flush_or_die (stdout,"attribute to stdout" ); }strbuf_release (&buf); -strbuf_release (&nbuf); +strbuf_release (&unquoted); } static NORETURN void error_with_usage(const char *msg)
builtin/check-ignore.c +6 -7
@@ -115,20 +115,19 @@ static int check_ignore(struct dir_struct *dir,static int check_ignore_stdin_paths (struct dir_struct *dir,const char *prefix) { -struct strbuf buf, nbuf; +struct strbuf buf =STRBUF_INIT ; +struct strbuf unquoted =STRBUF_INIT ;char *pathspec[2 ] = {NULL ,NULL };strbuf_getline_fn getline_fn;int num_ignored =0 ; getline_fn = nul_term_line ? strbuf_getline_nul : strbuf_getline_lf; -strbuf_init (&buf,0 ); -strbuf_init (&nbuf,0 );while (getline_fn (&buf, stdin) !=EOF ) {if (!nul_term_line && buf.buf[0 ] =='"' ) { -strbuf_reset (&nbuf); -if (unquote_c_style (&nbuf, buf.buf,NULL )) +strbuf_reset (&unquoted); +if (unquote_c_style (&unquoted, buf.buf,NULL ))die ("line is badly quoted" ); -strbuf_swap (&buf, &nbuf); +strbuf_swap (&buf, &unquoted); } pathspec[0 ] = buf.buf; num_ignored += check_ignore(dir, prefix,
@@ -136,7 +135,7 @@ static int check_ignore_stdin_paths(struct dir_struct *dir, const char *prefix)maybe_flush_or_die (stdout,"check-ignore to stdout" ); }strbuf_release (&buf); -strbuf_release (&nbuf); +strbuf_release (&unquoted);return num_ignored; }
builtin/checkout-index.c +6 -5
@@ -251,7 +251,8 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix) }if (read_from_stdin) { -struct strbuf buf =STRBUF_INIT , nbuf =STRBUF_INIT ; +struct strbuf buf =STRBUF_INIT ; +struct strbuf unquoted =STRBUF_INIT ;strbuf_getline_fn getline_fn;if (all)
@@ -261,16 +262,16 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)while (getline_fn (&buf, stdin) !=EOF ) {char *p;if (!nul_term_line && buf.buf[0 ] =='"' ) { -strbuf_reset (&nbuf); -if (unquote_c_style (&nbuf, buf.buf,NULL )) +strbuf_reset (&unquoted); +if (unquote_c_style (&unquoted, buf.buf,NULL ))die ("line is badly quoted" ); -strbuf_swap (&buf, &nbuf); +strbuf_swap (&buf, &unquoted); } p =prefix_path (prefix, prefix_length, buf.buf);checkout_file (p, prefix);free (p); } -strbuf_release (&nbuf); +strbuf_release (&unquoted);strbuf_release (&buf); }
builtin/hash-object.c +6 -5
@@ -58,20 +58,21 @@ static void hash_object(const char *path, const char *type, const char *vpath,static void hash_stdin_paths (const char *type,int no_filters,unsigned flags,int literally) { -struct strbuf buf =STRBUF_INIT , nbuf =STRBUF_INIT ; +struct strbuf buf =STRBUF_INIT ; +struct strbuf unquoted =STRBUF_INIT ;while (strbuf_getline (&buf, stdin) !=EOF ) {if (buf.buf[0 ] =='"' ) { -strbuf_reset (&nbuf); -if (unquote_c_style (&nbuf, buf.buf,NULL )) +strbuf_reset (&unquoted); +if (unquote_c_style (&unquoted, buf.buf,NULL ))die ("line is badly quoted" ); -strbuf_swap (&buf, &nbuf); +strbuf_swap (&buf, &unquoted); }hash_object (buf.buf, type, no_filters ?NULL : buf.buf, flags, literally); }strbuf_release (&buf); -strbuf_release (&nbuf); +strbuf_release (&unquoted); }int cmd_hash_object (int argc,const char **argv,const char *prefix)
builtin/update-index.c +6 -5
@@ -1075,16 +1075,17 @@ int cmd_update_index(int argc, const char **argv, const char *prefix) }if (read_from_stdin) { -struct strbuf buf =STRBUF_INIT , nbuf =STRBUF_INIT ; +struct strbuf buf =STRBUF_INIT ; +struct strbuf unquoted =STRBUF_INIT ;setup_work_tree ();while (getline_fn (&buf, stdin) !=EOF ) {char *p;if (!nul_term_line && buf.buf[0 ] =='"' ) { -strbuf_reset (&nbuf); -if (unquote_c_style (&nbuf, buf.buf,NULL )) +strbuf_reset (&unquoted); +if (unquote_c_style (&unquoted, buf.buf,NULL ))die ("line is badly quoted" ); -strbuf_swap (&buf, &nbuf); +strbuf_swap (&buf, &unquoted); } p =prefix_path (prefix, prefix_length, buf.buf);update_one (p);
@@ -1092,7 +1093,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)chmod_path (set_executable_bit, p);free (p); } -strbuf_release (&nbuf); +strbuf_release (&unquoted);strbuf_release (&buf); }
Signature · 1 line
2.7.0.244.g0701a9d
Re: [PATCH v4 05/21] check-attr: there are only two possible line terminations
Quoted 4 lines · Jeff King
Apparently we use this pattern in quite a few places (or they all copied from each other). Here's a possible fixup that could go after your series (it could go before, too, but it creates a lot of textual conflicts with your stuff).
Sounds good, but I'm in favor of leaving this as an unrelated clean-up that happens to touch the same area.
Re: [PATCH v4 05/21] check-attr: there are only two possible line terminations
Quoted 9 lines · Junio C Hamano
Quoted 4 lines · Jeff King
Apparently we use this pattern in quite a few places (or they all copied from each other). Here's a possible fixup that could go after your series (it could go before, too, but it creates a lot of textual conflicts with your stuff).
Sounds good, but I'm in favor of leaving this as an unrelated clean-up that happens to touch the same area.
Would you prefer me to hold it and send it later, after your series is merged, or just rebase it on master, have it graduate separately, and let you deal with the conflicts?
-Peff
Re: [PATCH v4 05/21] check-attr: there are only two possible line terminations
Quoted 6 lines · Jeff King
Quoted 2 lines
Sounds good, but I'm in favor of leaving this as an unrelated clean-up that happens to touch the same area.
Would you prefer me to hold it and send it later, after your series is merged, or just rebase it on master, have it graduate separately, and let you deal with the conflicts?
If you give me a choice, then I'd take the one that leaves less for me to keep track of ;-)
Thanks.
PATCH v4 6/21 check-ignore: there are only two possible line terminations
In reply to Junio C HamanoThe program by default reads LF terminated lines, with an option to use NUL terminated records. Instead of pretending that there can be other useful values for line_termination, use a boolean variable, nul_term_line, to tell if NUL terminated records are used, and switch between strbuf_getline_{lf,nul} based on it.
--- builtin/check-ignore.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
builtin/check-ignore.c +4 -3
@@ -117,13 +117,14 @@ static int check_ignore_stdin_paths(struct dir_struct *dir, const char *prefix) {struct strbuf buf, nbuf;char *pathspec[2 ] = {NULL ,NULL }; -int line_termination = nul_term_line ?0 :'\n' ; +strbuf_getline_fn getline_fn;int num_ignored =0 ; + getline_fn = nul_term_line ? strbuf_getline_nul : strbuf_getline_lf;strbuf_init (&buf,0 );strbuf_init (&nbuf,0 ); -while (strbuf_getline (&buf, stdin, line_termination) !=EOF ) { -if (line_termination && buf.buf[0 ] =='"' ) { +while (getline_fn (&buf, stdin) !=EOF ) { +if (!nul_term_line && buf.buf[0 ] =='"' ) {strbuf_reset (&nbuf);if (unquote_c_style (&nbuf, buf.buf,NULL ))die ("line is badly quoted" );
Signature · 1 line
2.7.0-250-ge1b5ba3
PATCH v4 7/21 update-index: there are only two possible line terminations
In reply to Junio C HamanoThe program by default reads LF terminated lines, with an option to use NUL terminated records. Instead of pretending that there can be other useful values for line_termination, use a boolean variable, nul_term_line, to tell if NUL terminated records are used, and switch between strbuf_getline_{lf,nul} based on it.
--- builtin/update-index.c | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-)
builtin/update-index.c +16 -11
@@ -468,12 +468,14 @@ static void update_one(const char *path)report ("add '%s'" , path); } -static void read_index_info (int line_termination) +static void read_index_info (int nul_term_line) {struct strbuf buf =STRBUF_INIT ;struct strbuf uq =STRBUF_INIT ; +strbuf_getline_fn getline_fn; -while (strbuf_getline (&buf, stdin, line_termination) !=EOF ) { + getline_fn = nul_term_line ? strbuf_getline_nul : strbuf_getline_lf; +while (getline_fn (&buf, stdin) !=EOF ) {char *ptr, *tab;char *path_name;unsigned char sha1[20 ];
@@ -522,7 +524,7 @@ static void read_index_info(int line_termination) goto bad_line; path_name = ptr; -if (line_termination && path_name[0 ] =='"' ) { +if (!nul_term_line && path_name[0 ] =='"' ) {strbuf_reset (&uq);if (unquote_c_style (&uq, path_name,NULL )) {die ("git update-index: bad quoting of path name" );
@@ -844,12 +846,12 @@ static int cacheinfo_callback(struct parse_opt_ctx_t *ctx,static int stdin_cacheinfo_callback (struct parse_opt_ctx_t *ctx,const struct option *opt,int unset) { -int *line_termination = opt->value; +int *nul_term_line = opt->value;if (ctx->argc !=1 )return error ("option '%s' must be the last argument" , opt->long_name); allow_add = allow_replace = allow_remove =1 ; -read_index_info (*line_termination); +read_index_info (*nul_term_line);return 0 ; }
@@ -901,7 +903,7 @@ static int reupdate_callback(struct parse_opt_ctx_t *ctx,int cmd_update_index (int argc,const char **argv,const char *prefix) { -int newfd, entries, has_errors =0 , line_termination ='\n' ; +int newfd, entries, has_errors =0 , nul_term_line =0 ;int untracked_cache =-1 ;int read_from_stdin =0 ;int prefix_length = prefix ?strlen (prefix) :0 ;
@@ -912,6 +914,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)int split_index =-1 ;struct lock_file *lock_file;struct parse_opt_ctx_t ctx; +strbuf_getline_fn getline_fn;int parseopt_state =PARSE_OPT_UNKNOWN ;struct option options[] = { OPT_BIT('q', NULL, &refresh_args.flags,
@@ -963,13 +966,13 @@ int cmd_update_index(int argc, const char **argv, const char *prefix) N_("add to index only; do not add content to object database"), 1),OPT_SET_INT (0 ,"force-remove" , &force_remove,N_ ("remove named paths even if present in worktree" ),1 ), -OPT_SET_INT ('z' ,NULL , &line_termination, -N_ ("with --stdin: input lines are terminated by null bytes" ),'\0' ), +OPT_BOOL ('z' ,NULL , &nul_term_line, +N_ ("with --stdin: input lines are terminated by null bytes" )), {OPTION_LOWLEVEL_CALLBACK, 0, "stdin", &read_from_stdin, NULL,N_ ("read list of paths to be updated from standard input" ),PARSE_OPT_NONEG |PARSE_OPT_NOARG , (parse_opt_cb *) stdin_callback}, - {OPTION_LOWLEVEL_CALLBACK, 0, "index-info", &line_termination, NULL, + {OPTION_LOWLEVEL_CALLBACK, 0, "index-info", &nul_term_line, NULL,N_ ("add entries from standard input to the index" ),PARSE_OPT_NONEG |PARSE_OPT_NOARG , (parse_opt_cb *) stdin_cacheinfo_callback},
@@ -1057,6 +1060,8 @@ int cmd_update_index(int argc, const char **argv, const char *prefix) } } argc =parse_options_end (&ctx); + + getline_fn = nul_term_line ? strbuf_getline_nul : strbuf_getline_lf;if (preferred_index_format) {if (preferred_index_format <INDEX_FORMAT_LB ||INDEX_FORMAT_UB < preferred_index_format)
@@ -1073,9 +1078,9 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)struct strbuf buf =STRBUF_INIT , nbuf =STRBUF_INIT ;setup_work_tree (); -while (strbuf_getline (&buf, stdin, line_termination) !=EOF ) { +while (getline_fn (&buf, stdin) !=EOF ) {char *p; -if (line_termination && buf.buf[0 ] =='"' ) { +if (!nul_term_line && buf.buf[0 ] =='"' ) {strbuf_reset (&nbuf);if (unquote_c_style (&nbuf, buf.buf,NULL ))die ("line is badly quoted" );
Signature · 1 line
2.7.0-250-ge1b5ba3
PATCH v4 8/21 checkout-index: there are only two possible line terminations
In reply to Junio C HamanoThe program by default reads LF terminated lines, with an option to use NUL terminated records. Instead of pretending that there can be other useful values for line_termination, use a boolean variable, nul_term_line, to tell if NUL terminated records are used, and switch between strbuf_getline_{lf,nul} based on it.
--- builtin/checkout-index.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)
builtin/checkout-index.c +8 -8
@@ -11,7 +11,7 @@#include "parse-options.h" #define CHECKOUT_ALL 4 -static int line_termination ='\n' ; +static int nul_term_line;static int checkout_stage;/* default to checkout stage0 */ static int to_tempfile;static char topath[4 ][TEMPORARY_FILENAME_LENGTH +1 ];
@@ -35,7 +35,8 @@ static void write_tempfile_record(const char *name, const char *prefix)fputs (topath[checkout_stage], stdout);putchar ('\t' ); -write_name_quoted_relative (name, prefix, stdout, line_termination); +write_name_quoted_relative (name, prefix, stdout, + nul_term_line ?'\0' :'\n' );for (i =0 ; i <4 ; i++) { topath[i][0 ] =0 ;
@@ -144,10 +145,7 @@ static int option_parse_u(const struct option *opt,static int option_parse_z (const struct option *opt,const char *arg,int unset) { -if (unset) - line_termination ='\n' ; -else - line_termination =0 ; + nul_term_line = !unset;return 0 ; }
@@ -254,13 +252,15 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)if (read_from_stdin) {struct strbuf buf =STRBUF_INIT , nbuf =STRBUF_INIT ; +strbuf_getline_fn getline_fn;if (all)die ("git checkout-index: don't mix '--all' and '--stdin'" ); -while (strbuf_getline (&buf, stdin, line_termination) !=EOF ) { + getline_fn = nul_term_line ? strbuf_getline_nul : strbuf_getline_lf; +while (getline_fn (&buf, stdin) !=EOF ) {char *p; -if (line_termination && buf.buf[0 ] =='"' ) { +if (!nul_term_line && buf.buf[0 ] =='"' ) {strbuf_reset (&nbuf);if (unquote_c_style (&nbuf, buf.buf,NULL ))die ("line is badly quoted" );
Signature · 1 line
2.7.0-250-ge1b5ba3
Re: [PATCH v4 08/21] checkout-index: there are only two possible line terminations
Review static int option_parse_u(const struct option *opt, · line 150
Quoted 11 lines · Junio C Hamano
(quoted diff) +1
-4@@ -144,10 +145,7 @@ static int option_parse_u(const struct option *opt, static int option_parse_z(const struct option *opt, const char *arg, int unset) { - if (unset) - line_termination = '\n'; - else - line_termination = 0; + nul_term_line = !unset; return 0; }
Is it worth doing this on top?
-- >8 --
Subject: [PATCH] checkout-index: simplify "-z" option parsing
Now that we act as a simple bool, there's no need to use a custom callback.
--- builtin/checkout-index.c | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-)
builtin/checkout-index.c +2 -10
@@ -142,13 +142,6 @@ static int option_parse_u(const struct option *opt,return 0 ; } -static int option_parse_z (const struct option *opt, -const char *arg,int unset) -{ - nul_term_line = !unset; -return 0 ; -} -static int option_parse_prefix (const struct option *opt,const char *arg,int unset) {
@@ -192,9 +185,8 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix) {OPTION_CALLBACK ,'u' ,"index" , &newfd,NULL ,N_ ("update stat information in the index file" ), PARSE_OPT_NOARG, option_parse_u }, - { OPTION_CALLBACK, 'z', NULL, NULL, NULL, -N_ ("paths are separated with NUL character" ), - PARSE_OPT_NOARG, option_parse_z }, +OPT_BOOL ('z' ,NULL , &nul_term_line, +N_ ("paths are separated with NUL character" )),OPT_BOOL (0 ,"stdin" , &read_from_stdin,N_ ("read list of paths from the standard input" )), OPT_BOOL(0, "temp", &to_tempfile,
Signature · 1 line
2.7.0.244.g0701a9d
Re: [PATCH v4 08/21] checkout-index: there are only two possible line terminations
Quoted 7 lines · Jeff King
Is it worth doing this on top?
-- >8 --
Subject: [PATCH] checkout-index: simplify "-z" option parsingNow that we act as a simple bool, there's no need to use a custom callback.
Of course this is a backward step if we wanted to add negation. But I suspect if we did so it would not be as "--no-nul-termination" (which involves inventing "--nul-termination" in the first place), but rather as a separate option. Anyway, since AFAIK nobody has plans to add one, the code has been like this for years, we don't significantly impede anyone from doing it later, I'm inclined to err on the side of simplifying.
If we do, there are two similar candidates:
-- >8 --
Subject: [PATCH] apply, ls-files: simplify "-z" parsing
As a short option, we cannot handle negation. Thus a callback handling "unset" is overkill, and we can just use OPT_SET_INT instead to handle setting the option.
---
These could also become OPT_BOOL, but then we have to translate that to '\n' / NUL later in the code.
I was also curious what "--no-foo" does (if a long option _is_ defined) with OPT_SET_INT. It resets the value to 0, which would be wrong in this case. I guess that makes a slight trap for anybody adding "--nul-termination" later (if they do not think about "--no-nul-termination", they may not realize it does not work). I'm not all that moved by that line of reasoning personally, though.
builtin/apply.c | 15 ++------------- builtin/ls-files.c | 13 ++----------- 2 files changed, 4 insertions(+), 24 deletions(-)
builtin/apply.c +2 -13
@@ -4464,16 +4464,6 @@ static int option_parse_p(const struct option *opt,return 0 ; } -static int option_parse_z (const struct option *opt, -const char *arg,int unset) -{ -if (unset) - line_termination ='\n' ; -else - line_termination =0 ; -return 0 ; -} -static int option_parse_space_change (const struct option *opt,const char *arg,int unset) {
@@ -4546,9 +4536,8 @@ int cmd_apply(int argc, const char **argv, const char *prefix_) N_( "attempt three-way merge if a patch does not apply")),OPT_FILENAME (0 ,"build-fake-ancestor" , &fake_ancestor,N_ ("build a temporary index based on embedded index information" )), - { OPTION_CALLBACK, 'z', NULL, NULL, NULL, -N_ ("paths are separated with NUL character" ), - PARSE_OPT_NOARG, option_parse_z }, +OPT_SET_INT ('z' ,NULL , &line_termination, +N_ ("paths are separated with NUL character" ),'\0' ),OPT_INTEGER ('C' ,NULL , &p_context,N_ ("ensure at least <n> lines of context match" )), { OPTION_CALLBACK, 0, "whitespace", &whitespace_option, N_("action"),
builtin/ls-files.c +2 -11
@@ -359,14 +359,6 @@ static const char * const ls_files_usage[] = { NULL }; -static int option_parse_z (const struct option *opt, -const char *arg,int unset) -{ - line_terminator = unset ?'\n' :'\0' ; - -return 0 ; -} -static int option_parse_exclude (const struct option *opt,const char *arg,int unset) {
@@ -408,9 +400,8 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)struct exclude_list *el;struct string_list exclude_list =STRING_LIST_INIT_NODUP ;struct option builtin_ls_files_options[] = { - {OPTION_CALLBACK ,'z' ,NULL ,NULL ,NULL , -N_ ("paths are separated with NUL character" ), -PARSE_OPT_NOARG , option_parse_z }, +OPT_SET_INT ('z' ,NULL , &line_terminator, +N_ ("paths are separated with NUL character" ),'\0' ),OPT_BOOL ('t' ,NULL , &show_tag,N_ ("identify the file status with tags" )), OPT_BOOL('v', NULL, &show_valid_bit,
Signature · 1 line
2.7.0.244.g0701a9d
Re: [PATCH v4 08/21] checkout-index: there are only two possible line terminations
In reply to Jeff KingQuoted 15 lines · Jeff King
Review static int option_parse_u(const struct option *opt, · line 150
Quoted 11 lines · Junio C Hamano
(quoted diff) +1
-4@@ -144,10 +145,7 @@ static int option_parse_u(const struct option *opt, static int option_parse_z(const struct option *opt, const char *arg, int unset) { - if (unset) - line_termination = '\n'; - else - line_termination = 0; + nul_term_line = !unset; return 0; }Is it worth doing this on top?
Yes.
To be bluntly honest, the above callback has no right to exist. I should have turned it from OPTION_CALLBACK to OPT_BOOL from the start.
Quoted 45 lines
-- >8 --
Subject: [PATCH] checkout-index: simplify "-z" option parsingNow that we act as a simple bool, there's no need to use a custom callback.
Signed-off-by Jeff King <[email protected]>---
builtin/checkout-index.c | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-)builtin/checkout-index.c +2
-10@@ -142,13 +142,6 @@ static int option_parse_u(const struct option *opt,return 0 ; } -static int option_parse_z (const struct option *opt, -const char *arg,int unset) -{ - nul_term_line = !unset; -return 0 ; -} -static int option_parse_prefix (const struct option *opt,const char *arg,int unset) {@@ -192,9 +185,8 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix) {OPTION_CALLBACK ,'u' ,"index" , &newfd,NULL ,N_ ("update stat information in the index file" ), PARSE_OPT_NOARG, option_parse_u }, - { OPTION_CALLBACK, 'z', NULL, NULL, NULL, -N_ ("paths are separated with NUL character" ), - PARSE_OPT_NOARG, option_parse_z }, +OPT_BOOL ('z' ,NULL , &nul_term_line, +N_ ("paths are separated with NUL character" )),OPT_BOOL (0 ,"stdin" , &read_from_stdin,N_ ("read list of paths from the standard input" )), OPT_BOOL(0, "temp", &to_tempfile,
Re: [PATCH v4 08/21] checkout-index: there are only two possible line terminations
Quoted 18 lines · Junio C Hamano
Quoted 15 lines · Jeff King
Review static int option_parse_u(const struct option *opt, · line 150
Quoted 11 lines · Junio C Hamano
(quoted diff) +1
-4@@ -144,10 +145,7 @@ static int option_parse_u(const struct option *opt, static int option_parse_z(const struct option *opt, const char *arg, int unset) { - if (unset) - line_termination = '\n'; - else - line_termination = 0; + nul_term_line = !unset; return 0; }Is it worth doing this on top?
Quoted 5 lines
Yes.
To be bluntly honest, the above callback has no right to exist. I should have turned it from
OPTION_CALLBACKtoOPT_BOOLfrom the start.
Having said that, I'd leave this change out of my tree for now, until the patches 1-9, which I'll split out from the rest of the series into a separate topic, moves out of the way.
I have this suspicion that "checkout-index" inherited the peculiar stance "update-index" took about the command line arguments and options (e.g. "update-index A --add B" and "update-index --add A B" do different things for path "A").
As far as I can see, there is no reason why "-u", just like "-z" that you noticed, has to be processed immediately as it is read before processing the remainder of the command line. And I think the change you suggested to stop using a callback for "-z" is better done with the same patch as the one that fixes "-u", whose log message would probably be:
checkout-index: simplify option parsing
For some reason, the command line parser for "-u" and "-z" options tried to make the effect of the options as they are processed by using OPTION_CALLBACK, but there is no reason to do so. Just flip the bit to remember that we have seen these options while parsing the command line, and use them after we are done parsing, just like all other options.
or something.
PATCH v4 9/21 strbuf: give strbuf_getline() to the "most text friendly" variant
In reply to Junio C HamanoNow there is no direct caller to strbuf_getline(), we can demote it to file-scope static private to strbuf.c implementation and rename it to strbuf_getdelim(). Rename strbuf_getline_crlf(), which is designed to be the most "text friendly" variant, and allow it to take over this simplest name, strbuf_getline(), so we can add more uses of it without having to type _crlf over and over again in the coming steps.
--- builtin/am.c | 8 ++++---- strbuf.c | 8 ++++---- strbuf.h | 25 ++++++++++++++----------- transport-helper.c | 3 ++- 4 files changed, 24 insertions(+), 20 deletions(-)
builtin/am.c +4 -4
@@ -613,7 +613,7 @@ static int is_mail(FILE *fp)if (regcomp (®ex, header_regex,REG_NOSUB |REG_EXTENDED ))die ("invalid pattern: %s" , header_regex); -while (!strbuf_getline_crlf (&sb, fp)) { +while (!strbuf_getline (&sb, fp)) {if (!sb.len)break ;/* End of header */
@@ -660,7 +660,7 @@ static int detect_patch_format(const char **paths) fp =xfopen (*paths,"r" ); -while (!strbuf_getline_crlf (&l1, fp)) { +while (!strbuf_getline (&l1, fp)) {if (l1.len)break ; }
@@ -681,9 +681,9 @@ static int detect_patch_format(const char **paths) }strbuf_reset (&l2); -strbuf_getline_crlf (&l2, fp); +strbuf_getline (&l2, fp);strbuf_reset (&l3); -strbuf_getline_crlf (&l3, fp); +strbuf_getline (&l3, fp);/* * If the second line is empty and the third is a From, Author or Date
strbuf.c +4 -4
@@ -501,7 +501,7 @@ int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term) }#endif -int strbuf_getline (struct strbuf *sb,FILE *fp,int term) +static int strbuf_getdelim (struct strbuf *sb,FILE *fp,int term) {if (strbuf_getwholeline (sb, fp, term))return EOF ;
@@ -510,7 +510,7 @@ int strbuf_getline(struct strbuf *sb, FILE *fp, int term)return 0 ; } -int strbuf_getline_crlf (struct strbuf *sb,FILE *fp) +int strbuf_getline (struct strbuf *sb,FILE *fp) {if (strbuf_getwholeline (sb, fp,'\n' ))return EOF ;
@@ -524,12 +524,12 @@ int strbuf_getline_crlf(struct strbuf *sb, FILE *fp)int strbuf_getline_lf (struct strbuf *sb,FILE *fp) { -return strbuf_getline (sb, fp,'\n' ); +return strbuf_getdelim (sb, fp,'\n' ); }int strbuf_getline_nul (struct strbuf *sb,FILE *fp) { -return strbuf_getline (sb, fp,'\0' ); +return strbuf_getdelim (sb, fp,'\0' ); }int strbuf_getwholeline_fd (struct strbuf *sb,int fd,int term)
strbuf.h +14 -11
@@ -354,8 +354,8 @@ extern void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm* * NOTE: The buffer is rewound if the read fails. If -1 is returned, * `errno` must be consulted, like you would do for `read(3)`. -* `strbuf_read()`, `strbuf_read_file()` and `strbuf_getline()` has the -* same behaviour as well. +* `strbuf_read()`, `strbuf_read_file()` and `strbuf_getline_*()` +* family of functions have the same behaviour as well. */ extern size_t strbuf_fread (struct strbuf *,size_t ,FILE *);
@@ -380,26 +380,29 @@ extern int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint);/** * Read a line from a FILE *, overwriting the existing contents -* of the strbuf. The second argument specifies the line -* terminator character, typically `'\n'`. +* of the strbuf. There are three public functions with this +* function signature, with different line termination convention. * Reading stops after the terminator or at EOF. The terminator * is removed from the buffer before returning. Returns 0 unless * there was nothing left before EOF, in which case it returns `EOF`. */ -extern int strbuf_getline (struct strbuf *,FILE *,int ); -typedef int (*strbuf_getline_fn )(struct strbuf *,FILE *); +/* Uses LF as the line terminator */ extern int strbuf_getline_lf (struct strbuf *sb,FILE *fp); + +/* Uses NUL as the line terminator */ extern int strbuf_getline_nul (struct strbuf *sb,FILE *fp); -/* -* Similar to strbuf_getline(), but uses '\n' as the terminator, -* and additionally treats a '\r' that comes immediately before '\n' -* as part of the terminator. +/** +* Similar to strbuf_getline_lf(), but additionally treats +* a '\r' that comes immediately before '\n' as part of the +* terminator. This is the most friendly version to be used +* to read "text" files that can come from platforms whose +* native text format is CRLF terminated. */ -extern int strbuf_getline_crlf (struct strbuf *,FILE *); +extern int strbuf_getline (struct strbuf *,FILE *);/**
transport-helper.c +2 -1
@@ -137,7 +137,8 @@ static struct child_process *get_helper(struct transport *transport) data->no_disconnect_req =0 ;/* -* Open the output as FILE* so strbuf_getline() can be used. +* Open the output as FILE* so strbuf_getline_*() family of +* functions can be used. * Do this with duped fd because fclose() will close the fd, * and stuff like taking over will require the fd to remain. */
Signature · 1 line
2.7.0-250-ge1b5ba3
Re: [PATCH v4 09/21] strbuf: give strbuf_getline() to the "most text friendly" variant
Quoted 12 lines · Junio C Hamano
Now there is no direct caller to
strbuf_getline(), we can demote it to file-scope static private tostrbuf.cimplementation and rename it tostrbuf_getdelim(). Renamestrbuf_getline_crlf(), which is designed to be the most "text friendly" variant, and allow it to take over this simplest name,strbuf_getline(), so we can add more uses of it without having to type_crlfover and over again in the coming steps.Signed-off-by Junio C Hamano <[email protected]>---
strbuf.h extern int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint); · line 384
Quoted 7 lines
strbuf.h +2
-2@@ -380,26 +380,29 @@ extern int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint);/** * Read a line from a FILE *, overwriting the existing contents -* of the strbuf. The second argument specifies the line -* terminator character, typically `'\n'`. +* of the strbuf. There are three public functions with this +* function signature, with different line termination convention.
s/public// perhaps?
Also, is it worth worrying that the comment may become outdated due to the mentioned "three"? Perhaps s/three/several/? Or:
The family of strbuf_getline*() functions share the same signature, but have different line termination conventions.
Quoted 4 lines
strbuf.h +0
-0* Reading stops after the terminator or at EOF. The terminator * is removed from the buffer before returning. Returns 0 unless * there was nothing left before EOF, in which case it returns `EOF`. */
strbuf.h extern int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint); · line 396
Quoted 10 lines
strbuf.h +3
-2-extern int strbuf_getline (struct strbuf *,FILE *,int ); -typedef int (*strbuf_getline_fn )(struct strbuf *,FILE *); +/* Uses LF as the line terminator */ extern int strbuf_getline_lf (struct strbuf *sb,FILE *fp); + +/* Uses NUL as the line terminator */ extern int strbuf_getline_nul (struct strbuf *sb,FILE *fp);
This documentation could have been included in patch 3/21.
Quoted 13 lines
strbuf.h +7
-5-/* -* Similar to strbuf_getline(), but uses '\n' as the terminator, -* and additionally treats a '\r' that comes immediately before '\n' -* as part of the terminator. +/** +* Similar to strbuf_getline_lf(), but additionally treats +* a '\r' that comes immediately before '\n' as part of the +* terminator. This is the most friendly version to be used +* to read "text" files that can come from platforms whose +* native text format is CRLF terminated. */ -extern int strbuf_getline_crlf (struct strbuf *,FILE *); +extern int strbuf_getline (struct strbuf *,FILE *);
Re: [PATCH v4 09/21] strbuf: give strbuf_getline() to the "most text friendly" variant
Quoted 7 lines · Eric Sunshine
Quoted 6 lines
/**
* Read a line from a FILE *, overwriting the existing contents
- * of the strbuf. The second argument specifies the line
- * terminator character, typically'\n'.
+ * of the strbuf. There are three public functions with this
+ * function signature, with different line termination convention.
Quoted 7 lines
s/public// perhaps?
Also, is it worth worrying that the comment may become outdated due to the mentioned "three"? Perhaps s/three/several/? Or:
The family of strbuf_getline*() functions share the same signature, but have different line termination conventions.
I like this one the best, and that is not because I foresee "three" to be changing (strbuf_getdelim() could become public again, but it has a different signature), but because it is concise and to the point.
Quoted 9 lines
Quoted 9 lines
* Reading stops after the terminator or at EOF. The terminator
* is removed from the buffer before returning. Returns 0 unless
* there was nothing left before EOF, in which case it returnsEOF. */strbuf.h +0
-2-extern int strbuf_getline (struct strbuf *,FILE *,int ); -typedef int (*strbuf_getline_fn )(struct strbuf *,FILE *);
Quoted 7 lines
strbuf.h extern int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint); · line 396
Quoted 5 lines
strbuf.h +3
-0+/* Uses LF as the line terminator */ extern int strbuf_getline_lf (struct strbuf *sb,FILE *fp); + +/* Uses NUL as the line terminator */ extern int strbuf_getline_nul (struct strbuf *sb,FILE *fp);This documentation could have been included in patch 3/21.
Perhaps. I'll see what the best tweak would be.
Thanks.
PATCH v4 10/21 hash-object: read --stdin-paths with strbuf_getline()
In reply to Junio C HamanoThe list of paths could have been written with a DOS editor.
--- builtin/hash-object.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
builtin/hash-object.c +1 -1
@@ -60,7 +60,7 @@ static void hash_stdin_paths(const char *type, int no_filters, unsigned flags, {struct strbuf buf =STRBUF_INIT , nbuf =STRBUF_INIT ; -while (strbuf_getline_lf (&buf, stdin) !=EOF ) { +while (strbuf_getline (&buf, stdin) !=EOF ) {if (buf.buf[0 ] =='"' ) {strbuf_reset (&nbuf);if (unquote_c_style (&nbuf, buf.buf,NULL ))
Signature · 1 line
2.7.0-250-ge1b5ba3
Re: [PATCH v4 10/21] hash-object: read --stdin-paths with strbuf_getline()
Quoted 12 lines · Junio C Hamano
The list of paths could have been written with a DOS editor.
Signed-off-by Junio C Hamano <[email protected]>--- builtin/hash-object.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)builtin/hash-object.c +0
-0
builtin/hash-object.c static void hash_stdin_paths(const char *type, int no_filters, unsigned flags, · line 66
Quoted 8 lines
builtin/hash-object.c +1
-1@@ -60,7 +60,7 @@ static void hash_stdin_paths(const char *type, int no_filters, unsigned flags, {struct strbuf buf =STRBUF_INIT , nbuf =STRBUF_INIT ; -while (strbuf_getline_lf (&buf, stdin) !=EOF ) { +while (strbuf_getline (&buf, stdin) !=EOF ) {if (buf.buf[0 ] =='"' ) {strbuf_reset (&nbuf);if (unquote_c_style (&nbuf, buf.buf,NULL ))
The implication here is that the paths cannot have a trailing CR unless they are quoted. I think that is probably OK. We quote such a case ourselves, and while it's _possible_ for somebody to feed us arbitrary output that they generated themselves, I would argue that anybody not quoting CR is generating bogus output.
-Peff
PATCH v4 11/21 revision: read --stdin with strbuf_getline()
In reply to Junio C HamanoReading with getwholeline() and manually stripping the terminating '\n' would leave CR at the end of the line if the input comes from a DOS editor.
Constrasting this with the previous few changes, one may realize that the way "log" family of commands read the paths with --stdin looks inconsistent and sloppy. It does not allow us to C-quote a textual input, and it does not accept NUL-terminated records. These are unfortunately way too late to fix X-<.
--- revision.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-)
revision.c +2 -7
@@ -1635,10 +1635,7 @@ static void append_prune_data(struct cmdline_pathspec *prune, const char **av)static void read_pathspec_from_stdin (struct rev_info *revs,struct strbuf *sb,struct cmdline_pathspec *prune) { -while (strbuf_getwholeline (sb, stdin,'\n' ) !=EOF ) { -int len = sb->len; -if (len && sb->buf[len -1 ] =='\n' ) - sb->buf[--len] ='\0' ; +while (strbuf_getline (sb, stdin) !=EOF ) {ALLOC_GROW (prune->path, prune->nr +1 , prune->alloc); prune->path[prune->nr++] =xstrdup (sb->buf); }
@@ -1655,10 +1652,8 @@ static void read_revisions_from_stdin(struct rev_info *revs, warn_on_object_refname_ambiguity =0 ;strbuf_init (&sb,1000 ); -while (strbuf_getwholeline (&sb, stdin,'\n' ) !=EOF ) { +while (strbuf_getline (&sb, stdin) !=EOF ) {int len = sb.len; -if (len && sb.buf[len -1 ] =='\n' ) - sb.buf[--len] ='\0' ;if (!len)break ;if (sb.buf[0 ] =='-' ) {
Signature · 1 line
2.7.0-250-ge1b5ba3
Re: [PATCH v4 11/21] revision: read --stdin with strbuf_getline()
Quoted 5 lines · Junio C Hamano
Reading with
getwholeline()and manually stripping the terminating '\n' would leave CR at the end of the line if the input comes from a DOS editor.Constrasting this with the previous few changes, one may realize
To what does "Contrasting this with the previous few changes" refer? Did this patch previously come later in the series, or am I missing something obvious?
Quoted 40 lines
that the way "log" family of commands read the paths with --stdin looks inconsistent and sloppy. It does not allow us to C-quote a textual input, and it does not accept NUL-terminated records. These are unfortunately way too late to fix X-<.
Signed-off-by Junio C Hamano <[email protected]>--- revision.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-)revision.c +2
-8@@ -1635,10 +1635,7 @@ static void append_prune_data(struct cmdline_pathspec *prune, const char **av)static void read_pathspec_from_stdin (struct rev_info *revs,struct strbuf *sb,struct cmdline_pathspec *prune) { -while (strbuf_getwholeline (sb, stdin,'\n' ) !=EOF ) { -int len = sb->len; -if (len && sb->buf[len -1 ] =='\n' ) - sb->buf[--len] ='\0' ; +while (strbuf_getline (sb, stdin) !=EOF ) {ALLOC_GROW (prune->path, prune->nr +1 , prune->alloc); prune->path[prune->nr++] =xstrdup (sb->buf); }@@ -1655,10 +1652,8 @@ static void read_revisions_from_stdin(struct rev_info *revs, warn_on_object_refname_ambiguity =0 ;strbuf_init (&sb,1000 ); -while (strbuf_getwholeline (&sb, stdin,'\n' ) !=EOF ) { +while (strbuf_getline (&sb, stdin) !=EOF ) {int len = sb.len; -if (len && sb.buf[len -1 ] =='\n' ) - sb.buf[--len] ='\0' ;if (!len)break ;if (sb.buf[0 ] =='-' ) { --2.7.0-250-ge1b5ba3
Re: [PATCH v4 11/21] revision: read --stdin with strbuf_getline()
Quoted 10 lines · Eric Sunshine
Quoted 5 lines · Junio C Hamano
Reading with
getwholeline()and manually stripping the terminating '\n' would leave CR at the end of the line if the input comes from a DOS editor.Constrasting this with the previous few changes, one may realize
To what does "Contrasting this with the previous few changes" refer? Did this patch previously come later in the series, or am I missing something obvious?
Perhaps that is what happened. It was meant to ask the readers to compare the handling of "--stdin" by the log family of functions (done by code in revision.c here) with other codepaths.
Will reword. Thanks.
Quoted 41 lines
Quoted 41 lines
that the way "log" family of commands read the paths with --stdin looks inconsistent and sloppy. It does not allow us to C-quote a textual input, and it does not accept NUL-terminated records. These are unfortunately way too late to fix X-<.
Signed-off-by Junio C Hamano <[email protected]>--- revision.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-)revision.c +2
-8@@ -1635,10 +1635,7 @@ static void append_prune_data(struct cmdline_pathspec *prune, const char **av)static void read_pathspec_from_stdin (struct rev_info *revs,struct strbuf *sb,struct cmdline_pathspec *prune) { -while (strbuf_getwholeline (sb, stdin,'\n' ) !=EOF ) { -int len = sb->len; -if (len && sb->buf[len -1 ] =='\n' ) - sb->buf[--len] ='\0' ; +while (strbuf_getline (sb, stdin) !=EOF ) {ALLOC_GROW (prune->path, prune->nr +1 , prune->alloc); prune->path[prune->nr++] =xstrdup (sb->buf); }@@ -1655,10 +1652,8 @@ static void read_revisions_from_stdin(struct rev_info *revs, warn_on_object_refname_ambiguity =0 ;strbuf_init (&sb,1000 ); -while (strbuf_getwholeline (&sb, stdin,'\n' ) !=EOF ) { +while (strbuf_getline (&sb, stdin) !=EOF ) {int len = sb.len; -if (len && sb.buf[len -1 ] =='\n' ) - sb.buf[--len] ='\0' ;if (!len)break ;if (sb.buf[0 ] =='-' ) { --2.7.0-250-ge1b5ba3
Re: [PATCH v4 11/21] revision: read --stdin with strbuf_getline()
In reply to Junio C HamanoQuoted 9 lines · Junio C Hamano
Reading with
getwholeline()and manually stripping the terminating '\n' would leave CR at the end of the line if the input comes from a DOS editor.Constrasting this with the previous few changes, one may realize that the way "log" family of commands read the paths with --stdin looks inconsistent and sloppy. It does not allow us to C-quote a textual input, and it does not accept NUL-terminated records. These are unfortunately way too late to fix X-<.
Unlike the previous patch, where we can reasonably expect somebody to quote a bare CR, this actually makes it impossible to ask about "foo\r". Well, maybe not impossible, if we strip only one CR (you could say "foo\r\r"), but certainly a behavior change.
Given the general crappiness of this interface for files with bizarre characters, I think we are much more likely to be doing the user a favor by stripping CRLF than hurting them by breaking "foo\r". After all, "foo\n" remains impossible.
-Peff
PATCH v4 12/21 rev-parse: read parseopt spec with strbuf_getline()
In reply to Junio C Hamano"rev-parse --parseopt" specification is clearly text and we should anticipate that we may be fed CRLF lines.
--- builtin/rev-parse.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
builtin/rev-parse.c +2 -2
@@ -383,7 +383,7 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)/* get the usage up to the first line with a -- on it */ for (;;) { -if (strbuf_getline_lf (&sb, stdin) ==EOF ) +if (strbuf_getline (&sb, stdin) ==EOF )die ("premature end of input" );ALLOC_GROW (usage, unb +1 , usz);if (!strcmp ("--" , sb.buf)) {
@@ -396,7 +396,7 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix) }/* parse: (<short>|<short>,<long>|<long>)[*=?!]*<arghint>? SP+ <help> */ -while (strbuf_getline_lf (&sb, stdin) !=EOF ) { +while (strbuf_getline (&sb, stdin) !=EOF ) {const char *s;const char *help;struct option *o;
Signature · 1 line
2.7.0-250-ge1b5ba3
PATCH v4 13/21 ident.c: read /etc/mailname with strbuf_getline()
In reply to Junio C HamanoJust in case /etc/mailname file was edited with a DOS editor, read it with strbuf_getline() so that a stray CR is not included as the last character of the mail hostname.
We _might_ want to more aggressively discard whitespace characters around the line with strbuf_trim(), but that is a bit outside the scope of this series.
--- ident.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
ident.c +1 -1
@@ -76,7 +76,7 @@ static int add_mailname_host(struct strbuf *buf) strerror(errno));return -1 ; } -if (strbuf_getline_lf (&mailnamebuf, mailname) ==EOF ) { +if (strbuf_getline (&mailnamebuf, mailname) ==EOF ) {if (ferror (mailname))warning ("cannot read /etc/mailname: %s" ,strerror (errno));
Signature · 1 line
2.7.0-250-ge1b5ba3
PATCH v4 14/21 remote.c: read $GIT_DIR/remotes/* with strbuf_getline()
In reply to Junio C HamanoThese files can be edited with a DOS editor, leaving CR at the end of the line if read with strbuf_getline().
--- remote.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
remote.c +1 -1
@@ -256,7 +256,7 @@ static void read_remotes_file(struct remote *remote)if (!f)return ; remote->origin =REMOTE_REMOTES ; -while (strbuf_getline_lf (&buf, f) !=EOF ) { +while (strbuf_getline (&buf, f) !=EOF ) {const char *v;strbuf_rtrim (&buf);
Signature · 1 line
2.7.0-250-ge1b5ba3
PATCH v4 15/21 clone/sha1_file: read info/alternates with strbuf_getline()
In reply to Junio C Hamano$GIT_OBJECT_DIRECTORY/info/alternates is a text file that can be edited with a DOS editor. We do not want to use the real path with CR appeneded at the end.
--- builtin/clone.c | 2 +- sha1_file.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
builtin/clone.c +1 -1
@@ -339,7 +339,7 @@ static void copy_alternates(struct strbuf *src, struct strbuf *dst,FILE *in =fopen (src->buf,"r" );struct strbuf line =STRBUF_INIT ; -while (strbuf_getline_lf (&line, in) !=EOF ) { +while (strbuf_getline (&line, in) !=EOF ) {char *abs_path;if (!line.len || line.buf[0 ] =='#' )continue ;
sha1_file.c +1 -1
@@ -396,7 +396,7 @@ void add_to_alternates_file(const char *reference)struct strbuf line =STRBUF_INIT ;int found =0 ; -while (strbuf_getline_lf (&line, in) !=EOF ) { +while (strbuf_getline (&line, in) !=EOF ) {if (!strcmp (reference, line.buf)) { found =1 ;break ;
Signature · 1 line
2.7.0-250-ge1b5ba3
Re: [PATCH v4 15/21] clone/sha1_file: read info/alternates with strbuf_getline()
Quoted 3 lines · Junio C Hamano
$GIT_OBJECT_DIRECTORY/info/alternates is a text file that can be edited with a DOS editor. We do not want to use the real path with CR appeneded at the end.
s/appeneded/appended/
Quoted 30 lines
Signed-off-by Junio C Hamano <[email protected]>---
builtin/clone.c +1
-1@@ -339,7 +339,7 @@ static void copy_alternates(struct strbuf *src, struct strbuf *dst,FILE *in =fopen (src->buf,"r" );struct strbuf line =STRBUF_INIT ; -while (strbuf_getline_lf (&line, in) !=EOF ) { +while (strbuf_getline (&line, in) !=EOF ) {char *abs_path;if (!line.len || line.buf[0 ] =='#' )continue ;sha1_file.c +1
-2@@ -396,7 +396,7 @@ void add_to_alternates_file(const char *reference)struct strbuf line =STRBUF_INIT ;int found =0 ; -while (strbuf_getline_lf (&line, in) !=EOF ) { +while (strbuf_getline (&line, in) !=EOF ) {if (!strcmp (reference, line.buf)) { found =1 ;break ; --2.7.0-250-ge1b5ba3
PATCH v4 16/21 transport-helper: read helper response with strbuf_getline()
In reply to Junio C HamanoOur implementation of helpers never use CRLF line endings, and they do not depend on the ability to place a CR as payload at the end of the line, so this is essentially a no-op for in-tree users. However, this allows third-party implementation of helpers to give us their line with CRLF line ending (they cannot expect us to feed CRLF to them, though).
--- transport-helper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
transport-helper.c +1 -1
@@ -54,7 +54,7 @@ static int recvline_fh(FILE *helper, struct strbuf *buffer, const char *name)strbuf_reset (buffer);if (debug)fprintf (stderr,"Debug: Remote helper: Waiting...\n" ); -if (strbuf_getline_lf (buffer, helper) ==EOF ) { +if (strbuf_getline (buffer, helper) ==EOF ) {if (debug)fprintf (stderr,"Debug: Remote helper quit.\n" );return 1 ;
Signature · 1 line
2.7.0-250-ge1b5ba3
Re: [PATCH v4 16/21] transport-helper: read helper response with strbuf_getline()
Quoted 6 lines · Junio C Hamano
Our implementation of helpers never use CRLF line endings, and they do not depend on the ability to place a CR as payload at the end of the line, so this is essentially a no-op for in-tree users. However, this allows third-party implementation of helpers to give us their line with CRLF line ending (they cannot expect us to feed CRLF to them, though).
I think this is OK. I cannot think of any transport-helper request or response which wants to pass anything more exotic than a refname as its final argument. And if we were to add one, it would need to take care to quote its argument somehow to allow "\n" anyway.
-Peff
PATCH v4 17/21 cat-file: read batch stream with strbuf_getline()
In reply to Junio C HamanoIt is possible to prepare a text file with a DOS editor and feed it as a batch command stream to the command.
--- builtin/cat-file.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
builtin/cat-file.c +1 -1
@@ -401,7 +401,7 @@ static int batch_objects(struct batch_options *opt) save_warning = warn_on_object_refname_ambiguity; warn_on_object_refname_ambiguity =0 ; -while (strbuf_getline_lf (&buf, stdin) !=EOF ) { +while (strbuf_getline (&buf, stdin) !=EOF ) {if (data.split_on_whitespace) {/* * Split at first whitespace, tying off the beginning
Signature · 1 line
2.7.0-250-ge1b5ba3
Re: [PATCH v4 17/21] cat-file: read batch stream with strbuf_getline()
Quoted 13 lines · Junio C Hamano
It is possible to prepare a text file with a DOS editor and feed it as a batch command stream to the command.
Signed-off-by Junio C Hamano <[email protected]>--- builtin/cat-file.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)builtin/cat-file.c +0
-0
builtin/cat-file.c static int batch_objects(struct batch_options *opt) · line 404
Quoted 5 lines
builtin/cat-file.c +1
-1@@ -401,7 +401,7 @@ static int batch_objects(struct batch_options *opt) save_warning = warn_on_object_refname_ambiguity; warn_on_object_refname_ambiguity =0 ; -while (strbuf_getline_lf (&buf, stdin) !=EOF ) { +while (strbuf_getline (&buf, stdin) !=EOF ) {
This function can actually take a pretty wide variety of input. It can take anything that get_sha1() will handle. So I think that:
printf ':/foo\r\n' | git cat-file --batch
will behave differently (before, we looked for the literal "foo\r" in a commit message). That's sufficiently crazy that I'm OK with giving it up.
A more interesting case is "rev-list" output, combined with "%(rest)". The former does not quote its filenames.
So in a repo like this:
echo bar >$(printf 'foo\r') git add . git commit -m one
you can currently do:
git rev-list --objects --all | git cat-file --batch-check='%(objectsize) %(rest)' | cat -A
and get:
154 $
32 $
4 foo^M$
but with your patch, we lose the CR, and get a filename that is not actually contained in the repo.
I think I'm still in favor of the change, though, for the same reason as some of the earlier ones. We already can't handle "foo\n", so this cannot be considered a completely robust interface. And we are much more likely to do good than harm by stripping such a CR.
-Peff
PATCH v4 18/21 column: read lines with strbuf_getline()
In reply to Junio C HamanoMultiple lines read here are concatenated on a single line to form a multi-column output line. We do not want to have a CR at the end, even if the input file consists of CRLF terminated lines.
--- builtin/column.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
builtin/column.c +1 -1
@@ -51,7 +51,7 @@ int cmd_column(int argc, const char **argv, const char *prefix)die (_ ("--command must be the first argument" )); }finalize_colopts (&colopts,-1 ); -while (!strbuf_getline_lf (&sb, stdin)) +while (!strbuf_getline (&sb, stdin))string_list_append (&list, sb.buf);print_columns (&list, colopts, &copts);
Signature · 1 line
2.7.0-250-ge1b5ba3
Re: [PATCH v4 18/21] column: read lines with strbuf_getline()
Quoted 3 lines · Junio C Hamano
Multiple lines read here are concatenated on a single line to form a multi-column output line. We do not want to have a CR at the end, even if the input file consists of CRLF terminated lines.
Hrm. I suspect this is OK in practice, though "git column" can technically receive unbounded input from the user. We don't specify in the documentation that it must be text, but I guess it's somewhat implied by the use of the word "line".
-Peff
PATCH v4 19/21 send-pack: read list of refs with strbuf_getline()
In reply to Junio C Hamano--- builtin/send-pack.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
builtin/send-pack.c +1 -1
@@ -212,7 +212,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)argv_array_push (&all_refspecs, buf); }else {struct strbuf line =STRBUF_INIT ; -while (strbuf_getline_lf (&line, stdin) !=EOF ) +while (strbuf_getline (&line, stdin) !=EOF )argv_array_push (&all_refspecs, line.buf);strbuf_release (&line); }
Signature · 1 line
2.7.0-250-ge1b5ba3
PATCH v4 20/21 grep: read -f file with strbuf_getline()
In reply to Junio C HamanoList of patterns file could come from a DOS editor.
This is iffy; you may actually be trying to find a line with ^M in it on a system whose line ending is LF. You can of course work it around by having a line that has "^M^M^J", let the strbuf_getline() eat the last "^M^J", leaving just the single "^M" as the pattern.
--- builtin/grep.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
builtin/grep.c +1 -1
@@ -562,7 +562,7 @@ static int file_callback(const struct option *opt, const char *arg, int unset) patterns = from_stdin ? stdin :fopen (arg,"r" );if (!patterns)die_errno (_ ("cannot open '%s'" ), arg); -while (strbuf_getline_lf (&sb, patterns) ==0 ) { +while (strbuf_getline (&sb, patterns) ==0 ) {/* ignore empty line like grep does */ if (sb.len ==0 )continue ;
Signature · 1 line
2.7.0-250-ge1b5ba3
PATCH v4 21/21 test-sha1-array: read command stream with strbuf_getline()
In reply to Junio C HamanoThe input to this command comes from a pipeline in t0064, whose upstream has bunch of "echo"s. It is not unreasonable to expect that it may be fed CRLF lines on DOSsy systems.
--- test-sha1-array.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
test-sha1-array.c +1 -1
@@ -11,7 +11,7 @@ int main(int argc, char **argv)struct sha1_array array =SHA1_ARRAY_INIT ;struct strbuf line =STRBUF_INIT ; -while (strbuf_getline_lf (&line, stdin) !=EOF ) { +while (strbuf_getline (&line, stdin) !=EOF ) {const char *arg;unsigned char sha1[20 ];
Signature · 1 line
2.7.0-250-ge1b5ba3
Re: [PATCH v4 00/21] Peace with CRLF
In reply to Junio C HamanoQuoted 4 lines · Junio C Hamano
- The update to mktree used "
lf_lines" in the preview, but this version flips it to the same "nul_term_line" to match other two, even though I think the latter is a terrible variable name with bad taste.
Heh. I also dislike that variable name, but not want to nit-pick when reading the preview.
I think just "use_nul_terminator" or something would be fine. It is really the "line" which bugs me, because they are not really lines anymore.
-Peff
Re: [PATCH v4 00/21] Peace with CRLF
In reply to Junio C HamanoQuoted 8 lines · Junio C Hamano
This is a reroll of
http://thread.gmane.org/gmane.comp.version-control.git/282614
which is a continuation to $gmane/275735, which is filed in the archive under another mailing list:
http://thread.gmane.org/gmane.comp.version-control.msysgit/21773
I just read through the whole thing and made a few comments. Please feel free to ignore my "how about this on top" suggestions if you don't like them, or just don't want to deal with them. Patches I didn't respond to all looked good to me.
There are a few "iffy" conversions where we take unconstrained input and there's no "usual" quoting mechanism available (you mentioned the one in "grep", and I pointed out a few more). I hate to break backwards-compatibility in any way, but I think in all of those cases that we are much more likely to be helping the user than hurting them.
So I think it's a net benefit in all of the cases.
-Peff
Re: [PREVIEW v3 0/9] Preview of "Peace with CRLF" rerolled
In reply to Junio C HamanoQuoted 9 lines · Junio C Hamano
Continued from
http://thread.gmane.org/gmane.comp.version-control.git/282614
Here is only the preparatory part that
- introduces
strbuf_getline_lf()andstrbuf_getine_nul()and converts existing callers tostrbut_getline()to use them;
Quoted 6 lines
- retires
strbuf_getline()that takes an arbitrary line terminator and demotes it tostrbuf_getdelim()that is merely an implementation detail insidestrbuf.c; and- gives the short-and-sweet name
strbuf_getline()to the most "text friendly"strbuf_getline_crlf().
With the exception of a few minor nits (some of which are even just cleanups we _could_ do while in the area), this looks like a good approach to me.
I'm happy that the more-immediate endgame will keep many callers with the short "strbuf_getline" (and that its invocation is now even shorter and more readable than before, with the '\n' noise gone).
-Peff