Adjust git handling

- Utilize scoped deletion for individual git_*_free() calls
- Protect against multiple usage when updating icons
- Reduce frequency of git update calls

(cherry picked from commit 26c331a837)

common/git/git_pull_handler.cpp

@@ -23,34 +23,49 @@
 
 #include "git_pull_handler.h"
 #include <git/kicad_git_common.h>
+#include <git/kicad_git_memory.h>
+#include <trace_helpers.h>
 
 #include <wx/log.h>
 
 #include <iostream>
 #include <time.h>
+#include <memory>
 
-GIT_PULL_HANDLER::GIT_PULL_HANDLER( KIGIT_COMMON* aRepo ) :  KIGIT_COMMON( *aRepo )
-{}
+GIT_PULL_HANDLER::GIT_PULL_HANDLER( KIGIT_COMMON* aCommon ) : KIGIT_REPO_MIXIN( aCommon )
+{
+}
 
 
 GIT_PULL_HANDLER::~GIT_PULL_HANDLER()
-{}
-
-
-bool GIT_PULL_HANDLER::PerformFetch()
 {
-    if( !m_repo )
+}
+
+
+bool GIT_PULL_HANDLER::PerformFetch( bool aSkipLock )
+{
+    if( !GetRepo() )
         return false;
 
+    std::unique_lock<std::mutex> lock( GetCommon()->m_gitActionMutex, std::try_to_lock );
+
+    if( !aSkipLock && !lock.owns_lock() )
+    {
+        wxLogTrace( traceGit, "GIT_PULL_HANDLER::PerformFetch() - Could not lock mutex" );
+        return false;
+    }
+
     // Fetch updates from remote repository
     git_remote* remote = nullptr;
 
-    if( git_remote_lookup( &remote, m_repo, "origin" ) != 0 )
+    if( git_remote_lookup( &remote, GetRepo(), "origin" ) != 0 )
     {
         AddErrorString( wxString::Format( _( "Could not lookup remote '%s'" ), "origin" ) );
         return false;
     }
 
+    KIGIT::GitRemotePtr remotePtr( remote );
+
     git_remote_callbacks remoteCallbacks;
     git_remote_init_callbacks( &remoteCallbacks, GIT_REMOTE_CALLBACKS_VERSION );
     remoteCallbacks.sideband_progress = progress_cb;
@@ -58,12 +73,11 @@ bool GIT_PULL_HANDLER::PerformFetch()
     remoteCallbacks.credentials = credentials_cb;
     remoteCallbacks.payload = this;
 
-    m_testedTypes = 0;
+    TestedTypes() = 0;
     ResetNextKey();
 
     if( git_remote_connect( remote, GIT_DIRECTION_FETCH, &remoteCallbacks, nullptr, nullptr ) )
     {
-        git_remote_free( remote );
         AddErrorString( wxString::Format( _( "Could not connect to remote '%s': %s" ), "origin",
                                           git_error_last()->message ) );
         return false;
@@ -75,28 +89,33 @@ bool GIT_PULL_HANDLER::PerformFetch()
 
     if( git_remote_fetch( remote, nullptr, &fetchOptions, nullptr ) )
     {
-        git_remote_free( remote );
         AddErrorString( wxString::Format( _( "Could not fetch data from remote '%s': %s" ),
                                           "origin", git_error_last()->message ) );
         return false;
     }
 
-    git_remote_free( remote );
-
     return true;
 }
 
 
 PullResult GIT_PULL_HANDLER::PerformPull()
 {
-    PullResult result = PullResult::Success;
+    PullResult                   result = PullResult::Success;
+    std::unique_lock<std::mutex> lock( GetCommon()->m_gitActionMutex, std::try_to_lock );
 
-    if( !PerformFetch() )
+    if( !lock.owns_lock() )
+    {
+        wxLogTrace( traceGit, "GIT_PULL_HANDLER::PerformPull() - Could not lock mutex" );
+        return PullResult::Error;
+    }
+
+    if( !PerformFetch( true ) )
         return PullResult::Error;
 
     git_oid pull_merge_oid = {};
 
-    if( git_repository_fetchhead_foreach( m_repo, fetchhead_foreach_cb, &pull_merge_oid ) )
+    if( git_repository_fetchhead_foreach( GetRepo(), fetchhead_foreach_cb,
+                                          &pull_merge_oid ) )
     {
         AddErrorString( _( "Could not read 'FETCH_HEAD'" ) );
         return PullResult::Error;
@@ -104,26 +123,24 @@ PullResult GIT_PULL_HANDLER::PerformPull()
 
     git_annotated_commit* fetchhead_commit;
 
-    if( git_annotated_commit_lookup( &fetchhead_commit, m_repo, &pull_merge_oid ) )
+    if( git_annotated_commit_lookup( &fetchhead_commit, GetRepo(), &pull_merge_oid ) )
     {
         AddErrorString( _( "Could not lookup commit" ) );
         return PullResult::Error;
     }
 
+    KIGIT::GitAnnotatedCommitPtr fetchheadCommitPtr( fetchhead_commit );
     const git_annotated_commit* merge_commits[] = { fetchhead_commit };
-    git_merge_analysis_t merge_analysis;
-    git_merge_preference_t merge_preference = GIT_MERGE_PREFERENCE_NONE;
+    git_merge_analysis_t        merge_analysis;
+    git_merge_preference_t      merge_preference = GIT_MERGE_PREFERENCE_NONE;
 
-    if( git_merge_analysis( &merge_analysis, &merge_preference, m_repo, merge_commits, 1 ) )
+    if( git_merge_analysis( &merge_analysis, &merge_preference, GetRepo(), merge_commits,
+                            1 ) )
     {
         AddErrorString( _( "Could not analyze merge" ) );
-        git_annotated_commit_free( fetchhead_commit );
         return PullResult::Error;
     }
 
-    if( !( merge_analysis & GIT_MERGE_ANALYSIS_NORMAL ) )
-        git_annotated_commit_free( fetchhead_commit );
-
     if( merge_analysis & GIT_MERGE_ANALYSIS_UNBORN )
     {
         AddErrorString( _( "Invalid HEAD.  Cannot merge." ) );
@@ -133,23 +150,26 @@ PullResult GIT_PULL_HANDLER::PerformPull()
     // Nothing to do if the repository is up to date
     if( merge_analysis & GIT_MERGE_ANALYSIS_UP_TO_DATE )
     {
-        git_repository_state_cleanup( m_repo );
+        wxLogTrace( traceGit, "GIT_PULL_HANDLER::PerformPull() - Repository is up to date" );
+        git_repository_state_cleanup( GetRepo() );
         return PullResult::UpToDate;
     }
 
     // Fast-forward is easy, just update the local reference
     if( merge_analysis & GIT_MERGE_ANALYSIS_FASTFORWARD )
     {
+        wxLogTrace( traceGit, "GIT_PULL_HANDLER::PerformPull() - Fast-forward merge" );
         return handleFastForward();
     }
 
     if( merge_analysis & GIT_MERGE_ANALYSIS_NORMAL )
     {
+        wxLogTrace( traceGit, "GIT_PULL_HANDLER::PerformPull() - Normal merge" );
         PullResult ret = handleMerge( merge_commits, 1 );
-        git_annotated_commit_free( fetchhead_commit );
         return ret;
     }
 
+    wxLogTrace( traceGit, "GIT_PULL_HANDLER::PerformPull() - Merge needs resolution" );
     //TODO: handle merges when they need to be resolved
 
     return result;
@@ -175,7 +195,7 @@ std::string GIT_PULL_HANDLER::getFirstLineFromCommitMessage( const std::string&
 
 std::string GIT_PULL_HANDLER::getFormattedCommitDate( const git_time& aTime )
 {
-    char dateBuffer[64];
+    char   dateBuffer[64];
     time_t time = static_cast<time_t>( aTime.time );
     strftime( dateBuffer, sizeof( dateBuffer ), "%Y-%b-%d %H:%M:%S", gmtime( &time ) );
     return dateBuffer;
@@ -184,92 +204,80 @@ std::string GIT_PULL_HANDLER::getFormattedCommitDate( const git_time& aTime )
 
 PullResult GIT_PULL_HANDLER::handleFastForward()
 {
-        // Update local references with fetched data
-        auto git_reference_deleter =
-                []( void* p )
-                {
-                    git_reference_free( static_cast<git_reference*>( p ) );
-                };
+    git_reference* rawRef = nullptr;
 
-        git_reference* rawRef = nullptr;
+    if( git_repository_head( &rawRef, GetRepo() ) )
+    {
+        AddErrorString( _( "Could not get repository head" ) );
+        return PullResult::Error;
+    }
 
-        if( git_repository_head( &rawRef, m_repo ) )
+    KIGIT::GitReferencePtr headRef( rawRef );
+
+    const char* updatedRefName = git_reference_name( rawRef );
+
+    git_oid updatedRefOid;
+
+    if( git_reference_name_to_id( &updatedRefOid, GetRepo(), updatedRefName ) )
+    {
+        AddErrorString( wxString::Format( _( "Could not get reference OID for reference '%s'" ),
+                                          updatedRefName ) );
+        return PullResult::Error;
+    }
+
+    git_checkout_options checkoutOptions;
+    git_checkout_init_options( &checkoutOptions, GIT_CHECKOUT_OPTIONS_VERSION );
+    checkoutOptions.checkout_strategy = GIT_CHECKOUT_SAFE;
+
+    if( git_checkout_head( GetRepo(), &checkoutOptions ) )
+    {
+        AddErrorString( _( "Failed to perform checkout operation." ) );
+        return PullResult::Error;
+    }
+
+    // Collect commit details for updated references
+    git_revwalk* revWalker = nullptr;
+    git_revwalk_new( &revWalker, GetRepo() );
+    KIGIT::GitRevWalkPtr revWalkerPtr( revWalker );
+    git_revwalk_sorting( revWalker, GIT_SORT_TIME );
+    git_revwalk_push_glob( revWalker, updatedRefName );
+
+    git_oid commitOid;
+
+    while( git_revwalk_next( &commitOid, revWalker ) == 0 )
+    {
+        git_commit* commit = nullptr;
+
+        if( git_commit_lookup( &commit, GetRepo(), &commitOid ) )
         {
-            AddErrorString( _( "Could not get repository head" ) );
+            AddErrorString( wxString::Format( _( "Could not lookup commit '{}'" ),
+                                              git_oid_tostr_s( &commitOid ) ) );
             return PullResult::Error;
         }
 
-        // Defer destruction of the git_reference until this function exits somewhere, since
-        // updatedRefName etc. just point to memory inside the reference
-        std::unique_ptr<git_reference, decltype( git_reference_deleter )> updatedRef( rawRef );
+        KIGIT::GitCommitPtr commitPtr( commit );
 
-        const char* updatedRefName = git_reference_name( updatedRef.get() );
+        CommitDetails details;
+        details.m_sha = git_oid_tostr_s( &commitOid );
+        details.m_firstLine = getFirstLineFromCommitMessage( git_commit_message( commit ) );
+        details.m_author = git_commit_author( commit )->name;
+        details.m_date = getFormattedCommitDate( git_commit_author( commit )->when );
 
-        git_oid     updatedRefOid;
+        std::pair<std::string, std::vector<CommitDetails>>& branchCommits =
+                m_fetchResults.emplace_back();
+        branchCommits.first = updatedRefName;
+        branchCommits.second.push_back( details );
+    }
 
-        if( git_reference_name_to_id( &updatedRefOid, m_repo, updatedRefName ) )
-        {
-            AddErrorString( wxString::Format( _( "Could not get reference OID for reference '%s'" ),
-                                              updatedRefName ) );
-            return PullResult::Error;
-        }
-
-        git_checkout_options checkoutOptions;
-        git_checkout_init_options( &checkoutOptions, GIT_CHECKOUT_OPTIONS_VERSION );
-        checkoutOptions.checkout_strategy = GIT_CHECKOUT_SAFE;
-
-        if( git_checkout_head( m_repo, &checkoutOptions ) )
-        {
-            AddErrorString( _( "Failed to perform checkout operation." ) );
-            return PullResult::Error;
-        }
-
-        // Collect commit details for updated references
-        git_revwalk* revWalker = nullptr;
-        git_revwalk_new( &revWalker, m_repo );
-        git_revwalk_sorting( revWalker, GIT_SORT_TIME );
-        git_revwalk_push_glob( revWalker, updatedRefName );
-
-        git_oid commitOid;
-
-        while( git_revwalk_next( &commitOid, revWalker ) == 0 )
-        {
-            git_commit* commit = nullptr;
-
-            if( git_commit_lookup( &commit, m_repo, &commitOid ) )
-            {
-                AddErrorString( wxString::Format( _( "Could not lookup commit '{}'" ),
-                                                  git_oid_tostr_s( &commitOid ) ) );
-                git_revwalk_free( revWalker );
-                return PullResult::Error;
-            }
-
-            CommitDetails details;
-            details.m_sha = git_oid_tostr_s( &commitOid );
-            details.m_firstLine = getFirstLineFromCommitMessage( git_commit_message( commit ) );
-            details.m_author = git_commit_author( commit )->name;
-            details.m_date = getFormattedCommitDate( git_commit_author( commit )->when );
-
-            std::pair<std::string, std::vector<CommitDetails>>& branchCommits =
-                    m_fetchResults.emplace_back();
-            branchCommits.first = updatedRefName;
-            branchCommits.second.push_back( details );
-
-            //TODO: log these to the parent
-            git_commit_free( commit );
-        }
-
-        git_revwalk_free( revWalker );
-
-        git_repository_state_cleanup( m_repo );
-        return PullResult::FastForward;
+    git_repository_state_cleanup( GetRepo() );
+    return PullResult::FastForward;
 }
 
 
 PullResult GIT_PULL_HANDLER::handleMerge( const git_annotated_commit** aMergeHeads,
                                           size_t                       aMergeHeadsCount )
 {
-    git_merge_options    merge_opts;
+    git_merge_options merge_opts;
     git_merge_options_init( &merge_opts, GIT_MERGE_OPTIONS_VERSION );
 
     git_checkout_options checkout_opts;
@@ -277,7 +285,7 @@ PullResult GIT_PULL_HANDLER::handleMerge( const git_annotated_commit** aMergeHea
 
     checkout_opts.checkout_strategy = GIT_CHECKOUT_SAFE;
 
-    if( git_merge( m_repo, aMergeHeads, aMergeHeadsCount, &merge_opts, &checkout_opts ) )
+    if( git_merge( GetRepo(), aMergeHeads, aMergeHeadsCount, &merge_opts, &checkout_opts ) )
     {
         AddErrorString( _( "Could not merge commits" ) );
         return PullResult::Error;
@@ -286,12 +294,14 @@ PullResult GIT_PULL_HANDLER::handleMerge( const git_annotated_commit** aMergeHea
     // Get the repository index
     git_index* index = nullptr;
 
-    if( git_repository_index( &index, m_repo ) )
+    if( git_repository_index( &index, GetRepo() ) )
     {
         AddErrorString( _( "Could not get repository index" ) );
         return PullResult::Error;
     }
 
+    KIGIT::GitIndexPtr indexPtr( index );
+
     // Check for conflicts
     git_index_conflict_iterator* conflicts = nullptr;
 
@@ -301,9 +311,11 @@ PullResult GIT_PULL_HANDLER::handleMerge( const git_annotated_commit** aMergeHea
         return PullResult::Error;
     }
 
-    const git_index_entry* ancestor = nullptr;
-    const git_index_entry* our = nullptr;
-    const git_index_entry* their = nullptr;
+    KIGIT::GitIndexConflictIteratorPtr conflictsPtr( conflicts );
+
+    const git_index_entry*    ancestor = nullptr;
+    const git_index_entry*    our = nullptr;
+    const git_index_entry*    their = nullptr;
     std::vector<ConflictData> conflict_data;
 
     while( git_index_conflict_next( &ancestor, &our, &their, conflicts ) == 0 )
@@ -362,14 +374,11 @@ PullResult GIT_PULL_HANDLER::handleMerge( const git_annotated_commit** aMergeHea
         git_index_write( index );
     }
 
-    git_index_conflict_iterator_free( conflicts );
-    git_index_free( index );
-
     return conflict_data.empty() ? PullResult::Success : PullResult::MergeFailed;
 }
 
 
 void GIT_PULL_HANDLER::UpdateProgress( int aCurrent, int aTotal, const wxString& aMessage )
 {
-    ReportProgress( aCurrent, aTotal, aMessage );
+
 }