tombreit / mkdocs-git-latest-changes-plugin

MkDocs plugin that allows you to display a list of recently modified pages from the Git log
https://tombreit.github.io/mkdocs-git-latest-changes-plugin/
MIT License
1 stars 2 forks source link

Improve `git log` parsing #5

Closed jtru closed 8 months ago

jtru commented 8 months ago

Before this patch, double quotes (") in commit message subjects break the pseudo-JSON the "--pretty" option's format string tries to wrestle from git log.

With this patch, git log is made to emit a newline-delimited representation of each commit, which is then parsed and (weakly) validated, and ends up in a dict without intermittent JSONification.

Further hardening against (accidental) injections via git log output might be a good idea still.

tombreit commented 8 months ago

Hi, many thanks, you are totally right.

I really should be creating tests for this project by now at the latest. If you have the time and inclination, you are welcome to support me (via pytest). Apart from that, I'll just start with the tests and then take care of your pull request.

jtru commented 8 months ago

Thanks for considering adopting this PR/suggestion! I stumbled over this problem when my colleague's attempt to make use of this neat little plugin blew up in his face due to my quoting-related habits in commit message subjects ;) I'd be happy to be able to eventually contribute test cases, too.

tombreit commented 8 months ago

Dear @jtru ,

I have just quickly changed the parsing of the git log strings (without your PR for now) and written a few (hopefully suitable) tests.

Would be great if you can/want to test this new version. It's not on PyPi, but you should be able to install it directly from Github:

$ python -m pip install 'mkdocs-git-latest-changes-plugin @ git+https://github.com/tombreit/mkdocs-git-latest-changes-plugin'
jtru commented 8 months ago

Ah yes, having NUL delimit fields should be the optimal approach; I like it :)

Unfortunately, as of 9b0fb6c, the plugin seems to have broken our mkdocs build (we self-host our infra, including gitlab and the venv this is building on during testing, internally on hosts with their FQDNs set to some.localdomain.example.com ) due to some part handling the SUPPORTED_REMOTE_REPOS dict less carefully than before:

[...]
DEBUG   -  mkdocs_git_latest_changes_plugin: Found latest_changes marker in latest.md
WARNING -  mkdocs_git_latest_changes_plugin: Repository config.repo_vendor 'example' not supported. Only 'github, gitlab' supported. Commit hashes and filepaths will not be linkified.
DEBUG   -  mkdocs_git_latest_changes_plugin: Initialized repo `<git.repo.base.Repo '/home/colo/gitlab/mkdocs/.git'>`, branch `main`...
INFO    -  mkdocs_git_latest_changes_plugin: 282 files found in git index and working tree.
DEBUG   -  mkdocs_git_latest_changes_plugin: Processing file `.gitignore`...
WARNING -  mkdocs_git_latest_changes_plugin: An exception of type KeyError occurred. Arguments:
           ('example',)
ERROR   -  Error reading page 'latest.md':
ERROR   -  An exception of type KeyError occurred. Arguments:
           ('example',)

FTR, this is the (example) Markdown that causes the error:

---
title: Latest Changes
description: Latest Changes
tags:
  - changes
---

# Hi!

{{ latest_changes }}

Alas, I'm not sure how I'd need to instrument mkdocs to make the stack trace more useful (i.e., do not omit line numbers) than what the mkdocs build --verbose invocation reproduced above yields... if you could help me out there, I think I'll be able to determine the root cause myself :)

jtru commented 8 months ago

Seems like get_remote_repo_urls() too eagerly uses parts of repo_url as a key for accessing the SUPPORTED_REMOTE_REPOS dict.

tombreit commented 8 months ago

Hi @jtru ,

could you please provide your mkdocs.yml? Or a complete MkDocs-repo? This whole "get repo vendor name" thing is a bit rough atm..

Feel free to open an issue for this bug - sorry for hijacking this pr discussion ;-)

Update: Should be fixed by https://github.com/tombreit/mkdocs-git-latest-changes-plugin/commit/6d3fe2202d6deb532804d32ff8ee0abaf74f8ef1

Perhaps it would be nice to support custom (gitlab-)repos like yours, eventually via a dedicated plugin setting repo_vendor? Than your file paths and commit hashes in your latest changes table would be linked to the respective remote remote item.

jtru commented 8 months ago

Sorry, I cannot share the actual repo I am working with, since some of the information contained is not public information. However, it should suffice to set

repo_url: https://example.thismakesthepluginexplode.invalidtld

in mkdocs.yml (and enable the plugin) in any repo to trigger the problem.

Catching a KeyError exception in get_remote_repo_urls() should do the trick to make this non-fatal, but I am not sure what is the correct approach to deal with the consequences of the error.

However, I think it should not even come this far, since a prior check's result claims that Commit hashes and filepaths will not be linkified., which should make the actual value of repo_url not matter, I guess?

tombreit commented 8 months ago

I know it's currently not very elegant, but as I mentioned: it should be fixed now, please try again with at least 3491883

jtru commented 8 months ago

Sorry, I had missed you updated the comment :) Tested again with 3491883, and mkdocs build succeeds again!

tombreit commented 8 months ago

Before 3491883 an unsupported repo_vendor triggered a warning, now only an info. So it should be possible to run mkdocs in strict mode in your case.

If you don't catch any more bugs, I might put a new release on pypi.

tombreit commented 8 months ago

Dear @jtru , what to do with your pull request? Are you ok with the current situation and should we close this PR?

jtru commented 8 months ago

I think your changes to address the initial problem are clearly superior to the approach I suggested. Thanks for putting in the effort, we're happy that we can use 0.0.17! :)