date:20170325

On Sat, Mar 25, 2017 at 9:40 AM, demerphq  wrote:
> On 25 March 2017 at 00:51, Ævar Arnfjörð Bjarmason  wrote:
>> They're changing their license[1] to Apache 2 which unlike the current
>> fuzzy compatibility with the current license[2] is explicitly
>> incompatible with GPLv2[3].
>
> Are you sure there is an issue? From the Apache page on this:
>
> Apache 2 software can therefore be included in GPLv3 projects, because
> the GPLv3 license accepts our software into GPLv3 works. However,
> GPLv3 software cannot be included in Apache projects. The licenses are
> incompatible in one direction only, and it is a result of ASF's
> licensing philosophy and the GPLv3 authors' interpretation of
> copyright law.
>
> Which seems to be the opposite of the concern you are expressing.

The Apache 2 license is indeed compatible with the GPLv3, but the Git
project explicitly uses GPLv2 with no "or later" clause:

$ head -n 18 COPYING

 Note that the only valid version of the GPL as far as this project
 is concerned is _this_ particular version of the license (ie v2, not
 v2.2 or v3.x or whatever), unless explicitly otherwise stated.

 HOWEVER, in order to allow a migration to GPLv3 if that seems like
 a good idea, I also ask that people involved with the project make
 their preferences known. In particular, if you trust me to make that
 decision, you might note so in your copyright message, ie something
 like

This file is licensed under the GPL v2, or a later version
at the discretion of Linus.

  might avoid issues. But we can also just decide to synchronize and
  contact all copyright holders on record if/when the occasion arises.

Linus Torvalds

t1503 broken ?

2017-03-25 Thread Torsten Bögershausen

./t1305-config-include.sh
seems to be broken:
not ok 19 - conditional include, $HOME expansion
not ok 21 - conditional include, relative path

Both Mac and Linux.

The problem seems to be the line

git config test.two >actual &&
and
git config test.four >actual &&

In both cases the config "test.two or test.four" is not found,
at least that is what my debugging shows.
After adding an
exit 0
before the test_done gives this:

 find trash\ directory.t1305-config-include/ -name "bar*"
trash directory.t1305-config-include/foo/.git/bar
trash directory.t1305-config-include/foo/.git/bar3
trash directory.t1305-config-include/foo/.git/bar5
trash directory.t1305-config-include/foo/.git/bar2
trash directory.t1305-config-include/bar4

Where should bar2 and bar4 be ?

[PATCH v2] [GSoC] remove_subtree(): reimplement using iterators

Use dir_iterator to traverse through remove_subtree()'s directory tree,
avoiding the need for recursive calls to readdir(). Simplify
remove_subtree()'s code.

A conversion similar in purpose was previously done at 46d092a
("for_each_reflog(): reimplement using iterators", 2016-05-21).

Signed-off-by: Daniel Ferreira 
---

This is a second-version patch of the Google Summer of Code microproject for
refactoring recursive readdir() calls to use dir_iterator instead. v1 can be
found in:

https://public-inbox.org/git/CAGZ79kZwT-9mHTiOJ5CEjk2wDFkn6+NcogjX0=vjhsah16a...@mail.gmail.com/T/#mae023e7a7d7626f00e0923833c4359f5af493730

Additionally, for debugging purposes I turned remove_subtree() into a no-op
and ran git tests. Some failures were at:

* t2000-checkout-cache-clash.sh
* t2003-checkout-cache-mkdir.sh

If you guys could check those files out and warn me if any additional tests
would be welcome, please let me know.

Thanks.

 entry.c | 28 +++-
 1 file changed, 7 insertions(+), 21 deletions(-)

diff --git a/entry.c b/entry.c
index c6eea240b..3cb92592d 100644
--- a/entry.c
+++ b/entry.c
@@ -2,6 +2,8 @@
 #include "blob.h"
 #include "dir.h"
 #include "streaming.h"
+#include "iterator.h"
+#include "dir-iterator.h"

 static void create_directories(const char *path, int path_len,
   const struct checkout *state)
@@ -46,29 +48,13 @@ static void create_directories(const char *path, int 
path_len,

 static void remove_subtree(struct strbuf *path)
 {
-   DIR *dir = opendir(path->buf);
-   struct dirent *de;
-   int origlen = path->len;
-
-   if (!dir)
-   die_errno("cannot opendir '%s'", path->buf);
-   while ((de = readdir(dir)) != NULL) {
-   struct stat st;
-
-   if (is_dot_or_dotdot(de->d_name))
-   continue;
-
-   strbuf_addch(path, '/');
-   strbuf_addstr(path, de->d_name);
-   if (lstat(path->buf, &st))
-   die_errno("cannot lstat '%s'", path->buf);
-   if (S_ISDIR(st.st_mode))
-   remove_subtree(path);
-   else if (unlink(path->buf))
+   struct dir_iterator *diter = dir_iterator_begin(path->buf);
+
+   while (dir_iterator_advance(diter) == ITER_OK) {
+   if (unlink(diter->path.buf))
die_errno("cannot unlink '%s'", path->buf);
-   strbuf_setlen(path, origlen);
}
-   closedir(dir);
+
if (rmdir(path->buf))
die_errno("cannot rmdir '%s'", path->buf);
 }
--
2.12.1.433.g82305b74f.dirty

Re: [PATCH v2] [GSoC] remove_subtree(): reimplement using iterators

On Sat, Mar 25, 2017 at 6:27 PM, Daniel Ferreira  wrote:
> Use dir_iterator to traverse through remove_subtree()'s directory tree,
> avoiding the need for recursive calls to readdir(). Simplify
> remove_subtree()'s code.
>
> A conversion similar in purpose was previously done at 46d092a
> ("for_each_reflog(): reimplement using iterators", 2016-05-21).
>
> Signed-off-by: Daniel Ferreira 
> ---
>
> This is a second-version patch of the Google Summer of Code microproject for
> refactoring recursive readdir() calls to use dir_iterator instead. v1 can be
> found in:
>
> https://public-inbox.org/git/CAGZ79kZwT-9mHTiOJ5CEjk2wDFkn6+NcogjX0=vjhsah16a...@mail.gmail.com/T/#mae023e7a7d7626f00e0923833c4359f5af493730
>
> Additionally, for debugging purposes I turned remove_subtree() into a no-op
> and ran git tests. Some failures were at:
>
> * t2000-checkout-cache-clash.sh
> * t2003-checkout-cache-mkdir.sh
>
> If you guys could check those files out and warn me if any additional tests
> would be welcome, please let me know.
>
> Thanks.
>
>  entry.c | 28 +++-
>  1 file changed, 7 insertions(+), 21 deletions(-)
>
> diff --git a/entry.c b/entry.c
> index c6eea240b..3cb92592d 100644
> --- a/entry.c
> +++ b/entry.c
> @@ -2,6 +2,8 @@
>  #include "blob.h"
>  #include "dir.h"
>  #include "streaming.h"
> +#include "iterator.h"
> +#include "dir-iterator.h"
>
>  static void create_directories(const char *path, int path_len,
>const struct checkout *state)
> @@ -46,29 +48,13 @@ static void create_directories(const char *path, int 
> path_len,
>
>  static void remove_subtree(struct strbuf *path)
>  {
> -   DIR *dir = opendir(path->buf);
> -   struct dirent *de;
> -   int origlen = path->len;
> -
> -   if (!dir)
> -   die_errno("cannot opendir '%s'", path->buf);
> -   while ((de = readdir(dir)) != NULL) {
> -   struct stat st;
> -
> -   if (is_dot_or_dotdot(de->d_name))
> -   continue;
> -
> -   strbuf_addch(path, '/');
> -   strbuf_addstr(path, de->d_name);
> -   if (lstat(path->buf, &st))
> -   die_errno("cannot lstat '%s'", path->buf);
> -   if (S_ISDIR(st.st_mode))
> -   remove_subtree(path);
> -   else if (unlink(path->buf))
> +   struct dir_iterator *diter = dir_iterator_begin(path->buf);
> +
> +   while (dir_iterator_advance(diter) == ITER_OK) {
> +   if (unlink(diter->path.buf))
> die_errno("cannot unlink '%s'", path->buf);
> -   strbuf_setlen(path, origlen);
> }
> -   closedir(dir);
> +
> if (rmdir(path->buf))
> die_errno("cannot rmdir '%s'", path->buf);

Even though it's very nice that lots of code is deleted. This is not
entirely correct, is it? Before this patch, rmdir() is called for
every recursive remove_subtree() call. After this patch, it's only
called once (and likely fails unless you have no subdirectories).

>  }
> --
> 2.12.1.433.g82305b74f.dirty
>



-- 
Duy

Re: [PATCH v6 24/27] t/helper: add test-ref-store to test ref-store functions

On Wed, Mar 22, 2017 at 8:37 PM, Jeff King  wrote:
> On Wed, Mar 22, 2017 at 09:34:12AM -0400, Jeff King wrote:
>
>> On Sat, Mar 18, 2017 at 09:03:34AM +0700, Nguyễn Thái Ngọc Duy wrote:
>>
>> > diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c
>> > new file mode 100644
>>
>> When nd/files-backend-git-dir is merged to 'next', the new tests in
>> t1405/t1406 break. They need this on top (which should probably just be
>> squashed into this commit when you re-roll).
>>
>> -- >8 --
>> Subject: [PATCH] test-ref-store: setup git directory
>>
>> Without setting up the git directory, we rely on the ".git"
>> fallback in setup_git_env(). This will cause us to abort
>> once b1ef400ee (setup_git_env: avoid blind fall-back to
>> ".git", 2016-10-20) is merged.
>
> After posting this, it occurred to me that "pu" should be showing the
> same failure, but it doesn't. The reason is that e9e167145 (worktree.c:
> kill parse_ref() in favor of refs_resolve_ref_unsafe(), 2017-03-18) from
> nd/worktree-kill-parse-ref sneaks in the setup call.
>
> We should move it back to the addition of test-ref-store, though, since
> nd/files-backend-git-dir may graduate separately.

Right. I needed that setup_git_directory() for the third ref store.
But now that I think about it, I need it for the first two as well
because git_path() is always involved. Will fix (since it looks like
files-backend-git-dir is not merged to next yet).
-- 
Duy

Re: t1503 broken ?

On Sat, Mar 25, 2017 at 5:46 PM, Torsten Bögershausen  wrote:
> ./t1305-config-include.sh
> seems to be broken:
> not ok 19 - conditional include, $HOME expansion
> not ok 21 - conditional include, relative path

let me guess, your "git" directory is in a symlink path?
-- 
Duy

Re: [PATCH v1 0/3] Add support for downloading blobs on demand

On Wed, Mar 22, 2017 at 11:52 PM, Ben Peart  wrote:
> We have a couple of patch series we’re working on (ObjectDB/Read-Object,
> Watchman integration)

Oops, sorry. I should be reworking the index-helper series for
watchman support, but I haven't time for it. Yes I'm also eyeing Lars
code as the communication channel for any external helper (which in
turn can talk to watchman or whatever). I guess I'll let you do it
then.
-- 
Duy

Re: [PATCH] Documentation/git-worktree: use working tree for trees on the file system

On Tue, Mar 21, 2017 at 10:48 PM, Junio C Hamano  wrote:
> Duy Nguyen  writes:
>
>> On Tue, Mar 21, 2017 at 1:50 AM, Jonathan Nieder  wrote:
>>> Junio C Hamano wrote:
 Stefan Beller  writes:
>>>
> While it may be true that you can have bare worktrees; I would question
> why anyone wants to do this, as the only thing it provides is an
> additional HEAD (plus its reflog).

 A more plausible situation is you start with a bare one as the
 primary and used to make local clones to do your work in the world
 before "git worktree".  It would be a natural extension to your
 workflow to instead create worktrees of of that bare one as the
 primary worktree with secondaries with working trees.
>>>
>>> For what it's worth, this conversation makes me think it was a mistake
>>> to call this construct a worktree.
>>
>> For the record, I am totally confused with Junio's last line, with two
>> "with"s, "worktree" and "working trees" in the same phrase :D
>
> In case this wasn't just a tangential note, what I meant was:
>
>  - In the old world, you may have had a single bare repository and
>then made clones, each of which has a working tree (i.e. non-bare
>clones), and worked inside these clones.
>
>  - In the "git worktree" world, you can start from that same single
>bare repository, but instead of cloning it, use "git worktree" to
>create "worktree"s, each of which has a working tree, and work
>inside these "worktree"s.
>
> and the latter would be a natural extension to the workflow the
> former wanted to use.

Yes I really want that, and even the ability to convert a normal one
repo (with one working tree) to the latter, moving the repository to
somewhere safe.

>>> It's fine for the command to have one name and the documentation to
>>> use a longer, clearer name to explain it.  What should that longer,
>>> clearer name be?
>>
>> No comments from me. I'll let you know that if Eric (or Junio?) didn't
>> stop me, we would have had $GIT_DIR/repos now instead of
>> $GIT_DIR/worktrees, just some extra confusion toppings.
>
> I forgot about that part of the history, but you are saying you
> wanted to call these "repos", not "worktrees"?

>From $GIT_DIR perspective (which points to
$GIT_COMMON_DIR/worktrees/blah) then they do look like a repository
with lots of part borrowed from $GIT_COMMON_DIR. I was simply saying
I'm bad at naming things. "worktrees" is a better name than "repos".

> I can see why
> somebody (or me?) would stop that by fearing "repo" is a bit too
> confusing with a "repository", in the same way that we are now
> realizing that "worktree" is too similar to an old synonym we used
> to call "working tree".
-- 
Duy

Re: [PATCH] Documentation/git-worktree: use working tree for trees on the file system

On Fri, Mar 24, 2017 at 12:55 AM, Junio C Hamano  wrote:
> Michael J Gruber  writes:
>
>> Are we at a point where we can still rename the new feature at least? If
>> yes, and keeping everything else is mandatory, than "workspace" or
>> "working space" may be a serious contender for naming the new thing.
>
> I do not have a good answer to the first question, but workspace
> does sound like a good name for what this feature is trying to
> achieve.
>

Now is not too late to rename the command from worktree to workspace
(and keep "worktree" as an alias that will be eventually deleted).
Should we do it? I would keep file names, function names... unchanged
though, not worth the amount of new conflicts.
-- 
Duy

Re: [PATCH v2] [GSoC] remove_subtree(): reimplement using iterators

2017-03-25 Thread Daniel Ferreira (theiostream)

You are correct, which shows that since all tests pass, we need to
come up with better cases for this function.

As for a solution, I believe that the best way to go for it is to
dir_iterator's implementation to have an "Option to iterate over
directory paths before vs. after their contents" (something predicted
in the commit that created it). If it iterates over directories after
all of its contents (currently it does so before) we just need to
check if the entry is a directory and if so, rmdir() it. Does that
make sense?

On Sat, Mar 25, 2017 at 8:51 AM, Duy Nguyen  wrote:
> On Sat, Mar 25, 2017 at 6:27 PM, Daniel Ferreira  wrote:
>> Use dir_iterator to traverse through remove_subtree()'s directory tree,
>> avoiding the need for recursive calls to readdir(). Simplify
>> remove_subtree()'s code.
>>
>> A conversion similar in purpose was previously done at 46d092a
>> ("for_each_reflog(): reimplement using iterators", 2016-05-21).
>>
>> Signed-off-by: Daniel Ferreira 
>> ---
>>
>> This is a second-version patch of the Google Summer of Code microproject for
>> refactoring recursive readdir() calls to use dir_iterator instead. v1 can be
>> found in:
>>
>> https://public-inbox.org/git/CAGZ79kZwT-9mHTiOJ5CEjk2wDFkn6+NcogjX0=vjhsah16a...@mail.gmail.com/T/#mae023e7a7d7626f00e0923833c4359f5af493730
>>
>> Additionally, for debugging purposes I turned remove_subtree() into a no-op
>> and ran git tests. Some failures were at:
>>
>> * t2000-checkout-cache-clash.sh
>> * t2003-checkout-cache-mkdir.sh
>>
>> If you guys could check those files out and warn me if any additional tests
>> would be welcome, please let me know.
>>
>> Thanks.
>>
>>  entry.c | 28 +++-
>>  1 file changed, 7 insertions(+), 21 deletions(-)
>>
>> diff --git a/entry.c b/entry.c
>> index c6eea240b..3cb92592d 100644
>> --- a/entry.c
>> +++ b/entry.c
>> @@ -2,6 +2,8 @@
>>  #include "blob.h"
>>  #include "dir.h"
>>  #include "streaming.h"
>> +#include "iterator.h"
>> +#include "dir-iterator.h"
>>
>>  static void create_directories(const char *path, int path_len,
>>const struct checkout *state)
>> @@ -46,29 +48,13 @@ static void create_directories(const char *path, int 
>> path_len,
>>
>>  static void remove_subtree(struct strbuf *path)
>>  {
>> -   DIR *dir = opendir(path->buf);
>> -   struct dirent *de;
>> -   int origlen = path->len;
>> -
>> -   if (!dir)
>> -   die_errno("cannot opendir '%s'", path->buf);
>> -   while ((de = readdir(dir)) != NULL) {
>> -   struct stat st;
>> -
>> -   if (is_dot_or_dotdot(de->d_name))
>> -   continue;
>> -
>> -   strbuf_addch(path, '/');
>> -   strbuf_addstr(path, de->d_name);
>> -   if (lstat(path->buf, &st))
>> -   die_errno("cannot lstat '%s'", path->buf);
>> -   if (S_ISDIR(st.st_mode))
>> -   remove_subtree(path);
>> -   else if (unlink(path->buf))
>> +   struct dir_iterator *diter = dir_iterator_begin(path->buf);
>> +
>> +   while (dir_iterator_advance(diter) == ITER_OK) {
>> +   if (unlink(diter->path.buf))
>> die_errno("cannot unlink '%s'", path->buf);
>> -   strbuf_setlen(path, origlen);
>> }
>> -   closedir(dir);
>> +
>> if (rmdir(path->buf))
>> die_errno("cannot rmdir '%s'", path->buf);
>
> Even though it's very nice that lots of code is deleted. This is not
> entirely correct, is it? Before this patch, rmdir() is called for
> every recursive remove_subtree() call. After this patch, it's only
> called once (and likely fails unless you have no subdirectories).
>
>>  }
>> --
>> 2.12.1.433.g82305b74f.dirty
>>
>
>
>
> --
> Duy

[PATCH] pretty: add extra headers and MIME boundary directly

2017-03-25 Thread René Scharfe

Use the after_subject member of struct pretty_print_context to pass the
extra_headers unchanged, and construct and add the MIME boundary headers
directly in pretty.c::pp_title_line() instead of writing both to a
static buffer in log-tree.c::log_write_email_headers() first.  That's
easier, quicker and gets rid of said static buffer.

Signed-off-by: Rene Scharfe 
---
 builtin/log.c |  3 ++-
 log-tree.c| 26 ++
 log-tree.h|  1 -
 pretty.c  | 15 +++
 4 files changed, 19 insertions(+), 26 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 281af8c1ec..be564039c1 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -989,7 +989,8 @@ static void make_cover_letter(struct rev_info *rev, int 
use_stdout,
open_next_file(NULL, rev->numbered_files ? NULL : "cover-letter", 
rev, quiet))
return;
 
-   log_write_email_headers(rev, head, &pp.after_subject, &need_8bit_cte);
+   log_write_email_headers(rev, head, &need_8bit_cte);
+   pp.after_subject = rev->extra_headers;
 
for (i = 0; !need_8bit_cte && i < nr; i++) {
const char *buf = get_commit_buffer(list[i], NULL);
diff --git a/log-tree.c b/log-tree.c
index 4618dd04ca..7049a17781 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -349,10 +349,8 @@ void fmt_output_email_subject(struct strbuf *sb, struct 
rev_info *opt)
 }
 
 void log_write_email_headers(struct rev_info *opt, struct commit *commit,
-const char **extra_headers_p,
 int *need_8bit_cte_p)
 {
-   const char *extra_headers = opt->extra_headers;
const char *name = oid_to_hex(opt->zero_commit ?
  &null_oid : &commit->object.oid);
 
@@ -374,26 +372,9 @@ void log_write_email_headers(struct rev_info *opt, struct 
commit *commit,
graph_show_oneline(opt->graph);
}
if (opt->mime_boundary) {
-   static char subject_buffer[1024];
static char buffer[1024];
struct strbuf filename =  STRBUF_INIT;
*need_8bit_cte_p = -1; /* NEVER */
-   snprintf(subject_buffer, sizeof(subject_buffer) - 1,
-"%s"
-"MIME-Version: 1.0\n"
-"Content-Type: multipart/mixed;"
-" boundary=\"%s%s\"\n"
-"\n"
-"This is a multi-part message in MIME "
-"format.\n"
-"--%s%s\n"
-"Content-Type: text/plain; "
-"charset=UTF-8; format=fixed\n"
-"Content-Transfer-Encoding: 8bit\n\n",
-extra_headers ? extra_headers : "",
-mime_boundary_leader, opt->mime_boundary,
-mime_boundary_leader, opt->mime_boundary);
-   extra_headers = subject_buffer;
 
if (opt->numbered_files)
strbuf_addf(&filename, "%d", opt->nr);
@@ -413,7 +394,6 @@ void log_write_email_headers(struct rev_info *opt, struct 
commit *commit,
opt->diffopt.stat_sep = buffer;
strbuf_release(&filename);
}
-   *extra_headers_p = extra_headers;
 }
 
 static void show_sig_lines(struct rev_info *opt, int status, const char *bol)
@@ -537,7 +517,6 @@ void show_log(struct rev_info *opt)
struct log_info *log = opt->loginfo;
struct commit *commit = log->commit, *parent = log->parent;
int abbrev_commit = opt->abbrev_commit ? opt->abbrev : 40;
-   const char *extra_headers = opt->extra_headers;
struct pretty_print_context ctx = {0};
 
opt->loginfo = NULL;
@@ -597,8 +576,7 @@ void show_log(struct rev_info *opt)
 */
 
if (cmit_fmt_is_mail(opt->commit_format)) {
-   log_write_email_headers(opt, commit, &extra_headers,
-   &ctx.need_8bit_cte);
+   log_write_email_headers(opt, commit, &ctx.need_8bit_cte);
ctx.rev = opt;
ctx.print_email_subject = 1;
} else if (opt->commit_format != CMIT_FMT_USERFORMAT) {
@@ -672,7 +650,7 @@ void show_log(struct rev_info *opt)
ctx.date_mode = opt->date_mode;
ctx.date_mode_explicit = opt->date_mode_explicit;
ctx.abbrev = opt->diffopt.abbrev;
-   ctx.after_subject = extra_headers;
+   ctx.after_subject = opt->extra_headers;
ctx.preserve_subject = opt->preserve_subject;
ctx.reflog_info = opt->reflog_info;
ctx.fmt = opt->commit_format;
diff --git a/log-tree.h b/log-tree.h
index 48f11fb740..7f9c4f22b5 100644
--- a/log-tree.h
+++ b/log-tree.h
@@ -22,7 +22,6 @@ void format_decorations_extended(struct strbuf *sb, const 
struct commit *commit,
 format_decorations_extended((strbuf), (commit), 
(color), " (", ", ", ")")
 void show_

Re: t1503 broken ?

On Sat, Mar 25, 2017 at 6:46 PM, Duy Nguyen  wrote:
> On Sat, Mar 25, 2017 at 5:46 PM, Torsten Bögershausen  wrote:
>> ./t1305-config-include.sh
>> seems to be broken:
>> not ok 19 - conditional include, $HOME expansion
>> not ok 21 - conditional include, relative path
>
> let me guess, your "git" directory is in a symlink path?

Yes I could reproduce it when I put my "git" in a symlink. There's a
note in document about "Symlinks in `$GIT_DIR` are not resolved before
matching" but failing tests is not acceptable. I'll fix it.
-- 
Duy

Re: [PATCH v2] [GSoC] remove_subtree(): reimplement using iterators

On Sat, Mar 25, 2017 at 7:13 PM, Daniel Ferreira (theiostream)
 wrote:
> You are correct, which shows that since all tests pass, we need to
> come up with better cases for this function.
>
> As for a solution, I believe that the best way to go for it is to
> dir_iterator's implementation to have an "Option to iterate over
> directory paths before vs. after their contents" (something predicted
> in the commit that created it). If it iterates over directories after
> all of its contents (currently it does so before) we just need to
> check if the entry is a directory and if so, rmdir() it. Does that
> make sense?

Yes. However since this is a microproject, intended to be done quickly
just to get you familiarize with the community and the process, I
suggest you look for another readdir() place that can be easily
converted to using dir-iterator first so you could go through the
whole thing. But of course I will not object you improving
dir-iterator and clean remove_subtree() up ;-)
-- 
Duy

Re: t1503 broken ?

On Sat, Mar 25, 2017 at 07:26:14PM +0700, Duy Nguyen wrote:
> On Sat, Mar 25, 2017 at 6:46 PM, Duy Nguyen  wrote:
> > On Sat, Mar 25, 2017 at 5:46 PM, Torsten Bögershausen  wrote:
> >> ./t1305-config-include.sh
> >> seems to be broken:
> >> not ok 19 - conditional include, $HOME expansion
> >> not ok 21 - conditional include, relative path
> >
> > let me guess, your "git" directory is in a symlink path?
> 
> Yes I could reproduce it when I put my "git" in a symlink. There's a
> note in document about "Symlinks in `$GIT_DIR` are not resolved before
> matching" but failing tests is not acceptable. I'll fix it.

The fix may be something like this. The problem is $GIT_DIR has symlinks
resolved, but we don't do the same for other paths in this code. As a
result, matching paths fails.

I'm a bit concerned about the change in expand_user_path() because I'm
not quite sure if it's a completely safe change. But at least could
you try the patch and see if it passe the tests on your machine too?

-- 8< --
diff --git a/config.c b/config.c
index 1a4d855..fc4eae9 100644
--- a/config.c
+++ b/config.c
@@ -191,7 +191,7 @@ static int prepare_include_condition_pattern(struct strbuf 
*pat)
return error(_("relative config include "
   "conditionals must come from files"));
 
-   strbuf_add_absolute_path(&path, cf->path);
+   strbuf_realpath(&path, cf->path, 1);
slash = find_last_dir_sep(path.buf);
if (!slash)
die("BUG: how is this possible?");
@@ -213,7 +213,7 @@ static int include_by_gitdir(const char *cond, size_t 
cond_len, int icase)
struct strbuf pattern = STRBUF_INIT;
int ret = 0, prefix;
 
-   strbuf_add_absolute_path(&text, get_git_dir());
+   strbuf_realpath(&text, get_git_dir(), 1);
strbuf_add(&pattern, cond, cond_len);
prefix = prepare_include_condition_pattern(&pattern);
 
diff --git a/path.c b/path.c
index 2224843..18eaac3 100644
--- a/path.c
+++ b/path.c
@@ -654,7 +654,7 @@ char *expand_user_path(const char *path)
const char *home = getenv("HOME");
if (!home)
goto return_null;
-   strbuf_addstr(&user_path, home);
+   strbuf_addstr(&user_path, real_path(home));
 #ifdef GIT_WINDOWS_NATIVE
convert_slashes(user_path.buf);
 #endif
-- 8< --
-- 
Duy

Re: [PATCH v3 2/2] l10n: Add git-add.txt to localized man pages

2017-03-25 Thread Jean-Noël AVILA

Le mercredi 22 mars 2017 11:02:09 CET, vous avez écrit :
> Jean-Noël Avila  writes:
> >> I am wondering if Documentation/po part should be a separate
> >> repository, with a dedicated i18n/l10n coordinator.  Would it make
> >> it easier for (1) those who write code and doc without knowing other
> >> languages, (2) those who update .pot and coordinate the l10n effort
> >> for the documentation and (3) those who translate them if we keep
> >> them in a single repository?
> > 
> > This is one of the points raised in the first RFC mail. Splitting this
> > part would help a lot manage the translations with their own workflow,
> > would not clutter the main repo with files not really needed for
> > packaging and would simplify dealing with the interaction with crowd
> > translation websites which can directly push translation content to a
> > git repo.
> 
> As I was in favor of splitting it out, I was trying to gauge what
> the downside of doing so would be, especially for those who are
> doing the translation work (it is obvious that it would help
> developers who are not translators, as nothing will change for them
> if we keep this new thing as a separate project).

There's one big downside of  this splitting. The gitman-l10n project would not 
be autonomous without the specific cloning at the particular place in the git 
project. po4a needs the original asciidoc files to perform the transclusion of 
the translated content into the structure of the documents. The setup that you 
are proposing would rule out simple CI checks and would make it complicated 
for the translators to set up their working copy and check the resulting man 
pages.

As I see it, there's the need for the Documentation folder to be contained in 
both project (while remaining the property of the git project). So I would 
think the other way around: for those interested in translated the 
documentation, some script would allow to checkout the git project inside the 
gitman-l10n project (like a kind of library).

This would be mainly transparent for the git developers.

> I'd prefer to start with the "optional gitman-l10n repository is
> checked out at Documentation/po only by convention" approach, before
> committing to bind it as a submodule.  Once we got comfortable with
> cooperating between these two projects, we do want to bind them
> using the submodule mechanism, but not before.

Obviously, my proposition would not allow to evolve towards such a setup, but 
is it really needed anyway ?

Re: [PATCH] pretty: add extra headers and MIME boundary directly

On Sat, Mar 25, 2017 at 01:16:42PM +0100, René Scharfe wrote:

> Use the after_subject member of struct pretty_print_context to pass the
> extra_headers unchanged, and construct and add the MIME boundary headers
> directly in pretty.c::pp_title_line() instead of writing both to a
> static buffer in log-tree.c::log_write_email_headers() first.  That's
> easier, quicker and gets rid of said static buffer.

I'm definitely pleased with the direction. A few comments:

> @@ -374,26 +372,9 @@ void log_write_email_headers(struct rev_info *opt, 
> struct commit *commit,
>   graph_show_oneline(opt->graph);
>   }
>   if (opt->mime_boundary) {
> - static char subject_buffer[1024];
>   static char buffer[1024];

We still have this other buffer, which ends up in stat_sep. It should
probably get the same treatment, though I think the module boundaries
make it a little more awkward. We look at it in diff_flush(), which
otherwise doesn't need to know much about the pretty-printing.

Perhaps stat_sep should be a callback?

> diff --git a/pretty.c b/pretty.c
> index d0f86f5d85..56e668781a 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -1641,6 +1641,21 @@ void pp_title_line(struct pretty_print_context *pp,
>   if (pp->after_subject) {
>   strbuf_addstr(sb, pp->after_subject);
>   }
> + if (pp->print_email_subject && pp->rev && pp->rev->mime_boundary) {
> + strbuf_addf(sb,
> + "MIME-Version: 1.0\n"

In the original, this would have been in "after_subject". Which means we
would print it even if print_email_subject is not true. Why do we need
to check it in the new conditional?

Not that I expect the behavior to be wrong either way; why would we have
a mime boundary without setting print_email_subject? But I would think
that "do we have a mime boundary" would be the right conditional to
trigger printing it.

-Peff

Re: Will OpenSSL's license change impact us?

On Sat, Mar 25, 2017 at 10:43 AM, demerphq  wrote:
>
>
> On 25 Mar 2017 10:18 a.m., "Ævar Arnfjörð Bjarmason" 
> wrote:
>
> On Sat, Mar 25, 2017 at 9:40 AM, demerphq  wrote:
>> On 25 March 2017 at 00:51, Ævar Arnfjörð Bjarmason 
>> wrote:
>>> They're changing their license[1] to Apache 2 which unlike the current
>>> fuzzy compatibility with the current license[2] is explicitly
>>> incompatible with GPLv2[3].
>>
>> Are you sure there is an issue? From the Apache page on this:
>>
>> Apache 2 software can therefore be included in GPLv3 projects, because
>> the GPLv3 license accepts our software into GPLv3 works. However,
>> GPLv3 software cannot be included in Apache projects. The licenses are
>> incompatible in one direction only, and it is a result of ASF's
>> licensing philosophy and the GPLv3 authors' interpretation of
>> copyright law.
>>
>> Which seems to be the opposite of the concern you are expressing.
>
> The Apache 2 license is indeed compatible with the GPLv3, but the Git
> project explicitly uses GPLv2 with no "or later" clause
>
>
> Read the paragraph immediately (I think) after the one I quoted where they
> state the situation is the same with GPL v2.

My understanding of that paragraph is that it's still laying out
caveats about exactly how GPLv3 is compatible with Apache 2, when it
is, when it isn't etc. But then it goes on to say:

"""
Despite our best efforts, the FSF has never considered the Apache
License to be compatible with GPL version 2, citing the patent
termination and indemnification provisions as restrictions not present
in the older GPL license. The Apache Software Foundation believes that
you should always try to obey the constraints expressed by the
copyright holder when redistributing their work.
"""

So they're just deferring to the FSF saying it's incompatible, the
FSF's statement:
https://www.gnu.org/licenses/license-list.html#apache2 "this license
is not compatible with GPL version 2".

Anyway, I'm not a lawyer. Just thought I'd send some E-Mail about this
since I noticed it, if it's an issue (and we could e.g. get the SFC to
comment, Jeff?) we might need to add e.g. some checks / macros to
ensure we're not compiling against an incompatible OpenSSL.

>
> $ head -n 18 COPYING
>
>  Note that the only valid version of the GPL as far as this project
>  is concerned is _this_ particular version of the license (ie v2, not
>  v2.2 or v3.x or whatever), unless explicitly otherwise stated.
>
>  HOWEVER, in order to allow a migration to GPLv3 if that seems like
>  a good idea, I also ask that people involved with the project make
>  their preferences known. In particular, if you trust me to make that
>  decision, you might note so in your copyright message, ie something
>  like
>
> This file is licensed under the GPL v2, or a later version
> at the discretion of Linus.
>
>   might avoid issues. But we can also just decide to synchronize and
>   contact all copyright holders on record if/when the occasion arises.
>
> Linus Torvalds
>
>

Re: [PATCH/RFC] parse-options: add facility to make options configurable

 On Sat, Mar 25, 2017 at 12:10 AM, Ævar Arnfjörð Bjarmason
 wrote:
> [...]
> This is all very proof-of-concept, and uses the ugly hack of s/const
> // for the options struct because I'm now keeping state in it, as
> noted in one of the TODO comments that should be moved.
> [...]
>  static int parse_long_opt(struct parse_opt_ctx_t *p, const char *arg,
> -  const struct option *options)
> +  struct option *options)
>  {
> const struct option *all_opts = options;
> [...]
> @@ -313,7 +314,17 @@ static int parse_long_opt(struct parse_opt_ctx_t *p, 
> const char *arg,
> continue;
> p->opt = rest + 1;
> }
> -   return get_value(p, options, all_opts, flags ^ opt_flags);
> +   if (!(ret = get_value(p, options, all_opts, flags ^ 
> opt_flags))) {
> +   /* TODO: Keep some different state on the side
> +* with info about what options we've
> +* retrieved via the CLI for use in the loop
> +* in parse_options_step, instead of making
> +* the 'options' non-const
> +*/
> +   if (options->flags & PARSE_OPT_CONFIGURABLE)
> +   options->flags |= PARSE_OPT_VIA_CLI;
> +   }
> +   return ret;
> }
> [...]
> +
> +   /* The loop above is driven by the argument vector, so we need
> +* to make a second pass and find those options that are
> +* configurable, and haven't been set via the command-line */
> +   for (; options->type != OPTION_END; options++) {
> +   if (!(options->flags & PARSE_OPT_CONFIGURABLE))
> +   continue;
> +
> +   if (options->flags & PARSE_OPT_VIA_CLI)
> +   continue;
> +
> +   /* TODO: Maybe factor the handling of OPTION_CALLBACK
> +* in get_value() into a function.
> +*
> +* Do we also need to save away the state from the
> +* loop above to handle unset? I think not, I think
> +* we're always unset here by definition, right?
> +*/
> +   return (*options->conf_callback)(options, NULL, 1) ? (-1) : 0;
> +   }
> +
> [...]

After looking at some of the internal APIs I'm thinking of replacing
this pattern with a hashmap.c hashmap where the keys are a
sprintf("%d:%s", short_name, long_name) to uniquely identify the
option. There's no other obvious way to uniquely address an option. I
guess I could just equivalently and more cheaply use the memory
address of each option to identify them, but that seems a bit hacky.

> @@ -110,6 +112,9 @@ struct option {
> int flags;
> parse_opt_cb *callback;
> intptr_t defval;
> +
> +   const char *conf_key;
> +   parse_opt_cb *conf_callback;
>  };

I've already found that this needs to be a char **conf_key, since
several command-line options have multiple ways to spell the option
name, e.g. add.ignoreerrors & add.ignore-errors, pack.writebitmaps &
repack.writebitmaps etc.

Re: [PATCH] pretty: add extra headers and MIME boundary directly

2017-03-25 Thread René Scharfe


Am 25.03.2017 um 17:17 schrieb Jeff King:

On Sat, Mar 25, 2017 at 01:16:42PM +0100, René Scharfe wrote:

@@ -374,26 +372,9 @@ void log_write_email_headers(struct rev_info *opt, struct 
commit *commit,
graph_show_oneline(opt->graph);
}
if (opt->mime_boundary) {
-   static char subject_buffer[1024];
static char buffer[1024];


We still have this other buffer, which ends up in stat_sep. It should
probably get the same treatment, though I think the module boundaries
make it a little more awkward. We look at it in diff_flush(), which
otherwise doesn't need to know much about the pretty-printing.

Perhaps stat_sep should be a callback?


Yes, it would be nice to avoid it, but I haven't found a clean way, yet. 
 In diff.c, where it's used, we don't have commit and rev_info 
available (which we'd have to pass to a callback, or consume right 
there), and that's probably how it should be.  Perhaps preparing the 
filename in advance and passing that as a string together with 
mime_boundary and no_inline might be the way to go.



diff --git a/pretty.c b/pretty.c
index d0f86f5d85..56e668781a 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1641,6 +1641,21 @@ void pp_title_line(struct pretty_print_context *pp,
if (pp->after_subject) {
strbuf_addstr(sb, pp->after_subject);
}
+   if (pp->print_email_subject && pp->rev && pp->rev->mime_boundary) {
+   strbuf_addf(sb,
+   "MIME-Version: 1.0\n"


In the original, this would have been in "after_subject". Which means we
would print it even if print_email_subject is not true. Why do we need
to check it in the new conditional?


No, we only would have printed it if log_write_email_headers() was 
called to append it to the static buffer.  print_email_subject is only 
set when we call log_write_email_headers(), so checking it makes sure 
that we get the same behavior as before.



Not that I expect the behavior to be wrong either way; why would we have
a mime boundary without setting print_email_subject? But I would think
that "do we have a mime boundary" would be the right conditional to
trigger printing it.


FWIW, the test suite still passes with the print_email_subject check 
removed.  And currently only cmd_format_patch() sets mime_boundary, so 
we don't need the check indeed.


René

Re: Will OpenSSL's license change impact us?

2017-03-25 Thread demerphq

On 25 March 2017 at 17:35, Ævar Arnfjörð Bjarmason  wrote:
> On Sat, Mar 25, 2017 at 10:43 AM, demerphq  wrote:
>>
>>
>> On 25 Mar 2017 10:18 a.m., "Ævar Arnfjörð Bjarmason" 
>> wrote:
>>
>> On Sat, Mar 25, 2017 at 9:40 AM, demerphq  wrote:
>>> On 25 March 2017 at 00:51, Ævar Arnfjörð Bjarmason 
>>> wrote:
 They're changing their license[1] to Apache 2 which unlike the current
 fuzzy compatibility with the current license[2] is explicitly
 incompatible with GPLv2[3].
>>>
>>> Are you sure there is an issue? From the Apache page on this:
>>>
>>> Apache 2 software can therefore be included in GPLv3 projects, because
>>> the GPLv3 license accepts our software into GPLv3 works. However,
>>> GPLv3 software cannot be included in Apache projects. The licenses are
>>> incompatible in one direction only, and it is a result of ASF's
>>> licensing philosophy and the GPLv3 authors' interpretation of
>>> copyright law.
>>>
>>> Which seems to be the opposite of the concern you are expressing.
>>
>> The Apache 2 license is indeed compatible with the GPLv3, but the Git
>> project explicitly uses GPLv2 with no "or later" clause
>>
>>
>> Read the paragraph immediately (I think) after the one I quoted where they
>> state the situation is the same with GPL v2.
>
> My understanding of that paragraph is that it's still laying out
> caveats about exactly how GPLv3 is compatible with Apache 2, when it
> is, when it isn't etc. But then it goes on to say:
>
> """
> Despite our best efforts, the FSF has never considered the Apache
> License to be compatible with GPL version 2, citing the patent
> termination and indemnification provisions as restrictions not present
> in the older GPL license. The Apache Software Foundation believes that
> you should always try to obey the constraints expressed by the
> copyright holder when redistributing their work.
> """
>
> So they're just deferring to the FSF saying it's incompatible, the
> FSF's statement:
> https://www.gnu.org/licenses/license-list.html#apache2 "this license
> is not compatible with GPL version 2".
>
> Anyway, I'm not a lawyer. Just thought I'd send some E-Mail about this
> since I noticed it, if it's an issue (and we could e.g. get the SFC to
> comment, Jeff?) we might need to add e.g. some checks / macros to
> ensure we're not compiling against an incompatible OpenSSL.

Just for the record this what Apache says, with the part I was
referring to earlier in slash style italics, and a couple of a key
points in star style bold:

quote
Apache 2 software *can therefore be included in GPLv3 projects*,
because the GPLv3 license accepts our software into GPLv3 works.
However, GPLv3 software cannot be included in Apache projects. *The
licenses are incompatible in one direction only*, and it is a result
of ASF's licensing philosophy and the GPLv3 authors' interpretation of
copyright law.

This licensing incompatibility applies only when some Apache project
software becomes a derivative work of some GPLv3 software, because
then the Apache software would have to be distributed under GPLv3.
This would be incompatible with ASF's requirement that all Apache
software must be distributed under the Apache License 2.0.

We avoid GPLv3 software because merely linking to it is considered by
the GPLv3 authors to create a derivative work. We want to honor their
license. Unless GPLv3 licensors relax this interpretation of their own
license regarding linking, our licensing philosophies are
fundamentally incompatible. /This is an identical issue for both GPLv2
and GPLv3./
quote

I read that as saying that you can use Apache 2 code in GPL projects,
but you can't use GPL code in Apache projects. Which makes sense as
Apache 2 is more liberal than GPL.

Yves

-- 
perl -Mre=debug -e "/just|another|perl|hacker/"

Re: [PATCH 0/7] PREVIEW: Introduce DC_AND_OPENSSL_SHA1 make flag

2017-03-25 Thread Junio C Hamano

Junio C Hamano  writes:

> The hash that names a packfile is constructed by sorting all the
> names of the objects contained in the packfile and running SHA-1
> hash over it.

Sorry, but I need to make a correction here.

This "SHA-1 over sorted object names" is a description of an ancient
behaviour before 1190a1ac ("pack-objects: name pack files after
trailer hash", 2013-12-05) happened.  These days the pack name is
the same as the csum-file checksum of the .pack contents.

This however does not change the fact that the site that feeds us a
packfile is in control of the hash, hence the name we give to the
resulting packfile.  Unlike the use of csum-file for the trailing
hash for the index file, which is only to protect against bit
flipping, "SHA-1 over .pack contents" done here is used to come up
with a unique name used for identification and deduplication (of the
packfile, not of individual objects), and the need for protection
against collision attack attempts does not change between the
implementation before 1190a1ac and after that commit.

Re: [PATCH] sha1dc: safeguard against outside definitions of BIGENDIAN

2017-03-25 Thread Junio C Hamano

Junio C Hamano  writes:

> Which leads me to wonder if a more robust solution that is in line
> with the original design of sha1dc/sha1.c code may be to do an
> unconditional "#undef BIGENDIAN" before the above block, so that no
> matter what the calling environment sets BIGENDIAN to (including
> "0"), it gets ignored and we always use the auto-selection.

So here is what I came up with as a replacement (this time as a
proper patch not a comment on a patch).

Dscho, could you see if this fixes your build?

Also, could people on big-endian boxes see if this breaks your
setting (relative to the state before this patch)?

Thanks.

-- >8 --
From: Junio C Hamano 
Date: Sat, 25 Mar 2017 10:05:13 -0700
Subject: [PATCH] sha1dc: avoid CPP macro collisions

In an early part of sha1dc/sha1.c, the code checks the endianness of
the target platform by inspecting common CPP macros defined on
big-endian boxes, and sets BIGENDIAN macro to 1.  If these common
CPP macros are not defined, the code declares that the target
platform is little endian and does nothing (most notably, it does
not #undef its BIGENDIAN macro).

The code that does so even has this comment

Note that all MSFT platforms are little endian,
so none of these will be defined under the MSC compiler.

and later, the defined-ness of the BIGENDIAN macro is used to switch
the implementation of sha1_load() macro.

One thing the code did not anticipate is that somebody might define
BIGENDIAN macro in some header it includes to 0 on a little-endian
target platform.  Because the auto-detection based on common macros
do not touch BIGENDIAN macro when it detects a little-endian target,
such a definition is still valid and then defined-ness test will say
"Ah, BIGENDIAN is defined" and takes the wrong sha1_load().

As this auto-detection logic pretends as if it owns the BIGENDIAN
macro by ignoring the setting that may come from the outside and by
not explicitly unsetting when it decides that it is working for a
little-endian target, solve this problem without breaking that
assumption.  Namely, we can rename BIGENDIAN this code uses to
something much less generic, i.e. SHA1DC_BIGENDIAN.  For extra
protection, undef the macro on a little-endian target.

It is possible to work it around by instead #undef BIGENDIAN in
the auto-detection code, but a macro (or include) that happens later
in the code can be implemented in terms of BIGENDIAN on Windows and
it is possible that the implementation gets upset when it sees the
CPP macro undef'ed (instead of set to 0).  Renaming the private macro
intended to be used only in this file to a less generic name relieves
us from having to worry about that kind of breakage.

Noticed-by: Johannes Schindelin 
Signed-off-by: Junio C Hamano 
---
 sha1dc/sha1.c | 11 ++-
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/sha1dc/sha1.c b/sha1dc/sha1.c
index 6dd0da3608..35e9dd5bf4 100644
--- a/sha1dc/sha1.c
+++ b/sha1dc/sha1.c
@@ -12,7 +12,7 @@

 /*
Because Little-Endian architectures are most common,
-   we only set BIGENDIAN if one of these conditions is met.
+   we only set SHA1DC_BIGENDIAN if one of these conditions is met.
Note that all MSFT platforms are little endian,
so none of these will be defined under the MSC compiler.
If you are compiling on a big endian platform and your compiler does not 
define one of these,
@@ -23,8 +23,9 @@
 defined(__BIG_ENDIAN__) || defined(__ARMEB__) || defined(__THUMBEB__) ||  
defined(__AARCH64EB__) || \
 defined(_MIPSEB) || defined(__MIPSEB) || defined(__MIPSEB__)

-#define BIGENDIAN  (1)
-
+#define SHA1DC_BIGENDIAN   1
+#else
+#undef SHA1DC_BIGENDIAN
 #endif /*ENDIANNESS SELECTION*/

 #define rotate_right(x,n) (((x)>>(n))|((x)<<(32-(n
@@ -35,11 +36,11 @@

 #define sha1_mix(W, t)  (rotate_left(W[t - 3] ^ W[t - 8] ^ W[t - 14] ^ W[t - 
16], 1))

-#if defined(BIGENDIAN)
+#if defined(SHA1DC_BIGENDIAN)
#define sha1_load(m, t, temp)  { temp = m[t]; }
 #else
#define sha1_load(m, t, temp)  { temp = m[t]; sha1_bswap32(temp); }
-#endif /*define(BIGENDIAN)*/
+#endif /* !defined(SHA1DC_BIGENDIAN) */

 #define sha1_store(W, t, x)*(volatile uint32_t *)&W[t] = x

-- 
2.12.2-399-g034667a458

Re: Will OpenSSL's license change impact us?

On Sat, Mar 25, 2017 at 5:57 PM, demerphq  wrote:
> On 25 March 2017 at 17:35, Ævar Arnfjörð Bjarmason  wrote:
>> On Sat, Mar 25, 2017 at 10:43 AM, demerphq  wrote:
>>>
>>>
>>> On 25 Mar 2017 10:18 a.m., "Ævar Arnfjörð Bjarmason" 
>>> wrote:
>>>
>>> On Sat, Mar 25, 2017 at 9:40 AM, demerphq  wrote:
 On 25 March 2017 at 00:51, Ævar Arnfjörð Bjarmason 
 wrote:
> They're changing their license[1] to Apache 2 which unlike the current
> fuzzy compatibility with the current license[2] is explicitly
> incompatible with GPLv2[3].

 Are you sure there is an issue? From the Apache page on this:

 Apache 2 software can therefore be included in GPLv3 projects, because
 the GPLv3 license accepts our software into GPLv3 works. However,
 GPLv3 software cannot be included in Apache projects. The licenses are
 incompatible in one direction only, and it is a result of ASF's
 licensing philosophy and the GPLv3 authors' interpretation of
 copyright law.

 Which seems to be the opposite of the concern you are expressing.
>>>
>>> The Apache 2 license is indeed compatible with the GPLv3, but the Git
>>> project explicitly uses GPLv2 with no "or later" clause
>>>
>>>
>>> Read the paragraph immediately (I think) after the one I quoted where they
>>> state the situation is the same with GPL v2.
>>
>> My understanding of that paragraph is that it's still laying out
>> caveats about exactly how GPLv3 is compatible with Apache 2, when it
>> is, when it isn't etc. But then it goes on to say:
>>
>> """
>> Despite our best efforts, the FSF has never considered the Apache
>> License to be compatible with GPL version 2, citing the patent
>> termination and indemnification provisions as restrictions not present
>> in the older GPL license. The Apache Software Foundation believes that
>> you should always try to obey the constraints expressed by the
>> copyright holder when redistributing their work.
>> """
>>
>> So they're just deferring to the FSF saying it's incompatible, the
>> FSF's statement:
>> https://www.gnu.org/licenses/license-list.html#apache2 "this license
>> is not compatible with GPL version 2".
>>
>> Anyway, I'm not a lawyer. Just thought I'd send some E-Mail about this
>> since I noticed it, if it's an issue (and we could e.g. get the SFC to
>> comment, Jeff?) we might need to add e.g. some checks / macros to
>> ensure we're not compiling against an incompatible OpenSSL.
>
> Just for the record this what Apache says, with the part I was
> referring to earlier in slash style italics, and a couple of a key
> points in star style bold:
>
> quote
> Apache 2 software *can therefore be included in GPLv3 projects*,
> because the GPLv3 license accepts our software into GPLv3 works.
> However, GPLv3 software cannot be included in Apache projects. *The
> licenses are incompatible in one direction only*, and it is a result
> of ASF's licensing philosophy and the GPLv3 authors' interpretation of
> copyright law.
>
> This licensing incompatibility applies only when some Apache project
> software becomes a derivative work of some GPLv3 software, because
> then the Apache software would have to be distributed under GPLv3.
> This would be incompatible with ASF's requirement that all Apache
> software must be distributed under the Apache License 2.0.
>
> We avoid GPLv3 software because merely linking to it is considered by
> the GPLv3 authors to create a derivative work. We want to honor their
> license. Unless GPLv3 licensors relax this interpretation of their own
> license regarding linking, our licensing philosophies are
> fundamentally incompatible. /This is an identical issue for both GPLv2
> and GPLv3./
> quote
>
> I read that as saying that you can use Apache 2 code in GPL projects,
> but you can't use GPL code in Apache projects. Which makes sense as
> Apache 2 is more liberal than GPL.

In GPLv3 projects only, not GPLv2 projects. The paragraphs you're
quoting all explicitly mention v3 only, so statements like
"incompatible in one direction" only apply to Apache 2 && GPLv3, but
don't at all apply to GPLv2, which is what we're using.

[PATCH v3 0/2] [GSoC] remove_subtree(): reimplement using iterators

This is the third version of the GSoC microproject
of refactoring remove_subtree() from recursively using
readdir() to use dir_iterator. Below are the threads for
other versions:

v1: 
https://public-inbox.org/git/CAGZ79kZwT-9mHTiOJ5CEjk2wDFkn6+NcogjX0=vjhsah16a...@mail.gmail.com/T/#mae023e7a7d7626f00e0923833c4359f5af493730
v2: 
https://public-inbox.org/git/cacsjy8dxh-qpbblfafwpawusba9gvxa7x+mxljevykhk1zo...@mail.gmail.com/T/#t

Duy suggested adding features to dir_iterator might go
beyond the intention of a microproject, but I figured I
might go for it to learn more about the project.

The dir_iterator reimplementation has been tested in a
separate binary I created (and linked with libgit.a) to
reproduce remove_subtree()'s contents. As pointed out in the
last thread, git's tests for this function were unable to
catch a daunting bug I had introduced, and I still haven't
been able to come up with a way to reproduce remove_subtree()
being called. Any help?

Thank you all again for all the reviews.

Daniel Ferreira (2):
  dir_iterator: iterate over dir after its contents
  remove_subtree(): reimplement using iterators

 dir-iterator.c | 100 -
 dir-iterator.h |   7 
 entry.c|  32 +++---
 3 files changed, 95 insertions(+), 44 deletions(-)

--
2.7.4 (Apple Git-66)

[PATCH v3 2/2] [GSoC] remove_subtree(): reimplement using iterators

Use dir_iterator to traverse through remove_subtree()'s directory tree,
avoiding the need for recursive calls to readdir(). Simplify
remove_subtree()'s code.

A conversion similar in purpose was previously done at 46d092a
("for_each_reflog(): reimplement using iterators", 2016-05-21).

Signed-off-by: Daniel Ferreira 
---
 entry.c | 32 +++-
 1 file changed, 11 insertions(+), 21 deletions(-)

diff --git a/entry.c b/entry.c
index c6eea24..670ffeb 100644
--- a/entry.c
+++ b/entry.c
@@ -2,6 +2,8 @@
 #include "blob.h"
 #include "dir.h"
 #include "streaming.h"
+#include "iterator.h"
+#include "dir-iterator.h"

 static void create_directories(const char *path, int path_len,
   const struct checkout *state)
@@ -46,29 +48,17 @@ static void create_directories(const char *path, int 
path_len,

 static void remove_subtree(struct strbuf *path)
 {
-   DIR *dir = opendir(path->buf);
-   struct dirent *de;
-   int origlen = path->len;
-
-   if (!dir)
-   die_errno("cannot opendir '%s'", path->buf);
-   while ((de = readdir(dir)) != NULL) {
-   struct stat st;
-
-   if (is_dot_or_dotdot(de->d_name))
-   continue;
-
-   strbuf_addch(path, '/');
-   strbuf_addstr(path, de->d_name);
-   if (lstat(path->buf, &st))
-   die_errno("cannot lstat '%s'", path->buf);
-   if (S_ISDIR(st.st_mode))
-   remove_subtree(path);
-   else if (unlink(path->buf))
+   struct dir_iterator *diter = dir_iterator_begin(path->buf);
+   diter->options.iterate_dirs_after_files = 1;
+
+   while (dir_iterator_advance(diter) == ITER_OK) {
+   if (S_ISDIR(diter->st.st_mode)) {
+   if (rmdir(diter->path.buf))
+   die_errno("cannot rmdir '%s'", path->buf);
+   } else if (unlink(diter->path.buf))
die_errno("cannot unlink '%s'", path->buf);
-   strbuf_setlen(path, origlen);
}
-   closedir(dir);
+
if (rmdir(path->buf))
die_errno("cannot rmdir '%s'", path->buf);
 }
--
2.7.4 (Apple Git-66)

[PATCH v3 1/2] [GSoC] dir_iterator: iterate over dir after its contents

Create an option for the dir_iterator API to iterate over a directory
path only after having iterated through its contents. This feature was
predicted, although not implemented by 0fe5043 ("dir_iterator: new API
for iterating over a directory tree", 2016-06-18).

This is useful for recursively removing a directory and calling rmdir()
on a directory only after all of its contents have been wiped.

An "options" member has been added to the dir_iterator struct. It
contains the "iterate_dirs_after_files" flag, that enables the feature
when set to 1. Default behavior continues to be iterating over directory
paths before its contents.

Two inline functions have been added to dir_iterator's code to avoid
code repetition inside dir_iterator_advance() and make code more clear.

No particular functions or wrappers for setting the options struct's
fields have been added to avoid either breaking the current dir_iterator
API or over-engineering an extremely simple option architecture.

Signed-off-by: Daniel Ferreira 
---
 dir-iterator.c | 100 -
 dir-iterator.h |   7 
 2 files changed, 84 insertions(+), 23 deletions(-)

diff --git a/dir-iterator.c b/dir-iterator.c
index 34182a9..833d56a 100644
--- a/dir-iterator.c
+++ b/dir-iterator.c
@@ -50,6 +50,43 @@ struct dir_iterator_int {
struct dir_iterator_level *levels;
 };

+static inline void push_dir_level(struct dir_iterator_int *iter, struct 
dir_iterator_level *level)
+{
+   level->dir_state = DIR_STATE_RECURSE;
+   ALLOC_GROW(iter->levels, iter->levels_nr + 1,
+  iter->levels_alloc);
+   level = &iter->levels[iter->levels_nr++];
+   level->initialized = 0;
+}
+
+static inline int pop_dir_level(struct dir_iterator_int *iter, struct 
dir_iterator_level *level)
+{
+   return --iter->levels_nr;
+}
+
+static inline int set_iterator_data(struct dir_iterator_int *iter, struct 
dir_iterator_level *level)
+{
+   if (lstat(iter->base.path.buf, &iter->base.st) < 0) {
+   if (errno != ENOENT)
+   warning("error reading path '%s': %s",
+   iter->base.path.buf,
+   strerror(errno));
+   return -1;
+   }
+
+   /*
+* We have to set these each time because
+* the path strbuf might have been realloc()ed.
+*/
+   iter->base.relative_path =
+   iter->base.path.buf + iter->levels[0].prefix_len;
+   iter->base.basename =
+   iter->base.path.buf + level->prefix_len;
+   level->dir_state = DIR_STATE_ITER;
+
+   return 0;
+}
+
 int dir_iterator_advance(struct dir_iterator *dir_iterator)
 {
struct dir_iterator_int *iter =
@@ -77,18 +114,16 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)
}

level->initialized = 1;
-   } else if (S_ISDIR(iter->base.st.st_mode)) {
+   } else if (S_ISDIR(iter->base.st.st_mode) &&
+   !iter->base.options.iterate_dirs_after_files) {
if (level->dir_state == DIR_STATE_ITER) {
/*
 * The directory was just iterated
 * over; now prepare to iterate into
-* it.
+* it (unless an option is set for us
+* to do otherwise).
 */
-   level->dir_state = DIR_STATE_RECURSE;
-   ALLOC_GROW(iter->levels, iter->levels_nr + 1,
-  iter->levels_alloc);
-   level = &iter->levels[iter->levels_nr++];
-   level->initialized = 0;
+   push_dir_level(iter, level);
continue;
} else {
/*
@@ -104,7 +139,7 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)
 * This level is exhausted (or wasn't opened
 * successfully); pop up a level.
 */
-   if (--iter->levels_nr == 0)
+   if (pop_dir_level(iter, level) == 0)
return dir_iterator_abort(dir_iterator);

continue;
@@ -120,16 +155,33 @@ int dir_iterator_advance(struct dir_iterator 
*dir_iterator)
de = readdir(level->dir);

if (!de) {
-   /* This level is exhausted; pop up a level. */
+   /* This level is exhausted  */
if (errno) {
warning("error reading directory %s: 
%s",
i

Re: t1503 broken ?

2017-03-25 Thread Torsten Bögershausen




On 03/25/2017 02:05 PM, Duy Nguyen wrote:

On Sat, Mar 25, 2017 at 07:26:14PM +0700, Duy Nguyen wrote:

On Sat, Mar 25, 2017 at 6:46 PM, Duy Nguyen  wrote:

On Sat, Mar 25, 2017 at 5:46 PM, Torsten Bögershausen  wrote:

./t1305-config-include.sh
seems to be broken:
not ok 19 - conditional include, $HOME expansion
not ok 21 - conditional include, relative path

let me guess, your "git" directory is in a symlink path?

Yes I could reproduce it when I put my "git" in a symlink. There's a
note in document about "Symlinks in `$GIT_DIR` are not resolved before
matching" but failing tests is not acceptable. I'll fix it.

The fix may be something like this. The problem is $GIT_DIR has symlinks
resolved, but we don't do the same for other paths in this code. As a
result, matching paths fails.

I'm a bit concerned about the change in expand_user_path() because I'm
not quite sure if it's a completely safe change. But at least could
you try the patch and see if it passe the tests on your machine too?

-- 8< --
diff --git a/config.c b/config.c
index 1a4d855..fc4eae9 100644
--- a/config.c
+++ b/config.c
@@ -191,7 +191,7 @@ static int prepare_include_condition_pattern(struct strbuf 
*pat)
return error(_("relative config include "
   "conditionals must come from files"));
  
-		strbuf_add_absolute_path(&path, cf->path);

+   strbuf_realpath(&path, cf->path, 1);
slash = find_last_dir_sep(path.buf);
if (!slash)
die("BUG: how is this possible?");
@@ -213,7 +213,7 @@ static int include_by_gitdir(const char *cond, size_t 
cond_len, int icase)
struct strbuf pattern = STRBUF_INIT;
int ret = 0, prefix;
  
-	strbuf_add_absolute_path(&text, get_git_dir());

+   strbuf_realpath(&text, get_git_dir(), 1);
strbuf_add(&pattern, cond, cond_len);
prefix = prepare_include_condition_pattern(&pattern);
  
diff --git a/path.c b/path.c

index 2224843..18eaac3 100644
--- a/path.c
+++ b/path.c
@@ -654,7 +654,7 @@ char *expand_user_path(const char *path)
const char *home = getenv("HOME");
if (!home)
goto return_null;
-   strbuf_addstr(&user_path, home);
+   strbuf_addstr(&user_path, real_path(home));
  #ifdef GIT_WINDOWS_NATIVE
convert_slashes(user_path.buf);
  #endif
-- 8< --


Thanks for the fast reply.
Yes, my path is a softlink - into a directory under NoBackup/ - to make 
the backup shorter.

And I had forgotten about this :-(
And yes, your patch fixes it- tested under Linux.

Re: [PATCH 2/7] Makefile: optionally compile with both SHA1DC and SHA1_OPENSSL

On Sat, Mar 25, 2017 at 12:24 AM, Johannes Schindelin
 wrote:
> The collision detection is not for free, though: when using the SHA1DC
> code, calculating the SHA-1 takes substantially longer than using
> OpenSSL's (in some case hardware-accelerated) SHA1-routines, and this
> happens even when switching off the collision detection in SHA1DC's code.

[...]in some case*s*[...]

Re: Will OpenSSL's license change impact us?

2017-03-25 Thread Theodore Ts'o

On Sat, Mar 25, 2017 at 06:51:21PM +0100, Ævar Arnfjörð Bjarmason wrote:
> In GPLv3 projects only, not GPLv2 projects. The paragraphs you're
> quoting all explicitly mention v3 only, so statements like
> "incompatible in one direction" only apply to Apache 2 && GPLv3, but
> don't at all apply to GPLv2, which is what we're using.

It's complicated.

It's fair enough to say that the FSF adopts a copyright maximalist
position, and by their interpretation, the two licenses are
incompatible, and it doesn't matter whether the two pieces of code are
linked staticaly, dynamically, or one calls the other over an RPC
call.

Not everyone agrees with their legal analysis.  May I suggest that we
not play amateur lawyer on the mailing list, and try to settle this
here?  Each distribution can make its own decision, which may be based
on its legal advice, the local laws and legal precedents in which they
operate, etc.  And indeed, different distributions have already come
to different conclusions with respect to various license compatibility
issues.  (Examples: dynamically linking GPL programs with OpenSSL
libraries under the old license, distributing ZFS modules for Linux,
etc.)

We don't expect lawyers to debug edge cases in a compiler's code
generation.  Programmers shouldn't try to parse edge cases in the law,
or try to use a soldering iron, unless they have explicit training and
expertise to do so.  :-)

- Ted

[PATCH] bisect: add quit command

2017-03-25 Thread Edmundo Carmona Antoranz

git bisect quit will call git reset HEAD so that the working tree
remains at the current revision
---
 Documentation/git-bisect.txt | 12 
 git-bisect.sh| 11 ++-
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-bisect.txt b/Documentation/git-bisect.txt
index bdd915a66..de79e9e44 100644
--- a/Documentation/git-bisect.txt
+++ b/Documentation/git-bisect.txt
@@ -23,6 +23,7 @@ on the subcommand:
  git bisect terms [--term-good | --term-bad]
  git bisect skip [(|)...]
  git bisect reset []
+ git bisect quit
  git bisect visualize
  git bisect replay 
  git bisect log
@@ -120,6 +121,17 @@ bad revision, while `git bisect reset HEAD` will leave you 
on the
 current bisection commit and avoid switching commits at all.
 
 
+Bisect quit
+~~~
+
+It's an equivalent of bisect reset but that stays at the current
+revision instead of taking your working tree to the starting revision.
+
+
+$ git bisect quit
+
+
+
 Alternate terms
 ~~~
 
diff --git a/git-bisect.sh b/git-bisect.sh
index ae3cb013e..adbff2c69 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -1,6 +1,6 @@
 #!/bin/sh
 
-USAGE='[help|start|bad|good|new|old|terms|skip|next|reset|visualize|replay|log|run]'
+USAGE='[help|start|bad|good|new|old|terms|skip|next|quit|reset|visualize|replay|log|run]'
 LONG_USAGE='git bisect help
print this long help message.
 git bisect start [--term-{old,good}= --term-{new,bad}=]
@@ -20,6 +20,9 @@ git bisect next
find next bisection to test and check it out.
 git bisect reset []
finish bisection search and go back to commit.
+git bisect quit
+   stop bisection on its tracks and stay on current revision.
+   Equivalent to git bisect reset HEAD
 git bisect visualize
show bisect status in gitk.
 git bisect replay 
@@ -433,6 +436,10 @@ Try 'git bisect reset '.")"
bisect_clean_state
 }
 
+bisect_quit() {
+   bisect_reset "HEAD"
+}
+
 bisect_clean_state() {
# There may be some refs packed during bisection.
git for-each-ref --format='%(refname) %(objectname)' refs/bisect/\* |
@@ -683,6 +690,8 @@ case "$#" in
bisect_visualize "$@" ;;
reset)
bisect_reset "$@" ;;
+   quit)
+   bisect_quit ;;
replay)
bisect_replay "$@" ;;
log)
-- 
2.11.0.rc1

Re: [PATCH] pretty: add extra headers and MIME boundary directly

On Sat, Mar 25, 2017 at 05:56:55PM +0100, René Scharfe wrote:

> Am 25.03.2017 um 17:17 schrieb Jeff King:
> > On Sat, Mar 25, 2017 at 01:16:42PM +0100, René Scharfe wrote:
> > > @@ -374,26 +372,9 @@ void log_write_email_headers(struct rev_info *opt, 
> > > struct commit *commit,
> > >   graph_show_oneline(opt->graph);
> > >   }
> > >   if (opt->mime_boundary) {
> > > - static char subject_buffer[1024];
> > >   static char buffer[1024];
> > 
> > We still have this other buffer, which ends up in stat_sep. It should
> > probably get the same treatment, though I think the module boundaries
> > make it a little more awkward. We look at it in diff_flush(), which
> > otherwise doesn't need to know much about the pretty-printing.
> > 
> > Perhaps stat_sep should be a callback?
> 
> Yes, it would be nice to avoid it, but I haven't found a clean way, yet.  In
> diff.c, where it's used, we don't have commit and rev_info available (which
> we'd have to pass to a callback, or consume right there), and that's
> probably how it should be.  Perhaps preparing the filename in advance and
> passing that as a string together with mime_boundary and no_inline might be
> the way to go.

I think the no-allocation way with a callback would be something like
the patch at the end of this email. Having to stuff the commit into
rev_info is horrible, because rev_info shouldn't be carrying data
specific to individual commits. But it's really no different than what's
there now, which is stuffing a filled-out commit-specific buffer into
the same struct.

Doing it with a single allocated buffer would involve less callback
rigmarole, but it doesn't change the fact that we are stuffing
commit-specific data into the rev_info (via the diff_options member).
And then you add on top the question of who is supposed to free it
(which is punted on in the original by using a static; yech).

The most correct way is that the caller of log_write_email_headers() and
diff_flush() should have a function-local strbuf which holds the data,
gets passed to diff_flush() as some kind opaque context, and then is
freed afterwards. We don't have such a context, but if we were to abuse
diff_options.stat_sep _temporarily_, that would still be a lot cleaner.
I.e., something like this:

  struct strbuf stat_sep = STRBUF_INIT;

  /* may write into stat_sep, depending on options */
  log_write_email_headers(..., &stat_sep);
  opt->diffopt.stat_sep = stat_sep.buf;

  diff_flush(&opt->diffopt);
  opt->diffopt.stat_sep = NULL;
  strbuf_release(&stat_sep);

But it's a bit tricky because those two hunks happen in separate
functions, which means passing the strbuf around.

> > > + if (pp->print_email_subject && pp->rev && pp->rev->mime_boundary) {
> > > + strbuf_addf(sb,
> > > + "MIME-Version: 1.0\n"
> > 
> > In the original, this would have been in "after_subject". Which means we
> > would print it even if print_email_subject is not true. Why do we need
> > to check it in the new conditional?
> 
> No, we only would have printed it if log_write_email_headers() was called to
> append it to the static buffer.  print_email_subject is only set when we
> call log_write_email_headers(), so checking it makes sure that we get the
> same behavior as before.

OK. So your logic is "you would always set print_email_subject alongside
log_write_email_headers()", and mine is "you would always call
log_write_email_headers() when opt->mime_boundary is set". Both are true
today, and I do not see a big chance of them becoming untrue.

So I'm OK with it either way.

> > Not that I expect the behavior to be wrong either way; why would we have
> > a mime boundary without setting print_email_subject? But I would think
> > that "do we have a mime boundary" would be the right conditional to
> > trigger printing it.
> 
> FWIW, the test suite still passes with the print_email_subject check
> removed.  And currently only cmd_format_patch() sets mime_boundary, so we
> don't need the check indeed.

Yeah, while working on the other patch I looked at all the callers
(there are only 2), and I think it's fine. I thought at first there
might be problems with:

  git format-patch --attach --format=not-email:%s

but we skip this code when it's a non-email format (and anyway,
format-patch always sets print_email_subject whether the format is email
or not).  That command _does_ generate nonsense, because
cmd_format_patch() insists on showing the trailing mime boundary even if
the format is not email. I'm not sure there's a good reason to use a
non-email format with format-patch in the first place, let alone with
--attach. Arguably it should bail as soon as it sees the incompatible
options.

Anyway. Here's my attempt at the callback version of stat_sep.

---
diff --git a/diff.c b/diff.c
index a628ac3a9..d061f9e18 100644
--- a/diff.c
+++ b/diff.c
@@ -4819,10 +4819,9 @@ void diff_flush(struct diff_options *options)
fprintf(options->file, "%s%

Re: [PATCH] bisect: add quit command

2017-03-25 Thread Edmundo Carmona Antoranz

I found out about "git bisect reset HEAD" while working on "git bisect
quit" but I think it's still worth it.

Let me know.

On Sat, Mar 25, 2017 at 3:17 PM, Edmundo Carmona Antoranz
 wrote:
> git bisect quit will call git reset HEAD so that the working tree
> remains at the current revision
> ---
>  Documentation/git-bisect.txt | 12 
>  git-bisect.sh| 11 ++-
>  2 files changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/git-bisect.txt b/Documentation/git-bisect.txt
> index bdd915a66..de79e9e44 100644
> --- a/Documentation/git-bisect.txt
> +++ b/Documentation/git-bisect.txt
> @@ -23,6 +23,7 @@ on the subcommand:
>   git bisect terms [--term-good | --term-bad]
>   git bisect skip [(|)...]
>   git bisect reset []
> + git bisect quit
>   git bisect visualize
>   git bisect replay 
>   git bisect log
> @@ -120,6 +121,17 @@ bad revision, while `git bisect reset HEAD` will leave 
> you on the
>  current bisection commit and avoid switching commits at all.
>
>
> +Bisect quit
> +~~~
> +
> +It's an equivalent of bisect reset but that stays at the current
> +revision instead of taking your working tree to the starting revision.
> +
> +
> +$ git bisect quit
> +
> +
> +
>  Alternate terms
>  ~~~
>
> diff --git a/git-bisect.sh b/git-bisect.sh
> index ae3cb013e..adbff2c69 100755
> --- a/git-bisect.sh
> +++ b/git-bisect.sh
> @@ -1,6 +1,6 @@
>  #!/bin/sh
>
> -USAGE='[help|start|bad|good|new|old|terms|skip|next|reset|visualize|replay|log|run]'
> +USAGE='[help|start|bad|good|new|old|terms|skip|next|quit|reset|visualize|replay|log|run]'
>  LONG_USAGE='git bisect help
> print this long help message.
>  git bisect start [--term-{old,good}= --term-{new,bad}=]
> @@ -20,6 +20,9 @@ git bisect next
> find next bisection to test and check it out.
>  git bisect reset []
> finish bisection search and go back to commit.
> +git bisect quit
> +   stop bisection on its tracks and stay on current revision.
> +   Equivalent to git bisect reset HEAD
>  git bisect visualize
> show bisect status in gitk.
>  git bisect replay 
> @@ -433,6 +436,10 @@ Try 'git bisect reset '.")"
> bisect_clean_state
>  }
>
> +bisect_quit() {
> +   bisect_reset "HEAD"
> +}
> +
>  bisect_clean_state() {
> # There may be some refs packed during bisection.
> git for-each-ref --format='%(refname) %(objectname)' refs/bisect/\* |
> @@ -683,6 +690,8 @@ case "$#" in
> bisect_visualize "$@" ;;
> reset)
> bisect_reset "$@" ;;
> +   quit)
> +   bisect_quit ;;
> replay)
> bisect_replay "$@" ;;
> log)
> --
> 2.11.0.rc1
>

Re: [PATCH/RFC] parse-options: add facility to make options configurable

2017-03-25 Thread brian m. carlson

On Fri, Mar 24, 2017 at 11:10:13PM +, Ævar Arnfjörð Bjarmason wrote:
> On Sun, Mar 19, 2017 at 2:43 PM, Ævar Arnfjörð Bjarmason  
> wrote:
> > I don't know if this is what Duy has in mind, but the facility I've
> > described is  purely an internal code reorganization issue. I.e. us
> > not having to write custom code for each bultin every time we want to
> > take an option from the command line || config.
> 
> Here's an implementation of this I hacked up this evening. This is
> very WIP as noted in the commit message / TODO comments, but it works!
> I thought I'd send it to the list for comments on the general approach
> before taking it much further.

For what it's worth, I think this is a good design.  It makes it easy to
add options when needed, but it doesn't override the defaults for the
entire command, which was my concern.

The potential for removing a decent amount of likely duplicative code
also makes me happy.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: [PATCH v4 1/2] diff --no-index: optionally follow symlinks

2017-03-25 Thread Dennis Kaarsemaker

On Fri, 2017-03-24 at 15:56 -0700, Junio C Hamano wrote:

> diff --git a/diff.c b/diff.c
> > index be11e4ef2b..2afecfb939 100644
> > --- a/diff.c
> > +++ b/diff.c
> > @@ -2815,7 +2815,7 @@ int diff_populate_filespec(struct diff_filespec *s, 
> > unsigned int flags)
> > s->size = xsize_t(st.st_size);
> > if (!s->size)
> > goto empty;
> > -   if (S_ISLNK(st.st_mode)) {
> > +   if (S_ISLNK(s->mode)) {
> 
> This change is conceptually wrong.  s->mode (often) comes from the
> index but in this codepath, after finding that s->oid is not valid
> or we want to read from the working tree instead (several lines
> before this part), we are committed to read from the working tree
> and check things with st.st_* fields, not s->mode, when we decide
> what to do with the thing we find on the filesystem, no?

Hmm, true. It just accidentally does the right thing because s->mode
happens to always match the expectations of this code. I will pass on
more information into diff_populate_filespec so an explicit check can
be done here.

> > @@ -2825,6 +2825,10 @@ int diff_populate_filespec(struct diff_filespec *s, 
> > unsigned int flags)
> > s->should_free = 1;
> > return 0;
> > }
> > +   if (S_ISLNK(st.st_mode)) {
> > +   stat(s->path, &st);
> > +   s->size = xsize_t(st.st_size);
> > +   }
> > if (size_only)
> > return 0;
> > if ((flags & CHECK_BINARY) &&
> 
> I suspect that this would conflict with a recent topic.  

Possibly. I used the same base commit for the newer versions as that
seems to be your preference. If there is a merge conflict, do you want
me to rebase against current master?

> But more importantly, this inserted code feels doubly wrong.
> 
>  - what allows us to unconditionally do "ah, symbolic link on the
>disk--find the target of the link, not the symbolic link itself"?
>We do not seem to be checking '--dereference' around here.

The implicit check above (which you already noted is faulty) allows us
to do this. So fixing the check above will also involve fixing this.

>  - does this code do a reasonable thing when the path is a symbolic
>link that points at a directory?  what does it mean to grab
>st.st_size for such a thing (and then go on to open() and xmmap()
>it)?

No, it does something entirely unreasonable. I hadn't even thought of
testing with symlinks to directories, as my ulterior motive was the
next commit that makes it work with pipes. This will be fixed.

Thanks very much for the thoroughness of your review!

D.

Re: [PATCH/RFC] parse-options: add facility to make options configurable

On Sat, Mar 25, 2017 at 05:47:49PM +0100, Ævar Arnfjörð Bjarmason wrote:

> After looking at some of the internal APIs I'm thinking of replacing
> this pattern with a hashmap.c hashmap where the keys are a
> sprintf("%d:%s", short_name, long_name) to uniquely identify the
> option. There's no other obvious way to uniquely address an option. I
> guess I could just equivalently and more cheaply use the memory
> address of each option to identify them, but that seems a bit hacky.

Rather than bolt this onto the parse-options code, what if it were a
separate mechanism that mapped config keys to options. E.g., imagine
something like:

  struct {
const char *config;
const char *option;
enum {
CONFIG_CMDLINE_BOOL,
CONFIG_CMDLINE_MAYBE_BOOL,
CONFIG_CMDLINE_VALUE
} type;
  } config_cmdline_map[] = {
{ "foo.one", "one", CONFIG_CMDLINE_BOOL },
{ "foo.two", "two", CONFIG_CMDLINE_VALUE },
  };

And then you "apply" that mapping by finding each item that's set in the
config, and then pretending that "--one" or "--two=" was set on
the command-line, before anything the user has said. This works as long
as the options use the normal last-one-wins rules, the user can
countermand with "--no-one", etc.

You have to write one line for each config/option mapping, but I think
we would need some amount of per-option anyway (i.e., I think the
decision was that options would have to be manually approved for use in
the system). So rather than a flag in the options struct, it becomes a
line in this mapping.

And you get two extra pieces of flexibility:

  1. The config names can map to option names however makes sense; we're
 not constrained by some programmatic rule (I think we _would_
 follow some general guidelines, but there are probably special
 cases for historic config, etc).

  2. A command can choose to apply one or more mappings, or not. So
 porcelain like git-log would call:

   struct option options[] = {...};
   apply_config_cmdline_map(revision_config_mapping, options);
   apply_config_cmdline_map(diff_config_mapping, options);
   apply_config_cmdline_map(log_mapping, options);

 but plumbing like git-diff-tree wouldn't call any of those.

I had in mind that apply_config_cmdline_map() would just call
parse_options, but I think even that is too constricting. The revision
and diff options don't use parse_options at all. So really, it would
probably be more like:

  struct argv_array fake_args = ARGV_ARRAY_INIT;
  apply_config_cmdline_map(revision_config_mapping, &fake_args);
  apply_config_cmdline_map(diff_config_mapping, &fake_args);
  apply_config_cmdline_map(log_mapping, &fake_args);
  argv_array_pushv(&fake_args, argv); /* add the real ones */

At this point we've recreated internally the related suggestion:

  [options]
  log = --one --two=whatever

which is the same as:

  [log]
  one = true
  two = whatever

So hopefully it's clear that the two are functionally equivalent, and
differ only in syntax (in this case we manually decided which options
are safe to pull from the config, but we'd have to parse the options.log
string, too, and we could make the same decision there).

-Peff

Re: Will OpenSSL's license change impact us?

On Sat, Mar 25, 2017 at 10:11 PM, Theodore Ts'o  wrote:
> On Sat, Mar 25, 2017 at 06:51:21PM +0100, Ęvar Arnfjörš Bjarmason wrote:
>> In GPLv3 projects only, not GPLv2 projects. The paragraphs you're
>> quoting all explicitly mention v3 only, so statements like
>> "incompatible in one direction" only apply to Apache 2 && GPLv3, but
>> don't at all apply to GPLv2, which is what we're using.
>
> It's complicated.
>
> It's fair enough to say that the FSF adopts a copyright maximalist
> position, and by their interpretation, the two licenses are
> incompatible, and it doesn't matter whether the two pieces of code are
> linked staticaly, dynamically, or one calls the other over an RPC
> call.
>
> Not everyone agrees with their legal analysis.  May I suggest that we
> not play amateur lawyer on the mailing list, and try to settle this
> here?  Each distribution can make its own decision, which may be based
> on its legal advice, the local laws and legal precedents in which they
> operate, etc.  And indeed, different distributions have already come
> to different conclusions with respect to various license compatibility
> issues.  (Examples: dynamically linking GPL programs with OpenSSL
> libraries under the old license, distributing ZFS modules for Linux,
> etc.)
>
> We don't expect lawyers to debug edge cases in a compiler's code
> generation.  Programmers shouldn't try to parse edge cases in the law,
> or try to use a soldering iron, unless they have explicit training and
> expertise to do so.  :-)

Yeah fully agree with the internet lawyering. I'm not looking for
that, just seeing if someone knows if this might be an issue for at
least some distros, then it's something for us to keep an eye on if
OpenSSL's license changes, and a sane default for us to adopt might be
to e.g. require that some flag be passed to the Makefile declaring
"yes I'm OK with combining AL2 + GPLv2" if the OpenSSL version is
newer than so-and-so.

Re: Will OpenSSL's license change impact us?

On Sat, Mar 25, 2017 at 10:44 PM, brian m. carlson
 wrote:
> On Sat, Mar 25, 2017 at 12:51:52AM +0100, Ævar Arnfjörð Bjarmason wrote:
>> They're changing their license[1] to Apache 2 which unlike the current
>> fuzzy compatibility with the current license[2] is explicitly
>> incompatible with GPLv2[3].
>>
>> We use OpenSSL for SHA1 by default unless NO_OPENSSL=YesPlease.
>>
>> This still hasn't happened, but given the lifetime of git versions
>> packaged up by distros knowing sooner than later if this is going to
>> be a practical problem would be good.
>>
>> If so perhaps we could copy the relevant subset of the code int our
>> tree, or libressl's, or improve block-sha1.
>
> I think that most distros don't link against OpenSSL because they can't
> take advantage of the system library exception.  I don't think that's
> going to change.

"ldd -r" against git itself on my Debian testing doesn't return
libssl, but git-imap-send is dynamically linked to it:

$ ldd -r /usr/lib/git-core/git-imap-send|grep ssl
libssl.so.1.0.2 => /usr/lib/x86_64-linux-gnu/libssl.so.1.0.2
(0x7f244b2f6000)
$ apt-cache show libssl1.0.2:amd64
Package: libssl1.0.2
Source: openssl1.0
Version: 1.0.2k-1
[...]
Homepage: https://www.openssl.org

> If we want to consider performance-related concerns, I think the easier
> solution is using Nettle, which is LGPL 2.1.  Considering that the
> current opinions for a new hash function are moving in the direction of
> SHA-3, which Nettle has, but OpenSSL does not, I think that might be a
> better decision overall.  It was certainly the implementation I would
> use if I were to implement it.

Yeah there's a lot of options open for just sha1-ing, but we also use
OpenSSL for TLS via imap-send.

Re: Will OpenSSL's license change impact us?

2017-03-25 Thread brian m. carlson

On Sat, Mar 25, 2017 at 12:51:52AM +0100, Ævar Arnfjörð Bjarmason wrote:
> They're changing their license[1] to Apache 2 which unlike the current
> fuzzy compatibility with the current license[2] is explicitly
> incompatible with GPLv2[3].
> 
> We use OpenSSL for SHA1 by default unless NO_OPENSSL=YesPlease.
> 
> This still hasn't happened, but given the lifetime of git versions
> packaged up by distros knowing sooner than later if this is going to
> be a practical problem would be good.
> 
> If so perhaps we could copy the relevant subset of the code int our
> tree, or libressl's, or improve block-sha1.

I think that most distros don't link against OpenSSL because they can't
take advantage of the system library exception.  I don't think that's
going to change.

If we want to consider performance-related concerns, I think the easier
solution is using Nettle, which is LGPL 2.1.  Considering that the
current opinions for a new hash function are moving in the direction of
SHA-3, which Nettle has, but OpenSSL does not, I think that might be a
better decision overall.  It was certainly the implementation I would
use if I were to implement it.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

EMAIL BENUTZER

2017-03-25 Thread HEIDI MEIER




-- 
Attn: Gewinner

Ihre E-Mail-Adresse hat die Summe von 1,200.000.00 Euro gewonnen.
Im Email Benützer Online Programm Weihnachtslotterie Navida , in Madrid
Spanien .
Wir schreiben ihnen ,um sich offiziel über die Auszeichnung zu
Benachrichtigen ,
damit Sie sich mit der Zuständigen Vermittlerin in verbindung setzten können.
Frau HEIDI  MEIER, per E-Mail: heidi_mei...@aol.com
Zur Bearbeitung und Freigabe Ihrer Zahlung per Scheck oder Banküberweisung.
Bitte füllen Sie das untenstehende Formular aus.
NAME: ___
FAMILIENNAME:_
ADRESSE:__
STADT: PLZ: LAND: ___
GEB: DATUM: __BERUF:
FESTNETZ TEL.NR: 
MOBILETELEFON NR: ___
FAX: ___
EMAIL:___
DATE
SIGNATURE:_

bitte füllen Sie das anschließende Formular vollständig aus und senden es
per email zurück !
Hochachtungsvoll
Francisco Blázquez
Koordinator.

Re: [PATCH/RFC] parse-options: add facility to make options configurable

On Sat, Mar 25, 2017 at 10:31 PM, Jeff King  wrote:
> On Sat, Mar 25, 2017 at 05:47:49PM +0100, Ævar Arnfjörð Bjarmason wrote:
>
>> After looking at some of the internal APIs I'm thinking of replacing
>> this pattern with a hashmap.c hashmap where the keys are a
>> sprintf("%d:%s", short_name, long_name) to uniquely identify the
>> option. There's no other obvious way to uniquely address an option. I
>> guess I could just equivalently and more cheaply use the memory
>> address of each option to identify them, but that seems a bit hacky.
>
> Rather than bolt this onto the parse-options code, what if it were a
> separate mechanism that mapped config keys to options. E.g., imagine
> something like:
>
>   struct {
> const char *config;
> const char *option;
> enum {
> CONFIG_CMDLINE_BOOL,
> CONFIG_CMDLINE_MAYBE_BOOL,
> CONFIG_CMDLINE_VALUE
> } type;
>   } config_cmdline_map[] = {
> { "foo.one", "one", CONFIG_CMDLINE_BOOL },
> { "foo.two", "two", CONFIG_CMDLINE_VALUE },
>   };
>
> And then you "apply" that mapping by finding each item that's set in the
> config, and then pretending that "--one" or "--two=" was set on
> the command-line, before anything the user has said. This works as long
> as the options use the normal last-one-wins rules, the user can
> countermand with "--no-one", etc.
>
> You have to write one line for each config/option mapping, but I think
> we would need some amount of per-option anyway (i.e., I think the
> decision was that options would have to be manually approved for use in
> the system). So rather than a flag in the options struct, it becomes a
> line in this mapping.
>
> And you get two extra pieces of flexibility:
>
>   1. The config names can map to option names however makes sense; we're
>  not constrained by some programmatic rule (I think we _would_
>  follow some general guidelines, but there are probably special
>  cases for historic config, etc).
>
>   2. A command can choose to apply one or more mappings, or not. So
>  porcelain like git-log would call:
>
>struct option options[] = {...};
>apply_config_cmdline_map(revision_config_mapping, options);
>apply_config_cmdline_map(diff_config_mapping, options);
>apply_config_cmdline_map(log_mapping, options);
>
>  but plumbing like git-diff-tree wouldn't call any of those.
>
> I had in mind that apply_config_cmdline_map() would just call
> parse_options, but I think even that is too constricting. The revision
> and diff options don't use parse_options at all. So really, it would
> probably be more like:
>
>   struct argv_array fake_args = ARGV_ARRAY_INIT;
>   apply_config_cmdline_map(revision_config_mapping, &fake_args);
>   apply_config_cmdline_map(diff_config_mapping, &fake_args);
>   apply_config_cmdline_map(log_mapping, &fake_args);
>   argv_array_pushv(&fake_args, argv); /* add the real ones */
>
> At this point we've recreated internally the related suggestion:
>
>   [options]
>   log = --one --two=whatever
>
> which is the same as:
>
>   [log]
>   one = true
>   two = whatever
>
> So hopefully it's clear that the two are functionally equivalent, and
> differ only in syntax (in this case we manually decided which options
> are safe to pull from the config, but we'd have to parse the options.log
> string, too, and we could make the same decision there).

I like the simplicity of this approach a lot. I.e. (to paraphrase it
just to make sure we're on the same page): Skip all the complexity of
reaching into the getopt guts, and just munge argc/argv given a config
we can stick ahead of the getopt (struct options) spec, inject some
options at the beginning if they're in the config, and off we go
without any further changes to the getopt guts.

There's two practical issues with this that are easy to solve with my
current approach, but I can't find an easy solution to using this
method.

The first is that we're replacing the semantics of:

"If you're specifying it on the command-line, we take it from there,
otherwise we use your config, if set, regardless of how the option
works"

with:

"We read your config, inject options implicitly at the start of the
command line, and then append whatever command-line you give us"

These two are not the same. Consider e.g. the commit.verbose config.
With my current patch if have commit.verbose=1 in your config and do
"commit --verbose" you just end up with a result equivalent to not
having it in your config, but since the --verbose option can be
supplied multiple times to increase verbosity with the injection
method you'd end up with the equivalent of commit.verbose=2.

I think the semantics I've implemented are much less confusing for the
user, i.e. you can specify an option that's configurable and know that
you're overriding your config, not potentially joining the
command-line with whatever's in your config. We have a lot of options
without last

Re: [PATCH v6 18/27] files-backend: replace submodule_allowed check in files_downcast()

On Mon, Mar 20, 2017 at 4:18 AM, Michael Haggerty  wrote:
>> +/* ref_store_init flags */
>> +#define REF_STORE_READ   (1 << 0)
>
> I asked [1] in reply to v5 whether `REF_STORE_READ` is really necessary
> but I don't think you replied. Surely a reference store that we can't
> read would be useless? Can't we just assume that any `ref_store` is
> readable and drop this constant?

I deleted it then I realized many lines like these

files_downcast(ref_store, REF_STORE_READ, "read_raw_ref");

would become

files_downcast(ref_store, 0, "read_raw_ref");

I think for newcomers, the former gives a better hint what the second
argument is than a plain zero in the latter, so I'm inclined to keep
it.
-- 
Duy

[PATCH] notes: Fix note_tree_consolidate not to break the note_tree structure

2017-03-25 Thread Mike Hommey

After a note is removed, note_tree_consolidate is called to eliminate
some useless nodes. The typical case is that if you had an int_node
with 2 PTR_TYPE_NOTEs in it, and remove one of them, then the
PTR_TYPE_INTERNAL pointer in the parent tree can be replaced with the
remaining PTR_TYPE_NOTE.

This works fine when PTR_TYPE_NOTEs are involved, but falls flat when
other types are involved.

To put things in more practical terms, let's say we start from an empty
notes tree, and add 3 notes:
- one for a sha1 that starts with 424
- one for a sha1 that starts with 428
- one for a sha1 that starts with 4c

To keep track of this, note_tree.root will have a PTR_TYPE_INTERNAL at
a[4], pointing to an int_node*.
In turn, that int_node* will have a PTR_TYPE_NOTE at a[0xc], pointing to
the leaf_node* with the key and value, and a PTR_TYPE_INTERNAL at a[2],
pointing to another int_node*.
That other int_node* will have 2 PTR_TYPE_NOTE, one at a[4] and the
other at a[8].

When looking for the note for the sha1 starting with 428, get_note() will
recurse through (simplified) root.a[4].a[2].a[8].

Now, if we remove the note for the sha1 that starts with 4c, we're left
with a int_node* with only one PTR_TYPE_INTERNAL entry in it. After
note_tree_consolidate runs, root.a[4] now points to what used to be
pointed at by root.a[4].a[2].

Which means looking up for the note for the sha1 starting with 428 now
fails because there is nothing at root.a[4].a[2] anymore: there is only
root.a[4].a[4] and root.a[4].a[8], which don't match the expected
structure for the lookup.

So if all there is left in an int_node* is a PTR_TYPE_INTERNAL pointer,
we can't safely remove it. I think the same applies for PTR_TYPE_SUBTREE
pointers. IOW, only PTR_TYPE_NOTEs are safe to be moved to the parent
int_node*.

This doesn't have a practical effect on git because all that happens
after a remove_note is a write_notes_tree, which just iterates the entire
note tree, but this affects anything using libgit.a that would try to do
lookups after removing notes.

Signed-off-by: Mike Hommey 
---
 notes.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/notes.c b/notes.c
index 2bab961ac..542563b28 100644
--- a/notes.c
+++ b/notes.c
@@ -153,8 +153,8 @@ static struct leaf_node *note_tree_find(struct notes_tree 
*t,
  * How to consolidate an int_node:
  * If there are > 1 non-NULL entries, give up and return non-zero.
  * Otherwise replace the int_node at the given index in the given parent node
- * with the only entry (or a NULL entry if no entries) from the given tree,
- * and return 0.
+ * with the only NOTE entry (or a NULL entry if no entries) from the given
+ * tree, and return 0.
  */
 static int note_tree_consolidate(struct int_node *tree,
struct int_node *parent, unsigned char index)
@@ -173,6 +173,8 @@ static int note_tree_consolidate(struct int_node *tree,
}
}
 
+   if (p && (GET_PTR_TYPE(p) != PTR_TYPE_NOTE))
+   return -2;
/* replace tree with p in parent[index] */
parent->a[index] = p;
free(tree);
-- 
2.12.1.dirty

[PATCH v7 02/28] files-backend: make files_log_ref_write() static

Created in 5f3c3a4e6f (files_log_ref_write: new function - 2015-11-10)
but probably never used outside refs-internal.c

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 refs/files-backend.c | 9 ++---
 refs/refs-internal.h | 4 
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 50188e92f9..42c0cc14f3 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -165,6 +165,9 @@ 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);
+static int files_log_ref_write(const char *refname, const unsigned char 
*old_sha1,
+  const unsigned char *new_sha1, const char *msg,
+  int flags, struct strbuf *err);
 
 static struct ref_dir *get_ref_dir(struct ref_entry *entry)
 {
@@ -2831,9 +2834,9 @@ static int log_ref_write_fd(int fd, const unsigned char 
*old_sha1,
return 0;
 }
 
-int files_log_ref_write(const char *refname, const unsigned char *old_sha1,
-   const unsigned char *new_sha1, const char *msg,
-   int flags, struct strbuf *err)
+static int files_log_ref_write(const char *refname, const unsigned char 
*old_sha1,
+  const unsigned char *new_sha1, const char *msg,
+  int flags, struct strbuf *err)
 {
int logfd, result;
 
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index fa93c9a32e..f732473e1d 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -228,10 +228,6 @@ struct ref_transaction {
enum ref_transaction_state state;
 };
 
-int files_log_ref_write(const char *refname, const unsigned char *old_sha1,
-   const unsigned char *new_sha1, const char *msg,
-   int flags, struct strbuf *err);
-
 /*
  * Check for entries in extras that are within the specified
  * directory, where dirname is a reference directory name including
-- 
2.11.0.157.gd943d85

[PATCH v7 00/28] Remove submodule from files-backend.c

v7 is mostly about style changes except the one bug in
test-ref-store.c, missing setup_git_directory().

There's one new patch, 03/28, which maps to the "if (!refs)" deletion
in the interdiff.

The one comment from v6 I haven't addressed in v7 is whether to delete
REF_STORE_READ. But if it is deleted, I think I'm doing it in
nd/worktree-kill-parse-ref, which is partly about cleanup refs code
anyway, to avoid another re-roll in this series.

diff --git a/refs.c b/refs.c
index 77a39f8b17..ec1f563824 100644
--- a/refs.c
+++ b/refs.c
@@ -1523,18 +1523,15 @@ static struct ref_store *ref_store_init(const char 
*gitdir,
 
 struct ref_store *get_main_ref_store(void)
 {
-   struct ref_store *refs;
-
if (main_ref_store)
return main_ref_store;
 
-   refs = ref_store_init(get_git_dir(),
- (REF_STORE_READ |
-  REF_STORE_WRITE |
-  REF_STORE_ODB |
-  REF_STORE_MAIN));
-   main_ref_store = refs;
-   return refs;
+   main_ref_store = ref_store_init(get_git_dir(),
+   (REF_STORE_READ |
+REF_STORE_WRITE |
+REF_STORE_ODB |
+REF_STORE_MAIN));
+   return main_ref_store;
 }
 
 /*
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 4242486118..a5b405436f 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1195,9 +1195,9 @@ static void files_reflog_path(struct files_ref_store 
*refs,
}
 }
 
-static void files_refname_path(struct files_ref_store *refs,
-  struct strbuf *sb,
-  const char *refname)
+static void files_ref_path(struct files_ref_store *refs,
+  struct strbuf *sb,
+  const char *refname)
 {
switch (ref_type(refname)) {
case REF_TYPE_PER_WORKTREE:
@@ -1283,7 +1283,7 @@ static void read_loose_refs(const char *dirname, struct 
ref_dir *dir)
struct strbuf path = STRBUF_INIT;
size_t path_baselen;
 
-   files_refname_path(refs, &path, dirname);
+   files_ref_path(refs, &path, dirname);
path_baselen = path.len;
 
d = opendir(path.buf);
@@ -1420,7 +1420,7 @@ static int files_read_raw_ref(struct ref_store *ref_store,
*type = 0;
strbuf_reset(&sb_path);
 
-   files_refname_path(refs, &sb_path, refname);
+   files_ref_path(refs, &sb_path, refname);
 
path = sb_path.buf;
 
@@ -1608,7 +1608,7 @@ static int lock_raw_ref(struct files_ref_store *refs,
*lock_p = lock = xcalloc(1, sizeof(*lock));
 
lock->ref_name = xstrdup(refname);
-   files_refname_path(refs, &ref_file, refname);
+   files_ref_path(refs, &ref_file, refname);
 
 retry:
switch (safe_create_leading_directories(ref_file.buf)) {
@@ -1949,8 +1949,6 @@ static struct ref_iterator *files_ref_iterator_begin(
refs = files_downcast(ref_store,
  REF_STORE_READ | (ref_paranoia ? 0 : 
REF_STORE_ODB),
  "ref_iterator_begin");
-   if (!refs)
-   return empty_ref_iterator_begin();
 
iter = xcalloc(1, sizeof(*iter));
ref_iterator = &iter->base;
@@ -2086,7 +2084,7 @@ static struct ref_lock *lock_ref_sha1_basic(struct 
files_ref_store *refs,
if (flags & REF_DELETING)
resolve_flags |= RESOLVE_REF_ALLOW_BAD_NAME;
 
-   files_refname_path(refs, &ref_file, refname);
+   files_ref_path(refs, &ref_file, refname);
resolved = !!refs_resolve_ref_unsafe(&refs->base,
 refname, resolve_flags,
 lock->old_oid.hash, type);
@@ -2387,7 +2385,7 @@ static void try_remove_empty_parents(struct 
files_ref_store *refs,
strbuf_setlen(&buf, q - buf.buf);
 
strbuf_reset(&sb);
-   files_refname_path(refs, &sb, buf.buf);
+   files_ref_path(refs, &sb, buf.buf);
if ((flags & REMOVE_EMPTY_PARENTS_REF) && rmdir(sb.buf))
flags &= ~REMOVE_EMPTY_PARENTS_REF;
 
@@ -2709,7 +2707,7 @@ static int files_rename_ref(struct ref_store *ref_store,
struct strbuf path = STRBUF_INIT;
int result;
 
-   files_refname_path(refs, &path, newrefname);
+   files_ref_path(refs, &path, newrefname);
result = remove_empty_directories(&path);
strbuf_release(&path);
 
@@ -2935,10 +2933,10 @@ static int log_ref_write_fd(int fd, const unsigned char 
*old_sha1,
return 0;
 }
 
-int files_log_ref_write(struct files_ref_store *refs,
-   const char *refname, const unsigned char *old_sha1,
-   const un

[PATCH v7 01/28] refs.h: add forward declaration for structs used in this file

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 refs.h | 4 
 1 file changed, 4 insertions(+)

diff --git a/refs.h b/refs.h
index 3df0d45ebb..2d6b6263fc 100644
--- a/refs.h
+++ b/refs.h
@@ -1,6 +1,10 @@
 #ifndef REFS_H
 #define REFS_H
 
+struct object_id;
+struct strbuf;
+struct string_list;
+
 /*
  * Resolve a reference, recursively following symbolic refererences.
  *
-- 
2.11.0.157.gd943d85

[PATCH v7 04/28] files-backend: delete dead code in files_init_db()

safe_create_dir() can do adjust_shared_perm() internally, and init-db
has always created 'refs' in shared mode since the beginning,
af6e277c5e (git-init-db: initialize shared repositories with --shared -
2005-12-22). So this code looks like extra adjust_shared_perm calls are
unnecessary.

And they are. But let's see why there are here in the first place.

This code was added in 6fb5acfd8f (refs: add methods to init refs db -
2016-09-04). From the diff alone this looks like a faithful refactored
code from init-db.c. But there is a subtle difference:

Between the safe_create_dir() block and adjust_shared_perm() block in
the old init-db.c, we may copy/recreate directories from the repo
template. So it makes sense that adjust_shared_perm() is re-executed
then to fix potential permission screwups.

After 6fb5acfd8f, refs dirs are created after template is copied. Nobody
will change directory permission again. So the extra adjust_shared_perm()
is redudant. Delete them.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 refs/files-backend.c | 4 
 1 file changed, 4 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index caeb8188fd..9a9dc4e50c 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -4107,10 +4107,6 @@ static int files_init_db(struct ref_store *ref_store, 
struct strbuf *err)
 */
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;
 }
 
-- 
2.11.0.157.gd943d85

[PATCH v7 11/28] files-backend: remove the use of git_path()

Given $GIT_DIR and $GIT_COMMON_DIR, files-backend is now in charge of
deciding what goes where (*). The end goal is to pass $GIT_DIR only. A
refs "view" of a linked worktree is a logical ref store that combines
two files backends together.

(*) Not entirely true since strbuf_git_path_submodule() still does path
translation underneath. But that's for another patch.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 refs/files-backend.c | 43 ++-
 1 file changed, 38 insertions(+), 5 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 741e52d143..9676cd32e4 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -923,7 +923,8 @@ struct files_ref_store {
 * store:
 */
const char *submodule;
-
+   char *gitdir;
+   char *gitcommondir;
char *packed_refs_path;
 
struct ref_entry *loose;
@@ -985,6 +986,8 @@ static struct ref_store *files_ref_store_create(const char 
*submodule)
 {
struct files_ref_store *refs = xcalloc(1, sizeof(*refs));
struct ref_store *ref_store = (struct ref_store *)refs;
+   struct strbuf sb = STRBUF_INIT;
+   const char *gitdir = get_git_dir();
 
base_ref_store_init(ref_store, &refs_be_files);
 
@@ -995,7 +998,11 @@ static struct ref_store *files_ref_store_create(const char 
*submodule)
return ref_store;
}
 
-   refs->packed_refs_path = git_pathdup("packed-refs");
+   refs->gitdir = xstrdup(gitdir);
+   get_common_dir_noenv(&sb, gitdir);
+   refs->gitcommondir = strbuf_detach(&sb, NULL);
+   strbuf_addf(&sb, "%s/packed-refs", refs->gitcommondir);
+   refs->packed_refs_path = strbuf_detach(&sb, NULL);
 
return ref_store;
 }
@@ -1173,11 +1180,26 @@ static void files_reflog_path(struct files_ref_store 
*refs,
  const char *refname)
 {
if (!refname) {
-   strbuf_git_path(sb, "logs");
+   /*
+* FIXME: of course this is wrong in multi worktree
+* setting. To be fixed real soon.
+*/
+   strbuf_addf(sb, "%s/logs", refs->gitcommondir);
return;
}
 
-   strbuf_git_path(sb, "logs/%s", refname);
+   switch (ref_type(refname)) {
+   case REF_TYPE_PER_WORKTREE:
+   case REF_TYPE_PSEUDOREF:
+   strbuf_addf(sb, "%s/logs/%s", refs->gitdir, refname);
+   break;
+   case REF_TYPE_NORMAL:
+   strbuf_addf(sb, "%s/logs/%s", refs->gitcommondir, refname);
+   break;
+   default:
+   die("BUG: unknown ref type %d of ref %s",
+   ref_type(refname), refname);
+   }
 }
 
 static void files_ref_path(struct files_ref_store *refs,
@@ -1189,7 +1211,18 @@ static void files_ref_path(struct files_ref_store *refs,
return;
}
 
-   strbuf_git_path(sb, "%s", refname);
+   switch (ref_type(refname)) {
+   case REF_TYPE_PER_WORKTREE:
+   case REF_TYPE_PSEUDOREF:
+   strbuf_addf(sb, "%s/%s", refs->gitdir, refname);
+   break;
+   case REF_TYPE_NORMAL:
+   strbuf_addf(sb, "%s/%s", refs->gitcommondir, refname);
+   break;
+   default:
+   die("BUG: unknown ref type %d of ref %s",
+   ref_type(refname), refname);
+   }
 }
 
 /*
-- 
2.11.0.157.gd943d85

[PATCH v7 06/28] files-backend: make sure files_rename_ref() always reach the end

This is a no-op patch. It prepares the function so that we can release
resources (to be added later in this function) before we return.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 refs/files-backend.c | 42 --
 1 file changed, 28 insertions(+), 14 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index f17daddffa..6d0fcc88f9 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2585,23 +2585,34 @@ static int files_rename_ref(struct ref_store *ref_store,
struct stat loginfo;
int log = !lstat(git_path("logs/%s", oldrefname), &loginfo);
struct strbuf err = STRBUF_INIT;
+   int ret;
 
-   if (log && S_ISLNK(loginfo.st_mode))
-   return error("reflog for %s is a symlink", oldrefname);
+   if (log && S_ISLNK(loginfo.st_mode)) {
+   ret = error("reflog for %s is a symlink", oldrefname);
+   goto out;
+   }
 
if (!resolve_ref_unsafe(oldrefname, RESOLVE_REF_READING | 
RESOLVE_REF_NO_RECURSE,
-   orig_sha1, &flag))
-   return error("refname %s not found", oldrefname);
+   orig_sha1, &flag)) {
+   ret = error("refname %s not found", oldrefname);
+   goto out;
+   }
 
-   if (flag & REF_ISSYMREF)
-   return error("refname %s is a symbolic ref, renaming it is not 
supported",
-   oldrefname);
-   if (!rename_ref_available(oldrefname, newrefname))
-   return 1;
+   if (flag & REF_ISSYMREF) {
+   ret = error("refname %s is a symbolic ref, renaming it is not 
supported",
+   oldrefname);
+   goto out;
+   }
+   if (!rename_ref_available(oldrefname, newrefname)) {
+   ret = 1;
+   goto out;
+   }
 
-   if (log && rename(git_path("logs/%s", oldrefname), 
git_path(TMP_RENAMED_LOG)))
-   return error("unable to move logfile logs/%s to 
"TMP_RENAMED_LOG": %s",
-   oldrefname, strerror(errno));
+   if (log && rename(git_path("logs/%s", oldrefname), 
git_path(TMP_RENAMED_LOG))) {
+   ret = error("unable to move logfile logs/%s to 
"TMP_RENAMED_LOG": %s",
+   oldrefname, strerror(errno));
+   goto out;
+   }
 
if (delete_ref(logmsg, oldrefname, orig_sha1, REF_NODEREF)) {
error("unable to delete old %s", oldrefname);
@@ -2657,7 +2668,8 @@ static int files_rename_ref(struct ref_store *ref_store,
goto rollback;
}
 
-   return 0;
+   ret = 0;
+   goto out;
 
  rollback:
lock = lock_ref_sha1_basic(refs, oldrefname, NULL, NULL, NULL,
@@ -2686,7 +2698,9 @@ static int files_rename_ref(struct ref_store *ref_store,
error("unable to restore logfile %s from "TMP_RENAMED_LOG": %s",
oldrefname, strerror(errno));
 
-   return 1;
+   ret = 1;
+ out:
+   return ret;
 }
 
 static int close_ref(struct ref_lock *lock)
-- 
2.11.0.157.gd943d85

[PATCH v7 15/28] refs.c: kill register_ref_store(), add register_submodule_ref_store()

This is the last function in this code (besides public API) that takes
submodule argument and handles both main/submodule cases. Break it down,
move main store registration in get_main_ref_store() and keep the rest
in register_submodule_ref_store().

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 refs.c | 45 -
 1 file changed, 20 insertions(+), 25 deletions(-)

diff --git a/refs.c b/refs.c
index 66dc84787d..87f64271ac 100644
--- a/refs.c
+++ b/refs.c
@@ -1412,29 +1412,6 @@ static struct ref_store 
*lookup_submodule_ref_store(const char *submodule)
 }
 
 /*
- * Register the specified ref_store to be the one that should be used
- * for submodule (or the main repository if submodule is NULL). It is
- * a fatal error to call this function twice for the same submodule.
- */
-static void register_ref_store(struct ref_store *refs, const char *submodule)
-{
-   if (!submodule) {
-   if (main_ref_store)
-   die("BUG: main_ref_store initialized twice");
-
-   main_ref_store = refs;
-   } else {
-   if (!submodule_ref_stores.tablesize)
-   hashmap_init(&submodule_ref_stores, submodule_hash_cmp, 
0);
-
-   if (hashmap_put(&submodule_ref_stores,
-   alloc_submodule_hash_entry(submodule, refs)))
-   die("BUG: ref_store for submodule '%s' initialized 
twice",
-   submodule);
-   }
-}
-
-/*
  * Create, record, and return a ref_store instance for the specified
  * submodule (or the main repository if submodule is NULL).
  */
@@ -1448,7 +1425,6 @@ static struct ref_store *ref_store_init(const char 
*submodule)
die("BUG: reference backend %s is unknown", be_name);
 
refs = be->init(submodule);
-   register_ref_store(refs, submodule);
return refs;
 }
 
@@ -1457,7 +1433,25 @@ static struct ref_store *get_main_ref_store(void)
if (main_ref_store)
return main_ref_store;
 
-   return ref_store_init(NULL);
+   main_ref_store = ref_store_init(NULL);
+   return main_ref_store;
+}
+
+/*
+ * Register the specified ref_store to be the one that should be used
+ * for submodule. It is a fatal error to call this function twice for
+ * the same submodule.
+ */
+static void register_submodule_ref_store(struct ref_store *refs,
+const char *submodule)
+{
+   if (!submodule_ref_stores.tablesize)
+   hashmap_init(&submodule_ref_stores, submodule_hash_cmp, 0);
+
+   if (hashmap_put(&submodule_ref_stores,
+   alloc_submodule_hash_entry(submodule, refs)))
+   die("BUG: ref_store for submodule '%s' initialized twice",
+   submodule);
 }
 
 struct ref_store *get_ref_store(const char *submodule)
@@ -1481,6 +1475,7 @@ struct ref_store *get_ref_store(const char *submodule)
return NULL;
 
refs = ref_store_init(submodule);
+   register_submodule_ref_store(refs, submodule);
return refs;
 }
 
-- 
2.11.0.157.gd943d85

[PATCH v7 09/28] files-backend: add and use files_reflog_path()

Keep repo-related path handling in one place. This will make it easier
to add submodule/multiworktree support later.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 refs/files-backend.c | 142 +++
 1 file changed, 86 insertions(+), 56 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 3e0bafcaf9..b0e0df9f66 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -165,7 +165,8 @@ 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);
-static int files_log_ref_write(const char *refname, const unsigned char 
*old_sha1,
+static int files_log_ref_write(struct files_ref_store *refs,
+  const char *refname, const unsigned char 
*old_sha1,
   const unsigned char *new_sha1, const char *msg,
   int flags, struct strbuf *err);
 
@@ -1167,6 +1168,18 @@ static const char *files_packed_refs_path(struct 
files_ref_store *refs)
return refs->packed_refs_path;
 }
 
+static void files_reflog_path(struct files_ref_store *refs,
+ struct strbuf *sb,
+ const char *refname)
+{
+   if (!refname) {
+   strbuf_git_path(sb, "logs");
+   return;
+   }
+
+   strbuf_git_path(sb, "logs/%s", refname);
+}
+
 /*
  * Get the packed_ref_cache for the specified files_ref_store,
  * creating it if necessary.
@@ -2313,7 +2326,9 @@ enum {
  * subdirs. flags is a combination of REMOVE_EMPTY_PARENTS_REF and/or
  * REMOVE_EMPTY_PARENTS_REFLOG.
  */
-static void try_remove_empty_parents(const char *refname, unsigned int flags)
+static void try_remove_empty_parents(struct files_ref_store *refs,
+const char *refname,
+unsigned int flags)
 {
struct strbuf buf = STRBUF_INIT;
struct strbuf sb = STRBUF_INIT;
@@ -2345,7 +2360,7 @@ static void try_remove_empty_parents(const char *refname, 
unsigned int flags)
flags &= ~REMOVE_EMPTY_PARENTS_REF;
 
strbuf_reset(&sb);
-   strbuf_git_path(&sb, "logs/%s", buf.buf);
+   files_reflog_path(refs, &sb, buf.buf);
if ((flags & REMOVE_EMPTY_PARENTS_REFLOG) && rmdir(sb.buf))
flags &= ~REMOVE_EMPTY_PARENTS_REFLOG;
}
@@ -2538,15 +2553,15 @@ static int rename_tmp_log_callback(const char *path, 
void *cb_data)
}
 }
 
-static int rename_tmp_log(const char *newrefname)
+static int rename_tmp_log(struct files_ref_store *refs, const char *newrefname)
 {
struct strbuf path = STRBUF_INIT;
struct strbuf tmp = STRBUF_INIT;
struct rename_cb cb;
int ret;
 
-   strbuf_git_path(&path, "logs/%s", newrefname);
-   strbuf_git_path(&tmp, "logs/%s", TMP_RENAMED_LOG);
+   files_reflog_path(refs, &path, newrefname);
+   files_reflog_path(refs, &tmp, TMP_RENAMED_LOG);
cb.tmp_renamed_log = tmp.buf;
ret = raceproof_create_file(path.buf, rename_tmp_log_callback, &cb);
if (ret) {
@@ -2606,9 +2621,9 @@ static int files_rename_ref(struct ref_store *ref_store,
int log, ret;
struct strbuf err = STRBUF_INIT;
 
-   strbuf_git_path(&sb_oldref, "logs/%s", oldrefname);
-   strbuf_git_path(&sb_newref, "logs/%s", newrefname);
-   strbuf_git_path(&tmp_renamed_log, "logs/%s", TMP_RENAMED_LOG);
+   files_reflog_path(refs, &sb_oldref, oldrefname);
+   files_reflog_path(refs, &sb_newref, newrefname);
+   files_reflog_path(refs, &tmp_renamed_log, TMP_RENAMED_LOG);
 
log = !lstat(sb_oldref.buf, &loginfo);
if (log && S_ISLNK(loginfo.st_mode)) {
@@ -2671,7 +2686,7 @@ static int files_rename_ref(struct ref_store *ref_store,
}
}
 
-   if (log && rename_tmp_log(newrefname))
+   if (log && rename_tmp_log(refs, newrefname))
goto rollback;
 
logmoved = log;
@@ -2786,10 +2801,15 @@ static int open_or_create_logfile(const char *path, 
void *cb)
  * set *logfd to -1. On failure, fill in *err, set *logfd to -1, and
  * return -1.
  */
-static int log_ref_setup(const char *refname, int force_create,
+static int log_ref_setup(struct files_ref_store *refs,
+const char *refname, int force_create,
 int *logfd, struct strbuf *err)
 {
-   char *logfile = git_pathdup("logs/%s", refname);
+   struct strbuf logfile_sb = STRBUF_INIT;
+   char *logfile;
+
+   files_reflog_path(refs, &logfile_sb, refname);
+   logfile = strbuf_detach(&logfile_sb, NULL);
 
if (force_create || should_autocreate_reflog(refname)) {
if (raceproof_create_

[PATCH v7 13/28] refs: rename lookup_ref_store() to lookup_submodule_ref_store()

With get_main_ref_store() being used inside get_ref_store(),
lookup_ref_store() is only used for submodule code path. Rename to
reflect that and delete dead code.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 refs.c | 13 -
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/refs.c b/refs.c
index 8aa33af4e8..a4a1a4ccfd 100644
--- a/refs.c
+++ b/refs.c
@@ -1395,17 +1395,13 @@ static struct ref_store *main_ref_store;
 static struct hashmap submodule_ref_stores;
 
 /*
- * Return the ref_store instance for the specified submodule (or the
- * main repository if submodule is NULL). If that ref_store hasn't
- * been initialized yet, return NULL.
+ * Return the ref_store instance for the specified submodule. If that
+ * ref_store hasn't been initialized yet, return NULL.
  */
-static struct ref_store *lookup_ref_store(const char *submodule)
+static struct ref_store *lookup_submodule_ref_store(const char *submodule)
 {
struct submodule_hash_entry *entry;
 
-   if (!submodule)
-   return main_ref_store;
-
if (!submodule_ref_stores.tablesize)
/* It's initialized on demand in register_ref_store(). */
return NULL;
@@ -1471,7 +1467,7 @@ struct ref_store *get_ref_store(const char *submodule)
if (!submodule || !*submodule) {
return get_main_ref_store();
} else {
-   refs = lookup_ref_store(submodule);
+   refs = lookup_submodule_ref_store(submodule);
 
if (!refs) {
struct strbuf submodule_sb = STRBUF_INIT;
@@ -1482,7 +1478,6 @@ struct ref_store *get_ref_store(const char *submodule)
strbuf_release(&submodule_sb);
}
}
-
return refs;
 }
 
-- 
2.11.0.157.gd943d85

[PATCH v7 10/28] files-backend: add and use files_ref_path()

Keep repo-related path handling in one place. This will make it easier
to add submodule/multiworktree support later.

This automatically adds the "if submodule then use the submodule version
of git_path" to other call sites too. But it does not mean those
operations are submodule-ready. Not yet.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 refs/files-backend.c | 47 +++
 1 file changed, 23 insertions(+), 24 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index b0e0df9f66..741e52d143 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1180,6 +1180,18 @@ static void files_reflog_path(struct files_ref_store 
*refs,
strbuf_git_path(sb, "logs/%s", refname);
 }
 
+static void files_ref_path(struct files_ref_store *refs,
+  struct strbuf *sb,
+  const char *refname)
+{
+   if (refs->submodule) {
+   strbuf_git_path_submodule(sb, refs->submodule, "%s", refname);
+   return;
+   }
+
+   strbuf_git_path(sb, "%s", refname);
+}
+
 /*
  * Get the packed_ref_cache for the specified files_ref_store,
  * creating it if necessary.
@@ -1249,19 +1261,10 @@ static void read_loose_refs(const char *dirname, struct 
ref_dir *dir)
struct strbuf refname;
struct strbuf path = STRBUF_INIT;
size_t path_baselen;
-   int err = 0;
 
-   if (refs->submodule)
-   err = strbuf_git_path_submodule(&path, refs->submodule, "%s", 
dirname);
-   else
-   strbuf_git_path(&path, "%s", dirname);
+   files_ref_path(refs, &path, dirname);
path_baselen = path.len;
 
-   if (err) {
-   strbuf_release(&path);
-   return;
-   }
-
d = opendir(path.buf);
if (!d) {
strbuf_release(&path);
@@ -1396,10 +1399,7 @@ static int files_read_raw_ref(struct ref_store 
*ref_store,
*type = 0;
strbuf_reset(&sb_path);
 
-   if (refs->submodule)
-   strbuf_git_path_submodule(&sb_path, refs->submodule, "%s", 
refname);
-   else
-   strbuf_git_path(&sb_path, "%s", refname);
+   files_ref_path(refs, &sb_path, refname);
 
path = sb_path.buf;
 
@@ -1587,7 +1587,7 @@ static int lock_raw_ref(struct files_ref_store *refs,
*lock_p = lock = xcalloc(1, sizeof(*lock));
 
lock->ref_name = xstrdup(refname);
-   strbuf_git_path(&ref_file, "%s", refname);
+   files_ref_path(refs, &ref_file, refname);
 
 retry:
switch (safe_create_leading_directories(ref_file.buf)) {
@@ -2056,7 +2056,7 @@ static struct ref_lock *lock_ref_sha1_basic(struct 
files_ref_store *refs,
if (flags & REF_DELETING)
resolve_flags |= RESOLVE_REF_ALLOW_BAD_NAME;
 
-   strbuf_git_path(&ref_file, "%s", refname);
+   files_ref_path(refs, &ref_file, refname);
resolved = !!resolve_ref_unsafe(refname, resolve_flags,
lock->old_oid.hash, type);
if (!resolved && errno == EISDIR) {
@@ -2355,7 +2355,7 @@ static void try_remove_empty_parents(struct 
files_ref_store *refs,
strbuf_setlen(&buf, q - buf.buf);
 
strbuf_reset(&sb);
-   strbuf_git_path(&sb, "%s", buf.buf);
+   files_ref_path(refs, &sb, buf.buf);
if ((flags & REMOVE_EMPTY_PARENTS_REF) && rmdir(sb.buf))
flags &= ~REMOVE_EMPTY_PARENTS_REF;
 
@@ -2672,7 +2672,7 @@ static int files_rename_ref(struct ref_store *ref_store,
struct strbuf path = STRBUF_INIT;
int result;
 
-   strbuf_git_path(&path, "%s", newrefname);
+   files_ref_path(refs, &path, newrefname);
result = remove_empty_directories(&path);
strbuf_release(&path);
 
@@ -3906,7 +3906,7 @@ static int files_transaction_commit(struct ref_store 
*ref_store,
update->type & REF_ISSYMREF) {
/* It is a loose reference. */
strbuf_reset(&sb);
-   strbuf_git_path(&sb, "%s", lock->ref_name);
+   files_ref_path(refs, &sb, lock->ref_name);
if (unlink_or_msg(sb.buf, err)) {
ret = TRANSACTION_GENERIC_ERROR;
goto cleanup;
@@ -4206,19 +4206,18 @@ static int files_reflog_expire(struct ref_store 
*ref_store,
 
 static int files_init_db(struct ref_store *ref_store, struct strbuf *err)
 {
+   struct files_ref_store *refs =
+   files_downcast(ref_store, 0, "init_db");
struct strbuf sb = STRBUF_INIT;
 
-   /* Check validity (but we don't need the result): */
-   files_downcast(ref_store, 0, "init_db");
-
/*
 * Create .git/refs/{heads,tags}

[PATCH v7 14/28] refs.c: flatten get_ref_store() a bit

This helps the future changes in this code. And because get_ref_store()
is destined to become get_submodule_ref_store(), the "get main store"
code path will be removed eventually. After this the patch to delete
that code will be cleaner.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 refs.c | 23 +--
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/refs.c b/refs.c
index a4a1a4ccfd..66dc84787d 100644
--- a/refs.c
+++ b/refs.c
@@ -1462,22 +1462,25 @@ static struct ref_store *get_main_ref_store(void)
 
 struct ref_store *get_ref_store(const char *submodule)
 {
+   struct strbuf submodule_sb = STRBUF_INIT;
struct ref_store *refs;
+   int ret;
 
if (!submodule || !*submodule) {
return get_main_ref_store();
-   } else {
-   refs = lookup_submodule_ref_store(submodule);
+   }
 
-   if (!refs) {
-   struct strbuf submodule_sb = STRBUF_INIT;
+   refs = lookup_submodule_ref_store(submodule);
+   if (refs)
+   return refs;
 
-   strbuf_addstr(&submodule_sb, submodule);
-   if (is_nonbare_repository_dir(&submodule_sb))
-   refs = ref_store_init(submodule);
-   strbuf_release(&submodule_sb);
-   }
-   }
+   strbuf_addstr(&submodule_sb, submodule);
+   ret = is_nonbare_repository_dir(&submodule_sb);
+   strbuf_release(&submodule_sb);
+   if (!ret)
+   return NULL;
+
+   refs = ref_store_init(submodule);
return refs;
 }
 
-- 
2.11.0.157.gd943d85

[PATCH v7 17/28] path.c: move some code out of strbuf_git_path_submodule()

refs is learning to avoid path rewriting that is done by
strbuf_git_path_submodule(). Factor out this code so it could be reused
by refs_* functions.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 path.c  | 35 +++
 submodule.c | 31 +++
 submodule.h |  6 ++
 3 files changed, 44 insertions(+), 28 deletions(-)

diff --git a/path.c b/path.c
index efcedafba6..03e7e33b6e 100644
--- a/path.c
+++ b/path.c
@@ -471,39 +471,19 @@ const char *worktree_git_path(const struct worktree *wt, 
const char *fmt, ...)
 }
 
 /* Returns 0 on success, negative on failure. */
-#define SUBMODULE_PATH_ERR_NOT_CONFIGURED -1
 static int do_submodule_path(struct strbuf *buf, const char *path,
 const char *fmt, va_list args)
 {
-   const char *git_dir;
struct strbuf git_submodule_common_dir = STRBUF_INIT;
struct strbuf git_submodule_dir = STRBUF_INIT;
-   const struct submodule *sub;
-   int err = 0;
+   int ret;
 
-   strbuf_addstr(buf, path);
-   strbuf_complete(buf, '/');
-   strbuf_addstr(buf, ".git");
-
-   git_dir = read_gitfile(buf->buf);
-   if (git_dir) {
-   strbuf_reset(buf);
-   strbuf_addstr(buf, git_dir);
-   }
-   if (!is_git_directory(buf->buf)) {
-   gitmodules_config();
-   sub = submodule_from_path(null_sha1, path);
-   if (!sub) {
-   err = SUBMODULE_PATH_ERR_NOT_CONFIGURED;
-   goto cleanup;
-   }
-   strbuf_reset(buf);
-   strbuf_git_path(buf, "%s/%s", "modules", sub->name);
-   }
-
-   strbuf_addch(buf, '/');
-   strbuf_addbuf(&git_submodule_dir, buf);
+   ret = submodule_to_gitdir(&git_submodule_dir, path);
+   if (ret)
+   goto cleanup;
 
+   strbuf_complete(&git_submodule_dir, '/');
+   strbuf_addbuf(buf, &git_submodule_dir);
strbuf_vaddf(buf, fmt, args);
 
if (get_common_dir_noenv(&git_submodule_common_dir, 
git_submodule_dir.buf))
@@ -514,8 +494,7 @@ static int do_submodule_path(struct strbuf *buf, const char 
*path,
 cleanup:
strbuf_release(&git_submodule_dir);
strbuf_release(&git_submodule_common_dir);
-
-   return err;
+   return ret;
 }
 
 char *git_pathdup_submodule(const char *path, const char *fmt, ...)
diff --git a/submodule.c b/submodule.c
index 3200b7bb2b..a31f68812c 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1596,3 +1596,34 @@ const char *get_superproject_working_tree(void)
 
return ret;
 }
+
+int submodule_to_gitdir(struct strbuf *buf, const char *submodule)
+{
+   const struct submodule *sub;
+   const char *git_dir;
+   int ret = 0;
+
+   strbuf_reset(buf);
+   strbuf_addstr(buf, submodule);
+   strbuf_complete(buf, '/');
+   strbuf_addstr(buf, ".git");
+
+   git_dir = read_gitfile(buf->buf);
+   if (git_dir) {
+   strbuf_reset(buf);
+   strbuf_addstr(buf, git_dir);
+   }
+   if (!is_git_directory(buf->buf)) {
+   gitmodules_config();
+   sub = submodule_from_path(null_sha1, submodule);
+   if (!sub) {
+   ret = -1;
+   goto cleanup;
+   }
+   strbuf_reset(buf);
+   strbuf_git_path(buf, "%s/%s", "modules", sub->name);
+   }
+
+cleanup:
+   return ret;
+}
diff --git a/submodule.h b/submodule.h
index c8a0c9cb29..fce2fb64d2 100644
--- a/submodule.h
+++ b/submodule.h
@@ -81,6 +81,12 @@ extern int push_unpushed_submodules(struct sha1_array 
*commits,
int dry_run);
 extern void connect_work_tree_and_git_dir(const char *work_tree, const char 
*git_dir);
 extern int parallel_submodules(void);
+/*
+ * Given a submodule path (as in the index), return the repository
+ * path of that submodule in 'buf'. Return -1 on error or when the
+ * submodule is not initialized.
+ */
+int submodule_to_gitdir(struct strbuf *buf, const char *submodule);
 
 /*
  * Prepare the "env_array" parameter of a "struct child_process" for executing
-- 
2.11.0.157.gd943d85

[PATCH v7 07/28] files-backend: convert git_path() to strbuf_git_path()

git_path() and friends are going to be killed in files-backend.c in near
future. And because there's a risk with overwriting buffer in
git_path(), let's convert them all to strbuf_git_path(). We'll have
easier time killing/converting strbuf_git_path() then because we won't
have to worry about memory management again.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 refs/files-backend.c | 130 ++-
 1 file changed, 97 insertions(+), 33 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 6d0fcc88f9..5f9b0ab9bc 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2316,6 +2316,7 @@ enum {
 static void try_remove_empty_parents(const char *refname, unsigned int flags)
 {
struct strbuf buf = STRBUF_INIT;
+   struct strbuf sb = STRBUF_INIT;
char *p, *q;
int i;
 
@@ -2337,14 +2338,19 @@ static void try_remove_empty_parents(const char 
*refname, unsigned int flags)
if (q == p)
break;
strbuf_setlen(&buf, q - buf.buf);
-   if ((flags & REMOVE_EMPTY_PARENTS_REF) &&
-   rmdir(git_path("%s", buf.buf)))
+
+   strbuf_reset(&sb);
+   strbuf_git_path(&sb, "%s", buf.buf);
+   if ((flags & REMOVE_EMPTY_PARENTS_REF) && rmdir(sb.buf))
flags &= ~REMOVE_EMPTY_PARENTS_REF;
-   if ((flags & REMOVE_EMPTY_PARENTS_REFLOG) &&
-   rmdir(git_path("logs/%s", buf.buf)))
+
+   strbuf_reset(&sb);
+   strbuf_git_path(&sb, "logs/%s", buf.buf);
+   if ((flags & REMOVE_EMPTY_PARENTS_REFLOG) && rmdir(sb.buf))
flags &= ~REMOVE_EMPTY_PARENTS_REFLOG;
}
strbuf_release(&buf);
+   strbuf_release(&sb);
 }
 
 /* make sure nobody touched the ref, and unlink */
@@ -2506,11 +2512,16 @@ static int files_delete_refs(struct ref_store 
*ref_store,
  */
 #define TMP_RENAMED_LOG  "logs/refs/.tmp-renamed-log"
 
-static int rename_tmp_log_callback(const char *path, void *cb)
+struct rename_cb {
+   const char *tmp_renamed_log;
+   int true_errno;
+};
+
+static int rename_tmp_log_callback(const char *path, void *cb_data)
 {
-   int *true_errno = cb;
+   struct rename_cb *cb = cb_data;
 
-   if (rename(git_path(TMP_RENAMED_LOG), path)) {
+   if (rename(cb->tmp_renamed_log, path)) {
/*
 * rename(a, b) when b is an existing directory ought
 * to result in ISDIR, but Solaris 5.8 gives ENOTDIR.
@@ -2518,7 +2529,7 @@ static int rename_tmp_log_callback(const char *path, void 
*cb)
 * but report EISDIR to raceproof_create_file() so
 * that it knows to retry.
 */
-   *true_errno = errno;
+   cb->true_errno = errno;
if (errno == ENOTDIR)
errno = EISDIR;
return -1;
@@ -2529,20 +2540,26 @@ static int rename_tmp_log_callback(const char *path, 
void *cb)
 
 static int rename_tmp_log(const char *newrefname)
 {
-   char *path = git_pathdup("logs/%s", newrefname);
-   int ret, true_errno;
+   struct strbuf path = STRBUF_INIT;
+   struct strbuf tmp = STRBUF_INIT;
+   struct rename_cb cb;
+   int ret;
 
-   ret = raceproof_create_file(path, rename_tmp_log_callback, &true_errno);
+   strbuf_git_path(&path, "logs/%s", newrefname);
+   strbuf_git_path(&tmp, TMP_RENAMED_LOG);
+   cb.tmp_renamed_log = tmp.buf;
+   ret = raceproof_create_file(path.buf, rename_tmp_log_callback, &cb);
if (ret) {
if (errno == EISDIR)
-   error("directory not empty: %s", path);
+   error("directory not empty: %s", path.buf);
else
error("unable to move logfile %s to %s: %s",
- git_path(TMP_RENAMED_LOG), path,
- strerror(true_errno));
+ tmp.buf, path.buf,
+ strerror(cb.true_errno));
}
 
-   free(path);
+   strbuf_release(&path);
+   strbuf_release(&tmp);
return ret;
 }
 
@@ -2583,10 +2600,17 @@ static int files_rename_ref(struct ref_store *ref_store,
int flag = 0, logmoved = 0;
struct ref_lock *lock;
struct stat loginfo;
-   int log = !lstat(git_path("logs/%s", oldrefname), &loginfo);
+   struct strbuf sb_oldref = STRBUF_INIT;
+   struct strbuf sb_newref = STRBUF_INIT;
+   struct strbuf tmp_renamed_log = STRBUF_INIT;
+   int log, ret;
struct strbuf err = STRBUF_INIT;
-   int ret;
 
+   strbuf_git_path(&sb_oldref, "logs/%s", oldrefname);
+   strbuf_git_path(&sb_newref, "logs/%s", newrefname);
+   strbuf_git_path(&tmp_renamed_log, TMP_RENAMED_LOG);
+
+   log = !lstat(sb_oldref.buf, &loginfo);
if (log && S_ISL

[PATCH v7 16/28] refs.c: make get_main_ref_store() public and use it

get_ref_store() will soon be renamed to get_submodule_ref_store().
Together with future get_worktree_ref_store(), the three functions
provide an appropriate ref store for different operation modes. New APIs
will be added to operate directly on ref stores.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 refs.c   | 36 ++--
 refs.h   |  3 +++
 refs/files-backend.c |  2 +-
 3 files changed, 22 insertions(+), 19 deletions(-)

diff --git a/refs.c b/refs.c
index 87f64271ac..1f4c1a2347 100644
--- a/refs.c
+++ b/refs.c
@@ -1314,7 +1314,7 @@ const char *resolve_ref_recursively(struct ref_store 
*refs,
 /* backend functions */
 int refs_init_db(struct strbuf *err)
 {
-   struct ref_store *refs = get_ref_store(NULL);
+   struct ref_store *refs = get_main_ref_store();
 
return refs->be->init_db(refs, err);
 }
@@ -1322,7 +1322,7 @@ int refs_init_db(struct strbuf *err)
 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,
+   return resolve_ref_recursively(get_main_ref_store(), refname,
   resolve_flags, sha1, flags);
 }
 
@@ -1428,7 +1428,7 @@ static struct ref_store *ref_store_init(const char 
*submodule)
return refs;
 }
 
-static struct ref_store *get_main_ref_store(void)
+struct ref_store *get_main_ref_store(void)
 {
if (main_ref_store)
return main_ref_store;
@@ -1488,14 +1488,14 @@ void base_ref_store_init(struct ref_store *refs,
 /* backend functions */
 int pack_refs(unsigned int flags)
 {
-   struct ref_store *refs = get_ref_store(NULL);
+   struct ref_store *refs = get_main_ref_store();
 
return refs->be->pack_refs(refs, flags);
 }
 
 int peel_ref(const char *refname, unsigned char *sha1)
 {
-   struct ref_store *refs = get_ref_store(NULL);
+   struct ref_store *refs = get_main_ref_store();
 
return refs->be->peel_ref(refs, refname, sha1);
 }
@@ -1503,7 +1503,7 @@ int peel_ref(const char *refname, unsigned char *sha1)
 int create_symref(const char *ref_target, const char *refs_heads_master,
  const char *logmsg)
 {
-   struct ref_store *refs = get_ref_store(NULL);
+   struct ref_store *refs = get_main_ref_store();
 
return refs->be->create_symref(refs, ref_target, refs_heads_master,
   logmsg);
@@ -1512,7 +1512,7 @@ int create_symref(const char *ref_target, const char 
*refs_heads_master,
 int ref_transaction_commit(struct ref_transaction *transaction,
   struct strbuf *err)
 {
-   struct ref_store *refs = get_ref_store(NULL);
+   struct ref_store *refs = get_main_ref_store();
 
return refs->be->transaction_commit(refs, transaction, err);
 }
@@ -1522,14 +1522,14 @@ int verify_refname_available(const char *refname,
 const struct string_list *skip,
 struct strbuf *err)
 {
-   struct ref_store *refs = get_ref_store(NULL);
+   struct ref_store *refs = get_main_ref_store();
 
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_store *refs = get_main_ref_store();
struct ref_iterator *iter;
 
iter = refs->be->reflog_iterator_begin(refs);
@@ -1540,7 +1540,7 @@ int for_each_reflog(each_ref_fn fn, void *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);
+   struct ref_store *refs = get_main_ref_store();
 
return refs->be->for_each_reflog_ent_reverse(refs, refname,
 fn, cb_data);
@@ -1549,14 +1549,14 @@ int for_each_reflog_ent_reverse(const char *refname, 
each_reflog_ent_fn fn,
 int for_each_reflog_ent(const char *refname, each_reflog_ent_fn fn,
void *cb_data)
 {
-   struct ref_store *refs = get_ref_store(NULL);
+   struct ref_store *refs = get_main_ref_store();
 
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);
+   struct ref_store *refs = get_main_ref_store();
 
return refs->be->reflog_exists(refs, refname);
 }
@@ -1564,14 +1564,14 @@ int reflog_exists(const char *refname)
 int safe_create_reflog(const char *refname, int force_create,
   struct strbuf *err)
 {
-   struct ref_store *refs = get_ref_store(NULL);
+   struct ref_store *refs = get_main_ref_store();
 
return refs->be->create_reflog(refs, refname, force_create, err);
 }
 
 int delete_reflog(co

[PATCH v7 05/28] files-backend: add and use files_packed_refs_path()

Keep repo-related path handling in one place. This will make it easier
to add submodule/multiworktree support later.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 refs/files-backend.c | 29 ++---
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 9a9dc4e50c..f17daddffa 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -923,6 +923,8 @@ struct files_ref_store {
 */
const char *submodule;
 
+   char *packed_refs_path;
+
struct ref_entry *loose;
struct packed_ref_cache *packed;
 };
@@ -985,7 +987,14 @@ static struct ref_store *files_ref_store_create(const char 
*submodule)
 
base_ref_store_init(ref_store, &refs_be_files);
 
-   refs->submodule = xstrdup_or_null(submodule);
+   if (submodule) {
+   refs->submodule = xstrdup(submodule);
+   refs->packed_refs_path = git_pathdup_submodule(
+   refs->submodule, "packed-refs");
+   return ref_store;
+   }
+
+   refs->packed_refs_path = git_pathdup("packed-refs");
 
return ref_store;
 }
@@ -1153,19 +1162,18 @@ static void read_packed_refs(FILE *f, struct ref_dir 
*dir)
strbuf_release(&line);
 }
 
+static const char *files_packed_refs_path(struct files_ref_store *refs)
+{
+   return refs->packed_refs_path;
+}
+
 /*
  * Get the packed_ref_cache for the specified files_ref_store,
  * creating it if necessary.
  */
 static struct packed_ref_cache *get_packed_ref_cache(struct files_ref_store 
*refs)
 {
-   char *packed_refs_file;
-
-   if (refs->submodule)
-   packed_refs_file = git_pathdup_submodule(refs->submodule,
-"packed-refs");
-   else
-   packed_refs_file = git_pathdup("packed-refs");
+   const char *packed_refs_file = files_packed_refs_path(refs);
 
if (refs->packed &&
!stat_validity_check(&refs->packed->validity, packed_refs_file))
@@ -1184,7 +1192,6 @@ static struct packed_ref_cache 
*get_packed_ref_cache(struct files_ref_store *ref
fclose(f);
}
}
-   free(packed_refs_file);
return refs->packed;
 }
 
@@ -2157,7 +2164,7 @@ static int lock_packed_refs(struct files_ref_store *refs, 
int flags)
}
 
if (hold_lock_file_for_update_timeout(
-   &packlock, git_path("packed-refs"),
+   &packlock, files_packed_refs_path(refs),
flags, timeout_value) < 0)
return -1;
/*
@@ -2423,7 +2430,7 @@ static int repack_without_refs(struct files_ref_store 
*refs,
return 0; /* no refname exists in packed refs */
 
if (lock_packed_refs(refs, 0)) {
-   unable_to_lock_message(git_path("packed-refs"), errno, err);
+   unable_to_lock_message(files_packed_refs_path(refs), errno, 
err);
return -1;
}
packed = get_packed_refs(refs);
-- 
2.11.0.157.gd943d85

[PATCH v7 12/28] refs.c: introduce get_main_ref_store()

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 refs.c | 13 +
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/refs.c b/refs.c
index e7606716dd..8aa33af4e8 100644
--- a/refs.c
+++ b/refs.c
@@ -1456,15 +1456,20 @@ static struct ref_store *ref_store_init(const char 
*submodule)
return refs;
 }
 
+static struct ref_store *get_main_ref_store(void)
+{
+   if (main_ref_store)
+   return main_ref_store;
+
+   return ref_store_init(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);
+   return get_main_ref_store();
} else {
refs = lookup_ref_store(submodule);
 
-- 
2.11.0.157.gd943d85

[PATCH v7 03/28] files-backend.c: delete dead code in files_ref_iterator_begin()

It's not in the diff context, but files_downcast() is called before this
check. If "refs" is NULL, we would have segfaulted before reaching the
check here. And we should never see NULL refs in backend code (frontend
should have caught it).

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 refs/files-backend.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 42c0cc14f3..caeb8188fd 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1897,9 +1897,6 @@ static struct ref_iterator *files_ref_iterator_begin(
struct files_ref_iterator *iter;
struct ref_iterator *ref_iterator;
 
-   if (!refs)
-   return empty_ref_iterator_begin();
-
if (ref_paranoia < 0)
ref_paranoia = git_env_bool("GIT_REF_PARANOIA", 0);
if (ref_paranoia)
-- 
2.11.0.157.gd943d85

[PATCH v7 08/28] files-backend: move "logs/" out of TMP_RENAMED_LOG

This makes reflog path building consistent, always in the form of

strbuf_git_path(sb, "logs/%s", refname);

It reduces the mental workload a bit in the next patch when that
function call is converted.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 refs/files-backend.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 5f9b0ab9bc..3e0bafcaf9 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2510,7 +2510,7 @@ static int files_delete_refs(struct ref_store *ref_store,
  * IOW, to avoid cross device rename errors, the temporary renamed log must
  * live into logs/refs.
  */
-#define TMP_RENAMED_LOG  "logs/refs/.tmp-renamed-log"
+#define TMP_RENAMED_LOG  "refs/.tmp-renamed-log"
 
 struct rename_cb {
const char *tmp_renamed_log;
@@ -2546,7 +2546,7 @@ static int rename_tmp_log(const char *newrefname)
int ret;
 
strbuf_git_path(&path, "logs/%s", newrefname);
-   strbuf_git_path(&tmp, TMP_RENAMED_LOG);
+   strbuf_git_path(&tmp, "logs/%s", TMP_RENAMED_LOG);
cb.tmp_renamed_log = tmp.buf;
ret = raceproof_create_file(path.buf, rename_tmp_log_callback, &cb);
if (ret) {
@@ -2608,7 +2608,7 @@ static int files_rename_ref(struct ref_store *ref_store,
 
strbuf_git_path(&sb_oldref, "logs/%s", oldrefname);
strbuf_git_path(&sb_newref, "logs/%s", newrefname);
-   strbuf_git_path(&tmp_renamed_log, TMP_RENAMED_LOG);
+   strbuf_git_path(&tmp_renamed_log, "logs/%s", TMP_RENAMED_LOG);
 
log = !lstat(sb_oldref.buf, &loginfo);
if (log && S_ISLNK(loginfo.st_mode)) {
@@ -2633,7 +2633,7 @@ static int files_rename_ref(struct ref_store *ref_store,
}
 
if (log && rename(sb_oldref.buf, tmp_renamed_log.buf)) {
-   ret = error("unable to move logfile logs/%s to 
"TMP_RENAMED_LOG": %s",
+   ret = error("unable to move logfile logs/%s to 
logs/"TMP_RENAMED_LOG": %s",
oldrefname, strerror(errno));
goto out;
}
@@ -2719,7 +2719,7 @@ static int files_rename_ref(struct ref_store *ref_store,
oldrefname, newrefname, strerror(errno));
if (!logmoved && log &&
rename(tmp_renamed_log.buf, sb_oldref.buf))
-   error("unable to restore logfile %s from "TMP_RENAMED_LOG": %s",
+   error("unable to restore logfile %s from 
logs/"TMP_RENAMED_LOG": %s",
oldrefname, strerror(errno));
ret = 1;
  out:
-- 
2.11.0.157.gd943d85

[PATCH v7 18/28] refs: move submodule code out of files-backend.c

files-backend is now initialized with a $GIT_DIR. Converting a submodule
path to where real submodule gitdir is located is done in get_ref_store().

This gives a slight performance improvement for submodules since we
don't convert submodule path to gitdir at every backend call like
before. We pay that once at ref-store creation.

More cleanup in files_downcast() and files_assert_main_repository()
follows shortly. It's separate to keep noises from this patch.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 refs.c   | 19 ++-
 refs/files-backend.c | 24 ++--
 refs/refs-internal.h |  9 -
 3 files changed, 20 insertions(+), 32 deletions(-)

diff --git a/refs.c b/refs.c
index 1f4c1a2347..d72b48a430 100644
--- a/refs.c
+++ b/refs.c
@@ -9,6 +9,7 @@
 #include "refs/refs-internal.h"
 #include "object.h"
 #include "tag.h"
+#include "submodule.h"
 
 /*
  * List of all available backends
@@ -1413,9 +1414,9 @@ static struct ref_store *lookup_submodule_ref_store(const 
char *submodule)
 
 /*
  * Create, record, and return a ref_store instance for the specified
- * submodule (or the main repository if submodule is NULL).
+ * gitdir.
  */
-static struct ref_store *ref_store_init(const char *submodule)
+static struct ref_store *ref_store_init(const char *gitdir)
 {
const char *be_name = "files";
struct ref_storage_be *be = find_ref_storage_backend(be_name);
@@ -1424,7 +1425,7 @@ static struct ref_store *ref_store_init(const char 
*submodule)
if (!be)
die("BUG: reference backend %s is unknown", be_name);
 
-   refs = be->init(submodule);
+   refs = be->init(gitdir);
return refs;
 }
 
@@ -1433,7 +1434,7 @@ struct ref_store *get_main_ref_store(void)
if (main_ref_store)
return main_ref_store;
 
-   main_ref_store = ref_store_init(NULL);
+   main_ref_store = ref_store_init(get_git_dir());
return main_ref_store;
 }
 
@@ -1474,8 +1475,16 @@ struct ref_store *get_ref_store(const char *submodule)
if (!ret)
return NULL;
 
-   refs = ref_store_init(submodule);
+   ret = submodule_to_gitdir(&submodule_sb, submodule);
+   if (ret) {
+   strbuf_release(&submodule_sb);
+   return NULL;
+   }
+
+   refs = ref_store_init(submodule_sb.buf);
register_submodule_ref_store(refs, submodule);
+
+   strbuf_release(&submodule_sb);
return refs;
 }
 
diff --git a/refs/files-backend.c b/refs/files-backend.c
index b62f374f9c..490f05a6f4 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -917,12 +917,6 @@ struct packed_ref_cache {
 struct files_ref_store {
struct ref_store base;
 
-   /*
-* The name of the submodule represented by this object, or
-* NULL if it represents the main repository's reference
-* store:
-*/
-   const char *submodule;
char *gitdir;
char *gitcommondir;
char *packed_refs_path;
@@ -982,22 +976,14 @@ 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 ref_store *files_ref_store_create(const char *submodule)
+static struct ref_store *files_ref_store_create(const char *gitdir)
 {
struct files_ref_store *refs = xcalloc(1, sizeof(*refs));
struct ref_store *ref_store = (struct ref_store *)refs;
struct strbuf sb = STRBUF_INIT;
-   const char *gitdir = get_git_dir();
 
base_ref_store_init(ref_store, &refs_be_files);
 
-   if (submodule) {
-   refs->submodule = xstrdup(submodule);
-   refs->packed_refs_path = git_pathdup_submodule(
-   refs->submodule, "packed-refs");
-   return ref_store;
-   }
-
refs->gitdir = xstrdup(gitdir);
get_common_dir_noenv(&sb, gitdir);
refs->gitcommondir = strbuf_detach(&sb, NULL);
@@ -1014,8 +1000,7 @@ static struct ref_store *files_ref_store_create(const 
char *submodule)
 static void files_assert_main_repository(struct files_ref_store *refs,
 const char *caller)
 {
-   if (refs->submodule)
-   die("BUG: %s called for a submodule", caller);
+   /* This function is to be fixed up in the next patch */
 }
 
 /*
@@ -1206,11 +1191,6 @@ static void files_ref_path(struct files_ref_store *refs,
   struct strbuf *sb,
   const char *refname)
 {
-   if (refs->submodule) {
-   strbuf_git_path_submodule(sb, refs->submodule, "%s", refname);
-   return;
-   }
-
switch (ref_type(refname)) {
case REF_TYPE_PER_WORKTREE:
case REF_TYPE_PSEUDOREF:
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index f732473e1d..dfa1817929 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -482,12 +482,11 @@ struct ref

[PATCH v7 22/28] refs: new transaction related ref-store api

The transaction struct now takes a ref store at creation and will
operate on that ref store alone.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 refs.c   | 55 
 refs.h   |  9 +
 refs/refs-internal.h |  1 +
 3 files changed, 53 insertions(+), 12 deletions(-)

diff --git a/refs.c b/refs.c
index c103f90b35..50ea24bd75 100644
--- a/refs.c
+++ b/refs.c
@@ -630,16 +630,20 @@ static int delete_pseudoref(const char *pseudoref, const 
unsigned char *old_sha1
return 0;
 }
 
-int delete_ref(const char *msg, const char *refname,
-  const unsigned char *old_sha1, unsigned int flags)
+int refs_delete_ref(struct ref_store *refs, const char *msg,
+   const char *refname,
+   const unsigned char *old_sha1,
+   unsigned int flags)
 {
struct ref_transaction *transaction;
struct strbuf err = STRBUF_INIT;
 
-   if (ref_type(refname) == REF_TYPE_PSEUDOREF)
+   if (ref_type(refname) == REF_TYPE_PSEUDOREF) {
+   assert(refs == get_main_ref_store());
return delete_pseudoref(refname, old_sha1);
+   }
 
-   transaction = ref_transaction_begin(&err);
+   transaction = ref_store_transaction_begin(refs, &err);
if (!transaction ||
ref_transaction_delete(transaction, refname, old_sha1,
   flags, msg, &err) ||
@@ -654,6 +658,13 @@ int delete_ref(const char *msg, const char *refname,
return 0;
 }
 
+int delete_ref(const char *msg, const char *refname,
+  const unsigned char *old_sha1, unsigned int flags)
+{
+   return refs_delete_ref(get_main_ref_store(), msg, refname,
+  old_sha1, flags);
+}
+
 int copy_reflog_msg(char *buf, const char *msg)
 {
char *cp = buf;
@@ -813,11 +824,20 @@ int read_ref_at(const char *refname, unsigned int flags, 
unsigned long at_time,
return 1;
 }
 
-struct ref_transaction *ref_transaction_begin(struct strbuf *err)
+struct ref_transaction *ref_store_transaction_begin(struct ref_store *refs,
+   struct strbuf *err)
 {
+   struct ref_transaction *tr;
assert(err);
 
-   return xcalloc(1, sizeof(struct ref_transaction));
+   tr = xcalloc(1, sizeof(struct ref_transaction));
+   tr->ref_store = refs;
+   return tr;
+}
+
+struct ref_transaction *ref_transaction_begin(struct strbuf *err)
+{
+   return ref_store_transaction_begin(get_main_ref_store(), err);
 }
 
 void ref_transaction_free(struct ref_transaction *transaction)
@@ -934,18 +954,20 @@ int update_ref_oid(const char *msg, const char *refname,
old_oid ? old_oid->hash : NULL, flags, onerr);
 }
 
-int update_ref(const char *msg, const char *refname,
-  const unsigned char *new_sha1, const unsigned char *old_sha1,
-  unsigned int flags, enum action_on_err onerr)
+int refs_update_ref(struct ref_store *refs, const char *msg,
+   const char *refname, const unsigned char *new_sha1,
+   const unsigned char *old_sha1, unsigned int flags,
+   enum action_on_err onerr)
 {
struct ref_transaction *t = NULL;
struct strbuf err = STRBUF_INIT;
int ret = 0;
 
if (ref_type(refname) == REF_TYPE_PSEUDOREF) {
+   assert(refs == get_main_ref_store());
ret = write_pseudoref(refname, new_sha1, old_sha1, &err);
} else {
-   t = ref_transaction_begin(&err);
+   t = ref_store_transaction_begin(refs, &err);
if (!t ||
ref_transaction_update(t, refname, new_sha1, old_sha1,
   flags, msg, &err) ||
@@ -976,6 +998,15 @@ int update_ref(const char *msg, const char *refname,
return 0;
 }
 
+int update_ref(const char *msg, const char *refname,
+  const unsigned char *new_sha1,
+  const unsigned char *old_sha1,
+  unsigned int flags, enum action_on_err onerr)
+{
+   return refs_update_ref(get_main_ref_store(), msg, refname, new_sha1,
+  old_sha1, flags, onerr);
+}
+
 char *shorten_unambiguous_ref(const char *refname, int strict)
 {
int i;
@@ -1607,7 +1638,7 @@ int create_symref(const char *ref_target, const char 
*refs_heads_master,
 int ref_transaction_commit(struct ref_transaction *transaction,
   struct strbuf *err)
 {
-   struct ref_store *refs = get_main_ref_store();
+   struct ref_store *refs = transaction->ref_store;
 
return refs->be->transaction_commit(refs, transaction, err);
 }
@@ -1726,7 +1757,7 @@ int reflog_expire(const char *refname, const unsigned 
char *sha1,
 int initial_ref_transaction_commit(struct ref_transaction *transaction,
   struct strbuf *err)
 {
-   struct ref

[PATCH v7 19/28] files-backend: replace submodule_allowed check in files_downcast()

files-backend.c is unlearning submodules. Instead of having a specific
check for submodules to see what operation is allowed, files backend
now takes a set of flags at init. Each operation will check if the
required flags is present before performing.

For now we have four flags: read, write and odb access. Main ref store
has all flags, obviously, while submodule stores are read-only and have
access to odb (*).

The "main" flag stays because many functions in the backend calls
frontend ones without a ref store, so these functions always target the
main ref store. Ideally the flag should be gone after ref-store-aware
api is in place and used by backends.

(*) Submodule code needs for_each_ref. Try take REF_STORE_ODB flag
out. At least t3404 would fail. The "have access to odb" in submodule is
a bit hacky since we don't know from he whether add_submodule_odb() has
been called.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 refs.c   | 15 +++---
 refs/files-backend.c | 81 +---
 refs/refs-internal.h |  9 +-
 3 files changed, 71 insertions(+), 34 deletions(-)

diff --git a/refs.c b/refs.c
index d72b48a430..241b4227b2 100644
--- a/refs.c
+++ b/refs.c
@@ -1416,7 +1416,8 @@ static struct ref_store *lookup_submodule_ref_store(const 
char *submodule)
  * Create, record, and return a ref_store instance for the specified
  * gitdir.
  */
-static struct ref_store *ref_store_init(const char *gitdir)
+static struct ref_store *ref_store_init(const char *gitdir,
+   unsigned int flags)
 {
const char *be_name = "files";
struct ref_storage_be *be = find_ref_storage_backend(be_name);
@@ -1425,7 +1426,7 @@ static struct ref_store *ref_store_init(const char 
*gitdir)
if (!be)
die("BUG: reference backend %s is unknown", be_name);
 
-   refs = be->init(gitdir);
+   refs = be->init(gitdir, flags);
return refs;
 }
 
@@ -1434,7 +1435,11 @@ struct ref_store *get_main_ref_store(void)
if (main_ref_store)
return main_ref_store;
 
-   main_ref_store = ref_store_init(get_git_dir());
+   main_ref_store = ref_store_init(get_git_dir(),
+   (REF_STORE_READ |
+REF_STORE_WRITE |
+REF_STORE_ODB |
+REF_STORE_MAIN));
return main_ref_store;
 }
 
@@ -1481,7 +1486,9 @@ struct ref_store *get_ref_store(const char *submodule)
return NULL;
}
 
-   refs = ref_store_init(submodule_sb.buf);
+   /* pretend that add_submodule_odb() has been called */
+   refs = ref_store_init(submodule_sb.buf,
+ REF_STORE_READ | REF_STORE_ODB);
register_submodule_ref_store(refs, submodule);
 
strbuf_release(&submodule_sb);
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 490f05a6f4..d97a924860 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -916,6 +916,7 @@ struct packed_ref_cache {
  */
 struct files_ref_store {
struct ref_store base;
+   unsigned int store_flags;
 
char *gitdir;
char *gitcommondir;
@@ -976,13 +977,15 @@ 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 ref_store *files_ref_store_create(const char *gitdir)
+static struct ref_store *files_ref_store_create(const char *gitdir,
+   unsigned int flags)
 {
struct files_ref_store *refs = xcalloc(1, sizeof(*refs));
struct ref_store *ref_store = (struct ref_store *)refs;
struct strbuf sb = STRBUF_INIT;
 
base_ref_store_init(ref_store, &refs_be_files);
+   refs->store_flags = flags;
 
refs->gitdir = xstrdup(gitdir);
get_common_dir_noenv(&sb, gitdir);
@@ -994,13 +997,17 @@ static struct ref_store *files_ref_store_create(const 
char *gitdir)
 }
 
 /*
- * Die if refs is for a submodule (i.e., not for the main repository).
- * caller is used in any necessary error messages.
+ * Die if refs is not the main ref store. caller is used in any
+ * necessary error messages.
  */
 static void files_assert_main_repository(struct files_ref_store *refs,
 const char *caller)
 {
-   /* This function is to be fixed up in the next patch */
+   if (refs->store_flags & REF_STORE_MAIN)
+   return;
+
+   die("BUG: unallowed operation (%s), only works "
+   "on main ref store\n", caller);
 }
 
 /*
@@ -1009,9 +1016,9 @@ static void files_assert_main_repository(struct 
files_ref_store *refs,
  * files_ref_store is for a submodule (i.e., not for the main
  * repository). caller is used in any necessary error messages.
  */
-static struct files_ref_store *files_downcast(
-

[PATCH v7 20/28] refs: rename get_ref_store() to get_submodule_ref_store() and make it public

This function is intended to replace *_submodule() refs API. It provides
a ref store for a specific submodule, which can be operated on by a new
set of refs API.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 refs.c   | 12 
 refs.h   | 11 +++
 refs/refs-internal.h | 12 
 3 files changed, 19 insertions(+), 16 deletions(-)

diff --git a/refs.c b/refs.c
index 241b4227b2..100a925bcc 100644
--- a/refs.c
+++ b/refs.c
@@ -1171,7 +1171,7 @@ 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_store *refs = get_submodule_ref_store(submodule);
struct ref_iterator *iter;
 
if (!refs)
@@ -1344,10 +1344,10 @@ int resolve_gitlink_ref(const char *submodule, const 
char *refname,
/* We need to strip off one or more trailing slashes */
char *stripped = xmemdupz(submodule, len);
 
-   refs = get_ref_store(stripped);
+   refs = get_submodule_ref_store(stripped);
free(stripped);
} else {
-   refs = get_ref_store(submodule);
+   refs = get_submodule_ref_store(submodule);
}
 
if (!refs)
@@ -1460,13 +1460,17 @@ static void register_submodule_ref_store(struct 
ref_store *refs,
submodule);
 }
 
-struct ref_store *get_ref_store(const char *submodule)
+struct ref_store *get_submodule_ref_store(const char *submodule)
 {
struct strbuf submodule_sb = STRBUF_INIT;
struct ref_store *refs;
int ret;
 
if (!submodule || !*submodule) {
+   /*
+* FIXME: This case is ideally not allowed. But that
+* can't happen until we clean up all the callers.
+*/
return get_main_ref_store();
}
 
diff --git a/refs.h b/refs.h
index a6cd12267f..e6d8f67895 100644
--- a/refs.h
+++ b/refs.h
@@ -562,5 +562,16 @@ int reflog_expire(const char *refname, const unsigned char 
*sha1,
 int ref_storage_backend_exists(const char *name);
 
 struct ref_store *get_main_ref_store(void);
+/*
+ * Return the ref_store instance 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. If the requested reference store has not yet
+ * been initialized, initialize it first.
+ *
+ * For backwards compatibility, submodule=="" is treated the same as
+ * submodule==NULL.
+ */
+struct ref_store *get_submodule_ref_store(const char *submodule);
 
 #endif /* REFS_H */
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 0cca280b5c..f20dde39ee 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -646,18 +646,6 @@ struct ref_store {
 void base_ref_store_init(struct ref_store *refs,
 const struct ref_storage_be *be);
 
-/*
- * Return the ref_store instance 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. If the requested reference store has not yet
- * been initialized, initialize it first.
- *
- * For backwards compatibility, submodule=="" is treated the same as
- * submodule==NULL.
- */
-struct ref_store *get_ref_store(const char *submodule);
-
 const char *resolve_ref_recursively(struct ref_store *refs,
const char *refname,
int resolve_flags,
-- 
2.11.0.157.gd943d85

[PATCH v7 24/28] refs: delete pack_refs() in favor of refs_pack_refs()

It only has one caller, not worth keeping just for convenience.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 builtin/pack-refs.c | 2 +-
 refs.c  | 5 -
 refs.h  | 1 -
 3 files changed, 1 insertion(+), 7 deletions(-)

diff --git a/builtin/pack-refs.c b/builtin/pack-refs.c
index 39f9a55d16..b106a392a4 100644
--- a/builtin/pack-refs.c
+++ b/builtin/pack-refs.c
@@ -17,5 +17,5 @@ int cmd_pack_refs(int argc, const char **argv, const char 
*prefix)
};
if (parse_options(argc, argv, prefix, opts, pack_refs_usage, 0))
usage_with_options(pack_refs_usage, opts);
-   return pack_refs(flags);
+   return refs_pack_refs(get_main_ref_store(), flags);
 }
diff --git a/refs.c b/refs.c
index 50ea24bd75..ec1f563824 100644
--- a/refs.c
+++ b/refs.c
@@ -1602,11 +1602,6 @@ int refs_pack_refs(struct ref_store *refs, unsigned int 
flags)
return refs->be->pack_refs(refs, flags);
 }
 
-int pack_refs(unsigned int flags)
-{
-   return refs_pack_refs(get_main_ref_store(), flags);
-}
-
 int refs_peel_ref(struct ref_store *refs, const char *refname,
  unsigned char *sha1)
 {
diff --git a/refs.h b/refs.h
index 37f4aa8bd5..1a07f9d86f 100644
--- a/refs.h
+++ b/refs.h
@@ -297,7 +297,6 @@ void warn_dangling_symrefs(FILE *fp, const char *msg_fmt,
  * flags: Combination of the above PACK_REFS_* flags.
  */
 int refs_pack_refs(struct ref_store *refs, unsigned int flags);
-int pack_refs(unsigned int flags);
 
 /*
  * Flags controlling ref_transaction_update(), ref_transaction_create(), etc.
-- 
2.11.0.157.gd943d85

[PATCH v7 26/28] t1405: some basic tests on main ref store

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 t/t1405-main-ref-store.sh (new +x) | 123 +
 1 file changed, 123 insertions(+)
 create mode 100755 t/t1405-main-ref-store.sh

diff --git a/t/t1405-main-ref-store.sh b/t/t1405-main-ref-store.sh
new file mode 100755
index 00..77e1c130c2
--- /dev/null
+++ b/t/t1405-main-ref-store.sh
@@ -0,0 +1,123 @@
+#!/bin/sh
+
+test_description='test main ref store api'
+
+. ./test-lib.sh
+
+RUN="test-ref-store main"
+
+test_expect_success 'pack_refs(PACK_REFS_ALL | PACK_REFS_PRUNE)' '
+   test_commit one &&
+   N=`find .git/refs -type f | wc -l` &&
+   test "$N" != 0 &&
+   $RUN pack-refs 3 &&
+   N=`find .git/refs -type f | wc -l`
+'
+
+test_expect_success 'peel_ref(new-tag)' '
+   git rev-parse HEAD >expected &&
+   git tag -a -m new-tag new-tag HEAD &&
+   $RUN peel-ref refs/tags/new-tag >actual &&
+   test_cmp expected actual
+'
+
+test_expect_success 'create_symref(FOO, refs/heads/master)' '
+   $RUN create-symref FOO refs/heads/master nothing &&
+   echo refs/heads/master >expected &&
+   git symbolic-ref FOO >actual &&
+   test_cmp expected actual
+'
+
+test_expect_success 'delete_refs(FOO, refs/tags/new-tag)' '
+   git rev-parse FOO -- &&
+   git rev-parse refs/tags/new-tag -- &&
+   $RUN delete-refs 0 FOO refs/tags/new-tag &&
+   test_must_fail git rev-parse FOO -- &&
+   test_must_fail git rev-parse refs/tags/new-tag --
+'
+
+test_expect_success 'rename_refs(master, new-master)' '
+   git rev-parse master >expected &&
+   $RUN rename-ref refs/heads/master refs/heads/new-master &&
+   git rev-parse new-master >actual &&
+   test_cmp expected actual &&
+   test_commit recreate-master
+'
+
+test_expect_success 'for_each_ref(refs/heads/)' '
+   $RUN for-each-ref refs/heads/ | cut -c 42- >actual &&
+   cat >expected <<-\EOF &&
+   master 0x0
+   new-master 0x0
+   EOF
+   test_cmp expected actual
+'
+
+test_expect_success 'resolve_ref(new-master)' '
+   SHA1=`git rev-parse new-master` &&
+   echo "$SHA1 refs/heads/new-master 0x0" >expected &&
+   $RUN resolve-ref refs/heads/new-master 0 >actual &&
+   test_cmp expected actual
+'
+
+test_expect_success 'verify_ref(new-master)' '
+   $RUN verify-ref refs/heads/new-master
+'
+
+test_expect_success 'for_each_reflog()' '
+   $RUN for-each-reflog | sort | cut -c 42- >actual &&
+   cat >expected <<-\EOF &&
+   HEAD 0x1
+   refs/heads/master 0x0
+   refs/heads/new-master 0x0
+   EOF
+   test_cmp expected actual
+'
+
+test_expect_success 'for_each_reflog_ent()' '
+   $RUN for-each-reflog-ent HEAD >actual &&
+   head -n1 actual | grep one &&
+   tail -n2 actual | head -n1 | grep recreate-master
+'
+
+test_expect_success 'for_each_reflog_ent_reverse()' '
+   $RUN for-each-reflog-ent-reverse HEAD >actual &&
+   head -n1 actual | grep recreate-master &&
+   tail -n2 actual | head -n1 | grep one
+'
+
+test_expect_success 'reflog_exists(HEAD)' '
+   $RUN reflog-exists HEAD
+'
+
+test_expect_success 'delete_reflog(HEAD)' '
+   $RUN delete-reflog HEAD &&
+   ! test -f .git/logs/HEAD
+'
+
+test_expect_success 'create-reflog(HEAD)' '
+   $RUN create-reflog HEAD 1 &&
+   test -f .git/logs/HEAD
+'
+
+test_expect_success 'delete_ref(refs/heads/foo)' '
+   git checkout -b foo &&
+   FOO_SHA1=`git rev-parse foo` &&
+   git checkout --detach &&
+   test_commit bar-commit &&
+   git checkout -b bar &&
+   BAR_SHA1=`git rev-parse bar` &&
+   $RUN update-ref updating refs/heads/foo $BAR_SHA1 $FOO_SHA1 0 &&
+   echo $BAR_SHA1 >expected &&
+   git rev-parse refs/heads/foo >actual &&
+   test_cmp expected actual
+'
+
+test_expect_success 'delete_ref(refs/heads/foo)' '
+   SHA1=`git rev-parse foo` &&
+   git checkout --detach &&
+   $RUN delete-ref msg refs/heads/foo $SHA1 0 &&
+   test_must_fail git rev-parse refs/heads/foo --
+'
+
+test_done
-- 
2.11.0.157.gd943d85

[PATCH v7 21/28] refs: add new ref-store api

This is not meant to cover all existing API. It adds enough to test ref
stores with the new test program test-ref-store, coming soon and to be
used by files-backend.c.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 refs.c   | 251 ++-
 refs.h   |  74 +++
 refs/files-backend.c |  13 +--
 refs/refs-internal.h |  31 +--
 4 files changed, 270 insertions(+), 99 deletions(-)

diff --git a/refs.c b/refs.c
index 100a925bcc..c103f90b35 100644
--- a/refs.c
+++ b/refs.c
@@ -171,11 +171,23 @@ int refname_is_safe(const char *refname)
return 1;
 }
 
+char *refs_resolve_refdup(struct ref_store *refs,
+ const char *refname, int resolve_flags,
+ unsigned char *sha1, int *flags)
+{
+   const char *result;
+
+   result = refs_resolve_ref_unsafe(refs, refname, resolve_flags,
+sha1, flags);
+   return xstrdup_or_null(result);
+}
+
 char *resolve_refdup(const char *refname, int resolve_flags,
 unsigned char *sha1, int *flags)
 {
-   return xstrdup_or_null(resolve_ref_unsafe(refname, resolve_flags,
- sha1, flags));
+   return refs_resolve_refdup(get_main_ref_store(),
+  refname, resolve_flags,
+  sha1, flags);
 }
 
 /* The argument to filter_refs */
@@ -185,13 +197,20 @@ struct ref_filter {
void *cb_data;
 };
 
-int read_ref_full(const char *refname, int resolve_flags, unsigned char *sha1, 
int *flags)
+int refs_read_ref_full(struct ref_store *refs, const char *refname,
+  int resolve_flags, unsigned char *sha1, int *flags)
 {
-   if (resolve_ref_unsafe(refname, resolve_flags, sha1, flags))
+   if (refs_resolve_ref_unsafe(refs, refname, resolve_flags, sha1, flags))
return 0;
return -1;
 }
 
+int read_ref_full(const char *refname, int resolve_flags, unsigned char *sha1, 
int *flags)
+{
+   return refs_read_ref_full(get_main_ref_store(), refname,
+ resolve_flags, sha1, flags);
+}
+
 int read_ref(const char *refname, unsigned char *sha1)
 {
return read_ref_full(refname, RESOLVE_REF_READING, sha1, NULL);
@@ -286,34 +305,52 @@ void warn_dangling_symrefs(FILE *fp, const char *msg_fmt, 
const struct string_li
for_each_rawref(warn_if_dangling_symref, &data);
 }
 
+int refs_for_each_tag_ref(struct ref_store *refs, each_ref_fn fn, void 
*cb_data)
+{
+   return refs_for_each_ref_in(refs, "refs/tags/", fn, cb_data);
+}
+
 int for_each_tag_ref(each_ref_fn fn, void *cb_data)
 {
-   return for_each_ref_in("refs/tags/", fn, cb_data);
+   return refs_for_each_tag_ref(get_main_ref_store(), fn, cb_data);
 }
 
 int for_each_tag_ref_submodule(const char *submodule, each_ref_fn fn, void 
*cb_data)
 {
-   return for_each_ref_in_submodule(submodule, "refs/tags/", fn, cb_data);
+   return refs_for_each_tag_ref(get_submodule_ref_store(submodule),
+fn, cb_data);
+}
+
+int refs_for_each_branch_ref(struct ref_store *refs, each_ref_fn fn, void 
*cb_data)
+{
+   return refs_for_each_ref_in(refs, "refs/heads/", fn, cb_data);
 }
 
 int for_each_branch_ref(each_ref_fn fn, void *cb_data)
 {
-   return for_each_ref_in("refs/heads/", fn, cb_data);
+   return refs_for_each_branch_ref(get_main_ref_store(), fn, cb_data);
 }
 
 int for_each_branch_ref_submodule(const char *submodule, each_ref_fn fn, void 
*cb_data)
 {
-   return for_each_ref_in_submodule(submodule, "refs/heads/", fn, cb_data);
+   return refs_for_each_branch_ref(get_submodule_ref_store(submodule),
+   fn, cb_data);
+}
+
+int refs_for_each_remote_ref(struct ref_store *refs, each_ref_fn fn, void 
*cb_data)
+{
+   return refs_for_each_ref_in(refs, "refs/remotes/", fn, cb_data);
 }
 
 int for_each_remote_ref(each_ref_fn fn, void *cb_data)
 {
-   return for_each_ref_in("refs/remotes/", fn, cb_data);
+   return refs_for_each_remote_ref(get_main_ref_store(), fn, cb_data);
 }
 
 int for_each_remote_ref_submodule(const char *submodule, each_ref_fn fn, void 
*cb_data)
 {
-   return for_each_ref_in_submodule(submodule, "refs/remotes/", fn, 
cb_data);
+   return refs_for_each_remote_ref(get_submodule_ref_store(submodule),
+   fn, cb_data);
 }
 
 int head_ref_namespaced(each_ref_fn fn, void *cb_data)
@@ -1120,14 +1157,17 @@ const char *find_descendant_ref(const char *dirname,
return NULL;
 }
 
-int rename_ref_available(const char *old_refname, const char *new_refname)
+int refs_rename_ref_available(struct ref_store *refs,
+ const char *old_refname,
+ const char *new_refname)
 {
struct string_list skip = STRING_LIST_INIT_NODUP;
struct str

[PATCH v7 23/28] files-backend: avoid ref api targetting main ref store

A small step towards making files-backend works as a non-main ref store
using the newly added store-aware API.

For the record, `join` and `nm` on refs.o and files-backend.o tell me
that files-backend no longer uses functions that defaults to
get_main_ref_store().

I'm not yet comfortable at the idea of removing
files_assert_main_repository() (or converting REF_STORE_MAIN to
REF_STORE_WRITE). More staring and testing is required before that can
happen. Well, except peel_ref(). I'm pretty sure that function is safe.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 refs/files-backend.c | 84 ++--
 1 file changed, 49 insertions(+), 35 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index dec8540a0f..a5b405436f 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1830,8 +1830,6 @@ static int files_peel_ref(struct ref_store *ref_store,
int flag;
unsigned char base[20];
 
-   files_assert_main_repository(refs, "peel_ref");
-
if (current_ref_iter && current_ref_iter->refname == refname) {
struct object_id peeled;
 
@@ -1841,7 +1839,8 @@ static int files_peel_ref(struct ref_store *ref_store,
return 0;
}
 
-   if (read_ref_full(refname, RESOLVE_REF_READING, base, &flag))
+   if (refs_read_ref_full(ref_store, refname,
+  RESOLVE_REF_READING, base, &flag))
return -1;
 
/*
@@ -2009,15 +2008,15 @@ static struct ref_iterator *files_ref_iterator_begin(
  * on success. On error, write an error message to err, set errno, and
  * return a negative value.
  */
-static int verify_lock(struct ref_lock *lock,
+static int verify_lock(struct ref_store *ref_store, struct ref_lock *lock,
   const unsigned char *old_sha1, int mustexist,
   struct strbuf *err)
 {
assert(err);
 
-   if (read_ref_full(lock->ref_name,
- mustexist ? RESOLVE_REF_READING : 0,
- lock->old_oid.hash, NULL)) {
+   if (refs_read_ref_full(ref_store, lock->ref_name,
+  mustexist ? RESOLVE_REF_READING : 0,
+  lock->old_oid.hash, NULL)) {
if (old_sha1) {
int save_errno = errno;
strbuf_addf(err, "can't verify ref '%s'", 
lock->ref_name);
@@ -2086,8 +2085,9 @@ static struct ref_lock *lock_ref_sha1_basic(struct 
files_ref_store *refs,
resolve_flags |= RESOLVE_REF_ALLOW_BAD_NAME;
 
files_ref_path(refs, &ref_file, refname);
-   resolved = !!resolve_ref_unsafe(refname, resolve_flags,
-   lock->old_oid.hash, type);
+   resolved = !!refs_resolve_ref_unsafe(&refs->base,
+refname, resolve_flags,
+lock->old_oid.hash, type);
if (!resolved && errno == EISDIR) {
/*
 * we are trying to lock foo but we used to
@@ -2104,8 +2104,9 @@ static struct ref_lock *lock_ref_sha1_basic(struct 
files_ref_store *refs,
refname);
goto error_return;
}
-   resolved = !!resolve_ref_unsafe(refname, resolve_flags,
-   lock->old_oid.hash, type);
+   resolved = !!refs_resolve_ref_unsafe(&refs->base,
+refname, resolve_flags,
+lock->old_oid.hash, type);
}
if (!resolved) {
last_errno = errno;
@@ -2143,7 +2144,7 @@ static struct ref_lock *lock_ref_sha1_basic(struct 
files_ref_store *refs,
goto error_return;
}
 
-   if (verify_lock(lock, old_sha1, mustexist, err)) {
+   if (verify_lock(&refs->base, lock, old_sha1, mustexist, err)) {
last_errno = errno;
goto error_return;
}
@@ -2398,7 +2399,7 @@ static void try_remove_empty_parents(struct 
files_ref_store *refs,
 }
 
 /* make sure nobody touched the ref, and unlink */
-static void prune_ref(struct ref_to_prune *r)
+static void prune_ref(struct files_ref_store *refs, struct ref_to_prune *r)
 {
struct ref_transaction *transaction;
struct strbuf err = STRBUF_INIT;
@@ -2406,7 +2407,7 @@ static void prune_ref(struct ref_to_prune *r)
if (check_refname_format(r->name, 0))
return;
 
-   transaction = ref_transaction_begin(&err);
+   transaction = ref_store_transaction_begin(&refs->base, &err);
if (!transaction ||
ref_transaction_delete(transaction, r->name, r->sha1,
   REF_ISPRUNING | REF_NODEREF, NULL, &err) ||
@@ -2420,10 +2421,10 @@ static void prune_ref(struct ref_to_prune *r)
strbuf_release

[PATCH v7 25/28] t/helper: add test-ref-store to test ref-store functions

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 Makefile|   1 +
 t/helper/.gitignore |   1 +
 t/helper/test-ref-store.c (new) | 277 
 3 files changed, 279 insertions(+)
 create mode 100644 t/helper/test-ref-store.c

diff --git a/Makefile b/Makefile
index a5a11e721a..5f3844e33e 100644
--- a/Makefile
+++ b/Makefile
@@ -622,6 +622,7 @@ TEST_PROGRAMS_NEED_X += test-parse-options
 TEST_PROGRAMS_NEED_X += test-path-utils
 TEST_PROGRAMS_NEED_X += test-prio-queue
 TEST_PROGRAMS_NEED_X += test-read-cache
+TEST_PROGRAMS_NEED_X += test-ref-store
 TEST_PROGRAMS_NEED_X += test-regex
 TEST_PROGRAMS_NEED_X += test-revision-walking
 TEST_PROGRAMS_NEED_X += test-run-command
diff --git a/t/helper/.gitignore b/t/helper/.gitignore
index d6e8b36798..5f68aa8f8a 100644
--- a/t/helper/.gitignore
+++ b/t/helper/.gitignore
@@ -19,6 +19,7 @@
 /test-path-utils
 /test-prio-queue
 /test-read-cache
+/test-ref-store
 /test-regex
 /test-revision-walking
 /test-run-command
diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c
new file mode 100644
index 00..2d84c45ffe
--- /dev/null
+++ b/t/helper/test-ref-store.c
@@ -0,0 +1,277 @@
+#include "cache.h"
+#include "refs.h"
+
+static const char *notnull(const char *arg, const char *name)
+{
+   if (!arg)
+   die("%s required", name);
+   return arg;
+}
+
+static unsigned int arg_flags(const char *arg, const char *name)
+{
+   return atoi(notnull(arg, name));
+}
+
+static const char **get_store(const char **argv, struct ref_store **refs)
+{
+   const char *gitdir;
+
+   if (!argv[0]) {
+   die("ref store required");
+   } else if (!strcmp(argv[0], "main")) {
+   *refs = get_main_ref_store();
+   } else if (skip_prefix(argv[0], "submodule:", &gitdir)) {
+   struct strbuf sb = STRBUF_INIT;
+   int ret;
+
+   ret = strbuf_git_path_submodule(&sb, gitdir, "objects/");
+   if (ret)
+   die("strbuf_git_path_submodule failed: %d", ret);
+   add_to_alternates_memory(sb.buf);
+   strbuf_release(&sb);
+
+   *refs = get_submodule_ref_store(gitdir);
+   } else
+   die("unknown backend %s", argv[0]);
+
+   if (!*refs)
+   die("no ref store");
+
+   /* consume store-specific optional arguments if needed */
+
+   return argv + 1;
+}
+
+
+static int cmd_pack_refs(struct ref_store *refs, const char **argv)
+{
+   unsigned int flags = arg_flags(*argv++, "flags");
+
+   return refs_pack_refs(refs, flags);
+}
+
+static int cmd_peel_ref(struct ref_store *refs, const char **argv)
+{
+   const char *refname = notnull(*argv++, "refname");
+   unsigned char sha1[20];
+   int ret;
+
+   ret = refs_peel_ref(refs, refname, sha1);
+   if (!ret)
+   puts(sha1_to_hex(sha1));
+   return ret;
+}
+
+static int cmd_create_symref(struct ref_store *refs, const char **argv)
+{
+   const char *refname = notnull(*argv++, "refname");
+   const char *target = notnull(*argv++, "target");
+   const char *logmsg = *argv++;
+
+   return refs_create_symref(refs, refname, target, logmsg);
+}
+
+static int cmd_delete_refs(struct ref_store *refs, const char **argv)
+{
+   unsigned int flags = arg_flags(*argv++, "flags");
+   struct string_list refnames = STRING_LIST_INIT_NODUP;
+
+   while (*argv)
+   string_list_append(&refnames, *argv++);
+
+   return refs_delete_refs(refs, &refnames, flags);
+}
+
+static int cmd_rename_ref(struct ref_store *refs, const char **argv)
+{
+   const char *oldref = notnull(*argv++, "oldref");
+   const char *newref = notnull(*argv++, "newref");
+   const char *logmsg = *argv++;
+
+   return refs_rename_ref(refs, oldref, newref, logmsg);
+}
+
+static int each_ref(const char *refname, const struct object_id *oid,
+   int flags, void *cb_data)
+{
+   printf("%s %s 0x%x\n", oid_to_hex(oid), refname, flags);
+   return 0;
+}
+
+static int cmd_for_each_ref(struct ref_store *refs, const char **argv)
+{
+   const char *prefix = notnull(*argv++, "prefix");
+
+   return refs_for_each_ref_in(refs, prefix, each_ref, NULL);
+}
+
+static int cmd_resolve_ref(struct ref_store *refs, const char **argv)
+{
+   unsigned char sha1[20];
+   const char *refname = notnull(*argv++, "refname");
+   int resolve_flags = arg_flags(*argv++, "resolve-flags");
+   int flags;
+   const char *ref;
+
+   ref = refs_resolve_ref_unsafe(refs, refname, resolve_flags,
+ sha1, &flags);
+   printf("%s %s 0x%x\n", sha1_to_hex(sha1), ref, flags);
+   return ref ? 0 : 1;
+}
+
+static int cmd_verify_ref(struct ref_store *refs, const char **argv)
+{
+   const char *refname = notnull(*argv++, "refname");
+   struct strbuf err = STRBUF_INIT;
+   int ret;
+
+

[PATCH v7 27/28] t1406: new tests for submodule ref store

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 t/t1406-submodule-ref-store.sh (new +x) | 95 +
 1 file changed, 95 insertions(+)
 create mode 100755 t/t1406-submodule-ref-store.sh

diff --git a/t/t1406-submodule-ref-store.sh b/t/t1406-submodule-ref-store.sh
new file mode 100755
index 00..22214ebd32
--- /dev/null
+++ b/t/t1406-submodule-ref-store.sh
@@ -0,0 +1,95 @@
+#!/bin/sh
+
+test_description='test submodule ref store api'
+
+. ./test-lib.sh
+
+RUN="test-ref-store submodule:sub"
+
+test_expect_success 'setup' '
+   git init sub &&
+   (
+   cd sub &&
+   test_commit first &&
+   git checkout -b new-master
+   )
+'
+
+test_expect_success 'pack_refs() not allowed' '
+   test_must_fail $RUN pack-refs 3
+'
+
+test_expect_success 'peel_ref(new-tag)' '
+   git -C sub rev-parse HEAD >expected &&
+   git -C sub tag -a -m new-tag new-tag HEAD &&
+   $RUN peel-ref refs/tags/new-tag >actual &&
+   test_cmp expected actual
+'
+
+test_expect_success 'create_symref() not allowed' '
+   test_must_fail $RUN create-symref FOO refs/heads/master nothing
+'
+
+test_expect_success 'delete_refs() not allowed' '
+   test_must_fail $RUN delete-refs 0 FOO refs/tags/new-tag
+'
+
+test_expect_success 'rename_refs() not allowed' '
+   test_must_fail $RUN rename-ref refs/heads/master refs/heads/new-master
+'
+
+test_expect_success 'for_each_ref(refs/heads/)' '
+   $RUN for-each-ref refs/heads/ | cut -c 42- >actual &&
+   cat >expected <<-\EOF &&
+   master 0x0
+   new-master 0x0
+   EOF
+   test_cmp expected actual
+'
+
+test_expect_success 'resolve_ref(master)' '
+   SHA1=`git -C sub rev-parse master` &&
+   echo "$SHA1 refs/heads/master 0x0" >expected &&
+   $RUN resolve-ref refs/heads/master 0 >actual &&
+   test_cmp expected actual
+'
+
+test_expect_success 'verify_ref(new-master)' '
+   $RUN verify-ref refs/heads/new-master
+'
+
+test_expect_success 'for_each_reflog()' '
+   $RUN for-each-reflog | sort | cut -c 42- >actual &&
+   cat >expected <<-\EOF &&
+   HEAD 0x1
+   refs/heads/master 0x0
+   refs/heads/new-master 0x0
+   EOF
+   test_cmp expected actual
+'
+
+test_expect_success 'for_each_reflog_ent()' '
+   $RUN for-each-reflog-ent HEAD >actual && cat actual &&
+   head -n1 actual | grep first &&
+   tail -n2 actual | head -n1 | grep master.to.new
+'
+
+test_expect_success 'for_each_reflog_ent_reverse()' '
+   $RUN for-each-reflog-ent-reverse HEAD >actual &&
+   head -n1 actual | grep master.to.new &&
+   tail -n2 actual | head -n1 | grep first
+'
+
+test_expect_success 'reflog_exists(HEAD)' '
+   $RUN reflog-exists HEAD
+'
+
+test_expect_success 'delete_reflog() not allowed' '
+   test_must_fail $RUN delete-reflog HEAD
+'
+
+test_expect_success 'create-reflog() not allowed' '
+   test_must_fail $RUN create-reflog HEAD 1
+'
+
+test_done
-- 
2.11.0.157.gd943d85

[PATCH v7 28/28] refs.h: add a note about sorting order of for_each_ref_*

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 refs.h | 4 ++--
 t/t1405-main-ref-store.sh  | 6 ++
 t/t1406-submodule-ref-store.sh | 6 ++
 3 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/refs.h b/refs.h
index 1a07f9d86f..49e97d7d5f 100644
--- a/refs.h
+++ b/refs.h
@@ -230,7 +230,7 @@ typedef int each_ref_fn(const char *refname,
  * it is not safe to modify references while an iteration is in
  * progress, unless the same callback function invocation that
  * modifies the reference also returns a nonzero value to immediately
- * stop the iteration.
+ * stop the iteration. Returned references are sorted.
  */
 int refs_for_each_ref(struct ref_store *refs,
  each_ref_fn fn, void *cb_data);
@@ -370,7 +370,7 @@ int for_each_reflog_ent_reverse(const char *refname, 
each_reflog_ent_fn fn, void
 
 /*
  * Calls the specified function for each reflog file until it returns nonzero,
- * and returns the value
+ * and returns the value. Reflog file order is unspecified.
  */
 int refs_for_each_reflog(struct ref_store *refs, each_ref_fn fn, void 
*cb_data);
 int for_each_reflog(each_ref_fn fn, void *cb_data);
diff --git a/t/t1405-main-ref-store.sh b/t/t1405-main-ref-store.sh
index 77e1c130c2..490521f8cb 100755
--- a/t/t1405-main-ref-store.sh
+++ b/t/t1405-main-ref-store.sh
@@ -53,6 +53,12 @@ test_expect_success 'for_each_ref(refs/heads/)' '
test_cmp expected actual
 '
 
+test_expect_success 'for_each_ref() is sorted' '
+   $RUN for-each-ref refs/heads/ | cut -c 42- >actual &&
+   sort actual > expected &&
+   test_cmp expected actual
+'
+
 test_expect_success 'resolve_ref(new-master)' '
SHA1=`git rev-parse new-master` &&
echo "$SHA1 refs/heads/new-master 0x0" >expected &&
diff --git a/t/t1406-submodule-ref-store.sh b/t/t1406-submodule-ref-store.sh
index 22214ebd32..13b5454c56 100755
--- a/t/t1406-submodule-ref-store.sh
+++ b/t/t1406-submodule-ref-store.sh
@@ -47,6 +47,12 @@ test_expect_success 'for_each_ref(refs/heads/)' '
test_cmp expected actual
 '
 
+test_expect_success 'for_each_ref() is sorted' '
+   $RUN for-each-ref refs/heads/ | cut -c 42- >actual &&
+   sort actual > expected &&
+   test_cmp expected actual
+'
+
 test_expect_success 'resolve_ref(master)' '
SHA1=`git -C sub rev-parse master` &&
echo "$SHA1 refs/heads/master 0x0" >expected &&
-- 
2.11.0.157.gd943d85

[GSoC] Proposal: turn git-add--interactive.perl into a builtin

2017-03-25 Thread Daniel Ferreira (theiostream)

Hi there. First of all, I'd like to thank all of the support up to now
with my microproject :). Here's a first draft of my proposal for
Google Summer of Code '17, based on the "Convert scripts to builtins"
idea. Please let me know what you think.

---

SYNOPSIS
There are many advantages to converting parts of git that are still
scripts to C builtins, among which execution speed, improved
compatibility and code deduplication. This proposal aims to apply this
to git-add--interactive, one of the most useful features of Git.

FEASIBILITY
Many git scripts have attracted attention for being turned into
builtins. There is ongoing work on git-stash
(https://public-inbox.org/git/20170321053135.thk77soxc4irx...@sigill.intra.peff.net/),
and porting interactive rebase is one of the ideas for this edition of
GSoC. Not as much attention, however, has been directed to
git-add--interactive.

There was only one discussion regarding the feasibility of its porting
(https://public-inbox.org/git/CAP8UFD2PcBsU6=FK4OHVrB7E98ycohS_0pYcbCBar=of1hl...@mail.gmail.com/).
It resulted in a consensus that doing it would be a task too large –
although interesting – for GSoC 2015 based on the amount of its lines
of code. It is, however, only a few lines larger than
git-rebase--interactive, which has been considered an appropriate
idea. As such, it looks like a possible project for three months of
full-time work.

Aside from the benefits cited above, turning git-add--interactive into
a builtin can reduce Git's dependency on Perl to the point where no
"common" command would continue to rely on it.

PROJECTED TIMELINE
- Prior to May 4
-- Refine my basic knowledge of Perl
-- Craft one or two small patches to some of Git's Perl components
(preferentially to git-add--interactive itself) to improve my
understanding of the language and of how Git's Perl scripts actually
work

- May 4 - May 30
-- Clarify implementation details with my mentor, and work on a more
detailed roadmap for the project
-- Investigate roughly how to replace command invocations from the
script with actual builtin functions; which Git APIs in Perl already
have functional equivalents in C; which parts will require a full
rewrite.

- May 30 - June 30 (start of coding period)
-- Define the architecture of the builtin within git (which
functions/interfaces will it have? where will its code reside?).
-- Implement a small subset of the builtin (to be defined with my
mentor) and glue it into the existing Perl script. Present this as a
first patch to get feedback early regarding the implementation and
avoid piling up mistakes early.
-- Do necessary changes based on this initial review.
-- Have roughly 1/3 of the script's functionality ported to C.

- June 30 - July 28
-- Port the remainder of the script to a builtin.
-- Have a weekly roadmap, sending a part of the patch every 15 days to
the mailing list for review and to avoid massive commits by the end of
GSoC.
-- Apply suggestions from community reviews when possible; if not,
save them for doing toward the end of GSoC (see below).
(Note: due to a previous commitment, during a five-day period of July
I will only be able to work part-time on GSoC. The actual week will be
known over the next weeks.)

- July 28 - August 29
-- By the start of this period, send a patch with the builtin fully
implemented to the mailing list.
-- Fix bugs, test extensively, possibly extend test coverage for
git-add--interactive.
-- Respond to the (predictably big) community feedback regarding the change.

I currently work full-time in a payments company (see below), but in
case of being accepted I am willing to quit my job some months early
to dedicate myself fully to GSoC starting June.

BIOGRAPHICAL INFORMATION
My name is Daniel Ferreira and I'm a student from São Paulo, Brazil. I
was accepted by Stanford University last year and I will start college
this fall. I started coding C about six years ago writing up system
modifications ("tweaks") for jailbroken iPhones. Since then, I have
written/contributed to a couple of open-source projects like an IRC
bot and other assorted things – all of them tracked on Git
(https://github.com/theiostream). I have also developed a
(closed-source) library in C for interacting with payment terminals in
the company I have worked for over the last two years (Pagar.me).
There, we use Git extensively for managing projects with around 20
people working concurrently.

MICROPROJECT
I have sent a series of patches to complete the microproject of
converting recursive calls to readdir() into calls to dir_iterator.
The most recent version can be found in
https://public-inbox.org/git/1490465551-71056-2-git-send-email-bnm...@gmail.com/T/#u.

Thanks,
-- Daniel.

Re: [PATCH v4 1/2] diff --no-index: optionally follow symlinks

2017-03-25 Thread Junio C Hamano

On Sat, Mar 25, 2017 at 2:30 PM, Dennis Kaarsemaker
 wrote:
>
>>  - does this code do a reasonable thing when the path is a symbolic
>>link that points at a directory?  what does it mean to grab
>>st.st_size for such a thing (and then go on to open() and xmmap()
>>it)?
>
> No, it does something entirely unreasonable. I hadn't even thought of
> testing with symlinks to directories, as my ulterior motive was the
> next commit that makes it work with pipes. This will be fixed.

To be quite honest, I do not mind it if the "toplevel pipe that came from the
command line is treated as if it were a regular file" was the only change in
this series, without doing anything for symbolic links. I do not use the
process substitution myself, but I can see why sometimes it is handy to
pass two process invocations on the command line of "diff" (if it were only
one, then "-" with the usual redirection already works, but you cannot do
two command using that syntax).

Perhaps we can have only that part and perfect it first and have it ready for
the next release, postponing the symlink dereferencing, which is a different
issue?

Re: Will OpenSSL's license change impact us?

On Sat, Mar 25, 2017 at 10:52:47PM +0100, Ævar Arnfjörð Bjarmason wrote:

> > If we want to consider performance-related concerns, I think the easier
> > solution is using Nettle, which is LGPL 2.1.  Considering that the
> > current opinions for a new hash function are moving in the direction of
> > SHA-3, which Nettle has, but OpenSSL does not, I think that might be a
> > better decision overall.  It was certainly the implementation I would
> > use if I were to implement it.
> 
> Yeah there's a lot of options open for just sha1-ing, but we also use
> OpenSSL for TLS via imap-send.

These days imap-send has basically two implementations: one that speaks
imap itself (optionally using openssl), and one that just uses curl's
imap support.  If you build with NO_OPENSSL, the curl implementation
kicks in by default.

So I think any distro worried about licensing can just "make NO_OPENSSL"
today and get full functionality.

Curl may use openssl behind the scenes, of course, but distros already
have to deal with that (at least on Debian, you can drop-in gnutls).

-Peff

Re: [PATCH 0/7] PREVIEW: Introduce DC_AND_OPENSSL_SHA1 make flag