Re: [PATCH v7 03/12] for-each-ref: change comment in ref_sort

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 12, 2015, 17:40 UTC
Message-ID: <xmqqy4joddul.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1434039003-10928-3-git-send-email-karthik.188@gmail.com>

Karthik Nayak <karthik.188@gmail.com> writes:

> The comment in 'ref_sort' hasn't been changed 9f613dd.

Bad grammar?  "hasn't been changed since 9f613dd", perhaps?

But more importantly, don't just give an abbreviated object name. I think "the comment hasn't changed since the for-each-ref command was originally introduced" is what you meant to say, and it is OK to append "since 9f613ddd (Add git-for-each-ref: helper for language bindings, 2006-09-15)" to that sentence as a supporting material.

> Change the comment to reflect changes made in the code since
> 9f613dd.

What change since 9f613dd do you have in mind, exactly, though?

I do not think the fact that this field indexes into used_atom[] array has ever changed during the life of this implementation. I see "static const char **used_atom;" in builtin/for-each-ref.c still in the 'master', and that is the array that holds the atoms that are used by the end-user request.

So I do not think "The comment was there from the beginning, it described the initial implementation, the implementation was updated and the comment has become stale" is a good justification for this change, as I do not think that is what has happened here.

You may be changing used_atom to something else later in your series, but then isn't that commit the appropriate place to update this comment?

Show 19 quoted lines

> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>  builtin/for-each-ref.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
> index 0dd2df2..bfad03f 100644
> --- a/builtin/for-each-ref.c
> +++ b/builtin/for-each-ref.c
> @@ -27,7 +27,7 @@ struct atom_value {
>  
>  struct ref_sort {
>  	struct ref_sort *next;
> -	int atom; /* index into used_atom array */
> +	int atom; /* index into 'struct atom_value *' array */
>  	unsigned reverse : 1;
>  };