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

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

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

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