[RFC PATCH 0/1] rebase --onto detection of already applied commits

[RFC PATCH 0/1] rebase --onto detection of already applied commits

[Cristian Ciocaltea]

Let's consider the following operation:

  git rebase --onto new-base upstream feature

where 'feature' contains a few commits on top of 'upstream' which need to be
rebased onto 'new-base'.

The problem is that some of those commits have been already applied to
'new-base' and we would like to skip them as in a regular rebase command that
doesn't use '--onto', i.e. expecting to see messages like:

  warning: skipped previously applied commit [...]

However, that doesn't happen and we either get

  dropping [...] -- patch contents already upstream

or a conflict if one of the rebased commits doesn't resolve to an empty patch
anymore, e.g. due to additional changes applied on the target branch. 

This seems to be similar to the behavior of '--reapply-cherry-picks' and cannot
be disabled via '--no-reapply-cherry-picks' or by any other means.

The proposed patch is just a quick workaround, as I'm not sure what would be the
proper or recommended approach to handle the scenario described. Any advice
would be highly appreciated!

Cristian Ciocaltea (1):
  rebase --onto: Skip previously applied commits

 builtin/rebase.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

-- 
2.38.1

[RFC PATCH 1/1] rebase --onto: Skip previously applied commits

[Cristian Ciocaltea]

When rebase is used with '--onto <newbase>', the patches that might have
been already applied on <newbase> are not detected, unless they resolve
to an empty commit. When the related files contain additional changes
merged, the rebase operation fails due to conflicts that require manual
intervention.

Ensure the '--onto' variant behaviour is consistent with the common
rebase by dropping the already applied commits on the target branch.

Note the current behavior is still reachable by using the
'--reapply-cherry-picks' flag.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 builtin/rebase.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index b22768ca5b9f..2907c6db5cce 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1659,8 +1659,12 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		strbuf_addstr(&buf, "...");
 		strbuf_addstr(&buf, branch_name);
 		options.onto_name = xstrdup(buf.buf);
-	} else if (!options.onto_name)
+	} else if (!options.onto_name) {
 		options.onto_name = options.upstream_name;
+	} else if (options.upstream) {
+		options.restrict_revision = options.upstream;
+		options.upstream = NULL;
+	}
 	if (strstr(options.onto_name, "...")) {
 		if (get_oid_mb(options.onto_name, &branch_base) < 0) {
 			if (keep_base)
-- 
2.38.1

Re: [RFC PATCH 1/1] rebase --onto: Skip previously applied commits

[Ævar Arnfjörð Bjarmason]

On Mon, Dec 12 2022, Cristian Ciocaltea wrote:

> When rebase is used with '--onto <newbase>', the patches that might have
> been already applied on <newbase> are not detected, unless they resolve
> to an empty commit. When the related files contain additional changes
> merged, the rebase operation fails due to conflicts that require manual
> intervention.
>
> Ensure the '--onto' variant behaviour is consistent with the common
> rebase by dropping the already applied commits on the target branch.
>
> Note the current behavior is still reachable by using the
> '--reapply-cherry-picks' flag.
>
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---
>  builtin/rebase.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index b22768ca5b9f..2907c6db5cce 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -1659,8 +1659,12 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  		strbuf_addstr(&buf, "...");
>  		strbuf_addstr(&buf, branch_name);
>  		options.onto_name = xstrdup(buf.buf);
> -	} else if (!options.onto_name)
> +	} else if (!options.onto_name) {
>  		options.onto_name = options.upstream_name;
> +	} else if (options.upstream) {
> +		options.restrict_revision = options.upstream;
> +		options.upstream = NULL;
> +	}
>  	if (strstr(options.onto_name, "...")) {
>  		if (get_oid_mb(options.onto_name, &branch_base) < 0) {
>  			if (keep_base)

When I apply this & run the tests e.g. t3418 will segfault, and t3403
seems to fail due to the new behavior not having adjusted the test.

Which would be my "C" for the "RFC" :)

I.e. try to get the tests working, and not segfaulting.

If this is a good idea UX wise (I haven't formed an opinion) the main
thing that should inform that is having to decide on the various cases
that the tests are checking for already.

Do I understand this correctly that we'll currently stop and requise a
"git rebase --continue" if we have an empty patch with "--onto", but
without "--onto" we'll just print a warning, and that you'd like
"--onto" to be consistent with the non-onto case?

Re: [RFC PATCH 1/1] rebase --onto: Skip previously applied commits

[Cristian Ciocaltea]

On 12/12/22 14:27, Ævar Arnfjörð Bjarmason wrote:
> 
> On Mon, Dec 12 2022, Cristian Ciocaltea wrote:
> 
>> When rebase is used with '--onto <newbase>', the patches that might have
>> been already applied on <newbase> are not detected, unless they resolve
>> to an empty commit. When the related files contain additional changes
>> merged, the rebase operation fails due to conflicts that require manual
>> intervention.
>>
>> Ensure the '--onto' variant behaviour is consistent with the common
>> rebase by dropping the already applied commits on the target branch.
>>
>> Note the current behavior is still reachable by using the
>> '--reapply-cherry-picks' flag.
>>
>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>> ---
>>   builtin/rebase.c | 6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/builtin/rebase.c b/builtin/rebase.c
>> index b22768ca5b9f..2907c6db5cce 100644
>> --- a/builtin/rebase.c
>> +++ b/builtin/rebase.c
>> @@ -1659,8 +1659,12 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>>   		strbuf_addstr(&buf, "...");
>>   		strbuf_addstr(&buf, branch_name);
>>   		options.onto_name = xstrdup(buf.buf);
>> -	} else if (!options.onto_name)
>> +	} else if (!options.onto_name) {
>>   		options.onto_name = options.upstream_name;
>> +	} else if (options.upstream) {
>> +		options.restrict_revision = options.upstream;
>> +		options.upstream = NULL;
>> +	}
>>   	if (strstr(options.onto_name, "...")) {
>>   		if (get_oid_mb(options.onto_name, &branch_base) < 0) {
>>   			if (keep_base)
> 
> When I apply this & run the tests e.g. t3418 will segfault, and t3403
> seems to fail due to the new behavior not having adjusted the test.

Right, a bunch of the merge related tests are now failing, which is one 
of the reasons I submitted the patch as RFC. If this is not a (totally) 
wrong solution to the problem, I will try to get all the tests working 
in a subsequent patch revision.

> Which would be my "C" for the "RFC" :)
> 
> I.e. try to get the tests working, and not segfaulting.
> 
> If this is a good idea UX wise (I haven't formed an opinion) the main
> thing that should inform that is having to decide on the various cases
> that the tests are checking for already.
> 
> Do I understand this correctly that we'll currently stop and requise a
> "git rebase --continue" if we have an empty patch with "--onto", but
> without "--onto" we'll just print a warning, and that you'd like
> "--onto" to be consistent with the non-onto case?

The main use case I'm trying to address is the one described in the 
cover letter, mainly to avoid getting conflicts for the already applied 
commits when the rebase will not result in an empty patch anymore. This 
would be, indeed, consistent with the non-onto rebase variant.

Currently, "rebase --onto" behaves like "--reapply-cherry-picks" is 
being enforced, without having the possibility to disable it, which I 
found rather confusing and not expected.

Thanks for the quick feedback,
Cristian

Re: [RFC PATCH 0/1] rebase --onto detection of already applied commits

[Junio C Hamano]

Cristian Ciocaltea <cristian.ciocaltea@collabora.com> writes:

> Let's consider the following operation:
>
>   git rebase --onto new-base upstream feature
>
> where 'feature' contains a few commits on top of 'upstream' which need to be
> rebased onto 'new-base'.

Isn't it what "git rebase new-base feature" for?  "My 'feature'
forked from where 'new-base' came from but they updated 'new-base'
so I do not know if some of what I had in my 'feature' is in
theirs. Please forward port what is still left in 'feature' on top
of updated 'new-base' that I just got from them".

The primary reason why we have an explicit "--onto" is so that
"rebase" is used just like

	git checkout --detach new-base
	git cherry-pick upstream..feature
	git checkout -B feature

to deal with a different situation, i.e. "My 'feature' forked from
'upstream', and I have a commit 'new-base'.  Just transplant the
whole thing on top of it", without having to worry about "what is
already in new-base?" at all.  After all, 'new-base' may not have
ANY ancestry relationship with the 'upstream', so "inspect commits
in the range upstream..new-base to exclude those that are the same
as the ones in upstream..feature" is not a well defined operation.

Re: [RFC PATCH 0/1] rebase --onto detection of already applied commits

[Cristian Ciocaltea]

On 12/13/22 02:13, Junio C Hamano wrote:
> Cristian Ciocaltea <cristian.ciocaltea@collabora.com> writes:
> 
>> Let's consider the following operation:
>>
>>    git rebase --onto new-base upstream feature
>>
>> where 'feature' contains a few commits on top of 'upstream' which need to be
>> rebased onto 'new-base'.
> 
> Isn't it what "git rebase new-base feature" for?  "My 'feature'
> forked from where 'new-base' came from but they updated 'new-base'
> so I do not know if some of what I had in my 'feature' is in
> theirs. Please forward port what is still left in 'feature' on top
> of updated 'new-base' that I just got from them".

I should have highlighted that 'feature' contains a bunch of commits 
that must not be rebased on top of 'new-base', but only the ones in the 
'upstream..feature' range need to be considered.

> The primary reason why we have an explicit "--onto" is so that
> "rebase" is used just like
> 
> 	git checkout --detach new-base
> 	git cherry-pick upstream..feature
> 	git checkout -B feature
> 
> to deal with a different situation, i.e. "My 'feature' forked from
> 'upstream', and I have a commit 'new-base'.  Just transplant the
> whole thing on top of it", without having to worry about "what is
> already in new-base?" at all.  After all, 'new-base' may not have
> ANY ancestry relationship with the 'upstream', so "inspect commits
> in the range upstream..new-base to exclude those that are the same
> as the ones in upstream..feature" is not a well defined operation.
> 

Thanks for clarifying the intended use case of '--onto'. I have wrongly 
assumed it could be used as a more flexible rebase alternative, allowing 
one to limit the range of commits to be considered for rebasing on top 
of a given base, without losing the feature of detecting the already 
applied commits.

Currently '--onto' works as if the user provided the 
'--reapply-cherry-picks' flag, so maybe a possible improvement of this 
patch could be to handle '--no-reapply-cherry-picks' to explicitly 
enable the detection.

Would this be an acceptable extension?

Thanks,
Cristian

Re: [RFC PATCH 0/1] rebase --onto detection of already applied commits

[Junio C Hamano]

Cristian Ciocaltea <cristian.ciocaltea@collabora.com> writes:

> Currently '--onto' works as if the user provided the
> '--reapply-cherry-picks' flag, so maybe a possible improvement of this
> patch could be to handle '--no-reapply-cherry-picks' to explicitly
> enable the detection.

That sounds like a workable approach that breaks no existing users.

Re: [RFC PATCH 0/1] rebase --onto detection of already applied commits

[Phillip Wood]

Hi Christian

On 13/12/2022 10:37, Cristian Ciocaltea wrote:
> Currently '--onto' works as if the user provided the 
> '--reapply-cherry-picks' flag,

--onto does not affect the cherry-pick detection. When running

	git rebase --onto new-base upstream feature

any commits in upstream have been cherry-picked from feature they will 
not be rebased. What it does not do is look for cherry-picks in 
onto...feature. It would be nice to add that but I'm not sure it is 
straight forward to do so and still exclude commits that have been 
cherry-picked from feature to upstream.

Best Wishes

Phillip

Re: [RFC PATCH 0/1] rebase --onto detection of already applied commits

[Cristian Ciocaltea]

Hi Phillip,

On 12/13/22 15:04, Phillip Wood wrote:
> Hi Christian
> 
> On 13/12/2022 10:37, Cristian Ciocaltea wrote:
>> Currently '--onto' works as if the user provided the 
>> '--reapply-cherry-picks' flag,
> 
> --onto does not affect the cherry-pick detection. When running
> 
>      git rebase --onto new-base upstream feature
> 
> any commits in upstream have been cherry-picked from feature they will 
> not be rebased. What it does not do is look for cherry-picks in 
> onto...feature. It would be nice to add that but I'm not sure it is 
> straight forward to do so and still exclude commits that have been 
> cherry-picked from feature to upstream.

The proposed patch enables looking for commits into new-base..feature 
range and excluding the ones reachable from upstream. Since this is a 
change in the existing behavior, we might need to introduce a new flag 
to enable it. I previously suggested to use '--no-reapply-cherry-picks' 
for this purpose, but now it's pretty obvious this will be a source of 
confusion, since the "cherry-picks" term refers to the commits picked 
from feature to upstream instead of new-base, as you already mentioned.

I agree it would be nice to support both exclusion ranges, but I'm not 
sure how complicated the implementation would be, since I don't have any 
previous experience with the Git internals. Could this be added as a 
separate feature at a later point?

Thanks,
Cristian

> Best Wishes
> 
> Phillip

Re: [RFC PATCH 0/1] rebase --onto detection of already applied commits

[Phillip Wood]

Hi Cristian

On 13/12/2022 15:34, Cristian Ciocaltea wrote:
> Hi Phillip,
> 
> On 12/13/22 15:04, Phillip Wood wrote:
>> Hi Christian
>>
>> On 13/12/2022 10:37, Cristian Ciocaltea wrote:
>>> Currently '--onto' works as if the user provided the 
>>> '--reapply-cherry-picks' flag,
>>
>> --onto does not affect the cherry-pick detection. When running
>>
>>      git rebase --onto new-base upstream feature
>>
>> any commits in upstream have been cherry-picked from feature they will 
>> not be rebased. What it does not do is look for cherry-picks in 
>> onto...feature. It would be nice to add that but I'm not sure it is 
>> straight forward to do so and still exclude commits that have been 
>> cherry-picked from feature to upstream.
> 
> The proposed patch enables looking for commits into new-base..feature 
> range and excluding the ones reachable from upstream. Since this is a 
> change in the existing behavior, we might need to introduce a new flag 
> to enable it. I previously suggested to use '--no-reapply-cherry-picks' 
> for this purpose, but now it's pretty obvious this will be a source of 
> confusion, since the "cherry-picks" term refers to the commits picked 
> from feature to upstream instead of new-base, as you already mentioned.
> 
> I agree it would be nice to support both exclusion ranges, but I'm not 
> sure how complicated the implementation would be, since I don't have any 
> previous experience with the Git internals. Could this be added as a 
> separate feature at a later point?

If we can I'd rather add code that excludes cherry-pick both ranges. To 
remove the cherry-picks that are in upstream and new-base you could 
rework the todo list generation as follows

1. Calculate the merge-base $mb of feature and upstream
2. Store the list of commits $mb..feature in an array and in a hash
    table indexed their patch-id.
3. Walk $mb..upstream calculating the patch-id for each commit and
    removing any commit in the list from step 2 that matches.
4. If onto is equal to upstream skip to step 7
5. Calculate the merge-base $mb of feature and onto.
6. Walk $mb..new-base calculating the patch-id for each commit and
    removing any commit in the list from step 2 that matches.
7. Generate the todo list using the modified list of commits from step
    2.

I don't have much time at the moment but I can try and help a bit more 
in the New Year if you want.

Best Wishes

Phillip

Re: [RFC PATCH 0/1] rebase --onto detection of already applied commits

[Cristian Ciocaltea]

Hi Phillip,

On 12/15/22 17:40, Phillip Wood wrote:
> Hi Cristian
> 
> On 13/12/2022 15:34, Cristian Ciocaltea wrote:
>> Hi Phillip,
>>
>> On 12/13/22 15:04, Phillip Wood wrote:
>>> Hi Christian
>>>
>>> On 13/12/2022 10:37, Cristian Ciocaltea wrote:
>>>> Currently '--onto' works as if the user provided the 
>>>> '--reapply-cherry-picks' flag,
>>>
>>> --onto does not affect the cherry-pick detection. When running
>>>
>>>      git rebase --onto new-base upstream feature
>>>
>>> any commits in upstream have been cherry-picked from feature they 
>>> will not be rebased. What it does not do is look for cherry-picks in 
>>> onto...feature. It would be nice to add that but I'm not sure it is 
>>> straight forward to do so and still exclude commits that have been 
>>> cherry-picked from feature to upstream.
>>
>> The proposed patch enables looking for commits into new-base..feature 
>> range and excluding the ones reachable from upstream. Since this is a 
>> change in the existing behavior, we might need to introduce a new flag 
>> to enable it. I previously suggested to use 
>> '--no-reapply-cherry-picks' for this purpose, but now it's pretty 
>> obvious this will be a source of confusion, since the "cherry-picks" 
>> term refers to the commits picked from feature to upstream instead of 
>> new-base, as you already mentioned.
>>
>> I agree it would be nice to support both exclusion ranges, but I'm not 
>> sure how complicated the implementation would be, since I don't have 
>> any previous experience with the Git internals. Could this be added as 
>> a separate feature at a later point?
> 
> If we can I'd rather add code that excludes cherry-pick both ranges. To 
> remove the cherry-picks that are in upstream and new-base you could 
> rework the todo list generation as follows
> 
> 1. Calculate the merge-base $mb of feature and upstream
> 2. Store the list of commits $mb..feature in an array and in a hash
>     table indexed their patch-id.
> 3. Walk $mb..upstream calculating the patch-id for each commit and
>     removing any commit in the list from step 2 that matches.
> 4. If onto is equal to upstream skip to step 7
> 5. Calculate the merge-base $mb of feature and onto.
> 6. Walk $mb..new-base calculating the patch-id for each commit and
>     removing any commit in the list from step 2 that matches.
> 7. Generate the todo list using the modified list of commits from step
>     2.
> 
> I don't have much time at the moment but I can try and help a bit more 
> in the New Year if you want.

Thank you for the implementation hints and your availability to help 
further! I will try to put this in practice and let you know as soon as 
I get something working.

Kind regards,
Cristian

> Best Wishes
> 
> Phillip

Re: [RFC PATCH 0/1] rebase --onto detection of already applied commits

[Phillip Wood]

On 15/12/2022 16:02, Cristian Ciocaltea wrote:
> Hi Phillip,
> 
> On 12/15/22 17:40, Phillip Wood wrote:
>> Hi Cristian
>>
>> On 13/12/2022 15:34, Cristian Ciocaltea wrote:
>>> Hi Phillip,
>>>
>>> On 12/13/22 15:04, Phillip Wood wrote:
>>>> Hi Christian
>>>>
>>>> On 13/12/2022 10:37, Cristian Ciocaltea wrote:
>>>>> Currently '--onto' works as if the user provided the 
>>>>> '--reapply-cherry-picks' flag,
>>>>
>>>> --onto does not affect the cherry-pick detection. When running
>>>>
>>>>      git rebase --onto new-base upstream feature
>>>>
>>>> any commits in upstream have been cherry-picked from feature they 
>>>> will not be rebased. What it does not do is look for cherry-picks in 
>>>> onto...feature. It would be nice to add that but I'm not sure it is 
>>>> straight forward to do so and still exclude commits that have been 
>>>> cherry-picked from feature to upstream.
>>>
>>> The proposed patch enables looking for commits into new-base..feature 
>>> range and excluding the ones reachable from upstream. Since this is a 
>>> change in the existing behavior, we might need to introduce a new 
>>> flag to enable it. I previously suggested to use 
>>> '--no-reapply-cherry-picks' for this purpose, but now it's pretty 
>>> obvious this will be a source of confusion, since the "cherry-picks" 
>>> term refers to the commits picked from feature to upstream instead of 
>>> new-base, as you already mentioned.
>>>
>>> I agree it would be nice to support both exclusion ranges, but I'm 
>>> not sure how complicated the implementation would be, since I don't 
>>> have any previous experience with the Git internals. Could this be 
>>> added as a separate feature at a later point?
>>
>> If we can I'd rather add code that excludes cherry-pick both ranges. 
>> To remove the cherry-picks that are in upstream and new-base you could 
>> rework the todo list generation as follows
>>
>> 1. Calculate the merge-base $mb of feature and upstream
>> 2. Store the list of commits $mb..feature in an array and in a hash
>>     table indexed their patch-id.
>> 3. Walk $mb..upstream calculating the patch-id for each commit and
>>     removing any commit in the list from step 2 that matches.
>> 4. If onto is equal to upstream skip to step 7
>> 5. Calculate the merge-base $mb of feature and onto.
>> 6. Walk $mb..new-base calculating the patch-id for each commit and
>>     removing any commit in the list from step 2 that matches.
>> 7. Generate the todo list using the modified list of commits from step
>>     2.
>>
>> I don't have much time at the moment but I can try and help a bit more 
>> in the New Year if you want.
> 
> Thank you for the implementation hints and your availability to help 
> further! I will try to put this in practice and let you know as soon as 
> I get something working.

I'd start by looking at the existing todo list generation in 
sequencer.c:sequencer_make_script()

Best Wishes

Phillip