Skip to contentSkip to conversation

filter-branch: Avoid an error message in the map function.

PATCH In review Johannes Sixt 3 participants 6 messages Jun 15, 2016

lore
Share & tools
lore

Johannes Sixt

author
#0 lore raw

PATCH 1/1 filter-branch: Avoid an error message in the map function.

When the map function didn't find the rewritten commit of the passed in original id, it printed the original id, but it still fell through to the 'cat', which failed with an error message.

Signed-off-by Johannes Sixt <[email protected]>

---
Is the sequence of && and || ok, or do you prefer if-then-else-fi?

-- Hannes

 git-filter-branch.sh |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)
git-filter-branch.sh +3 -2
index 22fb5bf..ff1cbcf 100644
@@ -16,8 +16,9 @@ USAGE="git-filter-branch [-d TEMPDIR] [FILTERS] DESTBRANCH [REV-RANGE]"
 map()
 {
 	# if it was not rewritten, take the original
-	test -r "$workdir/../map/$1" || echo "$1"
-	cat "$workdir/../map/$1"
+	test -r "$workdir/../map/$1" &&
+	cat "$workdir/../map/$1" ||
+	echo "$1"
 }
 
 # When piped a commit, output a script to set the ident of either
Signature · 1 line
1.5.3.rc0.5.g7cd9
#1 lore raw

Hi,

git-filter-branch.sh USAGE="git-filter-branch [-d TEMPDIR] [FILTERS] DESTBRANCH [REV-RANGE]" · line 21

Quoted 5 lines · Johannes Sixt
git-filter-branch.sh +3 -2
-	test -r "$workdir/../map/$1" || echo "$1"
-	cat "$workdir/../map/$1"
+	test -r "$workdir/../map/$1" &&
+	cat "$workdir/../map/$1" ||
+	echo "$1"

I think this does not do what you want. If I read it correctly, it will not do anything if $workdir/../map/$1 is not readable. I think you need this:

	(test -r "$workdir/../map/$1" &&
	cat "$workdir/../map/$1") ||
	echo "$1"

But that is a little too cute, so I personally would prefer an if-then-else-fi, because that is the idea of that code snippet.

Ciao,
Dscho

#2 lore raw
Quoted 15 lines · Johannes Schindelin

Hi,

git-filter-branch.sh USAGE="git-filter-branch [-d TEMPDIR] [FILTERS] DESTBRANCH [REV-RANGE]" · line 21

Quoted 5 lines · Johannes Sixt
git-filter-branch.sh +3 -2
-     test -r "$workdir/../map/$1" || echo "$1"
-     cat "$workdir/../map/$1"
+     test -r "$workdir/../map/$1" &&
+     cat "$workdir/../map/$1" ||
+     echo "$1"

I think this does not do what you want. If I read it correctly, it will not do anything if $workdir/../map/$1 is not readable. I think you need this:

Quoted 6 lines
        (test -r "$workdir/../map/$1" &&
        cat "$workdir/../map/$1") ||
        echo "$1"

But that is a little too cute, so I personally would prefer an if-then-else-fi, because that is the idea of that code snippet.

It does do what I think it should do. I tested it. Your elaborate version is not required. The reason is that in the shell && and || have equal precedence; if there is ... && cmd ... then cmd is run if the most recent result is success, and if there is ... || cmd ... then cmd is run if the most recent result is failure; in both cases cmd is otherwise skipped and does not count as "most recent result".

-- Hannes

#3 lore raw

Hi,

Quoted 14 lines · Johannes Sixt
Quoted 13 lines · Johannes Schindelin

git-filter-branch.sh USAGE="git-filter-branch [-d TEMPDIR] [FILTERS] DESTBRANCH [REV-RANGE]" · line 21

Quoted 5 lines · Johannes Sixt
git-filter-branch.sh +3 -2
-     test -r "$workdir/../map/$1" || echo "$1"
-     cat "$workdir/../map/$1"
+     test -r "$workdir/../map/$1" &&
+     cat "$workdir/../map/$1" ||
+     echo "$1"

I think this does not do what you want. If I read it correctly, it will not do anything if $workdir/../map/$1 is not readable. I think you need this:

Quoted 8 lines
Quoted 6 lines
        (test -r "$workdir/../map/$1" &&
        cat "$workdir/../map/$1") ||
        echo "$1"

But that is a little too cute, so I personally would prefer an if-then-else-fi, because that is the idea of that code snippet.

It does do what I think it should do. I tested it.

Okay. But take me as an example of an average programmer. I got confused. Therefore I would greatly appreciate it, if it were written with if-then-else-fi, because I get less confused then.

Thanks,
Dscho

#4 lore raw

PATCH 1/1 filter-branch: Avoid an error message in the map function.

When the map function didn't find the rewritten commit of the passed in original id, it printed the original id, but it still fell through to the 'cat', which failed with an error message.

Signed-off-by Johannes Sixt <[email protected]>
---
 git-filter-branch.sh |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)
git-filter-branch.sh +6 -2
index 22fb5bf..5fa9b61 100644
@@ -16,8 +16,12 @@
 map()
 {
 	# if it was not rewritten, take the original
-	test -r "$workdir/../map/$1" || echo "$1"
-	cat "$workdir/../map/$1"
+	if test -r "$workdir/../map/$1"
+	then
+		cat "$workdir/../map/$1"
+	else
+		echo "$1"
+	fi
 }
 
 # When piped a commit, output a script to set the ident of either
Signature · 1 line
1.5.3.rc0.5.g7cd9