git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 4/4] built-in rebase: call `git am` directly

From
Junio C Hamano <gitster@pobox.com>
Date
Jan 4, 2019, 18:38 UTC
Message-ID
<xmqqwonkclpx.fsf@gitster-ct.c.googlers.com>
In-Reply-To
<2b5ece8263936f0a7dfad864c0de43d784fdaf1f.1545398254.git.gitgitgadget@gmail.com>

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 22 quoted lines
> +static int write_basic_state(struct rebase_options *opts)
> +{
> +	write_file(state_dir_path("head-name", opts), "%s",
> +		   opts->head_name ? opts->head_name : "detached HEAD");
> +	write_file(state_dir_path("onto", opts), "%s",
> +		   opts->onto ? oid_to_hex(&opts->onto->object.oid) : "");
> +	write_file(state_dir_path("orig-head", opts), "%s",
> +		   oid_to_hex(&opts->orig_head));
> +	write_file(state_dir_path("quiet", opts), "%s",
> +		   opts->flags & REBASE_NO_QUIET ? "" : "t");
> +	if (opts->flags & REBASE_VERBOSE)
> +		write_file(state_dir_path("verbose", opts), "%s", "");
> +	if (opts->strategy)
> +		write_file(state_dir_path("strategy", opts), "%s",
> +			   opts->strategy);
> +	if (opts->strategy_opts)
> +		write_file(state_dir_path("strategy_opts", opts), "%s",
> +			   opts->strategy_opts);
> +	if (opts->allow_rerere_autoupdate >= 0)
> +		write_file(state_dir_path("allow_rerere_autoupdate", opts),
> +			   "-%s-rerere-autoupdate",
> +			   opts->allow_rerere_autoupdate ? "" : "-no");

Inside rebase, allow-rerere-autoupdate can be -1 (unspecified), 0 (declined) or 1 (requested), and this code is being consistent with that convention.

The "--[no-]rerere-autoupdate" option that is parsed via OPT_RERERE_AUTOUPDATE (used in builtin/rebase--interactive.c among other built-in commands) on the other hand is tertially that uses 0 (unspecified), 1 (requested) and 2 (declined). This might be a ticking timebomb to confuse us in the future that may be worth fixing but probably outside this series.

Show 8 quoted lines
> +	if (opts->gpg_sign_opt)
> +		write_file(state_dir_path("gpg_sign_opt", opts), "%s",
> +			   opts->gpg_sign_opt);
> +	if (opts->signoff)
> +		write_file(state_dir_path("strategy", opts), "--signoff");
> +
> +	return 0;
> +}

The above looks like a literal translation of a helper function with the same name in git-rebase--common.sh. Good.

Show 14 quoted lines
> @@ -459,6 +490,30 @@ static int reset_head(struct object_id *oid, const char *action,
>  	return ret;
>  }
>  
> +static int move_to_original_branch(struct rebase_options *opts)
> +{
> +	struct strbuf orig_head_reflog = STRBUF_INIT, head_reflog = STRBUF_INIT;
> +	int ret;
> +
> +	if (!opts->head_name)
> +		return 0; /* nothing to move back to */
> +
> +	if (!opts->onto)
> +		BUG("move_to_original_branch without onto");

This check is absent in the scripted version, but from the message we generate here, it is clear that the caller must not call this when there is no "onto" commit. Good.

Show 7 quoted lines
> +	strbuf_addf(&orig_head_reflog, "rebase finished: %s onto %s",
> +		    opts->head_name, oid_to_hex(&opts->onto->object.oid));
> +	strbuf_addf(&head_reflog, "rebase finished: returning to %s",
> +		    opts->head_name);
> +	ret = reset_head(NULL, "checkout", opts->head_name,
> +			 RESET_HEAD_REFS_ONLY,
> +			 orig_head_reflog.buf, head_reflog.buf);

The *action given to reset_head() here is "checkout". Makes me wonder about two things:

 - The only real use of the parameter in the callee is to prepare
   the error and advice messages from the unpack_trees machinery,
   but because we are using it in REFS_ONLY mode, it does not
   matter.  In fact it might even be misleading; perhaps pass NULL
   or something, so that a mistaken update to reset_head() later
   that lets REFS_ONLY request to go to unpack_trees machinery will
   catch it as a bug?
 - Another topic in flight wants to make sure that the post-checkout
   hook gets called when the RESET_HEAD_RUN_POST_CHECKOUT_HOOK flag
   is given by the caller, and IIRC, the use of the flag is strongly
   correlated to *action being "checkout".  Do we want to pass
   REFS_ONLY and RUN_POST_CHECKOUT_HOOK flag for this call, or do we
   rather keep it silent?  As the original scripted version did not
   use "checkout" here and never triggered post-checkout hook, I am
   inclined to say that we should not pass that other bit.  That
   then leads me to suspect that we do not want *action to be
   "checkout" here.
> +	strbuf_release(&orig_head_reflog);
> +	strbuf_release(&head_reflog);
> +	return ret;
> +}

Unlike the scripted version, this does not die() upon failure, so the caller needs to be careful about the returned status.

Show 26 quoted lines
> @@ -466,6 +521,129 @@ N_("Resolve all conflicts manually, mark them as resolved with\n"
>  "To abort and get back to the state before \"git rebase\", run "
>  "\"git rebase --abort\".");
>  
> +static int run_am(struct rebase_options *opts)
> +{
> +	struct child_process am = CHILD_PROCESS_INIT;
> +	struct child_process format_patch = CHILD_PROCESS_INIT;
> +	struct strbuf revisions = STRBUF_INIT;
> +	int status;
> +	char *rebased_patches;
> +
> +	am.git_cmd = 1;
> +	argv_array_push(&am.args, "am");
> +
> +	if (opts->action && !strcmp("continue", opts->action)) {
> +		argv_array_push(&am.args, "--resolved");
> +		argv_array_pushf(&am.args, "--resolvemsg=%s", resolvemsg);
> +		if (opts->gpg_sign_opt)
> +			argv_array_push(&am.args, opts->gpg_sign_opt);
> +		status = run_command(&am);
> +		if (status)
> +			return status;
> +
> +		discard_cache();
> +		return move_to_original_branch(opts);

It is curious why discard_cache() is placed exacly here, as if we want to preserve the contents of the in-core index when run_command() failed. But I do not think we care about the in-core index as the only thing that happen after "return status" is to return the control to run_specific_rebase(), let it jump to finished_rebase label to clean things up and rturn control to cmd_rebase() and exit based on the status value.

It's not like move_to_original_branch() wants to call read_cache() and get the result from the "am" that run_command() executed, either.

Puzzled.  Care to explain a bit more in the in-code comment?
Show 10 quoted lines
> +	}
> +	if (opts->action && !strcmp("skip", opts->action)) {
> +		argv_array_push(&am.args, "--skip");
> +		argv_array_pushf(&am.args, "--resolvemsg=%s", resolvemsg);
> +		status = run_command(&am);
> +		if (status)
> +			return status;
> +
> +		discard_cache();
> +		return move_to_original_branch(opts);
Ditto.
Show 5 quoted lines
> +	}
> +	if (opts->action && !strcmp("show-current-patch", opts->action)) {
> +		argv_array_push(&am.args, "--show-current-patch");
> +		return run_command(&am);
> +	}

Up to this point, it is a faithful conversion of the first case/esac statement. Good.

> +	strbuf_addf(&revisions, "%s...%s",
> +		    oid_to_hex(opts->root ?
> +			       /* this is now equivalent to ! -z "$upstream" */
Does "this" refer to the "opts->root being true" check?

Because you are flipping the polarity of the test from scripted version, shouldn't the comment be updated to "-z $upstream"?

> +			       &opts->onto->object.oid :
> +			       &opts->upstream->object.oid),
> +		    oid_to_hex(&opts->orig_head));
> +	rebased_patches = xstrdup(git_path("rebased-patches"));
> +	format_patch.out = open(rebased_patches,
> +				O_WRONLY | O_CREAT | O_TRUNC, 0666);

Unlike scripted version, we do not remove a (possibly) existing file. We give CREAT in case there is no existing one, and TRUNC in case there is an existing one. Makes sense. A more faithful translation would have unlink(2)ed a (possibly) existing one, and then because we can afford to, passed O_EXCL to avoid stomping on somebody else racing with us, but I do not think it is worth it.

> +	if (format_patch.out < 0) {
> +		status = error_errno(_("could not write '%s'"),
> +				     rebased_patches);
s/write '%s'/open '%s' for writing/?  I dunno.
Show 17 quoted lines
> +		free(rebased_patches);
> +		argv_array_clear(&am.args);
> +		return status;
> +	}
> +
> +	format_patch.git_cmd = 1;
> +	argv_array_pushl(&format_patch.args, "format-patch", "-k", "--stdout",
> +			 "--full-index", "--cherry-pick", "--right-only",
> +			 "--src-prefix=a/", "--dst-prefix=b/", "--no-renames",
> +			 "--no-cover-letter", "--pretty=mboxrd", NULL);
> +	if (opts->git_format_patch_opt.len)
> +		argv_array_split(&format_patch.args,
> +				 opts->git_format_patch_opt.buf);
> +	argv_array_push(&format_patch.args, revisions.buf);
> +	if (opts->restrict_revision)
> +		argv_array_pushf(&format_patch.args, "^%s",
> +				 oid_to_hex(&opts->restrict_revision->object.oid));

It is kinda surprising to see that we have learned quite a lot of fringe "configurations" we need to explicitly override like this.

Looks like a quite faithful conversion, anyway.
Show 8 quoted lines
> +	status = run_command(&format_patch);
> +	if (status) {
> +		unlink(rebased_patches);
> +		free(rebased_patches);
> +		argv_array_clear(&am.args);
> +
> +		reset_head(&opts->orig_head, "checkout", opts->head_name, 0,
> +			   "HEAD", NULL);

This one may need to trigger post-checkout hook. The scripted version does two different things depending on the value of $head_name, but we can just use the same code without conditional?

Show 16 quoted lines
> +		error(_("\ngit encountered an error while preparing the "
> +			"patches to replay\n"
> +			"these revisions:\n"
> +			"\n    %s\n\n"
> +			"As a result, git cannot rebase them."),
> +		      opts->revisions);
> +
> +		strbuf_release(&revisions);
> +		return status;
> +	}
> +	strbuf_release(&revisions);
> +
> +	am.in = open(rebased_patches, O_RDONLY);
> +	if (am.in < 0) {
> +		status = error_errno(_("could not read '%s'"),
> +				     rebased_patches);
s/write '%s'/open '%s' for reading/?  I dunno.
Show 44 quoted lines
> +		free(rebased_patches);
> +		argv_array_clear(&am.args);
> +		return status;
> +	}
> +
> +	argv_array_pushv(&am.args, opts->git_am_opts.argv);
> +	argv_array_push(&am.args, "--rebasing");
> +	argv_array_pushf(&am.args, "--resolvemsg=%s", resolvemsg);
> +	argv_array_push(&am.args, "--patch-format=mboxrd");
> +	if (opts->allow_rerere_autoupdate > 0)
> +		argv_array_push(&am.args, "--rerere-autoupdate");
> +	else if (opts->allow_rerere_autoupdate == 0)
> +		argv_array_push(&am.args, "--no-rerere-autoupdate");
> +	if (opts->gpg_sign_opt)
> +		argv_array_push(&am.args, opts->gpg_sign_opt);
> +	status = run_command(&am);
> +	unlink(rebased_patches);
> +	free(rebased_patches);
> +
> +	if (!status) {
> +		discard_cache();
> +		return move_to_original_branch(opts);
> +	}
> +
> +	if (is_directory(opts->state_dir))
> +		write_basic_state(opts);
> +
> +	return status;
> +}
> +
>  static int run_specific_rebase(struct rebase_options *opts)
>  {
>  	const char *argv[] = { NULL, NULL };
> @@ -546,6 +724,11 @@ static int run_specific_rebase(struct rebase_options *opts)
>  		goto finished_rebase;
>  	}
>  
> +	if (opts->type == REBASE_AM) {
> +		status = run_am(opts);
> +		goto finished_rebase;
> +	}
> +
>  	add_var(&script_snippet, "GIT_DIR", absolute_path(get_git_dir()));
>  	add_var(&script_snippet, "state_dir", opts->state_dir);

Overall, this was quite a pleasant read and a well constructed series. Other than two minor points (i.e. interaction with the 'post-checkout hook' topic, and discard_cache() before calling move_to_original_branch) I did not quite understand, looks good to me.

When merged to 'pu', I seem to be getting failure from t3425.5, .8 and .11, by the way. I haven't dug into the actual breakages any further than that.

Thanks.
Previous: Johannes Schindelin via GitGitGadgetNext: Johannes Schindelin
Message 12 of 27 in “Let the builtin rebase call the git am command directly”
  1. 0/4 Let the builtin rebase call the git am command directlyJohannes Schindelin via GitGitGadget, Dec 21, 2018
  2. 2/4 rebase: avoid double reflog entry when switching branchesJohannes Schindelin via GitGitGadget, Dec 21, 2018
  3. Junio C HamanoJan 4, 2019
  4. Johannes SchindelinJan 18, 2019
  5. 1/4 rebase: move `reset_head()` into a better spotJohannes Schindelin via GitGitGadget, Dec 21, 2018
  6. Junio C HamanoJan 4, 2019
  7. 3/4 rebase: teach `reset_head()` to optionally skip the worktreeJohannes Schindelin via GitGitGadget, Dec 21, 2018
  8. Junio C HamanoJan 4, 2019
  9. Junio C HamanoJan 4, 2019
  10. Johannes SchindelinJan 18, 2019
  11. 4/4 built-in rebase: call `git am` directlyJohannes Schindelin via GitGitGadget, Dec 21, 2018
  12. Junio C HamanoJan 4, 2019
  13. Johannes SchindelinJan 18, 2019
  14. Junio C HamanoJan 18, 2019
  15. Johannes SchindelinJan 18, 2019
  16. Junio C HamanoJan 4, 2019
  17. Junio C HamanoJan 7, 2019
  18. 0/4 Let the builtin rebase call the git am command directlyJohannes Schindelin via GitGitGadget, Jan 18, 2019
  19. 1/4 rebase: move `reset_head()` into a better spotJohannes Schindelin via GitGitGadget, Jan 18, 2019
  20. 2/4 rebase: avoid double reflog entry when switching branchesJohannes Schindelin via GitGitGadget, Jan 18, 2019
  21. 3/4 rebase: teach `reset_head()` to optionally skip the worktreeJohannes Schindelin via GitGitGadget, Jan 18, 2019
  22. 4/4 built-in rebase: call `git am` directlyJohannes Schindelin via GitGitGadget, Jan 18, 2019
  23. Alban GruinJan 26, 2019
  24. Johannes SchindelinJan 29, 2019
  25. Junio C HamanoJan 18, 2019
  26. Junio C HamanoJan 18, 2019
  27. Johannes SchindelinJan 18, 2019

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.