Karl Fogel wrote on Tue, Jan 26, 2021 at 16:11:31 -0600:
> This small patch makes it so that when you do 'svn propedit --revprop', the
> tmpfile's name is "svn-revprop-rN.tmp" (where N is the number of the revision
> whose revprop is being edited).
>
> For regular properties, nothing has changed: the tmpfile name is still just
> "svn-prop.tmp".
>
> This change is useful because many editors display the file's basename during
> editing (e.g., in a status line somewhere near the bottom of the screen). So
> if you get interrupted while editing a revprop, when you come back to your
> editor hours or days later, it's nice to have as many clues as possible as to
> what you were doing :-).
Personally, my $EDITOR exposes a getpid() function, so I can do
«:echo system('ps --forest | grep -B10 ' . getpid())» to see what I was doing.
> Comments welcome. It passes all tests, and I *think* it should be a pretty
> uncontroversial improvement, but I wanted to run it by you all before
> committing since it's been a while (~12 years?) since my most recent
> substantive change to code or test suite.
I suppose the only thing this could break is $EDITOR automation (e.g., syntax
highlighting rules) that's keyed on the tmpfile's name, which should be
acceptable, by the same token that extending the CLI output is acceptable.
Kinda wonder whether there's an easy way to get the property name into the
filename — escaped as needed (property names can contain colons and NTFS
doesn't like those in basenames) — but of course that's the best being the
enemy of the good.
> +++ subversion/svn/propedit-cmd.c (working copy)
> @@ -143,7 +143,9 @@
«svn diff -x-p», please ☺
> +++ subversion/tests/cmdline/prop_tests.py (working copy)
> @@ -2829,6 +2829,33 @@
> 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).
> +def tmpfile_name_matches_prop_type(sbox):
> + "propedit tmpfile name matches property type"
> +
> + sbox.build()
Pass read_only=True. (Reduces test execution time.)
> + # 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 'af968da2ce9' on your system.
> + os.environ['SVN_EDITOR'] = 'af968da2ce9'
This changes global state, which might break something in --parallel mode.
Just pass --editor-cmd.
> + svntest.actions.run_and_verify_svn(None,
> + ".*af968da2ce9.*svn-revprop-r1\\.tmp.*",
style: Consider using «r''» string literals, or character classes rather than
backslash escapes.
> + 'propedit', '--revprop',
> + '-r1', 'svn:log',
> + sbox.repo_url)
> +
> + svntest.actions.run_and_verify_svn(None,
> + ".*af968da2ce9.*svn-prop\\.tmp.*",
> + 'propedit', 'ignored-propname',
> + os.path.join(sbox.wc_dir, 'A', 'mu'))
style: Nowadays there's «sbox.ospath('A/mu')».
>
Bottom line: +0 on the change itself, but see the substantive review points
above.
Cheers,
Daniel
P.S. Your MUA doesn't wrap the email at 80 columns. Good for the patch, not
so good for the prose above it.
