git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
Cc: git@vger.kernel.org,
	 Christian Couder <christian.couder@gmail.com>,
	Phillip Wood <phillip.wood123@gmail.com>,
	 Josh Steadmon <steadmon@google.com>,
	 Christian Couder <chriscool@tuxfamily.org>,
	Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
Subject: Re: [GSoC][PATCH v3] t: migrate helper/test-oidmap.c to unit-tests/t-oidmap.c
Date: Wed, 03 Jul 2024 09:20:47 -0700	[thread overview]
Message-ID: <xmqqle2ie9b4.fsf@gitster.g> (raw)
In-Reply-To: <20240703062958.23262-2-shyamthakkar001@gmail.com> (Ghanshyam Thakkar's message of "Wed, 3 Jul 2024 11:59:53 +0530")

Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:

> helper/test-oidmap.c along with t0016-oidmap.sh test the oidmap.h
> library which is built on top of hashmap.h.
>
> Migrate them to the unit testing framework for better performance,
> concise code and better debugging. Along with the migration also plug
> memory leaks and make the test logic independent for all the tests.
> The migration removes 'put' tests from t0016, because it is used as
> setup to all the other tests, so testing it separately does not yield
> any benefit.
>

> Helped-by: Junio C Hamano <gitster@pobox.com>
> Helped-by: Phillip Wood <phillip.wood123@gmail.com>
> Reviewed-by: Josh Steadmon <steadmon@google.com>
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
> Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>

The trailer lines should come more-or-less in chronological order.
I do not know exact sequence of events, but mentoring would have
happened first before the initial iteration was posted, then Phillip
helped to improve it during iterations, the latest was reviewed by
Josh and then you folded the little test improvement from me and
Phillip?  And to seal the whole thing off, you add your sign-off at
the end.

Technically speaking, any change after a review invalidates an
earlier "Reviewed-by", but the updates we see here, relative to the
iteration that received the "Reviewed-by", are not significant or
large enough to warrant that, so let's pretend that Josh would be
happy with the end result, even with our little additions.

Which leads us to the trailer lines ordered like so:

    Mentored-by: Christian Couder <chriscool@tuxfamily.org>
    Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
    Helped-by: Phillip Wood <phillip.wood123@gmail.com>
    Helped-by: Junio C Hamano <gitster@pobox.com>
    Reviewed-by: Josh Steadmon <steadmon@google.com>
    Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>

The changes since the previous round look exactly as expected.
Looking very good.

Will queue.  Let's mark it for 'next'.

Thanks.



  parent reply	other threads:[~2024-07-03 16:20 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-07-03  6:29 [GSoC][PATCH v3] t: migrate helper/test-oidmap.c to unit-tests/t-oidmap.c Ghanshyam Thakkar
2024-07-03  7:04 ` Ghanshyam Thakkar
2024-07-03 16:20 ` Junio C Hamano [this message]
2024-07-04  9:44   ` Phillip Wood

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqle2ie9b4.fsf@gitster.g \
    --to=gitster@pobox.com \
    --cc=chriscool@tuxfamily.org \
    --cc=christian.couder@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=kaartic.sivaraam@gmail.com \
    --cc=phillip.wood123@gmail.com \
    --cc=shyamthakkar001@gmail.com \
    --cc=steadmon@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).