Re: [PATCH 1/2] cat-file: force flush of stdout on empty string

[Junio C Hamano <gitster@pobox.com>]

"John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:

> @@ -405,6 +405,11 @@ static void batch_one_object(const char *obj_name,
>  	int flags = opt->follow_symlinks ? GET_OID_FOLLOW_SYMLINKS : 0;
>  	enum get_oid_result result;
>  
> +	if (opt->buffer_output && obj_name[0] == '\0') {
> +		fflush(stdout);
> +		return;
> +	}
> +

This might work in practice, but it a bad design taste to add this
change here.  The function is designed to take an object name
string, and it even prepares a flag variable needed to make a call
to turn that object name into object data.  We do not need to
contaminate the interface with "usually this takes an object name,
but there are these other special cases ...".  The higher in the
callchain we place special cases, the better the lower level
functions become, as that allows them to concentrate on doing one
single thing well.

>  	result = get_oid_with_context(the_repository, obj_name,
>  				      flags, &data->oid, &ctx);
>  	if (result != FOUND) {
> @@ -609,7 +614,11 @@ static int batch_objects(struct batch_options *opt)
>  			data.rest = p;
>  		}
>  
> -		batch_one_object(input.buf, &output, opt, &data);
> +		 /*
> +		  * When in buffer mode and input.buf is an empty string,
> +		  * flush to stdout.
> +		  */

Checking "do we have the flush instruction (in which case we'd do
the flush here), or do we have textual name of an object (in which
case we'd call batch_one_object())?" here would be far cleaner and
results in an easier-to-explain code.  With a cleanly written code
to do so, it probably does not even need a new comment here.

This brings up another issue.  Is "flushing" the *ONLY* special
thing we would ever do in this codepath in the future?  I doubt so.
Squatting on an "empty string" is a selfish design that hurts those
who will come after you in the future, as they need to find other
ways to ask for a "special thing".

If we are inventing a special syntax that allows us to spell
commands that are distinguishable from a validly-spelled object name
to cause something special (like "flushing the output stream"),
perhaps we want to use a bit more extensible and explicit syntax and
use it from day one?

For example, if no string that begins with three dots can ever be a
valid way to spell an object name, perhaps "...flush" might be a
better "please do this special thing" syntax than an empty string.
It is easily extensible (the next special thing can follow suit to
say "...$verb" to tell the machinery to $verb the input).  When we
compare between an empty string and "...flush", the latter clearly
is more descriptive, too.

Note that I offhand do not know if "a valid string that name an
object would never begin with three-dot" is true.  Please check
if that is true if you choose to use it, or you can find and use
another convention that allows us to clearly distinguish the
"special" instruction and object names.

Thanks.

> +		 batch_one_object(input.buf, &output, opt, &data);
>  	}
>  
>  	strbuf_release(&input);