Re: [PATCH] git-web--browse: invoke kfmclient directly

From: Jeff King <peff@peff.net>
Date: Sep 18, 2011, 18:38 UTC
Message-ID: <20110918183846.GA31176@sigill.intra.peff.net>
In-Reply-To: <1316341224-4359-1-git-send-email-judge.packham@gmail.com>

On Sun, Sep 18, 2011 at 10:20:24PM +1200, Chris Packham wrote:

> Instead of using eval which causes problems when a URL contains an
> appropriately escaped ampersand (\&).

I think this probably should just remove all of the evals. I don't see how any of them is doing any good, and they're actively breaking URLs that need quoting.

Hmm. Actually, the one for custom browser commands might need it, because that one is expected to be a shell snippet. I suspect the simplest thing is to do something like:

  eval "$browser_cmd \"\$@\""

The other option would be to actually shell-quote each argument, which is a pain to do in the shell (but is what C git does).

Show 5 quoted lines

> For what it's worth I've included a testcase that detects my problem. I'm not
> sure if the testcase is really worth it because the test library suppresses X
> applications and even if it didn't the testcase is fairly trivial and might
> just annoy people by opening web-browsers (and it snaps up the last t99xx
> prefix).

Ick, yeah. Actually starting real browsers interacts too much with the world outside of the test scripts. The results will be annoying (new browser windows) and cause non-deterministic test results.

If you want to make a test, I think you would do better with something like:

  echo someurl_with_&_in_it >expect &&
  git config browser.custom.cmd echo &&
  git web--browse --browser=custom someurl_with_&_in_it >actual &&
  test_cmp expect actual

That won't test that we are invoking kfmclient correctly, obviously, but you can confirm at least that URLs are making it through to the browser script intact.

-Peff