Re: [PATCH v3 1/2] cache-tree: invalidate i-t-a paths after generating trees

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Nguyễn Thái Ngọc Duy  <pclouds@xxxxxxxxx> writes:

> Intent-to-add entries used to forbid writing trees so it was not a
> problem. After commit 3f6d56d (commit: ignore intent-to-add entries
> instead of refusing - 2012-02-07), we can generate trees from an index
> with i-t-a entries.
>
> However, the commit forgets to invalidate all paths leading to i-t-a
> entries. With fully valid cache-tree (e.g. after commit or
> write-tree), diff operations may prefer cache-tree to index and not
> see i-t-a entries in the index, because cache-tree does not have them.
>
> Reported-by: Jonathon Mah <me@xxxxxxxxxxxxxxx>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx>
> ---
>  This version ensures that entry_count can only be >= -1 after
>  update_one returns. Not ideal but good enough.
>
>  cache-tree.c          | 40 ++++++++++++++++++++++++++++++++++++----
>  t/t2203-add-intent.sh | 20 ++++++++++++++++++++
>  2 files changed, 56 insertions(+), 4 deletions(-)
>
> diff --git a/cache-tree.c b/cache-tree.c
> index 28ed657..1fbc81a 100644
> --- a/cache-tree.c
> +++ b/cache-tree.c
> @@ -248,6 +248,7 @@ static int update_one(struct cache_tree *it,
>  	int missing_ok = flags & WRITE_TREE_MISSING_OK;
>  	int dryrun = flags & WRITE_TREE_DRY_RUN;
>  	int i;
> +	int to_invalidate = 0;
>  
>  	if (0 <= it->entry_count && has_sha1_file(it->sha1))
>  		return it->entry_count;
> @@ -324,7 +325,14 @@ static int update_one(struct cache_tree *it,
>  			if (!sub)
>  				die("cache-tree.c: '%.*s' in '%s' not found",
>  				    entlen, path + baselen, path);
> -			i += sub->cache_tree->entry_count - 1;
> +			i--; /* this entry is already counted in "sub" */

Huh?

The "-1" in the original is the bog-standard compensation for the
for(;;i++) loop.

> +			if (sub->cache_tree->entry_count < 0) {
> +				i -= sub->cache_tree->entry_count;
> +				sub->cache_tree->entry_count = -1;
> +				to_invalidate = 1;
> +			}
> +			else
> +				i += sub->cache_tree->entry_count;

While the rewritten version is not *wrong* per-se, I have a feeling
that it may be much easier to read if written like this:

	if (sub->cache_tree_entry_count < 0) {
		to_invalidate = 1;
                to_skip = 0 - sub->cache_tree_entry_count;
		sub->cache_tree_entry_count = -1;
	} else {
		to_skip = sub->cache_tree_entry_count;
	}
        i += to_skip - 1;

> @@ -360,7 +383,7 @@ static int update_one(struct cache_tree *it,
>  	}
>  
>  	strbuf_release(&buffer);
> -	it->entry_count = i;
> +	it->entry_count = to_invalidate ? -i : i;
>  #if DEBUG
>  	fprintf(stderr, "cache-tree update-one (%d ent, %d subtree) %s\n",
>  		it->entry_count, it->subtree_nr,
> @@ -381,6 +404,15 @@ int cache_tree_update(struct cache_tree *it,
>  	i = update_one(it, cache, entries, "", 0, flags);
>  	if (i < 0)
>  		return i;
> +	/*
> +	 * update_one() uses negative entry_count as a way to mark an
> +	 * entry invalid _and_ pass the number of entries back to
> +	 * itself at the parent level. This is for internal use and
> +	 * should not be leaked out after the top-level update_one
> +	 * exits.
> +	 */
> +	if (it->entry_count < 0)
> +		it->entry_count = -1;

Nice.  I think what entry_count means immediately after update_one()
returned should be commented near the beginning of that function,
too, though.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]