date:20161025

On Tue, Oct 25, 2016 at 11:16:20AM -0700, Junio C Hamano wrote:

> diff --git a/sha1_file.c b/sha1_file.c
> index 5d2bcd3ed1..09045df1dc 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -1571,12 +1571,17 @@ int git_open(const char *name)
>   if (fd >= 0)
>   return fd;
>  
> - /* Might the failure be due to O_NOATIME? */
> - if (errno != ENOENT && sha1_file_open_flag) {
> - sha1_file_open_flag = 0;
> + /* 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.

Because of the order here, this would not be a regression (i.e., any
system that used to work will still eventually find a working comb), but
it does mean that systems without O_NOATIME do not get the benefit of
the new O_CLOEXEC protection.

Unfortunately, I think covering all of the cases would be 2^nr_flags.
That's only 4 right now, but it does not bode well as a pattern.

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.

-Peff

A bug with "git svn show-externals"

2016-10-25 Thread Tao Peng

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

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

Thanks,
Peng

Re: [PATCH] Allow stashes to be referenced by index only

2016-10-25 Thread Ramsay Jones



On 25/10/16 22:41, Junio C Hamano wrote:
> Aaron M Watson  writes:
> 
> Aaron M Watson  writes:
> 
>> Instead of referencing "stash@{n}" explicitly, it can simply be
>> referenced as "n".
>> Most users only reference stashes by their position
>> in the stash stask (what I refer to as the "index").
> 
> It is unclear if the first sentence is a statement of the fact, an
> expression of desire, or something else.  With the current codebase,
> it cannot simply be referenced as "n", and you either "wish it were
> possible", or "make it possible to do so", or perhaps little bit of
> both.
> 
> This is why we tend to use imperative mood to give an order to the
> codebase to "be like so" to make it clear.
> 
> Perhaps
> 
>   Instead of referencing "stash@{n}" explicitly, make it possible to
>   simply reference as "n".  Most users only reference stashes by their
>   position in the stash stask (what I refer to as the "index" here).

s/stask/stack/

ATB,
Ramsay Jones

Re: [PATCH] reset: --unmerge

Duy Nguyen  writes:

> BTW making git-add (and "git commit -a") refuse files with conflict
> markers present could prevent this mistake earlier and is probably a
> better option because the user won't have to discover 'reset
> --unmerge'.

That may help some users, but you are solving a different problem.

I do not say "save" unless I know the editor buffer contents is not
ready.  "This is ready to be saved" however is different from "This
resolution is correct", and I need the unmerged states in the index
to verify, namely by looking at "git diff" (no other parameters)
output that shows only the paths with unmerged stages and in the
compact combined diff format.

Somebody with a bright idea decided that vc-git-resolve-conflicts
variable should be on by default in Emacs 25.1 X-<, which causes
"save" after resolving conflicts to automatically run "git add", to
destroy this valuable tool.  My knee-jerk reaction, of course, to
such a default is "that's brain dead", but perhaps they did so for
some good reason that I fail to fathom.

Re: [PATCH v3 0/3] quick reroll of Lars's git_open() w/ O_CLOEXEC

Lars Schneider  writes:

> heavy CPU/network task in the background now (I can't/don't want to
> stop that ATM).
>
> Here is the relevant part of the log:
>
> ++ rm -f rot13-filter.log
> ++ git checkout --quiet --no-progress .
> + test_eval_ret_=255
> + want_trace
> + test t = t
> + test t = t
> + set +x
> error: last command exited with $?=255
>
> Looks like Git exists with 255. Any idea why that might happen?

Other than "the --quiet above may be hiding it?", I do not
immediately have a useful guess, sorry.

> @Dscho/Johannes: Can you try this on your Windows machines?
>
> Thanks,
> Lars

Re: [PATCH v3 3/3] read-cache: make sure file handles are not inherited by child processes

Eric Wong  writes:

> But I have a _slight_ preference towards Dscho's version in
>  in case we
> decide to start using another O_* flag in here.

Interesting.  The reason why I have a slight preferene to separate
the fixed part and the toggle-able part is exactly because I want
the code to be prepared in case we decide to start using other O_*
flags in the future.

I guess different people have different tastes, as usual ;-)

Re: [PATCH v3 0/3] quick reroll of Lars's git_open() w/ O_CLOEXEC

2016-10-25 Thread Lars Schneider

> On 25 Oct 2016, at 20:16, Junio C Hamano  wrote:
> 
> Here is to make sure everybody is on the same page regarding the
> series.  The patches are adjusted for comments from Eric and Dscho.

Thank you, Junio! Your v3 looks good to me and I compiled and tested
it on Windows.

There is one catch though:
I ran the tests multiple times on Windows and I noticed that the
following test seems flaky:
t0021 `required process filter should be used only for "clean" operation only'

This flaky-ness was not introduced by your v3. I was able to reproduce
the same behavior for v2. I wonder why I did not noticed that before. 
The only difference to before is that my Windows machines does some 
heavy CPU/network task in the background now (I can't/don't want to
stop that ATM).

Here is the relevant part of the log:

++ rm -f rot13-filter.log
++ git checkout --quiet --no-progress .
+ test_eval_ret_=255
+ want_trace
+ test t = t
+ test t = t
+ set +x
error: last command exited with $?=255

Looks like Git exists with 255. Any idea why that might happen?

@Dscho/Johannes: Can you try this on your Windows machines?

Thanks,
Lars

Re: [PATCH] Allow stashes to be referenced by index only

Aaron M Watson  writes:

Aaron M Watson  writes:

> Instead of referencing "stash@{n}" explicitly, it can simply be
> referenced as "n".
> Most users only reference stashes by their position
> in the stash stask (what I refer to as the "index").

It is unclear if the first sentence is a statement of the fact, an
expression of desire, or something else.  With the current codebase,
it cannot simply be referenced as "n", and you either "wish it were
possible", or "make it possible to do so", or perhaps little bit of
both.

This is why we tend to use imperative mood to give an order to the
codebase to "be like so" to make it clear.

Perhaps

  Instead of referencing "stash@{n}" explicitly, make it possible to
  simply reference as "n".  Most users only reference stashes by their
  position in the stash stask (what I refer to as the "index" here).

or something like that (which is what I tenatively rewritten this to
while queuing).

> @@ -404,6 +403,9 @@ parse_flags_and_rev()
>   die "$(eval_gettext "unknown option: 
> \$opt")"
>   FLAGS="${FLAGS}${FLAGS:+ }$opt"
>   ;;
> + *)
> + REV="${REV}${REV:+ }'$opt'"
> + ;;
>   esac
>   done
>  
> @@ -422,6 +424,15 @@ parse_flags_and_rev()
>   ;;
>   esac
>  
> + case "$1" in
> + *[!0-9]*)
> + :
> + ;;
> + *)
> + set -- "${ref_stash}@{$1}"
> + ;;
> + esac

I can see that you inherited the brokenness from an existing one in
the earlier hunk, but case arms in these two case statements are
indented one level too deep.  It would be good to fix it in a
follow-up patch (not a reroll of this patch).

Thanks.  Will queue.

Re: [PATCH v2 2/2] read-cache: make sure file handles are not inherited by child processes

2016-10-25 Thread Johannes Sixt


Am 24.10.2016 um 21:53 schrieb Lars Schneider:



On 24 Oct 2016, at 21:22, Johannes Sixt  wrote:

Am 24.10.2016 um 20:03 schrieb larsxschnei...@gmail.com:

From: Lars Schneider 

This fixes "convert: add filter..process option" (edcc8581) on
Windows.


Today's next falls flat on its face on Windows in t0021.15
"required process filter should filter data"; might it be the
failure meant here?(I haven't dug deeper, yet.)


Yes, this is the failure meant here :-)


I trust your word and Dscho's that it fixes a failure on Windows. In my 
case, however, it was an outdated perl (5.8.8) which left a message 
along the lines of "lookup of member flush in IO::Handle failed" in one 
of the *.log files. I came up with the following workaround.


Fix-method: shot-in-the-dark
Perl-foo: not-present
Not-signed-off-by: Me

diff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl
index ae4c50f..bb88c5f 100755
--- a/t/t0021/rot13-filter.pl
+++ b/t/t0021/rot13-filter.pl
@@ -22,6 +22,7 @@

 use strict;
 use warnings;
+use FileHandle;

 my $MAX_PACKET_CONTENT_SIZE = 65516;
 my @capabilities= @ARGV;
--
2.10.0.343.g37bc62b

Re: [PATCH v3 3/3] read-cache: make sure file handles are not inherited by child processes

2016-10-25 Thread Eric Wong

Junio C Hamano  wrote:
> @@ -156,7 +156,14 @@ void fill_stat_cache_info(struct cache_entry *ce, struct 
> stat *st)
>  static int ce_compare_data(const struct cache_entry *ce, struct stat *st)
>  {
>   int match = -1;
> - int fd = open(ce->name, O_RDONLY);
> + static int cloexec = O_CLOEXEC;
> + int fd = open(ce->name, O_RDONLY | cloexec);
> +
> + if ((cloexec & O_CLOEXEC) && fd < 0 && errno == EINVAL) {
> + /* Try again w/o O_CLOEXEC: the kernel might not support it */
> + cloexec &= ~O_CLOEXEC;
> + fd = open(ce->name, O_RDONLY | cloexec);
> + }

Seems fine, I prefer not using recursion so it's
easier-to-review.

But I have a _slight_ preference towards Dscho's version in
 in case we
decide to start using another O_* flag in here.
(but I'm not usually a C programmer)

Re: What's cooking in git.git (Oct 2016, #06; Mon, 24)

2016-10-25 Thread Johannes Sixt


Am 25.10.2016 um 20:13 schrieb Stefan Beller:

On Tue, Oct 25, 2016 at 10:15 AM, Junio C Hamano  wrote:

 - the "off-by-one fix" part of sb/submodule-ignore-trailing-slash
   needs to be in the upcoming release but the "trailing /. in base
   should not affect the resolution of ../relative/path" part that
   is still under discussion can wait.  Which means we'd need a few
   more !MINGW prerequisites in the tests by -rc0.
[...]


So maybe instead of adding !MINGW we rather want to apply
https://public-inbox.org/git/2908451e-4273-8826-8989-5572263cc...@kdbg.org/
instead for now?


I was about to submit this very patch again, and only then saw your 
message. So, yes, that's what I propose, too.


Dscho, does this patch fix the test failures that you observed, too? 
Unfortunately, it goes against our endeavor to reduce subshells.


-- Hannes

Re: [PATCH] git-svn: do not reuse caches memoized for a different architecture

2016-10-25 Thread Eric Wong

Johannes Schindelin  wrote:
> +++ b/perl/Git/SVN.pm
> @@ -1658,6 +1658,11 @@ sub tie_for_persistent_memoization {
>   if ($memo_backend > 0) {
>   tie %$hash => 'Git::SVN::Memoize::YAML', "$path.yaml";
>   } else {
> + # first verify that any existing file can actually be loaded
> + # (it may have been saved by an incompatible version)
> + if (-e "$path.db") {
> + unlink "$path.db" unless eval { retrieve("$path.db"); 1 
> };
> + }

That retrieve() call is unlikely to work without "use Storable"
to import it into the current package.

I also favor setting "$path.db" once to detect typos and avoid
going over 80 columns.  Additionally, having error-checking for
unlink might be useful.

So perhaps squashing this on top:

diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm
index 025c894..b3c1460 100644
--- a/perl/Git/SVN.pm
+++ b/perl/Git/SVN.pm
@@ -1660,10 +1660,15 @@ sub tie_for_persistent_memoization {
} else {
# first verify that any existing file can actually be loaded
# (it may have been saved by an incompatible version)
-   if (-e "$path.db") {
-   unlink "$path.db" unless eval { retrieve("$path.db"); 1 
};
+   my $db = "$path.db";
+   if (-e $db) {
+   use Storable qw(retrieve);
+
+   if (!eval { retrieve($db); 1 }) {
+   unlink $db or die "unlink $db failed: $!";
+   }
}
-   tie %$hash => 'Memoize::Storable', "$path.db", 'nstore';
+   tie %$hash => 'Memoize::Storable', $db, 'nstore';
}
 }
 

Thoughts?  Thanks.

Re: password forgot

2016-10-25 Thread Dennis Kaarsemaker

That is asking for your local computer password. This uninstaller also
has nothing to do with git itself, but with the third-party git package
you installed. The git project does not ship an 'uninstall.sh' 

On Tue, 2016-10-25 at 13:23 -0300, Luciano Schillagi wrote:
> sorry, I'm a little confused
> 
> and this? 
> 
> 
> 
> 
> 2016-10-25 13:16 GMT-03:00 Dennis Kaarsemaker :
> > On Tue, 2016-10-25 at 12:52 -0300, Luciano Schillagi wrote:
> > > Hi,
> > >
> > > I forgot my password in git, such as resetting?
> > 
> > Hi Luciano,
> > 
> > Git itself doesn't do any authentication, so I assume you lost the
> > password for an account on a hosted git solution such as gitlab or
> > github.
> > 
> > You should contact the support team of whatever hoster you use, the git
> > developers cannot help you here.
> > 
> > D.
> > 
> 
> 
>

Re: What's cooking in git.git (Oct 2016, #06; Mon, 24)

Jeff King  writes:

> On Mon, Oct 24, 2016 at 06:09:00PM -0700, Junio C Hamano wrote:
>
>>  - lt/abbrev-auto and its follow-up jk/abbrev-auto are about auto
>>scaling the default abbreviation length when Git produces a short
>>object name to adjust to the modern times.  Peff noticed one
>>fallout from it recently and its fix jc/abbrev-auto is not yet in
>>'next'.  I would not be surprised if there are other uncovered
>>fallouts remaining in the code, but at the same time, I expect
>>they are all cosmetic kind that do not affect correctness, so I
>>am inclined to include all of them in the upcoming release.
>
> Yeah, I'd agree any fallouts are likely to be purely cosmetic (and if
> there _is_ some script broken by this, it was an accident waiting to
> happen as soon as it was used in a repo with a partial hash collision).
>
> I'm still not sure if people will balk just at the increased length in
> all of their output. I think I'm finally starting to get used to it. :)

I am finally getting used to it.  At this point, I think the
transition plan would be to tell them to set core.abbrev to
whatever default they like.

>> * jc/abbrev-auto (2016-10-22) 4 commits
>>  - transport: compute summary-width dynamically
>>  - transport: allow summary-width to be computed dynamically
>>  - fetch: pass summary_width down the callchain
>>  - transport: pass summary_width down the callchain
>>  (this branch uses jk/abbrev-auto and lt/abbrev-auto.)
>> 
>>  "git push" and "git fetch" reports from what old object to what new
>>  object each ref was updated, using abbreviated refnames, and they
>>  attempt to align the columns for this and other pieces of
>>  information.  The way these codepaths compute how many display
>>  columns to allocate for the object names portion of this output has
>>  been updated to match the recent "auto scale the default
>>  abbreviation length" change.
>> 
>>  Will merge to 'next'.
>
> In case it was not obvious, I think this topic is good-to-go. And
> clearly any decision on lt/abbrev-auto should apply to this one, too. I
> notice you built it on jk/abbrev-auto, though, which is listed as
> "undecided". That's fine by me, but I think it would technically hold
> this topic hostage. You might want to adjust that before merging to
> next.

I am planning to merge both lt/* and jk/*; I should have said it
more clearly.

Thanks.

Re: [PATCH] hex: use unsigned index for ring buffer

Jeff King  writes:

>> 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 = _array[3 & ++index];
>> +struct strbuf *sb = _array[index];
>> +index = (index + 1) % ARRAY_SIZE(pathname_array);
>>  strbuf_reset(sb);
>>  return sb;
>
> This converts the pre-increment to a post-increment, but I don't think
> it matters.

Yes, I think that using the ring buffer from the beginning, not from
the second element from the beginning, is conceptually cleaner ;-).

Re: What's cooking in git.git (Oct 2016, #06; Mon, 24)

Stefan Beller  writes:

> Ok. The first 2 patches are in good shape for this cycle, though.

I thought both of you knew we are in agreement on that by now, but
yes, the off-by-one fix needs to be in the upcoming release.

> So maybe instead of adding !MINGW we rather want to apply
> https://public-inbox.org/git/2908451e-4273-8826-8989-5572263cc...@kdbg.org/
> instead for now?

That sounds good to me.

The "/." thing we would want to come to agreement during the
upcoming feature freeze and it would be very good if we can push the
result out early in the next cycle, but I feel that it is premature
for the upcoming release.

Thanks.

Re: [PATCH] hex: use unsigned index for ring buffer

On Tue, Oct 25, 2016 at 11:28:40AM -0700, Junio C Hamano wrote:

> OK, here is what I'll queue then.
> I assumed that René wants to sign it off ;-).
> 
> -- >8 --
> From: René Scharfe 
> 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.

Looks good to me.

> 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 = _array[3 & ++index];
> + struct strbuf *sb = _array[index];
> + index = (index + 1) % ARRAY_SIZE(pathname_array);
>   strbuf_reset(sb);
>   return sb;

This converts the pre-increment to a post-increment, but I don't think
it matters.

-Peff

Re: What's cooking in git.git (Oct 2016, #06; Mon, 24)

On Mon, Oct 24, 2016 at 06:09:00PM -0700, Junio C Hamano wrote:

>  - lt/abbrev-auto and its follow-up jk/abbrev-auto are about auto
>scaling the default abbreviation length when Git produces a short
>object name to adjust to the modern times.  Peff noticed one
>fallout from it recently and its fix jc/abbrev-auto is not yet in
>'next'.  I would not be surprised if there are other uncovered
>fallouts remaining in the code, but at the same time, I expect
>they are all cosmetic kind that do not affect correctness, so I
>am inclined to include all of them in the upcoming release.

Yeah, I'd agree any fallouts are likely to be purely cosmetic (and if
there _is_ some script broken by this, it was an accident waiting to
happen as soon as it was used in a repo with a partial hash collision).

I'm still not sure if people will balk just at the increased length in
all of their output. I think I'm finally starting to get used to it. :)

> * jc/abbrev-auto (2016-10-22) 4 commits
>  - transport: compute summary-width dynamically
>  - transport: allow summary-width to be computed dynamically
>  - fetch: pass summary_width down the callchain
>  - transport: pass summary_width down the callchain
>  (this branch uses jk/abbrev-auto and lt/abbrev-auto.)
> 
>  "git push" and "git fetch" reports from what old object to what new
>  object each ref was updated, using abbreviated refnames, and they
>  attempt to align the columns for this and other pieces of
>  information.  The way these codepaths compute how many display
>  columns to allocate for the object names portion of this output has
>  been updated to match the recent "auto scale the default
>  abbreviation length" change.
> 
>  Will merge to 'next'.

In case it was not obvious, I think this topic is good-to-go. And
clearly any decision on lt/abbrev-auto should apply to this one, too. I
notice you built it on jk/abbrev-auto, though, which is listed as
"undecided". That's fine by me, but I think it would technically hold
this topic hostage. You might want to adjust that before merging to
next.

-Peff

Re: [PATCH] hex: use unsigned index for ring buffer

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

-- >8 --
From: René Scharfe 
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 
Signed-off-by: Junio C Hamano 
---
 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 = _array[3 & ++index];
+   struct strbuf *sb = _array[index];
+   index = (index + 1) % ARRAY_SIZE(pathname_array);
strbuf_reset(sb);
return sb;
 }
-- 
2.10.1-777-gd068e6bde7

[PATCH v3 0/3] quick reroll of Lars's git_open() w/ O_CLOEXEC

Here is to make sure everybody is on the same page regarding the
series.  The patches are adjusted for comments from Eric and Dscho.

Lars Schneider (3):
  sha1_file: rename git_open_noatime() to git_open()
  sha1_file: open window into packfiles with O_CLOEXEC
  read-cache: make sure file handles are not inherited by child
processes

 builtin/pack-objects.c |  2 +-
 cache.h|  2 +-
 pack-bitmap.c  |  2 +-
 read-cache.c   |  9 -
 sha1_file.c| 25 +++--
 5 files changed, 26 insertions(+), 14 deletions(-)

-- 
2.10.1-777-gd068e6bde7

[PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC

From: Lars Schneider 

All processes that the Git main process spawns inherit the open file
descriptors of the main process. These leaked file descriptors can
cause problems.

Use the O_CLOEXEC flag similar to 05d1ed61 to fix the leaked file
descriptors.

Signed-off-by: Lars Schneider 
Signed-off-by: Junio C Hamano 
---

 * And the remainder of original 1/2, again taking suggestion by DScho.

 sha1_file.c | 13 +
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index 5d2bcd3ed1..09045df1dc 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1561,7 +1561,7 @@ int check_sha1_signature(const unsigned char *sha1, void 
*map,
 
 int git_open(const char *name)
 {
-   static int sha1_file_open_flag = O_NOATIME;
+   static int sha1_file_open_flag = O_NOATIME | O_CLOEXEC;
 
for (;;) {
int fd;
@@ -1571,12 +1571,17 @@ int git_open(const char *name)
if (fd >= 0)
return fd;
 
-   /* Might the failure be due to O_NOATIME? */
-   if (errno != ENOENT && sha1_file_open_flag) {
-   sha1_file_open_flag = 0;
+   /* 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;
}
 
+   /* Might the failure be due to O_NOATIME? */
+   if (errno != ENOENT && (sha1_file_open_flag & O_NOATIME)) {
+   sha1_file_open_flag &= ~O_NOATIME;
+   continue;
+   }
return -1;
}
 }
-- 
2.10.1-777-gd068e6bde7

[PATCH v3 1/3] sha1_file: rename git_open_noatime() to git_open()

From: Lars Schneider 

This function is meant to be used when reading from files in the
object store, and the original objective was to avoid smudging atime
of loose object files too often, hence its name.  Because we'll be
extending its role in the next commit to also arrange the file
descriptors they return auto-closed in the child processes, rename
it to lose "noatime" part that is too specific.

Signed-off-by: Lars Schneider 
Signed-off-by: Junio C Hamano 
---

 * This is a pure-rename step suggested by Dscho.

 builtin/pack-objects.c |  2 +-
 cache.h|  2 +-
 pack-bitmap.c  |  2 +-
 sha1_file.c| 12 ++--
 4 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 1e7c2a98a5..0fd52bd6b4 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -720,7 +720,7 @@ static off_t write_reused_pack(struct sha1file *f)
if (!is_pack_valid(reuse_packfile))
die("packfile is invalid: %s", reuse_packfile->pack_name);
 
-   fd = git_open_noatime(reuse_packfile->pack_name);
+   fd = git_open(reuse_packfile->pack_name);
if (fd < 0)
die_errno("unable to open packfile for reuse: %s",
  reuse_packfile->pack_name);
diff --git a/cache.h b/cache.h
index 0dc39a998c..a902ca1f8e 100644
--- a/cache.h
+++ b/cache.h
@@ -1122,7 +1122,7 @@ extern int write_sha1_file(const void *buf, unsigned long 
len, const char *type,
 extern int hash_sha1_file_literally(const void *buf, unsigned long len, const 
char *type, unsigned char *sha1, unsigned flags);
 extern int pretend_sha1_file(void *, unsigned long, enum object_type, unsigned 
char *);
 extern int force_object_loose(const unsigned char *sha1, time_t mtime);
-extern int git_open_noatime(const char *name);
+extern int git_open(const char *name);
 extern void *map_sha1_file(const unsigned char *sha1, unsigned long *size);
 extern int unpack_sha1_header(git_zstream *stream, unsigned char *map, 
unsigned long mapsize, void *buffer, unsigned long bufsiz);
 extern int parse_sha1_header(const char *hdr, unsigned long *sizep);
diff --git a/pack-bitmap.c b/pack-bitmap.c
index b949e51716..39bcc16846 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -266,7 +266,7 @@ static int open_pack_bitmap_1(struct packed_git *packfile)
return -1;
 
idx_name = pack_bitmap_filename(packfile);
-   fd = git_open_noatime(idx_name);
+   fd = git_open(idx_name);
free(idx_name);
 
if (fd < 0)
diff --git a/sha1_file.c b/sha1_file.c
index 266152de36..5d2bcd3ed1 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -370,7 +370,7 @@ void read_info_alternates(const char * relative_base, int 
depth)
int fd;
 
path = xstrfmt("%s/info/alternates", relative_base);
-   fd = git_open_noatime(path);
+   fd = git_open(path);
free(path);
if (fd < 0)
return;
@@ -663,7 +663,7 @@ static int check_packed_git_idx(const char *path, struct 
packed_git *p)
struct pack_idx_header *hdr;
size_t idx_size;
uint32_t version, nr, i, *index;
-   int fd = git_open_noatime(path);
+   int fd = git_open(path);
struct stat st;
 
if (fd < 0)
@@ -1069,7 +1069,7 @@ static int open_packed_git_1(struct packed_git *p)
while (pack_max_fds <= pack_open_fds && close_one_pack())
; /* nothing */
 
-   p->pack_fd = git_open_noatime(p->pack_name);
+   p->pack_fd = git_open(p->pack_name);
if (p->pack_fd < 0 || fstat(p->pack_fd, ))
return -1;
pack_open_fds++;
@@ -1559,7 +1559,7 @@ int check_sha1_signature(const unsigned char *sha1, void 
*map,
return hashcmp(sha1, real_sha1) ? -1 : 0;
 }
 
-int git_open_noatime(const char *name)
+int git_open(const char *name)
 {
static int sha1_file_open_flag = O_NOATIME;
 
@@ -1605,7 +1605,7 @@ static int open_sha1_file(const unsigned char *sha1)
struct alternate_object_database *alt;
int most_interesting_errno;
 
-   fd = git_open_noatime(sha1_file_name(sha1));
+   fd = git_open(sha1_file_name(sha1));
if (fd >= 0)
return fd;
most_interesting_errno = errno;
@@ -1613,7 +1613,7 @@ static int open_sha1_file(const unsigned char *sha1)
prepare_alt_odb();
for (alt = alt_odb_list; alt; alt = alt->next) {
const char *path = alt_sha1_path(alt, sha1);
-   fd = git_open_noatime(path);
+   fd = git_open(path);
if (fd >= 0)
return fd;
if (most_interesting_errno == ENOENT)
-- 
2.10.1-777-gd068e6bde7

[PATCH v3 3/3] read-cache: make sure file handles are not inherited by child processes

From: Lars Schneider 

This fixes "convert: add filter..process option" (edcc8581) on
Windows.

Consider the case of a file that requires filtering and is present in
branch A but not in branch B. If A is the current HEAD and we checkout B
then the following happens:

1. ce_compare_data() opens the file
2.   index_fd() detects that the file requires to run a clean filter and
 calls index_stream_convert_blob()
4. index_stream_convert_blob() calls convert_to_git_filter_fd()
5.   convert_to_git_filter_fd() calls apply_filter() which creates a
 new long running filter process (in case it is the first file
 of this kind to be filtered)
6.   The new filter process inherits all file handles. This is the
 default on Linux/OSX and is explicitly defined in the
 `CreateProcessW` call in `mingw.c` on Windows.
7. ce_compare_data() closes the file
8. Git unlinks the file as it is not present in B

The unlink operation does not work on Windows because the filter process
has still an open handle to the file. On Linux/OSX the unlink operation
succeeds but the file descriptors still leak into the child process.

Fix this problem by opening files in read-cache with the O_CLOEXEC flag
to ensure that the file descriptor does not remain open in a newly
spawned process similar to 05d1ed6148 ("mingw: ensure temporary file
handles are not inherited by child processes", 2016-08-22).

Signed-off-by: Lars Schneider 
Signed-off-by: Junio C Hamano 
---

 * With Eric's suggestion to avoid repeated EINVAL and fallback,
   implemented with Dscho's "write open twice explicitly, we are
   retrying after all" coding style.

 read-cache.c | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/read-cache.c b/read-cache.c
index 38d67faf70..db5d910642 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -156,7 +156,14 @@ void fill_stat_cache_info(struct cache_entry *ce, struct 
stat *st)
 static int ce_compare_data(const struct cache_entry *ce, struct stat *st)
 {
int match = -1;
-   int fd = open(ce->name, O_RDONLY);
+   static int cloexec = O_CLOEXEC;
+   int fd = open(ce->name, O_RDONLY | cloexec);
+
+   if ((cloexec & O_CLOEXEC) && fd < 0 && errno == EINVAL) {
+   /* Try again w/o O_CLOEXEC: the kernel might not support it */
+   cloexec &= ~O_CLOEXEC;
+   fd = open(ce->name, O_RDONLY | cloexec);
+   }
 
if (fd >= 0) {
unsigned char sha1[20];
-- 
2.10.1-777-gd068e6bde7

Re: What's cooking in git.git (Oct 2016, #06; Mon, 24)

2016-10-25 Thread Stefan Beller

On Tue, Oct 25, 2016 at 10:15 AM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> One of the initial ways to work around the bugfix was to
>>
>> git clone . root # <- add in this step and it works again.
>> git clone root super
>>
>> but instead I will do the preparation for the 'super' project not
>> in '.' but in 'root', just called differently ("super_remote" ?)
>>
>> An additional new test for cloning from '.' will be introduced, too.
>>
>> I plan on working on that with highest priority for git after finishing
>> some attr stuff that I currently have open. So expect a patch (or two)
>> this week.
>
> Hmph, I personally would prefer to defer the "correct behaviour for
> /." part for the next cycle, which is why I wrote:

Ok. The first 2 patches are in good shape for this cycle, though.
And the /. thing will wait until next cycle then, i.e. I can drop priority
as I wish

>
>  - the "off-by-one fix" part of sb/submodule-ignore-trailing-slash
>needs to be in the upcoming release but the "trailing /. in base
>should not affect the resolution of ../relative/path" part that
>is still under discussion can wait.  Which means we'd need a few
>more !MINGW prerequisites in the tests by -rc0.
>
> at the beginning of the message you are responding to, and I also
> thought that was consistent and in agreement with what you said
> earlier in 
> 
>
>> On Sat, Oct 22, 2016 at 10:11 AM, Junio C Hamano  wrote:
>>
>> >
>> > There isn't enough time to include this topic in the upcoming
>> > release within the current https://tinyurl.com/gitCal calendar,
>> > however, which places the final on Nov 11th.
>> >
>> > I am wondering if it makes sense to delay 2.11 by moving the final
>> > by 4 weeks to Dec 9th.
>> >
>> > Thoughts?
>> >
>> > Speaking of what to and not to include in the upcoming release, we
>> > do want to include Stefan's off-by-one fix to the submodule-helper,
>> > but that is blocked on Windows end due to the test.
>>
>> I'd be happy either way, i.e. we could revert that fix and make a release?
>> AFAICT, Windows only has broken tests, not broken functionality with that
>> submodule bug fix.
>
> to which I responded in 

and you said:
> It of course needs help from
> Windows folks to validate the results.

So maybe instead of adding !MINGW we rather want to apply
https://public-inbox.org/git/2908451e-4273-8826-8989-5572263cc...@kdbg.org/
instead for now?

Re: [PATCH] reset: --unmerge

2016-10-25 Thread Sergey Organov

Junio C Hamano  writes:

> The procedure to resolve a merge conflict typically goes like this:
>
>  - first open the file in the editor, and with the help of conflict
>markers come up with a resolution.
>
>  - save the file.
>
>  - look at the output from "git diff" to see the combined diff to
>double check if the resolution makes sense.
>
>  - perform other tests, like trying to build the result with "make".
>
>  - finally "git add file" to mark that you are done.
>
> and repeating the above until you are done with all the conflicted
> paths.  If you, for whatever reason, accidentally "git add file" by
> mistake until you are convinced that you resolved it correctly (e.g.
> doing "git add file" immediately after saving, without a chance to
> peruse the output from "git diff"), there is no good way to recover.

"git reset --unmerge file"

to undo accidental

"git add file"

during conflict resolution?

I'm afraid "unmerge" sounds like revert of "merge", rather than revert
of "resolve". I'd rather prefer to see something like:

git add --undo file

git merge --unresolve file

git reset --unresolve file

in that order, to deal with the issue.

-- Sergey

Re: [PATCH v1 00/19] Add configuration options for split-index

Duy Nguyen  writes:

> ... But those files
> people generate manually and refer to them with $GIT_INDEX_FILE, we
> can't know where they are.

Then we probably should stop doing that, i.e. disable split-index
automatically for these temporary ones, perhaps, and even with
Christian's series which allows use of split index on the primary
one easier (which is a good idea), make sure we don't auto split the
temporary ones the users create?

> Timestamps allow us to say, ok this base index file has not been read
> by anybody for N+ hours (or better, days), it's most likely not
> referenced by any temporary index files (including
> $GIT_DIR/index.lock) anymore because those files, by the definition of
> "temporary", must be gone by now

and if we guessed wrong, users will have a "temporary index" that
they meant to keep for longer term that is now broken here.  I am
not sure if that risk is worth taking.

Re: What's cooking in git.git (Oct 2016, #06; Mon, 24)

Stefan Beller  writes:

> One of the initial ways to work around the bugfix was to
>
> git clone . root # <- add in this step and it works again.
> git clone root super
>
> but instead I will do the preparation for the 'super' project not
> in '.' but in 'root', just called differently ("super_remote" ?)
>
> An additional new test for cloning from '.' will be introduced, too.
>
> I plan on working on that with highest priority for git after finishing
> some attr stuff that I currently have open. So expect a patch (or two)
> this week.

Hmph, I personally would prefer to defer the "correct behaviour for
/." part for the next cycle, which is why I wrote:

 - the "off-by-one fix" part of sb/submodule-ignore-trailing-slash
   needs to be in the upcoming release but the "trailing /. in base
   should not affect the resolution of ../relative/path" part that
   is still under discussion can wait.  Which means we'd need a few
   more !MINGW prerequisites in the tests by -rc0.

at the beginning of the message you are responding to, and I also
thought that was consistent and in agreement with what you said
earlier in 

> On Sat, Oct 22, 2016 at 10:11 AM, Junio C Hamano  wrote:
> 
> >
> > There isn't enough time to include this topic in the upcoming
> > release within the current https://tinyurl.com/gitCal calendar,
> > however, which places the final on Nov 11th.
> >
> > I am wondering if it makes sense to delay 2.11 by moving the final
> > by 4 weeks to Dec 9th.
> >
> > Thoughts?
> >
> > Speaking of what to and not to include in the upcoming release, we
> > do want to include Stefan's off-by-one fix to the submodule-helper,
> > but that is blocked on Windows end due to the test.
> 
> I'd be happy either way, i.e. we could revert that fix and make a release?
> AFAICT, Windows only has broken tests, not broken functionality with that
> submodule bug fix.

to which I responded in 

> If you are referring the "trailing /. should not make difference
> when resolving ../relative/path" change with "rever that fix", I
> think that may be a reasonable way to proceed.  Even though that
> change is a bugfix (at least from the point of view by me and j6t in
> the recent discussion), it is a behaviour change that we would want
> to see feedback from existing submodule users and deserves a longer
> gestation period.  And that part is not yet in 'next' yet ;-)
> 
> > If we want a longer gestation period, we'd ideally merge it to master
> > just after a release, such that we "cook" it in master without having
> > it in any release (we had a similar discussion for the diff heuristics 
> > IIRC).
> 
> Yes.  
> 
> It would mean that we would need a separate patch that adds the
> !MINGW prerequisite to some tests to what is on 'next', as the early
> patches on sb/submodule-ignore-trailing-slash~ that fixes off-by-one
> is the right thing to do either way.  It of course needs help from
> Windows folks to validate the results.

So...

Re: [PATCH v2 2/2] read-cache: make sure file handles are not inherited by child processes

Johannes Schindelin  writes:

> That still looks overly complicated, repeatedly ORing cloexec and
> recursing without need. How about this instead?
>
>   static int oflags = O_RDONLY | O_CLOEXEC;
>   int fd = open(ce->name, oflags);
>
>   if ((O_CLOEXEC & oflags) && fd < 0 && errno == EINVAL) {
>   /* Try again w/o O_CLOEXEC: the kernel might not support it */
>   oflags &= ~O_CLOEXEC;
>   fd = open(ce->name, oflags);
>   }

I deliberately separated the part that can and designed to be
toggled (O_CLOEXEC) and the part that is meant to be constant
(O_RDONLY), and I do not think the first part of suggestion is
particularly a good idea.

I didn't write the same open twice, but I agree that an extra "we
fallback to opening with a different flags" inside the if () { }
block that is there exactly for implementing that fallback is an
excellent idea.  I like that part of the suggestion.

Thanks.

Re: [PATCH v2 1/2] sha1_file: open window into packfiles with CLOEXEC

Johannes Schindelin  writes:

> The patch series may be a little bit more pleasant to read if you renamed
> git_open_noatime() to git_open() first, in a separate commit.

Possibly.

If this were "we added a new parameter at the same time and each
calling site of the renamed function with an extra parameter chooses
to pass different value to it", then keeping the rename and the
interface enhancement as two separate steps would help a lot.

But this one only renames and updates the internal implementation,
without changing what the calling sites need to do.  I am OK with
having them together in a single patch like the one posted.

>> @@ -1598,12 +1598,18 @@ int git_open_noatime(const char *name)
>>  if (fd >= 0)
>>  return fd;
>>  
>> -/* Might the failure be due to O_NOATIME? */
>> -if (errno != ENOENT && sha1_file_open_flag) {
>> -sha1_file_open_flag = 0;
>> +/* Try again w/o O_CLOEXEC: the kernel might not support it */
>> +if (O_CLOEXEC && errno == EINVAL &&
>> +(sha1_file_open_flag & O_CLOEXEC)) {
>> +sha1_file_open_flag &= ~O_CLOEXEC;
>
> How about
>
>   if ((O_CLOEXEC & sha1_file_open_flag) && errno == EINVAL) {
>   sha1_file_open_flag &= ~O_CLOEXEC;
>
> instead? It is shorter and should be just as easily optimized out by a
> C compiler if O_CLOEXEC was defined as 0.

Yup, I think that makes things easier to read for humans, too.

pre-planning for git contributor's summit: Feb 2, 2017, Brussels

GitHub is organizing Git Merge 2017, and is offering space to do another
contributor's summit. I want to share what details I have so far, get
any input on what we want the event to be like, and see who's interested
in coming (for venue planning purposes).

The details below are still tentative, but I imagine the date and rough
location will not change at this point, as it's scheduled to be adjacent
to FOSDEM.

The plan is to do the contributor summit on Thursday, Feb 2,
2017. The main conference track for Git Merge is the day after, then
FOSDEM on the weekend. The location will be a conference space in
Brussels, Belgium (probably the one in [1]).

Right now we have a room with a projector and whiteboard that fits about
20-25 people. It's pretty conference-roomish. If we're bigger or smaller
than that, there are some alternative rooms. I think we'd ideally know
our approximate size by Dec 1st. We'll probably have the room 10am-3pm,
but that's in flux.

The format, attendee list, etc, are up to us in the Git community. In
past year's it's been pretty loosely organized. One room, about 20 or so
people, and people bringing up topics for discussion, presenting short
talks, etc. I think I'd like to do a _little_ more planning this year,
and have people propose topics a few days ahead of time, rather than on
a whiteboard the morning of the conference.

As far as attendees, in the past it's basically been open to anybody who
participates in development of Git or the surrounding ecosystem. That's
seemed fine to me, but I'm open to input. I'd also say that people who
are _interested_ in joining Git development would be welcome, even if
they haven't contributed a patch yet. Maybe we can convince them. :)

I think in the past it has technically been "invite only", and I've just
had interested people email me and cross-checked them in the list
archive and git-log, or sent a polite "who are you" email to anybody I
didn't recognize. I'm open to changing that to something more
democratic, but AFAIK I didn't exclude anybody in past years.

The registration fee for Git Merge (the whole thing) is $100, and I
think contributor summit registration will be tied to that. I should be
getting some registration codes to hand out that will waive the fee
completely. All of the proceeds are going to Software Freedom
Conservancy, so I'd encourage people who can get their company to send
them to a conference to consider having them pay. But it shouldn't be a
big deal for me to waive fees for contributors who want to do so.

Speaking of money, we've used some Git project money to subsidize travel
for contributors in past years. That's always been done in a very ad hoc
way, with people emailing the Git program committee (me, Junio, Shawn)
and working it out privately. I'm open to alternate proposals there.

I think that's about it. What I'd like to hear from you:

1. A rough count of interested folks. Even if you're not sure you can
make it, let me know (either on the list or via private email) so I
can get an idea and communicate it to the folks booking the room.

2. Any ideas or discussion on the event, format, etc. If there's
something we need from the venue or event organizers, I can try to
arrange it. Sooner is better for such things.

-Peff

PS There isn't an official RFP for talks for the main track yet, nor for
the workshops which will happen the same day as the contributor
summit. But if you have an idea for a talk or a hands-on workshop,
let me know and I can put you in touch with the event organizers.

[1]
https://www.google.com/maps/place/The+Egg/@50.8357424,4.321425,15z/data=!4m13!1m7!3m6!1s0x0:0xe028c6680611d1da!2sThe+Egg!3b1!8m2!3d50.8331946!4d4.3270469!3m4!1s0x0:0xe028c6680611d1da!8m2!3d50.8331946!4d4.3270469

Re: password forgot

2016-10-25 Thread Dennis Kaarsemaker

On Tue, 2016-10-25 at 12:52 -0300, Luciano Schillagi wrote:
> Hi,
> 
> I forgot my password in git, such as resetting?

Hi Luciano,

Git itself doesn't do any authentication, so I assume you lost the
password for an account on a hosted git solution such as gitlab or
github.

You should contact the support team of whatever hoster you use, the git
developers cannot help you here.

D.

password forgot

2016-10-25 Thread Luciano Schillagi

Hi,

I forgot my password in git, such as resetting?

Thank You

Luciano

[PATCH] git-svn: do not reuse caches memoized for a different architecture

From: Gavin Lambert 

Reusing cached data speeds up git-svn by quite a fair bit. However, if
the YAML module is unavailable, the caches are written to disk in an
architecture-dependent manner. That leads to problems when upgrading,
say, from 32-bit to 64-bit Git for Windows.

Let's just try to read those caches back if we detect the absence of the
YAML module and the presence of the file, and delete the file if it
could not be read back correctly.

Note that the only way to catch the error when the memoized cache could
not be read back is to put the call inside an `eval { ... }` block
because it would die otherwise; the `eval` block should also return `1`
in case of success explicitly since the function reading back the cached
data does not return an appropriate value to test for success.

This fixes https://github.com/git-for-windows/git/issues/233.

Signed-off-by: Gavin Lambert 
Signed-off-by: Johannes Schindelin 
---
Published-As: https://github.com/dscho/git/releases/tag/svn-multi-arch-v1
Fetch-It-Via: git fetch https://github.com/dscho/git svn-multi-arch-v1

We carried this for some time in Git for Windows.

 perl/Git/SVN.pm | 5 +
 1 file changed, 5 insertions(+)

diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm
index 018beb8..025c894 100644
--- a/perl/Git/SVN.pm
+++ b/perl/Git/SVN.pm
@@ -1658,6 +1658,11 @@ sub tie_for_persistent_memoization {
if ($memo_backend > 0) {
tie %$hash => 'Git::SVN::Memoize::YAML', "$path.yaml";
} else {
+   # first verify that any existing file can actually be loaded
+   # (it may have been saved by an incompatible version)
+   if (-e "$path.db") {
+   unlink "$path.db" unless eval { retrieve("$path.db"); 1 
};
+   }
tie %$hash => 'Memoize::Storable', "$path.db", 'nstore';
}
 }

base-commit: 659889482ac63411daea38b2c3d127842ea04e4d
-- 
2.10.1.583.g721a9e0

Re: [PATCH 7/7] setup_git_env: avoid blind fall-back to ".git"

On Tue, Oct 25, 2016 at 07:38:30PM +0700, Duy Nguyen wrote:

> > diff --git a/environment.c b/environment.c
> > index cd5aa57..b1743e6 100644
> > --- a/environment.c
> > +++ b/environment.c
> > @@ -164,8 +164,11 @@ static void setup_git_env(void)
> > const char *replace_ref_base;
> >
> > git_dir = getenv(GIT_DIR_ENVIRONMENT);
> > -   if (!git_dir)
> > +   if (!git_dir) {
> > +   if (!startup_info->have_repository)
> > +   die("BUG: setup_git_env called without repository");
> 
> YES!!! Thank you for finally fixing this.

Good, I'm glad somebody besides me is excited about this. I've been
wanting to write this patch for a long time, but it took years of
chipping away at all the edge cases.

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

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.

For fun, here's a patch that uses backtrace(), but it does not actually
print the function names unless you compile with "-rdynamic" (and even
then it misses static functions). There are better libraries, but of
course that's one more thing for the user to deal with when building.

-Peff

---
diff --git a/usage.c b/usage.c
index 17f52c1b5c..4917c6bdfd 100644
--- a/usage.c
+++ b/usage.c
@@ -5,6 +5,9 @@
  */
 #include "git-compat-util.h"
 #include "cache.h"
+#ifdef HAVE_BACKTRACE
+#include 
+#endif
 
 static FILE *error_handle;
 static int tweaked_error_buffering;
@@ -24,6 +27,32 @@ void vreportf(const char *prefix, const char *err, va_list 
params)
fputc('\n', fh);
 }
 
+#ifdef HAVE_BACKTRACE
+static void maybe_backtrace(void)
+{
+   void *bt[100];
+   char **symbols;
+   int nr;
+
+   if (!git_env_bool("GIT_BACKTRACE_ON_DIE", 0))
+   return;
+
+   nr = backtrace(bt, ARRAY_SIZE(bt));
+   symbols = backtrace_symbols(bt, nr);
+   if (symbols) {
+   FILE *fh = error_handle ? error_handle : stderr;
+   int i;
+
+   fprintf(fh, "die() called from:\n");
+   for (i = 0; i < nr; i++)
+   fprintf(fh, "  %s\n", symbols[i]);
+   free(symbols);
+   }
+}
+#else
+#define maybe_backtrace()
+#endif
+
 static NORETURN void usage_builtin(const char *err, va_list params)
 {
vreportf("usage: ", err, params);
@@ -33,6 +62,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);
 }

Re: Integrating submodules with no side effects

2016-10-25 Thread Robert Dailey

On Wed, Oct 19, 2016 at 2:51 PM, Robert Dailey  wrote:
> On Wed, Oct 19, 2016 at 2:45 PM, Stefan Beller  wrote:
>> On Wed, Oct 19, 2016 at 12:19 PM, Robert Dailey
>>  wrote:
>>> On Wed, Oct 19, 2016 at 11:23 AM, Stefan Beller  wrote:
 You could try this patch series:
 https://github.com/jlehmann/git-submod-enhancements/tree/git-checkout-recurse-submodules
 (rebased to a newer version; no functional changes:)
 https://github.com/stefanbeller/git/tree/submodule-co
 (I'll rebase that later to origin/master)

>
> Do you have any info on how I can prevent that error? Ideally I want
> the integration to go smoothly and transparently, not just for the
> person doing the actual transition (me) but for everyone else that
> gets those changes from upstream. They should not even notice that it
> happened (i.e. no failed commands, awkward behavior, or manual steps).

 It depends on how long you want to postpone the transition, but I plan to
 upstream the series referenced above in the near future,
 which would enable your situation to Just Work (tm). ;)
>>>
>>> At first glance, what you've linked to essentially looks like
>>> automated `git submodule update` for every `git checkout`. Am I
>>> misunderstanding?
>>
>> Essentially yes, except with stricter rules than the actual submodule update
>> IIRC.
>>
>>>
>>> If I'm correct, this is not the same as what I'm talking about. The
>>> problem appears to be more internal: When a submodule is removed, the
>>> physical files that were there are not removed by Git.
>>
>> That is also done by that series: submodules ought to be treated as files:
>> If you checkout a new version where a file is deleted, the checkout command
>> will actually remove the file for you (and e.g. solve any
>> directory/file conflicts
>> that may happen in the transition.)
>>
>>> It leaves them
>>> there in the working copy as untracked files.
>>
>> That is the current behavior as checkout tries hard to ignore submodules.
>>
>>> The next step Git takes
>>> (again, just from outside observation) is to add those very same files
>>> to the working copy, since they were added to a commit. However, at
>>> this point Git fails because it's trying to create (write) files to
>>> the working copy when an exact file of that name already exists there.
>>> Git will not overwrite untracked files, so at this point it fails.
>>>
>>> What needs to happen, somehow, is Git sees that the files were
>>> actually part of a submodule (which was removed) and remove the
>>> physical files as well, assuming that they were not modified in the
>>> submodule itself. This will ensure that the next step (creating the
>>> files) will succeed since the files no longer block it.
>>
>> Yep.
>
> It's great we're finally on the same page ;-)
>
> However, I don't see how this problem can be solved with your script,
> or solved in general outside of that. Does this mean that Git needs to
> change to treat submodules as it does normal files, per your previous
> assertion, which means submodules should *not* be left behind in the
> working copy as untracked files?

I'll assume (due to the lack of responses) that the only viable
solution here is to integrate the submodule using a different
directory name than the one used by the submodule itself. It's
unfortunate but I'll do it if I have no other option.

Re: [PATCH 1/7] read info/{attributes,exclude} only when in repository

On Tue, Oct 25, 2016 at 07:24:50PM +0700, Duy Nguyen wrote:

> > Let's detect this situation explicitly and skip reading the
> > file (i.e., the same behavior we'd get if we were in a
> > repository and the file did not exist).
> 
> On the other hand, if we invoke attr machinery too early by mistake,
> before setup_git_directory* is called, then we skip
> .git/info/attributes file as well even though I think we should shout
> "call setup_git_directory first!" so the developer can fix it.

> I wonder if we should have two flags in startup_info to say "yes
> setup_git_dir... has been called, you can trust
> startup_info->have_repository" and "yes, i do not call setup_git_dir
> on purpose, quit complaining" then we could still catch unintended
> .git/info/attributes ignore while letting git grep --no-index work
> correctly.

Yeah, it would be nice for the low-level code to be able to detect such
errors. I don't mind if you want to extend startup_info in that way, but
it will probably introduce a period of instability and regressions
(sites that are perfectly fine, but forgot to set the "I know what I'm
doing" flag).

-Peff

Re: %C(auto) not working as expected

On Tue, Oct 25, 2016 at 7:58 PM, Jeff King  wrote:
> On Tue, Oct 25, 2016 at 07:52:21PM +0700, Duy Nguyen wrote:
>
>> > Yeah, adding a "%C(enable-auto-color)" or something would be backwards
>> > compatible and less painful than using "%C(auto)" everywhere. I do
>> > wonder if anybody actually _wants_ the "always show color, even if
>> > --no-color" behavior. I'm having trouble thinking of a good use for it.
>> >
>> > IOW, I'm wondering if anyone would disagree that the current behavior is
>> > simply buggy.
>>
>> Silence in two weeks. I vote (*) making %() honor --color
>> and turning the %(auto, no-op, for both log family and for-each-ref.
>> We could keep old behavior behind some environment variable if it's
>> not much work so it keeps working while people come here and tell us
>> about their use cases.
>
> Yeah, sorry.

Just to be clear (after re-reading my mail), the unwritten part after
"Silence in two weeks" was "so nobody disagreed with you", not "how is
your progress?". It's great that you keep working on this regardless
:D
-- 
Duy

Re: %C(auto) not working as expected

On Tue, Oct 25, 2016 at 07:52:21PM +0700, Duy Nguyen wrote:

> > Yeah, adding a "%C(enable-auto-color)" or something would be backwards
> > compatible and less painful than using "%C(auto)" everywhere. I do
> > wonder if anybody actually _wants_ the "always show color, even if
> > --no-color" behavior. I'm having trouble thinking of a good use for it.
> >
> > IOW, I'm wondering if anyone would disagree that the current behavior is
> > simply buggy.
> 
> Silence in two weeks. I vote (*) making %() honor --color
> and turning the %(auto, no-op, for both log family and for-each-ref.
> We could keep old behavior behind some environment variable if it's
> not much work so it keeps working while people come here and tell us
> about their use cases.

Yeah, sorry. I was blocked on making %(color:) in ref-filter work, which
required a bunch of refactoring in ref-filter, which conflicted heavily
with the kn/ref-filter-branch-list (which wants to do a lot of the same
things), and then I got blocked on reviewing that series (which overall
looks pretty sane, but I wanted to really dig in because I think it
hasn't gotten very careful review, or at least not recently).

So I'm still hoping to shave that yak at some point. Maybe this week.

-Peff

Re: %C(auto) not working as expected

On Mon, Oct 10, 2016 at 9:28 PM, Jeff King  wrote:
> On Mon, Oct 10, 2016 at 04:26:18PM +0700, Duy Nguyen wrote:
>
>> > If we do a revamp of the pretty-formats to bring them more in line with
>> > ref-filter (e.g., something like "%(color:red)") maybe that would be an
>> > opportunity to make minor adjustments. Though, hmm, it looks like
>> > for-each-ref already knows "%(color:red)", and it's unconditional.
>> >  So perhaps we would need to go through some deprecation period or
>> > other transition.
>>
>> We could add some new tag to change the behavior of all following %C
>> tags. Something like %C(tty) maybe (probably a bad name), then
>> discourage the use if "%C(auto" for terminal detection?
>
> Yeah, adding a "%C(enable-auto-color)" or something would be backwards
> compatible and less painful than using "%C(auto)" everywhere. I do
> wonder if anybody actually _wants_ the "always show color, even if
> --no-color" behavior. I'm having trouble thinking of a good use for it.
>
> IOW, I'm wondering if anyone would disagree that the current behavior is
> simply buggy.

Silence in two weeks. I vote (*) making %() honor --color
and turning the %(auto, no-op, for both log family and for-each-ref.
We could keep old behavior behind some environment variable if it's
not much work so it keeps working while people come here and tell us
about their use cases.

(*) I know.. voting is not how things work around here, unless you
vote with patches, but I can't take on another topic.

> Reading the thread at:
>
>   http://public-inbox.org/git/7v4njkmq07@alter.siamese.dyndns.org/
>
> I don't really see any compelling reason against it (there was some
> question of which config to use, but we already answered that with
> "%C(auto)", and use the value from the pretty_ctx).
>
> -Peff
-- 
Duy

Re: [PATCH 7/7] setup_git_env: avoid blind fall-back to ".git"

On Thu, Oct 20, 2016 at 1:24 PM, Jeff King  wrote:
> This passes the test suite (after the adjustments in the
> previous patches), but there's a risk of regression for any
> cases where the fallback usually works fine but the code
> isn't exercised by the test suite.  So by itself, this
> commit is a potential step backward, but lets us take two
> steps forward once we've identified and fixed any such
> instances.
>
> Signed-off-by: Jeff King 
> ---
>  environment.c | 5 -
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/environment.c b/environment.c
> index cd5aa57..b1743e6 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -164,8 +164,11 @@ static void setup_git_env(void)
> const char *replace_ref_base;
>
> git_dir = getenv(GIT_DIR_ENVIRONMENT);
> -   if (!git_dir)
> +   if (!git_dir) {
> +   if (!startup_info->have_repository)
> +   die("BUG: setup_git_env called without repository");

YES!!! Thank you for finally fixing this.

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

Re: [PATCH 1/7] read info/{attributes,exclude} only when in repository

On Thu, Oct 20, 2016 at 1:16 PM, Jeff King  wrote:
> The low-level attribute and gitignore code will try to look
> in $GIT_DIR/info for any repo-level configuration files,
> even if we have not actually determined that we are in a
> repository (e.g., running "git grep --no-index"). In such a
> case they end up looking for ".git/info/attributes", etc.
>
> This is generally harmless, as such a file is unlikely to
> exist outside of a repository, but it's still conceptually
> the wrong thing to do.
>
> Let's detect this situation explicitly and skip reading the
> file (i.e., the same behavior we'd get if we were in a
> repository and the file did not exist).

On the other hand, if we invoke attr machinery too early by mistake,
before setup_git_directory* is called, then we skip
.git/info/attributes file as well even though I think we should shout
"call setup_git_directory first!" so the developer can fix it.

I wonder if we should have two flags in startup_info to say "yes
setup_git_dir... has been called, you can trust
startup_info->have_repository" and "yes, i do not call setup_git_dir
on purpose, quit complaining" then we could still catch unintended
.git/info/attributes ignore while letting git grep --no-index work
correctly.
-- 
Duy

Re: [RFH] limiting ref advertisements

On Mon, Oct 24, 2016 at 8:29 PM, Jeff King  wrote:
> I'm looking into the oft-discussed idea of reducing the size of ref
> advertisements by having the client say "these are the refs I'm
> interested in". Let's set aside the protocol complexities for a
> moment and imagine we magically have some way to communicate a set of
> patterns to the server.
>
> What should those patterns look like?
>
> I had hoped that we could keep most of the pattern logic on the
> client-side. Otherwise we risk incompatibilities between how the client
> and server interpret a pattern. I had also hoped we could do some kind
> of prefix-matching, which would let the server look only at the
> interesting bits of the ref tree (so if you don't care about
> refs/changes, and the server has some ref storage that is hierarchical,
> they can literally get away without opening that sub-tree).
>
> The patch at the end of this email is what I came up with in that
> direction. It obviously won't compile without the twenty other patches
> implementing transport->advertise_prefixes

Yes! git-upload-pack-2 is making a come back, one form or another.

> but it gives you a sense of what I'm talking about.
>
> Unfortunately it doesn't work in all cases, because refspec sources may
> be unqualified. If I ask for:
>
>   git fetch $remote master:foo
>
> then we have to actually dwim-resolve "master" from the complete list of
> refs we get from the remote.  It could be "refs/heads/master",
> "refs/tags/master", etc. Worse, it could be "refs/master". In that case,
> at least, I think we are OK because we avoid advertising refs directly
> below "refs/" in the first place. But if you have a slash, like:
>
>   git fetch $remote jk/foo
>
> then that _could_ be "refs/jk/foo". Likewise, we cannot even optimize
> the common case of a fully-qualified ref, like "refs/heads/foo". If it
> exists, we obviously want to use that. But if it doesn't, then it
> could be refs/something-else/refs/heads/foo. That's unlikely, but it
> _does_ work now, and optimizing the advertisement would break it.
>
> So it seems like left-anchoring the refspecs can never be fully correct.
> We can communicate "master" to the server, who can then look at every
> ref it would advertise and ask "could this be called master"? But it
> will be setting in stone the set of "could this be" patterns. Granted,
> those haven't changed much over the history of git, but it seems awfully
> fragile.

The first thought that comes to mind is, if left anchoring does not
work, let's support both left and right anchoring. I guess you
considered and discarded this.

If prefix matching does not work, and assuming "some-prefix" sent by
client to be in fact "**/some-prefix" pattern at server side will set
the "could this be" in stone, how about use wildmatch? It's flexible
enough and we have full control over the pattern matching engine so C
Git <-> C Git should be good regardless of platforms. I understand
that wildmatch is still complicated enough that a re-implementation
can easily divert in behavior. But a pattern with only '*', '/**',
'/**/' and '**/' wildcards (in other words, no [] or ?) could make the
engine a lot simpler and still fit our needs (and give some room for
client-optimization).

> In an ideal world the client and server would negotiate to come to some
> agreement on the patterns being used. But as we are bolting this onto
> the existing protocol, I was really trying to do it without introducing
> an extra capabilities phase or extra round-trips. I.e., something like
> David Turner's "stick the refspec in the HTTP query parameters" trick,
> but working everywhere[1].
-- 
Duy

Re: [PATCH] doc: fix the 'revert a faulty merge' ASCII art tab spacing

Hi Philip,

On Mon, 24 Oct 2016, Philip Oakley wrote:

> The asciidoctor doc-tool stack does not always respect the 'tab = 8 spaces' 
> rule
> expectation, particularly for the Git-for-Windows generated html pages. This
> follows on from the 'doc: fix merge-base ASCII art tab spacing' fix.
> 
> Use just spaces within the block of the ascii art.
> 
> All other *.txt ascii art containing three dashes has been checked.
> Asciidoctor correctly formats the other art blocks that do contain tabs.
> 
> Signed-off-by: Philip Oakley  ---
> The git-scm doc pages https://git-scm.com/docs/ does not convert this
> how-to document to html, rather it links to the Github text pages, which
> does respect the 8 space tab rule.

I confirm that this fixes the misaligned branches when building the
documentation with asciidoctor 1.5.4 in Git for Windows' SDK.

Ciao,
Dscho

Re: [PATCH v2 0/2] Use CLOEXEC to avoid fd leaks

Hi Lars,

On Mon, 24 Oct 2016, larsxschnei...@gmail.com wrote:

> This mini patch series is necessary to make the "ls/filter-process" topic
> work properly for Windows. Right now the topic generates test failures
> on Windows as reported by Dscho:
> http://public-inbox.org/git/alpine.DEB.2.20.1610211457030.3264@virtualbox/

I acknowledge that this fixes the test failures in Git for Windows' SDK.

Thanks,
Dscho

Re: [PATCH] reset: --unmerge

On Tue, Oct 25, 2016 at 6:06 PM, Duy Nguyen  wrote:
> On Tue, Oct 25, 2016 at 6:10 AM, Junio C Hamano  wrote:
>> The procedure to resolve a merge conflict typically goes like this:
>>
>>  - first open the file in the editor, and with the help of conflict
>>markers come up with a resolution.
>>
>>  - save the file.
>>
>>  - look at the output from "git diff" to see the combined diff to
>>double check if the resolution makes sense.
>>
>>  - perform other tests, like trying to build the result with "make".
>>
>>  - finally "git add file" to mark that you are done.

BTW making git-add (and "git commit -a") refuse files with conflict
markers present could prevent this mistake earlier and is probably a
better option because the user won't have to discover 'reset
--unmerge'. If the user likes to add the file with conflict markers
anyway (because those look like conflict markers but are in fact not)
then they can go with "git add -f" or similar.
-- 
Duy

Re: [PATCH] reset: --unmerge

On Tue, Oct 25, 2016 at 6:10 AM, Junio C Hamano  wrote:
> The procedure to resolve a merge conflict typically goes like this:
>
>  - first open the file in the editor, and with the help of conflict
>markers come up with a resolution.
>
>  - save the file.
>
>  - look at the output from "git diff" to see the combined diff to
>double check if the resolution makes sense.
>
>  - perform other tests, like trying to build the result with "make".
>
>  - finally "git add file" to mark that you are done.
>
> and repeating the above until you are done with all the conflicted
> paths.  If you, for whatever reason, accidentally "git add file" by
> mistake until you are convinced that you resolved it correctly (e.g.
> doing "git add file" immediately after saving, without a chance to
> peruse the output from "git diff"), there is no good way to recover.

I made this exact mistake on a giant, half-resolved merge conflict the
other day and panicked. While I prepared myself to script update-index
to restore stages 2 and 3, I found "update-index --unresolve". It
sounds like this "reset --unmerge" is the same functionality, right?
I'm not objecting this patch though, update-index is not something a
casual user should use.

> There is "git checkout -m file" but that overwrites the working tree
> file to reproduce the conflicted state, which is not exactly what
> you want.  You only want to reproduce the conflicted state in the
> index, so that you can inspect the (proposed) merge resolution you
> already have in your working tree.
-- 
Duy

Re: [PATCH v1 16/19] read-cache: unlink old sharedindex files

On Sun, Oct 23, 2016 at 4:26 PM, Christian Couder
 wrote:
> +static int can_delete_shared_index(const char *shared_sha1_hex)
> +{
> +   struct stat st;
> +   unsigned long expiration;
> +   const char *shared_index = git_path("sharedindex.%s", 
> shared_sha1_hex);
> +
> +   /* Check timestamp */
> +   expiration = get_shared_index_expire_date();
> +   if (!expiration)
> +   return 0;
> +   if (stat(shared_index, ))
> +   return error_errno("could not stat '%s", shared_index);
> +   if (st.st_mtime > expiration)

I wonder if we should check ctime too, in case mtime is not reliable
(and ctime is less likely to be manipulated by user), just for extra
safety. If (st.st_mtime > expiration || st.st_ctime > expiration).

> +   return 0;
> +
> +   return 1;
> +}
> +
> +static void clean_shared_index_files(const char *current_hex)
> +{
> +   struct dirent *de;
> +   DIR *dir = opendir(get_git_dir());
> +
> +   if (!dir) {
> +   error_errno("unable to open git dir: %s", get_git_dir());

_()

> +   return;

Or just do "return error_errno(...)". The caller can ignore the return
value for now.

> +   }
> +
> +   while ((de = readdir(dir)) != NULL) {
> +   const char *sha1_hex;
> +   if (!skip_prefix(de->d_name, "sharedindex.", _hex))
> +   continue;
> +   if (!strcmp(sha1_hex, current_hex))
> +   continue;

Yeah.. make sure that the shared index linked to $GIT_DIR/index stay,
even if mtime is screwed up. I wonder if we should have the same
treatment for $GIT_DIR/index.lock though, as an extra safety measure.
If you call this function in write_locked_index, then
$GIT_DIR/index.lock is definitely there. Hmm.. maybe it _is_
current_hex, current_hex is not the value from $GIT_DIR/index...

> +   if (can_delete_shared_index(sha1_hex) > 0 &&
> +   unlink(git_path("%s", de->d_name)))
> +   error_errno("unable to unlink: %s", git_path("%s", 
> de->d_name));

_()

>  static int write_shared_index(struct index_state *istate,
> @@ -2211,8 +2269,11 @@ static int write_shared_index(struct index_state 
> *istate,
> }
> ret = rename_tempfile(_sharedindex,
>   git_path("sharedindex.%s", 
> sha1_to_hex(si->base->sha1)));
> -   if (!ret)
> +   if (!ret) {
> hashcpy(si->base_sha1, si->base->sha1);
> +   clean_shared_index_files(sha1_to_hex(si->base->sha1));

This operation is technically garbage collection and should belong to
"git gc --auto", which is already called automatically in a few
places. Is it not called often enough that we need to do the cleaning
up right after a new shared index is created?
-- 
Duy

Re: [PATCH v1 00/19] Add configuration options for split-index

On Sun, Oct 23, 2016 at 4:26 PM, Christian Couder
 wrote:
> Goal
> 
>
> We want to make it possible to use the split-index feature
> automatically by just setting a new "core.splitIndex" configuration
> variable to true.

Thanks. This definitely should help make split index a lot more
convenient for day-to-day use. The series looks ok (well, except the
pruning logic being discussed elsewhere in this thread, but I still
think it's a good strategy).

> This can be valuable as split-index can help significantly speed up
> `git rebase` especially along with the work to libify `git apply`
> that has been recently merged to master
> (see 
> https://github.com/git/git/commit/81358dc238372793b1590efa149cc1581d1fbd98).

I remember this. Since the apply libification work has landed, we can
now think about not writing index out after every apply step, which
gives the same (or more) speed up without the complication of
split-index. But if split-index is good enough for you, we can leave
it for another time.
-- 
Duy

Re: [PATCH v2 2/2] read-cache: make sure file handles are not inherited by child processes

Hi Junio,

On Mon, 24 Oct 2016, Junio C Hamano wrote:

> Eric Wong  writes:
> 
> > larsxschnei...@gmail.com wrote:
> >> +++ b/read-cache.c
> >> @@ -156,7 +156,11 @@ void fill_stat_cache_info(struct cache_entry *ce, 
> >> struct stat *st)
> >>  static int ce_compare_data(const struct cache_entry *ce, struct stat *st)
> >>  {
> >>int match = -1;
> >> -  int fd = open(ce->name, O_RDONLY);
> >> +  int fd = open(ce->name, O_RDONLY | O_CLOEXEC);
> >> +
> >> +  if (O_CLOEXEC && fd < 0 && errno == EINVAL)
> >> +  /* Try again w/o O_CLOEXEC: the kernel might not support it */
> >> +  fd = open(ce->name, O_RDONLY);
> >
> > In the case of O_CLOEXEC != 0 and repeated EINVALs,
> > it'd be good to use something like sha1_file_open_flag as in 1/2
> > so we don't repeatedly hit EINVAL.  Thanks.
> 
> Sounds sane.  
> 
> It's just only once, so perhaps we do not mind a recursion like
> this?
> 
>  read-cache.c | 9 ++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/read-cache.c b/read-cache.c
> index b594865d89..a6978b9321 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -156,11 +156,14 @@ void fill_stat_cache_info(struct cache_entry *ce, 
> struct stat *st)
>  static int ce_compare_data(const struct cache_entry *ce, struct stat *st)
>  {
>   int match = -1;
> - int fd = open(ce->name, O_RDONLY | O_CLOEXEC);
> + static int cloexec = O_CLOEXEC;
> + int fd = open(ce->name, O_RDONLY | cloexec);
>  
> - if (O_CLOEXEC && fd < 0 && errno == EINVAL)
> + if ((cloexec & O_CLOEXEC) && fd < 0 && errno == EINVAL) {
>   /* Try again w/o O_CLOEXEC: the kernel might not support it */
> - fd = open(ce->name, O_RDONLY);
> + cloexec &= ~O_CLOEXEC;
> + return ce_compare_data(ce, st);
> + }
>  

That still looks overly complicated, repeatedly ORing cloexec and
recursing without need. How about this instead?

static int oflags = O_RDONLY | O_CLOEXEC;
int fd = open(ce->name, oflags);

if ((O_CLOEXEC & oflags) && fd < 0 && errno == EINVAL) {
/* Try again w/o O_CLOEXEC: the kernel might not support it */
oflags &= ~O_CLOEXEC;
fd = open(ce->name, oflags);
}

Ciao,
Dscho

Re: [PATCH v2 1/2] sha1_file: open window into packfiles with CLOEXEC

Hi Lars,

On Mon, 24 Oct 2016, larsxschnei...@gmail.com wrote:

> From: Lars Schneider 
> 
> All processes that the Git main process spawns inherit the open file
> descriptors of the main process. These leaked file descriptors can
> cause problems.
> 
> Use the CLOEXEC flag similar to 05d1ed61 to fix the leaked file
> descriptors. Since `git_open_noatime` does not describe the function
> properly anymore rename it to `git_open`.

The patch series may be a little bit more pleasant to read if you renamed
git_open_noatime() to git_open() first, in a separate commit.

> @@ -1598,12 +1598,18 @@ int git_open_noatime(const char *name)
>   if (fd >= 0)
>   return fd;
>  
> - /* Might the failure be due to O_NOATIME? */
> - if (errno != ENOENT && sha1_file_open_flag) {
> - sha1_file_open_flag = 0;
> + /* Try again w/o O_CLOEXEC: the kernel might not support it */
> + if (O_CLOEXEC && errno == EINVAL &&
> + (sha1_file_open_flag & O_CLOEXEC)) {
> + sha1_file_open_flag &= ~O_CLOEXEC;

How about

if ((O_CLOEXEC & sha1_file_open_flag) && errno == EINVAL) {
sha1_file_open_flag &= ~O_CLOEXEC;

instead? It is shorter and should be just as easily optimized out by a
C compiler if O_CLOEXEC was defined as 0.

>   continue;
>   }
>  
> + /* Might the failure be due to O_NOATIME? */
> + if (errno != ENOENT && (sha1_file_open_flag & O_NOATIME)) {
> + sha1_file_open_flag &= ~O_NOATIME;
> + continue;
> + }

I *think* the --patience diff option would have made that patch a little
more obvious.

Otherwise, the patch looks fine to me,
Dscho

Re: [PATCH v1 14/19] read-cache: touch shared index files when used

On Sun, Oct 23, 2016 at 4:26 PM, Christian Couder
 wrote:
> @@ -2268,6 +2268,12 @@ int write_locked_index(struct index_state *istate, 
> struct lock_file *lock,

Doing this in read_index_from() would keep the shared file even more
"fresher" since read happens a lot more often than write. But I think
our main concern is not the temporary index files created by the user
scripts, but $GIT_DIR/index.lock (make sure we don't accidentally
delete its shared file before it gets renamed to $GIT_DIR/index). For
this case, I think refreshing in write_locked_index is enough.

> int ret = write_shared_index(istate, lock, flags);
> if (ret)
> return ret;
> +   } else {
> +   /* Signal that the shared index is used */
> +   const char *shared_index = git_path("sharedindex.%s",
> +   
> sha1_to_hex(si->base_sha1));
> +   if (!check_and_freshen_file(shared_index, 1))
> +   warning("could not freshen '%s'", shared_index);

_()
-- 
Duy

Re: [PATCH v1 10/19] read-cache: regenerate shared index if necessary

On Sun, Oct 23, 2016 at 4:26 PM, Christian Couder
 wrote:
> @@ -2233,7 +2263,8 @@ int write_locked_index(struct index_state *istate, 
> struct lock_file *lock,
> if ((v & 15) < 6)
> istate->cache_changed |= SPLIT_INDEX_ORDERED;
> }
> -   if (istate->cache_changed & SPLIT_INDEX_ORDERED) {
> +   if (istate->cache_changed & SPLIT_INDEX_ORDERED ||
> +   too_many_not_shared_entries(istate)) {

It's probably safer to keep this piece unchanged and add this
somewhere before it

if (too_many_not_shared_entries(istate))
istate->cache_changed |= SPLIT_INDEX_ORDERED;

We could keep cache_changed consistent until the end this way.

> int ret = write_shared_index(istate, lock, flags);
> if (ret)
> return ret;
> diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
> index db8c39f..507a1dd 100755
> --- a/t/t1700-split-index.sh
> +++ b/t/t1700-split-index.sh
> @@ -8,6 +8,7 @@ test_description='split index mode tests'
>  sane_unset GIT_TEST_SPLIT_INDEX
>
>  test_expect_success 'enable split index' '
> +   git config splitIndex.maxPercentChange 100 &&

An alternative name might be splitThreshold. I don't know, maybe
maxPercentChange is better.

> git update-index --split-index &&
> test-dump-split-index .git/index >actual &&
> indexversion=$(test-index-version <.git/index) &&
> --
> 2.10.1.462.g7e1e03a
>



-- 
Duy

Re: [PATCH v1 09/19] config: add git_config_get_max_percent_split_change()

On Sun, Oct 23, 2016 at 4:26 PM, Christian Couder
 wrote:
> This new function will be used in a following commit to get the
> +int git_config_get_max_percent_split_change(void)
> +{
> +   int val = -1;
> +
> +   if (!git_config_get_int("splitindex.maxpercentchange", )) {
> +   if (0 <= val && val <= 100)
> +   return val;
> +
> +   error("splitindex.maxpercentchange value '%d' "

We should keep camelCase form for easy reading. And wrap this string with _().

> + "should be between 0 and 100", val);

I wonder if anybody would try to put 12.3 here and confused by the
error message, because 0 <= 12.3 <= 100, but it's not an integer..
Ah.. never mind, die_bad_number() would be called first in this case
with a loud and clear complaint.

> +   return -1;
> +   }
> +
> +   return -1; /* default value */
-- 
Duy

Re: [PATCH v1 05/19] update-index: warn in case of split-index incoherency

On Sun, Oct 23, 2016 at 4:26 PM, Christian Couder
 wrote:
> When users are using `git update-index --(no-)split-index`, they
> may expect the split-index feature to be used or not according to
> the option they just used, but this might not be the case if the
> new "core.splitIndex" config variable has been set. In this case
> let's warn about what will happen and why.
>
> Signed-off-by: Christian Couder 
> ---
>  builtin/update-index.c | 11 ++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/update-index.c b/builtin/update-index.c
> index b75ea03..a14dbf2 100644
> --- a/builtin/update-index.c
> +++ b/builtin/update-index.c
> @@ -1098,12 +1098,21 @@ int cmd_update_index(int argc, const char **argv, 
> const char *prefix)
> }
>
> if (split_index > 0) {
> +   if (git_config_get_split_index() == 0)
> +   warning("core.splitIndex is set to false; "
> +   "remove or change it, if you really want to "
> +   "enable split index");

Wrap this string and the one below with _() so they can be translated.

> if (the_index.split_index)
> the_index.cache_changed |= SPLIT_INDEX_ORDERED;
> else
> add_split_index(_index);
> -   } else if (!split_index)
> +   } else if (!split_index) {
> +   if (git_config_get_split_index() == 1)
> +   warning("core.splitIndex is set to true; "
> +   "remove or change it, if you really want to "
> +   "disable split index");
> remove_split_index(_index);
> +   }
>
> switch (untracked_cache) {
> case UC_UNSPECIFIED:
> --
> 2.10.1.462.g7e1e03a
-- 
Duy

Re: [PATCH v1 03/19] split-index: add {add,remove}_split_index() functions

On Sun, Oct 23, 2016 at 4:26 PM, Christian Couder
 wrote:
> +void remove_split_index(struct index_state *istate)
> +{
> +   if (istate->split_index) {
> +   /*
> +* can't discard_split_index(_index); because that
> +* will destroy split_index->base->cache[], which may
> +* be shared with the_index.cache[]. So yeah we're
> +* leaking a bit here.

In the context of update-index, this is a one-time thing and leaking
is tolerable. But because it becomes a library function now, this leak
can become more serious, I think.

The only other (indirect) caller is read_index_from() so probably not
bad most of the time (we read at the beginning of a command only).
sequencer.c may discard and re-read the index many times though,
leaking could be visible there.

> +*/
> +   istate->split_index = NULL;
> +   istate->cache_changed |= SOMETHING_CHANGED;
> +   }
> +}
-- 
Duy

Re: [PATCH 0/4] nd/ita-empty-commit update

On Tue, Oct 25, 2016 at 12:58 AM, Junio C Hamano  wrote:
>> The name ita-invisible-in-index is not perfect but I could not think
>> of any better. Another name could be diff-cached-ignores-ita, but
>> that's just half of what it does. The other half is 
>> diff-files-includes-ita...
>
> I can't either, and it is one of the reasons why I am reluctant.
> Not being able to be named with a short-and-sweet name often is a
> sign that the thing to be named is conceptually not well thought
> out.

It's implementation detail leak, and probably why naming it for
"normal" people is so hard. Whatever the name is must somehow imply
"so these i-t-a markers actually live in the index and considered real
index entries, associated to empty blob, most of the time..."

> But as we need to give it some name to the flat to ease
> experimenting, let's take that name as-is.
-- 
Duy

Re: [PATCH v1 00/19] Add configuration options for split-index

On Tue, Oct 25, 2016 at 1:07 AM, Junio C Hamano  wrote:
>> - splitIndex.sharedIndexExpire
>>
>> To make sure that old sharedindex files are eventually removed
>> when a new one has been created, we "touch" the shared index file
>> every time it is used by a new split index file. Then we can
>> delete shared indexes with an mtime older than one week (by
>> default), when we create a new shared index file. The new
>> "splitIndex.sharedIndexExpire" config option lets people tweak
>> this grace period.
>
> I do not quite understand this justification.  Doesn't each of the
> "this hold only changes since the base index file" files have a
> backpointer that names the base index file it is a delta against?

Yes, but the shared file does not have pointers to all the files that
need it, which could be more than one. We know one of them,
$GIT_DIR/index, and possibly $GIT_DIR/index.lock too. But those files
people generate manually and refer to them with $GIT_INDEX_FILE, we
can't know where they are.

> Is it safe to remove a base index file when there is still a split
> index file that points at it?
>
> IOW, I do not see why it can be safe for the expiration decision to
> be based on timestamp (I would understand it if it were based on a
> refcnt, though).

Problem is we can't maintain these ref counts cheap and simple. We
don't want to update sharedindex file every time somebody references
to it (or stops referencing to it) because that defeats the purpose of
splitting it out and not touching it any more. Adding a separate file
for ref count could work, but it gets complex, especially when we
think about race condition at update time.

Timestamps allow us to say, ok this base index file has not been read
by anybody for N+ hours (or better, days), it's most likely not
referenced by any temporary index files (including
$GIT_DIR/index.lock) anymore because those files, by the definition of
"temporary", must be gone by now. We should definitely check and make
sure the file $GIT_DIR/index points to still exist. I'm going to read
the series now, so I don't know if the previous sentence is true.

It will probably be harder to handle race condition at updating
$GIT_DIR/index, which could be avoided by a sufficiently long grace
period with timestamps.
-- 
Duy

Re: Reporting Bug in Git Version Control System

2016-10-25 Thread Pranit Bauva

Hey Yash,

Junio has explained the problem very well. Since you seem to be a
beginner (guessing purely by your email) I will tell you how to fix
it.

Remember when you would have first installed git, you would have done
something like `git config --global user.name ` and
`git config --global user.email `, it gets
permanently stored in the git configuration file (~/.gitconfig). Now
all the commits in git are made with this name and email. If you want
to change this, again run the above commands with your new name and
email. After that commits will be done with a different name and
email. Hope this helps! :)

Regards,
Pranit Bauva

Re: [PATCH] Allow stashes to be referenced by index only

2016-10-25 Thread Aaron and Ashley Watson

On Tue, Oct 25, 2016 at 4:11 AM, Jeff King  wrote:
> On Mon, Oct 24, 2016 at 07:40:13PM -0400, Aaron M Watson wrote:
>
>> Instead of referencing "stash@{n}" explicitly, it can simply be
>> referenced as "n".  Most users only reference stashes by their position
>> in the stash stask (what I refer to as the "index"). The syntax for the
>> typical stash (stash@{n}) is slightly annoying and easy to forget, and
>> sometimes difficult to escape properly in a script. Because of this the
>> capability to do things with the stash by simply referencing the index
>> is desirable.
>>
>> This patch includes the superior implementation provided by Øsse Walle
>> (thanks for that), with a slight change to fix a broken test in the test
>> suite. I also merged the test scripts as suggested by Jeff King, and
>> un-wrapped the documentation as suggested by Junio Hamano.
>>
>> Signed-off-by: Aaron M Watson 
>> ---
>
> Thanks, this version looks good to me.
>
> Oddly, it does not seem to apply. I get:
>
>   $ git am -3 ~/patch
>   Applying: Allow stashes to be referenced by index only
>   Using index info to reconstruct a base tree...
>   M   git-stash.sh
>   error: patch failed: t/t3903-stash.sh:604
>   error: t/t3903-stash.sh: patch does not apply
>   error: Did you hand edit your patch?
>   It does not apply to blobs recorded in its index.
>   Patch failed at 0001 Allow stashes to be referenced by index only
>
> The culprit seems to be the final hunk header:
>
>> @@ -604,7 +624,21 @@ test_expect_success 'invalid ref of the form stash@{n}, 
>> n >= N' '
>
> This should be "604,6", as there are 6 context lines, and your patch
> does not remove any lines.
>
> I suspect the maintainer can fix it up while applying, but for my
> curiosity: did you hand-edit it, or is there a potential bug in git's
> diff code?

I did indeed edit the patch by hand (I forgot to remove the spaces
after the > in the test file), but
the bug appears to be in emacs's diff-mode, not in git.

>
> -Peff



-- 
Aaron and Ashley Watson

Re: [PATCH] Allow stashes to be referenced by index only