[PATCH] unpack-trees: don't shift conflicts left and right

[PATCH] unpack-trees: don't shift conflicts left and right

[René Scharfe]

If o->merge is set, the struct traverse_info member conflicts is shifted
left in unpack_callback, then passed through traverse_trees_recursive
to unpack_nondirectories, where it is shifted right before use.  Stop
the shifting and just pass the conflict bit mask as is.  Rename the
member to df_conflicts to prove that it isn't used anywhere else.

Signed-off-by: René Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 tree-walk.h    |  2 +-
 unpack-trees.c | 18 +++---------------
 2 files changed, 4 insertions(+), 16 deletions(-)

diff --git a/tree-walk.h b/tree-walk.h
index 2bf0db9..ae04b64 100644
--- a/tree-walk.h
+++ b/tree-walk.h
@@ -46,7 +46,7 @@ struct traverse_info {
 	int pathlen;
 	struct pathspec *pathspec;
 
-	unsigned long conflicts;
+	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 57b4074..b27f2a6 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -464,7 +464,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.conflicts |= df_conflicts;
+	newinfo.df_conflicts |= df_conflicts;
 
 	for (i = 0; i < n; i++, dirmask >>= 1) {
 		const unsigned char *sha1 = NULL;
@@ -565,17 +565,12 @@ static int unpack_nondirectories(int n, unsigned long mask,
 {
 	int i;
 	struct unpack_trees_options *o = info->data;
-	unsigned long conflicts;
+	unsigned long conflicts = info->df_conflicts | dirmask;
 
 	/* Do we have *only* directories? Nothing to do */
 	if (mask == dirmask && !src[0])
 		return 0;
 
-	conflicts = info->conflicts;
-	if (o->merge)
-		conflicts >>= 1;
-	conflicts |= dirmask;
-
 	/*
 	 * Ok, we've filled in up to any potential index entry in src[0],
 	 * now do the rest.
@@ -807,13 +802,6 @@ static int unpack_callback(int n, unsigned long mask, unsigned long dirmask, str
 
 	/* Now handle any directories.. */
 	if (dirmask) {
-		unsigned long conflicts = mask & ~dirmask;
-		if (o->merge) {
-			conflicts <<= 1;
-			if (src[0])
-				conflicts |= 1;
-		}
-
 		/* special case: "diff-index --cached" looking at a tree */
 		if (o->diff_index_cached &&
 		    n == 1 && dirmask == 1 && S_ISDIR(names->mode)) {
@@ -832,7 +820,7 @@ static int unpack_callback(int n, unsigned long mask, unsigned long dirmask, str
 			}
 		}
 
-		if (traverse_trees_recursive(n, dirmask, conflicts,
+		if (traverse_trees_recursive(n, dirmask, mask & ~dirmask,
 					     names, info) < 0)
 			return -1;
 		return mask;
-- 
1.8.3

Re: [PATCH] unpack-trees: don't shift conflicts left and right

[Junio C Hamano]

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:

> If o->merge is set, the struct traverse_info member conflicts is shifted
> left in unpack_callback, then passed through traverse_trees_recursive
> to unpack_nondirectories, where it is shifted right before use.  

> @@ -807,13 +802,6 @@ static int unpack_callback(int n, unsigned long mask, unsigned long dirmask, str
>  
>  	/* Now handle any directories.. */
>  	if (dirmask) {
> -		unsigned long conflicts = mask & ~dirmask;
> -		if (o->merge) {
> -			conflicts <<= 1;
> -			if (src[0])
> -				conflicts |= 1;
> -		}
> -
>  		/* special case: "diff-index --cached" looking at a tree */
>  		if (o->diff_index_cached &&
>  		    n == 1 && dirmask == 1 && S_ISDIR(names->mode)) {
> @@ -832,7 +820,7 @@ static int unpack_callback(int n, unsigned long mask, unsigned long dirmask, str
>  			}
>  		}
>  
> -		if (traverse_trees_recursive(n, dirmask, conflicts,
> +		if (traverse_trees_recursive(n, dirmask, mask & ~dirmask,
>  					     names, info) < 0)
>  			return -1;
>  		return mask;

This loses the bottom bit (i.e. are we merging and do have an index
entry?) passed to traverse_trees_recursive(), but when that bitmask
comes back to our callback, we immediately discard the bottom bit by
shifting before using it in unpack_nondirectories(), so this seems a
valid clean-up.

One thing renaming df_conficts to conflicts really proves is that
this field is not used by the traverse_trees machinery at all.

Before this patch, the bits in conflicts (now df_conflicts) mask had
the semantics that is not consistent with the dirmask/mask the
tree-walk machinery uses to communicate with its callers and
callbacks.  Everything in tree-walk.[ch] is about "walk N trees",
and bit 0 of dirmask and mask always means the first tree, not
"first tree, or in index if the callback is doing a merge", which
is used in the conflicts field before this patch.

I think the true source of the confusion is that the "conflicts"
field does not logically belong there.  It is not needed in the
general "walk N trees" machinery.

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?

Re: [PATCH] unpack-trees: don't shift conflicts left and right

[René Scharfe]

Am 16.06.2013 02:56, schrieb Junio C Hamano:
> One thing renaming df_conficts to conflicts really proves is that
> this field is not used by the traverse_trees machinery at all.
> 
> Before this patch, the bits in conflicts (now df_conflicts) mask had
> the semantics that is not consistent with the dirmask/mask the
> tree-walk machinery uses to communicate with its callers and
> callbacks.  Everything in tree-walk.[ch] is about "walk N trees",
> and bit 0 of dirmask and mask always means the first tree, not
> "first tree, or in index if the callback is doing a merge", which
> is used in the conflicts field before this patch.

Right.

> I think the true source of the confusion is that the "conflicts"
> field does not logically belong there.  It is not needed in the
> general "walk N trees" machinery.

NB: The only other caller of traverse_trees is git merge-tree.

> 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)?

---
 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;
 

Re: [PATCH] unpack-trees: don't shift conflicts left and right

[Junio C Hamano]

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:

>> 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???

> ---
>  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;
>  

Re: [PATCH] unpack-trees: don't shift conflicts left and right

[René Scharfe]

Am 17.06.2013 22:44, schrieb Junio C Hamano:
> René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
> 
>>> 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???

I'm not sure unpack_trees_options is the right place for that, but it
already has several members that aren't really "options".  It would
look something like this:


 tree-walk.h    | 1 -
 unpack-trees.c | 9 +++++++--
 unpack-trees.h | 1 +
 3 files changed, 8 insertions(+), 3 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..1c0ead0 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -454,6 +454,10 @@ static int traverse_trees_recursive(int n, unsigned long dirmask,
 	void *buf[MAX_UNPACK_TREES];
 	struct traverse_info newinfo;
 	struct name_entry *p;
+	struct unpack_trees_options *o = info->data;
+	unsigned long saved_df_conflicts = o->df_conflicts;
+
+	o->df_conflicts |= df_conflicts;
 
 	p = names;
 	while (!p->mode)
@@ -464,7 +468,6 @@ 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;
 
 	for (i = 0; i < n; i++, dirmask >>= 1) {
 		const unsigned char *sha1 = NULL;
@@ -480,6 +483,8 @@ static int traverse_trees_recursive(int n, unsigned long dirmask,
 	for (i = 0; i < n; i++)
 		free(buf[i]);
 
+	o->df_conflicts = saved_df_conflicts;
+
 	return ret;
 }
 
@@ -565,7 +570,7 @@ static int unpack_nondirectories(int n, unsigned long mask,
 {
 	int i;
 	struct unpack_trees_options *o = info->data;
-	unsigned long conflicts = info->df_conflicts | dirmask;
+	unsigned long conflicts = o->df_conflicts | dirmask;
 
 	/* Do we have *only* directories? Nothing to do */
 	if (mask == dirmask && !src[0])
diff --git a/unpack-trees.h b/unpack-trees.h
index 36a73a6..05ee968 100644
--- a/unpack-trees.h
+++ b/unpack-trees.h
@@ -66,6 +66,7 @@ struct unpack_trees_options {
 
 	struct cache_entry *df_conflict_entry;
 	void *unpack_data;
+	unsigned long df_conflicts;
 
 	struct index_state *dst_index;
 	struct index_state *src_index;

Re: [PATCH] unpack-trees: don't shift conflicts left and right

[René Scharfe]

Am 17.06.2013 22:44, schrieb Junio C Hamano:
> René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
> 
>>> 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???

How about going into the opposite direction and moving df_conflicts
handling more into traverse_tree?  If the function saved the mask
and dirmask in traverse_info then callbacks could calculate the
cumulated d/f conflicts by walking the info chain, similar to how
make_traverse_path works.  That would match the spirit of that
struct.  Below is a patch for that, for illustration.

We could then remove the mask and dirmask parameters from
traverse_callback_t functions, as the callbacks can then get them
through traverse_info.

René


 tree-walk.c    |  2 ++
 tree-walk.h    |  2 +-
 unpack-trees.c | 11 ++++++-----
 3 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/tree-walk.c b/tree-walk.c
index 6e30ef9..dae5db7 100644
--- a/tree-walk.c
+++ b/tree-walk.c
@@ -400,6 +400,8 @@ int traverse_trees(int n, struct tree_desc *t, struct traverse_info *info)
 		}
 		if (!mask)
 			break;
+		info->mask = mask;
+		info->dirmask = dirmask;
 		interesting = prune_traversal(e, info, &base, interesting);
 		if (interesting < 0)
 			break;
diff --git a/tree-walk.h b/tree-walk.h
index ae04b64..e308859 100644
--- a/tree-walk.h
+++ b/tree-walk.h
@@ -46,7 +46,7 @@ struct traverse_info {
 	int pathlen;
 	struct pathspec *pathspec;
 
-	unsigned long df_conflicts;
+	unsigned long mask, dirmask;
 	traverse_callback_t fn;
 	void *data;
 	int show_all_errors;
diff --git a/unpack-trees.c b/unpack-trees.c
index b27f2a6..58210d0 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -445,7 +445,6 @@ static int switch_cache_bottom(struct traverse_info *info)
 }
 
 static int traverse_trees_recursive(int n, unsigned long dirmask,
-				    unsigned long df_conflicts,
 				    struct name_entry *names,
 				    struct traverse_info *info)
 {
@@ -464,7 +463,6 @@ 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;
 
 	for (i = 0; i < n; i++, dirmask >>= 1) {
 		const unsigned char *sha1 = NULL;
@@ -565,12 +563,16 @@ static int unpack_nondirectories(int n, unsigned long mask,
 {
 	int i;
 	struct unpack_trees_options *o = info->data;
-	unsigned long conflicts = info->df_conflicts | dirmask;
+	unsigned long conflicts = dirmask;
+	const struct traverse_info *previnfo;
 
 	/* Do we have *only* directories? Nothing to do */
 	if (mask == dirmask && !src[0])
 		return 0;
 
+	for (previnfo = info->prev; previnfo; previnfo = previnfo->prev)
+		conflicts |= previnfo->mask & ~previnfo->dirmask;
+
 	/*
 	 * Ok, we've filled in up to any potential index entry in src[0],
 	 * now do the rest.
@@ -820,8 +822,7 @@ static int unpack_callback(int n, unsigned long mask, unsigned long dirmask, str
 			}
 		}
 
-		if (traverse_trees_recursive(n, dirmask, mask & ~dirmask,
-					     names, info) < 0)
+		if (traverse_trees_recursive(n, dirmask, names, info) < 0)
 			return -1;
 		return mask;
 	}

Re: [PATCH] unpack-trees: don't shift conflicts left and right

[Junio C Hamano]

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:

> Am 17.06.2013 22:44, schrieb Junio C Hamano:
>> 
>> ...  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???
>
> I'm not sure unpack_trees_options is the right place for that, but it
> already has several members that aren't really "options".

Yup, most notably the "df_conflict_entry" singleton sentinel.

> It would look something like this:

Hmm, that does not look too bad, actually.

>
>
>  tree-walk.h    | 1 -
>  unpack-trees.c | 9 +++++++--
>  unpack-trees.h | 1 +
>  3 files changed, 8 insertions(+), 3 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..1c0ead0 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -454,6 +454,10 @@ static int traverse_trees_recursive(int n, unsigned long dirmask,
>  	void *buf[MAX_UNPACK_TREES];
>  	struct traverse_info newinfo;
>  	struct name_entry *p;
> +	struct unpack_trees_options *o = info->data;
> +	unsigned long saved_df_conflicts = o->df_conflicts;
> +
> +	o->df_conflicts |= df_conflicts;
>  
>  	p = names;
>  	while (!p->mode)
> @@ -464,7 +468,6 @@ 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;
>  
>  	for (i = 0; i < n; i++, dirmask >>= 1) {
>  		const unsigned char *sha1 = NULL;
> @@ -480,6 +483,8 @@ static int traverse_trees_recursive(int n, unsigned long dirmask,
>  	for (i = 0; i < n; i++)
>  		free(buf[i]);
>  
> +	o->df_conflicts = saved_df_conflicts;
> +
>  	return ret;
>  }
>  
> @@ -565,7 +570,7 @@ static int unpack_nondirectories(int n, unsigned long mask,
>  {
>  	int i;
>  	struct unpack_trees_options *o = info->data;
> -	unsigned long conflicts = info->df_conflicts | dirmask;
> +	unsigned long conflicts = o->df_conflicts | dirmask;
>  
>  	/* Do we have *only* directories? Nothing to do */
>  	if (mask == dirmask && !src[0])
> diff --git a/unpack-trees.h b/unpack-trees.h
> index 36a73a6..05ee968 100644
> --- a/unpack-trees.h
> +++ b/unpack-trees.h
> @@ -66,6 +66,7 @@ struct unpack_trees_options {
>  
>  	struct cache_entry *df_conflict_entry;
>  	void *unpack_data;
> +	unsigned long df_conflicts;
>  
>  	struct index_state *dst_index;
>  	struct index_state *src_index;

Re: [PATCH] unpack-trees: don't shift conflicts left and right

[Junio C Hamano]

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:

> How about going into the opposite direction and moving df_conflicts
> handling more into traverse_tree?  If the function saved the mask
> and dirmask in traverse_info then callbacks could calculate the
> cumulated d/f conflicts by walking the info chain, similar to how
> make_traverse_path works.  That would match the spirit of that
> struct.  Below is a patch for that, for illustration.
>
> We could then remove the mask and dirmask parameters from
> traverse_callback_t functions, as the callbacks can then get them
> through traverse_info.

Hmph.

> @@ -565,12 +563,16 @@ static int unpack_nondirectories(int n, unsigned long mask,
>  {
>  	int i;
>  	struct unpack_trees_options *o = info->data;
> -	unsigned long conflicts = info->df_conflicts | dirmask;
> +	unsigned long conflicts = dirmask;

We grab the dirmask for the current level.

> +	const struct traverse_info *previnfo;
>  
>  	/* Do we have *only* directories? Nothing to do */
>  	if (mask == dirmask && !src[0])
>  		return 0;
>  
> +	for (previnfo = info->prev; previnfo; previnfo = previnfo->prev)
> +		conflicts |= previnfo->mask & ~previnfo->dirmask;

And OR-in the bits for non-directories in levels that are closer to
the root (i.e. if a path that corresponds to our parent directory is
a non-directory in one of the trees, our path cannot be a file in
that tree).

So the bit-math looks correct here.  conflicts ends up having bits
set for trees that cannot have a non-directory at the path we are
looking at.

But the need to go all the way up in the recursive callframes to get
the union of bitmask to get conflicts looks somewhat ugly.

I dunno.

Re: [PATCH] unpack-trees: don't shift conflicts left and right

[René Scharfe]

Am 18.06.2013 01:42, schrieb Junio C Hamano:
> But the need to go all the way up in the recursive callframes to get
> the union of bitmask to get conflicts looks somewhat ugly.

Yes, that's not pretty.  It's like make_traverse_path in that regard, 
though, so it fits in somehow.

I suspect the *real* solution is to put something like 
traverse_trees_recursive into tree-walk.c.  Both callers of 
traverse_tree recurse into subdirectories in slightly different ways. 
Providing that functionality in a central place should reduce code 
duplication.

René