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

[Junio C Hamano <gitster@pobox.com>]

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

> +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.

> +	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.

> @@ -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.

> +	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.

> @@ -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?

> +	}
> +	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.

> +	}
> +	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.

> +		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.

> +	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?

> +		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.

> +		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.