git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Glen Choo <chooglen@google.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH v2 0/3] remote: replace static variables with struct remote_state
Date: Wed, 13 Oct 2021 16:37:20 -0700	[thread overview]
Message-ID: <xmqqmtncebtb.fsf@gitster.g> (raw)
In-Reply-To: <kl6ltuhk1tdg.fsf@chooglen-macbookpro.roam.corp.google.com> (Glen Choo's message of "Wed, 13 Oct 2021 14:56:27 -0700")

Glen Choo <chooglen@google.com> writes:

> My primary line of thinking is that adding the backpointer from struct
> branch to struct repository adds "unnecessary" coupling between the two
> objects,...

meaning, that the paragraphs below will explain why the reference is
unnecessary?

> ...which causes the more concrete problems of:

> * Inconsistency between other functions that use struct repository as a
>   "context object". We now find ourselves having to justify why branch
>   functions can bypass the context object using a special pointer,
>   whereas other functions and structs (say, struct object) cannot.

Given that the API has been evolved over time from a "we deal only
with a single repository and everybody else can just fork into a
separate repository" to "now this function can work in the specified
repository", it is understandable that some still do take repository
as a separate parameter, even though the original counterpart that
did not take a repository pointer took an object that clearly can
belong to only one repository at a time.  They would need to be
cleaned up, and they do not make a good excuse to add more of them,
when we know we are dealing with objects that belong only to one
repository, like the "remote.c" contents we have been discussing.

IOW, we need to start somewhere to clean things up, so either we
teach multi-repository support to remote.c from day one, or we pass
a repository as a separate and redundant parameter, with the
intention to later clean up.  I suspect that this piece is isolated
enough that it is simpler to use as a starting point, and then the
callers into remote.c API can then be cleaned up, gradually extending
the circle.

The original counerpart may not have been about "the other object"
to begin with (e.g. "we have this string (not an object); resolve it
to an object name in the context of the given repository"), in which
case they are fine as they are, but I think most of the contents of
"remote.c" do not fall into this category.

> * Interface misuse, where callers can now dereference branch->repository
>   even though it is meant to be internal. This is also a possible source
>   of inconsistency.

I do not think it is meant to be internal to begin with.  If we
declare, after careful consideration, that an in-core branch object
given by remote.c API always belongs to one and only one repository,
then following the branch->repository pointer should be the
documented and kosher way to find out the repository by anybody who
is given the branch object.  A function that takes repo and branch
to pretend as if it can work with an inconsistent pair is an
invidation for the interface misuse, but I do not think it is a
misuse of the API for a codepath that obtained a branch instance
inspecting what repository it came from.

A function that takes a pair of remote and a branch object for
whatever reason may want to have an assert() that makes sure they
came from the same repository, by the way.

Also, some of the functions involved in this topic may need to say
"I have this string, please resolve it as an object name in the
context of this repository", and call a function that takes a
repository as "context".  The repository they use must be given by
their caller somehow---is there any valid case where it must be
different from the other parameter (i.e. contained object) they are
dealing with?  I do not think so.  So such a function would have to
say "resolve this string in the context of the repository
branch->repo" because the function was given an in-core branch
instance.

> * Extra memory consumption.

This is true.  It however is the only valid excuse I can fully agree
with for being hesitant to have "where did I come from" information.
We do grow each in-core branch and remote instance by an 8-byte
pointer (but we reduce the stackframe consumption by 8-byte along
each step in the callchain as we do not have to pass a repository as
a separate pointer parameter).

> objects). I do not think we are in the same position with struct branch;
> struct branch seems sufficiently separated to me.

Sufficiently separated in the sense that you take a branch from the
parent repository and throwing it at a function with a pointer to an
in-core repository for a submodule would make some sort of sense?  

Sorry, but I do not follow this part of your argument, at all.

  reply	other threads:[~2021-10-13 23:37 UTC|newest]

Thread overview: 56+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-10-07 19:07 [PATCH 0/2] remote: replace static variables with struct remote_state Glen Choo via GitGitGadget
2021-10-07 19:07 ` [PATCH 1/2] remote: move static variables into struct Glen Choo via GitGitGadget
2021-10-07 23:36   ` Junio C Hamano
2021-10-07 19:07 ` [PATCH 2/2] remote: add remote_state to struct repository Glen Choo via GitGitGadget
2021-10-07 23:39   ` Junio C Hamano
2021-10-08 17:30     ` Glen Choo
2021-10-13 19:31 ` [PATCH v2 0/3] remote: replace static variables with struct remote_state Glen Choo
2021-10-13 19:31   ` [PATCH v2 1/3] remote: move static variables into per-repository struct Glen Choo
2021-10-13 20:21     ` Junio C Hamano
2021-10-14 17:25       ` Glen Choo
2021-10-14 18:33         ` Junio C Hamano
2021-10-13 19:31   ` [PATCH v2 2/3] remote: use remote_state parameter internally Glen Choo
2021-10-13 20:23     ` Junio C Hamano
2021-10-13 19:31   ` [PATCH v2 3/3] remote: add struct repository parameter to external functions Glen Choo
2021-10-13 20:24     ` Junio C Hamano
2021-10-13 20:11   ` [PATCH v2 0/3] remote: replace static variables with struct remote_state Junio C Hamano
2021-10-13 20:27     ` Junio C Hamano
2021-10-13 22:00       ` Glen Choo
2021-10-13 21:56     ` Glen Choo
2021-10-13 23:37       ` Junio C Hamano [this message]
2021-10-14  1:25         ` Glen Choo
2021-10-19 22:43   ` [PATCH v3 0/4] " Glen Choo
2021-10-19 22:43     ` [PATCH v3 1/4] remote: move static variables into per-repository struct Glen Choo
2021-10-19 22:43     ` [PATCH v3 2/4] remote: use remote_state parameter internally Glen Choo
2021-10-20 19:45       ` Junio C Hamano
2021-10-20 20:31         ` Junio C Hamano
2021-10-20 22:08           ` Junio C Hamano
2021-10-25 18:09           ` Glen Choo
2021-10-25 19:36             ` Glen Choo
2021-10-25 20:33               ` Junio C Hamano
2021-10-25 23:00                 ` Glen Choo
2021-10-26  0:45                   ` Junio C Hamano
2021-10-26  1:22                     ` Junio C Hamano
2021-10-26 17:04                       ` Glen Choo
2021-10-27  2:28                         ` Junio C Hamano
2021-10-27 17:59                           ` Glen Choo
2021-10-27 20:03                             ` Junio C Hamano
2021-10-19 22:43     ` [PATCH v3 3/4] remote: remove the_repository->remote_state from static methods Glen Choo
2021-10-19 22:43     ` [PATCH v3 4/4] remote: add struct repository parameter to external functions Glen Choo
2021-10-28 18:30     ` [PATCH v4 0/6] remote: replace static variables with struct remote_state Glen Choo
2021-10-28 18:30       ` [PATCH v4 1/6] t5516: add test case for pushing remote refspecs Glen Choo
2021-10-28 20:17         ` Junio C Hamano
2021-11-15 18:42         ` Jonathan Tan
2021-11-15 20:09           ` Glen Choo
2021-10-28 18:30       ` [PATCH v4 2/6] remote: move static variables into per-repository struct Glen Choo
2021-10-28 18:30       ` [PATCH v4 3/6] remote: use remote_state parameter internally Glen Choo
2021-10-28 18:30       ` [PATCH v4 4/6] remote: remove the_repository->remote_state from static methods Glen Choo
2021-11-15 18:48         ` Jonathan Tan
2021-10-28 18:31       ` [PATCH v4 5/6] remote: die if branch is not found in repository Glen Choo
2021-11-15 18:50         ` Jonathan Tan
2021-11-15 20:06           ` Glen Choo
2021-11-16 17:45             ` Jonathan Tan
2021-10-28 18:31       ` [PATCH v4 6/6] remote: add struct repository parameter to external functions Glen Choo
2021-11-15 18:55         ` Jonathan Tan
2021-11-15 21:44           ` Glen Choo
2021-11-12  0:01       ` [PATCH v4 0/6] remote: replace static variables with struct remote_state Glen Choo

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

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqmtncebtb.fsf@gitster.g \
    --to=gitster@pobox.com \
    --cc=chooglen@google.com \
    --cc=git@vger.kernel.org \
    /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.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).