[PATCH 3/3] Documentation: add trailer.trimEmpty config variable
Signed-off-by: Christian Couder chrisc...@tuxfamily.org --- Documentation/git-interpret-trailers.txt | 11 +-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt index d6d9231..816dd65 100644 --- a/Documentation/git-interpret-trailers.txt +++ b/Documentation/git-interpret-trailers.txt @@ -64,10 +64,12 @@ folding rules, the encoding rules and probably many other rules. OPTIONS --- ---trim-empty:: +--[no-]trim-empty:: If the value part of any trailer contains only whitespace, the whole trailer will be removed from the resulting message. - This apply to existing trailers as well as new trailers. + This applies to existing trailers as well as new + trailers. This overrides a 'trailer.trimempty' configuration + variable. --trailer token[(=|:)value]:: Specify a (token, value) pair that should be applied as a @@ -77,6 +79,11 @@ OPTIONS CONFIGURATION VARIABLES --- +trailer.trimempty:: + This option tells if by default trailers with an empty value + part should be trimmed or not. See also the '--[no-]trim-empty' + option above. + trailer.separators:: This option tells which characters are recognized as trailer separators. By default only ':' is recognized as a trailer -- 2.2.1.313.gcc831f2 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/3] trailer: add a trailer.trimEmpty config option
This way people who always want trimed trailers
don't need to specify it on the command line.
Signed-off-by: Christian Couder chrisc...@tuxfamily.org
---
builtin/interpret-trailers.c | 2 +-
trailer.c| 13 ++---
trailer.h| 2 +-
3 files changed, 12 insertions(+), 5 deletions(-)
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index 46838d2..1adf814 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -18,7 +18,7 @@ static const char * const git_interpret_trailers_usage[] = {
int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
{
- int trim_empty = 0;
+ int trim_empty = -1;
struct string_list trailers = STRING_LIST_INIT_DUP;
struct option options[] = {
diff --git a/trailer.c b/trailer.c
index 623adeb..7614182 100644
--- a/trailer.c
+++ b/trailer.c
@@ -36,6 +36,8 @@ static struct trailer_item *first_conf_item;
static char *separators = :;
+static int trim_empty;
+
#define TRAILER_ARG_STRING $ARG
static int after_or_end(enum action_where where)
@@ -120,7 +122,7 @@ static void print_tok_val(const char *tok, const char *val)
printf(%s%c %s\n, tok, separators[0], val);
}
-static void print_all(struct trailer_item *first, int trim_empty)
+static void print_all(struct trailer_item *first)
{
struct trailer_item *item;
for (item = first; item; item = item-next) {
@@ -509,6 +511,8 @@ static int git_trailer_default_config(const char *conf_key,
const char *value, v
value, conf_key);
} else if (!strcmp(trailer_item, separators)) {
separators = xstrdup(value);
+ } else if (!strcmp(trailer_item, trimempty)) {
+ trim_empty = git_config_bool(conf_key, value);
}
}
return 0;
@@ -842,7 +846,7 @@ static void free_all(struct trailer_item **first)
}
}
-void process_trailers(const char *file, int trim_empty, struct string_list
*trailers)
+void process_trailers(const char *file, int trim, struct string_list *trailers)
{
struct trailer_item *in_tok_first = NULL;
struct trailer_item *in_tok_last = NULL;
@@ -854,6 +858,9 @@ void process_trailers(const char *file, int trim_empty,
struct string_list *trai
git_config(git_trailer_default_config, NULL);
git_config(git_trailer_config, NULL);
+ if (trim -1)
+ trim_empty = trim;
+
lines = read_input_file(file);
/* Print the lines before the trailers */
@@ -863,7 +870,7 @@ void process_trailers(const char *file, int trim_empty,
struct string_list *trai
process_trailers_lists(in_tok_first, in_tok_last, arg_tok_first);
- print_all(in_tok_first, trim_empty);
+ print_all(in_tok_first);
free_all(in_tok_first);
diff --git a/trailer.h b/trailer.h
index 8eb25d5..4f481d0 100644
--- a/trailer.h
+++ b/trailer.h
@@ -1,6 +1,6 @@
#ifndef TRAILER_H
#define TRAILER_H
-void process_trailers(const char *file, int trim_empty, struct string_list
*trailers);
+void process_trailers(const char *file, int trim, struct string_list
*trailers);
#endif /* TRAILER_H */
--
2.2.1.313.gcc831f2
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/3] trailer: add tests for trailer.trimEmpty
Signed-off-by: Christian Couder chrisc...@tuxfamily.org --- t/t7513-interpret-trailers.sh | 72 +++ 1 file changed, 72 insertions(+) diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh index bd0ab46..066d00b 100755 --- a/t/t7513-interpret-trailers.sh +++ b/t/t7513-interpret-trailers.sh @@ -892,4 +892,76 @@ test_expect_success 'with no command and no key' ' test_cmp expected actual ' +test_expect_success 'with trailer.trimEmpty set to true' ' + git config trailer.trimEmpty true + cat expected -EOF + + sign: A U Thor aut...@example.com + Thanks-to: Johannes + EOF + git interpret-trailers --trailer review: \ + --trailer Thanks-to:Johannes actual -EOF + EOF + test_cmp expected actual +' + +test_expect_success 'with trailer.trimEmpty set to false' ' + git config trailer.trimEmpty false + sed -e s/ Z\$/ / expected -EOF + + review: Z + sign: A U Thor aut...@example.com + Thanks-to: Johannes + EOF + git interpret-trailers --trailer review: \ + --trailer Thanks-to:Johannes actual -EOF + EOF + test_cmp expected actual +' + +test_expect_success 'with trailer.trimEmpty set to false and a message' ' + cat complex_message_body expected + sed -e s/ Z\$/ / expected -\EOF + Fixes: Z + Acked-by= Z + Reviewed-by: Z + Signed-off-by: Z + sign: A U Thor aut...@example.com + Thanks-to: Johannes + EOF + git interpret-trailers --trailer review: \ + --trailer Thanks-to:Johannes complex_message actual + test_cmp expected actual + git interpret-trailers --no-trim-empty --trailer review: \ + --trailer Thanks-to:Johannes complex_message actual + test_cmp expected actual +' + +test_expect_success 'with trailer.trimEmpty set to 1 and a message' ' + git config trailer.trimEmpty 1 + cat complex_message_body expected + sed -e s/ Z\$/ / expected -\EOF + sign: A U Thor aut...@example.com + Thanks-to: Johannes + EOF + git interpret-trailers --trailer review: \ + --trailer Thanks-to:Johannes complex_message actual + test_cmp expected actual +' + +test_expect_success 'with trailer.trimEmpty set to 1 and --no-trim-empty' ' + cat complex_message_body expected + sed -e s/ Z\$/ / expected -\EOF + Fixes: Z + Acked-by= Z + Reviewed-by: Z + Signed-off-by: Z + sign: A U Thor aut...@example.com + Thanks-to: Johannes + EOF + git interpret-trailers --no-trim-empty --trailer review: \ + --trailer Thanks-to:Johannes complex_message actual + test_cmp expected actual +' + test_done -- 2.2.1.313.gcc831f2 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: read() MAX_IO_SIZE bytes, more than SSIZE_MAX?
Joachim Schmitz jojo at schmitz-digital.de writes: Torsten Bögershausen tboegi at web.de writes: On 2015-02-07 17.45, Joachim Schmitz wrote: snip b) never ever should read() be asked to read more than SSIZE_MAX, this should be true for every platform on the planet? You may want to have is smaller than SSIZE_MAX (like the current 8MB vs. the possible 2TB on Linux), but surely never larger? Se also $gmane/232469, where that issue cropped up for MacOS X 64bit? Bye, Jojo
RE: read() MAX_IO_SIZE bytes, more than SSIZE_MAX?
On 2015-02-07 12:30PM Torsten Bögershausen wrote: On 2015-02-07 17.45, Joachim Schmitz wrote: Hi there While investigating the problem with hung git-upload-pack we think to have found a bug in wrapper.c: #define MAX_IO_SIZE (8*1024*1024) This is then used in xread() to split read()s into suitable chunks. So far so good, but read() is only guaranteed to read as much as SSIZE_MAX bytes at a time. And on our platform that is way lower than those 8MB (only 52kB, POSIX allows it to be as small as 32k), and as a (rather strange) consequence mmap() (from compat/mmap.c) fails with EACCESS (why EACCESS?), because xpread() returns something 0. How large is SSIZE_MAX on other platforms? What happens there if you try to read() more? Should't we rather use SSIZE_MAX on all platforms? If I'm reading the header files right, on Linux it is LONG_MAX (2TB?), so I guess we should really go for MIN(8*1024*1024,SSIZE_MAX)? How about changing wrapper.c like this: #ifndef MAX_IO_SIZE #define MAX_IO_SIZE (8*1024*1024) #endif - and to change config.mak.uname like this: ifeq ($(uname_S),NONSTOP_KERNEL) BASIC_CFLAGS += -DMAX_IO_SIZE=(32*1024) Does this work for you ? Yes, thank you Torsten. I have made this change in our branch (on behalf of Jojo). I think we can accept it. The (32*1024) does need to be properly quoted, however. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: read() MAX_IO_SIZE bytes, more than SSIZE_MAX?
On 2015-02-07 13:07PM Randall S. Becker wrote: On 2015-02-07 12:30PM Torsten Bögershausen wrote: On 2015-02-07 17.45, Joachim Schmitz wrote: Hi there While investigating the problem with hung git-upload-pack we think to have found a bug in wrapper.c: #define MAX_IO_SIZE (8*1024*1024) This is then used in xread() to split read()s into suitable chunks. So far so good, but read() is only guaranteed to read as much as SSIZE_MAX bytes at a time. And on our platform that is way lower than those 8MB (only 52kB, POSIX allows it to be as small as 32k), and as a (rather strange) consequence mmap() (from compat/mmap.c) fails with EACCESS (why EACCESS?), because xpread() returns something 0. How large is SSIZE_MAX on other platforms? What happens there if you try to read() more? Should't we rather use SSIZE_MAX on all platforms? If I'm reading the header files right, on Linux it is LONG_MAX (2TB?), so I guess we should really go for MIN(8*1024*1024,SSIZE_MAX)? How about changing wrapper.c like this: #ifndef MAX_IO_SIZE #define MAX_IO_SIZE (8*1024*1024) #endif - Although I do agree with Jojo, that MAX_IO_SIZE seems to be a platform constant and should be defined in terms of SSIZE_MAX. So something like: #ifndef MAX_IO_SIZE # ifdef SSIZE_MAX # define MAX_IO_SIZE (SSIZE_MAX) # else # define MAX_IO_SIZE (8*1024*1024) # endif #endif would be desirable. Cheers, Randall -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: read() MAX_IO_SIZE bytes, more than SSIZE_MAX?
Randall S. Becker rsbecker at nexbridge.com writes: On 2015-02-07 13:07PM Randall S. Becker wrote: On 2015-02-07 12:30PM Torsten Bögershausen wrote: On 2015-02-07 17.45, Joachim Schmitz wrote: spip Although I do agree with Jojo, that MAX_IO_SIZE seems to be a platform constant and should be defined in terms of SSIZE_MAX. So something like: #ifndef MAX_IO_SIZE # ifdef SSIZE_MAX # define MAX_IO_SIZE (SSIZE_MAX) # else # define MAX_IO_SIZE (8*1024*1024) # endif #endif would be desirable. It would be way too large on some platforms. those 8MB had been chosen for a good reason, I assume: /* * Limit size of IO chunks, because huge chunks only cause pain. OS X * 64-bit is buggy, returning EINVAL if len = INT_MAX; and even in * the absence of bugs, large chunks can result in bad latencies when * you decide to kill the process. */ However it should never be larger than SSIZE_MAX
read() MAX_IO_SIZE bytes, more than SSIZE_MAX?
Hi there While investigating the problem with hung git-upload-pack we think to have found a bug in wrapper.c: #define MAX_IO_SIZE (8*1024*1024) This is then used in xread() to split read()s into suitable chunks. So far so good, but read() is only guaranteed to read as much as SSIZE_MAX bytes at a time. And on our platform that is way lower than those 8MB (only 52kB, POSIX allows it to be as small as 32k), and as a (rather strange) consequence mmap() (from compat/mmap.c) fails with EACCESS (why EACCESS?), because xpread() returns something 0. How large is SSIZE_MAX on other platforms? What happens there if you try to read() more? Should't we rather use SSIZE_MAX on all platforms? If I'm reading the header files right, on Linux it is LONG_MAX (2TB?), so I guess we should really go for MIN(8*1024*1024,SSIZE_MAX)? bye, Jojo -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: read() MAX_IO_SIZE bytes, more than SSIZE_MAX?
Joachim Schmitz jojo at schmitz-digital.de writes: because xpread() returns something 0. something 0 of course (presumably -1)... bye, Jojo -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: hang in git-upload-pack
-Original Message- Sent: February 7, 2015 11:26 AM In HP-Nonstop we're experiencing hangs in git-upload-pack, which seems to be the result of reads from / writes to pipes don't ever finish or don't get interrupted properly (SIGPIPE, SIGCHLD?) Any idea why that might be and how to fix it? More context on this issue: This is a new port of Git 2.3 to HP NonStop OSS (POSIX-ish). With very minimal changes in git_compat-util.h to include floss and wrapper.c (below), we are able to clone remote repositories and work on local repositories without issue. However, when attempting to fetch from a local bare repository (set up as a remote but on the same server) into a working repository, or when a remote client attempts to clone from any repository on the server over any protocol, we end up with git-upload-pack hanging as the common point of failure. Note that this function has not worked in prior version of git, so we have no working reference to compare. The team is suspecting differences in how the OS deals with pipes but our primary need from the community is some guidance on continuing our investigation in resolving this. Most git tests succeed except for: t0025(test 2), t0301(test 12-expected),t5507(test 4 - suspicious of this),t9001(expected). A sample trace showing the issue is below. There are no external clients involved in this sample. This is git to git locally. The condition appears to be representative of all of the hangs. GIT_TRACE=1 /usr/local/bin/git --exec-path=/usr/local/bin fetch old_floss_tail 09:52:01.198401 trace: built-in: git 'fetch' 'old_floss_tail' 09:52:01.226684 trace: run_command: 'git-upload-pack '\''/home/git/floss.git/.git'\''' 09:52:01.229229 trace: exec: '/usr/local/bin/bash' '-c' 'git-upload-pack '\''/home/git/floss.git/.git'\''' 'git-upload-pack '\''/home/git/floss.git/.git'\''' 09:52:01.303638 trace: run_command: 'rev-list' '--objects' '--stdin' '--not' '--all' '--quiet' warning: no common commits 09:52:01.438320 trace: run_command: 'pack-objects' '--revs' '--thin' '--stdout' '--progress' '--delta-base-offset' '--include-tag' remote: 09:52:01.445274 trace: exec: 'git' 'pack-objects' '--revs' '--thin' '--stdout' '--progress' '--delta-base-offset' '--include-tag' remote: 09:52:01.463846 trace: built-in: git 'pack-objects' '--revs' '--thin' '--stdout' '--progress' '--delta-base-offset' '--include-tag' remote: Counting objects: 485, done. remote: Compressing objects: 100% (472/472), done. Hangs forever at this point. The git-unpack-load is stopped at (not that the addresses might mean much): xread + 0x130 (UCr) create_pack_file + 0x18F0 (UCr) upload_pack + 0x450 (UCr) . There are two git processes at: xread + 0x130 (UCr) read_in_full + 0x130 (UCr) get_packet_data + 0x4A0 (UCr) . And one git is at: xwrite + 0x130 (UCr) flush + 0x530 (UCr) sha1write + 0x600 (UCr) write_no_reuse_object + 0x1390 (UCr) . Wrapper.c change: @@ -173,7 +173,12 @@ void *xcalloc(size_t nmemb, size_t size) * the absence of bugs, large chunks can result in bad latencies when * you decide to kill the process. */ -#define MAX_IO_SIZE (8*1024*1024) +#ifdef __TANDEM +# include limits.h /* SSIZE_MAX == 52k */ +# define MAX_IO_SIZE SSIZE_MAX +#else +# define MAX_IO_SIZE (8*1024*1024) +#endif Best Regards, Randall -- Brief whoami: NonStopUNIX developer since approximately UNIX(421664400)/NonStop(2112884442) -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: read() MAX_IO_SIZE bytes, more than SSIZE_MAX?
On 2015-02-07 17.45, Joachim Schmitz wrote: Hi there While investigating the problem with hung git-upload-pack we think to have found a bug in wrapper.c: #define MAX_IO_SIZE (8*1024*1024) This is then used in xread() to split read()s into suitable chunks. So far so good, but read() is only guaranteed to read as much as SSIZE_MAX bytes at a time. And on our platform that is way lower than those 8MB (only 52kB, POSIX allows it to be as small as 32k), and as a (rather strange) consequence mmap() (from compat/mmap.c) fails with EACCESS (why EACCESS?), because xpread() returns something 0. How large is SSIZE_MAX on other platforms? What happens there if you try to read() more? Should't we rather use SSIZE_MAX on all platforms? If I'm reading the header files right, on Linux it is LONG_MAX (2TB?), so I guess we should really go for MIN(8*1024*1024,SSIZE_MAX)? How about changing wrapper.c like this: #ifndef MAX_IO_SIZE #define MAX_IO_SIZE (8*1024*1024) #endif - and to change config.mak.uname like this: ifeq ($(uname_S),NONSTOP_KERNEL) BASIC_CFLAGS += -DMAX_IO_SIZE=(32*1024) Does this work for you ? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
hang in git-upload-pack
Hi there In HP-Nonstop we're experiencing hangs in git-upload-pack, which seems to be the result of reads from / writes to pipes don't ever finish or don't get interrupted properly (SIGPIPE, SIGCHLD?) Any idea why that might be and how to fix it? bye, Jojo -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: read() MAX_IO_SIZE bytes, more than SSIZE_MAX?
Torsten Bögershausen tboegi at web.de writes: On 2015-02-07 17.45, Joachim Schmitz wrote: snip How about changing wrapper.c like this: #ifndef MAX_IO_SIZE #define MAX_IO_SIZE (8*1024*1024) #endif - and to change config.mak.uname like this: ifeq ($(uname_S),NONSTOP_KERNEL) BASIC_CFLAGS += -DMAX_IO_SIZE=(32*1024) Does this work for you ? Of course it would, but, a) 32k is smaller than we can go (and yes, we could make it 52k) b) never ever should read() be asked to read more than SSIZE_MAX, this should be true for every platform on the planet? You may want to have is smaller than SSIZE_MAX (like the current 8MB vs. the possible 2TB on Linux), but surely never larger? Bye, Jojo
Re: read() MAX_IO_SIZE bytes, more than SSIZE_MAX?
Joachim Schmitz jojo at schmitz-digital.de writes: snip and as a (rather strange) consequence mmap() (from compat/mmap.c) fails with EACCESS (why EACCESS?), because xpread() returns something 0. Seems mmap() should either set errno to EINVAL or not set it at all an just 'forward' whatever xpread() has set. As per http://man7.org/linux/man-pages/man2/mmap.2.html mmap() sets EINVAL if (amongst other things) it doesn't like the value of len, exactly the case here. bye, Jojo -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] trailer: add a trailer.trimEmpty config option
Christian Couder chrisc...@tuxfamily.org writes:
This way people who always want trimed trailers
don't need to specify it on the command line.
I sense that print_all() that cares about trimming illustrate a
design mistake in the original code structure. Why isn't the entire
program structured like this instead?
- read input and split that into three parts:
struct {
struct strbuf messsage_proper;
struct trailers {
int nr, alloc;
struct strbuf *array_of_trailers[];
} trailers;
struct strbuf lines_after_trailers;
};
- do trailer stuff by calling a central helper that does
trailer stuff a pointer to the middle, trailers, struct.
- when the trailer becomes a reusable subsystem, this central
helper will become the primary entry point to the API.
- trailer stuff will include adding new ones, removing
existing ones, and rewriting lines. What you do in the
current process_command_line_args() and
process_trailers_lists() [*1*] would come here.
- write out the result, given the outermost struct. This will
become another entry point in the API.
Structured that way, callers will supply that outermost structure,
and can trust that the trailers subsystem will not molest
message_proper or lines_after_trailers part. They can even process
the parts that the trailer subsystem would not touch, e.g. running
stripspace to the text in message_proper.
Viewed that way, it would be clear that strip empty ones should be
a new feature in the trailer stuff, not just filter out only
during the output phase. Having it in the output phase does not
feel that the labor/responsibility is split in the right way.
Another problem I have with filter out during the output phase
comes from the semantics/correctness in the resulting code, and I
suspect that it would need to be done a lot earlier, before you
allow the logic such as if I have X, do this, but if there is no X,
do this other thing. If you have an X that is empty in the input,
trimming that before such logic kicks in and trimming that in the
final output phase would give different results, and I think the
latter would give a less intuitive result.
As this is the second time I have to point out that the data
structure used by the current code to hold the trailers and other
stuff to work on smells fishy, I would seriously consider cleaning
up the existing code to make it easier to allow us to later create
a reusable API and trailer subsystem out of it, before piling new
features on top of it, if I were you.
Signed-off-by: Christian Couder chrisc...@tuxfamily.org
---
builtin/interpret-trailers.c | 2 +-
trailer.c| 13 ++---
trailer.h| 2 +-
3 files changed, 12 insertions(+), 5 deletions(-)
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index 46838d2..1adf814 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -18,7 +18,7 @@ static const char * const git_interpret_trailers_usage[] = {
int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
{
- int trim_empty = 0;
+ int trim_empty = -1;
struct string_list trailers = STRING_LIST_INIT_DUP;
struct option options[] = {
diff --git a/trailer.c b/trailer.c
index 623adeb..7614182 100644
--- a/trailer.c
+++ b/trailer.c
@@ -36,6 +36,8 @@ static struct trailer_item *first_conf_item;
static char *separators = :;
+static int trim_empty;
+
#define TRAILER_ARG_STRING $ARG
static int after_or_end(enum action_where where)
@@ -120,7 +122,7 @@ static void print_tok_val(const char *tok, const char
*val)
printf(%s%c %s\n, tok, separators[0], val);
}
-static void print_all(struct trailer_item *first, int trim_empty)
+static void print_all(struct trailer_item *first)
{
struct trailer_item *item;
for (item = first; item; item = item-next) {
@@ -509,6 +511,8 @@ static int git_trailer_default_config(const char
*conf_key, const char *value, v
value, conf_key);
} else if (!strcmp(trailer_item, separators)) {
separators = xstrdup(value);
+ } else if (!strcmp(trailer_item, trimempty)) {
+ trim_empty = git_config_bool(conf_key, value);
}
}
return 0;
@@ -842,7 +846,7 @@ static void free_all(struct trailer_item **first)
}
}
-void process_trailers(const char *file, int trim_empty, struct string_list
*trailers)
+void process_trailers(const char *file, int trim, struct string_list
*trailers)
{
struct trailer_item *in_tok_first = NULL;
struct trailer_item *in_tok_last = NULL;
@@ -854,6 +858,9 @@ void process_trailers(const char *file, int trim_empty,
Re: read() MAX_IO_SIZE bytes, more than SSIZE_MAX?
On 2015-02-07 18.29, Joachim Schmitz wrote: Torsten Bögershausen tboegi at web.de writes: On 2015-02-07 17.45, Joachim Schmitz wrote: snip How about changing wrapper.c like this: #ifndef MAX_IO_SIZE #define MAX_IO_SIZE (8*1024*1024) #endif - and to change config.mak.uname like this: ifeq ($(uname_S),NONSTOP_KERNEL) BASIC_CFLAGS += -DMAX_IO_SIZE=(32*1024) Does this work for you ? Of course it would, but, a) 32k is smaller than we can go (and yes, we could make it 52k) Sorry, I missed that: (52*1024) b) never ever should read() be asked to read more than SSIZE_MAX, this should be true for every platform on the planet? You may want to have is smaller than SSIZE_MAX (like the current 8MB vs. the possible 2TB on Linux), but surely never larger? Good question. I don't know every platform of the planet well enough to be helpful here, especially the ones which don't follow all the specifications. In other words: As long as we can not guarantee that SSIZE_MAX is defined, (and is defined to somethong useful for xread()/xwrite() ) we should be more defensive here: tweak only on platform where we know it is needed and we know that it works. And leave the other ones alone, until someone finds another platform which needs the same or another tweak and sends a tested patch. Thanks for the report, do you want to send a patch to the list ? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: read() MAX_IO_SIZE bytes, more than SSIZE_MAX?
Junio C Hamano gitster at pobox.com writes:
On Sat, Feb 7, 2015 at 12:32 PM, Torsten Bögershausen tboegi at
web.de wrote:
I don't know every platform of the planet well enough to be helpful
here,
especially the ones which don't follow all the specifications.
In other words: As long as we can not guarantee that SSIZE_MAX is
defined,
(and is defined to somethong useful for xread()/xwrite() )
we should be more defensive here:
tweak only on platform where we know it is needed and we know that it
works.
Yup, I agree that is a sensible way to go.
(1) if Makefile overrides the size, use it; otherwise
(2) if SSIZE_MAX is defined, and it is smaller than our internal
default, use it; otherwise
(3) use our internal default.
And leave our internal default to 8MB.
That way, nobody needs to do anything differently from his current build
set-up,
and I suspect that it would make step (1) optional.
something like this:
/* allow overwriting from e.g. Makefile */
#if !defined(MAX_IO_SIZE)
# define MAX_IO_SIZE (8*1024*1024)
#endif
/* for plattforms that have SSIZE and have it smaller */
#if defined(SSIZE_MAX (SSIZE_MAX MAX_IO_SIZE)
# undef MAX_IO_SIZE /* avoid warning */
# define MAX_IO_SIZE SSIZE_MAX
#endif
Steps 2 and 3 only , indeed step 1 not needed...
Bye,
JojoN�r��yb�X��ǧv�^�){.n�+ا���ܨ}���Ơz�j:+v���zZ+��+zf���h���~i���z� �w���?��)ߢf
Re: 'git rebase' silently drops changes?
On 06.02.2015 22:28, Sergey Organov wrote: # Now rebase my work. git rebase -f HEAD~1 # What? Where is my Precious change in a??? cat a /SCRIPT I.e., the modification marked [!] was silently lost during rebase! Just a wild guess: Maybe because you omitted -p / --preserve-merges from git rebase? -- Sebastian Schuberth -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: read() MAX_IO_SIZE bytes, more than SSIZE_MAX?
On Feb 7 2015 at 9:14 PM Junio C Hamano wrote: On Sat, Feb 7, 2015 at 2:31 PM, Joachim Schmitz j...@schmitz-digital.de wrote: Junio C Hamano gitster at pobox.com writes: Yup, I agree that is a sensible way to go. (1) if Makefile overrides the size, use it; otherwise (2) if SSIZE_MAX is defined, and it is smaller than our internal default, use it; otherwise (3) use our internal default. And leave our internal default to 8MB. That way, nobody needs to do anything differently from his current build set-up, and I suspect that it would make step (1) optional. something like this: /* allow overwriting from e.g. Makefile */ #if !defined(MAX_IO_SIZE) # define MAX_IO_SIZE (8*1024*1024) #endif /* for plattforms that have SSIZE and have it smaller */ #if defined(SSIZE_MAX (SSIZE_MAX MAX_IO_SIZE) # undef MAX_IO_SIZE /* avoid warning */ # define MAX_IO_SIZE SSIZE_MAX #endif No, not like that. If you do (1), that is only so that the Makefile can override a broken definition a platform may give to SSIZE_MAX. So (1) if Makefile gives one, use it without second-guessing with SSIZE_MAX. (2) if SSIZE_MAX is defined, and if it is smaller than our internal default, use it. (3) all other cases, us our internal default. That is reasonable. I am more concerned about our git-upload-pak (separate thread) anyway :) Cheers, Randall -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: read() MAX_IO_SIZE bytes, more than SSIZE_MAX?
On Sat, Feb 7, 2015 at 12:32 PM, Torsten Bögershausen tbo...@web.de wrote: I don't know every platform of the planet well enough to be helpful here, especially the ones which don't follow all the specifications. In other words: As long as we can not guarantee that SSIZE_MAX is defined, (and is defined to somethong useful for xread()/xwrite() ) we should be more defensive here: tweak only on platform where we know it is needed and we know that it works. Yup, I agree that is a sensible way to go. (1) if Makefile overrides the size, use it; otherwise (2) if SSIZE_MAX is defined, and it is smaller than our internal default, use it; otherwise (3) use our internal default. And leave our internal default to 8MB. That way, nobody needs to do anything differently from his current build set-up, and I suspect that it would make step (1) optional. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] trailer: add a trailer.trimEmpty config option
On Sat, Feb 7, 2015 at 9:20 PM, Junio C Hamano gits...@pobox.com wrote:
Christian Couder chrisc...@tuxfamily.org writes:
This way people who always want trimed trailers
don't need to specify it on the command line.
I sense that print_all() that cares about trimming illustrate a
design mistake in the original code structure. Why isn't the entire
program structured like this instead?
- read input and split that into three parts:
struct {
struct strbuf messsage_proper;
struct trailers {
int nr, alloc;
struct strbuf *array_of_trailers[];
} trailers;
struct strbuf lines_after_trailers;
};
It is not designed like this because you only asked me to design it
like this after the facts, when there was another email thread about
conflicts blocks and one function you created could be used by the
trailer code too.
If you had asked this from the beginning I would certainly have done
it more like this (though I think the struct trailers part is too
simplistic). Rearchitecturing the code now would bring only small
performance improvements and a lot of code churn. And the performance
is not needed anyway if the code is not used in the first place, so
I'd rather first make sure that the code can be used.
I think that very few new features are now needed to make it possible
to use the code in other commands like commit, format-patch, am, etc,
but this patch implements one of the needed features.
- do trailer stuff by calling a central helper that does
trailer stuff a pointer to the middle, trailers, struct.
- when the trailer becomes a reusable subsystem, this central
helper will become the primary entry point to the API.
- trailer stuff will include adding new ones, removing
existing ones, and rewriting lines. What you do in the
current process_command_line_args() and
process_trailers_lists() [*1*] would come here.
- write out the result, given the outermost struct. This will
become another entry point in the API.
Structured that way, callers will supply that outermost structure,
and can trust that the trailers subsystem will not molest
message_proper or lines_after_trailers part.
I don't think it is a big improvement because it is easy to see that
the current code doesn't molest the part before and after the
trailers.
They can even process
the parts that the trailer subsystem would not touch, e.g. running
stripspace to the text in message_proper.
That could be worth the rearchitecturing if people really wanted that,
but I think for now more people have been interested in having ways to
change the trailer part, so I prefer to work on this first.
Viewed that way, it would be clear that strip empty ones should be
a new feature in the trailer stuff, not just filter out only
during the output phase. Having it in the output phase does not
feel that the labor/responsibility is split in the right way.
My opinion is that having it in the output phase is best.
Another problem I have with filter out during the output phase
comes from the semantics/correctness in the resulting code, and I
suspect that it would need to be done a lot earlier, before you
allow the logic such as if I have X, do this, but if there is no X,
do this other thing. If you have an X that is empty in the input,
trimming that before such logic kicks in and trimming that in the
final output phase would give different results, and I think the
latter would give a less intuitive result.
I think it is much better in the output phase because it is very
natural for some projects to have a template message with empty
trailers like this:
Signed-off-by:
Reviewed-by:
Such a template message can for example remind contributors to the
project that they need to sign off their work and that they need to
have it reviewed by at least one person, and that to make it simpler
for everyone to review patches, the Signed-off-by trailers should
come before the Reviewed-by trailers in the commit message.
In this case, if you trim before you process command line trailers,
then, when you process some command line trailers that add sign offs,
the meaning of where=after cannot be based any more on the existing
empty Signed-off-by: in the template message.
As this is the second time I have to point out that the data
structure used by the current code to hold the trailers and other
stuff to work on smells fishy, I would seriously consider cleaning
up the existing code to make it easier to allow us to later create
a reusable API and trailer subsystem out of it, before piling new
features on top of it, if I were you.
I am not so sure that it would make it easier to allow us to later
create a reusable API and trailer subsystem out of it.
It is very difficult to predict what will be really needed in the
future and it is not
Re: read() MAX_IO_SIZE bytes, more than SSIZE_MAX?
On Sat, Feb 7, 2015 at 2:31 PM, Joachim Schmitz j...@schmitz-digital.de wrote: Junio C Hamano gitster at pobox.com writes: Yup, I agree that is a sensible way to go. (1) if Makefile overrides the size, use it; otherwise (2) if SSIZE_MAX is defined, and it is smaller than our internal default, use it; otherwise (3) use our internal default. And leave our internal default to 8MB. That way, nobody needs to do anything differently from his current build set-up, and I suspect that it would make step (1) optional. something like this: /* allow overwriting from e.g. Makefile */ #if !defined(MAX_IO_SIZE) # define MAX_IO_SIZE (8*1024*1024) #endif /* for plattforms that have SSIZE and have it smaller */ #if defined(SSIZE_MAX (SSIZE_MAX MAX_IO_SIZE) # undef MAX_IO_SIZE /* avoid warning */ # define MAX_IO_SIZE SSIZE_MAX #endif No, not like that. If you do (1), that is only so that the Makefile can override a broken definition a platform may give to SSIZE_MAX. So (1) if Makefile gives one, use it without second-guessing with SSIZE_MAX. (2) if SSIZE_MAX is defined, and if it is smaller than our internal default, use it. (3) all other cases, us our internal default. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
