Re: [PATCH] contrib: added git-diffall

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 [off-list ref] wrote:

Tim Henigan [off-list ref] writes:

quoted

+#!/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?

quoted

+                             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.

quoted

+             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.

quoted

+             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>

quoted

+             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.

quoted

+     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