[#1682] Fix space in local repo causing cloning exception #1685

chan-j-d · 2022-02-21T07:37:36Z

Currently, clone output paths are not quoted. The issue is rare as it can only occur against local repositories where the final directory contains a space.

This PR also includes changes to the existing GitClone::cloneFromBareAndUpdateBranch method. It used to be the only method that constructs its own git command without the use of a helper private method which is likely why it was missed and none of its directories are quoted. This change aims to standardize the format between different git clone methods with

The method handling the logic of setting up and running the command has to create its own 'command constructor' method to create the String. This way, it is easy to standardize changes to the command formats by looking at these methods without having to see what they are used for.
Defaulting the directory in which the commands are run to the current directory (location the user invokes java -jar RepoSense.jar). As of this PR, the cloneFromBareAndUpdateBranch is the only one that does not run the command from the current directory. This is the result of [#1546] RepoCloner: fix local repos relative pathing and spaces #1562. Before that, commands were run from the cloned repo directory but the commands themselves were constructed as if they were in the working directory. This caused issues when relative pathing was used. The fix changed all but one of the use of git clone to start from the working directory. Only the cloneFromBareAndUpdateBranch method was not changed as it seemed to break other things.

Commit message:

Output directories are not quoted which can lead to errors. The 
GitClone::cloneFromBareAndUpdateBranch method is also not standardized
to match other clone methods which always run from the working
directory. This can be difficult to maintain.

Let’s fix the issue of spaces in repo directory causing clone failure
and standardize the cloneFromBareAndUpdateBranch method.

…m:chan-j-d/RepoSense into 1657-escape-special-chars-in-bash-command

gok99 · 2022-03-05T15:59:42Z

src/main/java/reposense/git/GitClone.java

@@ -160,7 +160,7 @@ public static void cloneBare(RepoConfiguration config, Path rootPath,
     */
    public static void cloneFromBareAndUpdateBranch(Path rootPath, RepoConfiguration config)
            throws GitCloneException, IOException {
-        Path relativePath = rootPath.relativize(FileUtil.getBareRepoPath(config));
+        Path relativePath = FileUtil.getBareRepoPath(config);


As of this PR, the cloneFromBareAndUpdateBranch is the only one that does not run the command from the current directory.

Can I clarify if cloneFromBareAndUpdateBranch is also run from the current directory with the removal of relativize? Could include details about this change in the commit message as well.

The way it works at the moment is somewhat strange. Suppose our working directory is /directory/, then the rootPath here (in the only instance where this method is used) is in /directory/repos/ where repos is a hard-coded constant designated for where the cloned repos go.

FileUtil::getBareRepoPath would construct something like repos/reposense/reposense_bare. Code below:

RepoSense/src/main/java/reposense/util/FileUtil.java

Lines 254 to 257 in c656433

public static Path getBareRepoPath(RepoConfiguration config) {

return Paths.get(FileUtil.REPOS_ADDRESS,

config.getRepoFolderName(), config.getRepoName() + BARE_REPO_SUFFIX);

}

So the relativize ends up trying to relativize the paths /directory/repos/ and repos/reposense/reposense_bare which settles on /directory/repos/reposense/reposense_bare.

The ReportGenerator class was changed together with the above code and now runs from current directory, /directory/ and then adds on the entire bare repo path to the back. Functionally, it is the same.

I have updated it in the commit message as well.

gok99 · 2022-03-05T16:03:00Z

src/main/java/reposense/git/GitClone.java

@@ -55,7 +55,7 @@ public static CommandRunnerProcess cloneShallowBareAsync(RepoConfiguration confi
            String outputFolderName, LocalDateTime sinceDate) throws GitCloneException {
        try {
            return CommandRunner.runCommandAsync(rootPath,
-                                                    getCloneShallowBareCommand(config, outputFolderName, sinceDate));
+                    getCloneShallowBareCommand(config, outputFolderName, sinceDate));


The tests for GitClone seems lacking compared to the other Git classes. Perhaps we could figure out a way to test the various clone variations (in a separate issue).

In a strange coincidence, I did voice the same thought some time back and the response is here #1562 (comment) under point 2.

I think in general, cloning is quite expensive so they can be done together with the system tests. If the system tests work, then the cloning likely works with it. There is an argument to be made for cloneFromBareAndUpdateBranch which is a local-to-local clone done while analyzing but I'm not sure how we would go about testing it.

I think it makes sense to test these with the system tests, since the issues seem to be with system-specific quirks (although I'm not sure how much of GitClone is covered by the systemtests). I think the linked comment is probably right about tests in that we can only go to the extent of ensuring that the methods return the correct Strings and throw the right exceptions.

gok99

LGTM!

src/main/java/reposense/git/GitClone.java

dcshzj

LGTM!

…ception

github-actions · 2022-03-25T10:23:24Z

The following links are for previewing this pull request:

Dashboard Preview: https://dashboard-1685-pr-reposense-reposense.surge.sh
Docs Preview: https://docs-1685-pr-reposense-reposense.surge.sh

chan-j-d and others added 29 commits February 12, 2022 13:15

Change stringsutil

927ce3b

Change to single quote for linux

294a023

Change filepath quotation methods

496ba31

Change one additional addQuotes to addQuotesForFiles

adc344d

Fix checkstyle issues in main

c26250d

Add bash test case

7bd74f5

Change quotation logic

342dc02

Change test case

c029c58

Adjust bash test case

a55f2d3

Add println

e4af539

Remove println statements

c4d9a0b

Add println tests

51ff1a2

Fix test case once and for all

34ec18c

Add note about path argument to DG

e0402d6

Fix checkstyle

d6b1711

Add comment containing the link referencing the issue

4cd9fd1

Change to answer link

7765314

Merge branch 'master' into 1657-escape-special-chars-in-bash-command

712169d

Merge branch '1657-escape-special-chars-in-bash-command' of github.co…

124d39c

…m:chan-j-d/RepoSense into 1657-escape-special-chars-in-bash-command

Change notification to use markbind box instead

57e5d6e

Update architecture.md

81c9e81

Revert without backslash

9734f85

Change to two separate code strings without colon

0f2cb5a

Add new line between box header

38ab0f1

Merge branch 'master' into 1657-escape-special-chars-in-bash-command

44ebaf1

Fix method name

c4dc164

Change command pathing

294fc9e

Remove relativise

16c7db6

Fix merge issues

ed9df69

chan-j-d requested a review from a team February 24, 2022 05:10

chan-j-d added the s.ToReview label Feb 24, 2022

chan-j-d marked this pull request as ready for review February 24, 2022 05:11

gok99 reviewed Mar 5, 2022

View reviewed changes

gok99 approved these changes Mar 5, 2022

View reviewed changes

chan-j-d requested a review from a team March 5, 2022 17:48

chan-j-d mentioned this pull request Mar 13, 2022

[#1572] Accept different types of URLs besides https github #1644

Merged

3 tasks

zhoukerrr reviewed Mar 16, 2022

View reviewed changes

src/main/java/reposense/git/GitClone.java Show resolved Hide resolved

dcshzj approved these changes Mar 23, 2022

View reviewed changes

dcshzj added s.ToMerge and removed s.ToReview labels Mar 23, 2022

Merge branch 'master' into 1682-space-in-local-repo-causes-cloning-ex…

8ba5719

…ception

dcshzj merged commit 7e4d6c0 into reposense:master Mar 25, 2022

chan-j-d deleted the 1682-space-in-local-repo-causes-cloning-exception branch March 25, 2022 11:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#1682] Fix space in local repo causing cloning exception #1685

[#1682] Fix space in local repo causing cloning exception #1685

chan-j-d commented Feb 21, 2022 •

edited by dcshzj

Loading

gok99 Mar 5, 2022 •

edited

Loading

chan-j-d Mar 5, 2022 •

edited

Loading

gok99 Mar 5, 2022 •

edited

Loading

chan-j-d Mar 5, 2022

gok99 Mar 5, 2022 •

edited

Loading

gok99 left a comment

dcshzj left a comment

github-actions bot commented Mar 25, 2022

	public static Path getBareRepoPath(RepoConfiguration config) {
	return Paths.get(FileUtil.REPOS_ADDRESS,
	config.getRepoFolderName(), config.getRepoName() + BARE_REPO_SUFFIX);
	}

[#1682] Fix space in local repo causing cloning exception #1685

[#1682] Fix space in local repo causing cloning exception #1685

Conversation

chan-j-d commented Feb 21, 2022 • edited by dcshzj Loading

gok99 Mar 5, 2022 • edited Loading

Choose a reason for hiding this comment

chan-j-d Mar 5, 2022 • edited Loading

Choose a reason for hiding this comment

gok99 Mar 5, 2022 • edited Loading

Choose a reason for hiding this comment

chan-j-d Mar 5, 2022

Choose a reason for hiding this comment

gok99 Mar 5, 2022 • edited Loading

Choose a reason for hiding this comment

gok99 left a comment

Choose a reason for hiding this comment

dcshzj left a comment

Choose a reason for hiding this comment

github-actions bot commented Mar 25, 2022

chan-j-d commented Feb 21, 2022 •

edited by dcshzj

Loading

gok99 Mar 5, 2022 •

edited

Loading

chan-j-d Mar 5, 2022 •

edited

Loading

gok99 Mar 5, 2022 •

edited

Loading

gok99 Mar 5, 2022 •

edited

Loading