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

don't bother caching if we're using a nil repo #5414

Merged
merged 1 commit into from
Aug 31, 2018

Conversation

Stebalien
Copy link
Member

@Stebalien Stebalien commented Aug 31, 2018

(it'll be wrong anyways)

(it'll be wrong anyways)

License: MIT
Signed-off-by: Steven Allen <steven@stebalien.com>
}

wbs = bstore.NewIdStore(wbs)
bs = bstore.NewIdStore(bs)
Copy link
Member Author

Choose a reason for hiding this comment

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

I renamed this (made it slightly harder to disable caching). Any reason it was wbs?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's fine.

@kevina
Copy link
Contributor

kevina commented Aug 31, 2018

@Stebalien what's the motivation for this change. When is a nil repo used?

@Stebalien
Copy link
Member Author

@Stebalien what's the motivation for this change. When is a nil repo used?

When adding files with --only-hash.

Copy link
Contributor

@kevina kevina left a comment

Choose a reason for hiding this comment

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

I don't fully understand the reasons for this but I don't see the harm.

LGTM.

@Stebalien
Copy link
Member Author

The main reasons are:

  1. It doesn't do anything useful when we have a nil repo.
  2. It may actually record bad information when using a nil repo (because the repo will throw everything away).

In practice, given where we use the nil repo, this really shouldn't make much of a difference (maybe a slight performance gain).

@Stebalien Stebalien merged commit 59805f0 into master Aug 31, 2018
@Stebalien Stebalien deleted the nit/no-caching-nil-repo branch August 31, 2018 21:06
@ghost ghost removed the status/in-progress In progress label Aug 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants