Fix: cargo package failed on bare commit git repo.

[linyihai]

What does this PR try to resolve?

Fixes #14354

This approach chose to not generate a .cargo_vcs_info.json for bare commit git repo.

How should we test and review this PR?

Compare the test changes before and after the two commits

Additional information

[rustbot]

r? @weihanglo

rustbot has assigned @weihanglo.
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

[linyihai]

It hits the API limit

#2 [internal] load metadata for docker.io/library/alpine:3.20
#2 ERROR: failed to copy: httpReadSeeker: failed open: unexpected status code https://registry-1.docker.io/v2/library/alpine/manifests/sha256:eddacbc7e24bf8799a4ed3cdcfa50d4b88a323695ad80f317b6629883b2c2a78: 429 Too Many Requests - Server message: toomanyrequests: You have reached your pull rate limit. You may increase the limit by authenticating and upgrading: https://www.docker.com/increase-rate-limit

[weihanglo]

Thank you!

[weihanglo]

@rfcbot fcp merge

This insta-stabilizes an edge case of .cargo_vcs_info.json:

When a Git repo has no commits, the file content would be

{
  "git": {
    # there is no "sha1" field because `HEAD` is missing
    "dirty": true
  },
  "path_in_vcs": "..."
}

Thinking backward from #13695, the requirement is to have commit hash as well as the path_in_vcs field for other tools if they want to post-process further. The current approach this PR takes, generating vcs info file even with no sha field, is still useful for preserving path_in_vcs.

[rfcbot]

Team member @weihanglo has proposed to merge this. The next step is review by the rest of the tagged team members:

• @Eh2406
• @Muscraft
• @arlosi
• @ehuss
• @epage
• @joshtriplett
• @weihanglo

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[epage]

@weihanglo there are several solutions for this

• Don't include the file
• Do whatever non-git locations do
• not have the git table

and likely more.

Is there a motivation for the proposed solution? I didn't see it here or in the issue.

[weihanglo]

I am sorry that I just did a really quick FCP without thinking hard on it.

I don't have a good answer. Thinking backward from #13695, the requirement is to have commit hash as well as the path_in_vcs field for other tools if they want to post-process further. The current approach this PR takes, generating vcs info file even with no sha field, is still useful for preserving path_in_vcs.

However, I personally doubt it is useful in real life, since that happens on in an edge case that no commit exists. I am fine with whatever approach we choose. We just need to fix the regression.

[linyihai]

@weihanglo Thanks you rapidly response.

Sorry for that, I had change my mind. I'm not sure if the absence of this field will cause problems in other ways, so it's better to not include/generate the .cargo_vcs_info.json in this case.

Of course, this is only a reasonable change that I currently feel, which should be very rare for this scenario, and it may be better to maintain the consistency with the previous

[weihanglo]

Could fn git() return a None when nothing needed to be recorded? So that we don't need skip_serializing_if for sha1 field.

[linyihai]

Agree with you and updated it

[weihanglo]

Agree that not generating vcs info file is also a reasonable approach to take. It stays with the old behavior (not generating when --allow-dirty presents) without blocking cargo package.

[weihanglo]

Let's discuss different approaches in the issue then.

[weihanglo]

@rustbot fcp cancel

until we settle down a solution.

[weihanglo]

@rfcbot fcp cancel

[rfcbot]

@weihanglo proposal cancelled.

[weihanglo]

Thanks for the contribution. This should be the least controversial way to fix this. See the discussion in #14354.

@bors r+

[bors]

📌 Commit 37cda2d has been approved by weihanglo

It is now in the queue for this repository.

[bors]

⌛ Testing commit 37cda2d with merge b66cad8...

[bors]

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing b66cad8 to master...