search

dusty-phillips / gitifyhg

Tools for using git as a client to mercurial repositories
GNU General Public License v3.0
62 stars 17 forks source link

Hidden dependencies on environment can cause severe failures #26

Closed fingolfin closed 11 years ago

fingolfin commented 11 years ago

There are multiple parts in gitifyhg that make assumptions about their environment, but these are never explicitly checked for, which can cause failures (both in tests in real life).

One obvious source is commit de81e2b9fd013f348a36a9e6d0cf336063849878:

  1. It uses mq without checking that this extension is enabled. And mq is disabled by default. So if you don't enable it in your .hgrc, this causes tests to fail, and of course also causes failures in real use.
  2. It parses the content of an exception thrown by mercurial, and relies on the output being in english. This fails if the users has set a different locale, e.g. LANG=de_DE.

To fix issue 1, either gitifyhg should abort if mq is not enabled; or preferably, it should enable mq on the fly, if that is possible -- I hope that extensions.load(ui, "mq") will do just that.

To fix issue 2, the env var HGPLAIN should be set.

Using HGPLAIN will also override various user config settings, which seems like a good idea to me in any case.

There are more hidden assumptions, though. E.g. that the git and mercurial versions are recent enough, but I already filed issue #20 about that ;-).

dusty-phillips commented 11 years ago

Are you suggesting using HGPLAIN in general, or only during testing?

fingolfin commented 11 years ago

I am suggesting to use it in general.

dusty-phillips commented 11 years ago

I was wondering if it would be possible to flag mercurial internally, but I just checked and the only way to do that is to monkeypatch i18n.py. Setting the environment variable is definitely better.

alexsydell commented 11 years ago

As far as I remember, mq is included with mercurial (at least as of the version I was looking at the source for). I'd guess it shouldn't be necessary to enable the extension to be able to use the strip() function in the commit you linked to. I could be totally wrong here, but probably worth looking into before adding the extensions.load() call.

jedbrown commented 11 years ago

Should we use repair.strip instead? We have to call it one revision at a time for the older API, but that's also what mq.strip was doing.

fingolfin commented 11 years ago

@alexsydell: sorry, but yes, you are wrong: While mq is shipped with hg, it is disabled by default. This reported (and the now merged PR #27) was triggered by gitifyhg failing when I disabled mq in my .hgrc.

@jedbrown Unless you see a good reason to do so, I'd rather stick with mq.strip -- mq has been distributed with hg for ages now, and it was very easy to activate it by default.

alexsydell commented 11 years ago

@fingolfin I realize it's disabled by default, but I'm surprised that you can't just call that function anyway as part of the hg API. That is, running 'hg strip' clearly shouldn't work out of the box, but I'm curious why calling strip() in Python doesn't either. Ah well, empirical evidence obviously shows that was a bad assumption.