On Sun, Feb 23, 2020 at 04:29:18PM +0100, Stefan Sperling wrote:
> As is customary in OpenBSD we have been reviewing diffs on this list and
> handing out OKs. However most commit messages in the got repo do not carry
> this information, and that is bad.
> 
> Adding "OK someone" to log messages is already possible with 'got histedit',
> but it is not as straightforward to use as e.g. git's 'commit --amend'.
> 
> With this new feature it becomes much easier and I think it is better than
> git's concept because it allows adding OKs to all local commits, with one
> command, before pushing them out.
> 
> All that's needed is backdating the work tree to the last common commit
> (which will usually be origin/master) starting histedit in -m mode:
> 
> 	got up -c origin/master
> 	got he -m
> 
> Does this make sense? Any suggestions or concerns?

Yes, I have wanted this feature in the past. Ok. See one comment below.

> 
> diff dc990cbf2d39c406ff23bc36e6cbb822f747ee42 /home/stsp/src/got
> blob - bfd42f09a78d05ccbb7abf0aed72286753e1ca73
> file + got/got.1
> --- got/got.1
> +++ got/got.1
> @@ -988,13 +988,17 @@ If this option is used, no other command-line argument
>  .It Cm rb
>  Short alias for
>  .Cm rebase .
> -.It Cm histedit Oo Fl a Oc Oo Fl c Oc Op Fl F Ar histedit-script
> +.It Cm histedit Oo Fl a Oc Oo Fl c Oc Oo Fl F Ar histedit-script Oc Oo Fl m Oc
>  Edit commit history between the work tree's current base commit and
>  the tip commit of the work tree's current branch.
>  .Pp
>  Editing of commit history is controlled via a
>  .Ar histedit script
> -which can be edited interactively or passed on the command line.
> +which can be edited interactively, passed on the command line,
> +or generated with the
> +.Fl m
> +option if only log messages need to be edited.
> +.Pp
>  The format of the histedit script is line-based.
>  Each line in the script begins with a command name, followed by
>  whitespace and an argument.
> @@ -1088,6 +1092,14 @@ If this option is used, no other command-line argument
>  .It Fl c
>  Continue an interrupted histedit operation.
>  If this option is used, no other command-line arguments are allowed.
> +.It Fl m
> +Edit log messages only.
> +This option is a quick equivalent to a histedit script which edits
> +only log messages but otherwise uses every commit as-is.

Should this be "leaves every commit as-is," or am I misunderstanding?

> +The
> +.Fl m
> +option can only be used when starting a new histedit operation.
> +If this option is used, no other command-line arguments are allowed.
>  .El
>  .It Cm he
>  Short alias for
> @@ -1567,6 +1579,15 @@ This will also merge local changes, if any, with the i
>  .Pp
>  .Dl $ got update -b origin/master
>  .Dl $ got rebase master
> +.Pp
> +On the
> +.Dq master
> +branch, log messages for local changes can now be amended with
> +.Dq OK
> +by other developers and any other important new information:
> +.Pp
> +.Dl $ got update -c origin/master
> +.Dl $ got histedit -m
>  .Pp
>  Local changes on the
>  .Dq master
> blob - 4a5d5fcafa19f89e6039addb90f3c1979ea584e3
> file + got/got.c
> --- got/got.c
> +++ got/got.c
> @@ -5561,7 +5561,7 @@ done:
>  __dead static void
>  usage_histedit(void)
>  {
> -	fprintf(stderr, "usage: %s histedit [-a] [-c] [-F histedit-script]\n",
> +	fprintf(stderr, "usage: %s histedit [-a] [-c] [-F histedit-script] [-m]\n",
>  	    getprogname());
>  	exit(1);
>  }
> @@ -5627,8 +5627,8 @@ done:
>  }
>  
>  static const struct got_error *
> -histedit_write_commit_list(struct got_object_id_queue *commits, FILE *f,
> -    struct got_repository *repo)
> +histedit_write_commit_list(struct got_object_id_queue *commits,
> +    FILE *f, int edit_logmsg_only, struct got_repository *repo)
>  {
>  	const struct got_error *err = NULL;
>  	struct got_object_qid *qid;
> @@ -5641,6 +5641,13 @@ histedit_write_commit_list(struct got_object_id_queue 
>  		    f, repo);
>  		if (err)
>  			break;
> +		if (edit_logmsg_only) {
> +			int n = fprintf(f, "%c\n", GOT_HISTEDIT_MESG);
> +			if (n < 0) {
> +				err = got_ferror(f, GOT_ERR_IO);
> +				break;
> +			}
> +		}
>  	}
>  
>  	return err;
> @@ -6018,7 +6025,7 @@ histedit_edit_list_retry(struct got_histedit_list *, c
>  static const struct got_error *
>  histedit_edit_script(struct got_histedit_list *histedit_cmds,
>      struct got_object_id_queue *commits, const char *branch_name,
> -    struct got_repository *repo)
> +    int edit_logmsg_only, struct got_repository *repo)
>  {
>  	const struct got_error *err;
>  	FILE *f = NULL;
> @@ -6032,23 +6039,27 @@ histedit_edit_script(struct got_histedit_list *histedi
>  	if (err)
>  		goto done;
>  
> -	err = histedit_write_commit_list(commits, f, repo);
> +	err = histedit_write_commit_list(commits, f, edit_logmsg_only, repo);
>  	if (err)
>  		goto done;
>  
> -	if (fclose(f) != 0) {
> -		err = got_error_from_errno("fclose");
> -		goto done;
> -	}
> -	f = NULL;
> -
> -	err = histedit_run_editor(histedit_cmds, path, commits, repo);
> -	if (err) {
> -		if (err->code != GOT_ERR_HISTEDIT_SYNTAX &&
> -		    err->code != GOT_ERR_HISTEDIT_CMD)
> +	if (edit_logmsg_only) {
> +		rewind(f);
> +		err = histedit_parse_list(histedit_cmds, f, repo);
> +	} else {
> +		if (fclose(f) != 0) {
> +			err = got_error_from_errno("fclose");
>  			goto done;
> -		err = histedit_edit_list_retry(histedit_cmds, err,
> -		    commits, path, branch_name, repo);
> +		}
> +		f = NULL;
> +		err = histedit_run_editor(histedit_cmds, path, commits, repo);
> +		if (err) {
> +			if (err->code != GOT_ERR_HISTEDIT_SYNTAX &&
> +			    err->code != GOT_ERR_HISTEDIT_CMD)
> +				goto done;
> +			err = histedit_edit_list_retry(histedit_cmds, err,
> +			    commits, path, branch_name, repo);
> +		}
>  	}
>  done:
>  	if (f && fclose(f) != 0 && err == NULL)
> @@ -6163,7 +6174,7 @@ histedit_edit_list_retry(struct got_histedit_list *his
>  		} else if (resp == 'r') {
>  			histedit_free_list(histedit_cmds);
>  			err = histedit_edit_script(histedit_cmds,
> -			    commits, branch_name, repo);
> +			    commits, branch_name, 0, repo);
>  			if (err) {
>  				if (err->code != GOT_ERR_HISTEDIT_SYNTAX &&
>  				    err->code != GOT_ERR_HISTEDIT_CMD)
> @@ -6354,6 +6365,7 @@ cmd_histedit(int argc, char *argv[])
>  	struct got_commit_object *commit = NULL;
>  	int ch, rebase_in_progress = 0, did_something;
>  	int edit_in_progress = 0, abort_edit = 0, continue_edit = 0;
> +	int edit_logmsg_only = 0;
>  	const char *edit_script_path = NULL;
>  	unsigned char rebase_status = GOT_STATUS_NO_CHANGE;
>  	struct got_object_id_queue commits;
> @@ -6367,7 +6379,7 @@ cmd_histedit(int argc, char *argv[])
>  	TAILQ_INIT(&histedit_cmds);
>  	TAILQ_INIT(&merged_paths);
>  
> -	while ((ch = getopt(argc, argv, "acF:")) != -1) {
> +	while ((ch = getopt(argc, argv, "acF:m")) != -1) {
>  		switch (ch) {
>  		case 'a':
>  			abort_edit = 1;
> @@ -6378,6 +6390,9 @@ cmd_histedit(int argc, char *argv[])
>  		case 'F':
>  			edit_script_path = optarg;
>  			break;
> +		case 'm':
> +			edit_logmsg_only = 1;
> +			break;
>  		default:
>  			usage_histedit();
>  			/* NOTREACHED */
> @@ -6393,7 +6408,13 @@ cmd_histedit(int argc, char *argv[])
>  		err(1, "pledge");
>  #endif
>  	if (abort_edit && continue_edit)
> -		usage_histedit();
> +		errx(1, "histedit's -a and -c options are mutually exclusive");
> +	if (edit_script_path && edit_logmsg_only)
> +		errx(1, "histedit's -F and -m options are mutually exclusive");
> +	if (abort_edit && edit_logmsg_only)
> +		errx(1, "histedit's -a and -m options are mutually exclusive");
> +	if (continue_edit && edit_logmsg_only)
> +		errx(1, "histedit's -c and -m options are mutually exclusive");
>  	if (argc != 0)
>  		usage_histedit();
>  
> @@ -6432,6 +6453,14 @@ cmd_histedit(int argc, char *argv[])
>  	if (error)
>  		goto done;
>  
> +	if (edit_in_progress && edit_logmsg_only) {
> +		error = got_error_msg(GOT_ERR_HISTEDIT_BUSY,
> +		    "histedit operation is in progress in this "
> +		    "work tree and must be continued or aborted "
> +		    "before the -m option can be used");
> +		goto done;
> +	}
> +
>  	if (edit_in_progress && abort_edit) {
>  		error = got_worktree_histedit_continue(&resume_commit_id,
>  		    &tmp_branch, &branch, &base_commit_id, &fileindex,
> @@ -6564,7 +6593,7 @@ cmd_histedit(int argc, char *argv[])
>  			if (strncmp(branch_name, "refs/heads/", 11) == 0)
>  				branch_name += 11;
>  			error = histedit_edit_script(&histedit_cmds, &commits,
> -			    branch_name, repo);
> +			    branch_name, edit_logmsg_only, repo);
>  			if (error) {
>  				got_worktree_histedit_abort(worktree, fileindex,
>  				    repo, branch, base_commit_id,

-- 

Tracey Emery