Re: [PATCH] Update: svnlook diff and whitespace

On Fri, 19 Oct 2007, Stefan Sperling wrote:
...
> I'm replying here to your reply in the bug tracker
> (http://subversion.tigris.org/issues/show_bug.cgi?id=2912)
> because the reply got a bit long and I want to resend the diff
> to the list anyway.
...
> 1.) Regarding the diff to main.c:
>
> My diff assumes that the user simply does not care about files
> that contain whitespace-only changes and does not want any information
> printed for them, not even the filename.

This makes sense for all three "ignore whitespace"-related options.

> So the idea is that there is no output at all if there are only
> whitespace changes in a file and --ignore-whitespace is specified.
>
> This behaviour is consistent with how 'svn diff' is behaving.
 
Good.

> So in this case, the hunk you proposed alone is not sufficient
> because we need to delay printing the header to stdout until we know
> whether there really is a diff to print.
>
> I don't know if this behaviour is desired, we might as well print
> something like "whitespace-only changes ignored" or simply favour
> nothing following a diff header. I am open for opinions on that.
> But I think we should try to make 'svn diff' and 'svnlook diff'
> behave as similarly as possible.

Yeah, I'm in favor of how your previous patch works. Printing anything
in this case defeats the purpose...

...
> With my updated patch (attached) which, like the original,
> creates the diff header in a buffer to be able to decide not to
> print it, I get this:
...
> + /tmp/svnlook-stsp/bin/svnlook diff -r2 -x --ignore-space-change /tmp/svnroot
> + /tmp/svnlook-stsp/bin/svn diff -c2 -x --ignore-space-change file:///tmp/svnroot
>
>
> No output from neither svn nor svnlook.
>
>
> 2.) Regarding the test:
>
> See merge_conflict_markers_matching_eol() in merge_tests.py
> and conflict_markers_matching_eol() in update_tests.py
> They both do the exact same thing, I have copied this piece from
> either of these, I'm not sure which exactly.

I'd say as much, in an inline comment in the test.

Minor nits:

+ const char *extensions; /* diff extension args */ /* UTF-8! */

Those two comments could be collapsed into one.

The endline comment on the decl of txn_name could be put after the ";"
character.

+ svn_string_t *str;

This local variable is declared in two separate stack frames, so that we can
format a string and appended the formatted value to "header". Could we dump
the local intermediate var, and use apr_psprintf()/svn_stringbuf_append_cstr()
instead?

-- 
Daniel Rall