threads / patch / 18549

patchgit-filter-branch: avoid collisions with variables in eval'ed commands

Subject: [PATCH] git-filter-branch: avoid collisions with variables in eval'ed commands

## tl;dr

6 messages between Mar 25, 2009 and Mar 25, 2009. Diffs are folded; open one to read it.

replies: 5people: 3as markdown or json

newren@gmail.com· Mar 25, 2009, 21:13 UTC · lore
From: Elijah Newren <newren@gmail.com>

Avoid using simple variable names like 'i', since user commands are eval'ed and may clash with and overwrite our values.

Signed-off-by: Elijah Newren <newren@gmail.com>
---

I discovered this a few months ago, but apparently never got around to sending it earlier. Anyway, without this patch in a repository with a file called 'world' I see the following behavior:

$ git filter-branch --tree-filter '
   for i in $(find . -type f); do
     if ( file $i | grep "\btext\b" > /dev/null ); then
       perl -i -ple "s/\\\$(Id|Date|Source|Header|CVSHeader|Author|Revision):[^\
\$]*\\$/\\$\1\\$/" $i;
     fi;
   done ' -- --all

Rewrite 7a78604e90b9debfaa6a755019b5d3df541abcba (1/6)/usr/bin/git-filter-branch : line 249: ./world+1: syntax error: operand expected (error token is "./world+1 ")

In case anyone is wondering: This isn't the right way to nuke CVS keywords. It's a good start, but it'll miss a few files (particularly with older versions of the 'file' program) due to some types such as 'FORTRAN program source' not containing 'text'. Also, it's incredibly slow to the point that I dropped filter-branch entirely and wrote a 'git_fast_filter' helper (serving as a pipe between fast-export and fast-import) that I'll soon be making available.

 git-filter-branch.sh |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)
Show changes to git-filter-branch.sh +3 −3
diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index 20f6f51..5da63b1 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -272,10 +272,10 @@ test $commits -eq 0 && die "Found nothing to rewrite"
 
 # Rewrite the commits
 
-i=0
+git_filter_branch_count=0
 while read commit parents; do
-	i=$(($i+1))
-	printf "\rRewrite $commit ($i/$commits)"
+	git_filter_branch_count=$(($git_filter_branch_count+1))
+	printf "\rRewrite $commit ($git_filter_branch_count/$commits)"
 
 	case "$filter_subdir" in
 	"")

-- 
1.6.0.6
Petr Baudis· Mar 25, 2009, 21:24 UTC · re: newren@gmail.com · lore

Re: [PATCH] git-filter-branch: avoid collisions with variables in eval'ed commands

On Wed, Mar 25, 2009 at 03:13:01PM -0600, newren@gmail.com wrote:
Show 6 quoted lines
> From: Elijah Newren <newren@gmail.com>
> 
> Avoid using simple variable names like 'i', since user commands are eval'ed
> and may clash with and overwrite our values.
> 
> Signed-off-by: Elijah Newren <newren@gmail.com>
Almost-acked-by: Petr Baudis <pasky@suse.cz>
But:
>-i=0
>+git_filter_branch_count=0
Why branch_count? It counts commits, not branches, doesn't it?
> I discovered this a few months ago, but apparently never got around to
> sending it earlier.  Anyway, without this patch in a repository with a
> file called 'world' I see the following behavior:
Some hints:
> $ git filter-branch --tree-filter '
>    for i in $(find . -type f); do
This won't work right if your filenames contain $IFS.
>      if ( file $i | grep "\btext\b" > /dev/null ); then

if [[ "$(file $i)" == *text* ]] might run noticeably faster (though is slightly less precise). Having a filename-keyed cache of file types even more so.

>        perl -i -ple "s/\\\$(Id|Date|Source|Header|CVSHeader|Author|Revision):[^\
> \$]*\\$/\\$\1\\$/" $i;

Using '\'' instead of " could save you quite a few backslashes in net count.

>      fi;
>    done ' -- --all
-- 
				Petr "Pasky" Baudis
The average, healthy, well-adjusted adult gets up at seven-thirty
in the morning feeling just terrible. -- Jean Kerr
Elijah Newren· Mar 25, 2009, 21:33 UTC · re: Petr Baudis · lore

Re: [PATCH] git-filter-branch: avoid collisions with variables in eval'ed commands

Hi,
On Wed, Mar 25, 2009 at 3:24 PM, Petr Baudis <pasky@suse.cz> wrote:
Show 16 quoted lines
> On Wed, Mar 25, 2009 at 03:13:01PM -0600, newren@gmail.com wrote:
>> From: Elijah Newren <newren@gmail.com>
>>
>> Avoid using simple variable names like 'i', since user commands are eval'ed
>> and may clash with and overwrite our values.
>>
>> Signed-off-by: Elijah Newren <newren@gmail.com>
>
> Almost-acked-by: Petr Baudis <pasky@suse.cz>
>
> But:
>
>>-i=0
>>+git_filter_branch_count=0
>
> Why branch_count? It counts commits, not branches, doesn't it?

Oh, I was just changing i->git_filter_branch_i, then thought as long as it was long I might as well use a word instead of "i". Didn't think about the combined meaning. How about "git_filter_branch_commit_count"? Maybe a double underscore between the "namespace" and the "variable"?

Show 10 quoted lines
>> I discovered this a few months ago, but apparently never got around to
>> sending it earlier.  Anyway, without this patch in a repository with a
>> file called 'world' I see the following behavior:
>
> Some hints:
>
>> $ git filter-branch --tree-filter '
>>    for i in $(find . -type f); do
>
> This won't work right if your filenames contain $IFS.

Yeah, luckily for me, my repository didn't have any filenames with whitespace. :-)

Show 11 quoted lines
>>      if ( file $i | grep "\btext\b" > /dev/null ); then
>
> if [[ "$(file $i)" == *text* ]] might run noticeably faster (though is
> slightly less precise). Having a filename-keyed cache of file types even
> more so.
>
>>        perl -i -ple "s/\\\$(Id|Date|Source|Header|CVSHeader|Author|Revision):[^\
>> \$]*\\$/\\$\1\\$/" $i;
>
> Using '\'' instead of " could save you quite a few backslashes in net
> count.
Cool, thanks for the tips.
newren@gmail.com· Mar 25, 2009, 21:51 UTC · re: Elijah Newren · lore

Re: [PATCH] git-filter-branch: avoid collisions with variables in eval'ed commands

From: Elijah Newren <newren@gmail.com>

Avoid using simple variable names like 'i', since user commands are eval'ed and may clash with and overwrite our values.

Signed-off-by: Elijah Newren <newren@gmail.com>
---

If you have a different preference for the variable name, I'd be happy to change it (again). All I care about is avoiding conflicts with what people might pass in their scripts.

 git-filter-branch.sh |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)
Show changes to git-filter-branch.sh +3 −3
diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index 20f6f51..b90d3df 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -272,10 +272,10 @@ test $commits -eq 0 && die "Found nothing to rewrite"
 
 # Rewrite the commits
 
-i=0
+git_filter_branch__commit_count=0
 while read commit parents; do
-	i=$(($i+1))
-	printf "\rRewrite $commit ($i/$commits)"
+	git_filter_branch__commit_count=$(($git_filter_branch__commit_count+1))
+	printf "\rRewrite $commit ($git_filter_branch__commit_count/$commits)"
 
 	case "$filter_subdir" in
 	"")

-- 
1.6.0.6
Petr Baudis· Mar 25, 2009, 21:58 UTC · re: Elijah Newren · lore

Re: [PATCH] git-filter-branch: avoid collisions with variables in eval'ed commands

On Wed, Mar 25, 2009 at 03:33:31PM -0600, Elijah Newren wrote:
Show 25 quoted lines
> Hi,
> 
> On Wed, Mar 25, 2009 at 3:24 PM, Petr Baudis <pasky@suse.cz> wrote:
> > On Wed, Mar 25, 2009 at 03:13:01PM -0600, newren@gmail.com wrote:
> >> From: Elijah Newren <newren@gmail.com>
> >>
> >> Avoid using simple variable names like 'i', since user commands are eval'ed
> >> and may clash with and overwrite our values.
> >>
> >> Signed-off-by: Elijah Newren <newren@gmail.com>
> >
> > Almost-acked-by: Petr Baudis <pasky@suse.cz>
> >
> > But:
> >
> >>-i=0
> >>+git_filter_branch_count=0
> >
> > Why branch_count? It counts commits, not branches, doesn't it?
> 
> Oh, I was just changing i->git_filter_branch_i, then thought as long
> as it was long I might as well use a word instead of "i".  Didn't
> think about the combined meaning.  How about
> "git_filter_branch_commit_count"?  Maybe a double underscore between
> the "namespace" and the "variable"?

Oh, I sort of thought it's "git-filter branch count", not "git-filter-branch count". I'm dumbed down from all day spent in IKEA I guess. :/

I'd personally just use $__git_i, $__git_commit or something, but YMMV.
-- 
				Petr "Pasky" Baudis
The average, healthy, well-adjusted adult gets up at seven-thirty
in the morning feeling just terrible. -- Jean Kerr
Junio C Hamano· Mar 25, 2009, 23:34 UTC · re: Petr Baudis · lore

Re: [PATCH] git-filter-branch: avoid collisions with variables in eval'ed commands

Petr Baudis <pasky@suse.cz> writes:
Show 32 quoted lines
> On Wed, Mar 25, 2009 at 03:33:31PM -0600, Elijah Newren wrote:
>> Hi,
>> 
>> On Wed, Mar 25, 2009 at 3:24 PM, Petr Baudis <pasky@suse.cz> wrote:
>> > On Wed, Mar 25, 2009 at 03:13:01PM -0600, newren@gmail.com wrote:
>> >> From: Elijah Newren <newren@gmail.com>
>> >>
>> >> Avoid using simple variable names like 'i', since user commands are eval'ed
>> >> and may clash with and overwrite our values.
>> >>
>> >> Signed-off-by: Elijah Newren <newren@gmail.com>
>> >
>> > Almost-acked-by: Petr Baudis <pasky@suse.cz>
>> >
>> > But:
>> >
>> >>-i=0
>> >>+git_filter_branch_count=0
>> >
>> > Why branch_count? It counts commits, not branches, doesn't it?
>> 
>> Oh, I was just changing i->git_filter_branch_i, then thought as long
>> as it was long I might as well use a word instead of "i".  Didn't
>> think about the combined meaning.  How about
>> "git_filter_branch_commit_count"?  Maybe a double underscore between
>> the "namespace" and the "variable"?
>
> Oh, I sort of thought it's "git-filter branch count", not
> "git-filter-branch count". I'm dumbed down from all day spent in IKEA
> I guess. :/
>
> I'd personally just use $__git_i, $__git_commit or something, but YMMV.
Heh, I'll drop lmost-a from your earlier message and commit the result.
Thanks.

← back to recent threads