> On Fri, Apr 23, 2010 at 4:41 AM, Junio C Hamano <gitster@pobox.com> 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
> "$32T���\a50T����0;85i", new=0x9c51d2c
> "1�7<�\v��n]6{�+�__PLACEHOLDER__0_1__PLACEHOLDER__0_3220",
> 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
>
