Skip to content

removed command line arguments escape logic#2005

Merged
arturcic merged 1 commit intoGitTools:mainfrom
denisbredikhin:argument-escaping-fix
Mar 16, 2026
Merged

removed command line arguments escape logic#2005
arturcic merged 1 commit intoGitTools:mainfrom
denisbredikhin:argument-escaping-fix

Conversation

@denisbredikhin
Copy link
Copy Markdown
Contributor

This pull request fixes an issue introduced in PR #1988, where exec was replaced with execFile but the existing argument-escaping logic was left in place. Since execFile passes arguments directly to the process without invoking a shell, the previous escaping logic caused arguments to include literal quotes and backslashes. On Windows, this resulted in invalid path resolutions and errors like:

The filename, directory name, or volume label syntax is incorrect.

This update removes all manual argument escaping so that execFile receives the correct raw argument values.

My apologies for the regression introduced in the earlier PR.

Copilot AI review requested due to automatic review settings March 16, 2026 10:15
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Removes manual command-line argument quoting/escaping from ArgumentsBuilder now that tools are executed via execFile, which passes arguments directly to the process (avoiding the regression where literal quotes/backslashes broke Windows paths).

Changes:

  • Stop escaping/quoting values for addArgument, addKeyValue, addKeyValueEquals, and addCommaList.
  • Remove the OS-specific escape/needsEscaping helpers.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@arturcic
Copy link
Copy Markdown
Member

@denisbredikhin please check this one as well #2006

@denisbredikhin
Copy link
Copy Markdown
Contributor Author

@denisbredikhin please check this one as well #2006

I think changes in this PR should fix that issue too.
Maybe it would be good to change exception message from

throw new WarningException($"Could not parse /overrideconfig option: {keyValueOption}. Unsupported 'key'.");

to something like

throw new WarningException($"Could not parse /overrideconfig option: {keyValueOption}. Unsupported key {optionKey}.");

If you agree can you open a new issue at GitTools/GitVersion?

@arturcic
Copy link
Copy Markdown
Member

fine by me open an issue and a PR if you don't mind

@arturcic
Copy link
Copy Markdown
Member

@denisbredikhin hey when you're done with the changes please squash them as a commit

@denisbredikhin
Copy link
Copy Markdown
Contributor Author

@denisbredikhin hey when you're done with the changes please squash them as a commit

won't I be able to "Squash and merge" when you approve?

@arturcic
Copy link
Copy Markdown
Member

@denisbredikhin hey when you're done with the changes please squash them as a commit

won't I be able to "Squash and merge" when you approve?

we usually do not enable that option, so better you do that, we then merge

@denisbredikhin denisbredikhin force-pushed the argument-escaping-fix branch from 55caa58 to bc42486 Compare March 16, 2026 13:26
@sonarqubecloud
Copy link
Copy Markdown

@arturcic arturcic linked an issue Mar 16, 2026 that may be closed by this pull request
2 tasks
@arturcic arturcic merged commit e0d584e into GitTools:main Mar 16, 2026
15 checks passed
@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Mar 16, 2026

Thank you @denisbredikhin for your contribution!

@denisbredikhin
Copy link
Copy Markdown
Contributor Author

@arturcic I see so .vsix file in the assets of the release.
And marketplace is not updated...

@arturcic
Copy link
Copy Markdown
Member

We have a 2step release process, first it goes to a gittools.gittools-test extension which runs on another repository to validate it did not broke, and then goes the official, wait couple of minutes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[ISSUE]: Upgrade GitVersionTask from 4.3.3 to 4.4.1 breaks /overrideconfig

4 participants