Re: [PATCH] svn diff -cHEAD

Charles Acknin wrote:
> Last night I bumped into this:
>
> % svn diff -cHEAD
> subversion/svn/main.c:1103: (apr_err=205000)
> svn: Non-numeric change argument given to -c
>
> It looked like a trivial fix but it turned out to be quite tricky as
> Subversion doesn't have any revision keyword for 'HEAD-1'.
>
> It doesn't break anything, i.e. passes all diff_tests.
>
> (Warning: this is hacky-and-dirty. I'm lacking time to polish this
> off as I'm late with my SoC. Additionally one can find the time to
> implement -c -HEAD too.)

Ping... Has anybody had a chance to look at this patch? If there
aren't any responses, I'll file and issue in a few days.

-Hyrum

> [[[
> * subversion/svn/main.c
> (main): use svn_opt_parse_revision() instead of strtol() to read '-c''s
> revision argument (numeric or HEAD) and adapt to support HEAD argument.
>
> * subversion/libsvn_subr/opt.c
> (parse_one_rev): add a case for negative numbers as 'diff -c N' is now using
> this function.
>
> * subversion/libsvn_client/diff.c
> (diff_prepare_repos_repos): set revisions accordingly to our hack/hint from
> main() and save one roundtrip.
> ]]]
>
> [[[
> Index: subversion/svn/main.c
> ===================================================================
> --- subversion/svn/main.c (revision 25966)
> +++ subversion/svn/main.c (working copy)
> @@ -1082,8 +1082,6 @@
> break;
> case 'c':
> {
> - char *end;
> - svn_revnum_t changeno;
> if (opt_state.start_revision.kind != svn_opt_revision_unspecified)
> {
> err = svn_error_create
> @@ -1099,35 +1097,71 @@
> _("Can't specify -c with --old"));
> return svn_cmdline_handle_exit_error(err, pool, "svn: ");
> }
> - changeno = strtol(opt_arg, &end, 10);
> - if (end == opt_arg || *end != '\0')
> + if (svn_opt_parse_revision(&(opt_state.start_revision),
> + &(opt_state.end_revision),
> + opt_arg, pool) != 0)
> {
> - err = svn_error_create(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
> - _("Non-numeric change argument
> given to -c"));
> + err = svn_utf_cstring_to_utf8(&utf8_opt_arg, opt_arg, pool);
> + if (! err)
> + err = svn_error_createf
> + (SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
> + _("Syntax error in revision argument '%s'"),
> + utf8_opt_arg);
> return svn_cmdline_handle_exit_error(err, pool, "svn: ");
> }
> - if (changeno == 0)
> + if (opt_state.end_revision.kind != svn_opt_revision_unspecified)
> {
> - err = svn_error_create(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
> - _("There is no change 0"));
> + err = svn_error_create
> + (SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
> + _("Multiple revision arguments encountered; "
> + "can't specify N:M range with -c, "
> + "try '-r N:M' instead"));
> return svn_cmdline_handle_exit_error(err, pool, "svn: ");
> }
> - /* Figure out the range:
> - -c N -> -r N-1:N
> - -c -N -> -r N:N-1 */
> - if (changeno > 0)
> + if (opt_state.start_revision.kind == svn_opt_revision_number)
> {
> - opt_state.start_revision.value.number = changeno - 1;
> - opt_state.end_revision.value.number = changeno;
> + svn_revnum_t *change_rev
> + = &opt_state.start_revision.value.number;
> +
> + /* Figure out the range:
> + -c N -> -r N-1:N
> + -c -N -> -r N:N-1 */
> + if (*change_rev == 0)
> + {
> + err = svn_error_create(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
> + _("There is no change 0"));
> + return svn_cmdline_handle_exit_error(err, pool, "svn: ");
> + }
> + else if (*change_rev > 0)
> + {
> + opt_state.end_revision.value.number = *change_rev;
> + opt_state.start_revision.value.number = *change_rev - 1;
> + }
> + else
> + {
> + *change_rev = -(*change_rev);
> + opt_state.end_revision.value.number = *change_rev - 1;
> + opt_state.start_revision.value.number = *change_rev;
> + }
> + opt_state.end_revision.kind = svn_opt_revision_number;
> }
> + else if (opt_state.start_revision.kind == svn_opt_revision_head)
> + {
> + /* This is a hack for '-c HEAD': as there's no 'HEAD-1'
> + * rev-keyword, set both {start,end}_revision.kind to HEAD
> + * and their revision value to a dummy so we can
> + * differentiate from a true HEAD/HEAD repos-diff later on. */
> + opt_state.end_revision.kind = svn_opt_revision_head;
> + opt_state.end_revision.value.number = SVN_IGNORED_REVNUM;
> + opt_state.start_revision.value.number = SVN_IGNORED_REVNUM;
> + }
> else
> {
> - changeno = -changeno;
> - opt_state.start_revision.value.number = changeno;
> - opt_state.end_revision.value.number = changeno - 1;
> + err = svn_error_create
> + (SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
> + _("-c argument must either be numeric or HEAD"));
> + return svn_cmdline_handle_exit_error(err, pool, "svn: ");
> }
> - opt_state.start_revision.kind = svn_opt_revision_number;
> - opt_state.end_revision.kind = svn_opt_revision_number;
> used_change_arg = TRUE;
> }
> break;
> Index: subversion/libsvn_client/diff.c
> ===================================================================
> --- subversion/libsvn_client/diff.c (revision 25966)
> +++ subversion/libsvn_client/diff.c (working copy)
> @@ -1022,11 +1022,28 @@
> _("'%s' was not found in the repository at revision %ld"),
> drr->url2, drr->rev2);
>
> - /* Do the same for the first target. */
> SVN_ERR(svn_ra_reparent(ra_session, drr->url1, pool));
> - SVN_ERR(svn_client__get_revision_number
> - (&drr->rev1, ra_session, params->revision1,
> - (params->path1 == drr->url1) ? NULL : params->path1, pool));
> +
> + /* When the two revisions are both HEAD, avoid a useless second
> + * roundtrip to the server and copy the value from rev2 instead. */
> + if (params->revision1->kind == svn_opt_revision_head
> + && params->revision2->kind == params->revision1->kind)
> + {
> + /* This is a hack to get 'svn diff -cHEAD' to work. */
> + if (params->revision1->value.number == SVN_IGNORED_REVNUM
> + && params->revision2->value.number == SVN_IGNORED_REVNUM)
> + drr->rev1 = drr->rev2 - 1;
> + else
> + drr->rev1 = drr->rev2;
> + }
> + else
> + {
> + /* Do the same for the first target. */
> + SVN_ERR(svn_client__get_revision_number
> + (&drr->rev1, ra_session, params->revision1,
> + (params->path1 == drr->url1) ? NULL : params->path1, pool));
> + }
> +
> SVN_ERR(svn_ra_check_path(ra_session, "", drr->rev1, &kind1, pool));
> if (kind1 == svn_node_none)
> return svn_error_createf
> Index: subversion/libsvn_subr/opt.c
> ===================================================================
> --- subversion/libsvn_subr/opt.c (revision 25966)
> +++ subversion/libsvn_subr/opt.c (working copy)
> @@ -567,6 +567,19 @@
> *end = save;
> return end;
> }
> + else if ((*str == '-' && apr_isdigit(*(str + 1))))
> + {
> + /* It's a negative number. */
> + end = str + 2;
> + while (apr_isdigit(*end))
> + end++;
> + save = *end;
> + *end = '\0';
> + revision->kind = svn_opt_revision_number;
> + revision->value.number = SVN_STR_TO_REV(str);
> + *end = save;
> + return end;
> + }
> else if (apr_isalpha(*str))
> {
> end = str + 1;
> ]]]