Stefan Beller <sbeller@xxxxxxxxxx> writes: > On Tue, Jul 3, 2018 at 4:26 AM Johannes Schindelin via GitGitGadget > <gitgitgadget@xxxxxxxxx> wrote: >> >> From: Johannes Schindelin <johannes.schindelin@xxxxxx> >> >> When displaying a diff of diffs, it is possible that there is an outer >> `+` before a context line. That happens when the context changed between >> old and new commit. When that context line starts with a tab (after the >> space that marks it as context line), our diff machinery spits out a >> white-space error (space before tab), but in this case, that is >> incorrect. >> >> Work around this by detecting that situation and simply *not* printing >> the space in that case. > > ok. If that is the workaround that you deem to be the right thing for now. > (I do not have an opinion if that is the right approach, or if we'd want > to s/<TAB>/<SPACE>/ for example.) > >> This is slightly improper a fix because it is conceivable that an >> output_prefix might be configured with *just* the right length to let >> that tab jump to a different tab stop depending whether we emit that >> space or not. >> >> However, the proper fix would be relatively ugly and intrusive because >> it would have to *weaken* the WS_SPACE_BEFORE_TAB option in ws.c. I agree that weaking the error checking is a wrong solution. Is the root cause of this whole problem because for a diff of diff e.g. context that did not change between iterations - context in old interation -+whatever new contents added by old iteration + context in new interation updated by earlier step ++whatever new contents added by new iteration there needs to be a way to tell the ws.c whitespace breakage checking logic that the very first column is not interesting at all, and the "+" before "whatever" and " " before "context" should be considered to actually sit at the first (or zero-th) column of the diff output to be checked, but there is no interface to tell the machinery that wish, because there is no such need when inspecting a diff of contents? If the word "context" above were indented with HT, I can understand that the one common between iterations would trigger SP+HT violation that way. Is that what is happening here? Adding a way to tell that the apparent first column is to be ignored to ws.c machinery (or arranging the caller to skip the first column) may be more intrusive than it is worth, only to support this tool. Ignoring the problem altogether and live with an incorrectly colored SP-before-HT might be a less noisy but still acceptable solution from that point of view, though. I also wonder if we should be feeding the context lines to ws.c machinery in the first place though. In the above hypothetical diff-of-diff output, I _think_ the only two lines we want to check for ws.c breakage are the ones that begin with "whatever". We may find that both iterations are trying to introduce a ws breakage, or we may find that old one had violation which the new one corrected. A whitespace breakage on "context" lines, whether they are the ones being removed by the patch or the ones staying the same across the patch, is not worth painting---the normal diff-of-contents do not by default show them as violation, no?
