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

reverting and restoring automation section et al... #1340

Merged
merged 1 commit into from Nov 6, 2019

Conversation

equalsJeffH
Copy link
Contributor

@equalsJeffH equalsJeffH commented Nov 5, 2019

somehow I accidentally deleted @nsatragno's automation section when I merged-in the updates to draft-hodges-webauthn-registries*, sigh....

hopefully this fixes it without messing anything else up....

NOTE: WE SHOULD NOT MAKE ANY MERGES TO MASTER UNTIL THIS IS RESOLVED!


Preview | Diff

@equalsJeffH equalsJeffH added this to the L2-WD-02 milestone Nov 5, 2019
@equalsJeffH equalsJeffH self-assigned this Nov 5, 2019
Copy link
Member

@nsatragno nsatragno left a comment

Choose a reason for hiding this comment

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

Thank you, I was wondering where all that text had gone! :)

@equalsJeffH
Copy link
Contributor Author

thx for tugging my sleeve @nsatragno !

Others: PLEASE REVIEW -- it seems this is restoring various other commits also, see https://pr-preview.s3.amazonaws.com/w3c/webauthn/1340/28e8d9d...e2ecccd.html

@equalsJeffH
Copy link
Contributor Author

PLEASE: those nominated to review, please do so (and do so carefully)... upon closer inspection, it seems that I somehow removed the commits in the range [976391d..b5c4001], see this git history excerpt (master branch):

* | | | a55edbf 2019-10-18 | Align with RFC8288, update .txt & .html to be actual rendered versions of .xml (#1329) [=JeffH]
| |_|/  
|/| |   
* | | b5c4001 2019-10-17 | Timeout considerations (#1317) [Akshay Kumar]
* | | 6b03c65 2019-10-17 | revise draft-hodges-webauthn-registries per AD review and initiate work on rev -03 (#1322) [=JeffH]
|/ /  
* | 36fe4ed 2019-10-09 | Add note about use of floating point in extensions passed through that clients do not recognize (#1307) [Mike Jones]
* |   2773c49 2019-10-09 | Merge pull request #1312 from w3c/issue-1309-clarify-rp-server [Emil Lundberg]
|\ \  
| * | 4d16a31 2019-10-07 | Clarify that RP is split into server and script (origin/issue-1309-clarify-rp-server) [Emil Lundberg]
| |/  
* |   72e2f47 2019-10-09 | Merge pull request #1299 from w3c/issue-1188-rp-transports [Emil Lundberg]
|\ \  
| * | 5f3f988 2019-09-11 | Address @equalsJeffH's review comments (origin/issue-1188-rp-transports) [Emil Lundberg]
| * | f599d9b 2019-09-11 | Mention getTransports() in PublicKeyCredentialDescriptor definition [Emil Lundberg]
| * | 5781243 2019-09-11 | Recommend RPs to store transport hints [Emil Lundberg]
| * | 24ecf51 2019-09-11 | Formalise beginning of RP ops [Emil Lundberg]
* | | c7e44f6 2019-10-09 | Add privacy considerations about credential IDs (#1250) [Emil Lundberg]
* | | 00114e9 2019-10-09 | ack Nina (automation/webdriver) (#1310) [=JeffH]
| |/  
|/|   
* |   976391d 2019-10-02 | Merge pull request #1256 from nsatragno/automation [Adam Langley]

It seems merging this branch will restore these commits, but please do verify that this assertion is correct. Much thanks to @nsatragno for noticing and helping with restoration.

Copy link
Contributor

@selfissued selfissued left a comment

Choose a reason for hiding this comment

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

Let's get this restoring PR merged pronto.

@emlun
Copy link
Member

emlun commented Nov 6, 2019

Looks like the offending commit is a55edbf which is a squash of PR #1329 whose HEAD at the time was commit 6bb9b19. I managed to reconstruct the deleted branch, and those two commits have identical contents.

The deletion happened in the merge commit a9bff8b in PR #1329. The PR originally branched from commit 2b435a7, and a9bff8b merged commit b5c4001 from master. I think what happened is that while resolving the merge conflicts in the draft-hodges-webauthn-registries.* files, @equalsJeffH inadvertently removed the index.bs changes from staging, which meant the merge commit reverted them.

The heads that have been merged into master after a55edbf are a1d4e06 (squash of PR #1328), 562cafb, f67c44b, 101146b, 35c25de, b58f3a3, 243d8f7, 235385c (branched after a55edbf), 428bf82 (squash #1316), 97411db (squash #1323), 88468ca (squash #1324), a14e11d (branched after), 03f8406 (squash #1335), dbcf596 (branched after), and e48cb03 (squash #1334).

I've reconstructed a new copy of the merge commit a9bff8b without the accidental revert, and a new master branch merging that followed by the other heads. I'm pleased to say the diff between the reconstructed master and the head of this PR is the empty diff! 😄 I pushed the reconstructed master as emlun-reconstructed-master in case anyone wants to take a look.

Copy link
Member

@emlun emlun left a comment

Choose a reason for hiding this comment

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

Based on my investigation, the contents of commit e2ecccd is exactly identical to what would be in master at this point had the mistake not been made.

@akshayku akshayku merged commit 2e18951 into master Nov 6, 2019
@equalsJeffH equalsJeffH deleted the partially_revert_a55edbf892 branch November 6, 2019 20:08
WebAuthnBot pushed a commit that referenced this pull request Nov 6, 2019
WebAuthnBot pushed a commit that referenced this pull request Nov 6, 2019
@equalsJeffH
Copy link
Contributor Author

equalsJeffH commented Nov 7, 2019

thanks @emlun for the detailed analysis/explanation! (I had not yet seen it during the webauthn call earlier today)!

So, in master branch, I've diffed the spec rendered at the most recent commit 8927216 from the rendered spec at commit 976391d, i.e.: [8927216..976391d], and gone through the diff and matched up new/revised text with the commits listed by git hist in that interval, and it looks like everthing is there in the present most-recent state in master branch, in index.bs -- yay!

HOWEVER, it seems that we've overlooked restoring the revised .svg file that @agl commited & merged-to-master for figure 6 -- the file being: images/string-truncation.svg. see the potentially-relevant commit 428bf82 2019-10-29 | Truncate strings for authenticators where needed. (#1316) [Adam Langley]

@agl likely has this file on his machine and perhaps it's easiest to just open a new PR to update said file (rather than mess around trying to restore it using git-magic) ?

ANYWAY, we shouldn't publish an updated working draft until we've fixed this, please.

@emlun
Copy link
Member

emlun commented Nov 7, 2019

Isn't that the revised version of the image? To me it looks the same as the one in #1316 (comment), and both 428bf82 and master have the same blob for that file:

$ git ls-tree 428bf82 -- images/string-truncation.svg
100644 blob 54197159ae623ad5114d6c2b85e15824cfca1df7    images/string-truncation.svg
$ git ls-tree origin/master -- images/string-truncation.svg
100644 blob 54197159ae623ad5114d6c2b85e15824cfca1df7    images/string-truncation.svg

@agl
Copy link
Contributor

agl commented Nov 7, 2019

The string-truncation.svg currently in master (8927216) matches what I have on disk (SHA-256 96ff3e51827256f413593ee187c25f40f6d58d2a2c930bddf47ff942320c7d71).

@equalsJeffH
Copy link
Contributor Author

equalsJeffH commented Nov 7, 2019

Hm, thanks for checking.... very innaresting -- so yesterday, when visually reviewing the latest rendered spec—i.e. https://w3c.github.io/webauthn/#fig-stringTruncation —in Chrome, the latter image renders (for me anyway) with the vertical dashed lines miss-aligned. I recalled that @agl had committed an updated image (the one pointed to here #1316 (comment)) and so was suprised when the image in the spec in chrome renders mis-aligned (and still does this morning).

In firefox (on glinux and windows) it renders fine (is mis-aligned in chrome on both platforms for me)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants