From: David Aguilar <davvid@gmail.com>
Date: Thu, 12 Apr 2012 03:21:00 GMT
Subject: Re: [PATCH 8/9 v11] difftool: teach difftool to handle directory diffs
Message-ID: <CAJDDKr5q=a+5D0SKUiCTuL=qCxNBoLWJc=_aBFWu2ZfD_0VUzw@mail.gmail.com>
In-Reply-To: <CAFouetgfPuiYO9xKPGQ1GGjr6JvMXM7et_7oYrWRrg+RMj9mCQ@mail.gmail.com>

On Wed, Apr 11, 2012 at 9:46 AM, Tim Henigan <tim.henigan@gmail.com> wrote:
> On Wed, Apr 11, 2012 at 5:02 AM, David Aguilar <davvid@gmail.com> wrote:
>> On Tue, Apr 10, 2012 at 10:24 AM, Tim Henigan <tim.henigan@gmail.com> wrote:
>>> On Mon, Apr 9, 2012 at 8:14 AM, David Aguilar <davvid@gmail.com> wrote:
>>>> On Wed, Apr 4, 2012 at 12:21 PM, Tim Henigan <tim.henigan@gmail.com> wrote:
>>
>> I think the right thing to do would be to not override GIT_DIR at all.
>>  I haven't read it deeply enough to know whether it was being
>> overridden for a specific reason, but I think it should be safe to
>> leave it as-is.
>>
>> Git.pm ends up overriding these variables itself anyways when calling commands.
>
> I tried to avoid setting $GIT_DIR in earlier versions of the patch.
> However as discussed here [1], either 'git update-index' or 'git
> checkout-index' did not work as expected without explicitly setting
> $GIT_DIR.
>
> If $GIT_DIR is not set, 'update-index' and 'checkout-index' will only
> work if 'difftool' is called from the repo root.  If 'difftool' is
> called from a subdirectory, then one of the commands fails.
>
> I suspect that when $GIT_INDEX_FILE is set but $GIT_DIR is not, then
> $GIT_DIR is assumed to be 'pwd'.  However, I was not able to prove
> that.
>
>
>> The GIT_WORK_TREE check should use $repo->wc_path().  Git.pm's already
>> done all the hard work ;-)
>
> I also tried this in an earlier version of the patch.  As noted here
> [2], I found that when 'difftool' was run from a subdirectory of the
> repo root, '$repo->wc_path()' returned the subdirectory rather than
> the repo root.
>
> Thinking about this again now, I realize it was a side-effect of
> $GIT_DIR being set in the script.  The man page for git config states
> that:
>
>    If --git-dir or GIT_DIR is specified but none of --work-tree, GIT_WORK_TREE
>    and core.worktree is specified, the current working directory is regarded as
>    the top level of your working tree.
>
> So, if I explicitly set $GIT_DIR just for the 'update-index' and
> 'checkout-index' commands, I need to unset it afterwards.  This should
> allow '$repo->wc_path()' to behave as expected.
>
> [1]: http://thread.gmane.org/gmane.comp.version-control.git/193296/focus=193302
> [2]: http://thread.gmane.org/gmane.comp.version-control.git/193601/focus=193603

That's right.  If we set it for one command we have to remember to
unset it afterwards else it won't work as expected.

If we're setting GIT_DIR then we should probably set GIT_WORK_TREE
too.  It seems like one way would be to call repo_path() and wc_path()
early, set the variables with the returned values, and then worry
about managing GIT_WORK_TREE before+after calling checkout-index.
That might work.

And in case the code needs it, there's a GIT_PREFIX variable that is
set when the current directory is a subdir beneath the repo root.
-- 
David