Eric Sunshine <sunshine@xxxxxxxxxxxxxx> writes: > Thanks for submitting this GSoC microproject. See comments below... > > On Tue, Mar 18, 2025 at 7:58 AM Sampriyo Guin via GitGitGadget > <gitgitgadget@xxxxxxxxx> wrote: >> From: rimo <sampriyoguin@xxxxxxxxx> > > This name should match the Signed-off-by: name. Since the "From:" > header is generated from the author information in the commit, you > probably need to adjust your "user.name" configuration to fix this. > >> test -e changed to test_path_exists >> test -f changed to test_path_is_file > > People reading the patch would like to know why a change is being > made, so this is where you should explain the reason (for instance, > "the test_path_* functions provide better diagnostics upon failure" or > such). As Karthik mentioned[*], read the "Describe your changes well" > section in Documentation/SubmittingPatches to learn how to craft a > good commit message. > > [*]: https://lore.kernel.org/git/CAOLa=ZSkMp+H9PZeBZXK47=fx1sH=S54AuPT=oUosm7F7V8MGg@xxxxxxxxxxxxxx/ > >> Signed-off-by: Sampriyo Guin <sampriyoguin@xxxxxxxxx> >> --- >> , Jialuo She shejialuo@xxxxxxxxx , Christian Couder >> christian.couder@xxxxxxxxx, Ghanshyam Thakkar shyamthakkar001@xxxxxxxxx > > It appears that GitGitGadget didn't like how this list was formatted. > Instead, place each recipient on its own Cc: line. > >> t/chainlint/chained-subshell.expect | 2 +- >> t/chainlint/chained-subshell.test | 2 +- >> t/chainlint/function.expect | 2 +- >> t/chainlint/function.test | 2 +- >> 4 files changed, 4 insertions(+), 4 deletions(-) > > Let's not touch any of the "chainlint" files; they are checking > validity of a completely separate tool ("chainlint"), and have nothing > to do with checking Git itself. Instead, pick one of the t/t???-*.sh > files. Yeah, these changes to make them use test_path_* are not "fixes" but something else. The first step for a contributor is to understand why "test_path_*" are preferred over "test -[def]" and in what context, but touching these files shows that such understanding is missing, unfortunately. I find the "as specified in Git Microprojects" in the patch description the most disturbing, A simple fix as specified in Git Microprojects. as it may be an indication that some introductory write-up is misleading potential students in a wrong direction. Our mentors may need a bit more handholding at this early stage of dipping your toes in the water step, perhaps? Or is it up to the aspiring students to do their homework?
