Skip to content

Fix regex for new GitHub output of pull #2

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.

Sign up for GitHub

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

Closed
lordofbikes wants to merge 5 commits into from
Closed

Fix regex for new GitHub output of pull #2

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9de315f
added from-field
ilyasergey Mar 6, 2017
88bbc04
changelog update
ilyasergey Mar 6, 2017
fd26a44
fixes
ilyasergey Mar 6, 2017
9f3fa64
Merge changes from 'ilyasergey/github-wiki-notify'
lordofbikes Jan 29, 2022
c7e6d3f
update regex pattern for new GitHub output
lordofbikes Jan 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,14 @@
v1.0.3 / 2022-01-29:
Copy link
Owner

@awbush awbush Feb 5, 2022

Choose a reason for hiding this comment

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

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

Copy link
Owner

@awbush awbush Feb 6, 2022

Choose a reason for hiding this comment

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

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

Copy link
Author

@lordofbikes lordofbikes Feb 20, 2022

Choose a reason for hiding this comment

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

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.


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

@awbush awbush Feb 5, 2022

Choose a reason for hiding this comment

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

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

Copy link
Author

@lordofbikes lordofbikes Feb 20, 2022

Choose a reason for hiding this comment

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

You're right, therefore of course.

+ Request repo link from git remote, it is no longer in pullResult
+ pullResult answer is now 'Updating FROMSHA..TO__SHA'
+ also pullResult answer is localized!

v1.0.2 / 2017-03-06:

- Added the "from" key

v1.0.1 / 2012-01-31:

- Remove branch tags from log output (not that useful in github's wiki context)
Expand Down
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ Get a checkout of your Github repo:

Then create a cron (e.g. `crontab -e`):

*/15 * * * * github-wiki-notify.php --path="/some/path/repo.wiki" --email="list@example.com" --subject="Wiki updated!"
*/15 * * * * github-wiki-notify.php --path="/some/path/repo.wiki" --email="list@example.com" --subject="Wiki updated!" --from="from@example.org"

Problems? Want to contribute?
-----------------------------
Expand Down
25 changes: 18 additions & 7 deletions github-wiki-notify.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
$path = null;
$email = null;
$subject = null;
$from = null;
foreach ($argv as $arg)
{
if (preg_match('/--path=(.*)/', $arg, $match)) {
Expand All @@ -29,13 +30,15 @@
$email = $match[1];
} else if (preg_match('/--subject=(.*)/', $arg, $match)) {
$subject = $match[1];
} else if (preg_match('/--from=(.*)/', $arg, $match)) {
$from = $match[1];
}
}

if (is_null($path) || is_null($email))
if (is_null($path) || is_null($email) || is_null($from))
Copy link
Owner

@awbush awbush Feb 5, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Author

@lordofbikes lordofbikes Feb 20, 2022

Choose a reason for hiding this comment

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

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

Copy link
Owner

@awbush awbush Feb 20, 2022

Choose a reason for hiding this comment

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

Yes, version can stay 1.x.x

{
echo("Usage:\n");
echo(" " . basename(__FILE__) . " --path=/path/to/repo --email=list@example.com\n");
echo(" " . basename(__FILE__) . " --path=/path/to/repo --email=list@example.com --from=my@email.com\n");
exit(1);
}

Expand All @@ -44,18 +47,26 @@
exit(2);
}

// request repo-URL from git remote
$remote = `git remote -v`;
Copy link
Owner

@awbush awbush Feb 5, 2022

Choose a reason for hiding this comment

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

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)

$repo = 'unknown';
if (preg_match('/origin\s*(\S*)\s*\(fetch\)\n/', $remote, $match))
{
$repo = $match[1];
}

$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))
Copy link
Owner

@awbush awbush Feb 6, 2022

Choose a reason for hiding this comment

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

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).

Copy link
Author

@lordofbikes lordofbikes Feb 20, 2022

Choose a reason for hiding this comment

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

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?

Copy link
Owner

@awbush awbush Feb 20, 2022

Choose a reason for hiding this comment

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

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.

{
$repo = $match[1];
$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;
Copy link
Owner

@awbush awbush Feb 5, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Author

@lordofbikes lordofbikes Feb 20, 2022

Choose a reason for hiding this comment

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

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.

$changeLog = `git log --pretty=format:'%h - %s (%cr) <%an>' $revs`;

if (is_null($subject)) {
$subject = '[SCM]: ' . $repo . ' was updated';
}
$body = "To see the changes, visit:\n" . $wikiDiffUrl . "\n\nChangelog:\n" . $changeLog . "\n";
mail($email, $subject, $body, "From: $email");
mail($email, $subject, $body, "From: $from");
}
// else no updates