Re: Patch Series v3 for "use the $( ... ) construct for command substitution"

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Apr 4, 2014, 18:12 UTC
Message-ID: <20140404181215.GJ6851@google.com>
In-Reply-To: <vpqk3b56lik.fsf@anie.imag.fr>

Matthieu Moy wrote:

> Jonathan Nieder <jrnieder@gmail.com> writes:

Show 5 quoted lines

>> If the script is "obviously correct" enough then there is no need
>> to manually go through 140 files after that point.
>
> The script cannot be "obviously correct", as there are a lot of
> potential corner-cases (nested `, nesting ` within ", comments, ...).

To be a devil's advocate for a moment:

 * Comments are easy to detect.  Remember, we are not trying to handle
   some adversary sending arbitrary well-formed shell scripts but just
   the git source code which already follows a consistent style.  When
   I search for /#.*/ in .sh files, every match except for

	t/test-lib-functions.sh:output=`echo; echo "# Stderr is:"; cat "$stderr"`

   (which contains a backtick before the # mark) is a comment.

 * Nested ` is evil.  A search for the string \' quickly finds them all,
   and they are very very rare.

   The only exception I see is git-svn tests, which independently of
   everything else is an obvious thing to fix first.

 * Nesting `` within double-quotes has the same semantics as $()
   within quotes.  I don't think that poses a problem.

[...]

>> If the only way to get this done is to actually manually review those
>> 140 files, I just don't think it's worth it.
>
> Honnestly, I went through the series once and it wasn't that painful.

It is not just the people on the list reviewing now but others in the future wanting to understand whether it is safe to upgrade to a new version or where a bug they have run into came from. The simpler we can make the task of convincing oneself that the patch behaves as described, the better.

140 patches worth of churn once every couple of years is not terrible, but I really don't want to see this becoming a pattern. :/ And I don't see how the upside in this example warrants it.

Hoping that clarifies, Jonathan