1274095 - robustcheckout should reject symbolic revisions in --revision argument

Status: RESOLVED FIXED

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

Description

[Gregory Szorc [:gps]]

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.

Comment 1

[Gregory Szorc [:gps]]

Attachment: MozReview Request: robustcheckout: use changesets in tests (bug 1274095); r?jlund

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/

Comment 2

[Gregory Szorc [:gps]]

Attachment: MozReview Request: robustcheckout: reject --revision arguments that aren't SHA-1s (bug 1274095); r?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 3

[Jordan Lund (:jlund)]

Comment on attachment 8754147 [details]
MozReview Request: robustcheckout: use changesets in tests (bug 1274095); r?jlund

https://reviewboard.mozilla.org/r/53774/#review50838

Comment 4

[Jordan Lund (:jlund)]

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?

Comment 5

[Gregory Szorc [:gps]]

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.

Comment 6

[Gregory Szorc [:gps]]

https://hg.mozilla.org/hgcustom/version-control-tools/rev/18276c917fa111d202530b09d27b52d148150e9e
robustcheckout: use changesets in tests (bug 1274095); r=jlund

https://hg.mozilla.org/hgcustom/version-control-tools/rev/d115bc9578dac4ae2694ea93bb9662de9e6b0b67
robustcheckout: reject --revision arguments that aren't SHA-1s (bug 1274095); r=jlund

Comment 7

[Gregory Szorc [:gps]]

Will file follow-up to get mozharness upgraded.