Re: config for `format-patch --notes`?

[Norbert Kiesel]

Thanks for the reply.  I did not knew that branch descriptions
automatically make it in the cover letter.
Learned something new!

Unfortunately this would still mean I have to manually add the branch
name to the branch description.

I will try to create a patch for adding a config option for the
--notes command line option over the upcoming
free days.

Re: [PATCH 4/2] t5615-alternate-env: double-quotes in file names do not work on Windows

[Johannes Sixt]

Am 21.12.2016 um 23:42 schrieb Jeff King:

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 
---
 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?


It is not allowed; that was my conclusion. But now that you ask, I'll 
double-check.


-- Hannes

Re: [PATCH bw/realpath-wo-chdir] real_path: canonicalize directory separators in root parts

[Johannes Sixt]

Am 21.12.2016 um 23:33 schrieb Brandon Williams:

On 12/21, Johannes Sixt wrote:

+/* 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:/')


Correct. All other directory separators are canonicalized by the primary 
function, strbuf_realpath.


-- Hannes

builtin difftool parsing issue

[Paul Sbarra]

Sadly, I haven't been able to figure out how to get the mbox file from
this tread into gmail, but wanted to report a parsing issue I've found
with the builtin difftool.

Original Patch:
https://public-inbox.org/git/ac91e4818cfb5c5af6b5874662dbeb61cde1f69d.1480019834.git.johannes.schinde...@gmx.de/#t

> + *status = *++p;
> + if (!status || p[1])
> + return error("unexpected trailer: '%s'", p);
> + return 0;

The p[1] null check assumes the status is only one character long, but
git-diff's raw output format shows that a numeric value can follow in
the copy-edit and rename-edit cases.

I'm looking forward to seeing the builtin difftool land.  I came across it
while investigating adding --submodule=diff (expanding on diff's
recent addition) support and this looks more promising then the perl
script.  Hopefully I will make some progress.  Any tips/pointers would
be greatly appreciated.

Thanks

Re: [PATCH] mailinfo.c: move side-effects outside of assert

[Jeff King]

On Wed, Dec 21, 2016 at 07:53:15PM -0800, Kyle J. McKay wrote:

> It seems to me what you are saying is that Git's "assert" calls are
> DIAGNOSTIC and therefore belong in a release build -- well, except for the
> nedmalloc "assert" calls which do not.

Yes, I think that is a good way of thinking about it (modulo that I
really can't say one way or the other about nedmalloc's uses).

There _are_ some DEBUG-type things in Git that are protected by #ifdefs
that default to "off" (grep for DIFF_DEBUG, for instance). I'm actually
of the opinion that debugging code like that should be in all builds and
triggerable at run-time, provided it carries no significant performance
penalty when the run-time switch is not enabled. But I do agree that's a
totally separate question than from your DEBUG/DIAGNOSTIC distinction.

-Peff

Re: [PATCH] mailinfo.c: move side-effects outside of assert

[Kyle J. McKay]

On Dec 21, 2016, at 18:21, Kyle J. McKay wrote:


On Dec 21, 2016, at 07:55, Jeff King wrote:


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?


No, I don't think so.  NDEBUG is very clearly specified in POSIX [1].

If NDEBUG is defined then "assert(...)" disappears (and in a nice  
way so as not to precipitate "unused variable" warnings).  "N" being  
"No" or "Not" or "Negated" or "bar over the top" + "DEBUG" meaning  
Not DEBUG.  So the code that goes away when NDEBUG is defined is  
clearly debug code.


I think there is a useful distinction here that I make that's worth  
sharing.  Perhaps it's splitting hairs, but I categorize this "extra"  
code that we've been discussing ("assert(...)" or "if (!...) die(...)"  
or "verify(...)" into two groups:



1) DEBUG code

This is code that developers use when creating new features.  Or  
helpful code that's needed when stepping through a program with the  
debugger to debug a problem.  Or even code that's only used by some  
kind of external "test".  It may be expensive, it may do things that  
should never be done in a build for wider consumption (such as write  
information to special log files, write special syslog messages  
etc.).  Often this code is used in combination with a "-g" debug  
symbols build and possibly even a "-O0" or "-O1" option.


Code like this has no place in a release executable meant for general  
use by an end user.


2) DIAGNOSTIC code

This is near zero overhead code that is intended to be left in a  
release build meant for general use and normally sits there not doing  
anything and NOT leaching any performance out of the build either.   
Its sole purpose in life is to provide a trail of "bread crumbs" if  
the executable goes ***BOOM***.  These "bread crumbs" should be just  
enough when combined with feedback from the unfortunate user who  
experienced the meltdown to re-create the issue in a real DEBUG build  
and find and fix the problem.



It seems to me what you are saying is that Git's "assert" calls are  
DIAGNOSTIC and therefore belong in a release build -- well, except for  
the nedmalloc "assert" calls which do not.


What I'm saying is if they are diagnostic and not debug (and I'm not  
arguing one way or the other, but you've already indicated they are  
near zero overhead which suggests they are indeed diagnostic in  
nature), then they do not belong inside an "assert" which can be  
disabled with "NDEBUG".  I'm arguing that "assert" is not intended for  
diagnostic code, but only debug code as used by nedmalloc.  Having Git  
treat "NDEBUG" one way -- "no, no, do NOT define NDEBUG because that  
disables Git diagnostics and I promise you there's no performance  
penalty" -- versus nedmalloc -- "yes, yes please DO define NDEBUG  
unless you really need our slow debugging code to be present for  
debugging purposes" -- just creates needless unnecessary confusion.


--Kyle

Re: [PATCH] mailinfo.c: move side-effects outside of assert

[Jeff King]

On Wed, Dec 21, 2016 at 06:21:37PM -0800, Kyle J. McKay wrote:

> > 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?
> 
> No, I don't think so.  NDEBUG is very clearly specified in POSIX [1].

I know what NDEBUG is, and I know it is widely used in other projects.
My claim is only that I do not think that the use of NDEBUG was
generally considered when people wrote assert() in git. That is
certainly true of me. I don't know about other developers.

> > I think here you are getting into superstition. Is there any single
> > assert() in Git that will actually have an impact on performance?
> 
> You have suggested there is and that Git is enabling NDEBUG for exactly that
> reason -- to increase performance:

Sorry if I wasn't clear, but I meant in Git itself, not in the nedmalloc
compat code. I think it's worth considering them separately because the
latter is not part of most Git builds in the first place, and certainly
it is written in a different style, and may have different assumptions
about how people might build it.

I do not know the nedmalloc code well at all. I gave a brief read of the
results of "git grep 'assert('" and it looked like some of its
assertions are more involved than simple comparisons. So I guessed that
perhaps the reason that NDEBUG was added there when the code was
imported is that there is a performance difference there. But that's
just a guess. It may or may not be borne out by measurements.

I'd be surprised if any of the assertions in the rest of git have a
noticeable impact (and I could not detect one by running t/perf).

> > 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.
> 
> You have stated that you believe the current "assert" calls in Git
> (excluding nedmalloc) should not magically disappear when NDEBUG is defined.

Well, no, I mostly just said that I do not think there is any point in
defining NDEBUG in the first place, as there is little or no benefit to
removing those asserts from the built product.

> So precluding a more labor intensive approach where all currently existing
> "assert(...)" calls are replaced with an "if (!...) die(...)" combination,
> providing a "verify" macro is a quick way to make that happen.  Consider
> this, was the value that Jonathan provided for the "die" string immediately
> obvious to you?  It sure wasn't to me.  That means that whoever does the
> "assert(...)" -> "if(!...)die" swap out may need to be intimately familiar
> with that particular piece of code or the result will be no better than
> using a "verify" macro.

Sure, if you want to mass-convert them, doing so with a macro similar to
assert is the simplest way. I don't think we are in a huge hurry to do
that conversion though. I'm not complaining that NDEBUG works as
advertised by disabling asserts. I'm just claiming that it's largely
pointless in our code base, and I'd consider die("BUG") to be our
"usual" style. If any change is to be made, it would be to suggest
people prefer die("BUG") as a style guideline for future patches. But I
haven't seen anybody agree (or disagree) with the notion, so I'd
hesitate to impose my style suggestion without more discussion in that
area.

If we _did_ accept that as a style guideline, then we could move over
existing assert() calls over time (either as janitorial projects, or as
people touch the related code). But there's not a pressing need to do it
quickly.

-Peff

Re: [PATCH] mailinfo.c: move side-effects outside of assert

[Kyle J. McKay]

On Dec 21, 2016, at 07:55, Jeff King wrote:


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?


No, I don't think so.  NDEBUG is very clearly specified in POSIX [1].

If NDEBUG is defined then "assert(...)" disappears (and in a nice way  
so as not to precipitate "unused variable" warnings).  "N" being "No"  
or "Not" or "Negated" or "bar over the top" + "DEBUG" meaning Not  
DEBUG.  So the code that goes away when NDEBUG is defined is clearly  
debug code.


Considering the wide deployment and use of Git at this point I think  
rather the opposite to be true that "Git does Not require DEBUGging  
code to be enabled for everyday use."  The alternative that it does  
suggests it's not ready for prime time and quite clearly that's not  
the case.


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?


You have suggested there is and that Git is enabling NDEBUG for  
exactly that reason -- to increase performance:



On Dec 20, 2016, at 08:45, Jeff King wrote:

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



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.


You have stated that you believe the current "assert" calls in Git  
(excluding nedmalloc) should not magically disappear when NDEBUG is  
defined.  So precluding a more labor intensive approach where all  
currently existing "assert(...)" calls are replaced with an "if (!...)  
die(...)" combination, providing a "verify" macro is a quick way to  
make that happen.  Consider this, was the value that Jonathan provided  
for the "die" string immediately obvious to you?  It sure wasn't to  
me.  That means that whoever does the "assert(...)" -> "if(!...)die"  
swap out may need to be intimately familiar with that particular piece  
of code or the result will be no better than using a "verify" macro.


I'm just trying to find a quick and easy way to accommodate your  
wishes without redefining the semantics of NDEBUG. ;)


--Kyle

[1] http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/assert.h.html

Re: References to old messages

[Eric Wong]

Stephen & Linda Smith  wrote:
> I had been told to reference (i.e. footnote) a gmane URL.  With that service 
> no longer being being online, what is the preferred method footnoting?

What the others said about Message-IDs and public-inbox :)

If you only have old URLs pointing to gmane and no NNTP access,
you can also search for "gmane:$ARTICLE_NUMBER" via public-inbox
(at least up to messages around this summer):

https://public-inbox.org/git/?q=gmane:12345

https://public-inbox.org/git/_/text/help documents other
search prefixes you can use, too.

Re: config for `format-patch --notes`?

[Johan Herland]

On Wed, Dec 21, 2016 at 1:18 AM, Norbert Kiesel  wrote:
> 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 e.g. 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?

I assume you mean _config_ option here (format-patch has had a --notes
command-line option since v1.8.1). AFAICS, there's no config option
that corresponds to the format-patch --notes command-line option.

You can easily alias or script your way around this, e.g.:

  git config alias.fp 'format-patch --notes'

This creates a 'git fp' command that does what you want, I believe.

Alternatively, if you need more control/automation of the resulting
patches, you can write a script to edit the output files from
format-patch. Currently, I don't believe there is any format-patch
hook available to automatically trigger such a script, but, again,
that can be achieved with an alias.

If you believe a config option would be a useful addition for more
users than yourself, I am sure the community welcomes patches adding
a format.notes config option.

> 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.

>From git-config(1):

branch..description
Branch description, can be edited with git branch
--edit-description. Branch description is automatically added
in the format-patch cover letter or request-pull summary.


...Johan

-- 
Johan Herland, 
www.herland.net

What's cooking in git.git (Dec 2016, #06; Wed, 21)

[Junio C Hamano]

Here are the topics that have been cooking.  Commits prefixed with
'-' are only in 'pu' (proposed updates) while commits prefixed with
'+' are in 'next'.  The ones marked with '.' do not appear in any of
the integration branches, but I am still holding onto them.

You can find the changes described here in the integration branches
of the repositories listed at

http://git-blame.blogspot.com/p/git-public-repositories.html

--
[Graduated to "master"]

* jk/index-pack-wo-repo-from-stdin (2016-12-16) 4 commits
  (merged to 'next' on 2016-12-19 at 9a88221347)
 + index-pack: skip collision check when not in repository
 + t: use nongit() function where applicable
 + index-pack: complain when --stdin is used outside of a repo
 + t5000: extract nongit function to test-lib-functions.sh

 "git index-pack --stdin" needs an access to an existing repository,
 but "git index-pack file.pack" to generate an .idx file that
 corresponds to a packfile does not.


* jk/parseopt-usage-msg-opt (2016-12-14) 1 commit
  (merged to 'next' on 2016-12-19 at c488c7c6e1)
 + parse-options: print "fatal:" before usage_msg_opt()

 The function usage_msg_opt() has been updated to say "fatal:"
 before the custom message programs give, when they want to die
 with a message about wrong command line options followed by the
 standard usage string.


* jk/quote-env-path-list-component (2016-12-21) 5 commits
  (merged to 'next' on 2016-12-21 at 729971d31e)
 + t5615-alternate-env: double-quotes in file names do not work on Windows
  (merged to 'next' on 2016-12-16 at d2cd6008b9)
 + t5547-push-quarantine: run the path separator test on Windows, too
 + tmp-objdir: quote paths we add to alternates
 + alternates: accept double-quoted paths
 + Merge branch 'jk/alt-odb-cleanup' into jk/quote-env-path-list-component

 A recent update to receive-pack to make it easier to drop garbage
 objects made it clear that GIT_ALTERNATE_OBJECT_DIRECTORIES cannot
 have a pathname with a colon in it (no surprise!), and this in turn
 made it impossible to push into a repository at such a path.  This
 has been fixed by introducing a quoting mechanism used when
 appending such a path to the colon-separated list.


* jt/mailinfo-fold-in-body-headers (2016-12-20) 1 commit
  (merged to 'next' on 2016-12-20 at ec9ae616aa)
 + mailinfo.c: move side-effects outside of assert

 Fix for NDEBUG builds.


* nd/shallow-fixup (2016-12-07) 6 commits
  (merged to 'next' on 2016-12-13 at 1a3edb8bce)
 + shallow.c: remove useless code
 + shallow.c: bit manipulation tweaks
 + shallow.c: avoid theoretical pointer wrap-around
 + shallow.c: make paint_alloc slightly more robust
 + shallow.c: stop abusing COMMIT_SLAB_SIZE for paint_info's memory pools
 + shallow.c: rename fields in paint_info to better express their purposes

 Code cleanup in shallow boundary computation.


* sb/sequencer-abort-safety (2016-12-14) 6 commits
  (merged to 'next' on 2016-12-16 at ec71fb1217)
 + Revert "sequencer: remove useless get_dir() function"
  (merged to 'next' on 2016-12-13 at 6107e43d65)
 + sequencer: remove useless get_dir() function
 + sequencer: make sequencer abort safer
 + t3510: test that cherry-pick --abort does not unsafely change HEAD
 + am: change safe_to_abort()'s not rewinding error into a warning
 + am: fix filename in safe_to_abort() error message

 Unlike "git am --abort", "git cherry-pick --abort" moved HEAD back
 to where cherry-pick started while picking multiple changes, when
 the cherry-pick stopped to ask for help from the user, and the user
 did "git reset --hard" to a different commit in order to re-attempt
 the operation.


* vs/submodule-clone-nested-submodules-alternates (2016-12-12) 1 commit
  (merged to 'next' on 2016-12-13 at 8a317ab745)
 + submodule--helper: set alternateLocation for cloned submodules

 "git clone --reference $there --recurse-submodules $super" has been
 taught to guess repositories usable as references for submodules of
 $super that are embedded in $there while making a clone of the
 superproject borrow objects from $there; extend the mechanism to
 also allow submodules of these submodules to borrow repositories
 embedded in these clones of the submodules embedded in the clone of
 the superproject.

--
[New Topics]

* bw/push-submodule-only (2016-12-20) 3 commits
 - push: add option to push only submodules
 - submodules: add RECURSE_SUBMODULES_ONLY value
 - transport: reformat flag #defines to be more readable

 "git submodule push" learned "--recurse-submodules=only option to
 push submodules out without pushing the top-level superproject.


* mk/mingw-winansi-ttyname-termination-fix (2016-12-20) 1 commit
  (merged to 'next' on 2016-12-21 at 1e8e994605)
 + mingw: consider that UNICODE_STRING::Length counts bytes

 A potential but unlikely buffer overflow in Windows port has been
 fixed.

 Will merge to 'master'.


* nd/config-misc-fixes (2016-12-20) 4 commits
 

Re: References to old messages

[Junio C Hamano]

Stephen & Linda Smith  writes:

> I want to pick up work  on a patch that I was working on previously.  
>
> I had been told to reference (i.e. footnote) a gmane URL.  With that service 
> no longer being being online, what is the preferred method footnoting?
>
> sps

The NNTP interface of GMane is still working, so referring to e.g.
$gmane/286483 _could_ identify a message uniquely, but people who do
not usually talk NNTP to GMane won't be able to find the message
with 286483, so it is not very nice.

The same message is referred to like this around here these days:

https://public-inbox.org/git/1455685597-22445-1-git-send-email-isch...@cox.net/

The point is people know its message id is <14556855...@cox.net> and
refer to other archives with it, even when public-inbox.org is not
available.

Re: References to old messages

[Stefan Beller]

On Wed, Dec 21, 2016 at 3:12 PM, Stephen & Linda Smith  wrote:
> I want to pick up work  on a patch that I was working on previously.
>
> I had been told to reference (i.e. footnote) a gmane URL.  With that service
> no longer being being online, what is the preferred method footnoting?
>
> sps

See https://public-inbox.org/git/

References to old messages

[Stephen & Linda Smith]

I want to pick up work  on a patch that I was working on previously.  

I had been told to reference (i.e. footnote) a gmane URL.  With that service 
no longer being being online, what is the preferred method footnoting?

sps

Re: [PATCH 4/2] t5615-alternate-env: double-quotes in file names do not work on Windows

[Jeff King]

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 
> ---
>  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

Re: [PATCH bw/realpath-wo-chdir] real_path: canonicalize directory separators in root parts

[Brandon Williams]

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 
> ---
>  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

Re: Bug report: Git pull hang occasionally

[Kai Zhang]

I will verify it. Thanks.
> On Dec 21, 2016, at 1:32 PM, Junio C Hamano  wrote:
> 
> Junio C Hamano  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 
> 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 
> Signed-off-by: Junio C Hamano 
> ---
> 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(_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" &&
> + 

[PATCH bw/realpath-wo-chdir] real_path: canonicalize directory separators in root parts

[Johannes Sixt]

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 
---
 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(, path);
+   get_root_part(resolved, );
 
-   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(, 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(, 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(, 0, offset);
+   get_root_part(resolved, );
} else {
/*
 * relative symlink
-- 
2.11.0.79.gf6b77ca

[PATCH 4/2] t5615-alternate-env: double-quotes in file names do not work on Windows

[Johannes Sixt]

Protect a recently added test case with !MINGW.

Signed-off-by: Johannes Sixt 
---
 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

Re: Bug report: Git pull hang occasionally

[Junio C Hamano]

Junio C Hamano  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 
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 
Signed-off-by: Junio C Hamano 
---
 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(_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 

Re: [PATCH 0/2] Really fix the isatty() problem on Windows

[Junio C Hamano]

Johannes Sixt  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.

Re: [PATCH 0/2] Really fix the isatty() problem on Windows

[Johannes Sixt]

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

Re: Allow "git shortlog" to group by committer information

[Johannes Sixt]

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 &&
- 3C O Mitter
+ 2C O Mitter
+ 1Sin Nombre
 EOF
 git shortlog -nsc HEAD >actual &&
 test_cmp expect actual





I confirm that t4201 now passes on Windows with this fixup.

-- Hannes

Re: Bug report: Git pull hang occasionally

[Kai Zhang]

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  wrote:
> 
> Kai Zhang  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.
> 

Re: Races on ref .lock files?

[Junio C Hamano]

Andreas Krey  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-u...@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.

Re: Bug report: Git pull hang occasionally

[Junio C Hamano]

Kai Zhang  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.

Re: [PATCH] string-list: make compare function compatible with qsort(3)

[Junio C Hamano]

Junio C Hamano  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.

Re: Allow "git shortlog" to group by committer information

[Junio C Hamano]

Jeff King  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.

Bug report: Git pull hang occasionally

[Kai Zhang]

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 +] "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 +] "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

Re: [PATCH 00/13] gitk: tweak rendering of remote-tracking references

[Marc Branchaud]

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.

Re: [PATCH] winansi_isatty(): fix when Git is used from CMD

[Junio C Hamano]

Johannes Schindelin  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.

Re: [PATCH 2/2] mingw: replace isatty() hack

[Junio C Hamano]

Johannes Schindelin  writes:

> From: Jeff Hostetler 
>
> 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_MSYS0x4
>  
>  /*
>   ANSI codes used by git: m, K
> @@ -105,6 +108,9 @@ static int is_console(int fd)
>   } else if (!GetConsoleScreenBufferInfo(hcon, ))
>   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.

Re: [PATCH] string-list: make compare function compatible with qsort(3)

[Junio C Hamano]

René Scharfe  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.

Missing option for format-patch?

[Norbert Kiesel]

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.



P.S.: Today I'm sad and proud to say "Ich bin ein Berliner!" --nk

Re: [PATCH] winansi_isatty(): fix when Git is used from CMD

[Johannes Schindelin]

Hi Junio,

On Tue, 20 Dec 2016, Junio C Hamano wrote:

> Johannes Schindelin  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

Re: [PATCH] winansi_isatty(): fix when Git is used from CMD

[Johannes Schindelin]

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

Re: [PATCH 1/1] mingw: intercept isatty() to handle /dev/null as Git expects it

[Johannes Schindelin]

Hi,

On Fri, 16 Dec 2016, Junio C Hamano wrote:

> Johannes Sixt  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

[PATCH 1/2] mingw: adjust is_console() to work with stdin

[Johannes Schindelin]

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 
---
 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, ))
+   if (!fd) {
+   if (!GetConsoleMode(hcon, ))
+   return 0;
+   } else if (!GetConsoleScreenBufferInfo(hcon, ))
return 0;
 
/* initialize attributes */
-- 
2.11.0.rc3.windows.1

[PATCH 2/2] mingw: replace isatty() hack

[Johannes Schindelin]

From: Jeff Hostetler 

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 
Signed-off-by: Johannes Schindelin 
---
 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 
 #include 
+#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_MSYS0x4
 
 /*
  ANSI codes used by git: m, K
@@ -105,6 +108,9 @@ static int is_console(int fd)
} else if (!GetConsoleScreenBufferInfo(hcon, ))
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
+* 

[PATCH 0/2] Really fix the isatty() problem on Windows

[Johannes Schindelin]

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

Re: [PATCH] string-list: make compare function compatible with qsort(3)

[Jeff King]

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.fa3zpapbjngjc...@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

Re: Allow "git shortlog" to group by committer information

[Jeff King]

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

Re: [PATCH] mailinfo.c: move side-effects outside of assert

[Jeff King]

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

Re: Races on ref .lock files?

[Andreas Krey]

On Fri, 16 Dec 2016 09:20:07 +, 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-u...@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 
Date: Fri, 22 Jan 2010 07:29:21 -0800

[PATCH] string-list: make compare function compatible with qsort(3)

[René Scharfe]

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 
---
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 

Re: [PATCH 1/3] compat: add qsort_s()

[René Scharfe]

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é

Re: Races on ref .lock files?

[Andreas Krey]

On Fri, 16 Dec 2016 15:34:22 +, 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 
Date: Fri, 22 Jan 2010 07:29:21 -0800