You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by "lonicerae (via GitHub)" <gi...@apache.org> on 2024/12/27 10:38:47 UTC
[PR] Display the original name of component. [subversion]
lonicerae opened a new pull request, #26:
URL: https://github.com/apache/subversion/pull/26
* In the old days, the '--summerize' option could show the component's original's name in the svn URI because it's not escaped. After the commit 3dbf4fb0 the component's name would be escaped. So it's not easy to tell which file was modified based on the svn URI format. This small patch appends the original name to the URI result so that the user could read the file name after displaying the svn URI.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@subversion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Display the original name of component. [subversion]
Posted by Branko Čibej <br...@apache.org>.
On 2. 2. 25 13:28, Stefan Sperling wrote:
> On Sun, Feb 02, 2025 at 03:12:40AM +0100, Branko Čibej wrote:
>> On 8. 1. 25 13:52, Daniel Sahlberg wrote:
>>> I believe the reason was to "display names in human readable format",
>>> rather than to actually do local processing. Since this is only an issue
>>> when there is no WC, I was assuming they used it to check for example
>>> other branches than the current branch.
>> We already have svn info URL --show-item, the only thing missing in order to
>> display user-friendly names (only) is --show-item=path.
> This doesn't help when a path relative to some directory in the repository
> (such as ^/trunk) is needed. The Path item seems to always show the last
> component of the provided URL. Any intermediate directories are lost.
Yes. But everything else are URLs; for 'svn info URL', Path is the only
thing that's returned in a user-friendly format. One could argue that it
should be called Name, not Path, and that Path should be the whole
user-friendly relative path from the root; but it is what it is now. At
least you can get the relative URL, too.
I agree it's not ideal. *shrug*. I'd rather look at proposals to expand
'svn info (--show-item)' than to break the 'svn diff --summarize' output
format.
-- Brane
Re: [PR] Display the original name of component. [subversion]
Posted by Stefan Sperling <st...@stsp.name>.
On Sun, Feb 02, 2025 at 03:12:40AM +0100, Branko Čibej wrote:
> On 8. 1. 25 13:52, Daniel Sahlberg wrote:
> > I believe the reason was to "display names in human readable format",
> > rather than to actually do local processing. Since this is only an issue
> > when there is no WC, I was assuming they used it to check for example
> > other branches than the current branch.
>
> We already have svn info URL --show-item, the only thing missing in order to
> display user-friendly names (only) is --show-item=path.
This doesn't help when a path relative to some directory in the repository
(such as ^/trunk) is needed. The Path item seems to always show the last
component of the provided URL. Any intermediate directories are lost.
Re: [PR] Display the original name of component. [subversion]
Posted by Branko Čibej <br...@apache.org>.
On 8. 1. 25 13:52, Daniel Sahlberg wrote:
> Den tis 31 dec. 2024 kl 10:36 skrev Stefan Sperling <st...@stsp.name>:
>
> This change seems to be unnecessary to me. The stated motivation
> for it,
> i.e. translating URLs to local paths in order to decide which
> local files
> were modified, ignores risks which occur beyond the escaping of
> characters.
>
>
> If a mapping from repo URL to a local path is needed for scripting
> purposes
> without any further processing, then the file or directory can be
> checked
> out before diff --summarize is run. When provided a working copy
> path then
> diff --summarize displays on-disk paths in local style, spaces and
> all.
> So why not just use a working copy if local paths are required?
>
> When provided a URL, then diff --summarize displays a URL.
> There should already be tools which decode URI-encoded data for use in
> a script if that is needed. However, there are problems beyond
> characters
> which require ecaping in a URL. Scripts should take precautions when
> translating URLs to a local path in an automated way. The
> repository root
> needs to be infered somehow, e.g. the repository-relative path for
> the URL
> https://svn.apache.org/repos/asf/subversion/trunk/libsvn_client
> is subversion/trunk/libsvn_client, and scripts would need to know
> this.
>
> Worse, paths need to be normalized before use because a malicious
> server
> or proxy could return a URL containing paths components such as .. and
> then overwrite files outside the intended working area.
>
> The SVN client covers such problems during checkout. Checking out
> a working
> copy before operating on local paths is much safer than working
> from URLs.
>
>
> Thanks Stefan for your thoughts on this, your comments uncovered even
> more problems with this change.
>
> I believe the reason was to "display names in human readable format",
> rather than to actually do local processing. Since this is only an
> issue when there is no WC, I was assuming they used it to check for
> example other branches than the current branch.
We already have svn info URL --show-item, the only thing missing in
order to display user-friendly names (only) is --show-item=path.
-- Brane
Re: [PR] Display the original name of component. [subversion]
Posted by Daniel Sahlberg <da...@gmail.com>.
Den tis 31 dec. 2024 kl 10:36 skrev Stefan Sperling <st...@stsp.name>:
> This change seems to be unnecessary to me. The stated motivation for it,
> i.e. translating URLs to local paths in order to decide which local files
> were modified, ignores risks which occur beyond the escaping of characters.
>
> If a mapping from repo URL to a local path is needed for scripting purposes
> without any further processing, then the file or directory can be checked
> out before diff --summarize is run. When provided a working copy path then
> diff --summarize displays on-disk paths in local style, spaces and all.
> So why not just use a working copy if local paths are required?
>
> When provided a URL, then diff --summarize displays a URL.
> There should already be tools which decode URI-encoded data for use in
> a script if that is needed. However, there are problems beyond characters
> which require ecaping in a URL. Scripts should take precautions when
> translating URLs to a local path in an automated way. The repository root
> needs to be infered somehow, e.g. the repository-relative path for the URL
> https://svn.apache.org/repos/asf/subversion/trunk/libsvn_client
> is subversion/trunk/libsvn_client, and scripts would need to know this.
>
> Worse, paths need to be normalized before use because a malicious server
> or proxy could return a URL containing paths components such as .. and
> then overwrite files outside the intended working area.
>
> The SVN client covers such problems during checkout. Checking out a working
> copy before operating on local paths is much safer than working from URLs.
>
Thanks Stefan for your thoughts on this, your comments uncovered even more
problems with this change.
I believe the reason was to "display names in human readable format",
rather than to actually do local processing. Since this is only an issue
when there is no WC, I was assuming they used it to check for example other
branches than the current branch.
The scripting issue wasn't even mentioned by the OP, it was my arguments
for NOT making any changes.
Anyway, the OP reviewed this thread and decided to retract the PR, so I'm
only replying to have it in the e-mail archives as part of this thread.
Cheers,
Daniel
Re: [PR] Display the original name of component. [subversion]
Posted by Stefan Sperling <st...@stsp.name>.
On Mon, Dec 30, 2024 at 08:06:37PM +0100, Daniel Sahlberg wrote:
> Den lör 28 dec. 2024 kl 21:35 skrev Daniel Sahlberg <
> daniel.l.sahlberg@gmail.com>:
>
> > I also made a "review" in Github but it seems that wasn't forwarded to the
> > list so I'll copy-paste it below:
> >
> > Thank you for the patch!
> >
> > I understand the idea of the patch but I'm not sure I think this
> > implementation is a good idea.
> >
> > As noted in the code (lines 169-171) the format is the same as svn status.
> > Changing the output format will break any scripts relying on parsing the
> > text output. Of course, that could be acceptable, but the bar should be
> > quite high in my opinion.
> >
> > The extra column will be output both in svn diff [working copy path] and
> > svn diff [url]. When operating on a wc path, the extra data will be the
> > same (plus leading and trailing single quote). When operating on a URL, the
> > extra data will be the path component of the url, without url encoding.
> >
> > The new format would be very hard to parse:
> >
> > Spllitting on space will not work if the path contains a space (it will be
> > output verbatim at LEAST in the second component).
> > Splitting on single quote will not work if the path contains a single
> > qoute.
> >
> > All in all, I'm -0 to accept this patch: I don't agree but I wouldn't
> > stand in the way of consensus.
> >
> >
> >
> > Cheers,
> >
> > Daniel
> >
> >
> The user came back om Github with the following message:
> [[[
> Hey @dsahlberg-apache-org <https://github.com/dsahlberg-apache-org> ! Thank
> you so much for reviewing this PR! What you said is exactly what I was
> concerned about. Such a change would indeed render scripts that rely on
> command outputs unusable. To be honest, I'm not a seasoned SVN user, and
> this issue came up during conversations with my friends about their
> operations challenges. So, I submitted this commit with a "let's give it a
> try" mindset.
> I’d like to hear your thoughts on how we might improve this to partially
> retain the output format of the old version (i.e., properly displaying
> filenames that need escaping) while also supporting the current URI-style
> output. How could we achieve a balance between the two? I understand that
> removing the --summarize option allows filenames to display correctly, but
> it would be more helpful if filenames could be included in a single line.
> Thank you very much!
> ]]]
>
> To make it easier to grasp, I'm attaching a few samples
>
> I have a repo test, with a subdirectory dir1 and a file named path 'path'
> (the word path twice, the second time in single quotes).
>
> This is the current output
> [[[
> $ svn diff -c68 --summarize https://svn.example.com/svn/test
> A https://svn.example.com/svn/test/dir1/path%20'path'
> ]]]
>
> The PR would output
> [[[
> $ svn diff -c68 --summarize https://svn.example.com/svn/test
> A https://svn.example.com/svn/test/dir1/path%20'path' 'path 'path''
> ]]]
> I don't like this at all, the only way (that I can think of) to figure out
> where the "filename" part starts would be to count the number of single
> quotes and split before the ((n-2)/2+1):nd.
>
> Using svn_path_join_internal instead of svn_path_url_add_component would
> output:
> [[[
> $ svn diff -c68 --summarize https://svn.example.com/svn/test
> A https://svn.example.com/svn/test/dir1/path 'path'
> ]]]
> I think this last example would be acceptable. Most scripts would PROBABLY
> already cut from the 9:th character to the end of the line and that would
> still work. Using the URL as an argument to, for example, wget would
> require it to be quoted, but so does the existing output (at least to wget).
>
> I don't want to make this change myself, but rather looking for some +1's
> (or -1's) from other committers.
This change seems to be unnecessary to me. The stated motivation for it,
i.e. translating URLs to local paths in order to decide which local files
were modified, ignores risks which occur beyond the escaping of characters.
If a mapping from repo URL to a local path is needed for scripting purposes
without any further processing, then the file or directory can be checked
out before diff --summarize is run. When provided a working copy path then
diff --summarize displays on-disk paths in local style, spaces and all.
So why not just use a working copy if local paths are required?
When provided a URL, then diff --summarize displays a URL.
There should already be tools which decode URI-encoded data for use in
a script if that is needed. However, there are problems beyond characters
which require ecaping in a URL. Scripts should take precautions when
translating URLs to a local path in an automated way. The repository root
needs to be infered somehow, e.g. the repository-relative path for the URL
https://svn.apache.org/repos/asf/subversion/trunk/libsvn_client
is subversion/trunk/libsvn_client, and scripts would need to know this.
Worse, paths need to be normalized before use because a malicious server
or proxy could return a URL containing paths components such as .. and
then overwrite files outside the intended working area.
The SVN client covers such problems during checkout. Checking out a working
copy before operating on local paths is much safer than working from URLs.
Re: [PR] Display the original name of component. [subversion]
Posted by Daniel Sahlberg <da...@gmail.com>.
Den lör 28 dec. 2024 kl 21:35 skrev Daniel Sahlberg <
daniel.l.sahlberg@gmail.com>:
> I also made a "review" in Github but it seems that wasn't forwarded to the
> list so I'll copy-paste it below:
>
> Thank you for the patch!
>
> I understand the idea of the patch but I'm not sure I think this
> implementation is a good idea.
>
> As noted in the code (lines 169-171) the format is the same as svn status.
> Changing the output format will break any scripts relying on parsing the
> text output. Of course, that could be acceptable, but the bar should be
> quite high in my opinion.
>
> The extra column will be output both in svn diff [working copy path] and
> svn diff [url]. When operating on a wc path, the extra data will be the
> same (plus leading and trailing single quote). When operating on a URL, the
> extra data will be the path component of the url, without url encoding.
>
> The new format would be very hard to parse:
>
> Spllitting on space will not work if the path contains a space (it will be
> output verbatim at LEAST in the second component).
> Splitting on single quote will not work if the path contains a single
> qoute.
>
> All in all, I'm -0 to accept this patch: I don't agree but I wouldn't
> stand in the way of consensus.
>
>
>
> Cheers,
>
> Daniel
>
>
The user came back om Github with the following message:
[[[
Hey @dsahlberg-apache-org <https://github.com/dsahlberg-apache-org> ! Thank
you so much for reviewing this PR! What you said is exactly what I was
concerned about. Such a change would indeed render scripts that rely on
command outputs unusable. To be honest, I'm not a seasoned SVN user, and
this issue came up during conversations with my friends about their
operations challenges. So, I submitted this commit with a "let's give it a
try" mindset.
I’d like to hear your thoughts on how we might improve this to partially
retain the output format of the old version (i.e., properly displaying
filenames that need escaping) while also supporting the current URI-style
output. How could we achieve a balance between the two? I understand that
removing the --summarize option allows filenames to display correctly, but
it would be more helpful if filenames could be included in a single line.
Thank you very much!
]]]
To make it easier to grasp, I'm attaching a few samples
I have a repo test, with a subdirectory dir1 and a file named path 'path'
(the word path twice, the second time in single quotes).
This is the current output
[[[
$ svn diff -c68 --summarize https://svn.example.com/svn/test
A https://svn.example.com/svn/test/dir1/path%20'path'
]]]
The PR would output
[[[
$ svn diff -c68 --summarize https://svn.example.com/svn/test
A https://svn.example.com/svn/test/dir1/path%20'path' 'path 'path''
]]]
I don't like this at all, the only way (that I can think of) to figure out
where the "filename" part starts would be to count the number of single
quotes and split before the ((n-2)/2+1):nd.
Using svn_path_join_internal instead of svn_path_url_add_component would
output:
[[[
$ svn diff -c68 --summarize https://svn.example.com/svn/test
A https://svn.example.com/svn/test/dir1/path 'path'
]]]
I think this last example would be acceptable. Most scripts would PROBABLY
already cut from the 9:th character to the end of the line and that would
still work. Using the URL as an argument to, for example, wget would
require it to be quoted, but so does the existing output (at least to wget).
I don't want to make this change myself, but rather looking for some +1's
(or -1's) from other committers.
Cheers,
Daniel
Re: [PR] Display the original name of component. [subversion]
Posted by Daniel Sahlberg <da...@gmail.com>.
I also made a "review" in Github but it seems that wasn't forwarded to the
list so I'll copy-paste it below:
Thank you for the patch!
I understand the idea of the patch but I'm not sure I think this
implementation is a good idea.
As noted in the code (lines 169-171) the format is the same as svn status.
Changing the output format will break any scripts relying on parsing the
text output. Of course, that could be acceptable, but the bar should be
quite high in my opinion.
The extra column will be output both in svn diff [working copy path] and
svn diff [url]. When operating on a wc path, the extra data will be the
same (plus leading and trailing single quote). When operating on a URL, the
extra data will be the path component of the url, without url encoding.
The new format would be very hard to parse:
Spllitting on space will not work if the path contains a space (it will be
output verbatim at LEAST in the second component).
Splitting on single quote will not work if the path contains a single qoute.
All in all, I'm -0 to accept this patch: I don't agree but I wouldn't stand
in the way of consensus.
Cheers,
Daniel
Re: [PR] Display the original name of component. [subversion]
Posted by "dsahlberg-apache-org (via GitHub)" <gi...@apache.org>.
dsahlberg-apache-org commented on PR #26:
URL: https://github.com/apache/subversion/pull/26#issuecomment-2564432858
Changing the call to svn_path_url_add_component2() to svn_path_join() would remove the url encoding without (otherwise) changing the output format.
svn_path_join is deprecated, so probably not a good idea to call it directly - some additional work would be required to make the _internal function available (remove static and add to a private header).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@subversion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Display the original name of component. [subversion]
Posted by "lonicerae (via GitHub)" <gi...@apache.org>.
lonicerae commented on PR #26:
URL: https://github.com/apache/subversion/pull/26#issuecomment-2579127061
Thanks @hartmannathan ! Just subscribed.;)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@subversion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Display the original name of component. [subversion]
Posted by "hartmannathan (via GitHub)" <gi...@apache.org>.
hartmannathan commented on PR #26:
URL: https://github.com/apache/subversion/pull/26#issuecomment-2572198527
@lonicerae thanks for your input. You're always welcome to join the mailing lists (see [https://subversion.apache.org/mailing-lists.html](https://subversion.apache.org/mailing-lists.html))... Hope to see you there!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@subversion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Display the original name of component. [subversion]
Posted by "hartmannathan (via GitHub)" <gi...@apache.org>.
hartmannathan commented on PR #26:
URL: https://github.com/apache/subversion/pull/26#issuecomment-2569759416
In case you haven't seen it... There is a discussion about this PR on the mailing list (see thread at: [https://lists.apache.org/thread/8y5dbfxkzc1m1g9x5wpmb0kfp6yfdjgb](https://lists.apache.org/thread/8y5dbfxkzc1m1g9x5wpmb0kfp6yfdjgb))
In particular, please see the following message: [https://lists.apache.org/thread/9n51xxtdjmmxrmw4o2l75cbchkpngmjm](https://lists.apache.org/thread/9n51xxtdjmmxrmw4o2l75cbchkpngmjm) which explains how to use a working copy to get what you're asking for ("on-disk paths in local style").
IMHO we should not change the output format due to potential for breakage. Also, other problems are likely, as explained in the above message.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@subversion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Display the original name of component. [subversion]
Posted by "lonicerae (via GitHub)" <gi...@apache.org>.
lonicerae commented on PR #26:
URL: https://github.com/apache/subversion/pull/26#issuecomment-2565444995
Hey @dsahlberg-apache-org ! Thank you so much for reviewing this PR! What you said is exactly what I was concerned about. Such a change would indeed render scripts that rely on command outputs unusable. To be honest, I'm not a seasoned SVN user, and this issue came up during conversations with my friends about their operations challenges. So, I submitted this commit with a "let's give it a try" mindset.
I’d like to hear your thoughts on how we might improve this to partially retain the output format of the old version (i.e., properly displaying filenames that need escaping) while also supporting the current URI-style output. How could we achieve a balance between the two? I understand that removing the --summarize option allows filenames to display correctly, but it would be more helpful if filenames could be included in a single line. Thank you very much!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@subversion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Display the original name of component. [subversion]
Posted by "lonicerae (via GitHub)" <gi...@apache.org>.
lonicerae closed pull request #26: Display the original name of component.
URL: https://github.com/apache/subversion/pull/26
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@subversion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Display the original name of component. [subversion]
Posted by "MarsrockEarth (via GitHub)" <gi...@apache.org>.
MarsrockEarth commented on PR #26:
URL: https://github.com/apache/subversion/pull/26#issuecomment-2563566629
您的来信已收到
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@subversion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Display the original name of component. [subversion]
Posted by "lonicerae (via GitHub)" <gi...@apache.org>.
lonicerae commented on PR #26:
URL: https://github.com/apache/subversion/pull/26#issuecomment-2572115946
@hartmannathan @dsahlberg-apache-org @stspdotname You're so kind! Thank you very much! I should register an account to join the discussion on the mailing list. Let me share your valuable thoughts with my friend. I'll close this PR now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@subversion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org