You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Lamar Goddard <la...@gmail.com> on 2010/06/05 01:02:52 UTC
[PATCH] issue #3292: fix for 'svnlook diff' regression: doesn't print
added dirs
[[[
Fixes issue #3292: Output svnlook diff content for copied/moved directories
* subversion/svnlook/main.c
(print_diff_tree): If diff isn't outputted but header has content,
output the header.
]]]
Index: subversion/svnlook/main.c
===================================================================
--- subversion/svnlook/main.c (revision 951625)
+++ subversion/svnlook/main.c (working copy)
@@ -1087,6 +1087,11 @@
}
SVN_ERR(svn_cmdline_fflush(stdout));
}
+ else if (header->len != 0)
+ {
+ /* Print diff header. */
+ SVN_ERR(svn_cmdline_printf(pool, "%s", header->data));
+ }
/* Make sure we delete any temporary files. */
if (orig_path)
--
Lamar Goddard
lamargoddard@gmail.com
Re: [PATCH] issue #3292: fix for 'svnlook diff' regression:
doesn't print added dirs
Posted by Stefan Sperling <st...@elego.de>.
On Wed, Jun 09, 2010 at 06:10:29PM -0700, Lamar Goddard wrote:
> Oh, I don't think my reply on the other thread made it to the dev
> list, hopefully this one will.
>
> On Wed, Jun 9, 2010 at 1:11 AM, Stefan Sperling <st...@elego.de> wrote:
> > On Fri, Jun 04, 2010 at 06:02:52PM -0700, Lamar Goddard wrote:
> >> [[[
> >> Fixes issue #3292: Output svnlook diff content for copied/moved directories
> >>
> >> * subversion/svnlook/main.c
> >> (print_diff_tree): If diff isn't outputted but header has content,
> >> output the header.
> >> ]]]
> >
> > Hi Lamar,
> >
> > thanks for the patch. It is perferectly fine technically,
> > but I'm unsure about how we (Subversion developers) want 'svnlook diff'
> > to behave.
> >
> > svnlook diff in 1.4 behaves differently than svn diff does in trunk.
> > svn diff never shows anything for directories (copied or not), it only shows
> > diffs for files. Do we want svn diff and svnlook diff behave the same way?
> > If so, maybe the 1.5/1.6 behaviour is correct and the 1.4 behaviour was wrong?
>
>
> My 2¢:
>
> I'm not sure svnlook diff should match svn diff's behavior.
>
> From what I understand there's no equivalent svnlook command to svn
> log -v or svn info so there's currently no way for a well behaved hook
> script to determine where a directory copy originated. svn log -v
> provides that information for revisions in the repository and svn info
> does the same for working copy changes.
I'm not sure the output of svnlook diff is supposed to be machine
parsable. "svnlook help diff" says "Print GNU-style diffs of changed
files and properties." Note that this does not talk about directories,
nor about parsing.
Michael Diers has mentioned to me that hook scripts such as enforcer
have trouble parsing the svnlook diff output reliably. It seems like
a much better idea to add a --xml switch to svnlook diff, which should
then be used by hooks which want to parse diff information. The XML
would contain the same of even more information than the human readable
output. Anything hook scripts out there want to know about changes made
in a transaction should be represented in the XML.
> I don't think there's a need for an Added or Deleted header because
> those are isolated actions that are self contained in the one path.
> Copied is different because it relates to more than one path. Knowing
> the destination only gives you a piece of the puzzle.
Well, you may not need information about added or deleted directories.
But I think this depends on what you want to know.
svnlook diff should be able to provide sufficient information for
any hook script out there.
Stefan
Re: [PATCH] issue #3292: fix for 'svnlook diff' regression:
doesn't print added dirs
Posted by Stefan Sperling <st...@elego.de>.
On Wed, Jun 09, 2010 at 06:10:29PM -0700, Lamar Goddard wrote:
> Oh, I don't think my reply on the other thread made it to the dev
> list, hopefully this one will.
>
> On Wed, Jun 9, 2010 at 1:11 AM, Stefan Sperling <st...@elego.de> wrote:
> > On Fri, Jun 04, 2010 at 06:02:52PM -0700, Lamar Goddard wrote:
> >> [[[
> >> Fixes issue #3292: Output svnlook diff content for copied/moved directories
> >>
> >> * subversion/svnlook/main.c
> >> (print_diff_tree): If diff isn't outputted but header has content,
> >> output the header.
> >> ]]]
> >
> > Hi Lamar,
> >
> > thanks for the patch. It is perferectly fine technically,
> > but I'm unsure about how we (Subversion developers) want 'svnlook diff'
> > to behave.
> >
> > svnlook diff in 1.4 behaves differently than svn diff does in trunk.
> > svn diff never shows anything for directories (copied or not), it only shows
> > diffs for files. Do we want svn diff and svnlook diff behave the same way?
> > If so, maybe the 1.5/1.6 behaviour is correct and the 1.4 behaviour was wrong?
>
>
> My 2¢:
>
> I'm not sure svnlook diff should match svn diff's behavior.
>
> From what I understand there's no equivalent svnlook command to svn
> log -v or svn info so there's currently no way for a well behaved hook
> script to determine where a directory copy originated. svn log -v
> provides that information for revisions in the repository and svn info
> does the same for working copy changes.
I'm not sure the output of svnlook diff is supposed to be machine
parsable. "svnlook help diff" says "Print GNU-style diffs of changed
files and properties." Note that this does not talk about directories,
nor about parsing.
Michael Diers has mentioned to me that hook scripts such as enforcer
have trouble parsing the svnlook diff output reliably. It seems like
a much better idea to add a --xml switch to svnlook diff, which should
then be used by hooks which want to parse diff information. The XML
would contain the same of even more information than the human readable
output. Anything hook scripts out there want to know about changes made
in a transaction should be represented in the XML.
> I don't think there's a need for an Added or Deleted header because
> those are isolated actions that are self contained in the one path.
> Copied is different because it relates to more than one path. Knowing
> the destination only gives you a piece of the puzzle.
Well, you may not need information about added or deleted directories.
But I think this depends on what you want to know.
svnlook diff should be able to provide sufficient information for
any hook script out there.
Stefan
Re: [PATCH] issue #3292: fix for 'svnlook diff' regression: doesn't
print added dirs
Posted by Lamar Goddard <la...@gmail.com>.
Oh, I don't think my reply on the other thread made it to the dev
list, hopefully this one will.
On Wed, Jun 9, 2010 at 1:11 AM, Stefan Sperling <st...@elego.de> wrote:
> On Fri, Jun 04, 2010 at 06:02:52PM -0700, Lamar Goddard wrote:
>> [[[
>> Fixes issue #3292: Output svnlook diff content for copied/moved directories
>>
>> * subversion/svnlook/main.c
>> (print_diff_tree): If diff isn't outputted but header has content,
>> output the header.
>> ]]]
>
> Hi Lamar,
>
> thanks for the patch. It is perferectly fine technically,
> but I'm unsure about how we (Subversion developers) want 'svnlook diff'
> to behave.
>
> svnlook diff in 1.4 behaves differently than svn diff does in trunk.
> svn diff never shows anything for directories (copied or not), it only shows
> diffs for files. Do we want svn diff and svnlook diff behave the same way?
> If so, maybe the 1.5/1.6 behaviour is correct and the 1.4 behaviour was wrong?
My 2¢:
I'm not sure svnlook diff should match svn diff's behavior.
Re: [PATCH] issue #3292: fix for 'svnlook diff' regression: doesn't
print added dirs
Posted by Lamar Goddard <la...@gmail.com>.
Oh, I don't think my reply on the other thread made it to the dev
list, hopefully this one will.
On Wed, Jun 9, 2010 at 1:11 AM, Stefan Sperling <st...@elego.de> wrote:
> On Fri, Jun 04, 2010 at 06:02:52PM -0700, Lamar Goddard wrote:
>> [[[
>> Fixes issue #3292: Output svnlook diff content for copied/moved directories
>>
>> * subversion/svnlook/main.c
>> (print_diff_tree): If diff isn't outputted but header has content,
>> output the header.
>> ]]]
>
> Hi Lamar,
>
> thanks for the patch. It is perferectly fine technically,
> but I'm unsure about how we (Subversion developers) want 'svnlook diff'
> to behave.
>
> svnlook diff in 1.4 behaves differently than svn diff does in trunk.
> svn diff never shows anything for directories (copied or not), it only shows
> diffs for files. Do we want svn diff and svnlook diff behave the same way?
> If so, maybe the 1.5/1.6 behaviour is correct and the 1.4 behaviour was wrong?
My 2¢:
I'm not sure svnlook diff should match svn diff's behavior.
>From what I understand there's no equivalent svnlook command to svn
log -v or svn info so there's currently no way for a well behaved hook
script to determine where a directory copy originated. svn log -v
provides that information for revisions in the repository and svn info
does the same for working copy changes.
> Assuming we do want to show a Copied: header for copied directories in
> svnlook diff, why aren't we also showing an Added: header for added
> directories, and a Deleted: header for deleted directories?
I don't think there's a need for an Added or Deleted header because
those are isolated actions that are self contained in the one path.
Copied is different because it relates to more than one path. Knowing
the destination only gives you a piece of the puzzle. It's the
difference in the output between (current):
Property changed on: branches/foo
___________________________________________________________________
Added: svn:mergeinfo
+ /branches/bar:1-10
and (patched):
Copied: branches/foo (from rev 20,
branches/random_path/that_you_will/never_guess)
Property changed on: branches/foo
___________________________________________________________________
Added: svn:mergeinfo
+ /branches/bar:1-10
In the first case you only know the destination but in the second you
also know the source. Diff may not be the correct function but it's
where this information used to be provided for directories and still
is for files.
--
Lamar Goddard
lamargoddard@gmail.com
Re: [PATCH] issue #3292: fix for 'svnlook diff' regression:
doesn't print added dirs
Posted by Stefan Sperling <st...@elego.de>.
On Fri, Jun 04, 2010 at 06:02:52PM -0700, Lamar Goddard wrote:
> [[[
> Fixes issue #3292: Output svnlook diff content for copied/moved directories
>
> * subversion/svnlook/main.c
> (print_diff_tree): If diff isn't outputted but header has content,
> output the header.
> ]]]
Hi Lamar,
thanks for the patch. It is perferectly fine technically,
but I'm unsure about how we (Subversion developers) want 'svnlook diff'
to behave.
svnlook diff in 1.4 behaves differently than svn diff does in trunk.
svn diff never shows anything for directories (copied or not), it only shows
diffs for files. Do we want svn diff and svnlook diff behave the same way?
If so, maybe the 1.5/1.6 behaviour is correct and the 1.4 behaviour was wrong?
Assuming we do want to show a Copied: header for copied directories in
svnlook diff, why aren't we also showing an Added: header for added
directories, and a Deleted: header for deleted directories?
Stefan
> Index: subversion/svnlook/main.c
> ===================================================================
> --- subversion/svnlook/main.c (revision 951625)
> +++ subversion/svnlook/main.c (working copy)
> @@ -1087,6 +1087,11 @@
> }
> SVN_ERR(svn_cmdline_fflush(stdout));
> }
> + else if (header->len != 0)
> + {
> + /* Print diff header. */
> + SVN_ERR(svn_cmdline_printf(pool, "%s", header->data));
> + }
>
> /* Make sure we delete any temporary files. */
> if (orig_path)
>
>
>
> --
> Lamar Goddard
> lamargoddard@gmail.com
Re: [PATCH] issue #3292: fix for 'svnlook diff' regression: doesn't
print added dirs
Posted by Senthil Kumaran S <se...@collab.net>.
Lamar Goddard wrote:
> [[[
> Fixes issue #3292: Output svnlook diff content for copied/moved directories
>
> * subversion/svnlook/main.c
> (print_diff_tree): If diff isn't outputted but header has content,
> output the header.
> ]]]
The fix for this issue is already available via r878532.
Thank You.
--
Senthil Kumaran S
http://www.stylesen.org/