Merge pull request #9796 from desktop/delete-tags

Add delete tags functionality

app/src/lib/git/tag.ts

@@ -21,6 +21,21 @@ export async function createTag(
   await git(args, repository.path, 'createTag')
 }
 
+/**
+ * Delete a tag.
+ *
+ * @param repository        - The repository in which to create the new tag.
+ * @param name              - The name of the tag to delete.
+ */
+export async function deleteTag(
+  repository: Repository,
+  name: string
+): Promise<void> {
+  const args = ['tag', '-d', name]
+
+  await git(args, repository.path, 'deleteTag')
+}
+
 /**
  * Gets all the local tags. Returns a Map with the tag name and the commit it points to.
  *

app/src/lib/menu-item.ts

@@ -18,6 +18,11 @@ export interface IMenuItem {
    * See https://electronjs.org/docs/api/menu-item#roles
    */
   readonly role?: Electron.MenuItemConstructorOptions['role']
+
+  /**
+   * Submenu that will appear when hovering this menu item.
+   */
+  readonly submenu?: ReadonlyArray<IMenuItem>
 }
 
 /**

app/src/lib/stats/stats-database.ts

@@ -356,6 +356,11 @@ export interface IDailyMeasures {
    * How many tags have been created in total.
    */
   readonly tagsCreated: number
+
+  /**
+   * How many tags have been deleted.
+   */
+  readonly tagsDeleted: number
 }
 
 export class StatsDatabase extends Dexie {

app/src/lib/stats/stats-store.ts

@@ -138,6 +138,7 @@ const DefaultDailyMeasures: IDailyMeasures = {
   issueCreationWebpageOpenedCount: 0,
   tagsCreatedInDesktop: 0,
   tagsCreated: 0,
+  tagsDeleted: 0,
 }
 
 interface IOnboardingStats {
@@ -1369,6 +1370,12 @@ export class StatsStore implements IStatsStore {
     }))
   }
 
+  public recordTagDeleted() {
+    return this.updateDailyMeasures(m => ({
+      tagsDeleted: m.tagsDeleted + 1,
+    }))
+  }
+
   /** Post some data to our stats endpoint. */
   private post(body: object): Promise<Response> {
     const options: RequestInit = {

app/src/lib/stores/app-store.ts

@@ -3066,6 +3066,13 @@ export class AppStore extends TypedBaseStore<IAppState> {
     this._closePopup()
   }
 
+  /** This shouldn't be called directly. See `Dispatcher`. */
+  public async _deleteTag(repository: Repository, name: string): Promise<void> {
+    const gitStore = this.gitStoreCache.get(repository)
+
+    await gitStore.deleteTag(name)
+  }
+
   private updateCheckoutProgress(
     repository: Repository,
     checkoutProgress: ICheckoutProgress | null

app/src/lib/stores/git-store.ts

@@ -65,6 +65,7 @@ import {
   removeRemote,
   createTag,
   getAllTags,
+  deleteTag,
 } from '../git'
 import { GitError as DugiteError } from '../../lib/git'
 import { GitError } from 'dugite'
@@ -342,6 +343,22 @@ export class GitStore extends BaseStore {
     this.addTagToPush(name)
   }
 
+  public async deleteTag(name: string) {
+    const result = await this.performFailableOperation(async () => {
+      await deleteTag(this.repository, name)
+      return true
+    })
+
+    if (result === undefined) {
+      return
+    }
+
+    await this.refreshTags()
+    this.removeTagToPush(name)
+
+    this.statsStore.recordTagDeleted()
+  }
+
   /** The list of ordered SHAs. */
   public get history(): ReadonlyArray<string> {
     return this._history
@@ -454,6 +471,15 @@ export class GitStore extends BaseStore {
     this.emitUpdate()
   }
 
+  private removeTagToPush(tagToDelete: string) {
+    this._tagsToPush = this._tagsToPush.filter(
+      tagName => tagName !== tagToDelete
+    )
+
+    storeTagsToPush(this.repository, this._tagsToPush)
+    this.emitUpdate()
+  }
+
   public clearTagsToPush() {
     this._tagsToPush = []
 

app/src/main-process/main.ts

@@ -438,8 +438,8 @@ app.on('ready', () => {
   ipcMain.on(
     'show-contextual-menu',
     (event: Electron.IpcMainEvent, items: ReadonlyArray<IMenuItem>) => {
-      const menu = buildContextMenu(items, ix =>
-        event.sender.send('contextual-menu-action', ix)
+      const menu = buildContextMenu(items, indices =>
+        event.sender.send('contextual-menu-action', indices)
       )
 
       const window = BrowserWindow.fromWebContents(event.sender)

app/src/main-process/menu/build-context-menu.ts

@@ -39,44 +39,49 @@ function getEditMenuItems(): ReadonlyArray<MenuItem> {
  *                 the renderer.
  * @param onClick  A callback function for when one of the menu items
  *                 constructed from the template is clicked. Callback
- *                 is passed the index of the menu item in the template
- *                 as the first argument and the template item itself
- *                 as the second argument. Note that the callback will
- *                 not be called when expanded/automatically created
- *                 edit menu items are clicked.
+ *                 is passed an array of indices corresponding to the
+ *                 positions of each of the parent menus of the clicked
+ *                 item (so when clicking a top-level menu item an array
+ *                 with a single element will be passed). Note that the
+ *                 callback will not be called when expanded/automatically
+ *                 created edit menu items are clicked.
  */
 export function buildContextMenu(
   template: ReadonlyArray<IMenuItem>,
-  onClick: (ix: number, item: IMenuItem) => void
+  onClick: (indices: ReadonlyArray<number>) => void
 ): Menu {
-  const menuItems = new Array<MenuItem>()
+  return buildRecursiveContextMenu(template, onClick)
+}
 
-  for (const [ix, item] of template.entries()) {
-    // Special case editMenu in context menus. What we
-    // mean by this is that we want to insert all edit
-    // related menu items into the menu at this spot, we
-    // don't want a sub menu
+function buildRecursiveContextMenu(
+  menuItems: ReadonlyArray<IMenuItem>,
+  actionFn: (indices: ReadonlyArray<number>) => void,
+  currentIndices: ReadonlyArray<number> = []
+): Menu {
+  const menu = new Menu()
+
+  for (const [idx, item] of menuItems.entries()) {
     if (roleEquals(item.role, 'editmenu')) {
-      menuItems.push(...getEditMenuItems())
+      for (const editItem of getEditMenuItems()) {
+        menu.append(editItem)
+      }
     } else {
-      // TODO: We're always overriding the click function here.
-      // It's possible that we might want to add a role-based
-      // menu item without a custom click function at some point
-      // in the future.
-      menuItems.push(
+      const indices = [...currentIndices, idx]
+
+      menu.append(
         new MenuItem({
           label: item.label,
           type: item.type,
           enabled: item.enabled,
           role: item.role,
-          click: () => onClick(ix, item),
+          click: () => actionFn(indices),
+          submenu: item.submenu
+            ? buildRecursiveContextMenu(item.submenu, actionFn, indices)
+            : undefined,
         })
       )
     }
   }
 
-  const menu = new Menu()
-  menuItems.forEach(x => menu.append(x))
-
   return menu
 }

app/src/models/popup.ts

@@ -59,6 +59,7 @@ export enum PopupType {
   CreateFork,
   SChannelNoRevocationCheck,
   CreateTag,
+  DeleteTag,
   LocalChangesOverwritten,
   RebaseConflicts,
   RetryClone,
@@ -244,3 +245,8 @@ export type Popup =
       retryAction: RetryAction
       errorMessage: string
     }
+  | {
+      type: PopupType.DeleteTag
+      repository: Repository
+      tagName: string
+    }

app/src/ui/app.tsx

@@ -116,6 +116,7 @@ import { findUpstreamRemoteBranch } from '../lib/branch'
 import { GitHubRepository } from '../models/github-repository'
 import { CreateTag } from './create-tag'
 import { RetryCloneDialog } from './clone-repository/retry-clone-dialog'
+import { DeleteTag } from './delete-tag'
 
 const MinuteInMilliseconds = 1000 * 60
 const HourInMilliseconds = MinuteInMilliseconds * 60
@@ -1946,6 +1947,17 @@ export class App extends React.Component<IAppProps, IAppState> {
           />
         )
       }
+      case PopupType.DeleteTag: {
+        return (
+          <DeleteTag
+            key="delete-tag"
+            repository={popup.repository}
+            onDismissed={this.onPopupDismissed}
+            dispatcher={this.props.dispatcher}
+            tagName={popup.tagName}
+          />
+        )
+      }
       case PopupType.RetryClone: {
         return (
           <RetryCloneDialog

app/src/ui/delete-tag/delete-tag-dialog.tsx

@@ -0,0 +1,64 @@
+import * as React from 'react'
+
+import { Dispatcher } from '../dispatcher'
+import { Repository } from '../../models/repository'
+import { Dialog, DialogContent, DialogFooter } from '../dialog'
+import { Ref } from '../lib/ref'
+import { OkCancelButtonGroup } from '../dialog/ok-cancel-button-group'
+
+interface IDeleteTagProps {
+  readonly dispatcher: Dispatcher
+  readonly repository: Repository
+  readonly tagName: string
+  readonly onDismissed: () => void
+}
+
+interface IDeleteTagState {
+  readonly isDeleting: boolean
+}
+
+export class DeleteTag extends React.Component<
+  IDeleteTagProps,
+  IDeleteTagState
+> {
+  public constructor(props: IDeleteTagProps) {
+    super(props)
+
+    this.state = {
+      isDeleting: false,
+    }
+  }
+
+  public render() {
+    return (
+      <Dialog
+        id="delete-tag"
+        title={__DARWIN__ ? 'Delete Tag' : 'Delete tag'}
+        type="warning"
+        onSubmit={this.DeleteTag}
+        onDismissed={this.props.onDismissed}
+        disabled={this.state.isDeleting}
+        loading={this.state.isDeleting}
+      >
+        <DialogContent>
+          <p>
+            Are you sure you want to delete the tag{' '}
+            <Ref>{this.props.tagName}</Ref>?
+          </p>
+        </DialogContent>
+        <DialogFooter>
+          <OkCancelButtonGroup destructive={true} okButtonText="Delete" />
+        </DialogFooter>
+      </Dialog>
+    )
+  }
+
+  private DeleteTag = async () => {
+    const { dispatcher, repository, tagName } = this.props
+
+    this.setState({ isDeleting: true })
+
+    await dispatcher.deleteTag(repository, tagName)
+    this.props.onDismissed()
+  }
+}

app/src/ui/delete-tag/index.ts

@@ -0,0 +1 @@
+export { DeleteTag } from './delete-tag-dialog'

app/src/ui/dispatcher/dispatcher.ts

@@ -506,6 +506,13 @@ export class Dispatcher {
     return this.appStore._createTag(repository, name, targetCommitSha)
   }
 
+  /**
+   * Deletes the passed tag.
+   */
+  public deleteTag(repository: Repository, name: string): Promise<void> {
+    return this.appStore._deleteTag(repository, name)
+  }
+
   /**
    * Show the tag creation dialog.
    */
@@ -524,6 +531,20 @@ export class Dispatcher {
     })
   }
 
+  /**
+   * Show the confirmation dialog to delete a tag.
+   */
+  public showDeleteTagDialog(
+    repository: Repository,
+    tagName: string
+  ): Promise<void> {
+    return this.showPopup({
+      type: PopupType.DeleteTag,
+      repository,
+      tagName,
+    })
+  }
+
   /** Check out the given branch. */
   public checkoutBranch(
     repository: Repository,

app/src/ui/history/commit-list-item.tsx

@@ -25,9 +25,11 @@ interface ICommitProps {
   readonly onRevertCommit?: (commit: Commit) => void
   readonly onViewCommitOnGitHub?: (sha: string) => void
   readonly onCreateTag?: (targetCommitSha: string) => void
+  readonly onDeleteTag?: (tagName: string) => void
   readonly gitHubUsers: Map<string, IGitHubUser> | null
   readonly showUnpushedIndicator: boolean
   readonly unpushedIndicatorTitle?: string
+  readonly unpushedTags?: ReadonlyArray<string>
 }
 
 interface ICommitListItemState {
@@ -175,6 +177,17 @@ export class CommitListItem extends React.PureComponent<
         action: this.onCreateTag,
         enabled: this.props.onCreateTag !== undefined,
       })
+
+      const deleteTagsMenuItem = this.getDeleteTagsMenuItem()
+
+      if (deleteTagsMenuItem !== null) {
+        items.push(
+          {
+            type: 'separator',
+          },
+          deleteTagsMenuItem
+        )
+      }
     }
 
     items.push(
@@ -192,6 +205,42 @@ export class CommitListItem extends React.PureComponent<
 
     showContextualMenu(items)
   }
+
+  private getDeleteTagsMenuItem(): IMenuItem | null {
+    const { unpushedTags, onDeleteTag, commit } = this.props
+
+    if (
+      onDeleteTag === undefined ||
+      unpushedTags === undefined ||
+      commit.tags.length === 0
+    ) {
+      return null
+    }
+
+    if (commit.tags.length === 1) {
+      const tagName = commit.tags[0]
+
+      return {
+        label: `Delete tag ${tagName}`,
+        action: () => onDeleteTag(tagName),
+        enabled: unpushedTags.includes(tagName),
+      }
+    }
+
+    // Convert tags to a Set to avoid O(n^2)
+    const unpushedTagsSet = new Set(unpushedTags)
+
+    return {
+      label: 'Delete tag…',
+      submenu: commit.tags.map(tagName => {
+        return {
+          label: tagName,
+          action: () => onDeleteTag(tagName),
+          enabled: unpushedTagsSet.has(tagName),
+        }
+      }),
+    }
+  }
 }
 
 function renderRelativeTime(date: Date) {

app/src/ui/history/commit-list.tsx

@@ -49,6 +49,9 @@ interface ICommitListProps {
   /** Callback to fire to open the dialog to create a new tag on the given commit */
   readonly onCreateTag: (targetCommitSha: string) => void
 
+  /** Callback to fire to delete an unpushed tag */
+  readonly onDeleteTag: (tagName: string) => void
+
   /**
    * Optional callback that fires on page scroll in order to allow passing
    * a new scrollTop value up to the parent component for storing.
@@ -97,12 +100,13 @@ export class CommitList extends React.Component<ICommitListProps, {}> {
     const tagsToPushSet = new Set(this.props.tagsToPush || [])
 
     const isLocal = this.props.localCommitSHAs.includes(commit.sha)
-    const numUnpushedTags = commit.tags.filter(tagName =>
+    const unpushedTags = commit.tags.filter(tagName =>
       tagsToPushSet.has(tagName)
-    ).length
+    )
 
     const showUnpushedIndicator =
-      (isLocal || numUnpushedTags > 0) && this.props.isLocalRepository === false
+      (isLocal || unpushedTags.length > 0) &&
+      this.props.isLocalRepository === false
 
     return (
       <CommitListItem
@@ -112,12 +116,14 @@ export class CommitList extends React.Component<ICommitListProps, {}> {
         showUnpushedIndicator={showUnpushedIndicator}
         unpushedIndicatorTitle={this.getUnpushedIndicatorTitle(
           isLocal,
-          numUnpushedTags
+          unpushedTags.length
         )}
+        unpushedTags={unpushedTags}
         commit={commit}
         gitHubUsers={this.props.gitHubUsers}
         emoji={this.props.emoji}
         onCreateTag={this.props.onCreateTag}
+        onDeleteTag={this.props.onDeleteTag}
         onRevertCommit={this.props.onRevertCommit}
         onViewCommitOnGitHub={this.props.onViewCommitOnGitHub}
       />

app/src/ui/history/compare.tsx

@@ -286,6 +286,7 @@ export class CompareSidebar extends React.Component<
         onCommitSelected={this.onCommitSelected}
         onScroll={this.onScroll}
         onCreateTag={this.onCreateTag}
+        onDeleteTag={this.onDeleteTag}
         emptyListMessage={emptyListMessage}
         onCompareListScrolled={this.props.onCompareListScrolled}
         compareListScrollTop={this.props.compareListScrollTop}
@@ -601,6 +602,10 @@ export class CompareSidebar extends React.Component<
       this.props.localTags
     )
   }
+
+  private onDeleteTag = (tagName: string) => {
+    this.props.dispatcher.showDeleteTagDialog(this.props.repository, tagName)
+  }
 }
 
 function getPlaceholderText(state: ICompareState) {

app/src/ui/main-process-proxy.ts

@@ -74,24 +74,41 @@ let currentContextualMenuItems: ReadonlyArray<IMenuItem> | null = null
 export function registerContextualMenuActionDispatcher() {
   ipcRenderer.on(
     'contextual-menu-action',
-    (event: Electron.IpcRendererEvent, index: number) => {
-      if (!currentContextualMenuItems) {
-        return
-      }
-      if (index >= currentContextualMenuItems.length) {
+    (event: Electron.IpcRendererEvent, indices: number[]) => {
+      if (currentContextualMenuItems === null) {
         return
       }
 
-      const item = currentContextualMenuItems[index]
-      const action = item.action
-      if (action) {
-        action()
+      const menuItem = findSubmenuItem(currentContextualMenuItems, indices)
+
+      if (menuItem !== undefined && menuItem.action !== undefined) {
+        menuItem.action()
         currentContextualMenuItems = null
       }
     }
   )
 }
 
+function findSubmenuItem(
+  currentContextualMenuItems: ReadonlyArray<IMenuItem>,
+  indices: ReadonlyArray<number>
+): IMenuItem | undefined {
+  let foundMenuItem: IMenuItem | undefined = {
+    submenu: currentContextualMenuItems,
+  }
+
+  // Traverse the submenus of the context menu until we find the appropiate index.
+  for (const index of indices) {
+    if (foundMenuItem === undefined || foundMenuItem.submenu === undefined) {
+      return undefined
+    }
+
+    foundMenuItem = foundMenuItem.submenu[index]
+  }
+
+  return foundMenuItem
+}
+
 /** Show the given menu items in a contextual menu. */
 export function showContextualMenu(items: ReadonlyArray<IMenuItem>) {
   currentContextualMenuItems = items

app/test/unit/git/tag-test.ts

@@ -12,6 +12,7 @@ import {
   createBranch,
   createCommit,
   checkoutBranch,
+  deleteTag,
 } from '../../../src/lib/git'
 import {
   setupFixtureRepository,
@@ -81,6 +82,17 @@ describe('git/tag', () => {
     })
   })
 
+  describe('deleteTag', () => {
+    it('deletes a tag with the given name', async () => {
+      await createTag(repository, 'my-new-tag', 'HEAD')
+      await deleteTag(repository, 'my-new-tag')
+
+      const commit = await getCommit(repository, 'HEAD')
+      expect(commit).not.toBeNull()
+      expect(commit!.tags).toEqual([])
+    })
+  })
+
   describe('getAllTags', () => {
     it('returns an empty array when the repository has no tags', async () => {
       expect(await getAllTags(repository)).toEqual(new Map())