git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Jonathan Nieder <jrnieder@gmail.com>
To: Jeff King <peff@peff.net>
Cc: git@vger.kernel.org
Subject: Re: [PATCH 1/7] fsck_tree(): fix shadowed variable
Date: Mon, 5 Oct 2020 00:44:04 -0700	[thread overview]
Message-ID: <20201005074404.GD1166820@google.com> (raw)
In-Reply-To: <20201005071905.GA2291074@coredump.intra.peff.net>

Jeff King wrote:

> Commit b2f2039c2b (fsck: accept an oid instead of a "struct tree" for
> fsck_tree(), 2019-10-18) introduced a new "oid" parameter to
> fsck_tree(), and we pass it to the report() function when we find
> problems. However, that is shadowed within the tree-walking loop by the
> existing "oid" variable which we use to store the oid of each tree
> entry. As a result, we may report the wrong oid for some problems we
> detect within the loop (the entry oid, instead of the tree oid).
>
> Our tests didn't catch this because they checked only that we found the
> expected fsck problem, not that it was attached to the correct object.

Oh, goodness.  Does this mean we should be similarly checking oid in
the rest of the fsck test scripts?  (I'm not saying this patch should
do so, just curious about what you think on the subject.)

> Let's rename both variables in the function to avoid confusion. This
> makes the diff a little noisy (e.g., all of the report() calls outside
> the loop wee already correct but need touched), but makes sure we catch

nit: s/wee/are/, s/need touched/need to be touched/

> all cases and will avoid similar confusion in the future.

Thanks for doing that --- it does make reviewing easier.

> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  fsck.c                     | 40 +++++++++++++++++++-------------------
>  t/t7415-submodule-names.sh |  5 +++--
>  2 files changed, 23 insertions(+), 22 deletions(-)
> 
> diff --git a/fsck.c b/fsck.c
> index f82e2fe9e3..46a108839f 100644
> --- a/fsck.c
> +++ b/fsck.c
> @@ -633,7 +633,7 @@ static int verify_ordered(unsigned mode1, const char *name1,
>  	return c1 < c2 ? 0 : TREE_UNORDERED;
>  }
>  
> -static int fsck_tree(const struct object_id *oid,
> +static int fsck_tree(const struct object_id *tree_oid,

optional: we could call it "tree".

>  		     const char *buffer, unsigned long size,
>  		     struct fsck_options *options)
>  {
> @@ -654,7 +654,7 @@ static int fsck_tree(const struct object_id *oid,
>  	struct name_stack df_dup_candidates = { NULL };
>  
>  	if (init_tree_desc_gently(&desc, buffer, size)) {
> -		retval += report(options, oid, OBJ_TREE, FSCK_MSG_BAD_TREE, "cannot be parsed as a tree");
> +		retval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_BAD_TREE, "cannot be parsed as a tree");
>  		return retval;
>  	}
>  
> @@ -664,11 +664,11 @@ static int fsck_tree(const struct object_id *oid,
>  	while (desc.size) {
>  		unsigned short mode;
>  		const char *name, *backslash;
> -		const struct object_id *oid;
> +		const struct object_id *entry_oid;
>  
> -		oid = tree_entry_extract(&desc, &name, &mode);
> +		entry_oid = tree_entry_extract(&desc, &name, &mode);

optional: could call it "child".

>  
> -		has_null_sha1 |= is_null_oid(oid);
> +		has_null_sha1 |= is_null_oid(entry_oid);
>  		has_full_path |= !!strchr(name, '/');
>  		has_empty_name |= !*name;
>  		has_dot |= !strcmp(name, ".");
> @@ -678,10 +678,10 @@ static int fsck_tree(const struct object_id *oid,
>  
>  		if (is_hfs_dotgitmodules(name) || is_ntfs_dotgitmodules(name)) {
>  			if (!S_ISLNK(mode))
> -				oidset_insert(&gitmodules_found, oid);
> +				oidset_insert(&gitmodules_found, entry_oid);
>  			else
>  				retval += report(options,
> -						 oid, OBJ_TREE,
> +						 tree_oid, OBJ_TREE,
>  						 FSCK_MSG_GITMODULES_SYMLINK,
>  						 ".gitmodules is a symbolic link");

Right, this is a property of the enclosing tree, not the blob
representing the symlink target.  Good.

not about this patch: Could report() notice when the oid doesn't match
the type passed in, to more easily catch this kind of mistake?

[...]
> @@ -692,9 +692,9 @@ static int fsck_tree(const struct object_id *oid,
>  				has_dotgit |= is_ntfs_dotgit(backslash);
>  				if (is_ntfs_dotgitmodules(backslash)) {
>  					if (!S_ISLNK(mode))
> -						oidset_insert(&gitmodules_found, oid);
> +						oidset_insert(&gitmodules_found, entry_oid);
>  					else
> -						retval += report(options, oid, OBJ_TREE,
> +						retval += report(options, tree_oid, OBJ_TREE,
>  								 FSCK_MSG_GITMODULES_SYMLINK,
>  								 ".gitmodules is a symbolic link");

Likewise, good.

[...]
> --- a/t/t7415-submodule-names.sh
> +++ b/t/t7415-submodule-names.sh
> @@ -148,13 +148,14 @@ test_expect_success 'fsck detects symlinked .gitmodules file' '
>  		{
>  			printf "100644 blob $content\t$tricky\n" &&
>  			printf "120000 blob $target\t.gitmodules\n"
> -		} | git mktree &&
> +		} >bad-tree &&
> +		tree=$(git mktree <bad-tree) &&
>  
>  		# Check not only that we fail, but that it is due to the
>  		# symlink detector; this grep string comes from the config
>  		# variable name and will not be translated.
>  		test_must_fail git fsck 2>output &&
> -		test_i18ngrep gitmodulesSymlink output
> +		test_i18ngrep "tree $tree: gitmodulesSymlink" output

Makes sense.

By the way, why does GETTEXT_POISON lose the gitmodulesSymlink
keyword?  Is this just a limitation of GETTEXT_POISON losing
information that's passed in with %s?

With the commit message tweak mentioned above,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.

  reply	other threads:[~2020-10-05  7:44 UTC|newest]

Thread overview: 55+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-05  7:17 [PATCH 0/7] forbidding symlinked .gitattributes and .gitignore Jeff King
2020-10-05  7:19 ` [PATCH 1/7] fsck_tree(): fix shadowed variable Jeff King
2020-10-05  7:44   ` Jonathan Nieder [this message]
2020-10-05  8:20     ` Jeff King
2020-10-05  8:29       ` Jonathan Nieder
2020-10-05  7:19 ` [PATCH 2/7] fsck_tree(): wrap some long lines Jeff King
2020-10-05  7:46   ` Jonathan Nieder
2020-10-05  7:19 ` [PATCH 3/7] t7415: rename to expand scope Jeff King
2020-10-05  7:50   ` Jonathan Nieder
2020-10-05  8:24     ` Jeff King
2020-10-05  8:34       ` Jonathan Nieder
2020-10-05  8:49         ` Jeff King
2020-10-05  7:20 ` [PATCH 4/7] t7450: test verify_path() handling of gitmodules Jeff King
2020-10-05  7:53   ` Jonathan Nieder
2020-10-05  8:30     ` Jeff King
2020-10-05  8:38       ` Jonathan Nieder
2020-10-05  7:21 ` [PATCH 5/7] t0060: test obscured .gitattributes and .gitignore matching Jeff King
2020-10-05  8:03   ` Jonathan Nieder
2020-10-05  8:40     ` Jeff King
2020-10-05 21:20       ` Johannes Schindelin
2020-10-06 14:01         ` Jeff King
2020-10-05  7:24 ` [PATCH 6/7] verify_path(): disallow symlinks in .gitattributes and .gitignore Jeff King
2020-10-05  8:09   ` Jonathan Nieder
2020-10-05 12:07     ` Jeff King
2020-10-05  7:25 ` [PATCH 7/7] fsck: complain when .gitattributes or .gitignore is a symlink Jeff King
2020-10-05  8:12   ` Jonathan Nieder
2020-10-05  8:53     ` Jeff King
2020-10-05  7:32 ` [PATCH 0/7] forbidding symlinked .gitattributes and .gitignore Jonathan Nieder
2020-10-05  8:58   ` Jeff King
2020-10-05 12:16 ` [PATCH v2 0/8] " Jeff King
2020-10-05 12:16   ` [PATCH v2 1/8] fsck_tree(): fix shadowed variable Jeff King
2020-10-05 12:16   ` [PATCH v2 2/8] fsck_tree(): wrap some long lines Jeff King
2020-10-05 12:16   ` [PATCH v2 3/8] t7415: rename to expand scope Jeff King
2020-10-05 12:16   ` [PATCH v2 4/8] t7450: test verify_path() handling of gitmodules Jeff King
2020-10-05 12:16   ` [PATCH v2 5/8] t7450: test .gitmodules symlink matching against obscured names Jeff King
2020-10-05 12:16   ` [PATCH v2 6/8] t0060: test obscured .gitattributes and .gitignore matching Jeff King
2020-10-05 12:16   ` [PATCH v2 7/8] verify_path(): disallow symlinks in .gitattributes and .gitignore Jeff King
2020-10-27  3:35     ` Jonathan Nieder
2020-10-27  7:58       ` Jeff King
2020-10-27 22:00         ` Junio C Hamano
2020-10-28  9:41           ` Jeff King
2020-10-27 23:43         ` Jonathan Nieder
2020-10-28 19:18           ` Junio C Hamano
2020-10-05 12:16   ` [PATCH v2 8/8] fsck: complain when .gitattributes or .gitignore is a symlink Jeff King
2020-10-06 20:41   ` [PATCH v2 0/8] forbidding symlinked .gitattributes and .gitignore Junio C Hamano
2020-10-20 23:19   ` Philip Oakley
2020-10-23  8:17     ` [PATCH] documentation symlink restrictions for .git* files Jeff King
2020-10-23  8:27       ` Jeff King
2020-10-26 22:18       ` Philip Oakley
2020-10-26 22:53         ` Jeff King
2020-10-26 23:32           ` Junio C Hamano
2020-10-27  7:26             ` Jeff King
2020-10-27 18:45               ` Junio C Hamano
2020-10-27 21:00                 ` Philip Oakley
2020-10-28 19:14                   ` Junio C Hamano

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=20201005074404.GD1166820@google.com \
    --to=jrnieder@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=peff@peff.net \
    /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 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).