Re: [PATCH] unpack-trees: don't shift conflicts left and right
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jun 17, 2013, 20:44 UTC
- Message-ID
- <7vehc0bh79.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <51BF71D4.402@lsrfire.ath.cx>
René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
Show 15 quoted lines
>> The information is only useful for the unpack_trees callback, and
>> "info->data" is a more appropriate place to hang such a callback
>> specific data.
>>
>> Perhaps we should use info->data field to point at
>>
>> struct {
>> struct unpack_trees_options *o;
>> unsigned long df_conflict;
>> };
>>
>> and get rid of info->conflicts field?
>
> Here's a patch that does so, but it complicates matters quite a bit.
> Did I miss anything (or rather: add too much)?I do not think so. These bits are needed per recursion level, and it cannot be shoved into unpack_trees_options so I suspect that your patch is the best we can do. Or, perhaps we can
- add df_conflict to struct unpack_trees_options;
- have traverse_info->data point at struct unpack_trees_options as before; and
- save the old value of o->df_conflict on the stack of traverse_trees_recursive(), update the field in place, and restore it when the recursion returns???
Show 133 quoted lines
> ---
> tree-walk.h | 1 -
> unpack-trees.c | 38 +++++++++++++++++++++++++++++---------
> 2 files changed, 29 insertions(+), 10 deletions(-)
>
> diff --git a/tree-walk.h b/tree-walk.h
> index ae04b64..4876695 100644
> --- a/tree-walk.h
> +++ b/tree-walk.h
> @@ -46,7 +46,6 @@ struct traverse_info {
> int pathlen;
> struct pathspec *pathspec;
>
> - unsigned long df_conflicts;
> traverse_callback_t fn;
> void *data;
> int show_all_errors;
> diff --git a/unpack-trees.c b/unpack-trees.c
> index b27f2a6..1c1b4de 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -416,11 +416,17 @@ static int unpack_index_entry(struct cache_entry *ce,
> return ret;
> }
>
> +struct unpack_callback_info {
> + struct unpack_trees_options *options;
> + unsigned long df_conflicts;
> +};
> +
> static int find_cache_pos(struct traverse_info *, const struct name_entry *);
>
> static void restore_cache_bottom(struct traverse_info *info, int bottom)
> {
> - struct unpack_trees_options *o = info->data;
> + struct unpack_callback_info *uci = info->data;
> + struct unpack_trees_options *o = uci->options;
>
> if (o->diff_index_cached)
> return;
> @@ -429,7 +435,8 @@ static void restore_cache_bottom(struct traverse_info *info, int bottom)
>
> static int switch_cache_bottom(struct traverse_info *info)
> {
> - struct unpack_trees_options *o = info->data;
> + struct unpack_callback_info *uci = info->data;
> + struct unpack_trees_options *o = uci->options;
> int ret, pos;
>
> if (o->diff_index_cached)
> @@ -452,9 +459,14 @@ static int traverse_trees_recursive(int n, unsigned long dirmask,
> int i, ret, bottom;
> struct tree_desc t[MAX_UNPACK_TREES];
> void *buf[MAX_UNPACK_TREES];
> + struct unpack_callback_info *uci = info->data;
> + struct unpack_callback_info newuci;
> struct traverse_info newinfo;
> struct name_entry *p;
>
> + newuci = *uci;
> + newuci.df_conflicts |= df_conflicts;
> +
> p = names;
> while (!p->mode)
> p++;
> @@ -464,7 +476,7 @@ static int traverse_trees_recursive(int n, unsigned long dirmask,
> newinfo.pathspec = info->pathspec;
> newinfo.name = *p;
> newinfo.pathlen += tree_entry_len(p) + 1;
> - newinfo.df_conflicts |= df_conflicts;
> + newinfo.data = &newuci;
>
> for (i = 0; i < n; i++, dirmask >>= 1) {
> const unsigned char *sha1 = NULL;
> @@ -564,8 +576,9 @@ static int unpack_nondirectories(int n, unsigned long mask,
> const struct traverse_info *info)
> {
> int i;
> - struct unpack_trees_options *o = info->data;
> - unsigned long conflicts = info->df_conflicts | dirmask;
> + struct unpack_callback_info *uci = info->data;
> + struct unpack_trees_options *o = uci->options;
> + unsigned long conflicts = uci->df_conflicts | dirmask;
>
> /* Do we have *only* directories? Nothing to do */
> if (mask == dirmask && !src[0])
> @@ -644,7 +657,8 @@ static int find_cache_pos(struct traverse_info *info,
> const struct name_entry *p)
> {
> int pos;
> - struct unpack_trees_options *o = info->data;
> + struct unpack_callback_info *uci = info->data;
> + struct unpack_trees_options *o = uci->options;
> struct index_state *index = o->src_index;
> int pfxlen = info->pathlen;
> int p_len = tree_entry_len(p);
> @@ -699,7 +713,8 @@ static struct cache_entry *find_cache_entry(struct traverse_info *info,
> const struct name_entry *p)
> {
> int pos = find_cache_pos(info, p);
> - struct unpack_trees_options *o = info->data;
> + struct unpack_callback_info *uci = info->data;
> + struct unpack_trees_options *o = uci->options;
>
> if (0 <= pos)
> return o->src_index->cache[pos];
> @@ -742,7 +757,8 @@ static void debug_unpack_callback(int n,
> static int unpack_callback(int n, unsigned long mask, unsigned long dirmask, struct name_entry *names, struct traverse_info *info)
> {
> struct cache_entry *src[MAX_UNPACK_TREES + 1] = { NULL, };
> - struct unpack_trees_options *o = info->data;
> + struct unpack_callback_info *uci = info->data;
> + struct unpack_trees_options *o = uci->options;
> const struct name_entry *p = names;
>
> /* Find first entry with a real name (we could use "mask" too) */
> @@ -1054,11 +1070,15 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
>
> if (len) {
> const char *prefix = o->prefix ? o->prefix : "";
> + struct unpack_callback_info uci;
> struct traverse_info info;
>
> + memset(&uci, 0, sizeof(uci));
> + uci.options = o;
> +
> setup_traverse_info(&info, prefix);
> info.fn = unpack_callback;
> - info.data = o;
> + info.data = &uci;
> info.show_all_errors = o->show_all_errors;
> info.pathspec = o->pathspec;
>