# [RFC/PATCH 0/5] making upstream branch information accessible 24 messages from 2009-04-07 to 2009-04-13. Participants: Jeff King, Bert Wesarg, Michael J Gruber, Paolo Ciarrocchi, Junio C Hamano, Santi Béjar, Wincent Colaiuta. Thread: https://gitlist.dev/t/18762 ## Jeff King, 2009-04-07 07:02 Subject: [RFC/PATCH 0/5] making upstream branch information accessible Message-ID: <20090407070254.GA2870@coredump.intra.peff.net> URL: https://gitlist.dev/e/20090407070254.GA2870%40coredump.intra.peff.net ``` Here are slightly more cleaned-up versions of patches I've thrown out in the last few days. The aim is to make the information on upstream/tracking relationships more accessible both to scripts and to users. 1/5: for-each-ref: refactor get_short_ref function 2/5: for-each-ref: refactor refname handling Cleanup for 3/5. 3/5: for-each-ref: add "upstream" format field Plumbing support. 4/5: make get_short_ref a public function Clean up for 4/5. Builds on 1/5. 5/5: branch: show upstream branch when double verbose -Peff ``` ## Jeff King, 2009-04-07 07:05 Subject: [PATCH 1/5] for-each-ref: refactor get_short_ref function Message-ID: <20090407070501.GA2924@coredump.intra.peff.net> URL: https://gitlist.dev/e/20090407070501.GA2924%40coredump.intra.peff.net In-Reply-To: <20090407070254.GA2870@coredump.intra.peff.net> ``` This function took a "refinfo" object which is unnecessarily restrictive; it only ever looked at the refname field. This patch refactors it to take just the ref name as a string. While we're touching the relevant lines, let's give it consistent memory semantics. Previously, some code paths would return an allocated string and some would return the original string; now it will always return a malloc'd string. This doesn't actually fix a bug or a leak, because for-each-ref doesn't clean up its memory, but it makes the function a lot less surprising for reuse (which will happen in a later patch). Signed-off-by: Jeff King --- Actually, as the modified version is always prefix-shortened, it should be possible to rewrite this in a way that never allocates memory. I picked the least-invasive and most lazy approach. builtin-for-each-ref.c | 14 +++++++------- 1 files changed, 7 insertions(+), 7 deletions(-) diff --git a/builtin-for-each-ref.c b/builtin-for-each-ref.c index 5cbb4b0..4aaf75c 100644 --- a/builtin-for-each-ref.c +++ b/builtin-for-each-ref.c @@ -569,7 +569,7 @@ static void gen_scanf_fmt(char *scanf_fmt, const char *rule) /* * Shorten the refname to an non-ambiguous form */ -static char *get_short_ref(struct refinfo *ref) +static char *get_short_ref(const char *ref) { int i; static char **scanf_fmts; @@ -598,17 +598,17 @@ static char *get_short_ref(struct refinfo *ref) /* bail out if there are no rules */ if (!nr_rules) - return ref->refname; + return xstrdup(ref); - /* buffer for scanf result, at most ref->refname must fit */ - short_name = xstrdup(ref->refname); + /* buffer for scanf result, at most ref must fit */ + short_name = xstrdup(ref); /* skip first rule, it will always match */ for (i = nr_rules - 1; i > 0 ; --i) { int j; int short_name_len; - if (1 != sscanf(ref->refname, scanf_fmts[i], short_name)) + if (1 != sscanf(ref, scanf_fmts[i], short_name)) continue; short_name_len = strlen(short_name); @@ -642,7 +642,7 @@ static char *get_short_ref(struct refinfo *ref) } free(short_name); - return ref->refname; + return xstrdup(ref); } @@ -684,7 +684,7 @@ static void populate_value(struct refinfo *ref) if (formatp) { formatp++; if (!strcmp(formatp, "short")) - refname = get_short_ref(ref); + refname = get_short_ref(ref->refname); else die("unknown refname format %s", formatp); -- 1.6.2.2.450.gd6aa9.dirty ``` ## Jeff King, 2009-04-07 07:06 Subject: [PATCH 2/5] for-each-ref: refactor refname handling Message-ID: <20090407070651.GB2924@coredump.intra.peff.net> URL: https://gitlist.dev/e/20090407070651.GB2924%40coredump.intra.peff.net In-Reply-To: <20090407070254.GA2870@coredump.intra.peff.net> ``` This code handles some special magic like *-deref and the :short formatting specifier. The next patch will add another field which outputs a ref and wants to use the same code. This patch splits the "which ref are we outputting" from the actual formatting. There should be no behavioral change. Signed-off-by: Jeff King --- The diff is scary, but it is mostly reindentation. builtin-for-each-ref.c | 47 ++++++++++++++++++++++++++--------------------- 1 files changed, 26 insertions(+), 21 deletions(-) diff --git a/builtin-for-each-ref.c b/builtin-for-each-ref.c index 4aaf75c..b50c93b 100644 --- a/builtin-for-each-ref.c +++ b/builtin-for-each-ref.c @@ -672,32 +672,37 @@ static void populate_value(struct refinfo *ref) const char *name = used_atom[i]; struct atom_value *v = &ref->value[i]; int deref = 0; + const char *refname; + const char *formatp; + if (*name == '*') { deref = 1; name++; } - if (!prefixcmp(name, "refname")) { - const char *formatp = strchr(name, ':'); - const char *refname = ref->refname; - - /* look for "short" refname format */ - if (formatp) { - formatp++; - if (!strcmp(formatp, "short")) - refname = get_short_ref(ref->refname); - else - die("unknown refname format %s", - formatp); - } - if (!deref) - v->s = refname; - else { - int len = strlen(refname); - char *s = xmalloc(len + 4); - sprintf(s, "%s^{}", refname); - v->s = s; - } + if (!prefixcmp(name, "refname")) + refname = ref->refname; + else + continue; + + formatp = strchr(name, ':'); + /* look for "short" refname format */ + if (formatp) { + formatp++; + if (!strcmp(formatp, "short")) + refname = get_short_ref(refname); + else + die("unknown %.*s format %s", + formatp - name, name, formatp); + } + + if (!deref) + v->s = refname; + else { + int len = strlen(refname); + char *s = xmalloc(len + 4); + sprintf(s, "%s^{}", refname); + v->s = s; } } -- 1.6.2.2.450.gd6aa9.dirty ``` ## Jeff King, 2009-04-07 07:09 Subject: [PATCH 3/5] for-each-ref: add "upstream" format field Message-ID: <20090407070939.GC2924@coredump.intra.peff.net> URL: https://gitlist.dev/e/20090407070939.GC2924%40coredump.intra.peff.net In-Reply-To: <20090407070254.GA2870@coredump.intra.peff.net> ``` The logic for determining the upstream ref of a branch is somewhat complex to perform in a shell script. This patch provides a plumbing mechanism for scripts to access the C logic used internally by git-status, git-branch, etc. For example: $ git for-each-ref \ --format='%(refname:short) %(upstream:short)' \ refs/heads/ master origin/master Signed-off-by: Jeff King --- This is a cleaned-up version of what I sent previously. Mainly just code cleanups, and it no longer frees the branch struct, which seems to be allocated from semi-permanent storage during branch_get. Should the documentation explain the concept of "upstream" more fully? I noticed Santi sent a glossary patch earlier today, so maybe that is enough. Documentation/git-for-each-ref.txt | 5 +++++ builtin-for-each-ref.c | 14 ++++++++++++++ t/t6300-for-each-ref.sh | 22 ++++++++++++++++++++++ 3 files changed, 41 insertions(+), 0 deletions(-) diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt index 5061d3e..b362e9e 100644 --- a/Documentation/git-for-each-ref.txt +++ b/Documentation/git-for-each-ref.txt @@ -85,6 +85,11 @@ objectsize:: objectname:: The object name (aka SHA-1). +upstream:: + The name of a local ref which can be considered ``upstream'' + from the displayed ref. Respects `:short` in the same way as + `refname` above. + In addition to the above, for commit and tag objects, the header field names (`tree`, `parent`, `object`, `type`, and `tag`) can be used to specify the value in the header field. diff --git a/builtin-for-each-ref.c b/builtin-for-each-ref.c index b50c93b..277d1fb 100644 --- a/builtin-for-each-ref.c +++ b/builtin-for-each-ref.c @@ -8,6 +8,7 @@ #include "blob.h" #include "quote.h" #include "parse-options.h" +#include "remote.h" /* Quoting styles */ #define QUOTE_NONE 0 @@ -66,6 +67,7 @@ static struct { { "subject" }, { "body" }, { "contents" }, + { "upstream" }, }; /* @@ -682,6 +684,18 @@ static void populate_value(struct refinfo *ref) if (!prefixcmp(name, "refname")) refname = ref->refname; + else if(!prefixcmp(name, "upstream")) { + struct branch *branch; + /* only local branches may have an upstream */ + if (prefixcmp(ref->refname, "refs/heads/")) + continue; + branch = branch_get(ref->refname + 11); + + if (!branch || !branch->merge || !branch->merge[0] || + !branch->merge[0]->dst) + continue; + refname = branch->merge[0]->dst; + } else continue; diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh index 8bfae44..daf02d5 100755 --- a/t/t6300-for-each-ref.sh +++ b/t/t6300-for-each-ref.sh @@ -26,6 +26,13 @@ test_expect_success 'Create sample commit with known timestamp' ' git tag -a -m "Tagging at $datestamp" testtag ' +test_expect_success 'Create upstream config' ' + git update-ref refs/remotes/origin/master master && + git remote add origin nowhere && + git config branch.master.remote origin && + git config branch.master.merge refs/heads/master +' + test_atom() { case "$1" in head) ref=refs/heads/master ;; @@ -39,6 +46,7 @@ test_atom() { } test_atom head refname refs/heads/master +test_atom head upstream refs/remotes/origin/master test_atom head objecttype commit test_atom head objectsize 171 test_atom head objectname 67a36f10722846e891fbada1ba48ed035de75581 @@ -68,6 +76,7 @@ test_atom head contents 'Initial ' test_atom tag refname refs/tags/testtag +test_atom tag upstream '' test_atom tag objecttype tag test_atom tag objectsize 154 test_atom tag objectname 98b46b1d36e5b07909de1b3886224e3e81e87322 @@ -203,6 +212,7 @@ test_expect_success 'Check format "rfc2822" date fields output' ' cat >expected <<\EOF refs/heads/master +refs/remotes/origin/master refs/tags/testtag EOF @@ -214,6 +224,7 @@ test_expect_success 'Verify ascending sort' ' cat >expected <<\EOF refs/tags/testtag +refs/remotes/origin/master refs/heads/master EOF @@ -224,6 +235,7 @@ test_expect_success 'Verify descending sort' ' cat >expected <<\EOF 'refs/heads/master' +'refs/remotes/origin/master' 'refs/tags/testtag' EOF @@ -244,6 +256,7 @@ test_expect_success 'Quoting style: python' ' cat >expected <<\EOF "refs/heads/master" +"refs/remotes/origin/master" "refs/tags/testtag" EOF @@ -273,6 +286,15 @@ test_expect_success 'Check short refname format' ' test_cmp expected actual ' +cat >expected <actual && + test_cmp expected actual +' + test_expect_success 'Check for invalid refname format' ' test_must_fail git for-each-ref --format="%(refname:INVALID)" ' -- 1.6.2.2.450.gd6aa9.dirty ``` ## Jeff King, 2009-04-07 07:14 Subject: [PATCH 4/5] make get_short_ref a public function Message-ID: <20090407071420.GD2924@coredump.intra.peff.net> URL: https://gitlist.dev/e/20090407071420.GD2924%40coredump.intra.peff.net In-Reply-To: <20090407070254.GA2870@coredump.intra.peff.net> ``` Often we want to shorten a full ref name to something "prettier" to show a user. For example, "refs/heads/master" is often shown simply as "master", or "refs/remotes/origin/master" is shown as "origin/master". Many places in the code use a very simple formula: skip common prefixes like refs/heads, refs/remotes, etc. This is codified in the prettify_ref function. for-each-ref has a more correct (but more expensive) approach: consider the ref lookup rules, and try shortening as much as possible while remaining unambiguous. This patch makes the latter strategy globally available as shorten_unambiguous_ref. Signed-off-by: Jeff King --- Actually, I am not quite sure that this function is "more correct". It looks at the rev-parsing rules as a hierarchy, so if you have "refs/remotes/foo" and "refs/heads/foo", then it will abbreviate the first to "remotes/foo" (as expected) and the latter to just "foo". This is technically correct, as "refs/heads/foo" will be selected by "foo", but it will warn about ambiguity. Should we actually try to avoid reporting refs which would be ambiguous? Should this simply replace prettify_ref (and other places which should be using prettify_ref but aren't)? It is definitely more expensive, as it has to resolve refs to look for ambiguities, but I don't know if we care in most code paths. builtin-for-each-ref.c | 105 +----------------------------------------------- refs.c | 99 +++++++++++++++++++++++++++++++++++++++++++++ refs.h | 1 + 3 files changed, 101 insertions(+), 104 deletions(-) diff --git a/builtin-for-each-ref.c b/builtin-for-each-ref.c index 277d1fb..8c82484 100644 --- a/builtin-for-each-ref.c +++ b/builtin-for-each-ref.c @@ -546,109 +546,6 @@ static void grab_values(struct atom_value *val, int deref, struct object *obj, v } /* - * generate a format suitable for scanf from a ref_rev_parse_rules - * rule, that is replace the "%.*s" spec with a "%s" spec - */ -static void gen_scanf_fmt(char *scanf_fmt, const char *rule) -{ - char *spec; - - spec = strstr(rule, "%.*s"); - if (!spec || strstr(spec + 4, "%.*s")) - die("invalid rule in ref_rev_parse_rules: %s", rule); - - /* copy all until spec */ - strncpy(scanf_fmt, rule, spec - rule); - scanf_fmt[spec - rule] = '\0'; - /* copy new spec */ - strcat(scanf_fmt, "%s"); - /* copy remaining rule */ - strcat(scanf_fmt, spec + 4); - - return; -} - -/* - * Shorten the refname to an non-ambiguous form - */ -static char *get_short_ref(const char *ref) -{ - int i; - static char **scanf_fmts; - static int nr_rules; - char *short_name; - - /* pre generate scanf formats from ref_rev_parse_rules[] */ - if (!nr_rules) { - size_t total_len = 0; - - /* the rule list is NULL terminated, count them first */ - for (; ref_rev_parse_rules[nr_rules]; nr_rules++) - /* no +1 because strlen("%s") < strlen("%.*s") */ - total_len += strlen(ref_rev_parse_rules[nr_rules]); - - scanf_fmts = xmalloc(nr_rules * sizeof(char *) + total_len); - - total_len = 0; - for (i = 0; i < nr_rules; i++) { - scanf_fmts[i] = (char *)&scanf_fmts[nr_rules] - + total_len; - gen_scanf_fmt(scanf_fmts[i], ref_rev_parse_rules[i]); - total_len += strlen(ref_rev_parse_rules[i]); - } - } - - /* bail out if there are no rules */ - if (!nr_rules) - return xstrdup(ref); - - /* buffer for scanf result, at most ref must fit */ - short_name = xstrdup(ref); - - /* skip first rule, it will always match */ - for (i = nr_rules - 1; i > 0 ; --i) { - int j; - int short_name_len; - - if (1 != sscanf(ref, scanf_fmts[i], short_name)) - continue; - - short_name_len = strlen(short_name); - - /* - * check if the short name resolves to a valid ref, - * but use only rules prior to the matched one - */ - for (j = 0; j < i; j++) { - const char *rule = ref_rev_parse_rules[j]; - unsigned char short_objectname[20]; - char refname[PATH_MAX]; - - /* - * the short name is ambiguous, if it resolves - * (with this previous rule) to a valid ref - * read_ref() returns 0 on success - */ - mksnpath(refname, sizeof(refname), - rule, short_name_len, short_name); - if (!read_ref(refname, short_objectname)) - break; - } - - /* - * short name is non-ambiguous if all previous rules - * haven't resolved to a valid ref - */ - if (j == i) - return short_name; - } - - free(short_name); - return xstrdup(ref); -} - - -/* * Parse the object referred by ref, and grab needed value. */ static void populate_value(struct refinfo *ref) @@ -704,7 +601,7 @@ static void populate_value(struct refinfo *ref) if (formatp) { formatp++; if (!strcmp(formatp, "short")) - refname = get_short_ref(refname); + refname = shorten_unambiguous_ref(refname); else die("unknown %.*s format %s", formatp - name, name, formatp); diff --git a/refs.c b/refs.c index 59c373f..1e5e7b4 100644 --- a/refs.c +++ b/refs.c @@ -1652,3 +1652,102 @@ struct ref *find_ref_by_name(const struct ref *list, const char *name) return (struct ref *)list; return NULL; } + +/* + * generate a format suitable for scanf from a ref_rev_parse_rules + * rule, that is replace the "%.*s" spec with a "%s" spec + */ +static void gen_scanf_fmt(char *scanf_fmt, const char *rule) +{ + char *spec; + + spec = strstr(rule, "%.*s"); + if (!spec || strstr(spec + 4, "%.*s")) + die("invalid rule in ref_rev_parse_rules: %s", rule); + + /* copy all until spec */ + strncpy(scanf_fmt, rule, spec - rule); + scanf_fmt[spec - rule] = '\0'; + /* copy new spec */ + strcat(scanf_fmt, "%s"); + /* copy remaining rule */ + strcat(scanf_fmt, spec + 4); + + return; +} + +char *shorten_unambiguous_ref(const char *ref) +{ + int i; + static char **scanf_fmts; + static int nr_rules; + char *short_name; + + /* pre generate scanf formats from ref_rev_parse_rules[] */ + if (!nr_rules) { + size_t total_len = 0; + + /* the rule list is NULL terminated, count them first */ + for (; ref_rev_parse_rules[nr_rules]; nr_rules++) + /* no +1 because strlen("%s") < strlen("%.*s") */ + total_len += strlen(ref_rev_parse_rules[nr_rules]); + + scanf_fmts = xmalloc(nr_rules * sizeof(char *) + total_len); + + total_len = 0; + for (i = 0; i < nr_rules; i++) { + scanf_fmts[i] = (char *)&scanf_fmts[nr_rules] + + total_len; + gen_scanf_fmt(scanf_fmts[i], ref_rev_parse_rules[i]); + total_len += strlen(ref_rev_parse_rules[i]); + } + } + + /* bail out if there are no rules */ + if (!nr_rules) + return xstrdup(ref); + + /* buffer for scanf result, at most ref must fit */ + short_name = xstrdup(ref); + + /* skip first rule, it will always match */ + for (i = nr_rules - 1; i > 0 ; --i) { + int j; + int short_name_len; + + if (1 != sscanf(ref, scanf_fmts[i], short_name)) + continue; + + short_name_len = strlen(short_name); + + /* + * check if the short name resolves to a valid ref, + * but use only rules prior to the matched one + */ + for (j = 0; j < i; j++) { + const char *rule = ref_rev_parse_rules[j]; + unsigned char short_objectname[20]; + char refname[PATH_MAX]; + + /* + * the short name is ambiguous, if it resolves + * (with this previous rule) to a valid ref + * read_ref() returns 0 on success + */ + mksnpath(refname, sizeof(refname), + rule, short_name_len, short_name); + if (!read_ref(refname, short_objectname)) + break; + } + + /* + * short name is non-ambiguous if all previous rules + * haven't resolved to a valid ref + */ + if (j == i) + return short_name; + } + + free(short_name); + return xstrdup(ref); +} diff --git a/refs.h b/refs.h index 68c2d16..2d0f961 100644 --- a/refs.h +++ b/refs.h @@ -80,6 +80,7 @@ extern int for_each_reflog(each_ref_fn, void *); extern int check_ref_format(const char *target); extern const char *prettify_ref(const struct ref *ref); +extern char *shorten_unambiguous_ref(const char *ref); /** rename ref, return 0 on success **/ extern int rename_ref(const char *oldref, const char *newref, const char *logmsg); -- 1.6.2.2.450.gd6aa9.dirty ``` ## Jeff King, 2009-04-07 07:16 Subject: [PATCH 5/5] branch: show upstream branch when double verbose Message-ID: <20090407071656.GE2924@coredump.intra.peff.net> URL: https://gitlist.dev/e/20090407071656.GE2924%40coredump.intra.peff.net In-Reply-To: <20090407070254.GA2870@coredump.intra.peff.net> ``` This information is easily accessible when we are calculating the relationship. The only reason not to print it all the time is that it consumes a fair bit of screen space, and may not be of interest to the user. Signed-off-by: Jeff King --- This one is very RFC. Should this information be part of the regular "-v"? Should it be part of "git branch" with regular verbosity? Should the format be different? I wonder if master 1234abcd [origin/master: ahead 5, behind 6] whatever will be interpreted as "origin/master is ahead 5, behind 6" when it is really the reverse. Maybe "[ahead 5, behind 6 from origin/master]" would be better? Documentation/git-branch.txt | 4 +++- builtin-branch.c | 23 +++++++++++++++++------ 2 files changed, 20 insertions(+), 7 deletions(-) diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt index 31ba7f2..ba3dea6 100644 --- a/Documentation/git-branch.txt +++ b/Documentation/git-branch.txt @@ -100,7 +100,9 @@ OPTIONS -v:: --verbose:: - Show sha1 and commit subject line for each head. + Show sha1 and commit subject line for each head, along with + relationship to upstream branch (if any). If given twice, print + the name of the upstream branch, as well. --abbrev=:: Alter the sha1's minimum display length in the output listing. diff --git a/builtin-branch.c b/builtin-branch.c index ca81d72..3275821 100644 --- a/builtin-branch.c +++ b/builtin-branch.c @@ -301,19 +301,30 @@ static int ref_cmp(const void *r1, const void *r2) return strcmp(c1->name, c2->name); } -static void fill_tracking_info(struct strbuf *stat, const char *branch_name) +static void fill_tracking_info(struct strbuf *stat, const char *branch_name, + int show_upstream_ref) { int ours, theirs; struct branch *branch = branch_get(branch_name); - if (!stat_tracking_info(branch, &ours, &theirs) || (!ours && !theirs)) + if (!stat_tracking_info(branch, &ours, &theirs)) { + if (branch && branch->merge && branch->merge[0]->dst && + show_upstream_ref) + strbuf_addf(stat, "[%s] ", + shorten_unambiguous_ref(branch->merge[0]->dst)); return; + } + + strbuf_addch(stat, '['); + if (show_upstream_ref) + strbuf_addf(stat, "%s: ", + shorten_unambiguous_ref(branch->merge[0]->dst)); if (!ours) - strbuf_addf(stat, "[behind %d] ", theirs); + strbuf_addf(stat, "behind %d] ", theirs); else if (!theirs) - strbuf_addf(stat, "[ahead %d] ", ours); + strbuf_addf(stat, "ahead %d] ", ours); else - strbuf_addf(stat, "[ahead %d, behind %d] ", ours, theirs); + strbuf_addf(stat, "ahead %d, behind %d] ", ours, theirs); } static int matches_merge_filter(struct commit *commit) @@ -379,7 +390,7 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose, } if (item->kind == REF_LOCAL_BRANCH) - fill_tracking_info(&stat, item->name); + fill_tracking_info(&stat, item->name, verbose > 1); strbuf_addf(&out, " %s %s%s", find_unique_abbrev(item->commit->object.sha1, abbrev), -- 1.6.2.2.450.gd6aa9.dirty ``` ## Bert Wesarg, 2009-04-07 07:33 Subject: [PATCH] for-each-ref: remove multiple xstrdup() in get_short_ref() Message-ID: <1239089599-24760-1-git-send-email-bert.wesarg@googlemail.com> URL: https://gitlist.dev/e/1239089599-24760-1-git-send-email-bert.wesarg%40googlemail.com In-Reply-To: <20090407070254.GA2870@coredump.intra.peff.net> ``` Now that get_short_ref() always return an malloced string, consolidate to one xstrcpy() call. Signed-off-by: Bert Wesarg --- Also an Acked-by: Bert Wesarg for Jeffs patch. builtin-for-each-ref.c | 11 +++++------ 1 files changed, 5 insertions(+), 6 deletions(-) diff --git a/builtin-for-each-ref.c b/builtin-for-each-ref.c index 4aaf75c..108c128 100644 --- a/builtin-for-each-ref.c +++ b/builtin-for-each-ref.c @@ -596,13 +596,13 @@ static char *get_short_ref(const char *ref) } } - /* bail out if there are no rules */ - if (!nr_rules) - return xstrdup(ref); - /* buffer for scanf result, at most ref must fit */ short_name = xstrdup(ref); + /* bail out if there are no rules */ + if (!nr_rules) + return short_name; + /* skip first rule, it will always match */ for (i = nr_rules - 1; i > 0 ; --i) { int j; @@ -641,8 +641,7 @@ static char *get_short_ref(const char *ref) return short_name; } - free(short_name); - return xstrdup(ref); + return strcpy(short_name, ref); } -- tg: (e9786d7..) bw/ammend-jk/refactor-get_short_ref (depends on: jk/refactor-get_short_ref) ``` ## Bert Wesarg, 2009-04-07 07:39 Subject: Re: [PATCH 4/5] make get_short_ref a public function Message-ID: <36ca99e90904070039m15869c34jc9e12d5ccc48d82@mail.gmail.com> URL: https://gitlist.dev/e/36ca99e90904070039m15869c34jc9e12d5ccc48d82%40mail.gmail.com In-Reply-To: <20090407071420.GD2924@coredump.intra.peff.net> ``` On Tue, Apr 7, 2009 at 09:14, Jeff King wrote: > Often we want to shorten a full ref name to something "prettier" > to show a user. For example, "refs/heads/master" is often shown > simply as "master", or "refs/remotes/origin/master" is shown as > "origin/master". > > Many places in the code use a very simple formula: skip common > prefixes like refs/heads, refs/remotes, etc. This is codified in > the prettify_ref function. > > for-each-ref has a more correct (but more expensive) approach: > consider the ref lookup rules, and try shortening as much as > possible while remaining unambiguous. > > This patch makes the latter strategy globally available as > shorten_unambiguous_ref. > > Signed-off-by: Jeff King > --- > Actually, I am not quite sure that this function is "more correct". It > looks at the rev-parsing rules as a hierarchy, so if you have > "refs/remotes/foo" and "refs/heads/foo", then it will abbreviate the > first to "remotes/foo" (as expected) and the latter to just "foo". > > This is technically correct, as "refs/heads/foo" will be selected by > "foo", but it will warn about ambiguity. Should we actually try to avoid > reporting refs which would be ambiguous? Back than, there was the idea that the core.warnAmbiguousRefs config could be used for this. Anyway Acked-by: Bert Wesarg ``` ## Jeff King, 2009-04-07 07:44 Subject: Re: [PATCH] for-each-ref: remove multiple xstrdup() in get_short_ref() Message-ID: <20090407074435.GB7327@coredump.intra.peff.net> URL: https://gitlist.dev/e/20090407074435.GB7327%40coredump.intra.peff.net In-Reply-To: <1239089599-24760-1-git-send-email-bert.wesarg@googlemail.com> ``` On Tue, Apr 07, 2009 at 09:33:19AM +0200, Bert Wesarg wrote: > Now that get_short_ref() always return an malloced string, consolidate to > one xstrcpy() call. Makes sense to squash in on top of what I have. But I think it actually is pretty easy to always return a pointer into the existing string (patch based on current master): --- diff --git a/builtin-for-each-ref.c b/builtin-for-each-ref.c index 5cbb4b0..8b24a4a 100644 --- a/builtin-for-each-ref.c +++ b/builtin-for-each-ref.c @@ -569,7 +569,7 @@ static void gen_scanf_fmt(char *scanf_fmt, const char *rule) /* * Shorten the refname to an non-ambiguous form */ -static char *get_short_ref(struct refinfo *ref) +static const char *get_short_ref(const char *ref) { int i; static char **scanf_fmts; @@ -598,17 +598,17 @@ static char *get_short_ref(struct refinfo *ref) /* bail out if there are no rules */ if (!nr_rules) - return ref->refname; + return ref; /* buffer for scanf result, at most ref->refname must fit */ - short_name = xstrdup(ref->refname); + short_name = xstrdup(ref); /* skip first rule, it will always match */ for (i = nr_rules - 1; i > 0 ; --i) { int j; int short_name_len; - if (1 != sscanf(ref->refname, scanf_fmts[i], short_name)) + if (1 != sscanf(ref, scanf_fmts[i], short_name)) continue; short_name_len = strlen(short_name); @@ -637,12 +637,14 @@ static char *get_short_ref(struct refinfo *ref) * short name is non-ambiguous if all previous rules * haven't resolved to a valid ref */ - if (j == i) - return short_name; + if (j == i) { + ref += strlen(ref) - strlen(short_name); + break; + } } free(short_name); - return ref->refname; + return ref; } @@ -684,7 +686,7 @@ static void populate_value(struct refinfo *ref) if (formatp) { formatp++; if (!strcmp(formatp, "short")) - refname = get_short_ref(ref); + refname = get_short_ref(ref->refname); else die("unknown refname format %s", formatp); ``` ## Bert Wesarg, 2009-04-07 07:44 Subject: Re: [PATCH] for-each-ref: remove multiple xstrdup() in get_short_ref() Message-ID: <36ca99e90904070044p48a4fe7bwb7278bde2d64837c@mail.gmail.com> URL: https://gitlist.dev/e/36ca99e90904070044p48a4fe7bwb7278bde2d64837c%40mail.gmail.com In-Reply-To: <1239089599-24760-1-git-send-email-bert.wesarg@googlemail.com> ``` On Tue, Apr 7, 2009 at 09:33, Bert Wesarg wrote: > Now that get_short_ref() always return an malloced string, consolidate to > one xstrcpy() call. > > Signed-off-by: Bert Wesarg > > --- > Also an > > Acked-by: Bert Wesarg Sorry, wrong Message-ID for In-Reply-To, this Ack is for: [PATCH 1/5] for-each-ref: refactor get_short_ref function with Message-ID <20090407070501.GA2924@coredump.intra.peff.net> Bert ``` ## Bert Wesarg, 2009-04-07 07:54 Subject: Re: [PATCH] for-each-ref: remove multiple xstrdup() in get_short_ref() Message-ID: <36ca99e90904070054y3bbd21e0g44548162e71f3a13@mail.gmail.com> URL: https://gitlist.dev/e/36ca99e90904070054y3bbd21e0g44548162e71f3a13%40mail.gmail.com In-Reply-To: <20090407074435.GB7327@coredump.intra.peff.net> ``` On Tue, Apr 7, 2009 at 09:44, Jeff King wrote: > On Tue, Apr 07, 2009 at 09:33:19AM +0200, Bert Wesarg wrote: > >> Now that get_short_ref() always return an malloced string, consolidate to >> one xstrcpy() call. > > Makes sense to squash in on top of what I have. But I think it actually > is pretty easy to always return a pointer into the existing string > (patch based on current master): Yes, thats probably a good idea. The caller can always do a xstrdup(get_short_ref(ref)). > @@ -637,12 +637,14 @@ static char *get_short_ref(struct refinfo *ref) >                 * short name is non-ambiguous if all previous rules >                 * haven't resolved to a valid ref >                 */ > -               if (j == i) > -                       return short_name; > +               if (j == i) { > +                       ref += strlen(ref) - strlen(short_name); we have strlen(short_name) in short_name_len already. Bert ``` ## Michael J Gruber, 2009-04-07 07:57 Subject: Re: [PATCH 4/5] make get_short_ref a public function Message-ID: <49DB077A.1060506@drmicha.warpmail.net> URL: https://gitlist.dev/e/49DB077A.1060506%40drmicha.warpmail.net In-Reply-To: <20090407071420.GD2924@coredump.intra.peff.net> ``` Jeff King venit, vidit, dixit 07.04.2009 09:14: > Often we want to shorten a full ref name to something "prettier" > to show a user. For example, "refs/heads/master" is often shown > simply as "master", or "refs/remotes/origin/master" is shown as > "origin/master". > > Many places in the code use a very simple formula: skip common > prefixes like refs/heads, refs/remotes, etc. This is codified in > the prettify_ref function. > > for-each-ref has a more correct (but more expensive) approach: > consider the ref lookup rules, and try shortening as much as > possible while remaining unambiguous. > > This patch makes the latter strategy globally available as > shorten_unambiguous_ref. > > Signed-off-by: Jeff King > --- > Actually, I am not quite sure that this function is "more correct". It > looks at the rev-parsing rules as a hierarchy, so if you have > "refs/remotes/foo" and "refs/heads/foo", then it will abbreviate the > first to "remotes/foo" (as expected) and the latter to just "foo". > > This is technically correct, as "refs/heads/foo" will be selected by > "foo", but it will warn about ambiguity. Should we actually try to avoid > reporting refs which would be ambiguous? > > Should this simply replace prettify_ref (and other places which should > be using prettify_ref but aren't)? It is definitely more expensive, as > it has to resolve refs to look for ambiguities, but I don't know if we > care in most code paths. I would think that as long as the default is to warn about ambiguous refs we should not generate ambiguous refs... Other than that it's very nice, it can be used in many places. > > builtin-for-each-ref.c | 105 +----------------------------------------------- > refs.c | 99 +++++++++++++++++++++++++++++++++++++++++++++ > refs.h | 1 + > 3 files changed, 101 insertions(+), 104 deletions(-) > > diff --git a/builtin-for-each-ref.c b/builtin-for-each-ref.c > index 277d1fb..8c82484 100644 > --- a/builtin-for-each-ref.c > +++ b/builtin-for-each-ref.c > @@ -546,109 +546,6 @@ static void grab_values(struct atom_value *val, int deref, struct object *obj, v > } > > /* > - * generate a format suitable for scanf from a ref_rev_parse_rules > - * rule, that is replace the "%.*s" spec with a "%s" spec > - */ > -static void gen_scanf_fmt(char *scanf_fmt, const char *rule) > -{ > - char *spec; > - > - spec = strstr(rule, "%.*s"); > - if (!spec || strstr(spec + 4, "%.*s")) > - die("invalid rule in ref_rev_parse_rules: %s", rule); > - > - /* copy all until spec */ > - strncpy(scanf_fmt, rule, spec - rule); > - scanf_fmt[spec - rule] = '\0'; > - /* copy new spec */ > - strcat(scanf_fmt, "%s"); > - /* copy remaining rule */ > - strcat(scanf_fmt, spec + 4); > - > - return; > -} > - > -/* > - * Shorten the refname to an non-ambiguous form > - */ > -static char *get_short_ref(const char *ref) > -{ > - int i; > - static char **scanf_fmts; > - static int nr_rules; > - char *short_name; > - > - /* pre generate scanf formats from ref_rev_parse_rules[] */ > - if (!nr_rules) { > - size_t total_len = 0; > - > - /* the rule list is NULL terminated, count them first */ > - for (; ref_rev_parse_rules[nr_rules]; nr_rules++) > - /* no +1 because strlen("%s") < strlen("%.*s") */ > - total_len += strlen(ref_rev_parse_rules[nr_rules]); > - > - scanf_fmts = xmalloc(nr_rules * sizeof(char *) + total_len); > - > - total_len = 0; > - for (i = 0; i < nr_rules; i++) { > - scanf_fmts[i] = (char *)&scanf_fmts[nr_rules] > - + total_len; > - gen_scanf_fmt(scanf_fmts[i], ref_rev_parse_rules[i]); > - total_len += strlen(ref_rev_parse_rules[i]); > - } > - } > - > - /* bail out if there are no rules */ > - if (!nr_rules) > - return xstrdup(ref); > - > - /* buffer for scanf result, at most ref must fit */ > - short_name = xstrdup(ref); > - > - /* skip first rule, it will always match */ > - for (i = nr_rules - 1; i > 0 ; --i) { > - int j; > - int short_name_len; > - > - if (1 != sscanf(ref, scanf_fmts[i], short_name)) > - continue; > - > - short_name_len = strlen(short_name); > - > - /* > - * check if the short name resolves to a valid ref, > - * but use only rules prior to the matched one > - */ > - for (j = 0; j < i; j++) { > - const char *rule = ref_rev_parse_rules[j]; > - unsigned char short_objectname[20]; > - char refname[PATH_MAX]; > - > - /* > - * the short name is ambiguous, if it resolves > - * (with this previous rule) to a valid ref > - * read_ref() returns 0 on success > - */ > - mksnpath(refname, sizeof(refname), > - rule, short_name_len, short_name); > - if (!read_ref(refname, short_objectname)) > - break; > - } > - > - /* > - * short name is non-ambiguous if all previous rules > - * haven't resolved to a valid ref > - */ > - if (j == i) > - return short_name; > - } > - > - free(short_name); > - return xstrdup(ref); > -} > - > - > -/* > * Parse the object referred by ref, and grab needed value. > */ > static void populate_value(struct refinfo *ref) > @@ -704,7 +601,7 @@ static void populate_value(struct refinfo *ref) > if (formatp) { > formatp++; > if (!strcmp(formatp, "short")) > - refname = get_short_ref(refname); > + refname = shorten_unambiguous_ref(refname); > else > die("unknown %.*s format %s", > formatp - name, name, formatp); > diff --git a/refs.c b/refs.c > index 59c373f..1e5e7b4 100644 > --- a/refs.c > +++ b/refs.c > @@ -1652,3 +1652,102 @@ struct ref *find_ref_by_name(const struct ref *list, const char *name) > return (struct ref *)list; > return NULL; > } > + > +/* > + * generate a format suitable for scanf from a ref_rev_parse_rules > + * rule, that is replace the "%.*s" spec with a "%s" spec > + */ > +static void gen_scanf_fmt(char *scanf_fmt, const char *rule) > +{ > + char *spec; > + > + spec = strstr(rule, "%.*s"); > + if (!spec || strstr(spec + 4, "%.*s")) > + die("invalid rule in ref_rev_parse_rules: %s", rule); > + > + /* copy all until spec */ > + strncpy(scanf_fmt, rule, spec - rule); > + scanf_fmt[spec - rule] = '\0'; > + /* copy new spec */ > + strcat(scanf_fmt, "%s"); > + /* copy remaining rule */ > + strcat(scanf_fmt, spec + 4); > + > + return; > +} > + > +char *shorten_unambiguous_ref(const char *ref) > +{ > + int i; > + static char **scanf_fmts; > + static int nr_rules; > + char *short_name; > + > + /* pre generate scanf formats from ref_rev_parse_rules[] */ > + if (!nr_rules) { > + size_t total_len = 0; > + > + /* the rule list is NULL terminated, count them first */ > + for (; ref_rev_parse_rules[nr_rules]; nr_rules++) > + /* no +1 because strlen("%s") < strlen("%.*s") */ > + total_len += strlen(ref_rev_parse_rules[nr_rules]); > + > + scanf_fmts = xmalloc(nr_rules * sizeof(char *) + total_len); > + > + total_len = 0; > + for (i = 0; i < nr_rules; i++) { > + scanf_fmts[i] = (char *)&scanf_fmts[nr_rules] > + + total_len; > + gen_scanf_fmt(scanf_fmts[i], ref_rev_parse_rules[i]); > + total_len += strlen(ref_rev_parse_rules[i]); > + } > + } > + > + /* bail out if there are no rules */ > + if (!nr_rules) > + return xstrdup(ref); > + > + /* buffer for scanf result, at most ref must fit */ > + short_name = xstrdup(ref); > + > + /* skip first rule, it will always match */ > + for (i = nr_rules - 1; i > 0 ; --i) { > + int j; > + int short_name_len; > + > + if (1 != sscanf(ref, scanf_fmts[i], short_name)) > + continue; > + > + short_name_len = strlen(short_name); > + > + /* > + * check if the short name resolves to a valid ref, > + * but use only rules prior to the matched one > + */ > + for (j = 0; j < i; j++) { > + const char *rule = ref_rev_parse_rules[j]; > + unsigned char short_objectname[20]; > + char refname[PATH_MAX]; > + > + /* > + * the short name is ambiguous, if it resolves > + * (with this previous rule) to a valid ref > + * read_ref() returns 0 on success > + */ > + mksnpath(refname, sizeof(refname), > + rule, short_name_len, short_name); > + if (!read_ref(refname, short_objectname)) > + break; > + } > + > + /* > + * short name is non-ambiguous if all previous rules > + * haven't resolved to a valid ref > + */ > + if (j == i) > + return short_name; > + } > + > + free(short_name); > + return xstrdup(ref); > +} > diff --git a/refs.h b/refs.h > index 68c2d16..2d0f961 100644 > --- a/refs.h > +++ b/refs.h > @@ -80,6 +80,7 @@ extern int for_each_reflog(each_ref_fn, void *); > extern int check_ref_format(const char *target); > > extern const char *prettify_ref(const struct ref *ref); > +extern char *shorten_unambiguous_ref(const char *ref); > > /** rename ref, return 0 on success **/ > extern int rename_ref(const char *oldref, const char *newref, const char *logmsg); ``` ## Michael J Gruber, 2009-04-07 08:02 Subject: Re: [PATCH 5/5] branch: show upstream branch when double verbose Message-ID: <49DB089A.7080207@drmicha.warpmail.net> URL: https://gitlist.dev/e/49DB089A.7080207%40drmicha.warpmail.net In-Reply-To: <20090407071656.GE2924@coredump.intra.peff.net> ``` Jeff King venit, vidit, dixit 07.04.2009 09:16: > This information is easily accessible when we are > calculating the relationship. The only reason not to print > it all the time is that it consumes a fair bit of screen > space, and may not be of interest to the user. > > Signed-off-by: Jeff King > --- > This one is very RFC. Should this information be part of the regular > "-v"? Should it be part of "git branch" with regular verbosity? > > Should the format be different? I wonder if > > master 1234abcd [origin/master: ahead 5, behind 6] whatever > > will be interpreted as "origin/master is ahead 5, behind 6" when it is > really the reverse. Maybe "[ahead 5, behind 6 from origin/master]" would > be better? Maybe [origin/master +5 -6]? That should be short enough for sticking it into -v. We could even use [origin/master +0 -0] for an up-to-date branch then. In any case, I think often one is interested in one branch only. I would expect "git branch -v foo" to give me the -v info just for branch foo. Currently it does not. But that would be an independent patch on top. > Documentation/git-branch.txt | 4 +++- > builtin-branch.c | 23 +++++++++++++++++------ > 2 files changed, 20 insertions(+), 7 deletions(-) > > diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt > index 31ba7f2..ba3dea6 100644 > --- a/Documentation/git-branch.txt > +++ b/Documentation/git-branch.txt > @@ -100,7 +100,9 @@ OPTIONS > > -v:: > --verbose:: > - Show sha1 and commit subject line for each head. > + Show sha1 and commit subject line for each head, along with > + relationship to upstream branch (if any). If given twice, print > + the name of the upstream branch, as well. > > --abbrev=:: > Alter the sha1's minimum display length in the output listing. > diff --git a/builtin-branch.c b/builtin-branch.c > index ca81d72..3275821 100644 > --- a/builtin-branch.c > +++ b/builtin-branch.c > @@ -301,19 +301,30 @@ static int ref_cmp(const void *r1, const void *r2) > return strcmp(c1->name, c2->name); > } > > -static void fill_tracking_info(struct strbuf *stat, const char *branch_name) > +static void fill_tracking_info(struct strbuf *stat, const char *branch_name, > + int show_upstream_ref) > { > int ours, theirs; > struct branch *branch = branch_get(branch_name); > > - if (!stat_tracking_info(branch, &ours, &theirs) || (!ours && !theirs)) > + if (!stat_tracking_info(branch, &ours, &theirs)) { > + if (branch && branch->merge && branch->merge[0]->dst && > + show_upstream_ref) > + strbuf_addf(stat, "[%s] ", > + shorten_unambiguous_ref(branch->merge[0]->dst)); > return; > + } > + > + strbuf_addch(stat, '['); > + if (show_upstream_ref) > + strbuf_addf(stat, "%s: ", > + shorten_unambiguous_ref(branch->merge[0]->dst)); > if (!ours) > - strbuf_addf(stat, "[behind %d] ", theirs); > + strbuf_addf(stat, "behind %d] ", theirs); > else if (!theirs) > - strbuf_addf(stat, "[ahead %d] ", ours); > + strbuf_addf(stat, "ahead %d] ", ours); > else > - strbuf_addf(stat, "[ahead %d, behind %d] ", ours, theirs); > + strbuf_addf(stat, "ahead %d, behind %d] ", ours, theirs); > } > > static int matches_merge_filter(struct commit *commit) > @@ -379,7 +390,7 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose, > } > > if (item->kind == REF_LOCAL_BRANCH) > - fill_tracking_info(&stat, item->name); > + fill_tracking_info(&stat, item->name, verbose > 1); > > strbuf_addf(&out, " %s %s%s", > find_unique_abbrev(item->commit->object.sha1, abbrev), ``` ## Paolo Ciarrocchi, 2009-04-07 08:12 Subject: Re: [PATCH 5/5] branch: show upstream branch when double verbose Message-ID: <4d8e3fd30904070112w72ada661p99525aaa9437f8ff@mail.gmail.com> URL: https://gitlist.dev/e/4d8e3fd30904070112w72ada661p99525aaa9437f8ff%40mail.gmail.com In-Reply-To: <20090407071656.GE2924@coredump.intra.peff.net> ``` On Tue, Apr 7, 2009 at 9:16 AM, Jeff King wrote: > This information is easily accessible when we are > calculating the relationship. The only reason not to print > it all the time is that it consumes a fair bit of screen > space, and may not be of interest to the user. > > Signed-off-by: Jeff King > --- > This one is very RFC. Should this information be part of the regular > "-v"? Should it be part of "git branch" with regular verbosity? > > Should the format be different? I wonder if > >  master 1234abcd [origin/master: ahead 5, behind 6] whatever > > will be interpreted as "origin/master is ahead 5, behind 6" when it is > really the reverse. Maybe "[ahead 5, behind 6 from origin/master]" would > be better? Yes I think so. Thanks a lot for your work! -Paolo ``` ## Jeff King, 2009-04-07 21:41 Subject: Re: [PATCH] for-each-ref: remove multiple xstrdup() in get_short_ref() Message-ID: <20090407214144.GA17962@coredump.intra.peff.net> URL: https://gitlist.dev/e/20090407214144.GA17962%40coredump.intra.peff.net In-Reply-To: <20090407074435.GB7327@coredump.intra.peff.net> ``` On Tue, Apr 07, 2009 at 03:44:35AM -0400, Jeff King wrote: > + if (1 != sscanf(ref, scanf_fmts[i], short_name)) > [...] > + if (j == i) { > + ref += strlen(ref) - strlen(short_name); > + break; > + } Actually, I am not sure this is correct, either. It is making the assumption that the short_name is always a suffix of the ref. But one of the rev_parse_rules is: "refs/remotes/%.*s/HEAD" for which this is not true. _But_ as it happens, this rule doesn't actually work in reverse because of the way scanf works with %s. The code: sscanf("refs/remotes/origin/HEAD", "refs/remotes/%s/HEAD", buf); will put "origin/HEAD" in buf, not "origin"; %s eats until it sees whitespace (which should not be occuring in a ref, fortunately). So it actually _is_ correct to assume with the current code that the short_name is always a suffix, but I am not sure if that is what we actually want. We will always see "$remote/HEAD" instead of "$remote". Part of me actually thinks the "incorrect" behavior we are doing now is actually more explicit and readable. But if that is the case, we should perhaps simply be excluding that final rule explicitly, and then our "suffix" assumption will hold. -Peff ``` ## Junio C Hamano, 2009-04-08 06:22 Subject: Re: [PATCH 2/5] for-each-ref: refactor refname handling Message-ID: <7vprfnr7es.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vprfnr7es.fsf%40gitster.siamese.dyndns.org In-Reply-To: <20090407070651.GB2924@coredump.intra.peff.net> ``` Jeff King writes: > This code handles some special magic like *-deref and the > :short formatting specifier. The next patch will add another > field which outputs a ref and wants to use the same code. > > This patch splits the "which ref are we outputting" from the > actual formatting. There should be no behavioral change. > > Signed-off-by: Jeff King > --- > The diff is scary, but it is mostly reindentation. ... and an introduction of a bug ;-) > builtin-for-each-ref.c | 47 ++++++++++++++++++++++++++--------------------- > 1 files changed, 26 insertions(+), 21 deletions(-) > > diff --git a/builtin-for-each-ref.c b/builtin-for-each-ref.c > index 4aaf75c..b50c93b 100644 > --- a/builtin-for-each-ref.c > +++ b/builtin-for-each-ref.c > @@ -672,32 +672,37 @@ static void populate_value(struct refinfo *ref) > ... > + /* look for "short" refname format */ > + if (formatp) { > + formatp++; > + if (!strcmp(formatp, "short")) > + refname = get_short_ref(refname); > + else > + die("unknown %.*s format %s", > + formatp - name, name, formatp); die("unknown %.*s format %s", (int)(formatp - name), name, formatp); ``` ## Jeff King, 2009-04-08 06:27 Subject: Re: [PATCH 2/5] for-each-ref: refactor refname handling Message-ID: <20090408062717.GA26929@coredump.intra.peff.net> URL: https://gitlist.dev/e/20090408062717.GA26929%40coredump.intra.peff.net In-Reply-To: <7vprfnr7es.fsf@gitster.siamese.dyndns.org> ``` On Tue, Apr 07, 2009 at 11:22:51PM -0700, Junio C Hamano wrote: > > The diff is scary, but it is mostly reindentation. > ... and an introduction of a bug ;-) Oops. > > + die("unknown %.*s format %s", > > + formatp - name, name, formatp); > die("unknown %.*s format %s", > (int)(formatp - name), name, formatp); Hey, it's all 32 bits, right? ;) Thanks for spotting it. -Peff ``` ## Jeff King, 2009-04-09 08:18 Subject: Re: [PATCH 4/5] make get_short_ref a public function Message-ID: <20090409081857.GC17221@coredump.intra.peff.net> URL: https://gitlist.dev/e/20090409081857.GC17221%40coredump.intra.peff.net In-Reply-To: <36ca99e90904070039m15869c34jc9e12d5ccc48d82@mail.gmail.com> ``` On Tue, Apr 07, 2009 at 09:39:58AM +0200, Bert Wesarg wrote: > > Actually, I am not quite sure that this function is "more correct". It > > looks at the rev-parsing rules as a hierarchy, so if you have > > "refs/remotes/foo" and "refs/heads/foo", then it will abbreviate the > > first to "remotes/foo" (as expected) and the latter to just "foo". > > > > This is technically correct, as "refs/heads/foo" will be selected by > > "foo", but it will warn about ambiguity. Should we actually try to avoid > > reporting refs which would be ambiguous? > Back than, there was the idea that the core.warnAmbiguousRefs config > could be used for this. I'm not quite sure what you mean. Using this function, we may shorten an unambiguous name to one that will complain if core.warnAmbiguousRefs is set. So what I'm wondering is if it should use a different algorithm that produces a shortened ref which will not cause a warning. E.g., right now if we have: refs/heads/master refs/remotes/master showing %(refname:short) gets you: master remotes/master but "git show master" will warn about the ambiguous ref (but still show you the one you want). An alternative would be to show: heads/master remotes/master in this case. -Peff ``` ## Jeff King, 2009-04-09 08:23 Subject: Re: [PATCH 5/5] branch: show upstream branch when double verbose Message-ID: <20090409082350.GD17221@coredump.intra.peff.net> URL: https://gitlist.dev/e/20090409082350.GD17221%40coredump.intra.peff.net In-Reply-To: <49DB089A.7080207@drmicha.warpmail.net> ``` On Tue, Apr 07, 2009 at 10:02:34AM +0200, Michael J Gruber wrote: > > will be interpreted as "origin/master is ahead 5, behind 6" when it is > > really the reverse. Maybe "[ahead 5, behind 6 from origin/master]" would > > be better? > > Maybe [origin/master +5 -6]? That should be short enough for sticking it > into -v. We could even use [origin/master +0 -0] for an up-to-date > branch then. I am not opposed to that format, but I don't feel strongly. And not many people are voicing an opinion in this thread (strange, given that it is an opportunity for bikeshedding :) ). My patches are in next, so I think I am done on the topic for now. But feel free to submit a followup patch. > In any case, I think often one is interested in one branch only. I would > expect "git branch -v foo" to give me the -v info just for branch foo. > Currently it does not. But that would be an independent patch on top. Hmm. I think that is a little counterintuitive because we think of "-v" as simply "increase verbosity" (because that is what it means in just about every program). But here you are fundamentally changing the action that is occurring (from "create branch" to "show branch"). I think it would make more sense to have a "show branch" mode like: git branch -s foo which would probably have the more detailed output by default. -Peff ``` ## Bert Wesarg, 2009-04-09 09:05 Subject: Re: [PATCH 4/5] make get_short_ref a public function Message-ID: <36ca99e90904090205g8a6a5a6nea96f8c5f44e076a@mail.gmail.com> URL: https://gitlist.dev/e/36ca99e90904090205g8a6a5a6nea96f8c5f44e076a%40mail.gmail.com In-Reply-To: <20090409081857.GC17221@coredump.intra.peff.net> ``` On Thu, Apr 9, 2009 at 10:18, Jeff King wrote: > On Tue, Apr 07, 2009 at 09:39:58AM +0200, Bert Wesarg wrote: > >> > Actually, I am not quite sure that this function is "more correct". It >> > looks at the rev-parsing rules as a hierarchy, so if you have >> > "refs/remotes/foo" and "refs/heads/foo", then it will abbreviate the >> > first to "remotes/foo" (as expected) and the latter to just "foo". >> > >> > This is technically correct, as "refs/heads/foo" will be selected by >> > "foo", but it will warn about ambiguity. Should we actually try to avoid >> > reporting refs which would be ambiguous? >> Back than, there was the idea that the core.warnAmbiguousRefs config >> could be used for this. > > I'm not quite sure what you mean. Using this function, we may shorten an > unambiguous name to one that will complain if core.warnAmbiguousRefs is > set. So what I'm wondering is if it should use a different algorithm > that produces a shortened ref which will not cause a warning. > > E.g., right now if we have: > >  refs/heads/master >  refs/remotes/master > > showing %(refname:short) gets you: > >  master >  remotes/master > > but "git show master" will warn about the ambiguous ref (but still show > you the one you want). An alternative would be to show: > >  heads/master >  remotes/master > > in this case. Right, and the idea was to choose the alternatives based on the core.warnAmbiguousRefs setting, i.e. the former for false, the latter for true. For what I posted a patch some time ago: http://thread.gmane.org/gmane.comp.version-control.git/96464 (which I read though now) Bert > > -Peff > ``` ## Santi Béjar, 2009-04-09 10:15 Subject: Re: [PATCH 5/5] branch: show upstream branch when double verbose Message-ID: URL: https://gitlist.dev/e/adf1fd3d0904090315x10b8c481g311832c40c450c47%40mail.gmail.com In-Reply-To: <20090409082350.GD17221@coredump.intra.peff.net> ``` 2009/4/9 Jeff King : > On Tue, Apr 07, 2009 at 10:02:34AM +0200, Michael J Gruber wrote: > >> > will be interpreted as "origin/master is ahead 5, behind 6" when it is >> > really the reverse. Maybe "[ahead 5, behind 6 from origin/master]" would >> > be better? >> >> Maybe [origin/master +5 -6]? That should be short enough for sticking it >> into -v. We could even use [origin/master +0 -0] for an up-to-date >> branch then. > > I am not opposed to that format, but I don't feel strongly. And not many > people are voicing an opinion in this thread (strange, given that it is > an opportunity for bikeshedding :) ). I've been thinking about this and both formats seems OK for me, although using the +5 -6 format for just -v seems a good point. Just to bikeshed a bit more :) we could use a format more similar to the "git fetch" output, like: next c4628f8 [4...6 origin/next] Merge branch 'jk/no-perl' into next next c4628f8 [4.. origin/next] Merge branch 'jk/no-perl' into next next c4628f8 [..6 origin/next] Merge branch 'jk/no-perl' into next (three dots when they have diverged and two otherwise) It can suggest that the left number you know it is about things in next and the right number about things in origin/next. The problem is that is also looks like revisions. Just my 2cents, but feel free to ignore ;-) Santi ``` ## Jeff King, 2009-04-13 08:15 Subject: Re: [PATCH 4/5] make get_short_ref a public function Message-ID: <20090413081543.GA9846@coredump.intra.peff.net> URL: https://gitlist.dev/e/20090413081543.GA9846%40coredump.intra.peff.net In-Reply-To: <36ca99e90904090205g8a6a5a6nea96f8c5f44e076a@mail.gmail.com> ``` On Thu, Apr 09, 2009 at 11:05:06AM +0200, Bert Wesarg wrote: > > you the one you want). An alternative would be to show: > > > >  heads/master > >  remotes/master > > > > in this case. > Right, and the idea was to choose the alternatives based on the > core.warnAmbiguousRefs setting, i.e. the former for false, the latter > for true. > > For what I posted a patch some time ago: > > http://thread.gmane.org/gmane.comp.version-control.git/96464 Ah, OK, now I understand what you meant. I think that is the right solution. Thanks. -Peff ``` ## Jeff King, 2009-04-13 08:34 Subject: Re: [PATCH 5/5] branch: show upstream branch when double verbose Message-ID: <20090413083413.GB9846@coredump.intra.peff.net> URL: https://gitlist.dev/e/20090413083413.GB9846%40coredump.intra.peff.net In-Reply-To: ``` On Thu, Apr 09, 2009 at 12:15:08PM +0200, Santi Béjar wrote: > I've been thinking about this and both formats seems OK for me, > although using the +5 -6 format for just -v seems a good point. The trivial patch for this is below: --- diff --git a/builtin-branch.c b/builtin-branch.c index 3275821..c056a4d 100644 --- a/builtin-branch.c +++ b/builtin-branch.c @@ -317,14 +317,14 @@ static void fill_tracking_info(struct strbuf *stat, const char *branch_name, strbuf_addch(stat, '['); if (show_upstream_ref) - strbuf_addf(stat, "%s: ", + strbuf_addf(stat, "%s ", shorten_unambiguous_ref(branch->merge[0]->dst)); if (!ours) - strbuf_addf(stat, "behind %d] ", theirs); + strbuf_addf(stat, "-%d] ", theirs); else if (!theirs) - strbuf_addf(stat, "ahead %d] ", ours); + strbuf_addf(stat, "+%d] ", ours); else - strbuf_addf(stat, "ahead %d, behind %d] ", ours, theirs); + strbuf_addf(stat, "+%d -%d] ", ours, theirs); } static int matches_merge_filter(struct commit *commit) I actually think it looks a bit ugly without the upstream name, as the short bit in brackets blends in with the commit subject: next 1412037 [+2] shorter tracking format for branch -v whereas I think this is better: next 1412037 [origin/next +2] shorter tracking format for branch -v but I am still lukewarm on the concept personally (i.e., I am not opposed, but I am not taking it further than the "how about this" patch below, so if somebody thinks it is a good idea they should speak up and advocate for it). > Just to bikeshed a bit more :) we could use a format more similar to > the "git fetch" output, like: > > next c4628f8 [4...6 origin/next] Merge branch 'jk/no-perl' into next > next c4628f8 [4.. origin/next] Merge branch 'jk/no-perl' into next > next c4628f8 [..6 origin/next] Merge branch 'jk/no-perl' into next Personally, I find that pretty ugly, and a bit confusing. The ".." notation would make more sense if there were actual revisions on the end and not numbers. But then of course you have to show the numbers separately. Something like this makes at least has some logic to it: origin/next...next: 4,6 origin/next..next: 4 next..origin/next: 6 but it looks horribly ugly and is way more confusing than it needs to be. -Peff ``` ## Wincent Colaiuta, 2009-04-13 17:04 Subject: Re: [PATCH 5/5] branch: show upstream branch when double verbose Message-ID: <18CF07A6-68CB-4A17-ACC9-89CD8545F58B@wincent.com> URL: https://gitlist.dev/e/18CF07A6-68CB-4A17-ACC9-89CD8545F58B%40wincent.com In-Reply-To: <20090413083413.GB9846@coredump.intra.peff.net> ``` El 13/4/2009, a las 10:34, Jeff King escribió: > On Thu, Apr 09, 2009 at 12:15:08PM +0200, Santi Béjar wrote: > >> I've been thinking about this and both formats seems OK for me, >> although using the +5 -6 format for just -v seems a good point. > > The trivial patch for this is below: > > --- > diff --git a/builtin-branch.c b/builtin-branch.c > index 3275821..c056a4d 100644 > --- a/builtin-branch.c > +++ b/builtin-branch.c > @@ -317,14 +317,14 @@ static void fill_tracking_info(struct strbuf > *stat, const char *branch_name, > > strbuf_addch(stat, '['); > if (show_upstream_ref) > - strbuf_addf(stat, "%s: ", > + strbuf_addf(stat, "%s ", > shorten_unambiguous_ref(branch->merge[0]->dst)); > if (!ours) > - strbuf_addf(stat, "behind %d] ", theirs); > + strbuf_addf(stat, "-%d] ", theirs); > else if (!theirs) > - strbuf_addf(stat, "ahead %d] ", ours); > + strbuf_addf(stat, "+%d] ", ours); > else > - strbuf_addf(stat, "ahead %d, behind %d] ", ours, theirs); > + strbuf_addf(stat, "+%d -%d] ", ours, theirs); > } > > static int matches_merge_filter(struct commit *commit) I'm skeptical that people will know at a glance that "-" and "+" in this context map onto "behind" and "ahead". I think that trading off the clarity and obviousness of the words "behind" and "ahead" for the brevity of the symbols "-" and "+" wouldn't be a very good idea. Cheers, Wincent ```