Re: [PATCH 1/3] name-hash: refactor polymorphic index_name_exists()

[Eric Sunshine <sunshine@sunshineco.com>]

On Fri, Sep 13, 2013 at 3:41 PM, Junio C Hamano <gitster@pobox.com> wrote:
> On Fri, Sep 13, 2013 at 12:29 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Fri, Sep 13, 2013 at 2:40 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Eric Sunshine <sunshine@sunshineco.com> writes:
>>>
>>>> Since these two modes of operations are disjoint and have no code in
>>>> common (one searches index_state.name_hash; the other dir_hash), they
>>>> can be represented more naturally as distinct functions: one to search
>>>> for a file, and one for a directory.
>>>
>>> Good thinking.  Thanks for working on this; I agree with the general
>>> direction this series is going.
>>>
>>> I however wonder if it would be a good idea to rename the other one
>>> to {cache|index}_file_exists(), and even keep the *_name_exists()
>>> variant that switches between the two based on the trailing slash,
>>> to avoid breaking other topics in flight that may have added new
>>> callsites to *_name_exists().  Otherwise the new callsites will feed
>>> a path with a trailing slash to *_name_exists() and get a false
>>> result, no?
>>
>> An assert("no trailing /") was added to index_name_exists() precisely
>> for the purpose of catching cases of code incorrectly calling
>> index_name_exists() to search for a directory. No existing code in
>> 'master' trips the assertion (at least according to the tests),
>> however, the assertion may be annoying for topics in flight which do.
>>
>> Other than plopping these patches atop 'pu' and seeing if anything
>> breaks, what would be the "git way" of detecting in-flight topics
>> which add callers of index_name_exists()? (Excuse my git ignorance.)
>
> I would do a quick
>
> $ git diff master..pu | grep '^+.*_name_exists'
>
> which would be more conservative (it would also show an invocation
> moved from one place to another).

There appear to be no topics in flight which add new
index_name_exists() callers (which is not to say that no new callers
will be introduced before this topic lands in 'master').

I also plopped the patches atop 'pu' and there are no new tests
failures on Linux or Mac OS X, so the series does not seem to break
anything in flight. (There are a few test failures on 'pu' on Mac OS
X, but they are not related to this series.)

Given the above. How should I proceed? Do you still feel that it is
advisable to keep an index_name_exists() around for compatibility
reasons in case any new callers are introduced? Regardless of that
answer, do you want index_name_exists() renamed to
index_file_exists()?