patchguilt: Make sure the commit time is increasing

Git has various algorithms, most notably in git rev-list, git name-rev, and others, which depend on the commit time increasing. We want to keep the commit time the same as much as possible, but if necessary, adjust the time stamps of the patch files to obey this constraint.

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
---
 guilt |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

On Sun, Jul 04, 2010 at 10:23:59PM -0400, Theodore Ts'o wrote:

                                                    ^^^^^^^^^^^^^

hmm, I just realized, this is strictly speaking not necessary.

"stat -c %Y" means that guilt only works if GNU coreutils is installed, which means that "touch -d @secs-since-epoch" should also work.

This will be a problem on legacy systems such as Solaris (unless their path puts the GNU utilities head of their System V-style utilities), but that's going to be true of guilt in general, it looks like.

      	    	    	     	      - Ted

On Sun, Jul 04, 2010 at 10:51:17PM -0400, tytso@mit.edu wrote:

I've been meaning to make guilt less GNU-dependant, but I just don't use non-Linux systems enough (read: almost never) to do it myself.

Jeff.

On Sun, Jul 04, 2010 at 10:23:59PM -0400, Theodore Ts'o wrote:

Am I understanding this right? You want the timestamps to be monotonically increasing? Is the +60 the most obvious choice?

Can I get an example of how git can get confused?

Josef 'Jeff' Sipek.

On Jul 4, 2010, at 10:59 PM, jeffpc@josefsipek.net wrote:

Yup, that's correct. In more modern versions of git most (all?) of the places that depend on the committer time of the child commit to be greater than the committer time of its parents have been relaxed to accept up to a day's worth of clock skew, but in the interests of "be conservative in what you send", strictly increasing seemed like the best thing to do.

It's somewhat arbitrary. I figured a minute increase between commits was more aesthetically pleasing than a second, 5 minutes, or an hour, which were some other deltas that previous versions of my patch used while I was experimenting.

This first one is explicitly my/guilt's fault (and it's when I learned that I was causing problems by how I was using guilt in the ext4 tree):

http://kerneltrap.org/mailarchive/git/2010/4/22/28928/thread

In this thread we see how the clock skew gets in the way of an optimization that speeds up "git tag --contains" by over two orders of magnitude, but it gets screwed over by extreme clock skew. I suggested in that thread that if git is going to depend on it, then maybe "git commit" should either warn or error out if the git committer timestamp goes backwards --- and that's when I decided maybe I should offer up a patch to guilt to fix this, either before or instead of fixing up "git commit" to throw a warning/error:

http://www.spinics.net/lists/git/msg134307.html

Other threads:

http://kerneltrap.org/mailarchive/git/2010/4/8/27731/thread http://www.kerneltrap.com/mailarchive/git/2007/5/24/247375

-- Ted

On Mon, Jul 05, 2010 at 07:06:45AM -0400, Theodore Tso wrote:

Alright, makes sense.

I think we might need a little bit more logic in this patch...

if I commit, and immediately after push 10 patches, wouldn't the HEAD end up with a commit that's ~10 minutes in the future?

I do like the idea of git-commit warning/erroring, but I don't think that guilt issuing a warning is necessary. Afterall, it's only a timestamp change. It might be a bit of a shock for anyone looking at the timestamps expecting them to be out of order (based on the patch times), but I think it's better than warning all the time.

Jeff.

On Mon, Jul 05, 2010 at 02:52:38PM -0400, jeffpc@josefsipek.net wrote:

Hmm, good point. I hadn't considered that case. The most common case happens when I rebase to a new release, and then do a "guilt push -a". In that case the time that start with isn't "now", but whenver the last release happened, which is typically far enough in the past that we don't end up in the future. However, I agree that's a concern.

How about this?

I guess I didn't worry too much since "guilt push -a" is pretty noisy anyway. I've shortened the message, but if you think it's better to pull the message altogether feel free...

						- Ted

From: Theodore Ts'o <tytso@mit.edu>
Date: Sun, 4 Jul 2010 22:06:08 -0400
Subject: [PATCH] guilt: Make sure the commit time is increasing

Git has various algorithms, most notably in git rev-list, git name-rev, and others, which depend on the commit time increasing. We want to keep the commit time the same as much as possible, but if necessary, adjust the time stamps of the patch files to obey this constraint.

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
---
 guilt |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

tytso@mit.edu wrote:

I don’t think git has ever required commit dates to be _strictly_ monotonic.

At one point rev-list did require monotonic --- i.e., the committer date of each commit had to be equal to or later than that of each of its parents) with no clock skew but that was considered a bug and fixed by v1.5.5-rc1~16 (Make revision limiting more robust against occasional bad commit dates, 2008-03-17)

So I would suggest

 echo "Adjusting mod time of $(basename "$p")"
 touch -d "$ct" "$p"

If the parent commit time happens to be in the future, well, at least we’re not making it worse.

By the way, I think your idea to have commit warn about nonmonotonic commit dates is a good one. We should also decide on a rule, hopefully one the kernel repo obeys (30 days max skew? *crosses fingers*) and make git fsck warn loudly about violations.

On Jul 6, 2010, at 4:03 AM, Jonathan Nieder wrote:

You're right that it's been a while since git has run into problems with mild forms of clock skew (even Debian Stable is shipping v1.5.6) but I think it's better to times in the future if we can at all help it, and it's not like we're talking about a lot of extra complexity to guilt to test for this.

Having git commit warn, absolutely.

Unfortunately, there is already ~100 days of skew (see the earlier discusion on this thread by Jeff King) in the Linux kernel repo already...

-- Ted

Theodore Tso wrote:

Sorry, I’m a little lost. There are five phenomena one could forbid:

 1. Commits with timestamp equal to or before a parent
 2. Commits with timestamp before a parent
 3. Commits with timestamp unreasonably long before a parent
 4. Commits with timestamp unreasonably long before an ancestor
 5. Commits with timestamp in the future

Git has always been able to cope with #5 (timestamps in the future). I see no reason to avoid it, except that it is hard to assign a timestamp for commits on top of that one.

Git’s problem today is #4 (long-term slop). Maybe as Jeff suggested "unreasonably long" should defined per repository. Or we could measure the kernel’s maximum (something like 120 days?) and make that a hard limit.

Do #3 a few times, and you get #4. So ‘commit’ should warn about it (where ‘unreasonably long’ could be as short as 0 or 1 days).

#2 (nonmonotonic commits) was broken in ancient git; I think it’s too rigid of a rule to worry about it on that account. But a variant of the rationale for avoiding #3 applies to it.

I have never heard of any version of Git copying poorly with #1 (commits with the same timestamp). Avoiding it artificially leads inevitably to timestamps in the future when you somehow try to assign 100 timestamps for the series you have rebased on top of a patch committed a few seconds ago.

Incrementing the timestamp to ensure strictly monotonic commits seems like a recipe for trouble to me.

For guilt, I think the best thing to do would to save a Date: line for the author date with the From: and Subject: and then touch patches with the _current_ date when appropriate to avoid skew.

HTH, Jonathan

On Tue, Jul 06, 2010 at 10:09:17AM -0500, Jonathan Nieder wrote:

Um, I'm guessing you spent a lot of time typing your note, but not a lot of time looking at my most recent patch? My most recent patch for guilt simply will set the time of the patch to the current time to avoid setting it into the future.

      	      				- Ted

tytso@mit.edu wrote:

I read it, and I did not like that specific part.  I even responded to it.

I guess "leads inevitably to timestamps in the future" was a poor choice of words, though.

On Tue, Jul 6, 2010 at 10:03 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:

This might be a stupid question, but I'm not entirely clear on why it's not a strict requirement; surely it would be easy to ensure that the commit-time is at least as big as the parents when generating the commit...?

Is it to avoid the case where a user commits with the clock set to some point (potentially far) in the future, so all subsequent commits would have the same, artificially high commit time? Or is there some other reason I can't think of?

On Tue, Jul 06, 2010 at 03:53:56PM +0200, Erik Faye-Lund wrote:

You can have clock skew between distributed developers. So imagine you commit at 5:00pm, then I pull at 5:01pm, but it turns out your clock is two minutes fast, so it's actually 4:59pm.

What should my commit do? If I insist on monotonic increases, then my clock gets pushed forward artificially by your fast, broken clock (which is probably not the end of the world; in practice, if your clock is N seconds fast, there will presumably be some N second period where I'm not making a commit, and the clocks can "catch up" with each other).

-Peff

On Tue, Jul 6, 2010 at 4:29 PM, Jeff King <peff@peff.net> wrote:

Yeah, but this doesn't really answer my question; as you're saying, it's probably not the end of the world, at least when the skew is low.

But I can imagine it becoming a big deal when the skew is high. The again, perhaps this should constitute a "bad commit" and commit should error out if a parent commit was more than some number of minutes newer than the current time (or whatever)? That way, skewed commits would be caught early if a developer is working with other people, and a lot of the traversal could perhaps be faster (or more robust). If the developer with the skewed clock doesn't work with anyone, skew isn't really a problem, but perhaps he'd have to do some branch-filtering to un-skew commits when starting to work with others. And only if the skew is really high... like, multiple days... Which can't really be THAT common?

However, turning a technical problem that already have a solution that seems to work for everyone into a social one might be a bad idea. I'm really just thinking out loud here :)

On Tue, Jul 06, 2010 at 05:02:51PM +0200, Erik Faye-Lund wrote:

Guilt uses the modtime of the patch in a patch series for the committer time and the author time. The reasoning behind it doing this is so that you can do "git pop -a" followed by "git push -a" and if the patch files haven't changed, the commit id's don't change either.

But if you change a commit in the middle of the series, you can end up with clock skews that could be several days or weeks. Becuase of my ext4 workflow, the Linux kernel has a maximum skew of 100 days. Mea culpa; I stopped doing this as soon as I was told that git was depending on committer time being roughly increasing, and so I at least haven't introduced any such time skews since v2.6.34. And part of my making up for this has been to submit a patch to guilt to prevent this from happening again in the future, by fixing up guilt so that it won't request "git commit" to create timestamps that show very wild clock skews within a single linear branch.

We could still get potentially screwed though. Every so often I will see someone sending e-mail from a client host whose time is years if not decades in the past or in the future. If they were to do a "git commit", and then push that commit to a public repository, we could easily introduce a large clock skew into a git repo. Has that ever happened to date? Not to my knowledge. Could it happen? Very clearly, yes. Should we try to put in some safety checks to prevent it, or at least issue warnings? Maybe.

						- Ted

On Mon, Jul 05, 2010 at 03:22:01PM -0400, tytso@mit.edu wrote: ...

I'm going to play with this patch locally a little bit, but I'm all for it.

Speaking of weird timestamps...2001?  Where did that come from? :)

Depending on how my playing goes, I might remove the echo.

So, ct is now the +1min time.

And we're touching +1+1min.  I'll fix it up before applying.

Thanks,

Jeff.

On Tue, Jul 06, 2010 at 01:29:54PM -0400, jeffpc@josefsipek.net wrote:

Yup, I forgot to take the "+ 60" out.  Thanks for catching that.

					- Ted

FWIW, I pushed the change out. I think this is a major enough fix that I'll cut a new release soon.

Thanks!

Jeff.