[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 |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/guilt b/guilt
index b6e2a6c..2371e98 100755
--- a/guilt
+++ b/guilt

@@ -535,6 +535,13 @@ commit()
                         export GIT_AUTHOR_EMAIL="`echo $author_str | sed -e 's/[^<]*//'`"
 		fi
 
+		ct=$(git log -1 --pretty=%ct)
+		if [ $ct -gt $(stat -c %Y "$p") ]; then
+		    echo "Warning time went backwards, adjusting mod time of" \
+			$(basename "$p")
+		    touch -d @$(expr $ct + 60) "$p" || touch "$p"
+		fi
+
 		# must strip nano-second part otherwise git gets very
 		# confused, and makes up strange timestamps from the past
 		# (chances are it decides to interpret it as a unix

-- 
1.7.0.4

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

+		ct=$(git log -1 --pretty=%ct)
+		if [ $ct -gt $(stat -c %Y "$p") ]; then
+		    echo "Warning time went backwards, adjusting mod time of" \
+			$(basename "$p")
+		    touch -d @$(expr $ct + 60) "$p" || touch "$p"

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

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:23:59PM -0400, Theodore Ts'o wrote:

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.

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.

-- 
UNIX is user-friendly ... it's just selective about who its friends are

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

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

quoted

+		ct=$(git log -1 --pretty=%ct)
+		if [ $ct -gt $(stat -c %Y "$p") ]; then
+		    echo "Warning time went backwards, adjusting mod time of" \
+			$(basename "$p")
+		    touch -d @$(expr $ct + 60) "$p" || touch "$p"

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

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.

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.

-- 
The box said "Windows XP or better required". So I installed Linux.

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

Am I understanding this right?  You want the timestamps to be monotonically
increasing?  

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.

Is the +60 the most obvious choice?

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.

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

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:

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

quoted

Am I understanding this right?  You want the timestamps to be monotonically
increasing?  

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.

Alright, makes sense.

quoted

Is the +60 the most obvious choice?

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.

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?

quoted

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

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:

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.

-- 
What is the difference between Mechanical Engineers and Civil Engineers?
Mechanical Engineers build weapons, Civil Engineers build targets.

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

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?

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 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.

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 d5659084435a885e05a8fc9afbffe8cdd9535828 Mon Sep 17 00:00:00 2001

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(-)

diff --git a/guilt b/guilt
index b6e2a6c..edcfb34 100755
--- a/guilt
+++ b/guilt

@@ -535,6 +535,17 @@ commit()
                         export GIT_AUTHOR_EMAIL="`echo $author_str | sed -e 's/[^<]*//'`"
 		fi
 
+		ct=$(git log -1 --pretty=%ct)
+		if [ $ct -gt $(stat -c %Y "$p") ]; then
+		    echo "Adjusting mod time of" $(basename "$p")
+		    ct=$(expr $ct + 60)
+		    if [ $ct -gt $(date +%s) ]; then
+			touch "$p"
+		    else
+			touch -d @$(expr $ct + 60) "$p"
+		    fi
+		fi
+
 		# must strip nano-second part otherwise git gets very
 		# confused, and makes up strange timestamps from the past
 		# (chances are it decides to interpret it as a unix

-- 
1.7.0.4

tytso@mit.edu wrote:

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

quoted

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 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:

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)

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.

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.

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

On Tue, Jul 6, 2010 at 10:03 AM, Jonathan Nieder [off-list ref] wrote:

tytso@mit.edu wrote:

quoted

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

quoted

quoted

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 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)

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?

-- 
Erik "kusma" Faye-Lund

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

quoted

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)

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?

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 [off-list ref] wrote:

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

quoted

quoted

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)

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?

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).

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 :)

-- 
Erik "kusma" Faye-Lund

Theodore Tso 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.

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:

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.

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

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

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?

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.

From d5659084435a885e05a8fc9afbffe8cdd9535828 Mon Sep 17 00:00:00 2001

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

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

+		    ct=$(expr $ct + 60)

So, ct is now the +1min time.

+		    if [ $ct -gt $(date +%s) ]; then
+			touch "$p"
+		    else
+			touch -d @$(expr $ct + 60) "$p"

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

Thanks,

Jeff.

-- 
NT is to UNIX what a doughnut is to a particle accelerator.

tytso@mit.edu wrote:

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

quoted

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.

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.

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 06, 2010 at 01:29:54PM -0400, jeffpc@josefsipek.net wrote:

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

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.

-- 
The obvious mathematical breakthrough would be development of an easy way to
factor large prime numbers.
		- Bill Gates, The Road Ahead, pg. 265