From: Mark Jamsek Subject: Re: got backout/cherrypick auto commit To: Game of Trees Date: Fri, 20 Jan 2023 01:43:47 +1100 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 GPG: F2FF 13DE 6A06 C471 CA80 E6E2 2930 DC66 86EE CF68