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

history: Add option to show timestamps #3175

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
@bbarenblat
Contributor

bbarenblat commented Jun 27, 2016

Closes #677.


  • Documentation updated
@floam

This comment has been minimized.

Show comment
Hide comment
@floam

floam Jun 28, 2016

Member

Cool! I wonder how difficult it would be to modify history.cpp to also throw in the microsecond field (or just change the timestamps to be with some decimal points from timef(), I guess). This would let us expose what we currently only do by $CMD_DURATION through the history interface.

It'd be important to maintain compatibility with the history files (preferably both ways).

Member

floam commented Jun 28, 2016

Cool! I wonder how difficult it would be to modify history.cpp to also throw in the microsecond field (or just change the timestamps to be with some decimal points from timef(), I guess). This would let us expose what we currently only do by $CMD_DURATION through the history interface.

It'd be important to maintain compatibility with the history files (preferably both ways).

@@ -301,6 +301,7 @@ AC_CHECK_FUNCS( futimes )
AC_CHECK_FUNCS( wcslcpy lrand48_r killpg )
AC_CHECK_FUNCS( backtrace_symbols getifaddrs )
AC_CHECK_FUNCS( futimens clock_gettime )
AC_CHECK_FUNCS( localtime_r )

This comment has been minimized.

@krader1961

krader1961 Jun 28, 2016

Contributor

What is the point of this configure check? This change calls localtime() unconditionally. Which is reasonable since no OS fish runs on should not have an implementation for that function.

@krader1961

krader1961 Jun 28, 2016

Contributor

What is the point of this configure check? This change calls localtime() unconditionally. Which is reasonable since no OS fish runs on should not have an implementation for that function.

This comment has been minimized.

@floam

floam Jun 28, 2016

Member

It's not localtime, it's localtime_r which is sometimes not available on Win32 IIRC.

@floam

floam Jun 28, 2016

Member

It's not localtime, it's localtime_r which is sometimes not available on Win32 IIRC.

This comment has been minimized.

@zanchey

zanchey Jun 28, 2016

Member

AC_CHECK_FUNCS defines a preprocessor symbol if the function is available, but there is no check for this in the code (and no alternative handling).

@zanchey

zanchey Jun 28, 2016

Member

AC_CHECK_FUNCS defines a preprocessor symbol if the function is available, but there is no check for this in the code (and no alternative handling).

This comment has been minimized.

@bbarenblat

bbarenblat Jun 28, 2016

Contributor

@zanchey: Do we need to support platforms which don’t define localtime_r? I’m really not wild about using localtime, since it’s not thread-safe. Would it be acceptable for configure to just halt with an error if localtime_r is undefined?

@bbarenblat

bbarenblat Jun 28, 2016

Contributor

@zanchey: Do we need to support platforms which don’t define localtime_r? I’m really not wild about using localtime, since it’s not thread-safe. Would it be acceptable for configure to just halt with an error if localtime_r is undefined?

This comment has been minimized.

@krader1961

krader1961 Jun 28, 2016

Contributor

Somewhat surprisingly we don't currently have a single localtime() call in the code so it's use in this change would be thread-safe since it is the only thread that would ever call it. However, that's putting down a land mine. So this is a case where a fallback function needs to be defined if the platform doesn't provide it. The fallback should just use a simple global lock around the call to localtime() to ensure the single internal result buffer isn't overwritten before it's contents are copied into the user supplied result buffer. Look in the code for instances of scoped_lock locker(lock); //!OCLINT(side-effect) for how to do that.

I should also note that you change has another bug: You don't call tzset() (and neither does the current fish code). It's possible the internal timezone state is being set indirectly via some other call but you shouldn't rely on that.

@krader1961

krader1961 Jun 28, 2016

Contributor

Somewhat surprisingly we don't currently have a single localtime() call in the code so it's use in this change would be thread-safe since it is the only thread that would ever call it. However, that's putting down a land mine. So this is a case where a fallback function needs to be defined if the platform doesn't provide it. The fallback should just use a simple global lock around the call to localtime() to ensure the single internal result buffer isn't overwritten before it's contents are copied into the user supplied result buffer. Look in the code for instances of scoped_lock locker(lock); //!OCLINT(side-effect) for how to do that.

I should also note that you change has another bug: You don't call tzset() (and neither does the current fish code). It's possible the internal timezone state is being set indirectly via some other call but you shouldn't rely on that.

This comment has been minimized.

@floam

floam Jun 28, 2016

Member

It looks like it's going to be mingw that may not have this - seems like it can be expected on Cygwin? Perhaps it's not an issue?

@floam

floam Jun 28, 2016

Member

It looks like it's going to be mingw that may not have this - seems like it can be expected on Cygwin? Perhaps it's not an issue?

@floam

This comment has been minimized.

Show comment
Hide comment
@floam

floam Jun 28, 2016

Member

Would it perhaps be better to implement some of this in the classes in history.cpp rather than builtin.cpp?

I notice we are not calling history_t::get_string_representation now but instead a function we define in the global namespace, in builtin.cpp, to format and also print the history items.

Member

floam commented Jun 28, 2016

Would it perhaps be better to implement some of this in the classes in history.cpp rather than builtin.cpp?

I notice we are not calling history_t::get_string_representation now but instead a function we define in the global namespace, in builtin.cpp, to format and also print the history items.

@bbarenblat

This comment has been minimized.

Show comment
Hide comment
@bbarenblat

bbarenblat Jun 28, 2016

Contributor

@floam: Since the timestamp format is a property of the history builtin, not of the history itself, this seems like a better place for it. format_history_record is defined in the global namespace, but it’s static, so it won’t be exported.

Contributor

bbarenblat commented Jun 28, 2016

@floam: Since the timestamp format is a property of the history builtin, not of the history itself, this seems like a better place for it. format_history_record is defined in the global namespace, but it’s static, so it won’t be exported.

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Jun 29, 2016

Contributor

I've confirmed the configure check for localtime_r() is not required on Cygwin. We don't support building fish using mingw; only gcc (or clang). I'll merge this after merging PR #3181. I'll also update the documentation.

Thanks for doing this, @bbarenblat. In the future remember to update documentation and unit tests.

Contributor

krader1961 commented Jun 29, 2016

I've confirmed the configure check for localtime_r() is not required on Cygwin. We don't support building fish using mingw; only gcc (or clang). I'll merge this after merging PR #3181. I'll also update the documentation.

Thanks for doing this, @bbarenblat. In the future remember to update documentation and unit tests.

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Jun 29, 2016

Contributor

I've merged this change, minus the configure.ac change as commit 7e08679. I'm not going to close this until my change to update the documentation and share/functions/history.fish script is also merged.

Contributor

krader1961 commented Jun 29, 2016

I've merged this change, minus the configure.ac change as commit 7e08679. I'm not going to close this until my change to update the documentation and share/functions/history.fish script is also merged.

@floam floam added docs release notes and removed docs labels Jun 29, 2016

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Jul 1, 2016

Contributor

I fixed the history function issues with commit cbee315.

Contributor

krader1961 commented Jul 1, 2016

I fixed the history function issues with commit cbee315.

@krader1961 krader1961 closed this Jul 1, 2016

@floam floam added this to the next-2.x milestone Jul 3, 2016

@floam floam added the enhancement label Jul 3, 2016

@krader1961 krader1961 modified the milestones: fish 2.3.2, next-2.x Sep 3, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment