Adjusting SVN commands to consider the @ symbol #4551

ffeu · 2018-07-25T20:11:35Z

Have you followed the guidelines in our Contributing document?
Have you checked to ensure there aren't other open Pull Requests for the same change?
Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your changes? Here's an example.
Have you successfully run brew style with your changes locally?
Have you successfully run brew tests with your changes locally?

Details

This is another approach to solve problem presented by the #4358 PR.

Subversion uses '@' to point to a specific revision, so when creating formulas with an @ symbol, the local svn path inherits that character and Subversion gets confused.

Basically, an additional @ at the end solves it. So no need to change the paths, it's justing changing the commands.

However, this is not consistent through all commands. The commands are affected as follows:

svn checkout url1 foo@a   # properly checks out url1 to foo@a
svn switch url2 foo@a     # properly switchs foo@a to url2
svn update foo@a@         # properly updates foo@a 
svn info foo@a@           # properly obtains info on foo@a 
svn export foo@a@ newdir  # properly export foo@a contents to newdir

As previously discussed in the #4358 PR, this approach:

affects only SVN resources;
doesn't change current path used for svn repositories
it's expected to NOT interfere with cleanup or mirror

brew style is OK with my changes, but there were warnings unrelated to this PR.

Disclaimer

I kindly request special attention reviewing this PR, as my ruby experience is equal to 28 lines of code (this PR). :)

reitermarkus · 2018-07-25T21:49:13Z

Library/Homebrew/download_strategy.rb

+  end
+
+  def svn_cached_location
+    escape(cached_location)


Just use escape(cached_location) everywhere instead of svn_cached_location.

Also this should be reused for the unpack strategy.

reitermarkus · 2018-07-25T21:50:29Z

Library/Homebrew/download_strategy.rb

  end

  private

+  def escape(svn_url)


Should this be a URL or a path?

reitermarkus · 2018-07-25T21:52:28Z

Library/Homebrew/download_strategy.rb

-    args << target
+    target_arg = target.to_s
+    target_arg = escape(target_arg) if svncommand == "up"
+    args << target_arg


Can you reorder this a bit? Currently we have to check twice for target.directory? and once for svncommand == "up", which all check for the same thing.

…sion.rb

ffeu · 2018-07-25T22:40:51Z

Hi @reitermarkus, done!

removed the svn_cached_location(),
renamed escape(url) to svn_escape(path)
placed svn_escape() inside unpack_strategy/subversion.rb **
** Not sure if that would be the best place though - let me know!

MikeMcQuaid · 2018-07-26T09:13:08Z

Thanks for the detailed and great first PR! CC @apjanke for thoughts on this as he'd worked on similar recently.

reitermarkus · 2018-07-27T00:17:23Z

Library/Homebrew/download_strategy.rb

+      args << target
+    elsif svncommand == "update"
+      args << svn_escape(target)
+    end


It's still checking for the same thing twice. Make this

args = if target.directory? ["svn", "update", svn_escape(target)] else ["svn", "checkout", url, target] end

That make me think: Is svn update target the same as cd target; svn update?

(args)
Oh, that's indeed way better. I didn't see it and I'm blaming my lack of ruby skills! ;-)

@reitermarkus , about the cd target

Yes, the effect of cd target@peg; svn update is the same as svn update target@peg@ - to the repository, at least.

I thought of that but as I was not sure if or how brew could get affected to these path changes, I kept with this "maybe more conservative approach". As per your comment I looked a bit further and decided to give it a try.

So the export command ended up like this and worked fine (thanks to the chdir).

system_command! "svn", args: ["export", "--force", ".", unpack_dir], chdir: path.to_s

The update looked like this and it worked (note: no output is read from it).

args = if target.directory? ["cd", target.to_s, ";", "svn", "update"] else ["svn", "checkout", url, target] end

However, I could not make the svn info work with popen_read. It's used by the methods last_commit, repo_url and source_modified_time. Looks like popen_read keeps returning the output of the first command (cd).

Tried this:

xml = REXML::Document.new(Utils.popen_read("cd", cached_location.to_s, ";", "svn", "info", "--xml"))

and even tried with &> /dev/null:

xml = REXML::Document.new(Utils.popen_read("cd", cached_location.to_s, "&> /dev/null;", "svn", "info", "--xml"))

In the end, even if we manage to get past this problem in svn info, I think the svn_escape() is easier to read and may be easier to understand in the future.

One option would be bringing svn_escape() back to download_strategy and using it only there, leaving the export command with the chdir.

What do you think?

Instead of popen_read you can also use system_command. It returns a result which you can call stdout on.

I didn't test with system_command yet, but do you think this is better?

# with svn_escape xml = REXML::Document.new(Utils.popen_read( "svn", "info", "--xml", svn_escape(cached_location))) ... # so far, with system_command, cd ..; & .stdout xml = REXML::Document.new(system_command("cd", cached_location.to_s, ";", "svn", "info", "--xml").stdout)

To me it seems less obscure using svn_escape... and there's the possibility that more adjustments might have to be done (as it didn't work already with popen_read).

info = system_command("svn", args: ["info", "--xml"], chdir: cached_location) .stdout xml = REXML::Document.new(info)

ffeu · 2018-07-29T14:56:26Z

OK, @reitermarkus, please check latest changes, I've tested and it's working.

But now Subversion is the only place using system_command in the download_strategy, everywhere else uses quiet_system and safe_system, or popen_read when reading output. As you probably know the difference between them, is that alright?
Now the args part ended up being the only place with cd ...; svn. I thought of changing it to:

    args = target.directory? ? ["update"] : ["checkout", url, target]
    if revision
      ohai "Checking out #{@ref}"
      args << "-r" << revision
    end
    args << "--ignore-externals" if ignore_externals
    if args[0] == "update"
      system_command("svn", args: args, chdir: target.to_s)
    else
      system_command("svn", args: args)
    end

Which works, but the output of svn update and checkout are not being shown to the user anymore - I imagine that's no problem but I wanted to double check. If that last bit is OK I'll push it.

reitermarkus · 2018-07-30T09:34:46Z

Library/Homebrew/download_strategy.rb

-    args << url unless target.directory?
-    args << target
+    args = if target.directory?
+      ["cd", target.to_s, ";", "svn", "update"]


This also needs to use system_command! with chdir:.

That's what I meant with my last comment, I wanted to check because of the
check if args[0] == "update"...

reitermarkus

One last change, then I think this is good to go.

reitermarkus · 2018-07-30T13:18:56Z

Library/Homebrew/download_strategy.rb

    if revision
      ohai "Checking out #{@ref}"
      args << "-r" << revision
    end
    args << "--ignore-externals" if ignore_externals
-    safe_system(*args)
+    if args[0] == "update"


Make this target.directory?.

reitermarkus · 2018-07-30T13:22:43Z

Library/Homebrew/download_strategy.rb

-    args = ["svn", svncommand]
-    args << url unless target.directory?
-    args << target
+    args = target.directory? ? ["update"] : ["checkout", url, target]


Make this an empty array and insert the args from here below like this.

system_command("svn", args: ["update", *args], chdir: target.to_s)

reitermarkus · 2018-07-30T18:01:58Z

Thanks, @ffeu, great work!

apjanke · 2018-07-30T19:04:26Z

Sorry for the slow reply; I've been traveling.

This looks like a good approach to me.

ffeu added 3 commits July 25, 2018 16:42

adjusting svn commands to consider the @ symbol

2f18fff

removing unintentional added spaces

c3516cf

removing unintentional added spaces 2

3098a90

reitermarkus requested changes Jul 25, 2018

View reviewed changes

removing svn_cached_location and using svn_escape(path) inside subver…

9594a56

…sion.rb

removing unnecessary .to_s

a0f3093

reitermarkus requested changes Jul 27, 2018

View reviewed changes

ffeu added 2 commits July 27, 2018 10:23

adjusting args in fetch_repo

ad18c5c

changing to use system_command .. chdir

ce3ac54

just adjusting spaces

43d53a6

reitermarkus requested changes Jul 30, 2018

View reviewed changes

svn update/checkout with system_command

c63d881

reitermarkus requested changes Jul 30, 2018

View reviewed changes

just one target.directory check in fetch_repo

4bb82cc

reitermarkus approved these changes Jul 30, 2018

View reviewed changes

reitermarkus merged commit b5548f9 into Homebrew:master Jul 30, 2018

reitermarkus mentioned this pull request Jul 30, 2018

Add spec for SVN directory name containing @. #4573

Merged

6 tasks

ffeu deleted the fix_svn_with_at_symbol branch July 30, 2018 19:11

lock bot added the outdated PR was locked due to age label Aug 29, 2018

lock bot locked as resolved and limited conversation to collaborators Aug 29, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adjusting SVN commands to consider the @ symbol #4551

Adjusting SVN commands to consider the @ symbol #4551

ffeu commented Jul 25, 2018

reitermarkus Jul 25, 2018

reitermarkus Jul 25, 2018

reitermarkus Jul 25, 2018

reitermarkus Jul 25, 2018

ffeu commented Jul 25, 2018

MikeMcQuaid commented Jul 26, 2018

reitermarkus Jul 27, 2018

ffeu Jul 27, 2018

ffeu Jul 27, 2018

reitermarkus Jul 28, 2018

ffeu Jul 28, 2018

reitermarkus Jul 29, 2018 •

edited

Loading

ffeu commented Jul 29, 2018

reitermarkus Jul 30, 2018

ffeu Jul 30, 2018

reitermarkus left a comment

reitermarkus Jul 30, 2018

reitermarkus Jul 30, 2018

ffeu Jul 30, 2018

reitermarkus commented Jul 30, 2018

apjanke commented Jul 30, 2018

Adjusting SVN commands to consider the @ symbol #4551

Adjusting SVN commands to consider the @ symbol #4551

Conversation

ffeu commented Jul 25, 2018

Details

Disclaimer

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ffeu commented Jul 25, 2018

MikeMcQuaid commented Jul 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reitermarkus Jul 29, 2018 • edited Loading

Choose a reason for hiding this comment

ffeu commented Jul 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reitermarkus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reitermarkus commented Jul 30, 2018

apjanke commented Jul 30, 2018

reitermarkus Jul 29, 2018 •

edited

Loading