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, ==)

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