Your email box account needs to be upgraded now

[Suomu, Anu]

EMAIL UPGRADE NOTIFICATION

Your email box account needs to be upgraded 
now to our latest version of Microsoft Outlook Account For better performance. 
If not Upgraded your email box account will be suspended now.


Microsoft Verification Team

Microsoft outlook © Inc 2018.

Votre compte de boîte aux lettres doit maintenant être mis à niveau

[Suomu, Anu]

NOTIFICATION DE MISE À NIVEAU DE COURRIEL

Votre compte de boîte aux lettres doit maintenant être mis à 
niveau vers notre dernière version du compte 
Microsoft Outlook pour de meilleures performances. Si non mis à niveau, votre 
compte de boîte aux lettres sera suspendu maintenant.


Équipe de vérification Microsoft

Microsoft Outlook © Inc. 2018.

RE: [PATCH v3 1/3] reset: don't compute unstaged changes after reset when --quiet

[Johannes Schindelin]

Hi Ben,

On Mon, 22 Oct 2018, Ben Peart wrote:

> > From: Johannes Schindelin 
> > 
> > On Mon, 22 Oct 2018, Ben Peart wrote:
> > 
> > > When git reset is run with the --quiet flag, don't bother finding
> > > any additional unstaged changes as they won't be output anyway.
> > > This speeds up the git reset command by avoiding having to lstat()
> > > every file looking for changes that aren't going to be reported
> > > anyway.
> > >
> > > The savings can be significant.  In a repo with 200K files "git
> > > reset" drops from 7.16 seconds to 0.32 seconds for a savings of 96%.
> > 
> > That's very nice!
> > 
> > Those numbers, just out of curiosity, are they on Windows? Or on
> > Linux?
> > 
> 
> It's safe to assume all my numbers are on Windows. :-)

Excellent. These speed-ups will really help our users.

Thanks!
Dscho

Re: [PATCH v1 2/2] reset: add new reset.quietDefault config setting

[Ævar Arnfjörð Bjarmason]

On Wed, Oct 17 2018, Jeff King wrote:

> On Wed, Oct 17, 2018 at 02:19:59PM -0400, Eric Sunshine wrote:
>
>> On Wed, Oct 17, 2018 at 12:40 PM Ben Peart  wrote:
>> > Add a reset.quietDefault config setting that sets the default value of the
>> > --quiet flag when running the reset command.  This enables users to change
>> > the default behavior to take advantage of the performance advantages of
>> > avoiding the scan for unstaged changes after reset.  Defaults to false.
>>
>> As with the previous patch, my knee-jerk reaction is that this really
>> feels wrong being tied to --quiet. It's particularly unintuitive.
>>
>> What I _could_ see, and what would feel more natural is if you add a
>> new option (say, --optimize) which is more general, incorporating
>> whatever optimizations become available in the future, not just this
>> one special-case. A side-effect of --optimize is that it implies
>> --quiet, and that is something which can and should be documented.
>
> Heh, I just wrote something very similar elsewhere in the thread. I'm
> still not sure if it's a dumb idea, but at least we can be dumb
> together.

Same here. I'm in general if favor of having the ability to configure
porcelain command-line options, but in this case it seems like it would
be more logical to head for something like:

core.uiMessaging=[default,exhaustive,lossyButFaster,quiet]

Where default would be our current "exhaustive", and this --quiet case
would be covered by lossyButFaster, but also things like the
"--no-ahead-behind" flag for git-status.

Just on this implementation: The usual idiom for flags as config is
command.flag=xyz, not command.flagDefault=xyz, so this should be
reset.quiet.

Re: [PATCH] Poison gettext with the Ook language

[Johannes Schindelin]

Hi Ævar,

On Mon, 22 Oct 2018, Ævar Arnfjörð Bjarmason wrote:

> So I think the only reason to keep it [GETTEXT_POISON] compile-time is
> performance, but I don't think that matters. It's not like we're
> printing gigabytes of _() formatted output. Everything where formatting
> matters is plumbing which doesn't use this API. These messages are
> always for human consumption.

Well, let's make sure that your impression is correct before going too
far. I, too, had the impression that gettext cannot possibly be expensive,
especifally in Git for Windows' settings, where we do not even ship
translations. Yet see the commit message of cc5e1bf99247 (gettext: avoid
initialization if the locale dir is not present, 2018-04-21):

The runtime of a simple `git.exe version` call on Windows is
currently dominated by the gettext setup, adding a whopping ~150ms
to the ~210ms total.

I would be in favor of your change to make this a runtime option, of
course, as long as it does not affect performance greatly (in particular
on Windows, where we fight an uphill battle to make Git faster).

Ciao,
Dscho

Re: [PATCH v2 3/3] rebase (autostash): use an explicit OID to apply the stash

[Johannes Schindelin]

Hi,

On Tue, 23 Oct 2018, Junio C Hamano wrote:

> SZEDER Gábor  writes:
> 
> >> To prevent that from happening, let's append `^0` after the stash hash,
> >> to make sure that it is interpreted as an OID rather than as a number.
> >
> > Oh, this is clever.
> 
> Yeah, we can do this as we know we'd be dealing with a commit-ish.
> If we made a mistake to use a tree object to represent a stash, this
> trick wouldn't have been possible ;-)
> 
> > FWIW, all patches look good to me, barring the typo below.
> > ...
> >> +  /* Ensure that the hash is not mistake for a number */
> >
> > s/mistake/mistaken/
> 
> Will squash this in and add your reviewed-by before queuing.
> 
> Thanks, both.

Thank you all!
Dscho

Re: New semantic patches vs. in-flight topics [was: Re: [PATCH 00/19] Bring more repository handles into our code base]

[Junio C Hamano]

Junio C Hamano  writes:

> I actually think this round does a far nicer job playing well with
> other topics than any earlier series.  The pain you are observing I
> think come primarily from my not making the best use of these
> patches.
>
> Steppng back a bit, I'd imagine in an ideal world where "make
> coccicheck" can be done instantaneously _and_ the spatch machinery
> is just as reliable as C compilers
>
> What I _could_ do (and what I did do only for one round of pushing
> out 'pu') is to queue a coccinelle transformation at the tip of
> integration branches.  If "make coccicheck" ran in subsecond, I
> could even automate it in the script that is used to rebuild 'pu'
> every day, ...

Anyway, even though "make coccicheck" does not run in subsecond,
I've updated my machinery to rebuild the integration branches so
that I can optionally queue generated coccicheck patches, and what I
pushed out tonight has one at the tip of 'pu' and also another at
the tip of 'next'.  The latter seems to be passing all archs and
executing Windows run.

The tip of 'pu' has trouble with -Wunused on Apple around the
delta-islands series.


next: https://travis-ci.org/git/git/builds/444969406
pu:   https://travis-ci.org/git/git/builds/444969342

[PATCH] test: avoid failures when USE_NED_ALLOCATOR

[Carlo Marcelo Arenas Belón]

contrib/nedmalloc doesn't support MALLOC_CHECK_ or MALLOC_PERTURB_
so add it to the same exception that is being used with valgrind

Signed-off-by: Carlo Marcelo Arenas Belón 
---
 t/test-lib.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 44288cbb59..2ad9e6176c 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -143,7 +143,7 @@ fi
 # Add libc MALLOC and MALLOC_PERTURB test
 # only if we are not executing the test with valgrind
 if expr " $GIT_TEST_OPTS " : ".* --valgrind " >/dev/null ||
-   test -n "$TEST_NO_MALLOC_CHECK"
+   test -n "$TEST_NO_MALLOC_CHECK" || test -n "$USE_NED_ALLOCATOR"
 then
setup_malloc_check () {
: nothing
-- 
2.19.1

Re: [PATCH] compat: make sure git_mmap is not expected to write

[Johannes Schindelin]

Hi carlo,

your mail's "From:" line does not record your full name, but the
Signed-off-by: line does. Let's use the latter?

On Mon, 22 Oct 2018, carlo wrote:

> in f48000fc ("Yank writing-back support from gitfakemmap.", 2005-10-08)
> support for writting back changes was removed but the specific prot
> flag that would be used was not checked for)
> 
> Signed-off-by: Carlo Marcelo Arenas Belón 

ACK!

Thank you,
Johannes

> ---
>  compat/mmap.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/compat/mmap.c b/compat/mmap.c
> index 7f662fef7b..14d31010df 100644
> --- a/compat/mmap.c
> +++ b/compat/mmap.c
> @@ -4,7 +4,7 @@ void *git_mmap(void *start, size_t length, int prot, int 
> flags, int fd, off_t of
>  {
>   size_t n = 0;
>  
> - if (start != NULL || !(flags & MAP_PRIVATE))
> + if (start != NULL || flags != MAP_PRIVATE || prot != PROT_READ)
>   die("Invalid usage of mmap when built with NO_MMAP");
>  
>   start = xmalloc(length);
> -- 
> 2.19.1
> 
> 

Re: New semantic patches vs. in-flight topics [was: Re: [PATCH 00/19] Bring more repository handles into our code base]

[Carlo Arenas]

On Tue, Oct 23, 2018 at 2:40 AM Junio C Hamano  wrote:
>
> The tip of 'pu' has trouble with -Wunused on Apple around the
> delta-islands series.

FWIW the "problem" is actually with -Wunused-function and is AFAIK not
related to the semantic changes or Apple (AKA macOS)

Indeed, I saw this issue before also in Linux (Fedora Rawhide) when
using the latest clang and presumed it was just expected because the
topic branch was most likely incomplete.

Carlo

Re: [PATCH v3 0/3] repack -ad: fix after fetch --prune in a shallow repository

[Johannes Schindelin]

Hi,

On Mon, 22 Oct 2018, Johannes Schindelin via GitGitGadget wrote:

> Under certain circumstances, commits that were reachable can be made
> unreachable, e.g. via git fetch --prune. These commits might have been
> packed already, in which case git repack -adlf will just drop them without
> giving them the usual grace period before an eventual git prune (via git gc)
> prunes them.
> 
> This is a problem when the shallow file still lists them, which is the
> reason why git prune edits that file. And with the proposed changes, git
> repack -ad will now do the same.
> 
> Reported by Alejandro Pauly.
> 
> Changes since v2:
> 
>  * Fixed a typo in the last commit message.
>  * Added an explanation to the last commit message why we do not simply skip
>non-existing shallow commits at fetch time.
>  * Introduced a new, "quick prune" mode where prune_shallow() does not try
>to drop unreachable commits, but only non-existing ones.

BTW this was supposed to address Peff's concern that the SEEN flag would
not be marking all reachable shallow commits at the time when
`cmd_repack()` calls `prune_shallow()`, i.e. the last concern about this
stop-gap patch series.

Ciao,
Dscho

>  * Rebased to current master because there were too many merge conflicts for
>my liking otherwise.
> 
> Changes since v1:
> 
>  * Also trigger prune_shallow() when --unpack-unreachable= was
>passed to git repack.
>  * No need to trigger prune_shallow() when git repack was called with -k.
> 
> Johannes Schindelin (3):
>   repack: point out a bug handling stale shallow info
>   shallow: offer to prune only non-existing entries
>   repack -ad: prune the list of shallow commits
> 
>  builtin/prune.c  |  2 +-
>  builtin/repack.c |  6 ++
>  commit.h |  2 +-
>  shallow.c| 22 +-
>  t/t5537-fetch-shallow.sh | 27 +++
>  5 files changed, 52 insertions(+), 7 deletions(-)
> 
> 
> base-commit: c4df23f7927d8d00e666a3c8d1b3375f1dc8a3c1
> Published-As: 
> https://github.com/gitgitgadget/git/releases/tags/pr-9%2Fdscho%2Fshallow-and-fetch-prune-v3
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git 
> pr-9/dscho/shallow-and-fetch-prune-v3
> Pull-Request: https://github.com/gitgitgadget/git/pull/9
> 
> Range-diff vs v2:
> 
>  1:  d2be40131 ! 1:  ed8559b91 repack: point out a bug handling stale shallow 
> info
>  @@ -48,4 +48,6 @@
>   +   origin "+refs/heads/*:refs/remotes/origin/*"
>   +'
>   +
>  - test_done
>  + . "$TEST_DIRECTORY"/lib-httpd.sh
>  + start_httpd
>  + 
>  -:  - > 2:  f085eb4f7 shallow: offer to prune only non-existing 
> entries
>  2:  c7ee6e008 ! 3:  1f9ff57d5 repack -ad: prune the list of shallow commits
>  @@ -2,15 +2,19 @@
>   
>   repack -ad: prune the list of shallow commits
>   
>  -While it is true that we never add unreachable commits into pack 
> files
>  -intentionally (as `git repack`'s documentation states), we must not
>  -forget that a `git fetch --prune` (or even a `git fetch` when a ref 
> was
>  +`git repack` can drop unreachable commits without further warning,
>  +making the corresponding entries in `.git/shallow` invalid, which 
> causes
>  +serious problems when deepening the branches.
>  +
>  +One scenario where unreachable commits are dropped by `git repack` 
> is
>  +when a `git fetch --prune` (or even a `git fetch` when a ref was
>   force-pushed in the meantime) can make a commit unreachable that was
>   reachable before.
>   
>   Therefore it is not safe to assume that a `git repack -adlf` will 
> keep
>   unreachable commits alone (under the assumption that they had not 
> been
>  -packed in the first place).
>  +packed in the first place, which is an assumption at least some of 
> Git's
>  +code seems to make).
>   
>   This is particularly important to keep in mind when looking at the
>   `.git/shallow` file: if any commits listed in that file become
>  @@ -23,8 +27,16 @@
>   To avoid this problem, let's prune the shallow list in `git repack` 
> when
>   the `-d` option is passed, unless `-A` is passed, too (which would 
> force
>   the now-unreachable objects to be turned into loose objects instead 
> of
>  -being deleted). Additionally, e also need to take 
> `--keep-reachable` and
>  -`--unpack-unreachable=` into account.
>  +being deleted). Additionally, we also need to take 
> `--keep-reachable`
>  +and `--unpack-unreachable=` into account.
>  +
>  +Note: an alternative solution discussed during the review of this 
> patch
>  +was to teach `git fetch` to simply ignore entries in .git/shallow 
> if the
>  +corresponding commits do not exist locally. A quick test, however,
>  +reveale

Re: [PATCH] Poison gettext with the Ook language

[Ævar Arnfjörð Bjarmason]

On Tue, Oct 23 2018, Johannes Schindelin wrote:

> Hi Ævar,
>
> On Mon, 22 Oct 2018, Ævar Arnfjörð Bjarmason wrote:
>
>> So I think the only reason to keep it [GETTEXT_POISON] compile-time is
>> performance, but I don't think that matters. It's not like we're
>> printing gigabytes of _() formatted output. Everything where formatting
>> matters is plumbing which doesn't use this API. These messages are
>> always for human consumption.
>
> Well, let's make sure that your impression is correct before going too
> far. I, too, had the impression that gettext cannot possibly be expensive,
> especifally in Git for Windows' settings, where we do not even ship
> translations. Yet see the commit message of cc5e1bf99247 (gettext: avoid
> initialization if the locale dir is not present, 2018-04-21):
>
>   The runtime of a simple `git.exe version` call on Windows is
>   currently dominated by the gettext setup, adding a whopping ~150ms
>   to the ~210ms total.
>
> I would be in favor of your change to make this a runtime option, of
> course, as long as it does not affect performance greatly (in particular
> on Windows, where we fight an uphill battle to make Git faster).

How expensive gettext() may or may not be isn't relevant to the
GETTEXT_POISON compile-time option.

The effect of what I'm suggesting here, and which my WIP patch in
<875zxtd59e@evledraar.gmail.com> implements is that we'd do a
one-time getenv() for each process that prints a _() message that we
aren't doing now, and for each message call a function that would check
a boolean "are we in poison mode" static global variable.

Perhaps some of that's expensive on Windows, but given the recent
patches by Microsoft employees to add GIT_TEST_* env options I assumed
not, but in any case it won't have anything to do with how expensive
gettext may or may not be, you'll already be paying that cost now (or
not, with NO_GETTEXT).

Re: New semantic patches vs. in-flight topics [was: Re: [PATCH 00/19] Bring more repository handles into our code base]

[Junio C Hamano]

Carlo Arenas  writes:

> On Tue, Oct 23, 2018 at 2:40 AM Junio C Hamano  wrote:
>>
>> The tip of 'pu' has trouble with -Wunused on Apple around the
>> delta-islands series.
>
> FWIW the "problem" is actually with -Wunused-function and is AFAIK not
> related to the semantic changes or Apple (AKA macOS)

Oh, don't get me wrong.  By Apple, I meant "the versions of compiler
used on the Apple build at TravisCI site".  I could have sent the
above two lines in a separate topic, as the issue does not have
anything to do with "new semantic patches" discussion, but I thought
it would be obvious to everybody who is reading the thread---it
seems I thought wrong ;-)

[PATCH 0/3] Use nanosecond-precision file times on Windows

[Johannes Schindelin via GitGitGadget]

This is yet another patch series in the slow wave of patches coming over
from Git for Windows.

With this change, we now use preciser timestamps to determine e.g. whether
the Git index is out of date. This change made it into Git for Windows
already in version 2.6.0, i.e. for a little over three years.

Please note that this change originally caused a lot of trouble, as e.g.
libgit2 was unaware of our plans and used second-precision file times. So if
you used Git for Windows as well as a libgit2-based program to, say, update
the Git index, there would be a back-and-forth between index updates with
and without the fractional second parts, causing quite a bit of bad
performance.

These issues have been ironed out long ago, though, so it is high time to
contribute these patches to core Git.

Johannes Schindelin (1):
  mingw: factor out code to set stat() data

Karsten Blees (2):
  mingw: replace MSVCRT's fstat() with a Win32-based implementation
  mingw: implement nanosecond-precision file times

 compat/mingw.c   | 76 +++-
 compat/mingw.h   | 36 ---
 config.mak.uname |  2 --
 3 files changed, 76 insertions(+), 38 deletions(-)


base-commit: c4df23f7927d8d00e666a3c8d1b3375f1dc8a3c1
Published-As: 
https://github.com/gitgitgadget/git/releases/tags/pr-53%2Fdscho%2Fnanosecond-file-times-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git 
pr-53/dscho/nanosecond-file-times-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/53
-- 
gitgitgadget

[PATCH 1/3] mingw: factor out code to set stat() data

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin 

In our fstat() emulation, we convert the file metadata from Win32 data
structures to an emulated POSIX structure.

To structure the code better, let's factor that part out into its own
function.

Note: it would be tempting to try to unify this code with the part of
do_lstat() that does the same thing, but they operate on different data
structures: BY_HANDLE_FILE_INFORMATION vs WIN32_FILE_ATTRIBUTE_DATA. So
unfortunately, they cannot be unified.

Signed-off-by: Johannes Schindelin 
---
 compat/mingw.c | 39 +--
 1 file changed, 25 insertions(+), 14 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 18caf2196..d2e7d86db 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -736,6 +736,29 @@ static int do_stat_internal(int follow, const char 
*file_name, struct stat *buf)
return do_lstat(follow, alt_name, buf);
 }
 
+static int get_file_info_by_handle(HANDLE hnd, struct stat *buf)
+{
+   BY_HANDLE_FILE_INFORMATION fdata;
+
+   if (!GetFileInformationByHandle(hnd, &fdata)) {
+   errno = err_win_to_posix(GetLastError());
+   return -1;
+   }
+
+   buf->st_ino = 0;
+   buf->st_gid = 0;
+   buf->st_uid = 0;
+   buf->st_nlink = 1;
+   buf->st_mode = file_attr_to_st_mode(fdata.dwFileAttributes);
+   buf->st_size = fdata.nFileSizeLow |
+   (((off_t)fdata.nFileSizeHigh)<<32);
+   buf->st_dev = buf->st_rdev = 0; /* not used by Git */
+   buf->st_atime = filetime_to_time_t(&(fdata.ftLastAccessTime));
+   buf->st_mtime = filetime_to_time_t(&(fdata.ftLastWriteTime));
+   buf->st_ctime = filetime_to_time_t(&(fdata.ftCreationTime));
+   return 0;
+}
+
 int mingw_lstat(const char *file_name, struct stat *buf)
 {
return do_stat_internal(0, file_name, buf);
@@ -748,7 +771,6 @@ int mingw_stat(const char *file_name, struct stat *buf)
 int mingw_fstat(int fd, struct stat *buf)
 {
HANDLE fh = (HANDLE)_get_osfhandle(fd);
-   BY_HANDLE_FILE_INFORMATION fdata;
 
if (fh == INVALID_HANDLE_VALUE) {
errno = EBADF;
@@ -758,20 +780,9 @@ int mingw_fstat(int fd, struct stat *buf)
if (GetFileType(fh) != FILE_TYPE_DISK)
return _fstati64(fd, buf);
 
-   if (GetFileInformationByHandle(fh, &fdata)) {
-   buf->st_ino = 0;
-   buf->st_gid = 0;
-   buf->st_uid = 0;
-   buf->st_nlink = 1;
-   buf->st_mode = file_attr_to_st_mode(fdata.dwFileAttributes);
-   buf->st_size = fdata.nFileSizeLow |
-   (((off_t)fdata.nFileSizeHigh)<<32);
-   buf->st_dev = buf->st_rdev = 0; /* not used by Git */
-   buf->st_atime = filetime_to_time_t(&(fdata.ftLastAccessTime));
-   buf->st_mtime = filetime_to_time_t(&(fdata.ftLastWriteTime));
-   buf->st_ctime = filetime_to_time_t(&(fdata.ftCreationTime));
+   if (!get_file_info_by_handle(fh, buf))
return 0;
-   }
+
errno = EBADF;
return -1;
 }
-- 
gitgitgadget

[PATCH 2/3] mingw: replace MSVCRT's fstat() with a Win32-based implementation

[Karsten Blees via GitGitGadget]

From: Karsten Blees 

fstat() is the only stat-related CRT function for which we don't have a
full replacement yet (and thus the only reason to stick with MSVCRT's
'struct stat' definition).

Fully implement fstat(), in preparation of implementing a POSIX 2013
compatible 'struct stat' with nanosecond-precision file times.

This allows us also to implement some clever code to handle pipes and
character devices in our own way.

Signed-off-by: Karsten Blees 
Signed-off-by: Johannes Schindelin 
---
 compat/mingw.c | 31 +--
 1 file changed, 21 insertions(+), 10 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index d2e7d86db..07fc0b79a 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -771,20 +771,31 @@ int mingw_stat(const char *file_name, struct stat *buf)
 int mingw_fstat(int fd, struct stat *buf)
 {
HANDLE fh = (HANDLE)_get_osfhandle(fd);
+   DWORD avail, type = GetFileType(fh) & ~FILE_TYPE_REMOTE;
 
-   if (fh == INVALID_HANDLE_VALUE) {
-   errno = EBADF;
-   return -1;
-   }
-   /* direct non-file handles to MS's fstat() */
-   if (GetFileType(fh) != FILE_TYPE_DISK)
-   return _fstati64(fd, buf);
+   switch (type) {
+   case FILE_TYPE_DISK:
+   return get_file_info_by_handle(fh, buf);
 
-   if (!get_file_info_by_handle(fh, buf))
+   case FILE_TYPE_CHAR:
+   case FILE_TYPE_PIPE:
+   /* initialize stat fields */
+   memset(buf, 0, sizeof(*buf));
+   buf->st_nlink = 1;
+
+   if (type == FILE_TYPE_CHAR) {
+   buf->st_mode = _S_IFCHR;
+   } else {
+   buf->st_mode = _S_IFIFO;
+   if (PeekNamedPipe(fh, NULL, 0, NULL, &avail, NULL))
+   buf->st_size = avail;
+   }
return 0;
 
-   errno = EBADF;
-   return -1;
+   default:
+   errno = EBADF;
+   return -1;
+   }
 }
 
 static inline void time_t_to_filetime(time_t t, FILETIME *ft)
-- 
gitgitgadget

[PATCH 3/3] mingw: implement nanosecond-precision file times

[Karsten Blees via GitGitGadget]

From: Karsten Blees 

We no longer use any of MSVCRT's stat-functions, so there's no need to
stick to a CRT-compatible 'struct stat' either.

Define and use our own POSIX-2013-compatible 'struct stat' with nanosecond-
precision file times.

Note: This can cause performance issues when using Git variants with
different file time resolutions, as the timestamps are stored in the Git
index: after updating the index with a Git variant that uses
second-precision file times, a nanosecond-aware Git will think that
pretty much every single file listed in the index is out of date.

Signed-off-by: Karsten Blees 
Signed-off-by: Johannes Schindelin 
---
 compat/mingw.c   | 18 ++
 compat/mingw.h   | 36 ++--
 config.mak.uname |  2 --
 3 files changed, 36 insertions(+), 20 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 07fc0b79a..26016d02e 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -592,9 +592,11 @@ static inline long long filetime_to_hnsec(const FILETIME 
*ft)
return winTime - 1164447360LL;
 }
 
-static inline time_t filetime_to_time_t(const FILETIME *ft)
+static inline void filetime_to_timespec(const FILETIME *ft, struct timespec 
*ts)
 {
-   return (time_t)(filetime_to_hnsec(ft) / 1000);
+   long long hnsec = filetime_to_hnsec(ft);
+   ts->tv_sec = (time_t)(hnsec / 1000);
+   ts->tv_nsec = (hnsec % 1000) * 100;
 }
 
 /**
@@ -653,9 +655,9 @@ static int do_lstat(int follow, const char *file_name, 
struct stat *buf)
buf->st_size = fdata.nFileSizeLow |
(((off_t)fdata.nFileSizeHigh)<<32);
buf->st_dev = buf->st_rdev = 0; /* not used by Git */
-   buf->st_atime = filetime_to_time_t(&(fdata.ftLastAccessTime));
-   buf->st_mtime = filetime_to_time_t(&(fdata.ftLastWriteTime));
-   buf->st_ctime = filetime_to_time_t(&(fdata.ftCreationTime));
+   filetime_to_timespec(&(fdata.ftLastAccessTime), 
&(buf->st_atim));
+   filetime_to_timespec(&(fdata.ftLastWriteTime), &(buf->st_mtim));
+   filetime_to_timespec(&(fdata.ftCreationTime), &(buf->st_ctim));
if (fdata.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT) {
WIN32_FIND_DATAW findbuf;
HANDLE handle = FindFirstFileW(wfilename, &findbuf);
@@ -753,9 +755,9 @@ static int get_file_info_by_handle(HANDLE hnd, struct stat 
*buf)
buf->st_size = fdata.nFileSizeLow |
(((off_t)fdata.nFileSizeHigh)<<32);
buf->st_dev = buf->st_rdev = 0; /* not used by Git */
-   buf->st_atime = filetime_to_time_t(&(fdata.ftLastAccessTime));
-   buf->st_mtime = filetime_to_time_t(&(fdata.ftLastWriteTime));
-   buf->st_ctime = filetime_to_time_t(&(fdata.ftCreationTime));
+   filetime_to_timespec(&(fdata.ftLastAccessTime), &(buf->st_atim));
+   filetime_to_timespec(&(fdata.ftLastWriteTime), &(buf->st_mtim));
+   filetime_to_timespec(&(fdata.ftCreationTime), &(buf->st_ctim));
return 0;
 }
 
diff --git a/compat/mingw.h b/compat/mingw.h
index 571019d0b..9419b27e1 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -327,18 +327,41 @@ static inline int getrlimit(int resource, struct rlimit 
*rlp)
 }
 
 /*
- * Use mingw specific stat()/lstat()/fstat() implementations on Windows.
+ * Use mingw specific stat()/lstat()/fstat() implementations on Windows,
+ * including our own struct stat with 64 bit st_size and nanosecond-precision
+ * file times.
  */
 #ifndef __MINGW64_VERSION_MAJOR
 #define off_t off64_t
 #define lseek _lseeki64
+struct timespec {
+   time_t tv_sec;
+   long tv_nsec;
+};
 #endif
 
-/* use struct stat with 64 bit st_size */
+struct mingw_stat {
+_dev_t st_dev;
+_ino_t st_ino;
+_mode_t st_mode;
+short st_nlink;
+short st_uid;
+short st_gid;
+_dev_t st_rdev;
+off64_t st_size;
+struct timespec st_atim;
+struct timespec st_mtim;
+struct timespec st_ctim;
+};
+
+#define st_atime st_atim.tv_sec
+#define st_mtime st_mtim.tv_sec
+#define st_ctime st_ctim.tv_sec
+
 #ifdef stat
 #undef stat
 #endif
-#define stat _stati64
+#define stat mingw_stat
 int mingw_lstat(const char *file_name, struct stat *buf);
 int mingw_stat(const char *file_name, struct stat *buf);
 int mingw_fstat(int fd, struct stat *buf);
@@ -351,13 +374,6 @@ int mingw_fstat(int fd, struct stat *buf);
 #endif
 #define lstat mingw_lstat
 
-#ifndef _stati64
-# define _stati64(x,y) mingw_stat(x,y)
-#elif defined (_USE_32BIT_TIME_T)
-# define _stat32i64(x,y) mingw_stat(x,y)
-#else
-# define _stat64(x,y) mingw_stat(x,y)
-#endif
 
 int mingw_utime(const char *file_name, const struct utimbuf *times);
 #define utime mingw_utime
diff --git a/config.mak.uname b/config.mak.uname
index 8acdeb71f..f179d7a1d 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -370,7 +370,6 @@ ifeq ($(uname_S),Windows)
RUNTIME_PREFIX = YesPlease
   

[PATCH 0/1] Load system libraries the recommended way on Windows

[Johannes Schindelin via GitGitGadget]

The search order for DLLs on Windows is a bit funny, and for system
libraries, it is recommended to use a strict search path.

In practice, this should not make a difference, as the library has been
loaded into memory already, and therefore the LoadLibrary() call would just
return the handle instead of loading the library again.

Johannes Schindelin (1):
  mingw: load system libraries the recommended way

 compat/mingw.c  | 3 ++-
 contrib/credential/wincred/git-credential-wincred.c | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)


base-commit: c4df23f7927d8d00e666a3c8d1b3375f1dc8a3c1
Published-As: 
https://github.com/gitgitgadget/git/releases/tags/pr-55%2Fdscho%2Fmingw-load-sys-dll-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git 
pr-55/dscho/mingw-load-sys-dll-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/55
-- 
gitgitgadget

[PATCH 1/1] mingw: load system libraries the recommended way

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin 

When we access IPv6-related functions, we load the corresponding system
library using the `LoadLibrary()` function, which is not the recommended
way to load system libraries.

In practice, it does not make a difference: the `ws2_32.dll` library
containing the IPv6 functions is already loaded into memory, so
LoadLibrary() simply reuses the already-loaded library.

Still, recommended way is recommended way, so let's use that instead.

While at it, also adjust the code in contrib/ that loads system libraries.

Signed-off-by: Johannes Schindelin 
---
 compat/mingw.c  | 3 ++-
 contrib/credential/wincred/git-credential-wincred.c | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 18caf2196..9fd7db571 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -1577,7 +1577,8 @@ static void ensure_socket_initialization(void)
WSAGetLastError());
 
for (name = libraries; *name; name++) {
-   ipv6_dll = LoadLibrary(*name);
+   ipv6_dll = LoadLibraryExA(*name, NULL,
+ LOAD_LIBRARY_SEARCH_SYSTEM32);
if (!ipv6_dll)
continue;
 
diff --git a/contrib/credential/wincred/git-credential-wincred.c 
b/contrib/credential/wincred/git-credential-wincred.c
index 86518cd93..5bdad41de 100644
--- a/contrib/credential/wincred/git-credential-wincred.c
+++ b/contrib/credential/wincred/git-credential-wincred.c
@@ -75,7 +75,8 @@ static CredDeleteWT CredDeleteW;
 static void load_cred_funcs(void)
 {
/* load DLLs */
-   advapi = LoadLibrary("advapi32.dll");
+   advapi = LoadLibraryExA("advapi32.dll", NULL,
+   LOAD_LIBRARY_SEARCH_SYSTEM32);
if (!advapi)
die("failed to load advapi32.dll");
 
-- 
gitgitgadget

[PATCH 1/2] mingw: ensure `getcwd()` reports the correct case

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin 

When switching the current working directory, say, in PowerShell, it is
quite possible to use a different capitalization than the one that is
recorded on disk. While doing the same in `cmd.exe` adjusts the
capitalization magically, that does not happen in PowerShell so that
`getcwd()` returns the current directory in a different way than is
recorded on disk.

Typically this creates no problems except when you call

git log .

in a subdirectory called, say, "GIT/" but you switched to "Git/" and
your `getcwd()` reports the latter, then Git won't understand that you
wanted to see the history as per the `GIT/` subdirectory but it thinks you
wanted to see the history of some directory that may have existed in the
past (but actually never did).

So let's be extra careful to adjust the capitalization of the current
directory before working with it.

Reported by a few PowerShell power users ;-)

Signed-off-by: Johannes Schindelin 
---
 compat/mingw.c | 11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 18caf2196..2c3e27ce9 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -917,8 +917,15 @@ struct tm *localtime_r(const time_t *timep, struct tm 
*result)
 
 char *mingw_getcwd(char *pointer, int len)
 {
-   wchar_t wpointer[MAX_PATH];
-   if (!_wgetcwd(wpointer, ARRAY_SIZE(wpointer)))
+   wchar_t cwd[MAX_PATH], wpointer[MAX_PATH];
+   DWORD ret = GetCurrentDirectoryW(ARRAY_SIZE(cwd), cwd);
+
+   if (!ret || ret >= ARRAY_SIZE(cwd)) {
+   errno = ret ? ENAMETOOLONG : err_win_to_posix(GetLastError());
+   return NULL;
+   }
+   ret = GetLongPathNameW(cwd, wpointer, ARRAY_SIZE(wpointer));
+   if (!ret || ret >= ARRAY_SIZE(wpointer))
return NULL;
if (xwcstoutf(pointer, wpointer, len) < 0)
return NULL;
-- 
gitgitgadget

[PATCH 0/2] Work around case-insensitivity issues with cwd on Windows

[Johannes Schindelin via GitGitGadget]

On Windows, file names are recorded case-sensitively, but looked up
case-insensitively. Therefore, it is possible to switch to a directory by
using incorrect case, e.g. cd documentation will still get you into the 
Documentation subdirectory.

In Powershell, doing so will however report the current directory with the
specified spelling rather than the one recorded on disk, and Git will get
confused.

To remedy that, we fixed this in Git for Windows more than three years ago,
and needed only a small fix a couple of months later to accommodate for the
diverse scenarios encountered by the many Git for Windows users.

Not only to keep the story closer to what happened historically, but also to
make it easier to follow, I refrained from squashing these two patches.

Side note: the second patch is technically not battle-tested for that long:
it uses an API function that requires Windows Vista or later, and we only
recently started to clean up Git for Windows' code to drop fallbacks for
Windows XP. Read: this code used to load the GetFinalPathNameByHandle() 
function dynamically, and that is the only difference to the code that has
been "battle-tested" for close to three years.

Anton Serbulov (1):
  mingw: fix getcwd when the parent directory cannot be queried

Johannes Schindelin (1):
  mingw: ensure `getcwd()` reports the correct case

 compat/mingw.c | 50 --
 1 file changed, 48 insertions(+), 2 deletions(-)


base-commit: c4df23f7927d8d00e666a3c8d1b3375f1dc8a3c1
Published-As: 
https://github.com/gitgitgadget/git/releases/tags/pr-54%2Fdscho%2Fmingw-getcwd-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git 
pr-54/dscho/mingw-getcwd-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/54
-- 
gitgitgadget

[PATCH 2/2] mingw: fix getcwd when the parent directory cannot be queried

[Anton Serbulov via GitGitGadget]

From: Anton Serbulov 

`GetLongPathName()` function may fail when it is unable to query
the parent directory of a path component to determine the long name
for that component. It happens, because of it uses `FindFirstFile()`
function for each next short part of path. The `FindFirstFile()`
requires `List Directory` and `Synchronize` desired access for a calling
process.

In case of lacking such permission for some part of path,
the `GetLongPathName()` returns 0 as result and `GetLastError()`
returns ERROR_ACCESS_DENIED.

`GetFinalPathNameByHandle()` function can help in such cases, because
it requires `Read Attributes` and `Synchronize` desired access to the
target path only.

The `GetFinalPathNameByHandle()` function was introduced on
`Windows Server 2008/Windows Vista`. So we need to load it dynamically.

`CreateFile()` parameters:
`lpFileName` = path to the current directory
`dwDesiredAccess` = 0 (it means `Read Attributes` and `Synchronize`)
`dwShareMode` = FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE
(it prevents `Sharing Violation`)
`lpSecurityAttributes` = NULL (default security attributes)
`dwCreationDisposition` = OPEN_EXISTING
  (required to obtain a directory handle)
`dwFlagsAndAttributes` = FILE_FLAG_BACKUP_SEMANTICS
 (required to obtain a directory handle)
`hTemplateFile` = NULL (when opening an existing file or directory,
`CreateFile` ignores this parameter)

The string that is returned by `GetFinalPathNameByHandle()` function
uses the \\?\ syntax. To skip the prefix and convert backslashes
to slashes, the `normalize_ntpath()` mingw function will be used.

Note: `GetFinalPathNameByHandle()` function returns a final path.
It is the path that is returned when a path is fully resolved.
For example, for a symbolic link named "C:\tmp\mydir" that points to
"D:\yourdir", the final path would be "D:\yourdir".

Signed-off-by: Anton Serbulov 
Signed-off-by: Johannes Schindelin 
---
 compat/mingw.c | 39 +++
 1 file changed, 39 insertions(+)

diff --git a/compat/mingw.c b/compat/mingw.c
index 2c3e27ce9..19addfa5d 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -202,6 +202,31 @@ static int ask_yes_no_if_possible(const char *format, ...)
}
 }
 
+/* Normalizes NT paths as returned by some low-level APIs. */
+static wchar_t *normalize_ntpath(wchar_t *wbuf)
+{
+   int i;
+   /* fix absolute path prefixes */
+   if (wbuf[0] == '\\') {
+   /* strip NT namespace prefixes */
+   if (!wcsncmp(wbuf, L"\\??\\", 4) ||
+   !wcsncmp(wbuf, L"?\\", 4))
+   wbuf += 4;
+   else if (!wcsnicmp(wbuf, L"\\DosDevices\\", 12))
+   wbuf += 12;
+   /* replace remaining '...UNC\' with '\\' */
+   if (!wcsnicmp(wbuf, L"UNC\\", 4)) {
+   wbuf += 2;
+   *wbuf = '\\';
+   }
+   }
+   /* convert backslashes to slashes */
+   for (i = 0; wbuf[i]; i++)
+   if (wbuf[i] == '\\')
+   wbuf[i] = '/';
+   return wbuf;
+}
+
 int mingw_unlink(const char *pathname)
 {
int ret, tries = 0;
@@ -925,6 +950,20 @@ char *mingw_getcwd(char *pointer, int len)
return NULL;
}
ret = GetLongPathNameW(cwd, wpointer, ARRAY_SIZE(wpointer));
+   if (!ret && GetLastError() == ERROR_ACCESS_DENIED) {
+   HANDLE hnd = CreateFileW(cwd, 0,
+   FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, 
NULL,
+   OPEN_EXISTING, FILE_FLAG_BACKUP_SEMANTICS, NULL);
+   if (hnd == INVALID_HANDLE_VALUE)
+   return NULL;
+   ret = GetFinalPathNameByHandleW(hnd, wpointer, 
ARRAY_SIZE(wpointer), 0);
+   CloseHandle(hnd);
+   if (!ret || ret >= ARRAY_SIZE(wpointer))
+   return NULL;
+   if (xwcstoutf(pointer, normalize_ntpath(wpointer), len) < 0)
+   return NULL;
+   return pointer;
+   }
if (!ret || ret >= ARRAY_SIZE(wpointer))
return NULL;
if (xwcstoutf(pointer, wpointer, len) < 0)
-- 
gitgitgadget

Re: [PATCH] Poison gettext with the Ook language

[Johannes Schindelin]

Hi Ævar,

On Tue, 23 Oct 2018, Ævar Arnfjörð Bjarmason wrote:

> On Tue, Oct 23 2018, Johannes Schindelin wrote:
> 
> > On Mon, 22 Oct 2018, Ævar Arnfjörð Bjarmason wrote:
> >
> >> So I think the only reason to keep it [GETTEXT_POISON] compile-time is
> >> performance, but I don't think that matters. It's not like we're
> >> printing gigabytes of _() formatted output. Everything where formatting
> >> matters is plumbing which doesn't use this API. These messages are
> >> always for human consumption.
> >
> > Well, let's make sure that your impression is correct before going too
> > far. I, too, had the impression that gettext cannot possibly be expensive,
> > especifally in Git for Windows' settings, where we do not even ship
> > translations. Yet see the commit message of cc5e1bf99247 (gettext: avoid
> > initialization if the locale dir is not present, 2018-04-21):
> >
> > The runtime of a simple `git.exe version` call on Windows is
> > currently dominated by the gettext setup, adding a whopping ~150ms
> > to the ~210ms total.
> >
> > I would be in favor of your change to make this a runtime option, of
> > course, as long as it does not affect performance greatly (in particular
> > on Windows, where we fight an uphill battle to make Git faster).
> 
> How expensive gettext() may or may not be isn't relevant to the
> GETTEXT_POISON compile-time option.
> 
> The effect of what I'm suggesting here, and which my WIP patch in
> <875zxtd59e@evledraar.gmail.com> implements is that we'd do a
> one-time getenv() for each process that prints a _() message that we
> aren't doing now, and for each message call a function that would check
> a boolean "are we in poison mode" static global variable.

Yep, you are right, we are *already* going through _() as a function,
whether gettext() is initialized or not. My bad.

> Perhaps some of that's expensive on Windows, but given the recent
> patches by Microsoft employees to add GIT_TEST_* env options I assumed
> not, but in any case it won't have anything to do with how expensive
> gettext may or may not be, you'll already be paying that cost now (or
> not, with NO_GETTEXT).

Indeed, we want to measure performance better, and that's what those
environment variables are for.

Thanks,
Dscho

Failed stash caused untracked changes to be lost

[Quinn, David]

Issue: While running a git stash command including the '-u' flag to include 
untracked files, the command failed due to arguments in the incorrect order. 
After this untracked files the were present had been removed and permanently 
lost.

Environment: Windows 10, Powershell w/ PoshGit


State before running command: 9 Modified files, 2 (new) untracked files

Note: I only wanted to commit some of the modified files (essentially all the 
files/changes I wanted to commit were in one directory)

Actual command run:  git stash push -u -- Directory/To/Files/* -m "My Message"

Returned:

Saved working directory and index state WIP on [BranchName]: [Commit hash] 
[Commit Message]
fatal: pathspec '-m' did not match any files
error: unrecognized input

State after Command ran: 9 Modifed files, 0 untracked files


The command I should have ran should have been

git stash push -u -m "My Message"? -- Directory/To/Files/*


I have found the stash that was created by running this command:

gitk --all $(git fsck --no-reflog | Select-String "(dangling commit )(.*)" 
| %{ $_.Line.Split(' ')[2] })
?
and searching for the commit number that was returned from the original 
(paritally failed??) stash command. However there is nothing in that stash. It 
is empty.



I think that the fact my untracked files were lost is not correct behaviour and 
hence why I'm filing this bug report





NOTICE: This message, and any attachments, are for the intended recipient(s) 
only, may contain information that is privileged, confidential and/or 
proprietary and subject to important terms and conditions available at 
E-Communication 
Disclaimer.
 If you are not the intended recipient, please delete this message. CME Group 
and its subsidiaries reserve the right to monitor all email communications that 
occur on CME Group information systems.

[PATCH] khash: silence -Wunused-function

[Carlo Marcelo Arenas Belón]

after 36da893114 ("config.mak.dev: enable -Wunused-function", 2018-10-18)
macro generated code should use a similar solution than commit-slab to
silence the compiler.

Signed-off-by: Carlo Marcelo Arenas Belón 
---
 khash.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/khash.h b/khash.h
index d10caa0c35..39c2833877 100644
--- a/khash.h
+++ b/khash.h
@@ -234,7 +234,7 @@ static const double __ac_HASH_UPPER = 0.77;
__KHASH_IMPL(name, SCOPE, khkey_t, khval_t, kh_is_map, __hash_func, 
__hash_equal)
 
 #define KHASH_INIT(name, khkey_t, khval_t, kh_is_map, __hash_func, 
__hash_equal) \
-   KHASH_INIT2(name, static inline, khkey_t, khval_t, kh_is_map, 
__hash_func, __hash_equal)
+   KHASH_INIT2(name, __attribute__((__unused__)) static inline, khkey_t, 
khval_t, kh_is_map, __hash_func, __hash_equal)
 
 /* Other convenient macros... */
 
-- 
2.19.1

[PATCH v2] compat: make sure git_mmap is not expected to write

[Carlo Marcelo Arenas Belón]

in f48000fc ("Yank writing-back support from gitfakemmap.", 2005-10-08)
support for writting back changes was removed but the specific prot
flag that would be used was not checked for

Acked-by: Johannes Schindelin 
Signed-off-by: Carlo Marcelo Arenas Belón 
---
Changes in v2:

* reset-author to match signature
* cleanup commit message and add ACK

 compat/mmap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/compat/mmap.c b/compat/mmap.c
index 7f662fef7b..14d31010df 100644
--- a/compat/mmap.c
+++ b/compat/mmap.c
@@ -4,7 +4,7 @@ void *git_mmap(void *start, size_t length, int prot, int flags, 
int fd, off_t of
 {
size_t n = 0;
 
-   if (start != NULL || !(flags & MAP_PRIVATE))
+   if (start != NULL || flags != MAP_PRIVATE || prot != PROT_READ)
die("Invalid usage of mmap when built with NO_MMAP");
 
start = xmalloc(length);
-- 
2.19.1

Re: [PATCH 2/2] mingw: fix getcwd when the parent directory cannot be queried

[Stephen & Linda Smith]

On Tuesday, October 23, 2018 3:52:49 AM MST you wrote:
> From: Anton Serbulov 
> 
> `GetLongPathName()` function may fail when it is unable to query
> the parent directory of a path component to determine the long name
> for that component. It happens, because of it uses `FindFirstFile()`
s/of it/it/

> function for each next short part of path. The `FindFirstFile()`
> requires `List Directory` and `Synchronize` desired access for a calling
> process.

Re: [PATCH 2/2] mingw: fix getcwd when the parent directory cannot be queried

[Stephen Smith]

On Tuesday, October 23, 2018 6:16:43 AM MST Stephen Smith wrote:
> On Tuesday, October 23, 2018 3:52:49 AM MST you wrote:
> > From: Anton Serbulov 
> > 
> > `GetLongPathName()` function may fail when it is unable to query
> > the parent directory of a path component to determine the long name
> > for that component. It happens, because of it uses `FindFirstFile()`
> 
> s/of it/it/
> 
> > function for each next short part of path. The `FindFirstFile()`
> > requires `List Directory` and `Synchronize` desired access for a calling
> > process.

BTW - It was "Stephen" that sent the email. :-)

Re: [PATCH v4 6/7] revision.c: generation-based topo-order algorithm

[Derrick Stolee]

On 10/22/2018 9:37 AM, Jakub Narebski wrote:

"Derrick Stolee via GitGitGadget"  writes:


From: Derrick Stolee 

The current --topo-order algorithm requires walking all
reachable commits up front, topo-sorting them, all before
outputting the first value. This patch introduces a new
algorithm which uses stored generation numbers to
incrementally walk in topo-order, outputting commits as
we go. This can dramatically reduce the computation time
to write a fixed number of commits, such as when limiting
with "-n " or filling the first page of a pager.

When running a command like 'git rev-list --topo-order HEAD',
Git performed the following steps:

1. Run limit_list(), which parses all reachable commits,
adds them to a linked list, and distributes UNINTERESTING
flags. If all unprocessed commits are UNINTERESTING, then
it may terminate without walking all reachable commits.
This does not occur if we do not specify UNINTERESTING
commits.

2. Run sort_in_topological_order(), which is an implementation
of Kahn's algorithm. It first iterates through the entire
set of important commits and computes the in-degree of each
(plus one, as we use 'zero' as a special value here). Then,
we walk the commits in priority order, adding them to the
priority queue if and only if their in-degree is one. As
we remove commits from this priority queue, we decrement the
in-degree of their parents.

Because in-degree has very specific defined meaning of number of
children, i.e. the number of _incoming_ edges, I would say "if and only
if their in-degree-plus-one is one".  It is more exact, even if it looks
a bit funny.


3. While we are peeling commits for output, get_revision_1()
uses pop_commit on the full list of commits computed by
sort_in_topological_order().

All right, so those are separate steps (separate walks): prepare and
parse commits, topologically sort list of commits from previous step,
output sorted list of commits from previous step.


I would rephrase your explanation above as: prepare and parse commits, 
compute in-degrees, and peel commits of in-degree zero.



In the new algorithm, these three steps correspond to three
different commit walks. We run these walks simultaneously,
and advance each only as far as necessary to satisfy the
requirements of the 'higher order' walk.

What does 'higher order' walk means: steps 3, 2, 1, in this order,
i.e. output being the highest order, or something different?


Yes. We only walk "level 2" in order to satisfy how far we are in "level 3".


Sidenote: the new algorithm looks a bit like Unix pipeline, where each
step of pipeline does not output much more than next step needs /
requests.


That's essentially the idea.


  We know when we can
pause each walk by using generation numbers from the commit-
graph feature.

Do I understand it correctly that this is mainly used in Kahn's
algorithm to find out through the negative-cut index of generation
number which commits in the to-be-sorted list cannot have an in-degree
of zero (or otherise cannot be next commit to be shown in output)?


In each step of the algorithm, we operate under the assumption that 
certain vertices have "all necessary information".


In the case of "level 3", we need to know that all descendants were 
walked and our in-degree calculation is correct. We guarantee this by 
ensuring that "level 2" has walked beyond that commit's generation number.


In the case of "level 2", we need to know that we have parsed all 
descendants and determined their simplifications (if necessary, such as 
in file-history) and if they are UNINTERESTING. We guarantee this by 
ensuring that "level 1" has walked beyond that commit's generation number.


In the previous algorithm, these guarantees were handled by doing each 
step on all reachable commits before moving to the next level.



Recall that the generation number of a commit satisfies:

* If the commit has at least one parent, then the generation
   number is one more than the maximum generation number among
   its parents.

* If the commit has no parent, then the generation number is one.

There are two special generation numbers:

* GENERATION_NUMBER_INFINITY: this value is 0x and
   indicates that the commit is not stored in the commit-graph and
   the generation number was not previously calculated.

* GENERATION_NUMBER_ZERO: this value (0) is a special indicator
   to say that the commit-graph was generated by a version of Git
   that does not compute generation numbers (such as v2.18.0).

Since we use generation_numbers_enabled() before using the new
algorithm, we do not need to worry about GENERATION_NUMBER_ZERO.
However, the existence of GENERATION_NUMBER_INFINITY implies the
following weaker statement than the usual we expect from
generation numbers:

 If A and B are commits with generation numbers gen(A) and
 gen(B) and gen(A) < gen(B), then A cannot reach B

Re: [PATCH] range-diff: allow to diff files regardless submodule

[Lucas De Marchi]

On Thu, Oct 11, 2018 at 05:25:02PM +0900, Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason  writes:
> 
> > On Wed, Oct 10 2018, Lucas De Marchi wrote:
> >
> >> Do like it's done in grep so mode doesn't end up as
> >> 016, which means range-diff doesn't work if one has
> >> "submodule.diff = log" in the configuration. Without this
> >> while using range-diff I only get a
> >>
> >> Submodule a 000...000 (new submodule)
> >
> > I'm not familiar enough with this to tell what the real problem is
> > that's being solved from the commit message, but if it means that now
> > range-diff works in some configuration, presumably that can be reduced
> > to a simple set of commands that didn't work before but now does, and
> > therefore a test in t/t3206-range-diff.sh.
> 
> Yes, I agree on both counts (i.e. it was totally unclear what
> problem is being solved and what the root cause of the problem is,
> and we would want a new test to protect this "fix" from getting
> broken in the future.

have you seen I sent a v2 with proper test?

thanks
Lucas De Marchi

> 
> Thanks.

Re: [PATCH] Poison gettext with the Ook language

[Duy Nguyen]

On Mon, Oct 22, 2018 at 10:23 PM SZEDER Gábor  wrote:
> Once upon a time a GETTEXT_POISON build job failed on me, and the
> error message:
>
>   error: # GETTEXT POISON #
>
> was not particularly useful.  Ook wouldn't help with that...

Oook?

> So I came up with the following couple of patches that implement a
> "scrambled" format that makes the poisoned output legible for humans
> but still gibberish for machine consumption (i.e. grep-ing the text
> part would still fail):
>
>   error:  U.n.a.b.l.e. .t.o. .c.r.e.a.t.e. .'./home/szeder/src/git/t/trash 
> directory.t1404-update-ref-errors/.git/packed-refs...l.o.c.k.'.:. .File 
> exists...
>
> I have been running GETTEXT_POISON builds with this series for some
> months now, but haven't submitted it yet, because I haven't decided
> yet whether including strings (paths, refs, etc.) in the output as
> they are is a feature or a flaw.

A feature to me. The only thing that got through and but should not be
grep-able is strerror(). But that's orthogonal to this scrambling
stuff.

> And because it embarrassingly leaks every single translated string... :)

Easy to fix. I hope this means you have no more excuse to not push
this forward ;-)
-- 
Duy

Re: [PATCH 7/8] gettext: introduce GIT_GETTEXT_POISON=scrambled

[Duy Nguyen]

On Mon, Oct 22, 2018 at 10:23 PM SZEDER Gábor  wrote:
> [TODO: Fallout?
>A 'printf(_("foo: %s"), var);' call includes the contents of
>'var' unscrambled in the output.  Could that hide the
>translation of a string that should not have been translated?
>I'm afraid yes: to check the output of that printf() a sloppy
>test could do:
>
>  git plumbing-cmd >out && grep "var's content" out
>
>which would fail in a regular GETTEXT_POISON test run, but
>would succeed in a scrambled test run.  Does this matter in
>practice, do we care at all?

If var is supposed to be translated, _() must have been called before
the final string is stored in var and the content is already
scrambled. Whatever left unscrambled is machine-generated and should
be ok to grep, or we have found new strings that should be _() but
not.

PS. Another thing I'd like to have is to mark the beginning and end of
a scrambled text. For example, _("foo") produces "[f.o.o]" or
something like that. If we have it, it's easier to see "text legos"
(instead of full sentences) that makes translator's life harder. But
it could interfere with stuff (e.g. some strings must start with '#')
so let's forget it for now.

>
>Does gettext_scramble() need a FORMAT_PRESERVING annotation?
>Seems to work fine without it so far...]

I don't think you can. _() can be called on plain strings that just
happen to have '%' in them.
-- 
Duy

Re: [PATCH] Poison gettext with the Ook language

[Duy Nguyen]

On Tue, Oct 23, 2018 at 12:17 PM Ævar Arnfjörð Bjarmason
 wrote:
>
>
> On Tue, Oct 23 2018, Johannes Schindelin wrote:
>
> > Hi Ævar,
> >
> > On Mon, 22 Oct 2018, Ævar Arnfjörð Bjarmason wrote:
> >
> >> So I think the only reason to keep it [GETTEXT_POISON] compile-time is
> >> performance, but I don't think that matters. It's not like we're
> >> printing gigabytes of _() formatted output. Everything where formatting
> >> matters is plumbing which doesn't use this API. These messages are
> >> always for human consumption.
> >
> > Well, let's make sure that your impression is correct before going too
> > far. I, too, had the impression that gettext cannot possibly be expensive,
> > especifally in Git for Windows' settings, where we do not even ship
> > translations. Yet see the commit message of cc5e1bf99247 (gettext: avoid
> > initialization if the locale dir is not present, 2018-04-21):
> >
> >   The runtime of a simple `git.exe version` call on Windows is
> >   currently dominated by the gettext setup, adding a whopping ~150ms
> >   to the ~210ms total.
> >
> > I would be in favor of your change to make this a runtime option, of
> > course, as long as it does not affect performance greatly (in particular
> > on Windows, where we fight an uphill battle to make Git faster).
>
> How expensive gettext() may or may not be isn't relevant to the
> GETTEXT_POISON compile-time option.

If a user requests NO_GETTEXT, they should have zero (or near zero)
cost related to gettext. Which is true until now (the inline _()
version is expanded to pretty much no-op with the exception of NULL
string)

> The effect of what I'm suggesting here, and which my WIP patch in
> <875zxtd59e@evledraar.gmail.com> implements is that we'd do a
> one-time getenv() for each process that prints a _() message that we
> aren't doing now, and for each message call a function that would check
> a boolean "are we in poison mode" static global variable.

Just don't do the getenv() inside _() even if you just do it one time.
getenv() may invalidate whatever value from the previous call. I would
not want to find out my code broke because of an getenv() inside some
innocent _()... And we should still respect NO_GETTEXT, not doing any
extra work when it's defined.
-- 
Duy

Re: [PATCH] khash: silence -Wunused-function

[Duy Nguyen]

On Tue, Oct 23, 2018 at 1:42 PM Carlo Marcelo Arenas Belón
 wrote:
>
> after 36da893114 ("config.mak.dev: enable -Wunused-function", 2018-10-18)
> macro generated code should use a similar solution than commit-slab to
> silence the compiler.
>
> Signed-off-by: Carlo Marcelo Arenas Belón 
> ---
>  khash.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/khash.h b/khash.h
> index d10caa0c35..39c2833877 100644
> --- a/khash.h
> +++ b/khash.h
> @@ -234,7 +234,7 @@ static const double __ac_HASH_UPPER = 0.77;
> __KHASH_IMPL(name, SCOPE, khkey_t, khval_t, kh_is_map, __hash_func, 
> __hash_equal)
>
>  #define KHASH_INIT(name, khkey_t, khval_t, kh_is_map, __hash_func, 
> __hash_equal) \
> -   KHASH_INIT2(name, static inline, khkey_t, khval_t, kh_is_map, 
> __hash_func, __hash_equal)
> +   KHASH_INIT2(name, __attribute__((__unused__)) static inline, khkey_t, 
> khval_t, kh_is_map, __hash_func, __hash_equal)

Maybe move MAYBE_UNUSED from commit-slab-impl.h to git-compat-util.h
and use it here so we have an easier time if we have to use a
different way to mark unused functions on some exotic platform.

>
>  /* Other convenient macros... */
>
> --
> 2.19.1
>


-- 
Duy

Re: [PATCH v3 1/3] reset: don't compute unstaged changes after reset when --quiet

[Duy Nguyen]

On Tue, Oct 23, 2018 at 1:01 AM Ben Peart  wrote:
>
> > -Original Message-
> > From: Johannes Schindelin 
> > Sent: Monday, October 22, 2018 4:45 PM
> > To: Ben Peart 
> > Cc: git@vger.kernel.org; gits...@pobox.com; Ben Peart
> > ; p...@peff.net; sunsh...@sunshineco.com
> > Subject: Re: [PATCH v3 1/3] reset: don't compute unstaged changes after
> > reset when --quiet
> >
> > Hi Ben,
> >
> > On Mon, 22 Oct 2018, Ben Peart wrote:
> >
> > > From: Ben Peart 
> > >
> > > When git reset is run with the --quiet flag, don't bother finding any
> > > additional unstaged changes as they won't be output anyway.  This speeds
> > up
> > > the git reset command by avoiding having to lstat() every file looking for
> > > changes that aren't going to be reported anyway.
> > >
> > > The savings can be significant.  In a repo with 200K files "git reset"
> > > drops from 7.16 seconds to 0.32 seconds for a savings of 96%.
> >
> > That's very nice!
> >
> > Those numbers, just out of curiosity, are they on Windows? Or on Linux?
> >
>
> It's safe to assume all my numbers are on Windows. :-)

It does bug me about this. Next time please mention the platform you
tested on in the commit message. Not all platforms behave the same way
especially when it comes to performance.

>
> > Ciao,
> > Dscho
>
>


-- 
Duy

Re: [PATCH v4 7/7] t6012: make rev-list tests more interesting

[Jakub Narebski]

"Derrick Stolee via GitGitGadget"  writes:

> From: Derrick Stolee 
>
> As we are working to rewrite some of the revision-walk machinery,
> there could easily be some interesting interactions between the
> options that force topological constraints (--topo-order,
> --date-order, and --author-date-order) along with specifying a
> path.
>
> Add extra tests to t6012-rev-list-simplify.sh to add coverage of
> these interactions. To ensure interesting things occur, alter the
> repo data shape to have different orders depending on topo-, date-,
> or author-date-order.

Very nice, though I have noticed (please correct me if I am wrong) that
in all cases path limited query always have the same result for
--topo-order and for --date-order; as opposed to three different results
for three different revision sorting modes for path-less query.

>
> When testing using GIT_TEST_COMMIT_GRAPH, this assists in covering
> the new logic for topo-order walks using generation numbers. The
> extra tests can be added indepently.

Good.  I guess we are mainly interested in tests without limits and
exclusions, i.e. A or A B and not A..B or A...B walks.

>
> Signed-off-by: Derrick Stolee 
> ---
>  t/t6012-rev-list-simplify.sh | 45 
>  1 file changed, 36 insertions(+), 9 deletions(-)
>
> diff --git a/t/t6012-rev-list-simplify.sh b/t/t6012-rev-list-simplify.sh
> index b5a1190ffe..a10f0df02b 100755
> --- a/t/t6012-rev-list-simplify.sh
> +++ b/t/t6012-rev-list-simplify.sh
> @@ -12,6 +12,22 @@ unnote () {
>   git name-rev --tags --stdin | sed -e "s|$OID_REGEX (tags/\([^)]*\)) |\1 
> |g"
>  }
>  
> +#
> +# Create a test repo with interesting commit graph:
> +#
> +# A--B--G--H--I--K--L
> +#  \  \   / /
> +#   \  \ / /
> +#C--E---F J
> +#\_/
> +#
> +# The commits are laid out from left-to-right starting with
> +# the root commit A and terminating at the tip commit L.

Do I understand it correctly that it is a visualization of history
created by existing code (which is a very nice to have)?

> +#
> +# There are a few places where we adjust the commit date or
> +# author date to make the --topo-order, --date-order, and
> +# --author-date-order flags produce different output.

Sidenote: it looks like "a few places" is "one place" for now...

> +
>  test_expect_success setup '
>   echo "Hi there" >file &&
>   echo "initial" >lost &&
> @@ -21,10 +37,18 @@ test_expect_success setup '
>  
>   git branch other-branch &&
>  
> + git symbolic-ref HEAD refs/heads/unrelated &&
> + git rm -f "*" &&
> + echo "Unrelated branch" >side &&
> + git add side &&
> + test_tick && git commit -m "Side root" &&
> + note J &&
> + git checkout master &&

I see that this fragment is moved earlier, but I don't know what
consequences it does have.

> +
>   echo "Hello" >file &&
>   echo "second" >lost &&
>   git add file lost &&
> - test_tick && git commit -m "Modified file and lost" &&
> + test_tick && GIT_AUTHOR_DATE=$(($test_tick + 120)) git commit -m 
> "Modified file and lost" &&
>   note B &&

Nice trick, though I think it produces slightly unrealistic history (at
least in absence of the clock skew).  Author dates are ordinarily
earlier or equal to commit dates, and commits can be authored in
different order that they were committed.

>  
>   git checkout other-branch &&
> @@ -63,13 +87,6 @@ test_expect_success setup '
>   test_tick && git commit -a -m "Final change" &&
>   note I &&
>  
> - git symbolic-ref HEAD refs/heads/unrelated &&
> - git rm -f "*" &&
> - echo "Unrelated branch" >side &&
> - git add side &&
> - test_tick && git commit -m "Side root" &&
> - note J &&
> -
>   git checkout master &&
>   test_tick && git merge --allow-unrelated-histories -m "Coolest" 
> unrelated &&
>   note K &&
> @@ -103,14 +120,24 @@ check_result () {
>   check_outcome success "$@"
>  }
>  
> -check_result 'L K J I H G F E D C B A' --full-history
> +check_result 'L K J I H F E D C G B A' --full-history --topo-order
> +check_result 'L K I H G F E D C B J A' --full-history
> +check_result 'L K I H G F E D C B J A' --full-history --date-order
> +check_result 'L K I H G F E D B C J A' --full-history --author-date-order
>  check_result 'K I H E C B A' --full-history -- file
>  check_result 'K I H E C B A' --full-history --topo-order -- file
>  check_result 'K I H E C B A' --full-history --date-order -- file
> +check_result 'K I H E B C A' --full-history --author-date-order -- file
>  check_result 'I E C B A' --simplify-merges -- file
> +check_result 'I E C B A' --simplify-merges --topo-order -- file
> +check_result 'I E C B A' --simplify-merges --date-order -- file
> +check_result 'I E B C A' --simplify-merges --author-date-order -- file
>  check_result 'I B A' -- file
>  check_result 'I B A' --topo-order -- file
> +check_result 'I B A' --date-order -- file
> +check_result 'I B

[PATCH/RFC v2 1/1] Use off_t instead of size_t for functions dealing with streamed checkin

[tboegi]

From: Torsten Bögershausen 

When streaming data from disk into a blob, it should be possible to commit
a file with a file size > 4 GiB using the streaming functionality in Git.
Because of the streaming there is no need to load the whole data into
memory at once.
Today this is not possible on e.g. a 32 bit Linux system.
There is no good reason to limit the length of the file by using a size_t
in the code, which is a 32 bit value.
Loosen this restriction and use off_t instead of size_t in the call chain.

Signed-off-by: Torsten Bögershausen 
---

This is a suggestion for V2, changing even sha1-file.c,
so that the whole patch makes more sense.
The initial commit of a >4Gib file was tested on a 32 bit system

I didn't remove the wrapper functions, as I don't know
what their purpose is.

And: The commit message may need some tweaking, though

bulk-checkin.c | 6 +++---
 bulk-checkin.h | 2 +-
 sha1-file.c| 5 ++---
 3 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/bulk-checkin.c b/bulk-checkin.c
index 409ecb566b..34dbf5c4ea 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -96,7 +96,7 @@ static int already_written(struct bulk_checkin_state *state, 
struct object_id *o
  */
 static int stream_to_pack(struct bulk_checkin_state *state,
  git_hash_ctx *ctx, off_t *already_hashed_to,
- int fd, size_t size, enum object_type type,
+ int fd, off_t size, enum object_type type,
  const char *path, unsigned flags)
 {
git_zstream s;
@@ -189,7 +189,7 @@ static void prepare_to_stream(struct bulk_checkin_state 
*state,
 
 static int deflate_to_pack(struct bulk_checkin_state *state,
   struct object_id *result_oid,
-  int fd, size_t size,
+  int fd, off_t size,
   enum object_type type, const char *path,
   unsigned flags)
 {
@@ -258,7 +258,7 @@ static int deflate_to_pack(struct bulk_checkin_state *state,
 }
 
 int index_bulk_checkin(struct object_id *oid,
-  int fd, size_t size, enum object_type type,
+  int fd, off_t size, enum object_type type,
   const char *path, unsigned flags)
 {
int status = deflate_to_pack(&state, oid, fd, size, type,
diff --git a/bulk-checkin.h b/bulk-checkin.h
index f438f93811..09b2affdf3 100644
--- a/bulk-checkin.h
+++ b/bulk-checkin.h
@@ -7,7 +7,7 @@
 #include "cache.h"
 
 extern int index_bulk_checkin(struct object_id *oid,
- int fd, size_t size, enum object_type type,
+ int fd, off_t size, enum object_type type,
  const char *path, unsigned flags);
 
 extern void plug_bulk_checkin(void);
diff --git a/sha1-file.c b/sha1-file.c
index a4367b8f04..98d0f50ffa 100644
--- a/sha1-file.c
+++ b/sha1-file.c
@@ -1934,7 +1934,7 @@ static int index_core(struct object_id *oid, int fd, 
size_t size,
  * binary blobs, they generally do not want to get any conversion, and
  * callers should avoid this code path when filters are requested.
  */
-static int index_stream(struct object_id *oid, int fd, size_t size,
+static int index_stream(struct object_id *oid, int fd, off_t size,
enum object_type type, const char *path,
unsigned flags)
 {
@@ -1959,8 +1959,7 @@ int index_fd(struct object_id *oid, int fd, struct stat 
*st,
ret = index_core(oid, fd, xsize_t(st->st_size), type, path,
 flags);
else
-   ret = index_stream(oid, fd, xsize_t(st->st_size), type, path,
-  flags);
+   ret = index_stream(oid, fd, st->st_size, type, path, flags);
close(fd);
return ret;
 }
-- 
2.11.0

Re: [PATCH] khash: silence -Wunused-function

[René Scharfe]

Am 23.10.2018 um 13:34 schrieb Carlo Marcelo Arenas Belón:
> after 36da893114 ("config.mak.dev: enable -Wunused-function", 2018-10-18)
> macro generated code should use a similar solution than commit-slab to
> silence the compiler.

With Clang 6 and GCC 8 on Debian I don't get any warnings on master or
36da893114.

With Clang 6 on OpenBSD I get warnings about the unused khash functions
in delta-islands.c on master, though.

What do you get, or in other words: why did you send this patch?

> Signed-off-by: Carlo Marcelo Arenas Belón 
> ---
>  khash.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/khash.h b/khash.h
> index d10caa0c35..39c2833877 100644
> --- a/khash.h
> +++ b/khash.h
> @@ -234,7 +234,7 @@ static const double __ac_HASH_UPPER = 0.77;
>   __KHASH_IMPL(name, SCOPE, khkey_t, khval_t, kh_is_map, __hash_func, 
> __hash_equal)
>  
>  #define KHASH_INIT(name, khkey_t, khval_t, kh_is_map, __hash_func, 
> __hash_equal) \
> - KHASH_INIT2(name, static inline, khkey_t, khval_t, kh_is_map, 
> __hash_func, __hash_equal)
> + KHASH_INIT2(name, __attribute__((__unused__)) static inline, khkey_t, 
> khval_t, kh_is_map, __hash_func, __hash_equal)
>  
>  /* Other convenient macros... */
>  

This fixes the warning on OpenBSD for me.  So does moving the KHASH_INIT
line from delta-islands.c to a header file (e.g. khash.h or
delta-islands.h).

René

[PATCH 1/3] [Outreachy] t3903-stash: test without configured user name

[Slavica]

This is part of enhancement request that ask for `git stash` to work even if 
`user.name` is not configured.
The issue is discussed here: 
https://public-inbox.org/git/87o9debty4@evledraar.gmail.com/T/#u.

Signed-off-by: Slavica 
---
 t/t3903-stash.sh | 17 +
 1 file changed, 17 insertions(+)

diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 9e06494ba0..9ff34a65bc 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -1156,4 +1156,21 @@ test_expect_success 'stash --  works with binary 
files' '
test_path_is_file subdir/untracked
 '
 
+test_expect_failure 'stash with HOME as non-existing directory' '
+test_commit 1 &&
+test_config user.useconfigonly true &&
+test_config stash.usebuiltin true &&
+(
+HOME=$(pwd)/none &&
+export HOME &&
+unset GIT_AUTHOR_NAME &&
+unset GIT_AUTHOR_EMAIL &&
+unset GIT_COMMITTER_NAME &&
+unset GIT_COMMITTER_EMAIL &&
+test_must_fail git config user.email &&
+echo changed >1.t &&
+   git stash
+)
+'
+
 test_done
-- 
2.19.1.windows.1

Re: [PATCH] Poison gettext with the Ook language

[Ævar Arnfjörð Bjarmason]

On Tue, Oct 23 2018, Duy Nguyen wrote:

> On Tue, Oct 23, 2018 at 12:17 PM Ævar Arnfjörð Bjarmason
>  wrote:
>>
>>
>> On Tue, Oct 23 2018, Johannes Schindelin wrote:
>>
>> > Hi Ævar,
>> >
>> > On Mon, 22 Oct 2018, Ævar Arnfjörð Bjarmason wrote:
>> >
>> >> So I think the only reason to keep it [GETTEXT_POISON] compile-time is
>> >> performance, but I don't think that matters. It's not like we're
>> >> printing gigabytes of _() formatted output. Everything where formatting
>> >> matters is plumbing which doesn't use this API. These messages are
>> >> always for human consumption.
>> >
>> > Well, let's make sure that your impression is correct before going too
>> > far. I, too, had the impression that gettext cannot possibly be expensive,
>> > especifally in Git for Windows' settings, where we do not even ship
>> > translations. Yet see the commit message of cc5e1bf99247 (gettext: avoid
>> > initialization if the locale dir is not present, 2018-04-21):
>> >
>> >   The runtime of a simple `git.exe version` call on Windows is
>> >   currently dominated by the gettext setup, adding a whopping ~150ms
>> >   to the ~210ms total.
>> >
>> > I would be in favor of your change to make this a runtime option, of
>> > course, as long as it does not affect performance greatly (in particular
>> > on Windows, where we fight an uphill battle to make Git faster).
>>
>> How expensive gettext() may or may not be isn't relevant to the
>> GETTEXT_POISON compile-time option.
>
> If a user requests NO_GETTEXT, they should have zero (or near zero)
> cost related to gettext. Which is true until now (the inline _()
> version is expanded to pretty much no-op with the exception of NULL
> string)

I'm assuming by "until now" you mean until my RFC patch in
https://public-inbox.org/git/875zxtd59e@evledraar.gmail.com/

Yeah it's more expensive, but I don't see how it matters. We'd be
nano-optimizing a thing that's never going to become a
bottleneck. I.e. the patch is basically taking the commented-out
codepath in this test program:

#include 
#include 
#include 

int have_flag(void)
{
return 0;
/*
static int flag = -1;
if (flag == -1)
flag = strlen(getenv("GIT_TEST_FOO"));
return flag;
*/
}


int main(void)
{
while (1) {
const char *out = have_flag() ? "foo" : "bar";
puts(out);
}
}

When I test that on my laptop with gcc 8.2.0 I get ~230MB/s of output on
both versions, i.e. where have_flag() can be trivially constant folded,
and where we need to call getenv() for both of:

GIT_TEST_FOO= ./main | pv >/dev/null
GIT_TEST_FOO=1 ./main | pv >/dev/null

We don't have any codepaths where we're calling gettext() to output more
than a screenfull or two of output, so optimizing this doesn't make
sense.

>> The effect of what I'm suggesting here, and which my WIP patch in
>> <875zxtd59e@evledraar.gmail.com> implements is that we'd do a
>> one-time getenv() for each process that prints a _() message that we
>> aren't doing now, and for each message call a function that would check
>> a boolean "are we in poison mode" static global variable.
>
> Just don't do the getenv() inside _() even if you just do it one time.
> getenv() may invalidate whatever value from the previous call. I would
> not want to find out my code broke because of an getenv() inside some
> innocent _()...

How would any code break? We have various getenv("GIT_TEST_*...")
already that work the same way. Unless you set that in the environment
the function is idempotent, and I don't see how anyone would do that
accidentally.

> And we should still respect NO_GETTEXT, not doing any extra work when
> it's defined.

This is not how any of our NO_* defines work. *Sometimes* defining them
guarantees you do no extra work, but in other cases we've decided that
bending over backwards with #ifdef in various places isn't worth it.

E.g. grep_opt in grep.h is a good example of this. Even if you don't
compile with PCRE you're pointlessly memzero-ing out members of the
struct that'll never be used in your config, because it's easier to
maintain the code that way (types always checked at compile-time).

(And no, despite my involvement in PCRE I didn't introduce that
particular pattern)

For the reasons noted above I think NO_GETTEXT is another such
case. Just like GIT_TEST_SPLIT_INDEX (added in your d6e3c181bc
("read-cache: force split index mode with GIT_TEST_SPLIT_INDEX",
2014-06-13)) etc.

Re: [PATCH] khash: silence -Wunused-function

[Carlo Arenas]

On Tue, Oct 23, 2018 at 9:19 AM René Scharfe  wrote:

> With Clang 6 and GCC 8 on Debian I don't get any warnings on master or
> 36da893114.

I see it with Clang 7 on Fedora (at least Rawhide but suspect also to
affect the next release, now in beta: 29)

> With Clang 6 on OpenBSD I get warnings about the unused khash functions
> in delta-islands.c on master, though.

Apple LLVM 9 and 10 will also trigger similar warnings to what you see
in OpenBSD and as Junio mentioned recently in a different thread about
semantic patches, this currently affects the CI for "Apple":

  https://travis-ci.org/git/git/builds/444969342

Carlo

Re: [PATCH v2 3/3] rebase (autostash): use an explicit OID to apply the stash

[Alban Gruin]

Hi Johannes,

this looks good to me, too!

Cheers,
Alban

Re: [RFC 0/2] explicitly support or not support --exclude-promisor-objects

[Matthew DeVore]

On Tue, 23 Oct 2018, Junio C Hamano wrote:


Thanks; both patches make sense.

As the problematic feature appeared in 2.17.x track, I'll see if I
can easily make it ready to be merged down to maint-2.17 track later
when somebody wants to.



Great. Thank you for the review.

Re: [PATCH v3 3/3] reset: warn when refresh_index() takes more than 2 seconds

[Ben Peart]

On 10/22/2018 8:23 PM, Junio C Hamano wrote:

Ben Peart  writes:


From: Ben Peart 

refresh_index() is done after a reset command as an optimization.  Because
it can be an expensive call, warn the user if it takes more than 2 seconds
and tell them how to avoid it using the --quiet command line option or
reset.quiet config setting.


I am moderately negative on this step.  It will irritate users who
know about and still choose not to use the "--quiet" option, because
they want to gain performance in later real work and/or they want to
know what paths are now dirty.  A working tree that needs long time
to refresh will take long time to instead do "cached stat info says
it may be modified so let's run 'diff' for real---we may discover
that there wasn't any change after all" when a "git diff" is run
after a "reset --quiet" that does not refresh; i.e. there would be
valid reasons to run "reset" without "--quiet".

It feels a bit irresponsible to throw an ad without informing
pros-and-cons and to pretend that we are advising on BCP.  In
general, we do *not* advertise new features randomly like this.

Thanks.  The previous two steps looks quite sensible.



The challenge I'm trying to address is the discoverability of this 
significant performance win.  In earlier review feedback, all mention of 
this option speeding up reset was removed.  I added this patch to enable 
users to find out it even exists as an option.


While I modeled this on the untracked files/--uno and ahead/behind 
logic, I missed adding this to the 'advice' logic so that it can be 
turned off and avoid irritating users.  I'll send an updated patch that 
corrects that.

Re: New semantic patches vs. in-flight topics [was: Re: [PATCH 00/19] Bring more repository handles into our code base]

[Stefan Beller]

On Tue, Oct 23, 2018 at 2:38 AM Junio C Hamano  wrote:
>
> Junio C Hamano  writes:
>
> > I actually think this round does a far nicer job playing well with
> > other topics than any earlier series.  The pain you are observing I
> > think come primarily from my not making the best use of these
> > patches.
> >
> > Steppng back a bit, I'd imagine in an ideal world where "make
> > coccicheck" can be done instantaneously _and_ the spatch machinery
> > is just as reliable as C compilers
> >
> > What I _could_ do (and what I did do only for one round of pushing
> > out 'pu') is to queue a coccinelle transformation at the tip of
> > integration branches.  If "make coccicheck" ran in subsecond, I
> > could even automate it in the script that is used to rebuild 'pu'
> > every day, ...
>
> Anyway, even though "make coccicheck" does not run in subsecond,
> I've updated my machinery to rebuild the integration branches so
> that I can optionally queue generated coccicheck patches, and what I
> pushed out tonight has one at the tip of 'pu' and also another at
> the tip of 'next'.  The latter seems to be passing all archs and
> executing Windows run.

That is pretty exciting!

Looking at the commit in next, you also included the suggestion
from [1] to use a postincrement instead of a preincrement and I got
excited to see how we express such a thing in coccinelle,
but it turns out that it slipped in unrelated to the coccinelle patches.

How would we go from here?
It is not obvious to me how such changes would be integrated,
as regenerating them on top of pu will not help getting these changes
merged down, and applying the semantic patch on next (once
sb/more-repo-in-api lands in next) would created the merge conflicts for
all topics that are merged to next after that series.

[1] https://public-inbox.org/git/xmqqin1wyxvz@gitster-ct.c.googlers.com/

Re: [PATCH v3 2/3] reset: add new reset.quiet config setting

[Ben Peart]

On 10/22/2018 4:06 PM, Jeff King wrote:

On Mon, Oct 22, 2018 at 08:13:32PM +0100, Ramsay Jones wrote:


  -q::
  --quiet::
-   Be quiet, only report errors.
+--no-quiet::
+   Be quiet, only report errors. The default behavior respects the
+   `reset.quiet` config option, or `--no-quiet` if that is not set.


Sorry, I can't quite parse this; -q,--quiet and --no-quiet on the
command line (should) trump whatever rest.quiet is set to in the
configuration. Is that not the case?


That is the case, and what was meant by "the default behavior" (i.e.,
the behavior when none of these is used). Maybe there's a more clear way
of saying that.

-Peff



Is this more clear?

-q::
--quiet::
--no-quiet::
Be quiet, only report errors. The default behavior is set by the
`reset.quiet` config option. `--quiet` and `--no-quiet` will
overwrite the default behavior.

Re: [PATCH v3 2/3] reset: add new reset.quiet config setting

[Jeff King]

On Tue, Oct 23, 2018 at 01:31:58PM -0400, Ben Peart wrote:

> On 10/22/2018 4:06 PM, Jeff King wrote:
> > On Mon, Oct 22, 2018 at 08:13:32PM +0100, Ramsay Jones wrote:
> > 
> > > >   -q::
> > > >   --quiet::
> > > > -   Be quiet, only report errors.
> > > > +--no-quiet::
> > > > +   Be quiet, only report errors. The default behavior respects the
> > > > +   `reset.quiet` config option, or `--no-quiet` if that is not set.
> > > 
> > > Sorry, I can't quite parse this; -q,--quiet and --no-quiet on the
> > > command line (should) trump whatever rest.quiet is set to in the
> > > configuration. Is that not the case?
> > 
> > That is the case, and what was meant by "the default behavior" (i.e.,
> > the behavior when none of these is used). Maybe there's a more clear way
> > of saying that.
> > 
> 
> Is this more clear?
> 
> -q::
> --quiet::
> --no-quiet::
>   Be quiet, only report errors. The default behavior is set by the
>   `reset.quiet` config option. `--quiet` and `--no-quiet` will
>   overwrite the default behavior.

That looks OK to me (but then so did the earlier one ;) ).

I'd probably s/overwrite/override/.

-Peff

Re: [RFC 2/2] exclude-promisor-objects: declare when option is allowed

[Matthew DeVore]

On Tue, 23 Oct 2018, Junio C Hamano wrote:


Not really.  We were already doing a controlled failure via die(),
so these two tests would not have caught the problem in the code
before the fix in this patch.



BUG is apparently considered a "wrong" failure and not a controlled one
by test_must_fail. I just double-checked that the tests fail without
this patch.

not ok 119 - --exclude-promisor-objects does not BUG-crash
# 
#		test_must_fail git blame --exclude-promisor-objects one
# 
# failed 1 among 119 test(s)

1..119

Re: [PATCH v1 2/2] reset: add new reset.quietDefault config setting

[Ben Peart]

On 10/23/2018 5:13 AM, Ævar Arnfjörð Bjarmason wrote:


On Wed, Oct 17 2018, Jeff King wrote:


On Wed, Oct 17, 2018 at 02:19:59PM -0400, Eric Sunshine wrote:


On Wed, Oct 17, 2018 at 12:40 PM Ben Peart  wrote:

Add a reset.quietDefault config setting that sets the default value of the
--quiet flag when running the reset command.  This enables users to change
the default behavior to take advantage of the performance advantages of
avoiding the scan for unstaged changes after reset.  Defaults to false.


As with the previous patch, my knee-jerk reaction is that this really
feels wrong being tied to --quiet. It's particularly unintuitive.

What I _could_ see, and what would feel more natural is if you add a
new option (say, --optimize) which is more general, incorporating
whatever optimizations become available in the future, not just this
one special-case. A side-effect of --optimize is that it implies
--quiet, and that is something which can and should be documented.


Heh, I just wrote something very similar elsewhere in the thread. I'm
still not sure if it's a dumb idea, but at least we can be dumb
together.


Same here. I'm in general if favor of having the ability to configure
porcelain command-line options, but in this case it seems like it would
be more logical to head for something like:

 core.uiMessaging=[default,exhaustive,lossyButFaster,quiet]

Where default would be our current "exhaustive", and this --quiet case
would be covered by lossyButFaster, but also things like the
"--no-ahead-behind" flag for git-status.



This sounds like an easy way to choose a set of default values that we 
think make sense to get bundled together. That could be a way for users 
to quickly choose a set of good defaults but I still think you would 
want find grained control over the individual settings.


Coming up with the set of values to bundle together, figuring out the 
hierarchy of precedence for this new global config->individual 
config->individual command line, updating the code to make it all work 
is outside the scope of this particular patch series.



Just on this implementation: The usual idiom for flags as config is
command.flag=xyz, not command.flagDefault=xyz, so this should be
reset.quiet.



Thanks, I agree and fixed that in later iterations.

Re: [PATCH 7/9] submodule: fetch in submodules git directory instead of in worktree

[Stefan Beller]

On Wed, Oct 17, 2018 at 3:58 PM Jonathan Tan  wrote:
>
> > This patch started as a refactoring to make 'get_next_submodule' more
> > readable, but upon doing so, I realized that "git fetch" of the submodule
> > actually doesn't need to be run in the submodules worktree. So let's run
> > it in its git dir instead.
>
> The commit message needs to be updated, I think - this patch does
> significantly more than fetching in the gitdir.

>From my point of view, it is not significant, but refactoring.
I'll think how to write a better commit message.

> > This patch leaks the cp->dir in get_next_submodule, as any further
> > callback in run_processes_parallel doesn't have access to the child
> > process any more.
>
> The cp->dir is already leaked - probably better to write "cp->dir in
> get_next_submodule() is still leaked, but this will be fixed in a
> subsequent patch".

... which fails to mention the reason why (as it is hard to do given
the current design) but is more concise.

> > +static void prepare_submodule_repo_env_in_gitdir(struct argv_array *out)
> > +{
> > + prepare_submodule_repo_env_no_git_dir(out);
> > + argv_array_pushf(out, "%s=.", GIT_DIR_ENVIRONMENT);
>
> Why does GIT_DIR need to be set? Is it to avoid subcommands recursively
> checking the parent directories in case the CWD is a malformed Git
> repository? If yes, maybe it's worth adding a comment.

It is copying the structure from prepare_submodule_repo_env,
specifically 10f5c52656 (submodule: avoid auto-discovery in
prepare_submodule_repo_env(), 2016-09-01), which sounds
appealing (and brings real benefits for the working directory),
but I have not thought about this protection for the git dir.

Maybe another approach is to not set the cwd for the child process
and instead point GIT_DIR_ENVIRONMENT only to the right
directory.

Then the use of GIT_DIR_ENVIRONMENT is obvious and
is not just for protection of corner cases.

However I think this protection is really valuable for the
.git dir as well as the submodule may be broken and we do not
want to end up in an infinite loop (as the discovery would find
the superproject which then tries to recurse, again, into the
submodule with the broken git dir)

When adding the comment here, we'd also want to have
the comment in prepare_submodule_repo_env, which
could be its own preparation commit.

> > +static struct repository *get_submodule_repo_for(struct repository *r,
> > +  const struct submodule *sub)
> > +{
> > + struct repository *ret = xmalloc(sizeof(*ret));
> > +
> > + if (repo_submodule_init(ret, r, sub)) {
> > + /*
> > +  * No entry in .gitmodules? Technically not a submodule,
> > +  * but historically we supported repositories that happen to 
> > be
> > +  * in-place where a gitlink is. Keep supporting them.
> > +  */
> > + struct strbuf gitdir = STRBUF_INIT;
> > + strbuf_repo_worktree_path(&gitdir, r, "%s/.git", sub->path);
> > + if (repo_init(ret, gitdir.buf, NULL)) {
> > + strbuf_release(&gitdir);
> > + return NULL;
> > + }
> > + strbuf_release(&gitdir);
> > + }
> > +
> > + return ret;
> > +}
>
> This is the significant thing that this patch does more - an unskipped
> submodule is now something that either passes the checks in
> repo_submodule_init() or the checks in repo_init(), which seems to be
> stricter than the current check that ".git" points to a directory or is
> one. This means that we skip certain broken repositories, and this
> necessitates a change in the test.

I see. However there is no change in function, the check in repo_init
(or repo_submodule_init) is less strict than the check in the child process.
So if there are broken submodule repositories, the difference of this
patch is the layer at which it is caught, i.e. we would not spawn a child
that fails, but skip the submodule.

Thinking of that, maybe we need to announce that in get_next_submodule

>
> I think we should be more particular about what we're allowed to skip -
> in particular, maybe if we're planning to skip this submodule, its
> corresponding directory in the worktree (if one exists) needs to be
> empty.

If the working tree directory is empty for that submodule, it means
it is likely not initialized. But why would we use that as a signal to
skip the submodule?



> > - cp->dir = strbuf_detach(&submodule_path, NULL);
> > - prepare_submodule_repo_env(&cp->env_array);
> > + prepare_submodule_repo_env_in_gitdir(&cp->env_array);
> > + cp->dir = xstrdup(repo->gitdir);
>
> Here is where the functionality change (fetch in ".git") described in
> the commit message occurs.

True.

Thanks for the review, I'll try to split up this commit a bit more and
explain each part on its own.

Re: [PATCH v3 2/3] reset: add new reset.quiet config setting

[Ben Peart]

On 10/22/2018 10:45 AM, Duy Nguyen wrote:

On Mon, Oct 22, 2018 at 3:38 PM Ben Peart  wrote:


From: Ben Peart 

Add a reset.quiet config setting that sets the default value of the --quiet
flag when running the reset command.  This enables users to change the
default behavior to take advantage of the performance advantages of
avoiding the scan for unstaged changes after reset.  Defaults to false.

Signed-off-by: Ben Peart 
---
  Documentation/config.txt| 3 +++
  Documentation/git-reset.txt | 4 +++-
  builtin/reset.c | 1 +
  3 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index f6f4c21a54..a2d1b8b116 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2728,6 +2728,9 @@ rerere.enabled::
 `$GIT_DIR`, e.g. if "rerere" was previously used in the
 repository.

+reset.quiet::
+   When set to true, 'git reset' will default to the '--quiet' option.
+


With 'nd/config-split' topic moving pretty much all config keys out of
config.txt, you probably want to do the same for this series: add this
in a new file called Documentation/reset-config.txt then include the
file here like the sendemail one below.



Seems a bit overkill to pull a line of documentation into a separate 
file and replace it with a line of 'import' logic.  Perhaps if/when 
there is more documentation to pull out that would make more sense.



  include::sendemail-config.txt[]

  sequence.editor::

Re: [PATCH 1/3] [Outreachy] t3903-stash: test without configured user name

[Christian Couder]

On Tue, Oct 23, 2018 at 6:35 PM Slavica  wrote:
>
> This is part of enhancement request that ask for `git stash` to work even if 
> `user.name` is not configured.
> The issue is discussed here: 
> https://public-inbox.org/git/87o9debty4@evledraar.gmail.com/T/#u.

We prefer commit messages that contain as much as possible all the
information necessary to understand the patch without links to other
places.

It seems that only this email from you reached me. Did you send other
emails for patches 2/3 and 3/3?

[...]

> +(
> +HOME=$(pwd)/none &&
> +export HOME &&
> +unset GIT_AUTHOR_NAME &&
> +unset GIT_AUTHOR_EMAIL &&
> +unset GIT_COMMITTER_NAME &&
> +unset GIT_COMMITTER_EMAIL &&
> +test_must_fail git config user.email &&
> +echo changed >1.t &&
> +   git stash

It seems that the above line is not indented like the previous ones.

> +)
> +'

Thanks for contributing,
Christian.

[PATCH v4 2/3] reset: add new reset.quiet config setting

[Ben Peart]

From: Ben Peart 

Add a reset.quiet config setting that sets the default value of the --quiet
flag when running the reset command.  This enables users to change the
default behavior to take advantage of the performance advantages of
avoiding the scan for unstaged changes after reset.  Defaults to false.

Signed-off-by: Ben Peart 
---
 Documentation/config.txt| 3 +++
 Documentation/git-reset.txt | 5 -
 builtin/reset.c | 1 +
 3 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index f6f4c21a54..a2d1b8b116 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2728,6 +2728,9 @@ rerere.enabled::
`$GIT_DIR`, e.g. if "rerere" was previously used in the
repository.
 
+reset.quiet::
+   When set to true, 'git reset' will default to the '--quiet' option.
+
 include::sendemail-config.txt[]
 
 sequence.editor::
diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
index 1d697d9962..2dac95c71a 100644
--- a/Documentation/git-reset.txt
+++ b/Documentation/git-reset.txt
@@ -95,7 +95,10 @@ OPTIONS
 
 -q::
 --quiet::
-   Be quiet, only report errors.
+--no-quiet::
+   Be quiet, only report errors. The default behavior is set by the
+   `reset.quiet` config option. `--quiet` and `--no-quiet` will
+   override the default behavior.
 
 
 EXAMPLES
diff --git a/builtin/reset.c b/builtin/reset.c
index 04f0d9b4f5..3b43aee544 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -306,6 +306,7 @@ int cmd_reset(int argc, const char **argv, const char 
*prefix)
};
 
git_config(git_reset_config, NULL);
+   git_config_get_bool("reset.quiet", &quiet);
 
argc = parse_options(argc, argv, prefix, options, git_reset_usage,
PARSE_OPT_KEEP_DASHDASH);
-- 
2.18.0.windows.1

[PATCH v4 0/3] speed up git reset

[Ben Peart]

From: Ben Peart 

Updated the wording in the documentation and commit messages to (hopefully)
make it clearer. Added the warning about 'reset --quiet' to the advice
system so that it can be turned off.

Base Ref: 
Web-Diff: https://github.com/benpeart/git/commit/8a2fef45d4
Checkout: git fetch https://github.com/benpeart/git reset-refresh-index-v4 && 
git checkout 8a2fef45d4


### Patches

Ben Peart (3):
  reset: don't compute unstaged changes after reset when --quiet
  reset: add new reset.quiet config setting
  reset: warn when refresh_index() takes more than 2 seconds

 Documentation/config.txt|  7 +++
 Documentation/git-reset.txt |  5 -
 advice.c|  2 ++
 advice.h|  1 +
 builtin/reset.c | 15 ++-
 5 files changed, 28 insertions(+), 2 deletions(-)


base-commit: ca63497355222acefcca02b9cbb540a4768f3286
-- 
2.18.0.windows.1

[PATCH v4 1/3] reset: don't compute unstaged changes after reset when --quiet

[Ben Peart]

From: Ben Peart 

When git reset is run with the --quiet flag, don't bother finding any
additional unstaged changes as they won't be output anyway.  This speeds up
the git reset command by avoiding having to lstat() every file looking for
changes that aren't going to be reported anyway.

The savings can be significant.  In a repo on Windows with 200K files
"git reset" drops from 7.16 seconds to 0.32 seconds for a savings of 96%.

Signed-off-by: Ben Peart 
---
 builtin/reset.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/reset.c b/builtin/reset.c
index 11cd0dcb8c..04f0d9b4f5 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -375,7 +375,7 @@ int cmd_reset(int argc, const char **argv, const char 
*prefix)
int flags = quiet ? REFRESH_QUIET : 
REFRESH_IN_PORCELAIN;
if (read_from_tree(&pathspec, &oid, intent_to_add))
return 1;
-   if (get_git_work_tree())
+   if (!quiet && get_git_work_tree())
refresh_index(&the_index, flags, NULL, NULL,
  _("Unstaged changes after 
reset:"));
} else {
-- 
2.18.0.windows.1

[PATCH v4 3/3] reset: warn when refresh_index() takes more than 2 seconds

[Ben Peart]

From: Ben Peart 

refresh_index() is done after a reset command as an optimization.  Because
it can be an expensive call, warn the user if it takes more than 2 seconds
and tell them how to avoid it using the --quiet command line option or
reset.quiet config setting.

Signed-off-by: Ben Peart 
---
 Documentation/config.txt |  4 
 advice.c |  2 ++
 advice.h |  1 +
 builtin/reset.c  | 14 +-
 4 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index a2d1b8b116..415db31def 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -333,6 +333,10 @@ advice.*::
commitBeforeMerge::
Advice shown when linkgit:git-merge[1] refuses to
merge to avoid overwriting local changes.
+   resetQuiet::
+   Advice to consider using the `--quiet` option to 
linkgit:git-reset[1]
+   when the command takes more than 2 seconds to enumerate unstaged
+   changes after reset.
resolveConflict::
Advice shown by various commands when conflicts
prevent the operation from being performed.
diff --git a/advice.c b/advice.c
index 3561cd64e9..5f35656409 100644
--- a/advice.c
+++ b/advice.c
@@ -12,6 +12,7 @@ int advice_push_needs_force = 1;
 int advice_status_hints = 1;
 int advice_status_u_option = 1;
 int advice_commit_before_merge = 1;
+int advice_reset_quiet_warning = 1;
 int advice_resolve_conflict = 1;
 int advice_implicit_identity = 1;
 int advice_detached_head = 1;
@@ -65,6 +66,7 @@ static struct {
{ "statusHints", &advice_status_hints },
{ "statusUoption", &advice_status_u_option },
{ "commitBeforeMerge", &advice_commit_before_merge },
+   { "resetQuiet", &advice_reset_quiet_warning },
{ "resolveConflict", &advice_resolve_conflict },
{ "implicitIdentity", &advice_implicit_identity },
{ "detachedHead", &advice_detached_head },
diff --git a/advice.h b/advice.h
index ab24df0fd0..696bf0e7d2 100644
--- a/advice.h
+++ b/advice.h
@@ -12,6 +12,7 @@ extern int advice_push_needs_force;
 extern int advice_status_hints;
 extern int advice_status_u_option;
 extern int advice_commit_before_merge;
+extern int advice_reset_quiet_warning;
 extern int advice_resolve_conflict;
 extern int advice_implicit_identity;
 extern int advice_detached_head;
diff --git a/builtin/reset.c b/builtin/reset.c
index 3b43aee544..b31a0eae8a 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -25,6 +25,8 @@
 #include "submodule.h"
 #include "submodule-config.h"
 
+#define REFRESH_INDEX_DELAY_WARNING_IN_MS (2 * 1000)
+
 static const char * const git_reset_usage[] = {
N_("git reset [--mixed | --soft | --hard | --merge | --keep] [-q] 
[]"),
N_("git reset [-q] [] [--] ..."),
@@ -376,9 +378,19 @@ int cmd_reset(int argc, const char **argv, const char 
*prefix)
int flags = quiet ? REFRESH_QUIET : 
REFRESH_IN_PORCELAIN;
if (read_from_tree(&pathspec, &oid, intent_to_add))
return 1;
-   if (!quiet && get_git_work_tree())
+   if (!quiet && get_git_work_tree()) {
+   uint64_t t_begin, t_delta_in_ms;
+
+   t_begin = getnanotime();
refresh_index(&the_index, flags, NULL, NULL,
  _("Unstaged changes after 
reset:"));
+   t_delta_in_ms = (getnanotime() - t_begin) / 
100;
+   if (advice_reset_quiet_warning && t_delta_in_ms 
> REFRESH_INDEX_DELAY_WARNING_IN_MS) {
+   printf(_("\nIt took %.2f seconds to 
enumerate unstaged changes after reset.  You can\n"
+   "use '--quiet' to avoid this.  
Set the config setting reset.quiet to true\n"
+   "to make this the default.\n"), 
t_delta_in_ms / 1000.0);
+   }
+   }
} else {
int err = reset_index(&oid, reset_type, quiet);
if (reset_type == KEEP && !err)
-- 
2.18.0.windows.1

Re: [PATCH v1] load_cache_entries_threaded: remove unused src_offset parameter

[Ben Peart]

On 10/22/2018 7:05 PM, Junio C Hamano wrote:

Jeff King  writes:


If nobody uses it, should we drop the return value, too? Like:


Yup.



I'm good with that.

At one point I also had the additional #ifndef NO_PTHREADS lines but it 
was starting to get messy with the threaded vs non-threaded code paths 
so I removed them.  I'm fine with which ever people find more readable.


It does make me wonder if there are still platforms taking new build of 
git that don't support threads.  Do we still need to 
write/test/debug/read through the single threaded code paths?


Is the diff Peff sent enough or do you want me to send another iteration 
on the patch?


Thanks,

Ben



diff --git a/read-cache.c b/read-cache.c
index 78c9516eb7..4b44a2eae5 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2052,12 +2052,11 @@ static void *load_cache_entries_thread(void *_data)
return NULL;
  }
  
-static unsigned long load_cache_entries_threaded(struct index_state *istate, const char *mmap, size_t mmap_size,

-int nr_threads, struct 
index_entry_offset_table *ieot)
+static void load_cache_entries_threaded(struct index_state *istate, const char 
*mmap, size_t mmap_size,
+   int nr_threads, struct 
index_entry_offset_table *ieot)
  {
int i, offset, ieot_blocks, ieot_start, err;
struct load_cache_entries_thread_data *data;
-   unsigned long consumed = 0;
  
  	/* a little sanity checking */

if (istate->name_hash_initialized)
@@ -2115,12 +2114,9 @@ static unsigned long load_cache_entries_threaded(struct 
index_state *istate, con
if (err)
die(_("unable to join load_cache_entries thread: %s"), 
strerror(err));
mem_pool_combine(istate->ce_mem_pool, p->ce_mem_pool);
-   consumed += p->consumed;
}
  
  	free(data);

-
-   return consumed;
  }
  #endif
  


-Peff

[PATCH] Clear --exclude list after 'git rev-parse --all'

[Andreas Gruenbacher]

Commit [1] added the --exclude option to revision.c.  The --all,
--branches, --tags, --remotes, and --glob options clear the exclude
list.  Shortly therafter, commit [2] added the same to 'git rev-parse',
but without clearing the exclude list for the --all option.  Fix that.

[1] e7b432c52 ("revision: introduce --exclude= to tame wildcards", 
2013-08-30)
[2] 9dc01bf06 ("rev-parse: introduce --exclude= to tame wildcards", 
2013-11-01)

Signed-off-by: Andreas Gruenbacher 
---
 builtin/rev-parse.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 0f09bbbf6..c71e3b104 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -764,6 +764,7 @@ int cmd_rev_parse(int argc, const char **argv, const char 
*prefix)
}
if (!strcmp(arg, "--all")) {
for_each_ref(show_reference, NULL);
+   clear_ref_exclusion(&ref_excludes);
continue;
}
if (skip_prefix(arg, "--disambiguate=", &arg)) {
-- 
2.17.2

Re: [PATCH 1/3] [Outreachy] t3903-stash: test without configured user name

[Eric Sunshine]

On Tue, Oct 23, 2018 at 12:31 PM Slavica  wrote:
> This is part of enhancement request that ask for `git stash` to work even if 
> `user.name` is not configured.
> The issue is discussed here: 
> https://public-inbox.org/git/87o9debty4@evledraar.gmail.com/T/#u.

As Christian mentioned already, it's best to try to describe the issue
succinctly in the commit message so readers can understand it without
chasing a link. For this simple case, it should be sufficient to
explain that, due to an implementation detail, git-stash undesirably
requires 'user.name' and 'user.email' to be set, but shouldn't.

> Signed-off-by: Slavica 
> ---
> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> @@ -1156,4 +1156,21 @@ test_expect_success 'stash --  works with 
> binary files' '
> +test_expect_failure 'stash with HOME as non-existing directory' '

The purpose of this test is to demonstrate that git-stash has an
undesirable requirement that 'user.name' and 'user.email' be set. The
test title should reflect that. So, instead of talking about
non-existent HOME (which is just an implementation detail of the
test), a better test title would be something like "stash works when
user.name and user.email are not set".

> +test_commit 1 &&
> +test_config user.useconfigonly true &&
> +test_config stash.usebuiltin true &&
> +(
> +HOME=$(pwd)/none &&
> +export HOME &&
> +unset GIT_AUTHOR_NAME &&

Use sane_unset() for all of these rather than bare 'unset'.

> +unset GIT_AUTHOR_EMAIL &&
> +unset GIT_COMMITTER_NAME &&
> +unset GIT_COMMITTER_EMAIL &&
> +test_must_fail git config user.email &&
> +echo changed >1.t &&
> +   git stash

Christian already mentioned the odd indentation.

> +)
> +'

Re: [PATCH v3 1/3] reset: don't compute unstaged changes after reset when --quiet

[Johannes Schindelin]

Hi Duy,

On Tue, 23 Oct 2018, Duy Nguyen wrote:

> On Tue, Oct 23, 2018 at 1:01 AM Ben Peart  wrote:
> >
> > > -Original Message-
> > > From: Johannes Schindelin 
> > > Sent: Monday, October 22, 2018 4:45 PM
> > > To: Ben Peart 
> > > Cc: git@vger.kernel.org; gits...@pobox.com; Ben Peart
> > > ; p...@peff.net; sunsh...@sunshineco.com
> > > Subject: Re: [PATCH v3 1/3] reset: don't compute unstaged changes after
> > > reset when --quiet
> > >
> > > Hi Ben,
> > >
> > > On Mon, 22 Oct 2018, Ben Peart wrote:
> > >
> > > > From: Ben Peart 
> > > >
> > > > When git reset is run with the --quiet flag, don't bother finding any
> > > > additional unstaged changes as they won't be output anyway.  This speeds
> > > up
> > > > the git reset command by avoiding having to lstat() every file looking 
> > > > for
> > > > changes that aren't going to be reported anyway.
> > > >
> > > > The savings can be significant.  In a repo with 200K files "git reset"
> > > > drops from 7.16 seconds to 0.32 seconds for a savings of 96%.
> > >
> > > That's very nice!
> > >
> > > Those numbers, just out of curiosity, are they on Windows? Or on Linux?
> > >
> >
> > It's safe to assume all my numbers are on Windows. :-)
> 
> It does bug me about this. Next time please mention the platform you
> tested on in the commit message. Not all platforms behave the same way
> especially when it comes to performance.

And pretty much all different testing scenarios behave differently, too.
And at some stage, we're asking for too many fries.

In other words: we always accepted performance improvements when it could
be demonstrated that they improved a certain not too uncommon scenario,
and I do not think it would make sense to change this stance now. Not
unless you can demonstrate a good reason why we should.

Ciao,
Johannes

[PATCH 2/2] rebase --autostash: fix issue with dirty submodules

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin 

Since we cannot stash dirty submodules, there is no use in requiring
them to be clean (or stash them when they are not).

This brings the built-in rebase in line with the previous, scripted
version, which also did not care about dirty submodules (but it was
admittedly not very easy to figure that out).

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

Signed-off-by: Johannes Schindelin 
---
 builtin/rebase.c| 2 +-
 t/t3420-rebase-autostash.sh | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 09f55bfb9..1dd24301f 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1349,7 +1349,7 @@ int cmd_rebase(int argc, const char **argv, const char 
*prefix)
update_index_if_able(&the_index, &lock_file);
rollback_lock_file(&lock_file);
 
-   if (has_unstaged_changes(0) || has_uncommitted_changes(0)) {
+   if (has_unstaged_changes(1) || has_uncommitted_changes(1)) {
const char *autostash =
state_dir_path("autostash", &options);
struct child_process stash = CHILD_PROCESS_INIT;
diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh
index 7eb9f9041..f355c6825 100755
--- a/t/t3420-rebase-autostash.sh
+++ b/t/t3420-rebase-autostash.sh
@@ -351,7 +351,7 @@ test_expect_success 'autostash is saved on editor failure 
with conflict' '
test_cmp expected file0
 '
 
-test_expect_failure 'autostash with dirty submodules' '
+test_expect_success 'autostash with dirty submodules' '
test_when_finished "git reset --hard && git checkout master" &&
git checkout -b with-submodule &&
git submodule add ./ sub &&
-- 
gitgitgadget

[PATCH 0/2] Demonstrate and fix a rebase --autostash bug with dirty submodules

[Johannes Schindelin via GitGitGadget]

This bug report came in via Git for Windows (already with version 2.19.0,
but I misread the reporter's enthusiasm to take matters into his own hands).

The culprit is, in a nutshell, that the built-in rebase tries to run git
stash only when the worktree is dirty, but it includes submodules in that.
However, git stash cannot do anything about submodules, and if the only
changes are in submodules, then it won't even give us back an OID, and the
built-in rebase acts surprised.

The solution is easy: simply exclude the submodules from the question
whether the worktree is dirty.

What is surprisingly not simple is to get the regression test right. For
that reason, and because I firmly believe that it is easier to verify a fix
for a regression when the regression test is introduced separately (i.e.
making it simple to verify that there is a regression), I really want to
keep the first patch separate from the second one.

Since this bug concerns the built-in rebase, I based the patches on top of 
next.

Johannes Schindelin (2):
  rebase --autostash: demonstrate a problem with dirty submodules
  rebase --autostash: fix issue with dirty submodules

 builtin/rebase.c|  2 +-
 t/t3420-rebase-autostash.sh | 10 ++
 2 files changed, 11 insertions(+), 1 deletion(-)


base-commit: 209f214ca4ae4e301fc32e59ab26f937082f3ea3
Published-As: 
https://github.com/gitgitgadget/git/releases/tags/pr-56%2Fdscho%2Ffix-built-in-rebase-autostash-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git 
pr-56/dscho/fix-built-in-rebase-autostash-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/56
-- 
gitgitgadget

[PATCH 1/2] rebase --autostash: demonstrate a problem with dirty submodules

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin 

It has been reported that dirty submodules cause problems with the
built-in rebase when it is asked to autostash. The symptom is:

fatal: Unexpected stash response: ''

This patch adds a regression test that demonstrates that bug.

Original report: https://github.com/git-for-windows/git/issues/1820

Signed-off-by: Johannes Schindelin 
---
 t/t3420-rebase-autostash.sh | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh
index 0c4eefec7..7eb9f9041 100755
--- a/t/t3420-rebase-autostash.sh
+++ b/t/t3420-rebase-autostash.sh
@@ -351,4 +351,14 @@ test_expect_success 'autostash is saved on editor failure 
with conflict' '
test_cmp expected file0
 '
 
+test_expect_failure 'autostash with dirty submodules' '
+   test_when_finished "git reset --hard && git checkout master" &&
+   git checkout -b with-submodule &&
+   git submodule add ./ sub &&
+   test_tick &&
+   git commit -m add-submodule &&
+   echo changed >sub/file0 &&
+   git rebase -i --autostash HEAD
+'
+
 test_done
-- 
gitgitgadget

Re: [PATCH v1 2/2] reset: add new reset.quietDefault config setting

[Jeff King]

On Tue, Oct 23, 2018 at 02:11:01PM -0400, Ben Peart wrote:

> This sounds like an easy way to choose a set of default values that we think
> make sense to get bundled together. That could be a way for users to quickly
> choose a set of good defaults but I still think you would want find grained
> control over the individual settings.
> 
> Coming up with the set of values to bundle together, figuring out the
> hierarchy of precedence for this new global config->individual
> config->individual command line, updating the code to make it all work is
> outside the scope of this particular patch series.

True, it probably does make sense to give individual defaults. Having a
unifying option may help with the discoverability issue you were
thinking of elsewhere, though.

-Peff

Re: [PATCH v1 2/2] reset: add new reset.quietDefault config setting

[Ævar Arnfjörð Bjarmason]

On Tue, Oct 23 2018, Ben Peart wrote:

> On 10/23/2018 5:13 AM, Ævar Arnfjörð Bjarmason wrote:
>>
>> On Wed, Oct 17 2018, Jeff King wrote:
>>
>>> On Wed, Oct 17, 2018 at 02:19:59PM -0400, Eric Sunshine wrote:
>>>
 On Wed, Oct 17, 2018 at 12:40 PM Ben Peart  wrote:
> Add a reset.quietDefault config setting that sets the default value of the
> --quiet flag when running the reset command.  This enables users to change
> the default behavior to take advantage of the performance advantages of
> avoiding the scan for unstaged changes after reset.  Defaults to false.

 As with the previous patch, my knee-jerk reaction is that this really
 feels wrong being tied to --quiet. It's particularly unintuitive.

 What I _could_ see, and what would feel more natural is if you add a
 new option (say, --optimize) which is more general, incorporating
 whatever optimizations become available in the future, not just this
 one special-case. A side-effect of --optimize is that it implies
 --quiet, and that is something which can and should be documented.
>>>
>>> Heh, I just wrote something very similar elsewhere in the thread. I'm
>>> still not sure if it's a dumb idea, but at least we can be dumb
>>> together.
>>
>> Same here. I'm in general if favor of having the ability to configure
>> porcelain command-line options, but in this case it seems like it would
>> be more logical to head for something like:
>>
>>  core.uiMessaging=[default,exhaustive,lossyButFaster,quiet]
>>
>> Where default would be our current "exhaustive", and this --quiet case
>> would be covered by lossyButFaster, but also things like the
>> "--no-ahead-behind" flag for git-status.
>>
>
> This sounds like an easy way to choose a set of default values that we
> think make sense to get bundled together. That could be a way for
> users to quickly choose a set of good defaults but I still think you
> would want find grained control over the individual settings.

Would you? It seems wanting to configure reset's --quiet in particular
is purely a proxy goal for wanting to toggle off slow things in the
UI. Otherwise why focus on it, and not the plethora of other --quiet
options we have?

# Including (but probably not limited to):
$ git grep -e OPT__QUIET -e '(OPT|option).*"quiet"' -- '*.[ch]' | wc -l
34

> Coming up with the set of values to bundle together, figuring out the
> hierarchy of precedence for this new global config->individual
> config->individual command line[...]

If we'd still want reset.quiet & whatever the global "turn off slow
stuff" UI option is then this part is easy and
e.g. {transfer,fetch,receive}.fsckObjects can be used as a template for
how to do it.

https://github.com/git/git/blob/v2.19.0/fetch-pack.c#L1432-L1443
https://github.com/git/git/blob/v2.19.0/fetch-pack.c#L859-L863

I.e. the more specific option always overrides the less specific one.

> [...]updating the code to make it all work is outside the scope of
> this particular patch series.

Is that a Jedi mind trick to get out of patch review? :)

I understand that it's not the patch you wrote, but sometimes feedback
is "maybe we shouldn't do this, but this other thing".

The --ahead-behind config setting stalled on-list before:
https://public-inbox.org/git/36e3a9c3-f7e2-4100-1bfc-647b809a0...@jeffhostetler.com/

Now we have this similarly themed thing.

I think we need to be mindful of how changes like this can add up to
very confusing UI. I.e. in this case I can see a "how take make git fast
on large repos" post on stackoverflow in our future where the answer is
setting a bunch of seemingly irrelevant config options like reset.quiet
and status.aheadbehind=false etc.

So maybe we should take a step back and consider if the real thing we
want is just some way for the user to tell git "don't work so hard at
coming up with these values".

That can also be smart, e.g. some "auto" setting that tweaks it based on
estimated repo size so even with the same config your tiny dotfiles.git
will get "ahead/behind" reporting, but not when you cd into windows.git.

>> Just on this implementation: The usual idiom for flags as config is
>> command.flag=xyz, not command.flagDefault=xyz, so this should be
>> reset.quiet.
>>
>
> Thanks, I agree and fixed that in later iterations.

Re: [PATCH v1] load_cache_entries_threaded: remove unused src_offset parameter

[Jeff King]

On Tue, Oct 23, 2018 at 03:13:06PM -0400, Ben Peart wrote:

> At one point I also had the additional #ifndef NO_PTHREADS lines but it was
> starting to get messy with the threaded vs non-threaded code paths so I
> removed them.  I'm fine with which ever people find more readable.
> 
> It does make me wonder if there are still platforms taking new build of git
> that don't support threads.  Do we still need to write/test/debug/read
> through the single threaded code paths?

I think the classic offenders here were old Unix systems like AIX, etc.

I've no idea what the current state is on those platforms. I would love
it if we could drop NO_PTHREADS. There's a lot of gnarly code there, and
I strongly suspect a lot of bugs lurk in the non-threaded halves (e.g.,
especially around bits like "struct async" which is "maybe a thread, and
maybe a fork" depending on your system, which introduces all kinds of
subtle process-state dependencies).

But I'm not really sure how to find out aside from adding a deprecation
warning and seeing if anybody screams.

See also this RFC from Duy, which might at least make the code itself a
little easier to follow:

https://public-inbox.org/git/20181018180522.17642-1-pclo...@gmail.com/

-Peff

Re: [PATCH/RFC] thread-utils: better wrapper to avoid #ifdef NO_PTHREADS

[Jeff King]

On Thu, Oct 18, 2018 at 08:05:22PM +0200, Nguyễn Thái Ngọc Duy wrote:

> On Thu, Oct 18, 2018 at 7:09 PM Jeff King  wrote:
> > > In this particular case though I think we should be able to avoid so
> > > much #if if we make a wrapper for pthread api that would return an
> > > error or something when pthread is not available. But similar
> > > situation may happen elsewhere too.
> >
> > Yeah, I think that is generally the preferred method anyway, just
> > because of readability and simplicity.
> 
> I've wanted to do this for a while, so let's test the water and see if
> it's well received.
> 
> This patch is a proof of concept that adds just enough macros so that
> I can build index-pack.c on a single thread mode with zero #ifdef
> related to NO_PTHREADS.
> 
> Besides readability and simplicity, it reduces the chances of breaking
> conditional builds (e.g. you rename a variable name but forgot that
> the variable is in #if block that is not used by your
> compiler/platform).

Yes, I love this. We're already halfway there with things like
read_lock() in index-pack and elsewhere, which are conditionally no-ops.
The resulting code is much easier to read, I think.

> Performance-wise I don't think there is any loss for single thread
> mode. I rely on compilers recognizing HAVE_THREADS being a constant
> and remove dead code or at least optimize in favor of non-dead code.
> 
> Memory-wise, yes we use some more memory in single thread mode. But we
> don't have zillions of mutexes or thread id, so a bit extra memory
> does not worry me so much.

Yeah, I don't think carrying around a handful of ints is going to be a
big deal.

I also think we may want to make a fundamental shift in our view of
thread support. In the early days, it was "well, this is a thing that
modern systems can take advantage of for certain commands". But these
days I suspect it is more like "there are a handful of legacy systems
that do not even support threads".

I don't think we should break the build on those legacy systems, but
it's probably OK to stop thinking of it as "non-threaded platforms are
the default and must pay zero cost" and more as "threaded platforms are
the default, and non-threaded ones are OK to pay a small cost as long as
they still work".

> @@ -74,4 +79,29 @@ int init_recursive_mutex(pthread_mutex_t *m)
>   pthread_mutexattr_destroy(&a);
>   }
>   return ret;
> +#else
> + return ENOSYS;
> +#endif
> +}

I suspect some of these ENOSYS could just become a silent success.
("yep, I initialized your dummy mutex"). But it probably doesn't matter
much either way, as we would not generally even bother checking this
return.

> +#ifdef NO_PTHREADS
> +int dummy_pthread_create(pthread_t *pthread, const void *attr,
> +  void *(*fn)(void *), void *data)
> +{
> + return ENOSYS;
>  }

Whereas for this one, ENOSYS makes a lot of sense (we should avoid the
threaded code-path anyway when we see that online_cpus()==1, and this
would let us know when we mess that up).

> +int dummy_pthread_init(void *data)
> +{
> + /*
> +  * Do nothing.
> +  *
> +  * The main purpose of this function is to break compiler's
> +  * flow analysis or it may realize that functions like
> +  * pthread_mutex_init() is no-op, which means the (static)
> +  * variable is not used/initialized at all and trigger
> +  * -Wunused-variable
> +  */
> + return ENOSYS;
> +}

It might be worth marking the dummy variables as MAYBE_UNUSED, exactly
to avoid this kind of compiler complaint.

-Peff

Re: [PATCH] commit-reach: fix sorting commits by generation

[Thomas Gummerer]

On 10/22, René Scharfe wrote:
> Am 22.10.2018 um 23:10 schrieb Thomas Gummerer:
> > compare_commit_by_gen is used to sort a list of pointers to 'struct
> > commit'.  The comparison function for qsort is called with pointers to
> > the objects it needs to compare, so when sorting a list of 'struct
> > commit *', the arguments are of type 'struct commit **'.  However,
> > currently the comparison function casts it's arguments to 'struct
> > commit *' and uses those, leading to out of bounds memory access and
> > potentially to wrong results.  Fix that.
> > 
> > Signed-off-by: Thomas Gummerer 
> > ---
> > 
> > I noticed this by running the test suite through valgrind.  I'm not
> > familiar with this code, so I'm not sure why this didn't cause any
> > issues or how they would manifest, but this seems like the right fix
> > for this function either way.
> 
> Right; I sent a similar patch a while ago, but it seems to have fallen
> through the cracks:
> 
> https://public-inbox.org/git/d1b58614-989f-5998-6c53-c19eee409...@web.de/

Whoops I didn't notice that, I only checked whether the problem still
exists in pu.  I'd be more than happy to go with your patch instead.

> Anyway, your implied question was discussed back then.  Derrick wrote:
> 
>The reason to sort is to hopefully minimize the amount we walk by 
>exploring the "lower" commits first. This is a performance-only thing, 
>not a correctness issue (which is why the bug exists). Even then, it is 
>just a heuristic.

Thanks for pointing that out!

> Does b6723e4671 in pu (commit-reach: fix first-parent heuristic) change
> that picture?  Did a quick test and found no performance difference with
> and without the fix on top, i.e. proper sorting didn't seem to matter.

I just gave 'test-tool reach can_all_from_reach' a try and got the
same results, with or without the fix the times are very similar.  I
haven't had time to follow the commit-graph series though, so I'm not
sure I used it correctly.  I tried it on the linux repository with the
following input:

X:v4.10
X:v4.9
X:v4.8
X:v4.7
X:v4.6
X:v4.5
X:v4.4
X:v4.3
X:v4.2
X:v4.1
Y:v3.10
Y:v3.9
Y:v3.8
Y:v3.7
Y:v3.6
Y:v3.5
Y:v3.4
Y:v3.3
Y:v3.2
Y:v3.1

> >  commit-reach.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/commit-reach.c b/commit-reach.c
> > index bc522d6840..9efddfd7a0 100644
> > --- a/commit-reach.c
> > +++ b/commit-reach.c
> > @@ -516,8 +516,8 @@ int commit_contains(struct ref_filter *filter, struct 
> > commit *commit,
> >  
> >  static int compare_commits_by_gen(const void *_a, const void *_b)
> >  {
> > -   const struct commit *a = (const struct commit *)_a;
> > -   const struct commit *b = (const struct commit *)_b;
> > +   const struct commit *a = *(const struct commit **)_a;
> > +   const struct commit *b = *(const struct commit **)_b;
> >  
> > if (a->generation < b->generation)
> > return -1;
> > 
> 
> Looks good to me.
> 
> René

[PATCH] i18n: make GETTEXT_POISON a runtime option

[Ævar Arnfjörð Bjarmason]

Change the GETTEXT_POISON compile-time + runtime GIT_GETTEXT_POISON
test parameter to only be a GIT_TEST_GETTEXT_POISON= runtime
parameter, to be consistent with other parameters documented in
"Running tests with special setups" in t/README.

When I added GETTEXT_POISON in bb946bba76 ("i18n: add GETTEXT_POISON
to simulate unfriendly translator", 2011-02-22) I was concerned with
ensuring that the _() function would get constant folded if NO_GETTEXT
was defined or if it wasn't and GETTEXT_POISON wasn't defined.

But as the benchmark in my [1] shows doing a one-off runtime
getenv("GIT_TEST_[...]") is trivial, and since GETTEXT_POISON was
originally added this has become the common idiom in the codebase for
turning on special test setups.

So change GETTEXT_POISON to work the same way. Now the
GETTEXT_POISON=YesPlease compile-time option is gone, and running the
tests with GIT_TEST_GETTEXT_POISON=[true|false] can be toggled on/off
without recompiling.

This allows for conditionally amending tests to test with/without
poison, similar to what 859fdc0c3c ("commit-graph: define
GIT_TEST_COMMIT_GRAPH", 2018-08-29) did for GIT_TEST_COMMIT_GRAPH, but
this patch doesn't change any of the existing tests to work like that.

Notes on the implementation:

 * The only reason we need a new "git-sh-i18n--helper" and the
   corresponding "test-tool gettext-poison" is to expose
   git_env_bool() to shellscripts, since git-sh-i18n and friends need
   to inspect the $GIT_TEST_GETTEXT_POISON variable.

   We only call these if $GIT_TEST_GETTEXT_POISON is set, or in the
   test suite, and this code can go away for non-test code once the
   last i18n-using shellscript is rewritten in C.

 * We still compile a dedicated GETTEXT_POISON build in Travis CI,
   this is probably the wrong thing to do and should be followed-up
   with something similar to ae59a4e44f ("travis: run tests with
   GIT_TEST_SPLIT_INDEX", 2018-01-07) to re-use an existing test setup
   for running in the GIT_TEST_GETTEXT_POISON mode.

 * The reason for not doing:

   test_lazy_prereq GETTEXT_POISON 'test-tool gettext-poison'
   test_lazy_prereq C_LOCALE_OUTPUT '! test-tool gettext-poison'

   In test-lib.sh is because there's some interpolation problem with
   that syntax which makes t6040-tracking-info.sh fail. Hence using
   the simpler test_set_prereq.

See also
https://public-inbox.org/git/871s8gd32p@evledraar.gmail.com/ for
more discussion.

1. https://public-inbox.org/git/871s8gd32p@evledraar.gmail.com/

Signed-off-by: Ævar Arnfjörð Bjarmason 
---

On Mon, Oct 22 2018, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  writes:
>
>> So I think the only reason to keep it compile-time is performance, but I
>> don't think that matters. It's not like we're printing gigabytes of _()
>> formatted output. Everything where formatting matters is plumbing which
>> doesn't use this API. These messages are always for human consumption.
>>
>> So shouldn't we just drop this notion of GETTEXT_POISON at compile-time
>> entirely, and make GIT_GETTEXT_POISON= a test flag that all
>> builds of git are aware of? I think so.
>
> Haven't thought things through, but that sounds like a good thing to
> aim for.  Keep the ideas coming...

Here's a polished version of that. I think it makes sense to queue
this up before any other refactoring of GETTEXT_POISON, and some patch
to unconditionally preserve format specifiers as I suggested upthread
could go on top of this.

 .gitignore |  1 +
 .travis.yml|  2 +-
 Makefile   | 11 ++-
 builtin.h  |  1 +
 builtin/sh-i18n--helper.c  | 27 +++
 ci/lib-travisci.sh |  4 ++--
 gettext.c  |  5 ++---
 gettext.h  |  4 
 git-sh-i18n.sh |  3 ++-
 git.c  |  1 +
 po/README  | 13 -
 t/README   |  6 ++
 t/helper/test-gettext-poison.c |  9 +
 t/helper/test-tool.c   |  1 +
 t/helper/test-tool.h   |  1 +
 t/t-basic.sh   |  2 +-
 t/t3406-rebase-message.sh  |  2 +-
 t/t7201-co.sh  |  5 -
 t/test-lib-functions.sh|  8 
 t/test-lib.sh  | 11 ++-
 20 files changed, 72 insertions(+), 45 deletions(-)
 create mode 100644 builtin/sh-i18n--helper.c
 create mode 100644 t/helper/test-gettext-poison.c

diff --git a/.gitignore b/.gitignore
index 9d1363a1eb..f7b7977910 100644
--- a/.gitignore
+++ b/.gitignore
@@ -148,6 +148,7 @@
 /git-serve
 /git-sh-i18n
 /git-sh-i18n--envsubst
+/git-sh-i18n--helper
 /git-sh-setup
 /git-sh-i18n
 /git-shell
diff --git a/.travis.yml b/.travis.yml
index 4d4e26c9df..4523a2e5ec 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -26,7 +26,7 @@ addons:
 
 matrix:
   include:
-- env: jobname=GETTEXT_POISON
+- env: jobname=GIT_TEST_GETTEXT_POISON
   os: linux

git pull defaults for recursesubmodules

[Tommi Vainikainen]

I configured my local git to fetch with recurseSubmodules = on-demand,
which I found the most convenient setting. However then I noticed that
I mostly use git pull actually to fetch from remotes, but git pull
does not utilize any recurseSubmoddules setting now, or at least I
could not find such.

I would expect that if git-config has fetch.recurseSubmodules set,
also git pull should use this setting, or at least similar option such
as pull.recurseSubmodules should be available. I'd prefer sharing
fetch.recurseSubmodules setting here.

I've attached a minimal patch, which I believe implements this
configuration usage, and a test case to show my expected behavior for
git pull.

--
Tommi Vainikainen
From e4483ec5b3d9c38a6e30aa0ab9fa521cba582345 Mon Sep 17 00:00:00 2001
From: Tommi Vainikainen 
Date: Tue, 23 Oct 2018 23:47:58 +0300
Subject: [PATCH 1/1] pull: obey fetch.recurseSubmodules when fetching

"git pull" now uses same recurse-submodules config for fetching as "git
fetch" by default if not overridden from command line.1
---
 builtin/pull.c|  3 +++
 t/t5572-pull-submodule.sh | 11 +++
 2 files changed, 14 insertions(+)

diff --git a/builtin/pull.c b/builtin/pull.c
index 798ecf7faf..ed39b0e8ed 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -347,6 +347,9 @@ static int git_pull_config(const char *var, const char *value, void *cb)
 		recurse_submodules = git_config_bool(var, value) ?
 			RECURSE_SUBMODULES_ON : RECURSE_SUBMODULES_OFF;
 		return 0;
+	} else if (!strcmp(var, "fetch.recursesubmodules")) {
+		recurse_submodules = parse_fetch_recurse_submodules_arg(var, value);
+		return 0;
 	}
 	return git_default_config(var, value, cb);
 }
diff --git a/t/t5572-pull-submodule.sh b/t/t5572-pull-submodule.sh
index f916729a12..579ae5c374 100755
--- a/t/t5572-pull-submodule.sh
+++ b/t/t5572-pull-submodule.sh
@@ -75,6 +75,17 @@ test_expect_success "submodule.recurse option triggers recursive pull" '
 	test_path_is_file super/sub/merge_strategy_2.t
 '
 
+test_expect_success "fetch.recurseSubmodules=true triggers recursive pull" '
+	test_commit -C child fetch_recurse_submodules &&
+	git -C parent submodule update --remote &&
+	git -C parent add sub &&
+	git -C parent commit -m "update submodule" &&
+
+	git -C super config fetch.recurseSubmodules true &&
+	git -C super pull --no-rebase &&
+	test_path_is_file super/sub/fetch_recurse_submodules.t
+'
+
 test_expect_success " --[no-]recurse-submodule and submodule.recurse" '
 	test_commit -C child merge_strategy_3 &&
 	git -C parent submodule update --remote &&
-- 
2.19.1

[PATCH v2 1/2] commit-slabs: move MAYBE_UNUSED out

[Carlo Marcelo Arenas Belón]

after 36da893114 ("config.mak.dev: enable -Wunused-function", 2018-10-18)
it is expected to be used to prevent -Wunused-function warnings for code
that was macro generated

Signed-off-by: Carlo Marcelo Arenas Belón 
---
 commit-slab-impl.h | 4 ++--
 git-compat-util.h  | 2 ++
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/commit-slab-impl.h b/commit-slab-impl.h
index ac1e6d409a..5c0eb91a5d 100644
--- a/commit-slab-impl.h
+++ b/commit-slab-impl.h
@@ -1,10 +1,10 @@
 #ifndef COMMIT_SLAB_IMPL_H
 #define COMMIT_SLAB_IMPL_H
 
-#define MAYBE_UNUSED __attribute__((__unused__))
+#include "git-compat-util.h"
 
 #define implement_static_commit_slab(slabname, elemtype) \
-   implement_commit_slab(slabname, elemtype, static MAYBE_UNUSED)
+   implement_commit_slab(slabname, elemtype, MAYBE_UNUSED static)
 
 #define implement_shared_commit_slab(slabname, elemtype) \
implement_commit_slab(slabname, elemtype, )
diff --git a/git-compat-util.h b/git-compat-util.h
index 9a64998b24..e4d3967a23 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -408,6 +408,8 @@ static inline char *git_find_last_dir_sep(const char *path)
 #define LAST_ARG_MUST_BE_NULL
 #endif
 
+#define MAYBE_UNUSED __attribute__((__unused__))
+
 #include "compat/bswap.h"
 
 #include "wildmatch.h"
-- 
2.19.1

[PATCH v2 2/2] khash: silence -Wunused-function for delta-islands

[Carlo Marcelo Arenas Belón]

showing the following when compiled with latest clang (OpenBSD, Fedors
and macOS):

delta-islands.c:23:1: warning: unused function 'kh_destroy_str'
  [-Wunused-function]
delta-islands.c:23:1: warning: unused function 'kh_clear_str'
  [-Wunused-function]
delta-islands.c:23:1: warning: unused function 'kh_del_str' [-Wunused-function]

Reported-by: René Scharfe 
Suggested-by: Nguyễn Thái Ngọc Duy 
Signed-off-by: Carlo Marcelo Arenas Belón 
---
 khash.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/khash.h b/khash.h
index c0da40daa7..fb71c92547 100644
--- a/khash.h
+++ b/khash.h
@@ -226,7 +226,7 @@ static const double __ac_HASH_UPPER = 0.77;
__KHASH_IMPL(name, SCOPE, khkey_t, khval_t, kh_is_map, __hash_func, 
__hash_equal)
 
 #define KHASH_INIT(name, khkey_t, khval_t, kh_is_map, __hash_func, 
__hash_equal) \
-   KHASH_INIT2(name, static inline, khkey_t, khval_t, kh_is_map, 
__hash_func, __hash_equal)
+   KHASH_INIT2(name, MAYBE_UNUSED static inline, khkey_t, khval_t, 
kh_is_map, __hash_func, __hash_equal)
 
 /* Other convenient macros... */
 
-- 
2.19.1

[PATCH v2 0/2] delta-islands: avoid unused function messages

[Carlo Marcelo Arenas Belón]

the macro generated code from delta-islands (using khash) triggers
some unused function warnings in macOS, OpenBSD and some linux with a
newer version of clang

Carlo Marcelo Arenas Belón (2):
  commit-slabs: move MAYBE_UNUSED out
  khash: silence -Wunused-function for delta-islands

 commit-slab-impl.h | 4 ++--
 git-compat-util.h  | 2 ++
 khash.h| 2 +-
 3 files changed, 5 insertions(+), 3 deletions(-)

-- 
2.19.1

Re: git pull defaults for recursesubmodules

[Stefan Beller]

On Tue, Oct 23, 2018 at 2:04 PM Tommi Vainikainen  wrote:
>
> I configured my local git to fetch with recurseSubmodules = on-demand,
> which I found the most convenient setting. However then I noticed that
> I mostly use git pull actually to fetch from remotes, but git pull
> does not utilize any recurseSubmoddules setting now, or at least I
> could not find such.
>
> I would expect that if git-config has fetch.recurseSubmodules set,
> also git pull should use this setting, or at least similar option such
> as pull.recurseSubmodules should be available. I'd prefer sharing
> fetch.recurseSubmodules setting here.
>
> I've attached a minimal patch, which I believe implements this
> configuration usage, and a test case to show my expected behavior for
> git pull.

This makes sense to me and the patch looks good to me.
It is unclear to me if this is a regression or an oversight of
of a6d7eb2c7a (pull: optionally rebase submodules (remote
submodule changes only), 2017-06-23)

Thanks,
Stefan

[PATCH] revision.c: drop missing objects from cmdline

[Matthew DeVore]

No code which reads cmdline in struct rev_info can handle NULL objects
in cmdline.rev[i].item, so stop adding them to the cmdline.rev array.
Objects in cmdline are NULL when the given object is promisor and
--exclude-promisor-objects is enabled.

This new behavior avoids a segmentation fault in the added test case in
t0410.

We could simply die if add_rev_cmdline is called with a NULL item,
(rather than warn if --exclude-promisor-objects is set) but because the
amended test case already expects the command to finish successfully,
difference and show a warning. Note that this command:

git rev-list --objects --missing=print $missing_hash

Already fails with a "fatal: bad object HASH" message and this patch
does not change that.

Signed-off-by: Matthew DeVore 
---
 revision.c   | 12 
 t/t0410-partial-clone.sh | 11 ++-
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/revision.c b/revision.c
index a1ddb9e11c..8724dca2e2 100644
--- a/revision.c
+++ b/revision.c
@@ -1148,6 +1148,18 @@ static void add_rev_cmdline(struct rev_info *revs,
int whence,
unsigned flags)
 {
+   if (!item) {
+   /*
+* item is likely a promisor object returned from get_reference.
+*/
+   if (revs->exclude_promisor_objects) {
+   warning(_("ignoring missing object %s"), name);
+   return;
+   } else {
+   die(_("missing object %s"), name);
+   }
+   }
+
struct rev_cmdline_info *info = &revs->cmdline;
unsigned int nr = info->nr;
 
diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
index ba3887f178..e02cd3f818 100755
--- a/t/t0410-partial-clone.sh
+++ b/t/t0410-partial-clone.sh
@@ -366,7 +366,16 @@ test_expect_success 'rev-list accepts missing and promised 
objects on command li
 
git -C repo config core.repositoryformatversion 1 &&
git -C repo config extensions.partialclone "arbitrary string" &&
-   git -C repo rev-list --exclude-promisor-objects --objects "$COMMIT" 
"$TREE" "$BLOB"
+
+   printf "warning: ignoring missing object %s\n" \
+  "$COMMIT" "$TREE" "$BLOB" >expect &&
+   git -C repo rev-list --objects \
+   --exclude-promisor-objects "$COMMIT" "$TREE" "$BLOB" 2>actual &&
+   test_cmp expect actual &&
+
+   git -C repo rev-list --objects-edge-aggressive \
+   --exclude-promisor-objects "$COMMIT" "$TREE" "$BLOB" 2>actual &&
+   test_cmp expect actual
 '
 
 test_expect_success 'gc repacks promisor objects separately from non-promisor 
objects' '
-- 
2.19.1.568.g152ad8e336-goog

Re: [PATCH v2 1/2] commit-slabs: move MAYBE_UNUSED out

[Jeff King]

On Tue, Oct 23, 2018 at 02:50:19PM -0700, Carlo Marcelo Arenas Belón wrote:

> after 36da893114 ("config.mak.dev: enable -Wunused-function", 2018-10-18)
> it is expected to be used to prevent -Wunused-function warnings for code
> that was macro generated

Makes sense.

> diff --git a/commit-slab-impl.h b/commit-slab-impl.h
> index ac1e6d409a..5c0eb91a5d 100644
> --- a/commit-slab-impl.h
> +++ b/commit-slab-impl.h
> @@ -1,10 +1,10 @@
>  #ifndef COMMIT_SLAB_IMPL_H
>  #define COMMIT_SLAB_IMPL_H
>  
> -#define MAYBE_UNUSED __attribute__((__unused__))
> +#include "git-compat-util.h"

We shouldn't need to include git-compat-util.h, as the C files would
already do so, via this rule in CodingGuidelines:

   - The first #include in C files, except in platform specific compat/
   implementations, must be either "git-compat-util.h", "cache.h" or
   "builtin.h".  You do not have to include more than one of these.

(and if there is a case that does not, we should fix that).

>  #define implement_static_commit_slab(slabname, elemtype) \
> - implement_commit_slab(slabname, elemtype, static MAYBE_UNUSED)
> + implement_commit_slab(slabname, elemtype, MAYBE_UNUSED static)

Is this hunk necessary?

> diff --git a/git-compat-util.h b/git-compat-util.h
> index 9a64998b24..e4d3967a23 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -408,6 +408,8 @@ static inline char *git_find_last_dir_sep(const char 
> *path)
>  #define LAST_ARG_MUST_BE_NULL
>  #endif
>  
> +#define MAYBE_UNUSED __attribute__((__unused__))
> +
>  #include "compat/bswap.h"

Looks good.

-Peff

Re: [PATCH 8/9] fetch: retry fetching submodules if needed objects were not fetched

[Stefan Beller]

On Wed, Oct 17, 2018 at 5:40 PM Jonathan Tan  wrote:
>
> > Currently when git-fetch is asked to recurse into submodules, it dispatches
> > a plain "git-fetch -C " (with some submodule related options
> > such as prefix and recusing strategy, but) without any information of the
> > remote or the tip that should be fetched.
> >
> > This works surprisingly well in some workflows (such as using submodules
> > as a third party library), while not so well in other scenarios, such
> > as in a Gerrit topic-based workflow, that can tie together changes
> > (potentially across repositories) on the server side. One of the parts
> > of such a Gerrit workflow is to download a change when wanting to examine
> > it, and you'd want to have its submodule changes that are in the same
> > topic downloaded as well. However these submodule changes reside in their
> > own repository in their own ref (refs/changes/).
>
> It seems that the pertinent situation is when, in the superproject, you
> fetch a commit (which may be the target of a ref, or an ancestor of the
> target of a ref) that points to a submodule commit that was not fetched
> by the default refspec-less fetch that you describe in the first
> paragraph. I would just describe it as follows:
>
>   But this default fetch is not sufficient, as a newly fetched commit in
>   the superproject could point to a commit in the submodule that is not
>   in the default refspec. This is common in workflows like Gerrit's.
>   When fetching a Gerrit change under review (from refs/changes/??), the
>   commits in that change likely point to submodule commits that have not
>   been merged to a branch yet.

Makes sense.

> Another thing you need to clarify is what happens if the fetch-by-commit
> fails. Right now, it seems that it will make the whole thing fail, which
> might be a surprising change in behavior.

But a positive surprise, I would assume?

> The test stores the result in a normal branch, not a remote tracking
> branch. Is storing in a normal branch required?

In the test we fetch from another repository, such that in the
repository-under-test this will show up in a remote tracking branch?

> Also, do you know why this is required? A naive reading of the patch
> leads me to believe that this should work even if merely fetching to
> FETCH_HEAD.

See the next patch, check_for_new_submodule_commits() is missing
for FETCH_HEAD.

>
> > +struct get_next_submodule_task {
> > + struct repository *repo;
> > + const struct submodule *sub;
> > + unsigned free_sub : 1; /* Do we need to free the submodule? */
> > + struct oid_array *commits;
> > +};
>
> Firstly, I don't think "commits" needs to be a pointer.
>
> Document at least "commits". As far as I can tell, if NULL (or empty if
> you take my suggestion), this means that this task is the first pass for
> this particular submodule and the fetch needs to be done using the
> default refspec. Otherwise, this task is the second pass for this
> particular submodule and the fetch needs to be done passing the
> contained OIDs.

Makes sense. I think I'll make it a non-pointer, but will introduce another
flag or counter for the phase.

>
> > +static const struct submodule *get_default_submodule(const char *path)
> > +{
> > + struct submodule *ret = NULL;
> > + const char *name = default_name_or_path(path);
> > +
> > + if (!name)
> > + return NULL;
> > +
> > + ret = xmalloc(sizeof(*ret));
> > + memset(ret, 0, sizeof(*ret));
> > + ret->path = name;
> > + ret->name = name;
> > +
> > + return (const struct submodule *) ret;
> > +}
>
> What is a "default" submodule and why would you need one?

s/default/artificial/. Such a submodule is a submodule that has no
config in the .gitmodules file and its name == path.
We need to keep those around for historic reasons AFAICT, c.f.
c68f837576 (implement fetching of moved submodules, 2017-10-16)

> > + task = get_next_submodule_task_create(spf->r, ce->name);
> > +
> > + if (!task->sub) {
> > + free(task);
> > + continue;
> >   }
>
> Will task->sub ever be NULL?

Yes, if we fall back to these "default" submodule and just try if it
can be handled
as a submodule, but it cannot be handled as such,
get_next_submodule_task_create has

task->sub = submodule_from_path(r, &null_oid, path);
if (!task->sub) {
task->sub = get_default_submodule(path);

and get_default_submodule can return NULL.


>
> > + if (spf->retry_nr) {
> > + struct get_next_submodule_task *task = 
> > spf->retry[spf->retry_nr - 1];
> > + struct strbuf submodule_prefix = STRBUF_INIT;
> > + spf->retry_nr--;
> > +
> > + strbuf_addf(&submodule_prefix, "%s%s/", spf->prefix, 
> > task->sub->path);
> > +
> > + child_process_init(cp);
> > + prepare_submodule_repo_env_in_gitdir(&cp->env_array);
> > + cp->git_cmd = 1;
> >

Re: [PATCH 7/9] submodule: fetch in submodules git directory instead of in worktree

[Jonathan Tan]

> > Why does GIT_DIR need to be set? Is it to avoid subcommands recursively
> > checking the parent directories in case the CWD is a malformed Git
> > repository? If yes, maybe it's worth adding a comment.
> 
> It is copying the structure from prepare_submodule_repo_env,
> specifically 10f5c52656 (submodule: avoid auto-discovery in
> prepare_submodule_repo_env(), 2016-09-01), which sounds
> appealing (and brings real benefits for the working directory),
> but I have not thought about this protection for the git dir.
> 
> Maybe another approach is to not set the cwd for the child process
> and instead point GIT_DIR_ENVIRONMENT only to the right
> directory.
> 
> Then the use of GIT_DIR_ENVIRONMENT is obvious and
> is not just for protection of corner cases.
> 
> However I think this protection is really valuable for the
> .git dir as well as the submodule may be broken and we do not
> want to end up in an infinite loop (as the discovery would find
> the superproject which then tries to recurse, again, into the
> submodule with the broken git dir)
> 
> When adding the comment here, we'd also want to have
> the comment in prepare_submodule_repo_env, which
> could be its own preparation commit.

I agree with the protection. As for the preparation commit, I don't
think it's always the code author's responsibility to tidy up the
surrounding code, but since you're adding an identical comment here,
it's probably worth it to add the comment there too.

> > This is the significant thing that this patch does more - an unskipped
> > submodule is now something that either passes the checks in
> > repo_submodule_init() or the checks in repo_init(), which seems to be
> > stricter than the current check that ".git" points to a directory or is
> > one. This means that we skip certain broken repositories, and this
> > necessitates a change in the test.
> 
> I see. However there is no change in function, the check in repo_init
> (or repo_submodule_init) is less strict than the check in the child process.
> So if there are broken submodule repositories, the difference of this
> patch is the layer at which it is caught, i.e. we would not spawn a child
> that fails, but skip the submodule.
> 
> Thinking of that, maybe we need to announce that in get_next_submodule

The consequence of getting caught changes, though. Currently,
spf->result is set to 1 whenever a child process fails. But in this
patch, some of these repositories would be entirely skipped, meaning
that no child process is run, and spf->result is never modified.

> > I think we should be more particular about what we're allowed to skip -
> > in particular, maybe if we're planning to skip this submodule, its
> > corresponding directory in the worktree (if one exists) needs to be
> > empty.
> 
> If the working tree directory is empty for that submodule, it means
> it is likely not initialized. But why would we use that as a signal to
> skip the submodule?

What I meant was: if empty, skip it completely. Otherwise, do the
repo_submodule_init() and repo_init() thing, and if they both fail, set
spf->result to 1, preserving existing behavior.

Re: [PATCH 1/3] gpg-interface.c: use flags to determine key/signer info presence

[brian m. carlson]

On Mon, Oct 22, 2018 at 06:38:19PM +0200, Michał Górny wrote:
> Replace the logic used to determine whether key and signer information
> is present to use explicit flags in sigcheck_gpg_status[] array.  This
> is more future-proof, since it makes it possible to add additional
> statuses without having to explicitly update the conditions.

This series looks good to me.  I was going to ask after patch 2 whether
you were printing the subkey or primary key fingerprint, and then you
answered my question in patch 3.  Thanks for including both.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: [PATCH 7/9] submodule: fetch in submodules git directory instead of in worktree

[Stefan Beller]

On Tue, Oct 23, 2018 at 3:55 PM Jonathan Tan  wrote:
> > When adding the comment here, we'd also want to have
> > the comment in prepare_submodule_repo_env, which
> > could be its own preparation commit.
>
> I agree with the protection. As for the preparation commit, I don't
> think it's always the code author's responsibility to tidy up the
> surrounding code, but since you're adding an identical comment here,
> it's probably worth it to add the comment there too.

Am I the only one who dislikes inconsistent files? ;-)
(ie. clean in one place, not cleaned up in another)
I can see your point. Will add a comment

> > Thinking of that, maybe we need to announce that in get_next_submodule
>
> The consequence of getting caught changes, though. Currently,
> spf->result is set to 1 whenever a child process fails. But in this
> patch, some of these repositories would be entirely skipped, meaning
> that no child process is run, and spf->result is never modified.

Right.

> > If the working tree directory is empty for that submodule, it means
> > it is likely not initialized. But why would we use that as a signal to
> > skip the submodule?
>
> What I meant was: if empty, skip it completely. Otherwise, do the
> repo_submodule_init() and repo_init() thing, and if they both fail, set
> spf->result to 1, preserving existing behavior.

I did it the other way round:

If repo_[submodule_]init fails, see if we have a gitlink in tree and
an empty dir in the FS, to decide if we need to signal failure.

I can switch it around again, but it seemed easier to write as
that puts corner cases away into one else {} case.

Re: [PATCH v2 1/2] commit-slabs: move MAYBE_UNUSED out

[Carlo Arenas]

On Tue, Oct 23, 2018 at 3:00 PM Jeff King  wrote:
> On Tue, Oct 23, 2018 at 02:50:19PM -0700, Carlo Marcelo Arenas Belón wrote:
> >  #define implement_static_commit_slab(slabname, elemtype) \
> > - implement_commit_slab(slabname, elemtype, static MAYBE_UNUSED)
> > + implement_commit_slab(slabname, elemtype, MAYBE_UNUSED static)
>
> Is this hunk necessary?

it works eitherway but the proposed syntax is IMHO better aligned
(when used in a function definition) as it matches better the syntax
from C++ attribute: maybe_unused (since C++17) [1]

__attribute__(unused) (the GCC extension) is meant to be applied to
function declarations, but we are not generating those.

Carlo

[1] https://en.cppreference.com/w/cpp/language/attributes/maybe_unused

Re: git pull defaults for recursesubmodules

[brian m. carlson]

On Wed, Oct 24, 2018 at 12:04:06AM +0300, Tommi Vainikainen wrote:
> I configured my local git to fetch with recurseSubmodules = on-demand,
> which I found the most convenient setting. However then I noticed that
> I mostly use git pull actually to fetch from remotes, but git pull
> does not utilize any recurseSubmoddules setting now, or at least I
> could not find such.
> 
> I would expect that if git-config has fetch.recurseSubmodules set,
> also git pull should use this setting, or at least similar option such
> as pull.recurseSubmodules should be available. I'd prefer sharing
> fetch.recurseSubmodules setting here.
> 
> I've attached a minimal patch, which I believe implements this
> configuration usage, and a test case to show my expected behavior for
> git pull.

Typically, we prefer patches to be inline; descriptive content like this
goes after the --- line in the patch, or in a cover letter.

> From e4483ec5b3d9c38a6e30aa0ab9fa521cba582345 Mon Sep 17 00:00:00 2001
> From: Tommi Vainikainen 
> Date: Tue, 23 Oct 2018 23:47:58 +0300
> Subject: [PATCH 1/1] pull: obey fetch.recurseSubmodules when fetching
> 
> "git pull" now uses same recurse-submodules config for fetching as "git
> fetch" by default if not overridden from command line.1

You have an extra '1" at the end of this line.

Also, missing sign-off.  See Documentation/SubmittingPatches.

> diff --git a/builtin/pull.c b/builtin/pull.c
> index 798ecf7faf..ed39b0e8ed 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -347,6 +347,9 @@ static int git_pull_config(const char *var, const char 
> *value, void *cb)
>   recurse_submodules = git_config_bool(var, value) ?
>   RECURSE_SUBMODULES_ON : RECURSE_SUBMODULES_OFF;
>   return 0;
> + } else if (!strcmp(var, "fetch.recursesubmodules")) {
> + recurse_submodules = parse_fetch_recurse_submodules_arg(var, 
> value);
> + return 0;
>   }
>   return git_default_config(var, value, cb);
>  }
> diff --git a/t/t5572-pull-submodule.sh b/t/t5572-pull-submodule.sh
> index f916729a12..579ae5c374 100755
> --- a/t/t5572-pull-submodule.sh
> +++ b/t/t5572-pull-submodule.sh
> @@ -75,6 +75,17 @@ test_expect_success "submodule.recurse option triggers 
> recursive pull" '
>   test_path_is_file super/sub/merge_strategy_2.t
>  '
>  
> +test_expect_success "fetch.recurseSubmodules=true triggers recursive pull" '
> + test_commit -C child fetch_recurse_submodules &&
> + git -C parent submodule update --remote &&
> + git -C parent add sub &&
> + git -C parent commit -m "update submodule" &&
> +
> + git -C super config fetch.recurseSubmodules true &&
> + git -C super pull --no-rebase &&
> + test_path_is_file super/sub/fetch_recurse_submodules.t
> +'

Can we have a test that --no-recurse-submodules overrides
fetch.recurseSubmodules?
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: [PATCH 8/9] fetch: retry fetching submodules if needed objects were not fetched

[Jonathan Tan]

> > Another thing you need to clarify is what happens if the fetch-by-commit
> > fails. Right now, it seems that it will make the whole thing fail, which
> > might be a surprising change in behavior.
> 
> But a positive surprise, I would assume?

Whether positive or negative, I think that this needs to be mentioned in
the commit message.

As for positive or negative, I tend to agree that it's positive - sure,
some previously successful fetches would now fail, but the results of
those fetches could not be recursively checked out anyway.

> > The test stores the result in a normal branch, not a remote tracking
> > branch. Is storing in a normal branch required?
> 
> In the test we fetch from another repository, such that in the
> repository-under-test this will show up in a remote tracking branch?

If that were true, I would expect that when this line:

> git fetch --recurse-submodules --recurse-submodules-default on-demand origin 
> refs/changes/2:refs/heads/my_branch &&

is replaced by this line:

> git fetch --recurse-submodules --recurse-submodules-default on-demand origin 
> refs/changes/2 &&

then things would still work. The tests pass with the first line (after
I fixed a type mismatch) but not with the second. (Also I don't think a
remote-tracking branch is generated here - the output printed doesn't
indicate so, and refs/changes/2 is not a branch anyway.)

> > Also, do you know why this is required? A naive reading of the patch
> > leads me to believe that this should work even if merely fetching to
> > FETCH_HEAD.
> 
> See the next patch, check_for_new_submodule_commits() is missing
> for FETCH_HEAD.

I see in the next patch that there is an "if" branch in
store_updated_refs() without update_local_ref() in which
"check_for_new_submodule_commits(&rm->old_oid)" needs to be inserted. I
think this is a symptom that maybe check_for_new_submodule_commits()
needs to be extracted from update_local_ref() and put into
store_updated_refs() instead? In update_local_ref(), it is called on
ref->new_oid, which is actually the same as rm->old_oid anyway (there is
an oidcpy earlier).

> > > +static const struct submodule *get_default_submodule(const char *path)
> > > +{
> > > + struct submodule *ret = NULL;
> > > + const char *name = default_name_or_path(path);
> > > +
> > > + if (!name)
> > > + return NULL;
> > > +
> > > + ret = xmalloc(sizeof(*ret));
> > > + memset(ret, 0, sizeof(*ret));
> > > + ret->path = name;
> > > + ret->name = name;
> > > +
> > > + return (const struct submodule *) ret;
> > > +}
> >
> > What is a "default" submodule and why would you need one?
> 
> s/default/artificial/. Such a submodule is a submodule that has no
> config in the .gitmodules file and its name == path.
> We need to keep those around for historic reasons AFAICT, c.f.
> c68f837576 (implement fetching of moved submodules, 2017-10-16)

Ah, OK. I would call it a fake submodule then, and copy over the "No
entry in .gitmodules?" comment.

> > Will task->sub ever be NULL?
> 
> Yes, if we fall back to these "default" submodule and just try if it
> can be handled
> as a submodule, but it cannot be handled as such,
> get_next_submodule_task_create has
> 
> task->sub = submodule_from_path(r, &null_oid, path);
> if (!task->sub) {
> task->sub = get_default_submodule(path);
> 
> and get_default_submodule can return NULL.

Ah, yes you're right.

[PATCH 2/2] completion: Support `git mergetool --[no-]gui`

[Denton Liu]

Signed-off-by: Denton Liu 
Signed-off-by: Anmol Mago 
Signed-off-by: Brian Ho 
Signed-off-by: David Lu 
Signed-off-by: Ryan Wang 
---
 contrib/completion/git-completion.bash | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index db7fd87b6..a45b4a050 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1833,7 +1833,7 @@ _git_mergetool ()
return
;;
--*)
-   __gitcomp "--tool= --prompt --no-prompt"
+   __gitcomp "--tool= --prompt --no-prompt --gui --no-gui"
return
;;
esac
-- 
2.19.1

Re: [PATCH 1/3] [Outreachy] t3903-stash: test without configured user name

[Junio C Hamano]

Slavica  writes:

> +test_expect_failure 'stash with HOME as non-existing directory' '
> +test_commit 1 &&
> +test_config user.useconfigonly true &&
> +test_config stash.usebuiltin true &&
> +(
> +HOME=$(pwd)/none &&
> +export HOME &&

What is the reason why this test needs to move HOME away from
TRASH_DIRECTORY (set in t/test-lib.sh)?

> +unset GIT_AUTHOR_NAME &&
> +unset GIT_AUTHOR_EMAIL &&
> +unset GIT_COMMITTER_NAME &&
> +unset GIT_COMMITTER_EMAIL &&
> +test_must_fail git config user.email &&
> +echo changed >1.t &&
> + git stash
> +)
> +'
> +
>  test_done

[PATCH 1/2] mergetool: Accept -g/--[no-]gui as arguments

[Denton Liu]

In line with how difftool accepts a -g/--[no-]gui option, make mergetool
accept the same option in order to use the `merge.guitool` variable to
find the default mergetool instead of `merge.tool`.

Signed-off-by: Denton Liu 
Signed-off-by: Anmol Mago 
Signed-off-by: Brian Ho 
Signed-off-by: David Lu 
Signed-off-by: Ryan Wang 
---
 Documentation/git-mergetool.txt | 11 +++
 git-mergetool--lib.sh   | 16 +++-
 git-mergetool.sh| 11 +--
 3 files changed, 31 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt
index 3622d6648..0c7975a05 100644
--- a/Documentation/git-mergetool.txt
+++ b/Documentation/git-mergetool.txt
@@ -79,6 +79,17 @@ success of the resolution after the custom tool has exited.
Prompt before each invocation of the merge resolution program
to give the user a chance to skip the path.
 
+-g::
+--gui::
+   When 'git-mergetool' is invoked with the `-g` or `--gui` option
+   the default merge tool will be read from the configured
+   `merge.guitool` variable instead of `merge.tool`.
+
+--no-gui::
+   This overrides a previous `-g` or `--gui` setting and reads the
+   default merge tool will be read from the configured `merge.tool`
+   variable.
+
 -O::
Process files in the order specified in the
, which has one shell glob pattern per line.
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 9a8b97a2a..e317ae003 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -350,17 +350,23 @@ guess_merge_tool () {
 }
 
 get_configured_merge_tool () {
-   # Diff mode first tries diff.tool and falls back to merge.tool.
-   # Merge mode only checks merge.tool
+   # If first argument is true, find the guitool instead
+   if [ "$1" = true ]
+   then
+   gui_prefix=gui
+   fi
+
+   # Diff mode first tries diff.(gui)tool and falls back to 
merge.(gui)tool.
+   # Merge mode only checks merge.(gui)tool
if diff_mode
then
-   merge_tool=$(git config diff.tool || git config merge.tool)
+   merge_tool=$(git config diff.${gui_prefix}tool || git config 
merge.${gui_prefix}tool)
else
-   merge_tool=$(git config merge.tool)
+   merge_tool=$(git config merge.${gui_prefix}tool)
fi
if test -n "$merge_tool" && ! valid_tool "$merge_tool"
then
-   echo >&2 "git config option $TOOL_MODE.tool set to unknown 
tool: $merge_tool"
+   echo >&2 "git config option $TOOL_MODE.${gui_prefix}tool set to 
unknown tool: $merge_tool"
echo >&2 "Resetting to default..."
return 1
fi
diff --git a/git-mergetool.sh b/git-mergetool.sh
index d07c7f387..01b9ad59b 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -9,7 +9,7 @@
 # at the discretion of Junio C Hamano.
 #
 
-USAGE='[--tool=tool] [--tool-help] [-y|--no-prompt|--prompt] [-O] 
[file to merge] ...'
+USAGE='[--tool=tool] [--tool-help] [-y|--no-prompt|--prompt] 
[-g|--gui|--no-gui] [-O] [file to merge] ...'
 SUBDIRECTORY_OK=Yes
 NONGIT_OK=Yes
 OPTIONS_SPEC=
@@ -389,6 +389,7 @@ print_noop_and_exit () {
 
 main () {
prompt=$(git config --bool mergetool.prompt)
+   gui_tool=false
guessed_merge_tool=false
orderfile=
 
@@ -414,6 +415,12 @@ main () {
shift ;;
esac
;;
+   --no-gui)
+   gui_tool=false
+   ;;
+   -g|--gui)
+   gui_tool=true
+   ;;
-y|--no-prompt)
prompt=false
;;
@@ -443,7 +450,7 @@ main () {
if test -z "$merge_tool"
then
# Check if a merge tool has been configured
-   merge_tool=$(get_configured_merge_tool)
+   merge_tool=$(get_configured_merge_tool $gui_tool)
# Try to guess an appropriate merge tool if no tool has been 
set.
if test -z "$merge_tool"
then
-- 
2.19.1

Re: [PATCH v3 2/3] reset: add new reset.quiet config setting

[Junio C Hamano]

Ben Peart  writes:

>>> diff --git a/Documentation/config.txt b/Documentation/config.txt
>>> index f6f4c21a54..a2d1b8b116 100644
>>> --- a/Documentation/config.txt
>>> +++ b/Documentation/config.txt
>>> @@ -2728,6 +2728,9 @@ rerere.enabled::
>>>  `$GIT_DIR`, e.g. if "rerere" was previously used in the
>>>  repository.
>>>
>>> +reset.quiet::
>>> +   When set to true, 'git reset' will default to the '--quiet' option.
>>> +
>>
>> With 'nd/config-split' topic moving pretty much all config keys out of
>> config.txt, you probably want to do the same for this series: add this
>> in a new file called Documentation/reset-config.txt then include the
>> file here like the sendemail one below.
>>
>
> Seems a bit overkill to pull a line of documentation into a separate
> file and replace it with a line of 'import' logic.  Perhaps if/when
> there is more documentation to pull out that would make more sense.

This change (ehh, rather, perhaps nd/config-split topic) came at an
unfortunate moment.  Until I actually did one integration cycle to
rebuild 'pu' and merge this patch and the other topic, I had exactly
the same reaction as yours above to Duy's comment.  But seeing the
tree at the tip of 'pu' today, I do think the end result with a
single liner file that has configuration for the "reset" command
that is included in config.txt would make sense, and I also think
you would agree with it if you see the same tree.

How we should get there is a different story.  I think Duy's series
needs at least another update to move the split pieces into its own
subdirectory of Documentation/, and it is not all that urgent, while
this three-patch series (with the advice.* bit added) for "reset" is
pretty much ready to go 'next', so my gut feeling is that it is best
to keep the description here, and to ask Duy to base the updated
version of config-split topic on top.

Re: [PATCH/RFC] thread-utils: better wrapper to avoid #ifdef NO_PTHREADS

[Junio C Hamano]

Jeff King  writes:

> I also think we may want to make a fundamental shift in our view of
> thread support. In the early days, it was "well, this is a thing that
> modern systems can take advantage of for certain commands". But these
> days I suspect it is more like "there are a handful of legacy systems
> that do not even support threads".
>
> I don't think we should break the build on those legacy systems, but
> it's probably OK to stop thinking of it as "non-threaded platforms are
> the default and must pay zero cost" and more as "threaded platforms are
> the default, and non-threaded ones are OK to pay a small cost as long as
> they still work".

Good suggestion.

Re: [PATCH 2/3] mingw: replace MSVCRT's fstat() with a Win32-based implementation

[brian m. carlson]

On Tue, Oct 23, 2018 at 03:23:21AM -0700, Karsten Blees via GitGitGadget wrote:
> - if (!get_file_info_by_handle(fh, buf))
> + case FILE_TYPE_CHAR:
> + case FILE_TYPE_PIPE:
> + /* initialize stat fields */
> + memset(buf, 0, sizeof(*buf));
> + buf->st_nlink = 1;
> +
> + if (type == FILE_TYPE_CHAR) {
> + buf->st_mode = _S_IFCHR;
> + } else {
> + buf->st_mode = _S_IFIFO;
> + if (PeekNamedPipe(fh, NULL, 0, NULL, &avail, NULL))
> + buf->st_size = avail;

These lines strike me as a bit odd.  As far as I'm aware, Unix systems
don't return anything useful in this field when calling fstat on a pipe.
Is there a reason we fill this in on Windows?  If so, could the commit
message explain what that is?
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: [PATCH v4 2/3] reset: add new reset.quiet config setting

[Ramsay Jones]

On 23/10/2018 20:04, Ben Peart wrote:
> From: Ben Peart 

Sorry for the late reply, ... I've been away from email - I am
still trying to catch up.

> 
> Add a reset.quiet config setting that sets the default value of the --quiet
> flag when running the reset command.  This enables users to change the
> default behavior to take advantage of the performance advantages of
> avoiding the scan for unstaged changes after reset.  Defaults to false.
> 
> Signed-off-by: Ben Peart 
> ---
>  Documentation/config.txt| 3 +++
>  Documentation/git-reset.txt | 5 -
>  builtin/reset.c | 1 +
>  3 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index f6f4c21a54..a2d1b8b116 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2728,6 +2728,9 @@ rerere.enabled::
>   `$GIT_DIR`, e.g. if "rerere" was previously used in the
>   repository.
>  
> +reset.quiet::
> + When set to true, 'git reset' will default to the '--quiet' option.

Mention that this 'Defaults to false'?

> +
>  include::sendemail-config.txt[]
>  
>  sequence.editor::
> diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
> index 1d697d9962..2dac95c71a 100644
> --- a/Documentation/git-reset.txt
> +++ b/Documentation/git-reset.txt
> @@ -95,7 +95,10 @@ OPTIONS
>  
>  -q::
>  --quiet::
> - Be quiet, only report errors.
> +--no-quiet::
> + Be quiet, only report errors. The default behavior is set by the
> + `reset.quiet` config option. `--quiet` and `--no-quiet` will
> + override the default behavior.

Better than last time, but how about something like:

 -q::
 --quiet::
 --no-quiet::
  Be quiet, only report errors. The default behaviour of the
  command, which is to not be quiet, can be specified by the
  `reset.quiet` configuration variable. The `--quiet` and
  `--no-quiet` options can be used to override any configured
  default.

Hmm, I am not sure that is any better! :-D

Also, note that the --no-option is often described separately to
the --option (in a separate paragraph). I don't know if that would
help here.

[The default behaviour is _not_ set by the configuration, if no
configuration is specified. :-P ]

Not sure if that helps!

ATB,
Ramsay Jones

>  
>  
>  EXAMPLES
> diff --git a/builtin/reset.c b/builtin/reset.c
> index 04f0d9b4f5..3b43aee544 100644
> --- a/builtin/reset.c
> +++ b/builtin/reset.c
> @@ -306,6 +306,7 @@ int cmd_reset(int argc, const char **argv, const char 
> *prefix)
>   };
>  
>   git_config(git_reset_config, NULL);
> + git_config_get_bool("reset.quiet", &quiet);
>  
>   argc = parse_options(argc, argv, prefix, options, git_reset_usage,
>   PARSE_OPT_KEEP_DASHDASH);
> 

Re: Add config option to make "--keep-index" the default for "git stash push"

[brian m. carlson]

On Mon, Oct 22, 2018 at 10:11:50AM +0200, Ralf Jung wrote:
> Hi all,
> 
> I have been repeatedly bitten by the default behavior of `git stash` to stash
> not just the unstaged but also the staged changes, and then `git stash pop`
> making all changes unstaged. This means `git stash && git stash pop`, per
> default, loses information, and when I just spent 10min carefully selecting
> hunks to be staged that's quite frustrating.
> 
> I found that for myself, I usually expect `git stash` to only stash the 
> changes
> that are not yet staged.  So I'd like to configure git such that 
> `--keep-index`
> is the default.  That would also fix the information loss I mentioned above.
> (By now I am also aware of `git stash pop --index`, but (a) that's not the
> default either and (b) its documentation indicates it might not always work 
> very
> well.)  However, going over the `git-config` man page, I have not found any 
> way
> to change the default behavior.  Is that possible somehow?  And if not, could
> you consider this a feature request to add such an option?

First, let me say that I'm speaking for myself and not the whole list.
Other people may have other opinions, and some may want to see a patch
before deciding.

Personally, I am hesitant about such an option, since I know people use
stash in scripts and hooks (whether that's a good idea or not), and
ending up with an unclean tree after git stash would be surprising and
could potentially cause data loss.  (I have a co-worker who is doing
exactly this.)

We do have a --no-keep-index option, so there is an out for people who
don't want that behavior, and maybe that's enough to avoid problems.  I
usually deal with this situation by using an alias, which lets me have
more control over the exact behavior I want.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: [PATCH] test: avoid failures when USE_NED_ALLOCATOR

[Junio C Hamano]

Carlo Marcelo Arenas Belón   writes:

> contrib/nedmalloc doesn't support MALLOC_CHECK_ or MALLOC_PERTURB_
> so add it to the same exception that is being used with valgrind
>
> Signed-off-by: Carlo Marcelo Arenas Belón 
> ---
>  t/test-lib.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Unlike the case for valgrind, where we actively do not want to set
these two environment variables [*1*], I am assuming that nedmalloc
simply ignores these two environment variables.  Is there a reason
why we want to special case nedmalloc like this patch does, but
leave these set for other implementations that do not pay attention
to them?  

Of course, I am also assuming that there are implementations of
malloc(3), which are not nedmalloc, that can be used to build and
test Git and that do not react to these two environment variables.
The patch would make sense if all the other implementations of
malloc(3) paid attention to MALLOC_{CHECK,PERTURB}_ and nedmalloc
were the only odd-man out, but I do not think that is the case.

[Footnote] 

*1* see the last two paragraphs of the log message of a731fa91 ("Add
MALLOC_CHECK_ and MALLOC_PERTURB_ libc env to the test suite for
detecting heap corruption", 2012-09-14)


> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 44288cbb59..2ad9e6176c 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -143,7 +143,7 @@ fi
>  # Add libc MALLOC and MALLOC_PERTURB test
>  # only if we are not executing the test with valgrind
>  if expr " $GIT_TEST_OPTS " : ".* --valgrind " >/dev/null ||
> -   test -n "$TEST_NO_MALLOC_CHECK"
> +   test -n "$TEST_NO_MALLOC_CHECK" || test -n "$USE_NED_ALLOCATOR"
>  then
>   setup_malloc_check () {
>   : nothing

Re: [PATCH 1/3] gpg-interface.c: use flags to determine key/signer info presence

[Junio C Hamano]

"brian m. carlson"  writes:

> On Mon, Oct 22, 2018 at 06:38:19PM +0200, Michał Górny wrote:
>> Replace the logic used to determine whether key and signer information
>> is present to use explicit flags in sigcheck_gpg_status[] array.  This
>> is more future-proof, since it makes it possible to add additional
>> statuses without having to explicitly update the conditions.
>
> This series looks good to me.  I was going to ask after patch 2 whether
> you were printing the subkey or primary key fingerprint, and then you
> answered my question in patch 3.  Thanks for including both.

Yeah, this looks good to me too.  Thanks, both.

Re: [PATCH] range-diff: allow to diff files regardless submodule

[Junio C Hamano]

Lucas De Marchi  writes:

>> Yes, I agree on both counts (i.e. it was totally unclear what
>> problem is being solved and what the root cause of the problem is,
>> and we would want a new test to protect this "fix" from getting
>> broken in the future.
>
> have you seen I sent a v2 with proper test?

No, otherwise I wouln't have said it needs tests, and no, because I
haven't seen the v2, I do not know if it came with proper test or
other issues pointed out and fixes suggested in the review round
were addressed in v2.  Sorry.

When you ask such a question, please accompany it with "this is the
message-id" to avoid the receiver of the question locating a wrong
version of your patch from the archive.

Thanks.

Re: New semantic patches vs. in-flight topics [was: Re: [PATCH 00/19] Bring more repository handles into our code base]

[Junio C Hamano]

Stefan Beller  writes:

>> Anyway, even though "make coccicheck" does not run in subsecond,
>> I've updated my machinery to rebuild the integration branches so
>> that I can optionally queue generated coccicheck patches, and what I
>> pushed out tonight has one at the tip of 'pu' and also another at
>> the tip of 'next'.  The latter seems to be passing all archs and
>> executing Windows run.
>
> That is pretty exciting!
>
> Looking at the commit in next, you also included the suggestion
> from [1] to use a postincrement instead of a preincrement and I got
> excited to see how we express such a thing in coccinelle,
> but it turns out that it slipped in unrelated to the coccinelle patches.

See below, which was sitting in my working tree.

> How would we go from here?
> It is not obvious to me how such changes would be integrated,
> as regenerating them on top of pu will not help getting these changes
> merged down, and applying the semantic patch on next (once
> sb/more-repo-in-api lands in next) would created the merge conflicts for
> all topics that are merged to next after that series.

Conflicts with later topics is indeed worrysome.  That is why I did
it as an experiment.  If it becomes too painful, I'd probably stop
doing it while merging to anything other than 'pu', and then we can
follow the more distributed approach along the lines of what Szeder
suggested, to see how smoothly it goes.

-- >8 --
Subject: [PATCH] cocci: simplify "if (++u > 1)" to "if (u++)"

It is more common to use post-increment than pre-increment when the
side effect is the primary thing we want in our code and in C in
general (unlike C++).

Initializing a variable to 0, incrementing it every time we do
something, and checking if we have already done that thing to guard
the code to do that thing, is easier to understand when written

if (u++)
; /* we've done that! */
else
do_it(); /* just once. */

but if you try to use pre-increment, you end up with a less natural
looking

if (++u > 1)

Signed-off-by: Junio C Hamano 
---
 contrib/coccinelle/preincr.cocci | 5 +
 1 file changed, 5 insertions(+)
 create mode 100644 contrib/coccinelle/preincr.cocci

diff --git a/contrib/coccinelle/preincr.cocci b/contrib/coccinelle/preincr.cocci
new file mode 100644
index 00..7fe1e8d2d9
--- /dev/null
+++ b/contrib/coccinelle/preincr.cocci
@@ -0,0 +1,5 @@
+@ preincrement @
+identifier i;
+@@
+- ++i > 1
++ i++
-- 
2.19.1-542-gc4df23f792

Re: [PATCH v2] compat: make sure git_mmap is not expected to write

[Junio C Hamano]

Carlo Marcelo Arenas Belón   writes:

> in f48000fc ("Yank writing-back support from gitfakemmap.", 2005-10-08)
> support for writting back changes was removed but the specific prot
> flag that would be used was not checked for
>
> Acked-by: Johannes Schindelin 
> Signed-off-by: Carlo Marcelo Arenas Belón 
> ---
> Changes in v2:
> 
> * reset-author to match signature
> * cleanup commit message and add ACK

Thanks.  Looking good.


>  compat/mmap.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/compat/mmap.c b/compat/mmap.c
> index 7f662fef7b..14d31010df 100644
> --- a/compat/mmap.c
> +++ b/compat/mmap.c
> @@ -4,7 +4,7 @@ void *git_mmap(void *start, size_t length, int prot, int 
> flags, int fd, off_t of
>  {
>   size_t n = 0;
>  
> - if (start != NULL || !(flags & MAP_PRIVATE))
> + if (start != NULL || flags != MAP_PRIVATE || prot != PROT_READ)
>   die("Invalid usage of mmap when built with NO_MMAP");
>  
>   start = xmalloc(length);

Re: [RFC 2/2] exclude-promisor-objects: declare when option is allowed

[Junio C Hamano]

Matthew DeVore  writes:

> On Tue, 23 Oct 2018, Junio C Hamano wrote:
>
>> Not really.  We were already doing a controlled failure via die(),
>> so these two tests would not have caught the problem in the code
>> before the fix in this patch.
>>
>
> BUG is apparently considered a "wrong" failure and not a controlled one
> by test_must_fail. I just double-checked that the tests fail without
> this patch.

Ah, I was testing a wrong codepath.  Yes, it does call BUG("..."),
which is a prettier-looking abort(), but I somehow thought it was
doing die("BUG: ...").

In any case, thanks for the fix.

Re: [PATCH v3 1/3] repack: point out a bug handling stale shallow info

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" 
writes:

> From: Johannes Schindelin 
>
> A `git fetch --prune` can turn previously-reachable objects unreachable,
> even commits that are in the `shallow` list. A subsequent `git repack
> -ad` will then unceremoniously drop those unreachable commits, and the
> `shallow` list will become stale. This means that when we try to fetch
> with a larger `--depth` the next time, we may end up with:
>
>   fatal: error in object: unshallow 

Nicely analysed and described.  One minor thing nagged me in the
implementation but it is not a big issue.

> +...
> + d="$(git -C shallow-server rev-parse --verify D)" &&
> + git -C shallow-server checkout master &&
> +
> +...
> + ! grep $d shallow-client/.git/shallow &&

We know D (and $d) is not a tag, but perhaps the place that assigns
to $d (way above) can do the rev-parse of D^0, not D, to make it
more clear what is going on, especially given that...

> + git -C shallow-server branch branch-orig D^0 &&

... this does that D^0 thing here to makes us wonder if D needs
unwrapping before using it as a commit (not commit-ish). 

If we did so, we could use $d here instead of D^0.

> + git -C shallow-client fetch --prune --depth=2 \
> + origin "+refs/heads/*:refs/remotes/origin/*"
> +'
> +
>  . "$TEST_DIRECTORY"/lib-httpd.sh
>  start_httpd

Re: [PATCH] range-diff: allow to diff files regardless submodule

[Lucas De Marchi]

On Wed, Oct 24, 2018 at 11:12:51AM +0900, Junio C Hamano wrote:
> Lucas De Marchi  writes:
> 
> >> Yes, I agree on both counts (i.e. it was totally unclear what
> >> problem is being solved and what the root cause of the problem is,
> >> and we would want a new test to protect this "fix" from getting
> >> broken in the future.
> >
> > have you seen I sent a v2 with proper test?
> 
> No, otherwise I wouln't have said it needs tests, and no, because I
> haven't seen the v2, I do not know if it came with proper test or
> other issues pointed out and fixes suggested in the review round
> were addressed in v2.  Sorry.

Your reply arrived just a little after I sent the v2, so I thought it
was just the race and you would end up seeing the unread email in the
same thread. Sorry for not including the msg id:
20181011081750.24240-1-lucas.demar...@intel.com

thanks
Lucas De Marchi

> 
> When you ask such a question, please accompany it with "this is the
> message-id" to avoid the receiver of the question locating a wrong
> version of your patch from the archive.
> 
> Thanks.

Re: [PATCH v3 2/3] shallow: offer to prune only non-existing entries

[Junio C Hamano]

Jonathan, do you see any issues with the use of lookup_commit() in
this change wrt lazy clone?  I am wondering what happens when the
commit in question is at, an immediate parent of, or an immediate
child of a promisor object.  I _think_ this change won't make it
worse for two features in playing together, but thought that it
would be better to double check.

"Johannes Schindelin via GitGitGadget" 
writes:

> From: Johannes Schindelin 
>
> The `prune_shallow()` function wants a full reachability check to be
> completed before it goes to work, to ensure that all unreachable entries
> are removed from the shallow file.
>
> However, in the upcoming patch we do not even want to go that far. We
> really only need to remove entries corresponding to pruned commits, i.e.
> to commits that no longer exist.
>
> Let's support that use case.
>
> Signed-off-by: Johannes Schindelin 
> ---
>  builtin/prune.c |  2 +-
>  commit.h|  2 +-
>  shallow.c   | 22 +-
>  3 files changed, 19 insertions(+), 7 deletions(-)
>
> diff --git a/builtin/prune.c b/builtin/prune.c
> index 41230f821..6d6ab6cf1 100644
> --- a/builtin/prune.c
> +++ b/builtin/prune.c
> @@ -161,7 +161,7 @@ int cmd_prune(int argc, const char **argv, const char 
> *prefix)
>   free(s);
>  
>   if (is_repository_shallow(the_repository))
> - prune_shallow(show_only);
> + prune_shallow(show_only, 0);
>  
>   return 0;
>  }
> diff --git a/commit.h b/commit.h
> index 1d260d62f..ff34447ab 100644
> --- a/commit.h
> +++ b/commit.h
> @@ -249,7 +249,7 @@ extern void assign_shallow_commits_to_refs(struct 
> shallow_info *info,
>  uint32_t **used,
>  int *ref_status);
>  extern int delayed_reachability_test(struct shallow_info *si, int c);
> -extern void prune_shallow(int show_only);
> +extern void prune_shallow(int show_only, int quick_prune);
>  extern struct trace_key trace_shallow;
>  
>  extern int interactive_add(int argc, const char **argv, const char *prefix, 
> int patch);
> diff --git a/shallow.c b/shallow.c
> index 732e18d54..0a2671bc2 100644
> --- a/shallow.c
> +++ b/shallow.c
> @@ -247,6 +247,7 @@ static void check_shallow_file_for_update(struct 
> repository *r)
>  
>  #define SEEN_ONLY 1
>  #define VERBOSE   2
> +#define QUICK_PRUNE 4
>  
>  struct write_shallow_data {
>   struct strbuf *out;
> @@ -261,7 +262,11 @@ static int write_one_shallow(const struct commit_graft 
> *graft, void *cb_data)
>   const char *hex = oid_to_hex(&graft->oid);
>   if (graft->nr_parent != -1)
>   return 0;
> - if (data->flags & SEEN_ONLY) {
> + if (data->flags & QUICK_PRUNE) {
> + struct commit *c = lookup_commit(the_repository, &graft->oid);
> + if (!c || parse_commit(c))
> + return 0;
> + } else if (data->flags & SEEN_ONLY) {
>   struct commit *c = lookup_commit(the_repository, &graft->oid);
>   if (!c || !(c->object.flags & SEEN)) {
>   if (data->flags & VERBOSE)
> @@ -371,16 +376,23 @@ void advertise_shallow_grafts(int fd)
>  
>  /*
>   * mark_reachable_objects() should have been run prior to this and all
> - * reachable commits marked as "SEEN".
> + * reachable commits marked as "SEEN", except when quick_prune is non-zero,
> + * in which case lines are excised from the shallow file if they refer to
> + * commits that do not exist (any longer).
>   */
> -void prune_shallow(int show_only)
> +void prune_shallow(int show_only, int quick_prune)
>  {
>   struct lock_file shallow_lock = LOCK_INIT;
>   struct strbuf sb = STRBUF_INIT;
> + unsigned flags = SEEN_ONLY;
>   int fd;
>  
> + if (quick_prune)
> + flags |= QUICK_PRUNE;
> +
>   if (show_only) {
> - write_shallow_commits_1(&sb, 0, NULL, SEEN_ONLY | VERBOSE);
> + flags |= VERBOSE;
> + write_shallow_commits_1(&sb, 0, NULL, flags);
>   strbuf_release(&sb);
>   return;
>   }
> @@ -388,7 +400,7 @@ void prune_shallow(int show_only)
>  git_path_shallow(the_repository),
>  LOCK_DIE_ON_ERROR);
>   check_shallow_file_for_update(the_repository);
> - if (write_shallow_commits_1(&sb, 0, NULL, SEEN_ONLY)) {
> + if (write_shallow_commits_1(&sb, 0, NULL, flags)) {
>   if (write_in_full(fd, sb.buf, sb.len) < 0)
>   die_errno("failed to write to %s",
> get_lock_file_path(&shallow_lock));