From: Jeff King <peff@peff•net>
To: Junio C Hamano <gitster@pobox•com>
Cc: Michael Haggerty <mhagger@alum•mit.edu>,
Stefan Beller <sbeller@google•com>,
git@vger•kernel.org
Subject: Re: [PATCH v2 08/12] initial_ref_transaction_commit(): function for initial ref creation
Date: Mon, 15 Jun 2015 14:57:46 -0400 [thread overview]
Message-ID: <20150615185746.GB10080@peff.net> (raw)
In-Reply-To: <xmqqy4jk95pw.fsf@gitster.dls.corp.google.com>
On Mon, Jun 15, 2015 at 11:39:07AM -0700, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum•mit.edu> writes:
>
> > It would seem to make sense to add a test here that there are no
> > existing references, because that is how the function *should* be
> > used. But in fact, the "testgit" remote helper appears to call it
> > *after* having set up refs/remotes/<name>/HEAD and
> > refs/remotes/<name>/master, so we can't be so strict.
>
> Is "testgit" so hard to fix (or so sacred to break) that we want to
> sacrifice the quality of this part that is nearer to the core?
I do not think "testgit" is sacred; it is there to serve the needs of
git and not the other way around. But we do have to stop and think
whether what "testgit" is doing is representative of what a real
remote-helper might do.
It has been a while since I touched the remote-helper code (and it has
always been under-documented and confusing, from my recollection), but I
think it is not "testgit" that is making the refs, but rather that
"import"-style helpers will feed a data stream to git-fast-import
(actually the transport code does it on behalf of the remote).
So "fast-import" creates the refs, and then "clone" later wants to
recreate them. IMHO this is not ideal[1]; we should have fast-import create
the objects and report the ref tips, which can then be relayed to clone.
But with the way the code works now, this is not about fixing "testgit",
but rather would break any "import"-style helper.
Take all of that with a grain of salt, though. I might be totally wrong
about how this part of the code works.
-Peff
[1] This may also be buggy for regular fetches. E.g., do we correctly
respect "--force" (or lack thereof) and "+"-refspecs when importing?
I am not sure, as I think we may pass enough information to the
helper to handle this itself. But certainly if the responsibility
for updating refs was always in the caller (clone or fetch), then it
would follow the regular code path and Just Work.
next prev parent reply other threads:[~2015-06-15 18:58 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-06-13 14:42 [PATCH v2 00/12] Improve "refs" module encapsulation Michael Haggerty
2015-06-13 14:42 ` [PATCH v2 01/12] delete_ref(): move declaration to refs.h Michael Haggerty
2015-06-13 14:42 ` [PATCH v2 02/12] remove_branches(): remove temporary Michael Haggerty
2015-06-13 14:42 ` [PATCH v2 03/12] delete_ref(): handle special case more explicitly Michael Haggerty
2015-06-15 18:22 ` Junio C Hamano
2015-06-13 14:42 ` [PATCH v2 04/12] delete_refs(): new function for the refs API Michael Haggerty
2015-06-13 14:42 ` [PATCH v2 05/12] delete_refs(): improve error message Michael Haggerty
2015-06-15 18:29 ` Junio C Hamano
2015-06-19 14:20 ` Michael Haggerty
2015-06-13 14:42 ` [PATCH v2 06/12] prune_remote(): use delete_refs() Michael Haggerty
2015-06-15 18:35 ` Junio C Hamano
2015-06-15 18:39 ` Jeff King
2015-06-15 19:45 ` Junio C Hamano
2015-06-13 14:42 ` [PATCH v2 07/12] repack_without_refs(): make function private Michael Haggerty
2015-06-13 14:42 ` [PATCH v2 08/12] initial_ref_transaction_commit(): function for initial ref creation Michael Haggerty
2015-06-15 18:04 ` Junio C Hamano
2015-06-15 18:39 ` Junio C Hamano
2015-06-15 18:57 ` Jeff King [this message]
2015-06-15 18:53 ` Junio C Hamano
2015-06-20 2:15 ` Michael Haggerty
2015-06-13 14:42 ` [PATCH v2 09/12] refs: remove some functions from the module's public interface Michael Haggerty
2015-06-13 14:42 ` [PATCH v2 10/12] initial_ref_transaction_commit(): check for duplicate refs Michael Haggerty
2015-06-13 14:42 ` [PATCH v2 11/12] initial_ref_transaction_commit(): check for ref D/F conflicts Michael Haggerty
2015-06-13 14:42 ` [PATCH v2 12/12] refs: move the remaining ref module declarations to refs.h Michael Haggerty
2015-06-15 18:13 ` Junio C Hamano
2015-06-15 18:35 ` Jeff King
2015-06-15 19:48 ` Junio C Hamano
2015-06-20 1:53 ` Michael Haggerty
2015-06-20 3:30 ` Junio C Hamano
2015-06-15 5:20 ` [PATCH v2 00/12] Improve "refs" module encapsulation Stefan Beller
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=20150615185746.GB10080@peff.net \
--to=peff@peff$(echo .)net \
--cc=git@vger$(echo .)kernel.org \
--cc=gitster@pobox$(echo .)com \
--cc=mhagger@alum$(echo .)mit.edu \
--cc=sbeller@google$(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