[PATCH/RFC] fetch: allow command line --tags to override config

[PATCH/RFC] fetch: allow command line --tags to override config

[Daniel Johnson]

Originally, if remote.<name>.tagopt was set, the --tags and option would
have no effect when given to git fetch. So if
tagopt="--no-tags"

	git fetch --tags

would not actually fetch tags.

This patch changes this behavior to only follow what is written in the
config if there is no option passed by the command line.

Signed-off-by: Daniel Johnson <ComputerDruid@gmail.com>
---
 builtin/fetch.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 1b67f5f..7a53144 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -675,10 +675,12 @@ static int do_fetch(struct transport *transport,
 
 	for_each_ref(add_existing, &existing_refs);
 
-	if (transport->remote->fetch_tags == 2 && tags != TAGS_UNSET)
-		tags = TAGS_SET;
-	if (transport->remote->fetch_tags == -1)
-		tags = TAGS_UNSET;
+	if (tags == TAGS_DEFAULT) {
+		if (transport->remote->fetch_tags == 2)
+			tags = TAGS_SET;
+		if (transport->remote->fetch_tags == -1)
+			tags = TAGS_UNSET;
+	}
 
 	if (!transport->get_refs_list || !transport->fetch)
 		die("Don't know how to fetch from %s", transport->url);
-- 
1.7.2

Re: [PATCH/RFC] fetch: allow command line --tags to override config

[Tay Ray Chuan]

Hi,

On Thu, Aug 5, 2010 at 2:56 AM, Daniel Johnson <computerdruid@gmail.com> wrote:
> Originally, if remote.<name>.tagopt was set, the --tags and option would
> have no effect when given to git fetch. So if
> tagopt="--no-tags"
>
>        git fetch --tags
>
> would not actually fetch tags.
>
> This patch changes this behavior to only follow what is written in the
> config if there is no option passed by the command line.
>
> Signed-off-by: Daniel Johnson <ComputerDruid@gmail.com>

This is pretty confusing as it is. Could you please provide a patch
for the git-fetch documentation too?

-- 
Cheers,
Ray Chuan

[PATCH] Documentation: changes in the behavior of tagopt

[Daniel Johnson]

---
How does this look?

 Documentation/config.txt        |    4 +++-
 Documentation/fetch-options.txt |    8 ++++++--
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index f81fb91..682ebef 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1627,7 +1627,9 @@ remote.<name>.tagopt::
 	Setting this value to \--no-tags disables automatic tag following when
 	fetching from remote <name>. Setting it to \--tags will fetch every
 	tag from remote <name>, even if they are not reachable from remote
-	branch heads.
+	branch heads. Passing these flags directly to linkgit:git-fetch[1] can
+	override this setting. See options \--tags and \--no-tags of
+	linkgit:git-fetch[1].
 
 remote.<name>.vcs::
 	Setting this to a value <vcs> will cause git to interact with
diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
index 9333c42..2fdfeac 100644
--- a/Documentation/fetch-options.txt
+++ b/Documentation/fetch-options.txt
@@ -49,7 +49,9 @@ ifndef::git-pull[]
 endif::git-pull[]
 	By default, tags that point at objects that are downloaded
 	from the remote repository are fetched and stored locally.
-	This option disables this automatic tag following.
+	This option disables this automatic tag following. The default
+	behavior for a remote may be specified with the remote.<name>.tagopt
+	setting. See linkgit:git-clone[1].
 
 -t::
 --tags::
@@ -58,7 +60,9 @@ endif::git-pull[]
 	objects reachable from the branch heads that are being
 	tracked will not be fetched by this mechanism.  This
 	flag lets all tags and their associated objects be
-	downloaded.
+	downloaded. The default behavior for a remote may be
+	specified with the remote.<name>.tagopt setting. See
+	linkgit:git-clone[1].
 
 -u::
 --update-head-ok::
-- 
1.7.2

Re: [PATCH] Documentation: changes in the behavior of tagopt

[Tay Ray Chuan]

On Fri, Aug 6, 2010 at 9:54 PM, Daniel Johnson <computerdruid@gmail.com> wrote:
> ---
> How does this look?

Looks ok - now you'll have to squash this with your code patch.

-- 
Cheers,
Ray Chuan

[RFC/PATCHv2] fetch: allow command line --tags to override config

[Daniel Johnson]

Originally, if remote.<name>.tagopt was set, the --tags and option would
have no effect when given to git fetch. So if
tagopt="--no-tags"

	git fetch --tags

would not actually fetch tags.

This patch changes this behavior to only follow what is written in the
config if there is no option passed by the command line.

Signed-off-by: Daniel Johnson <ComputerDruid@gmail.com>
---
On Sat, Aug 7, 2010 at 10:17 PM, Tay Ray Chuan <rctay89@gmail.com> wrote:
> On Fri, Aug 6, 2010 at 9:54 PM, Daniel Johnson <computerdruid@gmail.com> wrote:
>> ---
>> How does this look?
>
> Looks ok - now you'll have to squash this with your code patch.
And here it is. Sorry about the lateness. I also fixed a mistake I made in the
documentation (linked to the wrong secton).

I'd still like comments on both the code and the change in behavior behind it.

 Documentation/config.txt        |    4 +++-
 Documentation/fetch-options.txt |    8 ++++++--
 builtin/fetch.c                 |   10 ++++++----
 3 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index f81fb91..682ebef 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1627,7 +1627,9 @@ remote.<name>.tagopt::
 	Setting this value to \--no-tags disables automatic tag following when
 	fetching from remote <name>. Setting it to \--tags will fetch every
 	tag from remote <name>, even if they are not reachable from remote
-	branch heads.
+	branch heads. Passing these flags directly to linkgit:git-fetch[1] can
+	override this setting. See options \--tags and \--no-tags of
+	linkgit:git-fetch[1].
 
 remote.<name>.vcs::
 	Setting this to a value <vcs> will cause git to interact with
diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
index 9333c42..470ac31 100644
--- a/Documentation/fetch-options.txt
+++ b/Documentation/fetch-options.txt
@@ -49,7 +49,9 @@ ifndef::git-pull[]
 endif::git-pull[]
 	By default, tags that point at objects that are downloaded
 	from the remote repository are fetched and stored locally.
-	This option disables this automatic tag following.
+	This option disables this automatic tag following. The default
+	behavior for a remote may be specified with the remote.<name>.tagopt
+	setting. See linkgit:git-config[1].
 
 -t::
 --tags::
@@ -58,7 +60,9 @@ endif::git-pull[]
 	objects reachable from the branch heads that are being
 	tracked will not be fetched by this mechanism.  This
 	flag lets all tags and their associated objects be
-	downloaded.
+	downloaded. The default behavior for a remote may be
+	specified with the remote.<name>.tagopt setting. See
+	linkgit:git-config[1].
 
 -u::
 --update-head-ok::
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 1b67f5f..7a53144 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -675,10 +675,12 @@ static int do_fetch(struct transport *transport,
 
 	for_each_ref(add_existing, &existing_refs);
 
-	if (transport->remote->fetch_tags == 2 && tags != TAGS_UNSET)
-		tags = TAGS_SET;
-	if (transport->remote->fetch_tags == -1)
-		tags = TAGS_UNSET;
+	if (tags == TAGS_DEFAULT) {
+		if (transport->remote->fetch_tags == 2)
+			tags = TAGS_SET;
+		if (transport->remote->fetch_tags == -1)
+			tags = TAGS_UNSET;
+	}
 
 	if (!transport->get_refs_list || !transport->fetch)
 		die("Don't know how to fetch from %s", transport->url);
-- 
1.7.2

Re: [RFC/PATCHv2] fetch: allow command line --tags to override config

[Junio C Hamano]

Daniel Johnson <computerdruid@gmail.com> writes:

> Originally, if remote.<name>.tagopt was set, the --tags and option would
> have no effect when given to git fetch. So if
> tagopt="--no-tags"
>
> 	git fetch --tags
>
> would not actually fetch tags.
>
> This patch changes this behavior to only follow what is written in the
> config if there is no option passed by the command line.
>
> Signed-off-by: Daniel Johnson <ComputerDruid@gmail.com>
> ---
> On Sat, Aug 7, 2010 at 10:17 PM, Tay Ray Chuan <rctay89@gmail.com> wrote:
>> On Fri, Aug 6, 2010 at 9:54 PM, Daniel Johnson <computerdruid@gmail.com> wrote:
>>> ---
>>> How does this look?
>>
>> Looks ok - now you'll have to squash this with your code patch.
> And here it is. Sorry about the lateness. I also fixed a mistake I made in the
> documentation (linked to the wrong secton).
>
> I'd still like comments on both the code and the change in behavior behind it.

The current behaviour seems to me a bug introduced while git-fetch was
rewritten in C (the original found in contrib/examples reads from the
config only when no --tags/--no-tags option is given from the command
line).

Is this something we can protect with a test script from future breakages?

Thanks.

[RFC/PATCH] t5525: test the tagopt variable and that it can be overridden

[Daniel Johnson]

---
> The current behaviour seems to me a bug introduced while git-fetch was
> rewritten in C (the original found in contrib/examples reads from the
> config only when no --tags/--no-tags option is given from the command
> line).
>
> Is this something we can protect with a test script from future breakages?
This should test that behavior. I'd appreciate feedback on how to improve this
test. I'm not sure if this is the right name/number either.

 t/t5525-fetch-tagopt.sh |   44 ++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 44 insertions(+), 0 deletions(-)
 create mode 100755 t/t5525-fetch-tagopt.sh

diff --git a/t/t5525-fetch-tagopt.sh b/t/t5525-fetch-tagopt.sh
new file mode 100755
index 0000000..17bd407
--- /dev/null
+++ b/t/t5525-fetch-tagopt.sh
@@ -0,0 +1,44 @@
+
+#!/bin/sh
+
+test_description='tagopt variable affects "git fetch" and is overridden by commandline.'
+
+. ./test-lib.sh
+
+setup_clone () {
+	(git clone --mirror . $1 &&
+	git remote add remote_$1 $1 &&
+	cd $1 &&
+	git tag tag_$1)
+}
+
+test_expect_success setup '
+	echo >file original &&
+	git add file &&
+	git commit -a -m original &&
+	setup_clone one &&
+	git config remote.remote_one.tagopt --no-tags &&
+	setup_clone two &&
+	git config remote.remote_two.tagopt --tags
+	'
+
+test_expect_success "fetch with tagopt=--no-tags does not get tag" '
+	git fetch remote_one &&
+	! (git show-ref tag_one)
+	'
+
+test_expect_success "fetch --tags with tagopt=--no-tags gets tag" '
+	git fetch --tags remote_one &&
+	(git show-ref tag_one)
+	'
+
+test_expect_success "fetch --no-tags with tagopt=--tags does not get tag" '
+	git fetch --no-tags remote_two &&
+	! (git show-ref tag_two)
+	'
+
+test_expect_success "fetch with tagopt=--tags gets tag" '
+	git fetch remote_two &&
+	(git show-ref tag_two)
+	'
+test_done
-- 
1.7.2

Re: [RFC/PATCH] t5525: test the tagopt variable and that it can be overridden

[Ævar Arnfjörð Bjarmason]

On Fri, Aug 13, 2010 at 20:13, Daniel Johnson <computerdruid@gmail.com> wrote:
> ---
>> The current behaviour seems to me a bug introduced while git-fetch was
>> rewritten in C (the original found in contrib/examples reads from the
>> config only when no --tags/--no-tags option is given from the command
>> line).
>>
>> Is this something we can protect with a test script from future breakages?
> This should test that behavior. I'd appreciate feedback on how to improve this
> test. I'm not sure if this is the right name/number either.

Thanks for tackling this.

>  t/t5525-fetch-tagopt.sh |   44 ++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 44 insertions(+), 0 deletions(-)
>  create mode 100755 t/t5525-fetch-tagopt.sh
>
> diff --git a/t/t5525-fetch-tagopt.sh b/t/t5525-fetch-tagopt.sh

The test name looks fine.

(The t55* test names are a bit of a mess with mixed pull/fetch, but
that's not something that should be dealt with here)

> new file mode 100755
> index 0000000..17bd407
> --- /dev/null
> +++ b/t/t5525-fetch-tagopt.sh
> @@ -0,0 +1,44 @@
> +
> +#!/bin/sh

Is that an empty line before the test begins? The shebang should be on
the first line.

> +test_description='tagopt variable affects "git fetch" and is overridden by commandline.'
> +
> +. ./test-lib.sh
> +
> +setup_clone () {
> +       (git clone --mirror . $1 &&
> +       git remote add remote_$1 $1 &&
> +       cd $1 &&
> +       git tag tag_$1)
> +}

Maybe only put the "cd $1 ..." inside a subshell for clarity.

> +test_expect_success setup '
> +       echo >file original &&
> +       git add file &&
> +       git commit -a -m original &&

Maybe this can use test_commit if you don't mind it creating a tag
too.

> +       setup_clone one &&
> +       git config remote.remote_one.tagopt --no-tags &&
> +       setup_clone two &&
> +       git config remote.remote_two.tagopt --tags
> +       '
> +
> +test_expect_success "fetch with tagopt=--no-tags does not get tag" '
> +       git fetch remote_one &&
> +       ! (git show-ref tag_one)
> +       '

Doesn't need a subshell? You should also use:

    test_must_fail git show-ref ...

> +test_expect_success "fetch --tags with tagopt=--no-tags gets tag" '
> +       git fetch --tags remote_one &&
> +       (git show-ref tag_one)
> +       '

Doesn't need a subshell?

> +test_expect_success "fetch --no-tags with tagopt=--tags does not get tag" '
> +       git fetch --no-tags remote_two &&
> +       ! (git show-ref tag_two)
> +       '
> +
> +test_expect_success "fetch with tagopt=--tags gets tag" '
> +       git fetch remote_two &&
> +       (git show-ref tag_two)
> +       '
> +test_done

test_must_fail etc etc.

Otherwise it looks good.

[RFC/PATCHv2] t5525: test the tagopt variable and that it can be overridden

[Daniel Johnson]

---
>> @@ -0,0 +1,44 @@
>> +
>> +#!/bin/sh
>
> Is that an empty line before the test begins? The shebang should be on
> the first line.
Embarrassing. That's what I get for using yank/put and not paying closer
attention. The rest is fixed too.

 t/t5525-fetch-tagopt.sh |   41 +++++++++++++++++++++++++++++++++++++++++
 1 files changed, 41 insertions(+), 0 deletions(-)
 create mode 100755 t/t5525-fetch-tagopt.sh

diff --git a/t/t5525-fetch-tagopt.sh b/t/t5525-fetch-tagopt.sh
new file mode 100755
index 0000000..4fbf7a1
--- /dev/null
+++ b/t/t5525-fetch-tagopt.sh
@@ -0,0 +1,41 @@
+#!/bin/sh
+
+test_description='tagopt variable affects "git fetch" and is overridden by commandline.'
+
+. ./test-lib.sh
+
+setup_clone () {
+	git clone --mirror . $1 &&
+	git remote add remote_$1 $1 &&
+	(cd $1 &&
+	git tag tag_$1)
+}
+
+test_expect_success setup '
+	test_commit test &&
+	setup_clone one &&
+	git config remote.remote_one.tagopt --no-tags &&
+	setup_clone two &&
+	git config remote.remote_two.tagopt --tags
+	'
+
+test_expect_success "fetch with tagopt=--no-tags does not get tag" '
+	git fetch remote_one &&
+	test_must_fail git show-ref tag_one
+	'
+
+test_expect_success "fetch --tags with tagopt=--no-tags gets tag" '
+	git fetch --tags remote_one &&
+	git show-ref tag_one
+	'
+
+test_expect_success "fetch --no-tags with tagopt=--tags does not get tag" '
+	git fetch --no-tags remote_two &&
+	test_must_fail git show-ref tag_two
+	'
+
+test_expect_success "fetch with tagopt=--tags gets tag" '
+	git fetch remote_two &&
+	git show-ref tag_two
+	'
+test_done
-- 
1.7.2

Re: [RFC/PATCHv2] t5525: test the tagopt variable and that it can be overridden

[Ævar Arnfjörð Bjarmason]

On Fri, Aug 13, 2010 at 21:27, Daniel Johnson <computerdruid@gmail.com> wrote:
> ---
>>> @@ -0,0 +1,44 @@
>>> +
>>> +#!/bin/sh
>>
>> Is that an empty line before the test begins? The shebang should be on
>> the first line.
> Embarrassing. That's what I get for using yank/put and not paying closer
> attention. The rest is fixed too.

Nice, I haven't actually *run it* but it looks good, so provided that
it passes tests when our beloved maintainer applies it:

Reviewed-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>

Re: [RFC/PATCHv2] t5525: test the tagopt variable and that it can be overridden

[Daniel Johnson]

[-- Attachment #1: Type: Text/Plain, Size: 815 bytes --]

On Friday 13 August 2010 17:39:04 Ævar Arnfjörð Bjarmason wrote:
> On Fri, Aug 13, 2010 at 21:27, Daniel Johnson <computerdruid@gmail.com> 
wrote:
> > ---
> > 
> >>> @@ -0,0 +1,44 @@
> >>> +
> >>> +#!/bin/sh
> >> 
> >> Is that an empty line before the test begins? The shebang should be on
> >> the first line.
> > 
> > Embarrassing. That's what I get for using yank/put and not paying closer
> > attention. The rest is fixed too.
> 
> Nice, I haven't actually *run it* but it looks good, so provided that
> it passes tests when our beloved maintainer applies it:
> 
> Reviewed-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>

Well, now that we have a fix and a test to help guard against future breakages, 
can someone tell me the next step as to getting them included?

Thanks,
-Dan

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [RFC/PATCHv2] t5525: test the tagopt variable and that it can be overridden

[Ævar Arnfjörð Bjarmason]

On Sat, Aug 14, 2010 at 21:32, Daniel Johnson <computerdruid@gmail.com> wrote:
> Well, now that we have a fix and a test to help guard against future breakages,
> can someone tell me the next step as to getting them included?

You've done all the needed things. Junio goes through the mail archive
a few things a week to pick up patches. Hopefully he'll pick this up.