Junio C Hamano [off-list ref] writes:
Quite a few topics are still outside 'pu' and I suspect some of the
larger ones deserve deeper reviews to help moving them to 'next'.
In principle, I'd prefer to keep any large topic that touch core
part of the system cooking in 'next' for at least a full cycle, and
the sooner they get merged to 'next', the better. Help is greatly
appreciated.
...
* ks/tree-diff-nway (2014-03-04) 19 commits
- combine-diff: speed it up, by using multiparent diff tree-walker directly
- tree-diff: rework diff_tree() to generate diffs for multiparent cases as well
- Portable alloca for Git
- tree-diff: reuse base str(buf) memory on sub-tree recursion
- tree-diff: no need to call "full" diff_tree_sha1 from show_path()
- tree-diff: rework diff_tree interface to be sha1 based
- tree-diff: diff_tree() should now be static
- tree-diff: remove special-case diff-emitting code for empty-tree cases
- tree-diff: simplify tree_entry_pathcmp
- tree-diff: show_path prototype is not needed anymore
- tree-diff: rename compare_tree_entry -> tree_entry_pathcmp
- tree-diff: move all action-taking code out of compare_tree_entry()
- tree-diff: don't assume compare_tree_entry() returns -1,0,1
- tree-diff: consolidate code for emitting diffs and recursion in one place
- tree-diff: show_tree() is not needed
- tree-diff: no need to pass match to skip_uninteresting()
- tree-diff: no need to manually verify that there is no mode change for a path
- combine-diff: move changed-paths scanning logic into its own function
- combine-diff: move show_log_first logic/action out of paths scanning
Instead of running N pair-wise diff-trees when inspecting a
N-parent merge, find the set of paths that were touched by walking
N+1 trees in parallel. These set of paths can then be turned into
N pair-wise diff-tree results to be processed through rename
detections and such. And N=2 case nicely degenerates to the usual
2-way diff-tree, which is very nice.
So I started re-reading this series, and decided that it might be
easier to advance the topic piece-by-piece. Will be merging up to
"consolidate code for emitting diffs" to 'next', after tweaking that
last commit a bit (see below).
Kirill Smelkov [off-list ref] writes:
Currently both compare_tree_entry() and show_path() invoke opt diff
s/show_path/show_entry/;
callbacks (opt->add_remove() and opt->change()), and also they both have
code which decides whether to recurse into sub-tree, and whether to emit
a tree as separate entry if DIFF_OPT_TREE_IN_RECURSIVE is set.
I.e. we have code duplication and logic scattered on two places.
Let's consolidate it...
...
+/* convert path, t1/t2 -> opt->diff_*() callbacks */
+static void emit_diff(struct diff_options *opt, struct strbuf *path,
+ struct tree_desc *t1, struct tree_desc *t2)
+{
+ unsigned int mode1 = t1 ? t1->entry.mode : 0;
+ unsigned int mode2 = t2 ? t2->entry.mode : 0;
+
+ if (mode1 && mode2) {
+ opt->change(opt, mode1, mode2, t1->entry.sha1, t2->entry.sha1,
+ 1, 1, path->buf, 0, 0);
This is not too bad per-se, but it would have been even better if
this part was done as:
if (t1 && t2) {
opt->change(opt, t1->entry.mode1, t1->entry.mode2,
t1->entry.sha1, t2->entry.sha1,
1, 1, path->buf, 0, 0);
}
Then we do not have to rely on an extra convention, "'mode == 0'
means the entry is missing", in addition to what we already have,
i.e. "t == NULL means the entry is missing".
This is minor, so I will not squash such a change in while merging
to 'next', but we may want to revisit and fix it up as a follow up
patch once the series is settled.
+ }
+ else {
Style; I've merged these two lines into one, i.e.
} else {
There was another instance of it in show_path(), which I also
tweaked.
Thanks.
Junio,
On Thu, Mar 20, 2014 at 03:31:35PM -0700, Junio C Hamano wrote:
Junio C Hamano [off-list ref] writes:
quoted
Quite a few topics are still outside 'pu' and I suspect some of the
larger ones deserve deeper reviews to help moving them to 'next'.
In principle, I'd prefer to keep any large topic that touch core
part of the system cooking in 'next' for at least a full cycle, and
the sooner they get merged to 'next', the better. Help is greatly
appreciated.
...
* ks/tree-diff-nway (2014-03-04) 19 commits
- combine-diff: speed it up, by using multiparent diff tree-walker directly
- tree-diff: rework diff_tree() to generate diffs for multiparent cases as well
- Portable alloca for Git
- tree-diff: reuse base str(buf) memory on sub-tree recursion
- tree-diff: no need to call "full" diff_tree_sha1 from show_path()
- tree-diff: rework diff_tree interface to be sha1 based
- tree-diff: diff_tree() should now be static
- tree-diff: remove special-case diff-emitting code for empty-tree cases
- tree-diff: simplify tree_entry_pathcmp
- tree-diff: show_path prototype is not needed anymore
- tree-diff: rename compare_tree_entry -> tree_entry_pathcmp
- tree-diff: move all action-taking code out of compare_tree_entry()
- tree-diff: don't assume compare_tree_entry() returns -1,0,1
- tree-diff: consolidate code for emitting diffs and recursion in one place
- tree-diff: show_tree() is not needed
- tree-diff: no need to pass match to skip_uninteresting()
- tree-diff: no need to manually verify that there is no mode change for a path
- combine-diff: move changed-paths scanning logic into its own function
- combine-diff: move show_log_first logic/action out of paths scanning
Instead of running N pair-wise diff-trees when inspecting a
N-parent merge, find the set of paths that were touched by walking
N+1 trees in parallel. These set of paths can then be turned into
N pair-wise diff-tree results to be processed through rename
detections and such. And N=2 case nicely degenerates to the usual
2-way diff-tree, which is very nice.
So I started re-reading this series, and decided that it might be
easier to advance the topic piece-by-piece. Will be merging up to
"consolidate code for emitting diffs" to 'next', after tweaking that
last commit a bit (see below).
Thanks, yes, I agree - merging it step-by-step is good approach as doing
it all at once requires more one-time effort, which is harder to do, and
otherwise the topic just stays without progress. So yes, let's do it
incrementally.
Kirill Smelkov [off-list ref] writes:
quoted
Currently both compare_tree_entry() and show_path() invoke opt diff
s/show_path/show_entry/;
I agree, thanks for noticing.
quoted
callbacks (opt->add_remove() and opt->change()), and also they both have
code which decides whether to recurse into sub-tree, and whether to emit
a tree as separate entry if DIFF_OPT_TREE_IN_RECURSIVE is set.
I.e. we have code duplication and logic scattered on two places.
Let's consolidate it...
...
+/* convert path, t1/t2 -> opt->diff_*() callbacks */
+static void emit_diff(struct diff_options *opt, struct strbuf *path,
+ struct tree_desc *t1, struct tree_desc *t2)
+{
+ unsigned int mode1 = t1 ? t1->entry.mode : 0;
+ unsigned int mode2 = t2 ? t2->entry.mode : 0;
+
+ if (mode1 && mode2) {
+ opt->change(opt, mode1, mode2, t1->entry.sha1, t2->entry.sha1,
+ 1, 1, path->buf, 0, 0);
This is not too bad per-se, but it would have been even better if
this part was done as:
if (t1 && t2) {
opt->change(opt, t1->entry.mode1, t1->entry.mode2,
t1->entry.sha1, t2->entry.sha1,
1, 1, path->buf, 0, 0);
}
Then we do not have to rely on an extra convention, "'mode == 0'
means the entry is missing", in addition to what we already have,
i.e. "t == NULL means the entry is missing".
I agree, but the reason it is done here so is to prepare for later changes in
"tree-diff: rework diff_tree() to generate diffs for multiparent cases as
well", where modes will be right-available from `struct combine_diff_path` and
would also indicate absent entries:
-/* convert path, t1/t2 -> opt->diff_*() callbacks */
-static void emit_diff(struct diff_options *opt, struct strbuf *path,
- struct tree_desc *t1, struct tree_desc *t2)
+/*
+ * convert path -> opt->diff_*() callbacks
+ *
+ * emits diff to first parent only, and tells diff tree-walker that we are done
+ * with p and it can be freed.
+ */
+static int emit_diff_first_parent_only(struct diff_options *opt, struct combine_diff_path *p)
{
- unsigned int mode1 = t1 ? t1->entry.mode : 0;
- unsigned int mode2 = t2 ? t2->entry.mode : 0;
-
- if (mode1 && mode2) {
- opt->change(opt, mode1, mode2, t1->entry.sha1, t2->entry.sha1,
- 1, 1, path->buf, 0, 0);
+ struct combine_diff_parent *p0 = &p->parent[0];
+ if (p->mode && p0->mode) {
+ opt->change(opt, p0->mode, p->mode, p0->sha1, p->sha1,
+ 1, 1, p->path, 0, 0);
}
else {
const unsigned char *sha1;
unsigned int mode;
int addremove;
- if (mode2) {
+ if (p->mode) {
addremove = '+';
- sha1 = t2->entry.sha1;
- mode = mode2;
+ sha1 = p->sha1;
+ mode = p->mode;
}
else {
addremove = '-';
- sha1 = t1->entry.sha1;
- mode = mode1;
+ sha1 = p0->sha1;
+ mode = p0->mode;
}
- opt->add_remove(opt, addremove, mode, sha1, 1, path->buf, 0);
+ opt->add_remove(opt, addremove, mode, sha1, 1, p->path, 0);
}
...
So this way we are preparing the code for that big interesting patch.
This is minor, so I will not squash such a change in while merging
to 'next', but we may want to revisit and fix it up as a follow up
patch once the series is settled.
I agree that this is minor, and could be reworked in-place, but
squashing it later is not applicable as the code is later removed.
quoted
+ }
+ else {
Style; I've merged these two lines into one, i.e.
} else {
There was another instance of it in show_path(), which I also
tweaked.
Ok and thanks,
Kirill