Re: [PATCH 9/9] Use timer_settime for new platforms

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 29, 2014, 18:02 UTC
Message-ID: <xmqqd2bj6uol.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1409330561-11806-9-git-send-email-jacob.e.keller@intel.com>

Jacob Keller <jacob.e.keller@intel.com> writes:

Show 10 quoted lines

> From: Jonas 'Sortie' Termansen <sortie@maxsi.org>
>
> setitimer() is an obsolescent XSI interface and may be removed in a
> future standard. Applications should use the core POSIX timer_settime()
> instead.
>
> It's important that code doesn't simply check if timer_settime is
> available as it can give false positives. Some systems like contemporary
> OpenBSD provides the function, but it unconditionally fails with ENOSYS
> at runtime.

Doesn't this paragraph need tweaking? I think you lost (which is a good thing) "notice that timer_settime() call failed with ENOSYS and switch to setitimer()", no?

Show 5 quoted lines

> Clean up the progress reporting and change it to use timer_settime,
> which will fall back to setitimer automatically if timer_settime is not
> supported. (see git-compat-util.h for how it does this). If both
> functions are not present, then git-compat-util.h provides replacements
> which will always fail with ENOSYS.

While this paragraph may be true if patch 8b and 9 are taken together, isn't what it describes mostly what 8b did, not 9?

Here by 8b I mean the change to git-compat-util.h in 8; the patch might want to be split into two, 8a for the autoconf part whose log message may begin with "This function was not previously used by git." and 8b that adds an emulation of timer_settime() API in terms of setitimer() API, or the other way around.

What 9 did is only "we used to use the setitmer() API to implement the progress reporting; now we use timer_settime() API" (yes, it is thanks to the abstraction given by 8, but the "callers has to only know about one API, not worrying about the other API" is a merit attributable to 8b, not this one).

Show 6 quoted lines

> The approach used here enables us to use a single API (timer_settime)
> without having to worry about checking for #ifdefs or if blocks which
> make it an unreadable nightmare. The major downside is for systems
> without timer_settime support, they may fall back on a wrapped
> implementation which could have subtle differences. This should be a
> minor issue as almost all modern systems provide timer_settime support.

As this paragraph.

> Note that this change means that git should never use setitimer on its
> own now, as the fallback implementation of timer_settime assumes that it
> is the sole user of ITIMER_REAL, and timer_delete will reset the
> ITIMER_REAL.

And this one.