Re: [PATCH 1/3] list-objects: add "void *data" parameter to show functions

From: Christian Couder <chriscool@tuxfamily.org>
Date: Apr 8, 2009, 04:39 UTC
Message-ID: <200904080639.35499.chriscool@tuxfamily.org>
In-Reply-To: <7vk55vubcv.fsf@gitster.siamese.dyndns.org>

Le mercredi 8 avril 2009, Junio C Hamano a écrit :

Show 15 quoted lines

> Christian Couder <chriscool@tuxfamily.org> writes:
> > The goal of this patch is to get rid of the "static struct rev_info
> > revs" static variable in "builtin-rev-list.c".
>
> Hmm.  If it were a more library-ish file, a removal of such a static
> variable might help you to make more than one calls to a library
> function, but does it matter in files like builtin-rev-list.c?  Its
> cmd_rev_list() is like main() --- it is meant to run once and exit.
>
> So if it is the only goal of this series, I am inclined to say that I do
> not have a reason to look at the rest of the series, but as a side effect
> does this removal make some other API better?  Perhaps a more library-ish
> function is in builtin-rev-list.c and this structure should really needs
> to be passed around as a parameter, but I cannot tell solely by reading
> the goal above, without reading the patches themselves.

In the cover letter, I wrote that the patch series removes restrictions on using the "show_bisect_vars" function.

In fact there was a restriction on the use of the BISECT_SHOW_ALL flag because that would use the "show_commit" function that was using static variables.

The restriction was described in a comment in "bisect.h" and this comment is removed by the series.

This is the relevant hunk in patch 2/3:

diff --git a/bisect.h b/bisect.h
index f5d1067..b1c334d 100644
--- a/bisect.h
+++ b/bisect.h
@@ -14,12 +14,14 @@ extern struct commit_list *filter_skipped(struct 
commit_list *list,
 #define BISECT_SHOW_TRIED      (1<<1)
 #define BISECT_SHOW_STRINGED   (1<<2)
 
-/*
- * The flag BISECT_SHOW_ALL should not be set if this function is called
- * from outside "builtin-rev-list.c" as otherwise it would use
- * static "revs" from this file.
- */
-extern int show_bisect_vars(struct rev_info *revs, int reaches, int all,
+struct rev_list_info {
+       struct rev_info *revs;
+       int show_timestamp;
+       int hdr_termination;
+       const char *header_prefix;
+};
+
+extern int show_bisect_vars(struct rev_list_info *info, int reaches, int 
all,
                            int flags);
 
 extern int bisect_next_vars(const char *prefix);


Best regards,
Christian.

> > Anyway this makes the code more clean and more generic, so it
> > should be a good thing in the long run.
>
> I wouldn't disagree with that "long run" thing, but the answer to the
> above question affects the placement of this series in my prioritized
> queue.