Git development
 help / color / mirror / Atom feed
* Re: [PATCH 27/36] attr: convert to new threadsafe API
From: Jeff King @ 2016-10-26 20:20 UTC (permalink / raw)
  To: Stefan Beller
  Cc: Simon Ruderich, Johannes Schindelin, Junio C Hamano,
	git@vger.kernel.org, Brandon Williams, Duy Nguyen
In-Reply-To: <CAGZ79kYgk9rQDju0MT2uniaxhAWpzJ9f1T9czgNnxfq+Wz6m+A@mail.gmail.com>

On Wed, Oct 26, 2016 at 12:51:02PM -0700, Stefan Beller wrote:

> > I seem to recall this does not work on Windows, where the pthread
> > functions are thin wrappers over CRITICAL_SECTION. Other threaded code
> > in git does an explicit setup step before entering threaded sections.
> > E.g., see start_threads() in builtin/grep.c.
> >
> 
> I wonder if we can have a similar thing as
> http://stackoverflow.com/a/9490113 in compat/win32/pthread.{h.c} as it is
> very convenient to not have to explicitly initialize mutexes?

I agree it would be much more convenient and get rid of some repetitive
boilerplate code. I'll leave it to Windows folks to decide if they are
OK with that approach or not (I do not offhand know of any reason it
would not work).

-Peff

^ permalink raw reply

* Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC
From: Jeff King @ 2016-10-26 20:17 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Lars Schneider, Eric Wong, Johannes Schindelin
In-Reply-To: <xmqqpomnatg6.fsf@gitster.mtv.corp.google.com>

On Wed, Oct 26, 2016 at 10:52:41AM -0700, Junio C Hamano wrote:

> > I actually wonder if it is worth carrying around the O_NOATIME hack at
> > all.
> 
> Yes, I share the thought.  We no longer have too many loose objects
> to matter.
> 
> I do not mind flipping the order, but I'd prefer to cook the result
> even longer.  I am tempted to suggest we take two step route:
> 
>  - ship 2.11 with the "atime has been there and we won't regress it"
>    shape, while cooking the "cloexec is semantically more
>    important" version in 'next' during the feature freeze
> 
>  - immediately after 2.11 merge it to 'master' for 2.12 to make sure
>    there is no fallout.

That sounds reasonable, though I'd consider jumping straight to "NOATIME
is not worth it; drop it" as the patch for post-2.11.

-Peff

^ permalink raw reply

* Re: [PATCH 0/2] git-svn: implement "git worktree" awareness
From: Eric Wong @ 2016-10-26 20:02 UTC (permalink / raw)
  To: Jakub Narębski
  Cc: git, Mathieu Arnold, Nguyễn Thái Ngọc Duy,
	Stefan Beller, Junio C Hamano
In-Reply-To: <20161014014623.15223-1-e@80x24.org>

Eric Wong <e@80x24.org> wrote:
> +Cc Jakub since gitweb could probably take advantage of get_record
> from the first patch, too.  I'm not completely sure about the API
> for this, though.

Jakub: ping?

+Cc: Junio, too.  I'm hoping to have this in 2.11.

> The following changes since commit 3cdd5d19178a54d2e51b5098d43b57571241d0ab:
> 
>   Sync with maint (2016-10-11 14:55:48 -0700)
> 
> are available in the git repository at:
> 
>   git://bogomips.org/git-svn.git svn-wt
> 
> for you to fetch changes up to 112423eb905cf28c9445781a7647ba590d597ab3:
> 
>   git-svn: "git worktree" awareness (2016-10-14 01:36:12 +0000)
> 
> ----------------------------------------------------------------
> Eric Wong (2):
>       git-svn: reduce scope of input record separator change
>       git-svn: "git worktree" awareness
> 
>  git-svn.perl              | 13 +++++++------
>  perl/Git.pm               | 16 +++++++++++++++-
>  perl/Git/SVN.pm           | 24 +++++++++++++++---------
>  perl/Git/SVN/Editor.pm    | 12 +++++-------
>  perl/Git/SVN/Fetcher.pm   | 15 +++++----------
>  perl/Git/SVN/Migration.pm | 37 ++++++++++++++++++++++---------------
>  6 files changed, 69 insertions(+), 48 deletions(-)

^ permalink raw reply

* Re: [PATCH 27/36] attr: convert to new threadsafe API
From: Stefan Beller @ 2016-10-26 19:51 UTC (permalink / raw)
  To: Jeff King
  Cc: Simon Ruderich, Johannes Schindelin, Junio C Hamano,
	git@vger.kernel.org, Brandon Williams, Duy Nguyen
In-Reply-To: <20161026121525.twgiavpgfbr2ahvn@sigill.intra.peff.net>

On Wed, Oct 26, 2016 at 5:15 AM, Jeff King <peff@peff.net> wrote:
> On Wed, Oct 26, 2016 at 11:35:58AM +0200, Simon Ruderich wrote:
>
>> >  static pthread_mutex_t attr_mutex;
>> > -#define attr_lock()pthread_mutex_lock(&attr_mutex)
>> > +static inline void attr_lock(void)
>> > +{
>> > +   static int initialized;
>> > +
>> > +   if (!initialized) {
>> > +           pthread_mutex_init(&attr_mutex, NULL);
>> > +           initialized = 1;
>> > +   }
>> > +   pthread_mutex_lock(&attr_mutex);
>> > +}
>>
>> This may initialize the mutex multiple times during the first
>> lock (which may happen in parallel).
>>
>> pthread provides static initializers. To quote the man page:
>>
>>     Variables of type pthread_mutex_t can also be initialized
>>     statically, using the constants PTHREAD_MUTEX_INITIALIZER
>>     (for fast mutexes), PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP
>>     (for recursive mutexes), and
>>     PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP (for error checking
>>     mutexes).
>
> I seem to recall this does not work on Windows, where the pthread
> functions are thin wrappers over CRITICAL_SECTION. Other threaded code
> in git does an explicit setup step before entering threaded sections.
> E.g., see start_threads() in builtin/grep.c.
>

I wonder if we can have a similar thing as
http://stackoverflow.com/a/9490113 in compat/win32/pthread.{h.c} as it is
very convenient to not have to explicitly initialize mutexes?

^ permalink raw reply

* Re: A bug with "git svn show-externals"
From: Eric Wong @ 2016-10-26 19:10 UTC (permalink / raw)
  To: Tao Peng; +Cc: git, Vineet Kumar
In-Reply-To: <B8D25850-A4DE-435C-A856-240D612D59DA@me.com>

Tao Peng <pengtao@me.com> wrote:
> Hi there,
> 
> I met a bug of the "git svn show-externals” command.  If a subdirectory item has a svn:externals property, and the format of the property is “URL first, then the local path”, running "git svn show-externals” command at the root level will result in an unusable output.
> 
> Example:
> $ svn pg svn:externals svn+ssh://src.foo.com/svn/ref/English.lproj/
> svn+ssh://src.foo.com/svn/orig/trunk/Resources/English.lproj/Localizable.strings Localizable.strings

+Cc Vineet who originally implemented this 9 years ago

I've never used externals much, but I guess it's common for
externals to be a full URL and not merely a relative path
to somewhere within the same SVN repo.

> $ git svn show-externals
> # /English.lproj/
> /English.lproj/svn+ssh://src.foo.com/svn/orig/trunk/Resources/English.lproj/Localizable.strings Localizable.strings

> 
> This bug is preventing my script from correctly finishing the svn-to-git repo migration work. Does anyone know a workaround to this bug?

Can you try the following change to ignore path prefixing for
full URLs?

diff --git a/git-svn.perl b/git-svn.perl
index 4d41d22..ced665a 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -1303,7 +1303,9 @@ sub cmd_show_externals {
 		my $s = $props->{'svn:externals'} or return;
 		$s =~ s/[\r\n]+/\n/g;
 		chomp $s;
-		$s =~ s#^#$path#gm;
+		if ($s !~ m#^[a-z\+]+://#i) {
+			$s =~ s#^#$path#gm;
+		}
 		print STDOUT "$s\n";
 	});
 }

^ permalink raw reply related

* Re: [PATCH] Documentation/git-diff: document git diff with 3+ commits
From: Junio C Hamano @ 2016-10-26 18:11 UTC (permalink / raw)
  To: Michael J Gruber; +Cc: git, Jacob Keller
In-Reply-To: <07712c2c94670ca4d91ef78cd7d3602a8d36b0c1.1477472970.git.git@drmicha.warpmail.net>

Michael J Gruber <git@drmicha.warpmail.net> writes:

> That one is difficult to discover but super useful, so document it:
> Specifying 3 or more commits makes git diff switch to combined diff.
>
> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
> ---
>
> Notes:
>     Note that we have the following now:
> ...
>     'git diff A..B' equivalent to 'git diff A B'
>     in contrast to 'git log A..B' listing commits between M and B only
>     (without the commits between M and A unless they are "in" B).

The standard answer is: 

    Do not use two-dot form with 'git diff', if you find it
    confusing.  Diff is about two endpoints, not about a range
    between two.

The reason why we do not reject can be easily guessed by any
intelligent person when some historical background is given, I
think.

 - In the beginning A...B did not exist.  A..B was the only "range"
   notation.

 - "git log A..B" was in wide use.  Remember, "git log A...B" did
   not exist.

 - People started mistyping "git diff A..B", which looked as if the
   user typed "git diff ^A B" to the internal.

 - Git _could_ have rejected that as a bogus request to diff two
   points, ^A (what is that???) and B, but "What else could the user
   have meant with 'git diff A..B' other than 'git diff A B'?" was
   an argument to favor doing _something_ useful rather than
   erroring out.  Remember, "A...B" did not exist when this
   happened.

So there.  We may want to deprecate "diff A..B" but I personally do
not think it is worth the effort.  There is no substitute for the
current "diff A...B" to allow us to deprecate both at the same time,
which is the required first step if we want to eventually swap their
meaning in a far future version of Git which would have been better
if we had hindsight.  But remember, "A...B" did not exist back then. 

> diff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt
> index bbab35fcaf..2047318a27 100644
> --- a/Documentation/git-diff.txt
> +++ b/Documentation/git-diff.txt
> @@ -12,6 +12,7 @@ SYNOPSIS
>  'git diff' [options] [<commit>] [--] [<path>...]
>  'git diff' [options] --cached [<commit>] [--] [<path>...]
>  'git diff' [options] <commit> <commit> [--] [<path>...]
> +'git diff' [options] <commit> <commit> <commit> [<commit>...]

Made me wonder "is [<A>...] 0-or-more As or 1-or-more As?".
Don't we allow pathspecs in this case?

>  'git diff' [options] <blob> <blob>
>  'git diff' [options] [--no-index] [--] <path> <path>
>  
> @@ -75,9 +76,16 @@ two blob objects, or changes between two files on disk.
>  	"git diff $(git-merge-base A B) B".  You can omit any one
>  	of <commit>, which has the same effect as using HEAD instead.
>  
> +'git diff' [options] <commit> <commit> <commit> [<commit>...]::
> +
> +	This is to view a combined diff between the first <commit>
> +	and the remaining ones, just like viewing a combined diff
> +	for a merge commit (see below) where the first <commit>
> +	is the merge commit and the remaining ones are the parents.
> +
>  Just in case if you are doing something exotic, it should be
>  noted that all of the <commit> in the above description, except
> -in the last two forms that use ".." notations, can be any
> +in the two forms that use ".." notations, can be any
>  <tree>.
>  
>  For a more complete list of ways to spell <commit>, see

^ permalink raw reply

* Re: [PATCH] hex: use unsigned index for ring buffer
From: Junio C Hamano @ 2016-10-26 17:53 UTC (permalink / raw)
  To: René Scharfe; +Cc: Jeff King, Git List
In-Reply-To: <3c95a89a-2a9b-2856-42a1-6b994f2e31cd@web.de>

René Scharfe <l.s.r@web.de> writes:

> Actually I didn't sign-off on purpose originally.  But OK, let's keep
> the version below.  I just feel strangely sad seeing that concise magic
> go.  Nevermind.

I actually share the sadness, too, but let's be stupid and obvious
here.

Thanks.

>
> Signed-off-by: Rene Scharfe <l.s.r@web.de>
>
>> -- >8 --
>> From: René Scharfe <l.s.r@web.de>
>> Date: Sun, 23 Oct 2016 19:57:30 +0200
>> Subject: [PATCH] hex: make wraparound of the index into ring-buffer explicit
>>
>> Overflow is defined for unsigned integers, but not for signed ones.
>>
>> We could make the ring-buffer index in sha1_to_hex() and
>> get_pathname() unsigned to be on the safe side to resolve this, but
>> let's make it explicit that we are wrapping around at whatever the
>> number of elements the ring-buffer has.  The compiler is smart enough
>> to turn modulus into bitmask for these codepaths that use
>> ring-buffers of a size that is a power of 2.
>>
>> Signed-off-by: René Scharfe <l.s.r@web.de>
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>> ---
>>  hex.c  | 3 ++-
>>  path.c | 3 ++-
>>  2 files changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/hex.c b/hex.c
>> index ab2610e498..845b01a874 100644
>> --- a/hex.c
>> +++ b/hex.c
>> @@ -78,7 +78,8 @@ char *sha1_to_hex(const unsigned char *sha1)
>>  {
>>  	static int bufno;
>>  	static char hexbuffer[4][GIT_SHA1_HEXSZ + 1];
>> -	return sha1_to_hex_r(hexbuffer[3 & ++bufno], sha1);
>> +	bufno = (bufno + 1) % ARRAY_SIZE(hexbuffer);
>> +	return sha1_to_hex_r(hexbuffer[bufno], sha1);
>>  }
>>
>>  char *oid_to_hex(const struct object_id *oid)
>> diff --git a/path.c b/path.c
>> index fe3c4d96c6..9bfaeda207 100644
>> --- a/path.c
>> +++ b/path.c
>> @@ -24,7 +24,8 @@ static struct strbuf *get_pathname(void)
>>  		STRBUF_INIT, STRBUF_INIT, STRBUF_INIT, STRBUF_INIT
>>  	};
>>  	static int index;
>> -	struct strbuf *sb = &pathname_array[3 & ++index];
>> +	struct strbuf *sb = &pathname_array[index];
>> +	index = (index + 1) % ARRAY_SIZE(pathname_array);
>>  	strbuf_reset(sb);
>>  	return sb;
>>  }
>>

^ permalink raw reply

* Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC
From: Junio C Hamano @ 2016-10-26 17:52 UTC (permalink / raw)
  To: Jeff King; +Cc: git, Lars Schneider, Eric Wong, Johannes Schindelin
In-Reply-To: <20161026164746.2fu57f4pji5qdtnh@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

> Of the two flags, I would say CLOEXEC is the more important one to
> respect because it may actually impact correctness (e.g., leaking
> descriptors to sub-processes). Whereas O_NOATIME is purely a performance
> optimization.

I tend to agree.

> I actually wonder if it is worth carrying around the O_NOATIME hack at
> all.

Yes, I share the thought.  We no longer have too many loose objects
to matter.

I do not mind flipping the order, but I'd prefer to cook the result
even longer.  I am tempted to suggest we take two step route:

 - ship 2.11 with the "atime has been there and we won't regress it"
   shape, while cooking the "cloexec is semantically more
   important" version in 'next' during the feature freeze

 - immediately after 2.11 merge it to 'master' for 2.12 to make sure
   there is no fallout.


^ permalink raw reply

* Re: [PATCH] hex: use unsigned index for ring buffer
From: René Scharfe @ 2016-10-26 17:08 UTC (permalink / raw)
  To: Junio C Hamano, Jeff King; +Cc: Git List
In-Reply-To: <xmqqd1ios2p3.fsf@gitster.mtv.corp.google.com>

Am 25.10.2016 um 20:28 schrieb Junio C Hamano:
> Jeff King <peff@peff.net> writes:
>
>> On Mon, Oct 24, 2016 at 04:53:50PM -0700, Junio C Hamano wrote:
>>
>>>> So how about this?  It gets rid of magic number 3 and works for array
>>>> size that's not a power of two.  And as a nice side effect it can't
>>>> trigger a signed overflow anymore.
>>>
>>> Looks good to me.  Peff?
>>
>> Any of the variants discussed in this thread is fine by me.
>
> OK, here is what I'll queue then.
> I assumed that René wants to sign it off ;-).

Actually I didn't sign-off on purpose originally.  But OK, let's keep
the version below.  I just feel strangely sad seeing that concise magic
go.  Nevermind.

Signed-off-by: Rene Scharfe <l.s.r@web.de>

> -- >8 --
> From: René Scharfe <l.s.r@web.de>
> Date: Sun, 23 Oct 2016 19:57:30 +0200
> Subject: [PATCH] hex: make wraparound of the index into ring-buffer explicit
>
> Overflow is defined for unsigned integers, but not for signed ones.
>
> We could make the ring-buffer index in sha1_to_hex() and
> get_pathname() unsigned to be on the safe side to resolve this, but
> let's make it explicit that we are wrapping around at whatever the
> number of elements the ring-buffer has.  The compiler is smart enough
> to turn modulus into bitmask for these codepaths that use
> ring-buffers of a size that is a power of 2.
>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  hex.c  | 3 ++-
>  path.c | 3 ++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/hex.c b/hex.c
> index ab2610e498..845b01a874 100644
> --- a/hex.c
> +++ b/hex.c
> @@ -78,7 +78,8 @@ char *sha1_to_hex(const unsigned char *sha1)
>  {
>  	static int bufno;
>  	static char hexbuffer[4][GIT_SHA1_HEXSZ + 1];
> -	return sha1_to_hex_r(hexbuffer[3 & ++bufno], sha1);
> +	bufno = (bufno + 1) % ARRAY_SIZE(hexbuffer);
> +	return sha1_to_hex_r(hexbuffer[bufno], sha1);
>  }
>
>  char *oid_to_hex(const struct object_id *oid)
> diff --git a/path.c b/path.c
> index fe3c4d96c6..9bfaeda207 100644
> --- a/path.c
> +++ b/path.c
> @@ -24,7 +24,8 @@ static struct strbuf *get_pathname(void)
>  		STRBUF_INIT, STRBUF_INIT, STRBUF_INIT, STRBUF_INIT
>  	};
>  	static int index;
> -	struct strbuf *sb = &pathname_array[3 & ++index];
> +	struct strbuf *sb = &pathname_array[index];
> +	index = (index + 1) % ARRAY_SIZE(pathname_array);
>  	strbuf_reset(sb);
>  	return sb;
>  }
>

^ permalink raw reply

* Re: [PATCH] reset: --unmerge
From: Junio C Hamano @ 2016-10-26 17:06 UTC (permalink / raw)
  To: Duy Nguyen; +Cc: Git Mailing List
In-Reply-To: <CACsJy8Dn7m2axEFkkQtnZMs2yzFwivAJyZCWxODg-HQ=qLnVMA@mail.gmail.com>

Duy Nguyen <pclouds@gmail.com> writes:

> Interestingly the thread/bug that resulted in that commit started with
> "report this bug to git" [2]. Something about git-stash. I quote the
> original mail here in case anyone wants to look into it (not sure if
> it's actually reported here before, I don't pay much attention to
> git-stash mails)
>
> -- 8< --
> Bad news, everyone!
>
> When a stash contains changes for several files, and "stash pop"
> encounters conflicts only in some of them, the rest of the files are
> stages automatically.

It indeed is curious.

That is the designed behaviour for _ANY_ mergy operation, and not
limited to "stash pop".

A clean application is added to the index so that you can find out
about them from "diff --cached", while conflicted ones keep their
unmerged stages so that the conflict can be resolved in the working
tree files.  There is no bad news here.

Once you resolve the conflict, you would add the final contents to
the working tree, but as anybody who knows how "git diff" after
resolving conflicts in the working tree files is useful would know,
"saving the editor buffer after removing conflict markers" is not a
valid signal that the user is confident that the contents is final.

> At least, that happens with Git 2.1.0 on my machine, and some
> commenters here: http://stackoverflow.com/a/1237337/615245
>
> So then when we unstage the files which had conflicts after resolving
> those, the result is mixed. Which doesn't look right.

Whoever wrote this does not understand how mergy operations in Git
works, I guess.

> What shall we do? Unstage the automatically-staged files? Revert the
> changes from this bug? It seems Git really wants the changes staged
> after the conflict resolution.

The first order of business is to learn how mergy operations in Git
is designed to work, and if they are in the business of building a
tool around Git to make the life of users better, avoid going against
the designed workflow.

If this "Bad news, everyone!" is why vc-git-resolve-conflicts was
added and defaults to true, I can feel safe in toggling it off
forever in my ~/.emacs, knowing that it is a totally broken option
that came from a desire to fix a problem that does not exist.


^ permalink raw reply

* Re: [PATCH] rebase: add --forget to cleanup rebase, leave HEAD untouched
From: Junio C Hamano @ 2016-10-26 16:51 UTC (permalink / raw)
  To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <20161026094658.20704-1-pclouds@gmail.com>

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> There are occasions when you decide to abort an in-progress rebase and
> move on to do something else but you forget to do "git rebase --abort"
> first. Or the rebase has been in progress for so long you forgot about
> it. By the time you realize that (e.g. by starting another rebase)
> it's already too late to retrace your steps. The solution is normally
>
>     rm -r .git/<some rebase dir>
>
> and continue with your life. But there could be two different
> directories for <some rebase dir> (and it obviously requires some
> knowledge of how rebase works), and the ".git" part could be much
> longer if you are not at top-dir, or in a linked worktree. And
> "rm -r" is very dangerous to do in .git, a mistake in there could
> destroy object database or other important data.
>
> Provide "git rebase --forget" for this exact use case.

Two and a half comments.

 - The title says "leave HEAD untouched".  Are my working tree files
   and my index also safe from this operation, or is HEAD the only
   thing that is protected?

 - I think I saw a variant of this gotcha for an unconcluded
   cherry-pick that was left behind, which the bash-prompt script
   did not notice but the next "git cherry-pick" did by complaining
   "you are in the middle" or something like that.  Perhaps we would
   want to have a similarly sounding option to help that case, too,
   not in this patch but as another patch on the same theme?

 - Would it have helped if bash-prompt were in use?  I am not saying
   that this patch becomes unnecessary if you use it; I am trying to
   see if it helps its users by reminding them what state they are
   in.


^ permalink raw reply

* Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC
From: Jeff King @ 2016-10-26 16:47 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Lars Schneider, Eric Wong, Johannes Schindelin
In-Reply-To: <xmqqa8drcc5i.fsf@gitster.mtv.corp.google.com>

On Wed, Oct 26, 2016 at 09:23:21AM -0700, Junio C Hamano wrote:

> >> +		/* Might the failure be due to O_NOATIME? */
> >> +		if (errno != ENOENT && (sha1_file_open_flag & O_NOATIME)) {
> >> +			sha1_file_open_flag &= ~O_NOATIME;
> >> +			continue;
> >> +		}
> >
> > We drop O_NOATIME, and end up with an empty flag field.
> >
> > But we will never have tried just O_CLOEXEC, which might have worked.
> 
> Yes, doing so would smudge atime, so one question is which one
> between noatime or cloexec is more important to be done at open(2)
> time.

Yes, but the missing case is one where we know that O_NOATIME does not
work (but O_CLOEXEC does), so we know we have to smudge the atime.

Of the two flags, I would say CLOEXEC is the more important one to
respect because it may actually impact correctness (e.g., leaking
descriptors to sub-processes). Whereas O_NOATIME is purely a performance
optimization.

I actually wonder if it is worth carrying around the O_NOATIME hack at
all.  Linus added it on 2005-04-23 via 144bde78e9; the aim was to reduce
the cost of opening loose object files. Some things have changed since
then:

  1. In June 2005, git learned about packfiles, which means we would do
     a lot fewer atime updates (rather than one per object access, we'd
     generally get one per packfile).

  2. In late 2006, Linux learned about "relatime", which is generally
     the default on modern installs. So performance around atime updates
     is a non-issue there these days.

     All the world isn't Linux, of course, but I can't help that feel
     that atime performance hackery is something that belongs at the
     system level, not in individual applications.

So I don't have hard numbers, but I'd be surprised if O_NOATIME is
really buying us anything these days.

-Peff

^ permalink raw reply

* Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC
From: Junio C Hamano @ 2016-10-26 16:23 UTC (permalink / raw)
  To: Jeff King; +Cc: git, Lars Schneider, Eric Wong, Johannes Schindelin
In-Reply-To: <20161026042555.neaxvnmggtcku5cc@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

>> +		/* Try again w/o O_CLOEXEC: the kernel might not support it */
>> +		if ((sha1_file_open_flag & O_CLOEXEC) && errno == EINVAL) {
>> +			sha1_file_open_flag &= ~O_CLOEXEC;
>>  			continue;
>>  		}
>
> So if we start with O_CLOEXEC|O_NOATIME, we drop CLOEXEC here and try
> again with just O_NOATIME. And then if _that_ fails...
>
>> +		/* Might the failure be due to O_NOATIME? */
>> +		if (errno != ENOENT && (sha1_file_open_flag & O_NOATIME)) {
>> +			sha1_file_open_flag &= ~O_NOATIME;
>> +			continue;
>> +		}
>
> We drop O_NOATIME, and end up with an empty flag field.
>
> But we will never have tried just O_CLOEXEC, which might have worked.

Yes, doing so would smudge atime, so one question is which one
between noatime or cloexec is more important to be done at open(2)
time.

It may be possible to open(2) only with cloexec and then fcntl(2)
FD_SET noatime immediately after, but going that way would explode
the combination even more, as it may not be possible to set these
two flags the other way around.

> I'm not sure it's worth worrying about or not; I don't know which
> systems are actually lacking either of the flags, or if they tend to
> have both.


^ permalink raw reply

* Re: [PATCH v1 00/19] Add configuration options for split-index
From: Junio C Hamano @ 2016-10-26 16:14 UTC (permalink / raw)
  To: Duy Nguyen
  Cc: Christian Couder, Git Mailing List,
	Ævar Arnfjörð Bjarmason, Christian Couder
In-Reply-To: <CACsJy8Ba0BY=pZwrKf5rcD5AaZ3YyKh9=ENKkj7hHpTqh00OnA@mail.gmail.com>

Duy Nguyen <pclouds@gmail.com> writes:

> On Wed, Oct 26, 2016 at 12:21 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
> Even if we ignore user index files (by forcing them all to be stored
> in one piece), there is a problem with the special temporary file
> index.lock, which must use split-index because it will become the new
> index. Handling race conditions could be tricky with ref counting.
> Timestamps help in this regard.

I actually think using the split-index only for the $GIT_DIR/index,
the primary one, and using the full index for others is a bad idea,
as we use temporary index ourselves when making partial commits,
which happens quite often.

So time-based GC it is.

I actually do not think index.lock is a problem, but is a solution,
if we only limit the split-index to the primary one (we can have at
most one, so we can GC the stale shared ones while holding the
lock).

But that is no longer important.



^ permalink raw reply

* Re: [PATCH/RFC] git.c: support "!!" aliases that do not move cwd
From: Junio C Hamano @ 2016-10-26 16:08 UTC (permalink / raw)
  To: Duy Nguyen
  Cc: Jeff King, Johannes Schindelin, Johannes Sixt, Git Mailing List
In-Reply-To: <CACsJy8DUqrsaqmrCHzzuS3Q7DXRAPkisOJbSmYPX8-AhmNUz6w@mail.gmail.com>

Duy Nguyen <pclouds@gmail.com> writes:

> I don't object the alias.<name>.<property> approach though. It's
> definitely a cleaner one in my opinion. It just needs people who can
> spend time to follow up until the end. But if someone decides to do
> that now, I'll drop the "(properties)!command" and try to support
> him/her.

I don't object to either approach, but what I would love to see
people avoid is to end up with both.

Thanks.

^ permalink raw reply

* Re: [PATCH 27/36] attr: convert to new threadsafe API
From: Duy Nguyen @ 2016-10-26 14:06 UTC (permalink / raw)
  To: Stefan Beller; +Cc: Junio C Hamano, git@vger.kernel.org, Brandon Williams
In-Reply-To: <CAGZ79kb7PaqnyXZ7u0z8Q__ahTLKX8RQwV=dw7vbD4C9LnjOtw@mail.gmail.com>

On Tue, Oct 25, 2016 at 2:18 AM, Stefan Beller <sbeller@google.com> wrote:
> On Mon, Oct 24, 2016 at 11:55 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
>>
>> Make that a double-asterisk.  The same problem appears in an updated
>> example in technical/api-gitattributes.txt doc, but the example in
>> the commit log message (below) is correct.
>
> The implementation is actually using a double pointer, see below,
> I forgot commit message and documentation
>
>>>     GIT_ATTR_RESULT_INIT_FOR(myresult, 1);
>>
>> Are you sure about this?  We've called attr_check_initl() already so
>> if this is declaring myresult, it would be decl-after-stmt.
>
> I forgot to update the commit message and Documentation.
> GIT_ATTR_RESULT_INIT_FOR is gone

I was asking whether this function/macro was not thread-safe and found
out it didn't exist as well, and it's bed time so I'm stopping. Will
continue my skimming on the next re-roll :)
-- 
Duy

^ permalink raw reply

* Re: [PATCH 27/36] attr: convert to new threadsafe API
From: Duy Nguyen @ 2016-10-26 13:52 UTC (permalink / raw)
  To: Stefan Beller; +Cc: Junio C Hamano, Git Mailing List, bmwill
In-Reply-To: <20161022233225.8883-28-sbeller@google.com>

On Sun, Oct 23, 2016 at 6:32 AM, Stefan Beller <sbeller@google.com> wrote:
> This revamps the API of the attr subsystem to be thread safe.
> Before we had the question and its results in one struct type.
> The typical usage of the API was
>
>     static struct git_attr_check *check;
>
>     if (!check)
>         check = git_attr_check_initl("text", NULL);


Two cents. I read the .txt first and my first thought was "is _initl a
typo, shouldn't it be jsut _init"? I know we have this 'l' variant at
least in argv-array, but there we have many ways of adding arguments.
And here it's just "initl", not "init" nor other "initX" variants,
which looks odd. I wonder if the name git_attr_check_init() would do
the job fine, since we don't have different init variants and the
naming convention is not strong enough to tell me "it's multiple
arguments ended with a NULL one" right away. If you're worried about
people forgetting NULL at the end, how about passing an array of
strings, terminated by NULL, instead?

Just thinking out loud. Maybe _initl _is_ a better name.
-- 
Duy

^ permalink raw reply

* Re: [PATCH 32/36] pathspec: allow querying for attributes
From: Duy Nguyen @ 2016-10-26 13:33 UTC (permalink / raw)
  To: Stefan Beller; +Cc: Junio C Hamano, Git Mailing List, bmwill
In-Reply-To: <20161022233225.8883-33-sbeller@google.com>

(sorry if this should have been answered if I went through the series
patch by patch, I wanted to do a proper review but finally have to
admit to myself I won't, so I just skim through a single giant diff
instead)

On Sun, Oct 23, 2016 at 6:32 AM, Stefan Beller <sbeller@google.com> wrote:
> +attr;;
> +After `attr:` comes a space separated list of "attribute
> +requirements", all of which must be met in order for the
> +path to be considered a match;

What about (attr=abc def,attr=ghi lkj)? Does it mean (abc && def) ||
(ghi && lkj), or abc && def && ghi && lkj? Or is it forbidden to have
multiple 'attr' attribute in the same pathspec?
-- 
Duy

^ permalink raw reply

* Re: [PATCH/RFC] git.c: support "!!" aliases that do not move cwd
From: Duy Nguyen @ 2016-10-26 13:23 UTC (permalink / raw)
  To: Jeff King
  Cc: Johannes Schindelin, Johannes Sixt, Junio C Hamano,
	Git Mailing List
In-Reply-To: <20161011150118.7eb474yg5c3oe5mn@sigill.intra.peff.net>

On Tue, Oct 11, 2016 at 10:01 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Oct 11, 2016 at 11:44:50AM +0200, Johannes Schindelin wrote:
>
>> > Yeah, that's reasonable, too. So:
>> >
>> >   [alias]
>> >     d2u = "!dos2unix"
>> >
>> > acts exactly as if:
>> >
>> >   [alias "d2u"]
>> >     command = dos2unix
>> >     type = shell
>> >
>> > was specified at that point, which is easy to understand.
>>
>> It is easy to understand, and even easier to get wrong or out of sync. I
>> really liked the ease of *one* `git config` call to add new aliases. Not
>> sure that I like the need for more such calls just to add *one* alias (one
>> config call for "shell", one for "don't cd up", etc).
>
> Could we simply support alias.d2u indefinitely, and you could use
> whichever format you felt like (the shorter, more limited one if you
> wanted, or the more verbose but flexible one)?

Before this thread goes completely dead... Since there's a lot more
work involved with the new alias.<name>.<property> approach (short
term would be git completion support, longer term would be the ability
to manipulate a config group more conveniently), I'm going with the
"(properties)!command" approach. But even then a new series is not
going to pop up, like, in the next two months.

I don't object the alias.<name>.<property> approach though. It's
definitely a cleaner one in my opinion. It just needs people who can
spend time to follow up until the end. But if someone decides to do
that now, I'll drop the "(properties)!command" and try to support
him/her.
-- 
Duy

^ permalink raw reply

* Re: [Question] Git histrory after greybus merge
From: Dmitry Safonov @ 2016-10-26 13:17 UTC (permalink / raw)
  To: open list
  Cc: gregkh, Greg KH, Linus Torvalds, Junio C Hamano, Git Mailing List
In-Reply-To: <CAJwJo6apgP-pTUccB1Hs81rQbaVCnxeh355fgCKvXc630WTJNg@mail.gmail.com>

Adding Cc: git list, Junio.

2016-10-26 15:55 GMT+03:00 Dmitry Safonov <0x7f454c46@gmail.com>:
> Hi,
>
> Is there any way to specify git-log or git-rev-list which root tree to use?
> I mean, I got the following situation:
> I saw the commit a67dd266adf4 ("netfilter: xtables: prepare for
> on-demand hook register")
> by git-blame and want to see commits on top of that particular commit.
> Earlier I've used for that:
> $ git log --reverse a67dd266adf4^..HEAD
>
> But now after merging greybus it follows the greybus's tree and shows me:
> [linux]$ git log --reverse a67dd266adf4^..HEAD --oneline
> cd26f1bd6bf3 greybus: Initial commit
> c8a797a98cb6 greybus: Import most recent greybus code to new repo.
> 06823c3eb9c4 greybus: README and .gitignore updates
>
> Which quite sucks as this isn't a hash I'm referencing.
> Anyway, back to the question, is there any option to tell git which tree to use?
> I'm sure this was asked before (on btrfs merge?), but I didn't find
> the answer so far.
> I'm using git v2.10.1 if anything.
>
> Thanks,
>              Dmitry

^ permalink raw reply

* Re: [PATCH 7/7] setup_git_env: avoid blind fall-back to ".git"
From: Jeff King @ 2016-10-26 12:31 UTC (permalink / raw)
  To: Duy Nguyen; +Cc: Git Mailing List
In-Reply-To: <CACsJy8AHMJ7OJFKyjYAsPuT0nG=e0RGn59PuX06PJ5pYjinsqA@mail.gmail.com>

On Wed, Oct 26, 2016 at 07:26:20PM +0700, Duy Nguyen wrote:

> > I'm not sure this is really any convenience over dumping a corefile
> > and using gdb to pull out the
> > symbols after the fact.
> 
> So are we back to forcing core files? I'm ok with that! The only
> inconvenience I see is pointing out where the core file is, which
> should be where `pwd` originally is. On linux we can even peek into
> /proc/sys/kernel/core_pattern if we want to be precise. ulimit can get
> in the way, but I don't if the default out there is enable or disable
> core dumping. Once we got OS info and git version, our chances of
> cracking the core files should be reasonably high.

TBH, most of the time I expect the solution to be walking the person
through:

  git clone git://kernel.org/pub/scm/git/git.git
  cd git
  make

  gdb --args ./git whatever
  break die
  run
  bt

which would cover most cases (reproducible breakage, and not in a
sub-program). It's relatively rare that even I resort to corefiles.

-Peff

^ permalink raw reply

* Re: [PATCH 7/7] setup_git_env: avoid blind fall-back to ".git"
From: Duy Nguyen @ 2016-10-26 12:26 UTC (permalink / raw)
  To: Jeff King; +Cc: Git Mailing List
In-Reply-To: <20161026121058.a2pmf57oc7p2mlsp@sigill.intra.peff.net>

On Wed, Oct 26, 2016 at 7:10 PM, Jeff King <peff@peff.net> wrote:
> On Wed, Oct 26, 2016 at 05:29:21PM +0700, Duy Nguyen wrote:
>
>> > I think you could conditionally make git_path() and all of its
>> > counterparts macros, similar to the way the trace code works. It seems
>> > like a pretty maintenance-heavy solution, though. I'd prefer
>> > conditionally compiling backtrace(); that also doesn't hit 100% of
>> > cases, but at least it isn't too invasive.
>>
>> OK, a more polished patch is this. There are warnings about
>> -fomit-function-pointers in glibc man page, at least in my simple
>> tests it does not cause any issue.
>
> Yeah, I tried with -fno-omit-frame-pointer, but it didn't help. The
> glibc backtrace(3) manpage specifically says:
>
>   The symbol names may be unavailable without the use of special linker
>   options. For systems using the GNU linker, it is necessary to use the
>   -rdynamic linker option. Note that names of "static" functions are not
>   exposed, and won't be available in the backtrace.
>
> which matches the behavior I get.
>
> Gcc ships with a libbacktrace which does seem to give reliable results
> (patch below for reference). But that's still relying on gcc, and on
> having debug symbols available.

Yep. On an optimized build you can't get anywhere without debug info,
which has a giant database to describe "if your rip/pc register is
here, then you clue to find your caller is there" for basically every
instruction in your program. Dwarf3 at least is a crazy world.

> I'm not sure this is really any convenience over dumping a corefile and using gdb to pull out the
> symbols after the fact.

So are we back to forcing core files? I'm ok with that! The only
inconvenience I see is pointing out where the core file is, which
should be where `pwd` originally is. On linux we can even peek into
/proc/sys/kernel/core_pattern if we want to be precise. ulimit can get
in the way, but I don't if the default out there is enable or disable
core dumping. Once we got OS info and git version, our chances of
cracking the core files should be reasonably high.
-- 
Duy

^ permalink raw reply

* Re: [PATCH 27/36] attr: convert to new threadsafe API
From: Jeff King @ 2016-10-26 12:15 UTC (permalink / raw)
  To: Simon Ruderich
  Cc: Johannes Schindelin, Stefan Beller, gitster, git, bmwill, pclouds
In-Reply-To: <20161026093558.drsdwn2hm7bbcktt@ruderich.org>

On Wed, Oct 26, 2016 at 11:35:58AM +0200, Simon Ruderich wrote:

> >  static pthread_mutex_t attr_mutex;
> > -#define attr_lock()pthread_mutex_lock(&attr_mutex)
> > +static inline void attr_lock(void)
> > +{
> > +	static int initialized;
> > +
> > +	if (!initialized) {
> > +		pthread_mutex_init(&attr_mutex, NULL);
> > +		initialized = 1;
> > +	}
> > +	pthread_mutex_lock(&attr_mutex);
> > +}
> 
> This may initialize the mutex multiple times during the first
> lock (which may happen in parallel).
> 
> pthread provides static initializers. To quote the man page:
> 
>     Variables of type pthread_mutex_t can also be initialized
>     statically, using the constants PTHREAD_MUTEX_INITIALIZER
>     (for fast mutexes), PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP
>     (for recursive mutexes), and
>     PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP (for error checking
>     mutexes).

I seem to recall this does not work on Windows, where the pthread
functions are thin wrappers over CRITICAL_SECTION. Other threaded code
in git does an explicit setup step before entering threaded sections.
E.g., see start_threads() in builtin/grep.c.

-Peff

^ permalink raw reply

* Re: [PATCH 7/7] setup_git_env: avoid blind fall-back to ".git"
From: Jeff King @ 2016-10-26 12:10 UTC (permalink / raw)
  To: Duy Nguyen; +Cc: Git Mailing List
In-Reply-To: <20161026102921.GA31311@ash>

On Wed, Oct 26, 2016 at 05:29:21PM +0700, Duy Nguyen wrote:

> > I think you could conditionally make git_path() and all of its
> > counterparts macros, similar to the way the trace code works. It seems
> > like a pretty maintenance-heavy solution, though. I'd prefer
> > conditionally compiling backtrace(); that also doesn't hit 100% of
> > cases, but at least it isn't too invasive.
> 
> OK, a more polished patch is this. There are warnings about
> -fomit-function-pointers in glibc man page, at least in my simple
> tests it does not cause any issue.

Yeah, I tried with -fno-omit-frame-pointer, but it didn't help. The
glibc backtrace(3) manpage specifically says:

  The symbol names may be unavailable without the use of special linker
  options. For systems using the GNU linker, it is necessary to use the
  -rdynamic linker option. Note that names of "static" functions are not
  exposed, and won't be available in the backtrace.

which matches the behavior I get.

Gcc ships with a libbacktrace which does seem to give reliable results
(patch below for reference). But that's still relying on gcc, and on
having debug symbols available. I'm not sure this is really any
convenience over dumping a corefile and using gdb to pull out the
symbols after the fact.

---
diff --git a/config.mak.uname b/config.mak.uname
index 76f885281c..62a14f10d3 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -40,6 +40,8 @@ ifeq ($(uname_S),Linux)
 	NEEDS_LIBRT = YesPlease
 	HAVE_GETDELIM = YesPlease
 	SANE_TEXT_GREP=-a
+	BASIC_CFLAGS += -DHAVE_BACKTRACE
+	EXTLIBS += -lbacktrace
 endif
 ifeq ($(uname_S),GNU/kFreeBSD)
 	HAVE_ALLOCA_H = YesPlease
diff --git a/usage.c b/usage.c
index 17f52c1b5c..4b9762ae62 100644
--- a/usage.c
+++ b/usage.c
@@ -5,6 +5,9 @@
  */
 #include "git-compat-util.h"
 #include "cache.h"
+#ifdef HAVE_BACKTRACE
+#include <backtrace.h>
+#endif
 
 static FILE *error_handle;
 static int tweaked_error_buffering;
@@ -24,6 +27,44 @@ void vreportf(const char *prefix, const char *err, va_list params)
 	fputc('\n', fh);
 }
 
+#ifdef HAVE_BACKTRACE
+static int full_callback(void *fh, uintptr_t pc,
+			 const char *filename, int lineno,
+			 const char *function)
+{
+	if (!function || !filename)
+		return 0;
+	fprintf(fh, "debug:  %s() at %s:%d\n", function, filename, lineno);
+	return 0;
+}
+
+static void error_callback(void *fh, const char *msg, int errnum)
+{
+	fprintf(fh, "backtrace error: %s", msg);
+	if (errnum > 0)
+		fprintf(fh, ": %s", strerror(errnum));
+	fputc('\n', fh);
+}
+
+static void maybe_backtrace(void)
+{
+	FILE *fh = error_handle ? error_handle : stderr;
+	struct backtrace_state *bt;
+
+	if (!git_env_bool("GIT_BACKTRACE_ON_DIE", 0))
+		return;
+
+	/* XXX obviously unportable use of /proc */
+	bt = backtrace_create_state("/proc/self/exe", 0, error_callback, fh);
+	if (bt) {
+		fprintf(fh, "debug: die() called at:\n");
+		backtrace_full(bt, 3, full_callback, error_callback, fh);
+	}
+}
+#else
+#define maybe_backtrace()
+#endif
+
 static NORETURN void usage_builtin(const char *err, va_list params)
 {
 	vreportf("usage: ", err, params);
@@ -33,6 +74,7 @@ static NORETURN void usage_builtin(const char *err, va_list params)
 static NORETURN void die_builtin(const char *err, va_list params)
 {
 	vreportf("fatal: ", err, params);
+	maybe_backtrace();
 	exit(128);
 }
 

^ permalink raw reply related

* Re: [PATCH 7/7] setup_git_env: avoid blind fall-back to ".git"
From: Duy Nguyen @ 2016-10-26 10:29 UTC (permalink / raw)
  To: Jeff King; +Cc: Git Mailing List
In-Reply-To: <20161025151524.y7wwtetohhqgcvob@sigill.intra.peff.net>

On Tue, Oct 25, 2016 at 11:15:25AM -0400, Jeff King wrote:
> > The "once we've identified" part could be tricky though. This message
> > alone will not give us any clue where it's called since it's buried
> > deep in git_path() usually, which is buried deep elsewhere. Without
> > falling back to core dumps (with debug info), glibc's backtrace
> > (platform specifc), the best we could do is turn git_path() into a
> > macro that takes __FILE__ and __LINE__ and somehow pass the info down
> > here, but "..." in macros is C99 specific, sigh..
> > 
> > Is it too bad to turn git_path() into a macro when we know the
> > compiler is C99 ? Older compilers will have no source location info in
> > git_path(), Hopefully they are rare, which means chances of this fault
> > popping up are also reduced.
> 
> I think you could conditionally make git_path() and all of its
> counterparts macros, similar to the way the trace code works. It seems
> like a pretty maintenance-heavy solution, though. I'd prefer
> conditionally compiling backtrace(); that also doesn't hit 100% of
> cases, but at least it isn't too invasive.

OK, a more polished patch is this. There are warnings about
-fomit-function-pointers in glibc man page, at least in my simple
tests it does not cause any issue.

> But I think I still prefer just letting the corefile and the debugger do
> their job. This error shouldn't happen much, and when it does, it should
> be easily reproducible. Getting the bug reporter to give either a
> reproduction recipe, or to run "gdb git" doesn't seem like that big a
> hurdle.

There are other places where backtrace() support may come handy
too. If -rdynamic was not needed, I would push for this patch. Too bad
backtrace() is not a perfect magic wand.

-- 8< --
diff --git a/config.mak.uname b/config.mak.uname
index b232908..b38f62a 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -40,6 +40,8 @@ ifeq ($(uname_S),Linux)
 	NEEDS_LIBRT = YesPlease
 	HAVE_GETDELIM = YesPlease
 	SANE_TEXT_GREP=-a
+	# for backtrace() support with glibc >= 2.1
+	BASIC_LDFLAGS += -rdynamic
 endif
 ifeq ($(uname_S),GNU/kFreeBSD)
 	HAVE_ALLOCA_H = YesPlease
diff --git a/git-compat-util.h b/git-compat-util.h
index 43718da..3561ab9 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -408,6 +408,7 @@ extern NORETURN void usage(const char *err);
 extern NORETURN void usagef(const char *err, ...) __attribute__((format (printf, 1, 2)));
 extern NORETURN void die(const char *err, ...) __attribute__((format (printf, 1, 2)));
 extern NORETURN void die_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));
+extern NORETURN void BUG(const char *err, ...) __attribute__((format (printf, 1, 2)));
 extern int error(const char *err, ...) __attribute__((format (printf, 1, 2)));
 extern int error_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));
 extern void warning(const char *err, ...) __attribute__((format (printf, 1, 2)));
@@ -709,6 +710,7 @@ extern int git_vsnprintf(char *str, size_t maxsize,
 #ifdef __GLIBC_PREREQ
 #if __GLIBC_PREREQ(2, 1)
 #define HAVE_STRCHRNUL
+#define HAVE_BACKTRACE
 #endif
 #endif
 
@@ -722,6 +724,23 @@ static inline char *gitstrchrnul(const char *s, int c)
 }
 #endif
 
+#ifdef HAVE_BACKTRACE
+#include <execinfo.h>
+static inline void dump_backtrace(FILE *fp)
+{
+	void *buffer[32];
+	int nptrs;
+
+	nptrs = backtrace(buffer, sizeof(buffer) / sizeof(*buffer));
+	fflush(fp);
+	backtrace_symbols_fd(buffer, nptrs, fileno(fp));
+}
+#else
+static inline void dump_backtrace(FILE *fp)
+{
+}
+#endif
+
 #ifdef NO_INET_PTON
 int inet_pton(int af, const char *src, void *dst);
 #endif
diff --git a/usage.c b/usage.c
index 17f52c1..b00603c 100644
--- a/usage.c
+++ b/usage.c
@@ -204,3 +204,16 @@ void warning(const char *warn, ...)
 	warn_routine(warn, params);
 	va_end(params);
 }
+
+void NORETURN BUG(const char *fmt, ...)
+{
+	va_list params;
+
+	va_start(params, fmt);
+	vreportf("BUG: ", fmt, params);
+	va_end(params);
+
+	dump_backtrace(error_handle ? error_handle : stderr);
+
+	exit(128);
+}
-- 8< --
--
Duy

^ permalink raw reply related


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox