Repository: GothenburgBitFactory/timewarrior (cpp)

Issue:
filtering by tag can produce wrong ids
I updated to dev and removed my 2 bytes long `tags.data` file. And the first run it got successfully recreated:
```
$ rm ~/.timewarrior/data/tags.data
$ timew summ
Tags database does not exist. Recreating from interval data...

Wk Date       Day Tags                                                                                                        Start      End    Time   Total
W9 2020-02-29 Sat [organze], fix broken TimeWarrior v1.2.0, accidentally tags.data file (JSON) got truncated to zero bytes 20:16:10 20:59:07 0:42:57
                  [.nonwo.], text / browse                                                                                 20:59:07 21:31:25 0:32:18
                  [organze], fix broken TimeWarrior v1.2.0, accidentally tags.data file (JSON) got truncated to zero bytes 21:31:25 22:14:45 0:43:20
                  [.rest......], wc.                                                                                       22:14:46 22:17:15 0:02:29
                  [.rest......], eat / drink                                                                               22:17:16 22:42:12 0:24:56
                  [organze], go reg working time                                                                           22:42:12        - 0:28:06 2:54:06

                                                                                                                                                     2:54:06

$ ll ~/.timewarrior/data/tags.data
-rw-r--r-- 1 saukrs Domain Users 75K Feb 29 23:10 /home/saukrs/.timewarrior/data/tags.data
```

Now if I search for some tag via grep, it gives me right IDs -- `@826` and `@444`:
```
$ timew summ :id :year | grep -- '--- monthly time declaration mark ---'
W1 2020-01-03 Fri @826 --- monthly time declaration mark ---, [pro-time]                                                                                                          10:00:07 10:00:08 0:00:01
                  @444 --- monthly time declaration mark ---, [pro-time]                                                                                                          23:46:52 23:46:56 0:00:04
```

But if I search for it directly via TimeW, ID of the second one, `@825` is falsely shown as equal to the decremented first one, `@826`:
```
$ timew summ :id :year '--- monthly time declaration mark ---'

Wk Date       Day ID   Tags                                                 Start      End    Time   Total
W1 2020-01-03 Fri @826 --- monthly time declaration mark ---, [pro-time] 10:00:07 10:00:08 0:00:01 0:00:01
W5 2020-01-31 Fri @825 --- monthly time declaration mark ---, [pro-time] 23:46:52 23:46:56 0:00:04 0:00:04

                                                                                                   0:00:05
```

Do you know a commit which avoids this bug so I could quickly rollback please?
Ping @sruffell 

---
```
$ timew diag

timew 1.2.0
       Platform: <unknown>

Compiler:
        Version: 7.3.0
           Caps: +stdc +stdc_hosted +200809 +200809 +LP64 +c8 +i32 +l64 +vp64 +time_t64
     Compliance: C++11

Build Features
          Built: Feb 29 2020 20:46:30
         Commit: 5ab9b86
          CMake: 3.13.2
     Build type: release

Configuration
  TIMEWARRIORDB: -
            Cfg: /home/saukrs/.timewarrior/timewarrior.cfg (-rw- 64 bytes)
       Database: /home/saukrs/.timewarrior (drwx 0 bytes)
    Color theme: Built-in default
                 00 01 02 03 04 05 06 07 08 09 10 11 12

Extensions
       Location: /home/saukrs/.timewarrior/extensions (drwx 0 bytes)
               (None)
```


Discussion:
>Do you know a commit which avoids this bug so I could quickly rollback please?

Well, bummer. If I had to guess, it *sounds* like it would be the change that does not load the entire database which I added.  You could try tag `v1.2.0` to see if it is fixed there.

~However, do you think you could give me a set of instructions from a brand new database that will reproduce so I can look into it?~ I just tried it and see what you mean below:
```

$ timew summ :ids

Wk Date       Day ID Tags    Start      End    Time   Total
W9 2020-02-29 Sat @5 tag1 15:28:31 15:28:33 0:00:02
                  @4 tag2 15:28:33 15:28:35 0:00:02
                  @3 tag3 15:28:35 15:28:36 0:00:01
                  @2 tag4 15:28:36 15:28:48 0:00:12
                  @1 tag1 15:28:48        - 0:02:25 0:02:42

                                                    0:02:42

$ timew summ tag1 :ids

Wk Date       Day ID Tags    Start      End    Time   Total
W9 2020-02-29 Sat @5 tag1 15:28:31 15:28:33 0:00:02
                  @4 tag1 15:28:48        - 0:02:28 0:02:30

                                                    0:02:30
```

So this is most definitely a bug I added as part of the commits that went in for #245. I'll see if I can submit a pull request this weekend, but this change is *not* in v1.2.0.

Also, thank you for trying the dev branch and reporting a definite issue that was not caught by the current test suite.
Specifically, the problem is that I did not account for non-contiguous ranges of intervals as seen here: https://github.com/GothenburgBitFactory/timewarrior/blob/dev/src/data.cpp#L741-L745
Thanks. No need to hurry then.

Local time here is running out for me to declare two months of my enterprise activity. And my accounting script is broken now.  After 20 minutes it will be March.

So I will skip the accounting and will try to fake my time declaration : (
I still think 1.2.0 will work for you if you wanted to build and try that. 
You're right, I built **v1.2.0**, 1d27150 dated Nov 22 2019:
```
$ timew diag
Tag info database does not exist. Recreating from interval data...

timew 1.2.0
       Platform: <unknown>

Compiler:
        Version: 7.3.0
           Caps: +stdc +stdc_hosted +200809 +200809 +LP64 +c8 +i32 +l64 +vp64 +time_t64
     Compliance: C++11

Build Features
          Built: Feb 29 2020 23:49:38
         Commit: 1d27150
          CMake: 3.13.2
     Build type: debug
```
... and it works correctly:
```
$ timew summ :id :year '--- monthly time declaration mark ---'

Wk Date       Day ID   Tags                                                 Start      End    Time   Total
W1 2020-01-03 Fri @826 --- monthly time declaration mark ---, [pro-time] 10:00:07 10:00:08 0:00:01 0:00:01
W5 2020-01-31 Fri @444 --- monthly time declaration mark ---, [pro-time] 23:46:52 23:46:56 0:00:04 0:00:04

                                                                                                   0:00:05
```

I got 1 minute left til the midnight : D
Thank you for the quick responses nevertheless : )
@sskras sorry about that again. I've submitted a pull request with the fix for this, so hopefully you'll be willing to run from the dev branch again after it's merged, or from 1.2.1 when it's released.

Patch:
diff --git a/src/Interval.cpp b/src/Interval.cpp
--- a/src/Interval.cpp
+++ b/src/Interval.cpp
@@ -69,7 +69,7 @@ bool Interval::hasTag (const std::string& tag) const
 }
 
 ////////////////////////////////////////////////////////////////////////////////
-std::set <std::string> Interval::tags () const
+const std::set <std::string>& Interval::tags () const
 {
   return _tags;
 }
diff --git a/src/Interval.h b/src/Interval.h
--- a/src/Interval.h
+++ b/src/Interval.h
@@ -41,7 +41,7 @@ class Interval : public Range
 
   bool empty () const;
   bool hasTag (const std::string&) const;
-  std::set <std::string> tags () const;
+  const std::set <std::string>& tags () const;
   void tag (const std::string&);
   void untag (const std::string&);
 
diff --git a/src/data.cpp b/src/data.cpp
--- a/src/data.cpp
+++ b/src/data.cpp
@@ -24,6 +24,7 @@
 //
 ////////////////////////////////////////////////////////////////////////////////
 
+#include <algorithm>
 #include <deque>
 
 #include <cmake.h>
@@ -586,6 +587,15 @@ std::vector <Range> subtractRanges (
   return results;
 }
 
+////////////////////////////////////////////////////////////////////////////////
+// An interval matches a filter range if the start/end overlaps
+bool matchesRange (const Interval& interval, const Range& filter)
+{
+  return ((filter.start.toEpoch () == 0 &&
+           filter.end.toEpoch () == 0
+          ) || interval.intersects (filter));
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // An interval matches a filter interval if the start/end overlaps, and all
 // filter interval tags are found in the interval.
@@ -625,14 +635,15 @@ std::vector <Range> subtractRanges (
 //
 bool matchesFilter (const Interval& interval, const Interval& filter)
 {
-  if ((filter.start.toEpoch () == 0 &&
-       filter.end.toEpoch () == 0
-     ) || interval.intersects (filter))
+  if (matchesRange (interval, filter))
   {
     for (auto& tag : filter.tags ())
+    {
       if (! interval.hasTag (tag))
+      {
         return false;
-
+      }
+    }
     return true;
   }
 
@@ -696,9 +707,9 @@ std::vector <Interval> getTracked (
     // Since we are moving backwards, and the intervals are in sorted order,
     // if the filter is after the interval, we know there will be no more
     // matches
-    if (matchesFilter (interval, filter))
+    if (matchesRange (interval, filter))
     {
-      intervals.push_front (interval);
+      intervals.push_front (std::move (interval));
     }
     else if (interval.start.toEpoch () >= filter.start.toEpoch ())
     {
@@ -738,14 +749,36 @@ std::vector <Interval> getTracked (
     }
   }
 
-  // Assign an ID to each interval.
+  // Assign an ID to each interval before we prune ones that do not have
+  // matching tags
   for (unsigned int i = 0; i < intervals.size (); ++i)
   {
     intervals[i].id = intervals.size () - i + id_skip;
   }
 
+  // Now prune the intervals without matching tags
+  if (filter.tags ().size () > 0)
+  {
+    auto cmp = [&filter] (const Interval& interval)
+               {
+                 for (const auto& tag : filter.tags ())
+                 {
+                   if (! interval.hasTag (tag))
+                   {
+                     return true;
+                   }
+                 }
+                 return false;
+               };
+
+    intervals.erase (std::remove_if (intervals.begin (), intervals.end (), cmp),
+                     intervals.end ());
+  }
+
   debug (format ("Loaded {1} tracked intervals", intervals.size ()));
-  return subset (filter, intervals);
+
+  return std::vector <Interval> (std::make_move_iterator (intervals.begin ()),
+                                 std::make_move_iterator (intervals.end ()));
 }
 
 ////////////////////////////////////////////////////////////////////////////////
diff --git a/src/timew.h b/src/timew.h
--- a/src/timew.h
+++ b/src/timew.h
@@ -50,6 +50,7 @@ std::vector <Range>     merge             (const std::vector <Range>&);
 std::vector <Range>     addRanges         (const Range&, const std::vector <Range>&, const std::vector <Range>&);
 std::vector <Range>     subtractRanges    (const std::vector <Range>&, const std::vector <Range>&);
 Range                   outerRange        (const std::vector <Interval>&);
+bool                    matchesRange      (const Interval&, const Range&);
 bool                    matchesFilter     (const Interval&, const Interval&);
 Interval                clip              (const Interval&, const Range&);
 std::vector <Interval>  getTracked        (Database&, const Rules&, Interval&);


Test patch:
diff --git a/test/summary.t b/test/summary.t
--- a/test/summary.t
+++ b/test/summary.t
@@ -137,6 +137,24 @@ W10 2017-03-09 Thu @4 Tag1 8:43:08 9:38:15 0:55:07 0:55:07
                                                    0:55:07
 """, out)
 
+    def test_non_contiguous_with_tag_filter(self):
+        """Summary should print data filtered by tag when tags are non-contiguous"""
+        self.t("track Tag1 2017-03-09T08:43:08 - 2017-03-09T09:38:15")
+        self.t("track Tag2 2017-03-09T11:38:39 - 2017-03-09T11:45:35")
+        self.t("track Tag1 Tag3 2017-03-09T11:46:21 - 2017-03-09T12:00:17")
+        self.t("track Tag2 Tag4 2017-03-09T12:01:49 - 2017-03-09T12:28:46")
+
+        code, out, err = self.t("summary 2017-03-09 Tag1 :ids")
+
+        self.assertIn("""
+Wk  Date       Day ID Tags          Start      End    Time   Total
+--- ---------- --- -- ---------- -------- -------- ------- -------
+W10 2017-03-09 Thu @4 Tag1        8:43:08  9:38:15 0:55:07
+                   @2 Tag1, Tag3 11:46:21 12:00:17 0:13:56 1:09:03
+
+                                                           1:09:03
+""", out)
+
     def test_with_day_gap(self):
         """Summary should skip days with no data"""
         self.t("track 2017-03-09T10:00:00 - 2017-03-09T11:00:00")