[chirp_devel] [developer] Add diffs-only mode to file diff utility - #1699

[chirp.cordless at xoxy.net]

On Jul 7, 2014, at 2:22 PM, Dan Smith - dsmith at danplanet.com  wrote:

>> # HG changeset patch
>> # User Dan Drogichen <chirp.cordless at xoxy.net>
>> # Date 1403149386 25200
>> #      Wed Jun 18 20:43:06 2014 -0700
>> # Node ID d03df310f7ae9734ee1a78128d062f334f81114b
>> # Parent  293cf2f7da0fa9d4000faf71bf8b4872f78f0bb1
>> [developer] Add diffs-only mode to file diff utility - #1699
>> 
>> Adds an alternate display mode to the developer tools dump/diff utility
>> in which only lines which are different are printed. Ranges of one or
>> more lines that do not differ are replaced with a single blank line.
> 
> Man, I *really* don't understand why this is a better way to look at it,
> but maybe I've been looking at real diffs for too long :)

This comment leaves me quite confused. This new mode that I'm
adding is _way_ closer in behavior to "real diffs", e.g. unix diff(1) or
"hg diff" than the dump/diff mode that is already present.
It's also much closer in behavior to the standalone tools/bitdiff.py
that Jens Jensen provided.

What "real diff" prints all 3500 lines of a file with one or two bytes
different? That's what the current code does. Not that this dump
isn't useful, it very much is. And given that it already exists for when
context is wanted, and that the value of context to diffs in a binary file
isn't that significant compared to the need to not miss any, this second
mode seems needed to me.


>> This mode is invoked by setting the first memory location in the
>> "Diff Radios" popup to -2. The existing functionality invoked by -1
>> is not affected.
> 
> Hmm, setting one of them to -2 and the other to what? The point of the
> first/second box is so that you can diff two different memory locations
> in two images. Does this enable only diffing two corresponding locations
> in this special mode?

I simply extended the existing interface which was there before I ever
heard of chirp. To paraphrase your question: "setting one of them to -1
and the other to what?". The existing code invokes the whole-image
dump/diff if either address is -1, and it doesn't matter what the other is,
because neither value is used as a memory location. In the sense that
you're complaining about, my -2 behaves exactly the same.

> How about we just add another entry to the menu so that we have "Diff
> memories" and "Naked diff memories" (or some other name that conveys
> what we're talking about.

How about we consider that no check boxes were used by whoever
coded the -1 behavior, and that wasn't considered a problem.
Why has it become a problem now?

If you're concerned about informing the (developer) user - well,
so am I. My intention is that the next bug I submit, pending this
one being accepted, is this item from the list I submitted June 3:
> Added some help text to the "Diff Radios" dialog explaining the
> hex dumps available with mem # = -1 and -2.



>> +    blankprinted = True
>>     for line in lines:
>>         if line.startswith("-"):
>>             tags = ("red", "bold")
>>         elif line.startswith("+"):
>>             tags = ("blue", "bold")
>> +            blankprinted = False
>> +        elif diffsonly == True:
>> +            if blankprinted:
>> +                continue
>> +            else:
>> +                line = ""
>> +                tags = ()
>> +                blankprinted = True
> 
> This is really confusing. Is the point to ensure that after a + line you
> get a blank line?

Almost. So that any range of one or more lines that don't differ
are replaced by exactly one blank line, which is what I said I was
doing in bug #1699. See quoted patch comment at the top.

> Regardless, doing this in the display routine seems
> like the wrong place to do this. Why not modify common.simple_diff() to
> generate what you want in the first place?

It could probably be done in either place. I was already modifying
show_diff_blob() for the fontsize (#1681). If that will get this accepted
I can redo the work to move this code to common.simple_diff().

-dan

> --Dan