Re: [PATCH 1/2] cat-file: force flush of stdout on empty string
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Nov 6, 2021, 00:58 UTC
- Message-ID
- <xmqqsfwaumlc.fsf@gitster.g>
- In-Reply-To
- <2d687baeed82e7b90d383bad8e209f50e0ce8c87.1636149400.git.gitgitgadget@gmail.com>
"John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 9 quoted lines
> @@ -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.
Show 12 quoted lines
> 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);