Re: [PATCH] bisect: display first bad commit without forking a new process

Christian Couder [off-list ref] writes:

Le Wednesday 27 May 2009, Christian Couder a écrit :

quoted

Previously "git diff-tree --pretty COMMIT" was run using
"run_command_v_opt" to display information about the first bad
commit.

The goal of this patch is to avoid a "fork" and an "exec" call
when displaying that information.

To do that, we manually setup revision information as
"git diff-tree --pretty" would do it, and then use the
"log_tree_commit" function.

Signed-off-by: Christian Couder <redacted>
---
 bisect.c |   30 +++++++++++++++++++++++++++---
 1 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/bisect.c b/bisect.c
index c43c120..e94a77b 100644
--- a/bisect.c
+++ b/bisect.c

@@ -816,6 +816,31 @@ static void check_good_are_ancestors_of_bad(const

char *prefix) }

 /*
+ * This does "git diff-tree --pretty COMMIT" without one fork+exec.
+ */
+static void show_diff_tree(const char *prefix, struct commit *commit)
+{
+	static struct rev_info opt;

Oops, "static" can be removed, it's a copy-paste error, sorry.

Is that "can" or "must"?  If the answer is the latter, shouldn't the
function be renamed to make it clear it is a bisect specific thing?

Junio C Hamano wrote:

Christian Couder [off-list ref] writes:

quoted

Le Wednesday 27 May 2009, Christian Couder a écrit :

quoted

Previously "git diff-tree --pretty COMMIT" was run using
"run_command_v_opt" to display information about the first bad
commit.

The goal of this patch is to avoid a "fork" and an "exec" call
when displaying that information.

To do that, we manually setup revision information as
"git diff-tree --pretty" would do it, and then use the
"log_tree_commit" function.

Signed-off-by: Christian Couder <redacted>
---
 bisect.c |   30 +++++++++++++++++++++++++++---
 1 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/bisect.c b/bisect.c
index c43c120..e94a77b 100644
--- a/bisect.c
+++ b/bisect.c

@@ -816,6 +816,31 @@ static void check_good_are_ancestors_of_bad(const

char *prefix) }

 /*
+ * This does "git diff-tree --pretty COMMIT" without one fork+exec.
+ */
+static void show_diff_tree(const char *prefix, struct commit *commit)
+{
+	static struct rev_info opt;

Oops, "static" can be removed, it's a copy-paste error, sorry.

Is that "can" or "must"?  If the answer is the latter, shouldn't the
function be renamed to make it clear it is a bisect specific thing?

I think he was referring to the 'static' in the variable declarations.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Register now for Nordic Meet on Nagios, June 3-4 in Stockholm
 http://nordicmeetonnagios.op5.org/

Considering the successes of the wars on alcohol, poverty, drugs and
terror, I think we should give some serious thought to declaring war
on peace.

Le Thursday 28 May 2009, Andreas Ericsson a écrit :

Junio C Hamano wrote:

quoted

Christian Couder [off-list ref] writes:

quoted

Le Wednesday 27 May 2009, Christian Couder a écrit :

quoted

Previously "git diff-tree --pretty COMMIT" was run using
"run_command_v_opt" to display information about the first bad
commit.

The goal of this patch is to avoid a "fork" and an "exec" call
when displaying that information.

To do that, we manually setup revision information as
"git diff-tree --pretty" would do it, and then use the
"log_tree_commit" function.

Signed-off-by: Christian Couder <redacted>
---
 bisect.c |   30 +++++++++++++++++++++++++++---
 1 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/bisect.c b/bisect.c
index c43c120..e94a77b 100644
--- a/bisect.c
+++ b/bisect.c

@@ -816,6 +816,31 @@ static void

check_good_are_ancestors_of_bad(const char *prefix) }

 /*
+ * This does "git diff-tree --pretty COMMIT" without one fork+exec.
+ */
+static void show_diff_tree(const char *prefix, struct commit
*commit) +{
+	static struct rev_info opt;

Oops, "static" can be removed, it's a copy-paste error, sorry.

Is that "can" or "must"?  If the answer is the latter, shouldn't the
function be renamed to make it clear it is a bisect specific thing?

I think he was referring to the 'static' in the variable declarations.

You are right. I will send a v2 of this patch anyway.

Thanks for this clarification,
Christian.