RFC: Changing anchor/target split to use "" instead of NULL

Back in the dark ages, before we had nailed down svn_path, we determined
that the structure of working copies demands that all operations be
rooted in a directory, even if the operation applies to a file. So, we
have this function svn_wc_get_actual_target, which splits a path into an
"anchor" and a "target". Examples:

  svn update foodir/barfile --> "foodir"/"barfile"
  svn update foodir --> "foodir"/NULL
  svn update --> ""/NULL

This discipline is also reflected in the tree editor (you can only
open_root a directory, not a file), since tree edits are often meant to
be applied to working copies. Because of that, svn_repos_dir_delta()
accepts separate src_parent_dir (read: anchor) and src_entry (read:
target) parameters. The four wc-updating ra_lib functions (do_update,
do_switch, do_status, do_diff) also accept a target parameter which is
expected to be NULL for operations on directories.

In issue 999, I wrote that target should probably be "", not NULL, for
directory operations. In many other contexts we use the empty path,
rather than NULL, to mean "right here". I have composed a patch
(attached) to make this change, and am looking for comments. There is
no way I will apply this change to the trunk if there isn't agreement
that it should be in 1.0, since the trunk should not start out life with
a subtle and permanent API change from the 1.0 branch.

Specifically, I'm looking for comments like:

  * You think this is wrong, that we really should be using NULL for
    the target, because this is different from other situations
    where we use a path element of "right here."

  * You think this is right (or at least debatable), but you would
    rather live with this API wart for a very long time than apply
    such a potentially destabilizing patch to the 1.0 branch.

  * You think this is right, and should make it into 1.0.

As an aside, I am developing a distinct antipathy towards
svn_path_is_empty(), because it is verbose and confusing (confusing
because it's a test for a negative, rather than for a positive). The
attached patch uses it all over the place, because I didn't feel that it
was right to ignore an svn_path abstraction, but I would much rather
write "if (*target)" than "if (! svn_path_is_empty (target))". I'd like
to solicit opinions on that point as well. Should I nuke it from
orbit? (We use it in 21 places right now.) Should I leave it alone but
feel free to ignore it? Or should I grit my teeth and use it?

Thanks.

For consistency, use "" instead of NULL for a no-op target.

* libsvn_wc/update_editor.c, include/svn_wc.h (svn_wc_get_actual_target):
   Set target to empty (not NULL) when anchor is subject.
* include/svn_repos.h (svn_repos_begin_report, svn_repos_dir_delta),
  include/svn_wc.h (svn_wc_get_status_editor, svn_wc_get_update_editor,
                    svn_wc_get_switch_editor),
  include/svn_ra.h (do_update, do_switch, do_status, do_diff):
   Expect target to be empty (not NULL) when anchor is subject.
* libsvn_wc/diff.c (directory_elements_diff),
  libsvn_wc/status.c (delete_entry, close_directory):
  libsvn_wc/update_editor.c (make_dir_baton, complete_directory,
                             open_root, close_edit),
  libsvn_client/diff.c (diff_wc_wc, diff_repos_wc),
  libsvn_client/commit.c (adjust_rel_targets, svn_client_commit),
  mod_dav_svn (dav_svn__update_report),
  clients/cmdline/commit-cmd.c (svn_cl__commit),
  libsvn_repos/delta.c (svn_repos_dir_delta),
  libsvn_ra_dav/fetch.c (end_element, make_reporter):
   Test for emptiness of target, rather than nullity.
* libsvn_client/switch.c (svn_client_switch),
  libsvn_client/export.c (svn_client_export),
  libsvn_client/diff.c (do_merge, diff_repos_repos, diff_repos_wc),
  mod_dav_svn (dav_svn__update_report),
  tests/libsvn_repos/repos-test.c (dir_deltas):
   Pass empty target (not NULL) when anchor is subject.
* libsvn_wc/diff.c (svn_wc_diff),
  libsvn_wc/update_editor.c (do_entry_deletion),
  mod_dav_svn/update.c (dav_svn__update_report),
  libsvn_ra_dav/fetch.c (start_element):
   Simplify code by assuming that target is not NULL.
* libsvn_ra_svn/client.c (ra_svn_update, ra_svn_switch, ra_svn_status,
                          ra_svn_diff),
  svnserve/serve.c (update, switch_cmd, status, diff):
   Stop translating NULL to "" and back over the wire.

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org