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

XEP-0234, XEP-0300: Add hash-used element to hashes and make use of it in Jingle File Transfer #497

Merged
merged 2 commits into from Aug 24, 2017

Conversation

Projects
None yet
6 participants
@vanitasvitae
Copy link
Contributor

vanitasvitae commented Aug 21, 2017

@vanitasvitae vanitasvitae force-pushed the vanitasvitae:hashUsed branch from cd970db to ad04cbd Aug 21, 2017

@horazont horazont changed the title Add hash-used element to hashes and make use of it in Jingle File Transfer XEP-0234, XEP-0300: Add hash-used element to hashes and make use of it in Jingle File Transfer Aug 22, 2017

@horazont horazont self-assigned this Aug 22, 2017

@horazont horazont requested a review from tfar Aug 22, 2017

@horazont

This comment has been minimized.

Copy link
Contributor

horazont commented Aug 22, 2017

Soo, @vanitasvitae, this PR will be my first victim, being new to the editor team. So let’s try to make this roll!

I requested a review from @tfar for the XEP-0300 part, and will try to hunt down Lance or Peter for comments on the XEP-0234 part (and possibly the XEP-0300 part).

@horazont horazont requested a review from stpeter Aug 22, 2017

@stpeter

This comment has been minimized.

Copy link
Member

stpeter commented Aug 22, 2017

@horazont You can find Lance via @legastero

@tfar

tfar approved these changes Aug 23, 2017

Copy link
Contributor

tfar left a comment

LGTM

</tr>
<tr>
<td>hash-used</td>
<td>Alternatively to a &lt;hash/&gt; element, the initiator can also include a &lt;hash-used/&gt; element. This comes handy to avoid reading the file twice to calculate the hash.</td>

This comment has been minimized.

@stpeter

stpeter Aug 23, 2017

Member

I don't think we mean "Alternatively" here because it seems that the "hash-used" element is sent in addition to, not instead of, the "hash" element (at least if I'm reading the text below correctly). An example would be helpful. English nit: "This comes in handy..." (or, less colloquially, "This avoids the need to read...").

This comment has been minimized.

@Flowdalic

Flowdalic Aug 23, 2017

Contributor

it seems that the "hash-used" element is sent in addition to,

Just to clarify: The idea is that it is not send in addition to. You either send a hash element, which includes the hash algo and the hash value, OR you send a hash-used, which just includes the hash algo.

This comment has been minimized.

@stpeter

stpeter Aug 23, 2017

Member

If so, then I must be misreading the text around line 799, which says:

In such a case however, the session-initiate message MUST contain a <hash-used/> element.... Additionally, the <checksum/> element MUST contain a <file/> element which MUST contain at least one <hash/> element....

In this situation, would there be both a "hash-used" element and a "hash" element?

This comment has been minimized.

@vanitasvitae

vanitasvitae Aug 23, 2017

Contributor

I probably forgot to change the second sentence. It should be changed to say "at least one hash or hash-used element".

@stpeter

This comment has been minimized.

Copy link
Member

stpeter commented Aug 23, 2017

That'd be better. ;-) +1

@vanitasvitae vanitasvitae force-pushed the vanitasvitae:hashUsed branch from ad04cbd to 79ea214 Aug 23, 2017

@vanitasvitae

This comment has been minimized.

Copy link
Contributor

vanitasvitae commented Aug 23, 2017

I changed the wording.

@horazont

This comment has been minimized.

Copy link
Contributor

horazont commented Aug 23, 2017

@stpeter Do you, as author, approve the XEP-0234 changes in this PR and/or shall we wait for Lance’ comments?

@legastero

This comment has been minimized.

Copy link
Contributor

legastero commented Aug 23, 2017

@stpeter

This comment has been minimized.

Copy link
Member

stpeter commented Aug 23, 2017

+1 here as well @horazont

@horazont horazont merged commit 79ea214 into xsf:master Aug 24, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment