From: Mark Jamsek Subject: Re: abort commit if merged log messages are left as-is To: Christian Weisgerber , gameoftrees@openbsd.org Date: Fri, 10 Feb 2023 13:05:27 +1100 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 GPG: F2FF 13DE 6A06 C471 CA80 E6E2 2930 DC66 86EE CF68