Eric Sunshine wrote:
> 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.
>> 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.
>> + 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.
>> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
