Karl Fogel wrote on Tue, Jan 26, 2021 at 22:18:25 -0600:
> If it passes the test suite, and you don't see any obvious further to the
> patch, I'll commit. Thanks again for your help, Daniel!
You're welcome. I see only a few more nits; see below. (Some of them I
noticed in the first iteration too, but I didn't want to pick too many
nits then.) I'm happy that the patch is correct and committable, though
it won't make the "Major features in this minor release" shortlist at
the top of CHANGES.
> [[[
> Distinguish between regular properties and revprops in tmpfile name.
>
> * subversion/svn/propedit-cmd.c (svn_cl__propedit): In the revprop
> case, create a tmpfile name that indicates that a revprop is
> being edited and on which revision.
I'd indent that differently, as follows:
* subversion/svn/propedit-cmd.c
(svn_cl__propedit): In the revprop case, create a tmpfile name that indicates
that a revprop is being edited and on which revision. Lorem ipsum dolor
sit amet.
Namely, put the symbol name on a new line, and indent continuation lines
differently to other kinds of lines — both for readability.
> +++ subversion/svn/propedit-cmd.c (working copy)
> @@ -143,7 +143,9 @@ svn_cl__propedit(apr_getopt_t *os,
> SVN_ERR(svn_cmdline__edit_string_externally(
> &propval, NULL,
> opt_state->editor_cmd, temp_dir,
> - propval, "svn-prop",
> + propval,
> + apr_psprintf(pool, "svn-revprop-r%ld",
> + (opt_state->start_revision.value.number)),
The parentheses are superfluous, so future readers are bound to wonder whether
some function call had gotten lost or something. Suggest to drop them.
> ctx->config,
> svn_prop_needs_translation(pname),
> opt_state->encoding, pool));
> +++ subversion/tests/cmdline/prop_tests.py (working copy)
> @@ -2829,6 +2829,38 @@ def prop_conflict_root(sbox):
> expected_status,
> extra_files=extra_files)
>
> +
> +# Test that editing a regular property results in a temporary file
> +# based on the name "svn-prop" but editing a revprop results in a
> +# temporary file based on the name "svn-revprop-rN" (where "N" is
> +# the number of the revision whose revprop would be edited).
'A file based on the name "foo"' doesn't parse well for me. If that isn't
a dialect difference, then perhaps 'A file named foo.tmp*'.
> +def tmpfile_name_matches_prop_type(sbox):
> + "propedit tmpfile name matches property type"
> +
> + sbox.build(read_only=True)
> +
> + # We want the editor invocation to fail -- all we care about is the
> + # name of the tmpfile left over after that failure. I'm guessing
> + # you don't have a editor named this on your system:
> + nonexistent_editor = 'af968da2ce9'
> +
> + svntest.actions.run_and_verify_svn(
> + None,
> + '.*' + nonexistent_editor + r'.*svn-revprop-r1\.tmp.*',
re.escape(), please? Mainly so if someone copies this function call into
a future test and adapts it for its new home, it'll be correct.
> + 'propedit', '--revprop',
> + '--editor-cmd', nonexistent_editor,
> + '-r1', 'svn:log',
> + sbox.repo_url)
>
The message I'm replying to and your message dated "Tue, 26 Jan 2021
22:23:52 -0600" were threaded at the same level (sibling nodes in my
MUA's tree display), even though the latter quoted the former.
I haven't investigated.
Cheers,
Daniel
