* [PATCH] unpack-trees: don't shift conflicts left and right @ 2013-06-15 23:44 René Scharfe 2013-06-16 0:56 ` Junio C Hamano 0 siblings, 1 reply; 9+ messages in thread From: René Scharfe @ 2013-06-15 23:44 UTC (permalink / raw) To: git; +Cc: Junio C Hamano 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] unpack-trees: don't shift conflicts left and right 2013-06-15 23:44 [PATCH] unpack-trees: don't shift conflicts left and right René Scharfe @ 2013-06-16 0:56 ` Junio C Hamano 2013-06-17 20:30 ` René Scharfe 0 siblings, 1 reply; 9+ messages in thread From: Junio C Hamano @ 2013-06-16 0:56 UTC (permalink / raw) To: René Scharfe; +Cc: git 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? ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] unpack-trees: don't shift conflicts left and right 2013-06-16 0:56 ` Junio C Hamano @ 2013-06-17 20:30 ` René Scharfe 2013-06-17 20:44 ` Junio C Hamano 0 siblings, 1 reply; 9+ messages in thread From: René Scharfe @ 2013-06-17 20:30 UTC (permalink / raw) To: Junio C Hamano; +Cc: git 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; ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] unpack-trees: don't shift conflicts left and right 2013-06-17 20:30 ` René Scharfe @ 2013-06-17 20:44 ` Junio C Hamano 2013-06-17 22:29 ` René Scharfe 2013-06-17 23:26 ` René Scharfe 0 siblings, 2 replies; 9+ messages in thread From: Junio C Hamano @ 2013-06-17 20:44 UTC (permalink / raw) To: René Scharfe; +Cc: git 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; > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] unpack-trees: don't shift conflicts left and right 2013-06-17 20:44 ` Junio C Hamano @ 2013-06-17 22:29 ` René Scharfe 2013-06-17 23:29 ` Junio C Hamano 2013-06-17 23:26 ` René Scharfe 1 sibling, 1 reply; 9+ messages in thread From: René Scharfe @ 2013-06-17 22:29 UTC (permalink / raw) To: Junio C Hamano; +Cc: git 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; ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] unpack-trees: don't shift conflicts left and right 2013-06-17 22:29 ` René Scharfe @ 2013-06-17 23:29 ` Junio C Hamano 0 siblings, 0 replies; 9+ messages in thread From: Junio C Hamano @ 2013-06-17 23:29 UTC (permalink / raw) To: René Scharfe; +Cc: git 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; ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] unpack-trees: don't shift conflicts left and right 2013-06-17 20:44 ` Junio C Hamano 2013-06-17 22:29 ` René Scharfe @ 2013-06-17 23:26 ` René Scharfe 2013-06-17 23:42 ` Junio C Hamano 1 sibling, 1 reply; 9+ messages in thread From: René Scharfe @ 2013-06-17 23:26 UTC (permalink / raw) To: Junio C Hamano; +Cc: git 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; } ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] unpack-trees: don't shift conflicts left and right 2013-06-17 23:26 ` René Scharfe @ 2013-06-17 23:42 ` Junio C Hamano 2013-06-18 19:33 ` René Scharfe 0 siblings, 1 reply; 9+ messages in thread From: Junio C Hamano @ 2013-06-17 23:42 UTC (permalink / raw) To: René Scharfe; +Cc: git 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] unpack-trees: don't shift conflicts left and right 2013-06-17 23:42 ` Junio C Hamano @ 2013-06-18 19:33 ` René Scharfe 0 siblings, 0 replies; 9+ messages in thread From: René Scharfe @ 2013-06-18 19:33 UTC (permalink / raw) To: Junio C Hamano; +Cc: git 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é ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2013-06-18 19:33 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-06-15 23:44 [PATCH] unpack-trees: don't shift conflicts left and right René Scharfe 2013-06-16 0:56 ` Junio C Hamano 2013-06-17 20:30 ` René Scharfe 2013-06-17 20:44 ` Junio C Hamano 2013-06-17 22:29 ` René Scharfe 2013-06-17 23:29 ` Junio C Hamano 2013-06-17 23:26 ` René Scharfe 2013-06-17 23:42 ` Junio C Hamano 2013-06-18 19:33 ` René Scharfe
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).