Re: [Outreachy] Potential intern.

From: Christian Couder <christian.couder@gmail.com>
Date: Oct 5, 2024, 18:22 UTC
Message-ID: <CAP8UFD2QN59LVAJgqwj5V6dJ9sTCHjdDG=XtuWcFkgANmfvnrQ@mail.gmail.com>
In-Reply-To: <CAPSxiM_fTMCzXg3OrcbJrn=UskyCaR3D=xgR4d6PrnnAdoYcCQ@mail.gmail.com>

Hi Usman,

On Sat, Oct 5, 2024 at 6:42 PM Usman Akinyemi <usmanakinyemi202@gmail.com> wrote:

Show 11 quoted lines

>
> Hi Patrick,
>
> Following this, I have gone through [1], [2] and some other resources.
>
> I was able to get potential three interesting MiniProject which I can
> work on. I have also checked through the mailing list to ensure no one
> is working on any of the particular file. As advised, I am sending
> this just to be sure if it’s worth doing and if it’s appropriate for a
> miniproject. I am sending three MiniProjects so I can have one to work
> with in case any of them is not appropriate.

Great, thanks for your interest in working on Git!

Show 20 quoted lines

> 1. Use test_path_is_* functions in test scripts
> An approved sample -
> https://lore.kernel.org/git/20240304095436.56399-2-shejialuo@gmail.com/
>
> Two email threads of discussion and feedback from maintainers.
>
> https://lore.kernel.org/git/CAPig+cR2-6qONkosu7=qEQSJa_fvYuVQ0to47D5qx904zW08Eg@mail.gmail.com/
> https://public-inbox.org/git/CAPig+cRfO8t1tdCL6MB4b9XopF3HkZ==hU83AFZ38b-2zsXDjQ@mail.gmail.com/
>
> Two potential test files which I saw that I can work one.
>
> t/t7003-filter-branch.sh
>
>
> test_expect_success 'test that the file was renamed' '
> test D = "$(git show HEAD:doh --)" &&
> ! test -f D.t &&
> test -f doh &&
> test D = "$(cat doh)"
> '

Yeah, this looks like a good instance where test_path_is_* functions could be used.

Show 11 quoted lines

> t/t2003-checkout-cache-mkdir.sh
>
> test_expect_success 'use --prefix=path2/' '
> rm -fr path0 path1 path2 &&
> mkdir path2 &&
> git checkout-index --prefix=path2/ -f -a &&
> test -f path2/path0 &&
> test -f path2/path1/file1 &&
> test ! -f path0 &&
> test ! -f path1/file1
> '

This looks like another good instance.

> These two are asserting that if a file exists, it can be changed to
> test_file_exist

There is no test_file_exist() function in our test framework as far as I can see. I think you should use test_path_is_file() or test_path_is_missing() to replace `test -f ...` and ` test ! -f ...` respectively.

Show 16 quoted lines

> 2. Avoid suppressing git’s exit code in test scripts
>
> Sample email thread about the same issue.
> https://public-inbox.org/git/pull.885.v2.git.git.1603032125151.gitgitgadget@gmail.com/
>
> First file - t/t6050-replace.sh
> code sample
> test_expect_success 'replace the author' '
> git cat-file commit $HASH2 | grep "author A U Thor" &&
> R=$(git cat-file commit $HASH2 | sed -e "s/A U/O/" | git hash-object
> -t commit --stdin -w) &&
> git cat-file commit $R | grep "author O Thor" &&
> git update-ref refs/replace/$HASH2 $R &&
> git show HEAD~5 | grep "O Thor" &&
> git show $HASH2 | grep "O Thor"
> '

Yeah, I think it would be a good change to remove the pipes after git commands in that code.

Show 12 quoted lines

> Second File - t/t3404-rebase-interactive.sh
> code sample that needs improvement
>
> test_expect_success 'retain authorship' '
> echo A > file7 &&
> git add file7 &&
> test_tick &&
> GIT_AUTHOR_NAME="Twerp Snog" git commit -m "different author" &&
> git tag twerp &&
> git rebase -i --onto primary HEAD^ &&
> git show HEAD | grep "^Author: Twerp Snog"
> '

Yeah, here too, I think it would be a good change to remove the pipes in that code.

Show 9 quoted lines

> 3. Modernise test.
> Description
> https://lore.kernel.org/git/CAPig+cQpUu2UO-+jWn1nTaDykWnxwuEitzVB7PnW2SS_b7V8Hg@mail.gmail.com/
> Sample code t/t7611-merge-abort.sh test_expect_success 'Reset index
> (but preserve worktree changes)' '
> git reset "$pre_merge_head" &&
> git diff > actual &&
> test_cmp expect actual
> '

From the sample, it's not clear that the code would benefit a lot from being modernized, so I would recommend focusing on improving one of the other code you mention above.

Thanks, Christian.