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

Fetch all history for all tags and branches when fetch-depth=0 #258

Merged
merged 1 commit into from
May 27, 2020

Conversation

ericsciple
Copy link
Contributor

@ericsciple ericsciple commented May 24, 2020

Fetch all history for all tags and branches when fetch-depth: 0

Fixes #240
Fixes #239
Fixes #214
Fixes #204
Fixes #93
Related #250
Related #249
Related #217

branches = await git.branchList(true)
for (const branch of branches) {
await git.branchDelete(true, branch)
// Remove any conflicting refs/remotes/origin/*
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Detects two types of conflicts:

Example 1: Consider ref is refs/heads/foo and an existing ref refs/remotes/origin/foo/bar exists

Example 2: Consider ref is refs/heads/foo/bar and an existing ref refs/remotes/origin/foo exists

@@ -242,6 +241,12 @@ class GitCommandManager {
this.gitEnv[name] = value
}

async shaExists(sha: string): Promise<boolean> {
const args = ['rev-parse', '--verify', '--quiet', `${sha}^{object}`]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

rev-parse magic :)

^{object} means the object can be any type

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i love git :)

it's a great command line tool that lets you do what you need to do

async fetch(refSpec: string[], fetchDepth?: number): Promise<void> {
const args = ['-c', 'protocol.version=2', 'fetch']
if (!refSpec.some(x => x === refHelper.tagsRefSpec)) {
args.push('--no-tags')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not adding --no-tags when +refs/tags/*:refs/tags/* is part of the refspec... i think it would technically work the same, but would just look weird

@@ -207,29 +213,6 @@ jobs:
- uses: actions/checkout@v2
```

## Fetch all tags
Copy link
Contributor Author

Choose a reason for hiding this comment

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

bye bye complicated stuff

- [Fetch all branches](#Fetch-all-branches)
- [Fetch all history for all tags and branches](#Fetch-all-history-for-all-tags-and-branches)

## Fetch all history for all tags and branches
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chrispat fyi

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this comment sums up the motivation nicely:

Seems pretty logical and reasonable to me that if I set fetch-depth: 0 then I clearly want everything and history matters to me and that you should not add the --no-tags parameter.

@@ -60,6 +62,19 @@ export async function getCheckoutInfo(
return result
}

export function getRefSpecForAllHistory(ref: string): string[] {
const result = ['+refs/heads/*:refs/remotes/origin/*', tagsRefSpec]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

branches and tags

const result = ['+refs/heads/*:refs/remotes/origin/*', tagsRefSpec]
if (ref) {
const upperRef = (ref || '').toUpperCase()
if (ref.toUpperCase().startsWith('REFS/PULL/')) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

and conditionally the pull request branch

@ericsciple ericsciple changed the title fetch all history when fetch-depth=0 Fetch all history for all tags and branches when fetch-depth: 0 May 26, 2020
@ericsciple ericsciple changed the title Fetch all history for all tags and branches when fetch-depth: 0 Fetch all history for all tags and branches when fetch-depth=0 May 26, 2020
@ericsciple ericsciple force-pushed the users/ericsciple/m262depth branch 2 times, most recently from f250f1b to 759d29e Compare May 26, 2020 02:35
src/ref-helper.ts Outdated Show resolved Hide resolved
// Example 1: Consider ref is refs/heads/foo and previously fetched refs/remotes/origin/foo/bar
// Example 2: Consider ref is refs/heads/foo/bar and previously fetched refs/remotes/origin/foo
if (ref) {
ref = ref.startsWith('refs/') ? ref : `refs/heads/${ref}`
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we do the same thing with tags? In case a force push has been made to a tag, a pull wouldn't work.

Copy link
Contributor Author

@ericsciple ericsciple May 26, 2020

Choose a reason for hiding this comment

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

Note, this PR doesn't change that behavior but I did think about tags too.

Technically could happen for tags, however unlikely because tags typically aren't pathy and also commonly never move. Further evidence: we don't handle today and it hasn't been a problem. Same during azp, never saw issue for tag.

If it were to happen here is what the error looks like:

$ git tag pathy/name
$ git tag pathy
fatal: cannot lock ref 'refs/tags/pathy': 'refs/tags/pathy/name' exists; cannot create 'refs/tags/pathy'

Delete the repo and reclone is one way to fix. Otherwise when fetching all history, will be deleted because of git fetch origin --prune '+refs/tags/*:refs/tags/*'

Copy link
Contributor

@thboop thboop left a comment

Choose a reason for hiding this comment

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

LGTM, minor feedback


// When all history is fetched, the ref we're interested in may have moved to a different
// commit (push or force push). If so, fetch again with a targeted refspec.
if (!(await refHelper.testRef(git, settings.ref, settings.commit))) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

print to console?

src/ref-helper.ts Outdated Show resolved Hide resolved
}
// Unexpected
else {
core.debug(`Unexpected ref format '${ref}' when testing ref info`)
Copy link
Member

Choose a reason for hiding this comment

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

do we need to error on unexpected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if i had confidence from telemetry i would error :)

@ericsciple
Copy link
Contributor Author

Note to self, tested E2E:

  • Branch move forward before checkout
  • Branch deleted before checkout
  • PR branch move forward before checkout
  • Tag move before checkout
  • Tag deleted before checkout
  • Conflicts for branch name (both ways)

@ericsciple ericsciple merged commit e52d022 into master May 27, 2020
@ericsciple ericsciple deleted the users/ericsciple/m262depth branch May 27, 2020 13:54
This was referenced Mar 13, 2021
jandubois referenced this pull request in rancher-sandbox/rancher-desktop Jun 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants