You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Stefan Sperling <st...@elego.de> on 2007/10/19 18:35:32 UTC
[PATCH] Update: svnlook diff and whitespace
Hello Michael,
I'm replying here to your reply in the bug tracker
(http://subversion.tigris.org/issues/show_bug.cgi?id=2912)
bcause the reply got a bit long and I want to resend the diff
to the list anyway.
I will add a link to this mail in the bug tracker once it
has reached the archives.
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.
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.
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.
I've created a recipe to illustrace this, and to test the patch.
See attachment.
With the updated patch you sent me in private mail, which
removes the buffering of the diff header, the following happens
when I run the recipe:
+ rm -rf /tmp/svnroot /tmp/wc
+ svnadmin create /tmp/svnroot
+ mkdir /tmp/wc
+ echo a b
+ > /tmp/wc/a
+ svn import -m importing /tmp/wc file:///tmp/svnroot/project
Adding /tmp/wc/a
Committed revision 1.
+ rm -rf /tmp/wc
+ svn co file:///tmp/svnroot/project /tmp/wc
A /tmp/wc/a
Checked out revision 1.
+ echo a b
+ > /tmp/wc/a
+ cd /tmp/wc
+ svn commit -m whitespace change
Sending a
Transmitting file data .
Committed revision 2.
+ /tmp/svnlook-cmpilato/bin/svnlook diff -r2 /tmp/svnroot
Modified: project/a
===================================================================
--- project/a 2007-10-19 18:24:36 UTC (rev 1)
+++ project/a 2007-10-19 18:24:37 UTC (rev 2)
@@ -1 +1 @@
-a b
+a b
+ /tmp/svnlook-cmpilato/bin/svn diff -c2 file:///tmp/svnroot
Index: project/a
===================================================================
--- project/a (revision 1)
+++ project/a (revision 2)
@@ -1 +1 @@
-a b
+a b
+ /tmp/svnlook-cmpilato/bin/svnlook diff -r2 -x --ignore-space-change /tmp/svnroot
Modified: project/a
===================================================================
+ /tmp/svnlook-cmpilato/bin/svn diff -c2 -x --ignore-space-change file:///tmp/svnroot
Note the diff header being printed with an empty line following it,
and 'svn diff' printing nothing.
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:
+ rm -rf /tmp/svnroot /tmp/wc
+ svnadmin create /tmp/svnroot
+ mkdir /tmp/wc
+ echo a b
+ > /tmp/wc/a
+ svn import -m importing /tmp/wc file:///tmp/svnroot/project
Adding /tmp/wc/a
Committed revision 1.
+ rm -rf /tmp/wc
+ svn co file:///tmp/svnroot/project /tmp/wc
A /tmp/wc/a
Checked out revision 1.
+ echo a b
+ > /tmp/wc/a
+ cd /tmp/wc
+ svn commit -m whitespace change
Sending a
Transmitting file data .
Committed revision 2.
+ /tmp/svnlook-stsp/bin/svnlook diff -r2 /tmp/svnroot
Modified: project/a
===================================================================
--- project/a 2007-10-19 18:23:14 UTC (rev 1)
+++ project/a 2007-10-19 18:23:15 UTC (rev 2)
@@ -1 +1 @@
-a b
+a b
+ /tmp/svnlook-stsp/bin/svn diff -c2 file:///tmp/svnroot
Index: project/a
===================================================================
--- project/a (revision 1)
+++ project/a (revision 2)
@@ -1 +1 @@
-a b
+a b
+ /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.
If my test does this wrong, then these tests do it wrong, too.
The goal of this test is of course to test whether
'svnlook diff --ignore-eolstyle' really works with all possible
eol styles. If there is a better way of doing this, I'd very much
like to know about it.
3.) There is a log message inside the patch now.
--
Stefan Sperling <st...@elego.de> Software Developer
elego Software Solutions GmbH HRB 77719
Gustav-Meyer-Allee 25, Gebaeude 12 Tel: +49 30 23 45 86 96
13355 Berlin Fax: +49 30 23 45 86 95
http://www.elego.de Geschaeftsfuehrer: Olaf Wagner
Re: [PATCH] Update: svnlook diff and whitespace
Posted by Stefan Sperling <st...@elego.de>.
On Tue, Oct 30, 2007 at 03:56:25PM -0400, C. Michael Pilato wrote:
> Patch committed with tweaks in r27495.
Thanks heaps :)
--
Stefan Sperling <st...@elego.de> Software Developer
elego Software Solutions GmbH HRB 77719
Gustav-Meyer-Allee 25, Gebaeude 12 Tel: +49 30 23 45 86 96
13355 Berlin Fax: +49 30 23 45 86 95
http://www.elego.de Geschaeftsfuehrer: Olaf Wagner
Re: [PATCH] Update: svnlook diff and whitespace
Posted by "C. Michael Pilato" <cm...@collab.net>.
Patch committed with tweaks in r27495.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: [PATCH] Update: svnlook diff and whitespace
Posted by Stefan Sperling <st...@elego.de>.
On Mon, Oct 22, 2007 at 10:33:14AM -0700, Daniel L. Rall wrote:
> 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,
Thanks for your comments.
For some reason I only noticed your mail now that Michael bounced
the thread back to the bottom of my svn-dev mailbox view in mutt.
Sorry :-/
(x-ref to "Issue 2646: Perl bindings" thread:)
You don't seem to be the only one having problem with mail,
and on top of that I am going to switch ISPs soon, too :)
Let's hope email won't fail us again,
--
Stefan Sperling <st...@elego.de> Software Developer
elego Software Solutions GmbH HRB 77719
Gustav-Meyer-Allee 25, Gebaeude 12 Tel: +49 30 23 45 86 96
13355 Berlin Fax: +49 30 23 45 86 95
http://www.elego.de Geschaeftsfuehrer: Olaf Wagner
Re: [PATCH] Update: svnlook diff and whitespace
Posted by "Daniel L. Rall" <dl...@collab.net>.
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