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

...

# HG changeset patch # User Dan Drogichen chirp.cordless@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