Re: [PATCH 1/1] poll: use GetTickCount64() to avoid wrap-around issues

[Steve Hoelzer]

On Fri, Nov 2, 2018 at 11:43 AM Johannes Sixt  wrote:
>
> Am 02.11.18 um 15:47 schrieb Steve Hoelzer:
> > On Thu, Nov 1, 2018 at 5:22 AM Johannes Sixt  wrote:
> >>
> >> Am 31.10.18 um 22:11 schrieb Steve Hoelzer via GitGitGadget:
> >>> @@ -614,7 +618,7 @@ restart:
> >>>
> >>>  if (!rc && orig_timeout && timeout != INFTIM)
> >>>{
> >>> -  elapsed = GetTickCount() - start;
> >>> +  elapsed = (DWORD)(GetTickCount64() - start);
> >>
> >> AFAICS, this subtraction in the old code is the correct way to take
> >> account of wrap-arounds in the tick count. The new code truncates the 64
> >> bit difference to 32 bits; the result is exactly identical to a
> >> difference computed from truncated 32 bit values, which is what we had
> >> in the old code.
> >>
> >> IOW, there is no change in behavior. The statement "avoid wrap-around
> >> issues" in the subject line is not correct. The patch's only effect is
> >> that it removes Warning C28159.
> >>
> >> What is really needed is that all quantities in the calculations are
> >> promoted to ULONGLONG. Unless, of course, we agree that a timeout of
> >> more than 49 days cannot happen ;)
> >
> > Yep, correct on all counts. I'm in favor of changing the commit message to
> > only say that this patch removes Warning C28159.
>
> How about this fixup instead?
>
>  8< 
> squash! poll: use GetTickCount64() to avoid wrap-around issues
>
> The value of timeout starts as an int value, and for this reason it
> cannot overflow unsigned long long aka ULONGLONG. The unsigned version
> of this initial value is available in orig_timeout. The difference
> (orig_timeout - elapsed) cannot wrap around because it is protected by
> a conditional (as can be seen in the patch text). Hence, the ULONGLONG
> difference can only have values that are smaller than the initial
> timeout value and truncation to int cannot overflow.
>
> Signed-off-by: Johannes Sixt 
> ---
>  compat/poll/poll.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/compat/poll/poll.c b/compat/poll/poll.c
> index 4abbfcb6a4..4459408c7d 100644
> --- a/compat/poll/poll.c
> +++ b/compat/poll/poll.c
> @@ -452,7 +452,7 @@ poll (struct pollfd *pfd, nfds_t nfd, int timeout)
>static HANDLE hEvent;
>WSANETWORKEVENTS ev;
>HANDLE h, handle_array[FD_SETSIZE + 2];
> -  DWORD ret, wait_timeout, nhandles, elapsed, orig_timeout = 0;
> +  DWORD ret, wait_timeout, nhandles, orig_timeout = 0;
>ULONGLONG start = 0;
>fd_set rfds, wfds, xfds;
>BOOL poll_again;
> @@ -618,8 +618,8 @@ poll (struct pollfd *pfd, nfds_t nfd, int timeout)
>
>if (!rc && orig_timeout && timeout != INFTIM)
>  {
> -  elapsed = (DWORD)(GetTickCount64() - start);
> -  timeout = elapsed >= orig_timeout ? 0 : orig_timeout - elapsed;
> +  ULONGLONG elapsed = GetTickCount64() - start;
> +  timeout = elapsed >= orig_timeout ? 0 : (int)(orig_timeout - elapsed);
>  }
>
>if (!rc && timeout)
> --
> 2.19.1.406.g1aa3f475f3

I like it. This still removes the warning and avoids overflow issues.

Steve

Re: [PATCH 1/1] poll: use GetTickCount64() to avoid wrap-around issues

[Steve Hoelzer]

On Thu, Nov 1, 2018 at 5:22 AM Johannes Sixt  wrote:
>
> Am 31.10.18 um 22:11 schrieb Steve Hoelzer via GitGitGadget:
> > From: Steve Hoelzer 
> >
> >  From Visual Studio 2015 Code Analysis: Warning C28159 Consider using
> > 'GetTickCount64' instead of 'GetTickCount'.
> >
> > Reason: GetTickCount() overflows roughly every 49 days. Code that does
> > not take that into account can loop indefinitely. GetTickCount64()
> > operates on 64 bit values and does not have that problem.
> >
> > Note: this patch has been carried in Git for Windows for almost two
> > years, but with a fallback for Windows XP, as the GetTickCount64()
> > function is only available on Windows Vista and later. However, in the
> > meantime we require Vista or later, hence we can drop that fallback.
> >
> > Signed-off-by: Steve Hoelzer 
> > Signed-off-by: Johannes Schindelin 
> > ---
> >   compat/poll/poll.c | 10 +++---
> >   1 file changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/compat/poll/poll.c b/compat/poll/poll.c
> > index ad5dcde439..4abbfcb6a4 100644
> > --- a/compat/poll/poll.c
> > +++ b/compat/poll/poll.c
> > @@ -18,6 +18,9 @@
> >  You should have received a copy of the GNU General Public License along
> >  with this program; if not, see <http://www.gnu.org/licenses/>.  */
> >
> > +/* To bump the minimum Windows version to Windows Vista */
> > +#include "git-compat-util.h"
> > +
> >   /* Tell gcc not to warn about the (nfd < 0) tests, below.  */
> >   #if (__GNUC__ == 4 && 3 <= __GNUC_MINOR__) || 4 < __GNUC__
> >   # pragma GCC diagnostic ignored "-Wtype-limits"
> > @@ -449,7 +452,8 @@ poll (struct pollfd *pfd, nfds_t nfd, int timeout)
> > static HANDLE hEvent;
> > WSANETWORKEVENTS ev;
> > HANDLE h, handle_array[FD_SETSIZE + 2];
> > -  DWORD ret, wait_timeout, nhandles, start = 0, elapsed, orig_timeout = 0;
> > +  DWORD ret, wait_timeout, nhandles, elapsed, orig_timeout = 0;
> > +  ULONGLONG start = 0;
> > fd_set rfds, wfds, xfds;
> > BOOL poll_again;
> > MSG msg;
> > @@ -465,7 +469,7 @@ poll (struct pollfd *pfd, nfds_t nfd, int timeout)
> > if (timeout != INFTIM)
> >   {
> > orig_timeout = timeout;
> > -  start = GetTickCount();
> > +  start = GetTickCount64();
> >   }
> >
> > if (!hEvent)
> > @@ -614,7 +618,7 @@ restart:
> >
> > if (!rc && orig_timeout && timeout != INFTIM)
> >   {
> > -  elapsed = GetTickCount() - start;
> > +  elapsed = (DWORD)(GetTickCount64() - start);
>
> AFAICS, this subtraction in the old code is the correct way to take
> account of wrap-arounds in the tick count. The new code truncates the 64
> bit difference to 32 bits; the result is exactly identical to a
> difference computed from truncated 32 bit values, which is what we had
> in the old code.
>
> IOW, there is no change in behavior. The statement "avoid wrap-around
> issues" in the subject line is not correct. The patch's only effect is
> that it removes Warning C28159.
>
> What is really needed is that all quantities in the calculations are
> promoted to ULONGLONG. Unless, of course, we agree that a timeout of
> more than 49 days cannot happen ;)

Yep, correct on all counts. I'm in favor of changing the commit message to
only say that this patch removes Warning C28159.

Steve

[PATCH 1/1] poll: use GetTickCount64() to avoid wrap-around issues

[Steve Hoelzer via GitGitGadget]

From: Steve Hoelzer 

>From Visual Studio 2015 Code Analysis: Warning C28159 Consider using
'GetTickCount64' instead of 'GetTickCount'.

Reason: GetTickCount() overflows roughly every 49 days. Code that does
not take that into account can loop indefinitely. GetTickCount64()
operates on 64 bit values and does not have that problem.

Note: this patch has been carried in Git for Windows for almost two
years, but with a fallback for Windows XP, as the GetTickCount64()
function is only available on Windows Vista and later. However, in the
meantime we require Vista or later, hence we can drop that fallback.

Signed-off-by: Steve Hoelzer 
Signed-off-by: Johannes Schindelin 
---
 compat/poll/poll.c | 10 +++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/compat/poll/poll.c b/compat/poll/poll.c
index ad5dcde439..4abbfcb6a4 100644
--- a/compat/poll/poll.c
+++ b/compat/poll/poll.c
@@ -18,6 +18,9 @@
You should have received a copy of the GNU General Public License along
with this program; if not, see <http://www.gnu.org/licenses/>.  */
 
+/* To bump the minimum Windows version to Windows Vista */
+#include "git-compat-util.h"
+
 /* Tell gcc not to warn about the (nfd < 0) tests, below.  */
 #if (__GNUC__ == 4 && 3 <= __GNUC_MINOR__) || 4 < __GNUC__
 # pragma GCC diagnostic ignored "-Wtype-limits"
@@ -449,7 +452,8 @@ poll (struct pollfd *pfd, nfds_t nfd, int timeout)
   static HANDLE hEvent;
   WSANETWORKEVENTS ev;
   HANDLE h, handle_array[FD_SETSIZE + 2];
-  DWORD ret, wait_timeout, nhandles, start = 0, elapsed, orig_timeout = 0;
+  DWORD ret, wait_timeout, nhandles, elapsed, orig_timeout = 0;
+  ULONGLONG start = 0;
   fd_set rfds, wfds, xfds;
   BOOL poll_again;
   MSG msg;
@@ -465,7 +469,7 @@ poll (struct pollfd *pfd, nfds_t nfd, int timeout)
   if (timeout != INFTIM)
 {
   orig_timeout = timeout;
-  start = GetTickCount();
+  start = GetTickCount64();
 }
 
   if (!hEvent)
@@ -614,7 +618,7 @@ restart:
 
   if (!rc && orig_timeout && timeout != INFTIM)
 {
-  elapsed = GetTickCount() - start;
+  elapsed = (DWORD)(GetTickCount64() - start);
   timeout = elapsed >= orig_timeout ? 0 : orig_timeout - elapsed;
 }
 
-- 
gitgitgadget

Re: [ANNOUNCE] Git for Windows 2.14.2(3)

[Steve Hoelzer]

Johannes,

On Mon, Oct 16, 2017 at 5:57 AM, Johannes Schindelin
<johannes.schinde...@gmx.de> wrote:
> Hi Steve,
>
> On Sun, 15 Oct 2017, Johannes Schindelin wrote:
>
>> On Fri, 13 Oct 2017, Steve Hoelzer wrote:
>>
>> > On Thu, Oct 12, 2017 at 5:53 PM, Johannes Schindelin
>> > <johannes.schinde...@gmx.de> wrote:
>> > >
>> > > It is my pleasure to announce that Git for Windows 2.14.2(3) is
>> > > available from:
>> > >
>> > > https://git-for-windows.github.io/
>> > >
>> > > Changes since Git for Windows v2.14.2(2) (October 5th 2017)
>> > >
>> > > New Features
>> > >
>> > >   * Comes with Git LFS v2.3.3.
>> >
>> > I just ran "git update" and afterward "git version" reported
>> > 2.14.2(3), but "git lfs version" still said 2.3.2.
>> >
>> > I also uninstalled/reinstalled Git for Windows and LFS is still 2.3.2.
>>
>> Ah bummer. I forgot to actually update it in the VM where I build the
>> releases :-(
>>
>> Will work on it tomorrow.
>
> I'll actually use this opportunity to revamp a part of Git for Windows'
> release engineering process to try to prevent similar things from
> happening in the future.
>
> Also, cURL v7.56.1 is slated to be released in exactly one week, and I
> have some important installer work to do this week, so I'll just defer the
> new Git for Windows version tentatively to Monday, 23rd.
>
> Git LFS users can always install Git LFS v2.3.3 specifically in the
> meantime, or use Git for Windows' snapshot versions
> (https://wingit.blob.core.windows.net/files/index.html).

Sounds like a good plan.

I think I have successfully updated to LFS 2.3.3 by copying the new
git-lfs.exe into C:\Program Files\Git\mingw64\bin. Is that right way
to do it?

Thanks,
Steve

Re: [ANNOUNCE] Git for Windows 2.14.2(3)

[Steve Hoelzer]

Johannes,

On Thu, Oct 12, 2017 at 5:53 PM, Johannes Schindelin
 wrote:
> Dear Git users,
>
> It is my pleasure to announce that Git for Windows 2.14.2(3) is available 
> from:
>
> https://git-for-windows.github.io/
>
> Changes since Git for Windows v2.14.2(2) (October 5th 2017)
>
> New Features
>
>   * Comes with Git LFS v2.3.3.

I just ran "git update" and afterward "git version" reported
2.14.2(3), but "git lfs version" still said 2.3.2.

I also uninstalled/reinstalled Git for Windows and LFS is still 2.3.2.

Steve

Re: [git-for-windows] [ANNOUNCE] Git for Windows 2.14.2(2)

[Steve Hoelzer]

On Thu, Oct 5, 2017 at 3:22 PM, Lars Schneider  wrote:
>
>> On 05 Oct 2017, at 22:02, Johannes Schindelin  
>> wrote:
>>
>> Dear Git users,
>>
>> It is my pleasure to announce that Git for Windows 2.14.2(2) is available 
>> from:
>>
>>   https://git-for-windows.github.io/
>>
>> Changes since Git for Windows v2.14.2 (September 26th 2017)
>>
>> New Features
>>
>>  * Comes with BusyBox v1.28.0pre.16467.b4c390e17.
>>  * Comes with Git LFS v2.3.2.
>
> Just a heads-up:
> Git LFS 2.3.2 contains a bug in the `git lfs clone` and `git lfs pull` calls:
> https://github.com/git-lfs/git-lfs/issues/2649
>
> I expect 2.3.3 to be out soonish.

It's out now.

Steve

Re: On a personal note

[Steve Hoelzer]

On Thu, Sep 3, 2015 at 5:00 AM, Johannes Schindelin
 wrote:
> Hey all,
>
> yes, it is true: since mid-August I am working for Microsoft. Over a
> year ago, I got into contact with the Visual Studio Online group at
> Microsoft, of which I am now a happy member. A large part of my mission
> is to improve the experience of Git for Windows. This is very exciting
> to me: I finally can focus pretty much full time on something that I
> could only address in my spare time previously.

This is great news for GfW users like me and git in general.
Congratulations, Johannes!

Steve
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] userdiff: support C# async methods and correct C# keywords

[Steve Hoelzer]

On Fri, Jun 6, 2014 at 12:34 PM, Junio C Hamano gits...@pobox.com wrote:
 Steve Hoelzer shoel...@gmail.com writes:

 instanceof() is listed as keywords, but there is no such thing (it is
 in Java, though); in C# we use typeof(), 'is', and 'as for similar
 purposes

 You would need to balance the quotes around as ;-)

Indeed. Doh.

 But reading the patch again after noticing that you have () after
 typeof but not after is/as, I am not sure if the change proposed
 here is even correct for the latter two.  I do not speal c-sharp, so
 I asked http://msdn.microsoft.com/en-us/library/cscsdfbt.aspx for
 some examples and here are what I found:

 Type t = typeof(ExampleClass)
 Base b = derived as Base;
 if (obj is MyObject) ...

 Unlike the control-flow keywords (e.g. do/while/for/...), do they
 typically appear at the beginning of lines?

No, I would never expect to see 'is' or 'as' at the beginning of a line.

 Isn't the purpose of these !^[ \t]* patterns to reject lines that
 begin with the language keywords that do not start functions, so
 listing a keyword that does not usually appear at the beginning of
 line looks like a churn that is not useful.

Not sure about the purpose of those lines, but I think you're correct.

Steve

 diff --git a/userdiff.c b/userdiff.c
 index fad52d6..96eda6c 100644
 --- a/userdiff.c
 +++ b/userdiff.c
 @@ -134,9 +134,9 @@ PATTERNS(cpp,
|[-+*/%^|=!]=|--|\\+\\+|=?|=?||\\|\\||::|-\\*?|\\.\\*),
  PATTERNS(csharp,
/* Keywords */
 -  !^[ 
 \t]*(do|while|for|if|else|instanceof|new|return|switch|case|throw|catch|using)\n
 +  !^[ 
 \t]*(do|while|for|foreach|if|else|typeof|is|as|new|return|switch|case|default|throw|try|catch|using)\n
/* Methods and constructors */
 -  ^[ 
 \t]*(((static|public|internal|private|protected|new|virtual|sealed|override|unsafe)[
  \t]+)*[][@.~_[:alnum:]]+[ \t]+[@._[:alnum:]]+[ \t]*\\(.*\\))[ \t]*$\n
 +  ^[ 
 \t]*(((static|public|internal|private|protected|new|virtual|sealed|override|unsafe|async)[
  \t]+)*[][@.~_[:alnum:]]+[ \t]+[@._[:alnum:]]+[ \t]*\\(.*\\))[ \t]*$\n
/* Properties */
^[ 
 \t]*(((static|public|internal|private|protected|new|virtual|sealed|override|unsafe)[
  \t]+)*[][@.~_[:alnum:]]+[ \t]+[@._[:alnum:]]+)[ \t]*$\n
/* Type definitions */
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] userdiff: support C# async methods and correct C# keywords

[Steve Hoelzer]

On Thu, Jun 5, 2014 at 5:59 PM, Junio C Hamano gits...@pobox.com wrote:
 Sup Yut Sum ch3co...@gmail.com writes:

 async is in C# 5.0
 foreach is in C# 1.0

 instanceof is in Java. The similar keywords are typeof, is, as in C# 1.0

 This one made me read it twice, until I realized you meant

 instanceof() is listed as keywords, but there is no such thing
 (it is in Java, though); in C# we use typeof() for similar
 purposes

The original email was a bit hard to parse. Junio's clarification left
out the C# keywords 'is' and 'as'. I suggest phrasing it like this:

instanceof() is listed as keywords, but there is no such thing (it is
in Java, though); in C# we use typeof(), 'is', and 'as for similar
purposes
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] environment: enable core.preloadindex by default

[Steve Hoelzer]

On Mon, Jun 2, 2014 at 3:01 PM, Karsten Blees karsten.bl...@gmail.com wrote:
 Git for Windows users may want to try core.fscache=true as well [1]. This 
 eliminates the UAC penalties for repositories located on the Windows system 
 drive [2].

 [1] https://github.com/msysgit/git/pull/94
 [2] https://code.google.com/p/msysgit/issues/detail?id=320

Thanks for the tip! I didn't know about fscache, but I'll definitely
give it a try. Is there a reason it is not turned on by default in Git
for Windows?

Steve
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] environment: enable core.preloadindex by default

[Steve Hoelzer]

There is consensus that the default should change because it will
benefit nearly all users (some just a little, but some a lot).
See [1] and replies.

[1]: 
http://git.661346.n2.nabble.com/git-status-takes-30-seconds-on-Windows-7-Why-tp7580816p7580853.html

Signed-off-by: Steve Hoelzer shoel...@gmail.com
---
 Documentation/config.txt | 4 ++--
 environment.c| 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1932e9b..4b3d965 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -613,9 +613,9 @@ core.preloadindex::
 +
 This can speed up operations like 'git diff' and 'git status' especially
 on filesystems like NFS that have weak caching semantics and thus
-relatively high IO latencies.  With this set to 'true', Git will do the
+relatively high IO latencies.  When enabled, Git will do the
 index comparison to the filesystem data in parallel, allowing
-overlapping IO's.
+overlapping IO's.  Defaults to true.

 core.createObject::
  You can set this to 'link', in which case a hardlink followed by
diff --git a/environment.c b/environment.c
index 5c4815d..1c686c9 100644
--- a/environment.c
+++ b/environment.c
@@ -71,7 +71,7 @@ unsigned long pack_size_limit_cfg;
 char comment_line_char = '#';

 /* Parallel index stat data preload? */
-int core_preload_index = 0;
+int core_preload_index = 1;

 /* This is set by setup_git_dir_gently() and/or git_default_config() */
 char *git_work_tree_cfg;
-- 
1.9.0.msysgit.0
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html