Le mercredi 8 avril 2009, Junio C Hamano a écrit :
> Christian Couder <chriscool@xxxxxxxxxxxxx> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html