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

[linyihai]

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)

Change of usage

1. Mirror Zgit/Zgitoxide when they parsed as string

Specify the feature you like

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

Besides, you can pass "all" to use the pre-defined features.

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

2. Specify partial fields when parsed as table

CARGO_UNSTABLE_GITOXIDE_FETCH=true cargo fetch

The rest fields will use Rust default value. That said, checkout and internal_use_git2 will be false.

Besides, you can pass true to the whole feature to specify the pre-defined features.

CARGO_UNSTABLE_GITOXIDE=true cargo fetch

[rustbot]

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

[epage]

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

[epage]

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?

[Nemo157]

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

[weihanglo]

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

[linyihai]

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？

[Nemo157]

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

cargo/src/cargo/util/context/mod.rs

Line 2673 in 88d55a8

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

[bors]

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

[ehuss]

r? weihanglo

[linyihai]

In my new commit, a string can be parsed as git/gitoxide features in ENV and Config.
You can pass 'all' or true 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'"

CARGO_UNSABLE_GIT=true cargo fetch
CARGO_UNSABLE_GITOXIDE=true cargo fetch

In Config, the git/gitoxide can be defined as

# as string
[unstable]
git = 'all'

# still can be defined as table
[unstable.git]
shallow_deps = true
shallow_index = true

the unspecified field can be skipped, because it defined as Rust default value.

Back to the original use of environment variables, now you just need to specify the field you want to modify, the rest field will use Rust default value.

CARGO_UNSTABLE_GITOXIDE_FETCH=true cargo fetch

But you should keep in mind, the rest field will use Rust default value, not pre-defined value. That said, checkout and internal_use_git2 will be false. (I haven't decided whether to use the rust default values or the predefined values)

See gitoxide_features_as_xx and git_features_as_xx in tests/testsuite/config.rs for more details.

[linyihai]

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.

[weihanglo]

Sorry for the late review 🙇🏾‍♂️

[weihanglo]

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

[linyihai]

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

[linyihai]

I had replaced vistor.visit_none() with below,

        // The effect here is the same as in [`deserialize_option`].
        if self.gctx.has_key(&self.key, self.env_prefix_ok)? {
            return visitor.visit_some(self);
        }

So we can still specify a inner field of a struct in the ENV, like CARGO_UNSTABLE_GITOXIDE_FETCH, even that CARGO_UNSTABLE_GIOXIDE doesn't exist.

But that's not enough, It will still warning unstable.git missing key, Why is this happening? After some groping, Problems gradually surfaced, see https://github.com/rust-lang/cargo/blob/master/src/cargo/util/context/de.rs#L268-L269

                if env_key.starts_with(field_key.as_env_key()) {
                    fields.insert(KeyKind::Normal(field.to_string()));

here will treat CARGO_UNSTABLE_GIT as prefix of CARGO_UNSTABLE_GIOXIDE_FETCH, and the unstable.git and unsable.gitoxide will be pushed into fields. This is a problem, when the unstable fields is a struct and the fields implement their deserialize_any method. So any environment variable that starts with the XX character will have missing XX keys.
See test struct_with_overlapping_inner_struct_and_defaults for more details.

[linyihai]

See #13687 (comment)

I wonder if this is worth fixing on separately PR. Because it's only a problem in my situation

[weihanglo]

Probably. I'll find a time reviewing this.
(it's always a headache for me to read config deserialization code 🤯)

[bors]

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