Re: body-CC-comment regression

[Junio C Hamano]

Matthieu Moy  writes:

> Junio C Hamano  writes:
>
>> Johan Hovold  writes:
>>
>>> That's precisely what the patch I posted earlier in the thread did.
>>
>> That's good.  I didn't see any patch yet 
>
> It's here:
>
> http://public-inbox.org/git/20170217110642.GD2625@localhost/
>
> but as I explained, this removes a feature suported since several major
> releases and we have no idea how many users may use the "mupliple emails
> in one field". The approach I proposed does not suffer from this.

OK, so you are aiming higher to please both parties, i.e. those who
place a single address but cruft at the end would get the cruft
stripped when we grab a usable address from the field, and those who
write two or more addresses would get all of these addresses?

That approach may still constrain what those in the former camp can
write in the "cruft" part, like they cannot write comma or semicolon
as part of the "cruft", no?  If that does not pose a practical problem,
then I can imagine it would work well for people in both camps.

Re: [PATCH 3/3] rename_ref: replace empty deletion message in HEAD's log

[Jeff King]

On Fri, Feb 17, 2017 at 02:43:50PM -0500, Jeff King wrote:

> Yes. I think the options are basically (in order of decreasing
> preference in my opinion):
> 
>   1. Log a rename entry (same sha1, but note the rename in the free-form
>  text).
> 
>   2. Log a delete (sha1 goes to null) followed by a creation (from null
>  back to the original sha1).
> 
>   3. Log nothing at all for HEAD.
> 
> This does half of (2). If we do the second half, then I'd prefer it to
> (3). But if we can do (1), that is better still (IMHO).
> 
> > *1* Is the reason why the code in files_rename_ref() we are looking
> > at does not adjust HEAD to point at the new ref is because it is
> > just handing one ref-store and obviouvious to symrefs in other
> > backends?
> 
> I'm actually confused about which bit of code is updating HEAD. I do not
> see it either in files_rename_ref() or in the caller. Yet it clearly
> happens. But that is the code that would know enough to do (1) or the
> second half of (2) above.

Ah, I found it. It's in replace_each_worktree_head_symref() these days,
which does not bother to pass a log message.

So I think the second half of (2) is probably something like the patch
below.

Thinking on it more, we probably _do_ want two entries. Because the
operations are not atomic, it's possible that we may end up in a
half-way state after the first entry is written. And when debugging such
a case, I'd much rather see the first half of the operation logged than
nothing at all.

-Peff

---
diff --git a/branch.c b/branch.c
index b955d4f31..5c12036b0 100644
--- a/branch.c
+++ b/branch.c
@@ -345,7 +345,8 @@ void die_if_checked_out(const char *branch, int 
ignore_current_worktree)
branch, wt->path);
 }
 
-int replace_each_worktree_head_symref(const char *oldref, const char *newref)
+int replace_each_worktree_head_symref(const char *oldref, const char *newref,
+ const char *logmsg)
 {
int ret = 0;
struct worktree **worktrees = get_worktrees(0);
@@ -358,7 +359,7 @@ int replace_each_worktree_head_symref(const char *oldref, 
const char *newref)
continue;
 
if (set_worktree_head_symref(get_worktree_git_dir(worktrees[i]),
-newref)) {
+newref, logmsg)) {
ret = -1;
error(_("HEAD of working tree %s is not updated"),
  worktrees[i]->path);
diff --git a/branch.h b/branch.h
index 3103eb9ad..b07788558 100644
--- a/branch.h
+++ b/branch.h
@@ -71,6 +71,7 @@ extern void die_if_checked_out(const char *branch, int 
ignore_current_worktree);
  * This will be used when renaming a branch. Returns 0 if successful, non-zero
  * otherwise.
  */
-extern int replace_each_worktree_head_symref(const char *oldref, const char 
*newref);
+extern int replace_each_worktree_head_symref(const char *oldref, const char 
*newref,
+const char *logmsg);
 
 #endif
diff --git a/builtin/branch.c b/builtin/branch.c
index cbaa6d03c..383005912 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -471,14 +471,15 @@ static void rename_branch(const char *oldname, const char 
*newname, int force)
 
if (rename_ref(oldref.buf, newref.buf, logmsg.buf))
die(_("Branch rename failed"));
-   strbuf_release();
 
if (recovery)
warning(_("Renamed a misnamed branch '%s' away"), oldref.buf + 
11);
 
-   if (replace_each_worktree_head_symref(oldref.buf, newref.buf))
+   if (replace_each_worktree_head_symref(oldref.buf, newref.buf, 
logmsg.buf))
die(_("Branch renamed to %s, but HEAD is not updated!"), 
newname);
 
+   strbuf_release();
+
strbuf_addf(, "branch.%s", oldref.buf + 11);
strbuf_release();
strbuf_addf(, "branch.%s", newref.buf + 11);
diff --git a/refs.h b/refs.h
index 9fbff90e7..b33035c4a 100644
--- a/refs.h
+++ b/refs.h
@@ -334,7 +334,8 @@ int create_symref(const char *refname, const char *target, 
const char *logmsg);
  * $GIT_DIR points to.
  * Return 0 if successful, non-zero otherwise.
  * */
-int set_worktree_head_symref(const char *gitdir, const char *target);
+int set_worktree_head_symref(const char *gitdir, const char *target,
+const char *logmsg);
 
 enum action_on_err {
UPDATE_REFS_MSG_ON_ERR,
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 4f1a88f6d..fa8d08e3c 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3024,7 +3024,7 @@ static int files_create_symref(struct ref_store 
*ref_store,
return ret;
 }
 
-int set_worktree_head_symref(const char *gitdir, const char *target)
+int set_worktree_head_symref(const char *gitdir, const char *target, const 
char *logmsg)
 {
static struct lock_file head_lock;
struct ref_lock *lock;
@@ -3052,7 +3052,7 @@ 

Re: [PATCH 3/3] rename_ref: replace empty deletion message in HEAD's log

[Jeff King]

On Fri, Feb 17, 2017 at 09:50:54AM -0800, Junio C Hamano wrote:

> > I see we actually already have a "logmsg" parameter. It already says
> > "Branch: renamed %s to %s". Could we just reuse that? I know that this
> > step is technically just the deletion, but I think it more accurately
> > describes the whole operation that the deletion is part of.
> 
> True, but stepping back a bit,...
> 
> Do we even want these "internal" delete_ref() invocations to be
> logged in HEAD's reflog?  I understand that this is inside the
> implementation of renaming an old ref to a new ref, and reflog
> message given to delete_ref() would matter only if the HEAD happens
> to be pointing at old ref---but then HEAD will be repointed to the
> new ref by somebody else [*1*] that called this function to rename
> old to new and it _will_ log it.  So I am not sure if it is a good
> thing to describe the deletion more readably with a message (which
> is what this patch does) in the first place.  If we can just say
> "don't log this deletion event in HEAD's reflog", wouldn't that be
> more desirable?

Yes. I think the options are basically (in order of decreasing
preference in my opinion):

  1. Log a rename entry (same sha1, but note the rename in the free-form
 text).

  2. Log a delete (sha1 goes to null) followed by a creation (from null
 back to the original sha1).

  3. Log nothing at all for HEAD.

This does half of (2). If we do the second half, then I'd prefer it to
(3). But if we can do (1), that is better still (IMHO).

> *1* Is the reason why the code in files_rename_ref() we are looking
> at does not adjust HEAD to point at the new ref is because it is
> just handing one ref-store and obviouvious to symrefs in other
> backends?

I'm actually confused about which bit of code is updating HEAD. I do not
see it either in files_rename_ref() or in the caller. Yet it clearly
happens. But that is the code that would know enough to do (1) or the
second half of (2) above.

-Peff

Re: [PATCH v3 11/16] refs.c: kill register_ref_store(), add register_submodule_ref_store()

[Junio C Hamano]

Nguyễn Thái Ngọc Duy   writes:

> 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().

Very nice.

Re: [PATCH v3 04/16] files-backend: replace *git_path*() with files_path()

[Junio C Hamano]

Nguyễn Thái Ngọc Duy   writes:

> This centralizes all path rewriting of files-backend.c in one place so
> we have easier time removing the path rewriting later. There could be
> some hidden indirect git_path() though, I didn't audit the code to the
> bottom.
>
> Side note: set_worktree_head_symref() is a bad boy and should not be in
> files-backend.c (probably should not exist in the first place). But
> we'll leave it there until we have better multi-worktree support in refs
> before we update it.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  refs/files-backend.c | 185 
> ++-
>  1 file changed, 94 insertions(+), 91 deletions(-)

In this step, files_path() is still "if refs->submodule field is
there, then use that to call strbuf_git_path_submodule() and
otherwise call strbuf_git_path()."  That is a very sensible
refactoring for things like packed-refs-file in this hunk:

>  static struct packed_ref_cache *get_packed_ref_cache(struct files_ref_store 
> *refs)
>  {
>   char *packed_refs_file;
> + struct strbuf sb = STRBUF_INIT;
>  
> - if (refs->submodule)
> - packed_refs_file = git_pathdup_submodule(refs->submodule,
> -  "packed-refs");
> - else
> - packed_refs_file = git_pathdup("packed-refs");
> + files_path(refs, , "packed-refs");
> + packed_refs_file = strbuf_detach(, NULL);

But the original code of some other changes do not follow that
pattern, e.g.

> @@ -1585,7 +1578,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_path(refs, _file, "%s", refname);

Is it the right way to review these changes to make sure that a
conversion from the original that is an unconditional
strbuf_git_path() to files_path() happens only if the function is
"files-assert-main-repository" clean?  lock_raw_ref() certainly is
one of those functions where the caller should not have a non-empty
submodule field in refs.

> @@ -2052,7 +2045,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_path(refs, _file, "%s", refname);

So is this one; lock_ref_sha1_basic() is protected with assert-main-repo.

> @@ -2343,7 +2336,7 @@ static int pack_if_possible_fn(struct ref_entry *entry, 
> void *cb_data)
>   * Remove empty parents, but spare refs/ and immediate subdirs.
>   * Note: munges *name.
>   */
> -static void try_remove_empty_parents(char *name)
> +static void try_remove_empty_parents(struct files_ref_store *refs, char 
> *name)
>  {
>   char *p, *q;
>   int i;
> @@ -2368,7 +2361,7 @@ static void try_remove_empty_parents(char *name)
>   if (q == p)
>   break;
>   *q = '\0';
> - strbuf_git_path(, "%s", name);
> + files_path(refs, , "%s", name);

But here it gets iffy.  try_remove_empty_parents() itself does not
assert, and its sole caller prune_ref() does not, either.  The sole
caller of prune_ref() which is prune_refs() does not.  As we climb
the call chain up, we reach files_pack_refs().  Am I confused to
doubt that the method is inherently main-repo only?

... ah, OK, files_downcast() at the beginning of pack_refs
forbids submodule.  So this is safe.


> @@ -2462,7 +2455,7 @@ static int repack_without_refs(struct files_ref_store 
> *refs,
>   if (lock_packed_refs(refs, 0)) {
>   struct strbuf sb = STRBUF_INIT;
>  
> - strbuf_git_path(, "packed-refs");
> + files_path(refs, , "packed-refs");

This is safe, as repack_without_refs() asserts that it is main-repo
only.

> @@ -2558,17 +2551,17 @@ static int files_delete_refs(struct ref_store 
> *ref_store,
>   */
>  #define TMP_RENAMED_LOG  "logs/refs/.tmp-renamed-log"
>  
> -static int rename_tmp_log(const char *newrefname)
> +static int rename_tmp_log(struct files_ref_store *refs, const char 
> *newrefname)
>  {

The sole caller files_rename_ref() is main-repo only and that is
guaranteed when downcast is done.

> -static int log_ref_setup(const char *refname, struct strbuf *logfile, struct 
> strbuf *err, int force_create)
> +static int log_ref_setup(struct files_ref_store *refs, const char *refname,
> +  struct strbuf *logfile, struct strbuf *err,
> +  int force_create)
>  {
>   int logfd, oflags = O_APPEND | O_WRONLY;
>  
> - strbuf_git_path(logfile, "logs/%s", refname);
> + files_path(refs, logfile, "logs/%s", refname);

This and friends of log_ref_write() eventually rolls up to
commit_ref_update() that has the main-repo only assertion, so they
should be safe.

Another 

Re: [PATCH v3 03/16] files-backend: add files_path()

[Junio C Hamano]

Nguyễn Thái Ngọc Duy   writes:

> This will be the replacement for both git_path() and git_path_submodule()
> in this file. The idea is backend takes a git path and use that,
> oblivious of submodule, linked worktrees and such.
>
> This is the middle step towards that. Eventually the "submodule" field
> in 'struct files_ref_store' should be replace by "gitdir". And a

s/replace//;

> compound ref_store is created to combine two files backends together,
> one represents the shared refs in $GIT_COMMON_DIR, one per-worktree. At
> that point, files_path() becomes a wrapper of strbuf_vaddf().

I like the general direction, obviously ;-)

>
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  refs/files-backend.c | 18 ++
>  1 file changed, 18 insertions(+)
>
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index f0c878b92..abb8a95e0 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -930,6 +930,24 @@ struct files_ref_store {
>  /* Lock used for the main packed-refs file: */
>  static struct lock_file packlock;
>  
> +__attribute__((format (printf, 3, 4)))
> +static void files_path(struct files_ref_store *refs, struct strbuf *sb,
> +const char *fmt, ...)
> +{
> + struct strbuf tmp = STRBUF_INIT;
> + va_list vap;
> +
> + va_start(vap, fmt);
> + strbuf_vaddf(, fmt, vap);
> + va_end(vap);
> + if (refs->submodule)
> + strbuf_git_path_submodule(sb, refs->submodule,
> +   "%s", tmp.buf);
> + else
> + strbuf_git_path(sb, "%s", tmp.buf);
> + strbuf_release();
> +}
> +
>  /*
>   * Increment the reference count of *packed_refs.
>   */

[PATCH] l10n: de.po: translate 241 messages

[Ralf Thielow]

Translate 241 messages came from git.pot update in 673bfad09
(l10n: git.pot: v2.12.0 round 1 (239 new, 15 removed)) and a4d94835a
(l10n: git.pot: v2.12.0 round 2 (2 new)).

Signed-off-by: Ralf Thielow 
---
 po/de.po | 746 ++-
 1 file changed, 407 insertions(+), 339 deletions(-)

diff --git a/po/de.po b/po/de.po
index 2326da1fd..1fd4cbb1e 100644
--- a/po/de.po
+++ b/po/de.po
@@ -916,17 +916,17 @@ msgstr ""
 msgid ""
 "The merge base %s is %s.\n"
 "This means the first '%s' commit is between %s and [%s].\n"
 msgstr ""
 "Die Merge-Basis %s ist %s.\n"
 "Das bedeutet, der erste '%s' Commit befindet sich zwischen %s und [%s]\n"
 
 #: bisect.c:750
-#, fuzzy, c-format
+#, c-format
 msgid ""
 "Some %s revs are not ancestors of the %s rev.\n"
 "git bisect cannot work properly in this case.\n"
 "Maybe you mistook %s and %s revs?\n"
 msgstr ""
 "Manche %s Commits sind keine Vorgänger des %s Commits.\n"
 "git bisect kann in diesem Fall nicht richtig arbeiten.\n"
 "Vielleicht verwechselten Sie %s und %s Commits?\n"
@@ -1343,19 +1343,19 @@ msgid "bad zlib compression level %d"
 msgstr "ungültiger zlib Komprimierungsgrad %d"
 
 #: config.c:993
 #, c-format
 msgid "invalid mode for object creation: %s"
 msgstr "Ungültiger Modus für Objekterstellung: %s"
 
 #: config.c:1149
-#, fuzzy, c-format
+#, c-format
 msgid "bad pack compression level %d"
-msgstr "Komprimierungsgrad für Paketierung"
+msgstr "ungültiger Komprimierungsgrad (%d) für Paketierung"
 
 #: config.c:1339
 msgid "unable to parse command-line config"
 msgstr ""
 "Konnte die über die Befehlszeile angegebene Konfiguration nicht parsen."
 
 #: config.c:1389
 msgid "unknown error occurred while reading the configuration files"
@@ -1375,19 +1375,19 @@ msgid "bad config variable '%s' in file '%s' at line %d"
 msgstr "ungültige Konfigurationsvariable '%s' in Datei '%s' bei Zeile %d"
 
 #: config.c:1804
 #, c-format
 msgid "%s has multiple values"
 msgstr "%s hat mehrere Werte"
 
 #: config.c:2225 config.c:2450
-#, fuzzy, c-format
+#, c-format
 msgid "fstat on %s failed"
-msgstr "\"stash\" fehlgeschlagen"
+msgstr "fstat auf %s fehlgeschlagen"
 
 #: config.c:2343
 #, c-format
 msgid "could not set '%s' to '%s'"
 msgstr "Konnte '%s' nicht zu '%s' setzen."
 
 #: config.c:2345
 #, c-format
@@ -1616,19 +1616,19 @@ msgstr "Fehler beim Sammeln von Namen und Informationen 
zum Kernel"
 
 #: dir.c:1981
 msgid "Untracked cache is disabled on this system or location."
 msgstr ""
 "Cache für unversionierte Dateien ist auf diesem System oder\n"
 "für dieses Verzeichnis deaktiviert."
 
 #: dir.c:2759
-#, fuzzy, c-format
+#, c-format
 msgid "could not migrate git directory from '%s' to '%s'"
-msgstr "Konnte Verzeichnis '%s' nicht erstellen."
+msgstr "Konnte Git-Verzeichnis nicht von '%s' nach '%s' migrieren."
 
 #: fetch-pack.c:213
 msgid "git fetch-pack: expected shallow list"
 msgstr "git fetch-pack: erwartete shallow-Liste"
 
 #: fetch-pack.c:225
 msgid "git fetch-pack: expected ACK/NAK, got EOF"
 msgstr "git fetch-pack: ACK/NAK erwartet, EOF bekommen"
@@ -1803,17 +1803,17 @@ msgstr "konnte temporäre Datei nicht erstellen"
 #: gpg-interface.c:217
 #, c-format
 msgid "failed writing detached signature to '%s'"
 msgstr "Fehler beim Schreiben der losgelösten Signatur nach '%s'"
 
 #: graph.c:96
 #, c-format
 msgid "ignore invalid color '%.*s' in log.graphColors"
-msgstr ""
+msgstr "Ignoriere ungültige Farbe '%.*s' in log.graphColors"
 
 #: grep.c:1794
 #, c-format
 msgid "'%s': unable to read %s"
 msgstr "'%s': konnte %s nicht lesen"
 
 #: grep.c:1811 builtin/clone.c:381 builtin/diff.c:81 builtin/rm.c:133
 #, c-format
@@ -2320,17 +2320,17 @@ msgstr "%s: 'literal' und 'glob' sind inkompatibel"
 #: pathspec.c:363
 #, c-format
 msgid "%s: '%s' is outside repository"
 msgstr "%s: '%s' liegt außerhalb des Repositories"
 
 #: pathspec.c:451
 #, c-format
 msgid "'%s' (mnemonic: '%c')"
-msgstr ""
+msgstr "'%s' (Kürzel: '%c')"
 
 #: pathspec.c:461
 #, c-format
 msgid "%s: pathspec magic not supported by this command: %s"
 msgstr ""
 "%s: Pfadspezifikationsangabe wird von diesem Befehl nicht unterstützt: %s"
 
 #: pathspec.c:511
@@ -2418,19 +2418,19 @@ msgid "%%(body) does not take arguments"
 msgstr "%%(body) akzeptiert keine Argumente"
 
 #: ref-filter.c:85
 #, c-format
 msgid "%%(subject) does not take arguments"
 msgstr "%%(subject) akzeptiert keine Argumente"
 
 #: ref-filter.c:92
-#, fuzzy, c-format
+#, c-format
 msgid "%%(trailers) does not take arguments"
-msgstr "%%(body) akzeptiert keine Argumente"
+msgstr "%%(trailers) akzeptiert keine Argumente"
 
 #: ref-filter.c:111
 #, c-format
 msgid "positive value expected contents:lines=%s"
 msgstr "Positiver Wert erwartet contents:lines=%s"
 
 #: ref-filter.c:113
 #, c-format
@@ -2613,32 +2613,30 @@ msgstr "  (benutzen Sie \"git branch --unset-upstream\" 
zum Beheben)\n"
 #, c-format
 msgid "Your branch is up-to-date with '%s'.\n"
 msgstr "Ihr Branch ist auf 

Re: body-CC-comment regression

[Matthieu Moy]

Junio C Hamano  writes:

> Johan Hovold  writes:
>
>> That's precisely what the patch I posted earlier in the thread did.
>
> That's good.  I didn't see any patch yet 

It's here:

http://public-inbox.org/git/20170217110642.GD2625@localhost/

but as I explained, this removes a feature suported since several major
releases and we have no idea how many users may use the "mupliple emails
in one field". The approach I proposed does not suffer from this.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH 12/15] unpack-trees: check if we can perform the operation for submodules

[Jacob Keller]

On Wed, Feb 15, 2017 at 4:38 PM, Stefan Beller  wrote:
> +   if (is_active_submodule_with_strategy(ce, SM_UPDATE_UNSPECIFIED))

Here, and in other cases where we use
is_active_submodule_with_strategy(), why do we only ever check
SM_UPDATE_UNSPECIFIED? It seems really weird that we're only going to
check submodules who's strategy is unspecified, when that defaults to
checkout if I recall correctly? Shouldn't we check both? This applies
to pretty much everywhere that you call this function that I noticed,
which is why I removed the context.

Thanks,
Jake

Re: [PATCH 08/15] submodules: introduce check to see whether to touch a submodule

[Jacob Keller]

On Wed, Feb 15, 2017 at 4:38 PM, Stefan Beller  wrote:
> In later patches we introduce the --recurse-submodule flag for commands
> that modify the working directory, e.g. git-checkout.
>
> It is potentially expensive to check if a submodule needs an update,
> because a common theme to interact with submodules is to spawn a child
> process for each interaction.
>
> So let's introduce a function that checks if a submodule needs
> to be checked for an update before attempting the update.
>
> Signed-off-by: Stefan Beller 
> ---
>  submodule.c | 27 +++
>  submodule.h | 13 +
>  2 files changed, 40 insertions(+)
>
> diff --git a/submodule.c b/submodule.c
> index 591f4a694e..2a37e03420 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -548,6 +548,33 @@ void set_config_update_recurse_submodules(int value)
> config_update_recurse_submodules = value;
>  }
>
> +int touch_submodules_in_worktree(void)
> +{
> +   /*
> +* Update can't be "none", "merge" or "rebase",
> +* treat any value as OFF, except an explicit ON.
> +*/
> +   return config_update_recurse_submodules == RECURSE_SUBMODULES_ON;
> +}

Ok, so here, we're just checking whether the value is
RECURSE_SUBMODULES_ON. The comment doesn't make sense to me at all.
What is "update" and why "can't" it be those values? How is this code
treating thise values as OFF exept for an explicit ON?

At first I thought this comment was related to check in the previous
patch. I think I see the patch is "recurse submodules = true" or
"recurse submodules = checkout" as a specific strategy? Ie:
recurse-submodules=checkout" means "recurse into submodules and update
them using checkout strategy?

Ok this starts to make a bit more sense. However, it's still somewhat
confusing to me.

Maybe this should be called something like
recurse_into_submodules_in_worktree() though that is pretty verbose.

I'm not sure I have a good name really. But I wonder why we need this
in the first place. Basically, we set RECURSE_SUBMODULES_* value which
could be OFF, ON, or even future extensions of CHECKOUT, REBASE,
MERGE, etc?

But shouldn't we just return the mode, and let the later code decide
"oh. actually I don't support this mode". For now, obviously we
wouldn't set any of the new modes yet.

> +
> +int is_active_submodule_with_strategy(const struct cache_entry *ce,
> + enum submodule_update_type strategy)
> +{
> +   const struct submodule *sub;
> +
> +   if (!S_ISGITLINK(ce->ce_mode))
> +   return 0;
> +
> +   if (!touch_submodules_in_worktree())
> +   return 0;
> +
> +   sub = submodule_from_path(null_sha1, ce->name);
> +   if (!sub)
> +   return 0;
> +
> +   return sub->update_strategy.type == strategy;
> +}
> +

I liked Junio's suggestion where this just returns the strategy that
it can use, or 0 if it shouldn't be updated. Then, other code just
decides: Yes, I can use this strategy or no I cannot.

Should this be tied in with the strategy used by the
recurse_submodules above? ie: when/if we support recursing using other
strategies, shouldn't we make these match here, so that if the recurse
mode is "checkout" we don't checkout a submodule that was configured
to be rebased? Or do you want to blindly apply checkout to all
submodules even if they don't have strategy?

I assume you do not, since you check here with passing a strategy
value, and then see if it matches.

this could be named something like:

get_active_submodule_strategy() and return the strategy directly. It
would then return 0 in any case where it shouldn't be updated. Later
code can then check the strategy.

>  static int has_remote(const char *refname, const struct object_id *oid,
>   int flags, void *cb_data)
>  {
> diff --git a/submodule.h b/submodule.h
> index b4e60c08d2..46d9f0f293 100644
> --- a/submodule.h
> +++ b/submodule.h
> @@ -65,6 +65,19 @@ extern void show_submodule_inline_diff(FILE *f, const char 
> *path,
> const struct diff_options *opt);
>  extern void set_config_fetch_recurse_submodules(int value);
>  extern void set_config_update_recurse_submodules(int value);
> +
> +/*
> + * Traditionally Git ignored changes made for submodules.
> + * This function checks if we are interested in the given submodule
> + * for any kind of operation.

This comment seems a bit weird.

> + */
> +extern int touch_submodules_in_worktree(void);
> +/*
> + * Check if the given ce entry is a submodule with the given update
> + * strategy configured.

I like Junio's suggestion of this "getting the current configured
strategy for a submodule. When we aren't set to recurse into
submodules we (obviously) return that we have no strategy since we're
not going to update it at all.

> + */
> +extern int is_active_submodule_with_strategy(const struct cache_entry *ce,
> +  

Re: [PATCH v2 00/16] Remove submodule from files-backend.c

[Junio C Hamano]

Nguyễn Thái Ngọc Duy   writes:

> I'll be sending two more follow-up series, if you are interested, soon:
>
> 1) v2 of nd/worktree-gc-protection
>
> which kills parse_ref() in worktree.c _and_ set_worktree_head_symref()
> in files-backend.c. Both are bad things that should not have happened.
> (PS. The topic name is misleading as this is mostly about eliminating
> warts, unless Junio intended to combine my second series as well)

Your description sounded that these two are just preparatory step
for the main one that would soon follow, and that was why these two
patches landed on a topic named as such without any of its friends
(which were yet to come).  If you prefer to keep these a separate
preparatory step from the remainder and have them graduate sooner,
let's do so, as that is my preference as well.  

Rename it "nd/worktree-kill-parse-ref" perhaps?

> This series introduces get_worktree_ref_store() and adds two new APIs
> refs_resolve_ref_unsafe() and refs_create_symref(). I'm still not sure
> if the refs_ prefix is good naming, but I had to pick something to get
> things going. Name suggestions are welcome.

> 2) the real worktree-gc-protection
>
> This series adds a bunch of new refs API, enough for revision.c to
> traverses all sorts of refs with a ref store instead of "submodule".
> Many _submodule API are removed as a result because they no longer
> have any callers (yay!). One of them remains though.

Yay indeed.

Re: body-CC-comment regression

[Junio C Hamano]

Johan Hovold  writes:

> That's precisely what the patch I posted earlier in the thread did.

That's good.  I didn't see any patch yet but the message you are
responding to is a response to Matthieu's message asking if you are
planning to work on it, so I'd assume you are and and look forward
to seeing a patch (or a series?) we can queue.

Thanks.

Re: [PATCH v3 0/2] Fix bugs in rev-parse's output when run in a subdirectory

[Junio C Hamano]

Johannes Schindelin  writes:

> The bug that bit me (hard!) and that triggered not only a long series of
> curses but also my writing a patch and sending it to the list was that
> `git rev-parse --git-path HEAD` would give *incorrect* output when run
> in a subdirectory of a regular checkout, but *correct* output when run
> in a subdirectory of an associated *worktree*.
>
> I had tested the script in question quite a bit, but in a worktree. And
> in production, it quietly did exactly the wrong thing.
>
> Changes relative to v2:
>
> - the "iffy" test in t1700 was made "uniffy"
>
> - clarified in the commit message of 2/2 why we can get away with the
>   "reset then use" pattern

It is no longer relevant between "reset then use" and "use then
reset", I think, because you did something much better, which is to
move strbuf_release() up so that it comes before the possible early
returns.

Both patches look good.  Let's queue this and move it to 'next'
shortly.  Personally, I think it is OK to fast-track this to
'master' before the final, but just like any other bugs, we've lived
with the bug for some time, and it is not a big deal if we have to
live with it for a bit longer.

Thanks.

Re: [PATCH 06/15] update submodules: add submodule config parsing

[Jacob Keller]

On Wed, Feb 15, 2017 at 4:38 PM, Stefan Beller  wrote:
> Similar to b33a15b08 (push: add recurseSubmodules config option,
> 2015-11-17) and 027771fcb1 (submodule: allow erroneous values for the
> fetchRecurseSubmodules option, 2015-08-17), we add submodule-config code
> that is later used to parse whether we are interested in updating
> submodules.
>
> We need the `die_on_error` parameter to be able to call this parsing
> function for the config file as well, which if incorrect lets Git die.
>
> As we're just touching the header file, also mark all functions extern.
>
> Signed-off-by: Stefan Beller 
> ---
>  submodule-config.c | 22 ++
>  submodule-config.h | 17 +
>  2 files changed, 31 insertions(+), 8 deletions(-)
>
> diff --git a/submodule-config.c b/submodule-config.c
> index 93453909cf..93f01c4378 100644
> --- a/submodule-config.c
> +++ b/submodule-config.c
> @@ -234,6 +234,28 @@ int parse_fetch_recurse_submodules_arg(const char *opt, 
> const char *arg)
> return parse_fetch_recurse(opt, arg, 1);
>  }
>
> +static int parse_update_recurse(const char *opt, const char *arg,
> +   int die_on_error)
> +{
> +   switch (git_config_maybe_bool(opt, arg)) {
> +   case 1:
> +   return RECURSE_SUBMODULES_ON;
> +   case 0:
> +   return RECURSE_SUBMODULES_OFF;
> +   default:
> +   if (!strcmp(arg, "checkout"))
> +   return RECURSE_SUBMODULES_ON;
> +   if (die_on_error)
> +   die("bad %s argument: %s", opt, arg);
> +   return RECURSE_SUBMODULES_ERROR;
> +   }
> +}

Ok so this function here reads a recurse submodules parameter which is
a boolean or it can be set to the word "checkout"? Why does checkout
need its own value separate from true? Just so that we have a synonym?
or so that we can expand on it in the future?

Re: body-CC-comment regression

[Johan Hovold]

On Fri, Feb 17, 2017 at 10:18:46AM -0800, Junio C Hamano wrote:
> Matthieu Moy  writes:
> 
> >> On Fri, Feb 17, 2017 at 02:16:42PM +0100, Matthieu Moy wrote:
> > ...
> > If I had a time machine, I'd probably go back then and forbid multiple
> > addresses there, but ...
> >
> >> There does not seem to be single commit in the kernel where multiple
> >> address are specified in a CC tag since after git-send-email started
> >> allowing it, but there are ten commits before (to my surprise), and that
> >> should be contrasted with at least 4178 commits with trailing comments
> >> including a # sign.
> >
> > Hey, there's a life outside the kernel ;-).
> > ...
> >>> 1) Stop calling Mail::Address even if available.[...]
> >>
> >> Right, that sounds like the right thing to do regardless.
> >>
> >>> 2) Modify our in-house parser to discard garbage after the >. [...]
> >>
> >> Sounds perfectly fine to me, and seems to work too after quick test.
> >
> > OK, sounds like the way to go.
> >
> > Do you want to work on a patch? If not, I should be able to do that
> > myself. The code changes are straightforward, but we probably want a
> > proper test for that.
> 
> The true headers and the things at the bottom seem to be handled in
> a separate loop in send-email, so treating Cc: found in the former
> and in the latter differently should be doable.  I think it is OK to
> explicitly treat the latter as "these are not e-mail addresses, but
> just a single e-mail address possibly with non-address cruft",
> without losing the ability to have more than one addresses on a
> single CC: e-mail header.

That's precisely what the patch I posted earlier in the thread did.

Thanks,
Johan

Re: body-CC-comment regression

[Junio C Hamano]

Matthieu Moy  writes:

>> On Fri, Feb 17, 2017 at 02:16:42PM +0100, Matthieu Moy wrote:
> ...
> If I had a time machine, I'd probably go back then and forbid multiple
> addresses there, but ...
>
>> There does not seem to be single commit in the kernel where multiple
>> address are specified in a CC tag since after git-send-email started
>> allowing it, but there are ten commits before (to my surprise), and that
>> should be contrasted with at least 4178 commits with trailing comments
>> including a # sign.
>
> Hey, there's a life outside the kernel ;-).
> ...
>>> 1) Stop calling Mail::Address even if available.[...]
>>
>> Right, that sounds like the right thing to do regardless.
>>
>>> 2) Modify our in-house parser to discard garbage after the >. [...]
>>
>> Sounds perfectly fine to me, and seems to work too after quick test.
>
> OK, sounds like the way to go.
>
> Do you want to work on a patch? If not, I should be able to do that
> myself. The code changes are straightforward, but we probably want a
> proper test for that.

The true headers and the things at the bottom seem to be handled in
a separate loop in send-email, so treating Cc: found in the former
and in the latter differently should be doable.  I think it is OK to
explicitly treat the latter as "these are not e-mail addresses, but
just a single e-mail address possibly with non-address cruft",
without losing the ability to have more than one addresses on a
single CC: e-mail header.

Re: [PATCH 3/3] rename_ref: replace empty deletion message in HEAD's log

[Junio C Hamano]

Jeff King  writes:

> On Thu, Feb 16, 2017 at 10:58:00PM -0500, Kyle Meyer wrote:
>
>> When the current branch is renamed, the deletion of the old ref is
>> recorded in HEAD's log with an empty message.  Now that delete_refs()
>> accepts a reflog message, provide a more descriptive message.  This
>> message will not be included in the reflog of the renamed branch, but
>> its log already covers the renaming event with a message of "Branch:
>> renamed ...".
>
> Right, makes sense. The code overall looks fine, though I have one
> nit:
>
>> @@ -2616,10 +2617,15 @@ static int files_rename_ref(struct ref_store 
>> *ref_store,
>>  return error("unable to move logfile logs/%s to 
>> "TMP_RENAMED_LOG": %s",
>>  oldrefname, strerror(errno));
>>  
>> -if (delete_ref(oldrefname, orig_sha1, REF_NODEREF, NULL)) {
>> +strbuf_addf(_del, "Deleted %s", oldrefname);
>
> We've been anything but consistent with our reflog messages in the past,
> but I think the general guideline for new ones is to use "command:
> action". Of course we don't _know_ the action here, because we're deep
> in rename_ref().
>
> Should we have the caller pass it in for us, as we do with delete_ref()
> and update_ref()?
>
> I see we actually already have a "logmsg" parameter. It already says
> "Branch: renamed %s to %s". Could we just reuse that? I know that this
> step is technically just the deletion, but I think it more accurately
> describes the whole operation that the deletion is part of.

True, but stepping back a bit,...

Do we even want these "internal" delete_ref() invocations to be
logged in HEAD's reflog?  I understand that this is inside the
implementation of renaming an old ref to a new ref, and reflog
message given to delete_ref() would matter only if the HEAD happens
to be pointing at old ref---but then HEAD will be repointed to the
new ref by somebody else [*1*] that called this function to rename
old to new and it _will_ log it.  So I am not sure if it is a good
thing to describe the deletion more readably with a message (which
is what this patch does) in the first place.  If we can just say
"don't log this deletion event in HEAD's reflog", wouldn't that be
more desirable?


[Footnote]

*1* Is the reason why the code in files_rename_ref() we are looking
at does not adjust HEAD to point at the new ref is because it is
just handing one ref-store and obviouvious to symrefs in other
backends?

Re: body-CC-comment regression

[Linus Torvalds]

On Fri, Feb 17, 2017 at 5:16 AM, Matthieu Moy
 wrote:
>
> I mostly agree for the SoB, but why should a Cc tag have only one email?

Because changing that clearly broke real and useful behavior.

The "multiple email addresses"  thing is bogus and wrong. Just don't do it.

How would you even parse it sanely? Are the Cc: lines now
SMTP-compliant with the whole escaping and all the usual "next line"
rules?

For example, in email, the rule for "next line" is that if you're in a
header block, and it starts with whitespace, then it's a continuation
of the last line.

That's *not* how Cc: lines work in commit messages. They are all
individual lines, and we have lots of tools (mainly just scripts with
grepping) that simply depend on it.

So this notion that the bottom of the commit message is some email
header crap is WRONG.

Stop it. It caused bugs. It's wrong. Don't do it.

   Linus

Re: body-CC-comment regression

[Johan Hovold]

On Fri, Feb 17, 2017 at 05:58:11PM +0100, Matthieu Moy wrote:
> > On Fri, Feb 17, 2017 at 02:16:42PM +0100, Matthieu Moy wrote:
> >> Johan Hovold  writes:
> >
> >> The "multiple emails per Cc: field" has been there for a while already
> >> (b1c8a11c8024 released in 2.6.0, sept 2015), some users may have got
> >> used to it. What you are proposing breaks their flow.
> >
> > Note that that commit never mentions multiple addresses in either
> > headers or body-tags -- it's all about being able to specify multiple
> > entries on the command line.
> 
> Indeed. I'm not the author of the patch, but I was supervising the
> students who wrote it and "multiple addresses in Cc:" was not the goal,
> but a (IMHO positive) side effect we discovered after the fact.

Yeah, and the broken --suppress-cc=self I mention below is indicative
of that too.

> If I had a time machine, I'd probably go back then and forbid multiple
> addresses there, but ...
> 
> > There does not seem to be single commit in the kernel where multiple
> > address are specified in a CC tag since after git-send-email started
> > allowing it, but there are ten commits before (to my surprise), and that
> > should be contrasted with at least 4178 commits with trailing comments
> > including a # sign.
> 
> Hey, there's a life outside the kernel ;-).

Sure, but it's the origin of git as well as the tags we're discussing (I
believe).

My point of bringing it up was that the multiple addresses in a CC-tag
was indeed an unintended (and undocumented) side-effect and I doubt many
people have started using it given that it's sort of counter-intuitive
(again, compare with SoB).

If either the trailing comments or multiple addresses in a CC-tag has to
go, I think dropping the latter is clearly the best choice.

> >> 1) Stop calling Mail::Address even if available.[...]
> >
> > Right, that sounds like the right thing to do regardless.
> >
> >> 2) Modify our in-house parser to discard garbage after the >. [...]
> >
> > Sounds perfectly fine to me, and seems to work too after quick test.
> 
> OK, sounds like the way to go.
> 
> Do you want to work on a patch? If not, I should be able to do that
> myself. The code changes are straightforward, but we probably want a
> proper test for that.

Feel free to implement it this way if that's what people prefer. As long
as trailing comments are supported and discarded, I don't really have a
preference.

> > addresses in a Cc-tag in that it breaks --suppress-cc=self, but I guess
> > that can be fixed separately.
> 
> OK. If it's unrelated enough, please start a separate thread to explain
> the problem (and/or write a patch ;-) ).

Well, it's related to the "offending" patch that added support for
multiple addresses in tags. By disallowing that, as my fix does, the
problem goes away.

# Now parse the message body
while(<$fh>) {
$message .=  $_;
if (/^(Signed-off-by|Cc): (.*)$/i) {
chomp;
my ($what, $c) = ($1, $2);
chomp $c;
my $sc = sanitize_address($c);

if ($sc eq $sender) {
next if ($suppress_cc{'self'});

The problem here is that $sc will never match $sender when there are more
than one address in a tag. For example:

From: Johan Hovold 
...

Cc: alpha , Johan Hovold 

results in

sc = alpha , Johan Hovold 
sender = Johan Hovold 

so that --suppress-cc=self is not honoured.

Thanks,
Johan

[L10N] Kickoff for Git 2.12.0 l10n round 2

[Jiang Xin]

Hi guys,

Git v2.12.0-rc1 introduced another two new messages need to be translated,
so let's start l10n for Git 2.12.0 round 2:

l10n: git.pot: v2.12.0 round 2 (2 new)

Generate po/git.pot from v2.12.0-rc1 for git v2.12.0 l10n round 2.

Signed-off-by: Jiang Xin 

You can get it from the usual place:

https://github.com/git-l10n/git-po/

As how to update your XX.po and help to translate Git, please see
"Updating a XX.po file" and other sections in “po/README" file.

--
Jiang Xin

Re: [PATCH 2/3] update-ref: pass reflog message argument to delete_refs

[Junio C Hamano]

Jeff King  writes:

> ...

All good review comments.

I briefly wondered if recording the deletion of the current branch
in HEAD's reflog has much practical values (Porcelain "git branch"
would not even allow deletion in the first place), but because it is
a rare and unusual event, it probably makes more sense to have a
record of it.

Re: [PATCH/RFC 00/15] Fix git-gc losing objects in multi worktree

[Johannes Schindelin]

Hi Duy,

On Fri, 17 Feb 2017, Nguyễn Thái Ngọc Duy wrote:

> So here is my latest attempt on fixing this issue. For people who are
> not aware of it, git-gc does not take per-worktree refs, reflogs and
> indexes into account. An odb prune may leave HEAD and references in
> other worktrees pointing to nowhere.

Thank you so much for working on this. The bug really affects my daily
work very, very negatively.

Will try to review as soon as possible.

Ciao,
Dscho

Re: [PATCH 1/3] delete_refs(): accept a reflog message argument

[Junio C Hamano]

Jeff King  writes:

>> diff --git a/refs.h b/refs.h
>> index 9fbff90e7..81627a63d 100644
>> --- a/refs.h
>> +++ b/refs.h
>> @@ -277,7 +277,7 @@ int reflog_exists(const char *refname);
>>   * be NULL_SHA1. flags is passed through to ref_transaction_delete().
>>   */
>>  int delete_ref(const char *refname, const unsigned char *old_sha1,
>> -   unsigned int flags);
>> +   unsigned int flags, const char *msg);
>
> Should the "msg" argument go at the beginning, to match update_ref()?

Probably.  rename/create have the message at the end but their
parameters are very different from update/delete.  The parameters
update and delete take are not identical, but we can view them as a
lot more similar than the other two.  So I think it makes sense for
delete to try matching update, even though trying to make all four
the same may proabably be pointless.

[PATCH v3 2/2] rev-parse: fix several options when running in a subdirectory

[Johannes Schindelin]

In addition to making git_path() aware of certain file names that need
to be handled differently e.g. when running in worktrees, the commit
557bd833bb (git_path(): be aware of file relocation in $GIT_DIR,
2014-11-30) also snuck in a new option for `git rev-parse`:
`--git-path`.

On the face of it, there is no obvious bug in that commit's diff: it
faithfully calls git_path() on the argument and prints it out, i.e. `git
rev-parse --git-path ` has the same precise behavior as
calling `git_path("")` in C.

The problem lies deeper, much deeper. In hindsight (which is always
unfair), implementing the .git/ directory discovery in
`setup_git_directory()` by changing the working directory may have
allowed us to avoid passing around a struct that contains information
about the current repository, but it bought us many, many problems.

In this case, when being called in a subdirectory, `git rev-parse`
changes the working directory to the top-level directory before calling
`git_path()`. In the new working directory, the result is correct. But
in the working directory of the calling script, it is incorrect.

Example: when calling `git rev-parse --git-path HEAD` in, say, the
Documentation/ subdirectory of Git's own source code, the string
`.git/HEAD` is printed.

Side note: that bug is hidden when running in a subdirectory of a
worktree that was added by the `git worktree` command: in that case, the
(correct) absolute path of the `HEAD` file is printed.

In the interest of time, this patch does not go the "correct" route to
introduce a struct with repository information (and removing global
state in the process), instead this patch chooses to detect when the
command was called in a subdirectory and forces the result to be an
absolute path.

While at it, we are also fixing the output of --git-common-dir and
--shared-index-path.

Lastly, please note that we reuse the same strbuf for all of the
relative_path() calls; this avoids frequent allocation (and duplicated
code), and it does not risk memory leaks, for two reasons: 1) the
cmd_rev_parse() function does not return anywhere between the use of
the new strbuf instance and its final release, and 2) git-rev-parse is
one of these "one-shot" programs in Git, i.e. it exits after running
for a very short time, meaning that all allocated memory is released
with the exit() call anyway.

Signed-off-by: Johannes Schindelin 
---
 builtin/rev-parse.c  | 15 +++
 t/t1500-rev-parse.sh |  4 ++--
 t/t1700-split-index.sh   |  2 +-
 t/t2027-worktree-list.sh |  4 ++--
 4 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index ff13e59e1db..2cfd8d2aae4 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -545,6 +545,7 @@ int cmd_rev_parse(int argc, const char **argv, const char 
*prefix)
unsigned int flags = 0;
const char *name = NULL;
struct object_context unused;
+   struct strbuf buf = STRBUF_INIT;
 
if (argc > 1 && !strcmp("--parseopt", argv[1]))
return cmd_parseopt(argc - 1, argv + 1, prefix);
@@ -599,7 +600,9 @@ int cmd_rev_parse(int argc, const char **argv, const char 
*prefix)
if (!strcmp(arg, "--git-path")) {
if (!argv[i + 1])
die("--git-path requires an argument");
-   puts(git_path("%s", argv[i + 1]));
+   strbuf_reset();
+   puts(relative_path(git_path("%s", argv[i + 1]),
+  prefix, ));
i++;
continue;
}
@@ -821,8 +824,9 @@ int cmd_rev_parse(int argc, const char **argv, const char 
*prefix)
continue;
}
if (!strcmp(arg, "--git-common-dir")) {
-   const char *pfx = prefix ? prefix : "";
-   puts(prefix_filename(pfx, strlen(pfx), 
get_git_common_dir()));
+   strbuf_reset();
+   puts(relative_path(get_git_common_dir(),
+  prefix, ));
continue;
}
if (!strcmp(arg, "--is-inside-git-dir")) {
@@ -845,7 +849,9 @@ int cmd_rev_parse(int argc, const char **argv, const char 
*prefix)
die(_("Could not read the index"));
if (the_index.split_index) {
const unsigned char *sha1 = 
the_index.split_index->base_sha1;
-   puts(git_path("sharedindex.%s", 
sha1_to_hex(sha1)));
+   const char *path = 
git_path("sharedindex.%s", sha1_to_hex(sha1));
+   strbuf_reset();
+  

[PATCH v3 1/2] rev-parse tests: add tests executed from a subdirectory

[Johannes Schindelin]

From: Michael Rappazzo 

t2027-worktree-list has an incorrect expectation for --git-common-dir
which has been adjusted and marked to expect failure.

Some of the tests added have been marked to expect failure.  These
demonstrate a problem with the way that some options to git rev-parse
behave when executed from a subdirectory of the main worktree.

[jes: fixed incorrect assumption that objects/ lives in the
worktree-specific git-dir (it lives in the common dir instead). Also
adjusted t1700 so that the test case does not *need* to be the last
one in that script.]

Signed-off-by: Michael Rappazzo 
Signed-off-by: Johannes Schindelin 
---
 t/t1500-rev-parse.sh | 28 
 t/t1700-split-index.sh   | 16 
 t/t2027-worktree-list.sh | 12 ++--
 3 files changed, 54 insertions(+), 2 deletions(-)

diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index 038e24c4014..f39f783f2db 100755
--- a/t/t1500-rev-parse.sh
+++ b/t/t1500-rev-parse.sh
@@ -87,4 +87,32 @@ test_rev_parse -C work -g ../repo.git -b t 
'GIT_DIR=../repo.git, core.bare = tru
 
 test_rev_parse -C work -g ../repo.git -b u 'GIT_DIR=../repo.git, core.bare 
undefined' false false true ''
 
+test_expect_success 'git-common-dir from worktree root' '
+   echo .git >expect &&
+   git rev-parse --git-common-dir >actual &&
+   test_cmp expect actual
+'
+
+test_expect_failure 'git-common-dir inside sub-dir' '
+   mkdir -p path/to/child &&
+   test_when_finished "rm -rf path" &&
+   echo "$(git -C path/to/child rev-parse --show-cdup).git" >expect &&
+   git -C path/to/child rev-parse --git-common-dir >actual &&
+   test_cmp expect actual
+'
+
+test_expect_success 'git-path from worktree root' '
+   echo .git/objects >expect &&
+   git rev-parse --git-path objects >actual &&
+   test_cmp expect actual
+'
+
+test_expect_failure 'git-path inside sub-dir' '
+   mkdir -p path/to/child &&
+   test_when_finished "rm -rf path" &&
+   echo "$(git -C path/to/child rev-parse --show-cdup).git/objects" 
>expect &&
+   git -C path/to/child rev-parse --git-path objects >actual &&
+   test_cmp expect actual
+'
+
 test_done
diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
index 292a0720fcc..b754865a618 100755
--- a/t/t1700-split-index.sh
+++ b/t/t1700-split-index.sh
@@ -200,4 +200,20 @@ EOF
test_cmp expect actual
 '
 
+test_expect_failure 'rev-parse --shared-index-path' '
+   test_create_repo split-index &&
+   (
+   cd split-index &&
+   git update-index --split-index &&
+   echo .git/sharedindex* >expect &&
+   git rev-parse --shared-index-path >actual &&
+   test_cmp expect actual &&
+   mkdir subdirectory &&
+   cd subdirectory &&
+   echo ../.git/sharedindex* >expect &&
+   git rev-parse --shared-index-path >actual &&
+   test_cmp expect actual
+   )
+'
+
 test_done
diff --git a/t/t2027-worktree-list.sh b/t/t2027-worktree-list.sh
index 465eeeacd3d..c1a072348e7 100755
--- a/t/t2027-worktree-list.sh
+++ b/t/t2027-worktree-list.sh
@@ -8,16 +8,24 @@ test_expect_success 'setup' '
test_commit init
 '
 
-test_expect_success 'rev-parse --git-common-dir on main worktree' '
+test_expect_failure 'rev-parse --git-common-dir on main worktree' '
git rev-parse --git-common-dir >actual &&
echo .git >expected &&
test_cmp expected actual &&
mkdir sub &&
git -C sub rev-parse --git-common-dir >actual2 &&
-   echo sub/.git >expected2 &&
+   echo ../.git >expected2 &&
test_cmp expected2 actual2
 '
 
+test_expect_failure 'rev-parse --git-path objects linked worktree' '
+   echo "$(git rev-parse --show-toplevel)/.git/objects" >expect &&
+   test_when_finished "rm -rf linked-tree && git worktree prune" &&
+   git worktree add --detach linked-tree master &&
+   git -C linked-tree rev-parse --git-path objects >actual &&
+   test_cmp expect actual
+'
+
 test_expect_success '"list" all worktrees from main' '
echo "$(git rev-parse --show-toplevel) $(git rev-parse --short HEAD) 
[$(git symbolic-ref --short HEAD)]" >expect &&
test_when_finished "rm -rf here && git worktree prune" &&
-- 
2.11.1.windows.1.2.g87ad093.dirty

[PATCH v3 0/2] Fix bugs in rev-parse's output when run in a subdirectory

[Johannes Schindelin]

The bug that bit me (hard!) and that triggered not only a long series of
curses but also my writing a patch and sending it to the list was that
`git rev-parse --git-path HEAD` would give *incorrect* output when run
in a subdirectory of a regular checkout, but *correct* output when run
in a subdirectory of an associated *worktree*.

I had tested the script in question quite a bit, but in a worktree. And
in production, it quietly did exactly the wrong thing.

Changes relative to v2:

- the "iffy" test in t1700 was made "uniffy"

- clarified in the commit message of 2/2 why we can get away with the
  "reset then use" pattern


Johannes Schindelin (1):
  rev-parse: fix several options when running in a subdirectory

Michael Rappazzo (1):
  rev-parse tests: add tests executed from a subdirectory

 builtin/rev-parse.c  | 15 +++
 t/t1500-rev-parse.sh | 28 
 t/t1700-split-index.sh   | 16 
 t/t2027-worktree-list.sh | 10 +-
 4 files changed, 64 insertions(+), 5 deletions(-)


base-commit: 076c05393a047247ea723896289b48d6549ed7d0
Published-As: https://github.com/dscho/git/releases/tag/git-path-in-subdir-v3
Fetch-It-Via: git fetch https://github.com/dscho/git git-path-in-subdir-v3

Interdiff vs v2:

 diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
 index 84af2802f6f..2cfd8d2aae4 100644
 --- a/builtin/rev-parse.c
 +++ b/builtin/rev-parse.c
 @@ -903,6 +903,7 @@ int cmd_rev_parse(int argc, const char **argv, const char 
*prefix)
continue;
verify_filename(prefix, arg, 1);
}
 +  strbuf_release();
if (verify) {
if (revs_count == 1) {
show_rev(type, sha1, name);
 @@ -912,6 +913,5 @@ int cmd_rev_parse(int argc, const char **argv, const char 
*prefix)
die_no_single_rev(quiet);
} else
show_default();
 -  strbuf_release();
return 0;
  }
 diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
 index 446ff34f966..6096f2c6309 100755
 --- a/t/t1700-split-index.sh
 +++ b/t/t1700-split-index.sh
 @@ -201,17 +201,16 @@ EOF
  '
  
  test_expect_success 'rev-parse --shared-index-path' '
 -  rm -rf .git &&
 -  test_create_repo . &&
 -  git update-index --split-index &&
 -  ls -t .git/sharedindex* | tail -n 1 >expect &&
 -  git rev-parse --shared-index-path >actual &&
 -  test_cmp expect actual &&
 -  mkdir work &&
 -  test_when_finished "rm -rf work" &&
 +  test_create_repo split-index &&
(
 -  cd work &&
 -  ls -t ../.git/sharedindex* | tail -n 1 >expect &&
 +  cd split-index &&
 +  git update-index --split-index &&
 +  echo .git/sharedindex* >expect &&
 +  git rev-parse --shared-index-path >actual &&
 +  test_cmp expect actual &&
 +  mkdir subdirectory &&
 +  cd subdirectory &&
 +  echo ../.git/sharedindex* >expect &&
git rev-parse --shared-index-path >actual &&
test_cmp expect actual
)

-- 
2.11.1.windows.1.2.g87ad093.dirty

Re: body-CC-comment regression

[Matthieu Moy]

> On Fri, Feb 17, 2017 at 02:16:42PM +0100, Matthieu Moy wrote:
>> Johan Hovold  writes:
>
>> The "multiple emails per Cc: field" has been there for a while already
>> (b1c8a11c8024 released in 2.6.0, sept 2015), some users may have got
>> used to it. What you are proposing breaks their flow.
>
> Note that that commit never mentions multiple addresses in either
> headers or body-tags -- it's all about being able to specify multiple
> entries on the command line.

Indeed. I'm not the author of the patch, but I was supervising the
students who wrote it and "multiple addresses in Cc:" was not the goal,
but a (IMHO positive) side effect we discovered after the fact.

If I had a time machine, I'd probably go back then and forbid multiple
addresses there, but ...

> There does not seem to be single commit in the kernel where multiple
> address are specified in a CC tag since after git-send-email started
> allowing it, but there are ten commits before (to my surprise), and that
> should be contrasted with at least 4178 commits with trailing comments
> including a # sign.

Hey, there's a life outside the kernel ;-).

>> 1) Stop calling Mail::Address even if available.[...]
>
> Right, that sounds like the right thing to do regardless.
>
>> 2) Modify our in-house parser to discard garbage after the >. [...]
>
> Sounds perfectly fine to me, and seems to work too after quick test.

OK, sounds like the way to go.

Do you want to work on a patch? If not, I should be able to do that
myself. The code changes are straightforward, but we probably want a
proper test for that.

> addresses in a Cc-tag in that it breaks --suppress-cc=self, but I guess
> that can be fixed separately.

OK. If it's unrelated enough, please start a separate thread to explain
the problem (and/or write a patch ;-) ).

Thanks,

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v2 1/2] rev-parse tests: add tests executed from a subdirectory

[Johannes Schindelin]

Hi Junio,

On Fri, 10 Feb 2017, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
> > index 292a0720fcc..1d6e27a09d8 100755
> > --- a/t/t1700-split-index.sh
> > +++ b/t/t1700-split-index.sh
> > @@ -200,4 +200,21 @@ EOF
> > test_cmp expect actual
> >  '
> >  
> > +test_expect_failure 'rev-parse --shared-index-path' '
> > +   rm -rf .git &&
> > +   test_create_repo . &&
> 
> Another thing that I notice only after merging this and other topics
> to 'pu' was that this piece needs to always come at the end of the
> script because of this removal.  It would make the test more robust
> to create a test repository for this test and work inside it.

Yes, this is indeed unnecessary and even undesirable.

I waited a couple of days to give the original author a chance to fix
this. As I want a fix really badly (because I am really embarrassed of
this bug), I adjusted t1700 appropriately and will send out v3 in a
second.

Ciao,
Johannes

Re: [PATCH v2 1/2] rev-parse tests: add tests executed from a subdirectory

[Johannes Schindelin]

Hi Junio,

On Fri, 10 Feb 2017, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
> > index 292a0720fcc..1d6e27a09d8 100755
> > --- a/t/t1700-split-index.sh
> > +++ b/t/t1700-split-index.sh
> > @@ -200,4 +200,21 @@ EOF
> > test_cmp expect actual
> >  '
> >  
> > +test_expect_failure 'rev-parse --shared-index-path' '
> > +   rm -rf .git &&
> > +   test_create_repo . &&
> > +   git update-index --split-index &&
> > +   ls -t .git/sharedindex* | tail -n 1 >expect &&
> > +   git rev-parse --shared-index-path >actual &&
> > +   test_cmp expect actual &&
> > +   mkdir work &&
> > +   test_when_finished "rm -rf work" &&
> > +   (
> > +   cd work &&
> > +   ls -t ../.git/sharedindex* | tail -n 1 >expect &&
> > +   git rev-parse --shared-index-path >actual &&
> > +   test_cmp expect actual
> > +   )
> 
> This looks iffy.

Indeed.

> If we expect multiple sharedindex* files, the first output from "ls -t"
> may or may not match the real one in use (multiple things do happen
> within a single second or whatever your filesystem's time granularity
> is).  Two "ls -t" run in this test would (hopefully) give stable
> results, but I suspect that the chance the first line in the output
> matches what --shared-index-path reports is 50% if we expect to have two
> sharedindex* files.
> 
> On the other hand, if we do not expect multiple sharedindex* files,
> use of "ls" piped to "tail" is simply misleading.
> 
> If this test can be written in such a way that there is only one
> such file that match the pattern, it would be the cleanest to
> understand and explain.  As there is only a single invocation of
> "update-index --split-index" immediately after a new repository is
> created, I suspect that the expectation to see only one sharedindex*
> file already holds (because its name is unpredictable, we still need
> to catch it with wildcard), and if that is the case, we can just
> lose "-t" and pipe to tail, i.e. "ls .git/sharedindex* >expect".

Indeed. We can expect only one sharedindex file to be present, and we do
not even have to call out to `ls` but can get away with calling `echo`
(which is a builtin in Bash).

Ciao,
Johannes

Re: [PATCH v2 2/2] rev-parse: fix several options when running in a subdirectory

[Johannes Schindelin]

Hi Junio,

On Fri, 10 Feb 2017, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
> > index ff13e59e1db..84af2802f6f 100644
> > --- a/builtin/rev-parse.c
> > +++ b/builtin/rev-parse.c
> > @@ -545,6 +545,7 @@ int cmd_rev_parse(int argc, const char **argv, const 
> > char *prefix)
> > unsigned int flags = 0;
> > const char *name = NULL;
> > struct object_context unused;
> > +   struct strbuf buf = STRBUF_INIT;
> >  
> > if (argc > 1 && !strcmp("--parseopt", argv[1]))
> > return cmd_parseopt(argc - 1, argv + 1, prefix);
> > @@ -599,7 +600,9 @@ int cmd_rev_parse(int argc, const char **argv, const 
> > char *prefix)
> > if (!strcmp(arg, "--git-path")) {
> > if (!argv[i + 1])
> > die("--git-path requires an argument");
> > -   puts(git_path("%s", argv[i + 1]));
> > +   strbuf_reset();
> > +   puts(relative_path(git_path("%s", argv[i + 1]),
> > +  prefix, ));
> > i++;
> > continue;
> > }
> > @@ -821,8 +824,9 @@ int cmd_rev_parse(int argc, const char **argv, const 
> > char *prefix)
> > continue;
> > }
> > if (!strcmp(arg, "--git-common-dir")) {
> > -   const char *pfx = prefix ? prefix : "";
> > -   puts(prefix_filename(pfx, strlen(pfx), 
> > get_git_common_dir()));
> > +   strbuf_reset();
> > +   puts(relative_path(get_git_common_dir(),
> > +  prefix, ));
> > continue;
> > }
> > if (!strcmp(arg, "--is-inside-git-dir")) {
> > @@ -845,7 +849,9 @@ int cmd_rev_parse(int argc, const char **argv, const 
> > char *prefix)
> > die(_("Could not read the index"));
> > if (the_index.split_index) {
> > const unsigned char *sha1 = 
> > the_index.split_index->base_sha1;
> > -   puts(git_path("sharedindex.%s", 
> > sha1_to_hex(sha1)));
> > +   const char *path = 
> > git_path("sharedindex.%s", sha1_to_hex(sha1));
> > +   strbuf_reset();
> > +   puts(relative_path(path, prefix, ));
> > }
> > continue;
> > }
> > @@ -906,5 +912,6 @@ int cmd_rev_parse(int argc, const char **argv, const 
> > char *prefix)
> > die_no_single_rev(quiet);
> > } else
> > show_default();
> > +   strbuf_release();
> 
> This uses "reset then use" pattern for repeated use of strbuf, and
> causes the string last held in the strbuf to leak on early return,

... which cannot happen due to the lack of an early return...

> which can be mitigated by using "use then reset" pattern.  I.e.
> 
>   if (!strcmp(arg, "--git-common-dir")) {
>   puts(relative_path(get_git_common_dir(),
>  prefix, ));
>   strbuf_reset();
>   continue;
>   }
> 
> I'd think.

This would not release the memory, though:

#define strbuf_reset(sb)  strbuf_setlen(sb, 0)

and

static inline void strbuf_setlen(struct strbuf *sb, size_t len)
{
if (len > (sb->alloc ? sb->alloc - 1 : 0))
die("BUG: strbuf_setlen() beyond buffer");
sb->len = len;
sb->buf[len] = '\0';
}

There is not a single free() statement there.

So the "use then reset" scheme would leak *just the same*.

> You'd still want to release it at the end anyway for good code hygiene,
> though.

Which I do.

Technically, this is not even necessary because all of the cmd_*()
functions are immediately followed by a call to exit(). Wasn't that the
genius idea in the early Git days, that we could simply get away with
sloppy memory management because the program exit()s shortly afterwards,
anyway? ;-)

In any case, I adjusted the commit message to clarify why the "reset then
use" scheme is correct here.

Ciao,
Johannes

Re: body-CC-comment regression

[Johan Hovold]

On Fri, Feb 17, 2017 at 02:16:42PM +0100, Matthieu Moy wrote:
> Johan Hovold  writes:
> 
> > There is another option, namely to only accept a single address for tags
> > in the body. I understand that being able to copy a CC-header to either
> > the header section or to the command line could be useful, but I don't
> > really see the point in allowing this in the tags in the body (a SoB
> > always has one address, and so should a CC-tag).
> 
> I mostly agree for the SoB, but why should a Cc tag have only one email?

For symmetry (with SoB) and readability reasons (one tag per line).
These are body tags, not mail headers, after all.

> The "multiple emails per Cc: field" has been there for a while already
> (b1c8a11c8024 released in 2.6.0, sept 2015), some users may have got
> used to it. What you are proposing breaks their flow.

Note that that commit never mentions multiple addresses in either
headers or body-tags -- it's all about being able to specify multiple
entries on the command line.

There does not seem to be single commit in the kernel where multiple
address are specified in a CC tag since after git-send-email started
allowing it, but there are ten commits before (to my surprise), and that
should be contrasted with at least 4178 commits with trailing comments
including a # sign.

> > And since this is a regression for something that has been working for
> > years that was introduced by a new feature, I also think it's reasonable
> > to (partially) revert the feature.
> 
> I'd find it rather ironic to fix your case by breaking a feature that
> has been working for more than a year :-(. What would you answer to a
> contributor comming one year from now and proposing to revert your
> reversion because it breaks his flow?

Such conflicts are not uncommon when dealing with regressions introduced
by new features, and need to be dealt with on a case-by-case basis. But
the fact that trailing comments have been properly supported for more
than four years should carry some weight.

> All that said, I think another fix would be both satisfactory for
> everyone and rather simple:
> 
> 1) Stop calling Mail::Address even if available. It used to make sense
>to do that when our in-house parser was really poor, but we now have
>something essentially as good as Mail::Address. We test our parser
>against Mail::Address and we do have a few known differences (see
>t9000), but they are really corner-cases and shouldn't matter.
>
>A good consequence of this is that we stop depending on the way Perl
>is installed to parse emails. Regardless of the current issue, I
>think it is a good thing.

Right, that sounds like the right thing to do regardless.

> 2) Modify our in-house parser to discard garbage after the >. The patch
>should look like (untested):
> 
> --- a/perl/Git.pm
> +++ b/perl/Git.pm
> @@ -903,11 +903,11 @@ sub parse_mailboxes {
> my (@addr_list, @phrase, @address, @comment, @buffer) = ();
> foreach my $token (@tokens) {
> if ($token =~ /^[,;]$/) {
> -   # if buffer still contains undeterminated strings
> -   # append it at the end of @address or @phrase
> -   if ($end_of_addr_seen) {
> -   push @phrase, @buffer;
> -   } else {
> +   # if buffer still contains undeterminated
> +   # strings append it at the end of @address,
> +   # unless we already saw the closing >, in
> +   # which case we discard it.
> +   if (!$end_of_addr_seen) {
> push @address, @buffer;
> }
>  
> What do you think?

Sounds perfectly fine to me, and seems to work too after quick test.

Note however that there's another minor issue with using multiple
addresses in a Cc-tag in that it breaks --suppress-cc=self, but I guess
that can be fixed separately.

Thanks,
Johan

RE: new git-diff switch to eliminate leading "+" and "-" characters

[Vanderhoof, Tzadik]

> From: Duy Nguyen [mailto:pclo...@gmail.com]
> Sent: Friday, February 17, 2017 6:54 AM
> To: Vanderhoof, Tzadik
> Cc: git@vger.kernel.org
> 
> > Would it make sense to develop such a switch or has there been work on
> that already?
> 
> I face this "problem" every day, but most editors nowadays have block-
> based editing that would allow you to remove a column of "+/-"
> easily. At least it has not bothered me to think of improving it.

Would a patch be welcome?

Tzadik

> --
> Duy


This e-mail, including attachments, may include confidential and/or
proprietary information, and may be used only by the person or entity
to which it is addressed. If the reader of this e-mail is not the intended
recipient or his or her authorized agent, the reader is hereby notified
that any dissemination, distribution or copying of this e-mail is
prohibited. If you have received this e-mail in error, please notify the
sender by replying to this message and delete this e-mail immediately.

RE: new git-diff switch to eliminate leading "+" and "-" characters

[Vanderhoof, Tzadik]

> From: Duy Nguyen [mailto:pclo...@gmail.com]
> Sent: Friday, February 17, 2017 6:54 AM
> To: Vanderhoof, Tzadik
> Cc: git@vger.kernel.org
> 
> On Tue, Feb 14, 2017 at 6:01 AM, Vanderhoof, Tzadik
>  wrote:
> > The output of git-diff includes lines beginning with "+" and "-" to indicate
> added and deleted lines.  A somewhat common task (at least for me) is to
> want to copy output from a "diff" (usually the deleted lines) and paste it 
> back
> into my code.
> >
> > This is quite inconvenient because of the leading "+" and "-" characters.  I
> know there are shell and IDE / editor workarounds but it would be nice if
> there was a switch to git-diff to make it leave out those characters, 
> especially
> since "--color" kind of makes those leading characters obsolete.
> 
> Color wouldn't show you new/old empty lines though (unless you use
> different background color, but I doubt that looks readable).
> 
> > Would it make sense to develop such a switch or has there been work on
> that already?
> 
> I face this "problem" every day, but most editors nowadays have block-
> based editing that would allow you to remove a column of "+/-"
> easily. At least it has not bothered me to think of improving it.

Would a patch be welcome?

Tzadik

> --
> Duy


This e-mail, including attachments, may include confidential and/or
proprietary information, and may be used only by the person or entity
to which it is addressed. If the reader of this e-mail is not the intended
recipient or his or her authorized agent, the reader is hereby notified
that any dissemination, distribution or copying of this e-mail is
prohibited. If you have received this e-mail in error, please notify the
sender by replying to this message and delete this e-mail immediately.

Re: [PATCH] mingw: make stderr unbuffered again

[Johannes Schindelin]

Hi Junio,

On Wed, 15 Feb 2017, Junio C Hamano wrote:

> Junio C Hamano  writes:
> 
> > Johannes Schindelin  writes:
> >
> >> FWIW I wish it were different, that git.git's `master` reflected more
> >> closely what the current Git for Windows version has.
> >
> > Well, we two wishing the same thing together without doing anything
> > else would make it happen.
> 
> ehh, would *not* make it happen, of course.

That is the reason why set aside a substantial part of my maintainer time
to upstream those patches. You may not see how much time it costs me, say,
to get the putty-w-args stuff upstream, but rest assured that I would not
be able to do this were it not for a company paying me to do exactly that.

> > As an experiment to see if our process can be improved, I've been
> > meaning to suggest (which is what was behind my "question at a bit
> > higher level" to Hannes [*1*]) asking you to throw me occasional
> > pull requests for changes that are only about Windows specific
> > issues, bypassing "patches on the list" for things like a hotfix to
> > js/mingw-isatty [*2*] and use of OpenSSL SHA-1 on Windows [*3*],
> > essentially treating Windows specific changes as "a sub-maintainer
> > makes pull requests" we already do with Paul, Eric and Pat.

The problem here is that Git for Windows is not a subsystem. It touches
pretty much all of Git. That is very different from the Tcl/Tk code, or
from git-svn (whose code is not shared by anything else in Git's source
code, despite the fact that it is written in Perl).

And even if you were to accept the occasional Pull Request from me, it
would not solve the even bigger problem that we essentially reject so much
expertise out there, so many potential contributions by very competent
developers who just have no desire to fight the contribution process.

> While this may ease the flow of upstreaming windows specific
> changes, we need a separate thing to address the on-going issue you
> raised in your message.  A Windows-less person would not know his

... or her...

> change to a generic code that is innocuous-looking has fallouts on
> Windows (read this sentence with "Windows" replaced with any
> specific platform name).  When somebody writes c == '/' that should
> have been written as is_dir_sep(c), you or Hannes often finds it
> during the review here, and after repeatedly seeing such reviews,
> that (slowly) rubs off on other Window-less folks.  A new code may
> still hit 'next' and 'master' with such an issue if it goes
> unnoticed during the review.

Apart from the fact that we have no prayer at coming up with a system that
keeps track of open issues (because we do not use any tracker, but instead
rely on people to remember that some thing still may not have been
addressed), there is a different problem here: you stated very explicitly
that it is okay for `pu` to be broken [*1*]. If it were different, if any
breakage would imply that a fix is really, really required lest the patch
series be evicted from `pu`, we could easily modify my Continuous Testing
setup to report more visibly what is broken. But since it is okay for `pu`
to be broken, that would just annoy everybody and people would learn to
ignore/procmail those reports.

> The CI you are setting up [*1*]

which *1*?

And... I already set it up. I just did not bother to make it more public
because the builds were broken more often than not. IIRC the entire month
of October was a solid red.

> may certainly be a step in the good direction. Having more people like
> Hannes working off of upstream may also be a great way to help the
> "forget 'next' and upstream in general" issue. Any other ideas?

The Continuous Integration is actually not so much a Continuous
Integration as it is a Continuous Testing. If it became more of a CI, that
would certainly reduce the impression that Git's bleeding edge only ever
works on Linux.

Ciao,
Johannes

Footnote *1*: You probably read between the lines that this is
unfortunate, in my opinion. It sets the mood that lets experimental (if
useful) features such as worktrees be broken for the better part of a
year.

libgit2::git_describe_workdir() fails with depth-cloned + detached head

[Lukas Lueg]

Hi,

I have a build on Travis failing with a weird error that happens while
trying to get a description from a repo. One of the integration tests
that are executed on Travis executes libgit2::git_describe_workdir()
on it's own repo and fails with code: -3, klass: 9, message: "object
not found - no match for id
(77f95f14776deb7e120a2a26f7b56abf2903bc62)".

The id 77f95f1 from the error message refers to a commit that is of no
particular interest and actually weeks behind; I never asked for it.

I was finally able to reproduce the problem by very carefully
following how Travis clones the repo:

git clone --depth=50 https://... repo_root
cd repo_root
git fetch origin +refs/pull/414/merge:
git checkout -qf FETCH_HEAD

If and only if the repo is cloned recursively, the call to
libgit2::git_describe_workdir() results in this error, always
referring to 77f95f1. If the repo is cloned without '--depth 50', no
error occurs. The repo does not even have submodules.

My local git client does the right thing and get's the latest commit
oid without problems.

Any idea what's going on here?

Best regards
Lukas

Re: new git-diff switch to eliminate leading "+" and "-" characters

[Duy Nguyen]

On Tue, Feb 14, 2017 at 6:01 AM, Vanderhoof, Tzadik
 wrote:
> The output of git-diff includes lines beginning with "+" and "-" to indicate 
> added and deleted lines.  A somewhat common task (at least for me) is to want 
> to copy output from a "diff" (usually the deleted lines) and paste it back 
> into my code.
>
> This is quite inconvenient because of the leading "+" and "-" characters.  I 
> know there are shell and IDE / editor workarounds but it would be nice if 
> there was a switch to git-diff to make it leave out those characters, 
> especially since "--color" kind of makes those leading characters obsolete.

Color wouldn't show you new/old empty lines though (unless you use
different background color, but I doubt that looks readable).

> Would it make sense to develop such a switch or has there been work on that 
> already?

I face this "problem" every day, but most editors nowadays have
block-based editing that would allow you to remove a column of "+/-"
easily. At least it has not bothered me to think of improving it.
-- 
Duy

[PATCH 15/15] rev-list: expose and document --single-worktree

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

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 Documentation/rev-list-options.txt | 8 
 revision.c | 2 ++
 2 files changed, 10 insertions(+)

diff --git a/Documentation/rev-list-options.txt 
b/Documentation/rev-list-options.txt
index 5da7cf5a8..dd773f97c 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -179,6 +179,14 @@ explicitly.
Pretend as if all objects mentioned by reflogs are listed on the
command line as ``.
 
+--single-worktree::
+   By default, all working trees will be examined by the
+   following options when there are more than one (see
+   linkgit:git-worktree[1]): `--all`, `--reflog` and
+   `--indexed-objects`.
+   This option forces them to examine the current working tree
+   only.
+
 --ignore-missing::
Upon seeing an invalid object name in the input, pretend as if
the bad input was not given.
diff --git a/revision.c b/revision.c
index ecfd6fea6..2a27e55fe 100644
--- a/revision.c
+++ b/revision.c
@@ -,6 +,8 @@ static int handle_revision_pseudo_opt(const char 
*submodule,
return error("invalid argument to --no-walk");
} else if (!strcmp(arg, "--do-walk")) {
revs->no_walk = 0;
+   } else if (!strcmp(arg, "--single-worktree")) {
+   revs->single_worktree = 1;
} else {
return 0;
}
-- 
2.11.0.157.gd943d85

[PATCH 14/15] revision.c: --reflog add HEAD reflog from all worktrees

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

Note that add_other_reflogs_to_pending() is a bit inefficient, since
it scans reflog for all refs of each worktree, including shared refs,
so the shared ref's reflog is scanned over and over again.

We could update refs API to pass "per-worktree only" flag to avoid
that. But long term we should be able to obtain a "per-worktree only"
ref store and would need to revert the changes in reflog iteration
API. So let's just wait until then.

add_reflogs_to_pending() is called by reachable.c so by default "git
prune" will examine reflog from all worktrees.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 revision.c   | 28 +++-
 t/t5304-prune.sh | 16 
 2 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/revision.c b/revision.c
index 8ee929cef..ecfd6fea6 100644
--- a/revision.c
+++ b/revision.c
@@ -1134,6 +1134,7 @@ struct all_refs_cb {
int warned_bad_reflog;
struct rev_info *all_revs;
const char *name_for_errormsg;
+   struct ref_store *refs;
 };
 
 int ref_excluded(struct string_list *ref_excludes, const char *path)
@@ -1169,6 +1170,7 @@ static void init_all_refs_cb(struct all_refs_cb *cb, 
struct rev_info *revs,
 {
cb->all_revs = revs;
cb->all_flags = flags;
+   cb->refs = NULL;
 }
 
 void clear_ref_exclusion(struct string_list **ref_excludes_p)
@@ -1237,17 +1239,41 @@ static int handle_one_reflog(const char *path, const 
struct object_id *oid,
struct all_refs_cb *cb = cb_data;
cb->warned_bad_reflog = 0;
cb->name_for_errormsg = path;
-   for_each_reflog_ent(path, handle_one_reflog_ent, cb_data);
+   refs_for_each_reflog_ent(cb->refs, path,
+handle_one_reflog_ent, cb_data);
return 0;
 }
 
+static void add_other_reflogs_to_pending(struct all_refs_cb *cb)
+{
+   struct worktree **worktrees, **p;
+
+   worktrees = get_worktrees(0);
+   for (p = worktrees; *p; p++) {
+   struct worktree *wt = *p;
+
+   if (wt->is_current)
+   continue;
+
+   cb->refs = get_worktree_ref_store(wt);
+   refs_for_each_reflog(cb->refs,
+handle_one_reflog,
+cb);
+   }
+   free_worktrees(worktrees);
+}
+
 void add_reflogs_to_pending(struct rev_info *revs, unsigned flags)
 {
struct all_refs_cb cb;
 
cb.all_revs = revs;
cb.all_flags = flags;
+   cb.refs = get_main_ref_store();
for_each_reflog(handle_one_reflog, );
+
+   if (!revs->single_worktree)
+   add_other_reflogs_to_pending();
 }
 
 static void add_cache_tree(struct cache_tree *it, struct rev_info *revs,
diff --git a/t/t5304-prune.sh b/t/t5304-prune.sh
index 683bdb031..6694c19a1 100755
--- a/t/t5304-prune.sh
+++ b/t/t5304-prune.sh
@@ -304,4 +304,20 @@ test_expect_success 'prune: handle HEAD in multiple 
worktrees' '
test_cmp third-worktree/blob actual
 '
 
+test_expect_success 'prune: handle HEAD reflog in multiple worktrees' '
+   git config core.logAllRefUpdates true &&
+   echo "lost blob for third-worktree" >expected &&
+   (
+   cd third-worktree &&
+   cat ../expected >blob &&
+   git add blob &&
+   git commit -m "second commit in third" &&
+   git reset --hard HEAD^
+   ) &&
+   git prune --expire=now &&
+   SHA1=`git hash-object expected` &&
+   git -C third-worktree show "$SHA1" >actual &&
+   test_cmp expected actual
+'
+
 test_done
-- 
2.11.0.157.gd943d85

[PATCH 13/15] files-backend: make reflog iterator go through per-worktree reflog

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

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

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 011a7e256..d429f8713 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3314,6 +3314,7 @@ struct files_reflog_iterator {
struct ref_iterator base;
 
struct dir_iterator *dir_iterator;
+   struct dir_iterator *dir_iterator2;
struct object_id oid;
 };
 
@@ -3334,6 +3335,10 @@ static int files_reflog_iterator_advance(struct 
ref_iterator *ref_iterator)
if (ends_with(diter->basename, ".lock"))
continue;
 
+   if (iter->dir_iterator2 &&
+   starts_with(diter->relative_path, "refs/bisect/"))
+   continue;
+
if (read_ref_full(diter->relative_path, 0,
  iter->oid.hash, )) {
error("bad ref for %s", diter->path.buf);
@@ -3346,7 +3351,11 @@ static int files_reflog_iterator_advance(struct 
ref_iterator *ref_iterator)
return ITER_OK;
}
 
-   iter->dir_iterator = NULL;
+   iter->dir_iterator = iter->dir_iterator2;
+   if (iter->dir_iterator2) {
+   iter->dir_iterator2 = NULL;
+   return files_reflog_iterator_advance(ref_iterator);
+   }
if (ref_iterator_abort(ref_iterator) == ITER_ERROR)
ok = ITER_ERROR;
return ok;
@@ -3367,6 +3376,12 @@ static int files_reflog_iterator_abort(struct 
ref_iterator *ref_iterator)
if (iter->dir_iterator)
ok = dir_iterator_abort(iter->dir_iterator);
 
+   if (iter->dir_iterator2) {
+   int ok2 = dir_iterator_abort(iter->dir_iterator2);
+   if (ok2 == ITER_ERROR)
+   ok = ok2;
+   }
+
base_ref_iterator_free(ref_iterator);
return ok;
 }
@@ -3389,6 +3404,13 @@ static struct ref_iterator 
*files_reflog_iterator_begin(struct ref_store *ref_st
files_path(refs, , "logs");
iter->dir_iterator = dir_iterator_begin(sb.buf);
strbuf_release();
+
+   if (strcmp(refs->gitdir.buf, refs->gitcommondir.buf)) {
+   strbuf_addf(, "%s/logs", refs->gitdir.buf);
+   iter->dir_iterator2 = dir_iterator_begin(sb.buf);
+   strbuf_release();
+   }
+
return ref_iterator;
 }
 
-- 
2.11.0.157.gd943d85

[PATCH 12/15] refs: add refs_for_each_reflog[_ent]()

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

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

diff --git a/refs.c b/refs.c
index ce165c0ea..622c6b669 100644
--- a/refs.c
+++ b/refs.c
@@ -1592,9 +1592,8 @@ int verify_refname_available(const char *refname,
return refs->be->verify_refname_available(refs, refname, extra, skip, 
err);
 }
 
-int for_each_reflog(each_ref_fn fn, void *cb_data)
+int refs_for_each_reflog(struct ref_store *refs, each_ref_fn fn, void *cb_data)
 {
-   struct ref_store *refs = get_main_ref_store();
struct ref_iterator *iter;
 
iter = refs->be->reflog_iterator_begin(refs);
@@ -1602,6 +1601,11 @@ int for_each_reflog(each_ref_fn fn, void *cb_data)
return do_for_each_ref_iterator(iter, fn, cb_data);
 }
 
+int for_each_reflog(each_ref_fn fn, void *cb_data)
+{
+   return refs_for_each_reflog(get_main_ref_store(), fn, cb_data);
+}
+
 int for_each_reflog_ent_reverse(const char *refname, each_reflog_ent_fn fn,
void *cb_data)
 {
@@ -1611,12 +1615,17 @@ int for_each_reflog_ent_reverse(const char *refname, 
each_reflog_ent_fn fn,
 fn, cb_data);
 }
 
+int refs_for_each_reflog_ent(struct ref_store *refs, const char *refname,
+each_reflog_ent_fn fn, void *cb_data)
+{
+   return refs->be->for_each_reflog_ent(refs, refname, fn, cb_data);
+}
+
 int for_each_reflog_ent(const char *refname, each_reflog_ent_fn fn,
void *cb_data)
 {
-   struct ref_store *refs = get_main_ref_store();
-
-   return refs->be->for_each_reflog_ent(refs, refname, fn, cb_data);
+   return refs_for_each_reflog_ent(get_main_ref_store(),
+   refname, fn, cb_data);
 }
 
 int reflog_exists(const char *refname)
diff --git a/refs.h b/refs.h
index 6665e5c57..5c1b99596 100644
--- a/refs.h
+++ b/refs.h
@@ -572,5 +572,8 @@ int refs_for_each_ref_in(struct ref_store *refs, const char 
*prefix,
 int refs_for_each_tag_ref(struct ref_store *refs, each_ref_fn fn, void 
*cb_data);
 int refs_for_each_branch_ref(struct ref_store *refs, each_ref_fn fn, void 
*cb_data);
 int refs_for_each_remote_ref(struct ref_store *refs, each_ref_fn fn, void 
*cb_data);
+int refs_for_each_reflog(struct ref_store *refs, each_ref_fn fn, void 
*cb_data);
+int refs_for_each_reflog_ent(struct ref_store *refs, const char *refname,
+each_reflog_ent_fn fn, void *cb_data);
 
 #endif /* REFS_H */
-- 
2.11.0.157.gd943d85

[PATCH 08/15] refs: add a refs_for_each_in() and friends

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

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 refs.c | 39 +++
 refs.h |  5 +
 2 files changed, 36 insertions(+), 8 deletions(-)

diff --git a/refs.c b/refs.c
index fc6cca3db..37b03d4ff 100644
--- a/refs.c
+++ b/refs.c
@@ -300,34 +300,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)
@@ -1200,10 +1218,15 @@ int for_each_ref_submodule(const char *submodule, 
each_ref_fn fn, void *cb_data)
   "", fn, 0, 0, cb_data);
 }
 
+int refs_for_each_ref_in(struct ref_store *refs, const char *prefix,
+each_ref_fn fn, void *cb_data)
+{
+   return do_for_each_ref(refs, prefix, fn, strlen(prefix), 0, cb_data);
+}
+
 int for_each_ref_in(const char *prefix, each_ref_fn fn, void *cb_data)
 {
-   return do_for_each_ref(get_main_ref_store(),
-  prefix, fn, strlen(prefix), 0, cb_data);
+   return refs_for_each_ref_in(get_main_ref_store(), prefix, fn, cb_data);
 }
 
 int for_each_fullref_in(const char *prefix, each_ref_fn fn, void *cb_data, 
unsigned int broken)
diff --git a/refs.h b/refs.h
index 8e3b4e839..8fc82deda 100644
--- a/refs.h
+++ b/refs.h
@@ -575,5 +575,10 @@ int refs_read_ref(struct ref_store *refs,
  const char *refname, unsigned char *sha1);
 int refs_head_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data);
 int refs_for_each_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data);
+int refs_for_each_ref_in(struct ref_store *refs, const char *prefix,
+each_ref_fn fn, void *cb_data);
+int refs_for_each_tag_ref(struct ref_store *refs, each_ref_fn fn, void 
*cb_data);
+int refs_for_each_branch_ref(struct ref_store *refs, each_ref_fn fn, void 
*cb_data);
+int refs_for_each_remote_ref(struct ref_store *refs, each_ref_fn fn, void 
*cb_data);
 
 #endif /* REFS_H */
-- 
2.11.0.157.gd943d85

[PATCH 11/15] revision.c: --all adds HEAD from all worktrees

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

Unless single_worktree is set, --all now adds HEAD from all worktrees.

Since reachable.c code does not use setup_revisions(), we need to call
other_head_refs_submodule() explicitly there to have the same effect on
"git prune", so that we won't accidentally delete objects needed by some
other HEADs.

A new FIXME is added because we would need something like

int refs_other_head_refs(struct ref_store *, each_ref_fn, cb_data);

in addition to other_head_refs() to handle it, which might require

int get_submodule_worktrees(const char *submodule, int flags);

It could be a separate topic to reduce the scope of this one.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 reachable.c  |  1 +
 refs.c   | 22 ++
 refs.h   |  1 +
 revision.c   | 13 +
 submodule.c  |  2 ++
 t/t5304-prune.sh | 12 
 6 files changed, 51 insertions(+)

diff --git a/reachable.c b/reachable.c
index d0199cace..61a6ec05c 100644
--- a/reachable.c
+++ b/reachable.c
@@ -177,6 +177,7 @@ void mark_reachable_objects(struct rev_info *revs, int 
mark_reflog,
 
/* detached HEAD is not included in the list above */
head_ref(add_one_ref, revs);
+   other_head_refs(add_one_ref, revs);
 
/* Add all reflog info */
if (mark_reflog)
diff --git a/refs.c b/refs.c
index fa2df7a1d..ce165c0ea 100644
--- a/refs.c
+++ b/refs.c
@@ -1676,3 +1676,25 @@ int rename_ref(const char *oldref, const char *newref, 
const char *logmsg)
 
return refs->be->rename_ref(refs, oldref, newref, logmsg);
 }
+
+int other_head_refs(each_ref_fn fn, void *cb_data)
+{
+   struct worktree **worktrees, **p;
+   int ret = 0;
+
+   worktrees = get_worktrees(0);
+   for (p = worktrees; *p; p++) {
+   struct worktree *wt = *p;
+   struct ref_store *refs;
+
+   if (wt->is_current)
+   continue;
+
+   refs = get_worktree_ref_store(wt);
+   ret = refs_head_ref(refs, fn, cb_data);
+   if (ret)
+   break;
+   }
+   free_worktrees(worktrees);
+   return ret;
+}
diff --git a/refs.h b/refs.h
index 986d408bd..6665e5c57 100644
--- a/refs.h
+++ b/refs.h
@@ -190,6 +190,7 @@ typedef int each_ref_fn(const char *refname,
  * stop the iteration.
  */
 int head_ref(each_ref_fn fn, void *cb_data);
+int other_head_refs(each_ref_fn fn, void *cb_data);
 int for_each_ref(each_ref_fn fn, void *cb_data);
 int for_each_ref_in(const char *prefix, each_ref_fn fn, void *cb_data);
 int for_each_fullref_in(const char *prefix, each_ref_fn fn, void *cb_data,
diff --git a/revision.c b/revision.c
index d82c37b44..8ee929cef 100644
--- a/revision.c
+++ b/revision.c
@@ -2105,6 +2105,13 @@ static int handle_revision_pseudo_opt(const char 
*submodule,
int argcount;
 
if (submodule) {
+   /*
+* We need some something like get_submodule_worktrees()
+* before we can go through all worktrees of a submodule,
+* .e.g with adding all HEADs from --all, which is not
+* supported right now, so stick to single worktree.
+*/
+   assert(revs->single_worktree != 0);
refs = get_submodule_ref_store(submodule);
} else
refs = get_main_ref_store();
@@ -2122,6 +2129,12 @@ static int handle_revision_pseudo_opt(const char 
*submodule,
if (!strcmp(arg, "--all")) {
handle_refs(refs, revs, *flags, refs_for_each_ref);
handle_refs(refs, revs, *flags, refs_head_ref);
+   if (!revs->single_worktree) {
+   struct all_refs_cb cb;
+
+   init_all_refs_cb(, revs, *flags);
+   other_head_refs(handle_one_ref, );
+   }
clear_ref_exclusion(>ref_excludes);
} else if (!strcmp(arg, "--branches")) {
handle_refs(refs, revs, *flags, refs_for_each_branch_ref);
diff --git a/submodule.c b/submodule.c
index 3ce589d55..aed47baaf 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1155,6 +1155,8 @@ static int find_first_merges(struct object_array *result, 
const char *path,
oid_to_hex(>object.oid));
init_revisions(, NULL);
rev_opts.submodule = path;
+   /* FIXME: can't handle linked worktrees in submodules yet */
+   revs.single_worktree = path != NULL;
setup_revisions(ARRAY_SIZE(rev_args)-1, rev_args, , _opts);
 
/* save all revisions from the above list that contain b */
diff --git a/t/t5304-prune.sh b/t/t5304-prune.sh
index cba45c7be..683bdb031 100755
--- a/t/t5304-prune.sh
+++ b/t/t5304-prune.sh
@@ -292,4 +292,16 @@ test_expect_success 'prune: handle index in multiple 
worktrees' '
test_cmp second-worktree/blob actual
 '
 
+test_expect_success 'prune: handle HEAD in multiple worktrees' '
+   git worktree add --detach 

[PATCH 09/15] revision.c: use refs_for_each*() instead of for_each_*_submodule()

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

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

diff --git a/revision.c b/revision.c
index d82f72ff3..d82c37b44 100644
--- a/revision.c
+++ b/revision.c
@@ -1189,12 +1189,19 @@ void add_ref_exclusion(struct string_list 
**ref_excludes_p, const char *exclude)
string_list_append(*ref_excludes_p, exclude);
 }
 
-static void handle_refs(const char *submodule, struct rev_info *revs, unsigned 
flags,
-   int (*for_each)(const char *, each_ref_fn, void *))
+static void handle_refs(struct ref_store *refs,
+   struct rev_info *revs, unsigned flags,
+   int (*for_each)(struct ref_store *, each_ref_fn, void 
*))
 {
struct all_refs_cb cb;
+
+   if (!refs) {
+   /* this could happen with uninitialized submodules */
+   return;
+   }
+
init_all_refs_cb(, revs, flags);
-   for_each(submodule, handle_one_ref, );
+   for_each(refs, handle_one_ref, );
 }
 
 static void handle_one_reflog_commit(unsigned char *sha1, void *cb_data)
@@ -2067,23 +2074,25 @@ void parse_revision_opt(struct rev_info *revs, struct 
parse_opt_ctx_t *ctx,
ctx->argc -= n;
 }
 
-static int for_each_bisect_ref(const char *submodule, each_ref_fn fn, void 
*cb_data, const char *term) {
+static int for_each_bisect_ref(struct ref_store *refs, each_ref_fn fn,
+  void *cb_data, const char *term)
+{
struct strbuf bisect_refs = STRBUF_INIT;
int status;
strbuf_addf(_refs, "refs/bisect/%s", term);
-   status = for_each_ref_in_submodule(submodule, bisect_refs.buf, fn, 
cb_data);
+   status = refs_for_each_ref_in(refs, bisect_refs.buf, fn, cb_data);
strbuf_release(_refs);
return status;
 }
 
-static int for_each_bad_bisect_ref(const char *submodule, each_ref_fn fn, void 
*cb_data)
+static int for_each_bad_bisect_ref(struct ref_store *refs, each_ref_fn fn, 
void *cb_data)
 {
-   return for_each_bisect_ref(submodule, fn, cb_data, term_bad);
+   return for_each_bisect_ref(refs, fn, cb_data, term_bad);
 }
 
-static int for_each_good_bisect_ref(const char *submodule, each_ref_fn fn, 
void *cb_data)
+static int for_each_good_bisect_ref(struct ref_store *refs, each_ref_fn fn, 
void *cb_data)
 {
-   return for_each_bisect_ref(submodule, fn, cb_data, term_good);
+   return for_each_bisect_ref(refs, fn, cb_data, term_good);
 }
 
 static int handle_revision_pseudo_opt(const char *submodule,
@@ -2092,8 +2101,14 @@ static int handle_revision_pseudo_opt(const char 
*submodule,
 {
const char *arg = argv[0];
const char *optarg;
+   struct ref_store *refs;
int argcount;
 
+   if (submodule) {
+   refs = get_submodule_ref_store(submodule);
+   } else
+   refs = get_main_ref_store();
+
/*
 * NOTE!
 *
@@ -2105,22 +2120,23 @@ static int handle_revision_pseudo_opt(const char 
*submodule,
 * register it in the list at the top of handle_revision_opt.
 */
if (!strcmp(arg, "--all")) {
-   handle_refs(submodule, revs, *flags, for_each_ref_submodule);
-   handle_refs(submodule, revs, *flags, head_ref_submodule);
+   handle_refs(refs, revs, *flags, refs_for_each_ref);
+   handle_refs(refs, revs, *flags, refs_head_ref);
clear_ref_exclusion(>ref_excludes);
} else if (!strcmp(arg, "--branches")) {
-   handle_refs(submodule, revs, *flags, 
for_each_branch_ref_submodule);
+   handle_refs(refs, revs, *flags, refs_for_each_branch_ref);
clear_ref_exclusion(>ref_excludes);
} else if (!strcmp(arg, "--bisect")) {
read_bisect_terms(_bad, _good);
-   handle_refs(submodule, revs, *flags, for_each_bad_bisect_ref);
-   handle_refs(submodule, revs, *flags ^ (UNINTERESTING | BOTTOM), 
for_each_good_bisect_ref);
+   handle_refs(refs, revs, *flags, for_each_bad_bisect_ref);
+   handle_refs(refs, revs, *flags ^ (UNINTERESTING | BOTTOM),
+   for_each_good_bisect_ref);
revs->bisect = 1;
} else if (!strcmp(arg, "--tags")) {
-   handle_refs(submodule, revs, *flags, 
for_each_tag_ref_submodule);
+   handle_refs(refs, revs, *flags, refs_for_each_tag_ref);
clear_ref_exclusion(>ref_excludes);
} else if (!strcmp(arg, "--remotes")) {
-   handle_refs(submodule, revs, *flags, 
for_each_remote_ref_submodule);
+   handle_refs(refs, revs, *flags, refs_for_each_remote_ref);
clear_ref_exclusion(>ref_excludes);
} else if ((argcount = parse_long_opt("glob", argv, ))) {
struct all_refs_cb cb;
-- 
2.11.0.157.gd943d85

[PATCH 05/15] refs: add refs_read_ref[_full]()

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

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

diff --git a/refs.c b/refs.c
index 9c86c44b8..06890db5d 100644
--- a/refs.c
+++ b/refs.c
@@ -186,16 +186,29 @@ 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 refs_read_ref(struct ref_store *refs, const char *refname, unsigned char 
*sha1)
+{
+   return refs_read_ref_full(refs, refname, RESOLVE_REF_READING, sha1, 
NULL);
+}
+
 int read_ref(const char *refname, unsigned char *sha1)
 {
-   return read_ref_full(refname, RESOLVE_REF_READING, sha1, NULL);
+   return refs_read_ref(get_main_ref_store(), refname, sha1);
 }
 
 int ref_exists(const char *refname)
diff --git a/refs.h b/refs.h
index bce77891a..229a97f59 100644
--- a/refs.h
+++ b/refs.h
@@ -568,5 +568,10 @@ int refs_create_symref(struct ref_store *refs,
   const char *refname,
   const char *target,
   const char *logmsg);
+int refs_read_ref_full(struct ref_store *refs,
+  const char *refname, int resolve_flags,
+  unsigned char *sha1, int *flags);
+int refs_read_ref(struct ref_store *refs,
+ const char *refname, unsigned char *sha1);
 
 #endif /* REFS_H */
-- 
2.11.0.157.gd943d85

[PATCH 07/15] refs: add refs_for_each_ref()

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

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 refs.c | 33 ++---
 refs.h |  1 +
 2 files changed, 23 insertions(+), 11 deletions(-)

diff --git a/refs.c b/refs.c
index 26758b8cf..fc6cca3db 100644
--- a/refs.c
+++ b/refs.c
@@ -1170,10 +1170,9 @@ int head_ref(each_ref_fn fn, void *cb_data)
  * non-zero value, stop the iteration and return that value;
  * otherwise, return 0.
  */
-static int do_for_each_ref(const char *submodule, const char *prefix,
+static int do_for_each_ref(struct ref_store *refs, const char *prefix,
   each_ref_fn fn, int trim, int flags, void *cb_data)
 {
-   struct ref_store *refs = get_submodule_ref_store(submodule);
struct ref_iterator *iter;
 
if (!refs)
@@ -1185,19 +1184,26 @@ static int do_for_each_ref(const char *submodule, const 
char *prefix,
return do_for_each_ref_iterator(iter, fn, cb_data);
 }
 
+int refs_for_each_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data)
+{
+   return do_for_each_ref(refs, "", fn, 0, 0, cb_data);
+}
+
 int for_each_ref(each_ref_fn fn, void *cb_data)
 {
-   return do_for_each_ref(NULL, "", fn, 0, 0, cb_data);
+   return do_for_each_ref(get_main_ref_store(), "", fn, 0, 0, cb_data);
 }
 
 int for_each_ref_submodule(const char *submodule, each_ref_fn fn, void 
*cb_data)
 {
-   return do_for_each_ref(submodule, "", fn, 0, 0, cb_data);
+   return do_for_each_ref(get_submodule_ref_store(submodule),
+  "", fn, 0, 0, cb_data);
 }
 
 int for_each_ref_in(const char *prefix, each_ref_fn fn, void *cb_data)
 {
-   return do_for_each_ref(NULL, prefix, fn, strlen(prefix), 0, cb_data);
+   return do_for_each_ref(get_main_ref_store(),
+  prefix, fn, strlen(prefix), 0, cb_data);
 }
 
 int for_each_fullref_in(const char *prefix, each_ref_fn fn, void *cb_data, 
unsigned int broken)
@@ -1206,19 +1212,23 @@ int for_each_fullref_in(const char *prefix, each_ref_fn 
fn, void *cb_data, unsig
 
if (broken)
flag = DO_FOR_EACH_INCLUDE_BROKEN;
-   return do_for_each_ref(NULL, prefix, fn, 0, flag, cb_data);
+   return do_for_each_ref(get_main_ref_store(),
+  prefix, fn, 0, flag, cb_data);
 }
 
 int for_each_ref_in_submodule(const char *submodule, const char *prefix,
each_ref_fn fn, void *cb_data)
 {
-   return do_for_each_ref(submodule, prefix, fn, strlen(prefix), 0, 
cb_data);
+   return do_for_each_ref(get_submodule_ref_store(submodule),
+  prefix, fn, strlen(prefix), 0, cb_data);
 }
 
 int for_each_replace_ref(each_ref_fn fn, void *cb_data)
 {
-   return do_for_each_ref(NULL, git_replace_ref_base, fn,
-  strlen(git_replace_ref_base), 0, cb_data);
+   return do_for_each_ref(get_main_ref_store(),
+  git_replace_ref_base, fn,
+  strlen(git_replace_ref_base),
+  0, cb_data);
 }
 
 int for_each_namespaced_ref(each_ref_fn fn, void *cb_data)
@@ -1226,14 +1236,15 @@ int for_each_namespaced_ref(each_ref_fn fn, void 
*cb_data)
struct strbuf buf = STRBUF_INIT;
int ret;
strbuf_addf(, "%srefs/", get_git_namespace());
-   ret = do_for_each_ref(NULL, buf.buf, fn, 0, 0, cb_data);
+   ret = do_for_each_ref(get_main_ref_store(),
+ buf.buf, fn, 0, 0, cb_data);
strbuf_release();
return ret;
 }
 
 int for_each_rawref(each_ref_fn fn, void *cb_data)
 {
-   return do_for_each_ref(NULL, "", fn, 0,
+   return do_for_each_ref(get_main_ref_store(), "", fn, 0,
   DO_FOR_EACH_INCLUDE_BROKEN, cb_data);
 }
 
diff --git a/refs.h b/refs.h
index 54c038e3c..8e3b4e839 100644
--- a/refs.h
+++ b/refs.h
@@ -574,5 +574,6 @@ int refs_read_ref_full(struct ref_store *refs,
 int refs_read_ref(struct ref_store *refs,
  const char *refname, unsigned char *sha1);
 int refs_head_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data);
+int refs_for_each_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data);
 
 #endif /* REFS_H */
-- 
2.11.0.157.gd943d85

[PATCH 10/15] refs: remove dead for_each_*_submodule()

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

These are used in revision.c. After the last patch they are replaced
with the refs_ version. Delete them (except for_each_remote_ref_submodule
which is still used by submodule.c)

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 refs.c | 30 --
 refs.h |  9 -
 2 files changed, 39 deletions(-)

diff --git a/refs.c b/refs.c
index 37b03d4ff..fa2df7a1d 100644
--- a/refs.c
+++ b/refs.c
@@ -310,12 +310,6 @@ int for_each_tag_ref(each_ref_fn fn, void *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 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);
@@ -326,12 +320,6 @@ int for_each_branch_ref(each_ref_fn fn, void *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 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);
@@ -1169,11 +1157,6 @@ int refs_head_ref(struct ref_store *refs, each_ref_fn 
fn, void *cb_data)
return 0;
 }
 
-int head_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data)
-{
-   return refs_head_ref(get_submodule_ref_store(submodule), fn, cb_data);
-}
-
 int head_ref(each_ref_fn fn, void *cb_data)
 {
return refs_head_ref(get_main_ref_store(), fn, cb_data);
@@ -1212,12 +1195,6 @@ int for_each_ref(each_ref_fn fn, void *cb_data)
return do_for_each_ref(get_main_ref_store(), "", fn, 0, 0, cb_data);
 }
 
-int for_each_ref_submodule(const char *submodule, each_ref_fn fn, void 
*cb_data)
-{
-   return do_for_each_ref(get_submodule_ref_store(submodule),
-  "", fn, 0, 0, cb_data);
-}
-
 int refs_for_each_ref_in(struct ref_store *refs, const char *prefix,
 each_ref_fn fn, void *cb_data)
 {
@@ -1239,13 +1216,6 @@ int for_each_fullref_in(const char *prefix, each_ref_fn 
fn, void *cb_data, unsig
   prefix, fn, 0, flag, cb_data);
 }
 
-int for_each_ref_in_submodule(const char *submodule, const char *prefix,
-   each_ref_fn fn, void *cb_data)
-{
-   return do_for_each_ref(get_submodule_ref_store(submodule),
-  prefix, fn, strlen(prefix), 0, cb_data);
-}
-
 int for_each_replace_ref(each_ref_fn fn, void *cb_data)
 {
return do_for_each_ref(get_main_ref_store(),
diff --git a/refs.h b/refs.h
index 8fc82deda..986d408bd 100644
--- a/refs.h
+++ b/refs.h
@@ -202,15 +202,6 @@ int for_each_glob_ref(each_ref_fn fn, const char *pattern, 
void *cb_data);
 int for_each_glob_ref_in(each_ref_fn fn, const char *pattern,
 const char *prefix, void *cb_data);
 
-int head_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data);
-int for_each_ref_submodule(const char *submodule,
-  each_ref_fn fn, void *cb_data);
-int for_each_ref_in_submodule(const char *submodule, const char *prefix,
-   each_ref_fn fn, void *cb_data);
-int for_each_tag_ref_submodule(const char *submodule,
-  each_ref_fn fn, void *cb_data);
-int for_each_branch_ref_submodule(const char *submodule,
- each_ref_fn fn, void *cb_data);
 int for_each_remote_ref_submodule(const char *submodule,
  each_ref_fn fn, void *cb_data);
 
-- 
2.11.0.157.gd943d85

[PATCH 03/15] revision.c: --indexed-objects add objects from all worktrees

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

This is the result of single_worktree flag never being set (no way to up
until now). To get objects from current index only, set single_worktree.

The other add_index_objects_to_pending's caller is mark_reachable_objects()
(e.g. "git prune") which also mark objects from all indexes.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 revision.c   | 21 +
 t/t5304-prune.sh |  9 +
 2 files changed, 30 insertions(+)

diff --git a/revision.c b/revision.c
index ece868a25..d82f72ff3 100644
--- a/revision.c
+++ b/revision.c
@@ -19,6 +19,7 @@
 #include "dir.h"
 #include "cache-tree.h"
 #include "bisect.h"
+#include "worktree.h"
 
 volatile show_early_output_fn_t show_early_output;
 
@@ -1291,8 +1292,28 @@ static void do_add_index_objects_to_pending(struct 
rev_info *revs,
 
 void add_index_objects_to_pending(struct rev_info *revs, unsigned int flags)
 {
+   struct worktree **worktrees, **p;
+
read_cache();
do_add_index_objects_to_pending(revs, _index);
+
+   if (revs->single_worktree)
+   return;
+
+   worktrees = get_worktrees(0);
+   for (p = worktrees; *p; p++) {
+   struct worktree *wt = *p;
+   struct index_state istate = {0};
+
+   if (wt->is_current)
+   continue; /* current index already taken care of */
+
+   if (read_index_from(,
+   worktree_git_path(wt, "index")) > 0)
+   do_add_index_objects_to_pending(revs, );
+   discard_index();
+   }
+   free_worktrees(worktrees);
 }
 
 static int add_parents_only(struct rev_info *revs, const char *arg_, int flags,
diff --git a/t/t5304-prune.sh b/t/t5304-prune.sh
index 133b5842b..cba45c7be 100755
--- a/t/t5304-prune.sh
+++ b/t/t5304-prune.sh
@@ -283,4 +283,13 @@ test_expect_success 'prune: handle alternate object 
database' '
git -C B prune
 '
 
+test_expect_success 'prune: handle index in multiple worktrees' '
+   git worktree add second-worktree &&
+   echo "new blob for second-worktree" >second-worktree/blob &&
+   git -C second-worktree add blob &&
+   git prune --expire=now &&
+   git -C second-worktree show :blob >actual &&
+   test_cmp second-worktree/blob actual
+'
+
 test_done
-- 
2.11.0.157.gd943d85

[PATCH 06/15] refs: add refs_head_ref()

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

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

diff --git a/refs.c b/refs.c
index 06890db5d..26758b8cf 100644
--- a/refs.c
+++ b/refs.c
@@ -1139,27 +1139,26 @@ int rename_ref_available(const char *old_refname, const 
char *new_refname)
return ok;
 }
 
-int head_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data)
+int refs_head_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data)
 {
struct object_id oid;
int flag;
 
-   if (submodule) {
-   if (resolve_gitlink_ref(submodule, "HEAD", oid.hash) == 0)
-   return fn("HEAD", , 0, cb_data);
-
-   return 0;
-   }
-
-   if (!read_ref_full("HEAD", RESOLVE_REF_READING, oid.hash, ))
+   if (!refs_read_ref_full(refs, "HEAD", RESOLVE_REF_READING,
+   oid.hash, ))
return fn("HEAD", , flag, cb_data);
 
return 0;
 }
 
+int head_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data)
+{
+   return refs_head_ref(get_submodule_ref_store(submodule), fn, cb_data);
+}
+
 int head_ref(each_ref_fn fn, void *cb_data)
 {
-   return head_ref_submodule(NULL, fn, cb_data);
+   return refs_head_ref(get_main_ref_store(), fn, cb_data);
 }
 
 /*
diff --git a/refs.h b/refs.h
index 229a97f59..54c038e3c 100644
--- a/refs.h
+++ b/refs.h
@@ -573,5 +573,6 @@ int refs_read_ref_full(struct ref_store *refs,
   unsigned char *sha1, int *flags);
 int refs_read_ref(struct ref_store *refs,
  const char *refname, unsigned char *sha1);
+int refs_head_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data);
 
 #endif /* REFS_H */
-- 
2.11.0.157.gd943d85

[PATCH 02/15] revision.c: refactor add_index_objects_to_pending()

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

The core code is factored out and take 'struct index_state *' instead so
that we can reuse it to add objects from index files other than .git/index
in the next patch.

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

diff --git a/revision.c b/revision.c
index b37dbec37..ece868a25 100644
--- a/revision.c
+++ b/revision.c
@@ -1263,13 +1263,13 @@ static void add_cache_tree(struct cache_tree *it, 
struct rev_info *revs,
 
 }
 
-void add_index_objects_to_pending(struct rev_info *revs, unsigned flags)
+static void do_add_index_objects_to_pending(struct rev_info *revs,
+   struct index_state *istate)
 {
int i;
 
-   read_cache();
-   for (i = 0; i < active_nr; i++) {
-   struct cache_entry *ce = active_cache[i];
+   for (i = 0; i < istate->cache_nr; i++) {
+   struct cache_entry *ce = istate->cache[i];
struct blob *blob;
 
if (S_ISGITLINK(ce->ce_mode))
@@ -1282,13 +1282,19 @@ void add_index_objects_to_pending(struct rev_info 
*revs, unsigned flags)
 ce->ce_mode, ce->name);
}
 
-   if (active_cache_tree) {
+   if (istate->cache_tree) {
struct strbuf path = STRBUF_INIT;
-   add_cache_tree(active_cache_tree, revs, );
+   add_cache_tree(istate->cache_tree, revs, );
strbuf_release();
}
 }
 
+void add_index_objects_to_pending(struct rev_info *revs, unsigned int flags)
+{
+   read_cache();
+   do_add_index_objects_to_pending(revs, _index);
+}
+
 static int add_parents_only(struct rev_info *revs, const char *arg_, int flags,
int exclude_parent)
 {
-- 
2.11.0.157.gd943d85

[PATCH 04/15] refs: move submodule slash stripping code to get_submodule_ref_store

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

This is a better place that will benefit all submodule callers instead
of just resolve_gitlink_ref()

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

diff --git a/refs.c b/refs.c
index 23e0a8eda..9c86c44b8 100644
--- a/refs.c
+++ b/refs.c
@@ -1321,25 +1321,10 @@ const char *resolve_ref_unsafe(const char *refname, int 
resolve_flags,
 int resolve_gitlink_ref(const char *submodule, const char *refname,
unsigned char *sha1)
 {
-   size_t len = strlen(submodule);
struct ref_store *refs;
int flags;
 
-   while (len && submodule[len - 1] == '/')
-   len--;
-
-   if (!len)
-   return -1;
-
-   if (submodule[len]) {
-   /* We need to strip off one or more trailing slashes */
-   char *stripped = xmemdupz(submodule, len);
-
-   refs = get_submodule_ref_store(stripped);
-   free(stripped);
-   } else {
-   refs = get_submodule_ref_store(submodule);
-   }
+   refs = get_submodule_ref_store(submodule);
 
if (!refs)
return -1;
@@ -1458,7 +1443,17 @@ struct ref_store *get_submodule_ref_store(const char 
*submodule)
 {
struct strbuf submodule_sb = STRBUF_INIT;
struct ref_store *refs;
+   char *to_free = NULL;
int ret;
+   size_t len;
+
+   if (submodule) {
+   len = strlen(submodule);
+   while (len && submodule[len - 1] == '/')
+   len--;
+   if (!len)
+   submodule = NULL;
+   }
 
if (!submodule || !*submodule) {
/*
@@ -1468,15 +1463,19 @@ struct ref_store *get_submodule_ref_store(const char 
*submodule)
return get_main_ref_store();
}
 
+   if (submodule[len])
+   /* We need to strip off one or more trailing slashes */
+   submodule = to_free = xmemdupz(submodule, len);
+
refs = lookup_submodule_ref_store(submodule);
if (refs)
-   return refs;
+   goto done;
 
strbuf_addstr(_sb, submodule);
ret = is_nonbare_repository_dir(_sb);
strbuf_release(_sb);
if (!ret)
-   return refs;
+   goto done;
 
ret = submodule_to_gitdir(_sb, submodule);
if (!ret)
@@ -1485,6 +1484,9 @@ struct ref_store *get_submodule_ref_store(const char 
*submodule)
 
if (refs)
register_submodule_ref_store(refs, submodule);
+
+done:
+   free(to_free);
return refs;
 }
 
-- 
2.11.0.157.gd943d85

[PATCH/RFC 00/15] Fix git-gc losing objects in multi worktree

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

So here is my latest attempt on fixing this issue. For people who are
not aware of it, git-gc does not take per-worktree refs, reflogs and
indexes into account. An odb prune may leave HEAD and references in other
worktrees pointing to nowhere.

This series is based on my "kill parse_ref()" series [1], which is
based on yet another one, which is on top of mh/submodule-hash. But
you can get everything from my github [2].

The series introduces a new set of refs_* API and replaces the old
*_submodule() one, opening up the opportunity to access refs from
another worktree. rev-list learns a new option, --single-worktree, to
control the new behavior.

reflog iterator from files-backend.c does not support per-worktree
items, so it's updated here to do that. It still looks ugly, but I
think this is a good "middle ground" until compound ref store comes.
At that point we can separate "single worktree" ref store vs "linked
worktree" one.

I'm adding Stefan here as well since I added a new FIXME in
submodule.c in 11/15. I think it's ok (again, for now). But another
look from submodule people would be much better.

[1] http://public-inbox.org/git/%3c20170216120302.5302-1-pclo...@gmail.com%3E/
[2] https://github.com/pclouds/git/commits/prune-in-worktrees-2

Nguyễn Thái Ngọc Duy (15):
  revision.h: new flag in struct rev_info wrt. worktree-related refs
  revision.c: refactor add_index_objects_to_pending()
  revision.c: --indexed-objects add objects from all worktrees
  refs: move submodule slash stripping code to get_submodule_ref_store
  refs: add refs_read_ref[_full]()
  refs: add refs_head_ref()
  refs: add refs_for_each_ref()
  refs: add a refs_for_each_in() and friends
  revision.c: use refs_for_each*() instead of for_each_*_submodule()
  refs: remove dead for_each_*_submodule()
  revision.c: --all adds HEAD from all worktrees
  refs: add refs_for_each_reflog[_ent]()
  files-backend: make reflog iterator go through per-worktree reflog
  revision.c: --reflog add HEAD reflog from all worktrees
  rev-list: expose and document --single-worktree

 Documentation/rev-list-options.txt |   8 ++
 reachable.c|   1 +
 refs.c | 171 -
 refs.h |  25 --
 refs/files-backend.c   |  24 +-
 revision.c | 130 +++-
 revision.h |   1 +
 submodule.c|   2 +
 t/t5304-prune.sh   |  37 
 9 files changed, 305 insertions(+), 94 deletions(-)

-- 
2.11.0.157.gd943d85

[PATCH 01/15] revision.h: new flag in struct rev_info wrt. worktree-related refs

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

The revision walker can walk through per-worktree refs like HEAD or
SHA-1 references in the index. These currently are from the current
worktree only. This new flag is added to change rev-list behavior in
this regard:

When single_worktree is set, only current worktree is considered. When
it is not set (which is the default), all worktrees are considered.

The default is chosen so because the two big components that rev-list
works with are object database (entirely shared between worktrees) and
refs (mostly shared). It makes sense that default behavior goes per-repo
too instead of per-worktree.

The flag will eventually be exposed as a rev-list argument with
documents. For now it stays internal until the new behavior is fully
implemented.

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

diff --git a/revision.h b/revision.h
index 9fac1a607..c851b94ad 100644
--- a/revision.h
+++ b/revision.h
@@ -88,6 +88,7 @@ struct rev_info {
topo_order:1,
simplify_merges:1,
simplify_by_decoration:1,
+   single_worktree:1,
tag_objects:1,
tree_objects:1,
blob_objects:1,
-- 
2.11.0.157.gd943d85

Hopefully

[Rita Micheal]

Dear friend,

My name is Mr Micheal Rita, I am the Bill and Exchange (assistant)
Manager of Bank of Africa Ouagadougou, Burkina Faso. In my department
I discovered an abandoned sum of teen million five hundred thousand United
State of American dollars (10.5MILLION USA DOLLARS) in an account that
belongs to one of our foreign customer who died in airline that crashed on 4th
October 2001.

Since I got information about his death I have been expecting his next
of kin to come over and claim his money because we can not release
it unless somebody applies for it as the next of kin or relation to the
deceased as indicated in our banking guidelines, but unfortunately
we learnt that all his supposed next of kin or relation died alongside
with him in the plane crash leaving nobody behind for the claim. It is
therefore upon this discovery that I decided to make this business
proposal to you and release the money to you as next of kin or relation
to the deceased for safety and subsequent disbursement since nobody
is coming for it and I don't want the money to go into the bank treasury
as unclaimed bill.

You will be entitled with 40% of the total sum while 60% will be for
me after which I will visit your Country to invest my own share when
the fund is successfully transferred into your account, Please I would
like you to keep this transaction confidential and as a top secret as
you may wish to know that I am a bank official.

Yours sincerely,
Mr Micheal Rita.

[PATCH v3 15/16] files-backend: remove submodule_allowed from files_downcast()

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

Since submodule or not is irrelevant to files-backend after the last
patch, remove the parameter from files_downcast() and kill
files_assert_main_repository().

PS. This implies that all ref operations are allowed for submodules. But
we may need to look more closely to see if that's really true...

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

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 627466043..d35032fcd 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1019,24 +1019,13 @@ 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.
- */
-static void files_assert_main_repository(struct files_ref_store *refs,
-const char *caller)
-{
-   /* This function is to be deleted in the next patch */
-}
-
-/*
  * Downcast ref_store to files_ref_store. Die if ref_store is not a
  * files_ref_store. If submodule_allowed is not true, then also die if
  * 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 submodule_allowed,
-   const char *caller)
+static struct files_ref_store *files_downcast(struct ref_store *ref_store,
+ const char *caller)
 {
struct files_ref_store *refs;
 
@@ -1046,9 +1035,6 @@ static struct files_ref_store *files_downcast(
 
refs = (struct files_ref_store *)ref_store;
 
-   if (!submodule_allowed)
-   files_assert_main_repository(refs, caller);
-
return refs;
 }
 
@@ -1384,7 +1370,7 @@ static int files_read_raw_ref(struct ref_store *ref_store,
  struct strbuf *referent, unsigned int *type)
 {
struct files_ref_store *refs =
-   files_downcast(ref_store, 1, "read_raw_ref");
+   files_downcast(ref_store, "read_raw_ref");
struct strbuf sb_contents = STRBUF_INIT;
struct strbuf sb_path = STRBUF_INIT;
const char *path;
@@ -1577,7 +1563,6 @@ static int lock_raw_ref(struct files_ref_store *refs,
int ret = TRANSACTION_GENERIC_ERROR;
 
assert(err);
-   files_assert_main_repository(refs, "lock_raw_ref");
 
*type = 0;
 
@@ -1801,7 +1786,7 @@ static enum peel_status peel_entry(struct ref_entry 
*entry, int repeel)
 static int files_peel_ref(struct ref_store *ref_store,
  const char *refname, unsigned char *sha1)
 {
-   struct files_ref_store *refs = files_downcast(ref_store, 0, "peel_ref");
+   struct files_ref_store *refs = files_downcast(ref_store, "peel_ref");
int flag;
unsigned char base[20];
 
@@ -1910,7 +1895,7 @@ static struct ref_iterator *files_ref_iterator_begin(
const char *prefix, unsigned int flags)
 {
struct files_ref_store *refs =
-   files_downcast(ref_store, 1, "ref_iterator_begin");
+   files_downcast(ref_store, "ref_iterator_begin");
struct ref_dir *loose_dir, *packed_dir;
struct ref_iterator *loose_iter, *packed_iter;
struct files_ref_iterator *iter;
@@ -2043,7 +2028,6 @@ static struct ref_lock *lock_ref_sha1_basic(struct 
files_ref_store *refs,
int attempts_remaining = 3;
int resolved;
 
-   files_assert_main_repository(refs, "lock_ref_sha1_basic");
assert(err);
 
lock = xcalloc(1, sizeof(struct ref_lock));
@@ -2191,8 +2175,6 @@ static int lock_packed_refs(struct files_ref_store *refs, 
int flags)
struct strbuf sb = STRBUF_INIT;
int ret;
 
-   files_assert_main_repository(refs, "lock_packed_refs");
-
if (!timeout_configured) {
git_config_get_int("core.packedrefstimeout", _value);
timeout_configured = 1;
@@ -2232,8 +2214,6 @@ static int commit_packed_refs(struct files_ref_store 
*refs)
int save_errno = 0;
FILE *out;
 
-   files_assert_main_repository(refs, "commit_packed_refs");
-
if (!packed_ref_cache->lock)
die("internal error: packed-refs not locked");
 
@@ -2265,8 +2245,6 @@ static void rollback_packed_refs(struct files_ref_store 
*refs)
struct packed_ref_cache *packed_ref_cache =
get_packed_ref_cache(refs);
 
-   files_assert_main_repository(refs, "rollback_packed_refs");
-
if (!packed_ref_cache->lock)
die("internal error: packed-refs not locked");
rollback_lock_file(packed_ref_cache->lock);
@@ -2412,7 +2390,7 @@ static void prune_refs(struct files_ref_store *refs, 
struct ref_to_prune *r)
 static int files_pack_refs(struct ref_store 

[PATCH v3 11/16] 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 | 36 +++-
 1 file changed, 19 insertions(+), 17 deletions(-)

diff --git a/refs.c b/refs.c
index 76a0e7b5a..55a80a83d 100644
--- a/refs.c
+++ b/refs.c
@@ -1402,25 +1402,19 @@ 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.
+ * for submodule. 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)
+static void register_submodule_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 (!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);
-   }
+   if (hashmap_put(_ref_stores,
+   alloc_submodule_hash_entry(submodule, refs)))
+   die("BUG: ref_store for submodule '%s' initialized twice",
+   submodule);
 }
 
 /*
@@ -1437,7 +1431,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;
 }
 
@@ -1449,6 +1442,12 @@ static struct ref_store *get_main_ref_store(void)
return main_ref_store;
 
refs = ref_store_init(NULL);
+   if (refs) {
+   if (main_ref_store)
+   die("BUG: main_ref_store initialized twice");
+
+   main_ref_store = refs;
+   }
return refs;
 }
 
@@ -1469,6 +1468,9 @@ struct ref_store *get_ref_store(const char *submodule)
if (is_nonbare_repository_dir(_sb))
refs = ref_store_init(submodule);
strbuf_release(_sb);
+
+   if (refs)
+   register_submodule_ref_store(refs, submodule);
return refs;
 }
 
-- 
2.11.0.157.gd943d85

[PATCH v3 07/16] 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 | 22 --
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 24f5bf7f1..07cf2cb93 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -924,6 +924,9 @@ struct files_ref_store {
 */
const char *submodule;
 
+   struct strbuf gitdir;
+   struct strbuf gitcommondir;
+
struct ref_entry *loose;
struct packed_ref_cache *packed;
 };
@@ -937,6 +940,7 @@ static void files_path(struct files_ref_store *refs, struct 
strbuf *sb,
 {
struct strbuf tmp = STRBUF_INIT;
va_list vap;
+   const char *ref;
 
va_start(vap, fmt);
strbuf_vaddf(, fmt, vap);
@@ -944,8 +948,14 @@ static void files_path(struct files_ref_store *refs, 
struct strbuf *sb,
if (refs->submodule)
strbuf_git_path_submodule(sb, refs->submodule,
  "%s", tmp.buf);
+   else if (!strcmp(tmp.buf, "packed-refs") || !strcmp(tmp.buf, "logs"))
+   strbuf_addf(sb, "%s/%s", refs->gitcommondir.buf, tmp.buf);
+   else if (is_per_worktree_ref(tmp.buf) ||
+(skip_prefix(tmp.buf, "logs/", ) &&
+ is_per_worktree_ref(ref)))
+   strbuf_addf(sb, "%s/%s", refs->gitdir.buf, tmp.buf);
else
-   strbuf_git_path(sb, "%s", tmp.buf);
+   strbuf_addf(sb, "%s/%s", refs->gitcommondir.buf, tmp.buf);
strbuf_release();
 }
 
@@ -1004,7 +1014,15 @@ 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);
+   strbuf_init(>gitdir, 0);
+   strbuf_init(>gitcommondir, 0);
+
+   if (submodule) {
+   refs->submodule = xstrdup(submodule);
+   } else {
+   strbuf_addstr(>gitdir, get_git_dir());
+   strbuf_addstr(>gitcommondir, get_git_common_dir());
+   }
 
return ref_store;
 }
-- 
2.11.0.157.gd943d85

[PATCH v3 09/16] 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 10994d992..ea13a5b86 100644
--- a/refs.c
+++ b/refs.c
@@ -1384,17 +1384,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;
@@ -1463,7 +1459,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;
@@ -1474,7 +1470,6 @@ struct ref_store *get_ref_store(const char *submodule)
strbuf_release(_sb);
}
}
-
return refs;
 }
 
-- 
2.11.0.157.gd943d85

[PATCH v3 08/16] 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 7a474198e..10994d992 100644
--- a/refs.c
+++ b/refs.c
@@ -1445,15 +1445,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 v3 14/16] 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().

The new code in init_submodule_ref_store() is basically a copy of
strbuf_git_path_submodule().

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() follows shortly. It's separate to keep
noises from this patch.

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

diff --git a/refs.c b/refs.c
index 25f657a6f..58ac9a2a8 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
@@ -1419,9 +1420,9 @@ static void register_submodule_ref_store(struct ref_store 
*refs,
 
 /*
  * Create, record, and return a ref_store instance for the specified
- * submodule (or the main repository if submodule is NULL).
+ * gitdir (or the main repository if gitdir is NULL).
  */
-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);
@@ -1430,7 +1431,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;
 }
 
@@ -1455,6 +1456,7 @@ 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();
@@ -1465,8 +1467,14 @@ struct ref_store *get_ref_store(const char *submodule)
return refs;
 
strbuf_addstr(_sb, submodule);
-   if (is_nonbare_repository_dir(_sb))
-   refs = ref_store_init(submodule);
+   ret = is_nonbare_repository_dir(_sb);
+   strbuf_release(_sb);
+   if (!ret)
+   return refs;
+
+   ret = submodule_to_gitdir(_sb, submodule);
+   if (!ret)
+   refs = ref_store_init(submodule_sb.buf);
strbuf_release(_sb);
 
if (refs)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 07cf2cb93..627466043 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -917,13 +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;
-
struct strbuf gitdir;
struct strbuf gitcommondir;
 
@@ -945,10 +938,7 @@ static void files_path(struct files_ref_store *refs, 
struct strbuf *sb,
va_start(vap, fmt);
strbuf_vaddf(, fmt, vap);
va_end(vap);
-   if (refs->submodule)
-   strbuf_git_path_submodule(sb, refs->submodule,
- "%s", tmp.buf);
-   else if (!strcmp(tmp.buf, "packed-refs") || !strcmp(tmp.buf, "logs"))
+   if (!strcmp(tmp.buf, "packed-refs") || !strcmp(tmp.buf, "logs"))
strbuf_addf(sb, "%s/%s", refs->gitcommondir.buf, tmp.buf);
else if (is_per_worktree_ref(tmp.buf) ||
 (skip_prefix(tmp.buf, "logs/", ) &&
@@ -1007,7 +997,7 @@ 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;
@@ -1017,8 +1007,9 @@ static struct ref_store *files_ref_store_create(const 
char *submodule)
strbuf_init(>gitdir, 0);
strbuf_init(>gitcommondir, 0);
 
-   if (submodule) {
-   refs->submodule = xstrdup(submodule);
+   if (gitdir) {
+   strbuf_addstr(>gitdir, gitdir);
+   get_common_dir_noenv(>gitcommondir, gitdir);
} else {
strbuf_addstr(>gitdir, get_git_dir());
strbuf_addstr(>gitcommondir, get_git_common_dir());
@@ -1034,8 +1025,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 

[PATCH v3 16/16] 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/files-backend.c |  2 +-
 refs/refs-internal.h | 12 
 4 files changed, 20 insertions(+), 17 deletions(-)

diff --git a/refs.c b/refs.c
index 58ac9a2a8..e7206a420 100644
--- a/refs.c
+++ b/refs.c
@@ -1160,7 +1160,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)
@@ -1333,10 +1333,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)
@@ -1452,13 +1452,17 @@ struct ref_store *get_main_ref_store(void)
return refs;
 }
 
-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 f803528fc..1287ba59c 100644
--- a/refs.h
+++ b/refs.h
@@ -554,5 +554,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/files-backend.c b/refs/files-backend.c
index d35032fcd..82be3f90f 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3117,7 +3117,7 @@ int set_worktree_head_symref(const char *gitdir, const 
char *target)
 * backends. This function needs to die.
 */
struct files_ref_store *refs =
-   files_downcast(get_ref_store(NULL), "set_head_symref");
+   files_downcast(get_main_ref_store(), "set_head_symref");
static struct lock_file head_lock;
struct ref_lock *lock;
struct strbuf head_path = STRBUF_INIT;
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index d7112770d..cb6882779 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -634,18 +634,6 @@ struct ref_store {
 void base_ref_store_init(struct ref_store *refs,
 const struct ref_storage_be *be);
 
-/*
- * Return the ref_store instance for the specified submodule. For the
- * main repository, use submodule==NULL; such a call cannot fail. For
- * a submodule, the submodule must exist and be a nonbare repository,
- * otherwise return NULL. If the requested reference store has not yet
- * been initialized, initialize it first.
- *
- * For backwards compatibility, submodule=="" is treated the same as
- * submodule==NULL.
- */
-struct ref_store *get_ref_store(const char *submodule);
-
 const char *resolve_ref_recursively(struct ref_store *refs,
const char *refname,
int resolve_flags,
-- 
2.11.0.157.gd943d85

[PATCH v3 12/16] 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 |  2 ++
 2 files changed, 20 insertions(+), 18 deletions(-)

diff --git a/refs.c b/refs.c
index 55a80a83d..25f657a6f 100644
--- a/refs.c
+++ b/refs.c
@@ -1303,7 +1303,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);
 }
@@ -1311,7 +1311,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);
 }
 
@@ -1434,7 +1434,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;
 
@@ -1483,14 +1483,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);
 }
@@ -1498,7 +1498,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);
@@ -1507,7 +1507,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);
 }
@@ -1517,14 +1517,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);
@@ -1535,7 +1535,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);
@@ -1544,14 +1544,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);
 }
@@ -1559,14 +1559,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 *refname)
 {
-   struct ref_store *refs = 

[PATCH v3 10/16] 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 | 19 +--
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/refs.c b/refs.c
index ea13a5b86..76a0e7b5a 100644
--- a/refs.c
+++ b/refs.c
@@ -1454,22 +1454,21 @@ 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;
 
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);
+   if (is_nonbare_repository_dir(_sb))
+   refs = ref_store_init(submodule);
+   strbuf_release(_sb);
return refs;
 }
 
-- 
2.11.0.157.gd943d85

[PATCH v3 13/16] 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  | 34 +++---
 submodule.c | 31 +++
 submodule.h |  1 +
 3 files changed, 39 insertions(+), 27 deletions(-)

diff --git a/path.c b/path.c
index efcedafba..3451d2916 100644
--- a/path.c
+++ b/path.c
@@ -475,35 +475,16 @@ const char *worktree_git_path(const struct worktree *wt, 
const char *fmt, ...)
 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 +495,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 ece17315d..3ce589d55 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1335,3 +1335,34 @@ void prepare_submodule_repo_env(struct argv_array *out)
}
argv_array_push(out, "GIT_DIR=.git");
 }
+
+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 23d76682b..2728494ce 100644
--- a/submodule.h
+++ b/submodule.h
@@ -70,6 +70,7 @@ extern int push_unpushed_submodules(struct sha1_array 
*commits,
int dry_run);
 void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir);
 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 v3 02/16] 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 | 111 +--
 1 file changed, 89 insertions(+), 22 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 75565c3aa..f0c878b92 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2169,6 +2169,8 @@ static int lock_packed_refs(struct files_ref_store *refs, 
int flags)
static int timeout_configured = 0;
static int timeout_value = 1000;
struct packed_ref_cache *packed_ref_cache;
+   struct strbuf sb = STRBUF_INIT;
+   int ret;
 
files_assert_main_repository(refs, "lock_packed_refs");
 
@@ -2177,10 +2179,13 @@ static int lock_packed_refs(struct files_ref_store 
*refs, int flags)
timeout_configured = 1;
}
 
-   if (hold_lock_file_for_update_timeout(
-   , git_path("packed-refs"),
-   flags, timeout_value) < 0)
+   strbuf_git_path(, "packed-refs");
+   ret = hold_lock_file_for_update_timeout(, sb.buf,
+   flags, timeout_value);
+   strbuf_release();
+   if (ret < 0)
return -1;
+
/*
 * Get the current packed-refs while holding the lock.  If the
 * packed-refs file has been modified since we last read it,
@@ -2335,6 +2340,9 @@ static void try_remove_empty_parents(char *name)
for (q = p; *q; q++)
;
while (1) {
+   struct strbuf sb = STRBUF_INIT;
+   int ret;
+
while (q > p && *q != '/')
q--;
while (q > p && *(q-1) == '/')
@@ -2342,7 +2350,10 @@ static void try_remove_empty_parents(char *name)
if (q == p)
break;
*q = '\0';
-   if (rmdir(git_path("%s", name)))
+   strbuf_git_path(, "%s", name);
+   ret = rmdir(sb.buf);
+   strbuf_release();
+   if (ret)
break;
}
 }
@@ -2431,7 +2442,11 @@ 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);
+   struct strbuf sb = STRBUF_INIT;
+
+   strbuf_git_path(, "packed-refs");
+   unable_to_lock_message(sb.buf, errno, err);
+   strbuf_release();
return -1;
}
packed = get_packed_refs(refs);
@@ -2529,8 +2544,10 @@ static int rename_tmp_log(const char *newrefname)
 {
int attempts_remaining = 4;
struct strbuf path = STRBUF_INIT;
+   struct strbuf tmp_renamed_log = STRBUF_INIT;
int ret = -1;
 
+   strbuf_git_path(_renamed_log, TMP_RENAMED_LOG);
  retry:
strbuf_reset();
strbuf_git_path(, "logs/%s", newrefname);
@@ -2546,7 +2563,7 @@ static int rename_tmp_log(const char *newrefname)
goto out;
}
 
-   if (rename(git_path(TMP_RENAMED_LOG), path.buf)) {
+   if (rename(tmp_renamed_log.buf, path.buf)) {
if ((errno==EISDIR || errno==ENOTDIR) && --attempts_remaining > 
0) {
/*
 * rename(a, b) when b is an existing
@@ -2574,6 +2591,7 @@ static int rename_tmp_log(const char *newrefname)
ret = 0;
 out:
strbuf_release();
+   strbuf_release(_renamed_log);
return ret;
 }
 
@@ -2614,9 +2632,15 @@ 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;
 
+   strbuf_git_path(_oldref, "logs/%s", oldrefname);
+   log = !lstat(sb_oldref.buf, );
+   strbuf_release(_oldref);
if (log && S_ISLNK(loginfo.st_mode))
return error("reflog for %s is a symlink", oldrefname);
 
@@ -2630,7 +2654,12 @@ static int files_rename_ref(struct ref_store *ref_store,
if (!rename_ref_available(oldrefname, newrefname))
return 1;
 
-   if (log && rename(git_path("logs/%s", oldrefname), 
git_path(TMP_RENAMED_LOG)))
+   strbuf_git_path(_oldref, "logs/%s", oldrefname);
+   strbuf_git_path(_renamed_log, TMP_RENAMED_LOG);
+ 

[PATCH v3 05/16] refs.c: share is_per_worktree_ref() to files-backend.c

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

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

diff --git a/refs.c b/refs.c
index 4f845798b..7a474198e 100644
--- a/refs.c
+++ b/refs.c
@@ -489,12 +489,6 @@ int dwim_log(const char *str, int len, unsigned char 
*sha1, char **log)
return logs_found;
 }
 
-static int is_per_worktree_ref(const char *refname)
-{
-   return !strcmp(refname, "HEAD") ||
-   starts_with(refname, "refs/bisect/");
-}
-
 static int is_pseudoref_syntax(const char *refname)
 {
const char *c;
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 59e65958a..f4aed49f5 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -651,4 +651,10 @@ const char *resolve_ref_recursively(struct ref_store *refs,
int resolve_flags,
unsigned char *sha1, int *flags);
 
+static inline int is_per_worktree_ref(const char *refname)
+{
+   return !strcmp(refname, "HEAD") ||
+   starts_with(refname, "refs/bisect/");
+}
+
 #endif /* REFS_REFS_INTERNAL_H */
-- 
2.11.0.157.gd943d85

[PATCH v3 04/16] files-backend: replace *git_path*() with files_path()

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

This centralizes all path rewriting of files-backend.c in one place so
we have easier time removing the path rewriting later. There could be
some hidden indirect git_path() though, I didn't audit the code to the
bottom.

Side note: set_worktree_head_symref() is a bad boy and should not be in
files-backend.c (probably should not exist in the first place). But
we'll leave it there until we have better multi-worktree support in refs
before we update it.

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

diff --git a/refs/files-backend.c b/refs/files-backend.c
index abb8a95e0..24f5bf7f1 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);
 
@@ -1178,12 +1179,10 @@ static void read_packed_refs(FILE *f, struct ref_dir 
*dir)
 static struct packed_ref_cache *get_packed_ref_cache(struct files_ref_store 
*refs)
 {
char *packed_refs_file;
+   struct strbuf sb = STRBUF_INIT;
 
-   if (refs->submodule)
-   packed_refs_file = git_pathdup_submodule(refs->submodule,
-"packed-refs");
-   else
-   packed_refs_file = git_pathdup("packed-refs");
+   files_path(refs, , "packed-refs");
+   packed_refs_file = strbuf_detach(, NULL);
 
if (refs->packed &&
!stat_validity_check(>packed->validity, packed_refs_file))
@@ -1249,10 +1248,7 @@ static void read_loose_refs(const char *dirname, struct 
ref_dir *dir)
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_path(refs, , "%s", dirname);
path_baselen = path.len;
 
if (err) {
@@ -1394,10 +1390,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_path(refs, _path, "%s", refname);
 
path = sb_path.buf;
 
@@ -1585,7 +1578,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_path(refs, _file, "%s", refname);
 
 retry:
switch (safe_create_leading_directories(ref_file.buf)) {
@@ -2052,7 +2045,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_path(refs, _file, "%s", refname);
resolved = !!resolve_ref_unsafe(refname, resolve_flags,
lock->old_oid.hash, type);
if (!resolved && errno == EISDIR) {
@@ -2197,7 +2190,7 @@ static int lock_packed_refs(struct files_ref_store *refs, 
int flags)
timeout_configured = 1;
}
 
-   strbuf_git_path(, "packed-refs");
+   files_path(refs, , "packed-refs");
ret = hold_lock_file_for_update_timeout(, sb.buf,
flags, timeout_value);
strbuf_release();
@@ -2343,7 +2336,7 @@ static int pack_if_possible_fn(struct ref_entry *entry, 
void *cb_data)
  * Remove empty parents, but spare refs/ and immediate subdirs.
  * Note: munges *name.
  */
-static void try_remove_empty_parents(char *name)
+static void try_remove_empty_parents(struct files_ref_store *refs, char *name)
 {
char *p, *q;
int i;
@@ -2368,7 +2361,7 @@ static void try_remove_empty_parents(char *name)
if (q == p)
break;
*q = '\0';
-   strbuf_git_path(, "%s", name);
+   files_path(refs, , "%s", name);
ret = rmdir(sb.buf);
strbuf_release();
if (ret)
@@ -2377,7 +2370,7 @@ static void try_remove_empty_parents(char *name)
 }
 
 /* make sure nobody touched the ref, and 

[PATCH v3 03/16] files-backend: add files_path()

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

This will be the replacement for both git_path() and git_path_submodule()
in this file. The idea is backend takes a git path and use that,
oblivious of submodule, linked worktrees and such.

This is the middle step towards that. Eventually the "submodule" field
in 'struct files_ref_store' should be replace by "gitdir". And a
compound ref_store is created to combine two files backends together,
one represents the shared refs in $GIT_COMMON_DIR, one per-worktree. At
that point, files_path() becomes a wrapper of strbuf_vaddf().

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

diff --git a/refs/files-backend.c b/refs/files-backend.c
index f0c878b92..abb8a95e0 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -930,6 +930,24 @@ struct files_ref_store {
 /* Lock used for the main packed-refs file: */
 static struct lock_file packlock;
 
+__attribute__((format (printf, 3, 4)))
+static void files_path(struct files_ref_store *refs, struct strbuf *sb,
+  const char *fmt, ...)
+{
+   struct strbuf tmp = STRBUF_INIT;
+   va_list vap;
+
+   va_start(vap, fmt);
+   strbuf_vaddf(, fmt, vap);
+   va_end(vap);
+   if (refs->submodule)
+   strbuf_git_path_submodule(sb, refs->submodule,
+ "%s", tmp.buf);
+   else
+   strbuf_git_path(sb, "%s", tmp.buf);
+   strbuf_release();
+}
+
 /*
  * Increment the reference count of *packed_refs.
  */
-- 
2.11.0.157.gd943d85

[PATCH v3 00/16] Remove submodule from files-backend.c

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

v3 only changes 07/16 but it's kinda important because I broke
packed-refs. packed-refs' path became $GIT_DIR/packed-refs instead of
$GIT_COMMON_DIR/packed-refs and as a result the majority of refs will
disappear in linked worktrees. Interdiff

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 685ea5c14..82be3f90f 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -938,9 +938,11 @@ static void files_path(struct files_ref_store *refs, 
struct strbuf *sb,
va_start(vap, fmt);
strbuf_vaddf(, fmt, vap);
va_end(vap);
-   if (is_per_worktree_ref(tmp.buf) ||
-   (skip_prefix(tmp.buf, "logs/", ) &&
-is_per_worktree_ref(ref)))
+   if (!strcmp(tmp.buf, "packed-refs") || !strcmp(tmp.buf, "logs"))
+   strbuf_addf(sb, "%s/%s", refs->gitcommondir.buf, tmp.buf);
+   else if (is_per_worktree_ref(tmp.buf) ||
+(skip_prefix(tmp.buf, "logs/", ) &&
+ is_per_worktree_ref(ref)))
strbuf_addf(sb, "%s/%s", refs->gitdir.buf, tmp.buf);
else
strbuf_addf(sb, "%s/%s", refs->gitcommondir.buf, tmp.buf);

Nguyễn Thái Ngọc Duy (16):
  refs-internal.c: make files_log_ref_write() static
  files-backend: convert git_path() to strbuf_git_path()
  files-backend: add files_path()
  files-backend: replace *git_path*() with files_path()
  refs.c: share is_per_worktree_ref() to files-backend.c
  refs-internal.h: correct is_per_worktree_ref()
  files-backend: remove the use of git_path()
  refs.c: introduce get_main_ref_store()
  refs: rename lookup_ref_store() to lookup_submodule_ref_store()
  refs.c: flatten get_ref_store() a bit
  refs.c: kill register_ref_store(), add register_submodule_ref_store()
  refs.c: make get_main_ref_store() public and use it
  path.c: move some code out of strbuf_git_path_submodule()
  refs: move submodule code out of files-backend.c
  files-backend: remove submodule_allowed from files_downcast()
  refs: rename get_ref_store() to get_submodule_ref_store() and make it public

 path.c   |  34 ++---
 refs.c   | 144 +++--
 refs.h   |  13 ++
 refs/files-backend.c | 349 +++
 refs/refs-internal.h |  28 ++---
 submodule.c  |  31 +
 submodule.h  |   1 +
 7 files changed, 348 insertions(+), 252 deletions(-)

-- 
2.11.0.157.gd943d85

[PATCH v3 06/16] refs-internal.h: correct is_per_worktree_ref()

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

All refs outside refs/ directory is per-worktree, not just HEAD.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 refs/refs-internal.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index f4aed49f5..69d02b6ba 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -653,7 +653,7 @@ const char *resolve_ref_recursively(struct ref_store *refs,
 
 static inline int is_per_worktree_ref(const char *refname)
 {
-   return !strcmp(refname, "HEAD") ||
+   return !starts_with(refname, "refs/") ||
starts_with(refname, "refs/bisect/");
 }
 
-- 
2.11.0.157.gd943d85

[PATCH v3 01/16] refs-internal.c: 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 cdb6b8ff5..75565c3aa 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 33adbf93b..59e65958a 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -222,10 +222,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

Re: git alias for options

[Ævar Arnfjörð Bjarmason]

On Fri, Feb 17, 2017 at 9:23 AM, hIpPy  wrote:
> Git has aliases for git commands. Is there a (an inbuilt) way to alias
> options? If not, what is the reason?

This has long been on my  wishlist, because there's a lot of
copy/pasted logic all over Git to make git foo --whatever aliased to
foo.whatever in the config, but only for some options.

It should ideally be part of something every option just supports, via
the getopts struct.

However, it can't allow you to modify whatever option you want,
because some things like git-commit-tree should not be customized
based on config, it would break things that rely on the plumbing
commands.

So it would have to be a whitelist for each option we allow to be
configured like this via the getopts struct.

Also there are surely other edge cases, like maybe the config option
now doesn't 1=1 map to the name for the option in some cases, or the
flag should be config-able but is has no long form (which we'd like
for the config), then we'd want to add that etc.

Re: body-CC-comment regression

[Matthieu Moy]

Johan Hovold  writes:

> There is another option, namely to only accept a single address for tags
> in the body. I understand that being able to copy a CC-header to either
> the header section or to the command line could be useful, but I don't
> really see the point in allowing this in the tags in the body (a SoB
> always has one address, and so should a CC-tag).

I mostly agree for the SoB, but why should a Cc tag have only one email?

The "multiple emails per Cc: field" has been there for a while already
(b1c8a11c8024 released in 2.6.0, sept 2015), some users may have got
used to it. What you are proposing breaks their flow.

> And since this is a regression for something that has been working for
> years that was introduced by a new feature, I also think it's reasonable
> to (partially) revert the feature.

I'd find it rather ironic to fix your case by breaking a feature that
has been working for more than a year :-(. What would you answer to a
contributor comming one year from now and proposing to revert your
reversion because it breaks his flow?

All that said, I think another fix would be both satisfactory for
everyone and rather simple:

1) Stop calling Mail::Address even if available. It used to make sense
   to do that when our in-house parser was really poor, but we now have
   something essentially as good as Mail::Address. We test our parser
   against Mail::Address and we do have a few known differences (see
   t9000), but they are really corner-cases and shouldn't matter.

   A good consequence of this is that we stop depending on the way Perl
   is installed to parse emails. Regardless of the current issue, I
   think it is a good thing.

2) Modify our in-house parser to discard garbage after the >. The patch
   should look like (untested):

--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -903,11 +903,11 @@ sub parse_mailboxes {
my (@addr_list, @phrase, @address, @comment, @buffer) = ();
foreach my $token (@tokens) {
if ($token =~ /^[,;]$/) {
-   # if buffer still contains undeterminated strings
-   # append it at the end of @address or @phrase
-   if ($end_of_addr_seen) {
-   push @phrase, @buffer;
-   } else {
+   # if buffer still contains undeterminated
+   # strings append it at the end of @address,
+   # unless we already saw the closing >, in
+   # which case we discard it.
+   if (!$end_of_addr_seen) {
push @address, @buffer;
}
 
What do you think?

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: git alias for options

[Michael J Gruber]

hIpPy venit, vidit, dixit 17.02.2017 09:23:
> Git has aliases for git commands. Is there a (an inbuilt) way to alias
> options? If not, what is the reason?
> 
> Thanks,
> hippy
> 

You can setup an alias for "command with options", for example:

git help s
`git s' is aliased to `status -s -b -uno .'

As you see here, this can include non-option arguments such as the
pathsepc '.'.

You cannot alias options independent of the command, though.

Michael

dotfiles in git template dir are not copied

[greg0ire]

Hi,

I noticed yesterday that dotfiles inside the directory configured in 
init.templatedir are not copied when creating a new repository.


Here is the line I think is responsible for this behavior : 
https://github.com/git/git/blob/master/builtin/init-db.c#L48


Is it a bug or a feature?

Steps to reproduce, provided you already have a template dir configured :

cd $(git config --path --get init.templatedir)
touch copied
touch .not_copied
cd /tmp
mkdir whatever
cd whatever
git init
ls -la .git

On my machine, the last command does not list .not_copied .

--
greg0ire

Re: body-CC-comment regression

[Johan Hovold]

On Thu, Feb 16, 2017 at 07:16:57PM +0100, Matthieu Moy wrote:
> Johan Hovold  writes:
> 
> > Hi,
> >
> > I recently noticed that after an upgrade, git-send-email (2.10.2)
> > started aborting when trying to send patches that had a linux-kernel
> > stable-tag in its body. For example,
> >
> > Cc: # 4.4
> >
> > was now parsed as
> >
> > "sta...@vger.kernel.org#4.4"
> >
> > which resulted in
> >
> > Died at /usr/libexec/git-core/git-send-email line 1332,  line 1.
> 
> This has changed in e3fdbcc8e1 (parse_mailboxes: accept extra text after
> <...> address, 2016-10-13), released v2.11.0 as you noticed:
> 
> > The problem with the resulting fixes that are now in 2.11.1 is that
> > git-send-email no longer discards the trailing comment but rather
> > shoves it into the name after adding some random white space:
> >
> > "# 3 . 3 . x : 1b9508f : sched : Rate-limit newidle" 
> > "
> >
> > This example is based on the example from
> > Documentation/process/stable-kernel-rules.rst:
> >
> > Cc:  # 3.3.x: 1b9508f: sched: Rate-limit newidle
> >
> > and this format for stable-tags has been documented at least since 2009
> > and 8e9b9362266d ("Doc/stable rules: add new cherry-pick logic"), and
> > has been supported by git since 2012 and 831a488b76e0 ("git-send-email:
> > remove garbage after email address") I believe.
> >
> > Can we please revert to the old behaviour of simply discarding such
> > comments (from body-CC:s) or at least make it configurable through a
> > configuration option?
> 
> The problem is that we now accept list of emails instead of just one
> email, so it's hard to define what "comments after the email", for
> example
> 
> Cc:  # , 
> 
> Is not accepted as two emails.
> 
> So, just stripping whatever comes after # before parsing the list of
> emails would change the behavior once more, and possibly break other
> user's flow. Dropping the garbage after the email while parsing is
> possible, but only when we use our in-house parser (and we currently use
> Perl's Mail::Address when available).
> 
> So, a proper fix is far from obvious, and unfortunately I won't have
> time to work on that, at least not before a while.

There is another option, namely to only accept a single address for tags
in the body. I understand that being able to copy a CC-header to either
the header section or to the command line could be useful, but I don't
really see the point in allowing this in the tags in the body (a SoB
always has one address, and so should a CC-tag).

And since this is a regression for something that has been working for
years that was introduced by a new feature, I also think it's reasonable
to (partially) revert the feature.

> OTOH, the current behavior isn't that bad. It accepts the input, and
> extracts a valid email out of it. Just the display name is admitedly
> suboptimal ...

Yeah, but the display name can end up with so much noise that auto-cc is
effectively broken for people submitting kernel patches (with stable
tags) as the only way to avoid it is to suppress all bodycc.

So what I'm proposing is to revert to the earlier behaviour of only
allowing one address per body tag by simply discarding anything
after the address.

Something like the below seems to do the trick.

Thanks,
Johan



>From f551b4ca9926624dc7af6c286d7cf0f97af39541 Mon Sep 17 00:00:00 2001
From: Johan Hovold 
Date: Fri, 17 Feb 2017 11:55:47 +0100
Subject: [PATCH] send-email: only allow one address per body tag

Adding comments after a tag in the body is a common practise (e.g. in
the Linux kernel) and git-send-email has been supporting this for years
by removing any trailing cruft after the address.

After some recent changes, any trailing comment is now instead appended
to the recipient name (with some random white space inserted) resulting
in undesirable noise in the headers, for example:

CC: "# 3 . 3 . x : 1b9508f : sched : Rate-limit newidle" 


Revert to the earlier behaviour of discarding anything after the (first)
address in a tag while parsing the body.

Note that multiple addresses after are still allowed after a
command-line switch (and in a CC-header).

Fixes: b1c8a11c8024 ("send-email: allow multiple emails using --cc, --to
and --bcc")
Fixes: e3fdbcc8e164 ("parse_mailboxes: accept extra text after <...>
address")
Signed-off-by: Johan Hovold 
---
 git-send-email.perl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 068d60b3e698..eea0a517f71b 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1563,7 +1563,7 @@ foreach my $t (@files) {
# Now parse the message body
while(<$fh>) {
$message .=  $_;
-   if (/^(Signed-off-by|Cc): (.*)$/i) {
+   if (/^(Signed-off-by|Cc): ([^>]*>?)/i) 

Re: [PATCH] tempfile: avoid "ferror | fclose" trick

[Michael Haggerty]

On 02/17/2017 09:07 AM, Jeff King wrote:
> [...]
> That's similar to what I wrote earlier, but if we don't mind overwriting
> errno unconditionally, I think just:
> 
>   errno = EIO; /* covers ferror(), overwritten by failing fclose() */
>   err |= ferror(fp);
>   err |= fclose(fp);
> 
> does the same thing.

True; I'd forgotten the convention that non-failing functions are
allowed to change errno. Your solution is obviously simpler and faster.

Michael

Re: [PATCH v2 00/19] object_id part 6

[Michael Haggerty]

On 02/14/2017 03:31 AM, brian m. carlson wrote:
> This is another series in the continuing conversion to struct object_id.
> 
> This series converts more of the builtin directory and some of the refs
> code to use struct object_id. Additionally, it implements an
> nth_packed_object_oid function which provides a struct object_id version
> of the nth_packed_object function, and a parse_oid_hex function that
> makes parsing easier.
> 
> The patch to use parse_oid_hex in the refs code has been split out into
> its own patch, just because I'm wary of that code and potentially
> breaking things, and I want it to be easy to revert in case things go
> wrong.  I have no reason to believe it is anything other than fully
> functional, however.
> 
> Changes from v1:
> * Implement parse_oid_hex and use it.
> * Make nth_packed_object_oid take a variable into which to store the
>   object ID.  This avoids concerns about unsafe casts.
> * Rebase on master.

Thanks as always for working on this!

I skimmed over the patches (looking more carefully at the refs-related
ones) and left a few minor comments but didn't find anything serious.

I'm curious; what fraction of the overall convert-to-object_id campaign
do you estimate is done so far? Are you getting close to the promised
land yet?

Michael

Re: [PATCH v2 15/19] refs: simplify parsing of reflog entries

[Michael Haggerty]

On 02/14/2017 03:31 AM, brian m. carlson wrote:
> The current code for reflog entries uses a lot of hard-coded constants,
> making it hard to read and modify.  Use parse_oid_hex and two temporary
> variables to simplify the code and reduce the use of magic constants.
> 
> Signed-off-by: brian m. carlson 
> ---
>  refs/files-backend.c | 12 +++-
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index d7a5fd2a7c..09227a3f63 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -3117,12 +3117,14 @@ static int show_one_reflog_ent(struct strbuf *sb, 
> each_reflog_ent_fn fn, void *c
>   char *email_end, *message;
>   unsigned long timestamp;
>   int tz;
> + const char *p = sb->buf;
> + const int minlen = 2 * GIT_SHA1_HEXSZ + 3;
>  
>   /* old SP new SP name  SP time TAB msg LF */
> - if (sb->len < 83 || sb->buf[sb->len - 1] != '\n' ||
> - get_oid_hex(sb->buf, ) || sb->buf[40] != ' ' ||
> - get_oid_hex(sb->buf + 41, ) || sb->buf[81] != ' ' ||
> - !(email_end = strchr(sb->buf + 82, '>')) ||
> + if (sb->len < minlen || sb->buf[sb->len - 1] != '\n' ||
> + parse_oid_hex(p, , ) || *p++ != ' ' ||
> + parse_oid_hex(p, , ) || *p++ != ' ' ||
> + !(email_end = strchr(p, '>')) ||
>   email_end[1] != ' ' ||
>   !(timestamp = strtoul(email_end + 2, , 10)) ||
>   !message || message[0] != ' ' ||
> @@ -3136,7 +3138,7 @@ static int show_one_reflog_ent(struct strbuf *sb, 
> each_reflog_ent_fn fn, void *c
>   message += 6;
>   else
>   message += 7;
> - return fn(, , sb->buf + 82, timestamp, tz, message, cb_data);
> + return fn(, , sb->buf + minlen - 1, timestamp, tz, message, 
> cb_data);

I think `sb->buf + minlen - 1` is just `p` here, isn't it?

Also, I think instead of the initial test `sb->len < minlen`, it would
be enough to check `!sb->len` (i.e., error if it's zero to avoid a
buffer underflow in `sb->buf[sb->len - 1])`, because the
`parse_oid_hex()` calls together with the `*p++ != ' '` and
`sb->buf[sb->len - 1] != '\n'` checks already ensure that the string has
at least the minimum length.

Then you could do away with `minlen` entirely.

Michael

Re: topological index field for commit objects

[Jakub Narębski]

On 17 February 2017 at 10:26, Jeff King  wrote:
> On Sat, Feb 04, 2017 at 02:43:01PM +0100, Jakub Narębski wrote:
>
>>  Do Git use EWAH / EWOK bitmaps for reachability analysis, or is it still
>>  limited to object counting?
>> >>>
>> >>> At GitHub we are using them for --contains analysis, along with mass
>> >>> ahead/behind (e.g., as in https://github.com/gitster/git/branches). My
>> >>> plan is to send patches upstream, but they need some cleanup first.
>> >>
>> >> Ping. have you got time to clean up those patches?
>> >
>> > No, I haven't. Don't hold your breath; it's something I hope to work on
>> > in the next 6 months, not the next 6 weeks.
>>
>> Ping, Was there any progress on this front? It is now almost 6 months
>> later...
>>
>> Could those patches be made available in a "dirty" form?
>
> I just pushed them up to the "jk/ahead-behind" branch of
> https://github.com/peff/git.
>
> They were originally done on top of v1.9.1, but I forward-ported them to
> the current "master" just now. The result compiles, but I haven't really
> tested it extensively. Caveat applier.

Thanks a lot! I'll check this out.

-- 
Jakub Narebski

Re: [PATCH v2 14/19] hex: introduce parse_oid_hex

[Michael Haggerty]

On 02/14/2017 03:31 AM, brian m. carlson wrote:
> Introduce a function, parse_oid_hex, which parses a hexadecimal object
> ID and if successful, sets a pointer to just beyond the last character.
> This allows for simpler, more robust parsing without needing to
> hard-code integer values throughout the codebase.
> 
> Signed-off-by: brian m. carlson 
> ---
>  cache.h | 8 
>  hex.c   | 8 
>  2 files changed, 16 insertions(+)
> 
> diff --git a/cache.h b/cache.h
> index 61fc86e6d7..5dc89a058c 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1319,6 +1319,14 @@ extern char *oid_to_hex_r(char *out, const struct 
> object_id *oid);
>  extern char *sha1_to_hex(const unsigned char *sha1); /* static buffer 
> result! */
>  extern char *oid_to_hex(const struct object_id *oid);/* same static 
> buffer as sha1_to_hex */
>  
> +/*
> + * Parse a hexadecimal object ID starting from hex, updating the pointer
> + * specified by p when parsing stops.  The resulting object ID is stored in 
> oid.
> + * Returns 0 on success.  Parsing will stop on the first NUL or other invalid
> + * character.
> + */
> +extern int parse_oid_hex(const char *hex, struct object_id *oid, const char 
> **p);
> +

I like this function. This is a convenient kind of interface to work
with. A few minor comments:

If you rename the nondescript `p` parameter to, say, `end`, its purpose
would be more transparent. Alternatively, `skip_prefix()` calls the
corresponding parameter `out`.

It would be nice for the docstring to mention that the object ID must be
a full, 40-character hex string. Otherwise "Parsing will stop on the
first NUL or other invalid character" makes it sound like the function
might be satisfied with fewer than 40 characters.

Finally, you might mention the useful fact that `p` is only updated if
the function succeeds.

> [...]

Michael

Re: topological index field for commit objects

[Jeff King]

On Sat, Feb 04, 2017 at 02:43:01PM +0100, Jakub Narębski wrote:

>  Do Git use EWAH / EWOK bitmaps for reachability analysis, or is it still
>  limited to object counting?
> >>>
> >>> At GitHub we are using them for --contains analysis, along with mass
> >>> ahead/behind (e.g., as in https://github.com/gitster/git/branches). My
> >>> plan is to send patches upstream, but they need some cleanup first.
> >>
> >> Ping. have you got time to clean up those patches?
> >
> > No, I haven't. Don't hold your breath; it's something I hope to work on
> > in the next 6 months, not the next 6 weeks.
> 
> Ping, Was there any progress on this front? It is now almost 6 months
> later...
> 
> Could those patches be made available in a "dirty" form?

I just pushed them up to the "jk/ahead-behind" branch of
https://github.com/peff/git.

They were originally done on top of v1.9.1, but I forward-ported them to
the current "master" just now. The result compiles, but I haven't really
tested it extensively. Caveat applier.

-Peff

Re: [PATCH v2 11/19] builtin/replace: convert to struct object_id

[Michael Haggerty]

On 02/14/2017 03:31 AM, brian m. carlson wrote:
> Convert various uses of unsigned char [20] to struct object_id.  Rename
> replace_object_sha1 to rename_object_oid.  Finally, specify a constant
> in terms of GIT_SHA1_HEXSZ.

The new name is not rename_object_oid but rather replace_object_oid.

> [...]

Michael

Re: [PATCH 3/3] rename_ref: replace empty deletion message in HEAD's log

[Jeff King]

On Thu, Feb 16, 2017 at 10:58:00PM -0500, Kyle Meyer wrote:

> When the current branch is renamed, the deletion of the old ref is
> recorded in HEAD's log with an empty message.  Now that delete_refs()
> accepts a reflog message, provide a more descriptive message.  This
> message will not be included in the reflog of the renamed branch, but
> its log already covers the renaming event with a message of "Branch:
> renamed ...".

Right, makes sense. The code overall looks fine, though I have one
nit:

> @@ -2616,10 +2617,15 @@ static int files_rename_ref(struct ref_store 
> *ref_store,
>   return error("unable to move logfile logs/%s to 
> "TMP_RENAMED_LOG": %s",
>   oldrefname, strerror(errno));
>  
> - if (delete_ref(oldrefname, orig_sha1, REF_NODEREF, NULL)) {
> + strbuf_addf(_del, "Deleted %s", oldrefname);

We've been anything but consistent with our reflog messages in the past,
but I think the general guideline for new ones is to use "command:
action". Of course we don't _know_ the action here, because we're deep
in rename_ref().

Should we have the caller pass it in for us, as we do with delete_ref()
and update_ref()?

I see we actually already have a "logmsg" parameter. It already says
"Branch: renamed %s to %s". Could we just reuse that? I know that this
step is technically just the deletion, but I think it more accurately
describes the whole operation that the deletion is part of.

-Peff

git alias for options

[hIpPy]

Git has aliases for git commands. Is there a (an inbuilt) way to alias
options? If not, what is the reason?

Thanks,
hippy

Re: [PATCH 2/3] update-ref: pass reflog message argument to delete_refs

[Jeff King]

On Thu, Feb 16, 2017 at 10:57:59PM -0500, Kyle Meyer wrote:

> Now that delete_refs() accepts a reflog message, pass the
> user-provided message to delete_refs() rather than silently dropping
> it.  The doesn't matter for the deleted ref's log because the log is
> deleted along with the ref, but this entry will show up in HEAD's
> reflog when deleting a checked out branch.

Sounds good.

> diff --git a/builtin/update-ref.c b/builtin/update-ref.c
> index a41f9adf1..f642acc22 100644
> --- a/builtin/update-ref.c
> +++ b/builtin/update-ref.c
> @@ -435,7 +435,7 @@ int cmd_update_ref(int argc, const char **argv, const 
> char *prefix)
>*/
>   return delete_ref(refname,
> (oldval && !is_null_sha1(oldsha1)) ? oldsha1 
> : NULL,
> -   flags, NULL);
> +   flags, msg);

This looks obviously correct.

> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
> index b0ffc0b57..65918d984 100755
> --- a/t/t1400-update-ref.sh
> +++ b/t/t1400-update-ref.sh
> @@ -85,6 +85,15 @@ test_expect_success "delete $m (by HEAD)" '
>  '
>  rm -f .git/$m
>  
> +test_expect_success "deleting current branch adds message to HEAD's log" '
> + git update-ref $m $A &&
> + git symbolic-ref HEAD $m &&
> + git update-ref -mdelmsg -d $m &&
> + ! test -f .git/$m &&
> + grep "delmsg$" .git/logs/HEAD >/dev/null
> +'
> +rm -f .git/$m

I think covering this with a test is good.

I don't know if it's also worth testing that deleting via HEAD also
writes the reflog. I.e.,:

  git update-ref -m delete-by-head -d HEAD

Some of the style here is a bit out-dated, but I think you are just
matching the surrounding tests.  So that's OK by me (though a patch to
modernize the whole thing would be welcome, too).

For reference, the two things I notice are:

  - we prefer test_path_is_missing to "! test -f" these days.

  - we don't redirect the output of grep (it's handled already in
non-verbose mode, and in verbose mode we try to be...verbose).

-Peff

Re: [PATCH 0/3] delete_ref(): support reflog messages

[Jeff King]

On Thu, Feb 16, 2017 at 10:57:57PM -0500, Kyle Meyer wrote:

> [Sorry for the slow response.]

No problem. The pace of open source varies wildly. :)

> >   - "git branch -m" does seem to realize when we are renaming HEAD,
> > because it updates HEAD to point to the new branch name. But it
> > should probably insert another reflog entry mentioning the rename
> > (we do for "git checkout foo", even when "foo" has the same sha1 as
> > the current HEAD).
> 
> I haven't worked out how to do this part yet.  I'm guessing the change
> will involve modifying split_head_update().
> 
> If this is added, should it be instead of, rather than in addition to,
> the deletion entry?  If a "Branch: renamed ..." entry is present, it
> doesn't seem like the deletion entry is providing any additional
> information.

I think you could do an "instead of" that goes from sha1 X to X, and
just mentions the rename. Or you could add a second entry after the
delete that takes it from 0{40} back to X.

I suspect the latter is easier to do, and I doubt anybody would care
that much of the exact form. These entries aren't really doing anything
for reachability. They're just giving an audit log of what happened. So
I don't think anybody would really care unless they were debugging a
confusing situation by hand. And as long as there's enough information
to figure out what happened, they'll be happy.

-Peff

Re: [PATCH 1/3] delete_refs(): accept a reflog message argument

[Jeff King]

On Thu, Feb 16, 2017 at 10:57:58PM -0500, Kyle Meyer wrote:

> When the current branch is renamed with 'git branch -m/-M' or deleted
> with 'git update-ref -m -d', the event is recorded in HEAD's log
> with an empty message.
> 
> In preparation for adding a more meaningful message to HEAD's log in
> these cases, update delete_ref() to take a message argument and pass
> it along to ref_transaction_delete().  Modify all callers to pass NULL
> for the new message argument; no change in behavior is intended.

Seems like a good first step.

> diff --git a/refs.h b/refs.h
> index 9fbff90e7..81627a63d 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -277,7 +277,7 @@ int reflog_exists(const char *refname);
>   * be NULL_SHA1. flags is passed through to ref_transaction_delete().
>   */
>  int delete_ref(const char *refname, const unsigned char *old_sha1,
> -unsigned int flags);
> +unsigned int flags, const char *msg);

Should the "msg" argument go at the beginning, to match update_ref()?

-Peff

Re: [PATCH] tempfile: avoid "ferror | fclose" trick

[Jeff King]

On Fri, Feb 17, 2017 at 09:00:09AM +0100, Michael Haggerty wrote:

> As you pointed out, if ferror() fails, it doesn't set errno properly. At
> least one caller tries to strerror(errno), so it would probably be good
> to store *something* in there, probably EIO.

Yeah, we discussed this up-thread a bit, and my "solution" was similar
to yours. I don't like it, because EIO is a real thing that can happen,
too, and it would certainly be surprising to a user to see. But it's
probably better than the alternative, which is getting whatever random
value happened to be in errno.

The only downside is that if the value of errno _was_ valid (because the
last thing you did really was writing to the filehandle, then we'd
overwrite it).

> To be really pedantic, there's also the question of what errno the
> caller would want if *both* ferror() and fclose() fail. Normally I would
> say "the first error that occurred", but in this case we don't know the
> correct errno from the error reported by ferror(), so maybe the fclose()
> errno is more likely to hint at the underlying reason for the failure.

Yes, I think we're better to take what fclose gives us, if we can.

> So I (reluctantly) propose
> 
>   if (ferror(fp)) {
>   if (!fclose(fp)) {
>   /*
>* ferror() doesn't set errno, so we have to
>* set one. (By contrast, when fclose() fails
>* too, we leave *its* errno in place.)
>*/
>   errno = EIO;
>   }
>   return -1;
>   }
>   return fclose();

That's similar to what I wrote earlier, but if we don't mind overwriting
errno unconditionally, I think just:

  errno = EIO; /* covers ferror(), overwritten by failing fclose() */
  err |= ferror(fp);
  err |= fclose(fp);

does the same thing.

-Peff

Re: [PATCH] tempfile: avoid "ferror | fclose" trick

[Michael Haggerty]

On 02/16/2017 10:31 PM, Jeff King wrote:
> On Thu, Feb 16, 2017 at 11:43:59AM -0500, Jeff King wrote:
> 
>> On Thu, Feb 16, 2017 at 11:10:18AM +0100, Andreas Schwab wrote:
>>
>   int xfclose(FILE *fp)
>   {
>   return ferror(fp) | fclose(fp);
>   }

 Yes, that's exactly what I had in mind (might be worth calling out the
 bitwise-OR, though, just to make it clear it's not a typo).
>>>
>>> Since the order of evaluation is unspecified, it would be better to
>>> force sequencing ferror before fclose.
>>
>> Good point. Arguably the call in tempfile.c is buggy.
> 
> Here's a fix.
> 
> I think close_tempfile() suffers from the same errno problem discussed
> earlier in this thread (i.e., that after calling it, you may get an
> error return with a random, unrelated errno value if ferror() failed but
> fclose() did not).
> 
> -- >8 --
> Subject: [PATCH] tempfile: avoid "ferror | fclose" trick
> 
> The current code wants to record an error condition from
> either ferror() or fclose(), but makes sure that we always
> call both functions. So it can't use logical-OR "||", which
> would short-circuit when ferror() is true. Instead, it uses
> bitwise-OR "|" to evaluate both functions and set one or
> more bits in the "err" flag if they reported a failure.
> 
> Unlike logical-OR, though, bitwise-OR does not introduce a
> sequence point, and the order of evaluation for its operands
> is unspecified. So a compiler would be free to generate code
> which calls fclose() first, and then ferror() on the
> now-freed filehandle.
> 
> There's no indication that this has happened in practice,
> but let's write it out in a way that follows the standard.
> 
> Noticed-by: Andreas Schwab 
> Signed-off-by: Jeff King 
> ---
>  tempfile.c | 8 ++--
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/tempfile.c b/tempfile.c
> index 2990c9242..ffcc27237 100644
> --- a/tempfile.c
> +++ b/tempfile.c
> @@ -247,12 +247,8 @@ int close_tempfile(struct tempfile *tempfile)
>   tempfile->fd = -1;
>   if (fp) {
>   tempfile->fp = NULL;
> -
> - /*
> -  * Note: no short-circuiting here; we want to fclose()
> -  * in any case!
> -  */
> - err = ferror(fp) | fclose(fp);
> + err = ferror(fp);
> + err |= fclose(fp);
>   } else {
>   err = close(fd);
>   }
> 

Thanks for fixing this; the old code was definitely wrong.

As you pointed out, if ferror() fails, it doesn't set errno properly. At
least one caller tries to strerror(errno), so it would probably be good
to store *something* in there, probably EIO.

To be really pedantic, there's also the question of what errno the
caller would want if *both* ferror() and fclose() fail. Normally I would
say "the first error that occurred", but in this case we don't know the
correct errno from the error reported by ferror(), so maybe the fclose()
errno is more likely to hint at the underlying reason for the failure.

So I (reluctantly) propose

if (ferror(fp)) {
if (!fclose(fp)) {
/*
 * ferror() doesn't set errno, so we have to
 * set one. (By contrast, when fclose() fails
 * too, we leave *its* errno in place.)
 */
errno = EIO;
}
return -1;
}
return fclose();

Michael