Tim Henigan <tim.henigan@xxxxxxxxx> writes: > There is no specific reason it must be bash. I changed from > "#!/bin/sh" to "#!/bin/bash -e" due to a bug report from a user on > Ubuntu [1]. The user reported that: > > "If you use /bin/sh on ubuntu you get the dash shell instead of bash shell. > This causes git_merge_tool_path to fail. The error isn't trapped, > so it exits > without displaying anything and without cleaning up." > > Given that all the other scripts distributed with git use /bin/sh, I > will change this script to match. You need to dig back to that bug report deeper and find out what exactly is causing the "failure", before blindly allowing /bin/dash to be used. I think the above function name is a typo of get_merge_tool_path that is borrowed from git-mergetool--lib.sh, but nothing in the function jumps as a blatant bash-ism at me from a quick reading. David, any idea on this? >> The following is only after a cursory scanning, so there may be other >> things that needs fixing, but anyway: >> >> - Don't use "which" in scripts. Its output is not machine parseable, and >> exit code is not reliable, across platforms. It is only meant for >> consumption by human who can read English (or natural language in the >> current locale). > > I used "which" in two places. Both were added to support problems > with missing standard tools on certain platforms (missing mktemp on > msysgit and missing option from tar on Mac [2]). Is there some other > standard way to detect the platform or if certain utils are present? There are examples in the script you are borrowing functions from, even in the function that allegedly fail for the dash user ;-). > The cleanup triggers on all the platforms I have tested (Ubuntu, > msysgit, Mac). I could change it, but for me it has "just worked". You set "trap cleanup" for exit event, and then the control reaches the end of the script, which is an exit event, and the cleanup function is called. So it is natural that if you manage to get to that "trap cleanup" line, of course cleanup will run. But if you dropped "trap" and "EXIT" from that line, it amounts to the same thing. A more important thing to know is that until the control reaches that "trap cleanup EXIT" line, you do not have that trap set. So if you caused the program to exit in an earlier part of the program (say, in the big "while read name" loop) before the control reaches this line, you won't see any cleanup. Perhaps that is what you wanted, but then again writing "trap" and "EXIT" there is pointless. Usually people set up a clean-up task with trap before going into complex stuff, so that no matter where in the complex code the trapped condition (typically a signal, but an exit event is also possible) happens, they can be sure the clean-up task is run. That is the reason behind the following ordering. >> If you are to set up temporary files or directories that you want to clean >> up, a good discipline is to follow this order: >> >> - define variable(s) to hold the temporary locations, e.g. >> tmpdir=$(mktemp ...) >> >> - set the trap before starting to use these temporary locations, e.g. >> trap 'rm -rf "$tmpdir' 0 1 2 3 15 >> >> - and then start populating tmpdir and do whatever you want to do. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html