[PATCH v2] git-tag: Allow --points-at syntax to create a tag pointing to specified commit

[PATCH v2] git-tag: Allow --points-at syntax to create a tag pointing to specified commit

[Michal Novotny]

This patch adds the option to specify SHA-1 commit hash using --points-at
option of git tag to create a tag pointing to a historical commit.

This was pretty easy in the past for the lightweight tags that are just simple
pointers (by creating .git/refs/tags/$tagname with SHA-1 hash) but it was not
possible for signed and annotated commits.

It's been tested for all of the tag types mentioned - lightweight tags, signed
tags and also annotated tags and everything is working fine in all scenarios
mentioned above.

Differences between v1 and v2 (this one):
 - The bogus sha1-lookup.h hunk has been removed as it's not required and
   I accidentally forgot to remove it before posting v1

Michal

Signed-off-by: Michal Novotny <minovotn@redhat.com>
---
 builtin/tag.c | 32 ++++++++++++++++++++++++++------
 1 file changed, 26 insertions(+), 6 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index f826688..f642acd 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -437,7 +437,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	struct create_tag_options opt;
 	char *cleanup_arg = NULL;
 	int annotate = 0, force = 0, lines = -1, list = 0,
-		delete = 0, verify = 0;
+		delete = 0, verify = 0, points_at_commit = 0;
 	const char *msgfile = NULL, *keyid = NULL;
 	struct msg_arg msg = { 0, STRBUF_INIT };
 	struct commit_list *with_commit = NULL;
@@ -521,8 +521,24 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 		die(_("-n option is only allowed with -l."));
 	if (with_commit)
 		die(_("--contains option is only allowed with -l."));
-	if (points_at.nr)
-		die(_("--points-at option is only allowed with -l."));
+	if (points_at.nr) {
+		if (points_at.nr > 1)
+			die(_("--points-at option is only allowed with -l or a single "
+				"SHA-1 hash is allowed to create a tag to commit."));
+		else {
+			unsigned char *ref = points_at.sha1[0];
+
+			struct object *obj = parse_object(ref);
+			if ((obj != NULL) && (obj->type == OBJ_COMMIT)) {
+				memcpy(object, ref, 20);
+				points_at_commit = 1;
+			}
+			else
+				die(_("--points-at option points to an invalid commit"));
+
+			free(ref);
+		}
+	}
 	if (delete)
 		return for_each_tag_name(argv, delete_tag);
 	if (verify)
@@ -548,12 +564,16 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 
 	tag = argv[0];
 
-	object_ref = argc == 2 ? argv[1] : "HEAD";
 	if (argc > 2)
 		die(_("too many params"));
 
-	if (get_sha1(object_ref, object))
-		die(_("Failed to resolve '%s' as a valid ref."), object_ref);
+	/* Option --points-at option is setting this already */
+	if (!points_at_commit) {
+		object_ref = argc == 2 ? argv[1] : "HEAD";
+
+		if (get_sha1(object_ref, object))
+			die(_("Failed to resolve '%s' as a valid ref."), object_ref);
+	}
 
 	if (strbuf_check_tag_ref(&ref, tag))
 		die(_("'%s' is not a valid tag name."), tag);
-- 
1.7.11.7

Re: [PATCH v2] git-tag: Allow --points-at syntax to create a tag pointing to specified commit

[John Keeping]

On Thu, Mar 14, 2013 at 01:34:54PM +0100, Michal Novotny wrote:
> This patch adds the option to specify SHA-1 commit hash using --points-at
> option of git tag to create a tag pointing to a historical commit.

What does this do that "git tag <name> <commit>" doesn't?

> This was pretty easy in the past for the lightweight tags that are just simple
> pointers (by creating .git/refs/tags/$tagname with SHA-1 hash) but it was not
> possible for signed and annotated commits.
> 
> It's been tested for all of the tag types mentioned - lightweight tags, signed
> tags and also annotated tags and everything is working fine in all scenarios
> mentioned above.
> 
> Differences between v1 and v2 (this one):
>  - The bogus sha1-lookup.h hunk has been removed as it's not required and
>    I accidentally forgot to remove it before posting v1
> 
> Michal
> 
> Signed-off-by: Michal Novotny <minovotn@redhat.com>
> ---
>  builtin/tag.c | 32 ++++++++++++++++++++++++++------
>  1 file changed, 26 insertions(+), 6 deletions(-)
> 
> diff --git a/builtin/tag.c b/builtin/tag.c
> index f826688..f642acd 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -437,7 +437,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>  	struct create_tag_options opt;
>  	char *cleanup_arg = NULL;
>  	int annotate = 0, force = 0, lines = -1, list = 0,
> -		delete = 0, verify = 0;
> +		delete = 0, verify = 0, points_at_commit = 0;
>  	const char *msgfile = NULL, *keyid = NULL;
>  	struct msg_arg msg = { 0, STRBUF_INIT };
>  	struct commit_list *with_commit = NULL;
> @@ -521,8 +521,24 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>  		die(_("-n option is only allowed with -l."));
>  	if (with_commit)
>  		die(_("--contains option is only allowed with -l."));
> -	if (points_at.nr)
> -		die(_("--points-at option is only allowed with -l."));
> +	if (points_at.nr) {
> +		if (points_at.nr > 1)
> +			die(_("--points-at option is only allowed with -l or a single "
> +				"SHA-1 hash is allowed to create a tag to commit."));
> +		else {
> +			unsigned char *ref = points_at.sha1[0];
> +
> +			struct object *obj = parse_object(ref);
> +			if ((obj != NULL) && (obj->type == OBJ_COMMIT)) {
> +				memcpy(object, ref, 20);
> +				points_at_commit = 1;
> +			}
> +			else
> +				die(_("--points-at option points to an invalid commit"));
> +
> +			free(ref);
> +		}
> +	}
>  	if (delete)
>  		return for_each_tag_name(argv, delete_tag);
>  	if (verify)
> @@ -548,12 +564,16 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>  
>  	tag = argv[0];
>  
> -	object_ref = argc == 2 ? argv[1] : "HEAD";
>  	if (argc > 2)
>  		die(_("too many params"));
>  
> -	if (get_sha1(object_ref, object))
> -		die(_("Failed to resolve '%s' as a valid ref."), object_ref);
> +	/* Option --points-at option is setting this already */
> +	if (!points_at_commit) {
> +		object_ref = argc == 2 ? argv[1] : "HEAD";
> +
> +		if (get_sha1(object_ref, object))
> +			die(_("Failed to resolve '%s' as a valid ref."), object_ref);
> +	}
>  
>  	if (strbuf_check_tag_ref(&ref, tag))
>  		die(_("'%s' is not a valid tag name."), tag);
> -- 
> 1.7.11.7

Re: [PATCH v2] git-tag: Allow --points-at syntax to create a tag pointing to specified commit

[Michal Novotny]

On 03/14/2013 02:36 PM, John Keeping wrote:
> On Thu, Mar 14, 2013 at 01:34:54PM +0100, Michal Novotny wrote:
>> This patch adds the option to specify SHA-1 commit hash using --points-at
>> option of git tag to create a tag pointing to a historical commit.
> What does this do that "git tag <name> <commit>" doesn't?

Oh, interesting. It's working now and I didn't know that as it was not
working some time ago I've been trying this approach. Maybe it's been
added recently as I also saw several sites having different approach of
tagging to specified commit (usually creating a new branch, tagging
there and rebasing etc.).

Thanks for information!
Michal

-- 
Michal Novotny <minovotn@redhat.com>, RHCE, Red Hat
Virtualization | libvirt-php bindings | php-virt-control.org

Re: [PATCH v2] git-tag: Allow --points-at syntax to create a tag pointing to specified commit

[Junio C Hamano]

Michal Novotny <minovotn@redhat.com> writes:

> Oh, interesting. It's working now and I didn't know that as it was not
> working some time ago I've been trying this approach. Maybe it's been
> added recently...

Pretty much from the very beginning "git tag <name> <commit>" has
been the way to create a tag, with missing <commit> defaulting to
HEAD.  In retrospect, it _might_ have been a more consistent UI
organization if the object to point the new tag at were given with a
command line argument like --point-at=<object>, absence of which
defaults to HEAD, but it is a bit too late for that.

By the way, your implementation is wrong and it shows that you are
not aware that a tag, either annotated or lightweight, can point at
any object, not just a commit.

> ... as I also saw several sites having different approach of
> tagging to specified commit (usually creating a new branch, tagging
> there and rebasing etc.).

There are at least two explanations that are more plausible than
that.

When rebasing an existing branch, especially if you are not familiar
with Git and want to be extra cautious, it is not unreasonable to
practice it by running the rebase on a new branch that you are
willing to discard when something goes in an unexpected way.  If the
tip of that throw-away branch happens to be where you want to tag,
it is easier to do

	git checkout -b new-branch <<some long object name>>
        git tag return-here

than

	git checkout -b new-branch <<some long object name>>
        git tag return-here <<the same long object name again>>

So it is understandable that "if untold, default to HEAD" is used in
such a workflow.  After all, we made it to default to HEAD exactly
because that is one of the most common thing to do.

Another plausible explanation is that these "sites" are written by
people who do not know what they are writing, which is not a big
news in the Internet.

It would surely be nice to get these "sites" fixed.