On 3/28/07, Peter Lundblad <plundblad@google.com> wrote:
> Charles Acknin writes:
> > Daniel and I had a little talk on #svn-dev about something that seemed
> > a bit weird in libsvn_wc/diff.c I noticed as running within gdb while
> > writing my GSoC proposal: for single-file diff on WC/WC (e.g. svn
> > diff foo), directory_elements_diff() would keep looping (line 692, rev
> > 24199) until all files of foo's directory have been processed instead
> > of breaking when foo is done. Those few lines will ensure we 'break'
> > when files passed as argument are processed.
> >
> If I understand what you're saying correctly, this is not a correctness
> bug, but just a minor performance glitch?
>
Well, about the nature of that 'thing', Daniel called it a bug while I
never mentioned it as such. I might have not been clear : )
> > [[[
> > Prevent from some useless processing when performing single-file diff on WC/WC.
> >
> Seems like that;)
>
>
> > * subversion/libsvn_wc/diff.c
> > (directory_elements_diff): add a boolean so that we know when the
> > file passed as argument is diffed
> > ]]]
> >
> >
> >
> > --- subversion/libsvn_wc/diff.c (revision 24199)
> > +++ subversion/libsvn_wc/diff.c (working copy)
> > @@ -697,6 +697,7 @@
> > const svn_wc_entry_t *entry;
> > struct dir_baton *subdir_baton;
> > const char *name, *path;
> > + svn_boolean_t file_diffed = FALSE;
> >
> > svn_pool_clear(subpool);
> >
> > @@ -726,6 +727,7 @@
> > {
> > case svn_node_file:
> > SVN_ERR(file_diff(dir_baton, path, entry, subpool));
> > + file_diffed = TRUE;
> > break;
> >
>
> It looks like this *always* breaks out of the loop after the first file.
> Is this correct when the WC have more than one changed file/directory?
>
Yes, actually directory_elements_diff() gets called for each file
that's passed on the CLI. The loop it gets called from is in
subversion/svn/diff-cmd.c:245
for (i = 0; i < targets->nelts; ++i)
I mean directory_elements_diff() seems like a single-target-function.
> > case svn_node_dir:
> > @@ -756,6 +758,9 @@
> > default:
> > break;
> > }
> > +
> > + if (file_diffed)
> > + break;
> > }
> >
> > svn_pool_destroy(subpool);
> >
>
> I guess the useless work you're after happens because the anchor and target
> are different and we're still looping over all entries in the anchor
> directory.
The useless work happens because we keep going as the file
directory_elements_diff() was called for is processed, when returning
from file_diff(). If the above assumption I raised is right, why not
break when the -- only -- target is file_diff()'ed?
> If you think that's a problem, maybe we should just get the entry
> we want in this case. If you can find a way to do that without extra code
> duplication added, then it might be worth it, but I doubt you can measure
> any performance noticable difference in a reasonably sized directory.
Yes it would be right to go straight with the target. And again, if
this function is single-target, I can't see any reason to loop over
all the 'entries' until it matches dir_baton->edit_baton->target. Or
have I missed something?
> Hint: Running "make check" before submitting a patch helps make sure it
> doesn't break anything;)
I did run the first 20 tests before sending as I saw #3
'diff-diff3-test' was called and returned on success but I didn't wait
until #30 that I'm now just realising it failed. Are we supposed to
run them all? It takes a bit long... : /
I'm going to start investigating from the 9 places it failed as in the
log. Or is it not worth a look at such a small performance issue?
Cheers,
Charles
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Mar 28 21:47:25 2007