git: Add tagging support #928

vancluever · 2018-08-22T00:44:37Z

This adds a few new higher-level functions:

CreateTag, which can be used to create both lightweight and annotated
tags with a supplied TagObjectOptions struct. PGP signing is possible as
well.
Tag, to fetch a single tag ref.
DeleteTag, to delete a tag. This simply deletes the ref. The object is
left orphaned to be GCed later (maybe, see below).

I'm not 100% sure if DeleteTag is the correct behavior - looking for
details on exactly what happens to a tag object if you delete the ref
and not the tag were sparse, and groking the Git source did not really
produce much insight to the untrained eye. This may be something that
comes up in review. If deletion of the object is necessary, the
in-memory storer may require some updates to allow DeleteLooseObject to
be supported.

Also, similar to how commit encoding needed to be corrected to avoid corruption of the commit for verification, tag objects needed the same thing. That has been added in another commit.

This adds a few methods: * CreateTag, which can be used to create both lightweight and annotated tags with a supplied TagObjectOptions struct. PGP signing is possible as well. * Tag, to fetch a single tag ref. As opposed to Tags or TagObjects, this will also fetch the tag object if it exists and return it along with the output. Lightweight tags just return the object as nil. * DeleteTag, to delete a tag. This simply deletes the ref. The object is left orphaned to be GCed later. I'm not 100% sure if DeleteTag is the correct behavior - looking for details on exactly *what* happens to a tag object if you delete the ref and not the tag were sparse, and groking the Git source did not really produce much insight to the untrained eye. This may be something that comes up in review. If deletion of the object is necessary, the in-memory storer may require some updates to allow DeleteLooseObject to be supported. Signed-off-by: Chris Marchesi <chrism@vancluevertech.com>

As with the update in ec3d2a8, tag encoding needed to be corrected to ensure extra newlines were not being added in during tag object encoding, so that it did not corrupt the object for verification. Signed-off-by: Chris Marchesi <chrism@vancluevertech.com>

vancluever · 2018-08-22T00:55:50Z

Looks like some encoding-related tests may be failing. I'll check these out when I have a chance (read: soon)!

It should be noted that Decode and Verify have not been touched, so more than likely the expected data in the failing tests may just need to be adjusted.

Tag encoding/decoding seems to be a lot more sensitive to requiring the exact expected format in the object, which generally includes messages canonicalized so that they have a newline on the end (even if they didn't before). As such, the message should be written with the newline (no need for an extra), and the PGP signature right after that, which will be newline split already, so there's no need to split it again. All of this means it's very important for the caller to send the message in the correct format - which I'm correcting in the next commit. Signed-off-by: Chris Marchesi <chrism@vancluevertech.com>

Tag messages are highly sensitive to being in the expected format, especially when encoding/decoding for PGP verification. As such, we do a simple trimming of whitespace on the incoming message and add a newline on the end, to ensure there are no surprises here. Signed-off-by: Chris Marchesi <chrism@vancluevertech.com>

The old one was created with defaults, which would have caused CI failures in 2 years. The new one is valid for 10 years: > gpg --list-secret-keys /root/.gnupg/pubring.kbx ------------------------ sec rsa4096 2018-08-22 [SC] [expires: 2028-08-19] 93A17FF01E54328546087C8E029395402EFCCD53 uid [ unknown] foo bar <foo@foo.foo> Signed-off-by: Chris Marchesi <chrism@vancluevertech.com>

vancluever · 2018-08-22T05:39:16Z

So the issue was not necessarily with the encoding/decoding.

Ultimately, the encoder/decoder expects the message to be in a canonicalized format where there is always a newline, even for short messages (no body). Assuming this, the (new) encoder was ultimately adding on extra newlines when it should not have been.

To correct this I've started to do canonicalization in the higher level - TagObjectOptions.Message is now run through strings.TrimSpace with a newline added at the end, to make sure that any incoming message is corrected, regardless of what shape it's in.

I figured there was a way to do this without having to have TagObjectOptions supply this in - there is. Added support for this in and removed the object type from TagObjectOptions. Signed-off-by: Chris Marchesi <chrism@vancluevertech.com>

vancluever · 2018-08-23T17:18:15Z

Made a small update today so you don't need to actually pass in the object type for an annotated tag. This will now be fetch from the passed in hash.

vancluever · 2018-08-23T18:03:42Z

I'm kind of finding how much fetching a tag object is not necessarily needed. As such, I may adjust Tag() so that it does not return the tag object as well, leaving that to be fetched/followed by the caller.

Contrary to the docs too, all tags have references (see https://git-scm.com/book/en/v2/Git-Internals-Git-References, Tags section), so the current Tags() iterator will actually work for fetching annotated tags too. The only difference is that the caller has to pull the object data manually by following the hash in the ref.

I've seen a lot of this in preliminary use, which is why I'm thinking it's not really necessary to return both:

ref, _, err := r.Tag(...)
_, obj, err := r.Tag(...)

I've mainly noticed that in using the current Tag function, that there were lots of times that I was ignoring the ref or the object, depending on what I needed. This was evident in the tests as well. As such, I think it just makes more sense for a singular tag fetcher to return just a ref, through which the caller may grab the annotation if they need it, and if it exists. Also, contrary to the docs on Tags, all tags have a ref, even if they are annotated. The difference between a lightweight tag and an annotated tag is the presence of the tag object, which the ref will point to if the tag is annotated. As such I've adjusted the docs with an example as to how one can get the annotation for a tag ref through the iterator. Source: https://git-scm.com/book/en/v2/Git-Internals-Git-References, tags section. Signed-off-by: Chris Marchesi <chrism@vancluevertech.com>

smola

Thank you for your contribution!
Just a minor comment + a request on DeleteTag behaviour.

smola · 2018-08-31T08:54:22Z

repository.go

+	return b.String(), nil
+}
+
+// Tag fetches a tag from the repository.


Could you avoid the word fetch here? Maybe just "returns"? I would rather avoid the possible confusion of any relation with actual git fetch.

smola · 2018-08-31T09:31:10Z

repository.go

+		return err
+	}
+
+	obj, err := r.TagObject(ref.Hash())


Git does not delete the object even if it's a loose object. It just defers to prune as you pointed out. I think we should do as git, not just for compatibility, but also to avoid divergences with different storage implementations.

It would be nice to have a test for the Prune function that checks that if you create an annotated tag and delete the lightweight one, the annotated tag is effectively deleted.

I actually thought I removed this, because as mentioned in the original PR message it's actually not even possible to test this with an annotated tag right now due to limitations of the in-memory storage interface.

I'll get it removed and add a test to delete an annotated tag with a Prune on the end.

smola · 2018-08-31T09:31:34Z

repository.go

-// Tags returns all the References from Tags. This method returns only lightweight
-// tags. Note that not all the tags are lightweight ones. To return annotated tags
-// too, you need to call TagObjects() method.
+// Tags returns all the tag References in a repository.


Thanks for the correction, this was, indeed, confusing at least!

smola · 2018-08-31T09:32:52Z

repository.go

+	if opts != nil {
+		target, err = r.createTagObject(name, hash, opts)
+		if err != nil {
+			return nil, err


I verified with plain git that if the tag object creation succeeds and the lightweight creation fails, it does not try to clean up and just leaves a dangling tag object behind. So we're good here (I had doubts initially).

This is to avoid any ambiguity with the act of "fetching" in git in general. Signed-off-by: Chris Marchesi <chrism@vancluevertech.com>

This is necessary to support pruning on Tag objects. Signed-off-by: Chris Marchesi <chrism@vancluevertech.com>

Deleting a tag ref for an annotated tag in normal git behavior does not delete the tag object right away. This is handled by the normal GC process. Signed-off-by: Chris Marchesi <chrism@vancluevertech.com>

Added a couple of tests for annotated tag deletion: * The first one is a general test and should work regardless of the fixture used - the tag object could possibly be packed, so we do a prune *and* a repack operation before testing to see if the object was GCed correctly. * The second one actually creates the tag to be deleted, so that the tag object gets created as a loose, unpacked object. This is so we can effectively test that purning unpacked objects is now working 100% correctly (this was failing before because tag objects were not supported for walking). Signed-off-by: Chris Marchesi <chrism@vancluevertech.com>

vancluever · 2018-09-07T16:03:06Z

Hey @smola! All the stuff has been added now.

Prune was actually not working for tags as tag objects were not supported for walking, incidentally enough. This has been added now. One of the annotated tag deletion tests exclusively tests unpacked objects too, so we know that Prune is working 100% correctly.

options.go

Just renaming the TagObjectOptions type to CreateTagOptions so that it's consistent with the other option types. Signed-off-by: Chris Marchesi <chrism@vancluevertech.com>

vancluever · 2018-09-07T20:53:46Z

@smola I mentioned it in Slack but just wanted to note (versus just resolving the conversation) that I made the naming updates as well. Thanks!

smola · 2018-09-10T08:51:54Z

cc @mcuadros

mcuadros · 2018-09-10T09:17:51Z

@vancluever thanks for the contribution.

…upport

Signed-off-by: Máximo Cuadros <mcuadros@gmail.com>

vancluever · 2018-09-10T14:38:53Z

Thanks @mcuadros!

vancluever added 2 commits August 21, 2018 17:43

vancluever added 3 commits August 21, 2018 22:20

git: Discern tag target type from supplied hash

119459a

I figured there was a way to do this without having to have TagObjectOptions supply this in - there is. Added support for this in and removed the object type from TagObjectOptions. Signed-off-by: Chris Marchesi <chrism@vancluevertech.com>

mcuadros requested a review from smola August 29, 2018 09:21

smola suggested changes Aug 31, 2018

View reviewed changes

vancluever added 4 commits September 7, 2018 09:00

git: s/fetch/returns/ on Tag function doc

6b3f46b

This is to avoid any ambiguity with the act of "fetching" in git in general. Signed-off-by: Chris Marchesi <chrism@vancluevertech.com>

git: Add Tag objects to the list of supported objects for walking

1000bc0

This is necessary to support pruning on Tag objects. Signed-off-by: Chris Marchesi <chrism@vancluevertech.com>

git: Don't touch tag objects orphaned by tag deletion

b19b3b8

Deleting a tag ref for an annotated tag in normal git behavior does not delete the tag object right away. This is handled by the normal GC process. Signed-off-by: Chris Marchesi <chrism@vancluevertech.com>

smola reviewed Sep 7, 2018

View reviewed changes

options.go Outdated Show resolved Hide resolved

git: s/TagObjectOptions/CreateTagOptions/

f8adfff

Just renaming the TagObjectOptions type to CreateTagOptions so that it's consistent with the other option types. Signed-off-by: Chris Marchesi <chrism@vancluevertech.com>

smola approved these changes Sep 10, 2018

View reviewed changes

mcuadros added 2 commits September 10, 2018 11:33

Merge branch 'master' of github.com:src-d/go-git into f-add-tagging-s…

bf0593d

…upport

repository: fix test for new Storage constructor

9ce4eea

Signed-off-by: Máximo Cuadros <mcuadros@gmail.com>

mcuadros merged commit 38060c9 into src-d:master Sep 10, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

git: Add tagging support #928

git: Add tagging support #928

vancluever commented Aug 22, 2018 •

edited

Loading

vancluever commented Aug 22, 2018 •

edited

Loading

vancluever commented Aug 22, 2018

vancluever commented Aug 23, 2018

vancluever commented Aug 23, 2018 •

edited

Loading

smola left a comment

smola Aug 31, 2018

vancluever Sep 7, 2018

smola Aug 31, 2018

vancluever Sep 7, 2018

smola Aug 31, 2018

smola Aug 31, 2018

vancluever commented Sep 7, 2018

vancluever commented Sep 7, 2018

smola commented Sep 10, 2018

mcuadros commented Sep 10, 2018

vancluever commented Sep 10, 2018

git: Add tagging support #928

git: Add tagging support #928

Conversation

vancluever commented Aug 22, 2018 • edited Loading

vancluever commented Aug 22, 2018 • edited Loading

vancluever commented Aug 22, 2018

vancluever commented Aug 23, 2018

vancluever commented Aug 23, 2018 • edited Loading

smola left a comment

Choose a reason for hiding this comment

smola Aug 31, 2018

Choose a reason for hiding this comment

vancluever Sep 7, 2018

Choose a reason for hiding this comment

smola Aug 31, 2018

Choose a reason for hiding this comment

vancluever Sep 7, 2018

Choose a reason for hiding this comment

smola Aug 31, 2018

Choose a reason for hiding this comment

smola Aug 31, 2018

Choose a reason for hiding this comment

vancluever commented Sep 7, 2018

vancluever commented Sep 7, 2018

smola commented Sep 10, 2018

mcuadros commented Sep 10, 2018

vancluever commented Sep 10, 2018

vancluever commented Aug 22, 2018 •

edited

Loading

vancluever commented Aug 22, 2018 •

edited

Loading

vancluever commented Aug 23, 2018 •

edited

Loading