On Jul 8, 2014, at 6:44 AM, Dan Smith - dsmith@danplanet.com wrote:
... Yeah, fair enough. I wasn't thinking about the "diff the whole file" there. I wrote that code, but I don't think I've used it that way even once. ...
I get that you're talking about the whole-file diff scenario, I see why using -2,-2 makes sense and why it makes sense to skip the common bits. Your commit message mentions a different diffing mode, but doesn't say anything about it only being available in the whole-file diff mode, so hopefully you can see where I was confused. I don't keep 100% context of all the code in my head all the time.
You know, I've been stewing on this, and had just about come to the conclusion that nobody but me knows that this tool exists in the current chirp code. When Jens pointed me at tools/bitdiff.py, And comments from Tom and you about my proposed changes that seemed very strange...
I started out to map the settings memory in the FT-60. I rarely use 'diff tabs' with other than -1 (or -2 after I added it) because I rarely look at channel memories; somebody already figured those out, and I'm looking elsewhere in the file. Almost all the changes in the list I've proposed are concerned with this -1/-2 whole-image hex dump mode. Maybe that will help going forward...
So, no context lines at all, right? If so, that still doesn't seem as useful to me, personally (although as I said, I don't use this). But, that's fine.
Since we seem to have converged on acceptance here, I'll stop arguing, but to explain: The -1 mode already provides all the context one could want. This is a hex dump, and context, while not useless, isn't all that meaningful. The alternate -2 mode is there to let me quickly and accurately identify every bit that's different, so I don't miss any. Then if I want context, -1 is a few clicks away...
Yeah, I very much think that this should be in simple_diff() itself, since this is a change to the format of the output of that, not just the simple colorization routine (model vs. view).
I can see where that's a more sensible partitioning of function. I will respin the patch for that change.
-dan