Re: [PATCH v4 4/4] convert: add "status=delayed" to filter process protocol
Junio C Hamano writes: > If you are depending on a single topic in 'next', it is better to > build on the tip of that topic, not on 'next', if you can figure out > where the tip is. In practice, while we are exchanging patches via > e-mail, there should be no noticeable difference either way [*1*], > ... And the forgotten foot note would have said something like this: *1* If you are depending only on a single topic in 'next', and other topics in 'next' do not interfere with your work, then by definition, your patches that apply cleanly on 'next' ought to apply cleanly on the tip of that single topic. ;-)
Re: [PATCH v4 4/4] convert: add "status=delayed" to filter process protocol
Lars Schneider writes: >> That's right. There might be some code sharing opportunity with Ben's >> code that is already in "next": >> https://github.com/git/git/blob/next/convert.c#L660-L677 >> >> Would it be useful for you if I send v5 with the changes rebased >> onto "next"? > > Hi Junio, > > sorry for bugging you again, but Ben's topic did not make it to "master" > today. Is it OK if I rebase my topic onto "next" and resend? Sorry, your earlier question was lost in the noise and I should have picked it up during my last sweep of leftover bits. If you are depending on a single topic in 'next', it is better to build on the tip of that topic, not on 'next', if you can figure out where the tip is. In practice, while we are exchanging patches via e-mail, there should be no noticeable difference either way [*1*], but once you start throwing a complex and long series, you may want to publish it to a public repository for reviewers and the maintainer to pull, and that workflow might give us an easier way to review, but a topic based on 'next' will never have a chance to be pulled to be merged for real, as merging its tip to 'master' means it will bring all other junk that may not ready. So if you anticipate that to happen someday, practicing to build on things that are only needed (e.g. if you depend on two topics, you may start by merging them on top of 'master' and then building your change on top) is a good idea. No matter what you do, please mention on top of what you built your work. Thanks.
Re: [PATCH v4 4/4] convert: add "status=delayed" to filter process protocol
> On 23 May 2017, at 10:43, Lars Schneider wrote:
>
>
>> On 23 May 2017, at 07:22, Junio C Hamano wrote:
>>
>> Lars Schneider writes:
>>
> + sigchain_pop(SIGPIPE);
> +
> + if (err || errno == EPIPE) {
This looks strange, at first glance.
Do we set errno to 0 before ?
Or is there a trick that EPIPE can only be reached,
if it is "our" error ?
>>>
>>> You are right and I'll fix it!
>>> Thanks for reminding me!
>>> Peff also noticed that some time ago:
>>> http://public-inbox.org/git/20170411200520.oivytvlzkdu7b...@sigill.intra.peff.net/
>>
>> Ben Peart's bp/sub-process-convert-filter topic also had the same
>> EPIPE issues in its earlier incarnation, IIRC. I haven't looked at
>> this topic for some time, but I wonder if we can share code with it.
>
> That's right. There might be some code sharing opportunity with Ben's
> code that is already in "next":
> https://github.com/git/git/blob/next/convert.c#L660-L677
>
> Would it be useful for you if I send v5 with the changes rebased
> onto "next"?
Hi Junio,
sorry for bugging you again, but Ben's topic did not make it to "master"
today. Is it OK if I rebase my topic onto "next" and resend?
Thanks,
Lars
Re: [PATCH v4 4/4] convert: add "status=delayed" to filter process protocol
> On 23 May 2017, at 07:22, Junio C Hamano wrote:
>
> Lars Schneider writes:
>
+ sigchain_pop(SIGPIPE);
+
+ if (err || errno == EPIPE) {
>>>
>>> This looks strange, at first glance.
>>> Do we set errno to 0 before ?
>>> Or is there a trick that EPIPE can only be reached,
>>> if it is "our" error ?
>>
>> You are right and I'll fix it!
>> Thanks for reminding me!
>> Peff also noticed that some time ago:
>> http://public-inbox.org/git/20170411200520.oivytvlzkdu7b...@sigill.intra.peff.net/
>
> Ben Peart's bp/sub-process-convert-filter topic also had the same
> EPIPE issues in its earlier incarnation, IIRC. I haven't looked at
> this topic for some time, but I wonder if we can share code with it.
That's right. There might be some code sharing opportunity with Ben's
code that is already in "next":
https://github.com/git/git/blob/next/convert.c#L660-L677
Would it be useful for you if I send v5 with the changes rebased
onto "next"?
Thanks,
Lars
Re: [PATCH v4 4/4] convert: add "status=delayed" to filter process protocol
Lars Schneider writes:
>>> + sigchain_pop(SIGPIPE);
>>> +
>>> + if (err || errno == EPIPE) {
>>
>> This looks strange, at first glance.
>> Do we set errno to 0 before ?
>> Or is there a trick that EPIPE can only be reached,
>> if it is "our" error ?
>
> You are right and I'll fix it!
> Thanks for reminding me!
> Peff also noticed that some time ago:
> http://public-inbox.org/git/20170411200520.oivytvlzkdu7b...@sigill.intra.peff.net/
Ben Peart's bp/sub-process-convert-filter topic also had the same
EPIPE issues in its earlier incarnation, IIRC. I haven't looked at
this topic for some time, but I wonder if we can share code with it.
Re: [PATCH v4 4/4] convert: add "status=delayed" to filter process protocol
> On 22 May 2017, at 21:58, Torsten Bögershausen wrote:
>
> On 2017-05-22 15:50, Lars Schneider wrote:
>> +
>> +int async_query_available_blobs(const char *cmd, struct string_list
>> *delayed_paths)
>> +{
>> +int err;
>> +char *line;
>> +struct cmd2process *entry;
>> +struct child_process *process;
>> +struct strbuf filter_status = STRBUF_INIT;
>> +
>> +entry = find_multi_file_filter_entry(&cmd_process_map, cmd);
>> +if (!entry) {
>> +error("external filter '%s' is not available anymore although "
>> + "not all paths have been filtered", cmd);
>> +return 0;
>> +}
>> +process = &entry->process;
>> +sigchain_push(SIGPIPE, SIG_IGN);
>> +
>> +err = packet_write_fmt_gently(
>> +process->in, "command=list_available_blobs\n");
>> +if (err)
>> +goto done;
>> +
>> +err = packet_flush_gently(process->in);
>> +if (err)
>> +goto done;
>> +
>> +for (;;) {
>> +const char* pre = "pathname=";
>> +const int pre_len = strlen(pre);
>> +line = packet_read_line(process->out, NULL);
>> +if (!line)
>> +break;
>> +err = strlen(line) <= pre_len || strncmp(line, pre, pre_len);
>> +if (err)
>> +goto done;
>> +string_list_insert(delayed_paths, xstrdup(line+pre_len));
>> +}
>> +
>> +read_multi_file_filter_status(process->out, &filter_status);
>> +err = strcmp(filter_status.buf, "success");
>> +
>> +done:
>> +sigchain_pop(SIGPIPE);
>> +
>> +if (err || errno == EPIPE) {
>
> This looks strange, at first glance.
> Do we set errno to 0 before ?
> Or is there a trick that EPIPE can only be reached,
> if it is "our" error ?
You are right and I'll fix it!
Thanks for reminding me!
Peff also noticed that some time ago:
http://public-inbox.org/git/20170411200520.oivytvlzkdu7b...@sigill.intra.peff.net/
Thanks,
Lars
Re: [PATCH v4 4/4] convert: add "status=delayed" to filter process protocol
> On 22 May 2017, at 21:52, Torsten Bögershausen wrote: > > On 2017-05-22 15:50, Lars Schneider wrote: >> +After Git received the pathnames, it will request the corresponding >> +blobs again. These requests contain a pathname and an empty content >> +section. The filter is expected to respond with the smudged content >> +in the usual way as explained above. >> + >> +packet: git> command=smudge >> +packet: git> pathname=path/testfile.dat >> +packet: git> >> +packet: git> # empty content! >> +packet: git< status=success >> +packet: git< >> +packet: git< SMUDGED_CONTENT >> +packet: git< >> +packet: git< >> + > > The documentation mentions "" 2 times. > Is this a bug in the docu ? Or a feature which may need a comment ? The first marks the end of the content and the second marks the end of an empty status list. Explained in the existing protocol here: https://github.com/git/git/blob/10c78a162fa821ee85203165b805ff46be454091/Documentation/gitattributes.txt#L451-L457 https://github.com/git/git/blob/10c78a162fa821ee85203165b805ff46be454091/Documentation/gitattributes.txt#L464 For clarity I should probably change it to this: ... packet: git< status=success packet: git< packet: git< SMUDGED_CONTENT packet: git< packet: git< # empty list, keep "status=success" unchanged! Thanks, Lars
Re: [PATCH v4 4/4] convert: add "status=delayed" to filter process protocol
On 2017-05-22 15:50, Lars Schneider wrote:
> +
> +int async_query_available_blobs(const char *cmd, struct string_list
> *delayed_paths)
> +{
> + int err;
> + char *line;
> + struct cmd2process *entry;
> + struct child_process *process;
> + struct strbuf filter_status = STRBUF_INIT;
> +
> + entry = find_multi_file_filter_entry(&cmd_process_map, cmd);
> + if (!entry) {
> + error("external filter '%s' is not available anymore although "
> + "not all paths have been filtered", cmd);
> + return 0;
> + }
> + process = &entry->process;
> + sigchain_push(SIGPIPE, SIG_IGN);
> +
> + err = packet_write_fmt_gently(
> + process->in, "command=list_available_blobs\n");
> + if (err)
> + goto done;
> +
> + err = packet_flush_gently(process->in);
> + if (err)
> + goto done;
> +
> + for (;;) {
> + const char* pre = "pathname=";
> + const int pre_len = strlen(pre);
> + line = packet_read_line(process->out, NULL);
> + if (!line)
> + break;
> + err = strlen(line) <= pre_len || strncmp(line, pre, pre_len);
> + if (err)
> + goto done;
> + string_list_insert(delayed_paths, xstrdup(line+pre_len));
> + }
> +
> + read_multi_file_filter_status(process->out, &filter_status);
> + err = strcmp(filter_status.buf, "success");
> +
> +done:
> + sigchain_pop(SIGPIPE);
> +
> + if (err || errno == EPIPE) {
This looks strange, at first glance.
Do we set errno to 0 before ?
Or is there a trick that EPIPE can only be reached,
if it is "our" error ?
> + if (!strcmp(filter_status.buf, "error")) {
> + /* The filter signaled a problem with the file. */
> + } else {
> + /*
> + * Something went wrong with the protocol filter.
> + * Force shutdown and restart if another blob requires
> + * filtering.
> + */
> + error("external filter '%s' failed", cmd);
> + kill_multi_file_filter(&cmd_process_map, entry);
> + }
> + }
> + return !err;
> +}
> +
Re: [PATCH v4 4/4] convert: add "status=delayed" to filter process protocol
On 2017-05-22 15:50, Lars Schneider wrote: > +After Git received the pathnames, it will request the corresponding > +blobs again. These requests contain a pathname and an empty content > +section. The filter is expected to respond with the smudged content > +in the usual way as explained above. > + > +packet: git> command=smudge > +packet: git> pathname=path/testfile.dat > +packet: git> > +packet: git> # empty content! > +packet: git< status=success > +packet: git< > +packet: git< SMUDGED_CONTENT > +packet: git< > +packet: git< > + The documentation mentions "" 2 times. Is this a bug in the docu ? Or a feature which may need a comment ?
Re: [PATCH v4 4/4] convert: add "status=delayed" to filter process protocol
I have no remaining concerns about the protocol specification in terms of implementing a filter with this capability.
