From: Tim Henigan <tim.henigan@gmail.com>
Date: Wed, 22 Feb 2012 02:02:43 GMT
Subject: Re: [PATCH] contrib: added git-diffall
Message-ID: <CAFouetiLmK3dXLRkBh+cTNA_OMPS77xo8z95WK5y4tk-o-UUog@mail.gmail.com>
In-Reply-To: <7vd397g8ic.fsf@alter.siamese.dyndns.org>

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@pobox.com> wrote:
> Tim Henigan <tim.henigan@gmail.com> 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