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

> 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.

>
> 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.

>
> 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?
>

>
> 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').
>

>
> 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.
>

>
> 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?
>

> 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?
>

> > @@ -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.
>

>
> 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.
>