Re: [PATCH v3 07/20] attr: remove an implicit dependency on the_index

[Brandon Williams <bmwill@google.com>]

On 06/06, Duy Nguyen wrote:
> On Wed, Jun 6, 2018 at 6:50 PM, Brandon Williams <bmwill@google.com> wrote:
> > On 06/06, Nguyễn Thái Ngọc Duy wrote:
> >> Make the attr API take an index_state instead of assuming the_index in
> >> attr code. All call sites are converted blindly to keep the patch
> >> simple and retain current behavior. Individual call sites may receive
> >> further updates to use the right index instead of the_index.
> >>
> >> There is one ugly temporary workaround added in attr.c that needs some
> >> more explanation.
> >>
> >> Commit c24f3abace (apply: file commited * with CRLF should roundtrip
> >> diff and apply - 2017-08-19) forces one convert_to_git() call to NOT
> >> read the index at all. But what do you know, we read it anyway by
> >> falling back to the_index. When "istate" from convert_to_git is now
> >> propagated down to read_attr_from_array() we will hit segfault
> >> somewhere inside read_blob_data_from_index.
> >>
> >> The right way of dealing with this is to kill "use_index" variable and
> >> only follow "istate" but at this stage we are not ready for that:
> >> while most git_attr_set_direction() calls just passes the_index to be
> >> assigned to use_index, unpack-trees passes a different one which is
> >> used by entry.c code, which has no way to know what index to use if we
> >> delete use_index. So this has to be done later.
> >
> > Ugh I remember this when I was doing some refactoring to the attr
> > subsystem a year or so ago.  I really wanted to get rid of the whole
> > "direction" thing because if I remember correctly its one of the only
> > remaining globals that affects the outcome of an attr check (everything
> > else was converted to use the attr check struct.  I always got way to
> > far into the weeds when trying to do that too.  I'm not expecting that
> > from this series (since its way out of scope) but maybe it'll be easier
> > afterwards.
> 
> It's not that much easier. This direction thing is still global by
> design. It would be great if we have something like Scheme's parameter
> (aka. dynamic scoping iirc) then we can still scope this nicely. Git
> does not live in such worlds.

Yeah i realized this after sending.  Either way thanks for simplifying
the global state by getting ride of the index global.

> -- 
> Duy

-- 
Brandon Williams