[PATCH] git-send-pack: Fix --all option when used with directory

[PATCH] git-send-pack: Fix --all option when used with directory

[Stanislav Kolotinskiy]

When using git send-pack with --all option
and a target directory, usage message is being
displayed instead of performing the actual transmission.

The reason for this issue is that refspecs variable is being
calculated in a different way comparing to previous versions,
and even though the number of refspecs (nr_refspecs) is 0,
refspecs contain all the arguments and switches passed to send-pack.

This ensures that send-pack will stop execution only when --all
or --mirror switch is used in conjunction with any refs passed.

Signed-off-by: Stanislav Kolotinskiy <stanislav@assembla.com>
---
 builtin/send-pack.c      |  2 +-
 t/t9904-send-pack-all.sh | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 33 insertions(+), 1 deletion(-)
 create mode 100755 t/t9904-send-pack-all.sh

diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index f6e5d64..19f0577 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -225,7 +225,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
 	 * --all and --mirror are incompatible; neither makes sense
 	 * with any refspecs.
 	 */
-	if ((refspecs && (send_all || args.send_mirror)) ||
+	if ((nr_refspecs > 0 && (send_all || args.send_mirror)) ||
 	    (send_all && args.send_mirror))
 		usage_with_options(send_pack_usage, options);
 
diff --git a/t/t9904-send-pack-all.sh b/t/t9904-send-pack-all.sh
new file mode 100755
index 0000000..f68d004
--- /dev/null
+++ b/t/t9904-send-pack-all.sh
@@ -0,0 +1,32 @@
+#!/bin/sh
+
+test_description='Make sure that send-pack --all copies all refs'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+
+	git init --bare bare_repo && git init repo && (
+		cd repo &&
+
+		git remote add origin ../bare_repo &&
+		date >file1 && git add file1 && test_tick &&
+		git commit -m Initial &&
+		git push origin master &&
+
+		git checkout -b other && date >file2 &&
+		git add file2 && test_tick &&
+		git commit -m Other &&
+		git push origin other
+	) && git init another && (
+		cd another &&
+
+		git config receive.denyCurrentBranch ignore
+	)
+'
+
+test_expect_success 'send-pack --all should copy all refs' '
+	cd bare_repo && git send-pack --all ../another
+'
+
+test_done
-- 
2.7.4

Re: [PATCH] git-send-pack: Fix --all option when used with directory

[Jeff King]

On Wed, Mar 23, 2016 at 06:24:22PM +0200, Stanislav Kolotinskiy wrote:

> When using git send-pack with --all option
> and a target directory, usage message is being
> displayed instead of performing the actual transmission.

Yeah, that seems very wrong.

Not that it matters for this bug, but for my own curiosity, what do you
use "send-pack --all" for? I've generally assumed that nobody directly
calls send-pack themselves these days, but of course we have no data to
support that either way. So I am always interested to hear about unusual
use cases.

> The reason for this issue is that refspecs variable is being
> calculated in a different way comparing to previous versions,
> and even though the number of refspecs (nr_refspecs) is 0,
> refspecs contain all the arguments and switches passed to send-pack.

Looks like this bisects to 068c77a (builtin/send-pack.c: use
parse_options API, 2015-08-19). Thanks for including a test, which made
the bisection easy.

I wondered how the original ever worked, since it also points to argv
with the "refspecs" variable. But we only do so when we see an actual
refspec argument, and otherwise leave it as NULL. Whereas the new code
lumps the destination and the refspecs into the same conditional.

That made me wonder if any other code cared about refspecs being NULL. I
don't think so, though. The other spots I looked at seem to use
nr_refspec, which is good. There is one interesting interaction with
--stdin, which _does_ always set refspecs to a non-NULL value.

In the original code (prior to 068c77a), doing this:

  git send-pack --stdin --all </dev/null

complained. If though we did not specify any refspecs, it saw that
"refspecs" was non-NULL.

With your patch, that works (since nr_refspecs ends up as 0, it is OK
with --all). I think that is probably OK, though of course it is a
somewhat nonsensical request.

Arguably the "if (stdin)" conditional should also check for --all or
--mirror and complain, but I doubt it really matters in practice.

> diff --git a/t/t9904-send-pack-all.sh b/t/t9904-send-pack-all.sh

The tests are roughly grouped by functionality. send-pack tests are in
the t540x range, and this should probably go there. Though I also
suspect it could easily be added to the end of an existing test script,
which is preferable.

> +test_expect_success setup '

This setup seems a bit more complicated than it needs to be. It's nice
to keep tests as simple as possible, so a reader can understand exactly
what is being tested.

Here are a few things I think we can simplify:

> +	git init --bare bare_repo && git init repo && (
> +		cd repo &&

We're in a repository already, so we should just be able to push
directly out of the "main" test repository.

> +		git remote add origin ../bare_repo &&

We don't need to define remotes; we can just push directly to paths.

> +		date >file1 && git add file1 && test_tick &&
> +		git commit -m Initial &&

You can use "test_commit" to make this simpler.

> +		git push origin master &&
> +
> +		git checkout -b other && date >file2 &&
> +		git add file2 && test_tick &&
> +		git commit -m Other &&
> +		git push origin other

I guess you have multiple branches here so that we can be sure that
"--all" is pushing all of them. But your later test doesn't actually
check that.

> +	) && git init another && (
> +		cd another &&
> +
> +		git config receive.denyCurrentBranch ignore
> +	)
> +'

If you make the destination repository bare, then you do not have to
worry about denyCurrentBranch.

> +test_expect_success 'send-pack --all should copy all refs' '
> +	cd bare_repo && git send-pack --all ../another
> +'

We try to keep mutations of the test script state (like "cd") inside a
subshell, so they don't influence later tests. There aren't any later
tests now, of course, but it's one less thing for somebody coming along
later to have to worry about.

-Peff

Re: [PATCH] git-send-pack: Fix --all option when used with directory

[Jeff King]

On Wed, Mar 23, 2016 at 05:22:13PM -0400, Jeff King wrote:

> > diff --git a/t/t9904-send-pack-all.sh b/t/t9904-send-pack-all.sh
> 
> The tests are roughly grouped by functionality. send-pack tests are in
> the t540x range, and this should probably go there. Though I also
> suspect it could easily be added to the end of an existing test script,
> which is preferable.
> 
> > +test_expect_success setup '
> 
> This setup seems a bit more complicated than it needs to be. It's nice
> to keep tests as simple as possible, so a reader can understand exactly
> what is being tested.
> 
> Here are a few things I think we can simplify:
> [...]

So I think we could replace your t9904 with something like this:

diff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh
index 04cea97..305ca7a 100755
--- a/t/t5400-send-pack.sh
+++ b/t/t5400-send-pack.sh
@@ -128,6 +128,18 @@ test_expect_success 'denyNonFastforwards trumps --force' '
 	test "$victim_orig" = "$victim_head"
 '
 
+test_expect_success 'send-pack --all sends all branches' '
+	# make sure we have at least 2 branches with different
+	# values, just to be thorough
+	git branch other-branch HEAD^ &&
+
+	git init --bare all.git &&
+	git send-pack --all all.git &&
+	git for-each-ref refs/heads >expect &&
+	git -C all.git for-each-ref refs/heads >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'push --all excludes remote-tracking hierarchy' '
 	mkdir parent &&
 	(

Re: [PATCH] git-send-pack: Fix --all option when used with directory

[Stanislav Kolotinskiy]

On 23/03/16 23:22, Jeff King wrote:
> Not that it matters for this bug, but for my own curiosity, what do you
> use "send-pack --all" for? I've generally assumed that nobody directly
> calls send-pack themselves these days, but of course we have no data to
> support that either way. So I am always interested to hear about unusual
> use cases.
Well, here at Assembla we're using send-pack --all for creating forks
from repos in a quick and efficient way.
> The tests are roughly grouped by functionality. send-pack tests are in
> the t540x range, and this should probably go there. Though I also
> suspect it could easily be added to the end of an existing test script,
> which is preferable.
I'm not really comfortable (yet) with git tests, so thanks for pointing 
to that.
I did see t5400, but thought that bug fixes should bring their own, 
separate,
test files. Also, thanks for all the explanations and for the adaptation 
of my
test to a way better version!

I'm going to update the patch and will send another version. Thanks again!

--
Regards,
Stanislav

Re: [PATCH] git-send-pack: Fix --all option when used with directory

[Jeff King]

On Thu, Mar 24, 2016 at 03:57:59PM +0200, Stanislav Kolotinskiy wrote:

> On 23/03/16 23:22, Jeff King wrote:
> >Not that it matters for this bug, but for my own curiosity, what do you
> >use "send-pack --all" for? I've generally assumed that nobody directly
> >calls send-pack themselves these days, but of course we have no data to
> >support that either way. So I am always interested to hear about unusual
> >use cases.
> Well, here at Assembla we're using send-pack --all for creating forks
> from repos in a quick and efficient way.

I guess a follow-up to my question is: what does "git send-pack" do that
"git push" does not? Again, this is just for my curiosity (the answer
may be "nothing, we just happened to build our scripts around
send-pack", and that is perfectly fine).

-Peff

Re: [PATCH] git-send-pack: Fix --all option when used with directory

[Stanislav Kolotinskiy]

On 24/03/16 19:37, Jeff King wrote:
> I guess a follow-up to my question is: what does "git send-pack" do that
> "git push" does not? Again, this is just for my curiosity (the answer
> may be "nothing, we just happened to build our scripts around
> send-pack", and that is perfectly fine).
Well, this is some really old code (eight years old), and git version 
used at the moment was around 1.6. So I'm pretty much sure that it just 
happened to be the decision of the developer that wrote this piece of code.

--
Stanislav

Re: [PATCH] git-send-pack: Fix --all option when used with directory

[Junio C Hamano]

Stanislav Kolotinskiy <stanislav@assembla.com> writes:

> Subject: Re: [PATCH] git-send-pack: Fix --all option when used with directory

Please see "git shortlog --no-merges" output from recent history and
notice that s/Fix/fix/ would make things more consistent.

> When using git send-pack with --all option and a target directory,
> usage message is being displayed instead of performing the actual
> transmission.
>
> The reason for this issue is that refspecs variable is being
> calculated in a different way comparing to previous versions, and
> even though the number of refspecs (nr_refspecs) is 0, refspecs
> contain all the arguments and switches passed to send-pack.

Thanks for noticing; the above explanation however does not make it
very clear why the symptom exhibits itself only when "directory" is
given (it also is unclear if "target" being a directory is special,
or if any remote repository specification, e.g. host:/path/to/dir,
triggers the same problem).

Re: [PATCH] git-send-pack: Fix --all option when used with directory

[Stanislav Kolotinskiy]

On 24/03/16 18:11, Junio C Hamano wrote:
> Please see "git shortlog --no-merges" output from recent history and
> notice that s/Fix/fix/ would make things more consistent.
Thanks for letting me know, I'll update that.
> Thanks for noticing; the above explanation however does not make it
> very clear why the symptom exhibits itself only when "directory" is
> given (it also is unclear if "target" being a directory is special,
> or if any remote repository specification, e.g. host:/path/to/dir,
> triggers the same problem).
I'll think about a better explanation and will post it here - thanks.

--
Regards,
Stanislav

Re: [PATCH] git-send-pack: Fix --all option when used with directory

[Jeff King]

On Thu, Mar 24, 2016 at 07:47:12PM +0200, Stanislav Kolotinskiy wrote:

> >Thanks for noticing; the above explanation however does not make it
> >very clear why the symptom exhibits itself only when "directory" is
> >given (it also is unclear if "target" being a directory is special,
> >or if any remote repository specification, e.g. host:/path/to/dir,
> >triggers the same problem).
> I'll think about a better explanation and will post it here - thanks.

Feel free to steal from the explanation I posted earlier.

-Peff

Re: [PATCH] git-send-pack: Fix --all option when used with directory

[Stanislav Kolotinskiy]

Thanks a lot, Jeff, your explanation really helped!

--
Regards,
Stanislav

On 24/03/16 20:02, Jeff King wrote:
> On Thu, Mar 24, 2016 at 07:47:12PM +0200, Stanislav Kolotinskiy wrote:
>
>>> Thanks for noticing; the above explanation however does not make it
>>> very clear why the symptom exhibits itself only when "directory" is
>>> given (it also is unclear if "target" being a directory is special,
>>> or if any remote repository specification, e.g. host:/path/to/dir,
>>> triggers the same problem).
>> I'll think about a better explanation and will post it here - thanks.
> Feel free to steal from the explanation I posted earlier.
>
> -Peff