Re: [PATCH v2 3/4] diff: add flag to indicate textconv was set via cmdline

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 31, 2017, 05:02 UTC
Message-ID: <xmqq4lqf3o5q.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20171030194646.27473-4-bmwill@google.com>

Brandon Williams <bmwill@google.com> writes:

Show 12 quoted lines

> diff --git a/builtin/log.c b/builtin/log.c
> index dc28d43eb..82131751d 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -485,7 +485,7 @@ static int show_blob_object(const struct object_id *oid, struct rev_info *rev, c
>  	unsigned long size;
>  
>  	fflush(rev->diffopt.file);
> -	if (!DIFF_OPT_TOUCHED(&rev->diffopt, ALLOW_TEXTCONV) ||
> +	if (!DIFF_OPT_TST(&rev->diffopt, TEXTCONV_SET_VIA_CMDLINE) ||
>  	    !DIFF_OPT_TST(&rev->diffopt, ALLOW_TEXTCONV))
>  		return stream_blob_to_fd(1, oid, NULL, 0);

The original is equivalent to

	if (! (DIFF_OPT_TOUCHED() && DIFF_OPT_TST()))
		return stream_blob_to_fd();

which means that we must have used DIFF_OPT_SET() or DIFF_OPT_CLR() to touch the ALLOW_TEXTCONV bit, and ALLOW_TEXTCONV bit is currently set, in order for the flow to skip this "just stream it out".

And the way it implemented it was:

#define DIFF_OPT_TOUCHED(opts, flag) ((opts)->touched_flags & DIFF_OPT_##flag) #define DIFF_OPT_SET(opts, flag) (((opts)->flags |= DIFF_OPT_##flag),((opts)->touched_flags |= DIFF_OPT_##flag)) #define DIFF_OPT_CLR(opts, flag) (((opts)->flags &= ~DIFF_OPT_##flag),((opts)->touched_flags |= DIFF_OPT_##flag))

Notice that touched_flags is SET in both OPT_SET() and OPT_CLR(), because the point of _TOUCHED() was "did the user made an explicit request to affect the value of the bit from the command line?".

Show 18 quoted lines

> diff --git a/diff.c b/diff.c
> index 3ad9c9b31..8b700b1bd 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -4762,11 +4762,13 @@ int diff_opt_parse(struct diff_options *options,
>  		DIFF_OPT_SET(options, ALLOW_EXTERNAL);
>  	else if (!strcmp(arg, "--no-ext-diff"))
>  		DIFF_OPT_CLR(options, ALLOW_EXTERNAL);
> -	else if (!strcmp(arg, "--textconv"))
> +	else if (!strcmp(arg, "--textconv")) {
>  		DIFF_OPT_SET(options, ALLOW_TEXTCONV);
> -	else if (!strcmp(arg, "--no-textconv"))
> +		DIFF_OPT_SET(options, TEXTCONV_SET_VIA_CMDLINE);
> +	} else if (!strcmp(arg, "--no-textconv")) {
>  		DIFF_OPT_CLR(options, ALLOW_TEXTCONV);
> -	else if (!strcmp(arg, "--ignore-submodules")) {
> +		DIFF_OPT_CLR(options, TEXTCONV_SET_VIA_CMDLINE);
> +	} else if (!strcmp(arg, "--ignore-submodules")) {

If we were aiming for faithful conversion, the above must be DIFF_OPT_SET(options, TEXTCONV_SET_VIA_CMDLINE), not CLR.

HOWEVER, I think it is fine to define TEXTCONV_SET_VIA_CMDLINE bit differently from what DIFF_OPT_TOUCHED(ALLOW_TEXTCONV) meant in the old code (i.e. "did the user made an explicit request to affect the value?"). That is, we can define the new one as "did the user explicitly SET the bit from the command line?", the conditional in show_blob_object() is prepared to take either interpretation. "User explicitly set the bit to true, and the bit is true" and "User explicitly set the bit to something, and the bit is true" are pretty much the same thing.

And that leads me to suggest dropping the last change here, to touch VIA_CMDLINE in response to "--no-textconv".

Other than that, looks good to me.

Thanks.