On Wed, Mar 18, 2015 at 12:57 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Duy Nguyen <pclouds@xxxxxxxxx> writes:
>
>> On Mon, Mar 16, 2015 at 09:05:45AM -0700, Junio C Hamano wrote:
>>> The offending one came from eec3e7e4 (cache-tree: invalidate i-t-a
>>> paths after generating trees, 2012-12-16), which was a fix to an
>>> earlier bug where a cache-tree written out of an index with i-t-a
>>> entries had incorrect information and still claimed it is fully
>>> valid after write-tree rebuilt it. The test probably should add
>>> another path without i-t-a bit, run the same "diff --cached" with
>>> updated expectation before write-tre, and run the "diff --cached"
>>> again to make sure it produces a result that match the updated
>>> expectation.
>>
>> Would adding another non-i-t-a entry help? Before this patch
>> "diff --cached" after write-tree shows the i-t-a entry only when
>> eec3e7e4 is applied. But with this patch we don't show i-t-a entry any
>> more, before or after write-tree, eec3e7e4 makes no visible difference.
>>
>> We could even revert eec3e7e4 and the outcome of "diff --cached" would
>> be the same because we just sort of move the "invalidation" part from
>> cache-tree to do_oneway_diff(). Not invalidating would speed up "diff
>> --cached" when i-t-a entries are present. Still it may be a good idea
>> to invalidate i-t-a paths to be on the safe side. Perhaps a patch like
>> this to resurrect the test?
>
> My unerstanding of what eec3e7e4 (cache-tree: invalidate i-t-a paths
> after generating trees, 2012-12-16) fixed was that in this sequence:
>
> ...
>
> So reverting the fix obviously is not the right thing to do. If the
> tests show different results from two invocations of "diff --cached"
> with your patch applied, there is something that is broken by your
> patch, because the index and the HEAD does not change across
> write-tree in that test.
Right. I missed this but I think this is a less important test because
I added a new test to make sure "diff --cached" ("git status" to be
exact) outputs the right thing when i-t-a entries are present.
> If on the other hand the tests show the same result from these two
> "diff --cached" and the result is different from what the test
> expects, that means your patch changed the world order, i.e. an
> i-t-a entry used to be treated as if it were adding an empty blob to
> the index but it is now treated as non-existent, then that is a good
> thing and the only thing we need to update is what the test expects.
> I am guessing that instead of expecting dir/bar to be shown, it now
> should expect no output?
Yes, no output.
> Does adding an non-i-t-a entry help? It does not hurt, and it makes
> the test uses a non-empty output, making its effect more visible,
> which may or may not count as helping.
But still, if I revert the change in cache-tree.c and force write-tree
to produce valid cache-tree when i-t-a entries are present, this test
still passes incorrectly. This is why I changed the test.
--
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html