git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: "brian m. carlson" <sandals@crustytoothpaste.net>
Cc: git@vger.kernel.org, "Derrick Stolee" <stolee@gmail.com>,
	"Duy Nguyen" <pclouds@gmail.com>,
	"SZEDER Gábor" <szeder.dev@gmail.com>,
	"Jakub Narebski" <jnareb@gmail.com>,
	"Christian Couder" <christian.couder@gmail.com>
Subject: Re: [PATCH v5 10/12] Add a base implementation of SHA-256 support
Date: Sat, 10 Nov 2018 16:52:37 +0100	[thread overview]
Message-ID: <878t2153qy.fsf@evledraar.gmail.com> (raw)
In-Reply-To: <20181107013026.GD890086@genre.crustytoothpaste.net>


On Wed, Nov 07 2018, brian m. carlson wrote:

> On Mon, Nov 05, 2018 at 12:39:14PM +0100, Ævar Arnfjörð Bjarmason wrote:
>> On Sun, Nov 04 2018, brian m. carlson wrote:
>> > +	{
>> > +		"sha256",
>> > +		/* "s256", big-endian */
>>
>> The existing entry/comment for sha1 is:
>>
>> 		"sha1",
>> 		/* "sha1", big-endian */
>>
>> So why the sha256/s256 difference in the code/comment? Wondering if I'm
>> missing something and we're using "s256" for something.
>
> Ah, good question.  The comment refers to the format_id field which
> follows this comment.  The value is the big-endian representation of
> "s256" as a 32-bit value.  I picked that over "sha2" to avoid confusion
> in case we add SHA-512 in the future, since that's also an SHA-2
> algorithm.
>
> Config files and command-line interfaces will use "sha1" or "sha256",
> and binary formats will use those 32-bit values ("sha1" or "s256").

Okey.

>> >  const char *empty_tree_oid_hex(void)
>> > diff --git a/sha256/block/sha256.c b/sha256/block/sha256.c
>> > [...]
>>
>> I had a question before about whether we see ourselves perma-forking
>> this implementation based off libtomcrypt, as I recall you said yes.
>
> Yes.
>
>> Still, I think it would be better to introduce this in at least two-four
>> commits where the upstream code is added as-is, then trimmed down to
>> size, then adapted to our coding style, and finally we add our own
>> utility functions.
>
> At this point, the only code that's actually used from libtomcrypt is
> the block transform.  The upstream code is split over multiple files in
> multiple directories and won't compile in our codebase without many
> files and a lot of work, so I don't feel good about either including
> code that doesn't compile or including large numbers of files that don't
> meet our coding standards (and that may still not compile because of
> platform issues).
>
>> It'll make it easier to forward-port any future upstream changes.
>
> I don't foresee many, if any, changes to this code.  It either
> implements the specification or it doesn't, and it's objectively easy to
> determine which.  There's not even an argument to port performance
> improvements, since almost everyone will be using a crypto library to
> provide this code because libraries perform so dramatically better.
> I've tried to not make the code perform worse than it did originally,
> but that's it.
>
> Furthermore, the modified code carries a relatively small amount of
> resemblance to the original, so if we did port changes forward, we'd
> probably have conflicts.
>
> It seems like you really want to include the upstream code as a separate
> commit and I understand where you're coming from with wanting to have
> this split out into logical commits, but due to the specific nature of
> this code, I see a lot of downsides and not a lot of upsides.

Yeah sorry to keep bringing this up. Your way makes sense, and I'd
forgotten the details since last time . I'll shut up about it:)

>> > +	perl -E "for (1..100000) { print q{aaaaaaaaaa}; }" | \
>> > +		test-tool sha256 >actual &&
>> > +	grep cdc76e5c9914fb9281a1c7e284d73e67f1809a48a497200e046d39ccc7112cd0 actual &&
>> > +	perl -E "for (1..100000) { print q{abcdefghijklmnopqrstuvwxyz}; }" | \
>> > +		test-tool sha256 >actual &&
>>
>> I've been wanting to make use depend on perl >= 5.10 (previous noises
>> about that on-list), but for now we claim to support >=5.8, which
>> doesn't have the -E switch.
>
> Good point.  I'll fix that.  After having written a lot of one-liners,
> I always write -E, and this was originally a one-liner.
>
>> But most importantly you aren't even using -E features here, and this
>> isn't very idoimatic Perl. Instead do, respectively:
>>
>>     perl -e 'print q{aaaaaaaaaa} x 100000'
>>     perl -e "print q{abcdefghijklmnopqrstuvwxyz} x 100000"
>
> I considered the more idiomatic version originally, but the latter could
> allocate a decent amount of memory in one chunk, and I wanted to avoid
> that.

~2.5MB for the latter, so trivial.

> I think what I'd like to do, actually, is turn on autoflush and
> use a postfix for, which would be more idiomatic and could potentially
> provide better testing of the chunking code.  I'll add a comment to that
> effect.

Okey. Maybe better to use syswrite() instead, or maybe print with
autoflush is more idiomatic, I don't know.

  reply	other threads:[~2018-11-10 15:52 UTC|newest]

Thread overview: 58+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-10-25  2:39 [PATCH v4 00/12] Base SHA-256 implementation brian m. carlson
2018-10-25  2:39 ` [PATCH v4 01/12] sha1-file: rename algorithm to "sha1" brian m. carlson
2018-10-25  2:39 ` [PATCH v4 02/12] sha1-file: provide functions to look up hash algorithms brian m. carlson
2018-10-25  2:39 ` [PATCH v4 03/12] hex: introduce functions to print arbitrary hashes brian m. carlson
2018-10-25  2:39 ` [PATCH v4 04/12] cache: make hashcmp and hasheq work with larger hashes brian m. carlson
2018-10-25  2:39 ` [PATCH v4 05/12] t: add basic tests for our SHA-1 implementation brian m. carlson
2018-10-25  2:39 ` [PATCH v4 06/12] t: make the sha1 test-tool helper generic brian m. carlson
2018-10-25  2:40 ` [PATCH v4 07/12] sha1-file: add a constant for hash block size brian m. carlson
2018-10-25  2:40 ` [PATCH v4 08/12] t/helper: add a test helper to compute hash speed brian m. carlson
2018-10-25  2:40 ` [PATCH v4 09/12] commit-graph: convert to using the_hash_algo brian m. carlson
2018-10-25  2:40 ` [PATCH v4 10/12] Add a base implementation of SHA-256 support brian m. carlson
2018-10-25  3:02   ` Carlo Arenas
2018-10-28 15:52     ` brian m. carlson
2018-10-29  0:39       ` Junio C Hamano
2018-10-31 22:55         ` brian m. carlson
2018-11-01  5:29           ` Junio C Hamano
2018-10-27  9:03   ` Christian Couder
2018-10-25  2:40 ` [PATCH v4 11/12] sha256: add an SHA-256 implementation using libgcrypt brian m. carlson
2018-10-25  2:40 ` [PATCH v4 12/12] hash: add an SHA-256 implementation using OpenSSL brian m. carlson
2018-11-04 23:44 ` [PATCH v5 00/12] Base SHA-256 implementation brian m. carlson
2018-11-04 23:44   ` [PATCH v5 01/12] sha1-file: rename algorithm to "sha1" brian m. carlson
2018-11-05  7:21     ` Ævar Arnfjörð Bjarmason
2018-11-04 23:44   ` [PATCH v5 02/12] sha1-file: provide functions to look up hash algorithms brian m. carlson
2018-11-13 18:42     ` Derrick Stolee
2018-11-13 18:45       ` Duy Nguyen
2018-11-14  1:01         ` brian m. carlson
2018-11-14  0:11       ` Ramsay Jones
2018-11-14  0:42         ` Ramsay Jones
2018-11-14  0:51           ` Jeff King
2018-11-14  2:11         ` brian m. carlson
2018-11-14  3:53           ` Ramsay Jones
2018-11-04 23:44   ` [PATCH v5 03/12] hex: introduce functions to print arbitrary hashes brian m. carlson
2018-11-04 23:44   ` [PATCH v5 04/12] cache: make hashcmp and hasheq work with larger hashes brian m. carlson
2018-11-04 23:44   ` [PATCH v5 05/12] t: add basic tests for our SHA-1 implementation brian m. carlson
2018-11-04 23:44   ` [PATCH v5 06/12] t: make the sha1 test-tool helper generic brian m. carlson
2018-11-04 23:44   ` [PATCH v5 07/12] sha1-file: add a constant for hash block size brian m. carlson
2018-11-04 23:44   ` [PATCH v5 08/12] t/helper: add a test helper to compute hash speed brian m. carlson
2018-11-04 23:44   ` [PATCH v5 09/12] commit-graph: convert to using the_hash_algo brian m. carlson
2018-11-04 23:44   ` [PATCH v5 10/12] Add a base implementation of SHA-256 support brian m. carlson
2018-11-05 11:39     ` Ævar Arnfjörð Bjarmason
2018-11-07  1:30       ` brian m. carlson
2018-11-10 15:52         ` Ævar Arnfjörð Bjarmason [this message]
2018-11-04 23:44   ` [PATCH v5 11/12] sha256: add an SHA-256 implementation using libgcrypt brian m. carlson
2018-11-04 23:44   ` [PATCH v5 12/12] hash: add an SHA-256 implementation using OpenSSL brian m. carlson
2018-11-05  2:45   ` [PATCH v5 00/12] Base SHA-256 implementation Junio C Hamano
2018-11-14  4:09   ` [PATCH v6 " brian m. carlson
2018-11-14  4:09     ` [PATCH v6 01/12] sha1-file: rename algorithm to "sha1" brian m. carlson
2018-11-14  4:09     ` [PATCH v6 02/12] sha1-file: provide functions to look up hash algorithms brian m. carlson
2018-11-14  4:09     ` [PATCH v6 03/12] hex: introduce functions to print arbitrary hashes brian m. carlson
2018-11-14  4:09     ` [PATCH v6 04/12] cache: make hashcmp and hasheq work with larger hashes brian m. carlson
2018-11-14  4:09     ` [PATCH v6 05/12] t: add basic tests for our SHA-1 implementation brian m. carlson
2018-11-14  4:09     ` [PATCH v6 06/12] t: make the sha1 test-tool helper generic brian m. carlson
2018-11-14  4:09     ` [PATCH v6 07/12] sha1-file: add a constant for hash block size brian m. carlson
2018-11-14  4:09     ` [PATCH v6 08/12] t/helper: add a test helper to compute hash speed brian m. carlson
2018-11-14  4:09     ` [PATCH v6 09/12] commit-graph: convert to using the_hash_algo brian m. carlson
2018-11-14  4:09     ` [PATCH v6 10/12] Add a base implementation of SHA-256 support brian m. carlson
2018-11-14  4:09     ` [PATCH v6 11/12] sha256: add an SHA-256 implementation using libgcrypt brian m. carlson
2018-11-14  4:09     ` [PATCH v6 12/12] hash: add an SHA-256 implementation using OpenSSL brian m. carlson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=878t2153qy.fsf@evledraar.gmail.com \
    --to=avarab@gmail.com \
    --cc=christian.couder@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=jnareb@gmail.com \
    --cc=pclouds@gmail.com \
    --cc=sandals@crustytoothpaste.net \
    --cc=stolee@gmail.com \
    --cc=szeder.dev@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).