Re: [PATCH 2/2] catfile.c: add --batch-command mode

[Phillip Wood <phillip.wood123@gmail.com>]

Hi John/Ævar

On 04/02/2022 12:11, Ævar Arnfjörð Bjarmason wrote:
> 
> On Thu, Feb 03 2022, John Cai via GitGitGadget wrote:
> 
>> From: John Cai <johncai86@gmail.com>
>> +		if (state == BATCH_STATE_INPUT && !cmd){
>> +			string_list_append(&revs, input.buf);
> 
> Nit: You can save yourself some malloc() churn here with:
> 
>      string_list_append_nodup(..., strbuf_detach(&input, NULL));
> 
> I.e. we're looping over the input, here we're done, so we might as well
> steal the already alloc'd string....

If we do that then the strbuf will have to reallocate its buffer when it 
reads the next input line on the next iteration of the loop. As the 
strbuf will have likely over allocated the buffer using the 
string_list_append_nodup() + strbuf_detach() will will be less efficient 
overall than using string_list_append().

Best Wishes

Phillip

>> +			continue;
>> +		}
>> +
>> +		if (!cmd)
>> +			die("unknown command: %s", input.buf);
>> +
>> +		state = cmd->next_state;
>> +		cmd->fn(opt, p, output, data, revs);
>> +	}
>> +	strbuf_release(&input);
>> +	string_list_clear(&revs, 0);
> 
> ...and these will do the right thing, as strbuf will notice the string
> is stolen (it'll be the slopbuf again), and due to the combination of
> *_DUP and *_nodup() we'll properly free it here too.
> 
>> +}
>> +
>>   static int batch_objects(struct batch_options *opt)
>>   {
>>   	struct strbuf input = STRBUF_INIT;
>> @@ -519,6 +665,7 @@ static int batch_objects(struct batch_options *opt)
>>   	struct expand_data data;
>>   	int save_warning;
>>   	int retval = 0;
>> +	const int command = opt->command;
>>   
>>   	if (!opt->format)
>>   		opt->format = "%(objectname) %(objecttype) %(objectsize)";
>> @@ -594,22 +741,25 @@ static int batch_objects(struct batch_options *opt)
>>   	save_warning = warn_on_object_refname_ambiguity;
>>   	warn_on_object_refname_ambiguity = 0;
>>   
>> -	while (strbuf_getline(&input, stdin) != EOF) {
>> -		if (data.split_on_whitespace) {
>> -			/*
>> -			 * Split at first whitespace, tying off the beginning
>> -			 * of the string and saving the remainder (or NULL) in
>> -			 * data.rest.
>> -			 */
>> -			char *p = strpbrk(input.buf, " \t");
>> -			if (p) {
>> -				while (*p && strchr(" \t", *p))
>> -					*p++ = '\0';
>> +	if (command)
>> +		batch_objects_command(opt, &output, &data);
>> +	else {
> 
> Style: {} braces for all arms if one requires it.
> 
>> +		while (strbuf_getline(&input, stdin) != EOF) {
>> +			if (data.split_on_whitespace) {
> 
> diff nit: maybe we can find some way to not require re-indenting the existing code. E.g.:
> 	
> 	if (command) {
> 		batch_objects_command(...);
> 	        goto cleanup;
> 	}
> 
> ...
> 
>> +				/*
>> +				 * Split at first whitespace, tying off the beginning
>> +				 * of the string and saving the remainder (or NULL) in
>> +				 * data.rest.
>> +				 */
>> +				char *p = strpbrk(input.buf, " \t");
>> +				if (p) {
>> +					while (*p && strchr(" \t", *p))
>> +						*p++ = '\0';
>> +				}
>> +				data.rest = p;
>>   			}
>> -			data.rest = p;
>> +			batch_one_object(input.buf, &output, opt, &data);
>>   		}
>> -
>> -		batch_one_object(input.buf, &output, opt, &data);
>>   	}
>>   
> 
> ...and then just add a "cleanup:" label here.
> 
>>   	strbuf_release(&input);
>> @@ -646,6 +796,7 @@ static int batch_option_callback(const struct option *opt,
>>   
>>   	bo->enabled = 1;
>>   	bo->print_contents = !strcmp(opt->long_name, "batch");
>> +	bo->command = !strcmp(opt->long_name, "batch-command");
>>   	bo->format = arg;
>>   
>>   	return 0;
>> @@ -682,6 +833,11 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
>>   			N_("show info about objects fed from the standard input"),
>>   			PARSE_OPT_OPTARG | PARSE_OPT_NONEG,
>>   			batch_option_callback),
>> +		OPT_CALLBACK_F(0, "batch-command", &batch, N_(""),
> 
> You're either missing a string here in "", or we don't need N_() to mark
> it for translation.
> 
>> +			 N_("enters batch mode that accepts commands"),
>> +			 PARSE_OPT_OPTARG | PARSE_OPT_NONEG,
>> +			 batch_option_callback),
>> +
>>   		OPT_BOOL(0, "follow-symlinks", &batch.follow_symlinks,
>>   			 N_("follow in-tree symlinks (used with --batch or --batch-check)")),
>>   		OPT_BOOL(0, "batch-all-objects", &batch.all_objects,
>> diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh
>> index 39382fa1958..7360d049113 100755
>> --- a/t/t1006-cat-file.sh
>> +++ b/t/t1006-cat-file.sh
>> @@ -85,6 +85,34 @@ $content"
>>   	test_cmp expect actual
>>       '
>>   
>> +    test -z "$content" ||
>> +    test_expect_success "--batch-command output of $type content is correct" '
>> +	maybe_remove_timestamp "$batch_output" $no_ts >expect &&
>> +	maybe_remove_timestamp "$(echo contents $sha1 | git cat-file --batch-command)" $no_ts >actual &&
>> +	test_cmp expect actual
>> +    '
>> +
>> +    test -z "$content" ||
>> +    test_expect_success "--batch-command session for $type content is correct" '
>> +	maybe_remove_timestamp "$batch_output" $no_ts >expect &&
>> +	maybe_remove_timestamp \
>> +		"$(test_write_lines "begin" "$sha1" "get contents" | git cat-file --batch-command)" \
>> +		$no_ts >actual &&
>> +	test_cmp expect actual
>> +    '
>> +
>> +    test_expect_success "--batch-command output of $type info is correct" '
>> +	echo "$sha1 $type $size" >expect &&
>> +	echo "info $sha1" | git cat-file --batch-command >actual &&
>> +	test_cmp expect actual
>> +    '
>> +
>> +    test_expect_success "--batch-command session for $type info is correct" '
>> +	echo "$sha1 $type $size" >expect &&
>> +	test_write_lines "begin" "$sha1" "get info" | git cat-file --batch-command >actual &&
>> +	test_cmp expect actual
>> +    '
>> +
>>       test_expect_success "custom --batch-check format" '
>>   	echo "$type $sha1" >expect &&
>>   	echo $sha1 | git cat-file --batch-check="%(objecttype) %(objectname)" >actual &&
>> @@ -141,6 +169,7 @@ test_expect_success '--batch-check without %(rest) considers whole line' '
>>   '
>>   
>>   tree_sha1=$(git write-tree)
>> +
> 
> stray newline addition.
> 
>>   tree_size=$(($(test_oid rawsz) + 13))
>>   tree_pretty_content="100644 blob $hello_sha1	hello"
>>   
>> @@ -175,7 +204,7 @@ test_expect_success \
>>       "Reach a blob from a tag pointing to it" \
>>       "test '$hello_content' = \"\$(git cat-file blob $tag_sha1)\""
>>   
>> -for batch in batch batch-check
>> +for batch in batch batch-check batch-command
>>   do
>>       for opt in t s e p
>>       do
>> @@ -281,6 +310,15 @@ test_expect_success "--batch-check with multiple sha1s gives correct format" '
>>       "$(echo_without_newline "$batch_check_input" | git cat-file --batch-check)"
>>   '
>>   
>> +test_expect_success "--batch-command with multiple sha1s gives correct format" '
>> +    echo "$batch_check_output" >expect &&
>> +    echo begin >input &&
>> +    echo_without_newline "$batch_check_input" >>input &&
>> +    echo "get info" >>input &&
>> +    git cat-file --batch-command <input >actual &&
>> +    test_cmp expect actual
>> +'
> 
> indentation with spaces, \t correctly used for the rest.