Hi Junio,
On Fri, 4 Jan 2019, Junio C Hamano wrote:
> "Johannes Schindelin via GitGitGadget" <gitgitgadget@xxxxxxxxx>
> 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.
Good point. We use -1 for unspecified in so many places, I think
OPT_RERERE_AUTOUPDATE needs to be fixed. But yes, I'll leave this as
#leftoverbits here.
> > @@ -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.
The only thing for which that the `action` is used, though, is the call to
`setup_unpack_trees_porcelain()`, which does not accept a `NULL`. I guess
I could replace it by the empty string. Will do that.
>
> > + 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.
Indeed. That function is only called from `run_am()`, and returns the
status in every instance. The caller of `run_am()`,
`run_specific_rebase()` also handles it correctly.
>
> > @@ -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?
I think that this call is just a left-over from a previous version that
did not have the REFS_ONLY flag to pass to `move_to_original_branch()`
(and it caused havoc before that flag was passed). Let me double-check
whether the `discard_cache()` even makes sense any longer.
*clicketyclick* indeed that is the case. Will remove all three
`discard_cache()` calls.
>
> > + }
> > + 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"?
It did flip the polarity, you are right, this comment is incorrect. It is
even more incorrect, though, as it talks about a shell construct that is
no longer applicable. Will fix.
>
> > + &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.
Okay.
> > + if (format_patch.out < 0) {
> > + status = error_errno(_("could not write '%s'"),
> > + rebased_patches);
>
> s/write '%s'/open '%s' for writing/? I dunno.
Yep, of course! Will fix.
> > + 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?
Yes, because `opts->head_name` is `NULL` in one case, and not `NULL` in
the other, and the `reset_head()` function performs the desired operation
in each case.
> > + 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.
Yep, will fix.
>
> > + 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.
Sorry for the trouble, and for my silence (I was heads-down into the Azure
Pipelines support).
I did not see any breakage in `pu` lately, hopefully things resolved
themselves?
Ciao,
Dscho
