Re: [PATCH v2 001/142] check-builtins.sh: use the $( ... ) construct for command substitution
David Kastrup writes:
> Junio C Hamano writes:
>
>> Elia Pinto writes:
>>
>>> The Git CodingGuidelines prefer the $( ... ) construct for command
>>> substitution instead of using the back-quotes, or grave accents (`..`).
>>>
>>> The backquoted form is the historical method for command substitution,
>>> and is supported by POSIX. However, all but the simplest uses become
>>> complicated quickly. In particular, embedded command substitutions
>>> and/or the use of double quotes require careful escaping with the backslash
>>> character. Because of this the POSIX shell adopted the $(…) feature from
>>> the Korn shell.
>>>
>>> The patch was generated by the simple script
>>>
>>> for _f in $(find . -name "*.sh")
>>> do
>>> sed -i 's@`\(.*\)`@$(\1)@g' ${_f}
>>> done
>>
>> "and then carefully proofread" is sorely needed here.
>
> It would already help to skip comment lines.
Actually no: most comments including `...` are code examples that we
want to fix too, except 3 instances (see my other message).
--
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 v2 001/142] check-builtins.sh: use the $( ... ) construct for command substitution
Junio C Hamano writes:
> Elia Pinto writes:
>
>> The Git CodingGuidelines prefer the $( ... ) construct for command
>> substitution instead of using the back-quotes, or grave accents (`..`).
>>
>> The backquoted form is the historical method for command substitution,
>> and is supported by POSIX. However, all but the simplest uses become
>> complicated quickly. In particular, embedded command substitutions
>> and/or the use of double quotes require careful escaping with the backslash
>> character. Because of this the POSIX shell adopted the $(…) feature from
>> the Korn shell.
>>
>> The patch was generated by the simple script
>>
>> for _f in $(find . -name "*.sh")
>> do
>> sed -i 's@`\(.*\)`@$(\1)@g' ${_f}
>> done
>
> "and then carefully proofread" is sorely needed here.
It would already help to skip comment lines.
--
David Kastrup
--
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 v2 001/142] check-builtins.sh: use the $( ... ) construct for command substitution
Elia Pinto writes:
> for _f in $(find . -name "*.sh")
> do
> sed -i 's@`\(.*\)`@$(\1)@g' ${_f}
> done
What does this do in the case there are multiple ` on the same line?
(nested backquotes or multiple `...` `...` on the same line)
There are not many instances, and it seems no nested backquotes in Git's
source code:
$ git grep '`.*`.*`' -- '*.sh'
t/t5506-remote-groups.sh: if test "`git log -1 --pretty=format:%s $1 --`"
= "`cat mark`"; then
t/t9300-fast-import.sh: test `git rev-parse master` = `git rev-parse
TEMP_TAG^`'
t/t9300-fast-import.sh: test `git rev-parse N2^{tree}` = `git rev-parse
N3^{tree}`'
t/t9300-fast-import.sh: test `git rev-parse N3` = `git rev-parse O1`'
t/t9300-fast-import.sh: test `git rev-parse N3` = `git rev-parse O2`'
t/t9300-fast-import.sh: test `git rev-parse refs/tags/O3-2nd` = `git rev-parse
O3^` &&
The instance in t5506-remote-groups.sh is correct (probably fixed by
hand, which would deserve to be in the commit message).
I did not receive the patch about t9300-fast-import.sh (the serie seems
to have stopped at PATCH 105/144).
The other problematic case I can think of is `...` in comments. It seems
I caught the problematic ones in t/t0204-gettext-reencode-sanity.sh and
t/t6111-rev-list-treesame.sh, and git-pull.sh was correct already.
$ git grep '#.*`.*`' -- '*.sh'
git-pull.sh:# Setup default fast-forward options via `pull.ff`
t/t0204-gettext-reencode-sanity.sh:# FreeBSD: `einfaldar` og "tvöfaldar"
[GNU libintl]
t/t4057-diff-combined-paths.sh:# `git diff -c --name-only HEAD HEAD^ HEAD^2`
t/t5522-pull-symlink.sh:# "cd `git rev-parse --show-cdup`", it ended up in the
wrong
t/t6111-rev-list-treesame.sh:#`-C-' `-*I-*J
t/test-lib-functions.sh:# for i in `test_seq 100`; do
t/test-lib-functions.sh:# for j in `test_seq 10 20`; do
t/test-lib-functions.sh:# for k in `test_seq a z`; do
Again, something to mention in commit messages I think.
The first review I did was completely manual, but I would not call it a
thorough one.
It's probably a better idea to publish your branch somewhere than to
continue sending batches of 140 emails to the list for this kind of
topics.
--
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 v2 001/142] check-builtins.sh: use the $( ... ) construct for command substitution
Junio C Hamano writes: > I've reworded the above like so: > > check-builtins.sh: use the $(...) construct for command substitution > > The Git CodingGuidelines prefer the $(...) construct for command > substitution instead of using the backquotes, or grave accents > (`...`). ... but I think the ", or grave accents" should go. -- 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 v2 001/142] check-builtins.sh: use the $( ... ) construct for command substitution
Junio C Hamano writes:
> Elia Pinto writes:
>
>> The Git CodingGuidelines prefer the $( ... ) construct for command
>> substitution instead of using the back-quotes, or grave accents (`..`).
>>
>> The backquoted form is the historical method for command substitution,
>> and is supported by POSIX. However, all but the simplest uses become
>> complicated quickly. In particular, embedded command substitutions
>> and/or the use of double quotes require careful escaping with the backslash
>> character. Because of this the POSIX shell adopted the $(…) feature from
>> the Korn shell.
>>
>> The patch was generated by the simple script
>>
>> for _f in $(find . -name "*.sh")
>> do
>> sed -i 's@`\(.*\)`@$(\1)@g' ${_f}
>> done
>
> "and then carefully proofread" is sorely needed here.
>
> What is that non-breaking space doing at the beginning of an
> indented line, or is it just my environment, by the way?
I've reworded the above like so:
check-builtins.sh: use the $(...) construct for command substitution
The Git CodingGuidelines prefer the $(...) construct for command
substitution instead of using the backquotes, or grave accents
(`...`).
The backquoted form is the historical method for command
substitution, and is supported by POSIX. However, all but the
simplest uses become complicated quickly. In particular, embedded
command substitutions and/or the use of double quotes require
careful escaping with the backslash character.
The patch was generated by:
for _f in $(find . -name "*.sh")
do
sed -i 's@`\(.*\)`@$(\1)@g' ${_f}
done
and then carefully proof-read.
Signed-off-by: Elia Pinto
Signed-off-by: Junio C Hamano
I looked at [*1*] to pick between "back-quote" and "backquote"; this
is a related tangent, but in that page, you will find this gem:
Within the backquoted style of command substitution,
shall retain its literal meaning, except when followed by: '$',
'`', or .
Stated another way, `` => $() conversion will make backslash inside
to bahave differently, and we would need to be careful when doing
such a change.
I've looked at and queued 001 and 002.
[Reference]
*1*
http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_06_03
--
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 v2 001/142] check-builtins.sh: use the $( ... ) construct for command substitution
Elia Pinto writes:
> The Git CodingGuidelines prefer the $( ... ) construct for command
> substitution instead of using the back-quotes, or grave accents (`..`).
>
> The backquoted form is the historical method for command substitution,
> and is supported by POSIX. However, all but the simplest uses become
> complicated quickly. In particular, embedded command substitutions
> and/or the use of double quotes require careful escaping with the backslash
> character. Because of this the POSIX shell adopted the $(…) feature from
> the Korn shell.
>
> The patch was generated by the simple script
>
> for _f in $(find . -name "*.sh")
> do
> sed -i 's@`\(.*\)`@$(\1)@g' ${_f}
> done
"and then carefully proofread" is sorely needed here.
What is that non-breaking space doing at the beginning of an
indented line, or is it just my environment, by the way?
> Signed-off-by: Elia Pinto
> ---
> check-builtins.sh |4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/check-builtins.sh b/check-builtins.sh
> index d6fe6cf..07cff69 100755
> --- a/check-builtins.sh
> +++ b/check-builtins.sh
> @@ -14,8 +14,8 @@ sort |
> bad=0
> while read builtin
> do
> - base=`expr "$builtin" : 'git-\(.*\)'`
> - x=`sed -ne 's/.*{ "'$base'", \(cmd_[^, ]*\).*/'$base' \1/p' git.c`
> + base=$(expr "$builtin" : 'git-\(.*\)')
> + x=$(sed -ne 's/.*{ "'$base'", \(cmd_[^, ]*\).*/'$base' \1/p' git.c)
> if test -z "$x"
> then
> echo "$base is builtin but not listed in git.c command list"
Looks ok to me.
--
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
[PATCH v2 001/142] check-builtins.sh: use the $( ... ) construct for command substitution
The Git CodingGuidelines prefer the $( ... ) construct for command
substitution instead of using the back-quotes, or grave accents (`..`).
The backquoted form is the historical method for command substitution,
and is supported by POSIX. However, all but the simplest uses become
complicated quickly. In particular, embedded command substitutions
and/or the use of double quotes require careful escaping with the backslash
character. Because of this the POSIX shell adopted the $(…) feature from
the Korn shell.
The patch was generated by the simple script
for _f in $(find . -name "*.sh")
do
sed -i 's@`\(.*\)`@$(\1)@g' ${_f}
done
Signed-off-by: Elia Pinto
---
check-builtins.sh |4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/check-builtins.sh b/check-builtins.sh
index d6fe6cf..07cff69 100755
--- a/check-builtins.sh
+++ b/check-builtins.sh
@@ -14,8 +14,8 @@ sort |
bad=0
while read builtin
do
- base=`expr "$builtin" : 'git-\(.*\)'`
- x=`sed -ne 's/.*{ "'$base'", \(cmd_[^, ]*\).*/'$base' \1/p' git.c`
+ base=$(expr "$builtin" : 'git-\(.*\)')
+ x=$(sed -ne 's/.*{ "'$base'", \(cmd_[^, ]*\).*/'$base' \1/p' git.c)
if test -z "$x"
then
echo "$base is builtin but not listed in git.c command list"
--
1.7.10.4
--
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
