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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 13, 2025, 22:05 UTC
Message-ID: <xmqqms5uzcd7.fsf@gitster.g>
In-Reply-To: <20251013183311.33329-3-siddharthasthana31@gmail.com>

Siddharth Asthana <siddharthasthana31@gmail.com> writes:

Show 8 quoted lines

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

Show 10 quoted lines

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

Show 27 quoted lines

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

Show 5 quoted lines

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