Fix regex for new GitHub output of pull #2

lordofbikes · 2022-01-29T15:19:28Z

Just found this little, very helpful script, but it was not working anymore.
Seems output from git pull has changed, so the regex patterns needed some revamp.
It turned out, that the output is even localized, thus the pullResult pattern is very basic, but I think it is safe enough to detect changes.

I also pulled improvement from https://github.com/ilyasergey/github-wiki-notify.

Updated the CHANGELOG and README too.

awbush

Thank you for the pull request! Need to at least bump the major version, but see comments.

awbush · 2022-02-05T11:10:27Z

github-wiki-notify.php

 }

-if (is_null($path) || is_null($email))
+if (is_null($path) || is_null($email) || is_null($from))


Should --from be optional and copied from --email when not present? This would enable backwards compatibility, but I'm not sure it makes sense to others.

For example, a dev@example.com mailing list using the same to and from so that replies from developers work. But does it make more sense to require them to specify it twice as --email=dev@example.com --from=dev@example.com?

I'm okay with requiring with the --from, but we should bump the semver to 2.0.0 to show the backwards-incompatible change.

Optional --from make absolutely sense to me too, I'll change this.
So version can be 1.0.x then?

Yes, version can stay 1.x.x

awbush · 2022-02-05T11:11:57Z

CHANGELOG.md

@@ -1,3 +1,14 @@
+v1.0.3 / 2022-01-29:
+
+- GitHub output has changed, therefor regex syntax has to be corrected


I think "therefor" should be "therefore" per Therefore vs. Therefor @ Grammarly

You're right, therefore of course.

awbush · 2022-02-05T11:13:28Z

github-wiki-notify.php

 }

+// request repo-URL from git remote
+$remote = `git remote -v`;


Instead of this and regex below, would git remote get-url origin be better (per docs in git remote -h)?

Example output of git remote -v:

origin git@github.com:awbush/test-wiki.wiki.git (fetch) origin git@github.com:awbush/test-wiki.wiki.git (push)

Example output of git remote get-url origin:

git@github.com:awbush/test-wiki.wiki.git

For an HTTPS checkout, the above would be:

https://github.com/awbush/test-wiki.wiki.git

(But note that $repo was previously awbush/test-wiki.wiki in this example; see also comment below for $wikiDiffUrl)

awbush · 2022-02-05T12:20:23Z

github-wiki-notify.php

-	$revs = $match[2];
-	$wikiDiffUrl = 'https://github.com/' . str_replace('.wiki', '/wiki', $repo) . '/_compare/' . $revs;
+	$revs = $match[1];
+	$wikiDiffUrl = str_replace('.wiki.git', '/wiki', $repo) . '/_compare/' . $revs;


This assumes the repo was checked out using HTTPS, which isn't a requirement. For example, an SSH clone like git clone git@github.com:awbush/test-wiki.wiki.git still works. After a change and running git pull it can have the output:

$ git pull remote: Enumerating objects: 4, done. remote: Counting objects: 100% (4/4), done. remote: Compressing objects: 100% (2/2), done. remote: Total 3 (delta 0), reused 0 (delta 0), pack-reused 0 Unpacking objects: 100% (3/3), 291 bytes | 145.00 KiB/s, done. From github.com:awbush/test-wiki.wiki a4da57f..7e129dd master -> origin/master Updating a4da57f..7e129dd Fast-forward Page2.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 Page2.md

For HTTPS checkout that's:

$ git pull ... From https://github.com/awbush/test-wiki.wiki a4da57f..7e129dd master -> origin/master Updating a4da57f..7e129dd ...

Perhaps we should be pulling $revs from the Updating ... piece?

And perhaps pull $repo from something else? $repo was originally meant to be awbush/test-wiki.wiki in the above example, i.e. the repo name at github, and not a full github.com:awbush/test-wiki.wiki.git or https://github.com/awbush/test-wiki.wiki.git as appears to be the case in this pull request.

I didn't consider SSH clone, as it needs to setup authentication on the host, what could be a security issue.
But of course, when the script is run from a save place, SSH clone is a valid option.

awbush · 2022-02-05T12:58:02Z

CHANGELOG.md

@@ -1,3 +1,14 @@
+v1.0.3 / 2022-01-29:


This should probably be v2.0.0 / ... since it introduces backwards incompatible changes (see comments below)

Actually this should be v2.0.1 and v1.0.2 below should be v2.0.0 (if keeping the --from requirement)

The last tagged version was v1.0.1. The v1.0.2 I pulled in from ilyasergey/github-wiki-notify was never official or even tagged.
So I suggest to collect changes and make this v1.0.2 with optional --from.

awbush · 2022-02-06T03:58:42Z

github-wiki-notify.php

+
 $pullResult = `git pull 2>&1`;
-if (preg_match('/From github\.com:(.*)\n\s*([^\s]+)/', $pullResult, $match))
+if (preg_match('/^\S* ([a-z0-9]{7}\.\.[a-z0-9]{7})\n/', $pullResult, $match))


What if you restore all the other code (remove git remote -v above and the changes inside this if block) and simply update this regex to support both the HTTPS and SSH formats? This regex would do it:

'/From .*?github\.com[\/:](.*)\n\s*([^\s]+)/'

So then $repo would be awbush/test-wiki.wiki and $revs would be a4da57f..7e129dd (in both examples below).

With your notes I agree to give it a second try.
What is your preferred solution?
Should I add a fixing commit to this branch or should I start over a new one?
And which version number should I use at least?

I like the idea of just updating the regex. It focuses on the issue: Add support for HTTPS remotes (while keeping support for legacy/SSH ones).

You can add a commit if that's easiest. I can squash merge into main branch.

lordofbikes · 2022-06-25T12:48:24Z

closing this in favor of #3

ilyasergey and others added 5 commits March 6, 2017 14:15

added from-field

9de315f

changelog update

88bbc04

fixes

fd26a44

Merge changes from 'ilyasergey/github-wiki-notify'

9f3fa64

update regex pattern for new GitHub output

c7e6d3f

awbush requested changes Feb 5, 2022

View reviewed changes

awbush reviewed Feb 6, 2022

View reviewed changes

lordofbikes mentioned this pull request Jun 25, 2022

Fix for localized git pull output #3

Open

lordofbikes closed this Jun 25, 2022

lordofbikes deleted the fix_regex branch June 25, 2022 12:48

		@@ -1,3 +1,14 @@
		v1.0.3 / 2022-01-29:

		- GitHub output has changed, therefor regex syntax has to be corrected

Fix regex for new GitHub output of pull #2

Fix regex for new GitHub output of pull #2

Uh oh!

Conversation

lordofbikes commented Jan 29, 2022

Uh oh!

awbush left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lordofbikes commented Jun 25, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants