GIT: [PATCH] exec_cmd: system_path memory leak fix
Hello All, I found memory leak at exec_cmd.c in system_path function with valgrind. After this patch valgrind doesn't show any memory leaks, but I'm not sure that it is the best way to solve this problem. There are a couple of places that uses system_path, if this way will be good, i'll make another patches to fix this leak. Waiting for feedback. Signed-off-by: Alex Kuleshov kuleshovm...@gmail.com -- 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] exec_cmd: system_path memory leak fix
Signed-off-by: 0xAX kuleshovm...@gmail.com
---
exec_cmd.c | 25 -
exec_cmd.h | 4 ++--
git.c | 12 +---
3 files changed, 27 insertions(+), 14 deletions(-)
diff --git a/exec_cmd.c b/exec_cmd.c
index 698e752..08f8f80 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -6,7 +6,7 @@
static const char *argv_exec_path;
static const char *argv0_path;
-const char *system_path(const char *path)
+char *system_path(const char *path)
{
#ifdef RUNTIME_PREFIX
static const char *prefix;
@@ -14,9 +14,10 @@ const char *system_path(const char *path)
static const char *prefix = PREFIX;
#endif
struct strbuf d = STRBUF_INIT;
+ char *new_path = NULL;
if (is_absolute_path(path))
- return path;
+ return strdup(path);
#ifdef RUNTIME_PREFIX
assert(argv0_path);
@@ -32,10 +33,13 @@ const char *system_path(const char *path)
Using static fallback '%s'.\n, prefix);
}
#endif
-
strbuf_addf(d, %s/%s, prefix, path);
- path = strbuf_detach(d, NULL);
- return path;
+ new_path = malloc((strlen(prefix) + strlen(path)) + 2);
+ sprintf(new_path, %s/%s, prefix, path);
+
+ strbuf_release(d);
+
+ return new_path;
}
const char *git_extract_argv0_path(const char *argv0)
@@ -68,16 +72,16 @@ void git_set_argv_exec_path(const char *exec_path)
/* Returns the highest-priority, location to look for git programs. */
-const char *git_exec_path(void)
+char *git_exec_path(void)
{
const char *env;
if (argv_exec_path)
- return argv_exec_path;
+ return strdup(argv_exec_path);
env = getenv(EXEC_PATH_ENVIRONMENT);
if (env *env) {
- return env;
+ return strdup(env);
}
return system_path(GIT_EXEC_PATH);
@@ -96,7 +100,9 @@ void setup_path(void)
const char *old_path = getenv(PATH);
struct strbuf new_path = STRBUF_INIT;
- add_path(new_path, git_exec_path());
+ char *exec_path = git_exec_path();
+
+ add_path(new_path, exec_path);
add_path(new_path, argv0_path);
if (old_path)
@@ -107,6 +113,7 @@ void setup_path(void)
setenv(PATH, new_path.buf, 1);
strbuf_release(new_path);
+ free(exec_path);
}
const char **prepare_git_cmd(const char **argv)
diff --git a/exec_cmd.h b/exec_cmd.h
index e4c9702..03c8599 100644
--- a/exec_cmd.h
+++ b/exec_cmd.h
@@ -3,12 +3,12 @@
extern void git_set_argv_exec_path(const char *exec_path);
extern const char *git_extract_argv0_path(const char *path);
-extern const char *git_exec_path(void);
+extern char *git_exec_path(void);
extern void setup_path(void);
extern const char **prepare_git_cmd(const char **argv);
extern int execv_git_cmd(const char **argv); /* NULL terminated */
LAST_ARG_MUST_BE_NULL
extern int execl_git_cmd(const char *cmd, ...);
-extern const char *system_path(const char *path);
+extern char *system_path(const char *path);
#endif /* GIT_EXEC_CMD_H */
diff --git a/git.c b/git.c
index 82d7a1c..499dc2a 100644
--- a/git.c
+++ b/git.c
@@ -99,13 +99,19 @@ static int handle_options(const char ***argv, int *argc,
int *envchanged)
exit(0);
}
} else if (!strcmp(cmd, --html-path)) {
- puts(system_path(GIT_HTML_PATH));
+ char *git_html_path = system_path(GIT_HTML_PATH);
+ puts(git_html_path);
+ free(git_html_path);
exit(0);
} else if (!strcmp(cmd, --man-path)) {
- puts(system_path(GIT_MAN_PATH));
+ char *git_man_path = system_path(GIT_MAN_PATH);
+ puts(git_man_path);
+ free(git_man_path);
exit(0);
} else if (!strcmp(cmd, --info-path)) {
- puts(system_path(GIT_INFO_PATH));
+ char *git_info_path = system_path(GIT_INFO_PATH);
+ puts(git_info_path);
+ free(git_info_path);
exit(0);
} else if (!strcmp(cmd, -p) || !strcmp(cmd, --paginate)) {
use_pager = 1;
--
2.2.0.rc3.191.gfdea3f0
--
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] exec_cmd: system_path memory leak fix
For example some testing with valgrind:
1. pu (952f0aaadff4afca1497450911587a973c8cca02)
valgrind git help
==27034== Memcheck, a memory error detector
==27034== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al.
==27034== Using Valgrind-3.10.0 and LibVEX; rerun with -h for copyright info
==27034== Command: git help
==27034==
==27034== HERE GIT HELP OUTPUT
==27034==
==27034== HEAP SUMMARY:
==27034== in use at exit: 65 bytes in 1 blocks
==27034== total heap usage: 71 allocs, 70 frees, 11,928 bytes allocated
==27034==
==27034== LEAK SUMMARY:
==27034==definitely lost: 65 bytes in 1 blocks
==27034==indirectly lost: 0 bytes in 0 blocks
==27034== possibly lost: 0 bytes in 0 blocks
==27034==still reachable: 0 bytes in 0 blocks
==27034== suppressed: 0 bytes in 0 blocks
==27034== Rerun with --leak-check=full to see details of leaked memory
==27034==
==27034== For counts of detected and suppressed errors, rerun with: -v
==27034== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
2. With patch:
valgrind git help
==28945== Memcheck, a memory error detector
==28945== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al.
==28945== Using Valgrind-3.10.0 and LibVEX; rerun with -h for copyright info
==28945== Command: git help
==28945==
==28945== HERE git help OUTPUT
==28945==
==28945== HEAP SUMMARY:
==28945== in use at exit: 0 bytes in 0 blocks
==28945== total heap usage: 72 allocs, 72 frees, 11,956 bytes allocated
==28945==
==28945== All heap blocks were freed -- no leaks are possible
==28945==
==28945== For counts of detected and suppressed errors, rerun with: -v
==28945== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
Of course it is very small leak, but i just started to deep in git
source code. Hope i will be useful for community.
Thank you.
0xAX kuleshovm...@gmail.com @ 2014-11-23 19:56 ALMT:
Signed-off-by: 0xAX kuleshovm...@gmail.com
---
exec_cmd.c | 25 -
exec_cmd.h | 4 ++--
git.c | 12 +---
3 files changed, 27 insertions(+), 14 deletions(-)
diff --git a/exec_cmd.c b/exec_cmd.c
index 698e752..08f8f80 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -6,7 +6,7 @@
static const char *argv_exec_path;
static const char *argv0_path;
-const char *system_path(const char *path)
+char *system_path(const char *path)
{
#ifdef RUNTIME_PREFIX
static const char *prefix;
@@ -14,9 +14,10 @@ const char *system_path(const char *path)
static const char *prefix = PREFIX;
#endif
struct strbuf d = STRBUF_INIT;
+ char *new_path = NULL;
if (is_absolute_path(path))
- return path;
+ return strdup(path);
#ifdef RUNTIME_PREFIX
assert(argv0_path);
@@ -32,10 +33,13 @@ const char *system_path(const char *path)
Using static fallback '%s'.\n, prefix);
}
#endif
-
strbuf_addf(d, %s/%s, prefix, path);
- path = strbuf_detach(d, NULL);
- return path;
+ new_path = malloc((strlen(prefix) + strlen(path)) + 2);
+ sprintf(new_path, %s/%s, prefix, path);
+
+ strbuf_release(d);
+
+ return new_path;
}
const char *git_extract_argv0_path(const char *argv0)
@@ -68,16 +72,16 @@ void git_set_argv_exec_path(const char *exec_path)
/* Returns the highest-priority, location to look for git programs. */
-const char *git_exec_path(void)
+char *git_exec_path(void)
{
const char *env;
if (argv_exec_path)
- return argv_exec_path;
+ return strdup(argv_exec_path);
env = getenv(EXEC_PATH_ENVIRONMENT);
if (env *env) {
- return env;
+ return strdup(env);
}
return system_path(GIT_EXEC_PATH);
@@ -96,7 +100,9 @@ void setup_path(void)
const char *old_path = getenv(PATH);
struct strbuf new_path = STRBUF_INIT;
- add_path(new_path, git_exec_path());
+ char *exec_path = git_exec_path();
+
+ add_path(new_path, exec_path);
add_path(new_path, argv0_path);
if (old_path)
@@ -107,6 +113,7 @@ void setup_path(void)
setenv(PATH, new_path.buf, 1);
strbuf_release(new_path);
+ free(exec_path);
}
const char **prepare_git_cmd(const char **argv)
diff --git a/exec_cmd.h b/exec_cmd.h
index e4c9702..03c8599 100644
--- a/exec_cmd.h
+++ b/exec_cmd.h
@@ -3,12 +3,12 @@
extern void git_set_argv_exec_path(const char *exec_path);
extern const char *git_extract_argv0_path(const char *path);
-extern const char *git_exec_path(void);
+extern char *git_exec_path(void);
extern void setup_path(void);
extern const char **prepare_git_cmd(const char **argv);
extern int execv_git_cmd(const char **argv); /* NULL terminated */
LAST_ARG_MUST_BE_NULL
extern int execl_git_cmd(const char *cmd, ...);
-extern const char *system_path(const char *path);
+extern char *system_path(const char *path);
#endif /* GIT_EXEC_CMD_H
[PATCH RFC] CYGWIN: avoid implicit declaration warning
gcc under cygwin reports several warnings like this: warning: implicit declaration of function 'memmem' [-Wimplicit-function-declaration] This has been observed under CYGWIN-32 with GCC 4.7.3 as well as CYGWIN-64 with gcc v4.8.3-5 x86-64 Do not #define _XOPEN_SOURCE 600 for CYGWIN. Reported-by: Ramsay Jones ram...@ramsay1.demon.co.uk Signed-off-by: Torsten Bögershausen tbo...@web.de --- This may be a start for a patch, tested under CYGWIN-32, both Windows7 and XP git-compat-util.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/git-compat-util.h b/git-compat-util.h index 400e921..cef2691 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -75,7 +75,8 @@ # endif #elif !defined(__APPLE__) !defined(__FreeBSD__) !defined(__USLC__) \ !defined(_M_UNIX) !defined(__sgi) !defined(__DragonFly__) \ - !defined(__TANDEM) !defined(__QNX__) !defined(__MirBSD__) + !defined(__TANDEM) !defined(__QNX__) !defined(__MirBSD__) \ + !defined(__CYGWIN__) #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 -- 1.9.1.dirty -- 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: [RFC 3/4] engine.pl: split the .o and .obj processing
Hi,
On Fri, 21 Nov 2014, Philip Oakley wrote:
From: Johannes Schindelin johannes.schinde...@gmx.de
On Thu, 20 Nov 2014, Philip Oakley wrote:
@@ -343,8 +346,10 @@ sub handleLinkLine
} elsif ($part =~ /\.(a|lib)$/) {
$part =~ s/\.a$/.lib/;
push(@libs, $part);
-} elsif ($part =~ /\.(o|obj)$/) {
+} elsif ($part =~ /\.o$/) { # was '/\.(o|obj)$'
push(@objfiles, $part);
+} elsif ($part =~ /\.obj$/) { # was '/\.(o|obj)$'
+# push(@objfiles, $part); # do nothing
How about the following instead?
+ } elsif ($part eq 'invalidcontinue.obj') {
+ # ignore
} elsif ($part =~ /\.(o|obj)$/) {
Looks good, I'll use that (after deciding whether .obj files should be
expected in a 'make' output anyway)
My idea to hardcode invalidcontinue.obj was that I'd rather see a failure
if an unexpected .obj is seen there. But I realize that my suggested
change does not exactly accomplish that.
Ciao,
Johannes
--
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] l10n: de.po: fix typos
From: Hartmut Henkel hartmut_hen...@gmx.de Signed-off-by: Hartmut Henkel hartmut_hen...@gmx.de Signed-off-by: Ralf Thielow ralf.thie...@gmail.com --- Junio, please apply this patch directly to your tree. Thanks! po/de.po | 18 +- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/po/de.po b/po/de.po index 56263b4..5a93ea8 100644 --- a/po/de.po +++ b/po/de.po @@ -644,11 +644,11 @@ msgstr %s: %s - %s #: lockfile.c:275 msgid BUG: reopen a lockfile that is still open -msgstr FEHLER: Wiedereröffnen einer bereits geöffneten Lock-Datei +msgstr FEHLER: Wiederöffnen einer bereits geöffneten Lock-Datei #: lockfile.c:277 msgid BUG: reopen a lockfile that has been committed -msgstr FEHLER: Wiedereröffnen einer bereits committeten Lock-Datei +msgstr FEHLER: Wiederöffnen einer bereits committeten Lock-Datei #: merge.c:41 msgid failed to read the cache @@ -1956,7 +1956,7 @@ msgstr Unbeobachtete Dateien nicht aufgelistet%s #: wt-status.c:1370 msgid (use -u option to show untracked files) -msgstr (benutzen Sie die Option -u, um unbeobachteten Dateien anzuzeigen) +msgstr (benutzen Sie die Option -u, um unbeobachtete Dateien anzuzeigen) #: wt-status.c:1376 msgid No changes @@ -2810,7 +2810,7 @@ msgstr Commits von Datei benutzen, anstatt \git-rev-list\ aufzurufen #: builtin/blame.c:2518 msgid Use file's contents as the final image -msgstr Inhalte der Dateien als entgültiges Abbild benutzen +msgstr Inhalte der Dateien als endgültiges Abbild benutzen #: builtin/blame.c:2519 builtin/blame.c:2520 msgid score @@ -3078,7 +3078,7 @@ msgstr Informationen zum Upstream-Branch ändern #: builtin/branch.c:823 msgid use colored output -msgstr farbliche Ausgaben verwenden +msgstr farbige Ausgaben verwenden #: builtin/branch.c:824 msgid act on remote-tracking branches @@ -5585,7 +5585,7 @@ msgstr Platzhalter als Python-String formatieren #: builtin/for-each-ref.c:1078 msgid quote placeholders suitably for tcl -msgstr Platzhalter als TCL-String formatieren +msgstr Platzhalter als Tcl-String formatieren #: builtin/for-each-ref.c:1081 msgid show only n matched refs @@ -6892,7 +6892,7 @@ msgstr #: builtin/ls-files.c:462 msgid show cached files in the output (default) -msgstr zwischengespeicherten Dateien in der Ausgabe anzeigen (Standard) +msgstr zwischengespeicherte Dateien in der Ausgabe anzeigen (Standard) #: builtin/ls-files.c:464 msgid show deleted files in the output @@ -8119,7 +8119,7 @@ msgstr Komprimierungsgrad für Paketierung #: builtin/pack-objects.c:2685 msgid do not hide commits by grafts -msgstr keine künstlichen Vorgänger-Commit (\grafts\) verbergen +msgstr keine künstlichen Vorgänger-Commits (\grafts\) verbergen #: builtin/pack-objects.c:2687 msgid use a bitmap index if available to speed up counting objects @@ -9695,7 +9695,7 @@ msgstr Remote-Tracking-Branches anzeigen #: builtin/show-branch.c:653 msgid color '*!+-' corresponding to the branch -msgstr '*!+-' entsprechend des Branches einfärgen +msgstr '*!+-' entsprechend des Branches einfärben #: builtin/show-branch.c:655 msgid show n more commits after the common ancestor -- 2.2.0.rc3.268.gf546f9c -- 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 RFC] CYGWIN: avoid implicit declaration warning
On 23/11/14 14:16, Torsten Bögershausen wrote: gcc under cygwin reports several warnings like this: warning: implicit declaration of function 'memmem' [-Wimplicit-function-declaration] This has been observed under CYGWIN-32 with GCC 4.7.3 as well as CYGWIN-64 with gcc v4.8.3-5 x86-64 Heh, thanks for looking into this. Your email came at a good time, since I was just about to boot my old laptop into windows XP to test my patch on 32-bit cygwin! (If I had not been watching the F1 Grand Prix on TV, I would already have done so! ;-) ). It's been a while since I updated my 32-bit cygwin installation (about 6 months) but I'm a little surprised you found this issue with gcc 4.7.3 (I'm _almost_ tempted to boot that laptop anyway just to see what versions of software it is running). Just for the reccord, my patch follows. ATB, Ramsay Jones Do not #define _XOPEN_SOURCE 600 for CYGWIN. Reported-by: Ramsay Jones ram...@ramsay1.demon.co.uk Signed-off-by: Torsten Bögershausen tbo...@web.de --- This may be a start for a patch, tested under CYGWIN-32, both Windows7 and XP git-compat-util.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/git-compat-util.h b/git-compat-util.h index 400e921..cef2691 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -75,7 +75,8 @@ # endif #elif !defined(__APPLE__) !defined(__FreeBSD__) !defined(__USLC__) \ !defined(_M_UNIX) !defined(__sgi) !defined(__DragonFly__) \ - !defined(__TANDEM) !defined(__QNX__) !defined(__MirBSD__) + !defined(__TANDEM) !defined(__QNX__) !defined(__MirBSD__) \ + !defined(__CYGWIN__) #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 -- 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] git-compat-util.h: don't define _XOPEN_SOURCE on cygwin
A recent update to the gcc compiler (v4.8.3-5 x86_64) on 64-bit cygwin leads to several new warnings about the implicit declaration of the memmem(), strlcpy() and strcasestr() functions. For example: CC archive.o archive.c: In function 'format_subst': archive.c:44:3: warning: implicit declaration of function 'memmem' \ [-Wimplicit-function-declaration] b = memmem(src, len, $Format:, 8); ^ archive.c:44:5: warning: assignment makes pointer from integer \ without a cast [enabled by default] b = memmem(src, len, $Format:, 8); ^ This seems to be caused by a change to the system headers which results in the _XOPEN_SOURCE macro now having prioity over the _GNU_SOURCE and _BSD_SOURCE macros (they are simply ignored). This in turn leads to the declarations of the above functions to be suppressed. In order to suppress the warnings, don't define the _XOPEN_SOURCE macro on cygwin Signed-off-by: Ramsay Jones ram...@ramsay1.demon.co.uk --- git-compat-util.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/git-compat-util.h b/git-compat-util.h index 400e921..cef2691 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -75,7 +75,8 @@ # endif #elif !defined(__APPLE__) !defined(__FreeBSD__) !defined(__USLC__) \ !defined(_M_UNIX) !defined(__sgi) !defined(__DragonFly__) \ - !defined(__TANDEM) !defined(__QNX__) !defined(__MirBSD__) + !defined(__TANDEM) !defined(__QNX__) !defined(__MirBSD__) \ + !defined(__CYGWIN__) #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 -- 2.1.0 -- 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: [PATCHv2] add: ignore only ignored files
Jeff King p...@peff.net writes: ... Possibly because I do not know that those instructions are written down anywhere. We usually catch such things in review these days, but there are many inconsistent spots in the existing suite. t/README has this Don't: - use '! git cmd' when you want to make sure the git command exits with failure in a controlled way by calling die(). Instead, use 'test_must_fail git cmd'. This will signal a failure if git dies in an unexpected way (e.g. segfault). On the other hand, don't use test_must_fail for running regular platform commands; just use '! cmd'. Though it can be improved by justifying just use '! cmd' further with a bit of rationale (e.g. We are not in the business of verifying that world given to us sanely works.), I think the current text is sufficient. Do we refer to t/README from CodingGuidelines where we tell the developers to always write tests to prevent other people from breaking tomorrow what you did today? If not, perhaps that is what needs to be added. -- 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: recent cygwin breakage
Ramsay Jones ram...@ramsay1.demon.co.uk writes: Just a quick heads-up on a recent cygwin breakage. I updated my (64-bit) cygwin installation yesterday and (along with many other packages) I noticed a new version of gcc (and presumably libc) was installed (gcc v4.8.3-5 x86-64). ... However, I haven't run any tests yet. Also, I would need to check this out on 32-bit cygwin (I haven't booted that laptop into Win XP for quite some time!). Hmm, I don't really know if this is an unintended side-effect of a recent change to cygwin (or a bug), but I couldn't see any mention of this on the cygwin mailing list. (I don't intend to report this to that mailing list; I don't want to subscribe to (yet another) busy list). :( Thanks. I wonder if it is safe to unconditionally drop XOPEN_SOURCE; would it cause problems for older Cygwin to those who have not updated to the recent one yet? The proposed change looks trivially correct otherwise. -- 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: GIT: [PATCH] exec_cmd: system_path memory leak fix
0xAX kuleshovm...@gmail.com writes: Hello All, I found memory leak at exec_cmd.c in system_path function with valgrind. After this patch valgrind doesn't show any memory leaks, but I'm not sure that it is the best way to solve this problem. There are a couple of places that uses system_path, if this way will be good, i'll make another patches to fix this leak. Waiting for feedback. It is a bit sad that the callers of the function are prepared for the returned value to be volatile (that is why the ones that want to do something else between the time they call it and the time they use the value returned do make copies), while other callers that immediately use the returned value do not have to be worried about freeing it, but with the change you have to force everybody to remember freeing the returned value. If you instead limited to your change only to these two points: - make struct strbuf d static; - return d.buf instead of the result of strbuf_detach(d) the updated system_path() will keep the promise all the caller expects from it, i.e. the value out of the function is volatile but the callers do not have to free it, in all cases, no? Signed-off-by: Alex Kuleshov kuleshovm...@gmail.com Please have that S-o-b: line (and the same name in GIT_AUTHOR_NAME) on the patches that we'd use git am on, not on the cover letter ;-). Nom de guerre nobody else recognizes outside your close friends may be fun, but I feel that it is not quite appropriate to appear in git shortlog -s output on a public project like ours. Thanks. -- 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] exec_cmd: system_path memory leak fix
0xAX kuleshovm...@gmail.com writes:
Signed-off-by: 0xAX kuleshovm...@gmail.com
---
The comment on names I've already mentioned elsewhere.
You need a better explanation than a no log message, as you are
not doing system-path memory leak fix.
You are doing a lot more. Perhaps the story would start like this:
system_path(): make the callers own the returned string
The function sometimes returns a newly allocated string and
sometimes returns a borrowed string, the latter of which the
callers must not free().
The existing callers all assume that the return value belongs to
the callee and most of them copy it with strdup() when they want
to keep it around. They end up leaking the returned copy when
the callee returned a new string.
Change the contract between the callers and system_path() to
make the returned string owned by the callers; they are
responsible for freeing it when done, but they do not have to
make their own copy to store it away.
This accidentally fixes some unsafe callers as well. For
example, ...
exec_cmd.c | 25 -
exec_cmd.h | 4 ++--
git.c | 12 +---
Even though I said that this changes the contract between the caller
and the callee and make things wasteful for some, I personally think
it is going in the right direction.
The change accidentally fixes some unsafe callers. For example, the
first hit from git grep system_path is this:
attr.c- static const char *system_wide;
attr.c- if (!system_wide)
attr.c: system_wide = system_path(ETC_GITATTRIBUTES);
attr.c- return system_wide;
This is obviously unsafe for a volatile return value from the callee
and needs to have strdup() on it, but with the patch there no longer
is need for such a caller-side strdup().
But this change also introduces new bugs, I think. For example, the
second hit from git grep system_path is this:
builtin/help.c: strbuf_addstr(new_path, system_path(GIT_MAN_PATH));
Now the caller owns and is responsible for freeing the returned
value, but without opening the file in question in an editor or a
pager we can tell immediately that there is no way this line is not
adding a new memory leak.
index 698e752..08f8f80 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -6,7 +6,7 @@
static const char *argv_exec_path;
static const char *argv0_path;
-const char *system_path(const char *path)
+char *system_path(const char *path)
{
#ifdef RUNTIME_PREFIX
static const char *prefix;
@@ -14,9 +14,10 @@ const char *system_path(const char *path)
static const char *prefix = PREFIX;
#endif
struct strbuf d = STRBUF_INIT;
+ char *new_path = NULL;
if (is_absolute_path(path))
- return path;
+ return strdup(path);
#ifdef RUNTIME_PREFIX
assert(argv0_path);
@@ -32,10 +33,13 @@ const char *system_path(const char *path)
Using static fallback '%s'.\n, prefix);
}
#endif
-
strbuf_addf(d, %s/%s, prefix, path);
- path = strbuf_detach(d, NULL);
- return path;
+ new_path = malloc((strlen(prefix) + strlen(path)) + 2);
+ sprintf(new_path, %s/%s, prefix, path);
+
+ strbuf_release(d);
+
+ return new_path;
Are you duplicating what strbuf_addf() is doing on the strbuf d,
manually creating the same in new_path, while discarding what the
existing code you did not remove with this patch already computed?
Isn't it sufficient to add strdup(path) for the absolute case and do
nothing else to this function? I have no idea what you are doing
here.
--
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 RFC] CYGWIN: avoid implicit declaration warning
Ramsay Jones ram...@ramsay1.demon.co.uk writes: On 23/11/14 14:16, Torsten Bögershausen wrote: gcc under cygwin reports several warnings like this: warning: implicit declaration of function 'memmem' [-Wimplicit-function-declaration] This has been observed under CYGWIN-32 with GCC 4.7.3 as well as CYGWIN-64 with gcc v4.8.3-5 x86-64 Heh, thanks for looking into this. Your email came at a good time, since I was just about to boot my old laptop into windows XP to test my patch on 32-bit cygwin! (If I had not been watching the F1 Grand Prix on TV, I would already have done so! ;-) ). It's been a while since I updated my 32-bit cygwin installation (about 6 months) but I'm a little surprised you found this issue with gcc 4.7.3 (I'm _almost_ tempted to boot that laptop anyway just to see what versions of software it is running). So you have an old installation to check how well the patched version is accepted by the old set of header files? Just for the reccord, my patch follows. ATB, Ramsay Jones Do not #define _XOPEN_SOURCE 600 for CYGWIN. Reported-by: Ramsay Jones ram...@ramsay1.demon.co.uk Signed-off-by: Torsten Bögershausen tbo...@web.de --- This may be a start for a patch, tested under CYGWIN-32, both Windows7 and XP git-compat-util.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/git-compat-util.h b/git-compat-util.h index 400e921..cef2691 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -75,7 +75,8 @@ # endif #elif !defined(__APPLE__) !defined(__FreeBSD__) !defined(__USLC__) \ !defined(_M_UNIX) !defined(__sgi) !defined(__DragonFly__) \ - !defined(__TANDEM) !defined(__QNX__) !defined(__MirBSD__) + !defined(__TANDEM) !defined(__QNX__) !defined(__MirBSD__) \ + !defined(__CYGWIN__) #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 -- 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: Doing a git add '' will add more files then expected
IMO, if you put an empty string you would be implying the same as with a dot (git add . ). The important thing is that git add without a pathspec returns an error, as it has always done, mainly because it people tend to work without gitignoring all the files they should, and allowing such behaviour would make things harder. -- Javier Domingo Cansino -- 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] exec_cmd: system_path memory leak fix
Junio C Hamano gits...@pobox.com @ 2014-11-24 00:51 ALMT:
0xAX kuleshovm...@gmail.com writes:
Signed-off-by: 0xAX kuleshovm...@gmail.com
---
The comment on names I've already mentioned elsewhere.
Yes, i understand about names.
You need a better explanation than a no log message, as you are
not doing system-path memory leak fix.
You are doing a lot more. Perhaps the story would start like this:
system_path(): make the callers own the returned string
Did it.
The function sometimes returns a newly allocated string and
sometimes returns a borrowed string, the latter of which the
callers must not free().
The existing callers all assume that the return value belongs to
the callee and most of them copy it with strdup() when they want
to keep it around. They end up leaking the returned copy when
the callee returned a new string.
Change the contract between the callers and system_path() to
make the returned string owned by the callers; they are
responsible for freeing it when done, but they do not have to
make their own copy to store it away.
Yes you're right, i just started to read git source code some days ago,
and it's hard to understand in some places for the start. Now i see it,
thanks for explanation.
This accidentally fixes some unsafe callers as well. For
example, ...
exec_cmd.c | 25 -
exec_cmd.h | 4 ++--
git.c | 12 +---
Even though I said that this changes the contract between the caller
and the callee and make things wasteful for some, I personally think
it is going in the right direction.
The change accidentally fixes some unsafe callers. For example, the
first hit from git grep system_path is this:
attr.c- static const char *system_wide;
attr.c- if (!system_wide)
attr.c: system_wide = system_path(ETC_GITATTRIBUTES);
attr.c- return system_wide;
This is obviously unsafe for a volatile return value from the callee
and needs to have strdup() on it, but with the patch there no longer
is need for such a caller-side strdup().
But this change also introduces new bugs, I think. For example, the
second hit from git grep system_path is this:
builtin/help.c: strbuf_addstr(new_path, system_path(GIT_MAN_PATH));
Now the caller owns and is responsible for freeing the returned
value, but without opening the file in question in an editor or a
pager we can tell immediately that there is no way this line is not
adding a new memory leak.
index 698e752..08f8f80 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -6,7 +6,7 @@
static const char *argv_exec_path;
static const char *argv0_path;
-const char *system_path(const char *path)
+char *system_path(const char *path)
{
#ifdef RUNTIME_PREFIX
static const char *prefix;
@@ -14,9 +14,10 @@ const char *system_path(const char *path)
static const char *prefix = PREFIX;
#endif
struct strbuf d = STRBUF_INIT;
+char *new_path = NULL;
if (is_absolute_path(path))
-return path;
+return strdup(path);
#ifdef RUNTIME_PREFIX
assert(argv0_path);
@@ -32,10 +33,13 @@ const char *system_path(const char *path)
Using static fallback '%s'.\n, prefix);
}
#endif
-
strbuf_addf(d, %s/%s, prefix, path);
-path = strbuf_detach(d, NULL);
-return path;
+new_path = malloc((strlen(prefix) + strlen(path)) + 2);
+sprintf(new_path, %s/%s, prefix, path);
+
+strbuf_release(d);
+
+return new_path;
Are you duplicating what strbuf_addf() is doing on the strbuf d,
manually creating the same in new_path, while discarding what the
existing code you did not remove with this patch already computed?
Isn't it sufficient to add strdup(path) for the absolute case and do
nothing else to this function? I have no idea what you are doing
here.
I have added changes from your previous feedback, how can I attach
second (changed) patch to this mail thread with git send-email?
--
Best regards.
0xAX
--
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] exec_cmd: system_path memory leak fix
Signed-off-by: Alex Kuleshov kuleshovm...@gmail.com
---
exec_cmd.c | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/exec_cmd.c b/exec_cmd.c
index 698e752..7ed9bcc 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -13,7 +13,7 @@ const char *system_path(const char *path)
#else
static const char *prefix = PREFIX;
#endif
- struct strbuf d = STRBUF_INIT;
+ static struct strbuf d = STRBUF_INIT;
if (is_absolute_path(path))
return path;
@@ -34,8 +34,7 @@ const char *system_path(const char *path)
#endif
strbuf_addf(d, %s/%s, prefix, path);
- path = strbuf_detach(d, NULL);
- return path;
+ return d.buf;
}
const char *git_extract_argv0_path(const char *argv0)
@@ -94,7 +93,7 @@ static void add_path(struct strbuf *out, const char *path)
void setup_path(void)
{
const char *old_path = getenv(PATH);
- struct strbuf new_path = STRBUF_INIT;
+ static struct strbuf new_path = STRBUF_INIT;
add_path(new_path, git_exec_path());
add_path(new_path, argv0_path);
--
2.2.0.rc3.190.g952f0aa.dirty
--
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: Fwd: Add git ignore as builtin
I would love to have such tool included in the toolchain, but being able to use it to edit all the ignore chain, defaulting to .gitignore. Explain the reason in my case. Usually, when ignoring stuff, you will probable ignore already your IDE/Editor files using a global gitignore. And most of the times in a per-project basis, you will be ignoring their output files. I only use .git/info/exclude when I have something really special that I don't want to share publicly, such as a data/ folder to run the project or so. That way, most of the times I will be modifying .gitignore, sometimes my global gitignore and very occasionally, .git/info/exclude. That's my case, and that I know of, people have that usage order, .gitignore global gitignore local gitignore. For sake of uniformity, I would use the same context specifiers as in git-config. Defaulting to --repo for .gitignore, using --local for the .git/info/exclue, using --global for the global gitignore, and --system for the system one. Also, about adding and excluding, I would recommend using verbs instead of arguments, which would be in consonance with git remote. git ignore exclude git ignore include You could also make it smart by allowing to use it as the Cisco managing commands, or the ip tool (ip a == ip address, ip a a == ip addr add, etc.), resulting in the following: git ignore e git ignore i -- Javier Domingo Cansino -- 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] exec_cmd: system_path memory leak fix
On Mon, Nov 24, 2014 at 01:19:29AM +0600, Alex Kuleshov wrote: Signed-off-by: Alex Kuleshov kuleshovm...@gmail.com --- exec_cmd.c | 7 +++ 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/exec_cmd.c b/exec_cmd.c index 698e752..7ed9bcc 100644 --- a/exec_cmd.c +++ b/exec_cmd.c @@ -13,7 +13,7 @@ const char *system_path(const char *path) #else static const char *prefix = PREFIX; #endif - struct strbuf d = STRBUF_INIT; + static struct strbuf d = STRBUF_INIT; if (is_absolute_path(path)) return path; @@ -34,8 +34,7 @@ const char *system_path(const char *path) #endif strbuf_addf(d, %s/%s, prefix, path); - path = strbuf_detach(d, NULL); - return path; + return d.buf; } If I am reading this right, calls to system_path() will always reuse the same buffer, even if they are called with another path argument. So all callers must make sure to make a copy if they are going to hold on to it for a long time. Grepping for callers shows us saving the result to a static variable in at least git_etc_gitattributes, copy_templates, and get_html_page_path. Don't these all need to learn to xstrdup the return value? -Peff -- 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: [PATCHv2] add: ignore only ignored files
On Sun, Nov 23, 2014 at 10:10:47AM -0800, Junio C Hamano wrote: Jeff King p...@peff.net writes: ... Possibly because I do not know that those instructions are written down anywhere. We usually catch such things in review these days, but there are many inconsistent spots in the existing suite. t/README has this Don't: - use '! git cmd' when you want to make sure the git command exits with failure in a controlled way by calling die(). Instead, use 'test_must_fail git cmd'. This will signal a failure if git dies in an unexpected way (e.g. segfault). On the other hand, don't use test_must_fail for running regular platform commands; just use '! cmd'. Thanks, I did not actually look and relied on my memory, which was obviously wrong. I agree that the instructions there are sufficient. Do we refer to t/README from CodingGuidelines where we tell the developers to always write tests to prevent other people from breaking tomorrow what you did today? If not, perhaps that is what needs to be added. That might make sense. It might also be that Torsten simply overlooked it when asking his question (i.e., there is nothing to fix, documentation is not always read completely, and we can move on). -Peff -- 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: [PATCHv2] add: ignore only ignored files
On Sat, Nov 22, 2014 at 10:20:10PM +0100, Torsten Bögershausen wrote: I do not think there is a real _downside_ to using test_must_fail for grep, except that it is a bit more verbose. We may burn CPU cycles It adds a single if/else chain. If your shell does not implement that as a fast builtin, you have bigger performance problems. :) I counted 19 test_must_fail grep under t/*sh, and 201 ! grep. I do not mind a patch to fix them, but with the usual caveat of avoiding stepping on the toes of any topics in flight. It is also fine to leave them until the area is touched. As a general rule for further review of shell scripts can we say ? ! git# incorrect, we don't capture e.g. segfaults of signal test_must_fail grep # correct, but not preferred for new code ! grep # preferred for new code test_must_fail git # correct I think that's true. The snippet from t/README Junio quoted lays it out pretty clearly, I think. If you didn't know there was documentation in t/README that was worth reading before writing tests, then that is the thing I think should go in CodingGuidelines. -Peff -- 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] exec_cmd: system_path memory leak fix
On Sun, Nov 23, 2014 at 2:19 PM, Alex Kuleshov kuleshovm...@gmail.com wrote:
Signed-off-by: Alex Kuleshov kuleshovm...@gmail.com
---
exec_cmd.c | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/exec_cmd.c b/exec_cmd.c
index 698e752..7ed9bcc 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -13,7 +13,7 @@ const char *system_path(const char *path)
#else
static const char *prefix = PREFIX;
#endif
- struct strbuf d = STRBUF_INIT;
+ static struct strbuf d = STRBUF_INIT;
Curious. Did the unit tests pass with this change?
In the original code, the strbuf starts out empty each time
system_path() is called, and strbuf_addf() populates it. After your
change, the strbuf starts empty only on the very first call, and
subsequent calls append more content rather than replacing it. At the
very least, to fix, you now need to invoke strbuf_reset() before
invoking strbuf_addf().
if (is_absolute_path(path))
return path;
@@ -34,8 +34,7 @@ const char *system_path(const char *path)
#endif
strbuf_addf(d, %s/%s, prefix, path);
- path = strbuf_detach(d, NULL);
- return path;
+ return d.buf;
}
const char *git_extract_argv0_path(const char *argv0)
@@ -94,7 +93,7 @@ static void add_path(struct strbuf *out, const char *path)
void setup_path(void)
{
const char *old_path = getenv(PATH);
- struct strbuf new_path = STRBUF_INIT;
+ static struct strbuf new_path = STRBUF_INIT;
add_path(new_path, git_exec_path());
add_path(new_path, argv0_path);
Not sure what this change is about. The last couple lines of this function are:
setenv(PATH, new_path.buf, 1);
strbuf_release(new_path);
which means that the buffer held by the strbuf is being released
anyhow, whether static or not.
--
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] exec_cmd: system_path memory leak fix
Alex Kuleshov kuleshovm...@gmail.com writes: Signed-off-by: Alex Kuleshov kuleshovm...@gmail.com So this one does not change the contract between the caller and the callee. It does not change the fact that you need to explain your change, though. The story should read something like this: system_path(): always return a volatile result The function sometimes returns a newly allocated string and sometimes returns a borrowed string, the latter of which the callers must not free(). The existing callers all assume that the return value belongs to the callee and most of them copy it with strdup() when they want to keep it around. They end up leaking the returned copy when the callee returned a new string. Make sure all callers receive a volatile result, so that they do not have to be worried about freeing the returned string. There however are existing callers that are already broken, for example, ... Fixing these callers are done as separate patches, that can be applied either before or after this patch. Personally, as I already said, I think the approach the previous version took (but not the implementation) to change the contract between the callers and this function is probably a good idea in the longer term. Leaking a bit by forgetting to convert a few callers to free the returned values will not affect the correctness, but making the returned value consistently more volatile will expose existing breakage more often and will break codepaths that happened to be working by accident. Thanks. -- 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: [msysGit] Re: [RFC 3/4] engine.pl: split the .o and .obj processing
From: Johannes Schindelin johannes.schinde...@gmx.de
How about the following instead?
+ } elsif ($part eq 'invalidcontinue.obj') {
+ # ignore
} elsif ($part =~ /\.(o|obj)$/) {
Looks good, I'll use that (after deciding whether .obj files should
be
expected in a 'make' output anyway)
My idea to hardcode invalidcontinue.obj was that I'd rather see a
failure
if an unexpected .obj is seen there. But I realize that my suggested
change does not exactly accomplish that.
I've decided to include the hard code suggested, and after a few tests
decided it was probably worth keeping the .obj processing, even though
we don't expect any.
At least I've now managed to generate a .sln project, though it won't
compile because of a number of other changes, and I haven't got to the
bottom of them all.
Notes:
Revert Windows: correct detection of EISDIR in mingw_open()
git\compat\mingw.c(315) : error C2065: 'O_ACCMODE' : undeclared
identifier (neither VS2008 nor VS2010 declare 'O_ACCMODE'). add an
#ifdef
_MSC_VER to define this if we are in MSVC.
Revert mingw.h: add dummy functions for sigset_t operations commit
4e6d207c45. the Additional Note: to
http://www.spinics.net/lists/git/msg240248.html indicates that this
breaks MSVC. I think it will also need an #ifdef to see if we are in
MSVC.
I've also found a problem with my setup (msysgit 1.7.9 for make) barfing
on the commit/ee9be06 perl: detect new files in MakeMaker builds which
'make -n' says 'No rule to make target `PM.stamp', needed by `perl.mak'.
There was also LINK : warning LNK4098: defaultlib 'MSVCRT' conflicts
with use of other libs; use /NODEFAULTLIB:library.
I've also got notes from way back that there was build order problem
with 'vcs-svn_lib.lib' - probably a sort order issue. IIRC at that time,
I had to compile the project twice so that the lib was visible the
second time.
So there's plenty to go at ;-) I'm going to be away this coming week, So
at least I have the whole next cycle to make some progress.
--
Philip
--
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: recent cygwin breakage
On 23/11/14 18:13, Junio C Hamano wrote: Ramsay Jones ram...@ramsay1.demon.co.uk writes: Just a quick heads-up on a recent cygwin breakage. I updated my (64-bit) cygwin installation yesterday and (along with many other packages) I noticed a new version of gcc (and presumably libc) was installed (gcc v4.8.3-5 x86-64). ... However, I haven't run any tests yet. Also, I would need to check this out on 32-bit cygwin (I haven't booted that laptop into Win XP for quite some time!). Hmm, I don't really know if this is an unintended side-effect of a recent change to cygwin (or a bug), but I couldn't see any mention of this on the cygwin mailing list. (I don't intend to report this to that mailing list; I don't want to subscribe to (yet another) busy list). :( Thanks. I wonder if it is safe to unconditionally drop XOPEN_SOURCE; would it cause problems for older Cygwin to those who have not updated to the recent one yet? The proposed change looks trivially correct otherwise. I honestly don't know. I have never attempted to rollback an update to cygwin. (I guess it is possible, but I really don't know how ...) However, ... ATB, Ramsay Jones -- 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 RFC] CYGWIN: avoid implicit declaration warning
On 23/11/14 18:53, Junio C Hamano wrote: Ramsay Jones ram...@ramsay1.demon.co.uk writes: On 23/11/14 14:16, Torsten Bögershausen wrote: gcc under cygwin reports several warnings like this: warning: implicit declaration of function 'memmem' [-Wimplicit-function-declaration] This has been observed under CYGWIN-32 with GCC 4.7.3 as well as CYGWIN-64 with gcc v4.8.3-5 x86-64 Heh, thanks for looking into this. Your email came at a good time, since I was just about to boot my old laptop into windows XP to test my patch on 32-bit cygwin! (If I had not been watching the F1 Grand Prix on TV, I would already have done so! ;-) ). It's been a while since I updated my 32-bit cygwin installation (about 6 months) but I'm a little surprised you found this issue with gcc 4.7.3 (I'm _almost_ tempted to boot that laptop anyway just to see what versions of software it is running). So you have an old installation to check how well the patched version is accepted by the old set of header files? ... I can, indeed, use this old installation to test this on 32-bit cygwin. sigh, I thought I had dodged that bullet! ;-) [I can't do this tonight and I'm pretty busy tomorrow, so it may have to wait until tomorrow evening at the earliest. sorry about that. :( ] This does not help much with 64-bit cygwin. They are effectively two different 'distributions'. ATB, Ramsay Jones -- 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] send-email: handle adjacent RFC 2047-encoded words properly
The RFC says that they are to be concatenated after decoding (i.e. the
intervening whitespace is ignored).
I change the sender's name to an all-Cyrillic string in the tests so that
its encoded form goes over the 76 characters in a line limit, forcing
format-patch to split it into multiple encoded words.
Since I have to modify the regular expression for an encoded word anyway,
I take the opportunity to bring it closer to the spec, most notably
disallowing embedded spaces and making it case-insensitive (thus allowing
the encoding to be specified as both q and Q).
Signed-off-by: Роман Донченко d...@corrigendum.ru
---
git-send-email.perl | 21 +++--
t/t9001-send-email.sh | 18 +-
2 files changed, 24 insertions(+), 15 deletions(-)
diff --git a/git-send-email.perl b/git-send-email.perl
index 9949db0..4bb9f6f 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -913,13 +913,22 @@ $time = time - scalar $#files;
sub unquote_rfc2047 {
local ($_) = @_;
+
+ my $et = qr/[!-@-~]+/; # encoded-text from RFC 2047
+ my $sep = qr/[ \t]+/;
+ my $encoded_word = qr/=\?($et)\?q\?($et)\?=/i;
+
my $encoding;
- s{=\?([^?]+)\?q\?(.*?)\?=}{
- $encoding = $1;
- my $e = $2;
- $e =~ s/_/ /g;
- $e =~ s/=([0-9A-F]{2})/chr(hex($1))/eg;
- $e;
+ s{$encoded_word(?:$sep$encoded_word)+}{
+ my @words = split $sep, $;
+ foreach (@words) {
+ m/$encoded_word/;
+ $encoding = $1;
+ $_ = $2;
+ s/_/ /g;
+ s/=([0-9A-F]{2})/chr(hex($1))/eg;
+ }
+ join '', @words;
}eg;
return wantarray ? ($_, $encoding) : $_;
}
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 19a3ced..318b870 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -236,7 +236,7 @@ test_expect_success $PREREQ 'self name with dot is
suppressed'
test_expect_success $PREREQ 'non-ascii self name is suppressed'
- test_suppress_self_quoted 'Füñný Nâmé' 'odd_?=m...@example.com' \
+ test_suppress_self_quoted 'Кириллическое Имя' 'odd_?=m...@example.com' \
'non_ascii_self_suppressed'
@@ -946,25 +946,25 @@ test_expect_success $PREREQ 'utf8 author is correctly
passed on' '
clean_fake_sendmail
test_commit weird_author
test_when_finished git reset --hard HEAD^
- git commit --amend --author Füñný Nâmé odd_?=m...@example.com
- git format-patch --stdout -1 funny_name.patch
+ git commit --amend --author Кириллическое Имя
odd_?=m...@example.com
+ git format-patch --stdout -1 nonascii_name.patch
git send-email --from=Example nob...@example.com \
--to=nob...@example.com \
--smtp-server=$(pwd)/fake.sendmail \
- funny_name.patch
- grep ^From: Füñný Nâmé odd_?=m...@example.com msgtxt1
+ nonascii_name.patch
+ grep ^From: Кириллическое Имя odd_?=m...@example.com msgtxt1
'
test_expect_success $PREREQ 'utf8 sender is not duplicated' '
clean_fake_sendmail
test_commit weird_sender
test_when_finished git reset --hard HEAD^
- git commit --amend --author Füñný Nâmé odd_?=m...@example.com
- git format-patch --stdout -1 funny_name.patch
- git send-email --from=Füñný Nâmé odd_?=m...@example.com \
+ git commit --amend --author Кириллическое Имя
odd_?=m...@example.com
+ git format-patch --stdout -1 nonascii_name.patch
+ git send-email --from=Кириллическое Имя odd_?=m...@example.com \
--to=nob...@example.com \
--smtp-server=$(pwd)/fake.sendmail \
- funny_name.patch
+ nonascii_name.patch
grep ^From: msgtxt1 msgfrom
test_line_count = 1 msgfrom
'
--
2.1.1
--
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: How safe are signed git tags? Only as safe as SHA-1 or somehow safer?
Hi, I wanted to chime in on the topic of SHA1 weaknesses and breaks. The problem is idea that SHA1 breaks are theoretical and will only be relevant in a decade or two. I think its a telling sign when even companies like Google [1] and Microsoft [2] who collaborate with spy agencies are moving away from SHA1 in verifying browser certs and the estimates by reputable cryptographers already put us in the realm of feasible breaks at this time, with the bar going lower with every passing year [3]. In three years common cyber criminals will be able to crack it using moderate sized computer clusters or by renting some AWS cycles. Please reconsider the urgency of moving away from SHA1 for security functions in Git. References: [1] http://thenextweb.com/google/2014/09/05/google-will-start-sunsetting-sha-1-cryptographic-hash-algorithm-chrome-month-finish-q1-2015/ [2] https://www.schneier.com/blog/archives/2013/11/microsoft_retir.html (Schneier on Security: Microsoft Retiring SHA-1 in 2016) [3] https://www.schneier.com/blog/archives/2012/10/when_will_we_se.html (When Will We See Collisions for SHA-1?) -- 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: How safe are signed git tags? Only as safe as SHA-1 or somehow safer?
On Tue, Nov 18, 2014 at 4:26 AM, Jeff King p...@peff.net wrote: Yes, it is only as safe as SHA-1 in the sense that you have GPG-signed only a SHA-1 hash. If somebody can find a collision with a hash you have signed, they can substitute the colliding data for the data you signed. I wonder if we can have an option to sign all blob content of the tree associated to a commit, and the content of parent commit(s). It's more expensive than signing just commit/tag content. But it's also safer without completely ditching SHA-1. -- Duy -- 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
K'itchen Offers Reviews
Best k'itchens offers reviews i could give are that you don’t pay until the k'itchen is delivered. - Kitchen Offers Reviews -- View this message in context: http://git.661346.n2.nabble.com/K-itchen-Offers-Reviews-tp7621702.html Sent from the git mailing list archive at Nabble.com. -- 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] exec_cmd: system_path memory leak fix
Jeff King: If I am reading this right, calls to system_path() will always reuse the same buffer, even if they are called with another path argument. So all callers must make sure to make a copy if they are going to hold on to it for a long time. Grepping for callers shows us saving the result to a static variable in at least git_etc_gitattributes, copy_templates, and get_html_page_path. Don't these all need to learn to xstrdup the return value? Hello Jeff, yes as i wrote in previous message i saw that there some places uses system_path function and I just wanted to find correct way to solve problem with system_path, in near time i'll check and adapt if need all of this places for updated system_path. Eric Sunshine: Curious. Did the unit tests pass with this change? Yes i launched unit tests and there are the same result as in origin/pu. Not sure what this change is about. The last couple lines of this function are: setenv(PATH, new_path.buf, 1); strbuf_release(new_path); which means that the buffer held by the strbuf is being released anyhow, whether static or not. Ah, yes, just a type, will update it. Junio C Hamano: Fixing these callers are done as separate patches, that can be applied either before or after this patch. How to do it better? Update this patch, fix all callers which broken and concat this patches to one or make separate patches? Thanks all for feedback. -- Best regards. 0xAX -- 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 RFC] CYGWIN: avoid implicit declaration warning
On 2014-11-24 00.15, Ramsay Jones wrote: On 23/11/14 18:53, Junio C Hamano wrote: Ramsay Jones ram...@ramsay1.demon.co.uk writes: On 23/11/14 14:16, Torsten Bögershausen wrote: gcc under cygwin reports several warnings like this: warning: implicit declaration of function 'memmem' [-Wimplicit-function-declaration] This has been observed under CYGWIN-32 with GCC 4.7.3 as well as CYGWIN-64 with gcc v4.8.3-5 x86-64 Heh, thanks for looking into this. Your email came at a good time, since I was just about to boot my old laptop into windows XP to test my patch on 32-bit cygwin! (If I had not been watching the F1 Grand Prix on TV, I would already have done so! ;-) ). It's been a while since I updated my 32-bit cygwin installation (about 6 months) but I'm a little surprised you found this issue with gcc 4.7.3 (I'm _almost_ tempted to boot that laptop anyway just to see what versions of software it is running). So you have an old installation to check how well the patched version is accepted by the old set of header files? ... I can, indeed, use this old installation to test this on 32-bit cygwin. sigh, I thought I had dodged that bullet! ;-) It depends what we mean with old: cygwin 1.5 is old, and I lost my test installation this summer: One netbook was converted from XP to Linux, the other machine needs to be re-installed and CYGWIN 1.5 is no longer available for download. I can confirm that Ramsays patch works with CYGWIN 1.7 32 Bit. -- 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] send-email: handle adjacent RFC 2047-encoded words properly
On Sun, Nov 23, 2014 at 3:50 PM, Роман Донченко d...@corrigendum.ru wrote:
The RFC says that they are to be concatenated after decoding (i.e. the
intervening whitespace is ignored).
I change the sender's name to an all-Cyrillic string in the tests so that
its encoded form goes over the 76 characters in a line limit, forcing
format-patch to split it into multiple encoded words.
Since I have to modify the regular expression for an encoded word anyway,
I take the opportunity to bring it closer to the spec, most notably
disallowing embedded spaces and making it case-insensitive (thus allowing
the encoding to be specified as both q and Q).
Signed-off-by: Роман Донченко d...@corrigendum.ru
This sounds like a worthy thing to do in general.
I wonder if the C implementation we have for mailinfo needs similar
update, though. I vaguely recall that we have case-insensitive start for
q/b segments, but do not remember the details offhand.
Was the change to the test to use Cyrillic really necessary, or did it
suffice if you simply extended the existsing Funny Name spelled with
strange accents, but you substituted the whole string anyway?
Until I found out what the new string says by running web-based
translation on it, I felt somewhat uneasy. As I do not read
Cyrillic/Russian, we may have been adding some profanity without
knowing. It turns out that the string just says Cyrillic Name, so I am
not against using the new string, but it simply looked odd to replace the
string whole-sale when you merely need a longer string. It made it look
as if a bug was specific to Cyrillic when it wasn't.
As you may notice by reading git log --no-merges from recent history,
we tend not to say I did X, I did Y. If the tone of the above message
were more similar to them, it may have been easier to read.
But other than these minor nits, the change looks good from
a cursory read.
Thanks.
---
git-send-email.perl | 21 +++--
t/t9001-send-email.sh | 18 +-
2 files changed, 24 insertions(+), 15 deletions(-)
diff --git a/git-send-email.perl b/git-send-email.perl
index 9949db0..4bb9f6f 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -913,13 +913,22 @@ $time = time - scalar $#files;
sub unquote_rfc2047 {
local ($_) = @_;
+
+ my $et = qr/[!-@-~]+/; # encoded-text from RFC 2047
+ my $sep = qr/[ \t]+/;
+ my $encoded_word = qr/=\?($et)\?q\?($et)\?=/i;
+
my $encoding;
- s{=\?([^?]+)\?q\?(.*?)\?=}{
- $encoding = $1;
- my $e = $2;
- $e =~ s/_/ /g;
- $e =~ s/=([0-9A-F]{2})/chr(hex($1))/eg;
- $e;
+ s{$encoded_word(?:$sep$encoded_word)+}{
+ my @words = split $sep, $;
+ foreach (@words) {
+ m/$encoded_word/;
+ $encoding = $1;
+ $_ = $2;
+ s/_/ /g;
+ s/=([0-9A-F]{2})/chr(hex($1))/eg;
+ }
+ join '', @words;
}eg;
return wantarray ? ($_, $encoding) : $_;
}
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 19a3ced..318b870 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -236,7 +236,7 @@ test_expect_success $PREREQ 'self name with dot is
suppressed'
test_expect_success $PREREQ 'non-ascii self name is suppressed'
- test_suppress_self_quoted 'Füñný Nâmé' 'odd_?=m...@example.com' \
+ test_suppress_self_quoted 'Кириллическое Имя'
'odd_?=m...@example.com' \
'non_ascii_self_suppressed'
@@ -946,25 +946,25 @@ test_expect_success $PREREQ 'utf8 author is correctly
passed on' '
clean_fake_sendmail
test_commit weird_author
test_when_finished git reset --hard HEAD^
- git commit --amend --author Füñný Nâmé odd_?=m...@example.com
- git format-patch --stdout -1 funny_name.patch
+ git commit --amend --author Кириллическое Имя
odd_?=m...@example.com
+ git format-patch --stdout -1 nonascii_name.patch
git send-email --from=Example nob...@example.com \
--to=nob...@example.com \
--smtp-server=$(pwd)/fake.sendmail \
- funny_name.patch
- grep ^From: Füñný Nâmé odd_?=m...@example.com msgtxt1
+ nonascii_name.patch
+ grep ^From: Кириллическое Имя odd_?=m...@example.com msgtxt1
'
test_expect_success $PREREQ 'utf8 sender is not duplicated' '
clean_fake_sendmail
test_commit weird_sender
test_when_finished git reset --hard HEAD^
- git commit --amend --author Füñný Nâmé odd_?=m...@example.com
- git format-patch --stdout -1 funny_name.patch
- git send-email --from=Füñný Nâmé odd_?=m...@example.com \
+ git commit --amend --author Кириллическое Имя
odd_?=m...@example.com
+ git format-patch --stdout -1
Re: [PATCH] exec_cmd: system_path memory leak fix
[jc: added those who were mentioned but were missing back to Cc] On Sun, Nov 23, 2014 at 11:02 PM, Alex Kuleshov kuleshovm...@gmail.com wrote: Junio C Hamano: Fixing these callers are done as separate patches, that can be applied either before or after this patch. How to do it better? Update this patch, fix all callers which broken and concat this patches to one or make separate patches? As I said, I do not think the approach your v2 takes is better than the original approach to pass the ownership of the returned value to the caller. I'd say that a cleaned up v1 that makes sure it adds a necessary strdup() in the codepath where it returns an absolute pathname given as-is, with necessary changes to callers that do not currently free the received result to free it when they are done, and to callers that currently do strdup() of the received result not to do strdup(), in a single patch, would be the right thing to do. I think I already wrote the bulk of proposed commit message for you for such a change earlier ;-) The one that talks about changing the contract between the system_path() and its callers. Thanks. -- 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
