* [PATCH 1/2] mingw: adjust is_console() to work with stdin
From: Johannes Schindelin @ 2016-12-21 17:53 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Pranit Bauva, Johannes Sixt
In-Reply-To: <cover.1482342791.git.johannes.schindelin@gmx.de>
When determining whether a handle corresponds to a *real* Win32 Console
(as opposed to, say, a character device such as /dev/null), we use the
GetConsoleOutputBufferInfo() function as a tell-tale.
However, that does not work for *input* handles associated with a
console. Let's just use the GetConsoleMode() function for input handles,
and since it does not work on output handles fall back to the previous
method for those.
This patch prepares for using is_console() instead of my previous
misguided attempt in cbb3f3c9b1 (mingw: intercept isatty() to handle
/dev/null as Git expects it, 2016-12-11) that broke everything on
Windows.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
compat/winansi.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/compat/winansi.c b/compat/winansi.c
index 5b2f5638ec..97a004b8ab 100644
--- a/compat/winansi.c
+++ b/compat/winansi.c
@@ -84,6 +84,7 @@ static void warn_if_raster_font(void)
static int is_console(int fd)
{
CONSOLE_SCREEN_BUFFER_INFO sbi;
+ DWORD mode;
HANDLE hcon;
static int initialized = 0;
@@ -98,7 +99,10 @@ static int is_console(int fd)
return 0;
/* check if its a handle to a console output screen buffer */
- if (!GetConsoleScreenBufferInfo(hcon, &sbi))
+ if (!fd) {
+ if (!GetConsoleMode(hcon, &mode))
+ return 0;
+ } else if (!GetConsoleScreenBufferInfo(hcon, &sbi))
return 0;
/* initialize attributes */
--
2.11.0.rc3.windows.1
^ permalink raw reply related
* [PATCH 2/2] mingw: replace isatty() hack
From: Johannes Schindelin @ 2016-12-21 17:53 UTC (permalink / raw)
To: git; +Cc: Jeff Hostetler, Junio C Hamano, Pranit Bauva, Johannes Sixt
In-Reply-To: <cover.1482342791.git.johannes.schindelin@gmx.de>
From: Jeff Hostetler <jeffhost@microsoft.com>
For over a year, Git for Windows has carried a patch that detects the
MSYS2 pseudo ttys used by Git for Windows' default Git Bash (i.e. a
terminal that is not backed by a Win32 Console).
This patch accesses internals that of a previous MSVC runtime that is no
longer valid in newer versions, therefore we needed a replacement for
that hack in order to be able to compile Git using recent Microsoft
Visual C++.
This patch back-ports that patch and makes even the MINGW (i.e.
GCC-compiled) Git use it.
As a side effect (which was the reason for the back-port), this patch
also fixes the previous misguided attempt to intercept isatty() so that
it handles character devices (such as /dev/null) as Git expects it.
Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
compat/winansi.c | 191 +++++++++++++++++++++++--------------------------------
1 file changed, 78 insertions(+), 113 deletions(-)
diff --git a/compat/winansi.c b/compat/winansi.c
index 97a004b8ab..3bfad0d065 100644
--- a/compat/winansi.c
+++ b/compat/winansi.c
@@ -6,9 +6,12 @@
#include "../git-compat-util.h"
#include <wingdi.h>
#include <winreg.h>
+#include "win32.h"
-/* In this file, we actually want to use Windows' own isatty(). */
-#undef isatty
+static int fd_is_interactive[3] = { 0, 0, 0 };
+#define FD_CONSOLE 0x1
+#define FD_SWAPPED 0x2
+#define FD_MSYS 0x4
/*
ANSI codes used by git: m, K
@@ -105,6 +108,9 @@ static int is_console(int fd)
} else if (!GetConsoleScreenBufferInfo(hcon, &sbi))
return 0;
+ if (fd >=0 && fd <= 2)
+ fd_is_interactive[fd] |= FD_CONSOLE;
+
/* initialize attributes */
if (!initialized) {
console = hcon;
@@ -466,76 +472,50 @@ static HANDLE duplicate_handle(HANDLE hnd)
return hresult;
}
-
-/*
- * Make MSVCRT's internal file descriptor control structure accessible
- * so that we can tweak OS handles and flags directly (we need MSVCRT
- * to treat our pipe handle as if it were a console).
- *
- * We assume that the ioinfo structure (exposed by MSVCRT.dll via
- * __pioinfo) starts with the OS handle and the flags. The exact size
- * varies between MSVCRT versions, so we try different sizes until
- * toggling the FDEV bit of _pioinfo(1)->osflags is reflected in
- * isatty(1).
- */
-typedef struct {
- HANDLE osfhnd;
- char osflags;
-} ioinfo;
-
-extern __declspec(dllimport) ioinfo *__pioinfo[];
-
-static size_t sizeof_ioinfo = 0;
-
-#define IOINFO_L2E 5
-#define IOINFO_ARRAY_ELTS (1 << IOINFO_L2E)
-
-#define FPIPE 0x08
-#define FDEV 0x40
-
-static inline ioinfo* _pioinfo(int fd)
-{
- return (ioinfo*)((char*)__pioinfo[fd >> IOINFO_L2E] +
- (fd & (IOINFO_ARRAY_ELTS - 1)) * sizeof_ioinfo);
-}
-
-static int init_sizeof_ioinfo(void)
-{
- int istty, wastty;
- /* don't init twice */
- if (sizeof_ioinfo)
- return sizeof_ioinfo >= 256;
-
- sizeof_ioinfo = sizeof(ioinfo);
- wastty = isatty(1);
- while (sizeof_ioinfo < 256) {
- /* toggle FDEV flag, check isatty, then toggle back */
- _pioinfo(1)->osflags ^= FDEV;
- istty = isatty(1);
- _pioinfo(1)->osflags ^= FDEV;
- /* return if we found the correct size */
- if (istty != wastty)
- return 0;
- sizeof_ioinfo += sizeof(void*);
- }
- error("Tweaking file descriptors doesn't work with this MSVCRT.dll");
- return 1;
-}
-
static HANDLE swap_osfhnd(int fd, HANDLE new_handle)
{
- ioinfo *pioinfo;
- HANDLE old_handle;
-
- /* init ioinfo size if we haven't done so */
- if (init_sizeof_ioinfo())
- return INVALID_HANDLE_VALUE;
-
- /* get ioinfo pointer and change the handles */
- pioinfo = _pioinfo(fd);
- old_handle = pioinfo->osfhnd;
- pioinfo->osfhnd = new_handle;
- return old_handle;
+ /*
+ * Create a copy of the original handle associated with fd
+ * because the original will get closed when we dup2().
+ */
+ HANDLE handle = (HANDLE)_get_osfhandle(fd);
+ HANDLE duplicate = duplicate_handle(handle);
+
+ /* Create a temp fd associated with the already open "new_handle". */
+ int new_fd = _open_osfhandle((intptr_t)new_handle, O_BINARY);
+
+ assert((fd == 1) || (fd == 2));
+
+ /*
+ * Use stock dup2() to re-bind fd to the new handle. Note that
+ * this will implicitly close(1) and close both fd=1 and the
+ * originally associated handle. It will open a new fd=1 and
+ * call DuplicateHandle() on the handle associated with new_fd.
+ * It is because of this implicit close() that we created the
+ * copy of the original.
+ *
+ * Note that the OS can recycle HANDLE (numbers) just like it
+ * recycles fd (numbers), so we must update the cached value
+ * of "console". You can use GetFileType() to see that
+ * handle and _get_osfhandle(fd) may have the same number
+ * value, but they refer to different actual files now.
+ *
+ * Note that dup2() when given target := {0,1,2} will also
+ * call SetStdHandle(), so we don't need to worry about that.
+ */
+ dup2(new_fd, fd);
+ if (console == handle)
+ console = duplicate;
+ handle = INVALID_HANDLE_VALUE;
+
+ /* Close the temp fd. This explicitly closes "new_handle"
+ * (because it has been associated with it).
+ */
+ close(new_fd);
+
+ fd_is_interactive[fd] |= FD_SWAPPED;
+
+ return duplicate;
}
#ifdef DETECT_MSYS_TTY
@@ -562,49 +542,32 @@ static void detect_msys_tty(int fd)
name = nameinfo->Name.Buffer;
name[nameinfo->Name.Length / sizeof(*name)] = 0;
- /* check if this could be a MSYS2 pty pipe ('msys-XXXX-ptyN-XX') */
- if (!wcsstr(name, L"msys-") || !wcsstr(name, L"-pty"))
+ /*
+ * Check if this could be a MSYS2 pty pipe ('msys-XXXX-ptyN-XX')
+ * or a cygwin pty pipe ('cygwin-XXXX-ptyN-XX')
+ */
+ if ((!wcsstr(name, L"msys-") && !wcsstr(name, L"cygwin-")) ||
+ !wcsstr(name, L"-pty"))
return;
- /* init ioinfo size if we haven't done so */
- if (init_sizeof_ioinfo())
- return;
-
- /* set FDEV flag, reset FPIPE flag */
- _pioinfo(fd)->osflags &= ~FPIPE;
- _pioinfo(fd)->osflags |= FDEV;
+ fd_is_interactive[fd] |= FD_MSYS;
}
#endif
+/* Wrapper for isatty(). Most calls in the main git code
+ * call isatty(1 or 2) to see if the instance is interactive
+ * and should: be colored, show progress, paginate output.
+ * We lie and give results for what the descriptor WAS at
+ * startup (and ignore any pipe redirection we internally
+ * do).
+ */
+#undef isatty
int winansi_isatty(int fd)
{
- int res = isatty(fd);
-
- if (res) {
- /*
- * Make sure that /dev/null is not fooling Git into believing
- * that we are connected to a terminal, as "_isatty() returns a
- * nonzero value if the descriptor is associated with a
- * character device."; for more information, see
- *
- * https://msdn.microsoft.com/en-us/library/f4s0ddew.aspx
- */
- HANDLE handle = winansi_get_osfhandle(fd);
- if (fd == STDIN_FILENO) {
- DWORD dummy;
-
- if (!GetConsoleMode(handle, &dummy))
- res = 0;
- } else if (fd == STDOUT_FILENO || fd == STDERR_FILENO) {
- CONSOLE_SCREEN_BUFFER_INFO dummy;
-
- if (!GetConsoleScreenBufferInfo(handle, &dummy))
- res = 0;
- }
- }
-
- return res;
+ if (fd >=0 && fd <= 2)
+ return fd_is_interactive[fd] != 0;
+ return isatty(fd);
}
void winansi_init(void)
@@ -615,6 +578,10 @@ void winansi_init(void)
/* check if either stdout or stderr is a console output screen buffer */
con1 = is_console(1);
con2 = is_console(2);
+
+ /* Also compute console bit for fd 0 even though we don't need the result here. */
+ is_console(0);
+
if (!con1 && !con2) {
#ifdef DETECT_MSYS_TTY
/* check if stdin / stdout / stderr are MSYS2 pty pipes */
@@ -658,12 +625,10 @@ void winansi_init(void)
*/
HANDLE winansi_get_osfhandle(int fd)
{
- HANDLE hnd = (HANDLE) _get_osfhandle(fd);
- if (isatty(fd) && GetFileType(hnd) == FILE_TYPE_PIPE) {
- if (fd == 1 && hconsole1)
- return hconsole1;
- else if (fd == 2 && hconsole2)
- return hconsole2;
- }
- return hnd;
+ if (fd == 1 && (fd_is_interactive[1] & FD_SWAPPED))
+ return hconsole1;
+ if (fd == 2 && (fd_is_interactive[2] & FD_SWAPPED))
+ return hconsole2;
+
+ return (HANDLE)_get_osfhandle(fd);
}
--
2.11.0.rc3.windows.1
^ permalink raw reply related
* Re: [PATCH 1/1] mingw: intercept isatty() to handle /dev/null as Git expects it
From: Johannes Schindelin @ 2016-12-21 17:56 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Sixt, git, Pranit Bauva
In-Reply-To: <xmqqh963snhs.fsf@gitster.mtv.corp.google.com>
Hi,
On Fri, 16 Dec 2016, Junio C Hamano wrote:
> Johannes Sixt <j6t@kdbg.org> writes:
>
> > Am 16.12.2016 um 19:08 schrieb Junio C Hamano:
> >> Sorry for not having waited for you to chime in before agreeing to
> >> fast-track this one, and now this is in 'master'.
> >
> > No reason to be sorry, things happen... Dscho's request for
> > fast-tracking was very reasonable, and the patch looked "obviously
> > correct".
>
> Oh, I do agree it was reasonable and looked obviously correct. And
> I suspect that it did not make anything worse, either, so there is
> not much harm done, other than that I now have to remember not to
> merge it down without further fixes to 'maint' and declare the NUL
> thing fixed ;-)
Actually, due to having way too many worktrees I managed to test the exact
wrong build product and failed to notice that the patch I asked to
fast-track broke paging in Git for Windows' Git Bash, too, not only in
CMD.
Bummer.
> > Unfortunately, I'm away from my Windows box over the weekend. It will
> > have to wait until Monday before I can test this idea.
>
> Thanks.
I just sent out a fixer-upper patch series, hopefully I got it right this
time.
Hannes, it would be very nice if you could beat on it for a bit.
Happy holidays,
Dscho
^ permalink raw reply
* Re: [PATCH] winansi_isatty(): fix when Git is used from CMD
From: Johannes Schindelin @ 2016-12-21 17:57 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Junio C Hamano, git, Pranit Bauva
In-Reply-To: <3f06ea33-b4de-48b4-593e-239eb6e87dd4@kdbg.org>
Hi Hannes,
On Mon, 19 Dec 2016, Johannes Sixt wrote:
> Am 18.12.2016 um 16:37 schrieb Johannes Sixt:
> > winansi.c is all about overriding MSVCRT's console handling. If we are
> > connected to a console, then by the time isatty() is called (from
> > outside the emulation layer), all handling of file descriptors 1 and 2
> > is already outside MSVCRT's control. In particular, we have determined
> > unambiguously whether a terminal is connected (see is_console()). I
> > suggest to have the implementation below (on top of the patch I'm
> > responding to).
> >
> > What do you think?
>
> I thought a bit more about this approach, and I retract it. I think it
> does not work when Git is connected to an MSYS TTY, i.e., when the
> "console" is in reality the pipe that is detected in detect_msys_tty().
>
> At the same time I wonder how your original winansi_isatty() could have
> worked: In this case, MSVCRT's isatty() would return 1 (because
> detect_msys_tty() has set things up that this happens), but then
> winansi_isatty() checks whether the handle underlying fd 0, 1 or 2 is a real
> Windows console. But it is not: it is a pipe. Am I missing something?
You did not miss anything. I did. I broke everything.
Very sorry for that!
Dscho
^ permalink raw reply
* Re: [PATCH] winansi_isatty(): fix when Git is used from CMD
From: Johannes Schindelin @ 2016-12-21 17:59 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Sixt, git, Pranit Bauva
In-Reply-To: <xmqqtw9yleop.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Tue, 20 Dec 2016, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > That code works because winansi_get_osfhandle() is in winansi.c, where its
> > call to isatty() is *not* redirected to winansi_isatty(). Good.
> > ...
> > My plan was actually ...
> > ...
> > Let's just clean up all of this in one go.
>
> I take this to mean that I should hold off applying/merging j6t's
> one liner and wait for your counterproposal tested by j6t. If I
> misread you please let me know.
>
> Thanks for working well together, as always.
I prepared a patch series based on `pu`, on top of Hannes' patch, and I
also prepared a branch that is based on `master`, obviously without
Hannes' patch.
I am indifferent whether to include it or not, as long as we have
something robust in the end that everybody is happy with.
Ciao,
Dscho
^ permalink raw reply
* Missing option for format-patch?
From: Norbert Kiesel @ 2016-12-21 18:01 UTC (permalink / raw)
To: git
Hi,
I use `git format-patch master..myBranch` quite a bit to send patches
to other developers. I also add notes to the commits so that I remember
which patches were emailed to whom. `git log` has an option to automatically
include the notes in the output. However, I can't find such an option for `git
format-patch`. Am I missing something?
Another nice option would to to somehow include the branch name in the
resulting output. Right now I use either notes or abuse the
`--subject` option for this.
</nk>
P.S.: Today I'm sad and proud to say "Ich bin ein Berliner!" --nk
^ permalink raw reply
* Re: [PATCH] string-list: make compare function compatible with qsort(3)
From: Junio C Hamano @ 2016-12-21 18:10 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List, Jeff King, Johannes Schindelin
In-Reply-To: <c7bac0b7-c555-162f-7880-0355831cee48@web.de>
René Scharfe <l.s.r@web.de> writes:
> The cmp member of struct string_list points to a comparison function
> that is used for sorting and searching of list items. It takes two
> string pointers -- like strcmp(3), which is in fact the default;
> cmp_items() provides a qsort(3) compatible interface by passing the
> string members of two struct string_list_item pointers to cmp.
>
> One shortcoming is that the comparison function is restricted to working
> with the string members of items; util is inaccessible to it. Another
> one is that the value of cmp is passed in a global variable to
> cmp_items(), making string_list_sort() non-reentrant.
I think it is insane to make util accessible to the comparison
function in the first place.
A string-list is primarily a table of strings that can be used to
quickly look up a string in it (or detect absense of it), and
optionally set and get the data associated to that string. If you
allow the comparison function to take anything other than the string
itself into account, you can no longer binary search unless you
force callers to specify what to put in util when a string is added
to the table, and you also remove the ability to modify util once a
string is added to the table.
The string-list API exposes the "append without sorting and then
sort before starting to look up efficiently with a binary search",
and I think that is its biggest misdesign. Such an optimization
would have been hidden from callers in a correctly designed API by
making sure sorting happens lazily when a function that wants to see
a sorted nature of the list for the first time, but somehow we ended
up with an API with separate functions _insert() and _append() with
an explicit _sort(). It then leads to unsorted_*_lookup() and
similar mess, that imply that a string-list can be used not as a
look-up table but just an unordered bag of items. In our attempt to
make it serve as these two quite different things, it has become
good API for neither of its two uses. The caller is forced to know
when the list is not sorted and unsorted_* variant must be used, for
example. "Perhaps it makes it even more flexible if we made util
available to ordering decision" is a line of thinking that makes it
even worse.
I do agree that non-reentrancy is an issue that is worth solving,
though.
^ permalink raw reply
* Re: [PATCH 2/2] mingw: replace isatty() hack
From: Junio C Hamano @ 2016-12-21 18:45 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Jeff Hostetler, Pranit Bauva, Johannes Sixt
In-Reply-To: <18174f0a7fbb4a0ccd8ca8380e00161826166a32.1482342791.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> From: Jeff Hostetler <jeffhost@microsoft.com>
>
> For over a year, Git for Windows has carried a patch that detects the
> MSYS2 pseudo ttys used by Git for Windows' default Git Bash (i.e. a
> terminal that is not backed by a Win32 Console).
>
> This patch accesses internals that of a previous MSVC runtime that is no
> longer valid in newer versions, therefore we needed a replacement for
> that hack in order to be able to compile Git using recent Microsoft
> Visual C++.
Sorry, but I cannot parse the early part of the first sentence of
the second paragraph before the comma; I am especially having
trouble around the first "that".
> This patch back-ports that patch and makes even the MINGW (i.e.
> GCC-compiled) Git use it.
>
> As a side effect (which was the reason for the back-port), this patch
> also fixes the previous misguided attempt to intercept isatty() so that
> it handles character devices (such as /dev/null) as Git expects it.
I had to read the above three times to understand which patches
three instances of "This patch" and one instance of "that patch"
refer to. I wish it were easier to read, but I think I got them all
right [*1*] after re-reading, and the story made sense to me.
> +static int fd_is_interactive[3] = { 0, 0, 0 };
> +#define FD_CONSOLE 0x1
> +#define FD_SWAPPED 0x2
> +#define FD_MSYS 0x4
>
> /*
> ANSI codes used by git: m, K
> @@ -105,6 +108,9 @@ static int is_console(int fd)
> } else if (!GetConsoleScreenBufferInfo(hcon, &sbi))
> return 0;
>
> + if (fd >=0 && fd <= 2)
Style: if (fd >= 0 && fd <= 2)
> +/* Wrapper for isatty(). Most calls in the main git code
Style: /*
* multi-line comment block begins with slash-asterisk
* and ends with asterisk-slash without anything else on
* the line.
*/
> + * call isatty(1 or 2) to see if the instance is interactive
> + * and should: be colored, show progress, paginate output.
> + * We lie and give results for what the descriptor WAS at
> + * startup (and ignore any pipe redirection we internally
> + * do).
> + */
> +#undef isatty
> int winansi_isatty(int fd)
> {
> + if (fd >=0 && fd <= 2)
Style: if (fd >= 0 && fd <= 2)
> + return fd_is_interactive[fd] != 0;
> + return isatty(fd);
> }
Thanks.
[Footnote]
*1* What I thought I understood in my own words:
Git for Windows has carried a patch that depended on internals
of MSVC runtime, but it does not work correctly with recent MSVC
runtime. A replacement was written originally for compiling
with VC++. The patch in this message is a backport of that
replacement, and it also fixes the previous attempt to make
isatty() work.
^ permalink raw reply
* Re: [PATCH] winansi_isatty(): fix when Git is used from CMD
From: Junio C Hamano @ 2016-12-21 18:54 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Johannes Sixt, git, Pranit Bauva
In-Reply-To: <alpine.DEB.2.20.1612211857480.155951@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> I prepared a patch series based on `pu`, on top of Hannes' patch, and I
> also prepared a branch that is based on `master`, obviously without
> Hannes' patch.
I think the latter is what you have at
$ git fetch https://github.com/dscho/git mingw-isatty-fixup-master
If that is correct, I am inclined to fetch that and queue it on
'pu', replacing Hannes's patch, and then to ask him to Test/Ack it.
Thanks.
^ permalink raw reply
* Re: [PATCH 00/13] gitk: tweak rendering of remote-tracking references
From: Marc Branchaud @ 2016-12-21 19:07 UTC (permalink / raw)
To: Michael Haggerty, Paul Mackerras; +Cc: git
In-Reply-To: <046b088c-afd5-66b9-fe3c-255e42a7d768@alum.mit.edu>
On 2016-12-20 07:05 PM, Michael Haggerty wrote:
> On 12/20/2016 04:01 PM, Marc Branchaud wrote:
>> On 2016-12-19 11:44 AM, Michael Haggerty wrote:
>>> This patch series changes a bunch of details about how remote-tracking
>>> references are rendered in the commit list of gitk:
>>
>> Thanks for this! I like the new, compact look very much!
>>
>> That said, I remember when I was a new git user and I leaned heavily on
>> gitk to understand how references worked. It was particularly
>> illuminating to see the remote references distinctly labeled, and the
>> fact that they were "remotes/origin/foo" gave me an Aha! moment where I
>> came to understand that the refs hierarchy is more flexible than just
>> the conventions coded into git itself. I eventually felt free to create
>> my own, private ref hierarchies.
>>
>> I am in no way opposed to this series. I just wanted to point out that
>> there was some utility in those labels. It makes me think that it might
>> be worthwhile for gitk to have a "raw-refs" mode, that shows the full
>> "refs/foo/bar/baz" paths of all the heads, tags, and whatever else. It
>> could be a useful teaching tool for git.
>
> Yes, I understand that the longer names might be useful for beginners,
> and the full names even more so. However, I think once a user has that
> "aha!" moment, the space wasted on all the redundant words is a real
> impediment to gitk's usability. It is common to have a few references on
> a single commit (especially if you pull from multiple remotes), in which
> case the summary line is completely invisible (and therefore its context
> menu is unreachable). I don't like the idea of dumbing down the UI
> permanently based on what users need at the very beginning of their Git
> usage.
I agree.
> Would it be possible to use the short names in the UI but to add the
> full names to a tooltip or to the context menu?
I don't know -- my Tk-fu is weak.
>>> * Omit the "remote/" prefix on normal remote-tracking references. They
>>
>> If you re-roll, s:remote/:remotes/:.
>
> Thanks.
>
>>> are already distinguished via their two-tone rendering and (usually)
>>> longer names, and this change saves a lot of visual clutter and
>>> horizontal space.
>>>
>>> * Render remote-tracking references that have more than the usual
>>> three slashes like
>>>
>>> origin/foo/bar
>>> ^^^^^^^
>>>
>>> rather than
>>>
>>> origin/foo/bar (formerly remotes/origin/foo/bar)
>>> ^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^^^
>>>
>>> , where the indicated part is the prefix that is rendered in a
>>> different color. Usually, such a reference represents a remote
>>> branch that contains a slash in its name, so the new split more
>>> accurately portrays the separation between remote name and remote
>>> branch name.
>>
>> *Love* this change! :)
>>
>>> * Introduce a separate constant to specify the background color used
>>> for the branch name part of remote-tracking references, to allow it
>>> to differ from the color used for local branches (which by default
>>> is bright green).
>>>
>>> * Change the default background colors for remote-tracking branches to
>>> light brown and brown (formerly they were pale orange and bright
>>> green).
>>
>> Please don't change the remotebgcolor default.
>>
>> Also, perhaps the default remoterefbgcolor should be
>> set remoterefbgcolor $headbgcolor
>> ?
>>
>> I say this because when I applied the series, without the last patch, I
>> was miffed that the remote/ref colour had changed.
>
> This is a one-time inconvenience that gitk developers will experience. I
> doubt that users jump backwards and forwards in gitk versions very often.
In what way do you mean it's restricted to gitk developers?
Patch 12 introduces remoterefbgcolor, with a specific default value.
Prior to that, the "ref part" of remote refs was rendered with
headbgcolor. Users who changed their headbgcolor are used to seeing the
"ref part" of remote refs in the same color as their local heads.
Applying patch 12 changes the "ref part" color of remote refs, for such
users.
All I'm saying is that make the remoterefbgcolor default be $headbgcolor
avoids this.
But, honestly, I don't feel strongly about it.
M.
^ permalink raw reply
* Bug report: Git pull hang occasionally
From: Kai Zhang @ 2016-12-21 19:47 UTC (permalink / raw)
To: git
Issue: Git pull hang occasionally, and when git pull start hanging, need manually "kill -9" to stop hanging
Environment:
Server side:
Git version: 2.11.0
OS: ubuntu 12.04
Nginx: 1.9.7.4
fcgiwrap: 1.1.0
Git repo: None bare, small size (less than 5 MB including .git folder), small file number (less than 100 files)
Nginx config for git:
location ~* /git(\/.*) {
root /var/git;
fastcgi_buffers 256 8k;
fastcgi_param SCRIPT_FILENAME /usr/lib/git-core/git-http-backend;
fastcgi_param GIT_HTTP_EXPORT_ALL true;
fastcgi_param GIT_PROJECT_ROOT /var/git;
fastcgi_param PATH_INFO $1;
fastcgi_pass unix:/var/run/fcgiwrap.socket;
include /opt/openresty/nginx/conf/fastcgi_params;
}
Client side:
Git version: 2.11.0
OS: ubuntu 12.04
All git operations go through http only
End to end work flow:
Keep committing small files to non-bare git repo on server side (twice per second), message will be sent to client side for every update, once client receives message, do git pull
Hanging frequency:
Around 4 times a day
Hanging command stack:
root 32640 23228 0 20:51 ? 00:00:00 git pull -v remote_name master --allow-unrelated-histories
root 32641 32640 0 20:51 ? 00:00:00 git fetch --update-head-ok -v remote_name master
root 32642 32641 0 20:51 ? 00:00:00 git-remote-http remote_name http://server:80/git/repo_name/.git
root 32651 32642 0 20:51 ? 00:00:00 git fetch-pack --stateless-rpc --stdin --lock-pack --thin --no-progress http://server:80/git/repo_name/.git/
Access log for hanging git pull:
10.1.0.10 - - [20/Dec/2016:20:38:10 +0000] "GET /git/repo_name/.git/info/refs?service=git-upload-pack HTTP/1.1" 200 363 "-" "git/2.11.0" "-"
10.1.0.10 - - [20/Dec/2016:20:38:10 +0000] "POST /git/repo_name/.git/git-upload-pack HTTP/1.1" 200 5 "-" "git/2.11.0" "-"
Error log for hanging git pull:
2016/12/20 20:38:10 [error] 9957#0: *687703 FastCGI sent in stderr: "fatal: 'HEAD' is a symref but it is not?" while reading response header from upstream, client: 10.1.0.11, server: server, request: "POST /git/repo_name/.git/git-upload-pack HTTP/1.1", upstream: "fastcgi://unix:/var/run/fcgiwrap.socket:", host: "server"
Some observation:
1. When hanging happen, same repository could be cloned or pulled by another process on the same client.
2. After killing hanging git pull, during retry, same repository can be sync up successfully.
3. Git pull has been executed twice per second. But hanging only happens around 4 times a day.
4. When "fatal: 'HEAD' is a symref but it is not?" happen for POST on server side, client side always start to hang. And when hanging happen on client side, this log for POST always appears. But, if "fatal: 'HEAD' is a symref but it is not?" happen for GET request on server side, client side never hang. For example:
2016/12/20 20:36:53 [error] 9954#0: *685174 FastCGI sent in stderr: "fatal: 'HEAD' is a symref but it is not?" while reading response header from upstream, client: 10.1.0.11, server: server, request: "GET /git/repo_name/.git/info/refs?service=git-upload-pack HTTP/1.1", upstream: "fastcgi://unix:/var/run/fcgiwrap.socket:", host: "server"
will not trigger hanging on client side. And this log "fatal: 'HEAD' is a symref but it is not?" is happening very rare (less than 10 times a day).
It seems a error handling issue on client side. Any help or pointer on where to look will be appreciated.
Regards
Kai
PS. I am not subscribed to the mailing list, please keep me in Cc
^ permalink raw reply
* Re: Allow "git shortlog" to group by committer information
From: Junio C Hamano @ 2016-12-21 20:44 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Sixt, Linus Torvalds, Git Mailing List
In-Reply-To: <20161221032221.s7jmgnfrr6tyuyuk@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> I do wonder if in general it should be the responsibility of skippable
> tests to make sure we end up with the same state whether they are run or
> not. That might manage the complexity more. But I certainly don't mind
> tests being defensive like you have here.
If we speak "in general", I would say that any test should be
prepared to be turned into a skippable one, and they should all make
sure they leave the same state whether they are skipped, they
succeed, or they fail in the middle.
That can theoretically be achievable (e.g. you assume you would
always start from an empty repository, do your thing and arrange to
leave an empty repository by doing test_when_finished), and the
cognitive cost of developers to do so can be reduced by teaching
test_expect_{success/failure} helpers to be responsible for the
"arrange to leave an empty repository" part. But it is quite a big
departure from the way our tests are currently done, i.e. prepare
the environment once and then each of multiple tests observes one
thing in that environment (e.g. "does it work well with --dry-run?
how about without?").
Also it will make the runtime cost of the tests a lot larger, as
setup and teardown need to happen for each individual test. So I do
not think it is a good goal in practice.
Perhaps what you suggest may be a good middle-ground. When you add
prerequisite to an existing test, it will become your responsibility
to make sure the test will leave the same state. That way, you
would know that tests that come later will not be affected by your
change.
^ permalink raw reply
* Re: [PATCH] string-list: make compare function compatible with qsort(3)
From: Junio C Hamano @ 2016-12-21 20:47 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List, Jeff King, Johannes Schindelin
In-Reply-To: <xmqqy3z9i24x.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> I do agree that non-reentrancy is an issue that is worth solving,
> though.
I sounded overly negative, but I do not think of any way other than
this patch to solve the reentrancy issue without using qsort_s().
Between the two, my preference is still the latter, but I could be
persuaded, I would guess.
^ permalink raw reply
* Re: Bug report: Git pull hang occasionally
From: Junio C Hamano @ 2016-12-21 20:59 UTC (permalink / raw)
To: Kai Zhang; +Cc: git
In-Reply-To: <9B7DCFB3-73A4-40DE-8FC6-867C5016EF95@netskope.com>
Kai Zhang <kai@netskope.com> writes:
> 2016/12/20 20:38:10 [error] 9957#0: *687703 FastCGI sent in stderr: "fatal: 'HEAD' is a symref but it is not?" while reading response header from upstream, client: 10.1.0.11, server: server, request: "POST /git/repo_name/.git/git-upload-pack HTTP/1.1", upstream: "fastcgi://unix:/var/run/fcgiwrap.socket:", host: "server"
(Not a solution)
In order to tell the client if HEAD is a symbolic ref and to what
underlying ref it points at if it is a symbolic ref, at the very
beginning of upload-pack, there is a call to head_ref_namespaced()
that uses find_symref(). find_symref() gets "HEAD" and a boolean
that says if it is a symbolic ref, but it does not get where the
symbolic ref points at, so it does resolve_ref_unsafe() to learn
that information.
Between the time head_ref_namespaced() checks the refs database and
finds that HEAD is a symbolic ref, and the time find_symref() calls
resolve_ref_unsafe() to find out where it leads to, if somebody else
updates HEAD, resolve_ref_unsafe() can give an unexpected result, as
all of these read-only operations are performed without any locking.
And the unexpected discrepancy is reported by find_symref() as
fatal. The server side dies, and somehow that fact is lost between
the upload-pack process and the client and somebody in the middle
(e.g. fastcgi interface or nginx webserver on the server side, or
the remote-curl helper on the client side) keeps the "git fetch"
process waiting.
So there seem to be two issues.
- Because of the unlocked read, find_symref() can observe an
inconsistent state. Perhaps it should be updated not to die but
to retry, expecting that transient inconsistency will go away.
- A fatal error in upload-pack is not reported back to the client
to cause it exit is an obvious one, and even if we find a way to
make this fatal error in find_symref() not to trigger, fatal
errors in other places in the code can trigger the same symptom.
^ permalink raw reply
* Re: Races on ref .lock files?
From: Junio C Hamano @ 2016-12-21 21:08 UTC (permalink / raw)
To: Andreas Krey; +Cc: git
In-Reply-To: <20161221100033.GB1206@inner.h.apk.li>
Andreas Krey <a.krey@gmx.de> writes:
> In a different instance, we have a simple bare git repo that we
> use for backup purposes. Which means there are lots of pushes
> going there (all to disjunct refs), and I now cared to look
> into those logfiles:
>
> ----snip
> Wed Dec 21 05:08:14 CET 2016
> fatal: Unable to create '/data/git-backup/backup.git/packed-refs.lock': File exists.
>
> If no other git process is currently running, this probably means a
> git process crashed in this repository earlier. Make sure no other git
> process is running and remove the file manually to continue.
> error: failed to run pack-refs
> To git-backup-user@socrepo.advantest.com:backup.git
> + 8aac9ae...2df6d56 refs/zz/current -> refs/backup/socvm217/ZworkspacesZsocvm217ZjohanabtZws-release_tools.Ycurr (forced update)
> ----snip
>
> I interpret this as "I updated the refs files, but packing them
> didn't work because someone else was also packing right now."
Correct.
> Is that happening as designed, or do I need to be afraid
> that some refs didn't make the push?
Correct and No. Packing refs into the packed-refs file is merely a
performance thing and done under the lock (needless to say, updating
individual refs is also done under the lock). Your push may have
competed with somebody else's push that started earlier and you may
have given up packing refs, but no ill effect should be left behind.
When the lock holder (the other guy who competes with your push)
stuffs refs into a packed-refs file, the values for the refs you
pushed may not be in the packed-refs file, because the other guy may
have observed and captured the value before your push updated them.
Those refs updated by you that are missed by the other guy will be
left as loose refs. Because whenever Git tries to find the value
for a ref, it always checks the loose refs first, there is no issue
due to this.
^ permalink raw reply
* Re: Allow "git shortlog" to group by committer information
From: Johannes Sixt @ 2016-12-21 21:09 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Linus Torvalds, Git Mailing List
In-Reply-To: <d2ac90d6-c4f4-a759-a6e2-2d7fe5bb1c1d@kdbg.org>
Am 20.12.2016 um 19:52 schrieb Johannes Sixt:
> Am 20.12.2016 um 19:35 schrieb Junio C Hamano:
>> test_expect_success 'shortlog --committer (internal)' '
>> + git checkout --orphan side &&
>> + git commit --allow-empty -m one &&
>> + git commit --allow-empty -m two &&
>> + GIT_COMMITTER_NAME="Sin Nombre" git commit --allow-empty -m three &&
>
> Clever! Thank you. Will test in 12 hours.
>
>> +
>> cat >expect <<-\EOF &&
>> - 3 C O Mitter
>> + 2 C O Mitter
>> + 1 Sin Nombre
>> EOF
>> git shortlog -nsc HEAD >actual &&
>> test_cmp expect actual
>>
>
I confirm that t4201 now passes on Windows with this fixup.
-- Hannes
^ permalink raw reply
* Re: Bug report: Git pull hang occasionally
From: Kai Zhang @ 2016-12-21 21:10 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqq8tr9huc0.fsf@gitster.mtv.corp.google.com>
Thank you for your insight and detailed explanation Junio.
I think what you said is what is happening in my environment. Both writing and reading are happening simultaneously.
> On Dec 21, 2016, at 12:59 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> Kai Zhang <kai@netskope.com> writes:
>
>> 2016/12/20 20:38:10 [error] 9957#0: *687703 FastCGI sent in stderr: "fatal: 'HEAD' is a symref but it is not?" while reading response header from upstream, client: 10.1.0.11, server: server, request: "POST /git/repo_name/.git/git-upload-pack HTTP/1.1", upstream: "fastcgi://unix:/var/run/fcgiwrap.socket:", host: "server"
>
> (Not a solution)
>
> In order to tell the client if HEAD is a symbolic ref and to what
> underlying ref it points at if it is a symbolic ref, at the very
> beginning of upload-pack, there is a call to head_ref_namespaced()
> that uses find_symref(). find_symref() gets "HEAD" and a boolean
> that says if it is a symbolic ref, but it does not get where the
> symbolic ref points at, so it does resolve_ref_unsafe() to learn
> that information.
>
> Between the time head_ref_namespaced() checks the refs database and
> finds that HEAD is a symbolic ref, and the time find_symref() calls
> resolve_ref_unsafe() to find out where it leads to, if somebody else
> updates HEAD, resolve_ref_unsafe() can give an unexpected result, as
> all of these read-only operations are performed without any locking.
>
> And the unexpected discrepancy is reported by find_symref() as
> fatal. The server side dies, and somehow that fact is lost between
> the upload-pack process and the client and somebody in the middle
> (e.g. fastcgi interface or nginx webserver on the server side, or
> the remote-curl helper on the client side) keeps the "git fetch"
> process waiting.
>
> So there seem to be two issues.
>
> - Because of the unlocked read, find_symref() can observe an
> inconsistent state. Perhaps it should be updated not to die but
> to retry, expecting that transient inconsistency will go away.
>
> - A fatal error in upload-pack is not reported back to the client
> to cause it exit is an obvious one, and even if we find a way to
> make this fatal error in find_symref() not to trigger, fatal
> errors in other places in the code can trigger the same symptom.
>
^ permalink raw reply
* Re: [PATCH 0/2] Really fix the isatty() problem on Windows
From: Johannes Sixt @ 2016-12-21 21:15 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Junio C Hamano, Pranit Bauva
In-Reply-To: <cover.1482342791.git.johannes.schindelin@gmx.de>
Am 21.12.2016 um 18:53 schrieb Johannes Schindelin:
> The current patch series is based on `pu`, as that already has the
> winansi_get_osfhandle() fix. For ease of testing, I also have a branch
> based on master which you can pull via
>
> git pull https://github.com/dscho/git mingw-isatty-fixup-master
Will test and report back tomorrow.
-- Hannes
^ permalink raw reply
* Re: [PATCH 0/2] Really fix the isatty() problem on Windows
From: Junio C Hamano @ 2016-12-21 21:21 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Johannes Schindelin, git, Pranit Bauva
In-Reply-To: <b0541907-ee79-207b-dc0f-1e3e7d761950@kdbg.org>
Johannes Sixt <j6t@kdbg.org> writes:
> Am 21.12.2016 um 18:53 schrieb Johannes Schindelin:
>> The current patch series is based on `pu`, as that already has the
>> winansi_get_osfhandle() fix. For ease of testing, I also have a branch
>> based on master which you can pull via
>>
>> git pull https://github.com/dscho/git mingw-isatty-fixup-master
>
> Will test and report back tomorrow.
Thanks.
^ permalink raw reply
* Re: Bug report: Git pull hang occasionally
From: Junio C Hamano @ 2016-12-21 21:32 UTC (permalink / raw)
To: Kai Zhang; +Cc: git
In-Reply-To: <xmqq8tr9huc0.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> And the unexpected discrepancy is reported by find_symref() as
> fatal. The server side dies, and somehow that fact is lost between
> the upload-pack process and the client and somebody in the middle
> (e.g. fastcgi interface or nginx webserver on the server side, or
> the remote-curl helper on the client side) keeps the "git fetch"
> process waiting.
>
> So there seem to be two issues.
>
> - Because of the unlocked read, find_symref() can observe an
> inconsistent state. Perhaps it should be updated not to die but
> to retry, expecting that transient inconsistency will go away.
>
> - A fatal error in upload-pack is not reported back to the client
> to cause it exit is an obvious one, and even if we find a way to
> make this fatal error in find_symref() not to trigger, fatal
> errors in other places in the code can trigger the same symptom.
I wonder if the latter is solved by recent patch 296b847c0d
("remote-curl: don't hang when a server dies before any output",
2016-11-18) on the client side.
-- >8 --
From: David Turner <dturner@twosigma.com>
Date: Fri, 18 Nov 2016 15:30:49 -0500
Subject: [PATCH] remote-curl: don't hang when a server dies before any output
In the event that a HTTP server closes the connection after giving a
200 but before giving any packets, we don't want to hang forever
waiting for a response that will never come. Instead, we should die
immediately.
One case where this happens is when attempting to fetch a dangling
object by its object name. In this case, the server dies before
sending any data. Prior to this patch, fetch-pack would wait for
data from the server, and remote-curl would wait for fetch-pack,
causing a deadlock.
Despite this patch, there is other possible malformed input that could
cause the same deadlock (e.g. a half-finished pktline, or a pktline but
no trailing flush). There are a few possible solutions to this:
1. Allowing remote-curl to tell fetch-pack about the EOF (so that
fetch-pack could know that no more data is coming until it says
something else). This is tricky because an out-of-band signal would
be required, or the http response would have to be re-framed inside
another layer of pkt-line or something.
2. Make remote-curl understand some of the protocol. It turns out
that in addition to understanding pkt-line, it would need to watch for
ack/nak. This is somewhat fragile, as information about the protocol
would end up in two places. Also, pkt-lines which are already at the
length limit would need special handling.
Both of these solutions would require a fair amount of work, whereas
this hack is easy and solves at least some of the problem.
Still to do: it would be good to give a better error message
than "fatal: The remote end hung up unexpectedly".
Signed-off-by: David Turner <dturner@twosigma.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
remote-curl.c | 8 ++++++++
t/t5551-http-fetch-smart.sh | 30 ++++++++++++++++++++++++++++++
2 files changed, 38 insertions(+)
diff --git a/remote-curl.c b/remote-curl.c
index f14c41f4c0..ee4423659f 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -400,6 +400,7 @@ struct rpc_state {
size_t pos;
int in;
int out;
+ int any_written;
struct strbuf result;
unsigned gzip_request : 1;
unsigned initial_buffer : 1;
@@ -456,6 +457,8 @@ static size_t rpc_in(char *ptr, size_t eltsize,
{
size_t size = eltsize * nmemb;
struct rpc_state *rpc = buffer_;
+ if (size)
+ rpc->any_written = 1;
write_or_die(rpc->in, ptr, size);
return size;
}
@@ -659,6 +662,8 @@ static int post_rpc(struct rpc_state *rpc)
curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, rpc_in);
curl_easy_setopt(slot->curl, CURLOPT_FILE, rpc);
+
+ rpc->any_written = 0;
err = run_slot(slot, NULL);
if (err == HTTP_REAUTH && !large_request) {
credential_fill(&http_auth);
@@ -667,6 +672,9 @@ static int post_rpc(struct rpc_state *rpc)
if (err != HTTP_OK)
err = -1;
+ if (!rpc->any_written)
+ err = -1;
+
curl_slist_free_all(headers);
free(gzip_body);
return err;
diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index 1ec5b2747a..43665ab4a8 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -276,6 +276,36 @@ test_expect_success 'large fetch-pack requests can be split across POSTs' '
test_line_count = 2 posts
'
+test_expect_success 'test allowreachablesha1inwant' '
+ test_when_finished "rm -rf test_reachable.git" &&
+ server="$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
+ master_sha=$(git -C "$server" rev-parse refs/heads/master) &&
+ git -C "$server" config uploadpack.allowreachablesha1inwant 1 &&
+
+ git init --bare test_reachable.git &&
+ git -C test_reachable.git remote add origin "$HTTPD_URL/smart/repo.git" &&
+ git -C test_reachable.git fetch origin "$master_sha"
+'
+
+test_expect_success 'test allowreachablesha1inwant with unreachable' '
+ test_when_finished "rm -rf test_reachable.git; git reset --hard $(git rev-parse HEAD)" &&
+
+ #create unreachable sha
+ echo content >file2 &&
+ git add file2 &&
+ git commit -m two &&
+ git push public HEAD:refs/heads/doomed &&
+ git push public :refs/heads/doomed &&
+
+ server="$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
+ master_sha=$(git -C "$server" rev-parse refs/heads/master) &&
+ git -C "$server" config uploadpack.allowreachablesha1inwant 1 &&
+
+ git init --bare test_reachable.git &&
+ git -C test_reachable.git remote add origin "$HTTPD_URL/smart/repo.git" &&
+ test_must_fail git -C test_reachable.git fetch origin "$(git rev-parse HEAD)"
+'
+
test_expect_success EXPENSIVE 'http can handle enormous ref negotiation' '
(
cd "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
--
2.11.0-442-g0c85c54a77
^ permalink raw reply related
* [PATCH 4/2] t5615-alternate-env: double-quotes in file names do not work on Windows
From: Johannes Sixt @ 2016-12-21 21:33 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Klaus Ethgen, git
In-Reply-To: <d9d2580c-a2e5-d9f3-1f56-6814b2b2285d@kdbg.org>
Protect a recently added test case with !MINGW.
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
I don't remember why I did not notice this failure sooner.
Perhaps I did, but then ran out of time to debug it...
The patch should go on top of jk/quote-env-path-list-component.
t/t5615-alternate-env.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/t5615-alternate-env.sh b/t/t5615-alternate-env.sh
index 69fd8f8911..26ebb0375d 100755
--- a/t/t5615-alternate-env.sh
+++ b/t/t5615-alternate-env.sh
@@ -79,7 +79,7 @@ test_expect_success 'mix of quoted and unquoted alternates' '
$two blob
'
-test_expect_success 'broken quoting falls back to interpreting raw' '
+test_expect_success !MINGW 'broken quoting falls back to interpreting raw' '
mv one.git \"one.git &&
check_obj \"one.git/objects <<-EOF
$one blob
--
2.11.0.79.gf6b77ca
^ permalink raw reply related
* [PATCH bw/realpath-wo-chdir] real_path: canonicalize directory separators in root parts
From: Johannes Sixt @ 2016-12-21 21:51 UTC (permalink / raw)
To: Brandon Williams
Cc: git, sbeller, peff, jacob.keller, gitster, ramsay, tboegi,
pclouds
In-Reply-To: <1481566615-75299-1-git-send-email-bmwill@google.com>
When an absolute path is resolved, resolution begins at the first path
component after the root part. The root part is just copied verbatim,
because it must not be inspected for symbolic links. For POSIX paths,
this is just the initial slash, but on Windows, the root part has the
forms c:\ or \\server\share. We do want to canonicalize the back-slashes
in the root part because these parts are compared to the result of
getcwd(), which does return a fully canonicalized path.
Factor out a helper that splits off the root part, and have it
canonicalize the copied part.
This change was prompted because t1504-ceiling-dirs.sh caught a breakage
in GIT_CEILING_DIRECTORIES handling on Windows.
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
This introduces the second #ifdef GIT_WINDOWS_NATIVE in this file.
It could be avoided if convert_slashes were defined as a do-nothing
on POSIX, but that would not help the other occurrence. Therefore,
I suggest to leave it at this.
abspath.c | 29 +++++++++++++++++------------
1 file changed, 17 insertions(+), 12 deletions(-)
diff --git a/abspath.c b/abspath.c
index 79ee310867..1d56f5ed9f 100644
--- a/abspath.c
+++ b/abspath.c
@@ -48,6 +48,19 @@ static void get_next_component(struct strbuf *next, struct strbuf *remaining)
strbuf_remove(remaining, 0, end - remaining->buf);
}
+/* copies root part from remaining to resolved, canonicalizing it on the way */
+static void get_root_part(struct strbuf *resolved, struct strbuf *remaining)
+{
+ int offset = offset_1st_component(remaining->buf);
+
+ strbuf_reset(resolved);
+ strbuf_add(resolved, remaining->buf, offset);
+#ifdef GIT_WINDOWS_NATIVE
+ convert_slashes(resolved->buf);
+#endif
+ strbuf_remove(remaining, 0, offset);
+}
+
/* We allow "recursive" symbolic links. Only within reason, though. */
#define MAXSYMLINKS 5
@@ -80,14 +93,10 @@ char *strbuf_realpath(struct strbuf *resolved, const char *path,
goto error_out;
}
- strbuf_reset(resolved);
+ strbuf_addstr(&remaining, path);
+ get_root_part(resolved, &remaining);
- if (is_absolute_path(path)) {
- /* absolute path; start with only root as being resolved */
- int offset = offset_1st_component(path);
- strbuf_add(resolved, path, offset);
- strbuf_addstr(&remaining, path + offset);
- } else {
+ if (!resolved->len) {
/* relative path; can use CWD as the initial resolved path */
if (strbuf_getcwd(resolved)) {
if (die_on_error)
@@ -95,7 +104,6 @@ char *strbuf_realpath(struct strbuf *resolved, const char *path,
else
goto error_out;
}
- strbuf_addstr(&remaining, path);
}
/* Iterate over the remaining path components */
@@ -150,10 +158,7 @@ char *strbuf_realpath(struct strbuf *resolved, const char *path,
if (is_absolute_path(symlink.buf)) {
/* absolute symlink; set resolved to root */
- int offset = offset_1st_component(symlink.buf);
- strbuf_reset(resolved);
- strbuf_add(resolved, symlink.buf, offset);
- strbuf_remove(&symlink, 0, offset);
+ get_root_part(resolved, &symlink);
} else {
/*
* relative symlink
--
2.11.0.79.gf6b77ca
^ permalink raw reply related
* Re: Bug report: Git pull hang occasionally
From: Kai Zhang @ 2016-12-21 22:31 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqshphge7o.fsf@gitster.mtv.corp.google.com>
I will verify it. Thanks.
> On Dec 21, 2016, at 1:32 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> Junio C Hamano <gitster@pobox.com> writes:
>
>> And the unexpected discrepancy is reported by find_symref() as
>> fatal. The server side dies, and somehow that fact is lost between
>> the upload-pack process and the client and somebody in the middle
>> (e.g. fastcgi interface or nginx webserver on the server side, or
>> the remote-curl helper on the client side) keeps the "git fetch"
>> process waiting.
>>
>> So there seem to be two issues.
>>
>> - Because of the unlocked read, find_symref() can observe an
>> inconsistent state. Perhaps it should be updated not to die but
>> to retry, expecting that transient inconsistency will go away.
>>
>> - A fatal error in upload-pack is not reported back to the client
>> to cause it exit is an obvious one, and even if we find a way to
>> make this fatal error in find_symref() not to trigger, fatal
>> errors in other places in the code can trigger the same symptom.
>
> I wonder if the latter is solved by recent patch 296b847c0d
> ("remote-curl: don't hang when a server dies before any output",
> 2016-11-18) on the client side.
>
> -- >8 --
> From: David Turner <dturner@twosigma.com>
> Date: Fri, 18 Nov 2016 15:30:49 -0500
> Subject: [PATCH] remote-curl: don't hang when a server dies before any output
>
> In the event that a HTTP server closes the connection after giving a
> 200 but before giving any packets, we don't want to hang forever
> waiting for a response that will never come. Instead, we should die
> immediately.
>
> One case where this happens is when attempting to fetch a dangling
> object by its object name. In this case, the server dies before
> sending any data. Prior to this patch, fetch-pack would wait for
> data from the server, and remote-curl would wait for fetch-pack,
> causing a deadlock.
>
> Despite this patch, there is other possible malformed input that could
> cause the same deadlock (e.g. a half-finished pktline, or a pktline but
> no trailing flush). There are a few possible solutions to this:
>
> 1. Allowing remote-curl to tell fetch-pack about the EOF (so that
> fetch-pack could know that no more data is coming until it says
> something else). This is tricky because an out-of-band signal would
> be required, or the http response would have to be re-framed inside
> another layer of pkt-line or something.
>
> 2. Make remote-curl understand some of the protocol. It turns out
> that in addition to understanding pkt-line, it would need to watch for
> ack/nak. This is somewhat fragile, as information about the protocol
> would end up in two places. Also, pkt-lines which are already at the
> length limit would need special handling.
>
> Both of these solutions would require a fair amount of work, whereas
> this hack is easy and solves at least some of the problem.
>
> Still to do: it would be good to give a better error message
> than "fatal: The remote end hung up unexpectedly".
>
> Signed-off-by: David Turner <dturner@twosigma.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> remote-curl.c | 8 ++++++++
> t/t5551-http-fetch-smart.sh | 30 ++++++++++++++++++++++++++++++
> 2 files changed, 38 insertions(+)
>
> diff --git a/remote-curl.c b/remote-curl.c
> index f14c41f4c0..ee4423659f 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -400,6 +400,7 @@ struct rpc_state {
> size_t pos;
> int in;
> int out;
> + int any_written;
> struct strbuf result;
> unsigned gzip_request : 1;
> unsigned initial_buffer : 1;
> @@ -456,6 +457,8 @@ static size_t rpc_in(char *ptr, size_t eltsize,
> {
> size_t size = eltsize * nmemb;
> struct rpc_state *rpc = buffer_;
> + if (size)
> + rpc->any_written = 1;
> write_or_die(rpc->in, ptr, size);
> return size;
> }
> @@ -659,6 +662,8 @@ static int post_rpc(struct rpc_state *rpc)
> curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, rpc_in);
> curl_easy_setopt(slot->curl, CURLOPT_FILE, rpc);
>
> +
> + rpc->any_written = 0;
> err = run_slot(slot, NULL);
> if (err == HTTP_REAUTH && !large_request) {
> credential_fill(&http_auth);
> @@ -667,6 +672,9 @@ static int post_rpc(struct rpc_state *rpc)
> if (err != HTTP_OK)
> err = -1;
>
> + if (!rpc->any_written)
> + err = -1;
> +
> curl_slist_free_all(headers);
> free(gzip_body);
> return err;
> diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
> index 1ec5b2747a..43665ab4a8 100755
> --- a/t/t5551-http-fetch-smart.sh
> +++ b/t/t5551-http-fetch-smart.sh
> @@ -276,6 +276,36 @@ test_expect_success 'large fetch-pack requests can be split across POSTs' '
> test_line_count = 2 posts
> '
>
> +test_expect_success 'test allowreachablesha1inwant' '
> + test_when_finished "rm -rf test_reachable.git" &&
> + server="$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
> + master_sha=$(git -C "$server" rev-parse refs/heads/master) &&
> + git -C "$server" config uploadpack.allowreachablesha1inwant 1 &&
> +
> + git init --bare test_reachable.git &&
> + git -C test_reachable.git remote add origin "$HTTPD_URL/smart/repo.git" &&
> + git -C test_reachable.git fetch origin "$master_sha"
> +'
> +
> +test_expect_success 'test allowreachablesha1inwant with unreachable' '
> + test_when_finished "rm -rf test_reachable.git; git reset --hard $(git rev-parse HEAD)" &&
> +
> + #create unreachable sha
> + echo content >file2 &&
> + git add file2 &&
> + git commit -m two &&
> + git push public HEAD:refs/heads/doomed &&
> + git push public :refs/heads/doomed &&
> +
> + server="$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
> + master_sha=$(git -C "$server" rev-parse refs/heads/master) &&
> + git -C "$server" config uploadpack.allowreachablesha1inwant 1 &&
> +
> + git init --bare test_reachable.git &&
> + git -C test_reachable.git remote add origin "$HTTPD_URL/smart/repo.git" &&
> + test_must_fail git -C test_reachable.git fetch origin "$(git rev-parse HEAD)"
> +'
> +
> test_expect_success EXPENSIVE 'http can handle enormous ref negotiation' '
> (
> cd "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
> --
> 2.11.0-442-g0c85c54a77
>
^ permalink raw reply
* Re: [PATCH bw/realpath-wo-chdir] real_path: canonicalize directory separators in root parts
From: Brandon Williams @ 2016-12-21 22:33 UTC (permalink / raw)
To: Johannes Sixt
Cc: git, sbeller, peff, jacob.keller, gitster, ramsay, tboegi,
pclouds
In-Reply-To: <097e3e2e-f46d-b0aa-be9c-68c274c5e3dc@kdbg.org>
On 12/21, Johannes Sixt wrote:
> When an absolute path is resolved, resolution begins at the first path
> component after the root part. The root part is just copied verbatim,
> because it must not be inspected for symbolic links. For POSIX paths,
> this is just the initial slash, but on Windows, the root part has the
> forms c:\ or \\server\share. We do want to canonicalize the back-slashes
> in the root part because these parts are compared to the result of
> getcwd(), which does return a fully canonicalized path.
>
> Factor out a helper that splits off the root part, and have it
> canonicalize the copied part.
>
> This change was prompted because t1504-ceiling-dirs.sh caught a breakage
> in GIT_CEILING_DIRECTORIES handling on Windows.
>
> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> ---
> This introduces the second #ifdef GIT_WINDOWS_NATIVE in this file.
> It could be avoided if convert_slashes were defined as a do-nothing
> on POSIX, but that would not help the other occurrence. Therefore,
> I suggest to leave it at this.
>
> abspath.c | 29 +++++++++++++++++------------
> 1 file changed, 17 insertions(+), 12 deletions(-)
>
> diff --git a/abspath.c b/abspath.c
> index 79ee310867..1d56f5ed9f 100644
> --- a/abspath.c
> +++ b/abspath.c
> @@ -48,6 +48,19 @@ static void get_next_component(struct strbuf *next, struct strbuf *remaining)
> strbuf_remove(remaining, 0, end - remaining->buf);
> }
>
> +/* copies root part from remaining to resolved, canonicalizing it on the way */
> +static void get_root_part(struct strbuf *resolved, struct strbuf *remaining)
> +{
> + int offset = offset_1st_component(remaining->buf);
> +
> + strbuf_reset(resolved);
> + strbuf_add(resolved, remaining->buf, offset);
> +#ifdef GIT_WINDOWS_NATIVE
> + convert_slashes(resolved->buf);
> +#endif
So then the only extra cononicalization that is happening here is
converting '\\server\share' to '//server/share'? (or 'c:\' to 'c:/')
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH 4/2] t5615-alternate-env: double-quotes in file names do not work on Windows
From: Jeff King @ 2016-12-21 22:42 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Junio C Hamano, Klaus Ethgen, git
In-Reply-To: <00b6235d-c1bc-30c2-6539-6c78c4ce9eb0@kdbg.org>
On Wed, Dec 21, 2016 at 10:33:43PM +0100, Johannes Sixt wrote:
> Protect a recently added test case with !MINGW.
>
> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> ---
> I don't remember why I did not notice this failure sooner.
> Perhaps I did, but then ran out of time to debug it...
>
> The patch should go on top of jk/quote-env-path-list-component.
> [...]
> -test_expect_success 'broken quoting falls back to interpreting raw' '
> +test_expect_success !MINGW 'broken quoting falls back to interpreting raw' '
> mv one.git \"one.git &&
> check_obj \"one.git/objects <<-EOF
> $one blob
Hmph. I explicitly avoided a colon in the filename so that it would run
on MINGW. Is a double-quote also not allowed?
-Peff
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox