fix review findings

This commit is contained in:
Konstantin Schaper
2020-12-01 15:16:55 +01:00
parent e8044747e3
commit 9ec8b4efac
17 changed files with 176 additions and 242 deletions

View File

@@ -31,6 +31,7 @@ import lombok.ToString;
import java.util.ArrayList;
import java.util.List;
import java.util.Optional;
import java.util.regex.Pattern;
/**
* Represents a tag in a repository.
@@ -43,6 +44,9 @@ import java.util.Optional;
@Getter
public final class Tag {
public static final String VALID_REV = "[0-9a-z]+";
public static final Pattern VALID_REV_PATTERN = Pattern.compile(VALID_REV);
private final String name;
private final String revision;
private final Long date;

View File

@@ -29,29 +29,52 @@ import sonia.scm.repository.spi.TagCommand;
import java.io.IOException;
public class TagCommandBuilder {
/**
* @since 2.11.0
*/
public final class TagCommandBuilder {
private final TagCommand command;
public TagCommandBuilder(TagCommand command) {
this.command = command;
}
/**
* Initialize a command to tag a revision.
*
* Set parameters and call {@link TagCreateCommandBuilder#execute()}.
*
* @since 2.11.0
*/
public TagCreateCommandBuilder create() {
return new TagCreateCommandBuilder();
}
/**
* Initialize a command to delete a tag.
*
* Set parameters and call {@link TagDeleteCommandBuilder#execute()}.
*
* @since 2.11.0
*/
public TagDeleteCommandBuilder delete() {
return new TagDeleteCommandBuilder();
}
public class TagCreateCommandBuilder {
public final class TagCreateCommandBuilder {
private final TagCreateRequest request = new TagCreateRequest();
/**
* @param revision The revision identifier for which to create the tag
*/
public TagCreateCommandBuilder setRevision(String revision) {
request.setRevision(revision);
return this;
}
/**
* @param name The name of the tag
*/
public TagCreateCommandBuilder setName(String name) {
request.setName(name);
return this;
@@ -62,9 +85,12 @@ public class TagCommandBuilder {
}
}
public class TagDeleteCommandBuilder {
public final class TagDeleteCommandBuilder {
private final TagDeleteRequest request = new TagDeleteRequest();
/**
* @param name The name of the tag that should be deleted
*/
public TagDeleteCommandBuilder setName(String name) {
request.setName(name);
return this;

View File

@@ -246,11 +246,9 @@ public abstract class RepositoryServiceProvider implements Closeable
throw new CommandNotSupportedException(Command.TAGS);
}
/**
* Method description
*
*
* @return
* @since 2.11.0
*/
public TagCommand getTagCommand()
{

View File

@@ -82,60 +82,16 @@ import static java.util.Optional.of;
public final class GitUtil {
private static final GitUserAgentProvider GIT_USER_AGENT_PROVIDER = new GitUserAgentProvider();
/**
* Field description
*/
public static final String REF_HEAD = "HEAD";
/**
* Field description
*/
public static final String REF_HEAD_PREFIX = "refs/heads/";
/**
* Field description
*/
public static final String REF_MASTER = "master";
/**
* Field description
*/
private static final String DIRECTORY_DOTGIT = ".git";
/**
* Field description
*/
private static final String DIRECTORY_OBJETCS = "objects";
/**
* Field description
*/
private static final String DIRECTORY_REFS = "refs";
/**
* Field description
*/
private static final String PREFIX_HEADS = "refs/heads/";
/**
* Field description
*/
private static final String PREFIX_TAG = "refs/tags/";
/**
* Field description
*/
private static final String REFSPEC = "+refs/heads/*:refs/remote/scm/%s/*";
/**
* Field description
*/
private static final String REMOTE_REF = "refs/remote/scm/%s/%s";
/**
* Field description
*/
private static final int TIMEOUT = 5;
/**
@@ -145,19 +101,11 @@ public final class GitUtil {
//~--- constructors ---------------------------------------------------------
/**
* Constructs ...
*/
private GitUtil() {
}
//~--- methods --------------------------------------------------------------
/**
* Method description
*
* @param repo
*/
public static void close(org.eclipse.jgit.lib.Repository repo) {
if (repo != null) {
repo.close();
@@ -186,7 +134,7 @@ public final class GitUtil {
if (c != null) {
tags.put(c.getId(), e.getKey());
} else if (logger.isWarnEnabled()) {
} else {
logger.warn("could not find commit for tag {}", e.getKey());
}
@@ -214,13 +162,6 @@ public final class GitUtil {
}
}
/**
* Method description
*
* @param directory
* @return
* @throws IOException
*/
public static org.eclipse.jgit.lib.Repository open(File directory)
throws IOException {
FS fs = FS.DETECTED;
@@ -239,33 +180,18 @@ public final class GitUtil {
return builder.build();
}
/**
* Method description
*
* @param formatter
*/
public static void release(DiffFormatter formatter) {
if (formatter != null) {
formatter.close();
}
}
/**
* Method description
*
* @param walk
*/
public static void release(TreeWalk walk) {
if (walk != null) {
walk.close();
}
}
/**
* Method description
*
* @param walk
*/
public static void release(RevWalk walk) {
if (walk != null) {
walk.close();
@@ -274,12 +200,6 @@ public final class GitUtil {
//~--- get methods ----------------------------------------------------------
/**
* Method description
*
* @param ref
* @return
*/
public static String getBranch(Ref ref) {
String branch = null;
@@ -290,12 +210,6 @@ public final class GitUtil {
return branch;
}
/**
* Method description
*
* @param name
* @return
*/
public static String getBranch(String name) {
String branch = null;

View File

@@ -25,6 +25,7 @@
package sonia.scm.repository.spi;
import com.google.common.base.Strings;
import org.apache.shiro.SecurityUtils;
import org.eclipse.jgit.api.Git;
import org.eclipse.jgit.api.errors.GitAPIException;
import org.eclipse.jgit.lib.ObjectId;
@@ -34,6 +35,7 @@ import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevObject;
import org.eclipse.jgit.revwalk.RevWalk;
import sonia.scm.NotFoundException;
import sonia.scm.event.ScmEventBus;
import sonia.scm.repository.GitUtil;
import sonia.scm.repository.InternalRepositoryException;
@@ -41,7 +43,6 @@ import sonia.scm.repository.PostReceiveRepositoryHookEvent;
import sonia.scm.repository.PreReceiveRepositoryHookEvent;
import sonia.scm.repository.RepositoryHookEvent;
import sonia.scm.repository.RepositoryHookType;
import sonia.scm.repository.Signature;
import sonia.scm.repository.Tag;
import sonia.scm.repository.api.HookContext;
import sonia.scm.repository.api.HookContextFactory;
@@ -50,6 +51,7 @@ import sonia.scm.repository.api.HookTagProvider;
import sonia.scm.repository.api.TagCreateRequest;
import sonia.scm.repository.api.TagDeleteRequest;
import sonia.scm.security.GPG;
import sonia.scm.user.User;
import javax.inject.Inject;
import java.io.IOException;
@@ -76,53 +78,72 @@ public class GitTagCommand extends AbstractGitCommand implements TagCommand {
@Override
public Tag create(TagCreateRequest request) {
try (Git git = new Git(context.open())) {
String revision = request.getRevision();
RevObject revObject;
Long tagTime;
final String name = request.getName();
final String revision = request.getRevision();
if (Strings.isNullOrEmpty(revision)) {
throw new IllegalArgumentException("Revision is required");
}
if (Strings.isNullOrEmpty(name)) {
throw new IllegalArgumentException("Name is required");
}
try (Git git = new Git(context.open())) {
RevObject revObject;
Long tagTime;
ObjectId taggedCommitObjectId = git.getRepository().resolve(revision);
if (taggedCommitObjectId == null) {
throw new NotFoundException("revision", revision);
}
try (RevWalk walk = new RevWalk(git.getRepository())) {
revObject = walk.parseAny(taggedCommitObjectId);
tagTime = GitUtil.getTagTime(walk, taggedCommitObjectId);
}
Tag tag = new Tag(request.getName(), revision, tagTime);
Tag tag = new Tag(name, revision, tagTime);
RepositoryHookEvent hookEvent = createTagHookEvent(TagHookContextProvider.createHookEvent(tag));
eventBus.post(new PreReceiveRepositoryHookEvent(hookEvent));
Ref ref =
User user = SecurityUtils.getSubject().getPrincipals().oneByType(User.class);
PersonIdent taggerIdent = new PersonIdent(user.getDisplayName(), user.getMail());
// Ref ref =
git.tag()
.setObjectId(revObject)
.setTagger(new PersonIdent("SCM-Manager", "noreply@scm-manager.org"))
.setName(request.getName())
.setTagger(taggerIdent)
.setName(name)
.call();
try (RevWalk walk = new RevWalk(git.getRepository())) {
revObject = walk.parseTag(ref.getObjectId());
final Optional<Signature> tagSignature = GitUtil.getTagSignature(revObject, gpg, walk);
tagSignature.ifPresent(tag::addSignature);
}
// Uncomment lines once jgit added support for signing tags
// try (RevWalk walk = new RevWalk(git.getRepository())) {
// revObject = walk.parseTag(ref.getObjectId());
// final Optional<Signature> tagSignature = GitUtil.getTagSignature(revObject, gpg, walk);
// tagSignature.ifPresent(tag::addSignature);
// }
eventBus.post(new PostReceiveRepositoryHookEvent(hookEvent));
return tag;
} catch (IOException | GitAPIException ex) {
throw new InternalRepositoryException(repository, "could not create tag " + request.getName(), ex);
throw new InternalRepositoryException(repository, "could not create tag " + name + " for revision " + revision, ex);
}
}
@Override
public void delete(TagDeleteRequest request) {
try (Git git = new Git(context.open())) {
String name = request.getName();
if (Strings.isNullOrEmpty(name)) {
throw new IllegalArgumentException("Name is required");
}
try (Git git = new Git(context.open())) {
final Repository repository = git.getRepository();
Optional<Ref> tagRef = findTagRef(git, name);
Tag tag;
@@ -135,7 +156,7 @@ public class GitTagCommand extends AbstractGitCommand implements TagCommand {
try (RevWalk walk = new RevWalk(repository)) {
final RevCommit commit = GitUtil.getCommit(repository, walk, tagRef.get());
Long tagTime = GitUtil.getTagTime(walk, commit.toObjectId());
Long tagTime = GitUtil.getTagTime(walk, tagRef.get().getObjectId());
tag = new Tag(name, commit.name(), tagTime);
}
@@ -144,7 +165,7 @@ public class GitTagCommand extends AbstractGitCommand implements TagCommand {
git.tagDelete().setTags(name).call();
eventBus.post(new PostReceiveRepositoryHookEvent(hookEvent));
} catch (GitAPIException | IOException e) {
throw new InternalRepositoryException(repository, "could not delete tag", e);
throw new InternalRepositoryException(repository, "could not delete tag " + name, e);
}
}
@@ -158,7 +179,7 @@ public class GitTagCommand extends AbstractGitCommand implements TagCommand {
return new RepositoryHookEvent(context, this.context.getRepository(), RepositoryHookType.PRE_RECEIVE);
}
private static class TagHookContextProvider extends HookContextProvider {
private static class TagHookContextProvider extends HookContextProvider {
private final List<Tag> newTags;
private final List<Tag> deletedTags;
@@ -177,7 +198,7 @@ public class GitTagCommand extends AbstractGitCommand implements TagCommand {
@Override
public Set<HookFeature> getSupportedFeatures() {
return singleton(HookFeature.BRANCH_PROVIDER);
return singleton(HookFeature.TAG_PROVIDER);
}
@Override
@@ -199,5 +220,5 @@ public class GitTagCommand extends AbstractGitCommand implements TagCommand {
public HookChangesetProvider getChangesetProvider() {
return r -> new HookChangesetResponse(emptyList());
}
}
}
}

View File

@@ -31,7 +31,10 @@ import com.google.common.collect.Lists;
import org.eclipse.jgit.api.Git;
import org.eclipse.jgit.api.errors.GitAPIException;
import org.eclipse.jgit.errors.IncorrectObjectTypeException;
import org.eclipse.jgit.lib.Constants;
import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevObject;
import org.eclipse.jgit.revwalk.RevTag;
import org.eclipse.jgit.revwalk.RevWalk;
@@ -76,15 +79,13 @@ public class GitTagsCommand extends AbstractGitCommand implements TagsCommand {
RevWalk revWalk = null;
try {
final Git git = new Git(open());
try (Git git = new Git(open())) {
revWalk = new RevWalk(git.getRepository());
List<Ref> tagList = git.tagList().call();
tags = Lists.transform(tagList,
new TransformFunction(git.getRepository(), revWalk, gpg, git));
new TransformFunction(git.getRepository(), revWalk, gpg));
} catch (GitAPIException ex) {
throw new InternalRepositoryException(repository, "could not read tags from repository", ex);
} finally {
@@ -114,18 +115,15 @@ public class GitTagsCommand extends AbstractGitCommand implements TagsCommand {
/**
* Constructs ...
*
* @param repository
* @param revWalk
*/
public TransformFunction(org.eclipse.jgit.lib.Repository repository,
public TransformFunction(Repository repository,
RevWalk revWalk,
GPG gpg,
Git git) {
GPG gpg) {
this.repository = repository;
this.revWalk = revWalk;
this.gpg = gpg;
this.git = git;
}
//~--- methods ------------------------------------------------------------
@@ -141,26 +139,17 @@ public class GitTagsCommand extends AbstractGitCommand implements TagsCommand {
Tag tag = null;
try {
RevObject revObject = GitUtil.getCommit(repository, revWalk, ref);
if (revObject != null) {
RevCommit revCommit = GitUtil.getCommit(repository, revWalk, ref);
if (revCommit != null) {
String name = GitUtil.getTagName(ref);
tag = new Tag(name, revObject.getId().name(), GitUtil.getTagTime(revWalk, ref.getObjectId()));
try {
RevTag revTag = GitUtil.getTag(repository, revWalk, ref);
final Optional<Signature> tagSignature = GitUtil.getTagSignature(revTag, gpg, revWalk);
if (tagSignature.isPresent()) {
tag.addSignature(tagSignature.get());
tag = new Tag(name, revCommit.getId().name(), GitUtil.getTagTime(revWalk, ref.getObjectId()));
RevObject revObject = revWalk.parseAny(ref.getObjectId());
if (revObject.getType() == Constants.OBJ_TAG) {
RevTag revTag = (RevTag) revObject;
GitUtil.getTagSignature(revTag, gpg, revWalk)
.ifPresent(tag::addSignature);
}
} catch (IncorrectObjectTypeException e) {
// Ignore because it is a lightweight tag which cannot have signatures
}
}
} catch (IOException ex) {
logger.error("could not get commit for tag", ex);
}
@@ -173,13 +162,12 @@ public class GitTagsCommand extends AbstractGitCommand implements TagsCommand {
/**
* Field description
*/
private org.eclipse.jgit.lib.Repository repository;
private final org.eclipse.jgit.lib.Repository repository;
/**
* Field description
*/
private RevWalk revWalk;
private final RevWalk revWalk;
private final GPG gpg;
private final Git git;
}
}

View File

@@ -134,8 +134,8 @@ public class GitTagCommandTest extends AbstractGitCommandTestBase {
}
private Optional<Tag> findTag(GitContext context, String name) throws IOException {
List<Tag> branches = readTags(context);
return branches.stream().filter(b -> name.equals(b.getName())).findFirst();
List<Tag> tags = readTags(context);
return tags.stream().filter(t -> name.equals(t.getName())).findFirst();
}
private HookContext createMockedContext(InvocationOnMock invocation) {

View File

@@ -27,6 +27,7 @@ package sonia.scm.repository.spi;
import com.aragost.javahg.Repository;
import com.aragost.javahg.commands.PullCommand;
import com.google.common.base.Strings;
import org.apache.shiro.SecurityUtils;
import sonia.scm.repository.InternalRepositoryException;
import sonia.scm.repository.Tag;
import sonia.scm.repository.api.TagCreateRequest;
@@ -37,6 +38,7 @@ import java.io.IOException;
public class HgTagCommand extends AbstractCommand implements TagCommand {
public static final String DEFAULT_BRANCH_NAME = "default";
private final HgWorkingCopyFactory workingCopyFactory;
public HgTagCommand(HgCommandContext context, HgWorkingCopyFactory workingCopyFactory) {
@@ -46,7 +48,7 @@ public class HgTagCommand extends AbstractCommand implements TagCommand {
@Override
public Tag create(TagCreateRequest request) {
try (WorkingCopy<Repository, Repository> workingCopy = workingCopyFactory.createWorkingCopy(getContext(), "default")) {
try (WorkingCopy<Repository, Repository> workingCopy = workingCopyFactory.createWorkingCopy(getContext(), DEFAULT_BRANCH_NAME)) {
Repository repository = getContext().open();
String rev = request.getRevision();
if (Strings.isNullOrEmpty(rev)) {
@@ -54,22 +56,22 @@ public class HgTagCommand extends AbstractCommand implements TagCommand {
}
com.aragost.javahg.commands.TagCommand.on(workingCopy.getWorkingRepository())
.rev(rev)
.user("SCM-Manager")
.user(SecurityUtils.getSubject().getPrincipal().toString())
.execute(request.getName());
pullChangesIntoCentralRepository(workingCopy, "default");
pullChangesIntoCentralRepository(workingCopy, DEFAULT_BRANCH_NAME);
return new Tag(request.getName(), rev);
}
}
@Override
public void delete(TagDeleteRequest request) {
try (WorkingCopy<Repository, Repository> workingCopy = workingCopyFactory.createWorkingCopy(getContext(), "default")) {
try (WorkingCopy<Repository, Repository> workingCopy = workingCopyFactory.createWorkingCopy(getContext(), DEFAULT_BRANCH_NAME)) {
com.aragost.javahg.commands.TagCommand.on(workingCopy.getWorkingRepository())
.user("SCM-Manager")
.user(SecurityUtils.getSubject().getPrincipal().toString())
.remove()
.execute(request.getName());
pullChangesIntoCentralRepository(workingCopy, "default");
pullChangesIntoCentralRepository(workingCopy, DEFAULT_BRANCH_NAME);
}
}

View File

@@ -24,34 +24,15 @@
package sonia.scm.repository.spi;
import com.aragost.javahg.commands.PullCommand;
import org.junit.Before;
import org.junit.Test;
import sonia.scm.repository.Tag;
import sonia.scm.repository.api.TagCommandBuilder;
import sonia.scm.repository.work.NoneCachingWorkingCopyPool;
import sonia.scm.repository.work.WorkdirProvider;
import java.io.IOException;
import java.util.List;
import static org.assertj.core.api.Assertions.assertThat;
public class HgTagsCommandTest extends AbstractHgCommandTestBase {
private SimpleHgWorkingCopyFactory workingCopyFactory;
@Before
public void initWorkingCopyFactory() {
workingCopyFactory = new SimpleHgWorkingCopyFactory(new NoneCachingWorkingCopyPool(new WorkdirProvider())) {
@Override
public void configure(PullCommand pullCommand) {
// we do not want to configure http hooks in this unit test
}
};
}
@Test
public void shouldGetTagDatesCorrectly() {
HgTagsCommand hgTagsCommand = new HgTagsCommand(cmdContext);
@@ -61,15 +42,4 @@ public class HgTagsCommandTest extends AbstractHgCommandTestBase {
assertThat(tags.get(0).getDate()).contains(1339586381000L);
}
@Test
public void shouldNotDie() throws IOException {
HgTagCommand hgTagCommand = new HgTagCommand(cmdContext, workingCopyFactory);
new TagCommandBuilder(hgTagCommand).create().setRevision("79b6baf49711").setName("newtag").execute();
HgTagsCommand hgTagsCommand = new HgTagsCommand(cmdContext);
final List<Tag> tags = hgTagsCommand.getTags();
assertThat(tags).isNotEmpty();
}
}

View File

@@ -22,11 +22,11 @@
* SOFTWARE.
*/
import React, {FC, useEffect, useState} from "react";
import React, { FC, useEffect, useState } from "react";
import { Modal, InputField, Button, apiClient } from "@scm-manager/ui-components";
import { WithTranslation, withTranslation } from "react-i18next";
import {Tag} from "@scm-manager/ui-types";
import {isBranchValid} from "../validation";
import { Tag } from "@scm-manager/ui-types";
import { isBranchValid } from "../validation";
type Props = WithTranslation & {
existingTagsLink: string;
@@ -91,7 +91,12 @@ const CreateTagModal: FC<Props> = ({ t, onClose, tagCreationLink, existingTagsLi
footer={
<>
<Button action={onClose}>{t("tags.create.cancel")}</Button>
<Button color="success" action={() => createTag()} loading={loading} disabled={!!validationError || newTagName.length === 0}>
<Button
color="success"
action={() => createTag()}
loading={loading}
disabled={!!validationError || newTagName.length === 0}
>
{t("tags.create.confirm")}
</Button>
</>

View File

@@ -23,7 +23,7 @@
*/
import { Links } from "./hal";
import {Signature} from "./Signature";
import { Signature } from "./Signature";
export type Tag = {
name: string;

View File

@@ -86,7 +86,7 @@
"delete": {
"button": "Delete branch",
"subtitle": "Delete branch",
"description": "Deleted branches can not be restored.",
"description": "Deleted branches cannot be restored.",
"confirmAlert": {
"title": "Delete branch",
"message": "Do you really want to delete the branch \"{{branch}}\"?",

View File

@@ -26,7 +26,7 @@ import { Trans, useTranslation, WithTranslation, withTranslation } from "react-i
import classNames from "classnames";
import styled from "styled-components";
import { ExtensionPoint } from "@scm-manager/ui-extensions";
import {Changeset, Link, ParentChangeset, Repository, Tag} from "@scm-manager/ui-types";
import { Changeset, Link, ParentChangeset, Repository, Tag } from "@scm-manager/ui-types";
import {
AvatarImage,
AvatarWrapper,

View File

@@ -33,6 +33,7 @@ type Props = {
tag: Tag;
baseUrl: string;
onDelete: (tag: Tag) => void;
// deleting: boolean;
};
const Created = styled.span`

View File

@@ -22,11 +22,11 @@
* SOFTWARE.
*/
import React, {FC, useState} from "react";
import {Link, Tag} from "@scm-manager/ui-types";
import React, { FC, useState } from "react";
import { Link, Tag } from "@scm-manager/ui-types";
import { useTranslation } from "react-i18next";
import TagRow from "./TagRow";
import {apiClient, ConfirmAlert, ErrorNotification} from "@scm-manager/ui-components";
import { apiClient, ConfirmAlert, ErrorNotification } from "@scm-manager/ui-components";
type Props = {
baseUrl: string;
@@ -98,7 +98,8 @@ const TagTable: FC<Props> = ({ baseUrl, tags, fetchTags }) => {
</thead>
<tbody>{renderRow()}</tbody>
</table>
</>);
</>
);
};
export default TagTable;

View File

@@ -60,7 +60,7 @@ const TagsOverview: FC<Props> = ({ repository, baseUrl }) => {
const renderTagsTable = () => {
if (!loading && tags?.length > 0) {
orderTags(tags);
return <TagTable baseUrl={baseUrl} tags={tags} fetchTags={() => fetchTags()} />;
return <TagTable baseUrl={baseUrl} tags={tags} fetchTags={fetchTags} />;
}
return <Notification type="info">{t("tags.overview.noTags")}</Notification>;
};

View File

@@ -29,16 +29,20 @@ import lombok.Setter;
import org.hibernate.validator.constraints.Length;
import javax.validation.constraints.NotEmpty;
import javax.validation.constraints.Pattern;
import static sonia.scm.repository.Branch.VALID_BRANCH_NAMES;
import static sonia.scm.repository.Tag.VALID_REV;
@Getter
@Setter
public class TagRequestDto {
// TODO: Validate revision via regex
@Pattern(regexp = VALID_REV)
@NotEmpty
@Length(min = 1, max = 100)
private String revision;
// TODO: Validate name via regex
@Pattern(regexp = VALID_BRANCH_NAMES)
@NotEmpty
@Length(min = 1, max = 100)
private String name;