Update Clone dialog's handling of clipboard text #9116

nyankoframe · 2021-04-26T04:50:22Z

Fixes #6260

Proposed changes

Adds a new method in FormClone to extract a URL from a text string.
Updates Clone form to use new method

Test methodology

Manual testing
Added new integration test in FormCloneTests.cs

Test environment(s)

GIT 2.31.0.windows.1
Windows 10.0.19042.868

✒️ I contribute this code under The Developer Certificate of Origin.

drewnoakes · 2021-04-26T05:42:18Z

GitCommands/PathUtil.cs

-                 || path.StartsWith("git:", StringComparison.CurrentCultureIgnoreCase)
-                 || path.StartsWith("ssh:", StringComparison.CurrentCultureIgnoreCase)
-                 || path.StartsWith("file:", StringComparison.CurrentCultureIgnoreCase));
+                && UrlRegex.IsMatch(path);


RegEx parsing of this kind of thing makes me a bit uneasy, as the format is generally quite involved. Why not use Url.TryParse instead?

I'm not sure if there is a Url.TryParse method (unless I'm looking in the wrong place?), but is this what you're referring to?
System.UriParser.IsWellFormedOriginalString

Ah, yes, Uri.TryCreate seems like a better match: https://docs.microsoft.com/en-us/dotnet/api/system.uri.trycreate?view=net-5.0#System_Uri_TryCreate_System_String_System_UriKind_System_Uri__

I've tried using Uri.TryCreate and it returns false if the string has any characters before or after the URL, which was the original problem - someone had a string in clipboard that of the form "git clone https://...." that wasn't getting detected as a valid potential URL because of the "git clone" text before the URL. I feel that using the regex to detect if the string contains a valid URL allows the user to manually remove non-URL text without having GE guess and potentially get it wrong.

Given a URL cannot contain spaces, you could take the input string, split it by spaces and check each value.

My concern is that the full URL format is complex, and this regex will not cover all valid cases. The Uri class is far more likely to be correct.

Thanks for the feedback, I've switched to using Uri.IsWellFormedUriString as that allows file paths to not be treated as URIs (Uri.TryCreate would consider them as URIs resulting in LocalRepositoryManager methods throwing an exception)

GitCommands/PathUtil.cs

UnitTests/GitCommands.Tests/Helpers/PathUtilTest.cs

nyankoframe · 2021-04-27T06:05:55Z

It looks like #9018 was already merged into the 3.5 milestone, so this PR might be redundant. @RussKie could you confirm? If so, I'll go ahead and close it.

RussKie · 2021-04-27T10:01:51Z

#9018 was merged into the master only

contributors.txt

RussKie

Thank you, but the API needs a little more work.

GitCommands/PathUtil.cs

RussKie · 2021-05-02T11:06:16Z

UnitTests/GitCommands.Tests/Helpers/PathUtilTest.cs

+        [TestCase("git clone https://github.com/gitextensions/gitextensions", true)]
+        [TestCase("HTTPS://MYPRIVATEGITHUB.COM:8080/LOUDREPO.GIT", true)]
+        [TestCase("git://myurl/myrepo.git", true)]
+        [TestCase("git clone https://github.com/gitextensions/gitextensions && cd gitextensions", true)]
+        [TestCase("github.com/gitextensions", false)]
+        [TestCase("github.com/gitextensions/gitextensions/pull/9018", false)]
+        [TestCase("git clone ssh://username@gerrit-server:/PROJECT", true)]


Neither of these are correct inputs for CanBeGitURL method - because the answer to the question "Can either of those be a git URL?" is "no".

[TestCase("git clone https://github.com/gitextensions/gitextensions", true)] [TestCase("git clone https://github.com/gitextensions/gitextensions && cd gitextensions", true)] [TestCase("github.com/gitextensions", false)] [TestCase("github.com/gitextensions/gitextensions/pull/9018", false)] [TestCase("git clone ssh://username@gerrit-server:/PROJECT", true)]

We can consider renaming the method to ContainsGitUrl, because then the answer becomes "yes". But then the signature must be changed as well to return the valid git URL... So we'll have to make it something like:

public static bool TryExtractGitUrl(string url, out string gitUrl)

I believe the comments are addressed

RussKie · 2021-05-02T11:10:39Z

GitCommands/PathUtil.cs

+            string[] pathElements = path.Split(' ');
+            bool validUrl = false;
+            foreach (string str in pathElements)
+            {
+                validUrl |= Uri.IsWellFormedUriString(str, UriKind.Absolute);
+            }


Likewise this method's purpose is to validate the input string, and not break it apart and check each part. The proposal makes the behaviour surprising - as now the question "Is 'git clone https://github.com/gitextensions/gitextensions' a URL?" results in "Yes", which is unexpected to anyone looking at the signature.

Agree. If the caller wants to check individual words, they can split the string and call this on each.

RussKie · 2021-05-02T11:11:58Z

~~Please rebase the PR on to the master branch. This won't go into 3.5.1~~ Scratch that.

gerhardol · 2021-05-15T17:04:06Z

@nyankoframe Any update? 3.5.1 is hopefully released quite soon

nyankoframe · 2021-05-18T04:56:01Z

@nyankoframe Any update? 3.5.1 is hopefully released quite soon

Sorry for the delay; I was a bit buried at work, but should have some time in the next few days to make the proposed changes.

drewnoakes · 2021-05-25T11:50:33Z

GitCommands/PathUtil.cs

+            return Uri.IsWellFormedUriString(path, UriKind.Absolute);
+        }
+
+        public static bool ExtractURLsFromString(string contents, List<string> urls)


A small change to the API here seems more idiomatic, somehow.

Suggested change

public static bool ExtractURLsFromString(string contents, List<string> urls)

public static bool TryExtractURLsFromString(string contents, out List<string> urls)

Thanks for the feedback; will update in the next iteration.

drewnoakes · 2021-05-25T11:51:09Z

GitCommands/PathUtil.cs

+                return false;
+            }
+
+            string[] parts = contents.Split(' ');


Could use LazyStringSplit here (I think the PR where I added that was merged) to avoid allocating a string[].

I don't see LazyStringSplit in the 3.5 branch, but will use it when I replace the original implementation in the main branch.

drewnoakes · 2021-05-25T11:51:50Z

GitCommands/PathUtil.cs

        /// <param name="path">A path to check.</param>
-        /// <returns><see langword="true"/> if the given path starts with 'http', 'ssh' or 'git'; otherwise <see langword="false"/>.</returns>
+        /// <returns><see langword="true"/> if the given path contains a URL; otherwise <see langword="false"/>.</returns>
        [Pure]
        public static bool IsUrl(string path)


This method doesn't add much value any more. It's basically just Uri.IsWellFormedUriString. Do we still need it?

I've removed this method and replaced references to it by calling Uri.IsWellFormedUriString.

nyankoframe · 2021-05-27T00:55:57Z

I don't understand why the submodule tests are failing in AppVeyor, but pass when I run them locally. :(

gerhardol

I would like to see the test being updated, but otherwise OK for me

gerhardol · 2021-05-29T20:15:16Z

UnitTests/GitCommands.Tests/Helpers/PathUtilTest.cs

+        [TestCase("file:", false)]
+        [TestCase("SSH:", true)]
+        [TestCase("SSH", false)]
+        [TestCase("https://myhost:12368/", true)]
        public void CanBeGitURL(string url, bool expected)


There are no tests for the second tests

|| url.EndsWith(".git", StringComparison.CurrentCultureIgnoreCase) || GitModule.IsValidGitWorkingDir(url);

There are some tests for paths/URLs that end with .git but I guess there are no tests for IsValidGitWorkingDir since it needs a specific directory structure.

gerhardol · 2021-05-29T20:20:21Z

I don't understand why the submodule tests are failing in AppVeyor, but pass when I run them locally. :(

Tests require the as much effort as the code...
This is likely not caused by this PR though.
Updated build is OK.

I cannot see what goes wrong, it seems a little like tests run in parallel (which is not possible for these tests).
The test timed out, likely due to a popup in the test sever, that will need to be investigated by logging in to the server before the timeout fires.

RussKie · 2021-05-30T03:57:38Z

GitCommands/PathUtil.cs

+        /// <param name="contents">A string to attempt to extract URLs from.</param>
+        /// <param name="urls">A <see cref="List{T}"/> that contains any URLs extracted from <paramref name="contents"/>.</param>
+        /// <returns><see langword="true"/> if the given string contains one or more URLs; otherwise <see langword="false"/>.</returns>
+        public static bool TryExtractURLsFromString(string contents, out List<string> urls)


Suggested change

public static bool TryExtractURLsFromString(string contents, out List<string> urls)

public static bool TryExtractUrls(string contents, out IReadOnlyList<string> urls)

RussKie · 2021-05-30T04:02:12Z

GitCommands/PathUtil.cs

+        /// <param name="contents">A string to attempt to extract URLs from.</param>
+        /// <param name="urls">A <see cref="List{T}"/> that contains any URLs extracted from <paramref name="contents"/>.</param>
+        /// <returns><see langword="true"/> if the given string contains one or more URLs; otherwise <see langword="false"/>.</returns>
+        public static bool TryExtractURLsFromString(string contents, out List<string> urls)


💭 To clarify - we now have a single callsite for this method in FormClone, right? If this is so, I'd turn this method into a private in FormClone, and test it through a test accessor. And since the only use case for this method is to use the first detected URL, there are questions why we need to allocate a list, etc.

Thank you for the feedback; I've changed this method to a private in FormClone and have added a test for it.

.

gerhardol · 2021-06-01T10:30:07Z

Note: 3.5.1 is hopefully shipped soon...

gerhardol · 2021-06-01T22:21:22Z

IntegrationTests/UI.IntegrationTests/CommandsDialogs/FormCloneTests.cs

+
+                    accessor.TryExtractUrl(text, out var url).Should().Equals(expected);
+
+                    url.Should().Equals(expectedUrl);


Should expected URL be tested if expected is false?

This change adds a new FormClone.TryExtractUrl method to split a string by spaces and extract the first string for which Uri.IsWellFormedUriString returns true. A new integration test is added for FormClone to validate the behavior of the new method. This change updates the Clone dialog to use the new method for extracting URLs from clipboard contents. Calls to PathUtil.IsUrl are replaced by direct calls to Uri.IsWellFormedUriString.

RussKie · 2021-06-05T12:15:04Z

Thank you!

RussKie · 2021-06-05T13:01:33Z

@nyankoframe can you please send a matching PR to the master branch?

This was changed in gitextensions#9116 to only check for a valid URL as part of the rework to break down a string of text to find a valid URL.

ghost assigned nyankoframe Apr 26, 2021

nyankoframe mentioned this pull request Apr 26, 2021

Ignore git clone prefix when checking clipboard for source URL #9018

Merged

nyankoframe force-pushed the ignore-git-clone-prefix-3.5 branch from ebb57e6 to 477a0bf Compare April 26, 2021 04:55

drewnoakes reviewed Apr 26, 2021

View reviewed changes

GitCommands/PathUtil.cs Outdated Show resolved Hide resolved

drewnoakes reviewed Apr 26, 2021

View reviewed changes

UnitTests/GitCommands.Tests/Helpers/PathUtilTest.cs Outdated Show resolved Hide resolved

nyankoframe force-pushed the ignore-git-clone-prefix-3.5 branch 2 times, most recently from ef30e74 to c687e82 Compare April 27, 2021 05:57

RussKie requested changes Apr 27, 2021

View reviewed changes

contributors.txt Outdated Show resolved Hide resolved

ghost added the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Apr 27, 2021

RussKie added this to the 3.5.1 milestone Apr 27, 2021

nyankoframe force-pushed the ignore-git-clone-prefix-3.5 branch from c687e82 to fcbb7d1 Compare May 2, 2021 04:01

ghost removed the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label May 2, 2021

nyankoframe changed the title ~~Use regex for detecting potential URLs~~ Use System.Uri methods for detecting potential URLs May 2, 2021

nyankoframe requested review from RussKie and drewnoakes May 2, 2021 04:04

RussKie previously requested changes May 2, 2021

View reviewed changes

ghost added the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label May 2, 2021

ghost removed the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label May 18, 2021

nyankoframe force-pushed the ignore-git-clone-prefix-3.5 branch from fcbb7d1 to a6c767b Compare May 25, 2021 01:25

nyankoframe requested a review from RussKie May 25, 2021 01:25

nyankoframe changed the title ~~Use System.Uri methods for detecting potential URLs~~ Add method for extracting URLs in text string May 25, 2021

nyankoframe force-pushed the ignore-git-clone-prefix-3.5 branch from a6c767b to 0d3845e Compare May 25, 2021 01:29

drewnoakes reviewed May 25, 2021

View reviewed changes

RussKie added the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label May 26, 2021

ghost removed the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label May 26, 2021

nyankoframe force-pushed the ignore-git-clone-prefix-3.5 branch from 0d3845e to 7b3852b Compare May 26, 2021 06:22

nyankoframe requested a review from drewnoakes May 26, 2021 06:22

nyankoframe force-pushed the ignore-git-clone-prefix-3.5 branch from 7b3852b to efb5c6b Compare May 27, 2021 22:34

gerhardol approved these changes May 29, 2021

View reviewed changes

RussKie reviewed May 30, 2021

View reviewed changes

RussKie added the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Jun 1, 2021

ghost removed the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Jun 1, 2021

nyankoframe force-pushed the ignore-git-clone-prefix-3.5 branch from efb5c6b to 212674b Compare June 1, 2021 18:54

nyankoframe changed the title ~~Add method for extracting URLs in text string~~ Update Clone dialog's handling of clipboard text Jun 1, 2021

nyankoframe requested a review from RussKie June 1, 2021 18:55

gerhardol reviewed Jun 1, 2021

View reviewed changes

gerhardol approved these changes Jun 1, 2021

View reviewed changes

nyankoframe force-pushed the ignore-git-clone-prefix-3.5 branch from 212674b to d462673 Compare June 2, 2021 21:42

RussKie approved these changes Jun 5, 2021

View reviewed changes

RussKie merged commit 4267698 into gitextensions:release/3.5 Jun 5, 2021

nyankoframe deleted the ignore-git-clone-prefix-3.5 branch June 6, 2021 01:07

nyankoframe mentioned this pull request Jun 6, 2021

Update Clone dialog's handling of clipboard text #9266

Merged

BlythMeister mentioned this pull request Aug 19, 2021

Clone no longer detects github ssh url #9520

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Clone dialog's handling of clipboard text #9116

Update Clone dialog's handling of clipboard text #9116

nyankoframe commented Apr 26, 2021 •

edited

Loading

drewnoakes Apr 26, 2021

nyankoframe Apr 26, 2021

drewnoakes Apr 26, 2021

nyankoframe Apr 27, 2021

drewnoakes Apr 27, 2021

nyankoframe May 2, 2021

nyankoframe commented Apr 27, 2021

RussKie commented Apr 27, 2021 via email •

edited by ghost

Loading

RussKie left a comment

RussKie May 2, 2021

gerhardol May 29, 2021

RussKie May 2, 2021

drewnoakes May 16, 2021

RussKie commented May 2, 2021 •

edited

Loading

gerhardol commented May 15, 2021

nyankoframe commented May 18, 2021

drewnoakes May 25, 2021

nyankoframe May 26, 2021

drewnoakes May 25, 2021

nyankoframe May 26, 2021

drewnoakes May 25, 2021

nyankoframe May 26, 2021

nyankoframe commented May 27, 2021

gerhardol left a comment

gerhardol May 29, 2021

nyankoframe May 29, 2021

gerhardol commented May 29, 2021

RussKie May 30, 2021

RussKie May 30, 2021

nyankoframe Jun 1, 2021

gerhardol commented Jun 1, 2021

gerhardol Jun 1, 2021

RussKie commented Jun 5, 2021

RussKie commented Jun 5, 2021

	public static bool ExtractURLsFromString(string contents, List<string> urls)
	public static bool TryExtractURLsFromString(string contents, out List<string> urls)

	public static bool TryExtractURLsFromString(string contents, out List<string> urls)
	public static bool TryExtractUrls(string contents, out IReadOnlyList<string> urls)


		accessor.TryExtractUrl(text, out var url).Should().Equals(expected);

		url.Should().Equals(expectedUrl);

Update Clone dialog's handling of clipboard text #9116

Update Clone dialog's handling of clipboard text #9116

Conversation

nyankoframe commented Apr 26, 2021 • edited Loading

Proposed changes

Test methodology

Test environment(s)

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

Choose a reason for hiding this comment

nyankoframe commented Apr 27, 2021

RussKie commented Apr 27, 2021 via email • edited by ghost Loading

RussKie 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

Choose a reason for hiding this comment

RussKie commented May 2, 2021 • edited Loading

gerhardol commented May 15, 2021

nyankoframe commented May 18, 2021

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

Choose a reason for hiding this comment

nyankoframe commented May 27, 2021

gerhardol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gerhardol commented May 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gerhardol commented Jun 1, 2021

Choose a reason for hiding this comment

RussKie commented Jun 5, 2021

RussKie commented Jun 5, 2021

nyankoframe commented Apr 26, 2021 •

edited

Loading

RussKie commented Apr 27, 2021 via email •

edited by ghost

Loading

RussKie commented May 2, 2021 •

edited

Loading