Re: [PATCH] Enable git rev-list to parse --quiet

Nick Andrew [off-list ref] writes:

...Without
a working "--quiet" nor exit code I can pipe the output to "wc -l"
but is there a more efficient/reliable way to implement the requirement?

Did you read the whole thread before asking the above question?

IOW, does this answer the above question?

    http://mid.gmane.org/7vy73zd8ok.fsf@gitster.siamese.dyndns.org

On Sun, Jul 20, 2008 at 12:56:28AM -0700, Junio C Hamano wrote:

Nick Andrew [off-list ref] writes:

quoted

...Without
a working "--quiet" nor exit code I can pipe the output to "wc -l"
but is there a more efficient/reliable way to implement the requirement?

Did you read the whole thread before asking the above question?

I took your answer to mean that I shouldn't be using git-rev-list
for this purpose, so I asked whether there's a better way to do
it. Johannes Schindelin gave a good answer to that.

IOW, does this answer the above question?

    http://mid.gmane.org/7vy73zd8ok.fsf@gitster.siamese.dyndns.org

I'm not happy with that patch due to this:

 static void finish_commit(struct commit *commit)
 {
+       if (check_empty)
+               exit(0);

Exiting a process from within a callback function seems to me to violate
the principle of least surprise. If the return code should be zero then
the cmd_rev_list function should return zero, and run_command will
return zero and handle_internal_command will exit zero. There must be
a better way to avoid redundant processing for the empty set case.

Nick.

Nick Andrew [off-list ref] writes:

Exiting a process from within a callback function seems to me to violate
the principle of least surprise.

Huh?  Who is surprised?

I do not know who taught you that "do not exit in a callback" dogma, but I
suspect it was misrepresented when it was taught to you.

A library that calls your function back could be structured this way:

	lib() {
        	perform some set-up that affects external world;
                call your callback function;
                clean-up the effect of previous set-up action;
	}

and exiting from your callback function is not a good idea as it prevents
the library from doing the necessary clean-up in such a case.

But that is true just in a(n extremely) general case.  Your generalization
is not particularly useful, methinks, and use of exit(0) in the patch is
very well justified (rather, I do not think they even need justifying).

 - The callback you are looking at is not a general purpose callback for
   other program's use, but written for a specific use of rev-list;

 - The purpose of that exit(0) is to signal "there is something" as
   quickly as possible, which was what you wanted out of rev-list;

 - Revision traversal is a read-only operation and we know that there is
   no externally visible set-up done in the function you are calling to
   get your callback called, that needs cleaning up later --- this is not
   expected to change, as there are longstanding existing callback
   functions supplied to traverse_commit_list() that die() upon seeing
   errors already.