Re: What's cooking in git.git (Mar 2014, #04; Thu, 20)
Junio,
On Thu, Mar 20, 2014 at 03:31:35PM -0700, Junio C Hamano wrote:
Junio C Hamano gits...@pobox.com 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).
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 k...@mns.spb.ru writes:
Currently both compare_tree_entry() and show_path() invoke opt diff
s/show_path/show_entry/;
I agree, thanks for noticing.
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,
Re: What's cooking in git.git (Mar 2014, #04; Thu, 20)
Torsten Bögershausen tbo...@web.de writes: On 03/20/2014 10:09 PM, Junio C Hamano wrote: * ap/remote-hg-skip-null-bookmarks (2014-03-19) 1 commit - remote-hg: do not fail on invalid bookmarks Will merge to 'next'. Hmm, am I the only one who has 11 failures in test-hg-hg-git.sh, like this: (Tested under Debian 7, commit 04570b241ddb53ad2f355977bae541bd7ff32af5) master cde5672] clear executable bit Author: A U Thor aut...@example.com 1 file changed, 0 insertions(+), 0 deletions(-) mode change 100755 = 100644 alpha WARNING: Ignoring invalid bookmark 'master' To hg::../hgrepo-git ! [remote rejected] master - master error: failed to push some refs to 'hg::../hgrepo-git' not ok 1 - executable bit [] # failed 11 among 11 test(s) 1..11 This has again been replaced; I'll queue it as d06f17f8 (remote-hg: do not fail on invalid bookmarks, 2014-03-21) on 'pu'. Please holler if this still breaks for you. Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Mar 2014, #04; Thu, 20)
Junio C Hamano gits...@pobox.com writes: Torsten Bögershausen tbo...@web.de writes: On 03/20/2014 10:09 PM, Junio C Hamano wrote: * ap/remote-hg-skip-null-bookmarks (2014-03-19) 1 commit - remote-hg: do not fail on invalid bookmarks Will merge to 'next'. Hmm, am I the only one who has 11 failures in test-hg-hg-git.sh, like this: (Tested under Debian 7, commit 04570b241ddb53ad2f355977bae541bd7ff32af5) master cde5672] clear executable bit Author: A U Thor aut...@example.com 1 file changed, 0 insertions(+), 0 deletions(-) mode change 100755 = 100644 alpha WARNING: Ignoring invalid bookmark 'master' To hg::../hgrepo-git ! [remote rejected] master - master error: failed to push some refs to 'hg::../hgrepo-git' not ok 1 - executable bit [] # failed 11 among 11 test(s) 1..11 This has again been replaced; I'll queue it as d06f17f8 (remote-hg: do not fail on invalid bookmarks, 2014-03-21) on 'pu'. Please holler if this still breaks for you. Thanks. Ah, you seem to have sent a good review on that patch while I was looking at other topics ;-) And they all make sense, I think. Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Mar 2014, #04; Thu, 20)
On 03/20/2014 10:09 PM, Junio C Hamano wrote: * ap/remote-hg-skip-null-bookmarks (2014-03-19) 1 commit - remote-hg: do not fail on invalid bookmarks Will merge to 'next'. Hmm, am I the only one who has 11 failures in test-hg-hg-git.sh, like this: (Tested under Debian 7, commit 04570b241ddb53ad2f355977bae541bd7ff32af5) master cde5672] clear executable bit Author: A U Thor aut...@example.com 1 file changed, 0 insertions(+), 0 deletions(-) mode change 100755 = 100644 alpha WARNING: Ignoring invalid bookmark 'master' To hg::../hgrepo-git ! [remote rejected] master - master error: failed to push some refs to 'hg::../hgrepo-git' not ok 1 - executable bit [] # failed 11 among 11 test(s) 1..11 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Mar 2014, #04; Thu, 20)
Junio C Hamano gits...@pobox.com 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 k...@mns.spb.ru 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.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Mar 2014, #04; Thu, 20)
Torsten Bögershausen tbo...@web.de writes: On 03/20/2014 10:09 PM, Junio C Hamano wrote: * ap/remote-hg-skip-null-bookmarks (2014-03-19) 1 commit - remote-hg: do not fail on invalid bookmarks Will merge to 'next'. Hmm, am I the only one who has 11 failures in test-hg-hg-git.sh, Still? I thought I pushed out an updated version. like this: (Tested under Debian 7, commit 04570b241ddb53ad2f355977bae541bd7ff32af5) master cde5672] clear executable bit Author: A U Thor aut...@example.com 1 file changed, 0 insertions(+), 0 deletions(-) mode change 100755 = 100644 alpha WARNING: Ignoring invalid bookmark 'master' To hg::../hgrepo-git ! [remote rejected] master - master error: failed to push some refs to 'hg::../hgrepo-git' not ok 1 - executable bit [] # failed 11 among 11 test(s) 1..11 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
