Subject: Re: [PATCH v3 13/15] replay: add --advance or 'cherry-pick' mode

From: Toon Claes <[email-masked]>
Date: Thu, 22 Jun 2023 12:05:03 +0200

Christian Couder <[email-masked]> writes:

> +	/*
> +	 * When the user specifies e.g.
> +	 *   git replay origin/main..mybranch
> +	 *   git replay ^origin/next mybranch1 mybranch2

When I'm trying these, I'm getting the error:
    error: option --onto or --advance is mandatory

In what situation can I omit both --onto and --advance?

> +static void determine_replay_mode(struct rev_cmdline_info *cmd_info,
> +				  const char *onto_name,
> +				  const char **advance_name,
> +				  struct commit **onto,

Would it make sense to call this target?

> +				  struct strset **update_refs)
> +{
> +	struct ref_info rinfo;
> +
> +	get_ref_information(cmd_info, &rinfo);
> +	if (!rinfo.positive_refexprs)
> +		die(_("need some commits to replay"));
> +	if (onto_name && *advance_name)
> +		die(_("--onto and --advance are incompatible"));

Do we actually need to disallow this? I mean from git-replay's point of
view, there's no technical limitation why can cannot support both modes
at once. The update-ref commands in the output will update both target
and source branches, but it's not up to us whether that's desired.

> +	else if (onto_name) {

No need to 'else' here IMHO.

> +		*onto = peel_committish(onto_name);
> +		if (rinfo.positive_refexprs <
> +		    strset_get_size(&rinfo.positive_refs))
> +			die(_("all positive revisions given must be references"));

I tested this locally with the following command:

$ git replay --onto main OID..OID

This command didn't give any errors, neither did it return any
update-ref lines. I would have expected to hit this die().

> +	} else if (*advance_name) {
> +		struct object_id oid;
> +		char *fullname = NULL;
> +
> +		*onto = peel_committish(*advance_name);
> +		if (repo_dwim_ref(the_repository, *advance_name, strlen(*advance_name),
> +			     &oid, &fullname, 0) == 1) {
> +			*advance_name = fullname;
> +		} else {
> +			die(_("argument to --advance must be a reference"));
> +		}
> +		if (rinfo.positive_refexprs > 1)
> +			die(_("cannot advance target with multiple source branches because ordering would be ill-defined"));

The sources aren't always branches, so I suggest something like:

+			die(_("cannot advance target with multiple sources because ordering would be ill-defined"));

> +	determine_replay_mode(&revs.cmdline, onto_name, &advance_name,
> +			      &onto, &update_refs);
> +
> +	if (!onto) /* FIXME: Should handle replaying down to root commit */
> +		die("Replaying down to root commit is not supported yet!");

When I was testing locally I tried the following:

$ git replay --onto main feature

I was expecting this command to find the common ancestor automatically,
but instead I got this error. I'm fine if for now the command does not
determine the common ancestor yet, but I think we should provide a
better error for this scenario.

> +test_expect_success 'using replay on bare repo to perform basic cherry-pick' '
> +	git -C bare replay --advance main topic1..topic2 >result-bare &&
> +	test_cmp expect result-bare
> +'
> +
>  test_done

Shall we add a test case when providing both --onto and --advance? And
one that omits both?