Re: [PATCH v2 5/7] diff: use skip-to-optional-val in parsing --relative

[Junio C Hamano]

Jeff King  writes:

> On Thu, Dec 07, 2017 at 02:31:38PM -0800, Junio C Hamano wrote:
>
>> If this goes on top as a standalone patch, then the reason why it is
>> separate from the other users of _default() is not because the way
>> it uses the null return is special, but because it was written by a
>> different author, I would think.
>
> Mostly I was just concerned that we missed a somewhat subtle bug in the
> first conversion, and I think it's worth calling out in the commit
> message why that callsite must be converted in a particular way. Whether
> that happens in a separate commit or squashed, I don't care too much.
>
> But I dunno. Maybe it is obvious now that the correct code is in there.
> ;)

It probably is too obvious to me only because the use case like this
one that wanted to treat --foo and --foo="" differently was the only
reason why I pushed against Christian's original one that hardcoded
the equivalence without allowing what the _default() variant lets us
do, I think.

Re: [PATCH v2 5/7] diff: use skip-to-optional-val in parsing --relative

[Jeff King]

On Thu, Dec 07, 2017 at 02:31:38PM -0800, Junio C Hamano wrote:

> If this goes on top as a standalone patch, then the reason why it is
> separate from the other users of _default() is not because the way
> it uses the null return is special, but because it was written by a
> different author, I would think.

Mostly I was just concerned that we missed a somewhat subtle bug in the
first conversion, and I think it's worth calling out in the commit
message why that callsite must be converted in a particular way. Whether
that happens in a separate commit or squashed, I don't care too much.

But I dunno. Maybe it is obvious now that the correct code is in there.
;)

-Peff

Re: [PATCH v2 5/7] diff: use skip-to-optional-val in parsing --relative

[Junio C Hamano]

Jeff King  writes:

> On Thu, Dec 07, 2017 at 01:59:39PM -0800, Junio C Hamano wrote:
>
>> Jeff King  writes:
>> 
>> > On Thu, Dec 07, 2017 at 09:30:32AM -0800, Junio C Hamano wrote:
>> >
>> >> Signed-off-by: Junio C Hamano 
>> >> ---
>> >
>> > It might be worth mentioning why this conversion is pulled out from the
>> > others (because its "default" case is "do not touch the pointer").
>> 
>> I am not sure what you mean by "pulled out from the others".  I did
>> not intend to keep these 3 additional patches permanently; rather, I
>> did them to help Christian's rerolling the series, and I do not think
>> this one should be separate from other ones that use the _default()
>> variant when that happens.
>
> Ah, I see. I had thought you meant these to be applied on top.

Ah, I do not particularly mind doing things incrementally, either.

If this goes on top as a standalone patch, then the reason why it is
separate from the other users of _default() is not because the way
it uses the null return is special, but because it was written by a
different author, I would think.  

The reason why _default() variant exists is because its callers want
to react differently to "--foo" from the way they react to "--foo="
(with an empty string as its value), and from that point of view,
this caller is not all that different from other ones, like the one
that parses --color-words Christian wrote in his initial round.

Re: [PATCH v2 5/7] diff: use skip-to-optional-val in parsing --relative

[Jeff King]

On Thu, Dec 07, 2017 at 01:59:39PM -0800, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > On Thu, Dec 07, 2017 at 09:30:32AM -0800, Junio C Hamano wrote:
> >
> >> Signed-off-by: Junio C Hamano 
> >> ---
> >
> > It might be worth mentioning why this conversion is pulled out from the
> > others (because its "default" case is "do not touch the pointer").
> 
> I am not sure what you mean by "pulled out from the others".  I did
> not intend to keep these 3 additional patches permanently; rather, I
> did them to help Christian's rerolling the series, and I do not think
> this one should be separate from other ones that use the _default()
> variant when that happens.

Ah, I see. I had thought you meant these to be applied on top.

-Peff

Re: [PATCH v2 5/7] diff: use skip-to-optional-val in parsing --relative

[Junio C Hamano]

Jeff King  writes:

> On Thu, Dec 07, 2017 at 09:30:32AM -0800, Junio C Hamano wrote:
>
>> Signed-off-by: Junio C Hamano 
>> ---
>
> It might be worth mentioning why this conversion is pulled out from the
> others (because its "default" case is "do not touch the pointer").

I am not sure what you mean by "pulled out from the others".  I did
not intend to keep these 3 additional patches permanently; rather, I
did them to help Christian's rerolling the series, and I do not think
this one should be separate from other ones that use the _default()
variant when that happens.

Re: [PATCH v2 5/7] diff: use skip-to-optional-val in parsing --relative

[Jeff King]

On Thu, Dec 07, 2017 at 09:30:32AM -0800, Junio C Hamano wrote:

> Signed-off-by: Junio C Hamano 
> ---

It might be worth mentioning why this conversion is pulled out from the
others (because its "default" case is "do not touch the pointer").

Other than that, it looks good to me.

-Peff

Re: [PATCH v2 5/7] diff: use skip-to-optional-val in parsing --relative

[Jacob Keller]

On Thu, Dec 7, 2017 at 9:30 AM, Junio C Hamano  wrote:
> Signed-off-by: Junio C Hamano 
> ---
>  diff.c | 7 +++
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/diff.c b/diff.c
> index cd032c6367..e99ac6ec8a 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -4563,11 +4563,10 @@ int diff_opt_parse(struct diff_options *options,
> options->flags.rename_empty = 1;
> else if (!strcmp(arg, "--no-rename-empty"))
> options->flags.rename_empty = 0;
> -   else if (!strcmp(arg, "--relative"))
> +   else if (skip_to_optional_val_default(arg, "--relative", , NULL)) 
> {
> options->flags.relative_name = 1;
> -   else if (skip_prefix(arg, "--relative=", )) {
> -   options->flags.relative_name = 1;
> -   options->prefix = arg;
> +   if (arg)
> +   options->prefix = arg;
> }
>
> /* xdiff options */
> --
> 2.15.1-480-gbc5668f98a
>

Yea, this is exactly what I had imagined as the fix.

Thanks,
Jake