{"thread":{"id":"34159","subject":"[PATCH] unpack-trees: don't shift conflicts left and right","startedAt":"2013-06-15T23:44:43Z","lastAt":"2013-06-18T19:33:25Z","messageCount":9,"participants":["René Scharfe","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"220968","messageId":"1371339883-22775-1-git-send-email-rene.scharfe@lsrfire.ath.cx","threadId":"34159","inReplyTo":null,"subject":"[PATCH] unpack-trees: don't shift conflicts left and right","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2013-06-15T23:44:43Z","receivedAt":"2013-06-15T23:44:43Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"If o->merge is set, the struct traverse_info member conflicts is shifted\nleft in unpack_callback, then passed through traverse_trees_recursive\nto unpack_nondirectories, where it is shifted right before use.  Stop\nthe shifting and just pass the conflict bit mask as is.  Rename the\nmember to df_conflicts to prove that it isn't used anywhere else.\n\nSigned-off-by: René Scharfe <rene.scharfe@lsrfire.ath.cx>\n---\n tree-walk.h    |  2 +-\n unpack-trees.c | 18 +++---------------\n 2 files changed, 4 insertions(+), 16 deletions(-)\n\ndiff --git a/tree-walk.h b/tree-walk.h\nindex 2bf0db9..ae04b64 100644\n--- a/tree-walk.h\n+++ b/tree-walk.h\n@@ -46,7 +46,7 @@ struct traverse_info {\n \tint pathlen;\n \tstruct pathspec *pathspec;\n \n-\tunsigned long conflicts;\n+\tunsigned long df_conflicts;\n \ttraverse_callback_t fn;\n \tvoid *data;\n \tint show_all_errors;\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 57b4074..b27f2a6 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -464,7 +464,7 @@ static int traverse_trees_recursive(int n, unsigned long dirmask,\n \tnewinfo.pathspec = info->pathspec;\n \tnewinfo.name = *p;\n \tnewinfo.pathlen += tree_entry_len(p) + 1;\n-\tnewinfo.conflicts |= df_conflicts;\n+\tnewinfo.df_conflicts |= df_conflicts;\n \n \tfor (i = 0; i < n; i++, dirmask >>= 1) {\n \t\tconst unsigned char *sha1 = NULL;\n@@ -565,17 +565,12 @@ static int unpack_nondirectories(int n, unsigned long mask,\n {\n \tint i;\n \tstruct unpack_trees_options *o = info->data;\n-\tunsigned long conflicts;\n+\tunsigned long conflicts = info->df_conflicts | dirmask;\n \n \t/* Do we have *only* directories? Nothing to do */\n \tif (mask == dirmask && !src[0])\n \t\treturn 0;\n \n-\tconflicts = info->conflicts;\n-\tif (o->merge)\n-\t\tconflicts >>= 1;\n-\tconflicts |= dirmask;\n-\n \t/*\n \t * Ok, we've filled in up to any potential index entry in src[0],\n \t * now do the rest.\n@@ -807,13 +802,6 @@ static int unpack_callback(int n, unsigned long mask, unsigned long dirmask, str\n \n \t/* Now handle any directories.. */\n \tif (dirmask) {\n-\t\tunsigned long conflicts = mask & ~dirmask;\n-\t\tif (o->merge) {\n-\t\t\tconflicts <<= 1;\n-\t\t\tif (src[0])\n-\t\t\t\tconflicts |= 1;\n-\t\t}\n-\n \t\t/* special case: \"diff-index --cached\" looking at a tree */\n \t\tif (o->diff_index_cached &&\n \t\t    n == 1 && dirmask == 1 && S_ISDIR(names->mode)) {\n@@ -832,7 +820,7 @@ static int unpack_callback(int n, unsigned long mask, unsigned long dirmask, str\n \t\t\t}\n \t\t}\n \n-\t\tif (traverse_trees_recursive(n, dirmask, conflicts,\n+\t\tif (traverse_trees_recursive(n, dirmask, mask & ~dirmask,\n \t\t\t\t\t     names, info) < 0)\n \t\t\treturn -1;\n \t\treturn mask;\n-- \n1.8.3\n"},{"id":"220969","messageId":"7va9mqlvpu.fsf@alter.siamese.dyndns.org","threadId":"34159","inReplyTo":"1371339883-22775-1-git-send-email-rene.scharfe@lsrfire.ath.cx","subject":"Re: [PATCH] unpack-trees: don't shift conflicts left and right","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-16T00:56:13Z","receivedAt":"2013-06-16T00:56:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:\n\n> If o->merge is set, the struct traverse_info member conflicts is shifted\n> left in unpack_callback, then passed through traverse_trees_recursive\n> to unpack_nondirectories, where it is shifted right before use.  \n\n> @@ -807,13 +802,6 @@ static int unpack_callback(int n, unsigned long mask, unsigned long dirmask, str\n>  \n>  \t/* Now handle any directories.. */\n>  \tif (dirmask) {\n> -\t\tunsigned long conflicts = mask & ~dirmask;\n> -\t\tif (o->merge) {\n> -\t\t\tconflicts <<= 1;\n> -\t\t\tif (src[0])\n> -\t\t\t\tconflicts |= 1;\n> -\t\t}\n> -\n>  \t\t/* special case: \"diff-index --cached\" looking at a tree */\n>  \t\tif (o->diff_index_cached &&\n>  \t\t    n == 1 && dirmask == 1 && S_ISDIR(names->mode)) {\n> @@ -832,7 +820,7 @@ static int unpack_callback(int n, unsigned long mask, unsigned long dirmask, str\n>  \t\t\t}\n>  \t\t}\n>  \n> -\t\tif (traverse_trees_recursive(n, dirmask, conflicts,\n> +\t\tif (traverse_trees_recursive(n, dirmask, mask & ~dirmask,\n>  \t\t\t\t\t     names, info) < 0)\n>  \t\t\treturn -1;\n>  \t\treturn mask;\n\nThis loses the bottom bit (i.e. are we merging and do have an index\nentry?) passed to traverse_trees_recursive(), but when that bitmask\ncomes back to our callback, we immediately discard the bottom bit by\nshifting before using it in unpack_nondirectories(), so this seems a\nvalid clean-up.\n\nOne thing renaming df_conficts to conflicts really proves is that\nthis field is not used by the traverse_trees machinery at all.\n\nBefore this patch, the bits in conflicts (now df_conflicts) mask had\nthe semantics that is not consistent with the dirmask/mask the\ntree-walk machinery uses to communicate with its callers and\ncallbacks.  Everything in tree-walk.[ch] is about \"walk N trees\",\nand bit 0 of dirmask and mask always means the first tree, not\n\"first tree, or in index if the callback is doing a merge\", which\nis used in the conflicts field before this patch.\n\nI think the true source of the confusion is that the \"conflicts\"\nfield does not logically belong there.  It is not needed in the\ngeneral \"walk N trees\" machinery.\n\nThe information is only useful for the unpack_trees callback, and\n\"info->data\" is a more appropriate place to hang such a callback\nspecific data.\n\nPerhaps we should use info->data field to point at\n\n\tstruct {\n        \tstruct unpack_trees_options *o;\n                unsigned long df_conflict;\n\t};\n\nand get rid of info->conflicts field?\n"},{"id":"221101","messageId":"51BF71D4.402@lsrfire.ath.cx","threadId":"34159","inReplyTo":"7va9mqlvpu.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] unpack-trees: don't shift conflicts left and right","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2013-06-17T20:30:12Z","receivedAt":"2013-06-17T20:30:12Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 16.06.2013 02:56, schrieb Junio C Hamano:\n> One thing renaming df_conficts to conflicts really proves is that\n> this field is not used by the traverse_trees machinery at all.\n> \n> Before this patch, the bits in conflicts (now df_conflicts) mask had\n> the semantics that is not consistent with the dirmask/mask the\n> tree-walk machinery uses to communicate with its callers and\n> callbacks.  Everything in tree-walk.[ch] is about \"walk N trees\",\n> and bit 0 of dirmask and mask always means the first tree, not\n> \"first tree, or in index if the callback is doing a merge\", which\n> is used in the conflicts field before this patch.\n\nRight.\n\n> I think the true source of the confusion is that the \"conflicts\"\n> field does not logically belong there.  It is not needed in the\n> general \"walk N trees\" machinery.\n\nNB: The only other caller of traverse_trees is git merge-tree.\n\n> The information is only useful for the unpack_trees callback, and\n> \"info->data\" is a more appropriate place to hang such a callback\n> specific data.\n> \n> Perhaps we should use info->data field to point at\n> \n> \tstruct {\n>          \tstruct unpack_trees_options *o;\n>          \tunsigned long df_conflict;\n> \t};\n> \n> and get rid of info->conflicts field?\n\nHere's a patch that does so, but it complicates matters quite a bit.\nDid I miss anything (or rather: add too much)?\n\n---\n tree-walk.h    |  1 -\n unpack-trees.c | 38 +++++++++++++++++++++++++++++---------\n 2 files changed, 29 insertions(+), 10 deletions(-)\n\ndiff --git a/tree-walk.h b/tree-walk.h\nindex ae04b64..4876695 100644\n--- a/tree-walk.h\n+++ b/tree-walk.h\n@@ -46,7 +46,6 @@ struct traverse_info {\n \tint pathlen;\n \tstruct pathspec *pathspec;\n \n-\tunsigned long df_conflicts;\n \ttraverse_callback_t fn;\n \tvoid *data;\n \tint show_all_errors;\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex b27f2a6..1c1b4de 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -416,11 +416,17 @@ static int unpack_index_entry(struct cache_entry *ce,\n \treturn ret;\n }\n \n+struct unpack_callback_info {\n+\tstruct unpack_trees_options *options;\n+\tunsigned long df_conflicts;\n+};\n+\n static int find_cache_pos(struct traverse_info *, const struct name_entry *);\n \n static void restore_cache_bottom(struct traverse_info *info, int bottom)\n {\n-\tstruct unpack_trees_options *o = info->data;\n+\tstruct unpack_callback_info *uci = info->data;\n+\tstruct unpack_trees_options *o = uci->options;\n \n \tif (o->diff_index_cached)\n \t\treturn;\n@@ -429,7 +435,8 @@ static void restore_cache_bottom(struct traverse_info *info, int bottom)\n \n static int switch_cache_bottom(struct traverse_info *info)\n {\n-\tstruct unpack_trees_options *o = info->data;\n+\tstruct unpack_callback_info *uci = info->data;\n+\tstruct unpack_trees_options *o = uci->options;\n \tint ret, pos;\n \n \tif (o->diff_index_cached)\n@@ -452,9 +459,14 @@ static int traverse_trees_recursive(int n, unsigned long dirmask,\n \tint i, ret, bottom;\n \tstruct tree_desc t[MAX_UNPACK_TREES];\n \tvoid *buf[MAX_UNPACK_TREES];\n+\tstruct unpack_callback_info *uci = info->data;\n+\tstruct unpack_callback_info newuci;\n \tstruct traverse_info newinfo;\n \tstruct name_entry *p;\n \n+\tnewuci = *uci;\n+\tnewuci.df_conflicts |= df_conflicts;\n+\n \tp = names;\n \twhile (!p->mode)\n \t\tp++;\n@@ -464,7 +476,7 @@ static int traverse_trees_recursive(int n, unsigned long dirmask,\n \tnewinfo.pathspec = info->pathspec;\n \tnewinfo.name = *p;\n \tnewinfo.pathlen += tree_entry_len(p) + 1;\n-\tnewinfo.df_conflicts |= df_conflicts;\n+\tnewinfo.data = &newuci;\n \n \tfor (i = 0; i < n; i++, dirmask >>= 1) {\n \t\tconst unsigned char *sha1 = NULL;\n@@ -564,8 +576,9 @@ static int unpack_nondirectories(int n, unsigned long mask,\n \t\t\t\t const struct traverse_info *info)\n {\n \tint i;\n-\tstruct unpack_trees_options *o = info->data;\n-\tunsigned long conflicts = info->df_conflicts | dirmask;\n+\tstruct unpack_callback_info *uci = info->data;\n+\tstruct unpack_trees_options *o = uci->options;\n+\tunsigned long conflicts = uci->df_conflicts | dirmask;\n \n \t/* Do we have *only* directories? Nothing to do */\n \tif (mask == dirmask && !src[0])\n@@ -644,7 +657,8 @@ static int find_cache_pos(struct traverse_info *info,\n \t\t\t  const struct name_entry *p)\n {\n \tint pos;\n-\tstruct unpack_trees_options *o = info->data;\n+\tstruct unpack_callback_info *uci = info->data;\n+\tstruct unpack_trees_options *o = uci->options;\n \tstruct index_state *index = o->src_index;\n \tint pfxlen = info->pathlen;\n \tint p_len = tree_entry_len(p);\n@@ -699,7 +713,8 @@ static struct cache_entry *find_cache_entry(struct traverse_info *info,\n \t\t\t\t\t    const struct name_entry *p)\n {\n \tint pos = find_cache_pos(info, p);\n-\tstruct unpack_trees_options *o = info->data;\n+\tstruct unpack_callback_info *uci = info->data;\n+\tstruct unpack_trees_options *o = uci->options;\n \n \tif (0 <= pos)\n \t\treturn o->src_index->cache[pos];\n@@ -742,7 +757,8 @@ static void debug_unpack_callback(int n,\n static int unpack_callback(int n, unsigned long mask, unsigned long dirmask, struct name_entry *names, struct traverse_info *info)\n {\n \tstruct cache_entry *src[MAX_UNPACK_TREES + 1] = { NULL, };\n-\tstruct unpack_trees_options *o = info->data;\n+\tstruct unpack_callback_info *uci = info->data;\n+\tstruct unpack_trees_options *o = uci->options;\n \tconst struct name_entry *p = names;\n \n \t/* Find first entry with a real name (we could use \"mask\" too) */\n@@ -1054,11 +1070,15 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n \n \tif (len) {\n \t\tconst char *prefix = o->prefix ? o->prefix : \"\";\n+\t\tstruct unpack_callback_info uci;\n \t\tstruct traverse_info info;\n \n+\t\tmemset(&uci, 0, sizeof(uci));\n+\t\tuci.options = o;\n+\n \t\tsetup_traverse_info(&info, prefix);\n \t\tinfo.fn = unpack_callback;\n-\t\tinfo.data = o;\n+\t\tinfo.data = &uci;\n \t\tinfo.show_all_errors = o->show_all_errors;\n \t\tinfo.pathspec = o->pathspec;\n \n"},{"id":"221103","messageId":"7vehc0bh79.fsf@alter.siamese.dyndns.org","threadId":"34159","inReplyTo":"51BF71D4.402@lsrfire.ath.cx","subject":"Re: [PATCH] unpack-trees: don't shift conflicts left and right","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-17T20:44:26Z","receivedAt":"2013-06-17T20:44:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:\n\n>> The information is only useful for the unpack_trees callback, and\n>> \"info->data\" is a more appropriate place to hang such a callback\n>> specific data.\n>> \n>> Perhaps we should use info->data field to point at\n>> \n>> \tstruct {\n>>          \tstruct unpack_trees_options *o;\n>>          \tunsigned long df_conflict;\n>> \t};\n>> \n>> and get rid of info->conflicts field?\n>\n> Here's a patch that does so, but it complicates matters quite a bit.\n> Did I miss anything (or rather: add too much)?\n\nI do not think so.  These bits are needed per recursion level, and\nit cannot be shoved into unpack_trees_options so I suspect that your\npatch is the best we can do.  Or, perhaps we can\n\n - add df_conflict to struct unpack_trees_options;\n\n - have traverse_info->data point at struct unpack_trees_options as\n   before; and\n\n - save the old value of o->df_conflict on the stack of\n   traverse_trees_recursive(), update the field in place, and\n   restore it when the recursion returns???\n\n> ---\n>  tree-walk.h    |  1 -\n>  unpack-trees.c | 38 +++++++++++++++++++++++++++++---------\n>  2 files changed, 29 insertions(+), 10 deletions(-)\n>\n> diff --git a/tree-walk.h b/tree-walk.h\n> index ae04b64..4876695 100644\n> --- a/tree-walk.h\n> +++ b/tree-walk.h\n> @@ -46,7 +46,6 @@ struct traverse_info {\n>  \tint pathlen;\n>  \tstruct pathspec *pathspec;\n>  \n> -\tunsigned long df_conflicts;\n>  \ttraverse_callback_t fn;\n>  \tvoid *data;\n>  \tint show_all_errors;\n> diff --git a/unpack-trees.c b/unpack-trees.c\n> index b27f2a6..1c1b4de 100644\n> --- a/unpack-trees.c\n> +++ b/unpack-trees.c\n> @@ -416,11 +416,17 @@ static int unpack_index_entry(struct cache_entry *ce,\n>  \treturn ret;\n>  }\n>  \n> +struct unpack_callback_info {\n> +\tstruct unpack_trees_options *options;\n> +\tunsigned long df_conflicts;\n> +};\n> +\n>  static int find_cache_pos(struct traverse_info *, const struct name_entry *);\n>  \n>  static void restore_cache_bottom(struct traverse_info *info, int bottom)\n>  {\n> -\tstruct unpack_trees_options *o = info->data;\n> +\tstruct unpack_callback_info *uci = info->data;\n> +\tstruct unpack_trees_options *o = uci->options;\n>  \n>  \tif (o->diff_index_cached)\n>  \t\treturn;\n> @@ -429,7 +435,8 @@ static void restore_cache_bottom(struct traverse_info *info, int bottom)\n>  \n>  static int switch_cache_bottom(struct traverse_info *info)\n>  {\n> -\tstruct unpack_trees_options *o = info->data;\n> +\tstruct unpack_callback_info *uci = info->data;\n> +\tstruct unpack_trees_options *o = uci->options;\n>  \tint ret, pos;\n>  \n>  \tif (o->diff_index_cached)\n> @@ -452,9 +459,14 @@ static int traverse_trees_recursive(int n, unsigned long dirmask,\n>  \tint i, ret, bottom;\n>  \tstruct tree_desc t[MAX_UNPACK_TREES];\n>  \tvoid *buf[MAX_UNPACK_TREES];\n> +\tstruct unpack_callback_info *uci = info->data;\n> +\tstruct unpack_callback_info newuci;\n>  \tstruct traverse_info newinfo;\n>  \tstruct name_entry *p;\n>  \n> +\tnewuci = *uci;\n> +\tnewuci.df_conflicts |= df_conflicts;\n> +\n>  \tp = names;\n>  \twhile (!p->mode)\n>  \t\tp++;\n> @@ -464,7 +476,7 @@ static int traverse_trees_recursive(int n, unsigned long dirmask,\n>  \tnewinfo.pathspec = info->pathspec;\n>  \tnewinfo.name = *p;\n>  \tnewinfo.pathlen += tree_entry_len(p) + 1;\n> -\tnewinfo.df_conflicts |= df_conflicts;\n> +\tnewinfo.data = &newuci;\n>  \n>  \tfor (i = 0; i < n; i++, dirmask >>= 1) {\n>  \t\tconst unsigned char *sha1 = NULL;\n> @@ -564,8 +576,9 @@ static int unpack_nondirectories(int n, unsigned long mask,\n>  \t\t\t\t const struct traverse_info *info)\n>  {\n>  \tint i;\n> -\tstruct unpack_trees_options *o = info->data;\n> -\tunsigned long conflicts = info->df_conflicts | dirmask;\n> +\tstruct unpack_callback_info *uci = info->data;\n> +\tstruct unpack_trees_options *o = uci->options;\n> +\tunsigned long conflicts = uci->df_conflicts | dirmask;\n>  \n>  \t/* Do we have *only* directories? Nothing to do */\n>  \tif (mask == dirmask && !src[0])\n> @@ -644,7 +657,8 @@ static int find_cache_pos(struct traverse_info *info,\n>  \t\t\t  const struct name_entry *p)\n>  {\n>  \tint pos;\n> -\tstruct unpack_trees_options *o = info->data;\n> +\tstruct unpack_callback_info *uci = info->data;\n> +\tstruct unpack_trees_options *o = uci->options;\n>  \tstruct index_state *index = o->src_index;\n>  \tint pfxlen = info->pathlen;\n>  \tint p_len = tree_entry_len(p);\n> @@ -699,7 +713,8 @@ static struct cache_entry *find_cache_entry(struct traverse_info *info,\n>  \t\t\t\t\t    const struct name_entry *p)\n>  {\n>  \tint pos = find_cache_pos(info, p);\n> -\tstruct unpack_trees_options *o = info->data;\n> +\tstruct unpack_callback_info *uci = info->data;\n> +\tstruct unpack_trees_options *o = uci->options;\n>  \n>  \tif (0 <= pos)\n>  \t\treturn o->src_index->cache[pos];\n> @@ -742,7 +757,8 @@ static void debug_unpack_callback(int n,\n>  static int unpack_callback(int n, unsigned long mask, unsigned long dirmask, struct name_entry *names, struct traverse_info *info)\n>  {\n>  \tstruct cache_entry *src[MAX_UNPACK_TREES + 1] = { NULL, };\n> -\tstruct unpack_trees_options *o = info->data;\n> +\tstruct unpack_callback_info *uci = info->data;\n> +\tstruct unpack_trees_options *o = uci->options;\n>  \tconst struct name_entry *p = names;\n>  \n>  \t/* Find first entry with a real name (we could use \"mask\" too) */\n> @@ -1054,11 +1070,15 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n>  \n>  \tif (len) {\n>  \t\tconst char *prefix = o->prefix ? o->prefix : \"\";\n> +\t\tstruct unpack_callback_info uci;\n>  \t\tstruct traverse_info info;\n>  \n> +\t\tmemset(&uci, 0, sizeof(uci));\n> +\t\tuci.options = o;\n> +\n>  \t\tsetup_traverse_info(&info, prefix);\n>  \t\tinfo.fn = unpack_callback;\n> -\t\tinfo.data = o;\n> +\t\tinfo.data = &uci;\n>  \t\tinfo.show_all_errors = o->show_all_errors;\n>  \t\tinfo.pathspec = o->pathspec;\n>  \n"},{"id":"221108","messageId":"51BF8DC7.4020008@lsrfire.ath.cx","threadId":"34159","inReplyTo":"7vehc0bh79.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] unpack-trees: don't shift conflicts left and right","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2013-06-17T22:29:27Z","receivedAt":"2013-06-17T22:29:27Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 17.06.2013 22:44, schrieb Junio C Hamano:\n> René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:\n> \n>>> The information is only useful for the unpack_trees callback, and\n>>> \"info->data\" is a more appropriate place to hang such a callback\n>>> specific data.\n>>>\n>>> Perhaps we should use info->data field to point at\n>>>\n>>> \tstruct {\n>>>           \tstruct unpack_trees_options *o;\n>>>           \tunsigned long df_conflict;\n>>> \t};\n>>>\n>>> and get rid of info->conflicts field?\n>>\n>> Here's a patch that does so, but it complicates matters quite a bit.\n>> Did I miss anything (or rather: add too much)?\n> \n> I do not think so.  These bits are needed per recursion level, and\n> it cannot be shoved into unpack_trees_options so I suspect that your\n> patch is the best we can do.  Or, perhaps we can\n> \n>   - add df_conflict to struct unpack_trees_options;\n> \n>   - have traverse_info->data point at struct unpack_trees_options as\n>     before; and\n> \n>   - save the old value of o->df_conflict on the stack of\n>     traverse_trees_recursive(), update the field in place, and\n>     restore it when the recursion returns???\n\nI'm not sure unpack_trees_options is the right place for that, but it\nalready has several members that aren't really \"options\".  It would\nlook something like this:\n\n\n tree-walk.h    | 1 -\n unpack-trees.c | 9 +++++++--\n unpack-trees.h | 1 +\n 3 files changed, 8 insertions(+), 3 deletions(-)\n\ndiff --git a/tree-walk.h b/tree-walk.h\nindex ae04b64..4876695 100644\n--- a/tree-walk.h\n+++ b/tree-walk.h\n@@ -46,7 +46,6 @@ struct traverse_info {\n \tint pathlen;\n \tstruct pathspec *pathspec;\n \n-\tunsigned long df_conflicts;\n \ttraverse_callback_t fn;\n \tvoid *data;\n \tint show_all_errors;\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex b27f2a6..1c0ead0 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -454,6 +454,10 @@ static int traverse_trees_recursive(int n, unsigned long dirmask,\n \tvoid *buf[MAX_UNPACK_TREES];\n \tstruct traverse_info newinfo;\n \tstruct name_entry *p;\n+\tstruct unpack_trees_options *o = info->data;\n+\tunsigned long saved_df_conflicts = o->df_conflicts;\n+\n+\to->df_conflicts |= df_conflicts;\n \n \tp = names;\n \twhile (!p->mode)\n@@ -464,7 +468,6 @@ static int traverse_trees_recursive(int n, unsigned long dirmask,\n \tnewinfo.pathspec = info->pathspec;\n \tnewinfo.name = *p;\n \tnewinfo.pathlen += tree_entry_len(p) + 1;\n-\tnewinfo.df_conflicts |= df_conflicts;\n \n \tfor (i = 0; i < n; i++, dirmask >>= 1) {\n \t\tconst unsigned char *sha1 = NULL;\n@@ -480,6 +483,8 @@ static int traverse_trees_recursive(int n, unsigned long dirmask,\n \tfor (i = 0; i < n; i++)\n \t\tfree(buf[i]);\n \n+\to->df_conflicts = saved_df_conflicts;\n+\n \treturn ret;\n }\n \n@@ -565,7 +570,7 @@ static int unpack_nondirectories(int n, unsigned long mask,\n {\n \tint i;\n \tstruct unpack_trees_options *o = info->data;\n-\tunsigned long conflicts = info->df_conflicts | dirmask;\n+\tunsigned long conflicts = o->df_conflicts | dirmask;\n \n \t/* Do we have *only* directories? Nothing to do */\n \tif (mask == dirmask && !src[0])\ndiff --git a/unpack-trees.h b/unpack-trees.h\nindex 36a73a6..05ee968 100644\n--- a/unpack-trees.h\n+++ b/unpack-trees.h\n@@ -66,6 +66,7 @@ struct unpack_trees_options {\n \n \tstruct cache_entry *df_conflict_entry;\n \tvoid *unpack_data;\n+\tunsigned long df_conflicts;\n \n \tstruct index_state *dst_index;\n \tstruct index_state *src_index;\n"},{"id":"221112","messageId":"51BF9B35.3010801@lsrfire.ath.cx","threadId":"34159","inReplyTo":"7vehc0bh79.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] unpack-trees: don't shift conflicts left and right","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2013-06-17T23:26:45Z","receivedAt":"2013-06-17T23:26:45Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 17.06.2013 22:44, schrieb Junio C Hamano:\n> René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:\n> \n>>> The information is only useful for the unpack_trees callback, and\n>>> \"info->data\" is a more appropriate place to hang such a callback\n>>> specific data.\n>>>\n>>> Perhaps we should use info->data field to point at\n>>>\n>>> \tstruct {\n>>>           \tstruct unpack_trees_options *o;\n>>>           \tunsigned long df_conflict;\n>>> \t};\n>>>\n>>> and get rid of info->conflicts field?\n>>\n>> Here's a patch that does so, but it complicates matters quite a bit.\n>> Did I miss anything (or rather: add too much)?\n> \n> I do not think so.  These bits are needed per recursion level, and\n> it cannot be shoved into unpack_trees_options so I suspect that your\n> patch is the best we can do.  Or, perhaps we can\n> \n>   - add df_conflict to struct unpack_trees_options;\n> \n>   - have traverse_info->data point at struct unpack_trees_options as\n>     before; and\n> \n>   - save the old value of o->df_conflict on the stack of\n>     traverse_trees_recursive(), update the field in place, and\n>     restore it when the recursion returns???\n\nHow about going into the opposite direction and moving df_conflicts\nhandling more into traverse_tree?  If the function saved the mask\nand dirmask in traverse_info then callbacks could calculate the\ncumulated d/f conflicts by walking the info chain, similar to how\nmake_traverse_path works.  That would match the spirit of that\nstruct.  Below is a patch for that, for illustration.\n\nWe could then remove the mask and dirmask parameters from\ntraverse_callback_t functions, as the callbacks can then get them\nthrough traverse_info.\n\nRené\n\n\n tree-walk.c    |  2 ++\n tree-walk.h    |  2 +-\n unpack-trees.c | 11 ++++++-----\n 3 files changed, 9 insertions(+), 6 deletions(-)\n\ndiff --git a/tree-walk.c b/tree-walk.c\nindex 6e30ef9..dae5db7 100644\n--- a/tree-walk.c\n+++ b/tree-walk.c\n@@ -400,6 +400,8 @@ int traverse_trees(int n, struct tree_desc *t, struct traverse_info *info)\n \t\t}\n \t\tif (!mask)\n \t\t\tbreak;\n+\t\tinfo->mask = mask;\n+\t\tinfo->dirmask = dirmask;\n \t\tinteresting = prune_traversal(e, info, &base, interesting);\n \t\tif (interesting < 0)\n \t\t\tbreak;\ndiff --git a/tree-walk.h b/tree-walk.h\nindex ae04b64..e308859 100644\n--- a/tree-walk.h\n+++ b/tree-walk.h\n@@ -46,7 +46,7 @@ struct traverse_info {\n \tint pathlen;\n \tstruct pathspec *pathspec;\n \n-\tunsigned long df_conflicts;\n+\tunsigned long mask, dirmask;\n \ttraverse_callback_t fn;\n \tvoid *data;\n \tint show_all_errors;\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex b27f2a6..58210d0 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -445,7 +445,6 @@ static int switch_cache_bottom(struct traverse_info *info)\n }\n \n static int traverse_trees_recursive(int n, unsigned long dirmask,\n-\t\t\t\t    unsigned long df_conflicts,\n \t\t\t\t    struct name_entry *names,\n \t\t\t\t    struct traverse_info *info)\n {\n@@ -464,7 +463,6 @@ static int traverse_trees_recursive(int n, unsigned long dirmask,\n \tnewinfo.pathspec = info->pathspec;\n \tnewinfo.name = *p;\n \tnewinfo.pathlen += tree_entry_len(p) + 1;\n-\tnewinfo.df_conflicts |= df_conflicts;\n \n \tfor (i = 0; i < n; i++, dirmask >>= 1) {\n \t\tconst unsigned char *sha1 = NULL;\n@@ -565,12 +563,16 @@ static int unpack_nondirectories(int n, unsigned long mask,\n {\n \tint i;\n \tstruct unpack_trees_options *o = info->data;\n-\tunsigned long conflicts = info->df_conflicts | dirmask;\n+\tunsigned long conflicts = dirmask;\n+\tconst struct traverse_info *previnfo;\n \n \t/* Do we have *only* directories? Nothing to do */\n \tif (mask == dirmask && !src[0])\n \t\treturn 0;\n \n+\tfor (previnfo = info->prev; previnfo; previnfo = previnfo->prev)\n+\t\tconflicts |= previnfo->mask & ~previnfo->dirmask;\n+\n \t/*\n \t * Ok, we've filled in up to any potential index entry in src[0],\n \t * now do the rest.\n@@ -820,8 +822,7 @@ static int unpack_callback(int n, unsigned long mask, unsigned long dirmask, str\n \t\t\t}\n \t\t}\n \n-\t\tif (traverse_trees_recursive(n, dirmask, mask & ~dirmask,\n-\t\t\t\t\t     names, info) < 0)\n+\t\tif (traverse_trees_recursive(n, dirmask, names, info) < 0)\n \t\t\treturn -1;\n \t\treturn mask;\n \t}\n"},{"id":"221114","messageId":"7vy5a89v0e.fsf@alter.siamese.dyndns.org","threadId":"34159","inReplyTo":"51BF8DC7.4020008@lsrfire.ath.cx","subject":"Re: [PATCH] unpack-trees: don't shift conflicts left and right","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-17T23:29:05Z","receivedAt":"2013-06-17T23:29:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:\n\n> Am 17.06.2013 22:44, schrieb Junio C Hamano:\n>> \n>> ...  Or, perhaps we can\n>> \n>>   - add df_conflict to struct unpack_trees_options;\n>> \n>>   - have traverse_info->data point at struct unpack_trees_options as\n>>     before; and\n>> \n>>   - save the old value of o->df_conflict on the stack of\n>>     traverse_trees_recursive(), update the field in place, and\n>>     restore it when the recursion returns???\n>\n> I'm not sure unpack_trees_options is the right place for that, but it\n> already has several members that aren't really \"options\".\n\nYup, most notably the \"df_conflict_entry\" singleton sentinel.\n\n> It would look something like this:\n\nHmm, that does not look too bad, actually.\n\n>\n>\n>  tree-walk.h    | 1 -\n>  unpack-trees.c | 9 +++++++--\n>  unpack-trees.h | 1 +\n>  3 files changed, 8 insertions(+), 3 deletions(-)\n>\n> diff --git a/tree-walk.h b/tree-walk.h\n> index ae04b64..4876695 100644\n> --- a/tree-walk.h\n> +++ b/tree-walk.h\n> @@ -46,7 +46,6 @@ struct traverse_info {\n>  \tint pathlen;\n>  \tstruct pathspec *pathspec;\n>  \n> -\tunsigned long df_conflicts;\n>  \ttraverse_callback_t fn;\n>  \tvoid *data;\n>  \tint show_all_errors;\n> diff --git a/unpack-trees.c b/unpack-trees.c\n> index b27f2a6..1c0ead0 100644\n> --- a/unpack-trees.c\n> +++ b/unpack-trees.c\n> @@ -454,6 +454,10 @@ static int traverse_trees_recursive(int n, unsigned long dirmask,\n>  \tvoid *buf[MAX_UNPACK_TREES];\n>  \tstruct traverse_info newinfo;\n>  \tstruct name_entry *p;\n> +\tstruct unpack_trees_options *o = info->data;\n> +\tunsigned long saved_df_conflicts = o->df_conflicts;\n> +\n> +\to->df_conflicts |= df_conflicts;\n>  \n>  \tp = names;\n>  \twhile (!p->mode)\n> @@ -464,7 +468,6 @@ static int traverse_trees_recursive(int n, unsigned long dirmask,\n>  \tnewinfo.pathspec = info->pathspec;\n>  \tnewinfo.name = *p;\n>  \tnewinfo.pathlen += tree_entry_len(p) + 1;\n> -\tnewinfo.df_conflicts |= df_conflicts;\n>  \n>  \tfor (i = 0; i < n; i++, dirmask >>= 1) {\n>  \t\tconst unsigned char *sha1 = NULL;\n> @@ -480,6 +483,8 @@ static int traverse_trees_recursive(int n, unsigned long dirmask,\n>  \tfor (i = 0; i < n; i++)\n>  \t\tfree(buf[i]);\n>  \n> +\to->df_conflicts = saved_df_conflicts;\n> +\n>  \treturn ret;\n>  }\n>  \n> @@ -565,7 +570,7 @@ static int unpack_nondirectories(int n, unsigned long mask,\n>  {\n>  \tint i;\n>  \tstruct unpack_trees_options *o = info->data;\n> -\tunsigned long conflicts = info->df_conflicts | dirmask;\n> +\tunsigned long conflicts = o->df_conflicts | dirmask;\n>  \n>  \t/* Do we have *only* directories? Nothing to do */\n>  \tif (mask == dirmask && !src[0])\n> diff --git a/unpack-trees.h b/unpack-trees.h\n> index 36a73a6..05ee968 100644\n> --- a/unpack-trees.h\n> +++ b/unpack-trees.h\n> @@ -66,6 +66,7 @@ struct unpack_trees_options {\n>  \n>  \tstruct cache_entry *df_conflict_entry;\n>  \tvoid *unpack_data;\n> +\tunsigned long df_conflicts;\n>  \n>  \tstruct index_state *dst_index;\n>  \tstruct index_state *src_index;\n"},{"id":"221115","messageId":"7vsj0g9udh.fsf@alter.siamese.dyndns.org","threadId":"34159","inReplyTo":"51BF9B35.3010801@lsrfire.ath.cx","subject":"Re: [PATCH] unpack-trees: don't shift conflicts left and right","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-17T23:42:50Z","receivedAt":"2013-06-17T23:42:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:\n\n> How about going into the opposite direction and moving df_conflicts\n> handling more into traverse_tree?  If the function saved the mask\n> and dirmask in traverse_info then callbacks could calculate the\n> cumulated d/f conflicts by walking the info chain, similar to how\n> make_traverse_path works.  That would match the spirit of that\n> struct.  Below is a patch for that, for illustration.\n>\n> We could then remove the mask and dirmask parameters from\n> traverse_callback_t functions, as the callbacks can then get them\n> through traverse_info.\n\nHmph.\n\n> @@ -565,12 +563,16 @@ static int unpack_nondirectories(int n, unsigned long mask,\n>  {\n>  \tint i;\n>  \tstruct unpack_trees_options *o = info->data;\n> -\tunsigned long conflicts = info->df_conflicts | dirmask;\n> +\tunsigned long conflicts = dirmask;\n\nWe grab the dirmask for the current level.\n\n> +\tconst struct traverse_info *previnfo;\n>  \n>  \t/* Do we have *only* directories? Nothing to do */\n>  \tif (mask == dirmask && !src[0])\n>  \t\treturn 0;\n>  \n> +\tfor (previnfo = info->prev; previnfo; previnfo = previnfo->prev)\n> +\t\tconflicts |= previnfo->mask & ~previnfo->dirmask;\n\nAnd OR-in the bits for non-directories in levels that are closer to\nthe root (i.e. if a path that corresponds to our parent directory is\na non-directory in one of the trees, our path cannot be a file in\nthat tree).\n\nSo the bit-math looks correct here.  conflicts ends up having bits\nset for trees that cannot have a non-directory at the path we are\nlooking at.\n\nBut the need to go all the way up in the recursive callframes to get\nthe union of bitmask to get conflicts looks somewhat ugly.\n\nI dunno.\n"},{"id":"221295","messageId":"51C0B605.10908@lsrfire.ath.cx","threadId":"34159","inReplyTo":"7vsj0g9udh.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] unpack-trees: don't shift conflicts left and right","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2013-06-18T19:33:25Z","receivedAt":"2013-06-18T19:33:25Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 18.06.2013 01:42, schrieb Junio C Hamano:\n> But the need to go all the way up in the recursive callframes to get\n> the union of bitmask to get conflicts looks somewhat ugly.\n\nYes, that's not pretty.  It's like make_traverse_path in that regard, \nthough, so it fits in somehow.\n\nI suspect the *real* solution is to put something like \ntraverse_trees_recursive into tree-walk.c.  Both callers of \ntraverse_tree recurse into subdirectories in slightly different ways. \nProviding that functionality in a central place should reduce code \nduplication.\n\nRené\n"}]}