Re: [PATCH] svn diff -cHEAD

Hyrum K. Wright wrote:
> 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.

Issue #3041.

-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;
>> ]]]
>
>