Re: [PATCH v3] diff-no-index: exit(1) if 'diff --quiet <repo file> <external file>' finds changes

From: Tim Henigan <tim.henigan@gmail.com>
Date: Jun 20, 2012, 13:38 UTC
Message-ID: <CAFouetgXkqJPYwjr5ob5ed_ooL-D56zXyjnOAWrVPdt_eZqw7g@mail.gmail.com>
In-Reply-To: <CAFouetgRq1qkqJmThJJeu=Mdx9jS0c9dw7NPSwuJUOSpskCY2A@mail.gmail.com>

On Tue, Jun 19, 2012 at 12:47 PM, Tim Henigan <tim.henigan@gmail.com> wrote:

Show 32 quoted lines

> On Tue, Jun 19, 2012 at 9:58 AM, Jeff King <peff@peff.net> wrote:
>> On Tue, Jun 19, 2012 at 09:05:40AM -0400, Tim Henigan wrote:
>>
>>> As a side note, I found that these tests fail if a relative path is
>>> used for the file in 'non/git'.  In other words, this passes:
>>>
>>>     test_expect_code 0 git diff --quiet a
>>> "$TRASH_DIRECTORY/test-outside/non/git/matching-file"
>>>
>>> but this fails:
>>>
>>>     test_expect_code 0 git diff --quiet a ../non/git/matching-file
>>>
>>> This surprised me, but I have not investigated any further.
>>
>> The problem is that path_outside_repo in diff-no-index.c does not bother
>> handling relative paths at all, and just assumes they are inside the
>> repository. This is obviously not true if the path starts with "..", in
>> which case you would need to compare the number of ".." with the current
>> depth in the repository.
>>
>> prefix_path already does this (and is what generates the later
>> "../non/git/matching-file is not in the repository" message). We could
>> perhaps get rid of path_outside_repo and just re-use prefix_path's
>> logic, something like (not tested):
>
> With your patch applied, I was able to use relative paths in my tests.
>  I also confirmed that all the t4*.sh tests pass.
>
> For what its worth, your patch looks correct to me.  Existing
> consumers of 'prefix_path' should get the same results as before and
> the one added xmalloc is paired with a free.

Jeff,

Are you planning to send this patch to the list? If not, can I include it as 1 of 2 before my patch? If we go that route, I'm not sure how to properly show you as the author...

Also, in an earlier email [1] you mentioned that it may be a good idea to rename 'found_changes' to something like 'xdiff_found_changes'. I like the idea...I could submit this change as another patch in the series, if you have no objections.

Thanks again for your review and help.

[1]: http://thread.gmane.org/gmane.comp.version-control.git/200160/focus=200163