Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: permissive git diff parameter #185

Merged
merged 9 commits into from
Sep 10, 2021
Merged

feat: permissive git diff parameter #185

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
7c0fa4f
feat: create "permissive git diff" parameter
nvuillam Aug 19, 2021
e7497ef
fix: parameter naming convention
scolladon-sfdc Aug 30, 2021
01c8b6e
feat: add permissive diff to inFile git comparison
scolladon-sfdc Aug 30, 2021
2e14e59
feat: add permissive flag to legacy command line
scolladon-sfdc Aug 30, 2021
3b96fef
style: parameter description
scolladon-sfdc Aug 30, 2021
faa1d0e
docs: add new parameter to the README
scolladon-sfdc Aug 30, 2021
8653e1f
test: permissive flag behavior
scolladon-sfdc Aug 30, 2021
2f9d750
feat: rename parameter to `--ignore-whitespace`
scolladon-sfdc Sep 3, 2021
db0dc0c
fix: command line argument usage
scolladon-sfdc Sep 7, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,20 +93,24 @@ If you encounter this issue while having installed the correct version of node o
## How to use it?

<!-- commands -->
* [`sfdx sgd:source:delta -f <string> [-t <string>] [-r <filepath>] [-i <filepath>] [-D <filepath>] [-s <filepath>] [-o <filepath>] [-a <number>] [-d] [--json] [--loglevel trace|debug|info|warn|error|fatal|TRACE|DEBUG|INFO|WARN|ERROR|FATAL]`](#sfdx-sgdsourcedelta--f-string--t-string--r-filepath--i-filepath--d-filepath--s-filepath--o-filepath--a-number--d---json---loglevel-tracedebuginfowarnerrorfataltracedebuginfowarnerrorfatal)
* [`sfdx sgd:source:delta -f <string> [-t <string>] [-r <filepath>] [-i <filepath>] [-D <filepath>] [-s <filepath>] [-W] [-o <filepath>] [-a <number>] [-d] [--json] [--loglevel trace|debug|info|warn|error|fatal|TRACE|DEBUG|INFO|WARN|ERROR|FATAL]`](#sfdx-sgdsourcedelta--f-string--t-string--r-filepath--i-filepath--d-filepath--s-filepath--w--o-filepath--a-number--d---json---loglevel-tracedebuginfowarnerrorfataltracedebuginfowarnerrorfatal)

## `sfdx sgd:source:delta -f <string> [-t <string>] [-r <filepath>] [-i <filepath>] [-D <filepath>] [-s <filepath>] [-o <filepath>] [-a <number>] [-d] [--json] [--loglevel trace|debug|info|warn|error|fatal|TRACE|DEBUG|INFO|WARN|ERROR|FATAL]`
## `sfdx sgd:source:delta -f <string> [-t <string>] [-r <filepath>] [-i <filepath>] [-D <filepath>] [-s <filepath>] [-W] [-o <filepath>] [-a <number>] [-d] [--json] [--loglevel trace|debug|info|warn|error|fatal|TRACE|DEBUG|INFO|WARN|ERROR|FATAL]`

Generate the sfdx content in source format and destructive change from two git commits

```
USAGE
$ sfdx sgd:source:delta -f <string> [-t <string>] [-r <filepath>] [-i <filepath>] [-D <filepath>] [-s <filepath>] [-o
<filepath>] [-a <number>] [-d] [--json] [--loglevel
$ sfdx sgd:source:delta -f <string> [-t <string>] [-r <filepath>] [-i <filepath>] [-D <filepath>] [-s <filepath>] [-W]
[-o <filepath>] [-a <number>] [-d] [--json] [--loglevel
trace|debug|info|warn|error|fatal|TRACE|DEBUG|INFO|WARN|ERROR|FATAL]

OPTIONS
-D, --ignore-destructive=ignore-destructive ignore file to use

-W, --ignore-whitespace ignore git diff whitespace (space,
tab, eol) changes

-a, --api-version=api-version [default: 52] salesforce API version

-d, --generate-delta generate delta files in [--output]
Expand Down
16 changes: 15 additions & 1 deletion __tests__/unit/lib/utils/fileGitDiff.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,21 @@ describe(`test if fileGitDiff`, () => {
expect(work).toStrictEqual(output)
})

test('can parse git diff contexte line', () => {
test('can apply permissive git diff', () => {
const output = 'diff'

child_process.spawnSync.mockImplementation(() => ({
stdout: output,
}))
const work = fileGitDiff(TEST_PATH, {
output: '',
repo: '',
ignoreWhitespace: true,
})
expect(work).toStrictEqual(output)
})

test('can parse git diff context line', () => {
const output = 'context line'

child_process.spawnSync.mockImplementation(() => ({
Expand Down
18 changes: 18 additions & 0 deletions __tests__/unit/lib/utils/repoGitDiff.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,24 @@ describe(`test if repoGitDiff`, () => {
expect(work).toStrictEqual(output)
})

test('can parse git permissively', () => {
const output = []
child_process.spawnSync.mockImplementation(() => ({
stdout: '',
}))
const work = repoGitDiff(
{
output: '',
repo: '',
ignore: FORCEIGNORE_MOCK_PATH,
ignoreWhitespace: true,
},
// eslint-disable-next-line no-undef
globalMetadata
)
expect(work).toStrictEqual(output)
})

test('can resolve deletion', () => {
const output = [
'D force-app/main/default/objects/Account/fields/awesome.field-meta.xml',
Expand Down
1 change: 1 addition & 0 deletions bin/cli
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ const options = program
messages.sourceFlag,
CliHelper.SOURCE_DEFAULT_VALUE
)
.option('-W, --ignore-whitespace', messages.ignoreWhitespaceFlag)
.option('-i, --ignore [file]', messages.ignoreFlag)
.option('-D, --ignore-destructive [file]', messages.ignoreDestructiveFlag)
.option(
Expand Down
1 change: 1 addition & 0 deletions messages/delta.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,4 +11,5 @@ module.exports = {
ignoreDestructiveFlag: 'ignore file to use',
apiVersionFlag: 'salesforce API version',
deltaFlag: 'generate delta files in [--output] folder',
ignoreWhitespaceFlag: 'ignore git diff whitespace (space, tab, eol) changes',
}
5 changes: 5 additions & 0 deletions src/commands/sgd/source/delta.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,10 @@ export default class SourceDeltaGenerate extends SfdxCommand {
description: messages.getMessage('sourceFlag'),
default: CliHelper.SOURCE_DEFAULT_VALUE,
}),
'ignore-whitespace': flags.boolean({
char: 'W',
description: messages.getMessage('ignoreWhitespaceFlag'),
}),
output: flags.filepath({
char: 'o',
description: messages.getMessage('outputFlag'),
Expand Down Expand Up @@ -78,6 +82,7 @@ export default class SourceDeltaGenerate extends SfdxCommand {
ignoreDestructive: this.flags['ignore-destructive'],
apiVersion: this.flags['api-version'],
repo: this.flags.repo,
ignoreWhitespace: this.flags['ignore-whitespace'],
generateDelta: this.flags['generate-delta'],
})
output.warnings = jobResult?.warnings?.map(warning => warning.message)
Expand Down
12 changes: 11 additions & 1 deletion src/utils/fileGitDiff.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,19 @@ const gc = require('./gitConstants')
const unitDiffParams = ['--no-pager', 'diff', '--no-prefix', '-U200']

module.exports = (filePath, config) => {
const ignoreWhitespaceParams = config.ignoreWhitespace
? gc.IGNORE_WHITESPACE_PARAMS
: []
const { stdout: diff } = childProcess.spawnSync(
'git',
[...unitDiffParams, config.from, config.to, '--', filePath],
[
...unitDiffParams,
...ignoreWhitespaceParams,
config.from,
config.to,
'--',
filePath,
],
{ cwd: config.repo, encoding: gc.UTF8_ENCODING }
)

Expand Down
6 changes: 6 additions & 0 deletions src/utils/gitConstants.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,11 @@ module.exports.ADDITION = 'A'
module.exports.DELETION = 'D'
module.exports.GIT_DIFF_TYPE_REGEX = /^.\s+/u
module.exports.MINUS = '-'
module.exports.IGNORE_WHITESPACE_PARAMS = [
'--ignore-all-space',
'--ignore-blank-lines',
'--ignore-cr-at-eol',
'--word-diff-regex',
Copy link
Collaborator

Choose a reason for hiding this comment

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

@nvuillam According to the git documentation, the --word-diff-regex parameter expects a <regex> value.
Otherwise it may apply a regex that is set locally in your gitattributes or git-config (extract from doc: "The regex can also be set via a diff driver or configuration option, see gitattributes[5] or git-config[1]. Giving it explicitly overrides any diff driver or configuration setting.").

To make sure the behaviour of this command is consistent for all SGD users, what do you think about specifying explicitly the regex to use? In other words, I suggesting to modify line 10 to specify explicitly the <regex> in --word-diff-regex=<regex> (I guess you could look into you gitattributes and git-config to retrieve the value that you used during your tests).

@scolladon To go further, in addition to specifying explicitly the default regex to apply, we could even offer the ability for the users to override it with their own regex (but that would a "nice-to-have" feature, while ensure a consistent behaviour in the command in more important IMHO).

Copy link
Owner

@scolladon scolladon Sep 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@mehdisfdc I love the idea to let the user override the default regex, I will implement it with the new parameter name (once everyone is ok) and with the default regex value

Copy link
Contributor Author

Choose a reason for hiding this comment

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

More flexibility for the user, I fully agree, great suggestion :)

Copy link
Owner

Choose a reason for hiding this comment

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

I bounce on @mehdisfdc's message, @nvuillam could you give us the git config taken by --word-diff-regex?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm sorry, I just used the parameter and I think I viewed a difference compared to without, but i've not sent any value to this parameter

Copy link
Owner

Choose a reason for hiding this comment

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

Could you retry on your side to make sure it does something ?
And if it does something on your side, as you don't use any regex with it, could you send us your gitattributes and git config please ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't have any .gitattributes defined

About git config, except user.name & user.email, I use

  • core.longpaths=true
  • core.quotepath=false

I'll try to make new tests, but unfortunately my work week is already busy, it may wait until next week :/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But why not publishing as it is now, and send --word-diff-regex to git if --word-diff-regex is sent to sfdx-git-delta, and append --word-diff-regex value only if it is sent ? :)

Copy link
Owner

Choose a reason for hiding this comment

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

I prefer not to do that because if no parameter are given to --word-diff-regex then it uses the local configuration in .gitattributes and .git/config, which can be anything and could lead to unexpected behaviour

Copy link
Owner

@scolladon scolladon Sep 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

So, I poced around and play a little bit with it.
It is also true for me that --word-diff-regex is required to have the right behavior.
I dig a little bit more in the documentation of this parameter in git and I found that there are default regex used (which are appened to the override foundable in .gitattributes and .git/config)

I wonder if we will have issues about this and how it will be to debug 😅 but I don't think it will be used by most of the users.
With that knowledge, it looks good to me 😄 for the version without the regex passed in arguments

]
module.exports.PLUS = '+'
module.exports.UTF8_ENCODING = 'utf8'
11 changes: 10 additions & 1 deletion src/utils/repoGitDiff.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,18 @@ const lcSensitivity = {
}

module.exports = (config, metadata) => {
const ignoreWhitespaceParams = config.ignoreWhitespace
? gc.IGNORE_WHITESPACE_PARAMS
: []
const { stdout: diff } = childProcess.spawnSync(
'git',
[...fullDiffParams, config.from, config.to, config.source],
[
...fullDiffParams,
...ignoreWhitespaceParams,
config.from,
config.to,
config.source,
],
{ cwd: config.repo, encoding: gc.UTF8_ENCODING }
)

Expand Down