Junio C Hamano <gitster@xxxxxxxxx> writes:
> The current API to hide such an embarrasing but necessary
> implementation details from the code that does not need to know
> them, i.e. the consumer of rev-info structure with various settings,
> is to invoke the command line parser. Bypassing and going directly
> down to the internal implementation detail of rev_info _is_ being
> sloppy. I would strongly prefer to see that the current series
> written for the API to allow us get it over with the "rebase -i"
> thing, and think revision setup API separately and later.
An updated API that hides the implementation details may look like
a vast enhancement of this patch.
I say "vast" because we need to do this for _all_ of the "else if"
cascade you see in revision.c, and probably for fields set by other
helper functions in the same file. Otherwise, it doesn't have much
value.
Anybody who is tempted to say "We need to do these only for the
complex ones, like the one that needs to set revs->pretty_given
while setting something else" hasn't learned from the example of
66b2ed09. Interactions between options start happening when new
options that are added, and that is when handling of a seemingly
unrelated old option that used to be very simple needs to do more in
the new world order. And that is why this illustration has a
wrapper for "--first-parent-only", which happens to be very simple
today.
We do not want revision traversal API's customers to write
revs.first_parent_only = 1;
because that's mucking with the implementation detail. The current
API to hide that detail is:
memset(&revs, 0, sizeof(revs);
argv_pushl(&args, "--first-parent-only", NULL);
... may be more options ...
setup_revisions(args.argc, args.argv, &revs, ...);
and
memset(&revs, 0, sizeof(revs);
rev_option_first_parent_only(&revs);
... may be more options ...
setup_revisions(0, NULL, &revs, ...);
would be a more statically-checked rewrite, if such an API was
available.
revision-internal.h | 18 ++++++++++++++++++
revision.c | 9 +++------
2 files changed, 21 insertions(+), 6 deletions(-)
diff --git a/revision-internal.h b/revision-internal.h
new file mode 100644
index 0000000000..dea4c48412
--- /dev/null
+++ b/revision-internal.h
@@ -0,0 +1,18 @@
+#ifndef REVISION_INTERNAL_H
+#define REVISION_INTERNAL_H 1
+
+static inline void rev_option_first_parent_only(struct rev_info *revs)
+{
+ revs->first_parent_only = 1;
+}
+
+static inline void rev_option_simplify_merges(struct rev_info *revs)
+{
+ revs->simplify_merges = 1;
+ revs->topo_order = 1;
+ revs->rewrite_parents = 1;
+ revs->simplify_history = 0;
+ revs->limited = 1;
+}
+
+#endif
diff --git a/revision.c b/revision.c
index 265611e01f..9418676264 100644
--- a/revision.c
+++ b/revision.c
@@ -20,6 +20,7 @@
#include "cache-tree.h"
#include "bisect.h"
#include "worktree.h"
+#include "revision-internal.h"
volatile show_early_output_fn_t show_early_output;
@@ -1807,7 +1808,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
revs->min_age = approxidate(optarg);
return argcount;
} else if (!strcmp(arg, "--first-parent")) {
- revs->first_parent_only = 1;
+ rev_option_first_parent_only(&revs);
} else if (!strcmp(arg, "--ancestry-path")) {
revs->ancestry_path = 1;
revs->simplify_history = 0;
@@ -1825,11 +1826,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
revs->sort_order = REV_SORT_IN_GRAPH_ORDER;
revs->topo_order = 1;
} else if (!strcmp(arg, "--simplify-merges")) {
- revs->simplify_merges = 1;
- revs->topo_order = 1;
- revs->rewrite_parents = 1;
- revs->simplify_history = 0;
- revs->limited = 1;
+ rev_option_simplify_merges(&revs);
} else if (!strcmp(arg, "--simplify-by-decoration")) {
revs->simplify_merges = 1;
revs->topo_order = 1;