Re: Log caching: Crash in `Get merge logs' without access to the repository root

On 14 March 2016 at 13:33, Ivan Zhakov <[email protected]> wrote:
> On 11 March 2016 at 23:54, Stefan Küng <[email protected]> wrote:
>> On Fri, Mar 11, 2016 at 6:40 PM, Stefan Kueng <[email protected]> wrote:
>>
>>>
>>> Patch looks good!
>>> Please commit.
>>>
>>> But while I was going through the code and already made some similar
>>> changes as in your patch I got an idea:
>>>
>>> maybe we could remove the separate handling of logs with/without merge
>>> info completely and just use one code path for all situations?
>>>
>>> This would mean getting rid of the call to InternalLogWithMerge() and call
>>> InternalLog() instead. Then we could also remove the CMergeLogger() class.
>>>
>>> I'm doing some testing with that change (not finished yet) soon.
>>> But in the mean time: what do you think?
>>>
>>>
>>
>> Actually, there's more needed to make it work properly. Your patch fixes the
>> crash, that's good.
>> But if there are actually merged revisions shown/fetched, then the skip
>> range gets messed up.
> My bad, I missed that.
>
>> We have to 'ignore' all the merged revs that come in and not use them for
>> book keeping. We only have to update the cache and report those revisions
>> back to the caller, but *not* use them for e.g. updating the skip ranges.
>> If we do, then instead of one log call we get at least one more, maybe even
>> two.
>>
>> I have something working so far, but before I commit I'd like to do some
>> more testing.
>> I'll commit my change tomorrow if I don't find any other problems...
>>
> There are cases when that wouldn't work, since the merge-aware log is
> very different. For example 'svn log -g -c 27193
> https://svn.code.sf.net/p/tortoisesvn/code/branches/1.9.x' reports the
> following revisions:
> [[[
> r27193 | steveking | 2016-02-22 21:25:45
> ------------------------------------------------------------------------
> r27179 | ivan-z | 2016-02-22 13:09:20
> Merged via: r27193
> ------------------------------------------------------------------------
> r27177 | ivan-z | 2016-02-19 18:30:56
> Merged via: r27193
> ------------------------------------------------------------------------
> r27176 | ivan-z | 2016-02-19 18:28:26
> Merged via: r27193
> ------------------------------------------------------------------------
> r27175 | ivan-z | 2016-02-19 18:27:41
> Merged via: r27193
> ------------------------------------------------------------------------
> r27169 | steveking | 2016-02-10 22:22:11
> Merged via: r27193
> ------------------------------------------------------------------------
> r27168 | steveking | 2016-02-10 22:20:50
> Merged via: r27193
> ------------------------------------------------------------------------
> r27167 | steveking | 2016-02-08 23:27:58
> Merged via: r27193
> ]]]
>
> But log caching doesn't have information about merges and would report
> just r27193 if it's cached. To reproduce with TortoiseSVN trunk@27240:
> 1. Enable Log Caching
> 2. Start TortoiseSVN log for
> https://svn.code.sf.net/p/tortoisesvn/code/branches/1.9.x:
>     TortoiseProc.exe /command:log
> /path:https://svn.code.sf.net/p/tortoisesvn/code/branches/1.9.x
> /startrev:27193 /endrev:27193
> 3. Click "Include merged revisions"
>    Merged revisions will not be displayed, while they should. They may
> appear after pressing F5, but they will not be reported as merged.
>
> I think the best solution would be just to skip log caching completely
> for merge-aware logs. I'm going to prepare patch for this.
>
Here is the patch. Changes from trunk:
- Do not populate skip ranges from merge-aware log results.
- Request all range for merge-aware logs. Merge-aware log cannot be
requested by sub-ranges due to non-linear nature of merge-aware logs.


-- 
Ivan Zhakov

------------------------------------------------------
http://tortoisesvn.tigris.org/ds/viewMessage.do?dsForumId=757&dsMessageId=3165849

To unsubscribe from this discussion, e-mail: [[email protected]].

Index: src/LogCache/CacheLogQuery.cpp
===================================================================
--- src/LogCache/CacheLogQuery.cpp	(revision 27240)
+++ src/LogCache/CacheLogQuery.cpp	(working copy)
@@ -160,7 +160,7 @@
 
 void CCacheLogQuery::CLogFiller::AutoAddSkipRange (revision_t revision)
 {
-    if ((depth == 0) && (firstNARevision > revision) && (currentPath.get() != NULL))
+    if (!options.GetIncludeMerges() && (firstNARevision > revision) && (currentPath.get() != NULL))
     {
         // due to only the parent path being renamed, the currentPath
         // may not have shown up in the log -> don't mark the range
@@ -1049,6 +1049,38 @@
     }
 }
 
+void CCacheLogQuery::InternalLogWithMerge(revision_t startRevision
+                                          , revision_t endRevision
+                                          , const CDictionaryBasedTempPath& startPath
+                                          , int limit
+                                          , const CLogOptions& options)
+{
+    // clear string translation caches
+
+    ResetObjectTranslations();
+
+    // fetch revisions only but include merge children
+
+    CTSVNPath path;
+    if (startPath.IsRoot())
+        path.SetFromSVN(URL);
+    else
+        path.SetFromSVN(URL + startPath.GetPath().c_str());
+
+    // Request all interesting revisions: we cannot use log caching for
+    // merge logs.
+    CLogFiller(repositoryInfoCache)
+        .FillLog(cache
+            , URL
+            , uuid
+            , svnQuery
+            , startRevision
+            , endRevision
+            , startPath
+            , limit
+            , options);
+}
+
 // follow copy history until the startRevision is reached
 
 CDictionaryBasedTempPath CCacheLogQuery::TranslatePegRevisionPath
@@ -1442,11 +1474,22 @@
                         , includeUserRevProps
                         , userRevProps);
 
-    InternalLog ( startRevision
-                , endRevision
-                , startPath
-                , limit
-                , options);
+    if (options.GetIncludeMerges())
+    {
+        InternalLogWithMerge(startRevision
+                             , endRevision
+                             , startPath
+                             , limit
+                             , options);
+    }
+    else
+    {
+        InternalLog(startRevision
+                    , endRevision
+                    , startPath
+                    , limit
+                    , options);
+    }
 }
 
 // relay the content of a single revision to the receiver
Index: src/LogCache/CacheLogQuery.h
===================================================================
--- src/LogCache/CacheLogQuery.h	(revision 27240)
+++ src/LogCache/CacheLogQuery.h	(working copy)
@@ -304,6 +304,12 @@
                      , int limit
                      , const CLogOptions& options);
 
+    void InternalLogWithMerge(revision_t startRevision
+                              , revision_t endRevision
+                              , const CDictionaryBasedTempPath& startPath
+                              , int limit
+                              , const CLogOptions& options);
+
     /// follow copy history until the startRevision is reached
     CDictionaryBasedTempPath TranslatePegRevisionPath
         ( revision_t pegRevision