Re: [PATCH v2 7/7] blame: actually use the diff opts parsed from the command line

[Michael Haggerty]

On 08/23/2016 11:56 AM, René Scharfe wrote:
> Am 22.08.2016 um 13:22 schrieb Michael Haggerty:
>> "git blame" already parsed generic diff options from the command line
>> via diff_opt_parse(), but instead of passing the resulting xdl_opts to
>> xdi_diff(), it sent its own xdl_opts, which only reflected the values of
>> the self-parsed options "-w" and "--minimal". Instead, rely on
>> diff_opt_parse() to parse all of the diff options, including "-w" and
>> "--minimal", and pass the resulting xdl_opts to xdi_diff().
> 
> Sounds useful: It allows more fine-grained control over which whitespace
> changes to ignore and which diff algorithm to use.  There is a bit of
> overlap (e.g. with -b meaning show blank boundaries vs. ignore
> whitespace changes), but with your patch blame's own options still take
> precedence, so there should be no unpleasant surprises.
> 
>> Signed-off-by: Michael Haggerty 
>> ---
>> Somebody who knows more about how diff operations are configured
>> should please review this. I'm not certain that the change as
>> implemented won't have other unwanted side-effects, though of course
>> I checked that the test suite runs correctly.
> 
> I don't qualify, but I'll comment anyway..
> 
>>  builtin/blame.c|  11 ++--
>>  t/t4059-diff-indent.sh | 160
>> +
>>  2 files changed, 165 insertions(+), 6 deletions(-)
>>  create mode 100755 t/t4059-diff-indent.sh
> 
> This new test doesn't call git blame.  Does it belong to a different
> commit?  And shouldn't the change to blame.c stand on its own, outside
> of this series?

Thanks for catching this. I squashed the test incorrectly; it was meant
to be part of the previous commit. I'll fix it in v3.

The reason that I would prefer to change `blame` as part of this patch
series is that I think it would be disconcerting for `git diff` and `git
blame` to use different heuristics when computing diffs. It would make
their output inconsistent.

But probably that means passing just the new option to `git blame`
rather than passing all possible `diff` options through.

> [...]

Your other comments may be moot given that changing `git blame` in this
way seems to have been a bad idea in the first place [1]. But I'll keep
them in mind if a new version contains similar code.

Thanks,
Michael

[1]
http://public-inbox.org/git/xmqqtwebwhbg@gitster.mtv.corp.google.com/

Re: [PATCH v2 05/38] refs: create a base class "ref_store" for files_ref_store

[Michael Haggerty]

On 09/04/2016 10:40 PM, David Turner wrote:
> On Sun, 2016-09-04 at 18:08 +0200, Michael Haggerty wrote:
> 
>> +/* A linked list of ref_stores for submodules: */
>> +static struct ref_store *submodule_ref_stores;
> 
> I don't like the per-submodule stores being in a linked list, which
> requires a linear search.  Stefan has, I think, been doing a bunch of
> work to scale git to support on the order of thousands of submodules,
> which this potentially breaks.  What about a hashmap?

I agree it's not ideal, but this linked list is not new. Before this
patch the same code was in `files-backend.c`, and before patch [03/38]
essentially the same linked list was stored as `submodule_ref_caches`.
So this is not a regression, and I'd rather not address it in this patch
series.

CC Stefan in case he'd like to put it on his radar. Honestly, I've never
checked how often the submodule reference cache is used in real life.

Michael

[PATCHv3] diff.c: emit moved lines with a different color

[Stefan Beller]

When we color the diff, we'll mark moved lines with a different color.

This is achieved by doing a two passes over the diff. The first pass
will inspect each line of the diff and store the removed lines and the
added lines in its own hash map.
The second pass will check for each added line if that is found in the
set of removed lines. If so it will color the added line differently as
with the new `moved-new` color mode. For each removed line we check the
set of added lines and if found emit that with the new color `moved-old`.

When detecting the moved lines, we cannot just rely on a line being equal,
but we need to take the context into account to detect when the moves were
reordered as we do not want to color moved but per-mutated lines.
To do that we use the hash of the preceding line.

This patch was motivated by e.g. reviewing 3b0c4200 ("apply: move
libified code from builtin/apply.c to apply.{c,h}", 2016-08-08)

Signed-off-by: Stefan Beller 
---

 * fixed memleaks
 * only run the new code when needed.
 * renamed variables to fit in better.


 Documentation/config.txt   |   5 +-
 contrib/completion/git-completion.bash |   2 +
 diff.c | 200 +++--
 diff.h |   5 +-
 4 files changed, 202 insertions(+), 10 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 0bcb679..f4f51c2 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -980,8 +980,9 @@ color.diff.::
of `context` (context text - `plain` is a historical synonym),
`meta` (metainformation), `frag`
(hunk header), 'func' (function in hunk header), `old` (removed lines),
-   `new` (added lines), `commit` (commit headers), or `whitespace`
-   (highlighting whitespace errors).
+   `new` (added lines), `commit` (commit headers), `whitespace`
+   (highlighting whitespace errors), `moved-old` (removed lines that
+   reappear), `moved-new` (added lines that were removed elsewhere).
 
 color.decorate.::
Use customized color for 'git log --decorate' output.  `` is one
diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 9c8f738..b558d12 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2115,6 +2115,8 @@ _git_config ()
color.diff.old
color.diff.plain
color.diff.whitespace
+   color.diff.moved-old
+   color.diff.moved-new
color.grep
color.grep.context
color.grep.filename
diff --git a/diff.c b/diff.c
index 534c12e..7fde014 100644
--- a/diff.c
+++ b/diff.c
@@ -18,6 +18,7 @@
 #include "ll-merge.h"
 #include "string-list.h"
 #include "argv-array.h"
+#include "git-compat-util.h"
 
 #ifdef NO_FAST_WORKING_DIRECTORY
 #define FAST_WORKING_DIRECTORY 0
@@ -42,6 +43,16 @@ static int diff_dirstat_permille_default = 30;
 static struct diff_options default_diff_options;
 static long diff_algorithm;
 
+static struct hashmap *moved_add;
+static struct hashmap *moved_del;
+static int hash_prev_added;
+static int hash_prev_removed;
+struct moved_entry {
+   struct hashmap_entry ent;
+   char *line;
+   int hash_prev_line;
+};
+
 static char diff_colors[][COLOR_MAXLEN] = {
GIT_COLOR_RESET,
GIT_COLOR_NORMAL,   /* CONTEXT */
@@ -52,6 +63,8 @@ static char diff_colors[][COLOR_MAXLEN] = {
GIT_COLOR_YELLOW,   /* COMMIT */
GIT_COLOR_BG_RED,   /* WHITESPACE */
GIT_COLOR_NORMAL,   /* FUNCINFO */
+   GIT_COLOR_BLUE, /* NEW MOVED */
+   GIT_COLOR_MAGENTA,  /* OLD MOVED */
 };
 
 static int parse_diff_color_slot(const char *var)
@@ -72,6 +85,10 @@ static int parse_diff_color_slot(const char *var)
return DIFF_WHITESPACE;
if (!strcasecmp(var, "func"))
return DIFF_FUNCINFO;
+   if (!strcasecmp(var, "moved-old"))
+   return DIFF_FILE_MOVED_OLD;
+   if (!strcasecmp(var, "moved-new"))
+   return DIFF_FILE_MOVED_NEW;
return -1;
 }
 
@@ -287,6 +304,25 @@ int git_diff_basic_config(const char *var, const char 
*value, void *cb)
return git_default_config(var, value, cb);
 }
 
+static int moved_entry_cmp(const struct moved_entry *a,
+  const struct moved_entry *b,
+  const void *unused)
+{
+   return strcmp(a->line, b->line) &&
+  a->hash_prev_line == b->hash_prev_line;
+}
+
+static struct moved_entry *prepare_entry(const char *line,
+unsigned long len,
+int hash_prev_line)
+{
+   struct moved_entry *ret = xmalloc(sizeof(*ret));
+   ret->ent.hash = memhash(line, len) ^ hash_prev_line;
+   ret->line = xmemdupz(line, len);
+   

Re: [RFC/PATCH 0/2] Color moved code differently

[Junio C Hamano]

Junio C Hamano  writes:

> I do not think you should step outside diff_flush().  Only when
> producing textual diff, you would have to run the textual diff
> twice by going over the q twice:
>
>  * The first pass would run diff_flush_patch(), which would call
>into xdiff the usual way, but the callback from xdiff would
>capture the removed lines and the added lines without making any
>output.
>
>  * The second pass would run diff_flush_patch(), but the callback
>from xdiff would be called with additional information, namely,
>the removed and the added lines captured in the first pass.
> ...
>The fn_out_consume() function working in the "second pass of
>moved from/moved to mode" would inspect line[] and see if it is
>an added or a removed line, and then:
>
>- if it is an added line, and it appears as a removed line
>  elsewhere in the patchset (you obtained the information in the
>  first pass), you show it as "this was moved from elsewhere".
>
>- if it is a removed line, and it appears as an added line
>  elsewhere in the patchset (you obtained the information in the
>  first pass), you show it as "this was moved to elsewhere".
>
> Or something like that.

Actually, the first pass above ends up queuing majority of the diff
output in-core _anyway_, so it would make much more sense not to
rely on the reproducibility of xdiff machinery by not calling it
twice.  Instead, I would imagine that the first pass can queue all
what it receives in the callback, then with the whole-patch picture
at hand, you can call updated fn_out_consume() yourself (instead of
letting xdiff machinery to call you), and that would become the
"second pass".

Re: [PATCH v2 05/38] refs: create a base class "ref_store" for files_ref_store

[David Turner]

On Sun, 2016-09-04 at 18:08 +0200, Michael Haggerty wrote:

> +/* A linked list of ref_stores for submodules: */
> +static struct ref_store *submodule_ref_stores;

I don't like the per-submodule stores being in a linked list, which
requires a linear search.  Stefan has, I think, been doing a bunch of
work to scale git to support on the order of thousands of submodules,
which this potentially breaks.  What about a hashmap?

[PATCH v14 41/41] 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, 43 insertions(+), 22 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 739b34d..38d5d11 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,59 @@ static int parse_mail_rebase(struct am_state *state, 
const char *mail)
  */
 static int run_apply(const struct am_state *state, const char *index_file)
 {
-   struct child_process cp = CHILD_PROCESS_INIT;
-
-   cp.git_cmd = 1;
-
-   if (index_file)
-   argv_array_pushf(_array, "GIT_INDEX_FILE=%s", 
index_file);
+   struct argv_array apply_paths = ARGV_ARRAY_INIT;
+   struct argv_array apply_opts = ARGV_ARRAY_INIT;
+   struct apply_state apply_state;
+   int res, opts_left;
+   static struct lock_file lock_file;
+   int force_apply = 0;
+   int options = 0;
+
+   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 through to git apply");
+
+   if (index_file) {
+   apply_state.index_file = 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"));
 
-   if (index_file)
-   argv_array_push(, "--cached");
-   else
-   argv_array_push(, "--index");
+   res = apply_all_patches(_state, apply_paths.argc, 
apply_paths.argv, options);
 
-   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.10.0.41.g9df52c3

[PATCH v14 07/41] 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.10.0.41.g9df52c3

[PATCH v14 21/41] 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.10.0.41.g9df52c3

[PATCH v14 33/41] 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 v14 34/41] 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.10.0.41.g9df52c3

[PATCH v14 39/41] apply: pass apply state to build_fake_ancestor()

[Christian Couder]

To libify git apply functionality, we will need to read from a
different index file in get_current_sha1(). This index file will be
stored in "struct apply_state", so let's pass the state to
build_fake_ancestor() which will later pass it to get_current_sha1().

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

diff --git a/apply.c b/apply.c
index 2dad0db..f29387d 100644
--- a/apply.c
+++ b/apply.c
@@ -4042,7 +4042,7 @@ 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 int build_fake_ancestor(struct patch *list, const char *filename)
+static int build_fake_ancestor(struct apply_state *state, struct patch *list)
 {
struct patch *patch;
struct index_state result = { NULL };
@@ -4089,12 +4089,13 @@ static int build_fake_ancestor(struct patch *list, 
const char *filename)
}
}
 
-   hold_lock_file_for_update(, filename, LOCK_DIE_ON_ERROR);
+   hold_lock_file_for_update(, state->fake_ancestor, 
LOCK_DIE_ON_ERROR);
res = write_locked_index(, , COMMIT_LOCK);
discard_index();
 
if (res)
-   return error("Could not write temporary index to %s", filename);
+   return error("Could not write temporary index to %s",
+state->fake_ancestor);
 
return 0;
 }
@@ -4709,7 +4710,7 @@ static int apply_patch(struct apply_state *state,
}
 
if (state->fake_ancestor &&
-   build_fake_ancestor(list, state->fake_ancestor)) {
+   build_fake_ancestor(state, list)) {
res = -128;
goto end;
}
-- 
2.10.0.41.g9df52c3

[PATCH v14 22/41] 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.10.0.41.g9df52c3

[PATCH v14 18/41] 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.10.0.41.g9df52c3

[PATCH v14 37/41] 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.

Note that, as we need to restore the saved routines in all
cases, we cannot return early any more in apply_all_patches().

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..27baf0e 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 *msg, 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 f015403..902346b 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.10.0.41.g9df52c3

[PATCH v14 40/41] apply: learn to use a different index file

[Christian Couder]

Sometimes we want to apply in a different index file.

Before the apply functionality was libified it was possible to
use the GIT_INDEX_FILE environment variable, for this purpose.

But now, as the apply functionality has been libified, it should
be possible to do that in a libified way.

Signed-off-by: Christian Couder 
---
 apply.c | 27 +--
 apply.h |  1 +
 2 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/apply.c b/apply.c
index f29387d..b1dd998 100644
--- a/apply.c
+++ b/apply.c
@@ -3993,12 +3993,21 @@ static int check_patch_list(struct apply_state *state, 
struct patch *patch)
return err;
 }
 
+static int read_apply_cache(struct apply_state *state)
+{
+   if (state->index_file)
+   return read_cache_from(state->index_file);
+   else
+   return read_cache();
+}
+
 /* This function tries to read the sha1 from the current index */
-static int get_current_sha1(const char *path, unsigned char *sha1)
+static int get_current_sha1(struct apply_state *state, const char *path,
+   unsigned char *sha1)
 {
int pos;
 
-   if (read_cache() < 0)
+   if (read_apply_cache(state) < 0)
return -1;
pos = cache_name_pos(path, strlen(path));
if (pos < 0)
@@ -4071,7 +4080,7 @@ static int build_fake_ancestor(struct apply_state *state, 
struct patch *list)
; /* ok */
} else if (!patch->lines_added && !patch->lines_deleted) {
/* mode-only change: update the current */
-   if (get_current_sha1(patch->old_name, sha1))
+   if (get_current_sha1(state, patch->old_name, sha1))
return error("mode change for %s, which is not "
 "in current HEAD", name);
} else
@@ -4675,10 +4684,16 @@ static int apply_patch(struct apply_state *state,
state->apply = 0;
 
state->update_index = state->check_index && state->apply;
-   if (state->update_index && state->newfd < 0)
-   state->newfd = hold_locked_index(state->lock_file, 1);
+   if (state->update_index && state->newfd < 0) {
+   if (state->index_file)
+   state->newfd = 
hold_lock_file_for_update(state->lock_file,
+
state->index_file,
+
LOCK_DIE_ON_ERROR);
+   else
+   state->newfd = hold_locked_index(state->lock_file, 1);
+   }
 
-   if (state->check_index && read_cache() < 0) {
+   if (state->check_index && read_apply_cache(state) < 0) {
error(_("unable to read index file"));
res = -128;
goto end;
diff --git a/apply.h b/apply.h
index 9fec536..b3d6783 100644
--- a/apply.h
+++ b/apply.h
@@ -63,6 +63,7 @@ struct apply_state {
int unsafe_paths;
 
/* Other non boolean parameters */
+   const char *index_file;
enum apply_verbosity apply_verbosity;
const char *fake_ancestor;
const char *patch_input_file;
-- 
2.10.0.41.g9df52c3

[PATCH v14 10/41] 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 d96ecb7..c6ac236 100644
--- a/Makefile
+++ b/Makefile
@@ -691,6 +691,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 v14 26/41] 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..3145e03 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_errno(_("closing file '%s'"), path);
+
+   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.10.0.41.g9df52c3

[PATCH v14 25/41] 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.10.0.41.g9df52c3

[PATCH v14 23/41] 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.10.0.41.g9df52c3

[PATCH v14 35/41] 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 db89ba7..d189b4f 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.10.0.41.g9df52c3

[PATCH v14 32/41] 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 | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/apply.c b/apply.c
index a4dfc64..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, );
}
@@ -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.10.0.41.g9df52c3

[PATCH v14 08/41] 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.10.0.41.g9df52c3

[PATCH v14 38/41] 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 27baf0e..2dad0db 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 v14 36/41] 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 d189b4f..bac8613 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.10.0.41.g9df52c3

[PATCH v14 28/41] 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 5cd7fd8..1f56303 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 v14 29/41] 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.

Helped-by: Stefan Beller 
Signed-off-by: Christian Couder 
---
 apply.h |  7 +++
 builtin/apply.c | 11 ---
 2 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/apply.h b/apply.h
index 53f09b5..48abd8e 100644
--- a/apply.h
+++ b/apply.h
@@ -108,4 +108,11 @@ 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);
 
+/*
+ * Some aspects of the apply behavior are controlled by the following
+ * bits in the "options" parameter passed to apply_all_patches().
+ */
+#define APPLY_OPT_INACCURATE_EOF   (1<<0) /* accept inaccurate eof */
+#define APPLY_OPT_RECOUNT  (1<<1) /* accept inaccurate line count 
*/
+
 #endif
diff --git a/builtin/apply.c b/builtin/apply.c
index 1f56303..da31af2 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.10.0.41.g9df52c3

[PATCH v14 31/41] 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 f640224..a4dfc64 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 28cbe6c..51b983b 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.10.0.41.g9df52c3

[PATCH v14 24/41] 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.10.0.41.g9df52c3

[PATCH v14 06/41] 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.10.0.41.g9df52c3

[PATCH v14 27/41] 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 3145e03..5cd7fd8 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.10.0.41.g9df52c3

[PATCH v14 05/41] 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 v14 16/41] 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.10.0.41.g9df52c3

[PATCH v14 09/41] 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.10.0.41.g9df52c3

[PATCH v14 12/41] 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.10.0.41.g9df52c3

[PATCH v14 11/41] 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.10.0.41.g9df52c3

[PATCH v14 17/41] 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.10.0.41.g9df52c3

[PATCH v14 19/41] 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.10.0.41.g9df52c3

[PATCH v14 13/41] 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.10.0.41.g9df52c3

[PATCH v14 20/41] 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.10.0.41.g9df52c3

[PATCH v14 14/41] 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.10.0.41.g9df52c3

[PATCH v14 15/41] 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.10.0.41.g9df52c3

[PATCH v14 04/41] 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.10.0.41.g9df52c3

[PATCH v14 02/41] 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 v14 03/41] 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 v14 00/41] 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/
v10: 
https://public-inbox.org/git/20160808210337.5038-1-chriscool%40tuxfamily.org/
v11: 
https://public-inbox.org/git/20160811085229.19017-1-chriscool%40tuxfamily.org/
v12: https://public-inbox.org/git/20160811184501.384-1-chrisc...@tuxfamily.org/
v13: https://public-inbox.org/git/20160827184547.4365-1-chrisc...@tuxfamily.org/

Highlevel view of the patches in the series
~~~

This is "part 2" of the full patch series. I am resending everything
that is not already in master to give people another chance to review
everything.

  - Patch 01/41 to 27/41 were in v10, but not in v11, v12, and v13.

They libify the apply functions by making them return error codes
instead of die()ing or exit()ing and by making sure they release all
the resources they use.

There is only one change in patch 26/41 where I now use error_errno()
right away instead of first using error() and then converting the
error() into error_errno() in patch 32/41.

  - Patches 28/41 to 31/41 were in v12 and v13.

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

In patch 29/41 I added some comments in apply.h above the definition
of APPLY_OPT_INACCURATE_EOF and APPLY_OPT_RECOUNT, as suggested by
Stefan. This is the only change compared to v13.

  - Patch 32/41 was in v10, v12 and v13.

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

One hunk has been removed because of the changes in 26/41.

  - Patches 33/41 to 37/41 were in v10, v12 and v13.

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

The only changes since v13 are in 37/41. The name of the first
argument to mute_routine() is changed from "bla" to "msg" as suggested
by Ramsey, and the commit message is improved as suggested by Stefan.

  - Patch 38/41 was in v10, v12 and v13, and hasn't changed.

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.

  - Patch 39/41 was new in v13, and hasn't changed.

It is a refactoring to prepare for patch 40/41.

  - Patch 40/41 was in v12 and v13, and hasn't changed.

It adds a "const char *index_file" into "struct apply_state", to make
it possible to use a special index file instead of the default one.

  - Patch 41/41 was in v10, v12 and v13.

This patch makes `git am` use the libified functionality.

The only change compared to v13 is that "through" instead of "thru" is
used in an error message.

General comments


Now this patch series is 41 patches long to give yet another
possibility for all the patches to be reviewed.

I took another look at all these patches myself and didn't find
anything other than the changes I made in 26/41 to improve.

I will send a diff between this version and v13 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 and also prevented the split-index mechanism to provide
many performance benefits.

By the way, current work is ongoing to make it possible to use
split-index more easily by adding a config variable, see:

https://public-inbox.org/git/20160711172254.13439-1-chriscool%40tuxfamily.org/

Using an earlier 

[PATCH v14 01/41] 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 the 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.10.0.41.g9df52c3

Re: [PATCH v13 00/14] libify apply and use lib in am, part 3

[Christian Couder]

On Thu, Sep 1, 2016 at 7:48 PM, Junio C Hamano  wrote:
> Christian Couder  writes:
>
>> Following Stefan's review, it looks like I will need to resend at
>> least 02/14, 10/14 and 14/14.
>> What do you prefer me to resend:
>> 1) all the last 40 or so patches
>> 2) the last 14 patches
>> 3) only the few patches that changed
>
> If this reroll is to be the candidate to be the final one, I'd
> prefer to see 1 to give an easy access to more sets of eyes, but if
> you just send them without giving these patches one more read-over
> before sending them out, it is not as valuable as it would be.

Ok, I will send the 41 patches very soon after having given them one
more read over and implementing the few changes suggested by Stefan
and Ramsay and one improvement I spotted.

> I think 2 is of the least value.
>
> If you do 3., pointing people at where the remainder of the series
> can be found is necessary.

There is always a pointer to the full patch series on GitHub anyway in
the cover letter, but I will send everything anyway.

Re: [PATCH v2 04/38] refs: add a backend method structure

[David Turner]

On Sun, 2016-09-04 at 18:08 +0200, Michael Haggerty wrote:
> From: Ronnie Sahlberg 
> 
> Add a `struct ref_storage_be` to represent types of reference stores. In
> OO notation, this is the class, and will soon hold some class
> methods (e.g., a factory to create new ref_store instances) and will
> also serve as the vtable for ref_store instances of that type.
> 
> As yet, the backends cannot do anything.
> 
> Signed-off-by: Ronnie Sahlberg 
> Signed-off-by: David Turner 
> Signed-off-by: Junio C Hamano 
> Signed-off-by: Jeff King 
> Signed-off-by: Michael Haggerty 
> Signed-off-by: Junio C Hamano 


nit: duplicate Sign-off from Junio.

[WIP PATCH v2] diff.c: emit moved lines with a different color

[Stefan Beller]

From: Stefan Beller 

When we color the diff, we'll mark moved lines with a different color.

This is achieved by doing a two passes over the diff. The first pass
will inspect each line of the diff and store the removed lines and the
added lines in its own hash map.
The second pass will check for each added line if that is found in the
set of removed lines. If so it will color the added line differently as
with the new `moved-new` color mode.
For each removed line we check the set of added lines and if found emit
that with the new color `moved-old`.

When detecting the moved lines, we cannot just rely on a line being equal,
but we need to take the context into account to detect when the moves were
reordered as we do not want to color moved but per-mutated lines.

This patch was motivated by e.g. reviewing 3b0c4200 ("apply: move
libified code from builtin/apply.c to apply.{c,h}", 2016-08-08)

Signed-off-by: Stefan Beller 
---

  This adds code only in the diff_flush part, just as you suggested Junio.
  
  I think the design is sound now, as it produces good highlights across files;
  though now the code still sucks.
   * leaking memory; 
   * we run the code to detect moves more often than we need, e.g.
 when we configured an external diff, we don't need to run it.
 Or when we do not use colors at all.
   * I need to think about if we need everything doubled for both add and
   removal; maybe we can use one variable for both cases.


  Jacob wrote:
  
  > In the example, the first and last lines of duplicate copies don't get
  > colored differently, and that threw  me off. I feel like that was
  > maybe not intentional? If it was, can you explain why?

  We need the context to detect permutations, improved version:
  
  http://i.imgur.com/MnaSZ1D.png


  Jakub wrote:
  
  > P.S. BTW. does this work with word-diff?
  
  No (not yet )
  
  Thanks,
  Stefan

 Documentation/config.txt   |   5 +-
 contrib/completion/git-completion.bash |   2 +
 diff.c | 200 -
 diff.h |   5 +-
 4 files changed, 205 insertions(+), 7 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 0bcb679..f4f51c2 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -980,8 +980,9 @@ color.diff.::
of `context` (context text - `plain` is a historical synonym),
`meta` (metainformation), `frag`
(hunk header), 'func' (function in hunk header), `old` (removed lines),
-   `new` (added lines), `commit` (commit headers), or `whitespace`
-   (highlighting whitespace errors).
+   `new` (added lines), `commit` (commit headers), `whitespace`
+   (highlighting whitespace errors), `moved-old` (removed lines that
+   reappear), `moved-new` (added lines that were removed elsewhere).
 
 color.decorate.::
Use customized color for 'git log --decorate' output.  `` is one
diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 9c8f738..b558d12 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2115,6 +2115,8 @@ _git_config ()
color.diff.old
color.diff.plain
color.diff.whitespace
+   color.diff.moved-old
+   color.diff.moved-new
color.grep
color.grep.context
color.grep.filename
diff --git a/diff.c b/diff.c
index 534c12e..d37cb4f 100644
--- a/diff.c
+++ b/diff.c
@@ -18,6 +18,7 @@
 #include "ll-merge.h"
 #include "string-list.h"
 #include "argv-array.h"
+#include "git-compat-util.h"
 
 #ifdef NO_FAST_WORKING_DIRECTORY
 #define FAST_WORKING_DIRECTORY 0
@@ -42,6 +43,11 @@ static int diff_dirstat_permille_default = 30;
 static struct diff_options default_diff_options;
 static long diff_algorithm;
 
+static struct hashmap *duplicates_added;
+static struct hashmap *duplicates_removed;
+static int hash_previous_line_added;
+static int hash_previous_line_removed;
+
 static char diff_colors[][COLOR_MAXLEN] = {
GIT_COLOR_RESET,
GIT_COLOR_NORMAL,   /* CONTEXT */
@@ -52,6 +58,8 @@ static char diff_colors[][COLOR_MAXLEN] = {
GIT_COLOR_YELLOW,   /* COMMIT */
GIT_COLOR_BG_RED,   /* WHITESPACE */
GIT_COLOR_NORMAL,   /* FUNCINFO */
+   GIT_COLOR_BLUE, /* NEW MOVED */
+   GIT_COLOR_MAGENTA,  /* OLD MOVED */
 };
 
 static int parse_diff_color_slot(const char *var)
@@ -72,6 +80,10 @@ static int parse_diff_color_slot(const char *var)
return DIFF_WHITESPACE;
if (!strcasecmp(var, "func"))
return DIFF_FUNCINFO;
+   if (!strcasecmp(var, "moved-old"))
+   return DIFF_FILE_MOVED_OLD;
+   if (!strcasecmp(var, "moved-new"))
+   return DIFF_FILE_MOVED_NEW;
return -1;
 }
 

Re: [PATCH v13 10/14] apply: change error_routine when silent

[Christian Couder]

On Sun, Sep 4, 2016 at 6:31 PM, Ramsay Jones
 wrote:
>
>
> On 04/09/16 11:54, Christian Couder wrote:
>> On Thu, Sep 1, 2016 at 10:19 AM, Christian Couder
>>  wrote:
>>> On Thu, Sep 1, 2016 at 12:20 AM, Stefan Beller  wrote:
>
> +static void mute_routine(const char *bla, va_list params)

 Instead of 'bla' you could go with 'format' as the man page for
 [f]printf puts it.
 Or you could leave it empty, i.e.

 static void mute_routine(const char *, va_list)
 ...
>>>
>>> Ok to do that.
>>
>> Actually I get the following error when doing that:
>>
>> apply.c: In function ‘mute_routine’:
>> apply.c:115:1: error: parameter name omitted
>>  static void mute_routine(const char *, va_list)
>>  ^
>> apply.c:115:1: error: parameter name omitted
>> make: *** [apply.o] Error 1
>
> Yes, this is not C++. ;-)
>
>> So I will leave it as is.
>
> I think I would prefer to see:
>
> static void mute_routine(const char *msg, va_list params)
>
> given that it would either be an error-msg or a warning-msg.

Ok for "msg".

Re: [PATCH v13 10/14] apply: change error_routine when silent

[Ramsay Jones]

On 04/09/16 11:54, Christian Couder wrote:
> On Thu, Sep 1, 2016 at 10:19 AM, Christian Couder
>  wrote:
>> On Thu, Sep 1, 2016 at 12:20 AM, Stefan Beller  wrote:
>>> On Sat, Aug 27, 2016 at 11:45 AM, Christian Couder
>>>  wrote:
 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)
>>>
>>> Instead of 'bla' you could go with 'format' as the man page for
>>> [f]printf puts it.
>>> Or you could leave it empty, i.e.
>>>
>>> static void mute_routine(const char *, va_list)
>>> ...
>>
>> Ok to do that.
> 
> Actually I get the following error when doing that:
> 
> apply.c: In function ‘mute_routine’:
> apply.c:115:1: error: parameter name omitted
>  static void mute_routine(const char *, va_list)
>  ^
> apply.c:115:1: error: parameter name omitted
> make: *** [apply.o] Error 1

Yes, this is not C++. ;-)

> So I will leave it as is.

I think I would prefer to see:

static void mute_routine(const char *msg, va_list params)

given that it would either be an error-msg or a warning-msg.

ATB,
Ramsay Jones

[PATCH v2 25/38] lock_raw_ref(): add a files_ref_store argument

[Michael Haggerty]

Signed-off-by: Michael Haggerty 
Signed-off-by: Junio C Hamano 
---
 refs/files-backend.c | 14 --
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 363f306..ae425c5 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1523,7 +1523,8 @@ static void unlock_ref(struct ref_lock *lock)
  *   avoided, namely if we were successfully able to read the ref
  * - Generate informative error messages in the case of failure
  */
-static int lock_raw_ref(const char *refname, int mustexist,
+static int lock_raw_ref(struct files_ref_store *refs,
+   const char *refname, int mustexist,
const struct string_list *extras,
const struct string_list *skip,
struct ref_lock **lock_p,
@@ -1531,15 +1532,14 @@ static int lock_raw_ref(const char *refname, int 
mustexist,
unsigned int *type,
struct strbuf *err)
 {
-   struct ref_store *ref_store = get_ref_store(NULL);
-   struct files_ref_store *refs =
-   files_downcast(ref_store, 0, "lock_raw_ref");
struct ref_lock *lock;
struct strbuf ref_file = STRBUF_INIT;
int attempts_remaining = 3;
int ret = TRANSACTION_GENERIC_ERROR;
 
assert(err);
+   assert_main_repository(>base, "lock_raw_ref");
+
*type = 0;
 
/* First lock the file so it can't change out from under us. */
@@ -1623,7 +1623,7 @@ static int lock_raw_ref(const char *refname, int 
mustexist,
 * fear that its value will change.
 */
 
-   if (files_read_raw_ref(ref_store, refname,
+   if (files_read_raw_ref(>base, refname,
   lock->old_oid.hash, referent, type)) {
if (errno == ENOENT) {
if (mustexist) {
@@ -3518,6 +3518,8 @@ static int lock_ref_for_update(struct ref_update *update,
   struct string_list *affected_refnames,
   struct strbuf *err)
 {
+   struct files_ref_store *refs =
+   get_files_ref_store(NULL, "lock_ref_for_update");
struct strbuf referent = STRBUF_INIT;
int mustexist = (update->flags & REF_HAVE_OLD) &&
!is_null_sha1(update->old_sha1);
@@ -3534,7 +3536,7 @@ static int lock_ref_for_update(struct ref_update *update,
return ret;
}
 
-   ret = lock_raw_ref(update->refname, mustexist,
+   ret = lock_raw_ref(refs, update->refname, mustexist,
   affected_refnames, NULL,
   >lock, ,
   >type, err);
-- 
2.9.3

[PATCH v2 24/38] repack_without_refs(): add a files_ref_store argument

[Michael Haggerty]

Signed-off-by: Michael Haggerty 
Signed-off-by: Junio C Hamano 
---
 refs/files-backend.c | 12 +++-
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 8d43e0b..363f306 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2384,14 +2384,14 @@ static int files_pack_refs(struct ref_store *ref_store, 
unsigned int flags)
  *
  * The refs in 'refnames' needn't be sorted. `err` must not be NULL.
  */
-static int repack_without_refs(struct string_list *refnames, struct strbuf 
*err)
+static int repack_without_refs(struct files_ref_store *refs,
+  struct string_list *refnames, struct strbuf *err)
 {
-   struct files_ref_store *refs =
-   get_files_ref_store(NULL, "repack_without_refs");
struct ref_dir *packed;
struct string_list_item *refname;
int ret, needs_repacking = 0, removed = 0;
 
+   assert_main_repository(>base, "repack_without_refs");
assert(err);
 
/* Look for a packed ref */
@@ -2453,13 +2453,15 @@ static int delete_ref_loose(struct ref_lock *lock, int 
flag, struct strbuf *err)
 
 int delete_refs(struct string_list *refnames, unsigned int flags)
 {
+   struct files_ref_store *refs =
+   get_files_ref_store(NULL, "delete_refs");
struct strbuf err = STRBUF_INIT;
int i, result = 0;
 
if (!refnames->nr)
return 0;
 
-   result = repack_without_refs(refnames, );
+   result = repack_without_refs(refs, refnames, );
if (result) {
/*
 * If we failed to rewrite the packed-refs file, then
@@ -3779,7 +3781,7 @@ static int files_transaction_commit(struct ref_store 
*ref_store,
}
}
 
-   if (repack_without_refs(_to_delete, err)) {
+   if (repack_without_refs(refs, _to_delete, err)) {
ret = TRANSACTION_GENERIC_ERROR;
goto cleanup;
}
-- 
2.9.3

[PATCH v2 21/38] refs: make pack_refs() virtual

[Michael Haggerty]

Signed-off-by: Michael Haggerty 
Signed-off-by: Junio C Hamano 
---
 refs.c   | 7 +++
 refs/files-backend.c | 6 --
 refs/refs-internal.h | 4 
 3 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/refs.c b/refs.c
index 662c417..961927a 100644
--- a/refs.c
+++ b/refs.c
@@ -1421,6 +1421,13 @@ void assert_main_repository(struct ref_store *refs, 
const char *caller)
 }
 
 /* backend functions */
+int pack_refs(unsigned int flags)
+{
+   struct ref_store *refs = get_ref_store(NULL);
+
+   return refs->be->pack_refs(refs, flags);
+}
+
 int ref_transaction_commit(struct ref_transaction *transaction,
   struct strbuf *err)
 {
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 4e1d431..7ad8821 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2354,10 +2354,10 @@ static void prune_refs(struct ref_to_prune *r)
}
 }
 
-int pack_refs(unsigned int flags)
+static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)
 {
struct files_ref_store *refs =
-   get_files_ref_store(NULL, "pack_refs");
+   files_downcast(ref_store, 0, "pack_refs");
struct pack_refs_cb_data cbdata;
 
memset(, 0, sizeof(cbdata));
@@ -4032,6 +4032,8 @@ struct ref_storage_be refs_be_files = {
files_ref_store_create,
files_transaction_commit,
 
+   files_pack_refs,
+
files_read_raw_ref,
files_verify_refname_available
 };
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 6c698f4..256f7f5 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -500,6 +500,8 @@ typedef int ref_transaction_commit_fn(struct ref_store 
*refs,
  struct ref_transaction *transaction,
  struct strbuf *err);
 
+typedef int pack_refs_fn(struct ref_store *ref_store, unsigned int flags);
+
 /*
  * Read a reference from the specified reference store, non-recursively.
  * Set type to describe the reference, and:
@@ -554,6 +556,8 @@ struct ref_storage_be {
ref_store_init_fn *init;
ref_transaction_commit_fn *transaction_commit;
 
+   pack_refs_fn *pack_refs;
+
read_raw_ref_fn *read_raw_ref;
verify_refname_available_fn *verify_refname_available;
 };
-- 
2.9.3

[PATCH v2 19/38] refs: make read_raw_ref() virtual

[Michael Haggerty]

Reference backends will be able to customize this function to implement
reference reading.

Signed-off-by: Michael Haggerty 
Signed-off-by: Junio C Hamano 
---
 refs.c   |  4 ++--
 refs/files-backend.c | 14 --
 refs/refs-internal.h | 36 +++-
 3 files changed, 29 insertions(+), 25 deletions(-)

diff --git a/refs.c b/refs.c
index e881874..7a7adeb 100644
--- a/refs.c
+++ b/refs.c
@@ -1251,8 +1251,8 @@ static const char *resolve_ref_recursively(struct 
ref_store *refs,
for (symref_count = 0; symref_count < SYMREF_MAXDEPTH; symref_count++) {
unsigned int read_flags = 0;
 
-   if (read_raw_ref(refs, refname,
-sha1, _refname, _flags)) {
+   if (refs->be->read_raw_ref(refs, refname,
+  sha1, _refname, _flags)) {
*flags |= read_flags;
if (errno != ENOENT || (resolve_flags & 
RESOLVE_REF_READING))
return NULL;
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 6382c6b..ff11a51 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1349,9 +1349,9 @@ static int resolve_packed_ref(struct files_ref_store 
*refs,
return -1;
 }
 
-int read_raw_ref(struct ref_store *ref_store,
-const char *refname, unsigned char *sha1,
-struct strbuf *referent, unsigned int *type)
+static int files_read_raw_ref(struct ref_store *ref_store,
+ const char *refname, unsigned char *sha1,
+ struct strbuf *referent, unsigned int *type)
 {
struct files_ref_store *refs =
files_downcast(ref_store, 1, "read_raw_ref");
@@ -1623,8 +1623,8 @@ static int lock_raw_ref(const char *refname, int 
mustexist,
 * fear that its value will change.
 */
 
-   if (read_raw_ref(ref_store, refname,
-lock->old_oid.hash, referent, type)) {
+   if (files_read_raw_ref(ref_store, refname,
+  lock->old_oid.hash, referent, type)) {
if (errno == ENOENT) {
if (mustexist) {
/* Garden variety missing reference. */
@@ -4029,5 +4029,7 @@ struct ref_storage_be refs_be_files = {
NULL,
"files",
files_ref_store_create,
-   files_transaction_commit
+   files_transaction_commit,
+
+   files_read_raw_ref
 };
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index fa41d51..19cb6e1 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -486,6 +486,20 @@ int do_for_each_ref_iterator(struct ref_iterator *iter,
 
 struct ref_store;
 
+/* refs backends */
+
+/*
+ * Initialize the ref_store for the specified submodule, or for the
+ * main repository if submodule == NULL. These functions should call
+ * base_ref_store_init() to initialize the shared part of the
+ * ref_store and to record the ref_store for later lookup.
+ */
+typedef struct ref_store *ref_store_init_fn(const char *submodule);
+
+typedef int ref_transaction_commit_fn(struct ref_store *refs,
+ struct ref_transaction *transaction,
+ struct strbuf *err);
+
 /*
  * Read a reference from the specified reference store, non-recursively.
  * Set type to describe the reference, and:
@@ -524,29 +538,17 @@ struct ref_store;
  * - in all other cases, referent will be untouched, and therefore
  *   refname will still be valid and unchanged.
  */
-int read_raw_ref(struct ref_store *ref_store,
-const char *refname, unsigned char *sha1,
-struct strbuf *referent, unsigned int *type);
-
-/* refs backends */
-
-/*
- * Initialize the ref_store for the specified submodule, or for the
- * main repository if submodule == NULL. These functions should call
- * base_ref_store_init() to initialize the shared part of the
- * ref_store and to record the ref_store for later lookup.
- */
-typedef struct ref_store *ref_store_init_fn(const char *submodule);
-
-typedef int ref_transaction_commit_fn(struct ref_store *refs,
- struct ref_transaction *transaction,
- struct strbuf *err);
+typedef int read_raw_ref_fn(struct ref_store *ref_store,
+   const char *refname, unsigned char *sha1,
+   struct strbuf *referent, unsigned int *type);
 
 struct ref_storage_be {
struct ref_storage_be *next;
const char *name;
ref_store_init_fn *init;
ref_transaction_commit_fn *transaction_commit;
+
+   read_raw_ref_fn *read_raw_ref;
 };
 
 extern struct ref_storage_be refs_be_files;
-- 
2.9.3

[PATCH v2 20/38] refs: make verify_refname_available() virtual

[Michael Haggerty]

Signed-off-by: Michael Haggerty 
Signed-off-by: Junio C Hamano 
---
 refs.c   | 10 ++
 refs/files-backend.c | 14 --
 refs/refs-internal.h |  7 +++
 3 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/refs.c b/refs.c
index 7a7adeb..662c417 100644
--- a/refs.c
+++ b/refs.c
@@ -1428,3 +1428,13 @@ int ref_transaction_commit(struct ref_transaction 
*transaction,
 
return refs->be->transaction_commit(refs, transaction, err);
 }
+
+int verify_refname_available(const char *refname,
+const struct string_list *extra,
+const struct string_list *skip,
+struct strbuf *err)
+{
+   struct ref_store *refs = get_ref_store(NULL);
+
+   return refs->be->verify_refname_available(refs, refname, extra, skip, 
err);
+}
diff --git a/refs/files-backend.c b/refs/files-backend.c
index ff11a51..4e1d431 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2549,13 +2549,14 @@ static int rename_tmp_log(const char *newrefname)
return ret;
 }
 
-int verify_refname_available(const char *newname,
-const struct string_list *extras,
-const struct string_list *skip,
-struct strbuf *err)
+static int files_verify_refname_available(struct ref_store *ref_store,
+ const char *newname,
+ const struct string_list *extras,
+ const struct string_list *skip,
+ struct strbuf *err)
 {
struct files_ref_store *refs =
-   get_files_ref_store(NULL, "verify_refname_available");
+   files_downcast(ref_store, 1, "verify_refname_available");
struct ref_dir *packed_refs = get_packed_refs(refs);
struct ref_dir *loose_refs = get_loose_refs(refs);
 
@@ -4031,5 +4032,6 @@ struct ref_storage_be refs_be_files = {
files_ref_store_create,
files_transaction_commit,
 
-   files_read_raw_ref
+   files_read_raw_ref,
+   files_verify_refname_available
 };
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 19cb6e1..6c698f4 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -542,6 +542,12 @@ typedef int read_raw_ref_fn(struct ref_store *ref_store,
const char *refname, unsigned char *sha1,
struct strbuf *referent, unsigned int *type);
 
+typedef int verify_refname_available_fn(struct ref_store *ref_store,
+   const char *newname,
+   const struct string_list *extras,
+   const struct string_list *skip,
+   struct strbuf *err);
+
 struct ref_storage_be {
struct ref_storage_be *next;
const char *name;
@@ -549,6 +555,7 @@ struct ref_storage_be {
ref_transaction_commit_fn *transaction_commit;
 
read_raw_ref_fn *read_raw_ref;
+   verify_refname_available_fn *verify_refname_available;
 };
 
 extern struct ref_storage_be refs_be_files;
-- 
2.9.3

[PATCH v2 28/38] lock_ref_sha1_basic(): add a files_ref_store argument

[Michael Haggerty]

Signed-off-by: Michael Haggerty 
Signed-off-by: Junio C Hamano 
---
 refs/files-backend.c | 26 +++---
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 2f8eb54..ab2c1de 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1983,15 +1983,14 @@ static int remove_empty_directories(struct strbuf *path)
  * Locks a ref returning the lock on success and NULL on failure.
  * On failure errno is set to something meaningful.
  */
-static struct ref_lock *lock_ref_sha1_basic(const char *refname,
+static struct ref_lock *lock_ref_sha1_basic(struct files_ref_store *refs,
+   const char *refname,
const unsigned char *old_sha1,
const struct string_list *extras,
const struct string_list *skip,
unsigned int flags, int *type,
struct strbuf *err)
 {
-   struct files_ref_store *refs =
-   get_files_ref_store(NULL, "lock_ref_sha1_basic");
struct strbuf ref_file = STRBUF_INIT;
struct ref_lock *lock;
int last_errno = 0;
@@ -2001,6 +2000,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char 
*refname,
int attempts_remaining = 3;
int resolved;
 
+   assert_main_repository(>base, "lock_ref_sha1_basic");
assert(err);
 
lock = xcalloc(1, sizeof(struct ref_lock));
@@ -2644,8 +2644,8 @@ int rename_ref(const char *oldrefname, const char 
*newrefname, const char *logms
 
logmoved = log;
 
-   lock = lock_ref_sha1_basic(newrefname, NULL, NULL, NULL, REF_NODEREF,
-  NULL, );
+   lock = lock_ref_sha1_basic(refs, newrefname, NULL, NULL, NULL,
+  REF_NODEREF, NULL, );
if (!lock) {
error("unable to rename '%s' to '%s': %s", oldrefname, 
newrefname, err.buf);
strbuf_release();
@@ -2663,8 +2663,8 @@ int rename_ref(const char *oldrefname, const char 
*newrefname, const char *logms
return 0;
 
  rollback:
-   lock = lock_ref_sha1_basic(oldrefname, NULL, NULL, NULL, REF_NODEREF,
-  NULL, );
+   lock = lock_ref_sha1_basic(refs, oldrefname, NULL, NULL, NULL,
+  REF_NODEREF, NULL, );
if (!lock) {
error("unable to lock %s for rollback: %s", oldrefname, 
err.buf);
strbuf_release();
@@ -3020,13 +3020,14 @@ static int files_create_symref(struct ref_store 
*ref_store,
   const char *refname, const char *target,
   const char *logmsg)
 {
+   struct files_ref_store *refs =
+   files_downcast(ref_store, 0, "create_symref");
struct strbuf err = STRBUF_INIT;
struct ref_lock *lock;
int ret;
 
-   files_downcast(ref_store, 0, "create_symref");
-
-   lock = lock_ref_sha1_basic(refname, NULL, NULL, NULL, REF_NODEREF, NULL,
+   lock = lock_ref_sha1_basic(refs, refname, NULL,
+  NULL, NULL, REF_NODEREF, NULL,
   );
if (!lock) {
error("%s", err.buf);
@@ -3939,6 +3940,8 @@ int reflog_expire(const char *refname, const unsigned 
char *sha1,
 reflog_expiry_cleanup_fn cleanup_fn,
 void *policy_cb_data)
 {
+   struct files_ref_store *refs =
+   get_files_ref_store(NULL, "reflog_expire");
static struct lock_file reflog_lock;
struct expire_reflog_cb cb;
struct ref_lock *lock;
@@ -3957,7 +3960,8 @@ int reflog_expire(const char *refname, const unsigned 
char *sha1,
 * reference itself, plus we might need to update the
 * reference if --updateref was specified:
 */
-   lock = lock_ref_sha1_basic(refname, sha1, NULL, NULL, REF_NODEREF,
+   lock = lock_ref_sha1_basic(refs, refname, sha1,
+  NULL, NULL, REF_NODEREF,
   , );
if (!lock) {
error("cannot lock ref '%s': %s", refname, err.buf);
-- 
2.9.3

[PATCH v2 11/38] refs: reorder definitions

[Michael Haggerty]

Move resolve_gitlink_ref() and related functions lower in the file to
avoid the need for forward declarations in the next step.

Signed-off-by: Michael Haggerty 
Signed-off-by: Junio C Hamano 
---
 refs/files-backend.c | 166 +--
 1 file changed, 83 insertions(+), 83 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index d896052..7673342 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1316,89 +1316,6 @@ static struct ref_dir *get_loose_refs(struct 
files_ref_store *refs)
return get_ref_dir(refs->loose);
 }
 
-#define MAXREFLEN (1024)
-
-/*
- * Called by resolve_gitlink_ref_recursive() after it failed to read
- * from the loose refs in refs. Find  in the packed-refs file
- * for the submodule.
- */
-static int resolve_gitlink_packed_ref(struct files_ref_store *refs,
- const char *refname, unsigned char *sha1)
-{
-   struct ref_entry *ref;
-   struct ref_dir *dir = get_packed_refs(refs);
-
-   ref = find_ref(dir, refname);
-   if (ref == NULL)
-   return -1;
-
-   hashcpy(sha1, ref->u.value.oid.hash);
-   return 0;
-}
-
-static int resolve_gitlink_ref_recursive(struct files_ref_store *refs,
-const char *refname, unsigned char 
*sha1,
-int recursion)
-{
-   int fd, len;
-   char buffer[128], *p;
-   char *path;
-
-   if (recursion > SYMREF_MAXDEPTH || strlen(refname) > MAXREFLEN)
-   return -1;
-   path = *refs->base.submodule
-   ? git_pathdup_submodule(refs->base.submodule, "%s", refname)
-   : git_pathdup("%s", refname);
-   fd = open(path, O_RDONLY);
-   free(path);
-   if (fd < 0)
-   return resolve_gitlink_packed_ref(refs, refname, sha1);
-
-   len = read(fd, buffer, sizeof(buffer)-1);
-   close(fd);
-   if (len < 0)
-   return -1;
-   while (len && isspace(buffer[len-1]))
-   len--;
-   buffer[len] = 0;
-
-   /* Was it a detached head or an old-fashioned symlink? */
-   if (!get_sha1_hex(buffer, sha1))
-   return 0;
-
-   /* Symref? */
-   if (strncmp(buffer, "ref:", 4))
-   return -1;
-   p = buffer + 4;
-   while (isspace(*p))
-   p++;
-
-   return resolve_gitlink_ref_recursive(refs, p, sha1, recursion+1);
-}
-
-int resolve_gitlink_ref(const char *path, const char *refname, unsigned char 
*sha1)
-{
-   int len = strlen(path);
-   struct strbuf submodule = STRBUF_INIT;
-   struct files_ref_store *refs;
-
-   while (len && path[len-1] == '/')
-   len--;
-   if (!len)
-   return -1;
-
-   strbuf_add(, path, len);
-   refs = get_files_ref_store(submodule.buf, "resolve_gitlink_ref");
-   if (!refs) {
-   strbuf_release();
-   return -1;
-   }
-   strbuf_release();
-
-   return resolve_gitlink_ref_recursive(refs, refname, sha1, 0);
-}
-
 /*
  * Return the ref_entry for the given refname from the packed
  * references.  If it does not exist, return NULL.
@@ -1572,6 +1489,89 @@ static void unlock_ref(struct ref_lock *lock)
free(lock);
 }
 
+#define MAXREFLEN (1024)
+
+/*
+ * Called by resolve_gitlink_ref_recursive() after it failed to read
+ * from the loose refs in refs. Find  in the packed-refs file
+ * for the submodule.
+ */
+static int resolve_gitlink_packed_ref(struct files_ref_store *refs,
+ const char *refname, unsigned char *sha1)
+{
+   struct ref_entry *ref;
+   struct ref_dir *dir = get_packed_refs(refs);
+
+   ref = find_ref(dir, refname);
+   if (ref == NULL)
+   return -1;
+
+   hashcpy(sha1, ref->u.value.oid.hash);
+   return 0;
+}
+
+static int resolve_gitlink_ref_recursive(struct files_ref_store *refs,
+const char *refname, unsigned char 
*sha1,
+int recursion)
+{
+   int fd, len;
+   char buffer[128], *p;
+   char *path;
+
+   if (recursion > SYMREF_MAXDEPTH || strlen(refname) > MAXREFLEN)
+   return -1;
+   path = *refs->base.submodule
+   ? git_pathdup_submodule(refs->base.submodule, "%s", refname)
+   : git_pathdup("%s", refname);
+   fd = open(path, O_RDONLY);
+   free(path);
+   if (fd < 0)
+   return resolve_gitlink_packed_ref(refs, refname, sha1);
+
+   len = read(fd, buffer, sizeof(buffer)-1);
+   close(fd);
+   if (len < 0)
+   return -1;
+   while (len && isspace(buffer[len-1]))
+   len--;
+   buffer[len] = 0;
+
+   /* Was it a detached head or an old-fashioned symlink? */
+   if (!get_sha1_hex(buffer, sha1))
+   return 0;
+

[PATCH v2 32/38] refs: add methods for reflog

[Michael Haggerty]

From: David Turner 

In the file-based backend, the reflog piggybacks on the ref lock.
Since other backends won't have the same sort of ref lock, ref backends
must also handle reflogs.

Signed-off-by: Ronnie Sahlberg 
Signed-off-by: David Turner 
Signed-off-by: Junio C Hamano 
Signed-off-by: Michael Haggerty 
Signed-off-by: Junio C Hamano 
---
 refs.c   | 63 ++
 refs/files-backend.c | 70 +---
 refs/refs-internal.h | 44 +++--
 3 files changed, 150 insertions(+), 27 deletions(-)

diff --git a/refs.c b/refs.c
index 798b08a..cb9d805 100644
--- a/refs.c
+++ b/refs.c
@@ -1461,3 +1461,66 @@ int verify_refname_available(const char *refname,
 
return refs->be->verify_refname_available(refs, refname, extra, skip, 
err);
 }
+
+int for_each_reflog(each_ref_fn fn, void *cb_data)
+{
+   struct ref_store *refs = get_ref_store(NULL);
+   struct ref_iterator *iter;
+
+   iter = refs->be->reflog_iterator_begin(refs);
+
+   return do_for_each_ref_iterator(iter, fn, cb_data);
+}
+
+int for_each_reflog_ent_reverse(const char *refname, each_reflog_ent_fn fn,
+   void *cb_data)
+{
+   struct ref_store *refs = get_ref_store(NULL);
+
+   return refs->be->for_each_reflog_ent_reverse(refs, refname,
+fn, cb_data);
+}
+
+int for_each_reflog_ent(const char *refname, each_reflog_ent_fn fn,
+   void *cb_data)
+{
+   struct ref_store *refs = get_ref_store(NULL);
+
+   return refs->be->for_each_reflog_ent(refs, refname, fn, cb_data);
+}
+
+int reflog_exists(const char *refname)
+{
+   struct ref_store *refs = get_ref_store(NULL);
+
+   return refs->be->reflog_exists(refs, refname);
+}
+
+int safe_create_reflog(const char *refname, int force_create,
+  struct strbuf *err)
+{
+   struct ref_store *refs = get_ref_store(NULL);
+
+   return refs->be->create_reflog(refs, refname, force_create, err);
+}
+
+int delete_reflog(const char *refname)
+{
+   struct ref_store *refs = get_ref_store(NULL);
+
+   return refs->be->delete_reflog(refs, refname);
+}
+
+int reflog_expire(const char *refname, const unsigned char *sha1,
+ unsigned int flags,
+ reflog_expiry_prepare_fn prepare_fn,
+ reflog_expiry_should_prune_fn should_prune_fn,
+ reflog_expiry_cleanup_fn cleanup_fn,
+ void *policy_cb_data)
+{
+   struct ref_store *refs = get_ref_store(NULL);
+
+   return refs->be->reflog_expire(refs, refname, sha1, flags,
+  prepare_fn, should_prune_fn,
+  cleanup_fn, policy_cb_data);
+}
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 4451e13..a810dfa 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2777,11 +2777,16 @@ static int log_ref_setup(const char *refname, struct 
strbuf *logfile, struct str
 }
 
 
-int safe_create_reflog(const char *refname, int force_create, struct strbuf 
*err)
+static int files_create_reflog(struct ref_store *ref_store,
+  const char *refname, int force_create,
+  struct strbuf *err)
 {
int ret;
struct strbuf sb = STRBUF_INIT;
 
+   /* Check validity (but we don't need the result): */
+   files_downcast(ref_store, 0, "create_reflog");
+
ret = log_ref_setup(refname, , err, force_create);
strbuf_release();
return ret;
@@ -3075,16 +3080,24 @@ int set_worktree_head_symref(const char *gitdir, const 
char *target)
return ret;
 }
 
-int reflog_exists(const char *refname)
+static int files_reflog_exists(struct ref_store *ref_store,
+  const char *refname)
 {
struct stat st;
 
+   /* Check validity (but we don't need the result): */
+   files_downcast(ref_store, 0, "reflog_exists");
+
return !lstat(git_path("logs/%s", refname), ) &&
S_ISREG(st.st_mode);
 }
 
-int delete_reflog(const char *refname)
+static int files_delete_reflog(struct ref_store *ref_store,
+  const char *refname)
 {
+   /* Check validity (but we don't need the result): */
+   files_downcast(ref_store, 0, "delete_reflog");
+
return remove_path(git_path("logs/%s", refname));
 }
 
@@ -3127,13 +3140,19 @@ static char *find_beginning_of_line(char *bob, char 
*scan)
return scan;
 }
 
-int for_each_reflog_ent_reverse(const char *refname, each_reflog_ent_fn fn, 
void *cb_data)
+static int files_for_each_reflog_ent_reverse(struct ref_store *ref_store,
+const char *refname,
+

[PATCH v2 26/38] commit_ref_update(): add a files_ref_store argument

[Michael Haggerty]

Signed-off-by: Michael Haggerty 
Signed-off-by: Junio C Hamano 
---
 refs/files-backend.c | 14 --
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index ae425c5..001b8ef 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2574,12 +2574,14 @@ static int files_verify_refname_available(struct 
ref_store *ref_store,
 
 static int write_ref_to_lockfile(struct ref_lock *lock,
 const unsigned char *sha1, struct strbuf *err);
-static int commit_ref_update(struct ref_lock *lock,
+static int commit_ref_update(struct files_ref_store *refs,
+struct ref_lock *lock,
 const unsigned char *sha1, const char *logmsg,
 struct strbuf *err);
 
 int rename_ref(const char *oldrefname, const char *newrefname, const char 
*logmsg)
 {
+   struct files_ref_store *refs = get_files_ref_store(NULL, "rename_ref");
unsigned char sha1[20], orig_sha1[20];
int flag = 0, logmoved = 0;
struct ref_lock *lock;
@@ -2652,7 +2654,7 @@ int rename_ref(const char *oldrefname, const char 
*newrefname, const char *logms
hashcpy(lock->old_oid.hash, orig_sha1);
 
if (write_ref_to_lockfile(lock, orig_sha1, ) ||
-   commit_ref_update(lock, orig_sha1, logmsg, )) {
+   commit_ref_update(refs, lock, orig_sha1, logmsg, )) {
error("unable to write current sha1 into %s: %s", newrefname, 
err.buf);
strbuf_release();
goto rollback;
@@ -2672,7 +2674,7 @@ int rename_ref(const char *oldrefname, const char 
*newrefname, const char *logms
flag = log_all_ref_updates;
log_all_ref_updates = 0;
if (write_ref_to_lockfile(lock, orig_sha1, ) ||
-   commit_ref_update(lock, orig_sha1, NULL, )) {
+   commit_ref_update(refs, lock, orig_sha1, NULL, )) {
error("unable to write current sha1 into %s: %s", oldrefname, 
err.buf);
strbuf_release();
}
@@ -2908,12 +2910,12 @@ static int write_ref_to_lockfile(struct ref_lock *lock,
  * to the loose reference lockfile. Also update the reflogs if
  * necessary, using the specified lockmsg (which can be NULL).
  */
-static int commit_ref_update(struct ref_lock *lock,
+static int commit_ref_update(struct files_ref_store *refs,
+struct ref_lock *lock,
 const unsigned char *sha1, const char *logmsg,
 struct strbuf *err)
 {
-   struct files_ref_store *refs =
-   get_files_ref_store(NULL, "commit_ref_update");
+   assert_main_repository(>base, "commit_ref_update");
 
clear_loose_ref_cache(refs);
if (log_ref_write(lock->ref_name, lock->old_oid.hash, sha1, logmsg, 0, 
err)) {
-- 
2.9.3

[PATCH v2 15/38] resolve_ref_recursively(): new function

[Michael Haggerty]

Add a new function, resolve_ref_recursively(), which is basically like
the old resolve_ref_unsafe() except that it takes a (ref_store *)
argument and also works for submodules.

Re-implement resolve_ref_unsafe() as a thin wrapper around
resolve_ref_recursively().

Signed-off-by: Michael Haggerty 
Signed-off-by: Junio C Hamano 
---
 refs.c | 14 +++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/refs.c b/refs.c
index 9e6b005..3723169 100644
--- a/refs.c
+++ b/refs.c
@@ -1216,13 +1216,14 @@ int for_each_rawref(each_ref_fn fn, void *cb_data)
 }
 
 /* This function needs to return a meaningful errno on failure */
-const char *resolve_ref_unsafe(const char *refname, int resolve_flags,
-  unsigned char *sha1, int *flags)
+static const char *resolve_ref_recursively(struct ref_store *refs,
+  const char *refname,
+  int resolve_flags,
+  unsigned char *sha1, int *flags)
 {
static struct strbuf sb_refname = STRBUF_INIT;
int unused_flags;
int symref_count;
-   struct ref_store *refs = get_ref_store(NULL);
 
if (!flags)
flags = _flags;
@@ -1291,6 +1292,13 @@ const char *resolve_ref_unsafe(const char *refname, int 
resolve_flags,
return NULL;
 }
 
+const char *resolve_ref_unsafe(const char *refname, int resolve_flags,
+  unsigned char *sha1, int *flags)
+{
+   return resolve_ref_recursively(get_ref_store(NULL), refname,
+  resolve_flags, sha1, flags);
+}
+
 /* A pointer to the ref_store for the main repository: */
 static struct ref_store *main_ref_store;
 
-- 
2.9.3

[PATCH v2 12/38] resolve_packed_ref(): rename function from resolve_missing_loose_ref()

[Michael Haggerty]

Signed-off-by: Michael Haggerty 
Signed-off-by: Junio C Hamano 
---
 refs/files-backend.c | 11 +--
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 7673342..32ca5ff 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1329,10 +1329,9 @@ static struct ref_entry *get_packed_ref(struct 
files_ref_store *refs,
 /*
  * A loose ref file doesn't exist; check for a packed ref.
  */
-static int resolve_missing_loose_ref(struct files_ref_store *refs,
-const char *refname,
-unsigned char *sha1,
-unsigned int *flags)
+static int resolve_packed_ref(struct files_ref_store *refs,
+ const char *refname,
+ unsigned char *sha1, unsigned int *flags)
 {
struct ref_entry *entry;
 
@@ -1383,7 +1382,7 @@ int read_raw_ref(const char *refname, unsigned char *sha1,
if (lstat(path, ) < 0) {
if (errno != ENOENT)
goto out;
-   if (resolve_missing_loose_ref(refs, refname, sha1, type)) {
+   if (resolve_packed_ref(refs, refname, sha1, type)) {
errno = ENOENT;
goto out;
}
@@ -1417,7 +1416,7 @@ int read_raw_ref(const char *refname, unsigned char *sha1,
 * ref is supposed to be, there could still be a
 * packed ref:
 */
-   if (resolve_missing_loose_ref(refs, refname, sha1, type)) {
+   if (resolve_packed_ref(refs, refname, sha1, type)) {
errno = EISDIR;
goto out;
}
-- 
2.9.3

[PATCH v2 17/38] resolve_gitlink_ref(): avoid memory allocation in many cases

[Michael Haggerty]

If we don't have to strip trailing '/' from the submodule path, then
don't allocate and copy the submodule name.

Signed-off-by: Michael Haggerty 
Signed-off-by: Junio C Hamano 
---
 refs.c | 19 +--
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/refs.c b/refs.c
index 647ead5..34c0c5e 100644
--- a/refs.c
+++ b/refs.c
@@ -1301,19 +1301,26 @@ const char *resolve_ref_unsafe(const char *refname, int 
resolve_flags,
 
 int resolve_gitlink_ref(const char *path, const char *refname, unsigned char 
*sha1)
 {
-   int len = strlen(path);
-   struct strbuf submodule = STRBUF_INIT;
+   size_t len = strlen(path);
struct ref_store *refs;
int flags;
 
-   while (len && path[len-1] == '/')
+   while (len && path[len - 1] == '/')
len--;
+
if (!len)
return -1;
 
-   strbuf_add(, path, len);
-   refs = get_ref_store(submodule.buf);
-   strbuf_release();
+   if (path[len]) {
+   /* We need to strip off one or more trailing slashes */
+   char *stripped = xmemdupz(path, len);
+
+   refs = get_ref_store(stripped);
+   free(stripped);
+   } else {
+   refs = get_ref_store(path);
+   }
+
if (!refs)
return -1;
 
-- 
2.9.3

[PATCH v2 36/38] refs: add method to rename refs

[Michael Haggerty]

From: David Turner 

This removes the last caller of function get_files_ref_store(), so
remove it.

Signed-off-by: David Turner 
Signed-off-by: Junio C Hamano 
Signed-off-by: Michael Haggerty 
Signed-off-by: Junio C Hamano 
---
 refs.c   |  7 +++
 refs/files-backend.c | 24 ++--
 refs/refs-internal.h |  4 
 3 files changed, 17 insertions(+), 18 deletions(-)

diff --git a/refs.c b/refs.c
index fcaf3ba..113e3c8 100644
--- a/refs.c
+++ b/refs.c
@@ -1547,3 +1547,10 @@ int delete_refs(struct string_list *refnames, unsigned 
int flags)
 
return refs->be->delete_refs(refs, refnames, flags);
 }
+
+int rename_ref(const char *oldref, const char *newref, const char *logmsg)
+{
+   struct ref_store *refs = get_ref_store(NULL);
+
+   return refs->be->rename_ref(refs, oldref, newref, logmsg);
+}
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 56397af..7cc4191 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -997,22 +997,6 @@ static struct files_ref_store *files_downcast(
return (struct files_ref_store *)ref_store;
 }
 
-/*
- * Return a pointer to the reference store for the specified
- * submodule. For the main repository, use submodule==NULL; such a
- * call cannot fail. For a submodule, the submodule must exist and be
- * a nonbare repository, otherwise return NULL. Verify that the
- * reference store is a files_ref_store, and cast it to that type
- * before returning it.
- */
-static struct files_ref_store *get_files_ref_store(const char *submodule,
-  const char *caller)
-{
-   struct ref_store *refs = get_ref_store(submodule);
-
-   return refs ? files_downcast(refs, 1, caller) : NULL;
-}
-
 /* The length of a peeled reference line in packed-refs, including EOL: */
 #define PEELED_LINE_LENGTH 42
 
@@ -2580,9 +2564,12 @@ static int commit_ref_update(struct files_ref_store 
*refs,
 const unsigned char *sha1, const char *logmsg,
 struct strbuf *err);
 
-int rename_ref(const char *oldrefname, const char *newrefname, const char 
*logmsg)
+static int files_rename_ref(struct ref_store *ref_store,
+   const char *oldrefname, const char *newrefname,
+   const char *logmsg)
 {
-   struct files_ref_store *refs = get_files_ref_store(NULL, "rename_ref");
+   struct files_ref_store *refs =
+   files_downcast(ref_store, 0, "rename_ref");
unsigned char sha1[20], orig_sha1[20];
int flag = 0, logmoved = 0;
struct ref_lock *lock;
@@ -4097,6 +4084,7 @@ struct ref_storage_be refs_be_files = {
files_peel_ref,
files_create_symref,
files_delete_refs,
+   files_rename_ref,
 
files_ref_iterator_begin,
files_read_raw_ref,
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index b3a2095..c598cb1 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -494,6 +494,9 @@ typedef int create_symref_fn(struct ref_store *ref_store,
 const char *logmsg);
 typedef int delete_refs_fn(struct ref_store *ref_store,
   struct string_list *refnames, unsigned int flags);
+typedef int rename_ref_fn(struct ref_store *ref_store,
+ const char *oldref, const char *newref,
+ const char *logmsg);
 
 /*
  * Iterate over the references in the specified ref_store that are
@@ -593,6 +596,7 @@ struct ref_storage_be {
peel_ref_fn *peel_ref;
create_symref_fn *create_symref;
delete_refs_fn *delete_refs;
+   rename_ref_fn *rename_ref;
 
ref_iterator_begin_fn *iterator_begin;
read_raw_ref_fn *read_raw_ref;
-- 
2.9.3

[PATCH v2 34/38] refs: make delete_refs() virtual

[Michael Haggerty]

From: David Turner 

In the file-based backend, delete_refs has some special optimization
to deal with packed refs.  In other backends, we might be able to make
ref deletion faster by putting all deletions into a single
transaction.  So we need a special backend function for this.

Signed-off-by: David Turner 
Signed-off-by: Junio C Hamano 
Signed-off-by: Michael Haggerty 
Signed-off-by: Junio C Hamano 
---
 refs.c   | 7 +++
 refs/files-backend.c | 6 --
 refs/refs-internal.h | 3 +++
 3 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/refs.c b/refs.c
index 3830918..6efa859 100644
--- a/refs.c
+++ b/refs.c
@@ -1532,3 +1532,10 @@ int initial_ref_transaction_commit(struct 
ref_transaction *transaction,
 
return refs->be->initial_transaction_commit(refs, transaction, err);
 }
+
+int delete_refs(struct string_list *refnames, unsigned int flags)
+{
+   struct ref_store *refs = get_ref_store(NULL);
+
+   return refs->be->delete_refs(refs, refnames, flags);
+}
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 81008d7..bcaa958 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2451,10 +2451,11 @@ static int delete_ref_loose(struct ref_lock *lock, int 
flag, struct strbuf *err)
return 0;
 }
 
-int delete_refs(struct string_list *refnames, unsigned int flags)
+static int files_delete_refs(struct ref_store *ref_store,
+struct string_list *refnames, unsigned int flags)
 {
struct files_ref_store *refs =
-   get_files_ref_store(NULL, "delete_refs");
+   files_downcast(ref_store, 0, "delete_refs");
struct strbuf err = STRBUF_INIT;
int i, result = 0;
 
@@ -4077,6 +4078,7 @@ struct ref_storage_be refs_be_files = {
files_pack_refs,
files_peel_ref,
files_create_symref,
+   files_delete_refs,
 
files_ref_iterator_begin,
files_read_raw_ref,
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 08c8586..ade6501 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -490,6 +490,8 @@ typedef int create_symref_fn(struct ref_store *ref_store,
 const char *ref_target,
 const char *refs_heads_master,
 const char *logmsg);
+typedef int delete_refs_fn(struct ref_store *ref_store,
+  struct string_list *refnames, unsigned int flags);
 
 /*
  * Iterate over the references in the specified ref_store that are
@@ -587,6 +589,7 @@ struct ref_storage_be {
pack_refs_fn *pack_refs;
peel_ref_fn *peel_ref;
create_symref_fn *create_symref;
+   delete_refs_fn *delete_refs;
 
ref_iterator_begin_fn *iterator_begin;
read_raw_ref_fn *read_raw_ref;
-- 
2.9.3

[PATCH v2 27/38] lock_ref_for_update(): add a files_ref_store argument

[Michael Haggerty]

Signed-off-by: Michael Haggerty 
Signed-off-by: Junio C Hamano 
---
 refs/files-backend.c | 11 ++-
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 001b8ef..2f8eb54 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3514,20 +3514,21 @@ static int check_old_oid(struct ref_update *update, 
struct object_id *oid,
  * - If it is an update of head_ref, add a corresponding REF_LOG_ONLY
  *   update of HEAD.
  */
-static int lock_ref_for_update(struct ref_update *update,
+static int lock_ref_for_update(struct files_ref_store *refs,
+  struct ref_update *update,
   struct ref_transaction *transaction,
   const char *head_ref,
   struct string_list *affected_refnames,
   struct strbuf *err)
 {
-   struct files_ref_store *refs =
-   get_files_ref_store(NULL, "lock_ref_for_update");
struct strbuf referent = STRBUF_INIT;
int mustexist = (update->flags & REF_HAVE_OLD) &&
!is_null_sha1(update->old_sha1);
int ret;
struct ref_lock *lock;
 
+   assert_main_repository(>base, "lock_ref_for_update");
+
if ((update->flags & REF_HAVE_NEW) && is_null_sha1(update->new_sha1))
update->flags |= REF_DELETING;
 
@@ -3730,8 +3731,8 @@ static int files_transaction_commit(struct ref_store 
*ref_store,
for (i = 0; i < transaction->nr; i++) {
struct ref_update *update = transaction->updates[i];
 
-   ret = lock_ref_for_update(update, transaction, head_ref,
- _refnames, err);
+   ret = lock_ref_for_update(refs, update, transaction,
+ head_ref, _refnames, err);
if (ret)
goto cleanup;
}
-- 
2.9.3

[PATCH v2 13/38] resolve_gitlink_packed_ref(): remove function

[Michael Haggerty]

Now that resolve_packed_ref() can work with an arbitrary
files_ref_store, there is no need to have a separate
resolve_gitlink_packed_ref() function.

Signed-off-by: Michael Haggerty 
Signed-off-by: Junio C Hamano 
---
 refs/files-backend.c | 26 +-
 1 file changed, 5 insertions(+), 21 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 32ca5ff..3b0c837 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1490,25 +1490,6 @@ static void unlock_ref(struct ref_lock *lock)
 
 #define MAXREFLEN (1024)
 
-/*
- * Called by resolve_gitlink_ref_recursive() after it failed to read
- * from the loose refs in refs. Find  in the packed-refs file
- * for the submodule.
- */
-static int resolve_gitlink_packed_ref(struct files_ref_store *refs,
- const char *refname, unsigned char *sha1)
-{
-   struct ref_entry *ref;
-   struct ref_dir *dir = get_packed_refs(refs);
-
-   ref = find_ref(dir, refname);
-   if (ref == NULL)
-   return -1;
-
-   hashcpy(sha1, ref->u.value.oid.hash);
-   return 0;
-}
-
 static int resolve_gitlink_ref_recursive(struct files_ref_store *refs,
 const char *refname, unsigned char 
*sha1,
 int recursion)
@@ -1524,8 +1505,11 @@ static int resolve_gitlink_ref_recursive(struct 
files_ref_store *refs,
: git_pathdup("%s", refname);
fd = open(path, O_RDONLY);
free(path);
-   if (fd < 0)
-   return resolve_gitlink_packed_ref(refs, refname, sha1);
+   if (fd < 0) {
+   unsigned int flags;
+
+   return resolve_packed_ref(refs, refname, sha1, );
+   }
 
len = read(fd, buffer, sizeof(buffer)-1);
close(fd);
-- 
2.9.3

[PATCH v2 09/38] {lock,commit,rollback}_packed_refs(): add files_ref_store arguments

[Michael Haggerty]

These functions currently only work in the main repository, so add an
assert_main_repository() check to each function.

Signed-off-by: Michael Haggerty 
Signed-off-by: Junio C Hamano 
---
 refs/files-backend.c | 32 
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 0c92ace..6913d45 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2215,14 +2215,14 @@ static int write_packed_entry_fn(struct ref_entry 
*entry, void *cb_data)
  * hold_lock_file_for_update(). Return 0 on success. On errors, set
  * errno appropriately and return a nonzero value.
  */
-static int lock_packed_refs(int flags)
+static int lock_packed_refs(struct files_ref_store *refs, int flags)
 {
-   struct files_ref_store *refs =
-   get_files_ref_store(NULL, "lock_packed_refs");
static int timeout_configured = 0;
static int timeout_value = 1000;
struct packed_ref_cache *packed_ref_cache;
 
+   assert_main_repository(>base, "lock_packed_refs");
+
if (!timeout_configured) {
git_config_get_int("core.packedrefstimeout", _value);
timeout_configured = 1;
@@ -2251,16 +2251,16 @@ static int lock_packed_refs(int flags)
  * lock_packed_refs()). Return zero on success. On errors, set errno
  * and return a nonzero value
  */
-static int commit_packed_refs(void)
+static int commit_packed_refs(struct files_ref_store *refs)
 {
-   struct files_ref_store *refs =
-   get_files_ref_store(NULL, "commit_packed_refs");
struct packed_ref_cache *packed_ref_cache =
get_packed_ref_cache(refs);
int error = 0;
int save_errno = 0;
FILE *out;
 
+   assert_main_repository(>base, "commit_packed_refs");
+
if (!packed_ref_cache->lock)
die("internal error: packed-refs not locked");
 
@@ -2287,13 +2287,13 @@ static int commit_packed_refs(void)
  * in-memory packed reference cache.  (The packed-refs file will be
  * read anew if it is needed again after this function is called.)
  */
-static void rollback_packed_refs(void)
+static void rollback_packed_refs(struct files_ref_store *refs)
 {
-   struct files_ref_store *refs =
-   get_files_ref_store(NULL, "rollback_packed_refs");
struct packed_ref_cache *packed_ref_cache =
get_packed_ref_cache(refs);
 
+   assert_main_repository(>base, "rollback_packed_refs");
+
if (!packed_ref_cache->lock)
die("internal error: packed-refs not locked");
rollback_lock_file(packed_ref_cache->lock);
@@ -2439,13 +2439,13 @@ int pack_refs(unsigned int flags)
memset(, 0, sizeof(cbdata));
cbdata.flags = flags;
 
-   lock_packed_refs(LOCK_DIE_ON_ERROR);
+   lock_packed_refs(refs, LOCK_DIE_ON_ERROR);
cbdata.packed_refs = get_packed_refs(refs);
 
do_for_each_entry_in_dir(get_loose_refs(refs), 0,
 pack_if_possible_fn, );
 
-   if (commit_packed_refs())
+   if (commit_packed_refs(refs))
die_errno("unable to overwrite old ref-pack file");
 
prune_refs(cbdata.ref_to_prune);
@@ -2481,7 +2481,7 @@ static int repack_without_refs(struct string_list 
*refnames, struct strbuf *err)
if (!needs_repacking)
return 0; /* no refname exists in packed refs */
 
-   if (lock_packed_refs(0)) {
+   if (lock_packed_refs(refs, 0)) {
unable_to_lock_message(git_path("packed-refs"), errno, err);
return -1;
}
@@ -2496,12 +2496,12 @@ static int repack_without_refs(struct string_list 
*refnames, struct strbuf *err)
 * All packed entries disappeared while we were
 * acquiring the lock.
 */
-   rollback_packed_refs();
+   rollback_packed_refs(refs);
return 0;
}
 
/* Write what remains */
-   ret = commit_packed_refs();
+   ret = commit_packed_refs(refs);
if (ret)
strbuf_addf(err, "unable to overwrite old ref-pack file: %s",
strerror(errno));
@@ -3929,7 +3929,7 @@ int initial_ref_transaction_commit(struct ref_transaction 
*transaction,
}
}
 
-   if (lock_packed_refs(0)) {
+   if (lock_packed_refs(refs, 0)) {
strbuf_addf(err, "unable to lock packed-refs file: %s",
strerror(errno));
ret = TRANSACTION_GENERIC_ERROR;
@@ -3944,7 +3944,7 @@ int initial_ref_transaction_commit(struct ref_transaction 
*transaction,
add_packed_ref(refs, update->refname, update->new_sha1);
}
 
-   if (commit_packed_refs()) {
+   if (commit_packed_refs(refs)) {
strbuf_addf(err, "unable to commit packed-refs file: %s",
   

[PATCH v2 23/38] refs: make peel_ref() virtual

[Michael Haggerty]

For now it only supports the main reference store.

Signed-off-by: Michael Haggerty 
Signed-off-by: Junio C Hamano 
---
 refs.c   | 7 +++
 refs/files-backend.c | 6 --
 refs/refs-internal.h | 3 +++
 3 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/refs.c b/refs.c
index 74874e5..f10f86a 100644
--- a/refs.c
+++ b/refs.c
@@ -1428,6 +1428,13 @@ int pack_refs(unsigned int flags)
return refs->be->pack_refs(refs, flags);
 }
 
+int peel_ref(const char *refname, unsigned char *sha1)
+{
+   struct ref_store *refs = get_ref_store(NULL);
+
+   return refs->be->peel_ref(refs, refname, sha1);
+}
+
 int create_symref(const char *ref_target, const char *refs_heads_master,
  const char *logmsg)
 {
diff --git a/refs/files-backend.c b/refs/files-backend.c
index bf53670..8d43e0b 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1759,9 +1759,10 @@ static enum peel_status peel_entry(struct ref_entry 
*entry, int repeel)
return status;
 }
 
-int peel_ref(const char *refname, unsigned char *sha1)
+static int files_peel_ref(struct ref_store *ref_store,
+ const char *refname, unsigned char *sha1)
 {
-   struct files_ref_store *refs = get_files_ref_store(NULL, "peel_ref");
+   struct files_ref_store *refs = files_downcast(ref_store, 0, "peel_ref");
int flag;
unsigned char base[20];
 
@@ -4037,6 +4038,7 @@ struct ref_storage_be refs_be_files = {
files_transaction_commit,
 
files_pack_refs,
+   files_peel_ref,
files_create_symref,
 
files_read_raw_ref,
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index bf96503..84c81ad 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -501,6 +501,8 @@ typedef int ref_transaction_commit_fn(struct ref_store 
*refs,
  struct strbuf *err);
 
 typedef int pack_refs_fn(struct ref_store *ref_store, unsigned int flags);
+typedef int peel_ref_fn(struct ref_store *ref_store,
+   const char *refname, unsigned char *sha1);
 typedef int create_symref_fn(struct ref_store *ref_store,
 const char *ref_target,
 const char *refs_heads_master,
@@ -561,6 +563,7 @@ struct ref_storage_be {
ref_transaction_commit_fn *transaction_commit;
 
pack_refs_fn *pack_refs;
+   peel_ref_fn *peel_ref;
create_symref_fn *create_symref;
 
read_raw_ref_fn *read_raw_ref;
-- 
2.9.3

[PATCH v2 05/38] refs: create a base class "ref_store" for files_ref_store

[Michael Haggerty]

We want ref_stores to be polymorphic, so invent a base class of which
files_ref_store is a derived class. For now there is exactly one
ref_store for the main repository and one for any submodules whose
references have been accessed.

Signed-off-by: Michael Haggerty 
Signed-off-by: Junio C Hamano 
---
 refs.c   |  93 +++
 refs/files-backend.c | 177 ---
 refs/refs-internal.h |  78 +++
 3 files changed, 270 insertions(+), 78 deletions(-)

diff --git a/refs.c b/refs.c
index d2a29bb..9d6bcb1 100644
--- a/refs.c
+++ b/refs.c
@@ -1151,8 +1151,12 @@ int head_ref(each_ref_fn fn, void *cb_data)
 static int do_for_each_ref(const char *submodule, const char *prefix,
   each_ref_fn fn, int trim, int flags, void *cb_data)
 {
+   struct ref_store *refs = get_ref_store(submodule);
struct ref_iterator *iter;
 
+   if (!refs)
+   return 0;
+
iter = files_ref_iterator_begin(submodule, prefix, flags);
iter = prefix_ref_iterator_begin(iter, prefix, trim);
 
@@ -1284,3 +1288,92 @@ const char *resolve_ref_unsafe(const char *refname, int 
resolve_flags,
errno = ELOOP;
return NULL;
 }
+
+/* A pointer to the ref_store for the main repository: */
+static struct ref_store *main_ref_store;
+
+/* A linked list of ref_stores for submodules: */
+static struct ref_store *submodule_ref_stores;
+
+void base_ref_store_init(struct ref_store *refs,
+const struct ref_storage_be *be,
+const char *submodule)
+{
+   refs->be = be;
+   if (!submodule) {
+   if (main_ref_store)
+   die("BUG: main_ref_store initialized twice");
+
+   refs->submodule = "";
+   refs->next = NULL;
+   main_ref_store = refs;
+   } else {
+   if (lookup_ref_store(submodule))
+   die("BUG: ref_store for submodule '%s' initialized 
twice",
+   submodule);
+
+   refs->submodule = xstrdup(submodule);
+   refs->next = submodule_ref_stores;
+   submodule_ref_stores = refs;
+   }
+}
+
+struct ref_store *ref_store_init(const char *submodule)
+{
+   const char *be_name = "files";
+   struct ref_storage_be *be = find_ref_storage_backend(be_name);
+
+   if (!be)
+   die("BUG: reference backend %s is unknown", be_name);
+
+   if (!submodule || !*submodule)
+   return be->init(NULL);
+   else
+   return be->init(submodule);
+}
+
+struct ref_store *lookup_ref_store(const char *submodule)
+{
+   struct ref_store *refs;
+
+   if (!submodule || !*submodule)
+   return main_ref_store;
+
+   for (refs = submodule_ref_stores; refs; refs = refs->next) {
+   if (!strcmp(submodule, refs->submodule))
+   return refs;
+   }
+
+   return NULL;
+}
+
+struct ref_store *get_ref_store(const char *submodule)
+{
+   struct ref_store *refs;
+
+   if (!submodule || !*submodule) {
+   refs = lookup_ref_store(NULL);
+
+   if (!refs)
+   refs = ref_store_init(NULL);
+   } else {
+   refs = lookup_ref_store(submodule);
+
+   if (!refs) {
+   struct strbuf submodule_sb = STRBUF_INIT;
+
+   strbuf_addstr(_sb, submodule);
+   if (is_nonbare_repository_dir(_sb))
+   refs = ref_store_init(submodule);
+   strbuf_release(_sb);
+   }
+   }
+
+   return refs;
+}
+
+void assert_main_repository(struct ref_store *refs, const char *caller)
+{
+   if (*refs->submodule)
+   die("BUG: %s called for a submodule", caller);
+}
diff --git a/refs/files-backend.c b/refs/files-backend.c
index ecf66e6..439c500 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -910,17 +910,11 @@ struct packed_ref_cache {
  * Future: need to be in "struct repository"
  * when doing a full libification.
  */
-static struct files_ref_store {
-   struct files_ref_store *next;
+struct files_ref_store {
+   struct ref_store base;
struct ref_entry *loose;
struct packed_ref_cache *packed;
-   /*
-* The submodule name, or "" for the main repo. We allocate
-* length 1 rather than FLEX_ARRAY so that the main
-* files_ref_store is initialized correctly.
-*/
-   char name[1];
-} ref_store, *submodule_ref_stores;
+};
 
 /* Lock used for the main packed-refs file: */
 static struct lock_file packlock;
@@ -973,53 +967,50 @@ static void clear_loose_ref_cache(struct files_ref_store 
*refs)
  * Create a new submodule ref cache and add it to the internal
  * set of caches.
  */
-static struct files_ref_store 

[PATCH v2 37/38] refs: make lock generic

[Michael Haggerty]

From: David Turner 

Instead of including a files-backend-specific struct ref_lock, change
the generic ref_update struct to include a void pointer that backends
can use for their own arbitrary data.

Signed-off-by: David Turner 
Signed-off-by: Junio C Hamano 
Signed-off-by: Michael Haggerty 
Signed-off-by: Junio C Hamano 
---
 refs/files-backend.c | 25 +
 refs/refs-internal.h |  2 +-
 2 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 7cc4191..0714c3f 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3552,9 +3552,8 @@ static int lock_ref_for_update(struct files_ref_store 
*refs,
 
ret = lock_raw_ref(refs, update->refname, mustexist,
   affected_refnames, NULL,
-  >lock, ,
+  , ,
   >type, err);
-
if (ret) {
char *reason;
 
@@ -3565,7 +3564,7 @@ static int lock_ref_for_update(struct files_ref_store 
*refs,
return ret;
}
 
-   lock = update->lock;
+   update->backend_data = lock;
 
if (update->type & REF_ISSYMREF) {
if (update->flags & REF_NODEREF) {
@@ -3613,7 +3612,8 @@ static int lock_ref_for_update(struct files_ref_store 
*refs,
for (parent_update = update->parent_update;
 parent_update;
 parent_update = parent_update->parent_update) {
-   oidcpy(_update->lock->old_oid, >old_oid);
+   struct ref_lock *parent_lock = 
parent_update->backend_data;
+   oidcpy(_lock->old_oid, >old_oid);
}
}
 
@@ -3634,7 +3634,7 @@ static int lock_ref_for_update(struct files_ref_store 
*refs,
 * The lock was freed upon failure of
 * write_ref_to_lockfile():
 */
-   update->lock = NULL;
+   update->backend_data = NULL;
strbuf_addf(err,
"cannot update ref '%s': %s",
update->refname, write_err);
@@ -3752,7 +3752,7 @@ static int files_transaction_commit(struct ref_store 
*ref_store,
/* Perform updates first so live commits remain referenced */
for (i = 0; i < transaction->nr; i++) {
struct ref_update *update = transaction->updates[i];
-   struct ref_lock *lock = update->lock;
+   struct ref_lock *lock = update->backend_data;
 
if (update->flags & REF_NEEDS_COMMIT ||
update->flags & REF_LOG_ONLY) {
@@ -3765,7 +3765,7 @@ static int files_transaction_commit(struct ref_store 
*ref_store,
lock->ref_name, old_msg);
free(old_msg);
unlock_ref(lock);
-   update->lock = NULL;
+   update->backend_data = NULL;
ret = TRANSACTION_GENERIC_ERROR;
goto cleanup;
}
@@ -3775,7 +3775,7 @@ static int files_transaction_commit(struct ref_store 
*ref_store,
if (commit_ref(lock)) {
strbuf_addf(err, "couldn't set '%s'", 
lock->ref_name);
unlock_ref(lock);
-   update->lock = NULL;
+   update->backend_data = NULL;
ret = TRANSACTION_GENERIC_ERROR;
goto cleanup;
}
@@ -3784,17 +3784,18 @@ static int files_transaction_commit(struct ref_store 
*ref_store,
/* Perform deletes now that updates are safely completed */
for (i = 0; i < transaction->nr; i++) {
struct ref_update *update = transaction->updates[i];
+   struct ref_lock *lock = update->backend_data;
 
if (update->flags & REF_DELETING &&
!(update->flags & REF_LOG_ONLY)) {
-   if (delete_ref_loose(update->lock, update->type, err)) {
+   if (delete_ref_loose(lock, update->type, err)) {
ret = TRANSACTION_GENERIC_ERROR;
goto cleanup;
}
 
if (!(update->flags & REF_ISPRUNING))
string_list_append(_to_delete,
-  update->lock->ref_name);
+  lock->ref_name);
}
}
 
@@ -3810,8 +3811,8 @@ static int files_transaction_commit(struct ref_store 

[PATCH v2 35/38] refs: add methods to init refs db

[Michael Haggerty]

From: David Turner 

Alternate refs backends might not need the refs/heads directory and so
on, so we make ref db initialization part of the backend.

Signed-off-by: David Turner 
Signed-off-by: Junio C Hamano 
Signed-off-by: Michael Haggerty 
Signed-off-by: Junio C Hamano 
---
 builtin/init-db.c| 21 +++--
 refs.c   |  8 
 refs.h   |  2 ++
 refs/files-backend.c | 18 ++
 refs/refs-internal.h |  3 +++
 5 files changed, 42 insertions(+), 10 deletions(-)

diff --git a/builtin/init-db.c b/builtin/init-db.c
index 3a45f0b..80192f6 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -180,13 +180,7 @@ static int create_default_files(const char *template_path)
char junk[2];
int reinit;
int filemode;
-
-   /*
-* Create .git/refs/{heads,tags}
-*/
-   safe_create_dir(git_path_buf(, "refs"), 1);
-   safe_create_dir(git_path_buf(, "refs/heads"), 1);
-   safe_create_dir(git_path_buf(, "refs/tags"), 1);
+   struct strbuf err = STRBUF_INIT;
 
/* Just look for `init.templatedir` */
git_config(git_init_db_config, NULL);
@@ -210,12 +204,19 @@ static int create_default_files(const char *template_path)
 */
if (get_shared_repository()) {
adjust_shared_perm(get_git_dir());
-   adjust_shared_perm(git_path_buf(, "refs"));
-   adjust_shared_perm(git_path_buf(, "refs/heads"));
-   adjust_shared_perm(git_path_buf(, "refs/tags"));
}
 
/*
+* We need to create a "refs" dir in any case so that older
+* versions of git can tell that this is a repository.
+*/
+   safe_create_dir(git_path("refs"), 1);
+   adjust_shared_perm(git_path("refs"));
+
+   if (refs_init_db())
+   die("failed to set up refs db: %s", err.buf);
+
+   /*
 * Create the default symlink from ".git/HEAD" to the "master"
 * branch, if it does not exist yet.
 */
diff --git a/refs.c b/refs.c
index 6efa859..fcaf3ba 100644
--- a/refs.c
+++ b/refs.c
@@ -1292,6 +1292,14 @@ static const char *resolve_ref_recursively(struct 
ref_store *refs,
return NULL;
 }
 
+/* backend functions */
+int refs_init_db(struct strbuf *err)
+{
+   struct ref_store *refs = get_ref_store(NULL);
+
+   return refs->be->init_db(refs, err);
+}
+
 const char *resolve_ref_unsafe(const char *refname, int resolve_flags,
   unsigned char *sha1, int *flags)
 {
diff --git a/refs.h b/refs.h
index 17e8cfb..d42458c 100644
--- a/refs.h
+++ b/refs.h
@@ -66,6 +66,8 @@ int ref_exists(const char *refname);
 
 int is_branch(const char *refname);
 
+extern int refs_init_db(struct strbuf *err);
+
 /*
  * If refname is a non-symbolic reference that refers to a tag object,
  * and the tag can be (recursively) dereferenced to a non-tag object,
diff --git a/refs/files-backend.c b/refs/files-backend.c
index bcaa958..56397af 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -4068,10 +4068,28 @@ static int files_reflog_expire(struct ref_store 
*ref_store,
return -1;
 }
 
+static int files_init_db(struct ref_store *ref_store, struct strbuf *err)
+{
+   /* Check validity (but we don't need the result): */
+   files_downcast(ref_store, 0, "init_db");
+
+   /*
+* Create .git/refs/{heads,tags}
+*/
+   safe_create_dir(git_path("refs/heads"), 1);
+   safe_create_dir(git_path("refs/tags"), 1);
+   if (get_shared_repository()) {
+   adjust_shared_perm(git_path("refs/heads"));
+   adjust_shared_perm(git_path("refs/tags"));
+   }
+   return 0;
+}
+
 struct ref_storage_be refs_be_files = {
NULL,
"files",
files_ref_store_create,
+   files_init_db,
files_transaction_commit,
files_initial_transaction_commit,
 
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index ade6501..b3a2095 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -479,6 +479,8 @@ struct ref_store;
  */
 typedef struct ref_store *ref_store_init_fn(const char *submodule);
 
+typedef int ref_init_db_fn(struct ref_store *refs, struct strbuf *err);
+
 typedef int ref_transaction_commit_fn(struct ref_store *refs,
  struct ref_transaction *transaction,
  struct strbuf *err);
@@ -583,6 +585,7 @@ struct ref_storage_be {
struct ref_storage_be *next;
const char *name;
ref_store_init_fn *init;
+   ref_init_db_fn *init_db;
ref_transaction_commit_fn *transaction_commit;
ref_transaction_commit_fn *initial_transaction_commit;
 
-- 
2.9.3

[PATCH v2 38/38] refs: implement iteration over only per-worktree refs

[Michael Haggerty]

From: David Turner 

Alternate refs backends might still use files to store per-worktree
refs. So provide a way to iterate over only the per-worktree references
in a ref_store. The other backend can set up a files ref_store and
iterate using the new DO_FOR_EACH_PER_WORKTREE_ONLY flag when iterating.

Signed-off-by: David Turner 
Signed-off-by: Junio C Hamano 
Signed-off-by: Michael Haggerty 
Signed-off-by: Junio C Hamano 
---
 refs/files-backend.c |  4 
 refs/refs-internal.h | 10 +-
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 0714c3f..2644792 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1798,6 +1798,10 @@ static int files_ref_iterator_advance(struct 
ref_iterator *ref_iterator)
int ok;
 
while ((ok = ref_iterator_advance(iter->iter0)) == ITER_OK) {
+   if (iter->flags & DO_FOR_EACH_PER_WORKTREE_ONLY &&
+   ref_type(iter->iter0->refname) != REF_TYPE_PER_WORKTREE)
+   continue;
+
if (!(iter->flags & DO_FOR_EACH_INCLUDE_BROKEN) &&
!ref_resolves_to_object(iter->iter0->refname,
iter->iter0->oid,
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 681982b..708b260 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -467,10 +467,18 @@ extern struct ref_iterator *current_ref_iter;
 int do_for_each_ref_iterator(struct ref_iterator *iter,
 each_ref_fn fn, void *cb_data);
 
-/* refs backends */
+/*
+ * Only include per-worktree refs in a do_for_each_ref*() iteration.
+ * Normally this will be used with a files ref_store, since that's
+ * where all reference backends will presumably store their
+ * per-worktree refs.
+ */
+#define DO_FOR_EACH_PER_WORKTREE_ONLY 0x02
 
 struct ref_store;
 
+/* refs backends */
+
 /*
  * Initialize the ref_store for the specified submodule, or for the
  * main repository if submodule == NULL. These functions should call
-- 
2.9.3

[PATCH v2 22/38] refs: make create_symref() virtual

[Michael Haggerty]

Signed-off-by: Michael Haggerty 
Signed-off-by: Junio C Hamano 
---
 refs.c   | 9 +
 refs/files-backend.c | 7 ++-
 refs/refs-internal.h | 5 +
 3 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/refs.c b/refs.c
index 961927a..74874e5 100644
--- a/refs.c
+++ b/refs.c
@@ -1428,6 +1428,15 @@ int pack_refs(unsigned int flags)
return refs->be->pack_refs(refs, flags);
 }
 
+int create_symref(const char *ref_target, const char *refs_heads_master,
+ const char *logmsg)
+{
+   struct ref_store *refs = get_ref_store(NULL);
+
+   return refs->be->create_symref(refs, ref_target, refs_heads_master,
+  logmsg);
+}
+
 int ref_transaction_commit(struct ref_transaction *transaction,
   struct strbuf *err)
 {
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 7ad8821..bf53670 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3011,12 +3011,16 @@ static int create_symref_locked(struct ref_lock *lock, 
const char *refname,
return 0;
 }
 
-int create_symref(const char *refname, const char *target, const char *logmsg)
+static int files_create_symref(struct ref_store *ref_store,
+  const char *refname, const char *target,
+  const char *logmsg)
 {
struct strbuf err = STRBUF_INIT;
struct ref_lock *lock;
int ret;
 
+   files_downcast(ref_store, 0, "create_symref");
+
lock = lock_ref_sha1_basic(refname, NULL, NULL, NULL, REF_NODEREF, NULL,
   );
if (!lock) {
@@ -4033,6 +4037,7 @@ struct ref_storage_be refs_be_files = {
files_transaction_commit,
 
files_pack_refs,
+   files_create_symref,
 
files_read_raw_ref,
files_verify_refname_available
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 256f7f5..bf96503 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -501,6 +501,10 @@ typedef int ref_transaction_commit_fn(struct ref_store 
*refs,
  struct strbuf *err);
 
 typedef int pack_refs_fn(struct ref_store *ref_store, unsigned int flags);
+typedef int create_symref_fn(struct ref_store *ref_store,
+const char *ref_target,
+const char *refs_heads_master,
+const char *logmsg);
 
 /*
  * Read a reference from the specified reference store, non-recursively.
@@ -557,6 +561,7 @@ struct ref_storage_be {
ref_transaction_commit_fn *transaction_commit;
 
pack_refs_fn *pack_refs;
+   create_symref_fn *create_symref;
 
read_raw_ref_fn *read_raw_ref;
verify_refname_available_fn *verify_refname_available;
-- 
2.9.3

[PATCH v2 33/38] refs: add method for initial ref transaction commit

[Michael Haggerty]

From: David Turner 

Signed-off-by: Ronnie Sahlberg 
Signed-off-by: David Turner 
Signed-off-by: Junio C Hamano 
Signed-off-by: Michael Haggerty 
Signed-off-by: Junio C Hamano 
---
 refs.c   | 8 
 refs/files-backend.c | 8 +---
 refs/refs-internal.h | 1 +
 3 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/refs.c b/refs.c
index cb9d805..3830918 100644
--- a/refs.c
+++ b/refs.c
@@ -1524,3 +1524,11 @@ int reflog_expire(const char *refname, const unsigned 
char *sha1,
   prepare_fn, should_prune_fn,
   cleanup_fn, policy_cb_data);
 }
+
+int initial_ref_transaction_commit(struct ref_transaction *transaction,
+  struct strbuf *err)
+{
+   struct ref_store *refs = get_ref_store(NULL);
+
+   return refs->be->initial_transaction_commit(refs, transaction, err);
+}
diff --git a/refs/files-backend.c b/refs/files-backend.c
index a810dfa..81008d7 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3839,11 +3839,12 @@ static int ref_present(const char *refname,
return string_list_has_string(affected_refnames, refname);
 }
 
-int initial_ref_transaction_commit(struct ref_transaction *transaction,
-  struct strbuf *err)
+static int files_initial_transaction_commit(struct ref_store *ref_store,
+   struct ref_transaction *transaction,
+   struct strbuf *err)
 {
struct files_ref_store *refs =
-   get_files_ref_store(NULL, "initial_ref_transaction_commit");
+   files_downcast(ref_store, 0, "initial_ref_transaction_commit");
int ret = 0, i;
struct string_list affected_refnames = STRING_LIST_INIT_NODUP;
 
@@ -4071,6 +4072,7 @@ struct ref_storage_be refs_be_files = {
"files",
files_ref_store_create,
files_transaction_commit,
+   files_initial_transaction_commit,
 
files_pack_refs,
files_peel_ref,
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index a20b622..08c8586 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -582,6 +582,7 @@ struct ref_storage_be {
const char *name;
ref_store_init_fn *init;
ref_transaction_commit_fn *transaction_commit;
+   ref_transaction_commit_fn *initial_transaction_commit;
 
pack_refs_fn *pack_refs;
peel_ref_fn *peel_ref;
-- 
2.9.3

[PATCH v2 29/38] split_symref_update(): add a files_ref_store argument

[Michael Haggerty]

Signed-off-by: Michael Haggerty 
Signed-off-by: Junio C Hamano 
---
 refs/files-backend.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index ab2c1de..3a0db5a 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3401,7 +3401,8 @@ static int split_head_update(struct ref_update *update,
  * Note that the new update will itself be subject to splitting when
  * the iteration gets to it.
  */
-static int split_symref_update(struct ref_update *update,
+static int split_symref_update(struct files_ref_store *refs,
+  struct ref_update *update,
   const char *referent,
   struct ref_transaction *transaction,
   struct string_list *affected_refnames,
@@ -3583,7 +3584,8 @@ static int lock_ref_for_update(struct files_ref_store 
*refs,
 * of processing the split-off update, so we
 * don't have to do it here.
 */
-   ret = split_symref_update(update, referent.buf, 
transaction,
+   ret = split_symref_update(refs, update,
+ referent.buf, transaction,
  affected_refnames, err);
if (ret)
return ret;
-- 
2.9.3

[PATCH v2 30/38] files_ref_iterator_begin(): take a ref_store argument

[Michael Haggerty]

Signed-off-by: Michael Haggerty 
Signed-off-by: Junio C Hamano 
---
 refs.c   | 2 +-
 refs/files-backend.c | 4 ++--
 refs/refs-internal.h | 8 
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/refs.c b/refs.c
index f10f86a..d00126b 100644
--- a/refs.c
+++ b/refs.c
@@ -1157,7 +1157,7 @@ static int do_for_each_ref(const char *submodule, const 
char *prefix,
if (!refs)
return 0;
 
-   iter = files_ref_iterator_begin(submodule, prefix, flags);
+   iter = files_ref_iterator_begin(refs, prefix, flags);
iter = prefix_ref_iterator_begin(iter, prefix, trim);
 
return do_for_each_ref_iterator(iter, fn, cb_data);
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 3a0db5a..f521b5d 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1863,11 +1863,11 @@ static struct ref_iterator_vtable 
files_ref_iterator_vtable = {
 };
 
 struct ref_iterator *files_ref_iterator_begin(
-   const char *submodule,
+   struct ref_store *ref_store,
const char *prefix, unsigned int flags)
 {
struct files_ref_store *refs =
-   get_files_ref_store(submodule, "ref_iterator_begin");
+   files_downcast(ref_store, 1, "ref_iterator_begin");
struct ref_dir *loose_dir, *packed_dir;
struct ref_iterator *loose_iter, *packed_iter;
struct files_ref_iterator *iter;
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 84c81ad..0af1079 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -404,13 +404,15 @@ struct ref_iterator *prefix_ref_iterator_begin(struct 
ref_iterator *iter0,
   const char *prefix,
   int trim);
 
+struct ref_store;
+
 /*
  * Iterate over the packed and loose references in the specified
- * submodule that are within find_containing_dir(prefix). If prefix is
+ * ref_store that are within find_containing_dir(prefix). If prefix is
  * NULL or the empty string, iterate over all references in the
  * submodule.
  */
-struct ref_iterator *files_ref_iterator_begin(const char *submodule,
+struct ref_iterator *files_ref_iterator_begin(struct ref_store *ref_store,
  const char *prefix,
  unsigned int flags);
 
@@ -484,8 +486,6 @@ extern struct ref_iterator *current_ref_iter;
 int do_for_each_ref_iterator(struct ref_iterator *iter,
 each_ref_fn fn, void *cb_data);
 
-struct ref_store;
-
 /* refs backends */
 
 /*
-- 
2.9.3

[PATCH v2 31/38] refs: add method iterator_begin

[Michael Haggerty]

Signed-off-by: Michael Haggerty 
Signed-off-by: Junio C Hamano 
---
 refs.c   |  2 +-
 refs/files-backend.c |  3 ++-
 refs/refs-internal.h | 24 
 3 files changed, 15 insertions(+), 14 deletions(-)

diff --git a/refs.c b/refs.c
index d00126b..798b08a 100644
--- a/refs.c
+++ b/refs.c
@@ -1157,7 +1157,7 @@ static int do_for_each_ref(const char *submodule, const 
char *prefix,
if (!refs)
return 0;
 
-   iter = files_ref_iterator_begin(refs, prefix, flags);
+   iter = refs->be->iterator_begin(refs, prefix, flags);
iter = prefix_ref_iterator_begin(iter, prefix, trim);
 
return do_for_each_ref_iterator(iter, fn, cb_data);
diff --git a/refs/files-backend.c b/refs/files-backend.c
index f521b5d..4451e13 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1862,7 +1862,7 @@ static struct ref_iterator_vtable 
files_ref_iterator_vtable = {
files_ref_iterator_abort
 };
 
-struct ref_iterator *files_ref_iterator_begin(
+static struct ref_iterator *files_ref_iterator_begin(
struct ref_store *ref_store,
const char *prefix, unsigned int flags)
 {
@@ -4054,6 +4054,7 @@ struct ref_storage_be refs_be_files = {
files_peel_ref,
files_create_symref,
 
+   files_ref_iterator_begin,
files_read_raw_ref,
files_verify_refname_available
 };
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 0af1079..5be62a2 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -404,18 +404,6 @@ struct ref_iterator *prefix_ref_iterator_begin(struct 
ref_iterator *iter0,
   const char *prefix,
   int trim);
 
-struct ref_store;
-
-/*
- * Iterate over the packed and loose references in the specified
- * ref_store that are within find_containing_dir(prefix). If prefix is
- * NULL or the empty string, iterate over all references in the
- * submodule.
- */
-struct ref_iterator *files_ref_iterator_begin(struct ref_store *ref_store,
- const char *prefix,
- unsigned int flags);
-
 /*
  * Iterate over the references in the main ref_store that have a
  * reflog. The paths within a directory are iterated over in arbitrary
@@ -488,6 +476,8 @@ int do_for_each_ref_iterator(struct ref_iterator *iter,
 
 /* refs backends */
 
+struct ref_store;
+
 /*
  * Initialize the ref_store for the specified submodule, or for the
  * main repository if submodule == NULL. These functions should call
@@ -509,6 +499,15 @@ typedef int create_symref_fn(struct ref_store *ref_store,
 const char *logmsg);
 
 /*
+ * Iterate over the references in the specified ref_store that are
+ * within find_containing_dir(prefix). If prefix is NULL or the empty
+ * string, iterate over all references in the submodule.
+ */
+typedef struct ref_iterator *ref_iterator_begin_fn(
+   struct ref_store *ref_store,
+   const char *prefix, unsigned int flags);
+
+/*
  * Read a reference from the specified reference store, non-recursively.
  * Set type to describe the reference, and:
  *
@@ -566,6 +565,7 @@ struct ref_storage_be {
peel_ref_fn *peel_ref;
create_symref_fn *create_symref;
 
+   ref_iterator_begin_fn *iterator_begin;
read_raw_ref_fn *read_raw_ref;
verify_refname_available_fn *verify_refname_available;
 };
-- 
2.9.3

[PATCH v2 00/38] Virtualization of the refs API

[Michael Haggerty]

This is v2 of the patch series to virtualize the references API
(though earlier patch series similar in spirit were submitted by
Ronnie Sahlberg and David Turner). Thanks to Junio, Eric, and Ramsay
for their comments about v1 [1].

Nobody pointed out any fundamental problems with v1, but this version
includes the following improvements:

* In "rename_ref_available(): add docstring":

  * Improve docstring as suggested by Junio.

* In "refs: create a base class "ref_store" for files_ref_store":

  * Let main_ref_store and submodule_ref_stores be initialized
implicitly rather than initializing them explicitly to NULL.

  * Add docstrings for those two variables.

  * Eliminate a temporary variable in `files_downcast()`.

* In "resolve_gitlink_ref(): avoid memory allocation in many cases":

  * Instead of keeping track of `orig_len`, after stripping off any
trailing slashes check whether `path[len - 1]` is NUL.

* In "refs: add methods for reflog":

  * Don't export `files_reflog_iterator_begin()` (suggested by
Ramsay).

In addition, given that v1 was pretty old, I have rebased the patch
series against the current upstream master. (The rebase was
unproblematic.)

This patch series is also available from my GitHub fork [2] as branch
"ref-store".

Michael

[1] http://public-inbox.org/git/cover.1464983301.git.mhag...@alum.mit.edu/t/
[2] https://github.com/mhagger/git

David Turner (8):
  rename_ref_available(): add docstring
  refs: add methods for reflog
  refs: add method for initial ref transaction commit
  refs: make delete_refs() virtual
  refs: add methods to init refs db
  refs: add method to rename refs
  refs: make lock generic
  refs: implement iteration over only per-worktree refs

Michael Haggerty (28):
  resolve_gitlink_ref(): eliminate temporary variable
  refs: rename struct ref_cache to files_ref_store
  refs: create a base class "ref_store" for files_ref_store
  add_packed_ref(): add a files_ref_store argument
  get_packed_ref(): add a files_ref_store argument
  resolve_missing_loose_ref(): add a files_ref_store argument
  {lock,commit,rollback}_packed_refs(): add files_ref_store arguments
  refs: reorder definitions
  resolve_packed_ref(): rename function from resolve_missing_loose_ref()
  resolve_gitlink_packed_ref(): remove function
  read_raw_ref(): take a (struct ref_store *) argument
  resolve_ref_recursively(): new function
  resolve_gitlink_ref(): implement using resolve_ref_recursively()
  resolve_gitlink_ref(): avoid memory allocation in many cases
  resolve_gitlink_ref(): rename path parameter to submodule
  refs: make read_raw_ref() virtual
  refs: make verify_refname_available() virtual
  refs: make pack_refs() virtual
  refs: make create_symref() virtual
  refs: make peel_ref() virtual
  repack_without_refs(): add a files_ref_store argument
  lock_raw_ref(): add a files_ref_store argument
  commit_ref_update(): add a files_ref_store argument
  lock_ref_for_update(): add a files_ref_store argument
  lock_ref_sha1_basic(): add a files_ref_store argument
  split_symref_update(): add a files_ref_store argument
  files_ref_iterator_begin(): take a ref_store argument
  refs: add method iterator_begin

Ronnie Sahlberg (2):
  refs: add a backend method structure
  refs: add a transaction_commit() method

 builtin/init-db.c|  21 +-
 refs.c   | 309 +++-
 refs.h   |  13 +-
 refs/files-backend.c | 566 +++
 refs/refs-internal.h | 220 +---
 5 files changed, 812 insertions(+), 317 deletions(-)

-- 
2.9.3

[PATCH v2 02/38] rename_ref_available(): add docstring

[Michael Haggerty]

From: David Turner 

And improve the internal variable names.

Signed-off-by: David Turner 
Signed-off-by: Junio C Hamano 
Signed-off-by: Michael Haggerty 
Signed-off-by: Junio C Hamano 
---
 refs.c   | 12 ++--
 refs/refs-internal.h | 12 +++-
 2 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/refs.c b/refs.c
index b4e7cac..4c609df 100644
--- a/refs.c
+++ b/refs.c
@@ -1081,20 +1081,20 @@ const char *find_descendant_ref(const char *dirname,
return NULL;
 }
 
-int rename_ref_available(const char *oldname, const char *newname)
+int rename_ref_available(const char *old_refname, const char *new_refname)
 {
struct string_list skip = STRING_LIST_INIT_NODUP;
struct strbuf err = STRBUF_INIT;
-   int ret;
+   int ok;
 
-   string_list_insert(, oldname);
-   ret = !verify_refname_available(newname, NULL, , );
-   if (!ret)
+   string_list_insert(, old_refname);
+   ok = !verify_refname_available(new_refname, NULL, , );
+   if (!ok)
error("%s", err.buf);
 
string_list_clear(, 0);
strbuf_release();
-   return ret;
+   return ok;
 }
 
 int head_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data)
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index efe5847..0206e2b 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -240,7 +240,17 @@ const char *find_descendant_ref(const char *dirname,
const struct string_list *extras,
const struct string_list *skip);
 
-int rename_ref_available(const char *oldname, const char *newname);
+/*
+ * Check whether an attempt to rename old_refname to new_refname would
+ * cause a D/F conflict with any existing reference (other than
+ * possibly old_refname). If there would be a conflict, emit an error
+ * message and return false; otherwise, return true.
+ *
+ * Note that this function is not safe against all races with other
+ * processes (though rename_ref() catches some races that might get by
+ * this check).
+ */
+int rename_ref_available(const char *old_refname, const char *new_refname);
 
 /* We allow "recursive" symbolic refs. Only within reason, though */
 #define SYMREF_MAXDEPTH 5
-- 
2.9.3

[PATCH v2 14/38] read_raw_ref(): take a (struct ref_store *) argument

[Michael Haggerty]

And make the function work for submodules.

Signed-off-by: Michael Haggerty 
Signed-off-by: Junio C Hamano 
---
 refs.c   |  4 +++-
 refs/files-backend.c | 18 +-
 refs/refs-internal.h |  9 ++---
 3 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/refs.c b/refs.c
index 9475844..9e6b005 100644
--- a/refs.c
+++ b/refs.c
@@ -1222,6 +1222,7 @@ const char *resolve_ref_unsafe(const char *refname, int 
resolve_flags,
static struct strbuf sb_refname = STRBUF_INIT;
int unused_flags;
int symref_count;
+   struct ref_store *refs = get_ref_store(NULL);
 
if (!flags)
flags = _flags;
@@ -1249,7 +1250,8 @@ const char *resolve_ref_unsafe(const char *refname, int 
resolve_flags,
for (symref_count = 0; symref_count < SYMREF_MAXDEPTH; symref_count++) {
unsigned int read_flags = 0;
 
-   if (read_raw_ref(refname, sha1, _refname, _flags)) {
+   if (read_raw_ref(refs, refname,
+sha1, _refname, _flags)) {
*flags |= read_flags;
if (errno != ENOENT || (resolve_flags & 
RESOLVE_REF_READING))
return NULL;
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 3b0c837..ce6fe94 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1349,11 +1349,12 @@ static int resolve_packed_ref(struct files_ref_store 
*refs,
return -1;
 }
 
-int read_raw_ref(const char *refname, unsigned char *sha1,
+int read_raw_ref(struct ref_store *ref_store,
+const char *refname, unsigned char *sha1,
 struct strbuf *referent, unsigned int *type)
 {
struct files_ref_store *refs =
-   get_files_ref_store(NULL, "read_raw_ref");
+   files_downcast(ref_store, 1, "read_raw_ref");
struct strbuf sb_contents = STRBUF_INIT;
struct strbuf sb_path = STRBUF_INIT;
const char *path;
@@ -1365,7 +1366,12 @@ int read_raw_ref(const char *refname, unsigned char 
*sha1,
 
*type = 0;
strbuf_reset(_path);
-   strbuf_git_path(_path, "%s", refname);
+
+   if (*refs->base.submodule)
+   strbuf_git_path_submodule(_path, refs->base.submodule, "%s", 
refname);
+   else
+   strbuf_git_path(_path, "%s", refname);
+
path = sb_path.buf;
 
 stat_ref:
@@ -1592,8 +1598,9 @@ static int lock_raw_ref(const char *refname, int 
mustexist,
unsigned int *type,
struct strbuf *err)
 {
+   struct ref_store *ref_store = get_ref_store(NULL);
struct files_ref_store *refs =
-   get_files_ref_store(NULL, "lock_raw_ref");
+   files_downcast(ref_store, 0, "lock_raw_ref");
struct ref_lock *lock;
struct strbuf ref_file = STRBUF_INIT;
int attempts_remaining = 3;
@@ -1683,7 +1690,8 @@ static int lock_raw_ref(const char *refname, int 
mustexist,
 * fear that its value will change.
 */
 
-   if (read_raw_ref(refname, lock->old_oid.hash, referent, type)) {
+   if (read_raw_ref(ref_store, refname,
+lock->old_oid.hash, referent, type)) {
if (errno == ENOENT) {
if (mustexist) {
/* Garden variety missing reference. */
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index b7367ab..fa41d51 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -484,9 +484,11 @@ extern struct ref_iterator *current_ref_iter;
 int do_for_each_ref_iterator(struct ref_iterator *iter,
 each_ref_fn fn, void *cb_data);
 
+struct ref_store;
+
 /*
- * Read the specified reference from the filesystem or packed refs
- * file, non-recursively. Set type to describe the reference, and:
+ * Read a reference from the specified reference store, non-recursively.
+ * Set type to describe the reference, and:
  *
  * - If refname is the name of a normal reference, fill in sha1
  *   (leaving referent unchanged).
@@ -522,7 +524,8 @@ int do_for_each_ref_iterator(struct ref_iterator *iter,
  * - in all other cases, referent will be untouched, and therefore
  *   refname will still be valid and unchanged.
  */
-int read_raw_ref(const char *refname, unsigned char *sha1,
+int read_raw_ref(struct ref_store *ref_store,
+const char *refname, unsigned char *sha1,
 struct strbuf *referent, unsigned int *type);
 
 /* refs backends */
-- 
2.9.3

[PATCH v2 08/38] resolve_missing_loose_ref(): add a files_ref_store argument

[Michael Haggerty]

Signed-off-by: Michael Haggerty 
Signed-off-by: Junio C Hamano 
---
 refs/files-backend.c | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 2fc8ec9..0c92ace 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1412,13 +1412,11 @@ static struct ref_entry *get_packed_ref(struct 
files_ref_store *refs,
 /*
  * A loose ref file doesn't exist; check for a packed ref.
  */
-static int resolve_missing_loose_ref(const char *refname,
+static int resolve_missing_loose_ref(struct files_ref_store *refs,
+const char *refname,
 unsigned char *sha1,
 unsigned int *flags)
 {
-   struct files_ref_store *refs =
-   get_files_ref_store(NULL, "resolve_missing_loose_ref");
-
struct ref_entry *entry;
 
/*
@@ -1438,6 +1436,8 @@ static int resolve_missing_loose_ref(const char *refname,
 int read_raw_ref(const char *refname, unsigned char *sha1,
 struct strbuf *referent, unsigned int *type)
 {
+   struct files_ref_store *refs =
+   get_files_ref_store(NULL, "read_raw_ref");
struct strbuf sb_contents = STRBUF_INIT;
struct strbuf sb_path = STRBUF_INIT;
const char *path;
@@ -1466,7 +1466,7 @@ int read_raw_ref(const char *refname, unsigned char *sha1,
if (lstat(path, ) < 0) {
if (errno != ENOENT)
goto out;
-   if (resolve_missing_loose_ref(refname, sha1, type)) {
+   if (resolve_missing_loose_ref(refs, refname, sha1, type)) {
errno = ENOENT;
goto out;
}
@@ -1500,7 +1500,7 @@ int read_raw_ref(const char *refname, unsigned char *sha1,
 * ref is supposed to be, there could still be a
 * packed ref:
 */
-   if (resolve_missing_loose_ref(refname, sha1, type)) {
+   if (resolve_missing_loose_ref(refs, refname, sha1, type)) {
errno = EISDIR;
goto out;
}
-- 
2.9.3

[PATCH v2 01/38] resolve_gitlink_ref(): eliminate temporary variable

[Michael Haggerty]

Signed-off-by: Michael Haggerty 
Signed-off-by: Junio C Hamano 
---
 refs/files-backend.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 12290d2..74c2c47 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1387,7 +1387,7 @@ static int resolve_gitlink_ref_recursive(struct ref_cache 
*refs,
 
 int resolve_gitlink_ref(const char *path, const char *refname, unsigned char 
*sha1)
 {
-   int len = strlen(path), retval;
+   int len = strlen(path);
struct strbuf submodule = STRBUF_INIT;
struct ref_cache *refs;
 
@@ -1404,8 +1404,7 @@ int resolve_gitlink_ref(const char *path, const char 
*refname, unsigned char *sh
}
strbuf_release();
 
-   retval = resolve_gitlink_ref_recursive(refs, refname, sha1, 0);
-   return retval;
+   return resolve_gitlink_ref_recursive(refs, refname, sha1, 0);
 }
 
 /*
-- 
2.9.3

[PATCH v2 07/38] get_packed_ref(): add a files_ref_store argument

[Michael Haggerty]

Signed-off-by: Michael Haggerty 
Signed-off-by: Junio C Hamano 
---
 refs/files-backend.c | 16 +---
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index c544de8..2fc8ec9 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1403,11 +1403,9 @@ int resolve_gitlink_ref(const char *path, const char 
*refname, unsigned char *sh
  * Return the ref_entry for the given refname from the packed
  * references.  If it does not exist, return NULL.
  */
-static struct ref_entry *get_packed_ref(const char *refname)
+static struct ref_entry *get_packed_ref(struct files_ref_store *refs,
+   const char *refname)
 {
-   struct files_ref_store *refs =
-   get_files_ref_store(NULL, "get_packed_ref");
-
return find_ref(get_packed_refs(refs), refname);
 }
 
@@ -1418,13 +1416,16 @@ static int resolve_missing_loose_ref(const char 
*refname,
 unsigned char *sha1,
 unsigned int *flags)
 {
+   struct files_ref_store *refs =
+   get_files_ref_store(NULL, "resolve_missing_loose_ref");
+
struct ref_entry *entry;
 
/*
 * The loose reference file does not exist; check for a packed
 * reference.
 */
-   entry = get_packed_ref(refname);
+   entry = get_packed_ref(refs, refname);
if (entry) {
hashcpy(sha1, entry->u.value.oid.hash);
*flags |= REF_ISPACKED;
@@ -1836,6 +1837,7 @@ static enum peel_status peel_entry(struct ref_entry 
*entry, int repeel)
 
 int peel_ref(const char *refname, unsigned char *sha1)
 {
+   struct files_ref_store *refs = get_files_ref_store(NULL, "peel_ref");
int flag;
unsigned char base[20];
 
@@ -1860,7 +1862,7 @@ int peel_ref(const char *refname, unsigned char *sha1)
 * have REF_KNOWS_PEELED.
 */
if (flag & REF_ISPACKED) {
-   struct ref_entry *r = get_packed_ref(refname);
+   struct ref_entry *r = get_packed_ref(refs, refname);
if (r) {
if (peel_entry(r, 0))
return -1;
@@ -2469,7 +2471,7 @@ static int repack_without_refs(struct string_list 
*refnames, struct strbuf *err)
 
/* Look for a packed ref */
for_each_string_list_item(refname, refnames) {
-   if (get_packed_ref(refname->string)) {
+   if (get_packed_ref(refs, refname->string)) {
needs_repacking = 1;
break;
}
-- 
2.9.3

[PATCH v2 10/38] refs: add a transaction_commit() method

[Michael Haggerty]

From: Ronnie Sahlberg 

Signed-off-by: Ronnie Sahlberg 
Signed-off-by: David Turner 
Signed-off-by: Junio C Hamano 
Signed-off-by: Jeff King 
Signed-off-by: Michael Haggerty 
Signed-off-by: Junio C Hamano 
---
 refs.c   |  9 +
 refs/files-backend.c | 10 ++
 refs/refs-internal.h |  5 +
 3 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/refs.c b/refs.c
index 9d6bcb1..9475844 100644
--- a/refs.c
+++ b/refs.c
@@ -1377,3 +1377,12 @@ void assert_main_repository(struct ref_store *refs, 
const char *caller)
if (*refs->submodule)
die("BUG: %s called for a submodule", caller);
 }
+
+/* backend functions */
+int ref_transaction_commit(struct ref_transaction *transaction,
+  struct strbuf *err)
+{
+   struct ref_store *refs = get_ref_store(NULL);
+
+   return refs->be->transaction_commit(refs, transaction, err);
+}
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 6913d45..d896052 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3710,11 +3710,12 @@ static int lock_ref_for_update(struct ref_update 
*update,
return 0;
 }
 
-int ref_transaction_commit(struct ref_transaction *transaction,
-  struct strbuf *err)
+static int files_transaction_commit(struct ref_store *ref_store,
+   struct ref_transaction *transaction,
+   struct strbuf *err)
 {
struct files_ref_store *refs =
-   get_files_ref_store(NULL, "ref_transaction_commit");
+   files_downcast(ref_store, 0, "ref_transaction_commit");
int ret = 0, i;
struct string_list refs_to_delete = STRING_LIST_INIT_NODUP;
struct string_list_item *ref_to_delete;
@@ -4103,5 +4104,6 @@ int reflog_expire(const char *refname, const unsigned 
char *sha1,
 struct ref_storage_be refs_be_files = {
NULL,
"files",
-   files_ref_store_create
+   files_ref_store_create,
+   files_transaction_commit
 };
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index b952038..b7367ab 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -535,10 +535,15 @@ int read_raw_ref(const char *refname, unsigned char *sha1,
  */
 typedef struct ref_store *ref_store_init_fn(const char *submodule);
 
+typedef int ref_transaction_commit_fn(struct ref_store *refs,
+ struct ref_transaction *transaction,
+ struct strbuf *err);
+
 struct ref_storage_be {
struct ref_storage_be *next;
const char *name;
ref_store_init_fn *init;
+   ref_transaction_commit_fn *transaction_commit;
 };
 
 extern struct ref_storage_be refs_be_files;
-- 
2.9.3

[PATCH v2 18/38] resolve_gitlink_ref(): rename path parameter to submodule

[Michael Haggerty]

Signed-off-by: Michael Haggerty 
Signed-off-by: Junio C Hamano 
---
 refs.c | 13 +++--
 refs.h |  9 +
 2 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/refs.c b/refs.c
index 34c0c5e..e881874 100644
--- a/refs.c
+++ b/refs.c
@@ -1299,26 +1299,27 @@ const char *resolve_ref_unsafe(const char *refname, int 
resolve_flags,
   resolve_flags, sha1, flags);
 }
 
-int resolve_gitlink_ref(const char *path, const char *refname, unsigned char 
*sha1)
+int resolve_gitlink_ref(const char *submodule, const char *refname,
+   unsigned char *sha1)
 {
-   size_t len = strlen(path);
+   size_t len = strlen(submodule);
struct ref_store *refs;
int flags;
 
-   while (len && path[len - 1] == '/')
+   while (len && submodule[len - 1] == '/')
len--;
 
if (!len)
return -1;
 
-   if (path[len]) {
+   if (submodule[len]) {
/* We need to strip off one or more trailing slashes */
-   char *stripped = xmemdupz(path, len);
+   char *stripped = xmemdupz(submodule, len);
 
refs = get_ref_store(stripped);
free(stripped);
} else {
-   refs = get_ref_store(path);
+   refs = get_ref_store(submodule);
}
 
if (!refs)
diff --git a/refs.h b/refs.h
index 9a29f1b..17e8cfb 100644
--- a/refs.h
+++ b/refs.h
@@ -77,11 +77,12 @@ int is_branch(const char *refname);
 int peel_ref(const char *refname, unsigned char *sha1);
 
 /**
- * Resolve refname in the nested "gitlink" repository that is located
- * at path.  If the resolution is successful, return 0 and set sha1 to
- * the name of the object; otherwise, return a non-zero value.
+ * Resolve refname in the nested "gitlink" repository in the specified
+ * submodule (which must be non-NULL). If the resolution is
+ * successful, return 0 and set sha1 to the name of the object;
+ * otherwise, return a non-zero value.
  */
-int resolve_gitlink_ref(const char *path, const char *refname,
+int resolve_gitlink_ref(const char *submodule, const char *refname,
unsigned char *sha1);
 
 /*
-- 
2.9.3

[PATCH v2 16/38] resolve_gitlink_ref(): implement using resolve_ref_recursively()

[Michael Haggerty]

resolve_ref_recursively() can handle references in arbitrary files
reference stores, so use it to resolve "gitlink" (i.e., submodule)
references. Aside from removing redundant code, this allows submodule
lookups to benefit from the much more robust code that we use for
reading non-submodule references. And, since the code is now agnostic
about reference backends, it will work for any future references
backend (so move its definition to refs.c).

Signed-off-by: Michael Haggerty 
Signed-off-by: Junio C Hamano 
---
 refs.c   | 24 +++
 refs/files-backend.c | 67 
 2 files changed, 24 insertions(+), 67 deletions(-)

diff --git a/refs.c b/refs.c
index 3723169..647ead5 100644
--- a/refs.c
+++ b/refs.c
@@ -1299,6 +1299,30 @@ const char *resolve_ref_unsafe(const char *refname, int 
resolve_flags,
   resolve_flags, sha1, flags);
 }
 
+int resolve_gitlink_ref(const char *path, const char *refname, unsigned char 
*sha1)
+{
+   int len = strlen(path);
+   struct strbuf submodule = STRBUF_INIT;
+   struct ref_store *refs;
+   int flags;
+
+   while (len && path[len-1] == '/')
+   len--;
+   if (!len)
+   return -1;
+
+   strbuf_add(, path, len);
+   refs = get_ref_store(submodule.buf);
+   strbuf_release();
+   if (!refs)
+   return -1;
+
+   if (!resolve_ref_recursively(refs, refname, 0, sha1, ) ||
+   is_null_sha1(sha1))
+   return -1;
+   return 0;
+}
+
 /* A pointer to the ref_store for the main repository: */
 static struct ref_store *main_ref_store;
 
diff --git a/refs/files-backend.c b/refs/files-backend.c
index ce6fe94..6382c6b 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1494,73 +1494,6 @@ static void unlock_ref(struct ref_lock *lock)
free(lock);
 }
 
-#define MAXREFLEN (1024)
-
-static int resolve_gitlink_ref_recursive(struct files_ref_store *refs,
-const char *refname, unsigned char 
*sha1,
-int recursion)
-{
-   int fd, len;
-   char buffer[128], *p;
-   char *path;
-
-   if (recursion > SYMREF_MAXDEPTH || strlen(refname) > MAXREFLEN)
-   return -1;
-   path = *refs->base.submodule
-   ? git_pathdup_submodule(refs->base.submodule, "%s", refname)
-   : git_pathdup("%s", refname);
-   fd = open(path, O_RDONLY);
-   free(path);
-   if (fd < 0) {
-   unsigned int flags;
-
-   return resolve_packed_ref(refs, refname, sha1, );
-   }
-
-   len = read(fd, buffer, sizeof(buffer)-1);
-   close(fd);
-   if (len < 0)
-   return -1;
-   while (len && isspace(buffer[len-1]))
-   len--;
-   buffer[len] = 0;
-
-   /* Was it a detached head or an old-fashioned symlink? */
-   if (!get_sha1_hex(buffer, sha1))
-   return 0;
-
-   /* Symref? */
-   if (strncmp(buffer, "ref:", 4))
-   return -1;
-   p = buffer + 4;
-   while (isspace(*p))
-   p++;
-
-   return resolve_gitlink_ref_recursive(refs, p, sha1, recursion+1);
-}
-
-int resolve_gitlink_ref(const char *path, const char *refname, unsigned char 
*sha1)
-{
-   int len = strlen(path);
-   struct strbuf submodule = STRBUF_INIT;
-   struct files_ref_store *refs;
-
-   while (len && path[len-1] == '/')
-   len--;
-   if (!len)
-   return -1;
-
-   strbuf_add(, path, len);
-   refs = get_files_ref_store(submodule.buf, "resolve_gitlink_ref");
-   if (!refs) {
-   strbuf_release();
-   return -1;
-   }
-   strbuf_release();
-
-   return resolve_gitlink_ref_recursive(refs, refname, sha1, 0);
-}
-
 /*
  * Lock refname, without following symrefs, and set *lock_p to point
  * at a newly-allocated lock object. Fill in lock->old_oid, referent,
-- 
2.9.3

[PATCH v2 04/38] refs: add a backend method structure

[Michael Haggerty]

From: Ronnie Sahlberg 

Add a `struct ref_storage_be` to represent types of reference stores. In
OO notation, this is the class, and will soon hold some class
methods (e.g., a factory to create new ref_store instances) and will
also serve as the vtable for ref_store instances of that type.

As yet, the backends cannot do anything.

Signed-off-by: Ronnie Sahlberg 
Signed-off-by: David Turner 
Signed-off-by: Junio C Hamano 
Signed-off-by: Jeff King 
Signed-off-by: Michael Haggerty 
Signed-off-by: Junio C Hamano 
---
 refs.c   | 19 +++
 refs.h   |  2 ++
 refs/files-backend.c |  5 +
 refs/refs-internal.h |  8 
 4 files changed, 34 insertions(+)

diff --git a/refs.c b/refs.c
index 4c609df..d2a29bb 100644
--- a/refs.c
+++ b/refs.c
@@ -10,6 +10,25 @@
 #include "tag.h"
 
 /*
+ * List of all available backends
+ */
+static struct ref_storage_be *refs_backends = _be_files;
+
+static struct ref_storage_be *find_ref_storage_backend(const char *name)
+{
+   struct ref_storage_be *be;
+   for (be = refs_backends; be; be = be->next)
+   if (!strcmp(be->name, name))
+   return be;
+   return NULL;
+}
+
+int ref_storage_backend_exists(const char *name)
+{
+   return find_ref_storage_backend(name) != NULL;
+}
+
+/*
  * How to handle various characters in refnames:
  * 0: An acceptable character for refs
  * 1: End-of-component
diff --git a/refs.h b/refs.h
index 1b02043..9a29f1b 100644
--- a/refs.h
+++ b/refs.h
@@ -544,4 +544,6 @@ int reflog_expire(const char *refname, const unsigned char 
*sha1,
  reflog_expiry_cleanup_fn cleanup_fn,
  void *policy_cb_data);
 
+int ref_storage_backend_exists(const char *name);
+
 #endif /* REFS_H */
diff --git a/refs/files-backend.c b/refs/files-backend.c
index aa09586..ecf66e6 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -4076,3 +4076,8 @@ int reflog_expire(const char *refname, const unsigned 
char *sha1,
unlock_ref(lock);
return -1;
 }
+
+struct ref_storage_be refs_be_files = {
+   NULL,
+   "files"
+};
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 0206e2b..2c9d134 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -525,4 +525,12 @@ int do_for_each_ref_iterator(struct ref_iterator *iter,
 int read_raw_ref(const char *refname, unsigned char *sha1,
 struct strbuf *referent, unsigned int *type);
 
+/* refs backends */
+struct ref_storage_be {
+   struct ref_storage_be *next;
+   const char *name;
+};
+
+extern struct ref_storage_be refs_be_files;
+
 #endif /* REFS_REFS_INTERNAL_H */
-- 
2.9.3

[PATCH v2 03/38] refs: rename struct ref_cache to files_ref_store

[Michael Haggerty]

The greater goal of this patch series is to develop the concept of a
reference store, which is a place that references, their values, and
their reflogs are stored, and to virtualize the reference interface so
that different types of ref_stores can be implemented. We will then, for
example, use ref_store instances to access submodule references and
worktree references.

Currently, we keep a ref_cache for each submodule that has had its
references iterated over. It is a far cry from a ref_store, but they are
stored the way we will want to store ref_stores, and ref_stores will
eventually have to hold the reference caches. So let's treat ref_caches
as embryo ref_stores, and build them out from there.

As the first step, simply rename `ref_cache` to `files_ref_store`, and
rename some functions and attributes correspondingly.

Signed-off-by: Michael Haggerty 
Signed-off-by: Junio C Hamano 
---
 refs/files-backend.c | 126 +--
 1 file changed, 63 insertions(+), 63 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 74c2c47..aa09586 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -39,7 +39,7 @@ struct ref_value {
struct object_id peeled;
 };
 
-struct ref_cache;
+struct files_ref_store;
 
 /*
  * Information used (along with the information in ref_entry) to
@@ -78,8 +78,8 @@ struct ref_dir {
 */
int sorted;
 
-   /* A pointer to the ref_cache that contains this ref_dir. */
-   struct ref_cache *ref_cache;
+   /* A pointer to the files_ref_store that contains this ref_dir. */
+   struct files_ref_store *ref_store;
 
struct ref_entry **entries;
 };
@@ -161,7 +161,7 @@ struct ref_entry {
 
 static void read_loose_refs(const char *dirname, struct ref_dir *dir);
 static int search_ref_dir(struct ref_dir *dir, const char *refname, size_t 
len);
-static struct ref_entry *create_dir_entry(struct ref_cache *ref_cache,
+static struct ref_entry *create_dir_entry(struct files_ref_store *ref_store,
  const char *dirname, size_t len,
  int incomplete);
 static void add_entry_to_dir(struct ref_dir *dir, struct ref_entry *entry);
@@ -183,7 +183,7 @@ static struct ref_dir *get_ref_dir(struct ref_entry *entry)
int pos = search_ref_dir(dir, "refs/bisect/", 12);
if (pos < 0) {
struct ref_entry *child_entry;
-   child_entry = create_dir_entry(dir->ref_cache,
+   child_entry = create_dir_entry(dir->ref_store,
   "refs/bisect/",
   12, 1);
add_entry_to_dir(dir, child_entry);
@@ -261,13 +261,13 @@ static void clear_ref_dir(struct ref_dir *dir)
  * dirname is the name of the directory with a trailing slash (e.g.,
  * "refs/heads/") or "" for the top-level directory.
  */
-static struct ref_entry *create_dir_entry(struct ref_cache *ref_cache,
+static struct ref_entry *create_dir_entry(struct files_ref_store *ref_store,
  const char *dirname, size_t len,
  int incomplete)
 {
struct ref_entry *direntry;
FLEX_ALLOC_MEM(direntry, name, dirname, len);
-   direntry->u.subdir.ref_cache = ref_cache;
+   direntry->u.subdir.ref_store = ref_store;
direntry->flag = REF_DIR | (incomplete ? REF_INCOMPLETE : 0);
return direntry;
 }
@@ -343,7 +343,7 @@ static struct ref_dir *search_for_subdir(struct ref_dir 
*dir,
 * therefore, create an empty record for it but mark
 * the record complete.
 */
-   entry = create_dir_entry(dir->ref_cache, subdirname, len, 0);
+   entry = create_dir_entry(dir->ref_store, subdirname, len, 0);
add_entry_to_dir(dir, entry);
} else {
entry = dir->entries[entry_index];
@@ -887,9 +887,9 @@ struct packed_ref_cache {
 
/*
 * Count of references to the data structure in this instance,
-* including the pointer from ref_cache::packed if any.  The
-* data will not be freed as long as the reference count is
-* nonzero.
+* including the pointer from files_ref_store::packed if any.
+* The data will not be freed as long as the reference count
+* is nonzero.
 */
unsigned int referrers;
 
@@ -910,17 +910,17 @@ struct packed_ref_cache {
  * Future: need to be in "struct repository"
  * when doing a full libification.
  */
-static struct ref_cache {
-   struct ref_cache *next;
+static struct files_ref_store {
+   struct files_ref_store *next;
struct ref_entry *loose;
struct 

[PATCH v2 06/38] add_packed_ref(): add a files_ref_store argument

[Michael Haggerty]

Signed-off-by: Michael Haggerty 
Signed-off-by: Junio C Hamano 
---
 refs/files-backend.c | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 439c500..c544de8 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1192,10 +1192,9 @@ static struct ref_dir *get_packed_refs(struct 
files_ref_store *refs)
  * lock_packed_refs()).  To actually write the packed-refs file, call
  * commit_packed_refs().
  */
-static void add_packed_ref(const char *refname, const unsigned char *sha1)
+static void add_packed_ref(struct files_ref_store *refs,
+  const char *refname, const unsigned char *sha1)
 {
-   struct files_ref_store *refs =
-   get_files_ref_store(NULL, "add_packed_ref");
struct packed_ref_cache *packed_ref_cache = get_packed_ref_cache(refs);
 
if (!packed_ref_cache->lock)
@@ -3879,6 +3878,8 @@ static int ref_present(const char *refname,
 int initial_ref_transaction_commit(struct ref_transaction *transaction,
   struct strbuf *err)
 {
+   struct files_ref_store *refs =
+   get_files_ref_store(NULL, "initial_ref_transaction_commit");
int ret = 0, i;
struct string_list affected_refnames = STRING_LIST_INIT_NODUP;
 
@@ -3938,7 +3939,7 @@ int initial_ref_transaction_commit(struct ref_transaction 
*transaction,
 
if ((update->flags & REF_HAVE_NEW) &&
!is_null_sha1(update->new_sha1))
-   add_packed_ref(update->refname, update->new_sha1);
+   add_packed_ref(refs, update->refname, update->new_sha1);
}
 
if (commit_packed_refs()) {
-- 
2.9.3

Re: [PATCH 1/6] git-gui: The term unified for remote in Japanese

[Jakub Narębski]

W dniu 03.09.2016 o 16:43, Satoshi Yasushima pisze:

>  msgid "Tracking branch %s is not a branch in the remote repository."
> -msgstr "トラッキング・ブランチ %s は遠隔リポジトリのブランチではありません。"
> +msgstr ""

What for is the above part of change (empty string)?

> +"トラッキング・ブランチ %s はリモートリポジトリのブランチではありません。"

-- 
Jakub Narębski

Re: Fixup of a fixup not working right

[Philip Oakley]

From: "Johannes Schindelin" 

Hi Junio & Philip,

On Fri, 2 Sep 2016, Junio C Hamano wrote:


"Philip Oakley"  writes:

> As I understand this it's implied by design. The issue is that the
> rebase is looking for that named commit within its current rebase
> range, and can't find it, so ignores it.
>
> There is a separate issue that all the fixup! fixup! messages are
> essentially treated as being concatenations of the original fixup!, no
> matter how many time the fiup is present.

They can be handled separately, but they come from the same "design"
that could be improved.  When the "original" is not in the range to
be rebased for whatever reason (including the most likely one, i.e.
it has already graduated to become part of the public history), the
best thing the user could do at that point may be, as you suggested
to Robert in your message, to turn the "fixup! original" that did
not make in time before "original" hit the public record into a
standalone "fix original" follow-up change, and then to squash
subsequent "fixup! fixup! original" (and other "fixup! original",
too) into that commit.  And a good direction forward may be to see
if "rebase -i" can be taught to be more helpful for the user who
wants to do that.

Perhaps a change like this to "rebase -i":

 - The search for "original" when handling "pick fixup! original",
   when it does not find "original", could turn it into "reword
   fixup! original" without changing its position in the instruction
   sequence.

 - The search for "original" when handling "pick fixup! fixup!
   original", could be (probably unconditionally) changed to look
   for "fixup! original" to amend, instead of looking for "original"
   as the current code (this is your "separate issue").  The same
   "if the commit to be amended is not found, turn it into reword"
   rule from the above applies to this one, too.

may be an improvement?


I would be *very* careful with such a change.


I agree about the need for care. The use case must be well understood.


The point is that fixup! messages are really special, and are always
intended to be squashed into the referenced commit *before* the latter
hits `master`.


I think it's here that we have the hidden use case. I agree that all fixups 
should be squashed before they hit the blessed golden  repository.


I suspect that some use cases have intermediate repositories that contain a 
'master' branch (it's just a name ;-) that isn't blessed and golden, e.g. at 
the team review repo level. In such cases it is possible for a fixup! to be 
passed up as part of the review, though it's not the current 
norm/expectation.




The entire design of the fixup! feature (using the commit subject as
identifier, which is only "unique enough" in a topic branch that is still
being developed) points to that.

I am fairly certain that we would run into tons of problems if we diluted
the concept of fixup! commits by changing the design so that fixup!
commits all of a sudden become their own, "real" commits that can be fixed
up themselves, as much of the current code simply does not expect that.


We already had that. the commit 22c5b13 (rebase -i: handle fixup! fixup! 
in --autosquash, 2013-06-27) was an attempt to work around misunderstandings 
about what fixed what.


In Robert's scenario (IIUC) that patch was too aggressive for the case where 
the original commit is not part of the rebase. The patch lept in a little 
too early in the processing so as to pretend that if it saw repeated "fixup! 
" strings at the start of the messages it pretented there was only one, so 
that they all applied in the original sequence.


I _think_ that the right approach would be to just bring such fixups 
together in the to-do list ("as normal"), but still have the minimal common 
commit message string still present, so that it would, in this case, still 
have a resulting squashed commit that starts "!fixup ". It's important to be 
moderately lenient to user choices - they may know something we don't, or at 
least accept that their use case is 'unusual' [1]




In short, I am opposed to this change.


It's not like G4W doesn't need fixup!s on the side branches e.g. 5eaffe9 
("fixup! Handle new t1501 test case properly with MinGW", 2016-07-12)



And even if I am overruled, I would strongly suggest to implement this on
top of my rebase-i-extra branch (i.e. in the rebase--helper instead of the
shell script) to avoid double churn.


I definitely agree there.



Ciao,
Johannes


--
Philip
[1] The removal of the "theirs" merge strategy is one I'd add to that list. 
Calling it, for example, "reversed" would have kept it available while 
reduced it's visibility. See http://marc.info/?l=git=121637513604413=2 

[PATCH 1/4] add: document the chmod option

[Thomas Gummerer]

The git add --chmod option was introduced in 4e55ed3 ("add: add
--chmod=+x / --chmod=-x options", 2016-05-31), but was never
documented.  Document the feature.

Signed-off-by: Thomas Gummerer 
---
 Documentation/git-add.txt | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index 6a96a66..8caa691 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -11,7 +11,7 @@ SYNOPSIS
 'git add' [--verbose | -v] [--dry-run | -n] [--force | -f] [--interactive | 
-i] [--patch | -p]
  [--edit | -e] [--[no-]all | --[no-]ignore-removal | [--update | -u]]
  [--intent-to-add | -N] [--refresh] [--ignore-errors] 
[--ignore-missing]
- [--] [...]
+ [--chmod=(+|-)x] [--] [...]
 
 DESCRIPTION
 ---
@@ -165,6 +165,11 @@ for "git add --no-all ...", i.e. ignored removed 
files.
be ignored, no matter if they are already present in the work
tree or not.
 
+--chmod=(+|-)::
+   Override the executable bit of the added files.  The executable
+   bit is only changed in the index, the files on disk are left
+   unchanged.
+
 \--::
This option can be used to separate command-line options from
the list of files, (useful when filenames might be mistaken
-- 
2.10.0.304.gf2ff484

[PATCH 0/4] git add --chmod: always change the file

[Thomas Gummerer]

Thanks to Peff and Junio for your inputs on the best way to solve this
problem.

The patch series is made up as follows:

[1/4]: Documentation for the chmod option
[2,3/4]: Small refactoring to simplify the final step
[4/4]: The actual change that introduces the new behaviour.

Thomas Gummerer (4):
  add: document the chmod option
  update-index: use the same structure for chmod as add
  read-cache: introduce chmod_index_entry
  add: modify already added files when --chmod is given

 Documentation/git-add.txt |  7 +-
 builtin/add.c | 36 +--
 builtin/checkout.c|  2 +-
 builtin/commit.c  |  2 +-
 builtin/update-index.c| 55 ++-
 cache.h   | 12 ++-
 read-cache.c  | 33 +---
 t/t3700-add.sh| 21 ++
 8 files changed, 107 insertions(+), 61 deletions(-)

-- 
2.10.0.304.gf2ff484

[PATCH 2/4] update-index: use the same structure for chmod as add

[Thomas Gummerer]

While the chmod options for update-index and the add have the same
functionality, they are using different ways to parse and handle the
option internally.  Unify these modes in order to make further
refactoring simpler.

Signed-off-by: Thomas Gummerer 
---
 builtin/update-index.c | 49 +
 1 file changed, 21 insertions(+), 28 deletions(-)

diff --git a/builtin/update-index.c b/builtin/update-index.c
index ba04b19..85a57db 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -419,11 +419,12 @@ static int add_cacheinfo(unsigned int mode, const 
unsigned char *sha1,
return 0;
 }
 
-static void chmod_path(int flip, const char *path)
+static void chmod_path(int force_mode, const char *path)
 {
int pos;
struct cache_entry *ce;
unsigned int mode;
+   char flip = force_mode == 0777 ? '+' : '-';
 
pos = cache_name_pos(path, strlen(path));
if (pos < 0)
@@ -432,17 +433,11 @@ static void chmod_path(int flip, const char *path)
mode = ce->ce_mode;
if (!S_ISREG(mode))
goto fail;
-   switch (flip) {
-   case '+':
-   ce->ce_mode |= 0111; break;
-   case '-':
-   ce->ce_mode &= ~0111; break;
-   default:
-   goto fail;
-   }
+   ce->ce_mode = create_ce_mode(force_mode);
cache_tree_invalidate_path(_index, path);
ce->ce_flags |= CE_UPDATE_IN_BASE;
active_cache_changed |= CE_ENTRY_CHANGED;
+
report("chmod %cx '%s'", flip, path);
return;
  fail:
@@ -788,16 +783,6 @@ static int really_refresh_callback(const struct option 
*opt,
return refresh(opt->value, REFRESH_REALLY);
 }
 
-static int chmod_callback(const struct option *opt,
-   const char *arg, int unset)
-{
-   char *flip = opt->value;
-   if ((arg[0] != '-' && arg[0] != '+') || arg[1] != 'x' || arg[2])
-   return error("option 'chmod' expects \"+x\" or \"-x\"");
-   *flip = arg[0];
-   return 0;
-}
-
 static int resolve_undo_clear_callback(const struct option *opt,
const char *arg, int unset)
 {
@@ -917,7 +902,8 @@ int cmd_update_index(int argc, const char **argv, const 
char *prefix)
int read_from_stdin = 0;
int prefix_length = prefix ? strlen(prefix) : 0;
int preferred_index_format = 0;
-   char set_executable_bit = 0;
+   char *chmod_arg = 0;
+   int force_mode = 0;
struct refresh_params refresh_args = {0, _errors};
int lock_error = 0;
int split_index = -1;
@@ -955,10 +941,8 @@ int cmd_update_index(int argc, const char **argv, const 
char *prefix)
PARSE_OPT_NOARG | /* disallow --cacheinfo= form */
PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,
(parse_opt_cb *) cacheinfo_callback},
-   {OPTION_CALLBACK, 0, "chmod", _executable_bit, N_("(+/-)x"),
-   N_("override the executable bit of the listed files"),
-   PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,
-   chmod_callback},
+   OPT_STRING( 0, "chmod", _arg, N_("(+/-)x"),
+   N_("override the executable bit of the listed files")),
{OPTION_SET_INT, 0, "assume-unchanged", _valid_only, NULL,
N_("mark files as \"not changing\""),
PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, MARK_FLAG},
@@ -1018,6 +1002,15 @@ int cmd_update_index(int argc, const char **argv, const 
char *prefix)
if (argc == 2 && !strcmp(argv[1], "-h"))
usage_with_options(update_index_usage, options);
 
+   if (!chmod_arg)
+   force_mode = 0;
+   else if (!strcmp(chmod_arg, "-x"))
+   force_mode = 0666;
+   else if (!strcmp(chmod_arg, "+x"))
+   force_mode = 0777;
+   else
+   die(_("option 'chmod' expects \"+x\" or \"-x\""));
+
git_config(git_default_config, NULL);
 
/* We can't free this memory, it becomes part of a linked list parsed 
atexit() */
@@ -1055,8 +1048,8 @@ int cmd_update_index(int argc, const char **argv, const 
char *prefix)
setup_work_tree();
p = prefix_path(prefix, prefix_length, path);
update_one(p);
-   if (set_executable_bit)
-   chmod_path(set_executable_bit, p);
+   if (force_mode)
+   chmod_path(force_mode, p);
free(p);
ctx.argc--;
ctx.argv++;
@@ -1100,8 +1093,8 @@ int cmd_update_index(int argc, const char **argv, const 
char *prefix)
}
p = prefix_path(prefix, prefix_length, buf.buf);

[PATCH 3/4] read-cache: introduce chmod_index_entry

[Thomas Gummerer]

As there are chmod options for both add and update-index, introduce a
new chmod_index_entry function to do the work.  Use it in update-index,
while it will be used in add in the next patch.

Signed-off-by: Thomas Gummerer 
---
 builtin/update-index.c |  8 +---
 cache.h|  2 ++
 read-cache.c   | 19 +++
 3 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/builtin/update-index.c b/builtin/update-index.c
index 85a57db..1569c81 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -423,20 +423,14 @@ static void chmod_path(int force_mode, const char *path)
 {
int pos;
struct cache_entry *ce;
-   unsigned int mode;
char flip = force_mode == 0777 ? '+' : '-';
 
pos = cache_name_pos(path, strlen(path));
if (pos < 0)
goto fail;
ce = active_cache[pos];
-   mode = ce->ce_mode;
-   if (!S_ISREG(mode))
+   if (chmod_cache_entry(ce, force_mode) < 0)
goto fail;
-   ce->ce_mode = create_ce_mode(force_mode);
-   cache_tree_invalidate_path(_index, path);
-   ce->ce_flags |= CE_UPDATE_IN_BASE;
-   active_cache_changed |= CE_ENTRY_CHANGED;
 
report("chmod %cx '%s'", flip, path);
return;
diff --git a/cache.h b/cache.h
index b780a91..44a4f76 100644
--- a/cache.h
+++ b/cache.h
@@ -369,6 +369,7 @@ extern void free_name_hash(struct index_state *istate);
 #define remove_file_from_cache(path) remove_file_from_index(_index, (path))
 #define add_to_cache(path, st, flags) add_to_index(_index, (path), (st), 
(flags), 0)
 #define add_file_to_cache(path, flags) add_file_to_index(_index, (path), 
(flags), 0)
+#define chmod_cache_entry(ce, force_mode) chmod_index_entry(_index, (ce), 
(force_mode))
 #define refresh_cache(flags) refresh_index(_index, (flags), NULL, NULL, 
NULL)
 #define ce_match_stat(ce, st, options) ie_match_stat(_index, (ce), (st), 
(options))
 #define ce_modified(ce, st, options) ie_modified(_index, (ce), (st), 
(options))
@@ -584,6 +585,7 @@ extern int remove_file_from_index(struct index_state *, 
const char *path);
 extern int add_to_index(struct index_state *, const char *path, struct stat *, 
int flags, int force_mode);
 extern int add_file_to_index(struct index_state *, const char *path, int 
flags, int force_mode);
 extern struct cache_entry *make_cache_entry(unsigned int mode, const unsigned 
char *sha1, const char *path, int stage, unsigned int refresh_options);
+extern int chmod_index_entry(struct index_state *, struct cache_entry *ce, int 
force_mode);
 extern int ce_same_name(const struct cache_entry *a, const struct cache_entry 
*b);
 extern void set_object_name_for_intent_to_add_entry(struct cache_entry *ce);
 extern int index_name_is_other(const struct index_state *, const char *, int);
diff --git a/read-cache.c b/read-cache.c
index 491e52d..367be57 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -756,6 +756,25 @@ struct cache_entry *make_cache_entry(unsigned int mode,
return ret;
 }
 
+/*
+ * Change the mode of an index entry to force_mode, where force_mode can
+ * either be 0777 or 0666.
+ * Returns -1 if the chmod for the particular cache entry failed (if it's
+ * not a regular file), 0 otherwise.
+ */
+int chmod_index_entry(struct index_state *istate, struct cache_entry *ce,
+  int force_mode)
+{
+   if (!S_ISREG(ce->ce_mode))
+   return -1;
+   ce->ce_mode = create_ce_mode(force_mode);
+   cache_tree_invalidate_path(istate, ce->name);
+   ce->ce_flags |= CE_UPDATE_IN_BASE;
+   istate->cache_changed |= CE_ENTRY_CHANGED;
+
+   return 0;
+}
+
 int ce_same_name(const struct cache_entry *a, const struct cache_entry *b)
 {
int len = ce_namelen(a);
-- 
2.10.0.304.gf2ff484

[PATCH 4/4] add: modify already added files when --chmod is given

[Thomas Gummerer]

When the chmod option was added to git add, it was hooked up to the diff
machinery, meaning that it only works when the version in the index
differs from the version on disk.

As the option was supposed to mirror the chmod option in update-index,
which always changes the mode in the index, regardless of the status of
the file, make sure the option behaves the same way in git add.

Reported-by: Jan Keromnes 
Signed-off-by: Thomas Gummerer 
---
 builtin/add.c  | 36 +---
 builtin/checkout.c |  2 +-
 builtin/commit.c   |  2 +-
 cache.h| 10 +-
 read-cache.c   | 14 ++
 t/t3700-add.sh | 21 +
 6 files changed, 59 insertions(+), 26 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index b1dddb4..892198a 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -26,10 +26,25 @@ static int patch_interactive, add_interactive, 
edit_interactive;
 static int take_worktree_changes;
 
 struct update_callback_data {
-   int flags, force_mode;
+   int flags;
int add_errors;
 };
 
+static void chmod_pathspec(struct pathspec *pathspec, int force_mode)
+{
+   int i;
+   
+   for (i = 0; i < active_nr; i++) {
+   struct cache_entry *ce = active_cache[i];
+
+   if (pathspec && !ce_path_match(ce, pathspec, NULL))
+   continue;
+
+   if (chmod_cache_entry(ce, force_mode) < 0)
+   fprintf(stderr, "cannot chmod '%s'", ce->name);
+   }
+}
+
 static int fix_unmerged_status(struct diff_filepair *p,
   struct update_callback_data *data)
 {
@@ -65,8 +80,7 @@ static void update_callback(struct diff_queue_struct *q,
die(_("unexpected diff status %c"), p->status);
case DIFF_STATUS_MODIFIED:
case DIFF_STATUS_TYPE_CHANGED:
-   if (add_file_to_index(_index, path,
-   data->flags, data->force_mode)) {
+   if (add_file_to_index(_index, path, data->flags)) {
if (!(data->flags & ADD_CACHE_IGNORE_ERRORS))
die(_("updating files failed"));
data->add_errors++;
@@ -84,15 +98,14 @@ static void update_callback(struct diff_queue_struct *q,
}
 }
 
-int add_files_to_cache(const char *prefix, const struct pathspec *pathspec,
-   int flags, int force_mode)
+int add_files_to_cache(const char *prefix,
+  const struct pathspec *pathspec, int flags)
 {
struct update_callback_data data;
struct rev_info rev;
 
memset(, 0, sizeof(data));
data.flags = flags;
-   data.force_mode = force_mode;
 
init_revisions(, prefix);
setup_revisions(0, NULL, , NULL);
@@ -281,7 +294,7 @@ static int add_config(const char *var, const char *value, 
void *cb)
return git_default_config(var, value, cb);
 }
 
-static int add_files(struct dir_struct *dir, int flags, int force_mode)
+static int add_files(struct dir_struct *dir, int flags)
 {
int i, exit_status = 0;
 
@@ -294,8 +307,7 @@ static int add_files(struct dir_struct *dir, int flags, int 
force_mode)
}
 
for (i = 0; i < dir->nr; i++)
-   if (add_file_to_index(_index, dir->entries[i]->name,
-   flags, force_mode)) {
+   if (add_file_to_index(_index, dir->entries[i]->name, 
flags)) {
if (!ignore_add_errors)
die(_("adding files failed"));
exit_status = 1;
@@ -441,11 +453,13 @@ int cmd_add(int argc, const char **argv, const char 
*prefix)
 
plug_bulk_checkin();
 
-   exit_status |= add_files_to_cache(prefix, , flags, force_mode);
+   exit_status |= add_files_to_cache(prefix, , flags);
 
if (add_new_files)
-   exit_status |= add_files(, flags, force_mode);
+   exit_status |= add_files(, flags);
 
+   if (force_mode)
+   chmod_pathspec(, force_mode);
unplug_bulk_checkin();
 
 finish:
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 8672d07..a83c78f 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -548,7 +548,7 @@ static int merge_working_tree(const struct checkout_opts 
*opts,
 * entries in the index.
 */
 
-   add_files_to_cache(NULL, NULL, 0, 0);
+   add_files_to_cache(NULL, NULL, 0);
/*
 * NEEDSWORK: carrying over local changes
 * when branches have different end-of-line
diff --git a/builtin/commit.c b/builtin/commit.c
index 77e3dc8..7a1ade0 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -387,7 +387,7 @@ static const char *prepare_index(int 

Re: [PATCH] stash: allow ref of a stash by index

[Philip Oakley]

From: "Johannes Schindelin" 

Hi,

On Sat, 3 Sep 2016, Jeff King wrote:


On Sat, Sep 03, 2016 at 07:21:18PM -0400, Aaron M Watson wrote:

> Allows stashes to be referenced by index only. Instead of referencing
> "stash@{n}" explicitly, it can simply be referenced as "n".

This says "what" but not "why". I assume it is "because the former is
more annoying to type".

Are there any backwards-compatibility issues you can think of?

I think that "123456" could be a sha1, but I do not see much point in
referencing a sha1 as the argument of "stash show". And it looks like
this code path is called only from is_stash_like(), so presumably the
same logic would apply to other callers.


Maybe we could make it unambiguous, e.g. by using # instead: #123456
cannot refer to a SHA-1.


The alternative is to limit the length to less that the shortest ambiguous 
sha1 length that has been used (by Git users - 5, 6, 7? )? Which is probably 
allowing 1-4 characters, which is a reasonbly deep stash index...


If you need to refer to stash@{9362} you have bigger problems.


But then, '#' are comment-starting in shells, so they would have to by
escaped. Maybe the best option would be to introduce a -n  option,
with the shortcut - thanks to e0319ff (parseopt: add
OPT_NUMBER_CALLBACK, 2009-05-07).

Ciao,
Johannes

Re: [RFC/PATCH 2/2] WIP xdiff: markup duplicates differently

[Jakub Narębski]

W dniu 04.09.2016 o 07:31, Stefan Beller pisze:
> On Sat, Sep 3, 2016 at 5:25 AM, Jakub Narębski  wrote:
>> W dniu 03.09.2016 o 05:31, Stefan Beller pisze:
>>
>>> When moving code (e.g. a function is moved to another part of the file or
>>> to a different file), the review process is different than reviewing new
>>> code. When reviewing moved code we are only interested in the diff as
>>> where there are differences in the moved code, e.g. namespace changes.
>>>
>>> However the inner part of these moved texts should not change.
>>> To aid a developer reviewing such code, emit it with a different prefix
>>> than the usual +,- to indicate it is overlapping code.
>>
>> What would be this different prefix?
> 
> I will discard the part of the different prefix as the design of 2/2
> will change.

It would be nice to have at least an option of using different prefix
(or pair of prefixes), as not always it is possible to use color to
markup duplicates.

P.S. BTW. does this work with word-diff?

Best regards,
-- 
Jakub Narębski

Re: [RFC/PATCH 0/2] Color moved code differently

[Jacob Keller]

On Fri, Sep 2, 2016 at 8:31 PM, Stefan Beller  wrote:
> When moving code (e.g. a function is moved to another part of the file or
> to a different file), the review process is different than reviewing new
> code. When reviewing moved code we are only interested in the diff as
> where there are differences in the moved code, e.g. namespace changes.
>
> However the inner part of these moved texts should not change.
> To aid a developer reviewing such code, we'll color pure moved stuff
> differently.
>
> A line is colored differently if that line and the surroundign 2 lines
> appear as-is in the opposite part of the diff.
>
> Example:
> http://i.imgur.com/ay84q0q.png
>

In the example, the first and last lines of duplicate copies don't get
colored differently, and that threw  me off. I feel like that was
maybe not intentional? If it was, can you explain why?

Thanks,
Jake