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.

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

Add filter configurations at a per-Source granularity #248

Merged
merged 2 commits into from
Jun 28, 2024

Conversation

ndhaller
Copy link
Contributor

@ndhaller ndhaller commented May 9, 2024

Adds treeless="true" and blobless="true" as attributes to a Combination Source.
During edkrepo clone and checkout, if enableSubmodule="true" on the Combination Source, the filter setting applies to the submodules as well.

--treeless and treeless="true" have priority over --blobless and blobless="true" if multiple are used.

During edkrepo clone, --treeless and --blobless do not apply to the submodule filter settings.

Related issues:

#239
add filter configurations at a per-Source or per-Remote granularity

#247
clone with both --treeless and --blobless encounter git error

Copy link
Contributor

@ashedesimone ashedesimone left a comment

Choose a reason for hiding this comment

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

The changes for adding partial clones to submodules looks good as do the changes to define the partial clone strategy at a per level in the manifest file. Can we separate them into two separate commits?

Your commit message indicates that this is also addressing #247 the two partial clone methods are not really intended to be combined as such I would prefer to address this by making them mutually exclusive arguments, erroring out and letting the user make the correct choice.

Copy link
Contributor

@kevinsun49 kevinsun49 left a comment

Choose a reason for hiding this comment

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

Since we're redefining the default behavior as "use what the manifest says" instead of "full clone", we may want to add a --full flag to the clone command to continue to provide that option.

@ndhaller
Copy link
Contributor Author

The changes for adding partial clones to submodules looks good as do the changes to define the partial clone strategy at a per level in the manifest file. Can we separate them into two separate commits?

Added a separate commit for the submodule change.

Copy link
Contributor

@ashedesimone ashedesimone left a comment

Choose a reason for hiding this comment

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

handling for partial clone filtering should also be added to the _init() method.

edkrepo/common/clone_utilities.py Show resolved Hide resolved
edkrepo/common/clone_utilities.py Outdated Show resolved Hide resolved
@@ -148,6 +156,33 @@ def _get_submodule_enable(manifest, remote_name, combo):
return source.enable_submodule
return False

def _get_blobless_enable(manifest, remote_name, combo):
Copy link
Contributor

Choose a reason for hiding this comment

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

These two methods will only take into account repositories which have manifest level filtering set and will not catch cases of users inputing a filter type on the clone cmd line. This could lead to a mismatch in clone types for the parent repo and the submodule containing them.

In the case where the parent repository is not already present on the system we must also check command line parameters. (Note: in some situations, no selective initialization, the initialization is performed in the _update() method)

In the case where the repository is already present we should defer to the git config file for that repository. Specifically the command git config remote.origin.partialclonefilter will return the partial clone type if any that was used to create the repository.

@ndhaller ndhaller force-pushed the treeless_blobless branch from a5d09be to 935a41d Compare June 18, 2024 03:24
edkrepo/common/clone_utilities.py Outdated Show resolved Hide resolved
ndhaller added 2 commits June 28, 2024 13:06
Adds treeless="true" and blobless="true" as attributes to a Combination Source.
Add arg --full to disable repo attributes.

--treeless, --blobless, and --full args will disable the per-Source attributes.
Allow up to one of --treeless, --blobless and --full args to be used per edkrepo clone.

Related issues:

tianocore#239
add filter configurations at a per-Source or per-Remote granularity

tianocore#247
clone with both --treeless and --blobless encounter git error

Signed-off-by: Nathaniel Haller <[email protected]>
Filter options applied to a repo during the edkrepo clone command will apply to
submodules when edkrepo updates submodules.

Signed-off-by: Nathaniel Haller <[email protected]>
@ndhaller ndhaller force-pushed the treeless_blobless branch from 935a41d to c536d54 Compare June 28, 2024 20:08
@ashedesimone ashedesimone merged commit 314f14f into tianocore:main Jun 28, 2024
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.

3 participants