* Re: [PATCH v15 13/27] bisect--helper: `bisect_start` shell function partially in C
From: Stephan Beyer @ 2016-11-20 20:19 UTC (permalink / raw)
To: Pranit Bauva, git
In-Reply-To: <52f6241f-e584-d830-ca66-084dc509c7fe@gmx.net>
On 11/20/2016 09:01 PM, Stephan Beyer wrote:
> First, replace the current set_terms() by
>
> static void set_terms(struct bisect_terms *terms, const char *bad,
> const char *good)
> {
> terms->term_good = xstrdup(good);
> terms->term_bad = xstrdup(bad);
> }
>
> ie, without calling write_terms(...).
I did not want to confuse you here but I forgot to mention that there
should also be freeing code, i.e. initialize your terms to NULL in the
beginning of cmd_builtin__helper, and always free them if it is not
null. This freeing code could also be in an extra function free_terms()
and you call it in set_terms() and for cleanup in the end.
^ permalink raw reply
* Re: Fwd: git diff with “--word-diff-regex” extremely slow compared to “--word-diff”?
From: Jeff King @ 2016-11-20 20:17 UTC (permalink / raw)
To: Matthieu S; +Cc: git
In-Reply-To: <CAEYvigLz3muWD-QFjMZUn=H3RQoxhTYX9EwB6=aiMjWOEN3CBA@mail.gmail.com>
On Fri, Nov 18, 2016 at 03:40:22PM -0800, Matthieu S wrote:
> Why is the speed so different if one uses --word-diff instead of
> --word-diff-regex= ? Is it just because my expression is (slightly)
> more complex than the default one (split on period instead of only
> whitespace) ? Or is it that the default word-diff is implemented
> differently/more efficiently? How can I overcome this speed slowdown?
I think it's probably both.
See diff.c:find_word_boundaries(). If there's no regex, we use a simple
loop over isspace() to find the boundaries. I don't recall anybody
measuring the performance before, but I'm not surprised to hear that
matching a regex is slower.
If I look at the output of "perf", though, it looks like we also spend a
lot more time in xdl_clean_mmatch(). Which isn't surprising. Your regex
treats commas as boundaries, which is going to generate a lot more
matches for this particular data set (though the output is the same, I
think, because of the nature of the change).
I would have expected "--word-diff-regex=[^[:space:]]" to be faster than
your regex, though, and it does not seem to be.
-Peff
^ permalink raw reply
* Re: [PATCH v15 18/27] bisect--helper: `bisect_autostart` shell function in C
From: Stephan Beyer @ 2016-11-20 20:15 UTC (permalink / raw)
To: Pranit Bauva, git
In-Reply-To: <01020157c38b1b1a-067117ef-cd0d-469b-ba80-ea1a1169f694-000000@eu-west-1.amazonses.com>
Hi,
On 10/14/2016 04:14 PM, Pranit Bauva wrote:
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 502bf18..1767916 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -422,6 +425,7 @@ static int bisect_next(...)
> {
> int res, no_checkout;
>
> + bisect_autostart(terms);
You are not checking for return values here. (The shell code simply
exited if there is no tty, but you don't.)
> @@ -754,6 +758,32 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,
> return retval || bisect_auto_next(terms, NULL);
> }
>
> +static int bisect_autostart(struct bisect_terms *terms)
> +{
> + if (is_empty_or_missing_file(git_path_bisect_start())) {
> + const char *yesno;
> + const char *argv[] = {NULL};
> + fprintf(stderr, _("You need to start by \"git bisect "
> + "start\"\n"));
> +
> + if (!isatty(0))
isatty(STDIN_FILENO)?
> + return 1;
> +
> + /*
> + * TRANSLATORS: Make sure to include [Y] and [n] in your
> + * translation. THe program will only accept English input
Typo "THe"
> + * at this point.
> + */
Taking "at this point" into consideration, I think the Y and n can be
easily translated now that it is in C. I guess, by using...
> + yesno = git_prompt(_("Do you want me to do it for you "
> + "[Y/n]? "), PROMPT_ECHO);
> + if (starts_with(yesno, "n") || starts_with(yesno, "N"))
... starts_with(yesno, _("n")) || starts_with(yesno, _("N"))
here (but not sure). However, this would be an extra patch on top of
this series.
> + exit(0);
Shouldn't this also be "return 1;"? Saying "no" is the same outcome as
not having a tty to ask for yes or no.
> int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
> {
> enum {
> @@ -790,6 +821,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
> N_("find the next bisection commit"), BISECT_NEXT),
> OPT_CMDMODE(0, "bisect-auto-next", &cmdmode,
> N_("verify the next bisection state then find the next bisection state"), BISECT_AUTO_NEXT),
> + OPT_CMDMODE(0, "bisect-autostart", &cmdmode,
> + N_("start the bisection if BISECT_START empty or missing"), BISECT_AUTOSTART),
The word "is" is missing.
~Stephan
^ permalink raw reply
* Re: [PATCH v15 15/27] bisect--helper: `bisect_next` and `bisect_auto_next` shell function in C
From: Stephan Beyer @ 2016-11-20 20:01 UTC (permalink / raw)
To: Pranit Bauva, git
In-Reply-To: <01020157c38b1af0-5d688c2e-868d-4d8c-a8fd-9a675f7f01da-000000@eu-west-1.amazonses.com>
Hi Pranit,
this one is hard to review because you do two or three commits in one here.
I think the first commit should be the exit()->return conversion, the
second commit is next and autonext, and the third commit is the pretty
trivial bisect_start commit ;) However, you did it this way and it's
always a hassle to split commit, so I don't really care...
However, I was reviewing this superficially, to be honest. This mail
skips the next and autonext part.
On 10/14/2016 04:14 PM, Pranit Bauva wrote:
> diff --git a/bisect.c b/bisect.c
> index 45d598d..7c97e85 100644
> --- a/bisect.c
> +++ b/bisect.c
> @@ -843,16 +878,21 @@ static int check_ancestors(const char *prefix)
> *
> * If that's not the case, we need to check the merge bases.
> * If a merge base must be tested by the user, its source code will be
> - * checked out to be tested by the user and we will exit.
> + * checked out to be tested by the user and we will return.
> */
> -static void check_good_are_ancestors_of_bad(const char *prefix, int no_checkout)
> +static int check_good_are_ancestors_of_bad(const char *prefix, int no_checkout)
> {
> char *filename = git_pathdup("BISECT_ANCESTORS_OK");
> struct stat st;
> - int fd;
> + int fd, res = 0;
>
> + /*
> + * We don't want to clean the bisection state
> + * as we need to get back to where we started
> + * by using `git bisect reset`.
> + */
> if (!current_bad_oid)
> - die(_("a %s revision is needed"), term_bad);
> + error(_("a %s revision is needed"), term_bad);
Only error() or return error()?
> @@ -873,8 +916,11 @@ static void check_good_are_ancestors_of_bad(const char *prefix, int no_checkout)
> filename);
> else
> close(fd);
> +
> + goto done;
> done:
I never understand why you do this. In case of adding a "fail" label
(and fail code like "res = -1;") between "goto done" and "done:", it's
fine... but without one this is just a nop.
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 1d3e17f..fcd7574 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -427,15 +560,24 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,
> no_checkout = 1;
>
> for (i = 0; i < argc; i++) {
> - if (!strcmp(argv[i], "--")) {
> + const char *arg;
> + if (starts_with(argv[i], "'"))
> + arg = sq_dequote(xstrdup(argv[i]));
> + else
> + arg = argv[i];
One is xstrdup'ed, one is not, so there'll be a leak somewhere, and it's
an inconsistent leak... I guess it's a bad idea to do it this way ;)
(Also below.)
> @@ -443,24 +585,31 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,
> no_checkout = 1;
> } else if (!strcmp(arg, "--term-good") ||
> !strcmp(arg, "--term-old")) {
> + if (starts_with(argv[++i], "'"))
> + terms->term_good = sq_dequote(xstrdup(argv[i]));
> + else
> + terms->term_good = xstrdup(argv[i]);
> must_write_terms = 1;
> - terms->term_good = xstrdup(argv[++i]);
> } else if (skip_prefix(arg, "--term-good=", &arg)) {
> must_write_terms = 1;
> - terms->term_good = xstrdup(arg);
> + terms->term_good = arg;
No ;) (See my other comments (to other patches) for the "terms" leaks.)
[This repeats several times below.]
> diff --git a/git-bisect.sh b/git-bisect.sh
> index f0896b3..d574c44 100755
> --- a/git-bisect.sh
> +++ b/git-bisect.sh
> @@ -109,6 +88,7 @@ bisect_skip() {
> bisect_state() {
> bisect_autostart
> state=$1
> + get_terms
> git bisect--helper --check-and-set-terms $state $TERM_GOOD $TERM_BAD || exit
> get_terms
> case "$#,$state" in
I can't say if this change is right or wrong. It looks right, but: How
does this relate to the other changes? Is this the right patch for it?
~Stephan
^ permalink raw reply
* Re: [PATCH v15 13/27] bisect--helper: `bisect_start` shell function partially in C
From: Stephan Beyer @ 2016-11-20 20:01 UTC (permalink / raw)
To: Pranit Bauva, git
In-Reply-To: <01020157c38b1ad3-ea75ed97-2514-427e-8e57-9f10efd4e6e9-000000@eu-west-1.amazonses.com>
Hi,
On 10/14/2016 04:14 PM, Pranit Bauva wrote:
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 6a5878c..1d3e17f 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -403,6 +408,205 @@ static int bisect_terms(struct bisect_terms *terms, const char **argv, int argc)
> return 0;
> }
>
> +static int bisect_start(struct bisect_terms *terms, int no_checkout,
> + const char **argv, int argc)
> +{
> + int i, has_double_dash = 0, must_write_terms = 0, bad_seen = 0;
> + int flags, pathspec_pos, retval = 0;
> + struct string_list revs = STRING_LIST_INIT_DUP;
> + struct string_list states = STRING_LIST_INIT_DUP;
> + struct strbuf start_head = STRBUF_INIT;
> + struct strbuf bisect_names = STRBUF_INIT;
> + struct strbuf orig_args = STRBUF_INIT;
> + const char *head;
> + unsigned char sha1[20];
> + FILE *fp = NULL;
> + struct object_id oid;
> +
> + if (is_bare_repository())
> + no_checkout = 1;
> +
> + for (i = 0; i < argc; i++) {
> + if (!strcmp(argv[i], "--")) {
> + has_double_dash = 1;
> + break;
> + }
> + }
> +
> + for (i = 0; i < argc; i++) {
> + const char *commit_id = xstrfmt("%s^{commit}", argv[i]);
> + const char *arg = argv[i];
> + if (!strcmp(argv[i], "--")) {
> + has_double_dash = 1;
> + break;
> + } else if (!strcmp(arg, "--no-checkout")) {
> + no_checkout = 1;
> + } else if (!strcmp(arg, "--term-good") ||
> + !strcmp(arg, "--term-old")) {
> + must_write_terms = 1;
> + terms->term_good = xstrdup(argv[++i]);
All these xstrdup() for the terms here and below will leak memory.
I recommend to use xstrdup() also at (*) below, and use
free(terms->term_good) above this line (and for every occurrence below,
of course).
> + } else if (skip_prefix(arg, "--term-good=", &arg)) {
> + must_write_terms = 1;
> + terms->term_good = xstrdup(arg);
[...]
> @@ -497,6 +705,11 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
> die(_("--bisect-terms requires 0 or 1 argument"));
> res = bisect_terms(&terms, argv, argc);
> break;
> + case BISECT_START:
> + terms.term_good = "good";
> + terms.term_bad = "bad";
Here is (*): use xstrdup("good") etc.
And then, as already mentioned for another patch, free(terms.*) below.
I personally am a friend of small functions and would prefer something
like as follows... (This is a comment about several patches of your
series, not only this one.)
First, replace the current set_terms() by
static void set_terms(struct bisect_terms *terms, const char *bad,
const char *good)
{
terms->term_good = xstrdup(good);
terms->term_bad = xstrdup(bad);
}
ie, without calling write_terms(...).
And then replace the *current* set_terms() calls by set_terms(...);
write_terms(...); calls.
Second, add
static void get_default_terms(struct bisect_terms *terms)
{
set_terms(terms, "bad", "good");
}
and use this instead of the two lines quoted above (and all its other
occurrences).
Third, use the new set_terms() everywhere instead of settings terms
members directly (with the exception of get_terms()).
This sounds like a safer variant (with respect to leaks and handling
them) to me than doing it the current way.
~Stephan
^ permalink raw reply
* Re: [PATCH v7 13/17] ref-filter: add `:dir` and `:base` options for ref printing atoms
From: Jakub Narębski @ 2016-11-20 18:43 UTC (permalink / raw)
To: Junio C Hamano, Karthik Nayak; +Cc: Jacob Keller, Git mailing list
In-Reply-To: <xmqq4m32kqet.fsf@gitster.mtv.corp.google.com>
W dniu 20.11.2016 o 18:32, Junio C Hamano pisze:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> We could have lstrip and rstrip as you suggested and perhaps make
>> it work together too. But I see this going off the scope of this
>> series. Maybe I'll follow up with another series introducing these
>> features. Since we can currently make do with 'strip=2' I'll drop
>> this patch from v8 of this series and pursue this idea after this.
>
> My primary point was that if we know we want to add "rstrip" later
> and still decide not to add it right now, it is OK, but we will
> regret it if we named the one we are going to add right now "strip".
> That will mean that future users, when "rstrip" is introduced, will
> end up having to choose between "strip" and "rstrip" (as opposed to
> "lstrip" and "rstrip"), wondering why left-variant is more important
> and named without left/right prefix.
Another solution would be to implement 'splice=<offset>[,<length>]',
where if length is omitted it means to the end; perhaps with special
casing (as in Perl) of negative <offset> and/or negative <length>.
Or implement POSIX shell expansion:
%(parameter%word) - Remove Smallest Suffix Glob Pattern.
%(parameter%%word) - Remove Largest Suffix Glob Pattern.
%(parameter#word) - Remove Smallest Prefix Pattern.
%(parameter##word) - Remove Largest Prefix Pattern.
Though this one looks like serious overkill...
--
Jakub Narębski
^ permalink raw reply
* Re: [PATCH v7 13/17] ref-filter: add `:dir` and `:base` options for ref printing atoms
From: Junio C Hamano @ 2016-11-20 17:32 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Jakub Narębski, Jacob Keller, Git mailing list
In-Reply-To: <CAOLa=ZRf+vPOPK=ovP7JmJ52qdgwuqkpGH4UfP=+caQeyu9Ucw@mail.gmail.com>
Karthik Nayak <karthik.188@gmail.com> writes:
> We could have lstrip and rstrip as you suggested and perhaps make it work
> together too. But I see this going off the scope of this series. Maybe
> I'll follow up
> with another series introducing these features. Since we can currently
> make do with
> 'strip=2' I'll drop this patch from v8 of this series and pursue this
> idea after this.
My primary point was that if we know we want to add "rstrip" later
and still decide not to add it right now, it is OK, but we will
regret it if we named the one we are going to add right now "strip".
That will mean that future users, when "rstrip" is introduced, will
end up having to choose between "strip" and "rstrip" (as opposed to
"lstrip" and "rstrip"), wondering why left-variant is more important
and named without left/right prefix.
^ permalink raw reply
* Re: [PATCH v7 13/17] ref-filter: add `:dir` and `:base` options for ref printing atoms
From: Karthik Nayak @ 2016-11-20 16:52 UTC (permalink / raw)
To: Jakub Narębski; +Cc: Junio C Hamano, Jacob Keller, Git mailing list
In-Reply-To: <CAOLa=ZRf+vPOPK=ovP7JmJ52qdgwuqkpGH4UfP=+caQeyu9Ucw@mail.gmail.com>
On Sun, Nov 20, 2016 at 8:46 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
> On Fri, Nov 18, 2016 at 11:48 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Jacob Keller <jacob.keller@gmail.com> writes:
>>
>>>>>> to get remotes from /refs/foo/abc/xyz we'd need to do
>>>>>> strip=1,strip=-1, which could be
>>>>>> done but ...
>>>>>
>>>>> ... would be unnecessary if this is the only use case:
>>>>>
>>>>>> strbuf_addf(&fmt,
>>>>>> "%%(if:notequals=remotes)%%(refname:base)%%(then)%s%%(else)%s%%(end)",
>>>>>> local.buf, remote.buf);
>>>>>
>>>>> You can "strip to leave only 2 components" and compare the result
>>>>> with refs/remotes instead, no?
>>>>>
>>>>
>>>> Of course, my only objective was that someone would find it useful to
>>>> have these two additional
>>>> atoms. So if you think it's unnecessary we could drop it entirely :D
>>>>
>>>> --
>>>> Regards,
>>>> Karthik Nayak
>>>
>>> I think having strip and rstrip make sense, (along with support for
>>> negative numbers) I don't think we need to make them work together
>>> unless someone is interested, since we can use strip=-2 to get the
>>> behavior we need today.
>>
>> I am OK with multiple strips Karthik suggests, e.g.
>>
>> %(refname:strip=1,rstrip=-1)
>>
>> if it is cleanly implemented.
>>
>> I have a bit of trouble with these names, though. If we call one
>> strip and the other rstrip, to only those who know about rstrip it
>> would be clear that strip is about stripping from the left. Perhaps
>> we should call it lstrip for symmetry and ease-of-remembering?
>>
>> refs/heads/master:lstrip=-1 => master (strip all but one level
>> from the left)
>>
>> refs/heads/master:rstrip=-2 => refs/heads (strip all but two
>> levels from the right)
>>
>> refs/heads/master:lstrip=1,rstrip=-1 => heads (strip one level
>> from the left and then strip all but one level from the right)
>>
>> I dunno.
>
> We could have lstrip and rstrip as you suggested and perhaps make it work
> together too. But I see this going off the scope of this series. Maybe
> I'll follow up
> with another series introducing these features. Since we can currently
> make do with
> 'strip=2' I'll drop this patch from v8 of this series and pursue this
> idea after this.
>
I meant 'strip=-2'. I mean I'll add in the negative striping in this
series and follow
up with something that'd introduce lstrip and rstrip.
--
Regards,
Karthik Nayak
^ permalink raw reply
* Re: [PATCH v7 14/17] ref-filter: allow porcelain to translate messages in the output
From: Karthik Nayak @ 2016-11-20 15:33 UTC (permalink / raw)
To: Jakub Narębski; +Cc: Git List, Jacob Keller, Matthieu Moy
In-Reply-To: <af0b7bdc-2b29-0d04-85f1-aa1d5a2ba549@gmail.com>
cc'in Matthieu since he wrote the patch.
On Sat, Nov 19, 2016 at 4:16 AM, Jakub Narębski <jnareb@gmail.com> wrote:
> W dniu 08.11.2016 o 21:12, Karthik Nayak pisze:
>> From: Karthik Nayak <karthik.188@gmail.com>
>>
>> Introduce setup_ref_filter_porcelain_msg() so that the messages used in
>> the atom %(upstream:track) can be translated if needed. This is needed
>> as we port branch.c to use ref-filter's printing API's.
>>
>> Written-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
>> Mentored-by: Christian Couder <christian.couder@gmail.com>
>> Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>> ---
>> ref-filter.c | 28 ++++++++++++++++++++++++----
>> ref-filter.h | 2 ++
>> 2 files changed, 26 insertions(+), 4 deletions(-)
>>
>> diff --git a/ref-filter.c b/ref-filter.c
>> index b47b900..944671a 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -15,6 +15,26 @@
>> #include "version.h"
>> #include "wt-status.h"
>>
>> +static struct ref_msg {
>> + const char *gone;
>> + const char *ahead;
>> + const char *behind;
>> + const char *ahead_behind;
>> +} msgs = {
>> + "gone",
>> + "ahead %d",
>> + "behind %d",
>> + "ahead %d, behind %d"
>> +};
>> +
>> +void setup_ref_filter_porcelain_msg(void)
>> +{
>> + msgs.gone = _("gone");
>> + msgs.ahead = _("ahead %d");
>> + msgs.behind = _("behind %d");
>> + msgs.ahead_behind = _("ahead %d, behind %d");
>> +}
>
> Do I understand it correctly that this mechanism is here to avoid
> repeated calls into gettext, as those messages would get repeated
> over and over; otherwise one would use foo = N_("...") and _(foo),
> isn't it?
>
> I wonder if there is some way to avoid duplication here, but I don't
> see anything easy and safe (e.g. against running setup_*() twice).
>
That is the intention.
--
Regards,
Karthik Nayak
^ permalink raw reply
* Re: [PATCH v7 13/17] ref-filter: add `:dir` and `:base` options for ref printing atoms
From: Karthik Nayak @ 2016-11-20 15:16 UTC (permalink / raw)
To: Jakub Narębski; +Cc: Junio C Hamano, Jacob Keller, Git mailing list
In-Reply-To: <20d067ef-9e2c-0d1f-f81a-06c154e95e4f@gmail.com>
On Fri, Nov 18, 2016 at 11:48 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jacob Keller <jacob.keller@gmail.com> writes:
>
>>>>> to get remotes from /refs/foo/abc/xyz we'd need to do
>>>>> strip=1,strip=-1, which could be
>>>>> done but ...
>>>>
>>>> ... would be unnecessary if this is the only use case:
>>>>
>>>>> strbuf_addf(&fmt,
>>>>> "%%(if:notequals=remotes)%%(refname:base)%%(then)%s%%(else)%s%%(end)",
>>>>> local.buf, remote.buf);
>>>>
>>>> You can "strip to leave only 2 components" and compare the result
>>>> with refs/remotes instead, no?
>>>>
>>>
>>> Of course, my only objective was that someone would find it useful to
>>> have these two additional
>>> atoms. So if you think it's unnecessary we could drop it entirely :D
>>>
>>> --
>>> Regards,
>>> Karthik Nayak
>>
>> I think having strip and rstrip make sense, (along with support for
>> negative numbers) I don't think we need to make them work together
>> unless someone is interested, since we can use strip=-2 to get the
>> behavior we need today.
>
> I am OK with multiple strips Karthik suggests, e.g.
>
> %(refname:strip=1,rstrip=-1)
>
> if it is cleanly implemented.
>
> I have a bit of trouble with these names, though. If we call one
> strip and the other rstrip, to only those who know about rstrip it
> would be clear that strip is about stripping from the left. Perhaps
> we should call it lstrip for symmetry and ease-of-remembering?
>
> refs/heads/master:lstrip=-1 => master (strip all but one level
> from the left)
>
> refs/heads/master:rstrip=-2 => refs/heads (strip all but two
> levels from the right)
>
> refs/heads/master:lstrip=1,rstrip=-1 => heads (strip one level
> from the left and then strip all but one level from the right)
>
> I dunno.
We could have lstrip and rstrip as you suggested and perhaps make it work
together too. But I see this going off the scope of this series. Maybe
I'll follow up
with another series introducing these features. Since we can currently
make do with
'strip=2' I'll drop this patch from v8 of this series and pursue this
idea after this.
On Sat, Nov 19, 2016 at 3:19 AM, Jakub Narębski <jnareb@gmail.com> wrote:
> W dniu 15.11.2016 o 18:42, Junio C Hamano pisze:
>> Jacob Keller <jacob.keller@gmail.com> writes:
>>
>>> dirname makes sense. What about implementing a reverse variant of
>>> strip, which you could perform stripping of right-most components and
>>> instead of stripping by a number, strip "to" a number, ie: keep the
>>> left N most components, and then you could use something like
>>> ...
>>> I think that would be more general purpose than basename, and less confusing?
>>
>> I think you are going in the right direction. I had a similar
>> thought but built around a different axis. I.e. if strip=1 strips
>> one from the left, perhaps we want to have rstrip=1 that strips one
>> from the right, and also strip=-1 to mean strip everything except
>> one from the left and so on?. I think this and your keep (and
>> perhaps you'll have rkeep for completeness) have the same expressive
>> power. I do not offhand have a preference one over the other.
>>
>> Somehow it sounds a bit strange to me to treat 'remotes' as the same
>> class of token as 'heads' and 'tags' (I'd expect 'heads' and
>> 'remotes/origin' would be at the same level in end-user's mind), but
>> that is probably an unrelated tangent. The reason this series wants
>> to introduce :base must be to emulate an existing feature, so that
>> existing feature is a concrete counter-example that argues against
>> my "it sounds a bit strange" reaction.
>
> If it is to implement the feature where we select if to display only
> local branches (refs/heads/**), only remote-tracking branches
> (refs/remotes/**/**), or only tags (refs/tags/**), then perhaps
> ':category' or ':type' would make sense?
>
> As in '%(refname:category)', e.g.
>
> %(if:equals=heads)%(refname:category)%(then)...%(end)
>
This is also a good idea but would bring about the same confusion that Junio
was referring to, i.e.
"Somehow it sounds a bit strange to me to treat 'remotes' as the same
class of token as 'heads' and 'tags' (I'd expect 'heads' and
'remotes/origin' would be at the same level in end-user's mind), but
that is probably an unrelated tangent. The reason this series wants
to introduce :base must be to emulate an existing feature, so that
existing feature is a concrete counter-example that argues against
my "it sounds a bit strange" reaction."
So right now the rstrip/lstrip idea seems to be a good way to go about
this, but I
think that'd be after this series.
--
Regards,
Karthik Nayak
^ permalink raw reply
* [PATCH] i18n: Fixed unmatched single quote in error message
From: Jiang Xin @ 2016-11-20 12:26 UTC (permalink / raw)
To: Junio C Hamano, Johannes Schindelin; +Cc: Git List, Jiang Xin
Fixed unmatched single quote introduced by commit:
* f56fffef9a sequencer: teach write_message() to append an optional LF
Signed-off-by: Jiang Xin <worldhello.net@gmail.com>
---
sequencer.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sequencer.c b/sequencer.c
index 6f0ff9e413..30b10ba143 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -248,7 +248,7 @@ static int write_message(const void *buf, size_t len, const char *filename,
}
if (append_eol && write(msg_fd, "\n", 1) < 0) {
rollback_lock_file(&msg_file);
- return error_errno(_("could not write eol to '%s"), filename);
+ return error_errno(_("could not write eol to '%s'"), filename);
}
if (commit_lock_file(&msg_file) < 0) {
rollback_lock_file(&msg_file);
--
2.11.0.rc0.11.g127c283
^ permalink raw reply related
* Re: [PATCH v7 10/17] ref-filter: introduce refname_atom_parser_internal()
From: Karthik Nayak @ 2016-11-20 7:34 UTC (permalink / raw)
To: Jakub Narębski; +Cc: Git List, Jacob Keller
In-Reply-To: <5df0a607-4d83-8211-457a-96d7bde46eff@gmail.com>
On Sat, Nov 19, 2016 at 3:06 AM, Jakub Narębski <jnareb@gmail.com> wrote:
> W dniu 08.11.2016 o 21:12, Karthik Nayak pisze:
>> From: Karthik Nayak <karthik.188@gmail.com>
>>
>> Since there are multiple atoms which print refs ('%(refname)',
>> '%(symref)', '%(push)', '%upstream'), it makes sense to have a common
>
> Minor typo; it should be: "%(upstream)"
>
Will fix that.
>> ground for parsing them. This would allow us to share implementations of
>> the atom modifiers between these atoms.
>>
>> Introduce refname_atom_parser_internal() to act as a common parsing
>> function for ref printing atoms. This would eventually be used to
>> introduce refname_atom_parser() and symref_atom_parser() and also be
>> internally used in remote_ref_atom_parser().
>>
>> Helped-by: Jeff King <peff@peff.net>
>> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
>> ---
> [...]
>
>> +static void refname_atom_parser_internal(struct refname_atom *atom,
>> + const char *arg, const char *name)
>> +{
>> + if (!arg)
>> + atom->option = R_NORMAL;
>> + else if (!strcmp(arg, "short"))
>> + atom->option = R_SHORT;
>> + else if (skip_prefix(arg, "strip=", &arg)) {
>> + atom->option = R_STRIP;
>> + if (strtoul_ui(arg, 10, &atom->strip) || atom->strip <= 0)
>> + die(_("positive value expected refname:strip=%s"), arg);
>> + } else
> ^^^^^^
>
> It looks like you have spurious tab here.
>
That would have gone unnoticed, thanks for pointing it out.
--
Regards,
Karthik Nayak
^ permalink raw reply
* Re: [PATCH v7 09/17] ref-filter: make "%(symref)" atom work with the ':short' modifier
From: Karthik Nayak @ 2016-11-20 7:31 UTC (permalink / raw)
To: Jakub Narębski; +Cc: Git List, Jacob Keller
In-Reply-To: <ce2862d5-874b-f244-f9b3-f74e18f7ad42@gmail.com>
On Sat, Nov 19, 2016 at 3:04 AM, Jakub Narębski <jnareb@gmail.com> wrote:
> W dniu 08.11.2016 o 21:12, Karthik Nayak pisze:
>>
>> Helped-by: Junio C Hamano <gitster@pobox.com>
>> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
>> ---
> [...]
>
>> +test_expect_success 'Add symbolic ref for the following tests' '
>> + git symbolic-ref refs/heads/sym refs/heads/master
>> +'
>> +
>> +cat >expected <<EOF
>> +refs/heads/master
>> +EOF
>
> This should be inside the relevant test, not outside. In other
> patches in this series you are putting setup together with the
> rest of test, by using "cat >expected <<-\EOF".
>
Ah! That's because I was just trying to keep it consistent. These tests
are added to t6300, where the `expected` block is usually outside the tests
themselves.
The other tests in the series are added to t6302, where we keep the `expected`
block within the tests themselves.
>> +
>> +test_expect_success 'Verify usage of %(symref) atom' '
>> + git for-each-ref --format="%(symref)" refs/heads/sym > actual &&
>
> This should be spelled " >actual", rather than " > actual"; there
> should be no space between redirection and file name.
>
>> + test_cmp expected actual
>> +'
>> +
>> +cat >expected <<EOF
>> +heads/master
>> +EOF
>> +
>> +test_expect_success 'Verify usage of %(symref:short) atom' '
>> + git for-each-ref --format="%(symref:short)" refs/heads/sym > actual &&
>> + test_cmp expected actual
>> +'
>
> Same here.
>
Will remove the space between '>' and 'actual', Thanks.
--
Regards,
Karthik Nayak
^ permalink raw reply
* Re: [PATCH v7 03/17] ref-filter: implement %(if:equals=<string>) and %(if:notequals=<string>)
From: Karthik Nayak @ 2016-11-20 7:23 UTC (permalink / raw)
To: Jakub Narębski; +Cc: Git List, Jacob Keller
In-Reply-To: <37c2cbf2-7160-49d7-f8f1-3b65d9ecf9ec@gmail.com>
On Sat, Nov 19, 2016 at 1:28 AM, Jakub Narębski <jnareb@gmail.com> wrote:
> W dniu 08.11.2016 o 21:11, Karthik Nayak pisze:
>> From: Karthik Nayak <karthik.188@gmail.com>
>>
>> Implement %(if:equals=<string>) wherein the if condition is only
>> satisfied if the value obtained between the %(if:...) and %(then) atom
>> is the same as the given '<string>'.
>>
>> Similarly, implement (if:notequals=<string>) wherein the if condition
>> is only satisfied if the value obtained between the %(if:...) and
>> %(then) atom is differnt from the given '<string>'.
> ^^^^^^^^
>
> s/differnt/different/ <-- typo
>
Will change that.
>>
>> Add tests and Documentation for the same.
>>
>> Mentored-by: Christian Couder <christian.couder@gmail.com>
>> Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>> ---
>> Documentation/git-for-each-ref.txt | 3 +++
>> ref-filter.c | 43 +++++++++++++++++++++++++++++++++-----
>> t/t6302-for-each-ref-filter.sh | 18 ++++++++++++++++
>> 3 files changed, 59 insertions(+), 5 deletions(-)
>>
>> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
>> index fed8126..b7b8560 100644
>> --- a/Documentation/git-for-each-ref.txt
>> +++ b/Documentation/git-for-each-ref.txt
>> @@ -155,6 +155,9 @@ if::
>> evaluating the string before %(then), this is useful when we
>> use the %(HEAD) atom which prints either "*" or " " and we
>> want to apply the 'if' condition only on the 'HEAD' ref.
>
> So %(if) is actually %(if:notempty) ? Just kidding.
>
It's not a bug, it's a feature ;)
>> + Append ":equals=<string>" or ":notequals=<string>" to compare
>> + the value between the %(if:...) and %(then) atoms with the
>> + given string.
>>
>> In addition to the above, for commit and tag objects, the header
>> field names (`tree`, `parent`, `object`, `type`, and `tag`) can
>> diff --git a/ref-filter.c b/ref-filter.c
>> index 8392303..44481c3 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -22,6 +22,8 @@ struct align {
>> };
>>
>> struct if_then_else {
>> + const char *if_equals,
>> + *not_equals;
>
> I guess using anonymous structs from C11 here...
>
>> unsigned int then_atom_seen : 1,
>> else_atom_seen : 1,
>> condition_satisfied : 1;
>> @@ -49,6 +51,10 @@ static struct used_atom {
>> enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, C_SUB } option;
>> unsigned int nlines;
>> } contents;
>> + struct {
>> + const char *if_equals,
>> + *not_equals;
>> + } if_then_else;
>
> ...to avoid code duplication there is rather out of question?
>
I believe it holds better context without the use of anonymous structs/unions.
But that's my perspective, I wouldn't mind changing it.
>> enum { O_FULL, O_SHORT } objectname;
>> } u;
>> } *used_atom;
>> @@ -169,6 +175,19 @@ static void align_atom_parser(struct used_atom *atom, const char *arg)
>> string_list_clear(¶ms, 0);
>> }
>>
>> +static void if_atom_parser(struct used_atom *atom, const char *arg)
>> +{
>> + if (!arg)
>> + return;
>> + else if (skip_prefix(arg, "equals=", &atom->u.if_then_else.if_equals))
>> + ;
>> + else if (skip_prefix(arg, "notequals=", &atom->u.if_then_else.not_equals))
>> + ;
>
> Those ';' should be perfectly aligned, isn't it?
>
This should be dropped with the new changes made on this patch.
--
Regards,
Karthik Nayak
^ permalink raw reply
* Re: [PATCH v7 00/17] port branch.c to use ref-filter's printing options
From: Karthik Nayak @ 2016-11-20 7:08 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List, Jacob Keller
In-Reply-To: <xmqqd1hsl5zm.fsf@gitster.mtv.corp.google.com>
On Sat, Nov 19, 2016 at 5:01 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> Thanks, will add it in.
>
> OK, here is a reroll of what I sent earlier in
>
> http://public-inbox.org/git/<xmqq7f84tqa7.fsf_-_@gitster.mtv.corp.google.com>
>
> but rebased so that it can happen as a preparatory bugfix before
> your series.
>
> The bug dates back to the very original implementation of %(HEAD) in
> 7a48b83219 ("for-each-ref: introduce %(HEAD) asterisk marker",
> 2013-11-18) and was moved to the current location in the v2.6 days
> at c95b758587 ("ref-filter: move code from 'for-each-ref'",
> 2015-06-14).
>
I'll rebase on this patch. Thanks for your efforts.
I assume you'll be merging it in before my series, so I wont be making
it a part of my series.
--
Regards,
Karthik Nayak
^ permalink raw reply
* Prereleases of Git for Windows
From: Johannes Schindelin @ 2016-11-19 15:10 UTC (permalink / raw)
To: git-for-windows; +Cc: git
Hi all,
I debated whether I should clutter the mailing list by announcing the
prereleases I published based on the v2.11.0-rc* releases of upstream Git.
I ended up deciding to announce that I won't announce them on the mailing
list, but only on Twitter [*1*]. After announcing the latest prerelease,
of course:
https://github.com/git-for-windows/git/releases/tag/v2.11.0-rc2.windows.1
Please give this a good beating and open tickets for issues you encounter
(unless you find that there are already open or closed tickets for the
same bug).
Thank you,
Johannes
Footnote: https://twitter.com/GitForWindows
^ permalink raw reply
* [PATCH v2 2/2] ref-filter: add support to display trailers as part of contents
From: Jacob Keller @ 2016-11-19 0:58 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jacob Keller
In-Reply-To: <20161119005815.3646-1-jacob.e.keller@intel.com>
From: Jacob Keller <jacob.keller@gmail.com>
Add %(trailers) and %(contents:trailers) to display the trailers as
interpreted by trailer_info_get. Update documentation and add a test for
the new feature.
Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
Documentation/git-for-each-ref.txt | 2 ++
ref-filter.c | 22 +++++++++++++++++++++-
t/t6300-for-each-ref.sh | 26 ++++++++++++++++++++++++++
3 files changed, 49 insertions(+), 1 deletion(-)
diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index f57e69bc83e3..e5807eede787 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -165,6 +165,8 @@ of all lines of the commit message up to the first blank line. The next
line is 'contents:body', where body is all of the lines after the first
blank line. The optional GPG signature is `contents:signature`. The
first `N` lines of the message is obtained using `contents:lines=N`.
+Additionally, the trailers as interpreted by linkgit:git-interpret-trailers[1]
+are obtained as 'contents:trailers'.
For sorting purposes, fields with numeric values sort in numeric order
(`objectsize`, `authordate`, `committerdate`, `creatordate`, `taggerdate`).
diff --git a/ref-filter.c b/ref-filter.c
index d4c2931f3aab..b6f1bb73ed37 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -13,6 +13,7 @@
#include "utf8.h"
#include "git-compat-util.h"
#include "version.h"
+#include "trailer.h"
typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
@@ -40,7 +41,7 @@ static struct used_atom {
enum { RR_NORMAL, RR_SHORTEN, RR_TRACK, RR_TRACKSHORT }
remote_ref;
struct {
- enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, C_SUB } option;
+ enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, C_SUB, C_TRAILERS } option;
unsigned int nlines;
} contents;
enum { O_FULL, O_SHORT } objectname;
@@ -85,6 +86,13 @@ static void subject_atom_parser(struct used_atom *atom, const char *arg)
atom->u.contents.option = C_SUB;
}
+static void trailers_atom_parser(struct used_atom *atom, const char *arg)
+{
+ if (arg)
+ die(_("%%(trailers) does not take arguments"));
+ atom->u.contents.option = C_TRAILERS;
+}
+
static void contents_atom_parser(struct used_atom *atom, const char *arg)
{
if (!arg)
@@ -95,6 +103,8 @@ static void contents_atom_parser(struct used_atom *atom, const char *arg)
atom->u.contents.option = C_SIG;
else if (!strcmp(arg, "subject"))
atom->u.contents.option = C_SUB;
+ else if (!strcmp(arg, "trailers"))
+ atom->u.contents.option = C_TRAILERS;
else if (skip_prefix(arg, "lines=", &arg)) {
atom->u.contents.option = C_LINES;
if (strtoul_ui(arg, 10, &atom->u.contents.nlines))
@@ -194,6 +204,7 @@ static struct {
{ "creatordate", FIELD_TIME },
{ "subject", FIELD_STR, subject_atom_parser },
{ "body", FIELD_STR, body_atom_parser },
+ { "trailers", FIELD_STR, trailers_atom_parser },
{ "contents", FIELD_STR, contents_atom_parser },
{ "upstream", FIELD_STR, remote_ref_atom_parser },
{ "push", FIELD_STR, remote_ref_atom_parser },
@@ -785,6 +796,7 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj
name++;
if (strcmp(name, "subject") &&
strcmp(name, "body") &&
+ strcmp(name, "trailers") &&
!starts_with(name, "contents"))
continue;
if (!subpos)
@@ -808,6 +820,14 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj
/* Size is the length of the message after removing the signature */
append_lines(&s, subpos, contents_end - subpos, atom->u.contents.nlines);
v->s = strbuf_detach(&s, NULL);
+ } else if (atom->u.contents.option == C_TRAILERS) {
+ struct trailer_info info;
+
+ /* Search for trailer info */
+ trailer_info_get(&info, subpos);
+ v->s = xmemdupz(info.trailer_start,
+ info.trailer_end - info.trailer_start);
+ trailer_info_release(&info);
} else if (atom->u.contents.option == C_BARE)
v->s = xstrdup(subpos);
}
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 19a2823025e7..eb4bac0fe477 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -553,4 +553,30 @@ test_expect_success 'Verify sort with multiple keys' '
refs/tags/bogo refs/tags/master > actual &&
test_cmp expected actual
'
+
+cat >trailers <<EOF
+Reviewed-by: A U Thor <author@example.com>
+Signed-off-by: A U Thor <author@example.com>
+EOF
+
+test_expect_success 'basic atom: head contents:trailers' '
+ echo "Some contents" > two &&
+ git add two &&
+ git commit -F - <<-EOF &&
+ trailers: this commit message has trailers
+
+ Some message contents
+
+ $(cat trailers)
+ EOF
+ git for-each-ref --format="%(contents:trailers)" refs/heads/master >actual &&
+ sanitize_pgp <actual >actual.clean &&
+ # git for-each-ref ends with a blank line
+ cat >expect <<-EOF &&
+ $(cat trailers)
+
+ EOF
+ test_cmp expect actual.clean
+'
+
test_done
--
2.11.0.rc2.152.g4d04e67
^ permalink raw reply related
* [PATCH v2 1/2] pretty: add %(trailers) format for displaying trailers of a commit message
From: Jacob Keller @ 2016-11-19 0:58 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jacob Keller
In-Reply-To: <20161119005815.3646-1-jacob.e.keller@intel.com>
From: Jacob Keller <jacob.keller@gmail.com>
Recent patches have expanded on the trailers.c code and we have the
builtin commant git-interpret-trailers which can be used to add or
modify trailer lines. However, there is no easy way to simply display
the trailers of a commit message.
Add support for %(trailers) format modifier which will use the
trailer_info_get() calls to read trailers in an identical way as git
interpret-trailers does. Use a long format option instead of a short
name so that future work can more easily unify ref-filter and pretty
formats.
Add documentation and tests for the same.
Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
Documentation/pretty-formats.txt | 2 ++
pretty.c | 17 +++++++++++++++++
t/t4205-log-pretty-formats.sh | 26 ++++++++++++++++++++++++++
3 files changed, 45 insertions(+)
diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 3bcee2ddb124..47b286b33e4e 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -199,6 +199,8 @@ endif::git-rev-list[]
than given and there are spaces on its left, use those spaces
- '%><(<N>)', '%><|(<N>)': similar to '% <(<N>)', '%<|(<N>)'
respectively, but padding both sides (i.e. the text is centered)
+-%(trailers): display the trailers of the body as interpreted by
+ linkgit:git-interpret-trailers[1]
NOTE: Some placeholders may depend on other options given to the
revision traversal engine. For example, the `%g*` reflog options will
diff --git a/pretty.c b/pretty.c
index 37b2c3b1f995..5e683830d9d6 100644
--- a/pretty.c
+++ b/pretty.c
@@ -10,6 +10,7 @@
#include "color.h"
#include "reflog-walk.h"
#include "gpg-interface.h"
+#include "trailer.h"
static char *user_format;
static struct cmt_fmt_map {
@@ -889,6 +890,16 @@ const char *format_subject(struct strbuf *sb, const char *msg,
return msg;
}
+static void format_trailers(struct strbuf *sb, const char *msg)
+{
+ struct trailer_info info;
+
+ trailer_info_get(&info, msg);
+ strbuf_add(sb, info.trailer_start,
+ info.trailer_end - info.trailer_start);
+ trailer_info_release(&info);
+}
+
static void parse_commit_message(struct format_commit_context *c)
{
const char *msg = c->message + c->message_off;
@@ -1292,6 +1303,12 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
strbuf_addstr(sb, msg + c->body_off);
return 1;
}
+
+ if (starts_with(placeholder, "(trailers)")) {
+ format_trailers(sb, msg + c->subject_off);
+ return strlen("(trailers)");
+ }
+
return 0; /* unknown placeholder */
}
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index f5435fd250ba..21eb8c8587f2 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -535,4 +535,30 @@ test_expect_success 'clean log decoration' '
test_cmp expected actual1
'
+cat >trailers <<EOF
+Signed-off-by: A U Thor <author@example.com>
+Acked-by: A U Thor <author@example.com>
+[ v2 updated patch description ]
+Signed-off-by: A U Thor <author@example.com>
+EOF
+
+test_expect_success 'pretty format %(trailers) shows trailers' '
+ echo "Some contents" >trailerfile &&
+ git add trailerfile &&
+ git commit -F - <<-EOF &&
+ trailers: this commit message has trailers
+
+ This commit is a test commit with trailers at the end. We parse this
+ message and display the trailers using %bT
+
+ $(cat trailers)
+ EOF
+ git log --no-walk --pretty="%(trailers)" >actual &&
+ cat >expect <<-EOF &&
+ $(cat trailers)
+
+ EOF
+ test_cmp expect actual
+'
+
test_done
--
2.11.0.rc2.152.g4d04e67
^ permalink raw reply related
* [PATCH 0/2] add format specifiers to display trailers
From: Jacob Keller @ 2016-11-19 0:58 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jacob Keller
From: Jacob Keller <jacob.keller@gmail.com>
This is based off of jt/use-trailer-api-in-commands so that we can make
use of the public trailer API that will parse a string for trailers.
I use trailers as a way to store extra commit metadata, and would like a
convenient way to obtain the trailers of a commit message easily. This
adds format specifiers to both the ref-filter API and the pretty
format specifiers, using %(trailers) for both (and also
contents:trailers for ref-filter).
Additionally, I am somewhat not a fan of the way that if you have a
series of trailers which are trailer format, but not recognized, such
as the following:
<text>
My-tag: my value
My-other-tag: my other value
[non-trailer line]
My-tag: my third value
Git interpret-trailers will not recognize this as a trailer block
because it doesn't have any standard git tags within it.
Junio suggested that we should treat all the configured trailer prefixes
as recognized so that it would work as well, but it doesn't appear to
do this at least for jt/use-trailer-api-in-commands
I think that's the right solution, since it's extensible, though it
would mean that interpret-trailers would behave differently on different
systems... not really sure it's all bad though.
interdiff v1:
diff --git c/Documentation/pretty-formats.txt w/Documentation/pretty-formats.txt
index 9ee68a4cb64a..47b286b33e4e 100644
--- c/Documentation/pretty-formats.txt
+++ w/Documentation/pretty-formats.txt
@@ -138,7 +138,6 @@ The placeholders are:
- '%s': subject
- '%f': sanitized subject line, suitable for a filename
- '%b': body
-- '%bT': trailers of body as interpreted by linkgit:git-interpret-trailers[1]
- '%B': raw body (unwrapped subject and body)
ifndef::git-rev-list[]
- '%N': commit notes
@@ -200,6 +199,8 @@ endif::git-rev-list[]
than given and there are spaces on its left, use those spaces
- '%><(<N>)', '%><|(<N>)': similar to '% <(<N>)', '%<|(<N>)'
respectively, but padding both sides (i.e. the text is centered)
+-%(trailers): display the trailers of the body as interpreted by
+ linkgit:git-interpret-trailers[1]
NOTE: Some placeholders may depend on other options given to the
revision traversal engine. For example, the `%g*` reflog options will
diff --git c/pretty.c w/pretty.c
index ea8764334865..5e683830d9d6 100644
--- c/pretty.c
+++ w/pretty.c
@@ -1300,16 +1300,15 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
format_sanitized_subject(sb, msg + c->subject_off);
return 1;
case 'b': /* body */
- switch (placeholder[1]) {
- case 'T':
- format_trailers(sb, msg + c->subject_off);
- return 2;
- default:
- break;
- }
strbuf_addstr(sb, msg + c->body_off);
return 1;
}
+
+ if (starts_with(placeholder, "(trailers)")) {
+ format_trailers(sb, msg + c->subject_off);
+ return strlen("(trailers)");
+ }
+
return 0; /* unknown placeholder */
}
diff --git c/t/t4205-log-pretty-formats.sh w/t/t4205-log-pretty-formats.sh
index 7a35941ddcbd..21eb8c8587f2 100755
--- c/t/t4205-log-pretty-formats.sh
+++ w/t/t4205-log-pretty-formats.sh
@@ -542,7 +542,7 @@ Acked-by: A U Thor <author@example.com>
Signed-off-by: A U Thor <author@example.com>
EOF
-test_expect_success 'pretty format %bT shows trailers' '
+test_expect_success 'pretty format %(trailers) shows trailers' '
echo "Some contents" >trailerfile &&
git add trailerfile &&
git commit -F - <<-EOF &&
@@ -553,7 +553,7 @@ test_expect_success 'pretty format %bT shows trailers' '
$(cat trailers)
EOF
- git log --no-walk --pretty="%bT" >actual &&
+ git log --no-walk --pretty="%(trailers)" >actual &&
cat >expect <<-EOF &&
$(cat trailers)
Jacob Keller (2):
pretty: add %bT format for displaying trailers of a commit message
ref-filter: add support to display trailers as part of contents
Documentation/git-for-each-ref.txt | 2 ++
Documentation/pretty-formats.txt | 1 +
pretty.c | 18 ++++++++++++++++++
ref-filter.c | 22 +++++++++++++++++++++-
t/t4205-log-pretty-formats.sh | 26 ++++++++++++++++++++++++++
t/t6300-for-each-ref.sh | 26 ++++++++++++++++++++++++++
6 files changed, 94 insertions(+), 1 deletion(-)
--
2.11.0.rc2.152.g4d04e67
^ permalink raw reply related
* Re: [PATCH 0/2] add format specifiers to display trailers
From: Jacob Keller @ 2016-11-18 23:42 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jacob Keller, Jonathan Tan, Git mailing list
In-Reply-To: <xmqq8tsgl5o4.fsf@gitster.mtv.corp.google.com>
On Fri, Nov 18, 2016 at 3:38 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jacob Keller <jacob.e.keller@intel.com> writes:
>
>> Git interpret-trailers will not recognize this as a trailer block
>> because it doesn't have any standard git tags within it. Would it be ok
>> to augment the trailer interpretation to say that if we have over 75%
>> trailers in the block that we accept it even if it doesn't have any real
>> recognized tags?
>
> I thought the documented way to do this is to configure one of your
> custom trailer as such. Jonathan?
>
That would be fine then, if that works.
>> pretty: add %bT format for displaying trailers of a commit message
>
> Are %(...) taken already? In longer term, it would be nice if we
> can unify the --pretty formats and for-each-ref formats, so it is
> probably better if we avoid adding any new short ones to the former.
>
Oh, I hadn't considered adding a longer one. I'll rework this to use
longer ones.
> We have %s and %b so that we can reconstruct the whole thing by
> using both. It is unclear how %bT fits in this picture. I wonder
> if we also need another placeholder that expands to the body of the
> message without the trailer---otherwise the whole set would become
> incoherent, no?
>
I'm not entirely sure what to do here. I just wanted a way to easily
format "just the trailers" of a message. We could add something that
formats just the non-trailers, that's not too difficult. Not really
sure what I'd call it though.
Thanks,
Jake
^ permalink raw reply
* Fwd: git diff with “--word-diff-regex” extremely slow compared to “--word-diff”?
From: Matthieu S @ 2016-11-18 23:40 UTC (permalink / raw)
To: git
In-Reply-To: <CAEYvigJ14xYDmRG2N0yTgM4spaaB7s9923w0+e9+QQEeFz0NTQ@mail.gmail.com>
Hi
When giving a custom regex to git diff --word-diff-regex= instead of
using the default --word-diff (which splits words on whitespace), git
slows down very considerably... I don't understand why such a speed
difference?
(this question was asked on stack overflow, but after two month
without answer, I'm asking it here instead. Post:
http://stackoverflow.com/questions/39027864/git-diff-with-word-diff-regex-extremely-slow-compared-to-word-diff).
Example (sorry, UNIX specific code): create two one-line files, and
two 200000-lines files:
echo aaa,bbb ,12,12,15 >file1.txt
echo aaa,bbb ,12,12,16 >file2.txt
awk '{for(i=0;i<200000;i++)print}' file1.txt > file1BIG.txt
awk '{for(i=0;i<200000;i++)print}' file2.txt > file2BIG.txt
Default --word-diff has no issues with the BIG files (cannot see time
difference):
git diff --word-diff file1.txt file2.txt
git diff --word-diff file1BIG.txt file2BIG.txt
Now use instead --word-diff-regex= argument (with regex from post:
http://stackoverflow.com/questions/10482773/also-use-comma-as-a-word-separator-in-diff
)
git diff --word-diff-regex=[^[:space:],] file1.txt file2.txt
git diff --word-diff-regex=[^[:space:],] file1BIG.txt file2BIG.txt
Why is the speed so different if one uses --word-diff instead of
--word-diff-regex= ? Is it just because my expression is (slightly)
more complex than the default one (split on period instead of only
whitespace) ? Or is it that the default word-diff is implemented
differently/more efficiently? How can I overcome this speed slowdown?
Thanks!!
Matthieu
PS: using git 2.7.4 on Ubuntu 16.04
^ permalink raw reply
* Re: [PATCH 13/16] submodule: teach unpack_trees() to update submodules
From: Stefan Beller @ 2016-11-18 23:39 UTC (permalink / raw)
To: Brandon Williams
Cc: git@vger.kernel.org, Junio C Hamano, Jonathan Nieder, Martin Fick,
David Turner
In-Reply-To: <20161116002520.GI66382@google.com>
On Tue, Nov 15, 2016 at 4:25 PM, Brandon Williams <bmwill@google.com> wrote:
> On 11/15, Stefan Beller wrote:
>> + int flags = CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE;
>
> For readability you may want to have spaces between the two flags
done
>
>> + if (o->index_only
>> + || (!((old->ce_flags & CE_VALID) || ce_skip_worktree(old))
>> + && (o->reset || ce_uptodate(old))))
>> + return 0;
>
> The coding guidelines say that git prefers to have the logical operators
> on the right side like this:
fixed all the coding style issues.
^ permalink raw reply
* Re: [PATCH 0/2] add format specifiers to display trailers
From: Junio C Hamano @ 2016-11-18 23:38 UTC (permalink / raw)
To: Jacob Keller, Jonathan Tan; +Cc: git, Jacob Keller
In-Reply-To: <20161118230825.20952-1-jacob.e.keller@intel.com>
Jacob Keller <jacob.e.keller@intel.com> writes:
> Git interpret-trailers will not recognize this as a trailer block
> because it doesn't have any standard git tags within it. Would it be ok
> to augment the trailer interpretation to say that if we have over 75%
> trailers in the block that we accept it even if it doesn't have any real
> recognized tags?
I thought the documented way to do this is to configure one of your
custom trailer as such. Jonathan?
> pretty: add %bT format for displaying trailers of a commit message
Are %(...) taken already? In longer term, it would be nice if we
can unify the --pretty formats and for-each-ref formats, so it is
probably better if we avoid adding any new short ones to the former.
We have %s and %b so that we can reconstruct the whole thing by
using both. It is unclear how %bT fits in this picture. I wonder
if we also need another placeholder that expands to the body of the
message without the trailer---otherwise the whole set would become
incoherent, no?
^ permalink raw reply
* Re: [PATCH 13/16] submodule: teach unpack_trees() to update submodules
From: Stefan Beller @ 2016-11-18 23:33 UTC (permalink / raw)
To: David Turner
Cc: git@vger.kernel.org, bmwill@google.com, gitster@pobox.com,
jrnieder@gmail.com, mogulguy10@gmail.com
In-Reply-To: <f54d446aa7734cb4aec4b51c7b81a2b6@exmbdft7.ad.twosigma.com>
On Tue, Nov 15, 2016 at 4:22 PM, David Turner <David.Turner@twosigma.com> wrote:
>> msgs[ERROR_NOT_UPTODATE_DIR] =
>> _("Updating the following directories would lose untracked
>> files in it:\n%s");
>> + msgs[ERROR_NOT_UPTODATE_SUBMODULE] =
>> + _("Updating the following submodules would lose modifications
>> in
>> +it:\n%s");
>
> s/it/them/
done, also fixed the existing ERROR_NOT_UPTODATE_DIR.
>> + if (!S_ISGITLINK(ce->ce_mode)) {
>
> I generally prefer to avoid if (!x) { A } else { B } -- I would rather just see if (x) { B } else { A }.
done.
>> + if (submodule_is_interesting(old->name, null_sha1)
>> + && ok_to_remove_submodule(old->name))
>> + return 0;
>> + }
>
> Do we need a return 1 in here somewhere? Because otherwise, we fall through and return 0 later.
Otherwise we would fall through and run
if (errno == ENOENT)
return 0;
return o->gently ? -1 :
add_rejected_path(o, error_type, ce->name);
which produces different results than 0?
^ permalink raw reply
* Re: [PATCH v7 00/17] port branch.c to use ref-filter's printing options
From: Junio C Hamano @ 2016-11-18 23:31 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Git List, Jacob Keller
In-Reply-To: <CAOLa=ZQtmQWpFMPa-SD29N7hASHAPp8SGGJsLu+AW_Kv-1LqwA@mail.gmail.com>
Karthik Nayak <karthik.188@gmail.com> writes:
> Thanks, will add it in.
OK, here is a reroll of what I sent earlier in
http://public-inbox.org/git/<xmqq7f84tqa7.fsf_-_@gitster.mtv.corp.google.com>
but rebased so that it can happen as a preparatory bugfix before
your series.
The bug dates back to the very original implementation of %(HEAD) in
7a48b83219 ("for-each-ref: introduce %(HEAD) asterisk marker",
2013-11-18) and was moved to the current location in the v2.6 days
at c95b758587 ("ref-filter: move code from 'for-each-ref'",
2015-06-14).
-- >8 --
Subject: [PATCH] for-each-ref: do not segv with %(HEAD) on an unborn branch
The code to flip between "*" and " " prefixes depending on what
branch is checked out used in --format='%(HEAD)' did not consider
that HEAD may resolve to an unborn branch and dereferenced a NULL.
This will become a lot easier to trigger as the codepath will be
used to reimplement "git branch [--list]" in the future.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
ref-filter.c | 2 +-
t/t6300-for-each-ref.sh | 10 ++++++++++
2 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/ref-filter.c b/ref-filter.c
index bc551a752c..d7e91a78da 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1017,7 +1017,7 @@ static void populate_value(struct ref_array_item *ref)
head = resolve_ref_unsafe("HEAD", RESOLVE_REF_READING,
sha1, NULL);
- if (!strcmp(ref->refname, head))
+ if (head && !strcmp(ref->refname, head))
v->s = "*";
else
v->s = " ";
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 19a2823025..039509a9cb 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -553,4 +553,14 @@ test_expect_success 'Verify sort with multiple keys' '
refs/tags/bogo refs/tags/master > actual &&
test_cmp expected actual
'
+
+test_expect_success 'do not dereference NULL upon %(HEAD) on unborn branch' '
+ test_when_finished "git checkout master" &&
+ git for-each-ref --format="%(HEAD) %(refname:short)" refs/heads/ >actual &&
+ sed -e "s/^\* / /" actual >expect &&
+ git checkout --orphan HEAD &&
+ git for-each-ref --format="%(HEAD) %(refname:short)" refs/heads/ >actual &&
+ test_cmp expect actual
+'
+
test_done
--
2.11.0-rc2-152-gc9ad1dc38a
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox