{"thread":{"id":"33101","subject":"[PATCH] Fix revision walk for commits with the same dates","startedAt":"2013-03-07T18:03:22Z","lastAt":"2013-03-24T02:18:49Z","messageCount":5,"participants":["Kacper Kornet","Junio C Hamano","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"210778","messageId":"20130307180321.GA26756@camk.edu.pl","threadId":"33101","inReplyTo":null,"subject":"[PATCH] Fix revision walk for commits with the same dates","fromName":"Kacper Kornet","fromEmail":"draenog@pld-linux.org","sentAt":"2013-03-07T18:03:22Z","receivedAt":"2013-03-07T18:03:22Z","isPatch":true,"sender":{"key":"draenog@pld-linux.org","avatar":"https://avatars.githubusercontent.com/u/608762?v=4"},"body":"git rev-list A^! --not B provides wrong answer if all commits in the\nrange A..B had the same commit times and there are more then 8 of them.\nThis commits fixes the logic in still_interesting function to prevent\nthis error.\n\nSigned-off-by: Kacper Kornet \n---\n revision.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/revision.c b/revision.c\nindex ef60205..cf620c6 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -709,7 +709,7 @@ static int still_interesting(struct commit_list *src, unsigned long date, int sl\n \t * Does the destination list contain entries with a date\n \t * before the source list? Definitely _not_ done.\n \t */\n-\tif (date < src->item->date)\n+\tif (date <= src->item->date)\n \t\treturn SLOP;\n \n \t/*\n-- \n1.8.2.rc2\n"},{"id":"211972","messageId":"20130322183819.GA18210@camk.edu.pl","threadId":"33101","inReplyTo":"20130307180321.GA26756@camk.edu.pl","subject":"[PATCH v2] Fix revision walk for commits with the same dates","fromName":"Kacper Kornet","fromEmail":"draenog@pld-linux.org","sentAt":"2013-03-22T18:38:19Z","receivedAt":"2013-03-22T18:38:19Z","isPatch":true,"sender":{"key":"draenog@pld-linux.org","avatar":"https://avatars.githubusercontent.com/u/608762?v=4"},"body":"Logic in still_interesting function allows to stop the commits\ntraversing if the oldest processed commit is not older then the\nyoungest commit on the list to process and the list contains only\ncommits marked as not interesting ones. It can be premature when dealing\nwith a set of coequal commits. For example git rev-list A^! --not B\nprovides wrong answer if all commits in the range A..B had the same\ncommit time and there are more then 7 of them.\n\nTo fix this problem the relevant part of the logic in still_interesting\nis changed to: the walk can be stopped if the oldest processed commit is\nyounger then the youngest commit on the list to processed.\n\nSigned-off-by: Kacper Kornet \n---\n\nI don't know whether the first version was overlooked or deemed as not\nworthy. So just in case I resend it. Changes since the first version:\n\n1. The test has been added\n2. The commit log has been rewritten\n\n\n revision.c | 2 +-\n t/t6009-rev-list-parent.sh | 13 +++++++++++++\n 2 files changed, 14 insertions(+), 1 deletion(-)\n\ndiff --git a/revision.c b/revision.c\nindex ef60205..cf620c6 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -709,7 +709,7 @@ static int still_interesting(struct commit_list *src, unsigned long date, int sl\n \t * Does the destination list contain entries with a date\n \t * before the source list? Definitely _not_ done.\n \t */\n-\tif (date < src->item->date)\n+\tif (date <= src->item->date)\n \t\treturn SLOP;\n \n \t/*\ndiff --git a/t/t6009-rev-list-parent.sh b/t/t6009-rev-list-parent.sh\nindex 3050740..66cda17 100755\n--- a/t/t6009-rev-list-parent.sh\n+++ b/t/t6009-rev-list-parent.sh\n@@ -133,4 +133,17 @@ test_expect_success 'dodecapus' '\n \tcheck_revlist \"--min-parents=13\" &&\n \tcheck_revlist \"--min-parents=4 --max-parents=11\" tetrapus\n '\n+\n+test_expect_success 'ancestors with the same commit time' '\n+\n+\ttest_tick_keep=$test_tick &&\n+\tfor i in 1 2 3 4 5 6 7 8; do\n+\t\ttest_tick=$test_tick_keep\n+\t\ttest_commit t$i\n+\tdone &&\n+\tgit rev-list t1^! --not t$i >result &&\n+\t>expect &&\n+\ttest_cmp expect result\n+'\n+\n test_done\n-- \n1.8.2\n\n-- \n Kacper Kornet\n"},{"id":"211980","messageId":"7va9pv6u4k.fsf@alter.siamese.dyndns.org","threadId":"33101","inReplyTo":"20130322183819.GA18210@camk.edu.pl","subject":"Re: [PATCH v2] Fix revision walk for commits with the same dates","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-22T20:45:47Z","receivedAt":"2013-03-22T20:45:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kacper Kornet writes:\n\n> Logic in still_interesting function allows to stop the commits\n> traversing if the oldest processed commit is not older then the\n> youngest commit on the list to process and the list contains only\n> commits marked as not interesting ones. It can be premature when dealing\n> with a set of coequal commits. For example git rev-list A^! --not B\n> provides wrong answer if all commits in the range A..B had the same\n> commit time and there are more then 7 of them.\n>\n> To fix this problem the relevant part of the logic in still_interesting\n> is changed to: the walk can be stopped if the oldest processed commit is\n> younger then the youngest commit on the list to processed.\n\nIs the made-up test case to freeze the clock even interesting? The\nslop logic is merely a heuristic to compensate for effects caused by\nskewed or non-monototic clocks, so in a different repository you may\neven need to fuzz the timestamp comparison further\n\n\tif (date - 10 < src->item->date)\n\nor something silly like that.\n\n\n\n> Signed-off-by: Kacper Kornet \n> ---\n>\n> I don't know whether the first version was overlooked or deemed as not\n> worthy. So just in case I resend it. Changes since the first version:\n>\n> 1. The test has been added\n> 2. The commit log has been rewritten\n>\n>\n> revision.c | 2 +-\n> t/t6009-rev-list-parent.sh | 13 +++++++++++++\n> 2 files changed, 14 insertions(+), 1 deletion(-)\n>\n> diff --git a/revision.c b/revision.c\n> index ef60205..cf620c6 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -709,7 +709,7 @@ static int still_interesting(struct commit_list *src, unsigned long date, int sl\n> \t * Does the destination list contain entries with a date\n> \t * before the source list? Definitely _not_ done.\n> \t */\n> -\tif (date < src->item->date)\n> +\tif (date <= src->item->date)\n> \t\treturn SLOP;\n> \n> \t/*\n> diff --git a/t/t6009-rev-list-parent.sh b/t/t6009-rev-list-parent.sh\n> index 3050740..66cda17 100755\n> --- a/t/t6009-rev-list-parent.sh\n> +++ b/t/t6009-rev-list-parent.sh\n> @@ -133,4 +133,17 @@ test_expect_success 'dodecapus' '\n> \tcheck_revlist \"--min-parents=13\" &&\n> \tcheck_revlist \"--min-parents=4 --max-parents=11\" tetrapus\n> '\n> +\n> +test_expect_success 'ancestors with the same commit time' '\n> +\n> +\ttest_tick_keep=$test_tick &&\n> +\tfor i in 1 2 3 4 5 6 7 8; do\n> +\t\ttest_tick=$test_tick_keep\n> +\t\ttest_commit t$i\n> +\tdone &&\n> +\tgit rev-list t1^! --not t$i >result &&\n> +\t>expect &&\n> +\ttest_cmp expect result\n> +'\n> +\n> test_done\n> -- \n> 1.8.2\n"},{"id":"211982","messageId":"20130322210741.GC18210@camk.edu.pl","threadId":"33101","inReplyTo":"7va9pv6u4k.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2] Fix revision walk for commits with the same dates","fromName":"Kacper Kornet","fromEmail":"draenog@pld-linux.org","sentAt":"2013-03-22T21:07:41Z","receivedAt":"2013-03-22T21:07:41Z","isPatch":true,"sender":{"key":"draenog@pld-linux.org","avatar":"https://avatars.githubusercontent.com/u/608762?v=4"},"body":"On Fri, Mar 22, 2013 at 01:45:47PM -0700, Junio C Hamano wrote:\n> Kacper Kornet writes:\n\n> > Logic in still_interesting function allows to stop the commits\n> > traversing if the oldest processed commit is not older then the\n> > youngest commit on the list to process and the list contains only\n> > commits marked as not interesting ones. It can be premature when dealing\n> > with a set of coequal commits. For example git rev-list A^! --not B\n> > provides wrong answer if all commits in the range A..B had the same\n> > commit time and there are more then 7 of them.\n\n> > To fix this problem the relevant part of the logic in still_interesting\n> > is changed to: the walk can be stopped if the oldest processed commit is\n> > younger then the youngest commit on the list to processed.\n\n> Is the made-up test case to freeze the clock even interesting? The\n> slop logic is merely a heuristic to compensate for effects caused by\n> skewed or non-monototic clocks, so in a different repository you may\n> even need to fuzz the timestamp comparison further\n\n> \tif (date - 10 < src->item->date)\n\n> or something silly like that.\n\nI don't think it is a made-up test case. For example it is easy to get a\nnumber of coequal commits by using git rebase -i. So I argue that git\nshould treat correctly ranges of such commits.\n\n-- \n Kacper Kornet\n"},{"id":"212065","messageId":"CAPig+cQjkE1PcfpsaAEYDq8zaLzQTEY_4j8B-Fp0=7=EnosFfw@mail.gmail.com","threadId":"33101","inReplyTo":"20130322183819.GA18210@camk.edu.pl","subject":"Re: [PATCH v2] Fix revision walk for commits with the same dates","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-03-24T02:18:49Z","receivedAt":"2013-03-24T02:18:49Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Mar 22, 2013 at 2:38 PM, Kacper Kornet wrote:\n> Logic in still_interesting function allows to stop the commits\n> traversing if the oldest processed commit is not older then the\n\ns/then/than/\n\n> youngest commit on the list to process and the list contains only\n> commits marked as not interesting ones. It can be premature when dealing\n> with a set of coequal commits. For example git rev-list A^! --not B\n> provides wrong answer if all commits in the range A..B had the same\n> commit time and there are more then 7 of them.\n"}]}