Re: [PATCH 1/2 v3] Make diffcore_std only can run once before a diff_flush.

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 22, 2010, 20:41 UTC
Message-ID: <7v1ve743vb.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1271945142-27015-2-git-send-email-struggleyb.nku@gmail.com>

Bo Yang <struggleyb.nku@gmail.com> writes:

> So, I think we really don't need to run diffcore_std more
> than one time.

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?

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().

Show 9 quoted lines

> @@ -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?

The trivial rewrite to use this macro is a good idea, but it probably should be a separate patch.

Show 5 quoted lines

> +#define DIFF_QUEUE_CLEAR(q) \
> +	do { \
> +		(q)->queue = NULL; \
> +		(q)->nr = (q)->alloc = (q)->run = 0; \
> +	} while(0);