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

[Jeff King <peff@peff.net>]

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

> 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