[#1657] Escape special chars for bash commands #1662

chan-j-d · 2022-02-13T10:58:34Z

Currently, the file path arguments are added to git commands using normal double quotes. However, for Bash terminals, double quotes still allow for special symbols like $ to perform a special function like referencing a variable name. There is a need to use single quotes for Bash terminals. Single quotes ' themselves are escaped as such '"'"' by breaking up the string into various quoted segments where a single quote is left in a double quoted segment as per this stackoverflow post.

The test case specifically tests against special bash characters according to this unix stackexchange post.

Unfortunately, Windows CMD does not work with single quotes and only accepts double quotes. As such, there needs to be a distinction in the way this is done for different OS.

A small note was added to the DG of the importance in using this additional method.

Commit message:

Special characters in file names are not properly translated into git
commands on bash for non-Windows OS. Usage of single quotes on bash is
only necessary to escape such special characters.

Let's fix the escaping of special characters for bash commands.

zhoukerrr

I think it is trivial but maybe we can cover the case on that line. Otherwise LGTM.

zhoukerrr · 2022-02-13T12:52:01Z

src/main/java/reposense/util/StringsUtil.java

+     */
+    public static String addQuotesForFilePath(String filePath) {
+        if (SystemUtil.isWindows()) {
+            return "\"" + filePath + "\"";


Is it possible to cover this line in the test as well?

Unfortunately, I don't think there is a good way to test the escaping specifically. We can add a test case just to check if quotation marks are added which does not seem very useful. For this case, it was to avoid mishandling of spaces in the repo directory and the test cases are located here.

For testing the escaping specifically, I don't think there's a good way to test this for Windows filesystems. The CMD's echo command echoes the quotation marks compared to bash's echo command which strips the quotation marks and does symbol substitution where appropriate.

Was this line missed just because the coverage report wasn't generated on Windows?

I think it's more that there isn't a specific test case that runs this line of code at all since the test case I added only runs on non-Windows systems.

gok99 · 2022-02-13T13:24:20Z

src/main/java/reposense/util/StringsUtil.java

+    /**
+     * Adds the appropriate quotation marks for a file path depending on the OS.
+     */
+    public static String addQuotesForFilePath(String filePath) {


It might be possible that developers forget to use these if they weren't sure how or when to use them. Do you think it would be a good idea now to create a CommandBuilder to take care of these things?

This could be tricky though, given that the construction of commands can be non-trivial so this might be out of scope for this PR.

Interesting. I actually voiced a similar idea here and you can read the response from dcshzj but besides adding paths, most other argument types are quite tailored to the specific git command. So this is a specific standard for file paths only.

Thanks for the link! It's interesting seeing similar discussions and considerations springing up independently at different points in this project :)

src/test/java/reposense/util/StringsUtilTest.java

src/main/java/reposense/util/StringsUtil.java

src/test/java/reposense/util/StringsUtilTest.java

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

gok99

LGTM!

docs/dg/architecture.md

dcshzj

LGTM!

chan-j-d · 2022-02-21T05:05:09Z

@dcshzj @fzdy1914 would it be possible to get a final review sooner or merge it earlier? I think the StringsUtil::addQuotesForFilePath method is necessary for issue #1682 which is further exacerbated in PR #1644 when the output directory now retains information of the entire path that might have more spaces.

dcshzj · 2022-02-21T11:59:55Z

I will move this PR along since it is necessary for some of the other PRs.

github-actions · 2022-02-24T02:24:27Z

The following links are for previewing this pull request:

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

chan-j-d added 15 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

dcshzj requested a review from a team February 13, 2022 11:02

dcshzj added the s.ToReview label Feb 13, 2022

chan-j-d requested a review from a team February 13, 2022 11:12

dcshzj removed the request for review from a team February 13, 2022 12:27

zhoukerrr reviewed Feb 13, 2022

View reviewed changes

gok99 reviewed Feb 13, 2022

View reviewed changes

Add comment containing the link referencing the issue

4cd9fd1

chan-j-d requested a review from gok99 February 13, 2022 14:15

gok99 requested changes Feb 13, 2022

View reviewed changes

src/main/java/reposense/util/StringsUtil.java Show resolved Hide resolved

src/test/java/reposense/util/StringsUtilTest.java Show resolved Hide resolved

chan-j-d and others added 3 commits February 14, 2022 01:09

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

gok99 approved these changes Feb 13, 2022

View reviewed changes

gok99 requested a review from a team February 13, 2022 17:24

dcshzj requested changes Feb 14, 2022

View reviewed changes

docs/dg/architecture.md Outdated Show resolved Hide resolved

chan-j-d and others added 5 commits February 14, 2022 12:39

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

chan-j-d requested a review from dcshzj February 14, 2022 06:15

dcshzj approved these changes Feb 14, 2022

View reviewed changes

dcshzj requested a review from a team February 14, 2022 08:13

dcshzj added s.FinalReview and removed s.ToReview labels Feb 14, 2022

chan-j-d and others added 2 commits February 21, 2022 12:44

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

44ebaf1

Fix method name

c4dc164

chan-j-d mentioned this pull request Feb 21, 2022

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

Merged

dcshzj added s.ToMerge and removed s.FinalReview labels Feb 21, 2022

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

693cdf7

dcshzj changed the title ~~[#1657] Escape special chars for Bash commands~~ [#1657] Escape special chars for bash commands Feb 24, 2022

dcshzj merged commit 4923449 into reposense:master Feb 24, 2022

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

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

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#1657] Escape special chars for bash commands #1662

[#1657] Escape special chars for bash commands #1662

chan-j-d commented Feb 13, 2022 •

edited by dcshzj

Loading

zhoukerrr left a comment

zhoukerrr Feb 13, 2022

chan-j-d Feb 13, 2022

gok99 Feb 13, 2022

chan-j-d Feb 13, 2022

gok99 Feb 13, 2022 •

edited

Loading

chan-j-d Feb 13, 2022 •

edited

Loading

gok99 Feb 13, 2022

gok99 left a comment

dcshzj left a comment

chan-j-d commented Feb 21, 2022

dcshzj commented Feb 21, 2022

github-actions bot commented Feb 24, 2022

[#1657] Escape special chars for bash commands #1662

[#1657] Escape special chars for bash commands #1662

Conversation

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

zhoukerrr left a comment

Choose a reason for hiding this comment

zhoukerrr Feb 13, 2022

Choose a reason for hiding this comment

chan-j-d Feb 13, 2022

Choose a reason for hiding this comment

gok99 Feb 13, 2022

Choose a reason for hiding this comment

chan-j-d Feb 13, 2022

Choose a reason for hiding this comment

gok99 Feb 13, 2022 • edited Loading

Choose a reason for hiding this comment

chan-j-d Feb 13, 2022 • edited Loading

Choose a reason for hiding this comment

gok99 Feb 13, 2022

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

chan-j-d commented Feb 21, 2022

dcshzj commented Feb 21, 2022

github-actions bot commented Feb 24, 2022

chan-j-d commented Feb 13, 2022 •

edited by dcshzj

Loading

gok99 Feb 13, 2022 •

edited

Loading

chan-j-d Feb 13, 2022 •

edited

Loading