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
Daniel BarkalowRe: git-checkout regression?
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*
Daniel Barkalow[PATCH] Write index file on any checkout of files
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
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
Daniel BarkalowRe: [PATCH] Write index file on any checkout of files
On Thu, 28 Feb 2008, Alexandre Julliard wrote:
Show 8 quoted lines
> 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*
Junio C HamanoRe: [PATCH] Write index file on any checkout of files
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.
Show 19 quoted lines
> -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-<.
Daniel BarkalowRe: [PATCH] Write index file on any checkout of files
On Thu, 28 Feb 2008, Junio C Hamano wrote:
Show 33 quoted lines
> 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?
Show 6 quoted lines
> 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*
Junio C HamanoRe: [PATCH] Write index file on any checkout of files
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.