+ [JENKINS-30176] Repository browser VisualSVN added #163

arnosthavelka · 2016-06-16T11:46:42Z

[JENKINS-30176] Repository browser VisualSVN added

arnosthavelka · 2016-06-16T12:57:52Z

Ad build failure) Well, external repository https://svn.jenkins-ci.org/ is not available. No test error is related to my change (there's no test on browser provider implementation).

arnosthavelka · 2016-07-15T09:35:16Z

Fixed just by merging to the latest sources (after version 2.6).

arnosthavelka · 2016-09-02T10:50:54Z

I am wondering why is it still opened (waiting)? And not accepted or rejected. I am forced to use my own plugin's build. I would like to handle SVN plugin as any other in Plugin Manager.

yultide · 2016-12-16T00:13:09Z

@jglick @recena Can someone merge this? I want to use VisualSVN server with Jenkins without a custom plugin build

oleg-nenashev

Javadoc does not seem to be finalized according to DOCUMENT ME! comments. There are also some minor glitches in the code

oleg-nenashev · 2016-12-16T00:35:01Z

src/main/java/hudson/scm/browsers/VisualSVN.java

+/*
+ * The MIT License
+ * 
+ * Copyright (c) 2004-2009, Sun Microsystems, Inc., Kohsuke Kawaguchi, Daniel Dyer


Likely the license header is wrong

oleg-nenashev · 2016-12-16T00:35:28Z

src/main/java/hudson/scm/browsers/VisualSVN.java

+// https://issues.jenkins-ci.org/browse/JENKINS-30176
+public class VisualSVN extends SubversionRepositoryBrowser {
+
+	private static final Logger LOGGER = Logger.getLogger(VisualSVN.class.getName());


oleg-nenashev · 2016-12-16T00:36:01Z

src/main/java/hudson/scm/browsers/VisualSVN.java

+    @Extension
+    public static class DescriptorImpl extends Descriptor<RepositoryBrowser<?>> {
+        public String getDisplayName() {
+            return "VisualSVN";


Good practice: Keep the name localizable. https://wiki.jenkins-ci.org/display/JENKINS/Internationalization

oleg-nenashev · 2016-12-16T00:36:53Z

src/main/java/hudson/scm/browsers/VisualSVN.java

+     * It may contain a query parameter like <tt>?root=foobar</tt>, so relative
+     * URL construction needs to be done with care.</p>
+     */
+    public final String url;


Better to use private field and getter. Otherwise you will be never able to change the data structure without breaking compatibility

oleg-nenashev · 2016-12-16T00:37:24Z

src/main/java/hudson/scm/browsers/VisualSVN.java

+        this.url = validateUrl(url);
+    }
+
+    private String validateUrl(URL url) {


Confusing method name. Nothing is validated here

oleg-nenashev · 2016-12-16T00:39:08Z

src/main/resources/hudson/scm/browsers/VisualSVN/config.jelly

+  <f:entry title="URL" field="url">
+    <f:textbox />
+  </f:entry>
+</j:jelly>


Good practice: add line breaks to the last file lines

Well, there's a lot of comments :-). I just copied some other plugin (comments, maybe XML additional line, etc.) and modified it for VisualSVN.

I wil try to fix it according comments above. It should be simple.

oleg-nenashev · 2016-12-16T00:40:35Z

src/main/java/hudson/scm/browsers/VisualSVN.java

+    	// https://demo-server.visualsvn.com/!/#tortoisesvn/commit/r27333/head/trunk/src/Utils/MiscUI/SciEdit.cpp
+        int r = path.getLogEntry().getRevision();
+    	String value = String.format("%s/commit/r%d/head%s", url, r, path.getValue());
+    	LOGGER.fine("DiffLink URL: " + value);


Better to use formatter to avoid non-required string concatenations

oleg-nenashev · 2016-12-16T00:40:40Z

src/main/java/hudson/scm/browsers/VisualSVN.java

+    @Override public URL getFileLink(Path path) throws IOException {
+    	// https://demo-server.visualsvn.com/!/#tortoisesvn/view/head/svnrobots.txt
+    	String value = String.format("%s/view/head%s", url, path.getValue());
+    	LOGGER.fine("FileLink URL: " + value);


oleg-nenashev · 2016-12-16T00:40:50Z

src/main/java/hudson/scm/browsers/VisualSVN.java

+                                   throws IOException {
+    	// https://demo-server.visualsvn.com/!/#tortoisesvn/commit/r27333
+        String value = String.format("%s/commit/r%d", url, changeSet.getRevision());
+    	LOGGER.fine("ChangeSetLink URL: " + value);


oleg-nenashev · 2016-12-16T00:41:34Z

src/main/java/hudson/scm/browsers/VisualSVN.java

+ * {@link RepositoryBrowser} for Subversion.
+ *
+ * @author Arnost Havelka
+ * @since 2.5.8


Please replace the since value by TODO. It may not be merged into the specified version

arnosthavelka · 2016-12-16T09:42:47Z

I tried to fix all commets, but I am not sure about source formatting (where is it defined) and formatter (I used String.format()).

Is it sufficient? If not, can you provide more details/hints?

oleg-nenashev · 2016-12-16T10:19:32Z

@arnosthavelka

So there are two approaches, which are good from the performance PoV. See the examples below.

Approach 1

if (LOGGER.isLoggable(Level.FINE)) {
    String value = String.format("%s/commit/r%d/head%s", url, r, path.getValue());
    LOGGER.fine("DiffLink URL: " + value);
}

Approach 2

LOGGER.log(Level.FINE, "DiffLink URL: {0}/commit/r{1}/head{2}", new Object[] {url, r, path.getValue()});

arnosthavelka · 2016-12-16T11:55:01Z

LOGGER is fixed --> I was focused on fomatting of the value itself, not the logging the value.

yultide · 2017-01-18T18:10:03Z

@oleg-nenashev any updates on when the merge will happen?

ghost · 2017-06-14T05:55:02Z

Just making some noise: any news on an integration date?

oleg-nenashev · 2017-06-14T06:17:54Z

@yultide @Bebef The plugin has no maintainer. @recena is inactive, and there is a plan to mark the plugin for adoption. See https://wiki.jenkins-ci.org/display/JENKINS/Adopt+a+Plugin for more info.

The only thing I can do here is to review the PR after the last changes

oleg-nenashev

The code will probably work, but the formatting is still messed up.

oleg-nenashev · 2017-06-14T06:19:29Z

src/main/java/hudson/scm/browsers/VisualSVN.java

+ */
+public class VisualSVN extends SubversionRepositoryBrowser {
+
+	private static final Logger LOGGER = Logger.getLogger(VisualSVN.class.getName());


tabs vs. spaces

oleg-nenashev · 2017-06-14T06:21:11Z

src/main/java/hudson/scm/browsers/VisualSVN.java

+    		throw new MalformedURLException(Messages.SubversionSCM_doCheckRemote_invalidUrl());
+    	}
+    	String value = url.toString();
+    	if (value == null || value.equals("")) {


StringUtils.isBlank(). Otherwise you will be accepting blank strings like " "

oleg-nenashev · 2017-06-14T06:21:57Z

src/main/java/hudson/scm/browsers/VisualSVN.java

+     * @throws               MalformedURLException  when URL is not valid (empty)
+     */
+    @DataBoundConstructor
+    public VisualSVN(URL url) throws MalformedURLException {


I am not sure if Stapler handles URLs reliably. CC @daniel-beck and @vivek

Should make the parameter type be String, to match that of the getter. Otherwise this will not be compatible with Pipeline’s Snippet Generator, for example, and might cause mayhem in JEP-201 etc.

Yes, it's better to fix it before merge

oleg-nenashev · 2017-06-14T06:23:10Z

src/main/java/hudson/scm/browsers/VisualSVN.java

+    	// https://demo-server.visualsvn.com/!/#tortoisesvn/view/head/svnrobots.txt
+    	String value = String.format("%s/view/head%s", url, path.getValue());
+    	if (LOGGER.isLoggable(Level.FINE)) {
+    		LOGGER.fine("FileLink URL: " + value);


I am not sure if it is helpful even at the FINE level. Maybe DEBUG?

oleg-nenashev · 2017-06-14T06:23:21Z

src/main/java/hudson/scm/browsers/VisualSVN.java

+    	// https://demo-server.visualsvn.com/!/#tortoisesvn/commit/r27333
+        String value = String.format("%s/commit/r%d", url, changeSet.getRevision());
+        if (LOGGER.isLoggable(Level.FINE)) {
+        	LOGGER.fine("ChangeSetLink URL: " + value);


oleg-nenashev · 2017-06-14T06:23:43Z

src/main/resources/hudson/scm/subversion/Messages.properties

@@ -1,86 +1,88 @@
-# The MIT License
-# 
-# Copyright (c) 2004-2011, Sun Microsystems, Inc., Kohsuke Kawaguchi, Seiji Sogabe, Yahoo! Inc.


Formatting changes messed up the diff here

Thanks for the update! 👍

actual diff

jglick

Probably good enough.

jglick · 2017-06-16T14:30:22Z

src/main/resources/hudson/scm/subversion/Messages.properties

@@ -1,86 +1,88 @@
-# The MIT License
-# 
-# Copyright (c) 2004-2011, Sun Microsystems, Inc., Kohsuke Kawaguchi, Seiji Sogabe, Yahoo! Inc.


actual diff

jglick · 2017-06-16T14:36:09Z

src/main/resources/hudson/scm/browsers/VisualSVN/config.jelly

+
+<?jelly escape-by-default='true'?>
+<j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:d="jelly:define" xmlns:l="/lib/layout" xmlns:t="/lib/hudson" xmlns:f="/lib/form">
+  <f:entry title="URL" field="url">


You forget a getUrl method, so config round-trip does not work.

Sorry, it's not clear to me. Where should I use getUrl method? I was not able to find any usage of it in other config.jelly files.
Please, can you be more specific or fix it? I just did "copy & past " on one browser (don't know which one now). The solution was working correctly as I know.

@arnosthavelka I am not an expert but I think it might be something like here:

<j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:d="jelly:define" xmlns:l="/lib/layout" xmlns:t="/lib/hudson" xmlns:f="/lib/form"> <f:entry title="URL" help="/plugin/WebSVN2/help-url.html"> <f:textbox field="reposUrl" name="webSVN2.url" value="${browser.url}"/> </f:entry> </j:jelly>

I am not sure whether the suggestion by queil is right answer My config.jelly has same content as for SVNWeb, ViewSVN, WebSVN. The other implementation have same <f:entry/> for "URL" content, but additional element <f:entry/> for "Repository".

Should I use this?
<f:entry title="URL" help="/plugin/VisualSVN/help-url.html">

@jglick Please, can you confirm it or clarify your claim?

The advice from @queil is incorrect. The f:entry and empty f:textbox are fine; you just need to add a getUrl getter method to the bean. See Jenkins docs on form databinding. (You could reproduce your bug using one of the JenkinsRule.configRoundtrip overloads if you cared to.)

No help attribute is necessary: you are following the conventional location of help-url.html.

arnosthavelka · 2017-08-16T07:25:52Z

@oleg-nenashev Please, can you remove "merge-conflict" tag? It's already up-to-date.

oleg-nenashev · 2017-08-16T10:03:53Z

removed the label, the PR still needs review feedback

oleg-nenashev · 2017-12-12T09:23:42Z

@jenkinsci/code-reviewers

jglick · 2017-12-12T19:25:09Z

src/main/resources/hudson/scm/browsers/VisualSVN/config.jelly

+
+<?jelly escape-by-default='true'?>
+<j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:d="jelly:define" xmlns:l="/lib/layout" xmlns:t="/lib/hudson" xmlns:f="/lib/form">
+  <f:entry title="URL" field="url">


The advice from @queil is incorrect. The f:entry and empty f:textbox are fine; you just need to add a getUrl getter method to the bean. See Jenkins docs on form databinding. (You could reproduce your bug using one of the JenkinsRule.configRoundtrip overloads if you cared to.)

No help attribute is necessary: you are following the conventional location of help-url.html.

arnosthavelka · 2017-12-14T12:10:49Z

I just merged latest changes and added getter for URL property as adviced by @jglick. It's strange that almost none other class have such getter.

@jglick: Is it OK now?

jglick

Still a mistake in databinding I think.

jglick · 2018-02-08T23:35:46Z

src/main/java/hudson/scm/browsers/VisualSVN.java

+     * @throws               MalformedURLException  when URL is not valid (empty)
+     */
+    @DataBoundConstructor
+    public VisualSVN(URL url) throws MalformedURLException {


Should make the parameter type be String, to match that of the getter. Otherwise this will not be compatible with Pipeline’s Snippet Generator, for example, and might cause mayhem in JEP-201 etc.

jglick · 2018-02-08T23:36:23Z

src/main/resources/hudson/scm/subversion/Messages.properties

@@ -90,4 +90,4 @@ CredentialsSVNAuthenticationProviderImpl.credentials_in_realm=Found credentials
 CredentialsSVNAuthenticationProviderImpl.sole_credentials=Using sole credentials {0} in realm \u2018{1}\u2019
 CredentialsSVNAuthenticationProviderImpl.missing_credentials=No credentials found for realm \u2018{0}\u2019 among \u2018{1}\u2019; falling back to {2}

-
+SubversionSCM.browsers.VisualSVN=VisualSVN


Best to add a trailing newline.

I just merged with latest changes and added trailing newline to the messages.properties file (as requested).

However I'm not sure about the request to change the param from URL to String. Almost every implementation (except Assembla) has it in the same way (as URL). Therefore I propose to do not change the parameter type, since I'm not able to verify the correctness of the change anymore. If it will be problem for Snippet Generator then someone will have to change it in all others implementations anyway.

Please, can we finish it now? We are discussing PR for 2 years now.

Unfortunately it's what we have for barely maintained plugins.
I and @jglick spent some time on cleaning up the PR queue this summer, but there were other maintainers before.
@kuisathaverat was interested in taking ownership of the plugin tho

@arnosthavelka I made a PR with the suggested pending changes, merge it on your branch and I think that this PR is ready to merge

arnosthavelka#1

arnosthavelka · 2018-03-02T07:07:16Z

@jglick Please, can you check my comment?

oleg-nenashev

1 change is required in the constructor the rest looks good to me, would be great to finally merge it

oleg-nenashev · 2018-03-04T00:23:25Z

src/main/java/hudson/scm/browsers/VisualSVN.java

+     * @throws               MalformedURLException  when URL is not valid (empty)
+     */
+    @DataBoundConstructor
+    public VisualSVN(URL url) throws MalformedURLException {


Yes, it's better to fix it before merge

oleg-nenashev · 2018-03-04T00:24:53Z

src/main/resources/hudson/scm/browsers/VisualSVN/config.jelly

+
+<?jelly escape-by-default='true'?>
+<j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:d="jelly:define" xmlns:l="/lib/layout" xmlns:t="/lib/hudson" xmlns:f="/lib/form">
+  <f:entry title="URL" field="url">


oleg-nenashev · 2018-03-04T00:26:59Z

src/main/resources/hudson/scm/subversion/Messages.properties

@@ -90,4 +90,4 @@ CredentialsSVNAuthenticationProviderImpl.credentials_in_realm=Found credentials
 CredentialsSVNAuthenticationProviderImpl.sole_credentials=Using sole credentials {0} in realm \u2018{1}\u2019
 CredentialsSVNAuthenticationProviderImpl.missing_credentials=No credentials found for realm \u2018{0}\u2019 among \u2018{1}\u2019; falling back to {2}

-
+SubversionSCM.browsers.VisualSVN=VisualSVN


Unfortunately it's what we have for barely maintained plugins.
I and @jglick spent some time on cleaning up the PR queue this summer, but there were other maintainers before.
@kuisathaverat was interested in taking ownership of the plugin tho

@jglick

[visualsvn] changes sugested by @jglick and @oleg-nenashev

oleg-nenashev

Let's ship it

…on-plugin into visualsvn

arnosthavelka · 2018-04-19T18:04:55Z

What's the status? To avoid merging additional changes :-).

kuisathaverat · 2018-04-20T08:02:31Z

@arnosthavelka it is my fault, I think I have merged it

+ [JENKINS-30176] Repository browser VisualSVN added

c00862e

Merge remote-tracking branch 'upstream/master' into visualsvn

abe54a9

oleg-nenashev reviewed Dec 16, 2016

View reviewed changes

arnosthavelka added 2 commits December 16, 2016 09:24

Merge remote-tracking branch 'upstream/master' into visualsvn

528186b

- fixes according code review

430a11f

logging fixed

19344ed

oleg-nenashev reviewed Jun 14, 2017

View reviewed changes

jglick approved these changes Jun 16, 2017

View reviewed changes

jglick requested changes Jun 16, 2017

View reviewed changes

oleg-nenashev added the merge-conflict label Jun 16, 2017

oleg-nenashev mentioned this pull request Jun 16, 2017

[JENKINS-30176] Add new VisualSVN Browser to support the to be released VisualSVN Web Interface #104

Closed

arnosthavelka added 3 commits June 19, 2017 16:32

Merge branch 'master' into visualsvn

bd03cf0

- fix of wrong merge

04c5811

Merge remote-tracking branch 'upstream/master' into visualsvn

a50f5c5

oleg-nenashev removed the merge-conflict label Aug 16, 2017

oleg-nenashev requested review from jglick and oleg-nenashev December 12, 2017 09:23

jglick requested changes Dec 12, 2017

View reviewed changes

arnosthavelka added 2 commits December 14, 2017 12:46

Merge remote-tracking branch 'upstream/master' into visualsvn

c712571

+ getter for URL property added (as recommended)

f500ac1

oleg-nenashev requested a review from jglick February 2, 2018 15:05

jglick requested changes Feb 8, 2018

View reviewed changes

arnosthavelka added 2 commits February 18, 2018 18:37

Merge remote-tracking branch 'upstream/master' into visualsvn

8d73400

+ trailing line added in messages.properties

8066914

oleg-nenashev requested a review from jglick March 4, 2018 00:22

oleg-nenashev requested changes Mar 4, 2018

View reviewed changes

kuisathaverat and others added 2 commits March 4, 2018 16:34

[visualsvn] changes sugested by @jglick and @oleg-nenashev

93bdb52

Merge pull request #1 from kuisathaverat/visualsvn

2c28d60

[visualsvn] changes sugested by @jglick and @oleg-nenashev

oleg-nenashev approved these changes Mar 5, 2018

View reviewed changes

jglick approved these changes Mar 5, 2018

View reviewed changes

arnosthavelka added 2 commits April 13, 2018 07:49

Merge remote-tracking branch 'upstream/master' into visualsvn

8ac230b

Merge branch 'visualsvn' of https://github.com/arnosthavelka/subversi…

d8dab37

…on-plugin into visualsvn

kuisathaverat merged commit 52b7a92 into jenkinsci:master Apr 20, 2018

+ [JENKINS-30176] Repository browser VisualSVN added #163

+ [JENKINS-30176] Repository browser VisualSVN added #163

Conversation

arnosthavelka commented Jun 16, 2016

arnosthavelka commented Jun 16, 2016 • edited Loading

arnosthavelka commented Jul 15, 2016

arnosthavelka commented Sep 2, 2016

yultide commented Dec 16, 2016

oleg-nenashev 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

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

Choose a reason for hiding this comment

arnosthavelka commented Dec 16, 2016

oleg-nenashev commented Dec 16, 2016

arnosthavelka commented Dec 16, 2016

yultide commented Jan 18, 2017

ghost commented Jun 14, 2017

oleg-nenashev commented Jun 14, 2017

oleg-nenashev 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

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

jglick 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

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

arnosthavelka commented Aug 16, 2017

oleg-nenashev commented Aug 16, 2017

oleg-nenashev commented Dec 12, 2017

Choose a reason for hiding this comment

arnosthavelka commented Dec 14, 2017

jglick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arnosthavelka Feb 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arnosthavelka commented Mar 2, 2018

oleg-nenashev 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

oleg-nenashev left a comment

Choose a reason for hiding this comment

arnosthavelka commented Apr 19, 2018

kuisathaverat commented Apr 20, 2018

arnosthavelka commented Jun 16, 2016 •

edited

Loading

arnosthavelka Feb 18, 2018 •

edited

Loading