[PATCH v3 05/11] update-index: move 'uc' var declaration

[Christian Couder]

Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
---
 builtin/update-index.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/builtin/update-index.c b/builtin/update-index.c
index c91e695..f667125 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -1116,8 +1116,6 @@ int cmd_update_index(int argc, const char **argv, const 
char *prefix)
the_index.cache_changed |= SOMETHING_CHANGED;
}
if (untracked_cache > UC_DISABLE) {
-   struct untracked_cache *uc;
-
if (untracked_cache < UC_FORCE) {
setup_work_tree();
if (!test_if_untracked_cache_is_supported())
@@ -1126,7 +1124,7 @@ int cmd_update_index(int argc, const char **argv, const 
char *prefix)
return 0;
}
if (!the_index.untracked) {
-   uc = xcalloc(1, sizeof(*uc));
+   struct untracked_cache *uc = xcalloc(1, sizeof(*uc));
strbuf_init(>ident, 100);
uc->exclude_per_dir = ".gitignore";
/* should be the same flags used by git-status */
-- 
2.7.0.rc2.11.g68ccdd4

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 10/11] config: add core.untrackedCache

[Christian Couder]

When we know that mtime is fully supported by the environment, we
might want the untracked cache to be always used by default without
any mtime test or kernel version check being performed.

Also when we know that mtime is not supported by the environment,
for example because the repo is shared over a network file system,
then we might want 'git update-index --untracked-cache' to fail
immediately instead of performing tests (because it might work on
some systems using the repo over the network file system but not
others).

The normal way to achieve the above in Git is to use a config
variable. That's why this patch introduces "core.untrackedCache".

To keep things simple, this variable is a bool which default to
false.

When "git status" is run, it now adds or removes the untracked
cache in the index to respect the value of this variable.

The job of `git update-index --[no-|force-]untracked-cache` was
to add or remove the untracked cache from the index. This was a
kind of configuration because this was persistant across git
commands. To make this kind of configuration compatible with the
new config variable, the simple thing to do, and what this patch
does, is to make `git update-index --[no-|force-]untracked-cache`
set or unset this config option.

This new behavior is a backward incompatible change, but that is
deliberate. The untracked cache feature has been experimental
and is very unlikely used by beginners.

When people will upgrade, this will remove any untracked cache
they used unless they set "core.untrackedCache" before upgrading.
This should be stated in the release notes.

Also `--untracked-cache` used to check that the underlying
operating system and file system change `st_mtime` field of a
directory if files are added or deleted in that directory. But
those tests take a long time and there is now
`--test-untracked-cache` to perform them.

That's why, to be more consistent with other git commands, this
patch prevents `--untracked-cache` to perform tests, so that
after this patch there is no difference any more between
`--untracked-cache` and `--force-untracked-cache`.

All the changes to `--[no-|force-]untracked-cache` make it
possible to deprecate those options in the future.

Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
Signed-off-by: Ævar Arnfjörð Bjarmason <ava...@gmail.com>
---
 Documentation/config.txt   |  7 
 Documentation/git-update-index.txt | 58 +++---
 builtin/update-index.c | 15 +
 cache.h|  1 +
 config.c   |  4 +++
 contrib/completion/git-completion.bash |  1 +
 environment.c  |  1 +
 t/t7063-status-untracked-cache.sh  |  4 +--
 wt-status.c|  5 +++
 9 files changed, 74 insertions(+), 22 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index f617886..08f0510 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -308,6 +308,13 @@ core.trustctime::
crawlers and some backup systems).
See linkgit:git-update-index[1]. True by default.
 
+core.untrackedCache::
+   Determines if untracked cache will be enabled. Using
+   'git update-index --[no-|force-]untracked-cache' will set
+   this variable. Before setting it to true, you should check
+   that mtime is working properly on your system.
+   See linkgit:git-update-index[1]. False by default.
+
 core.checkStat::
Determines which stat fields to match between the index
and work tree. The user can set this to 'default' or
diff --git a/Documentation/git-update-index.txt 
b/Documentation/git-update-index.txt
index a0ee0c9..bab6394 100644
--- a/Documentation/git-update-index.txt
+++ b/Documentation/git-update-index.txt
@@ -174,24 +174,29 @@ may not support it yet.
 
 --untracked-cache::
 --no-untracked-cache::
-   Enable or disable untracked cache extension. This could speed
-   up for commands that involve determining untracked files such
-   as `git status`. The underlying operating system and file
-   system must change `st_mtime` field of a directory if files
-   are added or deleted in that directory.
+   Enable or disable untracked cache extension. Please use
+   `--test-untracked-cache` before enabling it.
++
+These options are mostly aliases for setting the `core.untrackedCache`
+configuration variable to 'true' or 'false' in the local config file
+(see linkgit:git-config[1]). You can equivalently just set those
+configuration values directly. These options are just provided for
+backwards compatibility with the older versions of Git where this was
+the only way to enable or disable the untracked cache extension.
 
 --test-untracked-cache::
Only perform tests on the working directory to make sure
untracked cache can be used. You have to manually ena

[PATCH v3 09/11] dir: simplify untracked cache "ident" field

[Christian Couder]

It is not a good idea to compare kernel versions and disable
the untracked cache if it changes as people may upgrade and
still want the untracked cache to work. So let's just
compare work tree locations to decide if we should disable
it.

Also it's not useful to store many locations in the ident
field and compare to any of them. It can even be dangerous
if GIT_WORK_TREE is used with different values. So let's
just store one location, the location of the current work
tree.

If this location changed and we still want an untracked
cache, let's delete the cache and recreate it.

Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
---
 dir.c | 49 ++---
 1 file changed, 34 insertions(+), 15 deletions(-)

diff --git a/dir.c b/dir.c
index dba1ad0..7596a3f 100644
--- a/dir.c
+++ b/dir.c
@@ -1918,23 +1918,36 @@ static const char *get_ident_string(void)
return sb.buf;
 }
 
-static int ident_in_untracked(const struct untracked_cache *uc)
+static int ident_current_location_in_untracked(const struct untracked_cache 
*uc)
 {
-   const char *end = uc->ident.buf + uc->ident.len;
-   const char *p   = uc->ident.buf;
+   struct strbuf cur_loc = STRBUF_INIT;
+   int res = 0;
 
-   for (p = uc->ident.buf; p < end; p += strlen(p) + 1)
-   if (!strcmp(p, get_ident_string()))
-   return 1;
-   return 0;
+   /*
+* Previous git versions may have saved many strings in the
+* "ident" field, but it is insane to manage many locations,
+* so just take care of the first one.
+*/
+
+   strbuf_addf(_loc, "Location %s, system ", get_git_work_tree());
+
+   if (starts_with(uc->ident.buf, cur_loc.buf))
+   res = 1;
+
+   strbuf_release(_loc);
+
+   return res;
 }
 
-void add_untracked_ident(struct untracked_cache *uc)
+static void set_untracked_ident(struct untracked_cache *uc)
 {
-   if (ident_in_untracked(uc))
-   return;
+   strbuf_reset(>ident);
strbuf_addstr(>ident, get_ident_string());
-   /* this strbuf contains a list of strings, save NUL too */
+
+   /*
+* This strbuf used to contain a list of NUL separated
+* strings, so save NUL too for backward compatibility.
+*/
strbuf_addch(>ident, 0);
 }
 
@@ -1945,15 +1958,21 @@ static void new_untracked_cache(void)
uc->exclude_per_dir = ".gitignore";
/* should be the same flags used by git-status */
uc->dir_flags = DIR_SHOW_OTHER_DIRECTORIES | DIR_HIDE_EMPTY_DIRECTORIES;
+   set_untracked_ident(the_index.untracked);
the_index.untracked = uc;
+   the_index.cache_changed |= UNTRACKED_CHANGED;
 }
 
 void add_untracked_cache(void)
 {
-   if (!the_index.untracked)
+   if (!the_index.untracked) {
new_untracked_cache();
-   add_untracked_ident(the_index.untracked);
-   the_index.cache_changed |= UNTRACKED_CHANGED;
+   } else {
+   if (!ident_current_location_in_untracked(the_index.untracked)) {
+   free_untracked_cache(the_index.untracked);
+   new_untracked_cache();
+   }
+   }
 }
 
 void remove_untracked_cache(void)
@@ -2021,7 +2040,7 @@ static struct untracked_cache_dir 
*validate_untracked_cache(struct dir_struct *d
if (dir->exclude_list_group[EXC_CMDL].nr)
return NULL;
 
-   if (!ident_in_untracked(dir->untracked)) {
+   if (!ident_current_location_in_untracked(dir->untracked)) {
warning(_("Untracked cache is disabled on this system."));
return NULL;
}
-- 
2.7.0.rc2.11.g68ccdd4

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 06/11] dir: add add_untracked_cache()

[Christian Couder]

Factor out code into add_untracked_cache(), which will be used
in a later commit.

Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
---
 builtin/update-index.c | 11 +--
 dir.c  | 14 ++
 dir.h  |  1 +
 3 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/builtin/update-index.c b/builtin/update-index.c
index f667125..093725a 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -1123,16 +1123,7 @@ int cmd_update_index(int argc, const char **argv, const 
char *prefix)
if (untracked_cache == UC_TEST)
return 0;
}
-   if (!the_index.untracked) {
-   struct untracked_cache *uc = xcalloc(1, sizeof(*uc));
-   strbuf_init(>ident, 100);
-   uc->exclude_per_dir = ".gitignore";
-   /* should be the same flags used by git-status */
-   uc->dir_flags = DIR_SHOW_OTHER_DIRECTORIES | 
DIR_HIDE_EMPTY_DIRECTORIES;
-   the_index.untracked = uc;
-   }
-   add_untracked_ident(the_index.untracked);
-   the_index.cache_changed |= UNTRACKED_CHANGED;
+   add_untracked_cache();
if (verbose)
printf(_("Untracked cache enabled for '%s'\n"), 
get_git_work_tree());
} else if (untracked_cache == UC_DISABLE) {
diff --git a/dir.c b/dir.c
index d2a8f06..0f7e293 100644
--- a/dir.c
+++ b/dir.c
@@ -1938,6 +1938,20 @@ void add_untracked_ident(struct untracked_cache *uc)
strbuf_addch(>ident, 0);
 }
 
+void add_untracked_cache(void)
+{
+   if (!the_index.untracked) {
+   struct untracked_cache *uc = xcalloc(1, sizeof(*uc));
+   strbuf_init(>ident, 100);
+   uc->exclude_per_dir = ".gitignore";
+   /* should be the same flags used by git-status */
+   uc->dir_flags = DIR_SHOW_OTHER_DIRECTORIES | 
DIR_HIDE_EMPTY_DIRECTORIES;
+   the_index.untracked = uc;
+   }
+   add_untracked_ident(the_index.untracked);
+   the_index.cache_changed |= UNTRACKED_CHANGED;
+}
+
 static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct 
*dir,
  int base_len,
  const struct pathspec 
*pathspec)
diff --git a/dir.h b/dir.h
index 7b5855d..ee94c76 100644
--- a/dir.h
+++ b/dir.h
@@ -308,4 +308,5 @@ void free_untracked_cache(struct untracked_cache *);
 struct untracked_cache *read_untracked_extension(const void *data, unsigned 
long sz);
 void write_untracked_extension(struct strbuf *out, struct untracked_cache 
*untracked);
 void add_untracked_ident(struct untracked_cache *);
+void add_untracked_cache(void);
 #endif
-- 
2.7.0.rc2.11.g68ccdd4

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 07/11] dir: add new_untracked_cache()

[Christian Couder]

Factor out code into new_untracked_cache(), which will be used
multiple times in a later commit.

Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
---
 dir.c | 20 
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/dir.c b/dir.c
index 0f7e293..4227ba6 100644
--- a/dir.c
+++ b/dir.c
@@ -1938,16 +1938,20 @@ void add_untracked_ident(struct untracked_cache *uc)
strbuf_addch(>ident, 0);
 }
 
+static void new_untracked_cache(void)
+{
+   struct untracked_cache *uc = xcalloc(1, sizeof(*uc));
+   strbuf_init(>ident, 100);
+   uc->exclude_per_dir = ".gitignore";
+   /* should be the same flags used by git-status */
+   uc->dir_flags = DIR_SHOW_OTHER_DIRECTORIES | DIR_HIDE_EMPTY_DIRECTORIES;
+   the_index.untracked = uc;
+}
+
 void add_untracked_cache(void)
 {
-   if (!the_index.untracked) {
-   struct untracked_cache *uc = xcalloc(1, sizeof(*uc));
-   strbuf_init(>ident, 100);
-   uc->exclude_per_dir = ".gitignore";
-   /* should be the same flags used by git-status */
-   uc->dir_flags = DIR_SHOW_OTHER_DIRECTORIES | 
DIR_HIDE_EMPTY_DIRECTORIES;
-   the_index.untracked = uc;
-   }
+   if (!the_index.untracked)
+   new_untracked_cache();
add_untracked_ident(the_index.untracked);
the_index.cache_changed |= UNTRACKED_CHANGED;
 }
-- 
2.7.0.rc2.11.g68ccdd4

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 08/11] dir: add remove_untracked_cache()

[Christian Couder]

Factor out code into remove_untracked_cache(), which will be used
in a later commit.

Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
---
 builtin/update-index.c | 6 +-
 dir.c  | 9 +
 dir.h  | 1 +
 3 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/builtin/update-index.c b/builtin/update-index.c
index 093725a..3e5b4a6 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -1127,11 +1127,7 @@ int cmd_update_index(int argc, const char **argv, const 
char *prefix)
if (verbose)
printf(_("Untracked cache enabled for '%s'\n"), 
get_git_work_tree());
} else if (untracked_cache == UC_DISABLE) {
-   if (the_index.untracked) {
-   free_untracked_cache(the_index.untracked);
-   the_index.untracked = NULL;
-   the_index.cache_changed |= UNTRACKED_CHANGED;
-   }
+   remove_untracked_cache();
if (verbose)
printf(_("Untracked cache disabled\n"));
}
diff --git a/dir.c b/dir.c
index 4227ba6..dba1ad0 100644
--- a/dir.c
+++ b/dir.c
@@ -1956,6 +1956,15 @@ void add_untracked_cache(void)
the_index.cache_changed |= UNTRACKED_CHANGED;
 }
 
+void remove_untracked_cache(void)
+{
+   if (the_index.untracked) {
+   free_untracked_cache(the_index.untracked);
+   the_index.untracked = NULL;
+   the_index.cache_changed |= UNTRACKED_CHANGED;
+   }
+}
+
 static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct 
*dir,
  int base_len,
  const struct pathspec 
*pathspec)
diff --git a/dir.h b/dir.h
index ee94c76..3e5114d 100644
--- a/dir.h
+++ b/dir.h
@@ -309,4 +309,5 @@ struct untracked_cache *read_untracked_extension(const void 
*data, unsigned long
 void write_untracked_extension(struct strbuf *out, struct untracked_cache 
*untracked);
 void add_untracked_ident(struct untracked_cache *);
 void add_untracked_cache(void);
+void remove_untracked_cache(void);
 #endif
-- 
2.7.0.rc2.11.g68ccdd4

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 11/11] t7063: add tests for core.untrackedCache

[Christian Couder]

Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
---
 t/t7063-status-untracked-cache.sh | 48 +--
 1 file changed, 46 insertions(+), 2 deletions(-)

diff --git a/t/t7063-status-untracked-cache.sh 
b/t/t7063-status-untracked-cache.sh
index 253160a..f0af41c 100755
--- a/t/t7063-status-untracked-cache.sh
+++ b/t/t7063-status-untracked-cache.sh
@@ -18,6 +18,10 @@ if ! test_have_prereq UNTRACKED_CACHE; then
test_done
 fi
 
+test_expect_success 'core.untrackedCache is unset' '
+   test_must_fail git config --get core.untrackedCache
+'
+
 test_expect_success 'setup' '
git init worktree &&
cd worktree &&
@@ -28,6 +32,11 @@ test_expect_success 'setup' '
git update-index --untracked-cache
 '
 
+test_expect_success 'core.untrackedCache is true' '
+   UC=$(git config core.untrackedCache) &&
+   test "$UC" = "true"
+'
+
 test_expect_success 'untracked cache is empty' '
test-dump-untracked-cache >../actual &&
cat >../expect <../actual &&
-   cat >../expect <../expect-from-test-dump <../actual &&
+   echo "no untracked cache" >../expect &&
+   test_cmp ../expect ../actual
+'
+
+test_expect_success 'git status does not change anything' '
+   git status &&
+   test-dump-untracked-cache >../actual &&
+   test_cmp ../expect ../actual &&
+   UC=$(git config core.untrackedCache) &&
+   test "$UC" = "false"
+'
+
+test_expect_success 'setting core.untrackedCache and using git status creates 
the cache' '
+   git config core.untrackedCache true &&
+   test-dump-untracked-cache >../actual &&
+   test_cmp ../expect ../actual &&
+   git status &&
+   test-dump-untracked-cache >../actual &&
+   test_cmp ../expect-from-test-dump ../actual
+'
+
+test_expect_success 'unsetting core.untrackedCache and using git status 
removes the cache' '
+   git config --unset core.untrackedCache &&
+   test-dump-untracked-cache >../actual &&
+   test_cmp ../expect-from-test-dump ../actual &&
+   git status &&
+   test-dump-untracked-cache >../actual &&
+   test_cmp ../expect ../actual
+'
+
 test_done
-- 
2.7.0.rc2.11.g68ccdd4

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 04/11] update-index: add untracked cache notifications

[Christian Couder]

Attempting to flip the untracked-cache feature on for a random index
file with

cd /random/unrelated/place
git --git-dir=/somewhere/else/.git update-index --untracked-cache

would not work as you might expect. Because flipping the feature on
in the index also records the location of the corresponding working
tree (/random/unrelated/place in the above example), when the index
is subsequently used to keep track of files in the working tree in
/somewhere/else, the feature is disabled.

With this patch "git update-index --[test-]untracked-cache" tells the
user in which directory tests are performed. This makes it easy to
spot any problem.

Also in verbose mode, let's tell the user when the cache is enabled
or disabled.

Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
---
 builtin/update-index.c | 16 +++-
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/builtin/update-index.c b/builtin/update-index.c
index 6dd..c91e695 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -130,7 +130,7 @@ static int test_if_untracked_cache_is_supported(void)
if (!mkdtemp(mtime_dir.buf))
die_errno("Could not make temporary directory");
 
-   fprintf(stderr, _("Testing "));
+   fprintf(stderr, _("Testing mtime in '%s' "), xgetcwd());
atexit(remove_test_directory);
xstat_mtime_dir();
fill_stat_data(, );
@@ -1135,10 +1135,16 @@ int cmd_update_index(int argc, const char **argv, const 
char *prefix)
}
add_untracked_ident(the_index.untracked);
the_index.cache_changed |= UNTRACKED_CHANGED;
-   } else if (untracked_cache == UC_DISABLE && the_index.untracked) {
-   free_untracked_cache(the_index.untracked);
-   the_index.untracked = NULL;
-   the_index.cache_changed |= UNTRACKED_CHANGED;
+   if (verbose)
+   printf(_("Untracked cache enabled for '%s'\n"), 
get_git_work_tree());
+   } else if (untracked_cache == UC_DISABLE) {
+   if (the_index.untracked) {
+   free_untracked_cache(the_index.untracked);
+   the_index.untracked = NULL;
+   the_index.cache_changed |= UNTRACKED_CHANGED;
+   }
+   if (verbose)
+   printf(_("Untracked cache disabled\n"));
}
 
if (active_cache_changed) {
-- 
2.7.0.rc2.11.g68ccdd4

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 07/11] dir: add new_untracked_cache()

[Christian Couder]

On Wed, Dec 23, 2015 at 11:53 PM, Eric Sunshine <sunsh...@sunshineco.com> wrote:
> On Wed, Dec 23, 2015 at 4:03 PM, Christian Couder
> <christian.cou...@gmail.com> wrote:
>> Factor out code into new_untracked_cache(), which will be used
>> multiple times in a later commit.
>
> Odd indentation: s/^\s+//

Yeah, will fix.

>> Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
>> ---
>> diff --git a/dir.c b/dir.c
>> @@ -1938,16 +1938,20 @@ void add_untracked_ident(struct untracked_cache *uc)
>> strbuf_addch(>ident, 0);
>>  }
>>
>> +static void new_untracked_cache(void)
>> +{
>> +   struct untracked_cache *uc = xcalloc(1, sizeof(*uc));
>> +   strbuf_init(>ident, 100);
>> +   uc->exclude_per_dir = ".gitignore";
>> +   /* should be the same flags used by git-status */
>> +   uc->dir_flags = DIR_SHOW_OTHER_DIRECTORIES | 
>> DIR_HIDE_EMPTY_DIRECTORIES;
>> +   the_index.untracked = uc;
>> +}
>
> This and the previous patch both move the same code around. As a
> reviewer, I could easily see the two patches combined, and would not
> find the unified patch onerous to review.

Ok, I will squash them.

By the way it looks like I have overlooked some reviews by David and
Tosten from the previous round.
Sorry about that. I will take them into account in the next version.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 01/10] dir: free untracked cache when removing it

[Christian Couder]

Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
---
 builtin/update-index.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/update-index.c b/builtin/update-index.c
index 7431938..a6fff87 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -1123,6 +1123,7 @@ int cmd_update_index(int argc, const char **argv, const 
char *prefix)
add_untracked_ident(the_index.untracked);
the_index.cache_changed |= UNTRACKED_CHANGED;
} else if (!untracked_cache && the_index.untracked) {
+   free_untracked_cache(the_index.untracked);
the_index.untracked = NULL;
the_index.cache_changed |= UNTRACKED_CHANGED;
}
-- 
2.7.0.rc2.10.g544ad6b

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 10/10] t7063: add tests for core.untrackedCache

[Christian Couder]

Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
---
 t/t7063-status-untracked-cache.sh | 75 ---
 1 file changed, 71 insertions(+), 4 deletions(-)

diff --git a/t/t7063-status-untracked-cache.sh 
b/t/t7063-status-untracked-cache.sh
index 253160a..67e049e 100755
--- a/t/t7063-status-untracked-cache.sh
+++ b/t/t7063-status-untracked-cache.sh
@@ -18,6 +18,10 @@ if ! test_have_prereq UNTRACKED_CACHE; then
test_done
 fi
 
+test_expect_success 'core.untrackedCache is unset' '
+   test_must_fail git config --get core.untrackedCache
+'
+
 test_expect_success 'setup' '
git init worktree &&
cd worktree &&
@@ -30,13 +34,13 @@ test_expect_success 'setup' '
 
 test_expect_success 'untracked cache is empty' '
test-dump-untracked-cache >../actual &&
-   cat >../expect <../expect-empty <../status.expect <../actual &&
-   cat >../expect <../expect-from-test-dump <../actual &&
+   echo "no untracked cache" >../expect &&
+   test_cmp ../expect ../actual
+'
+
+test_expect_success 'git status does not change anything' '
+   git status &&
+   test-dump-untracked-cache >../actual &&
+   test_cmp ../expect ../actual
+'
+
+test_expect_success 'setting core.untrackedCache to true and using git status 
creates the cache' '
+   git config core.untrackedCache true &&
+   test-dump-untracked-cache >../actual &&
+   test_cmp ../expect ../actual &&
+   git status &&
+   test-dump-untracked-cache >../actual &&
+   test_cmp ../expect-from-test-dump ../actual
+'
+
+test_expect_success 'using --no-untracked-cache fails when core.untrackedCache 
is true' '
+   test_must_fail git update-index --no-untracked-cache &&
+   test-dump-untracked-cache >../actual &&
+   test_cmp ../expect-from-test-dump ../actual
+'
+
+test_expect_success 'setting core.untrackedCache to false and using git status 
removes the cache' '
+   git config core.untrackedCache false &&
+   test-dump-untracked-cache >../actual &&
+   test_cmp ../expect-from-test-dump ../actual &&
+   git status &&
+   test-dump-untracked-cache >../actual &&
+   test_cmp ../expect ../actual
+'
+
+test_expect_success 'using --untracked-cache fails when core.untrackedCache is 
false' '
+   test_must_fail git update-index --untracked-cache &&
+   test_must_fail git update-index --force-untracked-cache &&
+   test-dump-untracked-cache >../actual &&
+   test_cmp ../expect ../actual
+'
+
+test_expect_success 'setting core.untrackedCache to keep' '
+   git config core.untrackedCache keep &&
+   git update-index --untracked-cache &&
+   test-dump-untracked-cache >../actual &&
+   test_cmp ../expect-empty ../actual &&
+   git status &&
+   test-dump-untracked-cache >../actual &&
+   test_cmp ../expect-from-test-dump ../actual &&
+   git update-index --no-untracked-cache &&
+   test-dump-untracked-cache >../actual &&
+   test_cmp ../expect ../actual &&
+   git update-index --force-untracked-cache &&
+   test-dump-untracked-cache >../actual &&
+   test_cmp ../expect-empty ../actual &&
+   git status &&
+   test-dump-untracked-cache >../actual &&
+   test_cmp ../expect-from-test-dump ../actual
+'
+
 test_done
-- 
2.7.0.rc2.10.g544ad6b

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 09/10] config: add core.untrackedCache

[Christian Couder]

When we know that mtime is fully supported by the environment, we
might want the untracked cache to be always used by default without
any mtime test or kernel version check being performed.

Also when we know that mtime is not supported by the environment,
for example because the repo is shared over a network file system,
then we might want 'git update-index --untracked-cache' to fail
immediately instead of performing tests (because it might work on
some systems using the repo over the network file system but not
others).

The normal way to achieve the above in Git is to use a config
variable. That's why this patch introduces "core.untrackedCache".

This variable is a tristate, `keep`, `false` and `true`, which
default to `keep`.

When "git status" is run, it now adds or removes the untracked
cache in the index to respect the value of this variable. So it
does nothing if the value is `keep` or if the variable is unset;
it adds the untracked cache if the value is `true`; and it
removes the cache if the value is `false`.

`git update-index --[no-|force-]untracked-cache` still adds or
removes the untracked cache from the index, but this now fails
if it goes against the value of core.untrackedCache.

Also `--untracked-cache` used to check that the underlying
operating system and file system change `st_mtime` field of a
directory if files are added or deleted in that directory. But
those tests take a long time and there is now
`--test-untracked-cache` to perform them.

That's why, to be more consistent with other git commands, this
patch prevents `--untracked-cache` to perform tests, so that
after this patch there is no difference any more between
`--untracked-cache` and `--force-untracked-cache`.

This last change is backward incompatible and should be
mentioned in the release notes.

Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
Signed-off-by: Ævar Arnfjörð Bjarmason <ava...@gmail.com>
---
 Documentation/config.txt   |  7 
 Documentation/git-update-index.txt | 65 ++
 builtin/update-index.c | 35 --
 cache.h|  1 +
 config.c   | 11 ++
 contrib/completion/git-completion.bash |  1 +
 environment.c  |  1 +
 t/t7063-status-untracked-cache.sh  |  4 +--
 wt-status.c| 13 +++
 9 files changed, 109 insertions(+), 29 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index f617886..d892127 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -308,6 +308,13 @@ core.trustctime::
crawlers and some backup systems).
See linkgit:git-update-index[1]. True by default.
 
+core.untrackedCache::
+   Determines if untracked cache will be automatically enabled or
+   disabled. It can be `keep`, `true` or `false`. Before setting
+   it to `true`, you should check that mtime is working properly
+   on your system.
+   See linkgit:git-update-index[1]. `keep` by default.
+
 core.checkStat::
Determines which stat fields to match between the index
and work tree. The user can set this to 'default' or
diff --git a/Documentation/git-update-index.txt 
b/Documentation/git-update-index.txt
index a0afe17..813f6cc 100644
--- a/Documentation/git-update-index.txt
+++ b/Documentation/git-update-index.txt
@@ -174,27 +174,31 @@ may not support it yet.
 
 --untracked-cache::
 --no-untracked-cache::
-   Enable or disable untracked cache extension. This could speed
-   up for commands that involve determining untracked files such
-   as `git status`. The underlying operating system and file
-   system must change `st_mtime` field of a directory if files
-   are added or deleted in that directory.
+   Enable or disable untracked cache extension. Please use
+   `--test-untracked-cache` before enabling it.
++
+These options cannot override the `core.untrackedCache` configuration
+variable when it is set to `true` or `false` (see
+linkgit:git-config[1]). They can override it only when it is unset or
+set to `keep`. If you want untracked cache to persist, it is better
+anyway to just set this configuration variable to `true` or `false`
+directly.
 
 --test-untracked-cache::
Only perform tests on the working directory to make sure
untracked cache can be used. You have to manually enable
-   untracked cache using `--force-untracked-cache` (or
-   `--untracked-cache` but this will run the tests again)
-   afterwards if you really want to use it. If a test fails
-   the exit code is 1 and a message explains what is not
-   working as needed, otherwise the exit code is 0 and OK is
-   printed.
+   untracked cache using `--untracked-cache` or
+   `--force-untracked-cache` or the `core.untrackedCache`
+   configuration variable afte

[PATCH v4 08/10] dir: simplify untracked cache "ident" field

[Christian Couder]

It is not a good idea to compare kernel versions and disable
the untracked cache if it changes as people may upgrade and
still want the untracked cache to work. So let's just
compare work tree locations to decide if we should disable
it.

Also it's not useful to store many locations in the ident
field and compare to any of them. It can even be dangerous
if GIT_WORK_TREE is used with different values. So let's
just store one location, the location of the current work
tree.

If this location changed and we still want an untracked
cache, let's delete the cache and recreate it.

Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
---
 dir.c | 49 ++---
 1 file changed, 34 insertions(+), 15 deletions(-)

diff --git a/dir.c b/dir.c
index dba1ad0..1a8b5a2 100644
--- a/dir.c
+++ b/dir.c
@@ -1918,23 +1918,36 @@ static const char *get_ident_string(void)
return sb.buf;
 }
 
-static int ident_in_untracked(const struct untracked_cache *uc)
+static int ident_current_location_in_untracked(const struct untracked_cache 
*uc)
 {
-   const char *end = uc->ident.buf + uc->ident.len;
-   const char *p   = uc->ident.buf;
+   struct strbuf cur_loc = STRBUF_INIT;
+   int res = 0;
 
-   for (p = uc->ident.buf; p < end; p += strlen(p) + 1)
-   if (!strcmp(p, get_ident_string()))
-   return 1;
-   return 0;
+   /*
+* Previous git versions may have saved many strings in the
+* "ident" field, but it is insane to manage many locations,
+* so just take care of the first one.
+*/
+
+   strbuf_addf(_loc, "Location %s, system ", get_git_work_tree());
+
+   if (starts_with(uc->ident.buf, cur_loc.buf))
+   res = 1;
+
+   strbuf_release(_loc);
+
+   return res;
 }
 
-void add_untracked_ident(struct untracked_cache *uc)
+static void set_untracked_ident(struct untracked_cache *uc)
 {
-   if (ident_in_untracked(uc))
-   return;
+   strbuf_reset(>ident);
strbuf_addstr(>ident, get_ident_string());
-   /* this strbuf contains a list of strings, save NUL too */
+
+   /*
+* This strbuf used to contain a list of NUL separated
+* strings, so save NUL too for backward compatibility.
+*/
strbuf_addch(>ident, 0);
 }
 
@@ -1945,15 +1958,21 @@ static void new_untracked_cache(void)
uc->exclude_per_dir = ".gitignore";
/* should be the same flags used by git-status */
uc->dir_flags = DIR_SHOW_OTHER_DIRECTORIES | DIR_HIDE_EMPTY_DIRECTORIES;
+   set_untracked_ident(uc);
the_index.untracked = uc;
+   the_index.cache_changed |= UNTRACKED_CHANGED;
 }
 
 void add_untracked_cache(void)
 {
-   if (!the_index.untracked)
+   if (!the_index.untracked) {
new_untracked_cache();
-   add_untracked_ident(the_index.untracked);
-   the_index.cache_changed |= UNTRACKED_CHANGED;
+   } else {
+   if (!ident_current_location_in_untracked(the_index.untracked)) {
+   free_untracked_cache(the_index.untracked);
+   new_untracked_cache();
+   }
+   }
 }
 
 void remove_untracked_cache(void)
@@ -2021,7 +2040,7 @@ static struct untracked_cache_dir 
*validate_untracked_cache(struct dir_struct *d
if (dir->exclude_list_group[EXC_CMDL].nr)
return NULL;
 
-   if (!ident_in_untracked(dir->untracked)) {
+   if (!ident_current_location_in_untracked(dir->untracked)) {
warning(_("Untracked cache is disabled on this system."));
return NULL;
}
-- 
2.7.0.rc2.10.g544ad6b

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 00/10] Untracked cache improvements

[Christian Couder]

Here is a new version of a patch series to improve the untracked cache
feature.

This v4 implements core.untrackedCache as a tristate config
variable. When it's `true`, Git commands, especially `git status`,
should always add the untracked cache and use it, and when `false`,
Git commands should remove it. The default though is now `keep` in
which case the untracked cache is neither removed nor added, and used
if it is there.

Patch 1/10 is a small bugfix that has not changed since v3.

Patch 2/10 to 4/10 add some small features that are missing. The only
chqnge there since v3 is that we are now using `report()` to display
verbose information, thanks to Duy.

Patchs 5/10 to 7/10 are some refactoring to prepare for the following
patchs. Among them 6/10 is the result of merging two patchs from v3,
thanks to Eric.

Patch 8/10 deals with the "ident" field in "struct untracked_cache"
and is mostly the same as in v3. The difference is just a small bug
fix to prevent a crash.

Patch 9/10 adds core.untrackedCache. It has been changed compared to
v3 in the following ways:
  - the config variable is now a tristate, thanks to Junio,
  - we use `switch` to deal with different values, thanks to Torsten,
  - documentation for --test-untracked-cache is improved, thanks to
David.

Patch 10/10, which contains tests, has been changed to reflect changes
in 9/10 and to add a few tests.

So the changes compared to v3 are mostly small updates, and patchs
6/10, 9/10 and 10/10.

The patch series is also available there:

https://github.com/chriscool/git/tree/uc-notifs40

Thanks to the reviewers and helpers.

Christian Couder (10):
  dir: free untracked cache when removing it
  update-index: use enum for untracked cache options
  update-index: add --test-untracked-cache
  update-index: add untracked cache notifications
  update-index: move 'uc' var declaration
  dir: add {new,add}_untracked_cache()
  dir: add remove_untracked_cache()
  dir: simplify untracked cache "ident" field
  config: add core.untrackedCache
  t7063: add tests for core.untrackedCache

 Documentation/config.txt   |  7 +++
 Documentation/git-update-index.txt | 65 +++-
 builtin/update-index.c | 62 --
 cache.h|  1 +
 config.c   | 11 +
 contrib/completion/git-completion.bash |  1 +
 dir.c  | 70 --
 dir.h  |  2 +
 environment.c  |  1 +
 t/t7063-status-untracked-cache.sh  | 79 +++---
 wt-status.c| 13 ++
 11 files changed, 260 insertions(+), 52 deletions(-)

-- 
2.7.0.rc2.10.g544ad6b

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 03/10] update-index: add --test-untracked-cache

[Christian Couder]

It is nice to just be able to test if untracked cache is
supported without enabling it.

Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
---
 Documentation/git-update-index.txt | 12 +++-
 builtin/update-index.c |  5 +
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-update-index.txt 
b/Documentation/git-update-index.txt
index f4e5a85..a0afe17 100644
--- a/Documentation/git-update-index.txt
+++ b/Documentation/git-update-index.txt
@@ -18,7 +18,7 @@ SYNOPSIS
 [--[no-]skip-worktree]
 [--ignore-submodules]
 [--[no-]split-index]
-[--[no-|force-]untracked-cache]
+[--[no-|test-|force-]untracked-cache]
 [--really-refresh] [--unresolve] [--again | -g]
 [--info-only] [--index-info]
 [-z] [--stdin] [--index-version ]
@@ -180,6 +180,16 @@ may not support it yet.
system must change `st_mtime` field of a directory if files
are added or deleted in that directory.
 
+--test-untracked-cache::
+   Only perform tests on the working directory to make sure
+   untracked cache can be used. You have to manually enable
+   untracked cache using `--force-untracked-cache` (or
+   `--untracked-cache` but this will run the tests again)
+   afterwards if you really want to use it. If a test fails
+   the exit code is 1 and a message explains what is not
+   working as needed, otherwise the exit code is 0 and OK is
+   printed.
+
 --force-untracked-cache::
For safety, `--untracked-cache` performs tests on the working
directory to make sure untracked cache can be used. These
diff --git a/builtin/update-index.c b/builtin/update-index.c
index 1e546a3..6dd 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -40,6 +40,7 @@ enum uc_mode {
UC_UNSPECIFIED = -1,
UC_DISABLE = 0,
UC_ENABLE,
+   UC_TEST,
UC_FORCE
 };
 
@@ -1004,6 +1005,8 @@ int cmd_update_index(int argc, const char **argv, const 
char *prefix)
N_("enable or disable split index")),
OPT_BOOL(0, "untracked-cache", _cache,
N_("enable/disable untracked cache")),
+   OPT_SET_INT(0, "test-untracked-cache", _cache,
+   N_("test if the filesystem supports untracked 
cache"), UC_TEST),
OPT_SET_INT(0, "force-untracked-cache", _cache,
N_("enable untracked cache without testing the 
filesystem"), UC_FORCE),
OPT_END()
@@ -1119,6 +1122,8 @@ int cmd_update_index(int argc, const char **argv, const 
char *prefix)
setup_work_tree();
if (!test_if_untracked_cache_is_supported())
return 1;
+   if (untracked_cache == UC_TEST)
+   return 0;
}
if (!the_index.untracked) {
uc = xcalloc(1, sizeof(*uc));
-- 
2.7.0.rc2.10.g544ad6b

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 07/10] dir: add remove_untracked_cache()

[Christian Couder]

Factor out code into remove_untracked_cache(), which will be used
in a later commit.

Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
---
 builtin/update-index.c | 6 +-
 dir.c  | 9 +
 dir.h  | 1 +
 3 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/builtin/update-index.c b/builtin/update-index.c
index 74a6f8d..7844991 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -1126,11 +1126,7 @@ int cmd_update_index(int argc, const char **argv, const 
char *prefix)
add_untracked_cache();
report(_("Untracked cache enabled for '%s'"), 
get_git_work_tree());
} else if (untracked_cache == UC_DISABLE) {
-   if (the_index.untracked) {
-   free_untracked_cache(the_index.untracked);
-   the_index.untracked = NULL;
-   the_index.cache_changed |= UNTRACKED_CHANGED;
-   }
+   remove_untracked_cache();
report(_("Untracked cache disabled"));
}
 
diff --git a/dir.c b/dir.c
index 4227ba6..dba1ad0 100644
--- a/dir.c
+++ b/dir.c
@@ -1956,6 +1956,15 @@ void add_untracked_cache(void)
the_index.cache_changed |= UNTRACKED_CHANGED;
 }
 
+void remove_untracked_cache(void)
+{
+   if (the_index.untracked) {
+   free_untracked_cache(the_index.untracked);
+   the_index.untracked = NULL;
+   the_index.cache_changed |= UNTRACKED_CHANGED;
+   }
+}
+
 static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct 
*dir,
  int base_len,
  const struct pathspec 
*pathspec)
diff --git a/dir.h b/dir.h
index ee94c76..3e5114d 100644
--- a/dir.h
+++ b/dir.h
@@ -309,4 +309,5 @@ struct untracked_cache *read_untracked_extension(const void 
*data, unsigned long
 void write_untracked_extension(struct strbuf *out, struct untracked_cache 
*untracked);
 void add_untracked_ident(struct untracked_cache *);
 void add_untracked_cache(void);
+void remove_untracked_cache(void);
 #endif
-- 
2.7.0.rc2.10.g544ad6b

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 02/10] update-index: use enum for untracked cache options

[Christian Couder]

Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
---
 builtin/update-index.c | 18 +-
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/builtin/update-index.c b/builtin/update-index.c
index a6fff87..1e546a3 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -35,6 +35,14 @@ static int mark_skip_worktree_only;
 #define UNMARK_FLAG 2
 static struct strbuf mtime_dir = STRBUF_INIT;
 
+/* Untracked cache mode */
+enum uc_mode {
+   UC_UNSPECIFIED = -1,
+   UC_DISABLE = 0,
+   UC_ENABLE,
+   UC_FORCE
+};
+
 __attribute__((format (printf, 1, 2)))
 static void report(const char *fmt, ...)
 {
@@ -902,7 +910,7 @@ static int reupdate_callback(struct parse_opt_ctx_t *ctx,
 int cmd_update_index(int argc, const char **argv, const char *prefix)
 {
int newfd, entries, has_errors = 0, line_termination = '\n';
-   int untracked_cache = -1;
+   enum uc_mode untracked_cache = UC_UNSPECIFIED;
int read_from_stdin = 0;
int prefix_length = prefix ? strlen(prefix) : 0;
int preferred_index_format = 0;
@@ -997,7 +1005,7 @@ int cmd_update_index(int argc, const char **argv, const 
char *prefix)
OPT_BOOL(0, "untracked-cache", _cache,
N_("enable/disable untracked cache")),
OPT_SET_INT(0, "force-untracked-cache", _cache,
-   N_("enable untracked cache without testing the 
filesystem"), 2),
+   N_("enable untracked cache without testing the 
filesystem"), UC_FORCE),
OPT_END()
};
 
@@ -1104,10 +1112,10 @@ int cmd_update_index(int argc, const char **argv, const 
char *prefix)
the_index.split_index = NULL;
the_index.cache_changed |= SOMETHING_CHANGED;
}
-   if (untracked_cache > 0) {
+   if (untracked_cache > UC_DISABLE) {
struct untracked_cache *uc;
 
-   if (untracked_cache < 2) {
+   if (untracked_cache < UC_FORCE) {
setup_work_tree();
if (!test_if_untracked_cache_is_supported())
return 1;
@@ -1122,7 +1130,7 @@ int cmd_update_index(int argc, const char **argv, const 
char *prefix)
}
add_untracked_ident(the_index.untracked);
the_index.cache_changed |= UNTRACKED_CHANGED;
-   } else if (!untracked_cache && the_index.untracked) {
+   } else if (untracked_cache == UC_DISABLE && the_index.untracked) {
free_untracked_cache(the_index.untracked);
the_index.untracked = NULL;
the_index.cache_changed |= UNTRACKED_CHANGED;
-- 
2.7.0.rc2.10.g544ad6b

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 06/10] dir: add {new,add}_untracked_cache()

[Christian Couder]

Factor out code into new_untracked_cache() and
add_untracked_cache(), which will be used
in later commits.

Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
---
 builtin/update-index.c | 11 +--
 dir.c  | 18 ++
 dir.h  |  1 +
 3 files changed, 20 insertions(+), 10 deletions(-)

diff --git a/builtin/update-index.c b/builtin/update-index.c
index fe7aaa3..74a6f8d 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -1123,16 +1123,7 @@ int cmd_update_index(int argc, const char **argv, const 
char *prefix)
if (untracked_cache == UC_TEST)
return 0;
}
-   if (!the_index.untracked) {
-   struct untracked_cache *uc = xcalloc(1, sizeof(*uc));
-   strbuf_init(>ident, 100);
-   uc->exclude_per_dir = ".gitignore";
-   /* should be the same flags used by git-status */
-   uc->dir_flags = DIR_SHOW_OTHER_DIRECTORIES | 
DIR_HIDE_EMPTY_DIRECTORIES;
-   the_index.untracked = uc;
-   }
-   add_untracked_ident(the_index.untracked);
-   the_index.cache_changed |= UNTRACKED_CHANGED;
+   add_untracked_cache();
report(_("Untracked cache enabled for '%s'"), 
get_git_work_tree());
} else if (untracked_cache == UC_DISABLE) {
if (the_index.untracked) {
diff --git a/dir.c b/dir.c
index d2a8f06..4227ba6 100644
--- a/dir.c
+++ b/dir.c
@@ -1938,6 +1938,24 @@ void add_untracked_ident(struct untracked_cache *uc)
strbuf_addch(>ident, 0);
 }
 
+static void new_untracked_cache(void)
+{
+   struct untracked_cache *uc = xcalloc(1, sizeof(*uc));
+   strbuf_init(>ident, 100);
+   uc->exclude_per_dir = ".gitignore";
+   /* should be the same flags used by git-status */
+   uc->dir_flags = DIR_SHOW_OTHER_DIRECTORIES | DIR_HIDE_EMPTY_DIRECTORIES;
+   the_index.untracked = uc;
+}
+
+void add_untracked_cache(void)
+{
+   if (!the_index.untracked)
+   new_untracked_cache();
+   add_untracked_ident(the_index.untracked);
+   the_index.cache_changed |= UNTRACKED_CHANGED;
+}
+
 static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct 
*dir,
  int base_len,
  const struct pathspec 
*pathspec)
diff --git a/dir.h b/dir.h
index 7b5855d..ee94c76 100644
--- a/dir.h
+++ b/dir.h
@@ -308,4 +308,5 @@ void free_untracked_cache(struct untracked_cache *);
 struct untracked_cache *read_untracked_extension(const void *data, unsigned 
long sz);
 void write_untracked_extension(struct strbuf *out, struct untracked_cache 
*untracked);
 void add_untracked_ident(struct untracked_cache *);
+void add_untracked_cache(void);
 #endif
-- 
2.7.0.rc2.10.g544ad6b

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 04/10] update-index: add untracked cache notifications

[Christian Couder]

Attempting to flip the untracked-cache feature on for a random index
file with

cd /random/unrelated/place
git --git-dir=/somewhere/else/.git update-index --untracked-cache

would not work as you might expect. Because flipping the feature on
in the index also records the location of the corresponding working
tree (/random/unrelated/place in the above example), when the index
is subsequently used to keep track of files in the working tree in
/somewhere/else, the feature is disabled.

With this patch "git update-index --[test-]untracked-cache" tells the
user in which directory tests are performed. This makes it easy to
spot any problem.

Also in verbose mode, let's tell the user when the cache is enabled
or disabled.

Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
---
 builtin/update-index.c | 14 +-
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/builtin/update-index.c b/builtin/update-index.c
index 6dd..369c207 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -130,7 +130,7 @@ static int test_if_untracked_cache_is_supported(void)
if (!mkdtemp(mtime_dir.buf))
die_errno("Could not make temporary directory");
 
-   fprintf(stderr, _("Testing "));
+   fprintf(stderr, _("Testing mtime in '%s' "), xgetcwd());
atexit(remove_test_directory);
xstat_mtime_dir();
fill_stat_data(, );
@@ -1135,10 +1135,14 @@ int cmd_update_index(int argc, const char **argv, const 
char *prefix)
}
add_untracked_ident(the_index.untracked);
the_index.cache_changed |= UNTRACKED_CHANGED;
-   } else if (untracked_cache == UC_DISABLE && the_index.untracked) {
-   free_untracked_cache(the_index.untracked);
-   the_index.untracked = NULL;
-   the_index.cache_changed |= UNTRACKED_CHANGED;
+   report(_("Untracked cache enabled for '%s'"), 
get_git_work_tree());
+   } else if (untracked_cache == UC_DISABLE) {
+   if (the_index.untracked) {
+   free_untracked_cache(the_index.untracked);
+   the_index.untracked = NULL;
+   the_index.cache_changed |= UNTRACKED_CHANGED;
+   }
+   report(_("Untracked cache disabled"));
}
 
if (active_cache_changed) {
-- 
2.7.0.rc2.10.g544ad6b

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 05/10] update-index: move 'uc' var declaration

[Christian Couder]

Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
---
 builtin/update-index.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/builtin/update-index.c b/builtin/update-index.c
index 369c207..fe7aaa3 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -1116,8 +1116,6 @@ int cmd_update_index(int argc, const char **argv, const 
char *prefix)
the_index.cache_changed |= SOMETHING_CHANGED;
}
if (untracked_cache > UC_DISABLE) {
-   struct untracked_cache *uc;
-
if (untracked_cache < UC_FORCE) {
setup_work_tree();
if (!test_if_untracked_cache_is_supported())
@@ -1126,7 +1124,7 @@ int cmd_update_index(int argc, const char **argv, const 
char *prefix)
return 0;
}
if (!the_index.untracked) {
-   uc = xcalloc(1, sizeof(*uc));
+   struct untracked_cache *uc = xcalloc(1, sizeof(*uc));
strbuf_init(>ident, 100);
uc->exclude_per_dir = ".gitignore";
/* should be the same flags used by git-status */
-- 
2.7.0.rc2.10.g544ad6b

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC/PATCH] config: add core.trustmtime

[Christian Couder]

When we know that mtime is fully supported by the environment, we
don't want any slow down because we used --untracked-cache rather
than --force-untracked-cache, and we don't want untracked cache to
stop working because we updated a kernel.

Also when we know that mtime is not supported by the environment,
for example because the repo is shared over a network file system,
then we might want 'git update-index --untracked-cache' to fail
immediately instead of it testing if it works (because it might
work on some systems using the repo over the network file system
but not others).

Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
---
At Booking.com we know that mtime works everywhere and we don't
want the untracked cache to stop working when a kernel is upgraded
or when the repo is copied to a machine with a different kernel.
I will add tests later if people are ok with this.

 Documentation/config.txt   | 8 
 Documentation/git-update-index.txt | 6 +-
 builtin/update-index.c | 6 +-
 cache.h| 1 +
 config.c   | 7 +++
 contrib/completion/git-completion.bash | 1 +
 dir.c  | 2 +-
 environment.c  | 1 +
 8 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index b4b0194..186ad17 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -308,6 +308,14 @@ core.trustctime::
crawlers and some backup systems).
See linkgit:git-update-index[1]. True by default.
 
+core.trustmtime::
+   If unset or set to 'default' or 'check', Git will test if
+   mtime is working properly before using features that need a
+   working mtime. If false, Git will refuse to use such
+   features. If set to true, Git will blindly use such features
+   without testing if they work.
+   See linkgit:git-update-index[1].
+
 core.checkStat::
Determines which stat fields to match between the index
and work tree. The user can set this to 'default' or
diff --git a/Documentation/git-update-index.txt 
b/Documentation/git-update-index.txt
index 1a296bc..8b4c5af 100644
--- a/Documentation/git-update-index.txt
+++ b/Documentation/git-update-index.txt
@@ -182,7 +182,9 @@ may not support it yet.
For safety, `--untracked-cache` performs tests on the working
directory to make sure untracked cache can be used. These
tests can take a few seconds. `--force-untracked-cache` can be
-   used to skip the tests.
+   used to skip the tests. If you always want to skip those
+   tests, you can set the `core.trustmtime` configuration
+   variable to 'true', (see linkgit:git-config[1]).
 
 \--::
Do not interpret any more arguments as options.
@@ -398,6 +400,8 @@ It can be useful when the inode change time is regularly 
modified by
 something outside Git (file system crawlers and backup systems use
 ctime for marking files processed) (see linkgit:git-config[1]).
 
+Untracked cache needs a fully working mtime, so it will look at
+`core.trustmtime` configuration variable (see linkgit:git-config[1]).
 
 SEE ALSO
 
diff --git a/builtin/update-index.c b/builtin/update-index.c
index 7431938..c64439f 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -1107,9 +1107,13 @@ int cmd_update_index(int argc, const char **argv, const 
char *prefix)
if (untracked_cache > 0) {
struct untracked_cache *uc;
 
+   if (trust_mtime == 0) {
+   fprintf_ln(stderr,_("core.trustmtime is set to false"));
+   return 1;
+   }
if (untracked_cache < 2) {
setup_work_tree();
-   if (!test_if_untracked_cache_is_supported())
+   if (trust_mtime != 1 && 
!test_if_untracked_cache_is_supported())
return 1;
}
if (!the_index.untracked) {
diff --git a/cache.h b/cache.h
index 736abc0..69a6197 100644
--- a/cache.h
+++ b/cache.h
@@ -601,6 +601,7 @@ extern void set_alternate_index_output(const char *);
 /* Environment bits from configuration mechanism */
 extern int trust_executable_bit;
 extern int trust_ctime;
+extern int trust_mtime;
 extern int check_stat;
 extern int quote_path_fully;
 extern int has_symlinks;
diff --git a/config.c b/config.c
index 248a21a..d720b1f 100644
--- a/config.c
+++ b/config.c
@@ -691,6 +691,13 @@ static int git_default_core_config(const char *var, const 
char *value)
trust_ctime = git_config_bool(var, value);
return 0;
}
+   if (!strcmp(var, "core.trustmtime")) {
+   if (!strcasecmp(value, "default") || !strcasecmp(value, 
"check"))
+   trust_mtime = 

[PATCH v2] Documentation/git-update-index: add missing opts to synopsys

[Christian Couder]

Untracked cache and split index related options should appear
in the 'SYNOPSIS' section.

These options are already documented in the 'OPTIONS' section.

Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
---
Soon after sending the first version I realized that the split index
options were not in the synopsys either...

 Documentation/git-update-index.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/git-update-index.txt 
b/Documentation/git-update-index.txt
index 1a296bc..f4e5a85 100644
--- a/Documentation/git-update-index.txt
+++ b/Documentation/git-update-index.txt
@@ -17,6 +17,8 @@ SYNOPSIS
 [--[no-]assume-unchanged]
 [--[no-]skip-worktree]
 [--ignore-submodules]
+[--[no-]split-index]
+[--[no-|force-]untracked-cache]
 [--really-refresh] [--unresolve] [--again | -g]
 [--info-only] [--index-info]
 [-z] [--stdin] [--index-version ]
-- 
2.6.3.380.g494b52d

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Documentation/git-update-index: add missing opts to synopsys

[Christian Couder]

On Tue, Nov 24, 2015 at 9:46 PM, Jeff King <p...@peff.net> wrote:
> On Tue, Nov 24, 2015 at 12:55:07PM +0100, Christian Couder wrote:
>
>> Untracked cache related options should appear in the synopsis.
>>
>> Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
>> ---
>>  Documentation/git-update-index.txt | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/Documentation/git-update-index.txt 
>> b/Documentation/git-update-index.txt
>> index 1a296bc..3df9c26 100644
>> --- a/Documentation/git-update-index.txt
>> +++ b/Documentation/git-update-index.txt
>> @@ -17,6 +17,7 @@ SYNOPSIS
>>[--[no-]assume-unchanged]
>>[--[no-]skip-worktree]
>>[--ignore-submodules]
>> +  [--[no-|force-]untracked-cache]
>
> Thanks, and it looks like they are already documented in the OPTIONS
> section.

Yeah, I just added this information into the commit message and sent a
v2 that also add split index related options to the synopsis.

Thanks,
Christian.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] Documentation/git-update-index: add missing opts to synopsys

[Christian Couder]

On Wed, Nov 25, 2015 at 10:03 AM, Jeff King <p...@peff.net> wrote:
> On Wed, Nov 25, 2015 at 07:53:12AM +0100, Christian Couder wrote:
>
>> Untracked cache and split index related options should appear
>> in the 'SYNOPSIS' section.
>>
>> These options are already documented in the 'OPTIONS' section.
>>
>> Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
>> ---
>> Soon after sending the first version I realized that the split index
>> options were not in the synopsys either...
>
> Sorry, too late. I merged v1 as part of yesterday's cycle, as it seemed
> obviously correct (that's what I get... :) ).
>
> Can you please send the change as a patch on top?

Ok will do.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH] config: add core.trustmtime

[Christian Couder]

Hi Johannes,

On Wed, Nov 25, 2015 at 11:25 AM, Johannes Schindelin
<johannes.schinde...@gmx.de> wrote:
> Hi Christian,
>
> On Wed, 25 Nov 2015, Christian Couder wrote:
>
>> diff --git a/config.c b/config.c
>> index 248a21a..d720b1f 100644
>> --- a/config.c
>> +++ b/config.c
>> @@ -691,6 +691,13 @@ static int git_default_core_config(const char *var, 
>> const char *value)
>>   trust_ctime = git_config_bool(var, value);
>>   return 0;
>>   }
>> + if (!strcmp(var, "core.trustmtime")) {
>> + if (!strcasecmp(value, "default") || !strcasecmp(value, 
>> "check"))
>> + trust_mtime = -1;
>> + else
>> + trust_mtime = git_config_maybe_bool(var, value);
>
> If `core.trustmtime` is set to `OMG, never!`, `git_config_maybe_bool()`
> will set trust_mtime to -1, i.e. the exact same value as if you had set
> the variable to `default` or `check`...

Yeah, I think returning -1 is the safe thing to do in this case.

> Maybe you want to be a bit stricter here and either test the return value
> of `git_config_maybe_bool()` for -1 (and warn in that case), or just
> delete the `maybe_`?

I thought that if we ever add more options in the future then people
might be annoyed by older git versions warning about the new options.
But I am ok to add a warning.

Thanks,
Christian.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4] Documentation/git-update-index: add missing opts to synopsis

[Christian Couder]

Split index related options should appear in the 'SYNOPSIS'
section.

These options are already documented in the 'OPTIONS' section.

Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
---
This v4 contains only the split-index options and applies on top
of the new master that already contains the untracked-cache options. 

 Documentation/git-update-index.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/git-update-index.txt 
b/Documentation/git-update-index.txt
index 3df9c26..f4e5a85 100644
--- a/Documentation/git-update-index.txt
+++ b/Documentation/git-update-index.txt
@@ -17,6 +17,7 @@ SYNOPSIS
 [--[no-]assume-unchanged]
 [--[no-]skip-worktree]
 [--ignore-submodules]
+[--[no-]split-index]
 [--[no-|force-]untracked-cache]
 [--really-refresh] [--unresolve] [--again | -g]
 [--info-only] [--index-info]
-- 
2.6.3.380.g494b52d

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3] Documentation/git-update-index: add missing opts to synopsis

[Christian Couder]

Untracked cache and split index related options should appear
in the 'SYNOPSIS' section.

These options are already documented in the 'OPTIONS' section.

Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
---
The only change compared to v2 is s/synopsys/synopsis/ thanks to Eric.

 Documentation/git-update-index.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/git-update-index.txt 
b/Documentation/git-update-index.txt
index 1a296bc..f4e5a85 100644
--- a/Documentation/git-update-index.txt
+++ b/Documentation/git-update-index.txt
@@ -17,6 +17,8 @@ SYNOPSIS
 [--[no-]assume-unchanged]
 [--[no-]skip-worktree]
 [--ignore-submodules]
+[--[no-]split-index]
+[--[no-|force-]untracked-cache]
 [--really-refresh] [--unresolve] [--again | -g]
 [--info-only] [--index-info]
 [-z] [--stdin] [--index-version ]
-- 
2.6.3.380.g494b52d

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH] config: add core.trustmtime

[Christian Couder]

Hi Duy,

On Wed, Nov 25, 2015 at 8:51 PM, Duy Nguyen <pclo...@gmail.com> wrote:
> On Wed, Nov 25, 2015 at 10:00 AM, Ævar Arnfjörð Bjarmason
> <ava...@gmail.com> wrote:
>> On Wed, Nov 25, 2015 at 7:35 AM, Christian Couder
>> <christian.cou...@gmail.com> wrote:
>>> At Booking.com we know that mtime works everywhere and we don't
>>> want the untracked cache to stop working when a kernel is upgraded
>>> or when the repo is copied to a machine with a different kernel.
>>> I will add tests later if people are ok with this.
>>
>> I bit more info: I rolled Git out internally with this patch:
>> https://github.com/avar/git/commit/c63f7c12c2664631961add7cf3da901b0b6aa2f2
>>
>> The --untracked-cache feature hardcodes the equivalent of:
>>
>> pwd; uname --kernel-name --kernel-release --kernel-version
>>
>> Into the index. If any of those change it prints out the "cache is
>> disabled" warning.
>>
>> This patch will make it stop being so afraid of itself to the point of
>> disabling itself on minor kernel upgrades :)
>
> The problem is, there's no way to teach git to know it's a "minor"
> upgrade.. but if there is a config key to say "don't be paranoid, I
> know what I'm doing", then we can skip that check, or just warn
> instead of disabling the cache.

Yeah, in my patch if core.trustmtime is set to true or false the check
is skipped.

I am wondering why you didn't make it by default run the mtime checks
when a kernel change is detected. Maybe that would be better than
disabling itself.

>> A few other issues with this feature I've noticed:
>>
>>  * There's no way to just enable it globally via the config. Makes it
>> a bit of a hassle to use it. I wanted to have a config option to
>> enable it via the config, how about "index.untracked_cache = true" for
>> the config variable name?
>
> If you haven't noticed, all these experimental features have no real
> UI (update-index is plumbing). I have been waiting for someone like
> you to start using it and figure out the best UI (then implement it)
> ;)

Ok, we are happy to do that (including implementing it) :-)

I will take a look at something like index.untracked_cache. It will
probably also be a tristate like this:

- true: always enable it; die if core.trustmtime is false otherwise
warn if it is not true
- default/unset: same as current behavior
- false: die if it is enabled or when trying to enable to it

>>  * Doing "cd /tmp: git --git-dir=/git/somewhere/else/.git update-index
>> --untracked-cache" doesn't work how I'd expect. It hardcodes "/tmp" as
>> the directory that "works" into the index, so if you use the working
>> tree you'll never use the untracked cache. I spotted this because I
>> carry out a bunch of git maintenance commands with --git-dir instead
>> of cd-ing to the relevant directories. This works for most other
>> things in git, is it a bug that it doesn't work here?
>
> It needs the current directory at --untrack-cache time to test if the
> directory satisfies the requirements. So either you cd to that
> worktree, or you have to specify --worktree as well. Or am I missing
> something?

Maybe it could print out a message saying "Testing mtime in directory
$(pwd)" and if that works then "Untracked cache is enabled for
$(pwd)". That would make it clear that it will not work in other
directories.

Also maybe the mtime checks could be run when a directory change is detected.

>>  * If you "ctrl+c" git update-index --untracked-cache at an
>> inopportune time you'll end up with a mtime-test-XX directory in
>> your working tree. Perhaps this tempdir should be created in the .git
>> directory instead?
>
> No because in theory .git could be on a separate file system with
> different semantics. But we should probably clean those files at ^C.

Ok, I will have a look at cleaning the files at ^C.

>>  * Maybe we should have a --test-untracked-cache option, so you can
>> run the tests without enabling it.
>
> I'd say patches welcome.

Ok, I wll have a look at that too.

>> Aside from the slight hassle of enabling this and keeping it enabled
>> this feature is great. It's sped up "git status" across the board by
>> about 40%. Slightly less than that on faster spinning disks, slightly
>> more than that on slower ones.
>
> I'm still waiting for the day when watchman support gets merged and
> maybe poke Facebook guys to compare performance with Mercurial :)
> Well, we are probably still behind Mercurial on that day.

Yeah, it could be interesting to compare performance with Mercurial a

Re: [PATCH 0/5] Use watchman to reduce index refresh time

[Christian Couder]

On Tue, Nov 10, 2015 at 10:04 PM, David Turner <dtur...@twopensource.com> wrote:
> On Mon, 2015-11-09 at 21:06 +0100, Christian Couder wrote:
>> Using David's series I get worse results than all of the above but I
>> guess it's because his series is based on an ancient git version
>> (v2.0.0-rc0).
>
> My more-recent series is on top of 2.4, but (for webkit):
> mine: 0m0.206s
> duy's: 0m0.107s

I tried using your more recent series and I get basically no change
compared to the current git (without using untracked cache) on Webkit
with around 5600 untracked files. Maybe I am doing something wrong.

I compiled watchman and installed it. It is in /usr/local/bin/watchman
but I am not sure it is actually being used though I have configured
"core.usewatchman" to "true".

> However, I'm getting occasional index-helper segfaults due to
> istate->last_update being NULL.  (I'm using Duy's  index-helper branch
> from his github + this patchset + a function signature fix due to a
> newer version of libwatchman).  I haven't looked into why this is --
> maybe accidentally mixing git versions while testing?
>
> Also, after messing around for a while (on Duy's branch), I ended up in
> a state where git status would take ~2.5s every time.  The index helper
> was alive but evidently not working right.  Killing and restarting it
> worked.  Actually, I think I can repro this more easily: "git rm
> Changelog" seems to put the index-helper into this state.

Thanks for this. I have also seen strange things happening with the
index-helper.
When it is working I found that it provides around 10% improvement on
git rebase time.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] Documentation/git-update-index: add missing opts to synopsys

[Christian Couder]

Untracked cache related options should appear in the synopsis.

Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
---
 Documentation/git-update-index.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/git-update-index.txt 
b/Documentation/git-update-index.txt
index 1a296bc..3df9c26 100644
--- a/Documentation/git-update-index.txt
+++ b/Documentation/git-update-index.txt
@@ -17,6 +17,7 @@ SYNOPSIS
 [--[no-]assume-unchanged]
 [--[no-]skip-worktree]
 [--ignore-submodules]
+[--[no-|force-]untracked-cache]
 [--really-refresh] [--unresolve] [--again | -g]
 [--info-only] [--index-info]
 [-z] [--stdin] [--index-version ]
-- 
2.6.2.412.gf783589.dirty

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 1/2] builtin/apply: add 'lock_file' pointer into 'struct apply_state'

[Christian Couder]

From: Christian Couder <christian.cou...@gmail.com>

We cannot have a 'struct lock_file' allocated on the stack, as lockfile.c
keeps a linked list of all created lock_file structures.

Also 'struct apply_state' users might later want the same 'struct lock_file'
instance to be reused by different series of calls to the apply api.

So let's add a 'struct lock_file *lock_file' pointer into 'struct apply_state'
and have the user of 'struct apply_state' allocate memory for the actual
'struct lock_file' instance.

Let's also add an argument to init_apply_state(), so that the caller can
easily supply a pointer to the allocated instance.

Helped-by: Eric Sunshine <sunsh...@sunshineco.com>
Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
Signed-off-by: Junio C Hamano <gits...@pobox.com>
---

This is to replace:

"[PATCH v3 48/49] builtin/apply: move 'lock_file' global into 'struct 
apply_state'"

from the "libify apply and use lib in am, part 1" patch series.

The changes are the following:

  - The "static struct lock_file lock_file" is not removed anymore.

  - init_apply_state() doesn't allocate a 'struct lock_file' anymore
when passed a NULL 'struct lock_file' pointer.

  - A check that state->lock_file is not NULL has been added in
check_apply_state().

  - Title and commit message have been modified to reflect the above
changes.

This 2 patch long patch series on top of the other unchanged commits
from v3 is available here:

https://github.com/chriscool/git/commits/libify-apply63

 builtin/apply.c | 19 +++
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 5027f1b..cc635eb 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -52,6 +52,12 @@ struct apply_state {
const char *prefix;
int prefix_length;
 
+   /*
+* Since lockfile.c keeps a linked list of all created
+* lock_file structures, it isn't safe to free(lock_file).
+*/
+   struct lock_file *lock_file;
+
/* These control what gets looked at and modified */
int apply; /* this is not a dry-run */
int cached; /* apply to the index only */
@@ -4547,7 +4553,7 @@ static int apply_patch(struct apply_state *state,
 
state->update_index = state->check_index && state->apply;
if (state->update_index && newfd < 0)
-   newfd = hold_locked_index(_file, 1);
+   newfd = hold_locked_index(state->lock_file, 1);
 
if (state->check_index) {
if (read_cache() < 0)
@@ -4648,11 +4654,14 @@ static int option_parse_directory(const struct option 
*opt,
return 0;
 }
 
-static void init_apply_state(struct apply_state *state, const char *prefix)
+static void init_apply_state(struct apply_state *state,
+const char *prefix,
+struct lock_file *lock_file)
 {
memset(state, 0, sizeof(*state));
state->prefix = prefix;
state->prefix_length = state->prefix ? strlen(state->prefix) : 0;
+   state->lock_file = lock_file;
state->apply = 1;
state->line_termination = '\n';
state->p_value = 1;
@@ -4705,6 +4714,8 @@ static void check_apply_state(struct apply_state *state, 
int force_apply)
}
if (state->check_index)
state->unsafe_paths = 0;
+   if (!state->lock_file)
+   die("BUG: state->lock_file should not be NULL");
 }
 
 static int apply_all_patches(struct apply_state *state,
@@ -4769,7 +4780,7 @@ static int apply_all_patches(struct apply_state *state,
}
 
if (state->update_index) {
-   if (write_locked_index(_index, _file, COMMIT_LOCK))
+   if (write_locked_index(_index, state->lock_file, 
COMMIT_LOCK))
die(_("Unable to write new index file"));
}
 
@@ -4852,7 +4863,7 @@ int cmd_apply(int argc, const char **argv, const char 
*prefix)
OPT_END()
};
 
-   init_apply_state(, prefix);
+   init_apply_state(, prefix, _file);
 
argc = parse_options(argc, argv, state.prefix, builtin_apply_options,
apply_usage, 0);
-- 
2.8.2.444.g74f55c9.dirty

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 2/2] builtin/apply: move 'newfd' global into 'struct apply_state'

[Christian Couder]

From: Christian Couder <christian.cou...@gmail.com>

To libify the apply functionality the 'newfd' variable should
not be static and global to the file. Let's move it into
'struct apply_state'.

Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
Signed-off-by: Junio C Hamano <gits...@pobox.com>
---

This is to replace:

[PATCH v3 49/49] builtin/apply: move 'newfd' global into 'struct apply_state'

from the "libify apply and use lib in am, part 1" patch series.

There is no change compared to the previous version except that this
patch should apply cleanly on top of PATCH v4 1/2.

 builtin/apply.c | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index cc635eb..7338ee6 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -57,6 +57,7 @@ struct apply_state {
 * lock_file structures, it isn't safe to free(lock_file).
 */
struct lock_file *lock_file;
+   int newfd;
 
/* These control what gets looked at and modified */
int apply; /* this is not a dry-run */
@@ -120,8 +121,6 @@ struct apply_state {
int applied_after_fixing_ws;
 };
 
-static int newfd = -1;
-
 static const char * const apply_usage[] = {
N_("git apply [] [...]"),
NULL
@@ -4552,8 +4551,8 @@ static int apply_patch(struct apply_state *state,
state->apply = 0;
 
state->update_index = state->check_index && state->apply;
-   if (state->update_index && newfd < 0)
-   newfd = hold_locked_index(state->lock_file, 1);
+   if (state->update_index && state->newfd < 0)
+   state->newfd = hold_locked_index(state->lock_file, 1);
 
if (state->check_index) {
if (read_cache() < 0)
@@ -4662,6 +4661,7 @@ static void init_apply_state(struct apply_state *state,
state->prefix = prefix;
state->prefix_length = state->prefix ? strlen(state->prefix) : 0;
state->lock_file = lock_file;
+   state->newfd = -1;
state->apply = 1;
state->line_termination = '\n';
state->p_value = 1;
@@ -4782,6 +4782,7 @@ static int apply_all_patches(struct apply_state *state,
if (state->update_index) {
if (write_locked_index(_index, state->lock_file, 
COMMIT_LOCK))
die(_("Unable to write new index file"));
+   state->newfd = -1;
}
 
return !!errs;
-- 
2.8.2.444.g74f55c9.dirty

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 1/2] builtin/apply: add 'lock_file' pointer into 'struct apply_state'

[Christian Couder]

On Fri, Jun 3, 2016 at 6:58 PM, Christian Couder
<christian.cou...@gmail.com> wrote:
> From: Christian Couder <christian.cou...@gmail.com>

Sorry for this spurious "From:" line.
It looks like send-email added it, and I don't understand why it does it now.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 63/94] builtin/apply: make apply_all_patches() return -1 on error

[Christian Couder]

On Mon, May 16, 2016 at 5:44 AM, Eric Sunshine <sunsh...@sunshineco.com> wrote:
> On Wed, May 11, 2016 at 9:17 AM, Christian Couder
> <christian.cou...@gmail.com> wrote:
>> To finish libifying the apply functionality, apply_all_patches() should not
>> die() or exit() in case of error, but return -1.
>>
>> While doing that we must take care that file descriptors are properly closed
>> and, if needed, reset a sensible value.
>>
>> Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
>> ---
>> diff --git a/builtin/apply.c b/builtin/apply.c
>> @@ -4613,9 +4613,10 @@ static int apply_all_patches(struct apply_state 
>> *state,
>> }
>>
>> if (state->update_index) {
>> -   if (write_locked_index(_index, state->lock_file, 
>> COMMIT_LOCK))
>> -   die(_("Unable to write new index file"));
>> +   res = write_locked_index(_index, state->lock_file, 
>> COMMIT_LOCK);
>> state->newfd = -1;
>
> Does write_locked_index() unconditionally close the file descriptor
> even when an error occurs? If not, then isn't this potentially leaking
> 'newfd'?
>
> (My very cursory read of write_locked_index() seems to reveal that the
> file descriptor may indeed remain open upon index write failure.)

You are right, it is leaking newfd if write_locked_index() fails.
The solution to that is to call `rollback_lock_file(state->lock_file)`
and the following patch was supposed to do that:

[PATCH v2 82/94] apply: roll back index lock file in case of error

but it would do that only if `state->newfd >= 0` so we should set
state->newfd to -1 only if write_locked_index() succeeds.

I will fix this.

I am also going to add a comment to this patch saying that this patch
needs a following patch to call rollback_lock_file(state->lock_file)
in case of errors.

Or if you prefer, I can squash the patch that call
rollback_lock_file(state->lock_file) in case of errors into this
patch.

Thanks,
Christian.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 56/94] apply: move 'struct apply_state' to apply.h

[Christian Couder]

On Mon, May 16, 2016 at 6:03 PM, Junio C Hamano <gits...@pobox.com> wrote:
> Eric Sunshine <sunsh...@sunshineco.com> writes:
>
>> On Wed, May 11, 2016 at 9:17 AM, Christian Couder
>> <christian.cou...@gmail.com> wrote:
>>> To libify `git apply` functionality we must make 'struct apply_state'
>>> usable outside "builtin/apply.c".
>>
>> Why is this patch plopped right in the middle of a bunch of other
>> patches which are making functions return -1 rather than die()ing?
>> Seems out of place.
>
> Two possible places that would make more sense are (1) when it is
> introduced very early in the series, or (2) when it absorbed all the
> file-scope-static global states in the middle of the series.  I think
> either is fine.
>
> That would be a good place to end the first batch of the topic.
> Then the second batch would be "turn die() into error status that is
> propagated upwards".

I moved this patch at the beginning of second batch that I will send
hopefully soon...
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 54/94] builtin/apply: make parse_chunk() return a negative integer on error

[Christian Couder]

On Mon, May 16, 2016 at 5:04 AM, Eric Sunshine <sunsh...@sunshineco.com> wrote:
> On Wed, May 11, 2016 at 9:17 AM, Christian Couder
> <christian.cou...@gmail.com> wrote:
>> To libify `git apply` functionality we have to signal errors to the
>> caller instead of die()ing or exit()ing.
>>
>> To do that in a compatible manner with the rest of the error handling
>> in builtin/apply.c, find_header() should return -1 instead of calling
>> die() or exit().
>
> Why is this talking about making find_header() return -1? Didn't that
> happen in the previous patch?

Yeah, it should be parse_chunk() not find_header().

This is fixed in my current branch.

Thanks,
Christian.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 4/4] bundle v3: the beginning

[Christian Couder]

On Wed, Jun 1, 2016 at 4:00 PM, Duy Nguyen <pclo...@gmail.com> wrote:
> On Tue, May 31, 2016 at 8:18 PM, Christian Couder
> <christian.cou...@gmail.com> wrote:
>>>> [3] 
>>>> http://thread.gmane.org/gmane.comp.version-control.git/202902/focus=203020
>>>
>>> This points to  https://github.com/peff/git/commits/jk/external-odb
>>> which is dead. Jeff, do you still have it somewhere, or is it not
>>> worth looking at anymore?
>>
>> I have rebased, fixed and improved it a bit. I added write support for
>> blobs. But the result is not very clean right now.
>> I was going to send a RFC patch series after cleaning the result, but
>> as you ask, here are some links to some branches:
>>
>> - https://github.com/chriscool/git/commits/gl-external-odb3 (the
>> updated patches from Peff, plus 2 small patches from me)
>> - https://github.com/chriscool/git/commits/gl-external-odb7 (the same
>> as above, plus a number of WIP patches to add blob write support)
>
> Thanks. I had a super quick look. It would be nice if you could give a
> high level overview on this (if you're going to spend a lot more time on it).

Sorry about the late answer.

Here is a new series after some cleanup:

https://github.com/chriscool/git/commits/gl-external-odb12

The high level overview of the patch series I would like to send
really soon now could go like this:

---
Git can store its objects only in the form of loose objects in
separate files or packed objects in a pack file.
To be able to better handle some kind of objects, for example big
blobs, it would be nice if Git could store its objects in other object
databases (ODB).

To do that, this patch series makes it possible to register commands,
using "odb..command" config variables, to access external
ODBs. Each specified command will then be called the following ways:

  - " have": the command should output the sha1, size and
type of all the objects the external ODB contains, one object per
line.
  - " get ": the command should then read from the
external ODB the content of the object corresponding to  and
output it on stdout.
  - " put   ": the command should then read
from stdin an object and store it in the external ODB.

This RFC patch series does not address the following important parts
of a complete solution:

  - There is no way to transfer external ODB content using Git.
  - No real external ODB has been interfaced with Git. The tests use
another git repo in a separate directory for this purpose which is
probably useless in the real world.
---

> One random thought, maybe it's better to have a daemon for external
> odb right from the start (one for all odbs, or one per odb, I don't
> know). It could do fancy stuff like object caching if necessary, and
> it can avoid high cost handshake (e.g. via tls) every time a git
> process runs and gets one object. Reducing process spawn would
> definitely receive a big cheer from Windows crowd.

The caching could be done inside Git and I am not sure it's worth
optimizing this now.
It could also make it more difficult to write support for an external
ODB if we required a daemon.
Maybe later we can add support for "odb..daemon" if we think
that this is worth it.

> Any thought on object streaming support?

No I didn't think about this. In fact I am not sure what this means.

> It could be a big deal (might
> affect some design decisions).

Could you elaborate on this?

> I would also think about how pack v4
> fits in this (e.g. how a tree walker can still walk fast, a big
> promise of pack v4; I suppose if you still maintain "pack" concept
> over external odb then it might work). Not that it really matters.
> Pack v4 is the future, but the future can never be "today" :)

Sorry I haven't really followed pack v4 and I forgot what it is about.

Thanks,
Christian.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 1/2] builtin/apply: add 'lock_file' pointer into 'struct apply_state'

[Christian Couder]

On Fri, Jun 3, 2016 at 8:03 PM, Junio C Hamano <gits...@pobox.com> wrote:
> Christian Couder <christian.cou...@gmail.com> writes:
>
>> This is to replace:
>>
>> "[PATCH v3 48/49] builtin/apply: move 'lock_file' global into 'struct 
>> apply_state'"
>>
>> from the "libify apply and use lib in am, part 1" patch series.
>
> Thanks; will replace the tip 2 patches and requeue.
>
>> diff --git a/builtin/apply.c b/builtin/apply.c
>> index 5027f1b..cc635eb 100644
>> --- a/builtin/apply.c
>> +++ b/builtin/apply.c
>> @@ -52,6 +52,12 @@ struct apply_state {
>>   const char *prefix;
>>   int prefix_length;
>>
>> + /*
>> +  * Since lockfile.c keeps a linked list of all created
>> +  * lock_file structures, it isn't safe to free(lock_file).
>> +  */
>> + struct lock_file *lock_file;
>
> Is this even an issue for this thing anymore?  It is the
> responsibilty of the API user to manage the lifetime of what
> lock_file points at.  The caller may have it on heap and let it
> leak, or it may have in BSS in which case it won't even dream about
> freeing it.
>
> If a comment were needed for this field, it should say "when
> discarding/freeing apply_state, just discard this pointer without
> touching what the pointer points at; the storage the pointer points
> at does not belong to the API implementation but belongs to the API
> user".
>
> But I do not think such a comment is needed, because the situation
> is the same as other fields like *prefix, and for the same reason we
> do not do anything to these fields in clear_apply_state().

Ok, I just resent without this comment.

I still don't understand why there is an added:

From: Christian Couder <christian.cou...@gmail.com>

at the beginning of the emails.

> Other than that this looks great.

Thanks,
Christian.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 1/2] builtin/apply: add 'lock_file' pointer into 'struct apply_state'

[Christian Couder]

From: Christian Couder <christian.cou...@gmail.com>

We cannot have a 'struct lock_file' allocated on the stack, as lockfile.c
keeps a linked list of all created lock_file structures.

Also 'struct apply_state' users might later want the same 'struct lock_file'
instance to be reused by different series of calls to the apply api.

So let's add a 'struct lock_file *lock_file' pointer into 'struct apply_state'
and have the user of 'struct apply_state' allocate memory for the actual
'struct lock_file' instance.

Let's also add an argument to init_apply_state(), so that the caller can
easily supply a pointer to the allocated instance.

Helped-by: Eric Sunshine <sunsh...@sunshineco.com>
Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
Signed-off-by: Junio C Hamano <gits...@pobox.com>
---

This is to replace:

"[PATCH v3 48/49] builtin/apply: move 'lock_file' global into 'struct 
apply_state'"

from the "libify apply and use lib in am, part 1" patch series, and

"[PATCH v4 1/2] builtin/apply: add 'lock_file' pointer into 'struct 
apply_state'"

that was sent previously to replace v3 48/49 above.

See: http://thread.gmane.org/gmane.comp.version-control.git/296350/

The only change compared to v4 1/2 is that the comment above 'struct
lock_file *lock_file' in 'struct apply_state' has been replaced with a
much shorter one.

This 2 patch long patch series on top of the other unchanged commits
from v3 is available here:

https://github.com/chriscool/git/commits/libify-apply64

 builtin/apply.c | 16 
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 5027f1b..bbe0df1 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -52,6 +52,9 @@ struct apply_state {
const char *prefix;
int prefix_length;
 
+   /* These are lock_file related */
+   struct lock_file *lock_file;
+
/* These control what gets looked at and modified */
int apply; /* this is not a dry-run */
int cached; /* apply to the index only */
@@ -4547,7 +4550,7 @@ static int apply_patch(struct apply_state *state,
 
state->update_index = state->check_index && state->apply;
if (state->update_index && newfd < 0)
-   newfd = hold_locked_index(_file, 1);
+   newfd = hold_locked_index(state->lock_file, 1);
 
if (state->check_index) {
if (read_cache() < 0)
@@ -4648,11 +4651,14 @@ static int option_parse_directory(const struct option 
*opt,
return 0;
 }
 
-static void init_apply_state(struct apply_state *state, const char *prefix)
+static void init_apply_state(struct apply_state *state,
+const char *prefix,
+struct lock_file *lock_file)
 {
memset(state, 0, sizeof(*state));
state->prefix = prefix;
state->prefix_length = state->prefix ? strlen(state->prefix) : 0;
+   state->lock_file = lock_file;
state->apply = 1;
state->line_termination = '\n';
state->p_value = 1;
@@ -4705,6 +4711,8 @@ static void check_apply_state(struct apply_state *state, 
int force_apply)
}
if (state->check_index)
state->unsafe_paths = 0;
+   if (!state->lock_file)
+   die("BUG: state->lock_file should not be NULL");
 }
 
 static int apply_all_patches(struct apply_state *state,
@@ -4769,7 +4777,7 @@ static int apply_all_patches(struct apply_state *state,
}
 
if (state->update_index) {
-   if (write_locked_index(_index, _file, COMMIT_LOCK))
+   if (write_locked_index(_index, state->lock_file, 
COMMIT_LOCK))
die(_("Unable to write new index file"));
}
 
@@ -4852,7 +4860,7 @@ int cmd_apply(int argc, const char **argv, const char 
*prefix)
OPT_END()
};
 
-   init_apply_state(, prefix);
+   init_apply_state(, prefix, _file);
 
argc = parse_options(argc, argv, state.prefix, builtin_apply_options,
apply_usage, 0);
-- 
2.8.2.445.g4623162

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 2/2] builtin/apply: move 'newfd' global into 'struct apply_state'

[Christian Couder]

From: Christian Couder <christian.cou...@gmail.com>

To libify the apply functionality the 'newfd' variable should
not be static and global to the file. Let's move it into
'struct apply_state'.

Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
Signed-off-by: Junio C Hamano <gits...@pobox.com>
---

This is to replace:

"[PATCH v3 49/49] builtin/apply: move 'newfd' global into 'struct apply_state'"

from the "libify apply and use lib in am, part 1" patch series, and

"[PATCH v4 2/2] builtin/apply: move 'newfd' global into 'struct apply_state'"

that was sent previously to replace v3 49/49 above.

There is no change compared to both previous version except that this
patch should apply cleanly on top of PATCH v5 1/2.

 builtin/apply.c | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index bbe0df1..b3eb704 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -54,6 +54,7 @@ struct apply_state {
 
/* These are lock_file related */
struct lock_file *lock_file;
+   int newfd;
 
/* These control what gets looked at and modified */
int apply; /* this is not a dry-run */
@@ -117,8 +118,6 @@ struct apply_state {
int applied_after_fixing_ws;
 };
 
-static int newfd = -1;
-
 static const char * const apply_usage[] = {
N_("git apply [] [...]"),
NULL
@@ -4549,8 +4548,8 @@ static int apply_patch(struct apply_state *state,
state->apply = 0;
 
state->update_index = state->check_index && state->apply;
-   if (state->update_index && newfd < 0)
-   newfd = hold_locked_index(state->lock_file, 1);
+   if (state->update_index && state->newfd < 0)
+   state->newfd = hold_locked_index(state->lock_file, 1);
 
if (state->check_index) {
if (read_cache() < 0)
@@ -4659,6 +4658,7 @@ static void init_apply_state(struct apply_state *state,
state->prefix = prefix;
state->prefix_length = state->prefix ? strlen(state->prefix) : 0;
state->lock_file = lock_file;
+   state->newfd = -1;
state->apply = 1;
state->line_termination = '\n';
state->p_value = 1;
@@ -4779,6 +4779,7 @@ static int apply_all_patches(struct apply_state *state,
if (state->update_index) {
if (write_locked_index(_index, state->lock_file, 
COMMIT_LOCK))
die(_("Unable to write new index file"));
+   state->newfd = -1;
}
 
return !!errs;
-- 
2.8.2.445.g4623162

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] bisect--helper: `bisect_reset` shell function in C

[Christian Couder]

On Wed, Jun 8, 2016 at 9:59 AM, Eric Sunshine  wrote:
> On Tue, Jun 7, 2016 at 4:54 PM, Pranit Bauva  wrote:
>> Reimplement `bisect_reset` shell function in C and add a `--bisect-reset`
>> subcommand to `git bisect--helper` to call it from git-bisect.sh .
>>
>> Using `bisect_reset` subcommand is a temporary measure to port shell
>> functions to C so as to use the existing test suite. As more functions
>> are ported, this subcommand would be retired and will be called by some
>> other method.
>>
>> Note: --bisect-clean-state subcommand has not been retired as there are
>> still a function namely `bisect_start()` which still uses this
>> subcommand.
>>
>> Signed-off-by: Pranit Bauva 
>> ---
>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
>> @@ -118,12 +122,51 @@ int bisect_clean_state(void)
>> +int bisect_reset(const char *commit)
>
> s/^/static/
>
>> +{
>> +   struct strbuf branch = STRBUF_INIT;
>> +   int status = 0;
>> +
>> +   if (file_size(git_path_bisect_start()) < 1) {
>
> This doesn't even care about the size of the file, only if it
> encountered an error while stat()'ing it. Why not just use
> file_exists() instead (which you already use elsewhere in this
> function)? Alternately, if you're trying to be faithful to the shell
> code, then you *do* need to check that the file has non-zero size
> before issuing the "not bisecting" diagnostic, so:
>
> if ()
> printf("... not bisecting ...");

As file_size() returns an integer, there is no difference between
"file_size(git_path_bisect_start()) <= 0" and
"file_size(git_path_bisect_start()) < 1".
Or am I missing something?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/4] dir: introduce file_size() to check the size of file

[Christian Couder]

On Wed, Jun 8, 2016 at 10:13 AM, Eric Sunshine  wrote:
>
> I think this would be clearer if you instead added a function to
> bisect--helper.c which operates at a semantically higher level than
> what you have here (and drop this file_size() function). Specifically,
> add a function which tells you precisely what you want to know:
> whether the file exists and has non-zero size. For instance, in
> bisect--helper.c:
>
> static int file_empty_or_missing(const char *path)
> {
> struct stat st;
> return stat(...) < 0 || st.st_size == 0;
> }
>
> Or, if you have more cases where you want to know that it exists with
> non-zero size, then name it file_non_empty() and reverse the sense of
> the return value.

After a quick grep it seemed to me that there are a few places were we
stat a file just to get its size, so I suggested the file_size()
because it could help refactor other parts of the code.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] bisect--helper: `bisect_reset` shell function in C

[Christian Couder]

On Wed, Jun 8, 2016 at 11:51 AM, Christian Couder
<christian.cou...@gmail.com> wrote:
> On Wed, Jun 8, 2016 at 9:59 AM, Eric Sunshine <sunsh...@sunshineco.com> wrote:
>> On Tue, Jun 7, 2016 at 4:54 PM, Pranit Bauva <pranit.ba...@gmail.com> wrote:
>>> Reimplement `bisect_reset` shell function in C and add a `--bisect-reset`
>>> subcommand to `git bisect--helper` to call it from git-bisect.sh .
>>>
>>> Using `bisect_reset` subcommand is a temporary measure to port shell
>>> functions to C so as to use the existing test suite. As more functions
>>> are ported, this subcommand would be retired and will be called by some
>>> other method.
>>>
>>> Note: --bisect-clean-state subcommand has not been retired as there are
>>> still a function namely `bisect_start()` which still uses this
>>> subcommand.
>>>
>>> Signed-off-by: Pranit Bauva <pranit.ba...@gmail.com>
>>> ---
>>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
>>> @@ -118,12 +122,51 @@ int bisect_clean_state(void)
>>> +int bisect_reset(const char *commit)
>>
>> s/^/static/
>>
>>> +{
>>> +   struct strbuf branch = STRBUF_INIT;
>>> +   int status = 0;
>>> +
>>> +   if (file_size(git_path_bisect_start()) < 1) {
>>
>> This doesn't even care about the size of the file, only if it
>> encountered an error while stat()'ing it. Why not just use
>> file_exists() instead (which you already use elsewhere in this
>> function)? Alternately, if you're trying to be faithful to the shell
>> code, then you *do* need to check that the file has non-zero size
>> before issuing the "not bisecting" diagnostic, so:
>>
>> if ()

Ooops this was:

>> if (file_size(git_path_bisect_start()) <= 0)

but I made a copy paste mistake, sorry.

>> printf("... not bisecting ...");
>
> As file_size() returns an integer, there is no difference between
> "file_size(git_path_bisect_start()) <= 0" and
> "file_size(git_path_bisect_start()) < 1".
> Or am I missing something?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] bisect--helper: `bisect_clean_state` shell function in C

[Christian Couder]

On Wed, Jun 8, 2016 at 10:02 AM, Eric Sunshine  wrote:
> On Wed, Jun 8, 2016 at 3:46 AM, Pranit Bauva  wrote:
>> On Wed, Jun 8, 2016 at 4:01 AM, Eric Sunshine  
>> wrote:
>>> On Tue, Jun 7, 2016 at 4:54 PM, Pranit Bauva  wrote:
 +   struct string_list *refs = cb_data;
 +   char *ref = xstrfmt("refs/bisect/%s", refname);
>>>
>>> Here you're allocating a string...
>>>
 +   string_list_append(refs, ref);
 +   return 0;
 +}
 +
 +int bisect_clean_state(void)
 +{
 +   int result = 0;
 +   struct string_list refs_for_removal = STRING_LIST_INIT_DUP;
 +   for_each_ref_in("refs/bisect/", mark_for_removal, (void *) 
 _for_removal);
>>>
>>> ...and the allocated string gets inserted into a string_list which
>>> itself duplicates the string (STRING_LIST_INIT_DUP), so this is
>>> leaking the string you created with xstrfmt(), isn't it?
>>
>> Yes nice catch. I would prefer using the string_list with
>> STRING_LIST_INIT_DUP and free the ref.
>
> That's unnecessarily wasteful. Better would be to to use 
> STRING_LIST_INIT_NODUP.

In this case it is not possible to append "BISECT_HEAD" to
'refs_for_removal' before calling delete_refs() like this:

+   for_each_ref_in("refs/bisect/", mark_for_removal, (void *)
_for_removal);
+   string_list_append(_for_removal, "BISECT_HEAD");
+   result = delete_refs(_for_removal);
+   string_list_clear(_for_removal, 0);

And I think it's better to delete all the refs in the same call to
delete_refs().
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 4/4] bundle v3: the beginning

[Christian Couder]

On Wed, Jun 1, 2016 at 12:31 AM, Jeff King <p...@peff.net> wrote:
> On Fri, May 20, 2016 at 02:39:06PM +0200, Christian Couder wrote:
>
>> I wonder if this mechanism could also be used or extended to clone and
>> fetch an alternate object database.
>>
>> In [1], [2] and [3], and this was also discussed during the
>> Contributor Summit last month, Peff says that he started working on
>> alternate object database support a long time ago, and that the hard
>> part is a protocol extension to tell remotes that you can access some
>> objects in a different way.
>>
>> If a Git client would download a "$name.bndl" v3 bundle file that
>> would have a "data: $URL/alt-odb-$name.odb" extended header, the Git
>> client would just need to download "$URL/alt-odb-$name.odb" and use
>> the alternate object database support on this file.
>>
>> This way it would know all it has to know to access the objects in the
>> alternate database. The alternate object database may not contain the
>> real objects, if they are too big for example, but just files that
>> describe how to get the real objects.
>
> I'm not sure about this strategy.

I am also not sure that this is the best strategy, but I think it's
worth discussing.

> I see two complications:
>
>   1. I don't think bundles need to be a part of this "external odb"
>  strategy at all. If I understand correctly, I think you want to use
>  it as a place to stuff metadata that the server tells the client,
>  like "by the way, go here if you want another way to access some
>  objects".

Yeah, basically I think it might be possible to use the bundle
mechanism to transfer what an external ODB on the client would need to
be initialized or updated.

>  But there are lots of cases where the server might want to tell
>  the client that don't involve bundles at all.

The idea is also that anytime the server needs to send external ODB
data to the client, it would ask its own external ODB to prepare a
kind of bundle with that data and use the bundle v3 mechanism to send
it.
That may need the bundle v3 mechanism to be extended, but I don't see
in which cases it would not work.

>   2. A server pointing the client to another object store is actually
>  the least interesting bit of the protocol.
>
>  The more interesting cases (to me) are:
>
>a. The receiving side of a connection (e.g., a fetch client)
>   somehow has out-of-band access to some objects. How does it
>   tell the other side "do not bother sending me these objects; I
>   can get them in another way"?

I don't see a difference with regular objects that the fetch client
already has. If it already has some regular objects, a way to tell the
server "don't bother sending me these objects" is useful already and
it should be possible to use it to tell the server that there is no
need to send some objects stored in the external ODB too.

Also something like this is needed for shallow clones and narrow clones anyway.

>b. The receiving side of a connection has out-of-band access to
>   some objects. Some of these will be expensive to get (e.g.,
>   requiring a large download), and some may be fast (e.g.,
>   they've already been fetched to a local cache). How do we tell
>   the sending side not to assume we have cheap access to these
>   objects (e.g., for use as a delta base)?

I don't think we need to tell the sending side we have cheap access or
not to some objects.
If the objects are managed by the external ODB, it's the external ODB
on the server and on the client that will manage these objects. They
should not be used as delta bases.
Perhaps there is no mechanism to say that some objects (basically all
external ODB managed objects) should not be used as delta bases, but
that could be added.

Thanks,
Christian.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [BUG?] trailer command with multiple keys

[Christian Couder]

On Mon, Jun 6, 2016 at 2:27 PM, Michael J Gruber
 wrote:
> The command
>
> printf "body\n\ntest: foo\ntest: froz\n" | git -c
> trailer.test.key=tested -c trailer.test.command="echo by \$ARG"
> interpret-trailers
>
> gives:
>
> body
>
> tested: foo
> tested: froz
> tested: by froz
>
> I expected the command to be run on each "test" key, resulting in the
> output:
>
> body:
>
> tested: by foo
> tested: by froz
>
> (In a real life scenario, I would use ifexists replace.)[*]
>
> Maybe my expectation is wrong?

I wouldn't say that it is wrong, but for now trailer configuration
applies mostly to trailer passed as argument to `git
interpret-trailers`.

So you could perhaps get something close to what you want with:


$ printf "body\n\n" | git -c trailer.test.key=tested -c
trailer.test.command="/home/christian/git/test/interpret-trailers/trailer-script.sh
\$ARG"  interpret-trailers --trim-empty --trailer='test: foo'
--trailer='test: frotz'
body

tested: by foo
tested: by frotz


and:


$ cat /home/christian/git/test/interpret-trailers/trailer-script.sh
#!/bin/sh
test -n "$1" && echo "by $1"
:


> The code breaks out of the loop after the
> first matching in_tok, apparently intentionally so. But I'm not sure -
> the key is replaced for both instances.
>
> Simply replacing that "return 1" by a "ret = 1" etc. runs into problems
> with the way the freeing of in_tok and arg_tok is arranged there :|
>
> Basically, I expected the trailer command to work "grep/sed-like" on all
> key value pairs that have matching keys, passing the value to the
> command, and using the (each) command's output as the new value for each
> of these pairs.

Yeah, it could have been designed like that.

The problem is that something like:

   $ git config trailer.sign.key "Signed-off-by: "
   $ git config trailer.sign.ifmissing add
   $ git config trailer.sign.ifexists doNothing
   $ git config trailer.sign.command 'echo "$(git config
user.name) <$(git config user.email)>"'

and piping any commit message into "git interpret-trailers" should work too.

In short it's not very natural to have both of the following by default:

- a configured command should run once to get a chance to add a new
trailer, even it doesn't exist in the input file
- a configured command should run once per trailer in the input file

(especially because as I said above for now configuration applies
mostly to trailers on the command line).

One solution could be to add support for a new
trailer..commandMode config option that could take values like
"onceAnyway", "oncePerMatchingTrailerInInputFile",
"oncePerMatchingTrailerOnCommandLine" and it should be possible to use
one or more of those modes, like for example:

trailer.stuff.commandMode=onceAnyway,oncePerMatchingTrailerInInputFile

> [*] My prime use case: fill in reported-by etc. with short author names,
> completed the same way we complete --author=jun using a trailer command
> (interpret-trailers in the commit-msg hook):
>
> $ git help author
> `git author' is aliased to `!f() { a=$(git log -1 --all -i --format="%aN
> <%aE>" --author "$1"); echo ${a:-$1}; }; f'
>
> $ cat .git/hooks/commit-msg
> #!/bin/sh
> git interpret-trailers --in-place "$1"
>
> $ git config --get-regexp trailer
> trailer.report.key Reported-by
> trailer.report.command git author '$ARG'
> trailer.report.ifexists replace
> trailer.report.ifmissing doNothing

Yeah, it would be nice to be able to have things like that.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 4/4] bundle v3: the beginning

[Christian Couder]

On Wed, Jun 1, 2016 at 3:37 PM, Duy Nguyen <pclo...@gmail.com> wrote:
> On Tue, May 31, 2016 at 8:18 PM, Christian Couder
> <christian.cou...@gmail.com> wrote:
>>>> I wonder if this mechanism could also be used or extended to clone and
>>>> fetch an alternate object database.
>>>>
>>>> In [1], [2] and [3], and this was also discussed during the
>>>> Contributor Summit last month, Peff says that he started working on
>>>> alternate object database support a long time ago, and that the hard
>>>> part is a protocol extension to tell remotes that you can access some
>>>> objects in a different way.
>>>>
>>>> If a Git client would download a "$name.bndl" v3 bundle file that
>>>> would have a "data: $URL/alt-odb-$name.odb" extended header, the Git
>>>> client would just need to download "$URL/alt-odb-$name.odb" and use
>>>> the alternate object database support on this file.
>>>
>>> What does this file contain exactly? A list of SHA-1 that can be
>>> retrieved from this remote/alternate odb?
>>
>> It would depend on the external odb. Git could support different
>> external odb that have different trade-offs.
>>
>>> I wonder if we could just
>>> git-replace for this marking. The replaced content could contain the
>>> uri pointing to the alt odb.
>>
>> Yeah, interesting!
>> That's indeed another possibility that might not need the transfer of
>> any external odb.
>>
>> But in this case it might be cleaner to just have a separate ref hierarchy 
>> like:
>>
>> refs/external-odbs/my-ext-odb/
>>
>> instead of using the replace one.
>>
>> Or maybe:
>>
>> refs/replace/external-odbs/my-ext-odb/
>>
>> if we really want to use the replace hierarchy.
>
> Yep. replace hierarchy crossed my mind. But then I thought about
> performance degradation when there are more than one pack (we have to
> search through them all for every SHA-1) and discarded it because we
> would need to do the same linear search here. I guess we will most
> likely have one or two name spaces so it probably won't matter.

Yeah.

>>> We could optionally contact alt odb to
>>> retrieve real content, or just show the replaced/fake data when alt
>>> odb is out of reach.
>>
>> Yeah, I wonder if that really needs the replace mechanism.
>
> Replace mechanism provides good hook point. But it really depends how
> invasive this remote odb is. If a fake content is enough to avoid
> breakages up high, git-replace is enough. If you really need to pass
> remote odb info up so higher levels can do something more fancy, then
> it's insufficient.
>
>> By the way this makes me wonder if we could implement resumable clone
>> using some kind of replace ref.
>>
>> The client while cloning nearly as usual would download one or more
>> special replace refs that would points to objects with links to
>> download bundles using standard protocols.
>> Just after the clone, the client would read these objects and download
>> the bundles from these objects.
>> And then it would clone from these bundles.
>
> I thought we have settled on resumable clone, just waiting for an
> implementation :) Doing it your way, you would need to download these
> special objects too (in a pack?) and come back download some more
> bundles. It would be more efficient to show the bundle uri early and
> go download the bundle on the side while you go on to get the
> addition/smaller pack that contains the rest.

Yeah, something like the bundle v3 mechanism is probably more efficient.

Thanks,
Christian.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 41/44] am: use be_silent in 'struct apply_state' to shut up applying patches

[Christian Couder]

On Sat, Jun 11, 2016 at 12:07 AM, Junio C Hamano  wrote:
> The update in 33/44 to make am call into apply that is not ready to
> be called (e.g. the caller needs the dup(2) dance with /dev/null to
> be silent) gets finally corrected with this step, which makes the
> progress of the topic somewhat ugly and reviewing it a bit harder
> than necessary.  As it stands, the last several patches in the
> series smells more like "oops, we realize these things were not done
> properly the first time, and here are the follow-up patches to fix
> them up".
>
> I wonder if it is a good idea to delay integrating the apply
> machinery into "am" until it is ready?

Ok, I will do that and I think it should also avoid the build failures
on Windows.

Thanks,
Christian.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 43/44] builtin/apply: add a cli option for be_silent

[Christian Couder]

On Fri, Jun 10, 2016 at 10:59 PM, René Scharfe <l@web.de> wrote:
> Am 10.06.2016 um 22:11 schrieb Christian Couder:
>>
>> Let's make it possible to request a silent operation on the
>> command line.
>>
>> Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
>> ---
>>   builtin/apply.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/builtin/apply.c b/builtin/apply.c
>> index ddd61de..93744f8 100644
>> --- a/builtin/apply.c
>> +++ b/builtin/apply.c
>> @@ -74,6 +74,8 @@ int cmd_apply(int argc, const char **argv, const char
>> *prefix)
>> OPT_BOOL(0, "allow-overlap", _overlap,
>> N_("allow overlapping hunks")),
>> OPT__VERBOSE(_verbosely, N_("be verbose")),
>> +   OPT_BOOL(0, "silent", _silent,
>> +   N_("do not print any output")),
>> OPT_BIT(0, "inaccurate-eof", ,
>> N_("tolerate incorrectly detected missing new-line
>> at the end of file"),
>> APPLY_OPT_INACCURATE_EOF),
>
> Why not -q/--quiet as for most other commands?

First as I say in the cover letter, I am going to discard this patch.
That is because it is not necessary, and it appeared a bit
controversial whether I should use -q/--quiet or not in the v2
discussions.

> Furthermore, you could use OPT__VERBOSITY, which causes -v and -q to update
> the same variable variable and thus lets parseopt handle their interaction.
> Perhaps verbosity == 1 could mean verbose, 0 normal, -1 no infos, -2 no
> warnings and -3 no errors?

Yeah, that could be done.

> And if you add the ability to silence the apply functions before using them
> you don't have to export and unexport dup_devnull().

Yeah, I will do something like that in the next version.

Thanks,
Christian.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 31/44] run-command: make dup_devnull() non static

[Christian Couder]

On Sat, Jun 11, 2016 at 10:17 AM, Johannes Sixt <j...@kdbg.org> wrote:
> Am 10.06.2016 um 22:11 schrieb Christian Couder:
>>
>> --- a/run-command.c
>> +++ b/run-command.c
>> @@ -85,7 +85,7 @@ static inline void close_pair(int fd[2])
>>   }
>>
>>   #ifndef GIT_WINDOWS_NATIVE
>> -static inline void dup_devnull(int to)
>> +void dup_devnull(int to)
>
>
> I'm not adding arguments to what we've heard on whether to use /dev/null in
> this series or not. But if the outcome is to keep this patch, please remove
> the #ifndef that we see in the context lines (and the matching #endif), too.
> Otherwise, the build fails on Windows for each patch in the series until
> this change is reverted in patch 42/44.

Ok, I will find a way to avoid that build failure, though I don't test
on Windows.

Thanks,
Christian.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 00/94] libify apply and use lib in am

[Christian Couder]

On Fri, Jun 10, 2016 at 7:04 PM, Johannes Sixt  wrote:
> Am 10.06.2016 um 13:11 schrieb Johannes Schindelin:
>>
>> Not really. The reply (which I had not quite connected with my mail
>> because they were over a week apart) says this:
>>
>>> I fixed this by moving the "close(fd)" call just after the
>>> "apply_patch()" call.
>
> This bug in v1 was discovered by the test suite and fixed in v2.
>
>> and this:
>>
>>> I will have another look at the 2 other places where there are
>>> open()/close() or fopen()/fclose() calls.
>>
>> but nothing about a careful, systematic investigation of all error code
>> paths. As a consequence, I fully expect to encounter test failures as soon
>> as I test your patch series again, simply because resources are still in
>> use when they should no longer be used. In other words, my expectations
>> are now lower than they have been before, my concerns are not at all
>> addressed.
>
> Do you trust the test suite to some degree? It passes after the above bug
> was fixed in v2. In addition, haven't found any problems so far during daily
> use.
>
>>> This is the newest iteration:
>>>
>>> https://github.com/chriscool/git/commits/libify-apply-use-in-am65
>>
>> And that cute 65 in the name is the revision.
>
> Yeah, that number is painful. I would appreciate an unversioned branch name,
> too.

Ok, you can use
https://github.com/chriscool/git/commits/libify-apply-use-in-am then.

Thanks,
Christian.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v6 11/44] builtin/apply: make check_apply_state() return -1 instead of die()ing

[Christian Couder]

To libify `git apply` functionality we have to signal errors to the
caller instead of die()ing.

To do that in a compatible manner with the rest of the error handling
in "builtin/apply.c", check_apply_state() should return -1 instead of
calling die().

Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
---
 builtin/apply.c | 19 +++
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 2ae1243..d60ffce 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -4541,17 +4541,17 @@ static int option_parse_directory(const struct option 
*opt,
return 0;
 }
 
-static void check_apply_state(struct apply_state *state, int force_apply)
+static int check_apply_state(struct apply_state *state, int force_apply)
 {
int is_not_gitdir = !startup_info->have_repository;
 
if (state->apply_with_reject && state->threeway)
-   die("--reject and --3way cannot be used together.");
+   return error("--reject and --3way cannot be used together.");
if (state->cached && state->threeway)
-   die("--cached and --3way cannot be used together.");
+   return error("--cached and --3way cannot be used together.");
if (state->threeway) {
if (is_not_gitdir)
-   die(_("--3way outside a repository"));
+   return error(_("--3way outside a repository"));
state->check_index = 1;
}
if (state->apply_with_reject)
@@ -4559,16 +4559,18 @@ static void check_apply_state(struct apply_state 
*state, int force_apply)
if (!force_apply && (state->diffstat || state->numstat || 
state->summary || state->check || state->fake_ancestor))
state->apply = 0;
if (state->check_index && is_not_gitdir)
-   die(_("--index outside a repository"));
+   return error(_("--index outside a repository"));
if (state->cached) {
if (is_not_gitdir)
-   die(_("--cached outside a repository"));
+   return error(_("--cached outside a repository"));
state->check_index = 1;
}
if (state->check_index)
state->unsafe_paths = 0;
if (!state->lock_file)
-   die("BUG: state->lock_file should not be NULL");
+   return error("BUG: state->lock_file should not be NULL");
+
+   return 0;
 }
 
 static int apply_all_patches(struct apply_state *state,
@@ -4734,7 +4736,8 @@ int cmd_apply(int argc, const char **argv, const char 
*prefix)
argc = parse_options(argc, argv, state.prefix, builtin_apply_options,
apply_usage, 0);
 
-   check_apply_state(, force_apply);
+   if (check_apply_state(, force_apply))
+   exit(1);
 
ret = apply_all_patches(, argc, argv, options);
 
-- 
2.9.0.rc2.362.g3cd93d0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v6 33/44] builtin/am: use apply api in run_apply()

[Christian Couder]

This replaces run_apply() implementation with a new one that
uses the apply api that has been previously prepared in
apply.c and apply.h.

This shoud improve performance a lot in certain cases.

As the previous implementation was creating a new `git apply`
process to apply each patch, it could be slow on systems like
Windows where it is costly to create new processes.

Also the new `git apply` process had to read the index from
disk, and when the process was done the calling process
discarded its own index and read back from disk the new
index that had been created by the `git apply` process.

This could be very inefficient with big repositories that
have big index files, especially when the system decided
that it was a good idea to run the `git apply` processes on
a different processor core.

Also eliminating index reads enables further performance
improvements by using:

`git update-index --split-index`

For example here is a benchmark of a multi hundred commit
rebase on the Linux kernel on a Debian laptop with SSD:

command: git rebase --onto 1993b17 52bef0c 29dde7c

Vanilla "next" without split index:1m54.953s
Vanilla "next" with split index:   1m22.476s
This series on top of "next" without split index:  1m12.034s
This series on top of "next" with split index: 0m15.678s

(using branch "next" from mid April 2016.)

Benchmarked-by: Ævar Arnfjörð Bjarmason <ava...@gmail.com>
Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
---
 builtin/am.c | 104 ---
 1 file changed, 86 insertions(+), 18 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 3dfe70b..a16b06c 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -28,6 +28,7 @@
 #include "rerere.h"
 #include "prompt.h"
 #include "mailinfo.h"
+#include "apply.h"
 
 /**
  * Returns 1 if the file is empty or does not exist, 0 otherwise.
@@ -1521,39 +1522,106 @@ static int parse_mail_rebase(struct am_state *state, 
const char *mail)
  */
 static int run_apply(const struct am_state *state, const char *index_file)
 {
-   struct child_process cp = CHILD_PROCESS_INIT;
-
-   cp.git_cmd = 1;
-
-   if (index_file)
-   argv_array_pushf(_array, "GIT_INDEX_FILE=%s", 
index_file);
+   struct argv_array apply_paths = ARGV_ARRAY_INIT;
+   struct argv_array apply_opts = ARGV_ARRAY_INIT;
+   struct apply_state apply_state;
+   int save_stdout_fd, save_stderr_fd;
+   int res, opts_left;
+   char *save_index_file;
+   static struct lock_file lock_file;
+
+   struct option am_apply_options[] = {
+   { OPTION_CALLBACK, 0, "whitespace", _state, N_("action"),
+   N_("detect new or modified lines that have whitespace 
errors"),
+   0, apply_option_parse_whitespace },
+   { OPTION_CALLBACK, 0, "ignore-space-change", _state, NULL,
+   N_("ignore changes in whitespace when finding context"),
+   PARSE_OPT_NOARG, apply_option_parse_space_change },
+   { OPTION_CALLBACK, 0, "ignore-whitespace", _state, NULL,
+   N_("ignore changes in whitespace when finding context"),
+   PARSE_OPT_NOARG, apply_option_parse_space_change },
+   { OPTION_CALLBACK, 0, "directory", _state, N_("root"),
+   N_("prepend  to all filenames"),
+   0, apply_option_parse_directory },
+   { OPTION_CALLBACK, 0, "exclude", _state, N_("path"),
+   N_("don't apply changes matching the given path"),
+   0, apply_option_parse_exclude },
+   { OPTION_CALLBACK, 0, "include", _state, N_("path"),
+   N_("apply changes matching the given path"),
+   0, apply_option_parse_include },
+   OPT_INTEGER('C', NULL, _state.p_context,
+   N_("ensure at least  lines of context 
match")),
+   { OPTION_CALLBACK, 'p', NULL, _state, N_("num"),
+   N_("remove  leading slashes from traditional diff 
paths"),
+   0, apply_option_parse_p },
+   OPT_BOOL(0, "reject", _state.apply_with_reject,
+   N_("leave the rejected hunks in corresponding *.rej 
files")),
+   OPT_END()
+   };
 
/*
 * If we are allowed to fall back on 3-way merge, don't give false
 * errors during the initial attempt.
 */
+
if (state->threeway && !index_file) {
-   cp.no_stdout = 1;
-

[PATCH v6 37/44] apply: don't print on stdout when be_silent is set

[Christian Couder]

Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
---
 apply.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/apply.c b/apply.c
index dd9b301..2529534 100644
--- a/apply.c
+++ b/apply.c
@@ -4679,13 +4679,13 @@ static int apply_patch(struct apply_state *state,
goto end;
}
 
-   if (state->diffstat)
+   if (state->diffstat && !state->be_silent)
stat_patch_list(state, list);
 
-   if (state->numstat)
+   if (state->numstat && !state->be_silent)
numstat_patch_list(state, list);
 
-   if (state->summary)
+   if (state->summary && !state->be_silent)
summary_patch_list(list);
 
 end:
-- 
2.9.0.rc2.362.g3cd93d0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v6 34/44] write_or_die: use warning() instead of fprintf(stderr, ...)

[Christian Couder]

Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
---
 write_or_die.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/write_or_die.c b/write_or_die.c
index 49e80aa..c29f677 100644
--- a/write_or_die.c
+++ b/write_or_die.c
@@ -87,8 +87,7 @@ int write_or_whine_pipe(int fd, const void *buf, size_t 
count, const char *msg)
 {
if (write_in_full(fd, buf, count) < 0) {
check_pipe(errno);
-   fprintf(stderr, "%s: write error (%s)\n",
-   msg, strerror(errno));
+   warning("%s: write error (%s)\n", msg, strerror(errno));
return 0;
}
 
@@ -98,8 +97,7 @@ int write_or_whine_pipe(int fd, const void *buf, size_t 
count, const char *msg)
 int write_or_whine(int fd, const void *buf, size_t count, const char *msg)
 {
if (write_in_full(fd, buf, count) < 0) {
-   fprintf(stderr, "%s: write error (%s)\n",
-   msg, strerror(errno));
+   warning("%s: write error (%s)\n", msg, strerror(errno));
return 0;
}
 
-- 
2.9.0.rc2.362.g3cd93d0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v6 38/44] usage: add set_warn_routine()

[Christian Couder]

There are already set_die_routine() and set_error_routine(),
so let's add set_warn_routine() as this will be needed in a
following commit.

Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
---
 git-compat-util.h | 1 +
 usage.c   | 5 +
 2 files changed, 6 insertions(+)

diff --git a/git-compat-util.h b/git-compat-util.h
index 49d4029..f78706a 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -440,6 +440,7 @@ static inline int const_error(void)
 
 extern void set_die_routine(NORETURN_PTR void (*routine)(const char *err, 
va_list params));
 extern void set_error_routine(void (*routine)(const char *err, va_list 
params));
+extern void set_warn_routine(void (*routine)(const char *warn, va_list 
params));
 extern void set_die_is_recursing_routine(int (*routine)(void));
 extern void set_error_handle(FILE *);
 
diff --git a/usage.c b/usage.c
index 1dad03f..67e5526 100644
--- a/usage.c
+++ b/usage.c
@@ -70,6 +70,11 @@ void set_error_routine(void (*routine)(const char *err, 
va_list params))
error_routine = routine;
 }
 
+void set_warn_routine(void (*routine)(const char *warn, va_list params))
+{
+   warn_routine = routine;
+}
+
 void set_die_is_recursing_routine(int (*routine)(void))
 {
die_is_recursing = routine;
-- 
2.9.0.rc2.362.g3cd93d0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v6 32/44] environment: add set_index_file()

[Christian Couder]

Introduce set_index_file() to be able to temporarily change the index file.

It should be used like this:

/* Save current index file */
old_index_file = get_index_file();
set_index_file((char *)tmp_index_file);

/* Do stuff that will use tmp_index_file as the index file */
...

/* When finished reset the index file */
set_index_file(old_index_file);

Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
---
 cache.h   |  1 +
 environment.c | 10 ++
 2 files changed, 11 insertions(+)

diff --git a/cache.h b/cache.h
index 81d4ac3..28fc0bf 100644
--- a/cache.h
+++ b/cache.h
@@ -461,6 +461,7 @@ extern int is_inside_work_tree(void);
 extern const char *get_git_dir(void);
 extern const char *get_git_common_dir(void);
 extern char *get_object_directory(void);
+extern void set_index_file(char *index_file);
 extern char *get_index_file(void);
 extern char *get_graft_file(void);
 extern int set_git_dir(const char *path);
diff --git a/environment.c b/environment.c
index ca72464..7a53799 100644
--- a/environment.c
+++ b/environment.c
@@ -292,6 +292,16 @@ int odb_pack_keep(char *name, size_t namesz, const 
unsigned char *sha1)
return open(name, O_RDWR|O_CREAT|O_EXCL, 0600);
 }
 
+/*
+ * Temporarily change the index file.
+ * Please save the current index file using get_index_file() before changing
+ * the index file. And when finished, reset it to the saved value.
+ */
+void set_index_file(char *index_file)
+{
+   git_index_file = index_file;
+}
+
 char *get_index_file(void)
 {
if (!git_index_file)
-- 
2.9.0.rc2.362.g3cd93d0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v6 43/44] builtin/apply: add a cli option for be_silent

[Christian Couder]

Let's make it possible to request a silent operation on the
command line.

Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
---
 builtin/apply.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/builtin/apply.c b/builtin/apply.c
index ddd61de..93744f8 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -74,6 +74,8 @@ int cmd_apply(int argc, const char **argv, const char *prefix)
OPT_BOOL(0, "allow-overlap", _overlap,
N_("allow overlapping hunks")),
OPT__VERBOSE(_verbosely, N_("be verbose")),
+   OPT_BOOL(0, "silent", _silent,
+   N_("do not print any output")),
OPT_BIT(0, "inaccurate-eof", ,
N_("tolerate incorrectly detected missing new-line at 
the end of file"),
APPLY_OPT_INACCURATE_EOF),
-- 
2.9.0.rc2.362.g3cd93d0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v6 19/44] builtin/apply: make remove_file() return -1 on error

[Christian Couder]

To libify `git apply` functionality we have to signal errors to the
caller instead of die()ing.

To do that in a compatible manner with the rest of the error handling
in "builtin/apply.c", remove_file() should return -1 instead of
calling die().

Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
---
 builtin/apply.c | 17 +++--
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index e74b068..694c65b 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -4074,17 +4074,18 @@ static void patch_stats(struct apply_state *state, 
struct patch *patch)
}
 }
 
-static void remove_file(struct apply_state *state, struct patch *patch, int 
rmdir_empty)
+static int remove_file(struct apply_state *state, struct patch *patch, int 
rmdir_empty)
 {
if (state->update_index) {
if (remove_file_from_cache(patch->old_name) < 0)
-   die(_("unable to remove %s from index"), 
patch->old_name);
+   return error(_("unable to remove %s from index"), 
patch->old_name);
}
if (!state->cached) {
if (!remove_or_warn(patch->old_mode, patch->old_name) && 
rmdir_empty) {
remove_path(patch->old_name);
}
}
+   return 0;
 }
 
 static void add_index_file(struct apply_state *state,
@@ -4263,8 +4264,10 @@ static void write_out_one_result(struct apply_state 
*state,
 int phase)
 {
if (patch->is_delete > 0) {
-   if (phase == 0)
-   remove_file(state, patch, 1);
+   if (phase == 0) {
+   if (remove_file(state, patch, 1))
+   exit(1);
+   }
return;
}
if (patch->is_new > 0 || patch->is_copy) {
@@ -4276,8 +4279,10 @@ static void write_out_one_result(struct apply_state 
*state,
 * Rename or modification boils down to the same
 * thing: remove the old, write the new
 */
-   if (phase == 0)
-   remove_file(state, patch, patch->is_rename);
+   if (phase == 0) {
+   if (remove_file(state, patch, patch->is_rename))
+   exit(1);
+   }
if (phase == 1)
create_file(state, patch);
 }
-- 
2.9.0.rc2.362.g3cd93d0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v6 44/44] apply: use error_errno() where possible

[Christian Couder]

Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
---
 apply.c | 22 +++---
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/apply.c b/apply.c
index ef49709..cd4cd01 100644
--- a/apply.c
+++ b/apply.c
@@ -3505,7 +3505,7 @@ static int load_current(struct apply_state *state,
ce = active_cache[pos];
if (lstat(name, )) {
if (errno != ENOENT)
-   return error(_("%s: %s"), name, strerror(errno));
+   return error_errno("%s", name);
if (checkout_target(_index, ce, ))
return -1;
}
@@ -3664,7 +3664,7 @@ static int check_preimage(struct apply_state *state,
} else if (!state->cached) {
stat_ret = lstat(old_name, st);
if (stat_ret && errno != ENOENT)
-   return error(_("%s: %s"), old_name, strerror(errno));
+   return error_errno("%s", old_name);
}
 
if (state->check_index && !previous) {
@@ -3686,7 +3686,7 @@ static int check_preimage(struct apply_state *state,
} else if (stat_ret < 0) {
if (patch->is_new < 0)
goto is_new;
-   return error(_("%s: %s"), old_name, strerror(errno));
+   return error_errno("%s", old_name);
}
 
if (!state->cached && !previous)
@@ -3745,7 +3745,7 @@ static int check_to_create(struct apply_state *state,
 
return EXISTS_IN_WORKTREE;
} else if ((errno != ENOENT) && (errno != ENOTDIR)) {
-   return error("%s: %s", new_name, strerror(errno));
+   return error_errno("%s", new_name);
}
return 0;
 }
@@ -4260,9 +4260,9 @@ static int add_index_file(struct apply_state *state,
if (!state->cached) {
if (lstat(path, ) < 0) {
free(ce);
-   return error(_("unable to stat newly "
-  "created file '%s': %s"),
-path, strerror(errno));
+   return error_errno(_("unable to stat newly "
+"created file '%s'"),
+  path);
}
fill_stat_cache_info(ce, );
}
@@ -4316,7 +4316,7 @@ static int try_create_file(const char *path, unsigned int 
mode, const char *buf,
strbuf_release();
 
if (close(fd) < 0 && !res)
-   return error(_("closing file '%s': %s"), path, strerror(errno));
+   return error_errno(_("closing file '%s'"), path);
 
return res ? -1 : 0;
 }
@@ -4386,8 +4386,8 @@ static int create_one_file(struct apply_state *state,
++nr;
}
}
-   return error(_("unable to write file '%s' mode %o: %s"),
-path, mode, strerror(errno));
+   return error_errno(_("unable to write file '%s' mode %o: %s"),
+  path, mode);
 }
 
 static int add_conflicted_stages_file(struct apply_state *state,
@@ -4514,7 +4514,7 @@ static int write_out_one_reject(struct apply_state 
*state, struct patch *patch)
 
rej = fopen(namebuf, "w");
if (!rej)
-   return error(_("cannot open %s: %s"), namebuf, strerror(errno));
+   return error_errno(_("cannot open %s"), namebuf);
 
/* Normal git tools never deal with .rej, so do not pretend
 * this is a git patch by saying --git or giving extended
-- 
2.9.0.rc2.362.g3cd93d0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v6 09/44] builtin/apply: move init_apply_state() to apply.c

[Christian Couder]

To libify `git apply` functionality we must make init_apply_state()
usable outside "builtin/apply.c".

Let's do that by moving it into a new "apply.c".

Helped-by: Eric Sunshine <sunsh...@sunshineco.com>
Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
---
 Makefile|  1 +
 apply.c | 91 +
 apply.h | 10 +++
 builtin/apply.c | 88 ---
 4 files changed, 102 insertions(+), 88 deletions(-)
 create mode 100644 apply.c

diff --git a/Makefile b/Makefile
index 6b05249..6da1209 100644
--- a/Makefile
+++ b/Makefile
@@ -680,6 +680,7 @@ LIB_OBJS += abspath.o
 LIB_OBJS += advice.o
 LIB_OBJS += alias.o
 LIB_OBJS += alloc.o
+LIB_OBJS += apply.o
 LIB_OBJS += archive.o
 LIB_OBJS += archive-tar.o
 LIB_OBJS += archive-zip.o
diff --git a/apply.c b/apply.c
new file mode 100644
index 000..1f31bb4
--- /dev/null
+++ b/apply.c
@@ -0,0 +1,91 @@
+#include "cache.h"
+#include "lockfile.h"
+#include "apply.h"
+
+static void git_apply_config(void)
+{
+   git_config_get_string_const("apply.whitespace", 
_default_whitespace);
+   git_config_get_string_const("apply.ignorewhitespace", 
_default_ignorewhitespace);
+   git_config(git_default_config, NULL);
+}
+
+int parse_whitespace_option(struct apply_state *state, const char *option)
+{
+   if (!option) {
+   state->ws_error_action = warn_on_ws_error;
+   return 0;
+   }
+   if (!strcmp(option, "warn")) {
+   state->ws_error_action = warn_on_ws_error;
+   return 0;
+   }
+   if (!strcmp(option, "nowarn")) {
+   state->ws_error_action = nowarn_ws_error;
+   return 0;
+   }
+   if (!strcmp(option, "error")) {
+   state->ws_error_action = die_on_ws_error;
+   return 0;
+   }
+   if (!strcmp(option, "error-all")) {
+   state->ws_error_action = die_on_ws_error;
+   state->squelch_whitespace_errors = 0;
+   return 0;
+   }
+   if (!strcmp(option, "strip") || !strcmp(option, "fix")) {
+   state->ws_error_action = correct_ws_error;
+   return 0;
+   }
+   return error(_("unrecognized whitespace option '%s'"), option);
+}
+
+int parse_ignorewhitespace_option(struct apply_state *state,
+ const char *option)
+{
+   if (!option || !strcmp(option, "no") ||
+   !strcmp(option, "false") || !strcmp(option, "never") ||
+   !strcmp(option, "none")) {
+   state->ws_ignore_action = ignore_ws_none;
+   return 0;
+   }
+   if (!strcmp(option, "change")) {
+   state->ws_ignore_action = ignore_ws_change;
+   return 0;
+   }
+   return error(_("unrecognized whitespace ignore option '%s'"), option);
+}
+
+void init_apply_state(struct apply_state *state,
+ const char *prefix,
+ struct lock_file *lock_file)
+{
+   memset(state, 0, sizeof(*state));
+   state->prefix = prefix;
+   state->prefix_length = state->prefix ? strlen(state->prefix) : 0;
+   state->lock_file = lock_file;
+   state->newfd = -1;
+   state->apply = 1;
+   state->line_termination = '\n';
+   state->p_value = 1;
+   state->p_context = UINT_MAX;
+   state->squelch_whitespace_errors = 5;
+   state->ws_error_action = warn_on_ws_error;
+   state->ws_ignore_action = ignore_ws_none;
+   state->linenr = 1;
+   strbuf_init(>root, 0);
+
+   git_apply_config();
+   if (apply_default_whitespace && parse_whitespace_option(state, 
apply_default_whitespace))
+   exit(1);
+   if (apply_default_ignorewhitespace && 
parse_ignorewhitespace_option(state, apply_default_ignorewhitespace))
+   exit(1);
+}
+
+void clear_apply_state(struct apply_state *state)
+{
+   string_list_clear(>limit_by_name, 0);
+   string_list_clear(>symlink_changes, 0);
+   strbuf_release(>root);
+
+   /* >fn_table is cleared at the end of apply_patch() */
+}
diff --git a/apply.h b/apply.h
index 9a98eae..77502be 100644
--- a/apply.h
+++ b/apply.h
@@ -97,4 +97,14 @@ struct apply_state {
int applied_after_fixing_ws;
 };
 
+extern int parse_whitespace_option(struct apply_state *state,
+  const char *option);
+extern int parse_ignorewhitespace_option(struct apply_state *state,
+const char *option);
+
+extern void init_apply_state(struct apply_state *state,
+c

[PATCH v6 13/44] builtin/apply: make apply_all_patches() return -1 on error

[Christian Couder]

To finish libifying the apply functionality, apply_all_patches() should not
die() or exit() in case of error, but return -1.

While doing that we must take care that file descriptors are properly closed
and, if needed, reset a sensible value.

Also, according to the lockfile API, when finished with a lockfile, one
should either commit it or roll it back.

This is even more important now that the same lockfile can be passed
to init_apply_state() many times to be reused by series of calls to
the apply lib functions.

Helped-by: Eric Sunshine <sunsh...@sunshineco.com>
Helped-by: Nguyễn Thái Ngọc Duy <pclo...@gmail.com>
Helped-by: Johannes Schindelin <johannes.schinde...@gmx.de>
Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
---
 builtin/apply.c | 40 +++-
 1 file changed, 27 insertions(+), 13 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index a27fdd3..c27be35 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -4558,7 +4558,7 @@ static int apply_all_patches(struct apply_state *state,
if (!strcmp(arg, "-")) {
res = apply_patch(state, 0, "", options);
if (res < 0)
-   exit(1);
+   goto rollback_end;
errs |= res;
read_stdin = 0;
continue;
@@ -4568,21 +4568,23 @@ static int apply_all_patches(struct apply_state *state,
  arg);
 
fd = open(arg, O_RDONLY);
-   if (fd < 0)
-   die_errno(_("can't open patch '%s'"), arg);
+   if (fd < 0) {
+   error(_("can't open patch '%s': %s"), arg, 
strerror(errno));
+   goto rollback_end;
+   }
read_stdin = 0;
set_default_whitespace_mode(state);
res = apply_patch(state, fd, arg, options);
+   close(fd);
if (res < 0)
-   exit(1);
+   goto rollback_end;
errs |= res;
-   close(fd);
}
set_default_whitespace_mode(state);
if (read_stdin) {
res = apply_patch(state, 0, "", options);
if (res < 0)
-   exit(1);
+   goto rollback_end;
errs |= res;
}
 
@@ -4596,11 +4598,13 @@ static int apply_all_patches(struct apply_state *state,
   squelched),
squelched);
}
-   if (state->ws_error_action == die_on_ws_error)
-   die(Q_("%d line adds whitespace errors.",
-  "%d lines add whitespace errors.",
-  state->whitespace_error),
-   state->whitespace_error);
+   if (state->ws_error_action == die_on_ws_error) {
+   error(Q_("%d line adds whitespace errors.",
+"%d lines add whitespace errors.",
+state->whitespace_error),
+ state->whitespace_error);
+   goto rollback_end;
+   }
if (state->applied_after_fixing_ws && state->apply)
warning("%d line%s applied after"
" fixing whitespace errors.",
@@ -4614,12 +4618,22 @@ static int apply_all_patches(struct apply_state *state,
}
 
if (state->update_index) {
-   if (write_locked_index(_index, state->lock_file, 
COMMIT_LOCK))
-   die(_("Unable to write new index file"));
+   res = write_locked_index(_index, state->lock_file, 
COMMIT_LOCK);
+   if (res) {
+   error(_("Unable to write new index file"));
+   goto rollback_end;
+   }
state->newfd = -1;
}
 
return !!errs;
+
+rollback_end:
+   if (state->newfd >= 0) {
+   rollback_lock_file(state->lock_file);
+   state->newfd = -1;
+   }
+   return -1;
 }
 
 int cmd_apply(int argc, const char **argv, const char *prefix)
-- 
2.9.0.rc2.362.g3cd93d0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v6 20/44] builtin/apply: make add_conflicted_stages_file() return -1 on error

[Christian Couder]

To libify `git apply` functionality we have to signal errors to the
caller instead of die()ing.

To do that in a compatible manner with the rest of the error handling
in "builtin/apply.c", add_conflicted_stages_file() should return -1
instead of calling die().

Helped-by: Eric Sunshine <sunsh...@sunshineco.com>
Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
---
 builtin/apply.c | 20 +---
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 694c65b..0997384 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -4213,7 +4213,7 @@ static void create_one_file(struct apply_state *state,
die_errno(_("unable to write file '%s' mode %o"), path, mode);
 }
 
-static void add_conflicted_stages_file(struct apply_state *state,
+static int add_conflicted_stages_file(struct apply_state *state,
   struct patch *patch)
 {
int stage, namelen;
@@ -4221,7 +4221,7 @@ static void add_conflicted_stages_file(struct apply_state 
*state,
struct cache_entry *ce;
 
if (!state->update_index)
-   return;
+   return 0;
namelen = strlen(patch->new_name);
ce_size = cache_entry_size(namelen);
mode = patch->new_mode ? patch->new_mode : (S_IFREG | 0644);
@@ -4236,9 +4236,14 @@ static void add_conflicted_stages_file(struct 
apply_state *state,
ce->ce_flags = create_ce_flags(stage);
ce->ce_namelen = namelen;
hashcpy(ce->sha1, patch->threeway_stage[stage - 1].hash);
-   if (add_cache_entry(ce, ADD_CACHE_OK_TO_ADD) < 0)
-   die(_("unable to add cache entry for %s"), 
patch->new_name);
+   if (add_cache_entry(ce, ADD_CACHE_OK_TO_ADD) < 0) {
+   free(ce);
+   return error(_("unable to add cache entry for %s"),
+patch->new_name);
+   }
}
+
+   return 0;
 }
 
 static void create_file(struct apply_state *state, struct patch *patch)
@@ -4252,9 +4257,10 @@ static void create_file(struct apply_state *state, 
struct patch *patch)
mode = S_IFREG | 0644;
create_one_file(state, path, mode, buf, size);
 
-   if (patch->conflicted_threeway)
-   add_conflicted_stages_file(state, patch);
-   else
+   if (patch->conflicted_threeway) {
+   if (add_conflicted_stages_file(state, patch))
+   exit(1);
+   } else
add_index_file(state, path, mode, buf, size);
 }
 
-- 
2.9.0.rc2.362.g3cd93d0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v6 08/44] builtin/apply: make parse_ignorewhitespace_option() return -1 instead of die()ing

[Christian Couder]

To libify `git apply` functionality we have to signal errors to the
caller instead of die()ing.

To do that in a compatible manner with the rest of the error handling
in "builtin/apply.c", parse_ignorewhitespace_option() should return
-1 instead of calling die().

Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
---
 builtin/apply.c | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index e56e754..e2f970d 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -57,20 +57,20 @@ static int parse_whitespace_option(struct apply_state 
*state, const char *option
return error(_("unrecognized whitespace option '%s'"), option);
 }
 
-static void parse_ignorewhitespace_option(struct apply_state *state,
- const char *option)
+static int parse_ignorewhitespace_option(struct apply_state *state,
+const char *option)
 {
if (!option || !strcmp(option, "no") ||
!strcmp(option, "false") || !strcmp(option, "never") ||
!strcmp(option, "none")) {
state->ws_ignore_action = ignore_ws_none;
-   return;
+   return 0;
}
if (!strcmp(option, "change")) {
state->ws_ignore_action = ignore_ws_change;
-   return;
+   return 0;
}
-   die(_("unrecognized whitespace ignore option '%s'"), option);
+   return error(_("unrecognized whitespace ignore option '%s'"), option);
 }
 
 static void set_default_whitespace_mode(struct apply_state *state)
@@ -4616,8 +4616,8 @@ static void init_apply_state(struct apply_state *state,
git_apply_config();
if (apply_default_whitespace && parse_whitespace_option(state, 
apply_default_whitespace))
exit(1);
-   if (apply_default_ignorewhitespace)
-   parse_ignorewhitespace_option(state, 
apply_default_ignorewhitespace);
+   if (apply_default_ignorewhitespace && 
parse_ignorewhitespace_option(state, apply_default_ignorewhitespace))
+   exit(1);
 }
 
 static void clear_apply_state(struct apply_state *state)
-- 
2.9.0.rc2.362.g3cd93d0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v6 35/44] apply: add 'be_silent' variable to 'struct apply_state'

[Christian Couder]

This variable should prevent anything to be printed on both stderr
and stdout.

Let's not take care of stdout and apply_verbosely for now though,
as that will be taken care of in following patches.

Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
---
 apply.c | 43 +--
 apply.h |  1 +
 2 files changed, 30 insertions(+), 14 deletions(-)

diff --git a/apply.c b/apply.c
index 713d1c0..dbb2515 100644
--- a/apply.c
+++ b/apply.c
@@ -1612,8 +1612,9 @@ static void record_ws_error(struct apply_state *state,
return;
 
err = whitespace_error_string(result);
-   fprintf(stderr, "%s:%d: %s.\n%.*s\n",
-   state->patch_input_file, linenr, err, len, line);
+   if (!state->be_silent)
+   fprintf(stderr, "%s:%d: %s.\n%.*s\n",
+   state->patch_input_file, linenr, err, len, line);
free(err);
 }
 
@@ -1808,7 +1809,7 @@ static int parse_single_patch(struct apply_state *state,
return error(_("new file %s depends on old contents"), 
patch->new_name);
if (0 < patch->is_delete && newlines)
return error(_("deleted file %s still has contents"), 
patch->old_name);
-   if (!patch->is_delete && !newlines && context)
+   if (!patch->is_delete && !newlines && context && !state->be_silent)
fprintf_ln(stderr,
   _("** warning: "
 "file %s becomes empty but is not deleted"),
@@ -3031,8 +3032,8 @@ static int apply_one_fragment(struct apply_state *state,
 * Warn if it was necessary to reduce the number
 * of context lines.
 */
-   if ((leading != frag->leading) ||
-   (trailing != frag->trailing))
+   if ((leading != frag->leading ||
+trailing != frag->trailing) && !state->be_silent)
fprintf_ln(stderr, _("Context reduced to (%ld/%ld)"
 " to apply fragment at %d"),
   leading, trailing, applied_pos+1);
@@ -3529,7 +3530,8 @@ static int try_threeway(struct apply_state *state,
 read_blob_object(, pre_sha1, patch->old_mode))
return error("repository lacks the necessary blob to fall back 
on 3-way merge.");
 
-   fprintf(stderr, "Falling back to three-way merge...\n");
+   if (!state->be_silent)
+   fprintf(stderr, "Falling back to three-way merge...\n");
 
img = strbuf_detach(, );
prepare_image(_image, img, len, 1);
@@ -3559,7 +3561,9 @@ static int try_threeway(struct apply_state *state,
status = three_way_merge(image, patch->new_name,
 pre_sha1, our_sha1, post_sha1);
if (status < 0) {
-   fprintf(stderr, "Failed to fall back on three-way merge...\n");
+   if (!state->be_silent)
+   fprintf(stderr,
+   "Failed to fall back on three-way merge...\n");
return status;
}
 
@@ -3571,9 +3575,15 @@ static int try_threeway(struct apply_state *state,
hashcpy(patch->threeway_stage[0].hash, pre_sha1);
hashcpy(patch->threeway_stage[1].hash, our_sha1);
hashcpy(patch->threeway_stage[2].hash, post_sha1);
-   fprintf(stderr, "Applied patch to '%s' with conflicts.\n", 
patch->new_name);
+   if (!state->be_silent)
+   fprintf(stderr,
+   "Applied patch to '%s' with conflicts.\n",
+   patch->new_name);
} else {
-   fprintf(stderr, "Applied patch to '%s' cleanly.\n", 
patch->new_name);
+   if (!state->be_silent)
+   fprintf(stderr,
+   "Applied patch to '%s' cleanly.\n",
+   patch->new_name);
}
return 0;
 }
@@ -4472,7 +4482,8 @@ static int write_out_one_reject(struct apply_state 
*state, struct patch *patch)
"Applying patch %%s with %d rejects...",
cnt),
cnt);
-   say_patch_name(stderr, sb.buf, patch);
+   if (!state->be_silent)
+   say_patch_name(stderr, sb.buf, patch);
strbuf_release();
 
cnt = strlen(patch->new_name);
@@ -4499,10 +4510,12 @@ static int write_out_one_reject(struct apply_state 
*state, struct patch *patch)
 frag;
 cnt++, frag = frag->next) {

[PATCH v6 40/44] apply: change error_routine when be_silent is set

[Christian Couder]

Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
---
 apply.c | 23 +--
 apply.h |  4 
 2 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/apply.c b/apply.c
index 2529534..ef49709 100644
--- a/apply.c
+++ b/apply.c
@@ -109,6 +109,11 @@ void clear_apply_state(struct apply_state *state)
/* >fn_table is cleared at the end of apply_patch() */
 }
 
+static void mute_routine(const char *bla, va_list params)
+{
+   /* do nothing */
+}
+
 int check_apply_state(struct apply_state *state, int force_apply)
 {
int is_not_gitdir = !startup_info->have_repository;
@@ -143,6 +148,13 @@ int check_apply_state(struct apply_state *state, int 
force_apply)
if (!state->lock_file)
return error("BUG: state->lock_file should not be NULL");
 
+   if (state->be_silent) {
+   state->saved_error_routine = get_error_routine();
+   state->saved_warn_routine = get_warn_routine();
+   set_error_routine(mute_routine);
+   set_warn_routine(mute_routine);
+   }
+
return 0;
 }
 
@@ -4760,6 +4772,7 @@ int apply_all_patches(struct apply_state *state,
 {
int i;
int res;
+   int retval = -1;
int errs = 0;
int read_stdin = 1;
 
@@ -4838,12 +4851,18 @@ int apply_all_patches(struct apply_state *state,
state->newfd = -1;
}
 
-   return !!errs;
+   retval = !!errs;
 
 rollback_end:
if (state->newfd >= 0) {
rollback_lock_file(state->lock_file);
state->newfd = -1;
}
-   return -1;
+
+   if (state->be_silent) {
+   set_error_routine(state->saved_error_routine);
+   set_warn_routine(state->saved_warn_routine);
+   }
+
+   return retval;
 }
diff --git a/apply.h b/apply.h
index 034541a..c6cf33d 100644
--- a/apply.h
+++ b/apply.h
@@ -89,6 +89,10 @@ struct apply_state {
 */
struct string_list fn_table;
 
+   /* This is to save some reporting routines */
+   void (*saved_error_routine)(const char *err, va_list params);
+   void (*saved_warn_routine)(const char *warn, va_list params);
+
/* These control whitespace errors */
enum ws_error_action ws_error_action;
enum ws_ignore ws_ignore_action;
-- 
2.9.0.rc2.362.g3cd93d0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v6 36/44] apply: make 'be_silent' incompatible with 'apply_verbosely'

[Christian Couder]

It should be an error to have both be_silent and apply_verbosely set,
so let's check that in check_apply_state().

And by the way let's not automatically set apply_verbosely when
be_silent is set.

Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
---
 apply.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/apply.c b/apply.c
index dbb2515..dd9b301 100644
--- a/apply.c
+++ b/apply.c
@@ -122,8 +122,11 @@ int check_apply_state(struct apply_state *state, int 
force_apply)
return error(_("--3way outside a repository"));
state->check_index = 1;
}
-   if (state->apply_with_reject)
-   state->apply = state->apply_verbosely = 1;
+   if (state->apply_with_reject) {
+   state->apply = 1;
+   if (!state->be_silent)
+   state->apply_verbosely = 1;
+   }
if (!force_apply && (state->diffstat || state->numstat || 
state->summary || state->check || state->fake_ancestor))
state->apply = 0;
if (state->check_index && is_not_gitdir)
@@ -135,6 +138,8 @@ int check_apply_state(struct apply_state *state, int 
force_apply)
}
if (state->check_index)
state->unsafe_paths = 0;
+   if (state->be_silent && state->apply_verbosely)
+   return error(_("incompatible internal 'be_silent' and 
'apply_verbosely' flags"));
if (!state->lock_file)
return error("BUG: state->lock_file should not be NULL");
 
-- 
2.9.0.rc2.362.g3cd93d0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v6 05/44] builtin/apply: make parse_chunk() return a negative integer on error

[Christian Couder]

To libify `git apply` functionality we have to signal errors to the
caller instead of die()ing or exit()ing.

To do that in a compatible manner with the rest of the error handling
in builtin/apply.c, parse_chunk() should return -1 instead of calling
die() or exit().

As parse_chunk() is called only by apply_patch() which already
returns -1 when an error happened, let's make apply_patch() return -1
when parse_chunk() returns -1.

If find_header() returns -2 because no patch header has been found, it
is ok for parse_chunk() to also return -2. If find_header() returns -1
because an error happened, it is ok for parse_chunk() to do the same.

Helped-by: Eric Sunshine <sunsh...@sunshineco.com>
Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
---
 builtin/apply.c | 18 +++---
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 630c01c..a160338 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -1991,22 +1991,22 @@ static int use_patch(struct apply_state *state, struct 
patch *p)
return !state->has_include;
 }
 
-
 /*
  * Read the patch text in "buffer" that extends for "size" bytes; stop
  * reading after seeing a single patch (i.e. changes to a single file).
  * Create fragments (i.e. patch hunks) and hang them to the given patch.
- * Return the number of bytes consumed, so that the caller can call us
- * again for the next patch.
+ *
+ * Returns:
+ *   -1 on error,
+ *   -2 if no header was found,
+ *   the number of bytes consumed otherwise,
+ * so that the caller can call us again for the next patch.
  */
 static int parse_chunk(struct apply_state *state, char *buffer, unsigned long 
size, struct patch *patch)
 {
int hdrsize, patchsize;
int offset = find_header(state, buffer, size, , patch);
 
-   if (offset == -1)
-   exit(1);
-
if (offset < 0)
return offset;
 
@@ -2067,7 +2067,7 @@ static int parse_chunk(struct apply_state *state, char 
*buffer, unsigned long si
 */
if ((state->apply || state->check) &&
(!patch->is_binary && !metadata_changes(patch)))
-   die(_("patch with only garbage at line %d"), 
state->linenr);
+   return error(_("patch with only garbage at line %d"), 
state->linenr);
}
 
return offset + hdrsize + patchsize;
@@ -4449,6 +4449,10 @@ static int apply_patch(struct apply_state *state,
nr = parse_chunk(state, buf.buf + offset, buf.len - offset, 
patch);
if (nr < 0) {
free_patch(patch);
+   if (nr == -1) {
+   res = -1;
+   goto end;
+   }
break;
}
if (state->apply_in_reverse)
-- 
2.9.0.rc2.362.g3cd93d0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v6 18/44] builtin/apply: make build_fake_ancestor() return -1 on error

[Christian Couder]

To libify `git apply` functionality we have to signal errors to the
caller instead of die()ing.

To do that in a compatible manner with the rest of the error handling
in "builtin/apply.c", build_fake_ancestor() should return -1 instead
of calling die().

Helped-by: Eric Sunshine <sunsh...@sunshineco.com>
Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
---
 builtin/apply.c | 41 ++---
 1 file changed, 26 insertions(+), 15 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 429fddd..e74b068 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -3889,11 +3889,12 @@ static int preimage_sha1_in_gitlink_patch(struct patch 
*p, unsigned char sha1[20
 }
 
 /* Build an index that contains the just the files needed for a 3way merge */
-static void build_fake_ancestor(struct patch *list, const char *filename)
+static int build_fake_ancestor(struct patch *list, const char *filename)
 {
struct patch *patch;
struct index_state result = { NULL };
static struct lock_file lock;
+   int res;
 
/* Once we start supporting the reverse patch, it may be
 * worth showing the new sha1 prefix, but until then...
@@ -3911,31 +3912,38 @@ static void build_fake_ancestor(struct patch *list, 
const char *filename)
if (!preimage_sha1_in_gitlink_patch(patch, sha1))
; /* ok, the textual part looks sane */
else
-   die("sha1 information is lacking or useless for 
submodule %s",
-   name);
+   return error("sha1 information is lacking or "
+"useless for submodule %s", name);
} else if (!get_sha1_blob(patch->old_sha1_prefix, sha1)) {
; /* ok */
} else if (!patch->lines_added && !patch->lines_deleted) {
/* mode-only change: update the current */
if (get_current_sha1(patch->old_name, sha1))
-   die("mode change for %s, which is not "
-   "in current HEAD", name);
+   return error("mode change for %s, which is not "
+"in current HEAD", name);
} else
-   die("sha1 information is lacking or useless "
-   "(%s).", name);
+   return error("sha1 information is lacking or useless "
+"(%s).", name);
 
ce = make_cache_entry(patch->old_mode, sha1, name, 0, 0);
if (!ce)
-   die(_("make_cache_entry failed for path '%s'"), name);
-   if (add_index_entry(, ce, ADD_CACHE_OK_TO_ADD))
-   die ("Could not add %s to temporary index", name);
+   return error(_("make_cache_entry failed for path '%s'"),
+name);
+   if (add_index_entry(, ce, ADD_CACHE_OK_TO_ADD)) {
+   free(ce);
+   return error("Could not add %s to temporary index",
+name);
+   }
}
 
hold_lock_file_for_update(, filename, LOCK_DIE_ON_ERROR);
-   if (write_locked_index(, , COMMIT_LOCK))
-   die ("Could not write temporary index to %s", filename);
-
+   res = write_locked_index(, , COMMIT_LOCK);
discard_index();
+
+   if (res)
+   return error("Could not write temporary index to %s", filename);
+
+   return 0;
 }
 
 static void stat_patch_list(struct apply_state *state, struct patch *patch)
@@ -4476,8 +4484,11 @@ static int apply_patch(struct apply_state *state,
goto end;
}
 
-   if (state->fake_ancestor)
-   build_fake_ancestor(list, state->fake_ancestor);
+   if (state->fake_ancestor &&
+   build_fake_ancestor(list, state->fake_ancestor)) {
+   res = -1;
+   goto end;
+   }
 
if (state->diffstat)
stat_patch_list(state, list);
-- 
2.9.0.rc2.362.g3cd93d0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v6 03/44] builtin/apply: read_patch_file() return -1 instead of die()ing

[Christian Couder]

To libify `git apply` functionality we have to signal errors to the
caller instead of die()ing. Let's do that by returning -1 instead of
die()ing in read_patch_file().

Helped-by: Eric Sunshine <sunsh...@sunshineco.com>
Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
---
 builtin/apply.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 598e479..2ff8450 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -335,10 +335,10 @@ static void say_patch_name(FILE *output, const char *fmt, 
struct patch *patch)
 
 #define SLOP (16)
 
-static void read_patch_file(struct strbuf *sb, int fd)
+static int read_patch_file(struct strbuf *sb, int fd)
 {
if (strbuf_read(sb, fd, 0) < 0)
-   die_errno("git apply: failed to read");
+   return error_errno("git apply: failed to read");
 
/*
 * Make sure that we have some slop in the buffer
@@ -347,6 +347,7 @@ static void read_patch_file(struct strbuf *sb, int fd)
 */
strbuf_grow(sb, SLOP);
memset(sb->buf + sb->len, 0, SLOP);
+   return 0;
 }
 
 static unsigned long linelen(const char *buffer, unsigned long size)
@@ -4424,7 +4425,8 @@ static int apply_patch(struct apply_state *state,
int res = 0;
 
state->patch_input_file = filename;
-   read_patch_file(, fd);
+   if (read_patch_file(, fd))
+   return -1;
offset = 0;
while (offset < buf.len) {
struct patch *patch;
-- 
2.9.0.rc2.362.g3cd93d0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v6 15/44] builtin/apply: make gitdiff_*() return 1 at end of header

[Christian Couder]

The gitdiff_*() functions that are called as p->fn() in parse_git_header()
should return 1 instead of -1 in case of end of header or unrecognized
input, as these are not real errors. It just instructs the parser to break
out.

This makes it possible for gitdiff_*() functions to return -1 in case of a
real error. This will be done in a following patch.

Helped-by: Nguyễn Thái Ngọc Duy <pclo...@gmail.com>
Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
---
 builtin/apply.c | 12 +---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index eb98116..1142514 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -812,7 +812,7 @@ static int gitdiff_hdrend(struct apply_state *state,
  const char *line,
  struct patch *patch)
 {
-   return -1;
+   return 1;
 }
 
 /*
@@ -1016,7 +1016,7 @@ static int gitdiff_unrecognized(struct apply_state *state,
const char *line,
struct patch *patch)
 {
-   return -1;
+   return 1;
 }
 
 /*
@@ -1248,9 +1248,13 @@ static int parse_git_header(struct apply_state *state,
for (i = 0; i < ARRAY_SIZE(optable); i++) {
const struct opentry *p = optable + i;
int oplen = strlen(p->str);
+   int res;
if (len < oplen || memcmp(p->str, line, oplen))
continue;
-   if (p->fn(state, line + oplen, patch) < 0)
+   res = p->fn(state, line + oplen, patch);
+   if (res < 0)
+   return -1;
+   if (res > 0)
return offset;
break;
}
@@ -1429,6 +1433,8 @@ static int find_header(struct apply_state *state,
 */
if (!memcmp("diff --git ", line, 11)) {
int git_hdr_len = parse_git_header(state, line, len, 
size, patch);
+   if (git_hdr_len < 0)
+   return -1;
if (git_hdr_len <= len)
continue;
if (!patch->old_name && !patch->new_name) {
-- 
2.9.0.rc2.362.g3cd93d0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v6 42/44] run-command: make dup_devnull() static again

[Christian Couder]

As there is no caller of dup_devnull() outside run-command.c any more,
let's make dup_devnull() static again.

Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
---
 run-command.c | 2 +-
 run-command.h | 6 --
 2 files changed, 1 insertion(+), 7 deletions(-)

diff --git a/run-command.c b/run-command.c
index d8ed88c..c09d6b0 100644
--- a/run-command.c
+++ b/run-command.c
@@ -85,7 +85,7 @@ static inline void close_pair(int fd[2])
 }
 
 #ifndef GIT_WINDOWS_NATIVE
-void dup_devnull(int to)
+static void dup_devnull(int to)
 {
int fd = open("/dev/null", O_RDWR);
if (fd < 0)
diff --git a/run-command.h b/run-command.h
index e05ce7d..11f76b0 100644
--- a/run-command.h
+++ b/run-command.h
@@ -201,10 +201,4 @@ int run_processes_parallel(int n,
   task_finished_fn,
   void *pp_cb);
 
-/**
- * Misc helper functions
- */
-
-void dup_devnull(int to);
-
 #endif
-- 
2.9.0.rc2.362.g3cd93d0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v6 39/44] usage: add get_error_routine() and get_warn_routine()

[Christian Couder]

Let's make it possible to get the current error_routine and warn_routine,
so that we can store them before using set_error_routine() or
set_warn_routine() to use new ones.

This way we will be able put back the original routines, when we are done
with using new ones.

Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
---
 git-compat-util.h |  2 ++
 usage.c   | 10 ++
 2 files changed, 12 insertions(+)

diff --git a/git-compat-util.h b/git-compat-util.h
index f78706a..92af27d 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -440,7 +440,9 @@ static inline int const_error(void)
 
 extern void set_die_routine(NORETURN_PTR void (*routine)(const char *err, 
va_list params));
 extern void set_error_routine(void (*routine)(const char *err, va_list 
params));
+extern void (*get_error_routine(void))(const char *err, va_list params);
 extern void set_warn_routine(void (*routine)(const char *warn, va_list 
params));
+extern void (*get_warn_routine(void))(const char *warn, va_list params);
 extern void set_die_is_recursing_routine(int (*routine)(void));
 extern void set_error_handle(FILE *);
 
diff --git a/usage.c b/usage.c
index 67e5526..2fd3045 100644
--- a/usage.c
+++ b/usage.c
@@ -70,11 +70,21 @@ void set_error_routine(void (*routine)(const char *err, 
va_list params))
error_routine = routine;
 }
 
+void (*get_error_routine(void))(const char *err, va_list params)
+{
+   return error_routine;
+}
+
 void set_warn_routine(void (*routine)(const char *warn, va_list params))
 {
warn_routine = routine;
 }
 
+void (*get_warn_routine(void))(const char *warn, va_list params)
+{
+   return warn_routine;
+}
+
 void set_die_is_recursing_routine(int (*routine)(void))
 {
die_is_recursing = routine;
-- 
2.9.0.rc2.362.g3cd93d0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v6 31/44] run-command: make dup_devnull() non static

[Christian Couder]

We will need this function in a later commit to redirect stdout
and stderr to /dev/null.

Helped-by: Johannes Sixt <j...@kdbg.org>
Helped-by: Johannes Schindelin <johannes.schinde...@gmx.de>
Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
---
 run-command.c | 2 +-
 run-command.h | 6 ++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/run-command.c b/run-command.c
index af0c8a1..d8ed88c 100644
--- a/run-command.c
+++ b/run-command.c
@@ -85,7 +85,7 @@ static inline void close_pair(int fd[2])
 }
 
 #ifndef GIT_WINDOWS_NATIVE
-static inline void dup_devnull(int to)
+void dup_devnull(int to)
 {
int fd = open("/dev/null", O_RDWR);
if (fd < 0)
diff --git a/run-command.h b/run-command.h
index 11f76b0..e05ce7d 100644
--- a/run-command.h
+++ b/run-command.h
@@ -201,4 +201,10 @@ int run_processes_parallel(int n,
   task_finished_fn,
   void *pp_cb);
 
+/**
+ * Misc helper functions
+ */
+
+void dup_devnull(int to);
+
 #endif
-- 
2.9.0.rc2.362.g3cd93d0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v6 14/44] builtin/apply: make parse_traditional_patch() return -1 on error

[Christian Couder]

To libify `git apply` functionality we have to signal errors to the
caller instead of die()ing.

To do that in a compatible manner with the rest of the error handling
in "builtin/apply.c", parse_traditional_patch() should return -1
instead of calling die().

Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
---
 builtin/apply.c | 15 +--
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index c27be35..eb98116 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -755,10 +755,10 @@ static int has_epoch_timestamp(const char *nameline)
  * files, we can happily check the index for a match, but for creating a
  * new file we should try to match whatever "patch" does. I have no idea.
  */
-static void parse_traditional_patch(struct apply_state *state,
-   const char *first,
-   const char *second,
-   struct patch *patch)
+static int parse_traditional_patch(struct apply_state *state,
+  const char *first,
+  const char *second,
+  struct patch *patch)
 {
char *name;
 
@@ -803,7 +803,9 @@ static void parse_traditional_patch(struct apply_state 
*state,
}
}
if (!name)
-   die(_("unable to find filename in patch at line %d"), 
state->linenr);
+   return error(_("unable to find filename in patch at line %d"), 
state->linenr);
+
+   return 0;
 }
 
 static int gitdiff_hdrend(struct apply_state *state,
@@ -1462,7 +1464,8 @@ static int find_header(struct apply_state *state,
continue;
 
/* Ok, we'll consider it a patch */
-   parse_traditional_patch(state, line, line+len, patch);
+   if (parse_traditional_patch(state, line, line+len, patch))
+   return -1;
*hdrsize = len + nextlen;
state->linenr += 2;
return offset;
-- 
2.9.0.rc2.362.g3cd93d0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v6 12/44] builtin/apply: move check_apply_state() to apply.c

[Christian Couder]

To libify `git apply` functionality we must make check_apply_state()
usable outside "builtin/apply.c".

Let's do that by moving it into "apply.c".

Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
---
 apply.c | 32 
 apply.h |  1 +
 builtin/apply.c | 32 
 3 files changed, 33 insertions(+), 32 deletions(-)

diff --git a/apply.c b/apply.c
index c5a9ee2..84dae3d 100644
--- a/apply.c
+++ b/apply.c
@@ -90,3 +90,35 @@ void clear_apply_state(struct apply_state *state)
 
/* >fn_table is cleared at the end of apply_patch() */
 }
+
+int check_apply_state(struct apply_state *state, int force_apply)
+{
+   int is_not_gitdir = !startup_info->have_repository;
+
+   if (state->apply_with_reject && state->threeway)
+   return error("--reject and --3way cannot be used together.");
+   if (state->cached && state->threeway)
+   return error("--cached and --3way cannot be used together.");
+   if (state->threeway) {
+   if (is_not_gitdir)
+   return error(_("--3way outside a repository"));
+   state->check_index = 1;
+   }
+   if (state->apply_with_reject)
+   state->apply = state->apply_verbosely = 1;
+   if (!force_apply && (state->diffstat || state->numstat || 
state->summary || state->check || state->fake_ancestor))
+   state->apply = 0;
+   if (state->check_index && is_not_gitdir)
+   return error(_("--index outside a repository"));
+   if (state->cached) {
+   if (is_not_gitdir)
+   return error(_("--cached outside a repository"));
+   state->check_index = 1;
+   }
+   if (state->check_index)
+   state->unsafe_paths = 0;
+   if (!state->lock_file)
+   return error("BUG: state->lock_file should not be NULL");
+
+   return 0;
+}
diff --git a/apply.h b/apply.h
index 7d3a03b..1f2277e 100644
--- a/apply.h
+++ b/apply.h
@@ -106,5 +106,6 @@ extern int init_apply_state(struct apply_state *state,
const char *prefix,
struct lock_file *lock_file);
 extern void clear_apply_state(struct apply_state *state);
+extern int check_apply_state(struct apply_state *state, int force_apply);
 
 #endif
diff --git a/builtin/apply.c b/builtin/apply.c
index d60ffce..a27fdd3 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -4541,38 +4541,6 @@ static int option_parse_directory(const struct option 
*opt,
return 0;
 }
 
-static int check_apply_state(struct apply_state *state, int force_apply)
-{
-   int is_not_gitdir = !startup_info->have_repository;
-
-   if (state->apply_with_reject && state->threeway)
-   return error("--reject and --3way cannot be used together.");
-   if (state->cached && state->threeway)
-   return error("--cached and --3way cannot be used together.");
-   if (state->threeway) {
-   if (is_not_gitdir)
-   return error(_("--3way outside a repository"));
-   state->check_index = 1;
-   }
-   if (state->apply_with_reject)
-   state->apply = state->apply_verbosely = 1;
-   if (!force_apply && (state->diffstat || state->numstat || 
state->summary || state->check || state->fake_ancestor))
-   state->apply = 0;
-   if (state->check_index && is_not_gitdir)
-   return error(_("--index outside a repository"));
-   if (state->cached) {
-   if (is_not_gitdir)
-   return error(_("--cached outside a repository"));
-   state->check_index = 1;
-   }
-   if (state->check_index)
-   state->unsafe_paths = 0;
-   if (!state->lock_file)
-   return error("BUG: state->lock_file should not be NULL");
-
-   return 0;
-}
-
 static int apply_all_patches(struct apply_state *state,
 int argc,
 const char **argv,
-- 
2.9.0.rc2.362.g3cd93d0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v6 41/44] am: use be_silent in 'struct apply_state' to shut up applying patches

[Christian Couder]

Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
---
 builtin/am.c | 29 -
 1 file changed, 8 insertions(+), 21 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index a16b06c..43f7316 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1525,7 +1525,6 @@ static int run_apply(const struct am_state *state, const 
char *index_file)
struct argv_array apply_paths = ARGV_ARRAY_INIT;
struct argv_array apply_opts = ARGV_ARRAY_INIT;
struct apply_state apply_state;
-   int save_stdout_fd, save_stderr_fd;
int res, opts_left;
char *save_index_file;
static struct lock_file lock_file;
@@ -1559,18 +1558,6 @@ static int run_apply(const struct am_state *state, const 
char *index_file)
OPT_END()
};
 
-   /*
-* If we are allowed to fall back on 3-way merge, don't give false
-* errors during the initial attempt.
-*/
-
-   if (state->threeway && !index_file) {
-   save_stdout_fd = dup(1);
-   dup_devnull(1);
-   save_stderr_fd = dup(2);
-   dup_devnull(2);
-   }
-
if (index_file) {
save_index_file = get_index_file();
set_index_file((char *)index_file);
@@ -1593,6 +1580,14 @@ static int run_apply(const struct am_state *state, const 
char *index_file)
else
apply_state.check_index = 1;
 
+   /*
+* If we are allowed to fall back on 3-way merge, don't give false
+* errors during the initial attempt.
+*/
+
+   if (state->threeway && !index_file)
+   apply_state.be_silent = 1;
+
if (check_apply_state(_state, 0))
die("check_apply_state() failed");
 
@@ -1600,14 +1595,6 @@ static int run_apply(const struct am_state *state, const 
char *index_file)
 
res = apply_all_patches(_state, apply_paths.argc, 
apply_paths.argv, 0);
 
-   /* Restore stdout and stderr */
-   if (state->threeway && !index_file) {
-   dup2(save_stdout_fd, 1);
-   close(save_stdout_fd);
-   dup2(save_stderr_fd, 2);
-   close(save_stderr_fd);
-   }
-
if (index_file)
set_index_file(save_index_file);
 
-- 
2.9.0.rc2.362.g3cd93d0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v6 06/44] builtin/apply: make parse_single_patch() return -1 on error

[Christian Couder]

To libify `git apply` functionality we have to signal errors to the
caller instead of die()ing.

To do that in a compatible manner with the rest of the error handling
in builtin/apply.c, parse_single_patch() should return -1 instead of
calling die().

Let's do that by using error() and let's adjust the related test
cases accordingly.

Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
---
 builtin/apply.c| 17 +
 t/t4012-diff-binary.sh |  4 ++--
 2 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index a160338..2671586 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -1666,6 +1666,10 @@ static int parse_fragment(struct apply_state *state,
  *
  * The (fragment->patch, fragment->size) pair points into the memory given
  * by the caller, not a copy, when we return.
+ *
+ * Returns:
+ *   -1 in case of error,
+ *   the number of bytes in the patch otherwise.
  */
 static int parse_single_patch(struct apply_state *state,
  const char *line,
@@ -1683,8 +1687,10 @@ static int parse_single_patch(struct apply_state *state,
fragment = xcalloc(1, sizeof(*fragment));
fragment->linenr = state->linenr;
len = parse_fragment(state, line, size, patch, fragment);
-   if (len <= 0)
-   die(_("corrupt patch at line %d"), state->linenr);
+   if (len <= 0) {
+   free(fragment);
+   return error(_("corrupt patch at line %d"), 
state->linenr);
+   }
fragment->patch = line;
fragment->size = len;
oldlines += fragment->oldlines;
@@ -1720,9 +1726,9 @@ static int parse_single_patch(struct apply_state *state,
patch->is_delete = 0;
 
if (0 < patch->is_new && oldlines)
-   die(_("new file %s depends on old contents"), patch->new_name);
+   return error(_("new file %s depends on old contents"), 
patch->new_name);
if (0 < patch->is_delete && newlines)
-   die(_("deleted file %s still has contents"), patch->old_name);
+   return error(_("deleted file %s still has contents"), 
patch->old_name);
if (!patch->is_delete && !newlines && context)
fprintf_ln(stderr,
   _("** warning: "
@@ -2024,6 +2030,9 @@ static int parse_chunk(struct apply_state *state, char 
*buffer, unsigned long si
   size - offset - hdrsize,
   patch);
 
+   if (patchsize < 0)
+   return -1;
+
if (!patchsize) {
static const char git_binary[] = "GIT binary patch\n";
int hd = hdrsize + offset;
diff --git a/t/t4012-diff-binary.sh b/t/t4012-diff-binary.sh
index 643d729..0a8af76 100755
--- a/t/t4012-diff-binary.sh
+++ b/t/t4012-diff-binary.sh
@@ -68,7 +68,7 @@ test_expect_success C_LOCALE_OUTPUT 'apply detecting corrupt 
patch correctly' '
sed -e "s/-CIT/xCIT/" broken &&
test_must_fail git apply --stat --summary broken 2>detected &&
detected=$(cat detected) &&
-   detected=$(expr "$detected" : "fatal.*at line \\([0-9]*\\)\$") &&
+   detected=$(expr "$detected" : "error.*at line \\([0-9]*\\)\$") &&
detected=$(sed -ne "${detected}p" broken) &&
test "$detected" = xCIT
 '
@@ -77,7 +77,7 @@ test_expect_success C_LOCALE_OUTPUT 'apply detecting corrupt 
patch correctly' '
git diff --binary | sed -e "s/-CIT/xCIT/" >broken &&
test_must_fail git apply --stat --summary broken 2>detected &&
detected=$(cat detected) &&
-   detected=$(expr "$detected" : "fatal.*at line \\([0-9]*\\)\$") &&
+   detected=$(expr "$detected" : "error.*at line \\([0-9]*\\)\$") &&
detected=$(sed -ne "${detected}p" broken) &&
test "$detected" = xCIT
 '
-- 
2.9.0.rc2.362.g3cd93d0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v6 25/44] builtin/apply: make try_create_file() return -1 on error

[Christian Couder]

To libify `git apply` functionality we have to signal errors to the
caller instead of die()ing.

To do that in a compatible manner with the rest of the error handling
in "builtin/apply.c", try_create_file() should return -1 in case of
error.

Unfortunately try_create_file() currently returns -1 to signal a
recoverable error. To fix that, let's make it return 1 in case of
a recoverable error and -1 in case of an unrecoverable error.

Helped-by: Eric Sunshine <sunsh...@sunshineco.com>
Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
---
 builtin/apply.c | 41 ++---
 1 file changed, 30 insertions(+), 11 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index f35c901..37f0c2e 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -4139,38 +4139,45 @@ static int add_index_file(struct apply_state *state,
return 0;
 }
 
+/*
+ * Returns:
+ *  -1 if an unrecoverable error happened
+ *   0 if everything went well
+ *   1 if a recoverable error happened
+ */
 static int try_create_file(const char *path, unsigned int mode, const char 
*buf, unsigned long size)
 {
-   int fd;
+   int fd, res;
struct strbuf nbuf = STRBUF_INIT;
 
if (S_ISGITLINK(mode)) {
struct stat st;
if (!lstat(path, ) && S_ISDIR(st.st_mode))
return 0;
-   return mkdir(path, 0777);
+   return !!mkdir(path, 0777);
}
 
if (has_symlinks && S_ISLNK(mode))
/* Although buf:size is counted string, it also is NUL
 * terminated.
 */
-   return symlink(buf, path);
+   return !!symlink(buf, path);
 
fd = open(path, O_CREAT | O_EXCL | O_WRONLY, (mode & 0100) ? 0777 : 
0666);
if (fd < 0)
-   return -1;
+   return 1;
 
if (convert_to_working_tree(path, buf, size, )) {
size = nbuf.len;
buf  = nbuf.buf;
}
-   write_or_die(fd, buf, size);
+   res = !write_or_whine_pipe(fd, buf, size, path);
strbuf_release();
 
-   if (close(fd) < 0)
-   die_errno(_("closing file '%s'"), path);
-   return 0;
+   if (close(fd) < 0 && !res)
+   return error(_("closing file '%s': %s"), path, strerror(errno));
+
+   return res ? -1 : 0;
 }
 
 /*
@@ -4184,15 +4191,24 @@ static void create_one_file(struct apply_state *state,
const char *buf,
unsigned long size)
 {
+   int res;
+
if (state->cached)
return;
-   if (!try_create_file(path, mode, buf, size))
+
+   res = try_create_file(path, mode, buf, size);
+   if (res < 0)
+   exit(1);
+   if (!res)
return;
 
if (errno == ENOENT) {
if (safe_create_leading_directories(path))
return;
-   if (!try_create_file(path, mode, buf, size))
+   res = try_create_file(path, mode, buf, size);
+   if (res < 0)
+   exit(1);
+   if (!res)
return;
}
 
@@ -4211,7 +4227,10 @@ static void create_one_file(struct apply_state *state,
for (;;) {
char newpath[PATH_MAX];
mksnpath(newpath, sizeof(newpath), "%s~%u", path, nr);
-   if (!try_create_file(newpath, mode, buf, size)) {
+   res = try_create_file(newpath, mode, buf, size);
+   if (res < 0)
+   exit(1);
+   if (!res) {
if (!rename(newpath, path))
return;
unlink_or_warn(newpath);
-- 
2.9.0.rc2.362.g3cd93d0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v6 30/44] apply: make some parsing functions static again

[Christian Couder]

Some parsing functions that were used in both "apply.c" and
"builtin/apply.c" are now only used in the former, so they
can be made static to "apply.c".

Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
---
 apply.c | 6 +++---
 apply.h | 5 -
 2 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/apply.c b/apply.c
index 4920fa8..713d1c0 100644
--- a/apply.c
+++ b/apply.c
@@ -27,7 +27,7 @@ static void git_apply_config(void)
git_config(git_default_config, NULL);
 }
 
-int parse_whitespace_option(struct apply_state *state, const char *option)
+static int parse_whitespace_option(struct apply_state *state, const char 
*option)
 {
if (!option) {
state->ws_error_action = warn_on_ws_error;
@@ -57,8 +57,8 @@ int parse_whitespace_option(struct apply_state *state, const 
char *option)
return error(_("unrecognized whitespace option '%s'"), option);
 }
 
-int parse_ignorewhitespace_option(struct apply_state *state,
- const char *option)
+static int parse_ignorewhitespace_option(struct apply_state *state,
+const char *option)
 {
if (!option || !strcmp(option, "no") ||
!strcmp(option, "false") || !strcmp(option, "never") ||
diff --git a/apply.h b/apply.h
index 736f818..89e7982 100644
--- a/apply.h
+++ b/apply.h
@@ -97,11 +97,6 @@ struct apply_state {
int applied_after_fixing_ws;
 };
 
-extern int parse_whitespace_option(struct apply_state *state,
-  const char *option);
-extern int parse_ignorewhitespace_option(struct apply_state *state,
-const char *option);
-
 extern int apply_option_parse_exclude(const struct option *opt,
  const char *arg, int unset);
 extern int apply_option_parse_include(const struct option *opt,
-- 
2.9.0.rc2.362.g3cd93d0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v6 26/44] builtin/apply: make create_one_file() return -1 on error

[Christian Couder]

To libify `git apply` functionality we have to signal errors to the
caller instead of exit()ing.

To do that in a compatible manner with the rest of the error handling
in "builtin/apply.c", create_one_file() should return -1 instead of
calling exit().

Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
---
 builtin/apply.c | 36 +---
 1 file changed, 21 insertions(+), 15 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 37f0c2e..f51dc60 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -4184,32 +4184,36 @@ static int try_create_file(const char *path, unsigned 
int mode, const char *buf,
  * We optimistically assume that the directories exist,
  * which is true 99% of the time anyway. If they don't,
  * we create them and try again.
+ *
+ * Returns:
+ *   -1 on error
+ *   0 otherwise
  */
-static void create_one_file(struct apply_state *state,
-   char *path,
-   unsigned mode,
-   const char *buf,
-   unsigned long size)
+static int create_one_file(struct apply_state *state,
+  char *path,
+  unsigned mode,
+  const char *buf,
+  unsigned long size)
 {
int res;
 
if (state->cached)
-   return;
+   return 0;
 
res = try_create_file(path, mode, buf, size);
if (res < 0)
-   exit(1);
+   return -1;
if (!res)
-   return;
+   return 0;
 
if (errno == ENOENT) {
if (safe_create_leading_directories(path))
-   return;
+   return 0;
res = try_create_file(path, mode, buf, size);
if (res < 0)
-   exit(1);
+   return -1;
if (!res)
-   return;
+   return 0;
}
 
if (errno == EEXIST || errno == EACCES) {
@@ -4229,10 +4233,10 @@ static void create_one_file(struct apply_state *state,
mksnpath(newpath, sizeof(newpath), "%s~%u", path, nr);
res = try_create_file(newpath, mode, buf, size);
if (res < 0)
-   exit(1);
+   return -1;
if (!res) {
if (!rename(newpath, path))
-   return;
+   return 0;
unlink_or_warn(newpath);
break;
}
@@ -4241,7 +4245,8 @@ static void create_one_file(struct apply_state *state,
++nr;
}
}
-   die_errno(_("unable to write file '%s' mode %o"), path, mode);
+   return error(_("unable to write file '%s' mode %o: %s"),
+path, mode, strerror(errno));
 }
 
 static int add_conflicted_stages_file(struct apply_state *state,
@@ -4286,7 +4291,8 @@ static int create_file(struct apply_state *state, struct 
patch *patch)
 
if (!mode)
mode = S_IFREG | 0644;
-   create_one_file(state, path, mode, buf, size);
+   if (create_one_file(state, path, mode, buf, size))
+   return -1;
 
if (patch->conflicted_threeway)
return add_conflicted_stages_file(state, patch);
-- 
2.9.0.rc2.362.g3cd93d0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v6 24/44] builtin/apply: make write_out_results() return -1 on error

[Christian Couder]

To libify `git apply` functionality we have to signal errors to the
caller instead of exit()ing.

To do that in a compatible manner with the rest of the error handling
in "builtin/apply.c", write_out_results() should return -1 instead of
calling exit().

Helped-by: Eric Sunshine <sunsh...@sunshineco.com>
Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
---
 builtin/apply.c | 27 +--
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 291e24e..f35c901 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -4372,6 +4372,12 @@ static int write_out_one_reject(struct apply_state 
*state, struct patch *patch)
return -1;
 }
 
+/*
+ * Returns:
+ *  -1 if an error happened
+ *   0 if the patch applied cleanly
+ *   1 if the patch did not apply cleanly
+ */
 static int write_out_results(struct apply_state *state, struct patch *list)
 {
int phase;
@@ -4385,8 +4391,10 @@ static int write_out_results(struct apply_state *state, 
struct patch *list)
if (l->rejected)
errs = 1;
else {
-   if (write_out_one_result(state, l, phase))
-   exit(1);
+   if (write_out_one_result(state, l, phase)) {
+   string_list_clear(, 0);
+   return -1;
+   }
if (phase == 1) {
if (write_out_one_reject(state, l))
errs = 1;
@@ -4498,10 +4506,17 @@ static int apply_patch(struct apply_state *state,
goto end;
}
 
-   if (state->apply && write_out_results(state, list)) {
-   /* with --3way, we still need to write the index out */
-   res = state->apply_with_reject ? -1 : 1;
-   goto end;
+   if (state->apply) {
+   int write_res = write_out_results(state, list);
+   if (write_res < 0) {
+   res = -1;
+   goto end;
+   }
+   if (write_res > 0) {
+   /* with --3way, we still need to write the index out */
+   res = state->apply_with_reject ? -1 : 1;
+   goto end;
+   }
}
 
if (state->fake_ancestor &&
-- 
2.9.0.rc2.362.g3cd93d0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v6 22/44] builtin/apply: make create_file() return -1 on error

[Christian Couder]

To libify `git apply` functionality we have to signal errors to the
caller instead of exit()ing.

To do that in a compatible manner with the rest of the error handling
in "builtin/apply.c", create_file() should just return what
add_conflicted_stages_file() and add_index_file() are returning
instead of calling exit().

Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
---
 builtin/apply.c | 25 +
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 005ba78..76d473c 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -4258,7 +4258,7 @@ static int add_conflicted_stages_file(struct apply_state 
*state,
return 0;
 }
 
-static void create_file(struct apply_state *state, struct patch *patch)
+static int create_file(struct apply_state *state, struct patch *patch)
 {
char *path = patch->new_name;
unsigned mode = patch->new_mode;
@@ -4269,13 +4269,10 @@ static void create_file(struct apply_state *state, 
struct patch *patch)
mode = S_IFREG | 0644;
create_one_file(state, path, mode, buf, size);
 
-   if (patch->conflicted_threeway) {
-   if (add_conflicted_stages_file(state, patch))
-   exit(1);
-   } else {
-   if (add_index_file(state, path, mode, buf, size))
-   exit(1);
-   }
+   if (patch->conflicted_threeway)
+   return add_conflicted_stages_file(state, patch);
+   else
+   return add_index_file(state, path, mode, buf, size);
 }
 
 /* phase zero is to remove, phase one is to create */
@@ -4291,8 +4288,10 @@ static void write_out_one_result(struct apply_state 
*state,
return;
}
if (patch->is_new > 0 || patch->is_copy) {
-   if (phase == 1)
-   create_file(state, patch);
+   if (phase == 1) {
+   if (create_file(state, patch))
+   exit(1);
+   }
return;
}
/*
@@ -4303,8 +4302,10 @@ static void write_out_one_result(struct apply_state 
*state,
if (remove_file(state, patch, patch->is_rename))
exit(1);
}
-   if (phase == 1)
-   create_file(state, patch);
+   if (phase == 1) {
+   if (create_file(state, patch))
+   exit(1);
+   }
 }
 
 static int write_out_one_reject(struct apply_state *state, struct patch *patch)
-- 
2.9.0.rc2.362.g3cd93d0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v6 28/44] apply: rename and move opt constants to apply.h

[Christian Couder]

The constants for the "inaccurate-eof" and the "recount" options will
be used in both "apply.c" and "builtin/apply.c", so they need to go
into "apply.h", and therefore they need a name that is more specific
to the API they belong to.

Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
---
 apply.h |  3 +++
 builtin/apply.c | 11 ---
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/apply.h b/apply.h
index 1f2277e..3cc3da6 100644
--- a/apply.h
+++ b/apply.h
@@ -108,4 +108,7 @@ extern int init_apply_state(struct apply_state *state,
 extern void clear_apply_state(struct apply_state *state);
 extern int check_apply_state(struct apply_state *state, int force_apply);
 
+#define APPLY_OPT_INACCURATE_EOF   (1<<0)
+#define APPLY_OPT_RECOUNT  (1<<1)
+
 #endif
diff --git a/builtin/apply.c b/builtin/apply.c
index c84add0..a06ab55 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -4449,9 +4449,6 @@ static int write_out_results(struct apply_state *state, 
struct patch *list)
 
 static struct lock_file lock_file;
 
-#define INACCURATE_EOF (1<<0)
-#define RECOUNT(1<<1)
-
 /*
  * Try to apply a patch.
  *
@@ -4480,8 +4477,8 @@ static int apply_patch(struct apply_state *state,
int nr;
 
patch = xcalloc(1, sizeof(*patch));
-   patch->inaccurate_eof = !!(options & INACCURATE_EOF);
-   patch->recount =  !!(options & RECOUNT);
+   patch->inaccurate_eof = !!(options & APPLY_OPT_INACCURATE_EOF);
+   patch->recount =  !!(options & APPLY_OPT_RECOUNT);
nr = parse_chunk(state, buf.buf + offset, buf.len - offset, 
patch);
if (nr < 0) {
free_patch(patch);
@@ -4785,10 +4782,10 @@ int cmd_apply(int argc, const char **argv, const char 
*prefix)
OPT__VERBOSE(_verbosely, N_("be verbose")),
OPT_BIT(0, "inaccurate-eof", ,
N_("tolerate incorrectly detected missing new-line at 
the end of file"),
-   INACCURATE_EOF),
+   APPLY_OPT_INACCURATE_EOF),
OPT_BIT(0, "recount", ,
N_("do not trust the line counts in the hunk headers"),
-   RECOUNT),
+   APPLY_OPT_RECOUNT),
{ OPTION_CALLBACK, 0, "directory", , N_("root"),
N_("prepend  to all filenames"),
0, apply_option_parse_directory },
-- 
2.9.0.rc2.362.g3cd93d0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v6 27/44] builtin/apply: rename option parsing functions

[Christian Couder]

As these functions are going to be part of the libified
apply api, let's give them a name that is more specific
to the apply api.

Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
---
 builtin/apply.c | 40 
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index f51dc60..c84add0 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -4566,16 +4566,16 @@ end:
return res;
 }
 
-static int option_parse_exclude(const struct option *opt,
-   const char *arg, int unset)
+static int apply_option_parse_exclude(const struct option *opt,
+ const char *arg, int unset)
 {
struct apply_state *state = opt->value;
add_name_limit(state, arg, 1);
return 0;
 }
 
-static int option_parse_include(const struct option *opt,
-   const char *arg, int unset)
+static int apply_option_parse_include(const struct option *opt,
+ const char *arg, int unset)
 {
struct apply_state *state = opt->value;
add_name_limit(state, arg, 0);
@@ -4583,9 +4583,9 @@ static int option_parse_include(const struct option *opt,
return 0;
 }
 
-static int option_parse_p(const struct option *opt,
- const char *arg,
- int unset)
+static int apply_option_parse_p(const struct option *opt,
+   const char *arg,
+   int unset)
 {
struct apply_state *state = opt->value;
state->p_value = atoi(arg);
@@ -4593,8 +4593,8 @@ static int option_parse_p(const struct option *opt,
return 0;
 }
 
-static int option_parse_space_change(const struct option *opt,
-const char *arg, int unset)
+static int apply_option_parse_space_change(const struct option *opt,
+  const char *arg, int unset)
 {
struct apply_state *state = opt->value;
if (unset)
@@ -4604,8 +4604,8 @@ static int option_parse_space_change(const struct option 
*opt,
return 0;
 }
 
-static int option_parse_whitespace(const struct option *opt,
-  const char *arg, int unset)
+static int apply_option_parse_whitespace(const struct option *opt,
+const char *arg, int unset)
 {
struct apply_state *state = opt->value;
state->whitespace_option = arg;
@@ -4614,8 +4614,8 @@ static int option_parse_whitespace(const struct option 
*opt,
return 0;
 }
 
-static int option_parse_directory(const struct option *opt,
- const char *arg, int unset)
+static int apply_option_parse_directory(const struct option *opt,
+   const char *arg, int unset)
 {
struct apply_state *state = opt->value;
strbuf_reset(>root);
@@ -4729,13 +4729,13 @@ int cmd_apply(int argc, const char **argv, const char 
*prefix)
struct option builtin_apply_options[] = {
{ OPTION_CALLBACK, 0, "exclude", , N_("path"),
N_("don't apply changes matching the given path"),
-   0, option_parse_exclude },
+   0, apply_option_parse_exclude },
{ OPTION_CALLBACK, 0, "include", , N_("path"),
N_("apply changes matching the given path"),
-   0, option_parse_include },
+   0, apply_option_parse_include },
{ OPTION_CALLBACK, 'p', NULL, , N_("num"),
N_("remove  leading slashes from traditional diff 
paths"),
-   0, option_parse_p },
+   0, apply_option_parse_p },
OPT_BOOL(0, "no-add", _add,
N_("ignore additions made by the patch")),
OPT_BOOL(0, "stat", ,
@@ -4767,13 +4767,13 @@ int cmd_apply(int argc, const char **argv, const char 
*prefix)
N_("ensure at least  lines of context 
match")),
{ OPTION_CALLBACK, 0, "whitespace", , N_("action"),
N_("detect new or modified lines that have whitespace 
errors"),
-   0, option_parse_whitespace },
+   0, apply_option_parse_whitespace },
{ OPTION_CALLBACK, 0, "ignore-space-change", , NULL,
N_("ignore changes in whitespace when finding context"),
-   PARSE_OPT_NOARG, option_parse_space_change },
+   PARSE_OPT_NOARG, apply_option_parse_space_change },
   

[PATCH v6 10/44] apply: make init_apply_state() return -1 instead of exit()ing

[Christian Couder]

To libify `git apply` functionality we have to signal errors to the
caller instead of exit()ing.

To do that in a compatible manner with the rest of the error handling
in "builtin/apply.c", init_apply_state() should return -1 instead of
calling exit().

Helped-by: Eric Sunshine <sunsh...@sunshineco.com>
Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
---
 apply.c | 11 ++-
 apply.h |  6 +++---
 builtin/apply.c |  3 ++-
 3 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/apply.c b/apply.c
index 1f31bb4..c5a9ee2 100644
--- a/apply.c
+++ b/apply.c
@@ -55,9 +55,9 @@ int parse_ignorewhitespace_option(struct apply_state *state,
return error(_("unrecognized whitespace ignore option '%s'"), option);
 }
 
-void init_apply_state(struct apply_state *state,
- const char *prefix,
- struct lock_file *lock_file)
+int init_apply_state(struct apply_state *state,
+const char *prefix,
+struct lock_file *lock_file)
 {
memset(state, 0, sizeof(*state));
state->prefix = prefix;
@@ -76,9 +76,10 @@ void init_apply_state(struct apply_state *state,
 
git_apply_config();
if (apply_default_whitespace && parse_whitespace_option(state, 
apply_default_whitespace))
-   exit(1);
+   return -1;
if (apply_default_ignorewhitespace && 
parse_ignorewhitespace_option(state, apply_default_ignorewhitespace))
-   exit(1);
+   return -1;
+   return 0;
 }
 
 void clear_apply_state(struct apply_state *state)
diff --git a/apply.h b/apply.h
index 77502be..7d3a03b 100644
--- a/apply.h
+++ b/apply.h
@@ -102,9 +102,9 @@ extern int parse_whitespace_option(struct apply_state 
*state,
 extern int parse_ignorewhitespace_option(struct apply_state *state,
 const char *option);
 
-extern void init_apply_state(struct apply_state *state,
-const char *prefix,
-struct lock_file *lock_file);
+extern int init_apply_state(struct apply_state *state,
+   const char *prefix,
+   struct lock_file *lock_file);
 extern void clear_apply_state(struct apply_state *state);
 
 #endif
diff --git a/builtin/apply.c b/builtin/apply.c
index bc15545..2ae1243 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -4728,7 +4728,8 @@ int cmd_apply(int argc, const char **argv, const char 
*prefix)
OPT_END()
};
 
-   init_apply_state(, prefix, _file);
+   if (init_apply_state(, prefix, _file))
+   exit(1);
 
argc = parse_options(argc, argv, state.prefix, builtin_apply_options,
apply_usage, 0);
-- 
2.9.0.rc2.362.g3cd93d0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v6 17/44] builtin/apply: change die_on_unsafe_path() to check_unsafe_path()

[Christian Couder]

To libify `git apply` functionality we have to signal errors to the
caller instead of die()ing.

To do that in a compatible manner with the rest of the error handling
in "builtin/apply.c", die_on_unsafe_path() should return -1 using
error() instead of calling die(), so while doing that let's change
its name to check_unsafe_path().

Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
---
 builtin/apply.c | 11 ++-
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index b506369..429fddd 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -3697,7 +3697,7 @@ static int path_is_beyond_symlink(struct apply_state 
*state, const char *name_)
return ret;
 }
 
-static void die_on_unsafe_path(struct patch *patch)
+static int check_unsafe_path(struct patch *patch)
 {
const char *old_name = NULL;
const char *new_name = NULL;
@@ -3709,9 +3709,10 @@ static void die_on_unsafe_path(struct patch *patch)
new_name = patch->new_name;
 
if (old_name && !verify_path(old_name))
-   die(_("invalid path '%s'"), old_name);
+   return error(_("invalid path '%s'"), old_name);
if (new_name && !verify_path(new_name))
-   die(_("invalid path '%s'"), new_name);
+   return error(_("invalid path '%s'"), new_name);
+   return 0;
 }
 
 /*
@@ -3801,8 +3802,8 @@ static int check_patch(struct apply_state *state, struct 
patch *patch)
}
}
 
-   if (!state->unsafe_paths)
-   die_on_unsafe_path(patch);
+   if (!state->unsafe_paths && check_unsafe_path(patch))
+   return -1;
 
/*
 * An attempt to read from or delete a path that is beyond a
-- 
2.9.0.rc2.362.g3cd93d0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v6 23/44] builtin/apply: make write_out_one_result() return -1 on error

[Christian Couder]

To libify `git apply` functionality we have to signal errors to the
caller instead of exit()ing.

To do that in a compatible manner with the rest of the error handling
in "builtin/apply.c", write_out_one_result() should just return what
remove_file() and create_file() are returning instead of calling
exit().

Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
---
 builtin/apply.c | 38 --
 1 file changed, 16 insertions(+), 22 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 76d473c..291e24e 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -4276,36 +4276,29 @@ static int create_file(struct apply_state *state, 
struct patch *patch)
 }
 
 /* phase zero is to remove, phase one is to create */
-static void write_out_one_result(struct apply_state *state,
-struct patch *patch,
-int phase)
+static int write_out_one_result(struct apply_state *state,
+   struct patch *patch,
+   int phase)
 {
if (patch->is_delete > 0) {
-   if (phase == 0) {
-   if (remove_file(state, patch, 1))
-   exit(1);
-   }
-   return;
+   if (phase == 0)
+   return remove_file(state, patch, 1);
+   return 0;
}
if (patch->is_new > 0 || patch->is_copy) {
-   if (phase == 1) {
-   if (create_file(state, patch))
-   exit(1);
-   }
-   return;
+   if (phase == 1)
+   return create_file(state, patch);
+   return 0;
}
/*
 * Rename or modification boils down to the same
 * thing: remove the old, write the new
 */
-   if (phase == 0) {
-   if (remove_file(state, patch, patch->is_rename))
-   exit(1);
-   }
-   if (phase == 1) {
-   if (create_file(state, patch))
-   exit(1);
-   }
+   if (phase == 0)
+   return remove_file(state, patch, patch->is_rename);
+   if (phase == 1)
+   return create_file(state, patch);
+   return 0;
 }
 
 static int write_out_one_reject(struct apply_state *state, struct patch *patch)
@@ -4392,7 +4385,8 @@ static int write_out_results(struct apply_state *state, 
struct patch *list)
if (l->rejected)
errs = 1;
else {
-   write_out_one_result(state, l, phase);
+   if (write_out_one_result(state, l, phase))
+   exit(1);
if (phase == 1) {
if (write_out_one_reject(state, l))
errs = 1;
-- 
2.9.0.rc2.362.g3cd93d0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v6 16/44] builtin/apply: make gitdiff_*() return -1 on error

[Christian Couder]

To libify `git apply` functionality we have to signal errors to the
caller instead of die()ing.

To do that in a compatible manner with the rest of the error handling
in "builtin/apply.c", gitdiff_*() functions should return -1 instead
of calling die().

A previous patch made it possible for gitdiff_*() functions to
return -1 in case of error. Let's take advantage of that to
make gitdiff_verify_name() return -1 on error, and to have
gitdiff_oldname() and gitdiff_newname() directly return
what gitdiff_verify_name() returns.

Helped-by: Nguyễn Thái Ngọc Duy <pclo...@gmail.com>
Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
---
 builtin/apply.c | 40 +---
 1 file changed, 21 insertions(+), 19 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 1142514..b506369 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -827,54 +827,56 @@ static int gitdiff_hdrend(struct apply_state *state,
 #define DIFF_OLD_NAME 0
 #define DIFF_NEW_NAME 1
 
-static void gitdiff_verify_name(struct apply_state *state,
-   const char *line,
-   int isnull,
-   char **name,
-   int side)
+static int gitdiff_verify_name(struct apply_state *state,
+  const char *line,
+  int isnull,
+  char **name,
+  int side)
 {
if (!*name && !isnull) {
*name = find_name(state, line, NULL, state->p_value, TERM_TAB);
-   return;
+   return 0;
}
 
if (*name) {
int len = strlen(*name);
char *another;
if (isnull)
-   die(_("git apply: bad git-diff - expected /dev/null, 
got %s on line %d"),
-   *name, state->linenr);
+   return error(_("git apply: bad git-diff - expected 
/dev/null, got %s on line %d"),
+*name, state->linenr);
another = find_name(state, line, NULL, state->p_value, 
TERM_TAB);
-   if (!another || memcmp(another, *name, len + 1))
-   die((side == DIFF_NEW_NAME) ?
+   if (!another || memcmp(another, *name, len + 1)) {
+   free(another);
+   return error((side == DIFF_NEW_NAME) ?
_("git apply: bad git-diff - inconsistent new 
filename on line %d") :
_("git apply: bad git-diff - inconsistent old 
filename on line %d"), state->linenr);
+   }
free(another);
} else {
/* expect "/dev/null" */
if (memcmp("/dev/null", line, 9) || line[9] != '\n')
-   die(_("git apply: bad git-diff - expected /dev/null on 
line %d"), state->linenr);
+   return error(_("git apply: bad git-diff - expected 
/dev/null on line %d"), state->linenr);
}
+
+   return 0;
 }
 
 static int gitdiff_oldname(struct apply_state *state,
   const char *line,
   struct patch *patch)
 {
-   gitdiff_verify_name(state, line,
-   patch->is_new, >old_name,
-   DIFF_OLD_NAME);
-   return 0;
+   return gitdiff_verify_name(state, line,
+  patch->is_new, >old_name,
+  DIFF_OLD_NAME);
 }
 
 static int gitdiff_newname(struct apply_state *state,
   const char *line,
   struct patch *patch)
 {
-   gitdiff_verify_name(state, line,
-   patch->is_delete, >new_name,
-   DIFF_NEW_NAME);
-   return 0;
+   return gitdiff_verify_name(state, line,
+  patch->is_delete, >new_name,
+  DIFF_NEW_NAME);
 }
 
 static int gitdiff_oldmode(struct apply_state *state,
-- 
2.9.0.rc2.362.g3cd93d0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v6 21/44] builtin/apply: make add_index_file() return -1 on error

[Christian Couder]

To libify `git apply` functionality we have to signal errors to the
caller instead of die()ing.

To do that in a compatible manner with the rest of the error handling
in "builtin/apply.c", add_index_file() should return -1 instead of
calling die().

Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
---
 builtin/apply.c | 48 +++-
 1 file changed, 31 insertions(+), 17 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 0997384..005ba78 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -4088,11 +4088,11 @@ static int remove_file(struct apply_state *state, 
struct patch *patch, int rmdir
return 0;
 }
 
-static void add_index_file(struct apply_state *state,
-  const char *path,
-  unsigned mode,
-  void *buf,
-  unsigned long size)
+static int add_index_file(struct apply_state *state,
+ const char *path,
+ unsigned mode,
+ void *buf,
+ unsigned long size)
 {
struct stat st;
struct cache_entry *ce;
@@ -4100,7 +4100,7 @@ static void add_index_file(struct apply_state *state,
unsigned ce_size = cache_entry_size(namelen);
 
if (!state->update_index)
-   return;
+   return 0;
 
ce = xcalloc(1, ce_size);
memcpy(ce->name, path, namelen);
@@ -4111,20 +4111,32 @@ static void add_index_file(struct apply_state *state,
const char *s;
 
if (!skip_prefix(buf, "Subproject commit ", ) ||
-   get_sha1_hex(s, ce->sha1))
-   die(_("corrupt patch for submodule %s"), path);
+   get_sha1_hex(s, ce->sha1)) {
+   free(ce);
+   return error(_("corrupt patch for submodule %s"), path);
+   }
} else {
if (!state->cached) {
-   if (lstat(path, ) < 0)
-   die_errno(_("unable to stat newly created file 
'%s'"),
- path);
+   if (lstat(path, ) < 0) {
+   free(ce);
+   return error(_("unable to stat newly "
+  "created file '%s': %s"),
+path, strerror(errno));
+   }
fill_stat_cache_info(ce, );
}
-   if (write_sha1_file(buf, size, blob_type, ce->sha1) < 0)
-   die(_("unable to create backing store for newly created 
file %s"), path);
+   if (write_sha1_file(buf, size, blob_type, ce->sha1) < 0) {
+   free(ce);
+   return error(_("unable to create backing store "
+  "for newly created file %s"), path);
+   }
}
-   if (add_cache_entry(ce, ADD_CACHE_OK_TO_ADD) < 0)
-   die(_("unable to add cache entry for %s"), path);
+   if (add_cache_entry(ce, ADD_CACHE_OK_TO_ADD) < 0) {
+   free(ce);
+   return error(_("unable to add cache entry for %s"), path);
+   }
+
+   return 0;
 }
 
 static int try_create_file(const char *path, unsigned int mode, const char 
*buf, unsigned long size)
@@ -4260,8 +4272,10 @@ static void create_file(struct apply_state *state, 
struct patch *patch)
if (patch->conflicted_threeway) {
if (add_conflicted_stages_file(state, patch))
exit(1);
-   } else
-   add_index_file(state, path, mode, buf, size);
+   } else {
+   if (add_index_file(state, path, mode, buf, size))
+   exit(1);
+   }
 }
 
 /* phase zero is to remove, phase one is to create */
-- 
2.9.0.rc2.362.g3cd93d0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v6 07/44] builtin/apply: make parse_whitespace_option() return -1 instead of die()ing

[Christian Couder]

To libify `git apply` functionality we have to signal errors to the
caller instead of die()ing.

To do that in a compatible manner with the rest of the error handling
in builtin/apply.c, parse_whitespace_option() should return -1 instead
of calling die().

Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
---
 builtin/apply.c | 23 ---
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 2671586..e56e754 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -27,34 +27,34 @@ static const char * const apply_usage[] = {
NULL
 };
 
-static void parse_whitespace_option(struct apply_state *state, const char 
*option)
+static int parse_whitespace_option(struct apply_state *state, const char 
*option)
 {
if (!option) {
state->ws_error_action = warn_on_ws_error;
-   return;
+   return 0;
}
if (!strcmp(option, "warn")) {
state->ws_error_action = warn_on_ws_error;
-   return;
+   return 0;
}
if (!strcmp(option, "nowarn")) {
state->ws_error_action = nowarn_ws_error;
-   return;
+   return 0;
}
if (!strcmp(option, "error")) {
state->ws_error_action = die_on_ws_error;
-   return;
+   return 0;
}
if (!strcmp(option, "error-all")) {
state->ws_error_action = die_on_ws_error;
state->squelch_whitespace_errors = 0;
-   return;
+   return 0;
}
if (!strcmp(option, "strip") || !strcmp(option, "fix")) {
state->ws_error_action = correct_ws_error;
-   return;
+   return 0;
}
-   die(_("unrecognized whitespace option '%s'"), option);
+   return error(_("unrecognized whitespace option '%s'"), option);
 }
 
 static void parse_ignorewhitespace_option(struct apply_state *state,
@@ -4579,7 +4579,8 @@ static int option_parse_whitespace(const struct option 
*opt,
 {
struct apply_state *state = opt->value;
state->whitespace_option = arg;
-   parse_whitespace_option(state, arg);
+   if (parse_whitespace_option(state, arg))
+   exit(1);
return 0;
 }
 
@@ -4613,8 +4614,8 @@ static void init_apply_state(struct apply_state *state,
strbuf_init(>root, 0);
 
git_apply_config();
-   if (apply_default_whitespace)
-   parse_whitespace_option(state, apply_default_whitespace);
+   if (apply_default_whitespace && parse_whitespace_option(state, 
apply_default_whitespace))
+   exit(1);
if (apply_default_ignorewhitespace)
parse_ignorewhitespace_option(state, 
apply_default_ignorewhitespace);
 }
-- 
2.9.0.rc2.362.g3cd93d0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v6 04/44] builtin/apply: make find_header() return -1 instead of die()ing

[Christian Couder]

To libify `git apply` functionality we have to signal errors to the
caller instead of die()ing.

To do that in a compatible manner with the rest of the error handling
in builtin/apply.c, find_header() should return -1 instead of calling
die().

Unfortunately find_header() already returns -1 when no header is found,
so let's make it return -2 instead in this case.

Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
---
 builtin/apply.c   | 33 ++---
 t/t4254-am-corrupt.sh |  2 +-
 2 files changed, 23 insertions(+), 12 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 2ff8450..630c01c 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -1419,6 +1419,14 @@ static int parse_fragment_header(const char *line, int 
len, struct fragment *fra
return offset;
 }
 
+/*
+ * Find file diff header
+ *
+ * Returns:
+ *  -1 in case of error
+ *  -2 if no header was found
+ *   the size of the header in bytes (called "offset") otherwise
+ */
 static int find_header(struct apply_state *state,
   const char *line,
   unsigned long size,
@@ -1452,8 +1460,8 @@ static int find_header(struct apply_state *state,
struct fragment dummy;
if (parse_fragment_header(line, len, ) < 0)
continue;
-   die(_("patch fragment without header at line %d: %.*s"),
-   state->linenr, (int)len-1, line);
+   return error(_("patch fragment without header at line 
%d: %.*s"),
+state->linenr, (int)len-1, line);
}
 
if (size < len + 6)
@@ -1469,18 +1477,18 @@ static int find_header(struct apply_state *state,
continue;
if (!patch->old_name && !patch->new_name) {
if (!patch->def_name)
-   die(Q_("git diff header lacks filename 
information when removing "
-  "%d leading pathname component 
(line %d)",
-  "git diff header lacks filename 
information when removing "
-  "%d leading pathname components 
(line %d)",
-  state->p_value),
-   state->p_value, state->linenr);
+   return error(Q_("git diff header lacks 
filename information when removing "
+   "%d leading pathname 
component (line %d)",
+   "git diff header lacks 
filename information when removing "
+   "%d leading pathname 
components (line %d)",
+   state->p_value),
+state->p_value, 
state->linenr);
patch->old_name = xstrdup(patch->def_name);
patch->new_name = xstrdup(patch->def_name);
}
if (!patch->is_delete && !patch->new_name)
-   die("git diff header lacks filename information 
"
-   "(line %d)", state->linenr);
+   return error("git diff header lacks filename 
information "
+"(line %d)", state->linenr);
patch->is_toplevel_relative = 1;
*hdrsize = git_hdr_len;
return offset;
@@ -1505,7 +1513,7 @@ static int find_header(struct apply_state *state,
state->linenr += 2;
return offset;
}
-   return -1;
+   return -2;
 }
 
 static void record_ws_error(struct apply_state *state,
@@ -1996,6 +2004,9 @@ static int parse_chunk(struct apply_state *state, char 
*buffer, unsigned long si
int hdrsize, patchsize;
int offset = find_header(state, buffer, size, , patch);
 
+   if (offset == -1)
+   exit(1);
+
if (offset < 0)
return offset;
 
diff --git a/t/t4254-am-corrupt.sh b/t/t4254-am-corrupt.sh
index 85716dd..9bd7dd2 100755
--- a/t/t4254-am-corrupt.sh
+++ b/t/t4254-am-corrupt.sh
@@ -29,7 +29,7 @@ test_expect_success 'try to apply corrupted patch' '
 '
 
 test_expect_success 'compare diagnostic; ensure file is still here' '
-   echo "fatal: git diff header lacks filename information (line 4)" 
>ex

[PATCH v6 01/44] apply: move 'struct apply_state' to apply.h

[Christian Couder]

To libify `git apply` functionality we must make 'struct apply_state'
usable outside "builtin/apply.c".

Let's do that by creating a new "apply.h" and moving
'struct apply_state' there.

Helped-by: Eric Sunshine <sunsh...@sunshineco.com>
Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
---
 apply.h | 100 
 builtin/apply.c |  98 +-
 2 files changed, 101 insertions(+), 97 deletions(-)
 create mode 100644 apply.h

diff --git a/apply.h b/apply.h
new file mode 100644
index 000..9a98eae
--- /dev/null
+++ b/apply.h
@@ -0,0 +1,100 @@
+#ifndef APPLY_H
+#define APPLY_H
+
+enum ws_error_action {
+   nowarn_ws_error,
+   warn_on_ws_error,
+   die_on_ws_error,
+   correct_ws_error
+};
+
+enum ws_ignore {
+   ignore_ws_none,
+   ignore_ws_change
+};
+
+/*
+ * We need to keep track of how symlinks in the preimage are
+ * manipulated by the patches.  A patch to add a/b/c where a/b
+ * is a symlink should not be allowed to affect the directory
+ * the symlink points at, but if the same patch removes a/b,
+ * it is perfectly fine, as the patch removes a/b to make room
+ * to create a directory a/b so that a/b/c can be created.
+ *
+ * See also "struct string_list symlink_changes" in "struct
+ * apply_state".
+ */
+#define SYMLINK_GOES_AWAY 01
+#define SYMLINK_IN_RESULT 02
+
+struct apply_state {
+   const char *prefix;
+   int prefix_length;
+
+   /* These are lock_file related */
+   struct lock_file *lock_file;
+   int newfd;
+
+   /* These control what gets looked at and modified */
+   int apply; /* this is not a dry-run */
+   int cached; /* apply to the index only */
+   int check; /* preimage must match working tree, don't actually apply */
+   int check_index; /* preimage must match the indexed version */
+   int update_index; /* check_index && apply */
+
+   /* These control cosmetic aspect of the output */
+   int diffstat; /* just show a diffstat, and don't actually apply */
+   int numstat; /* just show a numeric diffstat, and don't actually apply 
*/
+   int summary; /* just report creation, deletion, etc, and don't actually 
apply */
+
+   /* These boolean parameters control how the apply is done */
+   int allow_overlap;
+   int apply_in_reverse;
+   int apply_with_reject;
+   int apply_verbosely;
+   int no_add;
+   int threeway;
+   int unidiff_zero;
+   int unsafe_paths;
+
+   /* Other non boolean parameters */
+   const char *fake_ancestor;
+   const char *patch_input_file;
+   int line_termination;
+   struct strbuf root;
+   int p_value;
+   int p_value_known;
+   unsigned int p_context;
+
+   /* Exclude and include path parameters */
+   struct string_list limit_by_name;
+   int has_include;
+
+   /* Various "current state" */
+   int linenr; /* current line number */
+   struct string_list symlink_changes; /* we have to track symlinks */
+
+   /*
+* For "diff-stat" like behaviour, we keep track of the biggest change
+* we've seen, and the longest filename. That allows us to do simple
+* scaling.
+*/
+   int max_change;
+   int max_len;
+
+   /*
+* Records filenames that have been touched, in order to handle
+* the case where more than one patches touch the same file.
+*/
+   struct string_list fn_table;
+
+   /* These control whitespace errors */
+   enum ws_error_action ws_error_action;
+   enum ws_ignore ws_ignore_action;
+   const char *whitespace_option;
+   int whitespace_error;
+   int squelch_whitespace_errors;
+   int applied_after_fixing_ws;
+};
+
+#endif
diff --git a/builtin/apply.c b/builtin/apply.c
index ecb7f1b..3a0c53a 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -20,103 +20,7 @@
 #include "xdiff-interface.h"
 #include "ll-merge.h"
 #include "rerere.h"
-
-enum ws_error_action {
-   nowarn_ws_error,
-   warn_on_ws_error,
-   die_on_ws_error,
-   correct_ws_error
-};
-
-
-enum ws_ignore {
-   ignore_ws_none,
-   ignore_ws_change
-};
-
-/*
- * We need to keep track of how symlinks in the preimage are
- * manipulated by the patches.  A patch to add a/b/c where a/b
- * is a symlink should not be allowed to affect the directory
- * the symlink points at, but if the same patch removes a/b,
- * it is perfectly fine, as the patch removes a/b to make room
- * to create a directory a/b so that a/b/c can be created.
- *
- * See also "struct string_list symlink_changes" in "struct
- * apply_state".
- */
-#define SYMLINK_GOES_AWAY 01
-#define SYMLINK_IN_RESULT 02
-
-struct apply_state {
-   const char *prefix;
-   int prefix_leng

[PATCH v6 02/44] builtin/apply: make apply_patch() return -1 instead of die()ing

[Christian Couder]

To libify `git apply` functionality we have to signal errors
to the caller instead of die()ing.

As a first step in this direction, let's make apply_patch() return
-1 in case of errors instead of dying. For now its only caller
apply_all_patches() will exit(1) when apply_patch() return -1.

In a later patch, apply_all_patches() will return -1 too instead of
exiting.

Helped-by: Eric Sunshine <sunsh...@sunshineco.com>
Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
---
 builtin/apply.c | 54 +++---
 1 file changed, 39 insertions(+), 15 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 3a0c53a..598e479 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -4404,6 +4404,14 @@ static struct lock_file lock_file;
 #define INACCURATE_EOF (1<<0)
 #define RECOUNT(1<<1)
 
+/*
+ * Try to apply a patch.
+ *
+ * Returns:
+ *  -1 if an error happened
+ *   0 if the patch applied
+ *   1 if the patch did not apply
+ */
 static int apply_patch(struct apply_state *state,
   int fd,
   const char *filename,
@@ -4413,6 +4421,7 @@ static int apply_patch(struct apply_state *state,
struct strbuf buf = STRBUF_INIT; /* owns the patch text */
struct patch *list = NULL, **listp = 
int skipped_patch = 0;
+   int res = 0;
 
state->patch_input_file = filename;
read_patch_file(, fd);
@@ -4445,8 +4454,10 @@ static int apply_patch(struct apply_state *state,
offset += nr;
}
 
-   if (!list && !skipped_patch)
-   die(_("unrecognized input"));
+   if (!list && !skipped_patch) {
+   res = error(_("unrecognized input"));
+   goto end;
+   }
 
if (state->whitespace_error && (state->ws_error_action == 
die_on_ws_error))
state->apply = 0;
@@ -4455,21 +4466,22 @@ static int apply_patch(struct apply_state *state,
if (state->update_index && state->newfd < 0)
state->newfd = hold_locked_index(state->lock_file, 1);
 
-   if (state->check_index) {
-   if (read_cache() < 0)
-   die(_("unable to read index file"));
+   if (state->check_index && read_cache() < 0) {
+   res = error(_("unable to read index file"));
+   goto end;
}
 
if ((state->check || state->apply) &&
check_patch_list(state, list) < 0 &&
-   !state->apply_with_reject)
-   exit(1);
+   !state->apply_with_reject) {
+   res = -1;
+   goto end;
+   }
 
if (state->apply && write_out_results(state, list)) {
-   if (state->apply_with_reject)
-   exit(1);
/* with --3way, we still need to write the index out */
-   return 1;
+   res = state->apply_with_reject ? -1 : 1;
+   goto end;
}
 
if (state->fake_ancestor)
@@ -4484,10 +4496,11 @@ static int apply_patch(struct apply_state *state,
if (state->summary)
summary_patch_list(list);
 
+end:
free_patch_list(list);
strbuf_release();
string_list_clear(>fn_table, 0);
-   return 0;
+   return res;
 }
 
 static void git_apply_config(void)
@@ -4625,6 +4638,7 @@ static int apply_all_patches(struct apply_state *state,
 int options)
 {
int i;
+   int res;
int errs = 0;
int read_stdin = 1;
 
@@ -4633,7 +4647,10 @@ static int apply_all_patches(struct apply_state *state,
int fd;
 
if (!strcmp(arg, "-")) {
-   errs |= apply_patch(state, 0, "", options);
+   res = apply_patch(state, 0, "", options);
+   if (res < 0)
+   exit(1);
+   errs |= res;
read_stdin = 0;
continue;
} else if (0 < state->prefix_length)
@@ -4646,12 +4663,19 @@ static int apply_all_patches(struct apply_state *state,
die_errno(_("can't open patch '%s'"), arg);
read_stdin = 0;
set_default_whitespace_mode(state);
-   errs |= apply_patch(state, fd, arg, options);
+   res = apply_patch(state, fd, arg, options);
+   if (res < 0)
+   exit(1);
+   errs |= res;
close(fd);
}
set_default_whitespace_mode(state);
-   if (read_stdin)
-   errs |= apply_patch(state, 0, "", options);
+   if (read_stdin) {
+   res 

[PATCH v6 00/44] libify apply and use lib in am, part 2

[Christian Couder]

com/chriscool/git/commits/libify-apply-use-in-am54

Performance numbers
~~~

Only tests on Linux have been performed using a very early version of
this series. It could be interesting to test on other platforms
especially Windows and perhaps OSX too.

  - Around mid April Ævar did a huge many-hundred commit rebase on the
kernel with untracked cache.

command: git rebase --onto 1993b17 52bef0c 29dde7c

Vanilla "next" without split index:1m54.953s
Vanilla "next" with split index:   1m22.476s
This series on top of "next" without split index:  1m12.034s
This series on top of "next" with split index: 0m15.678s

Ævar used his Debian laptop with SSD.

  - Around mid April I tested rebasing 13 commits in Booking.com's
monorepo on a Red Hat 6.5 server with split-index and
GIT_TRACE_PERFORMANCE=1.

With Git v2.8.0, the rebase took 6.375888383 s, with the git am
command launched by the rebase command taking 3.705677431 s.

With this series on top of next, the rebase took 3.044529494 s, with
the git am command launched by the rebase command taking 0.583521168
s.

Christian Couder (44):
  apply: move 'struct apply_state' to apply.h
  builtin/apply: make apply_patch() return -1 instead of die()ing
  builtin/apply: read_patch_file() return -1 instead of die()ing
  builtin/apply: make find_header() return -1 instead of die()ing
  builtin/apply: make parse_chunk() return a negative integer on error
  builtin/apply: make parse_single_patch() return -1 on error
  builtin/apply: make parse_whitespace_option() return -1 instead of
die()ing
  builtin/apply: make parse_ignorewhitespace_option() return -1 instead
of die()ing
  builtin/apply: move init_apply_state() to apply.c
  apply: make init_apply_state() return -1 instead of exit()ing
  builtin/apply: make check_apply_state() return -1 instead of die()ing
  builtin/apply: move check_apply_state() to apply.c
  builtin/apply: make apply_all_patches() return -1 on error
  builtin/apply: make parse_traditional_patch() return -1 on error
  builtin/apply: make gitdiff_*() return 1 at end of header
  builtin/apply: make gitdiff_*() return -1 on error
  builtin/apply: change die_on_unsafe_path() to check_unsafe_path()
  builtin/apply: make build_fake_ancestor() return -1 on error
  builtin/apply: make remove_file() return -1 on error
  builtin/apply: make add_conflicted_stages_file() return -1 on error
  builtin/apply: make add_index_file() return -1 on error
  builtin/apply: make create_file() return -1 on error
  builtin/apply: make write_out_one_result() return -1 on error
  builtin/apply: make write_out_results() return -1 on error
  builtin/apply: make try_create_file() return -1 on error
  builtin/apply: make create_one_file() return -1 on error
  builtin/apply: rename option parsing functions
  apply: rename and move opt constants to apply.h
  Move libified code from builtin/apply.c to apply.{c,h}
  apply: make some parsing functions static again
  run-command: make dup_devnull() non static
  environment: add set_index_file()
  builtin/am: use apply api in run_apply()
  write_or_die: use warning() instead of fprintf(stderr, ...)
  apply: add 'be_silent' variable to 'struct apply_state'
  apply: make 'be_silent' incompatible with 'apply_verbosely'
  apply: don't print on stdout when be_silent is set
  usage: add set_warn_routine()
  usage: add get_error_routine() and get_warn_routine()
  apply: change error_routine when be_silent is set
  am: use be_silent in 'struct apply_state' to shut up applying patches
  run-command: make dup_devnull() static again
  builtin/apply: add a cli option for be_silent
  apply: use error_errno() where possible

 Makefile   |1 +
 apply.c| 4868 
 apply.h|  133 ++
 builtin/am.c   |   91 +-
 builtin/apply.c| 4815 +--
 cache.h|1 +
 environment.c  |   10 +
 git-compat-util.h  |3 +
 run-command.c  |2 +-
 t/t4012-diff-binary.sh |4 +-
 t/t4254-am-corrupt.sh  |2 +-
 usage.c|   15 +
 write_or_die.c |6 +-
 13 files changed, 5132 insertions(+), 4819 deletions(-)
 create mode 100644 apply.c
 create mode 100644 apply.h

-- 
2.9.0.rc2.362.g3cd93d0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 63/94] builtin/apply: make apply_all_patches() return -1 on error

[Christian Couder]

On Wed, Jun 8, 2016 at 7:44 PM, Eric Sunshine <sunsh...@sunshineco.com> wrote:
> On Wed, Jun 8, 2016 at 12:37 PM, Christian Couder
> <christian.cou...@gmail.com> wrote:
>> On Mon, May 16, 2016 at 5:44 AM, Eric Sunshine <sunsh...@sunshineco.com> 
>> wrote:
>>> On Wed, May 11, 2016 at 9:17 AM, Christian Couder
>>> <christian.cou...@gmail.com> wrote:
>>>> if (state->update_index) {
>>>> -   if (write_locked_index(_index, state->lock_file, 
>>>> COMMIT_LOCK))
>>>> -   die(_("Unable to write new index file"));
>>>> +   res = write_locked_index(_index, state->lock_file, 
>>>> COMMIT_LOCK);
>>>> state->newfd = -1;
>>>
>>> Does write_locked_index() unconditionally close the file descriptor
>>> even when an error occurs? If not, then isn't this potentially leaking
>>> 'newfd'?
>>>
>>> (My very cursory read of write_locked_index() seems to reveal that the
>>> file descriptor may indeed remain open upon index write failure.)
>>
>> You are right, it is leaking newfd if write_locked_index() fails.
>> The solution to that is to call `rollback_lock_file(state->lock_file)`
>> and the following patch was supposed to do that:
>>
>> [PATCH v2 82/94] apply: roll back index lock file in case of error
>>
>> but it would do that only if `state->newfd >= 0` so we should set
>> state->newfd to -1 only if write_locked_index() succeeds.
>>
>> I will fix this.
>>
>> I am also going to add a comment to this patch saying that this patch
>> needs a following patch to call rollback_lock_file(state->lock_file)
>> in case of errors.
>>
>> Or if you prefer, I can squash the patch that call
>> rollback_lock_file(state->lock_file) in case of errors into this
>> patch.
>
> Squashing may indeed be preferable over leaving it in a "broken" state
> until the next patch, though I haven't thought too hard about it.
> Alternately, can the two patches somehow be swapped?

I just squashed them for now as the result looks reasonable.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 00/94] libify apply and use lib in am

[Christian Couder]

On Thu, Jun 9, 2016 at 11:10 PM, Johannes Sixt <j...@kdbg.org> wrote:
> Am 12.05.2016 um 20:02 schrieb Christian Couder:
>>
>>> I'll also use it in production for a while, although I am not a git-am
>>> consumer nor do I use git-rebase without -i, hence, my tests will
>>> probably
>>> only show that there is no bad fall-out.
>
>
> Meanwhile, I have retrained my muscle memory to stop before typing "-i"
> after "rebase" for an opportunity to consider whether bare rebase can be
> used.
>
> What should I say? I am impressed. It's like 100 times faster than rebase -i
> (on Windows). I'm now using it whenever I can, and more often than not I
> plan my rebase workflow so that I can go ahead without -i.

:-D

Thanks for the report again!

> Can't wait to test a re-roll on top of cc/apply-introduce-state!

It should happen really soon now...
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 00/94] libify apply and use lib in am

[Christian Couder]

Hi Dscho,

On Fri, Jun 10, 2016 at 9:01 AM, Johannes Schindelin
 wrote:
> Hi Hannes,
>
> On Thu, 9 Jun 2016, Johannes Sixt wrote:
>
>> Meanwhile, I have retrained my muscle memory to stop before typing "-i" after
>> "rebase" for an opportunity to consider whether bare rebase can be used.
>>
>> What should I say? I am impressed. It's like 100 times faster than rebase -i
>> (on Windows). I'm now using it whenever I can, and more often than not I plan
>> my rebase workflow so that I can go ahead without -i.
>
> That only means that I have to finalize my rebase--helper work (which I am
> busy doing, I am at the valgrind stage).
>
> I wonder whether that "100x" is a reliable number? ;-) FWIW I can measure
> something like a 4x speedup of the interactive rebase on Windows when
> running with the rebase--helper, and it is still noticably faster in my
> Linux VM, too.
>
>> Can't wait to test a re-roll on top of cc/apply-introduce-state!
>
> I lost track in the meantime: were those issues with unclosed file handles
> and unreleased memory in the error code paths addressed systematically? My
> mail about that seems to have been left unanswered, almost as if my
> concerns had been hand-waved away...

Haven't I answered to your email in this thread:

http://thread.gmane.org/gmane.comp.version-control.git/292403/

?

> If those issues have indeed been addressed properly, and a public
> repository reliably has the newest iteration of that patch series in a
> branch without a versioned name, I will be happy to test it in Git for
> Windows' SDK again.

This is the newest iteration:

https://github.com/chriscool/git/commits/libify-apply-use-in-am65

Thanks for testing,
Christian.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 48/49] builtin/apply: move 'lock_file' global into 'struct apply_state'

[Christian Couder]

On Wed, Jun 1, 2016 at 7:23 PM, Junio C Hamano <gits...@pobox.com> wrote:
> Christian Couder <christian.cou...@gmail.com> writes:
>
>> We cannot have a 'struct lock_file' allocated on the stack, as lockfile.c
>> keeps a linked list of all created lock_file structures. So let's make the
>> 'lock_file' variable a pointer to a 'struct lock_file'
>
> Good.
>
>> As the same instance of this struct can be reused, let's add an argument
>> to init_apply_state(), so that the caller can supply the same instance to
>> different calls.
>
> Good.
>
>> And let's alloc an instance in init_apply_state(), if the
>> caller doesn't want to supply one.
>
> This is questionable.
>
>> -static struct lock_file lock_file;
>
> I'd rather leave this as-is, and pass the pointer to it to
> init_apply_state().  For the purpose of "cmd_apply()" which is the
> first user of the "(semi-)libified apply API", that reads a single
> patch and applies it before exiting, that is sufficient.

Ok, I will leave this as-is.

> As to the "if the caller does not want to supply one, we will
> allocate one that will definitely be leaked", I am mildly opposed.
>
> By making it the responsibility of the caller, whenever a new caller
> of the libified apply API is written, those who write it is _forced_
> to think about not leaking the lockfile structure, which is a good
> thing.

Ok, I will remove the allocation in case the caller pass NULL.

> Other than that, all 49 patches look sensible to me.

Ok, I think I will resend only this patch (48/49) with the changes you
suggest, and maybe the next one 49/49 to avoid spamming the list.

> Thanks for working on this.

Thanks,
Christian.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC/PATCH 2/8] external odb foreach

[Christian Couder]

From: Jeff King 

---
 external-odb.c | 14 ++
 external-odb.h |  6 ++
 odb-helper.c   | 15 +++
 odb-helper.h   |  4 
 4 files changed, 39 insertions(+)

diff --git a/external-odb.c b/external-odb.c
index 1ccfa99..42978a3 100644
--- a/external-odb.c
+++ b/external-odb.c
@@ -113,3 +113,17 @@ int external_odb_fetch_object(const unsigned char *sha1)
 
return -1;
 }
+
+int external_odb_for_each_object(each_external_object_fn fn, void *data)
+{
+   struct odb_helper *o;
+
+   external_odb_init();
+
+   for (o = helpers; o; o = o->next) {
+   int r = odb_helper_for_each_object(o, fn, data);
+   if (r)
+   return r;
+   }
+   return 0;
+}
diff --git a/external-odb.h b/external-odb.h
index 2397477..cea8570 100644
--- a/external-odb.h
+++ b/external-odb.h
@@ -5,4 +5,10 @@ const char *external_odb_root(void);
 int external_odb_has_object(const unsigned char *sha1);
 int external_odb_fetch_object(const unsigned char *sha1);
 
+typedef int (*each_external_object_fn)(const unsigned char *sha1,
+  enum object_type type,
+  unsigned long size,
+  void *data);
+int external_odb_for_each_object(each_external_object_fn, void *);
+
 #endif /* EXTERNAL_ODB_H */
diff --git a/odb-helper.c b/odb-helper.c
index 7029a59..045cf6f 100644
--- a/odb-helper.c
+++ b/odb-helper.c
@@ -235,5 +235,20 @@ int odb_helper_fetch_object(struct odb_helper *o, const 
unsigned char *sha1,
return -1;
}
 
+   return 0;
+}
+
+int odb_helper_for_each_object(struct odb_helper *o,
+  each_external_object_fn fn,
+  void *data)
+{
+   int i;
+   for (i = 0; i < o->have_nr; i++) {
+   struct odb_helper_object *obj = >have[i];
+   int r = fn(obj->sha1, obj->type, obj->size, data);
+   if (r)
+   return r;
+   }
+
return 0;
 }
diff --git a/odb-helper.h b/odb-helper.h
index 0f704f9..8c3916d 100644
--- a/odb-helper.h
+++ b/odb-helper.h
@@ -1,6 +1,8 @@
 #ifndef ODB_HELPER_H
 #define ODB_HELPER_H
 
+#include "external-odb.h"
+
 struct odb_helper {
const char *name;
const char *cmd;
@@ -21,5 +23,7 @@ struct odb_helper *odb_helper_new(const char *name, int 
namelen);
 int odb_helper_has_object(struct odb_helper *o, const unsigned char *sha1);
 int odb_helper_fetch_object(struct odb_helper *o, const unsigned char *sha1,
int fd);
+int odb_helper_for_each_object(struct odb_helper *o,
+  each_external_object_fn, void *);
 
 #endif /* ODB_HELPER_H */
-- 
2.9.0.rc2.362.g3cd93d0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html