[RFC/PATCH 0/3] fix "Funny: git -p submodule summary"

From: Jeff King <peff@peff.net>
Date: Jan 27, 2009, 06:25 UTC
Message-ID: <20090127062512.GA10487@coredump.intra.peff.net>
In-Reply-To: <alpine.DEB.1.00.0901081601240.30769@pacific.mpi-cbg.de>

On Thu, Jan 08, 2009 at 04:07:08PM +0100, Johannes Schindelin wrote:

Show 6 quoted lines

> Just try this with a submodule that has more changes than fit on a screen:
> 
> 	$ git -p submodule summary
> 
> In my tests, it consistently fscks up my console.  I wonder if this is 
> related to ea27a18(spawn pager via run_command interface).

OK, here is a patch series that fixes the problem:

  1/3: git: s/run_command/run_builtin/
  2/3: run_command: handle missing command errors more gracefully
  3/3: git: use run_command to execute dashed externals

1 is a cleanup, 2 is infrastructure support, and 3 is the actual fix.

There are two potential downsides to the fix:

 1. There is an extra fork and a parent process sitting in memory for
    dashed externals. This is pretty necessary to any fix, since
    something has to wait to do pager cleanup, and we can't rely on the
    child to do so.

 2. A failed attempt to execute a dashed external results in an extra
    fork. For builtins, this has no impact, since they take precedence.
    For aliases, though, it means we will do an extra fork before
    realizing that there is no dashed external and trying the alias.

We can fix '2' by actually doing the PATH lookup ourselves, and only calling run_command if we know we have a match. We can also reduce the impact of both by only doing this multi-process magic if we have spawned a pager; then only a small subset of invocations needs to pay for it.

I chose not to do the second optimization because it makes the code more complex and inconsistent (we now have two different ways of doing the same thing, depending on a seemingly unrelated setting) and fragile (the pager might not be the only atexit handler installed).

The first (doing PATH lookup ourselves) might make sense, though.

JSixt, there are some tweaks to the Windows code to report back the exec error. They look obviously correct to me, but I have no box to test (even compile test) them on.

-Peff