gh issue artifact stack 6/12: Add download - #14569

Part of the pull request stack tracked in #14563.

This adds gh issue artifact download from #14528.

Description

Issue artifacts are Markdown documents and links attached to an issue. The previous pull requests added the gh issue artifact command group, list, view and delete. This one adds download, which writes artifacts to files. Without numbers, it downloads every artifact on the issue:

$ gh issue artifact download 142
✓ 2-OAuth-callback-plan.md
✓ 3-Staging-OAuth-runbook.url
✓ 5-Barista-feedback-notes.md

• Each file is named <number>-<name>. Documents end in .md, and links are saved as .url Internet Shortcut files. The name changes only where it would cause a problem: separators, whitespace, control characters and the characters Windows forbids each become -, on every OS.

• An existing file fails, the other artifacts are still written, and gh exits 1. --clobber overwrites the file, and --skip-existing skips it without failing. gh never writes through a symlink, even with --clobber.

• --dir chooses the directory. --output chooses the path for one artifact, and --output - prints its content with nothing else.

• --version downloads an earlier version of one artifact, named from that version's name.

• Piped, each artifact gets one line with the same five columns as delete: status, number, path, source and reason. download has no source, so that column is empty:

  $ gh issue artifact download 142 --skip-existing | cat -t
  skipped^I2^I2-OAuth-callback-plan.md^I^Ialready exists
  written^I3^I3-Staging-OAuth-runbook.url^I^I
  written^I5^I5-Barista-feedback-notes.md^I^I
  

• Like the other commands, it refuses pull requests after one lookup and GitHub Enterprise Server before any artifact request. A type gh doesn't know fails for that artifact, and the rest are still downloaded.

The shared result lines gain the skipped line. view's version lookup moves to the shared package as FindVersion, since --version follows the same rules in both commands.

How did you test this change?

I recorded the built gh against two issues in bagtoad/issue-artifacts-demo, each case in a new, empty directory and checked with ls or cat: every artifact, with the .url file's contents, selected numbers, the unusual names, --dir, an existing file failing and then --skip-existing and --clobber, --output to a file and to standard output, --version, and piped output. It leaves out symlinks, the error cases, issue URL arguments, pull requests, GitHub Enterprise Server and types gh doesn't know.

Open chapters and agent notes

Key points

• No artifact URLs yet. [Design] gh issue artifact download #14528 also names artifacts by their URLs. The API doesn't return artifact URLs yet, as discussed in RFC: gh issue artifact #14529, so download takes an issue and numbers, and its help leaves URLs out. A later pull request in this stack will add them.

• Every artifact comes from the list. The list endpoint returns each artifact's body, so downloading every artifact requests pages of 100, like list, and nothing else. With numbers, download gets each artifact in argument order, which also gives --version the edit history.

• Files are written through safepaths. write first checks the path with Lstat, which doesn't follow a symlink, so an existing file or a symlink fails with the spec's message:

  if info, err := os.Lstat(path); err == nil {
  	switch {
  	case w.skipExisting:
  		// Anything already there is skipped, a symlink included
  		return true, nil
  	case info.Mode()&fs.ModeSymlink != 0:
  		// Even with --clobber
  		return false, errors.New("is a symlink")
  	...

  Then it writes the file through the safepaths package, which never follows a symlink at the path, even one that appears after the check. openDir opens the file's directory with OpenRootDir at the first write, so a download that writes nothing creates no directory. An error from opening it is returned as it is, and only an error from creating the file with Root.Create can mean the file already exists:

  root, err := w.openDir(path)
  if err != nil {
  	// Such as "mkdir notes: file exists" when --dir is a dangling symlink
  	return false, err
  }
  f, err := root.Create(filepath.Base(path), 0o644, 0o755, w.clobber)
  if errors.Is(err, fs.ErrExist) && !w.clobber {
  	// A single name makes no directories, so only the file itself can exist
  	...

  A new file is created exclusively, so a file or symlink that appears after the check fails with the same "already exists" message, or is skipped with --skip-existing. With --clobber, an existing regular file is truncated in place. A symlink that appears after the check is replaced, unless it points to a directory or outside the directory, which safepaths refuses. It's never written through.

• File names change only what would cause a problem. artifactFileName keeps case, dots and every language, and shortens a name at a character boundary only when the file name would pass 255 bytes. The number in front keeps a name like CON from being a Windows device name. A name that already ends in .md, or .url for a link, in any case, doesn't get a second one.

• --output - prints only the body. It prints the body as it's stored, with no newline added, so a document matches the file --output <file> writes. With no result lines, its failures are ordinary errors, like view's.

• Skipped lines go to stdout in a terminal. Successes go to stdout and failures to stderr, as in delete. A skip isn't a failure, so its muted - line goes to stdout.

Notes for reviewers

Start with the download run function, then write and openDir, which hold the file safety rules. TestDownloadRun runs each row in a temporary directory and compares everything left in it, symlinks included. It has rows for names older artifacts may have, a type gh doesn't know, a link that isn't an http(s) URL, a dangling symlink as --dir or in --output's path, and changes to the path between the check and the write, such as a symlink to the checked file put in its place.

A few messages aren't in #14528. Each follows the closest existing message:

• X Failed to write 3-Staging-OAuth-runbook.url: artifact 3 doesn't link to an http(s) URL, for a stored link gh won't write as a shortcut.
• X Failed to download artifact 9: Not Found, for a failure before there's a file, like delete's Failed to delete artifact 9.
• is not a regular file, for --clobber on a directory.
• --output cannot be blank, like --worktree cannot be blank.

Related issues:

• [Design] gh issue artifact download #14528 is the download design, with a screen for each scenario.
• RFC: gh issue artifact #14529 is the RFC for the command group. Its discussion covers artifact URLs.

Authorship and follow-up

Who wrote this:

• A human wrote it.
• An agent wrote it under close human direction.
• An agent wrote it independently, and no human has guided the implementation beyond the initial prompt.

Who answers review comments:

• @BagToad will read and reply directly. Name the account.
• An agent will draft replies and @BagToad will read them before they are posted.
• Nobody has explicitly committed to replying.