Closed Bug 1274095 Opened 4 years ago Closed 4 years ago

robustcheckout should reject symbolic revisions in --revision argument

Categories

(Developer Services :: Mercurial: hg.mozilla.org, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: gps, Assigned: gps)

References

Details

Attachments

(2 files)

If a symbolic revision e.g. "default" is passed to --revision it should be rejected. This is because we only do a `hg pull` for symbolic revisions to avoid the round trip to the server to see if we have all data.

If we're going to implement this, we might as well consolidate --revision and --branch since we'll have logic in place to detect symbolic revisions from --revision arguments.
We're going to change --revision to be more strict about the arguments
it receives. Make the tests abide by the future rules before we do that.

Review commit: https://reviewboard.mozilla.org/r/53774/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53774/
Attachment #8754147 - Flags: review?(jlund)
Attachment #8754148 - Flags: review?(jlund)
We take shortcuts with regards to handling the --revision argument: if
the value resolves to a known revision, we skip pulling from the remote
and checkout directly. This is faster. But it assumes --revision is
constant. --revision isn't constant if it is symbolic. e.g. "default"
or "tip." To prevent these kinds of errors, reject values to --revision
that don't resemble SHA-1 fragments.

We enforce the check in 2 locations: first as a visual check during
argument processing then again when checking the revision is known
locally. The 2nd only occurs if the revision's value looks like a SHA-1
fragment but isn't a known SHA-1 fragment.

Review commit: https://reviewboard.mozilla.org/r/53776/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53776/
Comment on attachment 8754147 [details]
MozReview Request: robustcheckout: use changesets in tests (bug 1274095); r?jlund

https://reviewboard.mozilla.org/r/53774/#review50838
Attachment #8754147 - Flags: review?(jlund) → review+
Comment on attachment 8754148 [details]
MozReview Request: robustcheckout: reject --revision arguments that aren't SHA-1s (bug 1274095); r?jlund

https://reviewboard.mozilla.org/r/53776/#review50840

seems sane enough to me

::: hgext/robustcheckout/__init__.py:251
(Diff revision 1)
>      if revision:
> -        havewantedrev = revision in repo
> +        if revision in repo:

this can probably be one line now: `if revision and revision in repo:`

::: hgext/robustcheckout/__init__.py:253
(Diff revision 1)
>  
>      repo = hg.repository(ui, dest)
>      havewantedrev = False
>      if revision:
> -        havewantedrev = revision in repo
> +        if revision in repo:
> +            ctx = repo[revision]

bear with me, how is `revision` used as a key of a repo if its length can be 12-40chars?

is `repo` special and can shrinks hash lookup before returning value?
Attachment #8754148 - Flags: review?(jlund) → review+
https://reviewboard.mozilla.org/r/53776/#review50840

> bear with me, how is `revision` used as a key of a repo if its length can be 12-40chars?
> 
> is `repo` special and can shrinks hash lookup before returning value?

Yeah, repo.__getitem__ accepts pretty much anything. An integer. A hex SHA-1 or fragment. A binary SHA-1. A symbolic name. Pretty much any Mercurial identifier.
Will file follow-up to get mozharness upgraded.
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Blocks: 1274693
You need to log in before you can comment on or make changes to this bug.