Re: [PATCH 7/8] combine-diff: Fast changed-to-all-parents paths scanning

From: Kirill Smelkov <kirr@navytux.spb.ru>
Date: Feb 5, 2014, 19:14 UTC
Message-ID: <20140205191427.GA3923@mini.zxlink>
In-Reply-To: <xmqqfvnx5umg.fsf@gitster.dls.corp.google.com>

On Wed, Feb 05, 2014 at 09:36:55AM -0800, Junio C Hamano wrote:

Show 47 quoted lines

> Kirill Smelkov <kirr@mns.spb.ru> writes:
> 
> > Only, before I clean things up, I'd like to ask - would the following
> > patch be accepted
> >
> > ---- 8< ---
> > diff --git a/tree-walk.c b/tree-walk.c
> > index 79dba1d..4dc86c7 100644
> > --- a/tree-walk.c
> > +++ b/tree-walk.c
> > @@ -37,7 +37,7 @@ static void decode_tree_entry(struct tree_desc *desc, const char *buf, unsigned
> >  
> >  	/* Initialize the descriptor entry */
> >  	desc->entry.path = path;
> > -	desc->entry.mode = mode;
> > +	desc->entry.mode = canon_mode(mode);
> >  	desc->entry.sha1 = (const unsigned char *)(path + len);
> >  }
> >  
> > diff --git a/tree-walk.h b/tree-walk.h
> > index ae04b64..ae7fb3a 100644
> > --- a/tree-walk.h
> > +++ b/tree-walk.h
> > @@ -16,7 +16,7 @@ struct tree_desc {
> >  static inline const unsigned char *tree_entry_extract(struct tree_desc *desc, const char **pathp, unsigned int *modep)
> >  {
> >  	*pathp = desc->entry.path;
> > -	*modep = canon_mode(desc->entry.mode);
> > +	*modep = desc->entry.mode;
> >  	return desc->entry.sha1;
> >  }
> > ---- 8< ---
> >  
> > ?
> 
> Doesn't desc point into and walks over the data we read from the
> tree object directly?
> 
> We try to keep (tree|commit)->buffer intact and that is done pretty
> deliberately.  While you are walking a tree or parsing a commit,
> somebody else, perhaps called indirectly by a helper function you
> call, may call lookup_object() for the same object, get the copy
> that has already been read and start using it.  This kind of change
> will introduce bugs that are hard to debug unless it is done very
> carefully (e.g. starting from making tree.buffer into a const pointer
> and propagating constness throughout the system), which might not be
> worth the pain.

I agree object data should be immutable for good. The only thing I'm talking about here is mode, which is parsed from a tree buffer and is stored in separate field:

        ---- 8< ---- tree-walk.h
        struct name_entry {
                const unsigned char *sha1;
                const char *path;
                unsigned int mode;              <---
        };

        struct tree_desc {
                const void *buffer;
                struct name_entry entry;
                unsigned int size;
        };

        ---- 8< ---- tree-walk.c
        const char *get_mode(const char *str, unsigned int *modep)
        { ... }     /* parses symbolic mode from tree buffer to uint */

        void decode_tree_entry(struct tree_desc *desc, const char *buf, unsigned long size)
        {
                const char *path;
                unsigned int mode, len;

                ...
                path = get_mode(buf, &mode);

                /* Initialize the descriptor entry */
                desc->entry.path = path;
                desc->entry.mode = mode;        <---
                desc->entry.sha1 = (const unsigned char *)(path + len);

so we are not talking about modifying object buffers here. All I'm asking is about canonicalizing _already_ parsed and copied mode on the fly.

Does that change anything?

Sorry, if I'm maybe misunderstanding something, and thanks, Kirill