From c2188a840bc4153ae92112b04b2e06a90d3944aa Mon Sep 17 00:00:00 2001 From: Paul Martin Date: Wed, 27 Apr 2016 23:58:06 +0100 Subject: Ticket Reference handling #1048 + Supports referencing: + Tickets from other tickets via comments + Tickets from commits on any branch + Common TicketLink class used for both commits and tickets + TicketLink is temporary and persisted to ticket as a Reference + Support deletion of ticket references + Rebasing patchsets/branches will generate new references + Deleting old patchsets/branches will remove the relevant references + Substantial testing of use cases + With and without patchsets, deleting, amending + BranchTicketService used during testing to allow end-to-end ref testing + Relocated common git helper functions to JGitUtils --- .../java/com/gitblit/git/GitblitReceivePack.java | 243 +++++++++++++++ .../java/com/gitblit/git/PatchsetReceivePack.java | 333 +++++++++++---------- 2 files changed, 412 insertions(+), 164 deletions(-) (limited to 'src/main/java/com/gitblit/git') diff --git a/src/main/java/com/gitblit/git/GitblitReceivePack.java b/src/main/java/com/gitblit/git/GitblitReceivePack.java index 34bbea27..f271f6f1 100644 --- a/src/main/java/com/gitblit/git/GitblitReceivePack.java +++ b/src/main/java/com/gitblit/git/GitblitReceivePack.java @@ -22,18 +22,28 @@ import groovy.util.GroovyScriptEngine; import java.io.File; import java.io.IOException; import java.text.MessageFormat; +import java.util.ArrayList; import java.util.Collection; +import java.util.LinkedHashMap; import java.util.LinkedHashSet; import java.util.List; +import java.util.Map; import java.util.Set; +import java.util.SortedMap; +import java.util.TreeMap; import java.util.concurrent.TimeUnit; +import org.eclipse.jgit.lib.AnyObjectId; import org.eclipse.jgit.lib.BatchRefUpdate; import org.eclipse.jgit.lib.NullProgressMonitor; +import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.PersonIdent; import org.eclipse.jgit.lib.ProgressMonitor; +import org.eclipse.jgit.lib.Ref; +import org.eclipse.jgit.lib.RefUpdate; import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.revwalk.RevCommit; +import org.eclipse.jgit.revwalk.RevWalk; import org.eclipse.jgit.transport.PostReceiveHook; import org.eclipse.jgit.transport.PreReceiveHook; import org.eclipse.jgit.transport.ReceiveCommand; @@ -50,14 +60,24 @@ import com.gitblit.client.Translation; import com.gitblit.extensions.ReceiveHook; import com.gitblit.manager.IGitblit; import com.gitblit.models.RepositoryModel; +import com.gitblit.models.TicketModel; import com.gitblit.models.UserModel; +import com.gitblit.models.TicketModel.Change; +import com.gitblit.models.TicketModel.Field; +import com.gitblit.models.TicketModel.Patchset; +import com.gitblit.models.TicketModel.Status; +import com.gitblit.models.TicketModel.TicketAction; +import com.gitblit.models.TicketModel.TicketLink; import com.gitblit.tickets.BranchTicketService; +import com.gitblit.tickets.ITicketService; +import com.gitblit.tickets.TicketNotifier; import com.gitblit.utils.ArrayUtils; import com.gitblit.utils.ClientLogger; import com.gitblit.utils.CommitCache; import com.gitblit.utils.JGitUtils; import com.gitblit.utils.RefLogUtils; import com.gitblit.utils.StringUtils; +import com.google.common.collect.Lists; /** @@ -92,6 +112,11 @@ public class GitblitReceivePack extends ReceivePack implements PreReceiveHook, P protected final IStoredSettings settings; protected final IGitblit gitblit; + + protected final ITicketService ticketService; + + protected final TicketNotifier ticketNotifier; + public GitblitReceivePack( IGitblit gitblit, @@ -114,6 +139,14 @@ public class GitblitReceivePack extends ReceivePack implements PreReceiveHook, P } catch (IOException e) { } + if (gitblit.getTicketService().isAcceptingTicketUpdates(repository)) { + this.ticketService = gitblit.getTicketService(); + this.ticketNotifier = this.ticketService.createNotifier(); + } else { + this.ticketService = null; + this.ticketNotifier = null; + } + // set advanced ref permissions setAllowCreates(user.canCreateRef(repository)); setAllowDeletes(user.canDeleteRef(repository)); @@ -500,6 +533,104 @@ public class GitblitReceivePack extends ReceivePack implements PreReceiveHook, P } } } + + // + // if there are ref update receive commands that were + // successfully processed and there is an active ticket service for the repository + // then process any referenced tickets + // + if (ticketService != null) { + List allUpdates = ReceiveCommand.filter(batch.getCommands(), Result.OK); + if (!allUpdates.isEmpty()) { + int ticketsProcessed = 0; + for (ReceiveCommand cmd : allUpdates) { + switch (cmd.getType()) { + case CREATE: + case UPDATE: + if (cmd.getRefName().startsWith(Constants.R_HEADS)) { + Collection tickets = processReferencedTickets(cmd); + ticketsProcessed += tickets.size(); + for (TicketModel ticket : tickets) { + ticketNotifier.queueMailing(ticket); + } + } + break; + + case UPDATE_NONFASTFORWARD: + if (cmd.getRefName().startsWith(Constants.R_HEADS)) { + String base = JGitUtils.getMergeBase(getRepository(), cmd.getOldId(), cmd.getNewId()); + List deletedRefs = JGitUtils.identifyTicketsBetweenCommits(getRepository(), settings, base, cmd.getOldId().name()); + for (TicketLink link : deletedRefs) { + link.isDelete = true; + } + Change deletion = new Change(user.username); + deletion.pendingLinks = deletedRefs; + ticketService.updateTicket(repository, 0, deletion); + + Collection tickets = processReferencedTickets(cmd); + ticketsProcessed += tickets.size(); + for (TicketModel ticket : tickets) { + ticketNotifier.queueMailing(ticket); + } + } + break; + case DELETE: + //Identify if the branch has been merged + SortedMap bases = new TreeMap(); + try { + ObjectId dObj = cmd.getOldId(); + Collection tips = getRepository().getRefDatabase().getRefs(Constants.R_HEADS).values(); + for (Ref ref : tips) { + ObjectId iObj = ref.getObjectId(); + String mergeBase = JGitUtils.getMergeBase(getRepository(), dObj, iObj); + if (mergeBase != null) { + int d = JGitUtils.countCommits(getRepository(), getRevWalk(), mergeBase, dObj.name()); + bases.put(d, mergeBase); + //All commits have been merged into some other branch + if (d == 0) { + break; + } + } + } + + if (bases.isEmpty()) { + //TODO: Handle orphan branch case + } else { + if (bases.firstKey() > 0) { + //Delete references from the remaining commits that haven't been merged + String mergeBase = bases.get(bases.firstKey()); + List deletedRefs = JGitUtils.identifyTicketsBetweenCommits(getRepository(), + settings, mergeBase, dObj.name()); + + for (TicketLink link : deletedRefs) { + link.isDelete = true; + } + Change deletion = new Change(user.username); + deletion.pendingLinks = deletedRefs; + ticketService.updateTicket(repository, 0, deletion); + } + } + + } catch (IOException e) { + LOGGER.error(null, e); + } + break; + + default: + break; + } + } + + if (ticketsProcessed == 1) { + sendInfo("1 ticket updated"); + } else if (ticketsProcessed > 1) { + sendInfo("{0} tickets updated", ticketsProcessed); + } + } + + // reset the ticket caches for the repository + ticketService.resetCaches(repository); + } } protected void setGitblitUrl(String url) { @@ -616,4 +747,116 @@ public class GitblitReceivePack extends ReceivePack implements PreReceiveHook, P public UserModel getUserModel() { return user; } + + /** + * Automatically closes open tickets and adds references to tickets if made in the commit message. + * + * @param cmd + */ + private Collection processReferencedTickets(ReceiveCommand cmd) { + Map changedTickets = new LinkedHashMap(); + + final RevWalk rw = getRevWalk(); + try { + rw.reset(); + rw.markStart(rw.parseCommit(cmd.getNewId())); + if (!ObjectId.zeroId().equals(cmd.getOldId())) { + rw.markUninteresting(rw.parseCommit(cmd.getOldId())); + } + + RevCommit c; + while ((c = rw.next()) != null) { + rw.parseBody(c); + List ticketLinks = JGitUtils.identifyTicketsFromCommitMessage(getRepository(), settings, c); + if (ticketLinks == null) { + continue; + } + + for (TicketLink link : ticketLinks) { + + TicketModel ticket = ticketService.getTicket(repository, link.targetTicketId); + if (ticket == null) { + continue; + } + + Change change = null; + String commitSha = c.getName(); + String branchName = Repository.shortenRefName(cmd.getRefName()); + + switch (link.action) { + case Commit: { + //A commit can reference a ticket in any branch even if the ticket is closed. + //This allows developers to identify and communicate related issues + change = new Change(user.username); + change.referenceCommit(commitSha); + } break; + + case Close: { + // As this isn't a patchset theres no merging taking place when closing a ticket + if (ticket.isClosed()) { + continue; + } + + change = new Change(user.username); + change.setField(Field.status, Status.Fixed); + + if (StringUtils.isEmpty(ticket.responsible)) { + // unassigned tickets are assigned to the closer + change.setField(Field.responsible, user.username); + } + } + + default: { + //No action + } break; + } + + if (change != null) { + ticket = ticketService.updateTicket(repository, ticket.number, change); + } + + if (ticket != null) { + sendInfo(""); + sendHeader("#{0,number,0}: {1}", ticket.number, StringUtils.trimString(ticket.title, Constants.LEN_SHORTLOG)); + + switch (link.action) { + case Commit: { + sendInfo("referenced by push of {0} to {1}", commitSha, branchName); + changedTickets.put(ticket.number, ticket); + } break; + + case Close: { + sendInfo("closed by push of {0} to {1}", commitSha, branchName); + changedTickets.put(ticket.number, ticket); + } break; + + default: { } + } + + sendInfo(ticketService.getTicketUrl(ticket)); + sendInfo(""); + } else { + switch (link.action) { + case Commit: { + sendError("FAILED to reference ticket {0} by push of {1}", link.targetTicketId, commitSha); + } break; + + case Close: { + sendError("FAILED to close ticket {0} by push of {1}", link.targetTicketId, commitSha); + } break; + + default: { } + } + } + } + } + + } catch (IOException e) { + LOGGER.error("Can't scan for changes to reference or close", e); + } finally { + rw.reset(); + } + + return changedTickets.values(); + } } diff --git a/src/main/java/com/gitblit/git/PatchsetReceivePack.java b/src/main/java/com/gitblit/git/PatchsetReceivePack.java index ef0b409b..33fa4705 100644 --- a/src/main/java/com/gitblit/git/PatchsetReceivePack.java +++ b/src/main/java/com/gitblit/git/PatchsetReceivePack.java @@ -30,7 +30,6 @@ import java.util.concurrent.TimeUnit; import java.util.regex.Matcher; import java.util.regex.Pattern; -import org.eclipse.jgit.lib.AnyObjectId; import org.eclipse.jgit.lib.BatchRefUpdate; import org.eclipse.jgit.lib.NullProgressMonitor; import org.eclipse.jgit.lib.ObjectId; @@ -60,6 +59,8 @@ import com.gitblit.models.TicketModel.Field; import com.gitblit.models.TicketModel.Patchset; import com.gitblit.models.TicketModel.PatchsetType; import com.gitblit.models.TicketModel.Status; +import com.gitblit.models.TicketModel.TicketAction; +import com.gitblit.models.TicketModel.TicketLink; import com.gitblit.models.UserModel; import com.gitblit.tickets.BranchTicketService; import com.gitblit.tickets.ITicketService; @@ -485,9 +486,27 @@ public class PatchsetReceivePack extends GitblitReceivePack { switch (cmd.getType()) { case CREATE: case UPDATE: + if (cmd.getRefName().startsWith(Constants.R_HEADS)) { + Collection tickets = processReferencedTickets(cmd); + ticketsProcessed += tickets.size(); + for (TicketModel ticket : tickets) { + ticketNotifier.queueMailing(ticket); + } + } + break; + case UPDATE_NONFASTFORWARD: if (cmd.getRefName().startsWith(Constants.R_HEADS)) { - Collection tickets = processMergedTickets(cmd); + String base = JGitUtils.getMergeBase(getRepository(), cmd.getOldId(), cmd.getNewId()); + List deletedRefs = JGitUtils.identifyTicketsBetweenCommits(getRepository(), settings, base, cmd.getOldId().name()); + for (TicketLink link : deletedRefs) { + link.isDelete = true; + } + Change deletion = new Change(user.username); + deletion.pendingLinks = deletedRefs; + ticketService.updateTicket(repository, 0, deletion); + + Collection tickets = processReferencedTickets(cmd); ticketsProcessed += tickets.size(); for (TicketModel ticket : tickets) { ticketNotifier.queueMailing(ticket); @@ -604,15 +623,17 @@ public class PatchsetReceivePack extends GitblitReceivePack { return null; } } - + // check to see if this commit is already linked to a ticket - long id = identifyTicket(tipCommit, false); - if (id > 0) { - sendError("{0} has already been pushed to ticket {1,number,0}.", shortTipId, id); + if (ticket != null && + JGitUtils.getTicketNumberFromCommitBranch(getRepository(), tipCommit) == ticket.number) { + sendError("{0} has already been pushed to ticket {1,number,0}.", shortTipId, ticket.number); sendRejection(cmd, "everything up-to-date"); return null; } - + + List ticketLinks = JGitUtils.identifyTicketsFromCommitMessage(getRepository(), settings, tipCommit); + PatchsetCommand psCmd; if (ticket == null) { /* @@ -802,6 +823,10 @@ public class PatchsetReceivePack extends GitblitReceivePack { } break; } + + Change change = psCmd.getChange(); + change.pendingLinks = ticketLinks; + return psCmd; } @@ -890,11 +915,11 @@ public class PatchsetReceivePack extends GitblitReceivePack { /** * Automatically closes open tickets that have been merged to their integration - * branch by a client. + * branch by a client and adds references to tickets if made in the commit message. * * @param cmd */ - private Collection processMergedTickets(ReceiveCommand cmd) { + private Collection processReferencedTickets(ReceiveCommand cmd) { Map mergedTickets = new LinkedHashMap(); final RevWalk rw = getRevWalk(); try { @@ -907,105 +932,151 @@ public class PatchsetReceivePack extends GitblitReceivePack { RevCommit c; while ((c = rw.next()) != null) { rw.parseBody(c); - long ticketNumber = identifyTicket(c, true); - if (ticketNumber == 0L || mergedTickets.containsKey(ticketNumber)) { - continue; - } - - TicketModel ticket = ticketService.getTicket(repository, ticketNumber); - if (ticket == null) { - continue; - } - String integrationBranch; - if (StringUtils.isEmpty(ticket.mergeTo)) { - // unspecified integration branch - integrationBranch = null; - } else { - // specified integration branch - integrationBranch = Constants.R_HEADS + ticket.mergeTo; - } - - // ticket must be open and, if specified, the ref must match the integration branch - if (ticket.isClosed() || (integrationBranch != null && !integrationBranch.equals(cmd.getRefName()))) { + List ticketLinks = JGitUtils.identifyTicketsFromCommitMessage(getRepository(), settings, c); + if (ticketLinks == null) { continue; } - String baseRef = PatchsetCommand.getBasePatchsetBranch(ticket.number); - boolean knownPatchset = false; - Set refs = getRepository().getAllRefsByPeeledObjectId().get(c.getId()); - if (refs != null) { - for (Ref ref : refs) { - if (ref.getName().startsWith(baseRef)) { - knownPatchset = true; - break; - } - } - } - - String mergeSha = c.getName(); - String mergeTo = Repository.shortenRefName(cmd.getRefName()); - Change change; - Patchset patchset; - if (knownPatchset) { - // identify merged patchset by the patchset tip - patchset = null; - for (Patchset ps : ticket.getPatchsets()) { - if (ps.tip.equals(mergeSha)) { - patchset = ps; - break; - } + for (TicketLink link : ticketLinks) { + + if (mergedTickets.containsKey(link.targetTicketId)) { + continue; } - - if (patchset == null) { - // should not happen - unless ticket has been hacked - sendError("Failed to find the patchset for {0} in ticket {1,number,0}?!", - mergeSha, ticket.number); + + TicketModel ticket = ticketService.getTicket(repository, link.targetTicketId); + if (ticket == null) { continue; } + String integrationBranch; + if (StringUtils.isEmpty(ticket.mergeTo)) { + // unspecified integration branch + integrationBranch = null; + } else { + // specified integration branch + integrationBranch = Constants.R_HEADS + ticket.mergeTo; + } + + Change change; + Patchset patchset = null; + String mergeSha = c.getName(); + String mergeTo = Repository.shortenRefName(cmd.getRefName()); + + if (link.action == TicketAction.Commit) { + //A commit can reference a ticket in any branch even if the ticket is closed. + //This allows developers to identify and communicate related issues + change = new Change(user.username); + change.referenceCommit(mergeSha); + } else { + // ticket must be open and, if specified, the ref must match the integration branch + if (ticket.isClosed() || (integrationBranch != null && !integrationBranch.equals(cmd.getRefName()))) { + continue; + } + + String baseRef = PatchsetCommand.getBasePatchsetBranch(ticket.number); + boolean knownPatchset = false; + Set refs = getRepository().getAllRefsByPeeledObjectId().get(c.getId()); + if (refs != null) { + for (Ref ref : refs) { + if (ref.getName().startsWith(baseRef)) { + knownPatchset = true; + break; + } + } + } + + if (knownPatchset) { + // identify merged patchset by the patchset tip + for (Patchset ps : ticket.getPatchsets()) { + if (ps.tip.equals(mergeSha)) { + patchset = ps; + break; + } + } + + if (patchset == null) { + // should not happen - unless ticket has been hacked + sendError("Failed to find the patchset for {0} in ticket {1,number,0}?!", + mergeSha, ticket.number); + continue; + } + + // create a new change + change = new Change(user.username); + } else { + // new patchset pushed by user + String base = cmd.getOldId().getName(); + patchset = newPatchset(ticket, base, mergeSha); + PatchsetCommand psCmd = new PatchsetCommand(user.username, patchset); + psCmd.updateTicket(c, mergeTo, ticket, null); + + // create a ticket patchset ref + updateRef(psCmd.getPatchsetBranch(), c.getId(), patchset.type); + RefUpdate ru = updateRef(psCmd.getTicketBranch(), c.getId(), patchset.type); + updateReflog(ru); + + // create a change from the patchset command + change = psCmd.getChange(); + } + + // set the common change data about the merge + change.setField(Field.status, Status.Merged); + change.setField(Field.mergeSha, mergeSha); + change.setField(Field.mergeTo, mergeTo); + + if (StringUtils.isEmpty(ticket.responsible)) { + // unassigned tickets are assigned to the closer + change.setField(Field.responsible, user.username); + } + } + + ticket = ticketService.updateTicket(repository, ticket.number, change); + + if (ticket != null) { + sendInfo(""); + sendHeader("#{0,number,0}: {1}", ticket.number, StringUtils.trimString(ticket.title, Constants.LEN_SHORTLOG)); + + switch (link.action) { + case Commit: { + sendInfo("referenced by push of {0} to {1}", c.getName(), mergeTo); + } + break; - // create a new change - change = new Change(user.username); - } else { - // new patchset pushed by user - String base = cmd.getOldId().getName(); - patchset = newPatchset(ticket, base, mergeSha); - PatchsetCommand psCmd = new PatchsetCommand(user.username, patchset); - psCmd.updateTicket(c, mergeTo, ticket, null); - - // create a ticket patchset ref - updateRef(psCmd.getPatchsetBranch(), c.getId(), patchset.type); - RefUpdate ru = updateRef(psCmd.getTicketBranch(), c.getId(), patchset.type); - updateReflog(ru); - - // create a change from the patchset command - change = psCmd.getChange(); - } + case Close: { + sendInfo("closed by push of {0} to {1}", patchset, mergeTo); + mergedTickets.put(ticket.number, ticket); + } + break; - // set the common change data about the merge - change.setField(Field.status, Status.Merged); - change.setField(Field.mergeSha, mergeSha); - change.setField(Field.mergeTo, mergeTo); + default: { + + } + } - if (StringUtils.isEmpty(ticket.responsible)) { - // unassigned tickets are assigned to the closer - change.setField(Field.responsible, user.username); - } + sendInfo(ticketService.getTicketUrl(ticket)); + sendInfo(""); - ticket = ticketService.updateTicket(repository, ticket.number, change); - if (ticket != null) { - sendInfo(""); - sendHeader("#{0,number,0}: {1}", ticket.number, StringUtils.trimString(ticket.title, Constants.LEN_SHORTLOG)); - sendInfo("closed by push of {0} to {1}", patchset, mergeTo); - sendInfo(ticketService.getTicketUrl(ticket)); - sendInfo(""); - mergedTickets.put(ticket.number, ticket); - } else { - String shortid = mergeSha.substring(0, settings.getInteger(Keys.web.shortCommitIdLength, 6)); - sendError("FAILED to close ticket {0,number,0} by push of {1}", ticketNumber, shortid); + } else { + String shortid = mergeSha.substring(0, settings.getInteger(Keys.web.shortCommitIdLength, 6)); + + switch (link.action) { + case Commit: { + sendError("FAILED to reference ticket {0,number,0} by push of {1}", link.targetTicketId, shortid); + } + break; + case Close: { + sendError("FAILED to close ticket {0,number,0} by push of {1}", link.targetTicketId, shortid); + } break; + + default: { + + } + } + } } } + } catch (IOException e) { - LOGGER.error("Can't scan for changes to close", e); + LOGGER.error("Can't scan for changes to reference or close", e); } finally { rw.reset(); } @@ -1013,75 +1084,9 @@ public class PatchsetReceivePack extends GitblitReceivePack { return mergedTickets.values(); } - /** - * Try to identify a ticket id from the commit. - * - * @param commit - * @param parseMessage - * @return a ticket id or 0 - */ - private long identifyTicket(RevCommit commit, boolean parseMessage) { - // try lookup by change ref - Map> map = getRepository().getAllRefsByPeeledObjectId(); - Set refs = map.get(commit.getId()); - if (!ArrayUtils.isEmpty(refs)) { - for (Ref ref : refs) { - long number = PatchsetCommand.getTicketNumber(ref.getName()); - if (number > 0) { - return number; - } - } - } - - if (parseMessage) { - // parse commit message looking for fixes/closes #n - String dx = "(?:fixes|closes)[\\s-]+#?(\\d+)"; - String x = settings.getString(Keys.tickets.closeOnPushCommitMessageRegex, dx); - if (StringUtils.isEmpty(x)) { - x = dx; - } - try { - Pattern p = Pattern.compile(x, Pattern.CASE_INSENSITIVE); - Matcher m = p.matcher(commit.getFullMessage()); - while (m.find()) { - String val = m.group(1); - return Long.parseLong(val); - } - } catch (Exception e) { - LOGGER.error(String.format("Failed to parse \"%s\" in commit %s", x, commit.getName()), e); - } - } - return 0L; - } + - private int countCommits(String baseId, String tipId) { - int count = 0; - RevWalk walk = getRevWalk(); - walk.reset(); - walk.sort(RevSort.TOPO); - walk.sort(RevSort.REVERSE, true); - try { - RevCommit tip = walk.parseCommit(getRepository().resolve(tipId)); - RevCommit base = walk.parseCommit(getRepository().resolve(baseId)); - walk.markStart(tip); - walk.markUninteresting(base); - for (;;) { - RevCommit c = walk.next(); - if (c == null) { - break; - } - count++; - } - } catch (IOException e) { - // Should never happen, the core receive process would have - // identified the missing object earlier before we got control. - LOGGER.error("failed to get commit count", e); - return 0; - } finally { - walk.close(); - } - return count; - } + /** * Creates a new patchset with metadata. @@ -1091,7 +1096,7 @@ public class PatchsetReceivePack extends GitblitReceivePack { * @param tip */ private Patchset newPatchset(TicketModel ticket, String mergeBase, String tip) { - int totalCommits = countCommits(mergeBase, tip); + int totalCommits = JGitUtils.countCommits(getRepository(), getRevWalk(), mergeBase, tip); Patchset newPatchset = new Patchset(); newPatchset.tip = tip; -- cgit v1.2.3