Closed GoogleCodeExporter closed 8 years ago
How can I recreate the bug? It's not clear that the patch does anything
different; if the directory exists then [ -d dir ] will be true (and thus not
do the exit).
Is this a case of using a different shell instead of bash?
Original comment by alex.ble...@gmail.com
on 25 Nov 2010 at 7:31
> how can I recreate the bug?
steps 1+2 let you recreate the problem.
> it's not clear...
the problem is that the code doesn't do what you think it will do when the
directory *does* exist.
> Is this a case of using a different shell instead of bash?
Nope, I'm using bash. the shell script is just broken.
I spoke to a couple of people about this and determined that people don't
really understand the subtle pitfalls of shell scripting.
Here's the technical explanation of the relevant operations from BASH(1):
Lists
A list is a sequence of one or more pipelines separated by one of the operators ;, &,
&&, or ||, and optionally terminated by one of ;, &, or <newline>.
Of these list operators, && and || have equal precedence, followed by ; and &, which
have equal precedence.
...
Compound Commands
A compound command is one of the following:
...
{ list; }
list is simply executed in the current shell environment.
The key is that the order of operations is such that ; causes the stuff before
it to be evaluated completely, and then the stuff after it is executed
independently.
------
As that's unlikely to be helpful, I was suggested to explain the current code
> [ -d build/${CONFIGURATION} ] || echo build/${CONFIGURATION} not found; exit 1
> sudo cp -r build/${CONFIGURATION}/zfs.kext /System/Library/Extensions
... as if it were C instead of Shell:
bool test_dir() {
return is_dir("build/configuration");
}
void not_found() {
printf("build/configuration not found\n");
}
void setup() {
system("sudo cp ...");
}
int main(void) {
if (test_dir()) not_found(); exit 1;
setup();
return 0;
}
indented for clarity:
int main(void) {
if (test_dir())
not_found();
exit 1;
setup();
return 0;
}
This obviously wouldn't be the C that you'd want to write....
In C, you could do this:
if (test_dir()) { not_found(); exit 1; }
setup();
which is equivalent to:
if (test_dir()) {
not_found();
exit 1;
}
setup();
So, there are two ways to fix the current code:
> [ -d build/${CONFIGURATION} ] || { echo build/${CONFIGURATION} not found;
exit 1 }
> sudo cp -r build/${CONFIGURATION}/zfs.kext /System/Library/Extensions
or the attachment form. For people who are unfamiliar w/ shell scripting, I
think that using if ; then ; fi is probably clearer than using {}s. The
argument against using {} is that it isn't portable. It doesn't work on (t)csh
or dash.
Original comment by timel...@gmail.com
on 25 Nov 2010 at 9:49
OK, gotcha. My bad.
Original comment by alex.ble...@gmail.com
on 9 Dec 2010 at 2:18
Fixed in
https://github.com/alblue/mac-zfs/commit/653b45b6b670bc9a179936017e3c7ca655e9c82
b
Will leave this as fixed until this becomes part of an installed build, which
the 'untested' branch currently is not.
Original comment by alex.ble...@gmail.com
on 11 Dec 2010 at 2:18
Original issue reported on code.google.com by
timel...@gmail.com
on 10 Nov 2010 at 1:42Attachments: