Subject: Re: [PATCH 25/27] attr: store attribute stacks in hashmap

From: Brandon Williams <[email-masked]>
Date: Wed, 18 Jan 2017 12:50:26 -0800

On 01/18, Stefan Beller wrote:
> On Thu, Jan 12, 2017 at 3:53 PM, Brandon Williams <[email-masked]> wrote:
> > -static void prepare_attr_stack(const char *path, int dirlen)
> > +/*
> > + * This funciton should only be called from 'get_attr_stack()', which already
> 
> "function"
> 
> > +               /* system-wide frame */
> > +               if (git_attr_system()) {
> > +                       e = read_attr_from_file(git_etc_gitattributes(), 1);
> 
> read_attr_from_file may return NULL, so we'd have to treat this similar
> to below "root directory", i.e. xcalloc for an empty frame?

The push_stack function doesn't do anything if 'e' is NULL, so we should
be fine here.

> 
> > +
> > +               /* root directory */
> > +               if (!is_bare_repository() || direction == GIT_ATTR_INDEX) {
> > +                       e = read_attr(GITATTRIBUTES_FILE, 1);
> > +               } else {
> > +                       e = xcalloc(1, sizeof(struct attr_stack));
> > +               }
> > +               key = "";
> > +               push_stack(&core, e, key, strlen(key));
> 
> If this is a bare repo, could we just omit this frame instead of pushing
> an empty xcalloc'd frame? (Same for the stack frames of system wide
> and home dir) ?

The reasoning behind having the object created even if its a bare repo
is so that later we can easily see that a frame has been read and
included and doesn't need to attempt to reread the frame from disk
later.  It also made things simpler when storing the object in a hashmap
since storing a NULL ptr was awkward.

Though looking at Junio's discussion we may want to rethink how the
stacks are handled.  I still need to think about it some more.

-- 
Brandon Williams