Re: [PATCH v13 00/12] port tag.c to use ref-filter APIs
On Thu, Aug 27, 2015 at 1:49 AM, Junio C Hamano gits...@pobox.com wrote:
Karthik Nayak karthik@gmail.com writes:
On Wed, Aug 26, 2015 at 8:07 PM, Junio C Hamano gits...@pobox.com wrote:
... You can give a new format_ref_array_item()
that does not print but fills a strbuf to this caller, make
show_ref_array_item() a thin wrapper that calls it and prints it
with the final LF for other callers.
You're saying remove show_ref_array_item() (even the wrapper you mentioned
above) and just have something like format_ref_array_item() which
would output to a strbuf. and let the caller worry about the printing?
Among the current callers, the one in builtin/tag.c that wants to
trigger show_tag_lines() hack embedded in show_ref_array_item()
function can stop calling show_ref_array_item() and instead can do
for (i = 0; i array.nr; i++) {
struct strbuf out = STRBUF_INIT;
format_ref_array_item(out, ...);
if (filter-lines) {
... append tag lines to out ...
}
printf(%s\n, out.buf);
strbuf_reset(out);
}
The current and future callers of show_ref_array_item() that do not
want to trigger the show_tag_liens() hack embedded in there may
still want it to print the formatted string including the trailing
LF, so you can keep show_ref_array_item() as a thin wrapper around
format_ref_array_item() for them to call, e.g.
show_ref_array_item(...) {
struct strbuf out = STRBUF_INIT;
format_ref_array_item(out, ...);
printf(%s\n, out.buf);
strbuf_release(out);
}
But if it has only one caller each, you may not even want to have
show_ref_array_item(), if you are going to do the output to strbuf
variant.
This is exactly what I did at the moment, I'm also trying to get
%(contents:lines=N)
work. Thanks for explaining though.
--
Regards,
Karthik Nayak
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v13 00/12] port tag.c to use ref-filter APIs
On Wed, Aug 26, 2015 at 9:18 PM, Matthieu Moy matthieu@grenoble-inp.fr wrote: Junio C Hamano gits...@pobox.com writes: Junio C Hamano gits...@pobox.com writes: Matthieu Moy matthieu@grenoble-inp.fr writes: For the current code %(if:empty)%(align)%(end)%(then)Empty%(else)Not Empty%(end) would print non-empty, I guess the documentation holds in that case. Not sure if we require it to print non-empty. You don't want the %(if) condition to depend on whether --shell/--python/... is used. Since %(if:empty)%(align)%(end)%(then) holds when you don't use --shell, you also want it to hold when you quote. IOW, you should check for emptyness before (or actually without) doing the quoting. I guess this is what you're doing, and if so, I think it's The Right Thing. I agree that %(align)%(end) should expand to empty and %(if:empty)...%(then) should look at that empty string without quoting. So %(if:empty)%(align)%(end)%(then)Empty%(else)Not Empty%(end) should give Empty; otherwise the code is buggy, I think. (I shouldn't be typing while eating...) It should give Empty, but the --shell/--python/... may make the whole Empty, as the result of %(if:...)...%(end), be quoted. So you may see 'Empty' in the output. Agreed (with both points). Thanks, will work on this. -- Regards, Karthik Nayak -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v13 00/12] port tag.c to use ref-filter APIs
On Wed, Aug 26, 2015 at 8:07 PM, Junio C Hamano gits...@pobox.com wrote: Karthik Nayak karthik@gmail.com writes: On Wed, Aug 26, 2015 at 12:46 AM, Junio C Hamano gits...@pobox.com wrote: Karthik Nayak karthik@gmail.com writes: I didn't check how wide the original is supposed to be, but perhaps changing builtin/tag.c this way if (filter-lines) - format = %(align:16,left)%(refname:short)%(end); + format = %(align:15,left)%(refname:short)%(end) ; else format = %(refname:short); } may be one way to fix it. Check the original tag -l output for tags whose names are 14, 15, 16 and 17 display-columns wide and try to match it. That should be the fix, since it's a space problem. The problem with doing this is, the lines need to be displayed immediately after the refname, followed by a newline, what you're suggesting breaks that flow. That is only because show_ref_array_item() does too much. The function is given a placeholder string and a set of data to fill the placeholder with for an item, and the reason why the caller repeatedly calls it, once per item it has, is because it wants to produce a one-line-per-item output. An alternative valid way to structure the API is to have it format into a string and leave the printing to the caller. You can give a new format_ref_array_item() that does not print but fills a strbuf to this caller, make show_ref_array_item() a thin wrapper that calls it and prints it with the final LF for other callers. Another alternate approach, if you want to give tag -l annotation available to for-each-ref, would be to do this: if (filter-lines) format = xstrfmt(%%(align:15,left)%%(refname:short)%%(end) %%(contents:lines=%s), filter-lines); else format = %(refname:short); and teach a new %(contents:lines=1) atom. That way, you can lose the ugly special case call to show_tag_lines() that can only be at the end of the output. I somehow think this is a better approach. This seems like a good approach, since contents is already an atom, this would fit in easily. Of course you can (and probably would want to) do both, giving a bit lower level emit to a strbuf function to the callers _and_ losing hardcoded call to show_tag_lines(). You're saying remove show_ref_array_item() (even the wrapper you mentioned above) and just have something like format_ref_array_item() which would output to a strbuf. and let the caller worry about the printing? -- Regards, Karthik Nayak -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v13 00/12] port tag.c to use ref-filter APIs
Karthik Nayak karthik@gmail.com writes:
On Wed, Aug 26, 2015 at 8:07 PM, Junio C Hamano gits...@pobox.com wrote:
... You can give a new format_ref_array_item()
that does not print but fills a strbuf to this caller, make
show_ref_array_item() a thin wrapper that calls it and prints it
with the final LF for other callers.
You're saying remove show_ref_array_item() (even the wrapper you mentioned
above) and just have something like format_ref_array_item() which
would output to a strbuf. and let the caller worry about the printing?
Among the current callers, the one in builtin/tag.c that wants to
trigger show_tag_lines() hack embedded in show_ref_array_item()
function can stop calling show_ref_array_item() and instead can do
for (i = 0; i array.nr; i++) {
struct strbuf out = STRBUF_INIT;
format_ref_array_item(out, ...);
if (filter-lines) {
... append tag lines to out ...
}
printf(%s\n, out.buf);
strbuf_reset(out);
}
The current and future callers of show_ref_array_item() that do not
want to trigger the show_tag_liens() hack embedded in there may
still want it to print the formatted string including the trailing
LF, so you can keep show_ref_array_item() as a thin wrapper around
format_ref_array_item() for them to call, e.g.
show_ref_array_item(...) {
struct strbuf out = STRBUF_INIT;
format_ref_array_item(out, ...);
printf(%s\n, out.buf);
strbuf_release(out);
}
But if it has only one caller each, you may not even want to have
show_ref_array_item(), if you are going to do the output to strbuf
variant.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v13 00/12] port tag.c to use ref-filter APIs
Junio C Hamano gits...@pobox.com writes: Junio C Hamano gits...@pobox.com writes: Matthieu Moy matthieu@grenoble-inp.fr writes: For the current code %(if:empty)%(align)%(end)%(then)Empty%(else)Not Empty%(end) would print non-empty, I guess the documentation holds in that case. Not sure if we require it to print non-empty. You don't want the %(if) condition to depend on whether --shell/--python/... is used. Since %(if:empty)%(align)%(end)%(then) holds when you don't use --shell, you also want it to hold when you quote. IOW, you should check for emptyness before (or actually without) doing the quoting. I guess this is what you're doing, and if so, I think it's The Right Thing. I agree that %(align)%(end) should expand to empty and %(if:empty)...%(then) should look at that empty string without quoting. So %(if:empty)%(align)%(end)%(then)Empty%(else)Not Empty%(end) should give Empty; otherwise the code is buggy, I think. (I shouldn't be typing while eating...) It should give Empty, but the --shell/--python/... may make the whole Empty, as the result of %(if:...)...%(end), be quoted. So you may see 'Empty' in the output. Agreed (with both points). -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v13 00/12] port tag.c to use ref-filter APIs
Junio C Hamano gits...@pobox.com writes: Matthieu Moy matthieu@grenoble-inp.fr writes: For the current code %(if:empty)%(align)%(end)%(then)Empty%(else)Not Empty%(end) would print non-empty, I guess the documentation holds in that case. Not sure if we require it to print non-empty. You don't want the %(if) condition to depend on whether --shell/--python/... is used. Since %(if:empty)%(align)%(end)%(then) holds when you don't use --shell, you also want it to hold when you quote. IOW, you should check for emptyness before (or actually without) doing the quoting. I guess this is what you're doing, and if so, I think it's The Right Thing. I agree that %(align)%(end) should expand to empty and %(if:empty)...%(then) should look at that empty string without quoting. So %(if:empty)%(align)%(end)%(then)Empty%(else)Not Empty%(end) should give Empty; otherwise the code is buggy, I think. (I shouldn't be typing while eating...) It should give Empty, but the --shell/--python/... may make the whole Empty, as the result of %(if:...)...%(end), be quoted. So you may see 'Empty' in the output. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v13 00/12] port tag.c to use ref-filter APIs
Matthieu Moy matthieu@grenoble-inp.fr writes: For the current code %(if:empty)%(align)%(end)%(then)Empty%(else)Not Empty%(end) would print non-empty, I guess the documentation holds in that case. Not sure if we require it to print non-empty. You don't want the %(if) condition to depend on whether --shell/--python/... is used. Since %(if:empty)%(align)%(end)%(then) holds when you don't use --shell, you also want it to hold when you quote. IOW, you should check for emptyness before (or actually without) doing the quoting. I guess this is what you're doing, and if so, I think it's The Right Thing. I agree that %(align)%(end) should expand to empty and %(if:empty)...%(then) should look at that empty string without quoting. So %(if:empty)%(align)%(end)%(then)Empty%(else)Not Empty%(end) should give Empty; otherwise the code is buggy, I think. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v13 00/12] port tag.c to use ref-filter APIs
On Tue, Aug 25, 2015 at 4:05 AM, Junio C Hamano gits...@pobox.com wrote: Junio C Hamano gits...@pobox.com writes: Karthik Nayak karthik@gmail.com writes: On Mon, Aug 24, 2015 at 10:57 PM, Junio C Hamano gits...@pobox.com wrote: Karthik Nayak karthik@gmail.com writes: ... + performed. If used with '--quote' everything in between %(align:..) + and %(end) is quoted. ... I might have misunderstood you, But based on the discussion held here (thread.gmane.org/gmane.comp.version-control.git/276140) I thought we wanted everything inside the %(align) %(end) atoms to be quoted. Perhaps I misunderstood your intention in the doc. I took everything in between %(align:...) and %(end) is quoted to mean that %(if:empty)%(align)%(end)%(then)Empty%(else)Not Empty%(end) can never satisfy %(if:empty), because %(align)%(end) would expand to a string that has two single-quotes, that is not an empty string. If that is not what would happen in the branch --list enhancement, then the proposed behaviour is good, but the above documentation would need to be updated when it happens, I think. It at least is misleading. OK, now I checked the code, and I _think_ the recursive logic is doing the right thing (modulo minor nits on comment-vs-code discrepancy and code structure I sent separately). For the current code %(if:empty)%(align)%(end)%(then)Empty%(else)Not Empty%(end) would print non-empty, I guess the documentation holds in that case. Not sure if we require it to print non-empty. -- Regards, Karthik Nayak -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v13 00/12] port tag.c to use ref-filter APIs
Karthik Nayak karthik@gmail.com writes: On Tue, Aug 25, 2015 at 4:05 AM, Junio C Hamano gits...@pobox.com wrote: Junio C Hamano gits...@pobox.com writes: Karthik Nayak karthik@gmail.com writes: On Mon, Aug 24, 2015 at 10:57 PM, Junio C Hamano gits...@pobox.com wrote: Karthik Nayak karthik@gmail.com writes: ... + performed. If used with '--quote' everything in between %(align:..) + and %(end) is quoted. ... I might have misunderstood you, But based on the discussion held here (thread.gmane.org/gmane.comp.version-control.git/276140) I thought we wanted everything inside the %(align) %(end) atoms to be quoted. Perhaps I misunderstood your intention in the doc. I took everything in between %(align:...) and %(end) is quoted to mean that %(if:empty)%(align)%(end)%(then)Empty%(else)Not Empty%(end) can never satisfy %(if:empty), because %(align)%(end) would expand to a string that has two single-quotes, that is not an empty string. If that is not what would happen in the branch --list enhancement, then the proposed behaviour is good, but the above documentation would need to be updated when it happens, I think. It at least is misleading. OK, now I checked the code, and I _think_ the recursive logic is doing the right thing (modulo minor nits on comment-vs-code discrepancy and code structure I sent separately). For the current code %(if:empty)%(align)%(end)%(then)Empty%(else)Not Empty%(end) would print non-empty, I guess the documentation holds in that case. Not sure if we require it to print non-empty. You don't want the %(if) condition to depend on whether --shell/--python/... is used. Since %(if:empty)%(align)%(end)%(then) holds when you don't use --shell, you also want it to hold when you quote. IOW, you should check for emptyness before (or actually without) doing the quoting. I guess this is what you're doing, and if so, I think it's The Right Thing. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v13 00/12] port tag.c to use ref-filter APIs
Karthik Nayak karthik@gmail.com writes: On Wed, Aug 26, 2015 at 12:46 AM, Junio C Hamano gits...@pobox.com wrote: Karthik Nayak karthik@gmail.com writes: I didn't check how wide the original is supposed to be, but perhaps changing builtin/tag.c this way if (filter-lines) - format = %(align:16,left)%(refname:short)%(end); + format = %(align:15,left)%(refname:short)%(end) ; else format = %(refname:short); } may be one way to fix it. Check the original tag -l output for tags whose names are 14, 15, 16 and 17 display-columns wide and try to match it. That should be the fix, since it's a space problem. The problem with doing this is, the lines need to be displayed immediately after the refname, followed by a newline, what you're suggesting breaks that flow. That is only because show_ref_array_item() does too much. The function is given a placeholder string and a set of data to fill the placeholder with for an item, and the reason why the caller repeatedly calls it, once per item it has, is because it wants to produce a one-line-per-item output. An alternative valid way to structure the API is to have it format into a string and leave the printing to the caller. You can give a new format_ref_array_item() that does not print but fills a strbuf to this caller, make show_ref_array_item() a thin wrapper that calls it and prints it with the final LF for other callers. Another alternate approach, if you want to give tag -l annotation available to for-each-ref, would be to do this: if (filter-lines) format = xstrfmt(%%(align:15,left)%%(refname:short)%%(end) %%(contents:lines=%s), filter-lines); else format = %(refname:short); and teach a new %(contents:lines=1) atom. That way, you can lose the ugly special case call to show_tag_lines() that can only be at the end of the output. I somehow think this is a better approach. Of course you can (and probably would want to) do both, giving a bit lower level emit to a strbuf function to the callers _and_ losing hardcoded call to show_tag_lines(). -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v13 00/12] port tag.c to use ref-filter APIs
On Wed, Aug 26, 2015 at 12:46 AM, Junio C Hamano gits...@pobox.com wrote:
Karthik Nayak karthik@gmail.com writes:
Here is what I see...
ok 98 - verifying rfc1991 signature
expecting success:
echo rfc1991 gpghome/gpg.conf
echo rfc1991-signed-tag RFC1991 signed tag expect
git tag -l -n1 rfc1991-signed-tag actual
test_cmp expect actual
git tag -l -n2 rfc1991-signed-tag actual
test_cmp expect actual
git tag -l -n999 rfc1991-signed-tag actual
test_cmp expect actual
--- expect 2015-08-24 22:54:44.607272653 +
+++ actual 2015-08-24 22:54:44.611272643 +
@@ -1 +1 @@
-rfc1991-signed-tag RFC1991 signed tag
+rfc1991-signed-tagRFC1991 signed tag
not ok 99 - list tag with rfc1991 signature
Thats weird, I just ran all tests, and nothing failed.
You may be missing GPG or RFC1991 prerequisites and not running this
particular test, which could be why you missed it.
That explains.
Your builtin/tag.c calls show_ref_array_item() with
%(align:16,left)%(refname:short)%(end)
as the format, and rfc1991-signed-tag is longer than 16.
And immediately after showing that, there is this hack at the end of
show_ref_array_item() function, which I _think_ should not be there
in ref-filter.c:show_ref_array_item() in the first place.
if (lines 0) {
struct object_id oid;
hashcpy(oid.hash, info-objectname);
show_tag_lines(oid, lines);
}
This belongs to the caller who knows that it is dealing with a tag.
Explained the idea behind this below.
But that broken design aside, the reason why this breaks is because
you do not have a separating SP after the aligned short refs.
Makes sense.
I didn't check how wide the original is supposed to be, but perhaps
changing builtin/tag.c this way
if (filter-lines)
- format = %(align:16,left)%(refname:short)%(end);
+ format = %(align:15,left)%(refname:short)%(end) ;
else
format = %(refname:short);
}
may be one way to fix it. Check the original tag -l output for
tags whose names are 14, 15, 16 and 17 display-columns wide and try
to match it.
That should be the fix, since it's a space problem.
And then move the tag-specific code at the end of
show_ref_array_item() to here:
verify_ref_format(format);
filter_refs(array, filter, FILTER_REFS_TAGS);
ref_array_sort(sorting, array);
- for (i = 0; i array.nr; i++)
+ for (i = 0; i array.nr; i++) {
show_ref_array_item(array.items[i], format, QUOTE_NONE,
filter-lines);
+ if (lines) {
+ struct object_id oid;
+ hashcpy(oid.hash, info-objectname);
+ show_tag_lines(oid, lines);
+ }
+ }
perhaps.
The problem with doing this is, the lines need to be displayed
immediately after the refname,
followed by a newline, what you're suggesting breaks that flow.
We could pass a boolean to show_ref_array_item() and print a newline if no
lines are to be printed and probably print the newline in tag.c
itself, but seems confusing to me.
Heh, I did it myself. %(align:15) with trailing whitespace is what
you want.
An alternative way to spell it would be
%(align:16,left)%(refname:short) %(end)
I don't know which one is more readable, though.
I find this more readable, since the space is clearly visible, unlike
format = %(align:15,left)%(refname:short)%(end) ; in which the space
after %(end) is easily unnoticeable.
--
Regards,
Karthik Nayak
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v13 00/12] port tag.c to use ref-filter APIs
Karthik Nayak karthik@gmail.com writes:
Here is what I see...
ok 98 - verifying rfc1991 signature
expecting success:
echo rfc1991 gpghome/gpg.conf
echo rfc1991-signed-tag RFC1991 signed tag expect
git tag -l -n1 rfc1991-signed-tag actual
test_cmp expect actual
git tag -l -n2 rfc1991-signed-tag actual
test_cmp expect actual
git tag -l -n999 rfc1991-signed-tag actual
test_cmp expect actual
--- expect 2015-08-24 22:54:44.607272653 +
+++ actual 2015-08-24 22:54:44.611272643 +
@@ -1 +1 @@
-rfc1991-signed-tag RFC1991 signed tag
+rfc1991-signed-tagRFC1991 signed tag
not ok 99 - list tag with rfc1991 signature
Thats weird, I just ran all tests, and nothing failed.
You may be missing GPG or RFC1991 prerequisites and not running this
particular test, which could be why you missed it.
Your builtin/tag.c calls show_ref_array_item() with
%(align:16,left)%(refname:short)%(end)
as the format, and rfc1991-signed-tag is longer than 16.
And immediately after showing that, there is this hack at the end of
show_ref_array_item() function, which I _think_ should not be there
in ref-filter.c:show_ref_array_item() in the first place.
if (lines 0) {
struct object_id oid;
hashcpy(oid.hash, info-objectname);
show_tag_lines(oid, lines);
}
This belongs to the caller who knows that it is dealing with a tag.
But that broken design aside, the reason why this breaks is because
you do not have a separating SP after the aligned short refs.
I didn't check how wide the original is supposed to be, but perhaps
changing builtin/tag.c this way
if (filter-lines)
- format = %(align:16,left)%(refname:short)%(end);
+ format = %(align:15,left)%(refname:short)%(end) ;
else
format = %(refname:short);
}
may be one way to fix it. Check the original tag -l output for
tags whose names are 14, 15, 16 and 17 display-columns wide and try
to match it.
And then move the tag-specific code at the end of
show_ref_array_item() to here:
verify_ref_format(format);
filter_refs(array, filter, FILTER_REFS_TAGS);
ref_array_sort(sorting, array);
- for (i = 0; i array.nr; i++)
+ for (i = 0; i array.nr; i++) {
show_ref_array_item(array.items[i], format, QUOTE_NONE,
filter-lines);
+ if (lines) {
+ struct object_id oid;
+ hashcpy(oid.hash, info-objectname);
+ show_tag_lines(oid, lines);
+ }
+ }
perhaps.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v13 00/12] port tag.c to use ref-filter APIs
Junio C Hamano gits...@pobox.com writes: I didn't check how wide the original is supposed to be, but perhaps changing builtin/tag.c this way if (filter-lines) - format = %(align:16,left)%(refname:short)%(end); + format = %(align:15,left)%(refname:short)%(end) ; else format = %(refname:short); } may be one way to fix it. Check the original tag -l output for tags whose names are 14, 15, 16 and 17 display-columns wide and try to match it. Heh, I did it myself. %(align:15) with trailing whitespace is what you want. An alternative way to spell it would be %(align:16,left)%(refname:short) %(end) I don't know which one is more readable, though. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v13 00/12] port tag.c to use ref-filter APIs
On Tue, Aug 25, 2015 at 4:04 AM, Junio C Hamano gits...@pobox.com wrote: Matthieu Moy matthieu@grenoble-inp.fr writes: Karthik Nayak karthik@gmail.com writes: diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt index 1997657..06d468e 100644 --- a/Documentation/git-for-each-ref.txt +++ b/Documentation/git-for-each-ref.txt @@ -133,7 +133,8 @@ align:: `position` is either left, right or middle and `width` is the total length of the content with alignment. If the contents length is more than the width then no alignment is -performed. +performed. If used with '--quote' everything in between %(align:..) +and %(end) is quoted. There's no --quote, there are --shell, --python, ... (well, actually, I would have prefered to have a single --quote=language option, but that's not how it is now). I had already commented on a preliminary version of this series off-list. I think all my previous comments have been taken into account. Thanks, both. I think this is pretty close to being satisfactory ;-) There may probably be a handful of minor nits like the above that need to be addressed, but I do not think I saw anything glaringly wrong that makes the series unsalvageable. It was a very pleasant read. It's almost there, and I am very happy to see how this and other series evolved so far ;-) I like how it's totally different from what I started off with, it's been a great learning curve for me. Thanks for reviews. -- Regards, Karthik Nayak -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v13 00/12] port tag.c to use ref-filter APIs
On Tue, Aug 25, 2015 at 4:28 AM, Junio C Hamano gits...@pobox.com wrote: Junio C Hamano gits...@pobox.com writes: Matthieu Moy matthieu@grenoble-inp.fr writes: Karthik Nayak karthik@gmail.com writes: diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt index 1997657..06d468e 100644 --- a/Documentation/git-for-each-ref.txt +++ b/Documentation/git-for-each-ref.txt @@ -133,7 +133,8 @@ align:: `position` is either left, right or middle and `width` is the total length of the content with alignment. If the contents length is more than the width then no alignment is - performed. + performed. If used with '--quote' everything in between %(align:..) + and %(end) is quoted. There's no --quote, there are --shell, --python, ... (well, actually, I would have prefered to have a single --quote=language option, but that's not how it is now). I had already commented on a preliminary version of this series off-list. I think all my previous comments have been taken into account. Thanks, both. I think this is pretty close to being satisfactory ;-) There may probably be a handful of minor nits like the above that need to be addressed, but I do not think I saw anything glaringly wrong that makes the series unsalvageable. It was a very pleasant read. It's almost there, and I am very happy to see how this and other series evolved so far ;-) Having said all that, it seems that there is some trivial typo or thinko in the formatting code to break t7004. Here is what I see... ok 98 - verifying rfc1991 signature expecting success: echo rfc1991 gpghome/gpg.conf echo rfc1991-signed-tag RFC1991 signed tag expect git tag -l -n1 rfc1991-signed-tag actual test_cmp expect actual git tag -l -n2 rfc1991-signed-tag actual test_cmp expect actual git tag -l -n999 rfc1991-signed-tag actual test_cmp expect actual --- expect 2015-08-24 22:54:44.607272653 + +++ actual 2015-08-24 22:54:44.611272643 + @@ -1 +1 @@ -rfc1991-signed-tag RFC1991 signed tag +rfc1991-signed-tagRFC1991 signed tag not ok 99 - list tag with rfc1991 signature Thats weird, I just ran all tests, and nothing failed. Is this mid-way of the patch series? I tried it on the entire series and seems fine to me -- Regards, Karthik Nayak -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v13 00/12] port tag.c to use ref-filter APIs
On Tue, Aug 25, 2015 at 12:23 AM, Junio C Hamano gits...@pobox.com wrote: Karthik Nayak karthik@gmail.com writes: On Mon, Aug 24, 2015 at 10:57 PM, Junio C Hamano gits...@pobox.com wrote: Karthik Nayak karthik@gmail.com writes: ... + performed. If used with '--quote' everything in between %(align:..) + and %(end) is quoted. ... I might have misunderstood you, But based on the discussion held here (thread.gmane.org/gmane.comp.version-control.git/276140) I thought we wanted everything inside the %(align) %(end) atoms to be quoted. Perhaps I misunderstood your intention in the doc. I took everything in between %(align:...) and %(end) is quoted to mean that %(if:empty)%(align)%(end)%(then)Empty%(else)Not Empty%(end) can never satisfy %(if:empty), because %(align)%(end) would expand to a string that has two single-quotes, that is not an empty string. If that is not what would happen in the branch --list enhancement, then the proposed behaviour is good, but the above documentation would need to be updated when it happens, I think. It at least is misleading. Thanks. I'm not sure I checked this condition, will have a look at this, thanks for pointing it out. OK, now I checked the code, and I _think_ the recursive logic is doing the right thing (modulo minor nits on comment-vs-code discrepancy and code structure I sent separately). I'm not so sure about that, I'll get back on this. (I didn't think about empty string alignment and its effect on %(if:empty) and so on). -- Regards, Karthik Nayak -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v13 00/12] port tag.c to use ref-filter APIs
Karthik Nayak karthik@gmail.com writes: On Mon, Aug 24, 2015 at 1:30 AM, Matthieu Moy matthieu@grenoble-inp.fr wrote: Karthik Nayak karthik@gmail.com writes: diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt index 1997657..06d468e 100644 --- a/Documentation/git-for-each-ref.txt +++ b/Documentation/git-for-each-ref.txt @@ -133,7 +133,8 @@ align:: `position` is either left, right or middle and `width` is the total length of the content with alignment. If the contents length is more than the width then no alignment is - performed. + performed. If used with '--quote' everything in between %(align:..) + and %(end) is quoted. There's no --quote, there are --shell, --python, ... (well, actually, I would have prefered to have a single --quote=language option, but that's not how it is now). That'd be easy to implement, but I didn't because `git tag -l` is human readable and I didn't see the necessity for having something like `--quote_type` here. Agreed: tag does not have --shell, --python so and does not need it. But that's not my point: you write if used with '--quote', and the option name is not --quote. It should be if used with `--shell`, `--python`, ... then everything -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v13 00/12] port tag.c to use ref-filter APIs
On Mon, Aug 24, 2015 at 1:30 AM, Matthieu Moy matthieu@grenoble-inp.fr wrote: Karthik Nayak karthik@gmail.com writes: diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt index 1997657..06d468e 100644 --- a/Documentation/git-for-each-ref.txt +++ b/Documentation/git-for-each-ref.txt @@ -133,7 +133,8 @@ align:: `position` is either left, right or middle and `width` is the total length of the content with alignment. If the contents length is more than the width then no alignment is - performed. + performed. If used with '--quote' everything in between %(align:..) + and %(end) is quoted. There's no --quote, there are --shell, --python, ... (well, actually, I would have prefered to have a single --quote=language option, but that's not how it is now). That'd be easy to implement, but I didn't because `git tag -l` is human readable and I didn't see the necessity for having something like `--quote_type` here. It'd be better to just use `git for-each-ref refs/tags/` for that. I had already commented on a preliminary version of this series off-list. I think all my previous comments have been taken into account. Thanks, Thanks for the review :) -- Regards, Karthik Nayak -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v13 00/12] port tag.c to use ref-filter APIs
On Mon, Aug 24, 2015 at 8:46 PM, Matthieu Moy matthieu@grenoble-inp.fr wrote: Karthik Nayak karthik@gmail.com writes: On Mon, Aug 24, 2015 at 1:30 AM, Matthieu Moy matthieu@grenoble-inp.fr wrote: Karthik Nayak karthik@gmail.com writes: diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt index 1997657..06d468e 100644 --- a/Documentation/git-for-each-ref.txt +++ b/Documentation/git-for-each-ref.txt @@ -133,7 +133,8 @@ align:: `position` is either left, right or middle and `width` is the total length of the content with alignment. If the contents length is more than the width then no alignment is - performed. + performed. If used with '--quote' everything in between %(align:..) + and %(end) is quoted. There's no --quote, there are --shell, --python, ... (well, actually, I would have prefered to have a single --quote=language option, but that's not how it is now). That'd be easy to implement, but I didn't because `git tag -l` is human readable and I didn't see the necessity for having something like `--quote_type` here. Agreed: tag does not have --shell, --python so and does not need it. But that's not my point: you write if used with '--quote', and the option name is not --quote. It should be if used with `--shell`, `--python`, ... then everything Oops! misread what you sent, maybe' --quote_type' instead. -- Regards, Karthik Nayak -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v13 00/12] port tag.c to use ref-filter APIs
Karthik Nayak karthik@gmail.com writes: Interdiff: diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt index 1997657..06d468e 100644 --- a/Documentation/git-for-each-ref.txt +++ b/Documentation/git-for-each-ref.txt @@ -133,7 +133,8 @@ align:: `position` is either left, right or middle and `width` is the total length of the content with alignment. If the contents length is more than the width then no alignment is - performed. + performed. If used with '--quote' everything in between %(align:..) + and %(end) is quoted. That sounds like a buggy behaviour that we may want to fix later, though. Perhaps document it as a known bug, e.g. Currently it does not work well when used with language-specific quoting like --shell, etc. (while punting on fixing the implementation for now)? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v13 00/12] port tag.c to use ref-filter APIs
On Mon, Aug 24, 2015 at 10:57 PM, Junio C Hamano gits...@pobox.com wrote: Karthik Nayak karthik@gmail.com writes: Interdiff: diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt index 1997657..06d468e 100644 --- a/Documentation/git-for-each-ref.txt +++ b/Documentation/git-for-each-ref.txt @@ -133,7 +133,8 @@ align:: `position` is either left, right or middle and `width` is the total length of the content with alignment. If the contents length is more than the width then no alignment is - performed. + performed. If used with '--quote' everything in between %(align:..) + and %(end) is quoted. That sounds like a buggy behaviour that we may want to fix later, though. Perhaps document it as a known bug, e.g. Currently it does not work well when used with language-specific quoting like --shell, etc. (while punting on fixing the implementation for now)? I might have misunderstood you, But based on the discussion held here (thread.gmane.org/gmane.comp.version-control.git/276140) I thought we wanted everything inside the %(align) %(end) atoms to be quoted. So I'm a little confused by what you're trying to say. -- Regards, Karthik Nayak -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v13 00/12] port tag.c to use ref-filter APIs
Karthik Nayak karthik@gmail.com writes: On Mon, Aug 24, 2015 at 10:57 PM, Junio C Hamano gits...@pobox.com wrote: Karthik Nayak karthik@gmail.com writes: ... + performed. If used with '--quote' everything in between %(align:..) + and %(end) is quoted. ... I might have misunderstood you, But based on the discussion held here (thread.gmane.org/gmane.comp.version-control.git/276140) I thought we wanted everything inside the %(align) %(end) atoms to be quoted. Perhaps I misunderstood your intention in the doc. I took everything in between %(align:...) and %(end) is quoted to mean that %(if:empty)%(align)%(end)%(then)Empty%(else)Not Empty%(end) can never satisfy %(if:empty), because %(align)%(end) would expand to a string that has two single-quotes, that is not an empty string. If that is not what would happen in the branch --list enhancement, then the proposed behaviour is good, but the above documentation would need to be updated when it happens, I think. It at least is misleading. Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v13 00/12] port tag.c to use ref-filter APIs
Junio C Hamano gits...@pobox.com writes: Karthik Nayak karthik@gmail.com writes: On Mon, Aug 24, 2015 at 10:57 PM, Junio C Hamano gits...@pobox.com wrote: Karthik Nayak karthik@gmail.com writes: ... + performed. If used with '--quote' everything in between %(align:..) + and %(end) is quoted. ... I might have misunderstood you, But based on the discussion held here (thread.gmane.org/gmane.comp.version-control.git/276140) I thought we wanted everything inside the %(align) %(end) atoms to be quoted. Perhaps I misunderstood your intention in the doc. I took everything in between %(align:...) and %(end) is quoted to mean that %(if:empty)%(align)%(end)%(then)Empty%(else)Not Empty%(end) can never satisfy %(if:empty), because %(align)%(end) would expand to a string that has two single-quotes, that is not an empty string. If that is not what would happen in the branch --list enhancement, then the proposed behaviour is good, but the above documentation would need to be updated when it happens, I think. It at least is misleading. OK, now I checked the code, and I _think_ the recursive logic is doing the right thing (modulo minor nits on comment-vs-code discrepancy and code structure I sent separately). -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v13 00/12] port tag.c to use ref-filter APIs
Junio C Hamano gits...@pobox.com writes: Matthieu Moy matthieu@grenoble-inp.fr writes: Karthik Nayak karthik@gmail.com writes: diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt index 1997657..06d468e 100644 --- a/Documentation/git-for-each-ref.txt +++ b/Documentation/git-for-each-ref.txt @@ -133,7 +133,8 @@ align:: `position` is either left, right or middle and `width` is the total length of the content with alignment. If the contents length is more than the width then no alignment is - performed. + performed. If used with '--quote' everything in between %(align:..) + and %(end) is quoted. There's no --quote, there are --shell, --python, ... (well, actually, I would have prefered to have a single --quote=language option, but that's not how it is now). I had already commented on a preliminary version of this series off-list. I think all my previous comments have been taken into account. Thanks, both. I think this is pretty close to being satisfactory ;-) There may probably be a handful of minor nits like the above that need to be addressed, but I do not think I saw anything glaringly wrong that makes the series unsalvageable. It was a very pleasant read. It's almost there, and I am very happy to see how this and other series evolved so far ;-) Having said all that, it seems that there is some trivial typo or thinko in the formatting code to break t7004. Here is what I see... ok 98 - verifying rfc1991 signature expecting success: echo rfc1991 gpghome/gpg.conf echo rfc1991-signed-tag RFC1991 signed tag expect git tag -l -n1 rfc1991-signed-tag actual test_cmp expect actual git tag -l -n2 rfc1991-signed-tag actual test_cmp expect actual git tag -l -n999 rfc1991-signed-tag actual test_cmp expect actual --- expect 2015-08-24 22:54:44.607272653 + +++ actual 2015-08-24 22:54:44.611272643 + @@ -1 +1 @@ -rfc1991-signed-tag RFC1991 signed tag +rfc1991-signed-tagRFC1991 signed tag not ok 99 - list tag with rfc1991 signature -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v13 00/12] port tag.c to use ref-filter APIs
Matthieu Moy matthieu@grenoble-inp.fr writes: Karthik Nayak karthik@gmail.com writes: diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt index 1997657..06d468e 100644 --- a/Documentation/git-for-each-ref.txt +++ b/Documentation/git-for-each-ref.txt @@ -133,7 +133,8 @@ align:: `position` is either left, right or middle and `width` is the total length of the content with alignment. If the contents length is more than the width then no alignment is -performed. +performed. If used with '--quote' everything in between %(align:..) +and %(end) is quoted. There's no --quote, there are --shell, --python, ... (well, actually, I would have prefered to have a single --quote=language option, but that's not how it is now). I had already commented on a preliminary version of this series off-list. I think all my previous comments have been taken into account. Thanks, both. I think this is pretty close to being satisfactory ;-) There may probably be a handful of minor nits like the above that need to be addressed, but I do not think I saw anything glaringly wrong that makes the series unsalvageable. It was a very pleasant read. It's almost there, and I am very happy to see how this and other series evolved so far ;-) -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v13 00/12] port tag.c to use ref-filter APIs
Karthik Nayak karthik@gmail.com writes: diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt index 1997657..06d468e 100644 --- a/Documentation/git-for-each-ref.txt +++ b/Documentation/git-for-each-ref.txt @@ -133,7 +133,8 @@ align:: `position` is either left, right or middle and `width` is the total length of the content with alignment. If the contents length is more than the width then no alignment is - performed. + performed. If used with '--quote' everything in between %(align:..) + and %(end) is quoted. There's no --quote, there are --shell, --python, ... (well, actually, I would have prefered to have a single --quote=language option, but that's not how it is now). I had already commented on a preliminary version of this series off-list. I think all my previous comments have been taken into account. Thanks, -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
