git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Patch Series v3 for "use the $( ... ) construct for command substitution"
@ 2014-04-04 16:52 Elia Pinto
  2014-04-04 17:29 ` Jonathan Nieder
  0 siblings, 1 reply; 5+ messages in thread
From: Elia Pinto @ 2014-04-04 16:52 UTC (permalink / raw)
  To: git@vger.kernel.org; +Cc: Matthieu Moy, Eric Sunshine

This patch series contain the

 use the $( ... ) construct for command substitution

patches not already merged in ep/shell-command-substitution
in the mantainer repository. It is the version 3 of the
patch series.

I changed the commit message in accordance with those approved,
and I have rebased the patches to the master branch, adjusting where
necessary any conflicts. The patches can be applied also to pu and next.

I hope this is a better way to send the (a long) patch series to git.

Best Regards

------

The following changes since commit 82edd396632501b3dd1901d0f3ae5aa72759b5fb:

  Update draft release notes to 2.0 (2014-04-03 13:40:59 -0700)

are available in the git repository at:

  git@github.com:devzero2000/git-core.git ep/shell-command-substitution-v3

for you to fetch changes up to a4f27b5de27b9848c0720ac2c74a3b82c2531122:

  t7505-prepare-commit-msg-hook.sh: use the $( ... ) construct for
command substitution (2014-04-04 08:03:33 -0700)

----------------------------------------------------------------
Elia Pinto (140):
      howto-index.sh: use the $( ... ) construct for command substitution
      install-webdoc.sh: use the $( ... ) construct for command substitution
      git-checkout.sh: use the $( ... ) construct for command substitution
      git-clone.sh: use the $( ... ) construct for command substitution
      git-commit.sh: use the $( ... ) construct for command substitution
      git-fetch.sh: use the $( ... ) construct for command substitution
      git-ls-remote.sh: use the $( ... ) construct for command substitution
      git-merge.sh: use the $( ... ) construct for command substitution
      git-repack.sh: use the $( ... ) construct for command substitution
      git-resolve.sh: use the $( ... ) construct for command substitution
      git-revert.sh: use the $( ... ) construct for command substitution
      git-tag.sh: use the $( ... ) construct for command substitution
      t9360-mw-to-git-clone.sh: use the $( ... ) construct for command
substitution
      t9362-mw-to-git-utf8.sh: use the $( ... ) construct for command
substitution
      t9365-continuing-queries.sh: use the $( ... ) construct for
command substitution
      test-gitmw-lib.sh: use the $( ... ) construct for command substitution
      t7900-subtree.sh: use the $( ... ) construct for command substitution
      appp.sh: use the $( ... ) construct for command substitution
      txt-to-pot.sh: use the $( ... ) construct for command substitution
      git-pull.sh: use the $( ... ) construct for command substitution
      git-rebase--merge.sh: use the $( ... ) construct for command substitution
      git-rebase.sh: use the $( ... ) construct for command substitution
      git-stash.sh: use the $( ... ) construct for command substitution
      git-web--browse.sh: use the $( ... ) construct for command substitution
      lib-credential.sh: use the $( ... ) construct for command substitution
      lib-cvs.sh: use the $( ... ) construct for command substitution
      lib-gpg.sh: use the $( ... ) construct for command substitution
      p5302-pack-index.sh: use the $( ... ) construct for command substitution
      t0001-init.sh: use the $( ... ) construct for command substitution
      t0010-racy-git.sh: use the $( ... ) construct for command substitution
      t0020-crlf.sh: use the $( ... ) construct for command substitution
      t0025-crlf-auto.sh: use the $( ... ) construct for command substitution
      t0026-eol-config.sh: use the $( ... ) construct for command substitution
      t0030-stripspace.sh: use the $( ... ) construct for command substitution
      t0300-credentials.sh: use the $( ... ) construct for command substitution
      t1000-read-tree-m-3way.sh: use the $( ... ) construct for
command substitution
      t1001-read-tree-m-2way.sh: use the $( ... ) construct for
command substitution
      t1002-read-tree-m-u-2way.sh: use the $( ... ) construct for
command substitution
      t1003-read-tree-prefix.sh: use the $( ... ) construct for
command substitution
      t1004-read-tree-m-u-wf.sh: use the $( ... ) construct for
command substitution
      t1020-subdirectory.sh: use the $( ... ) construct for command substitution
      t1050-large.sh: use the $( ... ) construct for command substitution
      t1100-commit-tree-options.sh: use the $( ... ) construct for
command substitution
      t1401-symbolic-ref.sh: use the $( ... ) construct for command substitution
      t1410-reflog.sh: use the $( ... ) construct for command substitution
      t1511-rev-parse-caret.sh: use the $( ... ) construct for command
substitution
      t1512-rev-parse-disambiguation.sh: use the $( ... ) construct
for command substitution
      t2102-update-index-symlinks.sh: use the $( ... ) construct for
command substitution
      t3030-merge-recursive.sh: use the $( ... ) construct for command
substitution
      t3100-ls-tree-restrict.sh: use the $( ... ) construct for
command substitution
      t3101-ls-tree-dirname.sh: use the $( ... ) construct for command
substitution
      t3210-pack-refs.sh: use the $( ... ) construct for command substitution
      t3403-rebase-skip.sh: use the $( ... ) construct for command substitution
      t3511-cherry-pick-x.sh: use the $( ... ) construct for command
substitution
      t3600-rm.sh: use the $( ... ) construct for command substitution
      t3700-add.sh: use the $( ... ) construct for command substitution
      t3905-stash-include-untracked.sh: use the $( ... ) construct for
command substitution
      t3910-mac-os-precompose.sh: use the $( ... ) construct for
command substitution
      t4006-diff-mode.sh: use the $( ... ) construct for command substitution
      t4010-diff-pathspec.sh: use the $( ... ) construct for command
substitution
      t4012-diff-binary.sh: use the $( ... ) construct for command substitution
      t4013-diff-various.sh: use the $( ... ) construct for command substitution
      t4014-format-patch.sh: use the $( ... ) construct for command substitution
      t4036-format-patch-signer-mime.sh: use the $( ... ) construct
for command substitution
      t4038-diff-combined.sh: use the $( ... ) construct for command
substitution
      t4057-diff-combined-paths.sh: use the $( ... ) construct for
command substitution
      t4116-apply-reverse.sh: use the $( ... ) construct for command
substitution
      t4119-apply-config.sh: use the $( ... ) construct for command substitution
      t4204-patch-id.sh: use the $( ... ) construct for command substitution
      t5000-tar-tree.sh: use the $( ... ) construct for command substitution
      t5003-archive-zip.sh: use the $( ... ) construct for command substitution
      t5100-mailinfo.sh: use the $( ... ) construct for command substitution
      t5300-pack-object.sh: use the $( ... ) construct for command substitution
      t5301-sliding-window.sh: use the $( ... ) construct for command
substitution
      t5302-pack-index.sh: use the $( ... ) construct for command substitution
      t5303-pack-corruption-resilience.sh: use the $( ... ) construct
for command substitution
      t5304-prune.sh: use the $( ... ) construct for command substitution
      t5305-include-tag.sh: use the $( ... ) construct for command substitution
      t5500-fetch-pack.sh: use the $( ... ) construct for command substitution
      t5505-remote.sh: use the $( ... ) construct for command substitution
      t5506-remote-groups.sh: use the $( ... ) construct for command
substitution
      t5510-fetch.sh: use the $( ... ) construct for command substitution
      t5515-fetch-merge-logic.sh: use the $( ... ) construct for
command substitution
      t5516-fetch-push.sh: use the $( ... ) construct for command substitution
      t5517-push-mirror.sh: use the $( ... ) construct for command substitution
      t5520-pull.sh: use the $( ... ) construct for command substitution
      t5522-pull-symlink.sh: use the $( ... ) construct for command substitution
      t5530-upload-pack-error.sh: use the $( ... ) construct for
command substitution
      t5537-fetch-shallow.sh: use the $( ... ) construct for command
substitution
      t5538-push-shallow.sh: use the $( ... ) construct for command substitution
      t5550-http-fetch-dumb.sh: use the $( ... ) construct for command
substitution
      t5551-http-fetch-smart.sh: use the $( ... ) construct for
command substitution
      t5570-git-daemon.sh: use the $( ... ) construct for command substitution
      t5601-clone.sh: use the $( ... ) construct for command substitution
      t5700-clone-reference.sh: use the $( ... ) construct for command
substitution
      t5710-info-alternate.sh: use the $( ... ) construct for command
substitution
      t5900-repo-selection.sh: use the $( ... ) construct for command
substitution
      t6001-rev-list-graft.sh: use the $( ... ) construct for command
substitution
      t6002-rev-list-bisect.sh: use the $( ... ) construct for command
substitution
      t6015-rev-list-show-all-parents.sh: use the $( ... ) construct
for command substitution
      t6032-merge-large-rename.sh: use the $( ... ) construct for
command substitution
      t6034-merge-rename-nocruft.sh: use the $( ... ) construct for
command substitution
      t6132-pathspec-exclude.sh: use the $( ... ) construct for
command substitution
      t7001-mv.sh: use the $( ... ) construct for command substitution
      t7003-filter-branch.sh: use the $( ... ) construct for command
substitution
      t7004-tag.sh: use the $( ... ) construct for command substitution
      t7006-pager.sh: use the $( ... ) construct for command substitution
      t7103-reset-bare.sh: use the $( ... ) construct for command substitution
      t7406-submodule-update.sh: use the $( ... ) construct for
command substitution
      t7408-submodule-reference.sh: use the $( ... ) construct for
command substitution
      t7504-commit-msg-hook.sh: use the $( ... ) construct for command
substitution
      t7602-merge-octopus-many.sh: use the $( ... ) construct for
command substitution
      t7700-repack.sh: use the $( ... ) construct for command substitution
      t8003-blame-corner-cases.sh: use the $( ... ) construct for
command substitution
      t9001-send-email.sh: use the $( ... ) construct for command substitution
      t9100-git-svn-basic.sh: use the $( ... ) construct for command
substitution
      t9101-git-svn-props.sh: use the $( ... ) construct for command
substitution
      t9104-git-svn-follow-parent.sh: use the $( ... ) construct for
command substitution
      t9105-git-svn-commit-diff.sh: use the $( ... ) construct for
command substitution
      t9107-git-svn-migrate.sh: use the $( ... ) construct for command
substitution
      t9108-git-svn-glob.sh: use the $( ... ) construct for command substitution
      t9109-git-svn-multi-glob.sh: use the $( ... ) construct for
command substitution
      t9110-git-svn-use-svm-props.sh: use the $( ... ) construct for
command substitution
      t9114-git-svn-dcommit-merge.sh: use the $( ... ) construct for
command substitution
      t9118-git-svn-funky-branch-names.sh: use the $( ... ) construct
for command substitution
      t9119-git-svn-info.sh: use the $( ... ) construct for command substitution
      t9129-git-svn-i18n-commitencoding.sh: use the $( ... ) construct
for command substitution
      t9130-git-svn-authors-file.sh: use the $( ... ) construct for
command substitution
      t9132-git-svn-broken-symlink.sh: use the $( ... ) construct for
command substitution
      t9137-git-svn-dcommit-clobber-series.sh: use the $( ... )
construct for command substitution
      t9138-git-svn-authors-prog.sh: use the $( ... ) construct for
command substitution
      t9145-git-svn-master-branch.sh: use the $( ... ) construct for
command substitution
      t9150-svk-mergetickets.sh: use the $( ... ) construct for
command substitution
      t9300-fast-import.sh: use the $( ... ) construct for command substitution
      t9350-fast-export.sh: use the $( ... ) construct for command substitution
      t9501-gitweb-standalone-http-status.sh: use the $( ... )
construct for command substitution
      t9901-git-web--browse.sh: use the $( ... ) construct for command
substitution
      test-lib-functions.sh: use the $( ... ) construct for command substitution
      unimplemented.sh: use the $( ... ) construct for command substitution
      t7505-prepare-commit-msg-hook.sh: use the $( ... ) construct for
command substitution

 Documentation/howto-index.sh                    |   12 ++--
 Documentation/install-webdoc.sh                 |    6 +-
 contrib/examples/git-checkout.sh                |    8 +--
 contrib/examples/git-clone.sh                   |   20 +++----
 contrib/examples/git-commit.sh                  |   10 ++--
 contrib/examples/git-fetch.sh                   |    6 +-
 contrib/examples/git-ls-remote.sh               |    4 +-
 contrib/examples/git-merge.sh                   |    4 +-
contrib/examples/git-repack.sh                  |    2 +-
 contrib/examples/git-resolve.sh                 |    2 +-
 contrib/examples/git-revert.sh                  |    2 +-
 contrib/examples/git-tag.sh                     |    2 +-
 contrib/mw-to-git/t/t9360-mw-to-git-clone.sh    |   14 ++---
 contrib/mw-to-git/t/t9362-mw-to-git-utf8.sh     |    4 +-
 contrib/mw-to-git/t/t9365-continuing-queries.sh |    2 +-
 contrib/mw-to-git/t/test-gitmw-lib.sh           |    6 +-
 contrib/subtree/t/t7900-subtree.sh              |    2 +-
 contrib/thunderbird-patch-inline/appp.sh        |   14 ++---
 git-gui/po/glossary/txt-to-pot.sh               |    4 +-
 git-pull.sh                                     |    2 +-
 git-rebase--merge.sh                            |    4 +-
 git-rebase.sh                                   |    8 +--
 git-stash.sh                                    |    2 +-
 git-web--browse.sh                              |    6 +-
 t/lib-credential.sh                             |    2 +-
 t/lib-cvs.sh                                    |    2 +-
 t/lib-gpg.sh                                    |    2 +-
 t/perf/p5302-pack-index.sh                      |    2 +-
 t/t0001-init.sh                                 |   12 ++--
 t/t0010-racy-git.sh                             |    4 +-
 t/t0020-crlf.sh                                 |   42 +++++++-------
 t/t0025-crlf-auto.sh                            |   38 ++++++-------
 t/t0026-eol-config.sh                           |   20 +++----
 t/t0030-stripspace.sh                           |   20 +++----
 t/t0300-credentials.sh                          |    2 +-
 t/t1000-read-tree-m-3way.sh                     |    4 +-
 t/t1001-read-tree-m-2way.sh                     |   18 +++---
 t/t1002-read-tree-m-u-2way.sh                   |   10 ++--
 t/t1003-read-tree-prefix.sh                     |    2 +-
 t/t1004-read-tree-m-u-wf.sh                     |    8 +--
 t/t1020-subdirectory.sh                         |   22 ++++----
 t/t1050-large.sh                                |    4 +-
 t/t1100-commit-tree-options.sh                  |    4 +-
 t/t1401-symbolic-ref.sh                         |    2 +-
 t/t1410-reflog.sh                               |   24 ++++----
 t/t1511-rev-parse-caret.sh                      |    4 +-
 t/t1512-rev-parse-disambiguation.sh             |    8 +--
 t/t2102-update-index-symlinks.sh                |    2 +-
 t/t3030-merge-recursive.sh                      |    2 +-
 t/t3100-ls-tree-restrict.sh                     |    2 +-
 t/t3101-ls-tree-dirname.sh                      |    2 +-
 t/t3210-pack-refs.sh                            |    2 +-
 t/t3403-rebase-skip.sh                          |    2 +-
 t/t3511-cherry-pick-x.sh                        |   14 ++---
 t/t3600-rm.sh                                   |    4 +-
 t/t3700-add.sh                                  |   16 +++---
 t/t3905-stash-include-untracked.sh              |    4 +-
 t/t3910-mac-os-precompose.sh                    |   16 +++---
 t/t4006-diff-mode.sh                            |    2 +-
 t/t4010-diff-pathspec.sh                        |    4 +-
 t/t4012-diff-binary.sh                          |   16 +++---
 t/t4013-diff-various.sh                         |    6 +-
 t/t4014-format-patch.sh                         |   10 ++--
 t/t4036-format-patch-signer-mime.sh             |    2 +-
 t/t4038-diff-combined.sh                        |    2 +-
 t/t4057-diff-combined-paths.sh                  |    2 +-
 t/t4116-apply-reverse.sh                        |   12 ++--
 t/t4119-apply-config.sh                         |    2 +-
t/t4204-patch-id.sh                             |    4 +-
 t/t5000-tar-tree.sh                             |    6 +-
 t/t5003-archive-zip.sh                          |    2 +-
 t/t5100-mailinfo.sh                             |   12 ++--
 t/t5300-pack-object.sh                          |   18 +++---
 t/t5301-sliding-window.sh                       |   14 ++---
 t/t5302-pack-index.sh                           |   34 ++++++------
 t/t5303-pack-corruption-resilience.sh           |    8 +--
 t/t5304-prune.sh                                |    2 +-
 t/t5305-include-tag.sh                          |    8 +--
 t/t5500-fetch-pack.sh                           |   16 +++---
 t/t5505-remote.sh                               |    2 +-
 t/t5506-remote-groups.sh                        |    2 +-
 t/t5510-fetch.sh                                |   10 ++--
 t/t5515-fetch-merge-logic.sh                    |    4 +-
 t/t5516-fetch-push.sh                           |    4 +-
 t/t5517-push-mirror.sh                          |    2 +-
 t/t5520-pull.sh                                 |   10 ++--
 t/t5522-pull-symlink.sh                         |    2 +-
 t/t5530-upload-pack-error.sh                    |    2 +-
 t/t5537-fetch-shallow.sh                        |    4 +-
 t/t5538-push-shallow.sh                         |    4 +-
 t/t5550-http-fetch-dumb.sh                      |    8 +--
 t/t5551-http-fetch-smart.sh                     |    2 +-
 t/t5570-git-daemon.sh                           |    8 +--
 t/t5601-clone.sh                                |    2 +-
 t/t5700-clone-reference.sh                      |    2 +-
 t/t5710-info-alternate.sh                       |    2 +-
 t/t5900-repo-selection.sh                       |    2 +-
 t/t6001-rev-list-graft.sh                       |   12 ++--
 t/t6002-rev-list-bisect.sh                      |    6 +-
 t/t6015-rev-list-show-all-parents.sh            |    6 +-
 t/t6032-merge-large-rename.sh                   |    2 +-
 t/t6034-merge-rename-nocruft.sh                 |    2 +-
 t/t6132-pathspec-exclude.sh                     |    2 +-
 t/t7001-mv.sh                                   |    4 +-
 t/t7003-filter-branch.sh                        |    6 +-
 t/t7004-tag.sh                                  |   16 +++---
 t/t7006-pager.sh                                |    2 +-
 t/t7103-reset-bare.sh                           |    2 +-
 t/t7406-submodule-update.sh                     |    4 +-
 t/t7408-submodule-reference.sh                  |    2 +-
 t/t7504-commit-msg-hook.sh                      |    2 +-
 t/t7505-prepare-commit-msg-hook.sh              |   32 +++++------
 t/t7602-merge-octopus-many.sh                   |    8 +--
 t/t7700-repack.sh                               |    4 +-
 t/t8003-blame-corner-cases.sh                   |    4 +-
 t/t9001-send-email.sh                           |   10 ++--
 t/t9100-git-svn-basic.sh                        |   24 ++++----
 t/t9101-git-svn-props.sh                        |   30 +++++-----
 t/t9104-git-svn-follow-parent.sh                |   48 ++++++++--------
 t/t9105-git-svn-commit-diff.sh                  |    4 +-
 t/t9107-git-svn-migrate.sh                      |   16 +++---
 t/t9108-git-svn-glob.sh                         |   20 +++----
 t/t9109-git-svn-multi-glob.sh                   |   32 +++++------
 t/t9110-git-svn-use-svm-props.sh                |    2 +-
 t/t9114-git-svn-dcommit-merge.sh                |   12 ++--
 t/t9118-git-svn-funky-branch-names.sh           |    2 +-
 t/t9119-git-svn-info.sh                         |    2 +-
 t/t9129-git-svn-i18n-commitencoding.sh          |    4 +-
 t/t9130-git-svn-authors-file.sh                 |   12 ++--
 t/t9132-git-svn-broken-symlink.sh               |    4 +-
 t/t9137-git-svn-dcommit-clobber-series.sh       |   24 ++++----
 t/t9138-git-svn-authors-prog.sh                 |    2 +-
 t/t9145-git-svn-master-branch.sh                |    4 +-
 t/t9150-svk-mergetickets.sh                     |    2 +-
 t/t9300-fast-import.sh                          |   68 +++++++++++------------
 t/t9350-fast-export.sh                          |    6 +-
 t/t9501-gitweb-standalone-http-status.sh        |    6 +-
 t/t9901-git-web--browse.sh                      |    2 +-
 t/test-lib-functions.sh                         |    8 +--
 unimplemented.sh                                |    2 +-
 140 files changed, 592 insertions(+), 592 deletions(-)

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: Patch Series v3 for "use the $( ... ) construct for command substitution"
  2014-04-04 16:52 Patch Series v3 for "use the $( ... ) construct for command substitution" Elia Pinto
@ 2014-04-04 17:29 ` Jonathan Nieder
  2014-04-04 17:40   ` Matthieu Moy
  0 siblings, 1 reply; 5+ messages in thread
From: Jonathan Nieder @ 2014-04-04 17:29 UTC (permalink / raw)
  To: Elia Pinto
  Cc: git@vger.kernel.org, Matthieu Moy, Eric Sunshine, Junio C Hamano

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: Patch Series v3 for "use the $( ... ) construct for command substitution"
  2014-04-04 17:29 ` Jonathan Nieder
@ 2014-04-04 17:40   ` Matthieu Moy
  2014-04-04 18:12     ` Jonathan Nieder
  0 siblings, 1 reply; 5+ messages in thread
From: Matthieu Moy @ 2014-04-04 17:40 UTC (permalink / raw)
  To: Jonathan Nieder
  Cc: Elia Pinto, git@vger.kernel.org, Eric Sunshine, Junio C Hamano

Jonathan Nieder <jrnieder@gmail.com> 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/

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: Patch Series v3 for "use the $( ... ) construct for command substitution"
  2014-04-04 17:40   ` Matthieu Moy
@ 2014-04-04 18:12     ` Jonathan Nieder
  2014-04-04 19:27       ` Junio C Hamano
  0 siblings, 1 reply; 5+ messages in thread
From: Jonathan Nieder @ 2014-04-04 18:12 UTC (permalink / raw)
  To: Matthieu Moy
  Cc: Elia Pinto, git@vger.kernel.org, Eric Sunshine, Junio C Hamano

Matthieu Moy wrote:
> Jonathan Nieder <jrnieder@gmail.com> 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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: Patch Series v3 for "use the $( ... ) construct for command substitution"
  2014-04-04 18:12     ` Jonathan Nieder
@ 2014-04-04 19:27       ` Junio C Hamano
  0 siblings, 0 replies; 5+ messages in thread
From: Junio C Hamano @ 2014-04-04 19:27 UTC (permalink / raw)
  To: Jonathan Nieder
  Cc: Matthieu Moy, Elia Pinto, git@vger.kernel.org, Eric Sunshine

Jonathan Nieder <jrnieder@gmail.com> 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.

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2014-04-04 19:27 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-04-04 16:52 Patch Series v3 for "use the $( ... ) construct for command substitution" Elia Pinto
2014-04-04 17:29 ` Jonathan Nieder
2014-04-04 17:40   ` Matthieu Moy
2014-04-04 18:12     ` Jonathan Nieder
2014-04-04 19:27       ` Junio C Hamano

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).