source-controller icon indicating copy to clipboard operation
source-controller copied to clipboard

GitRepository Ignore Behavior Does Not Match Documentation

Open nebhale opened this issue 4 years ago • 2 comments

Currently, the documentation for the ignore key in GitRepository says:

// Ignore overrides the set of excluded patterns in the .sourceignore format
// (which is the same as .gitignore). If not provided, a default will be used,
// consult the documentation for your version to find out what those are.
// +optional

but it does not "override the set of excluded patterns", it "appends to the set of excluded patterns".

To illustrate, given the currently listed defaults (.git/, .gitignore, .gitmodules, .gitattributes) and a repository containing both .git/ and .gitignore, setting ignore: .git/ results in a tarball that is missing both .git/ and .gitignore. This wouldn't be override behavior because I'd have expected .gitignore (which is not in the overriden ignore key) to be in the tarball. However using ignore: !.gitignore results in a tarball that is missing .git/ and has .gitignore in it showing the append behavior.

I see two options to address this discrepancy

  1. Update the documentation to describe the append behavior
  2. Update the code to override instead of append

My personal preference is option 2 because it doesn't require a CR reader combine both what they can read as well as a piece of documentation they might not have close by in order to understand the outcome. That being said, I'm pretty ambivalent about which is chosen.

nebhale avatar Aug 19 '21 14:08 nebhale

The goal of the feature is as described in your option one, but the chosen wordings are a bit unfortunate. What the documentation block is trying to say (and this makes a bit more sense once you are familiar with the .gitignore format rules), is that rules added there allow overwriting the defaults or in-source ones, as the appended rules take precedence and/or allow overwriting defaults.

Option 2 is a no-go for me because the default exclusions have been carefully selected, and are sane defaults for the majority of sources. The extra mile you have to take as a user to figure out what rules you need to (over)write can thus be seen as a safety precaution to ensure users do not accidentally end up with large (non-YAML) files in the produced artifact, something that also helps us as maintainers to reduce the stream of support requests around resource usage and network traffic, etc.

hiddeco avatar Sep 02 '21 15:09 hiddeco

I'm trying to add a directory to the ignore list and was a bit concerned that adding a spec.ignore would replace the default ignored list. But as this issue points out, it only appends my Ignore value to the set of ExcludeVCS entries and removes the other Exclude{CI, Ext, Extra} entries - meaning if I still want those excluded I need to add them back.

Maybe what we need are two fields

  • spec.ignore - that matches the documentation and is a complete replacement
  • spec.localIgnore - that appends the list of default ignored entries to the entries specified here

This enables the user to completely override the ignore list and just add new entries to the existing defaults.

If both fields are specified, the spec.ignore completely replaces the defaults and then is appended to spec.localIgnore entries

palemtnrider avatar Oct 26 '21 22:10 palemtnrider