From: Omar Polo Subject: Re: got backout/cherrypick auto commit To: Mark Jamsek Cc: Game of Trees Date: Thu, 19 Jan 2023 11:19:28 +0100 On 2023/01/19 19:05:44 +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 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 :)