Re: [PATCH v2 1/2] for-each-ref: re-structure code for moving to 'ref-filter'
On 05/26/2015 09:19 PM, Matthieu Moy wrote: Seconded. Some reasons/guide to split: * Split trivial and non-trivial stuff. I can quickly review a rename-only patch even if it's a bit long (essentially, I'll check that you did find-and-replace properly), but reviewing a mix of renames and actual code change is hard. * Split controversial and non-controversial stuff. For example, you changed the ordering of fields in a struct. Perhaps it was not a good idea. Perhaps it was a good idea, but then you want this reordering to be alone in its patch so that you can explain why it's a good idea in the commit message (you'll see me use the word why a lot when talking about commit messages; not a coincidence).u Since one of the patches is to restructure and rename 'for-each-ref', I thought It would be ideal to introduce the data structures within that patch, What do you think? * Split code movement and other stuff. For example extraction of match_name_as_path() would be trivial if made in its own patch. I'd also make a separate patch introduce the ref_list data-structure to introduce struct ref_list and basic helper functions (constructors, mutators, destructors). It will take time and may appear to be counter-productive at first, but you'll get used to it, and you'll end up being actually more productive this way (well, maybe not you but the set you + reviewers). Thanks for this. -- Regards, Karthik -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] for-each-ref: re-structure code for moving to 'ref-filter'
Karthik Nayak karthik@gmail.com writes: On 05/26/2015 09:19 PM, Matthieu Moy wrote: Seconded. Some reasons/guide to split: * Split trivial and non-trivial stuff. I can quickly review a rename-only patch even if it's a bit long (essentially, I'll check that you did find-and-replace properly), but reviewing a mix of renames and actual code change is hard. * Split controversial and non-controversial stuff. For example, you changed the ordering of fields in a struct. Perhaps it was not a good idea. Perhaps it was a good idea, but then you want this reordering to be alone in its patch so that you can explain why it's a good idea in the commit message (you'll see me use the word why a lot when talking about commit messages; not a coincidence). Since one of the patches is to restructure and rename 'for-each-ref', I thought It would be ideal to introduce the data structures within that patch, What do you think? I don't have a universal answer: in general I prefer (let's say this list prefers) splitting as much as possible. It may make sense to group add data structure X with use data-structure X to make sure that functions you introduce have a caller. What's clear is that your PATCH 1/2 is not split enough. Just go through it, you'll see code movement (a pain to review in patch format), straigthforward renamings (easy to review as-is, but disturbs the reviewer when mixed with something else) and actual new code. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] for-each-ref: re-structure code for moving to 'ref-filter'
Also I think Matthieu already commented that filter was out of place for that struct. I still think your ref_list is better called ref_array, but that is a minor point. Use of foo_list in our codebase is predominantly (because we use commit_list very often in the core part of the system) for a linear linked list where you do not have a random access to the items. string-list is misnomer, I would think. ref_array also sounds good, yes! there might be confusion and might be considered a linked list rather than an array. Will change. I think you now know after seeing that 56-patch series ;-) Haha, That definitely helped. If that is the case, I would suggest making that turn it flex array a separate step. Sure. -- Regards, Karthik -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] for-each-ref: re-structure code for moving to 'ref-filter'
Junio C Hamano gits...@pobox.com writes: 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? Seconded. Some reasons/guide to split: * Split trivial and non-trivial stuff. I can quickly review a rename-only patch even if it's a bit long (essentially, I'll check that you did find-and-replace properly), but reviewing a mix of renames and actual code change is hard. * Split controversial and non-controversial stuff. For example, you changed the ordering of fields in a struct. Perhaps it was not a good idea. Perhaps it was a good idea, but then you want this reordering to be alone in its patch so that you can explain why it's a good idea in the commit message (you'll see me use the word why a lot when talking about commit messages; not a coincidence). * Split code movement and other stuff. For example extraction of match_name_as_path() would be trivial if made in its own patch. I'd also make a separate patch introduce the ref_list data-structure to introduce struct ref_list and basic helper functions (constructors, mutators, destructors). It will take time and may appear to be counter-productive at first, but you'll get used to it, and you'll end up being actually more productive this way (well, maybe not you but the set you + reviewers). -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] for-each-ref: re-structure code for moving to 'ref-filter'
Karthik Nayak karthik@gmail.com writes:
-struct refinfo {
- char *refname;
- unsigned char objectname[20];
- int flag;
+struct ref_filter_item {
+ unsigned char sha1[20];
+ int flags;
const char *symref;
struct atom_value *value;
+ char *name;
+};
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?
+
+struct ref_list {
+ int nr, alloc;
+ struct ref_filter_item **items;
+};
+
+struct ref_filter {
+ const char **name_patterns;
+};
+
+struct ref_filter_data {
+ struct ref_list list;
+ struct ref_filter filter;
};
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').
@@ -98,7 +112,7 @@ static int need_color_reset_at_eol;
/*
* Used to parse format string and sort specifiers
*/
-static int parse_atom(const char *atom, const char *ep)
+int parse_atom(const char *atom, const char *ep)
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.
@@ -175,7 +189,7 @@ static const char *find_next(const char *cp)
* Make sure the format string is well formed, and parse out
* the used atoms.
*/
-static int verify_format(const char *format)
+int verify_format(const char *format)
Ditto.
@@ -622,7 +636,7 @@ static inline char *copy_advance(char *dst, const char
*src)
/*
* Parse the object referred by ref, and grab needed value.
*/
-static void populate_value(struct refinfo *ref)
+static void populate_value(struct ref_filter_item *ref)
{
As long as this will stay private within the new ref-filter.c after
the move, this name is OK.
@@ -655,14 +669,14 @@ static void populate_value(struct refinfo *ref)
}
if (starts_with(name, refname))
- refname = ref-refname;
+ refname = ref-name;
else if (starts_with(name, symref))
refname = ref-symref ? ref-symref : ;
else if (starts_with(name, upstream)) {
/* only local branches may have an upstream */
- if (!starts_with(ref-refname, refs/heads/))
+ if (!starts_with(ref-name, refs/heads/))
continue;
- branch = branch_get(ref-refname + 11);
+ branch = branch_get(ref-name + 11);
if (!branch || !branch-merge || !branch-merge[0] ||
!branch-merge[0]-dst)
@@ -677,9 +691,9 @@ static void populate_value(struct refinfo *ref)
continue;
} else if (!strcmp(name, flag)) {
char buf[256], *cp = buf;
- if (ref-flag REF_ISSYMREF)
+ if (ref-flags REF_ISSYMREF)
Re: [PATCH v2 1/2] for-each-ref: re-structure code for moving to 'ref-filter'
Karthik Nayak karthik@gmail.com writes: 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. Also I think Matthieu already commented that filter was out of place for that struct. I still think your ref_list is better called ref_array, but that is a minor point. Use of foo_list in our codebase is predominantly (because we use commit_list very often in the core part of the system) for a linear linked list where you do not have a random access to the items. string-list is misnomer, I would think. 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. I think you now know after seeing that 56-patch series ;-) 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. If that is the case, I would suggest making that turn it flex array a separate step. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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
