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

> 
> 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....

>> +			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.