On 23-02-09 10:13PM, Stefan Sperling wrote:
> On Thu, Feb 09, 2023 at 10:03:37PM +0100, Christian Weisgerber wrote:
> > Stefan Sperling:
> > 
> > > > Oh, the log message must be actually modified?  Can't you just
> > > > compare mtime before/after,
> > > 
> > > There would be no way to reverse your decision to commit once the
> > > timestamp has been changed. But I guess that is fine.
> > 
> > The commit is aborted if
> > * the time stamp of the log message hasn't changed, or
> > * the log message is empty.
> > 
> > Is that practical?
> 
> Yes, I believe so.
> In particular because this behaviour is easy to explain and understand.
> And, of course, "empty" means: empty with comments removed

Yes, I realised after my initial review that the change considers
commits where the prefilled comments are edited (but the # remains) as
valid.  I didn't realise this was intended because of my interpretation
of the earlier remark:

> 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.

However, I actually prefer this because it means we can just keep the
cherrypicked log message verbatim. With that in mind, I thought much the
same as naddy insofar as can we now just compare mtimes and discard the
other comparison logic as the outcome should be the same?

diffstat /home/mark/src/got
 M  got/got.1  |  6+   6-
 M  got/got.c  |  3+  54-

2 files changed, 9 insertions(+), 60 deletions(-)

diff /home/mark/src/got
commit - 3b8fb9520f899be7cb492c652afa0cdce28e350e
path + /home/mark/src/got
blob - 6909ba95d7617d65c7fcad655d3baa1f1543340a
file + got/got.1
--- got/got.1
+++ got/got.1
@@ -2040,10 +2040,10 @@ At the very least, comment lines must be removed.
 the log messages of relevant merged commits will then appear in the editor,
 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
+Upon exiting the editor, if the time stamp of the log message file
+is unchanged or the log message is empty,
 .Cm got commit
-will fail with an unmodified log message error.
+will fail with an unmodified or empty log message error.
 .Pp
 If all the changes in all files touched by a given commit are discarded,
 e.g. with
@@ -2148,10 +2148,10 @@ At the very least, comment lines must be removed.
 the log messages of relevant reverse-merged commits will then appear in
 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
+Upon exiting the editor, if the time stamp of the log message file
+is unchanged or the log message is empty,
 .Cm got commit
-will fail with an unmodified log message error.
+will fail with an unmodified or empty log message error.
 .Pp
 If all the changes in all files touched by a given commit are discarded,
 e.g. with
blob - e4bb98750bf47f68c270846e694ff0d461408b05
file + got/got.c
--- got/got.c
+++ got/got.c
@@ -442,11 +442,9 @@ edit_logmsg(char **logmsg, const char *editor, const c
     int require_modification)
 {
 	const struct got_error *err = NULL;
-	char *line = NULL;
 	struct stat st, st2;
 	FILE *fp = NULL;
-	size_t len, logmsg_len;
-	char *initial_content_stripped = NULL, *buf = NULL, *s;
+	size_t logmsg_len;
 
 	*logmsg = NULL;
 
@@ -459,64 +457,17 @@ edit_logmsg(char **logmsg, const char *editor, const c
 	if (stat(logmsg_path, &st2) == -1)
 		return got_error_from_errno("stat");
 
-	if (require_modification &&
-	    st.st_mtime == st2.st_mtime && st.st_size == st2.st_size)
+	if (require_modification && st.st_mtime == st2.st_mtime)
 		return got_error_msg(GOT_ERR_COMMIT_MSG_EMPTY,
 		    "no changes made to commit message, aborting");
 
-	/*
-	 * Set up a stripped version of the initial content without comments
-	 * and blank lines. We need this in order to check if the message
-	 * has in fact been edited.
-	 */
-	initial_content_stripped = malloc(initial_content_len + 1);
-	if (initial_content_stripped == NULL)
-		return got_error_from_errno("malloc");
-	initial_content_stripped[0] = '\0';
-
-	buf = strdup(initial_content);
-	if (buf == NULL) {
-		err = got_error_from_errno("strdup");
-		goto done;
-	}
-	s = buf;
-	len = 0;
-	while ((line = strsep(&s, "\n")) != NULL) {
-		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) {
-			err = got_error(GOT_ERR_NO_SPACE);
-			goto done;
-		}
-	}
-	while (len > 0 && initial_content_stripped[len - 1] == '\n') {
-		initial_content_stripped[len - 1] = '\0';
-		len--;
-	}
-
 	fp = fopen(logmsg_path, "re");
 	if (fp == NULL) {
 		err = got_error_from_errno("fopen");
 		goto done;
 	}
 
-	/*
-	 * 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;
-	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);
+	/* strip comments and leading/trailing newlines */
 	err = read_logmsg(logmsg, &logmsg_len, fp, st2.st_size, 1);
 	if (err)
 		goto done;
@@ -526,8 +477,6 @@ done:
 		goto done;
 	}
 done:
-	free(initial_content_stripped);
-	free(buf);
 	if (fp && fclose(fp) == EOF && err == NULL)
 		err = got_error_from_errno("fclose");
 	if (err) {

-- 
Mark Jamsek <fnc.bsdbox.org|got.bsdbox.org>
GPG: F2FF 13DE 6A06 C471 CA80  E6E2 2930 DC66 86EE CF68