All of lore.kernel.org
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
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: Sun, 10 Aug 2014 10:29:18 -0700	[thread overview]
Message-ID: <xmqqppg88d8x.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: <53E4DF6D.8070904@ramsay1.demon.co.uk> (Ramsay Jones's message of "Fri, 08 Aug 2014 15:32:13 +0100")

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

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

You are right in that the "API" is giving more than the callers
converted to it at this point in the series.

Tanay is right in that the way the struct will be used, which is
illustrated by the example in the message you are responding to,
should be in the part of this series that gives the implementation
of the API before presenting the converted users, as the series
deems it part of the API to let the users peek into the struct.

It may turn out that we have to abstract it further when we need a
more elaborate data structure kept in the kv-info in the future.  At
that point it will become undesirable to keep giving the callers
direct access to it, because direct struct access means that the
particular aspect of the implementaiton detail will be cast in stone
and would not allow to be replaced with some other representation
that is more efficient for the implementation.

But as I said, what we see in this series can do for now.  The
future can come later ;-)

  reply	other threads:[~2014-08-10 17:29 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 [this message]
2014-08-11  9:59               ` Ramsay Jones

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=xmqqppg88d8x.fsf@gitster.dls.corp.google.com \
    --to=gitster@pobox.com \
    --cc=Matthieu.Moy@grenoble-inp.fr \
    --cc=artagnon@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=ramsay@ramsay1.demon.co.uk \
    --cc=tanayabh@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.