Re: [PATCH 2/4] ref-filter: add ref-filter API
Junio C Hamano gits...@pobox.com writes: And I do not think an array of things that are operated on should not be named ref_filter_item. Is the double-negation intended? It seems contradictory with: Surely, the latter set of operations to be applied may currently be only filtering, but who says it has to stay that way? (With which I do agree) -- 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 2/4] ref-filter: add ref-filter API
karthik nayak karthik@gmail.com writes:
At some point, I'd expect something like
filtered_list_of_refs = filer(full_list_of_refs, description_of_filter);
That would remove some refs from full_list_of_refs according to
description_of_filter.
(totally invented code, only to show the idea)
If there's a piece of code looking like this, then you need a data
structure to store list of refs (full_list_of_refs and
filtered_list_of_refs) and another to describe what you're doing with it
(description_of_filter).
The name ref_filter implies to me that it contains the description of
the filter, but looking at the code it doesn't seem to be the case.
But it does just that, doesn't it?
struct ref_filter {
int count, alloc;
struct ref_filter_item **items;
const char **name_patterns;
};
If you see it does contain 'name_patterns' according to which it will
filter the given refs,
But it also contains struct ref_filter_item **items, which as I
understand it contains a list of refs (with name, sha1 such).
That's the part I do not find natural: the same structure contains both
the list of refs and the way it should be filtered.
Re-reading the patch, I seem to understand that you're putting both on
the same struct because of the API of for_each_ref() which takes one
'data' pointer to be casted, so you want both the input (filter
description) and the output (list of refs after filtering) to be
contained in the same struct.
But I think this could be clearer in the code (and/or comment + commit
message). Perhaps stg like:
struct ref_filter_data /* Probably not the best name */ {
struct ref_list list;
struct ref_filter filter;
};
struct ref_list {
int count, alloc;
struct ref_filter_item **items;
const char **name_patterns;
};
struct ref_filter {
const char **name_patterns;
/* There will be more here later */
};
--
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 2/4] ref-filter: add ref-filter API
Christian Couder christian.cou...@gmail.com writes:
struct ref_list {
int count, alloc;
struct ref_filter_item **items;
const char **name_patterns;
};
Matthieu, I think you forgot to remove const char **name_patterns;
in the above struct, as you put it in the ref_filter struct below:
Yes, indeed. Too quick cut-and-paste.
I agree that it might be clearer to separate both. In this case
instead of ref_list the struct might be called ref_filter_array as
we already have argv_array in argv-array.h and sha1_array in
sha1-array.h.
I'd drop the filter part and make it ref_array then. There's no reason
we could not use it it places other than filter.
But we also have string_list which is an array underneath, so I think
both names (_array and _list) are fine.
--
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 2/4] ref-filter: add ref-filter API
On Sat, May 23, 2015 at 4:42 PM, Matthieu Moy
matthieu@grenoble-inp.fr wrote:
karthik nayak karthik@gmail.com writes:
At some point, I'd expect something like
filtered_list_of_refs = filer(full_list_of_refs, description_of_filter);
That would remove some refs from full_list_of_refs according to
description_of_filter.
(totally invented code, only to show the idea)
If there's a piece of code looking like this, then you need a data
structure to store list of refs (full_list_of_refs and
filtered_list_of_refs) and another to describe what you're doing with it
(description_of_filter).
The name ref_filter implies to me that it contains the description of
the filter, but looking at the code it doesn't seem to be the case.
But it does just that, doesn't it?
struct ref_filter {
int count, alloc;
struct ref_filter_item **items;
const char **name_patterns;
};
If you see it does contain 'name_patterns' according to which it will
filter the given refs,
But it also contains struct ref_filter_item **items, which as I
understand it contains a list of refs (with name, sha1 such).
That's the part I do not find natural: the same structure contains both
the list of refs and the way it should be filtered.
Re-reading the patch, I seem to understand that you're putting both on
the same struct because of the API of for_each_ref() which takes one
'data' pointer to be casted, so you want both the input (filter
description) and the output (list of refs after filtering) to be
contained in the same struct.
But I think this could be clearer in the code (and/or comment + commit
message). Perhaps stg like:
struct ref_filter_data /* Probably not the best name */ {
struct ref_list list;
struct ref_filter filter;
};
struct ref_list {
int count, alloc;
struct ref_filter_item **items;
const char **name_patterns;
};
Matthieu, I think you forgot to remove const char **name_patterns;
in the above struct, as you put it in the ref_filter struct below:
struct ref_filter {
const char **name_patterns;
/* There will be more here later */
};
I agree that it might be clearer to separate both. In this case
instead of ref_list the struct might be called ref_filter_array as
we already have argv_array in argv-array.h and sha1_array in
sha1-array.h.
--
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 2/4] ref-filter: add ref-filter API
Matthieu Moy matthieu@grenoble-inp.fr writes:
But I think this could be clearer in the code (and/or comment + commit
message). Perhaps stg like:
struct ref_filter_data /* Probably not the best name */ {
struct ref_list list;
struct ref_filter filter;
};
struct ref_list {
int count, alloc;
struct ref_filter_item **items;
};
If you plan to use ALLOC_GROW() API, name the bookkeeping variables
alloc nr, unless you have a compelling reason to deviate from
the prevailing practice.
struct ref_filter {
const char **name_patterns;
/* There will be more here later */
};
Very good suggestion.
Whatever the final name would be, it is a good idea to separate the
list of things that are operated on and the set of operations to
be applied. That makes things conceptually cleaner; you can have
multiple of the former operated on with a singleton of the latter
and then their results merged, etc. etc.
And I do not think an array of things that are operated on should
not be named ref_filter_item.
Surely, the latter set of operations to be applied may currently
be only filtering, but who says it has to stay that way? I have a
set of refs that represent my local branches I am interested
in. Please map them to their corresponding @{upstream} is a
reasonable request once you have an infrastructure to represent set
of refs to be worked on and set of operations to apply, and at
that point, the items are no longer filter-items (map-items?).
--
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 2/4] ref-filter: add ref-filter API
But it also contains struct ref_filter_item **items, which as I
understand it contains a list of refs (with name, sha1 such).
That's the part I do not find natural: the same structure contains both
the list of refs and the way it should be filtered.
Re-reading the patch, I seem to understand that you're putting both on
the same struct because of the API of for_each_ref() which takes one
'data' pointer to be casted, so you want both the input (filter
description) and the output (list of refs after filtering) to be
contained in the same struct.
Was kinda confused, This clears out things, Thanks.
But I think this could be clearer in the code (and/or comment + commit
message). Perhaps stg like:
struct ref_filter_data /* Probably not the best name */ {
struct ref_list list;
struct ref_filter filter;
};
struct ref_list {
int count, alloc;
struct ref_filter_item **items;
const char **name_patterns;
};
struct ref_filter {
const char **name_patterns;
/* There will be more here later */
};
This seems cleaner, agreed.
I agree that it might be clearer to separate both. In this case
instead of ref_list the struct might be called ref_filter_array as
we already have argv_array in argv-array.h and sha1_array in
sha1-array.h.
Somehow ref_list seems more real to me, list of refs.
And I do not think an array of things that are operated on should
not be named ref_filter_item.
Surely, the latter set of operations to be applied may currently
be only filtering, but who says it has to stay that way? I have a
set of refs that represent my local branches I am interested
in. Please map them to their corresponding @{upstream} is a
reasonable request once you have an infrastructure to represent set
of refs to be worked on and set of operations to apply, and at
that point, the items are no longer filter-items (map-items?).
That's also a good point to consider, I shall rename and restructure the
code as discussed here, thanks.
--
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 2/4] ref-filter: add ref-filter API
karthik nayak karthik@gmail.com writes: I miss a high-level description of what the code is doing. Essentially, there's the complete repository list of refs, and you want to filter only some of them, right? From the name, I would guess that ref_filter is the structure describing how you are filtering, but from the code it seems to be the list you're filtering, not the filter. Reading this again, A bit confused by what you're trying to imply. Could you rephrase please? At some point, I'd expect something like filtered_list_of_refs = filer(full_list_of_refs, description_of_filter); That would remove some refs from full_list_of_refs according to description_of_filter. (totally invented code, only to show the idea) If there's a piece of code looking like this, then you need a data structure to store list of refs (full_list_of_refs and filtered_list_of_refs) and another to describe what you're doing with it (description_of_filter). The name ref_filter implies to me that it contains the description of the filter, but looking at the code it doesn't seem to be the case. -- 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 2/4] ref-filter: add ref-filter API
On 05/22/2015 12:14 PM, Matthieu Moy wrote:
karthik nayak karthik@gmail.com writes:
I miss a high-level description of what the code is doing. Essentially,
there's the complete repository list of refs, and you want to filter
only some of them, right?
From the name, I would guess that ref_filter is the structure describing
how you are filtering, but from the code it seems to be the list you're
filtering, not the filter.
Reading this again, A bit confused by what you're trying to imply.
Could you rephrase please?
At some point, I'd expect something like
filtered_list_of_refs = filer(full_list_of_refs, description_of_filter);
That would remove some refs from full_list_of_refs according to
description_of_filter.
(totally invented code, only to show the idea)
If there's a piece of code looking like this, then you need a data
structure to store list of refs (full_list_of_refs and
filtered_list_of_refs) and another to describe what you're doing with it
(description_of_filter).
The name ref_filter implies to me that it contains the description of
the filter, but looking at the code it doesn't seem to be the case.
But it does just that, doesn't it?
strict ref_filter {
int count, alloc;
struct ref_filter_item **items;
const char **name_patterns;
};
If you see it does contain 'name_patterns' according to which it will
filter the given refs, but thats just the start, as 'for-each-ref' only
supports filtering based on the given pattern, eventually as I merge the
functionality of 'git tag -l' and 'git branch -l' it will contain more
filters like, 'contains_commit', 'merged' and so on. Eventually becoming
more of a filter description as you put it. I hope that clears out things :)
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 2/4] ref-filter: add ref-filter API
On 05/22/2015 12:10 AM, Eric Sunshine wrote: On Thu, May 21, 2015 at 1:30 PM, karthik nayak karthik@gmail.com wrote: On 05/21/2015 12:37 AM, Eric Sunshine wrote: On Wed, May 20, 2015 at 9:18 AM, Karthik Nayak karthik@gmail.com wrote: Makefile | 1 + ref-filter.c | 73 ref-filter.h | 47 ++ 3 files changed, 121 insertions(+) create mode 100644 ref-filter.c create mode 100644 ref-filter.h A shortcoming of this approach is that it's not blame-friendly. Although those of us following this patch series know that much of the code in this patch was copied from for-each-ref.c, git-blame will not recognize this unless invoked in the very expensive git blame -C -C -C fashion (if I understand correctly). The most blame-friendly way to perform this re-organization is to have the code relocation (line removals and line additions) occur in one patch. There are multiple ways you could arrange to do so. One would be to first have a patch which introduces just a skeleton of the intended API, with do-nothing function implementations. A subsequent patch would then relocate the code from for-each-ref.c to ref-filter.c, and update for-each-ref.c to call into the new (now fleshed-out) API. Did you read Junio's suggestion on how I should re-order this WIP patch series ? That's somewhat on the lines of what you're suggesting. I'll probably be going ahead with that, not really sure about how blame works entirely so what do you think about that? Yes, Junio's response did a much better job of saying what I intended. Also, his response said something I meant to mention but forgot: namely that, to ease the review task, code movement should be pure movement, and not involve other changes. Anyhow, follow Junio's advice. He knows what he's talking about. ;-) Alright, Thanks for clearing that out. 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 2/4] ref-filter: add ref-filter API
Karthik Nayak karthik@gmail.com writes:
+static int match_name_as_path(const char **pattern, const char *refname)
I would have appreciated a short docstring. The full doc would probably
be as long as the code, but a few examples of what matches and what
doesn't can help the reader.
+static struct ref_filter_item *new_ref_filter_item(const char *refname,
+const unsigned char *sha1,
+int flag)
+{
+ struct ref_filter_item *ref = xcalloc(1, sizeof(struct
ref_filter_item));
double-space after =.
+++ b/ref-filter.h
@@ -0,0 +1,47 @@
+#ifndef REF_FILTER_H
+#define REF_FILTER_H
+
+#include sha1-array.h
+#include refs.h
+#include commit.h
+
+/*
+ * ref-filter is meant to act as a common provider of API's for
+ * 'tag -l', 'branch -l' and 'for-each-ref'. ref-filter is the attempt
Don't be shy: attempt at unification - unification. This message may be
an attempt, but we'll polish it until it is more than that.
+ * at unification of these three commands so that they ay benefit from
they *may*?
+ * the functionality of each other.
+ */
I miss a high-level description of what the code is doing. Essentially,
there's the complete repository list of refs, and you want to filter
only some of them, right?
From the name, I would guess that ref_filter is the structure describing
how you are filtering, but from the code it seems to be the list you're
filtering, not the filter.
+/* An atom is a valid field atom used for sorting and formatting of refs.*/
used for is very vague. Be more precise, say how it will be involved
in sorting formatting.
+/* ref_filter will hold data pertaining to a list of refs. */
This is the answer to the what? question, which is not very hard to
infer from the code. That's not anwsering what for? or why?, which
are much harder to infer for the reader.
(plus you have a double-space after /*)
--
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 2/4] ref-filter: add ref-filter API
I miss a high-level description of what the code is doing. Essentially, there's the complete repository list of refs, and you want to filter only some of them, right? From the name, I would guess that ref_filter is the structure describing how you are filtering, but from the code it seems to be the list you're filtering, not the filter. Reading this again, A bit confused by what you're trying to imply. Could you rephrase please? -- 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 2/4] ref-filter: add ref-filter API
On Thu, May 21, 2015 at 1:30 PM, karthik nayak karthik@gmail.com wrote: On 05/21/2015 12:37 AM, Eric Sunshine wrote: On Wed, May 20, 2015 at 9:18 AM, Karthik Nayak karthik@gmail.com wrote: Makefile | 1 + ref-filter.c | 73 ref-filter.h | 47 ++ 3 files changed, 121 insertions(+) create mode 100644 ref-filter.c create mode 100644 ref-filter.h A shortcoming of this approach is that it's not blame-friendly. Although those of us following this patch series know that much of the code in this patch was copied from for-each-ref.c, git-blame will not recognize this unless invoked in the very expensive git blame -C -C -C fashion (if I understand correctly). The most blame-friendly way to perform this re-organization is to have the code relocation (line removals and line additions) occur in one patch. There are multiple ways you could arrange to do so. One would be to first have a patch which introduces just a skeleton of the intended API, with do-nothing function implementations. A subsequent patch would then relocate the code from for-each-ref.c to ref-filter.c, and update for-each-ref.c to call into the new (now fleshed-out) API. Did you read Junio's suggestion on how I should re-order this WIP patch series ? That's somewhat on the lines of what you're suggesting. I'll probably be going ahead with that, not really sure about how blame works entirely so what do you think about that? Yes, Junio's response did a much better job of saying what I intended. Also, his response said something I meant to mention but forgot: namely that, to ease the review task, code movement should be pure movement, and not involve other changes. Anyhow, follow Junio's advice. He knows what he's talking about. ;-) -- 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 2/4] ref-filter: add ref-filter API
On 05/21/2015 02:17 PM, Matthieu Moy wrote:
Karthik Nayak karthik@gmail.com writes:
+static int match_name_as_path(const char **pattern, const char *refname)
I would have appreciated a short docstring. The full doc would probably
be as long as the code, but a few examples of what matches and what
doesn't can help the reader.
Will patch with an explanation and some examples.
+static struct ref_filter_item *new_ref_filter_item(const char *refname,
+ const unsigned char *sha1,
+ int flag)
+{
+ struct ref_filter_item *ref = xcalloc(1, sizeof(struct
ref_filter_item));
double-space after =.
Noted.
+++ b/ref-filter.h
@@ -0,0 +1,47 @@
+#ifndef REF_FILTER_H
+#define REF_FILTER_H
+
+#include sha1-array.h
+#include refs.h
+#include commit.h
+
+/*
+ * ref-filter is meant to act as a common provider of API's for
+ * 'tag -l', 'branch -l' and 'for-each-ref'. ref-filter is the attempt
Don't be shy: attempt at unification - unification. This message may be
an attempt, but we'll polish it until it is more than that.
Haha, OK, will change that.
+ * at unification of these three commands so that they ay benefit from
they *may*?
Yes. Will change
+ * the functionality of each other.
+ */
I miss a high-level description of what the code is doing. Essentially,
there's the complete repository list of refs, and you want to filter
only some of them, right?
From the name, I would guess that ref_filter is the structure describing
how you are filtering, but from the code it seems to be the list you're
filtering, not the filter.
Will write a better explanation and description.
+/* An atom is a valid field atom used for sorting and formatting of refs.*/
used for is very vague. Be more precise, say how it will be involved
in sorting formatting.
Noted.
+/* ref_filter will hold data pertaining to a list of refs. */
This is the answer to the what? question, which is not very hard to
infer from the code. That's not anwsering what for? or why?, which
are much harder to infer for the reader.
(plus you have a double-space after /*)
Noted! Thanks for the suggestions :)
--
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 2/4] ref-filter: add ref-filter API
On 05/21/2015 12:37 AM, Eric Sunshine wrote: On Wed, May 20, 2015 at 9:18 AM, Karthik Nayak karthik@gmail.com wrote: add a ref-filter API to provide functions to filter refs for listing. This will act as a common library for commands like 'tag -l', 'branch -l' and 'for-each-ref'. ref-filter will enable each of these commands to benefit from the features of the others. Mentored-by: Christian Couder christian.cou...@gmail.com Mentored-by: Matthieu Moy matthieu@grenoble-inp.fr Signed-off-by: Karthik Nayak karthik@gmail.com --- Makefile | 1 + ref-filter.c | 73 ref-filter.h | 47 ++ 3 files changed, 121 insertions(+) create mode 100644 ref-filter.c create mode 100644 ref-filter.h A shortcoming of this approach is that it's not blame-friendly. Although those of us following this patch series know that much of the code in this patch was copied from for-each-ref.c, git-blame will not recognize this unless invoked in the very expensive git blame -C -C -C fashion (if I understand correctly). The most blame-friendly way to perform this re-organization is to have the code relocation (line removals and line additions) occur in one patch. There are multiple ways you could arrange to do so. One would be to first have a patch which introduces just a skeleton of the intended API, with do-nothing function implementations. A subsequent patch would then relocate the code from for-each-ref.c to ref-filter.c, and update for-each-ref.c to call into the new (now fleshed-out) API. Did you read Junio's suggestion on how I should re-order this WIP patch series ? That's somewhat on the lines of what you're suggesting. I'll probably be going ahead with that, not really sure about how blame works entirely so what do you think about that? -- 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 2/4] ref-filter: add ref-filter API
On Wed, May 20, 2015 at 9:18 AM, Karthik Nayak karthik@gmail.com wrote: add a ref-filter API to provide functions to filter refs for listing. This will act as a common library for commands like 'tag -l', 'branch -l' and 'for-each-ref'. ref-filter will enable each of these commands to benefit from the features of the others. Mentored-by: Christian Couder christian.cou...@gmail.com Mentored-by: Matthieu Moy matthieu@grenoble-inp.fr Signed-off-by: Karthik Nayak karthik@gmail.com --- Makefile | 1 + ref-filter.c | 73 ref-filter.h | 47 ++ 3 files changed, 121 insertions(+) create mode 100644 ref-filter.c create mode 100644 ref-filter.h A shortcoming of this approach is that it's not blame-friendly. Although those of us following this patch series know that much of the code in this patch was copied from for-each-ref.c, git-blame will not recognize this unless invoked in the very expensive git blame -C -C -C fashion (if I understand correctly). The most blame-friendly way to perform this re-organization is to have the code relocation (line removals and line additions) occur in one patch. There are multiple ways you could arrange to do so. One would be to first have a patch which introduces just a skeleton of the intended API, with do-nothing function implementations. A subsequent patch would then relocate the code from for-each-ref.c to ref-filter.c, and update for-each-ref.c to call into the new (now fleshed-out) API. -- 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
