* 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: 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: 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: [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: 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
* 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: [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
* 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 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] 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
* 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] 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
* 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 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
* [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
* [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 0/2] Really fix the isatty() problem on Windows
From: Johannes Schindelin @ 2016-12-21 17:53 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Pranit Bauva, Johannes Sixt
My previous fix may have fixed running the new
t/t6030-bisect-porcelain.sh script that tested the new bisect--helper,
which in turn used isatty() to determine whether it was running
interactively and was fooled by being redirected to /dev/null.
But it not only broke paging when running in a CMD window, due to
testing in the wrong worktree I also missed that it broke paging in Git
for Windows 2.x' Git Bash (i.e. a MinTTY terminal emulator).
Let's use this opportunity to actually clean up the entire isatty() mess
once and for all, as part of the problem was introduced by a clever hack
that messes with internals of the Microsoft C runtime, and which changed
recently, so it was not such a clever hack to begin with.
Happily, one of my colleagues had to address that latter problem
recently when he was tasked to make Git compile with Microsoft Visual C
(the rationale: debugging facilities of Visual Studio are really
outstanding, try them if you get a chance).
And incidentally, replacing the previous hack with the clean, new
solution, which specifies explicitly for the file descriptors 0, 1 and 2
whether we detected an MSYS2 pseudo-tty, whether we detected a real
Win32 Console, and whether we had to swap out a real Win32 Console for a
pipe to allow child processes to inherit it.
Hannes, I am really sorry that I did not test the original attempt
better, I tried to be a better boy this time. Could you maybe also give
it a try?
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
Jeff Hostetler (1):
mingw: replace isatty() hack
Johannes Schindelin (1):
mingw: adjust is_console() to work with stdin
compat/winansi.c | 197 +++++++++++++++++++++++--------------------------------
1 file changed, 83 insertions(+), 114 deletions(-)
base-commit: 1921ea0f1c7358b5200a456fba02aa79fb9e76d3
Based-On: junio/pu at https://github.com/dscho/git
Fetch-Base-Via: git fetch https://github.com/dscho/git junio/pu
Published-As: https://github.com/dscho/git/releases/tag/mingw-isatty-fixup-v1
Fetch-It-Via: git fetch https://github.com/dscho/git mingw-isatty-fixup-v1
--
2.11.0.rc3.windows.1
^ permalink raw reply
* Re: [PATCH] string-list: make compare function compatible with qsort(3)
From: Jeff King @ 2016-12-21 16:12 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List, Junio C Hamano, Johannes Schindelin
In-Reply-To: <c7bac0b7-c555-162f-7880-0355831cee48@web.de>
On Wed, Dec 21, 2016 at 10:36:41AM +0100, René Scharfe wrote:
> 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 this approach is OK for string_list, but it doesn't help the
general case that wants qsort_s() to actually access global data. I
don't know how common that is in our codebase, though.
So I'm fine with it, but I think we might eventually need to revisit the
qsort_s() thing anyway.
> Remove the intermediate layer, i.e. cmp_items(), make the comparison
> functions compatible with qsort(3) and pass them pointers to full items.
> This allows comparisons to also take the util member into account, and
> avoids the need to pass the real comparison function to an intermediate
> function, removing the need for a global function.
I'm not sure if access to the util field is really of any value, after
looking at it in:
http://public-inbox.org/git/20161125171546.fa3zpapbjngjcl26@sigill.intra.peff.net/
Though note that if we do take this patch, there are probably one or two
spots that could switch from QSORT() to string_list_sort().
-Peff
^ permalink raw reply
* Re: Allow "git shortlog" to group by committer information
From: Jeff King @ 2016-12-21 16:04 UTC (permalink / raw)
To: Jacob Keller
Cc: Junio C Hamano, Johannes Sixt, Linus Torvalds, Git Mailing List
In-Reply-To: <CA+P7+xrMgzFcuqwBg6z2_ZPgAVKwLX2eyK6D4C0v-c3zAMFqUg@mail.gmail.com>
On Tue, Dec 20, 2016 at 11:55:45PM -0800, Jacob Keller wrote:
> > 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.
> >
> > -Peff
>
> That seems like a good idea, but I'm not sure how you would implement
> it in practice? Would we just "rely" on a skipable test having a "do
> this if we skip, instead" block? That would be easier to spot but I
> think still relies on the skip-able tests being careful?
Yes, it definitely means the skip-able tests would have to be careful.
But it's putting the onus on them, rather than on all the other tests.
If the rule is "the on-disk state must be the same whether or not the
test runs", then I suspect many tests could get by with a
test_when_finished, like:
test_expect_success FOO 'test --foo knob' '
git commit -m "new commit for test" &&
test_when_finished "git reset --hard HEAD^" &&
git log --foo >actual &&
test_cmp expect actual
'
It gets harder if you have multiple such tests that rely on intermediate
state. Probably you'd have:
test_expect_success FOO 'clean up FOO state' '
git reset --hard HEAD^
'
at the end of the sequence.
I dunno. It is unclear to me whether such a rule is worth it.
Preemptively fighting these state-based bugs before they occur is a nice
thought, but I think it may end up being more work to write the tests
that way than it is to simply find and fix the bugs when they occur. Of
course it also changes where the work falls (the test writers versus the
bug hunters, and given that !MINGW is our most common prereq, I think
Windows devs are over-represented in the latter case).
-Peff
^ permalink raw reply
* Re: [PATCH] mailinfo.c: move side-effects outside of assert
From: Jeff King @ 2016-12-21 15:55 UTC (permalink / raw)
To: Kyle J. McKay
Cc: Johannes Schindelin, Jonathan Tan, Junio C Hamano,
Git mailing list
In-Reply-To: <222ACFD4-ED9A-4B94-8BDD-3C70648A684B@gmail.com>
On Tue, Dec 20, 2016 at 09:54:15PM -0800, Kyle J. McKay wrote:
> > I wasn't aware anybody actually built with NDEBUG at all. You'd have to
> > explicitly ask for it via CFLAGS, so I assume most people don't.
>
> Not a good assumption. You know what happens when you assume[1], right? ;)
Kind of. If it's a configuration that nobody[1] in the Git development
community intended to support or test, then isn't the person triggering
it the one making assumptions?
At any rate, I agree that setting NDEBUG should not create a broken
program, and some solution like your patch is a good idea here. I was
mainly speaking to the "do not bother" comment. It is not that I do not
bother to build with NDEBUG, it is that I think it is actively a bad
idea.
[1] Maybe I am alone in my surprise, and everybody working on Git is
using assert() with the intention that it can be disabled. But if
that were the case, I'd expect more push-back against "die(BUG)"
which does not have this feature. I don't recall a single discussion
to that effect, and searching for NDEBUG in the list archives turns
up hardly any mentions.
> I've been defining NDEBUG whenever I make a release build for quite some
> time (not just for Git) in order to squeeze every last possible drop of
> performance out of it.
I think here you are getting into superstition. Is there any single
assert() in Git that will actually have an impact on performance?
I'd be more impressed if you could show some operation that is faster
when built with NDEBUG than without. Running all of t/perf does not seem
to show any difference, and looking at the asserts themselves, they're
almost all single-instruction compares in code that isn't performance
critical anyway.
> > So from my perspective it is not so much "do not bother with release
> > builds" as "are release builds even a thing for git?"
>
> They should be if you're deploying Git in a performance critical
> environment.
I hope my history of patches shows that I do care about deploying Git in
a performance critical environment. But I only care about performance
tradeoffs that have a _measurable_ gain.
> Perhaps Git should provide a "verify" macro. Works like "assert" except
> that it doesn't go away when NDEBUG is defined. Being Git-provided it could
> also use Git's die function. Then Git could do a global replace of assert
> with verify and institute a no-assert policy.
What would be the advantage of that over `if(...) die("BUG: ...")`? It
does not require you to write a reason in the die(), but I am not sure
that is a good thing.
> > I do notice that we set NDEBUG for nedmalloc, though if I am reading the
> > Makefile right, it is just for compiling those files. It looks like
> > there are a ton of asserts there that _are_ potentially expensive, so
> > that makes sense.
>
> So there's no way to get a non-release build of nedmalloc inside Git then
> without hacking the Makefile? What if you need those assertions enabled?
> Maybe NDEBUG shouldn't be defined by default for any files.
AFAICT, yes. I'd leave it to people who actually build with nedmalloc to
decide whether it is worth caring about, and whether the asserts there
have a noticeable performance impact.
-Peff
^ permalink raw reply
* Re: Races on ref .lock files?
From: Andreas Krey @ 2016-12-21 10:00 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqpokru6yg.fsf@gitster.mtv.corp.google.com>
On Fri, 16 Dec 2016 09:20:07 +0000, Junio C Hamano wrote:
...
> > error: cannot lock ref 'stash-refs/pull-requests/18978/to': Unable to create '/opt/apps/..../repositories/68/stash-refs/pull-requests/18978/to.lock': File exists.
...
> I think (and I think you also think) these messages come from the
> Bitbucket side, not your "git push" (or "git fetch").
I *know* that this is the case - we don't have refs like that
on the local side. (Our users are more scared about them.)
...
> when there are locked refs. I'd naively think that unless you are
> pushing to that ref you showed an error message for, the receiving
> end shouldn't care if the ref is being written by somebody else, but
> who knows ;-) They may have their own reasons wanting to lock that
> ref that we think would be irrelevant for the operation, causing
> errors.
Possible. I'm going Byrans way for now, disabling the gc there.
But:
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."
Is that happening as designed, or do I need to be afraid
that some refs didn't make the push?
To ask differently, is git relying on people reading such
messages and following up on them? And thus isn't that
easy to use in automated processes? (Additional problem:
The user in question, besides being an automat, doesn't
have the capability to work in the target repository.)
Andreas
--
"Totally trivial. Famous last words."
From: Linus Torvalds <torvalds@*.org>
Date: Fri, 22 Jan 2010 07:29:21 -0800
^ permalink raw reply
* [PATCH] string-list: make compare function compatible with qsort(3)
From: René Scharfe @ 2016-12-21 9:36 UTC (permalink / raw)
To: Git List; +Cc: Jeff King, Junio C Hamano, Johannes Schindelin
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.
Remove the intermediate layer, i.e. cmp_items(), make the comparison
functions compatible with qsort(3) and pass them pointers to full items.
This allows comparisons to also take the util member into account, and
avoids the need to pass the real comparison function to an intermediate
function, removing the need for a global function.
A downside is that comparison functions take void pointers now and each
of them needs to cast its arguments to struct string_list_item pointers,
weakening type safety and adding some repetitive code. Programmers are
used to that, however, as that's par for the course with qsort(3).
Also two unsightly casts are added that remove the const qualifiers of
strings while building the structs to pass to the comparison function as
search keys. It should be safe, though, as we only ever use them for
reading.
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
Alternative approach to the qsort_s()-based series "string-list: make
string_list_sort() reentrant".
Documentation/technical/api-string-list.txt | 6 +++--
builtin/mailsplit.c | 5 +++-
mailmap.c | 5 ++--
merge-recursive.c | 4 ++-
string-list.c | 39 +++++++++++++++--------------
string-list.h | 4 +--
tmp-objdir.c | 4 ++-
7 files changed, 39 insertions(+), 28 deletions(-)
diff --git a/Documentation/technical/api-string-list.txt b/Documentation/technical/api-string-list.txt
index c08402b12..39eac59c7 100644
--- a/Documentation/technical/api-string-list.txt
+++ b/Documentation/technical/api-string-list.txt
@@ -205,5 +205,7 @@ Represents the list itself.
You should not tamper with it.
. Setting the `strdup_strings` member to 1 will strdup() the strings
before adding them, see above.
-. The `compare_strings_fn` member is used to specify a custom compare
- function, otherwise `strcmp()` is used as the default function.
+. The `cmp` member is used to specify a custom compare function. It has
+ the same signature as the one for qsort(1) and is passed two pointers
+ to `struct string_list_item`. If it's NULL then the `string` members
+ are compared with `strcmp(1)`; this is the default.
diff --git a/builtin/mailsplit.c b/builtin/mailsplit.c
index 30681681c..4e72e3128 100644
--- a/builtin/mailsplit.c
+++ b/builtin/mailsplit.c
@@ -147,8 +147,11 @@ static int populate_maildir_list(struct string_list *list, const char *path)
return ret;
}
-static int maildir_filename_cmp(const char *a, const char *b)
+static int maildir_filename_cmp(const void *one, const void *two)
{
+ const struct string_list_item *item_one = one, *item_two = two;
+ const char *a = item_one->string, *b = item_two->string;
+
while (*a && *b) {
if (isdigit(*a) && isdigit(*b)) {
long int na, nb;
diff --git a/mailmap.c b/mailmap.c
index c1a79c100..5290b5153 100644
--- a/mailmap.c
+++ b/mailmap.c
@@ -61,9 +61,10 @@ static void free_mailmap_entry(void *p, const char *s)
* namemap.cmp until we know no systems that matter have such an
* "unusual" string.h.
*/
-static int namemap_cmp(const char *a, const char *b)
+static int namemap_cmp(const void *one, const void *two)
{
- return strcasecmp(a, b);
+ const struct string_list_item *item_one = one, *item_two = two;
+ return strcasecmp(item_one->string, item_two->string);
}
static void add_mapping(struct string_list *map,
diff --git a/merge-recursive.c b/merge-recursive.c
index d32720944..4683ba43f 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -390,8 +390,10 @@ static struct string_list *get_unmerged(void)
return unmerged;
}
-static int string_list_df_name_compare(const char *one, const char *two)
+static int string_list_df_name_compare(const void *a, const void *b)
{
+ const struct string_list_item *item_a = a, *item_b = b;
+ const char *one = item_a->string, *two = item_b->string;
int onelen = strlen(one);
int twolen = strlen(two);
/*
diff --git a/string-list.c b/string-list.c
index 8c83cac18..c583a04ee 100644
--- a/string-list.c
+++ b/string-list.c
@@ -7,17 +7,26 @@ void string_list_init(struct string_list *list, int strdup_strings)
list->strdup_strings = strdup_strings;
}
+static int string_list_item_strcmp(const void *one, const void *two)
+{
+ const struct string_list_item *item_one = one, *item_two = two;
+ return strcmp(item_one->string, item_two->string);
+}
+
/* if there is no exact match, point to the index where the entry could be
* inserted */
static int get_entry_index(const struct string_list *list, const char *string,
int *exact_match)
{
int left = -1, right = list->nr;
- compare_strings_fn cmp = list->cmp ? list->cmp : strcmp;
+ compare_fn_t cmp = list->cmp ? list->cmp : string_list_item_strcmp;
+ struct string_list_item key = { NULL };
+
+ key.string = (char *)string;
while (left + 1 < right) {
int middle = (left + right) / 2;
- int compare = cmp(string, list->items[middle].string);
+ int compare = cmp(&key, &list->items[middle]);
if (compare < 0)
right = middle;
else if (compare > 0)
@@ -94,11 +103,11 @@ struct string_list_item *string_list_lookup(struct string_list *list, const char
void string_list_remove_duplicates(struct string_list *list, int free_util)
{
+ compare_fn_t cmp = list->cmp ? list->cmp : string_list_item_strcmp;
if (list->nr > 1) {
int src, dst;
- compare_strings_fn cmp = list->cmp ? list->cmp : strcmp;
for (src = dst = 1; src < list->nr; src++) {
- if (!cmp(list->items[dst - 1].string, list->items[src].string)) {
+ if (!cmp(&list->items[dst - 1], &list->items[src])) {
if (list->strdup_strings)
free(list->items[src].string);
if (free_util)
@@ -211,31 +220,23 @@ struct string_list_item *string_list_append(struct string_list *list,
list->strdup_strings ? xstrdup(string) : (char *)string);
}
-/* Yuck */
-static compare_strings_fn compare_for_qsort;
-
-/* Only call this from inside string_list_sort! */
-static int cmp_items(const void *a, const void *b)
-{
- const struct string_list_item *one = a;
- const struct string_list_item *two = b;
- return compare_for_qsort(one->string, two->string);
-}
-
void string_list_sort(struct string_list *list)
{
- compare_for_qsort = list->cmp ? list->cmp : strcmp;
- QSORT(list->items, list->nr, cmp_items);
+ compare_fn_t cmp = list->cmp ? list->cmp : string_list_item_strcmp;
+ QSORT(list->items, list->nr, cmp);
}
struct string_list_item *unsorted_string_list_lookup(struct string_list *list,
const char *string)
{
struct string_list_item *item;
- compare_strings_fn cmp = list->cmp ? list->cmp : strcmp;
+ compare_fn_t cmp = list->cmp ? list->cmp : string_list_item_strcmp;
+ struct string_list_item key = { NULL };
+
+ key.string = (char *)string;
for_each_string_list_item(item, list)
- if (!cmp(string, item->string))
+ if (!cmp(&key, item))
return item;
return NULL;
}
diff --git a/string-list.h b/string-list.h
index d3809a141..073025ddc 100644
--- a/string-list.h
+++ b/string-list.h
@@ -6,13 +6,13 @@ struct string_list_item {
void *util;
};
-typedef int (*compare_strings_fn)(const char *, const char *);
+typedef int (*compare_fn_t)(const void *, const void *);
struct string_list {
struct string_list_item *items;
unsigned int nr, alloc;
unsigned int strdup_strings:1;
- compare_strings_fn cmp; /* NULL uses strcmp() */
+ compare_fn_t cmp; /* NULL uses strcmp() on ->string */
};
#define STRING_LIST_INIT_NODUP { NULL, 0, 0, 0, NULL }
diff --git a/tmp-objdir.c b/tmp-objdir.c
index 64435f23a..b6209b199 100644
--- a/tmp-objdir.c
+++ b/tmp-objdir.c
@@ -173,8 +173,10 @@ static int pack_copy_priority(const char *name)
return 4;
}
-static int pack_copy_cmp(const char *a, const char *b)
+static int pack_copy_cmp(const void *one, const void *two)
{
+ const struct string_list_item *item_one = one, *item_two = two;
+ const char *a = item_one->string, *b = item_two->string;
return pack_copy_priority(a) - pack_copy_priority(b);
}
--
2.11.0
^ permalink raw reply related
* Re: [PATCH 1/3] compat: add qsort_s()
From: René Scharfe @ 2016-12-21 9:36 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Git List, Johannes Schindelin
In-Reply-To: <20161212195703.nmljhwwmrn56gbyd@sigill.intra.peff.net>
Am 12.12.2016 um 20:57 schrieb Jeff King:
> On Mon, Dec 12, 2016 at 08:51:14PM +0100, René Scharfe wrote:
>
>> It's kinda cool to have a bespoke compatibility layer for major platforms,
>> but the more I think about it the less I can answer why we would want that.
>> Safety, reliability and performance can't be good reasons -- if our fallback
>> function lacks in these regards then we have to improve it in any case.
>
> There may be cases that we don't want to support because of portability
> issues. E.g., if your libc has an assembly-optimized qsort() we wouldn't
> want to replicate that.
Offloading to GPUs might be a better example; I don't know of a libc
that does any of that, though (yet).
> I dunno. I am not that opposed to just saying "forget libc qsort, we
> always use our internal one which is consistent, performant, and safe".
> But when I suggested something similar for our regex library, I seem to
> recall there were complaints.
Well, I'm not sure how comparable they are, but perhaps we can avoid
compat code altogether in this case. Patch coming in a new thread.
René
^ permalink raw reply
* Re: Races on ref .lock files?
From: Andreas Krey @ 2016-12-21 8:33 UTC (permalink / raw)
To: Bryan Turner; +Cc: Git Users
In-Reply-To: <CAGyf7-EvbV4XWCsfLpCUDo5V4_wM3SSJcHxVh9Rp78JUC6S-yw@mail.gmail.com>
On Fri, 16 Dec 2016 15:34:22 +0000, Bryan Turner wrote:
...
> Bitbucket Server developer here.
Social media rock. :-)
> If you'd like to investigate more in depth, I'd encourage you to
> create a ticket on support.atlassian.com so we can work with you.
That is going to be postponed until we update our bitbucket instance
to the current state.
> Otherwise, if you just want to prevent seeing these messages, you can
> either fork the relevant repository in Bitbucket Server (which
> disables auto GC), or run "git config gc.auto 0" in
Doing that for now. Will come back either if it doesn't help,
or after the upgrade.
> within Bitbucket Server instead, so a future upgrade should
> automatically prevent these messages from appearing on clients.
I still wonder if git itself should prevent these, or is there
a (git level) recommendation not to enable auto-gc in repos where
people regularly push to?
- Andreas
--
"Totally trivial. Famous last words."
From: Linus Torvalds <torvalds@*.org>
Date: Fri, 22 Jan 2010 07:29:21 -0800
^ permalink raw reply
* Re: Allow "git shortlog" to group by committer information
From: Jacob Keller @ 2016-12-21 7:55 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Johannes Sixt, Linus Torvalds, Git Mailing List
In-Reply-To: <20161221032221.s7jmgnfrr6tyuyuk@sigill.intra.peff.net>
On Tue, Dec 20, 2016 at 7:22 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Dec 20, 2016 at 10:35:36AM -0800, Junio C Hamano wrote:
>
>> -- >8 --
>> Subject: SQUASH???
>>
>> Make sure the test does not depend on the result of the previous
>> tests; with MINGW prerequisite satisfied, a "reset to original and
>> rebuild" in an earlier test was skipped, resulting in different
>> history being tested with this and the next tests.
>
> Yeah, this looks good, and obviously correct.
>
> 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.
>
> -Peff
That seems like a good idea, but I'm not sure how you would implement
it in practice? Would we just "rely" on a skipable test having a "do
this if we skip, instead" block? That would be easier to spot but I
think still relies on the skip-able tests being careful?
Thanks,
Jake
^ 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