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

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

[Mark Rada]

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.

This also includes tests cases for t9502-gitweb-standalone-parse-output.

Signed-off-by: Mark Rada <marada@uwaterloo.ca>
---
 gitweb/gitweb.perl                        |   26 +++++++++++
 t/t9502-gitweb-standalone-parse-output.sh |   67 +++++++++++++++++++++++++++++
 2 files changed, 93 insertions(+), 0 deletions(-)
 create mode 100644 t/t9502-gitweb-standalone-parse-output.sh

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index e1beca5..fdecc3d 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2006,6 +2006,26 @@ sub git_get_full_hash {
 	return $retval;
 }
 
+# try and get a shorter hash id
+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,})$/) {
+			$retval = $1;
+		}
+	}
+	if (defined $o_git_dir) {
+		$git_dir = $o_git_dir;
+	}
+	return $retval;
+}
+
 # get type of given object
 sub git_get_type {
 	my $hash = shift;
@@ -5207,6 +5227,12 @@ sub git_snapshot {
 		die_error(404, 'Hash id was not valid');
 	}
 
+
+	if ($full_hash !~ /$hash/) {
+		$hash .= '-' . git_get_short_hash($project, $hash);
+	} else {
+		$hash = git_get_short_hash($project, $hash);
+	}
 	my $name = $project;
 	$name =~ s,([^/])/*\.git$,$1,;
 	$name = basename($name);
diff --git a/t/t9502-gitweb-standalone-parse-output.sh b/t/t9502-gitweb-standalone-parse-output.sh
new file mode 100644
index 0000000..1a2a27f
--- /dev/null
+++ b/t/t9502-gitweb-standalone-parse-output.sh
@@ -0,0 +1,67 @@
+#!/bin/sh
+#
+# Copyright (c) 2009 Mark Rada
+#
+
+test_description='gitweb as standalone script (parsing script output).
+
+This test runs gitweb (git web interface) as a CGI script from the
+commandline, and checks that it produces the correct output, either
+in the HTTP header or the actual script output.'
+
+
+. ./gitweb-lib.sh
+
+# ----------------------------------------------------------------------
+# snapshot file name
+
+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'
+test_debug 'cat gitweb.output'
+
+test_expect_success \
+	'snapshots: give short hash' \
+	'ID=`git rev-parse --short HEAD` &&
+	gitweb_run "p=.git;a=snapshot;h=$ID;sf=tgz" &&
+	grep ".git-$ID.tar.gz" gitweb.output'
+test_debug 'cat gitweb.output'
+
+test_expect_success \
+	'snapshots: give almost full hash' \
+	'ID=`git rev-parse --short=30 HEAD` &&
+	gitweb_run "p=.git;a=snapshot;h=$ID;sf=tgz" &&
+	ID=`git rev-parse --short HEAD` &&
+	grep ".git-$ID.tar.gz" gitweb.output'
+test_debug 'cat gitweb.output'
+
+test_expect_success \
+	'snapshots: give HEAD tree-ish' \
+	'gitweb_run "p=.git;a=snapshot;h=HEAD;sf=tgz" &&
+	ID=`git rev-parse --short HEAD` &&
+	grep ".git-HEAD-$ID.tar.gz" gitweb.output'
+test_debug 'cat gitweb.output'
+
+test_expect_success \
+	'snapshots: give branch name tree-ish' \
+	'gitweb_run "p=.git;a=snapshot;h=master;sf=tgz" &&
+	ID=`git rev-parse --short master` &&
+	grep ".git-master-$ID.tar.gz" gitweb.output'
+test_debug 'cat gitweb.output'
+
+test_expect_success \
+	'snapshots: give tag tree-ish' \
+	'gitweb_run "p=.git;a=snapshot;h=SnapshotFileTests;sf=tgz" &&
+	ID=`git rev-parse --short SnapshotFileTests` &&
+	grep ".git-SnapshotFileTests-$ID.tar.gz" gitweb.output'
+test_debug 'cat gitweb.output'
+
+
+test_done
-- 
1.6.4.2

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

[Junio C Hamano]

Mark Rada <marada@uwaterloo.ca> writes:

> 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.

> @@ -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?        

> +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?"
        '

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

[Mark Rada]

On 09-09-12 11:35 PM, Junio C Hamano wrote:
>> @@ -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 missed that, it should have been anchored.

> 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?

This was a manifestation of a suggestion from Jakub:
> Second, I'd rather have better names for snapshots than using full SHA-1.
> For snapshot of 'v1.5.0' of repository 'repo.git' I'd prefer for snapshot
> to be named 'repo-v1.5.0', and for snapshot of 'next' branch of the same
> project to be named for example 'repo-next-20090909', or perhaps
> 'repo-next-2009-09-10T09:16:18' or 'repo-next-20090909-g5f6b0ff',
> or 'repo-v1.6.5-rc0-164-g5f6b0ff'.
> 
> I'm not sure what would be the best name of snapshot of given 
> subdirectory...
> 
> 
> In short: I'd rather not improve on bad design of using full SHA-1
> in snapshot name.

For me, there are two fates that snapshots will end up with: being deleted
as soon as I have unrolled the contents, or long term archiving. For the
latter case, it is nice to have an idea of when it came from, though I
guess I should have appended a date in that case... ¯\(°_o)/¯

Thoughts?


>> +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?"
>         '

Can I have snapshot?!?! What do you have against LOLspeak? :P


-- 
Mark Rada (ferrous26)
marada@uwaterloo.ca

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

[Junio C Hamano]

Mark Rada <marada@uwaterloo.ca> writes:

> This was a manifestation of a suggestion from Jakub:
>
>> Second, I'd rather have better names for snapshots than using full SHA-1.
>> For snapshot of 'v1.5.0' of repository 'repo.git' I'd prefer for snapshot
>> to be named 'repo-v1.5.0', and for snapshot of 'next' branch of the same

I think I've already said "Don't use $full_hash but if the user gave you
descriptive e.g. v1.5.0 in $hash just use it", which I think matches what
Jakub said above.

>> project to be named for example 'repo-next-20090909', or perhaps
>> 'repo-next-2009-09-10T09:16:18' or 'repo-next-20090909-g5f6b0ff',
>> or 'repo-v1.6.5-rc0-164-g5f6b0ff'.

I do not particularly care about these, except that if the user asked for
'next', that string should be in the name somewhere, so the last one is
unnacceptable to me.  I'd rather vote for naming it just 'repo-next', as
if I were writing a robot that goes once-a-day to next and download, I
would likely to be doing it like this:

	D=`date +'%Y-%m-%d'` && mkdir "$D" && cd "$D" || exit
        wget ...snapshot-url-for-next-branch...
	wget ...snapshot-url-for-some-other-branch...
	wget ...snapshot-url-for-even-some-other-repository...
	...

and I do not want any frills other than what I _asked_ gitweb to give me,
which is "this repository, this branch".

But that is just my personal preference.  Treat it as no heavier than a
feature request from a random list participant.  It does not carry any
more weight than that merely because it comes from me.

> For me, there are two fates that snapshots will end up with: being deleted
> as soon as I have unrolled the contents, or long term archiving. For the
> latter case, it is nice to have an idea of when it came from, though I
> guess I should have appended a date in that case... ¯\(°_o)/¯

What date do you mean?  The commit date?  Or download date?

As long as it is clear which revision the snapshot came from, I do not
think anything fancier is necessary.

Besides, don't the paths in the archive have the timestamp of the commit
object?

If you are talking about download date for archival use, I think the
timestamp of the archive file itself is sufficient, and the person who is
downloading can (re)name the result in whatever way he wants.