diff mbox series

[v2,4/6] refs: add REF_SKIP_REFNAME_VERIFICATION flag

Message ID 0a297f0c3e884c2d4cfb24a6d3b9f1973fc83125.1638211786.git.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series Allow writing invalid OIDs into refs for testing purposes | expand

Commit Message

Han-Wen Nienhuys Nov. 29, 2021, 6:49 p.m. UTC 
From: Han-Wen Nienhuys <hanwen@google.com>

Use this flag with the test-helper in t1430, to avoid direct writes to the ref
database.

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
---
 refs.c                  |  7 ++---
 refs.h                  | 10 +++++--
 t/t1430-bad-ref-name.sh | 59 ++++++++++++++++++++++-------------------
 3 files changed, 44 insertions(+), 32 deletions(-)

Comments

Junio C Hamano Nov. 29, 2021, 11:22 p.m. UTC | #1 
"Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Han-Wen Nienhuys <hanwen@google.com>
>
> Use this flag with the test-helper in t1430, to avoid direct writes to the ref
> database.

OK, this is similar to 3/6 where invalid object names are allowed.
This is about allowing invalid refnames.  Makes sense.



> Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
> ---
>  refs.c                  |  7 ++---
>  refs.h                  | 10 +++++--
>  t/t1430-bad-ref-name.sh | 59 ++++++++++++++++++++++-------------------
>  3 files changed, 44 insertions(+), 32 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index d7cc0a23a3b..086b1341d2a 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1078,9 +1078,10 @@ int ref_transaction_update(struct ref_transaction *transaction,
>  {
>  	assert(err);
>  
> -	if ((new_oid && !is_null_oid(new_oid)) ?
> -	    check_refname_format(refname, REFNAME_ALLOW_ONELEVEL) :
> -	    !refname_is_safe(refname)) {
> +	if (!(flags & REF_SKIP_REFNAME_VERIFICATION) &&
> +	    ((new_oid && !is_null_oid(new_oid)) ?
> +		     check_refname_format(refname, REFNAME_ALLOW_ONELEVEL) :
> +			   !refname_is_safe(refname))) {

OK.  Initially my eyes went "Huh?", but it was just re-indentation
that fooled them.

> @@ -617,12 +617,18 @@ struct ref_transaction *ref_transaction_begin(struct strbuf *err);
>   */
>  #define REF_SKIP_OID_VERIFICATION (1 << 10)
>  
> +/*
> + * Skip verifying refname. This is useful for testing data corruption scenarios.
> + */
> +#define REF_SKIP_REFNAME_VERIFICATION (1 << 11)

OK.
Junio C Hamano Nov. 29, 2021, 11:31 p.m. UTC | #2 
"Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com> writes:

> +	if (!(flags & REF_SKIP_REFNAME_VERIFICATION) &&
> +	    ((new_oid && !is_null_oid(new_oid)) ?
> +		     check_refname_format(refname, REFNAME_ALLOW_ONELEVEL) :
> +			   !refname_is_safe(refname))) {

So, if somebody passes REF_SKIP_REFNAME_VERIFICATION in flags, we
will not do the check.

Again, like 3/6, this new bit is flipped on by test-helper
somewhere?  Again I do not see anybody doing so in these 6 patches,
but I should double check.
Han-Wen Nienhuys Nov. 30, 2021, 10:31 a.m. UTC | #3 
On Tue, Nov 30, 2021 at 12:31 AM Junio C Hamano <gitster@pobox.com> wrote:
> > +     if (!(flags & REF_SKIP_REFNAME_VERIFICATION) &&
> > +         ((new_oid && !is_null_oid(new_oid)) ?
> > +                  check_refname_format(refname, REFNAME_ALLOW_ONELEVEL) :
> > +                        !refname_is_safe(refname))) {
>
> So, if somebody passes REF_SKIP_REFNAME_VERIFICATION in flags, we
> will not do the check.
>
> Again, like 3/6, this new bit is flipped on by test-helper
> somewhere?  Again I do not see anybody doing so in these 6 patches,
> but I should double check.

The test helper takes the flag as an argument, in decimal. If you look
for 2048, you should find it.
Junio C Hamano Dec. 1, 2021, 7 p.m. UTC | #4 
Han-Wen Nienhuys <hanwen@google.com> writes:

> On Tue, Nov 30, 2021 at 12:31 AM Junio C Hamano <gitster@pobox.com> wrote:
>> > +     if (!(flags & REF_SKIP_REFNAME_VERIFICATION) &&
>> > +         ((new_oid && !is_null_oid(new_oid)) ?
>> > +                  check_refname_format(refname, REFNAME_ALLOW_ONELEVEL) :
>> > +                        !refname_is_safe(refname))) {
>>
>> So, if somebody passes REF_SKIP_REFNAME_VERIFICATION in flags, we
>> will not do the check.
>>
>> Again, like 3/6, this new bit is flipped on by test-helper
>> somewhere?  Again I do not see anybody doing so in these 6 patches,
>> but I should double check.
>
> The test helper takes the flag as an argument, in decimal. If you look
> for 2048, you should find it.

Awful---when the symbolic constants change in the code, the test
will silently break?

It has been this way since 80f2a609 (t/helper: add test-ref-store to
test ref-store functions, 2017-03-26), so it is nothing new, but at
some point, we should do a better job.  Even if it is used only as a
tool for testing, we shouldn't have to force developers to write in
assembly ;-)

Perhaps when the dust settles after this series graduates to be a
part of released version.  It may be a good bite-sized microproject
material for aspiring new developers.
Jeff King Dec. 1, 2021, 7:26 p.m. UTC | #5 
On Wed, Dec 01, 2021 at 11:00:04AM -0800, Junio C Hamano wrote:

> > The test helper takes the flag as an argument, in decimal. If you look
> > for 2048, you should find it.
> 
> Awful---when the symbolic constants change in the code, the test
> will silently break?

Agreed, this is quite nasty.

> It has been this way since 80f2a609 (t/helper: add test-ref-store to
> test ref-store functions, 2017-03-26), so it is nothing new, but at
> some point, we should do a better job.  Even if it is used only as a
> tool for testing, we shouldn't have to force developers to write in
> assembly ;-)

Sort of. The code to pass the flags was added then, but nobody was using
it until now.

At least for ref updates. Symref creation allowed "1" for force, but
that is a true boolean. It looks like some pack-refs calls pass "3" for
PACK_REFS_PRUNE|PACK_REFS_ALL.

So it may be considered a "new" issue in that sense.

> Perhaps when the dust settles after this series graduates to be a
> part of released version.  It may be a good bite-sized microproject
> material for aspiring new developers.

It is annoying to have to plumb through the names of the flags, but in
practice I don't think we need all (or even most) of them. And it's
equally annoying not to be able to grep for the flags. I already had a
hard enough time just grepping for the callers, and resorted to:

diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c
index 3986665037..1b7a124a86 100644
--- a/t/helper/test-ref-store.c
+++ b/t/helper/test-ref-store.c
@@ -271,6 +271,8 @@ int cmd__ref_store(int argc, const char **argv)
 	const char *func;
 	struct command *cmd;
 
+	trace_argv_printf(argv, "trace: test-tool:");
+
 	setup_git_directory();
 
 	argv = get_store(argv + 1, &refs);

to find them.

-Peff
Han-Wen Nienhuys Dec. 2, 2021, 4:40 p.m. UTC | #6 
On Wed, Dec 1, 2021 at 8:26 PM Jeff King <peff@peff.net> wrote:
>
> On Wed, Dec 01, 2021 at 11:00:04AM -0800, Junio C Hamano wrote:
>
> > > The test helper takes the flag as an argument, in decimal. If you look
> > > for 2048, you should find it.
> >
> > Awful---when the symbolic constants change in the code, the test
> > will silently break?
>
> Agreed, this is quite nasty.

I've added parsing symbolic constants in v3.
Junio C Hamano Dec. 2, 2021, 7:05 p.m. UTC | #7 
Han-Wen Nienhuys <hanwen@google.com> writes:

> On Wed, Dec 1, 2021 at 8:26 PM Jeff King <peff@peff.net> wrote:
>>
>> On Wed, Dec 01, 2021 at 11:00:04AM -0800, Junio C Hamano wrote:
>>
>> > > The test helper takes the flag as an argument, in decimal. If you look
>> > > for 2048, you should find it.
>> >
>> > Awful---when the symbolic constants change in the code, the test
>> > will silently break?
>>
>> Agreed, this is quite nasty.
>
> I've added parsing symbolic constants in v3.

Thanks for going an extra mile.
diff mbox series

Patch

diff --git a/refs.c b/refs.c
index d7cc0a23a3b..086b1341d2a 100644
--- a/refs.c
+++ b/refs.c
@@ -1078,9 +1078,10 @@  int ref_transaction_update(struct ref_transaction *transaction,
 {
 	assert(err);
 
-	if ((new_oid && !is_null_oid(new_oid)) ?
-	    check_refname_format(refname, REFNAME_ALLOW_ONELEVEL) :
-	    !refname_is_safe(refname)) {
+	if (!(flags & REF_SKIP_REFNAME_VERIFICATION) &&
+	    ((new_oid && !is_null_oid(new_oid)) ?
+		     check_refname_format(refname, REFNAME_ALLOW_ONELEVEL) :
+			   !refname_is_safe(refname))) {
 		strbuf_addf(err, _("refusing to update ref with bad name '%s'"),
 			    refname);
 		return -1;
diff --git a/refs.h b/refs.h
index 46e68fd4c2a..684f8b33d17 100644
--- a/refs.h
+++ b/refs.h
@@ -617,12 +617,18 @@  struct ref_transaction *ref_transaction_begin(struct strbuf *err);
  */
 #define REF_SKIP_OID_VERIFICATION (1 << 10)
 
+/*
+ * Skip verifying refname. This is useful for testing data corruption scenarios.
+ */
+#define REF_SKIP_REFNAME_VERIFICATION (1 << 11)
+
 /*
  * Bitmask of all of the flags that are allowed to be passed in to
  * ref_transaction_update() and friends:
  */
-#define REF_TRANSACTION_UPDATE_ALLOWED_FLAGS \
-	(REF_NO_DEREF | REF_FORCE_CREATE_REFLOG | REF_SKIP_OID_VERIFICATION)
+#define REF_TRANSACTION_UPDATE_ALLOWED_FLAGS                                  \
+	(REF_NO_DEREF | REF_FORCE_CREATE_REFLOG | REF_SKIP_OID_VERIFICATION | \
+	 REF_SKIP_REFNAME_VERIFICATION)
 
 /*
  * Add a reference update to transaction. `new_oid` is the value that
diff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh
index 4c77cf89a6c..56d0726944e 100755
--- a/t/t1430-bad-ref-name.sh
+++ b/t/t1430-bad-ref-name.sh
@@ -9,7 +9,8 @@  TEST_PASSES_SANITIZE_LEAK=true
 
 test_expect_success setup '
 	test_commit one &&
-	test_commit two
+	test_commit two &&
+	main_sha1=$(git rev-parse refs/heads/main)
 '
 
 test_expect_success 'fast-import: fail on invalid branch name ".badbranchname"' '
@@ -43,16 +44,16 @@  test_expect_success 'fast-import: fail on invalid branch name "bad[branch]name"'
 '
 
 test_expect_success 'git branch shows badly named ref as warning' '
-	cp .git/refs/heads/main .git/refs/heads/broken...ref &&
-	test_when_finished "rm -f .git/refs/heads/broken...ref" &&
+	test-tool ref-store main update-ref msg "refs/heads/broken...ref" $main_sha1 $ZERO_OID 2048 &&
+	test_when_finished "test-tool ref-store main delete-refs 1 msg refs/heads/broken...ref" &&
 	git branch >output 2>error &&
 	test_i18ngrep -e "ignoring ref with broken name refs/heads/broken\.\.\.ref" error &&
 	! grep -e "broken\.\.\.ref" output
 '
 
 test_expect_success 'branch -d can delete badly named ref' '
-	cp .git/refs/heads/main .git/refs/heads/broken...ref &&
-	test_when_finished "rm -f .git/refs/heads/broken...ref" &&
+	test-tool ref-store main update-ref msg "refs/heads/broken...ref" $main_sha1 $ZERO_OID 2048 &&
+	test_when_finished "test-tool ref-store main delete-refs 1 msg refs/heads/broken...ref" &&
 	git branch -d broken...ref &&
 	git branch >output 2>error &&
 	! grep -e "broken\.\.\.ref" error &&
@@ -60,8 +61,8 @@  test_expect_success 'branch -d can delete badly named ref' '
 '
 
 test_expect_success 'branch -D can delete badly named ref' '
-	cp .git/refs/heads/main .git/refs/heads/broken...ref &&
-	test_when_finished "rm -f .git/refs/heads/broken...ref" &&
+	test-tool ref-store main update-ref msg "refs/heads/broken...ref" $main_sha1 $ZERO_OID 2048 &&
+	test_when_finished "test-tool ref-store main delete-refs 1 msg refs/heads/broken...ref" &&
 	git branch -D broken...ref &&
 	git branch >output 2>error &&
 	! grep -e "broken\.\.\.ref" error &&
@@ -90,7 +91,7 @@  test_expect_success 'branch -D cannot delete absolute path' '
 '
 
 test_expect_success 'git branch cannot create a badly named ref' '
-	test_when_finished "rm -f .git/refs/heads/broken...ref" &&
+	test_when_finished "test-tool ref-store main delete-refs 1 msg refs/heads/broken...ref" &&
 	test_must_fail git branch broken...ref &&
 	git branch >output 2>error &&
 	! grep -e "broken\.\.\.ref" error &&
@@ -98,7 +99,7 @@  test_expect_success 'git branch cannot create a badly named ref' '
 '
 
 test_expect_success 'branch -m cannot rename to a bad ref name' '
-	test_when_finished "rm -f .git/refs/heads/broken...ref" &&
+	test_when_finished "test-tool ref-store main delete-refs 1 msg refs/heads/broken...ref" &&
 	test_might_fail git branch -D goodref &&
 	git branch goodref &&
 	test_must_fail git branch -m goodref broken...ref &&
@@ -109,8 +110,9 @@  test_expect_success 'branch -m cannot rename to a bad ref name' '
 '
 
 test_expect_failure 'branch -m can rename from a bad ref name' '
-	cp .git/refs/heads/main .git/refs/heads/broken...ref &&
-	test_when_finished "rm -f .git/refs/heads/broken...ref" &&
+	test-tool ref-store main update-ref msg "refs/heads/broken...ref" $main_sha1 $ZERO_OID 2048 &&
+
+	test_when_finished "test-tool ref-store main delete-refs 1 msg refs/heads/broken...ref" &&
 	git branch -m broken...ref renamed &&
 	test_cmp_rev main renamed &&
 	git branch >output 2>error &&
@@ -119,7 +121,7 @@  test_expect_failure 'branch -m can rename from a bad ref name' '
 '
 
 test_expect_success 'push cannot create a badly named ref' '
-	test_when_finished "rm -f .git/refs/heads/broken...ref" &&
+	test_when_finished "test-tool ref-store main delete-refs 1 msg refs/heads/broken...ref" &&
 	test_must_fail git push "file://$(pwd)" HEAD:refs/heads/broken...ref &&
 	git branch >output 2>error &&
 	! grep -e "broken\.\.\.ref" error &&
@@ -139,7 +141,7 @@  test_expect_failure 'push --mirror can delete badly named ref' '
 		cd dest &&
 		test_commit two &&
 		git checkout --detach &&
-		cp .git/refs/heads/main .git/refs/heads/broken...ref
+		test-tool ref-store main update-ref msg "refs/heads/broken...ref" $main_sha1 $ZERO_OID 2048
 	) &&
 	git -C src push --mirror "file://$top/dest" &&
 	git -C dest branch >output 2>error &&
@@ -148,9 +150,9 @@  test_expect_failure 'push --mirror can delete badly named ref' '
 '
 
 test_expect_success 'rev-parse skips symref pointing to broken name' '
-	test_when_finished "rm -f .git/refs/heads/broken...ref" &&
+	test_when_finished "test-tool ref-store main delete-refs 1 msg refs/heads/broken...ref" &&
 	git branch shadow one &&
-	cp .git/refs/heads/main .git/refs/heads/broken...ref &&
+	test-tool ref-store main update-ref msg "refs/heads/broken...ref" $main_sha1 $ZERO_OID 2048 &&
 	printf "ref: refs/heads/broken...ref\n" >.git/refs/tags/shadow &&
 	test_when_finished "rm -f .git/refs/tags/shadow" &&
 	git rev-parse --verify one >expect &&
@@ -160,8 +162,8 @@  test_expect_success 'rev-parse skips symref pointing to broken name' '
 '
 
 test_expect_success 'for-each-ref emits warnings for broken names' '
-	cp .git/refs/heads/main .git/refs/heads/broken...ref &&
-	test_when_finished "rm -f .git/refs/heads/broken...ref" &&
+	test-tool ref-store main update-ref msg "refs/heads/broken...ref" $main_sha1 $ZERO_OID 2048 &&
+	test_when_finished "test-tool ref-store main delete-refs 1 msg refs/heads/broken...ref" &&
 	printf "ref: refs/heads/broken...ref\n" >.git/refs/heads/badname &&
 	test_when_finished "rm -f .git/refs/heads/badname" &&
 	printf "ref: refs/heads/main\n" >.git/refs/heads/broken...symref &&
@@ -176,8 +178,8 @@  test_expect_success 'for-each-ref emits warnings for broken names' '
 '
 
 test_expect_success 'update-ref -d can delete broken name' '
-	cp .git/refs/heads/main .git/refs/heads/broken...ref &&
-	test_when_finished "rm -f .git/refs/heads/broken...ref" &&
+	test-tool ref-store main update-ref msg "refs/heads/broken...ref" $main_sha1 $ZERO_OID 2048 &&
+	test_when_finished "test-tool ref-store main delete-refs 1 msg refs/heads/broken...ref" &&
 	git update-ref -d refs/heads/broken...ref >output 2>error &&
 	test_must_be_empty output &&
 	test_must_be_empty error &&
@@ -187,8 +189,8 @@  test_expect_success 'update-ref -d can delete broken name' '
 '
 
 test_expect_success 'branch -d can delete broken name' '
-	cp .git/refs/heads/main .git/refs/heads/broken...ref &&
-	test_when_finished "rm -f .git/refs/heads/broken...ref" &&
+	test-tool ref-store main update-ref msg "refs/heads/broken...ref" $main_sha1 $ZERO_OID 2048 &&
+	test_when_finished "test-tool ref-store main delete-refs 1 msg refs/heads/broken...ref" &&
 	git branch -d broken...ref >output 2>error &&
 	test_i18ngrep "Deleted branch broken...ref (was broken)" output &&
 	test_must_be_empty error &&
@@ -198,8 +200,9 @@  test_expect_success 'branch -d can delete broken name' '
 '
 
 test_expect_success 'update-ref --no-deref -d can delete symref to broken name' '
-	cp .git/refs/heads/main .git/refs/heads/broken...ref &&
-	test_when_finished "rm -f .git/refs/heads/broken...ref" &&
+	test-tool ref-store main update-ref msg "refs/heads/broken...ref" $main_sha1 $ZERO_OID 2048 &&
+
+	test_when_finished "test-tool ref-store main delete-refs 1 msg refs/heads/broken...ref" &&
 	printf "ref: refs/heads/broken...ref\n" >.git/refs/heads/badname &&
 	test_when_finished "rm -f .git/refs/heads/badname" &&
 	git update-ref --no-deref -d refs/heads/badname >output 2>error &&
@@ -209,8 +212,9 @@  test_expect_success 'update-ref --no-deref -d can delete symref to broken name'
 '
 
 test_expect_success 'branch -d can delete symref to broken name' '
-	cp .git/refs/heads/main .git/refs/heads/broken...ref &&
-	test_when_finished "rm -f .git/refs/heads/broken...ref" &&
+	test-tool ref-store main update-ref msg "refs/heads/broken...ref" $main_sha1 $ZERO_OID 2048 &&
+
+	test_when_finished "test-tool ref-store main delete-refs 1 msg refs/heads/broken...ref" &&
 	printf "ref: refs/heads/broken...ref\n" >.git/refs/heads/badname &&
 	test_when_finished "rm -f .git/refs/heads/badname" &&
 	git branch -d badname >output 2>error &&
@@ -238,8 +242,9 @@  test_expect_success 'branch -d can delete dangling symref to broken name' '
 '
 
 test_expect_success 'update-ref -d can delete broken name through symref' '
-	cp .git/refs/heads/main .git/refs/heads/broken...ref &&
-	test_when_finished "rm -f .git/refs/heads/broken...ref" &&
+	test-tool ref-store main update-ref msg "refs/heads/broken...ref" $main_sha1 $ZERO_OID 2048 &&
+
+	test_when_finished "test-tool ref-store main delete-refs 1 msg refs/heads/broken...ref" &&
 	printf "ref: refs/heads/broken...ref\n" >.git/refs/heads/badname &&
 	test_when_finished "rm -f .git/refs/heads/badname" &&
 	git update-ref -d refs/heads/badname >output 2>error &&