Re: [PATCH v3 4/6] for-each-ref: introduce %(upstream:track[short])
- From
Ramkumar Ramachandra <artagnon@gmail.com>
- Date
- Nov 17, 2013, 05:58 UTC
- Message-ID
- <CALkWK0=3=a5hiU7zxDAk=077CePkmN+MaLZFcuYdeZJM8QXApw@mail.gmail.com>
- In-Reply-To
- <CAPig+cRqwOgE8by8DwuGc1nNA3C_bA07ob-GuRzETKoRPBe+CQ@mail.gmail.com>
Eric Sunshine wrote:
Show 5 quoted lines
> The "prompt" is not mentioned elsewhere in for-each-ref documentation, > and a person not familiar with contrib/completion/ may be confused by > this reference. It might make sense instead to explain the meanings of > ">", "<", "<>", and "=" directly since they are not necessarily > obvious to the casual reader.
Someone else raised the same point earlier. Okay, I'll elaborate.
Show 36 quoted lines
>> diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
>> index 5f1842f..ed81407 100644
>> --- a/builtin/for-each-ref.c
>> +++ b/builtin/for-each-ref.c
>> @@ -689,13 +690,46 @@ static void populate_value(struct refinfo *ref)
>> continue;
>>
>> formatp = strchr(name, ':');
>> - /* look for "short" refname format */
>> if (formatp) {
>> + int num_ours, num_theirs;
>> +
>> formatp++;
>> if (!strcmp(formatp, "short"))
>> refname = shorten_unambiguous_ref(refname,
>> warn_ambiguous_refs);
>> - else
>> + else if (!strcmp(formatp, "track") &&
>> + !prefixcmp(name, "upstream")) {
>> + char buf[40];
>> +
>> + stat_tracking_info(branch, &num_ours, &num_theirs);
>> + if (!num_ours && !num_theirs)
>> + v->s = "";
>> + else if (!num_ours) {
>> + sprintf(buf, "[behind %d]", num_theirs);
>> + v->s = xstrdup(buf);
>> + } else if (!num_theirs) {
>> + sprintf(buf, "[ahead %d]", num_ours);
>> + v->s = xstrdup(buf);
>> + } else {
>> + sprintf(buf, "[ahead %d, behind %d]",
>
> Is the intention that these strings ("[ahead %d]", etc.) will be
> internationalized in the future? If so, the allocated 40-character
> buffer may be insufficient.Similar strings in wt-status.c aren't ready for internationalization either. Besides, these xstrdup() calls leak memory and are quite suboptimal: if I allocate more memory, I'm going to be leaking more; so, I'd defer the internationalization discussion until we restructure this.
Show 23 quoted lines
>> + num_ours, num_theirs);
>> + v->s = xstrdup(buf);
>> + }
>> + continue;
>> + } else if (!strcmp(formatp, "trackshort") &&
>> + !prefixcmp(name, "upstream")) {
>> +
>> + stat_tracking_info(branch, &num_ours, &num_theirs);
>> + if (!num_ours && !num_theirs)
>> + v->s = "=";
>> + else if (!num_ours)
>> + v->s = "<";
>> + else if (!num_theirs)
>> + v->s = ">";
>> + else
>> + v->s = "<>";
>> + continue;
>> + } else
>> die("unknown %.*s format %s",
>> (int)(formatp - name), name, formatp);
>
> Is it still accurate to call this a "format" in the error message?
> 'track' and 'trackshort' seem more like decorations.By convention, all f-e-r formatting errors are reported as fatal errors (see color errors too, for instance), unlike pretty-formats. We might want to change this in a later series.
Show 37 quoted lines
>> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh >> index 5e29ffc..9d874fd 100755 >> --- a/t/t6300-for-each-ref.sh >> +++ b/t/t6300-for-each-ref.sh >> @@ -303,6 +303,28 @@ test_expect_success 'Check short upstream format' ' >> test_cmp expected actual >> ' >> >> +test_expect_success 'setup for upstream:track[short]' ' >> + test_commit two >> +' >> + >> +cat >expected <<EOF >> +[ahead 1] >> +EOF >> + >> +test_expect_success 'Check upstream:track format' ' >> + git for-each-ref --format="%(upstream:track)" refs/heads >actual && >> + test_cmp expected actual >> +' >> + >> +cat >expected <<EOF >> +> >> +EOF >> + >> +test_expect_success 'Check upstream:trackshort format' ' >> + git for-each-ref --format="%(upstream:trackshort)" refs/heads >actual && >> + test_cmp expected actual >> +' >> + >> cat >expected <<EOF >> $(git rev-parse --short HEAD) >> EOF > > Would it make sense also to add tests verifying that :track and > :trackshort correctly fail when applied to a key other than > "upstream"?
Sure.
And thanks for the detailed review.