* Re: [PATCH 1/2] help.c: Add support for htmldir relative to git_exec_path()
From: Johannes Sixt @ 2008-07-11 9:02 UTC (permalink / raw)
To: Steffen Prohaska; +Cc: Junio C Hamano, Git Mailing List, Johannes Schindelin
In-Reply-To: <4668B2FF-2B2B-4221-8151-F0AEA681983C@zib.de>
Zitat von Steffen Prohaska <prohaska@zib.de>:
>
> On Jul 4, 2008, at 2:35 PM, Johannes Schindelin wrote:
>
> > On Fri, 4 Jul 2008, Junio C Hamano wrote:
> >
> >> Could you check if there are copy-and-pasted duplicated code you can
> >> factor out before continuing this direction?
> >
> > Note also that Hannes tried very hard to get rid of those ugly "#ifdef
> > __MINGW32__"s by declaring/overriding functions in git-compat-util.h.
> >
> > I think that is such a good practice that we should not stop here.
>
> I'll send three patches that address Junio's and Dscho's comments:
>
> [PATCH 1/3] Move code interpreting path relative to exec-dir to new
> function system_path()
> [PATCH 2/3] help.c: Add support for htmldir relative to
> git_exec_path()
> [PATCH 3/3] help (Windows): Display HTML in default browser using
> Windows' shell API
>
>
> Hannes,
> the patches I'll send probably conflict with your planned work on
> GIT_EXEC_PATH that has been discussed on the msysgit list. I think
> you could built on my series and modify system_path().
Thanks. I haven't done a lot in that direction, yet, so your patches will be
helpful.
But according to the conclusion of our recent discussion
http://thread.gmane.org/gmane.comp.version-control.msysgit/2633/focus=2669
I shall modify system_path() to construct paths relative to the git executable,
which is essentially Makefile's $(bindir), not git_exec_path().
-- Hannes
^ permalink raw reply
* Re: Ext4 patchqueue corrupted ?
From: Petr Baudis @ 2008-07-11 8:47 UTC (permalink / raw)
To: Aneesh Kumar K.V
Cc: Theodore Tso, Ming Ming Cao, linux-ext4@vger.kernel.org, git
In-Reply-To: <20080711034606.GA779@skywalker>
Hi,
On Fri, Jul 11, 2008 at 09:16:06AM +0530, Aneesh Kumar K.V wrote:
> [master@ext4-patch-queue]$ git pull
> warning: no common commits
> remote: Total 0 (delta 0), reused 0 (delta 0)
> error: unable to find ec90e41198ede71d186bb7075e2edea7a6daae5a
> fatal: object ec90e41198ede71d186bb7075e2edea7a6daae5a not found
>
> kvaneesh@skywalker:/tmp$git clone git://repo.or.cz/ext4-patch-queue.git
> Initialized empty Git repository in /tmp/ext4-patch-queue/.git/
> remote: Total 0 (delta 0), reused 0 (delta 0)
> remote: aborting due to possible repository corruption on the remote side.
> error: waitpid (async) failed
> fatal: error in sideband demultiplexer
>
> The web interface doesn't list any commits.
very puzzling; I have backed up the broken objects store, can you
repush, please? I'm curious about how this could have happenned;
repo.or.cz now uses Git from latest next, which is a rather strange
coincidence. ;-) Or did any of the pushers do anything special about
pushin to the repository recently?
Huge amount of objects is missing, the repository has only few loose
objects and
-rw-rw-r-- 1 repo 65657 1408 2008-07-10 18:56 pack-503b8e3e6f3a650c8bc37df88e8a9d7b6193c900.idx
-rw-rw-r-- 1 repo 65657 3364 2008-07-10 18:56 pack-503b8e3e6f3a650c8bc37df88e8a9d7b6193c900.pack
(By the way, how do I list objects in a pack again? I keep forgetting
the trick...)
--
Petr "Pasky" Baudis
The last good thing written in C++ was the Pachelbel Canon. -- J. Olson
^ permalink raw reply
* Re: [JGIT PATCH 1/1] jgit: create a tag command
From: Mike Ralphson @ 2008-07-11 8:45 UTC (permalink / raw)
To: Shawn O. Pearce; +Cc: Robin Rosenberg, Marek Zawirski, git
In-Reply-To: <20080711020507.GD31862@spearce.org>
2008/7/11 Shawn O. Pearce <spearce@spearce.org>:
> Mike Ralphson <mike.ralphson@gmail.com> wrote:
>>
>> Loving the make_jgit stuff.
>
> So making jgit a single stand-alone, portable shell script for
> command line usage was a good idea? ;-)
It certainly seems so to me. It's a nice quick way of seeing what's
implemented (I was toying with adding a jgit help command which would
reflect over the TextBuiltins).
I'm not sure which if any platforms would eventually be better off
with a commandline jgit than trying to port c-git though.
> I think we are at the point where we need to either write a
> #@!*(!@(! command line option parser, import one, or stop writing
> command line programs. I would certainly appreciate any opinion
> you might have on the matter.
a) is a distraction, c) is a backwards step, so maybe b) wins.
I don't know what the state of the art of Java option parsers is but
there is a port of GNU getopt [1] which might drop in quite easily.
Mike
PS apologies for the patch format, I'm stuck with Outlook or gmail,
and a recalcitrant firewall for the moment.
[1] http://www.urbanophile.com/arenn/hacking/download.html
^ permalink raw reply
* Re: [JGIT PATCH 2/5] Don't display passwords on the console in fetch/push output
From: Robin Rosenberg @ 2008-07-11 8:30 UTC (permalink / raw)
To: Shawn O. Pearce; +Cc: Marek Zawirski, git
In-Reply-To: <20080711032004.GC32319@spearce.org>
fredagen den 11 juli 2008 05.20.04 skrev Shawn O. Pearce:
> + public String toPrivateString() {
> + return format(true);
> + }
etc
Yes, that's much better.
-- robin
^ permalink raw reply
* Re: [JGIT PATCH 1/1] jgit: create a tag command
From: Robin Rosenberg @ 2008-07-11 8:28 UTC (permalink / raw)
To: Shawn O. Pearce; +Cc: Mike Ralphson, Marek Zawirski, git
In-Reply-To: <20080711020507.GD31862@spearce.org>
fredagen den 11 juli 2008 04.05.07 skrev Shawn O. Pearce:
> Mike Ralphson <mike.ralphson@gmail.com> wrote:
> >
> > Loving the make_jgit stuff.
>
> So making jgit a single stand-alone, portable shell script for
> command line usage was a good idea? ;-)
>
> I think we are at the point where we need to either write a
> #@!*(!@(! command line option parser, import one, or stop writing
> command line programs. I would certainly appreciate any opinion
> you might have on the matter.
I've come to that conclusion too, but not yet to the code :)
-- robin
^ permalink raw reply
* Should we discuss Windows-related changes on git@vger.kernel.org?
From: Steffen Prohaska @ 2008-07-11 8:07 UTC (permalink / raw)
To: Johannes Sixt, Junio C Hamano
Cc: msysGit, Git Mailing List, Johannes Schindelin
I decided to stop queuing changes in 4msysgit. Instead I'd like to
bring the diff between Junio's and 4msysgit's master to zero. This
seems to be achievable after Junio merged Hannes' MinGW changes.
I think all Windows-related changes to the git code base could be
discussed directly on the main git list and the msysgit list would be
reserved for the MinGW/MSYS runtime environment and the installer. I do
not expect that Windows-related changes would add too much traffic to
the list. But, even though we have mechanisms to separate the
platform-specific code from the main code base (like git-compat-util.h),
the Windows-related changes eventually need to be integrated with the
mainline, so it might make sense to discuss them on the main list right
away.
What do you think?
Steffen
^ permalink raw reply
* Re: THREADED_DELTA_SEARCH
From: Pierre Habouzit @ 2008-07-11 7:41 UTC (permalink / raw)
To: Alex Riesen; +Cc: Nicolas Pitre, Johannes Schindelin, Junio C Hamano, Git ML
In-Reply-To: <81b0412b0807102259q1046051bwc513eadb50847675@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 887 bytes --]
On Fri, Jul 11, 2008 at 05:59:56AM +0000, Alex Riesen wrote:
> 2008/7/11 Nicolas Pitre <nico@cam.org>:
> > Furthermore, the number of threads is, by default, equivalent to the
> > number of detected CPUs. So if you have a 8-CPU machine, I sure hope
> > you have the RAM configuration that goes along with such a system.
>
> ....and while the threads limit is implemented, the global memory limit is not
> (well, frankly, I just don't know. Maybe it is there, and I can't find it).
You mean --window-memory= ?
I agree this feature should be opt-in. Linux, FreeBSD and Solaris
should probably do fine I assume ? I'm quite sure the two first will do,
I really know the latter less though.
--
·O· Pierre Habouzit
··O madcoder@debian.org
OOO http://www.madism.org
[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]
^ permalink raw reply
* [PATCH 3/3 FIXED] help (Windows): Display HTML in default browser using Windows' shell API
From: Steffen Prohaska @ 2008-07-11 7:37 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Johannes Schindelin, Johannes Sixt, Steffen Prohaska
In-Reply-To: <228FC26D-D318-487A-9BF3-B473096CB0C9@zib.de>
The system's default browser for displaying HTML help pages is now used
directly on Windows, instead of launching git-web--browser, which
requires a Unix shell. Avoiding MSYS' bash when possible is good
because it avoids potential path translation issues. In this case it is
not too hard to avoid launching a shell, so let's avoid it.
The Windows-specific code is implemented in compat/mingw.c to avoid
platform-specific code in the main code base. On Windows, open_html is
provided as a define. If open_html is not defined, git-web--browse is
used. This approach avoids platform-specific ifdefs by using
per-function ifdefs. The "ifndef open_html" together with the
introductory comment should sufficiently warn developers, so that they
hopefully will not break this mechanism.
Signed-off-by: Steffen Prohaska <prohaska@zib.de>
---
compat/mingw.c | 21 +++++++++++++++++++++
compat/mingw.h | 3 +++
help.c | 14 +++++++++++++-
3 files changed, 37 insertions(+), 1 deletions(-)
diff --git a/compat/mingw.c b/compat/mingw.c
index 3a05fe7..0ca73f7 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -1017,3 +1017,24 @@ sig_handler_t mingw_signal(int sig, sig_handler_t handler)
timer_fn = handler;
return old;
}
+
+static const char *make_backslash_path(const char *path) {
+ static char buf[PATH_MAX + 1];
+ char *c;
+
+ if (strlcpy(buf, path, PATH_MAX) >= PATH_MAX)
+ die ("Too long path: %.*s", 60, path);
+
+ for (c = buf; *c; c++) {
+ if (*c == '/')
+ *c = '\\';
+ }
+ return buf;
+}
+
+void mingw_open_html(const char *unixpath)
+{
+ const char *htmlpath = make_backslash_path(unixpath);
+ printf("Launching default browser to display HTML ...\n");
+ ShellExecute(NULL, "open", htmlpath, NULL, "\\", 0);
+}
diff --git a/compat/mingw.h b/compat/mingw.h
index 6bc049a..136361e 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -193,6 +193,9 @@ static inline unsigned int git_ntohl(unsigned int x)
sig_handler_t mingw_signal(int sig, sig_handler_t handler);
#define signal mingw_signal
+void mingw_open_html(const char *path);
+#define open_html mingw_open_html
+
/*
* git specific compatibility
*/
diff --git a/help.c b/help.c
index 0f055bf..52d39b8 100644
--- a/help.c
+++ b/help.c
@@ -644,6 +644,18 @@ static void get_html_page_path(struct strbuf *page_path, const char *page)
strbuf_addf(page_path, "%s/%s.html", html_path, page);
}
+/*
+ * If open_html is not defined in a platform-specific way (see for
+ * example compat/mingw.h), we use the script web--browse to display
+ * HTML.
+ */
+#ifndef open_html
+void open_html(const char *path)
+{
+ execl_git_cmd("web--browse", "-c", "help.browser", path, NULL);
+}
+#endif
+
static void show_html_page(const char *git_cmd)
{
const char *page = cmd_to_page(git_cmd);
@@ -651,7 +663,7 @@ static void show_html_page(const char *git_cmd)
get_html_page_path(&page_path, page);
- execl_git_cmd("web--browse", "-c", "help.browser", page_path.buf, NULL);
+ open_html(page_path.buf);
}
void help_unknown_cmd(const char *cmd)
--
1.5.6.1.282.gd8a0d
^ permalink raw reply related
* Re: [PATCH 3/3] help (Windows): Display HTML in default browser using Windows' shell API
From: Steffen Prohaska @ 2008-07-11 7:35 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Sixt, Git Mailing List, Johannes Schindelin
In-Reply-To: <1215761286-2489-3-git-send-email-prohaska@zib.de>
On Jul 11, 2008, at 9:28 AM, Steffen Prohaska wrote:
> +
> +static const char *make_backslash_path(const char* path) {
> + static char buf[PATH_MAX + 1];
> + char* c;
Style :-(. I'll send a fixed patch in a minute.
> +#ifndef open_html
> +void open_html(const char* path)
> +{
It'll fix this too.
Steffen
^ permalink raw reply
* [PATCH 2/3] help.c: Add support for htmldir relative to git_exec_path()
From: Steffen Prohaska @ 2008-07-11 7:28 UTC (permalink / raw)
To: Junio C Hamano, Johannes Sixt; +Cc: git, Johannes Schindelin, Steffen Prohaska
In-Reply-To: <1215761286-2489-1-git-send-email-prohaska@zib.de>
If htmldir (in the Makefile) is a relative path, this path will now be
interpreted relative to git_exec_path. This can be used to create an
installation that can be moved to a different directory without
re-compiling. The Windows installer (msysgit) is an example for such
a setup.
Note that the Makefile maps htmldir to the define GIT_HTML_PATH.
Signed-off-by: Steffen Prohaska <prohaska@zib.de>
---
help.c | 8 +++++---
1 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/help.c b/help.c
index ca9632b..0f055bf 100644
--- a/help.c
+++ b/help.c
@@ -633,13 +633,15 @@ static void show_info_page(const char *git_cmd)
static void get_html_page_path(struct strbuf *page_path, const char *page)
{
struct stat st;
+ const char *html_path = system_path(GIT_HTML_PATH);
/* Check that we have a git documentation directory. */
- if (stat(GIT_HTML_PATH "/git.html", &st) || !S_ISREG(st.st_mode))
- die("'%s': not a documentation directory.", GIT_HTML_PATH);
+ if (stat(mkpath("%s/git.html", html_path), &st)
+ || !S_ISREG(st.st_mode))
+ die("'%s': not a documentation directory.", html_path);
strbuf_init(page_path, 0);
- strbuf_addf(page_path, GIT_HTML_PATH "/%s.html", page);
+ strbuf_addf(page_path, "%s/%s.html", html_path, page);
}
static void show_html_page(const char *git_cmd)
--
1.5.6.1.282.gd8a0d
^ permalink raw reply related
* [PATCH 3/3] help (Windows): Display HTML in default browser using Windows' shell API
From: Steffen Prohaska @ 2008-07-11 7:28 UTC (permalink / raw)
To: Junio C Hamano, Johannes Sixt; +Cc: git, Johannes Schindelin, Steffen Prohaska
In-Reply-To: <1215761286-2489-2-git-send-email-prohaska@zib.de>
The system's default browser for displaying HTML help pages is now used
directly on Windows, instead of launching git-web--browser, which
requires a Unix shell. Avoiding MSYS' bash when possible is good
because it avoids potential path translation issues. In this case it is
not too hard to avoid launching a shell, so let's avoid it.
The Windows-specific code is implemented in compat/mingw.c to avoid
platform-specific code in the main code base. On Windows, open_html is
provided as a define. If open_html is not defined, git-web--browse is
used. This approach avoids platform-specific ifdefs by using
per-function ifdefs. The "ifndef open_html" together with the
introductory comment should sufficiently warn developers, so that they
hopefully will not break this mechanism.
Signed-off-by: Steffen Prohaska <prohaska@zib.de>
---
compat/mingw.c | 21 +++++++++++++++++++++
compat/mingw.h | 3 +++
help.c | 14 +++++++++++++-
3 files changed, 37 insertions(+), 1 deletions(-)
diff --git a/compat/mingw.c b/compat/mingw.c
index 3a05fe7..f7ef545 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -1017,3 +1017,24 @@ sig_handler_t mingw_signal(int sig, sig_handler_t handler)
timer_fn = handler;
return old;
}
+
+static const char *make_backslash_path(const char* path) {
+ static char buf[PATH_MAX + 1];
+ char* c;
+
+ if (strlcpy(buf, path, PATH_MAX) >= PATH_MAX)
+ die ("Too long path: %.*s", 60, path);
+
+ for (c = buf; *c; c++) {
+ if (*c == '/')
+ *c = '\\';
+ }
+ return buf;
+}
+
+void mingw_open_html(const char *unixpath)
+{
+ const char *htmlpath = make_backslash_path(unixpath);
+ printf("Launching default browser to display HTML ...\n");
+ ShellExecute(NULL, "open", htmlpath, NULL, "\\", 0);
+}
diff --git a/compat/mingw.h b/compat/mingw.h
index 6bc049a..136361e 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -193,6 +193,9 @@ static inline unsigned int git_ntohl(unsigned int x)
sig_handler_t mingw_signal(int sig, sig_handler_t handler);
#define signal mingw_signal
+void mingw_open_html(const char *path);
+#define open_html mingw_open_html
+
/*
* git specific compatibility
*/
diff --git a/help.c b/help.c
index 0f055bf..18116f3 100644
--- a/help.c
+++ b/help.c
@@ -644,6 +644,18 @@ static void get_html_page_path(struct strbuf *page_path, const char *page)
strbuf_addf(page_path, "%s/%s.html", html_path, page);
}
+/*
+ * If open_html is not defined in a platform-specific way (see for
+ * example compat/mingw.h), we use the script web--browse to display
+ * HTML.
+ */
+#ifndef open_html
+void open_html(const char* path)
+{
+ execl_git_cmd("web--browse", "-c", "help.browser", path, NULL);
+}
+#endif
+
static void show_html_page(const char *git_cmd)
{
const char *page = cmd_to_page(git_cmd);
@@ -651,7 +663,7 @@ static void show_html_page(const char *git_cmd)
get_html_page_path(&page_path, page);
- execl_git_cmd("web--browse", "-c", "help.browser", page_path.buf, NULL);
+ open_html(page_path.buf);
}
void help_unknown_cmd(const char *cmd)
--
1.5.6.1.282.gd8a0d
^ permalink raw reply related
* [PATCH 1/3] Move code interpreting path relative to exec-dir to new function system_path()
From: Steffen Prohaska @ 2008-07-11 7:28 UTC (permalink / raw)
To: Junio C Hamano, Johannes Sixt; +Cc: git, Johannes Schindelin, Steffen Prohaska
In-Reply-To: <4668B2FF-2B2B-4221-8151-F0AEA681983C@zib.de>
Expanding system paths relative to git_exec_path can be used for
creating an installation that can be moved to a different directory
without re-compiling. We use this approach for template_dir and the
system wide gitconfig. The Windows installer (msysgit) is an example
for such a setup.
This commit moves common code to a new function system_path(). System
paths that are to be interpreted relative to git_exec_path are passed to
system_path() and the return value is used instead of the original path.
system_path() prefixes a relative path with git_exec_path and leaves
absolute paths unmodified. For example, we now write
template_dir = system_path(DEFAULT_GIT_TEMPLATE_DIR);
Signed-off-by: Steffen Prohaska <prohaska@zib.de>
---
builtin-init-db.c | 14 ++------------
cache.h | 1 +
config.c | 11 ++---------
path.c | 11 +++++++++++
4 files changed, 16 insertions(+), 21 deletions(-)
diff --git a/builtin-init-db.c b/builtin-init-db.c
index e23b843..5ba213a 100644
--- a/builtin-init-db.c
+++ b/builtin-init-db.c
@@ -115,18 +115,8 @@ static void copy_templates(const char *template_dir)
if (!template_dir)
template_dir = getenv(TEMPLATE_DIR_ENVIRONMENT);
- if (!template_dir) {
- /*
- * if the hard-coded template is relative, it is
- * interpreted relative to the exec_dir
- */
- template_dir = DEFAULT_GIT_TEMPLATE_DIR;
- if (!is_absolute_path(template_dir)) {
- struct strbuf d = STRBUF_INIT;
- strbuf_addf(&d, "%s/%s", git_exec_path(), template_dir);
- template_dir = strbuf_detach(&d, NULL);
- }
- }
+ if (!template_dir)
+ template_dir = system_path(DEFAULT_GIT_TEMPLATE_DIR);
strcpy(template_path, template_dir);
template_len = strlen(template_path);
if (template_path[template_len-1] != '/') {
diff --git a/cache.h b/cache.h
index 0d8edda..dafa265 100644
--- a/cache.h
+++ b/cache.h
@@ -529,6 +529,7 @@ const char *make_nonrelative_path(const char *path);
const char *make_relative_path(const char *abs, const char *base);
int normalize_absolute_path(char *buf, const char *path);
int longest_ancestor_length(const char *path, const char *prefix_list);
+extern const char *system_path(const char *path);
/* Read and unpack a sha1 file into memory, write memory to a sha1 file */
extern int sha1_object_info(const unsigned char *, unsigned long *);
diff --git a/config.c b/config.c
index 2862cc4..1e066c7 100644
--- a/config.c
+++ b/config.c
@@ -581,15 +581,8 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data)
const char *git_etc_gitconfig(void)
{
static const char *system_wide;
- if (!system_wide) {
- system_wide = ETC_GITCONFIG;
- if (!is_absolute_path(system_wide)) {
- /* interpret path relative to exec-dir */
- struct strbuf d = STRBUF_INIT;
- strbuf_addf(&d, "%s/%s", git_exec_path(), system_wide);
- system_wide = strbuf_detach(&d, NULL);
- }
- }
+ if (!system_wide)
+ system_wide = system_path(ETC_GITCONFIG);
return system_wide;
}
diff --git a/path.c b/path.c
index 5983255..141496e 100644
--- a/path.c
+++ b/path.c
@@ -11,6 +11,7 @@
* which is what it's designed for.
*/
#include "cache.h"
+#include "exec_cmd.h"
static char bad_path[] = "/bad-path/";
@@ -439,3 +440,13 @@ int longest_ancestor_length(const char *path, const char *prefix_list)
return max_len;
}
+
+const char *system_path(const char *path)
+{
+ if (!is_absolute_path(path)) {
+ struct strbuf d = STRBUF_INIT;
+ strbuf_addf(&d, "%s/%s", git_exec_path(), path);
+ path = strbuf_detach(&d, NULL);
+ }
+ return path;
+}
--
1.5.6.1.282.gd8a0d
^ permalink raw reply related
* Re: [PATCH 1/2] help.c: Add support for htmldir relative to git_exec_path()
From: Steffen Prohaska @ 2008-07-11 7:27 UTC (permalink / raw)
To: Junio C Hamano, Johannes Sixt; +Cc: Git Mailing List, Johannes Schindelin
In-Reply-To: <alpine.DEB.1.00.0807041334010.9925@racer>
On Jul 4, 2008, at 2:35 PM, Johannes Schindelin wrote:
> On Fri, 4 Jul 2008, Junio C Hamano wrote:
>
>> Could you check if there are copy-and-pasted duplicated code you can
>> factor out before continuing this direction?
>
> Note also that Hannes tried very hard to get rid of those ugly "#ifdef
> __MINGW32__"s by declaring/overriding functions in git-compat-util.h.
>
> I think that is such a good practice that we should not stop here.
I'll send three patches that address Junio's and Dscho's comments:
[PATCH 1/3] Move code interpreting path relative to exec-dir to new
function system_path()
[PATCH 2/3] help.c: Add support for htmldir relative to
git_exec_path()
[PATCH 3/3] help (Windows): Display HTML in default browser using
Windows' shell API
Hannes,
the patches I'll send probably conflict with your planned work on
GIT_EXEC_PATH that has been discussed on the msysgit list. I think
you could built on my series and modify system_path().
Steffen
^ permalink raw reply
* Re: [PATCH] Added --export option to git-send-email.
From: Junio C Hamano @ 2008-07-11 7:03 UTC (permalink / raw)
To: Eduard - Gabriel Munteanu; +Cc: ryan, git
In-Reply-To: <20080710170735.4c5b237a@linux360.ro>
Eduard - Gabriel Munteanu <eduard.munteanu@linux360.ro> writes:
> This option allows the user to process patches with git-send-email and then
> import them into an email client, without having to send them directly. The
> output format is mbox.
Input format is also mbox, so an obvious question is "why?"
^ permalink raw reply
* Re: [PATCH] cherry: cache patch-ids to avoid repeating work
From: Junio C Hamano @ 2008-07-11 6:54 UTC (permalink / raw)
To: Geoffrey Irving; +Cc: Johannes Schindelin, Junio C Hamano, git@vger.kernel.org
In-Reply-To: <7f9d599f0807100733s4435a9bga89749f2f6e10cf@mail.gmail.com>
"Geoffrey Irving" <irving@naml.us> writes:
>>> Oops: avoiding the infinite loop only requires reading expected O(1)
>>> entries on load, so I can fix that if you like. It would only be all of
>>> them if it actually did detect the infinite loop.
>>
>> I have to admit that you lost me there. AFAIR the patch-id cache is a
>> simple commit->patch_id store, right? Then there should be no way to get
>> an infinite loop.
>
> If every entry is nonnull, find_helper loops forever.
Isn't it sufficient to make this part check the condition as well?
+ if (cache->count >= cache->size)
+ {
+ warning("%s is corrupt: count %"PRIu32" >= size %"PRIu32,
+ filename, cache->count, cache->size);
+ goto empty;
+ }
At runtime you keep the invariants that hashtable always has at most 3/4
full and whoever wrote the file you are reading must have honored that as
well, or there is something fishy going on.
>> Besides, this is a purely local cache, no? Never to be transmitted... So
>> not much chance of a malicious attack, except if you allow write access to
>> your local repository, in which case you are endangered no matter what.
>
> Yep, that's why it's only a hole in quotes, and why I didn't fix it.
You might want to protect yourself against file corruption, though.
Checksumming the whole file and checking it at opening defeats the point
of mmap'ed access, but at least the header may want to be checksummed?
^ permalink raw reply
* Re: [PATCH] bisect: test merge base if good rev is not an ancestor of bad rev
From: Junio C Hamano @ 2008-07-11 6:51 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Christian Couder, Michael Haggerty, Jeff King, git
In-Reply-To: <alpine.DEB.1.00.0807110155040.3279@eeepc-johanness>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> On Thu, 10 Jul 2008, Junio C Hamano wrote:
>
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>>
> Of course it can be that the user commits a pilot error and says "but that
> unrelated version was good", while the fork point(s) between good and bad
> was bad (and this might be even the intention of the user, to find _one_
> commit that introduced the bug).
>
> Speaking of plural, what if some of the merge bases are good, some are
> bad?
>
> Without carefully thinking it through, you might even _break_ the tool.
And you think it is better to make all of your _users_ think it through
every time? Isn't it more error prone?
> All I was proposing is keeping the current semantics, keeping the
> mechanism simple, and therefore reliable.
What I suggested to Christian (sorry, I've been busy and I still haven't
checked if that is what was implemented in the patch -- that is why I
suggested you to read the original thread) was:
- check good and bad to see if they are forked
- iff they are,
- have the user check merge bases and make sure they are all
good. otherwise, the initial good/bad pair is unsuitable for
bisection, so explain the situation and quit [*1*];
- otherwise, keep these good markers.
- do the usual bisection --- from this point on it is "simple and
reliable as it has always been".
And I do not think adding the "pre-check" stage before going into the main
part of the processing that we have always done is against "keeping the
mechanism simple and reliable".
[Footnote]
*1* We _could_ make things more complex by offering to swap good and bad
at this point and then continue bisecting to find a commit to cherry-pick
to forward port the fix. Arguably, that step would be a new code and
could start out to be buggy --- it _could_ be called destabilizing what
has been reliable, but even then, it would be a separate codepath and a
new bug will be something that triggers only when the user accepts that
offer. I do not see what the big deal is that you seem to be worried
about.
^ permalink raw reply
* Re: [PATCH 3/5] git-bisect: use dash-less form on git bisect log
From: Christian Couder @ 2008-07-11 6:39 UTC (permalink / raw)
To: Miklos Vajna; +Cc: Junio C Hamano, git
In-Reply-To: <eb505d0d8f7d2bf50073414baa799a80a9df7453.1215734605.git.vmiklos@frugalware.org>
Le vendredi 11 juillet 2008, Miklos Vajna a écrit :
> Given that users are supposed to type 'git bisect' now, make the output
> of 'git bisect log' consistent with this.
>
> Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
> ---
> git-bisect.sh | 8 ++++----
> 1 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/git-bisect.sh b/git-bisect.sh
> index 991b2ef..f4852a9 100755
> --- a/git-bisect.sh
> +++ b/git-bisect.sh
> @@ -149,7 +149,7 @@ bisect_start() {
> echo "$start_head" >"$GIT_DIR/BISECT_START" &&
> sq "$@" >"$GIT_DIR/BISECT_NAMES" &&
> eval "$eval" &&
> - echo "git-bisect start$orig_args" >>"$GIT_DIR/BISECT_LOG" || exit
> + echo "git bisect start$orig_args" >>"$GIT_DIR/BISECT_LOG" || exit
> #
> # Check if we can proceed to the next bisect state.
> #
> @@ -169,7 +169,7 @@ bisect_write() {
> esac
> git update-ref "refs/bisect/$tag" "$rev" || exit
> echo "# $state: $(git show-branch $rev)" >>"$GIT_DIR/BISECT_LOG"
> - test -n "$nolog" || echo "git-bisect $state $rev"
> >>"$GIT_DIR/BISECT_LOG" + test -n "$nolog" || echo "git bisect $state
> $rev" >>"$GIT_DIR/BISECT_LOG" }
>
> bisect_state() {
> @@ -426,9 +426,9 @@ bisect_clean_state() {
> bisect_replay () {
> test -r "$1" || die "cannot read $1 for replaying"
> bisect_reset
> - while read bisect command rev
> + while read git bisect command rev
> do
> - test "$bisect" = "git-bisect" || continue
> + test "$git $bisect" = "git bisect" || continue
> case "$command" in
> start)
> cmd="bisect_start $rev"
Wouldn't it be better if bisect_replay could read old logs?
Maybe with something like:
+ while read git bisect command rev
do
- test "$bisect" = "git-bisect" || continue
+ test "$git $bisect" = "git bisect" -o "$git" = "git-bisect" || continue
+ if test "$git" = "git-bisect"; then
+ rev="$command"
+ command="$bisect"
+ fi
case "$command" in
start)
cmd="bisect_start $rev"
Thanks,
Christian.
^ permalink raw reply
* Re: [PATCH] Fix backwards-incompatible handling of core.sharedRepository
From: Junio C Hamano @ 2008-07-11 5:34 UTC (permalink / raw)
To: Petr Baudis; +Cc: git
In-Reply-To: <7vr6a1mhqi.fsf@gitster.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Petr Baudis <pasky@suse.cz> writes:
>
>> The 06cbe8550324e0fd2290839bf3b9a92aa53b70ab core.sharedRepository
>> handling extension broke backwards compatibility; before, shared=1 meant
>> that Git merely ensured the repository is group-writable, not that it's
>> _only_ group-writable, which is the current behaviour.
>
> Donn't our existing tests catch this, and if the answer is no because we
> don't have any, could you add some?
> ...
>> diff --git a/path.c b/path.c
>> index 5983255..75c5915 100644
>> --- a/path.c
>> +++ b/path.c
>> @@ -269,7 +269,7 @@ int adjust_shared_perm(const char *path)
>> mode = st.st_mode;
>>
>> if (shared_repository) {
>> - int tweak = shared_repository;
>> + int tweak = (mode & 0777) | shared_repository;
>> if (!(mode & S_IWUSR))
>> tweak &= ~0222;
>> mode = (mode & ~0777) | tweak;
>
> I think this change is good. shared_repository has always been about
> widening the access and not about limiting.
Having said that, you really should protect this behaviour from regression
with a test case. I do not see practical difference for sane umask
values.
What umask are you using, and which file in the repository gets affected?
In the old code I see we do have checks for S_IXUSR and tweaks on S_IXGRP
and S_IXOTH, but this should make a difference only if your umask blocks
executable bit and the file in question is executable. Was it an
executable hook copied from template?
^ permalink raw reply
* Re: [EGIT PATCH 0/9] Repository change listeners
From: Shawn O. Pearce @ 2008-07-11 5:26 UTC (permalink / raw)
To: Robin Rosenberg; +Cc: git, Marek Zawirski
In-Reply-To: <1215729651-26781-1-git-send-email-robin.rosenberg@dewire.com>
Robin Rosenberg <robin.rosenberg@dewire.com> wrote:
> We want to make the UI react on changes to the repo, but the parts
> of the code that makes the changes doesn't know who wants to react
> on them. This adds a publish-subcribe mechanism, including detection
> of externally made changes, e.g. by C Git.
Aside from the few patches I commented on, I like the overall series
(not just this one, but the others you sent today as well). Its a
welcome improvement.
--
Shawn.
^ permalink raw reply
* Re: [EGIT PATCH 2/2] Provide the ability to configure the quickdiff baseline
From: Shawn O. Pearce @ 2008-07-11 5:00 UTC (permalink / raw)
To: Robin Rosenberg; +Cc: git, Marek Zawirski
In-Reply-To: <1215729672-26906-3-git-send-email-robin.rosenberg@dewire.com>
Robin Rosenberg <robin.rosenberg@dewire.com> wrote:
> + TreeEntry blobEnry = repository.mapTree(baseline).findBlobMember(gitPath);
> + if (blobEnry != null) {
> + Activator.trace("(GitQuickDiffProvider) compareTo: " + baseline);
> + ObjectLoader loader = repository.openBlob(blobEnry.getId());
For what it is worth, it is probably faster to use TreeWalk:
final TreeWalk tw = TreeWalk.forPath(
repository,
gitPath,
new ObjectId[]{ repository.resolve(baseline + "^{tree}") });
if (tw != null) {
Activator.trace("(GitQuickDiffProvider) compareTo: " + baseline);
ObjectLoader loader = repository.openBlob(tw.getObjectId(0));
I probably should teach TreeWalk how to do the ^{tree} on its own
so it can just take a tree-ish here, but you still had to invoke
resolve to go from String to ObjectId so its not major problem.
The reason TreeWalk is faster is we spend less time parsing the
entries, and we avoid entries which are after the path we need to
walk down. E.g. if the subtree we need to enter into is in the
middle of the parent tree TreeWalk won't even touch the remainder
of the parent tree records. Tree/TreeEntry still does.
--
Shawn.
^ permalink raw reply
* Re: [EGIT PATCH 7/9] Add a job to refresh projects when the index changes.
From: Shawn O. Pearce @ 2008-07-11 4:33 UTC (permalink / raw)
To: Robin Rosenberg; +Cc: git, Marek Zawirski
In-Reply-To: <1215729651-26781-8-git-send-email-robin.rosenberg@dewire.com>
Robin Rosenberg <robin.rosenberg@dewire.com> wrote:
> +
> + static class RIRefresh extends Job implements RepositoryListener {
...
> + while (projectsToScan.size() > 0) {
> + IProject p;
> + synchronized (projectsToScan) {
> + if (projectsToScan.size() == 0) {
> + }
> + p = projectsToScan.iterator().next();
This is going to throw some sort of no such element exception when
you call next when projectsToScan is empty. You are missing a
break inside of the if test that is meant to guard against this.
> + projectsToScan.remove(p);
> + }
You have gone to the trouble of allocating the Iterator to obtain
the next item, you should also just use it to do the deletion of
the next item. Its quicker to let the Iterator do the remove as
it already has the pointer to the proper element position.
--
Shawn.
^ permalink raw reply
* Re: [EGIT PATCH 4/9] Add a method to listen to changes in any repository
From: Shawn O. Pearce @ 2008-07-11 4:28 UTC (permalink / raw)
To: Robin Rosenberg; +Cc: git, Marek Zawirski
In-Reply-To: <1215729651-26781-5-git-send-email-robin.rosenberg@dewire.com>
Robin Rosenberg <robin.rosenberg@dewire.com> wrote:
> diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/Repository.java b/org.spearce.jgit/src/org/spearce/jgit/lib/Repository.java
> index 6f78652..dfa3045 100644
> --- a/org.spearce.jgit/src/org/spearce/jgit/lib/Repository.java
> +++ b/org.spearce.jgit/src/org/spearce/jgit/lib/Repository.java
> @@ -95,6 +95,7 @@ public class Repository {
> private GitIndex index;
>
> private List<RepositoryListener> listeners = new Vector<RepositoryListener>(); // thread safe
> + static private List<RepositoryListener> allListeners = new Vector<RepositoryListener>(); // thread safe
...
> void fireRefsMaybeChanged() {
> if (refs.lastRefModification != refs.lastNotifiedRefModification) {
> refs.lastNotifiedRefModification = refs.lastRefModification;
> final RefsChangedEvent event = new RefsChangedEvent(this);
> - for (final RepositoryListener l :
> - listeners.toArray(new RepositoryListener[listeners.size()])) {
> + List<RepositoryListener> all = new ArrayList<RepositoryListener>(
> + listeners);
> + all.addAll(allListeners);
> + for (final RepositoryListener l : all) {
> l.refsChanged(event);
I don't think this pattern is thread-safe like you think it is.
Adding (or removing) an allListener while you are trying
to copy the allListener collection for delivery can cause a
ConcurrentModificationException.
The preimage here is not even correct because toArray locks the
vector and will grow the input array if it was too small, but it
nulls out the later entries if the array was too large. Thus you
can NPE inside of the fire loop.
The only safe way to do this is to lock the collection while you copy
it into an array or list, then later iterate that to do the delivery.
Both of the fire implemenations in this patch have this issue. :-|
--
Shawn.
^ permalink raw reply
* Re: [EGIT PATCH 1/9] Create a listener structure for changes to refs and index
From: Shawn O. Pearce @ 2008-07-11 4:22 UTC (permalink / raw)
To: Robin Rosenberg; +Cc: git, Marek Zawirski
In-Reply-To: <1215729651-26781-2-git-send-email-robin.rosenberg@dewire.com>
Robin Rosenberg <robin.rosenberg@dewire.com> wrote:
> This version does not tell you which refs have changed, nor
> what changes have happened. There is not scanning for externally
> initiated changes either, though such changes can be found when
> a JGit client wants to read index or refs information.
> diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/RefDatabase.java b/org.spearce.jgit/src/org/spearce/jgit/lib/RefDatabase.java
> index 9e3e020..4be33b8 100644
> --- a/org.spearce.jgit/src/org/spearce/jgit/lib/RefDatabase.java
> +++ b/org.spearce.jgit/src/org/spearce/jgit/lib/RefDatabase.java
> @@ -81,6 +81,10 @@ class RefDatabase {
>
> private long packedRefsLength;
>
> + long lastRefModification;
> +
> + long lastNotifiedRefModification;
> +
> RefDatabase(final Repository r) {
> db = r;
> gitDir = db.getDirectory();
> @@ -155,6 +161,12 @@ class RefDatabase {
> }
> if (!lck.commit())
> throw new ObjectWritingException("Unable to write " + name);
> + setModified();
> + db.fireRefsMaybeChanged();
> + }
> +
> + void setModified() {
> + lastRefModification = System.currentTimeMillis();
> }
We don't ever actually need the time here as a time, do we?
Wouldn't a simple counter work just as well, but ensure we don't
lose notifications that occur more frequently then the resolution
of currentTimeMillis()?
--
Shawn.
^ permalink raw reply
* Re: [EGIT PATCH 2/6] Using the page site selection turned out to be cumbersome.
From: Shawn O. Pearce @ 2008-07-11 3:52 UTC (permalink / raw)
To: Robin Rosenberg; +Cc: git, Marek Zawirski
In-Reply-To: <20080711034717.GA32633@spearce.org>
"Shawn O. Pearce" <spearce@spearce.org> wrote:
> Robin Rosenberg <robin.rosenberg@dewire.com> wrote:
> > + <action
> > + class="org.spearce.egit.ui.internal.actions.BranchAction"
> > + id="org.spearce.egit.ui.action1"
> > + label="Kolla"
>
> What does this label mean?
>
> > + <action
> > + class="org.spearce.egit.ui.internal.actions.ResetAction"
> > + id="org.spearce.egit.ui.action1"
> > + label="Titta"
>
> Ditto. You can use i18n translations in plugin.xml by putting these
> into a properties file and using %propertyname inside the XML.
> So we can prepare translations, but given that all of our source
> is documented in English I would prefer the property names to be
> the English name if we are going to start supporting translations.
Never mind, I see you fixed this in 6/6. Don't bother reworking
the series of this minor issue.
--
Shawn.
^ permalink raw reply
* Re: [EGIT PATCH 2/6] Using the page site selection turned out to be cumbersome.
From: Shawn O. Pearce @ 2008-07-11 3:47 UTC (permalink / raw)
To: Robin Rosenberg; +Cc: git, Marek Zawirski
In-Reply-To: <1215729573-26536-3-git-send-email-robin.rosenberg@dewire.com>
Robin Rosenberg <robin.rosenberg@dewire.com> wrote:
> + <action
> + class="org.spearce.egit.ui.internal.actions.BranchAction"
> + id="org.spearce.egit.ui.action1"
> + label="Kolla"
What does this label mean?
> + <action
> + class="org.spearce.egit.ui.internal.actions.ResetAction"
> + id="org.spearce.egit.ui.action1"
> + label="Titta"
Ditto. You can use i18n translations in plugin.xml by putting these
into a properties file and using %propertyname inside the XML.
So we can prepare translations, but given that all of our source
is documented in English I would prefer the property names to be
the English name if we are going to start supporting translations.
--
Shawn.
^ 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