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

[Junio C Hamano <gitster@pobox.com>]

Brandon Williams <bmwill@google.com> writes:

> 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?".

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