{"thread":{"id":"40724","subject":"File owner/group and git","startedAt":"2015-11-05T02:03:57Z","lastAt":"2015-11-05T17:57:00Z","messageCount":3,"participants":["David Turner","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"272927","messageId":"1446689037.4131.47.camel@twopensource.com","threadId":"40724","inReplyTo":null,"subject":"File owner/group and git","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-11-05T02:03:57Z","receivedAt":"2015-11-05T02:03:57Z","isPatch":false,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"In unpack-trees.c, in verify_uptodate_1, we check ie_match_stat. This\nreturns OWNER_CHANGED if a file has changed ownership since the index\nwas updated. Do we actually care about that particular case? Or really\nanything other than DATA_CHANGED?\n\n(We noticed this due to a bug in our watchman code, which fakes up the\nowners, but it's a general question).\n"},{"id":"272929","messageId":"xmqqoaf9891r.fsf@gitster.mtv.corp.google.com","threadId":"40724","inReplyTo":"1446689037.4131.47.camel@twopensource.com","subject":"Re: File owner/group and git","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-11-05T02:38:40Z","receivedAt":"2015-11-05T02:38:40Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Turner writes:\n\n> In unpack-trees.c, in verify_uptodate_1, we check ie_match_stat. This\n> returns OWNER_CHANGED if a file has changed ownership since the index\n> was updated. Do we actually care about that particular case? Or really\n> anything other than DATA_CHANGED?\n\nThat's a 10-year old code and there aren't that many people left\nwho can answer the original rationale, I am afraid ;-)\n\nIn general, \"Do we actually care?\" is not the question we ask in\nthis area of the code. \"Does it help us to catch real changes, or\ndoes it change spuriously to make it too unreliable a signal to be\nuseful?\" is the question that drives the design of this part of the\nsystem.\n\nDATA_CHANGED is \"we know the contents are different without even\nlooking at the data\". If the size is different from the last time\nwe hashed the data, the contents must have changed. The inverse is\nnot true (and that is half of the \"racy git\" issue).\n\nOther *_CHANGED are finely classified only because originally we\ndidn't really know which are useful to treat as notable change\nevent, and \"changed\" variable had sufficient number of bits to hold\ndifferent classification, so that we could pick and choose which\nones we truly care. We knew MTIME was useful in the sense that even\nif the size is the same, updated mtime is good enough indication\nthat the stuff has changed, even to \"make\" utility.\n\nINODE and CTIME are not so stable on some filesystems (e.g. inum may\nnot be stable on a network share across remount) and in some\nenvironments (e.g. some virus scanners touch ctime to mark scanned\nfiles, cf. 1ce4790b), and would trigger false positives too often to\nbe useful. We always paid attention to them initially, but there\nare configurations to tell Git not raise them these days.\n\nOWNER probably falls into a category that is stable enough to be\nuseful, as the most likely way for it to change is not by running\n\"chown\" on the file in-place (which does not change the contents),\nbut by running \"mv\" to drop another file owned by somebody else to\nthe original location (which likely does change the contents). At\nthe same time, \"mv\" a different file into the path would likely\ntrigger changes to INODE and MTIME as well, so it cannot be more\nthan belt-and-suspenders measure to catch modification. In that\nsense ignoring OWNER would not hurt too much.\n\nIf it changes spuriously to make it too unreliable a signal to be\nuseful, it certainly is OK to introduce a knob to ignore it. It\nmight even make sense to ignore it unconditionally if the false hit\nhappens too frequently, but offhand my gut reaction is that there\nmay be something wrong in the environment (i.e. system outside Git\nin which Git runs) if owner/group changes spuriously to cause\nissues.\n"},{"id":"272961","messageId":"1446746220.4131.57.camel@twopensource.com","threadId":"40724","inReplyTo":"xmqqoaf9891r.fsf@gitster.mtv.corp.google.com","subject":"Re: File owner/group and git","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-11-05T17:57:00Z","receivedAt":"2015-11-05T17:57:00Z","isPatch":false,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Wed, 2015-11-04 at 18:38 -0800, Junio C Hamano wrote:\n> David Turner writes:\n> \n> > In unpack-trees.c, in verify_uptodate_1, we check ie_match_stat. This\n> > returns OWNER_CHANGED if a file has changed ownership since the index\n> > was updated. Do we actually care about that particular case? Or really\n> > anything other than DATA_CHANGED?\n> \n> That's a 10-year old code and there aren't that many people left\n> who can answer the original rationale, I am afraid ;-)\n> \n> In general, \"Do we actually care?\" is not the question we ask in\n> this area of the code. \"Does it help us to catch real changes, or\n> does it change spuriously to make it too unreliable a signal to be\n> useful?\" is the question that drives the design of this part of the\n> system.\n> \n> DATA_CHANGED is \"we know the contents are different without even\n> looking at the data\". If the size is different from the last time\n> we hashed the data, the contents must have changed. The inverse is\n> not true (and that is half of the \"racy git\" issue).\n> \n> Other *_CHANGED are finely classified only because originally we\n> didn't really know which are useful to treat as notable change\n> event, and \"changed\" variable had sufficient number of bits to hold\n> different classification, so that we could pick and choose which\n> ones we truly care. We knew MTIME was useful in the sense that even\n> if the size is the same, updated mtime is good enough indication\n> that the stuff has changed, even to \"make\" utility.\n> \n> INODE and CTIME are not so stable on some filesystems (e.g. inum may\n> not be stable on a network share across remount) and in some\n> environments (e.g. some virus scanners touch ctime to mark scanned\n> files, cf. 1ce4790b), and would trigger false positives too often to\n> be useful. We always paid attention to them initially, but there\n> are configurations to tell Git not raise them these days.\n> \n> OWNER probably falls into a category that is stable enough to be\n> useful, as the most likely way for it to change is not by running\n> \"chown\" on the file in-place (which does not change the contents),\n> but by running \"mv\" to drop another file owned by somebody else to\n> the original location (which likely does change the contents). At\n> the same time, \"mv\" a different file into the path would likely\n> trigger changes to INODE and MTIME as well, so it cannot be more\n> than belt-and-suspenders measure to catch modification. In that\n> sense ignoring OWNER would not hurt too much.\n> \n> If it changes spuriously to make it too unreliable a signal to be\n> useful, it certainly is OK to introduce a knob to ignore it. It\n> might even make sense to ignore it unconditionally if the false hit\n> happens too frequently, but offhand my gut reaction is that there\n> may be something wrong in the environment (i.e. system outside Git\n> in which Git runs) if owner/group changes spuriously to cause\n> issues.\n\nThanks.\n\nThe only case where we saw it was with our watchman code, which lies\nabout ownership (to save space/time). We're going to try ignoring\nOWNER_CHANGED in our watchman branch, and if that fixes the issue for\nour users, we'll stop worrying about it on the theory that Duy's\nwatchman stuff is the long-term path forward, and it doesn't have this\nissue.\n"}]}