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

Allows the default git/gitoxide configuration to be obtained from the ENV and config #13687

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

linyihai
Copy link
Contributor

@linyihai linyihai commented Apr 2, 2024

What does this PR try to resolve?

The default git/gitoxide feautures config can be obtained througt -Zgit and -Zgitoxide.
However, it cannot be obtained from environment variables or configurations.
It's not very ergonomic.

How should we test and review this PR?

The previous commit explained the staus quo, the next commit addressed the problem.

Additional information

Inspired by #11813 (comment)
See also #13285 (comment)

@rustbot
Copy link
Collaborator

rustbot commented Apr 2, 2024

r? @ehuss

rustbot has assigned @ehuss.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-unstable Area: nightly unstable support S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 2, 2024
@linyihai linyihai changed the title Allows the default git/gitoxide configuration to be obtained from the command line or from an environment variable Allows the default git/gitoxide configuration to be obtained from the ENV and config Apr 2, 2024
@epage
Copy link
Contributor

epage commented Apr 2, 2024

I assume this is meant to resolve the immediate part of #13688 (leaving aside the more general problem). Is that right?

@@ -908,15 +914,19 @@ fn parse_git(it: impl Iterator<Item = impl AsRef<str>>) -> CargoResult<Option<Gi
#[derive(Debug, Copy, Clone, Default, Deserialize)]
pub struct GitoxideFeatures {
/// All fetches are done with `gitoxide`, which includes git dependencies as well as the crates index.
#[serde(default = "default_true")]
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume these were chosen to align with GitoxideFeatures::safe() and to mirror if you do -Zgitoxide

However, this breaks down if you do CARGO_UNSTABLE_GITOXIDE_FETCH=true, then almost everything else becomes enabled (we should have a test to show this case).

Some other challenges with these defaults

  • Unclear what the motivation is (could have comment pointing to GitoxideFeature::safe)
  • Challenge in keeping in sync with GitoxideFeature::safe

Should we instead just default everything to false?

Copy link
Member

@Nemo157 Nemo157 Apr 2, 2024

Choose a reason for hiding this comment

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

What I would love to just work is CARGO_UNSTABLE_GITOXIDE=fetch,list_files and CARGO_UNSTABLE_GITOXIDE=true mirroring the CLI flag (which would also imply you could have unstable.gitoxide = "fetch,list_files" in the config file).

Copy link
Member

Choose a reason for hiding this comment

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

Yeah maybe we can make it a comma separated list to reduce the complexity.

Wonder do people really use individual gitoxide features? I always assume that people would just enable them as a whole, though gitoxide author recommends keeping them separately: #13252 (comment).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What I would love to just work is CARGO_UNSTABLE_GITOXIDE=fetch,list_files and CARGO_UNSTABLE_GITOXIDE=true mirroring the CLI flag (which would also imply you could have unstable.gitoxide = "fetch,list_files" in the config file).

So what we defined as table is going to be replaced with a string? It makes sense, I have no objection to this, but it is not clear that it will break the configuration of the previous version?

Copy link
Member

Choose a reason for hiding this comment

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

It's possible to support both, via #[serde(deserialize_with = ...)] and a custom deserializer that handles a string/list or a struct, like

fn progress_or_string<'de, D>(deserializer: D) -> Result<Option<ProgressConfig>, D::Error>

@bors
Copy link
Collaborator

bors commented Apr 4, 2024

☔ The latest upstream changes (presumably #13696) made this pull request unmergeable. Please resolve the merge conflicts.

@ehuss
Copy link
Contributor

ehuss commented Apr 6, 2024

r? weihanglo

@rustbot rustbot assigned weihanglo and unassigned ehuss Apr 6, 2024
- pass 'all' to enable all predefind git/gitoxide features
- stil support parse git/gitoxide as table in Config, but not support parser table in ENV
@linyihai
Copy link
Contributor Author

linyihai commented Apr 8, 2024

In my new commit, a string can be parsed as git/gitoxide features in ENV and Config.
You can pass 'all' to enable predefined features.

CARGO_UNSABLE_GIT='all' cargo fetch
CARGO_UNSABLE_GIT='shallow-deps' cargo fetch
cargo fetch --config "unstable.git='shallow-deps'"
cargo fetch --config "unstable.git='all'"

In Config, the git/gitoxide can be defined as

# as string
[unstable]
git = 'all'

# still can be defined as table, but all fields needed
[unstable.git]
shallow_deps = true
shallow_index = true

But the defect is that the fields as ENV will be ignored.

CARGO_UNSABLE_GIT_SHALLOW_DEPS CARGO_UNSABLE_GIT_SHALLOW_INDEX cargo fetch

In ENV, it seems that a struct cann't be parser from string and table in the sametime.

@linyihai
Copy link
Contributor Author

linyihai commented Apr 8, 2024

I assume this is meant to resolve the immediate part of #13688 (leaving aside the more general problem). Is that right?

Yes,this tend to mirror the -Zgit/-Zgitoxide as possible.

Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

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

Sorry for the late review 🙇🏾‍♂️

// then run `CARGO_UNSTABLE_GIT_SHALLOW_INDEX cargo fetch`, here will return `missing config key unstable.git`.
// It seems that anything that starts with CARGO_UNSTABLE_GIT, even like CARGO_UNSTABLE_GITOXIDE can trigger this.
// This is a workaround for now, but should be fixed in the future.
visitor.visit_none()
Copy link
Member

@weihanglo weihanglo May 10, 2024

Choose a reason for hiding this comment

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

While this seems fine for now, I am a bit worried about the change.

What about having an enum deriving serde?

#[derive(Debug, Copy, Clone, Default, Deserialize)]
#[serde(untagged)]
pub enum Gitoxide {
    #[default]
    All,
    Some(GitoxideFeatures),
}

so that I assumed it would work well with

[unstable]
gitoxide = true

# or
[unstable.gitoxide]
checkout = true

As well as enviroment variables?

CARGO_UNSTABLE_GITOXIDE=true
CARGO_UNSTABLE_GITOXIDE_CHECKOUT=true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't seem to be working. see gitoxide_features_as_table in the lastest commit.

@rustbot rustbot added the A-git Area: anything dealing with git label May 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-git Area: anything dealing with git A-unstable Area: nightly unstable support S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants