[bug] git-check-ignore and file names with unicode chars in name - sys-out filename is corrupted

[Paul Hammant]

Reproduction:

  $ echo "*.ignoreme" >> .gitignore
  # (and commit)
  $ touch "fooo-€.ignoreme"
  $ find . -print | grep fooo | xargs git check-ignore
  "./fooo-\342\202\254.ignoreme"

You could view that git-check-ignore isn't corrupting anything, it is
just outputting another form for the file name (octal escaped), but it
doesn't need to change it at all, and its causing downstream problems
in bash scripting.

Of course this may get munged by gmail or a list manager. In the text
above, you should see unicode char "euro sign" to the right of a dash,
and the left of .ignoreme

Git version is 2.9.2

-ph
--
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: [PATCHv2 0/6] git clone: Marry --recursive and --reference

[Jacob Keller]

On Mon, Aug 8, 2016 at 9:08 PM, Stefan Beller  wrote:
>
> v3:
>
> Thanks to Junios critial questions regarding the design, I took a step back
> to look at the bigger picture.
>
> --super-reference sounds confusing. (what is the super referring to?)
> So drop that approach.
>
> Instead we'll compute where the reference might be in the superproject scope
> and ask the submodule clone operation to consider an optional reference.
> If the referenced alternate is not there, we'll just warn about it and
> carry on.
>

This makes a lot more sense to me, and being able to use
"reference-if-able" for regular clones may be a useful addition as
well.

I looked over the other patches and didn't see anything obviously
wrong, but take that with a grain of salt as I didn't spend a lot of
time at it.

Thanks,
Jake
--
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

HELLO

[George Oloni]

Hello,

I hope this Message finds you well.

To start with my name is George Oloni. I am seeking for your cooperation to 
invest in your Country.

Please kindly respond to this message as to enable me to gives you More Details 
about my investment proposal.

Warm Regards
Mr. George Oloni
--
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 submission process, was Re: [PATCH v6 06/16] merge_recursive: abort properly upon errors

[Duy Nguyen]

On Tue, Aug 9, 2016 at 12:20 AM, Michael Haggerty  wrote:
> On 08/04/2016 05:58 PM, Johannes Schindelin wrote:
>> [...]
>> Even requiring every contributor to register with GitHub would be too much
>> of a limitation, I would wager.
>> [...]
>
> Is it *really* so insane to consider moving collaboration on the Git
> project to GitHub or some other similar platform?

In the very unlikely event that github is shut down, how do we get all
review comments out of it, assuming that we will use pull requests for
review?
-- 
Duy
--
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

[PATCHv2 0/6] git clone: Marry --recursive and --reference

[Stefan Beller]

v3:

Thanks to Junios critial questions regarding the design, I took a step back
to look at the bigger picture. 

--super-reference sounds confusing. (what is the super referring to?)
So drop that approach.

Instead we'll compute where the reference might be in the superproject scope
and ask the submodule clone operation to consider an optional reference.
If the referenced alternate is not there, we'll just warn about it and
carry on.



* fixed the style in patch 2.

* fixed another bug in the last patch, that is unrelated, but would have helped
  me a lot.

Thanks,
Stefan

 Documentation/git-clone.txt|   9 -
 builtin/clone.c|  36 
 builtin/submodule--helper.c| 105 
-
 git-submodule.sh   |   2 +-
 t/t7408-submodule-reference.sh | 162 
--
 5 files changed, 217 insertions(+), 97 deletions(-)

v2:
 * fixed the p1,2 cleanups
 * added documentation to patches 5,6
 * improved commit message in v4
 
 Thanks,
 Stefan
 
v1:
 
 Currently when cloning a superproject with --recursive and --reference
 only the superproject learns about its alternates. The submodules are
 cloned independently, which may incur lots of network costs.
 
 Assume that the reference repository has the submodules at the same
 paths as the to-be-cloned submodule and try to setup alternates from
 there.
 
 Some submodules in the referenced superproject may not be there, 
 (they are just not initialized/cloned/checked out), which yields
 an error for now. In future work we may want to soften the alternate
 check and not die in the clone when one of the given alternates doesn't
 exist.
 
 patch 1,2 are modernizing style of t7408, 
 patches 3,4 are not strictly necessary, but I think it is a good thing
 to not leave the submodule related C code in a crippled state (i.e.
 allowing only one reference). The shell code would also need this update,
 but it looked ugly to me, so I postpone it until more of the submodule code
 is written in C. 
 
 Thanks,
 Stefan 

Stefan Beller (6):
  t7408: modernize style
  t7408: merge short tests, factor out testing method
  submodule--helper module-clone: allow multiple references
  submodule--helper update-clone: allow multiple references
  submodule update: add super-reference flag
  clone: reference flag is used for submodules as well

 builtin/clone.c|  22 --
 builtin/submodule--helper.c|  45 
 git-submodule.sh   |  12 +++-
 t/t7408-submodule-reference.sh | 153 +++--
 4 files changed, 147 insertions(+), 85 deletions(-)

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

[PATCHv3 6/9] clone: implement optional references

[Stefan Beller]

In a later patch we want to try to create alternates for
all submodules, but they might not exist in the referenced
superproject. So add a way to skip the non existing references
and report them.

Signed-off-by: Stefan Beller 
---
 Documentation/git-clone.txt |  5 -
 builtin/clone.c | 32 ++--
 2 files changed, 30 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index ec41d3d..e316c4b 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -90,13 +90,16 @@ If you want to break the dependency of a repository cloned 
with `-s` on
 its source repository, you can simply run `git repack -a` to copy all
 objects from the source repository into a pack in the cloned repository.
 
---reference ::
+--reference[-if-able] ::
If the reference repository is on the local machine,
automatically setup `.git/objects/info/alternates` to
obtain objects from the reference repository.  Using
an already existing repository as an alternate will
require fewer objects to be copied from the repository
being cloned, reducing network and local storage costs.
+   When using the `--reference-if-able`, a non existing
+   directory is skipped with a warning instead of aborting
+   the clone.
 +
 *NOTE*: see the NOTE for the `--shared` option, and also the
 `--dissociate` option.
diff --git a/builtin/clone.c b/builtin/clone.c
index 052a769..8f3c4d4 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -51,6 +51,7 @@ static int option_progress = -1;
 static enum transport_family family;
 static struct string_list option_config = STRING_LIST_INIT_NODUP;
 static struct string_list option_required_reference = STRING_LIST_INIT_NODUP;
+static struct string_list option_optional_reference = STRING_LIST_INIT_NODUP;
 static int option_dissociate;
 static int max_jobs = -1;
 
@@ -81,6 +82,8 @@ static struct option builtin_clone_options[] = {
   N_("directory from which templates will be used")),
OPT_STRING_LIST(0, "reference", _required_reference, N_("repo"),
N_("reference repository")),
+   OPT_STRING_LIST(0, "reference-if-able", _optional_reference,
+   N_("repo"), N_("reference repository")),
OPT_BOOL(0, "dissociate", _dissociate,
 N_("use --reference only while cloning")),
OPT_STRING('o', "origin", _origin, N_("name"),
@@ -283,11 +286,22 @@ static void strip_trailing_slashes(char *dir)
 static int add_one_reference(struct string_list_item *item, void *cb_data)
 {
char *ref_git;
-   const char *repo;
+   const char *repo, *ref_git_s;
+   int *required = cb_data;
struct strbuf alternate = STRBUF_INIT;
 
-   /* Beware: read_gitfile(), real_path() and mkpath() return static 
buffer */
-   ref_git = xstrdup(real_path(item->string));
+   ref_git_s = *required ?
+   real_path(item->string) :
+   real_path_if_valid(item->string);
+   if (!ref_git_s) {
+   warning(_("Not using proposed alternate %s"), item->string);
+   return 0;
+   } else
+   /*
+* Beware: read_gitfile(), real_path() and mkpath()
+* return static buffer
+*/
+   ref_git = xstrdup(ref_git_s);
 
repo = read_gitfile(ref_git);
if (!repo)
@@ -304,7 +318,8 @@ static int add_one_reference(struct string_list_item *item, 
void *cb_data)
} else if (!is_directory(mkpath("%s/objects", ref_git))) {
struct strbuf sb = STRBUF_INIT;
if (get_common_dir(, ref_git))
-   die(_("reference repository '%s' as a linked checkout 
is not supported yet."),
+   die(_("reference repository '%s' as a "
+ "linked checkout is not supported yet."),
item->string);
die(_("reference repository '%s' is not a local repository."),
item->string);
@@ -325,7 +340,12 @@ static int add_one_reference(struct string_list_item 
*item, void *cb_data)
 
 static void setup_reference(void)
 {
-   for_each_string_list(_required_reference, add_one_reference, 
NULL);
+   int required = 1;
+   for_each_string_list(_required_reference,
+add_one_reference, );
+   required = 0;
+   for_each_string_list(_optional_reference,
+add_one_reference, );
 }
 
 static void copy_alternates(struct strbuf *src, struct strbuf *dst,
@@ -977,7 +997,7 @@ int cmd_clone(int argc, const char **argv, const char 
*prefix)
git_config_set(key.buf, repo);
strbuf_reset();
 
-   if (option_required_reference.nr)
+   if (option_required_reference.nr || option_optional_reference.nr)

[PATCHv3 7/9] submodule helper: pass through --reference-if-able

[Stefan Beller]

In a later patch we need to pass optional references
through to git clone, so add the capability for it.

Signed-off-by: Stefan Beller 
---
 builtin/submodule--helper.c | 20 
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 7096848..f360473 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -442,7 +442,8 @@ static int module_name(int argc, const char **argv, const 
char *prefix)
 }
 
 static int clone_submodule(const char *path, const char *gitdir, const char 
*url,
-  const char *depth, struct string_list *reference, 
int quiet)
+  const char *depth, struct string_list *reference,
+  struct string_list *reference_if_able, int quiet)
 {
struct child_process cp;
child_process_init();
@@ -459,6 +460,12 @@ static int clone_submodule(const char *path, const char 
*gitdir, const char *url
argv_array_pushl(, "--reference",
 item->string, NULL);
}
+   if (reference_if_able->nr) {
+   struct string_list_item *item;
+   for_each_string_list_item(item, reference_if_able)
+   argv_array_pushl(, "--reference-if-able",
+item->string, NULL);
+   }
if (gitdir && *gitdir)
argv_array_pushl(, "--separate-git-dir", gitdir, NULL);
 
@@ -481,6 +488,7 @@ static int module_clone(int argc, const char **argv, const 
char *prefix)
struct strbuf rel_path = STRBUF_INIT;
struct strbuf sb = STRBUF_INIT;
struct string_list reference = STRING_LIST_INIT_NODUP;
+   struct string_list reference_if_able = STRING_LIST_INIT_NODUP;
 
struct option module_clone_options[] = {
OPT_STRING(0, "prefix", ,
@@ -498,6 +506,9 @@ static int module_clone(int argc, const char **argv, const 
char *prefix)
OPT_STRING_LIST(0, "reference", ,
   N_("repo"),
   N_("reference repository")),
+   OPT_STRING_LIST(0, "reference-if-able", _if_able,
+  N_("repo"),
+  N_("reference repository")),
OPT_STRING(0, "depth", ,
   N_("string"),
   N_("depth for shallow clones")),
@@ -507,8 +518,8 @@ static int module_clone(int argc, const char **argv, const 
char *prefix)
 
const char *const git_submodule_helper_usage[] = {
N_("git submodule--helper clone [--prefix=] [--quiet] "
-  "[--reference ] [--name ] [--depth 
] "
-  "--url  --path "),
+  "[--reference[-if-able] ] [--name ] "
+  "[--depth ] --url  --path "),
NULL
};
 
@@ -532,7 +543,8 @@ static int module_clone(int argc, const char **argv, const 
char *prefix)
if (!file_exists(sm_gitdir)) {
if (safe_create_leading_directories_const(sm_gitdir) < 0)
die(_("could not create directory '%s'"), sm_gitdir);
-   if (clone_submodule(path, sm_gitdir, url, depth, , 
quiet))
+   if (clone_submodule(path, sm_gitdir, url, depth, ,
+   _if_able, quiet))
die(_("clone of '%s' into submodule path '%s' failed"),
url, path);
} else {
-- 
2.9.2.583.gd6329be.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

[PATCHv3 5/9] clone: clarify option_reference as required

[Stefan Beller]

In the next patch we introduce optional references; To better distinguish
between optional and required references we rename the variable.

Signed-off-by: Stefan Beller 
---
 builtin/clone.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index f044a8c..052a769 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -50,7 +50,7 @@ static int option_verbosity;
 static int option_progress = -1;
 static enum transport_family family;
 static struct string_list option_config = STRING_LIST_INIT_NODUP;
-static struct string_list option_reference = STRING_LIST_INIT_NODUP;
+static struct string_list option_required_reference = STRING_LIST_INIT_NODUP;
 static int option_dissociate;
 static int max_jobs = -1;
 
@@ -79,7 +79,7 @@ static struct option builtin_clone_options[] = {
N_("number of submodules cloned in parallel")),
OPT_STRING(0, "template", _template, N_("template-directory"),
   N_("directory from which templates will be used")),
-   OPT_STRING_LIST(0, "reference", _reference, N_("repo"),
+   OPT_STRING_LIST(0, "reference", _required_reference, N_("repo"),
N_("reference repository")),
OPT_BOOL(0, "dissociate", _dissociate,
 N_("use --reference only while cloning")),
@@ -325,7 +325,7 @@ static int add_one_reference(struct string_list_item *item, 
void *cb_data)
 
 static void setup_reference(void)
 {
-   for_each_string_list(_reference, add_one_reference, NULL);
+   for_each_string_list(_required_reference, add_one_reference, 
NULL);
 }
 
 static void copy_alternates(struct strbuf *src, struct strbuf *dst,
@@ -977,7 +977,7 @@ int cmd_clone(int argc, const char **argv, const char 
*prefix)
git_config_set(key.buf, repo);
strbuf_reset();
 
-   if (option_reference.nr)
+   if (option_required_reference.nr)
setup_reference();
 
fetch_pattern = value.buf;
-- 
2.9.2.583.gd6329be.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

[PATCHv2 5/6] submodule update: add super-reference flag

[Stefan Beller]

When we have a another clone of a superproject, we may want to
mirror the submodules using alternates. Introduce an option
`--super-reference` that let's a user point to another superproject,
which is assumed to have the same structure as the one they are
running the "submodule update" command from and has all submodules
checked out to borrow the submodule objects from within the other
superprojects git directory.

Signed-off-by: Stefan Beller 
---
 Documentation/git-submodule.txt |  8 +++-
 builtin/submodule--helper.c | 14 +-
 git-submodule.sh| 10 ++
 3 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index bf3bb37..6f2f873 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -16,7 +16,7 @@ SYNOPSIS
 'git submodule' [--quiet] deinit [-f|--force] (--all|[--] ...)
 'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch]
  [--[no-]recommend-shallow] [-f|--force] [--rebase|--merge]
- [--reference ] [--depth ] [--recursive]
+ [--[super-]reference ] [--depth ] [--recursive]
  [--jobs ] [--] [...]
 'git submodule' [--quiet] summary [--cached|--files] [(-n|--summary-limit) ]
  [commit] [--] [...]
@@ -370,6 +370,12 @@ the submodule itself.
This option is only valid for add and update commands.  These
commands sometimes need to clone a remote repository. In this case,
this option will be passed to the linkgit:git-clone[1] command.
+
+--super-reference ::
+   This option is only valid for the update command. When update needs
+   to clone a repository, a reference will be passed to the clone command
+   that points at the submodule path inside the reference superproject.
+
 +
 *NOTE*: Do *not* use this option unless you have read the note
 for linkgit:git-clone[1]'s `--reference` and `--shared` options carefully.
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index b7710a7..ea6b27c 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -584,6 +584,7 @@ struct submodule_update_clone {
int quiet;
int recommend_shallow;
struct string_list references;
+   struct string_list super_references;
const char *depth;
const char *recursive_prefix;
const char *prefix;
@@ -600,7 +601,7 @@ struct submodule_update_clone {
 };
 #define SUBMODULE_UPDATE_CLONE_INIT {0, MODULE_LIST_INIT, 0, \
SUBMODULE_UPDATE_STRATEGY_INIT, 0, -1, STRING_LIST_INIT_DUP, \
-   NULL, NULL, NULL, \
+   STRING_LIST_INIT_DUP, NULL, NULL, NULL, \
STRING_LIST_INIT_DUP, 0, NULL, 0, 0}
 
 
@@ -715,6 +716,15 @@ static int prepare_to_clone_next_submodule(const struct 
cache_entry *ce,
for_each_string_list_item(item, >references)
argv_array_pushl(>args, "--reference", 
item->string, NULL);
}
+   if (suc->super_references.nr) {
+   struct string_list_item *item;
+   for_each_string_list_item(item, >super_references) {
+   strbuf_reset();
+   argv_array_pushf(>args, "--reference=%s/%s",
+relative_path(item->string, 
suc->prefix, ),
+sub->path);
+   }
+   }
if (suc->depth)
argv_array_push(>args, suc->depth);
 
@@ -835,6 +845,8 @@ static int update_clone(int argc, const char **argv, const 
char *prefix)
   N_("rebase, merge, checkout or none")),
OPT_STRING_LIST(0, "reference", , N_("repo"),
   N_("reference repository")),
+   OPT_STRING_LIST(0, "super-reference", _references, 
N_("repo"),
+  N_("superproject of a reference repository")),
OPT_STRING(0, "depth", , "",
   N_("Create a shallow clone truncated to the "
  "specified number of revisions")),
diff --git a/git-submodule.sh b/git-submodule.sh
index 3b412f5..17f4ace 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -34,6 +34,7 @@ command=
 branch=
 force=
 reference=
+superreference=
 cached=
 recursive=
 init=
@@ -520,6 +521,14 @@ cmd_update()
--reference=*)
reference="$1"
;;
+   --super-reference)
+   case "$2" in '') usage ;; esac
+   superreference="--super-reference=$2"
+   shift
+   ;;
+   --super-reference=*)
+   superreference="$1"
+   ;;
-m|--merge)
update="merge"
;;
@@ -576,6 +585,7 @@ cmd_update()

[PATCHv2 6/6] clone: reference flag is used for submodules as well

[Stefan Beller]

When giving a --reference while also giving --recurse, the alternates
for the submodules are assumed to be in the superproject as well.

In case they are not, we error out when cloning the submodule.
However we error out completely, we did not record the alternates yet,
so a following update command succeeds as usual (with no alternates).

Signed-off-by: Stefan Beller 
---
 Documentation/git-clone.txt|  5 +
 builtin/clone.c| 22 ++
 t/t7408-submodule-reference.sh | 31 ++-
 3 files changed, 53 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index ec41d3d..2c6dea6 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -100,6 +100,11 @@ objects from the source repository into a pack in the 
cloned repository.
 +
 *NOTE*: see the NOTE for the `--shared` option, and also the
 `--dissociate` option.
++
+When `--reference` is given together with `--recursive`,
+the reference repository is assumed to contain the submodules
+as well and the submodules are setup as alternates of the
+submodules in the given reference project.
 
 --dissociate::
Borrow the objects from reference repositories specified
diff --git a/builtin/clone.c b/builtin/clone.c
index f044a8c..e8272b5 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -51,6 +51,7 @@ static int option_progress = -1;
 static enum transport_family family;
 static struct string_list option_config = STRING_LIST_INIT_NODUP;
 static struct string_list option_reference = STRING_LIST_INIT_NODUP;
+static struct string_list super_references = STRING_LIST_INIT_DUP;
 static int option_dissociate;
 static int max_jobs = -1;
 
@@ -280,10 +281,11 @@ static void strip_trailing_slashes(char *dir)
*end = '\0';
 }
 
-static int add_one_reference(struct string_list_item *item, void *cb_data)
+static int add_one_reference(struct string_list_item *item, void *cb_dir)
 {
char *ref_git;
const char *repo;
+   const char *dir = cb_dir;
struct strbuf alternate = STRBUF_INIT;
 
/* Beware: read_gitfile(), real_path() and mkpath() return static 
buffer */
@@ -316,6 +318,13 @@ static int add_one_reference(struct string_list_item 
*item, void *cb_data)
if (!access(mkpath("%s/info/grafts", ref_git), F_OK))
die(_("reference repository '%s' is grafted"), item->string);
 
+   if (option_recursive) {
+   struct strbuf sb = STRBUF_INIT;
+   char *rel = xstrdup(relative_path(item->string, dir, ));
+   string_list_append(_references, rel);
+   strbuf_reset();
+   }
+
strbuf_addf(, "%s/objects", ref_git);
add_to_alternates_file(alternate.buf);
strbuf_release();
@@ -323,9 +332,9 @@ static int add_one_reference(struct string_list_item *item, 
void *cb_data)
return 0;
 }
 
-static void setup_reference(void)
+static void setup_reference(char *dir)
 {
-   for_each_string_list(_reference, add_one_reference, NULL);
+   for_each_string_list(_reference, add_one_reference, dir);
 }
 
 static void copy_alternates(struct strbuf *src, struct strbuf *dst,
@@ -736,6 +745,7 @@ static int checkout(void)
 
if (!err && option_recursive) {
struct argv_array args = ARGV_ARRAY_INIT;
+   static struct string_list_item *item;
argv_array_pushl(, "submodule", "update", "--init", 
"--recursive", NULL);
 
if (option_shallow_submodules == 1)
@@ -744,6 +754,10 @@ static int checkout(void)
if (max_jobs != -1)
argv_array_pushf(, "--jobs=%d", max_jobs);
 
+   if (super_references.nr)
+   for_each_string_list_item(item, _references)
+   argv_array_pushf(, "--super-reference=%s", 
item->string);
+
err = run_command_v_opt(args.argv, RUN_GIT_CMD);
argv_array_clear();
}
@@ -978,7 +992,7 @@ int cmd_clone(int argc, const char **argv, const char 
*prefix)
strbuf_reset();
 
if (option_reference.nr)
-   setup_reference();
+   setup_reference(dir);
 
fetch_pattern = value.buf;
refspec = parse_fetch_refspec(1, _pattern);
diff --git a/t/t7408-submodule-reference.sh b/t/t7408-submodule-reference.sh
index 1d9326e..588d53e 100755
--- a/t/t7408-submodule-reference.sh
+++ b/t/t7408-submodule-reference.sh
@@ -53,7 +53,8 @@ test_expect_success 'submodule add --reference uses 
alternates' '
(
cd super &&
git submodule add --reference ../B "file://$base_dir/A" sub &&
-   git commit -m B-super-added
+   git commit -m B-super-added &&
+   git repack -ad
) &&
test_alternate_usage super/.git/modules/sub/objects/info/alternates 
super/sub
 '
@@ -65,4 +66,32 @@ test_expect_success 

[PATCHv3 8/9] submodule: try alternates when superproject has an alternate

[Stefan Beller]

Signed-off-by: Stefan Beller 
---
 Documentation/git-clone.txt|  4 
 builtin/submodule--helper.c| 47 ++
 t/t7408-submodule-reference.sh | 29 +-
 3 files changed, 79 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index e316c4b..cadf138 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -103,6 +103,10 @@ objects from the source repository into a pack in the 
cloned repository.
 +
 *NOTE*: see the NOTE for the `--shared` option, and also the
 `--dissociate` option.
++
+When using --reference any submodule that is cloned
+sets up a corresponding alternate at $GIT_DIR/modules if such a
+an alternate exists.
 
 --dissociate::
Borrow the objects from reference repositories specified
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index f360473..fc14843 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -635,6 +635,45 @@ static void next_submodule_warn_missing(struct 
submodule_update_clone *suc,
}
 }
 
+struct submodule_alternate_setup {
+   struct submodule_update_clone *suc;
+   const char *submodule_name;
+   struct child_process *child;
+   struct strbuf *out;
+};
+
+int add_possible_reference(struct alternate_object_database *alt, void *sas_cb)
+{
+   struct submodule_alternate_setup *sas = sas_cb;
+
+   /* directory name, minus trailing slash */
+   size_t namelen = alt->name - alt->base - 1;
+   struct strbuf name = STRBUF_INIT;
+   strbuf_add(, alt->base, namelen);
+
+   /*
+* If the alternate object store is another repository, try the
+* standard layout with .git/modules//objects
+*/
+   if (ends_with(name.buf, ".git/objects")) {
+   struct strbuf sb = STRBUF_INIT;
+   strbuf_add(, name.buf, name.len - strlen("objects"));
+   /*
+* We need to end the new path with '/' to mark it as a dir,
+* otherwise a submodule name containing '/' will be broken
+* as the last part of a missing submodule reference would
+* be taken as a file name.
+*/
+   strbuf_addf(, "modules/%s/", sas->submodule_name);
+   argv_array_pushf(>child->args,
+"--reference-if-able=%s", sb.buf);
+   strbuf_release();
+   }
+
+   strbuf_release();
+   return 0;
+}
+
 /**
  * Determine whether 'ce' needs to be cloned. If so, prepare the 'child' to
  * run the clone. Returns 1 if 'ce' needs to be cloned, 0 otherwise.
@@ -650,6 +689,7 @@ static int prepare_to_clone_next_submodule(const struct 
cache_entry *ce,
const char *displaypath = NULL;
char *url = NULL;
int needs_cloning = 0;
+   struct submodule_alternate_setup sas;
 
if (ce_stage(ce)) {
if (suc->recursive_prefix)
@@ -728,6 +768,13 @@ static int prepare_to_clone_next_submodule(const struct 
cache_entry *ce,
for_each_string_list_item(item, >references)
argv_array_pushl(>args, "--reference", 
item->string, NULL);
}
+
+   sas.submodule_name = sub->name;
+   sas.suc = suc;
+   sas.child = child;
+   sas.out = out;
+   foreach_alt_odb(add_possible_reference, );
+
if (suc->depth)
argv_array_push(>args, suc->depth);
 
diff --git a/t/t7408-submodule-reference.sh b/t/t7408-submodule-reference.sh
index 4a1b8f0..a9b89a3 100755
--- a/t/t7408-submodule-reference.sh
+++ b/t/t7408-submodule-reference.sh
@@ -7,7 +7,6 @@ test_description='test clone --reference'
 . ./test-lib.sh
 
 base_dir=$(pwd)
-
 test_alternate_is_used () {
alternates_file="$1" &&
working_dir="$2" &&
@@ -73,4 +72,32 @@ test_expect_success 'updating superproject keeps alternates' 
'
test_alternate_is_used 
super-clone/.git/modules/sub/objects/info/alternates super-clone/sub
 '
 
+test_expect_success 'submodules use alternates when cloning a superproject' '
+   test_when_finished "rm -rf super-clone" &&
+   git clone --reference super --recursive super super-clone &&
+   (
+   cd super-clone &&
+   # test superproject has alternates setup correctly
+   test_alternate_is_used .git/objects/info/alternates . &&
+   # test submodule has correct setup
+   test_alternate_is_used .git/modules/sub/objects/info/alternates 
sub
+   )
+'
+
+test_expect_success 'cloning superproject, missing submodule alternates' '
+   test_when_finished "rm -rf super-clone" &&
+   git clone super super2 &&
+   git clone --recursive --reference super2 super2 super-clone &&
+   (
+   cd super-clone &&
+   # test superproject has alternates setup correctly
+   

[PATCHv3 4/9] submodule--helper update-clone: allow multiple references

[Stefan Beller]

Allow the user to pass in multiple references to update_clone.
Currently this is only internal API, but once the shell script is
replaced by a C version, this is needed.

This fixes an API bug between the shell script and the helper.
Currently the helper accepts "--reference" "--reference=foo"
as a OPT_STRING whose value happens to be "--reference=foo", and
then uses

if (suc->reference)
argv_array_push(>args, suc->reference)

where suc->reference _is_ "--reference=foo" when invoking the
underlying "git clone", it cancels out.

With this change we omit one of the "--reference" arguments when
passing references from the shell script to the helper.

Signed-off-by: Stefan Beller 
---
 builtin/submodule--helper.c | 14 +-
 git-submodule.sh|  2 +-
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index ce9b3e9..7096848 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -584,7 +584,7 @@ struct submodule_update_clone {
/* configuration parameters which are passed on to the children */
int quiet;
int recommend_shallow;
-   const char *reference;
+   struct string_list references;
const char *depth;
const char *recursive_prefix;
const char *prefix;
@@ -600,7 +600,8 @@ struct submodule_update_clone {
int failed_clones_nr, failed_clones_alloc;
 };
 #define SUBMODULE_UPDATE_CLONE_INIT {0, MODULE_LIST_INIT, 0, \
-   SUBMODULE_UPDATE_STRATEGY_INIT, 0, -1, NULL, NULL, NULL, NULL, \
+   SUBMODULE_UPDATE_STRATEGY_INIT, 0, -1, STRING_LIST_INIT_DUP, \
+   NULL, NULL, NULL, \
STRING_LIST_INIT_DUP, 0, NULL, 0, 0}
 
 
@@ -710,8 +711,11 @@ static int prepare_to_clone_next_submodule(const struct 
cache_entry *ce,
argv_array_pushl(>args, "--path", sub->path, NULL);
argv_array_pushl(>args, "--name", sub->name, NULL);
argv_array_pushl(>args, "--url", url, NULL);
-   if (suc->reference)
-   argv_array_push(>args, suc->reference);
+   if (suc->references.nr) {
+   struct string_list_item *item;
+   for_each_string_list_item(item, >references)
+   argv_array_pushl(>args, "--reference", 
item->string, NULL);
+   }
if (suc->depth)
argv_array_push(>args, suc->depth);
 
@@ -830,7 +834,7 @@ static int update_clone(int argc, const char **argv, const 
char *prefix)
OPT_STRING(0, "update", ,
   N_("string"),
   N_("rebase, merge, checkout or none")),
-   OPT_STRING(0, "reference", , N_("repo"),
+   OPT_STRING_LIST(0, "reference", , N_("repo"),
   N_("reference repository")),
OPT_STRING(0, "depth", , "",
   N_("Create a shallow clone truncated to the "
diff --git a/git-submodule.sh b/git-submodule.sh
index c90dc33..3b412f5 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -575,7 +575,7 @@ cmd_update()
${wt_prefix:+--prefix "$wt_prefix"} \
${prefix:+--recursive-prefix "$prefix"} \
${update:+--update "$update"} \
-   ${reference:+--reference "$reference"} \
+   ${reference:+"$reference"} \
${depth:+--depth "$depth"} \
${recommend_shallow:+"$recommend_shallow"} \
${jobs:+$jobs} \
-- 
2.9.2.583.gd6329be.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

[PATCHv3 9/9] submodule--helper: use parallel processor correctly.

[Stefan Beller]

When developing the prior patches I had a temporary state in which
git-clone would segfault, when prepared the call in
prepare_to_clone_next_submodule. This lead to the call failing, i.e. in
`update_clone_task_finished` the task was scheduled to be tried again.
The second call to prepare_to_clone_next_submodule would return 0, as the
segfaulted clone did create the .git file already, such that was not
considered to need to be cloned again. I was seeing the "BUG: ce was
a submodule before?\n" message, which was the correct behavior at the
time as my local code was buggy. When trying to debug this failure, I
tried to use printing messages into the strbuf that is passed around,
but these messages were never printed as the die(..) doesn't
flush the `err` strbuf.

When implementing the die() in 665b35ecc (2016-06-09, "submodule--helper:
initial clone learns retry logic"), I considered this condition to be
a severe condition, which should lead to an immediate abort as we do not
trust ourselves any more. However the queued messages in `err` are valuable
so let's not toss them out by immediately dieing, but a graceful return.

Another thing to note: The error message itself was missleading. A return
value of 0 doesn't indicate the passed in `ce` is not a submodule any more,
but just that we do not consider cloning it any more.

Signed-off-by: Stefan Beller 
---
 builtin/submodule--helper.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index fc14843..3e40f99 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -815,8 +815,12 @@ static int update_clone_get_next_task(struct child_process 
*child,
if (index < suc->failed_clones_nr) {
int *p;
ce = suc->failed_clones[index];
-   if (!prepare_to_clone_next_submodule(ce, child, suc, err))
-   die("BUG: ce was a submodule before?");
+   if (!prepare_to_clone_next_submodule(ce, child, suc, err)) {
+   suc->current ++;
+   strbuf_addf(err, "BUG: submodule considered for 
cloning,"
+   "doesn't need cloning any more?\n");
+   return 0;
+   }
p = xmalloc(sizeof(*p));
*p = suc->current;
*idx_task_cb = p;
-- 
2.9.2.583.gd6329be.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

[PATCHv3 1/9] t7408: modernize style

[Stefan Beller]

No functional change intended. This commit only changes formatting
to the style we recently use, e.g. starting the body of a test with a
single quote on the same line as the header, and then having the test
indented in the following lines.

Whenever we change directories, we do that in subshells.

Signed-off-by: Stefan Beller 
---
 t/t7408-submodule-reference.sh | 140 +
 1 file changed, 71 insertions(+), 69 deletions(-)

diff --git a/t/t7408-submodule-reference.sh b/t/t7408-submodule-reference.sh
index eaea19b..beee0bb 100755
--- a/t/t7408-submodule-reference.sh
+++ b/t/t7408-submodule-reference.sh
@@ -8,74 +8,76 @@ test_description='test clone --reference'
 
 base_dir=$(pwd)
 
-U=$base_dir/UPLOAD_LOG
-
-test_expect_success 'preparing first repository' \
-'test_create_repo A && cd A &&
-echo first > file1 &&
-git add file1 &&
-git commit -m A-initial'
-
-cd "$base_dir"
-
-test_expect_success 'preparing second repository' \
-'git clone A B && cd B &&
-echo second > file2 &&
-git add file2 &&
-git commit -m B-addition &&
-git repack -a -d &&
-git prune'
-
-cd "$base_dir"
-
-test_expect_success 'preparing superproject' \
-'test_create_repo super && cd super &&
-echo file > file &&
-git add file &&
-git commit -m B-super-initial'
-
-cd "$base_dir"
-
-test_expect_success 'submodule add --reference' \
-'cd super && git submodule add --reference ../B "file://$base_dir/A" sub &&
-git commit -m B-super-added'
-
-cd "$base_dir"
-
-test_expect_success 'after add: existence of info/alternates' \
-'test_line_count = 1 super/.git/modules/sub/objects/info/alternates'
-
-cd "$base_dir"
-
-test_expect_success 'that reference gets used with add' \
-'cd super/sub &&
-echo "0 objects, 0 kilobytes" > expected &&
-git count-objects > current &&
-diff expected current'
-
-cd "$base_dir"
-
-test_expect_success 'cloning superproject' \
-'git clone super super-clone'
-
-cd "$base_dir"
-
-test_expect_success 'update with reference' \
-'cd super-clone && git submodule update --init --reference ../B'
-
-cd "$base_dir"
-
-test_expect_success 'after update: existence of info/alternates' \
-'test_line_count = 1 super-clone/.git/modules/sub/objects/info/alternates'
-
-cd "$base_dir"
-
-test_expect_success 'that reference gets used with update' \
-'cd super-clone/sub &&
-echo "0 objects, 0 kilobytes" > expected &&
-git count-objects > current &&
-diff expected current'
-
-cd "$base_dir"
+test_expect_success 'preparing first repository' '
+   test_create_repo A &&
+   (
+   cd A &&
+   echo first >file1 &&
+   git add file1 &&
+   git commit -m A-initial
+   )
+'
+
+test_expect_success 'preparing second repository' '
+   git clone A B &&
+   (
+   cd B &&
+   echo second >file2 &&
+   git add file2 &&
+   git commit -m B-addition &&
+   git repack -a -d &&
+   git prune
+   )
+'
+
+test_expect_success 'preparing superproject' '
+   test_create_repo super &&
+   (
+   cd super &&
+   echo file >file &&
+   git add file &&
+   git commit -m B-super-initial
+   )
+'
+
+test_expect_success 'submodule add --reference' '
+   (
+   cd super &&
+   git submodule add --reference ../B "file://$base_dir/A" sub &&
+   git commit -m B-super-added
+   )
+'
+
+test_expect_success 'after add: existence of info/alternates' '
+   test_line_count = 1 super/.git/modules/sub/objects/info/alternates
+'
+
+test_expect_success 'that reference gets used with add' '
+   (
+   cd super/sub &&
+   echo "0 objects, 0 kilobytes" > expected &&
+   git count-objects > current &&
+   diff expected current
+   )
+'
+
+test_expect_success 'cloning superproject' '
+   git clone super super-clone
+'
+
+test_expect_success 'update with reference' '
+   cd super-clone && git submodule update --init --reference ../B
+'
+
+test_expect_success 'after update: existence of info/alternates' '
+   test_line_count = 1 super-clone/.git/modules/sub/objects/info/alternates
+'
+
+test_expect_success 'that reference gets used with update' '
+   cd super-clone/sub &&
+   echo "0 objects, 0 kilobytes" > expected &&
+   git count-objects > current &&
+   diff expected current
+'
 
 test_done
-- 
2.9.2.583.gd6329be.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

[PATCHv3 2/9] t7408: merge short tests, factor out testing method

[Stefan Beller]

Tests consisting of one line each can be consolidated to have fewer tests
to run as well as fewer lines of code.

When having just a few git commands, do not create a new shell but
use the -C flag in Git to execute in the correct directory.

Signed-off-by: Stefan Beller 
---
 t/t7408-submodule-reference.sh | 57 ++
 1 file changed, 25 insertions(+), 32 deletions(-)

diff --git a/t/t7408-submodule-reference.sh b/t/t7408-submodule-reference.sh
index beee0bb..4a1b8f0 100755
--- a/t/t7408-submodule-reference.sh
+++ b/t/t7408-submodule-reference.sh
@@ -8,6 +8,15 @@ test_description='test clone --reference'
 
 base_dir=$(pwd)
 
+test_alternate_is_used () {
+   alternates_file="$1" &&
+   working_dir="$2" &&
+   test_line_count = 1 "$alternates_file" &&
+   echo "0 objects, 0 kilobytes" >expect &&
+   git -C "$working_dir" count-objects >actual &&
+   test_cmp expect actual
+}
+
 test_expect_success 'preparing first repository' '
test_create_repo A &&
(
@@ -40,44 +49,28 @@ test_expect_success 'preparing superproject' '
)
 '
 
-test_expect_success 'submodule add --reference' '
+test_expect_success 'submodule add --reference uses alternates' '
(
cd super &&
git submodule add --reference ../B "file://$base_dir/A" sub &&
-   git commit -m B-super-added
-   )
-'
-
-test_expect_success 'after add: existence of info/alternates' '
-   test_line_count = 1 super/.git/modules/sub/objects/info/alternates
+   git commit -m B-super-added &&
+   git repack -ad
+   ) &&
+   test_alternate_is_used super/.git/modules/sub/objects/info/alternates 
super/sub
 '
 
-test_expect_success 'that reference gets used with add' '
-   (
-   cd super/sub &&
-   echo "0 objects, 0 kilobytes" > expected &&
-   git count-objects > current &&
-   diff expected current
-   )
-'
-
-test_expect_success 'cloning superproject' '
-   git clone super super-clone
-'
-
-test_expect_success 'update with reference' '
-   cd super-clone && git submodule update --init --reference ../B
-'
-
-test_expect_success 'after update: existence of info/alternates' '
-   test_line_count = 1 super-clone/.git/modules/sub/objects/info/alternates
-'
+# The tests up to this point, and repositories created by them
+# (A, B, super and super/sub), are about setting up the stage
+# forsubsequent tests and meant to be kept throughout the
+# remainder of the test.
+# Tests from here on, if they create their own test repository,
+# are expected to clean after themselves.
 
-test_expect_success 'that reference gets used with update' '
-   cd super-clone/sub &&
-   echo "0 objects, 0 kilobytes" > expected &&
-   git count-objects > current &&
-   diff expected current
+test_expect_success 'updating superproject keeps alternates' '
+   test_when_finished "rm -rf super-clone" &&
+   git clone super super-clone &&
+   git -C super-clone submodule update --init --reference ../B &&
+   test_alternate_is_used 
super-clone/.git/modules/sub/objects/info/alternates super-clone/sub
 '
 
 test_done
-- 
2.9.2.583.gd6329be.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

[PATCHv3 3/9] submodule--helper module-clone: allow multiple references

[Stefan Beller]

Allow users to pass in multiple references, just as clone accepts multiple
references as well.

Signed-off-by: Stefan Beller 
---
 builtin/submodule--helper.c | 20 
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 6f6d67a..ce9b3e9 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -442,7 +442,7 @@ static int module_name(int argc, const char **argv, const 
char *prefix)
 }
 
 static int clone_submodule(const char *path, const char *gitdir, const char 
*url,
-  const char *depth, const char *reference, int quiet)
+  const char *depth, struct string_list *reference, 
int quiet)
 {
struct child_process cp;
child_process_init();
@@ -453,8 +453,12 @@ static int clone_submodule(const char *path, const char 
*gitdir, const char *url
argv_array_push(, "--quiet");
if (depth && *depth)
argv_array_pushl(, "--depth", depth, NULL);
-   if (reference && *reference)
-   argv_array_pushl(, "--reference", reference, NULL);
+   if (reference->nr) {
+   struct string_list_item *item;
+   for_each_string_list_item(item, reference)
+   argv_array_pushl(, "--reference",
+item->string, NULL);
+   }
if (gitdir && *gitdir)
argv_array_pushl(, "--separate-git-dir", gitdir, NULL);
 
@@ -470,13 +474,13 @@ static int clone_submodule(const char *path, const char 
*gitdir, const char *url
 
 static int module_clone(int argc, const char **argv, const char *prefix)
 {
-   const char *name = NULL, *url = NULL;
-   const char *reference = NULL, *depth = NULL;
+   const char *name = NULL, *url = NULL, *depth = NULL;
int quiet = 0;
FILE *submodule_dot_git;
char *p, *path = NULL, *sm_gitdir;
struct strbuf rel_path = STRBUF_INIT;
struct strbuf sb = STRBUF_INIT;
+   struct string_list reference = STRING_LIST_INIT_NODUP;
 
struct option module_clone_options[] = {
OPT_STRING(0, "prefix", ,
@@ -491,8 +495,8 @@ static int module_clone(int argc, const char **argv, const 
char *prefix)
OPT_STRING(0, "url", ,
   N_("string"),
   N_("url where to clone the submodule from")),
-   OPT_STRING(0, "reference", ,
-  N_("string"),
+   OPT_STRING_LIST(0, "reference", ,
+  N_("repo"),
   N_("reference repository")),
OPT_STRING(0, "depth", ,
   N_("string"),
@@ -528,7 +532,7 @@ static int module_clone(int argc, const char **argv, const 
char *prefix)
if (!file_exists(sm_gitdir)) {
if (safe_create_leading_directories_const(sm_gitdir) < 0)
die(_("could not create directory '%s'"), sm_gitdir);
-   if (clone_submodule(path, sm_gitdir, url, depth, reference, 
quiet))
+   if (clone_submodule(path, sm_gitdir, url, depth, , 
quiet))
die(_("clone of '%s' into submodule path '%s' failed"),
url, path);
} else {
-- 
2.9.2.583.gd6329be.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 submission process, was Re: [PATCH v6 06/16] merge_recursive: abort properly upon errors

[Michael Haggerty]

On 08/09/2016 12:36 AM, Junio C Hamano wrote:
> Michael Haggerty  writes:
> 
>> Is it *really* so insane to consider moving collaboration on the Git
>> project to GitHub or some other similar platform?
> 
> I only know how "pull requests" and "issues" discussion in GitHub
> Web interface _currently_ looks like, so if you have even more
> wonderful thing in the works, I _might_ be swayed otherwise,

If I did I couldn't say anyway, so let's assume that current GitHub is
what's on the table [1].

There are a couple of recent code-review improvements that you might
have missed:

* You can now get email updates about your own activity [2]. (Previously
you would only get emails about the activity of other people, which
would leave holes in the email record of the conversation.)

* There is also now better visibility of code review comments regarding
lines that are no longer part of a PR [3].

> but I
> do not think it is sane to expect that the same quality and quantity
> of reviews as we do here can be maintained with that thing.

Could you elaborate why you would expect quality and/or quantity of
reviews to suffer? I'm really curious, and I'd be happy to pass your
feedback along to my colleagues.

Here are some factors that I think will *improve* reviews:

* While you are reviewing patches, you can "zoom out" to see code beyond
the usual diff context. Currently a reviewer who wants more context has
to transition from reading the diff in email to applying the patch and
viewing it in another tool. Then the reviewer has to go back to email to
leave the comment.

* If you want to compile/run/edit/profile the code, you just need to
"git fetch" rather than messing around with "git am". For more involved
suggestions, it is possible to propose a PR against the original PR.

* It is easy to summon somebody else into the review conversation by
@-mentioning them. That person immediately can see the whole history of
the PR. (This is an improvement on the status quo, where a new entrant
to a conversation might have to dig through the email trash or an email
archive to see emails that they overlooked before they were added to the
CC list.)

* It is easy to subscribe/unsubscribe from particular discussions [4].
This makes it easier to follow the discussions you are interested in
without getting swamped with emails about other discussions. You can
unsubscribe from a discussion permanently, or in such a way that a new
@-mention brings you back in.

* It is easy to mention other PRs/commits/issues in a discussion, and
those mentions become clickable links (no jumping back and forth between
email client and web browser). Of course you can also link to arbitrary
URLs (e.g., mailing list archives).

* It is possible to search old issues and PRs for additional context.
(Granted, the history of the project from its ML era would have to be
searched separately.)

Given that I work for GitHub, I'm uncomfortable doing any more advocacy
here. If people have concrete questions, I'd be happy to answer them, on
the list or in private.

Michael

[1] In general, GitHub *does* get better over time, and we would benefit
from any future improvements.
[2] https://github.com/blog/2203-email-updates-about-your-own-activity
[3] https://github.com/blog/2123-more-code-review-tools
[4] https://github.com/blog/2183-improvements-to-notification-emails

--
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: What's cooking in git.git (Aug 2016, #03; Mon, 8)

[Stefan Beller]

On Mon, Aug 8, 2016 at 3:32 PM, Junio C Hamano  wrote:
>
> * sb/submodule-clone-rr (2016-08-06) 6 commits
>  - clone: reference flag is used for submodules as well
>  - submodule update: add super-reference flag
>  - submodule--helper update-clone: allow multiple references
>  - submodule--helper module-clone: allow multiple references
>  - t7408: merge short tests, factor out testing method
>  - t7408: modernize style
>
>  Waiting for review discussion to settle.
>  cf. <20160806012318.17968-1-sbel...@google.com>

I will reroll today with a totally different approach.
No need to further discuss the last 2 patches.

Thanks,
Stefan
--
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 submission process, was Re: [PATCH v6 06/16] merge_recursive: abort properly upon errors

[Junio C Hamano]

Michael Haggerty  writes:

> Is it *really* so insane to consider moving collaboration on the Git
> project to GitHub or some other similar platform?

I only know how "pull requests" and "issues" discussion in GitHub
Web interface _currently_ looks like, so if you have even more
wonderful thing in the works, I _might_ be swayed otherwise, but I
do not think it is sane to expect that the same quality and quantity
of reviews as we do here can be maintained with that thing.
--
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

What's cooking in git.git (Aug 2016, #03; Mon, 8)

[Junio C Hamano]

Here are the topics that have been cooking.  Commits prefixed with
'-' are only in 'pu' (proposed updates) while commits prefixed with
'+' are in 'next'.  The ones marked with '.' do not appear in any of
the integration branches, but I am still holding onto them.

Many topics in "Cooking" section without seeing activity have been
moved to "Stalled" status and marked as "Will discard".  This is
unfortunate but with way many people wanting to throw random new
topics while too few people able/willing to review them, it is
inevitable.

On the 'master' front, the individual commit count now exceeds 500
since the last major release, and the early preview -rc0 is expected
to happen at the end of the week.

The 'maint' branch has been accumulating enough material to make it
the next maintenance release v2.9.3, which hopefully happen late
this week.

You can find the changes described here in the integration branches
of the repositories listed at

http://git-blame.blogspot.com/p/git-public-repositories.html

--
[Graduated to "master"]

* ab/gitweb-link-html-escape (2016-08-01) 1 commit
  (merged to 'next' on 2016-08-03 at 44b6088)
 + gitweb: escape link body in format_ref_marker

 The characters in the label shown for tags/refs for commits in
 "gitweb" output are now properly escaped for proper HTML output.


* cp/completion-clone-recurse-submodules (2016-07-27) 1 commit
  (merged to 'next' on 2016-08-03 at cbf0d94)
 + completion: add option '--recurse-submodules' to 'git clone'



* da/subtree-modernize (2016-07-27) 2 commits
  (merged to 'next' on 2016-08-03 at 06ad015)
 + subtree: adjust function definitions to match CodingGuidelines
 + subtree: adjust style to match CodingGuidelines

 Style fixes for "git subtree" (in contrib/).


* ew/build-time-pager-tweaks (2016-08-04) 1 commit
  (merged to 'next' on 2016-08-05 at 4f0b11b)
 + pager: move pager-specific setup into the build

 The build procedure learned PAGER_ENV knob that lists what default
 environment variable settings to export for popular pagers.  This
 mechanism is used to tweak the default settings to MORE on FreeBSD.


* ew/git-svn-http-tests (2016-07-25) 2 commits
  (merged to 'next' on 2016-08-03 at 2b23920)
 + git svn: migrate tests to use lib-httpd
 + t/t91*: do not say how to avoid the tests

 Tests for "git svn" have been taught to reuse the lib-httpd test
 infrastructure when testing the subversion integration that
 interacts with subversion repositories served over the http://
 protocol.


* ib/t3700-add-chmod-x-updates (2016-08-01) 3 commits
  (merged to 'next' on 2016-08-03 at 1753346)
 + t3700: add a test_mode_in_index helper function
 + t3700: merge two tests into one
 + t3700: remove unwanted leftover files before running new tests

 The t3700 test about "add --chmod=-x" have been made a bit more
 robust and generally cleaned up.


* jc/hashmap-doc-init (2016-08-02) 1 commit
  (merged to 'next' on 2016-08-05 at 2eb946a)
 + hashmap: clarify that hashmap_entry can safely be discarded

 The API documentation for hashmap was unclear if hashmap_entry
 can be safely discarded without any other consideration.  State
 that it is safe to do so.


* jh/clean-smudge-f-doc (2016-08-03) 1 commit
  (merged to 'next' on 2016-08-04 at c2fc8e6)
 + clarify %f documentation

 A minor documentation update.

 This was split out from a stalled jh/clean-smudge-annex topic
 before discarding it.


* jk/difftool-in-subdir (2016-07-28) 3 commits
  (merged to 'next' on 2016-08-03 at 90f195a)
 + difftool: use Git::* functions instead of passing around state
 + difftool: avoid $GIT_DIR and $GIT_WORK_TREE
 + difftool: fix argument handling in subdirs

 "git difftool ..." started in a subdirectory failed to
 interpret the paths relative to that directory, which has been
 fixed.


* jk/pack-objects-optim (2016-07-29) 6 commits
  (merged to 'next' on 2016-08-03 at ad8caca)
 + pack-objects: compute local/ignore_pack_keep early
 + pack-objects: break out of want_object loop early
 + find_pack_entry: replace last_found_pack with MRU cache
 + add generic most-recently-used list
 + sha1_file: drop free_pack_by_name
 + t/perf: add tests for many-pack scenarios
 (this branch is used by jk/pack-objects-optim-mru and ks/pack-objects-bitmap.)

 "git pack-objects" has a few options that tell it not to pack
 objects found in certain packfiles, which require it to scan .idx
 files of all available packs.  The codepaths involved in these
 operations have been optimized for a common case of not having any
 non-local pack and/or any .kept pack.


* jk/parseopt-string-list (2016-08-03) 1 commit
  (merged to 'next' on 2016-08-04 at a7f0cd2)
 + blame: drop strdup of string literal

 A small memory leak in the command line parsing of "git blame"
 has been plugged.


* jk/reflog-date (2016-07-27) 7 commits
  (merged to 'next' on 2016-08-03 at cd8e92b)
 + date: clarify --date=raw description
 + date: add "unix" format
 + date: document and 

Re: patch submission process, was Re: [PATCH v6 06/16] merge_recursive: abort properly upon errors

[Michael Haggerty]

On 08/04/2016 05:58 PM, Johannes Schindelin wrote:
> [...]
> Even requiring every contributor to register with GitHub would be too much
> of a limitation, I would wager.
> [...]

Is it *really* so insane to consider moving collaboration on the Git
project to GitHub or some other similar platform?

* Many, MANY of the most prominent open-source projects are already
using GitHub. Many potential contributors already know how to use it and
already have accounts. Casual observers (e.g., people who only want to
clone the repo and/or read issues and PRs) don't even need an account.

* Even if you don't already have a GitHub account, it is vastly easier
to create one than to set up our current contribution workflow: figure
out the correct SMTP settings for your email provider, configure
git-send-email, test it and work out the kinks, figure out how to use
git-am (and even then, actually using git-am is a tedious chore for
people who don't use an email client that can run it automatically) [1].
We've seen how difficult our current workflow is by observing GSoC
candidates attempting to send their first patch. What we haven't seen is
the invisible GSoC candidates and other potential contributors who never
even get as far as attempting to send a patch.

* Interactions that involve code are done using Git commands directly,
via exchanging bona fide Git commits. Which means that...

* Commits have unambiguous SHA-1s, which we can use when discussing
them, linking to them, merging them, etc. It will no longer be a matter
of detective work to find out whether a discussion is about v1 or v3 of
a patch series, let alone v3 with some cleanups that were silently added
by Junio.

* Discussion of pull requests can be done either
  * via the website (super easy for beginners but powerful for
experienced users),
  * by setting up email notifications for your account and replying to
those emails, or
  * via an API.
  Such discussion is all in markdown, which supports light formatting,
hyperlinks, and @-mentions.

* GitHub creates permalink URLs for all of the important artifacts:
commits, pull requests, pull request comments, etc. These all can be
referenced easily from any discussion. This web of cross-links
accumulates over time and adds a lot of context to discussions.

* GitHub keeps spam under control.

I admit that if we move to GitHub we would be vulnerable if the company
turns evil or goes bankrupt. But is that really a bigger risk than we
accepted by relying on Gmane (a one-person hobbyist operation) for many
of our historical permalinks, which are now broken? In any case, each of
us has a mirror of the code, and there are utilities for backing up
other GitHub metadata. *If* GitHub becomes evil, there will be a lot of
other open-source projects in the same boat, so I am confident that the
tooling for salvaging such information will quickly become excellent.

Currently we force potential Git contributors to learn an email-based
workflow that is almost unique to this project, rather than steering
them towards a workflow (Git plus, potentially, GitHub) that they
probably already know, or if not is worth learning because the knowledge
will carry across to most other open-source projects, not to mention
their professional careers.

We would want to set down guidelines for how we use GitHub. For example,
we might insist that each version of a patch series (v1, v2, etc.) be
submitted as a fresh pull request with references to the previous
version(s) (which also automatically creates forwards links from the
previous versions to the new version). We might want to set up some
robots to help with repetitive activities, like style review, pinging
the right people, etc.

Junio, I'm very sensitive to the need not to decrease your efficiency.
But isn't there a chance that this could *increase* your efficiency? Is
it worth an experiment?

Is the Git project really such a unique snowflake that we need to use a
workflow (and force it on our contributors) that is different than the
workflows used by most other open-source projects?

Disclaimer: I work for GitHub, but in this email I'm speaking for myself.

Michael

[1] I concede that people who refuse on ideological grounds to use
proprietary software will find this step insurmountable. Perhaps we
could come up with a workaround for such people.

--
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 v10 00/40] libify apply and use lib in am, part 2

[Junio C Hamano]

Christian Couder  writes:

> diff --git a/apply.h b/apply.h
> index 27a3a7a..e2b89e8 100644
> --- a/apply.h
> +++ b/apply.h
> @@ -16,7 +16,7 @@ enum apply_ws_ignore {
>  enum apply_verbosity {
>  verbosity_silent = -1,
>  verbosity_normal = 0,
> -verbosity_verbose = 1,
> +verbosity_verbose = 1
>  };

Thanks for not forgetting this.

> @@ -107,20 +111,6 @@ struct apply_state {
>  int applied_after_fixing_ws;
>  };
>
> -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,
> -  const char *arg, int unset);
> -extern int apply_option_parse_p(const struct option *opt,
> -const char *arg,
> -int unset);
> -extern int apply_option_parse_whitespace(const struct option *opt,
> - const char *arg, int unset);
> -extern int apply_option_parse_directory(const struct option *opt,
> -const char *arg, int unset);
> -extern int apply_option_parse_space_change(const struct option *opt,
> -   const char *arg, int unset);
> -
>  extern int apply_parse_options(int argc, const char **argv,
> struct apply_state *state,
> int *force_apply, int *options,

Also these.
--
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 v10 33/40] environment: add set_index_file()

[Junio C Hamano]

Christian Couder  writes:

> Now if someone really needs to use this new function, 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);
>
> It is intended to be used by builtins commands, in fact only `git am`,
> to temporarily change the index file used by libified code.
>
> This is useful when libified code uses the global index, but a builtin
> command wants another index file to be used instead.

That is OK, but I do not think NO_THE_INDEX_COMPATIBILITY_MACROS has
much to do with this hack.  Even if you stop using the_index and
have the caller pass its own temporary index_state, that structure
does *not* know which file to read the (temporary) index from, or
which file to write the (temporary) index to.  In fact, apply.c
already does this in build_fake_ancestor():

static int build_fake_ancestor(struct patch *list, const char *filename)
{
...
hold_lock_file_for_update(, filename, LOCK_DIE_ON_ERROR);
res = write_locked_index(, , COMMIT_LOCK);
...
}

As you can see, this function works with a non-standard/default
index file _without_ having to use non-default index_state.  What
the set_index_file() hack allows you to do is to use interface that
does *NOT* pass "filename" like the caller does to this function.

Isn't the mention on NO_THE_INDEX_COMPATIBILITY_MACROS in the added
comments (there are two) pure red-herring?
--
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 v10 00/40] libify apply and use lib in am, part 2

[Christian Couder]

On Mon, Aug 8, 2016 at 11:02 PM, Christian Couder
 wrote:
>
> I will send a diff between this version and the previous one, as a
> reply to this email.

Here is the diff:

diff --git a/apply.c b/apply.c
index a73889e..2ec2a8a 100644
--- a/apply.c
+++ b/apply.c
@@ -4324,7 +4324,10 @@ static int try_create_file(const char *path,
unsigned int mode, const char *buf,
 size = nbuf.len;
 buf  = nbuf.buf;
 }
-res = !write_or_whine_pipe(fd, buf, size, path);
+
+res = write_in_full(fd, buf, size) < 0;
+if (res)
+error_errno(_("failed to write to '%s'"), path);
 strbuf_release();

 if (close(fd) < 0 && !res)
@@ -4626,7 +4629,7 @@ static int apply_patch(struct apply_state *state,
 int res = 0;

 state->patch_input_file = filename;
-if (read_patch_file(, fd))
+if (read_patch_file(, fd) < 0)
 return -128;
 offset = 0;
 while (offset < buf.len) {
@@ -4727,16 +4730,16 @@ static int apply_patch(struct apply_state *state,
 return res;
 }

-int apply_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;
 }

-int apply_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);
@@ -4744,9 +4747,9 @@ int apply_option_parse_include(const struct option *opt,
 return 0;
 }

-int apply_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);
@@ -4754,8 +4757,8 @@ int apply_option_parse_p(const struct option *opt,
 return 0;
 }

-int apply_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)
@@ -4765,8 +4768,8 @@ int apply_option_parse_space_change(const struct
option *opt,
 return 0;
 }

-int apply_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;
@@ -4775,8 +4778,8 @@ int apply_option_parse_whitespace(const struct
option *opt,
 return 0;
 }

-int apply_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);
diff --git a/apply.h b/apply.h
index 27a3a7a..e2b89e8 100644
--- a/apply.h
+++ b/apply.h
@@ -16,7 +16,7 @@ enum apply_ws_ignore {
 enum apply_verbosity {
 verbosity_silent = -1,
 verbosity_normal = 0,
-verbosity_verbose = 1,
+verbosity_verbose = 1
 };

 /*
@@ -94,7 +94,11 @@ struct apply_state {
  */
 struct string_list fn_table;

-/* This is to save some reporting routines */
+/*
+ * This is to save reporting routines before using
+ * set_error_routine() or set_warn_routine() to install muting
+ * routines when in verbosity_silent mode.
+ */
 void (*saved_error_routine)(const char *err, va_list params);
 void (*saved_warn_routine)(const char *warn, va_list params);

@@ -107,20 +111,6 @@ struct apply_state {
 int applied_after_fixing_ws;
 };

-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,
-  const char *arg, int unset);
-extern int apply_option_parse_p(const struct option *opt,
-const char *arg,
-int unset);
-extern int apply_option_parse_whitespace(const struct option *opt,
- const char *arg, int unset);
-extern int apply_option_parse_directory(const struct option *opt,
-const char *arg, int unset);
-extern int apply_option_parse_space_change(const struct option *opt,
-   const char *arg, int unset);
-
 extern int apply_parse_options(int argc, const char **argv,
struct apply_state *state,
int *force_apply, int *options,
diff --git a/cache.h 

[PATCH v2] .mailmap: use Christian Couder's Tuxfamily address

[Christian Couder]

Signed-off-by: Christian Couder 
---
 .mailmap | 1 +
 1 file changed, 1 insertion(+)

diff --git a/.mailmap b/.mailmap
index a714e69..9441a54 100644
--- a/.mailmap
+++ b/.mailmap
@@ -33,6 +33,7 @@ Cheng Renquan 
 Chris Shoemaker 
 Chris Wright  
 Cord Seele  
+Christian Couder  
 Christian Stimming  
 Csaba Henk  
 Dan Johnson 
-- 
2.9.2.614.g5428e0c

--
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 v10 24/40] 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 
---
 builtin/apply.c | 38 --
 1 file changed, 16 insertions(+), 22 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index fdfeab0..003acec 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -4287,36 +4287,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(128);
-   }
-   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(128);
-   }
-   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(128);
-   }
-   if (phase == 1) {
-   if (create_file(state, patch))
-   exit(128);
-   }
+   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)
@@ -4403,7 +4396,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(128);
if (phase == 1) {
if (write_out_one_reject(state, l))
errs = 1;
-- 
2.9.2.614.g4980f51

--
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 v10 38/40] apply: change error_routine when silent

[Christian Couder]

To avoid printing anything when applying with
`state->apply_verbosity == verbosity_silent`, let's save the
existing warn and error routines before applying, and let's
replace them with a routine that does nothing.

Then after applying, let's restore the saved routines.

Helped-by: Stefan Beller 
Signed-off-by: Christian Couder 
---
 apply.c | 21 -
 apply.h |  8 
 2 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/apply.c b/apply.c
index ddbb0a2..bf81b70 100644
--- a/apply.c
+++ b/apply.c
@@ -112,6 +112,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;
@@ -144,6 +149,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->apply_verbosity <= verbosity_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;
 }
 
@@ -4864,7 +4876,7 @@ int apply_all_patches(struct apply_state *state,
state->newfd = -1;
}
 
-   return !!errs;
+   res = !!errs;
 
 end:
if (state->newfd >= 0) {
@@ -4872,5 +4884,12 @@ int apply_all_patches(struct apply_state *state,
state->newfd = -1;
}
 
+   if (state->apply_verbosity <= verbosity_silent) {
+   set_error_routine(state->saved_error_routine);
+   set_warn_routine(state->saved_warn_routine);
+   }
+
+   if (res > -1)
+   return res;
return (res == -1 ? 1 : 128);
 }
diff --git a/apply.h b/apply.h
index bd4eb6d..5cde641 100644
--- a/apply.h
+++ b/apply.h
@@ -94,6 +94,14 @@ struct apply_state {
 */
struct string_list fn_table;
 
+   /*
+* This is to save reporting routines before using
+* set_error_routine() or set_warn_routine() to install muting
+* routines when in verbosity_silent mode.
+*/
+   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 apply_ws_error_action ws_error_action;
enum apply_ws_ignore ws_ignore_action;
-- 
2.9.2.614.g4980f51

--
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 v10 26/40] 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 
Helped-by: Jeff King 
Signed-off-by: Christian Couder 
---
 builtin/apply.c | 44 +---
 1 file changed, 33 insertions(+), 11 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index c787ead..9372999 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -4150,38 +4150,48 @@ 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_in_full(fd, buf, size) < 0;
+   if (res)
+   error_errno(_("failed to write to '%s'"), 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;
 }
 
 /*
@@ -4195,15 +4205,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(128);
+   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(128);
+   if (!res)
return;
}
 
@@ -4222,7 +4241,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(128);
+   if (!res) {
if (!rename(newpath, path))
return;
unlink_or_warn(newpath);
-- 
2.9.2.614.g4980f51

--
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 v10 27/40] 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 
---
 builtin/apply.c | 36 +---
 1 file changed, 21 insertions(+), 15 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 9372999..ad403f8 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -4198,32 +4198,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(128);
+   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(128);
+   return -1;
if (!res)
-   return;
+   return 0;
}
 
if (errno == EEXIST || errno == EACCES) {
@@ -4243,10 +4247,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(128);
+   return -1;
if (!res) {
if (!rename(newpath, path))
-   return;
+   return 0;
unlink_or_warn(newpath);
break;
}
@@ -4255,7 +4259,8 @@ static void create_one_file(struct apply_state *state,
++nr;
}
}
-   die_errno(_("unable to write file '%s' mode %o"), path, mode);
+   return error_errno(_("unable to write file '%s' mode %o"),
+  path, mode);
 }
 
 static int add_conflicted_stages_file(struct apply_state *state,
@@ -4300,7 +4305,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.2.614.g4980f51

--
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 v10 32/40] apply: use error_errno() where possible

[Christian Couder]

To avoid possible mistakes and to uniformly show the errno
related messages, let's use error_errno() where possible.

Signed-off-by: Christian Couder 
---
 apply.c | 18 +-
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/apply.c b/apply.c
index c0cb3f5..41a33d3 100644
--- a/apply.c
+++ b/apply.c
@@ -3497,7 +3497,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;
}
@@ -3647,7 +3647,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) {
@@ -3669,7 +3669,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)
@@ -3728,7 +3728,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;
 }
@@ -4247,9 +4247,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, );
}
@@ -4306,7 +4306,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;
 }
@@ -4503,7 +4503,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.2.614.g4980f51

--
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 v10 29/40] 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 
---
 apply.h |  3 +++
 builtin/apply.c | 11 ---
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/apply.h b/apply.h
index 53f09b5..ca1dcee 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 429fe44..9c396bb 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -4463,9 +4463,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.
  *
@@ -4495,8 +4492,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);
@@ -4811,10 +4808,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.2.614.g4980f51

--
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 v10 34/40] apply: make it possible to silently apply

[Christian Couder]

This changes 'int apply_verbosely' into 'enum apply_verbosity', and
changes the possible values of the variable from a bool to
a tristate.

The previous 'false' state is changed into 'verbosity_normal'.
The previous 'true' state is changed into 'verbosity_verbose'.

The new added state is 'verbosity_silent'. It should prevent
anything to be printed on both stderr and stdout.

This is needed because `git am` wants to first call apply
functionality silently, if it can then fall back on 3-way merge
in case of error.

Printing on stdout, and calls to warning() or error() are not
taken care of in this patch, as that will be done in following
patches.

Signed-off-by: Christian Couder 
---
 apply.c | 62 +
 apply.h |  8 +++-
 builtin/apply.c |  2 +-
 3 files changed, 48 insertions(+), 24 deletions(-)

diff --git a/apply.c b/apply.c
index 41a33d3..df85cbc 100644
--- a/apply.c
+++ b/apply.c
@@ -125,8 +125,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->apply_verbosity == verbosity_normal)
+   state->apply_verbosity = verbosity_verbose;
+   }
if (!force_apply && (state->diffstat || state->numstat || 
state->summary || state->check || state->fake_ancestor))
state->apply = 0;
if (state->check_index && is_not_gitdir)
@@ -1620,8 +1623,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->apply_verbosity > verbosity_silent)
+   fprintf(stderr, "%s:%d: %s.\n%.*s\n",
+   state->patch_input_file, linenr, err, len, line);
free(err);
 }
 
@@ -1816,7 +1820,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->apply_verbosity 
> verbosity_silent)
fprintf_ln(stderr,
   _("** warning: "
 "file %s becomes empty but is not deleted"),
@@ -2911,7 +2915,7 @@ static int apply_one_fragment(struct apply_state *state,
/* Ignore it, we already handled it */
break;
default:
-   if (state->apply_verbosely)
+   if (state->apply_verbosity > verbosity_normal)
error(_("invalid start of line: '%c'"), first);
applied_pos = -1;
goto out;
@@ -3026,7 +3030,7 @@ static int apply_one_fragment(struct apply_state *state,
state->apply = 0;
}
 
-   if (state->apply_verbosely && applied_pos != pos) {
+   if (state->apply_verbosity > verbosity_normal && applied_pos != 
pos) {
int offset = applied_pos - pos;
if (state->apply_in_reverse)
offset = 0 - offset;
@@ -3041,14 +3045,14 @@ 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->apply_verbosity > 
verbosity_silent)
fprintf_ln(stderr, _("Context reduced to (%ld/%ld)"
 " to apply fragment at %d"),
   leading, trailing, applied_pos+1);
update_image(state, img, applied_pos, , );
} else {
-   if (state->apply_verbosely)
+   if (state->apply_verbosity > verbosity_normal)
error(_("while searching for:\n%.*s"),
  (int)(old - oldlines), oldlines);
}
@@ -3539,7 +3543,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.");
 
-   

[PATCH v10 39/40] apply: refactor `git apply` option parsing

[Christian Couder]

Parsing `git apply` options can be useful to other commands that
want to call the libified apply functionality, because this way
they can easily pass some options from their own command line to
the libified apply functionality.

This will be used by `git am` in a following patch.

To make this possible, let's refactor the `git apply` option
parsing code into a new libified apply_parse_options() function.

Doing that makes it possible to remove some functions definitions
from "apply.h" and make them static in "apply.c".

Helped-by: Ramsay Jones 
Signed-off-by: Christian Couder 
---
 apply.c | 103 +---
 apply.h |  18 +++---
 builtin/apply.c |  74 ++--
 3 files changed, 97 insertions(+), 98 deletions(-)

diff --git a/apply.c b/apply.c
index bf81b70..2ec2a8a 100644
--- a/apply.c
+++ b/apply.c
@@ -4730,16 +4730,16 @@ static int apply_patch(struct apply_state *state,
return res;
 }
 
-int apply_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;
 }
 
-int apply_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);
@@ -4747,9 +4747,9 @@ int apply_option_parse_include(const struct option *opt,
return 0;
 }
 
-int apply_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);
@@ -4757,8 +4757,8 @@ int apply_option_parse_p(const struct option *opt,
return 0;
 }
 
-int apply_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)
@@ -4768,8 +4768,8 @@ int apply_option_parse_space_change(const struct option 
*opt,
return 0;
 }
 
-int apply_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;
@@ -4778,8 +4778,8 @@ int apply_option_parse_whitespace(const struct option 
*opt,
return 0;
 }
 
-int apply_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);
@@ -4893,3 +4893,80 @@ int apply_all_patches(struct apply_state *state,
return res;
return (res == -1 ? 1 : 128);
 }
+
+int apply_parse_options(int argc, const char **argv,
+   struct apply_state *state,
+   int *force_apply, int *options,
+   const char * const *apply_usage)
+{
+   struct option builtin_apply_options[] = {
+   { 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 },
+   { OPTION_CALLBACK, 'p', NULL, state, N_("num"),
+   N_("remove  leading slashes from traditional diff 
paths"),
+   0, apply_option_parse_p },
+   OPT_BOOL(0, "no-add", >no_add,
+   N_("ignore additions made by the patch")),
+   OPT_BOOL(0, "stat", >diffstat,
+   N_("instead of applying the patch, output diffstat for 
the input")),
+   OPT_NOOP_NOARG(0, "allow-binary-replacement"),
+   OPT_NOOP_NOARG(0, "binary"),
+   OPT_BOOL(0, "numstat", >numstat,
+  

[PATCH v10 16/40] 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 
Signed-off-by: Christian Couder 
---
 builtin/apply.c | 12 +---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index f99498b..eb918e5 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;
}
@@ -1430,6 +1434,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 -128;
if (git_hdr_len <= len)
continue;
if (!patch->old_name && !patch->new_name) {
-- 
2.9.2.614.g4980f51

--
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 v10 40/40] 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 
Signed-off-by: Christian Couder 
---
 builtin/am.c | 65 +++-
 1 file changed, 47 insertions(+), 18 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index b77bf11..54c5728 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.
@@ -1522,39 +1523,67 @@ 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;
+   struct argv_array apply_paths = ARGV_ARRAY_INIT;
+   struct argv_array apply_opts = ARGV_ARRAY_INIT;
+   struct apply_state apply_state;
+   int res, opts_left;
+   char *save_index_file;
+   static struct lock_file lock_file;
+   int force_apply = 0;
+   int options = 0;
+
+   if (index_file) {
+   save_index_file = get_index_file();
+   set_index_file((char *)index_file);
+   }
 
-   cp.git_cmd = 1;
+   if (init_apply_state(_state, NULL, _file))
+   die("BUG: init_apply_state() failed");
+
+   argv_array_push(_opts, "apply");
+   argv_array_pushv(_opts, state->git_apply_opts.argv);
+
+   opts_left = apply_parse_options(apply_opts.argc, apply_opts.argv,
+   _state, _apply, ,
+   NULL);
+
+   if (opts_left != 0)
+   die("unknown option passed thru to git apply");
 
if (index_file)
-   argv_array_pushf(_array, "GIT_INDEX_FILE=%s", 
index_file);
+   apply_state.cached = 1;
+   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) {
-   cp.no_stdout = 1;
-   cp.no_stderr = 1;
-   }
+   if (state->threeway && !index_file)
+   apply_state.apply_verbosity = verbosity_silent;
 
-   argv_array_push(, "apply");
+   if (check_apply_state(_state, force_apply))
+   die("BUG: check_apply_state() failed");
 
-   argv_array_pushv(, state->git_apply_opts.argv);
+   argv_array_push(_paths, am_path(state, "patch"));
+
+   res = apply_all_patches(_state, apply_paths.argc, 
apply_paths.argv, options);
 
if (index_file)
-   argv_array_push(, "--cached");
-   else
-   argv_array_push(, "--index");
+   set_index_file(save_index_file);
 
-   argv_array_push(, am_path(state, "patch"));
+   argv_array_clear(_paths);
+   argv_array_clear(_opts);
+   clear_apply_state(_state);
 
-   if (run_command())
-   return -1;
+   if (res)
+   return res;
 
-   /* Reload index as git-apply will have modified it. */
-   discard_cache();
-   read_cache_from(index_file ? index_file : get_index_file());
+   if (index_file) {
+   /* Reload index as apply_all_patches() will have modified it. */
+   discard_cache();
+   read_cache_from(index_file);
+   }
 
return 0;
 }
-- 
2.9.2.614.g4980f51

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org

[PATCH v10 08/40] 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 
---
 builtin/apply.c | 23 ---
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 10aaba7..06a76f2 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,
@@ -4589,7 +4589,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;
 }
 
@@ -4626,8 +4627,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.2.614.g4980f51

--
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 v10 19/40] 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 
Signed-off-by: Christian Couder 
---
 builtin/apply.c | 41 ++---
 1 file changed, 26 insertions(+), 15 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 166e94d..575981b 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -3900,11 +3900,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...
@@ -3922,31 +3923,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)
@@ -4495,8 +4503,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 = -128;
+   goto end;
+   }
 
if (state->diffstat)
stat_patch_list(state, list);
-- 
2.9.2.614.g4980f51

--
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 v10 37/40] 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 
---
 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 c7a51f8..de04df1 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.2.614.g4980f51

--
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 v10 36/40] 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 
---
 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 590bfdd..c7a51f8 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.2.614.g4980f51

--
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 v10 28/40] 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 
---
 builtin/apply.c | 40 
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index ad403f8..429fe44 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -4588,16 +4588,16 @@ static int apply_patch(struct apply_state *state,
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);
@@ -4605,9 +4605,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);
@@ -4615,8 +4615,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)
@@ -4626,8 +4626,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;
@@ -4636,8 +4636,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);
@@ -4755,13 +4755,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", ,
@@ -4793,13 +4793,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 },
{ OPTION_CALLBACK, 0, "ignore-whitespace", , NULL,
N_("ignore changes in whitespace when finding context"),
-   PARSE_OPT_NOARG, 

[PATCH v10 18/40] 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 a negative
integer instead of calling die(), so while doing that let's change
its name to check_unsafe_path().

Signed-off-by: Christian Couder 
---
 builtin/apply.c | 32 +---
 1 file changed, 21 insertions(+), 11 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 6b16173..166e94d 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -3704,7 +3704,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;
@@ -3716,9 +3716,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;
 }
 
 /*
@@ -3808,8 +3809,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 -128;
 
/*
 * An attempt to read from or delete a path that is beyond a
@@ -3837,10 +3838,14 @@ static int check_patch_list(struct apply_state *state, 
struct patch *patch)
prepare_symlink_changes(state, patch);
prepare_fn_table(state, patch);
while (patch) {
+   int res;
if (state->apply_verbosely)
say_patch_name(stderr,
   _("Checking patch %s..."), patch);
-   err |= check_patch(state, patch);
+   res = check_patch(state, patch);
+   if (res == -128)
+   return -128;
+   err |= res;
patch = patch->next;
}
return err;
@@ -4472,11 +4477,16 @@ static int apply_patch(struct apply_state *state,
goto end;
}
 
-   if ((state->check || state->apply) &&
-   check_patch_list(state, list) < 0 &&
-   !state->apply_with_reject) {
-   res = -1;
-   goto end;
+   if (state->check || state->apply) {
+   int r = check_patch_list(state, list);
+   if (r == -128) {
+   res = -128;
+   goto end;
+   }
+   if (r < 0 && !state->apply_with_reject) {
+   res = -1;
+   goto end;
+   }
}
 
if (state->apply && write_out_results(state, list)) {
-- 
2.9.2.614.g4980f51

--
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 v10 35/40] apply: don't print on stdout in verbosity_silent mode

[Christian Couder]

When apply_verbosity is set to verbosity_silent nothing should be
printed on both stderr and stdout.

To avoid printing on stdout, we can just skip calling the following
functions:

- stat_patch_list(),
- numstat_patch_list(),
- summary_patch_list().

It is safe to do that because the above functions have no side
effects other than printing:

- stat_patch_list() only computes some local values and then call
show_stats() and print_stat_summary(), those two functions only
compute local values and call printing functions,
- numstat_patch_list() also only computes local values and calls
printing functions,
- summary_patch_list() calls show_file_mode_name(), printf(),
show_rename_copy(), show_mode_change() that are only printing.

Signed-off-by: Christian Couder 
---
 apply.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/apply.c b/apply.c
index df85cbc..ddbb0a2 100644
--- a/apply.c
+++ b/apply.c
@@ -4702,13 +4702,13 @@ static int apply_patch(struct apply_state *state,
goto end;
}
 
-   if (state->diffstat)
+   if (state->diffstat && state->apply_verbosity > verbosity_silent)
stat_patch_list(state, list);
 
-   if (state->numstat)
+   if (state->numstat && state->apply_verbosity > verbosity_silent)
numstat_patch_list(state, list);
 
-   if (state->summary)
+   if (state->summary && state->apply_verbosity > verbosity_silent)
summary_patch_list(list);
 
 end:
-- 
2.9.2.614.g4980f51

--
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 v10 01/40] apply: make some names more specific

[Christian Couder]

To prepare for some structs and constants being moved from
builtin/apply.c to apply.h, we should give them some more
specific names to avoid possible name collisions in th global
namespace.

Signed-off-by: Christian Couder 
---
 builtin/apply.c | 20 ++--
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 1a488f9..ab8f0bd 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -21,7 +21,7 @@
 #include "ll-merge.h"
 #include "rerere.h"
 
-enum ws_error_action {
+enum apply_ws_error_action {
nowarn_ws_error,
warn_on_ws_error,
die_on_ws_error,
@@ -29,7 +29,7 @@ enum ws_error_action {
 };
 
 
-enum ws_ignore {
+enum apply_ws_ignore {
ignore_ws_none,
ignore_ws_change
 };
@@ -45,8 +45,8 @@ enum ws_ignore {
  * See also "struct string_list symlink_changes" in "struct
  * apply_state".
  */
-#define SYMLINK_GOES_AWAY 01
-#define SYMLINK_IN_RESULT 02
+#define APPLY_SYMLINK_GOES_AWAY 01
+#define APPLY_SYMLINK_IN_RESULT 02
 
 struct apply_state {
const char *prefix;
@@ -110,8 +110,8 @@ struct apply_state {
struct string_list fn_table;
 
/* These control whitespace errors */
-   enum ws_error_action ws_error_action;
-   enum ws_ignore ws_ignore_action;
+   enum apply_ws_error_action ws_error_action;
+   enum apply_ws_ignore ws_ignore_action;
const char *whitespace_option;
int whitespace_error;
int squelch_whitespace_errors;
@@ -3750,11 +3750,11 @@ static void prepare_symlink_changes(struct apply_state 
*state, struct patch *pat
if ((patch->old_name && S_ISLNK(patch->old_mode)) &&
(patch->is_rename || patch->is_delete))
/* the symlink at patch->old_name is removed */
-   register_symlink_changes(state, patch->old_name, 
SYMLINK_GOES_AWAY);
+   register_symlink_changes(state, patch->old_name, 
APPLY_SYMLINK_GOES_AWAY);
 
if (patch->new_name && S_ISLNK(patch->new_mode))
/* the symlink at patch->new_name is created or remains 
*/
-   register_symlink_changes(state, patch->new_name, 
SYMLINK_IN_RESULT);
+   register_symlink_changes(state, patch->new_name, 
APPLY_SYMLINK_IN_RESULT);
}
 }
 
@@ -3769,9 +3769,9 @@ static int path_is_beyond_symlink_1(struct apply_state 
*state, struct strbuf *na
break;
name->buf[name->len] = '\0';
change = check_symlink_changes(state, name->buf);
-   if (change & SYMLINK_IN_RESULT)
+   if (change & APPLY_SYMLINK_IN_RESULT)
return 1;
-   if (change & SYMLINK_GOES_AWAY)
+   if (change & APPLY_SYMLINK_GOES_AWAY)
/*
 * This cannot be "return 0", because we may
 * see a new one created at a higher level.
-- 
2.9.2.614.g4980f51

--
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 v10 14/40] builtin/apply: make apply_all_patches() return 128 or 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 either 128 or 1, so that it
gives the same exit code as when die() or exit(1) is called. This way
scripts relying on the exit code don't need to be changed.

While doing that we must take care that file descriptors are properly closed
and, if needed, reset to 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: Nguyễn Thái Ngọc Duy 
Helped-by: Johannes Schindelin 
Helped-by: Eric Sunshine 
Signed-off-by: Christian Couder 
---
 builtin/apply.c | 37 ++---
 1 file changed, 26 insertions(+), 11 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 075ada4..5530ba1 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -4578,15 +4578,18 @@ 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));
+   res = -128;
+   goto end;
+   }
read_stdin = 0;
set_default_whitespace_mode(state);
res = apply_patch(state, fd, arg, options);
+   close(fd);
if (res < 0)
goto end;
errs |= res;
-   close(fd);
}
set_default_whitespace_mode(state);
if (read_stdin) {
@@ -4606,11 +4609,14 @@ 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);
+   res = -128;
+   goto end;
+   }
if (state->applied_after_fixing_ws && state->apply)
warning("%d line%s applied after"
" fixing whitespace errors.",
@@ -4624,15 +4630,24 @@ 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"));
+   res = -128;
+   goto end;
+   }
state->newfd = -1;
}
 
return !!errs;
 
 end:
-   exit(res == -1 ? 1 : 128);
+   if (state->newfd >= 0) {
+   rollback_lock_file(state->lock_file);
+   state->newfd = -1;
+   }
+
+   return (res == -1 ? 1 : 128);
 }
 
 int cmd_apply(int argc, const char **argv, const char *prefix)
-- 
2.9.2.614.g4980f51

--
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 v10 20/40] 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 
---
 builtin/apply.c | 17 +++--
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 575981b..27fb6e2 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -4085,17 +4085,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,
@@ -4274,8 +4275,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(128);
+   }
return;
}
if (patch->is_new > 0 || patch->is_copy) {
@@ -4287,8 +4290,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(128);
+   }
if (phase == 1)
create_file(state, patch);
 }
-- 
2.9.2.614.g4980f51

--
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 v10 13/40] 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 
---
 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 6e0e992..2eac3e3 100644
--- a/apply.c
+++ b/apply.c
@@ -93,3 +93,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 e18a18a..53f09b5 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 bb89e07..075ada4 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -4551,38 +4551,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.2.614.g4980f51

--
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 v10 17/40] 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 
Signed-off-by: Christian Couder 
---
 builtin/apply.c | 40 +---
 1 file changed, 21 insertions(+), 19 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index eb918e5..6b16173 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.2.614.g4980f51

--
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 v10 07/40] 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 a negative
integer 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 
---
 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 c07d142..10aaba7 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -1671,6 +1671,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,
@@ -1688,8 +1692,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;
@@ -1725,9 +1731,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: "
@@ -2029,6 +2035,9 @@ static int parse_chunk(struct apply_state *state, char 
*buffer, unsigned long si
   size - offset - hdrsize,
   patch);
 
+   if (patchsize < 0)
+   return -128;
+
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.2.614.g4980f51

--
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 v10 33/40] environment: add set_index_file()

[Christian Couder]

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

Yeah, this is a short cut and this new function should not be used
by other code.

It adds a small technical debt, because unfortunately a very big
technical debt already exists as the apply code and a lot of other
"libified" code already call functions like read_cache(),
discard_cache() and cache_name_pos() instead of functions like
read_index_from(), discard_index() and index_name_pos() that are
available when the NO_THE_INDEX_COMPATIBILITY_MACROS env variable
is defined.

Avoiding this small new technical debt is unfortunately not as simple
as just changing these functions in "apply.c", as these functions can
be called by other "libified" code that can indirectly be called by
apply code.

For example cache_name_pos() is used in "dir.c" and "diff.c", and then
"dir.h" and "diff.h" are included in many other "libified" *.c files
(including "apply.c"). So it is very difficult to make sure that apply
code doesn't indirectly call any of the problematic functions.

And even if it was possible to make sure that now "apply.c" doesn't
indirectly call any of these functions, how could we make sure that
later a refactoring in other "libified" code will not change a
function that is indirectly called by "apply.c" to make it call another
function that indirectly calls a problematic function?

So it's a different project altogether to remove calls to problematic
functions (like read_cache(), discard_cache(), cache_name_pos() and so
on) in all the libified code, starting maybe with "dir.c" and "diff.c".

Now if someone really needs to use this new function, 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);

It is intended to be used by builtins commands, in fact only `git am`,
to temporarily change the index file used by libified code.

This is useful when libified code uses the global index, but a builtin
command wants another index file to be used instead.

Helped-by: Stefan Beller 
Signed-off-by: Christian Couder 
---
 cache.h   | 13 +
 environment.c | 16 
 2 files changed, 29 insertions(+)

diff --git a/cache.h b/cache.h
index b5f76a4..c9ad7f9 100644
--- a/cache.h
+++ b/cache.h
@@ -471,6 +471,19 @@ extern const char *strip_namespace(const char 
*namespaced_ref);
 extern const char *get_git_work_tree(void);
 
 /*
+ * Hack to temporarily change the index.
+ * Yeah, the libification of 'apply' took a short-circuit by adding
+ * this technical debt.
+ * Please use functions available when
+ * NO_THE_INDEX_COMPATIBILITY_MACROS is defined, instead of this
+ * function.
+ * If you really need to use this function, please save the current
+ * index file using get_index_file() before changing the index
+ * file. And when finished, reset it to the saved value.
+ */
+extern void set_index_file(char *index_file);
+
+/*
  * Return true if the given path is a git directory; note that this _just_
  * looks at the directory itself. If you want to know whether "foo/.git"
  * is a repository, you must feed that path, not just "foo".
diff --git a/environment.c b/environment.c
index ca72464..55b2b6b 100644
--- a/environment.c
+++ b/environment.c
@@ -292,6 +292,22 @@ int odb_pack_keep(char *name, size_t namesz, const 
unsigned char *sha1)
return open(name, O_RDWR|O_CREAT|O_EXCL, 0600);
 }
 
+/*
+ * Hack to temporarily change the index.
+ * Yeah, the libification of 'apply' took a short-circuit by adding
+ * this technical debt.
+ * Please use functions available when
+ * NO_THE_INDEX_COMPATIBILITY_MACROS is defined, instead of this
+ * function.
+ * If you really need to use this function, 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.2.614.g4980f51

--
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 v10 15/40] 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 
---
 builtin/apply.c | 15 +--
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 5530ba1..f99498b 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,
@@ -1467,7 +1469,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 -128;
*hdrsize = len + nextlen;
state->linenr += 2;
return offset;
-- 
2.9.2.614.g4980f51

--
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 v10 31/40] 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 
---
 apply.c | 6 +++---
 apply.h | 5 -
 2 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/apply.c b/apply.c
index 7b96130..c0cb3f5 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 5ec022c..df44b51 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.2.614.g4980f51

--
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 v10 25/40] 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 
Signed-off-by: Christian Couder 
---
 builtin/apply.c | 27 +--
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 003acec..c787ead 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -4383,6 +4383,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;
@@ -4396,8 +4402,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(128);
+   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;
@@ -4517,10 +4525,17 @@ static int apply_patch(struct apply_state *state,
}
}
 
-   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 = -128;
+   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.2.614.g4980f51

--
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 v10 11/40] 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().

Signed-off-by: Christian Couder 
---
 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 c858ca4..6e0e992 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;
@@ -79,9 +79,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 08c0a25..e18a18a 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 bb6ff77..61fd316 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -4741,7 +4741,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(128);
 
argc = parse_options(argc, argv, state.prefix, builtin_apply_options,
apply_usage, 0);
-- 
2.9.2.614.g4980f51

--
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 v10 23/40] 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 
---
 builtin/apply.c | 25 +
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index a646900..fdfeab0 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -4269,7 +4269,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;
@@ -4280,13 +4280,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(128);
-   } else {
-   if (add_index_file(state, path, mode, buf, size))
-   exit(128);
-   }
+   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 */
@@ -4302,8 +4299,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(128);
+   }
return;
}
/*
@@ -4314,8 +4313,10 @@ static void write_out_one_result(struct apply_state 
*state,
if (remove_file(state, patch, patch->is_rename))
exit(128);
}
-   if (phase == 1)
-   create_file(state, patch);
+   if (phase == 1) {
+   if (create_file(state, patch))
+   exit(128);
+   }
 }
 
 static int write_out_one_reject(struct apply_state *state, struct patch *patch)
-- 
2.9.2.614.g4980f51

--
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 v10 21/40] 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 
Signed-off-by: Christian Couder 
---
 builtin/apply.c | 20 +---
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 27fb6e2..ad0b875 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -4224,7 +4224,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;
@@ -4232,7 +4232,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);
@@ -4247,9 +4247,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)
@@ -4263,9 +4268,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(128);
+   } else
add_index_file(state, path, mode, buf, size);
 }
 
-- 
2.9.2.614.g4980f51

--
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 v10 06/40] 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 a negative integer
instead of calling die() or exit().

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

This makes it compatible with what find_header() and parse_binary()
already return.

Helped-by: Eric Sunshine 
Signed-off-by: Christian Couder 
---
 builtin/apply.c | 22 ++
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 434ba0c..c07d142 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -1996,22 +1996,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 if no header was found or parse_binary() failed,
+ *   -128 on another error,
+ *   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 == -128)
-   exit(128);
-
if (offset < 0)
return offset;
 
@@ -2071,8 +2071,10 @@ static int parse_chunk(struct apply_state *state, char 
*buffer, unsigned long si
 * empty to us here.
 */
if ((state->apply || state->check) &&
-   (!patch->is_binary && !metadata_changes(patch)))
-   die(_("patch with only garbage at line %d"), 
state->linenr);
+   (!patch->is_binary && !metadata_changes(patch))) {
+   error(_("patch with only garbage at line %d"), 
state->linenr);
+   return -128;
+   }
}
 
return offset + hdrsize + patchsize;
@@ -4455,6 +4457,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 == -128) {
+   res = -128;
+   goto end;
+   }
break;
}
if (state->apply_in_reverse)
-- 
2.9.2.614.g4980f51

--
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 v10 03/40] builtin/apply: make apply_patch() return -1 or -128 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 or -128 in case of errors instead of dying. For now its only
caller apply_all_patches() will exit(128) when apply_patch()
return -128 and it will exit(1) when it returns -1.

We exit() with code 128 because that was what die() was doing
and we want to keep the distinction between exiting with code 1
and exiting with code 128.

Helped-by: Eric Sunshine 
Signed-off-by: Christian Couder 
---
 builtin/apply.c | 60 ++---
 1 file changed, 45 insertions(+), 15 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index ed923ca..435030a 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -4404,6 +4404,15 @@ static struct lock_file lock_file;
 #define INACCURATE_EOF (1<<0)
 #define RECOUNT(1<<1)
 
+/*
+ * Try to apply a patch.
+ *
+ * Returns:
+ *  -128 if a bad error happened (like patch unreadable)
+ *  -1 if patch did not apply and user cannot deal with it
+ *   0 if the patch applied
+ *   1 if the patch did not apply but user might fix it
+ */
 static int apply_patch(struct apply_state *state,
   int fd,
   const char *filename,
@@ -4413,6 +4422,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 +4455,11 @@ static int apply_patch(struct apply_state *state,
offset += nr;
}
 
-   if (!list && !skipped_patch)
-   die(_("unrecognized input"));
+   if (!list && !skipped_patch) {
+   error(_("unrecognized input"));
+   res = -128;
+   goto end;
+   }
 
if (state->whitespace_error && (state->ws_error_action == 
die_on_ws_error))
state->apply = 0;
@@ -4455,21 +4468,23 @@ 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) {
+   error(_("unable to read index file"));
+   res = -128;
+   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 +4499,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)
@@ -4628,6 +4644,7 @@ static int apply_all_patches(struct apply_state *state,
 int options)
 {
int i;
+   int res;
int errs = 0;
int read_stdin = 1;
 
@@ -4636,7 +4653,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)
+   goto end;
+   errs |= res;
read_stdin = 0;
continue;
} else if (0 < state->prefix_length)
@@ -4649,12 +4669,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)
+   goto end;
+   errs |= res;
close(fd);
}
set_default_whitespace_mode(state);
-   if (read_stdin)
-   errs |= apply_patch(state, 0, "", options);
+   if (read_stdin) {
+   

[PATCH v10 09/40] 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 
---
 builtin/apply.c | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 06a76f2..ecb1f7a 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)
@@ -4629,8 +4629,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.2.614.g4980f51

--
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 v10 05/40] builtin/apply: make find_header() return -128 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, let's make find_header() return -128 instead of
calling die().

We could make it return -1, unfortunately find_header() already
returns -1 when no header is found.

Signed-off-by: Christian Couder 
---
 builtin/apply.c   | 40 
 t/t4254-am-corrupt.sh |  2 +-
 2 files changed, 29 insertions(+), 13 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index dd7afee..434ba0c 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 if no header was found
+ *  -128 in case of error
+ *   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,9 @@ 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);
+   error(_("patch fragment without header at line %d: 
%.*s"),
+state->linenr, (int)len-1, line);
+   return -128;
}
 
if (size < len + 6)
@@ -1468,19 +1477,23 @@ static int find_header(struct apply_state *state,
if (git_hdr_len <= len)
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);
+   if (!patch->def_name) {
+   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);
+   return -128;
+   }
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);
+   if (!patch->is_delete && !patch->new_name) {
+   error("git diff header lacks filename 
information "
+"(line %d)", state->linenr);
+   return -128;
+   }
patch->is_toplevel_relative = 1;
*hdrsize = git_hdr_len;
return offset;
@@ -1996,6 +2009,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 == -128)
+   exit(128);
+
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)" 
>expected &&
+   echo "error: git diff header lacks filename information (line 4)" 
>expected &&
test_path_is_file f &&
test_cmp expected actual
 '
-- 

[PATCH v10 02/40] 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.

Signed-off-by: Christian Couder 
---
 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..7493a40
--- /dev/null
+++ b/apply.h
@@ -0,0 +1,100 @@
+#ifndef APPLY_H
+#define APPLY_H
+
+enum apply_ws_error_action {
+   nowarn_ws_error,
+   warn_on_ws_error,
+   die_on_ws_error,
+   correct_ws_error
+};
+
+enum apply_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 APPLY_SYMLINK_GOES_AWAY 01
+#define APPLY_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 apply_ws_error_action ws_error_action;
+   enum apply_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 ab8f0bd..ed923ca 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 apply_ws_error_action {
-   nowarn_ws_error,
-   warn_on_ws_error,
-   die_on_ws_error,
-   correct_ws_error
-};
-
-
-enum apply_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 APPLY_SYMLINK_GOES_AWAY 01
-#define APPLY_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 

[PATCH v10 12/40] 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 
---
 builtin/apply.c | 19 +++
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 61fd316..bb89e07 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -4551,17 +4551,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)
@@ -4569,16 +4569,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,
@@ -4747,7 +4749,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(128);
 
ret = apply_all_patches(, argc, argv, options);
 
-- 
2.9.2.614.g4980f51

--
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 v10 22/40] 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 
---
 builtin/apply.c | 48 +++-
 1 file changed, 31 insertions(+), 17 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index ad0b875..a646900 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -4099,11 +4099,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;
@@ -4111,7 +4111,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);
@@ -4122,20 +4122,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)
@@ -4271,8 +4283,10 @@ static void create_file(struct apply_state *state, 
struct patch *patch)
if (patch->conflicted_threeway) {
if (add_conflicted_stages_file(state, patch))
exit(128);
-   } else
-   add_index_file(state, path, mode, buf, size);
+   } else {
+   if (add_index_file(state, path, mode, buf, size))
+   exit(128);
+   }
 }
 
 /* phase zero is to remove, phase one is to create */
-- 
2.9.2.614.g4980f51

--
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 v10 10/40] 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 
Signed-off-by: Christian Couder 
---
 Makefile|  1 +
 apply.c | 94 +
 apply.h | 10 ++
 builtin/apply.c | 91 ---
 4 files changed, 105 insertions(+), 91 deletions(-)
 create mode 100644 apply.c

diff --git a/Makefile b/Makefile
index 6a13386..3230fd0 100644
--- a/Makefile
+++ b/Makefile
@@ -683,6 +683,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..c858ca4
--- /dev/null
+++ b/apply.c
@@ -0,0 +1,94 @@
+#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;
+   string_list_init(>fn_table, 0);
+   string_list_init(>limit_by_name, 0);
+   string_list_init(>symlink_changes, 0);
+   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 7493a40..08c0a25 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,
+const char *prefix,
+struct lock_file *lock_file);
+extern void clear_apply_state(struct apply_state *state);
+
 #endif
diff --git a/builtin/apply.c 

[PATCH v10 04/40] 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: Stefan Beller 
Signed-off-by: Christian Couder 
---
 builtin/apply.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 435030a..dd7afee 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)
@@ -4425,7 +4426,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) < 0)
+   return -128;
offset = 0;
while (offset < buf.len) {
struct patch *patch;
-- 
2.9.2.614.g4980f51

--
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 v10 00/40] libify apply and use lib in am, part 2

[Christian Couder]

Goal


This is a patch series about libifying `git apply` functionality, and
using this libified functionality in `git am`, so that no 'git apply'
process is spawn anymore. This makes `git am` significantly faster, so
`git rebase`, when it uses the am backend, is also significantly
faster.

Previous discussions and patches series
~~~

This has initially been discussed in the following thread:

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

Then the following patch series were sent:

RFC: http://thread.gmane.org/gmane.comp.version-control.git/288489/
v1: http://thread.gmane.org/gmane.comp.version-control.git/292324/
v2: http://thread.gmane.org/gmane.comp.version-control.git/294248/
v3: http://thread.gmane.org/gmane.comp.version-control.git/295429/
v4: http://thread.gmane.org/gmane.comp.version-control.git/296350/
v5: http://thread.gmane.org/gmane.comp.version-control.git/296490/
v6: http://thread.gmane.org/gmane.comp.version-control.git/297024/
v7: http://thread.gmane.org/gmane.comp.version-control.git/297193/
v8: 
https://public-inbox.org/git/20160627182429.31550-1-chriscool%40tuxfamily.org/
v9: 
https://public-inbox.org/git/20160730172509.22939-1-chriscool%40tuxfamily.org/

Highlevel view of the patches in the series
~~~

This is "part 2" of the full patch series. This is built on top of the
"part 1" and as the "part 1" is now in "master", this "part 2" is
built on top of "master".

  - Patch 01/40 was in v8 and v9, and hasn't changed.

It renames some structs and constants that will be moved into apply.h
to give them a more specific name.

  - Patches 02/40 to 31/40 were in v1, v2, v6, v7, v8 and v9.

They finish libifying the apply functionality that was in
builtin/apply.c and move it into apply.{c,h}, but the libified
functionality is not yet used in `git am`.

There are a few minor changes in these patches. In 04/40 we now
consider that we have an error if read_patch_file() returns a negative
integer instead of just a non 0 integer, as Stefan suggested. In 26/40
we now use write_in_full() instead of write_or_whine_pipe(), as
suggested by Peff.

  - Patch 32/40 was in v6, v7, v8 and v9, and hasn't changed.

It replaces some calls to error() with calls to error_errno().

  - Patch 33/40 was in v2, v6, v7, v8 and v9.

It makes it possible to temporarily change the current index.

This is a hack to make it possible for `git am` to use the libified
apply functionality on a different index file.

`git am` used to do that by setting the GIT_INDEX_FILE env variable
before calling `git apply`.

The commit message has been improved again to explain more why we are
using this short cut and more comments have been added, especially in
apply.h, as suggested by Junio and Stefan.

  - Patches 34/40 to 38/40 were in v2, v6, v7, v8 and v9.

They implement a way to make the libified apply code silent by
changing the bool `apply_verbosely` into a tristate enum called
`apply_verbosity`, that can be one of `verbosity_verbose`,
`verbosity_normal` or `verbosity_silent`.

This is because "git am", since it was a shell script, has been
silencing the apply functionality by redirecting file descriptors to
/dev/null, but this is not acceptable in C.

The most significant change in these patches is that one patch (34/41
in v9: write_or_die: use warning() instead of fprintf(stderr, ...))
has been removed, as it was not needed anymore, since we don't use
write_or_whine_pipe(), as suggested by Peff.

Another change is that in 34/40, a spurious coma has been removed just
after the last element in an enum, as suggested by Junio.

The last change is that a comment has been improved in 38/40 as
suggested by Stefan.

  - Patch 39/40 was new in v9.

It refactors `git apply` option parsing to make it possible for `git
am` to easily pass some command line options to the libified applied
code as suggested by Junio.

Compared to v9, we now remove some useless function declarations from
"apply.h", and make the related functions static again in "apply.c",
as suggested by Ramsay.

  - Patch 40/40 was in v1, v2, v6, v7, v8 and v9, and hasn't changed.

This patch makes `git am` use the libified functionality. It now uses
the refactored code from the new patch 40/40 to parse `git apply`
options.


General comments


Sorry if this patch series is still long. Hopefully the early part of
this series until 32/40 will be ready soon to be moved to next and
master, and I may only need to resend the rest.

I will send a diff between this version and the previous one, as a
reply to this email.

The benefits are not just related to not creating new processes. When
`git am` launched a `git apply` process, this new process had to read
the index from disk. Then after the `git apply`process had terminated,
`git am` dropped its index and read the index from disk to get the
index that had been modified by the `git apply`process. This was
inefficient 

Re: [PATCH v5] pack-objects: teach it to use reachability bitmap index when generating non-stdout pack too

[Junio C Hamano]

Kirill Smelkov  writes:

> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index bc1c433..4ba0c4a 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2244,6 +2244,9 @@ pack.useBitmaps::
>   to stdout (e.g., during the server side of a fetch). Defaults to
>   true. You should not generally need to turn this off unless
>   you are debugging pack bitmaps.
> ++
> +*NOTE*: when packing to file (e.g., on repack) the default is always not to 
> use
> + pack bitmaps.

This is a bit hard to read and understand.

The patched result starts with "When true, git will use bitmap when
packing to stdout", i.e. when packing to file, git will not.  So
this *NOTE* is repeating the same thing.  The reader is made to
wonder "Why does it need to repeat the same thing?  Does this mean
when the variable is set, a pack sent to a disk uses the bitmap?"

I think what you actually do in the code is to make the variable
affect _only_ the standard-output case, and users need a command
line option if they want to use bitmap when writing to a file (the
code to do so looks correctly done).

> diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
> index 3893afd..ffecc6a 100755
> --- a/t/t5310-pack-bitmaps.sh
> +++ b/t/t5310-pack-bitmaps.sh
> @@ -118,6 +118,18 @@ test_expect_success 'incremental repack can disable 
> bitmaps' '
>   git repack -d --no-write-bitmap-index
>  '
>  
> +test_expect_success 'pack-objects to file can use bitmap' '
> + # make sure we still have 1 bitmap index from previous tests
> + ls .git/objects/pack/ | grep bitmap >output &&
> + test_line_count = 1 output &&
> + # verify equivalent packs are generated with/without using bitmap index
> + packasha1=$(git pack-objects --no-use-bitmap-index --all packa 
>  + packbsha1=$(git pack-objects --use-bitmap-index --all packb  &&
> + git show-index packa.objects &&
> + git show-index packb.objects &&
> + test_cmp packa.objects packb.objects
> +'

Looks good.
--
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: t0027 racy?

[Torsten Bögershausen]

On Mon, Aug 08, 2016 at 11:29:26AM -0400, Jeff King wrote:
> On Mon, Aug 08, 2016 at 05:05:07PM +0200, Johannes Schindelin wrote:
> 
> > I remember that you did a ton of work on t0027. Now I see problems, and
> > not only that the entire script now takes a whopping 4 minutes 20 seconds
> > to run on my high-end Windows machine.
> > 
> > It appears that t0027 fails randomly for me, in seemingly random places.
> > Sometimes all 1388 cases pass. Sometimes "29 - commit NNO files crlf=true
> > attr=auto LF" fails. Sometimes it is "24 - commit NNO files crlf=false
> > attr=auto LF". Sometimes it is "114 - commit NNO files crlf=false
> > attr=auto LF", and sometimes "111 - commit NNO files attr=auto aeol=lf
> > crlf=false CRLF_mix_LF".
> > 
> > When I run it with -i -v -x --tee, it passes every single time (taking
> > over 5 minutes, just to make things worse)...
> > 
> > Any idea about any possible races?
> 
> Try:
> 
>   https://github.com/peff/git/blob/meta/stress
> 
> which you can run as "sh /path/to/stress t0027" in the top-level of your
> git repository. I got failure within about 30 seconds on t0027 (though 5
> minutes? Yeesh. It runs in 9s on my laptop. I weep for you).
> 
> The verbose output is not very exciting, though:
> 
>   expecting success: 
>   check_warning "$lfwarn" ${pfx}_LF.err
> 
>   --- NNO_attr_auto_aeol_crlf_false_LF.err.expect 2016-08-08 
> 15:26:37.061701392 +
>   +++ NNO_attr_auto_aeol_crlf_false_LF.err.actual 2016-08-08 
> 15:26:37.061701392 +
>   @@ -1 +0,0 @@
>   -warning: LF will be replaced by CRLF
>   not ok 114 - commit NNO files crlf=false attr=auto LF

(I realized that t0027 is not yet self-explaining, I have it on my list)

NNO_attr_auto_aeol_crlf_false_LF means:

NNO:   "Not NOrmalized" A file had been commited with CRLF in the repo
attr_auto: .gitattributes has "* text=auto"
aeol_eol   .gitattributes has "eol=crlf"
crlf_false git config core.autocrlf = false
LF We commit a file with LF line endings.

This should happend:
- The file is commited "as is", with LF line endings.
- While commiting, git should print the warning
- "warning: LF will be replaced by CRLF" to stderr
- stderr is piped (redirected) into NNO_attr_auto_aeol_crlf_false_LF.err
- we grep for "will be replaced by" in xx.err and pipe it into xx.err.actual

The rest is test_cmp, this is what you see.
The warning is missing, but should be there:

The file has LF, and after commit and a new checkout these LF will
be convertet into CRLF.

So why isn't the warning there (but here on my oldish machines)
--
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] merge: use string_list_split() in add_strategies()

[Junio C Hamano]

Junio C Hamano  writes:

> If the input comes from the end user, we certainly would want to
> allow "word1 word2\tword3 " as input (i.e. squishing repeated

Any intelligent reader may have guessed already, but before I
stupidly told Emacs to refill the paragraph, the above example had
two SPs between word1 and word2.  Sorry for being sloppy.

> delimiters into one without introducing an "empty" element, allowing
> more than one delimiter characters like SP and HT, and ignoring
> leading or trailing runs of delimiter characters).
>
> If the input is generated internally, perhaps we should rethink the
> interface between the function that wants to do the for-each-word
> and its caller; if the caller wants to pass multiple things to the
> callee, it should be able to do so without first having to stuff
> these multiple things into a single string, only to force the callee
> to use this helper to split them out into individual pieces.
--
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: storing cover letter of a patch series?

[Junio C Hamano]

Stefan Beller  writes:

> On Thu, Sep 10, 2015 at 9:28 AM, Jacob Keller  wrote:
>> Hey,
>>
>> does anyone know of any tricks for storing a cover letter for a patch
>> series inside of git somehow? I'd guess the only obvious way currently
>> is to store it at the top of the series as an empty commit.. but this
>> doesn't get emailed *as* the cover letter...
>>
>> Is there some other way? Would others be interested in such a feature?
>
> Being late to this thread, but I think
>
>branch..description
>Branch description, can be edited with git branch
>--edit-description. Branch description is automatically added in
>the format-patch cover letter or request-pull summary.
>
> is what you want. Maybe we want to see a patch that adds the reverse
> functionality as well, i.e. git-am will store the the cover letter as the
> branch description and git-merge will propose the branch description for
> the merge commit.

Yes, but... ;-)  It is a bit too weak to be called a proper part of
a "version control system", in that the description, even though it
can be edited with "--edit-description", is not versioned.

It is consistent with the fact that rerolls of your branch by
rebuilding with "rebase -i" or "checkout --detached && until
satisified; do cherry-pick && commit --amend; done" is not versioned
and you may resort to the old-fashioned my-topic, my-topic-v2,
my-topic-v3, ..., but being consistent with a bad part of the system
does not deny the fact that it is a weak feature.
--
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/2] pack-objects: Teach --use-bitmap-index codepath to respect --local, --honor-pack-keep and --incremental

[Jeff King]

On Mon, Aug 08, 2016 at 12:06:13PM -0700, Junio C Hamano wrote:

> Jeff King  writes:
> 
> >> +  if grep -qFf nonlocal-loose 1.objects; then
> >> +  echo "Non-local object present in pack generated with --local"
> >> +  return 1
> >> +  fi
> >> +'
> >
> > grep -f isn't portable. However, I think:
> >
> >   echo $objsha1 >expect &&
> >   git show-index <1.pack | cut -d' ' -f2 >actual
> >   test_cmp expect actual
> >
> > would work (if you do stick with two entries, you might need to sort
> > your "expect").
> 
> Hmph, are you sure?  "grep -f pattern_file" is in POSIX.1.

Hmm, you're right. I specifically checked my local grep.1posix manpage,
but searching for "-f" didn't turn up anything, because it's formatted
with a Unicode minus sign (U+2212). Bleh.

-Peff
--
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/2] pack-objects: Teach --use-bitmap-index codepath to respect --local, --honor-pack-keep and --incremental

[Junio C Hamano]

Kirill Smelkov  writes:

>  8< 
> From: Kirill Smelkov 
> Subject: [PATCH v3] pack-objects: Teach --use-bitmap-index codepath to respect
>  --local, --honor-pack-keep and --incremental

(Not a question to Kirill)

Hmph.  I suspect that handling of in-body header by mailinfo not
prepared to see RFC2822 header folding.  "am -c" gives a single line
subject with " --local ..." as its first line in the body.

I'll leave it as a low-hanging fruit for somebody to fix ;-)

Subject: pack-objects: respect --local, etc. when bitmap is in use

might be shorter and more to the point, anyway.

> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index c4c2a3c..e06c1bf 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -944,13 +944,45 @@ static int have_duplicate_entry(const unsigned char 
> *sha1,
>   return 1;
>  }
>  
> +static int want_found_object(int exclude, struct packed_git *p)
> +{
> + if (exclude)
> + return 1;
> + if (incremental)
> + return 0;
> +
> + /*
> +  * When asked to do --local (do not include an
> +  * object that appears in a pack we borrow
> +  * from elsewhere) or --honor-pack-keep (do not
> +  * include an object that appears in a pack marked
> +  * with .keep), we need to make sure no copy of this
> +  * object come from in _any_ pack that causes us to
> +  * omit it, and need to complete this loop.  When
> +  * neither option is in effect, we know the object
> +  * we just found is going to be packed, so break
> +  * out of the search loop now.
> +  */

The blame is mine, but "no copy of this object appears in _any_ pack"
would be more correct and easier to read.

This code is no longer in a search loop; its caller is.  Further
rephrasing is needed.  "When asked to do ...these things..., finding
a pack that matches the criteria is sufficient for us to decide to
omit it.  However, even if this pack does not satisify the criteria,
we need to make sure no copy of this object appears in _any_ pack
that makes us to omit the object, so we need to check all the packs.
Signal that by returning -1 to the caller." or something along that
line.

>  /*
>   * Check whether we want the object in the pack (e.g., we do not want
>   * objects found in non-local stores if the "--local" option was used).
>   *
> - * As a side effect of this check, we will find the packed version of this
> - * object, if any. We therefore pass out the pack information to avoid having
> - * to look it up again later.
> + * As a side effect of this check, if object's pack entry was not already 
> found,
> + * we will find the packed version of this object, if any. We therefore pass
> + * out the pack information to avoid having to look it up again later.

The reasoning leading to "We therefore" is understandable, but "pass
out the pack information" is not quite.  Is this meant to explain
the fact that *found_pack and *found_offset are in-out parameters?

The explanation to justify why *found_pack and *found_offset that
used to be out parameters are made in-out parameters belongs to the
log message.  We do not want this in-code comment to explain the
updated code relative to what the code used to do; that is not
useful to those who read the code for the first time in the context
of the committed state.

/* 
 * Check whether we want to pack the object in the pack (e.g. ...).
 *
 * If the caller already knows an existing pack it wants to
 * take the object from, that is passed in *found_pack and
 * *found_offset; otherwise this function finds if there is
 * any pack that has the object and returns the pack and its
 * offset in these variables.
 */

> diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
> index 3893afd..e71caa4 100755
> --- a/t/t5310-pack-bitmaps.sh
> +++ b/t/t5310-pack-bitmaps.sh
> @@ -7,6 +7,19 @@ objpath () {
>   echo ".git/objects/$(echo "$1" | sed -e 's|\(..\)|\1/|')"
>  }
>  
> +# show objects present in pack ($1 should be associated *.idx)
> +packobjects () {
> + git show-index <$1 | cut -d' ' -f2
> +}

That is a misleading name for a helper function that produces a list
of objects that were packed.  "list_packed_objects", perhaps.

> +# hasany pattern-file content-file
> +# tests whether content-file has any entry from pattern-file with entries 
> being
> +# whole lines.
> +hasany () {
> + # NOTE `grep -f` is not portable
> + git grep --no-index -qFf $1 $2
> +}

I doubt "grep -f pattern_file" is not portable, but in any case, it
is probably a good idea to have this helper function to make the
caller easier to read.  Please name it "has_any", though, and quote
"$1" and "$2" as they are meant to be able to take any filename.

> +test_expect_success 'pack-objects respects --local (non-local loose)' '
> + mkdir -p alt_objects/pack &&

I'd really 

Re: [PATCH 1/2] pack-objects: Teach --use-bitmap-index codepath to respect --local, --honor-pack-keep and --incremental

[Junio C Hamano]

Jeff King  writes:

>> +if grep -qFf nonlocal-loose 1.objects; then
>> +echo "Non-local object present in pack generated with --local"
>> +return 1
>> +fi
>> +'
>
> grep -f isn't portable. However, I think:
>
>   echo $objsha1 >expect &&
>   git show-index <1.pack | cut -d' ' -f2 >actual
>   test_cmp expect actual
>
> would work (if you do stick with two entries, you might need to sort
> your "expect").

Hmph, are you sure?  "grep -f pattern_file" is in POSIX.1.

--
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 2/2] pack-objects: Teach it to use reachability bitmap index when generating non-stdout pack too

[Kirill Smelkov]

On Mon, Aug 08, 2016 at 11:28:02AM -0700, Junio C Hamano wrote:
> Kirill Smelkov  writes:
> 
> > Another question: I'm preparing another version of "pack-objects: Teach
> > --use-bitmap-index codepath to  respect --local ..." and was going to
> > put
> >
> > ( updated patch is in the end of this mail )
> >
> > in the top of the message. Is it ok or better not to do so and just respin
> > the patch in its own separate mail?
> 
> That would force those who pick leftover bits to _open_ and read a
> first few lines.
> 
> Definitely it is better than burying a patch after 60+ lines, but a
> separate patch with incremented "[PATCH v6 1/2]" on the subject line
> beats it hands-down from discoverability's point of view.

Thanks, I see. I've resent both patches as separate mails.
--
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] pack-objects: Teach --use-bitmap-index codepath to respect --local, --honor-pack-keep and --incremental

[Kirill Smelkov]

Since 6b8fda2d (pack-objects: use bitmaps when packing objects) there
are two codepaths in pack-objects: with & without using bitmap
reachability index.

However add_object_entry_from_bitmap(), despite its non-bitmapped
counterpart add_object_entry(), in no way does check for whether --local
or --honor-pack-keep or --incremental should be respected. In
non-bitmapped codepath this is handled in want_object_in_pack(), but
bitmapped codepath has simply no such checking at all.

The bitmapped codepath however was allowing to pass in all those options
and with bitmap indices still being used under such conditions -
potentially giving wrong output (e.g. including objects from non-local or
.keep'ed pack).

We can easily fix this by noting the following: when an object comes to
add_object_entry_from_bitmap() it can come for two reasons:

1. entries coming from main pack covered by bitmap index, and
2. object coming from, possibly alternate, loose or other packs.

"2" can be already handled by want_object_in_pack() and to cover
"1" we can teach want_object_in_pack() to expect that *found_pack can be
non-NULL, meaning calling client already found object's pack entry.

In want_object_in_pack() we care to start the checks from already found
pack, if we have one, this way determining the answer right away
in case neither --local nor --honour-pack-keep are active. In
particular, as p5310-pack-bitmaps.sh shows, we do not do harm to
served-with-bitmap clones performance-wise:

Test  56dfeb62  this tree
-
5310.2: repack to disk9.63(8.67+0.33)   9.47(8.55+0.28) -1.7%
5310.3: simulated clone   2.07(2.17+0.12)   2.03(2.14+0.12) -1.9%
5310.4: simulated fetch   0.78(1.03+0.02)   0.76(1.00+0.03) -2.6%
5310.6: partial bitmap1.97(2.43+0.15)   1.92(2.36+0.14) -2.5%

with all differences strangely showing we are a bit faster now, but
probably all being within noise.

And in the general case we care not to have duplicate
find_pack_entry_one(*found_pack) calls. Worst what can happen is we can
call want_found_object(*found_pack) -- newly introduced helper for
checking whether we want object -- twice, but since want_found_object()
is very lightweight it does not make any difference.

I appreciate help and discussing this change with Junio C Hamano and
Jeff King.

Signed-off-by: Kirill Smelkov 
---
 builtin/pack-objects.c  |  94 ++--
 t/t5310-pack-bitmaps.sh | 111 
 2 files changed, 172 insertions(+), 33 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index c4c2a3c..e06c1bf 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -944,13 +944,45 @@ static int have_duplicate_entry(const unsigned char *sha1,
return 1;
 }
 
+static int want_found_object(int exclude, struct packed_git *p)
+{
+   if (exclude)
+   return 1;
+   if (incremental)
+   return 0;
+
+   /*
+* When asked to do --local (do not include an
+* object that appears in a pack we borrow
+* from elsewhere) or --honor-pack-keep (do not
+* include an object that appears in a pack marked
+* with .keep), we need to make sure no copy of this
+* object come from in _any_ pack that causes us to
+* omit it, and need to complete this loop.  When
+* neither option is in effect, we know the object
+* we just found is going to be packed, so break
+* out of the search loop now.
+*/
+   if (!ignore_packed_keep &&
+   (!local || !have_non_local_packs))
+   return 1;
+
+   if (local && !p->pack_local)
+   return 0;
+   if (ignore_packed_keep && p->pack_local && p->pack_keep)
+   return 0;
+
+   /* we don't know yet; keep looking for more packs */
+   return -1;
+}
+
 /*
  * Check whether we want the object in the pack (e.g., we do not want
  * objects found in non-local stores if the "--local" option was used).
  *
- * As a side effect of this check, we will find the packed version of this
- * object, if any. We therefore pass out the pack information to avoid having
- * to look it up again later.
+ * As a side effect of this check, if object's pack entry was not already 
found,
+ * we will find the packed version of this object, if any. We therefore pass
+ * out the pack information to avoid having to look it up again later.
  */
 static int want_object_in_pack(const unsigned char *sha1,
   int exclude,
@@ -958,15 +990,30 @@ static int want_object_in_pack(const unsigned char *sha1,
   off_t *found_offset)
 {
struct packed_git *p;
+   int want;
 
if (!exclude && local && has_loose_object_nonlocal(sha1))
return 0;
 
-   *found_pack = NULL;

[PATCH v5] pack-objects: teach it to use reachability bitmap index when generating non-stdout pack too

[Kirill Smelkov]

Starting from 6b8fda2d (pack-objects: use bitmaps when packing objects)
if a repository has bitmap index, pack-objects can nicely speedup
"Counting objects" graph traversal phase. That however was done only for
case when resultant pack is sent to stdout, not written into a file.

The reason here is for on-disk repack by default we want:

- to produce good pack (with bitmap index not-yet-packed objects are
  emitted to pack in suboptimal order).

- to use more robust pack-generation codepath (avoiding possible
  bugs in bitmap code and possible bitmap index corruption).

Jeff King further explains:

The reason for this split is that pack-objects tries to determine how
"careful" it should be based on whether we are packing to disk or to
stdout. Packing to disk implies "git repack", and that we will likely
delete the old packs after finishing. We want to be more careful (so
as not to carry forward a corruption, and to generate a more optimal
pack), and we presumably run less frequently and can afford extra CPU.
Whereas packing to stdout implies serving a remote via "git fetch" or
"git push". This happens more frequently (e.g., a server handling many
fetching clients), and we assume the receiving end takes more
responsibility for verifying the data.

But this isn't always the case. One might want to generate on-disk
packfiles for a specialized object transfer. Just using "--stdout" and
writing to a file is not optimal, as it will not generate the matching
pack index.

So it would be useful to have some way of overriding this heuristic:
to tell pack-objects that even though it should generate on-disk
files, it is still OK to use the reachability bitmaps to do the
traversal.

So we can teach pack-objects to use bitmap index for initial object
counting phase when generating resultant pack file too:

- if we care it is not activated under git-repack:

  See above about repack robustness and not forward-carrying corruption.

- if we know bitmap index generation is not enabled for resultant pack:

  Current code has singleton bitmap_git so cannot work simultaneously
  with two bitmap indices.

  We also want to avoid (at least with current implementation)
  generating bitmaps off of bitmaps. The reason here is: when generating
  a pack, not-yet-packed objects will be emitted into pack in
  suboptimal order and added to tail of the bitmap as "extended entries".
  When the resultant pack + some new objects in associated repository
  are in turn used to generate another pack with bitmap, the situation
  repeats: new objects are again not emitted optimally and just added to
  bitmap tail - not in recency order.

  So the pack badness can grow over time when at each step we have
  bitmapped pack + some other objects. That's why we want to avoid
  generating bitmaps off of bitmaps, not to let pack badness grow.

- if we keep pack reuse enabled still only for "send-to-stdout" case:

  Because on pack reuse raw entries are directly written out to destination
  pack by write_reused_pack() bypassing needed for pack index generation
  bookkeeping done by regular codepath in write_one() and friends.

This way for pack-objects -> file we get nice speedup:

erp5.git[1] (~230MB) extracted from ~ 5GB lab.nexedi.com backup
repository managed by git-backup[2] via

time echo 0186ac99 | git pack-objects --revs erp5pack

before:  37.2s
after:   26.2s

And for `git repack -adb` packed git.git

time echo 5c589a73 | git pack-objects --revs gitpack

before:   7.1s
after:3.6s

i.e. it can be 30% - 50% speedup for pack extraction.

git-backup extracts many packs on repositories restoration. That was my
initial motivation for the patch.

[1] https://lab.nexedi.com/nexedi/erp5
[2] https://lab.nexedi.com/kirr/git-backup

NOTE

Jeff also suggests that pack.useBitmaps was probably a mistake to
introduce originally. This way we are not adding another config point,
but instead just always default to-file pack-objects not to use bitmap
index: Tools which need to generate on-disk packs with using bitmap, can
pass --use-bitmap-index explicitly. And git-repack does never pass
--use-bitmap-index, so this way we can be sure regular on-disk repacking
remains robust.

NOTE2

`git pack-objects --stdout >file.pack` + `git index-pack file.pack` is much 
slower
than `git pack-objects file.pack`. Extracting erp5.git pack from
lab.nexedi.com backup repository:

$ time echo 0186ac99 | git pack-objects --stdout --revs 
>erp5pack-stdout.pack

real0m22.309s
user0m21.148s
sys 0m0.932s

$ time git index-pack erp5pack-stdout.pack

real0m50.873s   <-- more than 2 times slower than time to generate pack 
itself!
user0m49.300s
sys 0m1.360s

So the time for

`pack-object --stdout >file.pack` + `index-pack file.pack`  is  72s,

while

`pack-objects file.pack` which does both pack and index is  27s.

And even

Re: Forward declaration of enum iterator_selection?

[Ramsay Jones]

On 08/08/16 19:28, Ramsay Jones wrote:
> 
> 
> On 08/08/16 17:30, Johannes Sixt wrote:
>> Am 07.08.2016 um 22:34 schrieb Ramsay Jones:
>>> On 05/08/16 23:26, Johannes Sixt wrote:
> [snip]
>>> At this point 'enum iterator_selection' is an incomplete type and may
>>> be used when the size of the object is not required. It is not needed,
>>> for example, when a typedef name is being declared as a pointer to, or
>>> as a function returning such a type. However, such a type must be
>>> complete before such a function is called or defined.
>>
>> All you say is true when it is a struct type, of course. But I doubt that 
>> there exists such a thing called "incomplete enumeration type" in C. In 
>> fact, with these keywords I found 
>> https://gcc.gnu.org/onlinedocs/gcc/Incomplete-Enums.html which indicates 
>> that this is a GCC extension.
> 
> Ah, well spotted!
> 
> You prompted me to look at the C99 (and C11) standards, in particular
> sections 6.7.2.2 (Enumeration specifiers) and 6.7.2.3 (Tags).
> 
> So, while (technically) enumeration types are incomplete prior to the
> closing } in its definition, the constraint imposed in 6.7.2.3-2 states:
> 
>   "A type specifier of the form
>   enum identifier
>   without an enumerator list shall only appear after
>   the type it specifies is complete"
> 
> which pretty much rules out its use here.

BTW, you can make gcc be that 'pickier compiler' you mentioned, thus:

$ rm refs.o
$ make CFLAGS='-g -O2 -Wall -std=c99 -pedantic' refs.o
* new build flags
CC refs.o
In file included from refs.c:8:0:
refs/refs-internal.h:363:14: warning: ISO C forbids forward references to 
‘enum’ types [-Wpedantic]
 typedef enum iterator_selection ref_iterator_select_fn(
  ^
$ 

:-D

ATB,
Ramsay Jones

--
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: Forward declaration of enum iterator_selection?

[Ramsay Jones]

On 08/08/16 17:30, Johannes Sixt wrote:
> Am 07.08.2016 um 22:34 schrieb Ramsay Jones:
>> On 05/08/16 23:26, Johannes Sixt wrote:
[snip]
>> At this point 'enum iterator_selection' is an incomplete type and may
>> be used when the size of the object is not required. It is not needed,
>> for example, when a typedef name is being declared as a pointer to, or
>> as a function returning such a type. However, such a type must be
>> complete before such a function is called or defined.
> 
> All you say is true when it is a struct type, of course. But I doubt that 
> there exists such a thing called "incomplete enumeration type" in C. In fact, 
> with these keywords I found 
> https://gcc.gnu.org/onlinedocs/gcc/Incomplete-Enums.html which indicates that 
> this is a GCC extension.

Ah, well spotted!

You prompted me to look at the C99 (and C11) standards, in particular
sections 6.7.2.2 (Enumeration specifiers) and 6.7.2.3 (Tags).

So, while (technically) enumeration types are incomplete prior to the
closing } in its definition, the constraint imposed in 6.7.2.3-2 states:

"A type specifier of the form
enum identifier
without an enumerator list shall only appear after
the type it specifies is complete"

which pretty much rules out its use here.

ATB,
Ramsay Jones

--
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 2/2] pack-objects: Teach it to use reachability bitmap index when generating non-stdout pack too

[Junio C Hamano]

Kirill Smelkov  writes:

> Another question: I'm preparing another version of "pack-objects: Teach
> --use-bitmap-index codepath to  respect --local ..." and was going to
> put
>
> ( updated patch is in the end of this mail )
>
> in the top of the message. Is it ok or better not to do so and just respin
> the patch in its own separate mail?

That would force those who pick leftover bits to _open_ and read a
first few lines.

Definitely it is better than burying a patch after 60+ lines, but a
separate patch with incremented "[PATCH v6 1/2]" on the subject line
beats it hands-down from discoverability's point of view.
--
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: t0027 racy?

[Torsten Bögershausen]

On 2016-08-08 17.05, Johannes Schindelin wrote:
> Hi Torsten,
> 
> I remember that you did a ton of work on t0027. Now I see problems, and
> not only that the entire script now takes a whopping 4 minutes 20 seconds
> to run on my high-end Windows machine.
> 
> It appears that t0027 fails randomly for me, in seemingly random places.
> Sometimes all 1388 cases pass. Sometimes "29 - commit NNO files crlf=true
> attr=auto LF" fails. Sometimes it is "24 - commit NNO files crlf=false
> attr=auto LF". Sometimes it is "114 - commit NNO files crlf=false
> attr=auto LF", and sometimes "111 - commit NNO files attr=auto aeol=lf
> crlf=false CRLF_mix_LF".
> 
> When I run it with -i -v -x --tee, it passes every single time (taking
> over 5 minutes, just to make things worse)...
> 
> Any idea about any possible races?
Just to double-check: I assume that you have this
commit ded2444ad8ab8128cae2b91b8efa57ea2dd8c7a5
Author: Torsten Bögershausen 
Date:   Mon Apr 25 18:56:27 2016 +0200

t0027: make commit_chk_wrnNNO() reliable
in your tree ?


Is there a special pattern ?
Did you
a) Update the machine ?
b) Update Git code base ?
or both ?
(Yes, I know that this may be stupid questions)

Is it only the NNO tests that fail ?
Did they ever pass ?
(I think I run them some time ago on a Virtual machine running Windows 7)

I see only "commit NNO files" in you report, they belong to
check_warning(), which is called around line 126 in t0027.


check_warning() does a grep on a file which has been re-directed from stderr.

How reproducible is the problem ?
If you add
exit 0
After the last "commit_chk_wrnNNO" line (line 418),
does
ls -l crlf*.err
give you any hint ?




--
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/2] pack-objects: Teach --use-bitmap-index codepath to respect --local, --honor-pack-keep and --incremental

[Kirill Smelkov]

( updated patch is in the end of this mail )

Jeff, first of all thanks for commenting,

On Mon, Aug 08, 2016 at 09:50:20AM -0400, Jeff King wrote:
> On Mon, Aug 08, 2016 at 03:37:35PM +0300, Kirill Smelkov wrote:
> 
> > @@ -958,15 +958,30 @@ static int want_object_in_pack(const unsigned char 
> > *sha1,
> >off_t *found_offset)
> >  {
> > struct packed_git *p;
> > +   struct packed_git *pack1 = *found_pack;
> > +   int pack1_seen = !pack1;
> >  
> > if (!exclude && local && has_loose_object_nonlocal(sha1))
> > return 0;
> >  
> > -   *found_pack = NULL;
> > -   *found_offset = 0;
> > +   /*
> > +* If we already know the pack object lives in, start checks from that
> > +* pack - in the usual case when neither --local was given nor .keep 
> > files
> > +* are present the loop will degenerate to have only 1 iteration.
> > +*/
> > +   for (p = (pack1 ? pack1 : packed_git); p;
> > +p = (pack1_seen ? p->next : packed_git), pack1_seen = 1) {
> > +   off_t offset;
> 
> Hmm. So this is basically sticking the found-pack at the front of the
> loop.
> 
> We either need to look at zero packs here (we already know where the
> object is, and we don't need to bother with --local or .keep lookups),
> or we need to look at all of them (to check for local/keep).
> 
> I guess you structured it this way to try to reuse the "can we break out
> early" logic from the middle of the loop. So we go through the loop one
> time, and then break out. And then this:
> 
> > +   if (p == pack1) {
> > +   if (pack1_seen)
> > +   continue;
> > +   offset = *found_offset;
> > +   }
> > +   else {
> > +   offset = find_pack_entry_one(sha1, p);
> > +   }
> 
> is meant to make that one-time through the loop cheaper. So I don't
> think it's wrong, but it's very confusing to me.
> 
> Would it be simpler to stick that logic in a function like:
> 
>   static int want_found_object(int exclude, struct packed_git *pack)
>   {
>   if (exclude)
>   return 1;
>   if (incremental)
>   return 0;
> 
>   /* if we can break early, then do so */
>   if (!ignore_packed_keep &&
>   (!local || !have_non_local_packs))
>   return 1;
> 
>   if (local && !p->pack_local)
>   return 0;
>   if (ignore_packed_keep && p->pack_local && p->pack_keep)
>   return 0;
> 
>   /* indeterminate; keep looking for more packs */
>   return -1;
>   }
> 
>   static int want_object_in_pack(...)
>   {
>   ...
>   if (!exclude && local && has_loose_object_nonlocal(sha1))
>   return 0;
> 
>   if (*found_pack) {
>   int ret = want_found_object(exclude, *found_pack);
>   if (ret != -1)
>   return ret;
>   }
> 
>   for (p = packed_git; p; p = p->next) {
>   off_t offset;
> 
>   if (p == *found_pack)
>   offset = *found_offset;
>   else
>   offset = find_pack_entry(sha1, p);
>   if (offset) {
>   ... fill in *found_pack ...
>   int ret = want_found_object(exclude, p);
>   if (ret != -1)
>   return ret;
>   }
>   }
>   return 1;
>   }
> 
> That's a little more verbose, but IMHO the flow is a lot easier to
> follow (especially as the later re-rolls of that series actually muck
> with the loop order more, but with this approach there's no conflict).

On Mon, Aug 08, 2016 at 09:08:51AM -0700, Junio C Hamano wrote:
> I agree; Kirill's version was so confusing that I couldn't see what
> it was trying to do with "pack1_seen" flag that is reset every time
> loop repeats (at least, before got my coffee ;-).  A helper function
> like the above makes the logic a lot easier to grasp.

Ok, at least I put today's record for the most confusing code. I agree
with your comments - it is better to simplify control-flow logic. Somehow
my head was refusing doing that and insisted on keeping the loop inside
intact. Maybe I should have a bit of rest... Scratch all that in favour
of want_found_object() and thanks for heads-up.



> >  static int add_object_entry(const unsigned char *sha1, enum object_type 
> > type,
> > const char *name, int exclude)
> >  {
> > -   struct packed_git *found_pack;
> > -   off_t found_offset;
> > +   struct packed_git *found_pack = NULL;
> > +   off_t found_offset = 0;
> > uint32_t index_pos;
> >  
> > if (have_duplicate_entry(sha1, exclude, _pos))
> > @@ -1073,6 +1088,9 @@ static int add_object_entry_from_bitmap(const 
> > unsigned char *sha1,
> > if (have_duplicate_entry(sha1, 0, _pos))
> > return 0;
> >  
> > +   if (!want_object_in_pack(sha1, 0, , ))
> > +   return 0;
> > +
> 
> 

Re: [PATCHv2 0/6] git clone: Marry --recursive and --reference

[Stefan Beller]

On Sat, Aug 6, 2016 at 10:29 AM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>>  Some submodules in the referenced superproject may not be there,
>>  (they are just not initialized/cloned/checked out), which yields
>>  an error for now.
>
> Perhaps you can teach "git clone --reference" an new option
> (--reference-if-able) to do this?  Then
>
> When `--reference` is given together with `--recursive`,
> the reference repository is assumed to contain the submodules
> as well and the submodules are setup as alternates of the
> submodules in the given reference project.
>
> in which "assumed" is a horrible wording (leave the reader
> wondering: "so what happens to my data when the assumption does not
> hold") can become a lot more reasonable
>
> When using --reference with --recursive, the --reference is used
> to specify a repository that has a copy of the superproject.  If
> that copy has submodules cloned for itself in its $GIT_DIR/modules,
> they are used as --reference when cloning submodules in the
> resulting clone.
>
> and readers expectation would match with the reality.  Their
> submodules would be cloned in a regular fashion if the central
> mirror does not have it, and would take advantage of it if there is
> already a clone.

That makes sense.

>
> Come to think of it, do we even need --super-reference?  "git clone
> --reference --recursive" is a two step process, in that first the
> superproject is cloned while creating objects/info/alternates, and
> then submodules are cloned (via "update --init").  Can we make the
> procedure to clone a submodule always look at the reference of the
> superproject (i.e. objects/info/alternates) and try to borrow from
> the place in it that corresponds to the submodule?  That way, not
> just "git clone --reference --recursive" would take advantage of the
> existing mirrors of submodules, a user who does this:
>
> $ git clone --reference $URL super
> $ cd super
> $ git submodule update --init ...
>
> would be able to take advantage of the "what the mirror the
> superproject uses already has" when cloning the submodules, no?

That would work, too.

I'll implement that.

Thanks,
Stefan
--
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 2/2] pack-objects: Teach it to use reachability bitmap index when generating non-stdout pack too

[Kirill Smelkov]

On Mon, Aug 08, 2016 at 11:08:34AM -0700, Junio C Hamano wrote:
> Kirill Smelkov  writes:
> 
> > Thanks for the info. I did not knew about show-index when I was starting
> > to work on this and later it just came out of sight. Please find
> > corrected patch below.
> >
> >  8< 
> > From: Kirill Smelkov 
> > Date: Fri, 29 Jul 2016 10:47:46 +0300
> > Subject: [PATCH v5] pack-objects: Teach it to use reachability bitmap index 
> > when
> >  generating non-stdout pack too
> 
> Please don't do this (not the patch text itself, but saying "Please
> find ..." and attaching the patch AFTER 60+ lines of response).
> When going through old/read messages to see if there are patches
> that fell through the cracks, if it is not immediately clear in the
> top part of the message that it contains an updated patch, such a
> patch will certainly be missed.
> 
> Please say "I'll follow up with a corrected patch" instead of
> "Please find ..." and respond to that message with just the patch.

Ok, I see. Should I resend this v5 as separated one or only starting
from next time?

Another question: I'm preparing another version of "pack-objects: Teach
--use-bitmap-index codepath to  respect --local ..." and was going to
put

( updated patch is in the end of this mail )

in the top of the message. Is it ok or better not to do so and just respin
the patch in its own separate mail?

Thanks beforehand for clarifying,
Kirill

P.S. I put updated patches in the same mail not because I'm trying to
make maintainer's life harder, but because this is the way I would
expect and prefer them to be coming to me...
--
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 2/2] pack-objects: Teach it to use reachability bitmap index when generating non-stdout pack too

[Junio C Hamano]

Kirill Smelkov  writes:

> Thanks for the info. I did not knew about show-index when I was starting
> to work on this and later it just came out of sight. Please find
> corrected patch below.
>
>  8< 
> From: Kirill Smelkov 
> Date: Fri, 29 Jul 2016 10:47:46 +0300
> Subject: [PATCH v5] pack-objects: Teach it to use reachability bitmap index 
> when
>  generating non-stdout pack too

Please don't do this (not the patch text itself, but saying "Please
find ..." and attaching the patch AFTER 60+ lines of response).
When going through old/read messages to see if there are patches
that fell through the cracks, if it is not immediately clear in the
top part of the message that it contains an updated patch, such a
patch will certainly be missed.

Please say "I'll follow up with a corrected patch" instead of
"Please find ..." and respond to that message with just the patch.
--
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 1/2] format-patch: Add a config option format.from to set the default for --from

[Junio C Hamano]

Josh Triplett  writes:

> I didn't realize you had already taken the patch series into next; I'd
> assumed from the various comments that you expected me to reroll it
> before you'd take it.
>
> Would you like me to write something up for the release notes regarding
> plans to change the default?

Given that we are at week #8 and -rc0 is coming soon, I suspect that
that note will happen not in this release but in the next one.

The patch in question (1/2) is merely a new convenience feature that
does not have to say anything about the future default, so we are
good with 1/2 as-is (not v2 version of it, but the original one
without enum), I think.




--
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] merge: use string_list_split() in add_strategies()

[Junio C Hamano]

Johannes Schindelin  writes:

> I wonder, however, if we could somhow turn things around by introducing
> something like
>
>   split_and_do_for_each(item_p, length, string, delimiter)
>   ...  ...
>
> that both string_list_split() *and* add_strategies() could use? We would
> then be able to avoid allocating the list and duplicating the items in the
> latter case.

I do think such a feature may be useful if we often work on pieces
of a string delimited by a delimiter, but if the caller does not see
the split result, then the function with "split" is probably
misnamed.

I however suspect the variant of this where "delimiter" can just be
a single byte would not be so useful.

If the input comes from the end user, we certainly would want to
allow "word1 word2\tword3 " as input (i.e. squishing repeated
delimiters into one without introducing an "empty" element, allowing
more than one delimiter characters like SP and HT, and ignoring
leading or trailing runs of delimiter characters).

If the input is generated internally, perhaps we should rethink the
interface between the function that wants to do the for-each-word
and its caller; if the caller wants to pass multiple things to the
callee, it should be able to do so without first having to stuff
these multiple things into a single string, only to force the callee
to use this helper to split them out into individual pieces.

--
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: storing cover letter of a patch series?

[Junio C Hamano]

Duy Nguyen  writes:

> git-notes was mentioned in this thread back in 2015, but I think it's
> discarded because of the argument that's part of the cover letter was
> not meant to be kept permanently.

I do not think the reason why we didn't think the notes mechanism
was a good match was mainly because the cover letter material was
about a branch as a whole, which does not have a good counter-part
in Git at the conceptual level.  "A branch is just a moving pointer
that points at one commit that happens to be at the tip" is not a
perfect match to "I am holding these N patches to achieve X and I am
constantly adding, rewinding and rebuilding".  The notes mechanism
gives an easy way to describe the former (i.e. annotate one commit,
and let various commands to move that notes as you rewind and
rebuild) but not the latter (i.e. "branch.description" configuration
is the best match, but that is just a check-box feature and does not
make any serious attempt to be part of a version-control system).

> But I think we can still use it as a
> local/temporary place for cover letter instead of the empty commit at
> the topic's tip. It is a mark of the beginning of commit, it does not
> require rewriting history when you update the cover letter, and
> git-merge can be taught to pick it up when you're ready to set it in
> stone.

That depends on what you exactly mean by "the beginning of".  Do you
mean the first commit that is on the topic?  Then that still requires
you to move it around when the topic is rebuilt.  If you mean the
commit on the mainline the topic forks from, then of course that
would not work, as you can fork multiple topics at the same commit.


--
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 submission process, was Re: [PATCH v6 06/16] merge_recursive: abort properly upon errors

[Junio C Hamano]

Lars Schneider  writes:

> ... then I am always super
> eager to send out a new roll just because I don't want any other reviewer
> to waste time on obviously wrong patches. However, I have the impression
> that frequent re-rolls are frowned upon.

Correct.  A good middle-ground is to just reply with "Yes, thanks
for your suggestion, will fix in the next round", while receiving
review comments.  Good reviewers who value their time will not to
waste their time by responding on a point that has already been
pointed out and acknowledged.

> 4.) Reviewing patches is super hard for me because my email client does not
> support patch color highlighting and I can't easily expand context or 
> look at
> the history of code touched by the patch (e.g via git blame). I tried to 
> setup
> Alpine but I wasn't happy with the interface either. I like patches with 
> a GitHub
> URL for review but then I need to find the right line in the original 
> email to
> write a comment.

Unless a patch is about an area you are super familiar with so that
you know what is beyond the context of the patch to be able to judge
if the change is good in the context of the file being touched, it
is always hard to review from inside a mail reader.

Running "git am" is a good first step to review such a patch, as
that lets you view the resulting code with the full power of Git.
As you gain experience on the codebase, you'll be able to spot more
problems while in your mail reader.
--
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: storing cover letter of a patch series?

[Stefan Beller]

On Thu, Sep 10, 2015 at 9:28 AM, Jacob Keller  wrote:
> Hey,
>
> does anyone know of any tricks for storing a cover letter for a patch
> series inside of git somehow? I'd guess the only obvious way currently
> is to store it at the top of the series as an empty commit.. but this
> doesn't get emailed *as* the cover letter...
>
> Is there some other way? Would others be interested in such a feature?

Being late to this thread, but I think

   branch..description
   Branch description, can be edited with git branch
   --edit-description. Branch description is automatically added in
   the format-patch cover letter or request-pull summary.

is what you want. Maybe we want to see a patch that adds the reverse
functionality as well, i.e. git-am will store the the cover letter as the
branch description and git-merge will propose the branch description for
the merge commit.

>
> I get very annoyed when I've written a nice long patch cover letter in
> vim before an email and then realize I should fix something else up,
> or accidentally cancel it because I didn't use the write "To:" address
> or something..
>
> I really think it should be possible to store something somehow as a
> blob that could be looked up later. Even if this was a slightly more
> manual process that would be helpful to store the message inside git
> itself.

I agree here. I personally do not use that variable (yet), as it doesn't seem
to be editable easily.
So here is what I do:
1) First series is generated with format-patch --cover-letter
2) any following v{2,3,4} is generated without the cover-letter but with
  --subject-prefix=PATCHv{2,3,4}
3) the cover letter is manually edited to be the next version and a section
  is added why the next version of the series is sent.

>
> In addition, this would help re-rolls since it would mean if I go back
> to a topic and re-roll it I can just update the message. If it were
> properly stored in my local history that would also mean I could see
> revisions on it.
>
> Any thoughts on how to do this?
>
> Regards,
> Jake
> --
> 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
--
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 submission process, was Re: [PATCH v6 06/16] merge_recursive: abort properly upon errors

[Junio C Hamano]

Johannes Schindelin  writes:

> I think you both got it wrong. The original citizens were the mail
> clients that allowed you to communicate with other human beings.
> ... It is our usage to transport machine-readable content (and not
> as an attachment!) that is the intruder.

It is not "its is our usage".

You are too young to remember or too old to remember the history, or
you are knowingly distorting it.  The original users of "patch" and
"diff" expected that e-mail to be a medium to safely exchange
changes to programs among themselves.
--
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 7/7] pack-objects: use mru list when iterating over packs

[Junio C Hamano]

Jeff King  writes:

>> It worries me a lot to lose the warning unconditionally, though.
>> That's the (only) coal-mine canary that lets us notice a problem
>> when we actually start hitting that last-ditch cycle breaking too
>> often.
>
> The dedicated cycle-detection will lose that warning, too (it doesn't
> touch that code, but it's effectively checking the same thing earlier).
>
> I agree it's unfortunate to lose. On the other hand, it is being lost
> because we are correctly handling the cycles, and there is nothing to
> warn about. So it ceases to be a problem, and starts being a normal,
> acceptable thing.

That unfortunately is beside the point.  The existing cycle breaking
was meant to be a last ditch effort to avoid producing a broken pack
(i.e. a suboptimal pack without cycle is better than unusable pack
with delta cycle), while letting us know that we found a case where
the remainder of the pack building machinery does not function well
without it (so that we know we broke something when we tweaked the
machinery without intending to break it).  Squelching the warnings
feels similar to "we see too many valgrind warnings, so let's stop
running valgrind"; I was hoping there would be a solution more like
"instead of not running, let's teach valgrind this and that codepath
is OK".
--
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 v5 9/9] status: unit tests for --porcelain=v2

[Junio C Hamano]

Jeff Hostetler  writes:

> +test_expect_success pre_initial_commit_0 '
> + ...
> + git status --porcelain=v2 --branch --untracked-files=normal >actual &&
> + test_cmp expect actual
> +'
> +
> +
> +test_expect_success pre_initial_commit_1 '
> + git add file_x file_y file_z dir1 &&
> + ...
> + cat >expect <<-EOF &&
> + # branch.oid (initial)
> + # branch.head master
> + 1 A. N... 00 100644 100644 $_z40 $OID_A dir1/file_a
> + ...

This makes one wonder what would/should be shown on the "A." column
if one of these files are added with "-N" (intent-to-add).  I do not
see any test for such entries in this patch, but I think it should.

> + git status --porcelain=v2 --branch --untracked-files=all >actual &&
> + test_cmp expect actual
> +'

> +## Try -z on the above
> +test_expect_success pre_initial_commit_2 '
> + lf_to_nul >expect <<-EOF &&
> + ...
> + git status -z --porcelain=v2 --branch --untracked-files=all >actual &&
> + test_cmp expect actual
> +'

"Try -z on the above" can and should be in the test title to help
those who are watching the test run.  Instead of trying to clarify
code with comments, clarify them by letting the code speak for
themselves.  I would have named the above three perhaps like this:

test_expect_success 'before first commit, nothing added'
test_expect_success 'before first commit, some files added'
test_expect_success 'before first commit, some files added (-z)'

"pre-initial-commit-$number" is not very interesting; it does not
convey a more interesting aspect of these tests, like (a) _0 is
distinct (nothing added) among the three, (b) _1 and _2 are about
the same state, just expressed differently, and (c) _1 is LF
terminated, _2 is NUL terminated.  The same comment applies to the
remainder of the test.  For example:

> +##
> +## Create second commit.
> +##
> +
> +test_expect_success second_commit_0 '

This "_0" does not tell us anything, but you are testing "When fully
committed, we only see untracked files (if we ask) and branch info
(if we ask).

Having said all that, it is OK to fix their titles after the current
9-patch series lands on 'next'; incremental refinements are easier
on reviewers than having to review too many rerolls.

This is probably a good place to see what happens to these untracked
files and branch info if we do not ask the command to show them.
Otherwise, it tests exactly the same as "initial_commit_0" and is
not all that interesting, no?

> +##
> +## Ignore a file
> +##
> +
> +test_expect_success ignore_file_0 '
> + echo x.ign >.gitignore &&
> + echo "ignore me" >x.ign &&
> + H1=$(git rev-parse HEAD) &&
> +
> + ## ignored file SHOULD NOT appear in output when --ignored is not used.
> + ...
> + git status --porcelain=v2 --branch --untracked-files=all >actual &&
> + test_cmp expect actual &&
> + ...
> + git status --porcelain=v2 --branch --ignored --untracked-files=all 
> >actual &&
> + rm x.ign &&
> + rm .gitignore &&

Arrange these files to be cleaned before you create them by having

test_when_finished "rm -f x.ign .gitignore" &&

at the very beginning of this test before they are created.
Otherwise, if any step before these removal fail, later test that
assume they are gone will be affected.  You already do so correctly
in the upstream_fields_0 test below.

> +##
> +## Create some conflicts.
> +##
> +
> +test_expect_success conflict_AA '
> + git branch AA_A master &&
> + git checkout AA_A &&
> + echo "Branch AA_A" >conflict.txt &&
> + OID_AA_A=$(git hash-object -t blob -- conflict.txt) &&
> + git add conflict.txt &&
> + git commit -m "branch aa_a" &&
> +
> + git branch AA_B master &&
> + git checkout AA_B &&
> + echo "Branch AA_B" >conflict.txt &&
> + OID_AA_B=$(git hash-object -t blob -- conflict.txt) &&
> + git add conflict.txt &&
> + git commit -m "branch aa_b" &&
> +
> + git branch AA_M AA_B &&
> + git checkout AA_M &&
> + test_must_fail git merge AA_A &&
> +
> + HM=$(git rev-parse HEAD) &&
> +
> + cat >expect <<-EOF &&
> + # branch.oid $HM
> + # branch.head AA_M
> + u AA N... 00 100644 100644 100644 $_z40 $OID_AA_B $OID_AA_A 
> conflict.txt
> + EOF

This is a small point, but doesn't the lowercase 'u' somehow look
ugly, especially because the status letters that immediately follow
it are all uppercase?

> + git status --porcelain=v2 --branch --untracked-files=all >actual &&
> + git reset --hard &&

This "reset" also may be 

Re: [PATCH v2 7/7] pack-objects: use mru list when iterating over packs

[Jeff King]

On Mon, Aug 08, 2016 at 09:28:29AM -0700, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > Here's a list of approaches I think we can use to fix this:
> >
> >   1. squelch the warning and ignore it. The downside here, besides not
> >  warning the user about true in-pack cycles, is that we have no
> >  opportunity to actually find a new delta (because we realize the
> >  problem only after the delta-compression phase).
> >
> >  My test repository is a bad packing of all of the forks of
> >  torvalds/linux, with 3600 packs. I'm happy to share it if anybody
> >  wants to see it, but note that it is 11GB.
> >
> >  The space overhead of the resulting pack in this case is ~3.2%
> >  (versus a pack generated by the original code, using the static
> >  pack order).  Which is really not that bad, but I'm sure there are
> >  more pathological cases (i.e., there were on the order of hundreds
> >  or maybe thousands of cycles that needed broken, out of about 15
> >  million total objects; but one could imagine the worst case as
> >  nr_of_objects/2).
> > ...
> >
> > So I dunno. I really like the MRU approach if we can salvage it.
> 
> I think I share the same feeling.  As long as the chance is small
> enough that the pack reordering creates a new cycle, the resulting
> pack would not become too bloated by the last-ditch cycle breaking
> code and finding a replacement delta instead of inflating it may not
> be worth the trouble.

Let me see how complicated it is to do the cycle-detection earlier. It
really shouldn't be that hard (we don't even need an auxiliary data
structure; we can just smudge the index value like write_object does).
I'm very curious to see the pack resulting pack size.

> It worries me a lot to lose the warning unconditionally, though.
> That's the (only) coal-mine canary that lets us notice a problem
> when we actually start hitting that last-ditch cycle breaking too
> often.

The dedicated cycle-detection will lose that warning, too (it doesn't
touch that code, but it's effectively checking the same thing earlier).

I agree it's unfortunate to lose. On the other hand, it is being lost
because we are correctly handling the cycles, and there is nothing to
warn about. So it ceases to be a problem, and starts being a normal,
acceptable thing.

-Peff
--
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 7/7] pack-objects: use mru list when iterating over packs

[Junio C Hamano]

Jeff King  writes:

> Here's a list of approaches I think we can use to fix this:
>
>   1. squelch the warning and ignore it. The downside here, besides not
>  warning the user about true in-pack cycles, is that we have no
>  opportunity to actually find a new delta (because we realize the
>  problem only after the delta-compression phase).
>
>  My test repository is a bad packing of all of the forks of
>  torvalds/linux, with 3600 packs. I'm happy to share it if anybody
>  wants to see it, but note that it is 11GB.
>
>  The space overhead of the resulting pack in this case is ~3.2%
>  (versus a pack generated by the original code, using the static
>  pack order).  Which is really not that bad, but I'm sure there are
>  more pathological cases (i.e., there were on the order of hundreds
>  or maybe thousands of cycles that needed broken, out of about 15
>  million total objects; but one could imagine the worst case as
>  nr_of_objects/2).
> ...
>
> So I dunno. I really like the MRU approach if we can salvage it.

I think I share the same feeling.  As long as the chance is small
enough that the pack reordering creates a new cycle, the resulting
pack would not become too bloated by the last-ditch cycle breaking
code and finding a replacement delta instead of inflating it may not
be worth the trouble.

It worries me a lot to lose the warning unconditionally, though.
That's the (only) coal-mine canary that lets us notice a problem
when we actually start hitting that last-ditch cycle breaking too
often.
--
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: Forward declaration of enum iterator_selection?

[Johannes Sixt]

Am 07.08.2016 um 22:34 schrieb Ramsay Jones:

On 05/08/16 23:26, Johannes Sixt wrote:

When refs.c is being compiled, the only mention of enum
iterator_selection is in this piece of code pulled in from
refs-internal.h(have a look at the preprocessed code):

typedef enum iterator_selection ref_iterator_select_fn(
 struct ref_iterator *iter0, struct ref_iterator *iter1,
 void *cb_data);

This looks like a forward declarations of an enumeration type name,
something that I thought is illegal in C. Am I wrong? (That may well be
the case, my C-foo is quite rusty.)


At this point 'enum iterator_selection' is an incomplete type and may
be used when the size of the object is not required. It is not needed,
for example, when a typedef name is being declared as a pointer to, or
as a function returning such a type. However, such a type must be
complete before such a function is called or defined.


All you say is true when it is a struct type, of course. But I doubt that 
there exists such a thing called "incomplete enumeration type" in C. In 
fact, with these keywords I found 
https://gcc.gnu.org/onlinedocs/gcc/Incomplete-Enums.html which indicates 
that this is a GCC extension.



[...] I would rather the 'enum iterator_selection' be defined
before this declaration. One solution could be to #include "iterator.h"
prior to _all_ #include "refs/refs-internal.h" in all compilation units
(Note it is in the opposite order in refs/iterator.c). Alternatively, you
could put the #include "../iterator.h" into refs/refs-internal.h directly
(some people would object to this).


I concur. Which one is the correct way to do, I do not know, either. It's 
a matter how the interface is intended to be used. Perhaps the typedef 
must be moved to iterator.h?


-- Hannes

--
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 11/12] convert: add filter..process option

[Jeff King]

On Mon, Aug 08, 2016 at 06:21:18PM +0200, Lars Schneider wrote:

> >> Happy answer with no content:
> >> 
> >> packet:  git< status=success\n
> >> 
> > 
> > This can just be spelled:
> > 
> >  git< status=success
> >  git< 
> >  git<    # empty content!
> >  git<    # empty list!
> 
> Is the first flush packet one too many?
> If there is nothing then I think we shouldn't
> send any packets?!
> 
> I agree with the remaining two flush packets.

There isn't nothing, there is a "status" field (though I think that
should probably be required, so I guess you could imagine it as a
stand-alone pkt, separate from the list terminated by the flush). But
regardless, you need the first flush to say "I am done telling you
up-front keys, now I am starting the content".

Otherwise, what would:

  git< status=success
  git< foo=bar
  git< 

be parsed as? Is "foo=bar" the first line of content, or the rest of the
pre-content header? (You could guess if you could see the total
conversation, but you can't; you have to parse it as it comes).

> There is one more thing: I introduced a return value "status=error-all".
> Using this the filter can signal Git that it does not want to process
> any other file using the particular command.
> 
> Jakub came up with this idea here:
> 
> "Another response, which I think should be standarized, or at
> least described in the documentation, is filter driver refusing
> to filter further (e.g. git-LFS and network is down), to be not
> restarted by Git."
> 
> http://public-inbox.org/git/607c07fe-5b6f-fd67-13e1-705020c267ee%40gmail.com/
> 
> I think it is a good idea. Do you see arguments against it?

No, that seems reasonable (I would have just implemented that by hanging
up the connection, but explicitly communicating is more robust).

-Peff
--
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 11/12] convert: add filter..process option

[Lars Schneider]

> On 08 Aug 2016, at 17:02, Jeff King  wrote:
> 
> On Sat, Aug 06, 2016 at 08:19:28PM +0200, Lars Schneider wrote:
> 
>>> I dunno. It's not _that_ big a deal to code around. I was just surprised
>>> not to see an up-front status when responding to a request. It seems
>>> like the normal thing in just about every protocol I've ever used.
>> 
>> Alright. The fact that it "surprised" you is a bad sign. 
>> How about this:
>> 
>> Happy answer:
>> 
>> packet:  git< status=accept\n
>> packet:  git< SMUDGED_CONTENT
>> packet:  git< 
>> packet:  git< status=success\n
>> 
> 
> I notice that the status pkt-lines are by themselves. I had assumed we'd
> be sending other data, too (presumably before, but I guess possibly
> after, too). Something like:
> 
>  git< status=accept
>  git< 
>  git< SMUDGED_CONTENT
>  git< 
>  git< status=success
>  git< 
> 
> I don't have any particular meta-information in mind, but I thought
> stuff like the tentative "size" field would be here.
> 
> I had imagined it at the front, but I guess it could go in either place.
> I wonder if keys at the end could simply replace ones from the beginning
> (so if you say "foo=bar" at the front, that is tentative, but if you
> then say "foo=revised" at the end, that takes precedence).
> 
> And so the happy answer is really:
> 
>  git< status=success
>  git< 
>  git< SMUDGED_CONTENT
>  git< 
>  git<   # empty list!
> 
> i.e., no second status. The original "success" still holds.

OK, that sounds sensible to me.


> And then:
> 
>> Happy answer with no content:
>> 
>> packet:  git< status=success\n
>> 
> 
> This can just be spelled:
> 
>  git< status=success
>  git< 
>  git<    # empty content!
>  git<    # empty list!

Is the first flush packet one too many?
If there is nothing then I think we shouldn't
send any packets?!

I agree with the remaining two flush packets.


>> Rejected content:
>> 
>> packet:  git< status=reject\n
>> 
> 
> I'd assume that an error status would end the output for that file
> immediately, no empty lists necessary (so what you have here). I'd
> probably just call this "error" (see below).

OK!


>> Error during content response:
>> 
>> packet:  git< status=accept\n
>> packet:  git< HALF_WRITTEN_ERRONEOUS_CONTENT
>> packet:  git< 
>> packet:  git< status=error\n
>> 
> 
> And then this would be:
> 
>  git< status=success
>  git< 
>  git< HALF_OF_CONTENT
>  git< 
>  git< status=error
>  git< 
> 
> And then you have only two status codes: success and error. Which keeps
> things simple.
> 
> There's one other case, which is when the filter dies halfway through
> the conversation, like:
> 
>  git< status=success
>  git< 
>  git< CONTENT
>  git< 
>  ... EOF on pipe ...
> 
> Any time git does not get the conversation all the way to the final
> flush after the trailers, it should be considered an error (because we
> can never know if the filter was about to say "whoops, status=error").

Right. I agree with the protocol above and I will implement it
that way.

There is one more thing: I introduced a return value "status=error-all".
Using this the filter can signal Git that it does not want to process
any other file using the particular command.

Jakub came up with this idea here:

"Another response, which I think should be standarized, or at
least described in the documentation, is filter driver refusing
to filter further (e.g. git-LFS and network is down), to be not
restarted by Git."

http://public-inbox.org/git/607c07fe-5b6f-fd67-13e1-705020c267ee%40gmail.com/

I think it is a good idea. Do you see arguments against it?

Thanks,
Lars
--
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/2] pack-objects: Teach --use-bitmap-index codepath to respect --local, --honor-pack-keep and --incremental

[Junio C Hamano]

Kirill Smelkov  writes:

>> > ...
>> > Suggested-by: Junio C Hamano 
>> > Discussed-with: Jeff King 
>> > ---
>> 
>> I do not think I suggested much of this to deserve credit like this,
>> though, as I certainly haven't thought about the pros-and-cons
>> between adding the same "some object in pack may not want to be in
>> the output" logic to the bitmap side, or punting the bitmap codepath
>> when local/keep are involved.
>
> I understand. Still for me it was you who convinced me to add proper
> support for e.g. --local vs bitmap instead of special-casing it.

OK, in such a case, it probably is more sensible to do it like:

...
with all differences strangely showing we are a bit faster now, but
probably all being within noise.

Credit for inspiring this solution and discussing the design of
the change goes to Junio and Jeff King.

Signed-off-by: Kirill Smelkov 
---
 builtin/pack-objects.c  |  36 -
 t/t5310-pack-bitmaps.sh | 103 

 2 files changed, 130 insertions(+), 9 deletions(-)

Don't forget your own sign-off ;-)
--
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/2] pack-objects: Teach --use-bitmap-index codepath to respect --local, --honor-pack-keep and --incremental

[Junio C Hamano]

Jeff King  writes:

> On Mon, Aug 08, 2016 at 03:37:35PM +0300, Kirill Smelkov wrote:
> ...
>   static int want_object_in_pack(...)
>   {
>   ...
>   if (!exclude && local && has_loose_object_nonlocal(sha1))
>   return 0;
>
>   if (*found_pack) {
>   int ret = want_found_object(exclude, *found_pack);
>   if (ret != -1)
>   return ret;
>   }
>
>   for (p = packed_git; p; p = p->next) {
>   off_t offset;
>
>   if (p == *found_pack)
>   offset = *found_offset;
>   else
>   offset = find_pack_entry(sha1, p);
>   if (offset) {
>   ... fill in *found_pack ...
>   int ret = want_found_object(exclude, p);
>   if (ret != -1)
>   return ret;
>   }
>   }
>   return 1;
>   }
>
> That's a little more verbose, but IMHO the flow is a lot easier to
> follow (especially as the later re-rolls of that series actually muck
> with the loop order more, but with this approach there's no conflict).

I agree; Kirill's version was so confusing that I couldn't see what
it was trying to do with "pack1_seen" flag that is reset every time
loop repeats (at least, before got my coffee ;-).  A helper function
like the above makes the logic a lot easier to grasp.

>>  static int add_object_entry(const unsigned char *sha1, enum object_type 
>> type,
>>  const char *name, int exclude)
>>  {
>> -struct packed_git *found_pack;
>> -off_t found_offset;
>> +struct packed_git *found_pack = NULL;
>> +off_t found_offset = 0;
>>  uint32_t index_pos;
>>  
>>  if (have_duplicate_entry(sha1, exclude, _pos))
>> @@ -1073,6 +1088,9 @@ static int add_object_entry_from_bitmap(const unsigned 
>> char *sha1,
>>  if (have_duplicate_entry(sha1, 0, _pos))
>>  return 0;
>>  
>> +if (!want_object_in_pack(sha1, 0, , ))
>> +return 0;
>> +
>
> This part looks correct and easy to understand.

Yes.
--
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 2/2] pack-objects: Teach it to use reachability bitmap index when generating non-stdout pack too

[Kirill Smelkov]

On Mon, Aug 08, 2016 at 09:56:00AM -0400, Jeff King wrote:
> On Fri, Jul 29, 2016 at 10:47:46AM +0300, Kirill Smelkov wrote:
> 
> > @@ -2527,7 +2528,7 @@ static int get_object_list_from_bitmap(struct 
> > rev_info *revs)
> > if (prepare_bitmap_walk(revs) < 0)
> > return -1;
> >  
> > -   if (pack_options_allow_reuse() &&
> > +   if (pack_options_allow_reuse() && pack_to_stdout &&
> > !reuse_partial_packfile_from_bitmap(
> 
> Should pack_to_stdout just be part of pack_options_allow_reuse()?

Yes, makes sense; thanks for catching this.


> > @@ -2812,7 +2813,23 @@ int cmd_pack_objects(int argc, const char **argv, 
> > const char *prefix)
> > if (!rev_list_all || !rev_list_reflog || !rev_list_index)
> > unpack_unreachable_expiration = 0;
> >  
> > -   if (!use_internal_rev_list || !pack_to_stdout || 
> > is_repository_shallow())
> > +   /*
> > +* "soft" reasons not to use bitmaps - for on-disk repack by default we 
> > want
> > +*
> > +* - to produce good pack (with bitmap index not-yet-packed objects are
> > +*   packed in suboptimal order).
> > +*
> > +* - to use more robust pack-generation codepath (avoiding possible
> > +*   bugs in bitmap code and possible bitmap index corruption).
> > +*/
> > +   if (!pack_to_stdout)
> > +   use_bitmap_index_default = 0;
> > +
> > +   if (use_bitmap_index < 0)
> > +   use_bitmap_index = use_bitmap_index_default;
> > +
> > +   /* "hard" reasons not to use bitmaps; these just won't work at all */
> > +   if (!use_internal_rev_list || (!pack_to_stdout && write_bitmap_index) 
> > || is_repository_shallow())
> > use_bitmap_index = 0;
> 
> This all makes sense and looks good.

Thanks.


> > +test_expect_success 'pack-objects to file can use bitmap' '
> > +   # make sure we still have 1 bitmap index from previous tests
> > +   ls .git/objects/pack/ | grep bitmap >output &&
> > +   test_line_count = 1 output &&
> > +   # verify equivalent packs are generated with/without using bitmap index
> > +   packasha1=$(git pack-objects --no-use-bitmap-index --all packa 
> >  > +   packbsha1=$(git pack-objects --use-bitmap-index --all packb  > &&
> > +   git verify-pack -v packa-$packasha1.pack >packa.verify &&
> > +   git verify-pack -v packb-$packbsha1.pack >packb.verify &&
> > +   grep -o "^$_x40" packa.verify |sort >packa.objects &&
> > +   grep -o "^$_x40" packb.verify |sort >packb.objects &&
> > +   test_cmp packa.objects packb.objects
> > +'
> 
> I don't think "grep -o" is portable. However, an easier way to do this
> is probably:
> 
>   # these are already in sorted order
>   git show-index packa.objects &&
>   git show-index packb.objects &&
>   test_cmp packa.objects packb.objects

Thanks for the info. I did not knew about show-index when I was starting
to work on this and later it just came out of sight. Please find
corrected patch below.

 8< 
From: Kirill Smelkov 
Date: Fri, 29 Jul 2016 10:47:46 +0300
Subject: [PATCH v5] pack-objects: Teach it to use reachability bitmap index when
 generating non-stdout pack too

Starting from 6b8fda2d (pack-objects: use bitmaps when packing objects)
if a repository has bitmap index, pack-objects can nicely speedup
"Counting objects" graph traversal phase. That however was done only for
case when resultant pack is sent to stdout, not written into a file.

The reason here is for on-disk repack by default we want:

- to produce good pack (with bitmap index not-yet-packed objects are
  emitted to pack in suboptimal order).

- to use more robust pack-generation codepath (avoiding possible
  bugs in bitmap code and possible bitmap index corruption).

Jeff Kind further explains:

The reason for this split is that pack-objects tries to determine how
"careful" it should be based on whether we are packing to disk or to
stdout. Packing to disk implies "git repack", and that we will likely
delete the old packs after finishing. We want to be more careful (so
as not to carry forward a corruption, and to generate a more optimal
pack), and we presumably run less frequently and can afford extra CPU.
Whereas packing to stdout implies serving a remote via "git fetch" or
"git push". This happens more frequently (e.g., a server handling many
fetching clients), and we assume the receiving end takes more
responsibility for verifying the data.

But this isn't always the case. One might want to generate on-disk
packfiles for a specialized object transfer. Just using "--stdout" and
writing to a file is not optimal, as it will not generate the matching
pack index.

So it would be useful to have some way of overriding this heuristic:
to tell pack-objects that even though it should generate on-disk
files, it is still OK to use the reachability bitmaps to do the
traversal.

So we can teach 

Re: t0027 racy?

[Jeff King]

On Mon, Aug 08, 2016 at 05:05:07PM +0200, Johannes Schindelin wrote:

> I remember that you did a ton of work on t0027. Now I see problems, and
> not only that the entire script now takes a whopping 4 minutes 20 seconds
> to run on my high-end Windows machine.
> 
> It appears that t0027 fails randomly for me, in seemingly random places.
> Sometimes all 1388 cases pass. Sometimes "29 - commit NNO files crlf=true
> attr=auto LF" fails. Sometimes it is "24 - commit NNO files crlf=false
> attr=auto LF". Sometimes it is "114 - commit NNO files crlf=false
> attr=auto LF", and sometimes "111 - commit NNO files attr=auto aeol=lf
> crlf=false CRLF_mix_LF".
> 
> When I run it with -i -v -x --tee, it passes every single time (taking
> over 5 minutes, just to make things worse)...
> 
> Any idea about any possible races?

Try:

  https://github.com/peff/git/blob/meta/stress

which you can run as "sh /path/to/stress t0027" in the top-level of your
git repository. I got failure within about 30 seconds on t0027 (though 5
minutes? Yeesh. It runs in 9s on my laptop. I weep for you).

The verbose output is not very exciting, though:

expecting success: 
check_warning "$lfwarn" ${pfx}_LF.err

--- NNO_attr_auto_aeol_crlf_false_LF.err.expect 2016-08-08 
15:26:37.061701392 +
+++ NNO_attr_auto_aeol_crlf_false_LF.err.actual 2016-08-08 
15:26:37.061701392 +
@@ -1 +0,0 @@
-warning: LF will be replaced by CRLF
not ok 114 - commit NNO files crlf=false attr=auto LF

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