[PATCH v6 03/27] files-backend: delete dead code in files_init_db()

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

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 0a6d2bf6bc..0ef80c51cc 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -4110,10 +4110,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

Re: Bug with .gitignore and branch switching

[Nevada Sanchez]

On Fri, Mar 17, 2017 at 5:23 PM, Junio C Hamano  wrote:
> Nevada Sanchez  writes:
>
>> Here's an easy to reproduce bug. It's the only example I know of where
>> git legitimately loses data in a way that is unrecoverable,
>> unexpected, and without warning.
>
> This is an example of a user explicitly telling git to discard data
> and git performing as it is told.
>
> There is no "untracked but precious" vs "untracked and expendable"
> difference in the current system.  An untracked file that matches
> patterns listed in .gitignore is treated as the latter.
>
> When you have an untracked file that .gitignore knows about in the
> working tree while you are on "feature", if switching to another
> branch requires to remove that file, the content there is deemed
> expendable, because the user said so by listing it in .gitignore.
>
> We've discussed the lack of "untracked but precious" class a few
> times on the list in the past, but I do not recall the topic came up
> in the recent past.  It perhaps is because nobody found that class
> useful enough so far.

I must admit that I missed that attribute of .gitignore (i.e.
untracked and **expendable**). I have grown accustomed to Git being
rather conservative and erring on the side of not losing data unless
the user is doing something deliberate (for example, 'git clean' won't
work unless you force it, checkouts fail if they do anything that
might lose data... unless it is in .gitignore, as I just learned).
When I saw this behavior, I assumed that it was a bug.

This isn't necessarily a situation I need to have fixed--it is not
part of my workflow and since that fateful commit, all feature
branches checked out after the change to .gitignore will not have any
problems as I switch branches. It was an unfortunate surprise to one
of my co-workers, not long after I reassured him that git was
conservative and will almost never accidentally lose data (even if it
means going to 'git reflog').

In keeping with this spirit, I would tend to lean towards having
"untracked but precious" being the default behavior (more
conservative), and if a user wants "untracked but expendable"
behavior, then that case requires special effort from the user (like
learning about and using a new type of ignore file). My guess is that
if the user is both ignoring and committing something to their
repository, it is probably a mistake, and as that user, I would rather
discover that mistake early with loud warning messages (and/or a
suggestion to use an alternate ignore strategy, or config flag), than
learn about it by losing data.

In summary, I do not need this fixed for my workflow, but want to
bring it to light in case other users are being similarly surprised. I
struggle to guess how far reaching of an impact it would have on
existing users to change the default behavior, but it would probably
be less than that of the push default behavior change that happened
not too long ago.

A quick and easy immediate step is to make note of this behavior in
the very first sentence of gitignore(5):

> A gitignore file specifies intentionally untracked files that Git should 
> ignore *and that Git is allowed to overwrite without warning*.

More details about untracked but expendable can be placed in the NOTES
section, but the last part of that sentence would be quite helpful.

Thank you,
-Nevada

Re: Bug with .gitignore and branch switching

[Duy Nguyen]

On Sat, Mar 18, 2017 at 5:02 AM, Jonathan Nieder  wrote:
> Junio C Hamano wrote:
>
>> There is no "untracked but precious" vs "untracked and expendable"
>> difference in the current system.  An untracked file that matches
>> patterns listed in .gitignore is treated as the latter.
> [...]
>> We've discussed the lack of "untracked but precious" class a few
>> times on the list in the past, but I do not recall the topic came up
>> in the recent past.  It perhaps is because nobody found that class
>> useful enough so far.
>
> The most recent example I can find is 2010:
> http://public-inbox.org/git/4c6a1c5b.4030...@workspacewhiz.com/.
>
> It also came up in 2007:
> http://public-inbox.org/git/c0e9f681e68d48eb8989022d11fee...@ntdev.corp.microsoft.com/
> Earlier in that year it even made the "What's not in 1.5.2" list.
> http://public-inbox.org/git/11793556383977-git-send-email-jun...@cox.net/
>
> Perhaps those references could be a useful starting point for an
> interested person's thinking.

I think I made it work in 2014 [1] using new "precious" attribute, but
never submitted it, probably because I was worried about the
interaction with untracked cache (adding .gitattributes as a new
dependency) though maybe we can avoid that by always checking for
preciousness after all the tree walking/filtering is done, either with
or without untracked cache. But I never addressed that loose end. Then
again, it could also be another useful starting point for interested
person's thinking ;-)

[1] 
https://github.com/pclouds/git/commit/0e7f7afa1879b055369ebd3f1224311c43c8a32b
-- 
Duy

Re: [PATCH] gitk: Add Chinese(zh_CN) translation

[Jiang Xin]

Hi Yanke,

You should send this patch to the gitk maintainer, not here.

Gitk has a separate repository, which has its own workflow according to
"Documentation/SubmittingPatches":

Some parts of the system have dedicated maintainers with their own
repositories.

...

 - gitk-git/ comes from Paul Mackerras's gitk project:

git://ozlabs.org/~paulus/gitk


在 2017年3月18日 上午2:14，  写道：
> From: YanKe 
>
> Signed-off-by: YanKe 
> ---
>  po/zh_CN.po | 1367 
> +++
>  1 file changed, 1367 insertions(+)
>  create mode 100644 po/zh_CN.po
>
> diff --git a/po/zh_CN.po b/po/zh_CN.po
> new file mode 100644
> index 000..17b7f89
> --- /dev/null
> +++ b/po/zh_CN.po



-- 
Jiang Xin

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

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

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 v6 00/27] Remove submodule from files-backend.c

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

v6 should address all Micheal's comments in v5, except a few that I
replied back. The interdiff looks a bit messy due to some new
refactoring (05/27) in files_rename_ref(), but it makes the final
output easier to read.

03/27 and 27/27 are also new patches that are not really necessary but
nice to have.

The series is rebased on latest master and does not depend on any
other topics, since they all have graduated to master.

diff --git a/path.c b/path.c
index 3451d2916f..03e7e33b6e 100644
--- a/path.c
+++ b/path.c
@@ -471,7 +471,6 @@ 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)
 {
diff --git a/refs.c b/refs.c
index 4e4945bd1c..24c7113d36 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 */
@@ -1521,12 +1533,7 @@ struct ref_store *get_main_ref_store(void)
   REF_STORE_WRITE |
   REF_STORE_ODB |
   REF_STORE_MAIN));
-   if (refs) {
-   if (main_ref_store)
-   die("BUG: main_ref_store initialized twice");
-
-   main_ref_store = refs;
-   }
+   main_ref_store = refs;
return refs;
 }
 
@@ -1569,17 +1576,20 @@ struct ref_store *get_submodule_ref_store(const char 
*submodule)
ret = is_nonbare_repository_dir(_sb);
strbuf_release(_sb);
if (!ret)
-   return refs;
+   return NULL;
 
ret = submodule_to_gitdir(_sb, submodule);
-   if (!ret)
-   /* pretend that add_submodule_odb() has been called */
-   refs = ref_store_init(submodule_sb.buf,
- REF_STORE_READ | REF_STORE_ODB);
-   strbuf_release(_sb);
+   if (ret) {
+   strbuf_release(_sb);
+   return NULL;
+   }
 
-   if (refs)
-   register_submodule_ref_store(refs, submodule);
+   /* 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(_sb);
return refs;
 }
 
diff --git a/refs.h b/refs.h
index 3fc140c16f..4f9983816a 100644
--- a/refs.h
+++ b/refs.h
@@ -3,7 +3,6 @@
 
 struct object_id;
 struct ref_store;
-struct ref_transaction;
 struct strbuf;
 struct string_list;
 
@@ -66,6 +65,9 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs,
 const char *resolve_ref_unsafe(const char *refname, int resolve_flags,
   unsigned char *sha1, int *flags);
 
+char *refs_resolve_refdup(struct ref_store *refs,
+ const char *refname, int resolve_flags,
+ unsigned char *sha1, int *flags);
 char *resolve_refdup(const char *refname, int resolve_flags,
 unsigned char *sha1, int *flags);
 
@@ -183,6 +185,7 @@ int dwim_log(const char *str, int len, unsigned char *sha1, 
char **ref);
  * `ref_transaction_commit` is called.  So `ref_transaction_verify`
  * won't report a verification failure until the commit is attempted.
  */
+struct ref_transaction;
 
 /*
  * Bit values set in the flags argument passed to each_ref_fn() and
@@ -227,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);
@@ -367,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 

[PATCH v6 02/27] files-backend: make files_log_ref_write() static

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

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 | 3 +++
 refs/refs-internal.h | 4 
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 50188e92f9..0a6d2bf6bc 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)
 {
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 v6 04/27] files-backend: add and use files_packed_refs_path()

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

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 0ef80c51cc..6e6962151a 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, _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();
 }
 
+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(>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;
 }
 
@@ -2160,7 +2167,7 @@ static int lock_packed_refs(struct files_ref_store *refs, 
int flags)
}
 
if (hold_lock_file_for_update_timeout(
-   , git_path("packed-refs"),
+   , files_packed_refs_path(refs),
flags, timeout_value) < 0)
return -1;
/*
@@ -2426,7 +2433,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 v6 20/27] refs: add new ref-store api

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

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 b179fb3106..a3fc60ef61 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, );
 }
 
+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;

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

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

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 | 86 +---
 refs/refs-internal.h |  9 +-
 3 files changed, 73 insertions(+), 37 deletions(-)

diff --git a/refs.c b/refs.c
index fe724869b8..305ab71249 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;
 }
 
@@ -1436,7 +1437,11 @@ struct ref_store *get_main_ref_store(void)
if (main_ref_store)
return main_ref_store;
 
-   refs = ref_store_init(get_git_dir());
+   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;
 }
@@ -1484,7 +1489,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(_sb);
diff --git a/refs/files-backend.c b/refs/files-backend.c
index db335e4ca6..7d8d4dcc16 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, _be_files);
+   refs->store_flags = flags;
 
refs->gitdir = xstrdup(gitdir);
get_common_dir_noenv(, 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 deleted 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(
-   struct ref_store *ref_store, int 

[PATCH v6 21/27] refs: new transaction related ref-store api

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

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 a3fc60ef61..5ea28ae128 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();
+   transaction = ref_store_transaction_begin(refs, );
if (!transaction ||
ref_transaction_delete(transaction, refname, old_sha1,
   flags, msg, ) ||
@@ -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, );
} else {
-   t = ref_transaction_begin();
+   t = ref_store_transaction_begin(refs, );
if (!t ||
ref_transaction_update(t, refname, new_sha1, old_sha1,
   flags, msg, ) ||
@@ -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;
@@ -1610,7 +1641,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);
 }
@@ -1729,7 +1760,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_store 

[PATCH v6 19/27] refs: rename get_ref_store() to get_submodule_ref_store() and make it public

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

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 305ab71249..b179fb3106 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)
@@ -1463,13 +1463,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

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

[Junio C Hamano]

Stefan Beller  writes:

> -List details of each worktree.  The main worktree is listed first, followed 
> by
> -each of the linked worktrees.  The output details include if the worktree is
> -bare, the revision currently checked out, and the branch currently checked 
> out
> -(or 'detached HEAD' if none).
> +List details of each working tree.  The main working tree is listed first,
> +followed by each of the linked working trees.  The output details include if
> +the working tree is bare, the revision currently checked out, and the branch
> +currently checked out (or 'detached HEAD' if none).

I do not think this is correct.

Think of a "worktree" something that roughly corresponds to
different location you can refer to with $GIT_DIR.

A $GIT_DIR may be a true repository.  Or it may be borrowing many of
the things from its primary repository.  Even though "git worktree"
Porcelain may not currently be equipped to create a "bare" $GIT_DIR,
there is no fundamental reason that a secondary "worktree" must have
a working tree, i.e. a "worktree" could be a "bare" one (it would be
the first use of per-worktree config; a secondary worktree that is a
bare can be made by borrowing from the primary worktree that has a
working tree).

Now, what does "git worktree list" enumearate?  It does not list
"working trees"; its output are list of things, each of which you
could point at with $GIT_DIR and work with.

[PATCH v6 22/27] files-backend: avoid ref api targetting main ref store

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

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 46f791a516..4242486118 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, ))
+   if (refs_read_ref_full(ref_store, refname,
+  RESOLVE_REF_READING, base, ))
return -1;
 
/*
@@ -2011,15 +2010,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);
@@ -2088,8 +2087,9 @@ static struct ref_lock *lock_ref_sha1_basic(struct 
files_ref_store *refs,
resolve_flags |= RESOLVE_REF_ALLOW_BAD_NAME;
 
files_refname_path(refs, _file, refname);
-   resolved = !!resolve_ref_unsafe(refname, resolve_flags,
-   lock->old_oid.hash, type);
+   resolved = !!refs_resolve_ref_unsafe(>base,
+refname, resolve_flags,
+lock->old_oid.hash, type);
if (!resolved && errno == EISDIR) {
/*
 * we are trying to lock foo but we used to
@@ -2106,8 +2106,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(>base,
+refname, resolve_flags,
+lock->old_oid.hash, type);
}
if (!resolved) {
last_errno = errno;
@@ -2145,7 +2146,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(>base, lock, old_sha1, mustexist, err)) {
last_errno = errno;
goto error_return;
}
@@ -2400,7 +2401,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;
@@ -2408,7 +2409,7 @@ static void prune_ref(struct ref_to_prune *r)
if (check_refname_format(r->name, 0))
return;
 
-   transaction = ref_transaction_begin();
+   transaction = ref_store_transaction_begin(>base, );
if (!transaction ||
ref_transaction_delete(transaction, r->name, r->sha1,
   REF_ISPRUNING | REF_NODEREF, NULL, ) ||
@@ -2422,10 +2423,10 @@ static void prune_ref(struct ref_to_prune *r)
strbuf_release();
 }
 
-static void 

[PATCH v6 26/27] t1406: new tests for submodule ref store

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

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 v6 24/27] t/helper: add test-ref-store to test ref-store functions

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

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 Makefile|   1 +
 t/helper/.gitignore |   1 +
 t/helper/test-ref-store.c (new) | 275 
 3 files changed, 277 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..57849ee59a
--- /dev/null
+++ b/t/helper/test-ref-store.c
@@ -0,0 +1,275 @@
+#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:", )) {
+   struct strbuf sb = STRBUF_INIT;
+   int ret;
+
+   ret = strbuf_git_path_submodule(, gitdir, "objects/");
+   if (ret)
+   die("strbuf_git_path_submodule failed: %d", ret);
+   add_to_alternates_memory(sb.buf);
+   strbuf_release();
+
+   *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(, *argv++);
+
+   return refs_delete_refs(refs, , 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, );
+   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;
+
+   ret = 

[PATCH v6 25/27] t1405: some basic tests on main ref store

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

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 v6 16/27] path.c: move some code out of strbuf_git_path_submodule()

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

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*

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 path.c  | 35 +++
 submodule.c | 36 
 submodule.h |  1 +
 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(_submodule_dir, buf);
+   ret = submodule_to_gitdir(_submodule_dir, path);
+   if (ret)
+   goto cleanup;
 
+   strbuf_complete(_submodule_dir, '/');
+   strbuf_addbuf(buf, _submodule_dir);
strbuf_vaddf(buf, fmt, args);
 
if (get_common_dir_noenv(_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(_submodule_dir);
strbuf_release(_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..3c445f274e 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1596,3 +1596,39 @@ const char *get_superproject_working_tree(void)
 
return ret;
 }
+
+/*
+ * 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)
+{
+   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..bddbba7f32 100644
--- a/submodule.h
+++ b/submodule.h
@@ -81,6 +81,7 @@ 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);
+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 v6 27/27] refs.h: add a note about sorting order of for_each_ref_*

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

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

[PATCH v6 12/27] refs: rename lookup_ref_store() to lookup_submodule_ref_store()

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

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 2637353b72..818d9f0ce9 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;
@@ -1474,7 +1470,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;
@@ -1485,7 +1481,6 @@ struct ref_store *get_ref_store(const char *submodule)
strbuf_release(_sb);
}
}
-
return refs;
 }
 
-- 
2.11.0.157.gd943d85

[PATCH v6 17/27] refs: move submodule code out of files-backend.c

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

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 404d3b62b5..fe724869b8 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;
 }
 
@@ -1435,7 +1436,7 @@ struct ref_store *get_main_ref_store(void)
if (main_ref_store)
return main_ref_store;
 
-   refs = ref_store_init(NULL);
+   refs = ref_store_init(get_git_dir());
main_ref_store = refs;
return refs;
 }
@@ -1477,8 +1478,16 @@ struct ref_store *get_ref_store(const char *submodule)
if (!ret)
return NULL;
 
-   refs = ref_store_init(submodule);
+   ret = submodule_to_gitdir(_sb, submodule);
+   if (ret) {
+   strbuf_release(_sb);
+   return NULL;
+   }
+
+   refs = ref_store_init(submodule_sb.buf);
register_submodule_ref_store(refs, submodule);
+
+   strbuf_release(_sb);
return refs;
 }
 
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 351934d36e..db335e4ca6 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, _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(, gitdir);
refs->gitcommondir = strbuf_detach(, 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 deleted in the next patch */
 }
 
 /*
@@ -1206,11 +1191,6 @@ static void files_refname_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_store;
 

[PATCH v6 08/27] files-backend: add and use files_reflog_path()

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

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 6a84e56db8..64d1ab3fe8 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.
@@ -2316,7 +2329,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;
@@ -2348,7 +2363,7 @@ static void try_remove_empty_parents(const char *refname, 
unsigned int flags)
flags &= ~REMOVE_EMPTY_PARENTS_REF;
 
strbuf_reset();
-   strbuf_git_path(, "logs/%s", buf.buf);
+   files_reflog_path(refs, , buf.buf);
if ((flags & REMOVE_EMPTY_PARENTS_REFLOG) && rmdir(sb.buf))
flags &= ~REMOVE_EMPTY_PARENTS_REFLOG;
}
@@ -2541,15 +2556,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(, "logs/%s", newrefname);
-   strbuf_git_path(, "logs/%s", TMP_RENAMED_LOG);
+   files_reflog_path(refs, , newrefname);
+   files_reflog_path(refs, , TMP_RENAMED_LOG);
cb.tmp_renamed_log = tmp.buf;
ret = raceproof_create_file(path.buf, rename_tmp_log_callback, );
if (ret) {
@@ -2609,9 +2624,9 @@ static int files_rename_ref(struct ref_store *ref_store,
int log, ret;
struct strbuf err = STRBUF_INIT;
 
-   strbuf_git_path(_oldref, "logs/%s", oldrefname);
-   strbuf_git_path(_newref, "logs/%s", newrefname);
-   strbuf_git_path(_renamed_log, "logs/%s", TMP_RENAMED_LOG);
+   files_reflog_path(refs, _oldref, oldrefname);
+   files_reflog_path(refs, _newref, newrefname);
+   files_reflog_path(refs, _renamed_log, TMP_RENAMED_LOG);
 
log = !lstat(sb_oldref.buf, );
if (log && S_ISLNK(loginfo.st_mode)) {
@@ -2674,7 +2689,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;
@@ -2789,10 +2804,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, _sb, refname);
+   logfile = strbuf_detach(_sb, NULL);
 
if (force_create || should_autocreate_reflog(refname)) {
if (raceproof_create_file(logfile, open_or_create_logfile, 
logfd)) {
@@ 

[PATCH v6 13/27] refs.c: flatten get_ref_store() a bit

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

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 818d9f0ce9..f458423301 100644
--- a/refs.c
+++ b/refs.c
@@ -1465,22 +1465,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(_sb, submodule);
-   if (is_nonbare_repository_dir(_sb))
-   refs = ref_store_init(submodule);
-   strbuf_release(_sb);
-   }
-   }
+   strbuf_addstr(_sb, submodule);
+   ret = is_nonbare_repository_dir(_sb);
+   strbuf_release(_sb);
+   if (!ret)
+   return NULL;
+
+   refs = ref_store_init(submodule);
return refs;
 }
 
-- 
2.11.0.157.gd943d85

[PATCH v6 11/27] refs.c: introduce get_main_ref_store()

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

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

diff --git a/refs.c b/refs.c
index e7606716dd..2637353b72 100644
--- a/refs.c
+++ b/refs.c
@@ -1456,15 +1456,23 @@ static struct ref_store *ref_store_init(const char 
*submodule)
return refs;
 }
 
+static struct ref_store *get_main_ref_store(void)
+{
+   struct ref_store *refs;
+
+   if (main_ref_store)
+   return main_ref_store;
+
+   refs = ref_store_init(NULL);
+   return refs;
+}
+
 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 v6 23/27] refs: delete pack_refs() in favor of refs_pack_refs()

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

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 5ea28ae128..77a39f8b17 100644
--- a/refs.c
+++ b/refs.c
@@ -1605,11 +1605,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 v6 14/27] refs.c: kill register_ref_store(), add register_submodule_ref_store()

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

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 | 43 +++
 1 file changed, 19 insertions(+), 24 deletions(-)

diff --git a/refs.c b/refs.c
index f458423301..11c460c5e9 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(_ref_stores, submodule_hash_cmp, 
0);
-
-   if (hashmap_put(_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;
 }
 
@@ -1460,9 +1436,27 @@ static struct ref_store *get_main_ref_store(void)
return main_ref_store;
 
refs = ref_store_init(NULL);
+   main_ref_store = refs;
return refs;
 }
 
+/*
+ * 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(_ref_stores, submodule_hash_cmp, 0);
+
+   if (hashmap_put(_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)
 {
struct strbuf submodule_sb = STRBUF_INIT;
@@ -1484,6 +1478,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 v6 06/27] files-backend: convert git_path() to strbuf_git_path()

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

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 6282e5868f..54e698dcea 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2319,6 +2319,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;
 
@@ -2340,14 +2341,19 @@ static void try_remove_empty_parents(const char 
*refname, unsigned int flags)
if (q == p)
break;
strbuf_setlen(, q - buf.buf);
-   if ((flags & REMOVE_EMPTY_PARENTS_REF) &&
-   rmdir(git_path("%s", buf.buf)))
+
+   strbuf_reset();
+   strbuf_git_path(, "%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();
+   strbuf_git_path(, "logs/%s", buf.buf);
+   if ((flags & REMOVE_EMPTY_PARENTS_REFLOG) && rmdir(sb.buf))
flags &= ~REMOVE_EMPTY_PARENTS_REFLOG;
}
strbuf_release();
+   strbuf_release();
 }
 
 /* make sure nobody touched the ref, and unlink */
@@ -2509,11 +2515,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.
@@ -2521,7 +2532,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;
@@ -2532,20 +2543,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, _errno);
+   strbuf_git_path(, "logs/%s", newrefname);
+   strbuf_git_path(, TMP_RENAMED_LOG);
+   cb.tmp_renamed_log = tmp.buf;
+   ret = raceproof_create_file(path.buf, rename_tmp_log_callback, );
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();
+   strbuf_release();
return ret;
 }
 
@@ -2586,10 +2603,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), );
+   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(_oldref, "logs/%s", oldrefname);
+   strbuf_git_path(_newref, "logs/%s", newrefname);
+   strbuf_git_path(_renamed_log, TMP_RENAMED_LOG);
+
+   log = !lstat(sb_oldref.buf, );
if (log && S_ISLNK(loginfo.st_mode)) {
ret = 

[PATCH v6 15/27] refs.c: make get_main_ref_store() public and use it

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

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 11c460c5e9..404d3b62b5 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)
 {
struct ref_store *refs;
 
@@ -1491,14 +1491,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);
 }
@@ -1506,7 +1506,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);
@@ -1515,7 +1515,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);
 }
@@ -1525,14 +1525,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);
@@ -1543,7 +1543,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);
@@ -1552,14 +1552,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);
 }
@@ -1567,14 +1567,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(const char 

[PATCH v6 09/27] files-backend: add and use files_refname_path()

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

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 64d1ab3fe8..1a13fb5e2b 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_refname_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(, refs->submodule, "%s", 
dirname);
-   else
-   strbuf_git_path(, "%s", dirname);
+   files_refname_path(refs, , dirname);
path_baselen = path.len;
 
-   if (err) {
-   strbuf_release();
-   return;
-   }
-
d = opendir(path.buf);
if (!d) {
strbuf_release();
@@ -1396,10 +1399,7 @@ static int files_read_raw_ref(struct ref_store 
*ref_store,
*type = 0;
strbuf_reset(_path);
 
-   if (refs->submodule)
-   strbuf_git_path_submodule(_path, refs->submodule, "%s", 
refname);
-   else
-   strbuf_git_path(_path, "%s", refname);
+   files_refname_path(refs, _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(_file, "%s", refname);
+   files_refname_path(refs, _file, refname);
 
 retry:
switch (safe_create_leading_directories(ref_file.buf)) {
@@ -2059,7 +2059,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(_file, "%s", refname);
+   files_refname_path(refs, _file, refname);
resolved = !!resolve_ref_unsafe(refname, resolve_flags,
lock->old_oid.hash, type);
if (!resolved && errno == EISDIR) {
@@ -2358,7 +2358,7 @@ static void try_remove_empty_parents(struct 
files_ref_store *refs,
strbuf_setlen(, q - buf.buf);
 
strbuf_reset();
-   strbuf_git_path(, "%s", buf.buf);
+   files_refname_path(refs, , buf.buf);
if ((flags & REMOVE_EMPTY_PARENTS_REF) && rmdir(sb.buf))
flags &= ~REMOVE_EMPTY_PARENTS_REF;
 
@@ -2675,7 +2675,7 @@ static int files_rename_ref(struct ref_store *ref_store,
struct strbuf path = STRBUF_INIT;
int result;
 
-   strbuf_git_path(, "%s", newrefname);
+   files_refname_path(refs, , newrefname);
result = remove_empty_directories();
strbuf_release();
 
@@ -3909,7 +3909,7 @@ static int files_transaction_commit(struct ref_store 
*ref_store,
update->type & REF_ISSYMREF) {
/* It is a loose reference. */
strbuf_reset();
-   strbuf_git_path(, "%s", lock->ref_name);
+   files_refname_path(refs, , lock->ref_name);
if (unlink_or_msg(sb.buf, err)) {
ret = TRANSACTION_GENERIC_ERROR;
goto cleanup;
@@ -4209,19 +4209,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 v6 05/27] files-backend: make sure files_rename_ref() always reach the end

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

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 6e6962151a..6282e5868f 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2588,23 +2588,34 @@ static int files_rename_ref(struct ref_store *ref_store,
struct stat loginfo;
int log = !lstat(git_path("logs/%s", oldrefname), );
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, ))
-   return error("refname %s not found", oldrefname);
+   orig_sha1, )) {
+   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);
@@ -2660,7 +2671,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,
@@ -2689,7 +2701,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 v6 10/27] files-backend: remove the use of git_path()

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

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 1a13fb5e2b..43d8ddac93 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, _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(, gitdir);
+   refs->gitcommondir = strbuf_detach(, NULL);
+   strbuf_addf(, "%s/packed-refs", refs->gitcommondir);
+   refs->packed_refs_path = strbuf_detach(, 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_refname_path(struct files_ref_store *refs,
@@ -1189,7 +1211,18 @@ static void files_refname_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 v6 07/27] files-backend: move "logs/" out of TMP_RENAMED_LOG

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

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 54e698dcea..6a84e56db8 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2513,7 +2513,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;
@@ -2549,7 +2549,7 @@ static int rename_tmp_log(const char *newrefname)
int ret;
 
strbuf_git_path(, "logs/%s", newrefname);
-   strbuf_git_path(, TMP_RENAMED_LOG);
+   strbuf_git_path(, "logs/%s", TMP_RENAMED_LOG);
cb.tmp_renamed_log = tmp.buf;
ret = raceproof_create_file(path.buf, rename_tmp_log_callback, );
if (ret) {
@@ -2611,7 +2611,7 @@ static int files_rename_ref(struct ref_store *ref_store,
 
strbuf_git_path(_oldref, "logs/%s", oldrefname);
strbuf_git_path(_newref, "logs/%s", newrefname);
-   strbuf_git_path(_renamed_log, TMP_RENAMED_LOG);
+   strbuf_git_path(_renamed_log, "logs/%s", TMP_RENAMED_LOG);
 
log = !lstat(sb_oldref.buf, );
if (log && S_ISLNK(loginfo.st_mode)) {
@@ -2636,7 +2636,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;
}
@@ -2722,7 +2722,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

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

[Junio C Hamano]

Jonathan Nieder  writes:

> I'm not sure I agree with this distinction.  "worktree" is just a
> short name for "working tree".

That is probably primarily my fault.  Loong before the "git
worktree" that is meant to replace the "workdir" thing in contrib/
was invented, some people said "working tree" while some other
people called the same thing "worktree", simply because it is
shorter.  Unfortunately I was among the latter, and wrote quite a
lot of documents and in-code comments, so it got stuck.  But later
we somehow ended up deciding on using "working tree" to refer to the
hierarchy of files that are checked out from a $GIT_DIR.

So if you see a message in the list archive, or an in-tree document
or in-code comment that was written way before that "let's call the
thing non-bare repositories have 'working tree'" decision was made
and have not been updated since, you are likely to find "worktree"
used to refer to "working tree".

And then relatively recently, Duy's feature started calling itself
"worktree".  The mention of "worktree" in git-worktree.txt, by
definition, is a lot newer than the "let's call the thing a non-bare
repository has 'working tree'" and was written after the word
"worktree" gained the new meaning, and it refers to that "different
things that can be referred to by setting your $GIT_DIR at them".

[PATCH v5 10/10] submodule add: respect submodule.active and submodule..active

[Brandon Williams]

In addition to adding submodule..url to the config, set
submodule..active to true unless submodule.active is configured
and the submodule's path matches the configured pathspec.

Signed-off-by: Brandon Williams 
---
 git-submodule.sh   | 14 ++
 t/t7413-submodule-is-active.sh | 21 +
 2 files changed, 35 insertions(+)

diff --git a/git-submodule.sh b/git-submodule.sh
index db94dea3b..6ec35e5fc 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -278,6 +278,20 @@ or you are unsure what this means choose another name with 
the '--name' option."
fi &&
git add --force .gitmodules ||
die "$(eval_gettext "Failed to register submodule '\$sm_path'")"
+
+   # NEEDSWORK: In a multi-working-tree world, this needs to be
+   # set in the per-worktree config.
+   if git config --get submodule.active >/dev/null
+   then
+   # If the submodule being adding isn't already covered by the
+   # current configured pathspec, set the submodule's active flag
+   if ! git submodule--helper is-active "$sm_path"
+   then
+   git config submodule."$sm_name".active "true"
+   fi
+   else
+   git config submodule."$sm_name".active "true"
+   fi
 }
 
 #
diff --git a/t/t7413-submodule-is-active.sh b/t/t7413-submodule-is-active.sh
index ea1644b58..9c785b07e 100755
--- a/t/t7413-submodule-is-active.sh
+++ b/t/t7413-submodule-is-active.sh
@@ -15,6 +15,12 @@ test_expect_success 'setup' '
test_commit -C super initial &&
git -C super submodule add ../sub sub1 &&
git -C super submodule add ../sub sub2 &&
+
+   # Remove submodule..active entries in order to test in an
+   # environment where only URLs are present in the conifg
+   git -C super config --unset submodule.sub1.active &&
+   git -C super config --unset submodule.sub2.active &&
+
git -C super commit -a -m "add 2 submodules at sub{1,2}"
 '
 
@@ -83,4 +89,19 @@ test_expect_success 'is-active with submodule.active and 
submodule..active
git -C super submodule--helper is-active sub2
 '
 
+test_expect_success 'is-active, submodule.active and submodule add' '
+   test_when_finished "rm -rf super2" &&
+   git init super2 &&
+   test_commit -C super2 initial &&
+   git -C super2 config --add submodule.active "sub*" &&
+
+   # submodule add should only add submodule..active
+   # to the config if not matched by the pathspec
+   git -C super2 submodule add ../sub sub1 &&
+   test_must_fail git -C super2 config --get submodule.sub1.active &&
+
+   git -C super2 submodule add ../sub mod &&
+   git -C super2 config --get submodule.mod.active
+'
+
 test_done
-- 
2.12.0.367.g23dc2f6d3c-goog

[PATCH v5 00/10] decoupling url and submodule interest

[Brandon Williams]

Changes in v5:
  * Add "NEEDSWORK" comments to indicate where attention is needed once
per-worktree config is a reality
  * --no-recurse now works by clearing the string list of paths.
  * module_list_active() now does post-processing instead of duplicating code.

Brandon Williams (10):
  submodule--helper: add is-active subcommand
  submodule status: use submodule--helper is-active
  submodule sync: skip work for inactive submodules
  submodule sync: use submodule--helper is-active
  submodule--helper clone: check for configured submodules using helper
  submodule: decouple url and submodule interest
  submodule init: initialize active submodules
  clone: teach --recurse-submodules to optionally take a pathspec
  submodule--helper init: set submodule..active
  submodule add: respect submodule.active and submodule..active

 Documentation/config.txt|  15 -
 Documentation/git-clone.txt |  14 +++--
 Documentation/git-submodule.txt |   4 +-
 builtin/clone.c |  50 ---
 builtin/submodule--helper.c |  68 
 git-submodule.sh|  55 ++--
 submodule.c |  50 ---
 t/t7400-submodule-basic.sh  | 136 
 t/t7413-submodule-is-active.sh  | 107 +++
 9 files changed, 445 insertions(+), 54 deletions(-)
 create mode 100755 t/t7413-submodule-is-active.sh

---interdiff with 'origin/bw/submodule-is-active'


diff --git a/builtin/clone.c b/builtin/clone.c
index 3dc8faac5..a7be61d6b 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -62,9 +62,8 @@ static int recurse_submodules_cb(const struct option *opt,
 const char *arg, int unset)
 {
if (unset)
-   return -1;
-
-   if (arg)
+   string_list_clear((struct string_list *)opt->value, 0);
+   else if (arg)
string_list_append((struct string_list *)opt->value, arg);
else
string_list_append((struct string_list *)opt->value,
@@ -984,6 +983,10 @@ int cmd_clone(int argc, const char **argv, const char 
*prefix)
string_list_sort(_recurse_submodules);
string_list_remove_duplicates(_recurse_submodules, 0);

+   /*
+* NEEDSWORK: In a multi-working-tree world, this needs to be
+* set in the per-worktree config.
+*/
for_each_string_list_item(item, _recurse_submodules) {
strbuf_addf(, "submodule.active=%s",
item->string);
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index a574596cb..7700d8948 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -273,29 +273,24 @@ static int module_list_compute(int argc, const char 
**argv,
 static void module_list_active(struct module_list *list)
 {
int i;
-
-   if (read_cache() < 0)
-   die(_("index file corrupt"));
+   struct module_list active_modules = MODULE_LIST_INIT;

gitmodules_config();

-   for (i = 0; i < active_nr; i++) {
-   const struct cache_entry *ce = active_cache[i];
+   for (i = 0; i < list->nr; i++) {
+   const struct cache_entry *ce = list->entries[i];

-   if (!S_ISGITLINK(ce->ce_mode) ||
-   !is_submodule_initialized(ce->name))
+   if (!is_submodule_initialized(ce->name))
continue;

-   ALLOC_GROW(list->entries, list->nr + 1, list->alloc);
-   list->entries[list->nr++] = ce;
-   while (i + 1 < active_nr &&
-  !strcmp(ce->name, active_cache[i + 1]->name))
-   /*
-* Skip entries with the same name in different stages
-* to make sure an entry is returned only once.
-*/
-   i++;
+   ALLOC_GROW(active_modules.entries,
+  active_modules.nr + 1,
+  active_modules.alloc);
+   active_modules.entries[active_modules.nr++] = ce;
}
+
+   free(list->entries);
+   *list = active_modules;
 }

 static int module_list(int argc, const char **argv, const char *prefix)
@@ -361,7 +356,12 @@ static void init_submodule(const char *path, const char 
*prefix, int quiet)
die(_("No url found for submodule path '%s' in .gitmodules"),
displaypath);

-   /* Set active flag for the submodule being initialized */
+   /*
+* NEEDSWORK: In a multi-working-tree world, this needs to be
+* set in the per-worktree config.
+*
+* Set active flag for the submodule being initialized
+*/
if (!is_submodule_initialized(path)) {
strbuf_reset();
strbuf_addf(, 

[PATCH] http-push: don't check return value of lookup_unknown_object()

[René Scharfe]

This function always returns a reference to an object, creating one if
needed, so remove the unnecessary NULL check.

Signed-off-by: Rene Scharfe 
---
 http-push.c | 8 +++-
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/http-push.c b/http-push.c
index 704b1c837c..f0e3108f71 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1431,11 +1431,9 @@ static void one_remote_ref(const char *refname)
 */
if (repo->can_update_info_refs && !has_object_file(>old_oid)) {
obj = lookup_unknown_object(ref->old_oid.hash);
-   if (obj) {
-   fprintf(stderr, "  fetch %s for %s\n",
-   oid_to_hex(>old_oid), refname);
-   add_fetch_request(obj);
-   }
+   fprintf(stderr, "  fetch %s for %s\n",
+   oid_to_hex(>old_oid), refname);
+   add_fetch_request(obj);
}
 
ref->next = remote_refs;
-- 
2.12.0

[PATCH v5 04/10] submodule sync: use submodule--helper is-active

[Brandon Williams]

Signed-off-by: Brandon Williams 
---
 git-submodule.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 577136148..db94dea3b 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -1089,14 +1089,14 @@ cmd_sync()
while read mode sha1 stage sm_path
do
die_if_unmatched "$mode" "$sha1"
-   name=$(git submodule--helper name "$sm_path")
 
# skip inactive submodules
-   if ! git config "submodule.$name.url" >/dev/null 2>/dev/null
+   if ! git submodule--helper is-active "$sm_path"
then
continue
fi
 
+   name=$(git submodule--helper name "$sm_path")
url=$(git config -f .gitmodules --get submodule."$name".url)
 
# Possibly a url relative to parent
-- 
2.12.0.367.g23dc2f6d3c-goog

[PATCH v5 07/10] submodule init: initialize active submodules

[Brandon Williams]

Teach `submodule init` to initialize submodules which have been
configured to be active by setting 'submodule.active' with a pathspec.

Now if no path arguments are given and 'submodule.active' is configured,
`init` will initialize all submodules which have been configured to be
active.  If no path arguments are given and 'submodule.active' is not
configured, then `init` will retain the old behavior of initializing all
submodules.

This allows users to record more complex patterns as it saves retyping
them whenever you invoke update.

Signed-off-by: Brandon Williams 
---
 Documentation/git-submodule.txt |  4 ++-
 builtin/submodule--helper.c | 30 ++
 t/t7400-submodule-basic.sh  | 57 +
 3 files changed, 90 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index e05d0cdde..74bc6200d 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -129,7 +129,9 @@ init [--] [...]::
repository will be assumed to be upstream.
 +
 Optional  arguments limit which submodules will be initialized.
-If no path is specified, all submodules are initialized.
+If no path is specified and submodule.active has been configured, submodules
+configured to be active will be initialized, otherwise all submodules are
+initialized.
 +
 When present, it will also copy the value of `submodule.$name.update`.
 This command does not alter existing information in .git/config.
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index f38e332c5..65208faa7 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -270,6 +270,29 @@ static int module_list_compute(int argc, const char **argv,
return result;
 }
 
+static void module_list_active(struct module_list *list)
+{
+   int i;
+   struct module_list active_modules = MODULE_LIST_INIT;
+
+   gitmodules_config();
+
+   for (i = 0; i < list->nr; i++) {
+   const struct cache_entry *ce = list->entries[i];
+
+   if (!is_submodule_initialized(ce->name))
+   continue;
+
+   ALLOC_GROW(active_modules.entries,
+  active_modules.nr + 1,
+  active_modules.alloc);
+   active_modules.entries[active_modules.nr++] = ce;
+   }
+
+   free(list->entries);
+   *list = active_modules;
+}
+
 static int module_list(int argc, const char **argv, const char *prefix)
 {
int i;
@@ -420,6 +443,13 @@ static int module_init(int argc, const char **argv, const 
char *prefix)
if (module_list_compute(argc, argv, prefix, , ) < 0)
return 1;
 
+   /*
+* If there are no path args and submodule.active is set then,
+* by default, only initialize 'active' modules.
+*/
+   if (!argc && git_config_get_value_multi("submodule.active"))
+   module_list_active();
+
for (i = 0; i < list.nr; i++)
init_submodule(list.entries[i]->name, prefix, quiet);
 
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index c09ce0d4c..fbbe932d1 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -1130,5 +1130,62 @@ test_expect_success 'submodule helper list is not 
confused by common prefixes' '
test_cmp expect actual
 '
 
+test_expect_success 'setup superproject with submodules' '
+   git init sub1 &&
+   test_commit -C sub1 test &&
+   test_commit -C sub1 test2 &&
+   git init multisuper &&
+   git -C multisuper submodule add ../sub1 sub0 &&
+   git -C multisuper submodule add ../sub1 sub1 &&
+   git -C multisuper submodule add ../sub1 sub2 &&
+   git -C multisuper submodule add ../sub1 sub3 &&
+   git -C multisuper commit -m "add some submodules"
+'
+
+cat >expect <<-EOF
+-sub0
+ sub1 (test2)
+ sub2 (test2)
+ sub3 (test2)
+EOF
+
+test_expect_success 'submodule update --init with a specification' '
+   test_when_finished "rm -rf multisuper_clone" &&
+   pwd=$(pwd) &&
+   git clone file://"$pwd"/multisuper multisuper_clone &&
+   git -C multisuper_clone submodule update --init . ":(exclude)sub0" &&
+   git -C multisuper_clone submodule status |cut -c 1,43- >actual &&
+   test_cmp expect actual
+'
+
+test_expect_success 'submodule update --init with submodule.active set' '
+   test_when_finished "rm -rf multisuper_clone" &&
+   pwd=$(pwd) &&
+   git clone file://"$pwd"/multisuper multisuper_clone &&
+   git -C multisuper_clone config submodule.active "." &&
+   git -C multisuper_clone config --add submodule.active ":(exclude)sub0" 
&&
+   git -C multisuper_clone submodule update --init &&
+   git -C multisuper_clone submodule status |cut -c 1,43- >actual &&
+   test_cmp expect actual
+'
+
+test_expect_success 'submodule update and setting 

[PATCH v5 01/10] submodule--helper: add is-active subcommand

[Brandon Williams]

The definition of which submodules are of interest by the user
is tied to the configuration submodule..url; when it is
set to a non-empty string, it is of interest.  We'd want to be
able to later change this definition, but there are many places
that explicitly check this condition in the scripted Porcelain.

Introduce the "is-active" subcommand to "submodule--helper", so
that the exact definition of what submodule is of interest can
be centrally defined (and changed in later steps).  In a few
patches that follow, this helper is used to replace the explicit
checks of the configuration variable in scripts.

Signed-off-by: Brandon Williams 
---
 builtin/submodule--helper.c| 11 +++
 t/t7413-submodule-is-active.sh | 31 +++
 2 files changed, 42 insertions(+)
 create mode 100755 t/t7413-submodule-is-active.sh

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 15a5430c0..5fe7e23b1 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1127,6 +1127,16 @@ static int absorb_git_dirs(int argc, const char **argv, 
const char *prefix)
return 0;
 }
 
+static int is_active(int argc, const char **argv, const char *prefix)
+{
+   if (argc != 2)
+   die("submodule--helper is-active takes exactly 1 arguments");
+
+   gitmodules_config();
+
+   return !is_submodule_initialized(argv[1]);
+}
+
 #define SUPPORT_SUPER_PREFIX (1<<0)
 
 struct cmd_struct {
@@ -1146,6 +1156,7 @@ static struct cmd_struct commands[] = {
{"init", module_init, SUPPORT_SUPER_PREFIX},
{"remote-branch", resolve_remote_submodule_branch, 0},
{"absorb-git-dirs", absorb_git_dirs, SUPPORT_SUPER_PREFIX},
+   {"is-active", is_active, 0},
 };
 
 int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
diff --git a/t/t7413-submodule-is-active.sh b/t/t7413-submodule-is-active.sh
new file mode 100755
index 0..f18e0c925
--- /dev/null
+++ b/t/t7413-submodule-is-active.sh
@@ -0,0 +1,31 @@
+#!/bin/sh
+
+test_description='Test submodule--helper is-active
+
+This test verifies that `git submodue--helper is-active` correclty identifies
+submodules which are "active" and interesting to the user.
+'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+   git init sub &&
+   test_commit -C sub initial &&
+   git init super &&
+   test_commit -C super initial &&
+   git -C super submodule add ../sub sub1 &&
+   git -C super submodule add ../sub sub2 &&
+   git -C super commit -a -m "add 2 submodules at sub{1,2}"
+'
+
+test_expect_success 'is-active works with urls' '
+   git -C super submodule--helper is-active sub1 &&
+   git -C super submodule--helper is-active sub2 &&
+
+   git -C super config --unset submodule.sub1.URL &&
+   test_must_fail git -C super submodule--helper is-active sub1 &&
+   git -C super config submodule.sub1.URL ../sub &&
+   git -C super submodule--helper is-active sub1
+'
+
+test_done
-- 
2.12.0.367.g23dc2f6d3c-goog

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

[Jonathan Nieder]

Hi,

Stefan Beller wrote:

> If I recall correctly, "worktree" is the feature/command, and
> "working tree" is an instance in the file system, even when you only
> have one working tree.

I'm not sure I agree with this distinction.  "worktree" is just a
short name for "working tree".

Unfortunately gitglossary(7) doesn't make this clear at all --- it
uses the term worktree a few times (and appears to mean "working tree"
when it does --- e.g.

Pathspecs are used on the command line of [...] and many other
commands to limit the scope of operations to some subset of
the tree or worktree.

) but never defines it.

[...]
> --- a/Documentation/git-worktree.txt
> +++ b/Documentation/git-worktree.txt
> @@ -58,10 +58,10 @@ as if `-b $(basename )` was specified.
>  
>  list::
>  
> -List details of each worktree.  The main worktree is listed first, followed 
> by
> -each of the linked worktrees.  The output details include if the worktree is
> -bare, the revision currently checked out, and the branch currently checked 
> out
> -(or 'detached HEAD' if none).
> +List details of each working tree.  The main working tree is listed first,
> +followed by each of the linked working trees.  The output details include if
> +the working tree is bare, the revision currently checked out, and the branch
> +currently checked out (or 'detached HEAD' if none).

The patch itself looks good.

Thanks,
Jonathan

Re: [PATCH] receive-pack: simplify run_update_post_hook()

[Jonathan Nieder]

René Scharfe wrote:

> Instead of counting the arguments to see if there are any and then
> building the full command use a single loop and add the hook command
> just before the first argument.  This reduces duplication and overall
> code size.
>
> Signed-off-by: Rene Scharfe 
> ---
>  builtin/receive-pack.c | 13 +
>  1 file changed, 5 insertions(+), 8 deletions(-)

Reviewed-by: Jonathan Nieder 

Re: [PATCH] grep: fix build with no thread support

[Jeff King]

On Fri, Mar 17, 2017 at 03:42:32PM -0700, Brandon Williams wrote:

> > > I didn't take a close look at it but this would seem to indicate that we
> > > don't worry to much about systems without pthreads support.  Just food
> > > for thought.
> > 
> > Hmm. We used to. What version did you test? Everything passes for me at
> > 0281e487f^ (after that it fails to build). So AFAICT v2.12.0 is the
> > first release which does not work with NO_PTHREADS.
> > 
> > -Peff
> 
> The version I ran tests on was what the master branch was pointing to a
> day or so ago:
> 
> v2.12.0-264-gd6db3f216

Ah, OK. I couldn't get such a recent version to build with NO_PTHREADS,
but I assume you mean after applying your two patches.

v2.11.0 is fine, but v2.12.0 with your patches shows the problem.
Bisecting (and applying the patches when necessary) points to my
46df6906f (execv_dashed_external: wait for child on signal death,
2017-01-06).

-Peff

[PATCH] shortlog: don't set after_subject to an empty string

[René Scharfe]

The string after_subject is added to a strbuf by pp_title_line() if
it's not NULL.  Adding an empty string has the same effect as not
adding anything, but the latter is easier, so don't bother changing
the context member from NULL to "".

Signed-off-by: Rene Scharfe 
---
 builtin/shortlog.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/builtin/shortlog.c b/builtin/shortlog.c
index f78bb4818d..7cff1839fc 100644
--- a/builtin/shortlog.c
+++ b/builtin/shortlog.c
@@ -149,7 +149,6 @@ void shortlog_add_commit(struct shortlog *log, struct 
commit *commit)
ctx.fmt = CMIT_FMT_USERFORMAT;
ctx.abbrev = log->abbrev;
ctx.print_email_subject = 1;
-   ctx.after_subject = "";
ctx.date_mode.type = DATE_NORMAL;
ctx.output_encoding = get_log_output_encoding();
 
-- 
2.12.0

Re: [PATCH] run-command: fix segfault when cleaning forked async process

[Brandon Williams]

On 03/17, Jeff King wrote:
> Callers of the run-command API may mark a child as
> "clean_on_exit"; it gets added to a list and killed when the
> main process dies.  Since commit 46df6906f
> (execv_dashed_external: wait for child on signal death,
> 2017-01-06), we respect an extra "wait_after_clean" flag,
> which we expect to find in the child_process struct.
> 
> When Git is built with NO_PTHREADS, we start "struct
> async" processes by forking rather than spawning a thread.
> The resulting processes get added to the cleanup list but
> they don't have a child_process struct, and the cleanup
> function ends up dereferencing NULL.
> 
> We should notice this case and assume that the processes do
> not need to be waited for (i.e., the same behavior they had
> before 46df6906f).
> 
> Reported-by: Brandon Williams 
> Signed-off-by: Jeff King 
> ---
> This is a regression in v2.12.0, though there is no hurry to get it into
> v2.12.1 unless your grep patches go in, too. Without them you can't
> actually build with NO_PTHREADS anyway.
> 
> However, applied directly on top of 46df6906f (which predates the build
> breakage), you can easily see the test failures with NO_PTHREADS and
> that this fixes them.
> 
>  run-command.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/run-command.c b/run-command.c
> index 5227f78ae..574b81d3e 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -48,7 +48,7 @@ static void cleanup_children(int sig, int in_signal)
>  
>   kill(p->pid, sig);
>  
> - if (p->process->wait_after_clean) {
> + if (p->process && p->process->wait_after_clean) {
>   p->next = children_to_wait_for;
>   children_to_wait_for = p;
>   } else {

Looks good to me! Thanks for tracking that down so quickly.  I'm glad it
was a quick fix.

-- 
Brandon Williams

Re: [PATCH] run-command: fix segfault when cleaning forked async process

[Jonathan Nieder]

Jeff King wrote:

> Reported-by: Brandon Williams 
> Signed-off-by: Jeff King 

Reviewed-by: Jonathan Nieder 

Thanks.

[PATCH v5 08/10] clone: teach --recurse-submodules to optionally take a pathspec

[Brandon Williams]

Teach clone --recurse-submodules to optionally take a pathspec argument
which describes which submodules should be recursively initialized and
cloned.  If no pathspec is provided, --recurse-submodules will
recursively initialize and clone all submodules by using a default
pathspec of ".".  In order to construct more complex pathspecs,
--recurse-submodules can be given multiple times.

This also configures the 'submodule.active' configuration option to be
the given pathspec, such that any future invocation of `git submodule
update` will keep up with the pathspec.

Additionally the switch '--recurse' is removed from the Documentation as
well as marked hidden in the options array, to streamline the options
for submodules.  A simple '--recurse' doesn't convey what is being
recursed, e.g. it could mean directories or trees (c.f. ls-tree) In a
lot of other commands we already have '--recurse-submodules' to mean
recursing into submodules, so advertise this spelling here as the
genuine option.

Signed-off-by: Brandon Williams 
---
 Documentation/git-clone.txt | 14 ++
 builtin/clone.c | 50 -
 t/t7400-submodule-basic.sh  | 68 +
 3 files changed, 120 insertions(+), 12 deletions(-)

diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index 35cc34b2f..30052cce4 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -14,7 +14,7 @@ SYNOPSIS
  [-o ] [-b ] [-u ] [--reference ]
  [--dissociate] [--separate-git-dir ]
  [--depth ] [--[no-]single-branch]
- [--recursive | --recurse-submodules] [--[no-]shallow-submodules]
+ [--recurse-submodules] [--[no-]shallow-submodules]
  [--jobs ] [--]  []
 
 DESCRIPTION
@@ -215,10 +215,14 @@ objects from the source repository into a pack in the 
cloned repository.
branch when `--single-branch` clone was made, no remote-tracking
branch is created.
 
---recursive::
---recurse-submodules::
-   After the clone is created, initialize all submodules within,
-   using their default settings. This is equivalent to running
+--recurse-submodules[=value, 0);
+   else if (arg)
+   string_list_append((struct string_list *)opt->value, arg);
+   else
+   string_list_append((struct string_list *)opt->value,
+  (const char *)opt->defval);
+
+   return 0;
+}
 
 static struct option builtin_clone_options[] = {
OPT__VERBOSITY(_verbosity),
@@ -74,10 +89,13 @@ static struct option builtin_clone_options[] = {
N_("don't use local hardlinks, always copy")),
OPT_BOOL('s', "shared", _shared,
N_("setup as shared repository")),
-   OPT_BOOL(0, "recursive", _recursive,
-   N_("initialize submodules in the clone")),
-   OPT_BOOL(0, "recurse-submodules", _recursive,
-   N_("initialize submodules in the clone")),
+   { OPTION_CALLBACK, 0, "recursive", _recurse_submodules,
+ N_("pathspec"), N_("initialize submodules in the clone"),
+ PARSE_OPT_OPTARG | PARSE_OPT_HIDDEN, 

Re: [PATCH] receive-pack: simplify run_update_post_hook()

[René Scharfe]

Am 17.03.2017 um 23:23 schrieb Jeff King:

On Fri, Mar 17, 2017 at 11:02:13PM +0100, René Scharfe wrote:


Instead of counting the arguments to see if there are any and then
building the full command use a single loop and add the hook command
just before the first argument.  This reduces duplication and overall
code size.


Yeah, I agree one loop is nicer.


-   argv_array_push(, hook);
for (cmd = commands; cmd; cmd = cmd->next) {
if (cmd->error_string || cmd->did_not_exist)
continue;
+   if (!proc.args.argc)
+   argv_array_push(, hook);
argv_array_push(, cmd->ref_name);
}
+   if (!proc.args.argc)
+   return;


It looks at first like the result leaks, because you have to realize
that the push will modify proc.args.argc. I wonder if:

  argv_array_push(, hook);
  for (cmd = commands; cmd; cmd = cmd->next) {
if (!cmd->error_string && !cmd->did_not_exist)
argv_array_push(, cmd->ref_name);
  }

  if (proc.args.argc == 1) {
argv_array_clear();
return;
  }

would be more obvious (at the cost of a pointless malloc in the corner
case. I can live with it either way.


Sure, that's even simpler.  I don't know how often the no-args branch 
would be taken and if the extra allocation would even matter -- that's 
why I tried to avoid it -- but probably the answers are not often and 
not much.  The only test case that hits it is for the deletion of a 
non-existent ref.


René

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

[Stefan Beller]

On Fri, Mar 17, 2017 at 3:55 PM, Jonathan Nieder  wrote:
> Hi,
>
> Stefan Beller wrote:
>
>> If I recall correctly, "worktree" is the feature/command, and
>> "working tree" is an instance in the file system, even when you only
>> have one working tree.
>
> I'm not sure I agree with this distinction.  "worktree" is just a
> short name for "working tree".
>
> Unfortunately gitglossary(7) doesn't make this clear at all --- it
> uses the term worktree a few times (and appears to mean "working tree"
> when it does --- e.g.
>
> Pathspecs are used on the command line of [...] and many other
> commands to limit the scope of operations to some subset of
> the tree or worktree.
>
> ) but never defines it.
>

So maybe it's time to look for volunteers for a cleanup patch. ;)
I tried finding the discussion on worktree vs "working tree"
and did not succeed. :/

Maybe Duy has a better memory there.

Thanks,
Stefan

Re: [PATCH v4 07/10] submodule init: initialize active submodules

[Stefan Beller]

>> Here we also need to have
>>
>>   git_config(submodule_config, NULL);
>>
>> such that is_submodule_initialized works correctly,
>> I would assume?
>
> No I don't think so.  is_submodule_initialized doesn't need them to be
> overlayed, it just needs the .gitmodules mappings.

ok, thanks! I assumed it would need it later on, when e.g. looking at
"submodule.active" from the config.

Thanks,
Stefan

Re: [PATCH v5 00/10] decoupling url and submodule interest

[Stefan Beller]

On Fri, Mar 17, 2017 at 3:37 PM, Brandon Williams  wrote:
> Changes in v5:
>   * Add "NEEDSWORK" comments to indicate where attention is needed once
> per-worktree config is a reality
>   * --no-recurse now works by clearing the string list of paths.
>   * module_list_active() now does post-processing instead of duplicating code.

Skimming these patches, they all look good to me.

Thanks,
Stefan

Re: [PATCH] receive-pack: simplify run_update_post_hook()

[Jeff King]

On Fri, Mar 17, 2017 at 11:02:13PM +0100, René Scharfe wrote:

> Instead of counting the arguments to see if there are any and then
> building the full command use a single loop and add the hook command
> just before the first argument.  This reduces duplication and overall
> code size.

Yeah, I agree one loop is nicer.

> - argv_array_push(, hook);
>   for (cmd = commands; cmd; cmd = cmd->next) {
>   if (cmd->error_string || cmd->did_not_exist)
>   continue;
> + if (!proc.args.argc)
> + argv_array_push(, hook);
>   argv_array_push(, cmd->ref_name);
>   }
> + if (!proc.args.argc)
> + return;

It looks at first like the result leaks, because you have to realize
that the push will modify proc.args.argc. I wonder if:

  argv_array_push(, hook);
  for (cmd = commands; cmd; cmd = cmd->next) {
if (!cmd->error_string && !cmd->did_not_exist)
argv_array_push(, cmd->ref_name);
  }

  if (proc.args.argc == 1) {
argv_array_clear();
return;
  }

would be more obvious (at the cost of a pointless malloc in the corner
case. I can live with it either way.

-Peff

[PATCH] run-command: fix segfault when cleaning forked async process

[Jeff King]

Callers of the run-command API may mark a child as
"clean_on_exit"; it gets added to a list and killed when the
main process dies.  Since commit 46df6906f
(execv_dashed_external: wait for child on signal death,
2017-01-06), we respect an extra "wait_after_clean" flag,
which we expect to find in the child_process struct.

When Git is built with NO_PTHREADS, we start "struct
async" processes by forking rather than spawning a thread.
The resulting processes get added to the cleanup list but
they don't have a child_process struct, and the cleanup
function ends up dereferencing NULL.

We should notice this case and assume that the processes do
not need to be waited for (i.e., the same behavior they had
before 46df6906f).

Reported-by: Brandon Williams 
Signed-off-by: Jeff King 
---
This is a regression in v2.12.0, though there is no hurry to get it into
v2.12.1 unless your grep patches go in, too. Without them you can't
actually build with NO_PTHREADS anyway.

However, applied directly on top of 46df6906f (which predates the build
breakage), you can easily see the test failures with NO_PTHREADS and
that this fixes them.

 run-command.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/run-command.c b/run-command.c
index 5227f78ae..574b81d3e 100644
--- a/run-command.c
+++ b/run-command.c
@@ -48,7 +48,7 @@ static void cleanup_children(int sig, int in_signal)
 
kill(p->pid, sig);
 
-   if (p->process->wait_after_clean) {
+   if (p->process && p->process->wait_after_clean) {
p->next = children_to_wait_for;
children_to_wait_for = p;
} else {
-- 
2.12.0.660.gf842b44fd

[PATCH v5 03/10] submodule sync: skip work for inactive submodules

[Brandon Williams]

Sync does some work determining what URLs should be used for a submodule
but then throws this work away if the submodule isn't active.  Instead
perform the activity check earlier and skip inactive submodule in order
to avoid doing unnecessary work.

Signed-off-by: Brandon Williams 
---
 git-submodule.sh | 38 +-
 1 file changed, 21 insertions(+), 17 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index ab233712d..577136148 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -1090,6 +1090,13 @@ cmd_sync()
do
die_if_unmatched "$mode" "$sha1"
name=$(git submodule--helper name "$sm_path")
+
+   # skip inactive submodules
+   if ! git config "submodule.$name.url" >/dev/null 2>/dev/null
+   then
+   continue
+   fi
+
url=$(git config -f .gitmodules --get submodule."$name".url)
 
# Possibly a url relative to parent
@@ -,27 +1118,24 @@ cmd_sync()
;;
esac
 
-   if git config "submodule.$name.url" >/dev/null 2>/dev/null
+   displaypath=$(git submodule--helper relative-path 
"$prefix$sm_path" "$wt_prefix")
+   say "$(eval_gettext "Synchronizing submodule url for 
'\$displaypath'")"
+   git config submodule."$name".url "$super_config_url"
+
+   if test -e "$sm_path"/.git
then
-   displaypath=$(git submodule--helper relative-path 
"$prefix$sm_path" "$wt_prefix")
-   say "$(eval_gettext "Synchronizing submodule url for 
'\$displaypath'")"
-   git config submodule."$name".url "$super_config_url"
+   (
+   sanitize_submodule_env
+   cd "$sm_path"
+   remote=$(get_default_remote)
+   git config remote."$remote".url "$sub_origin_url"
 
-   if test -e "$sm_path"/.git
+   if test -n "$recursive"
then
-   (
-   sanitize_submodule_env
-   cd "$sm_path"
-   remote=$(get_default_remote)
-   git config remote."$remote".url 
"$sub_origin_url"
-
-   if test -n "$recursive"
-   then
-   prefix="$prefix$sm_path/"
-   eval cmd_sync
-   fi
-   )
+   prefix="$prefix$sm_path/"
+   eval cmd_sync
fi
+   )
fi
done
 }
-- 
2.12.0.367.g23dc2f6d3c-goog

Re: fatal: Could not get current working directory: Permission denied | affected 2.10,2.11,2.12, but not 1.9.5 |

[René Scharfe]

Am 17.03.2017 um 23:29 schrieb Jeff King:

On Fri, Mar 17, 2017 at 10:07:18PM +0100, René Scharfe wrote:


  As  an extension to the POSIX.1-2001 standard, Linux (libc4, libc5,
glibc) getcwd()
  allocates the buffer dynamically using malloc(3) if buf is NULL.  In
this case, the
  allocated buffer has the length size unless size is zero, when buf
is allocated as big
  as necessary.  The caller should free(3) the returned buffer.

This sounds specific to Linux (though I am reading Linux man pages,
which claim this; Also it seems I might have misread it as it also states
"The pathname is returned as the function result and via the
argument buf, if present.").


I'm only interested in FreeBSD for now, as that's the platform Zenobiusz
reported the issue on and I haven't been able to reproduce it, so this is
still a bit exploratory, but hopefully getting closer.  This extension is
used in the first version of pwd(1) in FreeBSD's repo, comitted 1994-05-26,
so it was supported there basically forever.

The oldest version I found that's using the extention is NetBSD's pwd(1),
which was committed 1993-03-21 and carries a SCCS timestamp of 1991-02-20.
Visual Studio .NET 2003 supports it as well.


My big question is what happens on other platforms when they see a NULL
(but 0-sized) buffer. Any reasonable implementation would just return
ERANGE and we'd fall back to the existing code. But we often have to deal with 
unreasonable ones.

So do we need a "my OS understands getcwd(NULL)" build knob?


POSIX specifies the behavior in this case as unspecified.  An 
implementation could offer a different extension, and e.g. return a 
pointer to a static buffer.  So the answer would probably be "yes" if 
this was a patch meant for public consumption.  For now it's only 
compatible with BSDs, Windows, MacOSX, Linux. :)


René

[PATCH v5 02/10] submodule status: use submodule--helper is-active

[Brandon Williams]

Signed-off-by: Brandon Williams 
---
 git-submodule.sh | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 136e26a2c..ab233712d 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -1010,14 +1010,13 @@ cmd_status()
do
die_if_unmatched "$mode" "$sha1"
name=$(git submodule--helper name "$sm_path") || exit
-   url=$(git config submodule."$name".url)
displaypath=$(git submodule--helper relative-path 
"$prefix$sm_path" "$wt_prefix")
if test "$stage" = U
then
say "U$sha1 $displaypath"
continue
fi
-   if test -z "$url" ||
+   if ! git submodule--helper is-active "$sm_path" ||
{
! test -d "$sm_path"/.git &&
! test -f "$sm_path"/.git
-- 
2.12.0.367.g23dc2f6d3c-goog

Re: [PATCH] grep: fix build with no thread support

[Brandon Williams]

On 03/17, Jeff King wrote:
> On Fri, Mar 17, 2017 at 03:42:32PM -0700, Brandon Williams wrote:
> 
> > > > I didn't take a close look at it but this would seem to indicate that we
> > > > don't worry to much about systems without pthreads support.  Just food
> > > > for thought.
> > > 
> > > Hmm. We used to. What version did you test? Everything passes for me at
> > > 0281e487f^ (after that it fails to build). So AFAICT v2.12.0 is the
> > > first release which does not work with NO_PTHREADS.
> > > 
> > > -Peff
> > 
> > The version I ran tests on was what the master branch was pointing to a
> > day or so ago:
> > 
> > v2.12.0-264-gd6db3f216
> 
> Ah, OK. I couldn't get such a recent version to build with NO_PTHREADS,
> but I assume you mean after applying your two patches.
> 

Oh yes, sorry I should have mentioned that was with my patches applied.
My bad! :D

> v2.11.0 is fine, but v2.12.0 with your patches shows the problem.
> Bisecting (and applying the patches when necessary) points to my
> 46df6906f (execv_dashed_external: wait for child on signal death,
> 2017-01-06).
> 
> -Peff

-- 
Brandon Williams

Re: [PATCH 2/2] grep: fix builds with with no thread support

[Jonathan Nieder]

Brandon Williams wrote:

> Commit 0281e487fd91 ("grep: optionally recurse into submodules")
> added functions grep_submodule() and grep_submodule_launch() which
> uses "struct work_item" which is defined only when thread support

nit: which use (s/uses/use/)

> is available.
[...]
> Reported-by: Rahul Bedarkar 
> Signed-off-by: Brandon Williams 
> ---
>  builtin/grep.c | 21 +
>  1 file changed, 9 insertions(+), 12 deletions(-)

With or without that commit message tweak,
Reviewed-by: Jonathan Nieder 

Thanks.

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

[Stefan Beller]

If I recall correctly, "worktree" is the feature/command, and
"working tree" is an instance in the file system, even when you only
have one working tree.

Using that mental model, the documentation for 'git worktree list'
clearly talks about working trees, so fix it.

Reflow the lines as well.

Signed-off-by: Stefan Beller 
---

 Duy, this is my instant-make-a-apatch-to-fix-the-world reaction
 in reply to Jonathan.
 
 Thanks,
 Stefan

 Documentation/git-worktree.txt | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 553cf8413f..e080007ed0 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -58,10 +58,10 @@ as if `-b $(basename )` was specified.
 
 list::
 
-List details of each worktree.  The main worktree is listed first, followed by
-each of the linked worktrees.  The output details include if the worktree is
-bare, the revision currently checked out, and the branch currently checked out
-(or 'detached HEAD' if none).
+List details of each working tree.  The main working tree is listed first,
+followed by each of the linked working trees.  The output details include if
+the working tree is bare, the revision currently checked out, and the branch
+currently checked out (or 'detached HEAD' if none).
 
 lock::
 
-- 
2.12.0.306.g4a9b9b32d4.dirty

[PATCH v5 09/10] submodule--helper init: set submodule..active

[Brandon Williams]

When initializing a submodule set the submodule..active config to
true if the module hasn't already been configured to be active by some
other means (e.g. a pathspec set in submodule.active).

Signed-off-by: Brandon Williams 
---
 builtin/submodule--helper.c | 12 
 t/t7400-submodule-basic.sh  | 11 +++
 2 files changed, 23 insertions(+)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 65208faa7..7700d8948 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -357,6 +357,18 @@ static void init_submodule(const char *path, const char 
*prefix, int quiet)
displaypath);
 
/*
+* NEEDSWORK: In a multi-working-tree world, this needs to be
+* set in the per-worktree config.
+*
+* Set active flag for the submodule being initialized
+*/
+   if (!is_submodule_initialized(path)) {
+   strbuf_reset();
+   strbuf_addf(, "submodule.%s.active", sub->name);
+   git_config_set_gently(sb.buf, "true");
+   }
+
+   /*
 * Copy url setting when it is not set yet.
 * To look up the url in .git/config, we must not fall back to
 * .gitmodules, so look it up directly.
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 3af1c00ff..cf77a3a35 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -1256,4 +1256,15 @@ test_expect_success 'clone and subsequent updates 
correctly auto-initialize subm
test_cmp expect2 actual
 '
 
+test_expect_success 'init properly sets the config' '
+   test_when_finished "rm -rf multisuper_clone" &&
+   git clone --recurse-submodules="." \
+ --recurse-submodules=":(exclude)sub0" \
+ multisuper multisuper_clone &&
+
+   git -C multisuper_clone submodule init -- sub0 sub1 &&
+   git -C multisuper_clone config --get submodule.sub0.active &&
+   test_must_fail git -C multisuper_clone config --get 
submodule.sub1.active
+'
+
 test_done
-- 
2.12.0.367.g23dc2f6d3c-goog

[PATCH v5 05/10] submodule--helper clone: check for configured submodules using helper

[Brandon Williams]

Use the 'is_submodule_initialized()' helper to check for configured
submodules instead of manually checking for the submodule's URL in the
config.

Signed-off-by: Brandon Williams 
---
 builtin/submodule--helper.c | 15 +++
 1 file changed, 3 insertions(+), 12 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 5fe7e23b1..f38e332c5 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -759,7 +759,6 @@ static int prepare_to_clone_next_submodule(const struct 
cache_entry *ce,
struct strbuf displaypath_sb = STRBUF_INIT;
struct strbuf sb = STRBUF_INIT;
const char *displaypath = NULL;
-   char *url = NULL;
int needs_cloning = 0;
 
if (ce_stage(ce)) {
@@ -793,15 +792,8 @@ static int prepare_to_clone_next_submodule(const struct 
cache_entry *ce,
goto cleanup;
}
 
-   /*
-* Looking up the url in .git/config.
-* We must not fall back to .gitmodules as we only want
-* to process configured submodules.
-*/
-   strbuf_reset();
-   strbuf_addf(, "submodule.%s.url", sub->name);
-   git_config_get_string(sb.buf, );
-   if (!url) {
+   /* Check if the submodule has been initialized. */
+   if (!is_submodule_initialized(ce->name)) {
next_submodule_warn_missing(suc, out, displaypath);
goto cleanup;
}
@@ -835,7 +827,7 @@ static int prepare_to_clone_next_submodule(const struct 
cache_entry *ce,
argv_array_push(>args, "--depth=1");
argv_array_pushl(>args, "--path", sub->path, NULL);
argv_array_pushl(>args, "--name", sub->name, NULL);
-   argv_array_pushl(>args, "--url", url, NULL);
+   argv_array_pushl(>args, "--url", sub->url, NULL);
if (suc->references.nr) {
struct string_list_item *item;
for_each_string_list_item(item, >references)
@@ -845,7 +837,6 @@ static int prepare_to_clone_next_submodule(const struct 
cache_entry *ce,
argv_array_push(>args, suc->depth);
 
 cleanup:
-   free(url);
strbuf_reset(_sb);
strbuf_reset();
 
-- 
2.12.0.367.g23dc2f6d3c-goog

[PATCH v5 06/10] submodule: decouple url and submodule interest

[Brandon Williams]

Currently the submodule..url config option is used to determine if
a given submodule is of interest to the user.  This ends up being
cumbersome in a world where we want to have different submodules checked
out in different worktrees or a more generalized mechanism to select
which submodules are of interest.

In a future with worktree support for submodules, there will be multiple
working trees, each of which may only need a subset of the submodules
checked out.  The URL (which is where the submodule repository can be
obtained) should not differ between different working trees.

It may also be convenient for users to more easily specify groups of
submodules they are interested in as opposed to running "git submodule
init " on each submodule they want checked out in their working
tree.

To this end two config options are introduced, submodule.active and
submodule..active.  The submodule.active config holds a pathspec
that specifies which submodules should exist in the working tree.  The
submodule..active config is a boolean flag used to indicate if
that particular submodule should exist in the working tree.

Its important to note that submodule.active functions differently than
the other configuration options since it takes a pathspec.  This allows
users to adopt at least two new workflows:

  1. Submodules can be grouped with a leading directory, such that a
 pathspec e.g. 'lib/' would cover all library-ish modules to allow
 those who are interested in library-ish modules to set
 "submodule.active = lib/" just once to say any and all modules in
 'lib/' are interesting.

  2. Once the pathspec-attribute feature is invented, users can label
 submodules with attributes to group them, so that a broad pathspec
 with attribute requirements, e.g. ':(attr:lib)', can be used to say
 any and all modules with the 'lib' attribute are interesting.
 Since the .gitattributes file, just like the .gitmodules file, is
 tracked by the superproject, when a submodule moves in the
 superproject tree, the project can adjust which path gets the
 attribute in .gitattributes, just like it can adjust which path has
 the submodule in .gitmodules.

Neither of these two additional configuration options solve the problem
of wanting different submodules checked out in different worktrees
because multiple worktrees share .git/config.  Only once per-worktree
configurations become a reality can this be solved, but this is a
necessary preparatory step for that future.

Given these multiple ways to check if a submodule is of interest, the
more fine-grained submodule..active option has the highest order
of precedence followed by the pathspec check against submodule.active.
To ensure backwards compatibility, if neither of these options are set,
git falls back to checking the submodule..url option to determine
if a submodule is interesting.

Signed-off-by: Brandon Williams 
---
 Documentation/config.txt   | 15 ++--
 submodule.c| 50 --
 t/t7413-submodule-is-active.sh | 55 ++
 3 files changed, 110 insertions(+), 10 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 5e5c2ae5f..d2d79b9d4 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2920,8 +2920,9 @@ submodule..url::
The URL for a submodule. This variable is copied from the .gitmodules
file to the git config via 'git submodule init'. The user can change
the configured URL before obtaining the submodule via 'git submodule
-   update'. After obtaining the submodule, the presence of this variable
-   is used as a sign whether the submodule is of interest to git commands.
+   update'. If neither submodule..active or submodule.active are
+   set, the presence of this variable is used as a fallback to indicate
+   whether the submodule is of interest to git commands.
See linkgit:git-submodule[1] and linkgit:gitmodules[5] for details.
 
 submodule..update::
@@ -2959,6 +2960,16 @@ submodule..ignore::
"--ignore-submodules" option. The 'git submodule' commands are not
affected by this setting.
 
+submodule..active::
+   Boolean value indicating if the submodule is of interest to git
+   commands.  This config option takes precedence over the
+   submodule.active config option.
+
+submodule.active::
+   A repeated field which contains a pathspec used to match against a
+   submodule's path to determine if the submodule is of interest to git
+   commands.
+
 submodule.fetchJobs::
Specifies how many submodules are fetched/cloned at the same time.
A positive integer allows up to that number of submodules fetched
diff --git a/submodule.c b/submodule.c
index 0a2831d84..ad2779ee7 100644
--- a/submodule.c
+++ b/submodule.c
@@ -212,25 +212,59 @@ void gitmodules_config_sha1(const 

Re: [PATCH 2/2] name-rev: favor describing with tags and use committer date to tiebreak

[Junio C Hamano]

Junio C Hamano  writes:

> Lars Schneider  writes:
> ...
>> Following your explanation this patch looks good to me and this fixes the
>> test failure. TBH I never thought about the difference of these commands
>> before. "rev" and "ref" sound so similar although they denote completely 
>> different things.
>
> Thanks for testing.
> ...
> Hence, I would really prefer not to commit mine myself.  I'd rather
> see somebody from git-p4 circle to come up with a version that is
> more in line with the way things are done in the existing code and
> send a tested version for me to apply.

Tentatively I queued my hack together with the name-rev thing on
'pu', and Travis seems to be OK with the result.  

It would be nice if we can "fix" the use of "name-rev HEAD" in "git
p4" sooner rather than later.  If the code truly uses it to figure
out what branch we are currently on and acts on it, as the name of
that function suggests, it may be easy to construct a testcase where
the code without the fix does a wrong thing (e.g. have two branches
pointing at the same commit, and tell "git p4" to act on the one
that sorts later in the "git branch --list" output; the command
would act as if it were working on the other branch), and shows that
the patch fixes that problem.  That would be a bug worth fixing
quickly regardless of the "name-rev" change michael wanted to make.

Thanks.

Re: [PATCH] grep: fix build with no thread support

[Brandon Williams]

On 03/17, Jeff King wrote:
> On Fri, Mar 17, 2017 at 11:47:01AM -0700, Brandon Williams wrote:
> 
> > While taking a look at this bug I discovered that the test suite doesn't
> > pass 100% of the test when compiled with the NO_PTHREADS option. The
> > following tests seem to be failing:
> > 
> > t1060-object-corruption.sh   (Wstat: 256 Tests: 13 
> > Failed: 3)
> >   Failed tests:  7-9
> >   Non-zero exit status: 1
> > t5306-pack-nobase.sh (Wstat: 256 Tests: 4 
> > Failed: 1)
> >   Failed test:  4
> >   Non-zero exit status: 1
> > t5504-fetch-receive-strict.sh(Wstat: 256 Tests: 12 
> > Failed: 2)
> >   Failed tests:  4-5
> >   Non-zero exit status: 1
> > t5530-upload-pack-error.sh   (Wstat: 256 Tests: 10 
> > Failed: 1)
> >   Failed test:  10
> >   Non-zero exit status: 1
> > 
> > I didn't take a close look at it but this would seem to indicate that we
> > don't worry to much about systems without pthreads support.  Just food
> > for thought.
> 
> Hmm. We used to. What version did you test? Everything passes for me at
> 0281e487f^ (after that it fails to build). So AFAICT v2.12.0 is the
> first release which does not work with NO_PTHREADS.
> 
> -Peff

The version I ran tests on was what the master branch was pointing to a
day or so ago:

v2.12.0-264-gd6db3f216

-- 
Brandon Williams

Re: [PATCH] grep: fix build with no thread support

[Jeff King]

On Fri, Mar 17, 2017 at 11:47:01AM -0700, Brandon Williams wrote:

> While taking a look at this bug I discovered that the test suite doesn't
> pass 100% of the test when compiled with the NO_PTHREADS option. The
> following tests seem to be failing:
> 
> t1060-object-corruption.sh   (Wstat: 256 Tests: 13 
> Failed: 3)
>   Failed tests:  7-9
>   Non-zero exit status: 1
> t5306-pack-nobase.sh (Wstat: 256 Tests: 4 Failed: 
> 1)
>   Failed test:  4
>   Non-zero exit status: 1
> t5504-fetch-receive-strict.sh(Wstat: 256 Tests: 12 
> Failed: 2)
>   Failed tests:  4-5
>   Non-zero exit status: 1
> t5530-upload-pack-error.sh   (Wstat: 256 Tests: 10 
> Failed: 1)
>   Failed test:  10
>   Non-zero exit status: 1
> 
> I didn't take a close look at it but this would seem to indicate that we
> don't worry to much about systems without pthreads support.  Just food
> for thought.

Hmm. We used to. What version did you test? Everything passes for me at
0281e487f^ (after that it fails to build). So AFAICT v2.12.0 is the
first release which does not work with NO_PTHREADS.

-Peff

Re: Bug with .gitignore and branch switching

[Junio C Hamano]

Jonathan Nieder  writes:

> The most recent example I can find is 2010:
> http://public-inbox.org/git/4c6a1c5b.4030...@workspacewhiz.com/.
>
> It also came up in 2007:
> http://public-inbox.org/git/c0e9f681e68d48eb8989022d11fee...@ntdev.corp.microsoft.com/
> Earlier in that year it even made the "What's not in 1.5.2" list.
> http://public-inbox.org/git/11793556383977-git-send-email-jun...@cox.net/
>
> Perhaps those references could be a useful starting point for an
> interested person's thinking.

Thanks for links.  It seems that my thinking back in 1.5.3 timeperiod
was to introduce "precious" attribute.

I noticed that among the four-message "What's not in 1.5.2" series,
3/4 has a large discussion that may be relevant to Brandon's
"submodule is-active" thing.

Re: fatal: Could not get current working directory: Permission denied | affected 2.10,2.11,2.12, but not 1.9.5 |

[Jeff King]

On Fri, Mar 17, 2017 at 10:07:18PM +0100, René Scharfe wrote:

> >   As  an extension to the POSIX.1-2001 standard, Linux (libc4, libc5,
> > glibc) getcwd()
> >   allocates the buffer dynamically using malloc(3) if buf is NULL.  In
> > this case, the
> >   allocated buffer has the length size unless size is zero, when buf
> > is allocated as big
> >   as necessary.  The caller should free(3) the returned buffer.
> > 
> > This sounds specific to Linux (though I am reading Linux man pages,
> > which claim this; Also it seems I might have misread it as it also states
> > "The pathname is returned as the function result and via the
> > argument buf, if present.").
> 
> I'm only interested in FreeBSD for now, as that's the platform Zenobiusz
> reported the issue on and I haven't been able to reproduce it, so this is
> still a bit exploratory, but hopefully getting closer.  This extension is
> used in the first version of pwd(1) in FreeBSD's repo, comitted 1994-05-26,
> so it was supported there basically forever.
> 
> The oldest version I found that's using the extention is NetBSD's pwd(1),
> which was committed 1993-03-21 and carries a SCCS timestamp of 1991-02-20.
> Visual Studio .NET 2003 supports it as well.

My big question is what happens on other platforms when they see a NULL
(but 0-sized) buffer. Any reasonable implementation would just return
ERANGE and we'd fall back to the existing code. But we often have to deal with 
unreasonable ones.

So do we need a "my OS understands getcwd(NULL)" build knob?

-Peff

Re: [PATCH v4 07/10] submodule init: initialize active submodules

[Brandon Williams]

On 03/17, Stefan Beller wrote:
> On Thu, Mar 16, 2017 at 3:29 PM, Brandon Williams  wrote:
> > Teach `submodule init` to initialize submodules which have been
> > configured to be active by setting 'submodule.active' with a pathspec.
> >
> > Now if no path arguments are given and 'submodule.active' is configured,
> > `init` will initialize all submodules which have been configured to be
> > active.  If no path arguments are given and 'submodule.active' is not
> > configured, then `init` will retain the old behavior of initializing all
> > submodules.
> >
> > This allows users to record more complex patterns as it saves retyping
> > them whenever you invoke update.
> >
> > Signed-off-by: Brandon Williams 
> > ---
> 
> 
> > @@ -417,7 +445,13 @@ static int module_init(int argc, const char **argv, 
> > const char *prefix)
> > argc = parse_options(argc, argv, prefix, module_init_options,
> >  git_submodule_helper_usage, 0);
> >
> > -   if (module_list_compute(argc, argv, prefix, , ) < 0)
> > +   /*
> > +* If there are no path args and submodule.active is set then,
> > +* by default, only initialize 'active' modules.
> > +*/
> > +   if (!argc && git_config_get_value_multi("submodule.active"))
> > +   module_list_active();
> > +   else if (module_list_compute(argc, argv, prefix, , ) 
> > < 0)
> > return 1;
> 
> I would rather reuse module_list_compute and then post-process the list
> to filter out inactive submodules iff "submodule.active" is set as that seems
> cleaner and performance is not a pressing issue here?

Ok, Can do.  Shouldn't be very hard to do that.

> 
> >
> > +static void module_list_active(struct module_list *list)
> > +{
> > +   int i;
> > +
> > +   if (read_cache() < 0)
> > +   die(_("index file corrupt"));
> > +
> > +   gitmodules_config();
> 
> Here we also need to have
> 
>   git_config(submodule_config, NULL);
> 
> such that is_submodule_initialized works correctly,
> I would assume?

No I don't think so.  is_submodule_initialized doesn't need them to be
overlayed, it just needs the .gitmodules mappings.

> 
> > +
> > +   for (i = 0; i < active_nr; i++) {
> > +   const struct cache_entry *ce = active_cache[i];
> > +
> > +   if (!S_ISGITLINK(ce->ce_mode) ||
> > +   !is_submodule_initialized(ce->name))
> > +   continue;
> > +
> > +   ALLOC_GROW(list->entries, list->nr + 1, list->alloc);
> > +   list->entries[list->nr++] = ce;
> > +   while (i + 1 < active_nr &&
> > +  !strcmp(ce->name, active_cache[i + 1]->name))
> > +   /*
> > +* Skip entries with the same name in different 
> > stages
> > +* to make sure an entry is returned only once.
> > +*/
> > +   i++;
> > +   }
> > +}

-- 
Brandon Williams

Re: [PATCHv4] rev-parse: add --show-superproject-working-tree

[Jonathan Nieder]

Hi,

Stefan Beller wrote:

> Add the flag --show-superproject-working-tree to git-rev-parse
> to make it easy to find out if there is a superproject. When no
> superproject exists, the output will be empty.

I'm a little late for this, but think it's better to comment late than
never.

After this patch, git rev-parse has two options relating to the working
tree:

git rev-parse --is-inside-work-tree
git rev-parse --show-superproject-working-tree

Is this inconsistency necessary?  Now that there is a "git worktree"
command that allows making a second working tree for the same
repository, I would have thought we had standardized on work tree as
the name in UI.

Thoughts?
Jonathan

Re: [PATCH] revision: remove declaration of path_name()

[Jeff King]

On Fri, Mar 17, 2017 at 11:15:03PM +0100, René Scharfe wrote:

> The definition of path_name() was removed by 2824e1841 (list-objects:
> pass full pathname to callbacks); remove its declaration as well.

Yep, this should have been done back then. Thanks.

-Peff

Re: Bug with .gitignore and branch switching

[Stefan Beller]

On Fri, Mar 17, 2017 at 2:23 PM, Junio C Hamano  wrote:
> We've discussed the lack of "untracked but precious" class a few
> times on the list in the past, but I do not recall the topic came up
> in the recent past.  It perhaps is because nobody found that class
> useful enough so far.

My gut reaction on reading the bug report was that the root cause is
git-checkout doing the wrong thing by default. (cf. Git-Merge-2017,
"What’s Wrong With Git?", I am not sure if the video is yet available)

One argument in that talk was that Git promises to do "work on multiple
branches in parallel (context-switched, single threaded)", and git-checkout
is the apparent command to switch to another context (branch).
However by putting away only tracked content, we miss
doing a proper context switch for untracked and ignored files.

That partial switch has advantages in the typical use case, e.g.
* compiled objects in the worktree may not need to be recompiled.
* no need to do work for the untracked files (e.g. move to a special
  location).

Both these reasons argue for performance, instead of "correctness"
in the sense of "easy-to-understand commands for top level principles".

And in that talk the presenter concluded that git-stash was only invented
to circumvent these "correctness" problems, such that if git-checkout
were to also (de)populate the untracked and ignored files on branch
switch we would not need git-stash, because git-checkout did it for you
already. And by the omission of git-stash and an apparent
easier-to-understand git-checkout the whole git suite would become
easier for users.

I further conclude that when git-checkout were to behave "correct" as
outlined above, then this class of bug reports would not occur.

Just food for thought.

Thanks,
Stefan

[PATCH] revision: remove declaration of path_name()

[René Scharfe]

The definition of path_name() was removed by 2824e1841 (list-objects:
pass full pathname to callbacks); remove its declaration as well.

Signed-off-by: Rene Scharfe 
---
 revision.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/revision.h b/revision.h
index 9fac1a607d..14886ec92b 100644
--- a/revision.h
+++ b/revision.h
@@ -259,8 +259,6 @@ extern void put_revision_mark(const struct rev_info *revs,
 extern void mark_parents_uninteresting(struct commit *commit);
 extern void mark_tree_uninteresting(struct tree *tree);
 
-char *path_name(struct strbuf *path, const char *name);
-
 extern void show_object_with_name(FILE *, struct object *, const char *);
 
 extern void add_pending_object(struct rev_info *revs,
-- 
2.12.0

Re: Did you get my several email?

[Bar. Nasir Mehmood Raja]

Attention,
I have tried to contact you severally without success, I hope you get my 
message this time and respond immediately regarding the money and my lucrative 
proposal.
I would send you a detailed email as soon as I receive your timely response, I 
have not contacted you by mistake so do not be surprised and get back to me 
ASAP.
Regards,
Bar. Nasir Mehmood Raja

[PATCH] receive-pack: simplify run_update_post_hook()

[René Scharfe]

Instead of counting the arguments to see if there are any and then
building the full command use a single loop and add the hook command
just before the first argument.  This reduces duplication and overall
code size.

Signed-off-by: Rene Scharfe 
---
 builtin/receive-pack.c | 13 +
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 83492af05f..fb2a090a0c 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1128,25 +1128,22 @@ static const char *update(struct command *cmd, struct 
shallow_info *si)
 static void run_update_post_hook(struct command *commands)
 {
struct command *cmd;
-   int argc;
struct child_process proc = CHILD_PROCESS_INIT;
const char *hook;
 
hook = find_hook("post-update");
-   for (argc = 0, cmd = commands; cmd; cmd = cmd->next) {
-   if (cmd->error_string || cmd->did_not_exist)
-   continue;
-   argc++;
-   }
-   if (!argc || !hook)
+   if (!hook)
return;
 
-   argv_array_push(, hook);
for (cmd = commands; cmd; cmd = cmd->next) {
if (cmd->error_string || cmd->did_not_exist)
continue;
+   if (!proc.args.argc)
+   argv_array_push(, hook);
argv_array_push(, cmd->ref_name);
}
+   if (!proc.args.argc)
+   return;
 
proc.no_stdin = 1;
proc.stdout_to_stderr = 1;
-- 
2.12.0

Re: Bug with .gitignore and branch switching

[Jonathan Nieder]

Junio C Hamano wrote:

> There is no "untracked but precious" vs "untracked and expendable"
> difference in the current system.  An untracked file that matches
> patterns listed in .gitignore is treated as the latter.
[...]
> We've discussed the lack of "untracked but precious" class a few
> times on the list in the past, but I do not recall the topic came up
> in the recent past.  It perhaps is because nobody found that class
> useful enough so far.

The most recent example I can find is 2010:
http://public-inbox.org/git/4c6a1c5b.4030...@workspacewhiz.com/.

It also came up in 2007:
http://public-inbox.org/git/c0e9f681e68d48eb8989022d11fee...@ntdev.corp.microsoft.com/
Earlier in that year it even made the "What's not in 1.5.2" list.
http://public-inbox.org/git/11793556383977-git-send-email-jun...@cox.net/

Perhaps those references could be a useful starting point for an
interested person's thinking.

Jonathan

Re: [PATCH v4 09/10] submodule--helper init: set submodule..active

[Brandon Williams]

On 03/17, Stefan Beller wrote:
> On Thu, Mar 16, 2017 at 3:29 PM, Brandon Williams  wrote:
> > When initializing a submodule set the submodule..active config to
> > true if the module hasn't already been configured to be active by some
> > other means (e.g. a pathspec set in submodule.active).
> >
> > Signed-off-by: Brandon Williams 
> > ---
> >  builtin/submodule--helper.c |  7 +++
> >  t/t7400-submodule-basic.sh  | 11 +++
> >  2 files changed, 18 insertions(+)
> >
> > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> > index e95738b42..a574596cb 100644
> > --- a/builtin/submodule--helper.c
> > +++ b/builtin/submodule--helper.c
> > @@ -361,6 +361,13 @@ static void init_submodule(const char *path, const 
> > char *prefix, int quiet)
> > die(_("No url found for submodule path '%s' in 
> > .gitmodules"),
> > displaypath);
> >
> > +   /* Set active flag for the submodule being initialized */
> > +   if (!is_submodule_initialized(path)) {
> > +   strbuf_reset();
> > +   strbuf_addf(, "submodule.%s.active", sub->name);
> 
> In case a reroll is needed, you could mark this location with
> 
> /*
>  * NEEDSWORK: in a multi-working-tree world we need to set
>  * this per-worktree here.
>  */

I'll add this here and where this happens in clone.

-- 
Brandon Williams

Re: Bug with .gitignore and branch switching

[Jonathan Nieder]

Hi Nevada,

Nevada Sanchez wrote:

> # Commit a file that will end up in .gitignore
> echo 'original settings' > mine.conf
> git add mine.conf
> git commit -m "Unknowingly committed my settings."
>
> echo '*.conf' > .gitignore
> git add .gitignore
> git commit -m "Users shouldn't commit their settings"

Naming a file in .gitignore tells git that you do not want to track it
and are giving git permission to write over it.  This commonly happens
when people check in build products.  For example:

git rm -f my-build-product
echo /my-build-product >>.gitignore
git commit -m "Remove generated my-build-product file"
make my-build-product

git checkout HEAD^

Without that rule, this 'git checkout' command would fail.

That said, there are some cases (e.g. the .conf file case you mention)
where a person would want git not to track a file but do not want to
give git permission to write over it.  As you've seen, .gitignore does
not work well for this. :/

Ideas for next steps:

 1. The gitignore(5) manpage does not do a good job of emphasizing
that files named there are not precious and can be overwritten by
git.  Do you have ideas for wording that would help with that?
This would be especially welcome if you can phrase them in the
form of a patch against Documentation/gitignore.txt.

 2. Occasionally people have mentioned the idea of a .gitprecious file
listing precious files that git should not track and not overwrite
(e.g., keys and other configuration files, IDE state, or metadata
for another version control system being used in parallel).  Would
you be interested in working on that?

Thanks and hope that helps,
Jonathan

Re: Bug with .gitignore and branch switching

[Junio C Hamano]

Nevada Sanchez  writes:

> Here's an easy to reproduce bug. It's the only example I know of where
> git legitimately loses data in a way that is unrecoverable,
> unexpected, and without warning.

This is an example of a user explicitly telling git to discard data
and git performing as it is told.

There is no "untracked but precious" vs "untracked and expendable"
difference in the current system.  An untracked file that matches
patterns listed in .gitignore is treated as the latter.

When you have an untracked file that .gitignore knows about in the
working tree while you are on "feature", if switching to another
branch requires to remove that file, the content there is deemed
expendable, because the user said so by listing it in .gitignore.

We've discussed the lack of "untracked but precious" class a few
times on the list in the past, but I do not recall the topic came up
in the recent past.  It perhaps is because nobody found that class
useful enough so far.

What's cooking in git.git (Mar 2017, #07; Fri, 17)

[Junio C Hamano]

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

Quite a few topics have graduated to 'master' this week.  We now
have +300 non-merge commits spanning +70 topics since v2.12.0.

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

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

--
[Graduated to "master"]

* bc/object-id (2017-02-22) 19 commits
  (merged to 'next' on 2017-03-14 at 0b3ec5a05e)
 + wt-status: convert to struct object_id
 + builtin/merge-base: convert to struct object_id
 + Convert object iteration callbacks to struct object_id
 + sha1_file: introduce an nth_packed_object_oid function
 + refs: simplify parsing of reflog entries
 + refs: convert each_reflog_ent_fn to struct object_id
 + reflog-walk: convert struct reflog_info to struct object_id
 + builtin/replace: convert to struct object_id
 + Convert remaining callers of resolve_refdup to object_id
 + builtin/merge: convert to struct object_id
 + builtin/clone: convert to struct object_id
 + builtin/branch: convert to struct object_id
 + builtin/grep: convert to struct object_id
 + builtin/fmt-merge-message: convert to struct object_id
 + builtin/fast-export: convert to struct object_id
 + builtin/describe: convert to struct object_id
 + builtin/diff-tree: convert to struct object_id
 + builtin/commit: convert to struct object_id
 + hex: introduce parse_oid_hex

 "uchar [40]" to "struct object_id" conversion continues.


* bc/sha1-header-selection-with-cpp-macros (2017-03-15) 1 commit
  (merged to 'next' on 2017-03-15 at 71c3a4f4ba)
 + hash.h: move SHA-1 implementation selection into a header file
 (this branch is used by jk/sha1dc.)

 Our source code has used the SHA1_HEADER cpp macro after "#include"
 in the C code to switch among the SHA-1 implementations. Instead,
 list the exact header file names and switch among implementations
 using "#ifdef BLK_SHA1/#include "block-sha1/sha1.h"/.../#endif";
 this helps some IDE tools.


* bw/attr-pathspec (2017-03-13) 2 commits
  (merged to 'next' on 2017-03-14 at 3af5d6c1fc)
 + pathspec: allow escaped query values
 + pathspec: allow querying for attributes

 The pathspec mechanism learned to further limit the paths that
 match the pattern to those that have specified attributes attached
 via the gitattributes mechanism.


* cc/split-index-config (2017-03-06) 22 commits
  (merged to 'next' on 2017-03-12 at 53cdc2016d)
 + Documentation/git-update-index: explain splitIndex.*
 + Documentation/config: add splitIndex.sharedIndexExpire
 + read-cache: use freshen_shared_index() in read_index_from()
 + read-cache: refactor read_index_from()
 + t1700: test shared index file expiration
 + read-cache: unlink old sharedindex files
 + config: add git_config_get_expiry() from gc.c
 + read-cache: touch shared index files when used
 + sha1_file: make check_and_freshen_file() non static
 + Documentation/config: add splitIndex.maxPercentChange
 + t1700: add tests for splitIndex.maxPercentChange
 + read-cache: regenerate shared index if necessary
 + config: add git_config_get_max_percent_split_change()
 + Documentation/git-update-index: talk about core.splitIndex config var
 + Documentation/config: add information for core.splitIndex
 + t1700: add tests for core.splitIndex
 + update-index: warn in case of split-index incoherency
 + read-cache: add and then use tweak_split_index()
 + split-index: add {add,remove}_split_index() functions
 + config: add git_config_get_split_index()
 + t1700: change here document style
 + config: mark an error message up for translation

 The experimental "split index" feature has gained a few
 configuration variables to make it easier to use.


* jk/add-i-use-pathspecs (2017-03-14) 1 commit
  (merged to 'next' on 2017-03-14 at 13ce4d91e1)
 + add--interactive: do not expand pathspecs with ls-files

 "git add -p " unnecessarily expanded the pathspec to a
 list of individual files that matches the pathspec by running "git
 ls-files ", before feeding it to "git diff-index" to see
 which paths have changes, because historically the pathspec
 language supported by "diff-index" was weaker.  These days they are
 equivalent and there is no reason to internally expand it.  This
 helps both performance and avoids command line argument limit on
 some platforms.


* jk/cherry-pick-0-mainline (2017-03-15) 1 commit
  (merged to 'next' on 2017-03-16 at e9a888e5c4)
 + cherry-pick: detect bogus arguments to --mainline

 "git revert -m 0 $merge_commit" complained that reverting a merge
 needs to say relative to which parent the reversion needs to
 happen, as if "-m 0" weren't given.  The correct diagnosis is that
 "-m 0" does not refer to the first parent ("-m 1" does).  This has
 been 

Re: [PATCH v4 08/10] clone: teach --recurse-submodules to optionally take a pathspec

[Brandon Williams]

On 03/17, Stefan Beller wrote:
> On Thu, Mar 16, 2017 at 3:29 PM, Brandon Williams  wrote:
> > Teach clone --recurse-submodules to optionally take a pathspec argument
> > which describes which submodules should be recursively initialized and
> > cloned.  If no pathspec is provided, --recurse-submodules will
> > recursively initialize and clone all submodules by using a default
> > pathspec of ".".  In order to construct more complex pathspecs,
> > --recurse-submodules can be given multiple times.
> >
> > Additionally this configures the 'submodule.active' configuration option
> > to be the given pathspec, such that any future invocation of `git
> > submodule update` will keep up with the pathspec.
> 
>   Additionally the switch '--recurse' is removed from the Documentation
>   as well as marked hidden in the options array, to streamline the options
>   for submodules.  A simple '--recurse' doesn't convey what is being
>   recursed, e.g. it could mean directories or trees (c.f. ls-tree)
>   In a lot of other commands we already have '--recurse-submodules'
>   to mean recursing into submodules, so advertise this spelling here
>   as the genuine option.
> 
> would also be worth mentioning?
> 

Yeah I can just add this into the commit msg.

> 
> >  static int option_no_checkout, option_bare, option_mirror, 
> > option_single_branch = -1;
> > -static int option_local = -1, option_no_hardlinks, option_shared, 
> > option_recursive;
> > +static int option_local = -1, option_no_hardlinks, option_shared;
> >  static int option_shallow_submodules;
> >  static int deepen;
> >  static char *option_template, *option_depth, *option_since;
> > @@ -56,6 +56,22 @@ static struct string_list option_required_reference = 
> > STRING_LIST_INIT_NODUP;
> >  static struct string_list option_optional_reference = 
> > STRING_LIST_INIT_NODUP;
> >  static int option_dissociate;
> >  static int max_jobs = -1;
> > +static struct string_list option_recurse_submodules = 
> > STRING_LIST_INIT_NODUP;
> > +
> > +static int recurse_submodules_cb(const struct option *opt,
> > +const char *arg, int unset)
> > +{
> > +   if (unset)
> > +   return -1;
> > +
> > +   if (arg)
> > +   string_list_append((struct string_list *)opt->value, arg);
> > +   else
> 
> in this case I'd rather set the removed (int) option_recursive, because, then
> we would not need to sort and remove duplicates later on.
> Instead we can pass the string list literally to the config setter.
> (and in case option_recursive is set, we add an additional single
> "." then)

That's just one more thing to worry about though.  This felt a little
bit cleaner than doing more special casing.

> 
> > +   string_list_append((struct string_list *)opt->value,
> > +  (const char *)opt->defval);
> > +
> > +   return 0;
> > +}
> >
> 
> >
> > -   if (!err && option_recursive) {
> > +   if (!err && (option_recurse_submodules.nr > 0)) {
> 
> Well, checks like these would become more tangled.
> So maybe we could set option_recursive unconditionally
> in the callback (unless unset was given, then we reset it to 0)
> and later have a check if we need to add "." (when the string list
> is empty).
> 
> Speaking of unset, this seems like a regression here, as the callback
> would error out to "git clone --no-recurse", which is a valid use case
> currently? (Searching for "git clone --no-recurse" yields no results
> via Google, so maybe this use case is not so valid)
> 
> To get the behavior as is for unset we could just clear the string
> list instead of returning -1.

Yeah I can do that.

> 
> Thanks,
> Stefan

-- 
Brandon Williams

Re: fatal: Could not get current working directory: Permission denied | affected 2.10,2.11,2.12, but not 1.9.5 |

[René Scharfe]

Am 17.03.2017 um 20:45 schrieb Stefan Beller:

On Fri, Mar 17, 2017 at 12:34 PM, René Scharfe  wrote:

Am 15.03.2017 um 22:30 schrieb René Scharfe:

Am 15.03.2017 um 10:44 schrieb Zenobiusz Kunegunda:

$ git bisect bad
7333ed1788b4f2b162a35003044d77a716732a1f is the first bad commit
commit 7333ed1788b4f2b162a35003044d77a716732a1f
Author: René Scharfe 
Date:   Mon Jul 28 20:26:40 2014 +0200

setup: convert setup_git_directory_gently_1 et al. to strbuf


That's what I half-suspected, and I think by now I got an idea.  Here's
a test program:


And here's a patch for letting strbuf_getcwd() use the same getcwd(3)
extension that pwd(1) uses.  It avoids the need to guess the path's
length and thus reduces the chance of stumbling over strange error
codes.  I wonder if it helps in your case.

René

---
 strbuf.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/strbuf.c b/strbuf.c
index ace58e7367..4c02801edd 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -442,6 +442,14 @@ int strbuf_getcwd(struct strbuf *sb)
 {
size_t oldalloc = sb->alloc;
size_t guessed_len = 128;
+   char *cwd;
+
+   cwd = getcwd(NULL, 0);


from my local man pages:

  As  an extension to the POSIX.1-2001 standard, Linux (libc4, libc5,
glibc) getcwd()
  allocates the buffer dynamically using malloc(3) if buf is NULL.  In
this case, the
  allocated buffer has the length size unless size is zero, when buf
is allocated as big
  as necessary.  The caller should free(3) the returned buffer.

This sounds specific to Linux (though I am reading Linux man pages,
which claim this; Also it seems I might have misread it as it also states
"The pathname is returned as the function result and via the
argument buf, if present.").


I'm only interested in FreeBSD for now, as that's the platform Zenobiusz 
reported the issue on and I haven't been able to reproduce it, so this 
is still a bit exploratory, but hopefully getting closer.  This 
extension is used in the first version of pwd(1) in FreeBSD's repo, 
comitted 1994-05-26, so it was supported there basically forever.


The oldest version I found that's using the extention is NetBSD's 
pwd(1), which was committed 1993-03-21 and carries a SCCS timestamp of 
1991-02-20.  Visual Studio .NET 2003 supports it as well.



Looking further:

  These functions are often used to save the location of the current
  working directory for the purpose of returning to it later.  Opening the
  current directory (".")  and  calling  fchdir(2)  to return is
usually a faster
  and more reliable alternative when sufficiently many file descriptors are
  available, especially on platforms other than Linux.

Not sure if that opens another door here?


Reducing the use of absolute paths may be a good idea in general, but 
that would probably require major changes.  And Windows doesn't seem to 
offer fchdir() at all; I don't know if it has an equivalent function 
that could be used to build a replacement.


René

Re: [PATCH v3 2/5] setup: allow for prefix to be passed to git commands

[Junio C Hamano]

Brandon Williams  writes:

> I don't think that prefix can ever have ".." in it.  From what I
> understand it is always a path from the root of the repository to the
> cwd that the git command was invoked by.  So in your example prefix
> would be "src/".

The prefix would be NULL or "", as you will be at the root-level of
the working tree when you are running _IN_ the submodule (by
recursing into it).  Not src/, nor anything with ../ in it, I would
think.

Re: [PATCH v3 2/5] setup: allow for prefix to be passed to git commands

[Brandon Williams]

On 03/17, Junio C Hamano wrote:
> Brandon Williams  writes:
> 
> > ...  I was being cautious with this patch since git didn't currently
> > read GIT_PREFIX.
> 
> Ahh, I forgot about that.  Processes we spawn do expect GIT_PREFIX
> to tell them where the original $cwd was and they also do expect
> that "git" invoked by them would not be affected by GIT_PREFIX
> environment variable.  So we cannot change that now.
> 

Yep.  I just ran the test suite locally doing this...it fails on a bunch
of stuff :)

> If you recurse into sub-sub module, it is likely that you would want
> to update the TOPLEVEL_PREFIX relative to that sub-sub module you
> are descending into.

So the idea would be that 'super_prefix' would keep getting updated when
recursing deeper, but the 'prefix' should stay the same (in what I'm
proposing here that is).

> 
> That probably also means that processes we spawn now need to also
> pay attention to TOPLEVEL_PREFIX in addition to GIT_PREFIX, and we
> should NOT re-export what we got from TOPLEVEL_PREFIX to GIT_PREFIX.
> I.e. if a "git" process started from src/ subdirectory of the
> superproject that goes into module/sub1/ submodule, top-level prefix
> may export ../src/ to point at the original location, but the
> process that is running in the submodule will be running at the root
> level of the submodule working tree, so its prefix should be NULL or
> "", no?

I don't think that prefix can ever have ".." in it.  From what I
understand it is always a path from the root of the repository to the
cwd that the git command was invoked by.  So in your example prefix
would be "src/".

> 
> Adjusting pathspec and other file references on the caller's side,
> instead of exporting toplevel-prefix to have them adjusted by the
> callee, started to smell more and more like an easier/more correct
> approach to me, but perhaps I haven't thought things deeply enough.
> 
> I dunno.
> 

Yeah, doing all of the adjusting on the caller's side is quite
difficult.  Getting the pathspecs and file references right seems to be
overly complicated if done by the caller.  That why I opted to punt to
the callee; given enough information (e.g. prefix and super_prefix) the
callee should be able to handle both the pathspecs and file references
properly.

-- 
Brandon Williams

Bug with .gitignore and branch switching

[Nevada Sanchez]

Here's an easy to reproduce bug. It's the only example I know of where
git legitimately loses data in a way that is unrecoverable,
unexpected, and without warning.

```
git --version
# git version 2.12.0

mkdir git-demo
cd git-demo

git init

# Commit a file that will end up in .gitignore
echo 'original settings' > mine.conf
git add mine.conf
git commit -m "Unknowingly committed my settings."

echo '*.conf' > .gitignore
git add .gitignore
git commit -m "Users shouldn't commit their settings"

# Spin off a feature branch here (but don't check it out)
git branch feature

# Realize that we don't want that file committed
git rm mine.conf
git commit -m "Delete mine.conf"

echo 'Lots of laboriously tuned settings' > mine.conf

# Hop on the feature branch to do some work
git checkout feature

# Hmmm... My settings are gone
cat mine.conf
# original settings

# Lemme hop back
git checkout master

# Wait... they are gone for good!
cat mine.conf
# cat: mine.conf: No such file or directory
```

Re: [PATCH v3 2/5] setup: allow for prefix to be passed to git commands

[Junio C Hamano]

Brandon Williams  writes:

> ...  I was being cautious with this patch since git didn't currently
> read GIT_PREFIX.

Ahh, I forgot about that.  Processes we spawn do expect GIT_PREFIX
to tell them where the original $cwd was and they also do expect
that "git" invoked by them would not be affected by GIT_PREFIX
environment variable.  So we cannot change that now.

If you recurse into sub-sub module, it is likely that you would want
to update the TOPLEVEL_PREFIX relative to that sub-sub module you
are descending into.

That probably also means that processes we spawn now need to also
pay attention to TOPLEVEL_PREFIX in addition to GIT_PREFIX, and we
should NOT re-export what we got from TOPLEVEL_PREFIX to GIT_PREFIX.
I.e. if a "git" process started from src/ subdirectory of the
superproject that goes into module/sub1/ submodule, top-level prefix
may export ../src/ to point at the original location, but the
process that is running in the submodule will be running at the root
level of the submodule working tree, so its prefix should be NULL or
"", no?

Adjusting pathspec and other file references on the caller's side,
instead of exporting toplevel-prefix to have them adjusted by the
callee, started to smell more and more like an easier/more correct
approach to me, but perhaps I haven't thought things deeply enough.

I dunno.

Re: fatal: Could not get current working directory: Permission denied | affected 2.10,2.11,2.12, but not 1.9.5 |

[Stefan Beller]

On Fri, Mar 17, 2017 at 12:34 PM, René Scharfe  wrote:
> Am 15.03.2017 um 22:30 schrieb René Scharfe:
>> Am 15.03.2017 um 10:44 schrieb Zenobiusz Kunegunda:
>>> $ git bisect bad
>>> 7333ed1788b4f2b162a35003044d77a716732a1f is the first bad commit
>>> commit 7333ed1788b4f2b162a35003044d77a716732a1f
>>> Author: René Scharfe 
>>> Date:   Mon Jul 28 20:26:40 2014 +0200
>>>
>>> setup: convert setup_git_directory_gently_1 et al. to strbuf
>>
>> That's what I half-suspected, and I think by now I got an idea.  Here's
>> a test program:
>
> And here's a patch for letting strbuf_getcwd() use the same getcwd(3)
> extension that pwd(1) uses.  It avoids the need to guess the path's
> length and thus reduces the chance of stumbling over strange error
> codes.  I wonder if it helps in your case.
>
> René
>
> ---
>  strbuf.c | 8 
>  1 file changed, 8 insertions(+)
>
> diff --git a/strbuf.c b/strbuf.c
> index ace58e7367..4c02801edd 100644
> --- a/strbuf.c
> +++ b/strbuf.c
> @@ -442,6 +442,14 @@ int strbuf_getcwd(struct strbuf *sb)
>  {
> size_t oldalloc = sb->alloc;
> size_t guessed_len = 128;
> +   char *cwd;
> +
> +   cwd = getcwd(NULL, 0);

from my local man pages:

  As  an extension to the POSIX.1-2001 standard, Linux (libc4, libc5,
glibc) getcwd()
  allocates the buffer dynamically using malloc(3) if buf is NULL.  In
this case, the
  allocated buffer has the length size unless size is zero, when buf
is allocated as big
  as necessary.  The caller should free(3) the returned buffer.

This sounds specific to Linux (though I am reading Linux man pages,
which claim this; Also it seems I might have misread it as it also states
"The pathname is returned as the function result and via the
argument buf, if present.").

Looking further:

  These functions are often used to save the location of the current
  working directory for the purpose of returning to it later.  Opening the
  current directory (".")  and  calling  fchdir(2)  to return is
usually a faster
  and more reliable alternative when sufficiently many file descriptors are
  available, especially on platforms other than Linux.

Not sure if that opens another door here?

Thanks,
Stefan

Re: fatal: Could not get current working directory: Permission denied | affected 2.10,2.11,2.12, but not 1.9.5 |

[René Scharfe]

Am 15.03.2017 um 22:30 schrieb René Scharfe:
> Am 15.03.2017 um 10:44 schrieb Zenobiusz Kunegunda:
>> $ git bisect bad
>> 7333ed1788b4f2b162a35003044d77a716732a1f is the first bad commit
>> commit 7333ed1788b4f2b162a35003044d77a716732a1f
>> Author: René Scharfe 
>> Date:   Mon Jul 28 20:26:40 2014 +0200
>>
>> setup: convert setup_git_directory_gently_1 et al. to strbuf
> 
> That's what I half-suspected, and I think by now I got an idea.  Here's
> a test program:

And here's a patch for letting strbuf_getcwd() use the same getcwd(3)
extension that pwd(1) uses.  It avoids the need to guess the path's
length and thus reduces the chance of stumbling over strange error
codes.  I wonder if it helps in your case.

René

---
 strbuf.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/strbuf.c b/strbuf.c
index ace58e7367..4c02801edd 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -442,6 +442,14 @@ int strbuf_getcwd(struct strbuf *sb)
 {
size_t oldalloc = sb->alloc;
size_t guessed_len = 128;
+   char *cwd;
+
+   cwd = getcwd(NULL, 0);
+   if (cwd) {
+   size_t len = strlen(cwd);
+   strbuf_attach(sb, cwd, len, len + 1);
+   return 0;
+   }
 
for (;; guessed_len *= 2) {
strbuf_grow(sb, guessed_len);
-- 
2.12.0

Re: [GSoC][PATCH v5 2/3] credential-cache: use XDG_CACHE_HOME for socket

[Devin Lehmacher]

> or $HOME/.cache/git/credential/socket if $XDG_CACHE_HOME is not set.
> I don't have a good suggestion to re-word this paragraph. (I just
> spent ten minutes trying!).

I think it would not be too bad to not include this though because
`$HOME/.cache/` is what XDG specifies as the default if
`$XDG_CACHE_HOME` is unset.

Devin

Re: [PATCH v3 2/5] setup: allow for prefix to be passed to git commands

[Brandon Williams]

On 03/17, Junio C Hamano wrote:
> Stefan Beller  writes:
> 
> >> prefix = setup_git_directory_gently_1(nongit_ok);
> >> +   env_prefix = getenv(GIT_TOPLEVEL_PREFIX_ENVIRONMENT);
> >> +
> >> +   if (env_prefix)
> >> +   prefix = env_prefix;
> >> +
> >> if (prefix)
> >> setenv(GIT_PREFIX_ENVIRONMENT, prefix, 1);
> >
> > so we load that GIT_TOPLEVEL_PREFIX_ENVIRONMENT prefix
> > first, such that we essentially copy it into GIT_PREFIX_ENVIRONMENT,
> > such that e.g. aliased commands will know about the superprefix, too.
> 
> If the aliased commands or anything else spawned from this process
> is happy with GIT_PREFIX set to the outside of the current
> repository, doing this setenv() is OK.  If you are in ~/dir1, and
> your repository is in ~/repos/repo1, and if you somehow had a way
> to run your "git" inside ~/repos/repo1 without doing any chdir(2),
> then you are essentially setting ../../dir1/ as GIT_PREFIX for that
> "git" invocation (this has nothing to do with submodules).
> 
> But if your "git" is fine with GIT_PREFIX pointing outside the root
> level of the working tree of the current repository like that, do we
> even need a separate toplevel prefix environment, I have to wonder?
> 
> That is, if this "if TOPLEVEL_PREFIX environment is there, set it to
> local variable prefix and export it as GIT_PREFIX" is expected to
> work correctly for anything that would inherit that GIT_PREFIX, then
> we should be able to invoke the "git" that got TOPLEVEL_PREFIX
> without setting that environment, but instead setting the same value
> to GIT_PREFIX and we should get the same behaviour, no?
> 

Very true, potentially we could just use GIT_PREFIX instead of
introducing a brand new env var (which is essentially just the same
thing).  I was being cautious with this patch since git didn't currently
read GIT_PREFIX.  I was hoping other with more knowledge in this area
would voice their opinions and lead me in the right direction ;)

-- 
Brandon Williams

Re: [PATCH v3 2/5] setup: allow for prefix to be passed to git commands

[Brandon Williams]

On 03/17, Stefan Beller wrote:
> > prefix = setup_git_directory_gently_1(nongit_ok);
> > +   env_prefix = getenv(GIT_TOPLEVEL_PREFIX_ENVIRONMENT);
> > +
> > +   if (env_prefix)
> > +   prefix = env_prefix;
> > +
> > if (prefix)
> > setenv(GIT_PREFIX_ENVIRONMENT, prefix, 1);
> 
> so we load that GIT_TOPLEVEL_PREFIX_ENVIRONMENT prefix
> first, such that we essentially copy it into GIT_PREFIX_ENVIRONMENT,
> such that e.g. aliased commands will know about the superprefix, too.

I don't follow, this doesn't have anything to do with super-prefix.

> 
> ok, sounds reasonable to me; though I do not use this feature,
> so my judgement is not as good.
> 
> Do we need a test for this behavior?
> 
> Thanks,
> Stefan

-- 
Brandon Williams

Re: [PATCH v3 2/5] setup: allow for prefix to be passed to git commands

[Stefan Beller]

On Fri, Mar 17, 2017 at 12:08 PM, Brandon Williams  wrote:
> On 03/17, Stefan Beller wrote:
>> > prefix = setup_git_directory_gently_1(nongit_ok);
>> > +   env_prefix = getenv(GIT_TOPLEVEL_PREFIX_ENVIRONMENT);
>> > +
>> > +   if (env_prefix)
>> > +   prefix = env_prefix;
>> > +
>> > if (prefix)
>> > setenv(GIT_PREFIX_ENVIRONMENT, prefix, 1);
>>
>> so we load that GIT_TOPLEVEL_PREFIX_ENVIRONMENT prefix
>> first, such that we essentially copy it into GIT_PREFIX_ENVIRONMENT,
>> such that e.g. aliased commands will know about the superprefix, too.
>
> I don't follow, this doesn't have anything to do with super-prefix.
>

s/superprefix/prefix as passed in via GIT_TOPLEVEL_PREFIX_ENVIRONMENT/

sorry for the confusion.

Re: [PATCH v3 2/5] setup: allow for prefix to be passed to git commands

[Brandon Williams]

On 03/17, Stefan Beller wrote:
> On Fri, Mar 17, 2017 at 12:08 PM, Brandon Williams  wrote:
> > On 03/17, Stefan Beller wrote:
> >> > prefix = setup_git_directory_gently_1(nongit_ok);
> >> > +   env_prefix = getenv(GIT_TOPLEVEL_PREFIX_ENVIRONMENT);
> >> > +
> >> > +   if (env_prefix)
> >> > +   prefix = env_prefix;
> >> > +
> >> > if (prefix)
> >> > setenv(GIT_PREFIX_ENVIRONMENT, prefix, 1);
> >>
> >> so we load that GIT_TOPLEVEL_PREFIX_ENVIRONMENT prefix
> >> first, such that we essentially copy it into GIT_PREFIX_ENVIRONMENT,
> >> such that e.g. aliased commands will know about the superprefix, too.
> >
> > I don't follow, this doesn't have anything to do with super-prefix.
> >
> 
> s/superprefix/prefix as passed in via GIT_TOPLEVEL_PREFIX_ENVIRONMENT/
> 
> sorry for the confusion.

Alternatively we could not copy it into GIT_PREFIX_ENVIRONMENT.

-- 
Brandon Williams

Re: [PATCH v3 2/5] setup: allow for prefix to be passed to git commands

[Junio C Hamano]

Stefan Beller  writes:

>> prefix = setup_git_directory_gently_1(nongit_ok);
>> +   env_prefix = getenv(GIT_TOPLEVEL_PREFIX_ENVIRONMENT);
>> +
>> +   if (env_prefix)
>> +   prefix = env_prefix;
>> +
>> if (prefix)
>> setenv(GIT_PREFIX_ENVIRONMENT, prefix, 1);
>
> so we load that GIT_TOPLEVEL_PREFIX_ENVIRONMENT prefix
> first, such that we essentially copy it into GIT_PREFIX_ENVIRONMENT,
> such that e.g. aliased commands will know about the superprefix, too.

If the aliased commands or anything else spawned from this process
is happy with GIT_PREFIX set to the outside of the current
repository, doing this setenv() is OK.  If you are in ~/dir1, and
your repository is in ~/repos/repo1, and if you somehow had a way
to run your "git" inside ~/repos/repo1 without doing any chdir(2),
then you are essentially setting ../../dir1/ as GIT_PREFIX for that
"git" invocation (this has nothing to do with submodules).

But if your "git" is fine with GIT_PREFIX pointing outside the root
level of the working tree of the current repository like that, do we
even need a separate toplevel prefix environment, I have to wonder?

That is, if this "if TOPLEVEL_PREFIX environment is there, set it to
local variable prefix and export it as GIT_PREFIX" is expected to
work correctly for anything that would inherit that GIT_PREFIX, then
we should be able to invoke the "git" that got TOPLEVEL_PREFIX
without setting that environment, but instead setting the same value
to GIT_PREFIX and we should get the same behaviour, no?

Re: [PATCH v3 2/5] setup: allow for prefix to be passed to git commands

[Stefan Beller]

> prefix = setup_git_directory_gently_1(nongit_ok);
> +   env_prefix = getenv(GIT_TOPLEVEL_PREFIX_ENVIRONMENT);
> +
> +   if (env_prefix)
> +   prefix = env_prefix;
> +
> if (prefix)
> setenv(GIT_PREFIX_ENVIRONMENT, prefix, 1);

so we load that GIT_TOPLEVEL_PREFIX_ENVIRONMENT prefix
first, such that we essentially copy it into GIT_PREFIX_ENVIRONMENT,
such that e.g. aliased commands will know about the superprefix, too.

ok, sounds reasonable to me; though I do not use this feature,
so my judgement is not as good.

Do we need a test for this behavior?

Thanks,
Stefan

Re: [PATCH] grep: fix build with no thread support

[Brandon Williams]

While taking a look at this bug I discovered that the test suite doesn't
pass 100% of the test when compiled with the NO_PTHREADS option. The
following tests seem to be failing:

t1060-object-corruption.sh   (Wstat: 256 Tests: 13 Failed: 
3)
  Failed tests:  7-9
  Non-zero exit status: 1
t5306-pack-nobase.sh (Wstat: 256 Tests: 4 Failed: 1)
  Failed test:  4
  Non-zero exit status: 1
t5504-fetch-receive-strict.sh(Wstat: 256 Tests: 12 Failed: 
2)
  Failed tests:  4-5
  Non-zero exit status: 1
t5530-upload-pack-error.sh   (Wstat: 256 Tests: 10 Failed: 
1)
  Failed test:  10
  Non-zero exit status: 1

I didn't take a close look at it but this would seem to indicate that we
don't worry to much about systems without pthreads support.  Just food
for thought.

-- 
Brandon Williams

[PATCH 2/2] grep: fix builds with with no thread support

[Brandon Williams]

Commit 0281e487fd91 ("grep: optionally recurse into submodules")
added functions grep_submodule() and grep_submodule_launch() which
uses "struct work_item" which is defined only when thread support
is available.

The original implementation of grep_submodule() used the "struct
work_item" in order to gain access to a strbuf to store its output which
was to be printed at a later point in time.  This differs from how both
grep_file() and grep_sha1() handle their output.  This patch eliminates
the reliance on the "struct work_item" and instead opts to use the
output function stored in the output field of the "struct grep_opt"
object directly, making it behave similarly to both grep_file() and
grep_sha1().

Reported-by: Rahul Bedarkar 
Signed-off-by: Brandon Williams 
---
 builtin/grep.c | 21 +
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 9304c33e7..3f1251bab 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -538,7 +538,7 @@ static int grep_submodule_launch(struct grep_opt *opt,
int status, i;
const char *end_of_base;
const char *name;
-   struct work_item *w = opt->output_priv;
+   struct strbuf child_output = STRBUF_INIT;
 
end_of_base = strchr(gs->name, ':');
if (gs->identifier && end_of_base)
@@ -593,14 +593,16 @@ static int grep_submodule_launch(struct grep_opt *opt,
 * child process.  A '0' indicates a hit, a '1' indicates no hit and
 * anything else is an error.
 */
-   status = capture_command(, >out, 0);
+   status = capture_command(, _output, 0);
if (status && (status != 1)) {
/* flush the buffer */
-   write_or_die(1, w->out.buf, w->out.len);
+   write_or_die(1, child_output.buf, child_output.len);
die("process for submodule '%s' failed with exit code: %d",
gs->name, status);
}
 
+   opt->output(opt, child_output.buf, child_output.len);
+   strbuf_release(_output);
/* invert the return code to make a hit equal to 1 */
return !status;
 }
@@ -641,19 +643,14 @@ static int grep_submodule(struct grep_opt *opt, const 
unsigned char *sha1,
} else
 #endif
{
-   struct work_item w;
+   struct grep_source gs;
int hit;
 
-   grep_source_init(, GREP_SOURCE_SUBMODULE,
+   grep_source_init(, GREP_SOURCE_SUBMODULE,
 filename, path, sha1);
-   strbuf_init(, 0);
-   opt->output_priv = 
-   hit = grep_submodule_launch(opt, );
+   hit = grep_submodule_launch(opt, );
 
-   write_or_die(1, w.out.buf, w.out.len);
-
-   grep_source_clear();
-   strbuf_release();
+   grep_source_clear();
return hit;
}
 }
-- 
2.12.0.367.g23dc2f6d3c-goog

[PATCH 1/2] grep: set default output method

[Brandon Williams]

Signed-off-by: Brandon Williams 
---
 grep.c | 12 +++-
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/grep.c b/grep.c
index 0dbdc1d00..56ef0ecbf 100644
--- a/grep.c
+++ b/grep.c
@@ -12,6 +12,11 @@ static int grep_source_is_binary(struct grep_source *gs);
 
 static struct grep_opt grep_defaults;
 
+static void std_output(struct grep_opt *opt, const void *buf, size_t size)
+{
+   fwrite(buf, size, 1, stdout);
+}
+
 /*
  * Initialize the grep_defaults template with hardcoded defaults.
  * We could let the compiler do this, but without C99 initializers
@@ -42,6 +47,7 @@ void init_grep_defaults(void)
color_set(opt->color_selected, "");
color_set(opt->color_sep, GIT_COLOR_CYAN);
opt->color = -1;
+   opt->output = std_output;
 }
 
 static int parse_pattern_type_arg(const char *opt, const char *arg)
@@ -152,6 +158,7 @@ void grep_init(struct grep_opt *opt, const char *prefix)
opt->pathname = def->pathname;
opt->regflags = def->regflags;
opt->relative = def->relative;
+   opt->output = def->output;
 
color_set(opt->color_context, def->color_context);
color_set(opt->color_filename, def->color_filename);
@@ -1379,11 +1386,6 @@ static int look_ahead(struct grep_opt *opt,
return 0;
 }
 
-static void std_output(struct grep_opt *opt, const void *buf, size_t size)
-{
-   fwrite(buf, size, 1, stdout);
-}
-
 static int fill_textconv_grep(struct userdiff_driver *driver,
  struct grep_source *gs)
 {
-- 
2.12.0.367.g23dc2f6d3c-goog

Re: Shared repositories no longer securable against privilege escalation

[Junio C Hamano]

Michael Haggerty  writes:

> The locking was added intentionally, to ensure that the reflog for
> `HEAD` is written safely. But the code didn't do that prior to the
> commit that you referenced, so (as a special case) ignoring failures to
> lock `HEAD` due to insufficient permission is probably a reasonable
> compromise.
>
> I think the special case could be restricted even further to when `HEAD`
> has the `REF_LOG_ONLY` flag in the reference transaction. I don't think
> that `HEAD` would ever show up in a transaction solely to verify its old
> value in a typical server scenario, but if so, that situation could be
> special cased too.

I find both of these acceptably good changes.

Re: [PATCH] grep: fix build with no thread support

[Brandon Williams]

On 03/17, Junio C Hamano wrote:
> Rahul Bedarkar  writes:
> 
> > diff --git a/builtin/grep.c b/builtin/grep.c
> > index 2c727ef..4373d79 100644
> > --- a/builtin/grep.c
> > +++ b/builtin/grep.c
> > @@ -183,11 +183,13 @@ static void *run(void *arg)
> > if (!w)
> > break;
> >  
> > -   opt->output_priv = w;
> > -   if (w->source.type == GREP_SOURCE_SUBMODULE)
> > +   if (w->source.type == GREP_SOURCE_SUBMODULE) {
> > +   opt->output_priv = >out;
> > hit |= grep_submodule_launch(opt, >source);
> > -   else
> > +   } else {
> > +   opt->output_priv = w;
> > hit |= grep_source(opt, >source);
> > +   }
> > grep_source_clear_data(>source);
> > work_done(w);
> > }
> 
> This is not a part of the "fix" but merely a code clean-up, right?
> Just double-checking.
> 
> > @@ -538,7 +540,7 @@ static int grep_submodule_launch(struct grep_opt *opt,
> > int status, i;
> > const char *end_of_base;
> > const char *name;
> > -   struct work_item *w = opt->output_priv;
> > +   struct strbuf *w = opt->output_priv;
> >  
> > end_of_base = strchr(gs->name, ':');
> > if (gs->identifier && end_of_base)
> 
> OK, so the new code points output_priv at a strbuf; work_item
> contains an "out" strbuf, and that was why the original code was
> passing one, but this codepath does not need a full work_item to
> work with.  Is that what is going on?
> 
> > @@ -593,10 +595,10 @@ static int grep_submodule_launch(struct grep_opt *opt,
> >  * child process.  A '0' indicates a hit, a '1' indicates no hit and
> >  * anything else is an error.
> >  */
> > -   status = capture_command(, >out, 0);
> > +   status = capture_command(, w, 0);
> 
> And this is consistent with the above change.
> 
> > if (status && (status != 1)) {
> > /* flush the buffer */
> > -   write_or_die(1, w->out.buf, w->out.len);
> > +   write_or_die(1, w->buf, w->len);
> 
> So is this.
> 
> > die("process for submodule '%s' failed with exit code: %d",
> > gs->name, status);
> > }
> > @@ -641,19 +643,19 @@ static int grep_submodule(struct grep_opt *opt, const 
> > unsigned char *sha1,
> > } else
> >  #endif
> > {
> > -   struct work_item w;
> > +   struct grep_source gs;
> > int hit;
> > +   struct strbuf outbuf = STRBUF_INIT;
> >  
> > -   grep_source_init(, GREP_SOURCE_SUBMODULE,
> > +   grep_source_init(, GREP_SOURCE_SUBMODULE,
> >  filename, path, sha1);
> 
> Likewise for w.source that happened to have grep_source, but passing
> a bare grep_source is sufficient for the purpose of this codepath,
> without giving it to w.source?  
> 
> I didn't look at code that this patch does not touch, but if
> anything still looks at w.out and w.source and expect these to
> contain the string accumulated in the strbuf and the grep source the
> work item is working on, they will get broken by this change, no?
> 
> The first hunk that had a pure clean-up shows that w->source being
> the correct grep source seems to matter.
> 
> > -   strbuf_init(, 0);
> > -   opt->output_priv = 
> > -   hit = grep_submodule_launch(opt, );
> > +   opt->output_priv = 
> > +   hit = grep_submodule_launch(opt, );
> >  
> > -   write_or_die(1, w.out.buf, w.out.len);
> > +   write_or_die(1, outbuf.buf, outbuf.len);
> >  
> > -   grep_source_clear();
> > -   strbuf_release();
> > +   grep_source_clear();
> > +   strbuf_release();
> > return hit;
> > }
> >  }

Thanks for pointing out the bug (there seem to just be more and more in
this grep code...but what else can you expect from my first
contribution! :D) but I think it would be better to make the way the
recursive code output's more consistent with the way the rest of the
grep handles output.  That way we can get rid of some of the special
casing that I had done here.

I'll send out what I have locally in just a second.

-- 
Brandon Williams

Re: Shared repositories no longer securable against privilege escalation

[Junio C Hamano]

Joe Rayhawk  writes:

> that, at least on base POSIX, using --shared to share a repository
> between multiple UIDs literally eliminates the purpose of having
> multiple UIDs.

I do not think the world is _that_ blank-and-white.  If you cannot
trust those who push to the repository, you can give them git-only
access without a shell account and keep separating them with UIDs.
If you can, then the --shared setting is suitable for you.

Re: [PATCH v4 07/10] submodule init: initialize active submodules

[Stefan Beller]

On Thu, Mar 16, 2017 at 3:29 PM, Brandon Williams  wrote:
> Teach `submodule init` to initialize submodules which have been
> configured to be active by setting 'submodule.active' with a pathspec.
>
> Now if no path arguments are given and 'submodule.active' is configured,
> `init` will initialize all submodules which have been configured to be
> active.  If no path arguments are given and 'submodule.active' is not
> configured, then `init` will retain the old behavior of initializing all
> submodules.
>
> This allows users to record more complex patterns as it saves retyping
> them whenever you invoke update.
>
> Signed-off-by: Brandon Williams 
> ---


> @@ -417,7 +445,13 @@ static int module_init(int argc, const char **argv, 
> const char *prefix)
> argc = parse_options(argc, argv, prefix, module_init_options,
>  git_submodule_helper_usage, 0);
>
> -   if (module_list_compute(argc, argv, prefix, , ) < 0)
> +   /*
> +* If there are no path args and submodule.active is set then,
> +* by default, only initialize 'active' modules.
> +*/
> +   if (!argc && git_config_get_value_multi("submodule.active"))
> +   module_list_active();
> +   else if (module_list_compute(argc, argv, prefix, , ) < 
> 0)
> return 1;

I would rather reuse module_list_compute and then post-process the list
to filter out inactive submodules iff "submodule.active" is set as that seems
cleaner and performance is not a pressing issue here?

>
> +static void module_list_active(struct module_list *list)
> +{
> +   int i;
> +
> +   if (read_cache() < 0)
> +   die(_("index file corrupt"));
> +
> +   gitmodules_config();

Here we also need to have

  git_config(submodule_config, NULL);

such that is_submodule_initialized works correctly,
I would assume?

> +
> +   for (i = 0; i < active_nr; i++) {
> +   const struct cache_entry *ce = active_cache[i];
> +
> +   if (!S_ISGITLINK(ce->ce_mode) ||
> +   !is_submodule_initialized(ce->name))
> +   continue;
> +
> +   ALLOC_GROW(list->entries, list->nr + 1, list->alloc);
> +   list->entries[list->nr++] = ce;
> +   while (i + 1 < active_nr &&
> +  !strcmp(ce->name, active_cache[i + 1]->name))
> +   /*
> +* Skip entries with the same name in different stages
> +* to make sure an entry is returned only once.
> +*/
> +   i++;
> +   }
> +}