Re: What's cooking in git.git (Jun 2017, #09; Fri, 30)

[Ævar Arnfjörð Bjarmason]

On Fri, Jun 30 2017, Junio C. Hamano jotted:

> * xz/send-email-batch-size (2017-05-23) 1 commit
>  - send-email: --batch-size to work around some SMTP server limit
>
>  "git send-email" learned to overcome some SMTP server limitation
>  that does not allow many pieces of e-mails to be sent over a single
>  session.
>
>  Waiting for a response.
>  cf. 
>  cf. 
>
>  """I thought your wish (which I found reasonable) was to record
>  whatever information that would help us in the future in the log
>  message?  I was waiting for that to happen."""

I think it's fine in lieu of xiaoqiang zhao not being responsive to just
merge this as-is. The info that can help us in the future is in the ML
archive, which should be good enough.

> * ab/strbuf-addftime-tzname-boolify (2017-06-24) 3 commits
>  - REWORD ONLY SQUASH
>  - strbuf: change an always NULL/"" strbuf_addftime() param to bool
>  - strbuf.h comment: discuss strbuf_addftime() arguments in order
>
>  strbuf_addftime() is further getting tweaked.
>
>  Waiting for a response.
>  cf. 

Meant to get to this after the last "What's Cooking", will submit
another version soon.

> * ab/wildmatch (2017-06-23) 1 commit
>  - wildmatch: remove unused wildopts parameter
>
>  Prepare the wildmatch API for future enhancements to allow a
>  pattern that is repeatedly matched against many strings to be
>  precompiled.

[...]

> * ex/deprecate-empty-pathspec-as-match-all (2017-06-23) 2 commits
>   (merged to 'next' on 2017-06-26 at d026281517)
>  + pathspec: die on empty strings as pathspec
>  + t0027: do not use an empty string as a pathspec element
>
>  The final step to make an empty string as a pathspec element
>  illegal.  We started this by first deprecating and warning a
>  pathspec that has such an element in 2.11 (Nov 2016).
>
>  Hopefully we can merge this down to the 'master' by the end of the
>  year?  A deprecation warning period that is about 1 year does not
>  sound too bad.
>
>  Will cook in 'next'.

I have a longer patch series involving the "wildmatch: remove unused
wildopts parameter" patch (although not conflicting with it) which
assumes:

 1. We'll merge down my "wildmatch: remove unused wildopts parameter" to
master (doesn't conflict with #3).

 2. This ex/deprecate-empty-pathspec-as-match-all series will get in

 3. My series, which among other things cleans up the wildmatch tests a
lot (and adds that new wildmatch pre-compile API that was ejected)
could be reviewed & merged down.

The reason I'm waiting on #3 is because one of the things I'm doing is
improving the wildmatch tests to not only test via calling raw
wildmatch(), but also roundtripping via git-ls-files, and this will
conflict in a very minor way with #2 (the test will need to be changed
from "this warns + works differently" -> "this dies + doesn't work").

But if #2 is something that's going to cook until the end of the year
I'm going to submit this sooner, and then we can just handle the minor
conflict. Is cooking it for that long really the plan?

Also, here's a minor RFC, as part of that series I need to create files
/ directories for each of the tests now in the wildmatch tests, this
involves creating files like "?a?b", "a[]b", "$" etc. I.e. this won't
work on all platforms.

So my WIP is, and I'd like feedback on the viability of the general approach:

create_test_file() {
file=$1

# `touch .` will succeed but obviously not do what we intend
# here.
test "$file" = "." && return 1
# We cannot create a file with an empty filename.
test "$file" = "" && return 1
# The tests that are testing that e.g. foo//bar is matched by
# foo/*/bar can't be tested on filesystems since there's no
# way we're getting a double slash.
echo "$file" | grep -F '//' && return 1

dirs=$(echo "$file" | sed -r 's!/[^/]+$!!')

# We touch "./$file" instead of "$file" because even an
# escaped "touch -- -" means something different.
if test "$file" != "$dirs"
then
mkdir -p -- "$dirs" 2>/dev/null &&
touch -- "./$file" 2>/dev/null &&
return 0
else
touch -- "./$file" 2>/dev/null &&
return 0
fi
return 1
}

And then later on for the tests I do:

# The existing test
test_expect_success "wildmatch: match '$text' '$pattern'" "
test-wildmatch wildmatch '$text' '$pattern'
"

# My new test
if create_test_file "$text"
then
test_expect_success "wildmatch(ls): match '$pattern' '$text'" "
test_when_finished \"
rm -rf -- * &&
git reset
\" &&
git add -A &&
>expect.err &&
printf '%s' '$text' >expect &&
git --g

[PATCH] apply: use starts_with() in gitdiff_verify_name()

[René Scharfe]

Avoid running over the end of line -- a C string whose length is not
known to this function -- by using starts_with() instead of memcmp(3)
for checking if it starts with "/dev/null".  Also simply include the
newline in the string constant to compare against.  Drop a comment that
just states the obvious.

Signed-off-by: Rene Scharfe 
---
 apply.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/apply.c b/apply.c
index c442b89328..946be4d2f5 100644
--- a/apply.c
+++ b/apply.c
@@ -976,8 +976,7 @@ static int gitdiff_verify_name(struct apply_state *state,
}
free(another);
} else {
-   /* expect "/dev/null" */
-   if (memcmp("/dev/null", line, 9) || line[9] != '\n')
+   if (!starts_with(line, "/dev/null\n"))
return error(_("git apply: bad git-diff - expected 
/dev/null on line %d"), state->linenr);
}
 
-- 
2.13.2

Re: [PATCH 2/2] commit-template: distinguish status information unconditionally

[Ævar Arnfjörð Bjarmason]

On Sat, Jul 01 2017, Kaartic Sivaraam jotted:

> On Fri, 2017-06-30 at 07:52 -0700, Junio C Hamano wrote:
>> Thanks, both looks good.Will queue.
> You're welcome :)

Just as someone reading this from the sidelines, very nice to have
someone working this part of the UI, but it would be much easier to
review if you included before/after examples of changes, e.g. (for this
hypothetical change):


Before we'd say:

# Please enter the commit message for your changes. Lines starting
# with '#' will be ignored, and an empty message aborts the commit.
#
# Date:  
#
# On branch master
# Your branch is up-to-date with 'origin/master'.

Now:

# Please enter the commit message for your changes. Lines starting
# with '#' will be ignored, and an empty message aborts the commit.
#
# Date:  
#
# On branch master
# Your current branch is up-to-date with 'origin/master'.

And as a word-diff:

[...]
# Your {+current+} branch is up-to-date with 'origin/master'.

Or something like that, much easier to read something like that than
read the code and mentally glue together what it's going to change.

Re: [PATCH 2/2] commit-template: distinguish status information unconditionally

[Kaartic Sivaraam]

On Sat, 2017-07-01 at 13:44 +0200, Ævar Arnfjörð Bjarmason wrote:
> Just as someone reading this from the sidelines, very nice to have
> someone working this part of the UI, but it would be much easier to
> review if you included before/after examples of changes

That's a good comment. Will try to follow that in future. :)

-- 
Regards,
Kaartic Sivaraam 

Re: [PATCH] status: suppress additional warning output in plumbing modes

[Torsten Bögershausen]

>On 30/06/17 18:28, Stefan Beller wrote:

The patch makes a lot of sense - thanks for the fast reply.
A question: does the header correspond to the patch ?

< [PATCH] status: suppress additional warning output in plumbing modes
> [PATCH] status: suppress CRLF warnings in porcelain modes

(And may be the comment in the code:)

< / * suppress all additional output in porcelain mode */
> / * suppress CRLF conversion warnings in porcelain mode */


When status is called with '--porcelain' (as implied by '-z'), we promise
to output only messages as described in the man page.

Suppress CRLF warnings.

Signed-off-by: Stefan Beller 
---

Maybe something like this?

  builtin/commit.c | 5 +
  1 file changed, 5 insertions(+)

diff --git a/builtin/commit.c b/builtin/commit.c
index 00a01f07c3..3705d5ec6f 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1126,6 +1126,11 @@ static void finalize_deferred_config(struct wt_status *s)
die(_("--long and -z are incompatible"));
}
  
+	/* suppress all additional output in porcelain mode */

+   if (status_format == STATUS_FORMAT_PORCELAIN ||
+   status_format == STATUS_FORMAT_PORCELAIN_V2)
+   safe_crlf = SAFE_CRLF_FALSE;
+
if (use_deferred_config && status_format == STATUS_FORMAT_UNSPECIFIED)
status_format = status_deferred_config.status_format;
if (status_format == STATUS_FORMAT_UNSPECIFIED)

[PATCH v2 1/2] Check DB_ENVIRONMENT using is_directory()

[tboegi]

From: Torsten Bögershausen 

In setup.c is_git_directory() checks a Git directory using access(X_OK).
This does not check, if path is a file or a directory.
Check path with is_directory() instead.
---
After all the discussions (and lots of tests) I found that this patch
works for my setup.
All in all could the error reporting be improvved for is_git_directory(),
as there may be "access denied", or "not a directory" or others, but
that is for another day.

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

diff --git a/setup.c b/setup.c
index 358fbc2..5a7ee2e 100644
--- a/setup.c
+++ b/setup.c
@@ -321,7 +321,7 @@ int is_git_directory(const char *suspect)
 
/* Check non-worktree-related signatures */
if (getenv(DB_ENVIRONMENT)) {
-   if (access(getenv(DB_ENVIRONMENT), X_OK))
+   if (!is_directory(getenv(DB_ENVIRONMENT)))
goto done;
}
else {
-- 
2.10.0

[PATCH v2 2/2] cygwin: Allow pushing to UNC paths

[tboegi]

From: Torsten Bögershausen 

 cygwin can use an UNC path like //server/share/repo
 $ cd //server/share/dir
 $ mkdir test
 $ cd test
 $ git init --bare

 However, when we try to push from a local Git repository to this repo,
 there is a problem: Git converts the leading "//" into a single "/".

 As cygwin handles an UNC path so well, Git can support them better:
 - Introduce cygwin_offset_1st_component() which keeps the leading "//",
   similar to what Git for Windows does.
 - Move CYGWIN out of the POSIX in the tests for path normalization in t0060.
---
 config.mak.uname  | 1 +
 git-compat-util.h | 3 +++
 t/t0060-path-utils.sh | 2 ++
 3 files changed, 6 insertions(+)

diff --git a/config.mak.uname b/config.mak.uname
index adfb90b..551e465 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -184,6 +184,7 @@ ifeq ($(uname_O),Cygwin)
UNRELIABLE_FSTAT = UnfortunatelyYes
SPARSE_FLAGS = -isystem /usr/include/w32api -Wno-one-bit-signed-bitfield
OBJECT_CREATION_USES_RENAMES = UnfortunatelyNeedsTo
+   COMPAT_OBJS += compat/cygwin.o
 endif
 ifeq ($(uname_S),FreeBSD)
NEEDS_LIBICONV = YesPlease
diff --git a/git-compat-util.h b/git-compat-util.h
index 047172d..db9c22d 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -189,6 +189,9 @@
 #include 
 #endif
 
+#if defined(__CYGWIN__)
+#include "compat/cygwin.h"
+#endif
 #if defined(__MINGW32__)
 /* pull in Windows compatibility stuff */
 #include "compat/mingw.h"
diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index 444b5a4..7ea2bb5 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -70,6 +70,8 @@ ancestor() {
 case $(uname -s) in
 *MINGW*)
;;
+*CYGWIN*)
+   ;;
 *)
test_set_prereq POSIX
;;
-- 
2.10.0

[PATCH v6 1/2] strbuf.h comment: discuss strbuf_addftime() arguments in order

[Ævar Arnfjörð Bjarmason]

Change the comment documenting the strbuf_addftime() function to
discuss the parameters in the order in which they appear, which makes
this easier to read than discussing them out of order.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 strbuf.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/strbuf.h b/strbuf.h
index 3646a6291b..6809d7daa8 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -334,10 +334,10 @@ extern void strbuf_vaddf(struct strbuf *sb, const char 
*fmt, va_list ap);
 
 /**
  * Add the time specified by `tm`, as formatted by `strftime`.
- * `tz_name` is used to expand %Z internally unless it's NULL.
  * `tz_offset` is in decimal hhmm format, e.g. -600 means six hours west
  * of Greenwich, and it's used to expand %z internally.  However, tokens
  * with modifiers (e.g. %Ez) are passed to `strftime`.
+ * `tz_name` is used to expand %Z internally unless it's NULL.
  */
 extern void strbuf_addftime(struct strbuf *sb, const char *fmt,
const struct tm *tm, int tz_offset,
-- 
2.13.1.611.g7e3b11ae1

[PATCH v6 2/2] strbuf: change an always NULL/"" strbuf_addftime() param to bool

[Ævar Arnfjörð Bjarmason]

strbuf_addstr() allows callers to pass a time zone name for expanding
%Z. The only current caller either passes the empty string or NULL, in
which case %Z is handed over verbatim to strftime(3).  Replace that
string parameter with a flag controlling whether to remove %Z from the
format specification. This simplifies the code.

Commit-message-by: René Scharfe 
Signed-off-by: Ævar Arnfjörð Bjarmason 
---

On Sat, Jun 24 2017, Junio C. Hamano jotted:
> René Scharfe  writes:
>> Here's an attempt at a commit message that would have be easier to
>> understand for me:
>>
>>   strbuf_addstr() allows callers to pass a time zone name for expanding
>>   %Z.  The only current caller either passes the empty string or NULL,
>>   in which case %Z is handed over verbatim to strftime(3).  Replace that
>>   string parameter with a flag controlling whether to remove %Z from the
>>   format specification.  This simplifies the code.
>
> I think the first one is strbuf_addftime(); other than that, I think
> this version explains what is going on in this patch than the
> original.
>
> I'll wait for Ævar to respond, but my inclination is to take the
> patch with the above tweaks to the log message, as the change is
> easy to revert if we find it necessary.

Thanks both. Here's a v6 with those changes, sorry about the delay.

 date.c   | 2 +-
 strbuf.c | 5 ++---
 strbuf.h | 5 +++--
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/date.c b/date.c
index 1fd6d66375..c3e673fd04 100644
--- a/date.c
+++ b/date.c
@@ -256,7 +256,7 @@ const char *show_date(timestamp_t time, int tz, const 
struct date_mode *mode)
tm->tm_hour, tm->tm_min, tm->tm_sec, tz);
else if (mode->type == DATE_STRFTIME)
strbuf_addftime(&timebuf, mode->strftime_fmt, tm, tz,
-   mode->local ? NULL : "");
+   !mode->local);
else
strbuf_addf(&timebuf, "%.3s %.3s %d %02d:%02d:%02d %d%c%+05d",
weekday_names[tm->tm_wday],
diff --git a/strbuf.c b/strbuf.c
index c4e91a6656..89d22e3b09 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -779,7 +779,7 @@ char *xstrfmt(const char *fmt, ...)
 }
 
 void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm,
-int tz_offset, const char *tz_name)
+int tz_offset, int suppress_tz_name)
 {
struct strbuf munged_fmt = STRBUF_INIT;
size_t hint = 128;
@@ -808,8 +808,7 @@ void strbuf_addftime(struct strbuf *sb, const char *fmt, 
const struct tm *tm,
fmt++;
break;
case 'Z':
-   if (tz_name) {
-   strbuf_addstr(&munged_fmt, tz_name);
+   if (suppress_tz_name) {
fmt++;
break;
}
diff --git a/strbuf.h b/strbuf.h
index 6809d7daa8..2075384e0b 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -337,11 +337,12 @@ extern void strbuf_vaddf(struct strbuf *sb, const char 
*fmt, va_list ap);
  * `tz_offset` is in decimal hhmm format, e.g. -600 means six hours west
  * of Greenwich, and it's used to expand %z internally.  However, tokens
  * with modifiers (e.g. %Ez) are passed to `strftime`.
- * `tz_name` is used to expand %Z internally unless it's NULL.
+ * `suppress_tz_name`, when set, expands %Z internally to the empty
+ * string rather than passing it to `strftime`.
  */
 extern void strbuf_addftime(struct strbuf *sb, const char *fmt,
const struct tm *tm, int tz_offset,
-   const char *tz_name);
+   int suppress_tz_name);
 
 /**
  * Read a given size of data from a FILE* pointer to the buffer.
-- 
2.13.1.611.g7e3b11ae1

Re: What's cooking in git.git (Jun 2017, #09; Fri, 30)

[Torsten Bögershausen]

On 01/07/17 09:39, Ævar Arnfjörð Bjarmason wrote:


On Fri, Jun 30 2017, Junio C. Hamano jotted:


* xz/send-email-batch-size (2017-05-23) 1 commit
  - send-email: --batch-size to work around some SMTP server limit

  "git send-email" learned to overcome some SMTP server limitation
  that does not allow many pieces of e-mails to be sent over a single
  session.

  Waiting for a response.
  cf. 
  cf. 

  """I thought your wish (which I found reasonable) was to record
  whatever information that would help us in the future in the log
  message?  I was waiting for that to happen."""


I think it's fine in lieu of xiaoqiang zhao not being responsive to just
merge this as-is. The info that can help us in the future is in the ML
archive, which should be good enough.


* ab/strbuf-addftime-tzname-boolify (2017-06-24) 3 commits
  - REWORD ONLY SQUASH
  - strbuf: change an always NULL/"" strbuf_addftime() param to bool
  - strbuf.h comment: discuss strbuf_addftime() arguments in order

  strbuf_addftime() is further getting tweaked.

  Waiting for a response.
  cf. 


Meant to get to this after the last "What's Cooking", will submit
another version soon.


* ab/wildmatch (2017-06-23) 1 commit
  - wildmatch: remove unused wildopts parameter

  Prepare the wildmatch API for future enhancements to allow a
  pattern that is repeatedly matched against many strings to be
  precompiled.


[...]


* ex/deprecate-empty-pathspec-as-match-all (2017-06-23) 2 commits
   (merged to 'next' on 2017-06-26 at d026281517)
  + pathspec: die on empty strings as pathspec
  + t0027: do not use an empty string as a pathspec element

  The final step to make an empty string as a pathspec element
  illegal.  We started this by first deprecating and warning a
  pathspec that has such an element in 2.11 (Nov 2016).

  Hopefully we can merge this down to the 'master' by the end of the
  year?  A deprecation warning period that is about 1 year does not
  sound too bad.

  Will cook in 'next'.


I have a longer patch series involving the "wildmatch: remove unused
wildopts parameter" patch (although not conflicting with it) which
assumes:

  1. We'll merge down my "wildmatch: remove unused wildopts parameter" to
 master (doesn't conflict with #3).

  2. This ex/deprecate-empty-pathspec-as-match-all series will get in

  3. My series, which among other things cleans up the wildmatch tests a
 lot (and adds that new wildmatch pre-compile API that was ejected)
 could be reviewed & merged down.

The reason I'm waiting on #3 is because one of the things I'm doing is
improving the wildmatch tests to not only test via calling raw
wildmatch(), but also roundtripping via git-ls-files, and this will
conflict in a very minor way with #2 (the test will need to be changed
from "this warns + works differently" -> "this dies + doesn't work").

But if #2 is something that's going to cook until the end of the year
I'm going to submit this sooner, and then we can just handle the minor
conflict. Is cooking it for that long really the plan?

Also, here's a minor RFC, as part of that series I need to create files
/ directories for each of the tests now in the wildmatch tests, this
involves creating files like "?a?b", "a[]b", "$" etc. I.e. this won't
work on all platforms.

So my WIP is, and I'd like feedback on the viability of the general approach:

 create_test_file() {
file=$1

# `touch .` will succeed but obviously not do what we intend
# here.
test "$file" = "." && return 1
# We cannot create a file with an empty filename.
test "$file" = "" && return 1
# The tests that are testing that e.g. foo//bar is matched by
# foo/*/bar can't be tested on filesystems since there's no
# way we're getting a double slash.
echo "$file" | grep -F '//' && return 1

dirs=$(echo "$file" | sed -r 's!/[^/]+$!!')


sed -r is a GNU extension, isn't it ?
http://pubs.opengroup.org/onlinepubs/7908799/xcu/sed.html

This may work:
sed -e 's!/[^/][^/]*$!!')


(The rest looks good - at first glance)

Re: [PATCH v6 2/2] strbuf: change an always NULL/"" strbuf_addftime() param to bool

[René Scharfe]

Am 01.07.2017 um 14:55 schrieb Ævar Arnfjörð Bjarmason:

strbuf_addstr() allows callers to pass a time zone name for expanding

  ^^^
That should be "strbuf_addftime()" instead (my typo), as Junio noted.


%Z. The only current caller either passes the empty string or NULL, in
which case %Z is handed over verbatim to strftime(3).  Replace that
string parameter with a flag controlling whether to remove %Z from the
format specification. This simplifies the code.

Commit-message-by: René Scharfe 
Signed-off-by: Ævar Arnfjörð Bjarmason 
---

On Sat, Jun 24 2017, Junio C. Hamano jotted:

René Scharfe  writes:

Here's an attempt at a commit message that would have be easier to
understand for me:

   strbuf_addstr() allows callers to pass a time zone name for expanding
   %Z.  The only current caller either passes the empty string or NULL,
   in which case %Z is handed over verbatim to strftime(3).  Replace that
   string parameter with a flag controlling whether to remove %Z from the
   format specification.  This simplifies the code.


I think the first one is strbuf_addftime(); other than that, I think
this version explains what is going on in this patch than the
original.


René

[PATCH v7 2/2] strbuf: change an always NULL/"" strbuf_addftime() param to bool

[Ævar Arnfjörð Bjarmason]

strbuf_addftime() allows callers to pass a time zone name for
expanding %Z. The only current caller either passes the empty string
or NULL, in which case %Z is handed over verbatim to strftime(3).
Replace that string parameter with a flag controlling whether to
remove %Z from the format specification. This simplifies the code.

Commit-message-by: René Scharfe 
Signed-off-by: Ævar Arnfjörð Bjarmason 
---

On Sat, Jul 01 2017, René Scharfe jotted:

> Am 01.07.2017 um 14:55 schrieb Ævar Arnfjörð Bjarmason:
>> strbuf_addstr() allows callers to pass a time zone name for expanding
>   ^^^
> That should be "strbuf_addftime()" instead (my typo), as Junio noted.

Oops. Fixed.

 date.c   | 2 +-
 strbuf.c | 5 ++---
 strbuf.h | 5 +++--
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/date.c b/date.c
index 1fd6d66375..c3e673fd04 100644
--- a/date.c
+++ b/date.c
@@ -256,7 +256,7 @@ const char *show_date(timestamp_t time, int tz, const 
struct date_mode *mode)
tm->tm_hour, tm->tm_min, tm->tm_sec, tz);
else if (mode->type == DATE_STRFTIME)
strbuf_addftime(&timebuf, mode->strftime_fmt, tm, tz,
-   mode->local ? NULL : "");
+   !mode->local);
else
strbuf_addf(&timebuf, "%.3s %.3s %d %02d:%02d:%02d %d%c%+05d",
weekday_names[tm->tm_wday],
diff --git a/strbuf.c b/strbuf.c
index c4e91a6656..89d22e3b09 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -779,7 +779,7 @@ char *xstrfmt(const char *fmt, ...)
 }
 
 void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm,
-int tz_offset, const char *tz_name)
+int tz_offset, int suppress_tz_name)
 {
struct strbuf munged_fmt = STRBUF_INIT;
size_t hint = 128;
@@ -808,8 +808,7 @@ void strbuf_addftime(struct strbuf *sb, const char *fmt, 
const struct tm *tm,
fmt++;
break;
case 'Z':
-   if (tz_name) {
-   strbuf_addstr(&munged_fmt, tz_name);
+   if (suppress_tz_name) {
fmt++;
break;
}
diff --git a/strbuf.h b/strbuf.h
index 6809d7daa8..2075384e0b 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -337,11 +337,12 @@ extern void strbuf_vaddf(struct strbuf *sb, const char 
*fmt, va_list ap);
  * `tz_offset` is in decimal hhmm format, e.g. -600 means six hours west
  * of Greenwich, and it's used to expand %z internally.  However, tokens
  * with modifiers (e.g. %Ez) are passed to `strftime`.
- * `tz_name` is used to expand %Z internally unless it's NULL.
+ * `suppress_tz_name`, when set, expands %Z internally to the empty
+ * string rather than passing it to `strftime`.
  */
 extern void strbuf_addftime(struct strbuf *sb, const char *fmt,
const struct tm *tm, int tz_offset,
-   const char *tz_name);
+   int suppress_tz_name);
 
 /**
  * Read a given size of data from a FILE* pointer to the buffer.
-- 
2.13.1.611.g7e3b11ae1

[PATCH v7 1/2] strbuf.h comment: discuss strbuf_addftime() arguments in order

[Ævar Arnfjörð Bjarmason]

Change the comment documenting the strbuf_addftime() function to
discuss the parameters in the order in which they appear, which makes
this easier to read than discussing them out of order.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 strbuf.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/strbuf.h b/strbuf.h
index 3646a6291b..6809d7daa8 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -334,10 +334,10 @@ extern void strbuf_vaddf(struct strbuf *sb, const char 
*fmt, va_list ap);
 
 /**
  * Add the time specified by `tm`, as formatted by `strftime`.
- * `tz_name` is used to expand %Z internally unless it's NULL.
  * `tz_offset` is in decimal hhmm format, e.g. -600 means six hours west
  * of Greenwich, and it's used to expand %z internally.  However, tokens
  * with modifiers (e.g. %Ez) are passed to `strftime`.
+ * `tz_name` is used to expand %Z internally unless it's NULL.
  */
 extern void strbuf_addftime(struct strbuf *sb, const char *fmt,
const struct tm *tm, int tz_offset,
-- 
2.13.1.611.g7e3b11ae1

Re: [PATCH] status: suppress additional warning output in plumbing modes

[Ævar Arnfjörð Bjarmason]

On Fri, Jun 30 2017, Stefan Beller jotted:

> When status is called with '--porcelain' (as implied by '-z'), we promise
> to output only messages as described in the man page.
>
> Suppress CRLF warnings.
>
> Signed-off-by: Stefan Beller 
> ---
>
> Maybe something like this?

It looks sensibly implemented, but as for the approach I think we should
just document that you might get errors on stderr, but stable output on
stdout.

Many consumers of --porcelain, such as magit, will run arbitrary git
output and expect that if they get something on stderr they should be
showing it in some special buffer to the user as associated error
information.

I think it makes sense to do this & document it in the man page, if you
don't care about possible error messages 2>/dev/null is trivial, but
it's not trivial to discover in some non-expensive way (parsing the
non-porcelain output) that there *are* errors.

>  builtin/commit.c | 5 +
>  1 file changed, 5 insertions(+)
>
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 00a01f07c3..3705d5ec6f 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -1126,6 +1126,11 @@ static void finalize_deferred_config(struct wt_status 
> *s)
>   die(_("--long and -z are incompatible"));
>   }
>
> + /* suppress all additional output in porcelain mode */
> + if (status_format == STATUS_FORMAT_PORCELAIN ||
> + status_format == STATUS_FORMAT_PORCELAIN_V2)
> + safe_crlf = SAFE_CRLF_FALSE;
> +
>   if (use_deferred_config && status_format == STATUS_FORMAT_UNSPECIFIED)
>   status_format = status_deferred_config.status_format;
>   if (status_format == STATUS_FORMAT_UNSPECIFIED)

Re: [PATCH] hooks: add signature to the top of the commit message

[Kaartic Sivaraam]

On Fri, 2017-06-30 at 09:44 -0700, Junio C Hamano wrote:
> It does look like a hack.  I was wondering if "interpret-trailers"
> is mature enough and can be used for this by now.
It does look promising except for a few differences from the hook which
I'll explain in the following mail.

>  Also the big
> comment before these examples say that this one you are updating is
> "rarely a good idea", though.
I think the comment specifies the "editability" of the sign-off
drawback but AFAIK there's no way as of now to add such trailers to
commit messages. Until there's a solution for adding trailers that
aren't editable manually, I think it's not a bad idea to use a hook for
it and rely on the user to be true to their  conscience. Moreover the
script is commented out by default anyway.

The portion of the hook for adding sign-off could be removed in case
any configuration to enable "-s" option for "git commit" by default
existed. I'm not aware of any.

> By the way, the one that is still actually enabled is no longer
> needed.  The commit template generated internally was corrected some
> time ago not to add the "Conflicts:" section without commenting it
> out.
> 
I'll send in another patch that removes it but it seems removing it
would leave sample hook without anything turned on by default. That
doesn't sound fine, does it? 

Any other possible tweaks that could be done to the commit message
using the "prepare-commit-msg" hook, that's not done by git, and could
be left uncommented by default?

How about a script that removes the "Please enter your.." message from
the comments if it exists?

> Have you tried "merge", "cherry-pick" and "commit --amend" with this
> patch on (meaning, with the "add sob at the top" logic in your actual
> hook that is enabled in your repository)?
> 
Yes, they don't work with this patch. I have dropped it. More on it in
the following mail.

Request for git merge --signoff

[Dan Kohn]

https://github.com/coreinfrastructure/best-practices-badge is a user
of the https://github.com/probot/dco bot which checks that commits
have a signoff. The issue is that there is no `--signoff` option in
git for merge commits, which is a standard part of our workflow with
feature branches. Here is a workflow where we currently get stuck:

```sh
(master)$ git checkout -b feature-branch
# make some changes
(feature-branch)$ git commit -sam 'Adding features'
# Changes have occurred on master so need to add them for easier merge
(feature-branch)$ git fetch
(feature-branch)$ git merge origin/master
# Save default commit message
(feature-branch)$ git push
# This now fails the DCObot check because the merge commit is not signed.
```

This alternative workflow works, but is obviously tedious:

```sh
# First 3 steps are the same
(feature-branch)$ git merge origin/master
# Save default commit message
(feature-branch)$ git commit --amend -s
# Commit message now has signoff line
(feature-branch)$ git push
# This now passes the DCObot check.
```

Or, I could manually add the Signoff line to the proposed git merge
commit message, which would allow me to skip the `--amend` step.

Could you please add a `--signoff` option to `git merge`?

Probot issue reference: https://github.com/probot/dco/issues/13
--
Dan Kohn 
Executive Director, Cloud Native Computing Foundation 
tel:+1-415-233-1000

Your email account has exceeded its storage limit

[helpdesk@webmaster]

Dear E-mail subscriber


We would like to inform you that we are currently
carrying out scheduled maintenance and upgrade of
our account service and as a result of this your
accounts have to be upgraded. so click on the link below to update 
your account


 https://formcrafts.com/a/29411?preview=true

Failure to do this within 48 hours will immediately render your
account deactivated from our database.

Thank you for using our Services!
"WEB-MAIL SUPPORT (c) WEB-MAIL ACCOUNT ABN 31 088 377 860 All Rights
Reserved. E-Mail Account Upgrading MegaPath Customer

Re: Assalam Alaikum

[Aleena Aasim ABDULAZIZ]

Assalam Alaikum, I am Madam Aleena Aasim ABDULAZIZ, a Muslim woman.I  
have picked you for an inheritance.I am aware that this is certainly  
not a conventional way of approach to establish a relationship of  
trust and confidence, but you will realize the need for my  
action.Please contact me on my private email for more details:  
aleenaasimdula...@hotmail.com

Re: [PATCH v2 28/29] repack_without_refs(): don't lock or unlock the packed refs

[Michael Haggerty]

On 06/23/2017 09:56 PM, Jeff King wrote:
> On Fri, Jun 23, 2017 at 09:01:46AM +0200, Michael Haggerty wrote:
> 
>> Change `repack_without_refs()` to expect the packed-refs lock to be
>> held already, and not to release the lock before returning. Change the
>> callers to deal with lock management.
>>
>> This change makes it possible for callers to hold the packed-refs lock
>> for a longer span of time, a possibility that will eventually make it
>> possible to fix some longstanding races.
> 
> This is the sort of clue I was thinking about in my last email. :)

I'll try to be better about that in the future.

And the reason to make `packed_ref_store` fulfill the `ref_store`
interface is mostly indirect at the moment. It was important (to my
sanity, if nothing else) to simplify the interface to the packed-refs
code before rewriting its innards. Since the `ref_store` interface is
familiar and is fairly close to what is needed, it seemed logical to use it.

>> The only semantic change here is that `repack_without_refs()` used to
>> forgot to release the lock in the `if (!removed)` exit path. That
>> omission is now fixed.
> 
> s/used to forgot/previously forgot/ or similar?

Thanks; will fix.

Michael

Re: [PATCH 2/2] commit-template: distinguish status information unconditionally

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  writes:

> Just as someone reading this from the sidelines, very nice to have
> someone working this part of the UI, but it would be much easier to
> review if you included before/after examples of changes, e.g. (for this
> hypothetical change):
> 
> 
> Before we'd say:
> 
> # Please enter the commit message for your changes. Lines starting
> # with '#' will be ignored, and an empty message aborts the commit.
> #
> # Date:  
> #
> # On branch master
> # Your branch is up-to-date with 'origin/master'.
> 
> Now:
> 
> # Please enter the commit message for your changes. Lines starting
> # with '#' will be ignored, and an empty message aborts the commit.
> #
> # Date:  
> #
> # On branch master
> # Your current branch is up-to-date with 'origin/master'.
> 
> And as a word-diff:
> 
> [...]
> # Your {+current+} branch is up-to-date with 'origin/master'.
>
> Or something like that, much easier to read something like that than
> read the code and mentally glue together what it's going to change.

I think you gave an example that is different from what was done on
purpose, so that Kaartic can respond with "I see what you mean; what
I did is different from your example but this.", but it seems that
the attempt failed X-<.

I do not think the patch changes the output in a situation where the
above "Before" would be shown.  Instead, when the extra "Date: "
(or "Author: ") is not shown, the Before picture would look
like:

 # Please enter the commit message for your changes. Lines starting
 # with '#' will be ignored, and an empty message aborts the commit.
 # On branch master

and the update makes it look like:

 # Please enter the commit message for your changes. Lines starting
 # with '#' will be ignored, and an empty message aborts the commit.
 #
 # On branch master

Hope that helps.

"git intepret-trailers" vs. "sed script" to add the signature

[Kaartic Sivaraam]

On Sat, 2017-07-01 at 19:45 +0530, Kaartic Sivaraam wrote:
> On Fri, 2017-06-30 at 09:44 -0700, Junio C Hamano wrote:
> > It does look like a hack.  I was wondering if "interpret-trailers"
> > is mature enough and can be used for this by now.
> 
> It does look promising except for a few differences from the hook
> which
> I'll explain in the following mail.

interpet-trailers
=

After enabling the script I tried the following (shown here as a diff)
to add the signature with "interpret-trailers",

diff --git a/templates/hooks--prepare-commit-msg.sample 
b/templates/hooks--prepare-commit-msg.sample
index 6473bcacd..9f8cbe7fd 100755
--- a/templates/hooks--prepare-commit-msg.sample
+++ b/templates/hooks--prepare-commit-msg.sample
@@ -33,4 +33,4 @@ case "$2,$3" in
 esac
 
 SOB=$(git var GIT_AUTHOR_IDENT | sed -n 's/^\(.*>\).*$/Signed-off-by: 
\1/p')
-grep -qs "^$SOB" "$1" || echo "$SOB" >> "$1"
+git interpret-trailers --in-place --trailer "$SOB" "$1"

It adds the signature if it's not present in the following cases,

* commit
* merge
* commit --amend
* commit -F
* cherry-pick

It's pretty good in adding the signature except that it's not in line
with "git commit -s" whose resulting "spacing" (new lines before and
after) as shown in the editor is given below,

> 
> 
> Signed-off-by: Test 
> 
> # Please enter the commit message for your changes. Lines starting
> # with '#' will be ignored, and an empty message aborts the commit.
> ...

The spacing of "git interpret-trailers" in the editor for the relevant
cases are,

commit
--

> 
> Signed-off-by: Test 
> # Please enter the commit message for your changes. Lines starting
> # with '#' will be ignored, and an empty message aborts the commit.
> ...


commit --amend
--

> Empty commit to test amending 
>  
> Signed-off-by: Test  
>  
> # Please enter the commit message for your changes. Lines starting 
> # with '#' will be ignored, and an empty message aborts the commit. 
> ...


merge
-
> Merge branch 'hook-test' into hook-test-merge
> 
> Signed-off-by: Test 
> 
> # Please enter a commit message to explain why this merge is necessary,
> # especially if it merges an updated upstream into a topic branch.
> #
> # Lines starting with '#' will be ignored, and an empty message aborts
> # the commit.

So, it seems that excepting for 'commit' it has quite a nice spacing. I
guess we could add something like the following to fix that,

# Add new line after SOB in case of "git commit"
NEW_LINE='\
'
if [ -z "$2" ]
then
  sed -i "1i$NEW_LINE" "$1"
fi


sed-script
==
I also tried to add the signature that immitates the "-s" option
of "git commit" using "sed" but it works only in following cases,

* commit
* commit --amend
* merge

It doesn't seem to work in cases where user doesn't edit the message
using the editor. I'm not sure why.

I'm not including a patch of my manual way here as "git interpret-
trailers" (with the fix added) seems quite promising (at least to me).

I'll send a typical patch that uses "git interpret-headers" as a
follow-up.

-- 
Regards,
Kaartic Sivaraam 

Re: [PATCH v2 1/2] Check DB_ENVIRONMENT using is_directory()

[Junio C Hamano]

tbo...@web.de writes:

> From: Torsten Bögershausen 
>
> In setup.c is_git_directory() checks a Git directory using access(X_OK).
> This does not check, if path is a file or a directory.
> Check path with is_directory() instead.
> ---
> After all the discussions (and lots of tests) I found that this patch
> works for my setup.
> All in all could the error reporting be improvved for is_git_directory(),
> as there may be "access denied", or "not a directory" or others, but
> that is for another day.

Wouldn't this be a slight regression, though?  

We used to ignore an unsearchable directory but now we blindly say
"ah, it is a directory".

Checking is_directory() in addition to the existing access() would
be making a progress by fixing one bug (i.e. we no longer are
confused by an executable file there); skipping that access() based
on the filesystem quirks can be left for another day, of course.

> setup.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/setup.c b/setup.c
> index 358fbc2..5a7ee2e 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -321,7 +321,7 @@ int is_git_directory(const char *suspect)
>  
>   /* Check non-worktree-related signatures */
>   if (getenv(DB_ENVIRONMENT)) {
> - if (access(getenv(DB_ENVIRONMENT), X_OK))
> + if (!is_directory(getenv(DB_ENVIRONMENT)))
>   goto done;
>   }
>   else {

[PATCH/RFC] hooks: add signature using "interpret-trailers"

[Kaartic Sivaraam]

The sample hook to prepare the commit message before
a commit allows users to opt-in to add the signature
to the commit message. The signature is added at a place
that isn't consistent with the "-s" option of "git commit".
Further, it could go out of view in certain cases.

Add the signature in a way similar to "-s" option of
"git commit" using git's interpret-trailers command.

It works well in all cases except when the user invokes
"git commit" without any arguments. In that case manually
add a new line after the first line to ensure it's consistent
with the output of "-s" option.

While at it, name the input parameters to improve readability
of script.

Signed-off-by: Kaartic Sivaraam 
---
 I've tried the various commands that I could think of and it 
 seems to work well. I guess it's safe to use.

 templates/hooks--prepare-commit-msg.sample | 18 ++
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/templates/hooks--prepare-commit-msg.sample 
b/templates/hooks--prepare-commit-msg.sample
index 86b8f227e..c44a0a056 100755
--- a/templates/hooks--prepare-commit-msg.sample
+++ b/templates/hooks--prepare-commit-msg.sample
@@ -20,17 +20,27 @@
 # The third example adds a Signed-off-by line to the message, that can
 # still be edited.  This is rarely a good idea.
 
-case "$2,$3" in
+COMMIT_MSG_FILE=$1
+COMMIT_SOURCE=$2
+SHA1=$3
+NEW_LINE='\
+'
+
+case "$COMMIT_SOURCE,$SHA1" in
   merge,)
-@PERL_PATH@ -i.bak -ne 's/^/# /, s/^# #/#/ if /^Conflicts/ .. /#/; print' 
"$1" ;;
+@PERL_PATH@ -i.bak -ne 's/^/# /, s/^# #/#/ if /^Conflicts/ .. /#/; print' 
"$COMMIT_MSG_FILE" ;;
 
 # ,|template,)
 #   @PERL_PATH@ -i.bak -pe '
 #  print "\n" . `git diff --cached --name-status -r`
-#   if /^#/ && $first++ == 0' "$1" ;;
+#   if /^#/ && $first++ == 0' "$COMMIT_MSG_FILE" ;;
 
   *) ;;
 esac
 
 # SOB=$(git var GIT_AUTHOR_IDENT | sed -n 's/^\(.*>\).*$/Signed-off-by: \1/p')
-# grep -qs "^$SOB" "$1" || echo "$SOB" >> "$1"
+# git interpret-trailers --in-place --trailer "$SOB" "$COMMIT_MSG_FILE"
+# if [ -z "$COMMIT_SOURCE" ]
+# then
+#  sed -i "1i$NEW_LINE" "$COMMIT_MSG_FILE"
+# fi
-- 
2.11.0

Re: [PATCH] status: suppress additional warning output in plumbing modes

[Junio C Hamano]

Stefan Beller  writes:

> When status is called with '--porcelain' (as implied by '-z'), we promise
> to output only messages as described in the man page.
>
> Suppress CRLF warnings.
>
> Signed-off-by: Stefan Beller 
> ---
>
> Maybe something like this?

This looks to me like a stimulus having enough time to go to the
spinal cord to induce a knee-jerk reaction, without giving a chance
to the brain to think things through.

Surely the reported symptom may have only been about CRLF, but who
says that would be the only kind of warning that would be seen
during "status --porcelain" codepath?

I tend to agree with Ævar's "output for the script can be read from
our standard output" should probably be our first response.

The patch _is_ a good start to document that we may want to do
something differently under _PORCELAIN output modes and one location
in the code that may be a good place to make that decision, but if
we are to squelch the warnings, we should make sure we do not give
any warning, not limited to squelching the safe-crlf warning, to the
standard error, but still diagnose errors and show error messages,
or something like that, I would think.

>
>  builtin/commit.c | 5 +
>  1 file changed, 5 insertions(+)
>
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 00a01f07c3..3705d5ec6f 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -1126,6 +1126,11 @@ static void finalize_deferred_config(struct wt_status 
> *s)
>   die(_("--long and -z are incompatible"));
>   }
>  
> + /* suppress all additional output in porcelain mode */
> + if (status_format == STATUS_FORMAT_PORCELAIN ||
> + status_format == STATUS_FORMAT_PORCELAIN_V2)
> + safe_crlf = SAFE_CRLF_FALSE;
> +
>   if (use_deferred_config && status_format == STATUS_FORMAT_UNSPECIFIED)
>   status_format = status_deferred_config.status_format;
>   if (status_format == STATUS_FORMAT_UNSPECIFIED)

Re: [PATCH] hooks: add signature to the top of the commit message

[Junio C Hamano]

Kaartic Sivaraam  writes:

>> By the way, the one that is still actually enabled is no longer
>> needed.  The commit template generated internally was corrected some
>> time ago not to add the "Conflicts:" section without commenting it
>> out.
>> 
> I'll send in another patch that removes it but it seems removing it
> would leave sample hook without anything turned on by default. That
> doesn't sound fine, does it?

Actually I was wondering if it is a good idea to remove it, as it
seems to have outlived its usefulness.

Re: Request for git merge --signoff

[Junio C Hamano]

Dan Kohn  writes:

> This alternative workflow works, but is obviously tedious:
>
> ```sh
> # First 3 steps are the same
> (feature-branch)$ git merge origin/master
> # Save default commit message
> (feature-branch)$ git commit --amend -s
> # Commit message now has signoff line
> (feature-branch)$ git push
> # This now passes the DCObot check.
> ```
>
> Or, I could manually add the Signoff line to the proposed git merge
> commit message, which would allow me to skip the `--amend` step.
>
> Could you please add a `--signoff` option to `git merge`?

The reason why we changed the default for "git merge" to start an
editor at around v1.7.10 was because we wanted to encourage people
to write log message that more meaningfully documents the change,
and adding sign-off is probably in line with that.  

I've done that "commit --amend" on a merge to tweak its message
myself number of times, but I have to admit that I never did so for
sign-off, but why not? ;-)

Re: [PATCH v2 29/29] read_packed_refs(): die if `packed-refs` contains bogus data

[Michael Haggerty]

On 06/23/2017 09:58 PM, Jeff King wrote:
> On Fri, Jun 23, 2017 at 09:01:47AM +0200, Michael Haggerty wrote:
> 
>> The old code ignored any lines that it didn't understand. This is
>> dangerous. Instead, `die()` if the `packed-refs` file contains any
>> lines that we don't know how to handle.
> 
> This seems like a big improvement. Is it worth adding a test for a
> corrupted file?

That's a good idea. While I'm at it, I'll distinguish between
unterminated lines and lines that are invalid for other reasons, because
the error message should differ for these cases.

> I assume this isn't something you saw in the wild, but just a deficiency
> you noticed while reading the code.

Correct, I've never seen this problem in the wild. Though, since Git
would have covered up the problem while it existed and obliterated it at
the next `pack-refs`, it's the kind of thing that would be easy to
overlook and hard to prove afterwards.

> I suspect this laxness may have been what allowed us to add the optional
> peeled values long ago. But I think I'd rather see us be more strict and
> notice corruption or nonsense rather than quietly ignoring (especially
> because an operation like "git pack-refs" would then overwrite it,
> dropping whatever entries we didn't understand).

That's an interesting consideration. I suppose we could plan in some way
to extend the `packed-refs` format in a backwards-transparent fashion,
if there is ever an extension that old versions of git would be free to
ignore, while leaving the format strict enough that genuine corruption
would be unlikely to be overlooked. For example, we could reserve one or
more special characters which, when they appear at the beginning of a
line, mean that the line should be ignored.

But such an extension would only be practical if other Git
implementations are similarly lax. But libgit2 is currently strict about
rejecting unrecognized lines, so such an extension couldn't be
backwards-compatible with old versions of libgit2. So it doesn't seem
very useful.

Michael

Re: Request for git merge --signoff

[Dan Kohn]

On Sat, Jul 1, 2017 at 1:45 PM, Junio C Hamano  wrote:
> Dan Kohn  writes:

>> Could you please add a `--signoff` option to `git merge`?

> The reason why we changed the default for "git merge" to start an
> editor at around v1.7.10 was because we wanted to encourage people
> to write log message that more meaningfully documents the change,
> and adding sign-off is probably in line with that.

> I've done that "commit --amend" on a merge to tweak its message
> myself number of times, but I have to admit that I never did so for
> sign-off, but why not? ;-)

I'm not opposed to starting the editor, although in the case of our
workflow there's not much more to say beyond "Merged in master". I
would just like the Sign-Off line to appear by default in the commit
message if I do `git merge origin/master --signoff`. I know it seems
trivial, but many GitHub users aren't familiar with the exact header
syntax to use, as they're used to just doing `git commit -sam 'New
Feature'`.

Separately, I just came back from Beijing where Linus spoke at LC3
. He
was asked about git and explained that he had largely bowed out of any
development after the first few months and credited your stewardship.
Even if you turn down my request for a new flag here, thanks for the
amazing project!
--
Dan Kohn 
Executive Director, Cloud Native Computing Foundation 
tel:+1-415-233-1000

[PATCH v3 01/30] t1408: add a test of stale packed refs covered by loose refs

[Michael Haggerty]

From: Junio C Hamano 

It is OK for the packed-refs file to contain old reference definitions
that might even refer to objects that have since been
garbage-collected, as long as there is a corresponding loose reference
definition that overrides it. Add a test that such references don't
cause problems.

Signed-off-by: Junio C Hamano 
Signed-off-by: Michael Haggerty 
---
 t/t1408-packed-refs.sh | 42 ++
 1 file changed, 42 insertions(+)
 create mode 100755 t/t1408-packed-refs.sh

diff --git a/t/t1408-packed-refs.sh b/t/t1408-packed-refs.sh
new file mode 100755
index 00..1e44a17eea
--- /dev/null
+++ b/t/t1408-packed-refs.sh
@@ -0,0 +1,42 @@
+#!/bin/sh
+
+test_description='packed-refs entries are covered by loose refs'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+   test_tick &&
+   git commit --allow-empty -m one &&
+   one=$(git rev-parse HEAD) &&
+   git for-each-ref >actual &&
+   echo "$one commit   refs/heads/master" >expect &&
+   test_cmp expect actual &&
+
+   git pack-refs --all &&
+   git for-each-ref >actual &&
+   echo "$one commit   refs/heads/master" >expect &&
+   test_cmp expect actual &&
+
+   git checkout --orphan another &&
+   test_tick &&
+   git commit --allow-empty -m two &&
+   two=$(git rev-parse HEAD) &&
+   git checkout -B master &&
+   git branch -D another &&
+
+   git for-each-ref >actual &&
+   echo "$two commit   refs/heads/master" >expect &&
+   test_cmp expect actual &&
+
+   git reflog expire --expire=now --all &&
+   git prune &&
+   git tag -m v1.0 v1.0 master
+'
+
+test_expect_success 'no error from stale entry in packed-refs' '
+   git describe master >actual 2>&1 &&
+   echo "v1.0" >expect &&
+   test_cmp expect actual
+'
+
+test_done
-- 
2.11.0

[PATCH v3 00/30] Create a reference backend for packed refs

[Michael Haggerty]

This is v3 of a patch series creating a `packed_ref_store` reference
backend. Thanks to Peff and Junio for their comments about v2 [1].

Changes since v2:

* Delete some debugging `cat` commands in t1408.

* Add some tests of reading packed-refs files with bogus contents.

* When reporting corruption in packed-refs files, distinguish between
  unterminated lines and other corruption.

* Fixed a typo in a commit message.

This patch series can also be obtained from my GitHub fork [2] as
branch packed-ref-store.

Michael

[1] v1: http://public-inbox.org/git/cover.1497534157.git.mhag...@alum.mit.edu/
v2: http://public-inbox.org/git/cover.1498200513.git.mhag...@alum.mit.edu/
[2] https://github.com/mhagger/git

Junio C Hamano (1):
  t1408: add a test of stale packed refs covered by loose refs

Michael Haggerty (29):
  add_packed_ref(): teach function to overwrite existing refs
  packed_ref_store: new struct
  packed_ref_store: move `packed_refs_path` here
  packed_ref_store: move `packed_refs_lock` member here
  clear_packed_ref_cache(): take a `packed_ref_store *` parameter
  validate_packed_ref_cache(): take a `packed_ref_store *` parameter
  get_packed_ref_cache(): take a `packed_ref_store *` parameter
  get_packed_refs(): take a `packed_ref_store *` parameter
  add_packed_ref(): take a `packed_ref_store *` parameter
  lock_packed_refs(): take a `packed_ref_store *` parameter
  commit_packed_refs(): take a `packed_ref_store *` parameter
  rollback_packed_refs(): take a `packed_ref_store *` parameter
  get_packed_ref(): take a `packed_ref_store *` parameter
  repack_without_refs(): take a `packed_ref_store *` parameter
  packed_peel_ref(): new function, extracted from `files_peel_ref()`
  packed_ref_store: support iteration
  packed_read_raw_ref(): new function, replacing `resolve_packed_ref()`
  packed-backend: new module for handling packed references
  packed_ref_store: make class into a subclass of `ref_store`
  commit_packed_refs(): report errors rather than dying
  commit_packed_refs(): use a staging file separate from the lockfile
  packed_refs_lock(): function renamed from lock_packed_refs()
  packed_refs_lock(): report errors via a `struct strbuf *err`
  packed_refs_unlock(), packed_refs_is_locked(): new functions
  clear_packed_ref_cache(): don't protest if the lock is held
  commit_packed_refs(): remove call to `packed_refs_unlock()`
  repack_without_refs(): don't lock or unlock the packed refs
  t3210: add some tests of bogus packed-refs file contents
  read_packed_refs(): die if `packed-refs` contains bogus data

 Makefile   |   1 +
 refs.c |  18 +
 refs/files-backend.c   | 631 ---
 refs/packed-backend.c  | 872 +
 refs/packed-backend.h  |  25 ++
 refs/refs-internal.h   |  10 +
 t/t1408-packed-refs.sh |  42 +++
 t/t3210-pack-refs.sh   |  27 ++
 8 files changed, 1066 insertions(+), 560 deletions(-)
 create mode 100644 refs/packed-backend.c
 create mode 100644 refs/packed-backend.h
 create mode 100755 t/t1408-packed-refs.sh

-- 
2.11.0

[PATCH v3 04/30] packed_ref_store: move `packed_refs_path` here

[Michael Haggerty]

Move `packed_refs_path` from `files_ref_store` to `packed_ref_store`,
and rename it to `path` since its meaning is clear from its new
context.

Inline `files_packed_refs_path()`.

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

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 2efb71cee9..c4b8e2f63b 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -54,6 +54,9 @@ struct packed_ref_cache {
 struct packed_ref_store {
unsigned int store_flags;
 
+   /* The path of the "packed-refs" file: */
+   char *path;
+
/*
 * A cache of the values read from the `packed-refs` file, if
 * it might still be current; otherwise, NULL.
@@ -61,11 +64,13 @@ struct packed_ref_store {
struct packed_ref_cache *cache;
 };
 
-static struct packed_ref_store *packed_ref_store_create(unsigned int 
store_flags)
+static struct packed_ref_store *packed_ref_store_create(
+   const char *path, unsigned int store_flags)
 {
struct packed_ref_store *refs = xcalloc(1, sizeof(*refs));
 
refs->store_flags = store_flags;
+   refs->path = xstrdup(path);
return refs;
 }
 
@@ -79,7 +84,6 @@ struct files_ref_store {
 
char *gitdir;
char *gitcommondir;
-   char *packed_refs_path;
 
struct ref_cache *loose;
 
@@ -154,8 +158,8 @@ static struct ref_store *files_ref_store_create(const char 
*gitdir,
get_common_dir_noenv(&sb, gitdir);
refs->gitcommondir = strbuf_detach(&sb, NULL);
strbuf_addf(&sb, "%s/packed-refs", refs->gitcommondir);
-   refs->packed_refs_path = strbuf_detach(&sb, NULL);
-   refs->packed_ref_store = packed_ref_store_create(flags);
+   refs->packed_ref_store = packed_ref_store_create(sb.buf, flags);
+   strbuf_release(&sb);
 
return ref_store;
 }
@@ -343,11 +347,6 @@ static struct packed_ref_cache *read_packed_refs(const 
char *packed_refs_file)
return packed_refs;
 }
 
-static const char *files_packed_refs_path(struct files_ref_store *refs)
-{
-   return refs->packed_refs_path;
-}
-
 static void files_reflog_path(struct files_ref_store *refs,
  struct strbuf *sb,
  const char *refname)
@@ -401,7 +400,7 @@ static void validate_packed_ref_cache(struct 
files_ref_store *refs)
 {
if (refs->packed_ref_store->cache &&
!stat_validity_check(&refs->packed_ref_store->cache->validity,
-files_packed_refs_path(refs)))
+refs->packed_ref_store->path))
clear_packed_ref_cache(refs);
 }
 
@@ -415,7 +414,7 @@ static void validate_packed_ref_cache(struct 
files_ref_store *refs)
  */
 static struct packed_ref_cache *get_packed_ref_cache(struct files_ref_store 
*refs)
 {
-   const char *packed_refs_file = files_packed_refs_path(refs);
+   const char *packed_refs_file = refs->packed_ref_store->path;
 
if (!is_lock_file_locked(&refs->packed_refs_lock))
validate_packed_ref_cache(refs);
@@ -1352,7 +1351,7 @@ static int lock_packed_refs(struct files_ref_store *refs, 
int flags)
}
 
if (hold_lock_file_for_update_timeout(
-   &refs->packed_refs_lock, 
files_packed_refs_path(refs),
+   &refs->packed_refs_lock, 
refs->packed_ref_store->path,
flags, timeout_value) < 0)
return -1;
 
@@ -1633,7 +1632,7 @@ static int repack_without_refs(struct files_ref_store 
*refs,
return 0; /* no refname exists in packed refs */
 
if (lock_packed_refs(refs, 0)) {
-   unable_to_lock_message(files_packed_refs_path(refs), errno, 
err);
+   unable_to_lock_message(refs->packed_ref_store->path, errno, 
err);
return -1;
}
packed = get_packed_refs(refs);
-- 
2.11.0

[PATCH v3 09/30] get_packed_refs(): take a `packed_ref_store *` parameter

[Michael Haggerty]

It only cares about the packed-refs part of the reference store.

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

diff --git a/refs/files-backend.c b/refs/files-backend.c
index b2ef7b3bb9..bc5c0de84e 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -427,9 +427,9 @@ static struct ref_dir *get_packed_ref_dir(struct 
packed_ref_cache *packed_ref_ca
return get_ref_dir(packed_ref_cache->cache->root);
 }
 
-static struct ref_dir *get_packed_refs(struct files_ref_store *refs)
+static struct ref_dir *get_packed_refs(struct packed_ref_store *refs)
 {
-   return get_packed_ref_dir(get_packed_ref_cache(refs->packed_ref_store));
+   return get_packed_ref_dir(get_packed_ref_cache(refs));
 }
 
 /*
@@ -450,7 +450,7 @@ static void add_packed_ref(struct files_ref_store *refs,
if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL))
die("Reference has invalid format: '%s'", refname);
 
-   packed_refs = get_packed_refs(refs);
+   packed_refs = get_packed_refs(refs->packed_ref_store);
packed_entry = find_ref_entry(packed_refs, refname);
if (packed_entry) {
/* Overwrite the existing entry: */
@@ -592,7 +592,7 @@ static struct ref_cache *get_loose_ref_cache(struct 
files_ref_store *refs)
 static struct ref_entry *get_packed_ref(struct files_ref_store *refs,
const char *refname)
 {
-   return find_ref_entry(get_packed_refs(refs), refname);
+   return find_ref_entry(get_packed_refs(refs->packed_ref_store), refname);
 }
 
 /*
@@ -1633,7 +1633,7 @@ static int repack_without_refs(struct files_ref_store 
*refs,
unable_to_lock_message(refs->packed_ref_store->path, errno, 
err);
return -1;
}
-   packed = get_packed_refs(refs);
+   packed = get_packed_refs(refs->packed_ref_store);
 
/* Remove refnames from the cache */
for_each_string_list_item(refname, refnames)
-- 
2.11.0

[PATCH v3 03/30] packed_ref_store: new struct

[Michael Haggerty]

Start extracting the packed-refs-related data structures into a new
class, `packed_ref_store`. It doesn't yet implement `ref_store`, but
it will.

Signed-off-by: Michael Haggerty 
---
 refs/files-backend.c | 42 +-
 1 file changed, 33 insertions(+), 9 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 87cecde231..2efb71cee9 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -47,6 +47,28 @@ struct packed_ref_cache {
struct stat_validity validity;
 };
 
+/*
+ * A container for `packed-refs`-related data. It is not (yet) a
+ * `ref_store`.
+ */
+struct packed_ref_store {
+   unsigned int store_flags;
+
+   /*
+* A cache of the values read from the `packed-refs` file, if
+* it might still be current; otherwise, NULL.
+*/
+   struct packed_ref_cache *cache;
+};
+
+static struct packed_ref_store *packed_ref_store_create(unsigned int 
store_flags)
+{
+   struct packed_ref_store *refs = xcalloc(1, sizeof(*refs));
+
+   refs->store_flags = store_flags;
+   return refs;
+}
+
 /*
  * Future: need to be in "struct repository"
  * when doing a full libification.
@@ -60,13 +82,14 @@ struct files_ref_store {
char *packed_refs_path;
 
struct ref_cache *loose;
-   struct packed_ref_cache *packed;
 
/*
 * Lock used for the "packed-refs" file. Note that this (and
 * thus the enclosing `files_ref_store`) must not be freed.
 */
struct lock_file packed_refs_lock;
+
+   struct packed_ref_store *packed_ref_store;
 };
 
 /*
@@ -95,12 +118,12 @@ static int release_packed_ref_cache(struct 
packed_ref_cache *packed_refs)
 
 static void clear_packed_ref_cache(struct files_ref_store *refs)
 {
-   if (refs->packed) {
-   struct packed_ref_cache *packed_refs = refs->packed;
+   if (refs->packed_ref_store->cache) {
+   struct packed_ref_cache *packed_refs = 
refs->packed_ref_store->cache;
 
if (is_lock_file_locked(&refs->packed_refs_lock))
die("BUG: packed-ref cache cleared while locked");
-   refs->packed = NULL;
+   refs->packed_ref_store->cache = NULL;
release_packed_ref_cache(packed_refs);
}
 }
@@ -132,6 +155,7 @@ static struct ref_store *files_ref_store_create(const char 
*gitdir,
refs->gitcommondir = strbuf_detach(&sb, NULL);
strbuf_addf(&sb, "%s/packed-refs", refs->gitcommondir);
refs->packed_refs_path = strbuf_detach(&sb, NULL);
+   refs->packed_ref_store = packed_ref_store_create(flags);
 
return ref_store;
 }
@@ -375,8 +399,8 @@ static void files_ref_path(struct files_ref_store *refs,
  */
 static void validate_packed_ref_cache(struct files_ref_store *refs)
 {
-   if (refs->packed &&
-   !stat_validity_check(&refs->packed->validity,
+   if (refs->packed_ref_store->cache &&
+   !stat_validity_check(&refs->packed_ref_store->cache->validity,
 files_packed_refs_path(refs)))
clear_packed_ref_cache(refs);
 }
@@ -396,10 +420,10 @@ static struct packed_ref_cache 
*get_packed_ref_cache(struct files_ref_store *ref
if (!is_lock_file_locked(&refs->packed_refs_lock))
validate_packed_ref_cache(refs);
 
-   if (!refs->packed)
-   refs->packed = read_packed_refs(packed_refs_file);
+   if (!refs->packed_ref_store->cache)
+   refs->packed_ref_store->cache = 
read_packed_refs(packed_refs_file);
 
-   return refs->packed;
+   return refs->packed_ref_store->cache;
 }
 
 static struct ref_dir *get_packed_ref_dir(struct packed_ref_cache 
*packed_ref_cache)
-- 
2.11.0

[PATCH v3 08/30] get_packed_ref_cache(): take a `packed_ref_store *` parameter

[Michael Haggerty]

It only cares about the packed-refs part of the reference store.

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

diff --git a/refs/files-backend.c b/refs/files-backend.c
index f061506bf0..b2ef7b3bb9 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -404,24 +404,22 @@ static void validate_packed_ref_cache(struct 
packed_ref_store *refs)
 }
 
 /*
- * Get the packed_ref_cache for the specified files_ref_store,
+ * Get the packed_ref_cache for the specified packed_ref_store,
  * creating and populating it if it hasn't been read before or if the
  * file has been changed (according to its `validity` field) since it
  * was last read. On the other hand, if we hold the lock, then assume
  * that the file hasn't been changed out from under us, so skip the
  * extra `stat()` call in `stat_validity_check()`.
  */
-static struct packed_ref_cache *get_packed_ref_cache(struct files_ref_store 
*refs)
+static struct packed_ref_cache *get_packed_ref_cache(struct packed_ref_store 
*refs)
 {
-   const char *packed_refs_file = refs->packed_ref_store->path;
+   if (!is_lock_file_locked(&refs->lock))
+   validate_packed_ref_cache(refs);
 
-   if (!is_lock_file_locked(&refs->packed_ref_store->lock))
-   validate_packed_ref_cache(refs->packed_ref_store);
-
-   if (!refs->packed_ref_store->cache)
-   refs->packed_ref_store->cache = 
read_packed_refs(packed_refs_file);
+   if (!refs->cache)
+   refs->cache = read_packed_refs(refs->path);
 
-   return refs->packed_ref_store->cache;
+   return refs->cache;
 }
 
 static struct ref_dir *get_packed_ref_dir(struct packed_ref_cache 
*packed_ref_cache)
@@ -431,7 +429,7 @@ static struct ref_dir *get_packed_ref_dir(struct 
packed_ref_cache *packed_ref_ca
 
 static struct ref_dir *get_packed_refs(struct files_ref_store *refs)
 {
-   return get_packed_ref_dir(get_packed_ref_cache(refs));
+   return get_packed_ref_dir(get_packed_ref_cache(refs->packed_ref_store));
 }
 
 /*
@@ -1151,7 +1149,7 @@ static struct ref_iterator *files_ref_iterator_begin(
loose_iter = cache_ref_iterator_begin(get_loose_ref_cache(refs),
  prefix, 1);
 
-   iter->packed_ref_cache = get_packed_ref_cache(refs);
+   iter->packed_ref_cache = get_packed_ref_cache(refs->packed_ref_store);
acquire_packed_ref_cache(iter->packed_ref_cache);
packed_iter = cache_ref_iterator_begin(iter->packed_ref_cache->cache,
   prefix, 0);
@@ -1365,7 +1363,7 @@ static int lock_packed_refs(struct files_ref_store *refs, 
int flags)
 */
validate_packed_ref_cache(refs->packed_ref_store);
 
-   packed_ref_cache = get_packed_ref_cache(refs);
+   packed_ref_cache = get_packed_ref_cache(refs->packed_ref_store);
/* Increment the reference count to prevent it from being freed: */
acquire_packed_ref_cache(packed_ref_cache);
return 0;
@@ -1380,7 +1378,7 @@ static int lock_packed_refs(struct files_ref_store *refs, 
int flags)
 static int commit_packed_refs(struct files_ref_store *refs)
 {
struct packed_ref_cache *packed_ref_cache =
-   get_packed_ref_cache(refs);
+   get_packed_ref_cache(refs->packed_ref_store);
int ok, error = 0;
int save_errno = 0;
FILE *out;
@@ -1426,7 +1424,7 @@ static int commit_packed_refs(struct files_ref_store 
*refs)
 static void rollback_packed_refs(struct files_ref_store *refs)
 {
struct packed_ref_cache *packed_ref_cache =
-   get_packed_ref_cache(refs);
+   get_packed_ref_cache(refs->packed_ref_store);
 
files_assert_main_repository(refs, "rollback_packed_refs");
 
-- 
2.11.0

[PATCH v3 10/30] add_packed_ref(): take a `packed_ref_store *` parameter

[Michael Haggerty]

It only cares about the packed-refs part of the reference store.

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

diff --git a/refs/files-backend.c b/refs/files-backend.c
index bc5c0de84e..4943207098 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -438,19 +438,19 @@ static struct ref_dir *get_packed_refs(struct 
packed_ref_store *refs)
  * (see lock_packed_refs()). To actually write the packed-refs file,
  * call commit_packed_refs().
  */
-static void add_packed_ref(struct files_ref_store *refs,
+static void add_packed_ref(struct packed_ref_store *refs,
   const char *refname, const struct object_id *oid)
 {
struct ref_dir *packed_refs;
struct ref_entry *packed_entry;
 
-   if (!is_lock_file_locked(&refs->packed_ref_store->lock))
+   if (!is_lock_file_locked(&refs->lock))
die("BUG: packed refs not locked");
 
if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL))
die("Reference has invalid format: '%s'", refname);
 
-   packed_refs = get_packed_refs(refs->packed_ref_store);
+   packed_refs = get_packed_refs(refs);
packed_entry = find_ref_entry(packed_refs, refname);
if (packed_entry) {
/* Overwrite the existing entry: */
@@ -1579,7 +1579,7 @@ static int files_pack_refs(struct ref_store *ref_store, 
unsigned int flags)
 * we don't copy the peeled status, because we want it
 * to be re-peeled.
 */
-   add_packed_ref(refs, iter->refname, iter->oid);
+   add_packed_ref(refs->packed_ref_store, iter->refname, 
iter->oid);
 
/* Schedule the loose reference for pruning if requested. */
if ((flags & PACK_REFS_PRUNE)) {
@@ -3210,7 +3210,7 @@ static int files_initial_transaction_commit(struct 
ref_store *ref_store,
 
if ((update->flags & REF_HAVE_NEW) &&
!is_null_oid(&update->new_oid))
-   add_packed_ref(refs, update->refname,
+   add_packed_ref(refs->packed_ref_store, update->refname,
   &update->new_oid);
}
 
-- 
2.11.0

[PATCH v3 05/30] packed_ref_store: move `packed_refs_lock` member here

[Michael Haggerty]

Move the `packed_refs_lock` member from `files_ref_store` to
`packed_ref_store`, and rename it to `lock` since it's now more
obvious what it is locking.

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

diff --git a/refs/files-backend.c b/refs/files-backend.c
index c4b8e2f63b..de8293493f 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -62,6 +62,12 @@ struct packed_ref_store {
 * it might still be current; otherwise, NULL.
 */
struct packed_ref_cache *cache;
+
+   /*
+* Lock used for the "packed-refs" file. Note that this (and
+* thus the enclosing `packed_ref_store`) must not be freed.
+*/
+   struct lock_file lock;
 };
 
 static struct packed_ref_store *packed_ref_store_create(
@@ -87,12 +93,6 @@ struct files_ref_store {
 
struct ref_cache *loose;
 
-   /*
-* Lock used for the "packed-refs" file. Note that this (and
-* thus the enclosing `files_ref_store`) must not be freed.
-*/
-   struct lock_file packed_refs_lock;
-
struct packed_ref_store *packed_ref_store;
 };
 
@@ -125,7 +125,7 @@ static void clear_packed_ref_cache(struct files_ref_store 
*refs)
if (refs->packed_ref_store->cache) {
struct packed_ref_cache *packed_refs = 
refs->packed_ref_store->cache;
 
-   if (is_lock_file_locked(&refs->packed_refs_lock))
+   if (is_lock_file_locked(&refs->packed_ref_store->lock))
die("BUG: packed-ref cache cleared while locked");
refs->packed_ref_store->cache = NULL;
release_packed_ref_cache(packed_refs);
@@ -416,7 +416,7 @@ static struct packed_ref_cache *get_packed_ref_cache(struct 
files_ref_store *ref
 {
const char *packed_refs_file = refs->packed_ref_store->path;
 
-   if (!is_lock_file_locked(&refs->packed_refs_lock))
+   if (!is_lock_file_locked(&refs->packed_ref_store->lock))
validate_packed_ref_cache(refs);
 
if (!refs->packed_ref_store->cache)
@@ -447,7 +447,7 @@ static void add_packed_ref(struct files_ref_store *refs,
struct ref_dir *packed_refs;
struct ref_entry *packed_entry;
 
-   if (!is_lock_file_locked(&refs->packed_refs_lock))
+   if (!is_lock_file_locked(&refs->packed_ref_store->lock))
die("BUG: packed refs not locked");
 
if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL))
@@ -1351,7 +1351,8 @@ static int lock_packed_refs(struct files_ref_store *refs, 
int flags)
}
 
if (hold_lock_file_for_update_timeout(
-   &refs->packed_refs_lock, 
refs->packed_ref_store->path,
+   &refs->packed_ref_store->lock,
+   refs->packed_ref_store->path,
flags, timeout_value) < 0)
return -1;
 
@@ -1388,10 +1389,10 @@ static int commit_packed_refs(struct files_ref_store 
*refs)
 
files_assert_main_repository(refs, "commit_packed_refs");
 
-   if (!is_lock_file_locked(&refs->packed_refs_lock))
+   if (!is_lock_file_locked(&refs->packed_ref_store->lock))
die("BUG: packed-refs not locked");
 
-   out = fdopen_lock_file(&refs->packed_refs_lock, "w");
+   out = fdopen_lock_file(&refs->packed_ref_store->lock, "w");
if (!out)
die_errno("unable to fdopen packed-refs descriptor");
 
@@ -1409,7 +1410,7 @@ static int commit_packed_refs(struct files_ref_store 
*refs)
if (ok != ITER_DONE)
die("error while iterating over references");
 
-   if (commit_lock_file(&refs->packed_refs_lock)) {
+   if (commit_lock_file(&refs->packed_ref_store->lock)) {
save_errno = errno;
error = -1;
}
@@ -1430,9 +1431,9 @@ static void rollback_packed_refs(struct files_ref_store 
*refs)
 
files_assert_main_repository(refs, "rollback_packed_refs");
 
-   if (!is_lock_file_locked(&refs->packed_refs_lock))
+   if (!is_lock_file_locked(&refs->packed_ref_store->lock))
die("BUG: packed-refs not locked");
-   rollback_lock_file(&refs->packed_refs_lock);
+   rollback_lock_file(&refs->packed_ref_store->lock);
release_packed_ref_cache(packed_ref_cache);
clear_packed_ref_cache(refs);
 }
-- 
2.11.0

[PATCH v3 12/30] commit_packed_refs(): take a `packed_ref_store *` parameter

[Michael Haggerty]

It only cares about the packed-refs part of the reference store.

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

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 0d8f39089f..5d159620f0 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1388,21 +1388,21 @@ static int lock_packed_refs(struct packed_ref_store 
*refs, int flags)
  * lock_packed_refs()). Return zero on success. On errors, set errno
  * and return a nonzero value
  */
-static int commit_packed_refs(struct files_ref_store *refs)
+static int commit_packed_refs(struct packed_ref_store *refs)
 {
struct packed_ref_cache *packed_ref_cache =
-   get_packed_ref_cache(refs->packed_ref_store);
+   get_packed_ref_cache(refs);
int ok, error = 0;
int save_errno = 0;
FILE *out;
struct ref_iterator *iter;
 
-   files_assert_main_repository(refs, "commit_packed_refs");
+   packed_assert_main_repository(refs, "commit_packed_refs");
 
-   if (!is_lock_file_locked(&refs->packed_ref_store->lock))
+   if (!is_lock_file_locked(&refs->lock))
die("BUG: packed-refs not locked");
 
-   out = fdopen_lock_file(&refs->packed_ref_store->lock, "w");
+   out = fdopen_lock_file(&refs->lock, "w");
if (!out)
die_errno("unable to fdopen packed-refs descriptor");
 
@@ -1420,7 +1420,7 @@ static int commit_packed_refs(struct files_ref_store 
*refs)
if (ok != ITER_DONE)
die("error while iterating over references");
 
-   if (commit_lock_file(&refs->packed_ref_store->lock)) {
+   if (commit_lock_file(&refs->lock)) {
save_errno = errno;
error = -1;
}
@@ -1606,7 +1606,7 @@ static int files_pack_refs(struct ref_store *ref_store, 
unsigned int flags)
if (ok != ITER_DONE)
die("error while iterating over references");
 
-   if (commit_packed_refs(refs))
+   if (commit_packed_refs(refs->packed_ref_store))
die_errno("unable to overwrite old ref-pack file");
 
prune_refs(refs, refs_to_prune);
@@ -1662,7 +1662,7 @@ static int repack_without_refs(struct files_ref_store 
*refs,
}
 
/* Write what remains */
-   ret = commit_packed_refs(refs);
+   ret = commit_packed_refs(refs->packed_ref_store);
if (ret)
strbuf_addf(err, "unable to overwrite old ref-pack file: %s",
strerror(errno));
@@ -3227,7 +3227,7 @@ static int files_initial_transaction_commit(struct 
ref_store *ref_store,
   &update->new_oid);
}
 
-   if (commit_packed_refs(refs)) {
+   if (commit_packed_refs(refs->packed_ref_store)) {
strbuf_addf(err, "unable to commit packed-refs file: %s",
strerror(errno));
ret = TRANSACTION_GENERIC_ERROR;
-- 
2.11.0

[PATCH v3 14/30] get_packed_ref(): take a `packed_ref_store *` parameter

[Michael Haggerty]

It only cares about the packed-refs part of the reference store.

Signed-off-by: Michael Haggerty 
---
 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 a08d3fbadf..2b9d93d3b6 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -602,10 +602,10 @@ static struct ref_cache *get_loose_ref_cache(struct 
files_ref_store *refs)
  * Return the ref_entry for the given refname from the packed
  * references.  If it does not exist, return NULL.
  */
-static struct ref_entry *get_packed_ref(struct files_ref_store *refs,
+static struct ref_entry *get_packed_ref(struct packed_ref_store *refs,
const char *refname)
 {
-   return find_ref_entry(get_packed_refs(refs->packed_ref_store), refname);
+   return find_ref_entry(get_packed_refs(refs), refname);
 }
 
 /*
@@ -621,7 +621,7 @@ static int resolve_packed_ref(struct files_ref_store *refs,
 * The loose reference file does not exist; check for a packed
 * reference.
 */
-   entry = get_packed_ref(refs, refname);
+   entry = get_packed_ref(refs->packed_ref_store, refname);
if (entry) {
hashcpy(sha1, entry->u.value.oid.hash);
*flags |= REF_ISPACKED;
@@ -1044,7 +1044,9 @@ static int files_peel_ref(struct ref_store *ref_store,
 * have REF_KNOWS_PEELED.
 */
if (flag & REF_ISPACKED) {
-   struct ref_entry *r = get_packed_ref(refs, refname);
+   struct ref_entry *r =
+   get_packed_ref(refs->packed_ref_store, refname);
+
if (r) {
if (peel_entry(r, 0))
return -1;
@@ -1631,7 +1633,7 @@ static int repack_without_refs(struct files_ref_store 
*refs,
 
/* Look for a packed ref */
for_each_string_list_item(refname, refnames) {
-   if (get_packed_ref(refs, refname->string)) {
+   if (get_packed_ref(refs->packed_ref_store, refname->string)) {
needs_repacking = 1;
break;
}
-- 
2.11.0

[PATCH v3 06/30] clear_packed_ref_cache(): take a `packed_ref_store *` parameter

[Michael Haggerty]

It only cares about the packed-refs part of the reference store.

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

diff --git a/refs/files-backend.c b/refs/files-backend.c
index de8293493f..2b0db60b2b 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -120,15 +120,15 @@ static int release_packed_ref_cache(struct 
packed_ref_cache *packed_refs)
}
 }
 
-static void clear_packed_ref_cache(struct files_ref_store *refs)
+static void clear_packed_ref_cache(struct packed_ref_store *refs)
 {
-   if (refs->packed_ref_store->cache) {
-   struct packed_ref_cache *packed_refs = 
refs->packed_ref_store->cache;
+   if (refs->cache) {
+   struct packed_ref_cache *cache = refs->cache;
 
-   if (is_lock_file_locked(&refs->packed_ref_store->lock))
+   if (is_lock_file_locked(&refs->lock))
die("BUG: packed-ref cache cleared while locked");
-   refs->packed_ref_store->cache = NULL;
-   release_packed_ref_cache(packed_refs);
+   refs->cache = NULL;
+   release_packed_ref_cache(cache);
}
 }
 
@@ -401,7 +401,7 @@ static void validate_packed_ref_cache(struct 
files_ref_store *refs)
if (refs->packed_ref_store->cache &&
!stat_validity_check(&refs->packed_ref_store->cache->validity,
 refs->packed_ref_store->path))
-   clear_packed_ref_cache(refs);
+   clear_packed_ref_cache(refs->packed_ref_store);
 }
 
 /*
@@ -1435,7 +1435,7 @@ static void rollback_packed_refs(struct files_ref_store 
*refs)
die("BUG: packed-refs not locked");
rollback_lock_file(&refs->packed_ref_store->lock);
release_packed_ref_cache(packed_ref_cache);
-   clear_packed_ref_cache(refs);
+   clear_packed_ref_cache(refs->packed_ref_store);
 }
 
 struct ref_to_prune {
-- 
2.11.0

[PATCH v3 16/30] packed_peel_ref(): new function, extracted from `files_peel_ref()`

[Michael Haggerty]

This will later become a method of `packed_ref_store`.

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

diff --git a/refs/files-backend.c b/refs/files-backend.c
index c206791b91..185d05e1d6 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1013,6 +1013,18 @@ static int lock_raw_ref(struct files_ref_store *refs,
return ret;
 }
 
+static int packed_peel_ref(struct packed_ref_store *refs,
+  const char *refname, unsigned char *sha1)
+{
+   struct ref_entry *r = get_packed_ref(refs, refname);
+
+   if (!r || peel_entry(r, 0))
+   return -1;
+
+   hashcpy(sha1, r->u.value.peeled.hash);
+   return 0;
+}
+
 static int files_peel_ref(struct ref_store *ref_store,
  const char *refname, unsigned char *sha1)
 {
@@ -1043,17 +1055,9 @@ static int files_peel_ref(struct ref_store *ref_store,
 * be expensive and (b) loose references anyway usually do not
 * have REF_KNOWS_PEELED.
 */
-   if (flag & REF_ISPACKED) {
-   struct ref_entry *r =
-   get_packed_ref(refs->packed_ref_store, refname);
-
-   if (r) {
-   if (peel_entry(r, 0))
-   return -1;
-   hashcpy(sha1, r->u.value.peeled.hash);
-   return 0;
-   }
-   }
+   if (flag & REF_ISPACKED &&
+   !packed_peel_ref(refs->packed_ref_store, refname, sha1))
+   return 0;
 
return peel_object(base, sha1);
 }
-- 
2.11.0

[PATCH v3 07/30] validate_packed_ref_cache(): take a `packed_ref_store *` parameter

[Michael Haggerty]

It only cares about the packed-refs part of the reference store.

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

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 2b0db60b2b..f061506bf0 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -396,12 +396,11 @@ static void files_ref_path(struct files_ref_store *refs,
  * Check that the packed refs cache (if any) still reflects the
  * contents of the file. If not, clear the cache.
  */
-static void validate_packed_ref_cache(struct files_ref_store *refs)
+static void validate_packed_ref_cache(struct packed_ref_store *refs)
 {
-   if (refs->packed_ref_store->cache &&
-   !stat_validity_check(&refs->packed_ref_store->cache->validity,
-refs->packed_ref_store->path))
-   clear_packed_ref_cache(refs->packed_ref_store);
+   if (refs->cache &&
+   !stat_validity_check(&refs->cache->validity, refs->path))
+   clear_packed_ref_cache(refs);
 }
 
 /*
@@ -417,7 +416,7 @@ static struct packed_ref_cache *get_packed_ref_cache(struct 
files_ref_store *ref
const char *packed_refs_file = refs->packed_ref_store->path;
 
if (!is_lock_file_locked(&refs->packed_ref_store->lock))
-   validate_packed_ref_cache(refs);
+   validate_packed_ref_cache(refs->packed_ref_store);
 
if (!refs->packed_ref_store->cache)
refs->packed_ref_store->cache = 
read_packed_refs(packed_refs_file);
@@ -1364,7 +1363,7 @@ static int lock_packed_refs(struct files_ref_store *refs, 
int flags)
 * cache is still valid. We've just locked the file, but it
 * might have changed the moment *before* we locked it.
 */
-   validate_packed_ref_cache(refs);
+   validate_packed_ref_cache(refs->packed_ref_store);
 
packed_ref_cache = get_packed_ref_cache(refs);
/* Increment the reference count to prevent it from being freed: */
-- 
2.11.0

[PATCH v3 15/30] repack_without_refs(): take a `packed_ref_store *` parameter

[Michael Haggerty]

It only cares about the packed-refs part of the reference store.

Signed-off-by: Michael Haggerty 
---
 refs/files-backend.c | 20 ++--
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 2b9d93d3b6..c206791b91 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1621,19 +1621,19 @@ static int files_pack_refs(struct ref_store *ref_store, 
unsigned int flags)
  *
  * The refs in 'refnames' needn't be sorted. `err` must not be NULL.
  */
-static int repack_without_refs(struct files_ref_store *refs,
+static int repack_without_refs(struct packed_ref_store *refs,
   struct string_list *refnames, struct strbuf *err)
 {
struct ref_dir *packed;
struct string_list_item *refname;
int ret, needs_repacking = 0, removed = 0;
 
-   files_assert_main_repository(refs, "repack_without_refs");
+   packed_assert_main_repository(refs, "repack_without_refs");
assert(err);
 
/* Look for a packed ref */
for_each_string_list_item(refname, refnames) {
-   if (get_packed_ref(refs->packed_ref_store, refname->string)) {
+   if (get_packed_ref(refs, refname->string)) {
needs_repacking = 1;
break;
}
@@ -1643,11 +1643,11 @@ static int repack_without_refs(struct files_ref_store 
*refs,
if (!needs_repacking)
return 0; /* no refname exists in packed refs */
 
-   if (lock_packed_refs(refs->packed_ref_store, 0)) {
-   unable_to_lock_message(refs->packed_ref_store->path, errno, 
err);
+   if (lock_packed_refs(refs, 0)) {
+   unable_to_lock_message(refs->path, errno, err);
return -1;
}
-   packed = get_packed_refs(refs->packed_ref_store);
+   packed = get_packed_refs(refs);
 
/* Remove refnames from the cache */
for_each_string_list_item(refname, refnames)
@@ -1658,12 +1658,12 @@ static int repack_without_refs(struct files_ref_store 
*refs,
 * All packed entries disappeared while we were
 * acquiring the lock.
 */
-   rollback_packed_refs(refs->packed_ref_store);
+   rollback_packed_refs(refs);
return 0;
}
 
/* Write what remains */
-   ret = commit_packed_refs(refs->packed_ref_store);
+   ret = commit_packed_refs(refs);
if (ret)
strbuf_addf(err, "unable to overwrite old ref-pack file: %s",
strerror(errno));
@@ -1681,7 +1681,7 @@ static int files_delete_refs(struct ref_store *ref_store, 
const char *msg,
if (!refnames->nr)
return 0;
 
-   result = repack_without_refs(refs, refnames, &err);
+   result = repack_without_refs(refs->packed_ref_store, refnames, &err);
if (result) {
/*
 * If we failed to rewrite the packed-refs file, then
@@ -3101,7 +3101,7 @@ static int files_transaction_finish(struct ref_store 
*ref_store,
}
}
 
-   if (repack_without_refs(refs, &refs_to_delete, err)) {
+   if (repack_without_refs(refs->packed_ref_store, &refs_to_delete, err)) {
ret = TRANSACTION_GENERIC_ERROR;
goto cleanup;
}
-- 
2.11.0

[PATCH v3 02/30] add_packed_ref(): teach function to overwrite existing refs

[Michael Haggerty]

Teach `add_packed_ref()` to overwrite an existing entry if one already
exists for the specified `refname`. This means that we can call it
from `files_pack_refs()`, thereby reducing the amount that the latter
function needs to know about the internals of packed-reference
handling.

Signed-off-by: Michael Haggerty 
---
 refs/files-backend.c | 40 ++--
 1 file changed, 18 insertions(+), 22 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index b040bb3b0a..87cecde231 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -413,15 +413,16 @@ static struct ref_dir *get_packed_refs(struct 
files_ref_store *refs)
 }
 
 /*
- * Add a reference to the in-memory packed reference cache.  This may
- * only be called while the packed-refs file is locked (see
- * lock_packed_refs()).  To actually write the packed-refs file, call
- * commit_packed_refs().
+ * Add or overwrite a reference in the in-memory packed reference
+ * cache. This may only be called while the packed-refs file is locked
+ * (see lock_packed_refs()). To actually write the packed-refs file,
+ * call commit_packed_refs().
  */
 static void add_packed_ref(struct files_ref_store *refs,
   const char *refname, const struct object_id *oid)
 {
-   struct packed_ref_cache *packed_ref_cache = get_packed_ref_cache(refs);
+   struct ref_dir *packed_refs;
+   struct ref_entry *packed_entry;
 
if (!is_lock_file_locked(&refs->packed_refs_lock))
die("BUG: packed refs not locked");
@@ -429,8 +430,17 @@ static void add_packed_ref(struct files_ref_store *refs,
if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL))
die("Reference has invalid format: '%s'", refname);
 
-   add_ref_entry(get_packed_ref_dir(packed_ref_cache),
- create_ref_entry(refname, oid, REF_ISPACKED));
+   packed_refs = get_packed_refs(refs);
+   packed_entry = find_ref_entry(packed_refs, refname);
+   if (packed_entry) {
+   /* Overwrite the existing entry: */
+   oidcpy(&packed_entry->u.value.oid, oid);
+   packed_entry->flag = REF_ISPACKED;
+   oidclr(&packed_entry->u.value.peeled);
+   } else {
+   packed_entry = create_ref_entry(refname, oid, REF_ISPACKED);
+   add_ref_entry(packed_refs, packed_entry);
+   }
 }
 
 /*
@@ -1526,12 +1536,10 @@ static int files_pack_refs(struct ref_store *ref_store, 
unsigned int flags)
files_downcast(ref_store, REF_STORE_WRITE | REF_STORE_ODB,
   "pack_refs");
struct ref_iterator *iter;
-   struct ref_dir *packed_refs;
int ok;
struct ref_to_prune *refs_to_prune = NULL;
 
lock_packed_refs(refs, LOCK_DIE_ON_ERROR);
-   packed_refs = get_packed_refs(refs);
 
iter = cache_ref_iterator_begin(get_loose_ref_cache(refs), NULL, 0);
while ((ok = ref_iterator_advance(iter)) == ITER_OK) {
@@ -1540,8 +1548,6 @@ static int files_pack_refs(struct ref_store *ref_store, 
unsigned int flags)
 * in the packed ref cache. If the reference should be
 * pruned, also add it to refs_to_prune.
 */
-   struct ref_entry *packed_entry;
-
if (!should_pack_ref(iter->refname, iter->oid, iter->flags,
 flags))
continue;
@@ -1552,17 +1558,7 @@ static int files_pack_refs(struct ref_store *ref_store, 
unsigned int flags)
 * we don't copy the peeled status, because we want it
 * to be re-peeled.
 */
-   packed_entry = find_ref_entry(packed_refs, iter->refname);
-   if (packed_entry) {
-   /* Overwrite existing packed entry with info from loose 
entry */
-   packed_entry->flag = REF_ISPACKED;
-   oidcpy(&packed_entry->u.value.oid, iter->oid);
-   } else {
-   packed_entry = create_ref_entry(iter->refname, 
iter->oid,
-   REF_ISPACKED);
-   add_ref_entry(packed_refs, packed_entry);
-   }
-   oidclr(&packed_entry->u.value.peeled);
+   add_packed_ref(refs, iter->refname, iter->oid);
 
/* Schedule the loose reference for pruning if requested. */
if ((flags & PACK_REFS_PRUNE)) {
-- 
2.11.0

[PATCH v3 19/30] packed-backend: new module for handling packed references

[Michael Haggerty]

Now that the interface between `files_ref_store` and
`packed_ref_store` is relatively narrow, move the latter into a new
module, "refs/packed-backend.h" and "refs/packed-backend.c". It still
doesn't quite implement the `ref_store` interface, but it will soon.

This commit moves code around and adjusts its visibility, but doesn't
change anything.

Signed-off-by: Michael Haggerty 
---
 Makefile  |   1 +
 refs.c|  18 ++
 refs/files-backend.c  | 640 +-
 refs/packed-backend.c | 624 
 refs/packed-backend.h |  33 +++
 refs/refs-internal.h  |   9 +
 6 files changed, 686 insertions(+), 639 deletions(-)
 create mode 100644 refs/packed-backend.c
 create mode 100644 refs/packed-backend.h

diff --git a/Makefile b/Makefile
index e35542e631..aff9c6d7e2 100644
--- a/Makefile
+++ b/Makefile
@@ -817,6 +817,7 @@ LIB_OBJS += reflog-walk.o
 LIB_OBJS += refs.o
 LIB_OBJS += refs/files-backend.o
 LIB_OBJS += refs/iterator.o
+LIB_OBJS += refs/packed-backend.o
 LIB_OBJS += refs/ref-cache.o
 LIB_OBJS += ref-filter.o
 LIB_OBJS += remote.o
diff --git a/refs.c b/refs.c
index 32177969f0..5880c12372 100644
--- a/refs.c
+++ b/refs.c
@@ -173,6 +173,24 @@ int refname_is_safe(const char *refname)
return 1;
 }
 
+/*
+ * Return true if refname, which has the specified oid and flags, can
+ * be resolved to an object in the database. If the referred-to object
+ * does not exist, emit a warning and return false.
+ */
+int ref_resolves_to_object(const char *refname,
+  const struct object_id *oid,
+  unsigned int flags)
+{
+   if (flags & REF_ISBROKEN)
+   return 0;
+   if (!has_sha1_file(oid->hash)) {
+   error("%s does not point to a valid object!", refname);
+   return 0;
+   }
+   return 1;
+}
+
 char *refs_resolve_refdup(struct ref_store *refs,
  const char *refname, int resolve_flags,
  unsigned char *sha1, int *flags)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 346794cf7c..7df9747798 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2,6 +2,7 @@
 #include "../refs.h"
 #include "refs-internal.h"
 #include "ref-cache.h"
+#include "packed-backend.h"
 #include "../iterator.h"
 #include "../dir-iterator.h"
 #include "../lockfile.h"
@@ -14,85 +15,6 @@ struct ref_lock {
struct object_id old_oid;
 };
 
-/*
- * Return true if refname, which has the specified oid and flags, can
- * be resolved to an object in the database. If the referred-to object
- * does not exist, emit a warning and return false.
- */
-static int ref_resolves_to_object(const char *refname,
- const struct object_id *oid,
- unsigned int flags)
-{
-   if (flags & REF_ISBROKEN)
-   return 0;
-   if (!has_sha1_file(oid->hash)) {
-   error("%s does not point to a valid object!", refname);
-   return 0;
-   }
-   return 1;
-}
-
-struct packed_ref_cache {
-   struct ref_cache *cache;
-
-   /*
-* Count of references to the data structure in this instance,
-* including the pointer from files_ref_store::packed if any.
-* The data will not be freed as long as the reference count
-* is nonzero.
-*/
-   unsigned int referrers;
-
-   /* The metadata from when this packed-refs cache was read */
-   struct stat_validity validity;
-};
-
-/*
- * A container for `packed-refs`-related data. It is not (yet) a
- * `ref_store`.
- */
-struct packed_ref_store {
-   unsigned int store_flags;
-
-   /* The path of the "packed-refs" file: */
-   char *path;
-
-   /*
-* A cache of the values read from the `packed-refs` file, if
-* it might still be current; otherwise, NULL.
-*/
-   struct packed_ref_cache *cache;
-
-   /*
-* Lock used for the "packed-refs" file. Note that this (and
-* thus the enclosing `packed_ref_store`) must not be freed.
-*/
-   struct lock_file lock;
-};
-
-static struct packed_ref_store *packed_ref_store_create(
-   const char *path, unsigned int store_flags)
-{
-   struct packed_ref_store *refs = xcalloc(1, sizeof(*refs));
-
-   refs->store_flags = store_flags;
-   refs->path = xstrdup(path);
-   return refs;
-}
-
-/*
- * Die if refs is not the main ref store. caller is used in any
- * necessary error messages.
- */
-static void packed_assert_main_repository(struct packed_ref_store *refs,
- const char *caller)
-{
-   if (refs->store_flags & REF_STORE_MAIN)
-   return;
-
-   die("BUG: operation %s only allowed for main ref store", caller);
-}
-
 /*
  * Future: need to be in "struct repository"
  * when doing a full libification.
@@ -109,42

[PATCH v3 17/30] packed_ref_store: support iteration

[Michael Haggerty]

Add the infrastructure to iterate over a `packed_ref_store`. It's a
lot of boilerplate, but it's all part of a campaign to make
`packed_ref_store` implement `ref_store`. In the future, this iterator
will work much differently.

Signed-off-by: Michael Haggerty 
---
 refs/files-backend.c | 119 +++
 1 file changed, 110 insertions(+), 9 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 185d05e1d6..0490cc087e 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1062,10 +1062,102 @@ static int files_peel_ref(struct ref_store *ref_store,
return peel_object(base, sha1);
 }
 
+struct packed_ref_iterator {
+   struct ref_iterator base;
+
+   struct packed_ref_cache *cache;
+   struct ref_iterator *iter0;
+   unsigned int flags;
+};
+
+static int packed_ref_iterator_advance(struct ref_iterator *ref_iterator)
+{
+   struct packed_ref_iterator *iter =
+   (struct packed_ref_iterator *)ref_iterator;
+   int ok;
+
+   while ((ok = ref_iterator_advance(iter->iter0)) == ITER_OK) {
+   if (iter->flags & DO_FOR_EACH_PER_WORKTREE_ONLY &&
+   ref_type(iter->iter0->refname) != REF_TYPE_PER_WORKTREE)
+   continue;
+
+   if (!(iter->flags & DO_FOR_EACH_INCLUDE_BROKEN) &&
+   !ref_resolves_to_object(iter->iter0->refname,
+   iter->iter0->oid,
+   iter->iter0->flags))
+   continue;
+
+   iter->base.refname = iter->iter0->refname;
+   iter->base.oid = iter->iter0->oid;
+   iter->base.flags = iter->iter0->flags;
+   return ITER_OK;
+   }
+
+   iter->iter0 = NULL;
+   if (ref_iterator_abort(ref_iterator) != ITER_DONE)
+   ok = ITER_ERROR;
+
+   return ok;
+}
+
+static int packed_ref_iterator_peel(struct ref_iterator *ref_iterator,
+  struct object_id *peeled)
+{
+   struct packed_ref_iterator *iter =
+   (struct packed_ref_iterator *)ref_iterator;
+
+   return ref_iterator_peel(iter->iter0, peeled);
+}
+
+static int packed_ref_iterator_abort(struct ref_iterator *ref_iterator)
+{
+   struct packed_ref_iterator *iter =
+   (struct packed_ref_iterator *)ref_iterator;
+   int ok = ITER_DONE;
+
+   if (iter->iter0)
+   ok = ref_iterator_abort(iter->iter0);
+
+   release_packed_ref_cache(iter->cache);
+   base_ref_iterator_free(ref_iterator);
+   return ok;
+}
+
+static struct ref_iterator_vtable packed_ref_iterator_vtable = {
+   packed_ref_iterator_advance,
+   packed_ref_iterator_peel,
+   packed_ref_iterator_abort
+};
+
+static struct ref_iterator *packed_ref_iterator_begin(
+   struct packed_ref_store *refs,
+   const char *prefix, unsigned int flags)
+{
+   struct packed_ref_iterator *iter;
+   struct ref_iterator *ref_iterator;
+
+   iter = xcalloc(1, sizeof(*iter));
+   ref_iterator = &iter->base;
+   base_ref_iterator_init(ref_iterator, &packed_ref_iterator_vtable);
+
+   /*
+* Note that get_packed_ref_cache() internally checks whether
+* the packed-ref cache is up to date with what is on disk,
+* and re-reads it if not.
+*/
+
+   iter->cache = get_packed_ref_cache(refs);
+   acquire_packed_ref_cache(iter->cache);
+   iter->iter0 = cache_ref_iterator_begin(iter->cache->cache, prefix, 0);
+
+   iter->flags = flags;
+
+   return ref_iterator;
+}
+
 struct files_ref_iterator {
struct ref_iterator base;
 
-   struct packed_ref_cache *packed_ref_cache;
struct ref_iterator *iter0;
unsigned int flags;
 };
@@ -1118,7 +1210,6 @@ static int files_ref_iterator_abort(struct ref_iterator 
*ref_iterator)
if (iter->iter0)
ok = ref_iterator_abort(iter->iter0);
 
-   release_packed_ref_cache(iter->packed_ref_cache);
base_ref_iterator_free(ref_iterator);
return ok;
 }
@@ -1160,18 +1251,28 @@ static struct ref_iterator *files_ref_iterator_begin(
 * (If they've already been read, that's OK; we only need to
 * guarantee that they're read before the packed refs, not
 * *how much* before.) After that, we call
-* get_packed_ref_cache(), which internally checks whether the
-* packed-ref cache is up to date with what is on disk, and
-* re-reads it if not.
+* packed_ref_iterator_begin(), which internally checks
+* whether the packed-ref cache is up to date with what is on
+* disk, and re-reads it if not.
 */
 
loose_iter = cache_ref_iterator_begin(get_loose_ref_cache(refs),
  prefix, 1);
 
-   iter->packed_ref_cache = get_packed_ref_cache(refs->packed_ref_store);
-  

[PATCH v3 11/30] lock_packed_refs(): take a `packed_ref_store *` parameter

[Michael Haggerty]

It only cares about the packed-refs part of the reference store.

Signed-off-by: Michael Haggerty 
---
 refs/files-backend.c | 31 ++-
 1 file changed, 22 insertions(+), 9 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 4943207098..0d8f39089f 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -80,6 +80,19 @@ static struct packed_ref_store *packed_ref_store_create(
return refs;
 }
 
+/*
+ * Die if refs is not the main ref store. caller is used in any
+ * necessary error messages.
+ */
+static void packed_assert_main_repository(struct packed_ref_store *refs,
+ const char *caller)
+{
+   if (refs->store_flags & REF_STORE_MAIN)
+   return;
+
+   die("BUG: operation %s only allowed for main ref store", caller);
+}
+
 /*
  * Future: need to be in "struct repository"
  * when doing a full libification.
@@ -1334,13 +1347,13 @@ static void write_packed_entry(FILE *fh, const char 
*refname,
  * hold_lock_file_for_update(). Return 0 on success. On errors, set
  * errno appropriately and return a nonzero value.
  */
-static int lock_packed_refs(struct files_ref_store *refs, int flags)
+static int lock_packed_refs(struct packed_ref_store *refs, int flags)
 {
static int timeout_configured = 0;
static int timeout_value = 1000;
struct packed_ref_cache *packed_ref_cache;
 
-   files_assert_main_repository(refs, "lock_packed_refs");
+   packed_assert_main_repository(refs, "lock_packed_refs");
 
if (!timeout_configured) {
git_config_get_int("core.packedrefstimeout", &timeout_value);
@@ -1348,8 +1361,8 @@ static int lock_packed_refs(struct files_ref_store *refs, 
int flags)
}
 
if (hold_lock_file_for_update_timeout(
-   &refs->packed_ref_store->lock,
-   refs->packed_ref_store->path,
+   &refs->lock,
+   refs->path,
flags, timeout_value) < 0)
return -1;
 
@@ -1361,9 +1374,9 @@ static int lock_packed_refs(struct files_ref_store *refs, 
int flags)
 * cache is still valid. We've just locked the file, but it
 * might have changed the moment *before* we locked it.
 */
-   validate_packed_ref_cache(refs->packed_ref_store);
+   validate_packed_ref_cache(refs);
 
-   packed_ref_cache = get_packed_ref_cache(refs->packed_ref_store);
+   packed_ref_cache = get_packed_ref_cache(refs);
/* Increment the reference count to prevent it from being freed: */
acquire_packed_ref_cache(packed_ref_cache);
return 0;
@@ -1560,7 +1573,7 @@ static int files_pack_refs(struct ref_store *ref_store, 
unsigned int flags)
int ok;
struct ref_to_prune *refs_to_prune = NULL;
 
-   lock_packed_refs(refs, LOCK_DIE_ON_ERROR);
+   lock_packed_refs(refs->packed_ref_store, LOCK_DIE_ON_ERROR);
 
iter = cache_ref_iterator_begin(get_loose_ref_cache(refs), NULL, 0);
while ((ok = ref_iterator_advance(iter)) == ITER_OK) {
@@ -1629,7 +1642,7 @@ static int repack_without_refs(struct files_ref_store 
*refs,
if (!needs_repacking)
return 0; /* no refname exists in packed refs */
 
-   if (lock_packed_refs(refs, 0)) {
+   if (lock_packed_refs(refs->packed_ref_store, 0)) {
unable_to_lock_message(refs->packed_ref_store->path, errno, 
err);
return -1;
}
@@ -3198,7 +3211,7 @@ static int files_initial_transaction_commit(struct 
ref_store *ref_store,
}
}
 
-   if (lock_packed_refs(refs, 0)) {
+   if (lock_packed_refs(refs->packed_ref_store, 0)) {
strbuf_addf(err, "unable to lock packed-refs file: %s",
strerror(errno));
ret = TRANSACTION_GENERIC_ERROR;
-- 
2.11.0

[PATCH v3 20/30] packed_ref_store: make class into a subclass of `ref_store`

[Michael Haggerty]

Add the infrastructure to make `packed_ref_store` implement
`ref_store`, at least formally (few of the methods are actually
implemented yet). Change the functions in its interface to take
`ref_store *` arguments. Change `files_ref_store` to store a pointer
to `ref_store *` and to call functions via the virtual `ref_store`
interface where possible. This also means that a few
`packed_ref_store` functions can become static.

This is a work in progress. Some more `ref_store` methods will soon be
implemented (e.g., those having to do with reference transactions).
But some of them will never be implemented (e.g., those having to do
with symrefs or reflogs).

Signed-off-by: Michael Haggerty 
---
 refs/files-backend.c  |  16 ++--
 refs/packed-backend.c | 231 +-
 refs/packed-backend.h |  23 ++---
 refs/refs-internal.h  |   1 +
 4 files changed, 226 insertions(+), 45 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 7df9747798..60f4fa5e7a 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -28,7 +28,7 @@ struct files_ref_store {
 
struct ref_cache *loose;
 
-   struct packed_ref_store *packed_ref_store;
+   struct ref_store *packed_ref_store;
 };
 
 static void clear_loose_ref_cache(struct files_ref_store *refs)
@@ -311,8 +311,8 @@ static int files_read_raw_ref(struct ref_store *ref_store,
if (lstat(path, &st) < 0) {
if (errno != ENOENT)
goto out;
-   if (packed_read_raw_ref(refs->packed_ref_store, refname,
-   sha1, referent, type)) {
+   if (refs_read_raw_ref(refs->packed_ref_store, refname,
+ sha1, referent, type)) {
errno = ENOENT;
goto out;
}
@@ -351,8 +351,8 @@ static int files_read_raw_ref(struct ref_store *ref_store,
 * ref is supposed to be, there could still be a
 * packed ref:
 */
-   if (packed_read_raw_ref(refs->packed_ref_store, refname,
-   sha1, referent, type)) {
+   if (refs_read_raw_ref(refs->packed_ref_store, refname,
+ sha1, referent, type)) {
errno = EISDIR;
goto out;
}
@@ -683,7 +683,7 @@ static int files_peel_ref(struct ref_store *ref_store,
 * have REF_KNOWS_PEELED.
 */
if (flag & REF_ISPACKED &&
-   !packed_peel_ref(refs->packed_ref_store, refname, sha1))
+   !refs_peel_ref(refs->packed_ref_store, refname, sha1))
return 0;
 
return peel_object(base, sha1);
@@ -804,8 +804,8 @@ static struct ref_iterator *files_ref_iterator_begin(
 * ones in files_ref_iterator_advance(), after we have merged
 * the packed and loose references.
 */
-   packed_iter = packed_ref_iterator_begin(
-   refs->packed_ref_store, prefix,
+   packed_iter = refs_ref_iterator_begin(
+   refs->packed_ref_store, prefix, 0,
DO_FOR_EACH_INCLUDE_BROKEN);
 
iter->iter0 = overlay_ref_iterator_begin(loose_iter, packed_iter);
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index 6fa988c953..4676dc3959 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -50,6 +50,8 @@ static int release_packed_ref_cache(struct packed_ref_cache 
*packed_refs)
  * `ref_store`.
  */
 struct packed_ref_store {
+   struct ref_store base;
+
unsigned int store_flags;
 
/* The path of the "packed-refs" file: */
@@ -68,14 +70,17 @@ struct packed_ref_store {
struct lock_file lock;
 };
 
-struct packed_ref_store *packed_ref_store_create(
-   const char *path, unsigned int store_flags)
+struct ref_store *packed_ref_store_create(const char *path,
+ unsigned int store_flags)
 {
struct packed_ref_store *refs = xcalloc(1, sizeof(*refs));
+   struct ref_store *ref_store = (struct ref_store *)refs;
 
+   base_ref_store_init(ref_store, &refs_be_packed);
refs->store_flags = store_flags;
+
refs->path = xstrdup(path);
-   return refs;
+   return ref_store;
 }
 
 /*
@@ -91,6 +96,31 @@ static void packed_assert_main_repository(struct 
packed_ref_store *refs,
die("BUG: operation %s only allowed for main ref store", caller);
 }
 
+/*
+ * Downcast `ref_store` to `packed_ref_store`. Die if `ref_store` is
+ * not a `packed_ref_store`. Also die if `packed_ref_store` doesn't
+ * support at least the flags specified in `required_flags`. `caller`
+ * is used in any necessary error messages.
+ */
+static struct packed_ref_store *packed_downcast(struct ref_store *ref_store,
+   unsigned int required_flags,
+ 

[PATCH v3 13/30] rollback_packed_refs(): take a `packed_ref_store *` parameter

[Michael Haggerty]

It only cares about the packed-refs part of the reference store.

Signed-off-by: Michael Haggerty 
---
 refs/files-backend.c | 15 +++
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 5d159620f0..a08d3fbadf 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1434,18 +1434,17 @@ static int commit_packed_refs(struct packed_ref_store 
*refs)
  * in-memory packed reference cache.  (The packed-refs file will be
  * read anew if it is needed again after this function is called.)
  */
-static void rollback_packed_refs(struct files_ref_store *refs)
+static void rollback_packed_refs(struct packed_ref_store *refs)
 {
-   struct packed_ref_cache *packed_ref_cache =
-   get_packed_ref_cache(refs->packed_ref_store);
+   struct packed_ref_cache *packed_ref_cache = get_packed_ref_cache(refs);
 
-   files_assert_main_repository(refs, "rollback_packed_refs");
+   packed_assert_main_repository(refs, "rollback_packed_refs");
 
-   if (!is_lock_file_locked(&refs->packed_ref_store->lock))
+   if (!is_lock_file_locked(&refs->lock))
die("BUG: packed-refs not locked");
-   rollback_lock_file(&refs->packed_ref_store->lock);
+   rollback_lock_file(&refs->lock);
release_packed_ref_cache(packed_ref_cache);
-   clear_packed_ref_cache(refs->packed_ref_store);
+   clear_packed_ref_cache(refs);
 }
 
 struct ref_to_prune {
@@ -1657,7 +1656,7 @@ static int repack_without_refs(struct files_ref_store 
*refs,
 * All packed entries disappeared while we were
 * acquiring the lock.
 */
-   rollback_packed_refs(refs);
+   rollback_packed_refs(refs->packed_ref_store);
return 0;
}
 
-- 
2.11.0

[PATCH v3 30/30] read_packed_refs(): die if `packed-refs` contains bogus data

[Michael Haggerty]

The old code ignored any lines that it didn't understand, including
unterminated lines. This is dangerous. Instead, `die()` if the
`packed-refs` file contains any unterminated lines or lines that we
don't know how to handle.

This fixes the tests added in the last commit.

Signed-off-by: Michael Haggerty 
---
 refs/packed-backend.c | 10 +++---
 t/t3210-pack-refs.sh  |  6 +++---
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index 377c775adb..a28befbfa3 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -229,6 +229,9 @@ static struct packed_ref_cache *read_packed_refs(const char 
*packed_refs_file)
const char *refname;
const char *traits;
 
+   if (!line.len || line.buf[line.len - 1] != '\n')
+   die("unterminated line in %s: %s", packed_refs_file, 
line.buf);
+
if (skip_prefix(line.buf, "# pack-refs with:", &traits)) {
if (strstr(traits, " fully-peeled "))
peeled = PEELED_FULLY;
@@ -253,9 +256,7 @@ static struct packed_ref_cache *read_packed_refs(const char 
*packed_refs_file)
(peeled == PEELED_TAGS && starts_with(refname, 
"refs/tags/")))
last->flag |= REF_KNOWS_PEELED;
add_ref_entry(dir, last);
-   continue;
-   }
-   if (last &&
+   } else if (last &&
line.buf[0] == '^' &&
line.len == PEELED_LINE_LENGTH &&
line.buf[PEELED_LINE_LENGTH - 1] == '\n' &&
@@ -267,6 +268,9 @@ static struct packed_ref_cache *read_packed_refs(const char 
*packed_refs_file)
 * reference:
 */
last->flag |= REF_KNOWS_PEELED;
+   } else {
+   strbuf_setlen(&line, line.len - 1);
+   die("unexpected line in %s: %s", packed_refs_file, 
line.buf);
}
}
 
diff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh
index 4b65836283..2bb4b25ed9 100755
--- a/t/t3210-pack-refs.sh
+++ b/t/t3210-pack-refs.sh
@@ -194,7 +194,7 @@ test_expect_success 'notice d/f conflict with existing ref' 
'
test_must_fail git branch foo/bar/baz/lots/of/extra/components
 '
 
-test_expect_failure 'reject packed-refs with unterminated line' '
+test_expect_success 'reject packed-refs with unterminated line' '
cp .git/packed-refs .git/packed-refs.bak &&
test_when_finished "mv .git/packed-refs.bak .git/packed-refs" &&
printf "%s" "$HEAD refs/z" >>.git/packed-refs &&
@@ -203,7 +203,7 @@ test_expect_failure 'reject packed-refs with unterminated 
line' '
test_cmp expected_err err
 '
 
-test_expect_failure 'reject packed-refs containing junk' '
+test_expect_success 'reject packed-refs containing junk' '
cp .git/packed-refs .git/packed-refs.bak &&
test_when_finished "mv .git/packed-refs.bak .git/packed-refs" &&
printf "%s\n" "bogus content" >>.git/packed-refs &&
@@ -212,7 +212,7 @@ test_expect_failure 'reject packed-refs containing junk' '
test_cmp expected_err err
 '
 
-test_expect_failure 'reject packed-refs with a short SHA-1' '
+test_expect_success 'reject packed-refs with a short SHA-1' '
cp .git/packed-refs .git/packed-refs.bak &&
test_when_finished "mv .git/packed-refs.bak .git/packed-refs" &&
printf "%.7s %s\n" $HEAD refs/z >>.git/packed-refs &&
-- 
2.11.0

[PATCH v3 21/30] commit_packed_refs(): report errors rather than dying

[Michael Haggerty]

Report errors via a `struct strbuf *err` rather than by calling
`die()`. To enable this goal, change `write_packed_entry()` to report
errors via a return value and `errno` rather than dying.

Signed-off-by: Michael Haggerty 
---
 refs/files-backend.c  | 10 +++---
 refs/packed-backend.c | 85 +--
 refs/packed-backend.h |  2 +-
 3 files changed, 61 insertions(+), 36 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 60f4fa5e7a..2810785efc 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1094,6 +1094,7 @@ static int files_pack_refs(struct ref_store *ref_store, 
unsigned int flags)
struct ref_iterator *iter;
int ok;
struct ref_to_prune *refs_to_prune = NULL;
+   struct strbuf err = STRBUF_INIT;
 
lock_packed_refs(refs->packed_ref_store, LOCK_DIE_ON_ERROR);
 
@@ -1128,10 +1129,11 @@ static int files_pack_refs(struct ref_store *ref_store, 
unsigned int flags)
if (ok != ITER_DONE)
die("error while iterating over references");
 
-   if (commit_packed_refs(refs->packed_ref_store))
-   die_errno("unable to overwrite old ref-pack file");
+   if (commit_packed_refs(refs->packed_ref_store, &err))
+   die("unable to overwrite old ref-pack file: %s", err.buf);
 
prune_refs(refs, refs_to_prune);
+   strbuf_release(&err);
return 0;
 }
 
@@ -2693,9 +2695,7 @@ static int files_initial_transaction_commit(struct 
ref_store *ref_store,
   &update->new_oid);
}
 
-   if (commit_packed_refs(refs->packed_ref_store)) {
-   strbuf_addf(err, "unable to commit packed-refs file: %s",
-   strerror(errno));
+   if (commit_packed_refs(refs->packed_ref_store, err)) {
ret = TRANSACTION_GENERIC_ERROR;
goto cleanup;
}
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index 4676dc3959..18ce47fcb7 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -493,15 +493,19 @@ static struct ref_iterator *packed_ref_iterator_begin(
 
 /*
  * Write an entry to the packed-refs file for the specified refname.
- * If peeled is non-NULL, write it as the entry's peeled value.
+ * If peeled is non-NULL, write it as the entry's peeled value. On
+ * error, return a nonzero value and leave errno set at the value left
+ * by the failing call to `fprintf()`.
  */
-static void write_packed_entry(FILE *fh, const char *refname,
-  const unsigned char *sha1,
-  const unsigned char *peeled)
+static int write_packed_entry(FILE *fh, const char *refname,
+ const unsigned char *sha1,
+ const unsigned char *peeled)
 {
-   fprintf_or_die(fh, "%s %s\n", sha1_to_hex(sha1), refname);
-   if (peeled)
-   fprintf_or_die(fh, "^%s\n", sha1_to_hex(peeled));
+   if (fprintf(fh, "%s %s\n", sha1_to_hex(sha1), refname) < 0 ||
+   (peeled && fprintf(fh, "^%s\n", sha1_to_hex(peeled)) < 0))
+   return -1;
+
+   return 0;
 }
 
 int lock_packed_refs(struct ref_store *ref_store, int flags)
@@ -550,49 +554,74 @@ static const char PACKED_REFS_HEADER[] =
 /*
  * Write the current version of the packed refs cache from memory to
  * disk. The packed-refs file must already be locked for writing (see
- * lock_packed_refs()). Return zero on success. On errors, set errno
- * and return a nonzero value.
+ * lock_packed_refs()). Return zero on success. On errors, rollback
+ * the lockfile, write an error message to `err`, and return a nonzero
+ * value.
  */
-int commit_packed_refs(struct ref_store *ref_store)
+int commit_packed_refs(struct ref_store *ref_store, struct strbuf *err)
 {
struct packed_ref_store *refs =
packed_downcast(ref_store, REF_STORE_WRITE | REF_STORE_MAIN,
"commit_packed_refs");
struct packed_ref_cache *packed_ref_cache =
get_packed_ref_cache(refs);
-   int ok, error = 0;
-   int save_errno = 0;
+   int ok;
+   int ret = -1;
FILE *out;
struct ref_iterator *iter;
 
if (!is_lock_file_locked(&refs->lock))
-   die("BUG: packed-refs not locked");
+   die("BUG: commit_packed_refs() called when unlocked");
 
out = fdopen_lock_file(&refs->lock, "w");
-   if (!out)
-   die_errno("unable to fdopen packed-refs descriptor");
+   if (!out) {
+   strbuf_addf(err, "unable to fdopen packed-refs tempfile: %s",
+   strerror(errno));
+   goto error;
+   }
 
-   fprintf_or_die(out, "%s", PACKED_REFS_HEADER);
+   if (fprintf(out, "%s", PACKED_REFS_HEADER) < 0) {
+   strbuf_addf(err, "error writing to %s: %s",
+   get_lock_file_path(&refs->lock

[PATCH v3 18/30] packed_read_raw_ref(): new function, replacing `resolve_packed_ref()`

[Michael Haggerty]

Add a new function, `packed_read_raw_ref()`, which is nearly a
`read_raw_ref_fn`. Use it in place of `resolve_packed_ref()`.

Signed-off-by: Michael Haggerty 
---
 refs/files-backend.c | 36 +---
 1 file changed, 17 insertions(+), 19 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 0490cc087e..346794cf7c 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -608,27 +608,23 @@ static struct ref_entry *get_packed_ref(struct 
packed_ref_store *refs,
return find_ref_entry(get_packed_refs(refs), refname);
 }
 
-/*
- * A loose ref file doesn't exist; check for a packed ref.
- */
-static int resolve_packed_ref(struct files_ref_store *refs,
- const char *refname,
- unsigned char *sha1, unsigned int *flags)
+static int packed_read_raw_ref(struct packed_ref_store *refs,
+  const char *refname, unsigned char *sha1,
+  struct strbuf *referent, unsigned int *type)
 {
struct ref_entry *entry;
 
-   /*
-* The loose reference file does not exist; check for a packed
-* reference.
-*/
-   entry = get_packed_ref(refs->packed_ref_store, refname);
-   if (entry) {
-   hashcpy(sha1, entry->u.value.oid.hash);
-   *flags |= REF_ISPACKED;
-   return 0;
+   *type = 0;
+
+   entry = get_packed_ref(refs, refname);
+   if (!entry) {
+   errno = ENOENT;
+   return -1;
}
-   /* refname is not a packed reference. */
-   return -1;
+
+   hashcpy(sha1, entry->u.value.oid.hash);
+   *type = REF_ISPACKED;
+   return 0;
 }
 
 static int files_read_raw_ref(struct ref_store *ref_store,
@@ -674,7 +670,8 @@ static int files_read_raw_ref(struct ref_store *ref_store,
if (lstat(path, &st) < 0) {
if (errno != ENOENT)
goto out;
-   if (resolve_packed_ref(refs, refname, sha1, type)) {
+   if (packed_read_raw_ref(refs->packed_ref_store, refname,
+   sha1, referent, type)) {
errno = ENOENT;
goto out;
}
@@ -713,7 +710,8 @@ static int files_read_raw_ref(struct ref_store *ref_store,
 * ref is supposed to be, there could still be a
 * packed ref:
 */
-   if (resolve_packed_ref(refs, refname, sha1, type)) {
+   if (packed_read_raw_ref(refs->packed_ref_store, refname,
+   sha1, referent, type)) {
errno = EISDIR;
goto out;
}
-- 
2.11.0

[PATCH v3 29/30] t3210: add some tests of bogus packed-refs file contents

[Michael Haggerty]

If `packed-refs` contains indecipherable lines, we should emit an
error and quit rather than just skipping the lines. Unfortunately, we
currently do the latter. Add some failing tests demonstrating the
problem.

This will be fixed in the next commit.

Signed-off-by: Michael Haggerty 
---
 t/t3210-pack-refs.sh | 27 +++
 1 file changed, 27 insertions(+)

diff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh
index 9b182a0c32..4b65836283 100755
--- a/t/t3210-pack-refs.sh
+++ b/t/t3210-pack-refs.sh
@@ -194,6 +194,33 @@ test_expect_success 'notice d/f conflict with existing 
ref' '
test_must_fail git branch foo/bar/baz/lots/of/extra/components
 '
 
+test_expect_failure 'reject packed-refs with unterminated line' '
+   cp .git/packed-refs .git/packed-refs.bak &&
+   test_when_finished "mv .git/packed-refs.bak .git/packed-refs" &&
+   printf "%s" "$HEAD refs/z" >>.git/packed-refs &&
+   echo "fatal: unterminated line in .git/packed-refs: $HEAD refs/z" 
>expected_err &&
+   test_must_fail git for-each-ref >out 2>err &&
+   test_cmp expected_err err
+'
+
+test_expect_failure 'reject packed-refs containing junk' '
+   cp .git/packed-refs .git/packed-refs.bak &&
+   test_when_finished "mv .git/packed-refs.bak .git/packed-refs" &&
+   printf "%s\n" "bogus content" >>.git/packed-refs &&
+   echo "fatal: unexpected line in .git/packed-refs: bogus content" 
>expected_err &&
+   test_must_fail git for-each-ref >out 2>err &&
+   test_cmp expected_err err
+'
+
+test_expect_failure 'reject packed-refs with a short SHA-1' '
+   cp .git/packed-refs .git/packed-refs.bak &&
+   test_when_finished "mv .git/packed-refs.bak .git/packed-refs" &&
+   printf "%.7s %s\n" $HEAD refs/z >>.git/packed-refs &&
+   printf "fatal: unexpected line in .git/packed-refs: %.7s %s\n" $HEAD 
refs/z >expected_err &&
+   test_must_fail git for-each-ref >out 2>err &&
+   test_cmp expected_err err
+'
+
 test_expect_success 'timeout if packed-refs.lock exists' '
LOCK=.git/packed-refs.lock &&
>"$LOCK" &&
-- 
2.11.0

[PATCH v3 26/30] clear_packed_ref_cache(): don't protest if the lock is held

[Michael Haggerty]

The existing callers already check that the lock isn't held just
before calling `clear_packed_ref_cache()`, and in the near future we
want to be able to call this function when the lock is held.

Signed-off-by: Michael Haggerty 
---
 refs/packed-backend.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index f27943f9a1..96d92a5eea 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -133,8 +133,6 @@ static void clear_packed_ref_cache(struct packed_ref_store 
*refs)
if (refs->cache) {
struct packed_ref_cache *cache = refs->cache;
 
-   if (is_lock_file_locked(&refs->lock))
-   die("BUG: packed-ref cache cleared while locked");
refs->cache = NULL;
release_packed_ref_cache(cache);
}
-- 
2.11.0

[PATCH v3 22/30] commit_packed_refs(): use a staging file separate from the lockfile

[Michael Haggerty]

We will want to be able to hold the lockfile for `packed-refs` even
after we have activated the new values. So use a separate tempfile,
`packed-refs.new`, as a place to stage the new contents of the
`packed-refs` file. For now this is all done within
`commit_packed_refs()`, but that will change shortly.

Signed-off-by: Michael Haggerty 
---
 refs/packed-backend.c | 40 
 1 file changed, 32 insertions(+), 8 deletions(-)

diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index 18ce47fcb7..71f92ed6f0 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -68,6 +68,13 @@ struct packed_ref_store {
 * thus the enclosing `packed_ref_store`) must not be freed.
 */
struct lock_file lock;
+
+   /*
+* Temporary file used when rewriting new contents to the
+* "packed-refs" file. Note that this (and thus the enclosing
+* `packed_ref_store`) must not be freed.
+*/
+   struct tempfile tempfile;
 };
 
 struct ref_store *packed_ref_store_create(const char *path,
@@ -522,10 +529,16 @@ int lock_packed_refs(struct ref_store *ref_store, int 
flags)
timeout_configured = 1;
}
 
+   /*
+* Note that we close the lockfile immediately because we
+* don't write new content to it, but rather to a separate
+* tempfile.
+*/
if (hold_lock_file_for_update_timeout(
&refs->lock,
refs->path,
-   flags, timeout_value) < 0)
+   flags, timeout_value) < 0 ||
+   close_lock_file(&refs->lock))
return -1;
 
/*
@@ -567,13 +580,23 @@ int commit_packed_refs(struct ref_store *ref_store, 
struct strbuf *err)
get_packed_ref_cache(refs);
int ok;
int ret = -1;
+   struct strbuf sb = STRBUF_INIT;
FILE *out;
struct ref_iterator *iter;
 
if (!is_lock_file_locked(&refs->lock))
die("BUG: commit_packed_refs() called when unlocked");
 
-   out = fdopen_lock_file(&refs->lock, "w");
+   strbuf_addf(&sb, "%s.new", refs->path);
+   if (create_tempfile(&refs->tempfile, sb.buf) < 0) {
+   strbuf_addf(err, "unable to create file %s: %s",
+   sb.buf, strerror(errno));
+   strbuf_release(&sb);
+   goto out;
+   }
+   strbuf_release(&sb);
+
+   out = fdopen_tempfile(&refs->tempfile, "w");
if (!out) {
strbuf_addf(err, "unable to fdopen packed-refs tempfile: %s",
strerror(errno));
@@ -582,7 +605,7 @@ int commit_packed_refs(struct ref_store *ref_store, struct 
strbuf *err)
 
if (fprintf(out, "%s", PACKED_REFS_HEADER) < 0) {
strbuf_addf(err, "error writing to %s: %s",
-   get_lock_file_path(&refs->lock), strerror(errno));
+   get_tempfile_path(&refs->tempfile), 
strerror(errno));
goto error;
}
 
@@ -594,7 +617,7 @@ int commit_packed_refs(struct ref_store *ref_store, struct 
strbuf *err)
if (write_packed_entry(out, iter->refname, iter->oid->hash,
   peel_error ? NULL : peeled.hash)) {
strbuf_addf(err, "error writing to %s: %s",
-   get_lock_file_path(&refs->lock),
+   get_tempfile_path(&refs->tempfile),
strerror(errno));
ref_iterator_abort(iter);
goto error;
@@ -602,13 +625,13 @@ int commit_packed_refs(struct ref_store *ref_store, 
struct strbuf *err)
}
 
if (ok != ITER_DONE) {
-   strbuf_addf(err, "unable to write packed-refs file: "
+   strbuf_addf(err, "unable to rewrite packed-refs file: "
"error iterating over old contents");
goto error;
}
 
-   if (commit_lock_file(&refs->lock)) {
-   strbuf_addf(err, "error overwriting %s: %s",
+   if (rename_tempfile(&refs->tempfile, refs->path)) {
+   strbuf_addf(err, "error replacing %s: %s",
refs->path, strerror(errno));
goto out;
}
@@ -617,9 +640,10 @@ int commit_packed_refs(struct ref_store *ref_store, struct 
strbuf *err)
goto out;
 
 error:
-   rollback_lock_file(&refs->lock);
+   delete_tempfile(&refs->tempfile);
 
 out:
+   rollback_lock_file(&refs->lock);
release_packed_ref_cache(packed_ref_cache);
return ret;
 }
-- 
2.11.0

[PATCH v3 23/30] packed_refs_lock(): function renamed from lock_packed_refs()

[Michael Haggerty]

Rename `lock_packed_refs()` to `packed_refs_lock()` for consistency
with how other methods are named. Also, it's about to get some
companions.

Signed-off-by: Michael Haggerty 
---
 refs/files-backend.c  |  4 ++--
 refs/packed-backend.c | 10 +-
 refs/packed-backend.h |  2 +-
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 2810785efc..88de907148 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1096,7 +1096,7 @@ static int files_pack_refs(struct ref_store *ref_store, 
unsigned int flags)
struct ref_to_prune *refs_to_prune = NULL;
struct strbuf err = STRBUF_INIT;
 
-   lock_packed_refs(refs->packed_ref_store, LOCK_DIE_ON_ERROR);
+   packed_refs_lock(refs->packed_ref_store, LOCK_DIE_ON_ERROR);
 
iter = cache_ref_iterator_begin(get_loose_ref_cache(refs), NULL, 0);
while ((ok = ref_iterator_advance(iter)) == ITER_OK) {
@@ -2679,7 +2679,7 @@ static int files_initial_transaction_commit(struct 
ref_store *ref_store,
}
}
 
-   if (lock_packed_refs(refs->packed_ref_store, 0)) {
+   if (packed_refs_lock(refs->packed_ref_store, 0)) {
strbuf_addf(err, "unable to lock packed-refs file: %s",
strerror(errno));
ret = TRANSACTION_GENERIC_ERROR;
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index 71f92ed6f0..cd214e07a1 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -321,7 +321,7 @@ static struct ref_dir *get_packed_refs(struct 
packed_ref_store *refs)
 /*
  * Add or overwrite a reference in the in-memory packed reference
  * cache. This may only be called while the packed-refs file is locked
- * (see lock_packed_refs()). To actually write the packed-refs file,
+ * (see packed_refs_lock()). To actually write the packed-refs file,
  * call commit_packed_refs().
  */
 void add_packed_ref(struct ref_store *ref_store,
@@ -515,11 +515,11 @@ static int write_packed_entry(FILE *fh, const char 
*refname,
return 0;
 }
 
-int lock_packed_refs(struct ref_store *ref_store, int flags)
+int packed_refs_lock(struct ref_store *ref_store, int flags)
 {
struct packed_ref_store *refs =
packed_downcast(ref_store, REF_STORE_WRITE | REF_STORE_MAIN,
-   "lock_packed_refs");
+   "packed_refs_lock");
static int timeout_configured = 0;
static int timeout_value = 1000;
struct packed_ref_cache *packed_ref_cache;
@@ -567,7 +567,7 @@ static const char PACKED_REFS_HEADER[] =
 /*
  * Write the current version of the packed refs cache from memory to
  * disk. The packed-refs file must already be locked for writing (see
- * lock_packed_refs()). Return zero on success. On errors, rollback
+ * packed_refs_lock()). Return zero on success. On errors, rollback
  * the lockfile, write an error message to `err`, and return a nonzero
  * value.
  */
@@ -698,7 +698,7 @@ int repack_without_refs(struct ref_store *ref_store,
if (!needs_repacking)
return 0; /* no refname exists in packed refs */
 
-   if (lock_packed_refs(&refs->base, 0)) {
+   if (packed_refs_lock(&refs->base, 0)) {
unable_to_lock_message(refs->path, errno, err);
return -1;
}
diff --git a/refs/packed-backend.h b/refs/packed-backend.h
index 3d4057b65b..dbc00d3396 100644
--- a/refs/packed-backend.h
+++ b/refs/packed-backend.h
@@ -9,7 +9,7 @@ struct ref_store *packed_ref_store_create(const char *path,
  * hold_lock_file_for_update(). Return 0 on success. On errors, set
  * errno appropriately and return a nonzero value.
  */
-int lock_packed_refs(struct ref_store *ref_store, int flags);
+int packed_refs_lock(struct ref_store *ref_store, int flags);
 
 void add_packed_ref(struct ref_store *ref_store,
const char *refname, const struct object_id *oid);
-- 
2.11.0

[PATCH v3 24/30] packed_refs_lock(): report errors via a `struct strbuf *err`

[Michael Haggerty]

That way the callers don't have to come up with error messages
themselves.

Signed-off-by: Michael Haggerty 
---
 refs/files-backend.c  |  6 ++
 refs/packed-backend.c | 17 +++--
 refs/packed-backend.h |  6 +++---
 3 files changed, 16 insertions(+), 13 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 88de907148..8ea4e9ab05 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1096,7 +1096,7 @@ static int files_pack_refs(struct ref_store *ref_store, 
unsigned int flags)
struct ref_to_prune *refs_to_prune = NULL;
struct strbuf err = STRBUF_INIT;
 
-   packed_refs_lock(refs->packed_ref_store, LOCK_DIE_ON_ERROR);
+   packed_refs_lock(refs->packed_ref_store, LOCK_DIE_ON_ERROR, &err);
 
iter = cache_ref_iterator_begin(get_loose_ref_cache(refs), NULL, 0);
while ((ok = ref_iterator_advance(iter)) == ITER_OK) {
@@ -2679,9 +2679,7 @@ static int files_initial_transaction_commit(struct 
ref_store *ref_store,
}
}
 
-   if (packed_refs_lock(refs->packed_ref_store, 0)) {
-   strbuf_addf(err, "unable to lock packed-refs file: %s",
-   strerror(errno));
+   if (packed_refs_lock(refs->packed_ref_store, 0, err)) {
ret = TRANSACTION_GENERIC_ERROR;
goto cleanup;
}
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index cd214e07a1..78e877a9e3 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -515,7 +515,7 @@ static int write_packed_entry(FILE *fh, const char *refname,
return 0;
 }
 
-int packed_refs_lock(struct ref_store *ref_store, int flags)
+int packed_refs_lock(struct ref_store *ref_store, int flags, struct strbuf 
*err)
 {
struct packed_ref_store *refs =
packed_downcast(ref_store, REF_STORE_WRITE | REF_STORE_MAIN,
@@ -537,9 +537,15 @@ int packed_refs_lock(struct ref_store *ref_store, int 
flags)
if (hold_lock_file_for_update_timeout(
&refs->lock,
refs->path,
-   flags, timeout_value) < 0 ||
-   close_lock_file(&refs->lock))
+   flags, timeout_value) < 0) {
+   unable_to_lock_message(refs->path, errno, err);
+   return -1;
+   }
+
+   if (close_lock_file(&refs->lock)) {
+   strbuf_addf(err, "unable to close %s: %s", refs->path, 
strerror(errno));
return -1;
+   }
 
/*
 * Now that we hold the `packed-refs` lock, make sure that our
@@ -698,10 +704,9 @@ int repack_without_refs(struct ref_store *ref_store,
if (!needs_repacking)
return 0; /* no refname exists in packed refs */
 
-   if (packed_refs_lock(&refs->base, 0)) {
-   unable_to_lock_message(refs->path, errno, err);
+   if (packed_refs_lock(&refs->base, 0, err))
return -1;
-   }
+
packed = get_packed_refs(refs);
 
/* Remove refnames from the cache */
diff --git a/refs/packed-backend.h b/refs/packed-backend.h
index dbc00d3396..210e3f35ce 100644
--- a/refs/packed-backend.h
+++ b/refs/packed-backend.h
@@ -6,10 +6,10 @@ struct ref_store *packed_ref_store_create(const char *path,
 
 /*
  * Lock the packed-refs file for writing. Flags is passed to
- * hold_lock_file_for_update(). Return 0 on success. On errors, set
- * errno appropriately and return a nonzero value.
+ * hold_lock_file_for_update(). Return 0 on success. On errors, write
+ * an error message to `err` and return a nonzero value.
  */
-int packed_refs_lock(struct ref_store *ref_store, int flags);
+int packed_refs_lock(struct ref_store *ref_store, int flags, struct strbuf 
*err);
 
 void add_packed_ref(struct ref_store *ref_store,
const char *refname, const struct object_id *oid);
-- 
2.11.0

[PATCH v3 28/30] repack_without_refs(): don't lock or unlock the packed refs

[Michael Haggerty]

Change `repack_without_refs()` to expect the packed-refs lock to be
held already, and not to release the lock before returning. Change the
callers to deal with lock management.

This change makes it possible for callers to hold the packed-refs lock
for a longer span of time, a possibility that will eventually make it
possible to fix some longstanding races.

The only semantic change here is that `repack_without_refs()` used to
forget to release the lock in the `if (!removed)` exit path. That
omission is now fixed.

Signed-off-by: Michael Haggerty 
---
 refs/files-backend.c  | 47 +++
 refs/packed-backend.c | 32 
 2 files changed, 39 insertions(+), 40 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 93bdc8f0c8..e9b95592b6 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1149,24 +1149,16 @@ static int files_delete_refs(struct ref_store 
*ref_store, const char *msg,
if (!refnames->nr)
return 0;
 
-   result = repack_without_refs(refs->packed_ref_store, refnames, &err);
-   if (result) {
-   /*
-* If we failed to rewrite the packed-refs file, then
-* it is unsafe to try to remove loose refs, because
-* doing so might expose an obsolete packed value for
-* a reference that might even point at an object that
-* has been garbage collected.
-*/
-   if (refnames->nr == 1)
-   error(_("could not delete reference %s: %s"),
- refnames->items[0].string, err.buf);
-   else
-   error(_("could not delete references: %s"), err.buf);
+   if (packed_refs_lock(refs->packed_ref_store, 0, &err))
+   goto error;
 
-   goto out;
+   if (repack_without_refs(refs->packed_ref_store, refnames, &err)) {
+   packed_refs_unlock(refs->packed_ref_store);
+   goto error;
}
 
+   packed_refs_unlock(refs->packed_ref_store);
+
for (i = 0; i < refnames->nr; i++) {
const char *refname = refnames->items[i].string;
 
@@ -1174,9 +1166,24 @@ static int files_delete_refs(struct ref_store 
*ref_store, const char *msg,
result |= error(_("could not remove reference %s"), 
refname);
}
 
-out:
strbuf_release(&err);
return result;
+
+error:
+   /*
+* If we failed to rewrite the packed-refs file, then it is
+* unsafe to try to remove loose refs, because doing so might
+* expose an obsolete packed value for a reference that might
+* even point at an object that has been garbage collected.
+*/
+   if (refnames->nr == 1)
+   error(_("could not delete reference %s: %s"),
+ refnames->items[0].string, err.buf);
+   else
+   error(_("could not delete references: %s"), err.buf);
+
+   strbuf_release(&err);
+   return -1;
 }
 
 /*
@@ -2569,11 +2576,19 @@ static int files_transaction_finish(struct ref_store 
*ref_store,
}
}
 
+   if (packed_refs_lock(refs->packed_ref_store, 0, err)) {
+   ret = TRANSACTION_GENERIC_ERROR;
+   goto cleanup;
+   }
+
if (repack_without_refs(refs->packed_ref_store, &refs_to_delete, err)) {
ret = TRANSACTION_GENERIC_ERROR;
+   packed_refs_unlock(refs->packed_ref_store);
goto cleanup;
}
 
+   packed_refs_unlock(refs->packed_ref_store);
+
/* Delete the reflogs of any references that were deleted: */
for_each_string_list_item(ref_to_delete, &refs_to_delete) {
strbuf_reset(&sb);
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index 5cf6b3d40e..377c775adb 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -669,25 +669,12 @@ int commit_packed_refs(struct ref_store *ref_store, 
struct strbuf *err)
return -1;
 }
 
-/*
- * Rollback the lockfile for the packed-refs file, and discard the
- * in-memory packed reference cache.  (The packed-refs file will be
- * read anew if it is needed again after this function is called.)
- */
-static void rollback_packed_refs(struct packed_ref_store *refs)
-{
-   packed_assert_main_repository(refs, "rollback_packed_refs");
-
-   if (!is_lock_file_locked(&refs->lock))
-   die("BUG: packed-refs not locked");
-   packed_refs_unlock(&refs->base);
-   clear_packed_ref_cache(refs);
-}
-
 /*
  * Rewrite the packed-refs file, omitting any refs listed in
  * 'refnames'. On error, leave packed-refs unchanged, write an error
- * message to 'err', and return a nonzero value.
+ * message to 'err', and return a nonzero value. The packed refs lock
+ * must be held when calling this function; it will still be held when
+ * the function r

[PATCH v3 27/30] commit_packed_refs(): remove call to `packed_refs_unlock()`

[Michael Haggerty]

Instead, change the callers of `commit_packed_refs()` to call
`packed_refs_unlock()`.

Signed-off-by: Michael Haggerty 
---
 refs/files-backend.c  |  2 ++
 refs/packed-backend.c | 18 --
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 8ea4e9ab05..93bdc8f0c8 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1131,6 +1131,7 @@ static int files_pack_refs(struct ref_store *ref_store, 
unsigned int flags)
 
if (commit_packed_refs(refs->packed_ref_store, &err))
die("unable to overwrite old ref-pack file: %s", err.buf);
+   packed_refs_unlock(refs->packed_ref_store);
 
prune_refs(refs, refs_to_prune);
strbuf_release(&err);
@@ -2699,6 +2700,7 @@ static int files_initial_transaction_commit(struct 
ref_store *ref_store,
}
 
 cleanup:
+   packed_refs_unlock(refs->packed_ref_store);
transaction->state = REF_TRANSACTION_CLOSED;
string_list_clear(&affected_refnames, 0);
return ret;
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index 96d92a5eea..5cf6b3d40e 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -606,7 +606,6 @@ int commit_packed_refs(struct ref_store *ref_store, struct 
strbuf *err)
struct packed_ref_cache *packed_ref_cache =
get_packed_ref_cache(refs);
int ok;
-   int ret = -1;
struct strbuf sb = STRBUF_INIT;
FILE *out;
struct ref_iterator *iter;
@@ -619,7 +618,7 @@ int commit_packed_refs(struct ref_store *ref_store, struct 
strbuf *err)
strbuf_addf(err, "unable to create file %s: %s",
sb.buf, strerror(errno));
strbuf_release(&sb);
-   goto out;
+   return -1;
}
strbuf_release(&sb);
 
@@ -660,18 +659,14 @@ int commit_packed_refs(struct ref_store *ref_store, 
struct strbuf *err)
if (rename_tempfile(&refs->tempfile, refs->path)) {
strbuf_addf(err, "error replacing %s: %s",
refs->path, strerror(errno));
-   goto out;
+   return -1;
}
 
-   ret = 0;
-   goto out;
+   return 0;
 
 error:
delete_tempfile(&refs->tempfile);
-
-out:
-   packed_refs_unlock(ref_store);
-   return ret;
+   return -1;
 }
 
 /*
@@ -705,6 +700,7 @@ int repack_without_refs(struct ref_store *ref_store,
struct ref_dir *packed;
struct string_list_item *refname;
int needs_repacking = 0, removed = 0;
+   int ret;
 
packed_assert_main_repository(refs, "repack_without_refs");
assert(err);
@@ -740,7 +736,9 @@ int repack_without_refs(struct ref_store *ref_store,
}
 
/* Write what remains */
-   return commit_packed_refs(&refs->base, err);
+   ret = commit_packed_refs(&refs->base, err);
+   packed_refs_unlock(ref_store);
+   return ret;
 }
 
 static int packed_init_db(struct ref_store *ref_store, struct strbuf *err)
-- 
2.11.0

[PATCH v3 25/30] packed_refs_unlock(), packed_refs_is_locked(): new functions

[Michael Haggerty]

Add two new public functions, `packed_refs_unlock()` and
`packed_refs_is_locked()`, with which callers can manage and query the
`packed-refs` lock externally.

Call `packed_refs_unlock()` from `commit_packed_refs()` and
`rollback_packed_refs()`.

Signed-off-by: Michael Haggerty 
---
 refs/packed-backend.c | 31 +--
 refs/packed-backend.h |  3 +++
 2 files changed, 28 insertions(+), 6 deletions(-)

diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index 78e877a9e3..f27943f9a1 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -563,6 +563,29 @@ int packed_refs_lock(struct ref_store *ref_store, int 
flags, struct strbuf *err)
return 0;
 }
 
+void packed_refs_unlock(struct ref_store *ref_store)
+{
+   struct packed_ref_store *refs = packed_downcast(
+   ref_store,
+   REF_STORE_READ | REF_STORE_WRITE,
+   "packed_refs_unlock");
+
+   if (!is_lock_file_locked(&refs->lock))
+   die("BUG: packed_refs_unlock() called when not locked");
+   rollback_lock_file(&refs->lock);
+   release_packed_ref_cache(refs->cache);
+}
+
+int packed_refs_is_locked(struct ref_store *ref_store)
+{
+   struct packed_ref_store *refs = packed_downcast(
+   ref_store,
+   REF_STORE_READ | REF_STORE_WRITE,
+   "packed_refs_is_locked");
+
+   return is_lock_file_locked(&refs->lock);
+}
+
 /*
  * The packed-refs header line that we write out.  Perhaps other
  * traits will be added later.  The trailing space is required.
@@ -649,8 +672,7 @@ int commit_packed_refs(struct ref_store *ref_store, struct 
strbuf *err)
delete_tempfile(&refs->tempfile);
 
 out:
-   rollback_lock_file(&refs->lock);
-   release_packed_ref_cache(packed_ref_cache);
+   packed_refs_unlock(ref_store);
return ret;
 }
 
@@ -661,14 +683,11 @@ int commit_packed_refs(struct ref_store *ref_store, 
struct strbuf *err)
  */
 static void rollback_packed_refs(struct packed_ref_store *refs)
 {
-   struct packed_ref_cache *packed_ref_cache = get_packed_ref_cache(refs);
-
packed_assert_main_repository(refs, "rollback_packed_refs");
 
if (!is_lock_file_locked(&refs->lock))
die("BUG: packed-refs not locked");
-   rollback_lock_file(&refs->lock);
-   release_packed_ref_cache(packed_ref_cache);
+   packed_refs_unlock(&refs->base);
clear_packed_ref_cache(refs);
 }
 
diff --git a/refs/packed-backend.h b/refs/packed-backend.h
index 210e3f35ce..03b7c1de95 100644
--- a/refs/packed-backend.h
+++ b/refs/packed-backend.h
@@ -11,6 +11,9 @@ struct ref_store *packed_ref_store_create(const char *path,
  */
 int packed_refs_lock(struct ref_store *ref_store, int flags, struct strbuf 
*err);
 
+void packed_refs_unlock(struct ref_store *ref_store);
+int packed_refs_is_locked(struct ref_store *ref_store);
+
 void add_packed_ref(struct ref_store *ref_store,
const char *refname, const struct object_id *oid);
 
-- 
2.11.0

Re: [PATCH] hooks: add signature to the top of the commit message

[Philip Oakley]

From: "Junio C Hamano" 

Kaartic Sivaraam  writes:


By the way, the one that is still actually enabled is no longer
needed.  The commit template generated internally was corrected some
time ago not to add the "Conflicts:" section without commenting it
out.


I'll send in another patch that removes it but it seems removing it
would leave sample hook without anything turned on by default. That
doesn't sound fine, does it?


Actually I was wondering if it is a good idea to remove it, as it
seems to have outlived its usefulness.


Personally, I like the comfort of seeing the Conflicts: list, but if others 
have indicated otherwise...

--
Philip 

Re: [PATCH] hooks: add signature to the top of the commit message

[Kaartic Sivaraam]

On Sat, 2017-07-01 at 10:36 -0700, Junio C Hamano wrote:
> Actually I was wondering if it is a good idea to remove it, as it
> seems to have outlived its usefulness.
It does seem  to be a good idea but it would leave the 'prepare-commit-
msg' hook with no scripts that could be used by just activating it.
That's why I thought of adding a script that removes the "Please enter
your.." message from the comments if it exists. A typical patch will
follow.

Enabling the "prepare-commit-msg" hook with the patch that follows
would do have following result,

Before,

# Please enter the commit message for your changes. Lines starting
# with '#' will be ignored, and an empty message aborts the commit.
# On branch hook-test-merge
# Changes to be committed:
#   new file:   commit-msg
#

After,


# On branch hook-test-merge
# Changes to be committed:
#   new file:   commit-msg
#


A typical consequence for "git commit --amend" would be,

Before,

Test commit

# Please enter the commit message for your changes. Lines starting
# with '#' will be ignored, and an empty message aborts the commit.
#
# Date:  Sun Jul 2 00:11:28 2017 +0530


After,

Test commit

#
# Date:  Sun Jul 2 00:11:28 2017 +0530
#

Re: [RFC/PATCH v4 00/49] Add initial experimental external ODB support

[Christian Couder]

On Fri, Jun 23, 2017 at 8:24 PM, Ben Peart  wrote:
>
>
> On 6/20/2017 3:54 AM, Christian Couder wrote:

>> To be able to better handle some kind of objects, for example big
>> blobs, it would be nice if Git could store its objects in other object
>> databases (ODB).
>>
>> To do that, this patch series makes it possible to register commands,
>> also called "helpers", using "odb..command" config variables,
>> to access external ODBs where objects can be stored and retrieved.
>>
>> External ODBs should be able to tranfer information about the blobs
>> they store. This patch series shows how this is possible using kind of
>> replace refs.
>
> Great to see this making progress!
>
> My thoughts and questions are mostly about the overall design tradeoffs.
>
> Is your intention to enable the ODB to completely replace the regular object
> store or just to supplement it?

It is to supplement it, as I think the regular object store works very
well most of the time.

> I think it would be good to ensure the
> interface is robust and performant enough to actually replace the current
> object store interface (even if we don't actually do that just yet).

I agree that it should be robust and performant, but I don't think it
needs to be as performant in all cases as the current object store
right now.

> Another way of asking this is: do the 3 verbs (have, get, put) and the 3
> types of "get" enable you to wrap the current loose object and pack file
> code as ODBs and run completely via the external ODB interface?  If not,
> what is missing and can it be added?

Right now the "put" verb only send plain blobs, so the most logical
way to run completely via the external ODB interface would be to use
it to send and receive plain blobs. There are tests scripts (t0420,
t0470 and t0480) that use an http server as the external ODB and all
the blobs are stored in it.

And yeah for now it works only for blobs. There is a temporary patch
in the series that limits it to blobs. For the non RFC patch series, I
think it should either use the attribute system to tell which objects
should be run via the external ODB interface, or perhaps there should
be a way to ask each external ODB helper which kind of objects and
blobs it can handle. I should add that in the future work part.

> _Eventually_ it would be great to see the current object store(s) moved
> behind the new ODB interface.

This is not one of my goals and I think it could be a problem if we
want to keep the "fault in" mode.
In this mode the helper writes or reads directly to or from the
current object store, so it needs the current object store to be
available.

Also I think compatibility with other git implementations is important
and it is a good thing that they can all work on a common repository
format.

> When there are multiple ODB providers, what is the order they are called?

The external_odb_config() function creates the helpers for the
external ODBs in the order they are found in the config file, and then
these helpers are called in turn in the same order.

> If one fails a request (get, have, put) are the others called to see if they
> can fulfill the request?

Yes, but there are no tests to check that it works well. I will need
to add some.

> Can the order they are called for various verb be configured explicitly?

Right now, you can configure the order by changing the config file,
but the order will be the same for all the verbs.

> For
> example, it would be nice to have a "large object ODB handler" configured to
> get first try at all "put" verbs.  Then if it meets it's size requirements,
> it will handle the verb, otherwise it fail and git will try the other ODBs.

This can work if the "large object ODB handler" is configured first.

Also this is linked with how you define which objects are handled by
which helper. For example if the attribute system is used to describe
which external ODB is used for which files, there could be a way to
tell for example that blobs larger than 1MB are handled by the "large
object ODB handler" while those that are smaller are handled by
another helper.

>> Design
>> ~~
>>
>> * The "helpers" (registered commands)
>>
>> Each helper manages access to one external ODB.
>>
>> There are now 2 different modes for helper:
>>
>>- When "odb..scriptMode" is set to "true", the helper is
>>  launched each time Git wants to communicate with the 
>>  external ODB.
>>
>>- When "odb..scriptMode" is not set or set to "false", then
>>  the helper is launched once as a sub-process (using
>>  sub-process.h), and Git communicates with it using packet lines.
>
> Is it worth supporting two different modes long term?  It seems that this
> could be simplified (less code to write, debug, document, support) by only
> supporting the 2nd that uses the sub-process.  As far as I can tell, the
> capabilities are the same, it's just the second one is more performant when
> multiple calls are made.

Yeah, capabilities are the same, but 

Re: [RFC/PATCH v4 00/49] Add initial experimental external ODB support

[Christian Couder]

On Sat, Jul 1, 2017 at 9:41 PM, Christian Couder
 wrote:
> On Fri, Jun 23, 2017 at 8:24 PM, Ben Peart  wrote:

>> The fact that "git clone is taught a --initial-refspec" option" indicates
>> this isn't just an ODB implementation detail.  Is there a general capability
>> that is missing from the ODB interface that needs to be addressed here?
>
> Technically you don't need to teach `git clone` the --initial-refspec
> option to make it work.
> It can work like this:
>
> $ git init
> $ git remote add origin 
> $ git fetch origin 
> $ git config odb..command 
> $ git fetch origin
>
> But it is much simpler for the user to instead just do:
>
> $ git clone -c odb..command= --initial-refspec
>  
>
> I also think that the --initial-refspec option could perhaps be useful
> for other kinds of refs for example tags, notes or replace refs, to
> make sure that those refs are fetched first and that hooks can use
> them when fetching other refs like branches in the later part of the
> clone.

Actually I am not sure that it's possible to setup hooks per se before
or while cloning, but perhaps there are other kind of scripts or git
commands that could trigger and use the refs that have been fetched
first.

Re: Request for git merge --signoff

[Junio C Hamano]

Dan Kohn  writes:

> On Sat, Jul 1, 2017 at 1:45 PM, Junio C Hamano  wrote:
>> Dan Kohn  writes:
>
>>> Could you please add a `--signoff` option to `git merge`?
>
>> The reason why we changed the default for "git merge" to start an
>> editor at around v1.7.10 was because we wanted to encourage people
>> to write log message that more meaningfully documents the change,
>> and adding sign-off is probably in line with that.
>
>> I've done that "commit --amend" on a merge to tweak its message
>> myself number of times, but I have to admit that I never did so for
>> sign-off, but why not? ;-)
>
> I'm not opposed to starting the editor, although ...
> ...
> Even if you turn down my request for a new flag here,...

Was I a bit too oblique?  I said that do think "merge --signoff" is
in line with the latest philosophy, i.e. not rejecting the idea of
add such an option.  IOW, patches welcome (either from you or other
people).

Re: [PATCH] hooks: add signature to the top of the commit message

[Junio C Hamano]

"Philip Oakley"  writes:

> From: "Junio C Hamano" 
>> Kaartic Sivaraam  writes:
>>
 By the way, the one that is still actually enabled is no longer
 needed.  The commit template generated internally was corrected some
 time ago not to add the "Conflicts:" section without commenting it
 out.

>>> I'll send in another patch that removes it but it seems removing it
>>> would leave sample hook without anything turned on by default. That
>>> doesn't sound fine, does it?
>>
>> Actually I was wondering if it is a good idea to remove it, as it
>> seems to have outlived its usefulness.
>
> Personally, I like the comfort of seeing the Conflicts: list, but if
> others have indicated otherwise...

Oh, I think you misread the discussion while arriving from the
sideways.  My "it" in the "remove it" refers to the sample
prepare-commit-msg hook; among the three examples in that hook, only
one of them is enabled but that one was to comment out the "Conflicts"
section in the log message editor.  These days, that section already
appears in a commented-out form without the help of that hook, so
there is nothing useful in there---hence a suggestion for removal of
the sample.

Re: [PATCH] hooks: add signature to the top of the commit message

[Junio C Hamano]

Kaartic Sivaraam  writes:

> On Sat, 2017-07-01 at 10:36 -0700, Junio C Hamano wrote:
>> Actually I was wondering if it is a good idea to remove it, as it
>> seems to have outlived its usefulness.
> It does seem  to be a good idea but it would leave the 'prepare-commit-
> msg' hook with no scripts that could be used by just activating it.
> That's why I thought of adding a script that removes the "Please enter
> your.." message from the comments if it exists.

That sounds like a sample that is there not because it would be
useful, but because we couldn't think of any useful example.

IOW, I view it just as useful as a sample that does

#!/bin/sh
echo "# useless cruft" >>"$1"

whose sole value is to demonstrate that you could affect what you
see in the editor by modifying "$1".

Re: [RFC/PATCH v4 00/49] Add initial experimental external ODB support

[Junio C Hamano]

Christian Couder  writes:

>> I think it would be good to ensure the
>> interface is robust and performant enough to actually replace the current
>> object store interface (even if we don't actually do that just yet).
>
> I agree that it should be robust and performant, but I don't think it
> needs to be as performant in all cases as the current object store
> right now.

That sounds like starting from a defeatest position.  Is there a
reason why you think using an external interface could never perform
well enough to be usable in everyday work?

Re: [PATCH] hooks: add signature to the top of the commit message

[Philip Oakley]

From: "Junio C Hamano" 

"Philip Oakley"  writes:


From: "Junio C Hamano" 

Kaartic Sivaraam  writes:


By the way, the one that is still actually enabled is no longer
needed.  The commit template generated internally was corrected some
time ago not to add the "Conflicts:" section without commenting it
out.


I'll send in another patch that removes it but it seems removing it
would leave sample hook without anything turned on by default. That
doesn't sound fine, does it?


Actually I was wondering if it is a good idea to remove it, as it
seems to have outlived its usefulness.


Personally, I like the comfort of seeing the Conflicts: list, but if
others have indicated otherwise...


Oh, I think you misread the discussion while arriving from the
sideways.  My "it" in the "remove it" refers to the sample
prepare-commit-msg hook; among the three examples in that hook, only
one of them is enabled but that one was to comment out the "Conflicts"
section in the log message editor.  These days, that section already
appears in a commented-out form without the help of that hook, so
there is nothing useful in there---hence a suggestion for removal of
the sample.



Thanks, yes I had misread it. I hadn't managed the time to follow the 
details. Problem solved.


Philip 

[PATCH v3 1/3] sha1dc: update from upstream

[Ævar Arnfjörð Bjarmason]

Update sha1dc from the latest version by the upstream maintainer[1].

See commit 6b851e536b ("sha1dc: update from upstream", 2017-06-06) for
the last update.

This solves the Big Endian detection on Solaris reported against
v2.13.2[2], hopefully without any regressions. A version of this has
been tested on two Solaris SPARC installations, Cygwin (by jturney on
cygwin@Freenode), and on numerous more boring systems (mainly
linux/x86_64). See [3] for a discussion of the implementation and
platform-specific issues.

See commit a0103914c2 ("sha1dc: update from upstream", 2017-05-20) and
6b851e536b ("sha1dc: update from upstream", 2017-06-06) for previous
attempts in the 2.13 series to address various compile-time feature
detection in this library.

1. 
https://github.com/cr-marcstevens/sha1collisiondetection/commit/19d97bf5af05312267c2e874ee6bcf584d9e9681
2. 
   
(https://public-inbox.org/git/CAKKM46tHq13XiW5C8sux3=pz1vhsu_npg8exfwwcpd7rkzk...@mail.gmail.com/)
3. https://github.com/cr-marcstevens/sha1collisiondetection/pull/34

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 sha1dc/sha1.c | 90 ---
 1 file changed, 67 insertions(+), 23 deletions(-)

diff --git a/sha1dc/sha1.c b/sha1dc/sha1.c
index 3a1735e5ca..d5948826c8 100644
--- a/sha1dc/sha1.c
+++ b/sha1dc/sha1.c
@@ -10,6 +10,9 @@
 #include 
 #include 
 #include 
+#ifdef __unix__
+#include  /* make sure macros like _BIG_ENDIAN visible */
+#endif
 #endif
 
 #ifdef SHA1DC_CUSTOM_INCLUDE_SHA1_C
@@ -23,6 +26,13 @@
 #include "sha1.h"
 #include "ubc_check.h"
 
+#if (defined(__amd64__) || defined(__amd64) || defined(__x86_64__) || 
defined(__x86_64) || \
+ defined(i386) || defined(__i386) || defined(__i386__) || 
defined(__i486__)  || \
+ defined(__i586__) || defined(__i686__) || defined(_M_IX86) || 
defined(__X86__) || \
+ defined(_X86_) || defined(__THW_INTEL__) || defined(__I86__) || 
defined(__INTEL__) || \
+ defined(__386) || defined(_M_X64) || defined(_M_AMD64))
+#define SHA1DC_ON_INTEL_LIKE_PROCESSOR
+#endif
 
 /*
Because Little-Endian architectures are most common,
@@ -32,29 +42,70 @@
If you are compiling on a big endian platform and your compiler does not 
define one of these,
you will have to add whatever macros your tool chain defines to indicate 
Big-Endianness.
  */
-#ifdef SHA1DC_BIGENDIAN
-#undef SHA1DC_BIGENDIAN
-#endif
 
-#if (defined(_BYTE_ORDER) || defined(__BYTE_ORDER) || defined(__BYTE_ORDER__))
-
-#if ((defined(_BYTE_ORDER) && (_BYTE_ORDER == _BIG_ENDIAN)) || \
- (defined(__BYTE_ORDER) && (__BYTE_ORDER == __BIG_ENDIAN)) || \
- (defined(__BYTE_ORDER__) && (__BYTE_ORDER__ == __BIG_ENDIAN__)) )
+#if defined(__BYTE_ORDER__) && defined(__ORDER_BIG_ENDIAN__)
+/*
+ * Should detect Big Endian under GCC since at least 4.6.0 (gcc svn
+ * rev #165881). See
+ * https://gcc.gnu.org/onlinedocs/cpp/Common-Predefined-Macros.html
+ *
+ * This also works under clang since 3.2, it copied the GCC-ism. See
+ * clang.git's 3b198a97d2 ("Preprocessor: add __BYTE_ORDER__
+ * predefined macro", 2012-07-27)
+ */
+#if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
 #define SHA1DC_BIGENDIAN
 #endif
 
-#else
-
-#if (defined(_BIG_ENDIAN) || defined(__BIG_ENDIAN) || defined(__BIG_ENDIAN__) 
|| \
- defined(__ARMEB__) || defined(__THUMBEB__) || defined(__AARCH64EB__) || \
- defined(__MIPSEB__) || defined(__MIPSEB) || defined(_MIPSEB) || \
- defined(__sparc))
+/* Not under GCC-alike */
+#elif defined(__BYTE_ORDER) && defined(__BIG_ENDIAN)
+/*
+ * Should detect Big Endian under glibc.git since 14245eb70e ("entered
+ * into RCS", 1992-11-25). Defined in  which will have been
+ * brought in by standard headers. See glibc.git and
+ * https://sourceforge.net/p/predef/wiki/Endianness/
+ */
+#if __BYTE_ORDER == __BIG_ENDIAN
 #define SHA1DC_BIGENDIAN
 #endif
 
+/* Not under GCC-alike or glibc */
+#elif defined(_BYTE_ORDER) && defined(_BIG_ENDIAN) && defined(_LITTLE_ENDIAN)
+/*
+ * *BSD and newlib (embeded linux, cygwin, etc).
+ * the defined(_BIG_ENDIAN) && defined(_LITTLE_ENDIAN) part prevents
+ * this condition from matching with Solaris/sparc.
+ * (Solaris defines only one endian macro)
+ */
+#if _BYTE_ORDER == _BIG_ENDIAN
+#define SHA1DC_BIGENDIAN
 #endif
 
+/* Not under GCC-alike or glibc or *BSD or newlib */
+#elif (defined(__ARMEB__) || defined(__THUMBEB__) || defined(__AARCH64EB__) || 
\
+   defined(__MIPSEB__) || defined(__MIPSEB) || defined(_MIPSEB) || \
+   defined(__sparc))
+/*
+ * Should define Big Endian for a whitelist of known processors. See
+ * https://sourceforge.net/p/predef/wiki/Endianness/ and
+ * 
http://www.oracle.com/technetwork/server-storage/solaris/portingtosolaris-138514.html
+ */
+#define SHA1DC_BIGENDIAN
+
+/* Not under GCC-alike or glibc or *BSD or newlib or  */
+#elif defined(SHA1DC_ON_INTEL_LIKE_PROCESSOR)
+/*
+ * As a last resort before we do anything else we're not 100% sure
+ * about below, we blacklist specific processors here. We could add
+ * more,

[PATCH v3 3/3] sha1collisiondetection: automatically enable when submodule is populated

[Ævar Arnfjörð Bjarmason]

From: Junio C Hamano 

If a user wants to experiment with the version of collision
detecting sha1 from the submodule, the user needed to not just
populate the submodule but also needed to turn the knob.

A Makefile trick is easy enough to do so, so let's do this.  When
somebody with a copy of the submodule populated wants not to use it,
that can be done by overriding it in config.mak or from the command
line.

Signed-off-by: Junio C Hamano 
Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 Makefile | 4 
 1 file changed, 4 insertions(+)

diff --git a/Makefile b/Makefile
index f0cac1f246..3baa1eac0b 100644
--- a/Makefile
+++ b/Makefile
@@ -1009,6 +1009,10 @@ EXTLIBS =
 
 GIT_USER_AGENT = git/$(GIT_VERSION)
 
+ifeq ($(wildcard 
sha1collisiondetection/lib/sha1.h),sha1collisiondetection/lib/sha1.h)
+DC_SHA1_SUBMODULE = auto
+endif
+
 include config.mak.uname
 -include config.mak.autogen
 -include config.mak
-- 
2.13.1.611.g7e3b11ae1

[PATCH v3 2/3] sha1dc: optionally use sha1collisiondetection as a submodule

[Ævar Arnfjörð Bjarmason]

Add an option to use the sha1collisiondetection library from the
submodule in sha1collisiondetection/ instead of in the copy in the
sha1dc/ directory.

This allows us to try out the submodule in sha1collisiondetection
without breaking the build for anyone who's not expecting them as we
work out any kinks.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 .gitmodules|  4 
 Makefile   | 12 
 hash.h |  4 
 sha1collisiondetection |  1 +
 4 files changed, 21 insertions(+)
 create mode 100644 .gitmodules
 create mode 16 sha1collisiondetection

diff --git a/.gitmodules b/.gitmodules
new file mode 100644
index 00..cbeebdab7a
--- /dev/null
+++ b/.gitmodules
@@ -0,0 +1,4 @@
+[submodule "sha1collisiondetection"]
+   path = sha1collisiondetection
+   url = https://github.com/cr-marcstevens/sha1collisiondetection.git
+   branch = master
diff --git a/Makefile b/Makefile
index b94cd5633c..f0cac1f246 100644
--- a/Makefile
+++ b/Makefile
@@ -162,6 +162,12 @@ all::
 # algorithm. This is slower, but may detect attempted collision attacks.
 # Takes priority over other *_SHA1 knobs.
 #
+# Define DC_SHA1_SUBMODULE in addition to DC_SHA1 to use the
+# sha1collisiondetection shipped as a submodule instead of the
+# non-submodule copy in sha1dc/. This is an experimental option used
+# by the git project to migrate to using sha1collisiondetection as a
+# submodule.
+#
 # Define OPENSSL_SHA1 environment variable when running make to link
 # with the SHA1 routine from openssl library.
 #
@@ -1448,8 +1454,14 @@ ifdef APPLE_COMMON_CRYPTO
BASIC_CFLAGS += -DSHA1_APPLE
 else
DC_SHA1 := YesPlease
+ifdef DC_SHA1_SUBMODULE
+   LIB_OBJS += sha1collisiondetection/lib/sha1.o
+   LIB_OBJS += sha1collisiondetection/lib/ubc_check.o
+   BASIC_CFLAGS += -DDC_SHA1_SUBMODULE
+else
LIB_OBJS += sha1dc/sha1.o
LIB_OBJS += sha1dc/ubc_check.o
+endif
BASIC_CFLAGS += \
-DSHA1_DC \
-DSHA1DC_NO_STANDARD_INCLUDES \
diff --git a/hash.h b/hash.h
index a11fc9233f..bef3e630a0 100644
--- a/hash.h
+++ b/hash.h
@@ -8,7 +8,11 @@
 #elif defined(SHA1_OPENSSL)
 #include 
 #elif defined(SHA1_DC)
+#ifdef DC_SHA1_SUBMODULE
+#include "sha1collisiondetection/lib/sha1.h"
+#else
 #include "sha1dc/sha1.h"
+#endif
 #else /* SHA1_BLK */
 #include "block-sha1/sha1.h"
 #endif
diff --git a/sha1collisiondetection b/sha1collisiondetection
new file mode 16
index 00..19d97bf5af
--- /dev/null
+++ b/sha1collisiondetection
@@ -0,0 +1 @@
+Subproject commit 19d97bf5af05312267c2e874ee6bcf584d9e9681
-- 
2.13.1.611.g7e3b11ae1

[PATCH v3 0/3] Update sha1dc from upstream

[Ævar Arnfjörð Bjarmason]

The upstream discussion about solving Big Endian detection concluded
with something that hopefully works on all our platforms, see
https://github.com/cr-marcstevens/sha1collisiondetection/pull/34

This updates us to the latest upstream commit.

Junio C Hamano (1):
  sha1collisiondetection: automatically enable when submodule is
populated

Ævar Arnfjörð Bjarmason (2):
  sha1dc: update from upstream
  sha1dc: optionally use sha1collisiondetection as a submodule

 .gitmodules|  4 +++
 Makefile   | 16 +
 hash.h |  4 +++
 sha1collisiondetection |  1 +
 sha1dc/sha1.c  | 90 +-
 5 files changed, 92 insertions(+), 23 deletions(-)
 create mode 100644 .gitmodules
 create mode 16 sha1collisiondetection

-- 
2.13.1.611.g7e3b11ae1

Re: Request for git merge --signoff

[Ævar Arnfjörð Bjarmason]

On Sat, Jul 01 2017, Dan Kohn jotted:

> https://github.com/coreinfrastructure/best-practices-badge is a user
> of the https://github.com/probot/dco bot which checks that commits
> have a signoff. The issue is that there is no `--signoff` option in
> git for merge commits.

I think it's fine to add such a feature, but it seems like an obvious
bug to me in such a bot[1] that it's enforcing the DCO on merge commits.

The entire point of the DCO is to certify that you have the rights to
submit the patch etc., it's quite dubious to be applying that to merge
commits which contain no original work (most of the time, although of
course a merge commit can have significant conflict resolution).

So yeah, it would make sense to have a --signoff option, especially to
use when the merge actually does contain original work, but your stated
reason for wanting this just seems like an easily solved bug in the bot:
Exclude those commits that have no patch contents.

1. https://github.com/probot/dco/

Re: [RFC/PATCH v4 00/49] Add initial experimental external ODB support

[Christian Couder]

On Sat, Jul 1, 2017 at 10:33 PM, Junio C Hamano  wrote:
> Christian Couder  writes:
>
>>> I think it would be good to ensure the
>>> interface is robust and performant enough to actually replace the current
>>> object store interface (even if we don't actually do that just yet).
>>
>> I agree that it should be robust and performant, but I don't think it
>> needs to be as performant in all cases as the current object store
>> right now.
>
> That sounds like starting from a defeatest position.  Is there a
> reason why you think using an external interface could never perform
> well enough to be usable in everyday work?

Perhaps in the future we will be able to make it as performant as, or
perhaps even more performant, than the current object store, but in
the current implementation the following issues mean that it will be
less performant:

- The external object stores are searched for an object after the
object has not been found in the current object store. This means that
searching for an object will be slower if the object is in an external
object store. To overcome this the "have" information (when the
external helper implements it) could be merged with information about
what objects are in the current object store, for example in a big
table or bitmap, so that only one lookup in this table or bitmap would
be needed to know if an object is available and in which object store
it is. But I really don't want to get into this right now.

- When an external odb helper retrieves an object and passes it to
Git, Git (or the helper itself in "fault in" mode) then stores the
object in the current object store. This is because we assume that it
will be faster to retrieve it again if it is cached in the current
object store. There could be a capability that asks Git to not cache
the objects that are retrieved from the external odb, but again I
don't think it is necessary at all to implement this right now.

I still think though that in some cases, like when the external odb is
used to implement a bundle clone, using the external odb mechanism can
already be more performant.