threads / discuss / 35513

Unexpected cherry-pick behaviour

Subject: Unexpected cherry-pick behaviour

## tl;dr

13 messages between Dec 10, 2013 and Dec 16, 2013.

replies: 12people: 4as markdown or json

Paulo Matos· Dec 10, 2013, 11:04 UTC · lore
Hi,

I have installed latest 1.8.5.1 git to confirm the behaviour I had seen in previous versions.

What I see is that when I cherry-pick a patch across two branches (source and destination) in a repository, cherry-pick picks changes from the source branch which do not exist in the cherry-picked patch.

To reproduce please follow the following in a clean directory (apologies for the large repo I use as example): $ git clone git://gcc.gnu.org/git/gcc.git $ cd gcc $ git checkout -b gcc-4_8-branch origin/gcc-4_8-branch $ cd gcc $ sed -i '877i myport_hook ()' tree-ssa-threadedge.c $ git diff tree-ssa-threadedge.c index b31e961..f022eed 100644

--- a/gcc/tree-ssa-threadedge.c
+++ b/gcc/tree-ssa-threadedge.c
@@ -874,6 +874,7 @@ thread_across_edge (gimple dummy_cond,
        if (cond_arg_set_in_bb (e, e->dest))
         goto fail;
      }
+myport_hook ()

    stmt_count = 0;
$ git add tree-ssa-threadedge.c
$ git commit -m 'cherry-pick test'
[gcc-4_8-branch 49a2b7f] cherry-pick test
  1 file changed, 1 insertion(+)
$ git checkout master
$ git cherry-pick 49a2b7f # ensure you're cherry-picking the right sha
error: could not apply 49a2b7f... cherry-pick test
hint: after resolving the conflicts, mark the corrected paths
hint: with 'git add <paths>' or 'git rm <paths>'
hint: and commit the result with 'git commit'
$ git diff tree-ssa-threadedge.c
diff --cc gcc/tree-ssa-threadedge.c
index cb6accf,f022eed..0000000
--- a/gcc/tree-ssa-threadedge.c
+++ b/gcc/tree-ssa-threadedge.c
@@@ -936,34 -854,33 +936,47 @@@ thread_around_empty_blocks (edge taken_
      STACK is used to undo temporary equivalences created during the 
walk of
      E->dest.

  -   SIMPLIFY is a pass-specific function used to simplify statements.  
*/
  -
  -void
  -thread_across_edge (gimple dummy_cond,
  -                  edge e,
  -                  bool handle_dominating_asserts,
  -                  vec<tree> *stack,
  -                  tree (*simplify) (gimple, gimple))
  -{
  -  gimple stmt;
  +   SIMPLIFY is a pass-specific function used to simplify statements.

++<<<<<<< HEAD
  +   Our caller is responsible for restoring the state of the expression
  +   and const_and_copies stacks.  */
++=======
+   /* If E is a backedge, then we want to verify that the COND_EXPR,
+      SWITCH_EXPR or GOTO_EXPR at the end of e->dest is not affected
+      by any statements in e->dest.  If it is affected, then it is not
+      safe to thread this edge.  */
+   if (e->flags & EDGE_DFS_BACK)
+     {
+       if (cond_arg_set_in_bb (e, e->dest))
+       goto fail;
+     }
+ myport_hook ()
++>>>>>>> 49a2b7f... cherry-pick test

  -  stmt_count = 0;
  +static bool
  +thread_through_normal_block (edge e,
  +                           gimple dummy_cond,
  +                           bool handle_dominating_asserts,
  +                           vec<tree> *stack,
  +                           tree (*simplify) (gimple, gimple),
  +                           vec<jump_thread_edge *> *path,
  +                           bitmap visited,
  +                           bool *backedge_seen_p,
  +                           bitmap src_map,
  +                           bitmap dst_map)
  +{
  +  /* If we have traversed a backedge, then we do not want to look
  +     at certain expressions in the table that can not be relied upon.
  +     Luckily the only code that looked at those expressions is the
  +     SIMPLIFY callback, which we replace if we can no longer use it.  
*/
  +  if (*backedge_seen_p)
  +    simplify = dummy_simplify;

     /* PHIs create temporary equivalences.  */
  -  if (!record_temporary_equivalences_from_phis (e, stack))
  -    goto fail;
  +  if (!record_temporary_equivalences_from_phis (e, stack, 
*backedge_seen_p,
  +                                              src_map, dst_map))
  +    return false;

     /* Now walk each statement recording any context sensitive
        temporary equivalences we can detect.  */


Note how there are changes that are not part of the cherry-picked patch 
outside of the conflicting zone. This is trouble some because it means 
that when I go in to fix a patch and look only at the conflicting zone, 
I will have code outside the zone, that are _not_ part of the patch 
modified as well.

Is this a bug or a feature? If the latter, why this behaviour and how 
can I avoid it?

Cheers,
-- 
Paulo Matos
Junio C Hamano· Dec 10, 2013, 19:34 UTC · re: Paulo Matos · lore

Re: Unexpected cherry-pick behaviour

Paulo Matos <paulo@matos-sorge.com> writes:
Show 5 quoted lines
> Note how there are changes that are not part of the cherry-picked
> patch outside of the conflicting zone. This is trouble some because it
> means that when I go in to fix a patch and look only at the
> conflicting zone, I will have code outside the zone, that are _not_
> part of the patch modified as well.

My suspicion (I am too lazy to try it myself ;-) is that your addition of a line "myport_hook()" was done to a section of code that was modified between the original you added the line to and the target you are trying to cherry-pick your change (that will inevitably cause conflicts and you cannot avoid that), and the post-processing done to make the three-way merge result more readable are ejecting common changes out of the conflicted region.

Perhaps immediately after "cherry-pick" stopped and asked your help to resolve the conflicts, running

	$ git checkout --conflicts=diff3 gcc/tree-ssa-threadedge.c
and looking at the file again may show you what is going on better.
Paulo Matos· Dec 11, 2013, 10:04 UTC · re: Junio C Hamano · lore

Re: Unexpected cherry-pick behaviour

On 10/12/2013 19:34, Junio C Hamano wrote:
Show 6 quoted lines
> Perhaps immediately after "cherry-pick" stopped and asked your help
> to resolve the conflicts, running
> 
> 	$ git checkout --conflicts=diff3 gcc/tree-ssa-threadedge.c
> 
> and looking at the file again may show you what is going on better.

I don't know how to interpret the fact that the line you sent (with the obvious --conflicts being --conflict) outputs nothing...

Any suggestions?
-- 
Paulo Matos
Antoine Pelisse· Dec 11, 2013, 11:09 UTC · re: Paulo Matos · lore

Re: Unexpected cherry-pick behaviour

On Wed, Dec 11, 2013 at 11:04 AM, Paulo Matos <paulo@matos-sorge.com> wrote:
Show 12 quoted lines
> On 10/12/2013 19:34, Junio C Hamano wrote:
>>
>> Perhaps immediately after "cherry-pick" stopped and asked your help
>> to resolve the conflicts, running
>>
>>         $ git checkout --conflicts=diff3 gcc/tree-ssa-threadedge.c
>>
>> and looking at the file again may show you what is going on better.
>
>
> I don't know how to interpret the fact that the line you sent (with the
> obvious --conflicts being --conflict) outputs nothing...

That is expected. git-checkout with this option [1] will reset the conflict on gcc/tree-ssa-threadedge.c file to the initial conflict state, and use the diff3 markers. You should have a new look at that file as you will now be able to see the "ancestor" in the conflict.

[1] You can have a look either at git-checkout manpage or here: http://git-scm.com/docs/git-checkout, especially --merge and --conflict options.

Paulo Matos· Dec 11, 2013, 11:19 UTC · re: Antoine Pelisse · lore

Re: Unexpected cherry-pick behaviour

On 11/12/2013 11:09, Antoine Pelisse wrote:
Show 14 quoted lines
>> 
>> I don't know how to interpret the fact that the line you sent (with 
>> the
>> obvious --conflicts being --conflict) outputs nothing...
> 
> That is expected. git-checkout with this option [1] will reset the
> conflict on gcc/tree-ssa-threadedge.c file to the initial conflict
> state, and use the diff3 markers. You should have a new look at that
> file as you will now be able to see the "ancestor" in the conflict.
> 
> [1] You can have a look either at git-checkout manpage or here:
> http://git-scm.com/docs/git-checkout, especially --merge and
> --conflict options.
> --

Got it, but still not helpful as git is still modifying code out of the conflicting zone.

$ git checkout --conflict=diff3 tree-ssa-threadedge.c $ git diff tree-ssa-threadedge.c diff --cc gcc/tree-ssa-threadedge.c index cb6accf,f022eed..0000000

--- a/gcc/tree-ssa-threadedge.c
+++ b/gcc/tree-ssa-threadedge.c
@@@ -936,34 -854,33 +936,57 @@@ thread_around_empty_blocks (edge taken_
      STACK is used to undo temporary equivalences created during the 
walk of
      E->dest.

  -   SIMPLIFY is a pass-specific function used to simplify statements.  
*/
  -
  -void
  -thread_across_edge (gimple dummy_cond,
  -                  edge e,
  -                  bool handle_dominating_asserts,
  -                  vec<tree> *stack,
  -                  tree (*simplify) (gimple, gimple))
  -{
  -  gimple stmt;
  +   SIMPLIFY is a pass-specific function used to simplify statements.

++<<<<<<< ours
  +   Our caller is responsible for restoring the state of the expression
  +   and const_and_copies stacks.  */
++||||||| base
++  /* If E is a backedge, then we want to verify that the COND_EXPR,
++     SWITCH_EXPR or GOTO_EXPR at the end of e->dest is not affected
++     by any statements in e->dest.  If it is affected, then it is not
++     safe to thread this edge.  */
++  if (e->flags & EDGE_DFS_BACK)
++    {
++      if (cond_arg_set_in_bb (e, e->dest))
++      goto fail;
++    }
++=======
+   /* If E is a backedge, then we want to verify that the COND_EXPR,
+      SWITCH_EXPR or GOTO_EXPR at the end of e->dest is not affected
+      by any statements in e->dest.  If it is affected, then it is not
+      safe to thread this edge.  */
+   if (e->flags & EDGE_DFS_BACK)
+     {
+       if (cond_arg_set_in_bb (e, e->dest))
+       goto fail;
+     }
+ myport_hook ()
++>>>>>>> theirs

  -  stmt_count = 0;
  +static bool
  +thread_through_normal_block (edge e,
  +                           gimple dummy_cond,
  +                           bool handle_dominating_asserts,
  +                           vec<tree> *stack,
  +                           tree (*simplify) (gimple, gimple),
  +                           vec<jump_thread_edge *> *path,
  +                           bitmap visited,
  +                           bool *backedge_seen_p,
  +                           bitmap src_map,
  +                           bitmap dst_map)
  +{
  +  /* If we have traversed a backedge, then we do not want to look
  +     at certain expressions in the table that can not be relied upon.
  +     Luckily the only code that looked at those expressions is the
  +     SIMPLIFY callback, which we replace if we can no longer use it.  
*/
  +  if (*backedge_seen_p)
  +    simplify = dummy_simplify;

     /* PHIs create temporary equivalences.  */
  -  if (!record_temporary_equivalences_from_phis (e, stack))
  -    goto fail;
  +  if (!record_temporary_equivalences_from_phis (e, stack, 
*backedge_seen_p,
  +                                              src_map, dst_map))
  +    return false;

     /* Now walk each statement recording any context sensitive
        temporary equivalences we can detect.  */
-- 
Paulo Matos
Antoine Pelisse· Dec 14, 2013, 09:40 UTC · re: Paulo Matos · lore

Re: Unexpected cherry-pick behaviour

On Wed, Dec 11, 2013 at 12:19 PM, Paulo Matos <paulo@matos-sorge.com> wrote:
Show 20 quoted lines
> On 11/12/2013 11:09, Antoine Pelisse wrote:
>>>
>>>
>>> I don't know how to interpret the fact that the line you sent (with the
>>> obvious --conflicts being --conflict) outputs nothing...
>>
>>
>> That is expected. git-checkout with this option [1] will reset the
>> conflict on gcc/tree-ssa-threadedge.c file to the initial conflict
>> state, and use the diff3 markers. You should have a new look at that
>> file as you will now be able to see the "ancestor" in the conflict.
>>
>> [1] You can have a look either at git-checkout manpage or here:
>> http://git-scm.com/docs/git-checkout, especially --merge and
>> --conflict options.
>> --
>
>
> Got it, but still not helpful as git is still modifying code out of the
> conflicting zone.

Actually it didn't modify out of the conflicting zone. This is because you are having a look at a combine-diff which tries to show both how it changed master *and* the cherry-picked patch at the same time. If you only want to see the diff applied to master, you should run:

    $ git diff --ours
You can also have a look at what is currently being applied:
    $ git diff :1:gcc/tree-ssa-threadedge.c :3:gcc/tree-ssa-threadedge.c

By the way, does anybody know a better way to do that ? I happen to do that quite a lot when fixing complex conflicts and the command is quite inconvenient (I always end-up forgetting which numbers to use, etc..).

Hope that helps, Antoine

Philip Oakley· Dec 14, 2013, 13:07 UTC · re: Antoine Pelisse · lore

Re: Unexpected cherry-pick behaviour

----- Original Message ----- 
From: "Antoine Pelisse" <apelisse@gmail.com>
<snip>
Show 10 quoted lines
>
> You can also have a look at what is currently being applied:
>
>    $ git diff :1:gcc/tree-ssa-threadedge.c 
> :3:gcc/tree-ssa-threadedge.c
>
> By the way, does anybody know a better way to do that ? I happen to do
> that quite a lot when fixing complex conflicts and the command is
> quite inconvenient (I always end-up forgetting which numbers to use,
> etc..).
Would this be a good use of the
    * Magic pathspecs like ":(icase)
that was recently released (v1.8.5  2Dec13)  so that the merge stages 
can be named.

I'm not sure that the three 'merge stages' have well defined short names yet though.

[1] http://schacon.github.io/gitbook/5_advanced_branching_and_merging.html [2] https://www.kernel.org/pub/software/scm/git/docs/git-merge.html see True Merge 4.

Aside: the 'merge stages' terminology does overlap the common user 
discussion of commit staging e.g. $gmane/236127 (Officially start moving 
to the term 'staging area'). Any pathspec magic names should reflect the 
concept being indicated rather than the implementation - a thorny 
problem.
>
> Hope that helps,
> Antoine
> --
Philip 
Junio C Hamano· Dec 14, 2013, 19:39 UTC · re: Philip Oakley · lore

Re: Unexpected cherry-pick behaviour

"Philip Oakley" <philipoakley@iee.org> writes:
> Would this be a good use of the
>    * Magic pathspecs like ":(icase)
> that was recently released (v1.8.5  2Dec13)  so that the merge stages
> can be named.

Because the pathspec mechahism is for you to tell an operation that works on a collection of paths (e.g. "all the paths in the HEAD", "all the paths at stage #1 in the index") to narrow the set it operates on down to only those that match, I do not think it is a good match at all to what you are trying to do.

Philip Oakley· Dec 15, 2013, 09:33 UTC · re: Junio C Hamano · lore

Re: Unexpected cherry-pick behaviour

From: "Junio C Hamano" <gitster@pobox.com>, Saturday, December 14, 2013 
7:39 PM
Show 13 quoted lines
> "Philip Oakley" <philipoakley@iee.org> writes:
>
>> Would this be a good use of the
>>    * Magic pathspecs like ":(icase)
>> that was recently released (v1.8.5  2Dec13)  so that the merge stages
>> can be named.
>
> Because the pathspec mechahism is for you to tell an operation that
> works on a collection of paths (e.g. "all the paths in the HEAD",
> "all the paths at stage #1 in the index") to narrow the set it
> operates on down to only those that match, I do not think it is a
> good match at all to what you are trying to do.
>

My point was that the ":1:" syntax already was a "path at stage #1 in the index" indicator, and that it would be good to have a memorable name for the :1:2:3: stages as per Antoine's query.

It maybe that my referring to it as a 'magic pathspec' was a mistake, but the difficulty of remembering which number is ours:theirs:base still stands.

(for general info; the :<stage>: format is defined in 'git revision (7)' as the last method for Specifying Revisions)

Philip -- PS should the cc: git-owner@vger.kernel.org be dropped as effectively a duplicate?

Philip Oakley· Dec 16, 2013, 18:53 UTC · re: Philip Oakley · lore

Re: Unexpected cherry-pick behaviour

From: "Philip Oakley" <philipoakley@iee.org>
Show 20 quoted lines
> From: "Junio C Hamano" <gitster@pobox.com>, Saturday, December 14, 
> 2013 7:39 PM
>> "Philip Oakley" <philipoakley@iee.org> writes:
>>
>>> Would this be a good use of the
>>>    * Magic pathspecs like ":(icase)
>>> that was recently released (v1.8.5  2Dec13)  so that the merge 
>>> stages
>>> can be named.
>>
>> Because the pathspec mechahism is for you to tell an operation that
>> works on a collection of paths (e.g. "all the paths in the HEAD",
>> "all the paths at stage #1 in the index") to narrow the set it
>> operates on down to only those that match, I do not think it is a
>> good match at all to what you are trying to do.
>>
>
> My point was that the ":1:" syntax already was a "path at stage #1 in 
> the index" indicator, and that it would be good to have a memorable 
> name for the :1:2:3: stages as per Antoine's  query.

Could someone point me at where is this syntax decoded? My initial hunt around the code base didn't find the relevant location.

Show 10 quoted lines
>
> It maybe that my referring to it as a 'magic pathspec' was a mistake, 
> but the difficulty of remembering which number is ours:theirs:base 
> still stands.
>
> (for general info; the :<stage>:  format is defined in 'git revision 
> (7)' as the last method for Specifying Revisions)
>
> Philip
> --
Philip
Junio C Hamano· Dec 16, 2013, 20:15 UTC · re: Philip Oakley · lore

Re: Unexpected cherry-pick behaviour

"Philip Oakley" <philipoakley@iee.org> writes:
Show 23 quoted lines
> From: "Philip Oakley" <philipoakley@iee.org>
>> From: "Junio C Hamano" <gitster@pobox.com>, Saturday, December 14,
>> 2013 7:39 PM
>>> "Philip Oakley" <philipoakley@iee.org> writes:
>>>
>>>> Would this be a good use of the
>>>>    * Magic pathspecs like ":(icase)
>>>> that was recently released (v1.8.5  2Dec13)  so that the merge
>>>> stages
>>>> can be named.
>>>
>>> Because the pathspec mechahism is for you to tell an operation that
>>> works on a collection of paths (e.g. "all the paths in the HEAD",
>>> "all the paths at stage #1 in the index") to narrow the set it
>>> operates on down to only those that match, I do not think it is a
>>> good match at all to what you are trying to do.
>>>
>>
>> My point was that the ":1:" syntax already was a "path at stage #1
>> in the index" indicator, and that it would be good to have a
>> memorable name for the :1:2:3: stages as per Antoine's  query.
>
> Could someone point me at where is this syntax decoded?

sha1_name.c (anything that turns name to object name goes there, I think). Look for this comment:

	/*
	 * sha1:path --> object name of path in ent sha1
	 * :path -> object name of absolute path in index
	 * :./path -> object name of path relative to cwd in index
	 * :[0-3]:path -> object name of path in index at stage
	 * :/foo -> recent commit matching foo
	 */

I do not think adding ":ours:path" as a synonym to ":2:path" adds enough value to make it worthwhilte to worry about breaking the expectation of those who thought "ours:path/name" will be something they could track if they wanted to.

Show 13 quoted lines
> My initial hunt around the code base didn't find the relevant location.
>
>>
>> It maybe that my referring to it as a 'magic pathspec' was a
>> mistake, but the difficulty of remembering which number is
>> ours:theirs:base still stands.
>>
>> (for general info; the :<stage>:  format is defined in 'git revision
>> (7)' as the last method for Specifying Revisions)
>>
>> Philip
>> --
> Philip
Junio C Hamano· Dec 14, 2013, 19:33 UTC · re: Antoine Pelisse · lore

Re: Unexpected cherry-pick behaviour

Antoine Pelisse <apelisse@gmail.com> writes:
> If you only want to see the diff applied to master, you
> should run:
>
>     $ git diff --ours
Does "git diff HEAD" have the same/similar effect?
Show 5 quoted lines
> You can also have a look at what is currently being applied:
>
>     $ git diff :1:gcc/tree-ssa-threadedge.c :3:gcc/tree-ssa-threadedge.c
>
> By the way, does anybody know a better way to do that ?

In a merge, you can say "git diff ...MERGE_HEAD" (three-dots). You should be able to tell "git show" the commit you are trying to pick during a cherry-pick, I think.

Antoine Pelisse· Dec 14, 2013, 19:47 UTC · re: Junio C Hamano · lore

Re: Unexpected cherry-pick behaviour

On Sat, Dec 14, 2013 at 8:33 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 8 quoted lines
> Antoine Pelisse <apelisse@gmail.com> writes:
>
>> If you only want to see the diff applied to master, you
>> should run:
>>
>>     $ git diff --ours
>
> Does "git diff HEAD" have the same/similar effect?
Yes, it does produce the same output as --ours.
Show 9 quoted lines
>> You can also have a look at what is currently being applied:
>>
>>     $ git diff :1:gcc/tree-ssa-threadedge.c :3:gcc/tree-ssa-threadedge.c
>>
>> By the way, does anybody know a better way to do that ?
>
> In a merge, you can say "git diff ...MERGE_HEAD" (three-dots).  You
> should be able to tell "git show" the commit you are trying to pick
> during a cherry-pick, I think.
Thanks,

← back to recent threads