Re: Patch Series v3 for "use the $( ... ) construct for command substitution"
Jonathan Nieder writes: > 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. :/ Likewise. > And I don't see how the upside in this example warrants it. Paraphrasing http://article.gmane.org/gmane.linux.kernel/943020 It'd be good if we don't _add_ such things to the tree. But IMO it's such a minor thing that once it _is_ in the tree, it's not really worth the patch noise to go and fix it up. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Patch Series v3 for "use the $( ... ) construct for command substitution"
Matthieu Moy wrote: > Jonathan Nieder writes: >> 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Patch Series v3 for "use the $( ... ) construct for command substitution"
Jonathan Nieder writes: > 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, ...). > 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. I need to do a more carefull review, but using "git diff --color-words=." it can be really fast. Junio suggested splitting the series into batches of around 10 patches, sending one per week, but that would make too many patches IMHO (14 weeks ...). I'd suggest doing a first batch with only scripts that are not tests and pushing this to git.git. Then the remaining series will be a bit less scary. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Patch Series v3 for "use the $( ... ) construct for command substitution"
Hi, Elia Pinto wrote: > This patch series contain the > > use the $( ... ) construct for command substitution > > patches not already merged in ep/shell-command-substitution > in the mantainer repository. Thanks for working on this. The $() form is less error-prone than ``, so in that sense it can be worthwhile. [...] > Elia Pinto (140): I admit I'm not excited to review these at all. I wonder if it's possible to make the series easier to review. For example: * patch 0 makes preparatory changes to line wrapping or to avoid using `` some places to make an automatic transformation easier * patch 1 introduces a script to transform `` expressions to $() expressions * patch 2 just runs that script If the script is "obviously correct" enough then there is no need to manually go through 140 files after that point. 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. The `` construct is not *that* bad. Another possible direction could be to add a tool to make sure git doesn't get any new uses of ``, to let the changes flow in at a manageable rate without too many cases of "one step forward, one step back". Hope that helps, Jonathan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
