From: Karthik Nayak <karthik.188@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org, christian.couder@gmail.com,
matthieu.moy@grenoble-inp.fr
Subject: Re: [PATCH v2 1/2] for-each-ref: re-structure code for moving to 'ref-filter'
Date: Mon, 25 May 2015 23:29:29 +0530 [thread overview]
Message-ID: <55636301.7060803@gmail.com> (raw)
In-Reply-To: <xmqqegm4bmtg.fsf@gitster.dls.corp.google.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
next prev parent reply other threads:[~2015-05-25 17:59 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-05-25 12:39 [WIP][Patch v2 0/2] Ref-filter: unification of 'tag -l', 'branch -l' and 'for-each-ref' Karthik Nayak
2015-05-25 12:45 ` [PATCH v2 1/2] for-each-ref: re-structure code for moving to 'ref-filter' Karthik Nayak
2015-05-25 17:15 ` Junio C Hamano
2015-05-25 17:59 ` Karthik Nayak [this message]
2015-05-25 19:39 ` Junio C Hamano
2015-05-26 6:58 ` Karthik Nayak
2015-05-26 15:49 ` Matthieu Moy
2015-05-28 7:08 ` Karthik Nayak
2015-05-28 11:26 ` Matthieu Moy
2015-05-25 12:45 ` [PATCH v2 2/2] ref-filter: move code from 'for-each-ref' Karthik Nayak
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
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=55636301.7060803@gmail.com \
--to=karthik.188@gmail.com \
--cc=christian.couder@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=matthieu.moy@grenoble-inp.fr \
/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.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
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 read-only IMAP folder(s) and NNTP newsgroup(s).