* [PATCH 0/4] remote HEAD improvements take 2 @ 2009-02-13 8:54 Jay Soffian 2009-02-13 8:54 ` [PATCH 1/4] builtin-clone: move locate_head() to remote.c so it can be re-used Jay Soffian 2009-02-13 8:57 ` [PATCH 0/4] remote HEAD improvements take 2 Jay Soffian 0 siblings, 2 replies; 41+ messages in thread From: Jay Soffian @ 2009-02-13 8:54 UTC (permalink / raw) To: git; +Cc: Jay Soffian, peff, gitster, barkalow There is currently no porcelain for dealing with remote HEADs (i.e. $GIT_DIR/remotes/<remote>/HEAD). This series: 1) Refactors locate_head() from builtin-clone.c to remote.c so it can be used by builtin-remote.c as well. I also renamed it to guess_remote_head(). Daniel suggested having it specifically check that it returns a ref from refs/heads/, but I wasn't sure what impact that might have (good or bad...), so I punted on that change. 2) Teaches git remote show to display the remote HEAD: $ git remote show origin * remote origin URL: git://git.kernel.org/pub/scm/git/git.git HEAD: master 3) Teaches git remote a new "set-head" verb: To set a remote HEAD explicitly: $ git remote set-head <name> <branch> To set a remote HEAD to match the upstream repo: $ git remote set-head <name> -a To delete a remote HEAD: $ git remote set-head <name> -d I changed it from "sethead" to "set-head" per Jeff. I also remembered to update git-completion.bash this time. 4) Documents the new set-head verb. I also correct the git remote man page w/respect to the "-m <master>" option. The man page implied that the remote HEAD was set automatically when adding a remote (a la git clone), but this is not true. And, since I couldn't find anywhere else that the point of having a remote HEAD is documented, I documented it here. Jay Soffian (4): builtin-clone: move locate_head() to remote.c so it can be re-used builtin-remote: move duplicated cleanup code its own function builtin-remote: teach show to display remote HEAD builtin-remote: add set-head verb Documentation/git-remote.txt | 20 ++++++- builtin-clone.c | 41 +------------- builtin-remote.c | 96 +++++++++++++++++++++++++++++--- contrib/completion/git-completion.bash | 2 +- remote.c | 37 ++++++++++++ remote.h | 9 +++ 6 files changed, 156 insertions(+), 49 deletions(-) ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 1/4] builtin-clone: move locate_head() to remote.c so it can be re-used 2009-02-13 8:54 [PATCH 0/4] remote HEAD improvements take 2 Jay Soffian @ 2009-02-13 8:54 ` Jay Soffian 2009-02-13 8:54 ` [PATCH 2/4] builtin-remote: move duplicated cleanup code its own function Jay Soffian 2009-02-13 8:57 ` [PATCH 0/4] remote HEAD improvements take 2 Jay Soffian 1 sibling, 1 reply; 41+ messages in thread From: Jay Soffian @ 2009-02-13 8:54 UTC (permalink / raw) To: git; +Cc: Jay Soffian, peff, gitster, barkalow Move locate_head() to remote.c and rename it to guess_remote_head() to more accurately reflect what it does. This is in preparation for being able to call it from builtin-remote.c Signed-off-by: Jay Soffian <jaysoffian@gmail.com> --- builtin-clone.c | 41 +++-------------------------------------- remote.c | 37 +++++++++++++++++++++++++++++++++++++ remote.h | 9 +++++++++ 3 files changed, 49 insertions(+), 38 deletions(-) diff --git a/builtin-clone.c b/builtin-clone.c index f73029e..280b866 100644 --- a/builtin-clone.c +++ b/builtin-clone.c @@ -20,6 +20,7 @@ #include "dir.h" #include "pack-refs.h" #include "sigchain.h" +#include "remote.h" /* * Overall FIXMEs: @@ -293,43 +294,6 @@ static void remove_junk_on_signal(int signo) raise(signo); } -static const struct ref *locate_head(const struct ref *refs, - const struct ref *mapped_refs, - const struct ref **remote_head_p) -{ - const struct ref *remote_head = NULL; - const struct ref *remote_master = NULL; - const struct ref *r; - for (r = refs; r; r = r->next) - if (!strcmp(r->name, "HEAD")) - remote_head = r; - - for (r = mapped_refs; r; r = r->next) - if (!strcmp(r->name, "refs/heads/master")) - remote_master = r; - - if (remote_head_p) - *remote_head_p = remote_head; - - /* If there's no HEAD value at all, never mind. */ - if (!remote_head) - return NULL; - - /* If refs/heads/master could be right, it is. */ - if (remote_master && !hashcmp(remote_master->old_sha1, - remote_head->old_sha1)) - return remote_master; - - /* 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)) - return r; - - /* Nothing is the same */ - return NULL; -} - static struct ref *write_remote_refs(const struct ref *refs, struct refspec *refspec, const char *reflog) { @@ -532,7 +496,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix) mapped_refs = write_remote_refs(refs, &refspec, reflog_msg.buf); - head_points_at = locate_head(refs, mapped_refs, &remote_head); + head_points_at = guess_remote_head(refs, mapped_refs, + &remote_head); } else { warning("You appear to have cloned an empty repository."); diff --git a/remote.c b/remote.c index d7079c6..447f091 100644 --- a/remote.c +++ b/remote.c @@ -1376,3 +1376,40 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb) base, num_ours, num_theirs); return 1; } + +const struct ref *guess_remote_head(const struct ref *refs, + const struct ref *mapped_refs, + const struct ref **remote_head_p) +{ + const struct ref *remote_head = NULL; + const struct ref *remote_master = NULL; + const struct ref *r; + for (r = refs; r; r = r->next) + if (!strcmp(r->name, "HEAD")) + remote_head = r; + + for (r = mapped_refs; r; r = r->next) + if (!strcmp(r->name, "refs/heads/master")) + remote_master = r; + + if (remote_head_p) + *remote_head_p = remote_head; + + /* If there's no HEAD value at all, never mind. */ + if (!remote_head) + return NULL; + + /* If refs/heads/master could be right, it is. */ + if (remote_master && !hashcmp(remote_master->old_sha1, + remote_head->old_sha1)) + return remote_master; + + /* 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)) + return r; + + /* Nothing is the same */ + return NULL; +} diff --git a/remote.h b/remote.h index a46a5be..cabb14a 100644 --- a/remote.h +++ b/remote.h @@ -137,4 +137,13 @@ enum match_refs_flags { int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs); int format_tracking_info(struct branch *branch, struct strbuf *sb); +/* + * Look in refs for HEAD. Then look for a matching SHA1 in mapped_refs, + * first checking if refs/heads/master matches. Return NULL if nothing matches + * or if there is no HEAD in refs. remote_head_p is assigned HEAD if not NULL. + */ +const struct ref *guess_remote_head(const struct ref *refs, + const struct ref *mapped_refs, + const struct ref **remote_head_p); + #endif -- 1.6.2.rc0.209.g7c178 ^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH 2/4] builtin-remote: move duplicated cleanup code its own function 2009-02-13 8:54 ` [PATCH 1/4] builtin-clone: move locate_head() to remote.c so it can be re-used Jay Soffian @ 2009-02-13 8:54 ` Jay Soffian 2009-02-13 8:54 ` [PATCH 3/4] builtin-remote: teach show to display remote HEAD Jay Soffian 0 siblings, 1 reply; 41+ messages in thread From: Jay Soffian @ 2009-02-13 8:54 UTC (permalink / raw) To: git; +Cc: Jay Soffian, peff, gitster, barkalow Moved some identical lines of code into their own function in preparation for adding additional functionality which will use this function as well. Also removed a bogus NEEDSWORK comment per Daniel Barkalow: Actually, the comment is wrong; "remote" comes from remote_get(), which returns things from a cache in remote.c; there could be a remote_put() to let the code know that the caller is done with the object, but it wouldn't presently do anything. Signed-off-by: Jay Soffian <jaysoffian@gmail.com> --- builtin-remote.c | 17 +++++++++-------- 1 files changed, 9 insertions(+), 8 deletions(-) diff --git a/builtin-remote.c b/builtin-remote.c index ac69d37..b89a353 100644 --- a/builtin-remote.c +++ b/builtin-remote.c @@ -632,6 +632,13 @@ static void show_list(const char *title, struct string_list *list, printf(" %s\n", list->items[i].string); } +static void free_remote_ref_states(struct ref_states *states) +{ + string_list_clear(&states->new, 0); + string_list_clear(&states->stale, 0); + string_list_clear(&states->tracked, 0); +} + static int get_remote_ref_states(const char *name, struct ref_states *states, int query) @@ -738,10 +745,7 @@ static int show(int argc, const char **argv) } } - /* NEEDSWORK: free remote */ - string_list_clear(&states.new, 0); - string_list_clear(&states.stale, 0); - string_list_clear(&states.tracked, 0); + free_remote_ref_states(&states); } return result; @@ -792,10 +796,7 @@ static int prune(int argc, const char **argv) warn_dangling_symref(dangling_msg, refname); } - /* NEEDSWORK: free remote */ - string_list_clear(&states.new, 0); - string_list_clear(&states.stale, 0); - string_list_clear(&states.tracked, 0); + free_remote_ref_states(&states); } return result; -- 1.6.2.rc0.209.g7c178 ^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH 3/4] builtin-remote: teach show to display remote HEAD 2009-02-13 8:54 ` [PATCH 2/4] builtin-remote: move duplicated cleanup code its own function Jay Soffian @ 2009-02-13 8:54 ` Jay Soffian 2009-02-13 8:54 ` [PATCH 4/4] builtin-remote: add set-head verb Jay Soffian 0 siblings, 1 reply; 41+ messages in thread From: Jay Soffian @ 2009-02-13 8:54 UTC (permalink / raw) To: git; +Cc: Jay Soffian, peff, gitster, barkalow This is in preparation for teaching remote how to set refs/remotes/<remote>/HEAD to match what HEAD is set to at <remote>, but is useful in its own right. Signed-off-by: Jay Soffian <jaysoffian@gmail.com> --- builtin-remote.c | 26 ++++++++++++++++++++++++++ 1 files changed, 26 insertions(+), 0 deletions(-) diff --git a/builtin-remote.c b/builtin-remote.c index b89a353..465c87a 100644 --- a/builtin-remote.c +++ b/builtin-remote.c @@ -212,6 +212,7 @@ static void read_branches(void) struct ref_states { struct remote *remote; struct string_list new, stale, tracked; + char *head_name; }; static int handle_one_branch(const char *refname, @@ -271,6 +272,26 @@ static int get_ref_states(const struct ref *ref, struct ref_states *states) return 0; } +static char *get_head_name(const struct ref *refs) +{ + const struct ref *head_points_at; + struct ref *mapped_refs = NULL; + struct ref **tail = &mapped_refs; + struct refspec refspec; + + refspec.force = 0; + refspec.pattern = 1; + refspec.src = refspec.dst = "refs/heads/"; + + get_fetch_map(refs, &refspec, &tail, 0); + + head_points_at = guess_remote_head(refs, mapped_refs, NULL); + if (head_points_at) + return xstrdup(abbrev_branch(head_points_at->name)); + + return NULL; +} + struct known_remote { struct known_remote *next; struct remote *remote; @@ -637,6 +658,7 @@ static void free_remote_ref_states(struct ref_states *states) string_list_clear(&states->new, 0); string_list_clear(&states->stale, 0); string_list_clear(&states->tracked, 0); + free(states->head_name); } static int get_remote_ref_states(const char *name, @@ -658,6 +680,7 @@ static int get_remote_ref_states(const char *name, ref = transport_get_remote_refs(transport); transport_disconnect(transport); + states->head_name = get_head_name(ref); get_ref_states(ref, states); } @@ -702,6 +725,9 @@ static int show(int argc, const char **argv) printf("* remote %s\n URL: %s\n", *argv, states.remote->url_nr > 0 ? states.remote->url[0] : "(no URL)"); + if (!no_query) + printf(" HEAD: %s\n", states.head_name ? + states.head_name : "(unknown)"); for (i = 0; i < branch_list.nr; i++) { struct string_list_item *branch = branch_list.items + i; -- 1.6.2.rc0.209.g7c178 ^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH 4/4] builtin-remote: add set-head verb 2009-02-13 8:54 ` [PATCH 3/4] builtin-remote: teach show to display remote HEAD Jay Soffian @ 2009-02-13 8:54 ` Jay Soffian 2009-02-13 10:09 ` Junio C Hamano 0 siblings, 1 reply; 41+ messages in thread From: Jay Soffian @ 2009-02-13 8:54 UTC (permalink / raw) To: git; +Cc: Jay Soffian, peff, gitster, barkalow Provide a porcelain command for setting/deleting $GIT_DIR/remotes/<remote>/HEAD. While we're at it, document what $GIT_DIR/remotes/<remote>/HEAD is all about. Signed-off-by: Jay Soffian <jaysoffian@gmail.com> --- Documentation/git-remote.txt | 20 ++++++++++- builtin-remote.c | 54 +++++++++++++++++++++++++++++++- contrib/completion/git-completion.bash | 2 +- 3 files changed, 72 insertions(+), 4 deletions(-) diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt index fad983e..80f2cfe 100644 --- a/Documentation/git-remote.txt +++ b/Documentation/git-remote.txt @@ -13,6 +13,7 @@ SYNOPSIS 'git remote add' [-t <branch>] [-m <master>] [-f] [--mirror] <name> <url> 'git remote rename' <old> <new> 'git remote rm' <name> +'git remote set-head' <name> [-a | -d | <branch>] 'git remote show' [-n] <name> 'git remote prune' [-n | --dry-run] <name> 'git remote update' [group] @@ -53,8 +54,7 @@ is created. You can give more than one `-t <branch>` to track multiple branches without grabbing all branches. + With `-m <master>` option, `$GIT_DIR/remotes/<name>/HEAD` is set -up to point at remote's `<master>` branch instead of whatever -branch the `HEAD` at the remote repository actually points at. +up to point at remote's `<master>` branch. See also the set-head command. + In mirror mode, enabled with `\--mirror`, the refs will not be stored in the 'refs/remotes/' namespace, but in 'refs/heads/'. This option @@ -76,6 +76,22 @@ the configuration file format. Remove the remote named <name>. All remote tracking branches and configuration settings for the remote are removed. +'set-head':: + +Sets or deletes the default branch (`$GIT_DIR/remotes/<name>/HEAD`) for +the named remote. Having a default branch for a remote is not required, +but allows the name of the remote to be specified in lieu of a specific +branch. For example, if the default branch for `origin` is set to +`master`, then `origin` may be specified wherever you would normally +specify `origin/master`. ++ +With `-d`, `$GIT_DIR/remotes/<name>/HEAD` is deleted. ++ +With `-a`, the remote is queried to determine its `HEAD`, then +`$GIT_DIR/remotes/<name>/HEAD` is set to the same branch. ++ +Use `<branch>` to set `$GIT_DIR/remotes/<name>/HEAD` explicitly. + 'show':: Gives some information about the remote <name>. diff --git a/builtin-remote.c b/builtin-remote.c index 465c87a..677e20e 100644 --- a/builtin-remote.c +++ b/builtin-remote.c @@ -12,6 +12,7 @@ static const char * const builtin_remote_usage[] = { "git remote add [-t <branch>] [-m <master>] [-f] [--mirror] <name> <url>", "git remote rename <old> <new>", "git remote rm <name>", + "git remote set-head <name> [-a | -d | <branch>]", "git remote show [-n] <name>", "git remote prune [-n | --dry-run] <name>", "git remote [-v | --verbose] update [group]", @@ -658,7 +659,8 @@ static void free_remote_ref_states(struct ref_states *states) string_list_clear(&states->new, 0); string_list_clear(&states->stale, 0); string_list_clear(&states->tracked, 0); - free(states->head_name); + if (states->head_name) + free(states->head_name); } static int get_remote_ref_states(const char *name, @@ -777,6 +779,54 @@ static int show(int argc, const char **argv) return result; } +static int sethead(int argc, const char **argv) +{ + int opt_a = 0, opt_d = 0, result = 0; + struct strbuf buf = STRBUF_INIT, buf2 = STRBUF_INIT; + char *head_name = NULL; + + struct option options[] = { + OPT_GROUP("set-head specific options"), + OPT_BOOLEAN('a', 0, &opt_a, + "set refs/remotes/<name>/HEAD according to remote"), + OPT_BOOLEAN('d', 0, &opt_d, "delete refs/remotes/<name>/HEAD"), + OPT_END() + }; + argc = parse_options(argc, argv, options, builtin_remote_usage, 0); + if ((argc == 1 && !(opt_a || opt_d)) || + ((argc == 2 && (opt_a || opt_d))) || argc < 1 || argc > 2) + usage_with_options(builtin_remote_usage, options); + + strbuf_addf(&buf, "refs/remotes/%s/HEAD", argv[0]); + + if (opt_d) { + if (result |= delete_ref(buf.buf, NULL, REF_NODEREF)) + error("Could not delete %s", buf.buf); + } else if (opt_a) { + struct ref_states states; + memset(&states, 0, sizeof(states)); + get_remote_ref_states(argv[0], &states, 1); + head_name = xstrdup(states.head_name); + free_remote_ref_states(&states); + } else + head_name = xstrdup(argv[1]); + + if (head_name) { + unsigned char sha1[20]; + strbuf_addf(&buf2, "refs/remotes/%s/%s", argv[0], head_name); + /* make sure it's valid */ + if (!resolve_ref(buf2.buf, sha1, 1, NULL)) + result |= error("Not a valid ref: %s", buf2.buf); + else if (create_symref(buf.buf, buf2.buf, "remote set-head")) + result |= error("Could not setup %s", buf.buf); + free(head_name); + } + + strbuf_release(&buf); + strbuf_release(&buf2); + return result; +} + static int prune(int argc, const char **argv) { int dry_run = 0, result = 0; @@ -947,6 +997,8 @@ int cmd_remote(int argc, const char **argv, const char *prefix) result = mv(argc, argv); else if (!strcmp(argv[0], "rm")) result = rm(argc, argv); + else if (!strcmp(argv[0], "set-head")) + result = sethead(argc, argv); else if (!strcmp(argv[0], "show")) result = show(argc, argv); else if (!strcmp(argv[0], "prune")) diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index f44f63c..5de1922 100755 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -1414,7 +1414,7 @@ _git_config () _git_remote () { - local subcommands="add rename rm show prune update" + local subcommands="add rename rm show prune update set-head" local subcommand="$(__git_find_subcommand "$subcommands")" if [ -z "$subcommand" ]; then __gitcomp "$subcommands" -- 1.6.2.rc0.209.g7c178 ^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [PATCH 4/4] builtin-remote: add set-head verb 2009-02-13 8:54 ` [PATCH 4/4] builtin-remote: add set-head verb Jay Soffian @ 2009-02-13 10:09 ` Junio C Hamano 2009-02-13 10:21 ` Jay Soffian ` (2 more replies) 0 siblings, 3 replies; 41+ messages in thread From: Junio C Hamano @ 2009-02-13 10:09 UTC (permalink / raw) To: Jay Soffian; +Cc: git, peff, barkalow Jay Soffian <jaysoffian@gmail.com> writes: > Provide a porcelain command for setting/deleting > $GIT_DIR/remotes/<remote>/HEAD. The entire series looks sane from a very cursory look; especially the earlier ones are obviously good. Calling the subcommand a "verb" is somewhat new, though. Existing documentation for git commands that take multiple actions seem to call them subcommands, including "git-remote.txt" itself. > diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt > index fad983e..80f2cfe 100644 > --- a/Documentation/git-remote.txt > +++ b/Documentation/git-remote.txt > @@ -76,6 +76,22 @@ the configuration file format. > Remove the remote named <name>. All remote tracking branches and > configuration settings for the remote are removed. > > +'set-head':: > + > +Sets or deletes the default branch (`$GIT_DIR/remotes/<name>/HEAD`) for > +the named remote. Having a default branch for a remote is not required, > +but allows the name of the remote to be specified in lieu of a specific > +branch. For example, if the default branch for `origin` is set to > +`master`, then `origin` may be specified wherever you would normally > +specify `origin/master`. > ++ > +With `-d`, `$GIT_DIR/remotes/<name>/HEAD` is deleted. > ++ > +With `-a`, the remote is queried to determine its `HEAD`, then > +`$GIT_DIR/remotes/<name>/HEAD` is set to the same branch. > ++ > +Use `<branch>` to set `$GIT_DIR/remotes/<name>/HEAD` explicitly. > + Hmph, what does "-a" stand for? I would have expected to see "-u" that stands for "update" here. Also it may be better to be more explicit about both the syntax and the semantics of `<branch>`. Do you expect "refs/remotes/<name>/master" or just "master" (I assume the latter)? Is it an error if the branch does not exist in the specified hierarchy? Can you force to set to a branch that does not exist in your tracking side (yet) but you know exists on the remote side already? > diff --git a/builtin-remote.c b/builtin-remote.c > index 465c87a..677e20e 100644 > --- a/builtin-remote.c > +++ b/builtin-remote.c > @@ -658,7 +659,8 @@ static void free_remote_ref_states(struct ref_states *states) > string_list_clear(&states->new, 0); > string_list_clear(&states->stale, 0); > string_list_clear(&states->tracked, 0); > - free(states->head_name); > + if (states->head_name) > + free(states->head_name); > } Regression? > @@ -777,6 +779,54 @@ static int show(int argc, const char **argv) > return result; > } > > +static int sethead(int argc, const char **argv) set_head()? > +{ > + int opt_a = 0, opt_d = 0, result = 0; > + struct strbuf buf = STRBUF_INIT, buf2 = STRBUF_INIT; > + char *head_name = NULL; > + > + struct option options[] = { > + OPT_GROUP("set-head specific options"), > + OPT_BOOLEAN('a', 0, &opt_a, > + "set refs/remotes/<name>/HEAD according to remote"), > + OPT_BOOLEAN('d', 0, &opt_d, "delete refs/remotes/<name>/HEAD"), > + OPT_END() > + }; > + argc = parse_options(argc, argv, options, builtin_remote_usage, 0); > + if ((argc == 1 && !(opt_a || opt_d)) || > + ((argc == 2 && (opt_a || opt_d))) || argc < 1 || argc > 2) > + usage_with_options(builtin_remote_usage, options); The code will scale better, especially for a young subcommand that may acquire new options, if the check is done by each codepath that deals with a specific option to do this kind of check. That is, e.g. if (opt_delete) { error if the arg is not remote (alone) do the "delete" thing } else if (opt_update) { error if the arg is not remote (alone) do the "update" thing } else { error if the args are not (remote, branch) do the "set" thing } ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 4/4] builtin-remote: add set-head verb 2009-02-13 10:09 ` Junio C Hamano @ 2009-02-13 10:21 ` Jay Soffian 2009-02-13 11:42 ` [PATCH v2 4/4] builtin-remote: add set-head subcommand Jay Soffian 2009-02-13 10:35 ` [PATCH 4/4] builtin-remote: add set-head verb Junio C Hamano 2009-02-14 0:22 ` Jeff King 2 siblings, 1 reply; 41+ messages in thread From: Jay Soffian @ 2009-02-13 10:21 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, peff, barkalow On Fri, Feb 13, 2009 at 5:09 AM, Junio C Hamano <gitster@pobox.com> wrote: > Calling the subcommand a "verb" is somewhat new, though. Existing > documentation for git commands that take multiple actions seem to call > them subcommands, including "git-remote.txt" itself. Okay. > Hmph, what does "-a" stand for? I would have expected to see "-u" that > stands for "update" here. --automatic -- as in figure out the name automatically from the other side. > Also it may be better to be more explicit about both the syntax and the > semantics of `<branch>`. Okay. > Do you expect "refs/remotes/<name>/master" or > just "master" (I assume the latter)? Yes, the latter. If you did the wrong thing the error ought clue you in: $ ./git remote set-head origin refs/remotes/origin/master error: Not a valid ref: refs/remotes/origin/refs/remotes/origin/master > Is it an error if the branch does > not exist in the specified hierarchy? Yes it is an error per-above. Well, at least on-top of next it is. > Can you force to set to a branch > that does not exist in your tracking side (yet) but you know exists on the > remote side already? No. >> diff --git a/builtin-remote.c b/builtin-remote.c >> index 465c87a..677e20e 100644 >> --- a/builtin-remote.c >> +++ b/builtin-remote.c >> @@ -658,7 +659,8 @@ static void free_remote_ref_states(struct ref_states *states) >> string_list_clear(&states->new, 0); >> string_list_clear(&states->stale, 0); >> string_list_clear(&states->tracked, 0); >> - free(states->head_name); >> + if (states->head_name) >> + free(states->head_name); >> } > > Regression? Indeed. > set_head()? Yep. > The code will scale better, especially for a young subcommand that may acquire > new options, if the check is done by each codepath that deals with a > specific option to do this kind of check. That is, e.g. > > if (opt_delete) { > error if the arg is not remote (alone) > do the "delete" thing > } else if (opt_update) { > error if the arg is not remote (alone) > do the "update" thing > } else { > error if the args are not (remote, branch) > do the "set" thing > } Got it. I really am trying to match existing code, but it seems the standards have gotten higher, so I need to do better than existing code. Thanks, j. ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH v2 4/4] builtin-remote: add set-head subcommand 2009-02-13 10:21 ` Jay Soffian @ 2009-02-13 11:42 ` Jay Soffian 0 siblings, 0 replies; 41+ messages in thread From: Jay Soffian @ 2009-02-13 11:42 UTC (permalink / raw) To: git; +Cc: Jay Soffian, gitster, j.sixt, madduck Provide a porcelain command for setting/deleting $GIT_DIR/remotes/<remote>/HEAD. While we're at it, document what $GIT_DIR/remotes/<remote>/HEAD is all about. Signed-off-by: Jay Soffian <jaysoffian@gmail.com> --- This replaces "[PATCH 4/4] builtin-remote: add set-head verb". The rest of the series is unchanged. * Changed commit title to call it a subcommand, not a verb. * Clarified documentation slightly * Added --auto and --delete long options to make what -a and -d stand for more obvious. * Fixed small regression where I was checking a pointer for NULL before free()'ing it. * sethead() -> set_head() * Reworked the argument processing in set_head() to be more maintainable if additional switches are added later. On Fri, Feb 13, 2009 at 5:35 AM, Junio C Hamano <gitster@pobox.com> wrote: > You need to customize it to > favor the one that the HEAD points at on the local side before you start > (iow, try to keep the current value when you can). > > For example, if it points at "frotz" locally when the command was started, > and you found out that HEAD now points at the commit at the tip of "frotz" > and "master" branches by peeking, you do not want to repoint HEAD from > "frotz" to "master". This I did not do, and I still don't feel it is necessary. However, if you feel strongly about it I can do it as an additional patch to this series. But I'd like to see this in pu at least first. I really hope not doing it isn't a roadblock for the whole series. Documentation/git-remote.txt | 28 ++++++++++++++++- builtin-remote.c | 51 ++++++++++++++++++++++++++++++++ contrib/completion/git-completion.bash | 2 +- 3 files changed, 78 insertions(+), 3 deletions(-) diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt index fad983e..c9c0e6f 100644 --- a/Documentation/git-remote.txt +++ b/Documentation/git-remote.txt @@ -13,6 +13,7 @@ SYNOPSIS 'git remote add' [-t <branch>] [-m <master>] [-f] [--mirror] <name> <url> 'git remote rename' <old> <new> 'git remote rm' <name> +'git remote set-head' <name> [-a | -d | <branch>] 'git remote show' [-n] <name> 'git remote prune' [-n | --dry-run] <name> 'git remote update' [group] @@ -53,8 +54,7 @@ is created. You can give more than one `-t <branch>` to track multiple branches without grabbing all branches. + With `-m <master>` option, `$GIT_DIR/remotes/<name>/HEAD` is set -up to point at remote's `<master>` branch instead of whatever -branch the `HEAD` at the remote repository actually points at. +up to point at remote's `<master>` branch. See also the set-head command. + In mirror mode, enabled with `\--mirror`, the refs will not be stored in the 'refs/remotes/' namespace, but in 'refs/heads/'. This option @@ -76,6 +76,30 @@ the configuration file format. Remove the remote named <name>. All remote tracking branches and configuration settings for the remote are removed. +'set-head':: + +Sets or deletes the default branch (`$GIT_DIR/remotes/<name>/HEAD`) for +the named remote. Having a default branch for a remote is not required, +but allows the name of the remote to be specified in lieu of a specific +branch. For example, if the default branch for `origin` is set to +`master`, then `origin` may be specified wherever you would normally +specify `origin/master`. ++ +With `-d`, `$GIT_DIR/remotes/<name>/HEAD` is deleted. ++ +With `-a`, the remote is queried to determine its `HEAD`, then +`$GIT_DIR/remotes/<name>/HEAD` is set to the same branch. e.g., if the remote +`HEAD` is pointed at `next`, "`git remote set-head origin -a`" will set +`$GIT_DIR/refs/remotes/origin/HEAD` to `refs/remotes/origin/next`. This will +only work if `refs/remotes/origin/next` already exists; if not it must be +fetched first. ++ +Use `<branch>` to set `$GIT_DIR/remotes/<name>/HEAD` explicitly. e.g., "git +remote set-head origin master" will set `$GIT_DIR/refs/remotes/origin/HEAD` to +`refs/remotes/origin/master`. This will only work if +`refs/remotes/origin/master` already exists; if not it must be fetched first. ++ + 'show':: Gives some information about the remote <name>. diff --git a/builtin-remote.c b/builtin-remote.c index 465c87a..fcb166b 100644 --- a/builtin-remote.c +++ b/builtin-remote.c @@ -12,6 +12,7 @@ static const char * const builtin_remote_usage[] = { "git remote add [-t <branch>] [-m <master>] [-f] [--mirror] <name> <url>", "git remote rename <old> <new>", "git remote rm <name>", + "git remote set-head <name> [-a | -d | <branch>]", "git remote show [-n] <name>", "git remote prune [-n | --dry-run] <name>", "git remote [-v | --verbose] update [group]", @@ -777,6 +778,54 @@ static int show(int argc, const char **argv) return result; } +static int set_head(int argc, const char **argv) +{ + int opt_a = 0, opt_d = 0, result = 0; + struct strbuf buf = STRBUF_INIT, buf2 = STRBUF_INIT; + char *head_name = NULL; + + struct option options[] = { + OPT_GROUP("set-head specific options"), + OPT_BOOLEAN('a', "auto", &opt_a, + "set refs/remotes/<name>/HEAD according to remote"), + OPT_BOOLEAN('d', "delete", &opt_d, + "delete refs/remotes/<name>/HEAD"), + OPT_END() + }; + argc = parse_options(argc, argv, options, builtin_remote_usage, 0); + if (argc) + strbuf_addf(&buf, "refs/remotes/%s/HEAD", argv[0]); + + if (!opt_a && !opt_d && argc == 2) { + head_name = xstrdup(argv[1]); + } else if (opt_a && !opt_d && argc == 1) { + struct ref_states states; + memset(&states, 0, sizeof(states)); + get_remote_ref_states(argv[0], &states, 1); + head_name = xstrdup(states.head_name); + free_remote_ref_states(&states); + } else if (opt_d && !opt_a && argc == 1) { + if (delete_ref(buf.buf, NULL, REF_NODEREF)) + result |= error("Could not delete %s", buf.buf); + } else + usage_with_options(builtin_remote_usage, options); + + if (head_name) { + unsigned char sha1[20]; + strbuf_addf(&buf2, "refs/remotes/%s/%s", argv[0], head_name); + /* make sure it's valid */ + if (!resolve_ref(buf2.buf, sha1, 1, NULL)) + result |= error("Not a valid ref: %s", buf2.buf); + else if (create_symref(buf.buf, buf2.buf, "remote set-head")) + result |= error("Could not setup %s", buf.buf); + free(head_name); + } + + strbuf_release(&buf); + strbuf_release(&buf2); + return result; +} + static int prune(int argc, const char **argv) { int dry_run = 0, result = 0; @@ -947,6 +996,8 @@ int cmd_remote(int argc, const char **argv, const char *prefix) result = mv(argc, argv); else if (!strcmp(argv[0], "rm")) result = rm(argc, argv); + else if (!strcmp(argv[0], "set-head")) + result = set_head(argc, argv); else if (!strcmp(argv[0], "show")) result = show(argc, argv); else if (!strcmp(argv[0], "prune")) diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index f44f63c..5de1922 100755 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -1414,7 +1414,7 @@ _git_config () _git_remote () { - local subcommands="add rename rm show prune update" + local subcommands="add rename rm show prune update set-head" local subcommand="$(__git_find_subcommand "$subcommands")" if [ -z "$subcommand" ]; then __gitcomp "$subcommands" -- 1.6.2.rc0.67.g77afc ^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [PATCH 4/4] builtin-remote: add set-head verb 2009-02-13 10:09 ` Junio C Hamano 2009-02-13 10:21 ` Jay Soffian @ 2009-02-13 10:35 ` Junio C Hamano 2009-02-13 10:52 ` Jay Soffian 2009-02-14 0:22 ` Jeff King 2 siblings, 1 reply; 41+ messages in thread From: Junio C Hamano @ 2009-02-13 10:35 UTC (permalink / raw) To: Jay Soffian; +Cc: git, peff, barkalow Junio C Hamano <gitster@pobox.com> writes: > Also it may be better to be more explicit about both the syntax and the > semantics of `<branch>`. Do you expect "refs/remotes/<name>/master" or > just "master" (I assume the latter)? Is it an error if the branch does > not exist in the specified hierarchy? Can you force to set to a branch > that does not exist in your tracking side (yet) but you know exists on the > remote side already? A few things I forgot (and before I go to bed). If remotes/<name>/HEAD already points at a branch frotz, and you peek the remote (i.e. you do not actually run "fetch" to download objects, but just "ls-remote" it) and find out that "HEAD" does not point at the same commit as "frotz" but it now points at the same commit as another branch "nitfol", you probably would want to update it to point at "nitfol", but it was unclear from the description in the documentation if this option was meant to perform this kind fo update, or only to set a missing HEAD. If you meant to do an update, there is one thing to watch out for when you reuse the logic used by clone. It favors "master" if more than one branches point at the same commit as HEAD. You need to customize it to favor the one that the HEAD points at on the local side before you start (iow, try to keep the current value when you can). For example, if it points at "frotz" locally when the command was started, and you found out that HEAD now points at the commit at the tip of "frotz" and "master" branches by peeking, you do not want to repoint HEAD from "frotz" to "master". ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 4/4] builtin-remote: add set-head verb 2009-02-13 10:35 ` [PATCH 4/4] builtin-remote: add set-head verb Junio C Hamano @ 2009-02-13 10:52 ` Jay Soffian 0 siblings, 0 replies; 41+ messages in thread From: Jay Soffian @ 2009-02-13 10:52 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, peff, barkalow On Fri, Feb 13, 2009 at 5:35 AM, Junio C Hamano <gitster@pobox.com> wrote: > A few things I forgot (and before I go to bed). > > If remotes/<name>/HEAD already points at a branch frotz, and you peek the > remote (i.e. you do not actually run "fetch" to download objects, but just > "ls-remote" it) and find out that "HEAD" does not point at the same commit > as "frotz" but it now points at the same commit as another branch > "nitfol", you probably would want to update it to point at "nitfol", but > it was unclear from the description in the documentation if this option > was meant to perform this kind fo update, or only to set a missing HEAD. - set-head -d <remote> deletes - set-head -a <remote> sets HEAD to whatever remote it - set-head <remote> <foo> sets HEAD to foo I don't know how to be more explicit than that I think that's what the documentation indicates. > If you meant to do an update, there is one thing to watch out for when you > reuse the logic used by clone. It favors "master" if more than one > branches point at the same commit as HEAD. Yes, I know. > You need to customize it to > favor the one that the HEAD points at on the local side before you start > (iow, try to keep the current value when you can). > > For example, if it points at "frotz" locally when the command was started, > and you found out that HEAD now points at the commit at the tip of "frotz" > and "master" branches by peeking, you do not want to repoint HEAD from > "frotz" to "master". I think that is an unnecessary complication for what is really a corner case. And anyway, it is just hacking around the fact that we don't really know what the remote side is if multiple branch heads have the same SHA1 as HEAD. You proposed a series a while back so that git could unambiguously determine what a remote symref points to, and I think that's the better way to fix this problem. If the user runs "git remote set-head -a <remote>" they will get whatever "git show <remote>" indicates is the remote HEAD. If they don't like that, they can set their <remote>/HEAD explicitly using the alternate syntax. j. ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 4/4] builtin-remote: add set-head verb 2009-02-13 10:09 ` Junio C Hamano 2009-02-13 10:21 ` Jay Soffian 2009-02-13 10:35 ` [PATCH 4/4] builtin-remote: add set-head verb Junio C Hamano @ 2009-02-14 0:22 ` Jeff King 2009-02-14 2:00 ` Junio C Hamano 2 siblings, 1 reply; 41+ messages in thread From: Jeff King @ 2009-02-14 0:22 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jay Soffian, git, barkalow On Fri, Feb 13, 2009 at 02:09:01AM -0800, Junio C Hamano wrote: > 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. > 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). However, I think as a rule it is nice to always provide a "long" alternative for every option. With parse-options, there really is no downside; it is literally s/0/"auto"/ on the option line. And I think it: - lets people who want to illustrate a command in a more readable manner do so (e.g., if they are showing it to somebody else) - makes it more obvious in the documentation and usage message what the command does. That is, I remember commands much better as "this is the --auto option, whose meaningful name reminds me that it does X, and -a is the obvious shorthand" rather than "-a does X". - enables extra parse-options syntax like automatic "--no-" handling (though I doubt anyone is likely to use "--no-auto" in this case, the point is that it is easier to allow such constructs consistently than to try to guess when people might want it) Which is maybe more argument than you cared to read about this particular option, but I want to make clear that I think this should be the case for just about every command-line option we add. > Also it may be better to be more explicit about both the syntax and the > semantics of `<branch>`. Do you expect "refs/remotes/<name>/master" or > just "master" (I assume the latter)? Is it an error if the branch does 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. -Peff PS There is a thread elsewhere on the list discussing "what can I do to make life easy for reviewers?". I think this series (and Jay's patches in general) model many good behaviors: clearly labeled versions, discussion of what changed in each version, proper threading, and cc'ing people who have been involved. ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 4/4] builtin-remote: add set-head verb 2009-02-14 0:22 ` Jeff King @ 2009-02-14 2:00 ` Junio C Hamano 2009-02-14 2:18 ` Jeff King 2009-02-14 2:59 ` Jay Soffian 0 siblings, 2 replies; 41+ messages in thread From: Junio C Hamano @ 2009-02-14 2:00 UTC (permalink / raw) To: Jeff King; +Cc: Jay Soffian, git, barkalow Jeff King <peff@peff.net> writes: > On Fri, Feb 13, 2009 at 02:09:01AM -0800, Junio C Hamano wrote: > >> 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. > >> 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. ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 4/4] builtin-remote: add set-head verb 2009-02-14 2:00 ` Junio C Hamano @ 2009-02-14 2:18 ` Jeff King 2009-02-14 2:48 ` Jay Soffian 2009-02-14 2:59 ` Jay Soffian 1 sibling, 1 reply; 41+ messages in thread From: Jeff King @ 2009-02-14 2:18 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jay Soffian, git, barkalow On Fri, Feb 13, 2009 at 06:00:29PM -0800, Junio C Hamano wrote: > > 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. --- diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh index eb63718..8808580 100755 --- a/t/t5505-remote.sh +++ b/t/t5505-remote.sh @@ -136,6 +136,7 @@ EOF cat > test/expect << EOF * remote origin URL: $(pwd)/one + HEAD: master Remote branch merged with 'git pull' while on branch master master New remote branch (next fetch will store in remotes/origin) @@ -338,16 +339,6 @@ test_expect_success 'update default (overridden, with funny whitespace)' ' ' -test_expect_success '"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) - -' - test_expect_success 'reject adding remote with an invalid name' ' test_must_fail git remote add some:url desired-name ^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [PATCH 4/4] builtin-remote: add set-head verb 2009-02-14 2:18 ` Jeff King @ 2009-02-14 2:48 ` Jay Soffian 0 siblings, 0 replies; 41+ messages in thread From: Jay Soffian @ 2009-02-14 2:48 UTC (permalink / raw) To: Jeff King; +Cc: Junio C Hamano, git, barkalow On Fri, Feb 13, 2009 at 9:18 PM, Jeff King <peff@peff.net> 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. ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 4/4] builtin-remote: add set-head verb 2009-02-14 2:00 ` Junio C Hamano 2009-02-14 2:18 ` Jeff King @ 2009-02-14 2:59 ` Jay Soffian 2009-02-14 3:43 ` Jeff King 1 sibling, 1 reply; 41+ messages in thread From: Jay Soffian @ 2009-02-14 2:59 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jeff King, git, barkalow On Fri, Feb 13, 2009 at 9:00 PM, Junio C Hamano <gitster@pobox.com> 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. ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 4/4] builtin-remote: add set-head verb 2009-02-14 2:59 ` Jay Soffian @ 2009-02-14 3:43 ` Jeff King 2009-02-14 10:30 ` [PATCH] builtin-remote: better handling of multiple remote HEADs Jay Soffian 0 siblings, 1 reply; 41+ messages in thread From: Jeff King @ 2009-02-14 3:43 UTC (permalink / raw) To: Jay Soffian; +Cc: Junio C Hamano, git, barkalow 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 ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH] builtin-remote: better handling of multiple remote HEADs 2009-02-14 3:43 ` Jeff King @ 2009-02-14 10:30 ` Jay Soffian 2009-02-14 17:54 ` Jeff King 2009-02-15 5:27 ` [PATCH] builtin-remote: better handling of multiple remote HEADs Jeff King 0 siblings, 2 replies; 41+ messages in thread From: Jay Soffian @ 2009-02-14 10:30 UTC (permalink / raw) To: git; +Cc: Jay Soffian, barkalow, Jeff King, Junio C Hamano 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 <jaysoffian@gmail.com> --- On Fri, Feb 13, 2009 at 10:43 PM, Jeff King <peff@peff.net> wrote: > On Fri, Feb 13, 2009 at 09:59:06PM -0500, Jay Soffian wrote: > >> 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(-) diff --git a/builtin-clone.c b/builtin-clone.c index d179d1c..d57818c 100644 --- a/builtin-clone.c +++ b/builtin-clone.c @@ -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."); diff --git a/builtin-remote.c b/builtin-remote.c index fcb166b..608c0f3 100644 --- a/builtin-remote.c +++ b/builtin-remote.c @@ -212,8 +212,7 @@ static void read_branches(void) struct ref_states { struct remote *remote; - struct string_list new, stale, tracked; - char *head_name; + struct string_list new, stale, tracked, heads; }; static int handle_one_branch(const char *refname, @@ -273,24 +272,26 @@ static int get_ref_states(const struct ref *ref, struct ref_states *states) return 0; } -static char *get_head_name(const struct ref *refs) +static int get_head_names(const struct ref *refs, + const char *remote_name, struct ref_states *states) { - const struct ref *head_points_at; - struct ref *mapped_refs = NULL; - struct ref **tail = &mapped_refs; + struct ref *ref, *matches = NULL; + struct ref *fetch_map = NULL, **fetch_map_tail = &fetch_map; struct refspec refspec; refspec.force = 0; refspec.pattern = 1; refspec.src = refspec.dst = "refs/heads/"; + states->heads.strdup_strings = 1; + get_fetch_map(refs, &refspec, &fetch_map_tail, 0); + guess_remote_head(refs, fetch_map, NULL, &matches); + for(ref = matches; ref; ref = ref->next) + string_list_append(abbrev_branch(ref->name), &states->heads); - get_fetch_map(refs, &refspec, &tail, 0); - - head_points_at = guess_remote_head(refs, mapped_refs, NULL); - if (head_points_at) - return xstrdup(abbrev_branch(head_points_at->name)); + free_refs(fetch_map); + free_refs(matches); - return NULL; + return 0; } struct known_remote { @@ -659,7 +660,7 @@ static void free_remote_ref_states(struct ref_states *states) string_list_clear(&states->new, 0); string_list_clear(&states->stale, 0); string_list_clear(&states->tracked, 0); - free(states->head_name); + string_list_clear(&states->heads, 0); } static int get_remote_ref_states(const char *name, @@ -681,7 +682,7 @@ static int get_remote_ref_states(const char *name, ref = transport_get_remote_refs(transport); transport_disconnect(transport); - states->head_name = get_head_name(ref); + get_head_names(ref, name, states); get_ref_states(ref, states); } @@ -726,9 +727,15 @@ static int show(int argc, const char **argv) printf("* remote %s\n URL: %s\n", *argv, states.remote->url_nr > 0 ? states.remote->url[0] : "(no URL)"); - if (!no_query) - printf(" HEAD: %s\n", states.head_name ? - states.head_name : "(unknown)"); + if (no_query) + printf(" HEAD branch: (not queried)\n"); + else if (!states.heads.nr) + printf(" HEAD branch: (unknown)\n"); + else if (states.heads.nr == 1) + printf(" HEAD branch: %s\n", + states.heads.items[0].string); + else + show_list(" HEAD branch%s:", &states.heads, ""); for (i = 0; i < branch_list.nr; i++) { struct string_list_item *branch = branch_list.items + i; @@ -780,7 +787,7 @@ static int show(int argc, const char **argv) static int set_head(int argc, const char **argv) { - int opt_a = 0, opt_d = 0, result = 0; + int i, opt_a = 0, opt_d = 0, result = 0; struct strbuf buf = STRBUF_INIT, buf2 = STRBUF_INIT; char *head_name = NULL; @@ -802,7 +809,16 @@ static int set_head(int argc, const char **argv) struct ref_states states; 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"); + else if (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); } else if (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); else if (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); } diff --git a/remote.c b/remote.c index 447f091..6385a22 100644 --- a/remote.c +++ b/remote.c @@ -1379,18 +1379,23 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb) const struct ref *guess_remote_head(const struct ref *refs, const struct ref *mapped_refs, - const struct ref **remote_head_p) + const struct ref **remote_head_p, + struct ref **all_matches_p) { const struct ref *remote_head = NULL; const struct ref *remote_master = NULL; + const struct ref *ret = NULL; const struct ref *r; + struct ref **tail = all_matches_p; + for (r = refs; r; r = r->next) if (!strcmp(r->name, "HEAD")) remote_head = r; - for (r = mapped_refs; r; r = r->next) - if (!strcmp(r->name, "refs/heads/master")) - remote_master = r; + if (!all_matches_p) + for (r = mapped_refs; r; r = r->next) + if (!strcmp(r->name, "refs/heads/master")) + remote_master = r; if (remote_head_p) *remote_head_p = remote_head; @@ -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)) - return r; + !hashcmp(r->old_sha1, remote_head->old_sha1)) { + struct ref *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 */ - return NULL; + return ret; } diff --git a/remote.h b/remote.h index cabb14a..8409d42 100644 --- a/remote.h +++ b/remote.h @@ -141,9 +141,13 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb); * Look in refs for HEAD. Then look for a matching SHA1 in mapped_refs, * first checking if refs/heads/master matches. Return NULL if nothing matches * or if there is no HEAD in refs. remote_head_p is assigned HEAD if not NULL. + * If all_matches_p is NULL, return after the first possible match. Otherwise + * all_matches_p is set to a ref list of each branch head with the same SHA1 as + * HEAD. */ const struct ref *guess_remote_head(const struct ref *refs, const struct ref *mapped_refs, - const struct ref **remote_head_p); + const struct ref **remote_head_p, + struct ref **all_matches_p); #endif diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh index 8808580..49f99e9 100755 --- a/t/t5505-remote.sh +++ b/t/t5505-remote.sh @@ -136,7 +136,7 @@ EOF cat > test/expect << EOF * remote origin URL: $(pwd)/one - HEAD: master + HEAD branch: master Remote branch merged with 'git pull' while on branch master master New remote branch (next fetch will store in remotes/origin) @@ -147,6 +147,11 @@ cat > test/expect << EOF Local branches pushed with 'git push' master:upstream +refs/tags/lastbackup +* remote two + URL: ../two + HEAD branches: + another + master EOF test_expect_success 'show' ' @@ -155,6 +160,7 @@ test_expect_success 'show' ' refs/heads/master:refs/heads/upstream && git fetch && git branch -d -r origin/master && + git config --add remote.two.url ../two && (cd ../one && echo 1 > file && test_tick && @@ -163,13 +169,14 @@ test_expect_success 'show' ' refs/heads/master:refs/heads/upstream && git config --add remote.origin.push \ +refs/tags/lastbackup && - git remote show origin > output && + git remote show origin two > output && test_cmp expect output) ' cat > test/expect << EOF * remote origin URL: $(pwd)/one + HEAD branch: (not queried) Remote branch merged with 'git pull' while on branch master master Tracked remote branches @@ -198,6 +205,48 @@ test_expect_success 'prune' ' test_must_fail git rev-parse refs/remotes/origin/side) ' +test_expect_success 'set-head --delete' ' + (cd test && + git symbolic-ref refs/remotes/origin/HEAD && + git remote set-head --delete origin && + test_must_fail git symbolic-ref refs/remotes/origin/HEAD) +' + +cat > test/expect <<EOF +origin/HEAD set to master +EOF + +test_expect_success 'set-head --auto' ' + (cd test && + git remote set-head --auto origin > output && + git symbolic-ref refs/remotes/origin/HEAD && + test_cmp expect output) +' + +cat >test/expect <<EOF +error: Multiple remote HEAD branches. Please choose one explicitly with: + git remote set-head two another + git remote set-head two master +EOF + +test_expect_success 'set-head --auto fails w/multiple HEADs' ' + (cd test && + test_must_fail git remote set-head --auto two >& output && + test_cmp expect output) +' + +cat >test/expect <<EOF +refs/remotes/origin/side2 +EOF + +test_expect_success 'set-head explicit' ' + (cd test && + git remote set-head origin side2 && + git symbolic-ref refs/remotes/origin/HEAD >output && + git remote set-head origin master && + test_cmp expect output) +' + cat > test/expect << EOF Pruning origin URL: $(pwd)/one -- 1.6.2.rc0.238.g0c1fe ^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [PATCH] builtin-remote: better handling of multiple remote HEADs 2009-02-14 10:30 ` [PATCH] builtin-remote: better handling of multiple remote HEADs Jay Soffian @ 2009-02-14 17:54 ` Jeff King 2009-02-14 18:35 ` Jay Soffian 2009-02-14 20:21 ` Daniel Barkalow 2009-02-15 5:27 ` [PATCH] builtin-remote: better handling of multiple remote HEADs Jeff King 1 sibling, 2 replies; 41+ messages in thread From: Jeff King @ 2009-02-14 17:54 UTC (permalink / raw) To: Jay Soffian; +Cc: git, barkalow, Junio C Hamano 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. > +cat > test/expect <<EOF > +origin/HEAD set to master > +EOF > + > +test_expect_success 'set-head --auto' ' > + (cd test && > + git remote set-head --auto origin > output && > + git symbolic-ref refs/remotes/origin/HEAD && > + test_cmp expect output) > +' 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 ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH] builtin-remote: better handling of multiple remote HEADs 2009-02-14 17:54 ` Jeff King @ 2009-02-14 18:35 ` Jay Soffian 2009-02-14 18:54 ` Jeff King 2009-02-14 20:21 ` Daniel Barkalow 1 sibling, 1 reply; 41+ messages in thread From: Jay Soffian @ 2009-02-14 18:35 UTC (permalink / raw) To: Jeff King; +Cc: git, barkalow, Junio C Hamano On Sat, Feb 14, 2009 at 12:54 PM, Jeff King <peff@peff.net> wrote: >> + 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. >> +cat > test/expect <<EOF >> +origin/HEAD set to master >> +EOF >> + >> +test_expect_success 'set-head --auto' ' >> + (cd test && >> + git remote set-head --auto origin > output && >> + git symbolic-ref refs/remotes/origin/HEAD && >> + test_cmp expect output) >> +' > > 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) > ' Right. j. ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH] builtin-remote: better handling of multiple remote HEADs 2009-02-14 18:35 ` Jay Soffian @ 2009-02-14 18:54 ` Jeff King 2009-02-14 19:48 ` Junio C Hamano 0 siblings, 1 reply; 41+ messages in thread From: Jeff King @ 2009-02-14 18:54 UTC (permalink / raw) To: Jay Soffian; +Cc: git, barkalow, Junio C Hamano On Sat, Feb 14, 2009 at 01:35:03PM -0500, Jay Soffian wrote: > On Sat, Feb 14, 2009 at 12:54 PM, Jeff King <peff@peff.net> wrote: > >> + 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. > > 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) > > ' > > Right. 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 ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH] builtin-remote: better handling of multiple remote HEADs 2009-02-14 18:54 ` Jeff King @ 2009-02-14 19:48 ` Junio C Hamano 0 siblings, 0 replies; 41+ messages in thread From: Junio C Hamano @ 2009-02-14 19:48 UTC (permalink / raw) To: Jeff King; +Cc: Jay Soffian, git, barkalow Jeff King <peff@peff.net> writes: > 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". It is not that I am special. What is special is an otherwise obviously good patch with a few trivial mistakes that I can fix locally without worrying the fix-up may be wrong. It is not even per author, it is per patch, and it is a rare exception. Often, I notice these things *after* I applied and reviewed the results, so it already is in my work area. I then judge the tradeoff between an extra round (which as you stated needs another fresh review, patch application and testing here) and the possibility that I may make a silly mistake myself while attempting a fix-up (such a mistake by me will not be seen on the list and others do not have chance to catch them). For this reason, I try to keep these "will fix up no need for resend" to the minimum and only to the most trivial cases. > ... 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. A third option is: "I would change this and that" review comment message, followed by a separate message "Here is how I would have done it", addressed To: the original submitter (with in-body From: line), Cc: to the list and me. The original submitter can verify the latter one, and either agree to or disagree with it. If the reroll is good, then I can just pick it up. I think you have done that in the past yourself, and the process made my life a lot easier. ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH] builtin-remote: better handling of multiple remote HEADs 2009-02-14 17:54 ` Jeff King 2009-02-14 18:35 ` Jay Soffian @ 2009-02-14 20:21 ` Daniel Barkalow 2009-02-14 21:15 ` Jeff King 1 sibling, 1 reply; 41+ messages in thread From: Daniel Barkalow @ 2009-02-14 20:21 UTC (permalink / raw) To: Jeff King; +Cc: Jay Soffian, git, Junio C Hamano On Sat, 14 Feb 2009, Jeff King wrote: > On Sat, Feb 14, 2009 at 05:30:30AM -0500, Jay Soffian wrote: > > > + 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* ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH] builtin-remote: better handling of multiple remote HEADs 2009-02-14 20:21 ` Daniel Barkalow @ 2009-02-14 21:15 ` Jeff King 2009-02-15 6:08 ` Jeff King 0 siblings, 1 reply; 41+ messages in thread From: Jeff King @ 2009-02-14 21:15 UTC (permalink / raw) To: Daniel Barkalow; +Cc: Jay Soffian, git, Junio C Hamano 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. --- diff --git a/remote.c b/remote.c index 6385a22..afbaccc 100644 --- a/remote.c +++ b/remote.c @@ -1404,6 +1404,20 @@ const struct ref *guess_remote_head(const struct ref *refs, if (!remote_head) return NULL; + /* if the underlying transport can represent symrefs, + * then we don't need to guess at all */ + 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; + } + return r; + } + } + } + /* If refs/heads/master could be right, it is. */ if (remote_master && !hashcmp(remote_master->old_sha1, remote_head->old_sha1)) ^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [PATCH] builtin-remote: better handling of multiple remote HEADs 2009-02-14 21:15 ` Jeff King @ 2009-02-15 6:08 ` Jeff King 2009-02-15 6:10 ` [PATCH 1/5] test scripts: refactor start_httpd helper Jeff King ` (4 more replies) 0 siblings, 5 replies; 41+ messages in thread From: Jeff King @ 2009-02-15 6:08 UTC (permalink / raw) To: Daniel Barkalow; +Cc: Jay Soffian, git, Junio C Hamano 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: > > > 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 ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 1/5] test scripts: refactor start_httpd helper 2009-02-15 6:08 ` Jeff King @ 2009-02-15 6:10 ` Jeff King 2009-02-15 6:12 ` [PATCH 2/5] add basic http clone/fetch tests Jeff King ` (3 subsequent siblings) 4 siblings, 0 replies; 41+ messages in thread From: Jeff King @ 2009-02-15 6:10 UTC (permalink / raw) To: git; +Cc: Daniel Barkalow, Jay Soffian, Junio C Hamano 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 <peff@peff.net> --- Cleanup for the next patch. t/lib-httpd.sh | 9 +++++++-- t/t5540-http-push.sh | 8 +------- 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh index 3824020..88cfc51 100644 --- a/t/lib-httpd.sh +++ b/t/lib-httpd.sh @@ -82,13 +82,18 @@ prepare_httpd() { } start_httpd() { - prepare_httpd + prepare_httpd >&3 2>&4 trap 'stop_httpd; die' EXIT "$LIB_HTTPD_PATH" -d "$HTTPD_ROOT_PATH" \ -f "$TEST_PATH/apache.conf" $HTTPD_PARA \ - -c "Listen 127.0.0.1:$LIB_HTTPD_PORT" -k start + -c "Listen 127.0.0.1:$LIB_HTTPD_PORT" -k start \ + >&3 2>&4 + if ! test $? = 0; then + say "skipping test, web server setup failed" + test_done + fi } stop_httpd() { diff --git a/t/t5540-http-push.sh b/t/t5540-http-push.sh index c236b5e..5b93e5a 100755 --- a/t/t5540-http-push.sh +++ b/t/t5540-http-push.sh @@ -20,13 +20,7 @@ then fi . "$TEST_DIRECTORY"/lib-httpd.sh - -if ! start_httpd >&3 2>&4 -then - say "skipping test, web server setup failed" - test_done - exit -fi +start_httpd test_expect_success 'setup remote repository' ' cd "$ROOT_PATH" && -- 1.6.2.rc0.256.gf004c.dirty ^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH 2/5] add basic http clone/fetch tests 2009-02-15 6:08 ` Jeff King 2009-02-15 6:10 ` [PATCH 1/5] test scripts: refactor start_httpd helper Jeff King @ 2009-02-15 6:12 ` Jeff King 2009-02-15 8:01 ` Junio C Hamano 2009-02-15 6:12 ` [PATCH 3/5] refactor find_refs_by_name to accept const list Jeff King ` (2 subsequent siblings) 4 siblings, 1 reply; 41+ messages in thread From: Jeff King @ 2009-02-15 6:12 UTC (permalink / raw) To: git; +Cc: Daniel Barkalow, Jay Soffian, Junio C Hamano 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 <peff@peff.net> --- 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 diff --git a/Makefile b/Makefile index ffef867..d689654 100644 --- a/Makefile +++ b/Makefile @@ -1368,6 +1368,7 @@ GIT-CFLAGS: .FORCE-GIT-CFLAGS GIT-BUILD-OPTIONS: .FORCE-GIT-BUILD-OPTIONS @echo SHELL_PATH=\''$(subst ','\'',$(SHELL_PATH_SQ))'\' >$@ @echo TAR=\''$(subst ','\'',$(subst ','\'',$(TAR)))'\' >>$@ + @echo NO_CURL=\''$(subst ','\'',$(subst ','\'',$(NO_CURL)))'\' >>$@ ### Detect Tck/Tk interpreter path changes ifndef NO_TCLTK diff --git a/t/t5540-http-push.sh b/t/t5540-http-push.sh index 5b93e5a..072ef1b 100755 --- a/t/t5540-http-push.sh +++ b/t/t5540-http-push.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'} if git http-push > /dev/null 2>&1 || [ $? -eq 128 ] then diff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh new file mode 100755 index 0000000..b6e6ec9 --- /dev/null +++ b/t/t5550-http-fetch.sh @@ -0,0 +1,46 @@ +#!/bin/sh + +test_description='test fetching over http' +. ./test-lib.sh + +if test -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' ' + echo content >file && + git add file && + git commit -m one +' + +test_expect_success 'create http-accessible bare repository' ' + mkdir "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" && + (cd "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" && + git --bare init && + echo "exec git update-server-info" >hooks/post-update && + chmod +x hooks/post-update + ) && + git remote add public "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" && + git push public master:master +' + +test_expect_success 'clone http repository' ' + git clone $HTTPD_URL/repo.git clone && + test_cmp file clone/file +' + +test_expect_success 'fetch changes via http' ' + echo content >>file && + git commit -a -m two && + git push public + (cd clone && git pull) && + test_cmp file clone/file +' + +stop_httpd +test_done -- 1.6.2.rc0.256.gf004c.dirty ^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [PATCH 2/5] add basic http clone/fetch tests 2009-02-15 6:12 ` [PATCH 2/5] add basic http clone/fetch tests Jeff King @ 2009-02-15 8:01 ` Junio C Hamano 0 siblings, 0 replies; 41+ messages in thread From: Junio C Hamano @ 2009-02-15 8:01 UTC (permalink / raw) To: Jeff King; +Cc: git, Daniel Barkalow, Jay Soffian Jeff King <peff@peff.net> writes: > ... Of > course, one must use GIT_TEST_HTTPD to enable them. Junio, is that part > of your usual integration testing? No. I do not run server tests myself. I could enable it for tests run on my private machine, but the final pre-pushout testing don at k.org is more or less unattended, and it is a shared machine where I do not want to run server-ish tests. ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 3/5] refactor find_refs_by_name to accept const list 2009-02-15 6:08 ` Jeff King 2009-02-15 6:10 ` [PATCH 1/5] test scripts: refactor start_httpd helper Jeff King 2009-02-15 6:12 ` [PATCH 2/5] add basic http clone/fetch tests Jeff King @ 2009-02-15 6:12 ` Jeff King 2009-02-15 6:16 ` [PATCH 4/5] remote: refactor guess_remote_head Jeff King 2009-02-15 6:18 ` [PATCH 5/5] remote: use exact HEAD lookup if it is available Jeff King 4 siblings, 0 replies; 41+ messages in thread From: Jeff King @ 2009-02-15 6:12 UTC (permalink / raw) To: git; +Cc: Daniel Barkalow, Jay Soffian, Junio C Hamano 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 <peff@peff.net> --- cache.h | 2 +- refs.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/cache.h b/cache.h index 7f1a6e8..ba6c070 100644 --- a/cache.h +++ b/cache.h @@ -801,7 +801,7 @@ struct ref { #define REF_HEADS (1u << 1) #define REF_TAGS (1u << 2) -extern struct ref *find_ref_by_name(struct ref *list, const char *name); +extern struct ref *find_ref_by_name(const struct ref *list, const char *name); #define CONNECT_VERBOSE (1u << 0) extern struct child_process *git_connect(int fd[2], const char *url, const char *prog, int flags); diff --git a/refs.c b/refs.c index 6eb5f53..b2a37e1 100644 --- a/refs.c +++ b/refs.c @@ -1628,10 +1628,10 @@ int update_ref(const char *action, const char *refname, return 0; } -struct ref *find_ref_by_name(struct ref *list, const char *name) +struct ref *find_ref_by_name(const struct ref *list, const char *name) { for ( ; list; list = list->next) if (!strcmp(list->name, name)) - return list; + return (struct ref *)list; return NULL; } -- 1.6.2.rc0.256.gf004c.dirty ^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH 4/5] remote: refactor guess_remote_head 2009-02-15 6:08 ` Jeff King ` (2 preceding siblings ...) 2009-02-15 6:12 ` [PATCH 3/5] refactor find_refs_by_name to accept const list Jeff King @ 2009-02-15 6:16 ` Jeff King 2009-02-15 6:18 ` [PATCH 5/5] remote: use exact HEAD lookup if it is available Jeff King 4 siblings, 0 replies; 41+ messages in thread From: Jeff King @ 2009-02-15 6:16 UTC (permalink / raw) To: git; +Cc: Daniel Barkalow, Jay Soffian, Junio C Hamano 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 <peff@peff.net> --- 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(-) diff --git a/builtin-clone.c b/builtin-clone.c index d57818c..7130cab 100644 --- a/builtin-clone.c +++ b/builtin-clone.c @@ -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."); diff --git a/builtin-remote.c b/builtin-remote.c index 608c0f3..d6958d4 100644 --- a/builtin-remote.c +++ b/builtin-remote.c @@ -275,7 +275,7 @@ static int get_ref_states(const struct ref *ref, struct ref_states *states) static int get_head_names(const struct ref *refs, const char *remote_name, struct ref_states *states) { - struct ref *ref, *matches = NULL; + struct ref *ref, *matches; struct ref *fetch_map = NULL, **fetch_map_tail = &fetch_map; struct refspec refspec; @@ -284,7 +284,8 @@ static int get_head_names(const struct ref *refs, refspec.src = refspec.dst = "refs/heads/"; states->heads.strdup_strings = 1; get_fetch_map(refs, &refspec, &fetch_map_tail, 0); - guess_remote_head(refs, fetch_map, NULL, &matches); + matches = guess_remote_head(find_ref_by_name(refs, "HEAD"), + fetch_map, 1); for(ref = matches; ref; ref = ref->next) string_list_append(abbrev_branch(ref->name), &states->heads); diff --git a/remote.c b/remote.c index 6385a22..7ba1bff 100644 --- a/remote.c +++ b/remote.c @@ -1377,51 +1377,41 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb) return 1; } -const struct ref *guess_remote_head(const struct ref *refs, - const struct ref *mapped_refs, - const struct ref **remote_head_p, - struct ref **all_matches_p) +struct ref *copy_ref_with_peer(const struct ref *src) { - const struct ref *remote_head = NULL; - const struct ref *remote_master = NULL; - const struct ref *ret = NULL; - const struct ref *r; - struct ref **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; + struct ref *dst = copy_ref(src); + dst->peer_ref = copy_ref(src->peer_ref); + return dst; +} - if (remote_head_p) - *remote_head_p = remote_head; +struct ref *guess_remote_head(const struct ref *head, + const struct ref *refs, + int all) +{ + const struct ref *r; + struct ref *list = NULL; + struct ref **tail = &list; - /* If there's no HEAD value at all, never mind. */ - if (!remote_head) + if (!head) return NULL; /* If refs/heads/master could be right, it is. */ - if (remote_master && !hashcmp(remote_master->old_sha1, - remote_head->old_sha1)) - return remote_master; + if (!all) { + const struct ref *m; + m = find_ref_by_name(refs, "refs/heads/master"); + if (m && !hashcmp(m->old_sha1, head->old_sha1)) + return copy_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)) { - struct ref *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; } + } - return ret; + return list; } diff --git a/remote.h b/remote.h index 8409d42..111c4ba 100644 --- a/remote.h +++ b/remote.h @@ -138,16 +138,13 @@ int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs); int format_tracking_info(struct branch *branch, struct strbuf *sb); /* - * Look in refs for HEAD. Then look for a matching SHA1 in mapped_refs, - * first checking if refs/heads/master matches. Return NULL if nothing matches - * or if there is no HEAD in refs. remote_head_p is assigned HEAD if not NULL. - * If all_matches_p is NULL, return after the first possible match. Otherwise - * all_matches_p is set to a ref list of each branch head with the same SHA1 as - * HEAD. + * Find refs from a list which are likely to be pointed to by the given HEAD + * ref. If 'all' is false, returns the most likely ref; otherwise, returns a + * list of all candidate refs. If no match is found (or 'head' is NULL), + * returns NULL. All returns are newly allocated and should be freed. */ -const struct ref *guess_remote_head(const struct ref *refs, - const struct ref *mapped_refs, - const struct ref **remote_head_p, - struct ref **all_matches_p); +struct ref *guess_remote_head(const struct ref *head, + const struct ref *refs, + int all); #endif -- 1.6.2.rc0.256.gf004c.dirty ^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH 5/5] remote: use exact HEAD lookup if it is available 2009-02-15 6:08 ` Jeff King ` (3 preceding siblings ...) 2009-02-15 6:16 ` [PATCH 4/5] remote: refactor guess_remote_head Jeff King @ 2009-02-15 6:18 ` Jeff King 2009-02-15 15:22 ` Jay Soffian 2009-02-15 19:58 ` Jeff King 4 siblings, 2 replies; 41+ messages in thread From: Jeff King @ 2009-02-15 6:18 UTC (permalink / raw) To: git; +Cc: Daniel Barkalow, Jay Soffian, Junio C Hamano 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 <peff@peff.net> --- 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(-) diff --git a/remote.c b/remote.c index 7ba1bff..a96a910 100644 --- a/remote.c +++ b/remote.c @@ -1395,6 +1395,16 @@ struct ref *guess_remote_head(const struct ref *head, if (!head) return NULL; + /* + * Some transports support directly peeking at + * where HEAD points; if that is the case, then + * we don't have to guess. + */ + if (head->symref) { + r = find_ref_by_name(refs, head->symref); + return r ? copy_ref_with_peer(r) : NULL; + } + /* If refs/heads/master could be right, it is. */ if (!all) { const struct ref *m; diff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh index b6e6ec9..05b1b62 100755 --- a/t/t5550-http-fetch.sh +++ b/t/t5550-http-fetch.sh @@ -42,5 +42,16 @@ test_expect_success 'fetch changes via http' ' test_cmp file clone/file ' +test_expect_success 'http remote detects correct HEAD' ' + git push public master:other && + (cd clone && + git remote set-head origin -d && + git remote set-head origin -a && + git symbolic-ref refs/remotes/origin/HEAD > output && + echo refs/remotes/origin/master > expect && + test_cmp expect output + ) +' + stop_httpd test_done -- 1.6.2.rc0.256.gf004c.dirty ^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [PATCH 5/5] remote: use exact HEAD lookup if it is available 2009-02-15 6:18 ` [PATCH 5/5] remote: use exact HEAD lookup if it is available Jeff King @ 2009-02-15 15:22 ` Jay Soffian 2009-02-15 19:58 ` Jeff King 1 sibling, 0 replies; 41+ messages in thread From: Jay Soffian @ 2009-02-15 15:22 UTC (permalink / raw) To: Jeff King, Junio C Hamano; +Cc: git, Daniel Barkalow On Sun, Feb 15, 2009 at 1:18 AM, Jeff King <peff@peff.net> wrote: > A possible 6/5 would be to do something similar for local repos (or > resurrecting the HEAD proposal). http://thread.gmane.org/gmane.comp.version-control.git/102039 I read that thread, and it wasn't clear to me why it died. Was there a v3 patch round that I cannot find? The only unaddressed reply I see is from Jeff: http://thread.gmane.org/gmane.comp.version-control.git/102039 But that seems tangential to the proposal. j. ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 5/5] remote: use exact HEAD lookup if it is available 2009-02-15 6:18 ` [PATCH 5/5] remote: use exact HEAD lookup if it is available Jeff King 2009-02-15 15:22 ` Jay Soffian @ 2009-02-15 19:58 ` Jeff King 2009-02-15 20:00 ` [PATCH 1/2] transport: cleanup duplicated ref fetching code Jeff King 2009-02-15 20:01 ` [PATCH 2/2] transport: unambiguously determine local HEAD Jeff King 1 sibling, 2 replies; 41+ messages in thread From: Jeff King @ 2009-02-15 19:58 UTC (permalink / raw) To: git; +Cc: Daniel Barkalow, Jay Soffian, Junio C Hamano 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 ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 1/2] transport: cleanup duplicated ref fetching code 2009-02-15 19:58 ` Jeff King @ 2009-02-15 20:00 ` Jeff King 2009-02-15 20:01 ` [PATCH 2/2] transport: unambiguously determine local HEAD Jeff King 1 sibling, 0 replies; 41+ messages in thread From: Jeff King @ 2009-02-15 20:00 UTC (permalink / raw) To: git; +Cc: Daniel Barkalow, Jay Soffian, Junio C Hamano 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 <peff@peff.net> --- 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(-) diff --git a/transport.c b/transport.c index 9ad4a16..c9f31f6 100644 --- a/transport.c +++ b/transport.c @@ -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, -- 1.6.2.rc0.258.gcef3.dirty ^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH 2/2] transport: unambiguously determine local HEAD 2009-02-15 19:58 ` Jeff King 2009-02-15 20:00 ` [PATCH 1/2] transport: cleanup duplicated ref fetching code Jeff King @ 2009-02-15 20:01 ` Jeff King 1 sibling, 0 replies; 41+ messages in thread From: Jeff King @ 2009-02-15 20:01 UTC (permalink / raw) To: git; +Cc: Daniel Barkalow, Jay Soffian, Junio C Hamano 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 <peff@peff.net> --- Again, this fails tests in t5505, and is not meant for inclusion. transport.c | 39 +++++++++++++++++++++++++++++++++++++++ 1 files changed, 39 insertions(+), 0 deletions(-) diff --git a/transport.c b/transport.c index c9f31f6..e575203 100644 --- a/transport.c +++ b/transport.c @@ -12,6 +12,8 @@ #include "dir.h" #include "refs.h" +static int is_local(const char *url); + /* rsync support */ /* @@ -609,6 +611,40 @@ static int connect_setup(struct transport *transport) return 0; } +static void mark_head_via_local(const char *url, struct ref *refs) +{ + static const char *argv[] = { "symbolic-ref", "HEAD", NULL }; + static const char *env[] = { GIT_DIR_ENVIRONMENT, NULL }; + struct child_process cmd; + struct ref *head; + struct strbuf buf = STRBUF_INIT; + + head = find_ref_by_name(refs, "HEAD"); + if (!head) + return; + + memset(&cmd, 0, sizeof cmd); + 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); +} + static struct ref *get_refs_via_connect(struct transport *transport) { struct git_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); + return refs; } -- 1.6.2.rc0.258.gcef3.dirty ^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [PATCH] builtin-remote: better handling of multiple remote HEADs 2009-02-14 10:30 ` [PATCH] builtin-remote: better handling of multiple remote HEADs Jay Soffian 2009-02-14 17:54 ` Jeff King @ 2009-02-15 5:27 ` Jeff King 2009-02-15 5:34 ` Jeff King 2009-02-15 14:13 ` Jay Soffian 1 sibling, 2 replies; 41+ messages in thread From: Jeff King @ 2009-02-15 5:27 UTC (permalink / raw) To: Jay Soffian; +Cc: git, barkalow, Junio C Hamano On Sat, Feb 14, 2009 at 05:30:30AM -0500, Jay Soffian wrote: > +test_expect_success 'set-head --auto fails w/multiple HEADs' ' > + (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. -Peff ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH] builtin-remote: better handling of multiple remote HEADs 2009-02-15 5:27 ` [PATCH] builtin-remote: better handling of multiple remote HEADs Jeff King @ 2009-02-15 5:34 ` Jeff King 2009-02-15 14:13 ` Jay Soffian 1 sibling, 0 replies; 41+ messages in thread From: Jeff King @ 2009-02-15 5:34 UTC (permalink / raw) To: Jay Soffian; +Cc: git, barkalow, Junio C Hamano On Sun, Feb 15, 2009 at 12:27:40AM -0500, Jeff King wrote: > On Sat, Feb 14, 2009 at 05:30:30AM -0500, Jay Soffian wrote: > > > +test_expect_success 'set-head --auto fails w/multiple HEADs' ' > > + (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. 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. --- diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh index 49f99e9..f0be105 100755 --- a/t/t5505-remote.sh +++ b/t/t5505-remote.sh @@ -231,7 +231,7 @@ EOF test_expect_success 'set-head --auto fails w/multiple HEADs' ' (cd test && - test_must_fail git remote set-head --auto two >& output && + test_must_fail git remote set-head --auto two > output 2>&1 && test_cmp expect output) ' ^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [PATCH] builtin-remote: better handling of multiple remote HEADs 2009-02-15 5:27 ` [PATCH] builtin-remote: better handling of multiple remote HEADs Jeff King 2009-02-15 5:34 ` Jeff King @ 2009-02-15 14:13 ` Jay Soffian 2009-02-15 15:12 ` Jeff King 2009-02-16 2:21 ` Junio C Hamano 1 sibling, 2 replies; 41+ messages in thread From: Jay Soffian @ 2009-02-15 14:13 UTC (permalink / raw) To: Jeff King; +Cc: git, barkalow, Junio C Hamano On Sun, Feb 15, 2009 at 12:27 AM, Jeff King <peff@peff.net> wrote: > On Sat, Feb 14, 2009 at 05:30:30AM -0500, Jay Soffian wrote: > >> +test_expect_success 'set-head --auto fails w/multiple HEADs' ' >> + (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? [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. ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH] builtin-remote: better handling of multiple remote HEADs 2009-02-15 14:13 ` Jay Soffian @ 2009-02-15 15:12 ` Jeff King 2009-02-16 2:21 ` Junio C Hamano 1 sibling, 0 replies; 41+ messages in thread From: Jeff King @ 2009-02-15 15:12 UTC (permalink / raw) To: Jay Soffian; +Cc: git, barkalow, Junio C Hamano On Sun, Feb 15, 2009 at 09:13:09AM -0500, Jay Soffian wrote: > >> + (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 ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH] builtin-remote: better handling of multiple remote HEADs 2009-02-15 14:13 ` Jay Soffian 2009-02-15 15:12 ` Jeff King @ 2009-02-16 2:21 ` Junio C Hamano 2009-02-16 2:58 ` Jay Soffian 1 sibling, 1 reply; 41+ messages in thread From: Junio C Hamano @ 2009-02-16 2:21 UTC (permalink / raw) To: Jay Soffian; +Cc: Jeff King, git, barkalow Jay Soffian <jaysoffian@gmail.com> writes: > 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 expansion 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 Just to clarify, the above is not a recommendation for shell scripts in git project. The last one is the only one we recommend in our scripts. By the say, does anybody know why bash people recommend &>word form? Neither &>word nor >&word to send both stderr and stdout to the file is from true Bourne, but at least the use of >&word form for this purpose is more familiar to people who are used to Csh. ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH] builtin-remote: better handling of multiple remote HEADs 2009-02-16 2:21 ` Junio C Hamano @ 2009-02-16 2:58 ` Jay Soffian 0 siblings, 0 replies; 41+ messages in thread From: Jay Soffian @ 2009-02-16 2:58 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jeff King, git, barkalow On Sun, Feb 15, 2009 at 9:21 PM, Junio C Hamano <gitster@pobox.com> wrote: >> &>word >> >&word > Just to clarify, the above is not a recommendation for shell scripts in > git project. Indeed not! My use of it wasn't even intentional, I was a tcsh (cringe...) user for a long time and I only finally switched to bash about a year ago. It must've been muscle memory that made me type it, but a pox on bash for not rejecting it outright -- the bash man page claims posix'ish compliance when invoked as /bin/sh, so I don't know why it allows such syntax in its posix'ish mode. > By the say, does anybody know why bash people recommend &>word form? > > Neither &>word nor >&word to send both stderr and stdout to the file is > from true Bourne, but at least the use of >&word form for this purpose is > more familiar to people who are used to Csh. <tongue in cheek>it is exactly because it is familiar to csh people that they recommend the opposite</tongue in cheek> j. ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 0/4] remote HEAD improvements take 2 2009-02-13 8:54 [PATCH 0/4] remote HEAD improvements take 2 Jay Soffian 2009-02-13 8:54 ` [PATCH 1/4] builtin-clone: move locate_head() to remote.c so it can be re-used Jay Soffian @ 2009-02-13 8:57 ` Jay Soffian 1 sibling, 0 replies; 41+ messages in thread From: Jay Soffian @ 2009-02-13 8:57 UTC (permalink / raw) To: git On Fri, Feb 13, 2009 at 3:54 AM, Jay Soffian <jaysoffian@gmail.com> wrote: > There is currently no porcelain for dealing with remote HEADs (i.e. > $GIT_DIR/remotes/<remote>/HEAD). This series: Crud, sorry for the duplicates. And I just noticed a send-email bug -- it doesn't handle multiple Cc: recipients the way they are formated by format-patch. e.g, format patch does this: Cc: firstperson@example.com, secondperson@example.com But send-email only picks up the first address, and further, appears to mangle it. j. ^ permalink raw reply [flat|nested] 41+ messages in thread
end of thread, other threads:[~2009-02-16 3:00 UTC | newest] Thread overview: 41+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2009-02-13 8:54 [PATCH 0/4] remote HEAD improvements take 2 Jay Soffian 2009-02-13 8:54 ` [PATCH 1/4] builtin-clone: move locate_head() to remote.c so it can be re-used Jay Soffian 2009-02-13 8:54 ` [PATCH 2/4] builtin-remote: move duplicated cleanup code its own function Jay Soffian 2009-02-13 8:54 ` [PATCH 3/4] builtin-remote: teach show to display remote HEAD Jay Soffian 2009-02-13 8:54 ` [PATCH 4/4] builtin-remote: add set-head verb Jay Soffian 2009-02-13 10:09 ` Junio C Hamano 2009-02-13 10:21 ` Jay Soffian 2009-02-13 11:42 ` [PATCH v2 4/4] builtin-remote: add set-head subcommand Jay Soffian 2009-02-13 10:35 ` [PATCH 4/4] builtin-remote: add set-head verb Junio C Hamano 2009-02-13 10:52 ` Jay Soffian 2009-02-14 0:22 ` Jeff King 2009-02-14 2:00 ` Junio C Hamano 2009-02-14 2:18 ` Jeff King 2009-02-14 2:48 ` Jay Soffian 2009-02-14 2:59 ` Jay Soffian 2009-02-14 3:43 ` Jeff King 2009-02-14 10:30 ` [PATCH] builtin-remote: better handling of multiple remote HEADs Jay Soffian 2009-02-14 17:54 ` Jeff King 2009-02-14 18:35 ` Jay Soffian 2009-02-14 18:54 ` Jeff King 2009-02-14 19:48 ` Junio C Hamano 2009-02-14 20:21 ` Daniel Barkalow 2009-02-14 21:15 ` Jeff King 2009-02-15 6:08 ` Jeff King 2009-02-15 6:10 ` [PATCH 1/5] test scripts: refactor start_httpd helper Jeff King 2009-02-15 6:12 ` [PATCH 2/5] add basic http clone/fetch tests Jeff King 2009-02-15 8:01 ` Junio C Hamano 2009-02-15 6:12 ` [PATCH 3/5] refactor find_refs_by_name to accept const list Jeff King 2009-02-15 6:16 ` [PATCH 4/5] remote: refactor guess_remote_head Jeff King 2009-02-15 6:18 ` [PATCH 5/5] remote: use exact HEAD lookup if it is available Jeff King 2009-02-15 15:22 ` Jay Soffian 2009-02-15 19:58 ` Jeff King 2009-02-15 20:00 ` [PATCH 1/2] transport: cleanup duplicated ref fetching code Jeff King 2009-02-15 20:01 ` [PATCH 2/2] transport: unambiguously determine local HEAD Jeff King 2009-02-15 5:27 ` [PATCH] builtin-remote: better handling of multiple remote HEADs Jeff King 2009-02-15 5:34 ` Jeff King 2009-02-15 14:13 ` Jay Soffian 2009-02-15 15:12 ` Jeff King 2009-02-16 2:21 ` Junio C Hamano 2009-02-16 2:58 ` Jay Soffian 2009-02-13 8:57 ` [PATCH 0/4] remote HEAD improvements take 2 Jay Soffian
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.