List for cgit developers and users
 help / color / mirror / Atom feed
From: richard.maw at gmail.com (Richard)
Subject: [PATCH 06/16] Look up refs in namespace with cgit_get_sha1
Date: Sat, 30 Jul 2016 16:21:03 +0100	[thread overview]
Message-ID: <CAB2VqoZq3LYjG3uL2yDGg-=j2q854hXC6Qbd6dRrn+xj9R-WyA@mail.gmail.com> (raw)
In-Reply-To: <CAB2VqoaWUJgUfps9xiPtPFPfKDFrx_LFxAx_DFtC99zJy6Hv=w@mail.gmail.com>

On 29 July 2016 at 17:54, Richard <richard.maw at gmail.com> wrote:
> On 29 July 2016 at 16:34, Richard <richard.maw at gmail.com> wrote:
>> I'll have a go. I don't know how successful adding a feature for a
>> different project would be.
>
> After I've worked out how to add the support to libgit I'll know better,
> but it might be a good idea to:
>
> 1.  Make a patch to add support to libgit
> 2.  Port that patch into cgit,
>     so when the libgit patch is submitted I can point to cgit
>     to say that we're using cgit as a proving ground for the change.
> 3.  Submit namespace support patch to libgit.
> 4.  When there's a release with namespace support,
>     update to it and remove the patch from cgit.
>
> I'm not sure whether it would be better to transplant that part of
> libgit into cgit,
> or to patch libgit at build time.

At a first approximation, the change needed is to make everything that
interacts with the files backend have a namespace aware variant,
then make these variants the ones that get called when we are using a namespace.

There is prior-art for this in the initial namespaces implementation,
which only added a for_each_ref equivalent,
which calls do_for_each_ref with a namespaced ref prefix.

The first issue with this is that the full set of operations is:

pack_refs,
resolve_gitlink_ref, read_raw_ref, peel_ref, verify_refname_available,
do_for_each_ref
rename_ref, delete_refs, ref_transaction_commit, initial_ref_transaction_commit,
create_symref, set_worktree_head_symref,
reflog_exists, for_each_reflog_ent_reverse, for_each_reflog_ent,
for_each_reflog,
safe_create_reflog, files_log_ref_write, delete_reflog and reflog_expire.

pack_refs is the only one of those functions that wouldn't need to be
made namespace aware.
So to start with there's approximately 90 call-sites that would need
to be made namespace aware.

We can cull that further by only considering the interfaces for reading refs:

resolve_gitlink_ref, read_raw_ref, peel_ref, verify_refname_available,
do_for_each_ref
reflog_exists, for_each_reflog_ent_reverse, for_each_reflog_ent,
for_each_reflog,

This makes it about 50 call sites.

We would then need to work backwards from these functions to the APIs
we want to call (which is likely to be a significant fraction of
libgit) and add a way to thread through the call chains that we want
to opt into using the current namespace.

We would need to make some new command-line programs namespace aware,
otherwise git upstream would have no reason to accept the patches,
and we'd need to thread the flag for using the current namespace all
the way to the command-line,
rather than just detecting whether GIT_NAMESPACE was set,
since hook scripts are called with GIT_NAMESPACE set if it was set for
git-http-backend.

The age file script that was updated in this patch series would stop working
if git for-each-ref suddenly started to only handle refs in
GIT_NAMESPACE without an opt-in.


If we were to instead drop support for short branch names in the CGit API,
or continue to use our simpler dwim_ref implementation,
the only function that needs to be made namespace aware is resolve_ref_unsafe,
which we are currently handling by wrapping it with something that
translates the ref names back and forth.


In summary, I failed to find a nice way to add namespace support,
looking both top-down from the API and bottom-up from how it reads refs.
It would be a huge undertaking which reduces the odds of it being
accepted upstream,
but in doing this analysis I'm more sure that the work that's already
been submitted to cgit
is the smallest change necessary to support namespaced ref lookups.

Therefore I suggest it is best to stick with the current approach,
and the next version I submit would mark which functions should be revisited
if git gained greater support for namespaced ref resolution.
If this is a sticking point then I will attempt to make the changes in libgit,
but am uncertain whether it would be successful.


  reply	other threads:[~2016-07-30 15:21 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-07-15 22:59 [PATCH 00/16] [V2] Add namespace support to cgit richard.maw
2016-07-15 22:59 ` [PATCH 01/16] Fix archive generation richard.maw
2016-07-15 22:59 ` [PATCH 02/16] Add a wrapper for get_sha1 called cgit_get_sha1 richard.maw
2016-07-15 22:59 ` [PATCH 03/16] Parse repo.namespace richard.maw
2016-07-15 22:59 ` [PATCH 04/16] Print out parsed namespace on request richard.maw
2016-07-15 22:59 ` [PATCH 05/16] Set GIT_NAMESPACE when repo.namespace is provided richard.maw
2016-07-15 22:59 ` [PATCH 06/16] Look up refs in namespace with cgit_get_sha1 richard.maw
2016-07-29 14:32   ` Jason
     [not found]     ` <CAB2VqoZZiAsWpA1YbXARLaq8VMcWLJyXDG6-w0ag1JBOp_0M0Q@mail.gmail.com>
2016-07-29 16:54       ` richard.maw
2016-07-30 15:21         ` richard.maw [this message]
2016-07-15 22:59 ` [PATCH 07/16] Guess the default branch based on current namespace richard.maw
2016-07-29 14:36   ` Jason
2016-07-29 15:42     ` richard.maw
2016-07-15 22:59 ` [PATCH 08/16] Add cgit_for_each_namespaced_ref_in helper richard.maw
2016-07-15 22:59 ` [PATCH 09/16] Find the default branch based on the contents of the namespace richard.maw
2016-07-15 22:59 ` [PATCH 10/16] Only display refs in current namespace richard.maw
2016-07-15 22:59 ` [PATCH 11/16] Add namespace support to dumb-clone richard.maw
2016-07-29 14:38   ` Jason
2016-07-15 22:59 ` [PATCH 12/16] Display notes from namespace richard.maw
2016-07-29 14:44   ` Jason
2016-07-29 15:56     ` richard.maw
2016-07-15 22:59 ` [PATCH 13/16] Add documentation for repo.namespace richard.maw
2016-07-29 14:48   ` Jason
2016-07-29 15:59     ` richard.maw
2016-07-15 23:00 ` [PATCH 14/16] Allow agefile to be set per-repository richard.maw
2016-07-15 23:00 ` [PATCH 15/16] Update contrib script to update agefiles per namespace richard.maw
2016-07-29 14:51   ` Jason
2016-07-29 16:01     ` richard.maw
2016-07-15 23:00 ` [PATCH 16/16] Add documentation for repo.agefile richard.maw
2016-07-15 23:10 ` [PATCH 00/16] [V2] Add namespace support to cgit richard.maw
2016-07-28 16:40   ` richard.maw
2016-07-28 21:20     ` Jason

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='CAB2VqoZq3LYjG3uL2yDGg-=j2q854hXC6Qbd6dRrn+xj9R-WyA@mail.gmail.com' \
    --to=cgit@lists.zx2c4.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 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 NNTP newsgroup(s).