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 <fnc.bsdbox.org|got.bsdbox.org>
GPG: F2FF 13DE 6A06 C471 CA80  E6E2 2930 DC66 86EE CF68