Mailing-List: contact commits-help@subversion.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@subversion.apache.org
Received-SPF: pass (nike.apache.org: local policy)
Date: Sat, 27 Apr 2013 18:01:59 +0300
From: Daniel Shahaf <danielsh@elego.de>
To: Bert Huijben <bert@qqmail.nl>
Cc: dev@subversion.apache.org, commits@subversion.apache.org
Subject: [PATCH] Re: svn commit: r1476563 -
	/subversion/trunk/subversion/svnserve/serve.c
Message-ID: <20130427150159.GK3055@lp-shahaf.local>
References: <20130427122844.D3E1123889FA@eris.apache.org>
 <20130427143603.GH3055@lp-shahaf.local>
 <008101ce4355$973c84d0$c5b58e70$@qqmail.nl>
 <20130427144642.GJ3055@lp-shahaf.local>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20130427144642.GJ3055@lp-shahaf.local>
User-Agent: Mutt/1.5.18 (2008-05-17)

Daniel Shahaf wrote on Sat, Apr 27, 2013 at 17:46:42 +0300:
> Bert Huijben wrote on Sat, Apr 27, 2013 at 16:43:29 +0200:
> > (Another option would be to start using our tristate enum for this case...
> > But we made this svnserve<->libsvn_ra_svn api public following our old
> > inter-library rules)
> 
> +1
> 
> And svn_ra_svn__parse_number is now a private API, the public
> counterpart is deprecated (hat tip to stefan2 for the legwork).


[[[
Allow ra_svn protocol parsing to use svn_tristate_t instead of
a non-robust combination of TRUE, FALSE, and a custom macro.

This commit reverts r1476563, which introduced a bug (see r1466659).

* subversion/include/private/svn_ra_svn_private.h
  (svn_ra_svn__parse_tuple): Add '3' format code, like 'B'.

* subversion/libsvn_ra_svn/marshal.c
  (svn_ra_svn__parse_tuple): Ditto.

* subversion/svnserve/serve.c
  (update, switch_cmd): Use svn_tristate_t, revert r1476563.
]]]

[[[
Index: subversion/include/private/svn_ra_svn_private.h
===================================================================
--- subversion/include/private/svn_ra_svn_private.h	(revision 1476596)
+++ subversion/include/private/svn_ra_svn_private.h	(working copy)
@@ -186,6 +186,7 @@ svn_ra_svn__skip_leading_garbage(svn_ra_svn_conn_t
      w     const char **          Word
      b     svn_boolean_t *        Word ("true" or "false")
      B     apr_uint64_t *         Word ("true" or "false")
+     3     svn_tristate_t *       Word ("true" or "false")
      l     apr_array_header_t **  List
      (                            Begin tuple
      )                            End tuple
@@ -197,7 +198,10 @@ svn_ra_svn__skip_leading_garbage(svn_ra_svn_conn_t
  * contains two elements, an error will result.
  *
  * 'B' is similar to 'b', but may be used in the optional tuple specification.
- * It returns TRUE, FALSE, or SVN_RA_SVN_UNSPECIFIED_NUMBER.
+ * It returns TRUE, FALSE, or SVN_RA_SVN_UNSPECIFIED_NUMBER.  This format
+ * code is deprecated; new code should use '3' instead.
+ *
+ * '3' is like 'B', mutatis mutandis.
  *
  * If an optional part of a tuple contains no data, 'r' values will be
  * set to @c SVN_INVALID_REVNUM, 'n' and 'B' values will be set to
Index: subversion/libsvn_ra_svn/marshal.c
===================================================================
--- subversion/libsvn_ra_svn/marshal.c	(revision 1476596)
+++ subversion/libsvn_ra_svn/marshal.c	(working copy)
@@ -1230,6 +1230,15 @@ static svn_error_t *vparse_tuple(const apr_array_h
           else
             break;
         }
+      else if (**fmt == '3' && elt->kind == SVN_RA_SVN_WORD)
+        {
+          if (strcmp(elt->u.word, "true") == 0)
+            *va_arg(*ap, svn_tristate_t *) = svn_tristate_true;
+          else if (strcmp(elt->u.word, "false") == 0)
+            *va_arg(*ap, svn_tristate_t *) = svn_tristate_false;
+          else
+            break;
+        }
       else if (**fmt == 'l' && elt->kind == SVN_RA_SVN_LIST)
         *va_arg(*ap, apr_array_header_t **) = elt->u.list;
       else if (**fmt == '(' && elt->kind == SVN_RA_SVN_LIST)
@@ -1267,6 +1276,9 @@ static svn_error_t *vparse_tuple(const apr_array_h
             case 'B':
             case 'n':
               *va_arg(*ap, apr_uint64_t *) = SVN_RA_SVN_UNSPECIFIED_NUMBER;
+              break;
+            case '3':
+              *va_arg(*ap, svn_tristate_t *) = svn_tristate_unknown;
               break;
             case '(':
               nesting_level++;
Index: subversion/svnserve/serve.c
===================================================================
--- subversion/svnserve/serve.c	(revision 1476596)
+++ subversion/svnserve/serve.c	(working copy)
@@ -1815,8 +1815,8 @@ static svn_error_t *update(svn_ra_svn_conn_t *conn
   svn_revnum_t rev;
   const char *target, *full_path, *depth_word;
   svn_boolean_t recurse;
-  apr_uint64_t send_copyfrom_args; /* Optional; default FALSE */
-  apr_uint64_t ignore_ancestry; /* Optional; default FALSE */
+  svn_tristate_t send_copyfrom_args; /* Optional; default FALSE */
+  svn_tristate_t ignore_ancestry; /* Optional; default FALSE */
   /* Default to unknown.  Old clients won't send depth, but we'll
      handle that by converting recurse if necessary. */
   svn_depth_t depth = svn_depth_unknown;
@@ -1823,7 +1823,7 @@ static svn_error_t *update(svn_ra_svn_conn_t *conn
   svn_boolean_t is_checkout;
 
   /* Parse the arguments. */
-  SVN_ERR(svn_ra_svn__parse_tuple(params, pool, "(?r)cb?wB?B", &rev, &target,
+  SVN_ERR(svn_ra_svn__parse_tuple(params, pool, "(?r)cb?w3?3", &rev, &target,
                                   &recurse, &depth_word,
                                   &send_copyfrom_args, &ignore_ancestry));
   target = svn_relpath_canonicalize(target, pool);
@@ -1843,8 +1843,8 @@ static svn_error_t *update(svn_ra_svn_conn_t *conn
   SVN_ERR(accept_report(&is_checkout, NULL,
                         conn, pool, b, rev, target, NULL, TRUE,
                         depth,
-                        (send_copyfrom_args != FALSE), /* send_copyfrom_args */
-                        (ignore_ancestry != FALSE)));  /* ignore_ancestry */
+                        (send_copyfrom_args == svn_tristate_true),
+                        (ignore_ancestry == svn_tristate_true)));
   if (is_checkout)
     {
       SVN_ERR(log_command(b, conn, pool, "%s",
@@ -1855,7 +1855,8 @@ static svn_error_t *update(svn_ra_svn_conn_t *conn
     {
       SVN_ERR(log_command(b, conn, pool, "%s",
                           svn_log__update(full_path, rev, depth,
-                                          (send_copyfrom_args != FALSE),
+                                          (send_copyfrom_args
+                                           == svn_tristate_true),
                                           pool)));
     }
 
@@ -1873,11 +1874,11 @@ static svn_error_t *switch_cmd(svn_ra_svn_conn_t *
   /* Default to unknown.  Old clients won't send depth, but we'll
      handle that by converting recurse if necessary. */
   svn_depth_t depth = svn_depth_unknown;
-  apr_uint64_t send_copyfrom_args; /* Optional; default FALSE */
-  apr_uint64_t ignore_ancestry; /* Optional; default TRUE */
+  svn_tristate_t send_copyfrom_args; /* Optional; default FALSE */
+  svn_tristate_t ignore_ancestry; /* Optional; default TRUE */
 
   /* Parse the arguments. */
-  SVN_ERR(svn_ra_svn__parse_tuple(params, pool, "(?r)cbc?w?BB", &rev, &target,
+  SVN_ERR(svn_ra_svn__parse_tuple(params, pool, "(?r)cbc?w?33", &rev, &target,
                                   &recurse, &switch_url, &depth_word,
                                   &send_copyfrom_args, &ignore_ancestry));
   target = svn_relpath_canonicalize(target, pool);
@@ -1906,8 +1907,8 @@ static svn_error_t *switch_cmd(svn_ra_svn_conn_t *
   return accept_report(NULL, NULL,
                        conn, pool, b, rev, target, switch_path, TRUE,
                        depth,
-                       (send_copyfrom_args != FALSE) /* send_copyfrom_args */,
-                       (ignore_ancestry != FALSE) /* ignore_ancestry */);
+                       (send_changed_paths == svn_tristate_true),
+                       (ignore_ancestry != svn_tristate_false));
 }
 
 static svn_error_t *status(svn_ra_svn_conn_t *conn, apr_pool_t *pool,
]]]