git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Taylor Blau <me@ttaylorr.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org, martin.agren@gmail.com,
	sandals@crustytoothpaste.net, me@ttaylorr.com,
	abhishekkumar8222@gmail.com,
	Derrick Stolee <derrickstolee@github.com>,
	Derrick Stolee <dstolee@microsoft.com>
Subject: Re: [PATCH 2/3] commit-graph: use the hash version byte
Date: Fri, 14 Aug 2020 16:05:52 -0400	[thread overview]
Message-ID: <20200814200552.GA30103@syl.lan> (raw)
In-Reply-To: <xmqqo8ndvyje.fsf@gitster.c.googlers.com>

On Fri, Aug 14, 2020 at 12:05:09PM -0700, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Derrick Stolee <dstolee@microsoft.com>
> >
> > The commit-graph format reserved a byte among the header of the file to
> > store a "hash version". During the SHA-256 work, this was not modified
> > because file formats are not necessarily intended to work across hash
> > versions. If a repository has SHA-256 as its hash algorithm, it
> > automatically up-shifts the lengths of object names in all necessary
> > formats.
> >
> > However, since we have this byte available for adjusting the version, we
> > can make the file formats more obviously incompatible instead of relying
> > on other context from the repository.
>
> Very good idea.
>
> > Update the oid_version() method in commit-graph.c to add a new value, 2,
> > for sha-256. This automatically writes the new value in a SHA-256
> > repository _and_ verifies the value is correct. This is a breaking
> > change relative to the current 'master' branch since 092b677 (Merge
> > branch 'bc/sha-256-cvs-svn-updates', 2020-08-13) but it is not breaking
> > relative to any released version of Git.
>
> That is perfectly fine.  I think any file and on-wire protocol that
> uses anything but SHA-1 without identifying what it uses is a bug.
>
> Will queue.  Thanks.

Great. I have nothing to add other than my 'ack' that this is a good
idea.

> > +OID_VERSION=1
> > +if [ "$GIT_DEFAULT_HASH" = "sha256" ]
> > +then
> > +	OID_VERSION=2
> > +fi
> > +
> >  test_expect_success 'setup test - repo, commits, commit graph, log outputs' '
> >  	git init &&
> >  	mkdir A A/B A/B/C &&
> > @@ -35,7 +41,7 @@ test_expect_success 'setup test - repo, commits, commit graph, log outputs' '
> >  graph_read_expect () {
> >  	NUM_CHUNKS=5
> >  	cat >expect <<- EOF
> > -	header: 43475048 1 1 $NUM_CHUNKS 0
> > +	header: 43475048 1 $OID_VERSION $NUM_CHUNKS 0
> >  	num_commits: $1
> >  	chunks: oid_fanout oid_lookup commit_metadata bloom_indexes bloom_data
> >  	EOF
> > diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
> > index 044cf8a3de..5b65017676 100755
> > --- a/t/t5318-commit-graph.sh
> > +++ b/t/t5318-commit-graph.sh
> > @@ -5,6 +5,12 @@ test_description='commit graph'
> >
> >  GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS=0
> >
> > +OID_VERSION=1
> > +if [ "$GIT_DEFAULT_HASH" = "sha256" ]
> > +then
> > +	OID_VERSION=2
> > +fi
> > +
> >  test_expect_success 'setup full repo' '
> >  	mkdir full &&
> >  	cd "$TRASH_DIRECTORY/full" &&
> > @@ -77,7 +83,7 @@ graph_read_expect() {
> >  		NUM_CHUNKS=$((3 + $(echo "$2" | wc -w)))
> >  	fi
> >  	cat >expect <<- EOF
> > -	header: 43475048 1 1 $NUM_CHUNKS 0
> > +	header: 43475048 1 $OID_VERSION $NUM_CHUNKS 0
> >  	num_commits: $1
> >  	chunks: oid_fanout oid_lookup commit_metadata$OPTIONAL
> >  	EOF
> > @@ -412,6 +418,35 @@ test_expect_success 'replace-objects invalidates commit-graph' '
> >  	)
> >  '
> >
> > +test_expect_success 'warn on improper hash version' '
> > +	git init --object-format=sha1 sha1 &&
> > +	(
> > +		cd sha1 &&
> > +		test_commit 1 &&
> > +		git commit-graph write --reachable &&
> > +		mv .git/objects/info/commit-graph ../cg-sha1
> > +	) &&
> > +	git init --object-format=sha256 sha256 &&
> > +	(
> > +		cd sha256 &&
> > +		test_commit 1 &&
> > +		git commit-graph write --reachable &&
> > +		mv .git/objects/info/commit-graph ../cg-sha256
> > +	) &&
> > +	(
> > +		cd sha1 &&
> > +		mv ../cg-sha256 .git/objects/info/commit-graph &&
> > +		git log -1 2>err &&
> > +		test_i18ngrep "commit-graph hash version 2 does not match version 1" err
> > +	) &&
> > +	(
> > +		cd sha256 &&
> > +		mv ../cg-sha1 .git/objects/info/commit-graph &&
> > +		git log -1 2>err &&
> > +		test_i18ngrep "commit-graph hash version 1 does not match version 2" err
> > +	)
> > +'
> > +
> >  # the verify tests below expect the commit-graph to contain
> >  # exactly the commits reachable from the commits/8 branch.
> >  # If the file changes the set of commits in the list, then the
> > diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh
> > index ea28d522b8..6f1a324f4f 100755
> > --- a/t/t5324-split-commit-graph.sh
> > +++ b/t/t5324-split-commit-graph.sh
> > @@ -6,6 +6,12 @@ test_description='split commit graph'
> >  GIT_TEST_COMMIT_GRAPH=0
> >  GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS=0
> >
> > +OID_VERSION=1
> > +if [ "$GIT_DEFAULT_HASH" = "sha256" ]
> > +then
> > +	OID_VERSION=2
> > +fi
> > +
> >  test_expect_success 'setup repo' '
> >  	git init &&
> >  	git config core.commitGraph true &&
> > @@ -28,7 +34,7 @@ graph_read_expect() {
> >  		NUM_BASE=$2
> >  	fi
> >  	cat >expect <<- EOF
> > -	header: 43475048 1 1 3 $NUM_BASE
> > +	header: 43475048 1 $OID_VERSION 3 $NUM_BASE
> >  	num_commits: $1
> >  	chunks: oid_fanout oid_lookup commit_metadata
> >  	EOF
Thanks,
Taylor

  reply	other threads:[~2020-08-14 20:06 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-14 18:07 [PATCH 0/3] SHA-256: Update commit-graph and multi-pack-index formats Derrick Stolee via GitGitGadget
2020-08-14 18:07 ` [PATCH 1/3] t/README: document GIT_TEST_DEFAULT_HASH Derrick Stolee via GitGitGadget
2020-08-14 19:02   ` Junio C Hamano
2020-08-14 20:39   ` Eric Sunshine
2020-08-14 20:41     ` Derrick Stolee
2020-08-14 18:07 ` [PATCH 2/3] commit-graph: use the hash version byte Derrick Stolee via GitGitGadget
2020-08-14 19:05   ` Junio C Hamano
2020-08-14 20:05     ` Taylor Blau [this message]
2020-08-14 20:11   ` brian m. carlson
2020-08-14 20:22     ` Junio C Hamano
2020-08-14 20:36     ` Derrick Stolee
2020-08-15 13:46       ` Martin Ågren
2020-08-14 18:07 ` [PATCH 3/3] multi-pack-index: use " Derrick Stolee via GitGitGadget
2020-08-14 20:14   ` brian m. carlson
2020-08-14 19:25 ` [PATCH 0/3] SHA-256: Update commit-graph and multi-pack-index formats Junio C Hamano
2020-08-14 20:34   ` Derrick Stolee
2020-08-14 21:41     ` Junio C Hamano
2020-08-17 14:04 ` [PATCH v2 " Derrick Stolee via GitGitGadget
2020-08-17 14:04   ` [PATCH v2 1/3] t/README: document GIT_TEST_DEFAULT_HASH Derrick Stolee via GitGitGadget
2020-08-17 14:04   ` [PATCH v2 2/3] commit-graph: use the "hash version" byte Derrick Stolee via GitGitGadget
2020-08-17 14:04   ` [PATCH v2 3/3] multi-pack-index: use hash version byte Derrick Stolee via GitGitGadget
2020-08-17 23:12   ` [PATCH v2 0/3] SHA-256: Update commit-graph and multi-pack-index formats 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=20200814200552.GA30103@syl.lan \
    --to=me@ttaylorr.com \
    --cc=abhishekkumar8222@gmail.com \
    --cc=derrickstolee@github.com \
    --cc=dstolee@microsoft.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=gitster@pobox.com \
    --cc=martin.agren@gmail.com \
    --cc=sandals@crustytoothpaste.net \
    --subject='Re: [PATCH 2/3] commit-graph: use the hash version byte' \
    /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

Code repositories for project(s) associated with this 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).