Support homebrew/npm etc

[siutsin]

It will be great if I can install git-standup from homebrew, npm and apt-get etc.

[linkyndy]

I totally support this. Here's how to create a Homebrew formula. I can try to do it if there's no one else with more experience with this than me.

[kamranahmedse]

@linkyndy sure, go ahead.

[linkyndy]

Seems that we'd need a version for git-standup.

[kamranahmedse]

It seems like someone already has submitted the pull request.

[linkyndy]

Indeed, someone was faster :) Great, we have a brew formula.

[md5]

See https://github.com/Homebrew/homebrew-core/pull/251

[md5]

Ah. Just saw that @kamranahmedse referenced it.

One improvement that could be made would be to make git standup --help work by adding a man page. I'm using that command as a test, and git currently exits with $? = 1.

[kamranahmedse]

@md5 yeah good idea. Will add it as soon as I can.

[md5]

@kamranahmedse The failure of git standup --help was raised during the review of https://github.com/Homebrew/homebrew-core/issues/251

Another point that was raised is that it would be good to use make install in the formula. However, to do that, make install would need to be able to accept an alternate prefix. Do you think that's a change you can make as well? I would look into it myself, but I don't have time today and will be out of town for a few days starting tomorrow.

[apjanke]

However, to do that, make install would need to be able to accept an alternate prefix.

It's almost there already. prefix is there in the Makefile, and you can override any make variables by passing them on the command line: make install prefix=/opt/some/path/to/local. Problem is just that it assumes $(prefix)/bin already exists.

Here's a PR to fix it, and to set up other dir variables so it'll play nice with package managers or alternate installation locations. https://github.com/kamranahmedse/git-standup/pull/20

[md5]

@apjanke I didn't realize that variables in a Makefile worked like that. I thought it would need some explicit handling for overrides as you would use in a shell script.

[md5]

FYI, I ended up closing https://github.com/Homebrew/homebrew-core/issues/251

Since git-standup is so new, @apjanke suggested a Homebrew "tap" would be a better way to go before moving it into the Homebrew core. @kamranahmedse I think you could very easily move the formula I created into a kamranahmedse/homebrew-git-standup tap (here's one I did for gowsdl a while back). Having a tap would allow git-standup to be installed like so:

$ brew tap kamranahmedse/git-standup
$ brew install git-standup

Once that is stable and this repo is more than a month old, another PR can be opened to move it into homebrew-core.

[md5]

Here's the commit with the Formula I created for reference: https://github.com/md5/homebrew-core/commit/b8162d5edc9cb739eb936002e5a07779e1a9a1b6

[apjanke]

I didn't realize that variables in a Makefile worked like that. I thought it would need some explicit handling for overrides as you would use in a shell script.

Makefiles are quite different from shell scripts in a lot of ways. (The individual commands they end up running in targets are shell commands. But basically everything else about their structure, behavior, and use of variables is different.) It's worth reading the GNU make documentation if you're going to use make at all. The "Setting Variables" section will give you the lowdown on variable overriding behavior.

[kamranahmedse]

@md5 thank you :) Will create a repository for that.

[md5]

@kamranahmedse https://github.com/Homebrew/homebrew-core/pull/251 has been re-opened, so perhaps hold off a bit.

I still think the addition of a man page and updating the Homebrew formula to use system "make", "install", "prefix=#{prefix}" would be worthwhile changes.

[md5]

https://github.com/Homebrew/homebrew-core/pull/251 has been merged :+1:

[kamranahmedse]

@md5 thank you for the follow up, It looks great.

But, I can't put this in read-me just yet. Because, it has changed a lot since then and I guess we should wait for the man page and some enhancements that we have planned and will create a new PR.

[nicosomb]

Just opened a new PR for new release in homebrew project https://github.com/Homebrew/homebrew-core/pull/500

[DanyC97]

@nicosomb maybe we want to bump the version to latest one

[kamranahmedse]

@DanyC97 thanks to @nicosomb, he already submitted the PR with the updated formula https://github.com/Homebrew/homebrew-core/pull/501 still waiting on that one.

[nicosomb]

I opened this PR https://github.com/Homebrew/homebrew-core/pull/501 2 days ago But I need to fix tests. Help needed.

[kamranahmedse]

@nicosomb wish I could help but I have never released a brew package or worked with ruby, so I am not sure. 😔

[nicosomb]

It's my first one ;-) And you know your app, maybe you can try with me!

[kamranahmedse]

Hmm. Let's see :) Did you look at the stack-trace? http://bot.brew.sh/job/Homebrew%20Core%20Pull%20Requests/823/version=yosemite/console

[nicosomb]

Yes I did. But I don't know why git standup failed.

[apjanke]

IIRC, the previous failure under Homebrew was related to a tput call. This could be something related to terminal support. The test runs for Homebrew's BrewTestBot are done headless: the input and output streams are attached to pipes, and the environment variable $TERM is not defined. git standup uses terminal formatting features. You might want to make sure those degrade gracefully when it is run without a TTY attached and in an environment with no $TERM. That would let it run under test harnesses like this, and be called in more ways from inside a script.

[kamranahmedse]

Thanks @apjanke I will look into it.

[kamranahmedse]

@apjanke do you think https://github.com/kamranahmedse/git-standup/commit/92fe3f5a32f906f2842c59940f75a718db090868 would have any effect?

[apjanke]

I don't think so. That inner block was properly protected by if [[ -t 1 ]] already, so I don't think 92fe3f5a32f906f2842c59940f75a718db090868 was necessary or will have any effect. Those color variables will be empty by default.

This part probably needs an error check. Moving inside the if [[ -t 1 ]] will probably work. Then the if [[ -n "$ncolors" ]] && [[ "$ncolors" -ge 8 ]] part can be nested inside it.

  if which tput >/dev/null 2>&1; then
    ncolors=$(tput colors)
  fi

The thing to do for debugging this is to try to reproduce the failure in the first place. You can probably do so by unsetting $TERM, or setting it to a bogus value, and see what happens.

[apjanke]

Here's a clue. If it's erroring out and aborting the script, that means the error is happening after the set -e, which comes after that whole color initialization block.

There's this.

GIT_LOG_COMMAND="git --no-pager log \
    --all
...
    --pretty=format:'%Cred%h%Creset - %s %Cgreen(%cr) %C(bold blue)<%an>%Creset'"

That pretty-format specification uses terminal-dependent formatting. That could be it. Maybe you should switch between two different pretty formats depending on whether you're inside a terminal.

And personally, I'd move that set -e closer to the top, as a way of forcing yourself to use "cleaner" code.

[kamranahmedse]

Thank you for such a detailed insight :) Maybe you can create a PR with this, if possible. Anyways, I will try it out later today.

[apjanke]

Already there: #37 :)

[kamranahmedse]

Merged. Great :)

[apjanke]

Cool.

Following it up with #38, which will create a simple exit-with-success git standup -h, which we can use as a trivial test in Homebrew. Merge that (or something like it), and I think we'll be ready to make a Homebrew formula for it tomorrow.

[apjanke]

Cool. If you cut another release with #38 included and update https://github.com/Homebrew/homebrew-core/pull/501 to point to it (preferably squashing to a single commit), we'll merge it and you'll be in Homebrew.

[kamranahmedse]

Release is here. We'll have to wait for @nicosomb to update the PR https://github.com/Homebrew/homebrew-core/pull/501.

[nicosomb]

I just need to update the version?

[apjanke]

Update the version, remove the ENV["TERM"] = "xterm" from the test since it's no longer needed, and squash it to a single commit (using git rebase -i master).

[nicosomb]

Done.

[apjanke]

Looks good!

The test is still failing for some reason, though. Doesn't seem like it should. Maybe that's some other interaction with the sandbox that the test-bot runs it in.

I'll help you debug it tomorrow; it's getting late in my time zone tonight.

If you'd like to debug it more yourself, you can check out the branch for your PR, and run brew test-bot --ci-pr git-standup --keep-tmp --verbose and see if you find any clues.

[apjanke]

Aha, I think.

AUTHOR=`git config user.name`

It's possible for that to fail if there is no user.name defined. And now that the script is doing set -e, that failure will cause it to bail out.

I debugged this by putting set -x at the top of the script. That enables bash tracing of all the commands it runs. Combine that with set -e and the last command run is probably your culprit.

$ brew test --verbose git-standup
Testing git-standup
==> Using the sandbox
...
==> git standup
+ getopts hd:a:w: opt
+ shift 0
+ [[ 0 -gt 0 ]]
+ [[ -n '' ]]
+ [[ -t 1 ]]
++ git config user.name
+ AUTHOR=
Error: git-standup: failed
Failed executing: git standup

You might just want to remove the set -e, since git-standup doesn't do anything destructive, and -e doesn't produce useful error messages. Otherwise, stick that AUTHOR=git config user.name inside an if or those { } || { } error handling blocks.

[nicosomb]

OK, so it's a mission for @kamranahmedse ;-) !

[kamranahmedse]

Strange, in my case when I unset my username it still showed logs (but for every user now).

Anyways, I have removed the line set -e. Here is the link to updated release.

[apjanke]

Strange, in my case when I unset my username it still showed logs (but for every user now).

Here's my guess: When you unset your username, git config user.name is probably returning the empty string (and without set -e or a return value check, it's happily continuing). The argument to git log's --author is a regex pattern, and it is not anchored by default. So the empty pattern "" will match any string. and --author="" means display all authors.

Might want to check $AUTHOR for the empty string and error out if that's the case, since the default behavior is to show just the current user's changes.

[kamranahmedse]

Yeah, your guess is perfectly valid. :-)

But for now, instead of adding any additional checks, I think we can continue with the current release and see if this removal of set -e makes it pass the tests as is.

@nicosomb your turn ;-)

[nicosomb]

Already done today https://github.com/Homebrew/homebrew-core/pull/501 ;-)

[kamranahmedse]

Closing this in favour of https://github.com/kamranahmedse/git-standup/issues/42, since brew and npm packages have been created.