Thread: git push hook to check for outdated timestamps
Could we update our git hook to refuse a push of a new commit whose timestamp is more than, say, 24 hours in the past? Our commit history has some timestamps in it now that are over a month off, and it's really easy to do, because when you rebase a commit, it keeps the old timestamp. If you then merge or cherry-pick that into an official branch rather than patch + commit, you end up with this problem unless you are careful to fix it by hand. It would be nice to prevent further mistakes of this sort, as they create confusion. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 06/12/2015 09:31 AM, Robert Haas wrote: > Could we update our git hook to refuse a push of a new commit whose > timestamp is more than, say, 24 hours in the past? Our commit history > has some timestamps in it now that are over a month off, and it's > really easy to do, because when you rebase a commit, it keeps the old > timestamp. If you then merge or cherry-pick that into an official > branch rather than patch + commit, you end up with this problem unless > you are careful to fix it by hand. It would be nice to prevent > further mistakes of this sort, as they create confusion. > +1. I think 24 hours is probably fairly generous, but in principle this seems right - the timestamp would ideally be pretty close to the time it hits the public tree. cheers andrew
Andrew Dunstan <andrew@dunslane.net> writes:
> On 06/12/2015 09:31 AM, Robert Haas wrote:
>> Could we update our git hook to refuse a push of a new commit whose
>> timestamp is more than, say, 24 hours in the past? Our commit history
>> has some timestamps in it now that are over a month off, and it's
>> really easy to do, because when you rebase a commit, it keeps the old
>> timestamp. If you then merge or cherry-pick that into an official
>> branch rather than patch + commit, you end up with this problem unless
>> you are careful to fix it by hand. It would be nice to prevent
>> further mistakes of this sort, as they create confusion.
> I think 24 hours is probably fairly generous,
Yeah ... if we're going to try to enforce a linear-looking history, ISTM
the requirement ought to be "newer than the latest commit on the same
branch". Perhaps that would be unduly hard to implement though.
FWIW, our git_changelog script tries to avoid this problem by paying
attention to CommitDate not Date. But I agree that it's confusing when
those fields are far apart.
regards, tom lane
On Fri, Jun 12, 2015 at 4:33 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Andrew Dunstan <andrew@dunslane.net> writes:
> On 06/12/2015 09:31 AM, Robert Haas wrote:
>> Could we update our git hook to refuse a push of a new commit whose
>> timestamp is more than, say, 24 hours in the past? Our commit history
>> has some timestamps in it now that are over a month off, and it's
>> really easy to do, because when you rebase a commit, it keeps the old
>> timestamp. If you then merge or cherry-pick that into an official
>> branch rather than patch + commit, you end up with this problem unless
>> you are careful to fix it by hand. It would be nice to prevent
>> further mistakes of this sort, as they create confusion.
> I think 24 hours is probably fairly generous,
Yeah ... if we're going to try to enforce a linear-looking history, ISTM
the requirement ought to be "newer than the latest commit on the same
branch". Perhaps that would be unduly hard to implement though.
From a quick look at our existing script, I think that's doable, but I'd have to do some more detailed verification before I'm sure. And we'd have to figure out some way to deal with a push with multiple commits in it, but it should certainly be doable if the first one is.
Would we in that case want to enforce linearity *and* recent-ness, or just linearity? as in, do we care about the commit time even if it doesn't change the order?
FWIW, our git_changelog script tries to avoid this problem by paying
attention to CommitDate not Date. But I agree that it's confusing when
those fields are far apart.
That brings it back to the enforcing - would we want to enforce both those?
(And to confuse it even more, Date gets renamed to AuthorDate when you do a full log.. But AFAIK it's the same date, they just change the name of it)
On Sun, Jun 14, 2015 at 12:37:00PM +0200, Magnus Hagander wrote: > On Fri, Jun 12, 2015 at 4:33 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Andrew Dunstan <andrew@dunslane.net> writes: > > > On 06/12/2015 09:31 AM, Robert Haas wrote: > > >> Could we update our git hook to refuse a push of a new commit whose > > >> timestamp is more than, say, 24 hours in the past? Our commit history > > >> has some timestamps in it now that are over a month off, and it's > > >> really easy to do, because when you rebase a commit, it keeps the old > > >> timestamp. If you then merge or cherry-pick that into an official > > >> branch rather than patch + commit, you end up with this problem unless > > >> you are careful to fix it by hand. It would be nice to prevent > > >> further mistakes of this sort, as they create confusion. > > > > > I think 24 hours is probably fairly generous, > > > > Yeah ... if we're going to try to enforce a linear-looking history, ISTM > > the requirement ought to be "newer than the latest commit on the same > > branch". Perhaps that would be unduly hard to implement though. > > > > From a quick look at our existing script, I think that's doable, but I'd > have to do some more detailed verification before I'm sure. And we'd have > to figure out some way to deal with a push with multiple commits in it, but > it should certainly be doable if the first one is. > > Would we in that case want to enforce linearity *and* recent-ness, or just > linearity? as in, do we care about the commit time even if it doesn't > change the order? If a recency check is essentially free, let's check both. Otherwise, enforcing linearity alone is a 95% solution that implicitly bounds recency. > > FWIW, our git_changelog script tries to avoid this problem by paying > > attention to CommitDate not Date. But I agree that it's confusing when > > those fields are far apart. > > > > That brings it back to the enforcing - would we want to enforce both those? May as well. AuthorDate is the main source of trouble. You would need to go out of your way (e.g. git filter-branch) to push an old CommitDate, but let's check it just the same.
Noah Misch wrote: > On Sun, Jun 14, 2015 at 12:37:00PM +0200, Magnus Hagander wrote: > > Would we in that case want to enforce linearity *and* recent-ness, or just > > linearity? as in, do we care about the commit time even if it doesn't > > change the order? > > If a recency check is essentially free, let's check both. Otherwise, > enforcing linearity alone is a 95% solution that implicitly bounds recency. Should there also be a check to reject times too far in the future? I think a clock skew of a few minutes (up to ten?) is acceptable, but more than that would cause trouble for other committers later on. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Tue, Jun 23, 2015 at 10:15 PM, Noah Misch <noah@leadboat.com> wrote: >> That brings it back to the enforcing - would we want to enforce both those? > > May as well. AuthorDate is the main source of trouble. You would need to go > out of your way (e.g. git filter-branch) to push an old CommitDate, but let's > check it just the same. Actually, you just need the system clock to be off. I've noticed, for example, that when my VMware VMs go to sleep (because I close my laptop lid) their clocks don't run, so I have to remember to ntpdate afterwards if I want correct time. I don't happen to use those for committing to PostgreSQL, but somebody else might have a similar setup that they do use for that purpose. So +1 for sanity-checking the commit date, and +1 as well as Alvaro's proposal for checking for both past and future times. I think we should tolerate a bit more in terms of past timestamps than future timestamps. It's quite reasonable for someone to commit locally and then run make check-world or so before pushing; let's not make that unnecessarily annoying. But future timestamps should really only ever happen because of clock slew, and I don't think we need to tolerate very much of that. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 6/12/15 9:31 AM, Robert Haas wrote: > Could we update our git hook to refuse a push of a new commit whose > timestamp is more than, say, 24 hours in the past? Our commit history > has some timestamps in it now that are over a month off, and it's > really easy to do, because when you rebase a commit, it keeps the old > timestamp. If you then merge or cherry-pick that into an official > branch rather than patch + commit, you end up with this problem unless > you are careful to fix it by hand. It would be nice to prevent > further mistakes of this sort, as they create confusion. FWIW, I have been doing that, and I have not considered it a problem. If the patch was submitted three months ago, reviewed, and then committed unchanged, the date is what it is. Also, when I cherry-pick a commit from master to a back branch, I keep the author timestamp the same. I consider that a feature. Note that we have a while ago enabled author and committer to be distinct. But the above proposal would effectively require author date and committer date to be (almost) the same, which would create inconsistent information. Also, rebasing updates the committer date but not the author date, because, well, the commit was changed, but no authoring is involved. git rebase has options to vary this behavior, but note that the documentation uses the term "lie" in their description.
On Wed, Jun 24, 2015 at 11:37 AM, Peter Eisentraut <peter_e@gmx.net> wrote: > On 6/12/15 9:31 AM, Robert Haas wrote: >> Could we update our git hook to refuse a push of a new commit whose >> timestamp is more than, say, 24 hours in the past? Our commit history >> has some timestamps in it now that are over a month off, and it's >> really easy to do, because when you rebase a commit, it keeps the old >> timestamp. If you then merge or cherry-pick that into an official >> branch rather than patch + commit, you end up with this problem unless >> you are careful to fix it by hand. It would be nice to prevent >> further mistakes of this sort, as they create confusion. > > FWIW, I have been doing that, and I have not considered it a problem. > If the patch was submitted three months ago, reviewed, and then > committed unchanged, the date is what it is. Also, when I cherry-pick a > commit from master to a back branch, I keep the author timestamp the > same. I consider that a feature. I don't, because it means that the timestamps you see when you run git log are non-linear. I don't care myself if they are slightly out of order, although it seems that others do, but I do mind when they are months off. Typically when this happens to me, it's not a case of the patch being unchanged. I make changes on a branch and then use git rebase -i to squash them into a single patch which I then cherry-pick. But git rebase -i keeps the timestamp of the first (oldest) commit, so I end up with a commit that is timestamped as to when I began development, not when I finished it. So the date is just wrong. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 6/24/15 12:55 PM, Robert Haas wrote: >> FWIW, I have been doing that, and I have not considered it a problem. >> If the patch was submitted three months ago, reviewed, and then >> committed unchanged, the date is what it is. Also, when I cherry-pick a >> commit from master to a back branch, I keep the author timestamp the >> same. I consider that a feature. > > I don't, because it means that the timestamps you see when you run git > log are non-linear. I don't care myself if they are slightly out of > order, although it seems that others do, but I do mind when they are > months off. Why is that a problem? > Typically when this happens to me, it's not a case of the patch being > unchanged. I make changes on a branch and then use git rebase -i to > squash them into a single patch which I then cherry-pick. But git > rebase -i keeps the timestamp of the first (oldest) commit, so I end > up with a commit that is timestamped as to when I began development, > not when I finished it. So the date is just wrong. Sure, but then it's up to you to fix it, just like it's up to you to merge the commit messages and do other adjustments to the commit. But this is one of many cases, and we shouldn't throw out the whole concept because of it.
On Wed, Jun 24, 2015 at 10:03:50AM -0400, Robert Haas wrote: > On Tue, Jun 23, 2015 at 10:15 PM, Noah Misch <noah@leadboat.com> wrote: > >> That brings it back to the enforcing - would we want to enforce both those? > > > > May as well. AuthorDate is the main source of trouble. You would need to go > > out of your way (e.g. git filter-branch) to push an old CommitDate, but let's > > check it just the same. > > Actually, you just need the system clock to be off. I've noticed, for > example, that when my VMware VMs go to sleep (because I close my > laptop lid) their clocks don't run, so I have to remember to ntpdate > afterwards if I want correct time. I don't happen to use those for > committing to PostgreSQL, but somebody else might have a similar setup > that they do use for that purpose. I didn't think of that. So, yep, checking both is valuable. > So +1 for sanity-checking the commit date, and +1 as well as Alvaro's > proposal for checking for both past and future times. I think we > should tolerate a bit more in terms of past timestamps than future > timestamps. It's quite reasonable for someone to commit locally and > then run make check-world or so before pushing; let's not make that > unnecessarily annoying. But future timestamps should really only ever > happen because of clock slew, and I don't think we need to tolerate > very much of that. Sounds good.
On Wed, Jun 24, 2015 at 3:50 PM, Peter Eisentraut <peter_e@gmx.net> wrote: > On 6/24/15 12:55 PM, Robert Haas wrote: >>> FWIW, I have been doing that, and I have not considered it a problem. >>> If the patch was submitted three months ago, reviewed, and then >>> committed unchanged, the date is what it is. Also, when I cherry-pick a >>> commit from master to a back branch, I keep the author timestamp the >>> same. I consider that a feature. >> >> I don't, because it means that the timestamps you see when you run git >> log are non-linear. I don't care myself if they are slightly out of >> order, although it seems that others do, but I do mind when they are >> months off. > > Why is that a problem? Because I don't want to have to do git log --format=fuller to see when the thing was committed, basically. >> Typically when this happens to me, it's not a case of the patch being >> unchanged. I make changes on a branch and then use git rebase -i to >> squash them into a single patch which I then cherry-pick. But git >> rebase -i keeps the timestamp of the first (oldest) commit, so I end >> up with a commit that is timestamped as to when I began development, >> not when I finished it. So the date is just wrong. > > Sure, but then it's up to you to fix it, just like it's up to you to > merge the commit messages and do other adjustments to the commit. But > this is one of many cases, and we shouldn't throw out the whole concept > because of it. I don't particularly think that having separate AuthorDate and CommitterDate fields has any value. At least, it doesn't to me. But a linear-looking commit history does have value to me. Someone else might have different priorities; those are mine. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 6/25/15 8:08 PM, Robert Haas wrote: > Because I don't want to have to do git log --format=fuller to see when > the thing was committed, basically. Then I suggest to you the following configuration settings: [format] pretty=cmedium [pretty] cmedium="format:%C(auto,yellow)commit %H%C(reset)%nCommit: %cn <%ce>%nCommitDate: %cd%n%n%w(80,4,4)%B" > I don't particularly think that having separate AuthorDate and > CommitterDate fields has any value. At least, it doesn't to me. But > a linear-looking commit history does have value to me. Someone else > might have different priorities; those are mine. Sure, that's why Git is configurable to particular preferences. But we don't have to force everyone to use Git in a nonstandard way.
On Fri, Jun 26, 2015 at 4:11 PM, Peter Eisentraut <peter_e@gmx.net> wrote: > On 6/25/15 8:08 PM, Robert Haas wrote: >> Because I don't want to have to do git log --format=fuller to see when >> the thing was committed, basically. > > Then I suggest to you the following configuration settings: > > [format] > pretty=cmedium > [pretty] > cmedium="format:%C(auto,yellow)commit %H%C(reset)%nCommit: %cn <%ce>%nCommitDate: %cd%n%n%w(80,4,4)%B" > >> I don't particularly think that having separate AuthorDate and >> CommitterDate fields has any value. At least, it doesn't to me. But >> a linear-looking commit history does have value to me. Someone else >> might have different priorities; those are mine. > > Sure, that's why Git is configurable to particular preferences. But we > don't have to force everyone to use Git in a nonstandard way. I respect your opinion, but I think you are out-voted, unless some of the people who previously favored this proposal have changed their minds based on this discussion. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Peter Eisentraut wrote: > On 6/25/15 8:08 PM, Robert Haas wrote: > > Because I don't want to have to do git log --format=fuller to see when > > the thing was committed, basically. > > Then I suggest to you the following configuration settings: > > [format] > pretty=cmedium > [pretty] > cmedium="format:%C(auto,yellow)commit %H%C(reset)%nCommit: %cn <%ce>%nCommitDate: %cd%n%n%w(80,4,4)%B" I have been using a slightly tweaked version of this and I have found that the %w(80,4,4)%B thingy results in mangled formatting; for instance, commit bbfd7edae5 ends with this: Discussion: 54B58BA3.8040302@ohmu.fi Author: Oskari Saarenmaa, with some minor changes by me. whereas it originally was written as Discussion: 54B58BA3.8040302@ohmu.fi Author: Oskari Saarenmaa, with some minor changes by me. I find this a bad enough problem that I'll probably have to remove that. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 7/14/15 3:44 AM, Alvaro Herrera wrote: > Peter Eisentraut wrote: >> On 6/25/15 8:08 PM, Robert Haas wrote: >>> Because I don't want to have to do git log --format=fuller to see when >>> the thing was committed, basically. >> >> Then I suggest to you the following configuration settings: >> >> [format] >> pretty=cmedium >> [pretty] >> cmedium="format:%C(auto,yellow)commit %H%C(reset)%nCommit: %cn <%ce>%nCommitDate: %cd%n%n%w(80,4,4)%B" > > I have been using a slightly tweaked version of this and I have found > that the %w(80,4,4)%B thingy results in mangled formatting; I have since refined this to ... %n%n%w(0,4,4)%s%n%+b You might find that that works better. One of the curiosities is that the built-in log formats don't appear to be defined or easily definable in terms of the formatting language.
Peter Eisentraut wrote: > On 7/14/15 3:44 AM, Alvaro Herrera wrote: > > I have been using a slightly tweaked version of this and I have found > > that the %w(80,4,4)%B thingy results in mangled formatting; > > I have since refined this to > > ... %n%n%w(0,4,4)%s%n%+b > > You might find that that works better. Ah, yes it does, thanks. > One of the curiosities is that the built-in log formats don't appear to > be defined or easily definable in terms of the formatting language. TBH I'm not surprised. Normally the built-in formats for things grow organically in ad-hoc ways and the mini-languages for the generic mechanisms don't support all the same features. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services