Re: svn commit: r28567 - in branches/tree-conflicts/subversion: include libsvn_wc svn tests/cmdline

On Wed, Dec 19, 2007 at 11:04:52PM +0100, Charles Acknin wrote:
> On Dec 19, 2007 6:44 PM, <stsp@tigris.org> wrote:
> > @@ -1426,6 +1428,12 @@
> > err = svn_utf_cstring_to_utf8(&path_utf8, opt_arg, pool);
> > opt_state.from_source = svn_path_canonicalize(path_utf8, pool);
> > break;
> > + case 't':
> > + opt_state.show_tree_conflicts = TRUE;
> > + /* '-t' implies '-v', otherwise svn_wc_client3() won't return
> > + the entries we need. This hack isn't user-visible. */
> > + opt_state.verbose = TRUE;
> > + break;
>
> Shouldn't we let the user know that '-t' implies '-v'? Because in the
> end, he's going to see the extra stuff that '-v' prints, right?

No, user's won't see it, because we also suppress the normal -v output
if -t is given. -t overrides -v if both are given.

I've been thinking about changing this bit of code so it is less ambiguous.
That comment and the line below it should be removed.

But first let me explain what the problem is:

The real reason behind this is the semantics of svn_client_status3().
(Ooops, for some stupid reason the comment I put in there talks about
"svn_wc_client3" which does not even exist :-/ )

Anyway, here's that function's signature for reference:

svn_error_t *
svn_client_status3(svn_revnum_t *result_rev,
                   const char *path,
                   const svn_opt_revision_t *revision,
                   svn_wc_status_func2_t status_func,
                   void *status_baton,
                   svn_depth_t depth,
                   svn_boolean_t get_all,
                   svn_boolean_t update,
                   svn_boolean_t no_ignore,
                   svn_boolean_t ignore_externals,
                   svn_client_ctx_t *ctx,
                   apr_pool_t *pool);

Interesting piece of docstring:

 * - If @a get_all is set, retrieve all entries; otherwise,
 * retrieve only "interesting" entries (local mods and/or
 * out of date).
 *

If get_all is not set, we may not get any entries we are interested in,
namely the ones that contain tree conflict information in the this_dir
entry. So our definition of "interesting" is different to that of
svn_client_status3().

If you pass -v to svn status, get_all ends up being true.
See do_status() in subversion/svn/status-cmd.c where it's called:

  SVN_ERR(svn_client_status3(&repos_rev, target, rev,
                             print_status, status_baton,
                             opt_state->depth,
              get_all ---> opt_state->verbose,
                             opt_state->update,
                             opt_state->no_ignore,
                             opt_state->ignore_externals,
                             ctx, pool));

So internally, -v maps to "get me all entries no matter what".

The actual printing is done in a function called print_status().

There are two static functions both called print_status, one
in subversion/svn/status-cmd.c and one in subversion/svn/status.c,
which makes trying to follow the call chain a bit confusing at first.

For ease of reference, let's call the one in subversion/svn/status-cmd.c
print_status1() and the one in subversion/svn/status.c print_status2().

The status_func passed to svn_client_status3() above is print_status1().
It is invoked as a callback on each path that svn_client_status3() comes
across.

print_status1() calls print_status_normal_or_xml() which calls
svn_cl__print_status() which in turn calls print_status2().

This latter function does the actual printing, so this is where we
end up getting entries to examine for tree conflict data. The function
gets called per entry, so if get_all isn't set and the entry is not
"interesting" as per the definition of "interesting" in the
svn_client_status3 doc string, the function is not called at all!

That's why we need get_all set to true to be able to print tree
conflict info, and why I made "-t imply -v" even though this does
not extend to the actual output.

Regarding the output:

Because print_status2() is invoked for each path sperately, there is
no easy way of not getting in the way of the normal "svn status -v"
output with tree conflict info.

Let me illustrate:

This update produces a tree conflict on the directory A/D/G:

    stsp@ted [update_tests-44.2] $ svn up
    D A/D/G/rho
    D A/D/G/tau
    A A/D/G/rhino
    A A/D/G/tapir
    U A/D/G/pi
    C A/D/G
    Updated to revision 2.

svn status -v looks like this:

    stsp@ted [update_tests-44.2] $ svn st -v A/D/G
    ? 2 2 jrandom A/D/G
    ? A/D/G/rho
    A + - ? ? A/D/G/pig
                    2 2 jrandom A/D/G/rhino
                    2 2 jrandom A/D/G/tapir
    D 2 2 jrandom A/D/G/pi
    A + - ? ? A/D/G/tiger

svn status -t currently looks like this:

    stsp@ted [update_tests-44.2] $ svn st -t A/D/G
    ==== Tree conflicts in 'A/D/G' ===
    The update wants to delete the file 'rho'
    (possibly as part of a rename operation).
    You have edited 'rho' locally.
    
    The update wants to delete the file 'tau'
    (possibly as part of a rename operation).
    You have deleted 'tau' locally.
    Maybe you renamed it?
    
    The update wants to edit the file 'pi'.
    You have deleted 'pi' locally.
    Maybe you renamed it?

If -t implied -v _output_, it would have to look somewhat
like this (mockup):

    stsp@ted [update_tests-44.2] $ svn st -v -t A/D/G
    ? 2 2 jrandom A/D/G

    ==== Tree conflicts in 'A/D/G' ===
    The update wants to delete the file 'rho'
    (possibly as part of a rename operation).
    You have edited 'rho' locally.
    
    The update wants to delete the file 'tau'
    (possibly as part of a rename operation).
    You have deleted 'tau' locally.
    Maybe you renamed it?
    
    The update wants to edit the file 'pi'.
    You have deleted 'pi' locally.
    Maybe you renamed it?
    
    ? A/D/G/rho
    A + - ? ? A/D/G/pig
                    2 2 jrandom A/D/G/rhino
                    2 2 jrandom A/D/G/tapir
    D 2 2 jrandom A/D/G/pi
    A + - ? ? A/D/G/tiger

I think this looks ugly, and it would confuse scripts that
depend on the output of status -v and pass -t by accident.

status -t is really only meant for human consumption.

For tools, there is an API to get at the same information.
If shell scripts really need the data in a parsable format,
we can add a flag to svn status for dumping the raw tree conflict
data in the entries file to stdout, which is easily parsable.

So how can we get rid of the misleading comment you quoted?

A better solution may be to simply not make "-t" imply "-v"
(which is wrong anyway from the UI perspective) and call
svn_client_status3() like this instead:

  SVN_ERR(svn_client_status3(&repos_rev, target, rev,
                             print_status, status_baton,
                             opt_state->depth,
           get_all ----> (opt_state->verbose
  get_all continued ---> || opt_state->tree_conflicted),
                             opt_state->update,
                             opt_state->no_ignore,
                             opt_state->ignore_externals,
                             ctx, pool));

We could then remove the misleading comment in main.c
that caused this whole mail in the first place :)

> I similarly fixed 'svn di --svnpatch' to imply --no-diff-deleted
> earlier today, and I thought it was fair to comment it in 'svn di -h'
> too.

It might have been appropriate for that situation, but I guess
this is a different case.

> Charles

Thanks for your feedback :)

-- 
Stefan Sperling <stsp@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