git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v8 0/8] Rewrite `git_config()` using config-set API

From
Ramsay Jones <ramsay@ramsay1.demon.co.uk>
Date
Aug 11, 2014, 09:59 UTC
Message-ID
<53E893F1.9080408@ramsay1.demon.co.uk>
In-Reply-To
<xmqqppg88d8x.fsf@gitster.dls.corp.google.com>
On 10/08/14 18:29, Junio C Hamano wrote:
Show 20 quoted lines
> Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:
> 
>> On 08/08/14 15:07, Tanay Abhra wrote:
>> ...
>>> (cc to Ramsay)
>>>
>>> The discussion in both threads (v8 and v9), boils down to this,
>>> is the `key_value_info` struct really required to be declared public or should be
>>> just an implementation detail. I will give you the context,
>>
>> No, this is not the issue for me. The patch which introduces the
>> struct in cache.h does not make use of that struct in any interface.
>> It *is* an implementation detail of some code in config.c only.
>>
>> I do not know how that structure will be used in future patches. ;-)
> 
> It is debatable if it is a good API that tells the users "In the
> data I return to you, there is a structure hanging there with these
> two fields. Feel free to peek into it if you need what is recorded
> in them".
There is no debate in my mind. ;-)
In this future patch, the movement of the structure out of config.c would
be plain for everyone to see, and (hopefully) debate the merits of such
an 'interface'. Along with checking that it is properly documented. (which
patch should the documentation be in?) Where should it be documented?
 
>              But the contract between the the endgame "API" and its
> callers can include such a direct access, and then you can say that
> it is a part of the API, not just an implementation detail.

Sure. It's just a question of timing and allowing reviewers to see the actual change in one patch.

> I think you and Tanay are both right (and I am wrong ;-).
;-)

I don't have *very* strong feelings about this, which is why I didn't respond to the earlier replies by Tanay and Matthieu, but since I was cc-ed on this thread ... (It seemed to me that my comments had been misunderstood).

So yes, I think this "API" is ugly and could be improved upon, but I don't care sufficiently to make any further comment. :-D

ATB, Ramsay Jones

Previous: Junio C Hamano
Message 22 of 22 in “Rewrite `git_config()` using config-set API”
  1. 0/8 Rewrite `git_config()` using config-set APITanay Abhra, Aug 6, 2014
  2. 1/8 config.c: mark error and warnings strings for translationTanay Abhra, Aug 6, 2014
  3. 2/8 config.c: fix accuracy of line number in errorsTanay Abhra, Aug 6, 2014
  4. 3/8 add line number and file name info to `config_set`Tanay Abhra, Aug 6, 2014
  5. Ramsay JonesAug 6, 2014
  6. 4/8 change `git_config()` return value to voidTanay Abhra, Aug 6, 2014
  7. 5/8 config: add `git_die_config()` to the config-set APITanay Abhra, Aug 6, 2014
  8. 6/8 rewrite git_config() to use the config-set APITanay Abhra, Aug 6, 2014
  9. 7/8 add a test for semantic errors in config filesTanay Abhra, Aug 6, 2014
  10. 8/8 add tests for `git_config_get_string_const()`Tanay Abhra, Aug 6, 2014
  11. Matthieu MoyAug 6, 2014
  12. Tanay AbhraAug 6, 2014
  13. Junio C HamanoAug 6, 2014
  14. Matthieu MoyAug 7, 2014
  15. Matthieu MoyAug 6, 2014
  16. Junio C HamanoAug 7, 2014
  17. Matthieu MoyAug 7, 2014
  18. Junio C HamanoAug 7, 2014
  19. Tanay AbhraAug 8, 2014
  20. Ramsay JonesAug 8, 2014
  21. Junio C HamanoAug 10, 2014
  22. Ramsay JonesAug 11, 2014

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.