On 2023/01/19 19:05:44 +1100, Mark Jamsek <mark@jamsek.com> 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 always a bit reclutant to add flags to existing commands, but on
the other hand i often had to do the same (i.e. copy-paste from got
the message and/or manually truncating the id) so i can see how this
could be handy.

> 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!

one quick comment is that I'd prefer if we still open the editor to
allow the commit message to be edited (without a got update -c && got
histedit -m dance), especially for backout at list, where you might
want to add something to why you're reverting the commit.

some quick comments below, haven't played with it yet.

> diffstat eca2b1d0d697cef723df2d1394b84b636c02ced0 refs/heads/main
>  M  got/got.1               |   26+  0-
>  M  got/got.c               |  115+  6-
>  M  include/got_worktree.h  |    4+  0-
>  M  lib/worktree.c          |   23+  0-
> 
> 4 files changed, 168 insertions(+), 6 deletions(-)
> 
> diff eca2b1d0d697cef723df2d1394b84b636c02ced0 refs/heads/main
> commit - eca2b1d0d697cef723df2d1394b84b636c02ced0
> commit + e3bc7ce6dca5ed5a66f727442aea02dd81edc176
> blob - 06f7a35a329a5563b7c304c6b2be8b38e249e518
> blob + 98b9b0dc8c54279f115f4b2bbb2525d61a74b44c
> --- got/got.1
> +++ got/got.1
> @@ -2012,6 +2012,18 @@ conflicts must be resolved first.

need to add -a to the subcommand """synopsis"""

-.It Cm cherrypick Ar commit
+.It Cm cherrypick Oo Fl a Oc Ar commit

>  .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 .
> +.El
> +.Pp
>  .Tg bo
>  .It Cm backout Ar commit
>  .Dl Pq alias: Cm bo
> @@ -2055,6 +2067,20 @@ conflicts must be resolved first.

ditto for backout

>  .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:\ \&

not a mandoc/roff hacker but just

 .Qq backout: \&

should do it.  Not sure however if including the space is particularly
helpful.

> +and its abbreviated object ID.
> +.El
> +.Pp
>  .Tg rb
>  .It Xo
>  .Cm rebase
> blob - f1564fe9067f8708e5f87d50539aa46985cdc197
> blob + 20e819a2087f53305c3316ae9f5dd53defbdc028
> --- got/got.c
> +++ got/got.c
> @@ -9454,11 +9454,105 @@ 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;
> +	const char				*author = NULL;
> +	struct got_object_id			*commit_id = NULL;
> +	char					*bo = NULL, *committer = NULL;
> +	char					*logmsg0, *logmsg = NULL;
> +	char					*idstr = NULL;
> +
> +	/*
> +	 * 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;
> +
> +	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;
> +
> +	if (backout_commit_id) {
> +		if (asprintf(&bo, "backout %.10s: ", backout_commit_id) == -1) {

shouldn't be "backout: %.10s" according to the manpage change?

> +			err = got_error_from_errno("asprintf");
> +			goto done;
> +		}
> +	}
> [...]
> +static const struct got_error *
>  cmd_cherrypick(int argc, char *argv[])
>  {
>  	const struct got_error *error = NULL;
> @@ -9468,12 +9562,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;

nit: double ;

> +	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 */
> [...]
> blob - d3c26b9a044948f5b93e8f2f6adf673b8dd7e599
> blob + a1682caee9c4d198472296991b3a16ed4a8bc568
> --- include/got_worktree.h
> +++ include/got_worktree.h
> @@ -571,3 +571,7 @@ 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 *);

nit: double empty line

> +
> +
> +const struct got_error *
> +got_worktree_autocommit_complete(struct got_worktree *, struct got_repository *);
> blob - febf807fc9a1e5b9c2cdab208adc8217a492980e
> blob + 56b6756df955ba8abe906fc8084dc56ce4343343
> --- lib/worktree.c
> +++ lib/worktree.c
> @@ -9355,3 +9355,26 @@ 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;
> +}
> +

nit: ...and i'd drop the trailing one too :)