Re: [PATCH v3 2/3] replay: make atomic ref updates the default behavior

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

Siddharth Asthana <siddharthasthana31@gmail.com> writes:

> For users needing the traditional pipeline workflow, add a new
> `--update-refs=<mode>` option that preserves the original behavior:
>
>   git replay --update-refs=print --onto main topic1..topic2 | git update-ref --stdin
>
> The mode can be:
>   * `yes` (default): Update refs directly using an atomic transaction
>   * `print`: Output update-ref commands for pipeline use

Is it only me who still finds this awkward?  A question "update?"
that gets answered "yes" is quite understandable, but it is not
immediately obvious what it means to answer "print" to the same
question.  When the user gives the latter mode as the answer to the
question, the question being answered is not really "do you want to
update refs?" at all.

The question the command wants the user to answer is more like "what
action do you want to see performed on the refs?", isn't it?  The
user would answer to the question with "please update them" to get
the default mode, while "please print them" may be the answer the
user would give to get the useful-for-dry-run-and-development mode.

Perhaps phrase it more like "--ref-action=(update|print)"?  I dunno.

>  --advance <branch>::
>  	Starting point at which to create the new commits; must be a
>  	branch name.
>  +
> -When `--advance` is specified, the update-ref command(s) in the output
> -will update the branch passed as an argument to `--advance` to point at
> -the new commits (in other words, this mimics a cherry-pick operation).
> +When `--advance` is specified, the branch passed as an argument will be
> +updated to point at the new commits (or an update command will be printed
> +if `--update-refs=print` is used). This mimics a cherry-pick operation.

I do not find it clear what the reference to cherry-pick is trying
to convey.  It is like cherry-picking <something> while the <branch>
is checked out (hence the branch advances as the result of acquiring
these commits from <something>)?  Let me see if I understood you by
attempting to rephrase.

    The history is replayed on top of the <branch> and <branch> is
    updated to point at the tip of resulting history.

But what's the significance of saying so?  Did you want to contrast
it with "rebase --onto <branch>", i.e. merely specifying the
starting point without <branch> itself moving as the result?  If so,
it is probably a notable distinction worth pointing out, but just
saying "mimics a cherry-pick operation" alone is probably not enough
to get the intended audience understand what you wanted to tell
them.

    Side note.  I casually wrote "is updated to point" but with the
    option not to update (but show the way to update refs), we'd
    probably need to find a good phrase to express "where the
    command _wants_ to see the refs pointing at as the result",
    without referring to who/how the refs are made to point at these
    points.

> -To simply rebase `mybranch` onto `target`:
> +To simply rebase `mybranch` onto `target` (default behavior):

"the default"?

> diff --git a/builtin/replay.c b/builtin/replay.c
> index b64fc72063..457225363e 100644
> --- a/builtin/replay.c
> +++ b/builtin/replay.c
> @@ -284,6 +284,26 @@ static struct commit *pick_regular_commit(struct repository *repo,
>  	return create_commit(repo, result->tree, pickme, replayed_base);
>  }
>  
> +static int handle_ref_update(const char *mode,
> +			     struct ref_transaction *transaction,
> +			     const char *refname,
> +			     const struct object_id *new_oid,
> +			     const struct object_id *old_oid,
> +			     struct strbuf *err)
> +{
> +	if (!strcmp(mode, "print")) {
> +		printf("update %s %s %s\n",
> +		       refname,
> +		       oid_to_hex(new_oid),
> +		       oid_to_hex(old_oid));
> +		return 0;
> +	}
> +
> +	/* mode == "yes" - update refs directly */
> +	return ref_transaction_update(transaction, refname, new_oid, old_oid,
> +				      NULL, NULL, 0, "git replay", err);
> +}

Hmph, would it be easier to follow if the above is symmetric, i.e.,

	if (...) {
		what happens in the "print" mode
	} else {
		what happens in the "update ourselves" mode
	}

I wonder?

In any case, do not pass mode as "const char *" around in the call
chain.  Instead, reduce it down to an enum or integer (with CPP
macro) at the earliest possible place after you saw the command line
option.  That would allow you to even do

	switch (ref_action) {
	case PRINT_INSN:
		printf("update ...");
		return 0;
	case UPDATE_OURSELVES:
		return ref_transaction_update(...);
	default:
		BUG("Bad ref_action %d", ref_action);
	}

to future-proof for the third option.

> +		OPT_STRING(0, "update-refs", &update_refs_mode,
> +			   N_("mode"),
> +			   N_("control ref update behavior (yes|print)")),
>  		OPT_END()
>  	};

This one is fine, but then immediately after parse_options()
returns, do something like

	if (!strcmp(update_refs_mode, "print"))
		ref_action = PRINT_INSN;
	else if (!strcmp(update_refs_mode, "yes"))
		ref_action = UPDATE_OURSELVES;
	else
		die(_("unknown option --update-ref='%s'"),
		    update_refs_mode);

so that you do not have to keep strcmp() with "print", which risks
you to mistype "prnit" and no compiler would protect against that.