From: Mark Jamsek Subject: Re: abort commit if merged log messages are left as-is To: Omar Polo , gameoftrees@openbsd.org Date: Fri, 10 Feb 2023 00:23:48 +1100 On 23-02-09 01:59PM, Stefan Sperling wrote: > 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. Tbh, I wasn't too sure if I'd understood the desired behaviour so I really wanted to see if I was on the right track; but you're right, it should at the very least be commented else I'd have no idea why that condition was there even a month from now--let alone a year :) That said, I'm glad to see I did indeed get the behaviour right with a one liner! > 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. Yes, it provides the same behaviour, matching what I think op was after, and also reads good to me. Thanks! ok for me > ----------------------------------------------- > 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); > -- Mark Jamsek GPG: F2FF 13DE 6A06 C471 CA80 E6E2 2930 DC66 86EE CF68