GSoC Week 9

First evaluation complete!

Last night, the results of the first evaluation came in and I was so happy to see a “Congratulations! You have passed the first evaluation”. It felt really good to see it! A huge thanks to Christian and Kaartic for believing in me :)

First evaluation result

This does call for a small celebration! After all this is the first time I have earned money from my work! A salary I guess ;)

After quite some debugging, finally summary worked. The problem was a bit more skewed than it seemed and was far more serious than just for-status or prepare_submodule_summary(). I will explain what happened and how we waded through to make things first.

The Problem

As I said above, the problem was far more serious than just an option or a function. I will touch t7418 to make you familiar with what is going on. The first test in t7418creates a Git repository in a directory called upstream and a submodule called… well submodule. The submodule is added to the repository (everything is normal here no problem; this is the way it should have been done). Now, this whole setup is cloned into a directory at the same level called super (this is where the problem begins). When repositories with submodules are cloned, the submodule(s) in the repository have a gitfile instead of a .git directory like one would have expected. This is called a .gitfile and contains the path $GIT_DIR/modules/<submodle-name>/ therefore in our case it was .git/modules/submodule (confirmed by a cat super/submodule/.git); the aforementioned path is relative to the superproject’s root. Mind you this is NOT a directory but rather a file, hence running checking if it is a $GIT_DIR will always result in a failure. This is something we had been doing in the section:

if (is_git_directory(sm_git_dir))
		is_sm_git_dir = 1;

Here, our case always failed since it was a gitfile and where is_sm_git_dir was set to 0 only therefore failing some of the if statements following after; sections which would lead to the calculation of the total commits to display the git log --oneline of, therefore failure to reach these sections would result in total_commits being set to -1 and therefore resulting in no commits being tallied for output therefore justifying the what we got before (quoting from the previous blog):

$ git submodule summary 7c1532cab3125ae24f14ee9433b3c673f2964ef1
* bash 4f90705...f10c25c:

This is something I feel needs to be fixed with t7401 as well since the test only creates a scenario where we add a submodule which resides in a neighbouring directory therefore not involving any cases with the presence of a gitfile.

To make things clear, this is the structure of the whole testing space. Our space contains the submodule submodule, a superproject upstream containing submodule and a clone of the superproject called super.

├── submodule     # submodule
	└── .git  # this is a gitdir
├── super
│   └── submodule
│       └── .git  # this is a gitfile
			   └── gitdir: ../.git/modules/submodule
└── upstream
    └── submodule
        └── .git  # this is a gitfile

To get such type of an output, use tree -a in the parent directory.

The fix

Fixing this required expanding the domain of the check which was taking place above. It is obvious now that checking our submodule to be a gitdir will result in pretty devastating failures for almost any real world scenario of cloning a repository; it wouldn’t have worked even for the Git source code which contains the sha1collisiondetection submodule!

After a very detailed investigation with my mentors, we came up with a fix to check for a non-bare repository instead of a gitdir. Now, since a gitdir is a non-bare repository and a submodule with gitfile will be a non-bare repository as well, we will be able to cover a wider use-case with this. Therefore, the sameif-check mentioned before is now modified to:

strbuf_addstr(&sm_git_dir_sb,  p->sm_path);
if (is_nonbare_repository_dir(&sm_git_dir_sb))
	is_sm_git_dir = 1;

Now, everything works fine and t7418 and t7401 pass.

Other touchups

There were some comments by Christian on the v2 by PC. These were not making the enum static and passing the aforementioned enum as an argument to compute_summary_module_list() since the enum is used only by this function and module_summary(). There were some other changes as well such as correcting the parameters of various functions such as index_fd() etc.

Next steps

Since summary is resolved now, I posted it onto the List. Now, the job left is to keep supporting the patch. I will also start working on the port of git submodule add on the side. Before starting that, I will address the comments by Johannes and Philip on my patch.

Over and out,
Shourya Shukla