Re: [PATCH] Add option -L to git-tag.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Matthijs Melchior <mmelchior@xxxxxxxxx> writes:

>   This will list the selected tags and include annotations, if any.
>
> Signed-off-by: Matthijs Melchior <mmelchior@xxxxxxxxx>
> ---
>
> This patch has been created to allow me to easily see the annotations with tags.
> I have not found any other way to do this...

Hmmmm.  This feature ought to belong to git-show, but that
command already has its own interpretation of how a tag should
be shown.

Do we care about the "one line summary"?  Perhaps...

	$ git tag --pretty=short -L v2.6.11-tree
	v2.6.11-tree
            This is the 2.6.11 tree object.

	$ git tag -L v2.6.11-tree
	v2.6.11-tree
            This is the 2.6.11 tree object.

	    NOTE! There's no commit for this, since it happened...

The answer to this question would really depend on why we would
want to have this feature.  "git-tag" started as a way to
"create", and then it gained -l to "list" and -v to "verify".  I
think your -L is meant to be an extension to "list", but I
suspect that while you are listing many things you would keep
the annotation short-and-sweet, and you would want to view the
full description when your interest is focused at a single one.
Maybe we would want a separate "print" action for the full
contents and use the short one for "list but more verbosely than
usual"?

I dunno; I've never been very good at the user interfaces.

>  - Sorting the tag names resulting from git-rev-parse is not nessecary since
>    the list of tags is already deliverd in sorted order.

This I am a bit reluctant about, as that sorting done by
rev-parse is purely by accident (i.e. it is an implementation
detail).

>  - Using git-cat-file -t on every tag is expensive, but there is no alternative

This is Ok, as we have somebody working on doing git-tag as a
built-in, and once that happens, we do not have to pay the
performance penalty.  So I would think at this point we should
concentrate on discussing the usefulness of this new feature and
correctness of your implementation, as that would set the course
for the future.  On the other hand, "cat-file -t" performance
issues will not stay with us forever.

> @@ -26,13 +26,23 @@ do
>      -f)
>  	force=1
>  	;;
> -    -l)
> -	case "$#" in
> -	1)
> -		set x . ;;
> -	esac
> +    -l|-L)
> +	TAGSONLY=true
> +	[ "$1" = -L ] && TAGSONLY=false
> +	[ "$#" = 1 ] && set x .
>  	shift
> +	git rev-parse --symbolic --tags | grep "$@" |
> +	    while read TAG
> +	    do
> +		echo "$TAG"
> +		$TAGSONLY && continue
> +		OBJTYPE=$(git cat-file -t "$TAG")
> +		case $OBJTYPE in
> +		    tag)    git cat-file $OBJTYPE "$TAG" |
> +				sed '1,/^$/d;/^-----BEGIN PGP SIGNATURE-----$/Q;s/^/    /'
> +			    ;;
> +		esac

Micronit.  If you already know it is a tag, you do not have to
say "cat-file $OBJTYPE".

Style.  Please indent the case arm to the same level as, not
deeper than, case/esac.
This is the same as C's switch() { case ...: } indentation rule.

Please do not feed multiple expressions concatenated with
semicolon to sed, as it is one of the often observed portability
issues (not all the world is GNU yet).  Write it like this
instead:

        case "$OBJTYPE" in
        tag)
                git cat-file tag "$TAG" |
                sed -e '1,/^$/d' \
                    -e '/^-----BEGIN PGP SIGNATURE-----$/Q' \
                    -e s/^/    /'
                ;;
        esac

> +	    done
>  	exit $?

What does this command exit with now?  It used to be that

	$ git tag -l no-such-tag-at-all ; echo $?

said "1", I think, because grep did not match.

-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux