From: Phillip Wood <phillip.wood123@gmail.com>
Date: Wed, 23 Jul 2025 14:53:50 GMT
Subject: Re: [GSoC PATCH v5 2/5] repo: add the field references.format
Message-ID: <7d6ad7cb-e25a-41de-9588-6a3c1b0717e8@gmail.com>
In-Reply-To: <ldomqfgzts2fs3zuzuyfpsp4jsuec7a6ooisztqx6pe2373jzx@mqzh62weo2jm>

On 22/07/2025 20:25, Justin Tobler wrote:
> On 25/07/21 09:28PM, Lucas Seiki Oshiro wrote:
> 
>> +typedef const char *get_value_fn(struct repository *repo);
>> +
>> +struct field {
>> +	const char *key;
>> +	get_value_fn *add_field_callback;
>> +};
>> +
>> +static const char *get_references_format(struct repository *repo)
>> +{
>> +	return ref_storage_format_to_name(repo->ref_storage_format);
>> +}
>> +
>> +/* repo_info_fields keys should be in lexicographical order */
>> +static const struct field repo_info_fields[] = {
>> +	{ "references.format", get_references_format },
>> +};
> 
> Ok, so each key has a corresponding callback that is used to get its
> value. This works fine when we have one operation/callback per key, but
> I could see this being a bit inflexible in cases where performing a
> single operation could be expected to generate multiple keys worth of
> information at a time.
> 
> I certainly see this being the case with git-repo-stats where, for
> example, interating over references will produce multiple keyvalues
> indicating the number of branches, tags, remotes, etc. But, maybe for
> git-repo-info this will not be as much of a concern?

I think the fact that git_value_fn returns 'const char*' is a concern as 
it means we cannot return an allocated string. It would be better to 
pass a 'struct strbuf' to the callback and write the value to that 
instead. That way a callback can create the value piecemeal if needed 
and we don't have to worry about whether we should be free()ing the 
returned string.

An alternative approach would be to pass a function pointer to the 
callback which it then calls with the key and value to produce the output.

Thanks

Phillip
>> +
>> +static int repo_info_fields_cmp(const void *va, const void *vb)
>> +{
>> +	const struct field *a = va;
>> +	const struct field *b = vb;
>> +
>> +	return strcmp(a->key, b->key);
>> +}
>> +
>> +static get_value_fn *get_value_callback(const char *key)
>>   {
>> +	const struct field search_key = { key, NULL };
>> +	const struct field *found = bsearch(&search_key, repo_info_fields,
>> +					    ARRAY_SIZE(repo_info_fields),
>> +					    sizeof(struct field),
>> +					    repo_info_fields_cmp);
>> +	return found ? found->add_field_callback : NULL;
>> +}
>> +
>> +static int qsort_strcmp(const void *va, const void *vb)
>> +{
>> +	const char *a = *(const char **)va;
>> +	const char *b = *(const char **)vb;
>> +
>> +	return strcmp(a, b);
>> +}
>> +
>> +static int print_fields(int argc, const char **argv, struct repository *repo)
>> +{
>> +	const char *last = "";
>> +
>> +	QSORT(argv, argc, qsort_strcmp);
>> +
>> +	for (int i = 0; i < argc; i++) {
>> +		get_value_fn *callback;
>> +		const char *key = argv[i];
>> +		const char *value;
>> +
>> +		if (!strcmp(key, last))
>> +			continue;
>> +
>> +		callback = get_value_callback(key);
>> +
>> +		if (!callback)
>> +			return error("key %s not found", key);
>> +
>> +		value = callback(repo);
>> +		printf("%s=%s\n", key, value);
>> +		last = key;
>> +	}
> 
> If the user does not input any keys, we simply do nothing. I do wonder
> if this is really the best default behavior. Maybe instead we should
> error out? Or maybe treat it as though all keys were requested?
> 
> -Justin
> 
>> +
>>   	return 0;
>>   }
>>   
>> +static int repo_info(int argc, const char **argv, const char *prefix UNUSED,
>> +		     struct repository *repo)
>> +{
>> +	return print_fields(argc - 1, argv + 1, repo);
>> +}
>> +
>>   int cmd_repo(int argc, const char **argv, const char *prefix,
>>   	     struct repository *repo)
>>   {