[PATCH 4/9] Discard "deleted" cache entries after using them to update the working tree

[PATCH 4/9] Discard "deleted" cache entries after using them to update the working tree

[Daniel Barkalow]

Way back in read-tree.c, we used a mode 0 cache entry to indicate that
an entry had been deleted, so that the update code would remove the
working tree file, and we would just skip it when writing out the
index file afterward.

These days, unpack_trees is a library function, and it is still
leaving these entries in the active cache. Furthermore, unpack_trees
doesn't correctly ignore those entries, and who knows what other code
wouldn't expect them to be there, but just isn't yet called after a
call to unpack_trees. To avoid having other code trip over these
entries, have check_updates() remove them after it removes the working
tree files.

While we're at it, make the loop more obvious and skip passing in
globals to a static function with only one caller.

Signed-off-by: Daniel Barkalow <barkalow@iabervon.org>
---
It's likely that this should get merged in some way with Linus's series to 
handle the in-core index differently in general. But this is a simple 
solution to the problem that's sufficient for this particular series.

 unpack-trees.c |   16 +++++++++-------
 1 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/unpack-trees.c b/unpack-trees.c
index d6bae1b..ac919c7 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -288,17 +288,17 @@ static void unlink_entry(char *name, char *last_symlink)
 }
 
 static struct checkout state;
-static void check_updates(struct cache_entry **src, int nr,
-			struct unpack_trees_options *o)
+static void check_updates(struct unpack_trees_options *o)
 {
 	unsigned short mask = htons(CE_UPDATE);
 	unsigned cnt = 0, total = 0;
 	struct progress *progress = NULL;
 	char last_symlink[PATH_MAX];
+	int i;
 
 	if (o->update && o->verbose_update) {
-		for (total = cnt = 0; cnt < nr; cnt++) {
-			struct cache_entry *ce = src[cnt];
+		for (total = cnt = 0; cnt < active_nr; cnt++) {
+			struct cache_entry *ce = active_cache[cnt];
 			if (!ce->ce_mode || ce->ce_flags & mask)
 				total++;
 		}
@@ -309,14 +309,16 @@ static void check_updates(struct cache_entry **src, int nr,
 	}
 
 	*last_symlink = '\0';
-	while (nr--) {
-		struct cache_entry *ce = *src++;
+	for (i = 0; i < active_nr; i++) {
+		struct cache_entry *ce = active_cache[i];
 
 		if (!ce->ce_mode || ce->ce_flags & mask)
 			display_progress(progress, ++cnt);
 		if (!ce->ce_mode) {
 			if (o->update)
 				unlink_entry(ce->name, last_symlink);
+			remove_cache_entry_at(i);
+			i--;
 			continue;
 		}
 		if (ce->ce_flags & mask) {
@@ -375,7 +377,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
 			error("Merge requires file-level merging");
 	}
 
-	check_updates(active_cache, active_nr, o);
+	check_updates(o);
 	return 0;
 }
 
-- 
1.5.4.rc3.4.g16335

Re: [PATCH 4/9] Discard "deleted" cache entries after using them to update the working tree

[Junio C Hamano]

Daniel Barkalow <barkalow@iabervon.org> writes:

> It's likely that this should get merged in some way with Linus's series to 
> handle the in-core index differently in general. But this is a simple 
> solution to the problem that's sufficient for this particular series.

Just to let you know, the early parts of lt/in-core-index series
are already in 'next', and it will be one of the first topics to
graduate to 'master' post 1.5.4.

I think we both understand that this round is sent purely for
discussion and I won't be picking them up right now nor later
before the "for possible inclusion" resend you will be making
post 1.5.4, so it should not matter too much which exact version
this round is based on.

It however may be worthwhile for you to plan rebasing your
future rounds on top of 077c48df8a72b046a2f562fedffa1c3d3a73a4e2
(read-cache.c: fix a couple more CE_REMOVE conversion).

Thanks.

Re: [PATCH 4/9] Discard "deleted" cache entries after using them to update the working tree

[Daniel Barkalow]

On Fri, 25 Jan 2008, Junio C Hamano wrote:

> Daniel Barkalow <barkalow@iabervon.org> writes:
> 
> > It's likely that this should get merged in some way with Linus's series to 
> > handle the in-core index differently in general. But this is a simple 
> > solution to the problem that's sufficient for this particular series.
> 
> Just to let you know, the early parts of lt/in-core-index series
> are already in 'next', and it will be one of the first topics to
> graduate to 'master' post 1.5.4.
> 
> I think we both understand that this round is sent purely for
> discussion and I won't be picking them up right now nor later
> before the "for possible inclusion" resend you will be making
> post 1.5.4, so it should not matter too much which exact version
> this round is based on.
> 
> It however may be worthwhile for you to plan rebasing your
> future rounds on top of 077c48df8a72b046a2f562fedffa1c3d3a73a4e2
> (read-cache.c: fix a couple more CE_REMOVE conversion).

I've now got a version rebased on that. The conflicts are pretty 
straightforward, and the tests all pass (once I fix the unrelated bug in 
9/9 wrt the lockfile API change).

	-Daniel
*This .sig left intentionally blank*

[PATCH 4/9] Discard "deleted" cache entries after using them to update the working tree

[Daniel Barkalow]

Way back in read-tree.c, we used a mode 0 cache entry to indicate that
an entry had been deleted, so that the update code would remove the
working tree file, and we would just skip it when writing out the
index file afterward.

These days, unpack_trees is a library function, and it is still
leaving these entries in the active cache. Furthermore, unpack_trees
doesn't correctly ignore those entries, and who knows what other code
wouldn't expect them to be there, but just isn't yet called after a
call to unpack_trees. To avoid having other code trip over these
entries, have check_updates() remove them after it removes the working
tree files.

While we're at it, make the loop more obvious and skip passing in
globals to a static function with only one caller.

Signed-off-by: Daniel Barkalow <barkalow@iabervon.org>
---
 unpack-trees.c |   16 +++++++++-------
 1 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/unpack-trees.c b/unpack-trees.c
index f462a56..40d4130 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -288,16 +288,16 @@ static void unlink_entry(char *name, char *last_symlink)
 }
 
 static struct checkout state;
-static void check_updates(struct cache_entry **src, int nr,
-			struct unpack_trees_options *o)
+static void check_updates(struct unpack_trees_options *o)
 {
 	unsigned cnt = 0, total = 0;
 	struct progress *progress = NULL;
 	char last_symlink[PATH_MAX];
+	int i;
 
 	if (o->update && o->verbose_update) {
-		for (total = cnt = 0; cnt < nr; cnt++) {
-			struct cache_entry *ce = src[cnt];
+		for (total = cnt = 0; cnt < active_nr; cnt++) {
+			struct cache_entry *ce = active_cache[cnt];
 			if (ce->ce_flags & (CE_UPDATE | CE_REMOVE))
 				total++;
 		}
@@ -308,14 +308,16 @@ static void check_updates(struct cache_entry **src, int nr,
 	}
 
 	*last_symlink = '\0';
-	while (nr--) {
-		struct cache_entry *ce = *src++;
+	for (i = 0; i < active_nr; i++) {
+		struct cache_entry *ce = active_cache[i];
 
 		if (ce->ce_flags & (CE_UPDATE | CE_REMOVE))
 			display_progress(progress, ++cnt);
 		if (ce->ce_flags & CE_REMOVE) {
 			if (o->update)
 				unlink_entry(ce->name, last_symlink);
+			remove_cache_entry_at(i);
+			i--;
 			continue;
 		}
 		if (ce->ce_flags & CE_UPDATE) {
@@ -374,7 +376,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
 			error("Merge requires file-level merging");
 	}
 
-	check_updates(active_cache, active_nr, o);
+	check_updates(o);
 	return 0;
 }
 
-- 
1.5.4

Re: [PATCH 4/9] Discard "deleted" cache entries after using them to update the working tree

[Johannes Schindelin]

Hi,

On Mon, 4 Feb 2008, Daniel Barkalow wrote:

> Way back in read-tree.c, we used a mode 0 cache entry to indicate that 
> an entry had been deleted, so that the update code would remove the 
> working tree file, and we would just skip it when writing out the index 
> file afterward.
> 
> These days, unpack_trees is a library function, and it is still leaving 
> these entries in the active cache. Furthermore, unpack_trees doesn't 
> correctly ignore those entries, and who knows what other code wouldn't 
> expect them to be there, but just isn't yet called after a call to 
> unpack_trees. To avoid having other code trip over these entries, have 
> check_updates() remove them after it removes the working tree files.
> 
> While we're at it, make the loop more obvious and skip passing in 
> globals to a static function with only one caller.

After reading the code I understand what you mean.  How about

	While at it, make the loop removing those entries more obvious, 
	and avoid passing global variables as parameters to 
	check_updates(): there is only one call site anyway.

Ciao,
Dscho

Re: [PATCH 4/9] Discard "deleted" cache entries after using them to update the working tree

[Daniel Barkalow]

On Tue, 5 Feb 2008, Johannes Schindelin wrote:

> Hi,
> 
> On Mon, 4 Feb 2008, Daniel Barkalow wrote:
> 
> > Way back in read-tree.c, we used a mode 0 cache entry to indicate that 
> > an entry had been deleted, so that the update code would remove the 
> > working tree file, and we would just skip it when writing out the index 
> > file afterward.
> > 
> > These days, unpack_trees is a library function, and it is still leaving 
> > these entries in the active cache. Furthermore, unpack_trees doesn't 
> > correctly ignore those entries, and who knows what other code wouldn't 
> > expect them to be there, but just isn't yet called after a call to 
> > unpack_trees. To avoid having other code trip over these entries, have 
> > check_updates() remove them after it removes the working tree files.
> > 
> > While we're at it, make the loop more obvious and skip passing in 
> > globals to a static function with only one caller.
> 
> After reading the code I understand what you mean.  How about
> 
> 	While at it, make the loop removing those entries more obvious, 
> 	and avoid passing global variables as parameters to 
> 	check_updates(): there is only one call site anyway.

It's the loop's flow control that I'm making more obvious (using a normal 
for loop instead of a while loop that counts down the number of entries 
while counting up the pointer to the entry under consideration). I'll 
clarify the message for the next version.

	-Daniel
*This .sig left intentionally blank*