On 26 Jan 2021, Daniel Shahaf wrote:
>Karl Fogel wrote on Tue, Jan 26, 2021 at 16:11:31 -0600:
>> 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.
<giggle> Yes, of course :-).
>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.
Yes, I think people can adjust quite quickly to this, if they have such
customizations.
>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.
I thought about trying that too, but came to the same conclusion you did.
>> +++ subversion/svn/propedit-cmd.c (working copy)
>> @@ -143,7 +143,9 @@
>
>«svn diff -x-p», please ☺
Ah, okay. I'll try to remember this for next time!
>> +++ 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.)
W00t! Will do.
>> + # 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.
Gotcha.
>> + 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.
Good idea. I'm still not accustomed to r-strings in Python; I should get used
to it.
>> + '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')».
Ah, cool. Just for the record, I copied the above from elsewhere in the test
:-). But just because some of the source is outdated doesn't mean my new code
has to be! I'll use the new way.
>Bottom line: +0 on the change itself, but see the substantive review points
>above.
Thank you so much for the excellent review, Daniel! I will submit a new patch
with your points addressed.
>P.S. Your MUA doesn't wrap the email at 80 columns. Good for the patch, not
>so good for the prose above it.
*nod* I'll follow up with you privately about that, as I recently heard about
it from someone else. I'd like to figure out what's going on & solve it.
Best regards,
-Karl
