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

Determine if index clone can be fast-forwarded, if not re-clone #199

Closed
ahmetb opened this issue Jun 3, 2019 · 11 comments · Fixed by #202
Closed

Determine if index clone can be fast-forwarded, if not re-clone #199

ahmetb opened this issue Jun 3, 2019 · 11 comments · Fixed by #202
Labels
kind/proposal priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release.

Comments

@ahmetb
Copy link
Member

ahmetb commented Jun 3, 2019

/kind proposal
/priority important-soon

Once we start offering multiple indexes (#23), inevitably the plugin authors will do weird stuff like rewriting the git history (probably accidentally) and we don't want this to break krew update.

Decision 1: Should we try to handle this case?

So I think, ahead of time, we need to implement something that does:

  1. git fetch-ish something to retrieve the commits (requires connection)
  2. determine if the HEAD can be fast-forwarded (offline operation)
  3. if can be fast-forwarded, go ahead (offline)
  4. if cannot be fast-forwarded (at this point it's safe(?) to assume it's because someone rewrote the history), nuke the local clone and re-clone the repo.

Decision 2 (if decision 1 is accepted): Are the steps above a sound approach to this problem?

@k8s-ci-robot k8s-ci-robot added kind/proposal priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels Jun 3, 2019
@corneliusweig
Copy link
Contributor

I'm 100% for handling this case. We should make clear though that the index is managed by krew and any local changes there will be wiped.

I think that the sequence of steps can be slightly simplified.

  1. git fetch to fetch the current tip
  2. git reset --hard @{u} by default to handle both fast-forward and diverging history cases

@ahmetb
Copy link
Member Author

ahmetb commented Jun 3, 2019

I didn't realize we could do git reset --hard here. What would the {u} be? (master? If so, we should handle the case when people have repos that have a main branch with a different name.)

Do you know if there could be local changes that we need to do git clean -xfd first that might prevent git reset --hard (I assume not).

@corneliusweig
Copy link
Contributor

corneliusweig commented Jun 3, 2019

@{u} is a shorthand for the upstream of the current branch. So on master it would usually resolve to origin/master. In essence, it will always does the right thing, no matter what the branch name is, as long as the upstream branch is set up.

Git will of course not remove untracked local files. One exception is when there is an untracked local file with an identical path as an added file in upstream. Then, git reset --hard overwrites the local file with the upstream one without warning/error (just checked to be sure :).
In other words, we should be safe with git reset --hard and no extra git clean will be necessary.

@ahmetb
Copy link
Member Author

ahmetb commented Jun 3, 2019

So on master it would usually resolve to origin/master.

This sounds a bit fragile, for two reasons:

  1. as I said above default branches of some repos might not be master
  2. I wonder if there are any cases where cloning a repo would make the default remote name something other than origin.

I guess we have to do something like:

branchName := getBranchName(cloneDir)
forceFastForward(cloneDir,"origin/"+branchName)

WDYT?

@corneliusweig
Copy link
Contributor

corneliusweig commented Jun 3, 2019

Oh I think my explanation was misleading.

Let me give an example:

git checkout test
git branch --set-upstream-to cornelius/my-awesome-stuff   # set upstream to an arbitrary branch
git reset --hard @{u}   # will resolve to cornelius/my-awesome-stuff

The above sequence of commands will reset branch test to cornelius/my-awesome-stuff.

Does that make more sense? I think this will work just perfectly.

@ahmetb
Copy link
Member Author

ahmetb commented Jun 3, 2019

Not sure if I follow. How will we know the remote name as well as the "arbitrary branch" name? Is @{u} a magic string recognized by git?

@corneliusweig
Copy link
Contributor

corneliusweig commented Jun 3, 2019

Yes, it's a magic string. It always resolves to the configured upstream branch of the currently active branch.

@ahmetb
Copy link
Member Author

ahmetb commented Jun 3, 2019

Then I'm assuming we don't need

git checkout test
git branch --set-upstream-to cornelius/my-awesome-stuff   # set upstream to an arbitrary branch

because the cloned repos have automatic tracking branch set? (If not, perhaps we can make them so).

@corneliusweig
Copy link
Contributor

corneliusweig commented Jun 3, 2019

Yes. That was only for the sake of the example. The upstream should be correctly configured right after cloning.

@corneliusweig
Copy link
Contributor

corneliusweig commented Jun 3, 2019

I just found out that @{u} is shorthand for @{upstream} 😆

@ahmetb
Copy link
Member Author

ahmetb commented Jun 3, 2019

I have an older clone of krew-index.

$ git branch -vv
* master 5b467ea [origin/master] Bump the version of sort-manifests to v0.2.0 (#151)

shows that the tracking branch is correctly set.

$ git fetch

works, and

$ git reset --hard '@{upstream}'
HEAD is now at 532a299 Add instructions to list all plugins in krew caveat section (#156)

correctly updates when:

  1. I rewrite history, or
  2. I add a new commit, or
  3. I have a conflicting change that's not committed.

So we can move forward with this. 👍

(P.S. messaged you some stuff on Slack)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/proposal priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants