* Re: [PATCH] avoid pointer arithmetic involving NULL in FLEX_ALLOC_MEM
From: Jeff King @ 2016-10-15 17:13 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List, Junio C Hamano
In-Reply-To: <ccb15072-d949-fc84-ee45-45ba013f53c4@web.de>
On Sat, Oct 15, 2016 at 06:23:11PM +0200, René Scharfe wrote:
> Calculating offsets involving a NULL pointer is undefined. It works in
> practice (for now?), but we should not rely on it. Allocate first and
> then simply refer to the flexible array member by its name instead of
> performing pointer arithmetic up front. The resulting code is slightly
> shorter, easier to read and doesn't rely on undefined behaviour.
Yeah, this NULL computation is pretty nasty. I recall trying to get rid
of it, but I think it is impossible to do so portably while still using
the generic xalloc_flex() helper. I'm not sure why I didn't think of
just inlining it as you do here. It's not that many lines of code, and I
I agree the result is conceptually simpler.
> #define FLEX_ALLOC_MEM(x, flexname, buf, len) do { \
> - (x) = NULL; /* silence -Wuninitialized for offset calculation */ \
> - (x) = xalloc_flex(sizeof(*(x)), (char *)(&((x)->flexname)) - (char *)(x), (buf), (len)); \
> + size_t flex_array_len_ = (len); \
> + (x) = xcalloc(1, st_add3(sizeof(*(x)), flex_array_len_, 1)); \
> + memcpy((void *)(x)->flexname, (buf), flex_array_len_); \
This looks correct. I wondered at first why you bothered with
flex_array_len, but it is to avoid evaluating the "len" parameter
multiple times.
> } while (0)
> #define FLEXPTR_ALLOC_MEM(x, ptrname, buf, len) do { \
> (x) = xalloc_flex(sizeof(*(x)), sizeof(*(x)), (buf), (len)); \
Now that xalloc_flex() has only this one caller remaining, perhaps it
should just be inlined here, too, for simplicity.
-Peff
^ permalink raw reply
* Re: [PATCH v3 07/25] sequencer: completely revamp the "todo" script parsing
From: Jeff King @ 2016-10-15 17:19 UTC (permalink / raw)
To: Torsten Bögershausen
Cc: Johannes Schindelin, git, Jakub Narębski, Johannes Sixt
In-Reply-To: <933b13d6-5f24-c03a-a1a0-712ceb8ddcc8@web.de>
On Sat, Oct 15, 2016 at 07:03:46PM +0200, Torsten Bögershausen wrote:
> sequencer.c:633:14: warning: comparison of constant 2 with expression of type 'const enum todo_command' is always true [-Wtautological-constant-out-of-range-compare]
> if (command < ARRAY_SIZE(todo_command_strings))
> ~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 1 warning generated.
>
> 53f8024e (Johannes Schindelin 2016-10-10 19:25:07 +0200 633) if (command < ARRAY_SIZE(todo_command_strings))
>
Interesting. The compiler is right that this _should_ never happen, but
I think the patch is quite reasonable to be defensive in case the enum
happens to get a value outside of its acceptable range (which is
probably undefined behavior, but...).
I wonder if:
if ((int)command < ARRAY_SIZE(todo_command_strings))
silences the warning (I suppose size_t is probably an even better type,
though obviously it does not matter in practice).
-Peff
^ permalink raw reply
* Re: [PATCH v3 07/25] sequencer: completely revamp the "todo" script parsing
From: Torsten Bögershausen @ 2016-10-15 17:40 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Schindelin, git, Jakub Narębski, Johannes Sixt
In-Reply-To: <20161015171926.qgtvrjcaqwb436hx@sigill.intra.peff.net>
On 15.10.16 19:19, Jeff King wrote:
> On Sat, Oct 15, 2016 at 07:03:46PM +0200, Torsten Bögershausen wrote:
>
>> sequencer.c:633:14: warning: comparison of constant 2 with expression of type 'const enum todo_command' is always true [-Wtautological-constant-out-of-range-compare]
>> if (command < ARRAY_SIZE(todo_command_strings))
>> ~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> 1 warning generated.
>>
>> 53f8024e (Johannes Schindelin 2016-10-10 19:25:07 +0200 633) if (command < ARRAY_SIZE(todo_command_strings))
>>
>
> Interesting. The compiler is right that this _should_ never happen, but
> I think the patch is quite reasonable to be defensive in case the enum
> happens to get a value outside of its acceptable range (which is
> probably undefined behavior, but...).
>
> I wonder if:
>
> if ((int)command < ARRAY_SIZE(todo_command_strings))
>
> silences the warning (I suppose size_t is probably an even better type,
> though obviously it does not matter in practice).
>
> -Peff
>
Both do (silence the warning)
enum may be signed or unsigned, right ?
So the size_t variant seams to be a better choice
^ permalink raw reply
* Re: [PATCH v10 13/14] convert: add filter.<driver>.process option
From: Jeff King @ 2016-10-15 17:41 UTC (permalink / raw)
To: Lars Schneider; +Cc: Jakub Narębski, git, Junio C Hamano
In-Reply-To: <3732E902-2FE9-4C99-B27D-69B9A3FF8639@gmail.com>
On Sat, Oct 15, 2016 at 07:45:48AM -0700, Lars Schneider wrote:
> >> I have to agree that "capability=<capability>" might read a
> >> little bit nicer. However, Peff suggested "<capability>=true"
> >> as his preference and this is absolutely OK with me.
> >
> > From what I remember it was Peff stating that he thinks "<foo>=true"
> > is easier for parsing (it is, but we still need to support the harder
> > way parsing anyway), and offered that "<foo>" is good enough (if less
> > consistent).
I don't mind that much if you want to do it the other way. You are the
one writing the parsing/use code.
> > Also, with "capability=<foo>" we can be more self descriptive,
> > for example "supported-command=<foo>"; though "capability" is good
> > enough for me.
> >
> > For example
> >
> > packet: git> wants=clean
> > packet: git> wants=smudge
> > packet: git> wants=size
> > packet: git> 0000
> > packet: git< supports=clean
> > packet: git< supports=smudge
> > packet: git< 0000
> >
> > Though coming up with good names is hard; and as I said "capability"
> > is good enough; OTOH with "smudge=true" etc. we don't need to come
> > up with good name at all... though I wonder if it is a good thing `\_o,_/
>
> How about this (I borrowed these terms from contract negotiation)?
>
> packet: git> offers=clean
> packet: git> offers=smudge
> packet: git> offers=size
> packet: git> 0000
> packet: git< accepts=clean
> packet: git< accepts=smudge
> packet: git< 0000
>
> @Peff: Would that be OK for you?
Is it always an offers/accepts relationship? Can the response say "you
did not ask about <foo>, but just so you know I support it"?
I cannot think offhand of an example, but at the same time, if you leave
the terms as generic as possible, you do not end up later with words
that do not make sense. It is trading off one problem now (vagueness of
the protocol terms) for a potential one later (words that have a
specific meaning, but one that is not accurate).
I don't have a strong preference, though.
-Peff
^ permalink raw reply
* Re: [PATCH v3 07/25] sequencer: completely revamp the "todo" script parsing
From: Jeff King @ 2016-10-15 17:46 UTC (permalink / raw)
To: Torsten Bögershausen
Cc: Johannes Schindelin, git, Jakub Narębski, Johannes Sixt
In-Reply-To: <d9f4f658-94fb-cb9e-7da8-3a2fac120a9e@web.de>
On Sat, Oct 15, 2016 at 07:40:15PM +0200, Torsten Bögershausen wrote:
> > I wonder if:
> >
> > if ((int)command < ARRAY_SIZE(todo_command_strings))
> >
> > silences the warning (I suppose size_t is probably an even better type,
> > though obviously it does not matter in practice).
> >
> Both do (silence the warning)
>
> enum may be signed or unsigned, right ?
> So the size_t variant seams to be a better choice
Good catch. It technically needs to check the lower bound, too. In
theory, if somebody wanted to add an enum value that is negative, you'd
use a signed cast and check against both 0 and ARRAY_SIZE(). In
practice, that is nonsense for this case, and using an unsigned type
means that any negative values become large, and the check catches them.
-Peff
^ permalink raw reply
* Re: [PATCH v10 13/14] convert: add filter.<driver>.process option
From: Jakub Narębski @ 2016-10-15 19:42 UTC (permalink / raw)
To: Lars Schneider, Jeff King; +Cc: git, Junio C Hamano
In-Reply-To: <3732E902-2FE9-4C99-B27D-69B9A3FF8639@gmail.com>
W dniu 15.10.2016 o 16:45, Lars Schneider pisze:
>> On 12 Oct 2016, at 03:54, Jakub Narębski <jnareb@gmail.com> wrote:
>> W dniu 12.10.2016 o 00:26, Lars Schneider pisze:
>>>> On 09 Oct 2016, at 01:06, Jakub Narębski <jnareb@gmail.com> wrote:
>>>>>
>>>
>>>>> After the filter started
>>>>> Git sends a welcome message ("git-filter-client"), a list of
>>>>> supported protocol version numbers, and a flush packet. Git expects
>>>>> +to read a welcome response message ("git-filter-server") and exactly
>>>>> +one protocol version number from the previously sent list. All further
>>>>> +communication will be based on the selected version. The remaining
>>>>> +protocol description below documents "version=2". Please note that
>>>>> +"version=42" in the example below does not exist and is only there
>>>>> +to illustrate how the protocol would look like with more than one
>>>>> +version.
>>>>> +
>>>>> +After the version negotiation Git sends a list of all capabilities that
>>>>> +it supports and a flush packet. Git expects to read a list of desired
>>>>> +capabilities, which must be a subset of the supported capabilities list,
>>>>> +and a flush packet as response:
>>>>> +------------------------
>>>>> +packet: git> git-filter-client
>>>>> +packet: git> version=2
>>>>> +packet: git> version=42
>>>>> +packet: git> 0000
>>>>> +packet: git< git-filter-server
>>>>> +packet: git< version=2
>>>>> +packet: git> clean=true
>>>>> +packet: git> smudge=true
>>>>> +packet: git> not-yet-invented=true
>>>>> +packet: git> 0000
>>>>> +packet: git< clean=true
>>>>> +packet: git< smudge=true
>>>>> +packet: git< 0000
>>>>
>>>> WARNING: This example is different from description!!!
>>>
>>> Can you try to explain the difference more clearly? I read it multiple
>>> times and I think this is sound.
>>
>> I'm sorry it was *my mistake*. I have read the example exchange wrong.
>>
>> On the other hand that means that I have other comment, which I though
>> was addressed already in v10, namely that not all exchanges ends with
>> flush packet (inconsistency, and I think a bit of lack of extendability).
>
> Well, this part of the protocol is not supposed to be extensible because
> it is supposed to deal *only* with the version number. It needs to keep
> the same structure to ensure forward and backward compatibility.
>
> However, for consistency sake I will add a flush packet.
Thanks. That is one thing I feel quite strongly about.
I can agree that extendability does not matter much here: we can always
change the version number. But there might be some additional information
that filter process wants to send to Git in first exchange, and using
flush-terminated list here means that we don't need to change version
number, assuming that this additional information is advisory.
The consistency means in my opinion that it should be easier to implement
filter scripts.
>>>> In description above the example you have 4-part handshake, not 3-part;
>>>> the filter is described to send list of supported capabilities last
>>>> (a subset of what Git command supports).
>>>
>>> Part 1: Git sends a welcome message...
>>> Part 2: Git expects to read a welcome response message...
>>> Part 3: After the version negotiation Git sends a list of all capabilities...
>>> Part 4: Git expects to read a list of desired capabilities...
>>>
>>> I think example and text match, no?
>>
>> Yes, it does; as I have said already, I have misread the example.
>>
>> Anyway, in some cases 4-way handshake, where Git sends list of
>> supported capabilities first, is better. If the protocol has
>> to prepare something for each of capabilities, and perhaps check
>> those preparation status, it can do it after Git sends what it
>> could need, and before it sends what it does support.
>>
>> Though it looks a bit strange that client (as Git is client here)
>> sends its capabilities first...
>
> Git tells the filter what it can do. Then the filter decides what
> features it supports. I would prefer to keep it that way as I don't
> see a strong advantage for the other way around.
I think the current order is good, no need to change it.
As I said it is better for Git to send capabilities first.
>>>> By the way, now I look at it, the argument for using the
>>>> "<capability>=true" format instead of "capability=<capability>"
>>>> (or "supported-command=<capability>") is weak. The argument for
>>>> using "<variable>=<value>" to make it easier to implement parsing
>>>> is sound, but the argument for "<capability>=true" is weak.
>>>>
>>>> The argument was that with "<capability>=true" one can simply
>>>> parse metadata into hash / dictionary / hashmap, and choose
>>>> response based on that. Hash / hashmap / associative array
>>>> needs different keys, so the reasoning went for "<capability>=true"
>>>> over "capability=<capability>"... but the filter process still
>>>> needs to handle lines with repeating keys, namely "version=<N>"
>>>> lines!
>>>>
>>>> So the argument doesn't hold water IMVHO, and we can choose
>>>> version which reads better / is more natural.
>>>
>>> I have to agree that "capability=<capability>" might read a
>>> little bit nicer. However, Peff suggested "<capability>=true"
>>> as his preference and this is absolutely OK with me.
>>
>> From what I remember it was Peff stating that he thinks "<foo>=true"
>> is easier for parsing (it is, but we still need to support the harder
>> way parsing anyway), and offered that "<foo>" is good enough (if less
>> consistent).
>>
>>> I am happy to change that if a second reviewer shares your
>>> opinion.
>>
>> Also, with "capability=<foo>" we can be more self descriptive,
>> for example "supported-command=<foo>"; though "capability" is good
>> enough for me.
>>
>> For example
>>
>> packet: git> wants=clean
>> packet: git> wants=smudge
>> packet: git> wants=size
>> packet: git> 0000
>> packet: git< supports=clean
>> packet: git< supports=smudge
>> packet: git< 0000
>>
>> Though coming up with good names is hard; and as I said "capability"
>> is good enough; OTOH with "smudge=true" etc. we don't need to come
>> up with good name at all... though I wonder if it is a good thing `\_o,_/
>
> How about this (I borrowed these terms from contract negotiation)?
>
> packet: git> offers=clean
> packet: git> offers=smudge
> packet: git> offers=size
> packet: git> 0000
> packet: git< accepts=clean
> packet: git< accepts=smudge
> packet: git< 0000
>
> @Peff: Would that be OK for you?
I don't feel strongly about it. It could be "<capability>=true", it could
be "<capability>", it could be "capability=<capability>", it could be
something more descriptive.
I guess "<capability>=true" looks a bit strange (would it ever be there
"<capability>=false"?), but it is good enough for me.
One think we can all agree on is that each capability is to be send as
separate packets, and not as space or comma separated list in a single
packet (like for fetch / push).
>>>>> +------------------------
>>>>> +packet: git< status=abort
>>>>> +packet: git< 0000
>>>>> +------------------------
>>>>> +
>>>>> +After the filter has processed a blob it is expected to wait for
>>>>> +the next "key=value" list containing a command. Git will close
>>>>> +the command pipe on exit. The filter is expected to detect EOF
>>>>> +and exit gracefully on its own.
>>>>
>>>> Any "kill filter" solutions should probably be put here.
>>>
>>> Agreed.
>>>
>>>> I guess
>>>> that filter exiting means EOF on its standard output when read
>>>> by Git command, isn't it?
>>>
>>> Yes, but at this point Git is not listening anymore.
>>
>> I think it might be good idea to have here the information about
>> what filter process should do if it needs maybe lengthy closing
>> process, to not hold/stop Git command or to not be killed.
>
> I've added:
>
> "Git will wait until the filter process has stopped."
Thanks. Looks good for me.
I think any advices (like how to handle shutdown in filter without
blocking Git) could be added later, when we have some experience
making and using long-running multi-file filter drivers.
Thank you for your work on this series.
--
Jakub Narębski
^ permalink raw reply
* [PATCH] trailer: mark file-local symbol 'conf_head' static
From: Ramsay Jones @ 2016-10-15 20:57 UTC (permalink / raw)
To: Jonathan Tan; +Cc: Junio C Hamano, GIT Mailing-list
Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
---
Hi Jonathan,
If you need to re-roll your 'jt/trailer-with-cruft' branch, could you
please squash this into the relevant patch. [commit 3fb120de ("trailer:
use list.h for doubly-linked list", 14-10-2016)]
Thanks!
ATB,
Ramsay Jones
trailer.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/trailer.c b/trailer.c
index 1be6ec8..2862a48 100644
--- a/trailer.c
+++ b/trailer.c
@@ -42,7 +42,7 @@ struct arg_item {
struct conf_info conf;
};
-LIST_HEAD(conf_head);
+static LIST_HEAD(conf_head);
static char *separators = ":";
--
2.10.0
^ permalink raw reply related
* Re: [PATCH] convert: mark a file-local symbol static
From: Ramsay Jones @ 2016-10-15 21:01 UTC (permalink / raw)
To: Lars Schneider; +Cc: Junio C Hamano, GIT Mailing-list
In-Reply-To: <A430A9E0-B2A2-4857-8DEA-EBD7AA2C9E29@gmail.com>
On 15/10/16 16:05, Lars Schneider wrote:
>> On 11 Oct 2016, at 16:46, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote:
[snip]
>> -void stop_multi_file_filter(struct child_process *process)
>> +static void stop_multi_file_filter(struct child_process *process)
>
> Done! Do you have some kind of script to detect these things
> automatically or do you read the code that carefully?
Heh, I'm _far_ too lazy to read the code that carefully. :-D
A combination of 'make sparse' and a perl script (originally
posted to the list by Junio) find all of these for me.
ATB,
Ramsay Jones
^ permalink raw reply
* Re: [PATCH] convert: mark a file-local symbol static
From: Lars Schneider @ 2016-10-16 0:15 UTC (permalink / raw)
To: Ramsay Jones; +Cc: Junio C Hamano, GIT Mailing-list
In-Reply-To: <de24ed05-2857-9c17-920f-66770f898f80@ramsayjones.plus.com>
> On 15 Oct 2016, at 14:01, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote:
>
>
>
> On 15/10/16 16:05, Lars Schneider wrote:
>>> On 11 Oct 2016, at 16:46, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote:
> [snip]
>>> -void stop_multi_file_filter(struct child_process *process)
>>> +static void stop_multi_file_filter(struct child_process *process)
>>
>> Done! Do you have some kind of script to detect these things
>> automatically or do you read the code that carefully?
>
> Heh, I'm _far_ too lazy to read the code that carefully. :-D
>
> A combination of 'make sparse' and a perl script (originally
> posted to the list by Junio) find all of these for me.
Interesting. Do you have a link to this script?
Does it generate false positives?
Maybe I can add `make sparse` to the TravisCI build?
Cheers,
Lars
^ permalink raw reply
* Re: [PATCH v10 04/14] run-command: add clean_on_exit_handler
From: Johannes Schindelin @ 2016-10-16 8:03 UTC (permalink / raw)
To: Lars Schneider; +Cc: git, gitster, jnareb, peff
In-Reply-To: <9C686FFB-CB07-445E-B812-97781CAB113D@gmail.com>
Hi Lars,
On Sat, 15 Oct 2016, Lars Schneider wrote:
>
> > On 11 Oct 2016, at 05:12, Johannes Schindelin <johannes.schindelin@gmx.de> wrote:
> >
> > Hi Lars,
> >
> > On Sat, 8 Oct 2016, larsxschneider@gmail.com wrote:
> >
> >> @@ -31,6 +32,15 @@ static void cleanup_children(int sig, int in_signal)
> >> while (children_to_clean) {
> >> struct child_to_clean *p = children_to_clean;
> >> children_to_clean = p->next;
> >> +
> >> + if (p->process && !in_signal) {
> >> + struct child_process *process = p->process;
> >> + if (process->clean_on_exit_handler) {
> >> + trace_printf("trace: run_command: running exit handler for pid %d", p->pid);
> >
> > On Windows, pid_t translates to long long int, resulting in this build
> > error:
> >
> > -- snip --
> > In file included from cache.h:10:0,
> > from run-command.c:1:
> > run-command.c: In function 'cleanup_children':
> > run-command.c:39:18: error: format '%d' expects argument of type 'int', but argument 5 has type 'pid_t {aka long long int}' [-Werror=format=]
> > trace_printf("trace: run_command: running exit handler for pid %d", p->pid);
> > ^
> > trace.h:81:53: note: in definition of macro 'trace_printf'
> > trace_printf_key_fl(TRACE_CONTEXT, __LINE__, NULL, __VA_ARGS__)
> > ^~~~~~~~~~~
> > cc1.exe: all warnings being treated as errors
> > make: *** [Makefile:1987: run-command.o] Error 1
> > -- snap --
> >
> > Maybe use PRIuMAX as we do elsewhere (see output of `git grep
> > printf.*pid`):
> >
> > trace_printf("trace: run_command: running exit handler for pid %"
> > PRIuMAX, (uintmax_t)p->pid);
>
> Thanks for hint! I'll change it!
>
> However, I am building on Win 8.1 with your latest SDK and I cannot
> reproduce the error. Any idea why that might be the case?
Are you building with DEVELOPER=1?
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH v3 07/25] sequencer: completely revamp the "todo" script parsing
From: Johannes Schindelin @ 2016-10-16 8:09 UTC (permalink / raw)
To: Jeff King
Cc: Torsten Bögershausen, git, Jakub Narębski,
Johannes Sixt
In-Reply-To: <20161015174656.fmgk3le2b34nnjpx@sigill.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 1215 bytes --]
Hi,
On Sat, 15 Oct 2016, Jeff King wrote:
> On Sat, Oct 15, 2016 at 07:40:15PM +0200, Torsten Bögershausen wrote:
>
> > > I wonder if:
> > >
> > > if ((int)command < ARRAY_SIZE(todo_command_strings))
> > >
> > > silences the warning (I suppose size_t is probably an even better type,
> > > though obviously it does not matter in practice).
> > >
> > Both do (silence the warning)
> >
> > enum may be signed or unsigned, right ?
> > So the size_t variant seams to be a better choice
>
> Good catch. It technically needs to check the lower bound, too. In
> theory, if somebody wanted to add an enum value that is negative, you'd
> use a signed cast and check against both 0 and ARRAY_SIZE(). In
> practice, that is nonsense for this case, and using an unsigned type
> means that any negative values become large, and the check catches them.
I am pretty certain that I disagree with that warning: enums have been
used as equivalents of ints for a long time, and will be, for a long time
to come.
Given that this test is modified to `if (command < TODO_NOOP)` later, I
hope that you agree that it is not worth the trouble to appease that
compiler overreaction?
Ciao,
Dscho
^ permalink raw reply
* Re: What's cooking in git.git (Oct 2016, #03; Tue, 11)
From: Johannes Schindelin @ 2016-10-16 8:31 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Stefan Beller, git@vger.kernel.org
In-Reply-To: <xmqqzim6zzc7.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Fri, 14 Oct 2016, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> >> >> * sb/submodule-ignore-trailing-slash (2016-10-10) 2 commits
> >> >> (merged to 'next' on 2016-10-11 at e37425ed17)
> >> >> + submodule: ignore trailing slash in relative url
> >> >> + submodule: ignore trailing slash on superproject URL
> >> >>
> >> >> A minor regression fix for "git submodule".
> >> >>
> >> >> Will merge to 'master'.
> >> >
> >> > Going by the bug report, this *may* be more than
> >> > minor and worth merging down to maint as well, eventually.
> >>
> >> The topic was forked at a reasonably old commit so that it can be
> >> merged as far down to maint-2.9 if we wanted to. Which means the
> >> regression was fairly old and fix is not all that urgent as well.
> >
> > And if you merge it to `master` and `maint`,...
>
> I'll mark it as "wait for follow-up fix" in whats-cooking.txt (on
> 'todo' branch) to remind myself not to merge it yet.
May I request your guidance as to your preference how to proceed? Here is
the problem: Stefan's fix uncovered a bug in the way Git for Windows' Bash
hands off "Unix" paths to real Windows programs, such as `git
submodule-helper`: trailing `/.` gets truncated to `/`.
That is, when passing `/c/test/.` to the helper, what arrives is
actually `C:/test/`. So the helper, being expected to cut off trailing
slashes, cuts it off because it does not see the dot.
Note: when passing URLs (`https://repos.com/mine/.`) or when passing
Windows paths (`C:/test/.`), the helper *will* see the dot, and all is
fine.
Unfortunately the behavior of the MSYS2 Bash cannot be altered in that
respect because existing build workflows *depend* on the current behavior.
Please note we did not have that problem previously, as the helper was a
shell script and therefore stayed in the POSIX-emulated realm, no
POSIX->Windows path translation necessary.
Now, we have two tests in t0060 that try to catch changes in this
particular scenario, verifying that only the dots are cut off of paths
like `/a/b/c/.`.
Given that we cannot modify MSYS2 to appease those tests (because we would
break many more important things), and given that you can easily work
around this by using paths native to Windows when using Git interactively
(which the tests cannot do because they target Linux), I am inclined to
change the tests.
Here are the options I see:
A) remove the tests in question
B) mark them as !MINGW instead
C) change just those two tests from using `$PWD` (pseudo-Unix path) to
`$(pwd)` (native path)
I would like to hear your feedback about your preference, but not without
priming you a little bit by detailing my current opinion on the matter:
While I think B) would be the easiest to read, C) would document the
expected behavior better. A) would feel to me like shrugging, i.e. the
lazy, wrong thing to do.
What do you think?
Dscho
^ permalink raw reply
* Re: [PATCH] avoid pointer arithmetic involving NULL in FLEX_ALLOC_MEM
From: René Scharfe @ 2016-10-16 10:06 UTC (permalink / raw)
To: Jeff King; +Cc: Git List, Junio C Hamano
In-Reply-To: <20161015171325.k2jggjezfmhk3tz7@sigill.intra.peff.net>
Am 15.10.2016 um 19:13 schrieb Jeff King:
> On Sat, Oct 15, 2016 at 06:23:11PM +0200, René Scharfe wrote:
>
>> Calculating offsets involving a NULL pointer is undefined. It works in
>> practice (for now?), but we should not rely on it. Allocate first and
>> then simply refer to the flexible array member by its name instead of
>> performing pointer arithmetic up front. The resulting code is slightly
>> shorter, easier to read and doesn't rely on undefined behaviour.
>
> Yeah, this NULL computation is pretty nasty. I recall trying to get rid
> of it, but I think it is impossible to do so portably while still using
> the generic xalloc_flex() helper.
The only way I see is to pass the type to the macro explicitly (because
typeof is an extention), and that would make call sites ugly.
>> #define FLEX_ALLOC_MEM(x, flexname, buf, len) do { \
>> - (x) = NULL; /* silence -Wuninitialized for offset calculation */ \
>> - (x) = xalloc_flex(sizeof(*(x)), (char *)(&((x)->flexname)) - (char *)(x), (buf), (len)); \
>> + size_t flex_array_len_ = (len); \
>> + (x) = xcalloc(1, st_add3(sizeof(*(x)), flex_array_len_, 1)); \
>> + memcpy((void *)(x)->flexname, (buf), flex_array_len_); \
>
> This looks correct. I wondered at first why you bothered with
> flex_array_len, but it is to avoid evaluating the "len" parameter
> multiple times.
Right; we could drop that feature of the original macros and require
users to pass length expressions that don't have side effects -- all of
them already do that anyway. But let's keep it in this round; it just
costs one extra line.
>> } while (0)
>> #define FLEXPTR_ALLOC_MEM(x, ptrname, buf, len) do { \
>> (x) = xalloc_flex(sizeof(*(x)), sizeof(*(x)), (buf), (len)); \
>
> Now that xalloc_flex() has only this one caller remaining, perhaps it
> should just be inlined here, too, for simplicity.
-- >8 --
Subject: [PATCH 2/1] inline xalloc_flex() into FLEXPTR_ALLOC_MEM
Allocate and copy directly in FLEXPTR_ALLOC_MEM and remove the now
unused helper function xalloc_flex(). The resulting code is shorter
and the offset arithmetic is a bit simpler.
Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
git-compat-util.h | 12 +++---------
1 file changed, 3 insertions(+), 9 deletions(-)
diff --git a/git-compat-util.h b/git-compat-util.h
index f964e36..49ca28c 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -856,7 +856,9 @@ static inline void copy_array(void *dst, const void *src, size_t n, size_t size)
memcpy((void *)(x)->flexname, (buf), flex_array_len_); \
} while (0)
#define FLEXPTR_ALLOC_MEM(x, ptrname, buf, len) do { \
- (x) = xalloc_flex(sizeof(*(x)), sizeof(*(x)), (buf), (len)); \
+ size_t flex_array_len_ = (len); \
+ (x) = xcalloc(1, st_add3(sizeof(*(x)), flex_array_len_, 1)); \
+ memcpy((x) + 1, (buf), flex_array_len_); \
(x)->ptrname = (void *)((x)+1); \
} while(0)
#define FLEX_ALLOC_STR(x, flexname, str) \
@@ -864,14 +866,6 @@ static inline void copy_array(void *dst, const void *src, size_t n, size_t size)
#define FLEXPTR_ALLOC_STR(x, ptrname, str) \
FLEXPTR_ALLOC_MEM((x), ptrname, (str), strlen(str))
-static inline void *xalloc_flex(size_t base_len, size_t offset,
- const void *src, size_t src_len)
-{
- unsigned char *ret = xcalloc(1, st_add3(base_len, src_len, 1));
- memcpy(ret + offset, src, src_len);
- return ret;
-}
-
static inline char *xstrdup_or_null(const char *str)
{
return str ? xstrdup(str) : NULL;
--
2.10.1
^ permalink raw reply related
* [GIT PULL] l10n updates for 2.10.0 maint branch
From: Jiang Xin @ 2016-10-16 12:23 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List, Ralf Thielow, Dimitriy Ryazantcev
Hi Junio,
Please pull the following l10n updates for Git 2.10 to the maint branch.
The following changes since commit 9a4b694c539fead26833c2104c1a93d3a2b4c50a:
l10n: zh_CN: review for git v2.10.0 l10n (2016-09-11 21:34:23 +0800)
are available in the git repository at:
git://github.com/git-l10n/git-po tags/l10n-2.10.0-rnd2.4
for you to fetch changes up to 4dc2ce92fa170b797c850e0c17083bc376c01a85:
Merge branch 'russian-l10n' of https://github.com/DJm00n/git-po-ru
(2016-10-16 20:11:41 +0800)
----------------------------------------------------------------
l10n-2.10.0-rnd2.4
----------------------------------------------------------------
Dimitriy Ryazantcev (1):
l10n: ru.po: update Russian translation
Jiang Xin (1):
Merge branch 'russian-l10n' of https://github.com/DJm00n/git-po-ru
Ralf Thielow (2):
l10n: de.po: fix translation of autostash
l10n: de.po: translate 260 new messages
po/de.po | 5182 +++++++++++++++++++++++++++++++++-----------------------------
po/ru.po | 52 +-
2 files changed, 2815 insertions(+), 2419 deletions(-)
--
Jiang Xin
^ permalink raw reply
* Re: [PATCH v3 07/25] sequencer: completely revamp the "todo" script parsing
From: Jeff King @ 2016-10-16 19:42 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Torsten Bögershausen, git, Jakub Narębski,
Johannes Sixt
In-Reply-To: <alpine.DEB.2.20.1610161006080.197091@virtualbox>
On Sun, Oct 16, 2016 at 10:09:12AM +0200, Johannes Schindelin wrote:
> > Good catch. It technically needs to check the lower bound, too. In
> > theory, if somebody wanted to add an enum value that is negative, you'd
> > use a signed cast and check against both 0 and ARRAY_SIZE(). In
> > practice, that is nonsense for this case, and using an unsigned type
> > means that any negative values become large, and the check catches them.
>
> I am pretty certain that I disagree with that warning: enums have been
> used as equivalents of ints for a long time, and will be, for a long time
> to come.
I'm not sure I agree. IIRC, Assigning values outside the range of an enum has
always been fishy according to the standard, and a compiler really is
allowed to allocate a single bit for storage for this enum.
> Given that this test is modified to `if (command < TODO_NOOP)` later, I
> hope that you agree that it is not worth the trouble to appease that
> compiler overreaction?
I don't mind if there are transient warnings on some compilers in the
middle of a series, but I'm not sure when "later" is. The tip of "pu"
has this warning right now when built with clang.
I'm happy to test the TODO_NOOP version against clang (and prepare a
patch on top if it still complains), but that doesn't seem to have
Junio's tree at all yet.
-Peff
^ permalink raw reply
* Re: [PATCH] avoid pointer arithmetic involving NULL in FLEX_ALLOC_MEM
From: Jeff King @ 2016-10-16 19:46 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List, Junio C Hamano
In-Reply-To: <c99474f2-dac3-e42a-5e4a-02464cac3982@web.de>
On Sun, Oct 16, 2016 at 12:06:02PM +0200, René Scharfe wrote:
> > Yeah, this NULL computation is pretty nasty. I recall trying to get rid
> > of it, but I think it is impossible to do so portably while still using
> > the generic xalloc_flex() helper.
>
> The only way I see is to pass the type to the macro explicitly (because
> typeof is an extention), and that would make call sites ugly.
Yep, exactly. I really wish we could use typeof(); there are a couple
things that could be made less error-prone and ugly. But it's not even a
matter of C99; it's a GNU-ism, so I don't think we are even close to
having support for it everywhere.
> > This looks correct. I wondered at first why you bothered with
> > flex_array_len, but it is to avoid evaluating the "len" parameter
> > multiple times.
>
> Right; we could drop that feature of the original macros and require
> users to pass length expressions that don't have side effects -- all of
> them already do that anyway. But let's keep it in this round; it just
> costs one extra line.
Oh, I think it's worth keeping forever, if only because it makes the
macro interface have one fewer gotcha. Spending a line of code on that
is OK with me.
> > Now that xalloc_flex() has only this one caller remaining, perhaps it
> > should just be inlined here, too, for simplicity.
>
> -- >8 --
> Subject: [PATCH 2/1] inline xalloc_flex() into FLEXPTR_ALLOC_MEM
>
> Allocate and copy directly in FLEXPTR_ALLOC_MEM and remove the now
> unused helper function xalloc_flex(). The resulting code is shorter
> and the offset arithmetic is a bit simpler.
Looks good. Thanks for following up.
-Peff
^ permalink raw reply
* Draft of Git Rev News edition 20
From: Christian Couder @ 2016-10-16 20:09 UTC (permalink / raw)
To: git
Cc: Thomas Ferris Nicolaisen, Junio C Hamano, Johannes Schindelin,
Jeff King, Jakub Narebski, Kevin Daudt, Ramkumar Ramachandra,
Stephan Beyer, Dennis Kaarsemaker, Stefan Beller, Johannes Sixt,
Torsten Bögershausen, Eric Sunshine, Philip Oakley,
Andrew Johnson
Hi,
A draft of a new Git Rev News edition is available here:
https://github.com/git/git.github.io/blob/master/rev_news/drafts/edition-20.md
Everyone is welcome to contribute in any section either by editing the
above page on GitHub and sending a pull request, or by commenting on
this GitHub issue:
https://github.com/git/git.github.io/issues/186
You can also reply to this email.
I tried to cc everyone who appears in this edition but maybe I missed
some people, sorry about that.
Thomas and myself plan to publish this edition on Wednesday
October 19.
Thanks,
Christian.
^ permalink raw reply
* link broken on git man page
From: Howard Johnson @ 2016-10-16 20:42 UTC (permalink / raw)
To: git
This link below is broken in the man page. (I'm on Debian Jessie).
GIT(1) Git Manual
GIT(1)
NAME
git - the stupid content tracker
SYNOPSIS
git [--version] [--help] [-C <path>] [-c <name>=<value>]
[--exec-path[=<path>]] [--html-path] [--man-path] [--info-path]
[-p|--paginate|--no-pager] [--no-replace-objects] [--bare]
[--git-dir=<path>] [--work-tree=<path>] [--namespace=<name>]
<command> [<args>]
DESCRIPTION
Git is a fast, scalable, distributed revision control system with an
unusually rich command set that provides both high-level
operations and
full access to internals.
See gittutorial(7) to get started, then see Everyday Git[1] for a
useful minimum set of commands. The Git User’s Manual[2] has a more
in-depth introduction.
After you mastered the basic concepts, you can come back to this
page
to learn what commands Git offers. You can learn more about
individual
Git commands with "git help command". gitcli(7) manual page
gives you
an overview of the command-line command syntax.
Formatted and hyperlinked version of the latest Git
documentation can
be viewed at http://git-htmldocs.googlecode.com/git/git.html.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
^ permalink raw reply
* Re: link broken on git man page
From: Pranit Bauva @ 2016-10-16 21:04 UTC (permalink / raw)
To: Howard Johnson; +Cc: Git List
In-Reply-To: <db083f89-b502-fe61-ce62-e7c4701f8764@BridgeportContractor.com>
Hey Howard,
On Mon, Oct 17, 2016 at 2:12 AM, Howard Johnson
<hwj@bridgeportcontractor.com> wrote:
> This link below is broken in the man page. (I'm on Debian Jessie).
>
>
> GIT(1) Git Manual
> GIT(1)
>
> NAME
> git - the stupid content tracker
>
> SYNOPSIS
> git [--version] [--help] [-C <path>] [-c <name>=<value>]
> [--exec-path[=<path>]] [--html-path] [--man-path] [--info-path]
> [-p|--paginate|--no-pager] [--no-replace-objects] [--bare]
> [--git-dir=<path>] [--work-tree=<path>] [--namespace=<name>]
> <command> [<args>]
>
> DESCRIPTION
> Git is a fast, scalable, distributed revision control system with an
> unusually rich command set that provides both high-level operations
> and
> full access to internals.
>
> See gittutorial(7) to get started, then see Everyday Git[1] for a
> useful minimum set of commands. The Git User’s Manual[2] has a more
> in-depth introduction.
>
> After you mastered the basic concepts, you can come back to this page
> to learn what commands Git offers. You can learn more about
> individual
> Git commands with "git help command". gitcli(7) manual page gives you
> an overview of the command-line command syntax.
>
> Formatted and hyperlinked version of the latest Git documentation can
> be viewed at http://git-htmldocs.googlecode.com/git/git.html.
>
>
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
It is fixed in the recent version 2.10.0 . The new link is
https://git.github.io/htmldocs/git.html .
Regards,
Pranit Bauva
^ permalink raw reply
* Re: link broken on git man page
From: Jeff King @ 2016-10-16 21:17 UTC (permalink / raw)
To: Howard Johnson; +Cc: Jonathan Nieder, git
In-Reply-To: <db083f89-b502-fe61-ce62-e7c4701f8764@BridgeportContractor.com>
On Sun, Oct 16, 2016 at 01:42:12PM -0700, Howard Johnson wrote:
> This link below is broken in the man page. (I'm on Debian Jessie).
> [...]
> Formatted and hyperlinked version of the latest Git documentation can
> be viewed at http://git-htmldocs.googlecode.com/git/git.html.
This is fixed already in f793582 (doc: git-htmldocs.googlecode.com is no
more, 2016-06-22), which is in git v2.9.1. Debian testing/unstable are
at v2.9.3, but Jessie is still on v2.1.4.
I don't know the policy for cherry-picking patches to Debian stable
releases. I think only security fixes are done immediately, but
"important" fixes get rolled into stable updates every few months.
I don't know that this is "important", but it does point to information
that grew stale outside of the release's control, and as a trivial
documentation-only change, it shouldn't cause a regression.
+cc Jonathan, who works on git packaging for Debian.
-Peff
^ permalink raw reply
* Re: [PATCH v10 04/14] run-command: add clean_on_exit_handler
From: Lars Schneider @ 2016-10-16 21:57 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, gitster, jnareb, peff
In-Reply-To: <alpine.DEB.2.20.1610161003040.197091@virtualbox>
> On 16 Oct 2016, at 01:03, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
>
> Hi Lars,
>
> On Sat, 15 Oct 2016, Lars Schneider wrote:
>
>>
>>> On 11 Oct 2016, at 05:12, Johannes Schindelin <johannes.schindelin@gmx.de> wrote:
>>>
>>> On Windows, pid_t translates to long long int, resulting in this build
>>> error:
>>>
>>
>> Thanks for hint! I'll change it!
>>
>> However, I am building on Win 8.1 with your latest SDK and I cannot
>> reproduce the error. Any idea why that might be the case?
>
> Are you building with DEVELOPER=1?
Argh! Of course ... I forgot to add this flag to my config.mak on Windows.
Thanks,
Lars
^ permalink raw reply
* [PATCH v11 00/14] Git filter protocol
From: larsxschneider @ 2016-10-16 23:20 UTC (permalink / raw)
To: git; +Cc: gitster, jnareb, peff, ramsay, tboegi, Lars Schneider
From: Lars Schneider <larsxschneider@gmail.com>
The goal of this series is to avoid launching a new clean/smudge filter
process for each file that is filtered.
A short summary about v1 to v5 can be found here:
https://git.github.io/rev_news/2016/08/17/edition-18/
This series is also published on web:
https://github.com/larsxschneider/git/pull/15
Patches 1 and 2 are cleanups and not strictly necessary for the series.
Patches 3 to 12 are required preparation. Patch 13 is the main patch.
Patch 14 adds an example how to use the Git filter protocol in contrib.
Thanks a lot to
Jakub, Junio, Ramsay, Dscho, Torsten and Peff
for very helpful reviews,
Lars
## Changes since v10
* clarify documentation (Jakub)
* change "<capability>=true" to "capability=<capability>" in protocol (Jakub)
* make stop_multi_file_filter() static (Ramsay)
* add flush packet after version negotiation for consistency (Jakub)
* fix pid_t translation error on Windows (Dscho)
* fix Unquoted references in tests (Junio)
* remove smudge invocation count in tests (Junio)
Lars Schneider (14):
convert: quote filter names in error messages
convert: modernize tests
run-command: move check_pipe() from write_or_die to run_command
run-command: add clean_on_exit_handler
pkt-line: rename packet_write() to packet_write_fmt()
pkt-line: extract set_packet_header()
pkt-line: add packet_write_fmt_gently()
pkt-line: add packet_flush_gently()
pkt-line: add packet_write_gently()
pkt-line: add functions to read/write flush terminated packet streams
convert: make apply_filter() adhere to standard Git error handling
convert: prepare filter.<driver>.process option
convert: add filter.<driver>.process option
contrib/long-running-filter: add long running filter example
Documentation/gitattributes.txt | 157 ++++++++++-
builtin/archive.c | 4 +-
builtin/receive-pack.c | 4 +-
builtin/remote-ext.c | 4 +-
builtin/upload-archive.c | 4 +-
connect.c | 2 +-
contrib/long-running-filter/example.pl | 128 +++++++++
convert.c | 375 +++++++++++++++++++++---
daemon.c | 2 +-
http-backend.c | 2 +-
pkt-line.c | 152 +++++++++-
pkt-line.h | 12 +-
run-command.c | 39 ++-
run-command.h | 4 +-
shallow.c | 2 +-
t/t0021-conversion.sh | 502 ++++++++++++++++++++++++++++++---
t/t0021/rot13-filter.pl | 192 +++++++++++++
upload-pack.c | 30 +-
write_or_die.c | 13 -
19 files changed, 1495 insertions(+), 133 deletions(-)
create mode 100755 contrib/long-running-filter/example.pl
create mode 100755 t/t0021/rot13-filter.pl
## Interdiff (v10..v11)
diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index a182ef2..976243a 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -297,9 +297,11 @@ upon checkin. By default these commands process only a single
blob and terminate. If a long running `process` filter is used
in place of `clean` and/or `smudge` filters, then Git can process
all blobs with a single filter command invocation for the entire
-life of a single Git command, for example `git add --all`. See
-section below for the description of the protocol used to
-communicate with a `process` filter.
+life of a single Git command, for example `git add --all`. If a
+long running `process` filter is configured then it always takes
+precedence over a configured single blob filter. See section
+below for the description of the protocol used to communicate with
+a `process` filter.
One use of the content filtering is to massage the content into a shape
that is more convenient for the platform, filesystem, and the user to use.
@@ -393,10 +395,10 @@ text and therefore are terminated by a LF.
Git starts the filter when it encounters the first file
that needs to be cleaned or smudged. After the filter started
-Git sends a welcome message ("git-filter-client"), a list of
-supported protocol version numbers, and a flush packet. Git expects
-to read a welcome response message ("git-filter-server") and exactly
-one protocol version number from the previously sent list. All further
+Git sends a welcome message ("git-filter-client"), a list of supported
+protocol version numbers, and a flush packet. Git expects to read a welcome
+response message ("git-filter-server"), exactly one protocol version number
+from the previously sent list, and a flush packet. All further
communication will be based on the selected version. The remaining
protocol description below documents "version=2". Please note that
"version=42" in the example below does not exist and is only there
@@ -414,12 +416,13 @@ packet: git> version=42
packet: git> 0000
packet: git< git-filter-server
packet: git< version=2
-packet: git> clean=true
-packet: git> smudge=true
-packet: git> not-yet-invented=true
+packet: git< 0000
+packet: git> capability=clean
+packet: git> capability=smudge
+packet: git> capability=not-yet-invented
packet: git> 0000
-packet: git< clean=true
-packet: git< smudge=true
+packet: git< capability=clean
+packet: git< capability=smudge
packet: git< 0000
------------------------
Supported filter capabilities in version 2 are "clean" and
@@ -428,7 +431,7 @@ Supported filter capabilities in version 2 are "clean" and
Afterwards Git sends a list of "key=value" pairs terminated with
a flush packet. The list will contain at least the filter command
(based on the supported capabilities) and the pathname of the file
-to filter relative to the repository root. Right after these packets
+to filter relative to the repository root. Right after the flush packet
Git sends the content split in zero or more pkt-line packets and a
flush packet to terminate content. Please note, that the filter
must not send any response before it received the content and the
@@ -447,7 +450,10 @@ problems then the list must contain a "success" status. Right after
these packets the filter is expected to send the content in zero
or more pkt-line packets and a flush packet at the end. Finally, a
second list of "key=value" pairs terminated with a flush packet
-is expected. The filter can change the status in the second list.
+is expected. The filter can change the status in the second list
+or keep the status as is with an empty list. Please note that the
+empty list must be terminated with a flush packet regardless.
+
------------------------
packet: git< status=success
packet: git< 0000
@@ -457,7 +463,7 @@ packet: git< 0000 # empty list, keep "status=success" unchanged!
------------------------
If the result content is empty then the filter is expected to respond
-with a "success" status and an empty list.
+with a "success" status and a flush packet to signal the empty content.
------------------------
packet: git< status=success
packet: git< 0000
@@ -466,9 +472,7 @@ packet: git< 0000 # empty list, keep "status=success" unchanged!
------------------------
In case the filter cannot or does not want to process the content,
-it is expected to respond with an "error" status. Depending on the
-`filter.<driver>.required` flag Git will interpret that as error
-but it will not stop or restart the filter process.
+it is expected to respond with an "error" status.
------------------------
packet: git< status=error
packet: git< 0000
@@ -476,9 +480,7 @@ packet: git< 0000
If the filter experiences an error during processing, then it can
send the status "error" after the content was (partially or
-completely) sent. Depending on the `filter.<driver>.required` flag
-Git will interpret that as error but it will not stop or restart the
-filter process.
+completely) sent.
------------------------
packet: git< status=success
packet: git< 0000
@@ -488,31 +490,31 @@ packet: git< status=error
packet: git< 0000
------------------------
-If the filter dies during the communication or does not adhere to
-the protocol then Git will stop the filter process and restart it
-with the next file that needs to be processed. Depending on the
-`filter.<driver>.required` flag Git will interpret that as error.
-
-The error handling for all cases above mimic the behavior of
-the `filter.<driver>.clean` / `filter.<driver>.smudge` error
-handling.
-
In case the filter cannot or does not want to process the content
as well as any future content for the lifetime of the Git process,
-it is expected to respond with an "abort" status at any point in
-the protocol. Depending on the `filter.<driver>.required` flag Git
-will interpret that as error for the content as well as any future
-content for the lifetime of the Git process but it will not stop or
-restart the filter process.
+then it is expected to respond with an "abort" status at any point
+in the protocol.
------------------------
packet: git< status=abort
packet: git< 0000
------------------------
+Git neither stops nor restarts the filter process in case the
+"error"/"abort" status is set. However, Git sets its exit code
+according to the `filter.<driver>.required` flag, mimicking the
+behavior of the `filter.<driver>.clean` / `filter.<driver>.smudge`
+mechanism.
+
+If the filter dies during the communication or does not adhere to
+the protocol then Git will stop the filter process and restart it
+with the next file that needs to be processed. Depending on the
+`filter.<driver>.required` flag Git will interpret that as error.
+
After the filter has processed a blob it is expected to wait for
the next "key=value" list containing a command. Git will close
the command pipe on exit. The filter is expected to detect EOF
-and exit gracefully on its own.
+and exit gracefully on its own. Git will wait until the filter
+process has stopped.
A long running filter demo implementation can be found in
`contrib/long-running-filter/example.pl` located in the Git
@@ -520,10 +522,6 @@ core repository. If you develop your own long running filter
process then the `GIT_TRACE_PACKET` environment variables can be
very helpful for debugging (see linkgit:git[1]).
-If a `filter.<driver>.process` command is configured then it
-always takes precedence over a configured `filter.<driver>.clean`
-or `filter.<driver>.smudge` command.
-
Please note that you cannot use an existing `filter.<driver>.clean`
or `filter.<driver>.smudge` command with `filter.<driver>.process`
because the former two use a different inter process communication
diff --git a/contrib/long-running-filter/example.pl b/contrib/long-running-filter/example.pl
index f4102d2..3945705 100755
--- a/contrib/long-running-filter/example.pl
+++ b/contrib/long-running-filter/example.pl
@@ -70,13 +70,14 @@ sub packet_flush {
packet_txt_write("git-filter-server");
packet_txt_write("version=2");
+packet_flush();
-( packet_txt_read() eq ( 0, "clean=true" ) ) || die "bad capability";
-( packet_txt_read() eq ( 0, "smudge=true" ) ) || die "bad capability";
+( packet_txt_read() eq ( 0, "capability=clean" ) ) || die "bad capability";
+( packet_txt_read() eq ( 0, "capability=smudge" ) ) || die "bad capability";
( packet_bin_read() eq ( 1, "" ) ) || die "bad capability end";
-packet_txt_write("clean=true");
-packet_txt_write("smudge=true");
+packet_txt_write("capability=clean");
+packet_txt_write("capability=smudge");
packet_flush();
while (1) {
diff --git a/convert.c b/convert.c
index 1d89632..9d2aa68 100644
--- a/convert.c
+++ b/convert.c
@@ -567,7 +567,7 @@ static void kill_multi_file_filter(struct hashmap *hashmap, struct cmd2process *
free(entry);
}
-void stop_multi_file_filter(struct child_process *process)
+static void stop_multi_file_filter(struct child_process *process)
{
sigchain_push(SIGPIPE, SIG_IGN);
/* Closing the pipe signals the filter to initiate a shutdown. */
@@ -622,8 +622,11 @@ static struct cmd2process *start_multi_file_filter(struct hashmap *hashmap, cons
err = strcmp(packet_read_line(process->out, NULL), "version=2");
if (err)
goto done;
+ err = packet_read_line(process->out, NULL) != NULL;
+ if (err)
+ goto done;
- err = packet_write_list(process->in, "clean=true", "smudge=true", NULL);
+ err = packet_write_list(process->in, "capability=clean", "capability=smudge", NULL);
for (;;) {
cap_buf = packet_read_line(process->out, NULL);
@@ -631,10 +634,10 @@ static struct cmd2process *start_multi_file_filter(struct hashmap *hashmap, cons
break;
string_list_split_in_place(&cap_list, cap_buf, '=', 1);
- if (cap_list.nr != 2 || strcmp(cap_list.items[1].string, "true"))
+ if (cap_list.nr != 2 || strcmp(cap_list.items[0].string, "capability"))
continue;
- cap_name = cap_list.items[0].string;
+ cap_name = cap_list.items[1].string;
if (!strcmp(cap_name, "clean")) {
entry->supported_capabilities |= CAP_CLEAN;
} else if (!strcmp(cap_name, "smudge")) {
diff --git a/run-command.c b/run-command.c
index e5fd6ff..ca905a9 100644
--- a/run-command.c
+++ b/run-command.c
@@ -36,7 +36,10 @@ static void cleanup_children(int sig, int in_signal)
if (p->process && !in_signal) {
struct child_process *process = p->process;
if (process->clean_on_exit_handler) {
- trace_printf("trace: run_command: running exit handler for pid %d", p->pid);
+ trace_printf(
+ "trace: run_command: running exit handler for pid %"
+ PRIuMAX, (uintmax_t)p->pid
+ );
process->clean_on_exit_handler(process);
}
}
diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
index 9f892c0..a20b9f5 100755
--- a/t/t0021-conversion.sh
+++ b/t/t0021-conversion.sh
@@ -31,38 +31,35 @@ filter_git () {
rm -f git-stderr.log
}
-# Count unique lines in two files and compare them.
-test_cmp_count () {
- for FILE in $@
- do
- sort $FILE | uniq -c | sed "s/^[ ]*//" >$FILE.tmp
- cat $FILE.tmp >$FILE
- done &&
- test_cmp $@
-}
-
-# Count unique lines except clean invocations in two files and compare
-# them. Clean invocations are not counted because their number can vary.
+# Compare two files and ensure that `clean` and `smudge` respectively are
+# called at least once if specified in the `expect` file. The actual
+# invocation count is not relevant because their number can vary.
# c.f. http://public-inbox.org/git/xmqqshv18i8i.fsf@gitster.mtv.corp.google.com/
-test_cmp_count_except_clean () {
- for FILE in $@
+test_cmp_count () {
+ expect=$1
+ actual=$2
+ for FILE in "$expect" "$actual"
do
- sort $FILE | uniq -c | sed "s/^[ ]*//" |
- sed "s/^\([0-9]\) IN: clean/x IN: clean/" >$FILE.tmp
- cat $FILE.tmp >$FILE
+ sort "$FILE" | uniq -c | sed "s/^[ ]*//" |
+ sed "s/^\([0-9]\) IN: clean/x IN: clean/" |
+ sed "s/^\([0-9]\) IN: smudge/x IN: smudge/" >"$FILE.tmp" &&
+ mv "$FILE.tmp" "$FILE"
done &&
- test_cmp $@
+ test_cmp "$expect" "$actual"
}
-# Compare two files but exclude clean invocations because they can vary.
+# Compare two files but exclude all `clean` invocations because Git can
+# call `clean` zero or more times.
# c.f. http://public-inbox.org/git/xmqqshv18i8i.fsf@gitster.mtv.corp.google.com/
test_cmp_exclude_clean () {
- for FILE in $@
+ expect=$1
+ actual=$2
+ for FILE in "$expect" "$actual"
do
- grep -v "IN: clean" $FILE >$FILE.tmp
- cat $FILE.tmp >$FILE
+ grep -v "IN: clean" "$FILE" >"$FILE.tmp" &&
+ mv "$FILE.tmp" "$FILE"
done &&
- test_cmp $@
+ test_cmp "$expect" "$actual"
}
# Check that the contents of two files are equal and that their rot13 version
@@ -395,7 +392,7 @@ test_expect_success PERL 'required process filter should filter data' '
IN: clean testsubdir/test3 '\''sq'\'',\$x.r $S3 [OK] -- OUT: $S3 . [OK]
STOP
EOF
- test_cmp_count_except_clean expected.log rot13-filter.log &&
+ test_cmp_count expected.log rot13-filter.log &&
rm -f test2.r "testsubdir/test3 '\''sq'\'',\$x.r" &&
diff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl
index 1a6959c..ae4c50f 100755
--- a/t/t0021/rot13-filter.pl
+++ b/t/t0021/rot13-filter.pl
@@ -95,13 +95,14 @@ $debug->flush();
packet_txt_write("git-filter-server");
packet_txt_write("version=2");
+packet_flush();
-( packet_txt_read() eq ( 0, "clean=true" ) ) || die "bad capability";
-( packet_txt_read() eq ( 0, "smudge=true" ) ) || die "bad capability";
+( packet_txt_read() eq ( 0, "capability=clean" ) ) || die "bad capability";
+( packet_txt_read() eq ( 0, "capability=smudge" ) ) || die "bad capability";
( packet_bin_read() eq ( 1, "" ) ) || die "bad capability end";
foreach (@capabilities) {
- packet_txt_write( $_ . "=true" );
+ packet_txt_write( "capability=" . $_ );
}
packet_flush();
print $debug "init handshake complete\n";
--
2.10.0
^ permalink raw reply related
* [PATCH v11 01/14] convert: quote filter names in error messages
From: larsxschneider @ 2016-10-16 23:20 UTC (permalink / raw)
To: git; +Cc: gitster, jnareb, peff, ramsay, tboegi, Lars Schneider
In-Reply-To: <20161016232038.84951-1-larsxschneider@gmail.com>
From: Lars Schneider <larsxschneider@gmail.com>
Git filter driver commands with spaces (e.g. `filter.sh foo`) are hard
to read in error messages. Quote them to improve the readability.
Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
convert.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/convert.c b/convert.c
index 077f5e6..986c239 100644
--- a/convert.c
+++ b/convert.c
@@ -412,7 +412,7 @@ static int filter_buffer_or_fd(int in, int out, void *data)
child_process.out = out;
if (start_command(&child_process))
- return error("cannot fork to run external filter %s", params->cmd);
+ return error("cannot fork to run external filter '%s'", params->cmd);
sigchain_push(SIGPIPE, SIG_IGN);
@@ -430,13 +430,13 @@ static int filter_buffer_or_fd(int in, int out, void *data)
if (close(child_process.in))
write_err = 1;
if (write_err)
- error("cannot feed the input to external filter %s", params->cmd);
+ error("cannot feed the input to external filter '%s'", params->cmd);
sigchain_pop(SIGPIPE);
status = finish_command(&child_process);
if (status)
- error("external filter %s failed %d", params->cmd, status);
+ error("external filter '%s' failed %d", params->cmd, status);
strbuf_release(&cmd);
return (write_err || status);
@@ -477,15 +477,15 @@ static int apply_filter(const char *path, const char *src, size_t len, int fd,
return 0; /* error was already reported */
if (strbuf_read(&nbuf, async.out, len) < 0) {
- error("read from external filter %s failed", cmd);
+ error("read from external filter '%s' failed", cmd);
ret = 0;
}
if (close(async.out)) {
- error("read from external filter %s failed", cmd);
+ error("read from external filter '%s' failed", cmd);
ret = 0;
}
if (finish_async(&async)) {
- error("external filter %s failed", cmd);
+ error("external filter '%s' failed", cmd);
ret = 0;
}
--
2.10.0
^ permalink raw reply related
* [PATCH v11 02/14] convert: modernize tests
From: larsxschneider @ 2016-10-16 23:20 UTC (permalink / raw)
To: git; +Cc: gitster, jnareb, peff, ramsay, tboegi, Lars Schneider
In-Reply-To: <20161016232038.84951-1-larsxschneider@gmail.com>
From: Lars Schneider <larsxschneider@gmail.com>
Use `test_config` to set the config, check that files are empty with
`test_must_be_empty`, compare files with `test_cmp`, and remove spaces
after ">" and "<".
Please note that the "rot13" filter configured in "setup" keeps using
`git config` instead of `test_config` because subsequent tests might
depend on it.
Reviewed-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
t/t0021-conversion.sh | 58 +++++++++++++++++++++++++--------------------------
1 file changed, 29 insertions(+), 29 deletions(-)
diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
index e799e59..dc50938 100755
--- a/t/t0021-conversion.sh
+++ b/t/t0021-conversion.sh
@@ -38,8 +38,8 @@ script='s/^\$Id: \([0-9a-f]*\) \$/\1/p'
test_expect_success check '
- cmp test.o test &&
- cmp test.o test.t &&
+ test_cmp test.o test &&
+ test_cmp test.o test.t &&
# ident should be stripped in the repository
git diff --raw --exit-code :test :test.i &&
@@ -47,10 +47,10 @@ test_expect_success check '
embedded=$(sed -ne "$script" test.i) &&
test "z$id" = "z$embedded" &&
- git cat-file blob :test.t > test.r &&
+ git cat-file blob :test.t >test.r &&
- ./rot13.sh < test.o > test.t &&
- cmp test.r test.t
+ ./rot13.sh <test.o >test.t &&
+ test_cmp test.r test.t
'
# If an expanded ident ever gets into the repository, we want to make sure that
@@ -130,7 +130,7 @@ test_expect_success 'filter shell-escaped filenames' '
# delete the files and check them out again, using a smudge filter
# that will count the args and echo the command-line back to us
- git config filter.argc.smudge "sh ./argc.sh %f" &&
+ test_config filter.argc.smudge "sh ./argc.sh %f" &&
rm "$normal" "$special" &&
git checkout -- "$normal" "$special" &&
@@ -141,7 +141,7 @@ test_expect_success 'filter shell-escaped filenames' '
test_cmp expect "$special" &&
# do the same thing, but with more args in the filter expression
- git config filter.argc.smudge "sh ./argc.sh %f --my-extra-arg" &&
+ test_config filter.argc.smudge "sh ./argc.sh %f --my-extra-arg" &&
rm "$normal" "$special" &&
git checkout -- "$normal" "$special" &&
@@ -154,9 +154,9 @@ test_expect_success 'filter shell-escaped filenames' '
'
test_expect_success 'required filter should filter data' '
- git config filter.required.smudge ./rot13.sh &&
- git config filter.required.clean ./rot13.sh &&
- git config filter.required.required true &&
+ test_config filter.required.smudge ./rot13.sh &&
+ test_config filter.required.clean ./rot13.sh &&
+ test_config filter.required.required true &&
echo "*.r filter=required" >.gitattributes &&
@@ -165,17 +165,17 @@ test_expect_success 'required filter should filter data' '
rm -f test.r &&
git checkout -- test.r &&
- cmp test.o test.r &&
+ test_cmp test.o test.r &&
./rot13.sh <test.o >expected &&
git cat-file blob :test.r >actual &&
- cmp expected actual
+ test_cmp expected actual
'
test_expect_success 'required filter smudge failure' '
- git config filter.failsmudge.smudge false &&
- git config filter.failsmudge.clean cat &&
- git config filter.failsmudge.required true &&
+ test_config filter.failsmudge.smudge false &&
+ test_config filter.failsmudge.clean cat &&
+ test_config filter.failsmudge.required true &&
echo "*.fs filter=failsmudge" >.gitattributes &&
@@ -186,9 +186,9 @@ test_expect_success 'required filter smudge failure' '
'
test_expect_success 'required filter clean failure' '
- git config filter.failclean.smudge cat &&
- git config filter.failclean.clean false &&
- git config filter.failclean.required true &&
+ test_config filter.failclean.smudge cat &&
+ test_config filter.failclean.clean false &&
+ test_config filter.failclean.required true &&
echo "*.fc filter=failclean" >.gitattributes &&
@@ -197,8 +197,8 @@ test_expect_success 'required filter clean failure' '
'
test_expect_success 'filtering large input to small output should use little memory' '
- git config filter.devnull.clean "cat >/dev/null" &&
- git config filter.devnull.required true &&
+ test_config filter.devnull.clean "cat >/dev/null" &&
+ test_config filter.devnull.required true &&
for i in $(test_seq 1 30); do printf "%1048576d" 1; done >30MB &&
echo "30MB filter=devnull" >.gitattributes &&
GIT_MMAP_LIMIT=1m GIT_ALLOC_LIMIT=1m git add 30MB
@@ -207,7 +207,7 @@ test_expect_success 'filtering large input to small output should use little mem
test_expect_success 'filter that does not read is fine' '
test-genrandom foo $((128 * 1024 + 1)) >big &&
echo "big filter=epipe" >.gitattributes &&
- git config filter.epipe.clean "echo xyzzy" &&
+ test_config filter.epipe.clean "echo xyzzy" &&
git add big &&
git cat-file blob :big >actual &&
echo xyzzy >expect &&
@@ -215,20 +215,20 @@ test_expect_success 'filter that does not read is fine' '
'
test_expect_success EXPENSIVE 'filter large file' '
- git config filter.largefile.smudge cat &&
- git config filter.largefile.clean cat &&
+ test_config filter.largefile.smudge cat &&
+ test_config filter.largefile.clean cat &&
for i in $(test_seq 1 2048); do printf "%1048576d" 1; done >2GB &&
echo "2GB filter=largefile" >.gitattributes &&
git add 2GB 2>err &&
- ! test -s err &&
+ test_must_be_empty err &&
rm -f 2GB &&
git checkout -- 2GB 2>err &&
- ! test -s err
+ test_must_be_empty err
'
test_expect_success "filter: clean empty file" '
- git config filter.in-repo-header.clean "echo cleaned && cat" &&
- git config filter.in-repo-header.smudge "sed 1d" &&
+ test_config filter.in-repo-header.clean "echo cleaned && cat" &&
+ test_config filter.in-repo-header.smudge "sed 1d" &&
echo "empty-in-worktree filter=in-repo-header" >>.gitattributes &&
>empty-in-worktree &&
@@ -240,8 +240,8 @@ test_expect_success "filter: clean empty file" '
'
test_expect_success "filter: smudge empty file" '
- git config filter.empty-in-repo.clean "cat >/dev/null" &&
- git config filter.empty-in-repo.smudge "echo smudged && cat" &&
+ test_config filter.empty-in-repo.clean "cat >/dev/null" &&
+ test_config filter.empty-in-repo.smudge "echo smudged && cat" &&
echo "empty-in-repo filter=empty-in-repo" >>.gitattributes &&
echo dead data walking >empty-in-repo &&
--
2.10.0
^ permalink raw reply related
* [PATCH v11 03/14] run-command: move check_pipe() from write_or_die to run_command
From: larsxschneider @ 2016-10-16 23:20 UTC (permalink / raw)
To: git; +Cc: gitster, jnareb, peff, ramsay, tboegi, Lars Schneider
In-Reply-To: <20161016232038.84951-1-larsxschneider@gmail.com>
From: Lars Schneider <larsxschneider@gmail.com>
Move check_pipe() to run_command and make it public. This is necessary
to call the function from pkt-line in a subsequent patch.
While at it, make async_exit() static to run_command.c as it is no
longer used from outside.
Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
run-command.c | 17 +++++++++++++++--
run-command.h | 2 +-
write_or_die.c | 13 -------------
3 files changed, 16 insertions(+), 16 deletions(-)
diff --git a/run-command.c b/run-command.c
index 5a4dbb6..3269362 100644
--- a/run-command.c
+++ b/run-command.c
@@ -634,7 +634,7 @@ int in_async(void)
return !pthread_equal(main_thread, pthread_self());
}
-void NORETURN async_exit(int code)
+static void NORETURN async_exit(int code)
{
pthread_exit((void *)(intptr_t)code);
}
@@ -684,13 +684,26 @@ int in_async(void)
return process_is_async;
}
-void NORETURN async_exit(int code)
+static void NORETURN async_exit(int code)
{
exit(code);
}
#endif
+void check_pipe(int err)
+{
+ if (err == EPIPE) {
+ if (in_async())
+ async_exit(141);
+
+ signal(SIGPIPE, SIG_DFL);
+ raise(SIGPIPE);
+ /* Should never happen, but just in case... */
+ exit(141);
+ }
+}
+
int start_async(struct async *async)
{
int need_in, need_out;
diff --git a/run-command.h b/run-command.h
index 5066649..cf29a31 100644
--- a/run-command.h
+++ b/run-command.h
@@ -139,7 +139,7 @@ struct async {
int start_async(struct async *async);
int finish_async(struct async *async);
int in_async(void);
-void NORETURN async_exit(int code);
+void check_pipe(int err);
/**
* This callback should initialize the child process and preload the
diff --git a/write_or_die.c b/write_or_die.c
index 0734432..eab8c8d 100644
--- a/write_or_die.c
+++ b/write_or_die.c
@@ -1,19 +1,6 @@
#include "cache.h"
#include "run-command.h"
-static void check_pipe(int err)
-{
- if (err == EPIPE) {
- if (in_async())
- async_exit(141);
-
- signal(SIGPIPE, SIG_DFL);
- raise(SIGPIPE);
- /* Should never happen, but just in case... */
- exit(141);
- }
-}
-
/*
* Some cases use stdio, but want to flush after the write
* to get error handling (and to get better interactive
--
2.10.0
^ 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