GSoC Week 15

Feedback on the patches

Finally, I submitted the patches about ten days back (yes, I missed the previous blog because I kind of forgot about it). The patches got fine feedback and required some changes to be good to go.

On t7401: modernise cleanup and warn

The patch was well received though needed some changes in the commit (t7401: modernise style, 2020-07-23). It was better to redirect the output of the rev-parse to a file and then cut the output using cut -c1-7 instead of using the pipe | operator. Also, Taylor Blau and Junio C Hamano suggested I drop the commit (t7401: ensure uniformity in the '--for-status' test, 2020-07-10) since the change it made could be combined with the commit (t7401: change test_i18ncmp syntax for clarity, 2020-07-10). I also introduced a commit (t7401: change indentation for enhanced readability, 2020-08-11) which improves the indentation of the tests in the script for enhanced readability*. Also, the commit (t7401: add a WARNING and a NEEDSWORK, 2020-07-23) was to be changed a bit and keep the NEEDSWORK by making it a bit more verbose and precise, and removing the warning altogether, thus resulting in the commit (t7401: add a NEEDSWORK, 2020-07-23). I thank Denton, Junio and Taylor for reviewing the patch.

*I sent a v2 of the patch to the List as well which got some feedback too. I was wrong with the commit message of this commit. The reason for the whole indentation change is not to enhance readability per se but rather to fix the heredoc marker. A heredoc is a method to define a multiline string. The <<- in <<-EOF tells that this is a heredoc syntax (hence is called the heredoc marker). Therefore, in:

test_expect_success 'typechanged submodule(submodule->blob)' "
	git submodule summary >actual &&
	cat >expected <<-EOF &&
	* sm1 $head4(submodule)->$head5(blob):

	EOF
	test_i18ncmp actual expected
"

The <<- helps us to indent the whole test this way otherwise, if we attempted to bring this all in one line, we would have got a bit weird output.
I will have to improve the patch which modernises the test since the whole idea of storing the output of rev-parse in a file and then reading from it is not very good since the rev-parse call isn’t part of a test but rather a setup step for further tests. Quoting Junio:

If the focus of this test script were to ensure that rev-parse works
correctly, being careful to catch its exit status might have had a
good value, but for that, all the other operations that happen in this
helper function (including the “what happens when the loop body fails
for $name that is not at the end of the argument list?”) must also be
checked for their exit status in the first place.

Since that is not done, and since testing rev-parse should not have to be part of the job for submodule test, the net effect of the above change has quite dubious value—it clobbered a file ‘out’ that may be used by the caller.

on submodule: port subcommand summary from shell to C

The feedback on it was not a lot, but the fix was important. Junio suggested that I base my patch on jk/strvec. The patch by Jeff King is important in the sense that it completely changes the whole argument manipulation we used to do in Git. To explain, earlier, we used to push arguments to an argv_array this way:

argv_array_push(&my_arg, "checkout");

But now, the whole idea of managing argv via the argv_array API can be extended to manage any vector. The vectors are now called strvec instead of argv. The new convention being:

    argv_array => strvec
    argc       => nr
    argv       => v

Along with this, I also decided to drop the commit (submodule: expose the '--for-status' option of summary, 2020-08-02) since it seems a bit extra for now. Thus, the v3 of the patch was sent to the List, which has not received much feedback.

Next steps

I am also working on the git submodule add port, drawing on Prathamesh Chavan’s work. I am almost ready with a successful version of it and need to fix a couple of bugs. I feel that I have attained quite a lot of knowledge after the summary port and touchwood my skills seem a bit better. Last night, I solved multiple bugs in one sitting and was able to understand various functions I had never encountered before. All in all, I feel quite confident now. But my mentors’ feedback on this will matter more than what I think, so let’s see. I also need to fix up t7401 patch and push a v3 of it.

Over and out,
Shourya Shukla