On 23-02-10 09:49AM, Stefan Sperling wrote:
> On Fri, Feb 10, 2023 at 01:05:27PM +1100, Mark Jamsek wrote:
> > 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(-)
> 
> Yes, thanks. This looks better than we have now.
> 
> Just one concern:
> 
> > @@ -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)
> 
> The old code used mtime as a quick optimization to skip content comparison.
> Now we are relying on mtime alone.
> 
> To accommodate fast typists we will want nanosecond precision now.
> Otherwise, when someone writes the file and exits their editor within
> one second we would abort the commit regardless of the file having
> been written.
> 
> Instead of checking the deprecated st_mtime field, we should check:
> 
> 	st.st_mtim.tv_sec == st2.st_mtim.tv_sec &&
> 	st.st_mtim.tv_nsec == st2.st_mtim.tv_nsec
> 
> Or the equivalent:
> 
> 	timespeccmp(&st.st_mtim, &st2.st_mtim, ==)

Ho, that's much better! Thank you :)

Below diff is the same except with your improvement:

-----------------------------------------------
commit 50da8693beac27440a7df88952690b12d422bb13 (main)
from: Mark Jamsek <mark@jamsek.dev>
date: Fri Feb 10 09:55:50 2023 UTC
 
 got: use timestamp and emptiness to validate log message
 
 As suggested by naddy: consider commit log messages valid provided the temp
 file time stamp has changed and the file is not empty. This heuristic
 provides the desired behaviour (i.e., reusing cherrypicked/backed-out log
 messages) that's currently provided but is simpler to grok.
 
 Improved by stsp.
 
 M  got/got.1  |  6+   6-
 M  got/got.c  |  3+  54-

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

diff 3b8fb9520f899be7cb492c652afa0cdce28e350e 50da8693beac27440a7df88952690b12d422bb13
commit - 3b8fb9520f899be7cb492c652afa0cdce28e350e
commit + 50da8693beac27440a7df88952690b12d422bb13
blob - 6909ba95d7617d65c7fcad655d3baa1f1543340a
blob + d3ce58fd68ab0a1ef9be75ee3b113fb0080db8db
--- 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
blob + 4a0cd57f859bca5045a0003d40445f08adc26657
--- 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 && timespeccmp(&st.st_mtim, &st2.st_mtim, ==))
 		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