On 23-01-19 12:23PM, Stefan Sperling wrote:
> On Thu, Jan 19, 2023 at 07:05:44PM +1100, Mark Jamsek wrote:
> > As per the subject and todo item, the below diff introduces the -a flag
> > to both the backout and cherrypick commands.
> > 
> > I was glad to see this on the todo list because I'll use 'got cy -a'
> > a lot! I often run log or tog to copypasta the log message before
> > running `got cy branch && got ci -m msg` so I was keen to see this
> > implemented.
> > 
> > I'm really looking for feedback though as I'm not sure if the annotated
> > log message and base-commit bump stsp mentioned is adequate. Thanks!
> 
> Your code is not checking pre-conditions. This feature should require
> a fully up-to-date and clean work tree to reduce the likelyhood of
> problems during the merge and avoid unrelated changes getting mixed in.

Sorry, I did read that part you wrote in the todo and honestly wasn't
ignoring it! I intended to do this, I should've been more clear in my
email. I was looking for feedback on a couple things I was thinking
about while writing this but was late for a meeting and wanted to send
the email while it was on my mind in the hopes I'd have some feedback
I could apply when I returned. I really should've made the purpose of my
email clearer and emphasised its work-in-progress state. Sorry, Stefan,
I really didn't want to give the impression I'd just ignore your
requirements like that!

> > +	/*
> > +	 * XXX Don't think we should error here as 'got {backout,cherrypick}'
> > +	 * without -a doesn't error on these upa status codes.
> > +	 */
> > +	if (upa->conflicts || upa->missing || upa->not_deleted ||
> > +	    upa->obstructed || upa->unversioned ||
> > +	    upa->not_updated /* XXX shouldn't happen in cherrypick/backout? */)
> > +		return NULL;
> 
> Erroring out here would be better because the user was asking for
> an auto-commit an won't get it. I suppose the merge conflict stats
> have already been printed at this point, so a generic error message
> such as "aborting auto-commit due to unclean merge" should suffice.

Good point! I've done this too :)

Updated diff below adds a new got_worktree_autocommit_check() routine to
check for certain preconditions that stsp noted above and in the todo
entry; namely:
  - worktree is up-to-date
  - worktree is clean with no local changes
  - rebase is not in progress
  - histedit is not in progress
  - merge is not in progress

(cmd_{backout,cherrypick} already check for mixed base commits via
got_worktree_merge_files())

At present, we call this routine before the backout/cherrypick takes
place so if any of these preconditions fail, the bo/cy will not occur,
let alone the autocommit.  This placement was chosen so the user doesn't
get more changes mixed in with any existing changes; however, this
currently happens with 'got {bo,cy}' without the -a flag.

In the current revision, I've just followed 'got commit' such that, if
the -a flag is passed, we don't unveil() at all. I was going to call
apply_unveil() after we commit but this seems too late to matter.

We now also launch an editor as per op's suggestion to edit the commit
message, and show_diff is set now too so the diff of changes can be
viewed via a tmp file.

I've just read the follow-up emails and messages on IRC (I was
babysitting tonight so am late to the party :) and I'm happy to drop
this if we want to go in another direction. I just got a bit excited
when I read the todo this afternoon as it covers a use case I do
somewhat often, but it's no hassle to keep doing what I've been doing
till we come up with something else.

diffstat refs/remotes/origin/main refs/heads/main
 M  got/got.1               |   34+   2-
 M  got/got.c               |  172+  14-
 M  include/got_worktree.h  |    7+   0-
 M  lib/worktree.c          |  121+   0-

4 files changed, 334 insertions(+), 16 deletions(-)

diff refs/remotes/origin/main refs/heads/main
commit - 8fcd5cccdccdf61486b86753d28252df3bd5444a
commit + 2a2454aeaf19dd6e19863b38cd921e693714b1d9
blob - 06f7a35a329a5563b7c304c6b2be8b38e249e518
blob + 90cc3842f74e3344113426e4dd018b7012fe59ea
--- got/got.1
+++ got/got.1
@@ -1970,7 +1970,7 @@ The maximum is 3.
 The maximum is 3.
 .El
 .Tg cy
-.It Cm cherrypick Ar commit
+.It Cm cherrypick Oo Fl a Oc Ar commit
 .Dl Pq alias: Cm cy
 Merge changes from a single
 .Ar commit
@@ -2012,8 +2012,23 @@ conflicts must be resolved first.
 .Cm got update .
 If any relevant files already contain merge conflicts, these
 conflicts must be resolved first.
+.Pp
+The options for
+.Nm
+.Cm cherrypick
+are as follows:
+.Bl -tag -width Ds
+.It Fl a
+Automatically create a new commit on the work tree's current branch using the
+author, log message, and merged changes of
+.Ar commit .
+Before committing,
+.Nm
+will open an editor where the log message can be edited.
+.El
+.Pp
 .Tg bo
-.It Cm backout Ar commit
+.It Cm backout Oo Fl a Oc Ar commit
 .Dl Pq alias: Cm bo
 Reverse-merge changes from a single
 .Ar commit
@@ -2055,6 +2070,23 @@ conflicts must be resolved first.
 .Cm got update .
 If any relevant files already contain merge conflicts, these
 conflicts must be resolved first.
+.Pp
+The options for
+.Nm
+.Cm backout
+are as follows:
+.Bl -tag -width Ds
+.It Fl a
+Automatically create a new commit on the work tree's current branch using
+.Ar commit Ns 's
+reverse-merged changes and log message prepended with
+.Qq backout
+and its abbreviated object ID.
+Before committing,
+.Nm
+will open an editor where the log message can be edited.
+.El
+.Pp
 .Tg rb
 .It Xo
 .Cm rebase
blob - f1564fe9067f8708e5f87d50539aa46985cdc197
blob + b048fa4e9c4400385b56f352f602f3104c211e9f
--- got/got.c
+++ got/got.c
@@ -9454,11 +9454,142 @@ usage_cherrypick(void)
 __dead static void
 usage_cherrypick(void)
 {
-	fprintf(stderr, "usage: %s cherrypick commit-id\n", getprogname());
+	fprintf(stderr, "usage: %s cherrypick [-a] commit-id\n", getprogname());
 	exit(1);
 }
 
 static const struct got_error *
+autocommit_merged_changes(struct got_commit_object *commit,
+    struct got_update_progress_arg *upa, const char *backout_commit_id,
+    struct got_worktree *worktree, struct got_repository *repo)
+{
+	const struct got_error			*err = NULL;
+	struct collect_commit_logmsg_arg	 cla;
+	struct got_pathlist_head		 paths;
+	struct got_object_id			*commit_id = NULL;
+	FILE					*f = NULL;
+	const char				*author = NULL;
+	char					*bo = NULL, *committer = NULL;
+	char					*logmsg0, *logmsg = NULL;
+	char					*editor = NULL, *idstr = NULL;
+	char					*prepared_log = NULL;
+	size_t					 len;
+	int					 preserve_logmsg = 0;
+
+	if (upa->conflicts || upa->missing || upa->not_deleted ||
+	    upa->obstructed || upa->unversioned ||
+	    upa->not_updated /* XXX shouldn't happen in cherrypick/backout? */)
+		return got_error_msg(GOT_ERR_CONFLICTS,
+		    "auto-commit aborted due to unclean merge");
+
+	TAILQ_INIT(&paths);
+	memset(&cla, 0, sizeof(cla));
+
+	err = got_pathlist_append(&paths, "", NULL);
+	if (err)
+		return err;
+
+	err = got_object_commit_get_logmsg(&logmsg, commit);
+	if (err)
+		goto done;
+
+	len = strlen(logmsg);
+	if (logmsg[len - 1] == '\n')
+		logmsg[len - 1] = '\0';
+
+	if (backout_commit_id) {
+		if (asprintf(&bo, "backout %.10s: ", backout_commit_id) == -1) {
+			err = got_error_from_errno("asprintf");
+			goto done;
+		}
+	}
+
+	if (asprintf(&logmsg0, "%s%s", bo ? bo : "", logmsg + 1) == -1) {
+		err = got_error_from_errno("asprintf");
+		goto done;
+	}
+	free(logmsg);
+	logmsg = logmsg0;
+
+	err = get_author(&committer, repo, worktree);
+	if (err)
+		goto done;
+
+	/* don't use author in auto backout commit, committer is the author */
+	if (bo == NULL)
+		author = got_object_commit_get_author(commit);
+	if (author == NULL)
+		author = committer;
+
+	err = get_editor(&editor);
+	if (err)
+		goto done;
+
+	err = got_opentemp_named(&prepared_log, &f, "got-commit-logmsg", "");
+	if (err)
+		return err;
+
+	if (fprintf(f, "%s", logmsg) < 0) {
+		err = got_ferror(f, GOT_ERR_IO);
+		goto done;
+	}
+	if (fclose(f) == EOF) {
+		err = got_error_from_errno("fclose");
+		goto done;
+	}
+	f = NULL;
+
+	cla.branch_name = got_worktree_get_head_ref_name(worktree);
+	if (strncmp(cla.branch_name, "refs/heads/", 11) != 0) {
+		err = got_error(GOT_ERR_COMMIT_BRANCH);
+		goto done;
+	}
+	cla.branch_name += 11;
+	cla.editor = editor;
+	cla.non_interactive = 0;
+	cla.prepared_log = prepared_log;
+	cla.repo_path = got_repo_get_path(repo);
+	cla.worktree_path = got_worktree_get_root_path(worktree);
+
+	err = got_worktree_commit(&commit_id, worktree, &paths,
+	    author, committer, 0, 1, collect_commit_logmsg, &cla,
+	    print_status, NULL, repo);
+	if (err) {
+		if (err->code != GOT_ERR_COMMIT_MSG_EMPTY &&
+		    cla.logmsg_path != NULL)
+			preserve_logmsg = 1;
+		goto done;
+	}
+
+	err = got_object_id_str(&idstr, commit_id);
+	if (err)
+		goto done;
+
+	printf("Created commit %s\n", idstr);
+	err = got_worktree_autocommit_complete(worktree, repo);
+
+done:
+	if (preserve_logmsg) {
+		fprintf(stderr, "%s: log message preserved in %s\n",
+		    getprogname(), cla.logmsg_path);
+	} else if (cla.logmsg_path && unlink(cla.logmsg_path) == -1 &&
+	    err == NULL)
+		err = got_error_from_errno2("unlink", cla.logmsg_path);
+	if (f && fclose(f) == EOF && err == NULL)
+		err = got_error_from_errno("fclose");
+	if (prepared_log && unlink(prepared_log) != 0 && err == NULL)
+		err = got_error_from_errno2("unlink", prepared_log);
+	free(bo);
+	free(idstr);
+	free(logmsg);
+	free(commit_id);
+	free(committer);
+	free(cla.logmsg_path);
+	got_pathlist_free(&paths, GOT_PATHLIST_FREE_NONE);
+	return err;
+}
+
+static const struct got_error *
 cmd_cherrypick(int argc, char *argv[])
 {
 	const struct got_error *error = NULL;
@@ -9468,12 +9599,15 @@ cmd_cherrypick(int argc, char *argv[])
 	struct got_object_id *commit_id = NULL;
 	struct got_commit_object *commit = NULL;
 	struct got_object_qid *pid;
-	int ch;
+	int ch, auto_commit = 0;
 	struct got_update_progress_arg upa;
 	int *pack_fds = NULL;
 
-	while ((ch = getopt(argc, argv, "")) != -1) {
+	while ((ch = getopt(argc, argv, "a")) != -1) {
 		switch (ch) {
+		case 'a':
+			auto_commit = 1;
+			break;
 		default:
 			usage_cherrypick();
 			/* NOTREACHED */
@@ -9514,10 +9648,16 @@ cmd_cherrypick(int argc, char *argv[])
 	if (error != NULL)
 		goto done;
 
-	error = apply_unveil(got_repo_get_path(repo), 0,
-	    got_worktree_get_root_path(worktree));
-	if (error)
-		goto done;
+	if (auto_commit) {
+		error = got_worktree_autocommit_check(worktree, repo);
+		if (error)
+			goto done;
+	} else {
+		error = apply_unveil(got_repo_get_path(repo), 0,
+		    got_worktree_get_root_path(worktree));
+		if (error)
+			goto done;
+	}
 
 	error = got_repo_match_object_id(&commit_id, NULL, argv[0],
 	    GOT_OBJ_TYPE_COMMIT, NULL, repo);
@@ -9541,6 +9681,10 @@ done:
 	if (upa.did_something)
 		printf("Merged commit %s\n", commit_id_str);
 	print_merge_progress_stats(&upa);
+
+	if (auto_commit && upa.did_something)
+		error = autocommit_merged_changes(commit, &upa, NULL,
+		    worktree, repo);
 done:
 	if (commit)
 		got_object_commit_close(commit);
@@ -9565,7 +9709,7 @@ usage_backout(void)
 __dead static void
 usage_backout(void)
 {
-	fprintf(stderr, "usage: %s backout commit-id\n", getprogname());
+	fprintf(stderr, "usage: %s backout [-a] commit-id\n", getprogname());
 	exit(1);
 }
 
@@ -9579,12 +9723,16 @@ cmd_backout(int argc, char *argv[])
 	struct got_object_id *commit_id = NULL;
 	struct got_commit_object *commit = NULL;
 	struct got_object_qid *pid;
-	int ch;
+	int ch, auto_commit = 0;
 	struct got_update_progress_arg upa;
+
 	int *pack_fds = NULL;
 
-	while ((ch = getopt(argc, argv, "")) != -1) {
+	while ((ch = getopt(argc, argv, "a")) != -1) {
 		switch (ch) {
+		case 'a':
+			auto_commit = 1;
+			break;
 		default:
 			usage_backout();
 			/* NOTREACHED */
@@ -9624,10 +9772,16 @@ cmd_backout(int argc, char *argv[])
 	if (error != NULL)
 		goto done;
 
-	error = apply_unveil(got_repo_get_path(repo), 0,
-	    got_worktree_get_root_path(worktree));
-	if (error)
-		goto done;
+	if (auto_commit) {
+		error = got_worktree_autocommit_check(worktree, repo);
+		if (error)
+			goto done;
+	} else {
+		error = apply_unveil(got_repo_get_path(repo), 0,
+		    got_worktree_get_root_path(worktree));
+		if (error)
+			goto done;
+	}
 
 	error = got_repo_match_object_id(&commit_id, NULL, argv[0],
 	    GOT_OBJ_TYPE_COMMIT, NULL, repo);
@@ -9655,6 +9809,10 @@ done:
 	if (upa.did_something)
 		printf("Backed out commit %s\n", commit_id_str);
 	print_merge_progress_stats(&upa);
+
+	if (auto_commit && upa.did_something)
+		error = autocommit_merged_changes(commit, &upa, commit_id_str,
+		    worktree, repo);
 done:
 	if (commit)
 		got_object_commit_close(commit);
blob - d3c26b9a044948f5b93e8f2f6adf673b8dd7e599
blob + 6cf18d352a0eaf7624e295b162e5f3d00df4601a
--- include/got_worktree.h
+++ include/got_worktree.h
@@ -571,3 +571,10 @@ got_worktree_patch_complete(struct got_fileindex *, co
 /* Complete the patch operation. */
 const struct got_error *
 got_worktree_patch_complete(struct got_fileindex *, const char *);
+
+
+const struct got_error *
+got_worktree_autocommit_complete(struct got_worktree *, struct got_repository *);
+
+const struct got_error *
+got_worktree_autocommit_check(struct got_worktree *, struct got_repository *);
blob - febf807fc9a1e5b9c2cdab208adc8217a492980e
blob + 9d1e0a689acf40237249b6022189efb26a884b44
--- lib/worktree.c
+++ lib/worktree.c
@@ -9355,3 +9355,124 @@ got_worktree_patch_complete(struct got_fileindex *file
 
 	return err;
 }
+
+const struct got_error *
+got_worktree_autocommit_complete(struct got_worktree *worktree,
+    struct got_repository *repo)
+{
+	const struct got_error	*err, *sync_err;
+	struct got_fileindex	*fileindex = NULL;
+	char			*fileindex_path = NULL;
+
+	err = open_fileindex(&fileindex, &fileindex_path, worktree);
+	if (err)
+		return err;
+
+	err = bump_base_commit_id_everywhere(worktree, fileindex, NULL, NULL);
+	sync_err = sync_fileindex(fileindex, fileindex_path);
+	if (sync_err && err == NULL)
+		err = sync_err;
+
+	got_fileindex_free(fileindex);
+	free(fileindex_path);
+	return err;
+}
+
+struct worktree_is_clean_arg {
+	struct got_worktree	*worktree;
+	struct got_repository	*repo;
+};
+
+static const struct got_error *
+worktree_is_clean(void *arg, struct got_fileindex_entry *ie)
+{
+	const struct got_error		*err;
+	struct worktree_is_clean_arg	*a = arg;
+	struct stat			 sb;
+	char				*path;
+	unsigned char			 status;
+
+	if (asprintf(&path, "%s/%s", a->worktree->root_path, ie->path) == -1)
+		return got_error_from_errno("asprintf");
+
+	err = get_file_status(&status, &sb, ie, path, -1, NULL, a->repo);
+	free(path);
+	if (err)
+		return err;
+
+	if (status != GOT_STATUS_NO_CHANGE)
+		return got_error(GOT_ERR_MODIFIED);
+	if (get_staged_status(ie) != GOT_STATUS_NO_CHANGE)
+		return got_error_path(ie->path, GOT_ERR_FILE_STAGED);
+
+	return NULL;
+}
+
+const struct got_error *
+got_worktree_autocommit_check(struct got_worktree *worktree,
+    struct got_repository *repo)
+{
+	const struct got_error		*err;
+	struct got_fileindex		*fileindex = NULL;
+	struct got_object_id		*base_commit_id, *head_commit_id = NULL;
+	struct got_reference		*head_ref = NULL;
+	struct worktree_is_clean_arg	 wtc;
+	char				*fileindex_path = NULL;
+	int				 histedit_in_progress;
+	int				 rebase_in_progress, merge_in_progress;
+
+	err = got_worktree_histedit_in_progress(&histedit_in_progress,
+	    worktree);
+	if (err)
+		return err;
+	if (histedit_in_progress) {
+		err = got_error(GOT_ERR_HISTEDIT_BUSY);
+		return err;
+	}
+
+	err = got_worktree_rebase_in_progress(&rebase_in_progress, worktree);
+	if (err)
+		return err;
+	if (rebase_in_progress)
+		return got_error(GOT_ERR_REBASING);
+
+	err = got_worktree_merge_in_progress(&merge_in_progress, worktree, repo);
+	if (err)
+		return err;
+	if (merge_in_progress)
+		return got_error(GOT_ERR_MERGE_BUSY);
+
+	err = got_ref_open(&head_ref, repo,
+	    got_worktree_get_head_ref_name(worktree), 0);
+	if (err)
+		goto done;
+
+	err = got_ref_resolve(&head_commit_id, repo, head_ref);
+	if (err)
+		goto done;
+
+	base_commit_id = got_worktree_get_base_commit_id(worktree);
+
+	if (got_object_id_cmp(base_commit_id, head_commit_id) != 0) {
+		err = got_error(GOT_ERR_COMMIT_OUT_OF_DATE);
+		goto done;
+	}
+
+	err = open_fileindex(&fileindex, &fileindex_path, worktree);
+	if (err)
+		goto done;
+
+	wtc.worktree = worktree;
+	wtc.repo = repo;
+	err = got_fileindex_for_each_entry_safe(fileindex, worktree_is_clean,
+	    &wtc);
+
+done:
+	free(head_commit_id);
+	if (head_ref)
+		got_ref_close(head_ref);
+	if (fileindex)
+		got_fileindex_free(fileindex);
+	free(fileindex_path);
+	return err;
+}

-- 
Mark Jamsek <fnc.bsdbox.org>
GPG: F2FF 13DE 6A06 C471 CA80  E6E2 2930 DC66 86EE CF68