From: Junio C Hamano <gitster@pobox•com>
To: larsxschneider@gmail•com
Cc: git@vger•kernel.org, peff@peff•net, sbeller@google•com,
Johannes.Schindelin@gmx•de, jnareb@gmail•com, mlbright@gmail•com
Subject: Re: [PATCH v6 12/13] convert: add filter.<driver>.process option
Date: Mon, 29 Aug 2016 15:21:15 -0700 [thread overview]
Message-ID: <xmqqzinv6wtg.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <20160825110752.31581-13-larsxschneider@gmail.com> (larsxschneider@gmail.com's message of "Thu, 25 Aug 2016 13:07:51 +0200")
larsxschneider@gmail•com writes:
> +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.
> +------------------------
> +packet: git< status=error\n
> +packet: git< 0000
> +------------------------
> +
> +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 "error-all" status. Depending on
> +the `filter.<driver>.required` flag Git will interpret that as error
> +but it will not stop or restart the filter process.
> +------------------------
> +packet: git< status=error-all\n
> +packet: git< 0000
> +------------------------
This part of the document is well-written to help filter-writers.
One thing that was unclear from the above to me, when read as a
potential filter-writer, is when I am supposed to exit(2). After I
tell Git with error-all (I would have called it "abort", but that's
OK) that I desire no further communication, am I free to go? Or do
I wait until Git somehow disconnects (perhaps by closing the packet
stream I have been reading)?
> +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.
Hmph, is there a reason not to retry a half-converted-and-failed
blob with the fresh process? Note that this is not "you must do it
that way", and it is not even "I think doing so may be a better
idea". I merely want to know the reason behind this decision.
> +After the filter has processed a blob it is expected to wait for
> +the next "key=value" list containing a command. When the Git process
> +terminates, it will send a kill signal to the filter in that stage.
The "kill" may not be very nice. As Git side _knows_ that the
filter is waiting for the next command, having an explicit
"shutdown" command would give the filter a chance to implement a
clean exit--it may have some housekeeping tasks it wants to perform
once it is done. The "explicit shutdown" could just be "the pipe
gets closed", so from the implementation point of view there may not
be anything you need to further add to this patch (after all, when
we exit, the pipes to them would be closed), but the shutdown
protocol and the expectation on the behaviour of filter processes
would need to be documented.
> +If a `filter.<driver>.clean` or `filter.<driver>.smudge` command
> +is configured then these commands always take precedence over
> +a configured `filter.<driver>.process` command.
It may make more sense to give precedence to the .process (which is
a late-comer) if defined, ignoring .clean and .smudge, than the
other way around.
> +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
> +protocol than the latter one.
Would it be a useful sample program we can ship in contrib/ if you
created a "filter adapter" that reads these two configuration
variables and act as a filter.<driver>.process?
During an imaginary session of "git add .", I think I found where
you start THE filter process upon the first path that needs to be
filtered with one for the configured <driver>, and I think the same
place is where you reuse THE filter process, but I am not sure where
you are cleaning up by killing the filter once all paths are added.
Wouldn't you need some hooks at strategic places after such bulk
operation to tell the multi-file-filter machinery to walk all the
entries in cmd_process_map and tell the remaining filter processes
that they have no more tasks, or something? Are you relying on
these processes to exit upon a read failure after we exit and the
pipe going to the filter is severed?
Thanks.
next prev parent reply other threads:[~2016-08-29 22:21 UTC|newest]
Thread overview: 66+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-08-25 11:07 [PATCH v6 00/13] Git filter protocol larsxschneider
2016-08-25 11:07 ` [PATCH v6 01/13] pkt-line: rename packet_write() to packet_write_fmt() larsxschneider
2016-08-25 11:07 ` [PATCH v6 02/13] pkt-line: extract set_packet_header() larsxschneider
2016-08-25 11:07 ` [PATCH v6 03/13] pkt-line: add packet_write_fmt_gently() larsxschneider
2016-08-25 18:12 ` Stefan Beller
2016-08-25 18:47 ` Lars Schneider
2016-08-25 21:41 ` Junio C Hamano
2016-08-26 9:17 ` Lars Schneider
2016-08-26 17:10 ` Junio C Hamano
2016-08-26 17:23 ` Jeff King
2016-08-25 11:07 ` [PATCH v6 04/13] pkt-line: add packet_flush_gently() larsxschneider
2016-08-25 11:07 ` [PATCH v6 05/13] pkt-line: add packet_write_gently() larsxschneider
2016-08-25 21:50 ` Junio C Hamano
2016-08-26 9:40 ` Lars Schneider
2016-08-26 17:15 ` Junio C Hamano
2016-08-29 9:40 ` Lars Schneider
2016-08-25 11:07 ` [PATCH v6 06/13] pkt-line: add functions to read/write flush terminated packet streams larsxschneider
2016-08-25 18:46 ` Stefan Beller
2016-08-25 19:33 ` Lars Schneider
2016-08-25 22:31 ` Junio C Hamano
2016-08-26 0:55 ` Jacob Keller
2016-08-26 17:02 ` Stefan Beller
2016-08-26 17:21 ` Jeff King
2016-08-26 17:17 ` Junio C Hamano
2016-08-25 22:27 ` Junio C Hamano
2016-08-26 10:13 ` Lars Schneider
2016-08-26 17:21 ` Junio C Hamano
2016-08-29 9:43 ` Lars Schneider
2016-08-25 11:07 ` [PATCH v6 07/13] pack-protocol: fix maximum pkt-line size larsxschneider
2016-08-25 18:59 ` Stefan Beller
2016-08-25 19:35 ` Lars Schneider
2016-08-26 19:44 ` Junio C Hamano
2016-08-25 11:07 ` [PATCH v6 08/13] convert: quote filter names in error messages larsxschneider
2016-08-26 19:45 ` Junio C Hamano
2016-08-25 11:07 ` [PATCH v6 09/13] convert: modernize tests larsxschneider
2016-08-26 20:03 ` Junio C Hamano
2016-08-29 10:09 ` Lars Schneider
2016-08-25 11:07 ` [PATCH v6 10/13] convert: generate large test files only once larsxschneider
2016-08-25 19:17 ` Stefan Beller
2016-08-25 19:54 ` Lars Schneider
2016-08-29 17:52 ` Junio C Hamano
2016-08-30 11:47 ` Lars Schneider
2016-08-30 16:55 ` Junio C Hamano
2016-08-29 17:46 ` Junio C Hamano
2016-08-30 11:41 ` Lars Schneider
2016-08-30 16:37 ` Jeff King
2016-08-25 11:07 ` [PATCH v6 11/13] convert: make apply_filter() adhere to standard Git error handling larsxschneider
2016-08-25 11:07 ` [PATCH v6 12/13] convert: add filter.<driver>.process option larsxschneider
2016-08-29 22:21 ` Junio C Hamano [this message]
2016-08-30 16:27 ` Lars Schneider
2016-08-30 18:59 ` Junio C Hamano
2016-08-30 20:38 ` Lars Schneider
2016-08-30 22:23 ` Junio C Hamano
2016-08-31 4:57 ` Torsten Bögershausen
2016-08-31 13:14 ` Jakub Narębski
2016-08-30 20:46 ` Jakub Narębski
2016-09-05 19:47 ` Lars Schneider
2016-08-25 11:07 ` [PATCH v6 13/13] read-cache: make sure file handles are not inherited by child processes larsxschneider
2016-08-29 18:05 ` Junio C Hamano
2016-08-29 19:03 ` Lars Schneider
2016-08-29 19:45 ` Junio C Hamano
2016-08-30 12:32 ` Lars Schneider
2016-08-30 14:54 ` Torsten Bögershausen
2016-09-01 17:15 ` Junio C Hamano
2016-08-29 15:39 ` [PATCH v6 00/13] Git filter protocol Lars Schneider
2016-08-29 18:09 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqzinv6wtg.fsf@gitster.mtv.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=Johannes.Schindelin@gmx$(echo .)de \
--cc=git@vger$(echo .)kernel.org \
--cc=jnareb@gmail$(echo .)com \
--cc=larsxschneider@gmail$(echo .)com \
--cc=mlbright@gmail$(echo .)com \
--cc=peff@peff$(echo .)net \
--cc=sbeller@google$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox