* git-checkout regression?
@ 2008-02-28 14:11 Alexandre Julliard
2008-02-28 15:11 ` Daniel Barkalow
2008-02-28 15:58 ` [PATCH] Write index file on any checkout of files Daniel Barkalow
0 siblings, 2 replies; 8+ messages in thread
From: Alexandre Julliard @ 2008-02-28 14:11 UTC (permalink / raw)
To: git; +Cc: Daniel Barkalow
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: git-checkout regression?
2008-02-28 14:11 git-checkout regression? Alexandre Julliard
@ 2008-02-28 15:11 ` Daniel Barkalow
2008-02-28 15:58 ` [PATCH] Write index file on any checkout of files Daniel Barkalow
1 sibling, 0 replies; 8+ messages in thread
From: Daniel Barkalow @ 2008-02-28 15:11 UTC (permalink / raw)
To: Alexandre Julliard; +Cc: git
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*
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] Write index file on any checkout of files
2008-02-28 14:11 git-checkout regression? Alexandre Julliard
2008-02-28 15:11 ` Daniel Barkalow
@ 2008-02-28 15:58 ` Daniel Barkalow
2008-02-28 16:07 ` Alexandre Julliard
2008-02-28 20:25 ` Junio C Hamano
1 sibling, 2 replies; 8+ messages in thread
From: Daniel Barkalow @ 2008-02-28 15:58 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Alexandre Julliard
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] Write index file on any checkout of files
2008-02-28 15:58 ` [PATCH] Write index file on any checkout of files Daniel Barkalow
@ 2008-02-28 16:07 ` Alexandre Julliard
2008-02-28 16:11 ` Daniel Barkalow
2008-02-28 20:25 ` Junio C Hamano
1 sibling, 1 reply; 8+ messages in thread
From: Alexandre Julliard @ 2008-02-28 16:07 UTC (permalink / raw)
To: Daniel Barkalow; +Cc: Junio C Hamano, git
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Write index file on any checkout of files
2008-02-28 16:07 ` Alexandre Julliard
@ 2008-02-28 16:11 ` Daniel Barkalow
0 siblings, 0 replies; 8+ messages in thread
From: Daniel Barkalow @ 2008-02-28 16:11 UTC (permalink / raw)
To: Alexandre Julliard; +Cc: Junio C Hamano, git
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*
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Write index file on any checkout of files
2008-02-28 15:58 ` [PATCH] Write index file on any checkout of files Daniel Barkalow
2008-02-28 16:07 ` Alexandre Julliard
@ 2008-02-28 20:25 ` Junio C Hamano
2008-02-28 21:21 ` Daniel Barkalow
1 sibling, 1 reply; 8+ messages in thread
From: Junio C Hamano @ 2008-02-28 20:25 UTC (permalink / raw)
To: Daniel Barkalow; +Cc: git, 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.
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-<.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Write index file on any checkout of files
2008-02-28 20:25 ` Junio C Hamano
@ 2008-02-28 21:21 ` Daniel Barkalow
2008-02-28 21:44 ` Junio C Hamano
0 siblings, 1 reply; 8+ messages in thread
From: Daniel Barkalow @ 2008-02-28 21:21 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Alexandre Julliard
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*
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Write index file on any checkout of files
2008-02-28 21:21 ` Daniel Barkalow
@ 2008-02-28 21:44 ` Junio C Hamano
0 siblings, 0 replies; 8+ messages in thread
From: Junio C Hamano @ 2008-02-28 21:44 UTC (permalink / raw)
To: Daniel Barkalow; +Cc: git, Alexandre Julliard
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2008-02-28 21:45 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-02-28 14:11 git-checkout regression? Alexandre Julliard
2008-02-28 15:11 ` Daniel Barkalow
2008-02-28 15:58 ` [PATCH] Write index file on any checkout of files Daniel Barkalow
2008-02-28 16:07 ` Alexandre Julliard
2008-02-28 16:11 ` Daniel Barkalow
2008-02-28 20:25 ` Junio C Hamano
2008-02-28 21:21 ` Daniel Barkalow
2008-02-28 21:44 ` Junio C Hamano
Code repositories for project(s) associated with this public 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).