Unexpected cherry-pick behaviour

Unexpected cherry-pick behaviour

[Paulo Matos]

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

Re: Unexpected cherry-pick behaviour

[Junio C Hamano]

Paulo Matos <paulo@matos-sorge.com> writes:

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

Re: Unexpected cherry-pick behaviour

[Paulo Matos]

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

Any suggestions?

-- 
Paulo Matos

Re: Unexpected cherry-pick behaviour

[Antoine Pelisse]

On Wed, Dec 11, 2013 at 11:04 AM, Paulo Matos <paulo@matos-sorge.com> wrote:
> 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.

Re: Unexpected cherry-pick behaviour

[Paulo Matos]

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.

$ 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

Re: Unexpected cherry-pick behaviour

[Antoine Pelisse]

On Wed, Dec 11, 2013 at 12:19 PM, Paulo Matos <paulo@matos-sorge.com> wrote:
> 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

Re: Unexpected cherry-pick behaviour

[Philip Oakley]

----- Original Message ----- 
From: "Antoine Pelisse" <apelisse@gmail.com>
<snip>
>
> 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 

Re: Unexpected cherry-pick behaviour

[Junio C Hamano]

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

Re: Unexpected cherry-pick behaviour

[Philip Oakley]

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.

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? 

Re: Unexpected cherry-pick behaviour

[Philip Oakley]

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

Re: Unexpected cherry-pick behaviour

[Junio C Hamano]

"Philip Oakley" <philipoakley@iee.org> writes:

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



> 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

Re: Unexpected cherry-pick behaviour

[Junio C Hamano]

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?

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

Re: Unexpected cherry-pick behaviour

[Antoine Pelisse]

On Sat, Dec 14, 2013 at 8:33 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

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