{"thread":{"id":"51714","subject":"[BUG] builtin \"stash apply\" does not refresh index","startedAt":"2019-08-23T05:27:40Z","lastAt":"2019-08-23T16:56:48Z","messageCount":2,"participants":["Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"380995","messageId":"20190823052737.GA10592@sigill.intra.peff.net","threadId":"51714","inReplyTo":null,"subject":"[BUG] builtin \"stash apply\" does not refresh index","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-08-23T05:27:37Z","receivedAt":"2019-08-23T05:27:40Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Try this:\n\n  # a repo with one file\n  git init repo\n  cd repo\n  echo base >file\n  git add file\n  git commit -m base\n\n  # now stash a modification\n  echo modified >file\n  git stash\n\n  # then make the file stat dirty. This will change inode; on systems\n  # without inodes, probably \"sleep 1; touch file\" would work.\n  cp file other\n  mv other file\n\n  # now try to apply the stash\n  git stash apply\n\nThe final command gives me:\n\n  error: Your local changes to the following files would be overwritten by merge:\n\tfile\n  Please commit your changes or stash them before you merge.\n  Aborting\n\nThis started with 8a0fc8d19d (stash: convert apply to builtin,\n2019-02-25), which is in v2.22.0. Interestingly, do_stash_apply() does\nin fact call refresh_cache(). But it looks like we don't ever write it\nout to disk. So when we later call merge_recursive(), it presumably uses\nthe on-disk index, not what we have in memory.\n\nIt's not clear to me where the fix should go, though. Should \"stash\napply\" be writing out the refreshed index before proceeding? Or should\nmerge_recursive() be more careful about refreshing the index after it\nlocks it? The former is what happened with stash as a shell script, but\nthe latter would save us an otherwise pointless write.\n\nI'm also not sure if other parts of stash might have a similar bug due\nto the conversion to C. I think it's an easy mistake when converting\n\"git update-index --refresh\" into \"refresh_cache()\" and then some other\npart of the code takes the index lock (and thus looks at that index, not\nwhat we have in memory).\n\n-Peff\n"},{"id":"381018","messageId":"xmqqa7bzn7le.fsf@gitster-ct.c.googlers.com","threadId":"51714","inReplyTo":"20190823052737.GA10592@sigill.intra.peff.net","subject":"Re: [BUG] builtin \"stash apply\" does not refresh index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-08-23T16:56:45Z","receivedAt":"2019-08-23T16:56:48Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> This started with 8a0fc8d19d (stash: convert apply to builtin,\n> 2019-02-25), which is in v2.22.0. Interestingly, do_stash_apply() does\n> in fact call refresh_cache(). But it looks like we don't ever write it\n> out to disk. So when we later call merge_recursive(), it presumably uses\n> the on-disk index, not what we have in memory.\n\nOuch.\n\n> It's not clear to me where the fix should go, though. Should\n> \"stash apply\" be writing out the refreshed index before\n> proceeding? Or should merge_recursive() be more careful about\n> refreshing the index after it locks it?  The former is what\n> happened with stash as a shell script, but the latter would save\n> us an otherwise pointless write.\n\nI wonder if the third solution be possible.  \"I am a new caller of\nmerge-recursive machinery, I did some operations on the working tree\nand the index (in core), and while I would, in the old world order,\nwrite out the in-core index to on-disk and call the merge-recursive\nmachinery, expecing the callee to read the on-disk index, for the\nsake of performance, I am using this new calling convention, where a\nnew entry point of merge-recursivey machinery allows me to call it\nand tell it to use the in-core index.\"  For that to happen, the\ncodepath in the merge-recursive machinery entered from the new entry\npoint needs to learn not only omit a call to read_cache() but also\nit needs to write in-core index out lazily for the caller (e.g.\nimagine it needs to exit early without doing any of the merge?  The\ncaller skipped the \"otherwise pointless write\", which needs to now\nhappen before the whole thing exits).\n\nAnd before the third one, introduction of a new entry point that\nmakes merge-recursive machinery inherit the already populated\nin-core index, happens, I think the right solution is to write the\nin-core index out---the write is not pointless.\n\n\n\n\n\n"}]}