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

Simplify inner loop: just fetch $ref #845

Merged
merged 1 commit into from
Dec 9, 2023
Merged

Conversation

thockin
Copy link
Member

@thockin thockin commented Nov 18, 2023

Fixes #844

Old way:

  • ls-remote $ref $ref^{} and parse
  • compare to current
  • if changed, fetch
  • update worktree

New way:

  • fetch $ref
  • compare to current
  • if change, update worktree

Side-effect: fixed a bug where the remote had multiple refs with the same name, for example "main" could resolve to:

244999b795d4a7890f237ef3c8035d68ad56515d	refs/heads/main
be2c0aec052e300028d9c6d919787624290505b6	refs/remotes/upstream/main

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 18, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: thockin

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 18, 2023
Old way:
  - ls-remote $ref $ref^{} and parse
  - compare to current
  - if changed, fetch
  - update worktree

New way:
  - fetch $ref
  - compare to current
  - if change, update worktree
@thockin thockin merged commit 083d8dd into kubernetes:master Dec 9, 2023
4 of 5 checks passed
@nan-yu
Copy link
Contributor

nan-yu commented Feb 14, 2024

Not only does it simplifies the inner loop, but it also coincidentally fixes an issue with fetching the remote SHA.
It may be worth mentioning in the release note.

Here is an example:

In v4.1.0, it gets the remote SHA with git ls-remote $ref $ref^{} and parses the last line. The remote SHA is be2c0aec052e300028d9c6d919787624290505b6.

$ git ls-remote -q https://github.com/janetkuo/anthos-config-management-samples main  main^{}
244999b795d4a7890f237ef3c8035d68ad56515d    refs/heads/main
be2c0aec052e300028d9c6d919787624290505b6    refs/remotes/upstream/main

In v4.2.0, it gets the remote SHA with git fetch $ref, and the remote SHA is 244999b795d4a7890f237ef3c8035d68ad56515d.

$ git fetch https://github.com/janetkuo/anthos-config-management-samples main --verbose --no-progress --prune --no-auto-gc --depth 1
POST git-upload-pack (326 bytes)
POST git-upload-pack (991 bytes)
POST git-upload-pack (gzip 1791 to 952 bytes)
POST git-upload-pack (gzip 3391 to 1759 bytes)
POST git-upload-pack (gzip 6591 to 3357 bytes)
POST git-upload-pack (gzip 12991 to 6566 bytes)
POST git-upload-pack (gzip 25791 to 12989 bytes)
POST git-upload-pack (gzip 51391 to 25936 bytes)
POST git-upload-pack (gzip 6341 to 3235 bytes)
POST git-upload-pack (200 bytes)
From https://github.com/janetkuo/anthos-config-management-samples
 * branch                main       -> FETCH_HEAD
 * 

$ git rev-parse FETCH_HEAD^{}
244999b795d4a7890f237ef3c8035d68ad56515d

@thockin
Copy link
Member Author

thockin commented Feb 14, 2024

That's odd. It looks like 244999b795d4a7890f237ef3c8035d68ad56515d is a merge commit and be2c0aec052e300028d9c6d919787624290505b6 is a regular commit BUT NOT the immediately previous commit!

In fact, the ^{} does nothing:

$ git ls-remote -q https://github.com/janetkuo/anthos-config-management-samples main
244999b795d4a7890f237ef3c8035d68ad56515d	refs/heads/main
be2c0aec052e300028d9c6d919787624290505b6	refs/remotes/upstream/main

$ git ls-remote -q https://github.com/janetkuo/anthos-config-management-samples main^{}
# no output

It looks like git is being oh so helpful and returning all the refs named "main", not just the branch. It seems I should have been passing -h -t to avoid this. I guess it is not needed now. :)

Thanks for the info!

nan-yu added a commit to nan-yu/kpt-config-sync that referenced this pull request Feb 23, 2024
There is a bug in git-sync v4.1.0. When branches in different remotes
are out of sync, `git-sync` fetches the commit SHA from the last line,
which may not be the latest. This leads to an issue that Config Sync
couldn't pull the latest commit from HEAD.

The issue was fixed in v4.2.0 by
kubernetes/git-sync#845.
This commit updates git-sync to v4.2.1 to include the fix.

It also bumps the debian-base to latest version for CVE fixes.

b/325341042
nan-yu added a commit to nan-yu/kpt-config-sync that referenced this pull request Feb 27, 2024
There is a bug in git-sync v4.1.0. When branches in different remotes
are out of sync, `git-sync` fetches the commit SHA from the last line,
which may not be the latest. This leads to an issue that Config Sync
couldn't pull the latest commit from HEAD.

The issue was fixed in v4.2.0 by
kubernetes/git-sync#845.
This commit updates git-sync to v4.2.1 to include the fix.

It also bumps the debian-base to latest version for CVE fixes.

b/325341042
nan-yu added a commit to nan-yu/kpt-config-sync that referenced this pull request Feb 27, 2024
There is a bug in git-sync v4.1.0. When branches in different remotes
are out of sync, `git-sync` fetches the commit SHA from the last line,
which may not be the latest. This leads to an issue that Config Sync
couldn't pull the latest commit from HEAD.

The issue was fixed in v4.2.0 by
kubernetes/git-sync#845.
This commit updates git-sync to v4.2.1 to include the fix.

It also bumps the debian-base to latest version for CVE fixes.

b/325341042
nan-yu added a commit to nan-yu/kpt-config-sync that referenced this pull request Feb 27, 2024
There is a bug in git-sync v4.1.0. When branches in different remotes
are out of sync, `git-sync` fetches the commit SHA from the last line,
which may not be the latest. This leads to an issue that Config Sync
couldn't pull the latest commit from HEAD.

The issue was fixed in v4.2.0 by
kubernetes/git-sync#845.
This commit updates git-sync to v4.2.1 to include the fix.

It also bumps the debian-base to latest version for CVE fixes.

b/325341042
google-oss-prow bot pushed a commit to GoogleContainerTools/kpt-config-sync that referenced this pull request Feb 27, 2024
There is a bug in git-sync v4.1.0. When branches in different remotes
are out of sync, `git-sync` fetches the commit SHA from the last line,
which may not be the latest. This leads to an issue that Config Sync
couldn't pull the latest commit from HEAD.

The issue was fixed in v4.2.0 by
kubernetes/git-sync#845.
This commit updates git-sync to v4.2.1 to include the fix.

It also bumps the debian-base to latest version for CVE fixes.

b/325341042
nan-yu added a commit to nan-yu/kpt-config-sync that referenced this pull request Feb 27, 2024
…rTools#1147)

There is a bug in git-sync v4.1.0. When branches in different remotes
are out of sync, `git-sync` fetches the commit SHA from the last line,
which may not be the latest. This leads to an issue that Config Sync
couldn't pull the latest commit from HEAD.

The issue was fixed in v4.2.0 by
kubernetes/git-sync#845.
This commit updates git-sync to v4.2.1 to include the fix.

It also bumps the debian-base to latest version for CVE fixes.

b/325341042
google-oss-prow bot pushed a commit to GoogleContainerTools/kpt-config-sync that referenced this pull request Feb 27, 2024
There is a bug in git-sync v4.1.0. When branches in different remotes
are out of sync, `git-sync` fetches the commit SHA from the last line,
which may not be the latest. This leads to an issue that Config Sync
couldn't pull the latest commit from HEAD.

The issue was fixed in v4.2.0 by
kubernetes/git-sync#845.
This commit updates git-sync to v4.2.1 to include the fix.

It also bumps the debian-base to latest version for CVE fixes.

b/325341042
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Simplify inner loop
4 participants