You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by "Hyrum K. Wright" <hy...@mail.utexas.edu> on 2007/10/22 21:17:25 UTC
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;
> ]]]
Re: [PATCH] svn diff -cHEAD
Posted by Charles Acknin <ch...@gmail.com>.
On Dec 10, 2007 11:48 PM, Hyrum K. Wright <hy...@mail.utexas.edu> wrote:
> 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.
Thanks for keeping track Hyrum. I'll have some spare time hopefully
very soon and will be able to polish it, which should help to check it
in.
Charles
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] svn diff -cHEAD
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
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;
>> ]]]
>
>