threads / discuss / 26498

libreoffice merge issue ...

Subject: libreoffice merge issue ...

## tl;dr

10 messages between Feb 14, 2011 and Apr 29, 2011.

replies: 9people: 6as markdown or json

Michael Meeks· Feb 14, 2011, 16:07 UTC · lore
Hi guys,

We are having quite some fun merging git branches with LibreOffice, and I stumbled over this just now with master git with hash: 00e6ee724640701b32aca27cc930fd6409c87ae2

Setup (some large repos):
	git clone git://anongit.freedesktop.org/libreoffice/libs-core
	git checkout integration/dev300_m98
	git remote add stage git://anongit.freedesktop.org/libreoffice/staging/@REPO@
	git fetch stage
	Test[1]:
	git merge stage/premerge/dev300_m98
	git diff idl/source/cmptools/lex.cxx
	yields:
@@@ -147,11 -147,7 +147,15 @@@ SvToken & SvToken::operator = ( const S
  *************************************************************************/
  void SvTokenStream::InitCtor()
  {
++<<<<<<< HEAD
 +#ifdef DOS
 +    SetCharSet( CHARSET_ANSI );
 +#else
      SetCharSet( gsl_getSystemTextEncoding() );
 +#endif
++=======
++    SetCharSet( gsl_getSystemTextEncoding() );
++>>>>>>> stage/premerge/dev300_m98
      aStrTrue  = "TRUE";
      aStrFalse = "FALSE";
      nLine       = nColumn = 0;
	With the above master hash; whereas with v1.7.3.4 it yields nothing (as
it should IMHO) - we havn't edited things around that chunk in master.
	That is slightly concerning; thoughts much appreciated. Incidentally,
the whole 'make install' installs into ~/bin was extremely unexpected
and yielded 30minutes of pain trying to work out what was installed
where and why, and the interaction with --prefix, and ... now it seems I
should always run rehash; git --version before any command, and sanity
check things ;-)
	Thanks,
		Michael.
[1] - potentially you need:
[merge]
    renamelimit = 20000
in your ~/.gitconfig
-- 
 michael.meeks@novell.com  <><, Pseudo Engineer, itinerant idiot
Norbert Thiebaud· Feb 14, 2011, 16:52 UTC · re: Michael Meeks · lore

Re: libreoffice merge issue ...

On Mon, Feb 14, 2011 at 10:07 AM, Michael Meeks <michael.meeks@novell.com> wrote:

Show 11 quoted lines
> Hi guys,
>
> We are having quite some fun merging git branches with LibreOffice, and
> I stumbled over this just now with master git with hash:
> 00e6ee724640701b32aca27cc930fd6409c87ae2
>
> Setup (some large repos):
>
>        git clone git://anongit.freedesktop.org/libreoffice/libs-core
>        git checkout integration/dev300_m98
>        git remote add stage git://anongit.freedesktop.org/libreoffice/staging/@REPO@
correction: this should read
        git remote add stage
git://anongit.freedesktop.org/libreoffice/staging/libs-core
or you should use ./g instead of git for all these 3 git operations
Norbert
Show 49 quoted lines
>        git fetch stage
>
>        Test[1]:
>
>        git merge stage/premerge/dev300_m98
>        git diff idl/source/cmptools/lex.cxx
>
>        yields:
>
> @@@ -147,11 -147,7 +147,15 @@@ SvToken & SvToken::operator = ( const S
>  *************************************************************************/
>  void SvTokenStream::InitCtor()
>  {
> ++<<<<<<< HEAD
>  +#ifdef DOS
>  +    SetCharSet( CHARSET_ANSI );
>  +#else
>      SetCharSet( gsl_getSystemTextEncoding() );
>  +#endif
> ++=======
> ++    SetCharSet( gsl_getSystemTextEncoding() );
> ++>>>>>>> stage/premerge/dev300_m98
>      aStrTrue  = "TRUE";
>      aStrFalse = "FALSE";
>      nLine       = nColumn = 0;
>
>        With the above master hash; whereas with v1.7.3.4 it yields nothing (as
> it should IMHO) - we havn't edited things around that chunk in master.
>
>        That is slightly concerning; thoughts much appreciated. Incidentally,
> the whole 'make install' installs into ~/bin was extremely unexpected
> and yielded 30minutes of pain trying to work out what was installed
> where and why, and the interaction with --prefix, and ... now it seems I
> should always run rehash; git --version before any command, and sanity
> check things ;-)
>
>        Thanks,
>
>                Michael.
>
> [1] - potentially you need:
> [merge]
>    renamelimit = 20000
> in your ~/.gitconfig
> --
>  michael.meeks@novell.com  <><, Pseudo Engineer, itinerant idiot
>
>
>
Ferry Huberts· Feb 14, 2011, 17:26 UTC · re: Michael Meeks · lore

Re: libreoffice merge issue ...

On 02/14/2011 05:07 PM, Michael Meeks wrote:
Show 16 quoted lines
> Hi guys,
> 
> We are having quite some fun merging git branches with LibreOffice, and
> I stumbled over this just now with master git with hash:
> 00e6ee724640701b32aca27cc930fd6409c87ae2
> 
> Setup (some large repos):
> 
> 	git clone git://anongit.freedesktop.org/libreoffice/libs-core
> 	git checkout integration/dev300_m98
> 	git remote add stage git://anongit.freedesktop.org/libreoffice/staging/@REPO@
> 	git fetch stage
> 
> 	Test[1]:
> 
> 	git merge stage/premerge/dev300_m98

the merge has detected a conflict and has annotated the conflicted file(s). at least one of the conflicting files is idl/source/cmptools/lex.cxx (you might want to do 'git mergetool' after you have setup your merge tool)

> 	git diff idl/source/cmptools/lex.cxx
>

you're diffing the 'unmerged, annotated' file against the original file before the merge.

your merge is not yet complete, you'll have to resolve the conflicts first and then commit

Show 15 quoted lines
> 	yields:
> 
> @@@ -147,11 -147,7 +147,15 @@@ SvToken & SvToken::operator = ( const S
>   *************************************************************************/
>   void SvTokenStream::InitCtor()
>   {
> ++<<<<<<< HEAD
>  +#ifdef DOS
>  +    SetCharSet( CHARSET_ANSI );
>  +#else
>       SetCharSet( gsl_getSystemTextEncoding() );
>  +#endif
> ++=======
> ++    SetCharSet( gsl_getSystemTextEncoding() );
> ++>>>>>>> stage/premerge/dev300_m98
this is the annotation of the conflict
grtz
-- 
Ferry Huberts
Norbert Thiebaud· Feb 14, 2011, 17:33 UTC · re: Ferry Huberts · lore

Re: libreoffice merge issue ...

On Mon, Feb 14, 2011 at 11:26 AM, Ferry Huberts <mailings@hupie.com> wrote:
Show 9 quoted lines
>
>
> On 02/14/2011 05:07 PM, Michael Meeks wrote:
>> Hi guys,
>>
>> We are having quite some fun merging git branches with LibreOffice, and
>> I stumbled over this just now with master git with hash:
>> 00e6ee724640701b32aca27cc930fd6409c87ae2
>>
[...]
Show 18 quoted lines
>>       yields:
>>
>> @@@ -147,11 -147,7 +147,15 @@@ SvToken & SvToken::operator = ( const S
>>   *************************************************************************/
>>   void SvTokenStream::InitCtor()
>>   {
>> ++<<<<<<< HEAD
>>  +#ifdef DOS
>>  +    SetCharSet( CHARSET_ANSI );
>>  +#else
>>       SetCharSet( gsl_getSystemTextEncoding() );
>>  +#endif
>> ++=======
>> ++    SetCharSet( gsl_getSystemTextEncoding() );
>> ++>>>>>>> stage/premerge/dev300_m98
>
>
> this is the annotation of the conflict

The point is that there should not have been a conflict to start with. git 1.7.3.4 agree that there is no conflict.

Norbert
Show 6 quoted lines
>
> grtz
>
> --
> Ferry Huberts
>
Jeff King· Feb 15, 2011, 09:45 UTC · re: Michael Meeks · lore

Re: libreoffice merge issue ...

On Mon, Feb 14, 2011 at 04:07:15PM +0000, Michael Meeks wrote:
Show 17 quoted lines
> Setup (some large repos):
> 
> 	git clone git://anongit.freedesktop.org/libreoffice/libs-core
> 	git checkout integration/dev300_m98
> 	git remote add stage git://anongit.freedesktop.org/libreoffice/staging/@REPO@
> 	git fetch stage
> 
> 	Test[1]:
> 
> 	git merge stage/premerge/dev300_m98
> 	git diff idl/source/cmptools/lex.cxx
> 
> 	yields:
> [a conflict in idl/source/cmptools/lex.cxx]
> 
> 	With the above master hash; whereas with v1.7.3.4 it yields nothing (as
> it should IMHO) - we havn't edited things around that chunk in master.

Interesting. I looked at both sides of the merge and the merge base, and there definitely should not be a conflict there. The regression bisects to 83c9031 (unpack_trees(): skip trees that are the same in all input, 2010-12-22). Reverting that commit makes the problem go away.

-Peff
Junio C Hamano· Feb 15, 2011, 18:46 UTC · re: Jeff King · lore

Re: libreoffice merge issue ...

Jeff King <peff@peff.net> writes:
> Interesting. I looked at both sides of the merge and the merge base, and
> there definitely should not be a conflict there. The regression bisects
> to 83c9031 (unpack_trees(): skip trees that are the same in all input,
> 2010-12-22). Reverting that commit makes the problem go away.
Thanks; I was wondering about this myself but you bisected it faster.
Will revert.
Jeff King· Feb 16, 2011, 02:57 UTC · re: Junio C Hamano · lore

Re: libreoffice merge issue ...

On Tue, Feb 15, 2011 at 10:46:03AM -0800, Junio C Hamano wrote:
Show 10 quoted lines
> Jeff King <peff@peff.net> writes:
> 
> > Interesting. I looked at both sides of the merge and the merge base, and
> > there definitely should not be a conflict there. The regression bisects
> > to 83c9031 (unpack_trees(): skip trees that are the same in all input,
> > 2010-12-22). Reverting that commit makes the problem go away.
> 
> Thanks; I was wondering about this myself but you bisected it faster.
> 
> Will revert.

One other thing I noticed during the bisect: when using a version of git containing 83c9031, the merge took a lot longer. As in, 13 seconds with v1.7.3 versus 69 seconds with master.

That may simply be because the bug being demonstrated causes us to erroneously do more file-level merging than we would otherwise need to. But I thought it worth mentioning in case you want to delve into fixing the code.

-Peff
Junio C Hamano· Feb 16, 2011, 21:30 UTC · re: Jeff King · lore

Re: libreoffice merge issue ...

Jeff King <peff@peff.net> writes:
Show 10 quoted lines
>> Thanks; I was wondering about this myself but you bisected it faster.
>> 
>> Will revert.
>
> One other thing I noticed during the bisect: when using a version of git
> containing 83c9031, the merge took a lot longer. As in, 13 seconds with
> v1.7.3 versus 69 seconds with master.
>
> That may simply be because the bug being demonstrated causes us to
> erroneously do more file-level merging than we would otherwise need to.

Yeah, the reverted 83c9031 (unpack_trees(): skip trees that are the same in all input, 2010-12-22) also seems to have seriously broken intermediate merge merge-recursive makes. I actually recall scratching my head when I made 00e6ee7 (Merge branch 'maint', 2011-02-11) that was causing add/add conflict when it shouldn't. It turns out that quite a lot of entries were missing in contrib/ area from the virtual common ancestry tree synthesized by merge-recursive that called into the botched unpack_trees()---it of course would result in add/add conflict if a merge is done using such a tree as the common.

No, I haven't had a chance nor energy to dig further than what I reported above.

Damien Wyart· Apr 21, 2011, 14:01 UTC · re: Junio C Hamano · lore

Re: libreoffice merge issue ...

Hi,
Sorry to wake up an old thread.
* Junio C Hamano <gitster@pobox.com> [2011-02-16 13:30]:
Show 9 quoted lines
> Yeah, the reverted 83c9031 (unpack_trees(): skip trees that are the
> same in all input, 2010-12-22) also seems to have seriously broken
> intermediate merge merge-recursive makes. I actually recall scratching
> my head when I made 00e6ee7 (Merge branch 'maint', 2011-02-11) that
> was causing add/add conflict when it shouldn't. It turns out that
> quite a lot of entries were missing in contrib/ area from the virtual
> common ancestry tree synthesized by merge-recursive that called into
> the botched unpack_trees()---it of course would result in add/add
> conflict if a merge is done using such a tree as the common.
> No, I haven't had a chance nor energy to dig further than what
> I reported above.

Out of curiosity, I would like to know if digging further into this issue is still on your TODO list. I feel understanding exactly what was wrong in 83c9031 would be interesting ; having just the revert is a bit frustrating.

The initial optimization in 83c9031 seemed right at first glance, so I would be interesting in having a more final answer to this.

Many thanks in advance,
-- 
Damien Wyart
Jeff King· Apr 29, 2011, 12:55 UTC · re: Damien Wyart · lore

Re: libreoffice merge issue ...

On Thu, Apr 21, 2011 at 04:01:32PM +0200, Damien Wyart wrote:
Show 21 quoted lines
> * Junio C Hamano <gitster@pobox.com> [2011-02-16 13:30]:
> > Yeah, the reverted 83c9031 (unpack_trees(): skip trees that are the
> > same in all input, 2010-12-22) also seems to have seriously broken
> > intermediate merge merge-recursive makes. I actually recall scratching
> > my head when I made 00e6ee7 (Merge branch 'maint', 2011-02-11) that
> > was causing add/add conflict when it shouldn't. It turns out that
> > quite a lot of entries were missing in contrib/ area from the virtual
> > common ancestry tree synthesized by merge-recursive that called into
> > the botched unpack_trees()---it of course would result in add/add
> > conflict if a merge is done using such a tree as the common.
> 
> > No, I haven't had a chance nor energy to dig further than what
> > I reported above.
> 
> Out of curiosity, I would like to know if digging further into this
> issue is still on your TODO list. I feel understanding exactly what was
> wrong in 83c9031 would be interesting ; having just the revert is a bit
> frustrating.
> 
> The initial optimization in 83c9031 seemed right at first glance, so
> I would be interesting in having a more final answer to this.

I didn't dig further, but looking at 83c9031 with a fresh set of eyes, I think that merge-recursive falls under the "exceptions" that Junio listed in the commit message. That is, he indicates correctly that we cannot use this optimization for "reset --hard" or for checking out for the first time.

Similarly, merge-recursive is going to do many 3-way merges between trees into an empty index (when merging virtual ancestors), and we need to unpack those subtrees even if they are all the same. But given the conditions added by 83c9031 in unpack-trees.c:fast_forward_merge, I don't see anything preventing the optimization in how merge-recursive calls into unpack-trees.

We do a discard_cache() right before doing the 3-way merge for virtual ancestors. So we will see that there are no entries in our current index for that directory. That may be a clue that the optimization should not be used.

-Peff

← back to recent threads