From: Lars Schneider <redacted>
Hi all,
## Notable changes since v4
* drop the shutdown capability
* add error-all response to signal that the filter does not want to filter anymore
* extend init sequence to negotiate version number and capabilities
(plus detect wrongly configured version 1 filters)
* improve status response format according to Peff's suggestions
* fix Git for Window support
## Patches
* 01-09: Make pktline ready to be used for filters.
* 01 - @Junio: I know you told me twice that you don't like this patch but
please look closely: the patch does not add a new interface to
pktline. It is a private functions used 03.
* 08 - This patch changes the pktline interface as it renames packet_write() to
packet_write_fmt() as suggested by Junio.
(This patch is not required for the the series and could be dropped)
* 10-13: Prepare convert code.
* 14: Main contribution.
* 15: Proposed fix to make the long running filters work on Windows. See
the discussion here:
https://github.com/git-for-windows/git/issues/770#issuecomment-238361829
(This patch is not required in Git core and could be dropped)
## Jakub
* http://public-inbox.org/git/607c07fe-5b6f-fd67-13e1-705020c267ee%40gmail.com/
* make Git start the handshake with the filter to detect one-shot filter
mis configured as process-filter
* filter driver refusing to filter further
* http://public-inbox.org/git/e8b550ed-1765-764f-49e5-72e5a609d936%40gmail.com/
* improve commit messages
* http://public-inbox.org/git/0b7d7d96-dfdc-54a4-2c24-2aead6743ae1%40gmail.com/
* handle sigpipe correctly
* use cp instead of cat where appropriate
* explicitly name the test with a binary file (containing \0)
## Junio
* http://public-inbox.org/git/xmqq8twd8uld.fsf%40gitster.mtv.corp.google.com/
* shorten variable names
* do not initialize statics explicitly to 0
* no // comments
* replace "ret" with "!error"
* http://public-inbox.org/git/CAPc5daV3Tke4qHjtpri%3D6QCRaOax_K3uYhpFzRcd271%3DGHj1%2BQ%40mail.gmail.com/
* rename packet_write() to packet_write_fmt()
## Peff
* http://public-inbox.org/git/20160803224619.bwtbvmslhuicx2qi%40sigill.intra.peff.net/
* change capabilities format
* http://public-inbox.org/git/20160808150255.2otm3z5fluimpiqw%40sigill.intra.peff.net/
* change status response format
## Eric
* http://public-inbox.org/git/20160805185559.GB463%40starla/
* die if larger-than-4GB files are processed on 32-bit
Thanks a lot for the valuable reviews!
Best,
Lars
You can find a branch with the source on GitHub here:
https://github.com/larsxschneider/git/tree/protocol-filter/v5
I also rebased the series on top of the latest Git for Windows master:
https://github.com/larsxschneider/git/tree/protocol-filter/v5-win
Lars Schneider (15):
pkt-line: extract set_packet_header()
pkt-line: call packet_trace() only if a packet is actually send
pkt-line: add `gentle` parameter to format_packet()
pkt-line: add packet_write_gently()
pkt-line: add packet_write_gently_fmt()
pkt-line: add packet_flush_gently()
pkt-line: add functions to read/write flush terminated packet streams
pkt-line: rename packet_write() to packet_write_fmt()
pack-protocol: fix maximum pkt-line size
convert: quote filter names in error messages
convert: modernize tests
convert: generate large test files only once
convert: make apply_filter() adhere to standard Git error handling
convert: add filter.<driver>.process option
read-cache: make sure file handles are not inherited by child
processes
Documentation/gitattributes.txt | 139 +++++++-
Documentation/technical/protocol-common.txt | 6 +-
builtin/archive.c | 4 +-
builtin/receive-pack.c | 4 +-
builtin/remote-ext.c | 4 +-
builtin/upload-archive.c | 4 +-
connect.c | 2 +-
convert.c | 367 ++++++++++++++++++---
daemon.c | 2 +-
http-backend.c | 2 +-
pkt-line.c | 155 ++++++++-
pkt-line.h | 12 +-
read-cache.c | 2 +-
shallow.c | 2 +-
t/t0021-conversion.sh | 482 +++++++++++++++++++++++++---
t/t0021/rot13-filter.pl | 176 ++++++++++
unpack-trees.c | 1 +
upload-pack.c | 30 +-
18 files changed, 1267 insertions(+), 127 deletions(-)
create mode 100755 t/t0021/rot13-filter.pl
--
2.9.2
From: Lars Schneider <redacted>
Git's clean/smudge mechanism invokes an external filter process for every
single blob that is affected by a filter. If Git filters a lot of blobs
then the startup time of the external filter processes can become a
significant part of the overall Git execution time.
In a preliminary performance test this developer used a clean/smudge filter
written in golang to filter 12,000 files. This process took 364s with the
existing filter mechanism and 5s with the new mechanism. See details here:
https://github.com/github/git-lfs/pull/1382
This patch adds the `filter.<driver>.process` string option which, if used,
keeps the external filter process running and processes all blobs with
the packet format (pkt-line) based protocol over standard input and standard
output described below.
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, a list of supported protocol
version numbers, and a flush packet. Git expects to read the
welcome message and one protocol version number from the
previously sent list. Afterwards Git sends a list of supported
capabilities 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
------------------------
Supported filter capabilities in version 2 are "clean" and
"smudge".
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
Git sends the content split in zero or more pkt-line packets and a
flush packet to terminate content.
------------------------
packet: git> command=smudge\n
packet: git> pathname=path/testfile.dat\n
packet: git> 0000
packet: git> CONTENT
packet: git> 0000
------------------------
The filter is expected to respond with a list of "key=value" pairs
terminated with a flush packet. If the filter does not experience
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.
------------------------
packet: git< status=success\n
packet: git< 0000
packet: git< SMUDGED_CONTENT
packet: git< 0000
packet: git< 0000 # empty list!
------------------------
If the result content is empty then the filter is expected to respond
with a success status and an empty list.
------------------------
packet: git< status=success\n
packet: git< 0000
packet: git< 0000 # empty content!
packet: git< 0000 # empty list!
------------------------
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
------------------------
If the filter experiences an error during processing, then it can
send the status "error". 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=success\n
packet: git< 0000
packet: git< HALF_WRITTEN_ERRONEOUS_CONTENT
packet: git< 0000
packet: git< status=error\n
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.
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.
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.
Helped-by: Martin-Louis Bright [off-list ref]
Reviewed-by: Jakub Narebski <redacted>
Signed-off-by: Lars Schneider <redacted>
---
Documentation/gitattributes.txt | 139 ++++++++++++++-
convert.c | 343 ++++++++++++++++++++++++++++++++----
t/t0021-conversion.sh | 372 ++++++++++++++++++++++++++++++++++++++++
t/t0021/rot13-filter.pl | 176 +++++++++++++++++++
unpack-trees.c | 1 +
5 files changed, 1001 insertions(+), 30 deletions(-)
create mode 100755 t/t0021/rot13-filter.pl
@@ -300,7 +300,13 @@ checkout, when the `smudge` command is specified, the command is fed the blob object from its standard input, and its standard output is used to update the worktree file. Similarly, the `clean` command is used to convert the contents of worktree file-upon checkin.+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. 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.
@@ -375,6 +381,137 @@ substitution. For example: ------------------------+Long Running Filter Process+^^^^^^^^^^^^^^^^^^^^^^^^^^^++If the filter command (a string value) is defined via+`filter.<driver>.process` then Git can process all blobs with a+single filter invocation for the entire life of a single Git+command. This is achieved by using the following packet format+(pkt-line, see technical/protocol-common.txt) based protocol over+standard input and standard output.++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, a list of supported protocol+version numbers, and a flush packet. Git expects to read the+welcome message and one protocol version number from the+previously sent list. Afterwards Git sends a list of supported+capabilities 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+------------------------+Supported filter capabilities in version 2 are "clean" and+"smudge".++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+Git sends the content split in zero or more pkt-line packets and a+flush packet to terminate content.+------------------------+packet: git> command=smudge\n+packet: git> pathname=path/testfile.dat\n+packet: git> 0000+packet: git> CONTENT+packet: git> 0000+------------------------++The filter is expected to respond with a list of "key=value" pairs+terminated with a flush packet. If the filter does not experience+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.+------------------------+packet: git< status=success\n+packet: git< 0000+packet: git< SMUDGED_CONTENT+packet: git< 0000+packet: git< 0000 # empty list!+------------------------++If the result content is empty then the filter is expected to respond+with a success status and an empty list.+------------------------+packet: git< status=success\n+packet: git< 0000+packet: git< 0000 # empty content!+packet: git< 0000 # empty list!+------------------------++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+------------------------++If the filter experiences an error during processing, then it can+send the status "error". 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=success\n+packet: git< 0000+packet: git< HALF_WRITTEN_ERRONEOUS_CONTENT+packet: git< 0000+packet: git< status=error\n+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.++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.++A long running filter demo implementation can be found in+`t/t0021/rot13-filter.pl` located in the Git 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>.clean` or `filter.<driver>.smudge` command+is configured then these commands always take precedence over+a configured `filter.<driver>.process` 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+protocol than the latter one.++ Interaction between checkin/checkout attributes ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -427,7 +428,7 @@ static int filter_buffer_or_fd(int in, int out, void *data)return(write_err||status);}-staticintapply_filter(constchar*path,constchar*src,size_tlen,intfd,+staticintapply_single_file_filter(constchar*path,constchar*src,size_tlen,intfd,structstrbuf*dst,constchar*cmd){/*
@@ -441,12 +442,6 @@ static int apply_filter(const char *path, const char *src, size_t len, int fd,structasyncasync;structfilter_paramsparams;-if(!cmd||!*cmd)-return0;--if(!dst)-return1;-memset(&async,0,sizeof(async));async.proc=filter_buffer_or_fd;async.data=¶ms;
@@ -481,14 +476,311 @@ static int apply_filter(const char *path, const char *src, size_t len, int fd,return!err;}+#define CAP_CLEAN (1u<<0)+#define CAP_SMUDGE (1u<<1)++structcmd2process{+structhashmap_entryent;/* must be the first member! */+intsupported_capabilities;+constchar*cmd;+structchild_processprocess;+};++staticintcmd_process_map_initialized;+staticstructhashmapcmd_process_map;++staticintcmd2process_cmp(conststructcmd2process*e1,+conststructcmd2process*e2,+constvoid*unused)+{+returnstrcmp(e1->cmd,e2->cmd);+}++staticstructcmd2process*find_multi_file_filter_entry(structhashmap*hashmap,constchar*cmd)+{+structcmd2processkey;+hashmap_entry_init(&key,strhash(cmd));+key.cmd=cmd;+returnhashmap_get(hashmap,&key,NULL);+}++staticvoidkill_multi_file_filter(structhashmap*hashmap,structcmd2process*entry)+{+if(!entry)+return;+sigchain_push(SIGPIPE,SIG_IGN);+/*+*Wekillthefiltermostlikelybecauseanerrorhappenedalready.+*That'swhywearenotinterestedinanyerrorcodehere.+*/+close(entry->process.in);+close(entry->process.out);+sigchain_pop(SIGPIPE);+finish_command(&entry->process);+hashmap_remove(hashmap,entry,NULL);+free(entry);+}++staticintpacket_write_list(intfd,constchar*line,...)+{+va_listargs;+interr;+va_start(args,line);+for(;;)+{+if(!line)+break;+err=packet_write_gently_fmt(fd,"%s",line);+if(err)+returnerr;+line=va_arg(args,constchar*);+}+va_end(args);+returnpacket_flush_gently(fd);+}++staticstructcmd2process*start_multi_file_filter(structhashmap*hashmap,constchar*cmd)+{+interr;+structcmd2process*entry;+structchild_process*process;+constchar*argv[]={cmd,NULL};+structstring_listcap_list=STRING_LIST_INIT_NODUP;+char*cap_buf;+constchar*cap_name;++entry=xmalloc(sizeof(*entry));+hashmap_entry_init(entry,strhash(cmd));+entry->cmd=cmd;+entry->supported_capabilities=0;+process=&entry->process;++child_process_init(process);+process->argv=argv;+process->use_shell=1;+process->in=-1;+process->out=-1;++if(start_command(process)){+error("cannot fork to run external filter '%s'",cmd);+kill_multi_file_filter(hashmap,entry);+returnNULL;+}++sigchain_push(SIGPIPE,SIG_IGN);++err=packet_write_list(process->in,"git-filter-client","version=2",NULL);+if(err)+gotodone;++err=strcmp(packet_read_line(process->out,NULL),"git-filter-server");+if(err){+error("external filter '%s' does not support long running filter protocol",cmd);+gotodone;+}+err=strcmp(packet_read_line(process->out,NULL),"version=2");+if(err)+gotodone;++err=packet_write_list(process->in,"clean=true","smudge=true",NULL);++for(;;)+{+cap_buf=packet_read_line(process->out,NULL);+if(!cap_buf)+break;+string_list_split_in_place(&cap_list,cap_buf,'=',1);++if(cap_list.nr!=2||strcmp(cap_list.items[1].string,"true"))+continue;++cap_name=cap_list.items[0].string;+if(!strcmp(cap_name,"clean")){+entry->supported_capabilities|=CAP_CLEAN;+}elseif(!strcmp(cap_name,"smudge")){+entry->supported_capabilities|=CAP_SMUDGE;+}else{+warning(+"external filter '%s' requested unsupported filter capability '%s'",+cmd,cap_name+);+}++string_list_clear(&cap_list,0);+}++done:+sigchain_pop(SIGPIPE);++if(err||errno==EPIPE){+error("initialization for external filter '%s' failed",cmd);+kill_multi_file_filter(hashmap,entry);+returnNULL;+}++hashmap_add(hashmap,entry);+returnentry;+}++staticvoidread_multi_file_filter_values(intfd,structstrbuf*status){+structstrbuf**pair;+char*line;+for(;;){+line=packet_read_line(fd,NULL);+if(!line)+break;+pair=strbuf_split_str(line,'=',2);+if(pair[0]&&pair[0]->len&&pair[1]){+if(!strcmp(pair[0]->buf,"status=")){+strbuf_reset(status);+strbuf_addbuf(status,pair[1]);+}+}+}+}++staticintapply_multi_file_filter(constchar*path,constchar*src,size_tlen,+intfd,structstrbuf*dst,constchar*cmd,+constintwanted_capability)+{+interr;+structcmd2process*entry;+structchild_process*process;+structstatfile_stat;+structstrbufnbuf=STRBUF_INIT;+structstrbuffilter_status=STRBUF_INIT;+char*filter_type;++if(!cmd_process_map_initialized){+cmd_process_map_initialized=1;+hashmap_init(&cmd_process_map,(hashmap_cmp_fn)cmd2process_cmp,0);+entry=NULL;+}else{+entry=find_multi_file_filter_entry(&cmd_process_map,cmd);+}++fflush(NULL);++if(!entry){+entry=start_multi_file_filter(&cmd_process_map,cmd);+if(!entry)+return0;+}+process=&entry->process;++if(!(wanted_capability&entry->supported_capabilities))+return0;++if(CAP_CLEAN&wanted_capability)+filter_type="clean";+elseif(CAP_SMUDGE&wanted_capability)+filter_type="smudge";+else+die("unexpected filter type");++if(fd>=0&&!src){+if(fstat(fd,&file_stat)==-1)+return0;+len=xsize_t(file_stat.st_size);+}++sigchain_push(SIGPIPE,SIG_IGN);++err=packet_write_gently_fmt(process->in,"command=%s\n",filter_type);+if(err)+gotodone;++err=packet_write_gently_fmt(process->in,"pathname=%s\n",path);+if(err)+gotodone;++err=packet_flush_gently(process->in);+if(err)+gotodone;++if(fd>=0)+err=packet_write_stream_with_flush_from_fd(fd,process->in);+else+err=packet_write_stream_with_flush_from_buf(src,len,process->in);+if(err)+gotodone;++read_multi_file_filter_values(process->out,&filter_status);+err=strcmp(filter_status.buf,"success");+if(err)+gotodone;++err=packet_read_till_flush(process->out,&nbuf)<0;+if(err)+gotodone;++read_multi_file_filter_values(process->out,&filter_status);+err=strcmp(filter_status.buf,"success");++done:+sigchain_pop(SIGPIPE);++if(err||errno==EPIPE){+if(!strcmp(filter_status.buf,"error")){+/*+*Thefiltersignaledaproblemwiththefile.+*/+}elseif(!strcmp(filter_status.buf,"error-all")){+/*+*Thefiltersignaledapermanentproblem.Don'ttrytofilter+*fileswiththesamecommandforthelifetimeofthecurrent+*Gitprocess.+*/+entry->supported_capabilities&=~wanted_capability;+}else{+/*+*Somethingwentwrongwiththeprotocolfilter.+*Forceshutdownandrestartifanotherblobrequiresfiltering!+*/+error("external filter '%s' failed",cmd);+kill_multi_file_filter(&cmd_process_map,entry);+}+}else{+strbuf_swap(dst,&nbuf);+}+strbuf_release(&nbuf);+return!err;+}+staticstructconvert_driver{constchar*name;structconvert_driver*next;constchar*smudge;constchar*clean;+constchar*process;intrequired;}*user_convert,**user_convert_tail;+staticintapply_filter(constchar*path,constchar*src,size_tlen,+intfd,structstrbuf*dst,structconvert_driver*drv,+constintwanted_capability)+{+constchar*cmd=NULL;++if(!drv)+return0;++if(!dst)+return1;++if((CAP_CLEAN&wanted_capability)&&drv->clean)+cmd=drv->clean;+elseif((CAP_SMUDGE&wanted_capability)&&drv->smudge)+cmd=drv->smudge;++if(cmd&&*cmd)+returnapply_single_file_filter(path,src,len,fd,dst,cmd);+elseif(drv->process&&*drv->process)+returnapply_multi_file_filter(path,src,len,fd,dst,drv->process,wanted_capability);++return0;+}+staticintread_convert_config(constchar*var,constchar*value,void*cb){constchar*key,*name;
@@ -296,4 +299,373 @@ test_expect_success 'disable filter with empty override' 'test_must_be_emptyerr'+check_filter(){+rm-frot13-filter.logactual.log&&+"$@"2>git_stderr.log&&+test_must_be_emptygit_stderr.log&&+cat>expected.log&&+sortrot13-filter.log|uniq-c|sed"s/^[ ]*//">actual.log&&+test_cmpexpected.logactual.log+}++check_filter_count_clean(){+rm-frot13-filter.logactual.log&&+"$@"2>git_stderr.log&&+test_must_be_emptygit_stderr.log&&+cat>expected.log&&+sortrot13-filter.log|uniq-c|sed"s/^[ ]*//"|+sed"s/^\([0-9]\) IN: clean/x IN: clean/">actual.log&&+test_cmpexpected.logactual.log+}++check_filter_ignore_clean(){+rm-frot13-filter.logactual.log&&+"$@"&&+cat>expected.log&&+grep-v"IN: clean"rot13-filter.log>actual.log&&+test_cmpexpected.logactual.log+}++check_filter_no_call(){+rm-frot13-filter.log&&+"$@"2>git_stderr.log&&+test_must_be_emptygit_stderr.log&&+test_must_be_emptyrot13-filter.log+}++check_rot13(){+test_cmp$1$2&&+./../rot13.sh<$1>expected&&+gitcat-fileblob:$2>actual&&+test_cmpexpectedactual+}++test_expect_successPERL'required process filter should filter data''+test_config_globalfilter.protocol.process"$TEST_DIRECTORY/t0021/rot13-filter.pl clean smudge"&&+test_config_globalfilter.protocol.requiredtrue&&+rm-rfrepo&&+mkdirrepo&&+(+cdrepo&&+gitinit&&++echo"*.r filter=protocol">.gitattributes&&+gitadd.&&+gitcommit.-m"test commit"&&+gitbranchempty&&++cp../test.otest.r&&+cp../test2.otest2.r&&+mkdirtestsubdir&&+cp../test3-subdir.otestsubdir/test3-subdir.r&&+>test4-empty.r&&++check_filter\+gitadd.\+<<-\EOF&&+1IN:cleantest.r57[OK]--OUT:57[OK]+1IN:cleantest2.r14[OK]--OUT:14[OK]+1IN:cleantest4-empty.r0[OK]--OUT:0[OK]+1IN:cleantestsubdir/test3-subdir.r21[OK]--OUT:21[OK]+1start+1wrotefilterheader+EOF++check_filter_count_clean\+gitcommit.-m"test commit"\+<<-\EOF&&+xIN:cleantest.r57[OK]--OUT:57[OK]+xIN:cleantest2.r14[OK]--OUT:14[OK]+xIN:cleantest4-empty.r0[OK]--OUT:0[OK]+xIN:cleantestsubdir/test3-subdir.r21[OK]--OUT:21[OK]+1start+1wrotefilterheader+EOF++rm-ftest?.rtestsubdir/test3-subdir.r&&++check_filter_ignore_clean\+gitcheckout.\+<<-\EOF&&+start+wrotefilterheader+IN:smudgetest2.r14[OK]--OUT:14[OK]+IN:smudgetestsubdir/test3-subdir.r21[OK]--OUT:21[OK]+EOF++check_filter_ignore_clean\+gitcheckoutempty\+<<-\EOF&&+start+wrotefilterheader+EOF++check_filter_ignore_clean\+gitcheckoutmaster\+<<-\EOF&&+start+wrotefilterheader+IN:smudgetest.r57[OK]--OUT:57[OK]+IN:smudgetest2.r14[OK]--OUT:14[OK]+IN:smudgetest4-empty.r0[OK]--OUT:0[OK]+IN:smudgetestsubdir/test3-subdir.r21[OK]--OUT:21[OK]+EOF++check_rot13../test.otest.r&&+check_rot13../test2.otest2.r&&+check_rot13../test3-subdir.otestsubdir/test3-subdir.r+)+'++test_expect_successPERL'required process filter should filter smudge data and one-shot filter should clean''+test_config_globalfilter.protocol.clean./../rot13.sh&&+test_config_globalfilter.protocol.process"$TEST_DIRECTORY/t0021/rot13-filter.pl smudge"&&+test_config_globalfilter.protocol.requiredtrue&&+rm-rfrepo&&+mkdirrepo&&+(+cdrepo&&+gitinit&&++echo"*.r filter=protocol">.gitattributes&&+gitadd.&&+gitcommit.-m"test commit"&&+gitbranchempty&&++cp../test.otest.r&&+cp../test2.otest2.r&&++check_filter_no_call\+gitadd.&&++check_filter_no_call\+gitcommit.-m"test commit"&&++rm-ftest?.rtestsubdir/test3-subdir.r&&++check_filter_ignore_clean\+gitcheckout.\+<<-\EOF&&+start+wrotefilterheader+IN:smudgetest2.r14[OK]--OUT:14[OK]+EOF++gitcheckoutempty&&++check_filter_ignore_clean\+gitcheckoutmaster\+<<-\EOF&&+start+wrotefilterheader+IN:smudgetest.r57[OK]--OUT:57[OK]+IN:smudgetest2.r14[OK]--OUT:14[OK]+EOF++check_rot13../test.otest.r&&+check_rot13../test2.otest2.r+)+'++test_expect_successPERL'required process filter should clean only''+test_config_globalfilter.protocol.process"$TEST_DIRECTORY/t0021/rot13-filter.pl clean"&&+test_config_globalfilter.protocol.requiredtrue&&+rm-rfrepo&&+mkdirrepo&&+(+cdrepo&&+gitinit&&++echo"*.r filter=protocol">.gitattributes&&+gitadd.&&+gitcommit.-m"test commit"&&+gitbranchempty&&++cp../test.otest.r&&++check_filter\+gitadd.\+<<-\EOF&&+1IN:cleantest.r57[OK]--OUT:57[OK]+1start+1wrotefilterheader+EOF++check_filter_count_clean\+gitcommit.-m"test commit"\+<<-\EOF+xIN:cleantest.r57[OK]--OUT:57[OK]+1start+1wrotefilterheader+EOF+)+'++test_expect_successPERL'required process filter should process binary files larger LARGE_PACKET_MAX''+test_config_globalfilter.protocol.process"$TEST_DIRECTORY/t0021/rot13-filter.pl clean smudge"&&+test_config_globalfilter.protocol.requiredtrue&&+rm-rfrepo&&+mkdirrepo&&+(+cdrepo&&+gitinit&&++echo"*.file filter=protocol">.gitattributes&&+cat../generated-test-data/largish.file.rot13>large.rot13&&+cat../generated-test-data/largish.file>large.file&&+catlarge.file>large.original&&++gitaddlarge.file.gitattributes&&+gitcommit.-m"test commit"&&++rm-flarge.file&&+gitcheckout--large.file&&+gitcat-fileblob:large.file>actual&&+test_cmplarge.rot13actual+)+'++test_expect_successPERL'required process filter should with clean error should fail''+test_config_globalfilter.protocol.process"$TEST_DIRECTORY/t0021/rot13-filter.pl clean smudge"&&+test_config_globalfilter.protocol.requiredtrue&&+rm-rfrepo&&+mkdirrepo&&+(+cdrepo&&+gitinit&&++echo"*.r filter=protocol">.gitattributes&&++cp../test.otest.r&&+echo"this is going to fail">clean-write-fail.r&&+echo"content-test3-subdir">test3.r&&++# Note: There are three clean paths in convert.c we just test one here.+test_must_failgitadd.+)+'++test_expect_successPERL'process filter should restart after unexpected write failure''+test_config_globalfilter.protocol.process"$TEST_DIRECTORY/t0021/rot13-filter.pl clean smudge"&&+rm-rfrepo&&+mkdirrepo&&+(+cdrepo&&+gitinit&&++echo"*.r filter=protocol">.gitattributes&&++cp../test.otest.r&&+cp../test2.otest2.r&&+echo"this is going to fail">smudge-write-fail.o&&+catsmudge-write-fail.o>smudge-write-fail.r&&+gitadd.&&+gitcommit.-m"test commit"&&+rm-f*.r&&++check_filter_ignore_clean\+gitcheckout.\+<<-\EOF&&+start+wrotefilterheader+IN:smudgesmudge-write-fail.r22[OK]--OUT:22[WRITEFAIL]+start+wrotefilterheader+IN:smudgetest.r57[OK]--OUT:57[OK]+IN:smudgetest2.r14[OK]--OUT:14[OK]+EOF++check_rot13../test.otest.r&&+check_rot13../test2.otest2.r&&++!test_cmpsmudge-write-fail.osmudge-write-fail.r&&# Smudge failed!+./../rot13.sh<smudge-write-fail.o>expected&&+gitcat-fileblob:smudge-write-fail.r>actual&&+test_cmpexpectedactual# Clean worked!+)+'++test_expect_successPERL'process filter should not restart in case of an error''+test_config_globalfilter.protocol.process"$TEST_DIRECTORY/t0021/rot13-filter.pl clean smudge"&&+rm-rfrepo&&+mkdirrepo&&+(+cdrepo&&+gitinit&&++echo"*.r filter=protocol">.gitattributes&&++cp../test.otest.r&&+cp../test2.otest2.r&&+echo"this will cause an error">error.o&&+cperror.oerror.r&&+gitadd.&&+gitcommit.-m"test commit"&&+rm-f*.r&&++check_filter_ignore_clean\+gitcheckout.\+<<-\EOF&&+start+wrotefilterheader+IN:smudgeerror.r25[OK]--OUT:0[ERROR]+IN:smudgetest.r57[OK]--OUT:57[OK]+IN:smudgetest2.r14[OK]--OUT:14[OK]+EOF++check_rot13../test.otest.r&&+check_rot13../test2.otest2.r&&+test_cmperror.oerror.r+)+'++test_expect_successPERL'process filter should be able to signal an error for all future files''+test_config_globalfilter.protocol.process"$TEST_DIRECTORY/t0021/rot13-filter.pl clean smudge"&&+rm-rfrepo&&+mkdirrepo&&+(+cdrepo&&+gitinit&&++echo"*.r filter=protocol">.gitattributes&&++cp../test.otest.r&&+cp../test2.otest2.r&&+echo"error this blob and all future blobs">error-all.o&&+cperror-all.oerror-all.r&&+gitadd.&&+gitcommit.-m"test commit"&&+rm-f*.r&&++check_filter_ignore_clean\+gitcheckout.\+<<-\EOF&&+start+wrotefilterheader+IN:smudgeerror-all.r37[OK]--OUT:0[ERROR-ALL]+EOF++test_cmp../test.otest.r&&+test_cmp../test2.otest2.r&&+test_cmperror-all.oerror-all.r+)+'++test_expect_successPERL'invalid process filter must fail (and not hang!)''+test_config_globalfilter.protocol.processcat&&+test_config_globalfilter.protocol.requiredtrue&&+rm-rfrepo&&+mkdirrepo&&+(+cdrepo&&+gitinit&&++echo"*.r filter=protocol">.gitattributes&&++cp../test.otest.r&&+test_must_failgitadd.2>git_stderr.log&&+grep"not support long running filter protocol"git_stderr.log+)+'+ test_done
From: Jeff King <hidden> Date: 2016-08-10 18:35:30
On Wed, Aug 10, 2016 at 03:59:19PM +0200, Lars Schneider wrote:
quoted
It does still feel a little weird that you cannot tell the difference
between a write() error and bad input. Because you really might want to
do something different between the two. Like:
#define MAX_FILENAME (PKTLINE_DATA_MAXLEN - strlen("filename"))
if (filename > MAX_FILENAME) {
warning("woah, that name is ridiculous; truncating");
ret = packet_write_fmt_gently(fd, "%.*s", MAX_FILENAME, filename);
} else
ret = packet_write_fmt_gently(fd, "%s", filename);
I can do that. However, I wouldn't truncate the filename as this
might create a weird outcome. I would just let the filter fail.
Yeah, I think that is probably fine (I don't have a real opinion for
this particular case, but was mostly just trying to think about whether
the pktline interface was suitably flexible).
-Peff
From: Jeff King <hidden> Date: 2016-08-10 18:36:05
On Wed, Aug 10, 2016 at 03:51:35PM +0200, Lars Schneider wrote:
I guess my point is that I stumbled over the un-intutiive format_packet() behavior
and I wanted to improve the situation in a way that others don't run into this
trap. If you think that is no issue then it would be OK for me if we leave the
current behavior as is.
I don't think the behavior as-is is a problem, and it would remain OK as
long as no callers are added who format a packet but don't write it (or
don't write it in a timely manner).
But most importantly, if you are going to refactor code, you can't
regress the existing callers. So even if we did want to change how this
worked, this patch is not acceptable as-is; it would need to fix up all
of the callers of packet_buf_write().
-Peff
From: Jeff King <hidden> Date: 2016-08-10 18:40:08
On Wed, Aug 10, 2016 at 03:03:58PM +0200, larsxschneider@gmail.com wrote:
From: Lars Schneider <redacted>
The packet_trace() call is not ideal in format_packet() as we would print
a trace when a packet is formatted and (potentially) when the packet is
actually send. This was no problem up until now because format_packet()
was only used by one function. Fix it by moving the trace call into the
function that actually sends the packet.
It looks like there are two functions: packet_write() and
packet_buf_write().
Your patch only touches one of them, and it looks like we would fail to
trace many packets (e.g., see receive-pack.c:report(), which uses
packet_buf_write() and then write()s out the result).
-Peff
PS Also, s/send/sent/ in the commit message.
From: Jeff King <hidden> Date: 2016-08-10 18:43:20
On Wed, Aug 10, 2016 at 03:03:59PM +0200, larsxschneider@gmail.com wrote:
From: Lars Schneider <redacted>
format_packet() dies if the caller wants to format a packet larger than
LARGE_PACKET_MAX. Certain callers might prefer an error response instead.
I am not sure I agree here. Certainly I see the usefulness of gently
handling a failure to write(). But if you are passing in too-large
buffers, isn't that a bug in the program?
How would you recover, except by splitting up the content? That might
not be possible depending on how you are using the pkt-lines. And even
if it is, wouldn't it be simpler to split it up before sending it to
format_packet()?
-Peff
@@ -662,7 +662,7 @@ static void receive_needs(void)structobject*object=shallows.objects[i].item;if(object->flags&NOT_SHALLOW){structcommit_list*parents;-packet_write(1,"unshallow %s",+packet_write_fmt(1,"unshallow %s",oid_to_hex(&object->oid));object->flags&=~CLIENT_SHALLOW;/* make sure the real parents are parsed */
From: Jeff King <hidden> Date: 2016-08-10 18:57:26
On Wed, Aug 10, 2016 at 03:24:38PM +0200, Lars Schneider wrote:
quoted
On Wed, Aug 10, 2016 at 03:03:58PM +0200, larsxschneider@gmail.com wrote:
quoted
From: Lars Schneider <redacted>
The packet_trace() call is not ideal in format_packet() as we would print
a trace when a packet is formatted and (potentially) when the packet is
actually send. This was no problem up until now because format_packet()
was only used by one function. Fix it by moving the trace call into the
function that actually sends the packet.
It looks like there are two functions: packet_write() and
packet_buf_write().
I did not call trace in packet_buf_write() because this function does not
perform any writes.
Yes, but then who is responsible for the trace? The caller?
And why is it a bad thing to do it some time other than writing? It is
if you format and then _don't_ write the packet, but the current callers
are not doing that.
quoted
Your patch only touches one of them, and it looks like we would fail to
trace many packets (e.g., see receive-pack.c:report(), which uses
packet_buf_write() and then write()s out the result).
I see. But isn't it confusing if packet_buf_write() issues a trace call?
If I just call this function then nothing happens at all. Shouldn't the
trace call be made in receive-pack.c:report() ? Or shouldn't receive-pack
let pkt-line.c perform the write calls?
How would report() do that without re-parsing each of the packets?
-Peff
From: Lars Schneider <redacted>
packet_write() would die in case of a write error even though for some callers
an error would be acceptable. Add packet_write_gently_fmt() which writes a
formatted pkt-line and returns `0` for success and `-1` for an error.
Signed-off-by: Lars Schneider <redacted>
---
pkt-line.c | 13 +++++++++++++
pkt-line.h | 1 +
2 files changed, 14 insertions(+)
From: Lars Schneider <redacted>
Consider the case of a file that requires filtering and is present in branch A
but not in branch B. If A is the current HEAD and we checkout B then the
following happens:
1. ce_compare_data() opens the file
2. index_fd() detects that the file requires to run a clean filter and
calls index_stream_convert_blob()
4. index_stream_convert_blob() calls convert_to_git_filter_fd()
5. convert_to_git_filter_fd() calls apply_filter() which creates a new
long running filter process (in case it is the first file of this kind
to be filtered)
6. The new filter process inherits all file handles. This is the default
on Linux/OSX and is explicitly defined in the `CreateProcessW` call
in `mingw.c` on Windows.
7. ce_compare_data() closes the file
8. Git unlinks the file as it is not present in B
The unlink operation does not work on Windows because the filter process has
still an open handle to the file. Apparently that is no problem on Linux/OSX.
Probably because "[...] the two file descriptors share open file status flags"
(see fork(2)).
Fix this problem by opening files in read-cache with the `O_CLOEXEC` flag to
ensure that the file descriptor does not remain open in a newly spawned process.
`O_CLOEXEX` is defined as `O_NOINHERIT` on Windows. A similar fix for temporary
file handles was applied on Git for Windows already:
https://github.com/git-for-windows/git/commit/667b8b51ec850c3e1c7d75dee69dc13c29d1f162
Signed-off-by: Lars Schneider <redacted>
---
read-cache.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Jeff King <hidden> Date: 2016-08-10 19:04:51
On Wed, Aug 10, 2016 at 03:29:26PM +0200, Lars Schneider wrote:
quoted
On 10 Aug 2016, at 15:15, Jeff King [off-list ref] wrote:
On Wed, Aug 10, 2016 at 03:03:59PM +0200, larsxschneider@gmail.com wrote:
quoted
From: Lars Schneider <redacted>
format_packet() dies if the caller wants to format a packet larger than
LARGE_PACKET_MAX. Certain callers might prefer an error response instead.
I am not sure I agree here. Certainly I see the usefulness of gently
handling a failure to write(). But if you are passing in too-large
buffers, isn't that a bug in the program?
How would you recover, except by splitting up the content? That might
not be possible depending on how you are using the pkt-lines. And even
if it is, wouldn't it be simpler to split it up before sending it to
format_packet()?
Good argument. I agree - this patch should be dropped.
Actually, after reading further, one thought did occur to me. Let's say
you are writing to a smudge filter, and one of the header packets you
send has the filename in it. So you might do something like:
if (packet_write_fmt_gently(fd, "filename=%s", filename) < 0) {
if (filter_required)
die(...);
else
return -1; /* we tried our best; skip smudge */
}
The "recovery" there is not to try sending again, but rather to give up.
And that is presumably a sane outcome for somebody who tries to checkout
a filename larger than 64K.
It does still feel a little weird that you cannot tell the difference
between a write() error and bad input. Because you really might want to
do something different between the two. Like:
#define MAX_FILENAME (PKTLINE_DATA_MAXLEN - strlen("filename"))
if (filename > MAX_FILENAME) {
warning("woah, that name is ridiculous; truncating");
ret = packet_write_fmt_gently(fd, "%.*s", MAX_FILENAME, filename);
} else
ret = packet_write_fmt_gently(fd, "%s", filename);
-Peff
From: Lars Schneider <redacted>
set_packet_header() converts an integer to a 4 byte hex string. Make
this function locally available so that other pkt-line functions can
use it.
Signed-off-by: Lars Schneider <redacted>
---
pkt-line.c | 18 ++++++++++++------
1 file changed, 12 insertions(+), 6 deletions(-)
From: Lars Schneider <redacted>
According to LARGE_PACKET_MAX in pkt-line.h the maximal length of a
pkt-line packet is 65520 bytes. The pkt-line header takes 4 bytes and
therefore the pkt-line data component must not exceed 65516 bytes.
Signed-off-by: Lars Schneider <redacted>
---
Documentation/technical/protocol-common.txt | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
@@ -67,9 +67,9 @@ with non-binary data the same whether or not they contain the trailing LF (stripping the LF if present, and not complaining when it is missing).-The maximum length of a pkt-line's data component is 65520 bytes.-Implementations MUST NOT send pkt-line whose length exceeds 65524-(65520 bytes of payload + 4 bytes of length data).+The maximum length of a pkt-line's data component is 65516 bytes.+Implementations MUST NOT send pkt-line whose length exceeds 65520+(65516 bytes of payload + 4 bytes of length data). Implementations SHOULD NOT send an empty pkt-line ("0004").
Could the end of this function just be:
return packet_write_gently(fd, buf.buf, buf.len);
? I guess we'd prefer to avoid that, because it incurs an extra
memmove() of the data.
Similarly, I'd think this could share code with the non-gentle form
(which should be able to just call this and die() if returns an error).
Though sometimes the va_list transformation makes that awkward.
-Peff
From: Jeff King <hidden> Date: 2016-08-10 19:11:03
On Wed, Aug 10, 2016 at 03:36:45PM +0200, Lars Schneider wrote:
quoted
So now we have packet_write() and packet_write_gently(), but they differ
in more than just whether they are gentle. That seems like a weird
interface.
Should we either be picking a new name (e.g., packet_write_mem() or
something), or migrating packet_write() to packet_write_fmt()?
Done in "[PATCH v5 08/15] pkt-line: rename packet_write() to packet_write_fmt()"
Ah, OK. Generally I'd suggest to reorder things so that each patch looks
like a step forward (and so the early patches become preparatory steps,
and the justification in them is something like "we're going to add more
write functions, so let's give this a more descriptive name").
I agree. In a later patch I am using PKTLINE_DATA_MAXLEN inside pkt-line.c,
too. I will change it to your suggestion.
For now I would remove PKTLINE_DATA_MAXLEN because it should be an implementation
detail of pkt-line.c (plus it is not used by anyone).
From: Lars Schneider <redacted>
packet_flush() would die in case of a write error even though for some callers
an error would be acceptable. Add packet_flush_gently() which writes a pkt-line
flush packet and returns `0` for success and `-1` for failure.
Signed-off-by: Lars Schneider <redacted>
---
pkt-line.c | 6 ++++++
pkt-line.h | 1 +
2 files changed, 7 insertions(+)
From: Lars Schneider <redacted>
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 <redacted>
---
convert.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
@@ -397,7 +397,7 @@ static int filter_buffer_or_fd(int in, int out, void *data)child_process.out=out;if(start_command(&child_process))-returnerror("cannot fork to run external filter %s",params->cmd);+returnerror("cannot fork to run external filter '%s'",params->cmd);sigchain_push(SIGPIPE,SIG_IGN);
@@ -415,13 +415,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);
@@ -462,15 +462,15 @@ static int apply_filter(const char *path, const char *src, size_t len, int fd,return0;/* 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;}
From: Lars Schneider <hidden> Date: 2016-08-10 19:22:30
On 10 Aug 2016, at 15:30, Jeff King [off-list ref] wrote:
On Wed, Aug 10, 2016 at 03:24:38PM +0200, Lars Schneider wrote:
quoted
quoted
On Wed, Aug 10, 2016 at 03:03:58PM +0200, larsxschneider@gmail.com wrote:
quoted
From: Lars Schneider <redacted>
The packet_trace() call is not ideal in format_packet() as we would print
a trace when a packet is formatted and (potentially) when the packet is
actually send. This was no problem up until now because format_packet()
was only used by one function. Fix it by moving the trace call into the
function that actually sends the packet.
It looks like there are two functions: packet_write() and
packet_buf_write().
I did not call trace in packet_buf_write() because this function does not
perform any writes.
Yes, but then who is responsible for the trace? The caller?
From my point of view the one that issues the write call.
And why is it a bad thing to do it some time other than writing? It is
if you format and then _don't_ write the packet, but the current callers
are not doing that.
True, they don't do that. However, I don't think that is intuitive behavior
for future callers. If I call "format" and then trace tells me that a packet
was sent although it was not (yet).
quoted
quoted
Your patch only touches one of them, and it looks like we would fail to
trace many packets (e.g., see receive-pack.c:report(), which uses
packet_buf_write() and then write()s out the result).
I see. But isn't it confusing if packet_buf_write() issues a trace call?
If I just call this function then nothing happens at all. Shouldn't the
trace call be made in receive-pack.c:report() ? Or shouldn't receive-pack
let pkt-line.c perform the write calls?
How would report() do that without re-parsing each of the packets?
We could add a function to pkt-line that takes a list of strings and
generates a list of packets with a terminating flush packet out of it.
Then it sends the packets.
As a quick compromise we could also introduce a function "packet_buf_write_with_trace()"
that wraps the "packet_buf_write()" and calls trace. However, I can imagine that
would be perceived as ugly.
I guess my point is that I stumbled over the un-intutiive format_packet() behavior
and I wanted to improve the situation in a way that others don't run into this
trap. If you think that is no issue then it would be OK for me if we leave the
current behavior as is.
Thanks,
Lars
Could the end of this function just be:
return packet_write_gently(fd, buf.buf, buf.len);
? I guess we'd prefer to avoid that, because it incurs an extra
memmove() of the data.
I don't think the memmove would be that expensive. However, format_packet()
already creates the packet_header and packet_write_gently would do the same
again, no?
Yeah, I think you would want extra refactoring to have a shared common
function. I took a stab at it, but the result ends up pretty ugly; the
amount of boilerplate exceeds the duplication here (the really nasty
thing is that format_packet() is hard to split up, because the part you
want to switch out is in the middle, but it needs to keep some context
between the start and the end. In a higher level language you'd pass it
a callback to fill in the strbuf in the middle, but in C that just ends
up horrible).
quoted
Similarly, I'd think this could share code with the non-gentle form
(which should be able to just call this and die() if returns an error).
Though sometimes the va_list transformation makes that awkward.
Yeah, the code duplication annoyed me, too. va_list was the reason I did it
that way. Do you think that is something that needs to be addressed in the
series?
No, I don't think it needs to be. It's just a case of making sure that
the internals don't grow too crufty and unmanageable for future
maintainability.
-Peff
From: Lars Schneider <redacted>
packet_write_stream_with_flush_from_fd() and
packet_write_stream_with_flush_from_buf() write a stream of packets. All
content packets use the maximal packet size except for the last one.
After the last content packet a `flush` control packet is written.
packet_read_till_flush() reads arbitary sized packets until it detects
a `flush` packet.
Signed-off-by: Lars Schneider <redacted>
---
pkt-line.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
pkt-line.h | 7 +++++
2 files changed, 96 insertions(+)
From: Lars Schneider <redacted>
Generate more interesting large test files with pseudo random characters
in between and reuse these test files in multiple tests. Run tests formerly
marked as EXPENSIVE every time but with a smaller data set.
Signed-off-by: Lars Schneider <redacted>
---
t/t0021-conversion.sh | 48 ++++++++++++++++++++++++++++++++++++++----------
1 file changed, 38 insertions(+), 10 deletions(-)
@@ -31,7 +40,26 @@ test_expect_success setup 'cattest>test.i&&gitaddtesttest.ttest.i&&rm-ftesttest.ttest.i&&-gitcheckout--testtest.ttest.i+gitcheckout--testtest.ttest.i&&++mkdirgenerated-test-data&&+foriin$(test_seq1$T0021_LARGE_FILE_SIZE)+do+RANDOM_STRING="$(test-genrandomend$i|tr-dc"A-Za-z0-9")"+ROT_RANDOM_STRING="$(echo$RANDOM_STRING|./rot13.sh)"+# Generate 1MB of empty data and 100 bytes of random characters+# printf "$(test-genrandom start $i)"+printf"%1048576d"1>>generated-test-data/large.file&&+printf"$RANDOM_STRING">>generated-test-data/large.file&&+printf"%1048576d"1>>generated-test-data/large.file.rot13&&+printf"$ROT_RANDOM_STRING">>generated-test-data/large.file.rot13&&++iftest$i=$T0021_LARGISH_FILE_SIZE+then+catgenerated-test-data/large.file>generated-test-data/largish.file&&+catgenerated-test-data/large.file.rot13>generated-test-data/largish.file.rot13+fi+done'script='s/^\$Id: \([0-9a-f]*\) \$/\1/p'
@@ -199,9 +227,9 @@ test_expect_success 'required filter clean failure' ' test_expect_success'filtering large input to small output should use little memory''test_configfilter.devnull.clean"cat >/dev/null"&&test_configfilter.devnull.requiredtrue&&-foriin$(test_seq130);doprintf"%1048576d"1;done>30MB&&-echo"30MB filter=devnull">.gitattributes&&-GIT_MMAP_LIMIT=1mGIT_ALLOC_LIMIT=1mgitadd30MB+cpgenerated-test-data/large.filelarge.file&&+echo"large.file filter=devnull">.gitattributes&&+GIT_MMAP_LIMIT=1mGIT_ALLOC_LIMIT=1mgitaddlarge.file' test_expect_success'filter that does not read is fine''
@@ -214,15 +242,15 @@ test_expect_success 'filter that does not read is fine' 'test_cmpexpectactual'-test_expect_successEXPENSIVE'filter large file''+test_expect_success'filter large file''test_configfilter.largefile.smudgecat&&test_configfilter.largefile.cleancat&&-foriin$(test_seq12048);doprintf"%1048576d"1;done>2GB&&-echo"2GB filter=largefile">.gitattributes&&-gitadd2GB2>err&&+echo"large.file filter=largefile">.gitattributes&&+cpgenerated-test-data/large.filelarge.file&&+gitaddlarge.file2>err&&test_must_be_emptyerr&&-rm-f2GB&&-gitcheckout--2GB2>err&&+rm-flarge.file&&+gitcheckout--large.file2>err&&test_must_be_emptyerr'
From: Jeff King <hidden> Date: 2016-08-10 19:33:54
On Wed, Aug 10, 2016 at 03:04:00PM +0200, larsxschneider@gmail.com wrote:
From: Lars Schneider <redacted>
packet_write() has two shortcomings. First, it uses format_packet() which
lets the caller only send string data via "%s". That means it cannot be
used for arbitrary data that may contain NULs. Second, it will always
die on error.
Add packet_write_gently() which writes arbitrary data and returns `0` for
success and `-1` for an error.
So now we have packet_write() and packet_write_gently(), but they differ
in more than just whether they are gentle. That seems like a weird
interface.
Should we either be picking a new name (e.g., packet_write_mem() or
something), or migrating packet_write() to packet_write_fmt()?
It looks like this iteration drops the idea of callers using a
LARGE_PACKET_MAX buffer and only filling it at "buf+4" with
PKTLINE_DATA_MAXLEN bytes (which is fine).
I wonder if we still need PKTLINE_DATA_MAXLEN, or of it is just
obscuring things. The magic number "4" still appears separately here,
and it actually makes it harder to see that things are correct. I.e.,
doing:
if (size > sizeof(packet_write_buffer) - 4)
return -1;
memmove(packet_write_buffer + 4, buf, size);
is more obviously correct, because you do not have to wonder about the
relationship between the size of your buffer and the macro.
It might still be worth having PKTLINE_DATA_MAXLEN public, though, if
callers use it to size their input to packet_write_gently().
-Peff
From: Lars Schneider <redacted>
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 "<".
Signed-off-by: Lars Schneider <redacted>
---
t/t0021-conversion.sh | 62 +++++++++++++++++++++++++--------------------------
1 file changed, 31 insertions(+), 31 deletions(-)
@@ -38,8 +38,8 @@ script='s/^\$Id: \([0-9a-f]*\) \$/\1/p' test_expect_successcheck'-cmptest.otest&&-cmptest.otest.t&&+test_cmptest.otest&&+test_cmptest.otest.t&&# ident should be stripped in the repositorygitdiff--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"&&-gitcat-fileblob:test.t>test.r&&+gitcat-fileblob:test.t>test.r&&-./rot13.sh<test.o>test.t&&-cmptest.rtest.t+./rot13.sh<test.o>test.t&&+test_cmptest.rtest.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-gitconfigfilter.argc.smudge"sh ./argc.sh %f"&&+test_configfilter.argc.smudge"sh ./argc.sh %f"&&rm"$normal""$special"&&gitcheckout--"$normal""$special"&&
@@ -141,7 +141,7 @@ test_expect_success 'filter shell-escaped filenames' 'test_cmpexpect"$special"&&# do the same thing, but with more args in the filter expression-gitconfigfilter.argc.smudge"sh ./argc.sh %f --my-extra-arg"&&+test_configfilter.argc.smudge"sh ./argc.sh %f --my-extra-arg"&&rm"$normal""$special"&&gitcheckout--"$normal""$special"&&
@@ -197,8 +197,8 @@ test_expect_success 'required filter clean failure' '' test_expect_success'filtering large input to small output should use little memory''-gitconfigfilter.devnull.clean"cat >/dev/null"&&-gitconfigfilter.devnull.requiredtrue&&+test_configfilter.devnull.clean"cat >/dev/null"&&+test_configfilter.devnull.requiredtrue&&foriin$(test_seq130);doprintf"%1048576d"1;done>30MB&&echo"30MB filter=devnull">.gitattributes&&GIT_MMAP_LIMIT=1mGIT_ALLOC_LIMIT=1mgitadd30MB
@@ -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-genrandomfoo$((128*1024+1))>big&&echo"big filter=epipe">.gitattributes&&-gitconfigfilter.epipe.clean"echo xyzzy"&&+test_configfilter.epipe.clean"echo xyzzy"&&gitaddbig&&gitcat-fileblob:big>actual&&echoxyzzy>expect&&
@@ -215,20 +215,20 @@ test_expect_success 'filter that does not read is fine' '' test_expect_successEXPENSIVE'filter large file''-gitconfigfilter.largefile.smudgecat&&-gitconfigfilter.largefile.cleancat&&+test_configfilter.largefile.smudgecat&&+test_configfilter.largefile.cleancat&&foriin$(test_seq12048);doprintf"%1048576d"1;done>2GB&&echo"2GB filter=largefile">.gitattributes&&gitadd2GB2>err&&-!test-serr&&+test_must_be_emptyerr&&rm-f2GB&&gitcheckout--2GB2>err&&-!test-serr+test_must_be_emptyerr' test_expect_success"filter: clean empty file"'-gitconfigfilter.in-repo-header.clean"echo cleaned && cat"&&-gitconfigfilter.in-repo-header.smudge"sed 1d"&&+test_configfilter.in-repo-header.clean"echo cleaned && cat"&&+test_configfilter.in-repo-header.smudge"sed 1d"&&echo"empty-in-worktree filter=in-repo-header">>.gitattributes&&>empty-in-worktree&&
From: Lars Schneider <hidden> Date: 2016-08-10 19:38:30
On 10 Aug 2016, at 15:15, Jeff King [off-list ref] wrote:
On Wed, Aug 10, 2016 at 03:03:59PM +0200, larsxschneider@gmail.com wrote:
quoted
From: Lars Schneider <redacted>
format_packet() dies if the caller wants to format a packet larger than
LARGE_PACKET_MAX. Certain callers might prefer an error response instead.
I am not sure I agree here. Certainly I see the usefulness of gently
handling a failure to write(). But if you are passing in too-large
buffers, isn't that a bug in the program?
How would you recover, except by splitting up the content? That might
not be possible depending on how you are using the pkt-lines. And even
if it is, wouldn't it be simpler to split it up before sending it to
format_packet()?
Good argument. I agree - this patch should be dropped.
Thanks,
Lars
From: Lars Schneider <redacted>
packet_write() has two shortcomings. First, it uses format_packet() which
lets the caller only send string data via "%s". That means it cannot be
used for arbitrary data that may contain NULs. Second, it will always
die on error.
Add packet_write_gently() which writes arbitrary data and returns `0` for
success and `-1` for an error.
Signed-off-by: Lars Schneider <redacted>
---
pkt-line.c | 12 ++++++++++++
pkt-line.h | 1 +
2 files changed, 13 insertions(+)
From: Lars Schneider <redacted>
The packet_trace() call is not ideal in format_packet() as we would print
a trace when a packet is formatted and (potentially) when the packet is
actually send. This was no problem up until now because format_packet()
was only used by one function. Fix it by moving the trace call into the
function that actually sends the packet.
Signed-off-by: Lars Schneider <redacted>
---
pkt-line.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Lars Schneider <hidden> Date: 2016-08-10 19:46:17
On 10 Aug 2016, at 15:28, Jeff King [off-list ref] wrote:
On Wed, Aug 10, 2016 at 03:04:00PM +0200, larsxschneider@gmail.com wrote:
quoted
From: Lars Schneider <redacted>
packet_write() has two shortcomings. First, it uses format_packet() which
lets the caller only send string data via "%s". That means it cannot be
used for arbitrary data that may contain NULs. Second, it will always
die on error.
Add packet_write_gently() which writes arbitrary data and returns `0` for
success and `-1` for an error.
So now we have packet_write() and packet_write_gently(), but they differ
in more than just whether they are gentle. That seems like a weird
interface.
Should we either be picking a new name (e.g., packet_write_mem() or
something), or migrating packet_write() to packet_write_fmt()?
Done in "[PATCH v5 08/15] pkt-line: rename packet_write() to packet_write_fmt()"
It looks like this iteration drops the idea of callers using a
LARGE_PACKET_MAX buffer and only filling it at "buf+4" with
PKTLINE_DATA_MAXLEN bytes (which is fine).
I wonder if we still need PKTLINE_DATA_MAXLEN, or of it is just
obscuring things. The magic number "4" still appears separately here,
and it actually makes it harder to see that things are correct. I.e.,
doing:
if (size > sizeof(packet_write_buffer) - 4)
return -1;
memmove(packet_write_buffer + 4, buf, size);
is more obviously correct, because you do not have to wonder about the
relationship between the size of your buffer and the macro.
It might still be worth having PKTLINE_DATA_MAXLEN public, though, if
callers use it to size their input to packet_write_gently().
I agree. In a later patch I am using PKTLINE_DATA_MAXLEN inside pkt-line.c,
too. I will change it to your suggestion.
For now I would remove PKTLINE_DATA_MAXLEN because it should be an implementation
detail of pkt-line.c (plus it is not used by anyone).
Thanks,
Lars
From: Lars Schneider <redacted>
apply_filter() returns a boolean that tells the caller if it
"did convert or did not convert". The variable `ret` was used throughout
the function to track errors wheras `1` denoted success and `0` failure.
This is unusual for the Git source where `0` denotes success.
Rename the variable and flip its value to make the function easier
readable for Git developers.
Signed-off-by: Lars Schneider <redacted>
---
convert.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
From: Lars Schneider <redacted>
format_packet() dies if the caller wants to format a packet larger than
LARGE_PACKET_MAX. Certain callers might prefer an error response instead.
Add a parameter `gentle` to define if the function should signal an error
with the return value (gentle=1) or die (gentle=0).
Signed-off-by: Lars Schneider <redacted>
---
pkt-line.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
From: Lars Schneider <hidden> Date: 2016-08-10 20:01:31
On 10 Aug 2016, at 15:13, Jeff King [off-list ref] wrote:
On Wed, Aug 10, 2016 at 03:03:58PM +0200, larsxschneider@gmail.com wrote:
quoted
From: Lars Schneider <redacted>
The packet_trace() call is not ideal in format_packet() as we would print
a trace when a packet is formatted and (potentially) when the packet is
actually send. This was no problem up until now because format_packet()
was only used by one function. Fix it by moving the trace call into the
function that actually sends the packet.
It looks like there are two functions: packet_write() and
packet_buf_write().
I did not call trace in packet_buf_write() because this function does not
perform any writes.
Your patch only touches one of them, and it looks like we would fail to
trace many packets (e.g., see receive-pack.c:report(), which uses
packet_buf_write() and then write()s out the result).
I see. But isn't it confusing if packet_buf_write() issues a trace call?
If I just call this function then nothing happens at all. Shouldn't the
trace call be made in receive-pack.c:report() ? Or shouldn't receive-pack
let pkt-line.c perform the write calls?
-Lars
From: Lars Schneider <hidden> Date: 2016-08-10 21:14:58
On 10 Aug 2016, at 15:37, Jeff King [off-list ref] wrote:
On Wed, Aug 10, 2016 at 03:29:26PM +0200, Lars Schneider wrote:
quoted
quoted
On 10 Aug 2016, at 15:15, Jeff King [off-list ref] wrote:
On Wed, Aug 10, 2016 at 03:03:59PM +0200, larsxschneider@gmail.com wrote:
quoted
From: Lars Schneider <redacted>
format_packet() dies if the caller wants to format a packet larger than
LARGE_PACKET_MAX. Certain callers might prefer an error response instead.
I am not sure I agree here. Certainly I see the usefulness of gently
handling a failure to write(). But if you are passing in too-large
buffers, isn't that a bug in the program?
How would you recover, except by splitting up the content? That might
not be possible depending on how you are using the pkt-lines. And even
if it is, wouldn't it be simpler to split it up before sending it to
format_packet()?
Good argument. I agree - this patch should be dropped.
Actually, after reading further, one thought did occur to me. Let's say
you are writing to a smudge filter, and one of the header packets you
send has the filename in it. So you might do something like:
if (packet_write_fmt_gently(fd, "filename=%s", filename) < 0) {
if (filter_required)
die(...);
else
return -1; /* we tried our best; skip smudge */
}
The "recovery" there is not to try sending again, but rather to give up.
And that is presumably a sane outcome for somebody who tries to checkout
a filename larger than 64K.
Yes!
It does still feel a little weird that you cannot tell the difference
between a write() error and bad input. Because you really might want to
do something different between the two. Like:
#define MAX_FILENAME (PKTLINE_DATA_MAXLEN - strlen("filename"))
if (filename > MAX_FILENAME) {
warning("woah, that name is ridiculous; truncating");
ret = packet_write_fmt_gently(fd, "%.*s", MAX_FILENAME, filename);
} else
ret = packet_write_fmt_gently(fd, "%s", filename);
I can do that. However, I wouldn't truncate the filename as this
might create a weird outcome. I would just let the filter fail.
OK?
- Lars
Could the end of this function just be:
return packet_write_gently(fd, buf.buf, buf.len);
? I guess we'd prefer to avoid that, because it incurs an extra
memmove() of the data.
I don't think the memmove would be that expensive. However, format_packet()
already creates the packet_header and packet_write_gently would do the same
again, no?
Similarly, I'd think this could share code with the non-gentle form
(which should be able to just call this and die() if returns an error).
Though sometimes the va_list transformation makes that awkward.
Yeah, the code duplication annoyed me, too. va_list was the reason I did it
that way. Do you think that is something that needs to be addressed in the
series?
Thanks,
Lars
From: Stefan Beller <hidden> Date: 2016-08-12 16:33:27
On Wed, Aug 10, 2016 at 6:04 AM, [off-list ref] wrote:
From: Lars Schneider <redacted>
Git's clean/smudge mechanism invokes an external filter process for every
single blob that is affected by a filter. If Git filters a lot of blobs
then the startup time of the external filter processes can become a
significant part of the overall Git execution time.
In a preliminary performance test this developer used a clean/smudge filter
written in golang to filter 12,000 files. This process took 364s with the
existing filter mechanism and 5s with the new mechanism. See details here:
https://github.com/github/git-lfs/pull/1382
This patch adds the `filter.<driver>.process` string option which, if used,
keeps the external filter process running and processes all blobs with
the packet format (pkt-line) based protocol over standard input and standard
output described below.
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, a list of supported protocol
version numbers, and a flush packet. Git expects to read the
welcome message and one protocol version number from the
previously sent list. Afterwards Git sends a list of supported
capabilities 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
what follows is specific to version=2?
version 42 may deem capabilities a bad idea?
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
------------------------
Supported filter capabilities in version 2 are "clean" and
"smudge".
I assume version 2 is an example here and we actually start with v1?
Can you clarify why we need welcome messages?
(Is there a technical reason, or better debuggability for humans?)
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
Git sends the content split in zero or more pkt-line packets and a
flush packet to terminate content.
------------------------
packet: git> command=smudge\n
packet: git> pathname=path/testfile.dat\n
packet: git> 0000
packet: git> CONTENT
packet: git> 0000
------------------------
The filter is expected to respond with a list of "key=value" pairs
terminated with a flush packet. If the filter does not experience
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.
------------------------
packet: git< status=success\n
packet: git< 0000
packet: git< SMUDGED_CONTENT
packet: git< 0000
packet: git< 0000 # empty list!
------------------------
If the result content is empty then the filter is expected to respond
with a success status and an empty list.
------------------------
packet: git< status=success\n
packet: git< 0000
packet: git< 0000 # empty content!
packet: git< 0000 # empty list!
------------------------
Why do we need the last flush packet? We'd expect as many successes
as we send out contents? Do we plan on interleaving operation, i.e.
Git sends out 10 files but the filter process is not as fast as Git sending
out and the answers trickle in slowly?
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
------------------------
If the filter experiences an error during processing, then it can
send the status "error". 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=success\n
So the first success is meaningless essentially?
Would it make sense to move the sucess behind the content sending
in all cases?
packet: git< 0000
packet: git< HALF_WRITTEN_ERRONEOUS_CONTENT
packet: git< 0000
packet: git< status=error\n
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.
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.
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.
okay. I think you can omit most of the commit message as it is a duplicate
of the documentation?
Instead the commit message can answer questions that are not part of
the documentation. (See the questions above which can be summarized
as "Why do we do it this way and not differently?")
+ if (err || errno == EPIPE) {
+ if (!strcmp(filter_status.buf, "error")) {
+ /*
+ * The filter signaled a problem with the file.
+ */
/* This could go into a single line comment. */
+ } else if (!strcmp(filter_status.buf, "error-all")) {
+ /*
+ * The filter signaled a permanent problem. Don't try to filter
+ * files with the same command for the lifetime of the current
+ * Git process.
+ */
+ entry->supported_capabilities &= ~wanted_capability;
+ } else {
+ /*
+ * Something went wrong with the protocol filter.
+ * Force shutdown and restart if another blob requires filtering!
+ */
+ error("external filter '%s' failed", cmd);
failed .. Can you give more information to the user such that they can easier
debug? (blob/path or state / expected state)
From: Jeff King <hidden> Date: 2016-08-12 16:38:16
On Fri, Aug 12, 2016 at 09:33:18AM -0700, Stefan Beller wrote:
quoted
If the result content is empty then the filter is expected to respond
with a success status and an empty list.
------------------------
packet: git< status=success\n
packet: git< 0000
packet: git< 0000 # empty content!
packet: git< 0000 # empty list!
------------------------
Why do we need the last flush packet? We'd expect as many successes
as we send out contents? Do we plan on interleaving operation, i.e.
Git sends out 10 files but the filter process is not as fast as Git sending
out and the answers trickle in slowly?
There was prior discussion in the thread, but basically, it is there to
be able to signal an error that is encountered midway through sending
the file (i.e., to say "status=error"). If you do not have a final
flush, then you would send nothing, and the receiver would be left
wondering if you were successful, or if it simply did not get your error
report yet.
quoted
If the filter experiences an error during processing, then it can
send the status "error". 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=success\n
So the first success is meaningless essentially?
Would it make sense to move the sucess behind the content sending
in all cases?
No, the first success says "good so far, here's the file content". The
second says "I succeeded in sending you the file content".
You _can_ drop the first one, but it may be more convenient for the
receiver to know up-front that there was a failure.
-Peff
From: Stefan Beller <hidden> Date: 2016-08-12 16:48:26
On Fri, Aug 12, 2016 at 9:38 AM, Jeff King [off-list ref] wrote:
On Fri, Aug 12, 2016 at 09:33:18AM -0700, Stefan Beller wrote:
quoted
quoted
If the result content is empty then the filter is expected to respond
with a success status and an empty list.
------------------------
packet: git< status=success\n
packet: git< 0000
packet: git< 0000 # empty content!
packet: git< 0000 # empty list!
------------------------
Why do we need the last flush packet? We'd expect as many successes
as we send out contents? Do we plan on interleaving operation, i.e.
Git sends out 10 files but the filter process is not as fast as Git sending
out and the answers trickle in slowly?
There was prior discussion in the thread, but basically, it is there to
be able to signal an error that is encountered midway through sending
the file (i.e., to say "status=error"). If you do not have a final
flush, then you would send nothing, and the receiver would be left
wondering if you were successful, or if it simply did not get your error
report yet.
I did not follow the prior discussion, so I approached this review with
no prior knowledge from prior reviews, but instead read through and
was asking a lot of questions that came up immediately. In case my
questions are too dumb just omit them, but I thought they were good
material to answer in a commit message ("Why did we do it this way
and not differently").
Thanks for the explanation. So this is similar as the situation below
that we wait for the flush and then an error/success report?
quoted
quoted
If the filter experiences an error during processing, then it can
send the status "error". 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=success\n
So the first success is meaningless essentially?
Would it make sense to move the sucess behind the content sending
in all cases?
No, the first success says "good so far, here's the file content". The
second says "I succeeded in sending you the file content".
You _can_ drop the first one, but it may be more convenient for the
receiver to know up-front that there was a failure.
If there was a failure upfront, it would become
packet: git< 0000
# no content is encapsulated here
packet: git< 0000
packet: git< status=error\n
packet: git< 0000
so from a protocol side I'd claim it doesn't look bad.
I assume with convenient you mean the implementation
side of things?
If we do the success first and then error out halfway, we
still have to clean up, so I do not see how this impacts
implementation?
Thanks,
Stefan
From: Lars Schneider <hidden> Date: 2016-08-12 16:59:34
On 12 Aug 2016, at 18:33, Stefan Beller [off-list ref] wrote:
On Wed, Aug 10, 2016 at 6:04 AM, [off-list ref] wrote:
quoted
From: Lars Schneider <redacted>
Git's clean/smudge mechanism invokes an external filter process for every
single blob that is affected by a filter. If Git filters a lot of blobs
then the startup time of the external filter processes can become a
significant part of the overall Git execution time.
In a preliminary performance test this developer used a clean/smudge filter
written in golang to filter 12,000 files. This process took 364s with the
existing filter mechanism and 5s with the new mechanism. See details here:
https://github.com/github/git-lfs/pull/1382
This patch adds the `filter.<driver>.process` string option which, if used,
keeps the external filter process running and processes all blobs with
the packet format (pkt-line) based protocol over standard input and standard
output described below.
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, a list of supported protocol
version numbers, and a flush packet. Git expects to read the
welcome message and one protocol version number from the
previously sent list. Afterwards Git sends a list of supported
capabilities 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
what follows is specific to version=2?
version 42 may deem capabilities a bad idea?
"version=42" is just an example to show how the initialization could look
like in a distant future when we support even another protocol version.
You are correct, what follows is specific to version=2. I will state
that more clearly in the documentation.
Can you try to rephrase "version 42 may deem capabilities a bad idea?"
I am not sure I understand what you mean.
quoted
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
------------------------
Supported filter capabilities in version 2 are "clean" and
"smudge".
I assume version 2 is an example here and we actually start with v1?
No, it is actually called version 2 because I consider the current
clean/smudge protocol version 1.
Can you clarify why we need welcome messages?
(Is there a technical reason, or better debuggability for humans?)
The welcome message is necessary to distinguish the long running
filter protocol (v2) from the current one-shot filter protocol (v1).
This is becomes important if a users tries to use a v1 clean/smudge
filter with the v2 git config settings.
quoted
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
Git sends the content split in zero or more pkt-line packets and a
flush packet to terminate content.
------------------------
packet: git> command=smudge\n
packet: git> pathname=path/testfile.dat\n
packet: git> 0000
packet: git> CONTENT
packet: git> 0000
------------------------
The filter is expected to respond with a list of "key=value" pairs
terminated with a flush packet. If the filter does not experience
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.
------------------------
packet: git< status=success\n
packet: git< 0000
packet: git< SMUDGED_CONTENT
packet: git< 0000
packet: git< 0000 # empty list!
------------------------
If the result content is empty then the filter is expected to respond
with a success status and an empty list.
------------------------
packet: git< status=success\n
packet: git< 0000
packet: git< 0000 # empty content!
packet: git< 0000 # empty list!
------------------------
Why do we need the last flush packet? We'd expect as many successes
as we send out contents? Do we plan on interleaving operation, i.e.
Git sends out 10 files but the filter process is not as fast as Git sending
out and the answers trickle in slowly?
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
------------------------
If the filter experiences an error during processing, then it can
send the status "error". 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=success\n
So the first success is meaningless essentially?
Would it make sense to move the sucess behind the content sending
in all cases?
Again, I refer to Peff's answer.
quoted
packet: git< 0000
packet: git< HALF_WRITTEN_ERRONEOUS_CONTENT
packet: git< 0000
packet: git< status=error\n
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.
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.
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.
okay. I think you can omit most of the commit message as it is a duplicate
of the documentation?
Yes it duplicates the documentation.
Instead the commit message can answer questions that are not part of
the documentation. (See the questions above which can be summarized
as "Why do we do it this way and not differently?")
OK, point taken. I will write a new commit message for v6.
quoted
+ if (err || errno == EPIPE) {
+ if (!strcmp(filter_status.buf, "error")) {
+ /*
+ * The filter signaled a problem with the file.
+ */
/* This could go into a single line comment. */
OK, will change.
quoted
+ } else if (!strcmp(filter_status.buf, "error-all")) {
+ /*
+ * The filter signaled a permanent problem. Don't try to filter
+ * files with the same command for the lifetime of the current
+ * Git process.
+ */
+ entry->supported_capabilities &= ~wanted_capability;
+ } else {
+ /*
+ * Something went wrong with the protocol filter.
+ * Force shutdown and restart if another blob requires filtering!
+ */
+ error("external filter '%s' failed", cmd);
failed .. Can you give more information to the user such that they can easier
debug? (blob/path or state / expected state)
Agreed, will add!
However, we don't give this information with the current clean/smudge interface.
From: Stefan Beller <hidden> Date: 2016-08-12 17:07:16
On Fri, Aug 12, 2016 at 9:59 AM, Lars Schneider
[off-list ref] wrote:
The welcome message is necessary to distinguish the long running
filter protocol (v2) from the current one-shot filter protocol (v1).
This is becomes important if a users tries to use a v1 clean/smudge
filter with the v2 git config settings.
Oh I see, that's why we're at v2 now.
How do you distinguish between v1 and v2? Does the welcome message
need to follow a certain pattern to be recognized to make it v2+ ?
Thanks a lot for the review,
Lars
Sorry for repeating the questions (it seems I missed
a lot of the prior discussion), but I think these questions
may help future readers of the commit message,
Thanks,
Stefan
From: Lars Schneider <hidden> Date: 2016-08-12 17:09:08
On 12 Aug 2016, at 18:48, Stefan Beller [off-list ref] wrote:
On Fri, Aug 12, 2016 at 9:38 AM, Jeff King [off-list ref] wrote:
quoted
On Fri, Aug 12, 2016 at 09:33:18AM -0700, Stefan Beller wrote:
quoted
quoted
If the result content is empty then the filter is expected to respond
with a success status and an empty list.
------------------------
packet: git< status=success\n
packet: git< 0000
packet: git< 0000 # empty content!
packet: git< 0000 # empty list!
------------------------
Why do we need the last flush packet? We'd expect as many successes
as we send out contents? Do we plan on interleaving operation, i.e.
Git sends out 10 files but the filter process is not as fast as Git sending
out and the answers trickle in slowly?
There was prior discussion in the thread, but basically, it is there to
be able to signal an error that is encountered midway through sending
the file (i.e., to say "status=error"). If you do not have a final
flush, then you would send nothing, and the receiver would be left
wondering if you were successful, or if it simply did not get your error
report yet.
I did not follow the prior discussion, so I approached this review with
no prior knowledge from prior reviews, but instead read through and
was asking a lot of questions that came up immediately. In case my
questions are too dumb just omit them, but I thought they were good
material to answer in a commit message ("Why did we do it this way
and not differently").
Thanks! That's very helpful and I will address your questions in the commit
message as anyone looking at this commit in the future will have no prior
knowledge, too.
Thanks for the explanation. So this is similar as the situation below
that we wait for the flush and then an error/success report?
Correct. If we would just process the status packet then we wouldn't
even need to wait for the flush. I added flush because that allows us
to send an arbitrary list of key=value pairs in the future.
quoted
quoted
quoted
If the filter experiences an error during processing, then it can
send the status "error". 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=success\n
So the first success is meaningless essentially?
Would it make sense to move the sucess behind the content sending
in all cases?
No, the first success says "good so far, here's the file content". The
second says "I succeeded in sending you the file content".
You _can_ drop the first one, but it may be more convenient for the
receiver to know up-front that there was a failure.
If there was a failure upfront, it would become
packet: git< 0000
# no content is encapsulated here
packet: git< 0000
packet: git< status=error\n
packet: git< 0000
No, a failure upfront would look like this (see documentation):
------------------------
packet: git< status=error\n
packet: git< 0000
------------------------
No content and no 2nd key=value pair list is exchanged after an error.
so from a protocol side I'd claim it doesn't look bad.
I assume with convenient you mean the implementation
side of things?
If we do the success first and then error out halfway, we
still have to clean up, so I do not see how this impacts
implementation?
That is true. The reasoning is that an error in between is somewhat
less expected. Therefore additional work is OK.
An error upfront is much more likely because it is also a mechanism
for the filter to reject certain files. If the filter is configured
as "required=false" then this reject would actually be OK.
Thanks,
Lars
From: Lars Schneider <hidden> Date: 2016-08-12 17:14:07
On 12 Aug 2016, at 19:07, Stefan Beller [off-list ref] wrote:
On Fri, Aug 12, 2016 at 9:59 AM, Lars Schneider
[off-list ref] wrote:
quoted
The welcome message is necessary to distinguish the long running
filter protocol (v2) from the current one-shot filter protocol (v1).
This is becomes important if a users tries to use a v1 clean/smudge
filter with the v2 git config settings.
Oh I see, that's why we're at v2 now.
How do you distinguish between v1 and v2? Does the welcome message
need to follow a certain pattern to be recognized to make it v2+ ?
v1 has no format at all. It works like this:
1. Git starts the filter process
2. Git writes the entire file via stdin to the filter process
3. Git reads the result via stdout from the filter process
3. Git stops the filter process
Any v2+ would need to deal with the following:
packet: git> git-filter-client
packet: git> version=2
packet: git> version=2+
packet: git> 0000
packet: git< git-filter-server
Everything after could be different in v2+ compared to v2.
Thanks,
Lars
From: Johannes Schindelin <hidden> Date: 2016-08-18 14:24:58
Hi Lars,
On Wed, 10 Aug 2016, larsxschneider@gmail.com wrote:
From: Lars Schneider <redacted>
Consider the case of a file that requires filtering and is present in branch A
but not in branch B. If A is the current HEAD and we checkout B then the
following happens:
1. ce_compare_data() opens the file
2. index_fd() detects that the file requires to run a clean filter and
calls index_stream_convert_blob()
4. index_stream_convert_blob() calls convert_to_git_filter_fd()
5. convert_to_git_filter_fd() calls apply_filter() which creates a new
long running filter process (in case it is the first file of this kind
to be filtered)
6. The new filter process inherits all file handles. This is the default
on Linux/OSX and is explicitly defined in the `CreateProcessW` call
in `mingw.c` on Windows.
7. ce_compare_data() closes the file
8. Git unlinks the file as it is not present in B
The unlink operation does not work on Windows because the filter process has
still an open handle to the file. Apparently that is no problem on Linux/OSX.
Probably because "[...] the two file descriptors share open file status flags"
(see fork(2)).
We typically wrap the commit messages at 76 columns per row (personally,
I wrap already at 72, and it seems Junio wraps at 70).
Fix this problem by opening files in read-cache with the `O_CLOEXEC` flag to
ensure that the file descriptor does not remain open in a newly spawned process.
`O_CLOEXEX` is defined as `O_NOINHERIT` on Windows. A similar fix for temporary
In response to your commit note on GitHub, I submitted this patch series
(slightly cleaned up) yesterday (and you already commented on it):
https://public-inbox.org/git/cover.1471437637.git.johannes.schindelin@gmx.de
The patch is obviously correct, and needs the patch series I submitted to
compile on Windows (this note is more for Junio's interest than a comment
on this patch).
Ciao,
Dscho