12
\$\begingroup\$

I've recently taken an interest in analyzing commit history to find files with a lot of churn. The idea being that high churn files are likely targets for refactoring. Right now it's a simple console app that returns the number of times a given file was committed. (Renames are not currently followed. It's a known area for improvement.)

The basic design is to have a visitor walk the commit graph and raise events for a listener to gather information about them. Currently there is a single listener that diffs each commit with its parents and caches those diffs. This should be flexible enough to create another one that, say, collects committer stats though.

GitNStats v1.0.1 is on Github if you prefer to view it there.

Visitor.cs

using LibGit2Sharp;

namespace GitNStats
{
    public abstract class Visitor
    {
        public delegate void VisitedHandler(CommitVisitor visitor, Commit commit);
        public event VisitedHandler Visited;

        public abstract void Walk(Commit commit);

        protected virtual void OnVisited(CommitVisitor visitor, Commit commit)
        {
            Visited?.Invoke(visitor, commit);
        }
    }
}

CommitVisitor.cs

Git's commit history isn't a tree, it's a graph and we don't want to visit any commit more than once, so this class needs to keep track of the commits it's already visited.

using System;
using System.Collections.Generic;
using LibGit2Sharp;

namespace GitNStats
{
    /// <summary>
    /// Walks the commit graph back to the beginning of time.
    /// Guaranteed to only visit a commit once.
    /// </summary>
    public class CommitVisitor : Visitor
    {
        /// <summary>
        /// Walk the graph from this commit back.
        /// </summary>
        /// <param name="commit">The commit to start at.</param>
        public override void Walk(Commit commit)
        {
            Walk(commit, new HashSet<string>());
        }

        private void Walk(Commit commit, ISet<string> visited)
        {
            if (visited.Contains(commit.Sha))
            {
                // Exit so we don't walk the same path multiple times.
                return;
            }

            visited.Add(commit.Sha);
            OnVisited(this, commit);

            foreach(var parent in commit.Parents)
            {
                Walk(parent, visited);
            }
        }
    }

Listener.cs

using LibGit2Sharp;

namespace GitNStats
{
    /// <summary>
    /// Listens for a <see cref="Visitor"/> to raise the <see cref="Visitor.Visited"/> event.
    /// </summary>
    public interface Listener
    {
        /// <summary>
        /// To be executed whenever a visitor vistis a commit.
        /// </summary>
        /// <param name="visitor">The <see cref="Visitor"/> that raised the <see cref="Visitor.Visited"/> event.</param>
        /// <param name="visited">The <see cref="Commit"/> currently being visited.</param>
        void OnCommitVisited(Visitor visitor, Commit visited);
    }
}

DiffListener.cs

This is the class performs a diff that is roughly equivalent to a git log --stat and caches them for later use.

using System;
using System.Collections.Generic;
using System.Collections.Concurrent;

using LibGit2Sharp;

namespace GitNStats
{
    /// <summary>
    /// When a Commit is visited, compares that commit to it's parents 
    /// and stores the resulting TreeEntryChanges in the <see cref="Diffs"/> property.
    /// </summary>
    public class DiffListener : Listener
    {
        private readonly IRepository _repository;
        private readonly ConcurrentBag<TreeEntryChanges> _diffs = new ConcurrentBag<TreeEntryChanges>();

        /// <summary>
        /// The diff cache. 
        /// Clients should wait until the <see cref="Visitor"/> is done walking the graph before accessing.
        /// </summary>
        public IEnumerable<TreeEntryChanges> Diffs => _diffs;

        public DiffListener(IRepository repository)
        {
            _repository = repository;
        }

        /// <summary>
        /// Compares the <paramref name="visited"/> commit to it's parents and caches the diffs in <see cref="Diffs"/>.
        /// </summary>
        /// <param name="visitor">The <see cref="Visitor"/> that raised the <see cref="Visitor.Visited"/> event.</param>
        /// <param name="visited">The <see cref="Commit"/> currently being visited.</param>
        public void OnCommitVisited(Visitor visitor, Commit visited)
        {
            foreach (var parent in visited.Parents)
            {
                var diff = _repository.Diff.Compare<TreeChanges>(parent.Tree, visited.Tree);

                foreach (var changed in diff)
                {
                    _diffs.Add(changed);
                }
            }
        }
    }
}

Program.cs

Last, but not least, the main program. (And easily the code most in need of some love.)

using System;
using LibGit2Sharp;
using System.Linq;
using System.IO;
using CommandLine;

namespace GitNStats
{
    class Program
    {
        public static int Main(string[] args)
        {
            return Parser.Default.ParseArguments<Options>(args)
                .MapResult(options =>
                {
                    var repoPath = String.IsNullOrWhiteSpace(options.RepositoryPath)
                        ? Directory.GetCurrentDirectory()
                        : options.RepositoryPath;

                    return RunAnalysis(repoPath, options.BranchName);
                }, _ => 1);
        }

        private static int RunAnalysis(string repositoryPath, string branchName)
        {
            try
            {
                using (var repo = new Repository(repositoryPath))
                {
                    var branch = (branchName == null) ? repo.Head : repo.Branches[branchName];
                    if (branch == null)
                    {
                        WriteError($"Invalid branch: {branchName}");
                        return 1;
                    }

                    Console.WriteLine($"Repository: {repositoryPath}");
                    Console.WriteLine($"Branch: {branch.FriendlyName}");
                    Console.WriteLine();

                    var listener = new DiffListener(repo);
                    var visitor = new CommitVisitor();
                    visitor.Visited += listener.OnCommitVisited;

                    visitor.Walk(branch.Tip);

                    var changeCounts = listener.Diffs
                        .GroupBy(c => c.Path)
                        .Select(x => new {Path = x.Key, Count = x.Count()})
                        .OrderByDescending(s => s.Count);

                    Console.WriteLine("Commits\tPath");
                    foreach (var summary in changeCounts)
                    {
                        Console.WriteLine($"{summary.Count}\t{summary.Path}");
                    }
                    return 0;
                }
            }
            catch (RepositoryNotFoundException)
            {
                WriteError($"{repositoryPath} is not a git repository.");
                return 1;
            }
        }

        private static void WriteError(string message)
        {
            var currentColor = Console.ForegroundColor;
            Console.ForegroundColor = ConsoleColor.Red;
            try
            {
                Console.Error.WriteLine(message);
            }
            finally
            {
                Console.ForegroundColor = currentColor;
            }
        }
    }
}
\$\endgroup\$
0

2 Answers 2

7
\$\begingroup\$

CommitVisitor

Using Set<T>.Contains() and returning if it evaluates to truemakes the intent of the code more clear but internally if the element isn't in the Set<T> a second check is performed by calling Add(). So if speed should be an issue you should consider to check

if (!visited.Add(commit.Sha)) { return; }  

Listener

I don't quite get it. In the XML doc of the interface it states

Listens for a <see cref="Visitor"/> to raise the <see cref="Visitor.Visited"/> event.

which would maybe make sense if we look at the name of the only method which needs to be implemented but usually I wouldn't expect an OnXXX method to be part of an interface but be a protected or even private method of a class which purpose is to just raise the event.


\$\endgroup\$
1
  • \$\begingroup\$ It's YAGNI. I was thinking I might want to use reflection to load listeners later. Truth be told, I don't even need that interface. Thanks for the review Heslacher. \$\endgroup\$
    – RubberDuck
    Jul 10, 2017 at 10:07
3
\$\begingroup\$
public abstract class Visitor
{
    public delegate void VisitedHandler(CommitVisitor visitor, Commit commit);

Well, having your abstract class rely on its inheritors like that makes it pretty much useless. This should be declared like this.

public abstract class Visitor
{
    public delegate void VisitedHandler(Visitor visitor, Commit commit);
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.