`dvc check-ignore` command #4282

karajan1001 · 2020-07-25T03:34:48Z

update some tests first
fix #3736

❗ I have followed the Contributing to DVC checklist.
📖 If this PR requires documentation updates, I have created a separate PR (or issue, at least) in dvc.org and linked it here.
Docs for new command dvc check-ignore dvc.org#1629

Thank you for the contribution - we'll try to review it as soon as possible. 🙏

fix iterative#3736

dvc/ignore.py

abspath Co-authored-by: Ruslan Kuprieiev <[email protected]>

pared

Looks great!
Some notes, one change request (_get_normalize_path)

pared · 2020-07-27T12:34:15Z

tests/func/test_check_ignore.py

+    "file,ret,output",
+    [
+        ("file", 0, "{}:1:f*\tfile\n".format(DvcIgnore.DVCIGNORE_FILE)),
+        ("foo", 0, "{}:2:!foo\tfoo\n".format(DvcIgnore.DVCIGNORE_FILE)),


shouldn't we return 1 in this use case?

Hmm, just tested it with git and it seems that even if you exclude file, 0 is returned.
According to doc 0 should be returned when given path is ignored. So I would say that this is a bug in git. @karajan1001 @efiop what do you think?

@pared
This is what I would like to discuss.

Yes In Git, 0 should be returned when a given path is ignored. But 0 returns on any pattern match the file either included or excluded, 1 returns when none of the pattern match. (It might be a bug or a feature not clearly documented. I sent an email to Git last time but no reply)
But I felt illogical on this results. 0 for ignored and 1 for included or not matched might be more reasonable.

As a result. I left matches in

CheckIgnoreResult = namedtuple( "CheckIgnoreResult", ["file", "matches", "patterns"] )

which could be used for marking whether it is ignored or not.

@karajan1001
To me, it seems that either docs is wrong (because it says that 0 is returned if file is ignored) or implementation (which returns 0 upon any matching spec). I'll try to reach git-devel IRC, maybe we can get an answer faster.
For now, we can leave it as is.

tests/func/test_check_ignore.py

dvc/ignore.py

pared · 2020-07-27T14:29:20Z

dvc/ignore.py

+            # skip system pattern
+            if not pattern.file_info:
+                continue


Won't that result in no matches for .git and return 1 when we do check-ignore .git?
I guess in one way this is right behavior, since its not specified in any file, but, its also against tree.walk behavior which will ignore .git.
But I guess main point of check-ignore is to debug .dvcignores, so its fine.

I just followed Git.

Maybe I can return

$ dvc check-ignore .git ::System pattern .git $ echo $? 0

If that's not too much trouble, sure. But if its, feel free to keep it that way.

I think this is the same problem with the above one.
If 0 or 1 depends on whether the file is excluded, then we should return 0 here, but If it depends on whether any pattern in .dvcignore matches the file, then we should return 1 here.

@karajan1001 you mean the proper git return code?
To avoid unnecessary change requests, let's leave it as is.

karajan1001 · 2020-07-28T11:58:56Z

@pared
Something to be noticed, replacing

if (conditions): 
    return False

with

try: 
    ... 
except exceptions:
    return False

runs faster when conditions happens rarely, but it runs much slower when conditions is a common one. The exception handling cost lots of resources.
I don't know whether these functions are called with some out of workspace path frequently.

pared · 2020-07-28T12:41:10Z

@karajan1001
I think the most common out of workspace use case is with external dependencies. From the development point of view, I think we should stick to conditions because they are much more readable and don't require us to leave a long comment why are we doing things in a particular way.

karajan1001 · 2020-07-28T14:38:19Z

@karajan1001
I think the most common out of workspace use case is with external dependencies. From the development point of view, I think we should stick to conditions because they are much more readable and don't require us to leave a long comment why are we doing things in a particular way.

Thank you, if-condition is more readable and try-catch separates exception handling and function logic (All Exceptions been handled at one place and one level). The best practice might be sharing the same standard all over the project for all contributors. Because it makes us more ease to understand each other's code.
@pared @efiop I'm not sure if this project has a universal rule or code style? Or at what condition should we raise an exception and what situation to use if-condition

dvc/command/check_ignore.py

Co-authored-by: Jorge Orpinel <[email protected]>

jorgeorpinel · 2020-07-29T06:09:13Z

p.s. being able to review docs and core at the same time has some great benefits, especially when the draft doc is so thorough and with comprehensive examples so kudos and thanks again @karajan1001 !

efiop · 2020-07-29T20:35:04Z

@karajan1001 Sorry for the delay :)

I'm not sure if this project has a universal rule or code style? Or at what condition should we raise an exception and what situation to use if-condition

There is no strict policy. As you've noticed we mix and match these at different places in the code. For example, stuff like if self.tree.exists is usually used because we didn't finalize tree unification yet, where we need to unify the exceptions that all tree types raise, so that we could handle them in a general way. And sometimes in places where performance doesn't matter much, we just use ifs because they seem more readable in that particular place. So go with your gut in this case, we'll help out if we see that something could be improved. 🙂

As always, thank you for contributing amazing stuff! 🙏

dvc/command/check_ignore.py

dvc/main.py

@karajan1001

Part of the issue is a bug from iterative#3806 Kudos to @karajan1001 for finding this bug while working on iterative#4282

@karajan1001

Part of the issue is a bug from #3806 Kudos to @karajan1001 for finding this bug while working on #4282

This reverts commit 27eec49.

1. Argument `targets`'s description. 2. Error handling of `_get_normalize_path`

karajan1001 · 2020-07-30T11:07:32Z

@pared @efiop
According to discussions from in Discord

Reverted to the previous version, but several problems still remained. Maybe it's better to create independent issues for each?

Return 0 or 1 for files matching a ! started pattern.
Result insistency of __call__ ,is_ignored_dir, and is_ignored_file with paths outside the repo.
Sub-repo detection refactoring.

CI had failed for

No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.

efiop · 2020-07-31T00:11:08Z

dvc/main.py

@@ -84,7 +84,7 @@ def main(argv=None):  # noqa: C901
        ret = 255

    try:
-        if ret != 0:
+        if ret != 0 and ret != 1:


I think this is no longer needed after the rebase

Suggested change

if ret != 0 and ret != 1:

if ret != 0:

@efiop
Still got this warning after I deleted ret != 1

Return numbers are used for those who want to do some ignore-check in a script. We might solve it in two ways:

Return number only available in a --quiet mode, without -q they are always 0

Alert not show for some non-zero numbers.

@karajan1001, or just do something like this for now:

if ret == 1 and args and args.cmd == "check-ignore": pass

Not ideal, but should work for now.

Co-authored-by: Ruslan Kuprieiev <[email protected]>

1. Add a new test for the out side repo cases 2. check ignore now only check one file not file lists

efiop

Cool stuff! Thank you @karajan1001 ! 🙏

jorgeorpinel · 2020-08-03T19:03:19Z

dvc/command/check_ignore.py

+        "targets",
+        nargs="+",
+        help="Exact or wildcard paths of files or directories to check "
+        "ignore patterns.",


That sentence is a bit confusing. How about "File or directory paths, or ignore patterns to check"

@jorgeorpinel
I think the targets we checked here are wildcard patterns not ignore patterns. It's a pattern used in file searches so the full name need not be typed provided by the file system. For example, we have

aa ab bb

in our current path.
The command dvc check-ignore a* equals to dvc check-ignore aa ab. Our program received a list of files instead of a single pattern a*.

Only my personal opinion, not good at English, maybe ignore patterns can represent the same meaning?

the targets we checked here are wildcard patterns not ignore patterns

Good to know, thanks! Confusing probably... but makes sense. So those depend on the OS? I.e. POSIX shells have accept different wildcards than, say, Windows PowerShell.

And yeah no worries, it's getting too deep. I'll take care of rewriting it as needed in iterative/dvc.org/pull/1629 and submit a matching PR to the core repo.

Went for "File or directory paths to check (wildcards supported)" in iterative/dvc.org/pull/1629.

Just "File or directory paths to check" now... See #1673

jorgeorpinel · 2020-08-03T19:06:08Z

dvc/command/check_ignore.py

+
+
+def add_parser(subparsers, parent_parser):
+    ADD_HELP = "Debug DVC ignore/exclude files"


How about something like "Check whether files or directories are excluded due to .dvcignore." ?

Per https://github.com/iterative/dvc.org/pull/1629/files

@johntharian
No problem.

Wrong handle 😆

karajan1001 · 2020-08-04T10:55:21Z

@jorgeorpinel
I'm sorry Seems that it had been merged, and I can't change it anymore.
Maybe we could open another Issue (GoodFirstOne) for these problems?

jorgeorpinel · 2020-08-05T23:40:11Z

No problem, I'll submit a PR when I get a chance.

per iterative/dvc#4282 (review)

per #4282 (review) and #4282 (review)

* check_ignore: update help output per #4282 (review) and #4282 (review) * check-ignore: more output string updates per #4323 et al. * check-ignore: min text update to match iterative/dvc.org/pull/1673 Co-authored-by: Ruslan Kuprieiev <[email protected]>

Update some tests first

3a377ce

fix iterative#3736

karajan1001 changed the title ~~Update some tests first~~ dvc check-ignore command Jul 25, 2020

karajan1001 added 3 commits July 26, 2020 09:53

first edition

32154f7

solve failure in Windows

86a4ff7

For Windows ci

1f13e84

karajan1001 commented Jul 26, 2020

View reviewed changes

dvc/ignore.py Outdated Show resolved Hide resolved

weekly-digest bot mentioned this pull request Jul 26, 2020

Weekly Digest (19 July, 2020 - 26 July, 2020) #4285

Closed

karajan1001 mentioned this pull request Jul 26, 2020

cmd: document check-ignore iterative/dvc.org#1628

Closed

karajan1001 and others added 2 commits July 26, 2020 17:42

Some help ducuments issue.

c3e2fbd

Update dvc/ignore.py

3eea263

abspath Co-authored-by: Ruslan Kuprieiev <[email protected]>

efiop requested a review from pared July 27, 2020 00:27

karajan1001 mentioned this pull request Jul 27, 2020

ignore: dynamically collect dvcignore #4284

Merged

2 tasks

pared suggested changes Jul 27, 2020

View reviewed changes

karajan1001 added 2 commits July 28, 2020 19:16

Merge remote-tracking branch 'origin/master' into fix_3736

76481bd

Refactor with OutOfWorkSpaceError

27eec49

Solve a bug

10d23e6

jorgeorpinel reviewed Jul 29, 2020

View reviewed changes

dvc/command/check_ignore.py Outdated Show resolved Hide resolved

jorgeorpinel reviewed Jul 29, 2020

View reviewed changes

dvc/command/check_ignore.py Outdated Show resolved Hide resolved

jorgeorpinel reviewed Jul 29, 2020

View reviewed changes

dvc/command/check_ignore.py Outdated Show resolved Hide resolved

karajan1001 and others added 2 commits July 29, 2020 13:48

Update dvc/command/check_ignore.py

0ac9c85

Co-authored-by: Jorge Orpinel <[email protected]>

Update dvc/command/check_ignore.py

ab7237d

Co-authored-by: Jorge Orpinel <[email protected]>

efiop reviewed Jul 29, 2020

View reviewed changes

dvc/command/check_ignore.py Outdated Show resolved Hide resolved

Update dvc/command/check_ignore.py

3cf35f0

efiop reviewed Jul 29, 2020

View reviewed changes

dvc/main.py Outdated Show resolved Hide resolved

efiop mentioned this pull request Jul 30, 2020

dvc: fix --quiet mode #4304

Merged

2 tasks

efiop added a commit to efiop/dvc that referenced this pull request Jul 30, 2020

dvc: fix --quiet mode

a02c191

Part of the issue is a bug from iterative#3806 Kudos to @karajan1001 for finding this bug while working on iterative#4282

efiop added a commit that referenced this pull request Jul 30, 2020

dvc: fix --quiet mode (#4304)

815444f

Part of the issue is a bug from #3806 Kudos to @karajan1001 for finding this bug while working on #4282

karajan1001 added 3 commits July 30, 2020 14:51

Revert "Refactor with OutOfWorkSpaceError"

4844735

This reverts commit 27eec49.

Merge remote-tracking branch 'origin/master' into fix_3736

2749112

Two change request

4ee24f0

1. Argument `targets`'s description. 2. Error handling of `_get_normalize_path`

efiop reviewed Jul 31, 2020

View reviewed changes

karajan1001 and others added 2 commits July 31, 2020 08:42

Update dvc/main.py

54fbe31

Co-authored-by: Ruslan Kuprieiev <[email protected]>

check_ignore now only accept one path a time

930534b

1. Add a new test for the out side repo cases 2. check ignore now only check one file not file lists

weekly-digest bot mentioned this pull request Aug 2, 2020

Weekly Digest (26 July, 2020 - 2 August, 2020) #4315

Closed

efiop approved these changes Aug 3, 2020

View reviewed changes

efiop merged commit 11d906e into iterative:master Aug 3, 2020

jorgeorpinel reviewed Aug 3, 2020

View reviewed changes

jorgeorpinel mentioned this pull request Aug 3, 2020

Docs for new command dvc check-ignore iterative/dvc.org#1629

Merged

jorgeorpinel reviewed Aug 3, 2020

View reviewed changes

jorgeorpinel mentioned this pull request Aug 6, 2020

Two new modes --all and --stdin for check-ignore #4323

Merged

2 tasks

jorgeorpinel added a commit to karajan1001/dvc.org that referenced this pull request Aug 7, 2020

cmd: update check-ignore targets arg desc

c759585

per iterative/dvc#4282 (review)

karajan1001 mentioned this pull request Aug 7, 2020

check-ignore: confusing help message when no matches are found [qa] #4351

Closed

jorgeorpinel added a commit that referenced this pull request Aug 7, 2020

check_ignore: update help output

612dd94

per #4282 (review) and #4282 (review)

jorgeorpinel mentioned this pull request Aug 7, 2020

check-ignore: update help output [qa] #4358

Merged

2 tasks

weekly-digest bot mentioned this pull request Aug 9, 2020

Weekly Digest (2 August, 2020 - 9 August, 2020) #4365

Closed

karajan1001 deleted the fix_3736 branch April 7, 2021 07:41



		def add_parser(subparsers, parent_parser):
		ADD_HELP = "Debug DVC ignore/exclude files"

dvc check-ignore command #4282

dvc check-ignore command #4282

Uh oh!

Conversation

karajan1001 commented Jul 25, 2020 • edited by jorgeorpinel Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

pared left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karajan1001 Jul 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karajan1001 commented Jul 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pared commented Jul 28, 2020

Uh oh!

karajan1001 commented Jul 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jorgeorpinel commented Jul 29, 2020

Uh oh!

efiop commented Jul 29, 2020

Uh oh!

Uh oh!

Uh oh!

karajan1001 commented Jul 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karajan1001 Jul 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

efiop left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karajan1001 Aug 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jorgeorpinel Aug 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

`dvc check-ignore` command #4282

`dvc check-ignore` command #4282

karajan1001 commented Jul 25, 2020 •

edited by jorgeorpinel

Loading

karajan1001 Jul 28, 2020 •

edited

Loading

karajan1001 commented Jul 28, 2020 •

edited

Loading

karajan1001 commented Jul 28, 2020 •

edited

Loading

karajan1001 commented Jul 30, 2020 •

edited

Loading

karajan1001 Jul 31, 2020 •

edited

Loading

karajan1001 Aug 6, 2020 •

edited

Loading

jorgeorpinel Aug 3, 2020 •

edited

Loading

karajan1001 Aug 6, 2020 •

edited

Loading