Re: worktrees vs. alternates

[Jeff King]

On Thu, May 17, 2018 at 06:13:55AM +0530, Sitaram Chamarty wrote:

> I may have missed a few of the earlier messages, but in the last
> 20 or so in this thread, I did not see namespaces mentioned by
> anyone. (I.e., apologies if it was addressed and discarded
> earlier!)
> 
> I was under the impression that, as long as "read" access need
> not be controlled (Konstantin's situation, at least, and maybe
> Peff's too, for public repos), namespaces are a good way to
> create and manage that "mother repo".
> 
> Is that not true anymore?  Mind, I have not actually used them
> in anger anywhere, so I could be missing some really big point
> here.

The biggest problem with namespaces as they are currently implemented is
that they do not apply universally to all commands. If you only access
the repo via push/fetch, they may be fine. But as soon as you start
doing other operations (e.g., showing the history of a branch in a web
interface), you don't get to use the namespaced names anymore.

I think a different implementation of namespaces could do this better.
E.g., by controlling the view of the refs at the refs.c layer (or
perhaps as a filtering backend).

-Peff

Re: Add option to git to ignore binary files unless force added

[Jacob Keller]

On Wed, May 16, 2018 at 5:45 PM, Anmol Sethi  wrote:
> I think it’d be great to have an option to have git ignore binary files. My 
> repositories are always source only, committing a binary is always a mistake. 
> At the moment, I have to configure the .gitignore to ignore every binary file 
> and that gets tedious. Having git ignore all binary files would be great.
>
> This could be achieved via an option in .gitconfig or maybe a special line in 
> .gitignore.
>
> I just want to never accidentally commit a binary again.
>
> --
> Best,
> Anmol
>

I believe you can do a couple things. There should be a hook which you
can modify to validate that there are no binary files on
pre-commit[1], or pre-push[2] to verify that you never push commits
with binaries in them.

You could also implement the update hook on the server if you control
it, to allow it to block pushes which contain binary files.

Thanks,
Jake

[1]https://git-scm.com/docs/githooks#_pre_commit
[2]https://git-scm.com/docs/githooks#_pre_push
[3]https://git-scm.com/docs/githooks#update

Re: [PATCH v4 0/3] Add --progress and --dissociate to git submodule

[Junio C Hamano]

Casey Fitzpatrick  writes:

> These patches add --progress and --dissociate options to git submodule.
>
> The --progress option existed beforehand, but only for the update command and
> it was left undocumented.
>
> Both add and update submodule commands supported --reference, but not its pair
> option --dissociate which allows for independent clones rather than depending
> on the reference.

This round hasn't seen any further comment, and I think all the
previously raised issues have been resolved in this version, so
I am tempted to merge it to 'next'.

One thing that I noticed is that "git submodule -h" does not give
any hint about --dissociate even though it talks about --reference
with these patches applied, and I think it probably should [*1*].

We can make such an update as incremental patch on top of these, if
people cared enough about this, without a further reroll of these
three patches.

Thanks.


[Footnote]

*1* It's not like we must list each and every option available---it
is perfectly fine to omit less common ones to keep the "-h" output
manageably short.  But I have a feeling that "--reference" and
"--dissociate" should probably be at a similar level of importance,
and if we list one we probably should show the other, too.

Re: [PATCH] grep: handle corrupt index files early

[Junio C Hamano]

Duy Nguyen  writes:

> With a majority of call sites dying like this though, I wonder if we
> should just add repo_read_index_or_die() with die() inside. Then the
> next person won't likely accidentally forget _()

Yuck.

That sounds like inviting a major code churn.  I tend to agree that
it would be a good clean-up for longer term maintenance, but I am
not sure if I can honestly say I'd look forward to such a clean-up
at this point in the cycle when there are tons of large-ish topics
in flight X-<.

Add option to git to ignore binary files unless force added

[Anmol Sethi]

I think it’d be great to have an option to have git ignore binary files. My 
repositories are always source only, committing a binary is always a mistake. 
At the moment, I have to configure the .gitignore to ignore every binary file 
and that gets tedious. Having git ignore all binary files would be great.

This could be achieved via an option in .gitconfig or maybe a special line in 
.gitignore.

I just want to never accidentally commit a binary again.

-- 
Best,
Anmol

Re: worktrees vs. alternates

[Sitaram Chamarty]

On Wed, May 16, 2018 at 04:02:53PM -0400, Konstantin Ryabitsev wrote:
> On 05/16/18 15:37, Jeff King wrote:
> > Yes, that's pretty close to what we do at GitHub. Before doing any
> > repacking in the mother repo, we actually do the equivalent of:
> > 
> >   git fetch --prune ../$id.git +refs/*:refs/remotes/$id/*
> >   git repack -Adl
> > 
> > from each child to pick up any new objects to de-duplicate (our "mother"
> > repos are not real repos at all, but just big shared-object stores).
> 
> Yes, I keep thinking of doing the same, too -- instead of using
> torvalds/linux.git for alternates, have an internal repo where objects
> from all forks are stored. This conversation may finally give me the
> shove I've been needing to poke at this. :)

I may have missed a few of the earlier messages, but in the last
20 or so in this thread, I did not see namespaces mentioned by
anyone. (I.e., apologies if it was addressed and discarded
earlier!)

I was under the impression that, as long as "read" access need
not be controlled (Konstantin's situation, at least, and maybe
Peff's too, for public repos), namespaces are a good way to
create and manage that "mother repo".

Is that not true anymore?  Mind, I have not actually used them
in anger anywhere, so I could be missing some really big point
here.

sitaram


signature.asc
Description: PGP signature

[PATCH 1/2] refspec: consolidate ref-prefix generation logic

[Brandon Williams]

When using protocol v2 a client constructs a list of ref-prefixes which
are sent across the wire so that the server can do server-side filtering
of the ref-advertisement.  The logic that does this exists for both
fetch and push (even though no push support for v2 currently exists yet)
and is roughly the same so lets consolidate this logic and make it
general enough that it can be used for both the push and fetch cases.

Signed-off-by: Brandon Williams 
---
 builtin/fetch.c | 13 +
 refspec.c   | 29 +
 refspec.h   |  4 
 transport.c | 21 +
 4 files changed, 35 insertions(+), 32 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 3fad1f0db..80bb14370 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -351,18 +351,7 @@ static struct ref *get_ref_map(struct transport *transport,
 
const struct ref *remote_refs;
 
-   for (i = 0; i < rs->nr; i++) {
-   const struct refspec_item *item = >items[i];
-   if (!item->exact_sha1) {
-   const char *glob = strchr(item->src, '*');
-   if (glob)
-   argv_array_pushf(_prefixes, "%.*s",
-(int)(glob - item->src),
-item->src);
-   else
-   expand_ref_prefix(_prefixes, item->src);
-   }
-   }
+   refspec_ref_prefixes(rs, _prefixes);
 
remote_refs = transport_get_remote_refs(transport, _prefixes);
 
diff --git a/refspec.c b/refspec.c
index 97e76e8b1..c59a4ccf1 100644
--- a/refspec.c
+++ b/refspec.c
@@ -1,4 +1,5 @@
 #include "cache.h"
+#include "argv-array.h"
 #include "refs.h"
 #include "refspec.h"
 
@@ -192,3 +193,31 @@ int valid_fetch_refspec(const char *fetch_refspec_str)
refspec_item_clear();
return ret;
 }
+
+void refspec_ref_prefixes(const struct refspec *rs,
+ struct argv_array *ref_prefixes)
+{
+   int i;
+   for (i = 0; i < rs->nr; i++) {
+   const struct refspec_item *item = >items[i];
+   const char *prefix = NULL;
+
+   if (rs->fetch == REFSPEC_FETCH)
+   prefix = item->src;
+   else if (item->dst)
+   prefix = item->dst;
+   else if (item->src && !item->exact_sha1)
+   prefix = item->src;
+
+   if (prefix) {
+   if (item->pattern) {
+   const char *glob = strchr(prefix, '*');
+   argv_array_pushf(ref_prefixes, "%.*s",
+(int)(glob - prefix),
+prefix);
+   } else {
+   expand_ref_prefix(ref_prefixes, prefix);
+   }
+   }
+   }
+}
diff --git a/refspec.h b/refspec.h
index 7e1ff94ac..01b700e09 100644
--- a/refspec.h
+++ b/refspec.h
@@ -41,4 +41,8 @@ void refspec_clear(struct refspec *rs);
 
 int valid_fetch_refspec(const char *refspec);
 
+struct argv_array;
+void refspec_ref_prefixes(const struct refspec *rs,
+ struct argv_array *ref_prefixes);
+
 #endif /* REFSPEC_H */
diff --git a/transport.c b/transport.c
index 7e0b9abba..cbf0044c3 100644
--- a/transport.c
+++ b/transport.c
@@ -1088,30 +1088,11 @@ int transport_push(struct transport *transport,
int pretend = flags & TRANSPORT_PUSH_DRY_RUN;
int push_ret, ret, err;
struct argv_array ref_prefixes = ARGV_ARRAY_INIT;
-   int i;
 
if (check_push_refs(local_refs, rs) < 0)
return -1;
 
-   for (i = 0; i < rs->nr; i++) {
-   const struct refspec_item *item = >items[i];
-   const char *prefix = NULL;
-
-   if (item->dst)
-   prefix = item->dst;
-   else if (item->src && !item->exact_sha1)
-   prefix = item->src;
-
-   if (prefix) {
-   const char *glob = strchr(prefix, '*');
-   if (glob)
-   argv_array_pushf(_prefixes, "%.*s",
-(int)(glob - prefix),
-prefix);
-   else
-   expand_ref_prefix(_prefixes, 
prefix);
-   }
-   }
+   refspec_ref_prefixes(rs, _prefixes);
 
remote_refs = transport->vtable->get_refs_list(transport, 1,
   _prefixes);
-- 

[PATCH 2/2] fetch: generate ref-prefixes when using a configured refspec

[Brandon Williams]

Teach fetch to generate ref-prefixes, to be used for server-side
filtering of the ref-advertisement, based on the configured fetch
refspec ('remote..fetch') when no user provided refspec exists.

Signed-off-by: Brandon Williams 
---
 builtin/fetch.c| 10 +-
 t/t5702-protocol-v2.sh | 14 ++
 2 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 80bb14370..7cc7a52de 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -351,7 +351,15 @@ static struct ref *get_ref_map(struct transport *transport,
 
const struct ref *remote_refs;
 
-   refspec_ref_prefixes(rs, _prefixes);
+   if (rs->nr)
+   refspec_ref_prefixes(rs, _prefixes);
+   else if (transport->remote && transport->remote->fetch.nr)
+   refspec_ref_prefixes(>remote->fetch, _prefixes);
+
+   if (ref_prefixes.argc &&
+   (tags == TAGS_SET || (tags == TAGS_DEFAULT && !rs->nr))) {
+   argv_array_push(_prefixes, "refs/tags/");
+   }
 
remote_refs = transport_get_remote_refs(transport, _prefixes);
 
diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index 56f7c3c32..b6c72ab51 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -201,6 +201,20 @@ test_expect_success 'ref advertisment is filtered during 
fetch using protocol v2
! grep "refs/tags/three" log
 '
 
+test_expect_success 'default refspec is used to filter ref when fetchcing' '
+   test_when_finished "rm -f log" &&
+
+   GIT_TRACE_PACKET="$(pwd)/log" git -C file_child -c protocol.version=2 \
+   fetch origin &&
+
+   git -C file_child log -1 --format=%s three >actual &&
+   git -C file_parent log -1 --format=%s three >expect &&
+   test_cmp expect actual &&
+
+   grep "ref-prefix refs/heads/" log &&
+   grep "ref-prefix refs/tags/" log
+'
+
 # Test protocol v2 with 'http://' transport
 #
 . "$TEST_DIRECTORY"/lib-httpd.sh
-- 
2.17.0.441.gb46fe60e1d-goog

[PATCH 0/2] generating ref-prefixes for configured refspecs

[Brandon Williams]

Here's my short follow on series to the refspec refactoring.

When v2 was introduced ref-prefixes were only generated for user
provided refspecs (given via the command line).  This means that you can
only benefit from server-side ref filtering if you explicitly provide a
refspec, so this short series extends this to generate the ref-prefixes
even for the refspecs which are configured in 'remote..fetch'.

This series is based on the v2 of the refspec refactoring series.

Brandon Williams (2):
  refspec: consolidate ref-prefix generation logic
  fetch: generate ref-prefixes when using a configured refspec

 builtin/fetch.c| 19 ---
 refspec.c  | 29 +
 refspec.h  |  4 
 t/t5702-protocol-v2.sh | 14 ++
 transport.c| 21 +
 5 files changed, 56 insertions(+), 31 deletions(-)

-- 
2.17.0.441.gb46fe60e1d-goog

Re: [PATCH v3 13/28] t3702: abstract away SHA-1-specific constants

[brian m. carlson]

On Wed, May 16, 2018 at 10:18:33AM -0700, Stefan Beller wrote:
> This reminds me of the way we test alot of the patch format already.
> But there we use standard grep as opposed to egrep.
> 
> git grep egrep doesn't show a lot of hits, but all commits
> that mention egrep found via 'git log --grep egrep' mention
> that there is some sort of portability issue for using egrep
> specifically.
> 
> Is the ^index a problem for standard grep, i.e. do we need to fix
> other places?

I think this is just me preferring a more careful match, but if there
are potential portability problems, I can reroll to use regular grep.  I
don't think which one we use matters much one way or the other in this
case, or in general.  We don't tend to produce a lot of potentially
ambiguous matches in our testsuite.

I think the uses in the commit messages tend to point out quirks in
command line options over specifically concerns about the use of egrep
itself.  I suspect the implementations that want egrep over grep -E (the
latter being POSIX) also lack many of the POSIX options that people want
to use, although I'm not aware of egrep itself being broken anywhere.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: worktrees vs. alternates

[Jeff King]

On Wed, May 16, 2018 at 02:18:20PM -0700, Stefan Beller wrote:

> >
> > You can also do periodic maintenance like:
> >
> >   1. Copy each ref in the forked repositories into the parent repository
> >  (e.g., giving each child that borrows from the parent its own
> >  hierarchy in refs/remotes//*).
> 
> Can you just copy? I assume the mother repo doesn't know about
> all objects, hence by copying the ref, we have a "spotty" history.
> 
> And to improve copying could permanent symlinking be used instead?

Sorry, by copying, I meant "fetching". I.e., migrating objects and refs.

-Peff

Proposal

[Miss Zeliha Omer Faruk]

Hello

Greetings to you please i have a business proposal for you contact me
for more detailes asap thanks.

Best Regards,
Miss.Zeliha ömer faruk
Esentepe Mahallesi Büyükdere
Caddesi Kristal Kule Binasi
No:215
Sisli - Istanbul, Turkey

Proposal

[Miss Zeliha Omer Faruk]

Hello

Greetings to you please i have a business proposal for you contact me
for more detailes asap thanks.

Best Regards,
Miss.Zeliha ömer faruk
Esentepe Mahallesi Büyükdere
Caddesi Kristal Kule Binasi
No:215
Sisli - Istanbul, Turkey

Re: [PATCH v3 07/28] t1007: annotate with SHA1 prerequisite

[brian m. carlson]

On Wed, May 16, 2018 at 09:56:33AM -0700, Stefan Beller wrote:
> Hi brian,
> 
> On Tue, May 15, 2018 at 6:56 PM, brian m. carlson
>  wrote:
> For the 2 occurrences above I think the SHA1 requirement is not
> needed as the check if a blob exists (and the id is given as $1)
> is independent of the hash function, it is just important that
> the same hash function is used in the git-cat-file as well as...
> 
> ... here, where we create the blob to test without
> writing it into the object database. In a way we test that
> the absence of -w works correctly.
> 
> Oh, the $hello_sha1 is hard coded, which is why we
> think this test is SHA1 dependent.
> 
> But that would fit in line with the test_blob[_does_not]_exist
> being independent of the hashes?

These functions are technically independent of the hash, but the way we
call them is not.  Since we only look up certain fixed values in those
functions, they're going to fail if we use a different hash.

There really isn't a great way to annotate the tests independent of the
functions without duplicating a lot of the logic that occurs in the test
library, and I didn't really want to do that.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

[PATCH v2 08/36] transport: convert transport_push to use struct refspec

[Brandon Williams]

Convert the logic in 'transport_push()' which calculates a list of
ref-prefixes to use 'struct refspec'.

Signed-off-by: Brandon Williams 
---
 transport.c | 17 +
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/transport.c b/transport.c
index 3ad4d37dc..181db4d4d 100644
--- a/transport.c
+++ b/transport.c
@@ -,21 +,22 @@ int transport_push(struct transport *transport,
int porcelain = flags & TRANSPORT_PUSH_PORCELAIN;
int pretend = flags & TRANSPORT_PUSH_DRY_RUN;
int push_ret, ret, err;
-   struct refspec_item *tmp_rs;
+   struct refspec tmp_rs = REFSPEC_INIT_PUSH;
struct argv_array ref_prefixes = ARGV_ARRAY_INIT;
int i;
 
if (check_push_refs(local_refs, refspec_nr, refspec) < 0)
return -1;
 
-   tmp_rs = parse_push_refspec(refspec_nr, refspec);
-   for (i = 0; i < refspec_nr; i++) {
+   refspec_appendn(_rs, refspec, refspec_nr);
+   for (i = 0; i < tmp_rs.nr; i++) {
+   const struct refspec_item *item = _rs.items[i];
const char *prefix = NULL;
 
-   if (tmp_rs[i].dst)
-   prefix = tmp_rs[i].dst;
-   else if (tmp_rs[i].src && !tmp_rs[i].exact_sha1)
-   prefix = tmp_rs[i].src;
+   if (item->dst)
+   prefix = item->dst;
+   else if (item->src && !item->exact_sha1)
+   prefix = item->src;
 
if (prefix) {
const char *glob = strchr(prefix, '*');
@@ -1142,7 +1143,7 @@ int transport_push(struct transport *transport,
   _prefixes);
 
argv_array_clear(_prefixes);
-   free_refspec(refspec_nr, tmp_rs);
+   refspec_clear(_rs);
 
if (flags & TRANSPORT_PUSH_ALL)
match_flags |= MATCH_REFS_ALL;
-- 
2.17.0.441.gb46fe60e1d-goog

[PATCH v2 22/36] fetch: convert prune_refs to take a struct refspec

[Brandon Williams]

Convert 'prune_refs()' to take a 'struct refspec' as a parameter instead
of a list of 'struct refspec_item'.

Signed-off-by: Brandon Williams 
---
 builtin/fetch.c | 11 +--
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 836eb7545..5e46df70c 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -959,11 +959,11 @@ static int fetch_refs(struct transport *transport, struct 
ref *ref_map)
return ret;
 }
 
-static int prune_refs(struct refspec_item *refs, int ref_count, struct ref 
*ref_map,
-   const char *raw_url)
+static int prune_refs(struct refspec *rs, struct ref *ref_map,
+ const char *raw_url)
 {
int url_len, i, result = 0;
-   struct ref *ref, *stale_refs = get_stale_heads(refs, ref_count, 
ref_map);
+   struct ref *ref, *stale_refs = get_stale_heads(rs->items, rs->nr, 
ref_map);
char *url;
int summary_width = transport_summary_width(stale_refs);
const char *dangling_msg = dry_run
@@ -1158,10 +1158,9 @@ static int do_fetch(struct transport *transport,
 * don't care whether --tags was specified.
 */
if (rs->nr) {
-   prune_refs(rs->items, rs->nr, ref_map, transport->url);
+   prune_refs(rs, ref_map, transport->url);
} else {
-   prune_refs(transport->remote->fetch.items,
-  transport->remote->fetch.nr,
+   prune_refs(>remote->fetch,
   ref_map,
   transport->url);
}
-- 
2.17.0.441.gb46fe60e1d-goog

[PATCH v2 31/36] send-pack: store refspecs in a struct refspec

[Brandon Williams]

Convert send-pack.c to store refspecs in a 'struct refspec' instead of
as an array of 'const char *'.

Signed-off-by: Brandon Williams 
---
 builtin/send-pack.c | 24 +++-
 1 file changed, 7 insertions(+), 17 deletions(-)

diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index b5427f75e..ef512616f 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -126,8 +126,7 @@ static int send_pack_config(const char *k, const char *v, 
void *cb)
 
 int cmd_send_pack(int argc, const char **argv, const char *prefix)
 {
-   int i, nr_refspecs = 0;
-   const char **refspecs = NULL;
+   struct refspec rs = REFSPEC_INIT_PUSH;
const char *remote_name = NULL;
struct remote *remote = NULL;
const char *dest = NULL;
@@ -189,8 +188,7 @@ int cmd_send_pack(int argc, const char **argv, const char 
*prefix)
argc = parse_options(argc, argv, prefix, options, send_pack_usage, 0);
if (argc > 0) {
dest = argv[0];
-   refspecs = (const char **)(argv + 1);
-   nr_refspecs = argc - 1;
+   refspec_appendn(, argv + 1, argc - 1);
}
 
if (!dest)
@@ -209,31 +207,23 @@ int cmd_send_pack(int argc, const char **argv, const char 
*prefix)
args.push_options = push_options.nr ? _options : NULL;
 
if (from_stdin) {
-   struct argv_array all_refspecs = ARGV_ARRAY_INIT;
-
-   for (i = 0; i < nr_refspecs; i++)
-   argv_array_push(_refspecs, refspecs[i]);
-
if (args.stateless_rpc) {
const char *buf;
while ((buf = packet_read_line(0, NULL)))
-   argv_array_push(_refspecs, buf);
+   refspec_append(, buf);
} else {
struct strbuf line = STRBUF_INIT;
while (strbuf_getline(, stdin) != EOF)
-   argv_array_push(_refspecs, line.buf);
+   refspec_append(, line.buf);
strbuf_release();
}
-
-   refspecs = all_refspecs.argv;
-   nr_refspecs = all_refspecs.argc;
}
 
/*
 * --all and --mirror are incompatible; neither makes sense
 * with any refspecs.
 */
-   if ((nr_refspecs > 0 && (send_all || args.send_mirror)) ||
+   if ((rs.nr > 0 && (send_all || args.send_mirror)) ||
(send_all && args.send_mirror))
usage_with_options(send_pack_usage, options);
 
@@ -275,7 +265,7 @@ int cmd_send_pack(int argc, const char **argv, const char 
*prefix)
BUG("unknown protocol version");
}
 
-   transport_verify_remote_names(nr_refspecs, refspecs);
+   transport_verify_remote_names(rs.raw_nr, rs.raw);
 
local_refs = get_local_heads();
 
@@ -287,7 +277,7 @@ int cmd_send_pack(int argc, const char **argv, const char 
*prefix)
flags |= MATCH_REFS_MIRROR;
 
/* match them up */
-   if (match_push_refs(local_refs, _refs, nr_refspecs, refspecs, 
flags))
+   if (match_push_refs(local_refs, _refs, rs.raw_nr, rs.raw, flags))
return -1;
 
if (!is_empty_cas())
-- 
2.17.0.441.gb46fe60e1d-goog

[PATCH v2 29/36] push: convert to use struct refspec

[Brandon Williams]

Convert the refspecs in builtin/push.c to be stored in a 'struct
refspec' instead of being stored in a list of 'struct refspec_item's.

Signed-off-by: Brandon Williams 
---
 builtin/push.c | 38 +++---
 1 file changed, 15 insertions(+), 23 deletions(-)

diff --git a/builtin/push.c b/builtin/push.c
index df5df6c0d..ef42979d1 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -57,19 +57,10 @@ static enum transport_family family;
 
 static struct push_cas_option cas;
 
-static const char **refspec;
-static int refspec_nr;
-static int refspec_alloc;
+static struct refspec rs = REFSPEC_INIT_PUSH;
 
 static struct string_list push_options_config = STRING_LIST_INIT_DUP;
 
-static void add_refspec(const char *ref)
-{
-   refspec_nr++;
-   ALLOC_GROW(refspec, refspec_nr, refspec_alloc);
-   refspec[refspec_nr-1] = ref;
-}
-
 static const char *map_refspec(const char *ref,
   struct remote *remote, struct ref *local_refs)
 {
@@ -138,7 +129,7 @@ static void set_refspecs(const char **refs, int nr, const 
char *repo)
}
ref = map_refspec(ref, remote, local_refs);
}
-   add_refspec(ref);
+   refspec_append(, ref);
}
 }
 
@@ -226,7 +217,7 @@ static void setup_push_upstream(struct remote *remote, 
struct branch *branch,
}
 
strbuf_addf(, "%s:%s", branch->refname, branch->merge[0]->src);
-   add_refspec(refspec.buf);
+   refspec_append(, refspec.buf);
 }
 
 static void setup_push_current(struct remote *remote, struct branch *branch)
@@ -236,7 +227,7 @@ static void setup_push_current(struct remote *remote, 
struct branch *branch)
if (!branch)
die(_(message_detached_head_die), remote->name);
strbuf_addf(, "%s:%s", branch->refname, branch->refname);
-   add_refspec(refspec.buf);
+   refspec_append(, refspec.buf);
 }
 
 static int is_workflow_triangular(struct remote *remote)
@@ -253,7 +244,7 @@ static void setup_default_push_refspecs(struct remote 
*remote)
switch (push_default) {
default:
case PUSH_DEFAULT_MATCHING:
-   add_refspec(":");
+   refspec_append(, ":");
break;
 
case PUSH_DEFAULT_UNSPECIFIED:
@@ -341,7 +332,8 @@ static void advise_ref_needs_force(void)
advise(_(message_advice_ref_needs_force));
 }
 
-static int push_with_options(struct transport *transport, int flags)
+static int push_with_options(struct transport *transport, struct refspec *rs,
+int flags)
 {
int err;
unsigned int reject_reasons;
@@ -363,7 +355,7 @@ static int push_with_options(struct transport *transport, 
int flags)
 
if (verbosity > 0)
fprintf(stderr, _("Pushing to %s\n"), transport->url);
-   err = transport_push(transport, refspec_nr, refspec, flags,
+   err = transport_push(transport, rs->raw_nr, rs->raw, flags,
 _reasons);
if (err != 0) {
fprintf(stderr, "%s", push_get_color(PUSH_COLOR_ERROR));
@@ -397,6 +389,7 @@ static int do_push(const char *repo, int flags,
struct remote *remote = pushremote_get(repo);
const char **url;
int url_nr;
+   struct refspec *push_refspec = 
 
if (!remote) {
if (repo)
@@ -417,10 +410,9 @@ static int do_push(const char *repo, int flags,
if (push_options->nr)
flags |= TRANSPORT_PUSH_OPTIONS;
 
-   if (!refspec && !(flags & TRANSPORT_PUSH_ALL)) {
-   if (remote->push.raw_nr) {
-   refspec = remote->push.raw;
-   refspec_nr = remote->push.raw_nr;
+   if (!push_refspec->nr && !(flags & TRANSPORT_PUSH_ALL)) {
+   if (remote->push.nr) {
+   push_refspec = >push;
} else if (!(flags & TRANSPORT_PUSH_MIRROR))
setup_default_push_refspecs(remote);
}
@@ -432,7 +424,7 @@ static int do_push(const char *repo, int flags,
transport_get(remote, url[i]);
if (flags & TRANSPORT_PUSH_OPTIONS)
transport->push_options = push_options;
-   if (push_with_options(transport, flags))
+   if (push_with_options(transport, push_refspec, flags))
errs++;
}
} else {
@@ -440,7 +432,7 @@ static int do_push(const char *repo, int flags,
transport_get(remote, NULL);
if (flags & TRANSPORT_PUSH_OPTIONS)
transport->push_options = push_options;
-   if (push_with_options(transport, flags))
+   if (push_with_options(transport, push_refspec, flags))
errs++;
}
return !!errs;
@@ -631,7 +623,7 

[PATCH v2 23/36] remote: convert get_stale_heads to take a struct refspec

[Brandon Williams]

Convert 'get_stale_heads()' to take a 'struct refspec' as a parameter instead
of a list of 'struct refspec_item'.

Signed-off-by: Brandon Williams 
---
 builtin/fetch.c  |  2 +-
 builtin/remote.c |  3 +--
 remote.c | 18 +-
 remote.h |  2 +-
 4 files changed, 12 insertions(+), 13 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 5e46df70c..3fad1f0db 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -963,7 +963,7 @@ static int prune_refs(struct refspec *rs, struct ref 
*ref_map,
  const char *raw_url)
 {
int url_len, i, result = 0;
-   struct ref *ref, *stale_refs = get_stale_heads(rs->items, rs->nr, 
ref_map);
+   struct ref *ref, *stale_refs = get_stale_heads(rs, ref_map);
char *url;
int summary_width = transport_summary_width(stale_refs);
const char *dangling_msg = dry_run
diff --git a/builtin/remote.c b/builtin/remote.c
index 94dfcb78b..b8e66589f 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -347,8 +347,7 @@ static int get_ref_states(const struct ref *remote_refs, 
struct ref_states *stat
else
string_list_append(>tracked, 
abbrev_branch(ref->name));
}
-   stale_refs = get_stale_heads(states->remote->fetch.items,
-states->remote->fetch.nr, fetch_map);
+   stale_refs = get_stale_heads(>remote->fetch, fetch_map);
for (ref = stale_refs; ref; ref = ref->next) {
struct string_list_item *item =
string_list_append(>stale, 
abbrev_branch(ref->name));
diff --git a/remote.c b/remote.c
index 4a9bddf0d..358442e4b 100644
--- a/remote.c
+++ b/remote.c
@@ -698,7 +698,9 @@ static int match_name_with_pattern(const char *key, const 
char *name,
return ret;
 }
 
-static void query_refspecs_multiple(struct refspec_item *refs, int ref_count, 
struct refspec_item *query, struct string_list *results)
+static void query_refspecs_multiple(struct refspec *rs,
+   struct refspec_item *query,
+   struct string_list *results)
 {
int i;
int find_src = !query->src;
@@ -706,8 +708,8 @@ static void query_refspecs_multiple(struct refspec_item 
*refs, int ref_count, st
if (find_src && !query->dst)
error("query_refspecs_multiple: need either src or dst");
 
-   for (i = 0; i < ref_count; i++) {
-   struct refspec_item *refspec = [i];
+   for (i = 0; i < rs->nr; i++) {
+   struct refspec_item *refspec = >items[i];
const char *key = find_src ? refspec->dst : refspec->src;
const char *value = find_src ? refspec->src : refspec->dst;
const char *needle = find_src ? query->dst : query->src;
@@ -2068,8 +2070,7 @@ struct ref *guess_remote_head(const struct ref *head,
 struct stale_heads_info {
struct string_list *ref_names;
struct ref **stale_refs_tail;
-   struct refspec_item *refs;
-   int ref_count;
+   struct refspec *rs;
 };
 
 static int get_stale_heads_cb(const char *refname, const struct object_id *oid,
@@ -2082,7 +2083,7 @@ static int get_stale_heads_cb(const char *refname, const 
struct object_id *oid,
memset(, 0, sizeof(struct refspec_item));
query.dst = (char *)refname;
 
-   query_refspecs_multiple(info->refs, info->ref_count, , );
+   query_refspecs_multiple(info->rs, , );
if (matches.nr == 0)
goto clean_exit; /* No matches */
 
@@ -2110,7 +2111,7 @@ static int get_stale_heads_cb(const char *refname, const 
struct object_id *oid,
return 0;
 }
 
-struct ref *get_stale_heads(struct refspec_item *refs, int ref_count, struct 
ref *fetch_map)
+struct ref *get_stale_heads(struct refspec *rs, struct ref *fetch_map)
 {
struct ref *ref, *stale_refs = NULL;
struct string_list ref_names = STRING_LIST_INIT_NODUP;
@@ -2118,8 +2119,7 @@ struct ref *get_stale_heads(struct refspec_item *refs, 
int ref_count, struct ref
 
info.ref_names = _names;
info.stale_refs_tail = _refs;
-   info.refs = refs;
-   info.ref_count = ref_count;
+   info.rs = rs;
for (ref = fetch_map; ref; ref = ref->next)
string_list_append(_names, ref->name);
string_list_sort(_names);
diff --git a/remote.h b/remote.h
index 4ffbc0082..1a45542cd 100644
--- a/remote.h
+++ b/remote.h
@@ -267,7 +267,7 @@ struct ref *guess_remote_head(const struct ref *head,
  int all);
 
 /* Return refs which no longer exist on remote */
-struct ref *get_stale_heads(struct refspec_item *refs, int ref_count, struct 
ref *fetch_map);
+struct ref *get_stale_heads(struct refspec *rs, struct ref *fetch_map);
 
 /*
  * Compare-and-swap
-- 
2.17.0.441.gb46fe60e1d-goog

[PATCH v2 36/36] submodule: convert push_unpushed_submodules to take a struct refspec

[Brandon Williams]

Convert 'push_unpushed_submodules()' to take a 'struct refspec' as a
parameter instead of an array of 'const char *'.

Signed-off-by: Brandon Williams 
---
 submodule.c | 19 +--
 submodule.h |  3 ++-
 transport.c |  2 +-
 3 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/submodule.c b/submodule.c
index 74d35b257..cdeadd80e 100644
--- a/submodule.c
+++ b/submodule.c
@@ -968,7 +968,7 @@ int find_unpushed_submodules(struct oid_array *commits,
 
 static int push_submodule(const char *path,
  const struct remote *remote,
- const char **refspec, int refspec_nr,
+ const struct refspec *rs,
  const struct string_list *push_options,
  int dry_run)
 {
@@ -991,8 +991,8 @@ static int push_submodule(const char *path,
if (remote->origin != REMOTE_UNCONFIGURED) {
int i;
argv_array_push(, remote->name);
-   for (i = 0; i < refspec_nr; i++)
-   argv_array_push(, refspec[i]);
+   for (i = 0; i < rs->raw_nr; i++)
+   argv_array_push(, rs->raw[i]);
}
 
prepare_submodule_repo_env(_array);
@@ -1013,7 +1013,7 @@ static int push_submodule(const char *path,
  */
 static void submodule_push_check(const char *path, const char *head,
 const struct remote *remote,
-const char **refspec, int refspec_nr)
+const struct refspec *rs)
 {
struct child_process cp = CHILD_PROCESS_INIT;
int i;
@@ -1023,8 +1023,8 @@ static void submodule_push_check(const char *path, const 
char *head,
argv_array_push(, head);
argv_array_push(, remote->name);
 
-   for (i = 0; i < refspec_nr; i++)
-   argv_array_push(, refspec[i]);
+   for (i = 0; i < rs->raw_nr; i++)
+   argv_array_push(, rs->raw[i]);
 
prepare_submodule_repo_env(_array);
cp.git_cmd = 1;
@@ -1043,7 +1043,7 @@ static void submodule_push_check(const char *path, const 
char *head,
 
 int push_unpushed_submodules(struct oid_array *commits,
 const struct remote *remote,
-const char **refspec, int refspec_nr,
+const struct refspec *rs,
 const struct string_list *push_options,
 int dry_run)
 {
@@ -1069,8 +1069,7 @@ int push_unpushed_submodules(struct oid_array *commits,
 
for (i = 0; i < needs_pushing.nr; i++)
submodule_push_check(needs_pushing.items[i].string,
-head, remote,
-refspec, refspec_nr);
+head, remote, rs);
free(head);
}
 
@@ -1078,7 +1077,7 @@ int push_unpushed_submodules(struct oid_array *commits,
for (i = 0; i < needs_pushing.nr; i++) {
const char *path = needs_pushing.items[i].string;
fprintf(stderr, "Pushing submodule '%s'\n", path);
-   if (!push_submodule(path, remote, refspec, refspec_nr,
+   if (!push_submodule(path, remote, rs,
push_options, dry_run)) {
fprintf(stderr, "Unable to push submodule '%s'\n", 
path);
ret = 0;
diff --git a/submodule.h b/submodule.h
index e5526f6aa..aae0c9c8f 100644
--- a/submodule.h
+++ b/submodule.h
@@ -100,9 +100,10 @@ extern int submodule_touches_in_range(struct object_id *a,
 extern int find_unpushed_submodules(struct oid_array *commits,
const char *remotes_name,
struct string_list *needs_pushing);
+struct refspec;
 extern int push_unpushed_submodules(struct oid_array *commits,
const struct remote *remote,
-   const char **refspec, int refspec_nr,
+   const struct refspec *rs,
const struct string_list *push_options,
int dry_run);
 /*
diff --git a/transport.c b/transport.c
index e32bc320c..7e0b9abba 100644
--- a/transport.c
+++ b/transport.c
@@ -1157,7 +1157,7 @@ int transport_push(struct transport *transport,
 
if (!push_unpushed_submodules(,
  transport->remote,
- rs->raw, rs->raw_nr,
+ rs,
  transport->push_options,
  pretend)) 

[PATCH v2 33/36] http-push: store refspecs in a struct refspec

[Brandon Williams]

Convert http-push.c to store refspecs in a 'struct refspec' instead of
in an array of 'const char *'.

Signed-off-by: Brandon Williams 
---
 http-push.c | 17 -
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/http-push.c b/http-push.c
index f308ce019..a724ef03f 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1692,8 +1692,7 @@ int cmd_main(int argc, const char **argv)
 {
struct transfer_request *request;
struct transfer_request *next_request;
-   int nr_refspec = 0;
-   const char **refspec = NULL;
+   struct refspec rs = REFSPEC_INIT_PUSH;
struct remote_lock *ref_lock = NULL;
struct remote_lock *info_ref_lock = NULL;
struct rev_info revs;
@@ -1756,8 +1755,7 @@ int cmd_main(int argc, const char **argv)
}
continue;
}
-   refspec = argv;
-   nr_refspec = argc - i;
+   refspec_appendn(, argv, argc - i);
break;
}
 
@@ -1768,7 +1766,7 @@ int cmd_main(int argc, const char **argv)
if (!repo->url)
usage(http_push_usage);
 
-   if (delete_branch && nr_refspec != 1)
+   if (delete_branch && rs.nr != 1)
die("You must specify only one branch name when deleting a 
remote branch");
 
setup_git_directory();
@@ -1814,18 +1812,19 @@ int cmd_main(int argc, const char **argv)
 
/* Remove a remote branch if -d or -D was specified */
if (delete_branch) {
-   if (delete_remote_branch(refspec[0], force_delete) == -1) {
+   const char *branch = rs.items[i].src;
+   if (delete_remote_branch(branch, force_delete) == -1) {
fprintf(stderr, "Unable to delete remote branch %s\n",
-   refspec[0]);
+   branch);
if (helper_status)
-   printf("error %s cannot remove\n", refspec[0]);
+   printf("error %s cannot remove\n", branch);
}
goto cleanup;
}
 
/* match them up */
if (match_push_refs(local_refs, _refs,
-   nr_refspec, (const char **) refspec, push_all)) {
+   rs.raw_nr, rs.raw, push_all)) {
rc = -1;
goto cleanup;
}
-- 
2.17.0.441.gb46fe60e1d-goog

[PATCH v2 32/36] transport: remove transport_verify_remote_names

[Brandon Williams]

Remove 'transprot_verify_remote_names()' because all callers have
migrated to using 'struct refspec' which performs the same checks in
'parse_refspec()'.

Signed-off-by: Brandon Williams 
---
 builtin/send-pack.c |  2 --
 transport.c | 24 
 transport.h |  2 --
 3 files changed, 28 deletions(-)

diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index ef512616f..7c34bf467 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -265,8 +265,6 @@ int cmd_send_pack(int argc, const char **argv, const char 
*prefix)
BUG("unknown protocol version");
}
 
-   transport_verify_remote_names(rs.raw_nr, rs.raw);
-
local_refs = get_local_heads();
 
flags = MATCH_REFS_NONE;
diff --git a/transport.c b/transport.c
index a89f17744..fe96c0b80 100644
--- a/transport.c
+++ b/transport.c
@@ -619,29 +619,6 @@ void transport_print_push_status(const char *dest, struct 
ref *refs,
free(head);
 }
 
-void transport_verify_remote_names(int nr_heads, const char **heads)
-{
-   int i;
-
-   for (i = 0; i < nr_heads; i++) {
-   const char *local = heads[i];
-   const char *remote = strrchr(heads[i], ':');
-
-   if (*local == '+')
-   local++;
-
-   /* A matching refspec is okay.  */
-   if (remote == local && remote[1] == '\0')
-   continue;
-
-   remote = remote ? (remote + 1) : local;
-   if (check_refname_format(remote,
-   REFNAME_ALLOW_ONELEVEL|REFNAME_REFSPEC_PATTERN))
-   die("remote part of refspec is not a valid name in %s",
-   heads[i]);
-   }
-}
-
 static int git_transport_push(struct transport *transport, struct ref 
*remote_refs, int flags)
 {
struct git_transport_data *data = transport->data;
@@ -1097,7 +1074,6 @@ int transport_push(struct transport *transport,
   unsigned int *reject_reasons)
 {
*reject_reasons = 0;
-   transport_verify_remote_names(rs->raw_nr, rs->raw);
 
if (transport_color_config() < 0)
return -1;
diff --git a/transport.h b/transport.h
index e2c809af4..bac085ce0 100644
--- a/transport.h
+++ b/transport.h
@@ -227,8 +227,6 @@ int transport_helper_init(struct transport *transport, 
const char *name);
 int bidirectional_transfer_loop(int input, int output);
 
 /* common methods used by transport.c and builtin/send-pack.c */
-void transport_verify_remote_names(int nr_heads, const char **heads);
-
 void transport_update_tracking_ref(struct remote *remote, struct ref *ref, int 
verbose);
 
 int transport_refs_pushed(struct ref *ref);
-- 
2.17.0.441.gb46fe60e1d-goog

[PATCH v2 30/36] transport: convert transport_push to take a struct refspec

[Brandon Williams]

Convert 'transport_push()' to take a 'struct refspec' as a
parameter instead of an array of strings which represent
refspecs.

Signed-off-by: Brandon Williams 
---
 builtin/push.c |  3 +--
 transport.c| 17 +++--
 transport.h|  2 +-
 3 files changed, 9 insertions(+), 13 deletions(-)

diff --git a/builtin/push.c b/builtin/push.c
index ef42979d1..9cd8e8cd5 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -355,8 +355,7 @@ static int push_with_options(struct transport *transport, 
struct refspec *rs,
 
if (verbosity > 0)
fprintf(stderr, _("Pushing to %s\n"), transport->url);
-   err = transport_push(transport, rs->raw_nr, rs->raw, flags,
-_reasons);
+   err = transport_push(transport, rs, flags, _reasons);
if (err != 0) {
fprintf(stderr, "%s", push_get_color(PUSH_COLOR_ERROR));
error(_("failed to push some refs to '%s'"), transport->url);
diff --git a/transport.c b/transport.c
index 181db4d4d..a89f17744 100644
--- a/transport.c
+++ b/transport.c
@@ -1093,11 +1093,11 @@ static int run_pre_push_hook(struct transport 
*transport,
 }
 
 int transport_push(struct transport *transport,
-  int refspec_nr, const char **refspec, int flags,
+  struct refspec *rs, int flags,
   unsigned int *reject_reasons)
 {
*reject_reasons = 0;
-   transport_verify_remote_names(refspec_nr, refspec);
+   transport_verify_remote_names(rs->raw_nr, rs->raw);
 
if (transport_color_config() < 0)
return -1;
@@ -,16 +,14 @@ int transport_push(struct transport *transport,
int porcelain = flags & TRANSPORT_PUSH_PORCELAIN;
int pretend = flags & TRANSPORT_PUSH_DRY_RUN;
int push_ret, ret, err;
-   struct refspec tmp_rs = REFSPEC_INIT_PUSH;
struct argv_array ref_prefixes = ARGV_ARRAY_INIT;
int i;
 
-   if (check_push_refs(local_refs, refspec_nr, refspec) < 0)
+   if (check_push_refs(local_refs, rs->raw_nr, rs->raw) < 0)
return -1;
 
-   refspec_appendn(_rs, refspec, refspec_nr);
-   for (i = 0; i < tmp_rs.nr; i++) {
-   const struct refspec_item *item = _rs.items[i];
+   for (i = 0; i < rs->nr; i++) {
+   const struct refspec_item *item = >items[i];
const char *prefix = NULL;
 
if (item->dst)
@@ -1143,7 +1141,6 @@ int transport_push(struct transport *transport,
   _prefixes);
 
argv_array_clear(_prefixes);
-   refspec_clear(_rs);
 
if (flags & TRANSPORT_PUSH_ALL)
match_flags |= MATCH_REFS_ALL;
@@ -1155,7 +1152,7 @@ int transport_push(struct transport *transport,
match_flags |= MATCH_REFS_FOLLOW_TAGS;
 
if (match_push_refs(local_refs, _refs,
-   refspec_nr, refspec, match_flags)) {
+   rs->raw_nr, rs->raw, match_flags)) {
return -1;
}
 
@@ -1186,7 +1183,7 @@ int transport_push(struct transport *transport,
 
if (!push_unpushed_submodules(,
  transport->remote,
- refspec, refspec_nr,
+ rs->raw, rs->raw_nr,
  transport->push_options,
  pretend)) {
oid_array_clear();
diff --git a/transport.h b/transport.h
index e783cfa07..e2c809af4 100644
--- a/transport.h
+++ b/transport.h
@@ -197,7 +197,7 @@ void transport_set_verbosity(struct transport *transport, 
int verbosity,
 #define REJECT_NEEDS_FORCE 0x10
 
 int transport_push(struct transport *connection,
-  int refspec_nr, const char **refspec, int flags,
+  struct refspec *rs, int flags,
   unsigned int * reject_reasons);
 
 /*
-- 
2.17.0.441.gb46fe60e1d-goog

[PATCH v2 34/36] remote: convert match_push_refs to take a struct refspec

[Brandon Williams]

Convert 'match_push_refs()' to take a 'struct refspec' as a parameter
instead of an array of 'const char *'.

Signed-off-by: Brandon Williams 
---
 builtin/remote.c|  3 +--
 builtin/send-pack.c |  2 +-
 http-push.c |  3 +--
 remote.c| 21 -
 remote.h|  2 +-
 transport.c |  4 +---
 6 files changed, 13 insertions(+), 22 deletions(-)

diff --git a/builtin/remote.c b/builtin/remote.c
index b8e66589f..b84175cc6 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -387,8 +387,7 @@ static int get_push_ref_states(const struct ref 
*remote_refs,
local_refs = get_local_heads();
push_map = copy_ref_list(remote_refs);
 
-   match_push_refs(local_refs, _map, remote->push.raw_nr,
-   remote->push.raw, MATCH_REFS_NONE);
+   match_push_refs(local_refs, _map, >push, MATCH_REFS_NONE);
 
states->push.strdup_strings = 1;
for (ref = push_map; ref; ref = ref->next) {
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 7c34bf467..4923b1058 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -275,7 +275,7 @@ int cmd_send_pack(int argc, const char **argv, const char 
*prefix)
flags |= MATCH_REFS_MIRROR;
 
/* match them up */
-   if (match_push_refs(local_refs, _refs, rs.raw_nr, rs.raw, flags))
+   if (match_push_refs(local_refs, _refs, , flags))
return -1;
 
if (!is_empty_cas())
diff --git a/http-push.c b/http-push.c
index a724ef03f..ea5af6227 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1823,8 +1823,7 @@ int cmd_main(int argc, const char **argv)
}
 
/* match them up */
-   if (match_push_refs(local_refs, _refs,
-   rs.raw_nr, rs.raw, push_all)) {
+   if (match_push_refs(local_refs, _refs, , push_all)) {
rc = -1;
goto cleanup;
}
diff --git a/remote.c b/remote.c
index 84dda3fd0..0046d4e28 100644
--- a/remote.c
+++ b/remote.c
@@ -1285,23 +1285,20 @@ int check_push_refs(struct ref *src, int nr_refspec, 
const char **refspec_names)
  * dst (e.g. pushing to a new branch, done in match_explicit_refs).
  */
 int match_push_refs(struct ref *src, struct ref **dst,
-   int nr_refspec, const char **refspec, int flags)
+   struct refspec *rs, int flags)
 {
-   struct refspec rs = REFSPEC_INIT_PUSH;
int send_all = flags & MATCH_REFS_ALL;
int send_mirror = flags & MATCH_REFS_MIRROR;
int send_prune = flags & MATCH_REFS_PRUNE;
int errs;
-   static const char *default_refspec[] = { ":", NULL };
struct ref *ref, **dst_tail = tail_ref(dst);
struct string_list dst_ref_index = STRING_LIST_INIT_NODUP;
 
-   if (!nr_refspec) {
-   nr_refspec = 1;
-   refspec = default_refspec;
-   }
-   refspec_appendn(, refspec, nr_refspec);
-   errs = match_explicit_refs(src, *dst, _tail, );
+   /* If no refspec is provided, use the default ":" */
+   if (!rs->nr)
+   refspec_append(rs, ":");
+
+   errs = match_explicit_refs(src, *dst, _tail, rs);
 
/* pick the remainder */
for (ref = src; ref; ref = ref->next) {
@@ -1310,7 +1307,7 @@ int match_push_refs(struct ref *src, struct ref **dst,
const struct refspec_item *pat = NULL;
char *dst_name;
 
-   dst_name = get_ref_match(, ref, send_mirror, FROM_SRC, );
+   dst_name = get_ref_match(rs, ref, send_mirror, FROM_SRC, );
if (!dst_name)
continue;
 
@@ -1359,7 +1356,7 @@ int match_push_refs(struct ref *src, struct ref **dst,
/* We're already sending something to this ref. 
*/
continue;
 
-   src_name = get_ref_match(, ref, send_mirror, 
FROM_DST, NULL);
+   src_name = get_ref_match(rs, ref, send_mirror, 
FROM_DST, NULL);
if (src_name) {
if (!src_ref_index.nr)
prepare_ref_index(_ref_index, src);
@@ -1372,8 +1369,6 @@ int match_push_refs(struct ref *src, struct ref **dst,
string_list_clear(_ref_index, 0);
}
 
-   refspec_clear();
-
if (errs)
return -1;
return 0;
diff --git a/remote.h b/remote.h
index 9050ff75a..74c557457 100644
--- a/remote.h
+++ b/remote.h
@@ -163,7 +163,7 @@ char *apply_refspecs(struct refspec *rs, const char *name);
 
 int check_push_refs(struct ref *src, int nr_refspec, const char **refspec);
 int match_push_refs(struct ref *src, struct ref **dst,
-   int nr_refspec, const char **refspec, int all);
+   struct refspec *rs, int flags);
 void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
int force_update);
 
diff --git 

[PATCH v2 35/36] remote: convert check_push_refs to take a struct refspec

[Brandon Williams]

Convert 'check_push_refs()' to take a 'struct refspec' as a parameter
instead of an array of 'const char *'.

Signed-off-by: Brandon Williams 
---
 remote.c| 14 +-
 remote.h|  2 +-
 transport.c |  2 +-
 3 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/remote.c b/remote.c
index 0046d4e28..0d1a3d07f 100644
--- a/remote.c
+++ b/remote.c
@@ -1255,24 +1255,20 @@ static void prepare_ref_index(struct string_list 
*ref_index, struct ref *ref)
  * but we can catch some errors early before even talking to the
  * remote side.
  */
-int check_push_refs(struct ref *src, int nr_refspec, const char 
**refspec_names)
+int check_push_refs(struct ref *src, struct refspec *rs)
 {
-   struct refspec refspec = REFSPEC_INIT_PUSH;
int ret = 0;
int i;
 
-   refspec_appendn(, refspec_names, nr_refspec);
-
-   for (i = 0; i < refspec.nr; i++) {
-   struct refspec_item *rs = [i];
+   for (i = 0; i < rs->nr; i++) {
+   struct refspec_item *item = >items[i];
 
-   if (rs->pattern || rs->matching)
+   if (item->pattern || item->matching)
continue;
 
-   ret |= match_explicit_lhs(src, rs, NULL, NULL);
+   ret |= match_explicit_lhs(src, item, NULL, NULL);
}
 
-   refspec_clear();
return ret;
 }
 
diff --git a/remote.h b/remote.h
index 74c557457..62a656659 100644
--- a/remote.h
+++ b/remote.h
@@ -161,7 +161,7 @@ struct ref *ref_remove_duplicates(struct ref *ref_map);
 int query_refspecs(struct refspec *rs, struct refspec_item *query);
 char *apply_refspecs(struct refspec *rs, const char *name);
 
-int check_push_refs(struct ref *src, int nr_refspec, const char **refspec);
+int check_push_refs(struct ref *src, struct refspec *rs);
 int match_push_refs(struct ref *src, struct ref **dst,
struct refspec *rs, int flags);
 void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
diff --git a/transport.c b/transport.c
index 24a97d9e8..e32bc320c 100644
--- a/transport.c
+++ b/transport.c
@@ -1090,7 +1090,7 @@ int transport_push(struct transport *transport,
struct argv_array ref_prefixes = ARGV_ARRAY_INIT;
int i;
 
-   if (check_push_refs(local_refs, rs->raw_nr, rs->raw) < 0)
+   if (check_push_refs(local_refs, rs) < 0)
return -1;
 
for (i = 0; i < rs->nr; i++) {
-- 
2.17.0.441.gb46fe60e1d-goog

[PATCH v2 25/36] remote: convert query_refspecs to take a struct refspec

[Brandon Williams]

Convert 'query_refspecs()' to take a 'struct refspec' as a parameter instead
of a list of 'struct refspec_item'.

Signed-off-by: Brandon Williams 
---
 builtin/push.c |  3 +--
 remote.c   | 10 +-
 remote.h   |  2 +-
 3 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/builtin/push.c b/builtin/push.c
index 509dc6677..6b7e45890 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -83,8 +83,7 @@ static const char *map_refspec(const char *ref,
struct refspec_item query;
memset(, 0, sizeof(struct refspec_item));
query.src = matched->name;
-   if (!query_refspecs(remote->push.items, remote->push.nr, 
) &&
-   query.dst) {
+   if (!query_refspecs(>push, ) && query.dst) {
struct strbuf buf = STRBUF_INIT;
strbuf_addf(, "%s%s:%s",
query.force ? "+" : "",
diff --git a/remote.c b/remote.c
index 89415a263..dd68e6b22 100644
--- a/remote.c
+++ b/remote.c
@@ -725,7 +725,7 @@ static void query_refspecs_multiple(struct refspec *rs,
}
 }
 
-int query_refspecs(struct refspec_item *refs, int ref_count, struct 
refspec_item *query)
+int query_refspecs(struct refspec *rs, struct refspec_item *query)
 {
int i;
int find_src = !query->src;
@@ -735,8 +735,8 @@ int query_refspecs(struct refspec_item *refs, int 
ref_count, struct refspec_item
if (find_src && !query->dst)
return error("query_refspecs: need either src or dst");
 
-   for (i = 0; i < ref_count; i++) {
-   struct refspec_item *refspec = [i];
+   for (i = 0; i < rs->nr; i++) {
+   struct refspec_item *refspec = >items[i];
const char *key = find_src ? refspec->dst : refspec->src;
const char *value = find_src ? refspec->src : refspec->dst;
 
@@ -763,7 +763,7 @@ char *apply_refspecs(struct refspec *rs, const char *name)
memset(, 0, sizeof(struct refspec_item));
query.src = (char *)name;
 
-   if (query_refspecs(rs->items, rs->nr, ))
+   if (query_refspecs(rs, ))
return NULL;
 
return query.dst;
@@ -771,7 +771,7 @@ char *apply_refspecs(struct refspec *rs, const char *name)
 
 int remote_find_tracking(struct remote *remote, struct refspec_item *refspec)
 {
-   return query_refspecs(remote->fetch.items, remote->fetch.nr, refspec);
+   return query_refspecs(>fetch, refspec);
 }
 
 static struct ref *alloc_ref_with_prefix(const char *prefix, size_t prefixlen,
diff --git a/remote.h b/remote.h
index 0b1fcc051..9050ff75a 100644
--- a/remote.h
+++ b/remote.h
@@ -158,7 +158,7 @@ int ref_newer(const struct object_id *new_oid, const struct 
object_id *old_oid);
  */
 struct ref *ref_remove_duplicates(struct ref *ref_map);
 
-extern int query_refspecs(struct refspec_item *specs, int nr, struct 
refspec_item *query);
+int query_refspecs(struct refspec *rs, struct refspec_item *query);
 char *apply_refspecs(struct refspec *rs, const char *name);
 
 int check_push_refs(struct ref *src, int nr_refspec, const char **refspec);
-- 
2.17.0.441.gb46fe60e1d-goog

[PATCH v2 17/36] fetch: convert fetch_one to use struct refspec

[Brandon Williams]

Convert 'fetch_one()' to use 'struct refspec'.

Signed-off-by: Brandon Williams 
---
 builtin/fetch.c | 46 +++---
 1 file changed, 19 insertions(+), 27 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 7a1637d35..18704c6cd 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1356,10 +1356,8 @@ static inline void fetch_one_setup_partial(struct remote 
*remote)
 
 static int fetch_one(struct remote *remote, int argc, const char **argv, int 
prune_tags_ok)
 {
-   static const char **refs = NULL;
-   struct refspec_item *refspec;
-   int ref_nr = 0;
-   int j = 0;
+   struct refspec rs = REFSPEC_INIT_FETCH;
+   int i;
int exit_code;
int maybe_prune_tags;
int remote_via_config = remote_is_configured(remote, 0);
@@ -1394,35 +1392,29 @@ static int fetch_one(struct remote *remote, int argc, 
const char **argv, int pru
if (maybe_prune_tags && remote_via_config)
refspec_append(>fetch, TAG_REFSPEC);
 
-   if (argc > 0 || (maybe_prune_tags && !remote_via_config)) {
-   size_t nr_alloc = st_add3(argc, maybe_prune_tags, 1);
-   refs = xcalloc(nr_alloc, sizeof(const char *));
-   if (maybe_prune_tags) {
-   refs[j++] = xstrdup("refs/tags/*:refs/tags/*");
-   ref_nr++;
-   }
-   }
+   if (maybe_prune_tags && (argc || !remote_via_config))
+   refspec_append(, TAG_REFSPEC);
 
-   if (argc > 0) {
-   int i;
-   for (i = 0; i < argc; i++) {
-   if (!strcmp(argv[i], "tag")) {
-   i++;
-   if (i >= argc)
-   die(_("You need to specify a tag 
name."));
-   refs[j++] = xstrfmt("refs/tags/%s:refs/tags/%s",
-   argv[i], argv[i]);
-   } else
-   refs[j++] = argv[i];
-   ref_nr++;
+   for (i = 0; i < argc; i++) {
+   if (!strcmp(argv[i], "tag")) {
+   char *tag;
+   i++;
+   if (i >= argc)
+   die(_("You need to specify a tag name."));
+
+   tag = xstrfmt("refs/tags/%s:refs/tags/%s",
+ argv[i], argv[i]);
+   refspec_append(, tag);
+   free(tag);
+   } else {
+   refspec_append(, argv[i]);
}
}
 
sigchain_push_common(unlock_pack_on_signal);
atexit(unlock_pack);
-   refspec = parse_fetch_refspec(ref_nr, refs);
-   exit_code = do_fetch(gtransport, refspec, ref_nr);
-   free_refspec(ref_nr, refspec);
+   exit_code = do_fetch(gtransport, rs.items, rs.nr);
+   refspec_clear();
transport_disconnect(gtransport);
gtransport = NULL;
return exit_code;
-- 
2.17.0.441.gb46fe60e1d-goog

[PATCH v2 28/36] push: check for errors earlier

[Brandon Williams]

Move the error checking for using the "--mirror", "--all", and "--tags"
options earlier and explicitly check for the presence of the flags
instead of checking for a side-effect of the flag.

Signed-off-by: Brandon Williams 
---
 builtin/push.c | 31 ++-
 1 file changed, 14 insertions(+), 17 deletions(-)

diff --git a/builtin/push.c b/builtin/push.c
index 6b7e45890..df5df6c0d 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -417,23 +417,6 @@ static int do_push(const char *repo, int flags,
if (push_options->nr)
flags |= TRANSPORT_PUSH_OPTIONS;
 
-   if ((flags & TRANSPORT_PUSH_ALL) && refspec) {
-   if (!strcmp(*refspec, "refs/tags/*"))
-   return error(_("--all and --tags are incompatible"));
-   return error(_("--all can't be combined with refspecs"));
-   }
-
-   if ((flags & TRANSPORT_PUSH_MIRROR) && refspec) {
-   if (!strcmp(*refspec, "refs/tags/*"))
-   return error(_("--mirror and --tags are incompatible"));
-   return error(_("--mirror can't be combined with refspecs"));
-   }
-
-   if ((flags & (TRANSPORT_PUSH_ALL|TRANSPORT_PUSH_MIRROR)) ==
-   (TRANSPORT_PUSH_ALL|TRANSPORT_PUSH_MIRROR)) {
-   return error(_("--all and --mirror are incompatible"));
-   }
-
if (!refspec && !(flags & TRANSPORT_PUSH_ALL)) {
if (remote->push.raw_nr) {
refspec = remote->push.raw;
@@ -625,6 +608,20 @@ int cmd_push(int argc, const char **argv, const char 
*prefix)
die(_("--delete is incompatible with --all, --mirror and 
--tags"));
if (deleterefs && argc < 2)
die(_("--delete doesn't make sense without any refs"));
+   if (flags & TRANSPORT_PUSH_ALL) {
+   if (tags)
+   die(_("--all and --tags are incompatible"));
+   if (argc >= 2)
+   die(_("--all can't be combined with refspecs"));
+   }
+   if (flags & TRANSPORT_PUSH_MIRROR) {
+   if (tags)
+   die(_("--mirror and --tags are incompatible"));
+   if (argc >= 2)
+   die(_("--mirror can't be combined with refspecs"));
+   }
+   if ((flags & TRANSPORT_PUSH_ALL) && (flags & TRANSPORT_PUSH_MIRROR))
+   die(_("--all and --mirror are incompatible"));
 
if (recurse_submodules == RECURSE_SUBMODULES_CHECK)
flags |= TRANSPORT_RECURSE_SUBMODULES_CHECK;
-- 
2.17.0.441.gb46fe60e1d-goog

[PATCH v2 15/36] remote: remove add_prune_tags_to_fetch_refspec

[Brandon Williams]

Remove 'add_prune_tags_to_fetch_refspec()' function and instead have the
only caller directly add the tag refspec using 'refspec_append()'.

Signed-off-by: Brandon Williams 
---
 builtin/fetch.c | 2 +-
 remote.c| 5 -
 remote.h| 2 --
 3 files changed, 1 insertion(+), 8 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 30083d4bc..7a1637d35 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1392,7 +1392,7 @@ static int fetch_one(struct remote *remote, int argc, 
const char **argv, int pru
 
maybe_prune_tags = prune_tags_ok && prune_tags;
if (maybe_prune_tags && remote_via_config)
-   add_prune_tags_to_fetch_refspec(remote);
+   refspec_append(>fetch, TAG_REFSPEC);
 
if (argc > 0 || (maybe_prune_tags && !remote_via_config)) {
size_t nr_alloc = st_add3(argc, maybe_prune_tags, 1);
diff --git a/remote.c b/remote.c
index 26842ce37..4a9bddf0d 100644
--- a/remote.c
+++ b/remote.c
@@ -77,11 +77,6 @@ static const char *alias_url(const char *url, struct 
rewrites *r)
return xstrfmt("%s%s", r->rewrite[longest_i]->base, url + longest->len);
 }
 
-void add_prune_tags_to_fetch_refspec(struct remote *remote)
-{
-   refspec_append(>fetch, TAG_REFSPEC);
-}
-
 static void add_url(struct remote *remote, const char *url)
 {
ALLOC_GROW(remote->url, remote->url_nr + 1, remote->url_alloc);
diff --git a/remote.h b/remote.h
index e7d00fe2a..4ffbc0082 100644
--- a/remote.h
+++ b/remote.h
@@ -290,6 +290,4 @@ extern int parseopt_push_cas_option(const struct option *, 
const char *arg, int
 extern int is_empty_cas(const struct push_cas_option *);
 void apply_push_cas(struct push_cas_option *, struct remote *, struct ref *);
 
-void add_prune_tags_to_fetch_refspec(struct remote *remote);
-
 #endif
-- 
2.17.0.441.gb46fe60e1d-goog

[PATCH v2 27/36] remote: convert match_explicit_refs to take a struct refspec

[Brandon Williams]

Convert 'match_explicit_refs()' to take a 'struct refspec' as a
parameter instead of a list of 'struct refspec_item'.

Signed-off-by: Brandon Williams 
---
 remote.c | 9 -
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/remote.c b/remote.c
index 9eb79ea19..84dda3fd0 100644
--- a/remote.c
+++ b/remote.c
@@ -1073,12 +1073,11 @@ static int match_explicit(struct ref *src, struct ref 
*dst,
 }
 
 static int match_explicit_refs(struct ref *src, struct ref *dst,
-  struct ref ***dst_tail, struct refspec_item *rs,
-  int rs_nr)
+  struct ref ***dst_tail, struct refspec *rs)
 {
int i, errs;
-   for (i = errs = 0; i < rs_nr; i++)
-   errs += match_explicit(src, dst, dst_tail, [i]);
+   for (i = errs = 0; i < rs->nr; i++)
+   errs += match_explicit(src, dst, dst_tail, >items[i]);
return errs;
 }
 
@@ -1302,7 +1301,7 @@ int match_push_refs(struct ref *src, struct ref **dst,
refspec = default_refspec;
}
refspec_appendn(, refspec, nr_refspec);
-   errs = match_explicit_refs(src, *dst, _tail, rs.items, rs.nr);
+   errs = match_explicit_refs(src, *dst, _tail, );
 
/* pick the remainder */
for (ref = src; ref; ref = ref->next) {
-- 
2.17.0.441.gb46fe60e1d-goog

[PATCH v2 26/36] remote: convert get_ref_match to take a struct refspec

[Brandon Williams]

Convert 'get_ref_match()' to take a 'struct refspec' as a parameter
instead of a list of 'struct refspec_item'.

Signed-off-by: Brandon Williams 
---
 remote.c | 26 ++
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/remote.c b/remote.c
index dd68e6b22..9eb79ea19 100644
--- a/remote.c
+++ b/remote.c
@@ -1082,27 +1082,29 @@ static int match_explicit_refs(struct ref *src, struct 
ref *dst,
return errs;
 }
 
-static char *get_ref_match(const struct refspec_item *rs, int rs_nr, const 
struct ref *ref,
-   int send_mirror, int direction, const struct refspec_item 
**ret_pat)
+static char *get_ref_match(const struct refspec *rs, const struct ref *ref,
+  int send_mirror, int direction,
+  const struct refspec_item **ret_pat)
 {
const struct refspec_item *pat;
char *name;
int i;
int matching_refs = -1;
-   for (i = 0; i < rs_nr; i++) {
-   if (rs[i].matching &&
-   (matching_refs == -1 || rs[i].force)) {
+   for (i = 0; i < rs->nr; i++) {
+   const struct refspec_item *item = >items[i];
+   if (item->matching &&
+   (matching_refs == -1 || item->force)) {
matching_refs = i;
continue;
}
 
-   if (rs[i].pattern) {
-   const char *dst_side = rs[i].dst ? rs[i].dst : 
rs[i].src;
+   if (item->pattern) {
+   const char *dst_side = item->dst ? item->dst : 
item->src;
int match;
if (direction == FROM_SRC)
-   match = match_name_with_pattern(rs[i].src, 
ref->name, dst_side, );
+   match = match_name_with_pattern(item->src, 
ref->name, dst_side, );
else
-   match = match_name_with_pattern(dst_side, 
ref->name, rs[i].src, );
+   match = match_name_with_pattern(dst_side, 
ref->name, item->src, );
if (match) {
matching_refs = i;
break;
@@ -1112,7 +1114,7 @@ static char *get_ref_match(const struct refspec_item *rs, 
int rs_nr, const struc
if (matching_refs == -1)
return NULL;
 
-   pat = rs + matching_refs;
+   pat = >items[matching_refs];
if (pat->matching) {
/*
 * "matching refs"; traditionally we pushed everything
@@ -1309,7 +1311,7 @@ int match_push_refs(struct ref *src, struct ref **dst,
const struct refspec_item *pat = NULL;
char *dst_name;
 
-   dst_name = get_ref_match(rs.items, rs.nr, ref, send_mirror, 
FROM_SRC, );
+   dst_name = get_ref_match(, ref, send_mirror, FROM_SRC, );
if (!dst_name)
continue;
 
@@ -1358,7 +1360,7 @@ int match_push_refs(struct ref *src, struct ref **dst,
/* We're already sending something to this ref. 
*/
continue;
 
-   src_name = get_ref_match(rs.items, rs.nr, ref, 
send_mirror, FROM_DST, NULL);
+   src_name = get_ref_match(, ref, send_mirror, 
FROM_DST, NULL);
if (src_name) {
if (!src_ref_index.nr)
prepare_ref_index(_ref_index, src);
-- 
2.17.0.441.gb46fe60e1d-goog

[PATCH v2 24/36] remote: convert apply_refspecs to take a struct refspec

[Brandon Williams]

Convert 'apply_refspecs()' to take a 'struct refspec' as a parameter instead
of a list of 'struct refspec_item'.

Signed-off-by: Brandon Williams 
---
 builtin/fast-export.c |  2 +-
 remote.c  | 15 ++-
 remote.h  |  3 +--
 transport-helper.c|  6 +++---
 4 files changed, 11 insertions(+), 15 deletions(-)

diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 143999738..41fe49e4d 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -831,7 +831,7 @@ static void get_tags_and_duplicates(struct rev_cmdline_info 
*info)
 
if (refspecs.nr) {
char *private;
-   private = apply_refspecs(refspecs.items, refspecs.nr, 
full_name);
+   private = apply_refspecs(, full_name);
if (private) {
free(full_name);
full_name = private;
diff --git a/remote.c b/remote.c
index 358442e4b..89415a263 100644
--- a/remote.c
+++ b/remote.c
@@ -525,8 +525,7 @@ const char *remote_ref_for_branch(struct branch *branch, 
int for_push,
struct remote *remote = remote_get(remote_name);
 
if (remote && remote->push.nr &&
-   (dst = apply_refspecs(remote->push.items,
- remote->push.nr,
+   (dst = apply_refspecs(>push,
  branch->refname))) {
if (explicit)
*explicit = 1;
@@ -757,15 +756,14 @@ int query_refspecs(struct refspec_item *refs, int 
ref_count, struct refspec_item
return -1;
 }
 
-char *apply_refspecs(struct refspec_item *refspecs, int nr_refspec,
-const char *name)
+char *apply_refspecs(struct refspec *rs, const char *name)
 {
struct refspec_item query;
 
memset(, 0, sizeof(struct refspec_item));
query.src = (char *)name;
 
-   if (query_refspecs(refspecs, nr_refspec, ))
+   if (query_refspecs(rs->items, rs->nr, ))
return NULL;
 
return query.dst;
@@ -1571,7 +1569,7 @@ static const char *tracking_for_push_dest(struct remote 
*remote,
 {
char *ret;
 
-   ret = apply_refspecs(remote->fetch.items, remote->fetch.nr, refname);
+   ret = apply_refspecs(>fetch, refname);
if (!ret)
return error_buf(err,
 _("push destination '%s' on remote '%s' has no 
local tracking branch"),
@@ -1593,8 +1591,7 @@ static const char *branch_get_push_1(struct branch 
*branch, struct strbuf *err)
char *dst;
const char *ret;
 
-   dst = apply_refspecs(remote->push.items, remote->push.nr,
-branch->refname);
+   dst = apply_refspecs(>push, branch->refname);
if (!dst)
return error_buf(err,
 _("push refspecs for '%s' do not 
include '%s'"),
@@ -2203,7 +2200,7 @@ static int remote_tracking(struct remote *remote, const 
char *refname,
 {
char *dst;
 
-   dst = apply_refspecs(remote->fetch.items, remote->fetch.nr, refname);
+   dst = apply_refspecs(>fetch, refname);
if (!dst)
return -1; /* no tracking ref for refname at remote */
if (read_ref(dst, oid))
diff --git a/remote.h b/remote.h
index 1a45542cd..0b1fcc051 100644
--- a/remote.h
+++ b/remote.h
@@ -159,8 +159,7 @@ int ref_newer(const struct object_id *new_oid, const struct 
object_id *old_oid);
 struct ref *ref_remove_duplicates(struct ref *ref_map);
 
 extern int query_refspecs(struct refspec_item *specs, int nr, struct 
refspec_item *query);
-char *apply_refspecs(struct refspec_item *refspecs, int nr_refspec,
-const char *name);
+char *apply_refspecs(struct refspec *rs, const char *name);
 
 int check_push_refs(struct ref *src, int nr_refspec, const char **refspec);
 int match_push_refs(struct ref *src, struct ref **dst,
diff --git a/transport-helper.c b/transport-helper.c
index 33f51ebfc..1f8ff7e94 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -523,7 +523,7 @@ static int fetch_with_import(struct transport *transport,
continue;
name = posn->symref ? posn->symref : posn->name;
if (data->rs.nr)
-   private = apply_refspecs(data->rs.items, data->rs.nr, 
name);
+   private = apply_refspecs(>rs, name);
else
private = xstrdup(name);
if (private) {
@@ -805,7 +805,7 @@ static int push_update_refs_status(struct helper_data *data,
continue;
 
/* propagate back the update to the remote namespace */
-   private = 

[PATCH v2 21/36] fetch: convert get_ref_map to take a struct refspec

[Brandon Williams]

Convert 'get_ref_map()' to take a 'struct refspec' as a parameter
instead of a list of 'struct refspec_item'.

Signed-off-by: Brandon Williams 
---
 builtin/fetch.c | 43 ---
 1 file changed, 20 insertions(+), 23 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index ec54b1dfe..836eb7545 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -337,7 +337,7 @@ static void find_non_local_tags(struct transport *transport,
 }
 
 static struct ref *get_ref_map(struct transport *transport,
-  struct refspec_item *refspecs, int refspec_count,
+  struct refspec *rs,
   int tags, int *autotags)
 {
int i;
@@ -351,15 +351,16 @@ static struct ref *get_ref_map(struct transport 
*transport,
 
const struct ref *remote_refs;
 
-   for (i = 0; i < refspec_count; i++) {
-   if (!refspecs[i].exact_sha1) {
-   const char *glob = strchr(refspecs[i].src, '*');
+   for (i = 0; i < rs->nr; i++) {
+   const struct refspec_item *item = >items[i];
+   if (!item->exact_sha1) {
+   const char *glob = strchr(item->src, '*');
if (glob)
argv_array_pushf(_prefixes, "%.*s",
-(int)(glob - refspecs[i].src),
-refspecs[i].src);
+(int)(glob - item->src),
+item->src);
else
-   expand_ref_prefix(_prefixes, 
refspecs[i].src);
+   expand_ref_prefix(_prefixes, item->src);
}
}
 
@@ -367,13 +368,12 @@ static struct ref *get_ref_map(struct transport 
*transport,
 
argv_array_clear(_prefixes);
 
-   if (refspec_count) {
-   struct refspec_item *fetch_refspec;
-   int fetch_refspec_nr;
+   if (rs->nr) {
+   struct refspec *fetch_refspec;
 
-   for (i = 0; i < refspec_count; i++) {
-   get_fetch_map(remote_refs, [i], , 0);
-   if (refspecs[i].dst && refspecs[i].dst[0])
+   for (i = 0; i < rs->nr; i++) {
+   get_fetch_map(remote_refs, >items[i], , 0);
+   if (rs->items[i].dst && rs->items[i].dst[0])
*autotags = 1;
}
/* Merge everything on the command line (but not --tags) */
@@ -400,16 +400,13 @@ static struct ref *get_ref_map(struct transport 
*transport,
 * by ref_remove_duplicates() in favor of one of these
 * opportunistic entries with FETCH_HEAD_IGNORE.
 */
-   if (refmap.nr) {
-   fetch_refspec = refmap.items;
-   fetch_refspec_nr = refmap.nr;
-   } else {
-   fetch_refspec = transport->remote->fetch.items;
-   fetch_refspec_nr = transport->remote->fetch.nr;
-   }
+   if (refmap.nr)
+   fetch_refspec = 
+   else
+   fetch_refspec = >remote->fetch;
 
-   for (i = 0; i < fetch_refspec_nr; i++)
-   get_fetch_map(ref_map, _refspec[i], _tail, 
1);
+   for (i = 0; i < fetch_refspec->nr; i++)
+   get_fetch_map(ref_map, _refspec->items[i], 
_tail, 1);
} else if (refmap.nr) {
die("--refmap option is only meaningful with command-line 
refspec(s).");
} else {
@@ -1136,7 +1133,7 @@ static int do_fetch(struct transport *transport,
goto cleanup;
}
 
-   ref_map = get_ref_map(transport, rs->items, rs->nr, tags, );
+   ref_map = get_ref_map(transport, rs, tags, );
if (!update_head_ok)
check_not_current_branch(ref_map);
 
-- 
2.17.0.441.gb46fe60e1d-goog

[PATCH v2 14/36] remote: convert fetch refspecs to struct refspec

[Brandon Williams]

Convert the set of fetch refspecs stored in 'struct remote' to use
'struct refspec'.

Signed-off-by: Brandon Williams 
---
 builtin/fetch.c  | 20 ++--
 builtin/remote.c | 18 +-
 remote.c | 38 --
 remote.h |  5 +
 4 files changed, 32 insertions(+), 49 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 745020a10..30083d4bc 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -407,8 +407,8 @@ static struct ref *get_ref_map(struct transport *transport,
fetch_refspec = parse_fetch_refspec(refmap_nr, 
refmap_array);
fetch_refspec_nr = refmap_nr;
} else {
-   fetch_refspec = transport->remote->fetch;
-   fetch_refspec_nr = transport->remote->fetch_refspec_nr;
+   fetch_refspec = transport->remote->fetch.items;
+   fetch_refspec_nr = transport->remote->fetch.nr;
}
 
for (i = 0; i < fetch_refspec_nr; i++)
@@ -421,16 +421,16 @@ static struct ref *get_ref_map(struct transport 
*transport,
struct branch *branch = branch_get(NULL);
int has_merge = branch_has_merge_config(branch);
if (remote &&
-   (remote->fetch_refspec_nr ||
+   (remote->fetch.nr ||
 /* Note: has_merge implies non-NULL branch->remote_name */
 (has_merge && !strcmp(branch->remote_name, 
remote->name {
-   for (i = 0; i < remote->fetch_refspec_nr; i++) {
-   get_fetch_map(remote_refs, >fetch[i], 
, 0);
-   if (remote->fetch[i].dst &&
-   remote->fetch[i].dst[0])
+   for (i = 0; i < remote->fetch.nr; i++) {
+   get_fetch_map(remote_refs, 
>fetch.items[i], , 0);
+   if (remote->fetch.items[i].dst &&
+   remote->fetch.items[i].dst[0])
*autotags = 1;
if (!i && !has_merge && ref_map &&
-   !remote->fetch[0].pattern)
+   !remote->fetch.items[0].pattern)
ref_map->fetch_head_status = 
FETCH_HEAD_MERGE;
}
/*
@@ -1166,8 +1166,8 @@ static int do_fetch(struct transport *transport,
if (ref_count) {
prune_refs(refs, ref_count, ref_map, transport->url);
} else {
-   prune_refs(transport->remote->fetch,
-  transport->remote->fetch_refspec_nr,
+   prune_refs(transport->remote->fetch.items,
+  transport->remote->fetch.nr,
   ref_map,
   transport->url);
}
diff --git a/builtin/remote.c b/builtin/remote.c
index fb84729d6..94dfcb78b 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -333,10 +333,10 @@ static int get_ref_states(const struct ref *remote_refs, 
struct ref_states *stat
struct ref *ref, *stale_refs;
int i;
 
-   for (i = 0; i < states->remote->fetch_refspec_nr; i++)
-   if (get_fetch_map(remote_refs, states->remote->fetch + i, 
, 1))
+   for (i = 0; i < states->remote->fetch.nr; i++)
+   if (get_fetch_map(remote_refs, >remote->fetch.items[i], 
, 1))
die(_("Could not get fetch map for refspec %s"),
-   states->remote->fetch_refspec[i]);
+   states->remote->fetch.raw[i]);
 
states->new_refs.strdup_strings = 1;
states->tracked.strdup_strings = 1;
@@ -347,8 +347,8 @@ static int get_ref_states(const struct ref *remote_refs, 
struct ref_states *stat
else
string_list_append(>tracked, 
abbrev_branch(ref->name));
}
-   stale_refs = get_stale_heads(states->remote->fetch,
-states->remote->fetch_refspec_nr, 
fetch_map);
+   stale_refs = get_stale_heads(states->remote->fetch.items,
+states->remote->fetch.nr, fetch_map);
for (ref = stale_refs; ref; ref = ref->next) {
struct string_list_item *item =
string_list_append(>stale, 
abbrev_branch(ref->name));
@@ -590,8 +590,8 @@ static int migrate_file(struct remote *remote)
git_config_set_multivar(buf.buf, remote->push.raw[i], "^$", 0);
strbuf_reset();
strbuf_addf(, "remote.%s.fetch", remote->name);
-   for (i = 0; i < remote->fetch_refspec_nr; i++)
-   

[PATCH v2 20/36] fetch: convert do_fetch to take a struct refspec

[Brandon Williams]

Convert 'do_fetch()' to take a 'struct refspec' as a parameter instead
of a list of 'struct refspec_item'.

Signed-off-by: Brandon Williams 
---
 builtin/fetch.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 6b909cd5b..ec54b1dfe 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1112,7 +1112,7 @@ static void backfill_tags(struct transport *transport, 
struct ref *ref_map)
 }
 
 static int do_fetch(struct transport *transport,
-   struct refspec_item *refs, int ref_count)
+   struct refspec *rs)
 {
struct string_list existing_refs = STRING_LIST_INIT_DUP;
struct ref *ref_map;
@@ -1136,7 +1136,7 @@ static int do_fetch(struct transport *transport,
goto cleanup;
}
 
-   ref_map = get_ref_map(transport, refs, ref_count, tags, );
+   ref_map = get_ref_map(transport, rs->items, rs->nr, tags, );
if (!update_head_ok)
check_not_current_branch(ref_map);
 
@@ -1160,8 +1160,8 @@ static int do_fetch(struct transport *transport,
 * explicitly (via command line or configuration); we
 * don't care whether --tags was specified.
 */
-   if (ref_count) {
-   prune_refs(refs, ref_count, ref_map, transport->url);
+   if (rs->nr) {
+   prune_refs(rs->items, rs->nr, ref_map, transport->url);
} else {
prune_refs(transport->remote->fetch.items,
   transport->remote->fetch.nr,
@@ -1410,7 +1410,7 @@ static int fetch_one(struct remote *remote, int argc, 
const char **argv, int pru
 
sigchain_push_common(unlock_pack_on_signal);
atexit(unlock_pack);
-   exit_code = do_fetch(gtransport, rs.items, rs.nr);
+   exit_code = do_fetch(gtransport, );
refspec_clear();
transport_disconnect(gtransport);
gtransport = NULL;
-- 
2.17.0.441.gb46fe60e1d-goog

[PATCH v2 09/36] remote: convert check_push_refs to use struct refspec

[Brandon Williams]

Convert 'check_push_refs()' to use 'struct refspec'.

Signed-off-by: Brandon Williams 
---
 remote.c | 10 ++
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/remote.c b/remote.c
index 89820c476..191855118 100644
--- a/remote.c
+++ b/remote.c
@@ -1282,12 +1282,14 @@ static void prepare_ref_index(struct string_list 
*ref_index, struct ref *ref)
  */
 int check_push_refs(struct ref *src, int nr_refspec, const char 
**refspec_names)
 {
-   struct refspec_item *refspec = parse_push_refspec(nr_refspec, 
refspec_names);
+   struct refspec refspec = REFSPEC_INIT_PUSH;
int ret = 0;
int i;
 
-   for (i = 0; i < nr_refspec; i++) {
-   struct refspec_item *rs = refspec + i;
+   refspec_appendn(, refspec_names, nr_refspec);
+
+   for (i = 0; i < refspec.nr; i++) {
+   struct refspec_item *rs = [i];
 
if (rs->pattern || rs->matching)
continue;
@@ -1295,7 +1297,7 @@ int check_push_refs(struct ref *src, int nr_refspec, 
const char **refspec_names)
ret |= match_explicit_lhs(src, rs, NULL, NULL);
}
 
-   free_refspec(nr_refspec, refspec);
+   refspec_clear();
return ret;
 }
 
-- 
2.17.0.441.gb46fe60e1d-goog

[PATCH v2 19/36] refspec: remove the deprecated functions

[Brandon Williams]

Now that there are no callers of 'parse_push_refspec()',
'parse_fetch_refspec()', and 'free_refspec()', remove these
functions.

Signed-off-by: Brandon Williams 
---
 refspec.c | 49 -
 refspec.h |  5 -
 2 files changed, 54 deletions(-)

diff --git a/refspec.c b/refspec.c
index ab37b5ba1..97e76e8b1 100644
--- a/refspec.c
+++ b/refspec.c
@@ -121,55 +121,6 @@ static int parse_refspec(struct refspec_item *item, const 
char *refspec, int fet
return 1;
 }
 
-static struct refspec_item *parse_refspec_internal(int nr_refspec, const char 
**refspec, int fetch, int verify)
-{
-   int i;
-   struct refspec_item *rs = xcalloc(nr_refspec, sizeof(*rs));
-
-   for (i = 0; i < nr_refspec; i++) {
-   if (!parse_refspec([i], refspec[i], fetch))
-   goto invalid;
-   }
-
-   return rs;
-
- invalid:
-   if (verify) {
-   /*
-* nr_refspec must be greater than zero and i must be valid
-* since it is only possible to reach this point from within
-* the for loop above.
-*/
-   free_refspec(i+1, rs);
-   return NULL;
-   }
-   die("Invalid refspec '%s'", refspec[i]);
-}
-
-struct refspec_item *parse_fetch_refspec(int nr_refspec, const char **refspec)
-{
-   return parse_refspec_internal(nr_refspec, refspec, 1, 0);
-}
-
-struct refspec_item *parse_push_refspec(int nr_refspec, const char **refspec)
-{
-   return parse_refspec_internal(nr_refspec, refspec, 0, 0);
-}
-
-void free_refspec(int nr_refspec, struct refspec_item *refspec)
-{
-   int i;
-
-   if (!refspec)
-   return;
-
-   for (i = 0; i < nr_refspec; i++) {
-   free(refspec[i].src);
-   free(refspec[i].dst);
-   }
-   free(refspec);
-}
-
 void refspec_item_init(struct refspec_item *item, const char *refspec, int 
fetch)
 {
memset(item, 0, sizeof(*item));
diff --git a/refspec.h b/refspec.h
index 1063c64cc..7e1ff94ac 100644
--- a/refspec.h
+++ b/refspec.h
@@ -14,11 +14,6 @@ struct refspec_item {
char *dst;
 };
 
-struct refspec_item *parse_fetch_refspec(int nr_refspec, const char **refspec);
-struct refspec_item *parse_push_refspec(int nr_refspec, const char **refspec);
-
-void free_refspec(int nr_refspec, struct refspec_item *refspec);
-
 #define REFSPEC_FETCH 1
 #define REFSPEC_PUSH 0
 
-- 
2.17.0.441.gb46fe60e1d-goog

[PATCH v2 12/36] fast-export: convert to use struct refspec

[Brandon Williams]

Convert fast-export to use 'struct refspec' instead of using a list of
refspec_item's.

Signed-off-by: Brandon Williams 
---
 builtin/fast-export.c | 21 +++--
 1 file changed, 7 insertions(+), 14 deletions(-)

diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 6f105dc79..143999738 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -36,8 +36,7 @@ static int use_done_feature;
 static int no_data;
 static int full_tree;
 static struct string_list extra_refs = STRING_LIST_INIT_NODUP;
-static struct refspec_item *refspecs;
-static int refspecs_nr;
+static struct refspec refspecs = REFSPEC_INIT_FETCH;
 static int anonymize;
 
 static int parse_opt_signed_tag_mode(const struct option *opt,
@@ -830,9 +829,9 @@ static void get_tags_and_duplicates(struct rev_cmdline_info 
*info)
if (dwim_ref(e->name, strlen(e->name), , _name) != 1)
continue;
 
-   if (refspecs) {
+   if (refspecs.nr) {
char *private;
-   private = apply_refspecs(refspecs, refspecs_nr, 
full_name);
+   private = apply_refspecs(refspecs.items, refspecs.nr, 
full_name);
if (private) {
free(full_name);
full_name = private;
@@ -978,8 +977,8 @@ static void import_marks(char *input_file)
 static void handle_deletes(void)
 {
int i;
-   for (i = 0; i < refspecs_nr; i++) {
-   struct refspec_item *refspec = [i];
+   for (i = 0; i < refspecs.nr; i++) {
+   struct refspec_item *refspec = [i];
if (*refspec->src)
continue;
 
@@ -1040,18 +1039,12 @@ int cmd_fast_export(int argc, const char **argv, const 
char *prefix)
usage_with_options (fast_export_usage, options);
 
if (refspecs_list.nr) {
-   const char **refspecs_str;
int i;
 
-   ALLOC_ARRAY(refspecs_str, refspecs_list.nr);
for (i = 0; i < refspecs_list.nr; i++)
-   refspecs_str[i] = refspecs_list.items[i].string;
-
-   refspecs_nr = refspecs_list.nr;
-   refspecs = parse_fetch_refspec(refspecs_nr, refspecs_str);
+   refspec_append(, 
refspecs_list.items[i].string);
 
string_list_clear(_list, 1);
-   free(refspecs_str);
}
 
if (use_done_feature)
@@ -1090,7 +1083,7 @@ int cmd_fast_export(int argc, const char **argv, const 
char *prefix)
if (use_done_feature)
printf("done\n");
 
-   free_refspec(refspecs_nr, refspecs);
+   refspec_clear();
 
return 0;
 }
-- 
2.17.0.441.gb46fe60e1d-goog

[PATCH v2 13/36] remote: convert push refspecs to struct refspec

[Brandon Williams]

Convert the set of push refspecs stored in 'struct remote' to use
'struct refspec'.

Signed-off-by: Brandon Williams 
---
 builtin/push.c   | 10 +-
 builtin/remote.c | 14 +++---
 remote.c | 35 ++-
 remote.h |  6 ++
 4 files changed, 28 insertions(+), 37 deletions(-)

diff --git a/builtin/push.c b/builtin/push.c
index 00d81fb1d..509dc6677 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -79,11 +79,11 @@ static const char *map_refspec(const char *ref,
if (count_refspec_match(ref, local_refs, ) != 1)
return ref;
 
-   if (remote->push) {
+   if (remote->push.nr) {
struct refspec_item query;
memset(, 0, sizeof(struct refspec_item));
query.src = matched->name;
-   if (!query_refspecs(remote->push, remote->push_refspec_nr, 
) &&
+   if (!query_refspecs(remote->push.items, remote->push.nr, 
) &&
query.dst) {
struct strbuf buf = STRBUF_INIT;
strbuf_addf(, "%s%s:%s",
@@ -436,9 +436,9 @@ static int do_push(const char *repo, int flags,
}
 
if (!refspec && !(flags & TRANSPORT_PUSH_ALL)) {
-   if (remote->push_refspec_nr) {
-   refspec = remote->push_refspec;
-   refspec_nr = remote->push_refspec_nr;
+   if (remote->push.raw_nr) {
+   refspec = remote->push.raw;
+   refspec_nr = remote->push.raw_nr;
} else if (!(flags & TRANSPORT_PUSH_MIRROR))
setup_default_push_refspecs(remote);
}
diff --git a/builtin/remote.c b/builtin/remote.c
index d9da82dc8..fb84729d6 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -388,8 +388,8 @@ static int get_push_ref_states(const struct ref 
*remote_refs,
local_refs = get_local_heads();
push_map = copy_ref_list(remote_refs);
 
-   match_push_refs(local_refs, _map, remote->push_refspec_nr,
-   remote->push_refspec, MATCH_REFS_NONE);
+   match_push_refs(local_refs, _map, remote->push.raw_nr,
+   remote->push.raw, MATCH_REFS_NONE);
 
states->push.strdup_strings = 1;
for (ref = push_map; ref; ref = ref->next) {
@@ -435,14 +435,14 @@ static int get_push_ref_states_noquery(struct ref_states 
*states)
return 0;
 
states->push.strdup_strings = 1;
-   if (!remote->push_refspec_nr) {
+   if (!remote->push.nr) {
item = string_list_append(>push, _("(matching)"));
info = item->util = xcalloc(1, sizeof(struct push_info));
info->status = PUSH_STATUS_NOTQUERIED;
info->dest = xstrdup(item->string);
}
-   for (i = 0; i < remote->push_refspec_nr; i++) {
-   struct refspec_item *spec = remote->push + i;
+   for (i = 0; i < remote->push.nr; i++) {
+   const struct refspec_item *spec = >push.items[i];
if (spec->matching)
item = string_list_append(>push, 
_("(matching)"));
else if (strlen(spec->src))
@@ -586,8 +586,8 @@ static int migrate_file(struct remote *remote)
git_config_set_multivar(buf.buf, remote->url[i], "^$", 0);
strbuf_reset();
strbuf_addf(, "remote.%s.push", remote->name);
-   for (i = 0; i < remote->push_refspec_nr; i++)
-   git_config_set_multivar(buf.buf, remote->push_refspec[i], "^$", 
0);
+   for (i = 0; i < remote->push.raw_nr; i++)
+   git_config_set_multivar(buf.buf, remote->push.raw[i], "^$", 0);
strbuf_reset();
strbuf_addf(, "remote.%s.fetch", remote->name);
for (i = 0; i < remote->fetch_refspec_nr; i++)
diff --git a/remote.c b/remote.c
index bce6e7ce4..1b7258f77 100644
--- a/remote.c
+++ b/remote.c
@@ -77,14 +77,6 @@ static const char *alias_url(const char *url, struct 
rewrites *r)
return xstrfmt("%s%s", r->rewrite[longest_i]->base, url + longest->len);
 }
 
-static void add_push_refspec(struct remote *remote, const char *ref)
-{
-   ALLOC_GROW(remote->push_refspec,
-  remote->push_refspec_nr + 1,
-  remote->push_refspec_alloc);
-   remote->push_refspec[remote->push_refspec_nr++] = ref;
-}
-
 static void add_fetch_refspec(struct remote *remote, const char *ref)
 {
ALLOC_GROW(remote->fetch_refspec,
@@ -175,9 +167,11 @@ static struct remote *make_remote(const char *name, int 
len)
ret = xcalloc(1, sizeof(struct remote));
ret->prune = -1;  /* unspecified */
ret->prune_tags = -1;  /* unspecified */
+   ret->name = xstrndup(name, len);
+   refspec_init(>push, REFSPEC_PUSH);
+
ALLOC_GROW(remotes, remotes_nr + 1, remotes_alloc);
remotes[remotes_nr++] = ret;
-   ret->name = xstrndup(name, len);
 

[PATCH v2 18/36] fetch: convert refmap to use struct refspec

[Brandon Williams]

Convert the refmap in builtin/fetch.c to be stored in a
'struct refspec'.

Signed-off-by: Brandon Williams 
---
 builtin/fetch.c | 17 +++--
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 18704c6cd..6b909cd5b 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -60,8 +60,7 @@ static const char *submodule_prefix = "";
 static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
 static int recurse_submodules_default = RECURSE_SUBMODULES_ON_DEMAND;
 static int shown_url = 0;
-static int refmap_alloc, refmap_nr;
-static const char **refmap_array;
+static struct refspec refmap = REFSPEC_INIT_FETCH;
 static struct list_objects_filter_options filter_options;
 
 static int git_fetch_config(const char *k, const char *v, void *cb)
@@ -108,14 +107,12 @@ static int gitmodules_fetch_config(const char *var, const 
char *value, void *cb)
 
 static int parse_refmap_arg(const struct option *opt, const char *arg, int 
unset)
 {
-   ALLOC_GROW(refmap_array, refmap_nr + 1, refmap_alloc);
-
/*
 * "git fetch --refmap='' origin foo"
 * can be used to tell the command not to store anywhere
 */
-   if (*arg)
-   refmap_array[refmap_nr++] = arg;
+   refspec_append(, arg);
+
return 0;
 }
 
@@ -403,9 +400,9 @@ static struct ref *get_ref_map(struct transport *transport,
 * by ref_remove_duplicates() in favor of one of these
 * opportunistic entries with FETCH_HEAD_IGNORE.
 */
-   if (refmap_array) {
-   fetch_refspec = parse_fetch_refspec(refmap_nr, 
refmap_array);
-   fetch_refspec_nr = refmap_nr;
+   if (refmap.nr) {
+   fetch_refspec = refmap.items;
+   fetch_refspec_nr = refmap.nr;
} else {
fetch_refspec = transport->remote->fetch.items;
fetch_refspec_nr = transport->remote->fetch.nr;
@@ -413,7 +410,7 @@ static struct ref *get_ref_map(struct transport *transport,
 
for (i = 0; i < fetch_refspec_nr; i++)
get_fetch_map(ref_map, _refspec[i], _tail, 
1);
-   } else if (refmap_array) {
+   } else if (refmap.nr) {
die("--refmap option is only meaningful with command-line 
refspec(s).");
} else {
/* Use the defaults */
-- 
2.17.0.441.gb46fe60e1d-goog

[PATCH v2 16/36] transport-helper: convert to use struct refspec

[Brandon Williams]

Convert the refspecs in transport-helper.c to be stored in a
'struct refspec'.

Signed-off-by: Brandon Williams 
---
 transport-helper.c | 38 --
 1 file changed, 12 insertions(+), 26 deletions(-)

diff --git a/transport-helper.c b/transport-helper.c
index b156a37e7..33f51ebfc 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -36,8 +36,7 @@ struct helper_data {
char *export_marks;
char *import_marks;
/* These go from remote name (as in "list") to private name */
-   struct refspec_item *refspecs;
-   int refspec_nr;
+   struct refspec rs;
/* Transport options for fetch-pack/send-pack (should one of
 * those be invoked).
 */
@@ -107,9 +106,6 @@ static struct child_process *get_helper(struct transport 
*transport)
struct helper_data *data = transport->data;
struct strbuf buf = STRBUF_INIT;
struct child_process *helper;
-   const char **refspecs = NULL;
-   int refspec_nr = 0;
-   int refspec_alloc = 0;
int duped;
int code;
 
@@ -139,6 +135,7 @@ static struct child_process *get_helper(struct transport 
*transport)
 
data->helper = helper;
data->no_disconnect_req = 0;
+   refspec_init(>rs, REFSPEC_FETCH);
 
/*
 * Open the output as FILE* so strbuf_getline_*() family of
@@ -184,11 +181,8 @@ static struct child_process *get_helper(struct transport 
*transport)
data->export = 1;
else if (!strcmp(capname, "check-connectivity"))
data->check_connectivity = 1;
-   else if (!data->refspecs && skip_prefix(capname, "refspec ", 
)) {
-   ALLOC_GROW(refspecs,
-  refspec_nr + 1,
-  refspec_alloc);
-   refspecs[refspec_nr++] = xstrdup(arg);
+   else if (skip_prefix(capname, "refspec ", )) {
+   refspec_append(>rs, arg);
} else if (!strcmp(capname, "connect")) {
data->connect = 1;
} else if (!strcmp(capname, "stateless-connect")) {
@@ -207,14 +201,7 @@ static struct child_process *get_helper(struct transport 
*transport)
capname);
}
}
-   if (refspecs) {
-   int i;
-   data->refspec_nr = refspec_nr;
-   data->refspecs = parse_fetch_refspec(refspec_nr, refspecs);
-   for (i = 0; i < refspec_nr; i++)
-   free((char *)refspecs[i]);
-   free(refspecs);
-   } else if (data->import || data->bidi_import || data->export) {
+   if (!data->rs.nr && (data->import || data->bidi_import || 
data->export)) {
warning("This remote helper should implement refspec 
capability.");
}
strbuf_release();
@@ -378,8 +365,7 @@ static int release_helper(struct transport *transport)
 {
int res = 0;
struct helper_data *data = transport->data;
-   free_refspec(data->refspec_nr, data->refspecs);
-   data->refspecs = NULL;
+   refspec_clear(>rs);
res = disconnect_helper(transport);
free(transport->data);
return res;
@@ -536,8 +522,8 @@ static int fetch_with_import(struct transport *transport,
if (posn->status & REF_STATUS_UPTODATE)
continue;
name = posn->symref ? posn->symref : posn->name;
-   if (data->refspecs)
-   private = apply_refspecs(data->refspecs, 
data->refspec_nr, name);
+   if (data->rs.nr)
+   private = apply_refspecs(data->rs.items, data->rs.nr, 
name);
else
private = xstrdup(name);
if (private) {
@@ -815,11 +801,11 @@ static int push_update_refs_status(struct helper_data 
*data,
if (push_update_ref_status(, , remote_refs))
continue;
 
-   if (flags & TRANSPORT_PUSH_DRY_RUN || !data->refspecs || 
data->no_private_update)
+   if (flags & TRANSPORT_PUSH_DRY_RUN || !data->rs.nr || 
data->no_private_update)
continue;
 
/* propagate back the update to the remote namespace */
-   private = apply_refspecs(data->refspecs, data->refspec_nr, 
ref->name);
+   private = apply_refspecs(data->rs.items, data->rs.nr, 
ref->name);
if (!private)
continue;
update_ref("update by helper", private, >new_oid, NULL,
@@ -939,7 +925,7 @@ static int push_refs_with_export(struct transport 
*transport,
struct string_list revlist_args = STRING_LIST_INIT_DUP;
struct strbuf buf = STRBUF_INIT;
 
-   if (!data->refspecs)
+   if (!data->rs.nr)
die("remote-helper doesn't support push; 

[PATCH v2 10/36] remote: convert match_push_refs to use struct refspec

[Brandon Williams]

Convert 'match_push_refs()' to use struct refspec.

Signed-off-by: Brandon Williams 
---
 remote.c | 13 -
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/remote.c b/remote.c
index 191855118..bce6e7ce4 100644
--- a/remote.c
+++ b/remote.c
@@ -1312,7 +1312,7 @@ int check_push_refs(struct ref *src, int nr_refspec, 
const char **refspec_names)
 int match_push_refs(struct ref *src, struct ref **dst,
int nr_refspec, const char **refspec, int flags)
 {
-   struct refspec_item *rs;
+   struct refspec rs = REFSPEC_INIT_PUSH;
int send_all = flags & MATCH_REFS_ALL;
int send_mirror = flags & MATCH_REFS_MIRROR;
int send_prune = flags & MATCH_REFS_PRUNE;
@@ -1325,8 +1325,8 @@ int match_push_refs(struct ref *src, struct ref **dst,
nr_refspec = 1;
refspec = default_refspec;
}
-   rs = parse_push_refspec(nr_refspec, (const char **) refspec);
-   errs = match_explicit_refs(src, *dst, _tail, rs, nr_refspec);
+   refspec_appendn(, refspec, nr_refspec);
+   errs = match_explicit_refs(src, *dst, _tail, rs.items, rs.nr);
 
/* pick the remainder */
for (ref = src; ref; ref = ref->next) {
@@ -1335,7 +1335,7 @@ int match_push_refs(struct ref *src, struct ref **dst,
const struct refspec_item *pat = NULL;
char *dst_name;
 
-   dst_name = get_ref_match(rs, nr_refspec, ref, send_mirror, 
FROM_SRC, );
+   dst_name = get_ref_match(rs.items, rs.nr, ref, send_mirror, 
FROM_SRC, );
if (!dst_name)
continue;
 
@@ -1384,7 +1384,7 @@ int match_push_refs(struct ref *src, struct ref **dst,
/* We're already sending something to this ref. 
*/
continue;
 
-   src_name = get_ref_match(rs, nr_refspec, ref, 
send_mirror, FROM_DST, NULL);
+   src_name = get_ref_match(rs.items, rs.nr, ref, 
send_mirror, FROM_DST, NULL);
if (src_name) {
if (!src_ref_index.nr)
prepare_ref_index(_ref_index, src);
@@ -1396,6 +1396,9 @@ int match_push_refs(struct ref *src, struct ref **dst,
}
string_list_clear(_ref_index, 0);
}
+
+   refspec_clear();
+
if (errs)
return -1;
return 0;
-- 
2.17.0.441.gb46fe60e1d-goog

[PATCH v2 02/36] refspec: rename struct refspec to struct refspec_item

[Brandon Williams]

In preparation for introducing an abstraction around a collection of
refspecs (much like how a 'struct pathspec' is a collection of 'struct
pathspec_item's) rename the existing 'struct refspec' to 'struct
refspec_item'.

Signed-off-by: Brandon Williams 
---
 branch.c|  6 ++---
 builtin/clone.c |  4 +--
 builtin/fast-export.c   |  4 +--
 builtin/fetch.c | 12 -
 builtin/pull.c  |  2 +-
 builtin/push.c  |  4 +--
 builtin/remote.c|  8 +++---
 builtin/submodule--helper.c |  4 +--
 checkout.c  |  4 +--
 refspec.c   | 17 ++---
 refspec.h   | 10 
 remote.c| 50 ++---
 remote.h| 16 ++--
 transport-helper.c  |  2 +-
 transport.c |  4 +--
 15 files changed, 73 insertions(+), 74 deletions(-)

diff --git a/branch.c b/branch.c
index 32ccefc6b..f967c98f6 100644
--- a/branch.c
+++ b/branch.c
@@ -9,7 +9,7 @@
 #include "worktree.h"
 
 struct tracking {
-   struct refspec spec;
+   struct refspec_item spec;
char *src;
const char *remote;
int matches;
@@ -219,8 +219,8 @@ int validate_new_branchname(const char *name, struct strbuf 
*ref, int force)
 static int check_tracking_branch(struct remote *remote, void *cb_data)
 {
char *tracking_branch = cb_data;
-   struct refspec query;
-   memset(, 0, sizeof(struct refspec));
+   struct refspec_item query;
+   memset(, 0, sizeof(struct refspec_item));
query.dst = tracking_branch;
return !remote_find_tracking(remote, );
 }
diff --git a/builtin/clone.c b/builtin/clone.c
index 6d1614ed3..854088a3a 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -547,7 +547,7 @@ static struct ref *find_remote_branch(const struct ref 
*refs, const char *branch
 }
 
 static struct ref *wanted_peer_refs(const struct ref *refs,
-   struct refspec *refspec)
+   struct refspec_item *refspec)
 {
struct ref *head = copy_ref(find_ref_by_name(refs, "HEAD"));
struct ref *local_refs = head;
@@ -895,7 +895,7 @@ int cmd_clone(int argc, const char **argv, const char 
*prefix)
int err = 0, complete_refs_before_fetch = 1;
int submodule_progress;
 
-   struct refspec *refspec;
+   struct refspec_item *refspec;
const char *fetch_pattern;
 
fetch_if_missing = 0;
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index a13b7c8ef..6f105dc79 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -36,7 +36,7 @@ static int use_done_feature;
 static int no_data;
 static int full_tree;
 static struct string_list extra_refs = STRING_LIST_INIT_NODUP;
-static struct refspec *refspecs;
+static struct refspec_item *refspecs;
 static int refspecs_nr;
 static int anonymize;
 
@@ -979,7 +979,7 @@ static void handle_deletes(void)
 {
int i;
for (i = 0; i < refspecs_nr; i++) {
-   struct refspec *refspec = [i];
+   struct refspec_item *refspec = [i];
if (*refspec->src)
continue;
 
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 1fce68e9a..745020a10 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -203,7 +203,7 @@ static void add_merge_config(struct ref **head,
 
for (i = 0; i < branch->merge_nr; i++) {
struct ref *rm, **old_tail = *tail;
-   struct refspec refspec;
+   struct refspec_item refspec;
 
for (rm = *head; rm; rm = rm->next) {
if (branch_merge_matches(branch, i, rm->name)) {
@@ -340,7 +340,7 @@ static void find_non_local_tags(struct transport *transport,
 }
 
 static struct ref *get_ref_map(struct transport *transport,
-  struct refspec *refspecs, int refspec_count,
+  struct refspec_item *refspecs, int refspec_count,
   int tags, int *autotags)
 {
int i;
@@ -371,7 +371,7 @@ static struct ref *get_ref_map(struct transport *transport,
argv_array_clear(_prefixes);
 
if (refspec_count) {
-   struct refspec *fetch_refspec;
+   struct refspec_item *fetch_refspec;
int fetch_refspec_nr;
 
for (i = 0; i < refspec_count; i++) {
@@ -965,7 +965,7 @@ static int fetch_refs(struct transport *transport, struct 
ref *ref_map)
return ret;
 }
 
-static int prune_refs(struct refspec *refs, int ref_count, struct ref *ref_map,
+static int prune_refs(struct refspec_item *refs, int ref_count, struct ref 
*ref_map,
const char *raw_url)
 {
int url_len, i, result = 0;
@@ -1115,7 +1115,7 @@ static void backfill_tags(struct transport *transport, 
struct ref *ref_map)
 }
 
 static int do_fetch(struct transport *transport,
-   

[PATCH v2 11/36] clone: convert cmd_clone to use refspec_item_init

[Brandon Williams]

Convert 'cmd_clone()' to use 'refspec_item_init()' instead of relying on
the old 'parse_fetch_refspec()' to initialize a single refspec item.

Signed-off-by: Brandon Williams 
---
 builtin/clone.c | 10 --
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 854088a3a..8c5f4d8f0 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -895,8 +895,7 @@ int cmd_clone(int argc, const char **argv, const char 
*prefix)
int err = 0, complete_refs_before_fetch = 1;
int submodule_progress;
 
-   struct refspec_item *refspec;
-   const char *fetch_pattern;
+   struct refspec_item refspec;
 
fetch_if_missing = 0;
 
@@ -1078,8 +1077,7 @@ int cmd_clone(int argc, const char **argv, const char 
*prefix)
if (option_required_reference.nr || option_optional_reference.nr)
setup_reference();
 
-   fetch_pattern = value.buf;
-   refspec = parse_fetch_refspec(1, _pattern);
+   refspec_item_init(, value.buf, REFSPEC_FETCH);
 
strbuf_reset();
 
@@ -1139,7 +1137,7 @@ int cmd_clone(int argc, const char **argv, const char 
*prefix)
refs = transport_get_remote_refs(transport, NULL);
 
if (refs) {
-   mapped_refs = wanted_peer_refs(refs, refspec);
+   mapped_refs = wanted_peer_refs(refs, );
/*
 * transport_get_remote_refs() may return refs with null sha-1
 * in mapped_refs (see struct transport->get_refs_list
@@ -1233,6 +1231,6 @@ int cmd_clone(int argc, const char **argv, const char 
*prefix)
strbuf_release();
junk_mode = JUNK_LEAVE_ALL;
 
-   free(refspec);
+   refspec_item_clear();
return err;
 }
-- 
2.17.0.441.gb46fe60e1d-goog

[PATCH v2 04/36] refspec: introduce struct refspec

[Brandon Williams]

Introduce 'struct refspec', an abstraction around a collection of
'struct refspec_item's much like how 'struct pathspec' holds a
collection of 'struct pathspec_item's.

A refspec struct also contains an array of the original refspec strings
which will be used to facilitate the migration to using this new
abstraction throughout the code base.

Signed-off-by: Brandon Williams 
---
 refspec.c | 64 +++
 refspec.h | 25 ++
 2 files changed, 89 insertions(+)

diff --git a/refspec.c b/refspec.c
index 8bf4ebbd3..af9d0d4b3 100644
--- a/refspec.c
+++ b/refspec.c
@@ -178,3 +178,67 @@ void free_refspec(int nr_refspec, struct refspec_item 
*refspec)
}
free(refspec);
 }
+
+void refspec_item_init(struct refspec_item *item, const char *refspec, int 
fetch)
+{
+   memset(item, 0, sizeof(*item));
+
+   if (!parse_refspec(item, refspec, fetch))
+   die("Invalid refspec '%s'", refspec);
+}
+
+void refspec_item_clear(struct refspec_item *item)
+{
+   FREE_AND_NULL(item->src);
+   FREE_AND_NULL(item->dst);
+   item->force = 0;
+   item->pattern = 0;
+   item->matching = 0;
+   item->exact_sha1 = 0;
+}
+
+void refspec_init(struct refspec *rs, int fetch)
+{
+   memset(rs, 0, sizeof(*rs));
+   rs->fetch = fetch;
+}
+
+void refspec_append(struct refspec *rs, const char *refspec)
+{
+   struct refspec_item item;
+
+   refspec_item_init(, refspec, rs->fetch);
+
+   ALLOC_GROW(rs->items, rs->nr + 1, rs->alloc);
+   rs->items[rs->nr++] = item;
+
+   ALLOC_GROW(rs->raw, rs->raw_nr + 1, rs->raw_alloc);
+   rs->raw[rs->raw_nr++] = xstrdup(refspec);
+}
+
+void refspec_appendn(struct refspec *rs, const char **refspecs, int nr)
+{
+   int i;
+   for (i = 0; i < nr; i++)
+   refspec_append(rs, refspecs[i]);
+}
+
+void refspec_clear(struct refspec *rs)
+{
+   int i;
+
+   for (i = 0; i < rs->nr; i++)
+   refspec_item_clear(>items[i]);
+
+   FREE_AND_NULL(rs->items);
+   rs->alloc = 0;
+   rs->nr = 0;
+
+   for (i = 0; i < rs->raw_nr; i++)
+   free((char *)rs->raw[i]);
+   FREE_AND_NULL(rs->raw);
+   rs->raw_alloc = 0;
+   rs->raw_nr = 0;
+
+   rs->fetch = 0;
+}
diff --git a/refspec.h b/refspec.h
index fc9c1af77..da3135825 100644
--- a/refspec.h
+++ b/refspec.h
@@ -20,4 +20,29 @@ struct refspec_item *parse_push_refspec(int nr_refspec, 
const char **refspec);
 
 void free_refspec(int nr_refspec, struct refspec_item *refspec);
 
+#define REFSPEC_FETCH 1
+#define REFSPEC_PUSH 0
+
+#define REFSPEC_INIT_FETCH { .fetch = REFSPEC_FETCH }
+#define REFSPEC_INIT_PUSH { .fetch = REFSPEC_PUSH }
+
+struct refspec {
+   struct refspec_item *items;
+   int alloc;
+   int nr;
+
+   const char **raw;
+   int raw_alloc;
+   int raw_nr;
+
+   int fetch;
+};
+
+void refspec_item_init(struct refspec_item *item, const char *refspec, int 
fetch);
+void refspec_item_clear(struct refspec_item *item);
+void refspec_init(struct refspec *rs, int fetch);
+void refspec_append(struct refspec *rs, const char *refspec);
+void refspec_appendn(struct refspec *rs, const char **refspecs, int nr);
+void refspec_clear(struct refspec *rs);
+
 #endif /* REFSPEC_H */
-- 
2.17.0.441.gb46fe60e1d-goog

[PATCH v2 07/36] pull: convert get_tracking_branch to use refspec_item_init

[Brandon Williams]

Convert 'get_tracking_branch()' to use 'refspec_item_init()' instead of
the old 'parse_fetch_refspec()' function.

Signed-off-by: Brandon Williams 
---
 builtin/pull.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index 5a79deae5..09575fd23 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -676,12 +676,12 @@ static const char *get_upstream_branch(const char *remote)
  */
 static const char *get_tracking_branch(const char *remote, const char *refspec)
 {
-   struct refspec_item *spec;
+   struct refspec_item spec;
const char *spec_src;
const char *merge_branch;
 
-   spec = parse_fetch_refspec(1, );
-   spec_src = spec->src;
+   refspec_item_init(, refspec, REFSPEC_FETCH);
+   spec_src = spec.src;
if (!*spec_src || !strcmp(spec_src, "HEAD"))
spec_src = "HEAD";
else if (skip_prefix(spec_src, "heads/", _src))
@@ -701,7 +701,7 @@ static const char *get_tracking_branch(const char *remote, 
const char *refspec)
} else
merge_branch = NULL;
 
-   free_refspec(1, spec);
+   refspec_item_clear();
return merge_branch;
 }
 
-- 
2.17.0.441.gb46fe60e1d-goog

[PATCH v2 03/36] refspec: factor out parsing a single refspec

[Brandon Williams]

Factor out the logic which parses a single refspec into its own
function.  This makes it easier to reuse this logic in a future patch.

Signed-off-by: Brandon Williams 
---
 refspec.c | 195 +-
 1 file changed, 104 insertions(+), 91 deletions(-)

diff --git a/refspec.c b/refspec.c
index 22188f010..8bf4ebbd3 100644
--- a/refspec.c
+++ b/refspec.c
@@ -14,110 +14,123 @@ static struct refspec_item s_tag_refspec = {
 /* See TAG_REFSPEC for the string version */
 const struct refspec_item *tag_refspec = _tag_refspec;
 
-static struct refspec_item *parse_refspec_internal(int nr_refspec, const char 
**refspec, int fetch, int verify)
+/*
+ * Parses the provided refspec 'refspec' and populates the refspec_item 'item'.
+ * Returns 1 if successful and 0 if the refspec is invalid.
+ */
+static int parse_refspec(struct refspec_item *item, const char *refspec, int 
fetch)
 {
-   int i;
-   struct refspec_item *rs = xcalloc(nr_refspec, sizeof(*rs));
+   size_t llen;
+   int is_glob;
+   const char *lhs, *rhs;
+   int flags;
 
-   for (i = 0; i < nr_refspec; i++) {
-   size_t llen;
-   int is_glob;
-   const char *lhs, *rhs;
-   int flags;
+   is_glob = 0;
 
-   is_glob = 0;
+   lhs = refspec;
+   if (*lhs == '+') {
+   item->force = 1;
+   lhs++;
+   }
 
-   lhs = refspec[i];
-   if (*lhs == '+') {
-   rs[i].force = 1;
-   lhs++;
-   }
+   rhs = strrchr(lhs, ':');
 
-   rhs = strrchr(lhs, ':');
+   /*
+* Before going on, special case ":" (or "+:") as a refspec
+* for pushing matching refs.
+*/
+   if (!fetch && rhs == lhs && rhs[1] == '\0') {
+   item->matching = 1;
+   return 1;
+   }
 
+   if (rhs) {
+   size_t rlen = strlen(++rhs);
+   is_glob = (1 <= rlen && strchr(rhs, '*'));
+   item->dst = xstrndup(rhs, rlen);
+   }
+
+   llen = (rhs ? (rhs - lhs - 1) : strlen(lhs));
+   if (1 <= llen && memchr(lhs, '*', llen)) {
+   if ((rhs && !is_glob) || (!rhs && fetch))
+   return 0;
+   is_glob = 1;
+   } else if (rhs && is_glob) {
+   return 0;
+   }
+
+   item->pattern = is_glob;
+   item->src = xstrndup(lhs, llen);
+   flags = REFNAME_ALLOW_ONELEVEL | (is_glob ? REFNAME_REFSPEC_PATTERN : 
0);
+
+   if (fetch) {
+   struct object_id unused;
+
+   /* LHS */
+   if (!*item->src)
+   ; /* empty is ok; it means "HEAD" */
+   else if (llen == GIT_SHA1_HEXSZ && !get_oid_hex(item->src, 
))
+   item->exact_sha1 = 1; /* ok */
+   else if (!check_refname_format(item->src, flags))
+   ; /* valid looking ref is ok */
+   else
+   return 0;
+   /* RHS */
+   if (!item->dst)
+   ; /* missing is ok; it is the same as empty */
+   else if (!*item->dst)
+   ; /* empty is ok; it means "do not store" */
+   else if (!check_refname_format(item->dst, flags))
+   ; /* valid looking ref is ok */
+   else
+   return 0;
+   } else {
/*
-* Before going on, special case ":" (or "+:") as a refspec
-* for pushing matching refs.
+* LHS
+* - empty is allowed; it means delete.
+* - when wildcarded, it must be a valid looking ref.
+* - otherwise, it must be an extended SHA-1, but
+*   there is no existing way to validate this.
 */
-   if (!fetch && rhs == lhs && rhs[1] == '\0') {
-   rs[i].matching = 1;
-   continue;
+   if (!*item->src)
+   ; /* empty is ok */
+   else if (is_glob) {
+   if (check_refname_format(item->src, flags))
+   return 0;
}
-
-   if (rhs) {
-   size_t rlen = strlen(++rhs);
-   is_glob = (1 <= rlen && strchr(rhs, '*'));
-   rs[i].dst = xstrndup(rhs, rlen);
+   else
+   ; /* anything goes, for now */
+   /*
+* RHS
+* - missing is allowed, but LHS then must be a
+*   valid looking ref.
+* - empty is not allowed.
+* - otherwise it must be a valid looking ref.
+*/
+   if (!item->dst) {
+   if (check_refname_format(item->src, flags))
+   

[PATCH v2 01/36] refspec: move refspec parsing logic into its own file

[Brandon Williams]

In preparation for performing a refactor on refspec related code, move
the refspec parsing logic into its own file.

Signed-off-by: Brandon Williams 
---
 Makefile|   1 +
 branch.c|   1 +
 builtin/clone.c |   1 +
 builtin/fast-export.c   |   1 +
 builtin/fetch.c |   1 +
 builtin/merge.c |   1 +
 builtin/pull.c  |   1 +
 builtin/push.c  |   1 +
 builtin/remote.c|   1 +
 builtin/submodule--helper.c |   1 +
 checkout.c  |   1 +
 refspec.c   | 168 
 refspec.h   |  23 +
 remote.c| 165 +--
 remote.h|  20 -
 transport-helper.c  |   1 +
 transport.c |   1 +
 17 files changed, 205 insertions(+), 184 deletions(-)
 create mode 100644 refspec.c
 create mode 100644 refspec.h

diff --git a/Makefile b/Makefile
index ad880d1fc..4bca65383 100644
--- a/Makefile
+++ b/Makefile
@@ -928,6 +928,7 @@ 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 += refspec.o
 LIB_OBJS += ref-filter.o
 LIB_OBJS += remote.o
 LIB_OBJS += replace-object.o
diff --git a/branch.c b/branch.c
index 2672054f0..32ccefc6b 100644
--- a/branch.c
+++ b/branch.c
@@ -3,6 +3,7 @@
 #include "config.h"
 #include "branch.h"
 #include "refs.h"
+#include "refspec.h"
 #include "remote.h"
 #include "commit.h"
 #include "worktree.h"
diff --git a/builtin/clone.c b/builtin/clone.c
index 84f1473d1..6d1614ed3 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -14,6 +14,7 @@
 #include "parse-options.h"
 #include "fetch-pack.h"
 #include "refs.h"
+#include "refspec.h"
 #include "tree.h"
 #include "tree-walk.h"
 #include "unpack-trees.h"
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 530df12f0..a13b7c8ef 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -7,6 +7,7 @@
 #include "cache.h"
 #include "config.h"
 #include "refs.h"
+#include "refspec.h"
 #include "commit.h"
 #include "object.h"
 #include "tag.h"
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 7ee83ac0f..1fce68e9a 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -5,6 +5,7 @@
 #include "config.h"
 #include "repository.h"
 #include "refs.h"
+#include "refspec.h"
 #include "commit.h"
 #include "builtin.h"
 #include "string-list.h"
diff --git a/builtin/merge.c b/builtin/merge.c
index 9db5a2cf1..c362cfe90 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -14,6 +14,7 @@
 #include "run-command.h"
 #include "diff.h"
 #include "refs.h"
+#include "refspec.h"
 #include "commit.h"
 #include "diffcore.h"
 #include "revision.h"
diff --git a/builtin/pull.c b/builtin/pull.c
index 71aac5005..6247c956d 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -15,6 +15,7 @@
 #include "remote.h"
 #include "dir.h"
 #include "refs.h"
+#include "refspec.h"
 #include "revision.h"
 #include "submodule.h"
 #include "submodule-config.h"
diff --git a/builtin/push.c b/builtin/push.c
index ac3705370..fa65999b2 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -4,6 +4,7 @@
 #include "cache.h"
 #include "config.h"
 #include "refs.h"
+#include "refspec.h"
 #include "run-command.h"
 #include "builtin.h"
 #include "remote.h"
diff --git a/builtin/remote.c b/builtin/remote.c
index 8708e584e..c49513995 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -7,6 +7,7 @@
 #include "strbuf.h"
 #include "run-command.h"
 #include "refs.h"
+#include "refspec.h"
 #include "argv-array.h"
 
 static const char * const builtin_remote_usage[] = {
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index c2403a915..6ab032acb 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -12,6 +12,7 @@
 #include "run-command.h"
 #include "remote.h"
 #include "refs.h"
+#include "refspec.h"
 #include "connect.h"
 #include "revision.h"
 #include "diffcore.h"
diff --git a/checkout.c b/checkout.c
index ac42630f7..193ba8567 100644
--- a/checkout.c
+++ b/checkout.c
@@ -1,5 +1,6 @@
 #include "cache.h"
 #include "remote.h"
+#include "refspec.h"
 #include "checkout.h"
 
 struct tracking_name_data {
diff --git a/refspec.c b/refspec.c
new file mode 100644
index 0..ecb0bdff3
--- /dev/null
+++ b/refspec.c
@@ -0,0 +1,168 @@
+#include "cache.h"
+#include "refs.h"
+#include "refspec.h"
+
+static struct refspec s_tag_refspec = {
+   0,
+   1,
+   0,
+   0,
+   "refs/tags/*",
+   "refs/tags/*"
+};
+
+/* See TAG_REFSPEC for the string version */
+const struct refspec *tag_refspec = _tag_refspec;
+
+static struct refspec *parse_refspec_internal(int nr_refspec, const char 
**refspec, int fetch, int verify)
+{
+   int i;
+   struct refspec *rs = xcalloc(nr_refspec, sizeof(*rs));
+
+   for (i = 0; i < nr_refspec; i++) {
+   size_t llen;
+ 

[PATCH v2 05/36] refspec: convert valid_fetch_refspec to use parse_refspec

[Brandon Williams]

Convert 'valid_fetch_refspec()' to use the new 'parse_refspec()'
function to only parse a single refspec and eliminate an allocation.

Signed-off-by: Brandon Williams 
---
 refspec.c | 17 -
 refspec.h |  3 ++-
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/refspec.c b/refspec.c
index af9d0d4b3..ab37b5ba1 100644
--- a/refspec.c
+++ b/refspec.c
@@ -146,15 +146,6 @@ static struct refspec_item *parse_refspec_internal(int 
nr_refspec, const char **
die("Invalid refspec '%s'", refspec[i]);
 }
 
-int valid_fetch_refspec(const char *fetch_refspec_str)
-{
-   struct refspec_item *refspec;
-
-   refspec = parse_refspec_internal(1, _refspec_str, 1, 1);
-   free_refspec(1, refspec);
-   return !!refspec;
-}
-
 struct refspec_item *parse_fetch_refspec(int nr_refspec, const char **refspec)
 {
return parse_refspec_internal(nr_refspec, refspec, 1, 0);
@@ -242,3 +233,11 @@ void refspec_clear(struct refspec *rs)
 
rs->fetch = 0;
 }
+
+int valid_fetch_refspec(const char *fetch_refspec_str)
+{
+   struct refspec_item refspec;
+   int ret = parse_refspec(, fetch_refspec_str, REFSPEC_FETCH);
+   refspec_item_clear();
+   return ret;
+}
diff --git a/refspec.h b/refspec.h
index da3135825..1063c64cc 100644
--- a/refspec.h
+++ b/refspec.h
@@ -14,7 +14,6 @@ struct refspec_item {
char *dst;
 };
 
-int valid_fetch_refspec(const char *refspec);
 struct refspec_item *parse_fetch_refspec(int nr_refspec, const char **refspec);
 struct refspec_item *parse_push_refspec(int nr_refspec, const char **refspec);
 
@@ -45,4 +44,6 @@ void refspec_append(struct refspec *rs, const char *refspec);
 void refspec_appendn(struct refspec *rs, const char **refspecs, int nr);
 void refspec_clear(struct refspec *rs);
 
+int valid_fetch_refspec(const char *refspec);
+
 #endif /* REFSPEC_H */
-- 
2.17.0.441.gb46fe60e1d-goog

[PATCH v2 06/36] submodule--helper: convert push_check to use struct refspec

[Brandon Williams]

Convert 'push_check()' to use 'struct refspec'.

Signed-off-by: Brandon Williams 
---
 builtin/submodule--helper.c | 13 +++--
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index c0c4db007..88a149a2c 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1744,13 +1744,14 @@ static int push_check(int argc, const char **argv, 
const char *prefix)
 
/* Check the refspec */
if (argc > 2) {
-   int i, refspec_nr = argc - 2;
+   int i;
struct ref *local_refs = get_local_heads();
-   struct refspec_item *refspec = parse_push_refspec(refspec_nr,
-argv + 2);
+   struct refspec refspec = REFSPEC_INIT_PUSH;
 
-   for (i = 0; i < refspec_nr; i++) {
-   struct refspec_item *rs = refspec + i;
+   refspec_appendn(, argv + 2, argc - 2);
+
+   for (i = 0; i < refspec.nr; i++) {
+   const struct refspec_item *rs = [i];
 
if (rs->pattern || rs->matching)
continue;
@@ -1777,7 +1778,7 @@ static int push_check(int argc, const char **argv, const 
char *prefix)
rs->src);
}
}
-   free_refspec(refspec_nr, refspec);
+   refspec_clear();
}
free(head);
 
-- 
2.17.0.441.gb46fe60e1d-goog

[PATCH v2 00/36] refactoring refspecs

[Brandon Williams]

Changes in v2:
 * added missing extern keyword in the first patch
 * reordered patch 2 and 3 and updated some comments to be clearer.
 * fixed some memory leaks
 * squashed in some changes recommended by Aevar 

Brandon Williams (36):
  refspec: move refspec parsing logic into its own file
  refspec: rename struct refspec to struct refspec_item
  refspec: factor out parsing a single refspec
  refspec: introduce struct refspec
  refspec: convert valid_fetch_refspec to use parse_refspec
  submodule--helper: convert push_check to use struct refspec
  pull: convert get_tracking_branch to use refspec_item_init
  transport: convert transport_push to use struct refspec
  remote: convert check_push_refs to use struct refspec
  remote: convert match_push_refs to use struct refspec
  clone: convert cmd_clone to use refspec_item_init
  fast-export: convert to use struct refspec
  remote: convert push refspecs to struct refspec
  remote: convert fetch refspecs to struct refspec
  remote: remove add_prune_tags_to_fetch_refspec
  transport-helper: convert to use struct refspec
  fetch: convert fetch_one to use struct refspec
  fetch: convert refmap to use struct refspec
  refspec: remove the deprecated functions
  fetch: convert do_fetch to take a struct refspec
  fetch: convert get_ref_map to take a struct refspec
  fetch: convert prune_refs to take a struct refspec
  remote: convert get_stale_heads to take a struct refspec
  remote: convert apply_refspecs to take a struct refspec
  remote: convert query_refspecs to take a struct refspec
  remote: convert get_ref_match to take a struct refspec
  remote: convert match_explicit_refs to take a struct refspec
  push: check for errors earlier
  push: convert to use struct refspec
  transport: convert transport_push to take a struct refspec
  send-pack: store refspecs in a struct refspec
  transport: remove transport_verify_remote_names
  http-push: store refspecs in a struct refspec
  remote: convert match_push_refs to take a struct refspec
  remote: convert check_push_refs to take a struct refspec
  submodule: convert push_unpushed_submodules to take a struct refspec

 Makefile|   1 +
 branch.c|   7 +-
 builtin/clone.c |  13 +-
 builtin/fast-export.c   |  22 +--
 builtin/fetch.c | 134 ++
 builtin/merge.c |   1 +
 builtin/pull.c  |   9 +-
 builtin/push.c  |  80 
 builtin/remote.c|  37 ++--
 builtin/send-pack.c |  24 +--
 builtin/submodule--helper.c |  14 +-
 checkout.c  |   5 +-
 http-push.c |  18 +-
 refspec.c   | 194 
 refspec.h   |  44 +
 remote.c| 353 
 remote.h|  50 ++---
 submodule.c |  19 +-
 submodule.h |   3 +-
 transport-helper.c  |  39 ++--
 transport.c |  51 ++
 transport.h |   4 +-
 22 files changed, 527 insertions(+), 595 deletions(-)
 create mode 100644 refspec.c
 create mode 100644 refspec.h

-- 
2.17.0.441.gb46fe60e1d-goog

Re: [PATCH 11/11] read_cache: convert most calls to repo_read_index_or_die

[Brandon Williams]

On 05/16, Stefan Beller wrote:
> Signed-off-by: Stefan Beller 
> ---
>  blame.c   | 5 +++--
>  builtin/am.c  | 3 ++-
>  builtin/diff.c| 3 ++-
>  builtin/fsck.c| 3 ++-
>  builtin/merge-index.c | 3 ++-
>  check-racy.c  | 2 +-
>  diff.c| 5 +++--
>  merge-recursive.c | 3 ++-
>  revision.c| 5 +++--
>  sequencer.c   | 5 +++--
>  sha1-name.c   | 2 +-
>  11 files changed, 24 insertions(+), 15 deletions(-)
> 
> diff --git a/blame.c b/blame.c
> index 78c9808bd1a..ebfa1c8efcd 100644
> --- a/blame.c
> +++ b/blame.c
> @@ -5,6 +5,7 @@
>  #include "diff.h"
>  #include "diffcore.h"
>  #include "tag.h"
> +#include "repository.h"
>  #include "blame.h"
>  
>  void blame_origin_decref(struct blame_origin *o)
> @@ -159,7 +160,7 @@ static struct commit *fake_working_tree_commit(struct 
> diff_options *opt,
>   unsigned mode;
>   struct strbuf msg = STRBUF_INIT;
>  
> - read_cache();
> + repo_read_index_or_die(the_repository);
>   time();
>   commit = alloc_commit_node();
>   commit->object.parsed = 1;
> @@ -241,7 +242,7 @@ static struct commit *fake_working_tree_commit(struct 
> diff_options *opt,
>* want to run "diff-index --cached".
>*/
>   discard_cache();
> - read_cache();
> + repo_read_index_or_die(the_repository);
>  
>   len = strlen(path);
>   if (!mode) {
> diff --git a/builtin/am.c b/builtin/am.c
> index d834f9e62b6..3c6e77a5369 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -32,6 +32,7 @@
>  #include "apply.h"
>  #include "string-list.h"
>  #include "packfile.h"
> +#include "repository.h"
>  
>  /**
>   * Returns 1 if the file is empty or does not exist, 0 otherwise.
> @@ -1581,7 +1582,7 @@ static int fall_back_threeway(const struct am_state 
> *state, const char *index_pa
>   say(state, stdout, _("Falling back to patching base and 3-way 
> merge..."));
>  
>   discard_cache();
> - read_cache();
> + repo_read_index_or_die(the_repository);
>  
>   /*
>* This is not so wrong. Depending on which base we picked, orig_tree
> diff --git a/builtin/diff.c b/builtin/diff.c
> index 16bfb22f738..4bba211f1c7 100644
> --- a/builtin/diff.c
> +++ b/builtin/diff.c
> @@ -17,6 +17,7 @@
>  #include "builtin.h"
>  #include "submodule.h"
>  #include "sha1-array.h"
> +#include "repository.h"
>  
>  #define DIFF_NO_INDEX_EXPLICIT 1
>  #define DIFF_NO_INDEX_IMPLICIT 2
> @@ -210,7 +211,7 @@ static void refresh_index_quietly(void)
>   if (fd < 0)
>   return;
>   discard_cache();
> - read_cache();
> + repo_read_index(the_repository); /* do not die on error */
>   refresh_cache(REFRESH_QUIET|REFRESH_UNMERGED);
>   update_index_if_able(_index, _file);
>  }
> diff --git a/builtin/fsck.c b/builtin/fsck.c
> index 087360a6757..a42e98235da 100644
> --- a/builtin/fsck.c
> +++ b/builtin/fsck.c
> @@ -18,6 +18,7 @@
>  #include "decorate.h"
>  #include "packfile.h"
>  #include "object-store.h"
> +#include "repository.h"
>  
>  #define REACHABLE 0x0001
>  #define SEEN  0x0002
> @@ -795,7 +796,7 @@ int cmd_fsck(int argc, const char **argv, const char 
> *prefix)
>   if (keep_cache_objects) {
>   verify_index_checksum = 1;
>   verify_ce_order = 1;
> - read_cache();
> + repo_read_index_or_die(the_repository);
>   for (i = 0; i < active_nr; i++) {
>   unsigned int mode;
>   struct blob *blob;
> diff --git a/builtin/merge-index.c b/builtin/merge-index.c
> index c99443b095b..2d91c7c3b5e 100644
> --- a/builtin/merge-index.c
> +++ b/builtin/merge-index.c
> @@ -1,5 +1,6 @@
>  #include "builtin.h"
>  #include "run-command.h"
> +#include "repository.h"
>  
>  static const char *pgm;
>  static int one_shot, quiet;
> @@ -77,7 +78,7 @@ int cmd_merge_index(int argc, const char **argv, const char 
> *prefix)
>   if (argc < 3)
>   usage("git merge-index [-o] [-q]  (-a | [--] 
> [...])");
>  
> - read_cache();
> + repo_read_index_or_die(the_repository);
>  
>   i = 1;
>   if (!strcmp(argv[i], "-o")) {
> diff --git a/check-racy.c b/check-racy.c
> index 24b6542352a..9b884639cf4 100644
> --- a/check-racy.c
> +++ b/check-racy.c
> @@ -6,7 +6,7 @@ int main(int ac, char **av)
>   int dirty, clean, racy;
>  
>   dirty = clean = racy = 0;
> - read_cache();
> + repo_read_index_or_die(the_repository);
>   for (i = 0; i < active_nr; i++) {
>   struct cache_entry *ce = active_cache[i];
>   struct stat st;
> diff --git a/diff.c b/diff.c
> index 1289df4b1f9..383f52fa118 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -22,6 +22,7 @@
>  #include "argv-array.h"
>  #include "graph.h"
>  #include "packfile.h"
> +#include "repository.h"
>  
>  #ifdef NO_FAST_WORKING_DIRECTORY
>  #define FAST_WORKING_DIRECTORY 0
> @@ -4210,13 +4211,13 @@ void diff_setup_done(struct diff_options 

[PATCH 04/11] submodule: use repo_read_index_or_die

[Stefan Beller]

The code is used by the fetch command, which is a main porcelain command,
so we should localize its error messages.

Signed-off-by: Stefan Beller 
---
 submodule.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/submodule.c b/submodule.c
index 74d35b25779..71c042e1371 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1333,8 +1333,7 @@ int fetch_populated_submodules(struct repository *r,
if (!r->worktree)
goto out;
 
-   if (repo_read_index(r) < 0)
-   die("index file corrupt");
+   repo_read_index_or_die(r);
 
argv_array_push(, "fetch");
for (i = 0; i < options->argc; i++)
-- 
2.17.0.582.gccdcbd54c44.dirty

[PATCH 02/11] repository: introduce repo_read_index_or_die

[Stefan Beller]

A common pattern with the repo_read_index function is to die if the return
of repo_read_index is negative.  Move this pattern into a function.

This patch replaces the calls of repo_read_index with its _or_die version,
if the error message is exactly the same.

Signed-off-by: Stefan Beller 
---
 builtin/add.c   | 3 +--
 builtin/check-ignore.c  | 7 ---
 builtin/clean.c | 4 ++--
 builtin/mv.c| 3 +--
 builtin/reset.c | 3 +--
 builtin/rm.c| 3 +--
 builtin/submodule--helper.c | 3 +--
 repository.c| 6 ++
 repository.h| 8 
 9 files changed, 25 insertions(+), 15 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index c9e2619a9ad..e4751c198c1 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -445,8 +445,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
return 0;
}
 
-   if (read_cache() < 0)
-   die(_("index file corrupt"));
+   repo_read_index_or_die(the_repository);
 
die_in_unpopulated_submodule(_index, prefix);
 
diff --git a/builtin/check-ignore.c b/builtin/check-ignore.c
index ec9a959e08d..2a46bf9af7f 100644
--- a/builtin/check-ignore.c
+++ b/builtin/check-ignore.c
@@ -6,6 +6,7 @@
 #include "pathspec.h"
 #include "parse-options.h"
 #include "submodule.h"
+#include "repository.h"
 
 static int quiet, verbose, stdin_paths, show_non_matching, no_index;
 static const char * const check_ignore_usage[] = {
@@ -172,9 +173,9 @@ int cmd_check_ignore(int argc, const char **argv, const 
char *prefix)
if (show_non_matching && !verbose)
die(_("--non-matching is only valid with --verbose"));
 
-   /* read_cache() is only necessary so we can watch out for submodules. */
-   if (!no_index && read_cache() < 0)
-   die(_("index file corrupt"));
+   /* repo_read_index() is only necessary so we can watch out for 
submodules. */
+   if (!no_index)
+   repo_read_index_or_die(the_repository);
 
memset(, 0, sizeof(dir));
setup_standard_excludes();
diff --git a/builtin/clean.c b/builtin/clean.c
index fad533a0a73..6c1c6fdd7f9 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -16,6 +16,7 @@
 #include "column.h"
 #include "color.h"
 #include "pathspec.h"
+#include "repository.h"
 
 static int force = -1; /* unset */
 static int interactive;
@@ -954,8 +955,7 @@ int cmd_clean(int argc, const char **argv, const char 
*prefix)
if (remove_directories)
dir.flags |= DIR_SHOW_IGNORED_TOO | DIR_KEEP_UNTRACKED_CONTENTS;
 
-   if (read_cache() < 0)
-   die(_("index file corrupt"));
+   repo_read_index_or_die(the_repository);
 
if (!ignored)
setup_standard_excludes();
diff --git a/builtin/mv.c b/builtin/mv.c
index 7a63667d648..7f62e626dda 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -140,8 +140,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
usage_with_options(builtin_mv_usage, builtin_mv_options);
 
hold_locked_index(_file, LOCK_DIE_ON_ERROR);
-   if (read_cache() < 0)
-   die(_("index file corrupt"));
+   repo_read_index_or_die(the_repository);
 
source = internal_prefix_pathspec(prefix, argv, argc, 0);
modes = xcalloc(argc, sizeof(enum update_mode));
diff --git a/builtin/reset.c b/builtin/reset.c
index 7f1c3f02a30..fd514eec822 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -237,8 +237,7 @@ static void parse_args(struct pathspec *pathspec,
}
*rev_ret = rev;
 
-   if (read_cache() < 0)
-   die(_("index file corrupt"));
+   repo_read_index_or_die(the_repository);
 
parse_pathspec(pathspec, 0,
   PATHSPEC_PREFER_FULL |
diff --git a/builtin/rm.c b/builtin/rm.c
index 5b6fc7ee818..3b90191aa53 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -267,8 +267,7 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
 
hold_locked_index(_file, LOCK_DIE_ON_ERROR);
 
-   if (read_cache() < 0)
-   die(_("index file corrupt"));
+   repo_read_index_or_die(the_repository);
 
parse_pathspec(, 0,
   PATHSPEC_PREFER_CWD,
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index c2403a915ff..7aebed9bd66 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -323,8 +323,7 @@ static int module_list_compute(int argc, const char **argv,
if (pathspec->nr)
ps_matched = xcalloc(pathspec->nr, 1);
 
-   if (read_cache() < 0)
-   die(_("index file corrupt"));
+   repo_read_index_or_die(the_repository);
 
for (i = 0; i < active_nr; i++) {
const struct cache_entry *ce = active_cache[i];
diff --git a/repository.c b/repository.c
index beff3caa9e2..ceca14ba718 100644
--- a/repository.c
+++ b/repository.c
@@ 

[PATCH 03/11] builtin/grep: use repo_read_index_or_die

[Stefan Beller]

grep is a porcelain command, so translating its error message is a good
idea.

Signed-off-by: Stefan Beller 
---
 builtin/grep.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 69f0743619f..2c2d6cc6bca 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -488,8 +488,7 @@ static int grep_cache(struct grep_opt *opt, struct 
repository *repo,
strbuf_addstr(, repo->submodule_prefix);
}
 
-   if (repo_read_index(repo) < 0)
-   die("index file corrupt");
+   repo_read_index_or_die(repo);
 
for (nr = 0; nr < repo->index->cache_nr; nr++) {
const struct cache_entry *ce = repo->index->cache[nr];
-- 
2.17.0.582.gccdcbd54c44.dirty

[PATCH 00/11]

[Stefan Beller]

> If you have time, yes translate them all. I don't see how any of these
> strings are meant for script. If not, just _() the new string you
> added is fine.

> With a majority of call sites dying like this though, I wonder if we
> should just add repo_read_index_or_die() with die() inside. Then the
> next person won't likely accidentally forget _()

So this comment tricked me into coming up with a patch series. :)

Each patch is themed, I tried to make each commit special w.r.t. reviewers
attention.

We'd start out with a resend of the origin patch, which is boring.

Then we'll move all similar cases into one function (no-op for
introducing the repo_read_index_or_die function as all callers will have the
same error message and the same localisation).

Any following patch will be more controversial then the previous patches,
I would expect, as we introduce more and more change.

The last patch is just an attempt to finish the series gracefully,
and may contain errors (sometimes we do not want to die() in case of
corrupt index).

Is this series roughly what you had in mind?

Thanks,
Stefan


Stefan Beller (11):
  grep: handle corrupt index files early
  repository: introduce repo_read_index_or_die
  builtin/grep: use repo_read_index_or_die
  submodule: use repo_read_index_or_die
  builtin/ls-files: use repo_read_index_or_die
  read_cache: use repo_read_index_or_die with different error messages
  rerere: use repo_read_index_or_die
  check-attr: switch to repo_read_index_or_die
  checkout-index: switch to repo_read_index
  test helpers: switch to repo_read_index_or_die
  read_cache: convert most calls to repo_read_index_or_die

 blame.c  |  5 +++--
 builtin/add.c|  7 +++
 builtin/am.c |  3 ++-
 builtin/check-attr.c |  5 ++---
 builtin/check-ignore.c   |  7 ---
 builtin/checkout-index.c |  5 ++---
 builtin/clean.c  |  4 ++--
 builtin/commit.c |  9 +
 builtin/diff.c   |  3 ++-
 builtin/fsck.c   |  3 ++-
 builtin/grep.c   |  2 +-
 builtin/ls-files.c   |  7 +++
 builtin/merge-index.c|  3 ++-
 builtin/mv.c |  3 +--
 builtin/reset.c  |  3 +--
 builtin/rev-parse.c  |  4 ++--
 builtin/rm.c |  3 +--
 builtin/submodule--helper.c  |  3 +--
 check-racy.c |  2 +-
 diff.c   |  5 +++--
 merge-recursive.c|  3 ++-
 merge.c  |  4 ++--
 repository.c |  6 ++
 repository.h |  8 
 rerere.c | 10 --
 revision.c   |  5 +++--
 sequencer.c  |  5 +++--
 sha1-name.c  |  2 +-
 submodule.c  |  3 +--
 t/helper/test-dump-cache-tree.c  |  5 ++---
 t/helper/test-dump-untracked-cache.c |  4 ++--
 t/helper/test-lazy-init-name-hash.c  | 11 ++-
 t/helper/test-read-cache.c   |  3 ++-
 t/helper/test-scrap-cache-tree.c |  4 ++--
 t/helper/test-write-cache.c  |  3 ++-
 35 files changed, 89 insertions(+), 73 deletions(-)

-- 
2.17.0.582.gccdcbd54c44.dirty

[PATCH 08/11] check-attr: switch to repo_read_index_or_die

[Stefan Beller]

Signed-off-by: Stefan Beller 
---
 builtin/check-attr.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/builtin/check-attr.c b/builtin/check-attr.c
index 91444dc0448..bf05e7e93ca 100644
--- a/builtin/check-attr.c
+++ b/builtin/check-attr.c
@@ -4,6 +4,7 @@
 #include "attr.h"
 #include "quote.h"
 #include "parse-options.h"
+#include "repository.h"
 
 static int all_attrs;
 static int cached_attrs;
@@ -115,9 +116,7 @@ int cmd_check_attr(int argc, const char **argv, const char 
*prefix)
argc = parse_options(argc, argv, prefix, check_attr_options,
 check_attr_usage, PARSE_OPT_KEEP_DASHDASH);
 
-   if (read_cache() < 0) {
-   die("invalid cache");
-   }
+   repo_read_index_or_die(the_repository);
 
if (cached_attrs)
git_attr_set_direction(GIT_ATTR_INDEX, NULL);
-- 
2.17.0.582.gccdcbd54c44.dirty

[PATCH 10/11] test helpers: switch to repo_read_index_or_die

[Stefan Beller]

Signed-off-by: Stefan Beller 
---
 t/helper/test-dump-cache-tree.c  |  5 ++---
 t/helper/test-dump-untracked-cache.c |  4 ++--
 t/helper/test-lazy-init-name-hash.c  | 11 ++-
 t/helper/test-read-cache.c   |  3 ++-
 t/helper/test-scrap-cache-tree.c |  4 ++--
 t/helper/test-write-cache.c  |  3 ++-
 6 files changed, 16 insertions(+), 14 deletions(-)

diff --git a/t/helper/test-dump-cache-tree.c b/t/helper/test-dump-cache-tree.c
index 98a4891f1dc..a58c26084dd 100644
--- a/t/helper/test-dump-cache-tree.c
+++ b/t/helper/test-dump-cache-tree.c
@@ -2,7 +2,7 @@
 #include "cache.h"
 #include "tree.h"
 #include "cache-tree.h"
-
+#include "repository.h"
 
 static void dump_one(struct cache_tree *it, const char *pfx, const char *x)
 {
@@ -60,8 +60,7 @@ int cmd__dump_cache_tree(int ac, const char **av)
struct index_state istate;
struct cache_tree *another = cache_tree();
setup_git_directory();
-   if (read_cache() < 0)
-   die("unable to read index file");
+   repo_read_index_or_die(the_repository);
istate = the_index;
istate.cache_tree = another;
cache_tree_update(, WRITE_TREE_DRY_RUN);
diff --git a/t/helper/test-dump-untracked-cache.c 
b/t/helper/test-dump-untracked-cache.c
index d7c55c2355e..171ba143c9a 100644
--- a/t/helper/test-dump-untracked-cache.c
+++ b/t/helper/test-dump-untracked-cache.c
@@ -1,5 +1,6 @@
 #include "cache.h"
 #include "dir.h"
+#include "repository.h"
 
 static int compare_untracked(const void *a_, const void *b_)
 {
@@ -47,8 +48,7 @@ int cmd_main(int ac, const char **av)
ignore_untracked_cache_config = 1;
 
setup_git_directory();
-   if (read_cache() < 0)
-   die("unable to read index file");
+   repo_read_index_or_die(the_repository);
uc = the_index.untracked;
if (!uc) {
printf("no untracked cache\n");
diff --git a/t/helper/test-lazy-init-name-hash.c 
b/t/helper/test-lazy-init-name-hash.c
index b99a37080d9..e25bfe27ab9 100644
--- a/t/helper/test-lazy-init-name-hash.c
+++ b/t/helper/test-lazy-init-name-hash.c
@@ -1,6 +1,7 @@
 #include "test-tool.h"
 #include "cache.h"
 #include "parse-options.h"
+#include "repository.h"
 
 static int single;
 static int multi;
@@ -32,7 +33,7 @@ static void dump_run(void)
struct dir_entry *dir;
struct cache_entry *ce;
 
-   read_cache();
+   repo_read_index_or_die(the_repository);
if (single) {
test_lazy_init_name_hash(_index, 0);
} else {
@@ -70,7 +71,7 @@ static uint64_t time_runs(int try_threaded)
 
for (i = 0; i < count; i++) {
t0 = getnanotime();
-   read_cache();
+   repo_read_index_or_die(the_repository);
t1 = getnanotime();
nr_threads_used = test_lazy_init_name_hash(_index, 
try_threaded);
t2 = getnanotime();
@@ -117,7 +118,7 @@ static void analyze_run(void)
int i;
int nr;
 
-   read_cache();
+   repo_read_index_or_die(the_repository);
cache_nr_limit = the_index.cache_nr;
discard_cache();
 
@@ -132,7 +133,7 @@ static void analyze_run(void)
nr = cache_nr_limit;
 
for (i = 0; i < count; i++) {
-   read_cache();
+   repo_read_index_or_die(the_repository);
the_index.cache_nr = nr; /* cheap truncate of index */
t1s = getnanotime();
test_lazy_init_name_hash(_index, 0);
@@ -141,7 +142,7 @@ static void analyze_run(void)
the_index.cache_nr = cache_nr_limit;
discard_cache();
 
-   read_cache();
+   repo_read_index_or_die(the_repository);
the_index.cache_nr = nr; /* cheap truncate of index */
t1m = getnanotime();
nr_threads_used = test_lazy_init_name_hash(_index, 
1);
diff --git a/t/helper/test-read-cache.c b/t/helper/test-read-cache.c
index d674c88ba09..7a4a91bf328 100644
--- a/t/helper/test-read-cache.c
+++ b/t/helper/test-read-cache.c
@@ -1,5 +1,6 @@
 #include "test-tool.h"
 #include "cache.h"
+#include "repository.h"
 
 int cmd__read_cache(int argc, const char **argv)
 {
@@ -8,7 +9,7 @@ int cmd__read_cache(int argc, const char **argv)
cnt = strtol(argv[1], NULL, 0);
setup_git_directory();
for (i = 0; i < cnt; i++) {
-   read_cache();
+   repo_read_index_or_die(the_repository);
discard_cache();
}
return 0;
diff --git a/t/helper/test-scrap-cache-tree.c b/t/helper/test-scrap-cache-tree.c
index d26d3e7c8b1..401917abb49 100644
--- a/t/helper/test-scrap-cache-tree.c
+++ b/t/helper/test-scrap-cache-tree.c
@@ -3,6 +3,7 @@
 #include "lockfile.h"
 #include "tree.h"
 #include "cache-tree.h"
+#include 

[PATCH 11/11] read_cache: convert most calls to repo_read_index_or_die

[Stefan Beller]

Signed-off-by: Stefan Beller 
---
 blame.c   | 5 +++--
 builtin/am.c  | 3 ++-
 builtin/diff.c| 3 ++-
 builtin/fsck.c| 3 ++-
 builtin/merge-index.c | 3 ++-
 check-racy.c  | 2 +-
 diff.c| 5 +++--
 merge-recursive.c | 3 ++-
 revision.c| 5 +++--
 sequencer.c   | 5 +++--
 sha1-name.c   | 2 +-
 11 files changed, 24 insertions(+), 15 deletions(-)

diff --git a/blame.c b/blame.c
index 78c9808bd1a..ebfa1c8efcd 100644
--- a/blame.c
+++ b/blame.c
@@ -5,6 +5,7 @@
 #include "diff.h"
 #include "diffcore.h"
 #include "tag.h"
+#include "repository.h"
 #include "blame.h"
 
 void blame_origin_decref(struct blame_origin *o)
@@ -159,7 +160,7 @@ static struct commit *fake_working_tree_commit(struct 
diff_options *opt,
unsigned mode;
struct strbuf msg = STRBUF_INIT;
 
-   read_cache();
+   repo_read_index_or_die(the_repository);
time();
commit = alloc_commit_node();
commit->object.parsed = 1;
@@ -241,7 +242,7 @@ static struct commit *fake_working_tree_commit(struct 
diff_options *opt,
 * want to run "diff-index --cached".
 */
discard_cache();
-   read_cache();
+   repo_read_index_or_die(the_repository);
 
len = strlen(path);
if (!mode) {
diff --git a/builtin/am.c b/builtin/am.c
index d834f9e62b6..3c6e77a5369 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -32,6 +32,7 @@
 #include "apply.h"
 #include "string-list.h"
 #include "packfile.h"
+#include "repository.h"
 
 /**
  * Returns 1 if the file is empty or does not exist, 0 otherwise.
@@ -1581,7 +1582,7 @@ static int fall_back_threeway(const struct am_state 
*state, const char *index_pa
say(state, stdout, _("Falling back to patching base and 3-way 
merge..."));
 
discard_cache();
-   read_cache();
+   repo_read_index_or_die(the_repository);
 
/*
 * This is not so wrong. Depending on which base we picked, orig_tree
diff --git a/builtin/diff.c b/builtin/diff.c
index 16bfb22f738..4bba211f1c7 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -17,6 +17,7 @@
 #include "builtin.h"
 #include "submodule.h"
 #include "sha1-array.h"
+#include "repository.h"
 
 #define DIFF_NO_INDEX_EXPLICIT 1
 #define DIFF_NO_INDEX_IMPLICIT 2
@@ -210,7 +211,7 @@ static void refresh_index_quietly(void)
if (fd < 0)
return;
discard_cache();
-   read_cache();
+   repo_read_index(the_repository); /* do not die on error */
refresh_cache(REFRESH_QUIET|REFRESH_UNMERGED);
update_index_if_able(_index, _file);
 }
diff --git a/builtin/fsck.c b/builtin/fsck.c
index 087360a6757..a42e98235da 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -18,6 +18,7 @@
 #include "decorate.h"
 #include "packfile.h"
 #include "object-store.h"
+#include "repository.h"
 
 #define REACHABLE 0x0001
 #define SEEN  0x0002
@@ -795,7 +796,7 @@ int cmd_fsck(int argc, const char **argv, const char 
*prefix)
if (keep_cache_objects) {
verify_index_checksum = 1;
verify_ce_order = 1;
-   read_cache();
+   repo_read_index_or_die(the_repository);
for (i = 0; i < active_nr; i++) {
unsigned int mode;
struct blob *blob;
diff --git a/builtin/merge-index.c b/builtin/merge-index.c
index c99443b095b..2d91c7c3b5e 100644
--- a/builtin/merge-index.c
+++ b/builtin/merge-index.c
@@ -1,5 +1,6 @@
 #include "builtin.h"
 #include "run-command.h"
+#include "repository.h"
 
 static const char *pgm;
 static int one_shot, quiet;
@@ -77,7 +78,7 @@ int cmd_merge_index(int argc, const char **argv, const char 
*prefix)
if (argc < 3)
usage("git merge-index [-o] [-q]  (-a | [--] 
[...])");
 
-   read_cache();
+   repo_read_index_or_die(the_repository);
 
i = 1;
if (!strcmp(argv[i], "-o")) {
diff --git a/check-racy.c b/check-racy.c
index 24b6542352a..9b884639cf4 100644
--- a/check-racy.c
+++ b/check-racy.c
@@ -6,7 +6,7 @@ int main(int ac, char **av)
int dirty, clean, racy;
 
dirty = clean = racy = 0;
-   read_cache();
+   repo_read_index_or_die(the_repository);
for (i = 0; i < active_nr; i++) {
struct cache_entry *ce = active_cache[i];
struct stat st;
diff --git a/diff.c b/diff.c
index 1289df4b1f9..383f52fa118 100644
--- a/diff.c
+++ b/diff.c
@@ -22,6 +22,7 @@
 #include "argv-array.h"
 #include "graph.h"
 #include "packfile.h"
+#include "repository.h"
 
 #ifdef NO_FAST_WORKING_DIRECTORY
 #define FAST_WORKING_DIRECTORY 0
@@ -4210,13 +4211,13 @@ void diff_setup_done(struct diff_options *options)
options->rename_limit = diff_rename_limit_default;
if (options->setup & DIFF_SETUP_USE_CACHE) {
if (!active_cache)
-   /* read-cache does not die even when it fails
+ 

[PATCH 05/11] builtin/ls-files: use repo_read_index_or_die

[Stefan Beller]

Despite ls-files being a plumbing command, which promises to not change its
output ever, and to be easy on machines (e.g. non-localized output),
it may make sense to localize the error message for a corrupt index
nevertheless:

1. that is more consistent with the rest of Git.
2. Searching for "ls-tree corrupt index file" on the web doesn't yield
   any hits, that suggest the exact string is parsed for.
   Probably the script authors rely on the exit code of ls-tree anyways.

Signed-off-by: Stefan Beller 
---
 builtin/ls-files.c | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index a71f6bd088a..502f2f6db04 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -20,6 +20,7 @@
 #include "run-command.h"
 #include "submodule.h"
 #include "submodule-config.h"
+#include "repository.h"
 
 static int abbrev;
 static int show_deleted;
@@ -210,8 +211,7 @@ static void show_submodule(struct repository *superproject,
if (repo_submodule_init(, superproject, path))
return;
 
-   if (repo_read_index() < 0)
-   die("index file corrupt");
+   repo_read_index_or_die();
 
show_files(, dir);
 
@@ -579,8 +579,7 @@ int cmd_ls_files(int argc, const char **argv, const char 
*cmd_prefix)
prefix_len = strlen(prefix);
git_config(git_default_config, NULL);
 
-   if (repo_read_index(the_repository) < 0)
-   die("index file corrupt");
+   repo_read_index_or_die(the_repository);
 
argc = parse_options(argc, argv, prefix, builtin_ls_files_options,
ls_files_usage, 0);
-- 
2.17.0.582.gccdcbd54c44.dirty

[PATCH 09/11] checkout-index: switch to repo_read_index

[Stefan Beller]

Signed-off-by: Stefan Beller 
---
 builtin/checkout-index.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
index a730f6a1aa4..aaba99d36c0 100644
--- a/builtin/checkout-index.c
+++ b/builtin/checkout-index.c
@@ -10,6 +10,7 @@
 #include "quote.h"
 #include "cache-tree.h"
 #include "parse-options.h"
+#include "repository.h"
 
 #define CHECKOUT_ALL 4
 static int nul_term_line;
@@ -184,9 +185,7 @@ int cmd_checkout_index(int argc, const char **argv, const 
char *prefix)
git_config(git_default_config, NULL);
prefix_length = prefix ? strlen(prefix) : 0;
 
-   if (read_cache() < 0) {
-   die("invalid cache");
-   }
+   repo_read_index_or_die(the_repository);
 
argc = parse_options(argc, argv, prefix, builtin_checkout_index_options,
builtin_checkout_index_usage, 0);
-- 
2.17.0.582.gccdcbd54c44.dirty

[PATCH 07/11] rerere: use repo_read_index_or_die

[Stefan Beller]

By switching to repo_read_index_or_die, we'll get a slightly different
error message ("index file corrupt") as well as localization of it.

Signed-off-by: Stefan Beller 
---
 rerere.c | 10 --
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/rerere.c b/rerere.c
index 18cae2d11c9..5f35dd66f90 100644
--- a/rerere.c
+++ b/rerere.c
@@ -10,6 +10,7 @@
 #include "attr.h"
 #include "pathspec.h"
 #include "sha1-lookup.h"
+#include "repository.h"
 
 #define RESOLVED 0
 #define PUNTED 1
@@ -567,8 +568,7 @@ static int check_one_conflict(int i, int *type)
 static int find_conflict(struct string_list *conflict)
 {
int i;
-   if (read_cache() < 0)
-   return error("Could not read index");
+   repo_read_index_or_die(the_repository);
 
for (i = 0; i < active_nr;) {
int conflict_type;
@@ -600,8 +600,7 @@ int rerere_remaining(struct string_list *merge_rr)
int i;
if (setup_rerere(merge_rr, RERERE_READONLY))
return 0;
-   if (read_cache() < 0)
-   return error("Could not read index");
+   repo_read_index_or_die(the_repository);
 
for (i = 0; i < active_nr;) {
int conflict_type;
@@ -1103,8 +1102,7 @@ int rerere_forget(struct pathspec *pathspec)
struct string_list conflict = STRING_LIST_INIT_DUP;
struct string_list merge_rr = STRING_LIST_INIT_DUP;
 
-   if (read_cache() < 0)
-   return error("Could not read index");
+   repo_read_index_or_die(the_repository);
 
fd = setup_rerere(_rr, RERERE_NOAUTOUPDATE);
if (fd < 0)
-- 
2.17.0.582.gccdcbd54c44.dirty

[PATCH 01/11] grep: handle corrupt index files early

[Stefan Beller]

Any other caller of 'repo_read_index' dies upon a negative return of
it, so grep should, too.

Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 builtin/grep.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 6e7bc76785a..69f0743619f 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -488,7 +488,8 @@ static int grep_cache(struct grep_opt *opt, struct 
repository *repo,
strbuf_addstr(, repo->submodule_prefix);
}
 
-   repo_read_index(repo);
+   if (repo_read_index(repo) < 0)
+   die("index file corrupt");
 
for (nr = 0; nr < repo->index->cache_nr; nr++) {
const struct cache_entry *ce = repo->index->cache[nr];
-- 
2.17.0.582.gccdcbd54c44.dirty

[PATCH 06/11] read_cache: use repo_read_index_or_die with different error messages

[Stefan Beller]

This replaces all patterns of "if (read_cached() < 0) die(some-msg);"
with repo_read_index_or_die; this changes the output of the error message.

However as all error messages before were translated, these are for human
consumption, so a change in error message is not bad; in fact it makes Git
more consistent.

Signed-off-by: Stefan Beller 
---
 builtin/add.c   | 4 ++--
 builtin/commit.c| 9 +
 builtin/rev-parse.c | 4 ++--
 merge.c | 4 ++--
 4 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index e4751c198c1..910f619b7d5 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -19,6 +19,7 @@
 #include "bulk-checkin.h"
 #include "argv-array.h"
 #include "submodule.h"
+#include "repository.h"
 
 static const char * const builtin_add_usage[] = {
N_("git add [] [--] ..."),
@@ -229,8 +230,7 @@ static int edit_patch(int argc, const char **argv, const 
char *prefix)
 
git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
 
-   if (read_cache() < 0)
-   die(_("Could not read the index"));
+   repo_read_index_or_die(the_repository);
 
init_revisions(, prefix);
rev.diffopt.context = 7;
diff --git a/builtin/commit.c b/builtin/commit.c
index 5571d4a3e2b..9ebfb4db415 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -32,6 +32,8 @@
 #include "column.h"
 #include "sequencer.h"
 #include "mailmap.h"
+#include "sigchain.h"
+#include "repository.h"
 
 static const char * const builtin_commit_usage[] = {
N_("git commit [] [--] ..."),
@@ -428,8 +430,7 @@ static const char *prepare_index(int argc, const char 
**argv, const char *prefix
exit(1);
 
discard_cache();
-   if (read_cache() < 0)
-   die(_("cannot read the index"));
+   repo_read_index_or_die(the_repository);
 
hold_locked_index(_lock, LOCK_DIE_ON_ERROR);
add_remove_files();
@@ -853,8 +854,8 @@ static int prepare_to_commit(const char *index_file, const 
char *prefix,
struct object_id oid;
const char *parent = "HEAD";
 
-   if (!active_nr && read_cache() < 0)
-   die(_("Cannot read index"));
+   if (!active_nr)
+   repo_read_index_or_die(the_repository);
 
if (amend)
parent = "HEAD^1";
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 36b20877828..37f29fd850d 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -14,6 +14,7 @@
 #include "revision.h"
 #include "split-index.h"
 #include "submodule.h"
+#include "repository.h"
 
 #define DO_REVS1
 #define DO_NOREV   2
@@ -884,8 +885,7 @@ int cmd_rev_parse(int argc, const char **argv, const char 
*prefix)
continue;
}
if (!strcmp(arg, "--shared-index-path")) {
-   if (read_cache() < 0)
-   die(_("Could not read the index"));
+   repo_read_index_or_die(the_repository);
if (the_index.split_index) {
const unsigned char *sha1 = 
the_index.split_index->base_sha1;
const char *path = 
git_path("sharedindex.%s", sha1_to_hex(sha1));
diff --git a/merge.c b/merge.c
index f06a4773d4f..654d049e80d 100644
--- a/merge.c
+++ b/merge.c
@@ -8,6 +8,7 @@
 #include "tree-walk.h"
 #include "unpack-trees.h"
 #include "dir.h"
+#include "repository.h"
 
 static const char *merge_argument(struct commit *commit)
 {
@@ -70,8 +71,7 @@ int try_merge_command(const char *strategy, size_t xopts_nr,
argv_array_clear();
 
discard_cache();
-   if (read_cache() < 0)
-   die(_("failed to read the cache"));
+   repo_read_index_or_die(the_repository);
resolve_undo_clear();
 
return ret;
-- 
2.17.0.582.gccdcbd54c44.dirty

Re: [PATCH] shallow: remove unused variable

[Stefan Beller]

Hi Ramsay,

> That commit seems to rename the 'shallow_stat' symbol to the 
> 'the_repository_shallow_stat' symbol, but at the same time adds an 
> 'shallow_stat' field to the parsed_object_pool struct, so ... :(

Thanks for catching this! it shows again, how rebase can be a
dangerous tool if not used properly.
I'll look into this, and the solution most likely will be to squash
this patch into that commit.

Thanks!
Stefan

[PATCH] shallow: remove unused variable

[Ramsay Jones]

Signed-off-by: Ramsay Jones 
---

Hi Stefan,

If you need to re-roll your 'sb/object-store-grafts' branch, could
you please squash this into the relevant patch (whichever one that
would be)! ;-)

I have not looked to see which patch needs to change (just being
lazy, sorry!), but this variable was introduced by commit d73b49b707
("shallow: migrate shallow information into the object parser", 2018-05-15).

That commit seems to rename the 'shallow_stat' symbol to the 
'the_repository_shallow_stat' symbol, but at the same time adds an 
'shallow_stat' field to the parsed_object_pool struct, so ... :(

Thanks!

ATB,
Ramsay Jones

 shallow.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/shallow.c b/shallow.c
index 74bc78801..51447608a 100644
--- a/shallow.c
+++ b/shallow.c
@@ -17,8 +17,6 @@
 #include "commit-slab.h"
 #include "repository.h"
 
-struct stat_validity the_repository_shallow_stat;
-
 void set_alternate_shallow_file(struct repository *r, const char *path, int 
override)
 {
if (r->parsed_objects->is_shallow != -1)
-- 
2.17.0

Re: [PATCH v2 0/3] unpack_trees_options: free messages when done

[Elijah Newren]

Hi Martin,

On Wed, May 16, 2018 at 9:30 AM, Martin Ågren  wrote:
> On 16 May 2018 at 16:32, Elijah Newren  wrote:
>> On Sat, Apr 28, 2018 at 4:32 AM, Martin Ågren  wrote:
>>> As you noted elsewhere [1], Ben is also working in this area. I'd be
>>> perfectly happy to sit on these patches until both of your contributions
>>> come through to master.
>>>
>>> [1] 
>>> https://public-inbox.org/git/CABPp-BFh=gl6rnbst2bgtynkij1z5tmgar1via5_vytef5e...@mail.gmail.com/
>>
>> Instead of waiting for these to come through to master, could you just
>> submit based on the top of bp/merge-rename-config?
>
> Sure, here goes. This is based on bp/merge-rename-config, gets rid of
> all leaks of memory allocated in `setup_unpack_trees_porcelain()` and
> cuts the number of leaks in the test-suite (i.e., the subset of the
> tests that I run) by around 10%.

Awesome, thanks.  I've looked over patches 2 & 3; they look good to me.

Re: [PATCH] refspec.h: reinstate 'extern' to fix sparse warning

[Johannes Schindelin]

On Wed, 16 May 2018, Stefan Beller wrote:

> On Wed, May 16, 2018 at 2:42 PM, Brandon Williams  wrote:
>
> > Though now I'm confused, I thought we were going towards eliminating
> > using the extern keyword? ...of course I guess it means something
> > _slightly_ different when using with a variable vs a function :)
> 
> We're only eliminating it when it is redundant. :-)
> 
> For variables this is not redundant as we need it to tell apart the
> declaration and definition of it, so we have to keep it.

Otherwise we will end up with the variable *defined* for every file that
includes that header. And of course those different versions of the same
variable would have possibly different values...

Re: [PATCH] refspec.h: reinstate 'extern' to fix sparse warning

[Stefan Beller]

On Wed, May 16, 2018 at 2:42 PM, Brandon Williams  wrote:
> On 05/16, Ramsay Jones wrote:
>>
>> Signed-off-by: Ramsay Jones 
>> ---
>>
>> Hi Brandon,
>>
>> If you need to re-roll your 'bw/refspec-api' branch, could you please
>> squash this, or the equivalent change before the 'struct refname' to
>> 'struct refname_item' name change, into the relevant patch. (which
>> would be patch #1, commit 8999381ed).
>>
>> This patch was built on top of 'pu', but as I said above, patch #1
>> is where the original 'extern' keyword was dropped. (see first hunk
>> of the diff to 'remote.h').
>
> Of course I'll do that, I'm planning on sending out a v2 by the end of
> the day and I'll incorporate that.
>
> Though now I'm confused, I thought we were going towards eliminating
> using the extern keyword? ...of course I guess it means something
> _slightly_ different when using with a variable vs a function :)

We're only eliminating it when it is redundant. :-)

For variables this is not redundant as we need it to tell apart the
declaration and definition of it, so we have to keep it.

Re: [PATCH] refspec.h: reinstate 'extern' to fix sparse warning

[Brandon Williams]

On 05/16, Ramsay Jones wrote:
> 
> Signed-off-by: Ramsay Jones 
> ---
> 
> Hi Brandon,
> 
> If you need to re-roll your 'bw/refspec-api' branch, could you please
> squash this, or the equivalent change before the 'struct refname' to
> 'struct refname_item' name change, into the relevant patch. (which
> would be patch #1, commit 8999381ed).
> 
> This patch was built on top of 'pu', but as I said above, patch #1
> is where the original 'extern' keyword was dropped. (see first hunk
> of the diff to 'remote.h').

Of course I'll do that, I'm planning on sending out a v2 by the end of
the day and I'll incorporate that.

Though now I'm confused, I thought we were going towards eliminating
using the extern keyword? ...of course I guess it means something
_slightly_ different when using with a variable vs a function :)

> 
> Thanks!
> 
> ATB,
> Ramsay Jones
> 
>  refspec.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/refspec.h b/refspec.h
> index 374f8ea63..7e1ff94ac 100644
> --- a/refspec.h
> +++ b/refspec.h
> @@ -2,7 +2,7 @@
>  #define REFSPEC_H
>  
>  #define TAG_REFSPEC "refs/tags/*:refs/tags/*"
> -const struct refspec_item *tag_refspec;
> +extern const struct refspec_item *tag_refspec;
>  
>  struct refspec_item {
>   unsigned force : 1;
> -- 
> 2.17.0

-- 
Brandon Williams

[PATCH] refspec.h: reinstate 'extern' to fix sparse warning

[Ramsay Jones]

Signed-off-by: Ramsay Jones 
---

Hi Brandon,

If you need to re-roll your 'bw/refspec-api' branch, could you please
squash this, or the equivalent change before the 'struct refname' to
'struct refname_item' name change, into the relevant patch. (which
would be patch #1, commit 8999381ed).

This patch was built on top of 'pu', but as I said above, patch #1
is where the original 'extern' keyword was dropped. (see first hunk
of the diff to 'remote.h').

Thanks!

ATB,
Ramsay Jones

 refspec.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/refspec.h b/refspec.h
index 374f8ea63..7e1ff94ac 100644
--- a/refspec.h
+++ b/refspec.h
@@ -2,7 +2,7 @@
 #define REFSPEC_H
 
 #define TAG_REFSPEC "refs/tags/*:refs/tags/*"
-const struct refspec_item *tag_refspec;
+extern const struct refspec_item *tag_refspec;
 
 struct refspec_item {
unsigned force : 1;
-- 
2.17.0

Re: [PATCH v2 1/3] merge: setup `opts` later in `checkout_fast_forward()`

[Jacob Keller]

On Wed, May 16, 2018 at 12:29 PM, Martin Ågren  wrote:
> On 16 May 2018 at 18:41, Stefan Beller  wrote:
>> On Wed, May 16, 2018 at 9:30 AM, Martin Ågren  wrote:
>>>
>>> This patch is best viewed using something like this (note the tab!):
>>> --color-moved --anchored="  trees[nr_trees] = parse_tree_indirect"
>>
>> Heh! Having a "is best viewed" paragraph is the new shiny thing in
>> commit messages as 'git log origin/pu --grep "is best viewed"' tells me.
>
> :-)
>
>> Regarding the anchoring, I wonder if we can improve it by ignoring
>> whitespaces or just looking for substrings, or by allowing regexes or ...
>
> FWIW, because my first naive attempt failed (for some reason I did not
> consider the leading tab part of the "line" so I did not provide it), I
> had the same thought. Ignoring leading whitespace seemed easy enough in
> the implementation.
>
> Then I started thinking about all the ways in which whitespace can be
> ignored. My reaction in the end was to not try and open that can right
> there and then. I did not think about regexes.
>
> I guess this boils down to the usage. Copying the line to anchor on from
> an editor could run into these kind of whitespace-issues, and shell
> escaping. Typing an anchor could become easier with regexes since one
> could skip typing common substrings and just anchor on /unique-part/.
>
> Martin

Simpler approach is to just match substring instead. Then, the user
can decide how much of the string is required to get the anchor they
wanted.

Thanks,
Jake

Re: [PATCH v2 1/3] merge: setup `opts` later in `checkout_fast_forward()`

[Jacob Keller]

On Wed, May 16, 2018 at 9:41 AM, Stefan Beller  wrote:
> + Jonathan Tan for a side discussion on anchoring.
>
> On Wed, May 16, 2018 at 9:30 AM, Martin Ågren  wrote:
>>
>> This patch is best viewed using something like this (note the tab!):
>> --color-moved --anchored="  trees[nr_trees] = parse_tree_indirect"
>
> Heh! Having a "is best viewed" paragraph is the new shiny thing in
> commit messages as 'git log origin/pu --grep "is best viewed"' tells me.
>
> Regarding the anchoring, I wonder if we can improve it by ignoring
> whitespaces or just looking for substrings, or by allowing regexes or ...
>
> Thanks,
> Stefan

I think expanding it to be regexp would be nicest. To be honest, I
already thought it was substring based

It'd be *really* cool if we had a way for a commit messages (or maybe
notes?) to indicate the anchor so that git show could (optionally)
figure out the anchor automatically. It's been REALLY useful for me
when showing diffs to be able to provide a better idea of what a human
*actually* did vs what the smallest diff was.

Thanks,
Jake

Re: worktrees vs. alternates

[Stefan Beller]

>
> You can also do periodic maintenance like:
>
>   1. Copy each ref in the forked repositories into the parent repository
>  (e.g., giving each child that borrows from the parent its own
>  hierarchy in refs/remotes//*).

Can you just copy? I assume the mother repo doesn't know about
all objects, hence by copying the ref, we have a "spotty" history.

And to improve copying could permanent symlinking be used instead?

Re: Git log range reverse bug

[Johannes Sixt]

Am 16.05.2018 um 20:19 schrieb Mehdi Zeinali:

Git Version: Version: 2.14.2

When reversing a range in git log, it does not start from the expected commit:


--reverse does not change the meaning A..B to B..A or something. For a 
particular A..B specification, the set of commits selected when 
--reverse is given remains the same. Only the order in which they are 
listed is reversed.


-- Hannes

Re: [PATCH 1/1] git-p4: add unshelve command

[Luke Diamand]

On 13 May 2018 at 14:52, Merland Romain  wrote:
> Hello Luke,
>
> Very interseting
> This is indeed an option we are waiting since the introduction of option 
> --shelve for git p4 submit
> What I like most in your approach is the preservation of link to p4/master 
> inducing small changes of git-p4 existing functions already heavily tested.
> Also I like the dedicated branch you create, it is cleaner and then we can 
> cherry-pick in other git branches.

Thanks, I'd be interested to know how you get on with it!

> We made some basic tries on our side, just adding an option --unshelve to 
> P4Submit (for simplicity, but I like much more your P4Unshelve class)
> and trying to use the diff of p4 to generate a patch when possible, then 
> apply it in the current git branch on top of HEAD.
> Here it is, hope it can help a bit.
> Note it also uses p4 print -o for binary files.

I did try something like this earlier this year (if you look in the
archives you'll find it) but I found that it was getting quite
complicated trying to construct a sensible looking patch file from the
output of p4 describe. Better to let git's existing tools do that, as
they're going to be better than any half-baked attempt I might manage
in Python!

Thanks!
Luke



>
> diff --git a/git-p4.py b/git-p4.py
> index f4a6f3b4c..b466b46e1 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -1353,6 +1353,8 @@ class P4Submit(Command, P4UserMap):
>   metavar="CHANGELIST",
>   help="update an existing shelved 
> changelist, implies --shelve, "
> "repeat in-order for multiple 
> shelved changelists"),
> +optparse.make_option("--unshelve", dest="unshelve",
> + help="unshelve speficied ChangeList 
> into current BRANCH."),
>  optparse.make_option("--commit", dest="commit", 
> metavar="COMMIT",
>   help="submit only the specified 
> commit(s), one commit or xxx..xxx"),
>  optparse.make_option("--disable-rebase", 
> dest="disable_rebase", action="store_true",
> @@ -1367,6 +1369,7 @@ class P4Submit(Command, P4UserMap):
>  self.dry_run = False
>  self.shelve = False
>  self.update_shelve = list()
> +self.unshelve = ""
>  self.commit = ""
>  self.disable_rebase = False
>  self.prepare_p4_only = False
> @@ -2083,6 +2086,66 @@ class P4Submit(Command, P4UserMap):
>  if self.clientPath == "":
>  die("Error: Cannot locate perforce checkout of %s in client 
> view" % self.depotPath)
>
> +# special case of unshelving
> +# todo: put this code in a class like P4Sync or P4Rebase
> +if self.unshelve != "":
> +git_dir = os.getcwd() + '/'
> +print "Importing shelved CL %s into current git branch %s" % 
> (self.unshelve, self.master)
> +description = p4_describe(self.unshelve)
> +
> +# get changed files
> +files = p4CmdList(['files', "@=%s" % self.unshelve])
> +editedFiles = []
> +filesToAdd = []
> +filesToDelete = []
> +binaryFiles = []
> +something_to_commit = False
> +for f in files:
> +if not f["depotFile"].startswith(self.depotPath):
> +print "WARNING: file %s not in this p4 depot - skipping" 
> % f["depotFile"]
> +continue
> +
> +elif f["action"] == 'delete':
> +filesToDelete.append(f)
> +something_to_commit = True
> +elif f["action"] == 'add':
> +filesToAdd.append(f)
> +something_to_commit = True
> +elif f["type"] == 'binary':
> +binaryFiles.append(f)
> +something_to_commit = True
> +elif f["action"] == 'edit':
> +editedFiles.append(f)
> +something_to_commit = True
> +
> +f["clientFile"] = 
> f["depotFile"].replace(self.depotPath,self.clientPath)
> +f["gitFile"] = f["depotFile"].replace(self.depotPath,git_dir)
> +
> +if not something_to_commit:
> +print "Nothing to commit. Exiting"
> +return True
> +
> +# get the diff and copy to diff directory
> +for f in editedFiles:
> +p4diff = p4_read_pipe(['diff2', '-du', 
> f["depotFile"]+'#'+f["rev"], f["depotFile"]+'@='+self.unshelve])
> +p4diff = "\n".join(p4diff.split("\n")[1:])
> +p4diff = '--- '+f["gitFile"]+'\n' + '+++ '+f["gitFile"]+'\n' 
> + p4diff
> +write_pipe(['patch', '-d/', '-p0'], p4diff)
> +write_pipe(['git', 'add', '-f', f["gitFile"]], "")
> +for f in filesToAdd:
> +  

Re: worktrees vs. alternates

[Martin Fick]

On Wednesday, May 16, 2018 01:06:59 PM Jeff King wrote:
> On Wed, May 16, 2018 at 01:40:56PM -0600, Martin Fick 
wrote:
> > > In theory the fetch means that it's safe to actually
> > > prune in the mother repo, but in practice there are
> > > still races. They don't come up often, but if you
> > > have enough repositories, they do eventually. :)
> > 
> > Peff,
> > 
> > I would be very curious to hear what you think of this
> > approach to mitigating the effect of those races?
> > 
> > https://git.eclipse.org/r/c/122288/2
> 
> The crux of the problem is that we have no way to
> atomically mark an object as "I am using this -- do not
> delete" with respect to the actual deletion.
> 
> So if I'm reading your approach correctly, you put objects
> into a purgatory rather than delete them, and let some
> operations rescue them from purgatory if we had a race. 

Yes.  This has the cost of extra disk space for a while, but 
once I realized that we are incurring that cost already 
because for our repos, we already put things into purgatory 
to avoid getting stale NFS File handle errors during 
unrecoverable paths (while streaming an object).  So 
effectively this has no extra space cost then what is needed 
to run safely on NFS.

>   1. When do you rescue from purgatory? Any time the
> object is referenced? Do you then pull in all of its
> reachable objects too?

For my approach, I decided a) Yes b) No

Because:

a) Rescue on reference is cheap and allows any other policy 
to be built upon it, just ensure that policy references it 
at some point before it is prune from the purgatory.

b)  The other referenced objects will likely get pulled in 
on reference anyway or by virtue of being in the same pack.

>   2. How do you decide when to drop an object from
> purgatory? And specifically, how do you avoid racing with
> somebody using the object as you're pruning purgatory?

If you clean the purgatory during repacking after creating 
all the new packs and before deleting the old ones, you will 
have a significant grace window to handle most longer running 
operations.  In this way, repacking will have re-referenced 
any missing objects from the purgatory before it gets pruned 
causing them to be recovered if necessary.  Those missing 
objects, believed to be in the exact packs in the purgatory 
at that time, should only ever have been referenced by write 
operations that started before those packs were moved to the 
purgatory, which was before the previous repacking round 
ended.  This leaves write operations a full repacking cycle 
to complete in to avoid loosing objects.

>   3. How do you know that an operation has been run that
> will actually rescue the object, as opposed to silently
> having a corrupted state on disk?
> 
>  E.g., imagine this sequence:
> 
>a. git-prune computes reachability and finds that
> commit X is ready to be pruned
> 
>b. another process sees that commit X exists and
> builds a commit that references it as a parent
> 
>c. git-prune drops the object into purgatory
> 
>  Now we have a corrupt state created by the process in
> (b), since we have a reachable object in purgatory. But
> what if nobody goes back and tries to read those commits
> in the meantime?

See answer to #2, repacking itself should rescue any objects 
that need to be rescued before pruning the purgatory.

> I think this might be solvable by using the purgatory as a
> kind of "lock", where prune does something like:
> 
>   1. compute reachability
> 
>   2. move candidate objects into purgatory; nobody can
> look into purgatory except us

I don't think this is needed.

It should be OK to let others see the objects in the 
purgatory after 1 and before 3 as long as "seeing" them, 
causes them to be recovered!

>   3. compute reachability _again_, making sure that no
> purgatory objects are used (if so, rollback the deletion
> and try again)

Yes, you laid out the formula, but nothing says this 
recompute can't wait until the next repack (again see my 
answer to #2)!  i.e. there is no rush to cause a recovery as 
long as it gets recovered before it gets pruned from the 
purgatory.


> But even that's not quite there, because you need to have
> some consistent atomic view of what's "used". Just
> checking refs isn't enough, because some other process
> may be planning to reference a purgatory object but not
> yet have updated the ref. So you need some atomic way of
> saying "I am interested in using this object".

As long as all write paths also read the object first (I 
assume they do, or we would be in big trouble already), then 
this should not be an issue.  The idea is to force all reads 
(and thus all writes also) to recover the object,

-Martin

-- 
The Qualcomm Innovation Center, Inc. is a member of Code 
Aurora Forum, hosted by The Linux Foundation

Re: worktrees vs. alternates

[Jeff King]

On Wed, May 16, 2018 at 04:02:53PM -0400, Konstantin Ryabitsev wrote:

> On 05/16/18 15:37, Jeff King wrote:
> > Yes, that's pretty close to what we do at GitHub. Before doing any
> > repacking in the mother repo, we actually do the equivalent of:
> > 
> >   git fetch --prune ../$id.git +refs/*:refs/remotes/$id/*
> >   git repack -Adl
> > 
> > from each child to pick up any new objects to de-duplicate (our "mother"
> > repos are not real repos at all, but just big shared-object stores).
> 
> Yes, I keep thinking of doing the same, too -- instead of using
> torvalds/linux.git for alternates, have an internal repo where objects
> from all forks are stored. This conversation may finally give me the
> shove I've been needing to poke at this. :)
> 
> Is your delta-islands patch heading into upstream, or is that something
> that's going to remain external?

I have vague plans to submit it upstream, but I'm still not convinced
it's quite optimal. The resulting packs tend to be a fair bit larger
than they could be when packed by themselves, because we miss many delta
opportunities (and it's important to "repack -f --window=250" once in a
while, since we're throwing away so many delta candidates).

There's an alternative way of doing it, too, which I think git.or.cz
uses: it "layers" forks in a hierarchy. So if I fork torvalds/linux.git,
then I get my own repo that uses torvalds/linux as an alternate. And if
somebody forks my repo, then I'm their alternate, and they recursively
depend on torvalds/linux. So each fork basically layers a slice of its
own pack on top of the parent.

This is all from recollections of past discussions (which were sadly not
on the list -- I don't know if they've written up their scheme anywhere
public), so I may have some details wrong. But I think that their
repacking is done hierarchically, too: any objects which the root fork
might drop get migrated up to the children instead, and so forth, until
the leaf nodes can actually throw away objects.

The big problem with this is that Git tends to behave better when
objects are in the same pack:

  1. We don't bother looking for new deltas within the same pack,
 whereas a clone of a fork may actually try to find new deltas
 between the layers.

  2. Reachability bitmaps can't cross pack boundaries (due to the way
 they're implemented, but also the current on-disk format). So you
 can only bitmap the root repo, not any of the other layers.

> I feel like a whitepaper on "how we deal with bajillions of forks at
> GitHub" would be nice. :) I was previously told that it's unlikely such
> paper could be written due to so many custom-built things at GH, but I
> would be very happy if that turned out not to be the case.

We have a few engineering blog posts on the subject, like:

  https://githubengineering.com/counting-objects/
  https://githubengineering.com/introducing-dgit/
  https://githubengineering.com/building-resilience-in-spokes/

but we haven't done a very good job of keeping that up. I think a
summary whitepaper would interesting. Maybe one day...:)

-Peff

Re: worktrees vs. alternates

[Jeff King]

On Wed, May 16, 2018 at 01:40:56PM -0600, Martin Fick wrote:

> > In theory the fetch means that it's safe to actually prune
> > in the mother repo, but in practice there are still
> > races. They don't come up often, but if you have enough
> > repositories, they do eventually. :)
> 
> Peff,
> 
> I would be very curious to hear what you think of this 
> approach to mitigating the effect of those races?
> 
> https://git.eclipse.org/r/c/122288/2

The crux of the problem is that we have no way to atomically mark an
object as "I am using this -- do not delete" with respect to the actual
deletion. 

So if I'm reading your approach correctly, you put objects into a
purgatory rather than delete them, and let some operations rescue them
from purgatory if we had a race.  That's certainly a direction we've
considered, but I think there are some open questions, like:

  1. When do you rescue from purgatory? Any time the object is
 referenced? Do you then pull in all of its reachable objects too?

  2. How do you decide when to drop an object from purgatory? And
 specifically, how do you avoid racing with somebody using the
 object as you're pruning purgatory?

  3. How do you know that an operation has been run that will actually
 rescue the object, as opposed to silently having a corrupted state
 on disk?

 E.g., imagine this sequence:

   a. git-prune computes reachability and finds that commit X is
  ready to be pruned

   b. another process sees that commit X exists and builds a commit
  that references it as a parent

   c. git-prune drops the object into purgatory

 Now we have a corrupt state created by the process in (b), since we
 have a reachable object in purgatory. But what if nobody goes back
 and tries to read those commits in the meantime?

I think this might be solvable by using the purgatory as a kind of
"lock", where prune does something like:

  1. compute reachability

  2. move candidate objects into purgatory; nobody can look into
 purgatory except us

  3. compute reachability _again_, making sure that no purgatory objects
 are used (if so, rollback the deletion and try again)

But even that's not quite there, because you need to have some
consistent atomic view of what's "used". Just checking refs isn't
enough, because some other process may be planning to reference a
purgatory object but not yet have updated the ref. So you need some
atomic way of saying "I am interested in using this object".

-Peff

Re: worktrees vs. alternates

[Konstantin Ryabitsev]

On 05/16/18 15:37, Jeff King wrote:
> Yes, that's pretty close to what we do at GitHub. Before doing any
> repacking in the mother repo, we actually do the equivalent of:
> 
>   git fetch --prune ../$id.git +refs/*:refs/remotes/$id/*
>   git repack -Adl
> 
> from each child to pick up any new objects to de-duplicate (our "mother"
> repos are not real repos at all, but just big shared-object stores).

Yes, I keep thinking of doing the same, too -- instead of using
torvalds/linux.git for alternates, have an internal repo where objects
from all forks are stored. This conversation may finally give me the
shove I've been needing to poke at this. :)

Is your delta-islands patch heading into upstream, or is that something
that's going to remain external?

> I say "equivalent" because those commands can actually be a bit slow. So
> we do some hacky tricks like directly moving objects in the filesystem.
> 
> In theory the fetch means that it's safe to actually prune in the mother
> repo, but in practice there are still races. They don't come up often,
> but if you have enough repositories, they do eventually. :)

I feel like a whitepaper on "how we deal with bajillions of forks at
GitHub" would be nice. :) I was previously told that it's unlikely such
paper could be written due to so many custom-built things at GH, but I
would be very happy if that turned out not to be the case.

Best,
-- 
Konstantin Ryabitsev
Director, IT Infrastructure Security
The Linux Foundation



signature.asc
Description: OpenPGP digital signature

Re: worktrees vs. alternates

[Martin Fick]

On Wednesday, May 16, 2018 12:37:45 PM Jeff King wrote:
> On Wed, May 16, 2018 at 03:29:42PM -0400, Konstantin 
Ryabitsev wrote:
> Yes, that's pretty close to what we do at GitHub. Before
> doing any repacking in the mother repo, we actually do
> the equivalent of:
> 
>   git fetch --prune ../$id.git +refs/*:refs/remotes/$id/*
>   git repack -Adl
> 
> from each child to pick up any new objects to de-duplicate
> (our "mother" repos are not real repos at all, but just
> big shared-object stores).
... 
> In theory the fetch means that it's safe to actually prune
> in the mother repo, but in practice there are still
> races. They don't come up often, but if you have enough
> repositories, they do eventually. :)

Peff,

I would be very curious to hear what you think of this 
approach to mitigating the effect of those races?

https://git.eclipse.org/r/c/122288/2

-Martin
-- 
The Qualcomm Innovation Center, Inc. is a member of Code 
Aurora Forum, hosted by The Linux Foundation

Re: worktrees vs. alternates

[Jeff King]

On Wed, May 16, 2018 at 03:29:42PM -0400, Konstantin Ryabitsev wrote:

> On 05/16/18 15:23, Jeff King wrote:
> > I implemented "repack -k", which keeps all objects and just rolls them
> > into the new pack (along with any currently-loose unreachable objects).
> > Aside from corner cases (e.g., where somebody accidentally added a 20GB
> > file to an otherwise 100MB-repo and then rolled it back), it usually
> > doesn't significantly affect the repository size.
> 
> Hmm... I should read manpages more often! :)
> 
> So, do you suggest that this is a better approach:
> 
> - mother repos: "git repack -adk"
> - child repos: "git repack -Adl" (followed by prune)

Yes, that's pretty close to what we do at GitHub. Before doing any
repacking in the mother repo, we actually do the equivalent of:

  git fetch --prune ../$id.git +refs/*:refs/remotes/$id/*
  git repack -Adl

from each child to pick up any new objects to de-duplicate (our "mother"
repos are not real repos at all, but just big shared-object stores).

I say "equivalent" because those commands can actually be a bit slow. So
we do some hacky tricks like directly moving objects in the filesystem.

In theory the fetch means that it's safe to actually prune in the mother
repo, but in practice there are still races. They don't come up often,
but if you have enough repositories, they do eventually. :)

-Peff

Re: worktrees vs. alternates

[Konstantin Ryabitsev]

On 05/16/18 15:23, Jeff King wrote:
> I implemented "repack -k", which keeps all objects and just rolls them
> into the new pack (along with any currently-loose unreachable objects).
> Aside from corner cases (e.g., where somebody accidentally added a 20GB
> file to an otherwise 100MB-repo and then rolled it back), it usually
> doesn't significantly affect the repository size.

Hmm... I should read manpages more often! :)

So, do you suggest that this is a better approach:

- mother repos: "git repack -adk"
- child repos: "git repack -Adl" (followed by prune)

Currently, we do "-Adl" regardless, but we already track whether a repo
is being used for alternates anywhere (so we don't prune it) and can do
different flags if that improves performance.

Best,
-- 
Konstantin Ryabitsev
Director, IT Infrastructure Security
The Linux Foundation



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v2 1/3] merge: setup `opts` later in `checkout_fast_forward()`

[Martin Ågren]

On 16 May 2018 at 18:41, Stefan Beller  wrote:
> On Wed, May 16, 2018 at 9:30 AM, Martin Ågren  wrote:
>>
>> This patch is best viewed using something like this (note the tab!):
>> --color-moved --anchored="  trees[nr_trees] = parse_tree_indirect"
>
> Heh! Having a "is best viewed" paragraph is the new shiny thing in
> commit messages as 'git log origin/pu --grep "is best viewed"' tells me.

:-)

> Regarding the anchoring, I wonder if we can improve it by ignoring
> whitespaces or just looking for substrings, or by allowing regexes or ...

FWIW, because my first naive attempt failed (for some reason I did not
consider the leading tab part of the "line" so I did not provide it), I
had the same thought. Ignoring leading whitespace seemed easy enough in
the implementation.

Then I started thinking about all the ways in which whitespace can be
ignored. My reaction in the end was to not try and open that can right
there and then. I did not think about regexes.

I guess this boils down to the usage. Copying the line to anchor on from
an editor could run into these kind of whitespace-issues, and shell
escaping. Typing an anchor could become easier with regexes since one
could skip typing common substrings and just anchor on /unique-part/.

Martin

Re: worktrees vs. alternates

[Jeff King]

On Wed, May 16, 2018 at 03:01:13PM -0400, Konstantin Ryabitsev wrote:

> On 05/16/18 14:26, Martin Fick wrote:
> > If you are going to keep the unreferenced objects around 
> > forever, it might be better to keep them around in packed 
> > form?
> 
> I'm undecided about that. On the one hand this does create lots of small
> files and inevitably causes (some) performance degradation. On the other
> hand, I don't want to keep useless objects in the pack, because that
> would also cause performance degradation for people cloning the "mother
> repo." If my assumptions on any of that are incorrect, I'm happy to
> learn more.

I implemented "repack -k", which keeps all objects and just rolls them
into the new pack (along with any currently-loose unreachable objects).
Aside from corner cases (e.g., where somebody accidentally added a 20GB
file to an otherwise 100MB-repo and then rolled it back), it usually
doesn't significantly affect the repository size.

And it generally should not cause performance problems for people
cloning, since Git will create a custom pack for each client with only
the reachable objects.

There _is_ an interesting corner case where a reachable object might be
a delta against an unreachable one, which can cause a clone to have to
break that relationship and find a new delta. At GitHub we have some
custom code that tries to avoid these kind of delta dependencies (not
just to unreachable objects, but to other forks that share object
storage). You can see the patch at:

  https://github.com/peff/git jk/delta-islands

-Peff

Re: worktrees vs. alternates

[Martin Fick]

On Wednesday, May 16, 2018 03:11:47 PM Konstantin Ryabitsev 
wrote:
> On 05/16/18 15:03, Martin Fick wrote:
> >> I'm undecided about that. On the one hand this does
> >> create lots of small files and inevitably causes
> >> (some) performance degradation. On the other hand, I
> >> don't want to keep useless objects in the pack,
> >> because that would also cause performance degradation
> >> for people cloning the "mother repo." If my
> >> assumptions on any of that are incorrect, I'm happy to
> >> learn more.
> > 
> > My suggestion is to use science, not logic or hearsay.
> > :)
> > i.e. test it!
> 
> I think the answer will be "it depends." In many of our
> cases the repos that need those loose objects are rarely
> accessed -- usually because they are forks with older
> data (hence why they need objects that are no longer used
> by the mother repo). Therefore, performance impacts of
> occasionally touching a handful of loose objects will be
> fairly negligible. This is especially true on
> non-spinning media where seek times are low anyway.
> Having slimmer packs for the mother repo would be more
> beneficial in this case.
> 
> On the other hand, if the "child repo" is frequently used,
> then the impact of needing a bunch of loose objects would
> be greater. For the sake of simplicity, I think I'll
> leave things as they are -- it's cheaper to fix this via
> reducing seek times than by applying complicated logic
> trying to optimize on a per-repo basis.

I think a major performance issue with loose objects is not 
just the seek time, but also the fact that they are not 
delta compressed.  This means that sending them over the 
wire will likely have a significant cost before sending it. 
Unlike the seek time, this cost is not mitigated across 
concurrent fetches by the FS (or jgit if you were to use it) 
caching,

-Martin

-- 
The Qualcomm Innovation Center, Inc. is a member of Code 
Aurora Forum, hosted by The Linux Foundation

Re: worktrees vs. alternates

[Jeff King]

On Wed, May 16, 2018 at 10:58:19AM -0400, Konstantin Ryabitsev wrote:

> The parent repo is not keeping track of any other repositories that may
> be using it for alternates, which is why you basically:
> 
> 1. never run auto-gc in the parent repo
> 2. repack it manually using -Ad to keep loose objects that other repos
> may be borrowing (but we don't know if they are)
> 3. never prune the parent repo, because this may delete objects other
> repos are borrowing
> 
> Very infrequently you may consider this extra set of maintenance steps:
> 
> 1. Find every repo mentioning the parent repository in their alternates
> 2. Repack them without the -l switch (which copies all the borrowed
> objects into those repos)
> 3. Once all child repos have been repacked this way, prune the parent
> repo (it's safe now)
> 4. Repack child repos again, this time with the -l flag, to get your
> savings back.

You can also do periodic maintenance like:

  1. Copy each ref in the forked repositories into the parent repository
 (e.g., giving each child that borrows from the parent its own
 hierarchy in refs/remotes//*).

  2. Repack the parent as normal. It will retain any objects referenced
 by the children (because they are now referenced by it).

But note that:

  1. It's not atomic with respect to updates in the child repos (but
 then, neither is the single-repo case!).

  2. It doesn't know about reflogs or the index in the child
 repositories.

This is more or less how we use alternates at GitHub.

> I would heartily love a way to teach git-repack to recognize when an
> object it's borrowing from the parent repo is in danger of being pruned.
> The cheapest way of doing this would probably be to hardlink loose
> objects into its own objects directory and only consider "safe" objects
> those that are part of the parent repository's pack. This should make
> alternates a lot safer, just in case git-prune happens to run by accident.

If you set:

  git config core.repositoryformatversion 1
  git config extensions.preciousObjects true

in the parent, git-prune (repack -d) will refuse to run. That doesn't
solve the problem of how to repack, but it can help prevent accidental
misuse.

-Peff

Re: worktrees vs. alternates

[Konstantin Ryabitsev]

On 05/16/18 15:03, Martin Fick wrote:
>> I'm undecided about that. On the one hand this does create
>> lots of small files and inevitably causes (some)
>> performance degradation. On the other hand, I don't want
>> to keep useless objects in the pack, because that would
>> also cause performance degradation for people cloning the
>> "mother repo." If my assumptions on any of that are
>> incorrect, I'm happy to learn more.
> My suggestion is to use science, not logic or hearsay. :) 
> i.e. test it!

I think the answer will be "it depends." In many of our cases the repos
that need those loose objects are rarely accessed -- usually because
they are forks with older data (hence why they need objects that are no
longer used by the mother repo). Therefore, performance impacts of
occasionally touching a handful of loose objects will be fairly
negligible. This is especially true on non-spinning media where seek
times are low anyway. Having slimmer packs for the mother repo would be
more beneficial in this case.

On the other hand, if the "child repo" is frequently used, then the
impact of needing a bunch of loose objects would be greater. For the
sake of simplicity, I think I'll leave things as they are -- it's
cheaper to fix this via reducing seek times than by applying complicated
logic trying to optimize on a per-repo basis.

Best,
-- 
Konstantin Ryabitsev
Director, IT Infrastructure Security
The Linux Foundation



signature.asc
Description: OpenPGP digital signature

Re: worktrees vs. alternates

[Martin Fick]

On Wednesday, May 16, 2018 03:01:13 PM Konstantin Ryabitsev 
wrote:
> On 05/16/18 14:26, Martin Fick wrote:
> > If you are going to keep the unreferenced objects around
> > forever, it might be better to keep them around in
> > packed
> > form?
> 
> I'm undecided about that. On the one hand this does create
> lots of small files and inevitably causes (some)
> performance degradation. On the other hand, I don't want
> to keep useless objects in the pack, because that would
> also cause performance degradation for people cloning the
> "mother repo." If my assumptions on any of that are
> incorrect, I'm happy to learn more.

My suggestion is to use science, not logic or hearsay. :) 
i.e. test it!

-Martin

-- 
The Qualcomm Innovation Center, Inc. is a member of Code 
Aurora Forum, hosted by The Linux Foundation

Re: worktrees vs. alternates

[Konstantin Ryabitsev]

On 05/16/18 14:26, Martin Fick wrote:
> If you are going to keep the unreferenced objects around 
> forever, it might be better to keep them around in packed 
> form?

I'm undecided about that. On the one hand this does create lots of small
files and inevitably causes (some) performance degradation. On the other
hand, I don't want to keep useless objects in the pack, because that
would also cause performance degradation for people cloning the "mother
repo." If my assumptions on any of that are incorrect, I'm happy to
learn more.

Best,
-- 
Konstantin Ryabitsev
Director, IT Infrastructure Security
The Linux Foundation



signature.asc
Description: OpenPGP digital signature

Re: worktrees vs. alternates

[Martin Fick]

On Wednesday, May 16, 2018 02:12:24 PM Konstantin Ryabitsev 
wrote:
> The loose objects I'm thinking of are those that are
> generated when we do "git repack -Ad" -- this takes all
> unreachable objects and loosens them (see man git-repack
> for more info). Normally, these would be pruned after a
> certain period, but we're deliberately keeping them
> around forever just in case another repo relies on them
> via alternates. I want those repos to "claim" these loose
> objects via hardlinks, such that we can run git-prune on
> the mother repo instead of dragging all the unreachable
> objects on forever just in case.

If you are going to keep the unreferenced objects around 
forever, it might be better to keep them around in packed 
form?  We currently do that because we don't think there is 
a safe way to prune objects yet on a running server (which 
is why I am teaching jgit to be able to recover from a racy 
pruning error),

-Martin

-- 
The Qualcomm Innovation Center, Inc. is a member of Code 
Aurora Forum, hosted by The Linux Foundation

Re: Git log range reverse bug

[Derrick Stolee]

Hi Mendi,

On 5/16/2018 2:19 PM, Mehdi Zeinali wrote:

Git Version: Version: 2.14.2

When reversing a range in git log, it does not start from the expected commit:

$ git show 8e11b4a41ec21e47fb0bf8b76e1edba739f57a9b
commit 8e11b4a41ec21e47fb0bf8b76e1edba739f57a9b
Author: Some Name 
Date:   Mon Nov 3 19:01:53 2014 +
.
.
.

$ git show
Author: Some Other Name 
Date:   Wed May 16 16:49:10 2018 +
.
.
.

$ git log --reverse 8e11b4a41ec21e47fb0bf8b76e1edba739f57a9b..HEAD


This command is asking for the commits reachable from HEAD but NOT 
reachable from 8e11b4a41ec21e47fb0bf8b76e1edba739f57a9b. To see 
8e11b4a41ec21e47fb0bf8b76e1edba739f57a9b in the results, you would need 
to add "--boundary" to the command. That may still not show 
8e11b4a41ec21e47fb0bf8b76e1edba739f57a9b as the first commit, as there 
may be multiple, earlier boundary commits.


Thanks,
-Stolee

Git log range reverse bug

[Mehdi Zeinali]

Git Version: Version: 2.14.2

When reversing a range in git log, it does not start from the expected commit:

$ git show 8e11b4a41ec21e47fb0bf8b76e1edba739f57a9b
commit 8e11b4a41ec21e47fb0bf8b76e1edba739f57a9b
Author: Some Name 
Date:   Mon Nov 3 19:01:53 2014 +
.
.
.

$ git show
Author: Some Other Name 
Date:   Wed May 16 16:49:10 2018 +
.
.
.

$ git log --reverse 8e11b4a41ec21e47fb0bf8b76e1edba739f57a9b..HEAD
commit b4cfdb39f75070f143cdc2c4fbb98f4c6ee94260
Author: Another Name 
Date:   Mon Apr 29 22:16:32 2013 +
Some Commit message

commit 6e6d5cd2a07985ae647fc19e7404ce1edf908949
Author: Yet Another Name 
Date:   Mon Apr 29 22:35:00 2013 +
Some other Commit message

.
.
.

As you can see, the first commit is way off of the provided hash
Mehdi

Re: worktrees vs. alternates

[Konstantin Ryabitsev]

On 05/16/18 14:02, Ævar Arnfjörð Bjarmason wrote:
> 
> On Wed, May 16 2018, Konstantin Ryabitsev wrote:
> 
>> Maybe git-repack can be told to only borrow parent objects if they are
>> in packs. Anything not in packs should be hardlinked into the child
>> repo. That's my wishful think for the day. :)
> 
> Can you elaborate on how this would help?
> 
> We're just going to create loose objects on interactive "git commit",
> presumably you're not adding someone's working copy as the alternate.

The loose objects I'm thinking of are those that are generated when we
do "git repack -Ad" -- this takes all unreachable objects and loosens
them (see man git-repack for more info). Normally, these would be pruned
after a certain period, but we're deliberately keeping them around
forever just in case another repo relies on them via alternates. I want
those repos to "claim" these loose objects via hardlinks, such that we
can run git-prune on the mother repo instead of dragging all the
unreachable objects on forever just in case.

> Otherwise if it's just being pushed to all those pushes are going to be
> in packs, and the packs may contain e.g. pushes for the "pu" branch or
> whatever, which are objects that'll go away.

There are lots of cases where unreachable objects in one repo would
never become unreachable in another -- for example, if the author had
stopped updating it.

Hope this helps.

Best,
-- 
Konstantin Ryabitsev
Director, IT Infrastructure Security
The Linux Foundation



signature.asc
Description: OpenPGP digital signature

Re: worktrees vs. alternates

[Ævar Arnfjörð Bjarmason]

On Wed, May 16 2018, Konstantin Ryabitsev wrote:

> Maybe git-repack can be told to only borrow parent objects if they are
> in packs. Anything not in packs should be hardlinked into the child
> repo. That's my wishful think for the day. :)

Can you elaborate on how this would help?

We're just going to create loose objects on interactive "git commit",
presumably you're not adding someone's working copy as the alternate.

Otherwise if it's just being pushed to all those pushes are going to be
in packs, and the packs may contain e.g. pushes for the "pu" branch or
whatever, which are objects that'll go away.

Re: worktrees vs. alternates

[Ævar Arnfjörð Bjarmason]

On Wed, May 16 2018, Konstantin Ryabitsev wrote:

> On Wed, May 16, 2018 at 05:34:34PM +0200, Ævar Arnfjörð Bjarmason wrote:
>>I may have missed some edge case, but I believe this entire workaround
>>isn't needed if you guarantee that the parent repo doesn't contain any
>>objects that will get un-referenced.
>
> You can't guarantee that, because the parent repo can have its history
> rewritten either via a forced push, or via a rebase. Obviously, this
> won't happen in something like torvalds/linux.git, which is why it's
> pretty safe to alternate off of that repo for us, but codeaurora.org
> repos aren't always strictly-ff (e.g. because they may rebase themselves
> based on what is in upstream AOSP repos) -- so objects in them may
> become unreferenced and pruned away, corrupting any repos using them for
> alternates.

Right, it wouldn't work in the general case. I was thinking of the
use-case for doing this (say with known big monorepos) where you know a
given branch won't be unwound.

Still, there's a tiny variation on this that should work with arbitrary
repos whose master may be rewound, you just setup a refspec to fetch
their upstream HEAD into master-1 without having "+" in the
refspec. Then if they never rewind you keep fetching to master-1
forever.

If they do rewind you fetch that to master-2 and so forth, so you can
follow an upstream rewinding branch while still guaranteeing that no
objects ever disappear from your parent repo. This is still a lot
simpler than the juggling approach you noted, since it's just a tiny
shellscript around the "fetch".

This assumes that:

  1. Whenever this happens the history is still similar enough that the
 parent won't balloon in size like this, or at least it won't be
 worse than not using alternates at all.

 2. You're getting most of the gains of the object sharing by just
grabbing the upstream HEAD branch, i.e. you don't have some repo
with huge and N unrelated histories.

Re: worktrees vs. alternates

[Konstantin Ryabitsev]

On 05/16/18 13:14, Martin Fick wrote:
> On Wednesday, May 16, 2018 10:58:19 AM Konstantin Ryabitsev 
> wrote:
>>
>> 1. Find every repo mentioning the parent repository in
>> their alternates 2. Repack them without the -l switch
>> (which copies all the borrowed objects into those repos)
>> 3. Once all child repos have been repacked this way, prune
>> the parent repo (it's safe now)
> 
> This is probably only true if the repos are in read-only 
> mode?  I suspect this is still racy on a busy server with no 
> downtime.

We don't actually do this anywhere. :) It's a feature I keep hoping to
add one day to grokmirror, but keep putting off because of various
considerations. As you can imagine, if we have 300 forks of linux.git
all using torvalds/linux.git as their alternates, then repacking them
all without -l would balloon our disk usage 300-fold. At this time it's
just cheaper to keep a bunch of loose objects around forever at the cost
of decreased performance.

Maybe git-repack can be told to only borrow parent objects if they are
in packs. Anything not in packs should be hardlinked into the child
repo. That's my wishful think for the day. :)

Best,
-- 
Konstantin Ryabitsev
Director, IT Infrastructure Security
The Linux Foundation



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v3 13/28] t3702: abstract away SHA-1-specific constants

[Stefan Beller]

On Tue, May 15, 2018 at 6:56 PM, brian m. carlson
 wrote:
> Strip out the index lines in the diff before comparing them, as these
> will differ between hash algorithms.  This leads to a smaller, simpler
> change than editing the index line.
>
> Signed-off-by: brian m. carlson 
> ---
>  t/t3702-add-edit.sh | 7 +++
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/t/t3702-add-edit.sh b/t/t3702-add-edit.sh
> index 3cb74ca296..1be5cd5756 100755
> --- a/t/t3702-add-edit.sh
> +++ b/t/t3702-add-edit.sh
> @@ -40,7 +40,6 @@ test_expect_success 'setup' '
>
>  cat > expected-patch << EOF
>  diff --git a/file b/file
> -index b9834b5..9020acb 100644
>  --- a/file
>  +++ b/file
>  @@ -1,11 +1,6 @@
> @@ -80,7 +79,6 @@ EOF
>
>  cat > expected << EOF
>  diff --git a/file b/file
> -index b9834b5..ef6e94c 100644
>  --- a/file
>  +++ b/file
>  @@ -1,10 +1,12 @@
> @@ -100,7 +98,7 @@ EOF
>
>  echo "#!$SHELL_PATH" >fake-editor.sh
>  cat >> fake-editor.sh <<\EOF
> -mv -f "$1" orig-patch &&
> +egrep -v '^index' "$1" >orig-patch &&

This reminds me of the way we test alot of the patch format already.
But there we use standard grep as opposed to egrep.

git grep egrep doesn't show a lot of hits, but all commits
that mention egrep found via 'git log --grep egrep' mention
that there is some sort of portability issue for using egrep
specifically.

Is the ^index a problem for standard grep, i.e. do we need to fix
other places?

$ git grep -- "-v index"
t4061-diff-indent.sh:318:   grep -v index out-diff-files-raw
>out-diff-files-compacted &&
t4061-diff-indent.sh:327:   grep -v index out-diff-files-raw2
>out-diff-files-compacted2 &&
t4061-diff-indent.sh:336:   grep -v index out-diff-files-raw
>out-diff-files &&
t4061-diff-indent.sh:345:   grep -v index out-diff-files-raw2
>out-diff-files &&
t4061-diff-indent.sh:354:   grep -v index out-diff-files-raw3
>out-diff-files-compacted &&
t4061-diff-indent.sh:363:   grep -v index out-diff-files-raw4
>out-diff-files &&

The commit message seems to be the same at most of the patches
in this series, which makes sense, but a mention regarding the choice of
grep would be appreciated!

Thanks,
Stefan

Re: worktrees vs. alternates

[Martin Fick]

On Wednesday, May 16, 2018 10:58:19 AM Konstantin Ryabitsev 
wrote:
> 
> 1. Find every repo mentioning the parent repository in
> their alternates 2. Repack them without the -l switch
> (which copies all the borrowed objects into those repos)
> 3. Once all child repos have been repacked this way, prune
> the parent repo (it's safe now)

This is probably only true if the repos are in read-only 
mode?  I suspect this is still racy on a busy server with no 
downtime.

> 4. Repack child repos again, this time with the -l flag,
> to get your savings back.
 
> I would heartily love a way to teach git-repack to
> recognize when an object it's borrowing from the parent
> repo is in danger of being pruned. The cheapest way of
> doing this would probably be to hardlink loose objects
> into its own objects directory and only consider "safe"
> objects those that are part of the parent repository's
> pack. This should make alternates a lot safer, just in
> case git-prune happens to run by accident.

I think that hard linking is generally a good approach to 
solving many of the "pruning" races left in git.

I have uploaded a "hard linking" proposal to jgit that could 
potentially solve a similar situation that is not alternate 
specific, and only for packfiles, with the intent of 
eventually also doing something similar for loose 
objects.  You can see this here: 

https://git.eclipse.org/r/c/122288/2

I think it would be good to fill in more of these pruning 
gaps!

-Martin

-- 
The Qualcomm Innovation Center, Inc. is a member of Code 
Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v3 07/28] t1007: annotate with SHA1 prerequisite

[Stefan Beller]

Hi brian,

On Tue, May 15, 2018 at 6:56 PM, brian m. carlson
 wrote:
> Since this is a core test that tests basic functionality, annotate the
> assertions that have dependencies on SHA-1 with the appropriate
> prerequisite.
>
> Signed-off-by: brian m. carlson 
> ---
>  t/t1007-hash-object.sh | 16 
>  1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/t/t1007-hash-object.sh b/t/t1007-hash-object.sh
> index 532682f51c..a37753047e 100755
> --- a/t/t1007-hash-object.sh
> +++ b/t/t1007-hash-object.sh
> @@ -9,13 +9,13 @@ echo_without_newline() {
>  }
>
>  test_blob_does_not_exist() {
> -   test_expect_success 'blob does not exist in database' "
> +   test_expect_success SHA1 'blob does not exist in database' "
> test_must_fail git cat-file blob $1
> "
>  }
>
>  test_blob_exists() {
> -   test_expect_success 'blob exists in database' "
> +   test_expect_success SHA1 'blob exists in database' "
> git cat-file blob $1
> "
>  }

For the 2 occurrences above I think the SHA1 requirement is not
needed as the check if a blob exists (and the id is given as $1)
is independent of the hash function, it is just important that
the same hash function is used in the git-cat-file as well as...


> @@ -73,19 +73,19 @@ test_expect_success "Can't use --path with --no-filters" '
>
>  push_repo
>
> -test_expect_success 'hash a file' '
> +test_expect_success SHA1 'hash a file' '
> test $hello_sha1 = $(git hash-object hello)

... here, where we create the blob to test without
writing it into the object database. In a way we test that
the absence of -w works correctly.

Oh, the $hello_sha1 is hard coded, which is why we
think this test is SHA1 dependent.

But that would fit in line with the test_blob[_does_not]_exist
being independent of the hashes?

Stefan