git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v2 18/19] tree-diff: rework diff_tree() to generate diffs for multiparent cases as well

From
Junio C Hamano <gitster@pobox.com>
Date
Apr 7, 2014, 17:29 UTC
Message-ID
<xmqqr459m4j9.fsf@gitster.dls.corp.google.com>
In-Reply-To
<20140406214626.GA3843@mini.zxlink>
Kirill Smelkov <kirr@navytux.spb.ru> writes:
Show 12 quoted lines
> The following
> ...
> maybe looks a bit simpler, but calls tree_entry_pathcmp twice more times.
>
> Besides for important nparent=1 case we were not calling
> tree_entry_pathcmp at all and here we'll call it once, which would slow
> execution down a bit, as base_name_compare shows measurable enough in profile.
> To avoid that we'll need to add 'if (i==imin) continue' and this won't
> be so simple then. And for general nparent case, as I've said, we'll be
> calling tree_entry_pathcmp twice more times...
>
> Because of all that I'd suggest to go with my original version.
OK.
Show 63 quoted lines
> ... After some break on the topic,
> with a fresh eye I see a lot of confusion goes from the notation I've
> chosen initially (because of how I was reasoning about it on paper, when
> it was in flux) - i.e. xi for x[imin] and also using i as looping
> variable. And also because xi was already used for x[imin] I've used
> another letter 'k' denoting all other x'es, which leads to confusion...
>
>
> I propose we do the following renaming to clarify things:
>
>     A/a     ->      T/t     (to match resulting tree t name in the code)
>     X/x     ->      P/p     (to match parents trees tp in the code)
>     i       ->      imin    (so that i would be free for other tasks)
>
> then the above (with a prologue) would look like
>
> ---- 8< ----
>  *       T     P1       Pn
>  *       -     -        -
>  *      |t|   |p1|     |pn|
>  *      |-|   |--| ... |--|      imin = argmin(p1...pn)
>  *      | |   |  |     |  |
>  *      |-|   |--|     |--|
>  *      |.|   |. |     |. |
>  *       .     .        .
>  *       .     .        .
>  *
>  * at any time there could be 3 cases:
>  *
>  *      1)  t < p[imin];
>  *      2)  t > p[imin];
>  *      3)  t = p[imin].
>  *
>  * Schematic deduction of what every case means, and what to do, follows:
>  *
>  * 1)  t < p[imin]  ->  ∀j t ∉ Pj  ->  "+t" ∈ D(T,Pj)  ->  D += "+t";  t↓
>  *
>  * 2)  t > p[imin]
>  *
>  *     2.1) ∃j: pj > p[imin]  ->  "-p[imin]" ∉ D(T,Pj)  ->  D += ø;  ∀ pi=p[imin]  pi↓
>  *     2.2) ∀i  pi = p[imin]  ->  pi ∉ T  ->  "-pi" ∈ D(T,Pi)  ->  D += "-p[imin]";  ∀i pi↓
>  *
>  * 3)  t = p[imin]
>  *
>  *     3.1) ∃j: pj > p[imin]  ->  "+t" ∈ D(T,Pj)  ->  only pi=p[imin] remains to investigate
>  *     3.2) pi = p[imin]  ->  investigate δ(t,pi)
>  *      |
>  *      |
>  *      v
>  *
>  *     3.1+3.2) looking at δ(t,pi) ∀i: pi=p[imin] - if all != ø  ->
>  *
>  *                       ⎧δ(t,pi)  - if pi=p[imin]
>  *              ->  D += ⎨
>  *                       ⎩"+t"     - if pi>p[imin]
>  *
>  *
>  *     in any case t↓  ∀ pi=p[imin]  pi↓
> ...
> now xk is gone and i matches p[i] (= pi) etc so variable names correlate
> to algorithm description better.
>
> Does that maybe clarify things?

That sounds more consistent (modulo perhaps s/argmin/min/ at the beginning?).

> P.S. Sorry for maybe some crept-in mistakes - I've tried to verify it
> thoroughly, but am too sleepy to be completely sure. On the other hand I
> think and hope the patch should be ok.

Thanks and do not be sorry for "mistakes"---we have the review process exactly for catching them.

Previous: Kirill SmelkovNext: Junio C Hamano
Message 59 of 64 in “Multiparent diff tree-walker + combine-diff speedup”
  1. 00/19 Multiparent diff tree-walker + combine-diff speedupKirill Smelkov, Feb 24, 2014
  2. 01/19 combine-diff: move show_log_first logic/action out of paths scanningKirill Smelkov, Feb 24, 2014
  3. 02/19 combine-diff: move changed-paths scanning logic into its own functionKirill Smelkov, Feb 24, 2014
  4. 03/19 tree-diff: no need to manually verify that there is no mode change for a pathKirill Smelkov, Feb 24, 2014
  5. 04/19 tree-diff: no need to pass match to skip_uninteresting()Kirill Smelkov, Feb 24, 2014
  6. 05/19 tree-diff: show_tree() is not neededKirill Smelkov, Feb 24, 2014
  7. 06/19 tree-diff: consolidate code for emitting diffs and recursion in one placeKirill Smelkov, Feb 24, 2014
  8. 07/19 tree-diff: don't assume compare_tree_entry() returns -1,0,1Kirill Smelkov, Feb 24, 2014
  9. 08/19 tree-diff: move all action-taking code out of compare_tree_entry()Kirill Smelkov, Feb 24, 2014
  10. 09/19 tree-diff: rename compare_tree_entry -> tree_entry_pathcmpKirill Smelkov, Feb 24, 2014
  11. 10/19 tree-diff: show_path prototype is not needed anymoreKirill Smelkov, Feb 24, 2014
  12. 11/19 tree-diff: simplify tree_entry_pathcmpKirill Smelkov, Feb 24, 2014
  13. Junio C HamanoMar 24, 2014
  14. Kirill SmelkovMar 25, 2014
  15. 12/19 tree-diff: remove special-case diff-emitting code for empty-tree casesKirill Smelkov, Feb 24, 2014
  16. Junio C HamanoMar 24, 2014
  17. Kirill SmelkovMar 25, 2014
  18. Junio C HamanoMar 25, 2014
  19. Junio C HamanoMar 25, 2014
  20. Kirill SmelkovMar 26, 2014
  21. 13/19 tree-diff: diff_tree() should now be staticKirill Smelkov, Feb 24, 2014
  22. 14/19 tree-diff: rework diff_tree interface to be sha1 basedKirill Smelkov, Feb 24, 2014
  23. Junio C HamanoMar 24, 2014
  24. Kirill SmelkovMar 25, 2014
  25. Junio C HamanoMar 25, 2014
  26. Kirill SmelkovMar 26, 2014
  27. Junio C HamanoMar 26, 2014
  28. Kirill SmelkovMar 27, 2014
  29. Junio C HamanoMar 27, 2014
  30. Kirill SmelkovMar 27, 2014
  31. Johannes SixtMar 28, 2014
  32. Junio C HamanoMar 28, 2014
  33. Johannes SixtMar 28, 2014
  34. Junio C HamanoMar 28, 2014
  35. Johannes SixtMar 28, 2014
  36. Junio C HamanoMar 28, 2014
  37. 15/19 tree-diff: no need to call "full" diff_tree_sha1 from show_path()Kirill Smelkov, Feb 24, 2014
  38. Kirill SmelkovMar 27, 2014
  39. 16/19 tree-diff: reuse base str(buf) memory on sub-tree recursionKirill Smelkov, Feb 24, 2014
  40. Junio C HamanoMar 24, 2014
  41. Kirill SmelkovMar 25, 2014
  42. Kirill SmelkovMar 27, 2014
  43. 17/19 Portable alloca for GitKirill Smelkov, Feb 24, 2014
  44. Thomas SchwingeFeb 28, 2014
  45. Erik Faye-LundFeb 28, 2014
  46. Erik Faye-LundFeb 28, 2014
  47. Kirill SmelkovFeb 28, 2014
  48. Erik Faye-LundFeb 28, 2014
  49. Kirill SmelkovMar 5, 2014
  50. Junio C HamanoMar 24, 2014
  51. Kirill SmelkovMar 27, 2014
  52. Kirill SmelkovApr 9, 2014
  53. Erik Faye-LundApr 9, 2014
  54. Junio C HamanoApr 10, 2014
  55. 18/19 tree-diff: rework diff_tree() to generate diffs for multiparent cases as wellKirill Smelkov, Feb 24, 2014
  56. Kirill SmelkovMar 27, 2014
  57. Junio C HamanoApr 4, 2014
  58. Kirill SmelkovApr 6, 2014
  59. Junio C HamanoApr 7, 2014
  60. Junio C HamanoApr 7, 2014
  61. Kirill SmelkovApr 7, 2014
  62. 19/19 combine-diff: speed it up, by using multiparent diff tree-walker directlyKirill Smelkov, Feb 24, 2014
  63. Duy NguyenFeb 24, 2014
  64. Kirill SmelkovFeb 25, 2014

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.