Re: [PATCH 1/1] Mark messages for translations

[Johannes Sixt]

Am 15.01.2018 um 06:44 schrieb Alexander Shopov:

@@ -5,11 +5,11 @@
  #include "run-command.h"
  
  const char git_usage_string[] =

-   "git [--version] [--help] [-C ] [-c name=value]\n"
-   "   [--exec-path[=]] [--html-path] [--man-path] 
[--info-path]\n"
-   "   [-p | --paginate | --no-pager] [--no-replace-objects] 
[--bare]\n"
-   "   [--git-dir=] [--work-tree=] 
[--namespace=]\n"
-   "[]";
+   N_("git [--version] [--help] [-C ] [-c name=value]\n"
+  "   [--exec-path[=]] [--html-path] [--man-path] 
[--info-path]\n"
+  "   [-p | --paginate | --no-pager] [--no-replace-objects] 
[--bare]\n"
+  "   [--git-dir=] [--work-tree=] 
[--namespace=]\n"
+  "[]");
  
  const char git_more_info_string[] =

N_("'git help -a' and 'git help -g' list available subcommands and 
some\n"
@@ -92,7 +92,7 @@ static int handle_options(const char ***argv, int *argc, int 
*envchanged)
*envchanged = 1;
} else if (!strcmp(cmd, "--git-dir")) {
if (*argc < 2) {
-   fprintf(stderr, "No directory given for 
--git-dir.\n" );
+   fprintf(stderr, _("No directory given for 
--git-dir.\n" ));
usage(git_usage_string);


It is not obvious to me where git_usage_string is looked up in the 
message catalog. Should this not be


usage(_(git_usage_string));

(here and in later instances)? It is used that way in builtin/help.c, 
for example.



@@ -385,14 +385,14 @@ void setup_work_tree(void)
return;
  
  	if (work_tree_config_is_bogus)

-   die("unable to set up work tree using invalid config");
+   die(_("unable to set up work tree using invalid config"));
  
  	work_tree = get_git_work_tree();

git_dir = get_git_dir();
if (!is_absolute_path(git_dir))
git_dir = real_path(get_git_dir());
if (!work_tree || chdir(work_tree))
-   die("This operation must be run in a work tree");
+   die(_("This operation must be run in a work tree"));


We have settled with lower-case letters at the beginning of error 
messages. (See Documentation/CodingGuidelines, "Error Messages".) You 
could fix that while you are touching die, die_errno, etc, messages.



@@ -677,12 +677,12 @@ static const char *setup_explicit_git_dir(const char 
*gitdirenv,
else {
char *core_worktree;
if (chdir(gitdirenv))
-   die_errno("Could not chdir to '%s'", gitdirenv);
+   die_errno(_("Cannot chdir to '%s'"), gitdirenv);


I notice you change past tense to present tense in some cases. IMO, this 
makes the messages more consistent. Good.


I'm not a friend of geeky abbreviations like "chdir" or "cwd" in 
user-visible messages, and I would have taken the opportunity to change 
the messages accordingly. This is really only my personal taste, though, 
and it's possible that I'm alone in this camp.



if (chdir(git_work_tree_cfg))
-   die_errno("Could not chdir to '%s'", 
git_work_tree_cfg);
+   die_errno(_("Cannot chdir to '%s'"), 
git_work_tree_cfg);
core_worktree = xgetcwd();
if (chdir(cwd->buf))
-   die_errno("Could not come back to cwd");
+   die_errno(_("Cannot come back to cwd");

...

@@ -1207,7 +1207,7 @@ void sanitize_stdfds(void)
while (fd != -1 && fd < 2)
fd = dup(fd);
if (fd == -1)
-   die_errno("open /dev/null or dup failed");
+   die_errno(_("open /dev/null or dup failed"));
if (fd > 2)
close(fd);
  }
@@ -1222,12 +1222,12 @@ int daemonize(void)
case 0:
break;
case -1:
-   die_errno("fork failed");
+   die_errno(_("fork failed"));
default:
exit(0);
}
if (setsid() == -1)
-   die_errno("setsid failed");
+   die_errno(_("setsid failed"));


Here is a certain class of errors: They should occur only rarely 
(actually, is that true?) Then it is useful to have the function name in 
the message. Which rises the question: why translate them at all? It's 
possible that translators turn the message into unusable gibberish just 
to please their language. All of this is only IMHO; I don't use 
translated Git.


-- Hannes

Draft of Git Rev News edition 35

[Christian Couder]

Hi,

A draft of a new Git Rev News edition is available here:

  https://github.com/git/git.github.io/blob/master/rev_news/drafts/edition-35.md

Everyone is welcome to contribute in any section either by editing the
above page on GitHub and sending a pull request, or by commenting on
this GitHub issue:

  https://github.com/git/git.github.io/issues/271

You can also reply to this email.

In general all kinds of contribution, for example proofreading,
suggestions for articles or links, help on the issues in GitHub, and
so on, are very much appreciated.

I tried to cc everyone who appears in this edition, but maybe I missed
some people, sorry about that.

Jakub, Markus, Gabriel and me plan to publish this edition on
Wednesday January 17th.

Thanks,
Christian.

Re: [BUG] test_must_fail: does not correctly detect failures - Was Git 2.16.0-rc2 Test Summary on NonStop

[Johannes Sixt]

Am 15.01.2018 um 03:37 schrieb Randall S. Becker:

On January 14, 2018 4:33 PM, I wrote:

The exotic error code coming back from perl is 162. I can muck with it, if
there was a value more useful to git.
*  553  } else if (WIFEXITED(status)) {
*  554  code = WEXITSTATUS(status);
(eInspect 3,887):p code
$4 = 162

The perl code uses die to terminate with a non-specific non-zero error code.
What it seems is that there is an assumed value that the git tests depend on,
but perl.org describes the following:

"die raises an exception. Inside an eval the error message is stuffed into $@
and the eval is terminated with the undefined value. If the exception is
outside of all enclosing evals, then the uncaught exception prints LIST to
STDERR and exits with a non-zero value. If you need to exit the process with
a specific exit code, see exit."


I take "die exits with non-zero" as a piece of information for the 
*users* so that they can write "if perl foo.pl; then something; fi" in 
shell scripts. I do *not* interpret it as leeway for implementers of 
perl to choose any random value as exit code. Choosing 162 just to be 
funky would be short-sighted. [I'm saying all this without knowing how 
perl specifies 'die' beyond the paragraph you cited. Perhaps there's 
more about 'die' that justifies exit code 162.] I'd say that the perl 
port is broken.




So it seems that we might need to either not use die or change the tests not
to care about the exit code for specific tests involving perl. Suggestions?


Sadly, while changing the funky 162 completion code to 255 works
fine for t9001, it causes a bunch of other tests to fail (parts of
1308 and most of 1404). I can't tall whether this is test suite or
code related but I'm caught in the middle. Going to the platform
developers may eventually get the fix for t9001 (fixing perl), but
otherwise, I'm not sure why changing 162 to 255 causes 1308 and 1404
to blow so badly. In any event, I'm putting this away for a few days
due to $DAYJOB.


Why do you not choose 1? He, on my Linux box perl -e die exits with 255. 
So...


-- Hannes

[PATCH 1/1] Mark messages for translations

[Alexander Shopov]

Reuse already translated messages if possible
Do not translate messages aimed at developers of git

Signed-off-by: Alexander Shopov 
---
 git.c   | 30 +++---
 setup.c | 52 ++--
 2 files changed, 41 insertions(+), 41 deletions(-)

diff --git a/git.c b/git.c
index c870b9719..e66316ce5 100644
--- a/git.c
+++ b/git.c
@@ -5,11 +5,11 @@
 #include "run-command.h"
 
 const char git_usage_string[] =
-   "git [--version] [--help] [-C ] [-c name=value]\n"
-   "   [--exec-path[=]] [--html-path] [--man-path] 
[--info-path]\n"
-   "   [-p | --paginate | --no-pager] [--no-replace-objects] 
[--bare]\n"
-   "   [--git-dir=] [--work-tree=] 
[--namespace=]\n"
-   "[]";
+   N_("git [--version] [--help] [-C ] [-c name=value]\n"
+  "   [--exec-path[=]] [--html-path] [--man-path] 
[--info-path]\n"
+  "   [-p | --paginate | --no-pager] [--no-replace-objects] 
[--bare]\n"
+  "   [--git-dir=] [--work-tree=] 
[--namespace=]\n"
+  "[]");
 
 const char git_more_info_string[] =
N_("'git help -a' and 'git help -g' list available subcommands and 
some\n"
@@ -92,7 +92,7 @@ static int handle_options(const char ***argv, int *argc, int 
*envchanged)
*envchanged = 1;
} else if (!strcmp(cmd, "--git-dir")) {
if (*argc < 2) {
-   fprintf(stderr, "No directory given for 
--git-dir.\n" );
+   fprintf(stderr, _("No directory given for 
--git-dir.\n" ));
usage(git_usage_string);
}
setenv(GIT_DIR_ENVIRONMENT, (*argv)[1], 1);
@@ -106,7 +106,7 @@ static int handle_options(const char ***argv, int *argc, 
int *envchanged)
*envchanged = 1;
} else if (!strcmp(cmd, "--namespace")) {
if (*argc < 2) {
-   fprintf(stderr, "No namespace given for 
--namespace.\n" );
+   fprintf(stderr, _("No namespace given for 
--namespace.\n" ));
usage(git_usage_string);
}
setenv(GIT_NAMESPACE_ENVIRONMENT, (*argv)[1], 1);
@@ -120,7 +120,7 @@ static int handle_options(const char ***argv, int *argc, 
int *envchanged)
*envchanged = 1;
} else if (!strcmp(cmd, "--work-tree")) {
if (*argc < 2) {
-   fprintf(stderr, "No directory given for 
--work-tree.\n" );
+   fprintf(stderr, _("No directory given for 
--work-tree.\n" ));
usage(git_usage_string);
}
setenv(GIT_WORK_TREE_ENVIRONMENT, (*argv)[1], 1);
@@ -134,7 +134,7 @@ static int handle_options(const char ***argv, int *argc, 
int *envchanged)
*envchanged = 1;
} else if (!strcmp(cmd, "--super-prefix")) {
if (*argc < 2) {
-   fprintf(stderr, "No prefix given for 
--super-prefix.\n" );
+   fprintf(stderr, _("No prefix given for 
--super-prefix.\n" ));
usage(git_usage_string);
}
setenv(GIT_SUPER_PREFIX_ENVIRONMENT, (*argv)[1], 1);
@@ -156,7 +156,7 @@ static int handle_options(const char ***argv, int *argc, 
int *envchanged)
*envchanged = 1;
} else if (!strcmp(cmd, "-c")) {
if (*argc < 2) {
-   fprintf(stderr, "-c expects a configuration 
string\n" );
+   fprintf(stderr, _("-c expects a configuration 
string\n" ));
usage(git_usage_string);
}
git_config_push_parameter((*argv)[1]);
@@ -194,7 +194,7 @@ static int handle_options(const char ***argv, int *argc, 
int *envchanged)
*envchanged = 1;
} else if (!strcmp(cmd, "-C")) {
if (*argc < 2) {
-   fprintf(stderr, "No directory given for -C.\n" 
);
+   fprintf(stderr, _("No directory given for 
-C.\n" ));
usage(git_usage_string);
}
if ((*argv)[1][0]) {
@@ -209,7 +209,7 @@ static int handle_options(const char ***argv, int *argc, 
int *envchanged)
list_builtins();
exit(0);
} else {
-   fprintf(stderr, "Unknown option: %s\n", cmd);
+   

[PATCH 0/1] Marked end user messages for translation

[Alexander Shopov]

I have marked several messages for translations in git.c and setup.c
They pop from time to time on normal usage of git.
The messages are aimed at end users and may help them solve issues.
I have tried to keep the C formatting conventions in the two files.
I have also reused messages which provides uniformity of the used
verb tense and will ease the work of translators.

Alexander Shopov (1):
  Mark messages for translations

 git.c   | 30 +++---
 setup.c | 52 ++--
 2 files changed, 41 insertions(+), 41 deletions(-)

-- 
2.15.1

RE: [BUG] test_must_fail: does not correctly detect failures - Was Git 2.16.0-rc2 Test Summary on NonStop

[Randall S. Becker]

On January 14, 2018 4:33 PM, I wrote:
> On January 14, 2018 1:41 PM Johannes Sixt wrote:
> > Sent: > > Am 14.01.2018 um 17:50 schrieb Randall S. Becker:
> > > Follow-up: This looks like the completion code from perl on NonStop
> > > is not the same as expected by git in the case of failures. I need
> > > to debug this to get more details to the team. We have had
> > > completion issues before relating to interpretation problems between
> > > perl, bash, and git in this platform, so I'm assuming this to be
> > > likely again but need to
> > track down the specifics.
> > > Can anyone point me to where the detection is within git or the execv?
> >
> > Perhaps you are looking for wait_or_whine() in run-command.c? But this
> > function cannot do anything if perl alread exits with an exotic code
> > (>
> > 128 even though no signal was received).
> 
> The exotic error code coming back from perl is 162. I can muck with it, if
> there was a value more useful to git.
> *  553  } else if (WIFEXITED(status)) {
> *  554  code = WEXITSTATUS(status);
> (eInspect 3,887):p code
> $4 = 162
> 
> The perl code uses die to terminate with a non-specific non-zero error code.
> What it seems is that there is an assumed value that the git tests depend on,
> but perl.org describes the following:
> 
> "die raises an exception. Inside an eval the error message is stuffed into $@
> and the eval is terminated with the undefined value. If the exception is
> outside of all enclosing evals, then the uncaught exception prints LIST to
> STDERR and exits with a non-zero value. If you need to exit the process with
> a specific exit code, see exit."
> 
> So it seems that we might need to either not use die or change the tests not
> to care about the exit code for specific tests involving perl. Suggestions?

Sadly, while changing the funky 162 completion code to 255 works fine for 
t9001, it causes a bunch of other tests to fail (parts of 1308 and most of 
1404). I can't tall whether this is test suite or code related but I'm caught 
in the middle. Going to the platform developers may eventually get the fix for 
t9001 (fixing perl), but otherwise, I'm not sure why changing 162 to 255 causes 
1308 and 1404 to blow so badly. In any event, I'm putting this away for a few 
days due to $DAYJOB.

Cheers,
Randall

RE: [BUG] test_must_fail: does not correctly detect failures - Was Git 2.16.0-rc2 Test Summary on NonStop

[Randall S. Becker]

> -Original Message-
> From: Johannes Sixt [mailto:j...@kdbg.org]
> Sent: On January 14, 2018 1:41 PM wrote:
> Am 14.01.2018 um 17:50 schrieb Randall S. Becker:
> > Follow-up: This looks like the completion code from perl on NonStop is
> > not the same as expected by git in the case of failures. I need to
> > debug this to get more details to the team. We have had completion
> > issues before relating to interpretation problems between perl, bash,
> > and git in this platform, so I'm assuming this to be likely again but need 
> > to
> track down the specifics.
> > Can anyone point me to where the detection is within git or the execv?
> 
> Perhaps you are looking for wait_or_whine() in run-command.c? But this
> function cannot do anything if perl alread exits with an exotic code (>
> 128 even though no signal was received).

The exotic error code coming back from perl is 162. I can muck with it, if 
there was a value more useful to git.
*  553  } else if (WIFEXITED(status)) {
*  554  code = WEXITSTATUS(status);
(eInspect 3,887):p code
$4 = 162

The perl code uses die to terminate with a non-specific non-zero error code. 
What it seems is that there is an assumed value that the git tests depend on, 
but perl.org describes the following:

"die raises an exception. Inside an eval the error message is stuffed into $@ 
and the eval is terminated with the undefined value. If the exception is 
outside of all enclosing evals, then the uncaught exception prints LIST to 
STDERR and exits with a non-zero value. If you need to exit the process with a 
specific exit code, see exit."

So it seems that we might need to either not use die or change the tests not to 
care about the exit code for specific tests involving perl. Suggestions?

Sincerely,
Randall

-- Brief whoami:
  NonStop developer since approximately NonStop(2112884442)
  UNIX developer since approximately 421664400
-- In my real life, I talk too much.

Re: [PATCH v2] l10n: de.po: translate 72 new messages

[Matthias Rüster]

Acked-by: Matthias Rüster 

Thanks!


Am 14.01.2018 um 08:46 schrieb Ralf Thielow:
> Translate 72 new messages came from git.pot update in 18a907225 (l10n:
> git.pot: v2.16.0 round 1 (64 new, 25 removed)) and 005c62fe4 (l10n:
> git.pot: v2.16.0 round 2 (8 new, 4 removed)).
> 
> Signed-off-by: Ralf Thielow 
> ---
> 
> Thanks for the review, Matthias!
> 
>  po/de.po | 227 
> +++
>  1 file changed, 98 insertions(+), 129 deletions(-)
> 
> diff --git a/po/de.po b/po/de.po
> index b24b28875..eef4897fb 100644
> --- a/po/de.po
> +++ b/po/de.po
> @@ -1716,7 +1716,7 @@ msgstr "Konnte Git-Verzeichnis nicht von '%s' nach '%s' 
> migrieren."
>  #: editor.c:61
>  #, c-format
>  msgid "hint: Waiting for your editor to close the file...%c"
> -msgstr ""
> +msgstr "Hinweis: Warte auf das Schließen der Datei durch Ihren Editor...%c"
>  
>  #: entry.c:177
>  msgid "Filtering content"
> @@ -2087,12 +2087,12 @@ msgstr "Ungültiges Datumsformat: %s"
>  
>  #: list-objects-filter-options.c:30
>  msgid "multiple object filter types cannot be combined"
> -msgstr ""
> +msgstr "Mehrere Arten von Objekt-Filtern können nicht kombiniert werden."
>  
>  #: list-objects-filter-options.c:41 list-objects-filter-options.c:68
> -#, fuzzy, c-format
> +#, c-format
>  msgid "invalid filter-spec expression '%s'"
> -msgstr "Ungültige Datei: '%s'"
> +msgstr "Ungültiger filter-spec Ausdruck '%s'."
>  
>  #: lockfile.c:151
>  #, c-format
> @@ -2356,9 +2356,9 @@ msgid "Adding %s"
>  msgstr "Füge %s hinzu"
>  
>  #: merge-recursive.c:1958
> -#, fuzzy, c-format
> +#, c-format
>  msgid "Dirty index: cannot merge (dirty: %s)"
> -msgstr "Geänderter Index: kann Patches nicht anwenden (geändert: %s)"
> +msgstr "Geänderter Index: kann nicht mergen (geändert: %s)"
>  
>  #: merge-recursive.c:1962
>  msgid "Already up to date!"
> @@ -3015,6 +3015,8 @@ msgid ""
>  "The '%s' hook was ignored because it's not set as executable.\n"
>  "You can disable this warning with `git config advice.ignoredHook false`."
>  msgstr ""
> +"Der '%s' Hook wurde ignoriert, weil er nicht als ausführbar markiert ist.\n"
> +"Sie können diese Warnung mit `git config advice.ignoredHook false` 
> deaktivieren."
>  
>  #: send-pack.c:141
>  #, c-format
> @@ -3137,14 +3139,12 @@ msgid "%s: Unable to write new index file"
>  msgstr "%s: Konnte neue Index-Datei nicht schreiben"
>  
>  #: sequencer.c:496
> -#, fuzzy
>  msgid "could not resolve HEAD commit"
> -msgstr "Konnte HEAD-Commit nicht auflösen\n"
> +msgstr "Konnte HEAD-Commit nicht auflösen."
>  
>  #: sequencer.c:516
> -#, fuzzy
>  msgid "unable to update cache tree"
> -msgstr "Konnte Cache-Verzeichnis nicht aktualisieren\n"
> +msgstr "Konnte Cache-Verzeichnis nicht aktualisieren."
>  
>  #: sequencer.c:600
>  #, c-format
> @@ -3178,14 +3178,14 @@ msgstr ""
>  "  git rebase --continue\n"
>  
>  #: sequencer.c:702
> -#, fuzzy, c-format
> +#, c-format
>  msgid "could not parse commit %s"
> -msgstr "Konnte Commit %s nicht parsen\n"
> +msgstr "Konnte Commit %s nicht parsen."
>  
>  #: sequencer.c:707
> -#, fuzzy, c-format
> +#, c-format
>  msgid "could not parse parent commit %s"
> -msgstr "Konnte Eltern-Commit %s nicht parsen\n"
> +msgstr "Konnte Eltern-Commit %s nicht parsen."
>  
>  #: sequencer.c:836
>  #, c-format
> @@ -3316,14 +3316,14 @@ msgid "git %s: failed to refresh the index"
>  msgstr "git %s: Fehler beim Aktualisieren des Index"
>  
>  #: sequencer.c:1270
> -#, fuzzy, c-format
> +#, c-format
>  msgid "%s does not accept arguments: '%s'"
> -msgstr "%%(body) akzeptiert keine Argumente"
> +msgstr "%s akzeptiert keine Argumente: '%s'"
>  
>  #: sequencer.c:1279
> -#, fuzzy, c-format
> +#, c-format
>  msgid "missing arguments for %s"
> -msgstr "Objekt %s fehlt für %s"
> +msgstr "Fehlende Argumente für %s."
>  
>  #: sequencer.c:1322
>  #, c-format
> @@ -4965,7 +4965,7 @@ msgstr "versionierte Dateien aktualisieren"
>  
>  #: builtin/add.c:299
>  msgid "renormalize EOL of tracked files (implies -u)"
> -msgstr ""
> +msgstr "erneutes Normalisieren der Zeilenenden von versionierten Dateien 
> (impliziert -u)"
>  
>  #: builtin/add.c:300
>  msgid "record only the fact that the path will be added later"
> @@ -5507,36 +5507,34 @@ msgstr "git bisect--helper --next-all [--no-checkout]"
>  
>  #: builtin/bisect--helper.c:13
>  msgid "git bisect--helper --write-terms  "
> -msgstr ""
> +msgstr "git bisect--helper --write-terms  "
>  
>  #: builtin/bisect--helper.c:14
> -#, fuzzy
>  msgid "git bisect--helper --bisect-clean-state"
> -msgstr "git bisect--helper --next-all [--no-checkout]"
> +msgstr "git bisect--helper --bisect-clean-state"
>  
>  #: builtin/bisect--helper.c:46
> -#, fuzzy, c-format
> +#, c-format
>  msgid "'%s' is not a valid term"
> -msgstr "'%s' ist keine gültige Referenz."
> +msgstr "'%s' ist kein gültiger Begriff."
>  
>  #: builtin/bisect--helper.c:50
> -#, fuzzy, c-format
> +#, c-format
>  msgid "can't use the 

[PATCH v2] packed_ref_cache: don't use mmap() for small files

[Kim Gybels]

Take a hint from commit ea68b0ce9f8 (hash-object: don't use mmap() for
small files, 2010-02-21) and use read() instead of mmap() for small
packed-refs files.

This also fixes the problem[1] where xmmap() returns NULL for zero
length[2], for which munmap() later fails.

Alternatively, we could simply check for NULL before munmap(), or
introduce xmunmap() that could be used together with xmmap().

[1] https://github.com/git-for-windows/git/issues/1410
[2] Logic introduced in commit 9130ac1e196 (Better error messages for
corrupt databases, 2007-01-11)

Signed-off-by: Kim Gybels 
---
Change since v1: reworded commit message based on Johannes Schindelin's
feedback: shorter commit hashes, and included commit titles.

 refs/packed-backend.c | 14 --
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index dab8a85d9a..7177e5bc2f 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -455,6 +455,8 @@ static void verify_buffer_safe(struct snapshot *snapshot)
 last_line, eof - last_line);
 }
 
+#define SMALL_FILE_SIZE (32*1024)
+
 /*
  * Depending on `mmap_strategy`, either mmap or read the contents of
  * the `packed-refs` file into the snapshot. Return 1 if the file
@@ -489,21 +491,21 @@ static int load_contents(struct snapshot *snapshot)
die_errno("couldn't stat %s", snapshot->refs->path);
size = xsize_t(st.st_size);
 
-   switch (mmap_strategy) {
-   case MMAP_NONE:
+   if (!size) {
+   snapshot->buf = NULL;
+   snapshot->eof = NULL;
+   snapshot->mmapped = 0;
+   } else if (size <= SMALL_FILE_SIZE || mmap_strategy == MMAP_NONE) {
snapshot->buf = xmalloc(size);
bytes_read = read_in_full(fd, snapshot->buf, size);
if (bytes_read < 0 || bytes_read != size)
die_errno("couldn't read %s", snapshot->refs->path);
snapshot->eof = snapshot->buf + size;
snapshot->mmapped = 0;
-   break;
-   case MMAP_TEMPORARY:
-   case MMAP_OK:
+   } else {
snapshot->buf = xmmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 
0);
snapshot->eof = snapshot->buf + size;
snapshot->mmapped = 1;
-   break;
}
close(fd);
 
-- 
2.16.0.rc2.windows.1

Re: [BUG] test_must_fail: does not correctly detect failures - Was Git 2.16.0-rc2 Test Summary on NonStop

[Johannes Sixt]

Am 14.01.2018 um 17:50 schrieb Randall S. Becker:

Follow-up: This looks like the completion code from perl on NonStop is not
the same as expected by git in the case of failures. I need to debug this to
get more details to the team. We have had completion issues before relating
to interpretation problems between perl, bash, and git in this platform, so
I'm assuming this to be likely again but need to track down the specifics.
Can anyone point me to where the detection is within git or the execv?


Perhaps you are looking for wait_or_whine() in run-command.c? But this 
function cannot do anything if perl alread exits with an exotic code (> 
128 even though no signal was received).


-- Hannes

RE: [PATCH] Remoted unnecessary void* from hashmap.h that caused compile warnings

[Randall S. Becker]

On January 14, 2018 12:25 PM, Philip Oakley wrote:
> To: randall.s.bec...@rogers.com; git@vger.kernel.org
> Cc: Randall S. Becker 
> Subject: Re: [PATCH] Remoted unnecessary void* from hashmap.h that
> caused compile warnings
> 
> From: 
> Subject: [PATCH] Remoted unnecessary void* from hashmap.h that caused
> compile warnings
> 
> s/Remoted/Removed/ ?
> 
> Maybe shorten to " hashmap.h: remove unnecessary void* " (ex the
> superflous
> spaces)

I amended the commit fixing the typo and cleaning up the comment.

Thanks,
Randall

[PATCH] Removed unnecessary void* from hashmap.h that caused compile warnings

[randall . s . becker]

From: "Randall S. Becker" 

* hashmap.h: Revised the while loop in the hashmap_enable_item_counting
to remove unneeded void* item.

Signed-off-by: Randall S. Becker 
---
 hashmap.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hashmap.h b/hashmap.h
index 7ce79f3..d375d9c 100644
--- a/hashmap.h
+++ b/hashmap.h
@@ -400,7 +400,6 @@ static inline void hashmap_disable_item_counting(struct 
hashmap *map)
  */
 static inline void hashmap_enable_item_counting(struct hashmap *map)
 {
-   void *item;
unsigned int n = 0;
struct hashmap_iter iter;
 
@@ -408,7 +407,7 @@ static inline void hashmap_enable_item_counting(struct 
hashmap *map)
return;
 
hashmap_iter_init(map, );
-   while ((item = hashmap_iter_next()))
+   while (hashmap_iter_next())
n++;
 
map->do_count_items = 1;
-- 
2.8.5.23.g6fa7ec3

[PATCH v3 2/2] Doc/git-submodule: improve readability and grammar of a sentence

[Kaartic Sivaraam]

While at it, correctly quote important words.

Signed-off-by: Kaartic Sivaraam 
---
 Documentation/git-submodule.txt | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index ff612001d..71c5618e8 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -70,8 +70,8 @@ status [--cached] [--recursive] [--] [...]::
Show the status of the submodules. This will print the SHA-1 of the
currently checked out commit for each submodule, along with the
submodule path and the output of 'git describe' for the
-   SHA-1. Each SHA-1 will be prefixed with `-` if the submodule is not
-   initialized, `+` if the currently checked out submodule commit
+   SHA-1. Each SHA-1 will possibly be prefixed with `-` if the submodule is
+   not initialized, `+` if the currently checked out submodule commit
does not match the SHA-1 found in the index of the containing
repository and `U` if the submodule has merge conflicts.
 +
@@ -132,15 +132,15 @@ expects by cloning missing submodules and updating the 
working tree of
 the submodules. The "updating" can be done in several ways depending
 on command line options and the value of `submodule..update`
 configuration variable. The command line option takes precedence over
-the configuration variable. if neither is given, a checkout is performed.
-update procedures supported both from the command line as well as setting
-`submodule..update`:
+the configuration variable. If neither is given, a 'checkout' is performed.
+The 'update' procedures supported both from the command line as well as
+through the `submodule..update` configuration are:
 
checkout;; the commit recorded in the superproject will be
checked out in the submodule on a detached HEAD.
 +
 If `--force` is specified, the submodule will be checked out (using
-`git checkout --force` if appropriate), even if the commit specified
+`git checkout --force`), even if the commit specified
 in the index of the containing repository already matches the commit
 checked out in the submodule.
 
@@ -150,8 +150,8 @@ checked out in the submodule.
merge;; the commit recorded in the superproject will be merged
into the current branch in the submodule.
 
-The following procedures are only available via the `submodule..update`
-configuration variable:
+The following 'update' procedures are only available via the
+`submodule..update` configuration variable:
 
custom command;; arbitrary shell command that takes a single
argument (the sha1 of the commit recorded in the
-- 
2.16.0.rc1.281.g969645f98

[PATCH v3 1/2] Doc/gitsubmodules: make some changes to improve readability and syntax

[Kaartic Sivaraam]

* Only mention porcelain commands in examples

* Split a sentence for better readability

* Add missing apostrophes

* Clearly specify the advantages of using submodules

* Avoid abbreviations

* Use "Git" consistently

* Improve readability of certain lines

* Clarify when a submodule is considered active

Helped-by: Eric Sunshine 
Helped-by: Stefan Beller 
Signed-off-by: Kaartic Sivaraam 
---
 Documentation/gitsubmodules.txt | 100 +++-
 1 file changed, 79 insertions(+), 21 deletions(-)

diff --git a/Documentation/gitsubmodules.txt b/Documentation/gitsubmodules.txt
index 46cf120f6..4d6c17782 100644
--- a/Documentation/gitsubmodules.txt
+++ b/Documentation/gitsubmodules.txt
@@ -36,8 +36,8 @@ The `gitlink` entry contains the object name of the commit 
that the
 superproject expects the submodule’s working directory to be at.
 
 The section `submodule.foo.*` in the `.gitmodules` file gives additional
-hints to Gits porcelain layer such as where to obtain the submodule via
-the `submodule.foo.url` setting.
+hints to Git's porcelain layer. For example, the `submodule.foo.url`
+setting specifies where to obtain the submodule.
 
 Submodules can be used for at least two different use cases:
 
@@ -51,18 +51,21 @@ Submodules can be used for at least two different use cases:
 
 2. Splitting a (logically single) project into multiple
repositories and tying them back together. This can be used to
-   overcome current limitations of Gits implementation to have
+   overcome current limitations of Git's implementation to have
finer grained access:
 
-* Size of the git repository:
+* Size of the Git repository:
   In its current form Git scales up poorly for large repositories 
containing
   content that is not compressed by delta computation between trees.
-  However you can also use submodules to e.g. hold large binary assets
-  and these repositories are then shallowly cloned such that you do not
+  For example, you can use submodules to hold large binary assets
+  and these repositories can be shallowly cloned such that you do not
   have a large history locally.
 * Transfer size:
   In its current form Git requires the whole working tree present. It
   does not allow partial trees to be transferred in fetch or clone.
+  If the project you work on consists of multiple repositories tied
+  together as submodules in a superproject, you can avoid fetching the
+  working trees of the repositories you are not interested in.
 * Access control:
   By restricting user access to submodules, this can be used to implement
   read/write policies for different users.
@@ -73,9 +76,10 @@ The configuration of submodules
 Submodule operations can be configured using the following mechanisms
 (from highest to lowest precedence):
 
- * The command line for those commands that support taking submodule specs.
-   Most commands have a boolean flag '--recurse-submodules' whether to
-   recurse into submodules. Examples are `ls-files` or `checkout`.
+ * The command line for those commands that support taking submodules
+   as part of their pathspecs. Most commands have a boolean flag
+   `--recurse-submodules` which specify whether to recurse into submodules.
+   Examples are `grep` and `checkout`.
Some commands take enums, such as `fetch` and `push`, where you can
specify how submodules are affected.
 
@@ -87,8 +91,8 @@ Submodule operations can be configured using the following 
mechanisms
 For example an effect from the submodule's `.gitignore` file
 would be observed when you run `git status --ignore-submodules=none` in
 the superproject. This collects information from the submodule's working
-directory by running `status` in the submodule, which does pay attention
-to its `.gitignore` file.
+directory by running `status` in the submodule while paying attention
+to the `.gitignore` file of the submodule.
 +
 The submodule's `$GIT_DIR/config` file would come into play when running
 `git push --recurse-submodules=check` in the superproject, as this would
@@ -97,20 +101,20 @@ remotes are configured in the submodule as usual in the 
`$GIT_DIR/config`
 file.
 
  * The configuration file `$GIT_DIR/config` in the superproject.
-   Typical configuration at this place is controlling if a submodule
-   is recursed into at all via the `active` flag for example.
+   Git only recurses into active submodules (see "ACTIVE SUBMODULES"
+   section below).
 +
 If the submodule is not yet initialized, then the configuration
-inside the submodule does not exist yet, so configuration where to
+inside the submodule does not exist yet, so where to
 obtain the submodule from is configured here for example.
 
- * the `.gitmodules` file inside the superproject. Additionally to the
-   required mapping between submodule's name and path, a project usually
+ * The 

[PATCH v3 0/2] Doc/submodules: a few updates

[Kaartic Sivaraam]

Quoting from v1,

These are just a few improvements that I thought would make the 
documentation
related to submodules a little better in various way such as readability,
consistency etc., These were things I noticed while reading thise documents.

Changes since v2:

   - Made some changes suggested by Stefan.
   - A few more that caught my eyes.

Inter diff between v2 and v3:

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index 801d291ca..71c5618e8 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -70,8 +70,8 @@ status [--cached] [--recursive] [--] [...]::
Show the status of the submodules. This will print the SHA-1 of the
currently checked out commit for each submodule, along with the
submodule path and the output of 'git describe' for the
-   SHA-1. Each SHA-1 will be prefixed with `-` if the submodule is not
-   initialized, `+` if the currently checked out submodule commit
+   SHA-1. Each SHA-1 will possibly be prefixed with `-` if the submodule is
+   not initialized, `+` if the currently checked out submodule commit
does not match the SHA-1 found in the index of the containing
repository and `U` if the submodule has merge conflicts.
 +
diff --git a/Documentation/gitsubmodules.txt b/Documentation/gitsubmodules.txt
index ce2369c2d..47bbc62e8 100644
--- a/Documentation/gitsubmodules.txt
+++ b/Documentation/gitsubmodules.txt
@@ -101,7 +101,7 @@ remotes are configured in the submodule as usual in the 
`$GIT_DIR/config`
 file.
 
  * The configuration file `$GIT_DIR/config` in the superproject.
-   Git only recurses into active submodules (see 'ACTIVE SUBMODULES'
+   Git only recurses into active submodules (see "ACTIVE SUBMODULES"
section below).
 +
 If the submodule is not yet initialized, then the configuration
@@ -164,52 +164,59 @@ from another repository.
 To completely remove a submodule, manually delete
 `$GIT_DIR/modules//`.
 
-Active submodules
+ACTIVE SUBMODULES
 -
 
 A submodule is considered active,
 
-  (a) if `submodule..active` is set
+  (a) if `submodule..active` is set to `true`
  or
-  (b) if the submodules path matches the pathspec in `submodule.active`
+  (b) if the submodule's path matches the pathspec in `submodule.active`
  or
   (c) if `submodule..url` is set.
 
+and these are evaluated in this order.
+
 For example:
 
-[submodule "foo"]
-active = false
-url = https://example.org/foo
-[submodule "bar"]
-active = true
-url = https://example.org/bar
-[submodule "baz"]
-url = https://example.org/baz
+  [submodule "foo"]
+active = false
+url = https://example.org/foo
+  [submodule "bar"]
+active = true
+url = https://example.org/bar
+  [submodule "baz"]
+url = https://example.org/baz
 
-In the above config only the submodule bar and baz are active,
-bar due to (a) and baz due to (c).
+In the above config only the submodule 'bar' and 'baz' are active,
+'bar' due to (a) and 'baz' due to (c). 'foo' is inactive because
+(a) takes precedence over (c).
 
-Note that '(c)' is a historical artefact and will be ignored if the
-pathspec set in (b) excludes the submodule. For example:
+Note that (c) is a historical artefact and will be ignored if the
+(a) and (b) specify that the submodule is not active. In other words,
+if we have an `submodule..active` set to `false` or if the
+submodule's path is excluded in the pathspec in `submodule.active`, the
+url doesn't matter whether it is present or not. This is illustrated in
+the example that follows.
 
-[submodule "foo"]
-active = true
-url = https://example.org/foo
-[submodule "bar"]
-url = https://example.org/bar
-[submodule "baz"]
-url = https://example.org/baz
-[submodule "bob"]
-ignore = true
-[submodule]
-active = b*
-active = (:exclude) baz
+  [submodule "foo"]
+active = true
+url = https://example.org/foo
+  [submodule "bar"]
+url = https://example.org/bar
+  [submodule "baz"]
+url = https://example.org/baz
+  [submodule "bob"]
+ignore = true
+  [submodule]
+active = b*
+active = :(exclude) baz
 
-In here all submodules except baz (foo, bar, bob) are active.
+In here all submodules except 'baz' (foo, bar, bob) are active.
 'foo' due to its own active flag and all the others due to the
 submodule active pathspec, which specifies that any submodule
-starting with 'b' except 'baz' are also active, no matter if
-the .url field is present.
+starting with 'b' except 'baz' are also active, regardless of the
+presence of the .url field.
 
 Workflow for a third party library
 --



Kaartic Sivaraam (2):
  Doc/gitsubmodules: make some changes to improve readability and syntax
  Doc/git-submodule: improve readability and grammar of a sentence

 Documentation/git-submodule.txt |  16 

Re: [PATCH] Remoted unnecessary void* from hashmap.h that caused compile warnings

[Philip Oakley]

From: 
Subject: [PATCH] Remoted unnecessary void* from hashmap.h that caused 
compile warnings


s/Remoted/Removed/ ?

Maybe shorten to " hashmap.h: remove unnecessary void* " (ex the superflous 
spaces)

--
Philip



From: "Randall S. Becker" 

* The while loop in the inline method hashmap_enable_item_counting
 used an unneeded variable. The loop has been revised accordingly.

Signed-off-by: Randall S. Becker 
---
hashmap.h | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hashmap.h b/hashmap.h
index 7ce79f3..d375d9c 100644
--- a/hashmap.h
+++ b/hashmap.h
@@ -400,7 +400,6 @@ static inline void 
hashmap_disable_item_counting(struct hashmap *map)

 */
static inline void hashmap_enable_item_counting(struct hashmap *map)
{
- void *item;
 unsigned int n = 0;
 struct hashmap_iter iter;

@@ -408,7 +407,7 @@ static inline void hashmap_enable_item_counting(struct 
hashmap *map)

 return;

 hashmap_iter_init(map, );
- while ((item = hashmap_iter_next()))
+ while (hashmap_iter_next())
 n++;

 map->do_count_items = 1;
--
2.8.5.23.g6fa7ec3

RE: [BUG] test_must_fail: does not correctly detect failures - Was Git 2.16.0-rc2 Test Summary on NonStop

[Randall S. Becker]

On January 13, 2018 3:17 PM, I wrote:
> On January 13, 2018 2:31 PM, I wrote:
> > On January 13, 2018 1:08 PM, I wrote:
> > > Here’s where things are. This is probably the best git release so
> > > far
> > (ever).
> > > After applying a4cdf02, I had 6 total breakages. 3 existing, 3 new.
> > > Many reduced. The test took about 24 hours to run on platform, which
> > > is about 2 hours shorter than 2.13.5.
> > >
> > > t1308-config-set.sh (2 already discussed and expecting a fix, both
> > > appear
> > to
> > > be issues in the test script, not code) t1404-update-ref-errors.sh #
> > > 52 – reported but not discussed:
> > >    not ok 52 - delete fails cleanly if packed-refs file is locked.
> > >      The lock detection worked, but the test assumed the detection
> > > would occur in a different spot.
> > > t9001-send-email.sh (2 have existed for 2 years. 1 is new. We have
> > > not
> > used
> > > send-email on platform to this point).
> > >    not ok 31 - reject long lines
> > >      This is a new fail since 2.8.5
> > >   not ok 106 - sendemail.transferencoding=7bit fails on 8bit data
> > >  Still to be investigated. This may be a tooling issue on
Platform.
> > >   not ok 107 - --transfer-encoding overrides
> > > sendemail.transferEncoding
> > >  Still to be investigated. This may be a tooling issue on
Platform.
> >
> > I missed one:
> > not ok 134 - --dump-aliases must be used alone #
> > #   test_must_fail git send-email --dump-aliases
> > --to=janice@example
> > .com -1 refs/heads/accounting
> 
> Running the tests in debug, I found that they all (1308, 1404, 9001) use
> test_must_fail, and hit similar situations:
> 
> expecting success:
> test_must_fail git send-email --dump-aliases
--to=jan...@example.com
> -1 refs/heads/accounting
> --dump-aliases incompatible with other options
> test_must_fail: died by signal 34: git send-email --dump-aliases --
> to=jan...@example.com -1 refs/heads/accounting not ok 134 - --dump-
> aliases must be used alone #
> #   test_must_fail git send-email --dump-aliases
> --to=jan...@example.com -1 refs/heads/accounting #
> 
> It is looking like git is doing what it is supposed to be doing, but the
test
> scripts are not detecting failures properly. The test_must_fail routine is
> interestingly used in all of the above test cases that are failing. The
actual
> exit_code reported by git was 162, (a.k.a. signal 34 - which is not thrown
on
> the platform. The max signal is 31 (SIGABEND). test_must_fail has a weird
> combination of some errors pass and others don't, but I can't correlate
the
> intent of its use in these tests particularly with no acceptable signals
passed
> in. Adding a return 1 if 162 caused other tests to fail as well, so that's
not the
> fix.

Follow-up: This looks like the completion code from perl on NonStop is not
the same as expected by git in the case of failures. I need to debug this to
get more details to the team. We have had completion issues before relating
to interpretation problems between perl, bash, and git in this platform, so
I'm assuming this to be likely again but need to track down the specifics.
Can anyone point me to where the detection is within git or the execv?

Thanks,
Randall
 

[PATCH v3 1/2] submodule: port submodule subcommand 'sync' from shell to C

[Prathamesh Chavan]

Port the submodule subcommand 'sync' from shell to C using the same
mechanism as that used for porting submodule subcommand 'status'.
Hence, here the function cmd_sync() is ported from shell to C.
This is done by introducing four functions: module_sync(),
sync_submodule(), sync_submodule_cb() and print_default_remote().

The function print_default_remote() is introduced for getting
the default remote as stdout.

Mentored-by: Christian Couder 
Mentored-by: Stefan Beller 
Signed-off-by: Prathamesh Chavan 
---
 builtin/submodule--helper.c | 193 
 git-submodule.sh|  57 +
 2 files changed, 194 insertions(+), 56 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index a5c4a8a69..745d070ea 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -50,6 +50,20 @@ static char *get_default_remote(void)
return ret;
 }
 
+static int print_default_remote(int argc, const char **argv, const char 
*prefix)
+{
+   const char *remote;
+
+   if (argc != 1)
+   die(_("submodule--helper print-default-remote takes no 
arguments"));
+
+   remote = get_default_remote();
+   if (remote)
+   printf("%s\n", remote);
+
+   return 0;
+}
+
 static int starts_with_dot_slash(const char *str)
 {
return str[0] == '.' && is_dir_sep(str[1]);
@@ -358,6 +372,25 @@ static void module_list_active(struct module_list *list)
*list = active_modules;
 }
 
+static char *get_up_path(const char *path)
+{
+   int i;
+   struct strbuf sb = STRBUF_INIT;
+
+   for (i = count_slashes(path); i; i--)
+   strbuf_addstr(, "../");
+
+   /*
+* Check if 'path' ends with slash or not
+* for having the same output for dir/sub_dir
+* and dir/sub_dir/
+*/
+   if (!is_dir_sep(path[strlen(path) - 1]))
+   strbuf_addstr(, "../");
+
+   return strbuf_detach(, NULL);
+}
+
 static int module_list(int argc, const char **argv, const char *prefix)
 {
int i;
@@ -718,6 +751,164 @@ static int module_name(int argc, const char **argv, const 
char *prefix)
return 0;
 }
 
+struct sync_cb {
+   const char *prefix;
+   unsigned int flags;
+};
+
+#define SYNC_CB_INIT { NULL, 0 }
+
+static void sync_submodule(const char *path, const char *prefix,
+  unsigned int flags)
+{
+   const struct submodule *sub;
+   char *remote_key = NULL;
+   char *sub_origin_url, *super_config_url, *displaypath;
+   struct strbuf sb = STRBUF_INIT;
+   struct child_process cp = CHILD_PROCESS_INIT;
+   char *sub_config_path = NULL;
+
+   if (!is_submodule_active(the_repository, path))
+   return;
+
+   sub = submodule_from_path(_oid, path);
+
+   if (sub && sub->url) {
+   if (starts_with_dot_dot_slash(sub->url) ||
+   starts_with_dot_slash(sub->url)) {
+   char *remote_url, *up_path;
+   char *remote = get_default_remote();
+   strbuf_addf(, "remote.%s.url", remote);
+
+   if (git_config_get_string(sb.buf, _url))
+   remote_url = xgetcwd();
+
+   up_path = get_up_path(path);
+   sub_origin_url = relative_url(remote_url, sub->url, 
up_path);
+   super_config_url = relative_url(remote_url, sub->url, 
NULL);
+
+   free(remote);
+   free(up_path);
+   free(remote_url);
+   } else {
+   sub_origin_url = xstrdup(sub->url);
+   super_config_url = xstrdup(sub->url);
+   }
+   } else {
+   sub_origin_url = xstrdup("");
+   super_config_url = xstrdup("");
+   }
+
+   displaypath = get_submodule_displaypath(path, prefix);
+
+   if (!(flags & OPT_QUIET))
+   printf(_("Synchronizing submodule url for '%s'\n"),
+displaypath);
+
+   strbuf_reset();
+   strbuf_addf(, "submodule.%s.url", sub->name);
+   if (git_config_set_gently(sb.buf, super_config_url))
+   die(_("failed to register url for submodule path '%s'"),
+ displaypath);
+
+   if (!is_submodule_populated_gently(path, NULL))
+   goto cleanup;
+
+   prepare_submodule_repo_env(_array);
+   cp.git_cmd = 1;
+   cp.dir = path;
+   argv_array_pushl(, "submodule--helper",
+"print-default-remote", NULL);
+
+   strbuf_reset();
+   if (capture_command(, , 0))
+   die(_("failed to get the default remote for submodule '%s'"),
+ path);
+
+   strbuf_strip_suffix(, "\n");
+   remote_key = xstrfmt("remote.%s.url", sb.buf);
+

[PATCH v3 2/2] submodule: port submodule subcommand 'deinit' from shell to C

[Prathamesh Chavan]

The same mechanism is used even for porting this submodule
subcommand, as used in the ported subcommands till now.
The function cmd_deinit in split up after porting into four
functions: module_deinit(), for_each_listed_submodule(),
deinit_submodule() and deinit_submodule_cb().

Mentored-by: Christian Couder 
Mentored-by: Stefan Beller 
Signed-off-by: Prathamesh Chavan 
---
 builtin/submodule--helper.c | 147 
 git-submodule.sh|  55 +
 2 files changed, 148 insertions(+), 54 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 745d070ea..b1daca995 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -20,6 +20,7 @@
 #define OPT_QUIET (1 << 0)
 #define OPT_CACHED (1 << 1)
 #define OPT_RECURSIVE (1 << 2)
+#define OPT_FORCE (1 << 3)
 
 typedef void (*each_submodule_fn)(const struct cache_entry *list_item,
  void *cb_data);
@@ -909,6 +910,151 @@ static int module_sync(int argc, const char **argv, const 
char *prefix)
return 0;
 }
 
+struct deinit_cb {
+   const char *prefix;
+   unsigned int flags;
+};
+#define DEINIT_CB_INIT { NULL, 0 }
+
+static void deinit_submodule(const char *path, const char *prefix,
+unsigned int flags)
+{
+   const struct submodule *sub;
+   char *displaypath = NULL;
+   struct child_process cp_config = CHILD_PROCESS_INIT;
+   struct strbuf sb_config = STRBUF_INIT;
+   char *sub_git_dir = xstrfmt("%s/.git", path);
+
+   sub = submodule_from_path(_oid, path);
+
+   if (!sub || !sub->name)
+   goto cleanup;
+
+   displaypath = get_submodule_displaypath(path, prefix);
+
+   /* remove the submodule work tree (unless the user already did it) */
+   if (is_directory(path)) {
+   struct strbuf sb_rm = STRBUF_INIT;
+   const char *format;
+
+   /*
+* protect submodules containing a .git directory
+* NEEDSWORK: instead of dying, automatically call
+* absorbgitdirs and (possibly) warn.
+*/
+   if (is_directory(sub_git_dir))
+   die(_("Submodule work tree '%s' contains a .git "
+ "directory (use 'rm -rf' if you really want "
+ "to remove it including all of its history)"),
+   displaypath);
+
+   if (!(flags & OPT_FORCE)) {
+   struct child_process cp_rm = CHILD_PROCESS_INIT;
+   cp_rm.git_cmd = 1;
+   argv_array_pushl(_rm.args, "rm", "-qn",
+path, NULL);
+
+   if (run_command(_rm))
+   die(_("Submodule work tree '%s' contains local "
+ "modifications; use '-f' to discard 
them"),
+ displaypath);
+   }
+
+   strbuf_addstr(_rm, path);
+
+   if (!remove_dir_recursively(_rm, 0))
+   format = _("Cleared directory '%s'\n");
+   else
+   format = _("Could not remove submodule work tree 
'%s'\n");
+
+   if (!(flags & OPT_QUIET))
+   printf(format, displaypath);
+
+   strbuf_release(_rm);
+   }
+
+   if (mkdir(path, 0777))
+   printf(_("could not create empty submodule directory %s"),
+ displaypath);
+
+   cp_config.git_cmd = 1;
+   argv_array_pushl(_config.args, "config", "--get-regexp", NULL);
+   argv_array_pushf(_config.args, "submodule.%s\\.", sub->name);
+
+   /* remove the .git/config entries (unless the user already did it) */
+   if (!capture_command(_config, _config, 0) && sb_config.len) {
+   char *sub_key = xstrfmt("submodule.%s", sub->name);
+   /*
+* remove the whole section so we have a clean state when
+* the user later decides to init this submodule again
+*/
+   git_config_rename_section_in_file(NULL, sub_key, NULL);
+   if (!(flags & OPT_QUIET))
+   printf(_("Submodule '%s' (%s) unregistered for path 
'%s'\n"),
+sub->name, sub->url, displaypath);
+   free(sub_key);
+   }
+
+cleanup:
+   free(displaypath);
+   free(sub_git_dir);
+   strbuf_release(_config);
+}
+
+static void deinit_submodule_cb(const struct cache_entry *list_item,
+   void *cb_data)
+{
+   struct deinit_cb *info = cb_data;
+   deinit_submodule(list_item->name, info->prefix, info->flags);
+}
+
+static int module_deinit(int argc, const char **argv, const char *prefix)

[PATCH v3 0/2] Incremental rewrite of git-submodules

[Prathamesh Chavan]

Changes in v3:

* For the variables: super_config_url and sub_origin_url, xstrdup() was used
  while assigning "" to them, before freeing.

* In case of the function deinit_submodule, since the orignal code doesn't die
  upon failure of the function mkdir(), printf was used instead of die_errno.

As before you can find this series at:
https://github.com/pratham-pc/git/commits/patch-series-2

And its build report is available at:
https://travis-ci.org/pratham-pc/git/builds/
Branch: patch-series-2
Build #197

Prathamesh Chavan (2):
  submodule: port submodule subcommand 'sync' from shell to C
  submodule: port submodule subcommand 'deinit' from shell to C

 builtin/submodule--helper.c | 340 
 git-submodule.sh| 112 +--
 2 files changed, 342 insertions(+), 110 deletions(-)

-- 
2.15.1

[PATCH] Remoted unnecessary void* from hashmap.h that caused compile warnings

[randall . s . becker]

From: "Randall S. Becker" 

* The while loop in the inline method hashmap_enable_item_counting
  used an unneeded variable. The loop has been revised accordingly.

Signed-off-by: Randall S. Becker 
---
 hashmap.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hashmap.h b/hashmap.h
index 7ce79f3..d375d9c 100644
--- a/hashmap.h
+++ b/hashmap.h
@@ -400,7 +400,6 @@ static inline void hashmap_disable_item_counting(struct 
hashmap *map)
  */
 static inline void hashmap_enable_item_counting(struct hashmap *map)
 {
-   void *item;
unsigned int n = 0;
struct hashmap_iter iter;
 
@@ -408,7 +407,7 @@ static inline void hashmap_enable_item_counting(struct 
hashmap *map)
return;
 
hashmap_iter_init(map, );
-   while ((item = hashmap_iter_next()))
+   while (hashmap_iter_next())
n++;
 
map->do_count_items = 1;
-- 
2.8.5.23.g6fa7ec3

Re: [PATCH v3 4/3] read-cache: don't try to write index if we can't write shared index

[Thomas Gummerer]

On 01/14, Duy Nguyen wrote:
> On Sun, Jan 14, 2018 at 5:37 AM, Thomas Gummerer  wrote:
> > In a0a967568e ("update-index --split-index: do not split if $GIT_DIR is
> > read only", 2014-06-13), we tried to make sure we can still write an
> > index, even if the shared index can not be written.
> >
> > We did so by just calling 'do_write_locked_index()' from
> > 'write_shared_index()'.  'do_write_locked_index()' always at least
> > closes the tempfile nowadays, and used to close or commit the lockfile
> > if COMMIT_LOCK or CLOSE_LOCK were given at the time this feature was
> > introduced.  COMMIT_LOCK or CLOSE_LOCK is passed in by most callers of
> > 'write_locked_index()'.
> >
> > After calling 'write_shared_index()', we call 'write_split_index()',
> > which calls 'do_write_locked_index()' again, which then tries to use the
> > closed lockfile again, but in fact fails to do so as it's already
> > closed.
> >
> > In the current version, git will in fact segfault if it can't create a
> > new file in $gitdir, and this feature seems to never have worked in the
> > first place.
> >
> > Ever since introducing the split index feature, nobody has complained
> > about this failing, and it really just papers over repositories that
> > will sooner or later need fixing anyway.
> 
> Actually there's one valid case for this: you're accessing a read-only
> $GIT_DIR (.e.g maybe from a web server cgi script which may be run by
> user nobody or something) and creating a temporary index _outside_
> $GIT_DIR. I used to do this when I wanted to do "git grep" on some
> SHA-1 a couple times. Doing "git grep " directly (a couple
> times) pays full cost for walking trees. If you prepare an index
> first, you pay it only once.

Makes sense, I didn't realize that usecase, thanks!

> > Therefore just make being unable to write the split index a proper
> > error, and have users fix their repositories instead of trying (but
> > failing) to paper over the error.
> >
> > Signed-off-by: Thomas Gummerer 
> > ---
> >  read-cache.c | 11 ---
> >  1 file changed, 4 insertions(+), 7 deletions(-)
> >
> > diff --git a/read-cache.c b/read-cache.c
> > index d13ce83794..a9c8facdfd 100644
> > --- a/read-cache.c
> > +++ b/read-cache.c
> > @@ -2471,18 +2471,15 @@ static int clean_shared_index_files(const char 
> > *current_hex)
> > return 0;
> >  }
> >
> > -static int write_shared_index(struct index_state *istate,
> > - struct lock_file *lock, unsigned flags)
> > +static int write_shared_index(struct index_state *istate)
> >  {
> > struct tempfile *temp;
> > struct split_index *si = istate->split_index;
> > int ret;
> >
> > temp = mks_tempfile(git_path("sharedindex_XX"));
> > -   if (!temp) {
> > -   hashclr(si->base_sha1);
> > -   return do_write_locked_index(istate, lock, flags);
> 
> I think this code tries to do what's done near the beginning of
> write_locked_index() where we also bail out early:
> 
> -- 8< --
> if (!si || alternate_index_output ||
> (istate->cache_changed & ~EXTMASK)) {
> if (si)
> hashclr(si->base_sha1);
> ret = do_write_locked_index(istate, lock, flags);
> goto out;
> }
> -- 8< --
> 
> the only difference is it does not realize that it can't do "goto
> out;" like that code unless something goes wrong. I'll try to prepare
> a patch that move tempfile creation out of write_shared_index()
> instead. Patches coming in a bit..

Thanks for fixing this in a nicer way :)

> -- 
> Duy

Re: [PATCH] travis-ci: build Git during the 'script' phase

[Jeff King]

On Sun, Jan 14, 2018 at 11:43:05AM +0100, SZEDER Gábor wrote:

> On Sat, Jan 13, 2018 at 11:54 AM, Jeff King  wrote:
> > I think there's also a similar feature to include timings for each fold,
> > which might be worth pursuing.
> 
> If you look for 'travis_time' in the raw log, you'll find lines like
> these:
> 
>   travis_time:start:01ccbe40
>   $ some-command
>   ... and its output ...
>   
> travis_time:end:01ccbe40:start=1515840453305552968,finish=1515840471960386859,duration=18654833891
> 
> So it seems doable, but we'll have to do the timekeeping ourselves.
> Running 'time $cmd' is much easier, but that time won't be displayed
> next to the folds, of course.
> Do we really care that much?

I don't care that much (and I wasn't actually planning to push the fold
stuff into a patch, but would instead leave it to you people who were
already working on improving the ci script output ;) ).

Apparently there are exportable bash functions for all of this:

  http://www.garbers.co.za/2017/11/01/code-folding-and-timing-in-travis-ci/

but they're not part of the official API. So relying on them may be even
more questionable than relying on the travis_fold syntax.

-Peff

Re: [PATCH] travis-ci: build Git during the 'script' phase

[Jeff King]

On Sun, Jan 14, 2018 at 11:37:07AM +0100, SZEDER Gábor wrote:

> > +fold_cmd () {
> > +   local name=$1; shift
> > +   fold "$name"
> > +   "$@"
> > +   local ret=$?
> > +   unfold "$name"
> > +   return $ret
> > +}
> 
> We don't have to fiddle with the return value, because we run (almost
> all of) our build scripts with 'set -e', i.e. if the command fails then
> the script will exit immediately.

Yeah, that's probably enough for our simple scripts, though I have been
bit by "set -e" vagaries before (e.g., calling the function inside a
conditional block).

-Peff

Re: [PATCH] travis-ci: build Git during the 'script' phase

[SZEDER Gábor]

On Sat, Jan 13, 2018 at 11:54 AM, Jeff King  wrote:
> I think there's also a similar feature to include timings for each fold,
> which might be worth pursuing.

If you look for 'travis_time' in the raw log, you'll find lines like
these:

  travis_time:start:01ccbe40
  $ some-command
  ... and its output ...
  
travis_time:end:01ccbe40:start=1515840453305552968,finish=1515840471960386859,duration=18654833891

So it seems doable, but we'll have to do the timekeeping ourselves.
Running 'time $cmd' is much easier, but that time won't be displayed
next to the folds, of course.
Do we really care that much?


Gábor

Re: [PATCH] travis-ci: build Git during the 'script' phase

[SZEDER Gábor]

On Sat, Jan 13, 2018 at 11:32 AM, Jeff King  wrote:
> On Fri, Jan 12, 2018 at 02:32:54PM +0100, SZEDER Gábor wrote:
>
>> That's the just beginning of a looong list of executed test scripts in
>> seemingly pseudo-random order.  IMHO that's very rarely the interesting
>> part; I, for one, am only interested in that list in exceptional cases,
>> e.g. while tweaking the build dependencies or the 'prove --state=...'
>> options.
>
> Aren't folds supposed to do this? I.e., record all the output but only
> show it to the user if the command exited non-zero.
>
> According to:
>
>   https://blog.travis-ci.com/2013-05-22-improving-build-visibility-log-folds
>
> they auto-fold individual commands _except_ the ones in the script
> section. Is there a way to manually mark folds in the output?
>
> Hmph. I could not find an answer from travis, but googling seems to turn
> up that something like this would work:

Oh.  I did look for something like this in the Travis CI docs, found
nothing and then didn't bother with Google.  Rookie mistake, I know :)

But indeed, have a look at the raw trace log at:

  https://api.travis-ci.org/v3/job/328418291/log.txt

It starts with that "travis_fold:start:..." thing right away.

> diff --git a/ci/lib-travisci.sh b/ci/lib-travisci.sh
> index 07f27c7270..8c830aa3c0 100755
> --- a/ci/lib-travisci.sh
> +++ b/ci/lib-travisci.sh
> @@ -77,6 +77,23 @@ check_unignored_build_artifacts ()
> }
>  }
>
> +fold () {
> +   printf 'travis_fold:start:%s\r' "$1"
> +}
> +
> +unfold () {
> +   printf 'travis_fold:end:%s\r' "$1"
> +}
> +
> +fold_cmd () {
> +   local name=$1; shift
> +   fold "$name"
> +   "$@"
> +   local ret=$?
> +   unfold "$name"
> +   return $ret
> +}

We don't have to fiddle with the return value, because we run (almost
all of) our build scripts with 'set -e', i.e. if the command fails then
the script will exit immediately.

> +
>  # Set 'exit on error' for all CI scripts to let the caller know that
>  # something went wrong.
>  # Set tracing executed commands, primarily setting environment variables
> diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
> index d3a094603f..12b2590230 100755
> --- a/ci/run-build-and-tests.sh
> +++ b/ci/run-build-and-tests.sh
> @@ -7,8 +7,8 @@
>
>  ln -s $HOME/travis-cache/.prove t/.prove
>
> -make --jobs=2
> -make --quiet test
> +fold_cmd compile make --jobs=2
> +fold_cmd test make --quiet test
>
>  check_unignored_build_artifacts
>
>
> I've queued a CI job to see if this actually works. :)
>
> -Peff

Re: [PATCH/RFC] diff: add --compact-summary option to complement --stat

[Duy Nguyen]

On Sun, Jan 14, 2018 at 4:37 PM, Simon Ruderich  wrote:
> On Sat, Jan 13, 2018 at 08:22:11PM +0700, Nguyễn Thái Ngọc Duy wrote:
>> [snip]
>>
>> For mode changes, executable bit is denoted as "(+x)" or "(-x)" when
>> it's added or removed respectively. The same for when a regular file is
>> replaced with a symlink "(+l)" or the other way "(-l)". This also
>> applies to new files. New regulare files are "A", while new executable
>> files or symlinks are "A+x" or "A+l".
>
> I like the short summary. However I find the use of parentheses
> inconsistent.

I agree. I put them in parentheses because somehow to me plain "+x"
looks weird to me.

> Why not use them either always (also for "(A+l)")
> or never? Was there a specific reason why you added them just in
> one place?

Actually shortly after I sent the mail, I realized I could do better.
Since this is a mode _modification_, we could denote it with "M" (most
files in diffstat are "M" for obvious reasons, we just don't print it
because it adds no value), so here we could print "M+x" or "M-x". This
aligns well with "A+l" or "A+x" for example and is one character
shorter than my old way.
-- 
Duy

[PATCH 2/3] read-cache.c: move tempfile creation/cleanup out of write_shared_index

[Nguyễn Thái Ngọc Duy]

For one thing, we have more consistent cleanup procedure now and always
keep errno intact.

The real purpose is the ability to break out of write_locked_index()
early when mks_tempfile() fails in the next patch. It's more awkward to
do it if this mks_tempfile() is still inside write_shared_index().

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 read-cache.c | 33 +
 1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index 536086e1fe..c568643f55 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2472,31 +2472,18 @@ static int clean_shared_index_files(const char 
*current_hex)
 }
 
 static int write_shared_index(struct index_state *istate,
- struct lock_file *lock, unsigned flags)
+ struct tempfile **temp)
 {
-   struct tempfile *real_temp;
-   struct tempfile **temp = _temp;
struct split_index *si = istate->split_index;
int ret;
 
-   real_temp = mks_tempfile(git_path("sharedindex_XX"));
-   if (!real_temp) {
-   hashclr(si->base_sha1);
-   return do_write_locked_index(istate, lock, flags);
-   }
-   temp = _temp;
move_cache_to_base_index(istate);
ret = do_write_index(si->base, *temp, 1);
-   if (ret) {
-   delete_tempfile(temp);
+   if (ret)
return ret;
-   }
ret = adjust_shared_perm(get_tempfile_path(*temp));
if (ret) {
-   int save_errno = errno;
error("cannot fix permission bits on %s", 
get_tempfile_path(*temp));
-   delete_tempfile(temp);
-   errno = save_errno;
return ret;
}
ret = rename_tempfile(temp,
@@ -2567,7 +2554,21 @@ int write_locked_index(struct index_state *istate, 
struct lock_file *lock,
new_shared_index = istate->cache_changed & SPLIT_INDEX_ORDERED;
 
if (new_shared_index) {
-   ret = write_shared_index(istate, lock, flags);
+   struct tempfile *temp;
+   int saved_errno;
+
+   temp = mks_tempfile(git_path("sharedindex_XX"));
+   if (!temp) {
+   hashclr(si->base_sha1);
+   ret = do_write_locked_index(istate, lock, flags);
+   } else
+   ret = write_shared_index(istate, );
+
+   saved_errno = errno;
+   if (is_tempfile_active(temp))
+   delete_tempfile();
+   errno = saved_errno;
+
if (ret)
goto out;
}
-- 
2.15.1.600.g899a5f85c6

[PATCH 1/3] read-cache.c: change type of "temp" in write_shared_index()

[Nguyễn Thái Ngọc Duy]

This local variable 'temp' will be passed in from the caller in the next
patch. To reduce patch noise, let's change its type now while it's still
a local variable and get all the trival conversion out of the next patch.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 read-cache.c | 20 +++-
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index 2eb81a66b9..536086e1fe 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2474,30 +2474,32 @@ static int clean_shared_index_files(const char 
*current_hex)
 static int write_shared_index(struct index_state *istate,
  struct lock_file *lock, unsigned flags)
 {
-   struct tempfile *temp;
+   struct tempfile *real_temp;
+   struct tempfile **temp = _temp;
struct split_index *si = istate->split_index;
int ret;
 
-   temp = mks_tempfile(git_path("sharedindex_XX"));
-   if (!temp) {
+   real_temp = mks_tempfile(git_path("sharedindex_XX"));
+   if (!real_temp) {
hashclr(si->base_sha1);
return do_write_locked_index(istate, lock, flags);
}
+   temp = _temp;
move_cache_to_base_index(istate);
-   ret = do_write_index(si->base, temp, 1);
+   ret = do_write_index(si->base, *temp, 1);
if (ret) {
-   delete_tempfile();
+   delete_tempfile(temp);
return ret;
}
-   ret = adjust_shared_perm(get_tempfile_path(temp));
+   ret = adjust_shared_perm(get_tempfile_path(*temp));
if (ret) {
int save_errno = errno;
-   error("cannot fix permission bits on %s", 
get_tempfile_path(temp));
-   delete_tempfile();
+   error("cannot fix permission bits on %s", 
get_tempfile_path(*temp));
+   delete_tempfile(temp);
errno = save_errno;
return ret;
}
-   ret = rename_tempfile(,
+   ret = rename_tempfile(temp,
  git_path("sharedindex.%s", 
sha1_to_hex(si->base->sha1)));
if (!ret) {
hashcpy(si->base_sha1, si->base->sha1);
-- 
2.15.1.600.g899a5f85c6

[PATCH 3/3] read-cache: don't write index twice if we can't write shared index

[Nguyễn Thái Ngọc Duy]

In a0a967568e ("update-index --split-index: do not split if $GIT_DIR is
read only", 2014-06-13), we tried to make sure we can still write an
index, even if the shared index can not be written.

We did so by just calling 'do_write_locked_index()' just before
'write_shared_index()'.  'do_write_locked_index()' always at least
closes the tempfile nowadays, and used to close or commit the lockfile
if COMMIT_LOCK or CLOSE_LOCK were given at the time this feature was
introduced.  COMMIT_LOCK or CLOSE_LOCK is passed in by most callers of
'write_locked_index()'.

After calling 'write_shared_index()', we call 'write_split_index()',
which calls 'do_write_locked_index()' again, which then tries to use the
closed lockfile again, but in fact fails to do so as it's already
closed. This eventually leads to a segfault.

Make sure to write the main index only once.

[nd: most of the commit message and investigation done by Thomas, I only
tweaked the solution a bit]

Helped-by: Thomas Gummerer 
Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 read-cache.c   |  5 +++--
 t/t1700-split-index.sh | 19 +++
 2 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index c568643f55..c58c0a978a 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2561,8 +2561,9 @@ int write_locked_index(struct index_state *istate, struct 
lock_file *lock,
if (!temp) {
hashclr(si->base_sha1);
ret = do_write_locked_index(istate, lock, flags);
-   } else
-   ret = write_shared_index(istate, );
+   goto out;
+   }
+   ret = write_shared_index(istate, );
 
saved_errno = errno;
if (is_tempfile_active(temp))
diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
index af9b847761..5494389dbb 100755
--- a/t/t1700-split-index.sh
+++ b/t/t1700-split-index.sh
@@ -401,4 +401,23 @@ done <<\EOF
 0642 -rw-r---w-
 EOF
 
+test_expect_success POSIXPERM 'graceful handling splitting index is not 
allowed' '
+   test_create_repo ro &&
+   (
+   cd ro &&
+   test_commit initial &&
+   git update-index --split-index &&
+   test -f .git/sharedindex.*
+   ) &&
+   test_when_finished "chmod -R u+w ro" &&
+   chmod -R u-w ro &&
+   cp ro/.git/index new-index &&
+   GIT_INDEX_FILE="$(pwd)/new-index" git -C ro update-index --split-index 
&&
+   chmod -R u+w ro &&
+   rm ro/.git/sharedindex.* &&
+   GIT_INDEX_FILE=new-index git ls-files >actual &&
+   echo initial.t >expected &&
+   test_cmp expected actual
+'
+
 test_done
-- 
2.15.1.600.g899a5f85c6

Re: [PATCH/RFC] diff: add --compact-summary option to complement --stat

[Simon Ruderich]

On Sat, Jan 13, 2018 at 08:22:11PM +0700, Nguyễn Thái Ngọc Duy wrote:
> [snip]
>
> For mode changes, executable bit is denoted as "(+x)" or "(-x)" when
> it's added or removed respectively. The same for when a regular file is
> replaced with a symlink "(+l)" or the other way "(-l)". This also
> applies to new files. New regulare files are "A", while new executable
> files or symlinks are "A+x" or "A+l".

I like the short summary. However I find the use of parentheses
inconsistent. Why not use them either always (also for "(A+l)")
or never? Was there a specific reason why you added them just in
one place?

Regards
Simon
-- 
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9

Re: [PATCH v3 4/3] read-cache: don't try to write index if we can't write shared index

[Duy Nguyen]

On Sun, Jan 14, 2018 at 5:37 AM, Thomas Gummerer  wrote:
> In a0a967568e ("update-index --split-index: do not split if $GIT_DIR is
> read only", 2014-06-13), we tried to make sure we can still write an
> index, even if the shared index can not be written.
>
> We did so by just calling 'do_write_locked_index()' from
> 'write_shared_index()'.  'do_write_locked_index()' always at least
> closes the tempfile nowadays, and used to close or commit the lockfile
> if COMMIT_LOCK or CLOSE_LOCK were given at the time this feature was
> introduced.  COMMIT_LOCK or CLOSE_LOCK is passed in by most callers of
> 'write_locked_index()'.
>
> After calling 'write_shared_index()', we call 'write_split_index()',
> which calls 'do_write_locked_index()' again, which then tries to use the
> closed lockfile again, but in fact fails to do so as it's already
> closed.
>
> In the current version, git will in fact segfault if it can't create a
> new file in $gitdir, and this feature seems to never have worked in the
> first place.
>
> Ever since introducing the split index feature, nobody has complained
> about this failing, and it really just papers over repositories that
> will sooner or later need fixing anyway.

Actually there's one valid case for this: you're accessing a read-only
$GIT_DIR (.e.g maybe from a web server cgi script which may be run by
user nobody or something) and creating a temporary index _outside_
$GIT_DIR. I used to do this when I wanted to do "git grep" on some
SHA-1 a couple times. Doing "git grep " directly (a couple
times) pays full cost for walking trees. If you prepare an index
first, you pay it only once.

> Therefore just make being unable to write the split index a proper
> error, and have users fix their repositories instead of trying (but
> failing) to paper over the error.
>
> Signed-off-by: Thomas Gummerer 
> ---
>  read-cache.c | 11 ---
>  1 file changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/read-cache.c b/read-cache.c
> index d13ce83794..a9c8facdfd 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -2471,18 +2471,15 @@ static int clean_shared_index_files(const char 
> *current_hex)
> return 0;
>  }
>
> -static int write_shared_index(struct index_state *istate,
> - struct lock_file *lock, unsigned flags)
> +static int write_shared_index(struct index_state *istate)
>  {
> struct tempfile *temp;
> struct split_index *si = istate->split_index;
> int ret;
>
> temp = mks_tempfile(git_path("sharedindex_XX"));
> -   if (!temp) {
> -   hashclr(si->base_sha1);
> -   return do_write_locked_index(istate, lock, flags);

I think this code tries to do what's done near the beginning of
write_locked_index() where we also bail out early:

-- 8< --
if (!si || alternate_index_output ||
(istate->cache_changed & ~EXTMASK)) {
if (si)
hashclr(si->base_sha1);
ret = do_write_locked_index(istate, lock, flags);
goto out;
}
-- 8< --

the only difference is it does not realize that it can't do "goto
out;" like that code unless something goes wrong. I'll try to prepare
a patch that move tempfile creation out of write_shared_index()
instead. Patches coming in a bit..
-- 
Duy