{"thread":{"id":"53225","subject":"[PATCH 1/1] freshen_file(): use NULL `times' for implicit current-time","startedAt":"2020-04-14T15:23:26Z","lastAt":"2020-04-16T01:02:25Z","messageCount":8,"participants":["luciano.rocha@booking.com","Jeff King","Junio C Hamano","brian m. carlson"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"395417","messageId":"5e95d37d.1c69fb81.2b4ec.ce9f@mx.google.com","threadId":"53225","inReplyTo":null,"subject":"[PATCH 1/1] freshen_file(): use NULL `times' for implicit current-time","fromName":"","fromEmail":"luciano.rocha@booking.com","sentAt":"2020-04-14T14:27:26Z","receivedAt":"2020-04-14T15:23:26Z","isPatch":true,"sender":{"key":"luciano.rocha@booking.com","avatar":"https://avatars.githubusercontent.com/u/105658188?v=4"},"body":"Update freshen_file() to use a NULL `times', semantically equivalent to\nthe currently setup, with an explicit `actime' and `modtime' set to the\n\"current time\", but with the advantage that it works with other files\nnot owned by the current user.\n\nFixes an issue on shared repos with a split index, where eventually a\nuser's operation creates a shared index, and another user will later do\nan operation that will try to update its freshness, but will instead\nraise a warning:\n $ git status\n warning: could not freshen shared index '.git/sharedindex.bd736fa10e0519593fefdb2aec253534470865b2'\n\nSigned-off-by: Luciano Miguel Ferreira Rocha \n---\n sha1-file.c | 4 +---\n 1 file changed, 1 insertion(+), 3 deletions(-)\n\ndiff --git a/sha1-file.c b/sha1-file.c\nindex 6926851724..ccd34dd9e8 100644\n--- a/sha1-file.c\n+++ b/sha1-file.c\n@@ -881,9 +881,7 @@ void prepare_alt_odb(struct repository *r)\n /* Returns 1 if we have successfully freshened the file, 0 otherwise. */\n static int freshen_file(const char *fn)\n {\n-\tstruct utimbuf t;\n-\tt.actime = t.modtime = time(NULL);\n-\treturn !utime(fn, &t);\n+\treturn !utime(fn, NULL);\n }\n \n /*\n-- \n2.26.0\n\n"},{"id":"395434","messageId":"20200414195535.GA1879302@coredump.intra.peff.net","threadId":"53225","inReplyTo":"5e95d37d.1c69fb81.2b4ec.ce9f@mx.google.com","subject":"Re: [PATCH 1/1] freshen_file(): use NULL `times' for implicit current-time","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-04-14T19:55:35Z","receivedAt":"2020-04-14T19:57:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 14, 2020 at 04:27:26PM +0200, luciano.rocha@booking.com wrote:\n\n> Update freshen_file() to use a NULL `times', semantically equivalent to\n> the currently setup, with an explicit `actime' and `modtime' set to the\n> \"current time\", but with the advantage that it works with other files\n> not owned by the current user.\n> \n> Fixes an issue on shared repos with a split index, where eventually a\n> user's operation creates a shared index, and another user will later do\n> an operation that will try to update its freshness, but will instead\n> raise a warning:\n> $ git status\n> warning: could not freshen shared index '.git/sharedindex.bd736fa10e0519593fefdb2aec253534470865b2'\n\nThanks, this makes sense. And as a bonus, it's less code. ;)\n\nI don't recall having any particular reason not to use NULL in the\noriginal (I may simply not have been aware it was an option), and I\ncan't find any discussion either way.\n\nOne observation:\n\n> static int freshen_file(const char *fn)\n> {\n> -\tstruct utimbuf t;\n> -\tt.actime = t.modtime = time(NULL);\n> -\treturn !utime(fn, &t);\n> +\treturn !utime(fn, NULL);\n> }\n\nThe old code was setting the time based on the system time from time().\nWe've occasionally hit cases where the filesystem time and the system\ntime do not match exactly (this might be true on an NFS mount, for\nexample).\n\nIt's not clear to me whether utime(NULL) would be using the system or\nfilesystem time in such a case. If the former, then there's no change in\nbehavior. If the latter, I'd argue that it's probably an _improvement_,\nsince we're simulating the case that we wrote a new file with a new\nmtime.\n\n-Peff\n"},{"id":"395481","messageId":"20200415090906.wudutxpiyw7ooas6@C02Z11TTLVDN.corpad.adbkng.com","threadId":"53225","inReplyTo":"20200414195535.GA1879302@coredump.intra.peff.net","subject":"Re: [External] Re: [PATCH 1/1] freshen_file(): use NULL `times' for implicit current-time","fromName":"","fromEmail":"luciano.rocha@booking.com","sentAt":"2020-04-15T09:09:06Z","receivedAt":"2020-04-15T09:16:37Z","isPatch":true,"sender":{"key":"luciano.rocha@booking.com","avatar":"https://avatars.githubusercontent.com/u/105658188?v=4"},"body":"On Tue, Apr 14, 2020 at 03:55:35PM -0400, Jeff King wrote:\n> On Tue, Apr 14, 2020 at 04:27:26PM +0200, luciano.rocha@booking.com wrote:\n> \n> > Update freshen_file() to use a NULL `times', semantically equivalent to\n> > the currently setup, with an explicit `actime' and `modtime' set to the\n> > \"current time\", but with the advantage that it works with other files\n> > not owned by the current user.\n> > \n> > Fixes an issue on shared repos with a split index, where eventually a\n> > user's operation creates a shared index, and another user will later do\n> > an operation that will try to update its freshness, but will instead\n> > raise a warning:\n> > $ git status\n> > warning: could not freshen shared index '.git/sharedindex.bd736fa10e0519593fefdb2aec253534470865b2'\n> \n> Thanks, this makes sense. And as a bonus, it's less code. ;)\n> \n> I don't recall having any particular reason not to use NULL in the\n> original (I may simply not have been aware it was an option), and I\n> can't find any discussion either way.\n> \n> One observation:\n> \n> > static int freshen_file(const char *fn)\n> > {\n> > -\tstruct utimbuf t;\n> > -\tt.actime = t.modtime = time(NULL);\n> > -\treturn !utime(fn, &t);\n> > +\treturn !utime(fn, NULL);\n> > }\n> \n> The old code was setting the time based on the system time from time().\n> We've occasionally hit cases where the filesystem time and the system\n> time do not match exactly (this might be true on an NFS mount, for\n> example).\n> \n> It's not clear to me whether utime(NULL) would be using the system or\n> filesystem time in such a case. If the former, then there's no change in\n> behavior. If the latter, I'd argue that it's probably an _improvement_,\n> since we're simulating the case that we wrote a new file with a new\n> mtime.\n\nI'm not that familiar with kernel code, so can't say for sure. From a\ncursory look, it doesn't seem like it uses the remote server's time.\n\nBut it does seem to have a higher precision, for filesystems that\nsupport it.\n\nIn Linux, it ends up calling current_time(inode), which says:\n * Return the current time truncated to the time granularity supported by\n * the fs.\n\nAnd uses ktime_get_coarse_real_ts64(). That could explain any previous\ncase where utime() of time(NULL) was different that just utime() or an\neffect from writing to a file.\n\nArguably, even more of an improvement if it gives higher resolution.\n\nRegards,\n\n-- \nLuciano Rocha\n"},{"id":"395513","messageId":"20200415160535.GF2464307@coredump.intra.peff.net","threadId":"53225","inReplyTo":"20200415090906.wudutxpiyw7ooas6@C02Z11TTLVDN.corpad.adbkng.com","subject":"Re: [External] Re: [PATCH 1/1] freshen_file(): use NULL `times' for implicit current-time","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-04-15T16:05:35Z","receivedAt":"2020-04-15T16:05:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 15, 2020 at 11:09:06AM +0200, luciano.rocha@booking.com wrote:\n\n> > The old code was setting the time based on the system time from time().\n> > We've occasionally hit cases where the filesystem time and the system\n> > time do not match exactly (this might be true on an NFS mount, for\n> > example).\n> > \n> > It's not clear to me whether utime(NULL) would be using the system or\n> > filesystem time in such a case. If the former, then there's no change in\n> > behavior. If the latter, I'd argue that it's probably an _improvement_,\n> > since we're simulating the case that we wrote a new file with a new\n> > mtime.\n> \n> I'm not that familiar with kernel code, so can't say for sure. From a\n> cursory look, it doesn't seem like it uses the remote server's time.\n> \n> But it does seem to have a higher precision, for filesystems that\n> support it.\n\nYeah, that's another point in its favor.\n\nIt seems pretty clear to me that utime(NULL) should give either the same\nor better behavior in all cases.\n\n-Peff\n"},{"id":"395521","messageId":"xmqq7dyg7j2x.fsf@gitster.c.googlers.com","threadId":"53225","inReplyTo":"20200415160535.GF2464307@coredump.intra.peff.net","subject":"Re: [External] Re: [PATCH 1/1] freshen_file(): use NULL `times' for implicit current-time","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-15T17:30:14Z","receivedAt":"2020-04-15T17:30:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Wed, Apr 15, 2020 at 11:09:06AM +0200, luciano.rocha@booking.com wrote:\n>\n>> > The old code was setting the time based on the system time from time().\n>> > We've occasionally hit cases where the filesystem time and the system\n>> > time do not match exactly (this might be true on an NFS mount, for\n>> > example).\n>> > \n>> > It's not clear to me whether utime(NULL) would be using the system or\n>> > filesystem time in such a case. If the former, then there's no change in\n>> > behavior. If the latter, I'd argue that it's probably an _improvement_,\n>> > since we're simulating the case that we wrote a new file with a new\n>> > mtime.\n>> \n>> I'm not that familiar with kernel code, so can't say for sure. From a\n>> cursory look, it doesn't seem like it uses the remote server's time.\n>> \n>> But it does seem to have a higher precision, for filesystems that\n>> support it.\n>\n> Yeah, that's another point in its favor.\n>\n> It seems pretty clear to me that utime(NULL) should give either the same\n> or better behavior in all cases.\n\nYup, thanks Luciano for the patch and Peff for a (n always) good\nreview.\n\n"},{"id":"395539","messageId":"xmqq4ktk5t4h.fsf@gitster.c.googlers.com","threadId":"53225","inReplyTo":"5e95d37d.1c69fb81.2b4ec.ce9f@mx.google.com","subject":"Re: [PATCH 1/1] freshen_file(): use NULL `times' for implicit current-time","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-15T21:36:14Z","receivedAt":"2020-04-15T21:36:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"luciano.rocha@booking.com writes:\n\n> Update freshen_file() to use a NULL `times', semantically equivalent to\n> the currently setup, with an explicit `actime' and `modtime' set to the\n> \"current time\", but with the advantage that it works with other files\n> not owned by the current user.\n>\n> Fixes an issue on shared repos with a split index, where eventually a\n> user's operation creates a shared index, and another user will later do\n> an operation that will try to update its freshness, but will instead\n> raise a warning:\n> $ git status\n> warning: could not freshen shared index '.git/sharedindex.bd736fa10e0519593fefdb2aec253534470865b2'\n\nA couple of questions:\n\n - Does utime(fn, NULL) work for any non-owner user, or does the\n user need to have write access to it?\n\n - If the answer is not \"you need to be able to write\", doesn't the\n bug lie elsewhere, namely, why .git/sharedindex.* not writable by\n the current user, if it is a shared repository setting?\n\nThanks.\n\n> Signed-off-by: Luciano Miguel Ferreira Rocha \n> ---\n> sha1-file.c | 4 +---\n> 1 file changed, 1 insertion(+), 3 deletions(-)\n>\n> diff --git a/sha1-file.c b/sha1-file.c\n> index 6926851724..ccd34dd9e8 100644\n> --- a/sha1-file.c\n> +++ b/sha1-file.c\n> @@ -881,9 +881,7 @@ void prepare_alt_odb(struct repository *r)\n> /* Returns 1 if we have successfully freshened the file, 0 otherwise. */\n> static int freshen_file(const char *fn)\n> {\n> -\tstruct utimbuf t;\n> -\tt.actime = t.modtime = time(NULL);\n> -\treturn !utime(fn, &t);\n> +\treturn !utime(fn, NULL);\n> }\n> \n> /*\n"},{"id":"395552","messageId":"20200415234847.GI2751707@camp.crustytoothpaste.net","threadId":"53225","inReplyTo":"xmqq4ktk5t4h.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 1/1] freshen_file(): use NULL `times' for implicit current-time","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-04-15T23:48:47Z","receivedAt":"2020-04-15T23:49:00Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2020-04-15 at 21:36:14, Junio C Hamano wrote:\n> luciano.rocha@booking.com writes:\n> \n> > Update freshen_file() to use a NULL `times', semantically equivalent to\n> > the currently setup, with an explicit `actime' and `modtime' set to the\n> > \"current time\", but with the advantage that it works with other files\n> > not owned by the current user.\n> >\n> > Fixes an issue on shared repos with a split index, where eventually a\n> > user's operation creates a shared index, and another user will later do\n> > an operation that will try to update its freshness, but will instead\n> > raise a warning:\n> > $ git status\n> > warning: could not freshen shared index '.git/sharedindex.bd736fa10e0519593fefdb2aec253534470865b2'\n> \n> A couple of questions:\n> \n> - Does utime(fn, NULL) work for any non-owner user, or does the\n> user need to have write access to it?\n\nThe Linux man page says the following:\n\n Changing timestamps is permitted when: either the process has\n appropriate privileges, or the effective user ID equals the user ID of\n the file, or times is NULL and the process has write permission for\n the file.\n\nSo the answer is the user needs to have write access with utime(fn,\nNULL), but the same EUID (or root) with a specific time. I believe this\nbehavior is because it doesn't make sense to restrict the operation\nwhich uses the current time since a user with write permissions could\njust run open(2) and write(2) instead with the same effect, just less\nefficiently.\n\n(We've discussed this on the list before, but \"appropriate privileges\"\nis POSIX-speak for \"root access\" or the equivalent in an alternate\nsystem, such as capabilities.)\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"395558","messageId":"xmqqa73c4510.fsf@gitster.c.googlers.com","threadId":"53225","inReplyTo":"20200415234847.GI2751707@camp.crustytoothpaste.net","subject":"Re: [PATCH 1/1] freshen_file(): use NULL `times' for implicit current-time","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-16T01:02:03Z","receivedAt":"2020-04-16T01:02:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" writes:\n\n> So the answer is the user needs to have write access with utime(fn,\n> NULL), but the same EUID (or root) with a specific time.\n\nOK, with a specific time, write access is not sufficient, and that\nis why this patch helps.\n\nMakes perfect sense. Thanks for a dose of sanity ;-)\n"}]}