{"thread":{"id":"37891","subject":"[RFC] git checkout $tree -- $path always rewrites files","startedAt":"2014-11-07T08:13:24Z","lastAt":"2014-11-14T19:27:01Z","messageCount":23,"participants":["Jeff King","Duy Nguyen","Junio C Hamano","Martin von Zweigbergk","David Aguilar"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"251442","messageId":"20141107081324.GA19845@peff.net","threadId":"37891","inReplyTo":null,"subject":"[RFC] git checkout $tree -- $path always rewrites files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-11-07T08:13:24Z","receivedAt":"2014-11-07T08:13:24Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"I noticed that \"git checkout $tree -- $path\" will _always_ unlink and\nwrite a new copy of each matching path, even if they are up-to-date with\nthe index and the content in $tree is the same.\n\nHere's a simple reproduction:\n\n # some repo with a file in it\n git init\n echo content >foo && git add foo && git commit -m foo\n \n # give the file a known timestamp (it doesn't matter what)\n perl -e 'utime(123, 123, @ARGV)' foo\n git update-index --refresh\n \n # now check out an identical copy\n git checkout HEAD -- foo\n \n # and check whether it was updated\n perl -le 'print ((stat)[9]) for @ARGV' foo\n\nwhich yields a modern timestamp, showing that the file was rewritten.\n\nIf you use\n\n git checkout -- foo\n\ninstead to checkout from the index, that works fine (the final line\nprints 123). Similarly, if you load the new index and checkout\nseparately, like:\n\n git read-tree -m $tree\n git checkout -- foo\n\nthat also works. Of course it is not quite the same; read-tree loads the\nwhole index from $tree, whereas checkout would only touch a subset of\nthe entries. And that's the culprit; checkout does not use unpack-trees\nto only touch the entries that need updating. It uses read_tree_recursive\nwith a pathspec, and just replaces any index entries that match.\n\nBelow is a patch which makes it do what I expect by silently copying\nflags and stat-data from the old entry, but I feel like it is going in\nthe wrong direction. Besides causing a few tests to fail (which I\nsuspect is because I implemented it at the wrong layer), it really feels\nlike this should be using unpack-trees or something similar.\n\nAfter all, that is what \"git reset $tree -- foo\" does, and it gets this\ncase right (in fact, you can replace the read-tree above with it). It\nlooks like it actually does a pathspec-limited index-diff rather than\nunpack-trees, but the end effect is the same (and while checkout could\njust pass \"update\" to unpack-trees, we need to handle checking out\ndirectly from the index anyway, so I do not think it is a big deal to\nkeep the index update as a separate step).\n\nIs there a reason that we don't use this diff technique for checkout?\n\nAnyway, here is the (wrong) patch I came up with initially.\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 29b0f6d..802e556 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -273,19 +273,13 @@ static int checkout_paths(const struct checkout_opts *opts,\n \t\tce->ce_flags &= ~CE_MATCHED;\n \t\tif (!opts->ignore_skipworktree && ce_skip_worktree(ce))\n \t\t\tcontinue;\n-\t\tif (opts->source_tree && !(ce->ce_flags & CE_UPDATE))\n-\t\t\t/*\n-\t\t\t * \"git checkout tree-ish -- path\", but this entry\n-\t\t\t * is in the original index; it will not be checked\n-\t\t\t * out to the working tree and it does not matter\n-\t\t\t * if pathspec matched this entry. We will not do\n-\t\t\t * anything to this entry at all.\n-\t\t\t */\n-\t\t\tcontinue;\n+\n \t\t/*\n-\t\t * Either this entry came from the tree-ish we are\n-\t\t * checking the paths out of, or we are checking out\n-\t\t * of the index.\n+\t\t * If we are checking out from a tree-ish, we already\n+\t\t * did our pathspec matching. However, since we then\n+\t\t * drop the CE_UPDATE flag from any paths that do not\n+\t\t * need updating, we don't know which ones were not\n+\t\t * mentioned and which ones were simply up-to-date.\n \t\t *\n \t\t * If it comes from the tree-ish, we already know it\n \t\t * matches the pathspec and could just stamp\ndiff --git a/read-cache.c b/read-cache.c\nindex 8f3e9eb..fb2240b 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -962,8 +962,13 @@ static int add_index_entry_with_check(struct index_state *istate, struct cache_e\n \n \t/* existing match? Just replace it. */\n \tif (pos >= 0) {\n-\t\tif (!new_only)\n+\t\tif (!new_only) {\n+\t\t\tstruct cache_entry *old = istate->cache[pos];\n+\t\t\tif (!is_null_sha1(ce->sha1) &&\n+\t\t\t !hashcmp(old->sha1, ce->sha1))\n+\t\t\t\tcopy_cache_entry(ce, old);\n \t\t\treplace_index_entry(istate, pos, ce);\n+\t\t}\n \t\treturn 0;\n \t}\n \tpos = -pos-1;\n"},{"id":"251443","messageId":"20141107083805.GA26365@peff.net","threadId":"37891","inReplyTo":"20141107081324.GA19845@peff.net","subject":"Re: [RFC] git checkout $tree -- $path always rewrites files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-11-07T08:38:05Z","receivedAt":"2014-11-07T08:38:05Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 07, 2014 at 03:13:24AM -0500, Jeff King wrote:\n\n> I noticed that \"git checkout $tree -- $path\" will _always_ unlink and\n> write a new copy of each matching path, even if they are up-to-date with\n> the index and the content in $tree is the same.\n\nBy the way, one other thing I wondered while looking at this code: when\nwe checkout a working tree file, we unlink the old one and write the new\none in-place. Is there a particular reason we do this versus writing to\na temporary file and renaming it into place? That would give\nsimultaneous readers a more atomic view.\n\nI suspect the answer is something like: you cannot always do a rename,\nbecause you might have a typechange, directory becoming a file, or vice\nversa; so anyone relying on an atomic view during a checkout operation\nis already Doing It Wrong. Handling a content-change of an existing\npath would complicate the code, so we do not bother.\n\nBut I would be curious to hear confirmation of that.\n\n-Peff\n"},{"id":"251451","messageId":"CACsJy8CPLvmbcdHHmfu6g0dUXJVQ8NhwqfGPD=-kcBmzF_ha6g@mail.gmail.com","threadId":"37891","inReplyTo":"20141107083805.GA26365@peff.net","subject":"Re: [RFC] git checkout $tree -- $path always rewrites files","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-11-07T10:13:47Z","receivedAt":"2014-11-07T10:13:47Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Nov 7, 2014 at 3:38 PM, Jeff King wrote:\n> On Fri, Nov 07, 2014 at 03:13:24AM -0500, Jeff King wrote:\n>\n>> I noticed that \"git checkout $tree -- $path\" will _always_ unlink and\n>> write a new copy of each matching path, even if they are up-to-date with\n>> the index and the content in $tree is the same.\n>\n> By the way, one other thing I wondered while looking at this code: when\n> we checkout a working tree file, we unlink the old one and write the new\n> one in-place. Is there a particular reason we do this versus writing to\n> a temporary file and renaming it into place? That would give\n> simultaneous readers a more atomic view.\n>\n> I suspect the answer is something like: you cannot always do a rename,\n> because you might have a typechange, directory becoming a file, or vice\n> versa; so anyone relying on an atomic view during a checkout operation\n> is already Doing It Wrong. Handling a content-change of an existing\n> path would complicate the code, so we do not bother.\n\nNot a confirmation, but it looks like Linus did it just to make sure\nhe had new permissions right, in e447947 (Be much more liberal about\nthe file mode bits. - 2005-04-16).\n-- \nDuy\n"},{"id":"251459","messageId":"xmqqr3xfgdi4.fsf@gitster.dls.corp.google.com","threadId":"37891","inReplyTo":"CACsJy8CPLvmbcdHHmfu6g0dUXJVQ8NhwqfGPD=-kcBmzF_ha6g@mail.gmail.com","subject":"Re: [RFC] git checkout $tree -- $path always rewrites files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-07T16:51:47Z","receivedAt":"2014-11-07T16:51:47Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> On Fri, Nov 7, 2014 at 3:38 PM, Jeff King wrote:\n>> On Fri, Nov 07, 2014 at 03:13:24AM -0500, Jeff King wrote:\n>>\n>>> I noticed that \"git checkout $tree -- $path\" will _always_ unlink and\n>>> write a new copy of each matching path, even if they are up-to-date with\n>>> the index and the content in $tree is the same.\n>>\n>> By the way, one other thing I wondered while looking at this code: when\n>> we checkout a working tree file, we unlink the old one and write the new\n>> one in-place. Is there a particular reason we do this versus writing to\n>> a temporary file and renaming it into place? That would give\n>> simultaneous readers a more atomic view.\n>>\n>> I suspect the answer is something like: you cannot always do a rename,\n>> because you might have a typechange, directory becoming a file, or vice\n>> versa; so anyone relying on an atomic view during a checkout operation\n>> is already Doing It Wrong. Handling a content-change of an existing\n>> path would complicate the code, so we do not bother.\n>\n> Not a confirmation, but it looks like Linus did it just to make sure\n> he had new permissions right, in e447947 (Be much more liberal about\n> the file mode bits. - 2005-04-16).\n\nI think you are referring to the \"... to get the new one with the\nright permissions\" comment in that patch, but I do not think that\naffects the choice between (1) unlink and write anew to the final\nand (2) create a new temporary and rename. Either way, you do not\nupdate the existing file in-place and try to checkout the permission\nbits with chmod(2).\n"},{"id":"251462","messageId":"xmqqegtfgcfx.fsf@gitster.dls.corp.google.com","threadId":"37891","inReplyTo":"20141107081324.GA19845@peff.net","subject":"Re: [RFC] git checkout $tree -- $path always rewrites files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-07T17:14:42Z","receivedAt":"2014-11-07T17:14:42Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Is there a reason that we don't use this diff technique for checkout?\n\nI suspect that the reasons are probably mixture of these:\n\n (1) the code path may descend from checkout-index and predates the\n in-core diff machinery;\n\n (2) in the context of checkout-index, it was more desirable to be\n able to say \"I want the contents on the filesystem, even if you\n think I already have it there, as you as a new software are\n likely to be wrong and I know better\"; or\n\n (3) it was easier to code that way ;-)\n\nI do not see there is any reason not to do what you suggest.\n"},{"id":"251475","messageId":"20141107191554.GA5695@peff.net","threadId":"37891","inReplyTo":"CACsJy8CPLvmbcdHHmfu6g0dUXJVQ8NhwqfGPD=-kcBmzF_ha6g@mail.gmail.com","subject":"Re: [RFC] git checkout $tree -- $path always rewrites files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-11-07T19:15:55Z","receivedAt":"2014-11-07T19:15:55Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 07, 2014 at 05:13:47PM +0700, Duy Nguyen wrote:\n\n> > By the way, one other thing I wondered while looking at this code: when\n> > we checkout a working tree file, we unlink the old one and write the new\n> > one in-place. Is there a particular reason we do this versus writing to\n> > a temporary file and renaming it into place? That would give\n> > simultaneous readers a more atomic view.\n> >\n> > I suspect the answer is something like: you cannot always do a rename,\n> > because you might have a typechange, directory becoming a file, or vice\n> > versa; so anyone relying on an atomic view during a checkout operation\n> > is already Doing It Wrong. Handling a content-change of an existing\n> > path would complicate the code, so we do not bother.\n> \n> Not a confirmation, but it looks like Linus did it just to make sure\n> he had new permissions right, in e447947 (Be much more liberal about\n> the file mode bits. - 2005-04-16).\n\nThanks for digging that up. I think that only gives us half the story,\nthough. That explains why we would unlink/open instead of relying on\njust open(O_TRUNC). But I think opening a new tempfile would work the\nsame as the current code in that respect.\n\n-Peff\n"},{"id":"251476","messageId":"20141107191745.GB5695@peff.net","threadId":"37891","inReplyTo":"xmqqegtfgcfx.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC] git checkout $tree -- $path always rewrites files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-11-07T19:17:45Z","receivedAt":"2014-11-07T19:17:45Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 07, 2014 at 09:14:42AM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > Is there a reason that we don't use this diff technique for checkout?\n> \n> I suspect that the reasons are probably mixture of these:\n> \n> (1) the code path may descend from checkout-index and predates the\n> in-core diff machinery;\n> \n> (2) in the context of checkout-index, it was more desirable to be\n> able to say \"I want the contents on the filesystem, even if you\n> think I already have it there, as you as a new software are\n> likely to be wrong and I know better\"; or\n> \n> (3) it was easier to code that way ;-)\n> \n> I do not see there is any reason not to do what you suggest.\n\nOK. It's not very much code (and can mostly be lifted from git-reset),\nso I may take a stab at it.\n\n-Peff\n"},{"id":"251521","messageId":"CANiSa6ggX-DJSXLzjYwv1K2nF1ZrpJ3bHvPjh6gFnqSLQaqZFQ@mail.gmail.com","threadId":"37891","inReplyTo":"CANiSa6hufp=80TaesNpo1CxCbwVq3LPXvYaUSbcmzPE5pj_GGw@mail.gmail.com","subject":"Re: [RFC] git checkout $tree -- $path always rewrites files","fromName":"Martin von Zweigbergk","fromEmail":"martinvonz@gmail.com","sentAt":"2014-11-08T07:10:08Z","receivedAt":"2014-11-08T07:10:08Z","isPatch":false,"sender":{"key":"martinvonz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/891642?v=4"},"body":"Trying again from plain old gmail which I think does not send a\nmultipart content.\n\nOn Fri, Nov 7, 2014 at 11:06 PM, Martin von Zweigbergk\n wrote:\n> Is this also related to \"git checkout $rev .\" not removing removed files?\n> What you say about the difference in implementation between checkout and\n> reset sounds vaguely familiar from when I looked at it some years ago.\n> Curiously, I just talked to Jonathan about this over lunch yesterday. I\n> think we would both be happy to get that oddity of checkout fixed. If what\n> you mention here is indeed related to fixing that, it does complicate things\n> with regards to backwards compatibility.\n>\n>\n> On Fri Nov 07 2014 at 11:24:09 AM Jeff King wrote:\n>>\n>> On Fri, Nov 07, 2014 at 09:14:42AM -0800, Junio C Hamano wrote:\n>>\n>> > Jeff King writes:\n>> >\n>> > > Is there a reason that we don't use this diff technique for checkout?\n>> >\n>> > I suspect that the reasons are probably mixture of these:\n>> >\n>> > (1) the code path may descend from checkout-index and predates the\n>> > in-core diff machinery;\n>> >\n>> > (2) in the context of checkout-index, it was more desirable to be\n>> > able to say \"I want the contents on the filesystem, even if you\n>> > think I already have it there, as you as a new software are\n>> > likely to be wrong and I know better\"; or\n>> >\n>> > (3) it was easier to code that way ;-)\n>> >\n>> > I do not see there is any reason not to do what you suggest.\n>>\n>> OK. It's not very much code (and can mostly be lifted from git-reset),\n>> so I may take a stab at it.\n>>\n>> -Peff\n>> --\n>> To unsubscribe from this list: send the line \"unsubscribe git\" in\n>> the body of a message to majordomo@vger.kernel.org\n>> More majordomo info at http://vger.kernel.org/majordomo-info.html\n"},{"id":"251522","messageId":"CANiSa6hPHfh_Vz_X8cqKeEbOfAAj1YmxLr5LyG0ncm5U=r4SVQ@mail.gmail.com","threadId":"37891","inReplyTo":"CAPc5daWdzrHr8Rdksr3HycMRQu0=Ji7h=BPYjzZj7MH6Ko0VgQ@mail.gmail.com","subject":"Re: [RFC] git checkout $tree -- $path always rewrites files","fromName":"Martin von Zweigbergk","fromEmail":"martinvonz@gmail.com","sentAt":"2014-11-08T08:03:41Z","receivedAt":"2014-11-08T08:03:41Z","isPatch":false,"sender":{"key":"martinvonz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/891642?v=4"},"body":"I'm not sure I had seen that particular thread, but it seems like\nwe're all in agreement on that topic. I'm keeping my fingers crossed\nthat Jeff will have time to tackle that this time :-)\n\nOn Fri, Nov 7, 2014 at 11:35 PM, Junio C Hamano wrote:\n> I think that has direct linkage; what you have in mind I think is\n> http://thread.gmane.org/gmane.comp.version-control.git/234903/focus=234935\n>\n> What is on thread here is an implementation glitch, not semantic one.\n> Checking out a \"directory\" as opposed to \"set of paths that match the\n> pathspec\" was a deliberate design choice, not an implementation glitch.\n>\n> Pardon HTML, misspellings and grammos, typed on a tablet.\n>\n> On Nov 7, 2014 11:10 PM, \"Martin von Zweigbergk\" \n> wrote:\n>>\n>> Trying again from plain old gmail which I think does not send a\n>> multipart content.\n>>\n>> On Fri, Nov 7, 2014 at 11:06 PM, Martin von Zweigbergk\n>> wrote:\n>> > Is this also related to \"git checkout $rev .\" not removing removed\n>> > files?\n>> > What you say about the difference in implementation between checkout and\n>> > reset sounds vaguely familiar from when I looked at it some years ago.\n>> > Curiously, I just talked to Jonathan about this over lunch yesterday. I\n>> > think we would both be happy to get that oddity of checkout fixed. If\n>> > what\n>> > you mention here is indeed related to fixing that, it does complicate\n>> > things\n>> > with regards to backwards compatibility.\n>> >\n>> >\n>> > On Fri Nov 07 2014 at 11:24:09 AM Jeff King wrote:\n>> >>\n>> >> On Fri, Nov 07, 2014 at 09:14:42AM -0800, Junio C Hamano wrote:\n>> >>\n>> >> > Jeff King writes:\n>> >> >\n>> >> > > Is there a reason that we don't use this diff technique for\n>> >> > > checkout?\n>> >> >\n>> >> > I suspect that the reasons are probably mixture of these:\n>> >> >\n>> >> > (1) the code path may descend from checkout-index and predates the\n>> >> > in-core diff machinery;\n>> >> >\n>> >> > (2) in the context of checkout-index, it was more desirable to be\n>> >> > able to say \"I want the contents on the filesystem, even if you\n>> >> > think I already have it there, as you as a new software are\n>> >> > likely to be wrong and I know better\"; or\n>> >> >\n>> >> > (3) it was easier to code that way ;-)\n>> >> >\n>> >> > I do not see there is any reason not to do what you suggest.\n>> >>\n>> >> OK. It's not very much code (and can mostly be lifted from git-reset),\n>> >> so I may take a stab at it.\n>> >>\n>> >> -Peff\n>> >> --\n>> >> To unsubscribe from this list: send the line \"unsubscribe git\" in\n>> >> the body of a message to majordomo@vger.kernel.org\n>> >> More majordomo info at http://vger.kernel.org/majordomo-info.html\n"},{"id":"251523","messageId":"20141108083040.GA15833@peff.net","threadId":"37891","inReplyTo":"CAPc5daWdzrHr8Rdksr3HycMRQu0=Ji7h=BPYjzZj7MH6Ko0VgQ@mail.gmail.com","subject":"Re: [RFC] git checkout $tree -- $path always rewrites files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-11-08T08:30:41Z","receivedAt":"2014-11-08T08:30:41Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 07, 2014 at 11:35:59PM -0800, Junio C Hamano wrote:\n\n> I think that has direct linkage; what you have in mind I think is\n> http://thread.gmane.org/gmane.comp.version-control.git/234903/focus=234935\n\nThanks for that link.\n\nI did spend a few hours on this topic earlier today, and got very\nconfused trying to figure out what the deletion behavior _should_ be,\nand whether I was breaking it. For some reason I had zero recollection\nof a conversation from last year that I was obviously a major part of. I\nthink I am getting old. :)\n\nThe end of that thread concludes that a diff-based approach is not going\nto work, because we need to update the working tree even for files not\nmentioned by the diff. I do not think that is a show-stopper, though.\nIt just means that we need to load the new index as one step (done now\nwith read_tree_recursive, but ideally using diff), and then walk over\nthe whole resulting index applying our pathspec again (instead of\nrelying on CE_UPDATE flags).\n\nThis turns out not to be a big deal, because the existing code is\nalready doing most of that second pathspec application anyway. It does\nit because read_tree_recursive is not smart enough to update the \"seen\"\nbits for the pathspec. But now we would have another reason to do it\nthis way. :)\n\nSo just to be clear, the behavior we want is that:\n\n echo foo >some-new-path\n git add some-new-path\n git checkout HEAD -- .\n\nwill delete some-new-path (whereas the current code turns it into an\nuntracked file). What should:\n\n git checkout HEAD -- some-new-path\n\ndo in that case? With the current code, it actually barfs, complaining\nthat nothing matched some-new-path (because it is not part of HEAD, and\ntherefore we don't consider it at all), and aborts the whole operation.\nI think we would want to delete some-new-path in that case, too.\n\n-Peff\n"},{"id":"251524","messageId":"20141108084526.GA18912@peff.net","threadId":"37891","inReplyTo":"20141108083040.GA15833@peff.net","subject":"Re: [RFC] git checkout $tree -- $path always rewrites files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-11-08T08:45:27Z","receivedAt":"2014-11-08T08:45:27Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Nov 08, 2014 at 03:30:40AM -0500, Jeff King wrote:\n\n> So just to be clear, the behavior we want is that:\n> \n> echo foo >some-new-path\n> git add some-new-path\n> git checkout HEAD -- .\n> \n> will delete some-new-path (whereas the current code turns it into an\n> untracked file). What should:\n> \n> git checkout HEAD -- some-new-path\n> \n> do in that case? With the current code, it actually barfs, complaining\n> that nothing matched some-new-path (because it is not part of HEAD, and\n> therefore we don't consider it at all), and aborts the whole operation.\n> I think we would want to delete some-new-path in that case, too.\n\nAlso, t2022.3 has me very confused.\n\nIt is explicitly checking that if we have \"subdir/foo\" unmerged in the\nindex, and we \"git checkout $tree -- subdir\", and $tree does not mention\n\"foo\", that we _leave_ foo in place.\n\nThat seems very counter-intuitive to me. If you asked to make \"subdir\"\nlook like $tree, then we should clobber it. That change comes from\ne721c15 (checkout: avoid unnecessary match_pathspec calls, 2013-03-27),\nwhere it is mentioned as a _bugfix_. That in turn references 0a1283b\n(checkout $tree $path: do not clobber local changes in $path not in\n$tree, 2011-09-30), which explicitly goes against the goal we are\ntalking about here. It is not \"make my index and working tree look like\n$tree\" at all.\n\nSo now I'm doubly confused about what we want to do.\n\nIf we want to retain that behavior, I think we can still cover these\ncases by marking items missing from $tree as \"to remove\" during the\ndiff/\"update the index\" phase, and then being more gentle with \"to\nremove\" files (e.g., not clobbering changed worktree files unless \"-f\"\nis given).\n\nI am not sure that provides a sane user experience, though. Why is it OK\nto clobber local changes to a file if we are replacing it with other\ncontent, but _not_ if we are replacing it with nothing? Either the\ncontent we are losing is valuable or not, but it has nothing to do with\nwhat we are replacing. And Junio argued in the thread linked elsewhere\nthat the point of \"git checkout $tree -- $path\" is to clobber what is in\n$path, which I would agree with.\n\nI think the argument made in 0a1283b is that \"git checkout $tree $path\"\nis not \"make $path like $tree\", but rather \"pick bits of $path out of\n$tree\". Which would mean this whole deletion thing we are talking about\nis completely contrary to that.\n\nSo which is it?\n\n-Peff\n"},{"id":"251562","messageId":"CANiSa6gqu9cRJ4gY5M4ou_zQP=1+U2_C9nHDOoaX01yYn5C+aw@mail.gmail.com","threadId":"37891","inReplyTo":"20141108083040.GA15833@peff.net","subject":"Re: [RFC] git checkout $tree -- $path always rewrites files","fromName":"Martin von Zweigbergk","fromEmail":"martinvonz@gmail.com","sentAt":"2014-11-08T16:19:21Z","receivedAt":"2014-11-08T16:19:21Z","isPatch":false,"sender":{"key":"martinvonz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/891642?v=4"},"body":"First of all, thanks again for spending time on this.\n\nOn Sat, Nov 8, 2014 at 12:30 AM, Jeff King wrote:\n> On Fri, Nov 07, 2014 at 11:35:59PM -0800, Junio C Hamano wrote:\n>\n> So just to be clear, the behavior we want is that:\n>\n> echo foo >some-new-path\n> git add some-new-path\n> git checkout HEAD -- .\n>\n> will delete some-new-path (whereas the current code turns it into an\n> untracked file).\n\nYes, I think that's what I would expect.\n\n> What should:\n>\n> git checkout HEAD -- some-new-path\n>\n> do in that case? With the current code, it actually barfs, complaining\n> that nothing matched some-new-path (because it is not part of HEAD, and\n> therefore we don't consider it at all), and aborts the whole operation.\n> I think we would want to delete some-new-path in that case, too.\n\nI don't think we'd want it to be deleted. I would view 'git reset\n--hard' as the role model here, and that command (without paths) would\nnot remove the file. And applying it to a path should not change the\nbehavior, just restrict it to the paths, right?\n"},{"id":"251590","messageId":"20141109094243.GA17369@peff.net","threadId":"37891","inReplyTo":"CANiSa6gqu9cRJ4gY5M4ou_zQP=1+U2_C9nHDOoaX01yYn5C+aw@mail.gmail.com","subject":"Re: [RFC] git checkout $tree -- $path always rewrites files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-11-09T09:42:43Z","receivedAt":"2014-11-09T09:42:43Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Nov 08, 2014 at 08:19:21AM -0800, Martin von Zweigbergk wrote:\n\n> > What should:\n> >\n> > git checkout HEAD -- some-new-path\n> >\n> > do in that case? With the current code, it actually barfs, complaining\n> > that nothing matched some-new-path (because it is not part of HEAD, and\n> > therefore we don't consider it at all), and aborts the whole operation.\n> > I think we would want to delete some-new-path in that case, too.\n> \n> I don't think we'd want it to be deleted. I would view 'git reset\n> --hard' as the role model here, and that command (without paths) would\n> not remove the file. And applying it to a path should not change the\n> behavior, just restrict it to the paths, right?\n\nAre you sure about \"git reset\" here? If I do:\n\n git init\n echo content >file && git add file && git commit -m base\n echo modified >file\n echo new >some-new-path\n git add file some-new-path\n git reset --hard\n\nthen we delete some-new-path (it is not untracked, because the index\nknows about it). That makes sense to me. I.e., we treat it with the same\n\"preciousness\" whether it is named explicitly or not.\n\n-Peff\n"},{"id":"251620","messageId":"xmqqbnoge1ci.fsf@gitster.dls.corp.google.com","threadId":"37891","inReplyTo":"20141108083040.GA15833@peff.net","subject":"Re: [RFC] git checkout $tree -- $path always rewrites files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-09T17:21:49Z","receivedAt":"2014-11-09T17:21:49Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, Nov 07, 2014 at 11:35:59PM -0800, Junio C Hamano wrote:\n>\n>> I think that has direct linkage; what you have in mind I think is\n>> http://thread.gmane.org/gmane.comp.version-control.git/234903/focus=234935\n>\n> Thanks for that link.\n\nIt was one of the items in the \"git blame leftover bits\" list\n(websearch for that exact phrase), so I didn't have to do any\ndigging just for this thread ;-)\n\nBut I made a huge typo above. s/I think/I do not think/;\n\nThe original observation you made in this thread is that when \"git\ncheckout $tree - $pathspec\", whose defintion is to \"grab paths in\nthe $tree that match $pathspec and put them in the index, and then\noverwrite the working tree paths with the contents of these paths\nthat are updated in the index with the contents taken from the\n$tree\", unnecessarily rewrites the working tree paths even when they\nalready had contents that were up to date. That is what I called an\n\"implementation glitch\".\n\nThe old thread is a different topic. It is about changing the\nsemantics of the operation to \"make paths in the index and in the\n$tree identical, and then update the working tree paths to also\nmatch, all with respect to the $pathspec\". This, as Martin noted,\nneeds careful debate on the merit and transition plan if we decide\nthat it is worth doing. The \"do ignore paths that are not in $tree\"\nis a deliberate design choice.\n\nI'd prefer that these two to be treated separately. That is, even\nif our plan did not involve changing the semantics of the operation,\nwe would want to fix the implementation glitch. Compare the $tree\nwith the index using $pathspec, adjust the index by adding paths\nthat are missing from the index that are in $tree, but not removing\nthe entries in the index only because they are not in $tree (note:\nwhen the index has a path A, the $tree has a path A/B, and the\n$pathspec says A, we would end up removing A to make room for A/B,\nand that should be allowed---it does not fall into the \"only because\nthe path is not in $tree\". In such a scenario, we remove A not\nbecause $tree does not have A but because A/B that the $tree has is\nwhat we were asked to materialize). And after updating the index\nthat way, do an equivalent of \"git checkout -- $pathspec\". The\nentries that were the same between the $tree and the index will have\nthe up-to-dateness kept and will not unnecessarily rewrite an\nunmodified path that way, while things that are modified with the\noperation will be overwritten, I would think.\n\nAnd with that machinery in place, we could start thinking about\nupdating the semantics. It will be a small change to the loop that\ngoes over the result from diff_index() and modifying the code that\nused to do a \"not remove only because not in $tree\" to do a \"remove\nif not in $tree\".\n\n> So just to be clear, the behavior we want is that:\n>\n> echo foo >some-new-path\n> git add some-new-path\n> git checkout HEAD -- .\n>\n> will delete some-new-path (whereas the current code turns it into an\n> untracked file).\n\nWith the updated semantics proposed in the old thread, yes, that is\nwhat should happen.\n\n> git checkout HEAD -- some-new-path\n>\n> do in that case?\n\nLikewise. And if some-new-path were a directory, with existing path\nO and new path N both in the index but only the former in HEAD, the\noperation would revert some-new-path/O to that of HEAD and remove\nsome-new-path/N. That is the only logical thing we could do if we\nwere to take the updated sematics.\n\nThat is one of the reasons why I am not 100% convinced that the\nproposed updated semantics is better, even though I was fairly\npositive in the old discussion and also I kept the topic in the\n\"leftover bits\" list. The above command is a fairly common way to\nsay \"I started refactoring the existing path some-path/O and\nsprinkled its original contents spread into new files A, B and C in\nthe same directory. Now I no longer have O in the working tree, but\nlet me double check by grabbing it out of the state recoded in the\ncommit\". You expect that \"git checkout HEAD -- some-path\" would not\nlose A, B or C, knowing \"some-path\" only had O. That expectation\nwould even be stronger if you are used to the current semantics, but\nthat is something we could fix, if we decide that the proposed\nupdated semantics is better, with a careful transition plan.\n\nIt might be less risky if the updated semantics were to make the\npaths that are originally in the index but not in $tree untracked\n(as opposed to \"reset --hard\" emulation where they will be lost)\nunless they need to be removed to make room for D/F conflict issues,\nbut I haven't thought it through.\n\nThanks.\n"},{"id":"251632","messageId":"xmqqh9y8b4pi.fsf@gitster.dls.corp.google.com","threadId":"37891","inReplyTo":"20141108084526.GA18912@peff.net","subject":"Re: [RFC] git checkout $tree -- $path always rewrites files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-09T18:37:29Z","receivedAt":"2014-11-09T18:37:29Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Sat, Nov 08, 2014 at 03:30:40AM -0500, Jeff King wrote:\n>\n>> So just to be clear, the behavior we want is that:\n>> \n>> echo foo >some-new-path\n>> git add some-new-path\n>> git checkout HEAD -- .\n>> \n>> will delete some-new-path (whereas the current code turns it into an\n>> untracked file). What should:\n>> \n>> git checkout HEAD -- some-new-path\n>> \n>> do in that case? With the current code, it actually barfs, complaining\n>> that nothing matched some-new-path (because it is not part of HEAD, and\n>> therefore we don't consider it at all), and aborts the whole operation.\n>> I think we would want to delete some-new-path in that case, too.\n>\n> Also, t2022.3 has me very confused.\n>\n> It is explicitly checking that if we have \"subdir/foo\" unmerged in the\n> index, and we \"git checkout $tree -- subdir\", and $tree does not mention\n> \"foo\", that we _leave_ foo in place.\n\nThe definition of how \"checkout $tree -- $pathspec\" should behave\nlogically leads to that, I think. Grabbing everything that matches\nthe \"subdir\" pathspec from $tree, adding them to the index and\nchecking them out will not touch subdir/foo that does not appear in\nthat $tree.\n\nWith the proposed updated semantics, it would behave differently, so\nit is natural that we have tests that protect the traditional\ndefinition of how it should behave and we will have to visit them\nand update their expectation if we decide that the proposed updated\nsemantics is what we want.\n\n> That seems very counter-intuitive to me. If you asked to make \"subdir\"\n> look like $tree, then we should clobber it.\n\nYes. But the existing expectation is different ;-)\n\nIf the $tree has 'subdir/foo', we would want \"checkout $tree --\nsubdir/foo\" to keep working as a way for the user to declare the\nresolution of the ongoing merge. With the proposed change in\nsematics, \"git checkout $tree -- subdir\" will become a way to say\n\"Everything inside subdir/ I'd resolve to the state recorded in\n$tree\" during such a conflicted merge, and it will lose paths not in\nthe $tree.\n\nWhich may be a good thing, but existing users who are used to the\ntraditional behaviour will find it confusing and even risky (i.e. \"I\nam checking OUT; never expected it to lose any path\"). A counter\nargument that I find sensible, of course, is \"You told us to check\nout subdir/, and the state recorded for subdir/ in $tree does not\nhave 'foo' in it. That is the state you asked us to check out\".\n\nThe argument for \"checkout $tree -- subdir/foo\" is essentially the\nsame. The state recorded in $tree for subdir/foo is non-existence,\nand that is checked out with the proposed new semantics.\n"},{"id":"251834","messageId":"20141113183033.GA24107@peff.net","threadId":"37891","inReplyTo":"xmqqbnoge1ci.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC] git checkout $tree -- $path always rewrites files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-11-13T18:30:34Z","receivedAt":"2014-11-13T18:30:34Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Nov 09, 2014 at 09:21:49AM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > On Fri, Nov 07, 2014 at 11:35:59PM -0800, Junio C Hamano wrote:\n> >\n> >> I think that has direct linkage; what you have in mind I think is\n> >> http://thread.gmane.org/gmane.comp.version-control.git/234903/focus=234935\n> >\n> > Thanks for that link.\n> \n> It was one of the items in the \"git blame leftover bits\" list\n> (websearch for that exact phrase), so I didn't have to do any\n> digging just for this thread ;-)\n> \n> But I made a huge typo above. s/I think/I do not think/;\n\nOh. That might explain some of my confusion. :)\n\n> The original observation you made in this thread is that when \"git\n> checkout $tree - $pathspec\", whose defintion is to \"grab paths in\n> the $tree that match $pathspec and put them in the index, and then\n> overwrite the working tree paths with the contents of these paths\n> that are updated in the index with the contents taken from the\n> $tree\", unnecessarily rewrites the working tree paths even when they\n> already had contents that were up to date. That is what I called an\n> \"implementation glitch\".\n> \n> The old thread is a different topic.\n> [...]\n\nRight, I do agree that these things don't need to be linked. The reason\nI ended up dealing with the deletion thing is that one obvious way to\nimplement \"do not touch entries which have not changed\" is by running a\ndiff between $tree and the index. But doing a diff means we have to\nreconsider all of the code surrounding deletions (and handling of\nunmerged entries in the pathspec but not in $tree). I tried a few\nvariants and had trouble making it work (getting caught up either in the\n\"make sure each pathspec matched\" code, or the \"treat unmerged entries\nspecially\" behavior).\n\nIn the patch below, I ended up retaining the existing\nread_tree_recursive code, and just specially handling the replacement of\nindex entries (which is itself sort of a diff, but it fits nicely into\nthe existing scheme).\n\n> I'd prefer that these two to be treated separately.\n\nYeah, that makes sense after reading your emails. What I was really\nunclear on was whether the handling of deletion was a bug or a design\nchoice, and it is the latter (if it were the former, we would not need a\ntransition plan :) ).\n\nAnyway, here is the patch.\n\n-- >8 --\nSubject: checkout $tree: do not throw away unchanged index entries\n\nWhen we \"git checkout $tree\", we pull paths from $tree into\nthe index, and then check the resulting entries out to the\nworktree. Our method for the first step is rather\nheavy-handed, though; it clobbers the entire existing index\nentry, even if the content is the same. This means we lose\nour stat information, leading checkout_entry to later\nrewrite the entire file with identical content.\n\nInstead, let's see if we have the identical entry already in\nthe index, in which case we leave it in place. That lets\ncheckout_entry do the right thing. Our tests cover two\ninteresting cases:\n\n 1. We make sure that a file which has no changes is not\n rewritten.\n\n 2. We make sure that we do update a file that is unchanged\n in the index (versus $tree), but has working tree\n changes. We keep the old index entry, and\n checkout_entry is able to realize that our stat\n information is out of date.\n\nSigned-off-by: Jeff King \n---\nNote that the test refreshes the index manually (because we are tweaking\nthe timestamp of file2). In normal use this should not be necessary\n(i.e., your entries should generally be uptodate). I did wonder if\ncheckout should be refreshing the index itself, but it would a bunch of\nextra lstats in the common case.\n\n builtin/checkout.c | 31 +++++++++++++++++++++++++------\n t/t2022-checkout-paths.sh | 17 +++++++++++++++++\n 2 files changed, 42 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 5410dac..67cab4e 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -65,21 +65,40 @@ static int post_checkout_hook(struct commit *old, struct commit *new,\n static int update_some(const unsigned char *sha1, const char *base, int baselen,\n \t\tconst char *pathname, unsigned mode, int stage, void *context)\n {\n-\tint len;\n+\tstruct strbuf buf = STRBUF_INIT;\n \tstruct cache_entry *ce;\n+\tint pos;\n \n \tif (S_ISDIR(mode))\n \t\treturn READ_TREE_RECURSIVE;\n \n-\tlen = baselen + strlen(pathname);\n-\tce = xcalloc(1, cache_entry_size(len));\n+\tstrbuf_add(&buf, base, baselen);\n+\tstrbuf_addstr(&buf, pathname);\n+\n+\t/*\n+\t * If the entry is the same as the current index, we can leave the old\n+\t * entry in place. Whether it is UPTODATE or not, checkout_entry will\n+\t * do the right thing.\n+\t */\n+\tpos = cache_name_pos(buf.buf, buf.len);\n+\tif (pos >= 0) {\n+\t\tstruct cache_entry *old = active_cache[pos];\n+\t\tif (create_ce_mode(mode) == old->ce_mode &&\n+\t\t !hashcmp(old->sha1, sha1)) {\n+\t\t\told->ce_flags |= CE_UPDATE;\n+\t\t\tstrbuf_release(&buf);\n+\t\t\treturn 0;\n+\t\t}\n+\t}\n+\n+\tce = xcalloc(1, cache_entry_size(buf.len));\n \thashcpy(ce->sha1, sha1);\n-\tmemcpy(ce->name, base, baselen);\n-\tmemcpy(ce->name + baselen, pathname, len - baselen);\n+\tmemcpy(ce->name, buf.buf, buf.len);\n \tce->ce_flags = create_ce_flags(0) | CE_UPDATE;\n-\tce->ce_namelen = len;\n+\tce->ce_namelen = buf.len;\n \tce->ce_mode = create_ce_mode(mode);\n \tadd_cache_entry(ce, ADD_CACHE_OK_TO_ADD | ADD_CACHE_OK_TO_REPLACE);\n+\tstrbuf_release(&buf);\n \treturn 0;\n }\n \ndiff --git a/t/t2022-checkout-paths.sh b/t/t2022-checkout-paths.sh\nindex 8e3545d..f46d049 100755\n--- a/t/t2022-checkout-paths.sh\n+++ b/t/t2022-checkout-paths.sh\n@@ -61,4 +61,21 @@ test_expect_success 'do not touch unmerged entries matching $path but not in $tr\n \ttest_cmp expect.next0 actual.next0\n '\n \n+test_expect_success 'do not touch files that are already up-to-date' '\n+\tgit reset --hard &&\n+\techo one >file1 &&\n+\techo two >file2 &&\n+\tgit add file1 file2 &&\n+\tgit commit -m base &&\n+\techo modified >file1 &&\n+\ttest-chmtime =1000000000 file2 &&\n+\tgit update-index -q --refresh &&\n+\tgit checkout HEAD -- file1 file2 &&\n+\techo one >expect &&\n+\ttest_cmp expect file1 &&\n+\techo \"1000000000\tfile2\" >expect &&\n+\ttest-chmtime -v +0 file2 >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.1.2.596.g7379948\n"},{"id":"251839","messageId":"xmqqbnoa29ps.fsf@gitster.dls.corp.google.com","threadId":"37891","inReplyTo":"20141113183033.GA24107@peff.net","subject":"Re: [RFC] git checkout $tree -- $path always rewrites files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-13T19:15:27Z","receivedAt":"2014-11-13T19:15:27Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Sun, Nov 09, 2014 at 09:21:49AM -0800, Junio C Hamano wrote:\n>\n>> Jeff King writes:\n>> \n>> > On Fri, Nov 07, 2014 at 11:35:59PM -0800, Junio C Hamano wrote:\n>> >\n>> >> I think that has direct linkage; what you have in mind I think is\n>> >> http://thread.gmane.org/gmane.comp.version-control.git/234903/focus=234935\n>> >\n>> > Thanks for that link.\n>> \n>> It was one of the items in the \"git blame leftover bits\" list\n>> (websearch for that exact phrase), so I didn't have to do any\n>> digging just for this thread ;-)\n>> \n>> But I made a huge typo above. s/I think/I do not think/;\n>\n> Oh. That might explain some of my confusion. :)\n\nYeah, tells me to never type on a tablet X-<.\n\n>> I'd prefer that these two to be treated separately.\n>\n> Yeah, that makes sense after reading your emails. What I was really\n> unclear on was whether the handling of deletion was a bug or a design\n> choice, and it is the latter (if it were the former, we would not need a\n> transition plan :) ).\n\nYeah, I think we agree to refrain from saying if that design choice\nwas a good one or bad one at least for now.\n\n> Subject: checkout $tree: do not throw away unchanged index entries\n>\n> When we \"git checkout $tree\", we pull paths from $tree into\n> the index, and then check the resulting entries out to the\n> worktree. Our method for the first step is rather\n> heavy-handed, though; it clobbers the entire existing index\n> entry, even if the content is the same. This means we lose\n> our stat information, leading checkout_entry to later\n> rewrite the entire file with identical content.\n>\n> Instead, let's see if we have the identical entry already in\n> the index, in which case we leave it in place. That lets\n> checkout_entry do the right thing. Our tests cover two\n> interesting cases:\n>\n> 1. We make sure that a file which has no changes is not\n> rewritten.\n>\n> 2. We make sure that we do update a file that is unchanged\n> in the index (versus $tree), but has working tree\n> changes. We keep the old index entry, and\n> checkout_entry is able to realize that our stat\n> information is out of date.\n>\n> Signed-off-by: Jeff King \n> ---\n> Note that the test refreshes the index manually (because we are tweaking\n> the timestamp of file2). In normal use this should not be necessary\n> (i.e., your entries should generally be uptodate). I did wonder if\n> checkout should be refreshing the index itself, but it would a bunch of\n> extra lstats in the common case.\n>\n> builtin/checkout.c | 31 +++++++++++++++++++++++++------\n> t/t2022-checkout-paths.sh | 17 +++++++++++++++++\n> 2 files changed, 42 insertions(+), 6 deletions(-)\n>\n> diff --git a/builtin/checkout.c b/builtin/checkout.c\n> index 5410dac..67cab4e 100644\n> --- a/builtin/checkout.c\n> +++ b/builtin/checkout.c\n> @@ -65,21 +65,40 @@ static int post_checkout_hook(struct commit *old, struct commit *new,\n> static int update_some(const unsigned char *sha1, const char *base, int baselen,\n> \t\tconst char *pathname, unsigned mode, int stage, void *context)\n> {\n> ...\n> }\n\nMakes sense, including the use of strbuf (otherwise you would\nallocate ce and then discard when it turns out that it is not\nneeded, which is probably with the same allocation pressure, but\nlooks uglier).\n\n> diff --git a/t/t2022-checkout-paths.sh b/t/t2022-checkout-paths.sh\n> index 8e3545d..f46d049 100755\n> --- a/t/t2022-checkout-paths.sh\n> +++ b/t/t2022-checkout-paths.sh\n> @@ -61,4 +61,21 @@ test_expect_success 'do not touch unmerged entries matching $path but not in $tr\n> \ttest_cmp expect.next0 actual.next0\n> '\n> \n> +test_expect_success 'do not touch files that are already up-to-date' '\n> +\tgit reset --hard &&\n> +\techo one >file1 &&\n> +\techo two >file2 &&\n> +\tgit add file1 file2 &&\n> +\tgit commit -m base &&\n> +\techo modified >file1 &&\n> +\ttest-chmtime =1000000000 file2 &&\n\nIs the idea behind the hardcoded timestamp that this is sufficiently\nold (Sep 2001) that we will not get in trouble comparing with the\nreal timestamp we get from the filesystem (which will definitely newer\nthan that anyway) no matter when we run this test (unless you have a\ntime-machine, that is)?\n\n> +\tgit update-index -q --refresh &&\n> +\tgit checkout HEAD -- file1 file2 &&\n> +\techo one >expect &&\n> +\ttest_cmp expect file1 &&\n> +\techo \"1000000000\tfile2\" >expect &&\n> +\ttest-chmtime -v +0 file2 >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> test_done\n"},{"id":"251843","messageId":"20141113192655.GA3413@peff.net","threadId":"37891","inReplyTo":"xmqqbnoa29ps.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC] git checkout $tree -- $path always rewrites files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-11-13T19:26:55Z","receivedAt":"2014-11-13T19:26:55Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 13, 2014 at 11:15:27AM -0800, Junio C Hamano wrote:\n\n> > diff --git a/builtin/checkout.c b/builtin/checkout.c\n> > index 5410dac..67cab4e 100644\n> > --- a/builtin/checkout.c\n> > +++ b/builtin/checkout.c\n> > @@ -65,21 +65,40 @@ static int post_checkout_hook(struct commit *old, struct commit *new,\n> > static int update_some(const unsigned char *sha1, const char *base, int baselen,\n> > \t\tconst char *pathname, unsigned mode, int stage, void *context)\n> > {\n> > ...\n> > }\n> \n> Makes sense, including the use of strbuf (otherwise you would\n> allocate ce and then discard when it turns out that it is not\n> needed, which is probably with the same allocation pressure, but\n> looks uglier).\n\nExactly. Constructing it in ce->name does save you an allocation/memcpy\nin the case that we actually use the new entry, but I thought it would\nlook weirder. It probably doesn't matter much either way, so I tried to\nwrite the most obvious thing.\n\n(I also experimented with using make_cache_entry at one point, which\nrequires the strbuf; some of my thinking on what looks reasonable may be\nleft over from that approach).\n\n> > +test_expect_success 'do not touch files that are already up-to-date' '\n> > +\tgit reset --hard &&\n> > +\techo one >file1 &&\n> > +\techo two >file2 &&\n> > +\tgit add file1 file2 &&\n> > +\tgit commit -m base &&\n> > +\techo modified >file1 &&\n> > +\ttest-chmtime =1000000000 file2 &&\n> \n> Is the idea behind the hardcoded timestamp that this is sufficiently\n> old (Sep 2001) that we will not get in trouble comparing with the\n> real timestamp we get from the filesystem (which will definitely newer\n> than that anyway) no matter when we run this test (unless you have a\n> time-machine, that is)?\n\nI didn't actually calculate what the timestamp was. The important thing\nis that it is not the timestamp that your system would give to the file\nif git-checkout opened and rewrote it. :)\n\nI initially used \"123\", but was worried that would cause weird\nportability problems on systems. So I opted for something closer to\n\"normal\", but in the past. I think it is fine (modulo time machines),\nbut I'd be happy to put in some more obvious sentinel, too.\n\nAnd the worst case if you did have a time machine is that we might\naccidentally declare a buggy git to be correct (racily!). I can live\nwith that, but I guess you could use a relative value (like \"-10000\")\ninstead of a fixed sentinel (but then you'd have to record it for the\n\"expect\" check).\n\n-Peff\n"},{"id":"251845","messageId":"20141113200315.GA3869@peff.net","threadId":"37891","inReplyTo":"20141113192655.GA3413@peff.net","subject":"Re: [RFC] git checkout $tree -- $path always rewrites files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-11-13T20:03:16Z","receivedAt":"2014-11-13T20:03:16Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 13, 2014 at 02:26:55PM -0500, Jeff King wrote:\n\n> > Makes sense, including the use of strbuf (otherwise you would\n> > allocate ce and then discard when it turns out that it is not\n> > needed, which is probably with the same allocation pressure, but\n> > looks uglier).\n> \n> Exactly. Constructing it in ce->name does save you an allocation/memcpy\n> in the case that we actually use the new entry, but I thought it would\n> look weirder. It probably doesn't matter much either way, so I tried to\n> write the most obvious thing.\n\nActually, it is not that bad:\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 5410dac..5a78758 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -67,6 +67,7 @@ static int update_some(const unsigned char *sha1, const char *base, int baselen,\n {\n \tint len;\n \tstruct cache_entry *ce;\n+\tint pos;\n \n \tif (S_ISDIR(mode))\n \t\treturn READ_TREE_RECURSIVE;\n@@ -79,6 +80,23 @@ static int update_some(const unsigned char *sha1, const char *base, int baselen,\n \tce->ce_flags = create_ce_flags(0) | CE_UPDATE;\n \tce->ce_namelen = len;\n \tce->ce_mode = create_ce_mode(mode);\n+\n+\t/*\n+\t * If the entry is the same as the current index, we can leave the old\n+\t * entry in place. Whether it is UPTODATE or not, checkout_entry will\n+\t * do the right thing.\n+\t */\n+\tpos = cache_name_pos(ce->name, ce->ce_namelen);\n+\tif (pos >= 0) {\n+\t\tstruct cache_entry *old = active_cache[pos];\n+\t\tif (ce->ce_mode == old->ce_mode &&\n+\t\t !hashcmp(ce->sha1, old->sha1)) {\n+\t\t\told->ce_flags |= CE_UPDATE;\n+\t\t\tfree(ce);\n+\t\t\treturn 0;\n+\t\t}\n+\t}\n+\n \tadd_cache_entry(ce, ADD_CACHE_OK_TO_ADD | ADD_CACHE_OK_TO_REPLACE);\n \treturn 0;\n }\n\nand in some ways more readable, as you form the whole thing, and then as\nthe final step either add it, or realize that what is there is fine (I'd\nalmost wonder if it could be a flag to add_cache_entry).\n\n-Peff\n"},{"id":"251854","messageId":"xmqqmw7uztn0.fsf@gitster.dls.corp.google.com","threadId":"37891","inReplyTo":"20141113200315.GA3869@peff.net","subject":"Re: [RFC] git checkout $tree -- $path always rewrites files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-13T21:18:43Z","receivedAt":"2014-11-13T21:18:43Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Nov 13, 2014 at 02:26:55PM -0500, Jeff King wrote:\n>\n>> > Makes sense, including the use of strbuf (otherwise you would\n>> > allocate ce and then discard when it turns out that it is not\n>> > needed, which is probably with the same allocation pressure, but\n>> > looks uglier).\n>> \n>> Exactly. Constructing it in ce->name does save you an allocation/memcpy\n>> in the case that we actually use the new entry, but I thought it would\n>> look weirder. It probably doesn't matter much either way, so I tried to\n>> write the most obvious thing.\n>\n> Actually, it is not that bad:\n\nYeah, actually it does look better; want me to squash it into the\npatch before queuing?\n\n>\n> diff --git a/builtin/checkout.c b/builtin/checkout.c\n> index 5410dac..5a78758 100644\n> --- a/builtin/checkout.c\n> +++ b/builtin/checkout.c\n> @@ -67,6 +67,7 @@ static int update_some(const unsigned char *sha1, const char *base, int baselen,\n> {\n> \tint len;\n> \tstruct cache_entry *ce;\n> +\tint pos;\n> \n> \tif (S_ISDIR(mode))\n> \t\treturn READ_TREE_RECURSIVE;\n> @@ -79,6 +80,23 @@ static int update_some(const unsigned char *sha1, const char *base, int baselen,\n> \tce->ce_flags = create_ce_flags(0) | CE_UPDATE;\n> \tce->ce_namelen = len;\n> \tce->ce_mode = create_ce_mode(mode);\n> +\n> +\t/*\n> +\t * If the entry is the same as the current index, we can leave the old\n> +\t * entry in place. Whether it is UPTODATE or not, checkout_entry will\n> +\t * do the right thing.\n> +\t */\n> +\tpos = cache_name_pos(ce->name, ce->ce_namelen);\n> +\tif (pos >= 0) {\n> +\t\tstruct cache_entry *old = active_cache[pos];\n> +\t\tif (ce->ce_mode == old->ce_mode &&\n> +\t\t !hashcmp(ce->sha1, old->sha1)) {\n> +\t\t\told->ce_flags |= CE_UPDATE;\n> +\t\t\tfree(ce);\n> +\t\t\treturn 0;\n> +\t\t}\n> +\t}\n> +\n> \tadd_cache_entry(ce, ADD_CACHE_OK_TO_ADD | ADD_CACHE_OK_TO_REPLACE);\n> \treturn 0;\n> }\n>\n> and in some ways more readable, as you form the whole thing, and then as\n> the final step either add it, or realize that what is there is fine (I'd\n> almost wonder if it could be a flag to add_cache_entry).\n>\n> -Peff\n"},{"id":"251860","messageId":"20141113213719.GC7563@peff.net","threadId":"37891","inReplyTo":"xmqqmw7uztn0.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC] git checkout $tree -- $path always rewrites files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-11-13T21:37:19Z","receivedAt":"2014-11-13T21:37:19Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 13, 2014 at 01:18:43PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > On Thu, Nov 13, 2014 at 02:26:55PM -0500, Jeff King wrote:\n> >\n> >> > Makes sense, including the use of strbuf (otherwise you would\n> >> > allocate ce and then discard when it turns out that it is not\n> >> > needed, which is probably with the same allocation pressure, but\n> >> > looks uglier).\n> >> \n> >> Exactly. Constructing it in ce->name does save you an allocation/memcpy\n> >> in the case that we actually use the new entry, but I thought it would\n> >> look weirder. It probably doesn't matter much either way, so I tried to\n> >> write the most obvious thing.\n> >\n> > Actually, it is not that bad:\n> \n> Yeah, actually it does look better; want me to squash it into the\n> patch before queuing?\n\nYeah, if you like it, too, then let's go with it. Thanks.\n\n-Peff\n"},{"id":"251874","messageId":"20141114054440.GA54304@gmail.com","threadId":"37891","inReplyTo":"xmqqbnoge1ci.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC] git checkout $tree -- $path always rewrites files","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2014-11-14T05:44:41Z","receivedAt":"2014-11-14T05:44:41Z","isPatch":false,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Sun, Nov 09, 2014 at 09:21:49AM -0800, Junio C Hamano wrote:\n> Jeff King writes:\n> \n> > So just to be clear, the behavior we want is that:\n> >\n> > echo foo >some-new-path\n> > git add some-new-path\n> > git checkout HEAD -- .\n> >\n> > will delete some-new-path (whereas the current code turns it into an\n> > untracked file).\n> \n> With the updated semantics proposed in the old thread, yes, that is\n> what should happen.\n> \n> > git checkout HEAD -- some-new-path\n> >\n> > do in that case?\n> \n> Likewise. And if some-new-path were a directory, with existing path\n> O and new path N both in the index but only the former in HEAD, the\n> operation would revert some-new-path/O to that of HEAD and remove\n> some-new-path/N. That is the only logical thing we could do if we\n> were to take the updated sematics.\n> \n> That is one of the reasons why I am not 100% convinced that the\n> proposed updated semantics is better, even though I was fairly\n> positive in the old discussion and also I kept the topic in the\n> \"leftover bits\" list. The above command is a fairly common way to\n> say \"I started refactoring the existing path some-path/O and\n> sprinkled its original contents spread into new files A, B and C in\n> the same directory. Now I no longer have O in the working tree, but\n> let me double check by grabbing it out of the state recoded in the\n> commit\". You expect that \"git checkout HEAD -- some-path\" would not\n> lose A, B or C, knowing \"some-path\" only had O. That expectation\n> would even be stronger if you are used to the current semantics, but\n> that is something we could fix, if we decide that the proposed\n> updated semantics is better, with a careful transition plan.\n> \n> It might be less risky if the updated semantics were to make the\n> paths that are originally in the index but not in $tree untracked\n> (as opposed to \"reset --hard\" emulation where they will be lost)\n> unless they need to be removed to make room for D/F conflict issues,\n> but I haven't thought it through.\n\n\nGit has always been really careful to not lose data.\n\nOne way to avoid the problem of changing existing semantics is\nto make the new semantics accessible behind a flag, e.g.\n\"git checkout --hard HEAD -- some-new-path\".\n-- \nDavid\n"},{"id":"251897","messageId":"xmqqwq6x37ne.fsf@gitster.dls.corp.google.com","threadId":"37891","inReplyTo":"20141114054440.GA54304@gmail.com","subject":"Re: [RFC] git checkout $tree -- $path always rewrites files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-14T19:27:01Z","receivedAt":"2014-11-14T19:27:01Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Aguilar writes:\n\n> On Sun, Nov 09, 2014 at 09:21:49AM -0800, Junio C Hamano wrote:\n>> \n>> It might be less risky if the updated semantics were to make the\n>> paths that are originally in the index but not in $tree untracked\n>> (as opposed to \"reset --hard\" emulation where they will be lost)\n>> unless they need to be removed to make room for D/F conflict issues,\n>> but I haven't thought it through.\n>\n> Git has always been really careful to not lose data.\n>\n> One way to avoid the problem of changing existing semantics is\n> to make the new semantics accessible behind a flag, e.g.\n> \"git checkout --hard HEAD -- some-new-path\".\n\nYup, but you seem to be behind by a few exchanges, as we tentatively\ndecided that we won't talk about changing the semantics and concentrate\non fixing the implementation glitches only at least for now ;-)\n\nI find that \"--hard\" is not a very good name for the new mode.\nThere will be different kinds of \"more than what we usually do\"\nmodes of operations discovered over time in the coming years, and it\nis better to be more specific to denote \"in what way we are doing it\nharder\" (I think the difference the proposed new mode has is to also\ncheckout absense of the paths).\n\nBut in this particular case, making the paths that are absent in $tree\nwe are checking out of into untracked paths (instead of removing) is\na right balance of safety---it is similar to \"git reset HEAD\" (no\n\"--hard\") after adding a new path which leaves the file in the\nworking tree.\n"}]}