On Sun, Aug 30, 2015 at 1:23 PM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
> On Sat, Aug 29, 2015 at 10:12 AM, Karthik Nayak <karthik.188@xxxxxxxxx> wrote:
>> In 'tag.c' we can print N lines from the annotation of the tag using
>> the '-n<num>' option. Copy code from 'tag.c' to 'ref-filter' and
>> modify it to support appending of N lines from the annotation of tags
>> to the given strbuf.
>>
>> Implement %(contents:lines=X) where X lines of the given object are
>> obtained.
>>
>> Add documentation and test for the same.
>>
>> Signed-off-by: Karthik Nayak <karthik.188@xxxxxxxxx>
>> ---
>> diff --git a/builtin/tag.c b/builtin/tag.c
>> index 471d6b1..0fc7557 100644
>> --- a/builtin/tag.c
>> +++ b/builtin/tag.c
>> @@ -63,6 +64,11 @@ struct align {
>> unsigned int width;
>> };
>>
>> +struct contents {
>> + unsigned int lines;
>> + struct object_id oid;
>> +};
>> +
>> #define REF_FORMATTING_STATE_INIT { 0, NULL }
>>
>> struct ref_formatting_stack {
>> @@ -80,6 +86,7 @@ struct ref_formatting_state {
>> struct atom_value {
>> const char *s;
>> struct align *align;
>> + struct contents *contents;
>
> Same question as for 'align': Does 'contents' need to be
> heap-allocated because it must exist beyond the lifetime of
> 'atom_value'? If not, making it just a plain member of 'atom_value'
> would simplify memory management (no need to free it).
>
In this context that makes sense, as the value is only needed for the
current atom value.
> Also, will 'align' and 'contents' ever be used at the same time? If
> not, it might make sense to place them in a 'union' (not for the
> memory saving, but to make it clear to the reader that their use is
> mutually exclusive).
>
Not quite sure if it needs to be mutually exclusive (isn't that up to the user?)
But this can be done, as they're separate atoms and at a time only one of them
is used.
> More below.
>
>> void (*handler)(struct atom_value *atomv, struct ref_formatting_state *state);
>> unsigned long ul; /* used for sorting when not FIELD_STR */
>> };
>> @@ -569,6 +576,61 @@ static void find_subpos(const char *buf, unsigned long sz,
>> *nonsiglen = *sig - buf;
>> }
>>
>> +/*
>> + * If 'lines' is greater than 0, append that many lines from the given
>> + * object_id 'oid' to the given strbuf.
>> + */
>> +static void append_tag_lines(struct strbuf *out, const struct object_id *oid, int lines)
>> +{
>> + int i;
>> + unsigned long size;
>> + enum object_type type;
>> + char *buf, *sp, *eol;
>> + size_t len;
>> +
>> + buf = read_sha1_file(oid->hash, &type, &size);
>> + if (!buf)
>> + die_errno("unable to read object %s", oid_to_hex(oid));
>> + if (type != OBJ_COMMIT && type != OBJ_TAG)
>> + goto free_return;
>> + if (!size)
>> + die("an empty %s object %s?",
>> + typename(type), oid_to_hex(oid));
>> +
>> + /* skip header */
>> + sp = strstr(buf, "\n\n");
>> + if (!sp)
>> + goto free_return;
>> +
>> + /* only take up to "lines" lines, and strip the signature from a tag */
>> + if (type == OBJ_TAG)
>> + size = parse_signature(buf, size);
>> + for (i = 0, sp += 2; i < lines && sp < buf + size; i++) {
>> + if (i)
>> + strbuf_addstr(out, "\n ");
>> + eol = memchr(sp, '\n', size - (sp - buf));
>> + len = eol ? eol - sp : size - (sp - buf);
>> + strbuf_add(out, sp, len);
>> + if (!eol)
>> + break;
>> + sp = eol + 1;
>> + }
>> +free_return:
>> + free(buf);
>> +}
>
> I understand that you want to re-use this code from
> tag.c:show_tag_lines(), but (from a very cursory read) isn't this
> duplicating logic and processing already done elsewhere in
> ref-filter.c? More about this below.
>
Replied below.
>> +
>> +static void contents_lines_handler(struct atom_value *atomv, struct ref_formatting_state *state)
>> +{
>> + struct contents *contents = (struct contents *)atomv->contents;
>
> Why is this cast needed?
>
Will remove.
>> + struct strbuf s = STRBUF_INIT;
>> +
>> + append_tag_lines(&s, &contents->oid, contents->lines);
>> + quote_formatting(&state->stack->output, s.buf, state->quote_style);
>> + strbuf_release(&s);
>> +
>> + free(contents);
>> +}
>> @@ -588,7 +651,8 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj
>> strcmp(name, "contents") &&
>> strcmp(name, "contents:subject") &&
>> strcmp(name, "contents:body") &&
>> - strcmp(name, "contents:signature"))
>> + strcmp(name, "contents:signature") &&
>> + !starts_with(name, "contents:lines="))
>> continue;
>> if (!subpos)
>> find_subpos(buf, sz,
>> @@ -608,6 +672,15 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj
>> v->s = xmemdupz(sigpos, siglen);
>> else if (!strcmp(name, "contents"))
>> v->s = xstrdup(subpos);
>> + else if (skip_prefix(name, "contents:lines=", &valp)) {
>> + struct contents *contents = xmalloc(sizeof(struct contents));
>> +
>> + if (strtoul_ui(valp, 10, &contents->lines))
>> + die(_("positive width expected align:%s"), valp);
>> + hashcpy(contents->oid.hash, obj->sha1);
>
> The logic in append_tag_lines() which was copied from
> tag.c:show_tag_lines() goes through the effort of loading the object
> and parsing it, but hasn't the object already been loaded and parsed
> by the time you get to this point? Assuming I'm reading this
> correctly, wouldn't it make more sense to take advantage of the work
> already done loading and parsing the object rather than repeating it
> all inside append_tag_lines()?
>
Makes sense, I'll work on this.
>> + v->handler = contents_lines_handler;
>> + v->contents = contents;
>> + }
>> }
>> }
>> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
>> index cef7a41..0277498 100755
>> --- a/t/t6302-for-each-ref-filter.sh
>> +++ b/t/t6302-for-each-ref-filter.sh
>> @@ -166,4 +166,20 @@ test_expect_success 'nested alignment' '
>> test_cmp expect actual
>> '
>>
>> +test_expect_success 'check `%(contents:lines=X)`' '
>> + cat >expect <<-\EOF &&
>> + master three
>> + side four
>> + odd/spot three
>> + double-tag Annonated doubly
>> + four four
>> + one one
>> + signed-tag A signed tag message
>> + three three
>> + two two
>> + EOF
>> + git for-each-ref --format="%(refname:short) %(contents:lines=1)" >actual &&
>
> Maybe also test some edge cases, such as line=0, lines=-1 (an invalid
> value), lines=2, lines=9999999 (a value larger than the number of
> lines in any object).
>
Will do. Thanks for the review.
--
Regards,
Karthik Nayak
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html