Re: [PATCH v2 1/2] for-each-ref: re-structure code for moving to 'ref-filter'

[Karthik Nayak <karthik.188@gmail.com>]

 >
> I do not see much point in renaming between these two.  The latter
> makes it sound as if this is only for "filtering" and from that
> angle of view is probably a worse name.  If you do not think of a
> better one, and if you are going to name the array that contains
> this thing "ref_list", calling "ref_list_item" would be following
> suit to what string-list did.
 >

Well I just wanted to keep it related to 'ref-filter', I think
'ref_list_item'
sounds better after seeing your point of view.

>
> I somehow had an impression that we are trying to move away from
> calling the name of objects as "sha1[]" as a longer term goal?  I do
> not think it is particularly a good idea to start using "struct
> object_id" in this series (it can be done after everything is done
> and you still have time left in GSoC), but I do not see how much
> value this interim renaming (because eventually we would change not
> just name but type, and the final name will _not_ be sha1[] but more
> closer to "object name") adds value.
 >

I did that to resemble whats usually being used in similar structures,
a simple grep of "sha1[20];" resulted in 344 uses.
I didn't know about the "we are trying to move away from calling the 
name of objects as "sha1[]"". Will leave it as objectname then.

>
> You didn't explain why you reordered the fields, either.  Were you
> planning to make the name[] field to flex-array to reduce need for
> one level of redirection or something?
>

Yes! exactly why the re-order, was going to rebase it and squash it in, 
if the code seemed to be up and running.


>
> I agree that "grab" part of "grab_ref_cbdata" sounds unprofessional,
> and using "ref_filter_" to signal that this callback data is about
> ref-filter API might be a good idea (I do not think the original is
> too bad, either, though).  I do not think you would use this struct
> anywhere other than as the callback data; you would want to end its
> name with "_cbdata", not just "_data", to make it clear why these
> two unrelated things are in a single struct (the only time these two
> concepts, operation and operatee, meet is when they need to be
> passed to an "apply operation to operatee" function; there is no
> such "this set of operatee always are for this operation"
> association between them---which is what I mean by 'two unrelated
> things').
>

sure, will do :) thanks for putting that out.

>
> It was perfectly good name as a file-scope static; within the
> context of 'for-each-ref' implementation, when every somebody says
> "atom", you would know it is those %(atomic-data-item) things, and
> parse_atom() would be a helper function to do so.
>
> But it is *WAY* too generic a name to make public, where you are
> naming things in the whole context of Git implementation.  If you
> used the word "atom" while discussing formatting done with "git
> for-each-ref" with somebody else, it would be unambiguously clear
> what you mean; you wouldn't say "I am writing a function to parse
> 'atoms' in Git"---that's too broad and you will get "'atom', in what
> sense?" in response.
>

>
> Ditto.
>

Yes, that does seem to be too vague for a public function name, will 
amend it.

>
> As long as this will stay private within the new ref-filter.c after
> the move, this name is OK.
>

That'll mostly stay private, if required will change the name along.

>
> I see fallouts from the two renamed fields in the above hunks.  Was
> the rename necessary?
>
> refinfo keeps two names (ref and object) and calling one "refname"
> made perfect sense (and calling other "objectname" did, too).  Has
> anything around its use changed to invalidate that rationale after
> the structure was renamed?
>

I guess it was unnecessary, my bad.

 >
> When we say 'flag', it is obvious that it is a "flag word", i.e. a
> word that holds collection of flags.  Otherwise, we would have named
> each "unsigned foo_flag : 1" with meaningful names.  Was it
> necessary to make the field name longer?
>

Just felt flags to be more descriptive, well "Otherwise, we would have 
named each "unsigned foo_flag : 1" with meaningful names. " makes sense.

> > @@ -688,7 +702,7 @@ static void populate_value(struct refinfo *ref)
> >                   v->s = xstrdup(buf + 1);
> >               }
> >               continue;
> > -        } else if (!deref && grab_objectname(name, ref->objectname,
> v)) {
> > +        } else if (!deref && grab_objectname(name, ref->sha1, v)) {
> >               continue;
>
> Mental note: grab_objectname() still remains, so I'd guess that it
> will not be moved from this file or it will stay private after it is
> moved.
>

It will be private.

 >
> Another mental note: it was a consistent naming for the function to
> grab objectname to store the result into objectname[] field.  Now it
> stores into sha1[] field.
>

yes, seems a bit off.

>
> Yuck; I can see what you are doing but can you imitate what the more
> experienced people (e.g. peff, mhagger) do when restructuring
> existing code and do things in smaller increments?  For example, I
> think it should be a separate preparatory patch, even before these
> renames of structures, fields and functions, to extract this helper
> function out of grab_single_ref() function.
>

Shall do so.

>
> And extracting this could be another separate preparatory step
> before renames (but I didn't look too closely).
>

Sure.

 >
> I won't be commenting on the remainder of the patch in this message,
> as I need to step out.  I see quite a many instances of the same
> "overly generic public names" in your "s/static //".
>

Will work on it, thanks for the suggestions.

-- 
Regards,
Karthik