From: Josh Rickmar <openbsd+lists@zettaport.com>
Subject: Re: add histedit -e option
To: gameoftrees@openbsd.org
Date: Thu, 30 Sep 2021 20:11:25 -0400

On Thu, Sep 30, 2021 at 09:43:08PM +0200, Stefan Sperling wrote:
> Sometimes I find myself using only the 'edit' command in histedit scripts.
> This can happen after a couple of patches that depend on each other
> have received code review.
> 
> This can be useful when there are a handful of commits that need to be
> tweaked. I can now skip the histedit scripting step in this case:
> 
>   got up -c origin/main
>   got he -e
> 
> ok?
>  
> diff 3a9ff2603178620aa1e31e55cdac42a879e22dc8 5b6479f3914e22c83a9396309ece5795c48361e7
> blob - 237c161ab95271094fff0f6ca83316e7b1d60e7d
> blob + 450e4baab5541257e9c371762a5fe893fa1c19bb
> --- got/got.1
> +++ got/got.1
> @@ -1863,7 +1863,7 @@ None of the other options can be used together with
>  .It Cm rb
>  Short alias for
>  .Cm rebase .
> -.It Cm histedit Oo Fl a Oc Oo Fl c Oc Oo Fl f Oc Oo Fl F Ar histedit-script Oc Oo Fl m Oc Oo Fl l Oc Oo Fl X Oc Op Ar branch
> +.It Cm histedit Oo Fl a Oc Oo Fl c Oc Oo Fl e Oc Oo Fl f Oc Oo Fl F Ar histedit-script Oc Oo Fl m Oc Oo Fl l Oc Oo Fl X Oc Op Ar branch
>  Edit commit history between the work tree's current base commit and
>  the tip commit of the work tree's current branch.
>  .Pp
> @@ -2011,6 +2011,15 @@ 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 e
> +Interrupt the histedit operation for editing after merging each commit.
> +This option is a quick equivalent to a histedit script which uses the
> +.Cm edit
> +command for all commits.
> +The
> +.Fl e
> +option can only be used when starting a new histedit operation.
> +If this option is used, no other command-line arguments are allowed.
>  .It Fl f
>  Fold all commits into a single commit.
>  This option is a quick equivalent to a histedit script which folds all
> blob - 2a84457cfb630de071de04ba1bcc66b3700a19ae
> blob + 5bfbe6c3567efc8de019a1c22570d5a47bee75be
> --- got/got.c
> +++ got/got.c
> @@ -9162,7 +9162,7 @@ done:
>  __dead static void
>  usage_histedit(void)
>  {
> -	fprintf(stderr, "usage: %s histedit [-a] [-c] [-f] "
> +	fprintf(stderr, "usage: %s histedit [-a] [-c] [-e] [-f] "
>  	    "[-F histedit-script] [-m] [-l] [-X] [branch]\n",
>  	    getprogname());
>  	exit(1);
> @@ -9230,7 +9230,8 @@ done:
>  
>  static const struct got_error *
>  histedit_write_commit_list(struct got_object_id_queue *commits,
> -    FILE *f, int edit_logmsg_only, int fold_only, struct got_repository *repo)
> +    FILE *f, int edit_logmsg_only, int fold_only, int edit_only,
> +    struct got_repository *repo)
>  {
>  	const struct got_error *err = NULL;
>  	struct got_object_qid *qid;
> @@ -9241,7 +9242,9 @@ histedit_write_commit_list(struct got_object_id_queue 
>  
>  	STAILQ_FOREACH(qid, commits, entry) {
>  		histedit_cmd = got_histedit_cmds[0].name;
> -		if (fold_only && STAILQ_NEXT(qid, entry) != NULL)
> +		if (edit_only)
> +			histedit_cmd = "edit";
> +		else if (fold_only && STAILQ_NEXT(qid, entry) != NULL)
>  			histedit_cmd = "fold";
>  		err = histedit_write_commit(qid->id, histedit_cmd, f, repo);
>  		if (err)
> @@ -9655,7 +9658,8 @@ 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,
> -    int edit_logmsg_only, int fold_only, struct got_repository *repo)
> +    int edit_logmsg_only, int fold_only, int edit_only,
> +    struct got_repository *repo)
>  {
>  	const struct got_error *err;
>  	FILE *f = NULL;
> @@ -9670,11 +9674,11 @@ histedit_edit_script(struct got_histedit_list *histedi
>  		goto done;
>  
>  	err = histedit_write_commit_list(commits, f, edit_logmsg_only,
> -	    fold_only, repo);
> +	    fold_only, edit_only, repo);
>  	if (err)
>  		goto done;
>  
> -	if (edit_logmsg_only || fold_only) {
> +	if (edit_logmsg_only || fold_only || edit_only) {
>  		rewind(f);
>  		err = histedit_parse_list(histedit_cmds, f, repo);
>  	} else {
> @@ -9805,7 +9809,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, 0, 0, repo);
> +			    commits, branch_name, 0, 0, 0, repo);
>  			if (err) {
>  				if (err->code != GOT_ERR_HISTEDIT_SYNTAX &&
>  				    err->code != GOT_ERR_HISTEDIT_CMD)
> @@ -9997,7 +10001,7 @@ cmd_histedit(int argc, char *argv[])
>  	int ch, rebase_in_progress = 0, merge_in_progress = 0;
>  	struct got_update_progress_arg upa;
>  	int edit_in_progress = 0, abort_edit = 0, continue_edit = 0;
> -	int edit_logmsg_only = 0, fold_only = 0;
> +	int edit_logmsg_only = 0, fold_only = 0, edit_only = 0;
>  	int list_backups = 0, delete_backups = 0;
>  	const char *edit_script_path = NULL;
>  	struct got_object_id_queue commits;
> @@ -10012,7 +10016,7 @@ cmd_histedit(int argc, char *argv[])
>  	TAILQ_INIT(&merged_paths);
>  	memset(&upa, 0, sizeof(upa));
>  
> -	while ((ch = getopt(argc, argv, "acfF:mlX")) != -1) {
> +	while ((ch = getopt(argc, argv, "acefF:mlX")) != -1) {
>  		switch (ch) {
>  		case 'a':
>  			abort_edit = 1;
> @@ -10020,6 +10024,9 @@ cmd_histedit(int argc, char *argv[])
>  		case 'c':
>  			continue_edit = 1;
>  			break;
> +		case 'e':
> +			edit_only = 1;
> +			break;
>  		case 'f':
>  			fold_only = 1;
>  			break;
> @@ -10064,7 +10071,15 @@ cmd_histedit(int argc, char *argv[])
>  	if (fold_only && edit_logmsg_only)
>  		option_conflict('f', 'm');
>  	if (edit_script_path && fold_only)
> -		option_conflict('F', 'f');
> +		option_conflict('F', 'e');

this should remain the same, otherwise:

$ got he -F script -f
got: -F and -e options are mutually exclusive

> +	if (abort_edit && edit_only)
> +		option_conflict('a', 'e');
> +	if (continue_edit && edit_only)
> +		option_conflict('c', 'e');
> +	if (edit_only && edit_logmsg_only)
> +		option_conflict('e', 'm');
> +	if (edit_script_path && edit_only)
> +		option_conflict('F', 'e');
>  	if (list_backups) {
>  		if (abort_edit)
>  			option_conflict('l', 'a');
> @@ -10076,6 +10091,8 @@ cmd_histedit(int argc, char *argv[])
>  			option_conflict('l', 'm');
>  		if (fold_only)
>  			option_conflict('l', 'f');
> +		if (edit_only)
> +			option_conflict('l', 'e');
>  		if (delete_backups)
>  			option_conflict('l', 'X');
>  		if (argc != 0 && argc != 1)
> @@ -10091,6 +10108,8 @@ cmd_histedit(int argc, char *argv[])
>  			option_conflict('X', 'm');
>  		if (fold_only)
>  			option_conflict('X', 'f');
> +		if (edit_only)
> +			option_conflict('X', 'e');
>  		if (list_backups)
>  			option_conflict('X', 'l');
>  		if (argc != 0 && argc != 1)
> @@ -10181,6 +10200,13 @@ cmd_histedit(int argc, char *argv[])
>  		    "before the -f option can be used");
>  		goto done;
>  	}
> +	if (edit_in_progress && edit_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 -e option can be used");
> +		goto done;
> +	}
>  
>  	if (edit_in_progress && abort_edit) {
>  		error = got_worktree_histedit_continue(&resume_commit_id,
> @@ -10316,7 +10342,8 @@ 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, edit_logmsg_only, fold_only, repo);
> +			    branch_name, edit_logmsg_only, fold_only,
> +			    edit_only, repo);
>  			if (error) {
>  				got_worktree_histedit_abort(worktree, fileindex,
>  				    repo, branch, base_commit_id,
> blob - 61bcebf99d9b2d05bc608cd43b07f0c9e3cdf818
> blob + 65ef292945ab5a14c0c1674cbd8aec70fc5e9418
> --- regress/cmdline/histedit.sh
> +++ regress/cmdline/histedit.sh
> @@ -1766,6 +1766,153 @@ EOF
>  	test_done "$testroot" "$ret"
>  }
>  
> +test_histedit_edit_only() {
> +	local testroot=`test_init histedit_edit_only`
> +
> +	local orig_commit=`git_show_head $testroot/repo`
> +
> +	echo "modified alpha on master" > $testroot/repo/alpha
> +	(cd $testroot/repo && git rm -q beta)
> +	echo "new file on master" > $testroot/repo/epsilon/new
> +	(cd $testroot/repo && git add epsilon/new)
> +	git_commit $testroot/repo -m "committing changes"
> +	local old_commit1=`git_show_head $testroot/repo`
> +
> +	echo "modified zeta on master" > $testroot/repo/epsilon/zeta
> +	git_commit $testroot/repo -m "committing to zeta on master"
> +	local old_commit2=`git_show_head $testroot/repo`
> +
> +	got checkout -c $orig_commit $testroot/repo $testroot/wt > /dev/null
> +	ret="$?"
> +	if [ "$ret" != "0" ]; then
> +		test_done "$testroot" "$ret"
> +		return 1
> +	fi
> +
> +	(cd $testroot/wt && got histedit -e > $testroot/stdout)
> +
> +	local short_old_commit1=`trim_obj_id 28 $old_commit1`
> +	local short_old_commit2=`trim_obj_id 28 $old_commit2`
> +
> +	echo "G  alpha" > $testroot/stdout.expected
> +	echo "D  beta" >> $testroot/stdout.expected
> +	echo "A  epsilon/new" >> $testroot/stdout.expected
> +	echo "Stopping histedit for amending commit $old_commit1" \
> +		>> $testroot/stdout.expected
> +	cmp -s $testroot/stdout.expected $testroot/stdout
> +	ret="$?"
> +	if [ "$ret" != "0" ]; then
> +		diff -u $testroot/stdout.expected $testroot/stdout
> +		test_done "$testroot" "$ret"
> +		return 1
> +	fi
> +
> +	echo "edited modified alpha on master" > $testroot/wt/alpha
> +
> +	cat > $testroot/editor.sh <<EOF
> +#!/bin/sh
> +sed -i 's/.*/committing edited changes 1/' "\$1"
> +EOF
> +	chmod +x $testroot/editor.sh
> +
> +	(cd $testroot/wt && env EDITOR="$testroot/editor.sh" \
> +		VISUAL="$testroot/editor.sh" got histedit -c > $testroot/stdout)
> +
> +	local new_commit1=$(cd $testroot/wt && got info | \
> +		grep '^work tree base commit: ' | cut -d: -f2 | tr -d ' ')
> +	local short_new_commit1=`trim_obj_id 28 $new_commit1`
> +
> +	echo -n "$short_old_commit1 -> $short_new_commit1: " \
> +		> $testroot/stdout.expected
> +	echo "committing edited changes 1" >> $testroot/stdout.expected
> +	echo "G  epsilon/zeta" >> $testroot/stdout.expected
> +	echo "Stopping histedit for amending commit $old_commit2" \
> +		>> $testroot/stdout.expected
> +	cmp -s $testroot/stdout.expected $testroot/stdout
> +	ret="$?"
> +	if [ "$ret" != "0" ]; then
> +		diff -u $testroot/stdout.expected $testroot/stdout
> +		test_done "$testroot" "$ret"
> +		return 1
> +	fi
> +
> +	echo "edited zeta on master" > $testroot/wt/epsilon/zeta
> +
> +	cat > $testroot/editor.sh <<EOF
> +#!/bin/sh
> +sed -i 's/.*/committing edited changes 2/' "\$1"
> +EOF
> +	chmod +x $testroot/editor.sh
> +
> +	(cd $testroot/wt && env EDITOR="$testroot/editor.sh" \
> +		VISUAL="$testroot/editor.sh" got histedit -c > $testroot/stdout)
> +
> +	local new_commit2=`git_show_head $testroot/repo`
> +	local short_new_commit2=`trim_obj_id 28 $new_commit2`
> +
> +	echo -n "$short_old_commit2 -> $short_new_commit2: " \
> +		> $testroot/stdout.expected
> +	echo "committing edited changes 2" >> $testroot/stdout.expected
> +	echo "Switching work tree to refs/heads/master" \
> +		>> $testroot/stdout.expected
> +
> +	cmp -s $testroot/stdout.expected $testroot/stdout
> +	ret="$?"
> +	if [ "$ret" != "0" ]; then
> +		diff -u $testroot/stdout.expected $testroot/stdout
> +		test_done "$testroot" "$ret"
> +		return 1
> +	fi
> +
> +	echo "edited modified alpha on master" > $testroot/content.expected
> +	cat $testroot/wt/alpha > $testroot/content
> +	cmp -s $testroot/content.expected $testroot/content
> +	ret="$?"
> +	if [ "$ret" != "0" ]; then
> +		diff -u $testroot/content.expected $testroot/content
> +		test_done "$testroot" "$ret"
> +		return 1
> +	fi
> +
> +	if [ -e $testroot/wt/beta ]; then
> +		echo "removed file beta still exists on disk" >&2
> +		test_done "$testroot" "1"
> +		return 1
> +	fi
> +
> +	echo "new file on master" > $testroot/content.expected
> +	cat $testroot/wt/epsilon/new > $testroot/content
> +	cmp -s $testroot/content.expected $testroot/content
> +	ret="$?"
> +	if [ "$ret" != "0" ]; then
> +		diff -u $testroot/content.expected $testroot/content
> +		test_done "$testroot" "$ret"
> +		return 1
> +	fi
> +
> +	(cd $testroot/wt && got status > $testroot/stdout)
> +
> +	echo -n > $testroot/stdout.expected
> +	cmp -s $testroot/stdout.expected $testroot/stdout
> +	ret="$?"
> +	if [ "$ret" != "0" ]; then
> +		diff -u $testroot/stdout.expected $testroot/stdout
> +		test_done "$testroot" "$ret"
> +		return 1
> +	fi
> +
> +	(cd $testroot/wt && got log -l3 | grep ^commit > $testroot/stdout)
> +	echo "commit $new_commit2 (master)" > $testroot/stdout.expected
> +	echo "commit $new_commit1" >> $testroot/stdout.expected
> +	echo "commit $orig_commit" >> $testroot/stdout.expected
> +	cmp -s $testroot/stdout.expected $testroot/stdout
> +	ret="$?"
> +	if [ "$ret" != "0" ]; then
> +		diff -u $testroot/stdout.expected $testroot/stdout
> +	fi
> +	test_done "$testroot" "$ret"
> +}
> +
>  test_parseargs "$@"
>  run_test test_histedit_no_op
>  run_test test_histedit_swap
> @@ -1784,3 +1931,4 @@ run_test test_histedit_duplicate_commit_in_script
>  run_test test_histedit_fold_add_delete
>  run_test test_histedit_fold_only
>  run_test test_histedit_fold_only_empty_logmsg
> +run_test test_histedit_edit_only
> 

ok except for the one inline comment.