Re: [msysGit] Re: [PATCH bc/connect-plink] t5601-clone: remove broken and pointless check for plink.exe
On Thu, Aug 13, 2015 at 10:37 AM, Johannes Schindelin wrote: > Hi kusma, > > On 2015-08-12 13:58, Erik Faye-Lund wrote: >> On Wed, Aug 12, 2015 at 1:07 PM, Johannes Schindelin >> wrote: >>> >>> On 2015-08-11 22:51, Johannes Sixt wrote: >>>> Invoking plink requires special treatment, and we have support and even >>>> test cases for the commands 'plink' and 'tortoiseplink'. We also support >>>> .exe variants for these two and there is a test for 'plink.exe'. >>>> >>>> On Windows, however, where support for plink.exe would be relevant, the >>>> test case fails because it is not possible to execute a file with a .exe >>>> extension that is actually not a binary executable---it is a shell >>>> script in our test. We have to disable the test case on Windows. >>> >>> Oh how would I wish you were working on Git for Windows even *just* a bit >>> *with* me. At least I would wish for a more specific description of the >>> development environment, because it sure as hell is not anything anybody >>> can download and install as easily as Git for Windows' SDK. >>> >>> FWIW Git for Windows has this patch (that I wanted to contribute in due >>> time, what with being busy with all those tickets) to solve the problem >>> mentioned in your patch in a different way: >>> >>> https://github.com/git-for-windows/git/commit/2fff4b54a0d4e5c5e2e4638c9b0739d3c1ff1e45 >> >> Yuck. On Windows, it's the extension of a file that dictates what kind >> of file it is (and if it's executable or not), not the contents. > > Careful. If you continue along those lines, interactive rebase, `git add -p` > and all those wonderful scripts Git has will have to stop working. > > Because those scripts completely disagree with what you just said about > Windows if you think about it: *none* of them has an extension. > > I know that you do not mean this, of course, but that is the argument you > were making... ;-) > You should know better than to straw-man like that. I was not arguing to break any current functionality, but to not move further away from Windows' semantics. But if I would have, there's nothing that would stop us from renaming those scrips to *.sh, and let the filename dictate how to execute them. Or provide batch-files to wrap them. >> If we get a shell script written with the ".exe"-prefix, it's considered as >> an invalid executable by the system. > > And if we get a shell script without any `.exe` suffix, it is still > considered as an invalid executable by the system. Nope, it's considered an unknown file, not an executable at all. > And even if we tack on an `.sh` suffix (which is *not* in line with the way > Git works), it is *still* considered as an invalid executable by the system. That's not necessarily true; the Git for Windows installer (optionally, but on by default) registers /bin/sh as a file-handler for .sh files. Windows knows just fine how to execute them, unless the user opts out. But again, I was not arguing to patch git to not parse the shebang. -- 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: [msysGit] Re: [PATCH bc/connect-plink] t5601-clone: remove broken and pointless check for plink.exe
On Wed, Aug 12, 2015 at 1:07 PM, Johannes Schindelin wrote: > Hi Johannes, > > On 2015-08-11 22:51, Johannes Sixt wrote: >> Invoking plink requires special treatment, and we have support and even >> test cases for the commands 'plink' and 'tortoiseplink'. We also support >> .exe variants for these two and there is a test for 'plink.exe'. >> >> On Windows, however, where support for plink.exe would be relevant, the >> test case fails because it is not possible to execute a file with a .exe >> extension that is actually not a binary executable---it is a shell >> script in our test. We have to disable the test case on Windows. > > Oh how would I wish you were working on Git for Windows even *just* a bit > *with* me. At least I would wish for a more specific description of the > development environment, because it sure as hell is not anything anybody can > download and install as easily as Git for Windows' SDK. > > FWIW Git for Windows has this patch (that I wanted to contribute in due time, > what with being busy with all those tickets) to solve the problem mentioned > in your patch in a different way: > > https://github.com/git-for-windows/git/commit/2fff4b54a0d4e5c5e2e4638c9b0739d3c1ff1e45 Yuck. On Windows, it's the extension of a file that dictates what kind of file it is (and if it's executable or not), not the contents. If we get a shell script written with the ".exe"-prefix, it's considered as an invalid executable by the system. We should consider it the same way, otherwise we're on the path to user-experience schizophrenia. I'm not sure I consider this commit a step in the right direction. -- 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: [msysGit] [PATCH] compat/mingw: stubs for getpgid() and tcgetpgrp()
On Wed, Apr 15, 2015 at 8:29 PM, Johannes Sixt wrote: > Windows does not have process groups. It is, therefore, the simplest > to pretend that each process is in its own process group. Windows does have some concept of process groups, but probably not quite what you want: https://msdn.microsoft.com/en-us/library/windows/desktop/ms682083%28v=vs.85%29.aspx -- 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] systemd socket activation support
On Thu, Apr 2, 2015 at 8:09 AM, Shawn Landden wrote: > From: Shawn Landden > > v1.1: actually test... > > Signed-off-by: Shawn Landden > --- > daemon.c | 35 +++--- > git-daemon.service | 7 +++ > git-daemon.socket | 9 > sd-daemon.c| 132 > + > sd-daemon.h| 91 > 5 files changed, 268 insertions(+), 6 deletions(-) > create mode 100644 git-daemon.service > create mode 100644 git-daemon.socket > create mode 100644 sd-daemon.c > create mode 100644 sd-daemon.h > > diff --git a/daemon.c b/daemon.c > index 9ee2187..4677058 100644 > --- a/daemon.c > +++ b/daemon.c > @@ -5,6 +5,8 @@ > #include "strbuf.h" > #include "string-list.h" > > +#include "sd-daemon.c" > + You meant "sd-daemon.h", no? -- 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: t5539 broken under Mac OS X
On Tue, Jan 27, 2015 at 3:51 AM, Junio C Hamano wrote: > Erik Faye-Lund writes: > >> On Fri, Jan 16, 2015 at 1:04 AM, Junio C Hamano wrote: >>> Jeff King writes: >>> >>>> Exactly. I am happy to submit a patch, but I cannot think of any >>>> mechanisms besides: >>>> >>>> 1. Calling `id`, which I suspect is very not portable. >>>> >>>> 2. Writing a C program to check getuid(). That's portable for most >>>> Unixes. It looks like we already have a hacky wrapper on mingw that >>>> will always return "1". >>>> >>>> Is (2) too gross? >>> >>> Not overly gross compared to some existing test-*.c files, I would >>> say. >>> >>> I wondered what 'perl -e 'print $>' would say in mingw, and if that >>> is portable enough, though. >> >> $ perl -e 'print $>' >> 500 > > Thanks for a follow-up. > > Is "id -u" not useful over there? I ask because that is what is > used in the version tentatively queued on 'pu' for NOT_ROOT > prerequisite (the jk/sanity topic). It's pretty much the same thing: $ id -u 500 > The SANITY prerequisite in that topic needs to be replaced with the > one from Torsten that attempts to check what we want to know in a > more direct way; i.e. "after making a directory or a file read-only, > does the filesystem really honours that, or lets us clobber?" is > what we need to know to skip some tests, and we should check that, > instead of "is / writable by us?" or "are we root?". $ test -w / && echo yes yes $ mkdir foo && chmod a= foo $ test -w w && echo yes $ rm -r foo rm: directory `foo' is write protected; descend into it anyway? n $ rm -r foo < /dev/null $ ls -la foo ls: foo: No such file or directory $ So, Windows does only kind-of respect read-only flags. Dunno if this tells you something useful, though. -- 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: t5539 broken under Mac OS X
On Fri, Jan 16, 2015 at 1:04 AM, Junio C Hamano wrote: > Jeff King writes: > >> Exactly. I am happy to submit a patch, but I cannot think of any >> mechanisms besides: >> >> 1. Calling `id`, which I suspect is very not portable. >> >> 2. Writing a C program to check getuid(). That's portable for most >> Unixes. It looks like we already have a hacky wrapper on mingw that >> will always return "1". >> >> Is (2) too gross? > > Not overly gross compared to some existing test-*.c files, I would > say. > > I wondered what 'perl -e 'print $>' would say in mingw, and if that > is portable enough, though. $ perl -e 'print $>' 500 -- 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] win32: syslog: prevent potential realloc memory leak
Sorry for very late reply. I had a bug in my mail rules that caused
this email to skip my inbox. That should be fixed now.
On Mon, Dec 15, 2014 at 7:11 PM, Junio C Hamano wrote:
> Arjun Sreedharan writes:
>
>> use a temporary variable to free the memory in case
>> realloc() fails.
>>
>> Signed-off-by: Arjun Sreedharan
>> ---
>> compat/win32/syslog.c | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/compat/win32/syslog.c b/compat/win32/syslog.c
>> index d015e43..3409e43 100644
>> --- a/compat/win32/syslog.c
>> +++ b/compat/win32/syslog.c
>> @@ -16,7 +16,7 @@ void openlog(const char *ident, int logopt, int facility)
>> void syslog(int priority, const char *fmt, ...)
>> {
>> WORD logtype;
>> - char *str, *pos;
>> + char *str, *str_temp, *pos;
>> int str_len;
>> va_list ap;
>>
>> @@ -43,9 +43,11 @@ void syslog(int priority, const char *fmt, ...)
>> va_end(ap);
>>
>> while ((pos = strstr(str, "%1")) != NULL) {
>> + str_temp = str;
>> str = realloc(str, ++str_len + 1);
>> if (!str) {
>> warning("realloc failed: '%s'", strerror(errno));
>> + free(str_temp);
>> return;
>> }
>
> Hmm, the original, 088d8802 (mingw: implement syslog, 2010-11-04),
> that introduced the special casing for %1, says:
>
> Syslog does not usually exist on Windows, so implement our own
> using Window's ReportEvent mechanism.
>
> Strings containing "%1" gets expanded into them selves by
> ReportEvent, resulting in an unreadable string. "%2" and above
> is not a problem. Unfortunately, on Windows an IPv6 address can
> contain "%1", so expand "%1" to "% 1" before reporting. "%%1" is
> also a problem for ReportEvent, but that string cannot occur in
> an IPv6 address.
>
> It is unclear why it says '"%2" and above is not a problem' to me.
> Is that because they expand to something not "an unreadable string",
> or is that because in the original developer's testing only "%1" was
> observed?
%2 would expand to the first argument (this is a varargs-type
interface), but such an argument does not exist, so no expansion is
being performed.
> It also says "%%1" is a problem, and it does not occur in
> an IPv6 address, but that would suggest that every time a new caller
> is added to syslog(), this imitation of syslog() can break, as there
> is nothing that says the new caller must be reporting something
> about an IP address. Perhaps this loop should cleanse what it
> passes to ReportEvent() a bit more aggressively by expanding all "%"
> to "%-sp" or something)?
This suspicion is somewhat justified; I only did %1 because that was
the only one that had any theoretical way of being hit at the time the
patch was written. However, a quick test reveals that we currently
expand "%%1" to "%% 1", which is not problematic. And that makes sense
from looking at the code also.
However, removing the hunk and running the code reveals that Windows
no longer does that ugly recursive expansion. I'm sure it did when I
wrote the code, because I did test it. So it seems at least Windows
8.1 is less insane about this.
> Regardless of that funny %1 business, I notice in
>
>
> http://msdn.microsoft.com/en-us/library/windows/desktop/aa363679%28v=vs.85%29.aspx
>
> that each element of lpStrings array that is passed to ReportEvent()
> is limited to 32k or so. Wouldn't it make it a lot simpler if we
> removed the dynamic allocation and use a fixed sized 32k buffer here
> (and truncate the result as necessary)? That would make the "leak"
> disappear automatically.
That's a very good point. Yes, I think that makes more sense.
> Having said all that, if we were to still go with the current code
> structure, "str_temp" should be scoped inside the loop, as there is
> no need to make it available to the remainder of the function, I
> think. Also writing this way may make the intention more clear.
>
> while (...) {
> char *new_str = realloc(str, ...);
> if (!new_str) {
> free(str);
> return;
> }
> memmove(... to shuffle ...);
>
> And after starting to write the above, I notice that the current
> code around realloc may be completely bogus. It goes like this:
>
> while ((pos = strstr(str, "..."))) {
> str = realloc(str, ...);
> if (!str) { warn and bail; }
> memmove(pos + 1, pos + 1, ...);
> pos[1] = ' ';
> }
>
> If realloc() really allocated a new string, then pos that points
> into the original str has no relation to the reallocated str, so
> memmove() is not shuffling the string to make room for the SP in the
> string that will be given to ReportEvent() at all, no? This seems
> to be a bug introduced by 2a6b149c (mingw: avoid using strbuf in
> syslog
Re: [PATCH] wincred: fix get credential if username has @
Sorry for the extremely delayed reply, I had a bug in my mail-filters.
Hopefully fixed now.
On Wed, Nov 19, 2014 at 11:41 PM, Junio C Hamano wrote:
> Aleksey Vasenev writes:
>
>>> To: git@vger.kernel.org
>>> Cc: Junio C Hamano , Aleksey Vasenev
>
> Sorry, but I am hardly qualified to review this one, especially
> without any log message that explains what breaks and how it breaks
> with the current code, which may lead the reader to understand how
> the updated code fixes the issue. Cc'ing me does not help us very
> much.
>
> $ git shortlog --no-merges -n contrib/credential/wincred/
>
> gives me a few names who may be able to give us some inputs, so I'll
> Cc them.
>
> Thanks.
I noticed the breakage myself around the same time, and posted about it here:
https://groups.google.com/d/msg/msysgit/YVuCqmwwRyY/HULHj5OoE88J
Unfortunately, it stopped there.
>> Signed-off-by: Aleksey Vasenev
>> ---
>> .../credential/wincred/git-credential-wincred.c| 25
>> +++---
>> 1 file changed, 22 insertions(+), 3 deletions(-)
>>
>> diff --git a/contrib/credential/wincred/git-credential-wincred.c
>> b/contrib/credential/wincred/git-credential-wincred.c
>> index a1d38f0..0061340 100644
>> --- a/contrib/credential/wincred/git-credential-wincred.c
>> +++ b/contrib/credential/wincred/git-credential-wincred.c
>> @@ -111,14 +111,23 @@ static void write_item(const char *what, LPCWSTR wbuf,
>> int wlen)
>> * Match an (optional) expected string and a delimiter in the target string,
>> * consuming the matched text by updating the target pointer.
>> */
>> -static int match_part(LPCWSTR *ptarget, LPCWSTR want, LPCWSTR delim)
>> +
>> +static LPCWSTR wcsstr_last(LPCWSTR str, LPCWSTR find)
>> +{
>> + LPCWSTR res = NULL, pos;
>> + for (pos = wcsstr(str, find); pos; pos = wcsstr(pos + 1, find))
>> + res = pos;
>> + return res;
>> +}
>> +
Ugh, there's no wcsrstr? I guess this is a reasonable way to emulate it...
>> +static int match_part_with_last(LPCWSTR *ptarget, LPCWSTR want, LPCWSTR
>> delim, int last)
>> {
>> LPCWSTR delim_pos, start = *ptarget;
>> int len;
>>
>> /* find start of delimiter (or end-of-string if delim is empty) */
>> if (*delim)
>> - delim_pos = wcsstr(start, delim);
>> + delim_pos = last ? wcsstr_last(start, delim) : wcsstr(start,
>> delim);
>> else
>> delim_pos = start + wcslen(start);
>>
>> @@ -138,6 +147,16 @@ static int match_part(LPCWSTR *ptarget, LPCWSTR want,
>> LPCWSTR delim)
>> return !want || (!wcsncmp(want, start, len) && !want[len]);
>> }
>>
>> +static int match_part(LPCWSTR *ptarget, LPCWSTR want, LPCWSTR delim)
>> +{
>> + return match_part_with_last(ptarget, want, delim, 0);
>> +}
>> +
>> +static int match_part_last(LPCWSTR *ptarget, LPCWSTR want, LPCWSTR delim)
>> +{
>> + return match_part_with_last(ptarget, want, delim, 1);
>> +}
>> +
>> static int match_cred(const CREDENTIALW *cred)
>> {
>> LPCWSTR target = cred->TargetName;
>> @@ -146,7 +165,7 @@ static int match_cred(const CREDENTIALW *cred)
>>
>> return match_part(&target, L"git", L":") &&
>> match_part(&target, protocol, L"://") &&
>> - match_part(&target, wusername, L"@") &&
>> + match_part_last(&target, wusername, L"@") &&
>> match_part(&target, host, L"/") &&
>> match_part(&target, path, L"");
>> }
Looks reasonable enough to me.
Acked-by: Erik Faye-Lund
--
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: [msysGit] [PATCH 01/14] MINGW: compat/mingw.h: do not attempt to redefine lseek on mingw-w64
On Wed, Oct 8, 2014 at 8:00 PM, Marat Radchenko wrote: > Unlike MinGW, MinGW-W64 has lseek already properly defined in io.h. > > Signed-off-by: Marat Radchenko > Acked-by: Eric Faye-Lund I spell my name with a K, "Erik Faye-Lund". -- 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 v2 2/2] wincred: improve compatibility with windows versions
On Thu, Jan 10, 2013 at 1:10 PM, Karsten Blees wrote:
> static int match_cred(const CREDENTIALW *cred)
> {
> - return (!wusername || !wcscmp(wusername, cred->UserName)) &&
> - match_attr(cred, L"git_protocol", protocol) &&
> - match_attr(cred, L"git_host", host) &&
> - match_attr(cred, L"git_path", path);
> + LPCWSTR target = cred->TargetName;
> + if (wusername && wcscmp(wusername, cred->UserName))
> + return 0;
> +
> + return match_part(&target, L"git", L":") &&
> + match_part(&target, protocol, L"://") &&
> + match_part(&target, wusername, L"@") &&
This seems to have broken credentials with '@' in the username, which
isn't all that unusual these days... :(
--
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: Location of git config on Windows
On Mon, Aug 18, 2014 at 7:05 PM, Daniel Corbe wrote: > Erik Faye-Lund writes: > >> On Mon, Aug 18, 2014 at 5:47 PM, Erik Faye-Lund wrote: >>> On Mon, Aug 18, 2014 at 5:40 PM, Daniel Corbe wrote: >>>> >>>> Erik Faye-Lund writes: >>>> >>>>> Or you could just restart your shell when you disconnect... >>>> >>>> Well I'm not that daft. I tried that and if it had resolved my problem >>>> I wouldn't have posted. >>> >>> Hm, but isn't that what Karsten explains in his last paragraph? What >>> shell are you running msys or cmd? >> >> Our /etc/profile does this: >> >> https://github.com/msysgit/msysgit/blob/master/etc/profile#L38 >> >> ...however, our git-wrapper only does this: >> >> https://github.com/msysgit/msysgit/blob/master/src/git-wrapper/git-wrapper.c#L71 >> >> So yeah, we don't seem to actually check if %HOMEDRIVE%%HOMEPATH% >> exists. Perhaps fixing this is the right thing to do then? Since the >> git-wrapper is run for *every* invokation of git, you wouldn't even >> have to restart the shell in this case. > > But again, restarting the shell doesn't fix the problem. > Not for cmd, no. But for Git Bash, it should. -- 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: Location of git config on Windows
On Mon, Aug 18, 2014 at 5:47 PM, Erik Faye-Lund wrote: > On Mon, Aug 18, 2014 at 5:40 PM, Daniel Corbe wrote: >> >> Erik Faye-Lund writes: >> >>> Or you could just restart your shell when you disconnect... >> >> Well I'm not that daft. I tried that and if it had resolved my problem >> I wouldn't have posted. > > Hm, but isn't that what Karsten explains in his last paragraph? What > shell are you running msys or cmd? Our /etc/profile does this: https://github.com/msysgit/msysgit/blob/master/etc/profile#L38 ...however, our git-wrapper only does this: https://github.com/msysgit/msysgit/blob/master/src/git-wrapper/git-wrapper.c#L71 So yeah, we don't seem to actually check if %HOMEDRIVE%%HOMEPATH% exists. Perhaps fixing this is the right thing to do then? Since the git-wrapper is run for *every* invokation of git, you wouldn't even have to restart the shell in this case. -- 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: Location of git config on Windows
On Mon, Aug 18, 2014 at 5:40 PM, Daniel Corbe wrote: > > Erik Faye-Lund writes: > >> Or you could just restart your shell when you disconnect... > > Well I'm not that daft. I tried that and if it had resolved my problem > I wouldn't have posted. Hm, but isn't that what Karsten explains in his last paragraph? What shell are you running msys or cmd? -- 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: Location of git config on Windows
On Mon, Aug 18, 2014 at 5:14 PM, Daniel Corbe wrote: > > Karsten Blees writes: > >> Am 18.08.2014 00:01, schrieb Erik Faye-Lund: >>> On Sun, Aug 17, 2014 at 10:18 PM, Daniel Corbe wrote: >>>> >>>> I installed git on my Windows machine while it was connected to my >>>> corporate network. It picked up on that fact and used a mapped drive to >>>> store its configuration file. >>>> >>>> As a result, I cannot currently use git when disconnected from my >>>> network. It throws the following error message: fatal: unable to access >>>> 'Z:\/.config/git/config': Invalid argument >>>> >>>> Obviously this value is stored in the registry somewhere because I made >>>> an attempt to uninstall and reinstall git with the same results. >>>> >>>> Can someone give me some guidance here? >>> >>> Git looks for the per-user configuration in $HOME/.gitconfig, and if >>> $HOME is not set, it falls back to $HOMEDIR/$HOMEPATH/.gitconfig. My >>> guess would be some of these environment variables are incorrectly set >>> on your system. >> >> To be precise, git checks if %HOME% is set _and_ the directory exists before >> falling back to %HOMEDRIVE%%HOMEPATH%. >> >> If %HOMEDRIVE%%HOMEPATH% isn't set or the directory doesn't exist either, it >> falls back to %USERPROFILE%, which is always local (C:/Users/), >> even >> if disconnected from the network (at least that's how its supposed to be). >> >> > > Awesome! Thanks for the advice. > > %HOMEDRIVE% and %HOMEPATH% are indeed set by my system and point to an > (often disconnected) network drive. I manually forced %HOME% to > %USERPROFILE% and it works like a charm now. > > I would argue that on Windows %USERPROFILE% should be checked first (or > at least after %HOME%). Why? Then people won't be able to have their config files on network-shares, no? I think a somewhat better approach would be to resolve the home directory lazily, unless %HOME% is set. That way we can check that %HOMEDRIVE%%HOMEPATH% actually exists as it's being accessed. Or you could just restart your shell when you disconnect... -- 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: Location of git config on Windows
On Sun, Aug 17, 2014 at 10:18 PM, Daniel Corbe wrote: > > I installed git on my Windows machine while it was connected to my > corporate network. It picked up on that fact and used a mapped drive to > store its configuration file. > > As a result, I cannot currently use git when disconnected from my > network. It throws the following error message: fatal: unable to access > 'Z:\/.config/git/config': Invalid argument > > Obviously this value is stored in the registry somewhere because I made > an attempt to uninstall and reinstall git with the same results. > > Can someone give me some guidance here? Git looks for the per-user configuration in $HOME/.gitconfig, and if $HOME is not set, it falls back to $HOMEDIR/$HOMEPATH/.gitconfig. My guess would be some of these environment variables are incorrectly set on your system. -- 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: Undefined reference to __builtin_ctzll
On Wed, Aug 13, 2014 at 10:36 AM, Радослав Йовчев wrote:
> Dear GIT community,
>
>
> I found myself in situation where I had to install GIT on Debian 3.1
> sarge. It comes with GCC 3.3.5. I tried to built from source but the
> libgcc was not providing the ctzll function, thus I decided to put an
> implementation.
>
>
> I do not know how to post and do a nice patch (and whether somebody
> will care), but I guess, for reference I can post my solution. Just
> appended in compat/strlcpy.c the following:
>
>
> int __builtin_ctzll (long long x)
> {
> int i;
> for (i = 0; i < 8 * sizeof (long long); ++i)
> if (x & ((long long) 1 << i))
> break;
> return i;
> }
>
>
> I guess that some ifdef macro can be used to detect compiler version
> or missing __builtin_ctzll.
It seems __builtin_ctzll is only available in GCC 3.4.0 and beyond, so
I think a better fix is something like this:
diff --git a/ewah/ewok.h b/ewah/ewok.h
index 43adeb5..2700fa3 100644
--- a/ewah/ewok.h
+++ b/ewah/ewok.h
@@ -47,7 +47,7 @@ static inline uint32_t ewah_bit_popcount64(uint64_t x)
return (x * 0x0101010101010101ULL) >> 56;
}
-#ifdef __GNUC__
+#if (__GNUC__ == 3 && __GNUC_MINOR__ >= 4) || __GNUC__ > 3
#define ewah_bit_ctz64(x) __builtin_ctzll(x)
#else
static inline int ewah_bit_ctz64(uint64_t x)
--
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: [msysGit] [PATCH 4/6] t4210: skip command-line encoding tests on mingw
On Thu, Jul 17, 2014 at 5:37 PM, Stepan Kasal wrote: > From: Pat Thoyts > > On Windows the application command line is provided as unicode and in > mingw-git we convert that to utf-8. So these tests that require a iso-8859-1 > input are being subverted by the encoding transformations we perform and > should be skipped. > > Signed-off-by: Pat Thoyts > Signed-off-by: Stepan Kasal > --- > t/t4210-log-i18n.sh | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/t/t4210-log-i18n.sh b/t/t4210-log-i18n.sh > index 52a7472..9110404 100755 > --- a/t/t4210-log-i18n.sh > +++ b/t/t4210-log-i18n.sh > @@ -34,7 +34,7 @@ test_expect_success 'log --grep searches in log output > encoding (utf8)' ' > test_cmp expect actual > ' > > -test_expect_success 'log --grep searches in log output encoding (latin1)' ' > +test_expect_success NOT_MINGW 'log --grep searches in log output encoding > (latin1)' ' > cat >expect <<-\EOF && > latin1 > utf8 > @@ -43,7 +43,7 @@ test_expect_success 'log --grep searches in log output > encoding (latin1)' ' > test_cmp expect actual > ' > > -test_expect_success 'log --grep does not find non-reencoded values (utf8)' ' > +test_expect_success NOT_MINGW 'log --grep does not find non-reencoded values > (utf8)' ' Perhaps these checks would be more readable a few years in the future, if we make a separate capability along the lines of NON_UNICODE_LOCALE? -- 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: No fchmod() under msygit - Was: Re: [PATCH 00/14] Add submodule test harness
On Wed, Jul 9, 2014 at 10:00 PM, Eric Wong wrote: > Torsten Bögershausen wrote: >> (And why is it "& 0" and not "& 0777") > > This is to preserve the uncommon sticky/sgid/suid bits. Probably not > needed, but better to keep as much intact as possible. > >> Can we avoid the fchmod() all together ? > > For single-user systems, sure. > > For multi-user systems with git-imap-send users and passwords in > $GIT_CONFIG, I suggest not. You're saying this as if Windows is a single-user system. It's not, but it uses ACLs rather than POSIX permissions to manage file-system permissions. So far we've opted to ignore ACLs in Git for Windows, though. -- 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 7/7] determine_author_info: stop leaking name/email
On Mon, Jun 23, 2014 at 11:28 AM, Eric Sunshine wrote:
> On Wed, Jun 18, 2014 at 4:36 PM, Jeff King wrote:
>> When we get the author name and email either from an
>> existing commit or from the "--author" option, we create a
>> copy of the strings. We cannot just free() these copies,
>> since the same pointers may also be pointing to getenv()
>> storage which we do not own.
>>
>> Instead, let's treat these the same way as we do the date
>> buffer: keep a strbuf to be released, and point the bare
>> pointers at the strbuf.
>>
>> Signed-off-by: Jeff King
>> ---
>> builtin/commit.c | 20 +---
>> 1 file changed, 13 insertions(+), 7 deletions(-)
>>
>> diff --git a/builtin/commit.c b/builtin/commit.c
>> index 62abee0..72beb7f 100644
>> --- a/builtin/commit.c
>> +++ b/builtin/commit.c
>> @@ -546,16 +546,20 @@ static void strbuf_add_pair(struct strbuf *buf, const
>> struct pointer_pair *p)
>> strbuf_add(buf, p->begin, p->end - p->begin);
>> }
>>
>> -static char *xmemdupz_pair(const struct pointer_pair *p)
>> +static char *set_pair(struct strbuf *buf, struct pointer_pair *p)
>> {
>> - return xmemdupz(p->begin, p->end - p->begin);
>> + strbuf_reset(buf);
>> + strbuf_add_pair(buf, p);
>> + return buf->buf;
>> }
>>
>> static void determine_author_info(struct strbuf *author_ident)
>> {
>> char *name, *email, *date;
>> struct ident_split author;
>> - struct strbuf date_buf = STRBUF_INIT;
>> + struct strbuf name_buf = STRBUF_INIT,
>> + mail_buf = STRBUF_INIT,
>
> nit: The associated 'char *' variable is named "email", so perhaps
> s/mail_buf/email_buf/g
>
>> + date_buf = STRBUF_INIT;
>>
>> name = getenv("GIT_AUTHOR_NAME");
>> email = getenv("GIT_AUTHOR_EMAIL");
>> @@ -572,8 +576,8 @@ static void determine_author_info(struct strbuf
>> *author_ident)
>> if (split_ident_line(&ident, a, len) < 0)
>> die(_("commit '%s' has malformed author line"),
>> author_message);
>>
>> - name = xmemdupz_pair(&ident.name);
>> - email = xmemdupz_pair(&ident.mail);
>> + name = set_pair(&name_buf, &ident.name);
>> + email = set_pair(&mail_buf, &ident.mail);
>> if (ident.date.begin) {
>> strbuf_reset(&date_buf);
>> strbuf_addch(&date_buf, '@');
>> @@ -589,8 +593,8 @@ static void determine_author_info(struct strbuf
>> *author_ident)
>>
>> if (split_ident_line(&ident, force_author,
>> strlen(force_author)) < 0)
>> die(_("malformed --author parameter"));
>> - name = xmemdupz_pair(&ident.name);
>> - email = xmemdupz_pair(&ident.mail);
>> + name = set_pair(&name_buf, &ident.name);
>> + email = set_pair(&mail_buf, &ident.mail);
>
> Does the code become too convoluted with these changes? You're now
> maintaining three 'char *' variables in parallel with three strbuf
> variables. Is it possible to drop the 'char *' variables and just pass
> the .buf member of the strbufs to fmt_ident()?
>
> Alternately, you also could solve the leaks by having an envdup() helper:
>
> static char *envdup(const char *s)
> {
> const char *v = getenv(s);
> return v ? xstrdup(v) : NULL;
> }
>
> ...
> name = envdup("GIT_AUTHOR_NAME");
> email = envdup("GIT_AUTHOR_EMAIL");
> ...
>
> And then just free() 'name' and 'email' normally.
This approach has the added benefit of fixing the case where getenv
uses a static buffer, like POSIX allows.
--
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 v2 1/3] add strnncmp() function
On Tue, Jun 17, 2014 at 9:34 AM, Jeremiah Mahler wrote:
> Add a strnncmp() function which behaves like strncmp() except it takes
> the length of both strings instead of just one. It behaves the same as
> strncmp() up to the minimum common length between the strings. When the
> strings are identical up to this minimum common length, the length
> difference is returned.
>
> Signed-off-by: Jeremiah Mahler
> ---
> strbuf.c | 9 +
> strbuf.h | 2 ++
> 2 files changed, 11 insertions(+)
>
> diff --git a/strbuf.c b/strbuf.c
> index ac62982..4eb7954 100644
> --- a/strbuf.c
> +++ b/strbuf.c
> @@ -600,3 +600,12 @@ char *xstrdup_tolower(const char *string)
> result[i] = '\0';
> return result;
> }
> +
> +int strnncmp(const char *a, int len_a, const char *b, int len_b)
> +{
> + int min_len = (len_a < len_b) ? len_a : len_b;
> + int cmp = strncmp(a, b, min_len);
> + if (cmp)
> + return cmp;
> + return (len_a - len_b);
> +}
Using a name that sounds like it's from the stdlib makes me cringe a
little bit. Names that start with "str" reserved for stdlib[1][2], but
we already ignore this for strbuf (and perhaps some other functions).
However, in this case it doesn't seem *that* unlikely that we might
collide with some stdlib-extensions.
[1]:
http://pubs.opengroup.org/onlinepubs/007904975/functions/xsh_chap02_02.html#tag_02_02_02
[2]: http://www.gnu.org/software/libc/manual/html_node/Reserved-Names.html
--
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: [msysGit] Multi-line commit message changed to a single one
On Fri, Jun 13, 2014 at 1:44 PM, wrote: > Hi, > > Here is a simple script to show my problem. I don't know if it's a > wrong operation (from me), a wrong configuration/parameter, or either a bug, > but I don't get what I expected. > From a git repo (#1), I created a commit message containing two lines. > Then I created a patch (git format-patch) and copy/paste it to another git > repo (#2). When I import this patch (git am), the commit message is modified > and both lines are "merged" to a single one. > Screenshot providen (repo1 : two separated lines, repo2 : a single line) > with .sh script to reproduce the whole test-case. > > If someone could tell me what is wrong... Thanks in advance This isn't unique to Git for Windows, I just tested on Linux and the same occurs. What seems to happen is that git format-patch puts both lines in the subject of the e-mail, as you can find described here: https://www.kernel.org/pub/software/scm/git/docs/git-format-patch.html "By default, the subject of a single patch is "[PATCH] " followed by the concatenation of lines from the commit message up to the first blank line (see the DISCUSSION section of git-commit(1))." So, this is entirely intentional, it seems. -- 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 v2] Add a Windows-specific fallback to getenv("HOME");
On Thu, Jun 5, 2014 at 11:40 AM, Karsten Blees wrote:
> Am 05.06.2014 10:03, schrieb Stepan Kasal:
>> From: Johannes Schindelin
>> Date: Wed, 2 Jun 2010 00:41:33 +0200
>>
>> If HOME is not set, use $HOMEDRIVE$HOMEPATH
>>
>> Signed-off-by: Johannes Schindelin
>> Signed-off-by: Stepan Kasal
>> ---
>>
>> Hello Karsten,
>> thanks for your explanation. There are more things to be done, but
>> I hope you can ack this patch as a step forward.
>>
>
> No, not really. Its sure better than introducing a special
> get_home_directory(), but it still increases the diff between upstream and
> msysgit rather than reducing it. The main critique points still remain:
>
> * $HOME is usually set up correctly before calling git, so this is
> essentially dead code (just checked, portable git's git-bash.bat and
> git-cmd.bat also do this correctly)
What about when tools like TortoiseGit and Git Extensions call git?
We're not guaranteed that they did the $HOME-dance, are we?
> * even if $HOME was empty, git should setenv("HOME") so that child processes
> can benefit from it (similar to TMPDIR and TERM in current msysgit's
> mingw_startup()). Not setting $HOME because it may hypothetically break child
> processes is a very weak argument, as we always did set $HOME in etc/profile
> (since the initial version back in 2007).
>
> * no fallback to $USERPROFILE doesn't work with diconnected home share
>
> If you really have time to spare, I suggest you focus on getting the Unicode
> patches upstream so that we can progress from there (e.g. move $HOME setup to
> mingw_startup() so that we can get rid of redundant logic in etc/profile,
> git-wrapper, git-bash.bat, git-cmd.bat etc.).
Perhaps we can patch up the upstream to better match Git for Windows
without upstreaming the Unicode patches? Don't get me wrong; I think
upstreaming them is a good idea, but in case time is lacking...
--
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: [msysGit] Re: [PATCH] Add a Windows-specific fallback to getenv("HOME");
On Wed, Jun 4, 2014 at 5:14 PM, Johannes Schindelin
wrote:
> Hi Erik,
>
> On Wed, 4 Jun 2014, Erik Faye-Lund wrote:
>
>> On Wed, Jun 4, 2014 at 3:47 PM, Duy Nguyen wrote:
>> > On Wed, Jun 4, 2014 at 6:47 PM, Stepan Kasal wrote:
>> >> @@ -133,7 +133,7 @@ char *git_path(const char *fmt, ...)
>> >> void home_config_paths(char **global, char **xdg, char *file)
>> >> {
>> >> char *xdg_home = getenv("XDG_CONFIG_HOME");
>> >> - char *home = getenv("HOME");
>> >> + const char *home = get_home_directory();
>> >> char *to_free = NULL;
>> >>
>> >> if (!home) {
>> >
>> > Just checking. Instead of replace the call sites, can we check and
>> > setenv("HOME") if it's missing instead? MinGW port already replaces
>> > main(). Extra initialization should not be a problem. I feel
>> > "getenv("HOME")" a tiny bit more familiar than get_home_directory(),
>> > but that's really weak argument as the number of call sites has not
>> > increased in 4 years.
>>
>> Yeah. But we already set %HOME% to %HOMEDRIVE%%HOMEPATH% in
>> /etc/profile, git-cmd.bat, gitk.cmd *and* git-wrapper... Do we really
>> need one more place?
>>
>> It seems some of these could be dropped...
>
> No. Git is not always called through Bash or the git-wrapper,
> unfortunately.
I'm aware of that. But you said in a previous e-mail that e.g putty
got confused when we set HOME. How is this a problem for git.exe, but
not when we set it in the shell?
--
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: [msysGit] Re: [PATCH] Add a Windows-specific fallback to getenv("HOME");
On Wed, Jun 4, 2014 at 3:47 PM, Duy Nguyen wrote:
> On Wed, Jun 4, 2014 at 6:47 PM, Stepan Kasal wrote:
>> @@ -133,7 +133,7 @@ char *git_path(const char *fmt, ...)
>> void home_config_paths(char **global, char **xdg, char *file)
>> {
>> char *xdg_home = getenv("XDG_CONFIG_HOME");
>> - char *home = getenv("HOME");
>> + const char *home = get_home_directory();
>> char *to_free = NULL;
>>
>> if (!home) {
>
> Just checking. Instead of replace the call sites, can we check and
> setenv("HOME") if it's missing instead? MinGW port already replaces
> main(). Extra initialization should not be a problem. I feel
> "getenv("HOME")" a tiny bit more familiar than get_home_directory(),
> but that's really weak argument as the number of call sites has not
> increased in 4 years.
Yeah. But we already set %HOME% to %HOMEDRIVE%%HOMEPATH% in
/etc/profile, git-cmd.bat, gitk.cmd *and* git-wrapper... Do we really
need one more place?
It seems some of these could be dropped...
--
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/5] progress.c: replace signal() with sigaction()
On Wed, May 28, 2014 at 10:19 AM, David Kastrup wrote:
> Chris Packham writes:
>
>> On 28/05/14 18:14, Jeremiah Mahler wrote:
>>> From signal(2)
>>>
>>> The behavior of signal() varies across UNIX versions, and has also var‐
>>> ied historically across different versions of Linux. Avoid its use:
>>> use sigaction(2) instead. See Portability below.
>>
>> Minor nit. The last sentence applies to the man page you're quoting and
>> doesn't really make sense when viewed in the context of this commit
>> message. Same applies to other patches in this series.
>>
>>>
>>> Replaced signal() with sigaction() in progress.c
>>>
>>> Signed-off-by: Jeremiah Mahler
>>> ---
>>> progress.c | 6 +-
>>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/progress.c b/progress.c
>>> index 261314e..24df263 100644
>>> --- a/progress.c
>>> +++ b/progress.c
>>> @@ -66,8 +66,12 @@ static void set_progress_signal(void)
>>> static void clear_progress_signal(void)
>>> {
>>> struct itimerval v = {{0,},};
>>> +struct sigaction sa;
>>> +
>>> +memset(&sa, 0, sizeof(sa));
>>> +sa.sa_handler = SIG_IGN;
>>
>> A C99 initialiser here would save the call to memset. Unfortunately
>> Documentation/CodingGuidelines is fairly clear on not using C99
>> initialisers, given the fact we're now at git 2.0 maybe it's time to
>> revisit this policy?
>
> If I look at the initialization of v in the context immediately above
> the new code, it would appear that somebody already revisited this
> policy.
Huh, the initialization of v doesn't use C99-features...?
--
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 0/5] replace signal() with sigaction()
On Wed, May 28, 2014 at 10:11 AM, Chris Packham wrote:
> On 28/05/14 19:40, Johannes Sixt wrote:
>> Am 5/28/2014 8:14, schrieb Jeremiah Mahler:
>>> From signal(2)
>>>
>>> The behavior of signal() varies across UNIX versions, and has also var‐
>>> ied historically across different versions of Linux. Avoid its use:
>>> use sigaction(2) instead. See Portability below.
>>>
>>> This patch set replaces calls to signal() with sigaction() in all files
>>> except sigchain.c. sigchain.c is a bit more complicated than the others
>>> and will be done in a separate patch.
>>
>> In compat/mingw.c we have:
>>
>> int sigaction(int sig, struct sigaction *in, struct sigaction *out)
>> {
>> if (sig != SIGALRM)
>> return errno = EINVAL,
>> error("sigaction only implemented for SIGALRM");
>> if (out != NULL)
>> return errno = EINVAL,
>> error("sigaction: param 3 != NULL not implemented");
>>
>> timer_fn = in->sa_handler;
>> return 0;
>> }
>>
>> Notice "only implemented for SIGALRM". Are adding the missing signals
>> somewhere (here or in a later patch)?
>>
>
> * note: not a windows/mingw programmer *
>
> Will the ones setting SIG_IGN be OK? Presumably we won't get these
> signals on windows anyway so we're already getting what we want.
We'll still emit an useless error unless someone cooks up a fix.
--
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/RFC] clean: add a flag to back up cleaned files
On Tue, May 27, 2014 at 6:37 PM, Jeff King wrote:
> On Tue, May 27, 2014 at 04:17:34PM +0200, Erik Faye-Lund wrote:
>
>> The combination of "git clean" and fat fingers can some times cause
>> data-loss, which can be frustrating.
>>
>> So let's add a flag that imports the files to be deleted into the
>> object-database, in a way similar to what git-stash does. Maintain
>> a reflog of the previously backed up clean-runs.
>>
>> Signed-off-by: Erik Faye-Lund
>> ---
>> I've had a similar patch laying around for quite a while, but since
>> f538a91 ("git-clean: Display more accurate delete messages"), this
>> patch is a lot less nasty than before. So here you go, perhaps
>> someone else has fat fingers and hate to lose work?
>
> I've definitely considered doing something like this before (and for
> "git reset --hard"). My biggest concern would be poor performance in
> some cases. But since it's optional, and one can presumably override it
> with --no-backup for a specific large cleanup, it would not hurt anybody
> who does not want to play with it.
I also made it opt-in rather than opt-out by default. Perhaps it
shouldn't be, though - dunno.
>> + /* load HEAD into the index */
>> +
>> + tree = parse_tree_indirect(sha1);
>> + if (!tree)
>> + die(_("Failed to unpack tree object %s"), sha1);
>> +
>> + parse_tree(tree);
>> + init_tree_desc(&t, tree->buffer, tree->size);
>> +
>> + memset(&opts, 0, sizeof(opts));
>> + opts.head_idx = -1;
>> + opts.src_index = &the_index;
>> + opts.dst_index = &the_index;
>> + opts.index_only = 1;
>> +
>> + if (unpack_trees(1, &t, &opts)) {
>> + /* We've already reported the error, finish dying */
>> + exit(128);
>> + }
>
> This bit of the code surprised me. I guess you are trying to re-create
> the index state of the HEAD so that the commit you build on top of it
> contains _only_ the untracked files as changes, and not whatever
> intermediate index state you had. That makes some sense to me, as clean
> is never touching the index state.
TBH, I didn't really think this stuff through, I basically just hacked
on this until I got it to save away superfluous files when the index
matched HEAD. So this part is more accidental than designed. I'm not
very familiar with the index-maniuplation code in Git either.
I *think* the right thing to do would be to save the tree of HEAD
*plus* those deleted files in this case. That way it only records the
destruction itself. This does *not* seem to be what currently happens.
If I have a change staged, that staged change also gets included in
the commit. That's not ideal, I think.
> Though taking a step back for a moment, I'd like to think about doing
> something similar for "reset --hard", which is the other "destructive"
> operation in git[1]. In that case, I think saving the index state is also
> useful, because it is being reset, too (and while those blobs are
> recoverable, the exact state is sometimes useful).
I agree. I guess that should essentially do a "full" git-stash.
> If we were to do that, should it be a separate ref? Or should there be a
> single backup ref for such "oops, undo that" operations? If the latter,
> what should that ref look like? I think it would look something like
> refs/stash, with the index and the working tree state stored as separate
> commits (even though in the "clean" case, the index state is not likely
> to be that interesting, it is cheap to store and makes the recovery
> tools uniform to use).
Hmm, perhaps. I do like the concept of a "git undo" of sorts, but
perhaps that'll raise the expectations even further? Or maybe raising
them a good thing, so people add missing features? :)
> And if we are going to store it like that, should we just be using "git
> stash save --keep-index --include-untracked"? I think we would just need
> to teach it a "--no-reset" option to leave the tracked files as-is.
Hm, interesting. But it does leave me with a bit of a bad feeling; git
stash is currently a shell-script, and I want us to move *away* from
depending on those rather than towards... Or perhaps I just convinced
myself to port git-stash to C? I guess the full script won't be
needed, only the heavy lifting...
> I realize that I went a few steps down the "if..." chain there to get to
> "should we just be using stash?". But it would also make the code here
> dirt-simple.
Simplicity is good, and it's always good to hea
[PATCH/RFC] clean: add a flag to back up cleaned files
The combination of "git clean" and fat fingers can some times cause
data-loss, which can be frustrating.
So let's add a flag that imports the files to be deleted into the
object-database, in a way similar to what git-stash does. Maintain
a reflog of the previously backed up clean-runs.
Signed-off-by: Erik Faye-Lund
---
I've had a similar patch laying around for quite a while, but since
f538a91 ("git-clean: Display more accurate delete messages"), this
patch is a lot less nasty than before. So here you go, perhaps
someone else has fat fingers and hate to lose work?
Documentation/config.txt| 4 ++
Documentation/git-clean.txt | 4 ++
builtin/clean.c | 111 +++-
t/t7300-clean.sh| 12 +
4 files changed, 130 insertions(+), 1 deletion(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1932e9b..d58fe31 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -797,6 +797,10 @@ clean.requireForce::
A boolean to make git-clean do nothing unless given -f,
-i or -n. Defaults to true.
+clean.backup::
+ A boolean to make git-clean back up files before they are
+ deleted. Defaults to false.
+
color.branch::
A boolean to enable/disable color in the output of
linkgit:git-branch[1]. May be set to `always`,
diff --git a/Documentation/git-clean.txt b/Documentation/git-clean.txt
index 8997922..bc9d703 100644
--- a/Documentation/git-clean.txt
+++ b/Documentation/git-clean.txt
@@ -41,6 +41,10 @@ OPTIONS
--interactive::
Show what would be done and clean files interactively. See
``Interactive mode'' for details.
+-b::
+--backup::
+ Back up files to a reflog before deleting them. The tree of
+ backed up files are stored in the reflog for refs/clean-backup.
-n::
--dry-run::
diff --git a/builtin/clean.c b/builtin/clean.c
index 9a91515..96fb4b2 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -16,9 +16,12 @@
#include "column.h"
#include "color.h"
#include "pathspec.h"
+#include "tree-walk.h"
+#include "unpack-trees.h"
+#include "cache-tree.h"
static int force = -1; /* unset */
-static int interactive;
+static int interactive, backup;
static struct string_list del_list = STRING_LIST_INIT_DUP;
static unsigned int colopts;
@@ -120,6 +123,11 @@ static int git_clean_config(const char *var, const char
*value, void *cb)
return 0;
}
+ if (!strcmp(var, "clean.backup")) {
+ backup = git_config_bool(var, value);
+ return 0;
+ }
+
if (!strcmp(var, "clean.requireforce")) {
force = !git_config_bool(var, value);
return 0;
@@ -148,6 +156,93 @@ static int exclude_cb(const struct option *opt, const char
*arg, int unset)
return 0;
}
+static int backed_up_anything;
+
+static void backup_file(const char *path, struct stat *st)
+{
+ if (add_to_cache(path, st, 0))
+ die(_("backing up '%s' failed"), path);
+ backed_up_anything = 1;
+}
+
+static struct commit_list *parents;
+
+static void prepare_backup(void)
+{
+ struct unpack_trees_options opts;
+ unsigned char sha1[20];
+ struct tree *tree;
+ struct commit *parent;
+ struct tree_desc t;
+
+ if (get_sha1("HEAD", sha1))
+ die(_("You do not have the initial commit yet"));
+
+ /* prepare parent-list */
+ parent = lookup_commit_or_die(sha1, "HEAD");
+ commit_list_insert(parent, &parents);
+
+ /* load HEAD into the index */
+
+ tree = parse_tree_indirect(sha1);
+ if (!tree)
+ die(_("Failed to unpack tree object %s"), sha1);
+
+ parse_tree(tree);
+ init_tree_desc(&t, tree->buffer, tree->size);
+
+ memset(&opts, 0, sizeof(opts));
+ opts.head_idx = -1;
+ opts.src_index = &the_index;
+ opts.dst_index = &the_index;
+ opts.index_only = 1;
+
+ if (unpack_trees(1, &t, &opts)) {
+ /* We've already reported the error, finish dying */
+ exit(128);
+ }
+}
+
+static void finish_backup(void)
+{
+ const char *ref = "refs/clean-backup";
+ unsigned char commit_sha1[20];
+ struct strbuf msg = STRBUF_INIT;
+ char logfile[PATH_MAX];
+ struct stat st;
+
+ if (!backed_up_anything)
+ return;
+
+ if (!active_cache_tree)
+ active_cache_tree = cache_tree();
+
+ if (!cache_tree_fully_valid(active_cache_tree)) {
+ if (cache_tree_update(active_cache_tree,
+ (const struct cache_entry * const *)active_cache,
+ active_nr, 0) < 0)
+ die(
Re: [msysGit] Re: [PATCH/RFC] send-pack.c: Allow to disable side-band-64k
On Tue, May 20, 2014 at 10:46 AM, Thomas Braun
wrote:
> Am 19.05.2014 22:29, schrieb Erik Faye-Lund:
>>> [...]
>>> Would we need to wrap both ends, shouldn't wrapping only reading be
>>> good enough to prevent deadlocking?
>>>
>>> compat/poll/poll.c already contains a function called IsSocketHandle
>>> that is able to tell if a HANDLE points to a socket or not.
>>
>> This very quick attempt did not work out :(
>>
>> diff --git a/compat/mingw.c b/compat/mingw.c
>> index 0335958..ec1d81f 100644
>> --- a/compat/mingw.c
>> +++ b/compat/mingw.c
>> @@ -370,6 +370,65 @@ int mingw_open (const char *filename, int oflags, ...)
>> return fd;
>> }
>>
>> +#define is_console_handle(h) (((long) (h) & 3) == 3)
>> +
>> +static int is_socket_handle(HANDLE h)
>> +{
>> +WSANETWORKEVENTS ev;
>> +
>> +if (is_console_handle(h))
>> +return 0;
>> +
>> +/*
>> + * Under Wine, it seems that getsockopt returns 0 for pipes too.
>> + * WSAEnumNetworkEvents instead distinguishes the two correctly.
>> + */
>> +ev.lNetworkEvents = 0xDEADBEEF;
>> +WSAEnumNetworkEvents((SOCKET) h, NULL, &ev);
>> +return ev.lNetworkEvents != 0xDEADBEEF;
>> +}
>> +
>> +#undef read
>> +ssize_t mingw_read(int fd, void *buf, size_t count)
>> +{
>> +int ret;
>> +HANDLE fh = (HANDLE)_get_osfhandle(fd);
>> +
>> +if (fh == INVALID_HANDLE_VALUE) {
>> +errno = EBADF;
>> +return -1;
>> +}
>> +
>> +if (!is_socket_handle(fh))
>> +return read(fd, buf, count);
>> +
>> +ret = recv((SOCKET)fh, buf, count, 0);
>> +if (ret < 0)
>> +errno = WSAGetLastError();
>> +return ret;
>> +}
>> +
>> +#undef write
>> +ssize_t mingw_write(int fd, const void *buf, size_t count)
>> +{
>> +int ret;
>> +HANDLE fh = (HANDLE)_get_osfhandle(fd);
>> +
>> +if (fh == INVALID_HANDLE_VALUE) {
>> +errno = EBADF;
>> +return -1;
>> +}
>> +
>> +if (!is_socket_handle(fh))
>> +return write(fd, buf, count);
>> +
>> +return send((SOCKET)fh, buf, count, 0);
>> +if (ret < 0)
>> +errno = WSAGetLastError();
>> +return ret;
>> +}
>> +
>> +
>> static BOOL WINAPI ctrl_ignore(DWORD type)
>> {
>> return TRUE;
>> diff --git a/compat/mingw.h b/compat/mingw.h
>> index 08b83fe..1690098 100644
>> --- a/compat/mingw.h
>> +++ b/compat/mingw.h
>> @@ -177,6 +177,12 @@ int mingw_rmdir(const char *path);
>> int mingw_open (const char *filename, int oflags, ...);
>> #define open mingw_open
>>
>> +ssize_t mingw_read(int fd, void *buf, size_t count);
>> +#define read mingw_read
>> +
>> +ssize_t mingw_write(int fd, const void *buf, size_t count);
>> +#define write mingw_write
>> +
>> int mingw_fgetc(FILE *stream);
>> #define fgetc mingw_fgetc
>
> According to [1] you also have to pass WSA_FLAG_OVERLAPPED to avoid the
> deadlock.
>
> With that change I don't get a hang anymore but a read error with
> errno 10054 aka WSAECONNRESET.
>
> [1]: https://groups.google.com/forum/#!msg/msysgit/at8D7J-h7mw/PM9w-d41cDYJ
>
Yeah, sorry, I noticed this right after sending and tested with that
as well. My results were the same :/
--
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/RFC] send-pack.c: Allow to disable side-band-64k
On Mon, May 19, 2014 at 11:15 PM, Thomas Braun wrote: > Am 19.05.2014 21:33, schrieb Jonathan Nieder: > >> Hi, >> >> Thomas Braun wrote: >> >>> pushing over the dump git protocol with a windows git client. >> >> >> I've never heard of the dump git protocol. Do you mean the git >> protocol that's used with git:// URLs? > > > You are right I mean the protocol involving git:// URLs. But unfortunately I > got it wrong as according to [1] the git:// is one of the so-called smart > protocols. That was also the source where I read that there are smart and > dump protocols. > > [1]: http://git-scm.com/book/en/Git-Internals-Transfer-Protocols > > >> [...] >>> >>> Alternative approaches considered but deemed too invasive: >>> - Rewrite read/write wrappers in mingw.c in order to distinguish between >>>a file descriptor which has a socket behind and a file descriptor >>>which has a file behind. >> >> >> I assume here "too invasive" means "too much engineering effort"? >> >> It sounds like a clean fix, not too invasive at all. But I can >> understand wanting a stopgap in the meantime. > > > No actually I meant too invasive in the sense of "requiring large rewrites > which only benefit git on windows and hurt all others". > > The two fixes I can think of either involve: > - In a read *and* write wrapper the need to check if the fd is a socket, if > yes use send/recv if no use read/write. According to Erik's comments this > should be possible. But I would deem the expected performance penalty quite > large as that will be done in every call. You clearly haven't stepped through MSVCRT's read and write implementations :P I wouldn't worry too much about this, at least not until the numbers are in. -- 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: [msysGit] Re: [PATCH/RFC] send-pack.c: Allow to disable side-band-64k
On Mon, May 19, 2014 at 10:00 PM, Erik Faye-Lund wrote:
> On Mon, May 19, 2014 at 9:33 PM, Jonathan Nieder wrote:
>> Hi,
>>
>> Thomas Braun wrote:
>>
>>> pushing over the dump git protocol with a windows git client.
>>
>> I've never heard of the dump git protocol. Do you mean the git
>> protocol that's used with git:// URLs?
>>
>> [...]
>>> Alternative approaches considered but deemed too invasive:
>>> - Rewrite read/write wrappers in mingw.c in order to distinguish between
>>> a file descriptor which has a socket behind and a file descriptor
>>> which has a file behind.
>>
>> I assume here "too invasive" means "too much engineering effort"?
>>
>> It sounds like a clean fix, not too invasive at all. But I can
>> understand wanting a stopgap in the meantime.
>>
>
> Yeah, now that the problem seems to be understood, I don't think that
> would be too bad. I recently killed off our previous write()-wrapper
> in c9df6f4, but I see no reason why we can't add a new one.
>
> Would we need to wrap both ends, shouldn't wrapping only reading be
> good enough to prevent deadlocking?
>
> compat/poll/poll.c already contains a function called IsSocketHandle
> that is able to tell if a HANDLE points to a socket or not.
This very quick attempt did not work out :(
diff --git a/compat/mingw.c b/compat/mingw.c
index 0335958..ec1d81f 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -370,6 +370,65 @@ int mingw_open (const char *filename, int oflags, ...)
return fd;
}
+#define is_console_handle(h) (((long) (h) & 3) == 3)
+
+static int is_socket_handle(HANDLE h)
+{
+WSANETWORKEVENTS ev;
+
+if (is_console_handle(h))
+return 0;
+
+/*
+ * Under Wine, it seems that getsockopt returns 0 for pipes too.
+ * WSAEnumNetworkEvents instead distinguishes the two correctly.
+ */
+ev.lNetworkEvents = 0xDEADBEEF;
+WSAEnumNetworkEvents((SOCKET) h, NULL, &ev);
+return ev.lNetworkEvents != 0xDEADBEEF;
+}
+
+#undef read
+ssize_t mingw_read(int fd, void *buf, size_t count)
+{
+int ret;
+HANDLE fh = (HANDLE)_get_osfhandle(fd);
+
+if (fh == INVALID_HANDLE_VALUE) {
+errno = EBADF;
+return -1;
+}
+
+if (!is_socket_handle(fh))
+return read(fd, buf, count);
+
+ret = recv((SOCKET)fh, buf, count, 0);
+if (ret < 0)
+errno = WSAGetLastError();
+return ret;
+}
+
+#undef write
+ssize_t mingw_write(int fd, const void *buf, size_t count)
+{
+int ret;
+HANDLE fh = (HANDLE)_get_osfhandle(fd);
+
+if (fh == INVALID_HANDLE_VALUE) {
+errno = EBADF;
+return -1;
+}
+
+if (!is_socket_handle(fh))
+return write(fd, buf, count);
+
+return send((SOCKET)fh, buf, count, 0);
+if (ret < 0)
+errno = WSAGetLastError();
+return ret;
+}
+
+
static BOOL WINAPI ctrl_ignore(DWORD type)
{
return TRUE;
diff --git a/compat/mingw.h b/compat/mingw.h
index 08b83fe..1690098 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -177,6 +177,12 @@ int mingw_rmdir(const char *path);
int mingw_open (const char *filename, int oflags, ...);
#define open mingw_open
+ssize_t mingw_read(int fd, void *buf, size_t count);
+#define read mingw_read
+
+ssize_t mingw_write(int fd, const void *buf, size_t count);
+#define write mingw_write
+
int mingw_fgetc(FILE *stream);
#define fgetc mingw_fgetc
--
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: [msysGit] Re: [PATCH/RFC] send-pack.c: Allow to disable side-band-64k
On Mon, May 19, 2014 at 9:33 PM, Jonathan Nieder wrote: > Hi, > > Thomas Braun wrote: > >> pushing over the dump git protocol with a windows git client. > > I've never heard of the dump git protocol. Do you mean the git > protocol that's used with git:// URLs? > > [...] >> Alternative approaches considered but deemed too invasive: >> - Rewrite read/write wrappers in mingw.c in order to distinguish between >> a file descriptor which has a socket behind and a file descriptor >> which has a file behind. > > I assume here "too invasive" means "too much engineering effort"? > > It sounds like a clean fix, not too invasive at all. But I can > understand wanting a stopgap in the meantime. > Yeah, now that the problem seems to be understood, I don't think that would be too bad. I recently killed off our previous write()-wrapper in c9df6f4, but I see no reason why we can't add a new one. Would we need to wrap both ends, shouldn't wrapping only reading be good enough to prevent deadlocking? compat/poll/poll.c already contains a function called IsSocketHandle that is able to tell if a HANDLE points to a socket or not. -- 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: [msysGit] Re: [PATCH v2] config: preserve config file permissions on edits
On Mon, May 19, 2014 at 9:17 PM, Thomas Braun
wrote:
> Am Montag, den 19.05.2014, 11:59 +0200 schrieb Erik Faye-Lund:
>> On Mon, May 19, 2014 at 9:44 AM, Erik Faye-Lund wrote:
>> > On Mon, May 19, 2014 at 9:13 AM, Johannes Sixt
>> > wrote:
>> >> Am 5/6/2014 2:17, schrieb Eric Wong:
>> >>> Users may already store sensitive data such as imap.pass in
>> >>> ..git/config; making the file world-readable when "git config"
>> >>> is called to edit means their password would be compromised
>> >>> on a shared system.
>> >>>
>> >>> [v2: updated for section renames, as noted by Junio]
>> >>>
>> >>> Signed-off-by: Eric Wong
>> >>> ---
>> >>> config.c | 16
>> >>> t/t1300-repo-config.sh | 10 ++
>> >>> 2 files changed, 26 insertions(+)
>> >>>
>> >>> diff --git a/config.c b/config.c
>> >>> index a30cb5c..c227aa8 100644
>> >>> --- a/config.c
>> >>> +++ b/config.c
>> >>> @@ -1636,6 +1636,13 @@ int git_config_set_multivar_in_file(const char
>> >>> *config_filename,
>> >>> MAP_PRIVATE, in_fd, 0);
>> >>> close(in_fd);
>> >>>
>> >>> + if (fchmod(fd, st.st_mode & 0) < 0) {
>> >>> + error("fchmod on %s failed: %s",
>> >>> + lock->filename, strerror(errno));
>> >>> + ret = CONFIG_NO_WRITE;
>> >>> + goto out_free;
>> >>> + }
>> >>
>> >> This introduces a severe failure in the Windows port because we do not
>> >> implement fchmod. Existing fchmod invocations do not check the return
>> >> value, and they are only interested in removing the write bits, and we
>> >> generally don't care a lot if files remain writable.
>> >>
>> >> I'm not proficient enough to add any ACL fiddling to fchmod that would be
>> >> required by the above change, whose purpose is to be strict about
>> >> permissions. Nor am I interested (who the heck is sharing a Windows box
>> >> anyway? ;-)
>> >>
>> >> Therefore, here's just a work-around patch to keep things going on
>> >> Windows. Any opinions from the Windows corner?
>> >>
>> >
>> > Since we use MSVCRT's chmod implementation directly which only checks
>> > for S_IWRITE,and Get/SetFileAttributes to simply set or clear the
>> > FILE_ATTRIBUTE_READONLY-flag, perhaps we could do the same except
>> > using Get/SetFileInformationByHandle instead? That takes us in a
>> > better direction, IMO. Adding full ACL support seems like a bigger
>> > project.
>> >
>> > If there's no objection, I'll sketch up a patch for that...
>>
>> OK, this turned out a bit more yucky than I assumed, because
>> SetFileInformationByHandle is only available on Windows Vista and up.
>> There's a static library (FileExtd.lib) that ships with MSVC that
>> emulates it for legacy support, I guess I should have a look at what
>> that does.
>
> Do we still want to fully support Windows XP?
> Adding a work around here for a EOL operating system sounds like a lot
> of effort.
I personally don't care about Windows XP, but some other influential
people (J6T IIRC) disagreed the last time this was discussed. I don't
want to step on anyone's toes.
--
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: [msysGit] Re: [PATCH v2] config: preserve config file permissions on edits
On Mon, May 19, 2014 at 9:44 AM, Erik Faye-Lund wrote:
> On Mon, May 19, 2014 at 9:13 AM, Johannes Sixt wrote:
>> Am 5/6/2014 2:17, schrieb Eric Wong:
>>> Users may already store sensitive data such as imap.pass in
>>> ..git/config; making the file world-readable when "git config"
>>> is called to edit means their password would be compromised
>>> on a shared system.
>>>
>>> [v2: updated for section renames, as noted by Junio]
>>>
>>> Signed-off-by: Eric Wong
>>> ---
>>> config.c | 16
>>> t/t1300-repo-config.sh | 10 ++
>>> 2 files changed, 26 insertions(+)
>>>
>>> diff --git a/config.c b/config.c
>>> index a30cb5c..c227aa8 100644
>>> --- a/config.c
>>> +++ b/config.c
>>> @@ -1636,6 +1636,13 @@ int git_config_set_multivar_in_file(const char
>>> *config_filename,
>>> MAP_PRIVATE, in_fd, 0);
>>> close(in_fd);
>>>
>>> + if (fchmod(fd, st.st_mode & 0) < 0) {
>>> + error("fchmod on %s failed: %s",
>>> + lock->filename, strerror(errno));
>>> + ret = CONFIG_NO_WRITE;
>>> + goto out_free;
>>> + }
>>
>> This introduces a severe failure in the Windows port because we do not
>> implement fchmod. Existing fchmod invocations do not check the return
>> value, and they are only interested in removing the write bits, and we
>> generally don't care a lot if files remain writable.
>>
>> I'm not proficient enough to add any ACL fiddling to fchmod that would be
>> required by the above change, whose purpose is to be strict about
>> permissions. Nor am I interested (who the heck is sharing a Windows box
>> anyway? ;-)
>>
>> Therefore, here's just a work-around patch to keep things going on
>> Windows. Any opinions from the Windows corner?
>>
>
> Since we use MSVCRT's chmod implementation directly which only checks
> for S_IWRITE,and Get/SetFileAttributes to simply set or clear the
> FILE_ATTRIBUTE_READONLY-flag, perhaps we could do the same except
> using Get/SetFileInformationByHandle instead? That takes us in a
> better direction, IMO. Adding full ACL support seems like a bigger
> project.
>
> If there's no objection, I'll sketch up a patch for that...
OK, this turned out a bit more yucky than I assumed, because
SetFileInformationByHandle is only available on Windows Vista and up.
There's a static library (FileExtd.lib) that ships with MSVC that
emulates it for legacy support, I guess I should have a look at what
that does.
--
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: [msysGit] Re: [PATCH v2] config: preserve config file permissions on edits
On Mon, May 19, 2014 at 9:13 AM, Johannes Sixt wrote:
> Am 5/6/2014 2:17, schrieb Eric Wong:
>> Users may already store sensitive data such as imap.pass in
>> ..git/config; making the file world-readable when "git config"
>> is called to edit means their password would be compromised
>> on a shared system.
>>
>> [v2: updated for section renames, as noted by Junio]
>>
>> Signed-off-by: Eric Wong
>> ---
>> config.c | 16
>> t/t1300-repo-config.sh | 10 ++
>> 2 files changed, 26 insertions(+)
>>
>> diff --git a/config.c b/config.c
>> index a30cb5c..c227aa8 100644
>> --- a/config.c
>> +++ b/config.c
>> @@ -1636,6 +1636,13 @@ int git_config_set_multivar_in_file(const char
>> *config_filename,
>> MAP_PRIVATE, in_fd, 0);
>> close(in_fd);
>>
>> + if (fchmod(fd, st.st_mode & 0) < 0) {
>> + error("fchmod on %s failed: %s",
>> + lock->filename, strerror(errno));
>> + ret = CONFIG_NO_WRITE;
>> + goto out_free;
>> + }
>
> This introduces a severe failure in the Windows port because we do not
> implement fchmod. Existing fchmod invocations do not check the return
> value, and they are only interested in removing the write bits, and we
> generally don't care a lot if files remain writable.
>
> I'm not proficient enough to add any ACL fiddling to fchmod that would be
> required by the above change, whose purpose is to be strict about
> permissions. Nor am I interested (who the heck is sharing a Windows box
> anyway? ;-)
>
> Therefore, here's just a work-around patch to keep things going on
> Windows. Any opinions from the Windows corner?
>
Since we use MSVCRT's chmod implementation directly which only checks
for S_IWRITE,and Get/SetFileAttributes to simply set or clear the
FILE_ATTRIBUTE_READONLY-flag, perhaps we could do the same except
using Get/SetFileInformationByHandle instead? That takes us in a
better direction, IMO. Adding full ACL support seems like a bigger
project.
If there's no objection, I'll sketch up a patch for that...
--
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 2/2] wincred: avoid overwriting configured variables
On Wed, May 14, 2014 at 10:45 AM, Stepan Kasal wrote: > Hello kusma, > > On Tue, May 13, 2014 at 08:56:54AM +0200, Erik Faye-Lund wrote: >> > --- a/contrib/credential/wincred/Makefile >> > +++ b/contrib/credential/wincred/Makefile >> > @@ -1,12 +1,12 @@ >> > all: git-credential-wincred.exe >> > >> > -CC = gcc >> > -RM = rm -f >> > -CFLAGS = -O2 -Wall >> > - >> > -include ../../../config.mak.autogen >> > -include ../../../config.mak >> > >> > +CC ?= gcc >> > +RM ?= rm -f >> > +CFLAGS ?= -O2 -Wall >> > + >> > prefix ?= /usr/local >> > libexecdir ?= $(prefix)/libexec/git-core >> > >> >> Yeah, looks good to me. > > thanks, but it looks you replied only to my personal mail. Was it > intentional? No, sorry about that. Consider the patches Acked-by: Erik Faye-Lund -- 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 2/3] Add read-cache--daemon
On Tue, May 13, 2014 at 4:10 PM, Duy Nguyen wrote: > On Tue, May 13, 2014 at 9:06 PM, Erik Faye-Lund wrote: >> On Tue, May 13, 2014 at 3:49 PM, Duy Nguyen wrote: >>> On Tue, May 13, 2014 at 8:37 PM, Erik Faye-Lund wrote: >>>> On Tue, May 13, 2014 at 3:01 PM, Duy Nguyen wrote: >>>>> What do you think is a good replacement for unix socket on Windows? >>>>> It's only used to refresh the cache in the daemon, no sensitive data >>>>> sent over, so security is not a problem. I'm thinking maybe just >>>>> TCP/IP server, but that's going to be a system-wide daemon.. Perhaps >>>>> the windows daemon could just monitor $GIT_DIR/index and refresh it? >>>> >>>> Windows has support for Named Pipes, which seems like the right kind >>>> of communication channel. However, the programming model differs quite >>>> a bit from unix-sockets: >>>> >>>> http://msdn.microsoft.com/en-us/library/windows/desktop/aa365594%28v=vs.85%29.aspx >>> >>> Yeah that was my first option, but if code cannot be shared to >>> differences then we probably should go another way. The old >>> FindWindow/PostMessage still works with modern Windows, right? Maybe >>> we could create a window with a name derived from the daemon's pid and >>> save the name in the index, then PostMessage can signal the daemon. On >>> the UNIX front, we store pid and send SIGUSR1 instead..The good thing >>> here is the Git side will be very simple (PostMessage vs kill). >> >> Hmmm I'm a bit worried about having to load in USER32.DLL just to >> read the cache that way. But it seems we already do that, thanks to >> compat/poll/poll.c (it depends on DispatchMessage, >> MsgWaitForMultipleObjects, PeekMessage and TranslateMessage, all from >> that DLL). >> >> Preferably, we should delay-load USER32.DLL in compat/poll/poll.c, but >> if we start needing it for the reading the index, it'll be loaded by >> the vast majority of processes anyway. > > Thanks for the info. I'll see if we can still stick to named pipes. If > we have to load user32.dll, hopefully the gain will outweigh load time > for that dell. I just timed it here on my system, and omitting USER32.DLL didn't gain anything for "git --version", so I suspect I was worrying too soon. -- 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 2/3] Add read-cache--daemon
On Tue, May 13, 2014 at 3:49 PM, Duy Nguyen wrote: > On Tue, May 13, 2014 at 8:37 PM, Erik Faye-Lund wrote: >> On Tue, May 13, 2014 at 3:01 PM, Duy Nguyen wrote: >>> What do you think is a good replacement for unix socket on Windows? >>> It's only used to refresh the cache in the daemon, no sensitive data >>> sent over, so security is not a problem. I'm thinking maybe just >>> TCP/IP server, but that's going to be a system-wide daemon.. Perhaps >>> the windows daemon could just monitor $GIT_DIR/index and refresh it? >> >> Windows has support for Named Pipes, which seems like the right kind >> of communication channel. However, the programming model differs quite >> a bit from unix-sockets: >> >> http://msdn.microsoft.com/en-us/library/windows/desktop/aa365594%28v=vs.85%29.aspx > > Yeah that was my first option, but if code cannot be shared to > differences then we probably should go another way. The old > FindWindow/PostMessage still works with modern Windows, right? Maybe > we could create a window with a name derived from the daemon's pid and > save the name in the index, then PostMessage can signal the daemon. On > the UNIX front, we store pid and send SIGUSR1 instead..The good thing > here is the Git side will be very simple (PostMessage vs kill). Hmmm I'm a bit worried about having to load in USER32.DLL just to read the cache that way. But it seems we already do that, thanks to compat/poll/poll.c (it depends on DispatchMessage, MsgWaitForMultipleObjects, PeekMessage and TranslateMessage, all from that DLL). Preferably, we should delay-load USER32.DLL in compat/poll/poll.c, but if we start needing it for the reading the index, it'll be loaded by the vast majority of processes anyway. -- 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 2/3] Add read-cache--daemon
On Tue, May 13, 2014 at 3:01 PM, Duy Nguyen wrote: > What do you think is a good replacement for unix socket on Windows? > It's only used to refresh the cache in the daemon, no sensitive data > sent over, so security is not a problem. I'm thinking maybe just > TCP/IP server, but that's going to be a system-wide daemon.. Perhaps > the windows daemon could just monitor $GIT_DIR/index and refresh it? Windows has support for Named Pipes, which seems like the right kind of communication channel. However, the programming model differs quite a bit from unix-sockets: http://msdn.microsoft.com/en-us/library/windows/desktop/aa365594%28v=vs.85%29.aspx -- 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 8/8] read-cache: inform the daemon that the index has been updated
On Tue, May 13, 2014 at 1:15 PM, Nguyễn Thái Ngọc Duy wrote:
> The daemon would immediately load the new index in memory in
> background. Next time Git needs to read the index again, everything is
> ready.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy
> ---
> cache.h | 1 +
> read-cache.c | 53 -
> 2 files changed, 49 insertions(+), 5 deletions(-)
>
> diff --git a/cache.h b/cache.h
> index fb29c7e..3115b86 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -483,6 +483,7 @@ extern int is_index_unborn(struct index_state *);
> extern int read_index_unmerged(struct index_state *);
> #define COMMIT_LOCK(1 << 0)
> #define CLOSE_LOCK (1 << 1)
> +#define REFRESH_DAEMON (1 << 2)
> extern int write_locked_index(struct index_state *, struct lock_file *lock,
> unsigned flags);
> extern int discard_index(struct index_state *);
> extern int unmerged_index(const struct index_state *);
> diff --git a/read-cache.c b/read-cache.c
> index e98521f..d5c9247 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -16,6 +16,9 @@
> #include "varint.h"
> #include "split-index.h"
> #include "sigchain.h"
> +#include "unix-socket.h"
> +#include "pkt-line.h"
> +#include "run-command.h"
>
> static struct cache_entry *refresh_cache_entry(struct cache_entry *ce,
>unsigned int options);
> @@ -2030,6 +2033,32 @@ void set_alternate_index_output(const char *name)
> alternate_index_output = name;
> }
>
> +static void refresh_daemon(struct index_state *istate)
> +{
> + int fd;
> + fd = unix_stream_connect(git_path("daemon/index"));
> + if (fd < 0) {
> + struct child_process cp;
> + const char *av[] = {"read-cache--daemon", "--detach", NULL };
> + memset(&cp, 0, sizeof(cp));
> + cp.argv = av;
> + cp.git_cmd = 1;
> + cp.no_stdin = 1;
> + if (run_command(&cp))
> + warning(_("failed to start read-cache--daemon: %s"),
> + strerror(errno));
> + return;
> + }
> + /*
> +* packet_write() could die() but unless this is from
> +* update_index_if_able(), we're about to exit anyway,
> +* probably ok to die (for now). Blocking mode is another
> +* problem to deal with later.
> +*/
> + packet_write(fd, "refresh");
> + close(fd);
> +}
> +
Seems the argument 'istate' isn't used.
--
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 5/8] read-cache: try index data from shared memory
On Tue, May 13, 2014 at 1:15 PM, Nguyễn Thái Ngọc Duy wrote:
> Signed-off-by: Nguyễn Thái Ngọc Duy
> ---
> Documentation/config.txt | 4
> cache.h | 1 +
> config.c | 12
> environment.c| 1 +
> read-cache.c | 43 +++
> submodule.c | 1 +
> 6 files changed, 62 insertions(+)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index d8b6cc9..ccbe00b 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -617,6 +617,10 @@ relatively high IO latencies. With this set to 'true',
> Git will do the
> index comparison to the filesystem data in parallel, allowing
> overlapping IO's.
>
> +core.useReadCacheDaemon::
> + Use `git read-cache--daemon` to speed up index reading. See
> + linkgit:git-read-cache--daemon for more information.
> +
> core.createObject::
> You can set this to 'link', in which case a hardlink followed by
> a delete of the source are used to make sure that object creation
> diff --git a/cache.h b/cache.h
> index d0ff11c..fb29c7e 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -603,6 +603,7 @@ extern size_t packed_git_limit;
> extern size_t delta_base_cache_limit;
> extern unsigned long big_file_threshold;
> extern unsigned long pack_size_limit_cfg;
> +extern int use_read_cache_daemon;
>
> /*
> * Do replace refs need to be checked this run? This variable is
> diff --git a/config.c b/config.c
> index a30cb5c..5c832ad 100644
> --- a/config.c
> +++ b/config.c
> @@ -874,6 +874,18 @@ static int git_default_core_config(const char *var,
> const char *value)
> return 0;
> }
>
> +#ifdef HAVE_SHM
> + /*
> +* Currently git-read-cache--daemon is only built when
> +* HAVE_SHM is set. Ignore user settings if HAVE_SHM is not
> +* defined.
> +*/
> + if (!strcmp(var, "core.usereadcachedaemon")) {
> + use_read_cache_daemon = git_config_bool(var, value);
> + return 0;
> + }
> +#endif
> +
> /* Add other config variables here and to Documentation/config.txt. */
> return 0;
> }
> diff --git a/environment.c b/environment.c
> index 5c4815d..b76a414 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -63,6 +63,7 @@ int merge_log_config = -1;
> int precomposed_unicode = -1; /* see probe_utf8_pathname_composition() */
> struct startup_info *startup_info;
> unsigned long pack_size_limit_cfg;
> +int use_read_cache_daemon;
>
> /*
> * The character that begins a commented line in user-editable file
> diff --git a/read-cache.c b/read-cache.c
> index a5031f3..0e46523 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -1462,6 +1462,48 @@ static struct cache_entry *create_from_disk(struct
> ondisk_cache_entry *ondisk,
> return ce;
> }
>
> +static void *try_shm(void *mmap, size_t *mmap_size)
> +{
> + struct strbuf sb = STRBUF_INIT;
> + size_t old_size = *mmap_size;
> + void *new_mmap;
> + struct stat st;
> + int fd;
> +
> + if (old_size <= 20 || !use_read_cache_daemon)
> + return mmap;
> +
> + strbuf_addf(&sb, "/git-index-%s.lock",
> + sha1_to_hex((unsigned char *)mmap + old_size - 20));
> + fd = shm_open(sb.buf, O_RDONLY, 0777);
OK, so *here* you do an unguarded use of shm_open, but the code never
gets executed because of the HAVE_SHM guard in
git_default_core_config. Perhaps not introduce the compatibility shim
until this patch, then?
--
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 2/8] unix-socket: stub impl. for platforms with no unix socket support
On Tue, May 13, 2014 at 1:59 PM, Erik Faye-Lund wrote:
> On Tue, May 13, 2014 at 1:15 PM, Nguyễn Thái Ngọc Duy
> wrote:
>> With this we can make unix_stream_* calls without #ifdef.
>>
>> Signed-off-by: Nguyễn Thái Ngọc Duy
>> ---
>> Makefile | 2 ++
>> unix-socket.h | 18 ++
>> 2 files changed, 20 insertions(+)
>>
>> diff --git a/Makefile b/Makefile
>> index 028749b..d0a2b4b 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -1417,6 +1417,8 @@ ifndef NO_UNIX_SOCKETS
>> LIB_H += unix-socket.h
>> PROGRAM_OBJS += credential-cache.o
>> PROGRAM_OBJS += credential-cache--daemon.o
>> +else
>> + BASIC_CFLAGS += -DNO_UNIX_SOCKETS
>> endif
>>
>> ifdef NO_ICONV
>> diff --git a/unix-socket.h b/unix-socket.h
>> index e271aee..f1cba70 100644
>> --- a/unix-socket.h
>> +++ b/unix-socket.h
>> @@ -1,7 +1,25 @@
>> #ifndef UNIX_SOCKET_H
>> #define UNIX_SOCKET_H
>>
>> +#ifndef NO_UNIX_SOCKETS
>> +
>> int unix_stream_connect(const char *path);
>> int unix_stream_listen(const char *path);
>>
>> +#else
>> +
>> +static inline int unix_stream_connect(const char *path)
>> +{
>> + errno = ENOSYS;
>> + return -1;
>> +}
>> +
>> +static inline int unix_stream_listen(const char *path)
>> +{
>> + errno = ENOSYS;
>> + return -1;
>> +}
>> +
>> +#endif
>> +
>> #endif /* UNIX_SOCKET_H */
>
> OK, so I missed this before my other two comments. But still... in
> what way does errno=ENOSYS make this *work*? Won't we end up compiling
> lots of non-functional tools on Windows in this case?
ENOSYS makes git-credential-cache.c just die with the message "unable
to start cache daemon", and git-credential--daemon.c die with "unable
to bind to ".
--
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 2/8] unix-socket: stub impl. for platforms with no unix socket support
On Tue, May 13, 2014 at 1:15 PM, Nguyễn Thái Ngọc Duy wrote:
> With this we can make unix_stream_* calls without #ifdef.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy
> ---
> Makefile | 2 ++
> unix-socket.h | 18 ++
> 2 files changed, 20 insertions(+)
>
> diff --git a/Makefile b/Makefile
> index 028749b..d0a2b4b 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1417,6 +1417,8 @@ ifndef NO_UNIX_SOCKETS
> LIB_H += unix-socket.h
> PROGRAM_OBJS += credential-cache.o
> PROGRAM_OBJS += credential-cache--daemon.o
> +else
> + BASIC_CFLAGS += -DNO_UNIX_SOCKETS
> endif
>
> ifdef NO_ICONV
> diff --git a/unix-socket.h b/unix-socket.h
> index e271aee..f1cba70 100644
> --- a/unix-socket.h
> +++ b/unix-socket.h
> @@ -1,7 +1,25 @@
> #ifndef UNIX_SOCKET_H
> #define UNIX_SOCKET_H
>
> +#ifndef NO_UNIX_SOCKETS
> +
> int unix_stream_connect(const char *path);
> int unix_stream_listen(const char *path);
>
> +#else
> +
> +static inline int unix_stream_connect(const char *path)
> +{
> + errno = ENOSYS;
> + return -1;
> +}
> +
> +static inline int unix_stream_listen(const char *path)
> +{
> + errno = ENOSYS;
> + return -1;
> +}
> +
> +#endif
> +
> #endif /* UNIX_SOCKET_H */
OK, so I missed this before my other two comments. But still... in
what way does errno=ENOSYS make this *work*? Won't we end up compiling
lots of non-functional tools on Windows in this case?
--
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 4/8] Add read-cache--daemon for caching index and related stuff
On Tue, May 13, 2014 at 1:15 PM, Nguyễn Thái Ngọc Duy wrote: > diff --git a/Documentation/git-read-cache--daemon.txt > b/Documentation/git-read-cache--daemon.txt > new file mode 100644 > index 000..1b05be4 > --- /dev/null > +++ b/Documentation/git-read-cache--daemon.txt > @@ -0,0 +1,27 @@ > +git-read-cache--daemon(1) > += > + > +NAME > + > +git-daemon - A simple cache server for speeding up index file access > + > +SYNOPSIS > + > +[verse] > +'git daemon' [--detach] > + Huh, "git daemon" can't be right... > diff --git a/Makefile b/Makefile > index d0a2b4b..a44ab0b 100644 > --- a/Makefile > +++ b/Makefile > @@ -1504,6 +1504,12 @@ ifdef HAVE_DEV_TTY > BASIC_CFLAGS += -DHAVE_DEV_TTY > endif > > +ifdef HAVE_SHM > + BASIC_CFLAGS += -DHAVE_SHM > + EXTLIBS += -lrt > + PROGRAM_OBJS += read-cache--daemon.o > +endif I think this also needs to be protected against NO_UNIX_SOCKETS -- 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 2/3] Add read-cache--daemon
On Tue, May 13, 2014 at 1:15 PM, Nguyễn Thái Ngọc Duy wrote:
> diff --git a/Makefile b/Makefile
> index 028749b..98d22de 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1502,6 +1502,12 @@ ifdef HAVE_DEV_TTY
> BASIC_CFLAGS += -DHAVE_DEV_TTY
> endif
>
> +ifdef HAVE_SHM
> + BASIC_CFLAGS += -DHAVE_SHM
> + EXTLIBS += -lrt
> + PROGRAM_OBJS += read-cache--daemon.o
> +endif
> +
I think read-cache--daemon will fail in case of NO_UNIX_SOCKETS.
But, read-cache--daemon.c only gets compiled if we have shm_open...
> diff --git a/git-compat-util.h b/git-compat-util.h
> index f6d3a46..b2116ab 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -723,4 +723,12 @@ struct tm *git_gmtime_r(const time_t *, struct tm *);
> #define gmtime_r git_gmtime_r
> #endif
>
> +#ifndef HAVE_SHM
> +static inline int shm_open(const char *path, int flags, int mode)
> +{
> + errno = ENOSYS;
> + return -1;
> +}
> +#endif
...yet, you introduce a compatibility-shim...
> diff --git a/read-cache--daemon.c b/read-cache--daemon.c
> new file mode 100644
> index 000..52b4067
> --- /dev/null
> +++ b/read-cache--daemon.c
> @@ -0,0 +1,167 @@
> +#include "cache.h"
> +#include "sigchain.h"
> +#include "unix-socket.h"
> +#include "split-index.h"
> +#include "pkt-line.h"
> +
> +static char *socket_path;
> +static struct strbuf shm_index = STRBUF_INIT;
> +static struct strbuf shm_sharedindex = STRBUF_INIT;
> +
> +static void cleanup_socket(void)
> +{
> + if (socket_path)
> + unlink(socket_path);
> + if (shm_index.len)
> + shm_unlink(shm_index.buf);
> + if (shm_sharedindex.len)
> + shm_unlink(shm_sharedindex.buf);
> +}
> +
> +static void cleanup_socket_on_signal(int sig)
> +{
> + cleanup_socket();
> + sigchain_pop(sig);
> + raise(sig);
> +}
> +
> +static void share_index(struct index_state *istate, struct strbuf *shm_path)
> +{
> + struct strbuf sb = STRBUF_INIT;
> + void *map;
> + int fd;
> +
> + strbuf_addf(&sb, "/git-index-%s", sha1_to_hex(istate->sha1));
> + if (shm_path->len && strcmp(sb.buf, shm_path->buf)) {
> + shm_unlink(shm_path->buf);
> + strbuf_reset(shm_path);
> + }
> + fd = shm_open(sb.buf, O_RDWR | O_CREAT | O_TRUNC, 0700);
> + if (fd < 0)
> + return;
...that only gets called from read-cache--daemon.c, which already only
gets compiled if we have open?
--
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: Bug - Git reset --quiet not quiet
On Tue, May 13, 2014 at 11:38 AM, Johannes Sixt wrote: > Am 5/13/2014 11:09, schrieb Erik Faye-Lund: >> On Mon, May 12, 2014 at 9:16 PM, Thomas-Louis Laforest >> wrote: >>> When running this command on Git for Windows (version 1.9.2-preview20140411) >>> git reset --quiet --hard with one file having read/write lock git ask this >>> question : >>> Unlink of file '' failed. Should I try again? (y/n) >>> >>> I will have expected the command --quiet to remove the question and >>> auto-answer no. >>> This broke an automated script we use. > ... >> I guess this could be solved in a few ways. >> 1) Let mingw_unlink() know about the quiet-flag. This probably >> involves moving the quiet-flag from each tool into libgit.a. >> 2) Make the quiet-flags close stdout instead of suppressing every output. >> 3) Make the higher level call-sites of Git EBUSY-aware. This probably >> involves making an interactive convenience-wrapper of unlink, that >> accepts a quiet flag - similar to what mingw_unlink does. > > Is any of this really needed? We have this in ask_yes_no_if_possible(): > > if (!isatty(_fileno(stdin)) || !isatty(_fileno(stderr))) > return 0; > > i.e., we answer "no" automatically without asking if at least one of stdin > and stderr are not a terminal. Perhaps the OP's problem is that they do > not redirect these channels to files or something in their automated > scripts? In particular, it should be sufficient to redirect stdin from > /dev/null (a.k.a. "nul" on Windows). Well, sure. But if sounds like surprising behavior to me. And I also suspect doing this unconditionally on all platforms will fix strange corner-cases on other systems, when using Windows file-systems. AFAIK, the whole "cannot delete an open file"-thing is an NTFS-detail (and possibly also FAT, which is quite commonly used on non-Windows). -- 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: Bug - Git reset --quiet not quiet
On Mon, May 12, 2014 at 9:16 PM, Thomas-Louis Laforest wrote: > Good afternoon, > > When running this command on Git for Windows (version 1.9.2-preview20140411) > git reset --quiet --hard with one file having read/write lock git ask this > question : > Unlink of file '' failed. Should I try again? (y/n) > > I will have expected the command --quiet to remove the question and > auto-answer no. > This broke an automated script we use. Thanks for reporting this. The problem here is really a nasty case of layering: the question comes from a place deep inside the OS compatibility layer, which doesn't know about the --quiet flag. However, do note that if fixed, the command would still fail under these conditions. But it won't hang forever, as it does now. Mainline Git-folks: The problem here is essentially unlink returning EBUSY (although that's not *exactly* what happes - but it's close enough, implementation details in mingw_unlink), which most of the git codebase assume won't happen. On Windows, this happens *all* the time, usually due to antivirus-software scanning a newly written file. We currently retry a few times with some waiting in mingw_unlink, and then finally prompts the user. But this gives the problem described above, as mingw_unlink has no clue about --quiet. I guess this could be solved in a few ways. 1) Let mingw_unlink() know about the quiet-flag. This probably involves moving the quiet-flag from each tool into libgit.a. 2) Make the quiet-flags close stdout instead of suppressing every output. 3) Make the higher level call-sites of Git EBUSY-aware. This probably involves making an interactive convenience-wrapper of unlink, that accepts a quiet flag - similar to what mingw_unlink does. Option 1) seems quite error-prone to me - it's difficult to make sure all code-paths actually set this flag, so there's a good chance of regressions. Option 2) also sounds a bit risky, as we lose stdout forever, with no escape-hatch. So to me option 3) seems preferable although it might translate into a bit of churn. Thoughts? -- 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 2/2] wincred: avoid overwriting configured variables
On Tue, May 13, 2014 at 8:01 AM, Stepan Kasal wrote: > From: Pat Thoyts > Date: Wed, 24 Oct 2012 00:15:29 +0100 > > Signed-off-by: Pat Thoyts > Signed-off-by: Stepan Kasal > --- > contrib/credential/wincred/Makefile | 4 > 1 file changed, 4 deletions(-) > > diff --git a/contrib/credential/wincred/Makefile > b/contrib/credential/wincred/Makefile > index 39fa5e0..e64cd9a 100644 > --- a/contrib/credential/wincred/Makefile > +++ b/contrib/credential/wincred/Makefile > @@ -1,9 +1,5 @@ > all: git-credential-wincred.exe > > -CC = gcc > -RM = rm -f > -CFLAGS = -O2 -Wall > - Would it be better to set these if not already set, i.e: -CC = gcc -RM = rm -f -CFLAGS = -O2 -Wall +CC ?= gcc +RM ?= rm -f +CFLAGS ?= -O2 -Wall instead? -- 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 v1 04/25] contrib: remove 'buildsystems'
On Fri, May 9, 2014 at 12:57 PM, Felipe Contreras wrote: > Erik Faye-Lund wrote: >> On Fri, May 9, 2014 at 11:32 AM, Felipe Contreras >> wrote: >> > Erik Faye-Lund wrote: >> >> On Fri, May 9, 2014 at 10:48 AM, Felipe Contreras >> >> > You think changing the execution bit of a file is considered "activity"? >> >> >> >> Well, now we're getting into semantics, which I don't care so much >> >> about. >> > >> > Convenient. >> >> Yeah, the part above here goes in my "don't argue with idiots, they'll >> drag you down to their level and beat you with experience"-filter. >> Good luck trying to convince *anyone* with this line of argumentation. > > It has been demonstrated that there is inactivity. The fact that your > semantics about "inactivity" differ from the rest of the world is > irrelevant. > >> > The script doesn't depend on the version of the Makefile, and proof of >> > that is that is has *never* been changed even though the Makefile has. >> >> Except it has, in 74cf9bd. > > Once change in *four* years. My god! How are people ever going to keep > up with such amount of changes if it moves out-of-tree! > It's rather amusing to see you react to my definition of "activity", when you seem to have a rather unusual definition of "never"... -- 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 v1 04/25] contrib: remove 'buildsystems'
On Fri, May 9, 2014 at 11:32 AM, Felipe Contreras wrote: > Erik Faye-Lund wrote: >> On Fri, May 9, 2014 at 10:48 AM, Felipe Contreras >> wrote: >> > Erik Faye-Lund wrote: >> >> On Fri, May 9, 2014 at 10:14 AM, Felipe Contreras >> >> wrote: >> >> > If you want this script to remain in contrib, please: >> >> > >> >> > a) Write at least a few tests >> >> > b) Write some documentation >> >> > c) Explain why it cannot live outside the git.git repository like other >> >> > tools. [1][2][3] >> >> >> >> (Adding Marius, the original author to the CC-list) >> >> >> >> Uh, why is such a burden required all of a sudden? contrib/README >> >> mentions no such requirements, and the scripts have been accepted (and >> >> maintained) since. >> > >> > contrib/README mentions clearly the expectation that these scripts >> > eventually move to the core once they mature. This is never going to >> > happen for these. >> >> Yes, *expectation*. Not requirement. > > That's right, but these tools fail all expectations. > >> > It also mentions that inactive ones would be proposed for removal, and >> > this one is clearly inactive. It has 9 commits (if you count the one >> > that changes the execution bit). >> >> It mentions that Junio *might* suggest things to be removed, not that >> things *should* be removed if left unmaintained. > > That's right. > >> And this script is not unmaintained, it's simply just still working. > > Prove it. > > Either way, if there was people actively caring about these scripts, > there should be cleanups, tests, documentation. But there's nothing. > >> >> Besides, you say "No activity since 2010" - this is not the case, >> >> bc380fc is from November 2013. >> > >> > You think changing the execution bit of a file is considered "activity"? >> >> Well, now we're getting into semantics, which I don't care so much >> about. > > Convenient. > Yeah, the part above here goes in my "don't argue with idiots, they'll drag you down to their level and beat you with experience"-filter. Good luck trying to convince *anyone* with this line of argumentation. >> It shows some sort of interest in the scripts, at least. > > Not it doesn't. Jonathan Nieder updated the execution bit on a bunch of > scripts in contrib, these being just in the way. It doesn't show > interest at all. > All of those changes relate to the MSVC-build. So it's not "just some batch-fixup" as you're trying to suggest. >> >> And there's already *some* documentation in the scripts themselves. >> > >> > That's nice. So you can just copy that into a README. >> >> Feel free to scratch that itch yourself, you're the one inventing new >> requirements here. > > If you care about these scripts, you have an interesting way of showing > it. > >> >> Please stop your pointless crusade that'll only break other people's >> >> work-flows. >> > >> > If you care about these scripts, it should be trivial for you to add at >> > least a few tests, souldn't it? >> >> Again, testing this is not my itch. Feel free to scratch that one >> yourself, but please don't remove the script. > > If you don't care that these scripts keep working properly, I don't see > why anybody else would. > You're the one making up requirements for tests here, so this is your itch. This script gets fixed by it's stake-holders when it breaks, and that has worked out fine so far. >> > Please tell me how exactly will your work-flow be broken. More >> > specifically, tell me why your scripts cannot be moved outside of git, >> > like git-extras[1], git-deploy[2], git-ftp[3], and countless other >> > tools. >> >> Moving the script out of the repo makes it less convenient to bisect >> issues with MSVC, as it depends heavily on the top-level Makefile. >> Moving it out would require figuring out what version of the script >> matches a given git revision, which is a hassle. > > The script doesn't depend on the version of the Makefile, and proof of > that is that is has *never* been changed even though the Makefile has. Except it has, in 74cf9bd. -- 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 v1 04/25] contrib: remove 'buildsystems'
On Fri, May 9, 2014 at 10:48 AM, Felipe Contreras wrote: > Erik Faye-Lund wrote: >> On Fri, May 9, 2014 at 10:14 AM, Felipe Contreras >> wrote: >> > If you want this script to remain in contrib, please: >> > >> > a) Write at least a few tests >> > b) Write some documentation >> > c) Explain why it cannot live outside the git.git repository like other >> > tools. [1][2][3] >> >> (Adding Marius, the original author to the CC-list) >> >> Uh, why is such a burden required all of a sudden? contrib/README >> mentions no such requirements, and the scripts have been accepted (and >> maintained) since. > > contrib/README mentions clearly the expectation that these scripts > eventually move to the core once they mature. This is never going to > happen for these. Yes, *expectation*. Not requirement. > It also mentions that inactive ones would be proposed for removal, and > this one is clearly inactive. It has 9 commits (if you count the one > that changes the execution bit). It mentions that Junio *might* suggest things to be removed, not that things *should* be removed if left unmaintained. And this script is not unmaintained, it's simply just still working. >> Besides, you say "No activity since 2010" - this is not the case, >> bc380fc is from November 2013. > > You think changing the execution bit of a file is considered "activity"? > Well, now we're getting into semantics, which I don't care so much about. It shows some sort of interest in the scripts, at least. >> And there's already *some* documentation in the scripts themselves. > > That's nice. So you can just copy that into a README. Feel free to scratch that itch yourself, you're the one inventing new requirements here. >> Please stop your pointless crusade that'll only break other people's >> work-flows. > > If you care about these scripts, it should be trivial for you to add at > least a few tests, souldn't it? Again, testing this is not my itch. Feel free to scratch that one yourself, but please don't remove the script. > Please tell me how exactly will your work-flow be broken. More > specifically, tell me why your scripts cannot be moved outside of git, > like git-extras[1], git-deploy[2], git-ftp[3], and countless other > tools. Moving the script out of the repo makes it less convenient to bisect issues with MSVC, as it depends heavily on the top-level Makefile. Moving it out would require figuring out what version of the script matches a given git revision, which is a hassle. Again, please stop this pointless crusade. -- 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 v1 04/25] contrib: remove 'buildsystems'
On Fri, May 9, 2014 at 10:14 AM, Felipe Contreras wrote: > Erik Faye-Lund wrote: >> On Fri, May 9, 2014 at 2:58 AM, Felipe Contreras >> wrote: >> > No activity since 2010, no documentation, no tests. >> > >> > Signed-off-by: Felipe Contreras >> > --- >> > contrib/buildsystems/Generators.pm| 42 -- >> > contrib/buildsystems/Generators/QMake.pm | 189 - >> > contrib/buildsystems/Generators/Vcproj.pm | 626 >> > -- >> > contrib/buildsystems/engine.pl| 359 - >> > contrib/buildsystems/generate | 29 -- >> > contrib/buildsystems/parse.pl | 228 --- >> > 6 files changed, 1473 deletions(-) >> > delete mode 100644 contrib/buildsystems/Generators.pm >> > delete mode 100644 contrib/buildsystems/Generators/QMake.pm >> > delete mode 100644 contrib/buildsystems/Generators/Vcproj.pm >> > delete mode 100755 contrib/buildsystems/engine.pl >> > delete mode 100755 contrib/buildsystems/generate >> > delete mode 100755 contrib/buildsystems/parse.pl >> >> Please don't. This script is useful to build with the MSVC IDE, which >> enables us to use their excellent debugger. > > If you want this script to remain in contrib, please: > > a) Write at least a few tests > b) Write some documentation > c) Explain why it cannot live outside the git.git repository like other > tools. [1][2][3] (Adding Marius, the original author to the CC-list) Uh, why is such a burden required all of a sudden? contrib/README mentions no such requirements, and the scripts have been accepted (and maintained) since. Besides, you say "No activity since 2010" - this is not the case, bc380fc is from November 2013. And there's already *some* documentation in the scripts themselves. Please stop your pointless crusade that'll only break other people's work-flows. -- 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 v1 04/25] contrib: remove 'buildsystems'
On Fri, May 9, 2014 at 2:58 AM, Felipe Contreras wrote: > No activity since 2010, no documentation, no tests. > > Signed-off-by: Felipe Contreras > --- > contrib/buildsystems/Generators.pm| 42 -- > contrib/buildsystems/Generators/QMake.pm | 189 - > contrib/buildsystems/Generators/Vcproj.pm | 626 > -- > contrib/buildsystems/engine.pl| 359 - > contrib/buildsystems/generate | 29 -- > contrib/buildsystems/parse.pl | 228 --- > 6 files changed, 1473 deletions(-) > delete mode 100644 contrib/buildsystems/Generators.pm > delete mode 100644 contrib/buildsystems/Generators/QMake.pm > delete mode 100644 contrib/buildsystems/Generators/Vcproj.pm > delete mode 100755 contrib/buildsystems/engine.pl > delete mode 100755 contrib/buildsystems/generate > delete mode 100755 contrib/buildsystems/parse.pl Please don't. This script is useful to build with the MSVC IDE, which enables us to use their excellent debugger. -- 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] t3910: show failure of core.precomposeunicode with decomposed filenames
On Mon, May 5, 2014 at 11:46 PM, Jeff King wrote: > On Sun, May 04, 2014 at 08:13:15AM +0200, Torsten Bögershausen wrote: > >> > 1. Tell everyone that NFD in the git repo is wrong, and >> > they should make a new commit to normalize all their >> > in-repo files to be precomposed. >> > This is probably not the right thing to do, because it >> > still doesn't fix checkouts of old history. And it >> > spreads the problem to people on byte-preserving >> > filesystems (like ext4), because now they have to start >> > precomposing their filenames as they are adde to git. >> (typo: >> ^added) >> I'm not sure if I follow. People running ext4 (or Linux in general, >> or Windows, or Unix) do not suffer from file system >> "feature" of Mac OS, which accepts precomposed/decomposed Unicode >> but returns decompomsed. > > What I mean by "spreads the problem" is that git on Linux does not need > to care about utf8 at all. It treats filenames as a byte sequence. But > if we were to start enforcing "filenames should be precomposed utf8", > then people adding files on Linux would want to enforce that, too. > > People on Linux could ignore the issue as they do now, but they would > then create problems for OS X users if they add decomposed filenames. > IOW, if the OS X code assumes "all repo filenames are precomposed", then > other systems become a possible vector for violating that assumption. FWIW, Git for Windows also doesn't deal with that "filenames are just a byte-sequence"-notion. We have patches in place that require filenames to *either* be valid UTF-8 or Windows-1252. Windows itself treats filenames as Unicode characters, not arbitrary byte sequences. -- 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/RFC] Makefile: do not depend on curl-config
On Wed, Apr 30, 2014 at 9:46 PM, Sebastian Schuberth wrote: > On Wed, Apr 30, 2014 at 6:52 PM, Johannes Schindelin > wrote: > >>> We can keep this patch in the msysGit repo for the 2.0 release. >> >> FWIW the plan is to switch to mingwGitDevEnv for the 2.0 release. It is >> not quite clear as of yet how patches will be managed with said >> environment. > > The environment is just that: The environment to build Git for > Windows. This means that patches on top of Git for Windows could still > be maintained in msysgit/git (or a fork thereof) on GitHub. Thanks for the heads up. Even so, are you guys OK with me pushing this patch to our downstream repo? -- 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: [msysGit] Re: #178 parsing of pretty=format:"%an %ad" causes fatal: bad revision '%ad'
On Fri, May 2, 2014 at 7:23 PM, Jonathan Nieder wrote: > (resending with the correct address for the Git for Windows developers. > Sorry for the noise.) > Hi Dave, > > Dave Bradley wrote: > >> G:\ws_test_env\GIT_TESTBED_TMP\fest-swing-1.x>git log --all >> --pretty=format:"%an %ad" -- pom.xml >> Mon Nov 23 03:09:17 2009 + >> Mon Nov 23 02:42:24 2009 + >> >> G:\ws_test_env\GIT_TESTBED_TMP\fest-swing-1.x>git log --all >> "--pretty=format:"%an %ad"" -- pom.xml >> fatal: bad revision '%ad' > > On Linux, this example gets passed to git as six arguments: > > log > --all > --pretty=format:%an > %ad > -- > pom.xml > As does it on Windows. -- 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: #178 parsing of pretty=format:"%an %ad" causes fatal: bad revision '%ad'
On Fri, May 2, 2014 at 1:50 PM, Dave Bradley wrote: > Hi, > > I’m very new to ‘git’ github. I reported the #178 issue in github and the > issue has been closed, I believe this means no further discussion. When you say "the #178 issue in github", you really mean "issue #178 for Git for Windows on GitHub", available at https://github.com/msysgit/git/issues/178 for those interested. That issue tracker is for the Windows port of Git for Windows. It's intended to track breakages in Git for Windows compared to Git on, say Linux. It's not a general issue tracker for problems with Git. Still, it seems a lot of people think "I downloaded Git for Windows, and here's something that didn't work the way I expected it to, I'll file a bug". Those kinds of bug-reports usually gets closed quickly, as it's outside the scope of Git for Windows to decide how Git should behave - we try to make it behave consistently between Windows and Unixy-platforms. This is indeed the right forum to address your issue. So thank you for moving the discussion here. -- 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 v2] Makefile: default to -lcurl when no CURL_CONFIG or CURLDIR
On Wed, Apr 30, 2014 at 5:13 PM, Junio C Hamano wrote: > I think something along the lines of > > ifdef CURLDIR > CURL_LIBCURL = > else > CURL_CONFIG = curl-config > CURL_LIBCURL := $(shell sh -c '$(CURL_CONFIG) --libs' > 2>/dev/null) > fi > > may be the right way to write this? > > Note that $(shell $(CURL_CONFIG) --libs) when CURL_CONFIG is empty > would barf when $(CURL_CONFIG) expands to an empty string. There's still the fact that msysGit does *not* need CURLDIR, but doesn't have curl-config either. So I think this one will also complain for us. -- 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/RFC] Makefile: do not depend on curl-config
On Wed, Apr 30, 2014 at 5:27 PM, Junio C Hamano wrote:
> Erik Faye-Lund writes:
>
>> MinGW builds of cURL does not ship with curl-config unless built
>> with the autoconf based build system, which is not the practice
>> recommended by the documentation. MsysGit has had issues with
>> binaries of that sort, so it has switched away from autoconf-based
>> cURL-builds.
>>
>> Unfortunately, broke pushing over WebDAV on Windows, because
>> http-push.c depends on cURL's multi-threaded API, which we could
>> not determine the presence of any more.
>>
>> Since troublesome curl-versions are ancient, and not even present
>> in RedHat 5, let's just assume cURL is capable instead of doing a
>> non-robust check.
>>
>> Instead, add a check for curl_multi_init to our configure-script,
>> for those on ancient system. They probably already need to do the
>> configure-dance anyway.
>>
>> Signed-off-by: Erik Faye-Lund
>> ---
>>
>> OK, here's a proper patch. I've even tested it! ;)
>>
>>
>> Makefile | 8 +++-
>> configure.ac | 11 +++
>> 2 files changed, 14 insertions(+), 5 deletions(-)
>>
>> diff --git a/Makefile b/Makefile
>> index e90f57e..f6b5847 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -1133,13 +1133,11 @@ else
>> REMOTE_CURL_NAMES = $(REMOTE_CURL_PRIMARY) $(REMOTE_CURL_ALIASES)
>> PROGRAM_OBJS += http-fetch.o
>> PROGRAMS += $(REMOTE_CURL_NAMES)
>> - curl_check := $(shell (echo 070908; curl-config --vernum) 2>/dev/null
>> | sort -r | sed -ne 2p)
>> - ifeq "$(curl_check)" "070908"
>> - ifndef NO_EXPAT
>> + ifndef NO_EXPAT
>> + ifndef NO_CURL_MULTI
>> PROGRAM_OBJS += http-push.o
>> endif
>
> Dave's "ask curl-config about proper compilation options if
> available" and this change does *not* semantically conflict, as the
> former should stress "if available" part (but note that the key word
> is "should").
>
> How old/battle tested is this change?
Not very. I've successfully tested it on two systems, msysGit and RedHat 5.
> My inclination at this point
> is to revert the merge of Dave's series from 2.0 (yes, I know we
> have been looking at fixing it and I _think_ the issue of unpleasant
> error message you reported can be fixed, but at this rate we would
> not know if we eradicated all the issues for platforms and distros
> with confidence by the final), kick it back to 'next'/'pu', and
> queue this change also in 'next'/'pu' and cook them so that we can
> merge them early in the 2.1 cycle.
Sounds like a sensible plan to me. We can keep this patch in the
msysGit repo for the 2.0 release.
> This new makefile variable needs a comment at the top, no?
>
> Makefile | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/Makefile b/Makefile
> index f7d33da..3164b62 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -34,6 +34,10 @@ all::
> # git-http-push are not built, and you cannot use http:// and https://
> # transports (neither smart nor dumb).
> #
> +# Define NO_CURL_MULTI if your libcurl is sufficiently old and lacks
> +# curl_multi_init ("git http-push" to run the deprecated dumb push over
> +# http/webdab will not be built).
> +#
> # Define CURL_CONFIG to the path to a curl-config binary other than the
> # default 'curl-config'. If CURL_CONFIG is unset or points to a binary that
> # is not found, defaults to the CURLDIR behavior.
Ah yes. Sorry for missing that. Perhaps the text should even mention
the old break-off version, that is curl > v7.9.8 as far as I can
tell...?
--
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/RFC] Makefile: do not depend on curl-config
On Mon, Apr 28, 2014 at 6:29 PM, Erik Faye-Lund wrote:
> MinGW builds of cURL does not ship with curl-config unless built
> with the autoconf based build system, which is not the practice
> recommended by the documentation. MsysGit has had issues with
> binaries of that sort, so it has switched away from autoconf-based
> cURL-builds.
>
> Unfortunately, broke pushing over WebDAV on Windows, because
> http-push.c depends on cURL's multi-threaded API, which we could
> not determine the presence of any more.
>
> Since troublesome curl-versions are ancient, and not even present
> in RedHat 5, let's just assume cURL is capable instead of doing a
> non-robust check.
>
> Instead, add a check for curl_multi_init to our configure-script,
> for those on ancient system. They probably already need to do the
> configure-dance anyway.
>
> Signed-off-by: Erik Faye-Lund
Junio,
this patch still applies, and is required on Git for Windows for
http-push to work, even after f3f11fa ("Makefile: default to -lcurl
when no CURL_CONFIG or CURLDIR").
--
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 v2] Makefile: default to -lcurl when no CURL_CONFIG or CURLDIR
On Mon, Apr 28, 2014 at 11:01 PM, Dave Borowitz wrote: > The original implementation of CURL_CONFIG support did not match the > original behavior of using -lcurl when CURLDIR was not set. This broke > implementations that were lacking curl-config but did have libcurl > installed along system libraries, such as MSysGit. In other words, the > assumption that curl-config is always installed was incorrect. > > Instead, if CURL_CONFIG is empty or returns an empty result (e.g. due > to curl-config being missing), use the old behavior of falling back to > -lcurl. > > Signed-off-by: Dave Borowitz > --- > Makefile | 41 - > 1 file changed, 28 insertions(+), 13 deletions(-) > > diff --git a/Makefile b/Makefile > index 74a929b..81e8214 100644 > --- a/Makefile > +++ b/Makefile > @@ -35,14 +35,17 @@ all:: > # transports (neither smart nor dumb). > # > # Define CURL_CONFIG to the path to a curl-config binary other than the > -# default 'curl-config'. > +# default 'curl-config'. If CURL_CONFIG is unset or points to a binary that > +# is not found, defaults to the CURLDIR behavior. > # > # Define CURL_STATIC to statically link libcurl. Only applies if > # CURL_CONFIG is used. > # > # Define CURLDIR=/foo/bar if your curl header and library files are in > -# /foo/bar/include and /foo/bar/lib directories. This overrides CURL_CONFIG, > -# but is less robust. > +# /foo/bar/include and /foo/bar/lib directories. This overrides > +# CURL_CONFIG, but is less robust. If not set, and CURL_CONFIG is not set, > +# uses -lcurl with no additional library detection (other than > +# NEEDS_*_WITH_CURL). This is wrong, no? With CURL_CONFIG not set, it currently *does* run curl-config, see below. > # > # Define NO_EXPAT if you do not have expat installed. git-http-push is > # not built, and you cannot push using http:// and https:// transports > (dumb). > @@ -1127,9 +1130,27 @@ ifdef NO_CURL > REMOTE_CURL_NAMES = > else > ifdef CURLDIR > - # Try "-Wl,-rpath=$(CURLDIR)/$(lib)" in such a case. > - BASIC_CFLAGS += -I$(CURLDIR)/include > - CURL_LIBCURL = -L$(CURLDIR)/$(lib) > $(CC_LD_DYNPATH)$(CURLDIR)/$(lib) -lcurl > + CURL_LIBCURL = > + else > + CURL_CONFIG = curl-config > + ifeq "$(CURL_CONFIG)" "" > + CURL_LIBCURL = > + else > + CURL_LIBCURL := $(shell $(CURL_CONFIG) --libs) > + endif Doesn't that definition just define CURL_CONFIG unconditionally? How are the first condition ever supposed to get triggered? $ make make: curl-config: Command not found GIT_VERSION = 1.9.2.462.gf3f11fa make: curl-config: Command not found * new build flags * new link flags * new prefix flags GEN common-cmds.h ... Yuck. -- 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] wincred: add install target and avoid overwriting configured variables
On Wed, Apr 30, 2014 at 1:27 PM, Stepan Kasal wrote: > Hello, > >> On Wed, Apr 30, 2014 at 8:46 AM, Stepan Kasal wrote: >> > Date: Wed, 24 Oct 2012 00:15:29 +0100 >> > >> > Signed-off-by: Pat Thoyts >> > Signed-off-by: Stepan Kasal >> > --- >> > Another one from msysGit project. >> > Original subject by Pat; I would suggest: >> > wincred: improve Makefile > > On Wed, Apr 30, 2014 at 11:21:17AM +0200, Erik Faye-Lund wrote: >> I'm a little bit unsure about this, because the makefile was basically >> just copied from contrib/credential/osxkeychain/Makefile (which was >> the first credential helper) and tweaked slightly. >> >> So, what makes wincred special compared to gnome-keyring, netrc and >> osxkeychain wrt installation? Shouldn't all helpers get the same >> treatment? > > I can only guess that the hardwired CC and CFLAGS values can cause > problems. I doubt that a patch that doesn't describe exactly what kind of issues will get merged. And it certainly won't get my ack unless I understand why. > It is probably much sane on Windows to use the compiler that the user > set up for the build. But you can already do that, the same way as for the rest of git, by overloading these in config.mak in the root of the git repo. > I'm not sure if users of, say, OS X, have the same problems, so I > would hesitate to apply these changes to all helpers. Even if I bought that it's needed (which I'm currently skeptical to), I think the "dunno about OSX" is a bit of a cop-out. >> > From: Pat Thoyts >> > contrib/credential/wincred/Makefile | 22 ++ >> > 1 file changed, 14 insertions(+), 8 deletions(-) >> > >> > diff --git a/contrib/credential/wincred/Makefile >> > b/contrib/credential/wincred/Makefile >> > index bad45ca..3ce6aba 100644 >> > --- a/contrib/credential/wincred/Makefile >> > +++ b/contrib/credential/wincred/Makefile >> > @@ -1,14 +1,20 @@ >> > -all: git-credential-wincred.exe >> > - >> > -CC = gcc >> > -RM = rm -f >> > -CFLAGS = -O2 -Wall >> > - >> > -include ../../../config.mak.autogen >> > -include ../../../config.mak >> > >> > -git-credential-wincred.exe : git-credential-wincred.c >> > +prefix ?= /usr/local >> > +libexecdir ?= $(prefix)/libexec/git-core >> > + >> > +INSTALL ?= install >> > + >> > +GIT_CREDENTIAL_WINCRED := git-credential-wincred.exe >> >> Why this variable? IMO, it's just as "GIT_CREDENTIAL_WINCRED" easy to >> miss-spell as "git-credential-wincred.exe", and it doesn't seem to be >> possible to overload. > > If you mis-spell a variable name, nothing is build. If you misspell > a binary name, that binary may get compiled using a default rule; > that is why I would generally prefer variables. Following that logic, you should be submitting similar patches to the main git Makefile as well. Somehow I doubt that'll happen. > Moreover, if the cardinality of the set ever increases, the > indirection may get helpful. I don't think there's any reason to expect the number of binaries to increase, so that's moot. And if I'm wrong, let's deal with that when the time comes. It's not like this version is prepared for the variable being a list either - neither should there IMO. >> > + >> > +all: $(GIT_CREDENTIAL_WINCRED) >> > + >> >> Also, why move the all-target down from the top? Is it simply because >> of the definition above? > > Again, I agree with Pat that it is nicer this way, but no big > deal. We don't usually do "this is subjectively better"-patches in Git. Instead we try to follow the current style. -- 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] wincred: add install target and avoid overwriting configured variables
On Wed, Apr 30, 2014 at 8:46 AM, Stepan Kasal wrote: > From: Pat Thoyts > Date: Wed, 24 Oct 2012 00:15:29 +0100 > > Signed-off-by: Pat Thoyts > Signed-off-by: Stepan Kasal > --- > Another one from msysGit project. > Original subject by Pat; I would suggest: > wincred: improve Makefile I'm a little bit unsure about this, because the makefile was basically just copied from contrib/credential/osxkeychain/Makefile (which was the first credential helper) and tweaked slightly. So, what makes wincred special compared to gnome-keyring, netrc and osxkeychain wrt installation? Shouldn't all helpers get the same treatment? > contrib/credential/wincred/Makefile | 22 ++ > 1 file changed, 14 insertions(+), 8 deletions(-) > > diff --git a/contrib/credential/wincred/Makefile > b/contrib/credential/wincred/Makefile > index bad45ca..3ce6aba 100644 > --- a/contrib/credential/wincred/Makefile > +++ b/contrib/credential/wincred/Makefile > @@ -1,14 +1,20 @@ > -all: git-credential-wincred.exe > - > -CC = gcc > -RM = rm -f > -CFLAGS = -O2 -Wall > - > -include ../../../config.mak.autogen > -include ../../../config.mak > > -git-credential-wincred.exe : git-credential-wincred.c > +prefix ?= /usr/local > +libexecdir ?= $(prefix)/libexec/git-core > + > +INSTALL ?= install > + > +GIT_CREDENTIAL_WINCRED := git-credential-wincred.exe Why this variable? IMO, it's just as "GIT_CREDENTIAL_WINCRED" easy to miss-spell as "git-credential-wincred.exe", and it doesn't seem to be possible to overload. > + > +all: $(GIT_CREDENTIAL_WINCRED) > + Also, why move the all-target down from the top? Is it simply because of the definition above? -- 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 08/12] MINGW: fix main() signature in http-fetch.c and remote-curl.c
On Wed, Apr 30, 2014 at 10:35 AM, Karsten Blees wrote: > Am 29.04.2014 11:12, schrieb Marat Radchenko: >> On MinGW, compat/mingw.h defines a 'mingw_main' wrapper function. >> Fix `warning: passing argument 2 of 'mingw_main' from incompatible >> pointer type` in http-fetch.c and remote-curl.c by dropping 'const'. >> > > Would you mind cross checking your changes with the msysgit fork? Fixing the > same thing in a slightly different manner will just cause unnecessary > conflicts (i.e. unnecessary work for the Git for Windows maintainers, as well > as for you to come up with an alternate solution for something that's long > been fixed). > > See https://github.com/msysgit/git/commit/9206e7fd (squashed from > https://github.com/msysgit/git/commit/0115ef83 and > https://github.com/msysgit/git/commit/6949537a). > While it's certainly a good point, I think this is *our* fault for not upstreaming our changes, and the responsibility of cleaning up merges should lie on our shoulders. We've diverged a lot. Sure, Dscho does a great job making the divergence less painful, but IMO we should try to reduce the delta as well. For instance, I think the Unicode-support patches have proven themselves by now and should go upstream. And there's probably a ton of smaller free-standing clean-ups that could get the same treatment. -- 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 03/12] MINGW: compat/mingw.h: do not attempt to redefine lseek on mingw-w64
On Mon, Apr 28, 2014 at 9:59 PM, Torsten Bögershausen wrote: > On 2014-04-28 15.51, Marat Radchenko wrote: >> mingw-w64 has lseek defined in io.h. > [] >> #define off_t off64_t >> +#ifndef lseek >> #define lseek _lseeki64 >> +#endif > Is the commit message in line with the code? > > I would have expected something in this style: > > #if defined(__x86_64__) && ! defined(lseek)) > #include > #endif No, we want 64-bit off_t either way, to support large files. So we need the version that takes a 64-bit argument. -- 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: git version 1.9.0 missing git-http-push?
On Mon, Apr 28, 2014 at 8:39 PM, Dave Borowitz wrote: > On Mon, Apr 28, 2014 at 11:31 AM, Junio C Hamano wrote: >> Erik Faye-Lund writes: >> >>> We're using Curl 7.30.0 in msysGit (and thus also Git for Windows), so >>> we should be able to include it. However, we do not have curl-config >>> installed. >> >> Hmmm, between 2.0-rc0 and 2.0-rc1 there is 61a64fff (Makefile: use >> curl-config to determine curl flags, 2014-04-15) merged already, >> which makes this assumption and a claim based on that assumption: >> >> curl-config should always be installed alongside a curl >> distribution, and its purpose is to provide flags for building >> against libcurl, so use it instead of guessing flags and >> dependent libraries. >> >> which may make things worse for you guys, unless you are explicitly >> setting CURLDIR and other Makefile variables. > > Yes, I can see that assumption may not always hold. But I'm slightly > surprised this worked in the first place; are you saying this is a > system where: > -curl-config is not installed > -but -lcurl still works > -without setting CURLDIR > ? Yes, our cURL is installed globally together with the system libraries. > I think I should probably re-roll the patch to default to the old > behavior (blind -lcurl) if curl-config returns the empty string, which > I believe is also the case when the binary is not found. That sounds like it would work for MsysGit, yeah. -- 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/RFC] Makefile: do not depend on curl-config
MinGW builds of cURL does not ship with curl-config unless built with the autoconf based build system, which is not the practice recommended by the documentation. MsysGit has had issues with binaries of that sort, so it has switched away from autoconf-based cURL-builds. Unfortunately, broke pushing over WebDAV on Windows, because http-push.c depends on cURL's multi-threaded API, which we could not determine the presence of any more. Since troublesome curl-versions are ancient, and not even present in RedHat 5, let's just assume cURL is capable instead of doing a non-robust check. Instead, add a check for curl_multi_init to our configure-script, for those on ancient system. They probably already need to do the configure-dance anyway. Signed-off-by: Erik Faye-Lund --- OK, here's a proper patch. I've even tested it! ;) Makefile | 8 +++- configure.ac | 11 +++ 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/Makefile b/Makefile index e90f57e..f6b5847 100644 --- a/Makefile +++ b/Makefile @@ -1133,13 +1133,11 @@ else REMOTE_CURL_NAMES = $(REMOTE_CURL_PRIMARY) $(REMOTE_CURL_ALIASES) PROGRAM_OBJS += http-fetch.o PROGRAMS += $(REMOTE_CURL_NAMES) - curl_check := $(shell (echo 070908; curl-config --vernum) 2>/dev/null | sort -r | sed -ne 2p) - ifeq "$(curl_check)" "070908" - ifndef NO_EXPAT + ifndef NO_EXPAT + ifndef NO_CURL_MULTI PROGRAM_OBJS += http-push.o endif - endif - ifndef NO_EXPAT + ifdef EXPATDIR BASIC_CFLAGS += -I$(EXPATDIR)/include EXPAT_LIBEXPAT = -L$(EXPATDIR)/$(lib) $(CC_LD_DYNPATH)$(EXPATDIR)/$(lib) -lexpat diff --git a/configure.ac b/configure.ac index 2f43393..e7ef9f7 100644 --- a/configure.ac +++ b/configure.ac @@ -513,6 +513,17 @@ AC_CHECK_LIB([curl], [curl_global_init], [NO_CURL=], [NO_CURL=YesPlease]) +if test -z "$NO_CURL"; then + +AC_CHECK_DECLS([curl_multi_init], +[NO_CURL_MULTI=], +[NO_CURL_MULTI=UnfortunatelyYes], +[[#include ]]) + +GIT_CONF_SUBST([NO_CURL_MULTI]) + +fi + GIT_UNSTASH_FLAGS($CURLDIR) GIT_CONF_SUBST([NO_CURL]) -- 1.9.2.msysgit.2 -- 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 08/12] MINGW: config.mak.uname allow using CURL for non-msysGit builds
On Mon, Apr 28, 2014 at 6:23 PM, Marat Radchenko wrote: > On Mon, Apr 28, 2014 at 05:26:38PM +0200, Erik Faye-Lund wrote: >> On Mon, Apr 28, 2014 at 3:51 PM, Marat Radchenko >> wrote: >> > Also, fix `warning: passing argument 2 of 'mingw_main' from >> > incompatible pointer type` in http-fetch.c and remote-curl.c. >> >> These seems completely unrelated, perhaps it should be split in two? > > Okay, will split. Though there is a connection - until this patch, > http-fetch.c and remote-curl.c never built for MinGW. Ahh, thanks for pointing that out. But yeah, I think splitting is still the right option. -- 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 v2] Sleep 1 millisecond in poll() to avoid busy wait
On Mon, Apr 28, 2014 at 5:35 PM, Stepan Kasal wrote: > From: theoleblond > Date: Wed, 16 May 2012 06:52:49 -0700 > > SwitchToThread() only gives away the rest of the current time slice > to another thread in the current process. So if the thread that feeds > the file decscriptor we're polling is not in the current process, we > get busy-waiting. > > I played around with this quite a bit. After trying some more complex > schemes, I found that what worked best is to just sleep 1 millisecond > between iterations. Though it's a very short time, it still completely > eliminates the busy wait condition, without hurting perf. > > There code uses SleepEx(1, TRUE) to sleep. See this page for a good > discussion of why that is better than calling SwitchToThread, which > is what was used previously: > http://stackoverflow.com/questions/1383943/switchtothread-vs-sleep1 > > Note that calling SleepEx(0, TRUE) does *not* solve the busy wait. > > The most striking case was when testing on a UNC share with a large repo, > on a single CPU machine. Without the fix, it took 4 minutes 15 seconds, > and with the fix it took just 1:08! I think it's because git-upload-pack's > busy wait was eating the CPU away from the git process that's doing the > real work. With multi-proc, the timing is not much different, but tons of > CPU time is still wasted, which can be a killer on a server that needs to > do bunch of other things. > > I also tested the very fast local case, and didn't see any measurable > difference. On a big repo with 4500 files, the upload-pack took about 2 > seconds with and without the fix. > --- > > On Mon, Apr 28, 2014 at 05:05:47PM +0200, Johannes Sixt wrote: >> [...] but I very much prefer the commit message of the earlier post. > > ... but Paolo had a nice short description of the issue there; > I inserted that to the top of the earlier commit message. > > The latter diff (without the comment), gets us closer to gnulib's poll.c. > Good stuff! -- 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 06/12] MSVC: config.mak.uname: drop -D__USE_MINGW_ACCESS from compile definitions
On Mon, Apr 28, 2014 at 3:51 PM, Marat Radchenko wrote: > -D__USE_MINGW_ACCESS only affects MinGW and does nothing when > MSVC is used. Seems reasonable in itself. But, doesn't this mean that our access are currently broken on MSVC? The comment about __USE_MINGW_ACCESS is: /* Old versions of MSVCRT access() just ignored X_OK, while the version shipped with Vista, returns an error code. This will restore the old behaviour */ This sounds like we should lift the access-fix up one abstraction, into Git. But wait a minute. In Git for Windows, we already wrapped up access for unicode-support (03a102ff - "Win32: Unicode file name support (except dirent)"), doing the exact same thing already. This patch isn't upstreamed yet, though. Sounds like there's some cleaning up left to do on our behalf :) This clean-up makes sense regardless, though. -- 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: [RFC/PATCH v1] Towards MinGW(-W64) cross-compilation
On Mon, Apr 28, 2014 at 3:51 PM, Marat Radchenko wrote: > This patch series fixes building on modern MinGW and (32bit only yet) > MinGW-W64. > > *Compilation* tested on: > - MSVC (via WinGit environment) > - msysGit environment > - Linux cross-toolchain i686-pc-mingw32 (4.8.2) with mingw-runtime-3.20.2 > - Linux cross-toolchain i686-w64-mingw32 (4.8.2) with mingw64-runtime-3.1.0 > > Stuff still required to make Git build with x86_64 MinGW-W64 toolchain: > > 1. Drop -D_USE_32BIT_TIME_T that was added in fa93bb to config.mak.uname > because time_t cannot be 32bit on x86_64. I haven't yet figured out what > should break if this define is removed (pointers are welcome) and why it was > added in the first place. > > 2. Stop passing --large-address-aware to linker. I wonder if it does anything > for 32bit MinGW builds. > > 3. Fix several places with mismatched pointer size casts. > > Building it from Gentoo Linux: > > MinGW: > > crossdev -t i686-pc-mingw32 > ARCH=x86 emerge-i686-pc-mingw32 -u dev-libs/libiconv sys-libs/zlib > net-misc/curl sys-devel/gettext expat > cd > make CROSS_COMPILE=i686-pc-mingw32- CC=i686-pc-mingw32-gcc NO_OPENSSL=1 > MINGW=1 CURLDIR=/usr/i686-pc-mingw32/usr > > MinGW-W64 (32 bit): > > crossdev -t i686-w64-mingw32 > ARCH=x86 emerge-i686-w64-mingw32 -u dev-libs/libiconv sys-libs/zlib > net-misc/curl sys-devel/gettext expat > cd > make CROSS_COMPILE=i686-w64-mingw32- CC=i686-w64-mingw32-gcc NO_OPENSSL=1 > MINGW=1 CURLDIR=/usr/i686-w64-mingw32/usr > > Debian/Ubuntu build instructions are WIP (xdeb is non-trivial at all). Apart from some minor nits, this looks good to me. Thanks a lot for spending the time, and I look forward to a second iteration! -- 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 08/12] MINGW: config.mak.uname allow using CURL for non-msysGit builds
On Mon, Apr 28, 2014 at 3:51 PM, Marat Radchenko wrote: > Also, fix `warning: passing argument 2 of 'mingw_main' from > incompatible pointer type` in http-fetch.c and remote-curl.c. These seems completely unrelated, perhaps it should be split in two? -- 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 11/12] MINGW: do not fail at redefining pid_t on MinGW-W64
On Mon, Apr 28, 2014 at 3:51 PM, Marat Radchenko wrote:
> pid_t is available in sys/types.h on both MinGW and MinGW-W64
>
> Signed-off-by: Marat Radchenko
> ---
> compat/mingw.h | 1 -
> compat/msvc.h | 2 ++
> 2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/compat/mingw.h b/compat/mingw.h
> index c502a22..8850109 100644
> --- a/compat/mingw.h
> +++ b/compat/mingw.h
> @@ -5,7 +5,6 @@
> * things that are not available in header files
> */
>
> -typedef int pid_t;
> typedef int uid_t;
> typedef int socklen_t;
> #define hstrerror strerror
> diff --git a/compat/msvc.h b/compat/msvc.h
> index cb41ce3..e2fda48 100644
> --- a/compat/msvc.h
> +++ b/compat/msvc.h
> @@ -18,6 +18,8 @@
> #define PRIuMAX "I64u"
> #define PRId64 "I64d"
>
> +typedef int pid_t;
> +
> static __inline int strcasecmp (const char *s1, const char *s2)
> {
> int size1 = strlen(s1);
Looks good!
Acked-by: Erik Faye-Lund
--
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 10/12] MINGW: config.mak.uname: drop USE_NED_ALLOCATOR
On Mon, Apr 28, 2014 at 3:51 PM, Marat Radchenko wrote: > nedalloc was initially added in f0ed82 to fix slowness of standard WinXP > memory allocator. Since WinXP is EOLed, this point is no longer valid. > > The actual reason behind this commit is incompatibility of malloc.c.h > with MinGW-W64 headers. Alternative solution implies updating nedalloc > to something newer. Did you measure that malloc on newer Windows-versions are actually faster? AFAIK, malloc does a lot more inside the CRT than in the kernel... -- 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 12/12] MINGW: compat/mingw.h: drop fork() definition
On Mon, Apr 28, 2014 at 3:51 PM, Marat Radchenko wrote:
> fork() is not used in MinGW builds but causes a compiler warning
> on x86_64 MinGW-W64: conflicting types for built-in function 'fork'
>
> Signed-off-by: Marat Radchenko
> ---
> compat/mingw.h | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/compat/mingw.h b/compat/mingw.h
> index 8850109..2fbc8ea 100644
> --- a/compat/mingw.h
> +++ b/compat/mingw.h
> @@ -89,8 +89,6 @@ static inline int symlink(const char *oldpath, const char
> *newpath)
> { errno = ENOSYS; return -1; }
> static inline int fchmod(int fildes, mode_t mode)
> { errno = ENOSYS; return -1; }
> -static inline pid_t fork(void)
> -{ errno = ENOSYS; return -1; }
> static inline unsigned int alarm(unsigned int seconds)
> { return 0; }
> static inline int fsync(int fd)
I've been using a similar patch for a while, so:
Acked-by: Erik Faye-Lund
--
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 03/12] MINGW: compat/mingw.h: do not attempt to redefine lseek on mingw-w64
On Mon, Apr 28, 2014 at 5:09 PM, Marat Radchenko wrote: > On Mon, Apr 28, 2014 at 05:02:09PM +0200, Erik Faye-Lund wrote: >> msysGit has a declaration of it in io.h as well. But it's not a >> preprocessor-definition... Are you saying that it's a >> preprocessor-define in mingw-w64, that points to a 64-bit version? If >> so, looks good. > > MinGW is x86 only. > > MinGW-W64, a separate project, provides both x86 and x86_64. > > And here's relevant part of io.h from MingW-W64: > http://sourceforge.net/apps/trac/mingw-w64/browser/trunk/mingw-w64-headers/crt/io.h?rev=5437#L321 It looks like Line 335 is the real goodie: https://sourceforge.net/apps/trac/mingw-w64/browser/trunk/mingw-w64-headers/crt/io.h?rev=5437#L335 #define lseek lseek64 So yeah, looks good to me. -- 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 07/12] MINGW: config.mak.uname: reorganize MINGW settings
On Mon, Apr 28, 2014 at 5:04 PM, Marat Radchenko wrote: > On Mon, Apr 28, 2014 at 04:58:11PM +0200, Erik Faye-Lund wrote: >> On Mon, Apr 28, 2014 at 3:51 PM, Marat Radchenko >> wrote: >> > HAVE_LIBCHARSET_H and NO_R_TO_GCC_LINKER are not specific to >> > msysGit, they're general MinGW settings. >> >> Actually, HAVE_LIBCHARSET_H is. It's only present because we have >> libiconv installed. > > 1. What are other ways to provide iconv on MinGW? I'm not sure I understand. To set HAVE_LIBCHARSET_H, we need to have libcharset.h. MinGW doesn't supply by default to my knowledge, so we get it from iconv. The THIS_IS_MSYSGIT file is there for us to be able to pick the right defaults for msysGit, and us having libcharset is indeed a msysGit-detail. Not all iconv-flavors supply libcharset.h, so this tells a particularity about the one we have in msysGit. > 2. One can still completely disable iconv with NO_ICONV=1 Sure. And it does seem like the current setup assumes that anyone building for MinGW has iconv. But perhaps that's a mistake? All in all, I think maybe the line between MinGW and msysGit is a bit blurry at the moment. On the other hand, I don't know of anyone else than Sebastian that builds outside of msysGit. To be honest, I think the whole THIS_IS_MSYSGIT-block should have stayed downstream. -- 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 05/12] MINGW: git-compat-util.h: use inttypes.h for printf macros.
On Mon, Apr 28, 2014 at 5:00 PM, Marat Radchenko wrote: > On Mon, Apr 28, 2014 at 04:53:52PM +0200, Erik Faye-Lund wrote: >> Just checking that I understand: Does this mean that we now require an >> MSVC-version that has stdint.h? If so, I'm not against such a case. >> IMO, the biggest benefit of using MSVC is not building on legacy >> systems, but being able to use it's debugger. And for that purpose >> it's probably OK to increase the required version. > > Ouch, that was not intentional. What minimal MSVC version is currently > supported and who decides if it is OK to increase required one? I don't know what the oldest one anyone have ever used, but I don't think that matters. IMO, just noting it in the commit-message is good enough. Others might feel differently, though. stdint.h is available in VS10 and onwards. contrib/buildsystems/Generators/Vcproj.pm generates project files for VS9. -- 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 03/12] MINGW: compat/mingw.h: do not attempt to redefine lseek on mingw-w64
On Mon, Apr 28, 2014 at 3:51 PM, Marat Radchenko wrote: > mingw-w64 has lseek defined in io.h. msysGit has a declaration of it in io.h as well. But it's not a preprocessor-definition... Are you saying that it's a preprocessor-define in mingw-w64, that points to a 64-bit version? If so, looks good. -- 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 07/12] MINGW: config.mak.uname: reorganize MINGW settings
On Mon, Apr 28, 2014 at 3:51 PM, Marat Radchenko wrote: > HAVE_LIBCHARSET_H and NO_R_TO_GCC_LINKER are not specific to > msysGit, they're general MinGW settings. Actually, HAVE_LIBCHARSET_H is. It's only present because we have libiconv installed. -- 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 09/12] MINGW: config.mak.uname: drop -DNOGDI
On Mon, Apr 28, 2014 at 3:51 PM, Marat Radchenko wrote: > On MinGW-W64, MsgWaitForMultipleObjects is guarded with #ifndef NOGDI. > > Unfortunately, including wingdi.h brings in #define ERROR 0 which > conflicts with several Git internal enums. So, #undef ERROR. > > Signed-off-by: Marat Radchenko > --- > config.mak.uname | 4 ++-- > git-compat-util.h | 2 ++ > 2 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/config.mak.uname b/config.mak.uname > index a376b32..4883fd5 100644 > --- a/config.mak.uname > +++ b/config.mak.uname > @@ -363,7 +363,7 @@ ifeq ($(uname_S),Windows) > COMPAT_OBJS = compat/msvc.o compat/winansi.o \ > compat/win32/pthread.o compat/win32/syslog.o \ > compat/win32/dirent.o > - COMPAT_CFLAGS = -DNOGDI -DHAVE_STRING_H -DHAVE_ALLOCA_H -Icompat > -Icompat/regex -Icompat/win32 -DSTRIP_EXTENSION=\".exe\" > + COMPAT_CFLAGS = -DHAVE_STRING_H -DHAVE_ALLOCA_H -Icompat > -Icompat/regex -Icompat/win32 -DSTRIP_EXTENSION=\".exe\" > BASIC_LDFLAGS = -IGNORE:4217 -IGNORE:4049 -NOLOGO -SUBSYSTEM:CONSOLE > -NODEFAULTLIB:MSVCRT.lib > EXTLIBS = user32.lib advapi32.lib shell32.lib wininet.lib ws2_32.lib > invalidcontinue.obj > PTHREAD_LIBS = > @@ -503,7 +503,7 @@ ifneq (,$(findstring MINGW,$(uname_S))) > NO_INET_NTOP = YesPlease > NO_POSIX_GOODIES = UnfortunatelyYes > DEFAULT_HELP_FORMAT = html > - COMPAT_CFLAGS += -D__USE_MINGW_ANSI_STDIO=0 -D__USE_MINGW_ACCESS > -D_USE_32BIT_TIME_T -DNOGDI -Icompat -Icompat/win32 > + COMPAT_CFLAGS += -D__USE_MINGW_ANSI_STDIO=0 -D__USE_MINGW_ACCESS > -D_USE_32BIT_TIME_T -Icompat -Icompat/win32 > COMPAT_CFLAGS += -DSTRIP_EXTENSION=\".exe\" > COMPAT_OBJS += compat/mingw.o compat/winansi.o \ > compat/win32/pthread.o compat/win32/syslog.o \ > diff --git a/git-compat-util.h b/git-compat-util.h > index aa57a04..48bf0f7 100644 > --- a/git-compat-util.h > +++ b/git-compat-util.h > @@ -98,6 +98,8 @@ > #define WIN32_LEAN_AND_MEAN /* stops windows.h including winsock.h */ > #include > #include > +/* wingdi.h defines ERROR=0, undef it to avoid conflicts */ > +#undef ERROR > #define GIT_WINDOWS_NATIVE > #endif Perhaps it would be cleaner to just undef NOGDI in compat/poll/poll.c? That way we don't end up pulling in all kinds of unrelated GUI-stuff into places where they're not needed...? -- 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 02/12] MINGW: compat/bswap.h: include stdint.h
On Mon, Apr 28, 2014 at 4:52 PM, Marat Radchenko wrote: > On Mon, Apr 28, 2014 at 04:45:43PM +0200, Erik Faye-Lund wrote: >> bswap.h is included after stdint.h from git-compat-util.h anyway... > > That only becomes true after PATCH 05 when talking about MinGW. > > Will drop this one. Right, thanks. -- 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 05/12] MINGW: git-compat-util.h: use inttypes.h for printf macros.
On Mon, Apr 28, 2014 at 3:51 PM, Marat Radchenko wrote:
> Also, pass -D__USE_MINGW_ANSI_STDIO=0 to select MSVCRT-compatible
> macro definitions.
>
> Signed-off-by: Marat Radchenko
> ---
> compat/mingw.h| 2 --
> compat/msvc.h | 3 +++
> config.mak.uname | 3 ++-
> git-compat-util.h | 11 ++-
> 4 files changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/compat/mingw.h b/compat/mingw.h
> index 262b300..c502a22 100644
> --- a/compat/mingw.h
> +++ b/compat/mingw.h
> @@ -342,8 +342,6 @@ static inline char *mingw_find_last_dir_sep(const char
> *path)
> }
> #define find_last_dir_sep mingw_find_last_dir_sep
> #define PATH_SEP ';'
> -#define PRIuMAX "I64u"
> -#define PRId64 "I64d"
>
> void mingw_open_html(const char *path);
> #define open_html mingw_open_html
> diff --git a/compat/msvc.h b/compat/msvc.h
> index 580bb55..cb41ce3 100644
> --- a/compat/msvc.h
> +++ b/compat/msvc.h
> @@ -15,6 +15,9 @@
> #define strtoull _strtoui64
> #define strtoll _strtoi64
>
> +#define PRIuMAX "I64u"
> +#define PRId64 "I64d"
> +
> static __inline int strcasecmp (const char *s1, const char *s2)
> {
> int size1 = strlen(s1);
> diff --git a/config.mak.uname b/config.mak.uname
> index 6c2e6df..e5edae6 100644
> --- a/config.mak.uname
> +++ b/config.mak.uname
> @@ -321,6 +321,7 @@ ifeq ($(uname_S),Windows)
> NO_PREAD = YesPlease
> NEEDS_CRYPTO_WITH_SSL = YesPlease
> NO_LIBGEN_H = YesPlease
> + NO_INTTYPES_H = UnfortunatelyYes
> NO_POLL = YesPlease
> NO_SYMLINK_HEAD = YesPlease
> NO_IPV6 = YesPlease
> @@ -502,7 +503,7 @@ ifneq (,$(findstring MINGW,$(uname_S)))
> NO_INET_NTOP = YesPlease
> NO_POSIX_GOODIES = UnfortunatelyYes
> DEFAULT_HELP_FORMAT = html
> - COMPAT_CFLAGS += -D__USE_MINGW_ACCESS -D_USE_32BIT_TIME_T -DNOGDI
> -Icompat -Icompat/win32
> + COMPAT_CFLAGS += -D__USE_MINGW_ANSI_STDIO=0 -D__USE_MINGW_ACCESS
> -D_USE_32BIT_TIME_T -DNOGDI -Icompat -Icompat/win32
> COMPAT_CFLAGS += -DSTRIP_EXTENSION=\".exe\"
> COMPAT_OBJS += compat/mingw.o compat/winansi.o \
> compat/win32/pthread.o compat/win32/syslog.o \
> diff --git a/git-compat-util.h b/git-compat-util.h
> index f6d3a46..aa57a04 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -85,6 +85,12 @@
> #define _NETBSD_SOURCE 1
> #define _SGI_SOURCE 1
>
> +#ifndef NO_INTTYPES_H
> +#include
> +#else
> +#include
> +#endif
> +
Just checking that I understand: Does this mean that we now require an
MSVC-version that has stdint.h? If so, I'm not against such a case.
IMO, the biggest benefit of using MSVC is not building on legacy
systems, but being able to use it's debugger. And for that purpose
it's probably OK to increase the required version.
--
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 02/12] MINGW: compat/bswap.h: include stdint.h
On Mon, Apr 28, 2014 at 3:51 PM, Marat Radchenko wrote: > bswap.h uses uint32_t type which might not be defined. > This patch adds direct include so bswap.h can be safely included. > > Signed-off-by: Marat Radchenko > --- > compat/bswap.h | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/compat/bswap.h b/compat/bswap.h > index 120c6c1..d170447 100644 > --- a/compat/bswap.h > +++ b/compat/bswap.h > @@ -5,6 +5,8 @@ > * operation. > */ > > +#include > + > /* > * Default version that the compiler ought to optimize properly with > * constant values. Hmm, what's the symptom this fixes? From what I can tell, bswap.h is included after stdint.h from git-compat-util.h anyway... -- 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: [msysGit] Re: git version 1.9.0 missing git-http-push?
On Mon, Apr 28, 2014 at 3:47 PM, Marat Radchenko wrote: > On Mon, Apr 28, 2014 at 03:20:46PM +0200, Johannes Schindelin wrote: >> That way, upstream Git does not have anything to change (and we avoid >> discussing five versions of essentially the same patch :-P). > > This bug isn't specific to msysGit but also affects all environments > where curl-config is not available or cannot be run for some reason, > for example during cross-compilation. True. I think the assumption simply was a bad one, and it has probably gone unnoticed for a while because pushing over WebDAV is not all that common. If anyone turns up a system with an old enough libcurl, they should probably consult curlver.h instead. And if so, they are probably on a system so crippled they need to run automake anyway. -- 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: [msysGit] Re: git version 1.9.0 missing git-http-push?
On Mon, Apr 28, 2014 at 3:20 PM, Johannes Schindelin
wrote:
> Hi kusma,
>
> On Mon, 28 Apr 2014, Erik Faye-Lund wrote:
>
>> On Mon, Apr 28, 2014 at 10:48 AM, Erik Faye-Lund wrote:
>> > So it seems that 08900987 ("Decide whether to build http-push in the
>> > Makefile") makes a bad assumption about the availability of
>> > curl-config on new libcurl installations; it's not present on "stock"
>> > Windows builds.
>>
>> I wonder, though. That check is over 8 years old. Are that old systems
>> (that haven't been upgraded) still able to build Git? Even my old
>> RedHat 5 setup has curl 7.15.5...
>
> The easiest way in my humble opinion would be to install a script like
> this into /mingw/bin/:
>
> -- snip --
> #!/bin/sh
>
> case "$1" in
> --vernum)
> version="$(curl -V)" || exit
> version="$(echo "${version%% (*)*}" | tr . \ )"
> eval printf "%02d%02d%02d" ${version#curl }
> ;;
> --cflags)
> ;;
> --libs)
> echo -lcurl
> ;;
> esac
> -- snap --
>
> That way, upstream Git does not have anything to change (and we avoid
> discussing five versions of essentially the same patch :-P).
Huh? I think I only really proposed one patch...?
--
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: git version 1.9.0 missing git-http-push?
On Mon, Apr 28, 2014 at 11:01 AM, Erik Faye-Lund wrote:
> On Mon, Apr 28, 2014 at 10:48 AM, Erik Faye-Lund wrote:
>> So it seems that 08900987 ("Decide whether to build http-push in the
>> Makefile") makes a bad assumption about the availability of
>> curl-config on new libcurl installations; it's not present on "stock"
>> Windows builds.
>
> I wonder, though. That check is over 8 years old. Are that old systems
> (that haven't been upgraded) still able to build Git? Even my old
> RedHat 5 setup has curl 7.15.5...
>
> Perhaps the following is the right thing to do? If not, perhaps we
> could move this complication to configure.ac, which could get the
> version number from the header-file instead? That way, quirks only
> affect quirky systems...
>
> (white-space damaged, I'll post a proper patch if there's some agreement)
> ---
>
> diff --git a/Makefile b/Makefile
> index 29a555d..6da72e7 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1133,13 +1133,8 @@ else
> REMOTE_CURL_NAMES = $(REMOTE_CURL_PRIMARY) $(REMOTE_CURL_ALIASES)
> PROGRAM_OBJS += http-fetch.o
> PROGRAMS += $(REMOTE_CURL_NAMES)
> -curl_check := $(shell (echo 070908; curl-config --vernum)
> 2>/dev/null | sort -r | sed -ne 2p)
> -ifeq "$(curl_check)" "070908"
> -ifndef NO_EXPAT
> -PROGRAM_OBJS += http-push.o
> -endif
> -endif
> ifndef NO_EXPAT
> +PROGRAM_OBJS += http-push.o
> ifdef EXPATDIR
> BASIC_CFLAGS += -I$(EXPATDIR)/include
> EXPAT_LIBEXPAT = -L$(EXPATDIR)/$(lib)
> $(CC_LD_DYNPATH)$(EXPATDIR)/$(lib) -lexpat
I've built an installer with this patch applied, and it seems to do the trick:
https://dl.dropboxusercontent.com/u/1737924/Git-1.9.2-http-push.exe
--
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] poll/select: prevent busy-waiting
On Mon, Apr 28, 2014 at 1:42 PM, Stepan Kasal wrote:
> From: Paolo Bonzini
> Date: Mon, 21 May 2012 09:52:42 +0200
>
> Backported from Gnulib.
>
> 2012-05-21 Paolo Bonzini
>
> poll/select: prevent busy-waiting. SwitchToThread() only gives away
> the rest of the current time slice to another thread in the current
> process. So if the thread that feeds the file decscriptor we're
> polling is not in the current process, we get busy-waiting.
> * lib/poll.c: Use SleepEx(1, TRUE) instead of SwitchToThread().
> Patch from Theodore Leblond.
> * lib/select.c: Split polling out of the loop that sets the output
> fd_sets. Check for zero result and loop if the wait timeout is
> infinite.
>
> Signed-off-by: Stepan Kasal
> ---
> compat/poll/poll.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/compat/poll/poll.c b/compat/poll/poll.c
> index 31163f2..a9b41d8 100644
> --- a/compat/poll/poll.c
> +++ b/compat/poll/poll.c
> @@ -605,7 +605,7 @@ restart:
>
>if (!rc && timeout == INFTIM)
> {
> - SwitchToThread();
> + SleepEx (1, TRUE);
> goto restart;
> }
>
> --
> 1.9.2.msysgit.0.158.g6070cee
>
Thanks for taking the effort!
Acked-by: Erik Faye-Lund
--
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: git version 1.9.0 missing git-http-push?
On Mon, Apr 28, 2014 at 11:01 AM, Erik Faye-Lund wrote:
> On Mon, Apr 28, 2014 at 10:48 AM, Erik Faye-Lund wrote:
>> So it seems that 08900987 ("Decide whether to build http-push in the
>> Makefile") makes a bad assumption about the availability of
>> curl-config on new libcurl installations; it's not present on "stock"
>> Windows builds.
>
> I wonder, though. That check is over 8 years old. Are that old systems
> (that haven't been upgraded) still able to build Git? Even my old
> RedHat 5 setup has curl 7.15.5...
>
> Perhaps the following is the right thing to do? If not, perhaps we
> could move this complication to configure.ac, which could get the
> version number from the header-file instead? That way, quirks only
> affect quirky systems...
And here's a stab at that. Not really tested, as I don't have an
affected system, so it's probably broken somehow ;)
But if someone want's to pick it up, at least there's a starting-point.
---
diff --git a/Makefile b/Makefile
index 29a555d..b94f830 100644
--- a/Makefile
+++ b/Makefile
@@ -1133,11 +1133,8 @@ else
REMOTE_CURL_NAMES = $(REMOTE_CURL_PRIMARY) $(REMOTE_CURL_ALIASES)
PROGRAM_OBJS += http-fetch.o
PROGRAMS += $(REMOTE_CURL_NAMES)
- curl_check := $(shell (echo 070908; curl-config --vernum)
2>/dev/null | sort -r | sed -ne 2p)
- ifeq "$(curl_check)" "070908"
- ifndef NO_EXPAT
- PROGRAM_OBJS += http-push.o
- endif
+ ifndef NO_CAPABLE_CURL
+ PROGRAM_OBJS += http-push.o
endif
ifndef NO_EXPAT
ifdef EXPATDIR
diff --git a/configure.ac b/configure.ac
index 2f43393..47991c0 100644
--- a/configure.ac
+++ b/configure.ac
@@ -513,6 +513,16 @@ AC_CHECK_LIB([curl], [curl_global_init],
[NO_CURL=],
[NO_CURL=YesPlease])
+AC_COMPILE_IFELSE(
+ [AC_LANG_PROGRAM([#include ],
+ [#if LIBCURL_VERSION_NUM < 0x070908
+#error version too old
+#endif
+ ])],
+ [NO_CAPABLE_CURL=YesPlease],
+ [NO_CAPABLE_CURL=])
+GIT_CONF_SUBST([NO_CAPABLE_CURL])
+
GIT_UNSTASH_FLAGS($CURLDIR)
GIT_CONF_SUBST([NO_CURL])
--
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] Sleep 1 millisecond in poll() to avoid busy wait
On Mon, Apr 28, 2014 at 10:39 AM, Stepan Kasal wrote: > From: theoleblond > Date: Wed, 16 May 2012 06:52:49 -0700 > > I played around with this quite a bit. After trying some more complex > schemes, I found that what worked best is to just sleep 1 millisecond > between iterations. Though it's a very short time, it still completely > eliminates the busy wait condition, without hurting perf. > > There code uses SleepEx(1, TRUE) to sleep. See this page for a good > discussion of why that is better than calling SwitchToThread, which > is what was used previously: > http://stackoverflow.com/questions/1383943/switchtothread-vs-sleep1 > > Note that calling SleepEx(0, TRUE) does *not* solve the busy wait. > > The most striking case was when testing on a UNC share with a large repo, > on a single CPU machine. Without the fix, it took 4 minutes 15 seconds, > and with the fix it took just 1:08! I think it's because git-upload-pack's > busy wait was eating the CPU away from the git process that's doing the > real work. With multi-proc, the timing is not much different, but tons of > CPU time is still wasted, which can be a killer on a server that needs to > do bunch of other things. > > I also tested the very fast local case, and didn't see any measurable > difference. On a big repo with 4500 files, the upload-pack took about 2 > seconds with and without the fix. > --- > > This is one of the patches that lives in msysGit, could it be > accepted upstream? > It modifies the Windows compat function only. compat/poll/poll.c comes from Gnulib, so it would be better to submit the patch there and update. -- 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: git version 1.9.0 missing git-http-push?
On Mon, Apr 28, 2014 at 10:48 AM, Erik Faye-Lund wrote:
> So it seems that 08900987 ("Decide whether to build http-push in the
> Makefile") makes a bad assumption about the availability of
> curl-config on new libcurl installations; it's not present on "stock"
> Windows builds.
I wonder, though. That check is over 8 years old. Are that old systems
(that haven't been upgraded) still able to build Git? Even my old
RedHat 5 setup has curl 7.15.5...
Perhaps the following is the right thing to do? If not, perhaps we
could move this complication to configure.ac, which could get the
version number from the header-file instead? That way, quirks only
affect quirky systems...
(white-space damaged, I'll post a proper patch if there's some agreement)
---
diff --git a/Makefile b/Makefile
index 29a555d..6da72e7 100644
--- a/Makefile
+++ b/Makefile
@@ -1133,13 +1133,8 @@ else
REMOTE_CURL_NAMES = $(REMOTE_CURL_PRIMARY) $(REMOTE_CURL_ALIASES)
PROGRAM_OBJS += http-fetch.o
PROGRAMS += $(REMOTE_CURL_NAMES)
-curl_check := $(shell (echo 070908; curl-config --vernum)
2>/dev/null | sort -r | sed -ne 2p)
-ifeq "$(curl_check)" "070908"
-ifndef NO_EXPAT
-PROGRAM_OBJS += http-push.o
-endif
-endif
ifndef NO_EXPAT
+PROGRAM_OBJS += http-push.o
ifdef EXPATDIR
BASIC_CFLAGS += -I$(EXPATDIR)/include
EXPAT_LIBEXPAT = -L$(EXPATDIR)/$(lib)
$(CC_LD_DYNPATH)$(EXPATDIR)/$(lib) -lexpat
--
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: git version 1.9.0 missing git-http-push?
On Mon, Apr 28, 2014 at 9:36 AM, Marat Radchenko wrote:
> Silvola Tuomas wrote
>> Hello,
>>
>> I installed git for windows 1.9.0 but any push operation I tried with it
>> produced an error message saying "git: 'http-push' is not a git command".
>> Other commands like pull, add, and commit worked just fine.
>> At the end of this day I noticed that C:\Program Files
>> (x86)\Git\libexec\git-core just didn't have the file git-http-push. There
>> were git-http-backend, git-http-fetch and git-imap-send and such but no
>> git-http-push.
>>
>> I resolved my issue by uninstalling 1.9.0, installing an older version
>> instead (1.8.1.2; this is when push started working) and 1.9.0 right on
>> top of the older version. Now git push command works as expected.
>>
>> Br,
>> Tuomas Silvola
>
> From Makefile:
>
> curl_check := $(shell (echo 070908; curl-config --vernum) 2>/dev/null
> |
> sort -r | sed -ne 2p)
> ifeq "$(curl_check)" "070908"
> ifndef NO_EXPAT
> PROGRAM_OBJS += http-push.o
> endif
> endif
>
> if there's no curl-config, http-push.c is silently skipped. This check also
> doesn't play with cross-compiling when you cannot call curl-config because
> it is for other arch.
>
> There's also a mystic git-http-push$X that is not referenced from anywhere.
We're using Curl 7.30.0 in msysGit (and thus also Git for Windows), so
we should be able to include it. However, we do not have curl-config
installed.
Looking at 08900987 ("Decide whether to build http-push in the
Makefile"), that commit is from 2005, so it seems we've broken
something.
Further, looking a bit at our curl build-script, we don't seem to to
install curl-config. HOWEVER, 37e42ab ("curl: update to 7.28.1 and
enable ipv6"), dated 1. Feb 2013 adds a function to remove
curl-config. Pat, why is this?
My knee-jerk suspicion would be that it's because it's a stale
curl-config from a previous install (that *did* install it). However,
it doesn't seem like the mingw32 Makefile (the one you get without
running configure, it seems) even tries to build curl-config. In fact,
it seems this is simply built by configure itself. Which we don't run,
again since 37e42ab ("curl: update to 7.28.1 and enable ipv6").
So it seems that 08900987 ("Decide whether to build http-push in the
Makefile") makes a bad assumption about the availability of
curl-config on new libcurl installations; it's not present on "stock"
Windows builds.
--
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 v3] send-email: recognize absolute path on Windows
On Tue, Apr 22, 2014 at 6:50 PM, Junio C Hamano wrote:
> Erik Faye-Lund writes:
>
>>>> Shouldn't the latter also be anchored at the beginning of the string
>>>> with a leading "^"?
>>>>
>>>>> +}
>>>>> +
>>>>> +require File::Spec::Functions;
>>>>> +return File::Spec::Functions::file_name_is_absolute($path);
>>>>
>>>> We already "use File::Spec qw(something else)" at the beginning, no?
>>>> Why not throw file_name_is_absolute into that qw() instead?
>>>
>>> Ahh, OK, if you did so, you won't have any place to hook the "only
>>> on msys do this" trick into.
>>>
>>> It somehow feels somewhat confusing that we define a sub with the
>>> same name as the system one, while not overriding it entirely but
>>> delegate back to the system one. I am debating myself if it is more
>>> obvious if it is done this way:
>>>
>>> use File::Spec::Functions qw(file_name_is_absolute);
>>> if ($^O eq 'msys') {
>>> sub file_name_is_absolute {
>>> return $_[0] =~ /^\// || $_[0] =~ /^[A-Z]:/i;
>>> }
>>> }
>>>
>>
>> In this case, we end up requiring that module even when we end up
>> using it, no?
>
> Also somebody earlier mentioned that we would be redefining, which
> has a different kind of ugliness, so I'd agree with the code structure
> of what you sent out (which has been queued on 'pu').
>
> My earlier question "don't we want to make sure 'C:' is at the
> betginning of the string?" still stands, though. I do not think I
> futzed with your regexp in the version I queued on 'pu'.
Ah, yes of course. Thanks for spotting that. I also like the other
clean-ups you did to the regex (above).
--
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 v3] send-email: recognize absolute path on Windows
On Wed, Apr 16, 2014 at 7:19 PM, Junio C Hamano wrote:
> Junio C Hamano writes:
>
>> Erik Faye-Lund writes:
>>
>>> So let's manually check for these in that case, and fall back to
>>> the File::Spec-helper on other platforms (e.g Win32 with native
>>> Perl)
>>> ...
>>> +sub file_name_is_absolute {
>>> +my ($path) = @_;
>>> +
>>> +# msys does not grok DOS drive-prefixes
>>> +if ($^O eq 'msys') {
>>> +return ($path =~ m#^/# || $path =~ m#[a-zA-Z]\:#)
>>
>> Shouldn't the latter also be anchored at the beginning of the string
>> with a leading "^"?
>>
>>> +}
>>> +
>>> +require File::Spec::Functions;
>>> +return File::Spec::Functions::file_name_is_absolute($path);
>>
>> We already "use File::Spec qw(something else)" at the beginning, no?
>> Why not throw file_name_is_absolute into that qw() instead?
>
> Ahh, OK, if you did so, you won't have any place to hook the "only
> on msys do this" trick into.
>
> It somehow feels somewhat confusing that we define a sub with the
> same name as the system one, while not overriding it entirely but
> delegate back to the system one. I am debating myself if it is more
> obvious if it is done this way:
>
> use File::Spec::Functions qw(file_name_is_absolute);
> if ($^O eq 'msys') {
> sub file_name_is_absolute {
> return $_[0] =~ /^\// || $_[0] =~ /^[A-Z]:/i;
> }
> }
>
In this case, we end up requiring that module even when we end up
using it, no? Not that I have very strong objections for doing just
that, after all, it appears to be built-in. (As you might understand
from this message, my perl-fu is really lacking :-P)
--
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 6/6] xdiff/xprepare.c: reduce scope of variables
On Wed, Apr 16, 2014 at 11:33 AM, Elia Pinto wrote:
> Signed-off-by: Elia Pinto
> ---
> xdiff/xprepare.c |5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/xdiff/xprepare.c b/xdiff/xprepare.c
> index 63a22c6..e0b6987 100644
> --- a/xdiff/xprepare.c
> +++ b/xdiff/xprepare.c
> @@ -161,8 +161,7 @@ static int xdl_prepare_ctx(unsigned int pass, mmfile_t
> *mf, long narec, xpparam_
>xdlclassifier_t *cf, xdfile_t *xdf) {
> unsigned int hbits;
> long nrec, hsize, bsize;
> - unsigned long hav;
> - char const *blk, *cur, *top, *prev;
> + char const *blk, *cur, *prev;
> xrecord_t *crec;
> xrecord_t **recs, **rrecs;
> xrecord_t **rhash;
> @@ -193,7 +192,9 @@ static int xdl_prepare_ctx(unsigned int pass, mmfile_t
> *mf, long narec, xpparam_
>
> nrec = 0;
> if ((cur = blk = xdl_mmfile_first(mf, &bsize)) != NULL) {
> +char const *top;
> for (top = blk + bsize; cur < top; ) {
> +unsigned long hav;
> prev = cur;
We do not indent with spaces.
--
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 4/6] contrib/credential/osxkeychain/git-credential-osxkeychain.c: reduce scope of variables
On Wed, Apr 16, 2014 at 11:33 AM, Elia Pinto wrote:
> Signed-off-by: Elia Pinto
> ---
> contrib/credential/osxkeychain/git-credential-osxkeychain.c |8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c
> b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
> index bcd3f57..5ae09f6 100644
> --- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c
> +++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
> @@ -163,12 +163,12 @@ static void read_credential(void)
>
> int main(int argc, const char **argv)
> {
> - const char *usage =
> - "usage: git credential-osxkeychain ";
> -
> if (!argv[1])
> + {
> + const char *usage =
> + "usage: git credential-osxkeychain ";
> die(usage);
> -
> + }
Again, not our code-style.
--
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 5/6] contrib/credential/wincred/git-credential-wincred.c: reduce scope of variables
On Wed, Apr 16, 2014 at 11:33 AM, Elia Pinto wrote:
> Signed-off-by: Elia Pinto
> ---
> contrib/credential/wincred/git-credential-wincred.c |6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/contrib/credential/wincred/git-credential-wincred.c
> b/contrib/credential/wincred/git-credential-wincred.c
> index a1d38f0..edff71c 100644
> --- a/contrib/credential/wincred/git-credential-wincred.c
> +++ b/contrib/credential/wincred/git-credential-wincred.c
> @@ -258,11 +258,13 @@ static void read_credential(void)
>
> int main(int argc, char *argv[])
> {
> - const char *usage =
> - "usage: git credential-wincred \n";
>
> if (!argv[1])
> + {
> + const char *usage =
> + "usage: git credential-wincred \n";
> die(usage);
> + }
>
This is not the indentation/bracket-style we use in this source-file.
--
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 v3] send-email: recognize absolute path on Windows
From: Erik Faye-Lund
On Windows, absolute paths might start with a DOS drive prefix,
which these two checks failed to recognize.
Unfortunately, we cannot simply use the file_name_is_absolute
helper in File::Spec::Functions, because Git for Windows has an
MSYS-based Perl, where this helper doesn't grok DOS
drive-prefixes.
So let's manually check for these in that case, and fall back to
the File::Spec-helper on other platforms (e.g Win32 with native
Perl)
Signed-off-by: Erik Faye-Lund
---
So here's a version that does the old and long-time tested
approach without requiring breaking changes to msysGit's perl.
git-send-email.perl | 16 ++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/git-send-email.perl b/git-send-email.perl
index fdb0029..8f5f986 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1113,6 +1113,18 @@ sub ssl_verify_params {
}
}
+sub file_name_is_absolute {
+ my ($path) = @_;
+
+ # msys does not grok DOS drive-prefixes
+ if ($^O eq 'msys') {
+ return ($path =~ m#^/# || $path =~ m#[a-zA-Z]\:#)
+ }
+
+ require File::Spec::Functions;
+ return File::Spec::Functions::file_name_is_absolute($path);
+}
+
# Returns 1 if the message was sent, and 0 otherwise.
# In actuality, the whole program dies when there
# is an error sending a message.
@@ -1197,7 +1209,7 @@ X-Mailer: git-send-email $gitversion
if ($dry_run) {
# We don't want to send the email.
- } elsif ($smtp_server =~ m#^/#) {
+ } elsif (file_name_is_absolute($smtp_server)) {
my $pid = open my $sm, '|-';
defined $pid or die $!;
if (!$pid) {
@@ -1271,7 +1283,7 @@ X-Mailer: git-send-email $gitversion
printf (($dry_run ? "Dry-" : "")."Sent %s\n", $subject);
} else {
print (($dry_run ? "Dry-" : "")."OK. Log says:\n");
- if ($smtp_server !~ m#^/#) {
+ if (!file_name_is_absolute($smtp_server)) {
print "Server: $smtp_server\n";
print "MAIL FROM:<$raw_from>\n";
foreach my $entry (@recipients) {
--
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
Re: Re* [PATCH] send-email: recognize absolute path on Windows
On Tue, Apr 15, 2014 at 10:37 PM, Junio C Hamano wrote:
> Junio C Hamano writes:
>
>> Thanks, both. I'd expect another round then?
>>
>> -- >8 --
>> From: Erik Faye-Lund
>>
>> On Windows, absolute paths might start with a DOS drive prefix,
>> which these checks fail to recognize.
>>
>> Use file_name_is_absolute from File::Spec::Functions for
>> portability. The Perl module msysgit has been shipping needs to be
>> updated for this patch to work, though.
>>
>> Signed-off-by: Erik Faye-Lund
>> Helepd-by: Johannes Sixt
>> ---
>>
>> git-send-email.perl | 6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/git-send-email.perl b/git-send-email.perl
>> index fdb0029..eda3917 100755
>> --- a/git-send-email.perl
>> +++ b/git-send-email.perl
>> @@ -25,7 +25,7 @@
>> use Data::Dumper;
>> use Term::ANSIColor;
>> use File::Temp qw/ tempdir tempfile /;
>> -use File::Spec::Functions qw(catfile);
>> +use File::Spec::Functions qw(catfile file_name_is_absolute);
>> use Error qw(:try);
>> use Git;
>>
>> @@ -1197,7 +1197,7 @@ sub send_message {
>>
>> if ($dry_run) {
>> # We don't want to send the email.
>> - } elsif ($smtp_server =~ m#^/#) {
>> + } elsif (file_name_is_absolute($smtp_server)) {
>> my $pid = open my $sm, '|-';
>> defined $pid or die $!;
>> if (!$pid) {
>> @@ -1271,7 +1271,7 @@ sub send_message {
>> printf (($dry_run ? "Dry-" : "")."Sent %s\n", $subject);
>> } else {
>> print (($dry_run ? "Dry-" : "")."OK. Log says:\n");
>> - if ($smtp_server !~ m#^/#) {
>> + if (file_name_is_absolute($smtp_server)) {
>
> Obviously this has to be "!file_name_is_absolute($smtp_server)" ;-)
>
Heh, yeah. Apart from that, your patch is identical to mine. But, ugh.
Modifying File::Spec into thinking msys is Win32 doesn't seems to
work, as I get other random path-errors in that case:
"Error in tempdir() using \tmp\XX: Parent directory (\tmp) is
not a directory at /libexec/git-core/git-send-email line 554"
--
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] send-email: recognize absolute path on Windows
On Tue, Apr 15, 2014 at 12:42 PM, Erik Faye-Lund wrote: > On Tue, Apr 15, 2014 at 12:32 PM, Johannes Sixt wrote: >> Am 4/15/2014 10:44, schrieb Erik Faye-Lund: >>> From: Erik Faye-Lund >>> >>> On Windows, absolute paths might start with a DOS drive prefix, >>> which this check fails to recognize. >>> >>> Unfortunately, we cannot simply use the file_name_is_absolute >>> helper in File::Spec::Functions, because Git for Windows has an >>> MSYS-based Perl, where this helper doesn't grok DOS >>> drive-prefixes. >>> >>> So let's manually check for these in that case, and fall back to >>> the File::Spec-helper on other platforms (e.g Win32 with native >>> Perl) >>> >>> Signed-off-by: Erik Faye-Lund >>> --- >>> >>> Here's a patch that we've been running with a variation of in >>> Git for Windows for a while. That version wasn't quite palatable, >>> as it recognized DOS drive-prefixes on all platforms. >> >> Did you consider patching msysgit's lib/perl5/5.8.8/File/Spec.pm by >> inserting a line "msys => 'Win32'," near the top of the file; it is the >> hash table that decides which path "style" is selected depending on $^O. >> Then File::Spec->file_name_is_absolute($path) could be used without a >> wrapper. > > I did not, but that works, and is IMO much nicer. Thanks for the idea! Actually, after having tried that, other stuff starts to break... So back to the drawing-board. -- 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
