Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Git submodule command on Windows #1764

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

shivjm
Copy link

@shivjm shivjm commented Mar 23, 2022

Projectile produces an error when finding project files in a Git repository with submodules on Windows (#1600). This is because the command to find the submodules is hard-coded, including the quotes. This PR adds a projectile-get-git-sub-projects-command function to generate the command with the correct quoting at runtime. It also adds projectile-git-submodule-command-function, which is set to projectile-get-git-sub-projects-command by default, obsoleting projectile-git-submodule-command.

I’m not sure I can add tests for this since it’s specific to the combination of Git, submodules, and Windows. I did run checkdoc but it showed a whole lot of unrelated errors.


Before submitting a PR make sure the following things have been done (and denote this
by checking the relevant checkboxes):

  • The commits are consistent with our contribution guidelines
  • You've added tests (if possible) to cover your change(s)
  • All tests are passing (eldev test)
  • The new code is not generating bytecode or M-x checkdoc warnings
  • You've updated the changelog (if adding/changing user-visible functionality)
  • You've updated the readme (if adding/changing user-visible functionality)

Thanks!

@shivjm shivjm force-pushed the fix-git-submodule-command-on-windows branch from a6b7309 to 1604b99 Compare March 25, 2022 05:38
@bbatsov
Copy link
Owner

bbatsov commented Mar 25, 2022

I'm not particularly concerned about the tests here, but I'm wondering if there's a simpler way to fix this. Also we'll need to be extra clear in the function docstrings as to why this is not a simple string as most other commands. The public docs have to be updated as well to reflect the change.

projectile.el Outdated Show resolved Hide resolved
projectile.el Outdated
(_ "")))

(defun projectile-get-git-sub-projects-command ()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably this should be marked as private and also explain why the command has to be generated at runtime.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this make sense?

(defun projectile--get-git-sub-projects-command ()
  "Get the sub-projects command for Git.
Defined as a function in order to generate the properly-quoted command at
runtime."

@bbatsov
Copy link
Owner

bbatsov commented Mar 25, 2022

One thing that's not clear to me is whether this needs to be generated every time or can be generated just the first time (which would mean you can simply move the format in the original defcustom.

@shivjm
Copy link
Author

shivjm commented Mar 25, 2022

One thing that's not clear to me is whether this needs to be generated every time or can be generated just the first time (which would mean you can simply move the format in the original defcustom.

I also thought of this, and the only concern I had is that (if I’ve understood correctly) it would cause problems if the same configuration were reused across platforms. Otherwise I definitely think it would be better to just generate it once.

@bbatsov
Copy link
Owner

bbatsov commented Apr 2, 2022

I also thought of this, and the only concern I had is that (if I’ve understood correctly) it would cause problems if the same configuration were reused across platforms.

I don't quite understand what you mean by this. Can you elaborate a bit?

@shivjm
Copy link
Author

shivjm commented Apr 6, 2022

My understanding is that the custom variable will be processed once. Say you’re running Emacs on Linux. The custom variable will be set accordingly. If you then reuse that config, including the custom settings, for a Windows machine, the custom variable won’t be updated—it’ll remain in Linux form.

Assuming this is so, that might simply be an edge case we can ignore. I don’t know how important it is.

@shivjm shivjm force-pushed the fix-git-submodule-command-on-windows branch 2 times, most recently from f5e77a1 to 3721398 Compare April 8, 2022 17:16
@shivjm shivjm force-pushed the fix-git-submodule-command-on-windows branch from 3721398 to c5e2f9a Compare April 25, 2022 12:31
@shivjm shivjm force-pushed the fix-git-submodule-command-on-windows branch from c5e2f9a to c902f9f Compare June 20, 2022 14:42
@shivjm shivjm force-pushed the fix-git-submodule-command-on-windows branch from c902f9f to 95c514a Compare July 15, 2022 12:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants