Opened 9 years ago

Closed 8 years ago

#11120 closed defect (fixed)

Autodetect installed 3-way merge programs (invalidates #4434)

Reported by: kini Owned by: tbd
Priority: major Milestone: sage-4.7.2
Component: scripts Keywords: mercurial osx sd31
Cc: Merged in: sage-4.7.2.alpha3
Authors: Keshav Kini, John Palmieri Reviewers: John Palmieri, Keshav Kini
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #4434 #10594 Stopgaps:

Description (last modified by leif)

Depends on #10594, which already incorporates the patch to the Mercurial spkg (trac_11120-re-fix-trac_4434.mercurial-spkg.patch) attached here.


In Mercurial versions earlier than 1.0, a script called hgmerge was installed to the system when you installed mercurial. This script called whatever 3-way merge programs you had installed in order to manually intervene in Mercurial's merging process when it couldn't figure out how to resolve a conflict.

This was deprecated in version 1.0, and now Mercurial calls 3-way merge programs directly instead of relying on an hgmerge script. In order to make this work properly, one should set the proper configuration variables to help Mercurial find any installed 3-way merge programs.

There is a file, contrib/mergetools.rc, in the Mercurial source distribution which provides exactly these configuration variables, though the upstream installer script does not actually install it anywhere.

The problem reported at trac #4434 was caused, I guess, by the fact that hgmerge was deprecated and we sage people didn't notice, so the patch there simply caused our distribution of Mercurial to continue using hgmerge. The patch also did this only on the OS X platform, though as far as I can tell the same fundamental problem actually appears on Linux as well (see comments below).

A better way to fix the problem on trac #4434, and fix it for Linux too, would be to use the contrib/mergetools.rc file as it was designed to be used.


Apply only trac_11120-no-hgmerge.patch to the Sage scripts repository.

Attachments (2)

trac_11120-re-fix-trac_4434.mercurial-spkg.patch (1.5 KB) - added by kini 9 years ago.
apply to mercurial spkg's repository
trac_11120-no-hgmerge.patch (830 bytes) - added by jhpalmieri 8 years ago.
apply to scripts repo

Download all attachments as: .zip

Change History (27)

comment:1 Changed 9 years ago by dimpase

hmm, how does one check that removing hgmerge does not break things? reading #4434 says it's about a 3-way graphical thing, of which I am clueless...

comment:2 follow-up: Changed 9 years ago by kini

As we talked about in person, I guess the way to test it would be to try using sage -hg to merge files which cannot be automatically merged. I created this minimal example of such a repository and such a file, which you then pulled and tried out, using sage -hg with $SAGE_ROOT/bin/hgmerge removed. This worked just as well as it works on my Linux machine:

keshav@esterhazy ~/temp/hgmerge-test $ sage -hg log --style=compact
2[tip]:0   212fc073ffa9   2011-04-04 08:10 -0700   kini
  Three

1   478fc08a9390   2011-04-04 08:09 -0700   kini
  Two

0   a8cccf400996   2011-04-04 08:08 -0700   kini
  One

keshav@esterhazy ~/temp/hgmerge-test $ sage -hg merge
merging somefile
warning: conflicts during merge.
merging somefile failed!
0 files updated, 0 files merged, 0 files removed, 1 files unresolved
use 'hg resolve' to retry unresolved file merges or 'hg update -C' to abandon
keshav@esterhazy ~/temp/hgmerge-test $ 

Note that this is not really an error - it's just the lack of a default merge program being installed. Could it be possible that the problem that occasioned #4474 is present on Linux as well?

There is actually a sort of "merge tool autodect script" (actually just some configuration entries making mercurial smarter about where to find a range of common merging tools) which is provided but not installed by the upstream distribution of Mercurial, and which lies in the file src/contrib/mergetools.rc within the spkg's root directory. Putting this in $SAGE_LOCAL/etc/mercurial/hgrc.d/ should cause merge program problems to go away. I'll write a patch...

comment:3 Changed 9 years ago by kini

  • Description modified (diff)

comment:4 follow-up: Changed 9 years ago by kini

  • Description modified (diff)
  • Status changed from new to needs_review
  • Summary changed from Re-fix #4434 (merging broken on OS X) using MergeToolConfiguration in Mercurial 1.x to Autodetect installed 3-way merge programs (invalidates #4434)

comment:5 in reply to: ↑ 4 ; follow-up: Changed 9 years ago by dimpase

  • Status changed from needs_review to needs_work

Replying to kini:

applying trac_11120-re-fix-trac_4434.mercurial-spkg.patch
patching file spkg-install
Hunk #1 succeeded at 37 with fuzz 1 (offset -30 lines).
now at: trac_11120-re-fix-trac_4434.mercurial-spkg.patch

why do I see fuzz? I'm applying it to the mercurial spkg that comes with sage- 4.7.alpha3

As well, it seems that your patch either wasn't tested, or was tested against a diffrent spkg. Indeed, I get:

...
riting /usr/local/src/sage/sage-4.7.alpha3/local/lib/python/mercurial-1.6.4-py2.6.egg-info
cp: contrib/mergetools.rc: No such file or directory
Error installing mergetools.rc
Deleting dead links
...

at the end of the sage -f mecurial.... indeed, it should be "src/contrib/" rather than just "contrib/" in the argument of "cp".

comment:6 in reply to: ↑ 5 Changed 9 years ago by dimpase

Replying to dimpase:

OK, this depends on #11121, which isn't ready yet. Oops...

comment:7 Changed 9 years ago by kini

  • Description modified (diff)

Yes, you're right, it depends on #11121 (specifically on the patch there which applies to the mercurial spkg). Sorry, I'll make that more prominent in the ticket description. As for the error actually copying the file, it's actually because the filename is wrong. It still needs to be called mergetools.rc (or at least something.rc) in order to work, but in the vanilla Mercurial source distribution it is actually called contrib/mergetools.hgrc for some reason. It needs to be renamed during the copy over. I think I had renamed it in my sandbox by mistake at some point when trying to fiddle with it to test different diff viewers - stupid oversight on my part. I'll fix it. Sorry for the inconvenience.

comment:8 in reply to: ↑ 2 Changed 9 years ago by kini

  • Description modified (diff)
  • Status changed from needs_work to needs_review

Now that I've upgraded to and rebuilt alpha3 I can confirm that the spkg installs nicely and does what it's supposed to do after applying the patch. Marking as needs_review . Should I upload a new spkg file to sage.math.washington.edu, or does the release manager make the new spkg by applying a patch I upload to trac? I'll do both for now.

Replying to kini:

Note that this is not really an error - it's just the lack of a default merge program being installed. Could it be possible that the problem that occasioned #4474 is present on Linux as well?

I of course meant #4434 here -- oops.

Changed 9 years ago by kini

apply to mercurial spkg's repository

comment:9 Changed 9 years ago by jhpalmieri

Keshav: a request: while you're working on mercurial issues on OS X, could you also take a look at #10171?

comment:10 Changed 9 years ago by kini

John: I only made this ticket because I happened across the patch of #4434 while trying to fix #11121 :) I'm running Debian Wheezy and don't have a Mac... I see that Dmitrii is on the case, however.

comment:11 Changed 9 years ago by kini

  • Dependencies set to #11121

(testing #10966)

comment:12 Changed 9 years ago by kini

  • Description modified (diff)
  • Status changed from needs_review to needs_work

I'll fix this along with #10594, hopefully.

comment:13 Changed 9 years ago by kini

  • Authors set to Keshav Kini
  • Description modified (diff)
  • Keywords osx sd31 added; os x removed
  • Status changed from needs_work to needs_review

This should be fixed by this spkg. Please review!

comment:14 Changed 8 years ago by jhpalmieri

Two things: first, I'm not sure how to test this. Second, during an upgrade on OS X, the file hgmerge is still present in SAGE_LOCAL/bin. Is that going to be a problem, or will the settings in hgrc.d/mergetools.rc override it?

comment:15 Changed 8 years ago by kini

hgmerge remaining in SAGE_LOCAL/bin is not a problem; mergetools.rc overrides it. I guess the best way to test this is to try to replicate #4434 with the new spkg installed - it should be impossible.

By the way, I guess I should also eradicate sage -hgmerge - I'll submit a patch (not to the Mercurial spkg but to Sage) in a bit.

comment:16 Changed 8 years ago by jhpalmieri

  • Reviewers set to John Palmieri

Okay, if you can patch the scripts repo to get rid of "sage -hgmerge", then I think this can get a positive review.

comment:17 Changed 8 years ago by jhpalmieri

  • Authors changed from Keshav Kini to Keshav Kini, John Palmieri

Here's a patch to the scripts repo to remove the "sage -hgmerge" command.

Changed 8 years ago by jhpalmieri

apply to scripts repo

comment:18 Changed 8 years ago by jhpalmieri

  • Dependencies changed from #11121 to #10594
  • Description modified (diff)

comment:19 Changed 8 years ago by jhpalmieri

  • Description modified (diff)

comment:20 follow-up: Changed 8 years ago by kini

  • Dependencies changed from #10594 to #4434, #10594
  • Reviewers changed from John Palmieri to John Palmieri, Keshav Kini
  • Status changed from needs_review to positive_review

Looks fine. All doctests pass (though I doubt hgmerge was doctested anyway...). Grepping for "hgmerge" doesn't find anything significant. Eradication successful! Positive review from me.

comment:21 in reply to: ↑ 20 Changed 8 years ago by dimpase

Replying to kini:

Looks fine. All doctests pass (though I doubt hgmerge was doctested anyway...). Grepping for "hgmerge" doesn't find anything significant. Eradication successful! Positive review from me.

would be good to state clearly in the ticket description which patches should be applied, and which should not be applied. I.e, is trac_11120-re-fix-trac_4434.mercurial-spkg.patch needed?

comment:22 Changed 8 years ago by kini

It is clearly stated in the ticket description is it not? Read the bottom. No, that patch is not needed.

comment:23 Changed 8 years ago by leif

  • Dependencies changed from #4434, #10594 to #4434 #10594

comment:24 Changed 8 years ago by leif

  • Description modified (diff)

comment:25 Changed 8 years ago by leif

  • Component changed from packages to scripts
  • Merged in set to sage-4.7.2.alpha3
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.