Re: [PATCH 0/7] Restrict the usage of config_from_gitmodules()
On 06/22, Antonio Ospite wrote: > Hi, > > when I tried to reuse and extend 'config_from_gitmodules' in > https://public-inbox.org/git/20180514105823.8378-2-...@ao2.it/ it was > pointed out to me that special care is needed to make sure that this > function does not get abused to bring in arbitrary configuration stored > in the .gitmodules file, as the latter is meant only for submodule > specific configuration. > > So I thought that the function could be made private to better > communicate that. > > This is what this series is about. > > Patch 1 moves 'config_from_gitmodules' to submodule-config.c > > Patches 2 and 3 add helpers to handle special cases and avoid calling > 'config_from_gitmodules' directly, which might set a bad example for > future code. > > Patch 4 makes the symbol private to discourage its use in code not > related to submodules. > > Patches 5 and 6 enable reusing 'config_from_gitmodules' when it's safe > to do so. > > Patches 7 is just a cleanup and I am not even sure it is worth it, so we > might as well just drop it. > > The series can be seen as a continuation of the changes from > https://public-inbox.org/git/20170802194923.88239-1-bmw...@google.com/ > > Even though the helper functions may be less elegant than what was done > back then, they should better protect from misuse of > config_from_gitmodules. > > A further change could be to print warning messages when the backward > compatibility helpers find configuration in .gitmodules that should not > belong there, but I'll leave that to someone else. > > Thanks, >Antonio > > P.S. I added Jeff King to CC as he has done some work related to > .gitmodules recently, and I removed the vcsh poeple on this one. > Thanks for working on this. I think its a good approach and the end result makes it much harder for arbitrary config to sneak back in to the .gitmodules file. And after this series it looks like you should be in a good place to read the .gitmodules file from other places (not just in the worktree). As you've mentioned here I also agree we could do without the last patch but I'll leave that up to you. Other than a couple typos I found I think this series looks good! Thanks again for revisiting this. -- Brandon Williams
Re: [PATCH 7/7] submodule-config: cleanup backward compatibility helpers
On 06/22, Antonio Ospite wrote:
> Use one callback per configuration setting to handle the generic options
> which have to be supported for backward compatibility.
>
> This removes some duplication and some support code at the cost of
> parsing the .gitmodules file twice when calling the fetch command.
>
> Signed-off-by: Antonio Ospite
I'm not sure how I feel about this patch, I'm leaning towards not
needing it but I like the idea of eliminating the duplicate code. One
way you could get rid of having to read the .gitmodules file twice would
be something like the following but I don't really think its needed:
diff --git a/submodule-config.c b/submodule-config.c
index ce204fb53..7cc1864b5 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -681,19 +681,24 @@ void submodule_free(struct repository *r)
submodule_cache_clear(r->submodule_cache);
}
-struct fetch_config {
- int *max_children;
+struct fetch_clone_config {
+ int *fetchjobs;
int *recurse_submodules;
};
-static int gitmodules_fetch_config(const char *var, const char *value, void
*cb)
+static int gitmodules_fetch_clone_config(const char *var, const char *value,
+void *cb)
{
- struct fetch_config *config = cb;
+ struct fetch_clone_config *config = cb;
if (!strcmp(var, "submodule.fetchjobs")) {
- *(config->max_children) = parse_submodule_fetchjobs(var, value);
+ if (config->fetchjobs)
+ *(config->fetchjobs) =
+ parse_submodule_fetchjobs(var, value);
return 0;
} else if (!strcmp(var, "fetch.recursesubmodules")) {
- *(config ->recurse_submodules) =
parse_fetch_recurse_submodules_arg(var, value);
+ if (config->recurse_submodules)
+ *(config->recurse_submodules) =
+ parse_fetch_recurse_submodules_arg(var, value);
return 0;
}
@@ -702,23 +707,20 @@ static int gitmodules_fetch_config(const char *var, const
char *value, void *cb)
void fetch_config_from_gitmodules(int *max_children, int *recurse_submodules)
{
- struct fetch_config config = {
- .max_children = max_children,
- .recurse_submodules = recurse_submodules
+ struct fetch_clone_config config = {
+ .fetchjobs = max_children,
+ .recurse_submodules = recurse_submodules,
};
- config_from_gitmodules(gitmodules_fetch_config, the_repository,
);
-}
-
-static int gitmodules_update_clone_config(const char *var, const char *value,
- void *cb)
-{
- int *max_jobs = cb;
- if (!strcmp(var, "submodule.fetchjobs"))
- *max_jobs = parse_submodule_fetchjobs(var, value);
- return 0;
+ config_from_gitmodules(gitmodules_fetch_clone_config, the_repository,
+ );
}
void update_clone_config_from_gitmodules(int *max_jobs)
{
- config_from_gitmodules(gitmodules_update_clone_config, the_repository,
_jobs);
+ struct fetch_clone_config config = {
+ .fetchjobs = max_jobs,
+ .recurse_submodules = NULL,
+ };
+ config_from_gitmodules(gitmodules_fetch_clone_config, the_repository,
+ );
}
--
Brandon Williams
Re: [PATCH 6/7] submodule-config: reuse config_from_gitmodules in repo_read_gitmodules
On 06/22, Antonio Ospite wrote:
> Reuse config_from_gitmodules in repo_read_gitmodules to remove some
> duplication and also have a single point where the .gitmodules file is
> read.
>
> The change does not introduce any new behavior, the same gitmodules_cb
> config callback is still used which only deals with configuration
> specific to submodules.
>
> The config_from_gitmodules function is moved up in the file —unchanged—
> before its users to avoid a forward declaration.
>
> Signed-off-by: Antonio Ospite
> ---
> submodule-config.c | 50 +++---
> 1 file changed, 21 insertions(+), 29 deletions(-)
>
> diff --git a/submodule-config.c b/submodule-config.c
> index e50c944eb..ce204fb53 100644
> --- a/submodule-config.c
> +++ b/submodule-config.c
> @@ -591,6 +591,23 @@ static void submodule_cache_check_init(struct repository
> *repo)
> submodule_cache_init(repo->submodule_cache);
> }
>
> +/*
> + * Note: This function is private for a reason, the '.gitmodules' file should
> + * not be used as as a mechanism to retrieve arbitrary configuration stored
> in
> + * the repository.
> + *
> + * Runs the provided config function on the '.gitmodules' file found in the
> + * working directory.
> + */
> +static void config_from_gitmodules(config_fn_t fn, struct repository *repo,
> void *data)
> +{
> + if (repo->worktree) {
> + char *file = repo_worktree_path(repo, GITMODULES_FILE);
> + git_config_from_file(fn, file, data);
> + free(file);
> + }
> +}
> +
> static int gitmodules_cb(const char *var, const char *value, void *data)
> {
> struct repository *repo = data;
> @@ -608,19 +625,11 @@ void repo_read_gitmodules(struct repository *repo)
> {
> submodule_cache_check_init(repo);
>
> - if (repo->worktree) {
> - char *gitmodules;
> -
> - if (repo_read_index(repo) < 0)
> - return;
> -
> - gitmodules = repo_worktree_path(repo, GITMODULES_FILE);
> -
> - if (!is_gitmodules_unmerged(repo->index))
> - git_config_from_file(gitmodules_cb, gitmodules, repo);
> + if (repo_read_index(repo) < 0)
> + return;
>
> - free(gitmodules);
> - }
> + if (!is_gitmodules_unmerged(repo->index))
> + config_from_gitmodules(gitmodules_cb, repo, repo);
So the check for the repo's worktree has been pushed into
config_from_gitmodules(). This looks like the right thing to do in
order to get to a world where you'd rather read the gitmodules file from
the index instead of the worktree.
>
> repo->submodule_cache->gitmodules_read = 1;
> }
> @@ -672,23 +681,6 @@ void submodule_free(struct repository *r)
> submodule_cache_clear(r->submodule_cache);
> }
>
> -/*
> - * Note: This function is private for a reason, the '.gitmodules' file should
> - * not be used as as a mechanism to retrieve arbitrary configuration stored
> in
> - * the repository.
> - *
> - * Runs the provided config function on the '.gitmodules' file found in the
> - * working directory.
> - */
> -static void config_from_gitmodules(config_fn_t fn, struct repository *repo,
> void *data)
> -{
> - if (repo->worktree) {
> - char *file = repo_worktree_path(repo, GITMODULES_FILE);
> - git_config_from_file(fn, file, data);
> - free(file);
> - }
> -}
> -
> struct fetch_config {
> int *max_children;
> int *recurse_submodules;
> --
> 2.18.0
>
--
Brandon Williams
Re: [PATCH 3/7] submodule-config: add helper to get 'update-clone' config from .gitmodules
On 06/22, Antonio Ospite wrote: > Add a helper function to make it clearer that retrieving 'update-clone' > configuration from the .gitmodules file is a special case supported > solely for backward compatibility purposes. > > This change removes one direct use of 'config_from_gitmodules' for > options not strictly related to submodules: "submodule.fetchobjs" does s/fetchobjs/fetchjobs -- Brandon Williams
Re: [PATCH 2/7] submodule-config: add helper function to get 'fetch' config from .gitmodules
On 06/22, Antonio Ospite wrote:
> diff --git a/submodule-config.c b/submodule-config.c
> index b431555db..ef292eb7c 100644
> --- a/submodule-config.c
> +++ b/submodule-config.c
> @@ -688,3 +688,31 @@ void config_from_gitmodules(config_fn_t fn, void *data)
> free(file);
> }
> }
> +
> +struct fetch_config {
> + int *max_children;
> + int *recurse_submodules;
> +};
> +
> +static int gitmodules_fetch_config(const char *var, const char *value, void
> *cb)
> +{
> + struct fetch_config *config = cb;
> + if (!strcmp(var, "submodule.fetchjobs")) {
> + *(config->max_children) = parse_submodule_fetchjobs(var, value);
> + return 0;
> + } else if (!strcmp(var, "fetch.recursesubmodules")) {
> + *(config ->recurse_submodules) =
> parse_fetch_recurse_submodules_arg(var, value);
There shouldn't be a space before "->" in this line.
--
Brandon Williams
[PATCH v2] docs: link to gitsubmodules
Add a link to gitsubmodules(7) under the `submodule.active` entry in git-config(1). Signed-off-by: Brandon Williams --- Documentation/config.txt | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Documentation/config.txt b/Documentation/config.txt index ab641bf5a..340eb1f3c 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -3327,12 +3327,13 @@ submodule..ignore:: submodule..active:: Boolean value indicating if the submodule is of interest to git commands. This config option takes precedence over the - submodule.active config option. + submodule.active config option. See linkgit:gitsubmodules[7] for + details. submodule.active:: A repeated field which contains a pathspec used to match against a submodule's path to determine if the submodule is of interest to git - commands. + commands. See linkgit:gitsubmodules[7] for details. submodule.recurse:: Specifies if commands recurse into submodules by default. This -- 2.18.0.rc1.244.gcf134e6275-goog
[PATCH v3 8/8] fetch-pack: implement ref-in-want
Implement ref-in-want on the client side so that when a server supports
the "ref-in-want" feature, a client will send "want-ref" lines for each
reference the client wants to fetch.
Signed-off-by: Brandon Williams
---
fetch-pack.c | 35 +++---
remote.c | 1 +
remote.h | 1 +
t/t5703-upload-pack-ref-in-want.sh | 4 ++--
4 files changed, 36 insertions(+), 5 deletions(-)
diff --git a/fetch-pack.c b/fetch-pack.c
index f1709e816..681b44061 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1102,9 +1102,10 @@ static void add_shallow_requests(struct strbuf *req_buf,
static void add_wants(const struct ref *wants, struct strbuf *req_buf)
{
+ int use_ref_in_want = server_supports_feature("fetch", "ref-in-want",
0);
+
for ( ; wants ; wants = wants->next) {
const struct object_id *remote = >old_oid;
- const char *remote_hex;
struct object *o;
/*
@@ -1122,8 +1123,10 @@ static void add_wants(const struct ref *wants, struct
strbuf *req_buf)
continue;
}
- remote_hex = oid_to_hex(remote);
- packet_buf_write(req_buf, "want %s\n", remote_hex);
+ if (!use_ref_in_want || wants->exact_oid)
+ packet_buf_write(req_buf, "want %s\n",
oid_to_hex(remote));
+ else
+ packet_buf_write(req_buf, "want-ref %s\n", wants->name);
}
}
@@ -1334,6 +1337,29 @@ static void receive_shallow_info(struct fetch_pack_args
*args,
args->deepen = 1;
}
+static void receive_wanted_refs(struct packet_reader *reader, struct ref *refs)
+{
+ process_section_header(reader, "wanted-refs", 0);
+ while (packet_reader_read(reader) == PACKET_READ_NORMAL) {
+ struct object_id oid;
+ const char *end;
+ struct ref *r = NULL;
+
+ if (parse_oid_hex(reader->line, , ) || *end++ != ' ')
+ die("expected wanted-ref, got '%s'", reader->line);
+
+ for (r = refs; r; r = r->next) {
+ if (!strcmp(end, r->name)) {
+ oidcpy(>old_oid, );
+ break;
+ }
+ }
+ }
+
+ if (reader->status != PACKET_READ_DELIM)
+ die("error processing wanted refs: %d", reader->status);
+}
+
enum fetch_state {
FETCH_CHECK_LOCAL = 0,
FETCH_SEND_REQUEST,
@@ -1408,6 +1434,9 @@ static struct ref *do_fetch_pack_v2(struct
fetch_pack_args *args,
if (process_section_header(, "shallow-info", 1))
receive_shallow_info(args, );
+ if (process_section_header(, "wanted-refs", 1))
+ receive_wanted_refs(, ref);
+
/* get the pack */
process_section_header(, "packfile", 0);
if (get_pack(args, fd, pack_lockfile))
diff --git a/remote.c b/remote.c
index abe80c139..2c2376fff 100644
--- a/remote.c
+++ b/remote.c
@@ -1735,6 +1735,7 @@ int get_fetch_map(const struct ref *remote_refs,
if (refspec->exact_sha1) {
ref_map = alloc_ref(name);
get_oid_hex(name, _map->old_oid);
+ ref_map->exact_oid = 1;
} else {
ref_map = get_remote_ref(remote_refs, name);
}
diff --git a/remote.h b/remote.h
index 45ecc6cef..976292152 100644
--- a/remote.h
+++ b/remote.h
@@ -73,6 +73,7 @@ struct ref {
force:1,
forced_update:1,
expect_old_sha1:1,
+ exact_oid:1,
deletion:1;
enum {
diff --git a/t/t5703-upload-pack-ref-in-want.sh
b/t/t5703-upload-pack-ref-in-want.sh
index 979ab6d03..b94a51380 100755
--- a/t/t5703-upload-pack-ref-in-want.sh
+++ b/t/t5703-upload-pack-ref-in-want.sh
@@ -204,7 +204,7 @@ test_expect_success 'server is initially ahead - no ref in
want' '
grep "ERR upload-pack: not our ref" err
'
-test_expect_failure 'server is initially ahead - ref in want' '
+test_expect_success 'server is initially ahead - ref in want' '
git -C "$REPO" config uploadpack.allowRefInWant true &&
rm -rf local &&
cp -r "$LOCAL_PRISTINE" local &&
@@ -228,7 +228,7 @@ test_expect_success 'server is initially behind - no ref in
want' '
test_cmp expected actual
'
-test_expect_failure 'server is initially behind - ref in want' '
+test_expect_success 'server is initially behind - ref in want' '
git -C "$REPO" config uploadpack.allowRefInWant true &&
rm -rf local &&
cp -r "$LOCAL_PRISTINE" local &&
--
2.18.0.rc1.244.gcf134e6275-goog
[PATCH v3 7/8] fetch-pack: put shallow info in output parameter
Expand the transport fetch method signature, by adding an output
parameter, to allow transports to return information about the refs they
have fetched. Then communicate shallow status information through this
mechanism instead of by modifying the input list of refs.
This does require clients to sometimes generate the ref map twice: once
from the list of refs provided by the remote (as is currently done) and
potentially once from the new list of refs that the fetch mechanism
provides.
Signed-off-by: Brandon Williams
---
builtin/clone.c | 4 ++--
builtin/fetch.c | 28
fetch-object.c | 2 +-
fetch-pack.c | 17 +
transport-helper.c | 6 --
transport-internal.h | 9 -
transport.c | 34 --
transport.h | 3 ++-
8 files changed, 78 insertions(+), 25 deletions(-)
diff --git a/builtin/clone.c b/builtin/clone.c
index 99e73dae8..8f86d99c5 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -1155,7 +1155,7 @@ int cmd_clone(int argc, const char **argv, const char
*prefix)
}
if (!is_local && !complete_refs_before_fetch)
- transport_fetch_refs(transport, mapped_refs);
+ transport_fetch_refs(transport, mapped_refs, NULL);
remote_head = find_ref_by_name(refs, "HEAD");
remote_head_points_at =
@@ -1197,7 +1197,7 @@ int cmd_clone(int argc, const char **argv, const char
*prefix)
if (is_local)
clone_local(path, git_dir);
else if (refs && complete_refs_before_fetch)
- transport_fetch_refs(transport, mapped_refs);
+ transport_fetch_refs(transport, mapped_refs, NULL);
update_remote_refs(refs, mapped_refs, remote_head_points_at,
branch_top.buf, reflog_msg.buf, transport,
diff --git a/builtin/fetch.c b/builtin/fetch.c
index b600e1f10..8362a97a2 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -946,11 +946,13 @@ static int quickfetch(struct ref *ref_map)
return check_connected(iterate_ref_map, , );
}
-static int fetch_refs(struct transport *transport, struct ref *ref_map)
+static int fetch_refs(struct transport *transport, struct ref *ref_map,
+ struct ref **updated_remote_refs)
{
int ret = quickfetch(ref_map);
if (ret)
- ret = transport_fetch_refs(transport, ref_map);
+ ret = transport_fetch_refs(transport, ref_map,
+ updated_remote_refs);
if (ret)
transport_unlock_pack(transport);
return ret;
@@ -1106,7 +1108,7 @@ static void backfill_tags(struct transport *transport,
struct ref *ref_map)
transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);
transport_set_option(transport, TRANS_OPT_DEPTH, "0");
transport_set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, NULL);
- if (!fetch_refs(transport, ref_map))
+ if (!fetch_refs(transport, ref_map, NULL))
consume_refs(transport, ref_map);
if (gsecondary) {
@@ -1122,6 +1124,7 @@ static int do_fetch(struct transport *transport,
int autotags = (transport->remote->fetch_tags == 1);
int retcode = 0;
const struct ref *remote_refs;
+ struct ref *updated_remote_refs = NULL;
struct argv_array ref_prefixes = ARGV_ARRAY_INIT;
if (tags == TAGS_DEFAULT) {
@@ -1172,7 +1175,24 @@ static int do_fetch(struct transport *transport,
transport->url);
}
}
- if (fetch_refs(transport, ref_map) || consume_refs(transport, ref_map))
{
+
+ if (fetch_refs(transport, ref_map, _remote_refs)) {
+ free_refs(ref_map);
+ retcode = 1;
+ goto cleanup;
+ }
+ if (updated_remote_refs) {
+ /*
+* Regenerate ref_map using the updated remote refs. This is
+* to account for additional information which may be provided
+* by the transport (e.g. shallow info).
+*/
+ free_refs(ref_map);
+ ref_map = get_ref_map(transport->remote, updated_remote_refs,
rs,
+ tags, );
+ free_refs(updated_remote_refs);
+ }
+ if (consume_refs(transport, ref_map)) {
free_refs(ref_map);
retcode = 1;
goto cleanup;
diff --git a/fetch-object.c b/fetch-object.c
index 853624f81..48fe63dd6 100644
--- a/fetch-object.c
+++ b/fetch-object.c
@@ -19,7 +19,7 @@ static void fetch_refs(const char *remote_name, struct ref
*ref)
transport_set_option(transport, TRANS_OPT_FROM_PROMISOR, "1");
transport_set_option(transport, TRANS_OPT_NO_DEP
[PATCH v3 0/8] ref-in-want
Changes in v3: * Discussion seemed to settle on keeping the simplified version of ref-in-want where the "want-ref" line only accepts full ref names. If we want to we can add patterns at a later time. * Reverted back to v1's behavior where requesting a ref that doesn't exists is a hard error on the server. I went back and forth many times on what the right thing to do here is and decided that a hard error works much cleaner for the time being. * Some typos. Brandon Williams (8): test-pkt-line: add unpack-sideband subcommand upload-pack: implement ref-in-want upload-pack: test negotiation with changing repository fetch: refactor the population of peer ref OIDs fetch: refactor fetch_refs into two functions fetch: refactor to make function args narrower fetch-pack: put shallow info in output parameter fetch-pack: implement ref-in-want Documentation/config.txt| 7 + Documentation/technical/protocol-v2.txt | 28 ++- builtin/clone.c | 4 +- builtin/fetch.c | 131 - fetch-object.c | 2 +- fetch-pack.c| 52 +++-- remote.c| 1 + remote.h| 1 + t/helper/test-pkt-line.c| 33 t/lib-httpd.sh | 1 + t/lib-httpd/apache.conf | 8 + t/lib-httpd/one-time-sed.sh | 16 ++ t/t5703-upload-pack-ref-in-want.sh | 245 transport-helper.c | 6 +- transport-internal.h| 9 +- transport.c | 34 +++- transport.h | 3 +- upload-pack.c | 64 +++ 18 files changed, 568 insertions(+), 77 deletions(-) create mode 100644 t/lib-httpd/one-time-sed.sh create mode 100755 t/t5703-upload-pack-ref-in-want.sh -- 2.18.0.rc1.244.gcf134e6275-goog
[PATCH v3 6/8] fetch: refactor to make function args narrower
Refactor find_non_local_tags and get_ref_map to only take the
information they need instead of the entire transport struct. Besides
improving code clarity, this also improves their flexibility, allowing
for a different set of refs to be used instead of relying on the ones
stored in the transport struct.
Signed-off-by: Brandon Williams
---
builtin/fetch.c | 52 -
1 file changed, 26 insertions(+), 26 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index ee8b87c78..b600e1f10 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -254,9 +254,9 @@ static int will_fetch(struct ref **head, const unsigned
char *sha1)
return 0;
}
-static void find_non_local_tags(struct transport *transport,
- struct ref **head,
- struct ref ***tail)
+static void find_non_local_tags(const struct ref *refs,
+ struct ref **head,
+ struct ref ***tail)
{
struct string_list existing_refs = STRING_LIST_INIT_DUP;
struct string_list remote_refs = STRING_LIST_INIT_NODUP;
@@ -264,7 +264,7 @@ static void find_non_local_tags(struct transport *transport,
struct string_list_item *item = NULL;
for_each_ref(add_existing, _refs);
- for (ref = transport_get_remote_refs(transport, NULL); ref; ref =
ref->next) {
+ for (ref = refs; ref; ref = ref->next) {
if (!starts_with(ref->name, "refs/tags/"))
continue;
@@ -338,7 +338,8 @@ static void find_non_local_tags(struct transport *transport,
string_list_clear(_refs, 0);
}
-static struct ref *get_ref_map(struct transport *transport,
+static struct ref *get_ref_map(struct remote *remote,
+ const struct ref *remote_refs,
struct refspec *rs,
int tags, int *autotags)
{
@@ -346,27 +347,11 @@ static struct ref *get_ref_map(struct transport
*transport,
struct ref *rm;
struct ref *ref_map = NULL;
struct ref **tail = _map;
- struct argv_array ref_prefixes = ARGV_ARRAY_INIT;
/* opportunistically-updated references: */
struct ref *orefs = NULL, **oref_tail =
struct string_list existing_refs = STRING_LIST_INIT_DUP;
- const struct ref *remote_refs;
-
- 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);
-
- argv_array_clear(_prefixes);
if (rs->nr) {
struct refspec *fetch_refspec;
@@ -403,7 +388,7 @@ static struct ref *get_ref_map(struct transport *transport,
if (refmap.nr)
fetch_refspec =
else
- fetch_refspec = >remote->fetch;
+ fetch_refspec = >fetch;
for (i = 0; i < fetch_refspec->nr; i++)
get_fetch_map(ref_map, _refspec->items[i],
_tail, 1);
@@ -411,7 +396,6 @@ static struct ref *get_ref_map(struct transport *transport,
die("--refmap option is only meaningful with command-line
refspec(s).");
} else {
/* Use the defaults */
- struct remote *remote = transport->remote;
struct branch *branch = branch_get(NULL);
int has_merge = branch_has_merge_config(branch);
if (remote &&
@@ -450,7 +434,7 @@ static struct ref *get_ref_map(struct transport *transport,
/* also fetch all tags */
get_fetch_map(remote_refs, tag_refspec, , 0);
else if (tags == TAGS_DEFAULT && *autotags)
- find_non_local_tags(transport, _map, );
+ find_non_local_tags(remote_refs, _map, );
/* Now append any refs to be updated opportunistically: */
*tail = orefs;
@@ -1137,6 +1121,8 @@ static int do_fetch(struct transport *transport,
struct ref *ref_map;
int autotags = (transport->remote->fetch_tags == 1);
int retcode = 0;
+ const struct ref *remote_refs;
+ struct argv_array ref_prefixes = ARGV_ARRAY_INIT;
if (tags == TAGS_DEFAULT) {
if (transport->remote->fetch_tags == 2)
@@ -1152,7 +1138,21 @@ static int do_fetch(struct transport *transport,
goto cleanup;
}
- ref_map = get_ref_map(transport, rs, tags, );
+ if (rs->nr)
+ r
[PATCH v3 2/8] upload-pack: implement ref-in-want
Currently, while performing packfile negotiation, clients are only
allowed to specify their desired objects using object ids. This causes
a vulnerability to failure when an object turns non-existent during
negotiation, which may happen if, for example, the desired repository is
provided by multiple Git servers in a load-balancing arrangement and
there exists replication delay.
In order to eliminate this vulnerability, implement the ref-in-want
feature for the 'fetch' command in protocol version 2. This feature
enables the 'fetch' command to support requests in the form of ref names
through a new "want-ref " parameter. At the conclusion of
negotiation, the server will send a list of all of the wanted references
(as provided by "want-ref" lines) in addition to the generated packfile.
Signed-off-by: Brandon Williams
---
Documentation/config.txt| 7 ++
Documentation/technical/protocol-v2.txt | 28 -
t/t5703-upload-pack-ref-in-want.sh | 153
upload-pack.c | 64 ++
4 files changed, 251 insertions(+), 1 deletion(-)
create mode 100755 t/t5703-upload-pack-ref-in-want.sh
diff --git a/Documentation/config.txt b/Documentation/config.txt
index ab641bf5a..fb1dd7428 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -3479,6 +3479,13 @@ Note that this configuration variable is ignored if it
is seen in the
repository-level config (this is a safety measure against fetching from
untrusted repositories).
+uploadpack.allowRefInWant::
+ If this option is set, `upload-pack` will support the `ref-in-want`
+ feature of the protocol version 2 `fetch` command. This feature
+ is intended for the benefit of load-balanced servers which may
+ not have the same view of what OIDs their refs point to due to
+ replication delay.
+
url..insteadOf::
Any URL that starts with this value will be rewritten to
start, instead, with . In cases where some site serves a
diff --git a/Documentation/technical/protocol-v2.txt
b/Documentation/technical/protocol-v2.txt
index 49bda76d2..9dee75d45 100644
--- a/Documentation/technical/protocol-v2.txt
+++ b/Documentation/technical/protocol-v2.txt
@@ -299,12 +299,21 @@ included in the client's request:
for use with partial clone and partial fetch operations. See
`rev-list` for possible "filter-spec" values.
+If the 'ref-in-want' feature is advertised, the following argument can
+be included in the client's request as well as the potential addition of
+the 'wanted-refs' section in the server's response as explained below.
+
+want-ref
+ Indicates to the server that the client wants to retrieve a
+ particular ref, where is the full name of a ref on the
+ server.
+
The response of `fetch` is broken into a number of sections separated by
delimiter packets (0001), with each section beginning with its section
header.
output = *section
-section = (acknowledgments | shallow-info | packfile)
+section = (acknowledgments | shallow-info | wanted-refs | packfile)
(flush-pkt | delim-pkt)
acknowledgments = PKT-LINE("acknowledgments" LF)
@@ -319,6 +328,10 @@ header.
shallow = "shallow" SP obj-id
unshallow = "unshallow" SP obj-id
+wanted-refs = PKT-LINE("wanted-refs" LF)
+ *PKT-LINE(wanted-ref LF)
+wanted-ref = obj-id SP refname
+
packfile = PKT-LINE("packfile" LF)
*PKT-LINE(%x01-03 *%x00-ff)
@@ -379,6 +392,19 @@ header.
* This section is only included if a packfile section is also
included in the response.
+wanted-refs section
+ * This section is only included if the client has requested a
+ ref using a 'want-ref' line and if a packfile section is also
+ included in the response.
+
+ * Always begins with the section header "wanted-refs"
+
+ * The server will send a ref listing (" ") for
+ each reference requested using 'want-ref' lines.
+
+ * The server MUST NOT send any refs which were not requested
+ using 'want-ref' lines.
+
packfile section
* This section is only included if the client has sent 'want'
lines in its request and either requested that no more
diff --git a/t/t5703-upload-pack-ref-in-want.sh
b/t/t5703-upload-pack-ref-in-want.sh
new file mode 100755
index 0..0ef182970
--- /dev/null
+++ b/t/t5703-upload-pack-ref-in-want.sh
@@ -0,0 +1,153 @@
+#!/bin/sh
+
+test_description='upload-pack ref-in-want'
+
+. ./test-lib.sh
+
+get_actual_refs() {
+ sed -n '/wanted-refs/,/0001/p' actual_refs
+}
+
+get_actual_commits() {
+ sed -n '/packfile/,//p' o.pack &&
+ git index-pack o.pack &&
+ git verify-pack -v o.idx | grep commit | cut -c-40 | sort
>actual_commits
+}
[PATCH v3 4/8] fetch: refactor the population of peer ref OIDs
Populate peer ref OIDs in get_ref_map instead of do_fetch. Besides
tightening scopes of variables in the code, this also prepares for
get_ref_map being able to be called multiple times within do_fetch.
Signed-off-by: Brandon Williams
---
builtin/fetch.c | 36 ++--
1 file changed, 18 insertions(+), 18 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index ea5b9669a..545635448 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -351,6 +351,7 @@ static struct ref *get_ref_map(struct transport *transport,
/* opportunistically-updated references: */
struct ref *orefs = NULL, **oref_tail =
+ struct string_list existing_refs = STRING_LIST_INIT_DUP;
const struct ref *remote_refs;
if (rs->nr)
@@ -458,7 +459,23 @@ static struct ref *get_ref_map(struct transport *transport,
tail = >next;
}
- return ref_remove_duplicates(ref_map);
+ ref_map = ref_remove_duplicates(ref_map);
+
+ for_each_ref(add_existing, _refs);
+ for (rm = ref_map; rm; rm = rm->next) {
+ if (rm->peer_ref) {
+ struct string_list_item *peer_item =
+ string_list_lookup(_refs,
+ rm->peer_ref->name);
+ if (peer_item) {
+ struct object_id *old_oid = peer_item->util;
+ oidcpy(>peer_ref->old_oid, old_oid);
+ }
+ }
+ }
+ string_list_clear(_refs, 1);
+
+ return ref_map;
}
#define STORE_REF_ERROR_OTHER 1
@@ -1110,14 +1127,10 @@ static void backfill_tags(struct transport *transport,
struct ref *ref_map)
static int do_fetch(struct transport *transport,
struct refspec *rs)
{
- struct string_list existing_refs = STRING_LIST_INIT_DUP;
struct ref *ref_map;
- struct ref *rm;
int autotags = (transport->remote->fetch_tags == 1);
int retcode = 0;
- for_each_ref(add_existing, _refs);
-
if (tags == TAGS_DEFAULT) {
if (transport->remote->fetch_tags == 2)
tags = TAGS_SET;
@@ -1136,18 +1149,6 @@ static int do_fetch(struct transport *transport,
if (!update_head_ok)
check_not_current_branch(ref_map);
- for (rm = ref_map; rm; rm = rm->next) {
- if (rm->peer_ref) {
- struct string_list_item *peer_item =
- string_list_lookup(_refs,
- rm->peer_ref->name);
- if (peer_item) {
- struct object_id *old_oid = peer_item->util;
- oidcpy(>peer_ref->old_oid, old_oid);
- }
- }
- }
-
if (tags == TAGS_DEFAULT && autotags)
transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, "1");
if (prune) {
@@ -1183,7 +1184,6 @@ static int do_fetch(struct transport *transport,
}
cleanup:
- string_list_clear(_refs, 1);
return retcode;
}
--
2.18.0.rc1.244.gcf134e6275-goog
[PATCH v3 1/8] test-pkt-line: add unpack-sideband subcommand
Add an 'unpack-sideband' subcommand to the test-pkt-line helper to
enable unpacking packet line data sent multiplexed using a sideband.
Signed-off-by: Brandon Williams
---
t/helper/test-pkt-line.c | 33 +
1 file changed, 33 insertions(+)
diff --git a/t/helper/test-pkt-line.c b/t/helper/test-pkt-line.c
index 0f19e53c7..30775f986 100644
--- a/t/helper/test-pkt-line.c
+++ b/t/helper/test-pkt-line.c
@@ -1,3 +1,4 @@
+#include "cache.h"
#include "pkt-line.h"
static void pack_line(const char *line)
@@ -48,6 +49,36 @@ static void unpack(void)
}
}
+static void unpack_sideband(void)
+{
+ struct packet_reader reader;
+ packet_reader_init(, 0, NULL, 0,
+ PACKET_READ_GENTLE_ON_EOF |
+ PACKET_READ_CHOMP_NEWLINE);
+
+ while (packet_reader_read() != PACKET_READ_EOF) {
+ int band;
+ int fd;
+
+ switch (reader.status) {
+ case PACKET_READ_EOF:
+ break;
+ case PACKET_READ_NORMAL:
+ band = reader.line[0] & 0xff;
+ if (band < 1 || band > 2)
+ die("unexpected side band %d", band);
+ fd = band;
+
+ write_or_die(fd, reader.line + 1, reader.pktlen - 1);
+ break;
+ case PACKET_READ_FLUSH:
+ return;
+ case PACKET_READ_DELIM:
+ break;
+ }
+ }
+}
+
int cmd_main(int argc, const char **argv)
{
if (argc < 2)
@@ -57,6 +88,8 @@ int cmd_main(int argc, const char **argv)
pack(argc - 2, argv + 2);
else if (!strcmp(argv[1], "unpack"))
unpack();
+ else if (!strcmp(argv[1], "unpack-sideband"))
+ unpack_sideband();
else
die("invalid argument '%s'", argv[1]);
--
2.18.0.rc1.244.gcf134e6275-goog
[PATCH v3 5/8] fetch: refactor fetch_refs into two functions
Refactor the fetch_refs function into a function that does the fetching
of refs and another function that stores them.
Signed-off-by: Brandon Williams
---
builtin/fetch.c | 19 +--
1 file changed, 13 insertions(+), 6 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 545635448..ee8b87c78 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -967,10 +967,16 @@ static int fetch_refs(struct transport *transport, struct
ref *ref_map)
int ret = quickfetch(ref_map);
if (ret)
ret = transport_fetch_refs(transport, ref_map);
- if (!ret)
- ret |= store_updated_refs(transport->url,
- transport->remote->name,
- ref_map);
+ if (ret)
+ transport_unlock_pack(transport);
+ return ret;
+}
+
+static int consume_refs(struct transport *transport, struct ref *ref_map)
+{
+ int ret = store_updated_refs(transport->url,
+transport->remote->name,
+ref_map);
transport_unlock_pack(transport);
return ret;
}
@@ -1116,7 +1122,8 @@ static void backfill_tags(struct transport *transport,
struct ref *ref_map)
transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);
transport_set_option(transport, TRANS_OPT_DEPTH, "0");
transport_set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, NULL);
- fetch_refs(transport, ref_map);
+ if (!fetch_refs(transport, ref_map))
+ consume_refs(transport, ref_map);
if (gsecondary) {
transport_disconnect(gsecondary);
@@ -1165,7 +1172,7 @@ static int do_fetch(struct transport *transport,
transport->url);
}
}
- if (fetch_refs(transport, ref_map)) {
+ if (fetch_refs(transport, ref_map) || consume_refs(transport, ref_map))
{
free_refs(ref_map);
retcode = 1;
goto cleanup;
--
2.18.0.rc1.244.gcf134e6275-goog
[PATCH v3 3/8] upload-pack: test negotiation with changing repository
Add tests to check the behavior of fetching from a repository which
changes between rounds of negotiation (for example, when different
servers in a load-balancing agreement participate in the same stateless
RPC negotiation). This forms a baseline of comparison to the ref-in-want
functionality (which will be introduced to the client in subsequent
commits), and ensures that subsequent commits do not change existing
behavior.
As part of this effort, a mechanism to substitute strings in a single
HTTP response is added.
Signed-off-by: Brandon Williams
---
t/lib-httpd.sh | 1 +
t/lib-httpd/apache.conf| 8 +++
t/lib-httpd/one-time-sed.sh| 16 ++
t/t5703-upload-pack-ref-in-want.sh | 92 ++
4 files changed, 117 insertions(+)
create mode 100644 t/lib-httpd/one-time-sed.sh
diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index 435a37465..84f8efdd4 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -132,6 +132,7 @@ prepare_httpd() {
cp "$TEST_PATH"/passwd "$HTTPD_ROOT_PATH"
install_script broken-smart-http.sh
install_script error.sh
+ install_script one-time-sed.sh
ln -s "$LIB_HTTPD_MODULE_PATH" "$HTTPD_ROOT_PATH/modules"
diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index 724d9ae46..fe68d37bb 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -111,9 +111,14 @@ Alias /auth/dumb/ www/auth/dumb/
SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
SetEnv GIT_HTTP_EXPORT_ALL
+
+ SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
+ SetEnv GIT_HTTP_EXPORT_ALL
+
ScriptAliasMatch /smart_*[^/]*/(.*) ${GIT_EXEC_PATH}/git-http-backend/$1
ScriptAlias /broken_smart/ broken-smart-http.sh/
ScriptAlias /error/ error.sh/
+ScriptAliasMatch /one_time_sed/(.*) one-time-sed.sh/$1
Options FollowSymlinks
@@ -123,6 +128,9 @@ ScriptAlias /error/ error.sh/
Options ExecCGI
+
+ Options ExecCGI
+
Options ExecCGI
diff --git a/t/lib-httpd/one-time-sed.sh b/t/lib-httpd/one-time-sed.sh
new file mode 100644
index 0..a9c4aa5f4
--- /dev/null
+++ b/t/lib-httpd/one-time-sed.sh
@@ -0,0 +1,16 @@
+#!/bin/sh
+
+if [ -e one-time-sed ]; then
+ "$GIT_EXEC_PATH/git-http-backend" >out
+
+ sed "$(cat one-time-sed)" out_modified
+
+ if diff out out_modified >/dev/null; then
+ cat out
+ else
+ cat out_modified
+ rm one-time-sed
+ fi
+else
+ "$GIT_EXEC_PATH/git-http-backend"
+fi
diff --git a/t/t5703-upload-pack-ref-in-want.sh
b/t/t5703-upload-pack-ref-in-want.sh
index 0ef182970..979ab6d03 100755
--- a/t/t5703-upload-pack-ref-in-want.sh
+++ b/t/t5703-upload-pack-ref-in-want.sh
@@ -150,4 +150,96 @@ test_expect_success 'want-ref with ref we already have
commit for' '
check_output
'
+. "$TEST_DIRECTORY"/lib-httpd.sh
+start_httpd
+
+REPO="$HTTPD_DOCUMENT_ROOT_PATH/repo"
+LOCAL_PRISTINE="$(pwd)/local_pristine"
+
+test_expect_success 'setup repos for change-while-negotiating test' '
+ (
+ git init "$REPO" &&
+ cd "$REPO" &&
+ >.git/git-daemon-export-ok &&
+ test_commit m1 &&
+ git tag -d m1 &&
+
+ # Local repo with many commits (so that negotiation will take
+ # more than 1 request/response pair)
+ git clone "http://127.0.0.1:$LIB_HTTPD_PORT/smart/repo;
"$LOCAL_PRISTINE" &&
+ cd "$LOCAL_PRISTINE" &&
+ git checkout -b side &&
+ for i in $(seq 1 33); do test_commit s$i; done &&
+
+ # Add novel commits to upstream
+ git checkout master &&
+ cd "$REPO" &&
+ test_commit m2 &&
+ test_commit m3 &&
+ git tag -d m2 m3
+ ) &&
+ git -C "$LOCAL_PRISTINE" remote set-url origin
"http://127.0.0.1:$LIB_HTTPD_PORT/one_time_sed/repo; &&
+ git -C "$LOCAL_PRISTINE" config protocol.version 2
+'
+
+inconsistency() {
+ # Simulate that the server initially reports $2 as the ref
+ # corresponding to $1, and after that, $1 as the ref corresponding to
+ # $1. This corresponds to the real-life situation where the server's
+ # repository appears to change during negotiation, for example, when
+ # different servers in a load-balancing arrangement serve (stateless)
+ # RPCs during a single negotiation.
+ printf "s/%s/%s/" \
+ $(git -C "$REPO" rev-parse $1 | tr -d "\n") \
+ $(git -C "$REPO" rev-parse $2 | tr -d "\n") \
+
Re: [PATCH 0/8] ref-in-want
On 06/15, Jonathan Tan wrote: > > Supporting patterns would mean that we would possibly be able to > eliminate the ls-refs step, thus saving at least a RTT. (Originally I > thought that supporting patterns would also allow us to tolerate refs > being removed during the fetch process, but I see that this is already > handled by the server ignoring "want-ref " wherein doesn't > exist on the server.) What's your opinion on this? Should we keep it how it is in v2 of the series where the server ignores refs it doesn't know about or revert to what v1 of the series did and have it be a hard error? I've gone back and forth on what I think we should do so I'd like to hear at least one more opinion :) -- Brandon Williams
Re: [PATCH v2 2/8] upload-pack: implement ref-in-want
On 06/19, Junio C Hamano wrote: > Brandon Williams writes: > > > I also think that we should keep this first implementation of > > ref-in-want simple and *not* include patterns, even if that's what we > > may want someday down the road. Adding a new capability in the future > > for support of such patterns would be relatively simple and easy. > > I am all for many-small-steps over a single-giant-step approach. > > > The > > reason why I don't think we should add pattern support just yet is due > > to a request for "want-ref refs/tags/*" or a like request resulting in a > > larger than expected packfile every time "fetch --tags" is run. The > > issue being that in a fetch request "refs/tags/*" is too broad of a > > request and could be requesting 100s of tags when all we really wanted > > was to get the one or two new tags which are present on the remote > > (because we already have all the other tags present locally). > > I do not quite get this. Why does it have to result in a large > packfile? Doesn't the requester of refs/tags/* still show what it > has via "have" exchange? Sorry Jonathan Tan said it much clearer here: https://public-inbox.org/git/20180615190458.147775-1-jonathanta...@google.com/ -- Brandon Williams
Re: [PATCH v2 2/8] upload-pack: implement ref-in-want
On 06/15, Junio C Hamano wrote: > The story would be different if your request were > > git fetch refs/heads/*:refs/remotes/origin/* > > in which case, you are not even saying "I want this and that ref"; > you are saying "all refs in refs/heads/* whoever ends up serving me > happens to have". You may initially contact one of my friends and > learn that there are 'master' and 'bo' branches (and probably > others), and after conversation end up talking with me who is stale > and lack 'bo'. In such a case, I agree that it is not sensible for > me to fail the request as a whole and instead serve you whatever > branches I happen to have. I may lack 'bo' branch due to mirroring > lag, but I may also have 'extra' branch that others no longer have > due to mirroring lag of deletion of that branch! > > But then I think your "git fetch refs/heads/*:refs/remotes/origin/*" > should not fail not just because I do not have 'bo', but you also > should grab other old branches I have, which you didn't hear about > when you made the initial contact with my friend in the mirror pool. > > So, given that, would it make sense for 'want-ref ' request to > name "a particular ref" as the above document says? I have a > feeling that it should allow a pattern to be matched at the server > side (and it is not an error if the pattern did not match anything), > in addition to asking for a particular ref (in which case, lack of > that ref should be a hard failure, at least for the mirror that ends > up serving the packfile and the final "here are the refs your > request ended up fetching, with their values"). After some more in-office discussion about this I think I should revert back to making it a hard failure when a client requests a ref which doesn't exist on the server. This makes things more consistent with what happens today if I request a non-existent ref (Although that error is purely on the client). Its no worse than we have today and even with this solution not solving the issues of new/deleted refs (which are rare operations wrt updates) we still can get the benefit of not failing due to a ref updating. This is also very valuable for the servers which have to to ACL checks on individual ref names. I also think that we should keep this first implementation of ref-in-want simple and *not* include patterns, even if that's what we may want someday down the road. Adding a new capability in the future for support of such patterns would be relatively simple and easy. The reason why I don't think we should add pattern support just yet is due to a request for "want-ref refs/tags/*" or a like request resulting in a larger than expected packfile every time "fetch --tags" is run. The issue being that in a fetch request "refs/tags/*" is too broad of a request and could be requesting 100s of tags when all we really wanted was to get the one or two new tags which are present on the remote (because we already have all the other tags present locally). So I think the best way is to limit these patterns to the ls-refs request where we can then discover the few tags we're missing and then request just those tags explicitly, keeping the resulting packfile smaller. Thoughts? -- Brandon Williams
Re: [PATCH v2 7/8] fetch-pack: put shallow info in output parameter
On 06/14, Jonathan Tan wrote:
> > @@ -1122,6 +1124,7 @@ static int do_fetch(struct transport *transport,
> > int autotags = (transport->remote->fetch_tags == 1);
> > int retcode = 0;
> > const struct ref *remote_refs;
> > + struct ref *new_remote_refs = NULL;
>
> Above, you use the name "updated_remote_refs" - it's probably better to
> standardize on one. I think "updated" is better.
Good catch I'll update the variable name.
>
> (The transport calling it "fetched_refs" is fine, because that's what
> they are from the perspective of the transport. From the perspective of
> fetch-pack, it is indeed a new or updated set of remote refs.)
>
> > - if (fetch_refs(transport, ref_map) || consume_refs(transport, ref_map))
> > {
> > +
> > + if (fetch_refs(transport, ref_map, _remote_refs)) {
> > + free_refs(ref_map);
> > + retcode = 1;
> > + goto cleanup;
> > + }
> > + if (new_remote_refs) {
> > + free_refs(ref_map);
> > + ref_map = get_ref_map(transport->remote, new_remote_refs, rs,
> > + tags, );
> > + free_refs(new_remote_refs);
> > + }
> > + if (consume_refs(transport, ref_map)) {
> > free_refs(ref_map);
> > retcode = 1;
> > goto cleanup;
>
> Here, if we got updated remote refs, we need to regenerate ref_map,
> since it is the source of truth.
>
> Maybe add a comment in the "if (new_remote_refs)" block explaining this
> - something like: Regenerate ref_map using the updated remote refs,
> because the transport would place shallow (and other) information
> there.
That's probably a good idea to give future readers more context into why
this is happening.
>
> > - for (i = 0; i < nr_sought; i++)
> > + for (r = refs; r; r = r->next, i++)
> > if (status[i])
> > - sought[i]->status = REF_STATUS_REJECT_SHALLOW;
> > + r->status = REF_STATUS_REJECT_SHALLOW;
>
> You use i here without initializing it to 0. t5703 also fails with this
> patch - probably related to this, but I didn't check.
Oh yeah that's definitely a bug, thanks for catching that.
>
> If you initialize i here, I don't think you need to initialize it to 0
> at the top of this function.
--
Brandon Williams
Re: [PATCH 0/8] ref-in-want
On 06/15, Jonathan Tan wrote: > (replying to the original since my e-mail is about design) > > > This version of ref-in-want is a bit more restrictive than what Jonathan > > originally proposed (only full ref names are allowed instead of globs > > and OIDs), but it is meant to accomplish the same goal (solve the issues > > of refs changing during negotiation). > > One question remains: are we planning to expand this feature (e.g. to > support patterns ending in *, or to support any pattern that can appear > on the LHS of a refspec), and if yes, are we OK with having 2 or more > versions of the service in the wild, each having different pattern > support? > > Supporting patterns would mean that we would possibly be able to > eliminate the ls-refs step, thus saving at least a RTT. (Originally I > thought that supporting patterns would also allow us to tolerate refs > being removed during the fetch process, but I see that this is already > handled by the server ignoring "want-ref " wherein doesn't > exist on the server.) > > However, after some in-office discussion, I see that eliminating the > ls-refs step means that we lose some optimizations that can only be done > when we see that we already have a sought remote ref. For example, in a > repo like this: > > A > | > O > | > O B C > |/ / > O O > |/ > O > > in which we have rarely-updated branches that we still want to fetch > (e.g. an annotated tag when we fetch refs/tags/* or a Gerrit > refs/changes/* branch), having the ref advertisement first means that we > can omit them from our "want" or "want-ref" list. But not having them > means that we send "want-ref refs/tags/*" to the server, and during > negotiation inform the server of our master branch (A), and since the > server knows of a common ancestor of all our wants (A, B, C), it will > terminate the negotiation and send the objects specific to branches B > and C even though it didn't need to. > > So maybe we still need to keep the ls-refs step around, and thus, this > design of only accepting exact refs is perhaps good enough for now. I think that taking a smaller step first it probably better. This is something that we've done in the past with the shallow features and later capabilities were added to add different ways to request shallow fetches. That being said, if we find that this feature doesn't work as-is and needs the extra complexity of patterns from the start then they should be added. But it doesn't seem like there's a concrete reason at the moment. -- Brandon Williams
Re: [PATCH v2 2/2] fetch: send "refs/tags/" prefix upon CLI refspecs
On 06/18, Jonathan Tan wrote:
>
> This would cause different behavior. For example, if I run "git fetch
> refs/heads/master refs/tags/foo", I would expect tag following to work
> even if the tag's name does not start with refs/tags/foo.
I understand that some people *may* want tag following, but really its
confusing from an end user's standpoint. You can fetch a particular ref
and end up with a bunch of tags that you didn't want. Aside from that
my biggest issue is with performance. The ref filtering was added to
cut down on the number of refs sent from the server, now we're basically
adding them all back no matter what (unless a user goes and flips the
default to not fetch tags).
I think we're probably going to need some people other than us to
comment on how this should be handled.
>
> > > @@ -359,7 +359,7 @@ static struct ref *get_ref_map(struct transport
> > > *transport,
> > > refspec_ref_prefixes(>remote->fetch,
> > > _prefixes);
> > >
> > > if (ref_prefixes.argc &&
> > > - (tags == TAGS_SET || (tags == TAGS_DEFAULT && !rs->nr))) {
> > > + (tags == TAGS_SET || tags == TAGS_DEFAULT)) {
> >
> > Oh, I see. This always asks for refs/tags/ whereas before we only
> > asked for them if there were *no* refspec given. Maybe we can
> > change this to
> >
> > refspec_any_item_contains("refs/tags/")
> >
> > instead of always asking for the tags?
> > (that function would need to be written; I guess for a short term bugfix
> > this is good enough)
>
> Same answer as above.
>
> > How is the tag following documented (i.e. when is the user least
> > surprised that we do not tag-follow all and unconditionally)?
>
> It's documented under the --no-tags option in the man page for git
> fetch. I'm not sure what you mean by the user being surprised.
--
Brandon Williams
Re: [PATCH 15/15] cache.h: flip NO_THE_INDEX_COMPATIBILITY_MACROS switch
ec.h"
> diff --git a/builtin/pull.c b/builtin/pull.c
> index 1f2ecf3a88..1c3657dee2 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -5,6 +5,7 @@
> *
> * Fetch one or more remote refs and merge it/them into the current HEAD.
> */
> +#define USE_THE_INDEX_COMPATIBILITY_MACROS
> #include "cache.h"
> #include "config.h"
> #include "builtin.h"
> diff --git a/builtin/read-tree.c b/builtin/read-tree.c
> index ebc43eb805..ae6ca3a8c5 100644
> --- a/builtin/read-tree.c
> +++ b/builtin/read-tree.c
> @@ -4,6 +4,7 @@
> * Copyright (C) Linus Torvalds, 2005
> */
>
> +#define USE_THE_INDEX_COMPATIBILITY_MACROS
> #include "cache.h"
> #include "config.h"
> #include "lockfile.h"
> diff --git a/builtin/reset.c b/builtin/reset.c
> index a862c70fab..dd60eec9d6 100644
> --- a/builtin/reset.c
> +++ b/builtin/reset.c
> @@ -7,6 +7,7 @@
> *
> * Copyright (c) 2005, 2006 Linus Torvalds and Junio C Hamano
> */
> +#define USE_THE_INDEX_COMPATIBILITY_MACROS
> #include "builtin.h"
> #include "config.h"
> #include "lockfile.h"
> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
> index 4f49e96bfd..b5f8d6a83d 100644
> --- a/builtin/rev-parse.c
> +++ b/builtin/rev-parse.c
> @@ -3,6 +3,7 @@
> *
> * Copyright (C) Linus Torvalds, 2005
> */
> +#define USE_THE_INDEX_COMPATIBILITY_MACROS
> #include "cache.h"
> #include "config.h"
> #include "commit.h"
> diff --git a/builtin/rm.c b/builtin/rm.c
> index 65b448ef8e..e5b77e429d 100644
> --- a/builtin/rm.c
> +++ b/builtin/rm.c
> @@ -3,6 +3,7 @@
> *
> * Copyright (C) Linus Torvalds 2006
> */
> +#define USE_THE_INDEX_COMPATIBILITY_MACROS
> #include "builtin.h"
> #include "config.h"
> #include "lockfile.h"
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index bd250ca216..8abe15144b 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -1,3 +1,4 @@
> +#define USE_THE_INDEX_COMPATIBILITY_MACROS
> #include "builtin.h"
> #include "repository.h"
> #include "cache.h"
> diff --git a/builtin/update-index.c b/builtin/update-index.c
> index a8709a26ec..36e837248f 100644
> --- a/builtin/update-index.c
> +++ b/builtin/update-index.c
> @@ -3,6 +3,7 @@
> *
> * Copyright (C) Linus Torvalds, 2005
> */
> +#define USE_THE_INDEX_COMPATIBILITY_MACROS
> #include "cache.h"
> #include "config.h"
> #include "lockfile.h"
> diff --git a/cache.h b/cache.h
> index 89a107a7f7..b3a6d14a36 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -339,7 +339,7 @@ extern void remove_name_hash(struct index_state *istate,
> struct cache_entry *ce)
> extern void free_name_hash(struct index_state *istate);
>
>
> -#ifndef NO_THE_INDEX_COMPATIBILITY_MACROS
> +#ifdef USE_THE_INDEX_COMPATIBILITY_MACROS
> #define active_cache (the_index.cache)
> #define active_nr (the_index.cache_nr)
> #define active_alloc (the_index.cache_alloc)
> diff --git a/convert.c b/convert.c
> index 64d0d30e08..0895dc5994 100644
> --- a/convert.c
> +++ b/convert.c
> @@ -1,4 +1,3 @@
> -#define NO_THE_INDEX_COMPATIBILITY_MACROS
> #include "cache.h"
> #include "config.h"
> #include "attr.h"
> diff --git a/dir.c b/dir.c
> index ccf8b4975e..473c47eb2f 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -7,7 +7,6 @@
> * Copyright (C) Linus Torvalds, 2005-2006
> *Junio Hamano, 2005-2006
> */
> -#define NO_THE_INDEX_COMPATIBILITY_MACROS
> #include "cache.h"
> #include "config.h"
> #include "dir.h"
> diff --git a/name-hash.c b/name-hash.c
> index 163849831c..12eaa62980 100644
> --- a/name-hash.c
> +++ b/name-hash.c
> @@ -5,7 +5,6 @@
> *
> * Copyright (C) 2008 Linus Torvalds
> */
> -#define NO_THE_INDEX_COMPATIBILITY_MACROS
> #include "cache.h"
>
> struct dir_entry {
> diff --git a/pathspec.c b/pathspec.c
> index 27cd606786..6997707477 100644
> --- a/pathspec.c
> +++ b/pathspec.c
> @@ -1,4 +1,3 @@
> -#define NO_THE_INDEX_COMPATIBILITY_MACROS
> #include "cache.h"
> #include "config.h"
> #include "dir.h"
> diff --git a/read-cache.c b/read-cache.c
> index 372588260e..2a84ad0797 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -3,7 +3,6 @@
> *
> * Copyright (C) Linus Torvalds, 2005
> */
> -#define NO_THE_INDEX_COMPATIBILITY_MACROS
> #include "cache.h"
> #include "config.h"
> #include "tempfile.h"
> diff --git a/submodule.c b/submodule.c
> index 939d6870ec..c6ae29379d 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1,4 +1,3 @@
> -#define NO_THE_INDEX_COMPATIBILITY_MACROS
>
> #include "cache.h"
> #include "repository.h"
> diff --git a/t/helper/test-dump-untracked-cache.c
> b/t/helper/test-dump-untracked-cache.c
> index bd92fb305a..56a5ce8abb 100644
> --- a/t/helper/test-dump-untracked-cache.c
> +++ b/t/helper/test-dump-untracked-cache.c
> @@ -1,3 +1,4 @@
> +#define USE_THE_INDEX_COMPATIBILITY_MACROS
> #include "cache.h"
> #include "dir.h"
>
> diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
> index 7116ddfb94..a7ff69e9f3 100644
> --- a/t/helper/test-tool.h
> +++ b/t/helper/test-tool.h
> @@ -1,6 +1,8 @@
> #ifndef __TEST_TOOL_H__
> #define __TEST_TOOL_H__
>
> +#define USE_THE_INDEX_COMPATIBILITY_MACROS
> +
> int cmd__chmtime(int argc, const char **argv);
> int cmd__config(int argc, const char **argv);
> int cmd__ctype(int argc, const char **argv);
> diff --git a/tree.c b/tree.c
> index 244eb5e665..b5ed7bc22b 100644
> --- a/tree.c
> +++ b/tree.c
> @@ -1,4 +1,3 @@
> -#define NO_THE_INDEX_COMPATIBILITY_MACROS
> #include "cache.h"
> #include "cache-tree.h"
> #include "tree.h"
> diff --git a/unpack-trees.c b/unpack-trees.c
> index 3a85a02a77..fd09e812a2 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -1,4 +1,3 @@
> -#define NO_THE_INDEX_COMPATIBILITY_MACROS
> #include "cache.h"
> #include "argv-array.h"
> #include "repository.h"
> --
> 2.18.0.rc0.333.g22e6ee6cdf
>
--
Brandon Williams
Re: [PATCH 00/15] Kill the_index part 1, expose it
On 06/17, Duy Nguyen wrote: > On Sun, Jun 17, 2018 at 9:02 AM Elijah Newren wrote: > > > > On Fri, Jun 15, 2018 at 10:41 PM, Nguyễn Thái Ngọc Duy > > wrote: > > > This is the beginning of the end of the_index. The problem with > > > the_index is it lets library code anywhere access it freely. This is > > > not good because from high level you may not realize that the_index is > > > being used while you don't want to touch index at all, or you want to > > > use a different index instead. > > > > > > This is a long series, 86 patches [1], so I'm going to split and > > > submit it in 15-20 patches at a time. The first two parts are trivial > > > though and could be safely fast tracked if needed. > > > > You post this small little patch about unpack-trees.c, mentioning you > > don't know if it's even correct, and bait me into reviewing it and > > then spring on me that it's actually nearly 100 patches that need > > review... Very sneaky. ;-) > > To be fair, it's all Brandon's fault. If he didn't kick the_index out > of dir.c, it would not conflict with one of my out-of-tree patches in > unpack-trees.c, catch my attention and make me go down this rabbit > hole :D Haha well this is something I've wanted to do for over a year now, glad you've decided to run with it :) I guess I've gotten pretty good at getting people to go down rabbit holes. First Stefan with the object store refactoring and now you with the index stuff. All because I wanted git to be more object oriented and have less global state ;) -- Brandon Williams
Re: Adding nested repository with slash adds files instead of gitlink
On 06/18, Duy Nguyen wrote: > On Mon, Jun 18, 2018 at 1:23 PM Heiko Voigt wrote: > > > > Hi, > > > > I just discovered that when you have a slash at the end of a nested > > repository, the files contained in the repository get added instead of > > the gitlink. > > > > I found this when I was adding a submodule and wanted to commit a small > > change before that. You get the slash by using tab autocompletion. > > > > Here is a recipe to reproduce: > > > > mkdir test > > cd test; git init > > touch a; git add a; git commit -m a > > mkdir ../test.git; (cd ../test.git; git init --bare) > > git remote add origin ../test.git > > git push origin master > > git submodule add ../test.git submodule > > git reset > > git add submodule/ > > > > Now instead of just submodule gitlink there is an entry for submodule/a > > in the index. > > > > I just thought I put this out there. Will have a look if I find the time > > to cook up a proper testcase and investigate. > > This sounds like the submodule specific code in pathspec.c, which has > been replaced with something else in bw/pathspec-sans-the-index. If > you have time, try a version without those changes (e.g. v2.13 or > before) to see if it's a possible culprit. I just tested this with v2.13 and saw the same issue. I don't actually think this ever worked in the way you want it to Heiko. Maybe git add needs to be taught to be more intelligent when trying to add a submodule which doesn't exist in the index. -- Brandon Williams
Re: [PATCH v2 0/8] ref-in-want
On 06/15, Junio C Hamano wrote: > Brandon Williams writes: > > > Changes in v2: > > * issuing a want-ref line to a ref which doesn't exist is just ignored. > > * fixed some typos > > Do I lock some (logical) prerequisite changes? On 2.18-rc2 they > apply cleanly but the series fails its own test. No this is an error I made in this version of the series which another reviewer pointed out, I have a local v3 which addresses this (by removing the test since it isn't necessary anymore). Sorry for the mistake :) > > t5703-upload-pack-ref-in-want.sh > expecting success: > test-pkt-line pack >in <<-EOF && > command=fetch > 0001 > no-progress > want-ref refs/heads/non-existent > done > > EOF > > test_must_fail git serve --stateless-rpc 2>out grep "unknown ref" out > > test_must_fail: command succeeded: git serve --stateless-rpc > not ok 3 - invalid want-ref line -- Brandon Williams
Re: [PATCH 10/35] commit: add repository argument to lookup_commit
On 06/14, Stefan Beller wrote:
> On Thu, Jun 14, 2018 at 9:22 AM Duy Nguyen wrote:
> >
> > On Wed, May 30, 2018 at 2:51 AM Stefan Beller wrote:
> > > diff --git a/shallow.c b/shallow.c
> > > index 9bb07a56dca..60fe1fe1e58 100644
> > > --- a/shallow.c
> > > +++ b/shallow.c
> > > @@ -31,7 +31,7 @@ int register_shallow(struct repository *r, const struct
> > > object_id *oid)
> > > {
> > > struct commit_graft *graft =
> > > xmalloc(sizeof(struct commit_graft));
> > > - struct commit *commit = lookup_commit(oid);
> > > + struct commit *commit = lookup_commit(the_repository, oid);
> >
> > This looks wrong. register_shallow() has struct repository argument
> > 'r' and it should be used here instead.
>
> Right.
>
> > If this is a mechanical conversion, I will also be happy that the
> > switch from the_repo to r is done in a separate patch.
>
> This part of the code is not touched later in this series,
> so I'll fix it if a reroll is needed.
Yeah maybe at some point when lookup_commit can understand arbitrary
repositories we can change this from the_repository to r. This patch is
part of that mechanical change and has to be the_repository till
lookup_commit has been fully converted.
>
> > FYI I noticed this because I'm in a quest to kill the_index by passing
> > 'struct index_state *' throughout library code, and sometimes I pass
> > 'struct repository *' instead when I see that code uses more things
> > that just the index. And I have started to replace the_repository in
> > some places with a function argument.
> >
> > If some of my patches come first while you have not finished
> > repository conversion (very likely), you and I will have to pay
> > attention to this more often.
--
Brandon Williams
Re: [PATCH v2 8/8] fetch-pack: implement ref-in-want
On 06/14, Stefan Beller wrote:
> On Wed, Jun 13, 2018 at 2:39 PM Brandon Williams wrote:
>
> > +static void receive_wanted_refs(struct packet_reader *reader, struct ref
> > *refs)
> > +{
> ...
> > +
> > + for (r = refs; r; r = r->next) {
> > + if (!strcmp(end, r->name)) {
> > + oidcpy(>old_oid, );
> > + break;
> > + }
> > + }
>
> The server is documented as MUST NOT send additional refs,
> which is fine here, as we'd have no way of storing them anyway.
> Do we want to issue a warning, though?
>
> if (!r) /* never break'd */
> warning ("server send unexpected line '%s'", reader.line);
Depends, does this warning help out the end user or do you think it
would confuse users to see this and still have their fetch succeed?
>
>
>
> > diff --git a/remote.c b/remote.c
> > index abe80c139..c9d452ac0 100644
> > --- a/remote.c
> > +++ b/remote.c
> > @@ -1735,6 +1735,7 @@ int get_fetch_map(const struct ref *remote_refs,
> > if (refspec->exact_sha1) {
> > ref_map = alloc_ref(name);
> > get_oid_hex(name, _map->old_oid);
> > + ref_map->exact_sha1 = 1;
> > } else {
> > ref_map = get_remote_ref(remote_refs, name);
> > }
> > diff --git a/remote.h b/remote.h
> > index 45ecc6cef..e5338e368 100644
> > --- a/remote.h
> > +++ b/remote.h
> > @@ -73,6 +73,7 @@ struct ref {
> > force:1,
> > forced_update:1,
> > expect_old_sha1:1,
> > + exact_sha1:1,
>
> Can we rename that to exact_oid ?
I'll fix this.
--
Brandon Williams
Re: [PATCH v2 1/8] test-pkt-line: add unpack-sideband subcommand
On 06/14, Stefan Beller wrote:
> On Wed, Jun 13, 2018 at 2:39 PM Brandon Williams wrote:
> >
> > Add an 'unpack-sideband' subcommand to the test-pkt-line helper to
> > enable unpacking packet line data sent multiplexed using a sideband.
> >
> > Signed-off-by: Brandon Williams
> > ---
> > t/helper/test-pkt-line.c | 37 +
> > 1 file changed, 37 insertions(+)
> >
> > diff --git a/t/helper/test-pkt-line.c b/t/helper/test-pkt-line.c
> > index 0f19e53c7..2a551 100644
> > --- a/t/helper/test-pkt-line.c
> > +++ b/t/helper/test-pkt-line.c
> > @@ -1,3 +1,4 @@
> > +#include "cache.h"
> > #include "pkt-line.h"
> >
> > static void pack_line(const char *line)
> > @@ -48,6 +49,40 @@ static void unpack(void)
> > }
> > }
> >
> > +static void unpack_sideband(void)
> > +{
> > + struct packet_reader reader;
> > + packet_reader_init(, 0, NULL, 0,
> > + PACKET_READ_GENTLE_ON_EOF |
> > + PACKET_READ_CHOMP_NEWLINE);
> > +
> > + while (packet_reader_read() != PACKET_READ_EOF) {
> > + int band;
> > + int fd;
> > +
> > + switch (reader.status) {
> > + case PACKET_READ_EOF:
> > + break;
> > + case PACKET_READ_NORMAL:
> > + band = reader.line[0] & 0xff;
> > + if (band == 1)
> > + fd = 1;
> > + else
> > + fd = 2;
> > +
> > + write_or_die(fd, reader.line+1, reader.pktlen-1);
>
> white space around + and - ?
Will fix.
>
> > +
> > + if (band == 3)
> > + die("sind-band error");
>
> s/sind/side/ ?
Thanks for catching this.
>
> What values for band are possible?
> e.g. band==4 would also just write to fd=1;
> but I suspect we don't want that, yet.
>
> So maybe
>
> band = reader.line[0] & 0xff;
> if (band < 1 || band > 2)
> die("unexpected side band %d", band)
> fd = band;
>
> instead?
Yeah that's must cleaner logic.
--
Brandon Williams
Re: [PATCH v2 2/8] upload-pack: implement ref-in-want
On 06/14, Stefan Beller wrote: > Hi Brandon, > On Wed, Jun 13, 2018 at 2:39 PM Brandon Williams wrote: > > negotiation, which may happen if, for example, the desired repository is > > provided by multiple Git servers in a load-balancing arrangement. > > ... and the repository is not replicated evenly to all servers, yet. I'll update the commit msg to also include this. > > > In order to eliminate this vulnerability, implement the ref-in-want > > feature for the 'fetch' command in protocol version 2. This feature > > enables the 'fetch' command to support requests in the form of ref names > > through a new "want-ref " parameter. At the conclusion of > > negotiation, the server will send a list of all of the wanted references > > (as provided by "want-ref" lines) in addition to the generated packfile. > > This paragraph makes it sound as if it can be combined technically, > i.e. > > client: > want 01234... > want-ref master > > .. usual back and forth + pack.. > > server: > > wanted-ref: master 2345.. > > What happens if the client "wants" a sha1 that is advertised, > but happens to be the same as a wanted-ref? This would be fine, same as sending a want line with the same sha1 lots of times. Though there would still be a wanted-ref section from the server for the wanted-ref. > > > Signed-off-by: Brandon Williams > > --- > > Documentation/config.txt| 7 ++ > > Documentation/technical/protocol-v2.txt | 29 - > > t/t5703-upload-pack-ref-in-want.sh | 153 > > upload-pack.c | 64 ++ > > 4 files changed, 252 insertions(+), 1 deletion(-) > > create mode 100755 t/t5703-upload-pack-ref-in-want.sh > > > > diff --git a/Documentation/config.txt b/Documentation/config.txt > > index ab641bf5a..fb1dd7428 100644 > > --- a/Documentation/config.txt > > +++ b/Documentation/config.txt > > @@ -3479,6 +3479,13 @@ Note that this configuration variable is ignored if > > it is seen in the > > repository-level config (this is a safety measure against fetching from > > untrusted repositories). > > > > +uploadpack.allowRefInWant:: > > + If this option is set, `upload-pack` will support the `ref-in-want` > > + feature of the protocol version 2 `fetch` command. This feature > > + is intended for the benefit of load-balanced servers which may > > + not have the same view of what OIDs their refs point to due to > > + replication delay. > > Instead of saying who benefits, can we also say what the feature is about? > Didn't someone mention on the first round of this series, that technically > ref-in-want also provides smaller net work load as refs usually are shorter > than oids (specifically as oids will grow in the hash transisition plan > later)? > Is that worth mentioning? Well I basically just took this from what a previous reviewer thought it should say. I think what you have listed here isn't really a big benefit of using ref-in-want, its the issue with load-balanced servers that this is trying to solve. > > When using this feature is a ref advertisement still needed? Maybe in the future no, but as of right now the code is structured to still request a ref advertisement. > > > + > > url..insteadOf:: > > Any URL that starts with this value will be rewritten to > > start, instead, with . In cases where some site serves a > > diff --git a/Documentation/technical/protocol-v2.txt > > b/Documentation/technical/protocol-v2.txt > > index 49bda76d2..6020632b4 100644 > > --- a/Documentation/technical/protocol-v2.txt > > +++ b/Documentation/technical/protocol-v2.txt > > @@ -299,12 +299,22 @@ included in the client's request: > > for use with partial clone and partial fetch operations. See > > `rev-list` for possible "filter-spec" values. > > > > +If the 'ref-in-want' feature is advertised, the following argument can > > +be included in the client's request as well as the potential addition of > > +the 'wanted-refs' section in the server's response as explained below. > > + > > +want-ref > > + Indicates to the server that the client wants to retrieve a > > + particular ref, where is the full name of a ref on the > > + server. A server should ignore any "want-ref " lines where > > +doesn't exist on the server. > > Are patterns allowed?, e.g. I might want refs/tags/* at all times. Nope, "Where is the full name of a ref". We can maybe al
Re: [PATCH v2 00/31] object-store: lookup_commit
On 06/13, Stefan Beller wrote: > * removed mentions of cooci patches > * added forward declaration of commit buffer slabs. > * dropped 3 patches that add the repository to lookup_unkonwn_object, > parse_commit and parse_commit_gently, but were not converting those > functions. We'll convert these in the next series, as this series is > growing big already. > * This series can be found as branch 'object-store-lookup-commit' on github, > it applies on top of nd/commit-util-to-slab merged with > sb/object-store-grafts > > v1, https://public-inbox.org/git/20180530004810.30076-1-sbel...@google.com/ > > This applies on the merge of nd/commit-util-to-slab and > sb/object-store-grafts, > and is available at http://github.com/stefanbeller/ as branch > object-store-lookup-commit > as the merge has some merge conflicts as well as syntactical conflicts > (upload-pack.c > and fetch-pack.c introduce new calls of functions that would want to take a > repository struct > in the object-store-grafts series) > > As layed out in > https://public-inbox.org/git/20180517225154.9200-1-sbel...@google.com/ > this is getting close to finishing the set of object store series though the > last > unfinished part of this RFC hints at new work on the plate: > * To give this series a nice polish, we'd want to convert parse_commit, too. > But that requires the conversion of the new commit graph. Maybe we need > to split this series into 2. > * Once this is in good shape we can talk about converting parts of the > revision > walking code, > * which then can be used by the submodule code as the end goal for the > object store series. I've taken a look at the series and it looks good. I'm glad we're getting closer to this set of series being completed. Thanks for pushing this through :) -- Brandon Williams
Re: [PATCH v2 8/8] negotiator/default: use better style in comments
On 06/06, Jonathan Tan wrote:
> Signed-off-by: Jonathan Tan
> ---
> negotiator/default.c | 14 ++
> 1 file changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/negotiator/default.c b/negotiator/default.c
> index b8f45cf78..a9e52c943 100644
> --- a/negotiator/default.c
> +++ b/negotiator/default.c
> @@ -46,11 +46,10 @@ static int clear_marks(const char *refname, const struct
> object_id *oid,
> }
>
> /*
> - This function marks a rev and its ancestors as common.
> - In some cases, it is desirable to mark only the ancestors (for example
> - when only the server does not yet know that they are common).
> -*/
> -
> + * This function marks a rev and its ancestors as common.
> + * In some cases, it is desirable to mark only the ancestors (for example
> + * when only the server does not yet know that they are common).
> + */
> static void mark_common(struct data *data, struct commit *commit,
> int ancestors_only, int dont_parse)
> {
> @@ -80,9 +79,8 @@ static void mark_common(struct data *data, struct commit
> *commit,
> }
>
> /*
> - Get the next rev to send, ignoring the common.
> -*/
> -
> + * Get the next rev to send, ignoring the common.
> + */
> static const struct object_id *get_rev(struct data *data)
> {
> struct commit *commit = NULL;
> --
> 2.17.0.768.g1526ddbba1.dirty
>
Don't have this be a separate patch, squash it into the previous patch.
--
Brandon Williams
Re: [PATCH v2 5/8] fetch-pack: make negotiation-related vars local
gt;
> -static int send_fetch_request(int fd_out, const struct fetch_pack_args *args,
> +static int send_fetch_request(struct data *data, int fd_out,
> + const struct fetch_pack_args *args,
> const struct ref *wants, struct oidset *common,
> int *haves_to_send, int *in_vain)
> {
> @@ -1238,7 +1249,7 @@ static int send_fetch_request(int fd_out, const struct
> fetch_pack_args *args,
> add_common(_buf, common);
>
> /* Add initial haves */
> - ret = add_haves(_buf, haves_to_send, in_vain);
> + ret = add_haves(data, _buf, haves_to_send, in_vain);
> }
>
> /* Send request */
> @@ -1275,7 +1286,8 @@ static int process_section_header(struct packet_reader
> *reader,
> return ret;
> }
>
> -static int process_acks(struct packet_reader *reader, struct oidset *common)
> +static int process_acks(struct data *data, struct packet_reader *reader,
> + struct oidset *common)
> {
> /* received */
> int received_ready = 0;
> @@ -1294,7 +1306,7 @@ static int process_acks(struct packet_reader *reader,
> struct oidset *common)
> struct commit *commit;
> oidset_insert(common, );
> commit = lookup_commit();
> - mark_common(commit, 0, 1);
> + mark_common(data, commit, 0, 1);
> }
> continue;
> }
> @@ -1372,6 +1384,7 @@ static struct ref *do_fetch_pack_v2(struct
> fetch_pack_args *args,
> struct packet_reader reader;
> int in_vain = 0;
> int haves_to_send = INITIAL_FLUSH;
> + struct data data = { { compare_commits_by_commit_date } };
> packet_reader_init(, fd[0], NULL, 0,
> PACKET_READ_CHOMP_NEWLINE);
>
> @@ -1392,18 +1405,19 @@ static struct ref *do_fetch_pack_v2(struct
> fetch_pack_args *args,
> marked = 1;
>
> /* Filter 'ref' by 'sought' and those that aren't local
> */
> - mark_complete_and_common_ref(args, );
> + mark_complete_and_common_ref(, args, );
> filter_refs(args, , sought, nr_sought);
> if (everything_local(args, ))
> state = FETCH_DONE;
> else
> state = FETCH_SEND_REQUEST;
>
> - for_each_ref(rev_list_insert_ref_oid, NULL);
> - for_each_cached_alternate(insert_one_alternate_object);
> + for_each_ref(rev_list_insert_ref_oid, );
> + for_each_cached_alternate(,
> + insert_one_alternate_object);
> break;
> case FETCH_SEND_REQUEST:
> - if (send_fetch_request(fd[1], args, ref, ,
> + if (send_fetch_request(, fd[1], args, ref, ,
> _to_send, _vain))
> state = FETCH_GET_PACK;
> else
> @@ -1411,7 +1425,7 @@ static struct ref *do_fetch_pack_v2(struct
> fetch_pack_args *args,
> break;
> case FETCH_PROCESS_ACKS:
> /* Process ACKs/NAKs */
> - switch (process_acks(, )) {
> + switch (process_acks(, , )) {
> case 2:
> state = FETCH_GET_PACK;
> break;
> --
> 2.17.0.768.g1526ddbba1.dirty
>
--
Brandon Williams
Re: [PATCH v2 3/8] fetch-pack: directly end negotiation if ACK ready
On 06/14, Brandon Williams wrote:
> On 06/06, Jonathan Tan wrote:
> > When "ACK %s ready" is received, find_common() clears rev_list in an
> > attempt to stop further "have" lines from being sent [1]. It is much
> > more readable to explicitly break from the loop instead, so do this.
> >
> > This means that the memory in priority queue will be reclaimed only upon
> > program exit, similar to the cases in which "ACK %s ready" is not
>
> This seems fine for now though ideally we would remove the global
> priority queue and have it live on the stack somewhere in the call stack
> and it could be cleared unconditionally before returning.
Wait looks like a later commit does just this, maybe stick in a comment
saying a later patch is cleaning this up.
>
> > received. (A related problem occurs when do_fetch_pack() is invoked a
> > second time in the same process with a possibly non-empty priority
> > queue, but this will be solved in a subsequent patch in this patch set.)
> >
> > [1] The rationale is further described in the originating commit
> > f2cba9299b ("fetch-pack: Finish negotation if remote replies "ACK %s
> > ready"", 2011-03-14).
> >
> > Signed-off-by: Jonathan Tan
> > ---
> > fetch-pack.c | 7 +++
> > 1 file changed, 3 insertions(+), 4 deletions(-)
> >
> > diff --git a/fetch-pack.c b/fetch-pack.c
> > index 2812499a5..09f5c83c4 100644
> > --- a/fetch-pack.c
> > +++ b/fetch-pack.c
> > @@ -517,10 +517,8 @@ static int find_common(struct fetch_pack_args *args,
> > mark_common(commit, 0, 1);
> > retval = 0;
> > got_continue = 1;
> > - if (ack == ACK_ready) {
> > - clear_prio_queue(_list);
> > + if (ack == ACK_ready)
> > got_ready = 1;
> > - }
> > break;
> > }
> > }
> > @@ -530,6 +528,8 @@ static int find_common(struct fetch_pack_args *args,
> > print_verbose(args, _("giving up"));
> > break; /* give up */
> > }
> > + if (got_ready)
> > + break;
> > }
> > }
> > done:
> > @@ -1300,7 +1300,6 @@ static int process_acks(struct packet_reader *reader,
> > struct oidset *common)
> > }
> >
> > if (!strcmp(reader->line, "ready")) {
> > - clear_prio_queue(_list);
> > received_ready = 1;
> > continue;
> > }
> > --
> > 2.17.0.768.g1526ddbba1.dirty
> >
>
> --
> Brandon Williams
--
Brandon Williams
Re: [PATCH v2 4/8] fetch-pack: use ref adv. to prune "have" sent
On 06/06, Jonathan Tan wrote: > In negotiation using protocol v2, fetch-pack sometimes does not make > full use of the information obtained in the ref advertisement: > specifically, that if the server advertises a commit that the client > also has, the client never needs to inform the server that it has the > commit's parents, since it can just tell the server that it has the > advertised commit and it knows that the server can and will infer the > rest. > > This is because, in do_fetch_pack_v2(), rev_list_insert_ref_oid() is > invoked before everything_local(). This means that if we have a commit > that is both our ref and their ref, it would be enqueued by > rev_list_insert_ref_oid() as SEEN, and since it is thus already SEEN, > everything_local() would not enqueue it. Thanks for fixing this slight issue with v2. Though maybe we need to update the commit message here because a previous patch in this version of the series broke up everything_local() into various parts so that it is no longer responsible for enqueueing commits? > > If everything_local() were invoked first, as it is in do_fetch_pack() > for protocol v0, then everything_local() would enqueue it with > COMMON_REF | SEEN. The addition of COMMON_REF ensures that its parents > are not sent as "have" lines. > > Change the order in do_fetch_pack_v2() to be consistent with > do_fetch_pack(), and to avoid sending unnecessary "have" lines. > > Signed-off-by: Jonathan Tan > --- -- Brandon Williams
Re: [PATCH v2 3/8] fetch-pack: directly end negotiation if ACK ready
On 06/06, Jonathan Tan wrote:
> When "ACK %s ready" is received, find_common() clears rev_list in an
> attempt to stop further "have" lines from being sent [1]. It is much
> more readable to explicitly break from the loop instead, so do this.
>
> This means that the memory in priority queue will be reclaimed only upon
> program exit, similar to the cases in which "ACK %s ready" is not
This seems fine for now though ideally we would remove the global
priority queue and have it live on the stack somewhere in the call stack
and it could be cleared unconditionally before returning.
> received. (A related problem occurs when do_fetch_pack() is invoked a
> second time in the same process with a possibly non-empty priority
> queue, but this will be solved in a subsequent patch in this patch set.)
>
> [1] The rationale is further described in the originating commit
> f2cba9299b ("fetch-pack: Finish negotation if remote replies "ACK %s
> ready"", 2011-03-14).
>
> Signed-off-by: Jonathan Tan
> ---
> fetch-pack.c | 7 +++
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/fetch-pack.c b/fetch-pack.c
> index 2812499a5..09f5c83c4 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -517,10 +517,8 @@ static int find_common(struct fetch_pack_args *args,
> mark_common(commit, 0, 1);
> retval = 0;
> got_continue = 1;
> - if (ack == ACK_ready) {
> - clear_prio_queue(_list);
> + if (ack == ACK_ready)
> got_ready = 1;
> - }
> break;
> }
> }
> @@ -530,6 +528,8 @@ static int find_common(struct fetch_pack_args *args,
> print_verbose(args, _("giving up"));
> break; /* give up */
> }
> + if (got_ready)
> + break;
> }
> }
> done:
> @@ -1300,7 +1300,6 @@ static int process_acks(struct packet_reader *reader,
> struct oidset *common)
> }
>
> if (!strcmp(reader->line, "ready")) {
> - clear_prio_queue(_list);
> received_ready = 1;
> continue;
> }
> --
> 2.17.0.768.g1526ddbba1.dirty
>
--
Brandon Williams
Re: [PATCH v2 1/8] fetch-pack: split up everything_local()
On 06/06, Jonathan Tan wrote:
> The function everything_local(), despite its name, also (1) marks
> commits as COMPLETE and COMMON_REF and (2) invokes filter_refs() as
> important side effects. Extract (1) into its own function
> (mark_complete_and_common_ref()) and remove
> (2).
>
> The restoring of save_commit_buffer, which was introduced in a1c6d7c1a7
> ("fetch-pack: restore save_commit_buffer after use", 2017-12-08), is a
> concern of the parse_object() call in mark_complete_and_common_ref(), so
> it has been moved from the end of everything_local() to the end of
> mark_complete_and_common_ref().
Thanks, this is much cleaner.
>
> Signed-off-by: Jonathan Tan
> ---
> fetch-pack.c | 39 ++-
> 1 file changed, 30 insertions(+), 9 deletions(-)
>
> diff --git a/fetch-pack.c b/fetch-pack.c
> index a320ce987..5c87bb8bb 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -734,12 +734,20 @@ static int add_loose_objects_to_set(const struct
> object_id *oid,
> return 0;
> }
>
> -static int everything_local(struct fetch_pack_args *args,
> - struct ref **refs,
> - struct ref **sought, int nr_sought)
> +/*
> + * Mark recent commits available locally and reachable from a local ref as
> + * COMPLETE. If args->no_dependents is false, also mark COMPLETE remote refs
> as
> + * COMMON_REF (otherwise, we are not planning to participate in negotiation,
> and
> + * thus do not need COMMON_REF marks).
> + *
> + * The cutoff time for recency is determined by this heuristic: it is the
> + * earliest commit time of the objects in refs that are commits and that we
> know
> + * the commit time of.
> + */
> +static void mark_complete_and_common_ref(struct fetch_pack_args *args,
> + struct ref **refs)
> {
> struct ref *ref;
> - int retval;
> int old_save_commit_buffer = save_commit_buffer;
> timestamp_t cutoff = 0;
> struct oidset loose_oid_set = OIDSET_INIT;
> @@ -812,7 +820,18 @@ static int everything_local(struct fetch_pack_args *args,
> }
> }
>
> - filter_refs(args, refs, sought, nr_sought);
> + save_commit_buffer = old_save_commit_buffer;
> +}
> +
> +/*
> + * Returns 1 if every object pointed to by the given remote refs is available
> + * locally and reachable from a local ref, and 0 otherwise.
> + */
> +static int everything_local(struct fetch_pack_args *args,
> + struct ref **refs)
> +{
> + struct ref *ref;
> + int retval;
>
> for (retval = 1, ref = *refs; ref ; ref = ref->next) {
> const struct object_id *remote = >old_oid;
> @@ -829,8 +848,6 @@ static int everything_local(struct fetch_pack_args *args,
> ref->name);
> }
>
> - save_commit_buffer = old_save_commit_buffer;
> -
> return retval;
> }
>
> @@ -1053,7 +1070,9 @@ static struct ref *do_fetch_pack(struct fetch_pack_args
> *args,
> if (!server_supports("deepen-relative") && args->deepen_relative)
> die(_("Server does not support --deepen"));
>
> - if (everything_local(args, , sought, nr_sought)) {
> + mark_complete_and_common_ref(args, );
> + filter_refs(args, , sought, nr_sought);
> + if (everything_local(args, )) {
> packet_flush(fd[1]);
> goto all_done;
> }
> @@ -1377,7 +1396,9 @@ static struct ref *do_fetch_pack_v2(struct
> fetch_pack_args *args,
> for_each_cached_alternate(insert_one_alternate_object);
>
> /* Filter 'ref' by 'sought' and those that aren't local
> */
> - if (everything_local(args, , sought, nr_sought))
> + mark_complete_and_common_ref(args, );
> + filter_refs(args, , sought, nr_sought);
> + if (everything_local(args, ))
> state = FETCH_DONE;
> else
> state = FETCH_SEND_REQUEST;
> --
> 2.17.0.768.g1526ddbba1.dirty
>
--
Brandon Williams
[PATCH v2 6/8] fetch: refactor to make function args narrower
Refactor find_non_local_tags and get_ref_map to only take the
information they need instead of the entire transport struct. Besides
improving code clarity, this also improves their flexibility, allowing
for a different set of refs to be used instead of relying on the ones
stored in the transport struct.
Signed-off-by: Brandon Williams
---
builtin/fetch.c | 52 -
1 file changed, 26 insertions(+), 26 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index ee8b87c78..b600e1f10 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -254,9 +254,9 @@ static int will_fetch(struct ref **head, const unsigned
char *sha1)
return 0;
}
-static void find_non_local_tags(struct transport *transport,
- struct ref **head,
- struct ref ***tail)
+static void find_non_local_tags(const struct ref *refs,
+ struct ref **head,
+ struct ref ***tail)
{
struct string_list existing_refs = STRING_LIST_INIT_DUP;
struct string_list remote_refs = STRING_LIST_INIT_NODUP;
@@ -264,7 +264,7 @@ static void find_non_local_tags(struct transport *transport,
struct string_list_item *item = NULL;
for_each_ref(add_existing, _refs);
- for (ref = transport_get_remote_refs(transport, NULL); ref; ref =
ref->next) {
+ for (ref = refs; ref; ref = ref->next) {
if (!starts_with(ref->name, "refs/tags/"))
continue;
@@ -338,7 +338,8 @@ static void find_non_local_tags(struct transport *transport,
string_list_clear(_refs, 0);
}
-static struct ref *get_ref_map(struct transport *transport,
+static struct ref *get_ref_map(struct remote *remote,
+ const struct ref *remote_refs,
struct refspec *rs,
int tags, int *autotags)
{
@@ -346,27 +347,11 @@ static struct ref *get_ref_map(struct transport
*transport,
struct ref *rm;
struct ref *ref_map = NULL;
struct ref **tail = _map;
- struct argv_array ref_prefixes = ARGV_ARRAY_INIT;
/* opportunistically-updated references: */
struct ref *orefs = NULL, **oref_tail =
struct string_list existing_refs = STRING_LIST_INIT_DUP;
- const struct ref *remote_refs;
-
- 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);
-
- argv_array_clear(_prefixes);
if (rs->nr) {
struct refspec *fetch_refspec;
@@ -403,7 +388,7 @@ static struct ref *get_ref_map(struct transport *transport,
if (refmap.nr)
fetch_refspec =
else
- fetch_refspec = >remote->fetch;
+ fetch_refspec = >fetch;
for (i = 0; i < fetch_refspec->nr; i++)
get_fetch_map(ref_map, _refspec->items[i],
_tail, 1);
@@ -411,7 +396,6 @@ static struct ref *get_ref_map(struct transport *transport,
die("--refmap option is only meaningful with command-line
refspec(s).");
} else {
/* Use the defaults */
- struct remote *remote = transport->remote;
struct branch *branch = branch_get(NULL);
int has_merge = branch_has_merge_config(branch);
if (remote &&
@@ -450,7 +434,7 @@ static struct ref *get_ref_map(struct transport *transport,
/* also fetch all tags */
get_fetch_map(remote_refs, tag_refspec, , 0);
else if (tags == TAGS_DEFAULT && *autotags)
- find_non_local_tags(transport, _map, );
+ find_non_local_tags(remote_refs, _map, );
/* Now append any refs to be updated opportunistically: */
*tail = orefs;
@@ -1137,6 +1121,8 @@ static int do_fetch(struct transport *transport,
struct ref *ref_map;
int autotags = (transport->remote->fetch_tags == 1);
int retcode = 0;
+ const struct ref *remote_refs;
+ struct argv_array ref_prefixes = ARGV_ARRAY_INIT;
if (tags == TAGS_DEFAULT) {
if (transport->remote->fetch_tags == 2)
@@ -1152,7 +1138,21 @@ static int do_fetch(struct transport *transport,
goto cleanup;
}
- ref_map = get_ref_map(transport, rs, tags, );
+ if (rs->nr)
+ r
[PATCH v2 2/8] upload-pack: implement ref-in-want
Currently, while performing packfile negotiation, clients are only
allowed to specify their desired objects using object ids. This causes
a vulnerability to failure when an object turns non-existent during
negotiation, which may happen if, for example, the desired repository is
provided by multiple Git servers in a load-balancing arrangement.
In order to eliminate this vulnerability, implement the ref-in-want
feature for the 'fetch' command in protocol version 2. This feature
enables the 'fetch' command to support requests in the form of ref names
through a new "want-ref " parameter. At the conclusion of
negotiation, the server will send a list of all of the wanted references
(as provided by "want-ref" lines) in addition to the generated packfile.
Signed-off-by: Brandon Williams
---
Documentation/config.txt| 7 ++
Documentation/technical/protocol-v2.txt | 29 -
t/t5703-upload-pack-ref-in-want.sh | 153
upload-pack.c | 64 ++
4 files changed, 252 insertions(+), 1 deletion(-)
create mode 100755 t/t5703-upload-pack-ref-in-want.sh
diff --git a/Documentation/config.txt b/Documentation/config.txt
index ab641bf5a..fb1dd7428 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -3479,6 +3479,13 @@ Note that this configuration variable is ignored if it
is seen in the
repository-level config (this is a safety measure against fetching from
untrusted repositories).
+uploadpack.allowRefInWant::
+ If this option is set, `upload-pack` will support the `ref-in-want`
+ feature of the protocol version 2 `fetch` command. This feature
+ is intended for the benefit of load-balanced servers which may
+ not have the same view of what OIDs their refs point to due to
+ replication delay.
+
url..insteadOf::
Any URL that starts with this value will be rewritten to
start, instead, with . In cases where some site serves a
diff --git a/Documentation/technical/protocol-v2.txt
b/Documentation/technical/protocol-v2.txt
index 49bda76d2..6020632b4 100644
--- a/Documentation/technical/protocol-v2.txt
+++ b/Documentation/technical/protocol-v2.txt
@@ -299,12 +299,22 @@ included in the client's request:
for use with partial clone and partial fetch operations. See
`rev-list` for possible "filter-spec" values.
+If the 'ref-in-want' feature is advertised, the following argument can
+be included in the client's request as well as the potential addition of
+the 'wanted-refs' section in the server's response as explained below.
+
+want-ref
+ Indicates to the server that the client wants to retrieve a
+ particular ref, where is the full name of a ref on the
+ server. A server should ignore any "want-ref " lines where
+doesn't exist on the server.
+
The response of `fetch` is broken into a number of sections separated by
delimiter packets (0001), with each section beginning with its section
header.
output = *section
-section = (acknowledgments | shallow-info | packfile)
+section = (acknowledgments | shallow-info | wanted-refs | packfile)
(flush-pkt | delim-pkt)
acknowledgments = PKT-LINE("acknowledgments" LF)
@@ -319,6 +329,10 @@ header.
shallow = "shallow" SP obj-id
unshallow = "unshallow" SP obj-id
+wanted-refs = PKT-LINE("wanted-refs" LF)
+ *PKT-LINE(wanted-ref LF)
+wanted-ref = obj-id SP refname
+
packfile = PKT-LINE("packfile" LF)
*PKT-LINE(%x01-03 *%x00-ff)
@@ -379,6 +393,19 @@ header.
* This section is only included if a packfile section is also
included in the response.
+wanted-refs section
+ * This section is only included if the client has requested a
+ ref using a 'want-ref' line and if a packfile section is also
+ included in the response.
+
+ * Always begins with the section header "wanted-refs"
+
+ * The server will send a ref listing (" ") for
+ each reference requested using 'want-ref' lines.
+
+ * The server MUST NOT send any refs which were not requested
+ using 'want-ref' lines.
+
packfile section
* This section is only included if the client has sent 'want'
lines in its request and either requested that no more
diff --git a/t/t5703-upload-pack-ref-in-want.sh
b/t/t5703-upload-pack-ref-in-want.sh
new file mode 100755
index 0..0ef182970
--- /dev/null
+++ b/t/t5703-upload-pack-ref-in-want.sh
@@ -0,0 +1,153 @@
+#!/bin/sh
+
+test_description='upload-pack ref-in-want'
+
+. ./test-lib.sh
+
+get_actual_refs() {
+ sed -n '/wanted-refs/,/0001/p' actual_refs
+}
+
+get_actual_commits() {
+ sed -n '/packfile/,//p' o.pack &&
+ git index-pack o.pack &&
+ git verify-pack
[PATCH v2 7/8] fetch-pack: put shallow info in output parameter
Expand the transport fetch method signature, by adding an output
parameter, to allow transports to return information about the refs they
have fetched. Then communicate shallow status information through this
mechanism instead of by modifying the input list of refs.
This does require clients to sometimes generate the ref map twice: once
from the list of refs provided by the remote (as is currently done) and
potentially once from the new list of refs that the fetch mechanism
provides.
Signed-off-by: Brandon Williams
---
builtin/clone.c | 4 ++--
builtin/fetch.c | 23 +++
fetch-object.c | 2 +-
fetch-pack.c | 17 +
transport-helper.c | 6 --
transport-internal.h | 9 -
transport.c | 34 --
transport.h | 3 ++-
8 files changed, 73 insertions(+), 25 deletions(-)
diff --git a/builtin/clone.c b/builtin/clone.c
index 99e73dae8..8f86d99c5 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -1155,7 +1155,7 @@ int cmd_clone(int argc, const char **argv, const char
*prefix)
}
if (!is_local && !complete_refs_before_fetch)
- transport_fetch_refs(transport, mapped_refs);
+ transport_fetch_refs(transport, mapped_refs, NULL);
remote_head = find_ref_by_name(refs, "HEAD");
remote_head_points_at =
@@ -1197,7 +1197,7 @@ int cmd_clone(int argc, const char **argv, const char
*prefix)
if (is_local)
clone_local(path, git_dir);
else if (refs && complete_refs_before_fetch)
- transport_fetch_refs(transport, mapped_refs);
+ transport_fetch_refs(transport, mapped_refs, NULL);
update_remote_refs(refs, mapped_refs, remote_head_points_at,
branch_top.buf, reflog_msg.buf, transport,
diff --git a/builtin/fetch.c b/builtin/fetch.c
index b600e1f10..ddf44ba1a 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -946,11 +946,13 @@ static int quickfetch(struct ref *ref_map)
return check_connected(iterate_ref_map, , );
}
-static int fetch_refs(struct transport *transport, struct ref *ref_map)
+static int fetch_refs(struct transport *transport, struct ref *ref_map,
+ struct ref **updated_remote_refs)
{
int ret = quickfetch(ref_map);
if (ret)
- ret = transport_fetch_refs(transport, ref_map);
+ ret = transport_fetch_refs(transport, ref_map,
+ updated_remote_refs);
if (ret)
transport_unlock_pack(transport);
return ret;
@@ -1106,7 +1108,7 @@ static void backfill_tags(struct transport *transport,
struct ref *ref_map)
transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);
transport_set_option(transport, TRANS_OPT_DEPTH, "0");
transport_set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, NULL);
- if (!fetch_refs(transport, ref_map))
+ if (!fetch_refs(transport, ref_map, NULL))
consume_refs(transport, ref_map);
if (gsecondary) {
@@ -1122,6 +1124,7 @@ static int do_fetch(struct transport *transport,
int autotags = (transport->remote->fetch_tags == 1);
int retcode = 0;
const struct ref *remote_refs;
+ struct ref *new_remote_refs = NULL;
struct argv_array ref_prefixes = ARGV_ARRAY_INIT;
if (tags == TAGS_DEFAULT) {
@@ -1172,7 +1175,19 @@ static int do_fetch(struct transport *transport,
transport->url);
}
}
- if (fetch_refs(transport, ref_map) || consume_refs(transport, ref_map))
{
+
+ if (fetch_refs(transport, ref_map, _remote_refs)) {
+ free_refs(ref_map);
+ retcode = 1;
+ goto cleanup;
+ }
+ if (new_remote_refs) {
+ free_refs(ref_map);
+ ref_map = get_ref_map(transport->remote, new_remote_refs, rs,
+ tags, );
+ free_refs(new_remote_refs);
+ }
+ if (consume_refs(transport, ref_map)) {
free_refs(ref_map);
retcode = 1;
goto cleanup;
diff --git a/fetch-object.c b/fetch-object.c
index 853624f81..48fe63dd6 100644
--- a/fetch-object.c
+++ b/fetch-object.c
@@ -19,7 +19,7 @@ static void fetch_refs(const char *remote_name, struct ref
*ref)
transport_set_option(transport, TRANS_OPT_FROM_PROMISOR, "1");
transport_set_option(transport, TRANS_OPT_NO_DEPENDENTS, "1");
- transport_fetch_refs(transport, ref);
+ transport_fetch_refs(transport, ref, NULL);
fetch_if_missing = original_fetch_if_missing;
}
diff --git a/fetch-pack.c b/fetch-pack.c
index a320ce987..7799ee2cd 100644
---
[PATCH v2 5/8] fetch: refactor fetch_refs into two functions
Refactor the fetch_refs function into a function that does the fetching
of refs and another function that stores them.
Signed-off-by: Brandon Williams
---
builtin/fetch.c | 19 +--
1 file changed, 13 insertions(+), 6 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 545635448..ee8b87c78 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -967,10 +967,16 @@ static int fetch_refs(struct transport *transport, struct
ref *ref_map)
int ret = quickfetch(ref_map);
if (ret)
ret = transport_fetch_refs(transport, ref_map);
- if (!ret)
- ret |= store_updated_refs(transport->url,
- transport->remote->name,
- ref_map);
+ if (ret)
+ transport_unlock_pack(transport);
+ return ret;
+}
+
+static int consume_refs(struct transport *transport, struct ref *ref_map)
+{
+ int ret = store_updated_refs(transport->url,
+transport->remote->name,
+ref_map);
transport_unlock_pack(transport);
return ret;
}
@@ -1116,7 +1122,8 @@ static void backfill_tags(struct transport *transport,
struct ref *ref_map)
transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);
transport_set_option(transport, TRANS_OPT_DEPTH, "0");
transport_set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, NULL);
- fetch_refs(transport, ref_map);
+ if (!fetch_refs(transport, ref_map))
+ consume_refs(transport, ref_map);
if (gsecondary) {
transport_disconnect(gsecondary);
@@ -1165,7 +1172,7 @@ static int do_fetch(struct transport *transport,
transport->url);
}
}
- if (fetch_refs(transport, ref_map)) {
+ if (fetch_refs(transport, ref_map) || consume_refs(transport, ref_map))
{
free_refs(ref_map);
retcode = 1;
goto cleanup;
--
2.18.0.rc1.242.g61856ae69a-goog
[PATCH v2 3/8] upload-pack: test negotiation with changing repository
Add tests to check the behavior of fetching from a repository which
changes between rounds of negotiation (for example, when different
servers in a load-balancing agreement participate in the same stateless
RPC negotiation). This forms a baseline of comparison to the ref-in-want
functionality (which will be introduced to the client in subsequent
commits), and ensures that subsequent commits do not change existing
behavior.
As part of this effort, a mechanism to substitute strings in a single
HTTP response is added.
Signed-off-by: Brandon Williams
---
t/lib-httpd.sh | 1 +
t/lib-httpd/apache.conf| 8 +++
t/lib-httpd/one-time-sed.sh| 16 ++
t/t5703-upload-pack-ref-in-want.sh | 92 ++
4 files changed, 117 insertions(+)
create mode 100644 t/lib-httpd/one-time-sed.sh
diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index 435a37465..84f8efdd4 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -132,6 +132,7 @@ prepare_httpd() {
cp "$TEST_PATH"/passwd "$HTTPD_ROOT_PATH"
install_script broken-smart-http.sh
install_script error.sh
+ install_script one-time-sed.sh
ln -s "$LIB_HTTPD_MODULE_PATH" "$HTTPD_ROOT_PATH/modules"
diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index 724d9ae46..fe68d37bb 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -111,9 +111,14 @@ Alias /auth/dumb/ www/auth/dumb/
SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
SetEnv GIT_HTTP_EXPORT_ALL
+
+ SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
+ SetEnv GIT_HTTP_EXPORT_ALL
+
ScriptAliasMatch /smart_*[^/]*/(.*) ${GIT_EXEC_PATH}/git-http-backend/$1
ScriptAlias /broken_smart/ broken-smart-http.sh/
ScriptAlias /error/ error.sh/
+ScriptAliasMatch /one_time_sed/(.*) one-time-sed.sh/$1
Options FollowSymlinks
@@ -123,6 +128,9 @@ ScriptAlias /error/ error.sh/
Options ExecCGI
+
+ Options ExecCGI
+
Options ExecCGI
diff --git a/t/lib-httpd/one-time-sed.sh b/t/lib-httpd/one-time-sed.sh
new file mode 100644
index 0..a9c4aa5f4
--- /dev/null
+++ b/t/lib-httpd/one-time-sed.sh
@@ -0,0 +1,16 @@
+#!/bin/sh
+
+if [ -e one-time-sed ]; then
+ "$GIT_EXEC_PATH/git-http-backend" >out
+
+ sed "$(cat one-time-sed)" out_modified
+
+ if diff out out_modified >/dev/null; then
+ cat out
+ else
+ cat out_modified
+ rm one-time-sed
+ fi
+else
+ "$GIT_EXEC_PATH/git-http-backend"
+fi
diff --git a/t/t5703-upload-pack-ref-in-want.sh
b/t/t5703-upload-pack-ref-in-want.sh
index 0ef182970..979ab6d03 100755
--- a/t/t5703-upload-pack-ref-in-want.sh
+++ b/t/t5703-upload-pack-ref-in-want.sh
@@ -150,4 +150,96 @@ test_expect_success 'want-ref with ref we already have
commit for' '
check_output
'
+. "$TEST_DIRECTORY"/lib-httpd.sh
+start_httpd
+
+REPO="$HTTPD_DOCUMENT_ROOT_PATH/repo"
+LOCAL_PRISTINE="$(pwd)/local_pristine"
+
+test_expect_success 'setup repos for change-while-negotiating test' '
+ (
+ git init "$REPO" &&
+ cd "$REPO" &&
+ >.git/git-daemon-export-ok &&
+ test_commit m1 &&
+ git tag -d m1 &&
+
+ # Local repo with many commits (so that negotiation will take
+ # more than 1 request/response pair)
+ git clone "http://127.0.0.1:$LIB_HTTPD_PORT/smart/repo;
"$LOCAL_PRISTINE" &&
+ cd "$LOCAL_PRISTINE" &&
+ git checkout -b side &&
+ for i in $(seq 1 33); do test_commit s$i; done &&
+
+ # Add novel commits to upstream
+ git checkout master &&
+ cd "$REPO" &&
+ test_commit m2 &&
+ test_commit m3 &&
+ git tag -d m2 m3
+ ) &&
+ git -C "$LOCAL_PRISTINE" remote set-url origin
"http://127.0.0.1:$LIB_HTTPD_PORT/one_time_sed/repo; &&
+ git -C "$LOCAL_PRISTINE" config protocol.version 2
+'
+
+inconsistency() {
+ # Simulate that the server initially reports $2 as the ref
+ # corresponding to $1, and after that, $1 as the ref corresponding to
+ # $1. This corresponds to the real-life situation where the server's
+ # repository appears to change during negotiation, for example, when
+ # different servers in a load-balancing arrangement serve (stateless)
+ # RPCs during a single negotiation.
+ printf "s/%s/%s/" \
+ $(git -C "$REPO" rev-parse $1 | tr -d "\n") \
+ $(git -C "$REPO" rev-parse $2 | tr -d "\n") \
+
[PATCH v2 8/8] fetch-pack: implement ref-in-want
Implement ref-in-want on the client side so that when a server supports
the "ref-in-want" feature, a client will send "want-ref" lines for each
reference the client wants to fetch.
Signed-off-by: Brandon Williams
---
fetch-pack.c | 35 +++---
remote.c | 1 +
remote.h | 1 +
t/t5703-upload-pack-ref-in-want.sh | 4 ++--
4 files changed, 36 insertions(+), 5 deletions(-)
diff --git a/fetch-pack.c b/fetch-pack.c
index 7799ee2cd..51e8356ba 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1102,9 +1102,10 @@ static void add_shallow_requests(struct strbuf *req_buf,
static void add_wants(const struct ref *wants, struct strbuf *req_buf)
{
+ int use_ref_in_want = server_supports_feature("fetch", "ref-in-want",
0);
+
for ( ; wants ; wants = wants->next) {
const struct object_id *remote = >old_oid;
- const char *remote_hex;
struct object *o;
/*
@@ -1122,8 +1123,10 @@ static void add_wants(const struct ref *wants, struct
strbuf *req_buf)
continue;
}
- remote_hex = oid_to_hex(remote);
- packet_buf_write(req_buf, "want %s\n", remote_hex);
+ if (!use_ref_in_want || wants->exact_sha1)
+ packet_buf_write(req_buf, "want %s\n",
oid_to_hex(remote));
+ else
+ packet_buf_write(req_buf, "want-ref %s\n", wants->name);
}
}
@@ -1334,6 +1337,29 @@ static void receive_shallow_info(struct fetch_pack_args
*args,
args->deepen = 1;
}
+static void receive_wanted_refs(struct packet_reader *reader, struct ref *refs)
+{
+ process_section_header(reader, "wanted-refs", 0);
+ while (packet_reader_read(reader) == PACKET_READ_NORMAL) {
+ struct object_id oid;
+ const char *end;
+ struct ref *r = NULL;
+
+ if (parse_oid_hex(reader->line, , ) || *end++ != ' ')
+ die("expected wanted-ref, got '%s'", reader->line);
+
+ for (r = refs; r; r = r->next) {
+ if (!strcmp(end, r->name)) {
+ oidcpy(>old_oid, );
+ break;
+ }
+ }
+ }
+
+ if (reader->status != PACKET_READ_DELIM)
+ die("error processing wanted refs: %d", reader->status);
+}
+
enum fetch_state {
FETCH_CHECK_LOCAL = 0,
FETCH_SEND_REQUEST,
@@ -1408,6 +1434,9 @@ static struct ref *do_fetch_pack_v2(struct
fetch_pack_args *args,
if (process_section_header(, "shallow-info", 1))
receive_shallow_info(args, );
+ if (process_section_header(, "wanted-refs", 1))
+ receive_wanted_refs(, ref);
+
/* get the pack */
process_section_header(, "packfile", 0);
if (get_pack(args, fd, pack_lockfile))
diff --git a/remote.c b/remote.c
index abe80c139..c9d452ac0 100644
--- a/remote.c
+++ b/remote.c
@@ -1735,6 +1735,7 @@ int get_fetch_map(const struct ref *remote_refs,
if (refspec->exact_sha1) {
ref_map = alloc_ref(name);
get_oid_hex(name, _map->old_oid);
+ ref_map->exact_sha1 = 1;
} else {
ref_map = get_remote_ref(remote_refs, name);
}
diff --git a/remote.h b/remote.h
index 45ecc6cef..e5338e368 100644
--- a/remote.h
+++ b/remote.h
@@ -73,6 +73,7 @@ struct ref {
force:1,
forced_update:1,
expect_old_sha1:1,
+ exact_sha1:1,
deletion:1;
enum {
diff --git a/t/t5703-upload-pack-ref-in-want.sh
b/t/t5703-upload-pack-ref-in-want.sh
index 979ab6d03..b94a51380 100755
--- a/t/t5703-upload-pack-ref-in-want.sh
+++ b/t/t5703-upload-pack-ref-in-want.sh
@@ -204,7 +204,7 @@ test_expect_success 'server is initially ahead - no ref in
want' '
grep "ERR upload-pack: not our ref" err
'
-test_expect_failure 'server is initially ahead - ref in want' '
+test_expect_success 'server is initially ahead - ref in want' '
git -C "$REPO" config uploadpack.allowRefInWant true &&
rm -rf local &&
cp -r "$LOCAL_PRISTINE" local &&
@@ -228,7 +228,7 @@ test_expect_success 'server is initially behind - no ref in
want' '
test_cmp expected actual
'
-test_expect_failure 'server is initially behind - ref in want' '
+test_expect_success 'server is initially behind - ref in want' '
git -C "$REPO" config uploadpack.allowRefInWant true &&
rm -rf local &&
cp -r "$LOCAL_PRISTINE" local &&
--
2.18.0.rc1.242.g61856ae69a-goog
[PATCH v2 1/8] test-pkt-line: add unpack-sideband subcommand
Add an 'unpack-sideband' subcommand to the test-pkt-line helper to
enable unpacking packet line data sent multiplexed using a sideband.
Signed-off-by: Brandon Williams
---
t/helper/test-pkt-line.c | 37 +
1 file changed, 37 insertions(+)
diff --git a/t/helper/test-pkt-line.c b/t/helper/test-pkt-line.c
index 0f19e53c7..2a551 100644
--- a/t/helper/test-pkt-line.c
+++ b/t/helper/test-pkt-line.c
@@ -1,3 +1,4 @@
+#include "cache.h"
#include "pkt-line.h"
static void pack_line(const char *line)
@@ -48,6 +49,40 @@ static void unpack(void)
}
}
+static void unpack_sideband(void)
+{
+ struct packet_reader reader;
+ packet_reader_init(, 0, NULL, 0,
+ PACKET_READ_GENTLE_ON_EOF |
+ PACKET_READ_CHOMP_NEWLINE);
+
+ while (packet_reader_read() != PACKET_READ_EOF) {
+ int band;
+ int fd;
+
+ switch (reader.status) {
+ case PACKET_READ_EOF:
+ break;
+ case PACKET_READ_NORMAL:
+ band = reader.line[0] & 0xff;
+ if (band == 1)
+ fd = 1;
+ else
+ fd = 2;
+
+ write_or_die(fd, reader.line+1, reader.pktlen-1);
+
+ if (band == 3)
+ die("sind-band error");
+ break;
+ case PACKET_READ_FLUSH:
+ return;
+ case PACKET_READ_DELIM:
+ break;
+ }
+ }
+}
+
int cmd_main(int argc, const char **argv)
{
if (argc < 2)
@@ -57,6 +92,8 @@ int cmd_main(int argc, const char **argv)
pack(argc - 2, argv + 2);
else if (!strcmp(argv[1], "unpack"))
unpack();
+ else if (!strcmp(argv[1], "unpack-sideband"))
+ unpack_sideband();
else
die("invalid argument '%s'", argv[1]);
--
2.18.0.rc1.242.g61856ae69a-goog
[PATCH v2 4/8] fetch: refactor the population of peer ref OIDs
Populate peer ref OIDs in get_ref_map instead of do_fetch. Besides
tightening scopes of variables in the code, this also prepares for
get_ref_map being able to be called multiple times within do_fetch.
Signed-off-by: Brandon Williams
---
builtin/fetch.c | 36 ++--
1 file changed, 18 insertions(+), 18 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index ea5b9669a..545635448 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -351,6 +351,7 @@ static struct ref *get_ref_map(struct transport *transport,
/* opportunistically-updated references: */
struct ref *orefs = NULL, **oref_tail =
+ struct string_list existing_refs = STRING_LIST_INIT_DUP;
const struct ref *remote_refs;
if (rs->nr)
@@ -458,7 +459,23 @@ static struct ref *get_ref_map(struct transport *transport,
tail = >next;
}
- return ref_remove_duplicates(ref_map);
+ ref_map = ref_remove_duplicates(ref_map);
+
+ for_each_ref(add_existing, _refs);
+ for (rm = ref_map; rm; rm = rm->next) {
+ if (rm->peer_ref) {
+ struct string_list_item *peer_item =
+ string_list_lookup(_refs,
+ rm->peer_ref->name);
+ if (peer_item) {
+ struct object_id *old_oid = peer_item->util;
+ oidcpy(>peer_ref->old_oid, old_oid);
+ }
+ }
+ }
+ string_list_clear(_refs, 1);
+
+ return ref_map;
}
#define STORE_REF_ERROR_OTHER 1
@@ -1110,14 +1127,10 @@ static void backfill_tags(struct transport *transport,
struct ref *ref_map)
static int do_fetch(struct transport *transport,
struct refspec *rs)
{
- struct string_list existing_refs = STRING_LIST_INIT_DUP;
struct ref *ref_map;
- struct ref *rm;
int autotags = (transport->remote->fetch_tags == 1);
int retcode = 0;
- for_each_ref(add_existing, _refs);
-
if (tags == TAGS_DEFAULT) {
if (transport->remote->fetch_tags == 2)
tags = TAGS_SET;
@@ -1136,18 +1149,6 @@ static int do_fetch(struct transport *transport,
if (!update_head_ok)
check_not_current_branch(ref_map);
- for (rm = ref_map; rm; rm = rm->next) {
- if (rm->peer_ref) {
- struct string_list_item *peer_item =
- string_list_lookup(_refs,
- rm->peer_ref->name);
- if (peer_item) {
- struct object_id *old_oid = peer_item->util;
- oidcpy(>peer_ref->old_oid, old_oid);
- }
- }
- }
-
if (tags == TAGS_DEFAULT && autotags)
transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, "1");
if (prune) {
@@ -1183,7 +1184,6 @@ static int do_fetch(struct transport *transport,
}
cleanup:
- string_list_clear(_refs, 1);
return retcode;
}
--
2.18.0.rc1.242.g61856ae69a-goog
[PATCH v2 0/8] ref-in-want
Changes in v2: * issuing a want-ref line to a ref which doesn't exist is just ignored. * fixed some typos Brandon Williams (8): test-pkt-line: add unpack-sideband subcommand upload-pack: implement ref-in-want upload-pack: test negotiation with changing repository fetch: refactor the population of peer ref OIDs fetch: refactor fetch_refs into two functions fetch: refactor to make function args narrower fetch-pack: put shallow info in output parameter fetch-pack: implement ref-in-want Documentation/config.txt| 7 + Documentation/technical/protocol-v2.txt | 29 ++- builtin/clone.c | 4 +- builtin/fetch.c | 126 +++- fetch-object.c | 2 +- fetch-pack.c| 52 +++-- remote.c| 1 + remote.h| 1 + t/helper/test-pkt-line.c| 37 t/lib-httpd.sh | 1 + t/lib-httpd/apache.conf | 8 + t/lib-httpd/one-time-sed.sh | 16 ++ t/t5703-upload-pack-ref-in-want.sh | 245 transport-helper.c | 6 +- transport-internal.h| 9 +- transport.c | 34 +++- transport.h | 3 +- upload-pack.c | 64 +++ 18 files changed, 568 insertions(+), 77 deletions(-) create mode 100644 t/lib-httpd/one-time-sed.sh create mode 100755 t/t5703-upload-pack-ref-in-want.sh -- 2.18.0.rc1.242.g61856ae69a-goog
Re: [PATCH 2/8] upload-pack: implement ref-in-want
On 06/07, Ævar Arnfjörð Bjarmason wrote:
>
> On Wed, Jun 06 2018, Brandon Williams wrote:
>
> > On 06/05, Ævar Arnfjörð Bjarmason wrote:
> >>
> >> On Tue, Jun 05 2018, Brandon Williams wrote:
> >>
> >> > +uploadpack.allowRefInWant::
> >> > +If this option is set, `upload-pack` will support the
> >> > `ref-in-want`
> >> > +feature of the protocol version 2 `fetch` command.
> >> > +
> >>
> >> I think it makes sense to elaborate a bit on what this is for. Having
> >> read this series through, and to make sure I understood this, maybe
> >> something like this:
> >>
> >>This feature is intended for the benefit of load-balanced servers
> >>which may not have the same view of what SHA-1s their refs point to,
> >>but are guaranteed to never advertise a reference that another server
> >>serving the request doesn't know about.
> >>
> >> I.e. from what I can tell this gives no benefits for someone using a
> >> monolithic git server, except insofar as there would be a slight
> >> decrease in network traffic if the average length of refs is less than
> >> the length of a SHA-1.
> >
> > Yeah I agree that the motivation should probably be spelled out more,
> > thanks for the suggestion.
> >
> >>
> >> That's fair enough, just something we should prominently say.
> >>
> >> It does have the "disadvantage", if you can call it that, that it's
> >> introducing a race condition between when we read the ref advertisement
> >> and are promised XYZ refs, but may actually get ABC, but I can't think
> >> of a reason anyone would care about this in practice.
> >>
> >> The reason I'm saying "another server [...] doesn't know about" above is
> >> that 2/8 has this:
> >>
> >>if (read_ref(arg, ))
> >>die("unknown ref %s", arg);
> >>
> >> Doesn't that mean that if server A in your pool advertises master, next
> >> & pu, and you then go and fetch from server B advertising master & next,
> >> but not "pu" that the clone will die?
> >>
> >> Presumably at Google you either have something to ensure a consistent
> >> view, e.g. only advertise refs by name older than N seconds, or globally
> >> update ref name but not their contents, and don't allow deleting refs
> >> (or give them the same treatment).
> >>
> >> But that, and again, I may have misunderstood this whole thing,
> >> significantly reduces the utility of this feature for anyone "in the
> >> wild" since nothing shipped with "git" gives you that feature.
> >>
> >> The naïve way to do slave mirroring with stock git is to have a
> >> post-receive hook that pushes to your mirrors in a for-loop, or has them
> >> fetch from the master in a loop, and then round-robin LB those
> >> servers. Due to the "die on nonexisting" semantics in this extension
> >> that'll result in failed clones.
> >>
> >> So I think we should either be really vocal about that caveat, or
> >> perhaps think of how we could make that configurable, e.g. what happens
> >> if the server says "sorry, don't know about that one", and carries on
> >> with the rest it does know about?
> >
> > Jonathan actually pointed this out to me earlier and I think the best
> > way to deal with this is to just ignore the refs that the server doesn't
> > know about instead of dying here. I mean its no worse than what we
> > already have and we shouldn't hit this case too often. And that way the
> > fetch can still proceed.
> >
> >>
> >> Is there a way for client & server to gracefully recover from that?
> >> E.g. send "master" & "next" now, and when I pull again in a few seconds
> >> I get the new "pu"?
> >
> > I think in this case the client would just need to wait for some amount
> > of replication delay and attempt fetching at a later point.
> >
> >>
> >> Also, as a digression isn't that a problem shared with protocol v2 in
> >> general? I.e. without this extension isn't it going to make another
> >> connection to the naïve LB'd mirroring setup described above and find
> >> that SHA-1s as well as refs don't match?
> >
> > This is actually an issue with fetch using either v2 or v0. Unless I'm
> > misunderstanding what you're
Re: [PATCH 2/8] upload-pack: implement ref-in-want
On 06/05, Ævar Arnfjörð Bjarmason wrote:
>
> On Tue, Jun 05 2018, Brandon Williams wrote:
>
> > +uploadpack.allowRefInWant::
> > + If this option is set, `upload-pack` will support the `ref-in-want`
> > + feature of the protocol version 2 `fetch` command.
> > +
>
> I think it makes sense to elaborate a bit on what this is for. Having
> read this series through, and to make sure I understood this, maybe
> something like this:
>
>This feature is intended for the benefit of load-balanced servers
>which may not have the same view of what SHA-1s their refs point to,
>but are guaranteed to never advertise a reference that another server
>serving the request doesn't know about.
>
> I.e. from what I can tell this gives no benefits for someone using a
> monolithic git server, except insofar as there would be a slight
> decrease in network traffic if the average length of refs is less than
> the length of a SHA-1.
Yeah I agree that the motivation should probably be spelled out more,
thanks for the suggestion.
>
> That's fair enough, just something we should prominently say.
>
> It does have the "disadvantage", if you can call it that, that it's
> introducing a race condition between when we read the ref advertisement
> and are promised XYZ refs, but may actually get ABC, but I can't think
> of a reason anyone would care about this in practice.
>
> The reason I'm saying "another server [...] doesn't know about" above is
> that 2/8 has this:
>
> if (read_ref(arg, ))
> die("unknown ref %s", arg);
>
> Doesn't that mean that if server A in your pool advertises master, next
> & pu, and you then go and fetch from server B advertising master & next,
> but not "pu" that the clone will die?
>
> Presumably at Google you either have something to ensure a consistent
> view, e.g. only advertise refs by name older than N seconds, or globally
> update ref name but not their contents, and don't allow deleting refs
> (or give them the same treatment).
>
> But that, and again, I may have misunderstood this whole thing,
> significantly reduces the utility of this feature for anyone "in the
> wild" since nothing shipped with "git" gives you that feature.
>
> The naïve way to do slave mirroring with stock git is to have a
> post-receive hook that pushes to your mirrors in a for-loop, or has them
> fetch from the master in a loop, and then round-robin LB those
> servers. Due to the "die on nonexisting" semantics in this extension
> that'll result in failed clones.
>
> So I think we should either be really vocal about that caveat, or
> perhaps think of how we could make that configurable, e.g. what happens
> if the server says "sorry, don't know about that one", and carries on
> with the rest it does know about?
Jonathan actually pointed this out to me earlier and I think the best
way to deal with this is to just ignore the refs that the server doesn't
know about instead of dying here. I mean its no worse than what we
already have and we shouldn't hit this case too often. And that way the
fetch can still proceed.
>
> Is there a way for client & server to gracefully recover from that?
> E.g. send "master" & "next" now, and when I pull again in a few seconds
> I get the new "pu"?
I think in this case the client would just need to wait for some amount
of replication delay and attempt fetching at a later point.
>
> Also, as a digression isn't that a problem shared with protocol v2 in
> general? I.e. without this extension isn't it going to make another
> connection to the naïve LB'd mirroring setup described above and find
> that SHA-1s as well as refs don't match?
This is actually an issue with fetch using either v2 or v0. Unless I'm
misunderstanding what you're asking here.
>
> BREAK.
>
> Also is if this E-Mail wasn't long enough, on a completely different
> topic, in an earlier discussion in
> https://public-inbox.org/git/87inaje1uv@evledraar.gmail.com/ I noted
> that it would be neat-o to have optional wildmatch/pcre etc. matching
> for the use case you're not caring about here (and I don't expect you
> to, you're solving a different problem).
>
> But let's say I want to add that after this, and being unfamiliar with
> the protocol v2 conventions. Would that be a whole new
> ref-in-want-wildmatch-prefix capability with a new
> want-ref-wildmatch-prefix verb, or is there some less verbose way we can
> anticipate that use-case and internally version / advertise
> sub-capabilities?
>
> I don't know if that makes any sense, and would be fine with just a
> ref-in-want-wildmatch-pref
Re: [PATCH v3 17/20] cache.c: remove an implicit dependency on the_index
extern int add_file_to_index(struct index_state *, const char *path, int
> flags);
>
> -extern struct cache_entry *make_cache_entry(unsigned int mode, const
> unsigned char *sha1, const char *path, int stage, unsigned int
> refresh_options);
> +extern struct cache_entry *make_cache_entry(struct index_state
> *istate,unsigned int mode, const unsigned char *sha1, const char *path, int
> stage, unsigned int refresh_options);
minor nit: s/istate,unsigned/istate, unsigned/
> extern int chmod_index_entry(struct index_state *, struct cache_entry *ce,
> char flip);
> extern int ce_same_name(const struct cache_entry *a, const struct
> cache_entry *b);
> extern void set_object_name_for_intent_to_add_entry(struct cache_entry *ce);
> @@ -751,7 +752,7 @@ extern void fill_stat_cache_info(struct cache_entry *ce,
> struct stat *st);
> #define REFRESH_IGNORE_SUBMODULES0x0010 /* ignore submodules */
> #define REFRESH_IN_PORCELAIN 0x0020 /* user friendly output, not "needs
> update" */
> extern int refresh_index(struct index_state *, unsigned int flags, const
> struct pathspec *pathspec, char *seen, const char *header_msg);
> -extern struct cache_entry *refresh_cache_entry(struct cache_entry *,
> unsigned int);
> +extern struct cache_entry *refresh_index_entry(struct index_state *istate,
> struct cache_entry *, unsigned int);
>
> /*
> * Opportunistically update the index but do not complain if we can't.
> diff --git a/merge-recursive.c b/merge-recursive.c
> index b404ebac7c..9280deb6a1 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -315,7 +315,7 @@ static int add_cacheinfo(struct merge_options *o,
> struct cache_entry *ce;
> int ret;
>
> - ce = make_cache_entry(mode, oid ? oid->hash : null_sha1, path, stage,
> 0);
> + ce = make_cache_entry(_index, mode, oid ? oid->hash : null_sha1,
> path, stage, 0);
> if (!ce)
> return err(o, _("add_cacheinfo failed for path '%s'; merge
> aborting."), path);
>
> diff --git a/read-cache.c b/read-cache.c
> index 12cc22d157..c083318aa7 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -745,9 +745,11 @@ int add_file_to_index(struct index_state *istate, const
> char *path, int flags)
> return add_to_index(istate, path, , flags);
> }
>
> -struct cache_entry *make_cache_entry(unsigned int mode,
> - const unsigned char *sha1, const char *path, int stage,
> - unsigned int refresh_options)
> +struct cache_entry *make_cache_entry(struct index_state *istate,
> + unsigned int mode,
> + const unsigned char *sha1,
> + const char *path, int stage,
> + unsigned int refresh_options)
> {
> int size, len;
> struct cache_entry *ce, *ret;
> @@ -767,7 +769,7 @@ struct cache_entry *make_cache_entry(unsigned int mode,
> ce->ce_namelen = len;
> ce->ce_mode = create_ce_mode(mode);
>
> - ret = refresh_cache_entry(ce, refresh_options);
> + ret = refresh_index_entry(istate, ce, refresh_options);
> if (ret != ce)
> free(ce);
> return ret;
> @@ -1415,7 +1417,7 @@ int refresh_index(struct index_state *istate, unsigned
> int flags,
> if (ignore_submodules && S_ISGITLINK(ce->ce_mode))
> continue;
>
> - if (pathspec && !ce_path_match(_index, ce, pathspec, seen))
> + if (pathspec && !ce_path_match(istate, ce, pathspec, seen))
> filtered = 1;
>
> if (ce_stage(ce)) {
> @@ -1473,10 +1475,11 @@ int refresh_index(struct index_state *istate,
> unsigned int flags,
> return has_errors;
> }
>
> -struct cache_entry *refresh_cache_entry(struct cache_entry *ce,
> -unsigned int options)
> +struct cache_entry *refresh_index_entry(struct index_state *istate,
> + struct cache_entry *ce,
> + unsigned int options)
> {
> - return refresh_cache_ent(_index, ce, options, NULL, NULL);
> + return refresh_cache_ent(istate, ce, options, NULL, NULL);
> }
>
>
> diff --git a/resolve-undo.c b/resolve-undo.c
> index 5e4c8c5f75..9c45fe5d1d 100644
> --- a/resolve-undo.c
> +++ b/resolve-undo.c
> @@ -146,7 +146,7 @@ int unmerge_index_entry_at(struct index_state *istate,
> int pos)
> struct cache_entry *nce;
> if (!ru->mode[i])
> continue;
> - nce = make_cache_entry(ru->mode[i], ru->oid[i].hash,
> + nce = make_cache_entry(_index, ru->mode[i], ru->oid[i].hash,
> name, i + 1, 0);
> if (matched)
> nce->ce_flags |= CE_MATCHED;
> --
> 2.18.0.rc0.333.g22e6ee6cdf
>
--
Brandon Williams
Re: [PATCH v3 07/20] attr: remove an implicit dependency on the_index
On 06/06, Duy Nguyen wrote: > On Wed, Jun 6, 2018 at 6:50 PM, Brandon Williams wrote: > > On 06/06, Nguyễn Thái Ngọc Duy wrote: > >> Make the attr API take an index_state instead of assuming the_index in > >> attr code. All call sites are converted blindly to keep the patch > >> simple and retain current behavior. Individual call sites may receive > >> further updates to use the right index instead of the_index. > >> > >> There is one ugly temporary workaround added in attr.c that needs some > >> more explanation. > >> > >> Commit c24f3abace (apply: file commited * with CRLF should roundtrip > >> diff and apply - 2017-08-19) forces one convert_to_git() call to NOT > >> read the index at all. But what do you know, we read it anyway by > >> falling back to the_index. When "istate" from convert_to_git is now > >> propagated down to read_attr_from_array() we will hit segfault > >> somewhere inside read_blob_data_from_index. > >> > >> The right way of dealing with this is to kill "use_index" variable and > >> only follow "istate" but at this stage we are not ready for that: > >> while most git_attr_set_direction() calls just passes the_index to be > >> assigned to use_index, unpack-trees passes a different one which is > >> used by entry.c code, which has no way to know what index to use if we > >> delete use_index. So this has to be done later. > > > > Ugh I remember this when I was doing some refactoring to the attr > > subsystem a year or so ago. I really wanted to get rid of the whole > > "direction" thing because if I remember correctly its one of the only > > remaining globals that affects the outcome of an attr check (everything > > else was converted to use the attr check struct. I always got way to > > far into the weeds when trying to do that too. I'm not expecting that > > from this series (since its way out of scope) but maybe it'll be easier > > afterwards. > > It's not that much easier. This direction thing is still global by > design. It would be great if we have something like Scheme's parameter > (aka. dynamic scoping iirc) then we can still scope this nicely. Git > does not live in such worlds. Yeah i realized this after sending. Either way thanks for simplifying the global state by getting ride of the index global. > -- > Duy -- Brandon Williams
Re: [PATCH v3 15/20] attr: remove index from git_attr_set_direction()
x); > + git_attr_set_direction(GIT_ATTR_CHECKOUT); > > if (should_update_submodules() && o->update && !o->dry_run) > load_gitmodules_file(index, NULL); > @@ -421,7 +421,7 @@ static int check_updates(struct unpack_trees_options *o) > stop_progress(); > errs |= finish_delayed_checkout(); > if (o->update) > - git_attr_set_direction(GIT_ATTR_CHECKIN, NULL); > + git_attr_set_direction(GIT_ATTR_CHECKIN); > return errs != 0; > } > > -- > 2.18.0.rc0.333.g22e6ee6cdf > -- Brandon Williams
Re: [PATCH v3 07/20] attr: remove an implicit dependency on the_index
it_check_attr(_index, path, check)) {
> ll_driver_name = check->items[0].value;
> if (check->items[1].value) {
> marker_size = atoi(check->items[1].value);
> @@ -398,7 +398,7 @@ int ll_merge_marker_size(const char *path)
>
> if (!check)
> check = attr_check_initl("conflict-marker-size", NULL);
> - if (!git_check_attr(path, check) && check->items[0].value) {
> + if (!git_check_attr(_index, path, check) && check->items[0].value) {
> marker_size = atoi(check->items[0].value);
> if (marker_size <= 0)
> marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
> diff --git a/userdiff.c b/userdiff.c
> index a69241b25d..e835e78dd5 100644
> --- a/userdiff.c
> +++ b/userdiff.c
> @@ -278,7 +278,7 @@ struct userdiff_driver *userdiff_find_by_path(const char
> *path)
> check = attr_check_initl("diff", NULL);
> if (!path)
> return NULL;
> - if (git_check_attr(path, check))
> + if (git_check_attr(_index, path, check))
> return NULL;
>
> if (ATTR_TRUE(check->items[0].value))
> diff --git a/ws.c b/ws.c
> index a07caedd5a..5b67b426e7 100644
> --- a/ws.c
> +++ b/ws.c
> @@ -78,7 +78,7 @@ unsigned whitespace_rule(const char *pathname)
> if (!attr_whitespace_rule)
> attr_whitespace_rule = attr_check_initl("whitespace", NULL);
>
> - if (!git_check_attr(pathname, attr_whitespace_rule)) {
> + if (!git_check_attr(_index, pathname, attr_whitespace_rule)) {
> const char *value;
>
> value = attr_whitespace_rule->items[0].value;
> --
> 2.18.0.rc0.333.g22e6ee6cdf
>
--
Brandon Williams
Re: [PATCH 2/2] fetch: send "refs/tags/" prefix upon CLI refspecs
On 06/05, Jonathan Tan wrote:
> When performing tag following, in addition to using the server's
> "include-tag" capability to send tag objects (and emulating it if the
> server does not support that capability), "git fetch" relies upon the
> presence of refs/tags/* entries in the initial ref advertisement to
> locally create refs pointing to the aforementioned tag objects. When
> using protocol v2, refs/tags/* entries in the initial ref advertisement
> may be suppressed by a ref-prefix argument, leading to the tag object
> being downloaded, but the ref not being created.
>
> Commit dcc73cf7ff ("fetch: generate ref-prefixes when using a configured
> refspec", 2018-05-18) ensured that "refs/tags/" is always sent as a ref
> prefix when "git fetch" is invoked with no refspecs, but not when "git
> fetch" is invoked with refspecs. Extend that functionality to make it
> work in both situations.
>
> Signed-off-by: Jonathan Tan
> ---
> builtin/fetch.c| 2 +-
> t/t5702-protocol-v2.sh | 18 ++
> 2 files changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index ea5b9669a..1f447f1e8 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -359,7 +359,7 @@ static struct ref *get_ref_map(struct transport
> *transport,
> refspec_ref_prefixes(>remote->fetch, _prefixes);
>
> if (ref_prefixes.argc &&
> - (tags == TAGS_SET || (tags == TAGS_DEFAULT && !rs->nr))) {
> + (tags == TAGS_SET || tags == TAGS_DEFAULT)) {
> argv_array_push(_prefixes, "refs/tags/");
> }
This is difficult...Really I don't think the default should be to follow
tags. Mostly because this defeats the purpose of ref filtering when a
user only requests the master branch. Now instead of the server only
sending the master branch, you get the whole tags namespace as well.
>
> diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
> index 261e82b0f..6733579c1 100755
> --- a/t/t5702-protocol-v2.sh
> +++ b/t/t5702-protocol-v2.sh
> @@ -406,6 +406,24 @@ test_expect_success 'fetch supports various ways of have
> lines' '
> $(git -C server rev-parse completely-unrelated)
> '
>
> +test_expect_success 'fetch supports include-tag and tag following' '
> + rm -rf server client trace &&
> + git init server &&
> +
> + test_commit -C server to_fetch &&
> + git -C server tag -a annotated_tag -m message &&
> +
> + git init client &&
> + GIT_TRACE_PACKET="$(pwd)/trace" git -C client -c protocol.version=2 \
> + fetch "$(pwd)/server" to_fetch:to_fetch &&
> +
> + grep "fetch> ref-prefix to_fetch" trace &&
> + grep "fetch> ref-prefix refs/tags/" trace &&
> + grep "fetch> include-tag" trace &&
> +
> + git -C client cat-file -e $(git -C client rev-parse annotated_tag)
> +'
> +
> # Test protocol v2 with 'http://' transport
> #
> . "$TEST_DIRECTORY"/lib-httpd.sh
> --
> 2.17.0.768.g1526ddbba1.dirty
>
--
Brandon Williams
Re: [PATCH 2/2] fetch: send "refs/tags/" prefix upon CLI refspecs
On 06/05, Jonathan Tan wrote:
> When performing tag following, in addition to using the server's
> "include-tag" capability to send tag objects (and emulating it if the
> server does not support that capability), "git fetch" relies upon the
> presence of refs/tags/* entries in the initial ref advertisement to
> locally create refs pointing to the aforementioned tag objects. When
> using protocol v2, refs/tags/* entries in the initial ref advertisement
> may be suppressed by a ref-prefix argument, leading to the tag object
> being downloaded, but the ref not being created.
>
> Commit dcc73cf7ff ("fetch: generate ref-prefixes when using a configured
> refspec", 2018-05-18) ensured that "refs/tags/" is always sent as a ref
> prefix when "git fetch" is invoked with no refspecs, but not when "git
> fetch" is invoked with refspecs. Extend that functionality to make it
> work in both situations.
>
> Signed-off-by: Jonathan Tan
Test t5702-protocol-v2.sh doesn't pass with this patch.
> ---
> builtin/fetch.c| 2 +-
> t/t5702-protocol-v2.sh | 18 ++
> 2 files changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index ea5b9669a..1f447f1e8 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -359,7 +359,7 @@ static struct ref *get_ref_map(struct transport
> *transport,
> refspec_ref_prefixes(>remote->fetch, _prefixes);
>
> if (ref_prefixes.argc &&
> - (tags == TAGS_SET || (tags == TAGS_DEFAULT && !rs->nr))) {
> + (tags == TAGS_SET || tags == TAGS_DEFAULT)) {
> argv_array_push(_prefixes, "refs/tags/");
> }
>
> diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
> index 261e82b0f..6733579c1 100755
> --- a/t/t5702-protocol-v2.sh
> +++ b/t/t5702-protocol-v2.sh
> @@ -406,6 +406,24 @@ test_expect_success 'fetch supports various ways of have
> lines' '
> $(git -C server rev-parse completely-unrelated)
> '
>
> +test_expect_success 'fetch supports include-tag and tag following' '
> + rm -rf server client trace &&
> + git init server &&
> +
> + test_commit -C server to_fetch &&
> + git -C server tag -a annotated_tag -m message &&
> +
> + git init client &&
> + GIT_TRACE_PACKET="$(pwd)/trace" git -C client -c protocol.version=2 \
> + fetch "$(pwd)/server" to_fetch:to_fetch &&
> +
> + grep "fetch> ref-prefix to_fetch" trace &&
> + grep "fetch> ref-prefix refs/tags/" trace &&
> + grep "fetch> include-tag" trace &&
> +
> + git -C client cat-file -e $(git -C client rev-parse annotated_tag)
> +'
> +
> # Test protocol v2 with 'http://' transport
> #
> . "$TEST_DIRECTORY"/lib-httpd.sh
> --
> 2.17.0.768.g1526ddbba1.dirty
>
--
Brandon Williams
Re: [PATCH] docs: link to gitsubmodules
On 06/05, Jonathan Nieder wrote: > Jonathan Nieder wrote: > > > --- i/Documentation/config.txt > > +++ w/Documentation/config.txt > > @@ -3327,13 +3327,13 @@ submodule..ignore:: > > submodule..active:: > > Boolean value indicating if the submodule is of interest to git > > commands. This config option takes precedence over the > > - submodule.active config option. See linkgit:git-submodule[1] for > > + submodule.active config option. See linkgit:gitsubmodules[7] for > > details. > > > > submodule.active:: > > A repeated field which contains a pathspec used to match against a > > submodule's path to determine if the submodule is of interest to git > > - commands. See linkgit:git-submodule[1] for details. > > + commands. See linkgit:gitsubmodule[7] for details. > > Gah, and I can't spell. This one should have been > linkgit:gitsubmodules[7]. Updated diff below. Tested using > > make -C Documentation/ git-config.html gitsubmodules.html > w3m Documentation/git-config.html > > Thanks and sorry for the noise, > Jonathan > > diff --git i/Documentation/config.txt w/Documentation/config.txt > index 1277731aa4..340eb1f3c4 100644 > --- i/Documentation/config.txt > +++ w/Documentation/config.txt > @@ -3327,13 +3327,13 @@ submodule..ignore:: > submodule..active:: > Boolean value indicating if the submodule is of interest to git > commands. This config option takes precedence over the > - submodule.active config option. See linkgit:git-submodule[1] for > + submodule.active config option. See linkgit:gitsubmodules[7] for > details. > > submodule.active:: > A repeated field which contains a pathspec used to match against a > submodule's path to determine if the submodule is of interest to git > - commands. See linkgit:git-submodule[1] for details. > + commands. See linkgit:gitsubmodules[7] for details. > > submodule.recurse:: > Specifies if commands recurse into submodules by default. This Yep this is what I meant. -- Brandon Williams
Re: [PATCH] docs: link to gitsubmodules
On 06/05, Ævar Arnfjörð Bjarmason wrote: > > On Tue, Jun 05 2018, Brandon Williams wrote: > > > Add a link to gitsubmodules(7) under the `submodule.active` entry in > > git-config(1). > > Did you mean to change either the subject or content of this patch? Your > subject says gitsubmodules(7), but you link to git-submodule(1). Yep I meant for it to be to gitsubmodules(7), turns out I don't know how our documentation is built :) -- Brandon Williams
[PATCH] docs: link to gitsubmodules
Add a link to gitsubmodules(7) under the `submodule.active` entry in git-config(1). Signed-off-by: Brandon Williams --- Documentation/config.txt | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Documentation/config.txt b/Documentation/config.txt index ab641bf5a..1277731aa 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -3327,12 +3327,13 @@ submodule..ignore:: submodule..active:: Boolean value indicating if the submodule is of interest to git commands. This config option takes precedence over the - submodule.active config option. + submodule.active config option. See linkgit:git-submodule[1] for + details. submodule.active:: A repeated field which contains a pathspec used to match against a submodule's path to determine if the submodule is of interest to git - commands. + commands. See linkgit:git-submodule[1] for details. submodule.recurse:: Specifies if commands recurse into submodules by default. This -- 2.17.1.1185.g55be947832-goog
Re: [PATCH 0/3] refspec: refactor & fix free() behavior
On 06/05, Ævar Arnfjörð Bjarmason wrote: > Since Martin & Brandon both liked this direction I've fixed it > up. > > Martin: I didn't want to be the author of the actual fix for the bug > you found, so I rewrote your commit in 3/3. The diff is different, and > I slightly modified the 3rd paragraph of the commit message & added my > sign-off, but otherwise it's the same. Thanks for writing up a proper patch series for this fix. I liked breaking up your diff into two different patches to make it clear that all callers of refpsec_item_init relying on dieing. > > Martin Ågren (1): > refspec: initalize `refspec_item` in `valid_fetch_refspec()` > > Ævar Arnfjörð Bjarmason (2): > refspec: s/refspec_item_init/&_or_die/g > refspec: add back a refspec_item_init() function > > builtin/clone.c | 2 +- > builtin/pull.c | 2 +- > refspec.c | 13 + > refspec.h | 5 - > 4 files changed, 15 insertions(+), 7 deletions(-) > > -- > 2.17.0.290.gded63e768a > -- Brandon Williams
[PATCH 2/8] upload-pack: implement ref-in-want
Currently, while performing packfile negotiation, clients are only
allowed to specify their desired objects using object ids. This causes
a vulnerability to failure when an object turns non-existent during
negotiation, which may happen if, for example, the desired repository is
provided by multiple Git servers in a load-balancing arrangement.
In order to eliminate this vulnerability, implement the ref-in-want
feature for the 'fetch' command in protocol version 2. This feature
enables the 'fetch' command to support requests in the form of ref names
through a new "want-ref " parameter. At the conclusion of
negotiation, the server will send a list of all of the wanted references
(as provided by "want-ref" lines) in addition to the generated packfile.
Signed-off-by: Brandon Williams
---
Documentation/config.txt| 4 +
Documentation/technical/protocol-v2.txt | 28 -
t/t5703-upload-pack-ref-in-want.sh | 153
upload-pack.c | 64 ++
4 files changed, 248 insertions(+), 1 deletion(-)
create mode 100755 t/t5703-upload-pack-ref-in-want.sh
diff --git a/Documentation/config.txt b/Documentation/config.txt
index ab641bf5a..acafe6c8d 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -3479,6 +3479,10 @@ Note that this configuration variable is ignored if it
is seen in the
repository-level config (this is a safety measure against fetching from
untrusted repositories).
+uploadpack.allowRefInWant::
+ If this option is set, `upload-pack` will support the `ref-in-want`
+ feature of the protocol version 2 `fetch` command.
+
url..insteadOf::
Any URL that starts with this value will be rewritten to
start, instead, with . In cases where some site serves a
diff --git a/Documentation/technical/protocol-v2.txt
b/Documentation/technical/protocol-v2.txt
index 49bda76d2..8367e09b8 100644
--- a/Documentation/technical/protocol-v2.txt
+++ b/Documentation/technical/protocol-v2.txt
@@ -299,12 +299,21 @@ included in the client's request:
for use with partial clone and partial fetch operations. See
`rev-list` for possible "filter-spec" values.
+If the 'ref-in-want' feature is advertised, the following argument can
+be included in the client's request as well as the potential addition of
+the 'wanted-refs' section in the server's response as explained below.
+
+want-ref
+ Indicates to the server than the client wants to retrieve a
+ particular ref, where is the full name of a ref on the
+ server.
+
The response of `fetch` is broken into a number of sections separated by
delimiter packets (0001), with each section beginning with its section
header.
output = *section
-section = (acknowledgments | shallow-info | packfile)
+section = (acknowledgments | shallow-info | wanted-refs | packfile)
(flush-pkt | delim-pkt)
acknowledgments = PKT-LINE("acknowledgments" LF)
@@ -319,6 +328,10 @@ header.
shallow = "shallow" SP obj-id
unshallow = "unshallow" SP obj-id
+wanted-refs = PKT-LINE("wanted-refs" LF)
+ *PKT-Line(wanted-ref LF)
+wanted-ref = obj-id SP refname
+
packfile = PKT-LINE("packfile" LF)
*PKT-LINE(%x01-03 *%x00-ff)
@@ -379,6 +392,19 @@ header.
* This section is only included if a packfile section is also
included in the response.
+wanted-refs section
+ * This section is only included if the client has requested a
+ ref using a 'want-ref' line and if a packfile section is also
+ included in the response.
+
+ * Always begins with the section header "wanted-refs"
+
+ * The server will send a ref listing (" ") for
+ each reference requested using 'want-ref' lines.
+
+ * Ther server MUST NOT send any refs which were not requested
+ using 'want-ref' lines.
+
packfile section
* This section is only included if the client has sent 'want'
lines in its request and either requested that no more
diff --git a/t/t5703-upload-pack-ref-in-want.sh
b/t/t5703-upload-pack-ref-in-want.sh
new file mode 100755
index 0..0ef182970
--- /dev/null
+++ b/t/t5703-upload-pack-ref-in-want.sh
@@ -0,0 +1,153 @@
+#!/bin/sh
+
+test_description='upload-pack ref-in-want'
+
+. ./test-lib.sh
+
+get_actual_refs() {
+ sed -n '/wanted-refs/,/0001/p' actual_refs
+}
+
+get_actual_commits() {
+ sed -n '/packfile/,//p' o.pack &&
+ git index-pack o.pack &&
+ git verify-pack -v o.idx | grep commit | cut -c-40 | sort
>actual_commits
+}
+
+check_output() {
+ get_actual_refs &&
+ test_cmp expected_refs actual_refs &&
+ get_actual_commits &&
+ test_cmp expected_commits actual_commits
+}
+
+# c(o/foo) d(o/
[PATCH 0/8] ref-in-want
This series adds the ref-in-want feature which was originally proposed by Jonathan Tan (https://public-inbox.org/git/cover.1485381677.git.jonathanta...@google.com/). Back when ref-in-want was first discussed it was decided that we should first solve the issue of moving to a new wire format and find a way to limit the ref-advertisement before moving forward with ref-in-want. Now that protocol version 2 is a reality, and that refs can be filtered on the server side, we can revisit ref-in-want. This version of ref-in-want is a bit more restrictive than what Jonathan originally proposed (only full ref names are allowed instead of globs and OIDs), but it is meant to accomplish the same goal (solve the issues of refs changing during negotiation). Brandon Williams (8): test-pkt-line: add unpack-sideband subcommand upload-pack: implement ref-in-want upload-pack: test negotiation with changing repository fetch: refactor the population of peer ref OIDs fetch: refactor fetch_refs into two functions fetch: refactor to make function args narrower fetch-pack: put shallow info in output parameter fetch-pack: implement ref-in-want Documentation/config.txt| 4 + Documentation/technical/protocol-v2.txt | 28 ++- builtin/clone.c | 4 +- builtin/fetch.c | 126 +++- fetch-object.c | 2 +- fetch-pack.c| 52 +++-- remote.c| 1 + remote.h| 1 + t/helper/test-pkt-line.c| 37 t/lib-httpd.sh | 1 + t/lib-httpd/apache.conf | 8 + t/lib-httpd/one-time-sed.sh | 16 ++ t/t5703-upload-pack-ref-in-want.sh | 245 transport-helper.c | 6 +- transport-internal.h| 9 +- transport.c | 34 +++- transport.h | 3 +- upload-pack.c | 64 +++ 18 files changed, 564 insertions(+), 77 deletions(-) create mode 100644 t/lib-httpd/one-time-sed.sh create mode 100755 t/t5703-upload-pack-ref-in-want.sh -- 2.17.1.1185.g55be947832-goog
[PATCH 1/8] test-pkt-line: add unpack-sideband subcommand
Add an 'unpack-sideband' subcommand to the test-pkt-line helper to
enable unpacking packet line data sent multiplexed using a sideband.
Signed-off-by: Brandon Williams
---
t/helper/test-pkt-line.c | 37 +
1 file changed, 37 insertions(+)
diff --git a/t/helper/test-pkt-line.c b/t/helper/test-pkt-line.c
index 0f19e53c7..2a551 100644
--- a/t/helper/test-pkt-line.c
+++ b/t/helper/test-pkt-line.c
@@ -1,3 +1,4 @@
+#include "cache.h"
#include "pkt-line.h"
static void pack_line(const char *line)
@@ -48,6 +49,40 @@ static void unpack(void)
}
}
+static void unpack_sideband(void)
+{
+ struct packet_reader reader;
+ packet_reader_init(, 0, NULL, 0,
+ PACKET_READ_GENTLE_ON_EOF |
+ PACKET_READ_CHOMP_NEWLINE);
+
+ while (packet_reader_read() != PACKET_READ_EOF) {
+ int band;
+ int fd;
+
+ switch (reader.status) {
+ case PACKET_READ_EOF:
+ break;
+ case PACKET_READ_NORMAL:
+ band = reader.line[0] & 0xff;
+ if (band == 1)
+ fd = 1;
+ else
+ fd = 2;
+
+ write_or_die(fd, reader.line+1, reader.pktlen-1);
+
+ if (band == 3)
+ die("sind-band error");
+ break;
+ case PACKET_READ_FLUSH:
+ return;
+ case PACKET_READ_DELIM:
+ break;
+ }
+ }
+}
+
int cmd_main(int argc, const char **argv)
{
if (argc < 2)
@@ -57,6 +92,8 @@ int cmd_main(int argc, const char **argv)
pack(argc - 2, argv + 2);
else if (!strcmp(argv[1], "unpack"))
unpack();
+ else if (!strcmp(argv[1], "unpack-sideband"))
+ unpack_sideband();
else
die("invalid argument '%s'", argv[1]);
--
2.17.1.1185.g55be947832-goog
[PATCH 5/8] fetch: refactor fetch_refs into two functions
Refactor the fetch_refs function into a function that does the fetching
of refs and another function that stores them.
Signed-off-by: Brandon Williams
---
builtin/fetch.c | 19 +--
1 file changed, 13 insertions(+), 6 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 545635448..ee8b87c78 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -967,10 +967,16 @@ static int fetch_refs(struct transport *transport, struct
ref *ref_map)
int ret = quickfetch(ref_map);
if (ret)
ret = transport_fetch_refs(transport, ref_map);
- if (!ret)
- ret |= store_updated_refs(transport->url,
- transport->remote->name,
- ref_map);
+ if (ret)
+ transport_unlock_pack(transport);
+ return ret;
+}
+
+static int consume_refs(struct transport *transport, struct ref *ref_map)
+{
+ int ret = store_updated_refs(transport->url,
+transport->remote->name,
+ref_map);
transport_unlock_pack(transport);
return ret;
}
@@ -1116,7 +1122,8 @@ static void backfill_tags(struct transport *transport,
struct ref *ref_map)
transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);
transport_set_option(transport, TRANS_OPT_DEPTH, "0");
transport_set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, NULL);
- fetch_refs(transport, ref_map);
+ if (!fetch_refs(transport, ref_map))
+ consume_refs(transport, ref_map);
if (gsecondary) {
transport_disconnect(gsecondary);
@@ -1165,7 +1172,7 @@ static int do_fetch(struct transport *transport,
transport->url);
}
}
- if (fetch_refs(transport, ref_map)) {
+ if (fetch_refs(transport, ref_map) || consume_refs(transport, ref_map))
{
free_refs(ref_map);
retcode = 1;
goto cleanup;
--
2.17.1.1185.g55be947832-goog
[PATCH 7/8] fetch-pack: put shallow info in output parameter
Expand the transport fetch method signature, by adding an output
parameter, to allow transports to return information about the refs they
have fetched. Then communicate shallow status information through this
mechanism instead of by modifying the input list of refs.
This does require clients to sometimes generate the ref map twice: once
from the list of refs provided by the remote (as is currently done) and
potentially once from the new list of refs that the fetch mechanism
provides.
Signed-off-by: Brandon Williams
---
builtin/clone.c | 4 ++--
builtin/fetch.c | 23 +++
fetch-object.c | 2 +-
fetch-pack.c | 17 +
transport-helper.c | 6 --
transport-internal.h | 9 -
transport.c | 34 --
transport.h | 3 ++-
8 files changed, 73 insertions(+), 25 deletions(-)
diff --git a/builtin/clone.c b/builtin/clone.c
index 99e73dae8..8f86d99c5 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -1155,7 +1155,7 @@ int cmd_clone(int argc, const char **argv, const char
*prefix)
}
if (!is_local && !complete_refs_before_fetch)
- transport_fetch_refs(transport, mapped_refs);
+ transport_fetch_refs(transport, mapped_refs, NULL);
remote_head = find_ref_by_name(refs, "HEAD");
remote_head_points_at =
@@ -1197,7 +1197,7 @@ int cmd_clone(int argc, const char **argv, const char
*prefix)
if (is_local)
clone_local(path, git_dir);
else if (refs && complete_refs_before_fetch)
- transport_fetch_refs(transport, mapped_refs);
+ transport_fetch_refs(transport, mapped_refs, NULL);
update_remote_refs(refs, mapped_refs, remote_head_points_at,
branch_top.buf, reflog_msg.buf, transport,
diff --git a/builtin/fetch.c b/builtin/fetch.c
index b600e1f10..ddf44ba1a 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -946,11 +946,13 @@ static int quickfetch(struct ref *ref_map)
return check_connected(iterate_ref_map, , );
}
-static int fetch_refs(struct transport *transport, struct ref *ref_map)
+static int fetch_refs(struct transport *transport, struct ref *ref_map,
+ struct ref **updated_remote_refs)
{
int ret = quickfetch(ref_map);
if (ret)
- ret = transport_fetch_refs(transport, ref_map);
+ ret = transport_fetch_refs(transport, ref_map,
+ updated_remote_refs);
if (ret)
transport_unlock_pack(transport);
return ret;
@@ -1106,7 +1108,7 @@ static void backfill_tags(struct transport *transport,
struct ref *ref_map)
transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);
transport_set_option(transport, TRANS_OPT_DEPTH, "0");
transport_set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, NULL);
- if (!fetch_refs(transport, ref_map))
+ if (!fetch_refs(transport, ref_map, NULL))
consume_refs(transport, ref_map);
if (gsecondary) {
@@ -1122,6 +1124,7 @@ static int do_fetch(struct transport *transport,
int autotags = (transport->remote->fetch_tags == 1);
int retcode = 0;
const struct ref *remote_refs;
+ struct ref *new_remote_refs = NULL;
struct argv_array ref_prefixes = ARGV_ARRAY_INIT;
if (tags == TAGS_DEFAULT) {
@@ -1172,7 +1175,19 @@ static int do_fetch(struct transport *transport,
transport->url);
}
}
- if (fetch_refs(transport, ref_map) || consume_refs(transport, ref_map))
{
+
+ if (fetch_refs(transport, ref_map, _remote_refs)) {
+ free_refs(ref_map);
+ retcode = 1;
+ goto cleanup;
+ }
+ if (new_remote_refs) {
+ free_refs(ref_map);
+ ref_map = get_ref_map(transport->remote, new_remote_refs, rs,
+ tags, );
+ free_refs(new_remote_refs);
+ }
+ if (consume_refs(transport, ref_map)) {
free_refs(ref_map);
retcode = 1;
goto cleanup;
diff --git a/fetch-object.c b/fetch-object.c
index 853624f81..48fe63dd6 100644
--- a/fetch-object.c
+++ b/fetch-object.c
@@ -19,7 +19,7 @@ static void fetch_refs(const char *remote_name, struct ref
*ref)
transport_set_option(transport, TRANS_OPT_FROM_PROMISOR, "1");
transport_set_option(transport, TRANS_OPT_NO_DEPENDENTS, "1");
- transport_fetch_refs(transport, ref);
+ transport_fetch_refs(transport, ref, NULL);
fetch_if_missing = original_fetch_if_missing;
}
diff --git a/fetch-pack.c b/fetch-pack.c
index a320ce987..7799ee2cd 100644
---
[PATCH 8/8] fetch-pack: implement ref-in-want
Implement ref-in-want on the client side so that when a server supports
the "ref-in-want" feature, a client will send "want-ref" lines for each
reference the client wants to fetch.
Signed-off-by: Brandon Williams
---
fetch-pack.c | 35 +++---
remote.c | 1 +
remote.h | 1 +
t/t5703-upload-pack-ref-in-want.sh | 4 ++--
4 files changed, 36 insertions(+), 5 deletions(-)
diff --git a/fetch-pack.c b/fetch-pack.c
index 7799ee2cd..51e8356ba 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1102,9 +1102,10 @@ static void add_shallow_requests(struct strbuf *req_buf,
static void add_wants(const struct ref *wants, struct strbuf *req_buf)
{
+ int use_ref_in_want = server_supports_feature("fetch", "ref-in-want",
0);
+
for ( ; wants ; wants = wants->next) {
const struct object_id *remote = >old_oid;
- const char *remote_hex;
struct object *o;
/*
@@ -1122,8 +1123,10 @@ static void add_wants(const struct ref *wants, struct
strbuf *req_buf)
continue;
}
- remote_hex = oid_to_hex(remote);
- packet_buf_write(req_buf, "want %s\n", remote_hex);
+ if (!use_ref_in_want || wants->exact_sha1)
+ packet_buf_write(req_buf, "want %s\n",
oid_to_hex(remote));
+ else
+ packet_buf_write(req_buf, "want-ref %s\n", wants->name);
}
}
@@ -1334,6 +1337,29 @@ static void receive_shallow_info(struct fetch_pack_args
*args,
args->deepen = 1;
}
+static void receive_wanted_refs(struct packet_reader *reader, struct ref *refs)
+{
+ process_section_header(reader, "wanted-refs", 0);
+ while (packet_reader_read(reader) == PACKET_READ_NORMAL) {
+ struct object_id oid;
+ const char *end;
+ struct ref *r = NULL;
+
+ if (parse_oid_hex(reader->line, , ) || *end++ != ' ')
+ die("expected wanted-ref, got '%s'", reader->line);
+
+ for (r = refs; r; r = r->next) {
+ if (!strcmp(end, r->name)) {
+ oidcpy(>old_oid, );
+ break;
+ }
+ }
+ }
+
+ if (reader->status != PACKET_READ_DELIM)
+ die("error processing wanted refs: %d", reader->status);
+}
+
enum fetch_state {
FETCH_CHECK_LOCAL = 0,
FETCH_SEND_REQUEST,
@@ -1408,6 +1434,9 @@ static struct ref *do_fetch_pack_v2(struct
fetch_pack_args *args,
if (process_section_header(, "shallow-info", 1))
receive_shallow_info(args, );
+ if (process_section_header(, "wanted-refs", 1))
+ receive_wanted_refs(, ref);
+
/* get the pack */
process_section_header(, "packfile", 0);
if (get_pack(args, fd, pack_lockfile))
diff --git a/remote.c b/remote.c
index abe80c139..c9d452ac0 100644
--- a/remote.c
+++ b/remote.c
@@ -1735,6 +1735,7 @@ int get_fetch_map(const struct ref *remote_refs,
if (refspec->exact_sha1) {
ref_map = alloc_ref(name);
get_oid_hex(name, _map->old_oid);
+ ref_map->exact_sha1 = 1;
} else {
ref_map = get_remote_ref(remote_refs, name);
}
diff --git a/remote.h b/remote.h
index 45ecc6cef..e5338e368 100644
--- a/remote.h
+++ b/remote.h
@@ -73,6 +73,7 @@ struct ref {
force:1,
forced_update:1,
expect_old_sha1:1,
+ exact_sha1:1,
deletion:1;
enum {
diff --git a/t/t5703-upload-pack-ref-in-want.sh
b/t/t5703-upload-pack-ref-in-want.sh
index 979ab6d03..b94a51380 100755
--- a/t/t5703-upload-pack-ref-in-want.sh
+++ b/t/t5703-upload-pack-ref-in-want.sh
@@ -204,7 +204,7 @@ test_expect_success 'server is initially ahead - no ref in
want' '
grep "ERR upload-pack: not our ref" err
'
-test_expect_failure 'server is initially ahead - ref in want' '
+test_expect_success 'server is initially ahead - ref in want' '
git -C "$REPO" config uploadpack.allowRefInWant true &&
rm -rf local &&
cp -r "$LOCAL_PRISTINE" local &&
@@ -228,7 +228,7 @@ test_expect_success 'server is initially behind - no ref in
want' '
test_cmp expected actual
'
-test_expect_failure 'server is initially behind - ref in want' '
+test_expect_success 'server is initially behind - ref in want' '
git -C "$REPO" config uploadpack.allowRefInWant true &&
rm -rf local &&
cp -r "$LOCAL_PRISTINE" local &&
--
2.17.1.1185.g55be947832-goog
[PATCH 6/8] fetch: refactor to make function args narrower
Refactor find_non_local_tags and get_ref_map to only take the
information they need instead of the entire transport struct. Besides
improving code clarity, this also improves their flexibility, allowing
for a different set of refs to be used instead of relying on the ones
stored in the transport struct.
Signed-off-by: Brandon Williams
---
builtin/fetch.c | 52 -
1 file changed, 26 insertions(+), 26 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index ee8b87c78..b600e1f10 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -254,9 +254,9 @@ static int will_fetch(struct ref **head, const unsigned
char *sha1)
return 0;
}
-static void find_non_local_tags(struct transport *transport,
- struct ref **head,
- struct ref ***tail)
+static void find_non_local_tags(const struct ref *refs,
+ struct ref **head,
+ struct ref ***tail)
{
struct string_list existing_refs = STRING_LIST_INIT_DUP;
struct string_list remote_refs = STRING_LIST_INIT_NODUP;
@@ -264,7 +264,7 @@ static void find_non_local_tags(struct transport *transport,
struct string_list_item *item = NULL;
for_each_ref(add_existing, _refs);
- for (ref = transport_get_remote_refs(transport, NULL); ref; ref =
ref->next) {
+ for (ref = refs; ref; ref = ref->next) {
if (!starts_with(ref->name, "refs/tags/"))
continue;
@@ -338,7 +338,8 @@ static void find_non_local_tags(struct transport *transport,
string_list_clear(_refs, 0);
}
-static struct ref *get_ref_map(struct transport *transport,
+static struct ref *get_ref_map(struct remote *remote,
+ const struct ref *remote_refs,
struct refspec *rs,
int tags, int *autotags)
{
@@ -346,27 +347,11 @@ static struct ref *get_ref_map(struct transport
*transport,
struct ref *rm;
struct ref *ref_map = NULL;
struct ref **tail = _map;
- struct argv_array ref_prefixes = ARGV_ARRAY_INIT;
/* opportunistically-updated references: */
struct ref *orefs = NULL, **oref_tail =
struct string_list existing_refs = STRING_LIST_INIT_DUP;
- const struct ref *remote_refs;
-
- 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);
-
- argv_array_clear(_prefixes);
if (rs->nr) {
struct refspec *fetch_refspec;
@@ -403,7 +388,7 @@ static struct ref *get_ref_map(struct transport *transport,
if (refmap.nr)
fetch_refspec =
else
- fetch_refspec = >remote->fetch;
+ fetch_refspec = >fetch;
for (i = 0; i < fetch_refspec->nr; i++)
get_fetch_map(ref_map, _refspec->items[i],
_tail, 1);
@@ -411,7 +396,6 @@ static struct ref *get_ref_map(struct transport *transport,
die("--refmap option is only meaningful with command-line
refspec(s).");
} else {
/* Use the defaults */
- struct remote *remote = transport->remote;
struct branch *branch = branch_get(NULL);
int has_merge = branch_has_merge_config(branch);
if (remote &&
@@ -450,7 +434,7 @@ static struct ref *get_ref_map(struct transport *transport,
/* also fetch all tags */
get_fetch_map(remote_refs, tag_refspec, , 0);
else if (tags == TAGS_DEFAULT && *autotags)
- find_non_local_tags(transport, _map, );
+ find_non_local_tags(remote_refs, _map, );
/* Now append any refs to be updated opportunistically: */
*tail = orefs;
@@ -1137,6 +1121,8 @@ static int do_fetch(struct transport *transport,
struct ref *ref_map;
int autotags = (transport->remote->fetch_tags == 1);
int retcode = 0;
+ const struct ref *remote_refs;
+ struct argv_array ref_prefixes = ARGV_ARRAY_INIT;
if (tags == TAGS_DEFAULT) {
if (transport->remote->fetch_tags == 2)
@@ -1152,7 +1138,21 @@ static int do_fetch(struct transport *transport,
goto cleanup;
}
- ref_map = get_ref_map(transport, rs, tags, );
+ if (rs->nr)
+ r
[PATCH 3/8] upload-pack: test negotiation with changing repository
Add tests to check the behavior of fetching from a repository which
changes between rounds of negotiation (for example, when different
servers in a load-balancing agreement participate in the same stateless
RPC negotiation). This forms a baseline of comparison to the ref-in-want
functionality (which will be introduced to the client in subsequent
commits), and ensures that subsequent commits do not change existing
behavior.
As part of this effort, a mechanism to substitute strings in a single
HTTP response is added.
Signed-off-by: Brandon Williams
---
t/lib-httpd.sh | 1 +
t/lib-httpd/apache.conf| 8 +++
t/lib-httpd/one-time-sed.sh| 16 ++
t/t5703-upload-pack-ref-in-want.sh | 92 ++
4 files changed, 117 insertions(+)
create mode 100644 t/lib-httpd/one-time-sed.sh
diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index 435a37465..84f8efdd4 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -132,6 +132,7 @@ prepare_httpd() {
cp "$TEST_PATH"/passwd "$HTTPD_ROOT_PATH"
install_script broken-smart-http.sh
install_script error.sh
+ install_script one-time-sed.sh
ln -s "$LIB_HTTPD_MODULE_PATH" "$HTTPD_ROOT_PATH/modules"
diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index 724d9ae46..fe68d37bb 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -111,9 +111,14 @@ Alias /auth/dumb/ www/auth/dumb/
SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
SetEnv GIT_HTTP_EXPORT_ALL
+
+ SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
+ SetEnv GIT_HTTP_EXPORT_ALL
+
ScriptAliasMatch /smart_*[^/]*/(.*) ${GIT_EXEC_PATH}/git-http-backend/$1
ScriptAlias /broken_smart/ broken-smart-http.sh/
ScriptAlias /error/ error.sh/
+ScriptAliasMatch /one_time_sed/(.*) one-time-sed.sh/$1
Options FollowSymlinks
@@ -123,6 +128,9 @@ ScriptAlias /error/ error.sh/
Options ExecCGI
+
+ Options ExecCGI
+
Options ExecCGI
diff --git a/t/lib-httpd/one-time-sed.sh b/t/lib-httpd/one-time-sed.sh
new file mode 100644
index 0..a9c4aa5f4
--- /dev/null
+++ b/t/lib-httpd/one-time-sed.sh
@@ -0,0 +1,16 @@
+#!/bin/sh
+
+if [ -e one-time-sed ]; then
+ "$GIT_EXEC_PATH/git-http-backend" >out
+
+ sed "$(cat one-time-sed)" out_modified
+
+ if diff out out_modified >/dev/null; then
+ cat out
+ else
+ cat out_modified
+ rm one-time-sed
+ fi
+else
+ "$GIT_EXEC_PATH/git-http-backend"
+fi
diff --git a/t/t5703-upload-pack-ref-in-want.sh
b/t/t5703-upload-pack-ref-in-want.sh
index 0ef182970..979ab6d03 100755
--- a/t/t5703-upload-pack-ref-in-want.sh
+++ b/t/t5703-upload-pack-ref-in-want.sh
@@ -150,4 +150,96 @@ test_expect_success 'want-ref with ref we already have
commit for' '
check_output
'
+. "$TEST_DIRECTORY"/lib-httpd.sh
+start_httpd
+
+REPO="$HTTPD_DOCUMENT_ROOT_PATH/repo"
+LOCAL_PRISTINE="$(pwd)/local_pristine"
+
+test_expect_success 'setup repos for change-while-negotiating test' '
+ (
+ git init "$REPO" &&
+ cd "$REPO" &&
+ >.git/git-daemon-export-ok &&
+ test_commit m1 &&
+ git tag -d m1 &&
+
+ # Local repo with many commits (so that negotiation will take
+ # more than 1 request/response pair)
+ git clone "http://127.0.0.1:$LIB_HTTPD_PORT/smart/repo;
"$LOCAL_PRISTINE" &&
+ cd "$LOCAL_PRISTINE" &&
+ git checkout -b side &&
+ for i in $(seq 1 33); do test_commit s$i; done &&
+
+ # Add novel commits to upstream
+ git checkout master &&
+ cd "$REPO" &&
+ test_commit m2 &&
+ test_commit m3 &&
+ git tag -d m2 m3
+ ) &&
+ git -C "$LOCAL_PRISTINE" remote set-url origin
"http://127.0.0.1:$LIB_HTTPD_PORT/one_time_sed/repo; &&
+ git -C "$LOCAL_PRISTINE" config protocol.version 2
+'
+
+inconsistency() {
+ # Simulate that the server initially reports $2 as the ref
+ # corresponding to $1, and after that, $1 as the ref corresponding to
+ # $1. This corresponds to the real-life situation where the server's
+ # repository appears to change during negotiation, for example, when
+ # different servers in a load-balancing arrangement serve (stateless)
+ # RPCs during a single negotiation.
+ printf "s/%s/%s/" \
+ $(git -C "$REPO" rev-parse $1 | tr -d "\n") \
+ $(git -C "$REPO" rev-parse $2 | tr -d "\n") \
+
[PATCH 4/8] fetch: refactor the population of peer ref OIDs
Populate peer ref OIDs in get_ref_map instead of do_fetch. Besides
tightening scopes of variables in the code, this also prepares for
get_ref_map being able to be called multiple times within do_fetch.
Signed-off-by: Brandon Williams
---
builtin/fetch.c | 36 ++--
1 file changed, 18 insertions(+), 18 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index ea5b9669a..545635448 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -351,6 +351,7 @@ static struct ref *get_ref_map(struct transport *transport,
/* opportunistically-updated references: */
struct ref *orefs = NULL, **oref_tail =
+ struct string_list existing_refs = STRING_LIST_INIT_DUP;
const struct ref *remote_refs;
if (rs->nr)
@@ -458,7 +459,23 @@ static struct ref *get_ref_map(struct transport *transport,
tail = >next;
}
- return ref_remove_duplicates(ref_map);
+ ref_map = ref_remove_duplicates(ref_map);
+
+ for_each_ref(add_existing, _refs);
+ for (rm = ref_map; rm; rm = rm->next) {
+ if (rm->peer_ref) {
+ struct string_list_item *peer_item =
+ string_list_lookup(_refs,
+ rm->peer_ref->name);
+ if (peer_item) {
+ struct object_id *old_oid = peer_item->util;
+ oidcpy(>peer_ref->old_oid, old_oid);
+ }
+ }
+ }
+ string_list_clear(_refs, 1);
+
+ return ref_map;
}
#define STORE_REF_ERROR_OTHER 1
@@ -1110,14 +1127,10 @@ static void backfill_tags(struct transport *transport,
struct ref *ref_map)
static int do_fetch(struct transport *transport,
struct refspec *rs)
{
- struct string_list existing_refs = STRING_LIST_INIT_DUP;
struct ref *ref_map;
- struct ref *rm;
int autotags = (transport->remote->fetch_tags == 1);
int retcode = 0;
- for_each_ref(add_existing, _refs);
-
if (tags == TAGS_DEFAULT) {
if (transport->remote->fetch_tags == 2)
tags = TAGS_SET;
@@ -1136,18 +1149,6 @@ static int do_fetch(struct transport *transport,
if (!update_head_ok)
check_not_current_branch(ref_map);
- for (rm = ref_map; rm; rm = rm->next) {
- if (rm->peer_ref) {
- struct string_list_item *peer_item =
- string_list_lookup(_refs,
- rm->peer_ref->name);
- if (peer_item) {
- struct object_id *old_oid = peer_item->util;
- oidcpy(>peer_ref->old_oid, old_oid);
- }
- }
- }
-
if (tags == TAGS_DEFAULT && autotags)
transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, "1");
if (prune) {
@@ -1183,7 +1184,6 @@ static int do_fetch(struct transport *transport,
}
cleanup:
- string_list_clear(_refs, 1);
return retcode;
}
--
2.17.1.1185.g55be947832-goog
Re: [PATCH 6/6] Forbid "the_index" in dir.c and unpack-trees.c
On 06/05, Nguyễn Thái Ngọc Duy wrote:
> Use of global variables like the_index makes it very hard to follow
> the code flow, especially when it's buried deep in some minor helper
> function.
>
> We are gradually avoiding global variables, this hopefully will make
> it one step closer. The end game is, the set of "library" source files
> will have just one macro set: "LIB_CODE" (or something). This macro
> will prevent access to most (if not all) global variables in those
> files. We can't do that now, so we just prevent one thing at a time.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy
> ---
> Should I keep my trick of defining the_index to
> the_index_should_not_be_used_here? It may help somewhat when people
> accidentally use the_index.
We already have a set of index compatibility macros which this could
maybe be a part of. Though if we want to go this way I think we should
do the reverse of this and instead have GLOBAL_INDEX which must be
defined in order to have access to the global. That way new files are
already protected by this and it may be easier to reduce the number of
these defines through our codebase than to add them to every single
file.
>
> cache.h| 2 ++
> dir.c | 2 ++
> unpack-trees.c | 2 ++
> 3 files changed, 6 insertions(+)
>
> diff --git a/cache.h b/cache.h
> index 89a107a7f7..ecc96ccb0e 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -330,7 +330,9 @@ struct index_state {
> struct ewah_bitmap *fsmonitor_dirty;
> };
>
> +#ifndef NO_GLOBAL_INDEX
> extern struct index_state the_index;
> +#endif
>
> /* Name hashing */
> extern int test_lazy_init_name_hash(struct index_state *istate, int
> try_threaded);
> diff --git a/dir.c b/dir.c
> index ccf8b4975e..74d848db5a 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -8,6 +8,8 @@
> *Junio Hamano, 2005-2006
> */
> #define NO_THE_INDEX_COMPATIBILITY_MACROS
> +/* Do not use the_index. You should have access to it via function arg */
> +#define NO_GLOBAL_INDEX
> #include "cache.h"
> #include "config.h"
> #include "dir.h"
> diff --git a/unpack-trees.c b/unpack-trees.c
> index 3ace82ca27..9aebe9762b 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -1,4 +1,6 @@
> #define NO_THE_INDEX_COMPATIBILITY_MACROS
> +/* Do not use the_index here, you probably want o->src_index */
> +#define NO_GLOBAL_INDEX
> #include "cache.h"
> #include "argv-array.h"
> #include "repository.h"
> --
> 2.18.0.rc0.333.g22e6ee6cdf
>
--
Brandon Williams
Re: [PATCH] refspec: initalize `refspec_item` in `valid_fetch_refspec()`
c.h
> +++ b/refspec.h
> @@ -32,7 +32,8 @@ struct refspec {
> int fetch;
> };
>
> -void refspec_item_init(struct refspec_item *item, const char *refspec, int
> fetch);
> +int refspec_item_init(struct refspec_item *item, const char *refspec, int
> fetch);
> +void refspec_item_init_or_die(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);
>
> I.e. let's fix the bug, but with this admittedly more verbose fix we're
> left with exactly two memset() in refspec.c, one for each type of struct
> that's initialized by the API.
>
> The reason this is difficult now is because the current API conflates
> the init function with an init_or_die, which is what most callers want,
> so let's just split those concerns up. Then we're left with one init
> function that does the memset.
--
Brandon Williams
Re: [PATCH] refspec: initalize `refspec_item` in `valid_fetch_refspec()`
On 06/04, Martin Ågren wrote:
> We allocate a `struct refspec_item` on the stack without initializing
> it. In particular, its `dst` and `src` members will contain some random
> data from the stack. When we later call `refspec_item_clear()`, it will
> call `free()` on those pointers. So if the call to `parse_refspec()` did
> not assign to them, we will be freeing some random "pointers". This is
> undefined behavior.
>
> To the best of my understanding, this cannot currently be triggered by
> user-provided data. And for what it's worth, the test-suite does not
> trigger this with SANITIZE=address. It can be provoked by calling
> `valid_fetch_refspec(":*")`.
>
> Zero the struct, as is done in other users of `struct refspec_item`.
>
> Signed-off-by: Martin Ågren
> ---
> I found some time to look into this. It does not seem to be a
> user-visible bug, so not particularly critical.
Thanks for fixing this. I don't think I noticed this because at some
point in developing this series I had a memset call in parse_refspec.
I don't remember why I ended up removing it, but maybe it would have
been better to leave it in there.
>
> refspec.c | 5 -
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/refspec.c b/refspec.c
> index ada7854f7a..7dd7e361e5 100644
> --- a/refspec.c
> +++ b/refspec.c
> @@ -189,7 +189,10 @@ void refspec_clear(struct refspec *rs)
> int valid_fetch_refspec(const char *fetch_refspec_str)
> {
> struct refspec_item refspec;
> - int ret = parse_refspec(, fetch_refspec_str, REFSPEC_FETCH);
> + int ret;
> +
> + memset(, 0, sizeof(refspec));
> + ret = parse_refspec(, fetch_refspec_str, REFSPEC_FETCH);
> refspec_item_clear();
> return ret;
> }
> --
> 2.18.0.rc0.43.gb85e7bcbff
>
--
Brandon Williams
Re: [PATCH/RFC/BUG] unpack-trees.c: do not use "the_index"
On 06/02, Elijah Newren wrote: > On Fri, Jun 1, 2018 at 10:03 PM, Duy Nguyen wrote: > > On Fri, Jun 1, 2018 at 8:34 PM, Elijah Newren wrote: > >> On Fri, Jun 1, 2018 at 9:11 AM, Nguyễn Thái Ngọc Duy > >> wrote: > >>> This is more of a bug report than an actual fix because I'm not sure > >>> if "o->src_index" is always the correct answer instead of "the_index" > >>> here. But this is very similar to 7db118303a (unpack_trees: fix > >>> breakage when o->src_index != o->dst_index - 2018-04-23) and could > >>> potentially break things again... > > I'm pretty sure your patch is correct. Adding Brandon Williams to the > cc for comment since his patches came up in the analysis below... > > >> Actually, I don't think the patch will break anything in the current > >> code. Currently, all callers of unpack_trees() (even merge recursive > >> which uses different src_index and dst_index now) set src_index = > >> _index. So, these changes should continue to work as before (with > >> a minor possible exception of merge-recursive calling into other > >> functions from unpack-trees.c after unpack_trees() has finished..). > >> That's not to say that your patch is bug free, just that I think any > >> bugs shouldn't be triggerable from the current codebase. > > > > Ah.. I thought merge-recursive would do fancier things and used some > > temporary index. Good to know. > > Well, it does does use a temporary index, but for dst_index rather > than src_index. It then does some fancier things, but not until the > call to unpack_trees() is over. In particular, at that point, it > swaps the_index and tmp_index, reversing their roles so that now > tmp_index is the original index and the_index becomes the result after > unpack_trees() is run. That's done because I later want to use the > original index for calling verify_uptodate(). verify_uptodate() is > then used for was_tracked_and_matches() and was_tracked(). > > Anyway, the whole upshot of this is: > * merge-recursive uses src_index == _index for the unpack_trees() call. > * merge-recursive uses src_index == o->orig_index for subsequent > calls to verify_uptodate(), but verify_uptodate() doesn't call into > any of the sites you have modified. > > Further: > * Every other existing caller of unpack-trees in the code sets > src_index == _index, so this won't break any of them. > * There are only two callers in the codebase that set dst_index to > something other than _index -- diff-lib.c (which sets it to NULL) > and merge-recursive (which does the stuff described above). > > So, having done that analysis, I am now pretty convinced your patch > won't break anything. That's one half... > > >> Also, if any of the changes you made are wrong, what was there before > >> was also clearly wrong. So I think we're at least no worse off. > >> > >> But, I agree, it's not easy to verify what the correct thing should be > >> in all cases. I'll try to take a closer look in the next couple days. > > > > Thanks. I will also stare at this code some more in the next couple > > days trying to remember what these functions do. > > Your patch has two divisible parts: > > 1) Your modifications to > * clear_ce_flags_1() > * clear_ce_flags_dir() > * clear_ce_flags() > * mark_new_skip_worktree() > The clear_ce_flags*() functions are only called by each other and by > mark_new_skip_worktree(), which in turn is only called from > unpack_trees(). Also, in all of these, your change ends up only > modifying what index_state is passed to is_excluded_from_list(). > > 2) Your modifications to > * verify_clean_subdirectory() > * check_ok_to_remove() > In this case, the former is only called by the latter, and the latter > ends up only being called (via verify_absent_1()) from verify_absent() > and verify_absent_sparse(). > > I'll address each, in reverse order. > > 2) The stuff that affects verify_absent() > > While verify_absent() is not called from merge-recursive right now, it > was something I wanted to use in the future for very similar reasons > that verify_uptodate() started being called directly from > merge-recursive. In particular, if the rewrite of merge-recursive[A] > I want to do sets index_only when calling unpack_trees(), then does > the whole merge without touching the worktree, then at the end goes to > update the working tree, it will need to do extra checks. > verify_absent() will come in handy as one of those extra checks. For > that case, using the_index (the new index just creat
Re: [PATCH] fetch: do not pass ref-prefixes for fetch by exact SHA1
Thanks for finding this, I don't know how I missed moving that bit over when factoring it out. Well I guess I sort of rewrote it and combined two pieces of logic so that's how. Anyway, this looks right and thanks for adding the test. On Thu, May 31, 2018 at 12:23 AM, Jonathan Nieder wrote: > When v2.18.0-rc0~10^2~1 (refspec: consolidate ref-prefix generation > logic, 2018-05-16) factored out the ref-prefix generation code for > reuse, it left out the 'if (!item->exact_sha1)' test in the original > ref-prefix generation code. As a result, fetches by SHA-1 generate > ref-prefixes as though the SHA-1 being fetched were an abbreviated ref > name: > > $ GIT_TRACE_PACKET=1 bin-wrappers/git -c protocol.version=2 \ > fetch origin 12039e008f9a4e3394f3f94f8ea897785cb09448 > [...] > packet:fetch> ref-prefix 12039e008f9a4e3394f3f94f8ea897785cb09448 > packet:fetch> ref-prefix > refs/12039e008f9a4e3394f3f94f8ea897785cb09448 > packet:fetch> ref-prefix > refs/tags/12039e008f9a4e3394f3f94f8ea897785cb09448 > packet:fetch> ref-prefix > refs/heads/12039e008f9a4e3394f3f94f8ea897785cb09448 > packet:fetch> ref-prefix > refs/remotes/12039e008f9a4e3394f3f94f8ea897785cb09448 > packet:fetch> ref-prefix > refs/remotes/12039e008f9a4e3394f3f94f8ea897785cb09448/HEAD > packet:fetch> > > If there is another ref name on the command line or the object being > fetched is already available locally, then that's mostly harmless. > But otherwise, we error out with > > fatal: no matching remote head > > since the server did not send any refs we are interested in. Filter > out the exact_sha1 refspecs to avoid this. > > This patch adds a test to check this behavior that notices another > behavior difference between protocol v0 and v2 in the process. Add a > NEEDSWORK comment to clear it up. > > Signed-off-by: Jonathan Nieder > --- > Here's the change described in > https://public-inbox.org/git/20180531010739.gb36...@aiede.svl.corp.google.com/ > as a proper patch. > > Thoughts of all kinds welcome, as always. > > refspec.c | 2 ++ > refspec.h | 4 > t/t5516-fetch-push.sh | 19 +++ > 3 files changed, 25 insertions(+) > > diff --git a/refspec.c b/refspec.c > index c59a4ccf1e..ada7854f7a 100644 > --- a/refspec.c > +++ b/refspec.c > @@ -202,6 +202,8 @@ void refspec_ref_prefixes(const struct refspec *rs, > const struct refspec_item *item = >items[i]; > const char *prefix = NULL; > > + if (item->exact_sha1) > + continue; > if (rs->fetch == REFSPEC_FETCH) > prefix = item->src; > else if (item->dst) > diff --git a/refspec.h b/refspec.h > index 01b700e094..3a9363887c 100644 > --- a/refspec.h > +++ b/refspec.h > @@ -42,6 +42,10 @@ void refspec_clear(struct refspec *rs); > int valid_fetch_refspec(const char *refspec); > > struct argv_array; > +/* > + * Determine what values to pass to the peer in ref-prefix lines > + * (see Documentation/technical/protocol-v2.txt). > + */ > void refspec_ref_prefixes(const struct refspec *rs, > struct argv_array *ref_prefixes); > > diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh > index f4d28288f0..a5077d8b7c 100755 > --- a/t/t5516-fetch-push.sh > +++ b/t/t5516-fetch-push.sh > @@ -1121,6 +1121,25 @@ test_expect_success 'fetch exact SHA1' ' > ) > ' > > +test_expect_success 'fetch exact SHA1 in protocol v2' ' > + mk_test testrepo heads/master hidden/one && > + git push testrepo master:refs/hidden/one && > + git -C testrepo config transfer.hiderefs refs/hidden && > + check_push_result testrepo $the_commit hidden/one && > + > + mk_child testrepo child && > + git -C child config protocol.version 2 && > + > + # make sure $the_commit does not exist here > + git -C child repack -a -d && > + git -C child prune && > + test_must_fail git -C child cat-file -t $the_commit && > + > + # fetching the hidden object succeeds by default > + # NEEDSWORK: should this match the v0 behavior instead? > + git -C child fetch -v ../testrepo $the_commit:refs/heads/copy > +' > + > for configallowtipsha1inwant in true false > do > test_expect_success "shallow fetch reachable SHA1 (but not a ref), > allowtipsha1inwant=$configallowtipsha1inwant" ' > -- > 2.17.1.1185.g55be947832 >
Re: Is origin/HEAD only being created on clone a bug? #leftoverbits
On 05/29, Ævar Arnfjörð Bjarmason wrote: > Here's some more #leftoverbits where we have a clone/fetch feature > discrepancy and where clone is magical in ways that "fetch" isn't. > > If you make an initial commit and push to a remote repo "origin", you > don't get a remote origin/HEAD reference, and a "fetch" won't create it > either. > > You will get it if you subseuqently "clone" the repo, but not if you use > "git init / remote add / fetch / git checkout -t" which should otherwise > be equivalent. > > If you push to "master" (or whatever HEAD is) from the clone the > origin/HEAD will be updated accordingly, but from the repo you pushed > from & the one you did init+fetch instead of clone you'll never see it. > > Some code spelunking reveals remote_head_points_at, guess_remote_head() > etc. in builtin/clone.c. I.e. this is special-cased as part of the > "clone". > > Can anyone thing of a reason for why this shouldn't be fixed as a bug? > I've tried searching the archives but "origin/HEAD" comes up with too > many > results. > https://public-inbox.org/git/alpine.lsu.1.00.0803020556380.22...@racer.site/#t > seems to be the patch that initially added it, but it is not discussed > why this should be a clone-only special case that doesn't apply to > "fetch". I believe the issue has to deal with how symrefs are handled. I don't think any of the fetch code path does anything special with symrefs. Symref info does get sent over the wire for the HEAD symref but i think its just ignored. You'll see updates for origin/HEAD when you subsequently fetch most likely because its recorded as a symref locally which points at origin/master. This means that when you fetch origin/master, origin/HEAD will also but updated just because its locally a pointer to origin/master. With that said, yes we should probably fix this issue with fetch because I added symref support to protocol v2 so now symref information for refs other than HEAD can be sent across the wire but the client just throws that info away at the moment. -- Brandon Williams
[PATCH] config: document value 2 for protocol.version
Update the config documentation to note the value `2` as an acceptable value for the protocol.version config. Signed-off-by: Brandon Williams <bmw...@google.com> --- Documentation/config.txt | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Documentation/config.txt b/Documentation/config.txt index a05a88fec..ce778883d 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -2637,6 +2637,8 @@ protocol.version:: * `1` - the original wire protocol with the addition of a version string in the initial response from the server. +* `2` - wire protocol version 2. + -- pull.ff:: -- 2.17.0.441.gb46fe60e1d-goog
[PATCH v2 1/2] remote-curl: accept all encodings supported by curl
Configure curl to accept all encodings which curl supports instead of
only accepting gzip responses.
This fixes an issue when using an installation of curl which is built
without the "zlib" feature. Since aa90b9697 (Enable info/refs gzip
decompression in HTTP client, 2012-09-19) we end up requesting "gzip"
encoding anyway despite libcurl not being able to decode it. Worse,
instead of getting a clear error message indicating so, we end up
falling back to "dumb" http, producing a confusing and difficult to
debug result.
Since curl doesn't do any checking to verify that it supports the a
requested encoding, instead set the curl option `CURLOPT_ENCODING` with
an empty string indicating that curl should send an "Accept-Encoding"
header containing only the encodings supported by curl.
Reported-by: Anton Golubev <anton.golu...@gmail.com>
Signed-off-by: Brandon Williams <bmw...@google.com>
---
Version 2 of this series just tweaks the commit message and the test per
Jonathan's suggestion.
http.c | 2 +-
remote-curl.c | 2 +-
t/t5551-http-fetch-smart.sh | 13 +
3 files changed, 11 insertions(+), 6 deletions(-)
diff --git a/http.c b/http.c
index fed13b216..709150fc7 100644
--- a/http.c
+++ b/http.c
@@ -1788,7 +1788,7 @@ static int http_request(const char *url,
curl_easy_setopt(slot->curl, CURLOPT_URL, url);
curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);
- curl_easy_setopt(slot->curl, CURLOPT_ENCODING, "gzip");
+ curl_easy_setopt(slot->curl, CURLOPT_ENCODING, "");
ret = run_one_slot(slot, );
diff --git a/remote-curl.c b/remote-curl.c
index ceb05347b..565bba104 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -684,7 +684,7 @@ static int post_rpc(struct rpc_state *rpc)
curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 0);
curl_easy_setopt(slot->curl, CURLOPT_POST, 1);
curl_easy_setopt(slot->curl, CURLOPT_URL, rpc->service_url);
- curl_easy_setopt(slot->curl, CURLOPT_ENCODING, "gzip");
+ curl_easy_setopt(slot->curl, CURLOPT_ENCODING, "");
if (large_request) {
/* The request body is large and the size cannot be predicted.
diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index f5721b4a5..913089b14 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -26,14 +26,14 @@ setup_askpass_helper
cat >exp < GET /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1
> Accept: */*
-> Accept-Encoding: gzip
+> Accept-Encoding: ENCODINGS
> Pragma: no-cache
< HTTP/1.1 200 OK
< Pragma: no-cache
< Cache-Control: no-cache, max-age=0, must-revalidate
< Content-Type: application/x-git-upload-pack-advertisement
> POST /smart/repo.git/git-upload-pack HTTP/1.1
-> Accept-Encoding: gzip
+> Accept-Encoding: ENCODINGS
> Content-Type: application/x-git-upload-pack-request
> Accept: application/x-git-upload-pack-result
> Content-Length: xxx
@@ -79,8 +79,13 @@ test_expect_success 'clone http repository' '
/^< Date: /d
/^< Content-Length: /d
/^< Transfer-Encoding: /d
- " >act &&
- test_cmp exp act
+ " >actual &&
+ sed -e "s/^> Accept-Encoding: .*/> Accept-Encoding: ENCODINGS/" \
+ actual >actual.smudged &&
+ test_cmp exp actual.smudged &&
+
+ grep "Accept-Encoding:.*gzip" actual >actual.gzip &&
+ test_line_count = 2 actual.gzip
'
test_expect_success 'fetch changes via http' '
--
2.17.0.441.gb46fe60e1d-goog
[PATCH v2 2/2] remote-curl: accept compressed responses with protocol v2
Configure curl to accept compressed responses when using protocol v2 by setting `CURLOPT_ENCODING` to "", which indicates that curl should send an "Accept-Encoding" header with all supported compression encodings. Signed-off-by: Brandon Williams <bmw...@google.com> --- remote-curl.c | 1 + 1 file changed, 1 insertion(+) diff --git a/remote-curl.c b/remote-curl.c index 565bba104..99b0bedc6 100644 --- a/remote-curl.c +++ b/remote-curl.c @@ -1259,6 +1259,7 @@ static int proxy_request(struct proxy_state *p) slot = get_active_slot(); + curl_easy_setopt(slot->curl, CURLOPT_ENCODING, ""); curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 0); curl_easy_setopt(slot->curl, CURLOPT_POST, 1); curl_easy_setopt(slot->curl, CURLOPT_URL, p->service_url); -- 2.17.0.441.gb46fe60e1d-goog
Re: [PATCH 1/2] remote-curl: accept all encoding supported by curl
On 05/22, Daniel Stenberg wrote: > On Mon, 21 May 2018, Jonathan Nieder wrote: > > > > Looking at the code here, this succeeds if enough memory is available. > > > There is no check if the given parameter is part of > > > Curl_all_content_encodings(); > > > > By "this" are you referring to the preimage or the postimage? Are you > > suggesting a change in git or in libcurl? > > > > Curl_all_content_encodings() is an internal function in libcurl, so I'm > > assuming the latter. > > Ack, that certainly isn't the most wonderful API for selecting a compression > method. In reality, almost everyone sticks to passing on a "" to that option > to let libcurl pick and ask for the compression algos it knows since both > gzip and brotli are present only conditionally depending on build options. Thanks for the clarification. Sounds like the best option is to continue with this patch and let curl decide using "". > > I would agree that the libcurl setopt call should probably be made to fail > if asked to use a compression method not built-in/supported. Then an > application could in fact try different algos in order until one works or > ask to disable compression completely. > > In the generic HTTP case, it usually makes sense to ask for more than one > algorthim though, since this is asking the server for a compressed version > and typically a HTTP client doesn't know which compression methods the > server offers. Not sure this is actually true to the same extent for git. > > -- > > / daniel.haxx.se -- Brandon Williams
[PATCH 2/2] remote-curl: accept compressed responses with protocol v2
Configure curl to accept compressed responses when using protocol v2 by setting `CURLOPT_ENCODING` to "", which indicates that curl should send an "Accept-Encoding" header with all supported compression encodings. Signed-off-by: Brandon Williams <bmw...@google.com> --- remote-curl.c | 1 + 1 file changed, 1 insertion(+) diff --git a/remote-curl.c b/remote-curl.c index 565bba104..99b0bedc6 100644 --- a/remote-curl.c +++ b/remote-curl.c @@ -1259,6 +1259,7 @@ static int proxy_request(struct proxy_state *p) slot = get_active_slot(); + curl_easy_setopt(slot->curl, CURLOPT_ENCODING, ""); curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 0); curl_easy_setopt(slot->curl, CURLOPT_POST, 1); curl_easy_setopt(slot->curl, CURLOPT_URL, p->service_url); -- 2.17.0.441.gb46fe60e1d-goog
[PATCH 1/2] remote-curl: accept all encoding supported by curl
Configure curl to accept all encoding which curl supports instead of
only accepting gzip responses.
This is necessary to fix a bug when using an installation of curl which
doesn't support gzip. Since curl doesn't do any checking to verify that
it supports the encoding set when calling 'curl_easy_setopt()', curl can
end up sending an "Accept-Encoding" header indicating that it supports
a particular encoding when in fact it doesn't. Instead when the empty
string "" is used when setting `CURLOPT_ENCODING`, curl will send an
"Accept-Encoding" header containing only the encoding methods curl
supports.
Signed-off-by: Brandon Williams <bmw...@google.com>
---
http.c | 2 +-
remote-curl.c | 2 +-
t/t5551-http-fetch-smart.sh | 4 ++--
3 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/http.c b/http.c
index fed13b216..709150fc7 100644
--- a/http.c
+++ b/http.c
@@ -1788,7 +1788,7 @@ static int http_request(const char *url,
curl_easy_setopt(slot->curl, CURLOPT_URL, url);
curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);
- curl_easy_setopt(slot->curl, CURLOPT_ENCODING, "gzip");
+ curl_easy_setopt(slot->curl, CURLOPT_ENCODING, "");
ret = run_one_slot(slot, );
diff --git a/remote-curl.c b/remote-curl.c
index ceb05347b..565bba104 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -684,7 +684,7 @@ static int post_rpc(struct rpc_state *rpc)
curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 0);
curl_easy_setopt(slot->curl, CURLOPT_POST, 1);
curl_easy_setopt(slot->curl, CURLOPT_URL, rpc->service_url);
- curl_easy_setopt(slot->curl, CURLOPT_ENCODING, "gzip");
+ curl_easy_setopt(slot->curl, CURLOPT_ENCODING, "");
if (large_request) {
/* The request body is large and the size cannot be predicted.
diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index f5721b4a5..39c65482c 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -26,14 +26,14 @@ setup_askpass_helper
cat >exp < GET /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1
> Accept: */*
-> Accept-Encoding: gzip
+> Accept-Encoding: deflate, gzip
> Pragma: no-cache
< HTTP/1.1 200 OK
< Pragma: no-cache
< Cache-Control: no-cache, max-age=0, must-revalidate
< Content-Type: application/x-git-upload-pack-advertisement
> POST /smart/repo.git/git-upload-pack HTTP/1.1
-> Accept-Encoding: gzip
+> Accept-Encoding: deflate, gzip
> Content-Type: application/x-git-upload-pack-request
> Accept: application/x-git-upload-pack-result
> Content-Length: xxx
--
2.17.0.441.gb46fe60e1d-goog
Re: [PATCH v2 02/18] Add a new builtin: branch-diff
On 05/21, Jeff King wrote: > On Mon, May 21, 2018 at 10:56:47AM -0700, Stefan Beller wrote: > > > > It is very much about > > > comparing two *ranges of* revisions, and not just any ranges, no. Those > > > ranges need to be so related as to contain mostly identical changes. > > > > range-diff, eh? > > > > > Most fellow German software engineers (who seem to have a knack for > > > idiotically long variable/function names) would now probably suggest: > > > > > > git compare-patch-series-with-revised-patch-series > > > > or short: > > > > revision-compare > > compare-revs > > com-revs > > > > revised-diff > > revise-diff > > revised-compare > > > > diff-revise I haven't really been following all of the discussion but from what I can tell the point of this command is to generate a diff based on two different versions of a series, so why not call it 'series-diff'? :) -- Brandon Williams
Re: [PATCH 02/11] repository: introduce repo_read_index_or_die
On 05/21, Stefan Beller wrote:
> On Fri, May 18, 2018 at 11:37 PM, Duy Nguyen <pclo...@gmail.com> wrote:
> > On Wed, May 16, 2018 at 03:21:09PM -0700, Stefan Beller wrote:
> >> 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 <sbel...@google.com>
> >> ---
> >> 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 +--
> >
> > 'git grep -w -A3 read_cache' tells me you're missing (*)
>
> > (*) yes yes you did mention "if the error message is exactly the
> > same". But these look like good candicates to convert anyway
> >
> > builtin/diff-tree.c:if (read_cache() < 0)
> > builtin/diff-tree.c-die(_("index file corrupt"));
> >
>
> I would expect this to be covered in a follow up patch as I did look
> for (read_cache() < 0) specifically.
>
> > builtin/merge-ours.c: if (read_cache() < 0)
> > builtin/merge-ours.c: die_errno("read_cache failed");
> >
> > builtin/update-index.c: entries = read_cache();
> > builtin/update-index.c- if (entries < 0)
> > builtin/update-index.c- die("cache corrupted");
> >
> > merge-ours.c is interesting because it uses die_errno() version
> > instead. I think that might give inaccurate diagnostics because we do
> > not only fail when a libc/system call fails in read_cache(), so it
> > should be safe to just use die() here.
> >
> > update-index.c is also interesting because it actually saves the
> > return value. I don't think it's used anywhere, so you can just drop
> > that 'entries' variable. But perhaps it's good to make
> > repo_read_index_or_die() return the number of entries too.
>
> Yeah this series left out all the interesting cases, as I just sent it out
> without much thought.
>
> Returning the number of entries makes sense.
>
> One of the reviewers of the series questioned the overall goal of the
> series as we want to move away from _die() in top level code but this
> series moves more towards it.
I've heard every once in a while that we want to move toward this but I
don't believe there is an actual effort along those lines just yet. For
that to be the case we would need a clearly defined error handling
methodology (aside from the existing "die"ing behavior), which we don't
currently have.
>
> I don't know.
>
> Stefan
--
Brandon Williams
[PATCH 1/2] refspec: consolidate ref-prefix generation logic
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 <bmw...@google.com>
---
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_
[PATCH 2/2] fetch: generate ref-prefixes when using a configured refspec
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 <bmw...@google.com>
---
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
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
[PATCH v2 08/36] transport: convert transport_push to use struct refspec
Convert the logic in 'transport_push()' which calculates a list of
ref-prefixes to use 'struct refspec'.
Signed-off-by: Brandon Williams <bmw...@google.com>
---
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
Convert 'prune_refs()' to take a 'struct refspec' as a parameter instead
of a list of 'struct refspec_item'.
Signed-off-by: Brandon Williams <bmw...@google.com>
---
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
Convert send-pack.c to store refspecs in a 'struct refspec' instead of
as an array of 'const char *'.
Signed-off-by: Brandon Williams <bmw...@google.com>
---
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
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 <bmw...@google.com>
---
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, f
[PATCH v2 23/36] remote: convert get_stale_heads to take a struct refspec
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 <bmw...@google.com>
---
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
Convert 'push_unpushed_submodules()' to take a 'struct refspec' as a
parameter instead of an array of 'const char *'.
Signed-off-by: Brandon Williams <bmw...@google.com>
---
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-&g
[PATCH v2 33/36] http-push: store refspecs in a struct refspec
Convert http-push.c to store refspecs in a 'struct refspec' instead of
in an array of 'const char *'.
Signed-off-by: Brandon Williams <bmw...@google.com>
---
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
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 <bmw...@google.com>
---
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
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 <bmw...@google.com>
---
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
Convert 'match_push_refs()' to take a 'struct refspec' as a parameter
instead of an array of 'const char *'.
Signed-off-by: Brandon Williams <bmw...@google.com>
---
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 *
[PATCH v2 35/36] remote: convert check_push_refs to take a struct refspec
Convert 'check_push_refs()' to take a 'struct refspec' as a parameter
instead of an array of 'const char *'.
Signed-off-by: Brandon Williams <bmw...@google.com>
---
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
Convert 'query_refspecs()' to take a 'struct refspec' as a parameter instead
of a list of 'struct refspec_item'.
Signed-off-by: Brandon Williams <bmw...@google.com>
---
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
Convert 'fetch_one()' to use 'struct refspec'.
Signed-off-by: Brandon Williams <bmw...@google.com>
---
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
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 <bmw...@google.com>
---
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
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 <bmw...@google.com>
---
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
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 <bmw...@google.com>
---
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
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 <bmw...@google.com>
---
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
