On Tue, Feb 07, 2023 at 04:56:42PM +1100, Mark Jamsek wrote:
> On 23-02-06 11:25PM, Stefan Sperling wrote:
> > On Mon, Feb 06, 2023 at 09:41:18AM +0100, Omar Polo wrote:
> > > maybe for the commit/cherrypick case we could count the initial
> > > "comment" line as part of the commit message?  it'd still be possible
> > > to abort the operation and the message preserved if really desired to.
> > > just wondering.
> > 
> > You are suggesting that we treat the log message as modified as soon as
> > the user deletes one or more # comments that we put into the initial
> > template? This sounds like a reasonable idea.
> > 
> > For the 'commit' case, if someone is editing only # comments, then
> > the commit should still error out because of an empty commit message,
> > once the comments get stripped.
> > 
> > And it is true that reusing a log message as-is will be common practice
> > in the cherrpick case in particular. Not so sure about backout. But the
> > rules should be the same for both commands, otherwise it gets too confusing.
> > Ideally, we'd have one set of rules that works for commit/cherrypick/etc.
> > And it seems like what you are suggesting should work.
> 
> The below diff achieves this with a minimal change by comparing the
> edited log message len with the initial content len, and allowing the
> commit if the former is less than the latter; therefore, as soon as the
> user removes one of the commented lines, the message is viable even
> though the message is the same as the initial (stripped) content.
> > We can't just proceed if logmsg_len != initial_content_len because, as > you mentioned, the user may add chars to the commented text, which will
> be stripped and thus shouldn't count as a valid message edit.
> 
> Is this behaviour what you had in mind?
> 
> diff /home/mark/src/got
> commit - c9003c52fc7f70dd988402560adcd22709e74800
> path + /home/mark/src/got
> blob - a32137a402ce476d4300983fe5e9e17d680c8ac0
> file + got/got.c
> --- got/got.c
> +++ got/got.c
> @@ -486,7 +486,7 @@ edit_logmsg(char **logmsg, const char *editor, const c
>  		    "commit message cannot be empty, aborting");
>  		goto done;
>  	}
> -	if (require_modification &&
> +	if (require_modification && logmsg_len >= initial_content_len &&
>  	    strcmp(*logmsg, initial_content_stripped) == 0)
>  		err = got_error_msg(GOT_ERR_COMMIT_MSG_EMPTY,
>  		    "no changes made to commit message, aborting");

This approach is a little too suttle for my taste. For someone
looking at this code years from now this will not be very obvious.

I would suggest the approach implemente by the patch below.
Granted, this patch adds extra work. But log message should not be huge
in the first place, so this is still cheap.

-----------------------------------------------
 make edits made to comments count as a log message modification
 
 This supports use of cherrypick/backout without requiring the user
 to modify the log message of the original commit.
 
diff ae0cca99d4999f67c98e04bf1f806bd1147d9721 de8292f27338f3e98bca952fb7451aaeb32a79eb
commit - ae0cca99d4999f67c98e04bf1f806bd1147d9721
commit + de8292f27338f3e98bca952fb7451aaeb32a79eb
blob - 518e73d32dd54163f0c184e0373f963ae473079b
blob + 6909ba95d7617d65c7fcad655d3baa1f1543340a
--- got/got.1
+++ got/got.1
@@ -2038,8 +2038,13 @@ where the messages must be further adjusted to convey 
 is committed with
 .Cm got commit ,
 the log messages of relevant merged commits will then appear in the editor,
-where the messages must be further adjusted to convey the reasons for
+where the messages should be further adjusted to convey the reasons for
 cherrypicking the changes.
+At the very least, comment lines must be removed.
+Otherwise
+.Cm got commit
+will fail with an unmodified log message error.
+.Pp
 If all the changes in all files touched by a given commit are discarded,
 e.g. with
 .Cm got revert ,
@@ -2141,8 +2146,13 @@ the editor, where the messages must be further adjuste
 is committed with
 .Cm got commit ,
 the log messages of relevant reverse-merged commits will then appear in
-the editor, where the messages must be further adjusted to convey the
+the editor, where the messages should be further adjusted to convey the
 reasons for backing out the changes.
+At the very least, comment lines must be removed.
+Otherwise
+.Cm got commit
+will fail with an unmodified log message error.
+.Pp
 If all the changes in all files touched by a given commit are discarded,
 e.g. with
 .Cm got revert ,
blob - e7d51531acf964d92ac89f71868de68018e76ac8
blob + e4bb98750bf47f68c270846e694ff0d461408b05
--- got/got.c
+++ got/got.c
@@ -389,13 +389,60 @@ edit_logmsg(char **logmsg, const char *editor, const c
 }
 
 static const struct got_error *
+read_logmsg(char **logmsg, size_t *len, FILE *fp, size_t filesize,
+    int strip_comments)
+{
+	const struct got_error *err = NULL;
+	char *line = NULL;
+	size_t linesize = 0;
+
+	*logmsg = NULL;
+	*len = 0;
+
+	if (fseeko(fp, 0L, SEEK_SET) == -1)
+		return got_error_from_errno("fseeko");
+
+	*logmsg = malloc(filesize + 1);
+	if (*logmsg == NULL)
+		return got_error_from_errno("malloc");
+	(*logmsg)[0] = '\0';
+
+	while (getline(&line, &linesize, fp) != -1) {
+		if ((strip_comments && line[0] == '#') ||
+		    (*len == 0 && line[0] == '\n'))
+			continue; /* remove comments and leading empty lines */
+		*len = strlcat(*logmsg, line, filesize + 1);
+		if (*len >= filesize + 1) {
+			err = got_error(GOT_ERR_NO_SPACE);
+			goto done;
+		}
+	}
+	if (ferror(fp)) {
+		err = got_ferror(fp, GOT_ERR_IO);
+		goto done;
+	}
+
+	while (*len > 0 && (*logmsg)[*len - 1] == '\n') {
+		(*logmsg)[*len - 1] = '\0';
+		(*len)--;
+	}
+done:
+	free(line);
+	if (err) {
+		free(*logmsg);
+		*logmsg = NULL;
+		*len = 0;
+	}
+	return err;
+}
+
+static const struct got_error *
 edit_logmsg(char **logmsg, const char *editor, const char *logmsg_path,
     const char *initial_content, size_t initial_content_len,
     int require_modification)
 {
 	const struct got_error *err = NULL;
 	char *line = NULL;
-	size_t linesize = 0;
 	struct stat st, st2;
 	FILE *fp = NULL;
 	size_t len, logmsg_len;
@@ -435,8 +482,8 @@ edit_logmsg(char **logmsg, const char *editor, const c
 	s = buf;
 	len = 0;
 	while ((line = strsep(&s, "\n")) != NULL) {
-		if ((line[0] == '#' || (len == 0 && line[0] == '\n')))
-			continue; /* remove comments and leading empty lines */
+		if (len == 0 && line[0] == '\n')
+			continue; /* remove leading empty lines */
 		len = strlcat(initial_content_stripped, line,
 		    initial_content_len + 1);
 		if (len >= initial_content_len + 1) {
@@ -449,47 +496,35 @@ edit_logmsg(char **logmsg, const char *editor, const c
 		len--;
 	}
 
-	logmsg_len = st2.st_size;
-	*logmsg = malloc(logmsg_len + 1);
-	if (*logmsg == NULL)
-		return got_error_from_errno("malloc");
-	(*logmsg)[0] = '\0';
-
 	fp = fopen(logmsg_path, "re");
 	if (fp == NULL) {
 		err = got_error_from_errno("fopen");
 		goto done;
 	}
 
-	len = 0;
-	while (getline(&line, &linesize, fp) != -1) {
-		if ((line[0] == '#' || (len == 0 && line[0] == '\n')))
-			continue; /* remove comments and leading empty lines */
-		len = strlcat(*logmsg, line, logmsg_len + 1);
-		if (len >= logmsg_len + 1) {
-			err = got_error(GOT_ERR_NO_SPACE);
-			goto done;
-		}
-	}
-	free(line);
-	if (ferror(fp)) {
-		err = got_ferror(fp, GOT_ERR_IO);
+	/*
+	 * Check whether the log message was modified.
+	 * Editing or removal of comments does count as a modifcation to
+	 * support reuse of existing log messages during cherrypick/backout.
+	 */
+	err = read_logmsg(logmsg, &logmsg_len, fp, st2.st_size, 0);
+	if (err)
 		goto done;
-	}
-	while (len > 0 && (*logmsg)[len - 1] == '\n') {
-		(*logmsg)[len - 1] = '\0';
-		len--;
-	}
-
-	if (len == 0) {
-		err = got_error_msg(GOT_ERR_COMMIT_MSG_EMPTY,
-		    "commit message cannot be empty, aborting");
-		goto done;
-	}
 	if (require_modification &&
 	    strcmp(*logmsg, initial_content_stripped) == 0)
 		err = got_error_msg(GOT_ERR_COMMIT_MSG_EMPTY,
 		    "no changes made to commit message, aborting");
+
+	/* Read log message again, stripping comments this time around. */
+	free(*logmsg);
+	err = read_logmsg(logmsg, &logmsg_len, fp, st2.st_size, 1);
+	if (err)
+		goto done;
+	if (logmsg_len == 0) {
+		err = got_error_msg(GOT_ERR_COMMIT_MSG_EMPTY,
+		    "commit message cannot be empty, aborting");
+		goto done;
+	}
 done:
 	free(initial_content_stripped);
 	free(buf);