On Wed, Feb 05, 2014 at 09:36:55AM -0800, Junio C Hamano wrote:
> Kirill Smelkov <kirr@xxxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html