From: Stefan Sperling Subject: Re: abort commit if merged log messages are left as-is To: Mark Jamsek Cc: Omar Polo , gameoftrees@openbsd.org Date: Thu, 9 Feb 2023 13:59:21 +0100 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);