* [PATCH 1/2] object-store.h: move for_each_alternate_ref() from transport.h
2019-07-01 13:17 ` Jeff King
@ 2019-07-01 13:17 ` Jeff King
2019-07-01 13:23 ` Derrick Stolee
2019-07-01 13:18 ` [PATCH v2 2/2] check_everything_connected: assume alternate ref tips are valid Jeff King
2019-07-01 17:02 ` [PATCH] " Taylor Blau
2 siblings, 1 reply; 20+ messages in thread
From: Jeff King @ 2019-07-01 13:17 UTC (permalink / raw)
To: Derrick Stolee; +Cc: Junio C Hamano, git, Taylor Blau
There's nothing inherently transport-related about enumerating the
alternate ref tips. The code has lived in transport.[ch] because the
only use so far had been advertising available tips during transport.
But it could be used for more, and a future patch will teach rev-list to
access these refs.
Let's move it alongside the other alt-odb code, declaring it in
object-store.h with the implementation in sha1-file.c.
This lets us drop the inclusion of transport.h from receive-pack, which
perhaps shows how it was misplaced (though receive-pack is about
transporting objects, transport.h is mostly about the client side).
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/receive-pack.c | 1 -
object-store.h | 2 +
sha1-file.c | 97 ++++++++++++++++++++++++++++++++++++++++++
transport.c | 97 ------------------------------------------
transport.h | 2 -
5 files changed, 99 insertions(+), 100 deletions(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 77b7122456..557e686d5d 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -12,7 +12,6 @@
#include "object.h"
#include "remote.h"
#include "connect.h"
-#include "transport.h"
#include "string-list.h"
#include "sha1-array.h"
#include "connected.h"
diff --git a/object-store.h b/object-store.h
index 49f56ab8d9..7f7b3cdd80 100644
--- a/object-store.h
+++ b/object-store.h
@@ -33,6 +33,8 @@ void prepare_alt_odb(struct repository *r);
char *compute_alternate_path(const char *path, struct strbuf *err);
typedef int alt_odb_fn(struct object_directory *, void *);
int foreach_alt_odb(alt_odb_fn, void*);
+typedef void alternate_ref_fn(const struct object_id *oid, void *);
+void for_each_alternate_ref(alternate_ref_fn, void *);
/*
* Add the directory to the on-disk alternates file; the new entry will also
diff --git a/sha1-file.c b/sha1-file.c
index 888b6024d5..3c273c076a 100644
--- a/sha1-file.c
+++ b/sha1-file.c
@@ -743,6 +743,103 @@ char *compute_alternate_path(const char *path, struct strbuf *err)
return ref_git;
}
+static void fill_alternate_refs_command(struct child_process *cmd,
+ const char *repo_path)
+{
+ const char *value;
+
+ if (!git_config_get_value("core.alternateRefsCommand", &value)) {
+ cmd->use_shell = 1;
+
+ argv_array_push(&cmd->args, value);
+ argv_array_push(&cmd->args, repo_path);
+ } else {
+ cmd->git_cmd = 1;
+
+ argv_array_pushf(&cmd->args, "--git-dir=%s", repo_path);
+ argv_array_push(&cmd->args, "for-each-ref");
+ argv_array_push(&cmd->args, "--format=%(objectname)");
+
+ if (!git_config_get_value("core.alternateRefsPrefixes", &value)) {
+ argv_array_push(&cmd->args, "--");
+ argv_array_split(&cmd->args, value);
+ }
+ }
+
+ cmd->env = local_repo_env;
+ cmd->out = -1;
+}
+
+static void read_alternate_refs(const char *path,
+ alternate_ref_fn *cb,
+ void *data)
+{
+ struct child_process cmd = CHILD_PROCESS_INIT;
+ struct strbuf line = STRBUF_INIT;
+ FILE *fh;
+
+ fill_alternate_refs_command(&cmd, path);
+
+ if (start_command(&cmd))
+ return;
+
+ fh = xfdopen(cmd.out, "r");
+ while (strbuf_getline_lf(&line, fh) != EOF) {
+ struct object_id oid;
+ const char *p;
+
+ if (parse_oid_hex(line.buf, &oid, &p) || *p) {
+ warning(_("invalid line while parsing alternate refs: %s"),
+ line.buf);
+ break;
+ }
+
+ cb(&oid, data);
+ }
+
+ fclose(fh);
+ finish_command(&cmd);
+}
+
+struct alternate_refs_data {
+ alternate_ref_fn *fn;
+ void *data;
+};
+
+static int refs_from_alternate_cb(struct object_directory *e,
+ void *data)
+{
+ struct strbuf path = STRBUF_INIT;
+ size_t base_len;
+ struct alternate_refs_data *cb = data;
+
+ if (!strbuf_realpath(&path, e->path, 0))
+ goto out;
+ if (!strbuf_strip_suffix(&path, "/objects"))
+ goto out;
+ base_len = path.len;
+
+ /* Is this a git repository with refs? */
+ strbuf_addstr(&path, "/refs");
+ if (!is_directory(path.buf))
+ goto out;
+ strbuf_setlen(&path, base_len);
+
+ read_alternate_refs(path.buf, cb->fn, cb->data);
+
+out:
+ strbuf_release(&path);
+ return 0;
+}
+
+void for_each_alternate_ref(alternate_ref_fn fn, void *data)
+{
+ struct alternate_refs_data cb;
+ cb.fn = fn;
+ cb.data = data;
+ foreach_alt_odb(refs_from_alternate_cb, &cb);
+}
+
int foreach_alt_odb(alt_odb_fn fn, void *cb)
{
struct object_directory *ent;
diff --git a/transport.c b/transport.c
index f1fcd2c4b0..2def5a0c35 100644
--- a/transport.c
+++ b/transport.c
@@ -1380,100 +1380,3 @@ char *transport_anonymize_url(const char *url)
literal_copy:
return xstrdup(url);
}
-
-static void fill_alternate_refs_command(struct child_process *cmd,
- const char *repo_path)
-{
- const char *value;
-
- if (!git_config_get_value("core.alternateRefsCommand", &value)) {
- cmd->use_shell = 1;
-
- argv_array_push(&cmd->args, value);
- argv_array_push(&cmd->args, repo_path);
- } else {
- cmd->git_cmd = 1;
-
- argv_array_pushf(&cmd->args, "--git-dir=%s", repo_path);
- argv_array_push(&cmd->args, "for-each-ref");
- argv_array_push(&cmd->args, "--format=%(objectname)");
-
- if (!git_config_get_value("core.alternateRefsPrefixes", &value)) {
- argv_array_push(&cmd->args, "--");
- argv_array_split(&cmd->args, value);
- }
- }
-
- cmd->env = local_repo_env;
- cmd->out = -1;
-}
-
-static void read_alternate_refs(const char *path,
- alternate_ref_fn *cb,
- void *data)
-{
- struct child_process cmd = CHILD_PROCESS_INIT;
- struct strbuf line = STRBUF_INIT;
- FILE *fh;
-
- fill_alternate_refs_command(&cmd, path);
-
- if (start_command(&cmd))
- return;
-
- fh = xfdopen(cmd.out, "r");
- while (strbuf_getline_lf(&line, fh) != EOF) {
- struct object_id oid;
- const char *p;
-
- if (parse_oid_hex(line.buf, &oid, &p) || *p) {
- warning(_("invalid line while parsing alternate refs: %s"),
- line.buf);
- break;
- }
-
- cb(&oid, data);
- }
-
- fclose(fh);
- finish_command(&cmd);
-}
-
-struct alternate_refs_data {
- alternate_ref_fn *fn;
- void *data;
-};
-
-static int refs_from_alternate_cb(struct object_directory *e,
- void *data)
-{
- struct strbuf path = STRBUF_INIT;
- size_t base_len;
- struct alternate_refs_data *cb = data;
-
- if (!strbuf_realpath(&path, e->path, 0))
- goto out;
- if (!strbuf_strip_suffix(&path, "/objects"))
- goto out;
- base_len = path.len;
-
- /* Is this a git repository with refs? */
- strbuf_addstr(&path, "/refs");
- if (!is_directory(path.buf))
- goto out;
- strbuf_setlen(&path, base_len);
-
- read_alternate_refs(path.buf, cb->fn, cb->data);
-
-out:
- strbuf_release(&path);
- return 0;
-}
-
-void for_each_alternate_ref(alternate_ref_fn fn, void *data)
-{
- struct alternate_refs_data cb;
- cb.fn = fn;
- cb.data = data;
- foreach_alt_odb(refs_from_alternate_cb, &cb);
-}
diff --git a/transport.h b/transport.h
index 06e06d3d89..0b5f7806f6 100644
--- a/transport.h
+++ b/transport.h
@@ -262,6 +262,4 @@ int transport_refs_pushed(struct ref *ref);
void transport_print_push_status(const char *dest, struct ref *refs,
int verbose, int porcelain, unsigned int *reject_reasons);
-typedef void alternate_ref_fn(const struct object_id *oid, void *);
-void for_each_alternate_ref(alternate_ref_fn, void *);
#endif
--
2.22.0.776.g16867c022c
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 1/2] object-store.h: move for_each_alternate_ref() from transport.h
2019-07-01 13:17 ` [PATCH 1/2] object-store.h: move for_each_alternate_ref() from transport.h Jeff King
@ 2019-07-01 13:23 ` Derrick Stolee
0 siblings, 0 replies; 20+ messages in thread
From: Derrick Stolee @ 2019-07-01 13:23 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git, Taylor Blau
On 7/1/2019 9:17 AM, Jeff King wrote:
> There's nothing inherently transport-related about enumerating the
> alternate ref tips. The code has lived in transport.[ch] because the
> only use so far had been advertising available tips during transport.
> But it could be used for more, and a future patch will teach rev-list to
> access these refs.
>
> Let's move it alongside the other alt-odb code, declaring it in
> object-store.h with the implementation in sha1-file.c.
Thanks!
> This lets us drop the inclusion of transport.h from receive-pack, which
> perhaps shows how it was misplaced (though receive-pack is about
> transporting objects, transport.h is mostly about the client side).
That's an interesting de-coupling. Thanks for looking into reorganizing
the code. This series looks good to me.
-Stolee
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v2 2/2] check_everything_connected: assume alternate ref tips are valid
2019-07-01 13:17 ` Jeff King
2019-07-01 13:17 ` [PATCH 1/2] object-store.h: move for_each_alternate_ref() from transport.h Jeff King
@ 2019-07-01 13:18 ` Jeff King
2019-07-03 9:12 ` SZEDER Gábor
2019-07-01 17:02 ` [PATCH] " Taylor Blau
2 siblings, 1 reply; 20+ messages in thread
From: Jeff King @ 2019-07-01 13:18 UTC (permalink / raw)
To: Derrick Stolee; +Cc: Junio C Hamano, git, Taylor Blau
When we receive a remote ref update to sha1 "X", we want to check that
we have all of the objects needed by "X". We can assume that our
repository is not currently corrupted, and therefore if we have a ref
pointing at "Y", we have all of its objects. So we can stop our
traversal from "X" as soon as we hit "Y".
If we make the same non-corruption assumption about any repositories we
use to store alternates, then we can also use their ref tips to shorten
the traversal.
This is especially useful when cloning with "--reference", as we
otherwise do not have any local refs to check against, and have to
traverse the whole history, even though the other side may have sent us
few or no objects. Here are results for the included perf test (which
shows off more or less the maximal savings, getting one new commit and
sharing the whole history):
Test HEAD^ HEAD
--------------------------------------------------------------------
[on git.git]
5600.3: clone --reference 2.94(2.86+0.08) 0.09(0.08+0.01) -96.9%
[on linux.git]
5600.3: clone --reference 45.74(45.34+0.41) 0.36(0.30+0.08) -99.2%
Signed-off-by: Jeff King <peff@peff.net>
---
Identical to v1 except for dropping the transport.h include in
revision.c.
Documentation/rev-list-options.txt | 8 ++++
connected.c | 1 +
revision.c | 29 +++++++++++++++
t/perf/p5600-clone-reference.sh | 27 ++++++++++++++
t/t5618-alternate-refs.sh | 60 ++++++++++++++++++++++++++++++
5 files changed, 125 insertions(+)
create mode 100755 t/perf/p5600-clone-reference.sh
create mode 100755 t/t5618-alternate-refs.sh
diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index 71a1fcc093..90a2c027ea 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -182,6 +182,14 @@ explicitly.
Pretend as if all objects mentioned by reflogs are listed on the
command line as `<commit>`.
+--alternate-refs::
+ Pretend as if all objects mentioned as ref tips of alternate
+ repositories were listed on the command line. An alternate
+ repository is any repository whose object directory is specified
+ in `objects/info/alternates`. The set of included objects may
+ be modified by `core.alternateRefsCommand`, etc. See
+ linkgit:git-config[1].
+
--single-worktree::
By default, all working trees will be examined by the
following options when there are more than one (see
diff --git a/connected.c b/connected.c
index 1ab481fed6..cd9b324afa 100644
--- a/connected.c
+++ b/connected.c
@@ -80,6 +80,7 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
argv_array_push(&rev_list.args, "--all");
}
argv_array_push(&rev_list.args, "--quiet");
+ argv_array_push(&rev_list.args, "--alternate-refs");
if (opt->progress)
argv_array_pushf(&rev_list.args, "--progress=%s",
_("Checking connectivity"));
diff --git a/revision.c b/revision.c
index 621feb9df7..07412297f0 100644
--- a/revision.c
+++ b/revision.c
@@ -1554,6 +1554,32 @@ void add_index_objects_to_pending(struct rev_info *revs, unsigned int flags)
free_worktrees(worktrees);
}
+struct add_alternate_refs_data {
+ struct rev_info *revs;
+ unsigned int flags;
+};
+
+static void add_one_alternate_ref(const struct object_id *oid,
+ void *vdata)
+{
+ const char *name = ".alternate";
+ struct add_alternate_refs_data *data = vdata;
+ struct object *obj;
+
+ obj = get_reference(data->revs, name, oid, data->flags);
+ add_rev_cmdline(data->revs, obj, name, REV_CMD_REV, data->flags);
+ add_pending_object(data->revs, obj, name);
+}
+
+static void add_alternate_refs_to_pending(struct rev_info *revs,
+ unsigned int flags)
+{
+ struct add_alternate_refs_data data;
+ data.revs = revs;
+ data.flags = flags;
+ for_each_alternate_ref(add_one_alternate_ref, &data);
+}
+
static int add_parents_only(struct rev_info *revs, const char *arg_, int flags,
int exclude_parent)
{
@@ -1956,6 +1982,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
!strcmp(arg, "--no-walk") || !strcmp(arg, "--do-walk") ||
!strcmp(arg, "--bisect") || starts_with(arg, "--glob=") ||
!strcmp(arg, "--indexed-objects") ||
+ !strcmp(arg, "--alternate-refs") ||
starts_with(arg, "--exclude=") ||
starts_with(arg, "--branches=") || starts_with(arg, "--tags=") ||
starts_with(arg, "--remotes=") || starts_with(arg, "--no-walk="))
@@ -2442,6 +2469,8 @@ static int handle_revision_pseudo_opt(const char *submodule,
add_reflogs_to_pending(revs, *flags);
} else if (!strcmp(arg, "--indexed-objects")) {
add_index_objects_to_pending(revs, *flags);
+ } else if (!strcmp(arg, "--alternate-refs")) {
+ add_alternate_refs_to_pending(revs, *flags);
} else if (!strcmp(arg, "--not")) {
*flags ^= UNINTERESTING | BOTTOM;
} else if (!strcmp(arg, "--no-walk")) {
diff --git a/t/perf/p5600-clone-reference.sh b/t/perf/p5600-clone-reference.sh
new file mode 100755
index 0000000000..68fed66347
--- /dev/null
+++ b/t/perf/p5600-clone-reference.sh
@@ -0,0 +1,27 @@
+#!/bin/sh
+
+test_description='speed of clone --reference'
+. ./perf-lib.sh
+
+test_perf_default_repo
+
+test_expect_success 'create shareable repository' '
+ git clone --bare . shared.git
+'
+
+test_expect_success 'advance base repository' '
+ # Do not use test_commit here; its test_tick will
+ # use some ancient hard-coded date. The resulting clock
+ # skew will cause pack-objects to traverse in a very
+ # sub-optimal order, skewing the results.
+ echo content >new-file-that-does-not-exist &&
+ git add new-file-that-does-not-exist &&
+ git commit -m "new commit"
+'
+
+test_perf 'clone --reference' '
+ rm -rf dst.git &&
+ git clone --no-local --bare --reference shared.git . dst.git
+'
+
+test_done
diff --git a/t/t5618-alternate-refs.sh b/t/t5618-alternate-refs.sh
new file mode 100755
index 0000000000..3353216f09
--- /dev/null
+++ b/t/t5618-alternate-refs.sh
@@ -0,0 +1,60 @@
+#!/bin/sh
+
+test_description='test handling of --alternate-refs traversal'
+. ./test-lib.sh
+
+# Avoid test_commit because we want a specific and known set of refs:
+#
+# base -- one
+# \ \
+# two -- merged
+#
+# where "one" and "two" are on separate refs, and "merged" is available only in
+# the dependent child repository.
+test_expect_success 'set up local refs' '
+ git checkout -b one &&
+ test_tick &&
+ git commit --allow-empty -m base &&
+ test_tick &&
+ git commit --allow-empty -m one &&
+ git checkout -b two HEAD^ &&
+ test_tick &&
+ git commit --allow-empty -m two
+'
+
+# We'll enter the child repository after it's set up since that's where
+# all of the subsequent tests will want to run (and it's easy to forget a
+# "-C child" and get nonsense results).
+test_expect_success 'set up shared clone' '
+ git clone -s . child &&
+ cd child &&
+ git merge origin/one
+'
+
+test_expect_success 'rev-list --alternate-refs' '
+ git rev-list --remotes=origin >expect &&
+ git rev-list --alternate-refs >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'rev-list --not --alternate-refs' '
+ git rev-parse HEAD >expect &&
+ git rev-list HEAD --not --alternate-refs >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'limiting with alternateRefsPrefixes' '
+ test_config core.alternateRefsPrefixes refs/heads/one &&
+ git rev-list origin/one >expect &&
+ git rev-list --alternate-refs >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'log --source shows .alternate marker' '
+ git log --oneline --source --remotes=origin >expect.orig &&
+ sed "s/origin.* /.alternate /" <expect.orig >expect &&
+ git log --oneline --source --alternate-refs >actual &&
+ test_cmp expect actual
+'
+
+test_done
--
2.22.0.776.g16867c022c
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v2 2/2] check_everything_connected: assume alternate ref tips are valid
2019-07-01 13:18 ` [PATCH v2 2/2] check_everything_connected: assume alternate ref tips are valid Jeff King
@ 2019-07-03 9:12 ` SZEDER Gábor
2019-07-03 16:41 ` Jeff King
0 siblings, 1 reply; 20+ messages in thread
From: SZEDER Gábor @ 2019-07-03 9:12 UTC (permalink / raw)
To: Jeff King; +Cc: Derrick Stolee, Junio C Hamano, git, Taylor Blau
On Mon, Jul 01, 2019 at 09:18:15AM -0400, Jeff King wrote:
> diff --git a/t/t5618-alternate-refs.sh b/t/t5618-alternate-refs.sh
> new file mode 100755
> index 0000000000..3353216f09
> --- /dev/null
> +++ b/t/t5618-alternate-refs.sh
> @@ -0,0 +1,60 @@
> +test_expect_success 'log --source shows .alternate marker' '
> + git log --oneline --source --remotes=origin >expect.orig &&
> + sed "s/origin.* /.alternate /" <expect.orig >expect &&
Unnecessary redirection, 'sed' can open that file on its own as well.
> + git log --oneline --source --alternate-refs >actual &&
> + test_cmp expect actual
> +'
> +
> +test_done
> --
> 2.22.0.776.g16867c022c
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 2/2] check_everything_connected: assume alternate ref tips are valid
2019-07-03 9:12 ` SZEDER Gábor
@ 2019-07-03 16:41 ` Jeff King
2019-07-03 16:46 ` Junio C Hamano
2019-07-03 16:50 ` SZEDER Gábor
0 siblings, 2 replies; 20+ messages in thread
From: Jeff King @ 2019-07-03 16:41 UTC (permalink / raw)
To: SZEDER Gábor; +Cc: Derrick Stolee, Junio C Hamano, git, Taylor Blau
On Wed, Jul 03, 2019 at 11:12:25AM +0200, SZEDER Gábor wrote:
> On Mon, Jul 01, 2019 at 09:18:15AM -0400, Jeff King wrote:
> > diff --git a/t/t5618-alternate-refs.sh b/t/t5618-alternate-refs.sh
> > new file mode 100755
> > index 0000000000..3353216f09
> > --- /dev/null
> > +++ b/t/t5618-alternate-refs.sh
> > @@ -0,0 +1,60 @@
>
> > +test_expect_success 'log --source shows .alternate marker' '
> > + git log --oneline --source --remotes=origin >expect.orig &&
> > + sed "s/origin.* /.alternate /" <expect.orig >expect &&
>
> Unnecessary redirection, 'sed' can open that file on its own as well.
Sure, but is there a compelling reason not to feed it as stdin?
-Peff
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 2/2] check_everything_connected: assume alternate ref tips are valid
2019-07-03 16:41 ` Jeff King
@ 2019-07-03 16:46 ` Junio C Hamano
2019-07-03 16:50 ` SZEDER Gábor
1 sibling, 0 replies; 20+ messages in thread
From: Junio C Hamano @ 2019-07-03 16:46 UTC (permalink / raw)
To: Jeff King; +Cc: SZEDER Gábor, Derrick Stolee, git, Taylor Blau
Jeff King <peff@peff.net> writes:
> On Wed, Jul 03, 2019 at 11:12:25AM +0200, SZEDER Gábor wrote:
>
>> On Mon, Jul 01, 2019 at 09:18:15AM -0400, Jeff King wrote:
>> > diff --git a/t/t5618-alternate-refs.sh b/t/t5618-alternate-refs.sh
>> > new file mode 100755
>> > index 0000000000..3353216f09
>> > --- /dev/null
>> > +++ b/t/t5618-alternate-refs.sh
>> > @@ -0,0 +1,60 @@
>>
>> > +test_expect_success 'log --source shows .alternate marker' '
>> > + git log --oneline --source --remotes=origin >expect.orig &&
>> > + sed "s/origin.* /.alternate /" <expect.orig >expect &&
>>
>> Unnecessary redirection, 'sed' can open that file on its own as well.
>
> Sure, but is there a compelling reason not to feed it as stdin?
FWIW I do not think it is bad to redirect into a command at all ;-)
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 2/2] check_everything_connected: assume alternate ref tips are valid
2019-07-03 16:41 ` Jeff King
2019-07-03 16:46 ` Junio C Hamano
@ 2019-07-03 16:50 ` SZEDER Gábor
2019-07-03 17:05 ` Junio C Hamano
1 sibling, 1 reply; 20+ messages in thread
From: SZEDER Gábor @ 2019-07-03 16:50 UTC (permalink / raw)
To: Jeff King; +Cc: Derrick Stolee, Junio C Hamano, git, Taylor Blau
On Wed, Jul 03, 2019 at 12:41:16PM -0400, Jeff King wrote:
> On Wed, Jul 03, 2019 at 11:12:25AM +0200, SZEDER Gábor wrote:
>
> > On Mon, Jul 01, 2019 at 09:18:15AM -0400, Jeff King wrote:
> > > diff --git a/t/t5618-alternate-refs.sh b/t/t5618-alternate-refs.sh
> > > new file mode 100755
> > > index 0000000000..3353216f09
> > > --- /dev/null
> > > +++ b/t/t5618-alternate-refs.sh
> > > @@ -0,0 +1,60 @@
> >
> > > +test_expect_success 'log --source shows .alternate marker' '
> > > + git log --oneline --source --remotes=origin >expect.orig &&
> > > + sed "s/origin.* /.alternate /" <expect.orig >expect &&
> >
> > Unnecessary redirection, 'sed' can open that file on its own as well.
>
> Sure, but is there a compelling reason not to feed it as stdin?
Not really, other than there is no compelling reason to do so :)
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 2/2] check_everything_connected: assume alternate ref tips are valid
2019-07-03 16:50 ` SZEDER Gábor
@ 2019-07-03 17:05 ` Junio C Hamano
0 siblings, 0 replies; 20+ messages in thread
From: Junio C Hamano @ 2019-07-03 17:05 UTC (permalink / raw)
To: SZEDER Gábor; +Cc: Jeff King, Derrick Stolee, git, Taylor Blau
SZEDER Gábor <szeder.dev@gmail.com> writes:
> On Wed, Jul 03, 2019 at 12:41:16PM -0400, Jeff King wrote:
>> On Wed, Jul 03, 2019 at 11:12:25AM +0200, SZEDER Gábor wrote:
>>
>> > On Mon, Jul 01, 2019 at 09:18:15AM -0400, Jeff King wrote:
>> > > diff --git a/t/t5618-alternate-refs.sh b/t/t5618-alternate-refs.sh
>> > > new file mode 100755
>> > > index 0000000000..3353216f09
>> > > --- /dev/null
>> > > +++ b/t/t5618-alternate-refs.sh
>> > > @@ -0,0 +1,60 @@
>> >
>> > > +test_expect_success 'log --source shows .alternate marker' '
>> > > + git log --oneline --source --remotes=origin >expect.orig &&
>> > > + sed "s/origin.* /.alternate /" <expect.orig >expect &&
>> >
>> > Unnecessary redirection, 'sed' can open that file on its own as well.
>>
>> Sure, but is there a compelling reason not to feed it as stdin?
>
> Not really, other than there is no compelling reason to do so :)
For this particular one, it would not make much difference, but when
feeding a single file to a command that can take many instructions
as command line arguments, I tend to prefer
$ cmd <input \
-e 's/foo/bar/' \
-e 's/xyzzy/frotz/g' \
...
which I find slightly easier to read than
$ cmd \
-e 's/foo/bar/' \
-e 's/xyzzy/frotz/g' \
... \
input
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] check_everything_connected: assume alternate ref tips are valid
2019-07-01 13:17 ` Jeff King
2019-07-01 13:17 ` [PATCH 1/2] object-store.h: move for_each_alternate_ref() from transport.h Jeff King
2019-07-01 13:18 ` [PATCH v2 2/2] check_everything_connected: assume alternate ref tips are valid Jeff King
@ 2019-07-01 17:02 ` Taylor Blau
2019-07-02 5:29 ` Jeff King
2 siblings, 1 reply; 20+ messages in thread
From: Taylor Blau @ 2019-07-01 17:02 UTC (permalink / raw)
To: Jeff King; +Cc: Derrick Stolee, Junio C Hamano, git, Taylor Blau
Hi Peff,
On Mon, Jul 01, 2019 at 09:17:13AM -0400, Jeff King wrote:
> On Mon, Jul 01, 2019 at 08:59:45AM -0400, Jeff King wrote:
>
> > Yes, this is weakening the ties of the feature to the transport code.
> > Traditionally transport-oriented code was the only user, but it also
> > used the upload-pack transport under the hood to access the alternate
> > (that was changed a while ago to for-each-ref for speed).
> >
> > I don't think there's any functional difference in having it there, but
> > it could be moved to live alongside foreach_alt_odb() in sha1-file.c.
>
> Looks like this hasn't quite hit 'next' yet, so perhaps we can
> reorganize it as a preparatory patch.
>
> [1/2]: object-store.h: move for_each_alternate_ref() from transport.h
> [2/2]: check_everything_connected: assume alternate ref tips are valid
>
> Documentation/rev-list-options.txt | 8 +++
> builtin/receive-pack.c | 1 -
> connected.c | 1 +
> object-store.h | 2 +
> revision.c | 29 +++++++++
> sha1-file.c | 97 ++++++++++++++++++++++++++++++
> t/perf/p5600-clone-reference.sh | 27 +++++++++
> t/t5618-alternate-refs.sh | 60 ++++++++++++++++++
> transport.c | 97 ------------------------------
> transport.h | 2 -
> 10 files changed, 224 insertions(+), 100 deletions(-)
> create mode 100755 t/perf/p5600-clone-reference.sh
> create mode 100755 t/t5618-alternate-refs.sh
This looks good to me, too (and matches my recollection from our prior
off-list review against GitHub's fork).
One thing that I didn't catch in my initial review that I am seeing now
is the ".alternate" marker. Why did you choose this? I was thinking that
".have" would make more sense since it's consistent with what's shown in
the ref advertisement, but I think that actually ".alternate" is a
_better_ choice: the two really do refer to different things.
Either way, I don't think that it matters much: this series is already
on 'next' and I think that either is fine (especially since ".have"
refers to a nameless ref and ".alternate" refers to a nameless
pseudo-remote).
> -Peff
Thanks,
Taylor
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] check_everything_connected: assume alternate ref tips are valid
2019-07-01 17:02 ` [PATCH] " Taylor Blau
@ 2019-07-02 5:29 ` Jeff King
0 siblings, 0 replies; 20+ messages in thread
From: Jeff King @ 2019-07-02 5:29 UTC (permalink / raw)
To: Taylor Blau; +Cc: Derrick Stolee, Junio C Hamano, git
On Mon, Jul 01, 2019 at 12:02:45PM -0500, Taylor Blau wrote:
> One thing that I didn't catch in my initial review that I am seeing now
> is the ".alternate" marker. Why did you choose this? I was thinking that
> ".have" would make more sense since it's consistent with what's shown in
> the ref advertisement, but I think that actually ".alternate" is a
> _better_ choice: the two really do refer to different things.
Yeah, I had called these ".have" originally, but decided that was too
tied up with the current users, and not with the concept. I think
keeping the leading "." is worthwhile as that's an invalid refname.
I also thought about an empty string, but it's probably more informative
to show _something_. After all, the user would not see these unless they
specifically asked for them _and_ used something like --source, so
presumably it's a useful piece of information at that point (I don't
know of any other way to show these names except for --source).
I suppose one other option would be to name them after the oid itself.
So with --source you'd find out that 1234abcd came from 1234abcd (duh),
but also that its children came from 1234abcd. Maybe that has value. I
dunno.
It would be easy to change, but I'd also be OK punting until somebody
comes up with a compelling use case.
-Peff
^ permalink raw reply [flat|nested] 20+ messages in thread