From: Bo Yang Date: Wed, 28 Apr 2010 03:37:00 GMT Subject: Re: [PATCH 1/2 v3] Make diffcore_std only can run once before a diff_flush. Message-ID: In-Reply-To: Hi Junio, I have not receive any comments on this thread from you, but I think it worth some words. I want to make these series patches landed and could you please give some more advice on this? Regards! Bo On Fri, Apr 23, 2010 at 11:55 AM, Bo Yang wrote: > On Fri, Apr 23, 2010 at 4:41 AM, Junio C Hamano wrote: >> It actually is stronger than that; we should never run it more than once, >> and it would be a bug if we did so.  Which codepath tries to call *_std() >> twice? > > In command 'git log --follow ...' > log_tree_diff call diff_tree_sha1 and then diff_tree_diff_flush, when > '--follow' is given, the former function will call > try_to_follow_renames, which will call diffcore_std to detect rename. > And then, diff_tree_diff_flush call 'diffcore_std' again > unconditional. (and I have try to find a condition to make the call, > but I fail, so I figure out this patch.) > > Breakpoint 1, diffcore_std (options=0xbf9cc044) at diff.c:3748 > 3748            if (diff_queued_diff.run) > (gdb) bt > #0  diffcore_std (options=0xbf9cc044) at diff.c:3748 > #1  0x08124206 in try_to_follow_renames (t1=0xbf9cc130, t2=0xbf9cc11c, > base=0x81571c9 "", opt=0xbf9cc468) at tree-diff.c:358 > #2  0x08124480 in diff_tree_sha1 (old=0x9c51d8c > "$\033\222T���\a\035\200T����\210;8\235i", new=0x9c51d2c > "\201�\017<�\v��n]\226{�+�\001\003\232\232\230", >    base=0x81571c9 "", opt=0xbf9cc468) at tree-diff.c:418 > #3  0x080e660e in log_tree_diff (opt=0xbf9cc220, commit=0x9c51d28, > log=0xbf9cc1ac) at log-tree.c:536 > #4  0x080e668f in log_tree_commit (opt=0xbf9cc220, commit=0x9c51d28) > at log-tree.c:560 > #5  0x0807faa1 in cmd_log_walk (rev=0xbf9cc220) at builtin/log.c:237 > #6  0x080806e2 in cmd_log (argc=5, argv=0xbf9cc788, prefix=0x0) at > builtin/log.c:481 > #7  0x0804b8eb in run_builtin (p=0x8161524, argc=5, argv=0xbf9cc788) > at git.c:260 > #8  0x0804ba51 in handle_internal_command (argc=5, argv=0xbf9cc788) at git.c:416 > #9  0x0804bb2c in run_argv (argcp=0xbf9cc700, argv=0xbf9cc704) at git.c:458 > #10 0x0804bcbe in main (argc=5, argv=0xbf9cc788) at git.c:529 > (gdb) c > Continuing. > > Breakpoint 1, diffcore_std (options=0xbf9cc468) at diff.c:3748 > 3748            if (diff_queued_diff.run) > (gdb) bt > #0  diffcore_std (options=0xbf9cc468) at diff.c:3748 > #1  0x080e6356 in log_tree_diff_flush (opt=0xbf9cc220) at log-tree.c:449 > #2  0x080e6619 in log_tree_diff (opt=0xbf9cc220, commit=0x9c51d28, > log=0xbf9cc1ac) at log-tree.c:537 > #3  0x080e668f in log_tree_commit (opt=0xbf9cc220, commit=0x9c51d28) > at log-tree.c:560 > #4  0x0807faa1 in cmd_log_walk (rev=0xbf9cc220) at builtin/log.c:237 > #5  0x080806e2 in cmd_log (argc=5, argv=0xbf9cc788, prefix=0x0) at > builtin/log.c:481 > #6  0x0804b8eb in run_builtin (p=0x8161524, argc=5, argv=0xbf9cc788) > at git.c:260 > #7  0x0804ba51 in handle_internal_command (argc=5, argv=0xbf9cc788) at git.c:416 > #8  0x0804bb2c in run_argv (argcp=0xbf9cc700, argv=0xbf9cc704) at git.c:458 > #9  0x0804bcbe in main (argc=5, argv=0xbf9cc788) at git.c:529 > (gdb) > >> The standard calling sequence is: >> >>  - start from an empty queue. >> >>  - use diff_change() and diff_addremove() to populate the queue. >> >>  - call diffcore_std(). if you need to use a non-standard chain of >>   diffcore transformations, you _could_ call the diffcore_* routines that >>   diffcore_std() calls, if you choose to, but as you found out, some of >>   them are not idempotent operations, and shouldn't be called twice. >> >>  - and finally call diffcore_flush(). >> >>> @@ -3745,6 +3742,12 @@ void diffcore_fix_diff_index(struct diff_options *options) >>> >>>  void diffcore_std(struct diff_options *options) >>>  { >>> +     /* We never run this function more than one time, because the >>> +      * rename/copy detection logic can only run once. >>> +      */ >>> +     if (diff_queued_diff.run) >>> +             return; >> >> Shouldn't this be a BUG() instead? > > Anyone may call diff_tree_sha1 and then diffcore_std, and > diff_tree_sha1 may call another diffcore_std if '--follow' given. If > this is a BUG, the calling pattern, diff_tree_sha1 -> diffcore_std > should all disappear from our code. And this involved much code > refactor. And I suggest my way that we avoid the duplicate call > actively in diffcore_std. > >> The trivial rewrite to use this macro is a good idea, but it probably >> should be a separate patch. >> >>> +#define DIFF_QUEUE_CLEAR(q) \ >>> +     do { \ >>> +             (q)->queue = NULL; \ >>> +             (q)->nr = (q)->alloc = (q)->run = 0; \ >>> +     } while(0); >> > > You mean split this commit into two? > > Regards! > Bo > -- > My blog: http://blog.morebits.org >