From: Sebastien Marie <semarie@online.fr>
Subject: Re: patch: generic commit header management in 'got log'
To: gameoftrees@openbsd.org
Date: Sat, 28 Sep 2019 13:44:18 +0200

On Sat, Sep 28, 2019 at 12:10:14PM +0200, Stefan Sperling wrote:
> On Sat, Sep 28, 2019 at 10:43:41AM +0200, Sebastien Marie wrote:
> > Now, for got(1), I think by default "got log" should show only specific headers
> > (date, author, commiter...), and discard any other headers. Only whitelisted
> > headers would be printed. It would be a generic way to deal with gpgsig header too.
> > 
> > With "got cat" we have the 'raw' format output, and all headers are showed.
> > 
> > The following diff implements that. Any lines before first empty line is an
> > header, and only whitelisted headers are showed by default. All next lines are
> > showed (it is the commit message). This way explicit support for removing gpgsig
> > from log could be discarded.
> > 
> > Comments or OK ?
> 
> Thanks, I am so happy to see this horrible GPG stripping code go away!
> 
> I was not aware of the "header" and "body" distinction git is making.
> This effectively means that author information is considered part of log
> message "headers" in a technical sense, right? That's kind of weird.

yes. but you could also see "headers" as formatted metadatas on the commit.

> > +		if (headers == 1) {
> > +			if (line[0] != '\0' &&
> > +			    strncmp(line, "tree ", 5) != 0 &&
> > +			    strncmp(line, "author ", 7) != 0 &&
> > +			    strncmp(line, "parent ", 7) != 0 &&
> > +			    strncmp(line, "committer ", 10) != 0 &&
> 
> We have macros for the above in include/got_object.h:
> 
> #define GOT_COMMIT_LABEL_TREE		"tree "
> #define GOT_COMMIT_LABEL_PARENT		"parent "
> #define GOT_COMMIT_LABEL_AUTHOR		"author "
> #define GOT_COMMIT_LABEL_COMMITTER	"committer "
> 
> Could we use those? I realize it is longer to write:
> 
> 	strncmp(line, GOT_COMMIT_LABEL_TREE,
> 	    strlen(GOT_COMMIT_LABEL_TREE)) != 0 &&
> 
> But I prefer to use macros for markers in data that's being parsed.

it is fine.
 
> > +			    strncmp(line, "numparents ", 11) != 0 &&
> > +			    strncmp(line, "messagelen ", 11) != 0) {
> > +
> >  				continue;
> 
> "numparents" and "messagelent" don't appear in the actual object.
> They are specific to 'got cat' output, to help scripts with parsing.
> So the above two checks should not be needed.

ah ok :)

New diff below.
-- 
Sebastien Marie

diff 500467ff1bf0dbd15c0941dd741e80c35c708818 /home/semarie/repos/openbsd/got
blob - b6cd712c154536cdfbd3c4dec1d9f014e4630378
file + lib/object_parse.c
--- lib/object_parse.c
+++ lib/object_parse.c
@@ -419,16 +419,13 @@ got_object_commit_get_committer_gmtoff(struct got_comm
 	return commit->committer_gmtoff;
 }
 
-#define GOT_GPG_BEGIN_STR "gpgsig -----BEGIN PGP SIGNATURE-----"
-#define GOT_GPG_END_STR " -----END PGP SIGNATURE-----"
-
 const struct got_error *
 got_object_commit_get_logmsg(char **logmsg, struct got_commit_object *commit)
 {
 	const struct got_error *err = NULL;
-	int gpgsig = 0;
 	char *msg0, *msg, *line, *s;
 	size_t len;
+	int headers = 1;
 
 	*logmsg = NULL;
 
@@ -436,32 +433,36 @@ got_object_commit_get_logmsg(char **logmsg, struct got
 	if (msg0 == NULL)
 		return got_error_from_errno("strdup");
 
-	/* Copy log message line by line to strip out GPG sigs... */
+	/* Copy log message line by line to strip out unusual headers... */
 	msg = msg0;
 	do {
-		line = strsep(&msg, "\n");
+		if ((line = strsep(&msg, "\n")) == NULL)
+			break;
 
-		if (line) {
-			/* Skip over GPG signatures. */
-			if (gpgsig) {
-				if (strcmp(line, GOT_GPG_END_STR) == 0) {
-					gpgsig = 0;
-					/* Skip empty line after sig. */
-					line = strsep(&msg, "\n");
-				}
+		if (headers == 1) {
+			if (line[0] != '\0' &&
+			    strncmp(line, GOT_COMMIT_LABEL_TREE,
+			        strlen(GOT_COMMIT_LABEL_TREE)) != 0 &&
+			    strncmp(line, GOT_COMMIT_LABEL_AUTHOR,
+			        strlen(GOT_COMMIT_LABEL_AUTHOR)) != 0 &&
+			    strncmp(line, GOT_COMMIT_LABEL_PARENT,
+			        strlen(GOT_COMMIT_LABEL_PARENT)) != 0 &&
+			    strncmp(line, GOT_COMMIT_LABEL_COMMITTER,
+			        strlen(GOT_COMMIT_LABEL_COMMITTER)) != 0)
 				continue;
-			} else if (strcmp(line, GOT_GPG_BEGIN_STR) == 0) {
-				gpgsig = 1;
-				continue;
-			}
-			if (asprintf(&s, "%s%s\n",
-			    *logmsg ? *logmsg : "", line) == -1) {
-				err = got_error_from_errno("asprintf");
-				goto done;
-			}
-			free(*logmsg);
-			*logmsg = s;
+
+			if (line[0] == '\0')
+				headers = 0;
 		}
+
+		if (asprintf(&s, "%s%s\n",
+		    *logmsg ? *logmsg : "", line) == -1) {
+			err = got_error_from_errno("asprintf");
+			goto done;
+		}
+		free(*logmsg);
+		*logmsg = s;
+
 	} while (line);
 
 	/* Trim redundant trailing whitespace. */