git-checkout regression?

git-checkout regression?

[Alexandre Julliard]

Since the rewrite in C, "git checkout HEAD file" no longer updates the
index entry for file, it needs an extra git-update-index --refresh. Is
that a deliberate change?

-- 
Alexandre Julliard
julliard@winehq.org

Re: git-checkout regression?

[Daniel Barkalow]

On Thu, 28 Feb 2008, Alexandre Julliard wrote:

> Since the rewrite in C, "git checkout HEAD file" no longer updates the
> index entry for file, it needs an extra git-update-index --refresh. Is
> that a deliberate change?

Nope, looks like I forgot to rewrite the index file with the new stat info 
from writing the files. I'll have a patch in a little while...

	-Daniel
*This .sig left intentionally blank*

[PATCH] Write index file on any checkout of files

[Daniel Barkalow]

We need to rewrite the index file when we check out files, even if we
haven't modified the blob info by reading from another tree, so that
we get the stat cache to include the fact that we just modified the
file so it doesn't need to be refreshed.

Signed-off-by: Daniel Barkalow <barkalow@iabervon.org>
---
 builtin-checkout.c |   24 +++++++++++-------------
 1 files changed, 11 insertions(+), 13 deletions(-)

diff --git a/builtin-checkout.c b/builtin-checkout.c
index 4a4bb8b..7c620e6 100644
--- a/builtin-checkout.c
+++ b/builtin-checkout.c
@@ -67,17 +67,8 @@ static int update_some(const unsigned char *sha1, const char *base, int baselen,
 
 static int read_tree_some(struct tree *tree, const char **pathspec)
 {
-	int newfd;
-	struct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));
-	newfd = hold_locked_index(lock_file, 1);
-	read_cache();
-
 	read_tree_recursive(tree, "", 0, 0, pathspec, update_some);
 
-	if (write_cache(newfd, active_cache, active_nr) ||
-	    commit_locked_index(lock_file))
-		die("unable to write new index file");
-
 	/* update the index with the given tree's info
 	 * for all args, expanding wildcards, and exit
 	 * with any non-zero return code.
@@ -85,7 +76,7 @@ static int read_tree_some(struct tree *tree, const char **pathspec)
 	return 0;
 }
 
-static int checkout_paths(const char **pathspec)
+static int checkout_paths(const char **pathspec, int newfd, struct lock_file *lock_file)
 {
 	int pos;
 	struct checkout state;
@@ -116,6 +107,10 @@ static int checkout_paths(const char **pathspec)
 		}
 	}
 
+	if (write_cache(newfd, active_cache, active_nr) ||
+	    commit_locked_index(lock_file))
+		die("unable to write new index file");
+
 	resolve_ref("HEAD", rev, 0, &flag);
 	head = lookup_commit_reference_gently(rev, 1);
 
@@ -554,11 +549,14 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 			}
 		}
 
+		int newfd;
+		struct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));
+		newfd = hold_locked_index(lock_file, 1);
+		read_cache();
+
 		if (source_tree)
 			read_tree_some(source_tree, pathspec);
-		else
-			read_cache();
-		return checkout_paths(pathspec);
+		return checkout_paths(pathspec, newfd, lock_file);
 	}
 
 	if (new.name && !new.commit) {
-- 
1.5.4.3.328.gcaed

Re: [PATCH] Write index file on any checkout of files

[Alexandre Julliard]

Daniel Barkalow <barkalow@iabervon.org> writes:

> We need to rewrite the index file when we check out files, even if we
> haven't modified the blob info by reading from another tree, so that
> we get the stat cache to include the fact that we just modified the
> file so it doesn't need to be refreshed.

That fixes the problem for me. Thanks!

-- 
Alexandre Julliard
julliard@winehq.org

Re: [PATCH] Write index file on any checkout of files

[Daniel Barkalow]

On Thu, 28 Feb 2008, Alexandre Julliard wrote:

> Daniel Barkalow <barkalow@iabervon.org> writes:
> 
> > We need to rewrite the index file when we check out files, even if we
> > haven't modified the blob info by reading from another tree, so that
> > we get the stat cache to include the fact that we just modified the
> > file so it doesn't need to be refreshed.
> 
> That fixes the problem for me. Thanks!

You're welcome, and good catch.

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH] Write index file on any checkout of files

[Junio C Hamano]

Daniel Barkalow <barkalow@iabervon.org> writes:

> We need to rewrite the index file when we check out files, even if we
> haven't modified the blob info by reading from another tree, so that
> we get the stat cache to include the fact that we just modified the
> file so it doesn't need to be refreshed.

Thanks, Alexandre, for spotting.

> -static int checkout_paths(const char **pathspec)
> +static int checkout_paths(const char **pathspec, int newfd, struct lock_file *lock_file)
>  {
> ...
> @@ -554,11 +549,14 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
>  			}
>  		}
>  
> +		int newfd;
> +		struct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));
> +		newfd = hold_locked_index(lock_file, 1);
> +		read_cache();
> +
>  		if (source_tree)
>  			read_tree_some(source_tree, pathspec);
> -		else
> -			read_cache();
> -		return checkout_paths(pathspec);
> +		return checkout_paths(pathspec, newfd, lock_file);

Aside from decl-after-statement, I suspect that at this point
these all should go to checkout_paths() function itself, that
takes pathspec and source_tree (which could be NULL), but that
is only "logical code organization" issue.  Thanks for fixing.

I however would have liked if this were caught while the topic
was still cooking in 'next'.

There was a process failure somewhere, which makes me worry more
than just this single bug that escaped to 'master'.  Alex
Riesen's segv fix in another thread makes it double X-<.

Re: [PATCH] Write index file on any checkout of files

[Daniel Barkalow]

On Thu, 28 Feb 2008, Junio C Hamano wrote:

> Daniel Barkalow <barkalow@iabervon.org> writes:
> 
> > We need to rewrite the index file when we check out files, even if we
> > haven't modified the blob info by reading from another tree, so that
> > we get the stat cache to include the fact that we just modified the
> > file so it doesn't need to be refreshed.
> 
> Thanks, Alexandre, for spotting.
> 
> > -static int checkout_paths(const char **pathspec)
> > +static int checkout_paths(const char **pathspec, int newfd, struct lock_file *lock_file)
> >  {
> > ...
> > @@ -554,11 +549,14 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
> >  			}
> >  		}
> >  
> > +		int newfd;
> > +		struct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));
> > +		newfd = hold_locked_index(lock_file, 1);
> > +		read_cache();
> > +
> >  		if (source_tree)
> >  			read_tree_some(source_tree, pathspec);
> > -		else
> > -			read_cache();
> > -		return checkout_paths(pathspec);
> > +		return checkout_paths(pathspec, newfd, lock_file);
> 
> Aside from decl-after-statement, I suspect that at this point
> these all should go to checkout_paths() function itself, that
> takes pathspec and source_tree (which could be NULL), but that
> is only "logical code organization" issue.  Thanks for fixing.

Yeah, that's a better organization, and makes the code nice. Want it as a 
replacement patch or as an additional patch on top of this one?

> I however would have liked if this were caught while the topic
> was still cooking in 'next'.
> 
> There was a process failure somewhere, which makes me worry more
> than just this single bug that escaped to 'master'.  Alex
> Riesen's segv fix in another thread makes it double X-<.

This one is a stat cache problem, and those tend not to get noticed for a 
while in normal use, simply because everything works fine, but makes 
subsequent operations a bit slower.

Alex's is a case where an operation that should fail fails ungracefully, 
which is again unlikely to be uncovered by normal use, because people just 
don't do that.

I think that cooking in next isn't likely to matter much to either of 
these classes of problems, and we need to catch them in regression 
testing. They're also not the sort of thing that we're likely to test for 
specifically, so it would be nice to have something in test-lib to do it 
in general. It would be nice to have automatic checks after every test 
that nothing segfaulted, and a helper (which could be sprinkled liberally 
through the test suite) to make sure that nothing has stale stat cache 
info (without being actually changed).

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH] Write index file on any checkout of files

[Junio C Hamano]

Daniel Barkalow <barkalow@iabervon.org> writes:

> Yeah, that's a better organization, and makes the code nice. Want it as a 
> replacement patch or as an additional patch on top of this one?

Thanks.  Replacement would be nice, as I do look at patches
during my lunchtime but not apply them until evening, on my
day-job days.