From: Ramsay Jones <ramsay@ramsay1•demon.co.uk>
To: Junio C Hamano <gitster@pobox•com>
Cc: Tanay Abhra <tanayabh@gmail•com>,
Matthieu Moy <Matthieu.Moy@grenoble-inp•fr>,
git@vger•kernel.org, Ramkumar Ramachandra <artagnon@gmail•com>
Subject: Re: [PATCH v8 0/8] Rewrite `git_config()` using config-set API
Date: Mon, 11 Aug 2014 10:59:13 +0100 [thread overview]
Message-ID: <53E893F1.9080408@ramsay1.demon.co.uk> (raw)
In-Reply-To: <xmqqppg88d8x.fsf@gitster.dls.corp.google.com>
On 10/08/14 18:29, Junio C Hamano wrote:
> 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
prev parent reply other threads:[~2014-08-11 9:59 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-08-06 14:53 [PATCH v8 0/8] Rewrite `git_config()` using config-set API Tanay Abhra
2014-08-06 14:53 ` [PATCH v8 1/8] config.c: mark error and warnings strings for translation Tanay Abhra
2014-08-06 14:53 ` [PATCH v8 2/8] config.c: fix accuracy of line number in errors Tanay Abhra
2014-08-06 14:53 ` [PATCH v8 3/8] add line number and file name info to `config_set` Tanay Abhra
2014-08-06 15:49 ` Ramsay Jones
2014-08-06 14:53 ` [PATCH v8 4/8] change `git_config()` return value to void Tanay Abhra
2014-08-06 14:53 ` [PATCH v8 5/8] config: add `git_die_config()` to the config-set API Tanay Abhra
2014-08-06 14:53 ` [PATCH v8 6/8] rewrite git_config() to use " Tanay Abhra
2014-08-06 14:53 ` [PATCH v8 7/8] add a test for semantic errors in config files Tanay Abhra
2014-08-06 14:53 ` [PATCH v8 8/8] add tests for `git_config_get_string_const()` Tanay Abhra
2014-08-06 15:32 ` Matthieu Moy
2014-08-06 15:44 ` Tanay Abhra
2014-08-06 21:22 ` Junio C Hamano
2014-08-07 8:30 ` Matthieu Moy
2014-08-06 15:26 ` [PATCH v8 0/8] Rewrite `git_config()` using config-set API Matthieu Moy
2014-08-07 19:03 ` Junio C Hamano
2014-08-07 19:35 ` Matthieu Moy
2014-08-07 20:31 ` Junio C Hamano
2014-08-08 14:07 ` Tanay Abhra
2014-08-08 14:32 ` Ramsay Jones
2014-08-10 17:29 ` Junio C Hamano
2014-08-11 9:59 ` Ramsay Jones [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=53E893F1.9080408@ramsay1.demon.co.uk \
--to=ramsay@ramsay1$(echo .)demon.co.uk \
--cc=Matthieu.Moy@grenoble-inp$(echo .)fr \
--cc=artagnon@gmail$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=gitster@pobox$(echo .)com \
--cc=tanayabh@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox