This is an archive of the discontinued Mercurial Phabricator instance.

scmutil: add writereporequirements() and route requires writing through it
ClosedPublic

Authored by pulkit on Jun 15 2020, 11:03 AM.

Details

Summary

In upcoming patches, to implement Share Safe plan we will be introducing
requires file in store. We need to route all callers to a single function
to check for a share-safe requirement and if present, write requirements to
.hg/store/requires instead.

After this patch, callers directly calling scmutil.writerequires() are only
those where we don't have the repo object, for example when initializing
the repository object itself.

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

pulkit created this revision.Jun 15 2020, 11:03 AM

"callers directly calling scmutil.writerequires() are once
where" Do you mean "only" ?

The patch seems to go in the right direction, but the function seems rarely needed enough that is probably worth not making it a method. Could we update the function to take a repo (instead of vs) and act accordingly ?

"callers directly calling scmutil.writerequires() are once
where" Do you mean "only" ?

Yes.

The patch seems to go in the right direction, but the function seems rarely needed enough that is probably worth not making it a method. Could we update the function to take a repo (instead of vs) and act accordingly ?

There are some places where we don't have a repo object. So those we route to scmutil.writerequires which accepts a vfs. Did you mean updating scmutil.writerequires or updating the new method I added?

"callers directly calling scmutil.writerequires() are once
where" Do you mean "only" ?

Yes.

The patch seems to go in the right direction, but the function seems rarely needed enough that is probably worth not making it a method. Could we update the function to take a repo (instead of vs) and act accordingly ?

There are some places where we don't have a repo object. So those we route to scmutil.writerequires which accepts a vfs. Did you mean updating scmutil.writerequires or updating the new method I added?

what I mean is "not having a new method on repo"

"callers directly calling scmutil.writerequires() are once
where" Do you mean "only" ?

Yes.

The patch seems to go in the right direction, but the function seems rarely needed enough that is probably worth not making it a method. Could we update the function to take a repo (instead of vs) and act accordingly ?

There are some places where we don't have a repo object. So those we route to scmutil.writerequires which accepts a vfs. Did you mean updating scmutil.writerequires or updating the new method I added?

what I mean is "not having a new method on repo"

I tried that. It looks a bit ugly because that function will then call repo._writerequires().

"callers directly calling scmutil.writerequires() are once
where" Do you mean "only" ?

Yes.

The patch seems to go in the right direction, but the function seems rarely needed enough that is probably worth not making it a method. Could we update the function to take a repo (instead of vs) and act accordingly ?

There are some places where we don't have a repo object. So those we route to scmutil.writerequires which accepts a vfs. Did you mean updating scmutil.writerequires or updating the new method I added?

what I mean is "not having a new method on repo"

I tried that. It looks a bit ugly because that function will then call repo._writerequires().

I don't get it, why do we need to have a repo method at all ?

pulkit updated this revision to Diff 21675.Jun 22 2020, 5:35 AM
pulkit retitled this revision from localrepo: add writerequirements() and route requires writing through it to scmutil: add writereporequirements() and route requires writing through it.Jun 24 2020, 8:55 AM
pulkit edited the summary of this revision. (Show Details)
pulkit updated this revision to Diff 21694.
pulkit updated this revision to Diff 21738.Jul 1 2020, 5:59 AM

Gentle ping for review.

"callers directly calling scmutil.writerequires() are once
where" Do you mean "only" ?

Yes.

The patch seems to go in the right direction, but the function seems rarely needed enough that is probably worth not making it a method. Could we update the function to take a repo (instead of vs) and act accordingly ?

There are some places where we don't have a repo object. So those we route to scmutil.writerequires which accepts a vfs. Did you mean updating scmutil.writerequires or updating the new method I added?

what I mean is "not having a new method on repo"

I tried that. It looks a bit ugly because that function will then call repo._writerequires().

I don't get it, why do we need to have a repo method at all ?

The patch was updated to not have a method on repo.

This revision was not accepted when it landed; it landed in state Needs Review.
This revision was automatically updated to reflect the committed changes.