From: Jay Soffian <hidden> Date: 2016-06-15 22:46:16
Junio, this is based on pu, as it builds on my and Jeff's git remote
changes. Patches 1-7 are about new output style for "git remote show"
for everything but the push refspecs. Patches 8-13 then culiminate in
teaching it a new output style for push refspecs as well.
1-4 were previously a single patch which I broke up per your feedback
(gmane 110806).
Jay Soffian (13):
remote: rename variable and eliminate redundant function call
remote: remove unused code in get_ref_states
remote: fix two inconsistencies in the output of "show <remote>"
remote: make get_remote_ref_states() always populate states.tracked
remote: name remote_refs consistently
string-list: new for_each_string_list() function
remote: new show output style
refactor duplicated get_local_heads() to remote.c
refactor duplicated ref_newer() to remote.c
remote.c: make match_refs() copy src ref before assigning to peer_ref
remote.c: don't short-circuit match_refs() when error in
match_explicit_refs()
remote.c: refactor get_remote_ref_states()
remote: new show output style for push refspecs
builtin-remote.c | 469 ++++++++++++++++++++++++++++++++++++++++-----------
builtin-send-pack.c | 78 +---------
http-push.c | 72 +--------
remote.c | 88 +++++++++-
remote.h | 2 +
string-list.c | 10 +
string-list.h | 5 +
t/t5505-remote.sh | 66 +++++---
8 files changed, 516 insertions(+), 274 deletions(-)
From: Jay Soffian <hidden> Date: 2016-06-15 22:46:16
Remote and stale branches are emitted in alphabetical order, but new and
tracked branches are not. So sort the latter to be consistent with the
former. This also lets us use more efficient string_list_has_string()
instead of unsorted_string_list_has_string().
"show <remote>" prunes symrefs, but "show <remote> -n" does not. Fix the
latter to match the former.
Signed-off-by: Jay Soffian <redacted>
---
builtin-remote.c | 15 ++++++++++-----
t/t5505-remote.sh | 2 +-
2 files changed, 11 insertions(+), 6 deletions(-)
From: Jay Soffian <hidden> Date: 2016-06-15 22:46:16
get_ref_states() populates the util pointer of the string_list_item's
that it adds to states->new and states->tracked, but nothing ever uses
the pointer, so we can get rid of the extra code.
Signed-off-by: Jay Soffian <redacted>
---
builtin-remote.c | 13 +++----------
1 files changed, 3 insertions(+), 10 deletions(-)
From: Jay Soffian <hidden> Date: 2016-06-15 22:46:16
The variable name "remote" is used as both a "char *" and as a "struct
remote *"; this is confusing, so rename the former to remote_name.
There is no need to call "sort_string_list(&branch_list)" as branch_list
is populated via string_list_insert(), which maintains its order.
Signed-off-by: Jay Soffian <redacted>
---
builtin-remote.c | 13 ++++++-------
1 files changed, 6 insertions(+), 7 deletions(-)
From: Jay Soffian <hidden> Date: 2016-06-15 22:46:16
Add a convenience function for iterating over a string_list's items via
a callback.
Signed-off-by: Jay Soffian <redacted>
---
string-list.c | 10 ++++++++++
string-list.h | 5 +++++
2 files changed, 15 insertions(+), 0 deletions(-)
@@ -20,6 +20,11 @@ void string_list_clear(struct string_list *list, int free_util);typedefvoid(*string_list_clear_func_t)(void*p,constchar*str);voidstring_list_clear_func(structstring_list*list,string_list_clear_func_tclearfunc);+/* Use this function to iterate over each item */+typedefint(*string_list_each_func_t)(structstring_list_item*,void*);+intfor_each_string_list(string_list_each_func_t,+structstring_list*list,void*cb_data);+/* Use these functions only on sorted lists: */intstring_list_has_string(conststructstring_list*list,constchar*string);intstring_list_find_insert_index(conststructstring_list*list,constchar*string,
From: Jay Soffian <hidden> Date: 2016-06-15 22:46:16
When not querying the remote, show() was having to populate
states.tracked itself. It makes more sense for get_remote_ref_states()
to do this consistently. Since show() is the only caller of
get_remote_ref_states() with query=0, this change does not affect the
other call sites.
Signed-off-by: Jay Soffian <redacted>
---
builtin-remote.c | 41 ++++++++++++++++++++---------------------
1 files changed, 20 insertions(+), 21 deletions(-)
From: Jay Soffian <hidden> Date: 2016-06-15 22:46:16
Clarify the code a bit by consistently referring to the remote refs
returned by transport_get_remote_refs() as remote_refs.
Signed-off-by: Jay Soffian <redacted>
---
builtin-remote.c | 22 +++++++++++-----------
1 files changed, 11 insertions(+), 11 deletions(-)
@@ -236,13 +236,14 @@ static int handle_one_branch(const char *refname,return0;}-staticintget_ref_states(conststructref*ref,structref_states*states)+staticintget_ref_states(conststructref*remote_refs,structref_states*states){structref*fetch_map=NULL,**tail=&fetch_map;+structref*ref;inti;for(i=0;i<states->remote->fetch_refspec_nr;i++)-if(get_fetch_map(ref,states->remote->fetch+i,&tail,1))+if(get_fetch_map(remote_refs,states->remote->fetch+i,&tail,1))die("Could not get fetch map for refspec %s",states->remote->fetch_refspec[i]);
From: Jay Soffian <hidden> Date: 2016-06-15 22:46:16
get_local_heads() appears to have been copied from builtin-send-pack.c
to http-push.c via cut and paste. This patch moves the function and its
helper one_local_ref() to remote.c.
The two copies of one_local_ref() were not identical. I used the more
recent version from builtin-send-pack.c after confirming with Jeff King
that it was an oversight that commit 30affa1e did not update both
copies.
Signed-off-by: Jay Soffian <redacted>
---
builtin-send-pack.c | 29 ++---------------------------
http-push.c | 23 ++---------------------
remote.c | 26 ++++++++++++++++++++++++++
remote.h | 1 +
4 files changed, 31 insertions(+), 48 deletions(-)
@@ -133,33 +133,8 @@ static int ref_newer(const unsigned char *new_sha1,returnfound;}-staticstructref*local_refs,**local_tail;staticstructref*remote_refs,**remote_tail;-staticintone_local_ref(constchar*refname,constunsignedchar*sha1,intflag,void*cb_data)-{-structref*ref;-intlen;--/* we already know it starts with refs/ to get here */-if(check_ref_format(refname+5))-return0;--len=strlen(refname)+1;-ref=xcalloc(1,sizeof(*ref)+len);-hashcpy(ref->new_sha1,sha1);-memcpy(ref->name,refname,len);-*local_tail=ref;-local_tail=&ref->next;-return0;-}--staticvoidget_local_heads(void)-{-local_tail=&local_refs;-for_each_ref(one_local_ref,NULL);-}-staticintreceive_status(intin,structref*refs){structref*hint;
@@ -387,7 +362,7 @@ static int refs_pushed(struct ref *ref)staticintdo_send_pack(intin,intout,structremote*remote,constchar*dest,intnr_refspec,constchar**refspec){-structref*ref;+structref*ref,*local_refs;intnew_refs;intask_for_status_report=0;intallow_deleting_refs=0;
@@ -405,7 +380,7 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest/* No funny business with the matcher */remote_tail=get_remote_heads(in,&remote_refs,0,NULL,REF_NORMAL,&extra_have);-get_local_heads();+local_refs=get_local_heads();/* Does the other end support the reporting? */if(server_supports("report-status"))
@@ -2195,7 +2176,7 @@ int main(int argc, char **argv)intrc=0;inti;intnew_refs;-structref*ref;+structref*ref,*local_refs;char*rewritten_url=NULL;git_extract_argv0_path(argv[0]);
@@ -2302,7 +2283,7 @@ int main(int argc, char **argv)fetch_indices();/* Get a list of all local and remote heads to validate refspecs */-get_local_heads();+local_refs=get_local_heads();fprintf(stderr,"Fetching remote heads...\n");get_dav_remote_heads();
@@ -1427,3 +1427,29 @@ struct ref *guess_remote_head(const struct ref *head,returnlist;}++staticintone_local_ref(constchar*refname,constunsignedchar*sha1,intflag,void*cb_data)+{+structref***local_tail=cb_data;+structref*ref;+intlen;++/* we already know it starts with refs/ to get here */+if(check_ref_format(refname+5))+return0;++len=strlen(refname)+1;+ref=xcalloc(1,sizeof(*ref)+len);+hashcpy(ref->new_sha1,sha1);+memcpy(ref->name,refname,len);+**local_tail=ref;+*local_tail=&ref->next;+return0;+}++structref*get_local_heads(void)+{+structref*local_refs,**local_tail=&local_refs;+for_each_ref(one_local_ref,&local_tail);+returnlocal_refs;+}
From: Jay Soffian <hidden> Date: 2016-06-15 22:46:16
get_remote_ref_states() has three callers, but each is interested in
slightly different information. Give it a bit-field flag so that callers
can specify which pieces of information they need.
Signed-off-by: Jay Soffian <redacted>
---
builtin-remote.c | 23 +++++++++++++++--------
1 files changed, 15 insertions(+), 8 deletions(-)
@@ -685,14 +688,15 @@ static int get_remote_ref_states(const char *name,states->remote->url[0]:NULL);remote_refs=transport_get_remote_refs(transport);transport_disconnect(transport);-get_head_names(remote_refs,name,states);-get_ref_states(remote_refs,states);+states->queried=1;+if(query&GET_REF_STATES)+get_ref_states(remote_refs,states);+if(query&GET_HEAD_NAMES)+get_head_names(remote_refs,name,states);}else{for_each_ref(append_ref_to_tracked_list,states);sort_string_list(&states->tracked);}-states->queried=query;-return0;}
@@ -796,7 +800,7 @@ int show_local_info_item(struct string_list_item *item, void *cb_data)staticintshow(intargc,constchar**argv){-intno_query=0,result=0;+intno_query=0,result=0,query_flag=0;structoptionoptions[]={OPT_GROUP("show specific options"),OPT_BOOLEAN('n',NULL,&no_query,"do not query remotes"),
From: Jay Soffian <hidden> Date: 2016-06-15 22:46:16
The existing output of "git remote show <remote>" is too verbose for the
information it provides. This patch teaches it to provide more
information in less space.
The output for push refspecs is addressed in followup patch.
Before the patch:
$ git remote show origin
* remote origin
URL: git://git.kernel.org/pub/scm/git/git.git
HEAD branch: master
Remote branch merged with 'git pull' while on branch master
master
Remote branch merged with 'git pull' while on branch next
next
Remote branches merged with 'git pull' while on branch octopus
foo bar baz frotz
New remote branch (next fetch will store in remotes/origin)
html
Stale tracking branch (use 'git remote prune')
bogus
Tracked remote branches
maint
man
master
next
pu
todo
After this patch:
$ git remote show origin
* remote origin
URL: git://git.kernel.org/pub/scm/git/git.git
HEAD branch: master
Remote branches:
bogus stale (use 'git remote prune' to remove)
html new (next fetch will store in remotes/origin)
maint tracked
man tracked
master tracked
next tracked
pu tracked
todo tracked
Local branches configured for 'git pull':
master rebases onto remote master
next rebases onto remote next
octopus merges with remote foo
and with remote bar
and with remote baz
and with remote frotz
$ git remote show origin -n
* remote origin
URL: git://git.kernel.org/pub/scm/git/git.git
HEAD branch: (not queried)
Remote branches: (status not queried)
bogus
maint
man
master
next
pu
todo
Local branches configured for 'git pull':
master rebases onto remote master
next rebases onto remote next
octopus merges with remote foo
and with remote bar
and with remote baz
and with remote frotz
Signed-off-by: Jay Soffian <redacted>
---
I incorporated the feedback I received to gmane 110626. As the commit
message mentions, the push refspec output is addressed in a later patch
in this series.
builtin-remote.c | 187 ++++++++++++++++++++++++++++++++++++++++-------------
t/t5505-remote.sh | 31 ++++++----
2 files changed, 161 insertions(+), 57 deletions(-)
@@ -184,9 +189,9 @@ static int config_read_branches(const char *key, const char *value, void *cb)info=item->util;if(type==REMOTE){if(info->remote_name)-warning("more than one branch.%s",key);+warning("more than one %s",orig_key);info->remote_name=xstrdup(value);-}else{+}elseif(type==MERGE){char*space=strchr(value,' ');value=abbrev_branch(value);while(space){
@@ -698,6 +691,105 @@ static int get_remote_ref_states(const char *name,for_each_ref(append_ref_to_tracked_list,states);sort_string_list(&states->tracked);}+states->queried=query;++return0;+}++structshow_info{+structstring_list*list;+structref_states*states;+intwidth;+intany_rebase;+};++intadd_remote_to_show_info(structstring_list_item*item,void*cb_data)+{+structshow_info*info=cb_data;+intn=strlen(item->string);+if(n>info->width)+info->width=n;+string_list_insert(item->string,info->list);+return0;+}++intshow_remote_info_item(structstring_list_item*item,void*cb_data)+{+structshow_info*info=cb_data;+structref_states*states=info->states;+constchar*name=item->string;++if(states->queried){+constchar*fmt="%s";+constchar*arg="";+if(string_list_has_string(&states->new,name)){+fmt=" new (next fetch will store in remotes/%s)";+arg=states->remote->name;+}elseif(string_list_has_string(&states->tracked,name))+arg=" tracked";+elseif(string_list_has_string(&states->stale,name))+arg=" stale (use 'git remote prune' to remove)";+else+arg=" ???";+printf(" %-*s",info->width,name);+printf(fmt,arg);+printf("\n");+}else+printf(" %s\n",name);++return0;+}++intadd_local_to_show_info(structstring_list_item*branch_item,void*cb_data)+{+structshow_info*show_info=cb_data;+structref_states*states=show_info->states;+structbranch_info*branch_info=branch_item->util;+structstring_list_item*item;+intn;++if(!branch_info->merge.nr||!branch_info->remote_name||+strcmp(states->remote->name,branch_info->remote_name))+return0;+if((n=strlen(branch_item->string))>show_info->width)+show_info->width=n;+if(branch_info->rebase)+show_info->any_rebase=1;++item=string_list_insert(branch_item->string,show_info->list);+item->util=branch_info;++return0;+}++intshow_local_info_item(structstring_list_item*item,void*cb_data)+{+structshow_info*show_info=cb_data;+structbranch_info*branch_info=item->util;+structstring_list*merge=&branch_info->merge;+constchar*also;+inti;++if(branch_info->rebase&&branch_info->merge.nr>1){+error("invalid branch.%s.merge; cannot rebase onto > 1 branch",+item->string);+return0;+}++printf(" %-*s ",show_info->width,item->string);+if(branch_info->rebase){+printf("rebases onto remote %s\n",merge->items[0].string);+return0;+}elseif(show_info->any_rebase){+printf(" merges with remote %s\n",merge->items[0].string);+also=" and with remote";+}else{+printf("merges with remote %s\n",merge->items[0].string);+also=" and with remote";+}+for(i=1;i<merge->nr;i++)+printf(" %-*s %s %s\n",show_info->width,"",also,+merge->items[i].string);return0;}
@@ -731,37 +828,37 @@ static int show(int argc, const char **argv)elseif(!states.heads.nr)printf(" HEAD branch: (unknown)\n");elseif(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++){-structstring_list_item*branch=branch_list.items+i;-structbranch_info*info=branch->util;-intj;--if(!info->merge.nr||strcmp(*argv,info->remote_name))-continue;-printf(" Remote branch%s merged with 'git pull' "-"while on branch %s\n ",-info->merge.nr>1?"es":"",-branch->string);-for(j=0;j<info->merge.nr;j++)-printf(" %s",info->merge.items[j].string);-printf("\n");+printf(" HEAD branch: %s\n",states.heads.items[0].string);+else{+printf(" HEAD branch (remote HEAD is ambiguous,"+" may be one of the following):\n");+for(i=0;i<states.heads.nr;i++)+printf(" %s\n",states.heads.items[i].string);}-if(!no_query){-show_list(" New remote branch%s (next fetch "-"will store in remotes/%s)",-&states.new,states.remote->name);-show_list(" Stale tracking branch%s (use 'git remote "-"prune')",&states.stale,"");-}+/* remote branch info */+info.width=0;+for_each_string_list(add_remote_to_show_info,&states.new,&info);+for_each_string_list(add_remote_to_show_info,&states.tracked,&info);+for_each_string_list(add_remote_to_show_info,&states.stale,&info);+if(info.list->nr)+printf(" Remote branch%s:%s\n",+info.list->nr>1?"es":"",+no_query?" (status not queried)":"");+for_each_string_list(show_remote_info_item,info.list,&info);+string_list_clear(info.list,0);-show_list(" Tracked remote branch%s",&states.tracked,"");+/* git pull info */+info.width=0;+info.any_rebase=0;+for_each_string_list(add_local_to_show_info,&branch_list,&info);+if(info.list->nr)+printf(" Local branch%s configured for 'git pull':\n",+info.list->nr>1?"es":"");+for_each_string_list(show_local_info_item,info.list,&info);+string_list_clear(info.list,0);+/* git push info */if(states.remote->push_refspec_nr){printf(" Local branch%s pushed with 'git push'\n",states.remote->push_refspec_nr>1?
From: Jay Soffian <hidden> Date: 2016-06-15 22:46:16
ref_newer() appears to have been copied from builtin-send-pack.c to
http-push.c via cut and paste. This patch moves the function and its
helper unmark_and_free() to remote.c.
Signed-off-by: Jay Soffian <redacted>
---
Hmm, I just noticed that the version in builtin-send-pack.c uses 1
whereas http-push.c uses TMP_MARK as the mark argument. I used the
version from builtin-send-pack.c. I don't know if this is signfificant.
builtin-send-pack.c | 49 -------------------------------------------------
http-push.c | 49 -------------------------------------------------
remote.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
remote.h | 1 +
4 files changed, 50 insertions(+), 98 deletions(-)
@@ -84,55 +84,6 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *extreturn0;}-staticvoidunmark_and_free(structcommit_list*list,unsignedintmark)-{-while(list){-structcommit_list*temp=list;-temp->item->object.flags&=~mark;-list=temp->next;-free(temp);-}-}--staticintref_newer(constunsignedchar*new_sha1,-constunsignedchar*old_sha1)-{-structobject*o;-structcommit*old,*new;-structcommit_list*list,*used;-intfound=0;--/* Both new and old must be commit-ish and new is descendant of-*old.Otherwisewerequire--force.-*/-o=deref_tag(parse_object(old_sha1),NULL,0);-if(!o||o->type!=OBJ_COMMIT)-return0;-old=(structcommit*)o;--o=deref_tag(parse_object(new_sha1),NULL,0);-if(!o||o->type!=OBJ_COMMIT)-return0;-new=(structcommit*)o;--if(parse_commit(new)<0)-return0;--used=list=NULL;-commit_list_insert(new,&list);-while(list){-new=pop_most_recent_commit(&list,1);-commit_list_insert(new,&used);-if(new==old){-found=1;-break;-}-}-unmark_and_free(list,1);-unmark_and_free(used,1);-returnfound;-}-staticstructref*remote_refs,**remote_tail;staticintreceive_status(intin,structref*refs)
@@ -1843,55 +1843,6 @@ static int is_zero_sha1(const unsigned char *sha1)return1;}-staticvoidunmark_and_free(structcommit_list*list,unsignedintmark)-{-while(list){-structcommit_list*temp=list;-temp->item->object.flags&=~mark;-list=temp->next;-free(temp);-}-}--staticintref_newer(constunsignedchar*new_sha1,-constunsignedchar*old_sha1)-{-structobject*o;-structcommit*old,*new;-structcommit_list*list,*used;-intfound=0;--/* Both new and old must be commit-ish and new is descendant of-*old.Otherwisewerequire--force.-*/-o=deref_tag(parse_object(old_sha1),NULL,0);-if(!o||o->type!=OBJ_COMMIT)-return0;-old=(structcommit*)o;--o=deref_tag(parse_object(new_sha1),NULL,0);-if(!o||o->type!=OBJ_COMMIT)-return0;-new=(structcommit*)o;--if(parse_commit(new)<0)-return0;--used=list=NULL;-commit_list_insert(new,&list);-while(list){-new=pop_most_recent_commit(&list,TMP_MARK);-commit_list_insert(new,&used);-if(new==old){-found=1;-break;-}-}-unmark_and_free(list,TMP_MARK);-unmark_and_free(used,TMP_MARK);-returnfound;-}-staticvoidadd_remote_info_ref(structremote_ls_ctx*ls){structstrbuf*buf=(structstrbuf*)ls->userData;
@@ -1271,6 +1272,54 @@ int resolve_remote_symref(struct ref *ref, struct ref *list)return1;}+staticvoidunmark_and_free(structcommit_list*list,unsignedintmark)+{+while(list){+structcommit_list*temp=list;+temp->item->object.flags&=~mark;+list=temp->next;+free(temp);+}+}++intref_newer(constunsignedchar*new_sha1,constunsignedchar*old_sha1)+{+structobject*o;+structcommit*old,*new;+structcommit_list*list,*used;+intfound=0;++/* Both new and old must be commit-ish and new is descendant of+*old.Otherwisewerequire--force.+*/+o=deref_tag(parse_object(old_sha1),NULL,0);+if(!o||o->type!=OBJ_COMMIT)+return0;+old=(structcommit*)o;++o=deref_tag(parse_object(new_sha1),NULL,0);+if(!o||o->type!=OBJ_COMMIT)+return0;+new=(structcommit*)o;++if(parse_commit(new)<0)+return0;++used=list=NULL;+commit_list_insert(new,&list);+while(list){+new=pop_most_recent_commit(&list,1);+commit_list_insert(new,&used);+if(new==old){+found=1;+break;+}+}+unmark_and_free(list,1);+unmark_and_free(used,1);+returnfound;+}+/**Returntrueifthereisanythingtoreport,otherwisefalse.*/
From: Jay Soffian <hidden> Date: 2016-06-15 22:46:16
The existing output of "git remote show <remote>" with respect to push
ref specs is basically just to show the raw refspec. This patch teaches
the command to interpret the refspecs and show how each branch will be
pushed to the destination. The output gives the user an idea of what
"git push" should do if it is run w/o any arguments.
Example new output:
1a. Typical output with no push refspec (i.e. matching branches only)
$ git remote show origin
* remote origin
[...]
Local refs configured for 'git push':
master pushes to master (up to date)
next pushes to next (local out of date)
1b. Same as above, w/o querying the remote:
$ git remote show origin -n
* remote origin
[...]
Local ref configured for 'git push' (status not queried):
(matching) pushes to (matching)
2a. With a forcing refspec (+), and a new topic
(something like push = refs/heads/*:refs/heads/*):
$ git remote show origin
* remote origin
[...]
Local refs configured for 'git push':
master pushes to master (fast forwardable)
new-topic pushes to new-topic (create)
next pushes to next (local out of date)
pu forces to pu (up to date)
2b. Same as above, w/o querying the remote
$ git remote show origin -n
* remote origin
[...]
Local refs configured for 'git push' (status not queried):
master pushes to master
new-topic pushes to new-topic
next pushes to next
pu forces to pu
3. With a remote configured as a mirror:
* remote backup
[...]
Local refs will be mirrored by 'git push'
Signed-off-by: Jay Soffian <redacted>
---
After spending quite a long time standing in the paint aisle looking
at all the different colors, this is my favorite.
You might like a different color paint. For example, an obvious choice
might seem to be single character status fields a-la ls-files. But that
would sacrifice clarity (IMO) as the user then has to go lookup what the
values mean in a man page, and in any case wouldn't be' consistent with
the rest of the new "show" output.
Also, "forces to" may sound a little funny, but I think it is obvious
what it means and lines up more nicely than "force pushes" or any other
choice I considered.
Anyway, I built this new shed, so I get to pick the color. (And
hopefully no one blows it up w/a stick of dynamite) :-)
builtin-remote.c | 201 ++++++++++++++++++++++++++++++++++++++++++++++++----
t/t5505-remote.sh | 57 +++++++++------
2 files changed, 219 insertions(+), 39 deletions(-)
@@ -693,9 +808,12 @@ static int get_remote_ref_states(const char *name,get_ref_states(remote_refs,states);if(query&GET_HEAD_NAMES)get_head_names(remote_refs,name,states);+if(query&GET_PUSH_REF_STATES)+get_push_ref_states(remote_refs,states);}else{for_each_ref(append_ref_to_tracked_list,states);sort_string_list(&states->tracked);+get_push_ref_states_noquery(states);}return0;}
@@ -703,7 +821,7 @@ static int get_remote_ref_states(const char *name,structshow_info{structstring_list*list;structref_states*states;-intwidth;+intwidth,width2;intany_rebase;};
@@ -798,6 +916,58 @@ int show_local_info_item(struct string_list_item *item, void *cb_data)return0;}+intadd_push_to_show_info(structstring_list_item*push_item,void*cb_data)+{+structshow_info*show_info=cb_data;+structpush_info*push_info=push_item->util;+structstring_list_item*item;+intn;+if((n=strlen(push_item->string))>show_info->width)+show_info->width=n;+if((n=strlen(push_info->dest))>show_info->width2)+show_info->width2=n;+item=string_list_append(push_item->string,show_info->list);+item->util=push_item->util;+return0;+}++intshow_push_info_item(structstring_list_item*item,void*cb_data)+{+structshow_info*show_info=cb_data;+structpush_info*push_info=item->util;+char*src=item->string,*status=NULL;++switch(push_info->status){+casePUSH_STATUS_CREATE:+status="create";+break;+casePUSH_STATUS_DELETE:+status="delete";+src="(none)";+break;+casePUSH_STATUS_UPTODATE:+status="up to date";+break;+casePUSH_STATUS_FASTFORWARD:+status="fast forwardable";+break;+casePUSH_STATUS_OUTOFDATE:+status="local out of date";+break;+casePUSH_STATUS_NOTQUERIED:+break;+}+if(status)+printf(" %-*s %s to %-*s (%s)\n",show_info->width,src,+push_info->forced?"forces":"pushes",+show_info->width2,push_info->dest,status);+else+printf(" %-*s %s to %s\n",show_info->width,src,+push_info->forced?"forces":"pushes",+push_info->dest);+return0;+}+staticintshow(intargc,constchar**argv){intno_query=0,result=0,query_flag=0;
@@ -866,19 +1036,18 @@ static int show(int argc, const char **argv)string_list_clear(info.list,0);/* git push info */-if(states.remote->push_refspec_nr){-printf(" Local branch%s pushed with 'git push'\n",-states.remote->push_refspec_nr>1?-"es":"");-for(i=0;i<states.remote->push_refspec_nr;i++){-structrefspec*spec=states.remote->push+i;-printf(" %s%s%s%s\n",-spec->force?"+":"",-abbrev_branch(spec->src),-spec->dst?":":"",-spec->dst?abbrev_branch(spec->dst):"");-}-}+if(states.remote->mirror)+printf(" Local refs will be mirrored by 'git push'\n");++info.width=info.width2=0;+for_each_string_list(add_push_to_show_info,&states.push,&info);+sort_string_list(info.list);+if(info.list->nr)+printf(" Local ref%s configured for 'git push'%s:\n",+info.list->nr>1?"s":"",+no_query?" (status not queried)":"");+for_each_string_list(show_push_info_item,info.list,&info);+string_list_clear(info.list,0);free_remote_ref_states(&states);}
From: Jay Soffian <hidden> Date: 2016-06-15 22:46:16
match_refs() returns non-zero if there is an error in
match_explicit_refs(), without handling any remaining pattern ref specs.
Its existing callers exit upon receiving non-zero, so a partial result
is of no consequence to them; however it is about to have a new caller
that is interested in the complete result even if there are errors in
match_explicit_refs().
Signed-off-by: Jay Soffian <redacted>
---
remote.c | 6 ++++--
1 files changed, 4 insertions(+), 2 deletions(-)
From: Jay Soffian <hidden> Date: 2016-06-15 22:46:16
In some instances, match_refs() was setting the peer_ref field in the
dst ref list such that it pointed to a ref in the src list, instead of
to a copy. This would cause a double-free when both the src and dst ref
lists were freed, as free_refs() frees the peer_ref. A double-free would
also occur if the same branch was pushed to two different destinations
such as:
push = refs/heads/master:refs/heads/backup
push = refs/heads/master:refs/heads/master
This patch corrects the problem by copying the ref when it has been
plucked from the src list.
Signed-off-by: Jay Soffian <redacted>
---
remote.c | 7 +++++--
1 files changed, 5 insertions(+), 2 deletions(-)
@@ -940,6 +941,7 @@ static int match_explicit(struct ref *src, struct ref *dst,matched_src=matched_dst=NULL;switch(count_refspec_match(rs->src,src,&matched_src)){case1:+copy_src=1;break;case0:/* The source could be in the get_sha1() format
@@ -949,6 +951,7 @@ static int match_explicit(struct ref *src, struct ref *dst,matched_src=try_explicit_object_name(rs->src);if(!matched_src)returnerror("src refspec %s does not match any.",rs->src);+copy_src=0;break;default:returnerror("src refspec %s matches more than one.",rs->src);
@@ -994,7 +997,7 @@ static int match_explicit(struct ref *src, struct ref *dst,returnerror("dst ref %s receives from more than one src.",matched_dst->name);else{-matched_dst->peer_ref=matched_src;+matched_dst->peer_ref=copy_src?copy_ref(matched_src):matched_src;matched_dst->force=rs->force;}return0;
From: Jeff King <hidden> Date: 2016-06-15 22:46:16
On Mon, Feb 23, 2009 at 01:28:57AM -0500, Jay Soffian wrote:
ref_newer() appears to have been copied from builtin-send-pack.c to
http-push.c via cut and paste. This patch moves the function and its
helper unmark_and_free() to remote.c.
Signed-off-by: Jay Soffian <redacted>
---
Hmm, I just noticed that the version in builtin-send-pack.c uses 1
whereas http-push.c uses TMP_MARK as the mark argument. I used the
version from builtin-send-pack.c. I don't know if this is signfificant.
I think the TMP_MARK one is probably better. At the very least, the
send-pack one should be using SEEN instead of a bare '1'. But given that
it is not part of the usual revision machinery, it seems like that was
what TMP_MARK was designed for.
-Peff
From: Jeff King <hidden> Date: 2016-06-15 22:46:16
On Mon, Feb 23, 2009 at 01:28:55AM -0500, Jay Soffian wrote:
The existing output of "git remote show <remote>" is too verbose for the
information it provides. This patch teaches it to provide more
information in less space.
Before the patch:
[...]
After this patch:
[...]
Wow. IMHO, the new one is _way_ easier to read.
-Peff
From: Jeff King <hidden> Date: 2016-06-15 22:46:16
On Mon, Feb 23, 2009 at 01:29:00AM -0500, Jay Soffian wrote:
get_remote_ref_states() has three callers, but each is interested in
slightly different information. Give it a bit-field flag so that callers
can specify which pieces of information they need.
Hmph. I think this is probably an indication that
get_remote_ref_states() should really be 3 functions:
common_stuff();
query_one();
query_two();
and then callers can choose the subset they are interested in.
Which is really more or less equivalent; it just seems like extra
obfuscation to have a single function with a bit-field.
-Peff
From: Jeff King <hidden> Date: 2016-06-15 22:46:16
On Mon, Feb 23, 2009 at 01:28:48AM -0500, Jay Soffian wrote:
Jay Soffian (13):
remote: rename variable and eliminate redundant function call
remote: remove unused code in get_ref_states
remote: fix two inconsistencies in the output of "show <remote>"
remote: make get_remote_ref_states() always populate states.tracked
remote: name remote_refs consistently
string-list: new for_each_string_list() function
remote: new show output style
refactor duplicated get_local_heads() to remote.c
refactor duplicated ref_newer() to remote.c
remote.c: make match_refs() copy src ref before assigning to peer_ref
remote.c: don't short-circuit match_refs() when error in
match_explicit_refs()
remote.c: refactor get_remote_ref_states()
remote: new show output style for push refspecs
I gave these a quick read (but not any extensive apply+build+experiment
cycle), and they all look good to me. I don't know when I'll have time
to try breaking them, though. ;)
Thank you for splitting them. It made reading a lot more pleasant.
-Peff
From: Jay Soffian <hidden> Date: 2016-06-15 22:46:16
ref_newer() appears to have been copied from builtin-send-pack.c to
http-push.c via cut and paste. This patch moves the function and its
helper unmark_and_free() to remote.c. There was a slight difference
between the two implementations, one used TMP_MARK for the mark, the
other used 1. Per Jeff King, I went with TMP_MARK as more correct.
Signed-off-by: Jay Soffian <redacted>
---
On Mon, Feb 23, 2009 at 1:45 AM, Jeff King [off-list ref] wrote:
I think the TMP_MARK one is probably better. At the very least, the
send-pack one should be using SEEN instead of a bare '1'. But given that
it is not part of the usual revision machinery, it seems like that was
what TMP_MARK was designed for.
Okay then. This replaces the previous in the series using TMP_MARK
instead, otherwise identical.
j.
builtin-send-pack.c | 49 -------------------------------------------------
http-push.c | 49 -------------------------------------------------
remote.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
remote.h | 1 +
4 files changed, 50 insertions(+), 98 deletions(-)
@@ -84,55 +84,6 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *extreturn0;}-staticvoidunmark_and_free(structcommit_list*list,unsignedintmark)-{-while(list){-structcommit_list*temp=list;-temp->item->object.flags&=~mark;-list=temp->next;-free(temp);-}-}--staticintref_newer(constunsignedchar*new_sha1,-constunsignedchar*old_sha1)-{-structobject*o;-structcommit*old,*new;-structcommit_list*list,*used;-intfound=0;--/* Both new and old must be commit-ish and new is descendant of-*old.Otherwisewerequire--force.-*/-o=deref_tag(parse_object(old_sha1),NULL,0);-if(!o||o->type!=OBJ_COMMIT)-return0;-old=(structcommit*)o;--o=deref_tag(parse_object(new_sha1),NULL,0);-if(!o||o->type!=OBJ_COMMIT)-return0;-new=(structcommit*)o;--if(parse_commit(new)<0)-return0;--used=list=NULL;-commit_list_insert(new,&list);-while(list){-new=pop_most_recent_commit(&list,1);-commit_list_insert(new,&used);-if(new==old){-found=1;-break;-}-}-unmark_and_free(list,1);-unmark_and_free(used,1);-returnfound;-}-staticstructref*remote_refs,**remote_tail;staticintreceive_status(intin,structref*refs)
@@ -1843,55 +1843,6 @@ static int is_zero_sha1(const unsigned char *sha1)return1;}-staticvoidunmark_and_free(structcommit_list*list,unsignedintmark)-{-while(list){-structcommit_list*temp=list;-temp->item->object.flags&=~mark;-list=temp->next;-free(temp);-}-}--staticintref_newer(constunsignedchar*new_sha1,-constunsignedchar*old_sha1)-{-structobject*o;-structcommit*old,*new;-structcommit_list*list,*used;-intfound=0;--/* Both new and old must be commit-ish and new is descendant of-*old.Otherwisewerequire--force.-*/-o=deref_tag(parse_object(old_sha1),NULL,0);-if(!o||o->type!=OBJ_COMMIT)-return0;-old=(structcommit*)o;--o=deref_tag(parse_object(new_sha1),NULL,0);-if(!o||o->type!=OBJ_COMMIT)-return0;-new=(structcommit*)o;--if(parse_commit(new)<0)-return0;--used=list=NULL;-commit_list_insert(new,&list);-while(list){-new=pop_most_recent_commit(&list,TMP_MARK);-commit_list_insert(new,&used);-if(new==old){-found=1;-break;-}-}-unmark_and_free(list,TMP_MARK);-unmark_and_free(used,TMP_MARK);-returnfound;-}-staticvoidadd_remote_info_ref(structremote_ls_ctx*ls){structstrbuf*buf=(structstrbuf*)ls->userData;
@@ -1271,6 +1272,54 @@ int resolve_remote_symref(struct ref *ref, struct ref *list)return1;}+staticvoidunmark_and_free(structcommit_list*list,unsignedintmark)+{+while(list){+structcommit_list*temp=list;+temp->item->object.flags&=~mark;+list=temp->next;+free(temp);+}+}++intref_newer(constunsignedchar*new_sha1,constunsignedchar*old_sha1)+{+structobject*o;+structcommit*old,*new;+structcommit_list*list,*used;+intfound=0;++/* Both new and old must be commit-ish and new is descendant of+*old.Otherwisewerequire--force.+*/+o=deref_tag(parse_object(old_sha1),NULL,0);+if(!o||o->type!=OBJ_COMMIT)+return0;+old=(structcommit*)o;++o=deref_tag(parse_object(new_sha1),NULL,0);+if(!o||o->type!=OBJ_COMMIT)+return0;+new=(structcommit*)o;++if(parse_commit(new)<0)+return0;++used=list=NULL;+commit_list_insert(new,&list);+while(list){+new=pop_most_recent_commit(&list,TMP_MARK);+commit_list_insert(new,&used);+if(new==old){+found=1;+break;+}+}+unmark_and_free(list,TMP_MARK);+unmark_and_free(used,TMP_MARK);+returnfound;+}+/**Returntrueifthereisanythingtoreport,otherwisefalse.*/
From: Jay Soffian <hidden> Date: 2016-06-15 22:46:16
On Mon, Feb 23, 2009 at 1:50 AM, Jeff King [off-list ref] wrote:
On Mon, Feb 23, 2009 at 01:29:00AM -0500, Jay Soffian wrote:
quoted
get_remote_ref_states() has three callers, but each is interested in
slightly different information. Give it a bit-field flag so that callers
can specify which pieces of information they need.
Hmph. I think this is probably an indication that
get_remote_ref_states() should really be 3 functions:
common_stuff();
query_one();
query_two();
and then callers can choose the subset they are interested in.
Which is really more or less equivalent; it just seems like extra
obfuscation to have a single function with a bit-field.
I see your hmph and raise you a hmph. :-)
Well, I _had_ tried as you suggested first, and thought it yuckier. It
would actually be more like:
caller1() {
setup_for_get();
get_thing_one();
}
caller2() {
setup_for_get();
get_thing_two();
}
caller3() {
if (query) {
setup_for_get();
get_thing_one();
get_thing_two();
get_thing_three();
} else {
get_thing_one_noquery()
get_thing_two_noquery();
}
As opposed to:
caller1() {
get_things(ONE)
}
caller2() {
get_things(TWO)
}
caller3() {
things = 0;
if (query)
things = (ONE|TWO|THREE)
get_things(things)
}
I'm not sure why passing a flag saying what you want is obfuscating.
Also, I did find other places in git that do this sort of thing (e.g.,
get_remote_heads).
(Awaiting Junio's lucid reply explaining why my justification is bogus.) :-)
j.
From: Jay Soffian <hidden> Date: 2016-06-15 22:46:16
On Mon, Feb 23, 2009 at 1:59 AM, Jeff King [off-list ref] wrote:
I gave these a quick read (but not any extensive apply+build+experiment
cycle), and they all look good to me. I don't know when I'll have time
to try breaking them, though. ;)
Thank you for splitting them. It made reading a lot more pleasant.
Thank you for the review. I humbly await your breakage attempts.
j.
From: Jay Soffian <hidden> Date: 2016-06-15 22:46:16
---
So here's the inter-diff doing it the way you suggest. I'm surprised it
didn't add any LOC. If you really think this is better I'll re-do the
end of the series to do it this way.
builtin-remote.c | 81 ++++++++++++++++++++++++++---------------------------
1 files changed, 40 insertions(+), 41 deletions(-)
@@ -383,7 +379,7 @@ static int get_push_ref_states_noquery(struct ref_states *states)}staticintget_head_names(conststructref*remote_refs,-constchar*remote_name,structref_states*states)+structref_states*states){structref*ref,*matches;structref*fetch_map=NULL,**fetch_map_tail=&fetch_map;
@@ -785,37 +781,15 @@ static int append_ref_to_tracked_list(const char *refname,return0;}-staticintget_remote_ref_states(constchar*name,-structref_states*states,-intquery)+staticconststructref*get_remote_refs(structref_states*states){structtransport*transport;conststructref*remote_refs;--states->remote=remote_get(name);-if(!states->remote)-returnerror("No such remote: %s",name);--read_branches();--if(query){-transport=transport_get(NULL,states->remote->url_nr>0?-states->remote->url[0]:NULL);-remote_refs=transport_get_remote_refs(transport);-transport_disconnect(transport);-states->queried=1;-if(query&GET_REF_STATES)-get_ref_states(remote_refs,states);-if(query&GET_HEAD_NAMES)-get_head_names(remote_refs,name,states);-if(query&GET_PUSH_REF_STATES)-get_push_ref_states(remote_refs,states);-}else{-for_each_ref(append_ref_to_tracked_list,states);-sort_string_list(&states->tracked);-get_push_ref_states_noquery(states);-}-return0;+transport=transport_get(NULL,states->remote->url_nr>0?+states->remote->url[0]:NULL);+remote_refs=transport_get_remote_refs(transport);+transport_disconnect(transport);+returnremote_refs;}structshow_info{
@@ -970,7 +944,7 @@ int show_push_info_item(struct string_list_item *item, void *cb_data)staticintshow(intargc,constchar**argv){-intno_query=0,result=0,query_flag=0;+intno_query=0,result=0;structoptionoptions[]={OPT_GROUP("show specific options"),OPT_BOOLEAN('n',NULL,&no_query,"do not query remotes"),
@@ -985,17 +959,31 @@ static int show(int argc, const char **argv)if(argc<1)returnshow_all();-if(!no_query)-query_flag=(GET_REF_STATES|GET_HEAD_NAMES|GET_PUSH_REF_STATES);-+read_branches();memset(&states,0,sizeof(states));memset(&info,0,sizeof(info));+states.queried=!no_query;info.states=&states;info.list=&info_list;+for(;argc;argc--,argv++){inti;-get_remote_ref_states(*argv,&states,query_flag);+if(!(states.remote=remote_get(*argv))){+error("No such remote: %s",*argv);+continue;+}+if(no_query){+for_each_ref(append_ref_to_tracked_list,&states);+sort_string_list(&states.tracked);+get_push_ref_states_noquery(&states);+}else{+conststructref*remote_refs;+remote_refs=get_remote_refs(&states);+get_ref_states(remote_refs,&states);+get_head_names(remote_refs,&states);+get_push_ref_states(remote_refs,&states);+}printf("* remote %s\n URL: %s\n",*argv,states.remote->url_nr>0?
@@ -1077,8 +1065,13 @@ static int set_head(int argc, const char **argv)head_name=xstrdup(argv[1]);}elseif(opt_a&&!opt_d&&argc==1){structref_statesstates;+conststructref*remote_refs;memset(&states,0,sizeof(states));-get_remote_ref_states(argv[0],&states,GET_HEAD_NAMES);+read_branches();+if(!(states.remote=remote_get(argv[0])))+returnerror("No such remote: %s",argv[0]);+remote_refs=get_remote_refs(&states);+get_head_names(remote_refs,&states);if(!states.heads.nr)result|=error("Cannot determine remote HEAD");elseif(states.heads.nr>1){
@@ -1134,11 +1127,17 @@ static int prune(int argc, const char **argv)?" %s will become dangling!\n":" %s has become dangling!\n");+read_branches();memset(&states,0,sizeof(states));for(;argc;argc--,argv++){inti;--get_remote_ref_states(*argv,&states,GET_REF_STATES);+conststructref*remote_refs;+if(!(states.remote=remote_get(*argv))){+error("No such remote: %s",*argv);+continue;+}+remote_refs=get_remote_refs(&states);+get_ref_states(remote_refs,&states);if(states.stale.nr){printf("Pruning %s\n",*argv);
You are only moving code from http-push.c and builtin-send-pack.c to
remote.c, but you have to add this '#include "tag.h"' here. How high are
the chances that you can remove the #include lines from http-push.c and
builtin-send-pack.c?
-- Hannes
You are only moving code from http-push.c and builtin-send-pack.c to
remote.c, but you have to add this '#include "tag.h"' here. How high are
the chances that you can remove the #include lines from http-push.c and
builtin-send-pack.c?
50/50 as it turns out. Junio, please squash this in if you don't mind:
From: Junio C Hamano <hidden> Date: 2016-06-15 22:46:16
Does this series _semantically_ depend on topics other than your other
"set HEAD" topic that is still not in 'master'?
I see you made it cleanly apply on top of 'pu', but frankly, I really hate
to see a series based on 'pu' as a matter of principle and discipline.
Other topics in 'pu' may not be ready for a long time, and you do not want
this series to be taken hostage to them. Having to fork your topic off of
many other topics not even in 'next' means it would be more cumbersome for
me to handle your series when any of the other topics that you base your
work on need to be discarded or replaced.
In other words, you make your series unworkable with if you introduce too
many dependencies.
I've rebased it on top of Peff's jk/head-lookup topic (which in turn
depends on your js/remote-set-head topic), but I suspect that you would
want this series fork from and only depend on js/remote-set-head topic.
In any case, please check the result after I push the results out to make
sure that I did not make any silly mistakes while rebasing the series.
I also suspect that you would want to reroll the entire series anyway; see
separate comments on 10/13 and 11/13.
Thanks.
From: Junio C Hamano <hidden> Date: 2016-06-15 22:46:16
If this issue was present before you started touching the affected code,
it should come much earlier in the series than [10/13]. If on the other
hand this was introduced by earlier one in the series, the patch that
introduced the bug should be corrected in place in the sequence without
this patch.
From: Jay Soffian <hidden> Date: 2016-06-15 22:46:16
On Mon, Feb 23, 2009 at 8:34 PM, Junio C Hamano [off-list ref] wrote:
If this issue was present before you started touching the affected code,
it should come much earlier in the series than [10/13]. If on the other
hand this was introduced by earlier one in the series, the patch that
introduced the bug should be corrected in place in the sequence without
this patch.
This issue existed before, but the other callers of match_heads() only
call it once before the process terminates, and don't bother to free
what it returns. So those callers didn't notice. This topic introduces
the first caller that calls match_heads() repeatedly, so it frees the
returned ref list after it has copied out what it needs.
The change is independent and can be moved anywhere in the series, or
even introduced as a patch independent of the series itself. Which
would you prefer? (Actually, would you mind just applying it how you
see fit?)
j.
From: Jay Soffian <hidden> Date: 2016-06-15 22:46:16
On Mon, Feb 23, 2009 at 8:34 PM, Junio C Hamano [off-list ref] wrote:
Does this series _semantically_ depend on topics other than your other
"set HEAD" topic that is still not in 'master'?
No, but I wanted Jeff's changes. I have checked that I can apply the
"set HEAD" series, then Jeff's changes, then this latest series all to
next w/o conflict.
I've not tried it w/o Jeff's changes in the middle.
I see you made it cleanly apply on top of 'pu', but frankly, I really hate
to see a series based on 'pu' as a matter of principle and discipline.
Other topics in 'pu' may not be ready for a long time, and you do not want
this series to be taken hostage to them. Having to fork your topic off of
many other topics not even in 'next' means it would be more cumbersome for
me to handle your series when any of the other topics that you base your
work on need to be discarded or replaced.
Understood. I thought this situation was a little different though due
to Jeff's changes in the middle which really build on my set HEAD
topic.
In other words, you make your series unworkable with if you introduce too
many dependencies.
I've rebased it on top of Peff's jk/head-lookup topic (which in turn
depends on your js/remote-set-head topic), but I suspect that you would
want this series fork from and only depend on js/remote-set-head topic.
In any case, please check the result after I push the results out to make
sure that I did not make any silly mistakes while rebasing the series.
Will do, thanks.
I also suspect that you would want to reroll the entire series anyway; see
separate comments on 10/13 and 11/13.
I've replied to those. Would you mind applying 10/13 and 11/13
independently of this series? This topic depends on them since
otherwise there will be double-frees, but they can go anywhere before
the topic.
j.
From: Jay Soffian <hidden> Date: 2016-06-15 22:46:16
On Mon, Feb 23, 2009 at 10:09 PM, Jay Soffian [off-list ref] wrote:
No, but I wanted Jeff's changes. I have checked that I can apply the
"set HEAD" series, then Jeff's changes, then this latest series all to
next w/o conflict.
Put another way, IMO, the set HEAD series, Jeff's changes, and this
series might very well be a single topic.
quoted
In other words, you make your series unworkable with if you introduce too
many dependencies.
For future reference, should I build on top of master or next?
j.