Re: [RFC/PATCH v4 2/2] gitweb: append short hash ids to snapshot files

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 13, 2009, 03:35 UTC
Message-ID: <7v7hw34ivl.fsf@alter.siamese.dyndns.org>
In-Reply-To: <4AAC2917.6000306@mailservices.uwaterloo.ca>

Mark Rada <marada@uwaterloo.ca> writes:

Show 15 quoted lines

> Teach gitweb how to produce nicer snapshot names by only using the
> short hash id. If clients make requests using a tree-ish that is not a
> partial or full SHA-1 hash, then the short hash will also be appended
> to whatever they asked for.
> ...
> +sub git_get_short_hash {
> +	my $project = shift;
> +	my $hash = shift;
> +	my $o_git_dir = $git_dir;
> +	my $retval = undef;
> +	$git_dir = "$projectroot/$project";
> +	if (open my $fd, '-|', git_cmd(), 'rev-parse', '--short', $hash) {
> +		$hash = <$fd>;
> +		close $fd;
> +		if (defined $hash && $hash =~ /^([0-9a-fA-F]{7,})$/) {

If you want to make sure it is 7 or longer, ask rev-parse to give you 7 or longer explicitly, so that you won't be hit by default changing under you in the future.

Show 8 quoted lines

> @@ -5207,6 +5227,12 @@ sub git_snapshot {
> ...
> +
> +	if ($full_hash !~ /$hash/) {
> +		$hash .= '-' . git_get_short_hash($project, $hash);
> +	} else {
> +		$hash = git_get_short_hash($project, $hash);
> +	}

I do not get this test.  What is this unanchored pattern match about?

I do not think you wanted to allow matching a partial 1234567 $hash to substitute a full 01234567..... $full_hash, so I am guessing that you meant to say "$full_hash !~ /^$hash/" at least, or perhaps you meant even "$full_hash ne $hash".

But that still does not make much sense to me. Perhaps you meant to catch a case where $hash is a tagname (or refname), i.e. $hash = 'v1.6.3' or $hash = 'next'?

If that is indeed the case, then perhaps you should check for that more explicitly, perhaps using "git show-ref $hash" or something. I do not know if the complexity (not just the "detect handcrafted $hash string that is not an SHA-1", but this whole "give shorten one" topic) is worth it, though. And if you drop the hunk that changes user supplied $hash to $full_hash in the output file name in your [PATCH 1/2], I do not think you need this anyway. If somebody asked for 'next', he will get 'next'.

If somebody asked for 01234... (full 40 hexdigits) because that was the link on the gitweb output page, it might make sense to give him a shortened name, but then the above conditional needs to be only:

	if ($full_hash eq $hash) {
        	$hash = git_get_short_hash($project, $hash);
	}

no?

Show 9 quoted lines

> +test_commit \
> +	'SnapshotFileTests' \
> +	'i can has snapshot?'
> +test_expect_success \
> +	'snapshots: give full hash' \
> +	'ID=`git rev-parse --verify HEAD` &&
> +	gitweb_run "p=.git;a=snapshot;h=$ID;sf=tgz" &&
> +	ID=`git rev-parse --short HEAD` &&
> +	grep ".git-$ID.tar.gz" gitweb.output'

I'd rather see these indented like:

        test_expect_success 'snapshots: give full hash' '
		ID=$(git rev-parse --verify HEAD) &&
		gitweb_run ...
        '

Also, if I am not mistaken, "test_commit" is not about doing any test, but is a short-hand for doing an operation, right? It would be better to have it inside test_expect_success just in case your "git commit" or some other commands are broken. I.e. like

	test_expect_success 'create a test commit' '
		test_commit SnapshotFileTests "Can I have shapshot?"
        '