Mark Rada <marada@xxxxxxxxxxxx> 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?"
'
--
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