feat: permissive git diff parameter #185

nvuillam · 2021-08-19T16:09:25Z

Manage a new argument --permissivediff to add the following parameters to git diff call

[
    '--ignore-all-space',
    '--ignore-blank-lines',
    '--ignore-cr-at-eol',
    '--word-diff-regex',
]

It allows to avoid detecting as a difference the indentation/spaces stuff

Fixes #184

codecov · 2021-08-20T14:11:43Z

Codecov Report

Merging #185 (db0dc0c) into master (6b2adb4) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #185   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           19        19           
  Lines          459       462    +3     
=========================================
+ Hits           459       462    +3

Impacted Files	Coverage Δ
src/utils/fileGitDiff.js	`100.00% <100.00%> (ø)`
src/utils/gitConstants.js	`100.00% <100.00%> (ø)`
src/utils/repoGitDiff.js	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6b2adb4...db0dc0c. Read the comment docs.

scolladon · 2021-08-20T14:15:32Z

Hi @nvuillam !

I copy paste my comment from the related issue

I have questions:

Why this should not be the standard behaviour? => What will be the added value to that and why both use cases should exists?
why using --word-diff-regex parameter? With which value?

Suggestion: the parameter name must follow the cli parameter apache convention. I suggest the following --permissive-diff -P if we keep this wording

Please add me to your fork so I can contribute and help you solve CI/CD issues you may encounter

Manage a new argument --permissivediff to add the following parameters to git diff call [ '--ignore-all-space', '--ignore-blank-lines', '--ignore-cr-at-eol', '--word-diff-regex', ] It allows to avoid detecting as a difference the indentation/spaces stuff

follow apache command line

nvuillam · 2021-09-02T21:47:40Z

@scolladon I see you made the code more clean, nice ! :)

When do you think this could be released ? It would greatly help sfdx-hardis ^^

scolladon · 2021-09-03T06:44:45Z

Hi @nvuillam !

When do you think this could be released ? It would greatly help sfdx-hardis ^^

We are going to discuss about it today with @mehdisfdc and see how we can improve (if we can) and when to ship it
We'll keep you posted!

nvuillam · 2021-09-03T06:49:11Z

That's great, thanks for the feedback :)

mehdicherf · 2021-09-03T08:33:26Z

README.md

  trace|debug|info|warn|error|fatal|TRACE|DEBUG|INFO|WARN|ERROR|FATAL]

 OPTIONS
  -D, --ignore-destructive=ignore-destructive                                       ignore file to use
+
+  -P, --permissive-diff                                                             ignore git diff white char (space,


@scolladon As discussed together, would you prefer to name the new command -W, --ignore-whitespace instead of -P, --permissive-diff? Both are fine with me.
@nvuillam Any opinion on this alternative name?

@mehdisfdc --ignore-whitespace-diff maybe ? :)
Anyway, while the capability is here, I don't matter the argument name :)

I like -W, --ignore-whitespace 👍

mehdicherf · 2021-09-03T08:47:28Z

src/utils/gitConstants.js

+  '--ignore-all-space',
+  '--ignore-blank-lines',
+  '--ignore-cr-at-eol',
+  '--word-diff-regex',


@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).

@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

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

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

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

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 ?

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 :/

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 ? :)

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

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

codeclimate · 2021-09-07T15:19:50Z

Code Climate has analyzed commit db0dc0c and detected 0 issues on this pull request.

View more on Code Climate.

scolladon changed the title ~~Allow to use permissive git diff to ignore diffs related to indentation~~ feat: permissive git diff parameter Aug 20, 2021

scolladon force-pushed the feature/permissivediff branch from 520f5e7 to 7c0fa4f Compare August 30, 2021 06:51

scolladon-sfdc added 6 commits August 30, 2021 09:05

fix: parameter naming convention

e7497ef

follow apache command line

feat: add permissive diff to inFile git comparison

01c8b6e

feat: add permissive flag to legacy command line

2e14e59

style: parameter description

3b96fef

docs: add new parameter to the README

faa1d0e

test: permissive flag behavior

8653e1f

mehdicherf reviewed Sep 3, 2021

View reviewed changes

scolladon-sfdc added 2 commits September 3, 2021 11:59

feat: rename parameter to --ignore-whitespace

2f9d750

fix: command line argument usage

db0dc0c

mehdicherf approved these changes Sep 10, 2021

View reviewed changes

scolladon approved these changes Sep 10, 2021

View reviewed changes

scolladon merged commit 6d57783 into scolladon:master Sep 10, 2021

scolladon deleted the feature/permissivediff branch September 10, 2021 07:47

scolladon mentioned this pull request Mar 14, 2022

--ignore-whitespace ignores everything instead #262

Closed

1 task

scolladon mentioned this pull request Dec 12, 2023

chore(main): release 5.31.1 #739

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: permissive git diff parameter #185

feat: permissive git diff parameter #185

nvuillam commented Aug 19, 2021 •

edited

Loading

codecov bot commented Aug 20, 2021 •

edited

Loading

scolladon commented Aug 20, 2021

nvuillam commented Sep 2, 2021

scolladon commented Sep 3, 2021 •

edited

Loading

nvuillam commented Sep 3, 2021

mehdicherf Sep 3, 2021

nvuillam Sep 3, 2021

scolladon Sep 3, 2021

mehdicherf Sep 3, 2021

scolladon Sep 3, 2021 •

edited

Loading

nvuillam Sep 3, 2021

scolladon Sep 6, 2021

nvuillam Sep 7, 2021

scolladon Sep 7, 2021

nvuillam Sep 7, 2021

nvuillam Sep 7, 2021

scolladon Sep 7, 2021

scolladon Sep 7, 2021 •

edited

Loading

codeclimate bot commented Sep 7, 2021

feat: permissive git diff parameter #185

feat: permissive git diff parameter #185

Conversation

nvuillam commented Aug 19, 2021 • edited Loading

codecov bot commented Aug 20, 2021 • edited Loading

Codecov Report

scolladon commented Aug 20, 2021

nvuillam commented Sep 2, 2021

scolladon commented Sep 3, 2021 • edited Loading

nvuillam commented Sep 3, 2021

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

scolladon Sep 3, 2021 • edited Loading

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

scolladon Sep 7, 2021 • edited Loading

Choose a reason for hiding this comment

codeclimate bot commented Sep 7, 2021

nvuillam commented Aug 19, 2021 •

edited

Loading

codecov bot commented Aug 20, 2021 •

edited

Loading

scolladon commented Sep 3, 2021 •

edited

Loading

scolladon Sep 3, 2021 •

edited

Loading

scolladon Sep 7, 2021 •

edited

Loading