Skip to content

Comments

git-flow-avh: fix fish completion#97637

Closed
rajadain wants to merge 1 commit intoHomebrew:masterfrom
rajadain:git-flow-avg-fix-fish-completion
Closed

git-flow-avh: fix fish completion#97637
rajadain wants to merge 1 commit intoHomebrew:masterfrom
rajadain:git-flow-avg-fix-fish-completion

Conversation

@rajadain
Copy link
Contributor

  • Have you followed the guidelines for contributing?
  • Have you ensured that your commits follow the commit style guide?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

The previous completion would override the default git completion, resulting in error messages like:

Error: git completion not found

which came from: https://github.com/petervanderdoes/git-flow-completion/blob/db3c032411c2d6fd897b2bfc52d15a6fc4e3bfa3/git.fish#L41

By renaming the included completions to git-flow.fish, we ensure that they are scoped to the correct formula.

@SMillerDev
Copy link
Member

Has this been fixed upstream too?

@rajadain
Copy link
Contributor Author

Thanks for taking a look! Looking at https://github.com/petervanderdoes/git-flow-completion/pulls, there are pull requests going back 6 years regarding similar issues (fish completion) that haven't been merged. I haven't opened a PR there. Would you recommend I do so?

@SMillerDev
Copy link
Member

If you make a pull request we can just pull that one in as a patch. That seems like a fix that would benefit everyone

@chenrui333 chenrui333 added the CI-issue Failure due to temporary CI issue label Mar 23, 2022
@rajadain
Copy link
Contributor Author

Ah alright, I'll make one!

@rajadain
Copy link
Contributor Author

Alright I've made this PR petervanderdoes/git-flow-completion#16.

How would you recommend we "pull that one in as a patch"?

I see we can update the head install here:

url "https://github.com/petervanderdoes/git-flow-completion.git", branch: "develop"

But how can we update stable which requires a released archive?

url "https://github.com/petervanderdoes/git-flow-completion/archive/0.6.0.tar.gz"

Should I make a release off my fork?

@SMillerDev
Copy link
Member

you can use a patch do block with your commit URL and the sha checksum of the patch

@rajadain rajadain force-pushed the git-flow-avg-fix-fish-completion branch from 5456e69 to 677a791 Compare March 24, 2022 21:13
@rajadain
Copy link
Contributor Author

Thank you for your patience. I've added a reference patch in 677a791474dd615f8769a723ee72807ea9f38d61, but I still find it necessary to keep the

fish_completion.install "git.fish" => "git-flow.fish"

line.

If I set it to just git.fish, the final file is still named "git.fish" instead of the desired "git-flow.fish". If I set it to git-flow.fish, it fails with:

==> Patching completion
==> Applying 65d8580e48236ae8f086e078ed772c8891e41087.patch
patching file README.markdown
Error: An exception occurred within a child process:
  Errno::ENOENT: No such file or directory - git-flow.fish

Seemingly not picking up the file renamed in the patch.

Is this correct? Should I be patching differently?

Any help is greatly appreciated!

@rajadain
Copy link
Contributor Author

Trying 53e47eb which although doesn't work as desired on my machine, may work as intended, and may pass tests.

@rajadain
Copy link
Contributor Author

So I've tried three variants of the patch:

  1. 677a791 with git.fish => git-flow.fish, which fails this test: https://github.com/Homebrew/homebrew-core/runs/5683371578?check_suite_focus=true
  2. 53e47eb with git-flow.fish, which fails this test: https://github.com/Homebrew/homebrew-core/runs/5686515154?check_suite_focus=true
  3. a3b92f4 with git.fish, which fails this test: https://github.com/Homebrew/homebrew-core/runs/5686590227?check_suite_focus=true

Any recommendations on how to proceed?

@Bo98
Copy link
Member

Bo98 commented Mar 25, 2022

macOS /usr/bin/patch does not support file renames in git diffs, Linux does.

Given that's the only thing the patch does, you might as well drop the patch and do git.fish => git-flow.fish which should do the rename on install.

@rajadain
Copy link
Contributor Author

Very well.

I'm adding the diff here for posterity:

diff --git a/Formula/git-flow-avh.rb b/Formula/git-flow-avh.rb
index 0add433876d..b129937d0cb 100644
--- a/Formula/git-flow-avh.rb
+++ b/Formula/git-flow-avh.rb
@@ -10,6 +10,13 @@ class GitFlowAvh < Formula
     resource "completion" do
       url "https://github.com/petervanderdoes/git-flow-completion/archive/0.6.0.tar.gz"
       sha256 "b1b78b785aa2c06f81cc29fcf03a7dfc451ad482de67ca0d89cdb0f941f5594b"
+
+      # Remove once the following is merged:
+      # https://github.com/petervanderdoes/git-flow-completion/pull/16
+      patch do
+        url "https://github.com/petervanderdoes/git-flow-completion/commit/65d8580e48236ae8f086e078ed772c8891e41087.patch?full_index=1"
+        sha256 "6093489cdaa8b6c1ca396318319b47cc6b2420441c023d6588cecb1cd2299bbc"
+      end
     end
   end
 
@@ -48,7 +55,7 @@ class GitFlowAvh < Formula
     resource("completion").stage do
       bash_completion.install "git-flow-completion.bash"
       zsh_completion.install "git-flow-completion.zsh"
-      fish_completion.install "git.fish" => "git-flow.fish"
+      fish_completion.install "git-flow.fish"
     end
   end

Going to drop the commits that add the patch, and keep the rename.

@rajadain rajadain force-pushed the git-flow-avg-fix-fish-completion branch from a3b92f4 to bfe6cd2 Compare March 25, 2022 13:57
The previous completion would override the default git
completions, resulting in error messages like:

Error: git completion not found

which came from: https://github.com/petervanderdoes/git-flow-completion/blob/db3c032411c2d6fd897b2bfc52d15a6fc4e3bfa3/git.fish#L41

By renaming the included completions to git-flow.fish,
we ensure that they are scoped to the correct formula.
@rajadain rajadain force-pushed the git-flow-avg-fix-fish-completion branch from bfe6cd2 to 189bbe6 Compare March 25, 2022 13:59
@rajadain
Copy link
Contributor Author

Just rebased atop latest master

@rajadain
Copy link
Contributor Author

This is ready for a final review. Thank you all for your help in moving this forward.

@cho-m cho-m removed the CI-issue Failure due to temporary CI issue label Mar 27, 2022
@BrewTestBot
Copy link
Contributor

🤖 A scheduled task has triggered a merge.

@rajadain rajadain deleted the git-flow-avg-fix-fish-completion branch March 27, 2022 16:36
@github-actions github-actions bot added the outdated PR was locked due to age label Apr 27, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

outdated PR was locked due to age

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants