Re: reducing object store size with remote alternates or shallow clone?

Brandon Casey [off-list ref] writes:

On 08/24/2010 11:45 AM, Junio C Hamano wrote:

quoted

How about doing

    $ LINUS=git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.git

quoted

    $ git fetch $LINUS
    $ git bundle create myfork.bundle HEAD..master

I think you mean

      $ git fetch $LINUS master
      $ git bundle create myfork.bundle FETCH_HEAD..master

Thanks, of course you are right.

Strictly speaking, as I know there is only one branch in the repository of
Linus, there is no need to say "master" when fetching, but it would be a
good discipline to explicitly specify what you mean on the command line.

On 08/24/2010 01:59 PM, Junio C Hamano wrote:

Brandon Casey [off-list ref] writes:

quoted

On 08/24/2010 11:45 AM, Junio C Hamano wrote:

quoted

How about doing

    $ LINUS=git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.git

quoted

    $ git fetch $LINUS
    $ git bundle create myfork.bundle HEAD..master

I think you mean

      $ git fetch $LINUS master
      $ git bundle create myfork.bundle FETCH_HEAD..master

Thanks, of course you are right.

Strictly speaking, as I know there is only one branch in the repository of
Linus, there is no need to say "master" when fetching

Hmm.  It appears that if the current checked-out branch has a configured
merge ref, then a fetch that supplies a repository url (not a remote name)
and no fetch refspec, will not fall back to fetch HEAD from the remote
repository.

i.e. the following fetch does not retrieve any objects nor update FETCH_HEAD

   $ git clone git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.git linux
   $ cd linux
   $ git fetch git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.git

but, if you create a new branch, that has no merge ref configuration, then
git behaves as expected:

   $ git branch foo
   $ git checkout foo
   $ git fetch git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.git

Namely we retrieve new objects and update FETCH_HEAD:

   From git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6
   * branch            HEAD       -> FETCH_HEAD


I think the problem is in builtin/fetch.c: get_ref_map().

When fetch is called as above, with a repository url but no refspec,
we get this call sequence:

   cmd_fetch -> fetch_one
     fetch_one -> do_fetch(argc = 0)
       do_fetch -> get_ref_map(ref_count = 0)
         line 148: has_merge is assigned 1 since the current checked out
                   branch has a merge ref configured
         The 'if' branch is entered, the 'for' loop is not entered,
         ref_map retains its NULL initialization value and
         get_ref_map() returns NULL
       do_fetch -> fetch_refs(ref_map = NULL)
         and the transports do nothing since no refs have been requested

Perhaps the fix should look something like this (warning copy/paste):

diff --git a/builtin/fetch.c b/builtin/fetch.c
index fab3fce..218e71d 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c

@@ -146,7 +146,8 @@ static struct ref *get_ref_map(struct transport *transport,
                struct remote *remote = transport->remote;
                struct branch *branch = branch_get(NULL);
                int has_merge = branch_has_merge_config(branch);
-               if (remote && (remote->fetch_refspec_nr || has_merge)) {
+               if (remote && (remote->fetch_refspec_nr || (has_merge &&
+                               !strcmp(branch->remote_name, remote->name)))) {
                        for (i = 0; i < remote->fetch_refspec_nr; i++) {
                                get_fetch_map(remote_refs, &remote->fetch[i], &tail, 0);
                                if (remote->fetch[i].dst &&


-Brandon

From: Brandon Casey <redacted>

When 'git fetch' is supplied a single argument, it tries to match it
against a configured remote and then fetch the refs specified by the
named remote's fetchspec.  Additionally, or alternatively, if the current
branch has a merge ref configured, and if the name of the remote supplied
to fetch matches the one in the branch's configuration, then git also adds
the merge ref to the list of refs to update.

If the argument to fetch does not specify a named remote, or if the name
supplied does not match the remote configured for the current branch, then
the current branch's merge configuration should not be considered.

git currently mishandles the case when the argument to fetch specifies a
GIT URL(i.e. not a named remote) and the current branch has a configured
merge ref.  In this case, fetch should ignore the branch's merge ref and
attempt to fetch from the remote repository's HEAD branch.  But, since
fetch only checks _whether_ the current branch has a merge ref configured,
and does _not_ check whether the branch's configured remote matches the
command line argument (until later), it will mistakenly enter the wrong
branch of an 'if' statement and will not fall back to fetch the HEAD branch.
The fetch ends up doing nothing and returns with a successful zero status.

Fix this by comparing the remote repository's name to the branch's remote
name, in addition to whether it has a configured merge ref, sooner, so that
fetch can correctly decide whether the branch's configuration is interesting
or not, and fall back to fetching from the remote's HEAD branch when
appropriate.

This fixes the test in t5510.

Signed-off-by: Brandon Casey <redacted>
---
 builtin/fetch.c  |    3 ++-
 t/t5510-fetch.sh |    2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index ea14d5d..be6c27a 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c

@@ -146,7 +146,8 @@ static struct ref *get_ref_map(struct transport *transport,
 		struct remote *remote = transport->remote;
 		struct branch *branch = branch_get(NULL);
 		int has_merge = branch_has_merge_config(branch);
-		if (remote && (remote->fetch_refspec_nr || has_merge)) {
+		if (remote && (remote->fetch_refspec_nr || (has_merge &&
+				!strcmp(branch->remote_name, remote->name)))) {
 			for (i = 0; i < remote->fetch_refspec_nr; i++) {
 				get_fetch_map(remote_refs, &remote->fetch[i], &tail, 0);
 				if (remote->fetch[i].dst &&

diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 3c7569c..8fbd894 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh

@@ -240,7 +240,7 @@ test_expect_success 'fetch with a non-applying branch.<name>.merge' '
 	git fetch blub
 '
 
-test_expect_failure 'fetch from GIT URL with a non-applying branch.<name>.merge' '
+test_expect_success 'fetch from GIT URL with a non-applying branch.<name>.merge' '
 	git update-ref -d FETCH_HEAD &&
 	git fetch one &&
 	git rev-parse --verify FETCH_HEAD

-- 
1.7.2.1

From: Brandon Casey <redacted>

When 'git fetch' is supplied just a repository URL (not a remote name),
and without a fetch refspec, it should fetch from the remote HEAD branch
and update FETCH_HEAD with the fetched ref.  Currently, when 'git fetch'
is called like this, it fails to retrieve anything, and does not update
FETCH_HEAD, if the current checked-out branch has a configured merge ref.

i.e. this fetch fails to retrieve anything nor update FETCH_HEAD:

   git checkout master
   git config branch.master.merge refs/heads/master
   git fetch git://git.kernel.org/pub/scm/git/git.git

but this one does:

   git config --unset branch.master.merge
   git fetch git://git.kernel.org/pub/scm/git/git.git

Add a test to demonstrate this flaw.

Signed-off-by: Brandon Casey <redacted>
---
 t/t5510-fetch.sh |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 4eb10f6..3c7569c 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh

@@ -240,6 +240,12 @@ test_expect_success 'fetch with a non-applying branch.<name>.merge' '
 	git fetch blub
 '
 
+test_expect_failure 'fetch from GIT URL with a non-applying branch.<name>.merge' '
+	git update-ref -d FETCH_HEAD &&
+	git fetch one &&
+	git rev-parse --verify FETCH_HEAD
+'
+
 # the strange name is: a\!'b
 test_expect_success 'quoting of a strangely named repo' '
 	test_must_fail git fetch "a\\!'\''b" > result 2>&1 &&

-- 
1.7.2.1

Brandon Casey wrote:

If the argument to fetch does not specify a named remote, or if the name
supplied does not match the remote configured for the current branch, then
the current branch's merge configuration should not be considered.

Thanks for a fix.

What will happen with this (invalid) branch?

	[branch "tmp"]
		merge = refs/heads/tmp

Brandon Casey [off-list ref] writes:

From: Brandon Casey <redacted>

When 'git fetch' is supplied just a repository URL (not a remote name),
and without a fetch refspec, it should fetch from the remote HEAD branch
and update FETCH_HEAD with the fetched ref.  Currently, when 'git fetch'
is called like this, it fails to retrieve anything, and does not update
FETCH_HEAD, if the current checked-out branch has a configured merge ref.

i.e. this fetch fails to retrieve anything nor update FETCH_HEAD:

   git checkout master
   git config branch.master.merge refs/heads/master
   git fetch git://git.kernel.org/pub/scm/git/git.git

Hmph, we can call it a regression, since we certainly won't see this
failure with versions of git that is unaware of branch.*.merge.

But what should we be expecting?

Just as a datapoint, an ancient git (e.g. v1.4.0), the above command line
would have fetched the HEAD from the remote side, no matter what that
symref is pointing at.  Your [2/2] patch replicates this behaviour, which
is fine by me [*1*].

Your test only checks if we leave _anything_ in FETCH_HEAD, and does not
check if we only fetch one, if we fetch all the refs, or if we fetch what
the configuration branch.*.merge asks for (but without the corresponding
branch.*.remote configuration, doing so is pointless).

I think it would be better to have two tests.  One arranges the current
branch to track the same branch the HEAD at the remote points at, and the
other arranges the current branch to track a branch different from the
HEAD at the remote points at.  In both cases, as "fetch" should ignore the
configuration, we should get the object pointed by the HEAD on the remote
side.

Thanks.


[Footnote]

*1* A plausible alternative is to match the given URL against list of
existing remote.<name>.url (make sure there is only one), and behave as if
that the remote name is given.  I can be persuaded either way, but not
looking at the configuration feels a lot simpler to explain and understand
(i.e. "with name, we use the set of configuration variable given to that
name; without name, there is no configuration for us to look up").

On 08/25/2010 04:16 PM, Jonathan Nieder wrote:

Brandon Casey wrote:

quoted

If the argument to fetch does not specify a named remote, or if the name
supplied does not match the remote configured for the current branch, then
the current branch's merge configuration should not be considered.

Thanks for a fix.

quoted

+++ b/builtin/fetch.c

@@ -146,7 +146,8 @@ static struct ref *get_ref_map(struct transport *transport,
 		struct remote *remote = transport->remote;
 		struct branch *branch = branch_get(NULL);
 		int has_merge = branch_has_merge_config(branch);
-		if (remote && (remote->fetch_refspec_nr || has_merge)) {
+		if (remote && (remote->fetch_refspec_nr || (has_merge &&
+				!strcmp(branch->remote_name, remote->name)))) {

What will happen with this (invalid) branch?

	[branch "tmp"]
		merge = refs/heads/tmp

The same thing that would have happened before, since a few lines
further down there is this:

   if (has_merge &&
       !strcmp(branch->remote_name, remote->name))
           add_merge_config(&ref_map, remote_refs, branch, &tail);

I didn't trace branch_get() to check whether it returns an object
with remote_name initialized in all cases.  I relied on the form
of the existing code.  Perhaps it's worth investigating.  If something
needs to be fixed, then it was already broken and deserves a separate
patch anyway.

-Brandon