Re: [PATCH] ref-filter: treat CRLF as same as LF in find_subpos
From: Junio C Hamano <hidden>
Date: 2017-05-23 03:01:48
Subsystem:
the rest · Maintainer:
Linus Torvalds
Possibly related (same subject, not in this thread)
- 2017-05-23 · Re: [PATCH] ref-filter: treat CRLF as same as LF in find_subpos · Junio C Hamano <hidden>
- 2017-05-23 · Re: [PATCH] ref-filter: treat CRLF as same as LF in find_subpos · Junio C Hamano <hidden>
- 2017-05-22 · Re: [PATCH] ref-filter: treat CRLF as same as LF in find_subpos · Jeff King <hidden>
- 2017-05-22 · Re: [PATCH] ref-filter: treat CRLF as same as LF in find_subpos · DOAN Tran Cong Danh <hidden>
- 2017-05-22 · Re: [PATCH] ref-filter: treat CRLF as same as LF in find_subpos · Junio C Hamano <hidden>
Junio C Hamano [off-list ref] writes:
Jeff King [off-list ref] writes:quoted
I suspect that's more work because we'd need to refactor pretty.c a bit to make the right functionality available. But the end result would be much more maintainable.I actually think the entire codeflow of "find positions and length of threeparts" using find_subpos() and then "copy the length bytes starting position for C_{SUB,BODY,SIG,LINES,...}" must be rethought, if the behavior of pretty.c::pretty_print_commit() is to be matched. With the current code, %(contents:body) and other atoms that are handled in ref-filter.c::grab_sub_body_contents() keep trailing whitespaces on their lines with the current code that copies length bytes starting the position using xmemdupz(). There need to be some code that loses these trailing whiltespaces in the copied result. While I do not claim that refactoring and reusing code from pretty.c is the only viable way forward, it is clear to me that a patch that updates find_subpos() and changes nothing else falls short X-<.
I wonder if this would be a viable alternative (this is just a smoking-break hack without an attempt to think through corner cases---for example we need to make sure we work sensibly when the object does not have _any_ body past the header, but I do not think the original works well in that case, either). ref-filter.c | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-)
diff --git a/ref-filter.c b/ref-filter.c
index 1fc5e9970d..10f8fe15f5 100644
--- a/ref-filter.c
+++ b/ref-filter.c@@ -949,13 +949,7 @@ static void find_subpos(const char *buf, unsigned long sz, const char **sig, unsigned long *siglen) { const char *eol; - /* skip past header until we hit empty line */ - while (*buf && *buf != '\n') { - eol = strchrnul(buf, '\n'); - if (*eol) - eol++; - buf = eol; - } + /* skip any empty lines */ while (*buf == '\n') buf++;
@@ -1011,10 +1005,11 @@ static void append_lines(struct strbuf *out, const char *buf, unsigned long size } /* See grab_values */ -static void grab_sub_body_contents(struct atom_value *val, int deref, struct object *obj, void *buf, unsigned long sz) +static void grab_sub_body_contents(struct atom_value *val, int deref, struct object *obj, void *rawbuf, unsigned long sz) { int i; const char *subpos = NULL, *bodypos = NULL, *sigpos = NULL; + struct strbuf buf = STRBUF_INIT; unsigned long sublen = 0, bodylen = 0, nonsiglen = 0, siglen = 0; for (i = 0; i < used_atom_cnt; i++) {
@@ -1030,11 +1025,18 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj strcmp(name, "trailers") && !starts_with(name, "contents")) continue; - if (!subpos) - find_subpos(buf, sz, + if (!subpos) { + char *eoh = memmem(rawbuf, sz, "\n\n", 2); + eoh += 2; + sz -= eoh - (char *)rawbuf; + rawbuf = eoh; + strbuf_add(&buf, rawbuf, sz); + strbuf_stripspace(&buf, 0); + find_subpos(buf.buf, sz, &subpos, &sublen, &bodypos, &bodylen, &nonsiglen, &sigpos, &siglen); + } if (atom->u.contents.option == C_SUB) v->s = copy_subject(subpos, sublen);
@@ -1060,8 +1062,9 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj info.trailer_end - info.trailer_start); trailer_info_release(&info); } else if (atom->u.contents.option == C_BARE) - v->s = xstrdup(subpos); + v->s = xmemdupz(rawbuf, sz); } + strbuf_release(&buf); } /*