[PATCH] check_everything_connected: assume alternate ref tips are valid

[PATCH] check_everything_connected: assume alternate ref tips are valid

[Jeff King]

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>
---
 Documentation/rev-list-options.txt |  8 ++++
 connected.c                        |  1 +
 revision.c                         | 30 +++++++++++++++
 t/perf/p5600-clone-reference.sh    | 27 ++++++++++++++
 t/t5618-alternate-refs.sh          | 60 ++++++++++++++++++++++++++++++
 5 files changed, 126 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..b9be08e9df 100644
--- a/revision.c
+++ b/revision.c
@@ -28,6 +28,7 @@
 #include "commit-graph.h"
 #include "prio-queue.h"
 #include "hashmap.h"
+#include "transport.h"
 
 volatile show_early_output_fn_t show_early_output;
 
@@ -1554,6 +1555,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 +1983,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 +2470,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.775.g4ba9815492

Re: [PATCH] check_everything_connected: assume alternate ref tips are valid

[Jeff King]

On Fri, Jun 28, 2019 at 06:11:31AM -0400, Jeff King wrote:

> Subject: [PATCH] check_everything_connected: assume alternate ref tips...

Heh. This subject should just be "check_connected" these days. I wrote
this patch originally many years ago, before 7043c7071c
(check_everything_connected: use a struct with named options,
2016-07-15) gave the function a shorter name.

But the patch has been updated recently to match other updates to the
alternates code. In particular, in the original "rev-list
--alternate-refs" behaved as if all of the refs from the alternate were
mentioned on the command line, and you could use --exclude, etc as if
they were real refs. But we've since simplified the alternates interface
to just get a list of oids, so now it behaves more like
"--indexed-objects" in that it acts as if those oids were mentioned on
the command line.

I think "--alternate-refs" is still the right name, though, since we're
just giving the oids at the ref tips. And I think the included
documentation makes this clear.

-Peff

Re: [PATCH] check_everything_connected: assume alternate ref tips are valid

[Derrick Stolee]

On 6/28/2019 6:11 AM, Jeff King wrote:
> 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.

I was confused by this paragraph, because I didn't know about
for_each_alternate_ref() and how refs_From_alternate_cb() will
strip the "/objects" and append "/refs" to check refs if they
exist. All of that logic is in transport.c but used by
fetch-pack.c and builtin/receive-pack.c. But now we are adding
to revision.c, so the restriction to "this helps data transfer"
is getting murkier.

Is this something that should be extracted to the object-store
layer? Or is it so tricky to use that we shouldn't make it too
easy to fall into a bad pattern?

> 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%

It's really hard to argue with numbers like these. Kudos!

> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  Documentation/rev-list-options.txt |  8 ++++
>  connected.c                        |  1 +
>  revision.c                         | 30 +++++++++++++++
>  t/perf/p5600-clone-reference.sh    | 27 ++++++++++++++
>  t/t5618-alternate-refs.sh          | 60 ++++++++++++++++++++++++++++++

Other than the high-level questions above, the code and tests look
good to me.

Thanks,
-Stolee 

Re: [PATCH] check_everything_connected: assume alternate ref tips are valid

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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.
> ...
> 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"));

Quite honestly, I am very surprised that we did not do this.  The
idea of alternate object store, as well as reducing transfer cost by
advertising their tips as '.have' phony refs, is almost as old as
the pack protocol itself.

Re: [PATCH] check_everything_connected: assume alternate ref tips are valid

[Jeff King]

On Fri, Jun 28, 2019 at 08:51:04AM -0400, Derrick Stolee wrote:

> On 6/28/2019 6:11 AM, Jeff King wrote:
> > 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.
> 
> I was confused by this paragraph, because I didn't know about
> for_each_alternate_ref() and how refs_From_alternate_cb() will
> strip the "/objects" and append "/refs" to check refs if they
> exist. All of that logic is in transport.c but used by
> fetch-pack.c and builtin/receive-pack.c. But now we are adding
> to revision.c, so the restriction to "this helps data transfer"
> is getting murkier.

Using it for data transfer is still the main thing for our internal
calls, but I think it's worth exposing it for general use via rev-list.
I imagine it would mostly be for poking around and debugging, but it
should allow things like:

  # what do we have that our alternate does not
  git rev-list --all --not --alternate-refs

> Is this something that should be extracted to the object-store
> layer? Or is it so tricky to use that we shouldn't make it too
> easy to fall into a bad pattern?

I'm not sure what you have in mind, exactly. If you are asking whether
there are more places that alternate refs could be used, I can't think
of any. If you are asking whether this is in the wrong place, no, I
think it's the right place. :)

-Peff

Re: [PATCH] check_everything_connected: assume alternate ref tips are valid

[Jeff King]

On Fri, Jun 28, 2019 at 09:22:56AM -0700, Junio C Hamano wrote:

> >  	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"));
> 
> Quite honestly, I am very surprised that we did not do this.  The
> idea of alternate object store, as well as reducing transfer cost by
> advertising their tips as '.have' phony refs, is almost as old as
> the pack protocol itself.

Yeah, as you note we are already telling the other side of the push
"hey, we already have these objects". So we are almost always just
walking over our own local objects in the connectivity check, which is
silly.

I only did "clone --reference" in the perf test because it was the
simplest, but a push to a server with alternates should be similarly
improved. E.g., doing this in a clone of linux.git:

  git init --bare dst.git
  echo '../../.git/objects' >dst.git/objects/info/alternates
  time git push dst.git HEAD

goes from 40+ seconds to 100ms or so. Again, obviously that's the best
case, but it should also improve the normal case of somebody pulling
down "torvalds/linux.git" and pushing it back up to their own
"peff/linux.git", too.

I don't have real-world numbers yet from GitHub, because we're not
actually advertising .haves on push right now. All of the Git pieces are
now in places to do so, but we still have to make some tweaks at our
replication layer. But soon. :)

-Peff

Re: [PATCH] check_everything_connected: assume alternate ref tips are valid

[Derrick Stolee]

On 6/29/2019 3:43 AM, Jeff King wrote:
> On Fri, Jun 28, 2019 at 08:51:04AM -0400, Derrick Stolee wrote:
> 
>> On 6/28/2019 6:11 AM, Jeff King wrote:
>>> 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.
>>
>> I was confused by this paragraph, because I didn't know about
>> for_each_alternate_ref() and how refs_From_alternate_cb() will
>> strip the "/objects" and append "/refs" to check refs if they
>> exist. All of that logic is in transport.c but used by
>> fetch-pack.c and builtin/receive-pack.c. But now we are adding
>> to revision.c, so the restriction to "this helps data transfer"
>> is getting murkier.
> 
> Using it for data transfer is still the main thing for our internal
> calls, but I think it's worth exposing it for general use via rev-list.
> I imagine it would mostly be for poking around and debugging, but it
> should allow things like:
> 
>   # what do we have that our alternate does not
>   git rev-list --all --not --alternate-refs

So this is an example where the alternate refs are being used without
any network activity.

>> Is this something that should be extracted to the object-store
>> layer? Or is it so tricky to use that we shouldn't make it too
>> easy to fall into a bad pattern?
> 
> I'm not sure what you have in mind, exactly. If you are asking whether
> there are more places that alternate refs could be used, I can't think
> of any. If you are asking whether this is in the wrong place, no, I
> think it's the right place. :)

Just double-checking that it is appropriate for revision.c to take
dependence on transport.h instead of moving the alternate ref stuff
into a different header file. I trust your opinion.

-Stolee

Re: [PATCH] check_everything_connected: assume alternate ref tips are valid

[Derrick Stolee]

On 6/29/2019 3:55 AM, Jeff King wrote:
> On Fri, Jun 28, 2019 at 09:22:56AM -0700, Junio C Hamano wrote:
> 
>>>  	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"));
>>
>> Quite honestly, I am very surprised that we did not do this.  The
>> idea of alternate object store, as well as reducing transfer cost by
>> advertising their tips as '.have' phony refs, is almost as old as
>> the pack protocol itself.
> 
> Yeah, as you note we are already telling the other side of the push
> "hey, we already have these objects". So we are almost always just
> walking over our own local objects in the connectivity check, which is
> silly.
> 
> I only did "clone --reference" in the perf test because it was the
> simplest, but a push to a server with alternates should be similarly
> improved. E.g., doing this in a clone of linux.git:
> 
>   git init --bare dst.git
>   echo '../../.git/objects' >dst.git/objects/info/alternates
>   time git push dst.git HEAD
> 
> goes from 40+ seconds to 100ms or so. Again, obviously that's the best
> case, but it should also improve the normal case of somebody pulling
> down "torvalds/linux.git" and pushing it back up to their own
> "peff/linux.git", too.
> 
> I don't have real-world numbers yet from GitHub, because we're not
> actually advertising .haves on push right now. All of the Git pieces are
> now in places to do so, but we still have to make some tweaks at our
> replication layer. But soon. :)

Exciting! Should improve the user's experience keeping their forks
updated!

-Stolee

Re: [PATCH] check_everything_connected: assume alternate ref tips are valid

[Jeff King]

On Mon, Jul 01, 2019 at 08:25:09AM -0400, Derrick Stolee wrote:

> > I'm not sure what you have in mind, exactly. If you are asking whether
> > there are more places that alternate refs could be used, I can't think
> > of any. If you are asking whether this is in the wrong place, no, I
> > think it's the right place. :)
> 
> Just double-checking that it is appropriate for revision.c to take
> dependence on transport.h instead of moving the alternate ref stuff
> into a different header file. I trust your opinion.

Ah, I see. I misunderstood you before.

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.

-Peff

Re: [PATCH] check_everything_connected: assume alternate ref tips are valid

[Jeff King]

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

-Peff

[PATCH 1/2] object-store.h: move for_each_alternate_ref() from transport.h

[Jeff King]

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

[PATCH v2 2/2] check_everything_connected: assume alternate ref tips are valid

[Jeff King]

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

Re: [PATCH 1/2] object-store.h: move for_each_alternate_ref() from transport.h

[Derrick Stolee]

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

Re: [PATCH] check_everything_connected: assume alternate ref tips are valid

[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

Re: [PATCH] check_everything_connected: assume alternate ref tips are valid

[Jeff King]

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

Re: [PATCH v2 2/2] check_everything_connected: assume alternate ref tips are valid

[SZEDER Gábor]

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

Re: [PATCH v2 2/2] check_everything_connected: assume alternate ref tips are valid

[Jeff King]

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

Re: [PATCH v2 2/2] check_everything_connected: assume alternate ref tips are valid

[Junio C Hamano]

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 ;-)

Re: [PATCH v2 2/2] check_everything_connected: assume alternate ref tips are valid

[SZEDER Gábor]

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 :)

Re: [PATCH v2 2/2] check_everything_connected: assume alternate ref tips are valid

[Junio C Hamano]

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