login
Search
This patch series was inspired by http://thread.gmane.org/gmane.comp.version-control.git/109301/ I've based it on pu. Jay Soffian (4): remote: minor code cleanups in preparation for changing "show" output remote: move append_ref_to_tracked_list to get rid of prototype string-list: add for_each_string_list() remote: new show output style builtin-remote.c | 232 +++++++++++++++++++++++++++++++++++------------------ string-list.c | 10 +++ string-list.h | 5 + t/t5505-remote.sh | 32 ++++---- 4 files changed, 184 insertions(+), 95 deletions(-) --
* Rename char *remote to remote_name to distinguish it clearly from the
struct remote pointer, also named remote.
* There is no need to call sort_string_list() on branch_list, as its
items are added to it via string_list_insert() which maintains its
order.
* Sort states->new and states->tracked so that we can use binary search
string_list_has_string() on them instead of less efficient linear
unsorted_string_list_has_string. This alters the output of "remote
show" slightly, so update the tests to match.
* Simplify get_ref_states(); nothing is using the pointer to states that
is being copied into util.
* Have get_remote_ref_states() populate states->tracked even when it is
not querying the remote so that this need not be done by the caller.
Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
I added a function prototype for get_remote_ref_states() so I didn't
need to move its location in this diff, which kept the diff cleaner.
The next patch then moves the function and gets rid of the prototype.
builtin-remote.c | 45 ++++++++++++++++++++++-----------------------
t/t5505-remote.sh | 2 +-
2 files changed, 23 insertions(+), 24 deletions(-)
diff --git a/builtin-remote.c b/builtin-remote.c
index d6958d4..ea5e808 100644
--- a/builtin-remote.c
+++ b/builtin-remote.c
@@ -23,6 +23,9 @@ static int verbose;
static int show_all(void);
+static int append_ref_to_tracked_list(const char *refname,
+ const unsigned char *sha1, int flags, void *cb_data);
+
static inline int postfixcmp(const char *string, const char *postfix)
{
int len1 = strlen(string), len2 = strlen(postfix);
@@ -144,7 +147,7 @@ static int add(int argc, const char **argv)
}
struct branch_info {
- char *remote;
+ char *remote_name;
struct string_list merge;
};
@@ -183,9 +186,9 @@ static int config_read_branches(const char *key, const char *value, void *cb)
item->util = xcalloc(sizeof(struct branch_info), 1);
info = item->util;
if (type == REMOTE) ...Relocate the append_ref_to_tracked_list() function above all its callers
so that we can get rid of the prototype.
Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
Doing this as a separate patch makes the prior patch easier to review,
but I think it could be squashed when you apply the series.
builtin-remote.c | 37 +++++++++++++++++--------------------
1 files changed, 17 insertions(+), 20 deletions(-)
diff --git a/builtin-remote.c b/builtin-remote.c
index ea5e808..b61f754 100644
--- a/builtin-remote.c
+++ b/builtin-remote.c
@@ -23,9 +23,6 @@ static int verbose;
static int show_all(void);
-static int append_ref_to_tracked_list(const char *refname,
- const unsigned char *sha1, int flags, void *cb_data);
-
static inline int postfixcmp(const char *string, const char *postfix)
{
int len1 = strlen(string), len2 = strlen(postfix);
@@ -659,6 +656,23 @@ static void free_remote_ref_states(struct ref_states *states)
string_list_clear(&states->heads, 0);
}
+static int append_ref_to_tracked_list(const char *refname,
+ const unsigned char *sha1, int flags, void *cb_data)
+{
+ struct ref_states *states = cb_data;
+ struct refspec refspec;
+
+ if (flags & REF_ISSYMREF)
+ return 0;
+
+ memset(&refspec, 0, sizeof(refspec));
+ refspec.dst = (char *)refname;
+ if (!remote_find_tracking(states->remote, &refspec))
+ string_list_append(abbrev_branch(refspec.src), &states->tracked);
+
+ return 0;
+}
+
static int get_remote_ref_states(const char *name,
struct ref_states *states,
int query)
@@ -688,23 +702,6 @@ static int get_remote_ref_states(const char *name,
return 0;
}
-static int append_ref_to_tracked_list(const char *refname,
- const unsigned char *sha1, int flags, void *cb_data)
-{
- struct ref_states *states = cb_data;
- struct refspec refspec;
-
- if (flags & REF_ISSYMREF)
- return 0;
-
- memset(&refspec, 0, sizeof(refspec));
- refspec.dst = (char *)refname;
- if (!remote_find_tracking(states->remote, ...Give string-list a convenience function for iterating over each of the
items in a string_list.
Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
string-list.c | 10 ++++++++++
string-list.h | 5 +++++
2 files changed, 15 insertions(+), 0 deletions(-)
diff --git a/string-list.c b/string-list.c
index 15e14cf..1ac536e 100644
--- a/string-list.c
+++ b/string-list.c
@@ -92,6 +92,16 @@ struct string_list_item *string_list_lookup(const char *string, struct string_li
return list->items + i;
}
+int for_each_string_list(string_list_each_func_t fn,
+ struct string_list *list, void *cb_data)
+{
+ int i, ret = 0;
+ for (i = 0; i < list->nr; i++)
+ if ((ret = fn(&list->items[i], cb_data)))
+ break;
+ return ret;
+}
+
void string_list_clear(struct string_list *list, int free_util)
{
if (list->items) {
diff --git a/string-list.h b/string-list.h
index d32ba05..14bbc47 100644
--- a/string-list.h
+++ b/string-list.h
@@ -20,6 +20,11 @@ void string_list_clear(struct string_list *list, int free_util);
typedef void (*string_list_clear_func_t)(void *p, const char *str);
void string_list_clear_func(struct string_list *list, string_list_clear_func_t clearfunc);
+/* Use this function to iterate over each item */
+typedef int (*string_list_each_func_t)(struct string_list_item *, void *);
+int for_each_string_list(string_list_each_func_t,
+ struct string_list *list, void *cb_data);
+
/* Use these functions only on sorted lists: */
int string_list_has_string(const struct string_list *list, const char *string);
int string_list_find_insert_index(const struct string_list *list, const char *string,
--
1.6.2.rc1.218.g1b4fab
--
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:
$ 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 w/remote foo, bar, baz, 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 w/remote foo, bar, baz, frotz
Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
Dscho, using --patience made this more readable. So thank you for
adding that.
builtin-remote.c | 172 ++++++++++++++++++++++++++++++++++++++--------------
t/t5505-remote.sh | 32 +++++-----
2 files changed, 141 insertions(+), 63 ...Jay, thanks a ton! I'd been poking at this myself off and on over the last week -- I'm very glad to see you've done the work! A couple of suggestions on the new format: First, a nit: I don't know if the "w/remote" notation makes sense to non-English speakers. I also like the alignment achieved by "merges with remote " (note the trailing space). Second, I think it would be good to also change the format of the 'git push' list, for consistency: Local branches configured for 'git push': master fast-forwards remote upstream refs/tags/lastbackup updates remote refs/tags/lastbackup Though I'm not really happy using "updates" when + is specified in the push refspec. What, precisely, is a "non-fast forward update" anyway? Is it essentially a rebase? If so, maybe "rebases onto remote " would be better (again with a trailing space to get nice alignment). Thanks again! M. --
A non-fast forward update in terms of pushing means you overwrite whatever the remote currently has set as that branch's head. This can be desirable in a private repository or branch, but is usually not desired on shared branches. -- Cheers, Sverre Rabbelier --
Let me see if I understand what you're saying. If my local repo has o--o--o--A--B <-- origin/thing \ X--Y--Z <-- mystuff and I do 'git push origin +mystuff:thing', does the origin end up with A--B <-- (branch with no symbolic reference) / o--o--o--X--Y--Z <-- origin/thing So commits A and B are basically left dangling? If that's the case, then I'd say "replaces" or "overwrites" is the right word to use in the 'remote show' output. But more importantly, I think the 'git push' man page needs to explain this! M. --
Correct, that's why it's not desirable to do so when it's not your From my cursory glance the term 'non-fast forward' is indeed not explained in the man-page, feel like writing a patch? ;) -- Cheers, Sverre Rabbelier --
I tried out a few different option and none was very satisfactory to me.
1)
master rebases onto remote master
another-branch merges with remote next
some-other-branch rebases onto remote master
Here, the unaligned "with" and "onto" is ugly.
2)
master rebases onto remote master
another-branch merges with remote next
some-other-branch rebases onto remote master
This looks better to me. However, if none of your branches rebase,
then the extra space looks like it is a mistake. e.g.:
master merges with remote master
another-branch merges with remote next
some-other-branch merges with remote master
I could add code to detect whether all the branches merge and then not
output the space, but, sigh. And I couldn't think of any other
combination of words that had the same character spacing.
So that's how I ended up with "merges w/remote". This is also slightly
less wide. I always try to have output fit into 80 columns (how
quaint, I know) and a merging branch might have multiple upstreams.
e.g.
another-branch merges w/foo, bar, baz
IOW, the output in the patch wasn't arbitrary. I did think about it
quite a bit. Which isn't to say it's right, just it's the best I came
up with.
I'm somewhat confused by "w/remote" making sense to non-English
I left that out on purpose. The only folks with push refspecs put them
their manually, and the raw refspec is clearer and more concise than
any English words can convey. That was my reasoning anyway.
Thanks for the feedback.
j.
--
I also had a version that used the word "upstream" instead of remote, and another that put the actual remote name in place. e.g. master merges with upstream master master merges with origin master (assuming remote is "origin" of course.) j. --
or
3)
master rebases onto remote master
another-branch merges with remote next
some-other-branch rebases onto remote master
or
another-branch merges with foo
and with bar
and with baz
But generally the output is not as idomatic as w/foo. If English is a
non-native language then "with foo" is going to be much clearer - and if
--
Julian
---
"You're just the sort of person I imagined marrying, when I was little...
except, y'know, not green... and without all the patches of fungus."
-- Swamp Thing
--
All valid points. But, dang it, I really think intelligently adding that extra space the right thing to do. Quite right. Julian made the point better than I: it's fairly idiomatic. I think it's reasonable to assume that many users of the English version of git aren't native English speakers. Fair enough. Thanks again! M. --
What is that about everything being easy for the man who doesn't have
to do it? :-)
(It was no big deal, I just needed to rework a few things cause I'd
Yeah, I'm fixing this too by properly expanded out the refspecs the
same way that it's done by {builtin-send-pack, http-push}.c.
Hopefully I can have a patch out later today.
j.
--
Any reason why this list of branches is now horizontal instead of vertical? I must admit that I don't know what is meant by "HEAD branches". Can this list grow large? I'm asking because I changed horizontal branch lists of 'remote show' output to vertical lists some time ago because I found that vertical lists are much easier to read if they grow large. -- Hannes --
Not a good one, but I thought it fit better w/how an octopus branch The git:// protocol does not currently have a way to determine what a remote sym ref is pointed to, so it guesses by matching SHA1s. In theory this list is unbounded, in practice I don't think it is likely to be large. I like the suggestion above about how to display the octopus merge tho, so I will put this back how it was. j. --
This does too many things in a single patch.
Ideally this would have been four patches for reviewability:
- one "trivial and obviously correct bits" (s/remote/remote_name/ and
removal of sort_string_list(&branch_list)) patch;
- the change for states->{new,tracked}, should stand on its own; I think
the reordering of the output should be described much better and
defended independently. "Earlier it was sorted by this order, which
did not make sense for such and such reasons; this fixes the logic to
sort the list by the name of the tracked branch, which makes it easier
to read", or something like that.
- change to the states->tracked population rule; and
- get_ref_states() to lose the util bit.
It probably is Ok to squash the last two, though.
--
Okay. I'm re-doing 4/4 anyway, so I'll just re-do the series. j. --
