* [PATCH v9 11/14] convert: make apply_filter() adhere to standard Git error handling
From: larsxschneider @ 2016-10-04 12:59 UTC (permalink / raw)
To: git; +Cc: ramsay, jnareb, gitster, j6t, tboegi, peff, mlbright,
Lars Schneider
In-Reply-To: <20161004125947.67104-1-larsxschneider@gmail.com>
From: Lars Schneider <larsxschneider@gmail.com>
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 whereas `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 <larsxschneider@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
convert.c | 15 ++++++---------
1 file changed, 6 insertions(+), 9 deletions(-)
diff --git a/convert.c b/convert.c
index 986c239..597f561 100644
--- a/convert.c
+++ b/convert.c
@@ -451,7 +451,7 @@ static int apply_filter(const char *path, const char *src, size_t len, int fd,
*
* (child --> cmd) --> us
*/
- int ret = 1;
+ int err = 0;
struct strbuf nbuf = STRBUF_INIT;
struct async async;
struct filter_params params;
@@ -477,23 +477,20 @@ static int apply_filter(const char *path, const char *src, size_t len, int fd,
return 0; /* error was already reported */
if (strbuf_read(&nbuf, async.out, len) < 0) {
- error("read from external filter '%s' failed", cmd);
- ret = 0;
+ err = error("read from external filter '%s' failed", cmd);
}
if (close(async.out)) {
- error("read from external filter '%s' failed", cmd);
- ret = 0;
+ err = error("read from external filter '%s' failed", cmd);
}
if (finish_async(&async)) {
- error("external filter '%s' failed", cmd);
- ret = 0;
+ err = error("external filter '%s' failed", cmd);
}
- if (ret) {
+ if (!err) {
strbuf_swap(dst, &nbuf);
}
strbuf_release(&nbuf);
- return ret;
+ return !err;
}
static struct convert_driver {
--
2.10.0
^ permalink raw reply related
* [PATCH v9 10/14] pkt-line: add functions to read/write flush terminated packet streams
From: larsxschneider @ 2016-10-04 12:59 UTC (permalink / raw)
To: git; +Cc: ramsay, jnareb, gitster, j6t, tboegi, peff, mlbright,
Lars Schneider
In-Reply-To: <20161004125947.67104-1-larsxschneider@gmail.com>
From: Lars Schneider <larsxschneider@gmail.com>
write_packetized_from_fd() and write_packetized_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.
read_packetized_to_strbuf() reads arbitrary sized packets until it
detects a `flush` packet.
Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
pkt-line.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
pkt-line.h | 8 ++++++++
2 files changed, 77 insertions(+)
diff --git a/pkt-line.c b/pkt-line.c
index 3fd4dc0..8ffde22 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -196,6 +196,47 @@ void packet_buf_write(struct strbuf *buf, const char *fmt, ...)
va_end(args);
}
+int write_packetized_from_fd(int fd_in, int fd_out)
+{
+ static char buf[LARGE_PACKET_DATA_MAX];
+ int err = 0;
+ ssize_t bytes_to_write;
+
+ while (!err) {
+ bytes_to_write = xread(fd_in, buf, sizeof(buf));
+ if (bytes_to_write < 0)
+ return COPY_READ_ERROR;
+ if (bytes_to_write == 0)
+ break;
+ err = packet_write_gently(fd_out, buf, bytes_to_write);
+ }
+ if (!err)
+ err = packet_flush_gently(fd_out);
+ return err;
+}
+
+int write_packetized_from_buf(const char *src_in, size_t len, int fd_out)
+{
+ static char buf[LARGE_PACKET_DATA_MAX];
+ int err = 0;
+ size_t bytes_written = 0;
+ size_t bytes_to_write;
+
+ while (!err) {
+ if ((len - bytes_written) > sizeof(buf))
+ bytes_to_write = sizeof(buf);
+ else
+ bytes_to_write = len - bytes_written;
+ if (bytes_to_write == 0)
+ break;
+ err = packet_write_gently(fd_out, src_in + bytes_written, bytes_to_write);
+ bytes_written += bytes_to_write;
+ }
+ if (!err)
+ err = packet_flush_gently(fd_out);
+ return err;
+}
+
static int get_packet_data(int fd, char **src_buf, size_t *src_size,
void *dst, unsigned size, int options)
{
@@ -305,3 +346,31 @@ char *packet_read_line_buf(char **src, size_t *src_len, int *dst_len)
{
return packet_read_line_generic(-1, src, src_len, dst_len);
}
+
+ssize_t read_packetized_to_strbuf(int fd_in, struct strbuf *sb_out)
+{
+ int packet_len;
+
+ size_t orig_len = sb_out->len;
+ size_t orig_alloc = sb_out->alloc;
+
+ for (;;) {
+ strbuf_grow(sb_out, LARGE_PACKET_DATA_MAX);
+ packet_len = packet_read(fd_in, NULL, NULL,
+ // TODO: explain + 1
+ sb_out->buf + sb_out->len, LARGE_PACKET_DATA_MAX+1,
+ PACKET_READ_GENTLE_ON_EOF);
+ if (packet_len <= 0)
+ break;
+ sb_out->len += packet_len;
+ }
+
+ if (packet_len < 0) {
+ if (orig_alloc == 0)
+ strbuf_release(sb_out);
+ else
+ strbuf_setlen(sb_out, orig_len);
+ return packet_len;
+ }
+ return sb_out->len - orig_len;
+}
diff --git a/pkt-line.h b/pkt-line.h
index 3fa0899..18eac64 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -25,6 +25,8 @@ void packet_buf_flush(struct strbuf *buf);
void packet_buf_write(struct strbuf *buf, const char *fmt, ...) __attribute__((format (printf, 2, 3)));
int packet_flush_gently(int fd);
int packet_write_fmt_gently(int fd, const char *fmt, ...) __attribute__((format (printf, 2, 3)));
+int write_packetized_from_fd(int fd_in, int fd_out);
+int write_packetized_from_buf(const char *src_in, size_t len, int fd_out);
/*
* Read a packetized line into the buffer, which must be at least size bytes
@@ -77,8 +79,14 @@ char *packet_read_line(int fd, int *size);
*/
char *packet_read_line_buf(char **src_buf, size_t *src_len, int *size);
+/*
+ * Reads a stream of variable sized packets until a flush packet is detected.
+ */
+ssize_t read_packetized_to_strbuf(int fd_in, struct strbuf *sb_out);
+
#define DEFAULT_PACKET_MAX 1000
#define LARGE_PACKET_MAX 65520
+#define LARGE_PACKET_DATA_MAX (LARGE_PACKET_MAX - 4)
extern char packet_buffer[LARGE_PACKET_MAX];
#endif
--
2.10.0
^ permalink raw reply related
* [PATCH v9 12/14] convert: prepare filter.<driver>.process option
From: larsxschneider @ 2016-10-04 12:59 UTC (permalink / raw)
To: git; +Cc: ramsay, jnareb, gitster, j6t, tboegi, peff, mlbright,
Lars Schneider
In-Reply-To: <20161004125947.67104-1-larsxschneider@gmail.com>
From: Lars Schneider <larsxschneider@gmail.com>
Refactor the existing 'single shot filter mechanism' and prepare the
new 'long running filter mechanism'.
Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
---
convert.c | 60 ++++++++++++++++++++++++++++++++++--------------------------
1 file changed, 34 insertions(+), 26 deletions(-)
diff --git a/convert.c b/convert.c
index 597f561..71e11ff 100644
--- a/convert.c
+++ b/convert.c
@@ -442,7 +442,7 @@ static int filter_buffer_or_fd(int in, int out, void *data)
return (write_err || status);
}
-static int apply_filter(const char *path, const char *src, size_t len, int fd,
+static int apply_single_file_filter(const char *path, const char *src, size_t len, int fd,
struct strbuf *dst, const char *cmd)
{
/*
@@ -456,12 +456,6 @@ static int apply_filter(const char *path, const char *src, size_t len, int fd,
struct async async;
struct filter_params params;
- if (!cmd || !*cmd)
- return 0;
-
- if (!dst)
- return 1;
-
memset(&async, 0, sizeof(async));
async.proc = filter_buffer_or_fd;
async.data = ¶ms;
@@ -493,6 +487,9 @@ 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)
+
static struct convert_driver {
const char *name;
struct convert_driver *next;
@@ -501,6 +498,29 @@ static struct convert_driver {
int required;
} *user_convert, **user_convert_tail;
+static int apply_filter(const char *path, const char *src, size_t len,
+ int fd, struct strbuf *dst, struct convert_driver *drv,
+ const unsigned int wanted_capability)
+{
+ const char *cmd = NULL;
+
+ if (!drv)
+ return 0;
+
+ if (!dst)
+ return 1;
+
+ if ((CAP_CLEAN & wanted_capability) && drv->clean)
+ cmd = drv->clean;
+ else if ((CAP_SMUDGE & wanted_capability) && drv->smudge)
+ cmd = drv->smudge;
+
+ if (cmd && *cmd)
+ return apply_single_file_filter(path, src, len, fd, dst, cmd);
+
+ return 0;
+}
+
static int read_convert_config(const char *var, const char *value, void *cb)
{
const char *key, *name;
@@ -839,7 +859,7 @@ int would_convert_to_git_filter_fd(const char *path)
if (!ca.drv->required)
return 0;
- return apply_filter(path, NULL, 0, -1, NULL, ca.drv->clean);
+ return apply_filter(path, NULL, 0, -1, NULL, ca.drv, CAP_CLEAN);
}
const char *get_convert_attr_ascii(const char *path)
@@ -872,18 +892,12 @@ int convert_to_git(const char *path, const char *src, size_t len,
struct strbuf *dst, enum safe_crlf checksafe)
{
int ret = 0;
- const char *filter = NULL;
- int required = 0;
struct conv_attrs ca;
convert_attrs(&ca, path);
- if (ca.drv) {
- filter = ca.drv->clean;
- required = ca.drv->required;
- }
- ret |= apply_filter(path, src, len, -1, dst, filter);
- if (!ret && required)
+ ret |= apply_filter(path, src, len, -1, dst, ca.drv, CAP_CLEAN);
+ if (!ret && ca.drv && ca.drv->required)
die("%s: clean filter '%s' failed", path, ca.drv->name);
if (ret && dst) {
@@ -907,7 +921,7 @@ void convert_to_git_filter_fd(const char *path, int fd, struct strbuf *dst,
assert(ca.drv);
assert(ca.drv->clean);
- if (!apply_filter(path, NULL, 0, fd, dst, ca.drv->clean))
+ if (!apply_filter(path, NULL, 0, fd, dst, ca.drv, CAP_CLEAN))
die("%s: clean filter '%s' failed", path, ca.drv->name);
crlf_to_git(path, dst->buf, dst->len, dst, ca.crlf_action, checksafe);
@@ -919,15 +933,9 @@ static int convert_to_working_tree_internal(const char *path, const char *src,
int normalizing)
{
int ret = 0, ret_filter = 0;
- const char *filter = NULL;
- int required = 0;
struct conv_attrs ca;
convert_attrs(&ca, path);
- if (ca.drv) {
- filter = ca.drv->smudge;
- required = ca.drv->required;
- }
ret |= ident_to_worktree(path, src, len, dst, ca.ident);
if (ret) {
@@ -938,7 +946,7 @@ static int convert_to_working_tree_internal(const char *path, const char *src,
* CRLF conversion can be skipped if normalizing, unless there
* is a smudge filter. The filter might expect CRLFs.
*/
- if (filter || !normalizing) {
+ if ((ca.drv && ca.drv->smudge) || !normalizing) {
ret |= crlf_to_worktree(path, src, len, dst, ca.crlf_action);
if (ret) {
src = dst->buf;
@@ -946,8 +954,8 @@ static int convert_to_working_tree_internal(const char *path, const char *src,
}
}
- ret_filter = apply_filter(path, src, len, -1, dst, filter);
- if (!ret_filter && required)
+ ret_filter = apply_filter(path, src, len, -1, dst, ca.drv, CAP_SMUDGE);
+ if (!ret_filter && ca.drv && ca.drv->required)
die("%s: smudge filter %s failed", path, ca.drv->name);
return ret | ret_filter;
--
2.10.0
^ permalink raw reply related
* [PATCH v9 13/14] convert: add filter.<driver>.process option
From: larsxschneider @ 2016-10-04 12:59 UTC (permalink / raw)
To: git; +Cc: ramsay, jnareb, gitster, j6t, tboegi, peff, mlbright,
Lars Schneider
In-Reply-To: <20161004125947.67104-1-larsxschneider@gmail.com>
From: Lars Schneider <larsxschneider@gmail.com>
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. The full protocol is explained in detail in
`Documentation/gitattributes.txt`.
A few key decisions:
* The long running filter process is referred to as filter protocol
version 2 because the existing single shot filter invocation is
considered version 1.
* Git sends a welcome message and expects a response right after the
external filter process has started. This ensures that Git will not
hang if a version 1 filter is incorrectly used with the
filter.<driver>.process option for version 2 filters. In addition,
Git can detect this kind of error and warn the user.
* The status of a filter operation (e.g. "success" or "error) is set
before the actual response and (if necessary!) re-set after the
response. The advantage of this two step status response is that if
the filter detects an error early, then the filter can communicate
this and Git does not even need to create structures to read the
response.
* All status responses are pkt-line lists terminated with a flush
packet. This allows us to send other status fields with the same
protocol in the future.
Helped-by: Martin-Louis Bright <mlbright@gmail.com>
Reviewed-by: Jakub Narebski <jnareb@gmail.com>
Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Documentation/gitattributes.txt | 157 +++++++++++++-
convert.c | 295 +++++++++++++++++++++++++-
t/t0021-conversion.sh | 447 +++++++++++++++++++++++++++++++++++++++-
t/t0021/rot13-filter.pl | 191 +++++++++++++++++
4 files changed, 1080 insertions(+), 10 deletions(-)
create mode 100755 t/t0021/rot13-filter.pl
diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index 7aff940..5868f00 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -293,7 +293,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.
@@ -373,6 +379,155 @@ not exist, or may have different contents. So, smudge and clean commands
should not try to access the file on disk, but only act as filters on the
content provided to them on standard input.
+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 a packet format (pkt-line,
+see technical/protocol-common.txt) based protocol over standard
+input and standard output as follows. All packets, except for the
+"*CONTENT" packets and the "0000" flush packet, are considered
+text and therefore are terminated by a LF.
+
+Git starts the filter when it encounters the first file
+that needs to be cleaned or smudged. After the filter started
+Git sends a welcome message ("git-filter-client"), a list of
+supported protocol version numbers, and a flush packet. Git expects
+to read a welcome response message ("git-filter-server") and exactly
+one protocol version number from the previously sent list. All further
+communication will be based on the selected version. The remaining
+protocol description below documents "version=2". Please note that
+"version=42" in the example below does not exist and is only there
+to illustrate how the protocol would look like with more than one
+version.
+
+After the version negotiation Git sends a list of all capabilities that
+it supports and a flush packet. Git expects to read a list of desired
+capabilities, which must be a subset of the supported capabilities list,
+and a flush packet as response:
+------------------------
+packet: git> git-filter-client
+packet: git> version=2
+packet: git> version=42
+packet: git> 0000
+packet: git< git-filter-server
+packet: git< version=2
+packet: git> clean=true
+packet: git> smudge=true
+packet: git> not-yet-invented=true
+packet: git> 0000
+packet: git< clean=true
+packet: git< smudge=true
+packet: git< 0000
+------------------------
+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. Please note, that the filter
+must not send any response before it received the content and the
+final flush packet.
+------------------------
+packet: git> command=smudge
+packet: git> pathname=path/testfile.dat
+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
+packet: git< 0000
+packet: git< SMUDGED_CONTENT
+packet: git< 0000
+packet: git< 0000 # empty list, keep "status=success" unchanged!
+------------------------
+
+If the result content is empty then the filter is expected to respond
+with a "success" status and an empty list.
+------------------------
+packet: git< status=success
+packet: git< 0000
+packet: git< 0000 # empty content!
+packet: git< 0000 # empty list, keep "status=success" unchanged!
+------------------------
+
+In case the filter cannot or does not want to process the content,
+it is expected to respond with an "error" status. Depending on the
+`filter.<driver>.required` flag Git will interpret that as error
+but it will not stop or restart the filter process.
+------------------------
+packet: git< status=error
+packet: git< 0000
+------------------------
+
+If the filter experiences an error during processing, then it can
+send the status "error" after the content was (partially or
+completely) sent. Depending on the `filter.<driver>.required` flag
+Git will interpret that as error but it will not stop or restart the
+filter process.
+------------------------
+packet: git< status=success
+packet: git< 0000
+packet: git< HALF_WRITTEN_ERRONEOUS_CONTENT
+packet: git< 0000
+packet: git< status=error
+packet: git< 0000
+------------------------
+
+If the filter dies during the communication or does not adhere to
+the protocol then Git will stop the filter process and restart it
+with the next file that needs to be processed. Depending on the
+`filter.<driver>.required` flag Git will interpret that as error.
+
+The error handling for all cases above mimic the behavior of
+the `filter.<driver>.clean` / `filter.<driver>.smudge` error
+handling.
+
+In case the filter cannot or does not want to process the content
+as well as any future content for the lifetime of the Git process,
+it is expected to respond with an "abort" status at any point in
+the protocol. Depending on the `filter.<driver>.required` flag Git
+will interpret that as error for the content as well as any future
+content for the lifetime of the Git process but it will not stop or
+restart the filter process.
+------------------------
+packet: git< status=abort
+packet: git< 0000
+------------------------
+
+After the filter has processed a blob it is expected to wait for
+the next "key=value" list containing a command. Git will close
+the command pipe on exit. The filter is expected to detect EOF
+and exit gracefully on its own.
+
+If you develop your own long running filter
+process then the `GIT_TRACE_PACKET` environment variables can be
+very helpful for debugging (see linkgit:git[1]).
+
+If a `filter.<driver>.process` command is configured then it
+always takes precedence over a configured `filter.<driver>.clean`
+or `filter.<driver>.smudge` command.
+
+Please note that you cannot use an existing `filter.<driver>.clean`
+or `filter.<driver>.smudge` command with `filter.<driver>.process`
+because the former two use a different inter process communication
+protocol than the latter one.
+
+
Interaction between checkin/checkout attributes
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/convert.c b/convert.c
index 71e11ff..88581d6 100644
--- a/convert.c
+++ b/convert.c
@@ -3,6 +3,7 @@
#include "run-command.h"
#include "quote.h"
#include "sigchain.h"
+#include "pkt-line.h"
/*
* convert.c - convert a file when checking it out and checking it in.
@@ -490,11 +491,287 @@ static int apply_single_file_filter(const char *path, const char *src, size_t le
#define CAP_CLEAN (1u<<0)
#define CAP_SMUDGE (1u<<1)
+struct cmd2process {
+ struct hashmap_entry ent; /* must be the first member! */
+ unsigned int supported_capabilities;
+ const char *cmd;
+ struct child_process process;
+};
+
+static int cmd_process_map_initialized;
+static struct hashmap cmd_process_map;
+
+static int cmd2process_cmp(const struct cmd2process *e1,
+ const struct cmd2process *e2,
+ const void *unused)
+{
+ return strcmp(e1->cmd, e2->cmd);
+}
+
+static struct cmd2process *find_multi_file_filter_entry(struct hashmap *hashmap, const char *cmd)
+{
+ struct cmd2process key;
+ hashmap_entry_init(&key, strhash(cmd));
+ key.cmd = cmd;
+ return hashmap_get(hashmap, &key, NULL);
+}
+
+static void kill_multi_file_filter(struct hashmap *hashmap, struct cmd2process *entry)
+{
+ if (!entry)
+ return;
+ sigchain_push(SIGPIPE, SIG_IGN);
+ /*
+ * We kill the filter most likely because an error happened already.
+ * That's why we are not interested in any error code here.
+ */
+ close(entry->process.in);
+ close(entry->process.out);
+ sigchain_pop(SIGPIPE);
+ finish_command(&entry->process);
+ hashmap_remove(hashmap, entry, NULL);
+ free(entry);
+}
+
+static int packet_write_list(int fd, const char *line, ...)
+{
+ va_list args;
+ int err;
+ va_start(args, line);
+ for (;;) {
+ if (!line)
+ break;
+ if (strlen(line) > LARGE_PACKET_DATA_MAX)
+ return -1;
+ err = packet_write_fmt_gently(fd, "%s\n", line);
+ if (err)
+ return err;
+ line = va_arg(args, const char*);
+ }
+ va_end(args);
+ return packet_flush_gently(fd);
+}
+
+static struct cmd2process *start_multi_file_filter(struct hashmap *hashmap, const char *cmd)
+{
+ int err;
+ struct cmd2process *entry;
+ struct child_process *process;
+ const char *argv[] = { cmd, NULL };
+ struct string_list cap_list = STRING_LIST_INIT_NODUP;
+ char *cap_buf;
+ const char *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;
+ process->wait_on_exit = 1;
+
+ if (start_command(process)) {
+ error("cannot fork to run external filter '%s'", cmd);
+ kill_multi_file_filter(hashmap, entry);
+ return NULL;
+ }
+
+ sigchain_push(SIGPIPE, SIG_IGN);
+
+ err = packet_write_list(process->in, "git-filter-client", "version=2", NULL);
+ if (err)
+ goto done;
+
+ err = strcmp(packet_read_line(process->out, NULL), "git-filter-server");
+ if (err) {
+ error("external filter '%s' does not support filter protocol version 2", cmd);
+ goto done;
+ }
+ err = strcmp(packet_read_line(process->out, NULL), "version=2");
+ if (err)
+ goto done;
+
+ 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;
+ } else if (!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);
+ return NULL;
+ }
+
+ hashmap_add(hashmap, entry);
+ return entry;
+}
+
+static void read_multi_file_filter_status(int fd, struct strbuf *status) {
+ struct strbuf **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]);
+ }
+ }
+ strbuf_list_free(pair);
+ }
+}
+
+static int apply_multi_file_filter(const char *path, const char *src, size_t len,
+ int fd, struct strbuf *dst, const char *cmd,
+ const unsigned int wanted_capability)
+{
+ int err;
+ struct cmd2process *entry;
+ struct child_process *process;
+ struct stat file_stat;
+ struct strbuf nbuf = STRBUF_INIT;
+ struct strbuf filter_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)
+ return 0;
+ }
+ process = &entry->process;
+
+ if (!(wanted_capability & entry->supported_capabilities))
+ return 0;
+
+ if (CAP_CLEAN & wanted_capability)
+ filter_type = "clean";
+ else if (CAP_SMUDGE & wanted_capability)
+ filter_type = "smudge";
+ else
+ die("unexpected filter type");
+
+ if (fd >= 0 && !src) {
+ if (fstat(fd, &file_stat) == -1)
+ return 0;
+ len = xsize_t(file_stat.st_size);
+ }
+
+ sigchain_push(SIGPIPE, SIG_IGN);
+
+ assert(strlen(filter_type) < LARGE_PACKET_DATA_MAX - strlen("command=\n"));
+ err = packet_write_fmt_gently(process->in, "command=%s\n", filter_type);
+ if (err)
+ goto done;
+
+ err = strlen(path) > LARGE_PACKET_DATA_MAX - strlen("pathname=\n");
+ if (err) {
+ error("path name too long for external filter");
+ goto done;
+ }
+
+ err = packet_write_fmt_gently(process->in, "pathname=%s\n", path);
+ if (err)
+ goto done;
+
+ err = packet_flush_gently(process->in);
+ if (err)
+ goto done;
+
+ if (fd >= 0)
+ err = write_packetized_from_fd(fd, process->in);
+ else
+ err = write_packetized_from_buf(src, len, process->in);
+ if (err)
+ goto done;
+
+ read_multi_file_filter_status(process->out, &filter_status);
+ err = strcmp(filter_status.buf, "success");
+ if (err)
+ goto done;
+
+ err = read_packetized_to_strbuf(process->out, &nbuf) < 0;
+ if (err)
+ goto done;
+
+ read_multi_file_filter_status(process->out, &filter_status);
+ err = strcmp(filter_status.buf, "success");
+
+done:
+ sigchain_pop(SIGPIPE);
+
+ if (err || errno == EPIPE) {
+ if (!strcmp(filter_status.buf, "error")) {
+ /* The filter signaled a problem with the file. */
+ } else if (!strcmp(filter_status.buf, "abort")) {
+ /*
+ * 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);
+ kill_multi_file_filter(&cmd_process_map, entry);
+ }
+ } else {
+ strbuf_swap(dst, &nbuf);
+ }
+ strbuf_release(&nbuf);
+ return !err;
+}
+
static struct convert_driver {
const char *name;
struct convert_driver *next;
const char *smudge;
const char *clean;
+ const char *process;
int required;
} *user_convert, **user_convert_tail;
@@ -510,13 +787,15 @@ static int apply_filter(const char *path, const char *src, size_t len,
if (!dst)
return 1;
- if ((CAP_CLEAN & wanted_capability) && drv->clean)
+ if ((CAP_CLEAN & wanted_capability) && !drv->process && drv->clean)
cmd = drv->clean;
- else if ((CAP_SMUDGE & wanted_capability) && drv->smudge)
+ else if ((CAP_SMUDGE & wanted_capability) && !drv->process && drv->smudge)
cmd = drv->smudge;
if (cmd && *cmd)
return apply_single_file_filter(path, src, len, fd, dst, cmd);
+ else if (drv->process && *drv->process)
+ return apply_multi_file_filter(path, src, len, fd, dst, drv->process, wanted_capability);
return 0;
}
@@ -558,6 +837,9 @@ static int read_convert_config(const char *var, const char *value, void *cb)
if (!strcmp("clean", key))
return git_config_string(&drv->clean, var, value);
+ if (!strcmp("process", key))
+ return git_config_string(&drv->process, var, value);
+
if (!strcmp("required", key)) {
drv->required = git_config_bool(var, value);
return 0;
@@ -919,7 +1201,7 @@ void convert_to_git_filter_fd(const char *path, int fd, struct strbuf *dst,
convert_attrs(&ca, path);
assert(ca.drv);
- assert(ca.drv->clean);
+ assert(ca.drv->clean || ca.drv->process);
if (!apply_filter(path, NULL, 0, fd, dst, ca.drv, CAP_CLEAN))
die("%s: clean filter '%s' failed", path, ca.drv->name);
@@ -944,9 +1226,10 @@ static int convert_to_working_tree_internal(const char *path, const char *src,
}
/*
* CRLF conversion can be skipped if normalizing, unless there
- * is a smudge filter. The filter might expect CRLFs.
+ * is a smudge or process filter (even if the process filter doesn't
+ * support smudge). The filters might expect CRLFs.
*/
- if ((ca.drv && ca.drv->smudge) || !normalizing) {
+ if ((ca.drv && (ca.drv->smudge || ca.drv->process)) || !normalizing) {
ret |= crlf_to_worktree(path, src, len, dst, ca.crlf_action);
if (ret) {
src = dst->buf;
@@ -1407,7 +1690,7 @@ struct stream_filter *get_stream_filter(const char *path, const unsigned char *s
struct stream_filter *filter = NULL;
convert_attrs(&ca, path);
- if (ca.drv && (ca.drv->smudge || ca.drv->clean))
+ if (ca.drv && (ca.drv->process || ca.drv->smudge || ca.drv->clean))
return NULL;
if (ca.crlf_action == CRLF_AUTO || ca.crlf_action == CRLF_AUTO_CRLF)
diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
index dc50938..52b7fe9 100755
--- a/t/t0021-conversion.sh
+++ b/t/t0021-conversion.sh
@@ -4,13 +4,77 @@ test_description='blob conversion via gitattributes'
. ./test-lib.sh
-cat <<EOF >rot13.sh
+TEST_ROOT="$(pwd)"
+
+cat <<EOF >"$TEST_ROOT/rot13.sh"
#!$SHELL_PATH
tr \
'abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ' \
'nopqrstuvwxyzabcdefghijklmNOPQRSTUVWXYZABCDEFGHIJKLM'
EOF
-chmod +x rot13.sh
+chmod +x "$TEST_ROOT/rot13.sh"
+
+generate_random_characters () {
+ LEN=$1
+ NAME=$2
+ test-genrandom some-seed $LEN |
+ perl -pe "s/./chr((ord($&) % 26) + ord('a'))/sge" >"$TEST_ROOT/$NAME"
+}
+
+file_size () {
+ cat "$1" | wc -c | sed "s/^[ ]*//"
+}
+
+filter_git () {
+ rm -f rot13-filter.log &&
+ git "$@" 2>git-stderr.log &&
+ sed '/Waiting for/d' git-stderr.log >git-stderr-clean.log &&
+ test_must_be_empty git-stderr-clean.log &&
+ rm -f git-stderr.log git-stderr-clean.log
+}
+
+# Count unique lines in two files and compare them.
+test_cmp_count () {
+ for FILE in $@
+ do
+ sort $FILE | uniq -c | sed "s/^[ ]*//" >$FILE.tmp
+ cat $FILE.tmp >$FILE
+ done &&
+ test_cmp $@
+}
+
+# Count unique lines except clean invocations in two files and compare
+# them. Clean invocations are not counted because their number can vary.
+# c.f. http://public-inbox.org/git/xmqqshv18i8i.fsf@gitster.mtv.corp.google.com/
+test_cmp_count_except_clean () {
+ for FILE in $@
+ do
+ sort $FILE | uniq -c | sed "s/^[ ]*//" |
+ sed "s/^\([0-9]\) IN: clean/x IN: clean/" >$FILE.tmp
+ cat $FILE.tmp >$FILE
+ done &&
+ test_cmp $@
+}
+
+# Compare two files but exclude clean invocations because they can vary.
+# c.f. http://public-inbox.org/git/xmqqshv18i8i.fsf@gitster.mtv.corp.google.com/
+test_cmp_exclude_clean () {
+ for FILE in $@
+ do
+ grep -v "IN: clean" $FILE >$FILE.tmp
+ cat $FILE.tmp >$FILE
+ done &&
+ test_cmp $@
+}
+
+# Check that the contents of two files are equal and that their rot13 version
+# is equal to the committed content.
+test_cmp_committed_rot13 () {
+ test_cmp "$1" "$2" &&
+ "$TEST_ROOT/rot13.sh" <"$1" >expected &&
+ git cat-file blob :"$2" >actual &&
+ test_cmp expected actual
+}
test_expect_success setup '
git config filter.rot13.smudge ./rot13.sh &&
@@ -31,7 +95,10 @@ test_expect_success setup '
cat test >test.i &&
git add test test.t test.i &&
rm -f test test.t test.i &&
- git checkout -- test test.t test.i
+ git checkout -- test test.t test.i &&
+
+ echo "content-test2" >test2.o &&
+ echo "content-test3 - filename with special characters" >"test3 '\''sq'\'',\$x.o"
'
script='s/^\$Id: \([0-9a-f]*\) \$/\1/p'
@@ -279,4 +346,378 @@ test_expect_success 'diff does not reuse worktree files that need cleaning' '
test_line_count = 0 count
'
+test_expect_success PERL 'required process filter should filter data' '
+ test_config_global filter.protocol.process "$TEST_DIRECTORY/t0021/rot13-filter.pl clean smudge" &&
+ test_config_global filter.protocol.required true &&
+ rm -rf repo &&
+ mkdir repo &&
+ (
+ cd repo &&
+ git init &&
+
+ echo "git-stderr.log" >.gitignore &&
+ echo "*.r filter=protocol" >.gitattributes &&
+ git add . &&
+ git commit . -m "test commit 1" &&
+ git branch empty-branch &&
+
+ cp "$TEST_ROOT/test.o" test.r &&
+ cp "$TEST_ROOT/test2.o" test2.r &&
+ mkdir testsubdir &&
+ cp "$TEST_ROOT/test3 '\''sq'\'',\$x.o" "testsubdir/test3 '\''sq'\'',\$x.r" &&
+ >test4-empty.r &&
+
+ S=$(file_size test.r) &&
+ S2=$(file_size test2.r) &&
+ S3=$(file_size "testsubdir/test3 '\''sq'\'',\$x.r") &&
+
+ filter_git add . &&
+ cat >expected.log <<-EOF &&
+ START
+ init handshake complete
+ IN: clean test.r $S [OK] -- OUT: $S . [OK]
+ IN: clean test2.r $S2 [OK] -- OUT: $S2 . [OK]
+ IN: clean test4-empty.r 0 [OK] -- OUT: 0 [OK]
+ IN: clean testsubdir/test3 '\''sq'\'',\$x.r $S3 [OK] -- OUT: $S3 . [OK]
+ STOP
+ EOF
+ test_cmp_count expected.log rot13-filter.log &&
+
+ filter_git commit . -m "test commit 2" &&
+ cat >expected.log <<-EOF &&
+ START
+ init handshake complete
+ IN: clean test.r $S [OK] -- OUT: $S . [OK]
+ IN: clean test2.r $S2 [OK] -- OUT: $S2 . [OK]
+ IN: clean test4-empty.r 0 [OK] -- OUT: 0 [OK]
+ IN: clean testsubdir/test3 '\''sq'\'',\$x.r $S3 [OK] -- OUT: $S3 . [OK]
+ IN: clean test.r $S [OK] -- OUT: $S . [OK]
+ IN: clean test2.r $S2 [OK] -- OUT: $S2 . [OK]
+ IN: clean test4-empty.r 0 [OK] -- OUT: 0 [OK]
+ IN: clean testsubdir/test3 '\''sq'\'',\$x.r $S3 [OK] -- OUT: $S3 . [OK]
+ STOP
+ EOF
+ test_cmp_count_except_clean expected.log rot13-filter.log &&
+
+ rm -f test2.r "testsubdir/test3 '\''sq'\'',\$x.r" &&
+
+ filter_git checkout --quiet --no-progress . &&
+ cat >expected.log <<-EOF &&
+ START
+ init handshake complete
+ IN: smudge test2.r $S2 [OK] -- OUT: $S2 . [OK]
+ IN: smudge testsubdir/test3 '\''sq'\'',\$x.r $S3 [OK] -- OUT: $S3 . [OK]
+ STOP
+ EOF
+ test_cmp_exclude_clean expected.log rot13-filter.log &&
+
+ filter_git checkout --quiet --no-progress empty-branch &&
+ cat >expected.log <<-EOF &&
+ START
+ init handshake complete
+ IN: clean test.r $S [OK] -- OUT: $S . [OK]
+ STOP
+ EOF
+ test_cmp_exclude_clean expected.log rot13-filter.log &&
+
+ filter_git checkout --quiet --no-progress master &&
+ cat >expected.log <<-EOF &&
+ START
+ init handshake complete
+ IN: smudge test.r $S [OK] -- OUT: $S . [OK]
+ IN: smudge test2.r $S2 [OK] -- OUT: $S2 . [OK]
+ IN: smudge test4-empty.r 0 [OK] -- OUT: 0 [OK]
+ IN: smudge testsubdir/test3 '\''sq'\'',\$x.r $S3 [OK] -- OUT: $S3 . [OK]
+ STOP
+ EOF
+ test_cmp_exclude_clean expected.log rot13-filter.log &&
+
+ test_cmp_committed_rot13 "$TEST_ROOT/test.o" test.r &&
+ test_cmp_committed_rot13 "$TEST_ROOT/test2.o" test2.r &&
+ test_cmp_committed_rot13 "$TEST_ROOT/test3 '\''sq'\'',\$x.o" "testsubdir/test3 '\''sq'\'',\$x.r"
+ )
+'
+
+test_expect_success PERL 'required process filter takes precedence' '
+ test_config_global filter.protocol.clean false &&
+ test_config_global filter.protocol.process "$TEST_DIRECTORY/t0021/rot13-filter.pl clean" &&
+ test_config_global filter.protocol.required true &&
+ rm -rf repo &&
+ mkdir repo &&
+ (
+ cd repo &&
+ git init &&
+
+ echo "*.r filter=protocol" >.gitattributes &&
+ cp "$TEST_ROOT/test.o" test.r &&
+ S=$(file_size test.r) &&
+
+ # Check that the process filter is invoked here
+ filter_git add . &&
+ cat >expected.log <<-EOF &&
+ START
+ init handshake complete
+ IN: clean test.r $S [OK] -- OUT: $S . [OK]
+ STOP
+ EOF
+ test_cmp_count expected.log rot13-filter.log
+ )
+'
+
+test_expect_success PERL 'required process filter should be used only for "clean" operation only' '
+ test_config_global filter.protocol.process "$TEST_DIRECTORY/t0021/rot13-filter.pl clean" &&
+ rm -rf repo &&
+ mkdir repo &&
+ (
+ cd repo &&
+ git init &&
+
+ echo "*.r filter=protocol" >.gitattributes &&
+ cp "$TEST_ROOT/test.o" test.r &&
+ S=$(file_size test.r) &&
+
+ filter_git add . &&
+ cat >expected.log <<-EOF &&
+ START
+ init handshake complete
+ IN: clean test.r $S [OK] -- OUT: $S . [OK]
+ STOP
+ EOF
+ test_cmp_count expected.log rot13-filter.log &&
+
+ rm test.r &&
+
+ filter_git checkout --quiet --no-progress . &&
+ # If the filter would be used for "smudge", too, we would see
+ # "IN: smudge test.r 57 [OK] -- OUT: 57 . [OK]" here
+ cat >expected.log <<-EOF &&
+ START
+ init handshake complete
+ STOP
+ EOF
+ test_cmp_exclude_clean expected.log rot13-filter.log
+ )
+'
+
+test_expect_success PERL 'required process filter should process multiple packets' '
+ test_config_global filter.protocol.process "$TEST_DIRECTORY/t0021/rot13-filter.pl clean smudge" &&
+ test_config_global filter.protocol.required true &&
+
+ rm -rf repo &&
+ mkdir repo &&
+ (
+ cd repo &&
+ git init &&
+
+ # Generate data requiring 1, 2, 3 packets
+ S=65516 && # PKTLINE_DATA_MAXLEN -> Maximal size of a packet
+ generate_random_characters $(($S )) 1pkt_1__.file &&
+ generate_random_characters $(($S +1)) 2pkt_1+1.file &&
+ generate_random_characters $(($S*2-1)) 2pkt_2-1.file &&
+ generate_random_characters $(($S*2 )) 2pkt_2__.file &&
+ generate_random_characters $(($S*2+1)) 3pkt_2+1.file &&
+
+ for FILE in "$TEST_ROOT"/*.file
+ do
+ cp "$FILE" . &&
+ "$TEST_ROOT/rot13.sh" <"$FILE" >"$FILE.rot13"
+ done &&
+
+ echo "*.file filter=protocol" >.gitattributes &&
+ filter_git add *.file .gitattributes &&
+ cat >expected.log <<-EOF &&
+ START
+ init handshake complete
+ IN: clean 1pkt_1__.file $(($S )) [OK] -- OUT: $(($S )) . [OK]
+ IN: clean 2pkt_1+1.file $(($S +1)) [OK] -- OUT: $(($S +1)) .. [OK]
+ IN: clean 2pkt_2-1.file $(($S*2-1)) [OK] -- OUT: $(($S*2-1)) .. [OK]
+ IN: clean 2pkt_2__.file $(($S*2 )) [OK] -- OUT: $(($S*2 )) .. [OK]
+ IN: clean 3pkt_2+1.file $(($S*2+1)) [OK] -- OUT: $(($S*2+1)) ... [OK]
+ STOP
+ EOF
+ test_cmp_count expected.log rot13-filter.log &&
+
+ rm -f *.file &&
+
+ filter_git checkout --quiet --no-progress -- *.file &&
+ cat >expected.log <<-EOF &&
+ START
+ init handshake complete
+ IN: smudge 1pkt_1__.file $(($S )) [OK] -- OUT: $(($S )) . [OK]
+ IN: smudge 2pkt_1+1.file $(($S +1)) [OK] -- OUT: $(($S +1)) .. [OK]
+ IN: smudge 2pkt_2-1.file $(($S*2-1)) [OK] -- OUT: $(($S*2-1)) .. [OK]
+ IN: smudge 2pkt_2__.file $(($S*2 )) [OK] -- OUT: $(($S*2 )) .. [OK]
+ IN: smudge 3pkt_2+1.file $(($S*2+1)) [OK] -- OUT: $(($S*2+1)) ... [OK]
+ STOP
+ EOF
+ test_cmp_exclude_clean expected.log rot13-filter.log &&
+
+ for FILE in *.file
+ do
+ test_cmp_committed_rot13 "$TEST_ROOT/$FILE" $FILE
+ done
+ )
+'
+
+test_expect_success PERL 'required process filter with clean error should fail' '
+ test_config_global filter.protocol.process "$TEST_DIRECTORY/t0021/rot13-filter.pl clean smudge" &&
+ test_config_global filter.protocol.required true &&
+ rm -rf repo &&
+ mkdir repo &&
+ (
+ cd repo &&
+ git init &&
+
+ echo "*.r filter=protocol" >.gitattributes &&
+
+ cp "$TEST_ROOT/test.o" test.r &&
+ echo "this is going to fail" >clean-write-fail.r &&
+ echo "content-test3-subdir" >test3.r &&
+
+ test_must_fail git add .
+ )
+'
+
+test_expect_success PERL 'process filter should restart after unexpected write failure' '
+ test_config_global filter.protocol.process "$TEST_DIRECTORY/t0021/rot13-filter.pl clean smudge" &&
+ rm -rf repo &&
+ mkdir repo &&
+ (
+ cd repo &&
+ git init &&
+
+ echo "*.r filter=protocol" >.gitattributes &&
+
+ cp "$TEST_ROOT/test.o" test.r &&
+ cp "$TEST_ROOT/test2.o" test2.r &&
+ echo "this is going to fail" >smudge-write-fail.o &&
+ cp smudge-write-fail.o smudge-write-fail.r &&
+
+ S=$(file_size test.r) &&
+ S2=$(file_size test2.r) &&
+ SF=$(file_size smudge-write-fail.r) &&
+
+ git add . &&
+ rm -f *.r &&
+
+ rm -f rot13-filter.log &&
+ git checkout --quiet --no-progress . 2>git-stderr.log &&
+
+ grep "smudge write error at" git-stderr.log &&
+ grep "error: external filter" git-stderr.log &&
+
+ cat >expected.log <<-EOF &&
+ START
+ init handshake complete
+ IN: smudge smudge-write-fail.r $SF [OK] -- OUT: $SF [WRITE FAIL]
+ START
+ init handshake complete
+ IN: smudge test.r $S [OK] -- OUT: $S . [OK]
+ IN: smudge test2.r $S2 [OK] -- OUT: $S2 . [OK]
+ STOP
+ EOF
+ test_cmp_exclude_clean expected.log rot13-filter.log &&
+
+ test_cmp_committed_rot13 "$TEST_ROOT/test.o" test.r &&
+ test_cmp_committed_rot13 "$TEST_ROOT/test2.o" test2.r &&
+
+ # Smudge failed
+ ! test_cmp smudge-write-fail.o smudge-write-fail.r &&
+ "$TEST_ROOT/rot13.sh" <smudge-write-fail.o >expected &&
+ git cat-file blob :smudge-write-fail.r >actual &&
+ test_cmp expected actual
+ )
+'
+
+test_expect_success PERL 'process filter should not be restarted if it signals an error' '
+ test_config_global filter.protocol.process "$TEST_DIRECTORY/t0021/rot13-filter.pl clean smudge" &&
+ rm -rf repo &&
+ mkdir repo &&
+ (
+ cd repo &&
+ git init &&
+
+ echo "*.r filter=protocol" >.gitattributes &&
+
+ cp "$TEST_ROOT/test.o" test.r &&
+ cp "$TEST_ROOT/test2.o" test2.r &&
+ echo "this will cause an error" >error.o &&
+ cp error.o error.r &&
+
+ S=$(file_size test.r) &&
+ S2=$(file_size test2.r) &&
+ SE=$(file_size error.r) &&
+
+ git add . &&
+ rm -f *.r &&
+
+ filter_git checkout --quiet --no-progress . &&
+ cat >expected.log <<-EOF &&
+ START
+ init handshake complete
+ IN: smudge error.r $SE [OK] -- OUT: 0 [ERROR]
+ IN: smudge test.r $S [OK] -- OUT: $S . [OK]
+ IN: smudge test2.r $S2 [OK] -- OUT: $S2 . [OK]
+ STOP
+ EOF
+ test_cmp_exclude_clean expected.log rot13-filter.log &&
+
+ test_cmp_committed_rot13 "$TEST_ROOT/test.o" test.r &&
+ test_cmp_committed_rot13 "$TEST_ROOT/test2.o" test2.r &&
+ test_cmp error.o error.r
+ )
+'
+
+test_expect_success PERL 'process filter signals abort once to abort processing of all future files' '
+ test_config_global filter.protocol.process "$TEST_DIRECTORY/t0021/rot13-filter.pl clean smudge" &&
+ rm -rf repo &&
+ mkdir repo &&
+ (
+ cd repo &&
+ git init &&
+
+ echo "*.r filter=protocol" >.gitattributes &&
+
+ cp "$TEST_ROOT/test.o" test.r &&
+ cp "$TEST_ROOT/test2.o" test2.r &&
+ echo "error this blob and all future blobs" >abort.o &&
+ cp abort.o abort.r &&
+
+ SA=$(file_size abort.r) &&
+
+ git add . &&
+ rm -f *.r &&
+
+ filter_git checkout --quiet --no-progress . &&
+ cat >expected.log <<-EOF &&
+ START
+ init handshake complete
+ IN: smudge abort.r $SA [OK] -- OUT: 0 [ABORT]
+ STOP
+ EOF
+ test_cmp_exclude_clean expected.log rot13-filter.log &&
+
+ test_cmp "$TEST_ROOT/test.o" test.r &&
+ test_cmp "$TEST_ROOT/test2.o" test2.r &&
+ test_cmp abort.o abort.r
+ )
+'
+
+test_expect_success PERL 'invalid process filter must fail (and not hang!)' '
+ test_config_global filter.protocol.process cat &&
+ test_config_global filter.protocol.required true &&
+ rm -rf repo &&
+ mkdir repo &&
+ (
+ cd repo &&
+ git init &&
+
+ echo "*.r filter=protocol" >.gitattributes &&
+
+ cp "$TEST_ROOT/test.o" test.r &&
+ test_must_fail git add . 2>git-stderr.log &&
+ grep "does not support filter protocol version" git-stderr.log
+ )
+'
+
test_done
diff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl
new file mode 100755
index 0000000..1a6959c
--- /dev/null
+++ b/t/t0021/rot13-filter.pl
@@ -0,0 +1,191 @@
+#!/usr/bin/perl
+#
+# Example implementation for the Git filter protocol version 2
+# See Documentation/gitattributes.txt, section "Filter Protocol"
+#
+# The script takes the list of supported protocol capabilities as
+# arguments ("clean", "smudge", etc).
+#
+# This implementation supports special test cases:
+# (1) If data with the pathname "clean-write-fail.r" is processed with
+# a "clean" operation then the write operation will die.
+# (2) If data with the pathname "smudge-write-fail.r" is processed with
+# a "smudge" operation then the write operation will die.
+# (3) If data with the pathname "error.r" is processed with any
+# operation then the filter signals that it cannot or does not want
+# to process the file.
+# (4) If data with the pathname "abort.r" is processed with any
+# operation then the filter signals that it cannot or does not want
+# to process the file and any file after that is processed with the
+# same command.
+#
+
+use strict;
+use warnings;
+
+my $MAX_PACKET_CONTENT_SIZE = 65516;
+my @capabilities = @ARGV;
+
+open my $debug, ">>", "rot13-filter.log" or die "cannot open log file: $!";
+
+sub rot13 {
+ my $str = shift;
+ $str =~ y/A-Za-z/N-ZA-Mn-za-m/;
+ return $str;
+}
+
+sub packet_bin_read {
+ my $buffer;
+ my $bytes_read = read STDIN, $buffer, 4;
+ if ( $bytes_read == 0 ) {
+ # EOF - Git stopped talking to us!
+ print $debug "STOP\n";
+ exit();
+ }
+ elsif ( $bytes_read != 4 ) {
+ die "invalid packet: '$buffer'";
+ }
+ my $pkt_size = hex($buffer);
+ if ( $pkt_size == 0 ) {
+ return ( 1, "" );
+ }
+ elsif ( $pkt_size > 4 ) {
+ my $content_size = $pkt_size - 4;
+ $bytes_read = read STDIN, $buffer, $content_size;
+ if ( $bytes_read != $content_size ) {
+ die "invalid packet ($content_size bytes expected; $bytes_read bytes read)";
+ }
+ return ( 0, $buffer );
+ }
+ else {
+ die "invalid packet size: $pkt_size";
+ }
+}
+
+sub packet_txt_read {
+ my ( $res, $buf ) = packet_bin_read();
+ unless ( $buf =~ s/\n$// ) {
+ die "A non-binary line MUST be terminated by an LF.";
+ }
+ return ( $res, $buf );
+}
+
+sub packet_bin_write {
+ my $buf = shift;
+ print STDOUT sprintf( "%04x", length($buf) + 4 );
+ print STDOUT $buf;
+ STDOUT->flush();
+}
+
+sub packet_txt_write {
+ packet_bin_write( $_[0] . "\n" );
+}
+
+sub packet_flush {
+ print STDOUT sprintf( "%04x", 0 );
+ STDOUT->flush();
+}
+
+print $debug "START\n";
+$debug->flush();
+
+( packet_txt_read() eq ( 0, "git-filter-client" ) ) || die "bad initialize";
+( packet_txt_read() eq ( 0, "version=2" ) ) || die "bad version";
+( packet_bin_read() eq ( 1, "" ) ) || die "bad version end";
+
+packet_txt_write("git-filter-server");
+packet_txt_write("version=2");
+
+( packet_txt_read() eq ( 0, "clean=true" ) ) || die "bad capability";
+( packet_txt_read() eq ( 0, "smudge=true" ) ) || die "bad capability";
+( packet_bin_read() eq ( 1, "" ) ) || die "bad capability end";
+
+foreach (@capabilities) {
+ packet_txt_write( $_ . "=true" );
+}
+packet_flush();
+print $debug "init handshake complete\n";
+$debug->flush();
+
+while (1) {
+ my ($command) = packet_txt_read() =~ /^command=([^=]+)$/;
+ print $debug "IN: $command";
+ $debug->flush();
+
+ my ($pathname) = packet_txt_read() =~ /^pathname=([^=]+)$/;
+ print $debug " $pathname";
+ $debug->flush();
+
+ # Flush
+ packet_bin_read();
+
+ my $input = "";
+ {
+ binmode(STDIN);
+ my $buffer;
+ my $done = 0;
+ while ( !$done ) {
+ ( $done, $buffer ) = packet_bin_read();
+ $input .= $buffer;
+ }
+ print $debug " " . length($input) . " [OK] -- ";
+ $debug->flush();
+ }
+
+ my $output;
+ if ( $pathname eq "error.r" or $pathname eq "abort.r" ) {
+ $output = "";
+ }
+ elsif ( $command eq "clean" and grep( /^clean$/, @capabilities ) ) {
+ $output = rot13($input);
+ }
+ elsif ( $command eq "smudge" and grep( /^smudge$/, @capabilities ) ) {
+ $output = rot13($input);
+ }
+ else {
+ die "bad command '$command'";
+ }
+
+ print $debug "OUT: " . length($output) . " ";
+ $debug->flush();
+
+ if ( $pathname eq "error.r" ) {
+ print $debug "[ERROR]\n";
+ $debug->flush();
+ packet_txt_write("status=error");
+ packet_flush();
+ }
+ elsif ( $pathname eq "abort.r" ) {
+ print $debug "[ABORT]\n";
+ $debug->flush();
+ packet_txt_write("status=abort");
+ packet_flush();
+ }
+ else {
+ packet_txt_write("status=success");
+ packet_flush();
+
+ if ( $pathname eq "${command}-write-fail.r" ) {
+ print $debug "[WRITE FAIL]\n";
+ $debug->flush();
+ die "${command} write error";
+ }
+
+ while ( length($output) > 0 ) {
+ my $packet = substr( $output, 0, $MAX_PACKET_CONTENT_SIZE );
+ packet_bin_write($packet);
+ # dots represent the number of packets
+ print $debug ".";
+ if ( length($output) > $MAX_PACKET_CONTENT_SIZE ) {
+ $output = substr( $output, $MAX_PACKET_CONTENT_SIZE );
+ }
+ else {
+ $output = "";
+ }
+ }
+ packet_flush();
+ print $debug " [OK]\n";
+ $debug->flush();
+ packet_flush();
+ }
+}
--
2.10.0
^ permalink raw reply related
* [PATCH v9 01/14] convert: quote filter names in error messages
From: larsxschneider @ 2016-10-04 12:59 UTC (permalink / raw)
To: git; +Cc: ramsay, jnareb, gitster, j6t, tboegi, peff, mlbright,
Lars Schneider
In-Reply-To: <20161004125947.67104-1-larsxschneider@gmail.com>
From: Lars Schneider <larsxschneider@gmail.com>
Git filter driver commands with spaces (e.g. `filter.sh foo`) are hard
to read in error messages. Quote them to improve the readability.
Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
convert.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/convert.c b/convert.c
index 077f5e6..986c239 100644
--- a/convert.c
+++ b/convert.c
@@ -412,7 +412,7 @@ static int filter_buffer_or_fd(int in, int out, void *data)
child_process.out = out;
if (start_command(&child_process))
- return error("cannot fork to run external filter %s", params->cmd);
+ return error("cannot fork to run external filter '%s'", params->cmd);
sigchain_push(SIGPIPE, SIG_IGN);
@@ -430,13 +430,13 @@ static int filter_buffer_or_fd(int in, int out, void *data)
if (close(child_process.in))
write_err = 1;
if (write_err)
- error("cannot feed the input to external filter %s", params->cmd);
+ error("cannot feed the input to external filter '%s'", params->cmd);
sigchain_pop(SIGPIPE);
status = finish_command(&child_process);
if (status)
- error("external filter %s failed %d", params->cmd, status);
+ error("external filter '%s' failed %d", params->cmd, status);
strbuf_release(&cmd);
return (write_err || status);
@@ -477,15 +477,15 @@ static int apply_filter(const char *path, const char *src, size_t len, int fd,
return 0; /* error was already reported */
if (strbuf_read(&nbuf, async.out, len) < 0) {
- error("read from external filter %s failed", cmd);
+ error("read from external filter '%s' failed", cmd);
ret = 0;
}
if (close(async.out)) {
- error("read from external filter %s failed", cmd);
+ error("read from external filter '%s' failed", cmd);
ret = 0;
}
if (finish_async(&async)) {
- error("external filter %s failed", cmd);
+ error("external filter '%s' failed", cmd);
ret = 0;
}
--
2.10.0
^ permalink raw reply related
* [PATCH v9 00/14] Git filter protocol
From: larsxschneider @ 2016-10-04 12:59 UTC (permalink / raw)
To: git; +Cc: ramsay, jnareb, gitster, j6t, tboegi, peff, mlbright,
Lars Schneider
From: Lars Schneider <larsxschneider@gmail.com>
The goal of this series is to avoid launching a new clean/smudge filter
process for each file that is filtered.
A short summary about v1 to v5 can be found here:
https://git.github.io/rev_news/2016/08/17/edition-18/
This series is also published on web:
https://github.com/larsxschneider/git/pull/13
Patches 1 and 2 are cleanups and not strictly necessary for the series.
Patches 3 to 12 are required preparation. Patch 13 is the main patch.
Patch 14 adds an example how to use the Git filter protocol in contrib.
Thanks a lot to
Ramsay, Jakub, Junio, Johannes, Torsten, and Peff
for very helpful reviews,
Lars
## Major changes since v8
* refactor test cases to make them more robust and more readable
* split main "convert: add filter" patch into three:
1. preparation patch
2. main patch
3. contrib patch
* add run-command "wait_on_exit" flag to fix flaky t0021, see discussion:
http://public-inbox.org/git/xmqq8tubitjs.fsf@gitster.mtv.corp.google.com/
## All changes since v8
### Ramsay
* make async_exit()
http://public-inbox.org/git/78f2bdd0-f6ad-db5c-f9f2-f90528bc4f77@ramsayjones.plus.com/
### Jacub
* improve commit message for:
pkt-line: rename packet_write() to packet_write_fmt
pkt-line: extract set_packet_header()
* remove unnecessary brackets in packet_write_gently
* introduce variable 'packet_size' to make packet_write_gently()
more readable
* replace 'PKTLINE_DATA_MAXLEN' with 'LARGE_PACKET_DATA_MAX'
* rename 'read_packetized_to_buf' to 'read_packetized_to_strbuf'
* use 'ssize_t' as proper return value for write_in_full()
* rename 'paket_len' to 'packet_len'
* remove unnecessary options variable in read_packetized_to_strbuf()
* rename variables in read_packetized_to_strbuf()
* fix +1 in read_packetized_to_strbuf()
* fix length comparison in packet_read()
* replace spaces with tabs in Perl scripts
* ... many smaller changes. See interdiff below.
## Interdiff (v8..v9)
```diff
diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index 946dcad..a182ef2 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -387,9 +387,9 @@ If the filter command (a string value) is defined via
single filter invocation for the entire life of a single Git
command. This is achieved by using a packet format (pkt-line,
see technical/protocol-common.txt) based protocol over standard
-input and standard output as follows. All packets are considered
-text and therefore are terminated by an LF. Exceptions are the
-"*CONTENT" packets and the flush packet.
+input and standard output as follows. All packets, except for the
+"*CONTENT" packets and the "0000" flush packet, are considered
+text and therefore are terminated by a LF.
Git starts the filter when it encounters the first file
that needs to be cleaned or smudged. After the filter started
@@ -403,10 +403,10 @@ protocol description below documents "version=2". Please note that
to illustrate how the protocol would look like with more than one
version.
-After the version negotiation 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:
+After the version negotiation Git sends a list of all capabilities that
+it supports and a flush packet. Git expects to read a list of desired
+capabilities, which must be a subset of the supported capabilities list,
+and a flush packet as response:
------------------------
packet: git> git-filter-client
packet: git> version=2
@@ -430,7 +430,9 @@ 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.
+flush packet to terminate content. Please note, that the filter
+must not send any response before it received the content and the
+final flush packet.
------------------------
packet: git> command=smudge
packet: git> pathname=path/testfile.dat
@@ -451,16 +453,16 @@ packet: git< status=success
packet: git< 0000
packet: git< SMUDGED_CONTENT
packet: git< 0000
-packet: git< 0000 # empty list!
+packet: git< 0000 # empty list, keep "status=success" unchanged!
------------------------
If the result content is empty then the filter is expected to respond
-with a success status and an empty list.
+with a "success" status and an empty list.
------------------------
packet: git< status=success
packet: git< 0000
packet: git< 0000 # empty content!
-packet: git< 0000 # empty list!
+packet: git< 0000 # empty list, keep "status=success" unchanged!
------------------------
In case the filter cannot or does not want to process the content,
@@ -497,10 +499,11 @@ handling.
In case the filter cannot or does not want to process the content
as well as any future content for the lifetime of the Git process,
-it is expected to respond with an "abort" status. Depending on
-the `filter.<driver>.required` flag Git will interpret that as error
-for the content as well as any future content for the lifetime of the
-Git process but it will not stop or restart the filter process.
+it is expected to respond with an "abort" status at any point in
+the protocol. Depending on the `filter.<driver>.required` flag Git
+will interpret that as error for the content as well as any future
+content for the lifetime of the Git process but it will not stop or
+restart the filter process.
------------------------
packet: git< status=abort
packet: git< 0000
diff --git a/contrib/long-running-filter/example.pl b/contrib/long-running-filter/example.pl
index c13a631..f4102d2 100755
--- a/contrib/long-running-filter/example.pl
+++ b/contrib/long-running-filter/example.pl
@@ -3,6 +3,9 @@
# Example implementation for the Git filter protocol version 2
# See Documentation/gitattributes.txt, section "Filter Protocol"
#
+# Please note, this pass-thru filter is a minimal skeleton. No proper
+# error handling was implemented.
+#
use strict;
use warnings;
@@ -18,7 +21,7 @@ sub packet_bin_read {
exit();
}
elsif ( $bytes_read != 4 ) {
- die "invalid packet size '$bytes_read' field";
+ die "invalid packet: '$buffer'";
}
my $pkt_size = hex($buffer);
if ( $pkt_size == 0 ) {
@@ -28,27 +31,27 @@ sub packet_bin_read {
my $content_size = $pkt_size - 4;
$bytes_read = read STDIN, $buffer, $content_size;
if ( $bytes_read != $content_size ) {
- die "invalid packet ($content_size expected; $bytes_read read)";
+ die "invalid packet ($content_size bytes expected; $bytes_read bytes read)";
}
return ( 0, $buffer );
}
else {
- die "invalid packet size";
+ die "invalid packet size: $pkt_size";
}
}
sub packet_txt_read {
my ( $res, $buf ) = packet_bin_read();
- unless ( $buf =~ /\n$/ ) {
- die "A non-binary line SHOULD BE terminated by an LF.";
+ unless ( $buf =~ s/\n$// ) {
+ die "A non-binary line MUST be terminated by an LF.";
}
- return ( $res, substr( $buf, 0, -1 ) );
+ return ( $res, $buf );
}
sub packet_bin_write {
- my ($packet) = @_;
- print STDOUT sprintf( "%04x", length($packet) + 4 );
- print STDOUT $packet;
+ my $buf = shift;
+ print STDOUT sprintf( "%04x", length($buf) + 4 );
+ print STDOUT $buf;
STDOUT->flush();
}
@@ -119,5 +122,6 @@ while (1) {
}
}
packet_flush(); # flush content!
- packet_flush(); # empty list!
+ packet_flush(); # empty list, keep "status=success" unchanged!
+
}
diff --git a/convert.c b/convert.c
index bd66257..88581d6 100644
--- a/convert.c
+++ b/convert.c
@@ -541,7 +541,7 @@ static int packet_write_list(int fd, const char *line, ...)
for (;;) {
if (!line)
break;
- if (strlen(line) > PKTLINE_DATA_MAXLEN)
+ if (strlen(line) > LARGE_PACKET_DATA_MAX)
return -1;
err = packet_write_fmt_gently(fd, "%s\n", line);
if (err)
@@ -573,6 +573,7 @@ static struct cmd2process *start_multi_file_filter(struct hashmap *hashmap, cons
process->use_shell = 1;
process->in = -1;
process->out = -1;
+ process->wait_on_exit = 1;
if (start_command(process)) {
error("cannot fork to run external filter '%s'", cmd);
@@ -588,7 +589,7 @@ static struct cmd2process *start_multi_file_filter(struct hashmap *hashmap, cons
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);
+ error("external filter '%s' does not support filter protocol version 2", cmd);
goto done;
}
err = strcmp(packet_read_line(process->out, NULL), "version=2");
@@ -634,7 +635,7 @@ static struct cmd2process *start_multi_file_filter(struct hashmap *hashmap, cons
return entry;
}
-static void read_multi_file_filter_values(int fd, struct strbuf *status) {
+static void read_multi_file_filter_status(int fd, struct strbuf *status) {
struct strbuf **pair;
char *line;
for (;;) {
@@ -648,6 +649,7 @@ static void read_multi_file_filter_values(int fd, struct strbuf *status) {
strbuf_addbuf(status, pair[1]);
}
}
+ strbuf_list_free(pair);
}
}
@@ -698,17 +700,16 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len
sigchain_push(SIGPIPE, SIG_IGN);
- err = strlen(filter_type) > PKTLINE_DATA_MAXLEN;
- if (err)
- goto done;
-
+ assert(strlen(filter_type) < LARGE_PACKET_DATA_MAX - strlen("command=\n"));
err = packet_write_fmt_gently(process->in, "command=%s\n", filter_type);
if (err)
goto done;
- err = strlen(path) > PKTLINE_DATA_MAXLEN;
- if (err)
+ err = strlen(path) > LARGE_PACKET_DATA_MAX - strlen("pathname=\n");
+ if (err) {
+ error("path name too long for external filter");
goto done;
+ }
err = packet_write_fmt_gently(process->in, "pathname=%s\n", path);
if (err)
@@ -725,16 +726,16 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len
if (err)
goto done;
- read_multi_file_filter_values(process->out, &filter_status);
+ read_multi_file_filter_status(process->out, &filter_status);
err = strcmp(filter_status.buf, "success");
if (err)
goto done;
- err = read_packetized_to_buf(process->out, &nbuf) < 0;
+ err = read_packetized_to_strbuf(process->out, &nbuf) < 0;
if (err)
goto done;
- read_multi_file_filter_values(process->out, &filter_status);
+ read_multi_file_filter_status(process->out, &filter_status);
err = strcmp(filter_status.buf, "success");
done:
@@ -753,7 +754,7 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len
} else {
/*
* Something went wrong with the protocol filter.
- * Force shutdown and restart if another blob requires filtering!
+ * Force shutdown and restart if another blob requires filtering.
*/
error("external filter '%s' failed", cmd);
kill_multi_file_filter(&cmd_process_map, entry);
@@ -786,9 +787,9 @@ static int apply_filter(const char *path, const char *src, size_t len,
if (!dst)
return 1;
- if (!drv->process && (CAP_CLEAN & wanted_capability) && drv->clean)
+ if ((CAP_CLEAN & wanted_capability) && !drv->process && drv->clean)
cmd = drv->clean;
- else if (!drv->process && (CAP_SMUDGE & wanted_capability) && drv->smudge)
+ else if ((CAP_SMUDGE & wanted_capability) && !drv->process && drv->smudge)
cmd = drv->smudge;
if (cmd && *cmd)
diff --git a/pkt-line.c b/pkt-line.c
index a0a8543..b82aaca 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -137,7 +137,7 @@ static int packet_write_fmt_1(int fd, int gently,
const char *fmt, va_list args)
{
struct strbuf buf = STRBUF_INIT;
- size_t count;
+ ssize_t count;
format_packet(&buf, fmt, args);
count = write_in_full(fd, buf.buf, buf.len);
@@ -174,15 +174,15 @@ int packet_write_fmt_gently(int fd, const char *fmt, ...)
static int packet_write_gently(const int fd_out, const char *buf, size_t size)
{
static char packet_write_buffer[LARGE_PACKET_MAX];
+ const size_t packet_size = size + 4;
- if (size > sizeof(packet_write_buffer) - 4) {
+ if (packet_size > sizeof(packet_write_buffer))
return error("packet write failed - data exceeds max packet size");
- }
+
packet_trace(buf, size, 1);
- size += 4;
- set_packet_header(packet_write_buffer, size);
- memcpy(packet_write_buffer + 4, buf, size - 4);
- if (write_in_full(fd_out, packet_write_buffer, size) == size)
+ set_packet_header(packet_write_buffer, packet_size);
+ memcpy(packet_write_buffer + 4, buf, size);
+ if (write_in_full(fd_out, packet_write_buffer, packet_size) == packet_size)
return 0;
return error("packet write failed");
}
@@ -198,7 +198,7 @@ void packet_buf_write(struct strbuf *buf, const char *fmt, ...)
int write_packetized_from_fd(int fd_in, int fd_out)
{
- static char buf[PKTLINE_DATA_MAXLEN];
+ static char buf[LARGE_PACKET_DATA_MAX];
int err = 0;
ssize_t bytes_to_write;
@@ -217,7 +217,7 @@ int write_packetized_from_fd(int fd_in, int fd_out)
int write_packetized_from_buf(const char *src_in, size_t len, int fd_out)
{
- static char buf[PKTLINE_DATA_MAXLEN];
+ static char buf[LARGE_PACKET_DATA_MAX];
int err = 0;
size_t bytes_written = 0;
size_t bytes_to_write;
@@ -347,29 +347,34 @@ char *packet_read_line_buf(char **src, size_t *src_len, int *dst_len)
return packet_read_line_generic(-1, src, src_len, dst_len);
}
-ssize_t read_packetized_to_buf(int fd_in, struct strbuf *sb_out)
+ssize_t read_packetized_to_strbuf(int fd_in, struct strbuf *sb_out)
{
- int paket_len;
- int options = PACKET_READ_GENTLE_ON_EOF;
+ int packet_len;
- size_t oldlen = sb_out->len;
- size_t oldalloc = sb_out->alloc;
+ size_t orig_len = sb_out->len;
+ size_t orig_alloc = sb_out->alloc;
for (;;) {
- strbuf_grow(sb_out, PKTLINE_DATA_MAXLEN+1);
- paket_len = packet_read(fd_in, NULL, NULL,
- sb_out->buf + sb_out->len, PKTLINE_DATA_MAXLEN+1, options);
- if (paket_len <= 0)
+ strbuf_grow(sb_out, LARGE_PACKET_DATA_MAX);
+ packet_len = packet_read(fd_in, NULL, NULL,
+ /* strbuf_grow() above always allocates one extra byte to
+ * store a '\0' at the end of the string. packet_read()
+ * writes a '\0' extra byte at the end, too. Let it know
+ * that there is already room for the extra byte.
+ */
+ sb_out->buf + sb_out->len, LARGE_PACKET_DATA_MAX+1,
+ PACKET_READ_GENTLE_ON_EOF);
+ if (packet_len <= 0)
break;
- sb_out->len += paket_len;
+ sb_out->len += packet_len;
}
- if (paket_len < 0) {
- if (oldalloc == 0)
+ if (packet_len < 0) {
+ if (orig_alloc == 0)
strbuf_release(sb_out);
else
- strbuf_setlen(sb_out, oldlen);
- return paket_len;
+ strbuf_setlen(sb_out, orig_len);
+ return packet_len;
}
- return sb_out->len - oldlen;
+ return sb_out->len - orig_len;
}
diff --git a/pkt-line.h b/pkt-line.h
index 3d873f3..18eac64 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -82,11 +82,11 @@ char *packet_read_line_buf(char **src_buf, size_t *src_len, int *size);
/*
* Reads a stream of variable sized packets until a flush packet is detected.
*/
-ssize_t read_packetized_to_buf(int fd_in, struct strbuf *sb_out);
+ssize_t read_packetized_to_strbuf(int fd_in, struct strbuf *sb_out);
#define DEFAULT_PACKET_MAX 1000
#define LARGE_PACKET_MAX 65520
-#define PKTLINE_DATA_MAXLEN (LARGE_PACKET_MAX - 4)
+#define LARGE_PACKET_DATA_MAX (LARGE_PACKET_MAX - 4)
extern char packet_buffer[LARGE_PACKET_MAX];
#endif
diff --git a/run-command.c b/run-command.c
index b72f6d1..96c54fe 100644
--- a/run-command.c
+++ b/run-command.c
@@ -6,19 +6,6 @@
#include "thread-utils.h"
#include "strbuf.h"
-void check_pipe(int err)
-{
- if (err == EPIPE) {
- if (in_async())
- async_exit(141);
-
- signal(SIGPIPE, SIG_DFL);
- raise(SIGPIPE);
- /* Should never happen, but just in case... */
- exit(141);
- }
-}
-
void child_process_init(struct child_process *child)
{
memset(child, 0, sizeof(*child));
@@ -34,6 +21,9 @@ void child_process_clear(struct child_process *child)
struct child_to_clean {
pid_t pid;
+ char *name;
+ int stdin;
+ int wait;
struct child_to_clean *next;
};
static struct child_to_clean *children_to_clean;
@@ -41,14 +31,35 @@ static int installed_child_cleanup_handler;
static void cleanup_children(int sig, int in_signal)
{
- while (children_to_clean) {
+ int status;
struct child_to_clean *p = children_to_clean;
+
+ /* Close the the child's stdin as indicator that Git will exit soon */
+ while (p) {
+ if (p->wait)
+ if (p->stdin > 0)
+ close(p->stdin);
+ p = p->next;
+ }
+
+ while (children_to_clean) {
+ p = children_to_clean;
children_to_clean = p->next;
+
+ if (p->wait) {
+ fprintf(stderr, _("Waiting for '%s' to finish..."), p->name);
+ while ((waitpid(p->pid, &status, 0)) < 0 && errno == EINTR)
+ ; /* nothing */
+ fprintf(stderr, _("done!\n"));
+ }
+
kill(p->pid, sig);
- if (!in_signal)
+ if (!in_signal) {
+ free(p->name);
free(p);
}
}
+}
static void cleanup_children_on_signal(int sig)
{
@@ -62,10 +73,16 @@ static void cleanup_children_on_exit(void)
cleanup_children(SIGTERM, 0);
}
-static void mark_child_for_cleanup(pid_t pid)
+static void mark_child_for_cleanup(pid_t pid, const char *name, int stdin, int wait)
{
struct child_to_clean *p = xmalloc(sizeof(*p));
p->pid = pid;
+ p->wait = wait;
+ p->stdin = stdin;
+ if (name)
+ p->name = xstrdup(name);
+ else
+ p->name = "process";
p->next = children_to_clean;
children_to_clean = p;
@@ -76,6 +93,13 @@ static void mark_child_for_cleanup(pid_t pid)
}
}
+#ifdef NO_PTHREADS
+static void mark_child_for_cleanup_no_wait(pid_t pid, const char *name, int timeout, int stdin)
+{
+ mark_child_for_cleanup(pid, NULL, 0, 0);
+}
+#endif
+
static void clear_child_for_cleanup(pid_t pid)
{
struct child_to_clean **pp;
@@ -434,8 +458,9 @@ int start_command(struct child_process *cmd)
}
if (cmd->pid < 0)
error_errno("cannot fork() for %s", cmd->argv[0]);
- else if (cmd->clean_on_exit)
- mark_child_for_cleanup(cmd->pid);
+ else if (cmd->clean_on_exit || cmd->wait_on_exit)
+ mark_child_for_cleanup(
+ cmd->pid, cmd->argv[0], cmd->in, cmd->wait_on_exit);
/*
* Wait for child's execvp. If the execvp succeeds (or if fork()
@@ -495,8 +520,9 @@ int start_command(struct child_process *cmd)
failed_errno = errno;
if (cmd->pid < 0 && (!cmd->silent_exec_failure || errno != ENOENT))
error_errno("cannot spawn %s", cmd->argv[0]);
- if (cmd->clean_on_exit && cmd->pid >= 0)
- mark_child_for_cleanup(cmd->pid);
+ if ((cmd->clean_on_exit || cmd->wait_on_exit) && cmd->pid >= 0)
+ mark_child_for_cleanup(
+ cmd->pid, cmd->argv[0], cmd->in, cmd->clean_on_exit_timeout);
argv_array_clear(&nargv);
cmd->argv = sargv;
@@ -647,7 +673,7 @@ int in_async(void)
return !pthread_equal(main_thread, pthread_self());
}
-void NORETURN async_exit(int code)
+static void NORETURN async_exit(int code)
{
pthread_exit((void *)(intptr_t)code);
}
@@ -697,13 +723,26 @@ int in_async(void)
return process_is_async;
}
-void NORETURN async_exit(int code)
+static void NORETURN async_exit(int code)
{
exit(code);
}
#endif
+void check_pipe(int err)
+{
+ if (err == EPIPE) {
+ if (in_async())
+ async_exit(141);
+
+ signal(SIGPIPE, SIG_DFL);
+ raise(SIGPIPE);
+ /* Should never happen, but just in case... */
+ exit(141);
+ }
+}
+
int start_async(struct async *async)
{
int need_in, need_out;
@@ -765,7 +804,7 @@ int start_async(struct async *async)
exit(!!async->proc(proc_in, proc_out, async->data));
}
- mark_child_for_cleanup(async->pid);
+ mark_child_for_cleanup_no_wait(async->pid);
if (need_in)
close(fdin[0]);
diff --git a/run-command.h b/run-command.h
index e7c5f71..f7b9907 100644
--- a/run-command.h
+++ b/run-command.h
@@ -42,7 +42,10 @@ struct child_process {
unsigned silent_exec_failure:1;
unsigned stdout_to_stderr:1;
unsigned use_shell:1;
+ /* kill the child on Git exit */
unsigned clean_on_exit:1;
+ /* close the child's stdin on Git exit and wait until it terminates */
+ unsigned wait_on_exit:1;
};
#define CHILD_PROCESS_INIT { NULL, ARGV_ARRAY_INIT, ARGV_ARRAY_INIT }
@@ -54,8 +57,6 @@ int finish_command(struct child_process *);
int finish_command_in_signal(struct child_process *);
int run_command(struct child_process *);
-void check_pipe(int err);
-
/*
* Returns the path to the hook file, or NULL if the hook is missing
* or disabled. Note that this points to static storage that will be
@@ -141,7 +142,7 @@ struct async {
int start_async(struct async *async);
int finish_async(struct async *async);
int in_async(void);
-void NORETURN async_exit(int code);
+void check_pipe(int err);
/**
* This callback should initialize the child process and preload the
diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
index 210c4f6..52b7fe9 100755
--- a/t/t0021-conversion.sh
+++ b/t/t0021-conversion.sh
@@ -4,13 +4,77 @@ test_description='blob conversion via gitattributes'
. ./test-lib.sh
-cat <<EOF >rot13.sh
+TEST_ROOT="$(pwd)"
+
+cat <<EOF >"$TEST_ROOT/rot13.sh"
#!$SHELL_PATH
tr \
'abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ' \
'nopqrstuvwxyzabcdefghijklmNOPQRSTUVWXYZABCDEFGHIJKLM'
EOF
-chmod +x rot13.sh
+chmod +x "$TEST_ROOT/rot13.sh"
+
+generate_random_characters () {
+ LEN=$1
+ NAME=$2
+ test-genrandom some-seed $LEN |
+ perl -pe "s/./chr((ord($&) % 26) + ord('a'))/sge" >"$TEST_ROOT/$NAME"
+}
+
+file_size () {
+ cat "$1" | wc -c | sed "s/^[ ]*//"
+}
+
+filter_git () {
+ rm -f rot13-filter.log &&
+ git "$@" 2>git-stderr.log &&
+ sed '/Waiting for/d' git-stderr.log >git-stderr-clean.log &&
+ test_must_be_empty git-stderr-clean.log &&
+ rm -f git-stderr.log git-stderr-clean.log
+}
+
+# Count unique lines in two files and compare them.
+test_cmp_count () {
+ for FILE in $@
+ do
+ sort $FILE | uniq -c | sed "s/^[ ]*//" >$FILE.tmp
+ cat $FILE.tmp >$FILE
+ done &&
+ test_cmp $@
+}
+
+# Count unique lines except clean invocations in two files and compare
+# them. Clean invocations are not counted because their number can vary.
+# c.f. http://public-inbox.org/git/xmqqshv18i8i.fsf@gitster.mtv.corp.google.com/
+test_cmp_count_except_clean () {
+ for FILE in $@
+ do
+ sort $FILE | uniq -c | sed "s/^[ ]*//" |
+ sed "s/^\([0-9]\) IN: clean/x IN: clean/" >$FILE.tmp
+ cat $FILE.tmp >$FILE
+ done &&
+ test_cmp $@
+}
+
+# Compare two files but exclude clean invocations because they can vary.
+# c.f. http://public-inbox.org/git/xmqqshv18i8i.fsf@gitster.mtv.corp.google.com/
+test_cmp_exclude_clean () {
+ for FILE in $@
+ do
+ grep -v "IN: clean" $FILE >$FILE.tmp
+ cat $FILE.tmp >$FILE
+ done &&
+ test_cmp $@
+}
+
+# Check that the contents of two files are equal and that their rot13 version
+# is equal to the committed content.
+test_cmp_committed_rot13 () {
+ test_cmp "$1" "$2" &&
+ "$TEST_ROOT/rot13.sh" <"$1" >expected &&
+ git cat-file blob :"$2" >actual &&
+ test_cmp expected actual
+}
test_expect_success setup '
git config filter.rot13.smudge ./rot13.sh &&
@@ -34,7 +98,7 @@ test_expect_success setup '
git checkout -- test test.t test.i &&
echo "content-test2" >test2.o &&
- echo "content-test3 - subdir" >"test3 - subdir.o"
+ echo "content-test3 - filename with special characters" >"test3 '\''sq'\'',\$x.o"
'
script='s/^\$Id: \([0-9a-f]*\) \$/\1/p'
@@ -282,47 +346,6 @@ test_expect_success 'diff does not reuse worktree files that need cleaning' '
test_line_count = 0 count
'
-check_filter () {
- rm -f rot13-filter.log actual.log &&
- "$@" 2> git_stderr.log &&
- test_must_be_empty git_stderr.log &&
- cat >expected.log &&
- sort rot13-filter.log | uniq -c | sed "s/^[ ]*//" >actual.log &&
- test_cmp expected.log actual.log
-}
-
-check_filter_count_clean () {
- rm -f rot13-filter.log actual.log &&
- "$@" 2> git_stderr.log &&
- test_must_be_empty git_stderr.log &&
- cat >expected.log &&
- sort rot13-filter.log | uniq -c | sed "s/^[ ]*//" |
- sed "s/^\([0-9]\) IN: clean/x IN: clean/" >actual.log &&
- test_cmp expected.log actual.log
-}
-
-check_filter_ignore_clean () {
- rm -f rot13-filter.log actual.log &&
- "$@" &&
- cat >expected.log &&
- grep -v "IN: clean" rot13-filter.log >actual.log &&
- test_cmp expected.log actual.log
-}
-
-check_filter_no_call () {
- rm -f rot13-filter.log &&
- "$@" 2> git_stderr.log &&
- test_must_be_empty git_stderr.log &&
- test_must_be_empty rot13-filter.log
-}
-
-check_rot13 () {
- test_cmp "$1" "$2" &&
- ./../rot13.sh <"$1" >expected &&
- git cat-file blob :"$2" >actual &&
- test_cmp expected actual
-}
-
test_expect_success PERL 'required process filter should filter data' '
test_config_global filter.protocol.process "$TEST_DIRECTORY/t0021/rot13-filter.pl clean smudge" &&
test_config_global filter.protocol.required true &&
@@ -332,81 +355,91 @@ test_expect_success PERL 'required process filter should filter data' '
cd repo &&
git init &&
+ echo "git-stderr.log" >.gitignore &&
echo "*.r filter=protocol" >.gitattributes &&
git add . &&
- git commit . -m "test commit" &&
- git branch empty &&
+ git commit . -m "test commit 1" &&
+ git branch empty-branch &&
- cp ../test.o test.r &&
- cp ../test2.o test2.r &&
+ cp "$TEST_ROOT/test.o" test.r &&
+ cp "$TEST_ROOT/test2.o" test2.r &&
mkdir testsubdir &&
- cp "../test3 - subdir.o" "testsubdir/test3 - subdir.r" &&
+ cp "$TEST_ROOT/test3 '\''sq'\'',\$x.o" "testsubdir/test3 '\''sq'\'',\$x.r" &&
>test4-empty.r &&
- check_filter \
- git add . \
- <<-\EOF &&
- 1 IN: clean test.r 57 [OK] -- OUT: 57 . [OK]
- 1 IN: clean test2.r 14 [OK] -- OUT: 14 . [OK]
- 1 IN: clean test4-empty.r 0 [OK] -- OUT: 0 [OK]
- 1 IN: clean testsubdir/test3 - subdir.r 23 [OK] -- OUT: 23 . [OK]
- 1 START
- 1 STOP
- 1 wrote filter header
+ S=$(file_size test.r) &&
+ S2=$(file_size test2.r) &&
+ S3=$(file_size "testsubdir/test3 '\''sq'\'',\$x.r") &&
+
+ filter_git add . &&
+ cat >expected.log <<-EOF &&
+ START
+ init handshake complete
+ IN: clean test.r $S [OK] -- OUT: $S . [OK]
+ IN: clean test2.r $S2 [OK] -- OUT: $S2 . [OK]
+ IN: clean test4-empty.r 0 [OK] -- OUT: 0 [OK]
+ IN: clean testsubdir/test3 '\''sq'\'',\$x.r $S3 [OK] -- OUT: $S3 . [OK]
+ STOP
EOF
+ test_cmp_count expected.log rot13-filter.log &&
- check_filter_count_clean \
- git commit . -m "test commit" \
- <<-\EOF &&
- x IN: clean test.r 57 [OK] -- OUT: 57 . [OK]
- x IN: clean test2.r 14 [OK] -- OUT: 14 . [OK]
- x IN: clean test4-empty.r 0 [OK] -- OUT: 0 [OK]
- x IN: clean testsubdir/test3 - subdir.r 23 [OK] -- OUT: 23 . [OK]
- 1 START
- 1 STOP
- 1 wrote filter header
+ filter_git commit . -m "test commit 2" &&
+ cat >expected.log <<-EOF &&
+ START
+ init handshake complete
+ IN: clean test.r $S [OK] -- OUT: $S . [OK]
+ IN: clean test2.r $S2 [OK] -- OUT: $S2 . [OK]
+ IN: clean test4-empty.r 0 [OK] -- OUT: 0 [OK]
+ IN: clean testsubdir/test3 '\''sq'\'',\$x.r $S3 [OK] -- OUT: $S3 . [OK]
+ IN: clean test.r $S [OK] -- OUT: $S . [OK]
+ IN: clean test2.r $S2 [OK] -- OUT: $S2 . [OK]
+ IN: clean test4-empty.r 0 [OK] -- OUT: 0 [OK]
+ IN: clean testsubdir/test3 '\''sq'\'',\$x.r $S3 [OK] -- OUT: $S3 . [OK]
+ STOP
EOF
+ test_cmp_count_except_clean expected.log rot13-filter.log &&
- rm -f test?.r "testsubdir/test3 - subdir.r" &&
+ rm -f test2.r "testsubdir/test3 '\''sq'\'',\$x.r" &&
- check_filter_ignore_clean \
- git checkout . \
- <<-\EOF &&
+ filter_git checkout --quiet --no-progress . &&
+ cat >expected.log <<-EOF &&
START
- wrote filter header
- IN: smudge test2.r 14 [OK] -- OUT: 14 . [OK]
- IN: smudge testsubdir/test3 - subdir.r 23 [OK] -- OUT: 23 . [OK]
+ init handshake complete
+ IN: smudge test2.r $S2 [OK] -- OUT: $S2 . [OK]
+ IN: smudge testsubdir/test3 '\''sq'\'',\$x.r $S3 [OK] -- OUT: $S3 . [OK]
STOP
EOF
+ test_cmp_exclude_clean expected.log rot13-filter.log &&
- check_filter_ignore_clean \
- git checkout empty \
- <<-\EOF &&
+ filter_git checkout --quiet --no-progress empty-branch &&
+ cat >expected.log <<-EOF &&
START
- wrote filter header
+ init handshake complete
+ IN: clean test.r $S [OK] -- OUT: $S . [OK]
STOP
EOF
+ test_cmp_exclude_clean expected.log rot13-filter.log &&
- check_filter_ignore_clean \
- git checkout master \
- <<-\EOF &&
+ filter_git checkout --quiet --no-progress master &&
+ cat >expected.log <<-EOF &&
START
- wrote filter header
- IN: smudge test.r 57 [OK] -- OUT: 57 . [OK]
- IN: smudge test2.r 14 [OK] -- OUT: 14 . [OK]
+ init handshake complete
+ IN: smudge test.r $S [OK] -- OUT: $S . [OK]
+ IN: smudge test2.r $S2 [OK] -- OUT: $S2 . [OK]
IN: smudge test4-empty.r 0 [OK] -- OUT: 0 [OK]
- IN: smudge testsubdir/test3 - subdir.r 23 [OK] -- OUT: 23 . [OK]
+ IN: smudge testsubdir/test3 '\''sq'\'',\$x.r $S3 [OK] -- OUT: $S3 . [OK]
STOP
EOF
+ test_cmp_exclude_clean expected.log rot13-filter.log &&
- check_rot13 ../test.o test.r &&
- check_rot13 ../test2.o test2.r &&
- check_rot13 "../test3 - subdir.o" "testsubdir/test3 - subdir.r"
+ test_cmp_committed_rot13 "$TEST_ROOT/test.o" test.r &&
+ test_cmp_committed_rot13 "$TEST_ROOT/test2.o" test2.r &&
+ test_cmp_committed_rot13 "$TEST_ROOT/test3 '\''sq'\'',\$x.o" "testsubdir/test3 '\''sq'\'',\$x.r"
)
'
-test_expect_success PERL 'required process filter should clean only and take precedence' '
- test_config_global filter.protocol.clean ./../rot13.sh &&
+test_expect_success PERL 'required process filter takes precedence' '
+ test_config_global filter.protocol.clean false &&
test_config_global filter.protocol.process "$TEST_DIRECTORY/t0021/rot13-filter.pl clean" &&
test_config_global filter.protocol.required true &&
rm -rf repo &&
@@ -416,40 +449,55 @@ test_expect_success PERL 'required process filter should clean only and take pre
git init &&
echo "*.r filter=protocol" >.gitattributes &&
- git add . &&
- git commit . -m "test commit" &&
- git branch empty &&
-
- cp ../test.o test.r &&
-
- check_filter \
- git add . \
- <<-\EOF &&
- 1 IN: clean test.r 57 [OK] -- OUT: 57 . [OK]
- 1 START
- 1 STOP
- 1 wrote filter header
- EOF
+ cp "$TEST_ROOT/test.o" test.r &&
+ S=$(file_size test.r) &&
- check_filter_count_clean \
- git commit . -m "test commit" \
- <<-\EOF
- x IN: clean test.r 57 [OK] -- OUT: 57 . [OK]
- 1 START
- 1 STOP
- 1 wrote filter header
+ # Check that the process filter is invoked here
+ filter_git add . &&
+ cat >expected.log <<-EOF &&
+ START
+ init handshake complete
+ IN: clean test.r $S [OK] -- OUT: $S . [OK]
+ STOP
EOF
+ test_cmp_count expected.log rot13-filter.log
)
'
-generate_test_data () {
- LEN=$1
- NAME=$2
- test-genrandom end $LEN |
- perl -pe "s/./chr((ord($&) % 26) + 97)/sge" >../$NAME.file &&
- cp ../$NAME.file . &&
- ./../rot13.sh <../$NAME.file >../$NAME.file.rot13
-}
+test_expect_success PERL 'required process filter should be used only for "clean" operation only' '
+ test_config_global filter.protocol.process "$TEST_DIRECTORY/t0021/rot13-filter.pl clean" &&
+ rm -rf repo &&
+ mkdir repo &&
+ (
+ cd repo &&
+ git init &&
+
+ echo "*.r filter=protocol" >.gitattributes &&
+ cp "$TEST_ROOT/test.o" test.r &&
+ S=$(file_size test.r) &&
+
+ filter_git add . &&
+ cat >expected.log <<-EOF &&
+ START
+ init handshake complete
+ IN: clean test.r $S [OK] -- OUT: $S . [OK]
+ STOP
+ EOF
+ test_cmp_count expected.log rot13-filter.log &&
+
+ rm test.r &&
+
+ filter_git checkout --quiet --no-progress . &&
+ # If the filter would be used for "smudge", too, we would see
+ # "IN: smudge test.r 57 [OK] -- OUT: 57 . [OK]" here
+ cat >expected.log <<-EOF &&
+ START
+ init handshake complete
+ STOP
+ EOF
+ test_cmp_exclude_clean expected.log rot13-filter.log
+ )
+'
test_expect_success PERL 'required process filter should process multiple packets' '
test_config_global filter.protocol.process "$TEST_DIRECTORY/t0021/rot13-filter.pl clean smudge" &&
@@ -461,42 +509,57 @@ test_expect_success PERL 'required process filter should process multiple packet
cd repo &&
git init &&
- # Generate data that requires 3 packets
- PKTLINE_DATA_MAXLEN=65516 &&
+ # Generate data requiring 1, 2, 3 packets
+ S=65516 && # PKTLINE_DATA_MAXLEN -> Maximal size of a packet
+ generate_random_characters $(($S )) 1pkt_1__.file &&
+ generate_random_characters $(($S +1)) 2pkt_1+1.file &&
+ generate_random_characters $(($S*2-1)) 2pkt_2-1.file &&
+ generate_random_characters $(($S*2 )) 2pkt_2__.file &&
+ generate_random_characters $(($S*2+1)) 3pkt_2+1.file &&
- generate_test_data $(($PKTLINE_DATA_MAXLEN )) 1pkt_1__ &&
- generate_test_data $(($PKTLINE_DATA_MAXLEN + 1)) 2pkt_1+1 &&
- generate_test_data $(($PKTLINE_DATA_MAXLEN * 2 - 1)) 2pkt_2-1 &&
- generate_test_data $(($PKTLINE_DATA_MAXLEN * 2 )) 2pkt_2__ &&
- generate_test_data $(($PKTLINE_DATA_MAXLEN * 2 + 1)) 3pkt_2+1 &&
+ for FILE in "$TEST_ROOT"/*.file
+ do
+ cp "$FILE" . &&
+ "$TEST_ROOT/rot13.sh" <"$FILE" >"$FILE.rot13"
+ done &&
echo "*.file filter=protocol" >.gitattributes &&
- check_filter \
- git add *.file .gitattributes \
- <<-\EOF &&
- 1 IN: clean 1pkt_1__.file 65516 [OK] -- OUT: 65516 . [OK]
- 1 IN: clean 2pkt_1+1.file 65517 [OK] -- OUT: 65517 .. [OK]
- 1 IN: clean 2pkt_2-1.file 131031 [OK] -- OUT: 131031 .. [OK]
- 1 IN: clean 2pkt_2__.file 131032 [OK] -- OUT: 131032 .. [OK]
- 1 IN: clean 3pkt_2+1.file 131033 [OK] -- OUT: 131033 ... [OK]
- 1 START
- 1 STOP
- 1 wrote filter header
+ filter_git add *.file .gitattributes &&
+ cat >expected.log <<-EOF &&
+ START
+ init handshake complete
+ IN: clean 1pkt_1__.file $(($S )) [OK] -- OUT: $(($S )) . [OK]
+ IN: clean 2pkt_1+1.file $(($S +1)) [OK] -- OUT: $(($S +1)) .. [OK]
+ IN: clean 2pkt_2-1.file $(($S*2-1)) [OK] -- OUT: $(($S*2-1)) .. [OK]
+ IN: clean 2pkt_2__.file $(($S*2 )) [OK] -- OUT: $(($S*2 )) .. [OK]
+ IN: clean 3pkt_2+1.file $(($S*2+1)) [OK] -- OUT: $(($S*2+1)) ... [OK]
+ STOP
EOF
- git commit . -m "test commit" &&
+ test_cmp_count expected.log rot13-filter.log &&
rm -f *.file &&
- git checkout -- *.file &&
- for f in *.file
+ filter_git checkout --quiet --no-progress -- *.file &&
+ cat >expected.log <<-EOF &&
+ START
+ init handshake complete
+ IN: smudge 1pkt_1__.file $(($S )) [OK] -- OUT: $(($S )) . [OK]
+ IN: smudge 2pkt_1+1.file $(($S +1)) [OK] -- OUT: $(($S +1)) .. [OK]
+ IN: smudge 2pkt_2-1.file $(($S*2-1)) [OK] -- OUT: $(($S*2-1)) .. [OK]
+ IN: smudge 2pkt_2__.file $(($S*2 )) [OK] -- OUT: $(($S*2 )) .. [OK]
+ IN: smudge 3pkt_2+1.file $(($S*2+1)) [OK] -- OUT: $(($S*2+1)) ... [OK]
+ STOP
+ EOF
+ test_cmp_exclude_clean expected.log rot13-filter.log &&
+
+ for FILE in *.file
do
- git cat-file blob :$f >actual &&
- test_cmp ../$f.rot13 actual
+ test_cmp_committed_rot13 "$TEST_ROOT/$FILE" $FILE
done
)
'
-test_expect_success PERL 'required process filter should with clean error should fail' '
+test_expect_success PERL 'required process filter with clean error should fail' '
test_config_global filter.protocol.process "$TEST_DIRECTORY/t0021/rot13-filter.pl clean smudge" &&
test_config_global filter.protocol.required true &&
rm -rf repo &&
@@ -507,11 +570,10 @@ test_expect_success PERL 'required process filter should with clean error should
echo "*.r filter=protocol" >.gitattributes &&
- cp ../test.o test.r &&
+ cp "$TEST_ROOT/test.o" test.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_fail git add .
)
'
@@ -526,38 +588,48 @@ test_expect_success PERL 'process filter should restart after unexpected write f
echo "*.r filter=protocol" >.gitattributes &&
- cp ../test.o test.r &&
- cp ../test2.o test2.r &&
+ cp "$TEST_ROOT/test.o" test.r &&
+ cp "$TEST_ROOT/test2.o" test2.r &&
echo "this is going to fail" >smudge-write-fail.o &&
- cat smudge-write-fail.o >smudge-write-fail.r &&
+ cp smudge-write-fail.o smudge-write-fail.r &&
+
+ S=$(file_size test.r) &&
+ S2=$(file_size test2.r) &&
+ SF=$(file_size smudge-write-fail.r) &&
+
git add . &&
- git commit . -m "test commit" &&
rm -f *.r &&
- check_filter_ignore_clean \
- git checkout . \
- <<-\EOF &&
+ rm -f rot13-filter.log &&
+ git checkout --quiet --no-progress . 2>git-stderr.log &&
+
+ grep "smudge write error at" git-stderr.log &&
+ grep "error: external filter" git-stderr.log &&
+
+ cat >expected.log <<-EOF &&
START
- wrote filter header
- IN: smudge smudge-write-fail.r 22 [OK] -- OUT: 22 [WRITE FAIL]
+ init handshake complete
+ IN: smudge smudge-write-fail.r $SF [OK] -- OUT: $SF [WRITE FAIL]
START
- wrote filter header
- IN: smudge test.r 57 [OK] -- OUT: 57 . [OK]
- IN: smudge test2.r 14 [OK] -- OUT: 14 . [OK]
+ init handshake complete
+ IN: smudge test.r $S [OK] -- OUT: $S . [OK]
+ IN: smudge test2.r $S2 [OK] -- OUT: $S2 . [OK]
STOP
EOF
+ test_cmp_exclude_clean expected.log rot13-filter.log &&
- check_rot13 ../test.o test.r &&
- check_rot13 ../test2.o test2.r &&
+ test_cmp_committed_rot13 "$TEST_ROOT/test.o" test.r &&
+ test_cmp_committed_rot13 "$TEST_ROOT/test2.o" test2.r &&
- ! test_cmp smudge-write-fail.o smudge-write-fail.r && # Smudge failed!
- ./../rot13.sh <smudge-write-fail.o >expected &&
+ # Smudge failed
+ ! test_cmp smudge-write-fail.o smudge-write-fail.r &&
+ "$TEST_ROOT/rot13.sh" <smudge-write-fail.o >expected &&
git cat-file blob :smudge-write-fail.r >actual &&
- test_cmp expected actual # Clean worked!
+ test_cmp expected actual
)
'
-test_expect_success PERL 'process filter should not restart in case of an error' '
+test_expect_success PERL 'process filter should not be restarted if it signals an error' '
test_config_global filter.protocol.process "$TEST_DIRECTORY/t0021/rot13-filter.pl clean smudge" &&
rm -rf repo &&
mkdir repo &&
@@ -567,32 +639,36 @@ test_expect_success PERL 'process filter should not restart in case of an error'
echo "*.r filter=protocol" >.gitattributes &&
- cp ../test.o test.r &&
- cp ../test2.o test2.r &&
+ cp "$TEST_ROOT/test.o" test.r &&
+ cp "$TEST_ROOT/test2.o" test2.r &&
echo "this will cause an error" >error.o &&
cp error.o error.r &&
+
+ S=$(file_size test.r) &&
+ S2=$(file_size test2.r) &&
+ SE=$(file_size error.r) &&
+
git add . &&
- git commit . -m "test commit" &&
rm -f *.r &&
- check_filter_ignore_clean \
- git checkout . \
- <<-\EOF &&
+ filter_git checkout --quiet --no-progress . &&
+ cat >expected.log <<-EOF &&
START
- wrote filter header
- IN: smudge error.r 25 [OK] -- OUT: 0 [ERROR]
- IN: smudge test.r 57 [OK] -- OUT: 57 . [OK]
- IN: smudge test2.r 14 [OK] -- OUT: 14 . [OK]
+ init handshake complete
+ IN: smudge error.r $SE [OK] -- OUT: 0 [ERROR]
+ IN: smudge test.r $S [OK] -- OUT: $S . [OK]
+ IN: smudge test2.r $S2 [OK] -- OUT: $S2 . [OK]
STOP
EOF
+ test_cmp_exclude_clean expected.log rot13-filter.log &&
- check_rot13 ../test.o test.r &&
- check_rot13 ../test2.o test2.r &&
+ test_cmp_committed_rot13 "$TEST_ROOT/test.o" test.r &&
+ test_cmp_committed_rot13 "$TEST_ROOT/test2.o" test2.r &&
test_cmp error.o error.r
)
'
-test_expect_success PERL 'process filter should be able to signal an error for all future files' '
+test_expect_success PERL 'process filter signals abort once to abort processing of all future files' '
test_config_global filter.protocol.process "$TEST_DIRECTORY/t0021/rot13-filter.pl clean smudge" &&
rm -rf repo &&
mkdir repo &&
@@ -602,25 +678,27 @@ test_expect_success PERL 'process filter should be able to signal an error for a
echo "*.r filter=protocol" >.gitattributes &&
- cp ../test.o test.r &&
- cp ../test2.o test2.r &&
+ cp "$TEST_ROOT/test.o" test.r &&
+ cp "$TEST_ROOT/test2.o" test2.r &&
echo "error this blob and all future blobs" >abort.o &&
cp abort.o abort.r &&
+
+ SA=$(file_size abort.r) &&
+
git add . &&
- git commit . -m "test commit" &&
rm -f *.r &&
- check_filter_ignore_clean \
- git checkout . \
- <<-\EOF &&
+ filter_git checkout --quiet --no-progress . &&
+ cat >expected.log <<-EOF &&
START
- wrote filter header
- IN: smudge abort.r 37 [OK] -- OUT: 0 [ABORT]
+ init handshake complete
+ IN: smudge abort.r $SA [OK] -- OUT: 0 [ABORT]
STOP
EOF
+ test_cmp_exclude_clean expected.log rot13-filter.log &&
- test_cmp ../test.o test.r &&
- test_cmp ../test2.o test2.r &&
+ test_cmp "$TEST_ROOT/test.o" test.r &&
+ test_cmp "$TEST_ROOT/test2.o" test2.r &&
test_cmp abort.o abort.r
)
'
@@ -636,9 +714,9 @@ test_expect_success PERL 'invalid process filter must fail (and not hang!)' '
echo "*.r filter=protocol" >.gitattributes &&
- cp ../test.o test.r &&
- test_must_fail git add . 2> git_stderr.log &&
- grep "not support long running filter protocol" git_stderr.log
+ cp "$TEST_ROOT/test.o" test.r &&
+ test_must_fail git add . 2>git-stderr.log &&
+ grep "does not support filter protocol version" git-stderr.log
)
'
diff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl
index 8958f71..1a6959c 100755
--- a/t/t0021/rot13-filter.pl
+++ b/t/t0021/rot13-filter.pl
@@ -26,10 +26,10 @@ use warnings;
my $MAX_PACKET_CONTENT_SIZE = 65516;
my @capabilities = @ARGV;
-open my $debug, ">>", "rot13-filter.log";
+open my $debug, ">>", "rot13-filter.log" or die "cannot open log file: $!";
sub rot13 {
- my ($str) = @_;
+ my $str = shift;
$str =~ y/A-Za-z/N-ZA-Mn-za-m/;
return $str;
}
@@ -38,13 +38,12 @@ sub packet_bin_read {
my $buffer;
my $bytes_read = read STDIN, $buffer, 4;
if ( $bytes_read == 0 ) {
-
# EOF - Git stopped talking to us!
print $debug "STOP\n";
exit();
}
elsif ( $bytes_read != 4 ) {
- die "invalid packet size '$bytes_read' field";
+ die "invalid packet: '$buffer'";
}
my $pkt_size = hex($buffer);
if ( $pkt_size == 0 ) {
@@ -54,27 +53,27 @@ sub packet_bin_read {
my $content_size = $pkt_size - 4;
$bytes_read = read STDIN, $buffer, $content_size;
if ( $bytes_read != $content_size ) {
- die "invalid packet ($content_size expected; $bytes_read read)";
+ die "invalid packet ($content_size bytes expected; $bytes_read bytes read)";
}
return ( 0, $buffer );
}
else {
- die "invalid packet size";
+ die "invalid packet size: $pkt_size";
}
}
sub packet_txt_read {
my ( $res, $buf ) = packet_bin_read();
- unless ( $buf =~ /\n$/ ) {
- die "A non-binary line SHOULD BE terminated by an LF.";
+ unless ( $buf =~ s/\n$// ) {
+ die "A non-binary line MUST be terminated by an LF.";
}
- return ( $res, substr( $buf, 0, -1 ) );
+ return ( $res, $buf );
}
sub packet_bin_write {
- my ($packet) = @_;
- print STDOUT sprintf( "%04x", length($packet) + 4 );
- print STDOUT $packet;
+ my $buf = shift;
+ print STDOUT sprintf( "%04x", length($buf) + 4 );
+ print STDOUT $buf;
STDOUT->flush();
}
@@ -105,7 +104,7 @@ foreach (@capabilities) {
packet_txt_write( $_ . "=true" );
}
packet_flush();
-print $debug "wrote filter header\n";
+print $debug "init handshake complete\n";
$debug->flush();
while (1) {
@@ -175,6 +174,7 @@ while (1) {
while ( length($output) > 0 ) {
my $packet = substr( $output, 0, $MAX_PACKET_CONTENT_SIZE );
packet_bin_write($packet);
+ # dots represent the number of packets
print $debug ".";
if ( length($output) > $MAX_PACKET_CONTENT_SIZE ) {
$output = substr( $output, $MAX_PACKET_CONTENT_SIZE );
Lars Schneider (14):
convert: quote filter names in error messages
convert: modernize tests
run-command: move check_pipe() from write_or_die to run_command
run-command: add wait_on_exit
pkt-line: rename packet_write() to packet_write_fmt()
pkt-line: extract set_packet_header()
pkt-line: add packet_write_fmt_gently()
pkt-line: add packet_flush_gently()
pkt-line: add packet_write_gently()
pkt-line: add functions to read/write flush terminated packet streams
convert: make apply_filter() adhere to standard Git error handling
convert: prepare filter.<driver>.process option
convert: add filter.<driver>.process option
contrib/long-running-filter: add long running filter example
Documentation/gitattributes.txt | 159 ++++++++++-
builtin/archive.c | 4 +-
builtin/receive-pack.c | 4 +-
builtin/remote-ext.c | 4 +-
builtin/upload-archive.c | 4 +-
connect.c | 2 +-
contrib/long-running-filter/example.pl | 127 +++++++++
convert.c | 370 +++++++++++++++++++++---
daemon.c | 2 +-
http-backend.c | 2 +-
pkt-line.c | 148 +++++++++-
pkt-line.h | 12 +-
run-command.c | 72 ++++-
run-command.h | 5 +-
shallow.c | 2 +-
t/t0021-conversion.sh | 505 ++++++++++++++++++++++++++++++---
t/t0021/rot13-filter.pl | 191 +++++++++++++
upload-pack.c | 30 +-
write_or_die.c | 13 -
19 files changed, 1519 insertions(+), 137 deletions(-)
create mode 100755 contrib/long-running-filter/example.pl
create mode 100755 t/t0021/rot13-filter.pl
--
2.10.0
^ permalink raw reply related
* [PATCH v9 08/14] pkt-line: add packet_flush_gently()
From: larsxschneider @ 2016-10-04 12:59 UTC (permalink / raw)
To: git; +Cc: ramsay, jnareb, gitster, j6t, tboegi, peff, mlbright,
Lars Schneider
In-Reply-To: <20161004125947.67104-1-larsxschneider@gmail.com>
From: Lars Schneider <larsxschneider@gmail.com>
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 like packet_flush() but does not die in
case of an error. The function is used in a subsequent patch.
Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
pkt-line.c | 8 ++++++++
pkt-line.h | 1 +
2 files changed, 9 insertions(+)
diff --git a/pkt-line.c b/pkt-line.c
index 56915f0..286eb09 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -91,6 +91,14 @@ void packet_flush(int fd)
write_or_die(fd, "0000", 4);
}
+int packet_flush_gently(int fd)
+{
+ packet_trace("0000", 4, 1);
+ if (write_in_full(fd, "0000", 4) == 4)
+ return 0;
+ return error("flush packet write failed");
+}
+
void packet_buf_flush(struct strbuf *buf)
{
packet_trace("0000", 4, 1);
diff --git a/pkt-line.h b/pkt-line.h
index 3caea77..3fa0899 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -23,6 +23,7 @@ void packet_flush(int fd);
void packet_write_fmt(int fd, const char *fmt, ...) __attribute__((format (printf, 2, 3)));
void packet_buf_flush(struct strbuf *buf);
void packet_buf_write(struct strbuf *buf, const char *fmt, ...) __attribute__((format (printf, 2, 3)));
+int packet_flush_gently(int fd);
int packet_write_fmt_gently(int fd, const char *fmt, ...) __attribute__((format (printf, 2, 3)));
/*
--
2.10.0
^ permalink raw reply related
* [PATCH v9 14/14] contrib/long-running-filter: add long running filter example
From: larsxschneider @ 2016-10-04 12:59 UTC (permalink / raw)
To: git; +Cc: ramsay, jnareb, gitster, j6t, tboegi, peff, mlbright,
Lars Schneider
In-Reply-To: <20161004125947.67104-1-larsxschneider@gmail.com>
From: Lars Schneider <larsxschneider@gmail.com>
Add a simple pass-thru filter as example implementation for the Git
filter protocol version 2. See Documentation/gitattributes.txt, section
"Filter Protocol" for more info.
Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
---
Documentation/gitattributes.txt | 4 +-
contrib/long-running-filter/example.pl | 127 +++++++++++++++++++++++++++++++++
2 files changed, 130 insertions(+), 1 deletion(-)
create mode 100755 contrib/long-running-filter/example.pl
diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index 5868f00..a182ef2 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -514,7 +514,9 @@ the next "key=value" list containing a command. Git will close
the command pipe on exit. The filter is expected to detect EOF
and exit gracefully on its own.
-If you develop your own long running filter
+A long running filter demo implementation can be found in
+`contrib/long-running-filter/example.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]).
diff --git a/contrib/long-running-filter/example.pl b/contrib/long-running-filter/example.pl
new file mode 100755
index 0000000..f4102d2
--- /dev/null
+++ b/contrib/long-running-filter/example.pl
@@ -0,0 +1,127 @@
+#!/usr/bin/perl
+#
+# Example implementation for the Git filter protocol version 2
+# See Documentation/gitattributes.txt, section "Filter Protocol"
+#
+# Please note, this pass-thru filter is a minimal skeleton. No proper
+# error handling was implemented.
+#
+
+use strict;
+use warnings;
+
+my $MAX_PACKET_CONTENT_SIZE = 65516;
+
+sub packet_bin_read {
+ my $buffer;
+ my $bytes_read = read STDIN, $buffer, 4;
+ if ( $bytes_read == 0 ) {
+
+ # EOF - Git stopped talking to us!
+ exit();
+ }
+ elsif ( $bytes_read != 4 ) {
+ die "invalid packet: '$buffer'";
+ }
+ my $pkt_size = hex($buffer);
+ if ( $pkt_size == 0 ) {
+ return ( 1, "" );
+ }
+ elsif ( $pkt_size > 4 ) {
+ my $content_size = $pkt_size - 4;
+ $bytes_read = read STDIN, $buffer, $content_size;
+ if ( $bytes_read != $content_size ) {
+ die "invalid packet ($content_size bytes expected; $bytes_read bytes read)";
+ }
+ return ( 0, $buffer );
+ }
+ else {
+ die "invalid packet size: $pkt_size";
+ }
+}
+
+sub packet_txt_read {
+ my ( $res, $buf ) = packet_bin_read();
+ unless ( $buf =~ s/\n$// ) {
+ die "A non-binary line MUST be terminated by an LF.";
+ }
+ return ( $res, $buf );
+}
+
+sub packet_bin_write {
+ my $buf = shift;
+ print STDOUT sprintf( "%04x", length($buf) + 4 );
+ print STDOUT $buf;
+ STDOUT->flush();
+}
+
+sub packet_txt_write {
+ packet_bin_write( $_[0] . "\n" );
+}
+
+sub packet_flush {
+ print STDOUT sprintf( "%04x", 0 );
+ STDOUT->flush();
+}
+
+( packet_txt_read() eq ( 0, "git-filter-client" ) ) || die "bad initialize";
+( packet_txt_read() eq ( 0, "version=2" ) ) || die "bad version";
+( packet_bin_read() eq ( 1, "" ) ) || die "bad version end";
+
+packet_txt_write("git-filter-server");
+packet_txt_write("version=2");
+
+( packet_txt_read() eq ( 0, "clean=true" ) ) || die "bad capability";
+( packet_txt_read() eq ( 0, "smudge=true" ) ) || die "bad capability";
+( packet_bin_read() eq ( 1, "" ) ) || die "bad capability end";
+
+packet_txt_write("clean=true");
+packet_txt_write("smudge=true");
+packet_flush();
+
+while (1) {
+ my ($command) = packet_txt_read() =~ /^command=([^=]+)$/;
+ my ($pathname) = packet_txt_read() =~ /^pathname=([^=]+)$/;
+
+ packet_bin_read();
+
+ my $input = "";
+ {
+ binmode(STDIN);
+ my $buffer;
+ my $done = 0;
+ while ( !$done ) {
+ ( $done, $buffer ) = packet_bin_read();
+ $input .= $buffer;
+ }
+ }
+
+ my $output;
+ if ( $command eq "clean" ) {
+ ### Perform clean here ###
+ $output = $input;
+ }
+ elsif ( $command eq "smudge" ) {
+ ### Perform smudge here ###
+ $output = $input;
+ }
+ else {
+ die "bad command '$command'";
+ }
+
+ packet_txt_write("status=success");
+ packet_flush();
+ while ( length($output) > 0 ) {
+ my $packet = substr( $output, 0, $MAX_PACKET_CONTENT_SIZE );
+ packet_bin_write($packet);
+ if ( length($output) > $MAX_PACKET_CONTENT_SIZE ) {
+ $output = substr( $output, $MAX_PACKET_CONTENT_SIZE );
+ }
+ else {
+ $output = "";
+ }
+ }
+ packet_flush(); # flush content!
+ packet_flush(); # empty list, keep "status=success" unchanged!
+
+}
--
2.10.0
^ permalink raw reply related
* [PATCH v9 04/14] run-command: add wait_on_exit
From: larsxschneider @ 2016-10-04 12:59 UTC (permalink / raw)
To: git; +Cc: ramsay, jnareb, gitster, j6t, tboegi, peff, mlbright,
Lars Schneider
In-Reply-To: <20161004125947.67104-1-larsxschneider@gmail.com>
From: Lars Schneider <larsxschneider@gmail.com>
The flag 'clean_on_exit' kills child processes spawned by Git on exit.
A hard kill like this might not be desired in all cases.
Add 'wait_on_exit' which closes the child's stdin on Git exit and waits
until the child process has terminated.
The flag is used in a subsequent patch.
Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
---
run-command.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++--------
run-command.h | 3 +++
2 files changed, 50 insertions(+), 8 deletions(-)
diff --git a/run-command.c b/run-command.c
index 3269362..96c54fe 100644
--- a/run-command.c
+++ b/run-command.c
@@ -21,6 +21,9 @@ void child_process_clear(struct child_process *child)
struct child_to_clean {
pid_t pid;
+ char *name;
+ int stdin;
+ int wait;
struct child_to_clean *next;
};
static struct child_to_clean *children_to_clean;
@@ -28,12 +31,33 @@ static int installed_child_cleanup_handler;
static void cleanup_children(int sig, int in_signal)
{
+ int status;
+ struct child_to_clean *p = children_to_clean;
+
+ /* Close the the child's stdin as indicator that Git will exit soon */
+ while (p) {
+ if (p->wait)
+ if (p->stdin > 0)
+ close(p->stdin);
+ p = p->next;
+ }
+
while (children_to_clean) {
- struct child_to_clean *p = children_to_clean;
+ p = children_to_clean;
children_to_clean = p->next;
+
+ if (p->wait) {
+ fprintf(stderr, _("Waiting for '%s' to finish..."), p->name);
+ while ((waitpid(p->pid, &status, 0)) < 0 && errno == EINTR)
+ ; /* nothing */
+ fprintf(stderr, _("done!\n"));
+ }
+
kill(p->pid, sig);
- if (!in_signal)
+ if (!in_signal) {
+ free(p->name);
free(p);
+ }
}
}
@@ -49,10 +73,16 @@ static void cleanup_children_on_exit(void)
cleanup_children(SIGTERM, 0);
}
-static void mark_child_for_cleanup(pid_t pid)
+static void mark_child_for_cleanup(pid_t pid, const char *name, int stdin, int wait)
{
struct child_to_clean *p = xmalloc(sizeof(*p));
p->pid = pid;
+ p->wait = wait;
+ p->stdin = stdin;
+ if (name)
+ p->name = xstrdup(name);
+ else
+ p->name = "process";
p->next = children_to_clean;
children_to_clean = p;
@@ -63,6 +93,13 @@ static void mark_child_for_cleanup(pid_t pid)
}
}
+#ifdef NO_PTHREADS
+static void mark_child_for_cleanup_no_wait(pid_t pid, const char *name, int timeout, int stdin)
+{
+ mark_child_for_cleanup(pid, NULL, 0, 0);
+}
+#endif
+
static void clear_child_for_cleanup(pid_t pid)
{
struct child_to_clean **pp;
@@ -421,8 +458,9 @@ int start_command(struct child_process *cmd)
}
if (cmd->pid < 0)
error_errno("cannot fork() for %s", cmd->argv[0]);
- else if (cmd->clean_on_exit)
- mark_child_for_cleanup(cmd->pid);
+ else if (cmd->clean_on_exit || cmd->wait_on_exit)
+ mark_child_for_cleanup(
+ cmd->pid, cmd->argv[0], cmd->in, cmd->wait_on_exit);
/*
* Wait for child's execvp. If the execvp succeeds (or if fork()
@@ -482,8 +520,9 @@ int start_command(struct child_process *cmd)
failed_errno = errno;
if (cmd->pid < 0 && (!cmd->silent_exec_failure || errno != ENOENT))
error_errno("cannot spawn %s", cmd->argv[0]);
- if (cmd->clean_on_exit && cmd->pid >= 0)
- mark_child_for_cleanup(cmd->pid);
+ if ((cmd->clean_on_exit || cmd->wait_on_exit) && cmd->pid >= 0)
+ mark_child_for_cleanup(
+ cmd->pid, cmd->argv[0], cmd->in, cmd->clean_on_exit_timeout);
argv_array_clear(&nargv);
cmd->argv = sargv;
@@ -765,7 +804,7 @@ int start_async(struct async *async)
exit(!!async->proc(proc_in, proc_out, async->data));
}
- mark_child_for_cleanup(async->pid);
+ mark_child_for_cleanup_no_wait(async->pid);
if (need_in)
close(fdin[0]);
diff --git a/run-command.h b/run-command.h
index cf29a31..f7b9907 100644
--- a/run-command.h
+++ b/run-command.h
@@ -42,7 +42,10 @@ struct child_process {
unsigned silent_exec_failure:1;
unsigned stdout_to_stderr:1;
unsigned use_shell:1;
+ /* kill the child on Git exit */
unsigned clean_on_exit:1;
+ /* close the child's stdin on Git exit and wait until it terminates */
+ unsigned wait_on_exit:1;
};
#define CHILD_PROCESS_INIT { NULL, ARGV_ARRAY_INIT, ARGV_ARRAY_INIT }
--
2.10.0
^ permalink raw reply related
* [PATCH v9 03/14] run-command: move check_pipe() from write_or_die to run_command
From: larsxschneider @ 2016-10-04 12:59 UTC (permalink / raw)
To: git; +Cc: ramsay, jnareb, gitster, j6t, tboegi, peff, mlbright,
Lars Schneider
In-Reply-To: <20161004125947.67104-1-larsxschneider@gmail.com>
From: Lars Schneider <larsxschneider@gmail.com>
Move check_pipe() to run_command and make it public. This is necessary
to call the function from pkt-line in a subsequent patch.
While at it, make async_exit() static to run_command.c as it is no
longer used from outside.
Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
run-command.c | 17 +++++++++++++++--
run-command.h | 2 +-
write_or_die.c | 13 -------------
3 files changed, 16 insertions(+), 16 deletions(-)
diff --git a/run-command.c b/run-command.c
index 5a4dbb6..3269362 100644
--- a/run-command.c
+++ b/run-command.c
@@ -634,7 +634,7 @@ int in_async(void)
return !pthread_equal(main_thread, pthread_self());
}
-void NORETURN async_exit(int code)
+static void NORETURN async_exit(int code)
{
pthread_exit((void *)(intptr_t)code);
}
@@ -684,13 +684,26 @@ int in_async(void)
return process_is_async;
}
-void NORETURN async_exit(int code)
+static void NORETURN async_exit(int code)
{
exit(code);
}
#endif
+void check_pipe(int err)
+{
+ if (err == EPIPE) {
+ if (in_async())
+ async_exit(141);
+
+ signal(SIGPIPE, SIG_DFL);
+ raise(SIGPIPE);
+ /* Should never happen, but just in case... */
+ exit(141);
+ }
+}
+
int start_async(struct async *async)
{
int need_in, need_out;
diff --git a/run-command.h b/run-command.h
index 5066649..cf29a31 100644
--- a/run-command.h
+++ b/run-command.h
@@ -139,7 +139,7 @@ struct async {
int start_async(struct async *async);
int finish_async(struct async *async);
int in_async(void);
-void NORETURN async_exit(int code);
+void check_pipe(int err);
/**
* This callback should initialize the child process and preload the
diff --git a/write_or_die.c b/write_or_die.c
index 0734432..eab8c8d 100644
--- a/write_or_die.c
+++ b/write_or_die.c
@@ -1,19 +1,6 @@
#include "cache.h"
#include "run-command.h"
-static void check_pipe(int err)
-{
- if (err == EPIPE) {
- if (in_async())
- async_exit(141);
-
- signal(SIGPIPE, SIG_DFL);
- raise(SIGPIPE);
- /* Should never happen, but just in case... */
- exit(141);
- }
-}
-
/*
* Some cases use stdio, but want to flush after the write
* to get error handling (and to get better interactive
--
2.10.0
^ permalink raw reply related
* [PATCH v9 05/14] pkt-line: rename packet_write() to packet_write_fmt()
From: larsxschneider @ 2016-10-04 12:59 UTC (permalink / raw)
To: git; +Cc: ramsay, jnareb, gitster, j6t, tboegi, peff, mlbright,
Lars Schneider
In-Reply-To: <20161004125947.67104-1-larsxschneider@gmail.com>
From: Lars Schneider <larsxschneider@gmail.com>
packet_write() should be called packet_write_fmt() because it is a
printf-like function that takes a format string as first parameter.
packet_write_fmt() should be used for text strings only. Arbitrary
binary data should use a new packet_write() function that is introduced
in a subsequent patch.
Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin/archive.c | 4 ++--
builtin/receive-pack.c | 4 ++--
builtin/remote-ext.c | 4 ++--
builtin/upload-archive.c | 4 ++--
connect.c | 2 +-
daemon.c | 2 +-
http-backend.c | 2 +-
pkt-line.c | 2 +-
pkt-line.h | 2 +-
shallow.c | 2 +-
upload-pack.c | 30 +++++++++++++++---------------
11 files changed, 29 insertions(+), 29 deletions(-)
diff --git a/builtin/archive.c b/builtin/archive.c
index a1e3b94..49f4914 100644
--- a/builtin/archive.c
+++ b/builtin/archive.c
@@ -47,10 +47,10 @@ static int run_remote_archiver(int argc, const char **argv,
if (name_hint) {
const char *format = archive_format_from_filename(name_hint);
if (format)
- packet_write(fd[1], "argument --format=%s\n", format);
+ packet_write_fmt(fd[1], "argument --format=%s\n", format);
}
for (i = 1; i < argc; i++)
- packet_write(fd[1], "argument %s\n", argv[i]);
+ packet_write_fmt(fd[1], "argument %s\n", argv[i]);
packet_flush(fd[1]);
buf = packet_read_line(fd[0], NULL);
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 011db00..1ce7682 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -218,7 +218,7 @@ static int receive_pack_config(const char *var, const char *value, void *cb)
static void show_ref(const char *path, const unsigned char *sha1)
{
if (sent_capabilities) {
- packet_write(1, "%s %s\n", sha1_to_hex(sha1), path);
+ packet_write_fmt(1, "%s %s\n", sha1_to_hex(sha1), path);
} else {
struct strbuf cap = STRBUF_INIT;
@@ -233,7 +233,7 @@ static void show_ref(const char *path, const unsigned char *sha1)
if (advertise_push_options)
strbuf_addstr(&cap, " push-options");
strbuf_addf(&cap, " agent=%s", git_user_agent_sanitized());
- packet_write(1, "%s %s%c%s\n",
+ packet_write_fmt(1, "%s %s%c%s\n",
sha1_to_hex(sha1), path, 0, cap.buf);
strbuf_release(&cap);
sent_capabilities = 1;
diff --git a/builtin/remote-ext.c b/builtin/remote-ext.c
index 88eb8f9..11b48bf 100644
--- a/builtin/remote-ext.c
+++ b/builtin/remote-ext.c
@@ -128,9 +128,9 @@ static void send_git_request(int stdin_fd, const char *serv, const char *repo,
const char *vhost)
{
if (!vhost)
- packet_write(stdin_fd, "%s %s%c", serv, repo, 0);
+ packet_write_fmt(stdin_fd, "%s %s%c", serv, repo, 0);
else
- packet_write(stdin_fd, "%s %s%chost=%s%c", serv, repo, 0,
+ packet_write_fmt(stdin_fd, "%s %s%chost=%s%c", serv, repo, 0,
vhost, 0);
}
diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c
index 2caedf1..dc872f6 100644
--- a/builtin/upload-archive.c
+++ b/builtin/upload-archive.c
@@ -88,11 +88,11 @@ int cmd_upload_archive(int argc, const char **argv, const char *prefix)
writer.git_cmd = 1;
if (start_command(&writer)) {
int err = errno;
- packet_write(1, "NACK unable to spawn subprocess\n");
+ packet_write_fmt(1, "NACK unable to spawn subprocess\n");
die("upload-archive: %s", strerror(err));
}
- packet_write(1, "ACK\n");
+ packet_write_fmt(1, "ACK\n");
packet_flush(1);
while (1) {
diff --git a/connect.c b/connect.c
index 722dc3f..5330d9c 100644
--- a/connect.c
+++ b/connect.c
@@ -730,7 +730,7 @@ struct child_process *git_connect(int fd[2], const char *url,
* Note: Do not add any other headers here! Doing so
* will cause older git-daemon servers to crash.
*/
- packet_write(fd[1],
+ packet_write_fmt(fd[1],
"%s %s%chost=%s%c",
prog, path, 0,
target_host, 0);
diff --git a/daemon.c b/daemon.c
index 425aad0..afce1b9 100644
--- a/daemon.c
+++ b/daemon.c
@@ -281,7 +281,7 @@ static int daemon_error(const char *dir, const char *msg)
{
if (!informative_errors)
msg = "access denied or repository not exported";
- packet_write(1, "ERR %s: %s", msg, dir);
+ packet_write_fmt(1, "ERR %s: %s", msg, dir);
return -1;
}
diff --git a/http-backend.c b/http-backend.c
index adc8c8c..eef0a36 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -464,7 +464,7 @@ static void get_info_refs(struct strbuf *hdr, char *arg)
hdr_str(hdr, content_type, buf.buf);
end_headers(hdr);
- packet_write(1, "# service=git-%s\n", svc->name);
+ packet_write_fmt(1, "# service=git-%s\n", svc->name);
packet_flush(1);
argv[0] = svc->name;
diff --git a/pkt-line.c b/pkt-line.c
index 62fdb37..0a9b61c 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -118,7 +118,7 @@ static void format_packet(struct strbuf *out, const char *fmt, va_list args)
packet_trace(out->buf + orig_len + 4, n - 4, 1);
}
-void packet_write(int fd, const char *fmt, ...)
+void packet_write_fmt(int fd, const char *fmt, ...)
{
static struct strbuf buf = STRBUF_INIT;
va_list args;
diff --git a/pkt-line.h b/pkt-line.h
index 3cb9d91..1902fb3 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -20,7 +20,7 @@
* side can't, we stay with pure read/write interfaces.
*/
void packet_flush(int fd);
-void packet_write(int fd, const char *fmt, ...) __attribute__((format (printf, 2, 3)));
+void packet_write_fmt(int fd, const char *fmt, ...) __attribute__((format (printf, 2, 3)));
void packet_buf_flush(struct strbuf *buf);
void packet_buf_write(struct strbuf *buf, const char *fmt, ...) __attribute__((format (printf, 2, 3)));
diff --git a/shallow.c b/shallow.c
index 54e2db7..d666e24 100644
--- a/shallow.c
+++ b/shallow.c
@@ -260,7 +260,7 @@ static int advertise_shallow_grafts_cb(const struct commit_graft *graft, void *c
{
int fd = *(int *)cb;
if (graft->nr_parent == -1)
- packet_write(fd, "shallow %s\n", oid_to_hex(&graft->oid));
+ packet_write_fmt(fd, "shallow %s\n", oid_to_hex(&graft->oid));
return 0;
}
diff --git a/upload-pack.c b/upload-pack.c
index ca7f941..cd47de6 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -393,13 +393,13 @@ static int get_common_commits(void)
if (multi_ack == 2 && got_common
&& !got_other && ok_to_give_up()) {
sent_ready = 1;
- packet_write(1, "ACK %s ready\n", last_hex);
+ packet_write_fmt(1, "ACK %s ready\n", last_hex);
}
if (have_obj.nr == 0 || multi_ack)
- packet_write(1, "NAK\n");
+ packet_write_fmt(1, "NAK\n");
if (no_done && sent_ready) {
- packet_write(1, "ACK %s\n", last_hex);
+ packet_write_fmt(1, "ACK %s\n", last_hex);
return 0;
}
if (stateless_rpc)
@@ -416,20 +416,20 @@ static int get_common_commits(void)
const char *hex = sha1_to_hex(sha1);
if (multi_ack == 2) {
sent_ready = 1;
- packet_write(1, "ACK %s ready\n", hex);
+ packet_write_fmt(1, "ACK %s ready\n", hex);
} else
- packet_write(1, "ACK %s continue\n", hex);
+ packet_write_fmt(1, "ACK %s continue\n", hex);
}
break;
default:
got_common = 1;
memcpy(last_hex, sha1_to_hex(sha1), 41);
if (multi_ack == 2)
- packet_write(1, "ACK %s common\n", last_hex);
+ packet_write_fmt(1, "ACK %s common\n", last_hex);
else if (multi_ack)
- packet_write(1, "ACK %s continue\n", last_hex);
+ packet_write_fmt(1, "ACK %s continue\n", last_hex);
else if (have_obj.nr == 1)
- packet_write(1, "ACK %s\n", last_hex);
+ packet_write_fmt(1, "ACK %s\n", last_hex);
break;
}
continue;
@@ -437,10 +437,10 @@ static int get_common_commits(void)
if (!strcmp(line, "done")) {
if (have_obj.nr > 0) {
if (multi_ack)
- packet_write(1, "ACK %s\n", last_hex);
+ packet_write_fmt(1, "ACK %s\n", last_hex);
return 0;
}
- packet_write(1, "NAK\n");
+ packet_write_fmt(1, "NAK\n");
return -1;
}
die("git upload-pack: expected SHA1 list, got '%s'", line);
@@ -650,7 +650,7 @@ static void receive_needs(void)
while (result) {
struct object *object = &result->item->object;
if (!(object->flags & (CLIENT_SHALLOW|NOT_SHALLOW))) {
- packet_write(1, "shallow %s",
+ packet_write_fmt(1, "shallow %s",
oid_to_hex(&object->oid));
register_shallow(object->oid.hash);
shallow_nr++;
@@ -662,7 +662,7 @@ static void receive_needs(void)
struct object *object = shallows.objects[i].item;
if (object->flags & NOT_SHALLOW) {
struct commit_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 */
@@ -741,7 +741,7 @@ static int send_ref(const char *refname, const struct object_id *oid,
struct strbuf symref_info = STRBUF_INIT;
format_symref_info(&symref_info, cb_data);
- packet_write(1, "%s %s%c%s%s%s%s%s agent=%s\n",
+ packet_write_fmt(1, "%s %s%c%s%s%s%s%s agent=%s\n",
oid_to_hex(oid), refname_nons,
0, capabilities,
(allow_unadvertised_object_request & ALLOW_TIP_SHA1) ?
@@ -753,11 +753,11 @@ static int send_ref(const char *refname, const struct object_id *oid,
git_user_agent_sanitized());
strbuf_release(&symref_info);
} else {
- packet_write(1, "%s %s\n", oid_to_hex(oid), refname_nons);
+ packet_write_fmt(1, "%s %s\n", oid_to_hex(oid), refname_nons);
}
capabilities = NULL;
if (!peel_ref(refname, peeled.hash))
- packet_write(1, "%s %s^{}\n", oid_to_hex(&peeled), refname_nons);
+ packet_write_fmt(1, "%s %s^{}\n", oid_to_hex(&peeled), refname_nons);
return 0;
}
--
2.10.0
^ permalink raw reply related
* [PATCH v3 0/6] Pull out require_clean_work_tree() functionality from builtin/pull.c
From: Johannes Schindelin @ 2016-10-04 13:04 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <cover.1473580914.git.johannes.schindelin@gmx.de>
This is the 5th last patch series of my work to accelerate interactive
rebases in particular on Windows.
Basically, all it does is to make reusable some functions that were
ported over from git-pull.sh but made private to builtin/pull.c.
Changes since v2:
- added a hint for translators.
- changed the existing error messages to start with a lower-case, as per
our current convention (the previous error messages were inherited
from code written before that convention was in place).
- struck the "truly" adjective from the commit message, as it did not
get Junio's consent.
Johannes Schindelin (6):
pull: drop confusing prefix parameter of die_on_unclean_work_tree()
pull: make code more similar to the shell script again
Make the require_clean_work_tree() function reusable
Export also the has_un{staged,committed}_changed() functions
wt-status: teach has_{unstaged,uncommitted}_changes() about submodules
wt-status: begin error messages with lower-case
builtin/pull.c | 71 +++-------------------------------------------------
wt-status.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
wt-status.h | 6 +++++
3 files changed, 87 insertions(+), 68 deletions(-)
Published-As: https://github.com/dscho/git/releases/tag/require-clean-work-tree-v3
Fetch-It-Via: git fetch https://github.com/dscho/git require-clean-work-tree-v3
Interdiff vs v2:
diff --git a/builtin/pull.c b/builtin/pull.c
index c639167..0bf9802 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -810,7 +810,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
if (!autostash)
require_clean_work_tree(N_("pull with rebase"),
- "Please commit or stash them.", 1, 0);
+ "please commit or stash them.", 1, 0);
if (get_rebase_fork_point(rebase_fork_point, repo, *refspecs))
hashclr(rebase_fork_point);
diff --git a/wt-status.c b/wt-status.c
index a86918a..010276b 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -2264,15 +2264,16 @@ int require_clean_work_tree(const char *action, const char *hint, int ignore_sub
rollback_lock_file(lock_file);
if (has_unstaged_changes(ignore_submodules)) {
- error(_("Cannot %s: You have unstaged changes."), _(action));
+ /* TRANSLATORS: the action is e.g. "pull with rebase" */
+ error(_("cannot %s: You have unstaged changes."), _(action));
err = 1;
}
if (has_uncommitted_changes(ignore_submodules)) {
if (err)
- error(_("Additionally, your index contains uncommitted changes."));
+ error(_("additionally, your index contains uncommitted changes."));
else
- error(_("Cannot %s: Your index contains uncommitted changes."),
+ error(_("cannot %s: Your index contains uncommitted changes."),
_(action));
err = 1;
}
--
2.10.0.windows.1.325.ge6089c1
base-commit: 0cf36115dce7438a0eafad54a81cc57175e8fb54
^ permalink raw reply
* [PATCH v3 1/6] pull: drop confusing prefix parameter of die_on_unclean_work_tree()
From: Johannes Schindelin @ 2016-10-04 13:05 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <cover.1475586229.git.johannes.schindelin@gmx.de>
In cmd_pull(), when verifying that there are no changes preventing a
rebasing pull, we diligently pass the prefix parameter to the
die_on_unclean_work_tree() function which in turn diligently passes it
to the has_unstaged_changes() and has_uncommitted_changes() functions.
The casual reader might now be curious (as this developer was) whether
that means that calling `git pull --rebase` in a subdirectory will
ignore unstaged changes in other parts of the working directory. And be
puzzled that `git pull --rebase` (correctly) complains about those
changes outside of the current directory.
The puzzle is easily resolved: while we take pains to pass around the
prefix and even pass it to init_revisions(), the fact that no paths are
passed to init_revisions() ensures that the prefix is simply ignored.
That, combined with the fact that we will *always* want a *full* working
directory check before running a rebasing pull, is reason enough to
simply do away with the actual prefix parameter and to pass NULL
instead, as if we were running this from the top-level working directory
anyway.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/pull.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/builtin/pull.c b/builtin/pull.c
index 398aae1..d4bd635 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -328,12 +328,12 @@ static int git_pull_config(const char *var, const char *value, void *cb)
/**
* Returns 1 if there are unstaged changes, 0 otherwise.
*/
-static int has_unstaged_changes(const char *prefix)
+static int has_unstaged_changes(void)
{
struct rev_info rev_info;
int result;
- init_revisions(&rev_info, prefix);
+ init_revisions(&rev_info, NULL);
DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);
DIFF_OPT_SET(&rev_info.diffopt, QUICK);
diff_setup_done(&rev_info.diffopt);
@@ -344,7 +344,7 @@ static int has_unstaged_changes(const char *prefix)
/**
* Returns 1 if there are uncommitted changes, 0 otherwise.
*/
-static int has_uncommitted_changes(const char *prefix)
+static int has_uncommitted_changes(void)
{
struct rev_info rev_info;
int result;
@@ -352,7 +352,7 @@ static int has_uncommitted_changes(const char *prefix)
if (is_cache_unborn())
return 0;
- init_revisions(&rev_info, prefix);
+ init_revisions(&rev_info, NULL);
DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);
DIFF_OPT_SET(&rev_info.diffopt, QUICK);
add_head_to_pending(&rev_info);
@@ -365,7 +365,7 @@ static int has_uncommitted_changes(const char *prefix)
* If the work tree has unstaged or uncommitted changes, dies with the
* appropriate message.
*/
-static void die_on_unclean_work_tree(const char *prefix)
+static void die_on_unclean_work_tree(void)
{
struct lock_file *lock_file = xcalloc(1, sizeof(*lock_file));
int do_die = 0;
@@ -375,12 +375,12 @@ static void die_on_unclean_work_tree(const char *prefix)
update_index_if_able(&the_index, lock_file);
rollback_lock_file(lock_file);
- if (has_unstaged_changes(prefix)) {
+ if (has_unstaged_changes()) {
error(_("Cannot pull with rebase: You have unstaged changes."));
do_die = 1;
}
- if (has_uncommitted_changes(prefix)) {
+ if (has_uncommitted_changes()) {
if (do_die)
error(_("Additionally, your index contains uncommitted changes."));
else
@@ -875,7 +875,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
die(_("Updating an unborn branch with changes added to the index."));
if (!autostash)
- die_on_unclean_work_tree(prefix);
+ die_on_unclean_work_tree();
if (get_rebase_fork_point(rebase_fork_point, repo, *refspecs))
hashclr(rebase_fork_point);
--
2.10.0.windows.1.325.ge6089c1
^ permalink raw reply related
* [PATCH v3 2/6] pull: make code more similar to the shell script again
From: Johannes Schindelin @ 2016-10-04 13:05 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <cover.1475586229.git.johannes.schindelin@gmx.de>
When converting the pull command to a builtin, the
require_clean_work_tree() function was renamed and the pull-specific
parts hard-coded.
This makes it impossible to reuse the code, so let's modify the code to
make it more similar to the original shell script again.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/pull.c | 30 ++++++++++++++++++++----------
1 file changed, 20 insertions(+), 10 deletions(-)
diff --git a/builtin/pull.c b/builtin/pull.c
index d4bd635..d1e093c 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -365,10 +365,11 @@ static int has_uncommitted_changes(void)
* If the work tree has unstaged or uncommitted changes, dies with the
* appropriate message.
*/
-static void die_on_unclean_work_tree(void)
+static int require_clean_work_tree(const char *action, const char *hint,
+ int gently)
{
struct lock_file *lock_file = xcalloc(1, sizeof(*lock_file));
- int do_die = 0;
+ int err = 0;
hold_locked_index(lock_file, 0);
refresh_cache(REFRESH_QUIET);
@@ -376,20 +377,28 @@ static void die_on_unclean_work_tree(void)
rollback_lock_file(lock_file);
if (has_unstaged_changes()) {
- error(_("Cannot pull with rebase: You have unstaged changes."));
- do_die = 1;
+ /* TRANSLATORS: the action is e.g. "pull with rebase" */
+ error(_("Cannot %s: You have unstaged changes."), _(action));
+ err = 1;
}
if (has_uncommitted_changes()) {
- if (do_die)
+ if (err)
error(_("Additionally, your index contains uncommitted changes."));
else
- error(_("Cannot pull with rebase: Your index contains uncommitted changes."));
- do_die = 1;
+ error(_("Cannot %s: Your index contains uncommitted changes."),
+ _(action));
+ err = 1;
}
- if (do_die)
- exit(1);
+ if (err) {
+ if (hint)
+ error("%s", hint);
+ if (!gently)
+ exit(err);
+ }
+
+ return err;
}
/**
@@ -875,7 +884,8 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
die(_("Updating an unborn branch with changes added to the index."));
if (!autostash)
- die_on_unclean_work_tree();
+ require_clean_work_tree(N_("pull with rebase"),
+ "Please commit or stash them.", 0);
if (get_rebase_fork_point(rebase_fork_point, repo, *refspecs))
hashclr(rebase_fork_point);
--
2.10.0.windows.1.325.ge6089c1
^ permalink raw reply related
* [PATCH v3 3/6] Make the require_clean_work_tree() function reusable
From: Johannes Schindelin @ 2016-10-04 13:05 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <cover.1475586229.git.johannes.schindelin@gmx.de>
It is remarkable that libgit.a did not sport this function yet... Let's
move it into a more prominent (and into an actually reusable) spot:
wt-status.[ch].
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/pull.c | 77 +---------------------------------------------------------
wt-status.c | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
wt-status.h | 3 +++
3 files changed, 80 insertions(+), 76 deletions(-)
diff --git a/builtin/pull.c b/builtin/pull.c
index d1e093c..14ef8b5 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -17,6 +17,7 @@
#include "revision.h"
#include "tempfile.h"
#include "lockfile.h"
+#include "wt-status.h"
enum rebase_type {
REBASE_INVALID = -1,
@@ -326,82 +327,6 @@ static int git_pull_config(const char *var, const char *value, void *cb)
}
/**
- * Returns 1 if there are unstaged changes, 0 otherwise.
- */
-static int has_unstaged_changes(void)
-{
- struct rev_info rev_info;
- int result;
-
- init_revisions(&rev_info, NULL);
- DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);
- DIFF_OPT_SET(&rev_info.diffopt, QUICK);
- diff_setup_done(&rev_info.diffopt);
- result = run_diff_files(&rev_info, 0);
- return diff_result_code(&rev_info.diffopt, result);
-}
-
-/**
- * Returns 1 if there are uncommitted changes, 0 otherwise.
- */
-static int has_uncommitted_changes(void)
-{
- struct rev_info rev_info;
- int result;
-
- if (is_cache_unborn())
- return 0;
-
- init_revisions(&rev_info, NULL);
- DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);
- DIFF_OPT_SET(&rev_info.diffopt, QUICK);
- add_head_to_pending(&rev_info);
- diff_setup_done(&rev_info.diffopt);
- result = run_diff_index(&rev_info, 1);
- return diff_result_code(&rev_info.diffopt, result);
-}
-
-/**
- * If the work tree has unstaged or uncommitted changes, dies with the
- * appropriate message.
- */
-static int require_clean_work_tree(const char *action, const char *hint,
- int gently)
-{
- struct lock_file *lock_file = xcalloc(1, sizeof(*lock_file));
- int err = 0;
-
- hold_locked_index(lock_file, 0);
- refresh_cache(REFRESH_QUIET);
- update_index_if_able(&the_index, lock_file);
- rollback_lock_file(lock_file);
-
- if (has_unstaged_changes()) {
- /* TRANSLATORS: the action is e.g. "pull with rebase" */
- error(_("Cannot %s: You have unstaged changes."), _(action));
- err = 1;
- }
-
- if (has_uncommitted_changes()) {
- if (err)
- error(_("Additionally, your index contains uncommitted changes."));
- else
- error(_("Cannot %s: Your index contains uncommitted changes."),
- _(action));
- err = 1;
- }
-
- if (err) {
- if (hint)
- error("%s", hint);
- if (!gently)
- exit(err);
- }
-
- return err;
-}
-
-/**
* Appends merge candidates from FETCH_HEAD that are not marked not-for-merge
* into merge_heads.
*/
diff --git a/wt-status.c b/wt-status.c
index 9628c1d..b92c54d 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -16,6 +16,7 @@
#include "strbuf.h"
#include "utf8.h"
#include "worktree.h"
+#include "lockfile.h"
static const char cut_line[] =
"------------------------ >8 ------------------------\n";
@@ -2209,3 +2210,78 @@ void wt_status_print(struct wt_status *s)
break;
}
}
+
+/**
+ * Returns 1 if there are unstaged changes, 0 otherwise.
+ */
+static int has_unstaged_changes(void)
+{
+ struct rev_info rev_info;
+ int result;
+
+ init_revisions(&rev_info, NULL);
+ DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);
+ DIFF_OPT_SET(&rev_info.diffopt, QUICK);
+ diff_setup_done(&rev_info.diffopt);
+ result = run_diff_files(&rev_info, 0);
+ return diff_result_code(&rev_info.diffopt, result);
+}
+
+/**
+ * Returns 1 if there are uncommitted changes, 0 otherwise.
+ */
+static int has_uncommitted_changes(void)
+{
+ struct rev_info rev_info;
+ int result;
+
+ if (is_cache_unborn())
+ return 0;
+
+ init_revisions(&rev_info, NULL);
+ DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);
+ DIFF_OPT_SET(&rev_info.diffopt, QUICK);
+ add_head_to_pending(&rev_info);
+ diff_setup_done(&rev_info.diffopt);
+ result = run_diff_index(&rev_info, 1);
+ return diff_result_code(&rev_info.diffopt, result);
+}
+
+/**
+ * If the work tree has unstaged or uncommitted changes, dies with the
+ * appropriate message.
+ */
+int require_clean_work_tree(const char *action, const char *hint, int gently)
+{
+ struct lock_file *lock_file = xcalloc(1, sizeof(*lock_file));
+ int err = 0;
+
+ hold_locked_index(lock_file, 0);
+ refresh_cache(REFRESH_QUIET);
+ update_index_if_able(&the_index, lock_file);
+ rollback_lock_file(lock_file);
+
+ if (has_unstaged_changes()) {
+ /* TRANSLATORS: the action is e.g. "pull with rebase" */
+ error(_("Cannot %s: You have unstaged changes."), _(action));
+ err = 1;
+ }
+
+ if (has_uncommitted_changes()) {
+ if (err)
+ error(_("Additionally, your index contains uncommitted changes."));
+ else
+ error(_("Cannot %s: Your index contains uncommitted changes."),
+ _(action));
+ err = 1;
+ }
+
+ if (err) {
+ if (hint)
+ error("%s", hint);
+ if (!gently)
+ exit(err);
+ }
+
+ return err;
+}
diff --git a/wt-status.h b/wt-status.h
index e401837..03ecf53 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -128,4 +128,7 @@ void status_printf_ln(struct wt_status *s, const char *color, const char *fmt, .
__attribute__((format (printf, 3, 4)))
void status_printf(struct wt_status *s, const char *color, const char *fmt, ...);
+/* The following function expect that the caller took care of reading the index. */
+int require_clean_work_tree(const char *action, const char *hint, int gently);
+
#endif /* STATUS_H */
--
2.10.0.windows.1.325.ge6089c1
^ permalink raw reply related
* [PATCH v3 4/6] Export also the has_un{staged,committed}_changed() functions
From: Johannes Schindelin @ 2016-10-04 13:05 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <cover.1475586229.git.johannes.schindelin@gmx.de>
They will be used in the upcoming rebase helper.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
wt-status.c | 4 ++--
wt-status.h | 4 +++-
2 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/wt-status.c b/wt-status.c
index b92c54d..f4103e4 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -2214,7 +2214,7 @@ void wt_status_print(struct wt_status *s)
/**
* Returns 1 if there are unstaged changes, 0 otherwise.
*/
-static int has_unstaged_changes(void)
+int has_unstaged_changes(void)
{
struct rev_info rev_info;
int result;
@@ -2230,7 +2230,7 @@ static int has_unstaged_changes(void)
/**
* Returns 1 if there are uncommitted changes, 0 otherwise.
*/
-static int has_uncommitted_changes(void)
+int has_uncommitted_changes(void)
{
struct rev_info rev_info;
int result;
diff --git a/wt-status.h b/wt-status.h
index 03ecf53..68e367a 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -128,7 +128,9 @@ void status_printf_ln(struct wt_status *s, const char *color, const char *fmt, .
__attribute__((format (printf, 3, 4)))
void status_printf(struct wt_status *s, const char *color, const char *fmt, ...);
-/* The following function expect that the caller took care of reading the index. */
+/* The following functions expect that the caller took care of reading the index. */
+int has_unstaged_changes(void);
+int has_uncommitted_changes(void);
int require_clean_work_tree(const char *action, const char *hint, int gently);
#endif /* STATUS_H */
--
2.10.0.windows.1.325.ge6089c1
^ permalink raw reply related
* [PATCH v3 5/6] wt-status: teach has_{unstaged,uncommitted}_changes() about submodules
From: Johannes Schindelin @ 2016-10-04 13:05 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <cover.1475586229.git.johannes.schindelin@gmx.de>
Sometimes we are *actually* interested in those changes... For
example when an interactive rebase wants to continue with a staged
submodule update.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/pull.c | 2 +-
wt-status.c | 16 +++++++++-------
wt-status.h | 7 ++++---
3 files changed, 14 insertions(+), 11 deletions(-)
diff --git a/builtin/pull.c b/builtin/pull.c
index 14ef8b5..c639167 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -810,7 +810,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
if (!autostash)
require_clean_work_tree(N_("pull with rebase"),
- "Please commit or stash them.", 0);
+ "Please commit or stash them.", 1, 0);
if (get_rebase_fork_point(rebase_fork_point, repo, *refspecs))
hashclr(rebase_fork_point);
diff --git a/wt-status.c b/wt-status.c
index f4103e4..061597a 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -2214,13 +2214,14 @@ void wt_status_print(struct wt_status *s)
/**
* Returns 1 if there are unstaged changes, 0 otherwise.
*/
-int has_unstaged_changes(void)
+int has_unstaged_changes(int ignore_submodules)
{
struct rev_info rev_info;
int result;
init_revisions(&rev_info, NULL);
- DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);
+ if (ignore_submodules)
+ DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);
DIFF_OPT_SET(&rev_info.diffopt, QUICK);
diff_setup_done(&rev_info.diffopt);
result = run_diff_files(&rev_info, 0);
@@ -2230,7 +2231,7 @@ int has_unstaged_changes(void)
/**
* Returns 1 if there are uncommitted changes, 0 otherwise.
*/
-int has_uncommitted_changes(void)
+int has_uncommitted_changes(int ignore_submodules)
{
struct rev_info rev_info;
int result;
@@ -2239,7 +2240,8 @@ int has_uncommitted_changes(void)
return 0;
init_revisions(&rev_info, NULL);
- DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);
+ if (ignore_submodules)
+ DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);
DIFF_OPT_SET(&rev_info.diffopt, QUICK);
add_head_to_pending(&rev_info);
diff_setup_done(&rev_info.diffopt);
@@ -2251,7 +2253,7 @@ int has_uncommitted_changes(void)
* If the work tree has unstaged or uncommitted changes, dies with the
* appropriate message.
*/
-int require_clean_work_tree(const char *action, const char *hint, int gently)
+int require_clean_work_tree(const char *action, const char *hint, int ignore_submodules, int gently)
{
struct lock_file *lock_file = xcalloc(1, sizeof(*lock_file));
int err = 0;
@@ -2261,13 +2263,13 @@ int require_clean_work_tree(const char *action, const char *hint, int gently)
update_index_if_able(&the_index, lock_file);
rollback_lock_file(lock_file);
- if (has_unstaged_changes()) {
+ if (has_unstaged_changes(ignore_submodules)) {
/* TRANSLATORS: the action is e.g. "pull with rebase" */
error(_("Cannot %s: You have unstaged changes."), _(action));
err = 1;
}
- if (has_uncommitted_changes()) {
+ if (has_uncommitted_changes(ignore_submodules)) {
if (err)
error(_("Additionally, your index contains uncommitted changes."));
else
diff --git a/wt-status.h b/wt-status.h
index 68e367a..54fec77 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -129,8 +129,9 @@ __attribute__((format (printf, 3, 4)))
void status_printf(struct wt_status *s, const char *color, const char *fmt, ...);
/* The following functions expect that the caller took care of reading the index. */
-int has_unstaged_changes(void);
-int has_uncommitted_changes(void);
-int require_clean_work_tree(const char *action, const char *hint, int gently);
+int has_unstaged_changes(int ignore_submodules);
+int has_uncommitted_changes(int ignore_submodules);
+int require_clean_work_tree(const char *action, const char *hint,
+ int ignore_submodules, int gently);
#endif /* STATUS_H */
--
2.10.0.windows.1.325.ge6089c1
^ permalink raw reply related
* [PATCH v3 6/6] wt-status: begin error messages with lower-case
From: Johannes Schindelin @ 2016-10-04 13:06 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <cover.1475586229.git.johannes.schindelin@gmx.de>
The previous code still followed the old git-pull.sh code which did not
adhere to our new convention.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/pull.c | 2 +-
wt-status.c | 6 +++---
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/builtin/pull.c b/builtin/pull.c
index c639167..0bf9802 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -810,7 +810,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
if (!autostash)
require_clean_work_tree(N_("pull with rebase"),
- "Please commit or stash them.", 1, 0);
+ "please commit or stash them.", 1, 0);
if (get_rebase_fork_point(rebase_fork_point, repo, *refspecs))
hashclr(rebase_fork_point);
diff --git a/wt-status.c b/wt-status.c
index 061597a..010276b 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -2265,15 +2265,15 @@ int require_clean_work_tree(const char *action, const char *hint, int ignore_sub
if (has_unstaged_changes(ignore_submodules)) {
/* TRANSLATORS: the action is e.g. "pull with rebase" */
- error(_("Cannot %s: You have unstaged changes."), _(action));
+ error(_("cannot %s: You have unstaged changes."), _(action));
err = 1;
}
if (has_uncommitted_changes(ignore_submodules)) {
if (err)
- error(_("Additionally, your index contains uncommitted changes."));
+ error(_("additionally, your index contains uncommitted changes."));
else
- error(_("Cannot %s: Your index contains uncommitted changes."),
+ error(_("cannot %s: Your index contains uncommitted changes."),
_(action));
err = 1;
}
--
2.10.0.windows.1.325.ge6089c1
^ permalink raw reply related
* Re: What's cooking in git.git (Oct 2016, #01; Mon, 3)
From: Lars Schneider @ 2016-10-04 13:07 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqtwct3w0p.fsf@gitster.mtv.corp.google.com>
> On 04 Oct 2016, at 00:31, Junio C Hamano <gitster@pobox.com> wrote:
>
>
> * ls/filter-process (2016-09-23) 11 commits
> - convert: add filter.<driver>.process option
> - convert: make apply_filter() adhere to standard Git error handling
> - convert: modernize tests
> - convert: quote filter names in error messages
> - pkt-line: add functions to read/write flush terminated packet streams
> - pkt-line: add packet_write_gently()
> - pkt-line: add packet_flush_gently()
> - pkt-line: add packet_write_fmt_gently()
> - run-command: move check_pipe() from write_or_die to run_command
> - pkt-line: extract set_packet_header()
> - pkt-line: rename packet_write() to packet_write_fmt()
>
> The smudge/clean filter API expect an external process is spawned
> to filter the contents for each path that has a filter defined. A
> new type of "process" filter API has been added to allow the first
> request to run the filter for a path to spawn a single process, and
> all filtering need is served by this single process for multiple
> paths, reducing the process creation overhead.
>
> Somehow I thought this was getting ready for 'next' but it seems
> at least another round of reroll is coming?
I thought so, too, but t0021 was flaky and Jakub had a number of good
suggestions. I just posted v9:
http://public-inbox.org/git/20161004125947.67104-1-larsxschneider@gmail.com/
- Lars
^ permalink raw reply
* Re: [PATCH 0/18] alternate object database cleanups
From: Jeff King @ 2016-10-04 13:41 UTC (permalink / raw)
To: Jacob Keller; +Cc: Git mailing list, René Scharfe
In-Reply-To: <CA+P7+xob7ohj1MxuLGGLwwJyi4RfqUTeLkbw86u+VvbU=uEyAw@mail.gmail.com>
On Mon, Oct 03, 2016 at 10:47:31PM -0700, Jacob Keller wrote:
> > The number of patches is a little intimidating, but I tried hard to
> > break the refactoring down into a sequence of obviously-correct steps.
> > You can be the judge of my success.
>
> I read through them once. I'm going to re-read through them again and
> leave any comments I had.
Thanks for having the fortitude to read them all. :) After looking at
your comments, I don't _think_ there's anything that necessitates a
re-roll, but I'll respond to a few of them individually.
-Peff
^ permalink raw reply
* Re: [PATCH 01/18] t5613: drop reachable_via function
From: Jeff King @ 2016-10-04 13:43 UTC (permalink / raw)
To: Jacob Keller; +Cc: Git mailing list, René Scharfe
In-Reply-To: <CA+P7+xq3G=CNHNNQ5YpkHkud_5SUrTBOwZ3y7d8DvM9nKyXV9g@mail.gmail.com>
On Mon, Oct 03, 2016 at 10:48:31PM -0700, Jacob Keller wrote:
> On Mon, Oct 3, 2016 at 1:33 PM, Jeff King <peff@peff.net> wrote:
> > This function was never used since its inception in dd05ea1
> > (test case for transitive info/alternates, 2006-05-07).
> > Which is just as well, since it mutates the repo state in a
> > way that would invalidate further tests, without cleaning up
> > after itself. Let's get rid of it so that nobody is tempted
> > to use it.
> >
>
> Makes sense. It wouldn't be a good idea to leave this around since it
> didn't clean up after itself. Curious why no test actually used it
> though..
I wondered that, too. Sometimes in cases like this a call got dropped
during a re-roll on the list. But the original email:
http://public-inbox.org/git/20060507181947.GE23738@admingilde.org/
has the problem, too. So probably it was just leftover cruft that was
made obsolete even before it hit the list.
-Peff
^ permalink raw reply
* Re: [PATCH 04/18] t5613: whitespace/style cleanups
From: Jeff King @ 2016-10-04 13:47 UTC (permalink / raw)
To: Jacob Keller; +Cc: Git mailing list, René Scharfe
In-Reply-To: <CA+P7+xoX6XsKRFUCx+vhUo7ARYMEXktzcbFS=zh1NUTgRdhdUA@mail.gmail.com>
On Mon, Oct 03, 2016 at 10:52:39PM -0700, Jacob Keller wrote:
> On Mon, Oct 3, 2016 at 1:34 PM, Jeff King <peff@peff.net> wrote:
> > Our normal test style these days puts the opening quote of
> > the body on the description line, and indents the body with
> > a single tab. This ancient test did not follow this.
> >
>
> I was surprised you didn't do this first, but it doesn't really make a
> difference either way. This is also a pretty straight forward
> improvement, and I can see why you'd want to split this out to review
> separately.
I was trying to leave it to the end, to move the substantive changes up
front (and because there _isn't_ a correct style for some of the things
it was doing). But it just got too painful to do the "don't chdir" patch
without updating the style. I agree it might have made more sense at the
very beginning, but I didn't think it mattered enough to go through the
trouble of rebasing the earlier patches (which would essentially be
rewriting them).
-Peff
^ permalink raw reply
* Re: [PATCH 06/18] t5613: clarify "too deep" recursion tests
From: Jeff King @ 2016-10-04 13:48 UTC (permalink / raw)
To: Jacob Keller; +Cc: Git mailing list, René Scharfe
In-Reply-To: <CA+P7+xok5PoNKO+8R6zF9SXYfDq6BboDTDz9WZYEczs0pFK+pw@mail.gmail.com>
On Mon, Oct 03, 2016 at 10:57:48PM -0700, Jacob Keller wrote:
> > diff --git a/t/t5613-info-alternate.sh b/t/t5613-info-alternate.sh
> > index 7bc1c3c..b393613 100755
> > --- a/t/t5613-info-alternate.sh
> > +++ b/t/t5613-info-alternate.sh
> > @@ -39,6 +39,18 @@ test_expect_success 'preparing third repository' '
> > )
> > '
> >
> > +# Note: These tests depend on the hard-coded value of 5 as "too deep". We start
> > +# the depth at 0 and count links, not repositories, so in a chain like:
> > +#
> > +# A -> B -> C -> D -> E -> F -> G -> H
> > +# 0 1 2 3 4 5 6
> > +#
>
> Ok so we count links, but wouldn't we have 5 links when we hit F, and
> not G? Or am I missing something here?
This is what I was trying to get at with the "start the depth at 0". We
disallow a depth greater than 5, but because we start at 0-counting,
it's really six links. I guess saying "5 as too deep" is really the
misleading part. It should be "5 as the maximum depth".
-Peff
^ permalink raw reply
* Re: [PATCH 08/18] link_alt_odb_entry: refactor string handling
From: Jeff King @ 2016-10-04 13:53 UTC (permalink / raw)
To: Jacob Keller; +Cc: Git mailing list, René Scharfe
In-Reply-To: <CA+P7+xrUOnDebwZnfu-xv-GuTJka4-eNUAfBudQf5ZhnkczU6w@mail.gmail.com>
On Mon, Oct 03, 2016 at 11:05:42PM -0700, Jacob Keller wrote:
> This definitely makes reading the following function much easier,
> though the diff is a bit funky. I think the end result is much
> clearer.
Yeah, it's really hard to see that all of the "ent" setup is kept,
because it moves _and_ changes its content (from pfxlen to pathbuf.len).
I actually tried to split this into two patches to make the diff easier
to read, but there are two mutually dependent changes: moving to
pathbuf.len everywhere requires not-freeing pathbuf in the early code
path. But if you do that and don't move all of "is it usable" checks up,
then you have to add a bunch of new error-handling code that would just
get ripped out in the next patch.
There's definitely _some_ of that in this series already (e.g., the
counting logic in alt_sha1_path() added by patch 14 that just gets
ripped out in patch 15 when fill_sha1_path() learns to use a strbuf). I
tried to balance "show each individual obvious step" with "don't make
people review a bunch of scaffolding that's not going to be in the final
product".
-Peff
^ permalink raw reply
* Re: [PATCH 16/18] count-objects: report alternates via verbose mode
From: Jeff King @ 2016-10-04 13:56 UTC (permalink / raw)
To: Jacob Keller; +Cc: Git mailing list, René Scharfe
In-Reply-To: <CA+P7+xoR-14fmzrYU_sGoTRgcfP8fe5y-+kSkoM-2=E3Jb56FQ@mail.gmail.com>
On Mon, Oct 03, 2016 at 11:46:32PM -0700, Jacob Keller wrote:
> \On Mon, Oct 3, 2016 at 1:36 PM, Jeff King <peff@peff.net> wrote:
> > There's no way to get the list of alternates that git
> > computes internally; our tests only infer it based on which
> > objects are available. In addition to testing, knowing this
> > list may be helpful for somebody debugging their alternates
> > setup.
> >
> > Let's add it to the "count-objects -v" output. We could give
> > it a separate flag, but there's not really any need.
> > "count-objects -v" is already a debugging catch-all for the
> > object database, its output is easily extensible to new data
> > items, and printing the alternates is not expensive (we
> > already had to find them to count the objects).
> >
>
> Makes sense. Unless there's a compelling reason you'd want to print
> out these alternates *without* anything else from -v, but you can just
> use grep like the test does so this seems fine to me.
Yeah. I could definitely be persuaded otherwise, but I just couldn't see
these being used for anything useful beyond debugging.
-Peff
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox