Re: [PATCH] ref-filter: treat CRLF as same as LF in find_subpos

From: Junio C Hamano <gitster@pobox.com>
Date: May 23, 2017, 03:01 UTC
Message-ID: <xmqqy3tomg2y.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <xmqq37bwnxg4.fsf@gitster.mtv.corp.google.com>

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

Show 19 quoted lines

> Jeff King <peff@peff.net> writes:
>
>> 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);
 }
 
 /*