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

From: Toon Claes <toon@iotcl.com>
Date: Jun 22, 2023, 10:05 UTC
Message-ID: <87h6qzst8u.fsf@iotcl.com>
In-Reply-To: <20230602102533.876905-14-christian.couder@gmail.com>

Christian Couder <christian.couder@gmail.com> 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?

Show 9 quoted lines

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

Show 13 quoted lines

> +	} 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"));

Show 5 quoted lines

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

Show 6 quoted lines

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