Re: What's cooking in git.git (Jan 2010, #01; Mon, 04)

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 5, 2010, 01:35 UTC
Message-ID: <7vhbr1bagk.fsf@alter.siamese.dyndns.org>
In-Reply-To: <4B421766.4040506@kdbg.org>

Johannes Sixt <j6t@kdbg.org> writes:

Show 19 quoted lines

> Junio C Hamano schrieb:
>> * jk/run-command-use-shell (2010-01-01) 8 commits
>>  - t4030, t4031: work around bogus MSYS bash path conversion
>>  - t0021: use $SHELL_PATH for the filter script
>>  - diff: run external diff helper with shell
>>  - textconv: use shell to run helper
>>  - editor: use run_command's shell feature
>>  - run-command: optimize out useless shell calls
>>  - run-command: convert simple callsites to use_shell
>>  - run-command: add "use shell" option
>
> Two notes about this:
>
> 1. My patch "t0021:..." contains an unrelated change to t4030 (it
> changes a /bin/sh to $SHELL_PATH) that is not necessary. I included it
> in my first version of the patch, but later noticed that we already
> have many similar uses of /bin/sh instead of $SHELL_PATH in test
> scriptlets and decided to remove the change, but I only changed the
> commit message and forgot to unstage t4030.

While you are technically correct that the change you made in t4030 is not justified by the commit log message in the sense that the "hexdump" script will go through run_command() interface and is not subject to the special rules filter writers need to keep in mind, the patch text itself is a good change, isn't it? Do you want me to split the commit into two (one with the current message with a patch only to t0021, and another to t4030 with a justification like "SHELL_PATH is what the user told us to use")?

> 2. If you intend to merge the early part of the topic to master early
> and hold "diff:..." and "textconv:..." in next a bit longer (as
> proposed by Jeff), then you should move "t0021:..." after
> "run-command: optimize out useless shell calls".

As "run-command: convert simple callsites to use_shell" is the one that changes the filter_buffer(), do you want to have t0021 patch before that one, to prepare the test for the coming change?