Thank you for taking the time to review this patch. I appreciate it.
On Tue, Feb 21, 2012 at 4:51 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Tim Henigan <tim.henigan@xxxxxxxxx> writes:
>> +#!/bin/bash -e
>
> Does this have to be bash-only (iow, infested with bash-isms beyond
> repair), or is you wrote this merely from inertia?
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.
> 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?
>> + if test -z "$paths"
>> + then
>> + paths=$1
>> + else
>> + paths="$paths $1"
>> + fi
>
> Just a style tip; if you are going to let shell $IFS split this list
> anyway, it is customary to write the above as
>
> paths="$paths$1 "
Nice...I will change to use this format.
>> + git diff --name-only "$left"..."$right" -- $paths > "$tmp"/filelist
>
> git diff ... -- $paths >"$tmp/filelist"
... <snip>
> mkdir -p "$tmp/$left_dir" "$tmp/$right_dir"
Will change all instances.
>> + if test -n "$compare_staged"
>> + then
>> + ls_list=$(git ls-tree HEAD $name)
>> + if test -n "$ls_list"
>> + then
>> + mkdir -p "$tmp"/"$left_dir"/"$(dirname "$name")"
>> + git show HEAD:"$name" > "$tmp"/"$left_dir"/"$name"
>> + fi
>> + else
>> + mkdir -p "$tmp"/"$left_dir"/"$(dirname "$name")"
>> + git show :"$name" > "$tmp"/"$left_dir"/"$name"
>> + fi
>
> That's misleadingly indented. First I thought "in what case would we want
> to switch the LHS between HEAD:$path and :$path when doing diff --cached?"
> but the overindented four lines starting from the funny "fi" is about non
> cached case.
That is embarrassing. I will fix it.
<snip>
>> + find "$tmp/$right_dir" -type f|while read file; do
>> + cp "$file" "$git_top_dir/${file#$tmp/$right_dir/}"
>> + done
>
> Why is this loop written in such a dense way? Everything else (except for
> that misindented part) were almost to our CodingStyle and was fairly easy
> to read, though.
I missed this in my style cleanup. I will fix it.
>> + fi
>> +
>> + # Remove the tmp directory
>> + rm -rf "$tmp"
>> +}
>> +
>> +trap cleanup EXIT
>
> Does this even trigger? This is not Perl that parses and runs set-up code
> before executing everything else, so I suspect this last line amounts to
> the same thing as writing just
>
> cleanup
>
> without trap nor signal names.
The cleanup triggers on all the platforms I have tested (Ubuntu,
msysgit, Mac). I could change it, but for me it has "just worked".
> 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.
I will review the changes needed for this before submitting v2 of my patch.
[1]: https://github.com/thenigan/git-diffall/pull/9
[2]: https://github.com/thenigan/git-diffall/pull/2#issuecomment-498472
--
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