reftable progress

reftable progress

[Han-Wen Nienhuys]

Hi folks,

I have some alpha-quality code for Reftable support in Git at

 https://github.com/hanwen/git/tree/reftable

I'd be curious for some feedback, both on the library
(https://github.com/google/reftable) and the glue code in Git.

-- 
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--
Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado

Re: reftable progress

[Junio C Hamano]

Han-Wen Nienhuys <hanwen@google.com> writes:

> Hi folks,
>
> I have some alpha-quality code for Reftable support in Git at
>
>  https://github.com/hanwen/git/tree/reftable
>
> I'd be curious for some feedback, both on the library
> (https://github.com/google/reftable) and the glue code in Git.

If you are asking for feedback, sendign it over to this list with 
[RFC PATCH n/3]  as the subject prefix would have better chance.

I have a feeling that the patch to show-ref is done at the wrong
level.  The show_ref() function is given as the callback function
to head_ref() and for_each_ref(), and the way these functions call
the callback function is part of the ref API.  "In the reftable
format, ... are stored in the reference database too," is perfectly
fine (that is the implementation detail of the ref API backend) but
"and are produced when iterating over the refs" is not.  Hide that
inside the ref API backend you are writing for reftable and this
change will become unnecessary.

I agree with the general direction of the second patch that extends
setup.c to allow .git/refs to be a regular file to signal that the
repository is using reftable (instead of filesystem-based refs), but
the title "fix repo detection" is misleading---it is adjusting for
the new format (as there is nothing to "fix" if there weren't the
new format).  Also, the actual implementation may want to be a bit
more strict, i.e. saying "we allow refs/ to be a searchable
directory, and we also allow a readable regular file".

Thanks.

Re: reftable progress

[Han-Wen Nienhuys]

On Thu, Jan 9, 2020 at 9:18 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Han-Wen Nienhuys <hanwen@google.com> writes:
>
> > Hi folks,
> >
> > I have some alpha-quality code for Reftable support in Git at
> >
> >  https://github.com/hanwen/git/tree/reftable
> >
> > I'd be curious for some feedback, both on the library
> > (https://github.com/google/reftable) and the glue code in Git.
>
> If you are asking for feedback, sendign it over to this list with
> [RFC PATCH n/3]  as the subject prefix would have better chance.

Thanks, I'll look into that when I am done with my current TODO list.

In the meantime, I've updated the code at the above URL. The major
missing item is currently reflogs; hopefully I'll get to that next
week.

I asked around, and it looks like we have slight preference for the
BSD license (https://developers.google.com/open-source/licenses/bsd).
According to the FSF, this is OK to combine with GPL software, so
would you be OK with that license?

thanks,

> I have a feeling that the patch to show-ref is done at the wrong
> level.  The show_ref() function is given as the callback function
> to head_ref() and for_each_ref(), and the way these functions call
> the callback function is part of the ref API.  "In the reftable
> format, ... are stored in the reference database too," is perfectly
> fine (that is the implementation detail of the ref API backend) but
> "and are produced when iterating over the refs" is not.  Hide that
> inside the ref API backend you are writing for reftable and this
> change will become unnecessary.

The API is a little surprising here, because it means that an iterator
should always dereference a symref, regardless of storage format.
Since it is not specific to the ref backend, this behavior should not
be in the ref backend code. But I've modified it now, and it seems to
work.

-- 
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--

Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich

Registergericht und -nummer: Hamburg, HRB 86891

Sitz der Gesellschaft: Hamburg

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado

Re: reftable progress

[Junio C Hamano]

Han-Wen Nienhuys <hanwen@google.com> writes:

> I asked around, and it looks like we have slight preference for the
> BSD license (https://developers.google.com/open-source/licenses/bsd).
> According to the FSF, this is OK to combine with GPL software, so
> would you be OK with that license?

Sure, I think we are happy with adopting BSD code (and it would keep
it more avaiable for other reimplementation of Git, which is good).

> The API is a little surprising here, because it means that an iterator
> should always dereference a symref, regardless of storage format.

I do not think forcing the callers to dereference is clearly better
(if so, we may want to consider changing the API to do so, but I do
not see such a strong reason).  Only when the vast majority of
callers would want just the name of the underlying ref without
learning the actual object value, such an optimization may make
sense, but it would make the callers more difficult to write
correctly (iow, somebody will forget to teach "this is a symref, so
we need to ignore the incoming oid parameter and call read_ref()
ourselves to learn the name of the object" to their each_ref_fn
callback functions, no?

Re: reftable progress

[Han-Wen Nienhuys]

On Tue, Jan 21, 2020 at 7:34 PM Junio C Hamano <gitster@pobox.com> wrote:
> > I asked around, and it looks like we have slight preference for the
> > BSD license (https://developers.google.com/open-source/licenses/bsd).
> > According to the FSF, this is OK to combine with GPL software, so
> > would you be OK with that license?
>
> Sure, I think we are happy with adopting BSD code (and it would keep
> it more avaiable for other reimplementation of Git, which is good).

Yep. I'll go ahead with the license change then. Thanks!


> > The API is a little surprising here, because it means that an iterator
> > should always dereference a symref, regardless of storage format.
>
> I do not think forcing the callers to dereference is clearly better
> (if so, we may want to consider changing the API to do so, but I do
> not see such a strong reason).  Only when the vast majority of
> callers would want just the name of the underlying ref without
> learning the actual object value, such an optimization may make
> sense, but it would make the callers more difficult to write
> correctly (iow, somebody will forget to teach "this is a symref, so
> we need to ignore the incoming oid parameter and call read_ref()
> ourselves to learn the name of the object" to their each_ref_fn
> callback functions, no?

There is an intermediate layer that wraps the refs/*-backend.c
functions where this could be done.

-- 
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--

Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich

Registergericht und -nummer: Hamburg, HRB 86891

Sitz der Gesellschaft: Hamburg

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado

Reftable progress

[Han-Wen Nienhuys]

Hi there,

I wanted to give a few updates on reftable. I'm posting a new version
of the patch series separately.  It has a number of interesting
improvements

* The source in the git tree now uses the strbuf library, which was
the largest stumbling block for integration.

*  The code is still completely self-contained (the
REFTABLE_STANDALONE #define includes 100 lines worth of strbuf
compatible code; enough to run the unittests.)

*  The number of test failures has dipped below 1000.

I would like to post the next version on top of Phillip's patches that
make ref_xxx functions take a struct repository* argument. Is there a
branch that I can target for rebasing?

The bottom 2 commits should be OK to merge as is into next.
("checkout: add '\n' to reflog message", "lib-t6000.sh: write tag
using git-update-ref")

The major open question is how to handle per-worktree refs.  My idea
is that, on creation, the reftable backend can figure out if it is
running in a worktree or in the main repository. If it is running in
worktree X, we could read/write pseudorefs as ~X/PSEUDO_REF.

Could we discuss next steps for merging at least the library? I think
that would solve one of the major complaints, which is that history is
kept in a separate repository.

-- 
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--
Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado

Re: Reftable progress

[Han-Wen Nienhuys]

On Tue, Jun 23, 2020 at 12:03 AM Han-Wen Nienhuys <hanwen@google.com> wrote:
> The bottom 2 commits should be OK to merge as is into next.
> ("checkout: add '\n' to reflog message", "lib-t6000.sh: write tag
> using git-update-ref")

The patch t3432: use git-reflog to inspect the reflog for HEAD can
also be merged as is.

> The major open question is how to handle per-worktree refs.  My idea
> is that, on creation, the reftable backend can figure out if it is
> running in a worktree or in the main repository. If it is running in
> worktree X, we could read/write pseudorefs as ~X/PSEUDO_REF.

I figured out a better way to do this. See the series that I am  posting today.

Test failures are down to 556.

> Could we discuss next steps for merging at least the library? I think
> that would solve one of the major complaints, which is that history is
> kept in a separate repository.

anyone? Junio, Jonathan?

-- 
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--
Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado

Re: Reftable progress

[Junio C Hamano]

Han-Wen Nienhuys <hanwen@google.com> writes:

> On Tue, Jun 23, 2020 at 12:03 AM Han-Wen Nienhuys <hanwen@google.com> wrote:
>> The bottom 2 commits should be OK to merge as is into next.
>> ("checkout: add '\n' to reflog message", "lib-t6000.sh: write tag
>> using git-update-ref")
>
> The patch t3432: use git-reflog to inspect the reflog for HEAD can
> also be merged as is.

Yeah, carving test updates in the early parts out of the series and
have them in 'next' and then in 'master' may make sense.  Good idea.

>> The major open question is how to handle per-worktree refs.  My idea
>> is that, on creation, the reftable backend can figure out if it is
>> running in a worktree or in the main repository. If it is running in
>> worktree X, we could read/write pseudorefs as ~X/PSEUDO_REF.
>
> I figured out a better way to do this. See the series that I am  posting today.

It seems that patches started trickling in but perhaps missing
pieces are mailspools somewhere and would arrive later?  I'll check
the list archive tomorrow to see if all 20 patches have arrived.

Thanks.