Hi Frank,
On Thu, Aug 1, 2019 at 9:55 PM Frank Rowand <frowand.list@xxxxxxxxx> wrote:
> On 8/1/19 5:13 AM, Geert Uytterhoeven wrote:
> > On Wed, Jul 31, 2019 at 10:30 PM Frank Rowand <frowand.list@xxxxxxxxx> wrote:
> >> On 7/31/19 5:37 AM, Geert Uytterhoeven wrote:
> >>> Add new -c/--color options, to enhance the diff output with color, and
> >>> improve the user's experience.
> >>>
> >>> Signed-off-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
> >>> ---
> >>> scripts/dtc/dtx_diff | 10 +++++++++-
> >>> 1 file changed, 9 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/scripts/dtc/dtx_diff b/scripts/dtc/dtx_diff
> >>> index e9ad7834a22d9459..4e2c8617f69a333e 100755
> >>> --- a/scripts/dtc/dtx_diff
> >>> +++ b/scripts/dtc/dtx_diff
> >>> @@ -20,6 +20,8 @@ Usage:
> >>>
> >>>
> >>> --annotate synonym for -T
> >>> + --color synonym for -c
> >>> + -c enable colored output
> >>> -f print full dts in diff (--unified=99999)
> >>> -h synonym for --help
> >>> -help synonym for --help
> >
> >> I like the idea, but...
> >>
> >> I have various linux distro releases across my many systems, but only one is
> >> new enough to have the diff command that supports --color.
> >
> > Seems to have been added in diffutils release 3.4 (2016-08-08).
> > I almost can't believe it was that recent, but then I remembered using a
> > wrapper before (colordiff; other wrappers may exist).
> >
> >> Can you enhance this patch to test whether --color is supported? Maybe
> >> something like (untested):
> >>
> >> -c | --color )
> >> if `diff --color <(echo a) <(echo a) 2>/dev/null` ; then
> >> diff_color="--color=always"
> >> fi
> >> shift
> >> ;;
> >>
> >> Then add some text to the usage for -c and --color saying that they will
> >> be silently ignored if diff does not support --color.
> >>
> >> I first wrote up a suggested version that printed an error message and
> >> exited, but I think silently ignoring is more robust, even though it
> >> may be more confusing to someone who is wondering why --color does not
> >> work.
> >
> > Given this is an optional feature, to be enabled explicitly by the user,
> > I'm not so fond of going through hoops to auto-detect the availability.
> >
> > So what about just documenting this in the help text instead?
> >
> > - -c enable colored output
> > + -c enable colored output (requires diff with --color support)
>
> ----- thought 1 -----
>
> My first thought was:
>
> If the hoops were complex and ugly, I might agree with you. But since it is
> a simple one line "if" (two lines including "fi") I prefer the check.
>
> The help text update looks good to me, along with the check.
OK.
> ----- thought 2 -----
>
> Then I reconsidered, and thought "well, Geert has a good idea". So I
> decided to see how useful the diff error message would be. The message is:
>
> $ scripts/dtc/dtx_diff -c a.dts b.dts
> diff: unrecognized option '--color=always'
> diff: Try 'diff --help' for more information.
> $
> Possible hints to resolve the above error:
> (hints might not fix the problem)
>
> No hints available.
>
> It is interesting that the shell prompt arrives before the full set of
> messages from the script, but that is not my issue. My issue is that
That is due to the output coming from the two "<(compile_to_dts ...)"
sub-processes, not from the diff sub-process.
> when the diff fails, the script tries to find suggestions to solve
> the problem. (The suggestions exist to catch some likely problems
> with the shell variable "ARCH".)
Interesting. I didn't know about the hints (never saw them), and had to
try hard to trigger them (I usually do DTB comparisons only).
But I succeeded ;-)
With a small tweak as my diff does support --color:
$ scripts/dtc/dtx_diff -c
arch/arm64/boot/dts/renesas/r8a7799*{ebisu,draak}.dts
diff: unrecognized option '--olor=always'
diff: Try 'diff --help' for more information.
$
Possible hints to resolve the above error:
(hints might not fix the problem)
shell variable $ARCH not set
architecture arm64 is in file path,
but does not match shell variable $ARCH
>>$ARCH<< is: >><<
Possible hints to resolve the above error:
(hints might not fix the problem)
shell variable $ARCH not set
architecture arm64 is in file path,
but does not match shell variable $ARCH
>>$ARCH<< is: >><<
> Unfortunately in the case of the "--color" problem, the useful warning
> from diff becomes less visible because of the early prompt and the
> not so helpful messages about hints.
Yeah, they are not so helpful.
In fact $ARCH is indeed not set, but that's not an issue at all.
> If the hints related messages were not present, then I was ready to
> accept that the diff warning was sufficient. But since the hints
> messages are present, and hiding them would be more complex than
> the original check that I suggested for whether diff supports
> the --color option, I am back to my first thought: I prefer the
> check whether diff supports "--color" is done when dtx_diff detects
> the "--color" option.
OK, you managed to convince me. Will fix.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
