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

[Junio C Hamano <gitster@pobox.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?

> 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;
>  };