[#1774] Fix unintended behavior related to --since d1 #1776

yhtMinceraft1010X · 2022-05-07T08:10:16Z

Fixes #1774.
Fixes #1778.

Proposed commit message

The use of Instant.ofEpochMilli(Long.MIN_VALUE) as the arbitrary first
commit date now fails shallow cloning for
testSinceBeginningDateRangeWithShallowCloning system test.

In addition, using this arbitrary date in convertToGitDateRangeArgs
when converting timezone results in an overflow causing sinceDate to
be greater than untilDate. Thus, no commit results are retrieved,
leading to failing system tests.

Also, the arbitrary first commit date is in UTC. However, in the
convertToGitDateRangeArgs method, the zoneId used for the conversion
is from the config, which may be different. This leads to an erroneous
timezone conversion.

Finally, using --since d1 with --period results in an unintended date
range because the --period and the arbitrary commit date from 
--since d1 are used to calculate the until date as the actual 
earliest commit date is not immediately available.

Let's change Instant.ofEpochMilli() to use 0 so that the arbitrary date
is 1970-01-01 and the overflow of the sinceDate can be avoided.

At the same time, let's convert the arbitrary date to the correct
timezone within the ArgsParser class and add a component test for
parsing --since d1 to check that the correct timezone is used for each
RepoConfiguration before cloning any repos.

Finally, let's raise a warning when --since d1 is being used with 
--period.

Other information

Screenshot showing the overflow of the sinceDate when Instant.ofEpochMilli(Long.MIN_VALUE) is used:

Notice how the sinceDate has + sign instead of - sign.

Timezone Trivia

There also seems to be something strange with the timezone here:

Although the code for this method has been changed, the timezone conversion logic in lines 60-61 is still preserved. The time appears to be converted to +07:30 instead of +08:00. However, tests still work correctly. This is likely following the old SG timezone being used before 1 January 1982.

The use of Instant.ofEpochMilli(Long.MIN_VALUE) as the arbitrary first commit date now fails shallow cloning for testSinceBeginningDateRangeWithShallowCloning system test. In addition, using this arbitrary date in convertToGitDateRangeArgs when converting timezone results in an overflow causing sinceDate to be greater than untilDate. Thus, no commit results are retrieved, leading to failing system tests. Finally, the arbitrary first commit date is in UTC. However, in the convertToGitDateRangeArgs method, the zoneId used for the conversion is from the config, which may be different. This leads to an erroneous timezone conversion. Let's change Instant.ofEpochMilli() to use 0 so that the arbitrary date is 1970-01-01. At the same time, let's convert the arbitrary date to the correct timezone within the ArgsParser class and add a component test for parsing --since d1. This ensures that an arbitrary since date is adjusted to the correct timezone for each RepoConfiguration before cloning any repos.

gok99

Thanks for identifying the issue! Looks good to me, except for some minor questions.

src/main/java/reposense/commits/CommitResultAggregator.java

gok99 · 2022-05-08T13:22:32Z

src/main/java/reposense/parser/ArgsParser.java

@@ -290,6 +290,10 @@ public static CliArguments parse(String[] args) throws HelpScreenException, Pars

            if (isSinceDateProvided) {
                sinceDate = TimeUtil.getSinceDate(cliSinceDate.get());
+                // For --since d1, need to adjust the arbitrary date based on timezone
+                if (sinceDate.equals(SinceDateArgumentType.getArbitraryFirstCommitDateUtcLocal())) {


Could this logic go into TimeUtil::getSinceDate?

Would just like to get this clarified too - there seems to be an identical clause in getSinceDate. If it can't/shouldn't be moved into getSinceDate, I'll approve the PR.

Now that you've mentioned it, I think the clause in TimeUtil.getSinceDate can be safely removed. Since the new arbitrary date is 1970-01-01 00:00:00 in UTC time, adding withHour(0).withMinute(0).withSecond(0) does nothing to it so it is effectively returning the same date. The equality check in ArgsParser should still pass.

That being said, we still have to keep withHour(0).withMinute(0).withSecond(0) in TimeUtil.getSinceDate since the getDateMinusAMonth and getDateMinusNDays methods still need to convert the since date to midnight.

As the until date is calculated using the --period with reference to the arbitrary date of --since d1, the result is an incorrect date range. This may result in no data being provided. Let's throw a warning when the user may be using --since d1 with the --period flag.

yhtMinceraft1010X · 2022-05-15T03:31:08Z

For #1778, I decided to use a warning instead of throwing an exception and terminating the program before cloning. This is due to the possibility that someone might put --since 1/1/1970 --period 30d. In this case, an exception would be thrown since the given date would be equal to the arbitrary date that is used to check for whether --since d1 might be used and no cloning would be done.

Although this is an unlikely scenario, I believe no exception should be thrown since setting 31/1/1970 as the until date is expected behaviour.

dcshzj · 2022-05-18T01:32:57Z

docs/ug/cli.md

@@ -172,6 +172,7 @@ This flag overrides the `Ignore file size limit` field in the CSV config file.
 <box type="info" seamless>

 * If both start date and end date are not specified, the date of generating the report will be taken as the end date.
+* May analyse the incorrect date range if used with `--since d1`. The program will throw a warning.


Suggested change

* May analyse the incorrect date range if used with `--since d1`. The program will throw a warning.

* May analyze the incorrect date range if used with `--since d1`. The program will throw a warning.

dcshzj · 2022-05-18T01:33:18Z

docs/ug/cli.md

@@ -216,7 +217,8 @@ Cannot be used with `--last-modified-date`. This may result in an incorrect last
 <box type="info" seamless>

 * If the start date is not specified, only commits made one month before the end date (if specified) or the date of generating the report, will be captured and analyzed.
-* If `d1` is specified as the start date (`--since d1` or `-s d1`), then the earliest commit date of all repositories will be taken as the start date.
+* If `d1` is specified as the start date (`--since d1` or `-s d1`), then the program will search for the earliest commit date of all repositories and use that as the start date.
+* If `d1` is specified together with `--period`, then the program will warn that the date range being analysed may be incorrect.


Suggested change

* If `d1` is specified together with `--period`, then the program will warn that the date range being analysed may be incorrect.

* If `d1` is specified together with `--period`, then the program will warn that the date range being analyzed may be incorrect.

dcshzj · 2022-05-18T01:44:04Z

src/main/java/reposense/parser/ArgsParser.java

+                if (TimeUtil.isEqualToArbitraryFirstDateUtc(sinceDate)) {
+                    if (isPeriodProvided) {
+                        logger.warning(MESSAGE_SINCE_D1_WITH_PERIOD);
+                    }
+                    sinceDate = TimeUtil.getArbitraryFirstCommitDateConverted(zoneId);


The indentation for this part seems to be quite deep. Is there any way to flatten this?

dcshzj · 2022-05-18T01:46:42Z

src/main/java/reposense/parser/SinceDateArgumentType.java

    public static final String FIRST_COMMIT_DATE_SHORTHAND = "d1";
+    private static final ZonedDateTime ARBITRARY_FIRST_COMMIT_DATE = ZonedDateTime.ofInstant(
+            Instant.ofEpochMilli(0), ZoneId.of("Z"));
+    private static final LocalDateTime ARBITRARY_FIRST_COMMIT_DATE_UTC_LOCAL = ARBITRARY_FIRST_COMMIT_DATE


The variable naming seems a little confusing. May I suggest using ARBITRARY_FIRST_COMMIT_DATE_UTC and ARBITRARY_FIRST_COMMIT_DATE_LOCAL instead?

dcshzj

LGTM!

github-actions · 2022-06-05T15:29:45Z

The following links are for previewing this pull request:

Dashboard Preview: https://dashboard-1776-pr-reposense-reposense.surge.sh
Docs Preview: https://docs-1776-pr-reposense-reposense.surge.sh

…ense#1776) The use of Instant.ofEpochMilli(Long.MIN_VALUE) as the arbitrary first commit date now fails shallow cloning for testSinceBeginningDateRangeWithShallowCloning system test. In addition, using this arbitrary date in convertToGitDateRangeArgs when converting timezone results in an overflow causing sinceDate to be greater than untilDate. Thus, no commit results are retrieved, leading to failing system tests. Also, the arbitrary first commit date is in UTC. However, in the convertToGitDateRangeArgs method, the zoneId used for the conversion is from the config, which may be different. This leads to an erroneous timezone conversion. Finally, using --since d1 with --period results in an unintended date range because the --period and the arbitrary commit date from --since d1 are used to calculate the until date as the actual earliest commit date is not immediately available. Let's change Instant.ofEpochMilli() to use 0 so that the arbitrary date is 1970-01-01 and the overflow of the sinceDate can be avoided. At the same time, let's convert the arbitrary date to the correct timezone within the ArgsParser class and add a component test for parsing --since d1 to check that the correct timezone is used for each RepoConfiguration before cloning any repos. Finally, let's raise a warning when --since d1 is being used with --period.

yhtMinceraft1010X mentioned this pull request May 7, 2022

Temporary Hotfix - Remove --since d1 from tests #1775

Closed

yhtMinceraft1010X requested a review from a team May 7, 2022 08:13

Remove unnecessary line

37812bb

yhtMinceraft1010X mentioned this pull request May 8, 2022

Docs: fix broken links in about.md #1777

Merged

gok99 reviewed May 8, 2022

View reviewed changes

Add TimeUtil abstraction for arbitrary dates

fc4fd16

yhtMinceraft1010X mentioned this pull request May 12, 2022

Combining --since d1 with --period gives invalid date ranges #1778

Closed

yhtMinceraft1010X added 2 commits May 15, 2022 10:46

Remove redundant check and explain new arbitrary date

06383a3

dcshzj requested changes May 18, 2022

View reviewed changes

dcshzj added the s.ToReview label May 18, 2022

Improve code and docs quality

1c2f8c3

yhtMinceraft1010X mentioned this pull request May 18, 2022

[#1661] Upgrade to Gradle 7 #1780

Merged

yhtMinceraft1010X changed the title ~~[#1774] Fix CI failing due to --since d1~~ [#1774] Fix unintended behavior related to --since d1 May 18, 2022

yhtMinceraft1010X mentioned this pull request Jun 5, 2022

System Test Failed #1783

Closed

yhtMinceraft1010X requested a review from dcshzj June 5, 2022 08:15

dcshzj approved these changes Jun 5, 2022

View reviewed changes

dcshzj added s.ToMerge and removed s.ToReview labels Jun 5, 2022

dcshzj merged commit 538365a into reposense:master Jun 5, 2022

yhtMinceraft1010X deleted the 1774-investigate-failing-ci branch June 15, 2022 05:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#1774] Fix unintended behavior related to --since d1 #1776

[#1774] Fix unintended behavior related to --since d1 #1776

yhtMinceraft1010X commented May 7, 2022 •

edited by dcshzj

Loading

gok99 left a comment

gok99 May 8, 2022

gok99 May 11, 2022

yhtMinceraft1010X May 15, 2022

yhtMinceraft1010X commented May 15, 2022

dcshzj May 18, 2022

dcshzj May 18, 2022

dcshzj May 18, 2022

dcshzj May 18, 2022

dcshzj left a comment

github-actions bot commented Jun 5, 2022

	* May analyse the incorrect date range if used with `--since d1`. The program will throw a warning.
	* May analyze the incorrect date range if used with `--since d1`. The program will throw a warning.

	* If `d1` is specified together with `--period`, then the program will warn that the date range being analysed may be incorrect.
	* If `d1` is specified together with `--period`, then the program will warn that the date range being analyzed may be incorrect.

[#1774] Fix unintended behavior related to --since d1 #1776

[#1774] Fix unintended behavior related to --since d1 #1776

Conversation

yhtMinceraft1010X commented May 7, 2022 • edited by dcshzj Loading

Proposed commit message

Other information

Timezone Trivia

gok99 left a comment

Choose a reason for hiding this comment

gok99 May 8, 2022

Choose a reason for hiding this comment

gok99 May 11, 2022

Choose a reason for hiding this comment

yhtMinceraft1010X May 15, 2022

Choose a reason for hiding this comment

yhtMinceraft1010X commented May 15, 2022

dcshzj May 18, 2022

Choose a reason for hiding this comment

dcshzj May 18, 2022

Choose a reason for hiding this comment

dcshzj May 18, 2022

Choose a reason for hiding this comment

dcshzj May 18, 2022

Choose a reason for hiding this comment

dcshzj left a comment

Choose a reason for hiding this comment

github-actions bot commented Jun 5, 2022

yhtMinceraft1010X commented May 7, 2022 •

edited by dcshzj

Loading