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 clone on Windows #868

Merged
merged 1 commit into from Jan 19, 2023
Merged

Conversation

adamlamar
Copy link

@adamlamar adamlamar commented Jan 11, 2023

This changes wtf? to wtf#. The ? is not allowed in filenames or directories on Windows, but # is. If I understand #775 correctly, the # should be a reasonably good substitute since it has the same requirements as ?.

The corner case zimfile was also updated accordingly and the test should pass.

Successful clone on Windows:

$ git clone https://github.com/adamlamar/libkiwix.git
Cloning into 'libkiwix'...
remote: Enumerating objects: 12725, done.
remote: Counting objects: 100% (210/210), done.
remote: Compressing objects: 100% (97/97), done.
remote: Total 12725 (delta 127), reused 168 (delta 111), pack-reused 12515
Receiving objects: 100% (12725/12725), 6.99 MiB | 11.50 MiB/s, done.
Resolving deltas: 100% (8415/8415), done.

Fixes #867

@veloman-yunkan
Copy link
Collaborator

@adamlamar Thank you for the fix!

I have only one very superficial objection. Question mark plays well with wtf (and that is why some mental effort was spent on picking up a good and short question-like meme when creating a new unit test for #866). Anyone looking at the new filenames may experience some cognitive dissonance. They might think:

- WTF? Why "wtf#"? Shouldn't it be "wtf?"? 😄

They will definitely have hard time figuring out the reason behind this counterintuitive usage of the hash symbol.

There are two solutions:

  1. Replace the entire "wtf?" string with another one containing a hash symbol (e.g. C#). For consistency I would also update the content of the HTML file.
  2. Assuming that the create_corner_cases_zim_file is not supposed to be executed on Windows, the "wtf?" files can be generated from inside that script.

And, BTW, nothing prevents us from implementing both options - then both "#" and "?" will be tested!

@mgautierfr What do you think? Should we just merge this PR or it makes sense to pursue consistency that perhaps no one will care about?

@adamlamar
Copy link
Author

Hey @veloman-yunkan, I agree it would be better if the test case made more sense in semantic terms. wtf# is a bit weird :) I was just laser focused on fixing this one issue since its blocking the kiwix-desktop windows build. I like the C# suggestion, pushed a change using C#.

@adamlamar
Copy link
Author

Oh and I agree that we could write a test case for all of the characters that need encoding (?, #, & if I understand correctly), but we'd have to avoid committing the file with ?. I suppose we could have the script generate the necessary files, create the zim file, and then delete them (without committing to git).

@@ -0,0 +1 @@
c#.html
Copy link
Collaborator

Choose a reason for hiding this comment

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

This comment applies to the name of the file.

Holding on to my previous concern wrt semantic consistency, I have to note that C is not C#. Maybe rename this file to c_sharp.html?

Copy link
Author

Choose a reason for hiding this comment

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

Should be fixed now 👍

Copy link
Collaborator

@veloman-yunkan veloman-yunkan left a comment

Choose a reason for hiding this comment

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

Almost good to merge.

However, I don't think that we need all those three commits (each with their own 36KiB version of corner_cases.zim) in the history. Please squash all commits into one with a commit message explaining why the change is more extensive than it could have been.

@adamlamar
Copy link
Author

Sure, I squashed and pushed.

The question mark (?) is not a valid filename character on Windows.
Changing to a the pound sign (#) so that this repository can still be
cloned on Windows.
Copy link
Collaborator

@veloman-yunkan veloman-yunkan left a comment

Choose a reason for hiding this comment

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

Thanks for bearing with me and my whims!

@kelson42 kelson42 merged commit b9937e6 into kiwix:main Jan 19, 2023
@adamlamar adamlamar deleted the windows-git-clone branch January 30, 2023 22:00
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.

Cannot git clone this repository on Windows
3 participants