Re: [PATCH] Display change history as a diff between two dirs

[Roland Kaufmann <rlndkfmn+git@gmail.com>]

On 2011-10-30 07:09, Junio C Hamano wrote:
> I do not think any of our scripted Porcelains use "set -e"; rather
> they try to catch errors and produce messages that are more
> appropriate for specific error sites.

git-dirdiff being a wrapper script, I reasoned that the underlaying
command would already have printed a sufficient error message, so what
was left was just to exit. But you're right in that some of the commands
will not give sufficient context. I'll put in some more detailed error
handling.

> I do not think any of our scripted Porcelains use "mktemp"
> especially "mktemp -d". How portable is it?

Even if it is not part of Posix, I reckon since it is a part of the
coreutils, it is available in most GNU userlands, inclusive GnuWin32.
I don't have any live BSD systems available, but based on the man pages
it seems to be available there too, albeit with some different options.

--tmpdir seems to be more of a problem in than respect than -d. I'll see
if I can find a set of options that are digestable to most platforms.

I think it is unfortunate to use the current directory as scratch space;
it can be a network disk, or even read-only. mktemp can in contrast make
a directory in a place which is not spilled to disk.

> It is not clear to me why you need to grab the list of paths in toc
> and iterate over them one by one. IOW, why isn't this sufficient?

This design decision was probably appropriate in an earlier iteration,
but as you point out, it is indeed superfluous now! I apologize for not
realizing that myself.

> suspect is true), we would probably want to make it an option to
> "git diff" itself, and not a separate git subcommand like "dirdiff".
> I can see

Although being an able *user* of Git, I am not (yet) familiar enough
with the codebase to have a patch ready for `git diff` itself. It would
certainly require more rigorous testing.

> "git diff" (and possibly even things like "git log -p") populating
> two temporary directories (your old/ and new/) and running a custom
> program specified by GIT_EXTERNAL_TREEDIFF environment variable,
> instead of doing

Would it be better to have yet another configuration available for this 
option instead of reusing the existing infrastructure for `git difftool`?

> It also is not clear what could be used in "$@". Obviously you would
>  not want things like "-U20" and "--stat" fed to the first "list of
> paths" phase, but there may be some options you may want to give to
> the inner "external diff" thing.

Ideally, it should work the same way as `git difftool`.

> For example, how well does this approach work with -M/-C (I do not
> think it would do anything useful, but I haven't thought things
> through).  It would be nice if we gave the hint to the external

As of now, files that are moved will turn up a different places in the
tree without any annotations, and off the top of my head I don't see any
obvious way to propagate such hints. If the diff tool is sophisticated
can probably use the same heuristics as Git currently does, but I am
unaware of any that is able to do that yet.

> I wouldn't mind carrying a polished version of this in contrib/ for
> a cycle or two in order to let people try it out and get kinks out of
>  its design.

I would appreciate that! I'll submit a reworked proposal taking you
comments into account. It may take a few days due to unrelated
engagements, though.

-- 
   Roland.