From: Junio C Hamano <hidden> Date: 2016-06-15 22:46:10
Jeff King [off-list ref] writes:
On Fri, Feb 13, 2009 at 02:09:01AM -0800, Junio C Hamano wrote:
quoted
The entire series looks sane from a very cursory look; especially the
earlier ones are obviously good.
I also think it looks good. You raised a few style points below which I
thought were sensible and won't bother repeating.
quoted
Hmph, what does "-a" stand for? I would have expected to see "-u" that
stands for "update" here.
It was immediately obvious to me as "auto" (I think I even suggested
"-a" in another thread, so maybe that is why it seems so sensible to
me).
Yeah, latest round has --auto in it. Thanks, Jay.
I thought it was obvious that you would do:
git remote set-head master
in the same way that you would do:
git remote add -m master $remote $url
But I suppose clarifying it doesn't hurt.
I do not care too deeply if an explicit request to "set-head --auto"
screws up and sets a HEAD that was pointing at the right branch to another
branch because the command is not taught to give preference to the branch
HEAD originally points at, so I do not think I have any more issues with
the series for now, even though I may notice things later.
I have this series queued to private topic branch but it still does not
pass tests (breaks #8 and #18 of t5505 at least) by itself; the previous
round was no better. I think it is just the matter of updating the
expected output in the tests, but I didn't look further.
From: Jeff King <hidden> Date: 2016-06-15 22:46:10
On Fri, Feb 13, 2009 at 06:00:29PM -0800, Junio C Hamano wrote:
quoted
It was immediately obvious to me as "auto" (I think I even suggested
"-a" in another thread, so maybe that is why it seems so sensible to
me).
Yeah, latest round has --auto in it. Thanks, Jay.
Oops. I even went back and double-checked to make sure that it was not
there, but it would have helped if I actually checked the latest
version. :(
I do not care too deeply if an explicit request to "set-head --auto"
screws up and sets a HEAD that was pointing at the right branch to another
branch because the command is not taught to give preference to the branch
HEAD originally points at, so I do not think I have any more issues with
the series for now, even though I may notice things later.
I think that is reasonable; that is a separate enhancement which can
come later and is no reason to block the existing patches.
I have this series queued to private topic branch but it still does not
pass tests (breaks #8 and #18 of t5505 at least) by itself; the previous
round was no better. I think it is just the matter of updating the
expected output in the tests, but I didn't look further.
Test #8 is just a matter of updating output. But #18 is explicitly about
checking that "remote show" does not show symbolic refs. But Jay's patch
is about explicitly showing symbolic refs (just doing so as a ref-name
instead of a sha1):
* FAIL 18: "remote show" does not show symbolic refs
git clone one three &&
(cd three &&
git remote show origin > output &&
! grep HEAD < output &&
! grep -i stale < output)
I guess we could tighten the grep to
! egrep "HEAD: [0-9a-f]{40}" < output
but it may just make sense to get rid of the test; the exact output is
already covered by test #8. Squashable patch is below.
---
@@ -338,16 +339,6 @@ test_expect_success 'update default (overridden, with funny whitespace)' ''-test_expect_success'"remote show" does not show symbolic refs''--gitcloneonethree&&-(cdthree&&-gitremoteshoworigin>output&&-!grepHEAD<output&&-!grep-istale<output)--'- test_expect_success'reject adding remote with an invalid name''test_must_failgitremoteaddsome:urldesired-name
From: Jay Soffian <hidden> Date: 2016-06-15 22:46:10
On Fri, Feb 13, 2009 at 9:18 PM, Jeff King [off-list ref] wrote:
Test #8 is just a matter of updating output. But #18 is explicitly about
checking that "remote show" does not show symbolic refs. But Jay's patch
is about explicitly showing symbolic refs (just doing so as a ref-name
instead of a sha1):
* FAIL 18: "remote show" does not show symbolic refs
git clone one three &&
(cd three &&
git remote show origin > output &&
! grep HEAD < output &&
! grep -i stale < output)
I guess we could tighten the grep to
! egrep "HEAD: [0-9a-f]{40}" < output
but it may just make sense to get rid of the test; the exact output is
already covered by test #8. Squashable patch is below.
Thank you Jeff. I swear I ran t5505-remote.sh successfully but
obviously I am misremembering.
j.
From: Jay Soffian <hidden> Date: 2016-06-15 22:46:10
On Fri, Feb 13, 2009 at 9:00 PM, Junio C Hamano [off-list ref] wrote:
I do not care too deeply if an explicit request to "set-head --auto"
screws up and sets a HEAD that was pointing at the right branch to another
branch because the command is not taught to give preference to the branch
HEAD originally points at
So I don't mind fixing this, but here's the thing.
Say user has refs/remotes/origin/HEAD set to frotz. They then run "git
show remote origin" and we see that HEAD on the remote end could be
either master or frotz (both have the same SHA1).
What should we show in the output of "git remote show origin" next to
the HEAD line? master, or frotz? If we show master, then user might
wonder why "git remote set-head origin --auto" leaves
refs/remotes/origin/HEAD set to frotz. If we show frotz, then user
might wonder why when they cloned the repo in the first place they
ended up with HEAD set to master.
I'm bothered by that inconsistency, which is why I didn't follow-up
with another patch immediately.
But I will propose an alternative. In the output of "get remote show
origin", we show all matching branches. If the user does a set-head
--auto and we cannot determine HEAD unambiguously, we do something
like:
$ git remote set-head origin --auto
error: Multiple branches match HEAD. Please choose one explicitly with:
git remote set-head origin master
git remote set-head origin frotz
Hmm?
j.
From: Jeff King <hidden> Date: 2016-06-15 22:46:10
On Fri, Feb 13, 2009 at 09:59:06PM -0500, Jay Soffian wrote:
What should we show in the output of "git remote show origin" next to
the HEAD line? master, or frotz? If we show master, then user might
wonder why "git remote set-head origin --auto" leaves
refs/remotes/origin/HEAD set to frotz. If we show frotz, then user
might wonder why when they cloned the repo in the first place they
ended up with HEAD set to master.
I'm bothered by that inconsistency, which is why I didn't follow-up
with another patch immediately.
Hrm. Yeah, I think to avoid surprising the user, "--auto" has to use
whatever we showed in "git remote show".
But I will propose an alternative. In the output of "get remote show
origin", we show all matching branches. If the user does a set-head
--auto and we cannot determine HEAD unambiguously, we do something
like:
$ git remote set-head origin --auto
error: Multiple branches match HEAD. Please choose one explicitly with:
git remote set-head origin master
git remote set-head origin frotz
I like that proposal. It doesn't hide from the user that we are doing a
matching guess, which means we are less likely to surprise them in the
long run.
-Peff
From: Jay Soffian <hidden> Date: 2016-06-15 22:46:10
It is not currently possible to determine the remote HEAD unambiguously
when multiple remote branches share the same SHA1 as the remote HEAD.
In this situation, git remote set-head --auto should not try to guess
which HEAD the user wants. This patch causes set-head to provide a
useful error instead:
$ git remote set-head origin --auto
error: Multiple remote HEAD branches. Please choose one explicitly with:
git remote set-head origin another
git remote set-head origin master
Also, the output of git remote show now shows the multiple HEADs:
$ git remote show origin
* remote origin
URL: ...
HEAD branches:
another
master
Signed-off-by: Jay Soffian <redacted>
---
On Fri, Feb 13, 2009 at 10:43 PM, Jeff King [off-list ref] wrote:
On Fri, Feb 13, 2009 at 09:59:06PM -0500, Jay Soffian wrote:
quoted
But I will propose an alternative. In the output of "get remote show
origin", we show all matching branches. If the user does a set-head
--auto and we cannot determine HEAD unambiguously, we do something
like:
$ git remote set-head origin --auto
error: Multiple branches match HEAD. Please choose one explicitly with:
git remote set-head origin master
git remote set-head origin frotz
I like that proposal. It doesn't hide from the user that we are doing a
matching guess, which means we are less likely to surprise them in the
long run.
-Peff
Voilà
Junio - this obviously goes on-top of the rest of the builtin-remote
series I sent.
builtin-clone.c | 2 +-
builtin-remote.c | 56 +++++++++++++++++++++++++++++++++++------------------
remote.c | 28 ++++++++++++++++++-------
remote.h | 6 ++++-
t/t5505-remote.sh | 53 ++++++++++++++++++++++++++++++++++++++++++++++++-
5 files changed, 114 insertions(+), 31 deletions(-)
@@ -510,7 +510,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)mapped_refs=write_remote_refs(refs,&refspec,reflog_msg.buf);head_points_at=guess_remote_head(refs,mapped_refs,-&remote_head);+&remote_head,NULL);}else{warning("You appear to have cloned an empty repository.");
@@ -802,7 +809,16 @@ static int set_head(int argc, const char **argv)structref_statesstates;memset(&states,0,sizeof(states));get_remote_ref_states(argv[0],&states,1);-head_name=xstrdup(states.head_name);+if(!states.heads.nr)+result|=error("Cannot determine remote HEAD");+elseif(states.heads.nr>1){+result|=error("Multiple remote HEAD branches. "+"Please choose one explicitly with:");+for(i=0;i<states.heads.nr;i++)+fprintf(stderr," git remote set-head %s %s\n",+argv[0],states.heads.items[i].string);+}else+head_name=xstrdup(states.heads.items[0].string);free_remote_ref_states(&states);}elseif(opt_d&&!opt_a&&argc==1){if(delete_ref(buf.buf,NULL,REF_NODEREF))
@@ -818,6 +834,8 @@ static int set_head(int argc, const char **argv)result|=error("Not a valid ref: %s",buf2.buf);elseif(create_symref(buf.buf,buf2.buf,"remote set-head"))result|=error("Could not setup %s",buf.buf);+if(opt_a)+printf("%s/HEAD set to %s\n",argv[0],head_name);free(head_name);}
@@ -1407,9 +1412,16 @@ const struct ref *guess_remote_head(const struct ref *refs,/* Look for another ref that points there */for(r=mapped_refs;r;r=r->next)if(r!=remote_head&&-!hashcmp(r->old_sha1,remote_head->old_sha1))-returnr;+!hashcmp(r->old_sha1,remote_head->old_sha1)){+structref*cpy;+if(!ret)+ret=r;+if(!all_matches_p)+break;+*tail=cpy=copy_ref(r);+cpy->peer_ref=NULL;+tail=&cpy->next;+}-/* Nothing is the same */-returnNULL;+returnret;}
From: Jeff King <hidden> Date: 2016-06-15 22:46:10
On Sat, Feb 14, 2009 at 05:30:30AM -0500, Jay Soffian wrote:
In this situation, git remote set-head --auto should not try to guess
which HEAD the user wants. This patch causes set-head to provide a
useful error instead:
$ git remote set-head origin --auto
error: Multiple remote HEAD branches. Please choose one explicitly with:
git remote set-head origin another
git remote set-head origin master
Thanks. The patch looks good to me, with two comments and one style nit:
+ else if (states.heads.nr == 1)
+ printf(" HEAD branch: %s\n",
+ states.heads.items[0].string);
+ else
+ show_list(" HEAD branch%s:", &states.heads, "");
I was happy to see the common case of "we unambiguously determined HEAD"
falls back to nicer output (though I admit I did a double-take seeing
both show_list and the states.heads.nr check, I see it is because
show_list always insists on a newline).
That should help current users with simple setups, but also support
unambiguous HEAD reporting in the future (and based on what Daniel said
earlier, http should just need a client patch to pass the information
up the callstack).
+ if (opt_a)
+ printf("%s/HEAD set to %s\n", argv[0], head_name);
This was a surprise based on reading the commit message, but I think it
is a sensible enhancement.
I had to read this test a few times to convince myself it was right,
since you throw away the output of symbolic-ref. I think it makes more
sense to just test the post-command state, which is what you actually
care about (and then you are also not dependent on the human-readable
output of "remote set-head"). I.e.:
cat > test/expect <<EOF
refs/remotes/origin/master
EOF
test_expect_success 'set-head --auto' '
(cd test &&
git remote set-head --auto origin &&
git symbolic-ref refs/remotes/origin/HEAD > output &&
test_cmp expect output)
'
-Peff
I had to read this test a few times to convince myself it was right,
since you throw away the output of symbolic-ref. I think it makes more
sense to just test the post-command state, which is what you actually
care about (and then you are also not dependent on the human-readable
output of "remote set-head"). I.e.:
cat > test/expect <<EOF
refs/remotes/origin/master
EOF
test_expect_success 'set-head --auto' '
(cd test &&
git remote set-head --auto origin &&
git symbolic-ref refs/remotes/origin/HEAD > output &&
test_cmp expect output)
'
From: Jeff King <hidden> Date: 2016-06-15 22:46:10
On Sat, Feb 14, 2009 at 01:35:03PM -0500, Jay Soffian wrote:
On Sat, Feb 14, 2009 at 12:54 PM, Jeff King [off-list ref] wrote:
quoted
quoted
+ if (opt_a)
+ printf("%s/HEAD set to %s\n", argv[0], head_name);
This was a surprise based on reading the commit message, but I think it
is a sensible enhancement.
It seemed that when doing something "--automatically" it might be nice
to tell the user what we just did, but I'm confused why this was a
surprise.
I just meant that the commit message did not mention changes in this
area, and it is largely orthogonal to the rest of the patch (you could
just as easily apply this hunk without the rest of your patch, and it
would have the same value). Thus I was surprised.
But I do think it is a good change.
I suspect Junio can just fix this up during application if he agrees.
<random process musing>
Which made me think of something else, with all of this talk about
reviewers that has been going on. Junio is actually in a little bit of a
special position with small changes (like style issues) to say "I'll
apply this, but tweak these changes". But the rest of us are stuck
saying "I would change this one line" to the list; then either:
- the original submitter re-rolls the patch, which takes their time
and everyone else's time to look at the new patch, see that it is
trivially changed, etc
or
- Junio has to read the followup comments, then go back and find the
spot in the original patch to mark it up.
Which means that there is a transaction cost to little comments due to
the extra communication. And that cost can dwarf the actual time for the
change.
I don't know if there is a better method, or better tool support. I
guess reviewers could act like the maintainer, tweaking patches and then
publishing the result, which Junio would then pull. Or instead of
publishing the result, publishing an interdiff along with comments. But
basically putting the comments into a form that can be communicated and
applied more easily, which cuts down on the communication costs.
I don't know. Just thinking out loud.
-Peff
From: Daniel Barkalow <hidden> Date: 2016-06-15 22:46:10
On Sat, 14 Feb 2009, Jeff King wrote:
On Sat, Feb 14, 2009 at 05:30:30AM -0500, Jay Soffian wrote:
quoted
+ else if (states.heads.nr == 1)
+ printf(" HEAD branch: %s\n",
+ states.heads.items[0].string);
+ else
+ show_list(" HEAD branch%s:", &states.heads, "");
I was happy to see the common case of "we unambiguously determined HEAD"
falls back to nicer output (though I admit I did a double-take seeing
both show_list and the states.heads.nr check, I see it is because
show_list always insists on a newline).
That should help current users with simple setups, but also support
unambiguous HEAD reporting in the future (and based on what Daniel said
earlier, http should just need a client patch to pass the information
up the callstack).
I haven't checked lately, but I think that what's actually needed is to
have the locate_head() function notice if the struct ref for HEAD actually
has the symref field non-NULL, and report that as the unambiguous answer.
This should also allow it to automatically pick up any other
disambiguation by future sources of lists of refs that include HEAD,
whether that's git protocol extensions, filesystem access to the repo, or
foreign VCSes where some branches is inherently primary, or whatever.
(The direct purpose of collecting the information for http was so that it
could figure out the sha1 at all for the remote HEAD when it's a symref;
the code just doesn't then explicitly throw the information away.)
-Daniel
*This .sig left intentionally blank*
From: Jeff King <hidden> Date: 2016-06-15 22:46:10
On Sat, Feb 14, 2009 at 03:21:30PM -0500, Daniel Barkalow wrote:
I haven't checked lately, but I think that what's actually needed is to
have the locate_head() function notice if the struct ref for HEAD actually
has the symref field non-NULL, and report that as the unambiguous answer.
Indeed. Something like the patch below works (on top of Jay's patches).
But it has two shortcomings:
1. There is no test script, since we have no infrastructure for
testing over http. I might be able to build something on top of
what's in the http-push tests.
I was hoping we could do the same trick for local file repos, which
would be easy to test. But the transport code just treats them as
regular pack uploaders; only some specialized code in clone cares
about the difference.
In theory we could add a new transport for local repos. I don't
think it would make sense for its get_remote_refs function to get
_all_ of the refs, but it could specially peek at HEAD and set the
symref field appropriately.
2. The guess_remote_head function is getting a little long. I think it
would help to refactor it into two functions; one for finding the
remote HEAD in the refs list, and the other for guessing at a ref
which matches the HEAD.
I will try to make something a little neater later today.
This should also allow it to automatically pick up any other
disambiguation by future sources of lists of refs that include HEAD,
whether that's git protocol extensions, filesystem access to the repo, or
foreign VCSes where some branches is inherently primary, or whatever.
Yes, I think the symref field for the ref is a very sensible way of
communicating the information for that reason.
Patch is below.
---
@@ -1404,6 +1404,20 @@ const struct ref *guess_remote_head(const struct ref *refs,if(!remote_head)returnNULL;+/* if the underlying transport can represent symrefs,+*thenwedon'tneedtoguessatall*/+if(remote_head->symref){+for(r=mapped_refs;r;r=r->next){+if(!strcmp(r->name,remote_head->symref)){+if(all_matches_p){+*all_matches_p=copy_ref(r);+(*all_matches_p)->peer_ref=NULL;+}+returnr;+}+}+}+/* If refs/heads/master could be right, it is. */if(remote_master&&!hashcmp(remote_master->old_sha1,remote_head->old_sha1))
I missed it before, but there is a typo in this test (>&) that causes it
to barf.
Actually, it should be "> output 2>&1" to pick up the error message we
are expecting. Squashable diff is below.
The test script actually passes for me now.
---
From: Jeff King <hidden> Date: 2016-06-15 22:46:10
On Sat, Feb 14, 2009 at 04:15:48PM -0500, Jeff King wrote:
On Sat, Feb 14, 2009 at 03:21:30PM -0500, Daniel Barkalow wrote:
quoted
I haven't checked lately, but I think that what's actually needed is to
have the locate_head() function notice if the struct ref for HEAD actually
has the symref field non-NULL, and report that as the unambiguous answer.
Indeed. Something like the patch below works (on top of Jay's patches).
[...]
I will try to make something a little neater later today.
Here is a cleaner series. It depends on all of Jay's remote patches,
including the set-head one. I can resend once the dust is a little more
settled on those patches.
The first two are prep for adding a test in 5/5:
1/5: test scripts: refactor start_httpd helper
2/5: add basic http clone/fetch tests
These ones are code cleanup for 5/5:
3/5: refactor find_refs_by_name to accept const list
4/5: remote: refactor guess_remote_head
And this is the useful one.
5/5: remote: use exact HEAD lookup if it is available
-Peff
From: Jeff King <hidden> Date: 2016-06-15 22:46:10
There are some redirects and some error checking that need
to be done by the caller; let's move both into the
start_httpd function so that all callers don't have to
repeat them (there is only one caller now, but another will
follow in this series).
This doesn't violate any assumptions that aren't already
being made by lib-httpd, which is happy to say "skipping"
and call test_done for a number of other cases.
Signed-off-by: Jeff King <redacted>
---
Cleanup for the next patch.
t/lib-httpd.sh | 9 +++++++--
t/t5540-http-push.sh | 8 +-------
2 files changed, 8 insertions(+), 9 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 22:46:10
This was mostly being tested implicitly by the "http push"
tests. But making a separate test script means that:
- we will run fetch tests even when http pushing support
is not built
- when there are failures on fetching, they are easier to
see and isolate, as they are not in the middle of push
tests
This script defaults to running the webserver on port 5550,
and puts the original t5540 on port 5540, so that the two
can be run simultaneously without conflict (but both still
respect an externally set LIB_HTTPD_PORT).
Signed-off-by: Jeff King <redacted>
---
I started this as test infrastructure for the final patch in the series,
but I think it's nice to have a few http sanity checks in general. Of
course, one must use GIT_TEST_HTTPD to enable them. Junio, is that part
of your usual integration testing?
Makefile | 1 +
t/t5540-http-push.sh | 1 +
t/t5550-http-fetch.sh | 46 ++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 48 insertions(+), 0 deletions(-)
create mode 100755 t/t5550-http-fetch.sh
@@ -11,6 +11,7 @@ This test runs various sanity checks on http-push.'ROOT_PATH="$PWD"LIB_HTTPD_DAV=t+LIB_HTTPD_PORT=${LIB_HTTPD_PORT-'5540'}ifgithttp-push>/dev/null2>&1||[$?-eq128]then
@@ -0,0 +1,46 @@+#!/bin/sh++test_description='test fetching over http'+../test-lib.sh++iftest-n"$NO_CURL";then+say'skipping test, git built without http support'+test_done+fi++."$TEST_DIRECTORY"/lib-httpd.sh+LIB_HTTPD_PORT=${LIB_HTTPD_PORT-'5550'}+start_httpd++test_expect_success'setup repository''+echocontent>file&&+gitaddfile&&+gitcommit-mone+'++test_expect_success'create http-accessible bare repository''+mkdir"$HTTPD_DOCUMENT_ROOT_PATH/repo.git"&&+(cd"$HTTPD_DOCUMENT_ROOT_PATH/repo.git"&&+git--bareinit&&+echo"exec git update-server-info">hooks/post-update&&+chmod+xhooks/post-update+)&&+gitremoteaddpublic"$HTTPD_DOCUMENT_ROOT_PATH/repo.git"&&+gitpushpublicmaster:master+'++test_expect_success'clone http repository''+gitclone$HTTPD_URL/repo.gitclone&&+test_cmpfileclone/file+'++test_expect_success'fetch changes via http''+echocontent>>file&&+gitcommit-a-mtwo&&+gitpushpublic+(cdclone&&gitpull)&&+test_cmpfileclone/file+'++stop_httpd+test_done
From: Jeff King <hidden> Date: 2016-06-15 22:46:10
Since it doesn't actually touch its argument, this makes
sense.
However, we still want to return a non-const version (which
requires a cast) so that this:
struct ref *a, *b;
a = find_ref_by_name(b);
works. Unfortunately, you can also silently strip the const
from a variable:
struct ref *a;
const struct ref *b;
a = find_ref_by_name(b);
This is a classic C const problem because there is no way to
say "return the type with the same constness that was passed
to us"; we provide the same semantics as standard library
functions like strchr.
Signed-off-by: Jeff King <redacted>
---
cache.h | 2 +-
refs.c | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 22:46:10
This function had a lot of complications which made it hard
to read and extend, and confusing to call (because of
optional unused parameters). This patch attempts to address
that:
- we used to manually search through the ref lists; this
should be a one-line call to find_ref_by_name
- guess_remote_head used to do two things: find the HEAD
ref, and then find a matching ref. And only one of the
two callers actually cared about returning the HEAD ref.
Since it's a one-liner, just have the caller do it
themselves (and remember it or not as they wish).
- there were two ways of getting results out of the
function (a return value, and an "all_matched" out
parameter), but no caller cared about both. One of them
returned a pointer into the passed-in ref list and one
of them returned a newly allocated list (with the
peer_ref fields stripped). Let's be simple and
consistent: the return value is always a ref copy with
a valid peer_ref. If "all" is requested, all candidates
are returned. Otherwise, the best candidate is returned.
Signed-off-by: Jeff King <redacted>
---
The diff is horrible to read, and would look much better as a whole
function replacement. It would make a great test case for a "short lines
are uninteresting" diff feature.
builtin-clone.c | 5 +--
builtin-remote.c | 5 ++-
remote.c | 62 ++++++++++++++++++++++-------------------------------
remote.h | 17 ++++++--------
4 files changed, 38 insertions(+), 51 deletions(-)
@@ -508,9 +508,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)clear_extra_refs();mapped_refs=write_remote_refs(refs,&refspec,reflog_msg.buf);--head_points_at=guess_remote_head(refs,mapped_refs,-&remote_head,NULL);+remote_head=find_ref_by_name(refs,"HEAD");+head_points_at=guess_remote_head(remote_head,mapped_refs,0);}else{warning("You appear to have cloned an empty repository.");
@@ -1377,51 +1377,41 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb)return1;}-conststructref*guess_remote_head(conststructref*refs,-conststructref*mapped_refs,-conststructref**remote_head_p,-structref**all_matches_p)+structref*copy_ref_with_peer(conststructref*src){-conststructref*remote_head=NULL;-conststructref*remote_master=NULL;-conststructref*ret=NULL;-conststructref*r;-structref**tail=all_matches_p;--for(r=refs;r;r=r->next)-if(!strcmp(r->name,"HEAD"))-remote_head=r;--if(!all_matches_p)-for(r=mapped_refs;r;r=r->next)-if(!strcmp(r->name,"refs/heads/master"))-remote_master=r;+structref*dst=copy_ref(src);+dst->peer_ref=copy_ref(src->peer_ref);+returndst;+}-if(remote_head_p)-*remote_head_p=remote_head;+structref*guess_remote_head(conststructref*head,+conststructref*refs,+intall)+{+conststructref*r;+structref*list=NULL;+structref**tail=&list;-/* If there's no HEAD value at all, never mind. */-if(!remote_head)+if(!head)returnNULL;/* If refs/heads/master could be right, it is. */-if(remote_master&&!hashcmp(remote_master->old_sha1,-remote_head->old_sha1))-returnremote_master;+if(!all){+conststructref*m;+m=find_ref_by_name(refs,"refs/heads/master");+if(m&&!hashcmp(m->old_sha1,head->old_sha1))+returncopy_ref_with_peer(m);+}/* Look for another ref that points there */-for(r=mapped_refs;r;r=r->next)-if(r!=remote_head&&-!hashcmp(r->old_sha1,remote_head->old_sha1)){-structref*cpy;-if(!ret)-ret=r;-if(!all_matches_p)+for(r=refs;r;r=r->next){+if(r!=head&&!hashcmp(r->old_sha1,head->old_sha1)){+*tail=copy_ref_with_peer(r);+tail=&((*tail)->next);+if(!all)break;-*tail=cpy=copy_ref(r);-cpy->peer_ref=NULL;-tail=&cpy->next;}+}-returnret;+returnlist;}
@@ -138,16 +138,13 @@ int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs);intformat_tracking_info(structbranch*branch,structstrbuf*sb);/*-*LookinrefsforHEAD.ThenlookforamatchingSHA1inmapped_refs,-*firstcheckingifrefs/heads/mastermatches.ReturnNULLifnothingmatches-*orifthereisnoHEADinrefs.remote_head_pisassignedHEADifnotNULL.-*Ifall_matches_pisNULL,returnafterthefirstpossiblematch.Otherwise-*all_matches_pissettoareflistofeachbranchheadwiththesameSHA1as-*HEAD.+*FindrefsfromalistwhicharelikelytobepointedtobythegivenHEAD+*ref.If'all'isfalse,returnsthemostlikelyref;otherwise,returnsa+*listofallcandidaterefs.Ifnomatchisfound(or'head'isNULL),+*returnsNULL.Allreturnsarenewlyallocatedandshouldbefreed.*/-conststructref*guess_remote_head(conststructref*refs,-conststructref*mapped_refs,-conststructref**remote_head_p,-structref**all_matches_p);+structref*guess_remote_head(conststructref*head,+conststructref*refs,+intall);#endif
From: Jeff King <hidden> Date: 2016-06-15 22:46:11
Our usual method for determining the ref pointed to by HEAD
is to compare HEAD's sha1 to the sha1 of all refs, trying to
find a unique match.
However, some transports actually get to look at HEAD
directly; we should make use of that information when it is
available. Currently, only http remotes support this
feature.
Signed-off-by: Jeff King <redacted>
---
A possible 6/5 would be to do something similar for local repos (or
resurrecting the HEAD proposal).
remote.c | 10 ++++++++++
t/t5550-http-fetch.sh | 11 +++++++++++
2 files changed, 21 insertions(+), 0 deletions(-)
@@ -1395,6 +1395,16 @@ struct ref *guess_remote_head(const struct ref *head,if(!head)returnNULL;+/*+*Sometransportssupportdirectlypeekingat+*whereHEADpoints;ifthatisthecase,then+*wedon'thavetoguess.+*/+if(head->symref){+r=find_ref_by_name(refs,head->symref);+returnr?copy_ref_with_peer(r):NULL;+}+/* If refs/heads/master could be right, it is. */if(!all){conststructref*m;
I missed it before, but there is a typo in this test (>&) that causes it
to barf.
Didn't barf for me, but it turns out it's because it's a bash'ism[1],
and that's the default /bin/sh on OS X. Out of curiosity, on what
platform did it fail for you?
[1] Redirecting Standard Output and Standard Error
Bash allows both the standard output (file descriptor 1) and
the standard error
output (file descriptor 2) to be redirected to the file whose
name is the expan-
sion of word with this construct.
There are two formats for redirecting standard output and standard error:
&>word
and
>&word
Of the two forms, the first is preferred. This is semantically
equivalent to
>word 2>&1
j.
From: Jeff King <hidden> Date: 2016-06-15 22:46:12
On Sun, Feb 15, 2009 at 09:13:09AM -0500, Jay Soffian wrote:
quoted
quoted
+ (cd test &&
+ test_must_fail git remote set-head --auto two >& output &&
+ test_cmp expect output)
I missed it before, but there is a typo in this test (>&) that causes it
to barf.
Didn't barf for me, but it turns out it's because it's a bash'ism[1],
and that's the default /bin/sh on OS X. Out of curiosity, on what
platform did it fail for you?
Ah, I didn't realize that was valid under any shell. ;) My /bin/sh is
dash, which explains it.
-Peff
From: Jeff King <hidden> Date: 2016-06-15 22:46:12
On Sun, Feb 15, 2009 at 01:18:18AM -0500, Jeff King wrote:
A possible 6/5 would be to do something similar for local repos (or
resurrecting the HEAD proposal).
Here is a quick and dirty series to unambiguously determine the HEAD for
local repos, but I am undecided on whether this is actually a good idea.
Note that this fails Jay's tests in t5505 which expect the ambiguity; to
be considered for inclusion, it would need to test "../two" as a remote
as well as "file://$(pwd)/../two", making sure each behaved correctly.
But I am posting it here to stimulate discussion on whether it is even
something we want.
1/2: transport: cleanup duplicated ref fetching code
2/2: transport: unambiguously determine local HEAD
-Peff
From: Jeff King <hidden> Date: 2016-06-15 22:46:12
When fetching refs through the git protocol, the
fetch_refs_via_pack will establish the connection and get
the ref list if it has not already been done.
Since the code is only two lines, it was done inline rather
than calling the transport's get_refs function. However,
calling that function better matches the intent, and is
future-proof against enhancements in get_refs_via_connect.
Signed-off-by: Jeff King <redacted>
---
Enhancements like the one that is coming in the next patch.
Though I think that fetch_pack doesn't currently care if the HEAD symref
is set up, it makes sense to me to be consistent.
transport.c | 6 ++----
1 files changed, 2 insertions(+), 4 deletions(-)
@@ -646,10 +646,8 @@ static int fetch_refs_via_pack(struct transport *transport,for(i=0;i<nr_heads;i++)origh[i]=heads[i]=xstrdup(to_fetch[i]->name);-if(!data->conn){-connect_setup(transport);-get_remote_heads(data->fd[0],&refs_tmp,0,NULL,0,NULL);-}+if(!data->conn)+refs_tmp=transport->get_refs_list(transport);refs=fetch_pack(&args,data->fd,data->conn,refs_tmp?refs_tmp:transport->remote_refs,
From: Jeff King <hidden> Date: 2016-06-15 22:46:12
When we fetch refs using the git protocol, we have to guess
at which ref is pointed to by the HEAD. In the case of a
local filesystem repo, however, we can cheat by going to
that repo and peeking directly at the contents of HEAD.
Signed-off-by: Jeff King <redacted>
---
Again, this fails tests in t5505, and is not meant for inclusion.
transport.c | 39 +++++++++++++++++++++++++++++++++++++++
1 files changed, 39 insertions(+), 0 deletions(-)
@@ -12,6 +12,8 @@#include"dir.h"#include"refs.h"+staticintis_local(constchar*url);+/* rsync support *//*
@@ -609,6 +611,40 @@ static int connect_setup(struct transport *transport)return0;}+staticvoidmark_head_via_local(constchar*url,structref*refs)+{+staticconstchar*argv[]={"symbolic-ref","HEAD",NULL};+staticconstchar*env[]={GIT_DIR_ENVIRONMENT,NULL};+structchild_processcmd;+structref*head;+structstrbufbuf=STRBUF_INIT;++head=find_ref_by_name(refs,"HEAD");+if(!head)+return;++memset(&cmd,0,sizeofcmd);+cmd.argv=argv;+cmd.env=env;+cmd.dir=url;+cmd.git_cmd=1;+cmd.no_stdin=1;+cmd.no_stderr=1;+cmd.out=-1;++if(start_command(&cmd)<0)+return;+if(strbuf_read(&buf,cmd.out,64)<0)+return;+if(finish_command(&cmd)!=0){+strbuf_release(&buf);+return;+}++strbuf_trim(&buf);+head->symref=strbuf_detach(&buf,NULL);+}+staticstructref*get_refs_via_connect(structtransport*transport){structgit_transport_data*data=transport->data;
@@ -617,6 +653,9 @@ static struct ref *get_refs_via_connect(struct transport *transport)connect_setup(transport);get_remote_heads(data->fd[0],&refs,0,NULL,0,NULL);+if(is_local(transport->url))+mark_head_via_local(transport->url,refs);+returnrefs;}