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