* Re: [PATCH v3 1/2] cache-tree: invalidate i-t-a paths after generating trees
From: Nguyen Thai Ngoc Duy @ 2012-12-15 2:52 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King, Jonathon Mah
In-Reply-To: <7v4njq1ze7.fsf@alter.siamese.dyndns.org>
On Thu, Dec 13, 2012 at 9:04 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> @@ -324,7 +325,14 @@ static int update_one(struct cache_tree *it,
>> if (!sub)
>> die("cache-tree.c: '%.*s' in '%s' not found",
>> entlen, path + baselen, path);
>> - i += sub->cache_tree->entry_count - 1;
>> + i--; /* this entry is already counted in "sub" */
>
> Huh?
>
> The "-1" in the original is the bog-standard compensation for the
> for(;;i++) loop.
Exactly. It took me a while to figure out what " - 1" was for and I
wanted to avoid that for other developers. Only I worded it badly.
I'll replace the for loop with a while loop to make it clearer...
>
>> + if (sub->cache_tree->entry_count < 0) {
>> + i -= sub->cache_tree->entry_count;
>> + sub->cache_tree->entry_count = -1;
>> + to_invalidate = 1;
>> + }
>> + else
>> + i += sub->cache_tree->entry_count;
>
> While the rewritten version is not *wrong* per-se, I have a feeling
> that it may be much easier to read if written like this:
>
> if (sub->cache_tree_entry_count < 0) {
> to_invalidate = 1;
> to_skip = 0 - sub->cache_tree_entry_count;
> sub->cache_tree_entry_count = -1;
> } else {
> to_skip = sub->cache_tree_entry_count;
> }
> i += to_skip - 1;
>
..or this would be fine too. Which way to go?
A while we're still at the cache tree
> - if (ce->ce_flags & (CE_REMOVE | CE_INTENT_TO_ADD))
> - continue; /* entry being removed or placeholder */
> + /*
> + * CE_REMOVE entries are removed before the index is
> + * written to disk. Skip them to remain consistent
> + * with the future on-disk index.
> + */
> + if (ce->ce_flags & CE_REMOVE)
> + continue;
> +
A CE_REMOVE entry is removed later from the index, however it is still
counted in entry_count. entry_count serves two purposes: to skip the
number of processed entries in the in-memory index, and to record the
number of entries in the on-disk index. These numbers do not match
when CE_REMOVE is present. We have correct in-memory entry_count,
which means incorrect on-disk entry_count in this case.
I tested with an index that has a/b and a/c. The latter has CE_REMOVE.
After writing cache tree I get:
$ git ls-files --stage
100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0 a/b
$ ../test-dump-cache-tree
878e27c626266ac04087a203e4bdd396dcf74763 (2 entries, 1 subtrees)
878e27c626266ac04087a203e4bdd396dcf74763 #(ref) (1 entries, 1 subtrees)
4277b6e69d25e5efa77c455340557b384a4c018a a/ (2 entries, 0 subtrees)
4277b6e69d25e5efa77c455340557b384a4c018a #(ref) a/ (1 entries, 0 subtrees)
If I throw out that index, create a new one with a/b alone and write-tree, I get
$ ../test-dump-cache-tree
878e27c626266ac04087a203e4bdd396dcf74763 (1 entries, 1 subtrees)
4277b6e69d25e5efa77c455340557b384a4c018a a/ (1 entries, 0 subtrees)
Shall we fix this too? I'm thinking of adding "skip" argument to update_one and
i += sub->cache_tree->entry_count - 1;
would become
i += sub->cache_tree->entry_count + skip - 1;
and entry_count would always reflect on-disk value. This "skip" can be
reused for this i-t-a patch as well.
--
Duy
^ permalink raw reply
* Re: [PATCH 1/4] Support builds when sys/param.h is missing
From: David Michael @ 2012-12-15 2:46 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git@vger.kernel.org
In-Reply-To: <7vpq2curpa.fsf@alter.siamese.dyndns.org>
Hi,
On Fri, Dec 14, 2012 at 6:41 PM, Junio C Hamano <gitster@pobox.com> wrote:
> I have this suspicion that nobody would notice if we simply stopped
> including the header.
While I'm not aware of any subtleties it could be causing on other
platforms, it does seem fine to drop sys/param.h on my test GNU/Linux
systems.
I can resend the series and just remove the include, if preferred.
Thanks.
David
^ permalink raw reply
* Re: [PATCH 1/4] Support builds when sys/param.h is missing
From: Junio C Hamano @ 2012-12-14 23:41 UTC (permalink / raw)
To: David Michael; +Cc: git@vger.kernel.org
In-Reply-To: <CAEvUa7mHHy1=aOMfg4hNnDzod4zSnNHZN87isgf6Yxh5cRAf0Q@mail.gmail.com>
David Michael <fedora.dm0@gmail.com> writes:
> An option is added to the Makefile to skip the inclusion of sys/param.h.
> The only known platform with this condition thus far is the z/OS UNIX System
> Services environment.
>
> Signed-off-by: David Michael <fedora.dm0@gmail.com>
> ---
Hmm, makes me wonder if we can remove that inclusion everywhere
instead. What definitions are we getting from it?
8695c8b (Add "show-files" command to show the list of managed (or
non-managed) files., 2005-04-11) added it to show-files, and without
the include the compilation failed due to lack of MAXPATHLEN, but
you shouldn't be using that in the first place (we tend to either
use PATH_MAX or lift the upper limit using strbuf these days.).
65bb491 (Add the ability to prefix something to the pathname to
"checkout-cache.c", 2005-04-21) does the same to checkout-cache.c
to grab MAXPATHLEN.
bb233d6 (Add support for a "GIT_INDEX_FILE" environment variable.,
2005-04-21) moves these two inclusions to cache.h; removing it
bteaks compilation due to lack of MAXPATHLEN, ULONG_MAX, etc.,
I didn't check what else sys/parms.h was accicdentally slurping into
the compilation, but ULONG_MAX is what we explicitly ask for by
including either inttypes or stdint these days.
Later 4050c0d (Clean up compatibility definitions., 2005-12-05)
further consolidated the headers by moving inclusion around. This
commit did not audit unused headers.
I have this suspicion that nobody would notice if we simply stopped
including the header.
^ permalink raw reply
* Re: [PATCH 4/4] Declare that HP NonStop systems require strings.h
From: Junio C Hamano @ 2012-12-14 23:17 UTC (permalink / raw)
To: Johannes Sixt; +Cc: David Michael, git@vger.kernel.org
In-Reply-To: <50CB8C80.6040802@kdbg.org>
Johannes Sixt <j6t@kdbg.org> writes:
> Am 14.12.2012 20:57, schrieb David Michael:
>> This platform previously included strings.h automatically. However, the
>> build system now requires an explicit option to do so.
>>
>> Signed-off-by: David Michael <fedora.dm0@gmail.com>
>> ---
>> Makefile | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/Makefile b/Makefile
>> index fb78f7f..e84b0cb 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -1357,6 +1357,7 @@ ifeq ($(uname_S),NONSTOP_KERNEL)
>> # Added manually, see above.
>> NEEDS_SSL_WITH_CURL = YesPlease
>> HAVE_LIBCHARSET_H = YesPlease
>> + HAVE_STRINGS_H = YesPlease
>> NEEDS_LIBICONV = YesPlease
>> NEEDS_LIBINTL_BEFORE_LIBICONV = YesPlease
>> NO_SYS_SELECT_H = UnfortunatelyYes
>
> If NONSTOP_KERNEL is the platform that defines __TANDEM, then this
> should be squashed into the previous patch, shouldn't it?
Correct; otherwise 3/4 would break build on that platform.
^ permalink raw reply
* Re: [PATCH 4/4] Declare that HP NonStop systems require strings.h
From: Johannes Sixt @ 2012-12-14 23:16 UTC (permalink / raw)
To: Joachim Schmitz; +Cc: gitster, fedora.dm0, Git Mailing List
In-Reply-To: <00c601cdda4c$eb6be9a0$c243bce0$@schmitz-digital.de>
Am 14.12.2012 23:46, schrieb Joachim Schmitz:
> Johannes Sixt wrote:
>> Am 14.12.2012 20:57, schrieb David Michael:
>>> This platform previously included strings.h automatically. However,
>>> the build system now requires an explicit option to do so.
>>>
>>> Signed-off-by: David Michael <fedora.dm0@gmail.com>
>>> ---
>>> Makefile | 1 +
>>> 1 file changed, 1 insertion(+)
>>>
>>> diff --git a/Makefile b/Makefile
>>> index fb78f7f..e84b0cb 100644
>>> --- a/Makefile
>>> +++ b/Makefile
>>> @@ -1357,6 +1357,7 @@ ifeq ($(uname_S),NONSTOP_KERNEL)
>>> # Added manually, see above.
>>> NEEDS_SSL_WITH_CURL = YesPlease
>>> HAVE_LIBCHARSET_H = YesPlease
>>> + HAVE_STRINGS_H = YesPlease
>>> NEEDS_LIBICONV = YesPlease
>>> NEEDS_LIBINTL_BEFORE_LIBICONV = YesPlease
>>> NO_SYS_SELECT_H = UnfortunatelyYes
>>
>> If NONSTOP_KERNEL is the platform that defines __TANDEM, then this
>> should be squashed into the previous patch, shouldn't it?
>
> Patch 4/4 does not work without 3/4, Not for HP-NonStop.
That is clear from the order of the patches in the series.
To put it in a different way: Do all supported platforms still work
after 3/4, but without 4/4?
-- Hannes
^ permalink raw reply
* Re: possible Improving diff algoritm
From: Bernhard R. Link @ 2012-12-14 22:29 UTC (permalink / raw)
To: Javier Domingo; +Cc: Junio C Hamano, Geert Bosch, Morten Welinder, Kevin, git
In-Reply-To: <CALZVap=r0toqWT7aJxiKtezmR8s4QDd0x92JX-eBLWhKaJsmOw@mail.gmail.com>
* Javier Domingo <javierdo1@gmail.com> [121214 13:20]:
> I think the idea of being preferable to have a blank line at the end
> of the added/deleted block is key in this case.
For symmetry I'd suggest to make it preferable to have blank lines
at the end or the beginning.
{
old
+ }
+
+ {
+ new
}
vs
{
old
}
+
+ {
+ new
+ }
is just the same case in blue.
(Although empty lines alone feels not quite optimal, it is at least a
good start).
Bernhard R. Link
^ permalink raw reply
* Re: [PATCH 4/4] Declare that HP NonStop systems require strings.h
From: Joachim Schmitz @ 2012-12-14 22:45 UTC (permalink / raw)
To: git
In-Reply-To: <50CB8C80.6040802@kdbg.org>
Johannes Sixt wrote:
> Am 14.12.2012 20:57, schrieb David Michael:
>> This platform previously included strings.h automatically. However,
>> the build system now requires an explicit option to do so.
>>
>> Signed-off-by: David Michael <fedora.dm0@gmail.com>
>> ---
>> Makefile | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/Makefile b/Makefile
>> index fb78f7f..e84b0cb 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -1357,6 +1357,7 @@ ifeq ($(uname_S),NONSTOP_KERNEL)
>> # Added manually, see above.
>> NEEDS_SSL_WITH_CURL = YesPlease
>> HAVE_LIBCHARSET_H = YesPlease
>> + HAVE_STRINGS_H = YesPlease
>> NEEDS_LIBICONV = YesPlease
>> NEEDS_LIBINTL_BEFORE_LIBICONV = YesPlease
>> NO_SYS_SELECT_H = UnfortunatelyYes
>
> If NONSTOP_KERNEL is the platform that defines __TANDEM, then this
> should be squashed into the previous patch, shouldn't it?
Patch 4/4 does not work without 3/4, Not for HP-NonStop.
Bye, Jojo
^ permalink raw reply
* [PATCH/RFC 3/3] silence some -Wuninitialized warnings around errors
From: Jeff King @ 2012-12-14 22:13 UTC (permalink / raw)
To: git
In-Reply-To: <20121214220903.GA18418@sigill.intra.peff.net>
When git is compiled with "gcc -Wuninitialized -O3", some
inlined calls provide an additional opportunity for the
compiler to do static analysis on variable initialization.
For example, with two functions like this:
int get_foo(int *foo)
{
if (something_that_might_fail() < 0)
return error("unable to get foo");
*foo = 0;
return 0;
}
void some_fun(void)
{
int foo;
if (get_foo(&foo) < 0)
return -1;
printf("foo is %d\n", foo);
}
If get_foo() is not inlined, then when compiling some_fun,
gcc sees only that a pointer to the local variable is
passed, and must assume that it is an out parameter that
is initialized after get_foo returns.
However, when get_foo() is inlined, the compiler may look at
all of the code together and see that some code paths in
get_foo() do not initialize the variable. As a result, it
prints a warning. But what the compiler can't see is that
error() always returns -1, and therefore we know that either
we return early from some_fun, or foo ends up initialized,
and the code is safe. The warning is a false positive.
By converting the error() call to:
error("unable to get foo");
return -1;
we explicitly make the compiler aware of the constant return
value, and it silences the warning.
Signed-off-by: Jeff King <peff@peff.net>
---
Not sure if we should do this or not. This does silence the warnings,
but it's kind of ugly.
builtin/reflog.c | 6 ++++--
vcs-svn/svndiff.c | 6 ++++--
2 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/builtin/reflog.c b/builtin/reflog.c
index b3c9e27..c2ea9d3 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -494,8 +494,10 @@ static int parse_expire_cfg_value(const char *var, const char *value, unsigned l
static int parse_expire_cfg_value(const char *var, const char *value, unsigned long *expire)
{
- if (!value)
- return config_error_nonbool(var);
+ if (!value) {
+ config_error_nonbool(var);
+ return -1;
+ }
if (!strcmp(value, "never") || !strcmp(value, "false")) {
*expire = 0;
return 0;
diff --git a/vcs-svn/svndiff.c b/vcs-svn/svndiff.c
index 74c97c4..46e96f6 100644
--- a/vcs-svn/svndiff.c
+++ b/vcs-svn/svndiff.c
@@ -121,7 +121,8 @@ static int read_int(struct line_buffer *in, uintmax_t *result, off_t *len)
*len = sz - 1;
return 0;
}
- return error_short_read(in);
+ error_short_read(in);
+ return -1;
}
static int parse_int(const char **buf, size_t *result, const char *end)
@@ -140,7 +141,8 @@ static int parse_int(const char **buf, size_t *result, const char *end)
*buf = pos + 1;
return 0;
}
- return error("invalid delta: unexpected end of instructions section");
+ error("invalid delta: unexpected end of instructions section");
+ return -1;
}
static int read_offset(struct line_buffer *in, off_t *result, off_t *len)
--
1.8.0.2.4.g59402aa
^ permalink raw reply related
* [PATCH/RFC 2/3] inline error functions with constant returns
From: Jeff King @ 2012-12-14 22:12 UTC (permalink / raw)
To: git
In-Reply-To: <20121214220903.GA18418@sigill.intra.peff.net>
The error() function reports an error message to stderr and
returns -1. That makes it handy for returning errors from
functions with a single-line:
return error("something went wrong", ...);
In this case, we know something that the compiler does not,
namely that this is equivalent to:
error("something went wrong", ...);
return -1;
Knowing that the return value is constant can let the
compiler do better control flow analysis, which means it can
give more accurate answers for static warnings, like
-Wuninitialized. But because error() is found in a different
compilation unit, the compiler doesn't get to see the code
when making decisions about the caller.
This patch makes error(), along with a handful of functions
which wrap it, an inline function, giving the compiler the
extra information. This prevents some false positives when
-Wunitialized is used with -O3.
Signed-off-by: Jeff King <peff@peff.net>
---
Not really meant for inclusion. The opterror() bit does silence one
warning, but I think the error() inlining is doing absolutely nothing.
cache.h | 10 +++++++++-
config.c | 9 ---------
git-compat-util.h | 13 ++++++++++++-
parse-options.c | 11 ++++++-----
parse-options.h | 8 +++++++-
usage.c | 12 +-----------
6 files changed, 35 insertions(+), 28 deletions(-)
diff --git a/cache.h b/cache.h
index 18fdd18..fb7c5e2 100644
--- a/cache.h
+++ b/cache.h
@@ -1135,10 +1135,18 @@ extern const char *get_commit_output_encoding(void);
extern int check_repository_format_version(const char *var, const char *value, void *cb);
extern int git_env_bool(const char *, int);
extern int git_config_system(void);
-extern int config_error_nonbool(const char *);
extern const char *get_log_output_encoding(void);
extern const char *get_commit_output_encoding(void);
+/*
+ * Call this to report error for your variable that should not
+ * get a boolean value (i.e. "[my] var" means "true").
+ */
+static inline int config_error_nonbool(const char *var)
+{
+ return error("Missing value for '%s'", var);
+}
+
extern int git_config_parse_parameter(const char *, config_fn_t fn, void *data);
struct config_include_data {
diff --git a/config.c b/config.c
index fb3f868..ea4a98f 100644
--- a/config.c
+++ b/config.c
@@ -1655,12 +1655,3 @@ int git_config_rename_section(const char *old_name, const char *new_name)
{
return git_config_rename_section_in_file(NULL, old_name, new_name);
}
-
-/*
- * Call this to report error for your variable that should not
- * get a boolean value (i.e. "[my] var" means "true").
- */
-int config_error_nonbool(const char *var)
-{
- return error("Missing value for '%s'", var);
-}
diff --git a/git-compat-util.h b/git-compat-util.h
index 2e79b8a..c38de42 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -285,9 +285,20 @@ extern void warning(const char *err, ...) __attribute__((format (printf, 1, 2)))
extern NORETURN void usagef(const char *err, ...) __attribute__((format (printf, 1, 2)));
extern NORETURN void die(const char *err, ...) __attribute__((format (printf, 1, 2)));
extern NORETURN void die_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));
-extern int error(const char *err, ...) __attribute__((format (printf, 1, 2)));
extern void warning(const char *err, ...) __attribute__((format (printf, 1, 2)));
+extern void (*error_routine)(const char *err, va_list params);
+
+__attribute__((format (printf, 1, 2)))
+static inline int error(const char *err, ...)
+{
+ va_list params;
+ va_start(params, err);
+ error_routine(err, params);
+ va_end(params);
+ return -1;
+}
+
extern void set_die_routine(NORETURN_PTR void (*routine)(const char *err, va_list params));
extern void set_error_routine(void (*routine)(const char *err, va_list params));
diff --git a/parse-options.c b/parse-options.c
index c1c66bd..5268d4e 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -18,13 +18,14 @@ int opterror(const struct option *opt, const char *reason, int flags)
return error("BUG: switch '%c' %s", opt->short_name, reason);
}
-int opterror(const struct option *opt, const char *reason, int flags)
+void opterror_report(const struct option *opt, const char *reason, int flags)
{
if (flags & OPT_SHORT)
- return error("switch `%c' %s", opt->short_name, reason);
- if (flags & OPT_UNSET)
- return error("option `no-%s' %s", opt->long_name, reason);
- return error("option `%s' %s", opt->long_name, reason);
+ error("switch `%c' %s", opt->short_name, reason);
+ else if (flags & OPT_UNSET)
+ error("option `no-%s' %s", opt->long_name, reason);
+ else
+ error("option `%s' %s", opt->long_name, reason);
}
static int get_arg(struct parse_opt_ctx_t *p, const struct option *opt,
diff --git a/parse-options.h b/parse-options.h
index 71a39c6..23673c7 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -176,7 +176,13 @@ extern int optbug(const struct option *opt, const char *reason);
const struct option *options);
extern int optbug(const struct option *opt, const char *reason);
-extern int opterror(const struct option *opt, const char *reason, int flags);
+extern void opterror_report(const struct option *opt, const char *reason, int flags);
+static inline int opterror(const struct option *opt, const char *reason, int flags)
+{
+ opterror_report(opt, reason, flags);
+ return -1;
+}
+
/*----- incremental advanced APIs -----*/
enum {
diff --git a/usage.c b/usage.c
index 8eab281..9f8e342 100644
--- a/usage.c
+++ b/usage.c
@@ -53,7 +53,7 @@ static NORETURN_PTR void (*die_routine)(const char *err, va_list params) = die_b
* (ugh), so keep things static. */
static NORETURN_PTR void (*usage_routine)(const char *err, va_list params) = usage_builtin;
static NORETURN_PTR void (*die_routine)(const char *err, va_list params) = die_builtin;
-static void (*error_routine)(const char *err, va_list params) = error_builtin;
+void (*error_routine)(const char *err, va_list params) = error_builtin;
static void (*warn_routine)(const char *err, va_list params) = warn_builtin;
void set_die_routine(NORETURN_PTR void (*routine)(const char *err, va_list params))
@@ -130,16 +130,6 @@ void NORETURN die_errno(const char *fmt, ...)
va_end(params);
}
-int error(const char *err, ...)
-{
- va_list params;
-
- va_start(params, err);
- error_routine(err, params);
- va_end(params);
- return -1;
-}
-
void warning(const char *warn, ...)
{
va_list params;
--
1.8.0.2.4.g59402aa
^ permalink raw reply related
* [PATCH 1/3] remote-testsvn: fix unitialized variable
From: Jeff King @ 2012-12-14 22:11 UTC (permalink / raw)
To: git; +Cc: Florian Achleitner
In-Reply-To: <20121214220903.GA18418@sigill.intra.peff.net>
In remote-test-svn, there is a parse_rev_note function to
parse lines of the form "Revision-number" from notes. If it
finds such a line and parses it, it returns 0, copying the
value into a "struct rev_note". If it finds an entry that is
garbled or out of range, it returns -1 to signal an error.
However, if it does not find any "Revision-number" line at
all, it returns success but does not put anything into the
rev_note. So upon a successful return, the rev_note may or
may not be initialized, and the caller has no way of
knowing.
gcc does not usually catch the use of the unitialized
variable because the conditional assignment happens in a
separate function from the point of use. However, when
compiling with -O3, gcc will inline parse_rev_note and
notice the problem.
We can fix it by returning "-1" when no note is found (so on
a zero return, we always found a valid value).
Signed-off-by: Jeff King <peff@peff.net>
---
I think this is the right fix, but I am not too familiar with this code,
so I might be missing a case where a missing "Revision-number" should
provide some sentinel value (like "0") instead of returning an error. In
fact, of the two callsites, one already does such a zero-initialization.
remote-testsvn.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/remote-testsvn.c b/remote-testsvn.c
index 51fba05..5ddf11c 100644
--- a/remote-testsvn.c
+++ b/remote-testsvn.c
@@ -90,10 +90,12 @@ static int parse_rev_note(const char *msg, struct rev_note *res)
if (end == value || i < 0 || i > UINT32_MAX)
return -1;
res->rev_nr = i;
+ return 0;
}
msg += len + 1;
}
- return 0;
+ /* didn't find it */
+ return -1;
}
static int note2mark_cb(const unsigned char *object_sha1,
--
1.8.0.2.4.g59402aa
^ permalink raw reply related
* [PATCH/RFC 0/3] compiling git with gcc -O3 -Wuninitialized
From: Jeff King @ 2012-12-14 22:09 UTC (permalink / raw)
To: git
I always compile git with "gcc -Wall -Werror", because it catches a lot
of dubious constructs, and we usually keep the code warning-free.
However, I also typically compile with "-O0" because I end up debugging
a fair bit.
Sometimes, though, I compile with -O3, which yields a bunch of new
"variable might be used uninitialized" warnings. What's happening is
that as functions get inlined, the compiler can do more static analysis
of the variables. So given two functions like:
int get_foo(int *foo)
{
if (something_that_might_fail() < 0)
return error("unable to get foo");
*foo = 0;
return 0;
}
void some_fun(void)
{
int foo;
if (get_foo(&foo) < 0)
return -1;
printf("foo is %d\n", foo);
}
If get_foo() is not inlined, then when compiling some_fun, gcc sees only
that a pointer to the local variable is passed, and must assume that it
is an out parameter that is initialized after get_foo returns.
However, when get_foo() is inlined, the compiler may look at all of the
code together and see that some code paths in get_foo() do not
initialize the variable. And we get the extra warnings.
In some cases, this can actually reveal real bugs. The first patch fixes
such a bug:
[1/3]: remote-testsvn: fix unitialized variable
In most cases, though (including the example above), it's a false
positive. We know something the compiler does not: that error() always
returns -1, and therefore we will either exit early from some_fun, or
"foo" will be properly initialized.
The second two patches:
[2/3]: inline error functions with constant returns
[3/3]: silence some -Wuninitialized warnings around errors
try to expose that return value more clearly to the calling code. After
applying both, git compiles cleanly with "-Wall -O3". But the patches
themselves are kind of ugly.
Patch 2/3 tries inlining error() and a few other functions, which lets
the caller see the return value. Unfortunately, this doesn't actually
work in all cases. I think what is happening is that because error() is
a variadic function, gcc refuses to inline it (and if you give it the
"always_inline" attribute, it complains loudly). So it works for some
functions, but not for error(), which is the most common one.
Patch 3/3 takes a more heavy-handed approach, and replaces some
instances of "return error(...)" with "error(...); return -1". This
works, but it's kind of ugly. The whole point of error()'s return code
is to allow the "return error(...)" shorthand, and it basically means we
cannot use it in some instances.
I really like keeping us -Wall clean (because it means when warnings do
come up, it's easy to pay attention to them). But I feel like patch 3 is
making the code less readable just to silence the false positives. We
can always use the "int foo = foo" trick, but I'd like to avoid that
where we can. Not only is it ugly in itself, but it means that we've
shut off the warnings if a problem is ever introduced to that spot.
Can anybody think of a clever way to expose the constant return value of
error() to the compiler? We could do it with a macro, but that is also
out for error(), as we do not assume the compiler has variadic macros. I
guess we could hide it behind "#ifdef __GNUC__", since it is after all
only there to give gcc's analyzer more information. But I'm not sure
there is a way to make a macro that is syntactically identical. I.e.,
you cannot just replace "error(...)" in "return error(...);" with a
function call plus a value for the return statement. You'd need
something more like:
#define RETURN_ERROR(fmt, ...) \
do { \
error(fmt, __VA_ARGS__); \
return -1; \
} while(0) \
which is awfully ugly.
Thoughts?
-Peff
^ permalink raw reply
* Re: [PATCH 4/4] Declare that HP NonStop systems require strings.h
From: Johannes Sixt @ 2012-12-14 20:30 UTC (permalink / raw)
To: David Michael; +Cc: git@vger.kernel.org, gitster
In-Reply-To: <CAEvUa7mSOe6gs8JqkewYV=CXt78Y68nTFbFEfEOuCzaV5-DO8A@mail.gmail.com>
Am 14.12.2012 20:57, schrieb David Michael:
> This platform previously included strings.h automatically. However, the
> build system now requires an explicit option to do so.
>
> Signed-off-by: David Michael <fedora.dm0@gmail.com>
> ---
> Makefile | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/Makefile b/Makefile
> index fb78f7f..e84b0cb 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1357,6 +1357,7 @@ ifeq ($(uname_S),NONSTOP_KERNEL)
> # Added manually, see above.
> NEEDS_SSL_WITH_CURL = YesPlease
> HAVE_LIBCHARSET_H = YesPlease
> + HAVE_STRINGS_H = YesPlease
> NEEDS_LIBICONV = YesPlease
> NEEDS_LIBINTL_BEFORE_LIBICONV = YesPlease
> NO_SYS_SELECT_H = UnfortunatelyYes
If NONSTOP_KERNEL is the platform that defines __TANDEM, then this
should be squashed into the previous patch, shouldn't it?
-- Hannes
^ permalink raw reply
* [PATCH 4/4] Declare that HP NonStop systems require strings.h
From: David Michael @ 2012-12-14 19:57 UTC (permalink / raw)
To: git@vger.kernel.org; +Cc: gitster
This platform previously included strings.h automatically. However, the
build system now requires an explicit option to do so.
Signed-off-by: David Michael <fedora.dm0@gmail.com>
---
Makefile | 1 +
1 file changed, 1 insertion(+)
diff --git a/Makefile b/Makefile
index fb78f7f..e84b0cb 100644
--- a/Makefile
+++ b/Makefile
@@ -1357,6 +1357,7 @@ ifeq ($(uname_S),NONSTOP_KERNEL)
# Added manually, see above.
NEEDS_SSL_WITH_CURL = YesPlease
HAVE_LIBCHARSET_H = YesPlease
+ HAVE_STRINGS_H = YesPlease
NEEDS_LIBICONV = YesPlease
NEEDS_LIBINTL_BEFORE_LIBICONV = YesPlease
NO_SYS_SELECT_H = UnfortunatelyYes
--
1.7.11.7
^ permalink raw reply related
* [PATCH 3/4] Generalize the inclusion of strings.h
From: David Michael @ 2012-12-14 19:57 UTC (permalink / raw)
To: git@vger.kernel.org; +Cc: gitster
The header strings.h was formerly only included for a particular platform to
define strcasecmp, but another platform requiring this inclusion has been
found. The build system will now include the file based on its presence
determined by configure.
Signed-off-by: David Michael <fedora.dm0@gmail.com>
---
Makefile | 6 ++++++
configure.ac | 6 ++++++
git-compat-util.h | 2 +-
3 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/Makefile b/Makefile
index 735a834..fb78f7f 100644
--- a/Makefile
+++ b/Makefile
@@ -74,6 +74,8 @@ all::
# Define NO_D_TYPE_IN_DIRENT if your platform defines DT_UNKNOWN but lacks
# d_type in struct dirent (Cygwin 1.5, fixed in Cygwin 1.7).
#
+# Define HAVE_STRINGS_H if you have strings.h and need it for strcasecmp.
+#
# Define NO_STRCASESTR if you don't have strcasestr.
#
# Define NO_MEMMEM if you don't have memmem.
@@ -1892,6 +1894,10 @@ ifdef HAVE_LIBCHARSET_H
EXTLIBS += $(CHARSET_LIB)
endif
+ifdef HAVE_STRINGS_H
+ BASIC_CFLAGS += -DHAVE_STRINGS_H
+endif
+
ifdef HAVE_DEV_TTY
BASIC_CFLAGS += -DHAVE_DEV_TTY
endif
diff --git a/configure.ac b/configure.ac
index 3347c17..4176db8 100644
--- a/configure.ac
+++ b/configure.ac
@@ -886,6 +886,12 @@ AC_CHECK_HEADER([libcharset.h],
[HAVE_LIBCHARSET_H=YesPlease],
[HAVE_LIBCHARSET_H=])
GIT_CONF_SUBST([HAVE_LIBCHARSET_H])
+#
+# Define HAVE_STRINGS_H if you have strings.h
+AC_CHECK_HEADER([strings.h],
+[HAVE_STRINGS_H=YesPlease],
+[HAVE_STRINGS_H=])
+GIT_CONF_SUBST([HAVE_STRINGS_H])
# Define CHARSET_LIB if libiconv does not export the locale_charset symbol
# and libcharset does
CHARSET_LIB=
diff --git a/git-compat-util.h b/git-compat-util.h
index ace1549..d7359ef 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -99,7 +99,7 @@
#include <stdlib.h>
#include <stdarg.h>
#include <string.h>
-#ifdef __TANDEM /* or HAVE_STRINGS_H or !NO_STRINGS_H? */
+#ifdef HAVE_STRINGS_H
#include <strings.h> /* for strcasecmp() */
#endif
#include <errno.h>
--
1.7.11.7
^ permalink raw reply related
* [PATCH 1/4] Support builds when sys/param.h is missing
From: David Michael @ 2012-12-14 19:56 UTC (permalink / raw)
To: git@vger.kernel.org; +Cc: gitster
An option is added to the Makefile to skip the inclusion of sys/param.h.
The only known platform with this condition thus far is the z/OS UNIX System
Services environment.
Signed-off-by: David Michael <fedora.dm0@gmail.com>
---
Makefile | 5 +++++
configure.ac | 6 ++++++
git-compat-util.h | 2 ++
3 files changed, 13 insertions(+)
diff --git a/Makefile b/Makefile
index 736ecd4..8c3a0dd 100644
--- a/Makefile
+++ b/Makefile
@@ -165,6 +165,8 @@ all::
# Define NO_POLL if you do not have or don't want to use poll().
# This also implies NO_SYS_POLL_H.
#
+# Define NO_SYS_PARAM_H if you don't have sys/param.h.
+#
# Define NO_PTHREADS if you do not have or do not want to use Pthreads.
#
# Define NO_PREAD if you have a problem with pread() system call (e.g.
@@ -1748,6 +1750,9 @@ endif
ifdef NO_SYS_POLL_H
BASIC_CFLAGS += -DNO_SYS_POLL_H
endif
+ifdef NO_SYS_PARAM_H
+ BASIC_CFLAGS += -DNO_SYS_PARAM_H
+endif
ifdef NO_INTTYPES_H
BASIC_CFLAGS += -DNO_INTTYPES_H
endif
diff --git a/configure.ac b/configure.ac
index ad215cc..317bfc6 100644
--- a/configure.ac
+++ b/configure.ac
@@ -699,6 +699,12 @@ AC_CHECK_HEADER([sys/poll.h],
[NO_SYS_POLL_H=UnfortunatelyYes])
GIT_CONF_SUBST([NO_SYS_POLL_H])
#
+# Define NO_SYS_PARAM_H if you don't have sys/param.h
+AC_CHECK_HEADER([sys/param.h],
+[NO_SYS_PARAM_H=],
+[NO_SYS_PARAM_H=UnfortunatelyYes])
+GIT_CONF_SUBST([NO_SYS_PARAM_H])
+#
# Define NO_INTTYPES_H if you don't have inttypes.h
AC_CHECK_HEADER([inttypes.h],
[NO_INTTYPES_H=],
diff --git a/git-compat-util.h b/git-compat-util.h
index 2e79b8a..ace1549 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -104,7 +104,9 @@
#endif
#include <errno.h>
#include <limits.h>
+#ifndef NO_SYS_PARAM_H
#include <sys/param.h>
+#endif
#include <sys/types.h>
#include <dirent.h>
#include <sys/time.h>
--
1.7.11.7
^ permalink raw reply related
* [PATCH 2/4] Detect when the passwd struct is missing pw_gecos
From: David Michael @ 2012-12-14 19:56 UTC (permalink / raw)
To: git@vger.kernel.org; +Cc: gitster
NO_GECOS_IN_PWENT was documented with other Makefile variables but was only
enforced by manually defining it to the C preprocessor. This adds support
for detecting the condition with configure and defining the make variable.
Signed-off-by: David Michael <fedora.dm0@gmail.com>
---
Makefile | 3 +++
configure.ac | 8 ++++++++
2 files changed, 11 insertions(+)
diff --git a/Makefile b/Makefile
index 8c3a0dd..735a834 100644
--- a/Makefile
+++ b/Makefile
@@ -1657,6 +1657,9 @@ endif
ifdef NO_D_INO_IN_DIRENT
BASIC_CFLAGS += -DNO_D_INO_IN_DIRENT
endif
+ifdef NO_GECOS_IN_PWENT
+ BASIC_CFLAGS += -DNO_GECOS_IN_PWENT
+endif
ifdef NO_ST_BLOCKS_IN_STRUCT_STAT
BASIC_CFLAGS += -DNO_ST_BLOCKS_IN_STRUCT_STAT
endif
diff --git a/configure.ac b/configure.ac
index 317bfc6..3347c17 100644
--- a/configure.ac
+++ b/configure.ac
@@ -759,6 +759,14 @@ AC_CHECK_MEMBER(struct dirent.d_type,
[#include <dirent.h>])
GIT_CONF_SUBST([NO_D_TYPE_IN_DIRENT])
#
+# Define NO_GECOS_IN_PWENT if you don't have pw_gecos in struct passwd
+# in the C library.
+AC_CHECK_MEMBER(struct passwd.pw_gecos,
+[NO_GECOS_IN_PWENT=],
+[NO_GECOS_IN_PWENT=YesPlease],
+[#include <pwd.h>])
+GIT_CONF_SUBST([NO_GECOS_IN_PWENT])
+#
# Define NO_SOCKADDR_STORAGE if your platform does not have struct
# sockaddr_storage.
AC_CHECK_TYPE(struct sockaddr_storage,
--
1.7.11.7
^ permalink raw reply related
* Re: Build fixes for another obscure Unix
From: David Michael @ 2012-12-14 19:48 UTC (permalink / raw)
To: git@vger.kernel.org
In-Reply-To: <kaem06$3go$1@ger.gmane.org>
Hi,
On Fri, Dec 14, 2012 at 2:54 AM, Joachim Schmitz
<jojo@schmitz-digital.de> wrote:
> For what's it worth: I ACK your HP-NonStop patch (as you can see by my
> comment in git-compat-util.h I was thinking along the same line)
> https://github.com/dm0-/git/commit/933d72a5cfdc63fa9c3c68afa2f4899d9c3f791e
> together with its prerequisit
> https://github.com/dm0-/git/commit/301032c6488aeabb94ccc81bfb6d65ff2c23b924
>
> ACKed by: Joachim Schmitz <jojo@schmitz-digital.de>
Okay, thanks for verifying. Especially since another port needing
that header was just sent to the list, I'd prefer to see some
generalized feature test rather than building and maintaining an
explicit OS list.
No one has suggested any adjustments, so I'll send out those patches now.
Thanks.
David
^ permalink raw reply
* [PATCH 2/2] Port to QNX
From: Matt Kraai @ 2012-12-14 18:38 UTC (permalink / raw)
To: git; +Cc: Matt Kraai
In-Reply-To: <1355510300-31541-1-git-send-email-kraai@ftbfs.org>
From: Matt Kraai <matt.kraai@amo.abbott.com>
Signed-off-by: Matt Kraai <matt.kraai@amo.abbott.com>
---
Makefile | 19 +++++++++++++++++++
git-compat-util.h | 8 ++++++--
2 files changed, 25 insertions(+), 2 deletions(-)
diff --git a/Makefile b/Makefile
index 736ecd4..ed2539d 100644
--- a/Makefile
+++ b/Makefile
@@ -78,6 +78,8 @@ all::
#
# Define NO_MEMMEM if you don't have memmem.
#
+# Define NO_GETPAGESIZE if you don't have getpagesize.
+#
# Define NO_STRLCPY if you don't have strlcpy.
#
# Define NO_STRTOUMAX if you don't have both strtoimax and strtoumax in the
@@ -1448,6 +1450,20 @@ else
NO_CURL = YesPlease
endif
endif
+ifeq ($(uname_S),QNX)
+ COMPAT_CFLAGS += -DSA_RESTART=0
+ NEEDS_SOCKET = YesPlease
+ NO_MKDTEMP = YesPlease
+ NO_MKSTEMPS = YesPlease
+ NO_FNMATCH_CASEFOLD = YesPlease
+ NO_GETPAGESIZE = YesPlease
+ NO_ICONV = YesPlease
+ NO_MEMMEM = YesPlease
+ NO_NSEC = YesPlease
+ NO_STRCASESTR = YesPlease
+ NO_STRLCPY = YesPlease
+ PTHREAD_LIBS =
+endif
-include config.mak.autogen
-include config.mak
@@ -1859,6 +1875,9 @@ ifdef NO_MEMMEM
COMPAT_CFLAGS += -DNO_MEMMEM
COMPAT_OBJS += compat/memmem.o
endif
+ifdef NO_GETPAGESIZE
+ COMPAT_CFLAGS += -DNO_GETPAGESIZE
+endif
ifdef INTERNAL_QSORT
COMPAT_CFLAGS += -DINTERNAL_QSORT
COMPAT_OBJS += compat/qsort.o
diff --git a/git-compat-util.h b/git-compat-util.h
index 2e79b8a..6c588ca 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -75,7 +75,7 @@
# endif
#elif !defined(__APPLE__) && !defined(__FreeBSD__) && !defined(__USLC__) && \
!defined(_M_UNIX) && !defined(__sgi) && !defined(__DragonFly__) && \
- !defined(__TANDEM)
+ !defined(__TANDEM) && !defined(__QNX__)
#define _XOPEN_SOURCE 600 /* glibc2 and AIX 5.3L need 500, OpenBSD needs 600 for S_ISLNK() */
#define _XOPEN_SOURCE_EXTENDED 1 /* AIX 5.3L needs this */
#endif
@@ -99,7 +99,7 @@
#include <stdlib.h>
#include <stdarg.h>
#include <string.h>
-#ifdef __TANDEM /* or HAVE_STRINGS_H or !NO_STRINGS_H? */
+#if defined(__TANDEM) || defined(__QNX__) /* or HAVE_STRINGS_H or !NO_STRINGS_H? */
#include <strings.h> /* for strcasecmp() */
#endif
#include <errno.h>
@@ -411,6 +411,10 @@ void *gitmemmem(const void *haystack, size_t haystacklen,
const void *needle, size_t needlelen);
#endif
+#ifdef NO_GETPAGESIZE
+#define getpagesize() sysconf(_SC_PAGESIZE)
+#endif
+
#ifdef FREAD_READS_DIRECTORIES
#ifdef fopen
#undef fopen
--
1.8.1-rc1
^ permalink raw reply related
* [PATCH 0/2] Port to QNX
From: Matt Kraai @ 2012-12-14 18:38 UTC (permalink / raw)
To: git
This series ports Git to QNX. It builds on both QNX 6.3.2 and QNX
6.5.0. The test suite does not pass. Unless the corresponding
software is installed, the following arguments must be passed to Make:
NO_CURL=1 NO_GETTEXT=1 NO_OPENSSL=1 NO_PERL=1 NO_PYTHON=1 NO_TCLTK=1
[1/2]: Make lock local to fetch_pack
QNX 6.3.2's unistd.h declares a function named lock, which causes
fetch-pack.c to fail to compile if lock has file-scope. Since it's
only used in a single function, move it therein.
[2/2]: Port to QNX
^ permalink raw reply
* [PATCH 1/2] Make lock local to fetch_pack
From: Matt Kraai @ 2012-12-14 18:38 UTC (permalink / raw)
To: git; +Cc: Matt Kraai
In-Reply-To: <1355510300-31541-1-git-send-email-kraai@ftbfs.org>
From: Matt Kraai <matt.kraai@amo.abbott.com>
lock is only used by fetch_pack, so move it into that function.
Signed-off-by: Matt Kraai <matt.kraai@amo.abbott.com>
---
fetch-pack.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/fetch-pack.c b/fetch-pack.c
index 099ff4d..9d9762d 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -874,8 +874,6 @@ static int fetch_pack_config(const char *var, const char *value, void *cb)
return git_default_config(var, value, cb);
}
-static struct lock_file lock;
-
static void fetch_pack_setup(void)
{
static int did_setup;
@@ -896,6 +894,7 @@ struct ref *fetch_pack(struct fetch_pack_args *args,
struct string_list *sought,
char **pack_lockfile)
{
+ static struct lock_file lock;
struct stat st;
struct ref *ref_cpy;
--
1.8.1-rc1
^ permalink raw reply related
* Re: [PATCH] Documentation/git: add missing info about --git-dir command-line option
From: Manlio Perillo @ 2012-12-14 17:26 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vmwxj3vxx.fsf@alter.siamese.dyndns.org>
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Il 12/12/2012 20:35, Junio C Hamano ha scritto:
> Manlio Perillo <manlio.perillo@gmail.com> writes:
>
>> The Documentation/git.txt file, in the GIT_DIR environment variable
>> section, did not mentioned that this value can also be set using the
>> --git-dir command line option.
>> ---
>
> s/mentioned/mention/; Also it may help to say
>
> Unlike other environment variables (e.g. GIT_WORK_TREE,
> GIT_NAMESPACE),
>
I'm sorry, I just copied this text "as is" (I'm lazy) in the commit
message failing to notice the use of the tab character.
When I checked the patch email message, my editor rendered the tab
character as a single space...
That's the reason why I have all my editors configured to never ever use
tabs.
Manlio Perillo
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
iEYEARECAAYFAlDLYUMACgkQscQJ24LbaUTUPACcDhufXkawZZPBV0p/af1GFu1D
/BcAnjPARpeTi4EdyM/3wV0eI9U9Fu51
=rSfl
-----END PGP SIGNATURE-----
^ permalink raw reply
* [PATCH] README: Git is released under the GPLv2, not just "the GPL"
From: Stefano Lattarini @ 2012-12-14 15:37 UTC (permalink / raw)
To: git; +Cc: gitster
And this is clearly stressed by Linus in the COPYING file. So make it
clear in the README as well, to avoid possible misunderstandings.
Signed-off-by: Stefano Lattarini <stefano.lattarini@gmail.com>
---
README | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/README b/README
index d2690ec..c50e6f4 100644
--- a/README
+++ b/README
@@ -19,9 +19,10 @@ Git is a fast, scalable, distributed revision control system with an
unusually rich command set that provides both high-level operations
and full access to internals.
-Git is an Open Source project covered by the GNU General Public License.
-It was originally written by Linus Torvalds with help of a group of
-hackers around the net. It is currently maintained by Junio C Hamano.
+Git is an Open Source project covered by the GNU General Public License
+(version 2). It was originally written by Linus Torvalds with help
+of a group of hackers around the net. It is currently maintained by
+Junio C Hamano.
Please read the file INSTALL for installation instructions.
--
1.8.0.1.347.gf94c325
^ permalink raw reply related
* Re: How to avoid the ^M induced by Meld and Git
From: Karl Brand @ 2012-12-14 13:45 UTC (permalink / raw)
To: Michael J Gruber; +Cc: git
In-Reply-To: <50C8AE14.4000704@erasmusmc.nl>
FYI: It was all down to unexpected dos formatting of one of the files.
Here's how i sorted out my unwanted ^M line endings in case anyone
stumbles on this thread with the same issue i had. (reposted from
http://stackoverflow.com/questions/13799631/why-is-m-being-added-to-a-script-r-after-modifying-with-meld/13879162#13879162
)
Thanks again to those investing effort on this issue - cheers!
In a terminal i ran cat script.r | hexdump -C | head and found a 0d 0a
in the file, which is DOS formatting for a new line (carriage return 0d
immediately followed by a line feed 0a). I ran the same command on
another_script.r i was merging with but only observed 0a, no 0d 0a,
indicating Unix formatting.
To check further if this was the source of the ^M line endings, script.r
was converted to unix formatting via dos2unix script.r & verified that
0d 0a was converted to 0a using hexdump -C as above. I performed a merge
using Meld in attempting to replicate the process which yielded ^M line
endings in my script's. I re-oppened both files in Emacs/ESS and found
no ^M line endings. Short of converting script.r back to dos formatting
and repeating the above procedure to see if the ^M line endings
re-appear, i believe i've solved my ^M issue, which simply is that,
unbeknownst to me, one of my files was dos formatted. My take home
message: in a Windows dominated environ, never assume that one's
personal linux environment doesn't contain DOS bits. Or line endings.
On 12/12/12 17:17, Karl Brand wrote:
>
>
> On 12/12/12 17:08, Michael J Gruber wrote:
>> Karl Brand venit, vidit, dixit 12.12.2012 16:34:
>>>
>>>
>>> On 12/12/12 15:57, Michael J Gruber wrote:
>>>> Karl Brand venit, vidit, dixit 11.12.2012 13:33:
>>>>> Esteemed Git users,
>>>>>
>>>>> What i do:
>>>>>
>>>>> 1. Create a script.r using Emacs/ESS.
>>>>> 2. Make some modifications to script.r with the nice diff gui, Meld
>>>>> 3. Commit these modifications using git commit -am "my message"
>>>>> 4. Reopen script.r in Emacs/ESS to continue working.
>>>>>
>>>>> The lines added (&/edited ?) using Meld all end with ^M which i
>>>>> certainly don't want. Lines not added/edited with Meld do NOT end
>>>>> with ^M.
>>>>
>>>> What happens if you leave out step 3? If the same happens then Meld is
>>>> the culprit. (Unless you've set some special options, git does not
>>>> modify your file on commit, so this can't be git related.)
>>>>
>>>
>>> Leaving out step 3. results in exactly the same thing. Thus Git doesn't
>>> appear to be responsible for the ^M's. Thanks a lot for your effort on
>>> this and my apologies for not taking the care to dissect this more
>>> carefully as you suggested. Over to the Meld mailing list...
>>
>> I didn't mean to shy you away ;)
>
> Applying recent lessons form StackO'flow :P
>>
>> Could it be that there is a ^M very early in the file (or rather
>> something else which isn't covered by dos2unix) so that Meld thinks it's
>> DOS and inserts line endings as DOS? At least that's what vim would do.
>>
> If there is i don't find it using Emacs, but Emacs may only show what
> dos2unix sees... Will post back the Meld insights (here's hoping!)
>
>> In any case, Meld people over there should know (or get to know) that
>> effect.
>>
>>>>> There are plenty of posts around about these being line endings
>>>>> used for
>>>>> windows which can appear when working on a script under a *nix OS
>>>>> which
>>>>> has previously been edited in a Windows OS. This is not the case
>>>>> here -
>>>>> everything is taking place on Ubuntu 12.04.
>>>>>
>>>>> FWIW: the directory is being synced by dropbox; and in Meld,
>>>>> Preferences
>>>>> > Encoding tab, "utf8" is entered in the text box.
>>>>>
>>>>> Current work around is running in a terminal: dos2unix
>>>>> /path/to/script.r
>>>>> which strips the ^M's
>>>>>
>>>>> But this just shouldn't be necessary and I'd really appreciate the
>>>>> reflections & advice on how to stop inducing these ^M's !
>>>>>
>>>>> With thanks,
>>>>>
>>>>> Karl
>>>>>
>>>>> (re)posted here as suggested off topic at SO:
>>>>> http://stackoverflow.com/questions/13799631/create-script-r-in-emacs-modify-with-meld-git-commit-reopen-in-emacs-m
>>>>>
>>>>>
>>>>
>>>
>
--
Karl Brand
Dept of Cardiology and Dept of Bioinformatics
Erasmus MC
Dr Molewaterplein 50
3015 GE Rotterdam
T +31 (0)10 703 2460 |M +31 (0)642 777 268 |F +31 (0)10 704 4161
^ permalink raw reply
* Re: What's cooking in git.git (Dec 2012, #03; Wed, 12)
From: Max Horn @ 2012-12-14 13:11 UTC (permalink / raw)
To: Felipe Contreras; +Cc: Junio C Hamano, git
In-Reply-To: <CAMP44s3Es-rLjwe6sgOi9OmwQouM4AbFKAbGB5UgS6sUtYRgKQ@mail.gmail.com>
Felipe,
please stop referring to "facts" and "obvious". You pretend to be a being of pure reason and that everything you say is logical, drawn from facts. But you forget or perhaps do not know that logic by itself proofs nothing, it all depends on the axioms you impose. And yours are quite different from what others consider as such, and apparently also inconsistent.
So, instead of trying to twist things around so that broken things in your code are not broken after all, why not simply re-roll your patch with the "obvious" fixes applies? As you write yourself, time is not pressing at all -- so I don't see why your patch should be merged now, and fixed later, contrary to how other people's patches are treated? Why not fix them first, and then apply? We do have time, after all! And nobody is expecting you to do that while you are on vacation, either. Nor that you do it instantly.
Just say: "OK, I see there is a problem with the patches; even though I consider it unimportant, I will play by the rules everybody here has to follow, and re-roll the patch series. But this is of low priority for me, so I cannot say right now when it will happen".
Everybody would be happy then. Except perhaps the hypothetical users, who would have to wait a bit longer -- but oh, not really, because they can just use remote-bzr from your repo, yay :-). I really like that about it, it lives in contrib, so one can use it w/o it being merged into git.git.
Instead, you make claims that make you look like a foolish and arrogant ass, all the while insulting Junio and me implicitly. Why do you do that??? It delays acceptance of your nice work. As you write, this hurts the users. So why do it?
Since you keep complaining that nobody ever really can point to anything wrong your said, I'll do you the favor by deconstructing one of the claims you made:
On 13.12.2012, at 20:06, Felipe Contreras wrote:
> On Thu, Dec 13, 2012 at 6:04 AM, Max Horn <postbox@quendi.de> wrote:
>>
>> On 13.12.2012, at 11:08, Felipe Contreras wrote:
>>
>>> On Thu, Dec 13, 2012 at 2:11 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>>>
>>>>>> New remote helper for bzr (v3). With minor fixes, this may be ready
>>>>>> for 'next'.
>>>>>
>>>>> What minor fixes?
>>>>
>>>> Lookng at the above (fixup), $gmane/210744 comes to mind
>>>
>>> That doesn't matter. The code and the tests would work just fine.
>>
>>
>> It doesn't matter? I find that statement hard to align with what the maintainer of git, and thus the person who decides whether your patch series gets merged or not, wrote just above? In fact, it seems to me that what Junio said matters a great deal...
>
> So you think Junio knows more about remote-bzr than I do?
This is a classical straw man argument. No, I do not think that. But I do think that Junio knows enough to review your code, and I do think that the point he raised is valid. You disagree with the importance of his point
> I repeat; it
> doesn't affect the tests, it doesn't affect the code, it doesn't cause
> any problem. remote-bzr could be merged today, in fact, it could have
> been merged a month ago.
>
> You don't trust me? Here, look:
>
[..]
> All this code is a no-op, because, as Junio pointed out, cmd is null.
> How is that a problem? It's not.
It is a problem. Because either the code inside the if is important, and then this is a bug. Or it is not important -- then it should not be there in the first place.
Either way, the patch series should be re-rolled. Of course in a whatever time frame suits you. If you are not willing to do that, this is sad, but of course also your right!
[...]
>> This is a very strange attitude...
>>
>> In another email, you complained about nobody reviewing your patches respectively nobody voicing any constructive criticism. Yet Junio did just that, and again in $gmane/210745 -- and you replied to neither, and acted on neither (not even by refuting the points brought up), and now summarily dismiss them as irrelevant. I find that quite disturbing :-(.
>
> I didn't say it was irrelevant, it should be fixed,
Actually, you wrote:
"That doesn't matter."
So I paraphrased. In any case, I am glad to hear you finally agree that it should be fixed (which you did *not* say in your initial reply). So the problem we have seems to be that you do not understand how patches typically handled in git.git. Well, based on my observation: If reviews point out things in a patch series that are not optimal or even broken, it is expected that the submitter fixes this locally and resubmits a new version of the series. In some cases, it is possible to make exceptions, e.g. trivial typo fixes can be applied on the fly. But otherwise, you re-roll, you do not get your stuff merged just based on the promise that you'll submit a series of fixes later. Esp. if the fixes are relatively easy.
Of course more exceptions can be made, but based on what I saw, this is rare, and has to be justified quite well. I fail to see a justification in this case... You mentioned "the users" but AFAIK there are no known users yet, and even if, they can simply use remote-bzr from your tree.
This could perhaps be documented explicitly in Documentation/SubmittingPatches. Not sure if I think this would be a good idea or even helpful, just thinking out aloud.
> but Junio said
> "With minor fixes, this may be ready for 'next'." which is no true
> IMO, it's ready *now*, it was ready one month ago. For 'next', this
> problem doesn't matter.
>
> The feedback is appreciated, but delaying the merging of this code for
> no reason makes little sense to me.
And here goes the insult. You say Junio has no reason to delay the merging. When you really mean that you don't agree with his reasons. So you attack his professionalism and integrity by alluding that he has some ulterior motives to delay the patch. E.g. that he is hates you, is just mean, does it out of stubbornness, etc.
> Junio, of course, can do whatever he wants. The removal of this no-op code can wait, or it can be done
> on top of v3, there's no need for re-roll, and Junio already
> complained about the v3 re-roll.
>
> And I didn't act because I was on vacations, git development is not my
> only priority.
Of course. Nobody is complaining that you take too long to reply. We are just unhappy in the way you reply when you do reply :-(.
> And even if I had time, I don't see why I should
> prioritize this fix, it's not important, the code is ready.
Another straw man: Nobody asked you to prioritize the fix, take your time. It was you who asked that the series should be applied without any further fixes.
>
>>>> but there may be others. It is the responsibility of a contributor to keep
>>>> track of review comments others give to his or her patches and
>>>> reroll them, so I do not recall every minor details, sorry.
>>>
>>> There is nothing that prevents remote-bzr from being merged.
>>
>> Well, I think that is up to Junio to decide in the end, though :-). He wrote
>
> No. He can decide if the code gets merged, but he is not the voice of
> truth. Nothing prevents him from merging the code, except himself.
> There is no known issue with the code, that is a true fact.
Here are a multitude of fallacies hidden, partially explainable by a differing set of axioms, and/or shear arrogance.
Let us re-reread what was said: Initially, you claimed that "There is nothing that prevents remote-bzr from being merged".
Now, it wasn't said in the above, but let me make it explicit: This statement is "obviously"[1] wrong. There are parts of the patch series Junio thinks are not up to par, and he made it quite clear that he will not merge it until these things are resolved.
Hence, ignoring all else, there obviously *is* something that prevents remote-bzr from being merged. That is a "fact". You even admit so yourself, also contradicting yourself:
"Nothing prevents him from merging the code, except himself."
So how is it possible that you can claim that there is nothing that prevents the merge? Ignoring the self-contradicting aspects of what you wrote, the basis for your differing conclusion seems to be that you change the terms of discussion and are using a different set of axioms. In particular, you apparently redefine
"things that prevent remote-bzr from being merged"
as
"things that in Felipe's view prevent remote-bzr from being merged".
Of course one can arbitrarily bend the rules by this definition. For example, we could redefine "nothing prevents the merge" as
"no technical reasons prevent the merge", and the latter is indeed quite true; your patch series applies perfectly fine, git can do that. Of course the same holds for a patch which removes git.c from the repository, so I don't think this definition is particularly useful...
Back to your self-contradictory statement: It could be parsed as an (not well-formed) attempt to say that Junio has no objective reasons to reject your patch. I.e. you again imply that Junio's decision that the patch is not merge-ready is not based upon "logical conclusions from the given set of facts". Indeed, I would dare say that many people on the list will have interpreted your statements this way... At least I did.
This is something what a lot of people would consider a strong insult towards the professionalism and integrity of Junio. There are more examples of this in previous communications between you and other people in the list.
You finally add "There is no known issue with the code, that is a true fact.". Within your axiom set, this is certainly true. It certainly is not true in mine or Junio's... Yet you very strongly emphasis with your statement that your set of axioms is the correct one to use here, although I would guess that most people would disagree.
This is especially arrogant in view of the another straw man argument you are employing: By writing "[Junio] he is not the voice of truth.", you implicate that I or anybody were of this opinion. But I am not, and what I wrote cannot logically be construed as saying so. At least not within what most people would consider as axiom set; of course if your axiom set includes "Max believes Junio is the voice of truth", your claim because truth, albeit a tautological one. But let me make clear that any such axioms, or set of axioms leading to that implicating, are inconsistent: In my view, of course Junio is fallible and makes mistakes, and can be wrong etc. -- like any human being. Including most definitely me and you.
This is a horrible way of working within a team effort :-(. I find this a great pity, because I believe you are doing some really nice work, I esp. like your remote-hg which works much better for me than the others I tried so far.
Bye,
Max
[1] As a mathematician, I was taught to avoid the word "obvious" in any written form of proof, as it makes you sound arrogant, and it also discourages the reader from thinking critical about a statement, which is considered extremely bad. But since you like it so much, I am using here on purpose.
^ permalink raw reply
* Re: possible Improving diff algoritm
From: Javier Domingo @ 2012-12-14 12:20 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Geert Bosch, Morten Welinder, Kevin, git
In-Reply-To: <7vzk1izcv6.fsf@alter.siamese.dyndns.org>
I think the idea of being preferable to have a blank line at the end
of the added/deleted block is key in this case.
Javier Domingo
2012/12/13 Junio C Hamano <gitster@pobox.com>:
> Geert Bosch <bosch@adacore.com> writes:
>
>> It would seem that just looking at the line length (stripped) of
>> the last line, might be sufficient for cost function to minimize.
>> Here the some would be 3 vs 0. In case of ties, use the last
>> possibility with minimum cost.
>
> -- 8< --
> #ifdef A
>
> some stuff
> about A
>
> #endif
> #ifdef Z
>
> some more stuff
> about Z
>
> #endif
> -- >8 --
>
> If you insert a block for M following the existing formatting
> convention in the middle, your heuristics will pick the blank line
> after "about A" as having minimum cost, no?
>
> You inherently have to know the nature of the payload, as your eyes
> that judge the result use that knowledge when doing so, I am afraid.
> I think your "define a function that gives a good score to lines
> that are likely to be good breaking points" idea has merit, but I
> think that should be tied to the content type, most likely via the
> attribute mechanism.
>
> In any case, I consider this as a low-impact (as Michael Haggerty
> noted, it is impossible to introduce a bug that subtly break the
> output; your result is either totally borked or is correct) and
> low-hanging fruit (it can be done as a postprocessing phase after
> the xdiff machinery has done the heavy-lifting of computing LCA), if
> somebody wants to experiment and implement one. As long as the new
> heuristics is hidden behind an explicit command line option to avoid
> other "consequences", I wouldn't discourage interested parties from
> working on it. It is not just my itch, though.
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ 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