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

From: Brandon Williams <bmwill@google.com>
Date: Jan 18, 2017, 20:50 UTC
Message-ID: <20170118205026.GC10641@google.com>
In-Reply-To: <CAGZ79kZv2=dNt=TeJXbac4S20WcdOZo=iVa-b+4zkOoGVM1OFA@mail.gmail.com>

On 01/18, Stefan Beller wrote:

Show 13 quoted lines

> On Thu, Jan 12, 2017 at 3:53 PM, Brandon Williams <bmwill@google.com> 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.

Show 14 quoted lines

> 
> > +
> > +               /* 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