{"thread":{"id":"12363","subject":"git-checkout regression?","startedAt":"2008-02-28T14:11:24Z","lastAt":"2008-02-28T21:44:50Z","messageCount":8,"participants":["Alexandre Julliard","Daniel Barkalow","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"70257","messageId":"87wsop188j.fsf@wine.dyndns.org","threadId":"12363","inReplyTo":null,"subject":"git-checkout regression?","fromName":"Alexandre Julliard","fromEmail":"julliard@winehq.org","sentAt":"2008-02-28T14:11:24Z","receivedAt":"2008-02-28T14:11:24Z","isPatch":false,"sender":{"key":"julliard@winehq.org","avatar":null},"body":"Since the rewrite in C, \"git checkout HEAD file\" no longer updates the\nindex entry for file, it needs an extra git-update-index --refresh. Is\nthat a deliberate change?\n\n-- \nAlexandre Julliard\njulliard@winehq.org\n"},{"id":"70261","messageId":"alpine.LNX.1.00.0802281010070.19665@iabervon.org","threadId":"12363","inReplyTo":"87wsop188j.fsf@wine.dyndns.org","subject":"Re: git-checkout regression?","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2008-02-28T15:11:10Z","receivedAt":"2008-02-28T15:11:10Z","isPatch":false,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Thu, 28 Feb 2008, Alexandre Julliard wrote:\n\n> Since the rewrite in C, \"git checkout HEAD file\" no longer updates the\n> index entry for file, it needs an extra git-update-index --refresh. Is\n> that a deliberate change?\n\nNope, looks like I forgot to rewrite the index file with the new stat info \nfrom writing the files. I'll have a patch in a little while...\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"70274","messageId":"alpine.LNX.1.00.0802281058190.19665@iabervon.org","threadId":"12363","inReplyTo":"87wsop188j.fsf@wine.dyndns.org","subject":"[PATCH] Write index file on any checkout of files","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2008-02-28T15:58:59Z","receivedAt":"2008-02-28T15:58:59Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"We need to rewrite the index file when we check out files, even if we\nhaven't modified the blob info by reading from another tree, so that\nwe get the stat cache to include the fact that we just modified the\nfile so it doesn't need to be refreshed.\n\nSigned-off-by: Daniel Barkalow <barkalow@iabervon.org>\n---\n builtin-checkout.c |   24 +++++++++++-------------\n 1 files changed, 11 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin-checkout.c b/builtin-checkout.c\nindex 4a4bb8b..7c620e6 100644\n--- a/builtin-checkout.c\n+++ b/builtin-checkout.c\n@@ -67,17 +67,8 @@ static int update_some(const unsigned char *sha1, const char *base, int baselen,\n \n static int read_tree_some(struct tree *tree, const char **pathspec)\n {\n-\tint newfd;\n-\tstruct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));\n-\tnewfd = hold_locked_index(lock_file, 1);\n-\tread_cache();\n-\n \tread_tree_recursive(tree, \"\", 0, 0, pathspec, update_some);\n \n-\tif (write_cache(newfd, active_cache, active_nr) ||\n-\t    commit_locked_index(lock_file))\n-\t\tdie(\"unable to write new index file\");\n-\n \t/* update the index with the given tree's info\n \t * for all args, expanding wildcards, and exit\n \t * with any non-zero return code.\n@@ -85,7 +76,7 @@ static int read_tree_some(struct tree *tree, const char **pathspec)\n \treturn 0;\n }\n \n-static int checkout_paths(const char **pathspec)\n+static int checkout_paths(const char **pathspec, int newfd, struct lock_file *lock_file)\n {\n \tint pos;\n \tstruct checkout state;\n@@ -116,6 +107,10 @@ static int checkout_paths(const char **pathspec)\n \t\t}\n \t}\n \n+\tif (write_cache(newfd, active_cache, active_nr) ||\n+\t    commit_locked_index(lock_file))\n+\t\tdie(\"unable to write new index file\");\n+\n \tresolve_ref(\"HEAD\", rev, 0, &flag);\n \thead = lookup_commit_reference_gently(rev, 1);\n \n@@ -554,11 +549,14 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n \t\t\t}\n \t\t}\n \n+\t\tint newfd;\n+\t\tstruct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));\n+\t\tnewfd = hold_locked_index(lock_file, 1);\n+\t\tread_cache();\n+\n \t\tif (source_tree)\n \t\t\tread_tree_some(source_tree, pathspec);\n-\t\telse\n-\t\t\tread_cache();\n-\t\treturn checkout_paths(pathspec);\n+\t\treturn checkout_paths(pathspec, newfd, lock_file);\n \t}\n \n \tif (new.name && !new.commit) {\n-- \n1.5.4.3.328.gcaed\n"},{"id":"70275","messageId":"87hcft12ug.fsf@wine.dyndns.org","threadId":"12363","inReplyTo":"alpine.LNX.1.00.0802281058190.19665@iabervon.org","subject":"Re: [PATCH] Write index file on any checkout of files","fromName":"Alexandre Julliard","fromEmail":"julliard@winehq.org","sentAt":"2008-02-28T16:07:51Z","receivedAt":"2008-02-28T16:07:51Z","isPatch":true,"sender":{"key":"julliard@winehq.org","avatar":null},"body":"Daniel Barkalow <barkalow@iabervon.org> writes:\n\n> We need to rewrite the index file when we check out files, even if we\n> haven't modified the blob info by reading from another tree, so that\n> we get the stat cache to include the fact that we just modified the\n> file so it doesn't need to be refreshed.\n\nThat fixes the problem for me. Thanks!\n\n-- \nAlexandre Julliard\njulliard@winehq.org\n"},{"id":"70277","messageId":"alpine.LNX.1.00.0802281111050.19665@iabervon.org","threadId":"12363","inReplyTo":"87hcft12ug.fsf@wine.dyndns.org","subject":"Re: [PATCH] Write index file on any checkout of files","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2008-02-28T16:11:26Z","receivedAt":"2008-02-28T16:11:26Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Thu, 28 Feb 2008, Alexandre Julliard wrote:\n\n> Daniel Barkalow <barkalow@iabervon.org> writes:\n> \n> > We need to rewrite the index file when we check out files, even if we\n> > haven't modified the blob info by reading from another tree, so that\n> > we get the stat cache to include the fact that we just modified the\n> > file so it doesn't need to be refreshed.\n> \n> That fixes the problem for me. Thanks!\n\nYou're welcome, and good catch.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"70298","messageId":"7vfxvcj0b7.fsf@gitster.siamese.dyndns.org","threadId":"12363","inReplyTo":"alpine.LNX.1.00.0802281058190.19665@iabervon.org","subject":"Re: [PATCH] Write index file on any checkout of files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-02-28T20:25:16Z","receivedAt":"2008-02-28T20:25:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Daniel Barkalow <barkalow@iabervon.org> writes:\n\n> We need to rewrite the index file when we check out files, even if we\n> haven't modified the blob info by reading from another tree, so that\n> we get the stat cache to include the fact that we just modified the\n> file so it doesn't need to be refreshed.\n\nThanks, Alexandre, for spotting.\n\n> -static int checkout_paths(const char **pathspec)\n> +static int checkout_paths(const char **pathspec, int newfd, struct lock_file *lock_file)\n>  {\n> ...\n> @@ -554,11 +549,14 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n>  \t\t\t}\n>  \t\t}\n>  \n> +\t\tint newfd;\n> +\t\tstruct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));\n> +\t\tnewfd = hold_locked_index(lock_file, 1);\n> +\t\tread_cache();\n> +\n>  \t\tif (source_tree)\n>  \t\t\tread_tree_some(source_tree, pathspec);\n> -\t\telse\n> -\t\t\tread_cache();\n> -\t\treturn checkout_paths(pathspec);\n> +\t\treturn checkout_paths(pathspec, newfd, lock_file);\n\nAside from decl-after-statement, I suspect that at this point\nthese all should go to checkout_paths() function itself, that\ntakes pathspec and source_tree (which could be NULL), but that\nis only \"logical code organization\" issue.  Thanks for fixing.\n\nI however would have liked if this were caught while the topic\nwas still cooking in 'next'.\n\nThere was a process failure somewhere, which makes me worry more\nthan just this single bug that escaped to 'master'.  Alex\nRiesen's segv fix in another thread makes it double X-<.\n"},{"id":"70316","messageId":"alpine.LNX.1.00.0802281547320.19665@iabervon.org","threadId":"12363","inReplyTo":"7vfxvcj0b7.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Write index file on any checkout of files","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2008-02-28T21:21:33Z","receivedAt":"2008-02-28T21:21:33Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Thu, 28 Feb 2008, Junio C Hamano wrote:\n\n> Daniel Barkalow <barkalow@iabervon.org> writes:\n> \n> > We need to rewrite the index file when we check out files, even if we\n> > haven't modified the blob info by reading from another tree, so that\n> > we get the stat cache to include the fact that we just modified the\n> > file so it doesn't need to be refreshed.\n> \n> Thanks, Alexandre, for spotting.\n> \n> > -static int checkout_paths(const char **pathspec)\n> > +static int checkout_paths(const char **pathspec, int newfd, struct lock_file *lock_file)\n> >  {\n> > ...\n> > @@ -554,11 +549,14 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n> >  \t\t\t}\n> >  \t\t}\n> >  \n> > +\t\tint newfd;\n> > +\t\tstruct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));\n> > +\t\tnewfd = hold_locked_index(lock_file, 1);\n> > +\t\tread_cache();\n> > +\n> >  \t\tif (source_tree)\n> >  \t\t\tread_tree_some(source_tree, pathspec);\n> > -\t\telse\n> > -\t\t\tread_cache();\n> > -\t\treturn checkout_paths(pathspec);\n> > +\t\treturn checkout_paths(pathspec, newfd, lock_file);\n> \n> Aside from decl-after-statement, I suspect that at this point\n> these all should go to checkout_paths() function itself, that\n> takes pathspec and source_tree (which could be NULL), but that\n> is only \"logical code organization\" issue.  Thanks for fixing.\n\nYeah, that's a better organization, and makes the code nice. Want it as a \nreplacement patch or as an additional patch on top of this one?\n\n> I however would have liked if this were caught while the topic\n> was still cooking in 'next'.\n> \n> There was a process failure somewhere, which makes me worry more\n> than just this single bug that escaped to 'master'.  Alex\n> Riesen's segv fix in another thread makes it double X-<.\n\nThis one is a stat cache problem, and those tend not to get noticed for a \nwhile in normal use, simply because everything works fine, but makes \nsubsequent operations a bit slower.\n\nAlex's is a case where an operation that should fail fails ungracefully, \nwhich is again unlikely to be uncovered by normal use, because people just \ndon't do that.\n\nI think that cooking in next isn't likely to matter much to either of \nthese classes of problems, and we need to catch them in regression \ntesting. They're also not the sort of thing that we're likely to test for \nspecifically, so it would be nice to have something in test-lib to do it \nin general. It would be nice to have automatic checks after every test \nthat nothing segfaulted, and a helper (which could be sprinkled liberally \nthrough the test suite) to make sure that nothing has stale stat cache \ninfo (without being actually changed).\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"70320","messageId":"7vablkhi25.fsf@gitster.siamese.dyndns.org","threadId":"12363","inReplyTo":"alpine.LNX.1.00.0802281547320.19665@iabervon.org","subject":"Re: [PATCH] Write index file on any checkout of files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-02-28T21:44:50Z","receivedAt":"2008-02-28T21:44:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Daniel Barkalow <barkalow@iabervon.org> writes:\n\n> Yeah, that's a better organization, and makes the code nice. Want it as a \n> replacement patch or as an additional patch on top of this one?\n\nThanks.  Replacement would be nice, as I do look at patches\nduring my lunchtime but not apply them until evening, on my\nday-job days.\n"}]}