Re: [PATCH] diff: remove another ternary expression always evaluating to true
On 08/08/2013 11:01 PM, Johannes Schindelin wrote:
Show 27 quoted lines
> Hi Stefan,
>
> On Thu, 8 Aug 2013, Stefan Beller wrote:
>
>> The condition before the changed line dereferences 'one' to query the mode,
>> so if the condition evaluates to true, the variable one must not be null.
>
> To show this better, please use -U10 (or some other appropriate context
> option) in the future.
>
>> Therefore we do not need the ternary operator depending on one, giving
>> either one->path or two->path. This always evaluates to one->path, so we
>> can remove the ternary operator.
>>
>> The condition and the usage of the ternary operator have been introduced
>> by the same commit (752c0c24, 2009-10-19, Add the --submodule option to
>> the diff option family). As that commit message refers to a GitTogether
>> I'd assume that patch was crafted in a hurry, so maybe overlooking the
>> need for a ternary operator there.
>
> If this is my code, I do not need a GitTogether to excuse my sloppiness.
> In this particular case, I imagine the appropriate fix is to test for
> one->path instead of removing the conditional, though.
>
> Ciao,
> Johannes
>
Ok, here is a resend with -U10 I forgot about the -U10 as gitk shows a different number of lines of context around. Is there a way to configure gitk to show less lines of code or a default -U10 for send-email/format-patch?
So you rather propose to have - show_submodule_summary(o->file, one ? one->path : two->path, + show_submodule_summary(o->file, one->path ? one->path : two->path,
instead of the patch below? (The test suite run without any problem using that patch)
Stefan
--8<--
From 3a580c51f0bf70745f26eb5045d646dfead2afd3 Mon Sep 17 00:00:00 2001
From: Stefan Beller <stefanbeller@googlemail.com>
Date: Thu, 8 Aug 2013 20:54:24 +0200
Subject: [PATCH] diff: remove another ternary expression always evaluating to
true
The condition before the changed line dereferences 'one' to query the mode, so if the condition evaluates to true, the variable one must not be null. Therefore we do not need the ternary operator depending on one, giving either one->path or two->path. This always evaluates to one->path, so we can remove the ternary operator.
The condition and the usage of the ternary operator have been introduced by the same commit (752c0c24, 2009-10-19, Add the --submodule option to the diff option family). As that commit message refers to a GitTogether I'd assume that patch was crafted in a hurry, so maybe overlooking the need for a ternary operator there.
Signed-off-by: Stefan Beller <stefanbeller@googlemail.com>
---
diff.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
Show changes to diff.c +1 −2
diff --git a/diff.c b/diff.c
index 80f8439..f30b7e4 100644
--- a/diff.c
+++ b/diff.c
@@ -2245,22 +2245,21 @@ static void builtin_diff(const char *name_a,
struct userdiff_driver *textconv_one = NULL;
struct userdiff_driver *textconv_two = NULL;
struct strbuf header = STRBUF_INIT;
const char *line_prefix = diff_line_prefix(o);
if (DIFF_OPT_TST(o, SUBMODULE_LOG) &&
(!one->mode || S_ISGITLINK(one->mode)) &&
(!two->mode || S_ISGITLINK(two->mode))) {
const char *del = diff_get_color_opt(o, DIFF_FILE_OLD);
const char *add = diff_get_color_opt(o, DIFF_FILE_NEW);
- show_submodule_summary(o->file, one ? one->path : two->path,
- line_prefix,
+ show_submodule_summary(o->file, one->path, line_prefix,
one->sha1, two->sha1, two->dirty_submodule,
meta, del, add, reset);
return;
}
if (DIFF_OPT_TST(o, ALLOW_TEXTCONV)) {
textconv_one = get_textconv(one);
textconv_two = get_textconv(two);
}
--
1.8.4.rc1.25.gd121ba2
Stefan Beller
