Re: [PATCH 5/8] string-list: add pos to iterator callback
Jeff King p...@peff.net writes: When we are running a string-list foreach or filter, the callback function sees only the string_list_item, along with a void* data pointer provided by the caller. This is sufficient for most purposes. However, it can also be useful to know the position of the item within the string (for example, if the data pointer s/the string/-list/ (or s//_list/). points to a secondary array in which each element corresponds to part of the string list). We can help this use case by providing the position to each callback. Signed-off-by: Jeff King p...@peff.net --- The diff here is noisy, and I expect in the long run that the one caller I add to builtin/tag.c later in the series will eventually stop using string_list entirely (in favor of a custom struct), which may leave us with no callers that actually use the new field. I do think the logic above is sound, though, and it's a potentially useful thing. There may be other sites that avoid the for_each wrapper in favor of iterating themselves simply _because_ they needed to know the position (I would just do the same here, except that my new caller wants to use filter_string_list, which is a little more complicated). While I can buy that some callers would want to learn the pos in the list, I am not sure if this is a good direction to go. Primarily, I am having trouble sifting between want and need. How often do callers want to do this? If only narrow specialized callers want this, is it unreasonable to ask them to add a int ctr in their cb_data and use pos = ((struct theirs *)cb_data)-ctr++ in their callback instead? -- 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 5/8] string-list: add pos to iterator callback
On Tue, Jul 01, 2014 at 10:45:19AM -0700, Junio C Hamano wrote:
Jeff King p...@peff.net writes:
When we are running a string-list foreach or filter, the
callback function sees only the string_list_item, along with
a void* data pointer provided by the caller. This is
sufficient for most purposes.
However, it can also be useful to know the position of the
item within the string (for example, if the data pointer
s/the string/-list/ (or s//_list/).
Thanks, yeah.
While I can buy that some callers would want to learn the pos in the
list, I am not sure if this is a good direction to go. Primarily, I
am having trouble sifting between want and need.
How often do callers want to do this? If only narrow specialized
callers want this, is it unreasonable to ask them to add a int ctr
in their cb_data and use pos = ((struct theirs *)cb_data)-ctr++
in their callback instead?
Not all that often, I suppose (I only add one caller in this series). I
just found it a little hack-ish to force callers to keep their own
counter when we already have it (especially because it is not too hard
to get it wrong, for example by forgetting to increment the counter in
one code path of the callback).
Here's what the caller would look like without pos (done as a patch on
top of the series, so pos is still there, but no longer used).
diff --git a/builtin/tag.c b/builtin/tag.c
index f17192c..dc6f105 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -151,14 +151,20 @@ static int util_is_not_null(struct string_list_item *it,
int pos, void *data)
return !!it-util;
}
-static int matches_contains(struct string_list_item *it, int pos, void *data)
+struct contains_callback_data {
+ int ctr;
+ unsigned char *contains;
+};
+
+static int matches_contains(struct string_list_item *it, int pos, void *vdata)
{
- unsigned char *contains = data;
- return contains[pos];
+ struct contains_callback_data *data = vdata;
+ return data-contains[data-ctr++];
}
static void limit_by_contains(struct string_list *tags, struct commit_list
*with)
{
+ struct contains_callback_data cbdata;
struct commit_list *tag_commits = NULL, **tail = tag_commits;
unsigned char *result;
int i;
@@ -180,7 +186,10 @@ static void limit_by_contains(struct string_list *tags,
struct commit_list *with
result = xmalloc(tags-nr);
commit_contains(with, tag_commits, NULL, result);
- filter_string_list(tags, 1, matches_contains, result);
+
+ cbdata.ctr = 0;
+ cbdata.contains = result;
+ filter_string_list(tags, 1, matches_contains, cbdata);
free(result);
free_commit_list(tag_commits);
So I think it's a small change in a lot of places rather than a kind of
ugly change in one spot.
All that being said, I think the real issue here is that I want more
than a string list (I am already using the util field for the sha1, but
this code would be potentially simpler if I could also store the commit
object). In the long run I hope to factor out a ref-listing API that can
be used by tag, branch, and for-each-ref. And then this string-list code
would go away in favor of a more expansive struct. So it may not be
worth worrying about keeping it too clean.
-Peff
--
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
[PATCH 5/8] string-list: add pos to iterator callback
When we are running a string-list foreach or filter, the
callback function sees only the string_list_item, along with
a void* data pointer provided by the caller. This is
sufficient for most purposes.
However, it can also be useful to know the position of the
item within the string (for example, if the data pointer
points to a secondary array in which each element
corresponds to part of the string list). We can help this
use case by providing the position to each callback.
Signed-off-by: Jeff King p...@peff.net
---
The diff here is noisy, and I expect in the long run that the one caller
I add to builtin/tag.c later in the series will eventually stop using
string_list entirely (in favor of a custom struct), which may leave us
with no callers that actually use the new field.
I do think the logic above is sound, though, and it's a potentially
useful thing. There may be other sites that avoid the for_each wrapper
in favor of iterating themselves simply _because_ they needed to know
the position (I would just do the same here, except that my new caller
wants to use filter_string_list, which is a little more complicated).
builtin/clone.c| 2 +-
builtin/remote.c | 12 ++--
notes.c| 1 +
setup.c| 1 +
string-list.c | 6 +++---
string-list.h | 9 +++--
test-path-utils.c | 2 +-
test-string-list.c | 2 +-
8 files changed, 21 insertions(+), 14 deletions(-)
diff --git a/builtin/clone.c b/builtin/clone.c
index b12989d..89d0709 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -227,7 +227,7 @@ static void strip_trailing_slashes(char *dir)
*end = '\0';
}
-static int add_one_reference(struct string_list_item *item, void *cb_data)
+static int add_one_reference(struct string_list_item *item, int pos, void
*cb_data)
{
char *ref_git;
const char *repo;
diff --git a/builtin/remote.c b/builtin/remote.c
index c9102e8..bea0b58 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -925,7 +925,7 @@ struct show_info {
int any_rebase;
};
-static int add_remote_to_show_info(struct string_list_item *item, void
*cb_data)
+static int add_remote_to_show_info(struct string_list_item *item, int pos,
void *cb_data)
{
struct show_info *info = cb_data;
int n = strlen(item-string);
@@ -935,7 +935,7 @@ static int add_remote_to_show_info(struct string_list_item
*item, void *cb_data)
return 0;
}
-static int show_remote_info_item(struct string_list_item *item, void *cb_data)
+static int show_remote_info_item(struct string_list_item *item, int pos, void
*cb_data)
{
struct show_info *info = cb_data;
struct ref_states *states = info-states;
@@ -962,7 +962,7 @@ static int show_remote_info_item(struct string_list_item
*item, void *cb_data)
return 0;
}
-static int add_local_to_show_info(struct string_list_item *branch_item, void
*cb_data)
+static int add_local_to_show_info(struct string_list_item *branch_item, int
pos, void *cb_data)
{
struct show_info *show_info = cb_data;
struct ref_states *states = show_info-states;
@@ -984,7 +984,7 @@ static int add_local_to_show_info(struct string_list_item
*branch_item, void *cb
return 0;
}
-static int show_local_info_item(struct string_list_item *item, void *cb_data)
+static int show_local_info_item(struct string_list_item *item, int pos, void
*cb_data)
{
struct show_info *show_info = cb_data;
struct branch_info *branch_info = item-util;
@@ -1016,7 +1016,7 @@ static int show_local_info_item(struct string_list_item
*item, void *cb_data)
return 0;
}
-static int add_push_to_show_info(struct string_list_item *push_item, void
*cb_data)
+static int add_push_to_show_info(struct string_list_item *push_item, int pos,
void *cb_data)
{
struct show_info *show_info = cb_data;
struct push_info *push_info = push_item-util;
@@ -1045,7 +1045,7 @@ static int cmp_string_with_push(const void *va, const
void *vb)
return cmp ? cmp : strcmp(a_push-dest, b_push-dest);
}
-static int show_push_info_item(struct string_list_item *item, void *cb_data)
+static int show_push_info_item(struct string_list_item *item, int pos, void
*cb_data)
{
struct show_info *show_info = cb_data;
struct push_info *push_info = item-util;
diff --git a/notes.c b/notes.c
index 5fe691d..f7a84f9 100644
--- a/notes.c
+++ b/notes.c
@@ -881,6 +881,7 @@ static int string_list_add_note_lines(struct string_list
*list,
}
static int string_list_join_lines_helper(struct string_list_item *item,
+int pos,
void *cb_data)
{
struct strbuf *buf = cb_data;
diff --git a/setup.c b/setup.c
index 0a22f8b..0fe92fe 100644
--- a/setup.c
+++ b/setup.c
@@ -586,6 +586,7 @@ static dev_t get_device_or_die(const char *path, const char
*prefix, int prefix_
* subsequent entries.
*/
static int
