Re: git 2.6.0 apply --cached failed
It probably is this one: http://thread.gmane.org/gmane.comp.version-control.git/270370/focus=270501 Older Git was loose and did not notice it, but the second hunk of your patch is judged to be broken, with no added or deleted line whatsoever, by the latest version. -- 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
git 2.6.0 apply --cached failed
Hi, Using git 2.6.0 on Linux 64-bit git apply --cached failed Please test with command with the repository inside the attached tarball. With git 2.6, git apply --cached < patch.patch fatal: corrupt patch at line 27 Expected result: no error Step to reproduce: Please run the following shell script. #/bin/sh set -e mkdir 2 cd 2 cat > d0c886e2-2cfe-4636-825b-3622fac0b27f
Re: [PATCH] clone --dissociate: avoid locking pack files
On Wed, Sep 30, 2015 at 10:28:14PM +0300, Max Kirillov wrote:
> On Mon, Sep 28, 2015 at 09:44:57PM +0200, Johannes Schindelin wrote:
>> -if (option_dissociate)
>> +if (option_dissociate) {
>> +struct packed_git *p;
>> +
>> +for (p = packed_git; p; p = p->next) {
>> +close_pack_windows(p);
>> +close_pack_index(p);
>> +}
>> dissociate_from_references();
>> +}
> This does not seem to close handles to the pack files
> themseves, does Windows still allow removing the files? I
> probably did not tried that, because I started from handles,
> and discovered mapped files only later.
Apparently, pack file is closed just after mapping if it's
smaller than core.packedGitWindowSize. Could it be the
reason that this patch worked in you test case?
--
Max
--
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
message not appear in mailing list
Hi, Why the message not appear in mailing list for many hours? There is no reject reply message. I sent the mail in plain text with a tarball attachment. http://dir.gmane.org/gmane.comp.version-control.git -- 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
[BUG?] applypatch-msg hook no-longer thinks stdin is a tty
Hi, I have a applypatch-msg hook that implements some policy for acceptable commit messages and reject non-conformant patches. It also is able to prompt me to override it's rejection. The prompting only happens when stdin is a tty (as determined by pythons sys.stdin.isatty()) For example this would reject the patch and cause am stop $ cat bad.patch | git am And this would prompt me to allow the patch and am would proceed if I answer Y $ git am bad.patch Patch message invalid apply (Y/N)? As of git 2.6 this has stopped working and stdin always fails the tty check. Here's a minimal reproduction $ cat >.git/hooks/applypatch-msg
[PATCH/RFC 0/2] close packs files when they are not needed
> The right approach may to have a helper in sha1_file.c that closes > and cleans up _all_ packs, and call it from here, instead of having > builtin/clone.c even know about implementation details such as > packed_git is a linked list, etc. Like this? Note I did not test it to actually work for the current code. Trying now what I could do with lsof, just to be sure myself, but probably its use not appropriate for the project. Max Kirillov (2): sha1_file: close all pack files after running sha1_file: set packfile to O_CLOEXEC at open builtin/pack-objects.c | 2 +- cache.h| 3 +- git.c | 2 ++ pack-bitmap.c | 2 +- sha1_file.c| 80 +++--- 5 files changed, 63 insertions(+), 26 deletions(-) -- 2.3.4.2801.g3d0809b -- 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/RFC 1/2] sha1_file: close all pack files after running
When a builtin has done its job, but waits for pager or not waited
by its caller and still hanging it keeps pack files opened.
This can cause a number of issues, for example on Windows git gc
cannot remove the packs.
Fix this by explicitly closing all pack files and unmapping memory
from the packs after running builtin. Do not die() if the memory region
is still used though, just print a warning, because failure to close
a file should not prevent the currently running program from finishing
its task.
Signed-off-by: Max Kirillov
---
cache.h | 1 +
git.c | 2 ++
sha1_file.c | 32 +---
3 files changed, 32 insertions(+), 3 deletions(-)
diff --git a/cache.h b/cache.h
index 79066e5..153bc46 100644
--- a/cache.h
+++ b/cache.h
@@ -1279,6 +1279,7 @@ extern void unuse_pack(struct pack_window **);
extern void free_pack_by_name(const char *);
extern void clear_delta_base_cache(void);
extern struct packed_git *add_packed_git(const char *, int, int);
+extern void close_all_packs(void);
/*
* Return the SHA-1 of the nth object within the specified packfile.
diff --git a/git.c b/git.c
index 5feba41..ad34680 100644
--- a/git.c
+++ b/git.c
@@ -1,4 +1,5 @@
#include "builtin.h"
+#include "cache.h"
#include "exec_cmd.h"
#include "help.h"
#include "run-command.h"
@@ -348,6 +349,7 @@ static int run_builtin(struct cmd_struct *p, int argc,
const char **argv)
trace_argv_printf(argv, "trace: built-in: git");
status = p->fn(argc, argv, prefix);
+ close_all_packs();
if (status)
return status;
diff --git a/sha1_file.c b/sha1_file.c
index 08302f5..62f1dad 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -773,20 +773,28 @@ void *xmmap(void *start, size_t length,
return ret;
}
-void close_pack_windows(struct packed_git *p)
+static int close_pack_windows_nodie(struct packed_git *p)
{
while (p->windows) {
struct pack_window *w = p->windows;
if (w->inuse_cnt)
- die("pack '%s' still has open windows to it",
- p->pack_name);
+ return 1;
+
munmap(w->base, w->len);
pack_mapped -= w->len;
pack_open_windows--;
p->windows = w->next;
free(w);
}
+
+ return 0;
+}
+
+void close_pack_windows(struct packed_git *p)
+{
+ if (close_pack_windows_nodie(p))
+ die("pack '%s' still has open windows to it", p->pack_name);
}
/*
@@ -866,6 +874,24 @@ static int close_one_pack(void)
return 0;
}
+void close_all_packs(void)
+{
+ struct packed_git *p = NULL;
+
+ for (p = packed_git; p; p = p->next) {
+ if (close_pack_windows_nodie(p))
+ warning("pack '%s' still has open windows to it",
p->pack_name);
+
+ if (p->pack_fd != -1) {
+ if (close(p->pack_fd) != 0)
+ warning("close(%s) failed: %d", p->pack_name,
errno);
+ p->pack_fd = -1;
+ }
+
+ close_pack_index(p);
+ }
+}
+
void unuse_pack(struct pack_window **w_cursor)
{
struct pack_window *w = *w_cursor;
--
2.3.4.2801.g3d0809b
--
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/RFC 2/2] sha1_file: set packfile to O_CLOEXEC at open
Windows does not support setting O_CLOEXEC by fcntl,
but there is an open flag O_NOINHERIT which results in same
behaviour. Use it in git_open_noatime() and also bring
setting O_CLOEXEC there also to make it consistent. Rename
the function to git_open_noatime_cloexec(), to avoid confusion.
Signed-off-by: Max Kirillov
---
builtin/pack-objects.c | 2 +-
cache.h| 2 +-
pack-bitmap.c | 2 +-
sha1_file.c| 48
4 files changed, 31 insertions(+), 23 deletions(-)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 1c63f8f..a8052f4 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -714,7 +714,7 @@ static off_t write_reused_pack(struct sha1file *f)
if (!is_pack_valid(reuse_packfile))
die("packfile is invalid: %s", reuse_packfile->pack_name);
- fd = git_open_noatime(reuse_packfile->pack_name);
+ fd = git_open_noatime_cloexec(reuse_packfile->pack_name);
if (fd < 0)
die_errno("unable to open packfile for reuse: %s",
reuse_packfile->pack_name);
diff --git a/cache.h b/cache.h
index 153bc46..77ef5ca 100644
--- a/cache.h
+++ b/cache.h
@@ -972,7 +972,7 @@ extern int write_sha1_file(const void *buf, unsigned long
len, const char *type,
extern int hash_sha1_file_literally(const void *buf, unsigned long len, const
char *type, unsigned char *sha1, unsigned flags);
extern int pretend_sha1_file(void *, unsigned long, enum object_type, unsigned
char *);
extern int force_object_loose(const unsigned char *sha1, time_t mtime);
-extern int git_open_noatime(const char *name);
+extern int git_open_noatime_cloexec(const char *name);
extern void *map_sha1_file(const unsigned char *sha1, unsigned long *size);
extern int unpack_sha1_header(git_zstream *stream, unsigned char *map,
unsigned long mapsize, void *buffer, unsigned long bufsiz);
extern int parse_sha1_header(const char *hdr, unsigned long *sizep);
diff --git a/pack-bitmap.c b/pack-bitmap.c
index 637770a..4a3b23c 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -274,7 +274,7 @@ static int open_pack_bitmap_1(struct packed_git *packfile)
return -1;
idx_name = pack_bitmap_filename(packfile);
- fd = git_open_noatime(idx_name);
+ fd = git_open_noatime_cloexec(idx_name);
free(idx_name);
if (fd < 0)
diff --git a/sha1_file.c b/sha1_file.c
index 62f1dad..2da5de2 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -385,7 +385,7 @@ void read_info_alternates(const char * relative_base, int
depth)
int fd;
sprintf(path, "%s/%s", relative_base, alt_file_name);
- fd = git_open_noatime(path);
+ fd = git_open_noatime_cloexec(path);
if (fd < 0)
return;
if (fstat(fd, &st) || (st.st_size == 0)) {
@@ -575,7 +575,7 @@ static int check_packed_git_idx(const char *path, struct
packed_git *p)
struct pack_idx_header *hdr;
size_t idx_size;
uint32_t version, nr, i, *index;
- int fd = git_open_noatime(path);
+ int fd = git_open_noatime_cloexec(path);
struct stat st;
if (fd < 0)
@@ -995,7 +995,6 @@ static int open_packed_git_1(struct packed_git *p)
struct pack_header hdr;
unsigned char sha1[20];
unsigned char *idx_sha1;
- long fd_flag;
if (!p->index_data && open_pack_index(p))
return error("packfile %s index unavailable", p->pack_name);
@@ -1013,7 +1012,7 @@ static int open_packed_git_1(struct packed_git *p)
while (pack_max_fds <= pack_open_fds && close_one_pack())
; /* nothing */
- p->pack_fd = git_open_noatime(p->pack_name);
+ p->pack_fd = git_open_noatime_cloexec(p->pack_name);
if (p->pack_fd < 0 || fstat(p->pack_fd, &st))
return -1;
pack_open_fds++;
@@ -1026,16 +1025,6 @@ static int open_packed_git_1(struct packed_git *p)
} else if (p->pack_size != st.st_size)
return error("packfile %s size changed", p->pack_name);
- /* We leave these file descriptors open with sliding mmap;
-* there is no point keeping them open across exec(), though.
-*/
- fd_flag = fcntl(p->pack_fd, F_GETFD, 0);
- if (fd_flag < 0)
- return error("cannot determine file descriptor flags");
- fd_flag |= FD_CLOEXEC;
- if (fcntl(p->pack_fd, F_SETFD, fd_flag) == -1)
- return error("cannot set FD_CLOEXEC");
-
/* Verify we recognize this pack file format. */
if (read_in_full(p->pack_fd, &hdr, sizeof(hdr)) != sizeof(hdr))
return error("file %s is far too short to be a packfile",
p->pack_name);
@@ -1515,17 +1504,21 @@ int check_sha1_signature(const unsigned char *sha1,
void *map,
return hashcmp(sha1, real_sha1) ? -1 : 0;
}
-int git_open_noatime(const char *name)
+int git_open_noatime_cloexec(const ch
Re: [PATCH 41/68] init: use strbufs to store paths
On Wed, Sep 30, 2015 at 01:00:56PM -0700, Junio C Hamano wrote: > > Wow, my patch isn't even close to reasonable. I didn't realize because > > we do not compile this code at all for non-Mac platforms. Sorry. > > Perhaps the way we completely stub out the platform specific helpers > contributes to this kind of gotchas? I am wondering how much additional > safety we would gain if we start doing something like this. I think it is an improvement, but it does not solve all of the problems. I also botched the implementation of probe_utf8_pathname_composition, and that does not get compiled on most platforms (though we _could_ compile it and just never call it). -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
[PATCHv6 4/8] strbuf: add strbuf_read_once to read without blocking
The new call will read from a file descriptor into a strbuf once. The
underlying call xread_nonblock is meant to execute without blocking if
the file descriptor is set to O_NONBLOCK. It is a bug to call
strbuf_read_once on a file descriptor which would block.
Signed-off-by: Stefan Beller
Signed-off-by: Junio C Hamano
---
strbuf.c | 11 +++
strbuf.h | 9 +
2 files changed, 20 insertions(+)
diff --git a/strbuf.c b/strbuf.c
index cce5eed..35e71b8 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -384,6 +384,17 @@ ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint)
return sb->len - oldlen;
}
+ssize_t strbuf_read_once(struct strbuf *sb, int fd, size_t hint)
+{
+ ssize_t cnt;
+
+ strbuf_grow(sb, hint ? hint : 8192);
+ cnt = xread_nonblock(fd, sb->buf + sb->len, sb->alloc - sb->len - 1);
+ if (cnt > 0)
+ strbuf_setlen(sb, sb->len + cnt);
+ return cnt;
+}
+
#define STRBUF_MAXLINK (2*PATH_MAX)
int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint)
diff --git a/strbuf.h b/strbuf.h
index aef2794..ea69665 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -367,6 +367,15 @@ extern size_t strbuf_fread(struct strbuf *, size_t, FILE
*);
extern ssize_t strbuf_read(struct strbuf *, int fd, size_t hint);
/**
+ * Read from a file descriptor that is marked as O_NONBLOCK without
+ * blocking. Returns the number of new bytes appended to the sb.
+ * Negative return value signals there was an error returned from
+ * underlying read(2), in which case the caller should check errno.
+ * e.g. errno == EAGAIN when the read may have blocked.
+ */
+extern ssize_t strbuf_read_once(struct strbuf *, int fd, size_t hint);
+
+/**
* Read the contents of a file, specified by its path. The third argument
* can be used to give a hint about the file size, to avoid reallocs.
*/
--
2.5.0.275.gf20166c.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
[PATCHv6 3/8] xread_nonblock: add functionality to read from fds without blocking
Provide a wrapper to read(), similar to xread(), that restarts on
EINTR but not EAGAIN (or EWOULDBLOCK). This enables the caller to
handle polling itself, possibly polling multiple sockets or performing
some other action.
Helped-by: Jacob Keller
Helped-by: Jeff King ,
Helped-by: Junio C Hamano
Signed-off-by: Stefan Beller
Signed-off-by: Junio C Hamano
---
git-compat-util.h | 1 +
wrapper.c | 22 ++
2 files changed, 23 insertions(+)
diff --git a/git-compat-util.h b/git-compat-util.h
index c6d391f..9ccea85 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -718,6 +718,7 @@ extern void *xcalloc(size_t nmemb, size_t size);
extern void *xmmap(void *start, size_t length, int prot, int flags, int fd,
off_t offset);
extern void *xmmap_gently(void *start, size_t length, int prot, int flags, int
fd, off_t offset);
extern ssize_t xread(int fd, void *buf, size_t len);
+extern ssize_t xread_nonblock(int fd, void *buf, size_t len);
extern ssize_t xwrite(int fd, const void *buf, size_t len);
extern ssize_t xpread(int fd, void *buf, size_t len, off_t offset);
extern int xdup(int fd);
diff --git a/wrapper.c b/wrapper.c
index 5517928..41a21e1 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -217,6 +217,28 @@ ssize_t xread(int fd, void *buf, size_t len)
}
/*
+ * xread_nonblock() is the same a read(), but it automatically restarts read()
+ * interrupted operations (EINTR). xread_nonblock() DOES NOT GUARANTEE that
+ * "len" bytes is read. EWOULDBLOCK is turned into EAGAIN.
+ */
+ssize_t xread_nonblock(int fd, void *buf, size_t len)
+{
+ ssize_t nr;
+ if (len > MAX_IO_SIZE)
+ len = MAX_IO_SIZE;
+ while (1) {
+ nr = read(fd, buf, len);
+ if (nr < 0) {
+ if (errno == EINTR)
+ continue;
+ if (errno == EWOULDBLOCK)
+ errno = EAGAIN;
+ }
+ return nr;
+ }
+}
+
+/*
* xwrite() is the same a write(), but it automatically restarts write()
* operations with a recoverable error (EAGAIN and EINTR). xwrite() DOES NOT
* GUARANTEE that "len" bytes is written even if the operation is successful.
--
2.5.0.275.gf20166c.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
[PATCHv6 7/8] fetch_populated_submodules: use new parallel job processing
In a later patch we enable parallel processing of submodules, this
only adds the possibility for it. So this change should not change
any user facing behavior.
Signed-off-by: Stefan Beller
Signed-off-by: Junio C Hamano
---
submodule.c | 128
1 file changed, 94 insertions(+), 34 deletions(-)
diff --git a/submodule.c b/submodule.c
index 1d64e57..ff5bc32 100644
--- a/submodule.c
+++ b/submodule.c
@@ -12,6 +12,7 @@
#include "sha1-array.h"
#include "argv-array.h"
#include "blob.h"
+#include "thread-utils.h"
static int config_fetch_recurse_submodules = RECURSE_SUBMODULES_ON_DEMAND;
static struct string_list changed_submodule_paths;
@@ -615,37 +616,91 @@ static void calculate_changed_submodule_paths(void)
initialized_fetch_ref_tips = 0;
}
+struct submodule_parallel_fetch {
+ int count;
+ struct argv_array args;
+ const char *work_tree;
+ const char *prefix;
+ int command_line_option;
+ int quiet;
+ int result;
+};
+#define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0}
+
+static int get_next_submodule(void **task_cb, struct child_process *cp,
+ struct strbuf *err, void *data);
+
+static int fetch_start_failure(struct child_process *cp,
+ struct strbuf *err,
+ void *cb, void *task_cb)
+{
+ struct submodule_parallel_fetch *spf = cb;
+
+ spf->result = 1;
+
+ return 0;
+}
+
+static int fetch_finish(int retvalue, struct child_process *cp,
+ struct strbuf *err, void *cb, void *task_cb)
+{
+ struct submodule_parallel_fetch *spf = cb;
+
+ if (retvalue)
+ spf->result = 1;
+
+ return 0;
+}
+
int fetch_populated_submodules(const struct argv_array *options,
const char *prefix, int command_line_option,
int quiet)
{
- int i, result = 0;
- struct child_process cp = CHILD_PROCESS_INIT;
- struct argv_array argv = ARGV_ARRAY_INIT;
- const char *work_tree = get_git_work_tree();
- if (!work_tree)
+ int i;
+ int max_parallel_jobs = 1;
+ struct submodule_parallel_fetch spf = SPF_INIT;
+
+ spf.work_tree = get_git_work_tree();
+ spf.command_line_option = command_line_option;
+ spf.quiet = quiet;
+ spf.prefix = prefix;
+
+ if (!spf.work_tree)
goto out;
if (read_cache() < 0)
die("index file corrupt");
- argv_array_push(&argv, "fetch");
+ argv_array_push(&spf.args, "fetch");
for (i = 0; i < options->argc; i++)
- argv_array_push(&argv, options->argv[i]);
- argv_array_push(&argv, "--recurse-submodules-default");
+ argv_array_push(&spf.args, options->argv[i]);
+ argv_array_push(&spf.args, "--recurse-submodules-default");
/* default value, "--submodule-prefix" and its value are added later */
- cp.env = local_repo_env;
- cp.git_cmd = 1;
- cp.no_stdin = 1;
-
calculate_changed_submodule_paths();
+ run_processes_parallel(max_parallel_jobs,
+ get_next_submodule,
+ fetch_start_failure,
+ fetch_finish,
+ &spf);
+
+ argv_array_clear(&spf.args);
+out:
+ string_list_clear(&changed_submodule_paths, 1);
+ return spf.result;
+}
- for (i = 0; i < active_nr; i++) {
+static int get_next_submodule(void **task_cb, struct child_process *cp,
+ struct strbuf *err, void *data)
+{
+ int ret = 0;
+ struct submodule_parallel_fetch *spf = data;
+
+ for ( ; spf->count < active_nr; spf->count++) {
struct strbuf submodule_path = STRBUF_INIT;
struct strbuf submodule_git_dir = STRBUF_INIT;
struct strbuf submodule_prefix = STRBUF_INIT;
- const struct cache_entry *ce = active_cache[i];
+ const struct cache_entry *ce = active_cache[spf->count];
const char *git_dir, *default_argv;
const struct submodule *submodule;
@@ -657,7 +712,7 @@ int fetch_populated_submodules(const struct argv_array
*options,
submodule = submodule_from_name(null_sha1, ce->name);
default_argv = "yes";
- if (command_line_option == RECURSE_SUBMODULES_DEFAULT) {
+ if (spf->command_line_option == RECURSE_SUBMODULES_DEFAULT) {
if (submodule &&
submodule->fetch_recurse !=
RECURSE_SUBMODULES_NONE) {
@@ -680,40 +735,45 @@ int fetch_populated_submodules(const struct argv_array
*options,
default_argv = "on-demand";
}
[PATCHv6 6/8] run-command: add an asynchronous parallel child processor
This allows to run external commands in parallel with ordered output
on stderr.
If we run external commands in parallel we cannot pipe the output directly
to the our stdout/err as it would mix up. So each process's output will
flow through a pipe, which we buffer. One subprocess can be directly
piped to out stdout/err for a low latency feedback to the user.
Example:
Let's assume we have 5 submodules A,B,C,D,E and each fetch takes a
different amount of time as the different submodules vary in size, then
the output of fetches in sequential order might look like this:
time -->
output: |---A---| |-B-| |---C---| |-D-| |-E-|
When we schedule these submodules into maximal two parallel processes,
a schedule and sample output over time may look like this:
process 1: |---A---| |-D-| |-E-|
process 2: |-B-| |---C---|
output:|---A---|B|---C---|DE
So A will be perceived as it would run normally in the single child
version. As B has finished by the time A is done, we can dump its whole
progress buffer on stderr, such that it looks like it finished in no
time. Once that is done, C is determined to be the visible child and
its progress will be reported in real time.
So this way of output is really good for human consumption, as it only
changes the timing, not the actual output.
For machine consumption the output needs to be prepared in the tasks,
by either having a prefix per line or per block to indicate whose tasks
output is displayed, because the output order may not follow the
original sequential ordering:
|A| |--B--| |-C-|
will be scheduled to be all parallel:
process 1: |A|
process 2: |--B--|
process 3: |-C-|
output:|A|CB
This happens because C finished before B did, so it will be queued for
output before B.
Signed-off-by: Stefan Beller
Signed-off-by: Junio C Hamano
---
run-command.c | 350 +
run-command.h | 78 +++
t/t0061-run-command.sh | 20 +++
test-run-command.c | 25
4 files changed, 473 insertions(+)
diff --git a/run-command.c b/run-command.c
index 28e1d55..28048a7 100644
--- a/run-command.c
+++ b/run-command.c
@@ -3,6 +3,8 @@
#include "exec_cmd.h"
#include "sigchain.h"
#include "argv-array.h"
+#include "thread-utils.h"
+#include "strbuf.h"
void child_process_init(struct child_process *child)
{
@@ -852,3 +854,351 @@ int capture_command(struct child_process *cmd, struct
strbuf *buf, size_t hint)
close(cmd->out);
return finish_command(cmd);
}
+
+struct parallel_processes {
+ void *data;
+
+ int max_processes;
+ int nr_processes;
+
+ get_next_task_fn get_next_task;
+ start_failure_fn start_failure;
+ task_finished_fn task_finished;
+
+ struct {
+ unsigned in_use : 1;
+ struct child_process process;
+ struct strbuf err;
+ void *data;
+ } *children;
+ /*
+* The struct pollfd is logically part of *children,
+* but the system call expects it as its own array.
+*/
+ struct pollfd *pfd;
+
+ unsigned shutdown : 1;
+
+ int output_owner;
+ struct strbuf buffered_output; /* of finished children */
+} parallel_processes_struct;
+
+static int default_start_failure(struct child_process *cp,
+struct strbuf *err,
+void *pp_cb,
+void *pp_task_cb)
+{
+ int i;
+
+ strbuf_addstr(err, "Starting a child failed:");
+ for (i = 0; cp->argv[i]; i++)
+ strbuf_addf(err, " %s", cp->argv[i]);
+
+ return 0;
+}
+
+static int default_task_finished(int result,
+struct child_process *cp,
+struct strbuf *err,
+void *pp_cb,
+void *pp_task_cb)
+{
+ int i;
+
+ if (!result)
+ return 0;
+
+ strbuf_addf(err, "A child failed with return code %d:", result);
+ for (i = 0; cp->argv[i]; i++)
+ strbuf_addf(err, " %s", cp->argv[i]);
+
+ return 0;
+}
+
+static void kill_children(struct parallel_processes *pp, int signo)
+{
+ int i, n = pp->max_processes;
+
+ for (i = 0; i < n; i++)
+ if (pp->children[i].in_use)
+ kill(pp->children[i].process.pid, signo);
+}
+
+static void handle_children_on_signal(int signo)
+{
+ struct parallel_processes *pp = ¶llel_processes_struct;
+
+ kill_children(pp, signo);
+ sigchain_pop(signo);
+ raise(signo);
+}
+
+static struct parallel_processes *pp_init(int n,
+ get_next_task_fn get_next_task,
+ start_failure_fn start_failure,
+ task_finished_fn task_finished,
+
[PATCHv6 8/8] submodules: allow parallel fetching, add tests and documentation
This enables the work of the previous patches.
Signed-off-by: Stefan Beller
Signed-off-by: Junio C Hamano
---
Documentation/fetch-options.txt | 7 +++
builtin/fetch.c | 6 +-
builtin/pull.c | 6 ++
submodule.c | 3 +--
submodule.h | 2 +-
t/t5526-fetch-submodules.sh | 19 +++
6 files changed, 39 insertions(+), 4 deletions(-)
diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
index 45583d8..6b109f6 100644
--- a/Documentation/fetch-options.txt
+++ b/Documentation/fetch-options.txt
@@ -100,6 +100,13 @@ ifndef::git-pull[]
reference to a commit that isn't already in the local submodule
clone.
+-j::
+--jobs=::
+ Number of parallel children to be used for fetching submodules.
+ Each will fetch from different submodules, such that fetching many
+ submodules will be faster. By default submodules will be fetched
+ one at a time.
+
--no-recurse-submodules::
Disable recursive fetching of submodules (this has the same effect as
using the '--recurse-submodules=no' option).
diff --git a/builtin/fetch.c b/builtin/fetch.c
index ee1f1a9..f28eac6 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -37,6 +37,7 @@ static int prune = -1; /* unspecified */
static int all, append, dry_run, force, keep, multiple, update_head_ok,
verbosity;
static int progress = -1, recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
static int tags = TAGS_DEFAULT, unshallow, update_shallow;
+static int max_children = 1;
static const char *depth;
static const char *upload_pack;
static struct strbuf default_rla = STRBUF_INIT;
@@ -99,6 +100,8 @@ static struct option builtin_fetch_options[] = {
N_("fetch all tags and associated objects"), TAGS_SET),
OPT_SET_INT('n', NULL, &tags,
N_("do not fetch all tags (--no-tags)"), TAGS_UNSET),
+ OPT_INTEGER('j', "jobs", &max_children,
+ N_("number of submodules fetched in parallel")),
OPT_BOOL('p', "prune", &prune,
N_("prune remote-tracking branches no longer on remote")),
{ OPTION_CALLBACK, 0, "recurse-submodules", NULL, N_("on-demand"),
@@ -1217,7 +1220,8 @@ int cmd_fetch(int argc, const char **argv, const char
*prefix)
result = fetch_populated_submodules(&options,
submodule_prefix,
recurse_submodules,
- verbosity < 0);
+ verbosity < 0,
+ max_children);
argv_array_clear(&options);
}
diff --git a/builtin/pull.c b/builtin/pull.c
index 722a83c..f0af196 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -94,6 +94,7 @@ static int opt_force;
static char *opt_tags;
static char *opt_prune;
static char *opt_recurse_submodules;
+static char *max_children;
static int opt_dry_run;
static char *opt_keep;
static char *opt_depth;
@@ -177,6 +178,9 @@ static struct option pull_options[] = {
N_("on-demand"),
N_("control recursive fetching of submodules"),
PARSE_OPT_OPTARG),
+ OPT_PASSTHRU('j', "jobs", &max_children, N_("n"),
+ N_("number of submodules pulled in parallel"),
+ PARSE_OPT_OPTARG),
OPT_BOOL(0, "dry-run", &opt_dry_run,
N_("dry run")),
OPT_PASSTHRU('k', "keep", &opt_keep, NULL,
@@ -524,6 +528,8 @@ static int run_fetch(const char *repo, const char
**refspecs)
argv_array_push(&args, opt_prune);
if (opt_recurse_submodules)
argv_array_push(&args, opt_recurse_submodules);
+ if (max_children)
+ argv_array_push(&args, max_children);
if (opt_dry_run)
argv_array_push(&args, "--dry-run");
if (opt_keep)
diff --git a/submodule.c b/submodule.c
index ff5bc32..cf8bf5d 100644
--- a/submodule.c
+++ b/submodule.c
@@ -654,10 +654,9 @@ static int fetch_finish(int retvalue, struct child_process
*cp,
int fetch_populated_submodules(const struct argv_array *options,
const char *prefix, int command_line_option,
- int quiet)
+ int quiet, int max_parallel_jobs)
{
int i;
- int max_parallel_jobs = 1;
struct submodule_parallel_fetch spf = SPF_INIT;
spf.work_tree = get_git_work_tree();
diff --git a/submodule.h b/submodule.h
index 5507c3d..cbc0003 100644
--- a/submodule.h
+++ b/submodule.h
@@ -31,7 +31,7 @@ void set_config_fetch_recurse_submodules(int value);
void check_for_new_submodule_commits(unsigned char new_sha1[20]);
int fetch_populated_submodules(const struct argv_array *options,
[PATCHv6 5/8] sigchain: add command to pop all common signals
The new method removes all common signal handlers that were installed
by sigchain_push.
CC: Jeff King
Signed-off-by: Stefan Beller
---
sigchain.c | 9 +
sigchain.h | 1 +
2 files changed, 10 insertions(+)
diff --git a/sigchain.c b/sigchain.c
index faa375d..2ac43bb 100644
--- a/sigchain.c
+++ b/sigchain.c
@@ -50,3 +50,12 @@ void sigchain_push_common(sigchain_fun f)
sigchain_push(SIGQUIT, f);
sigchain_push(SIGPIPE, f);
}
+
+void sigchain_pop_common(void)
+{
+ sigchain_pop(SIGPIPE);
+ sigchain_pop(SIGQUIT);
+ sigchain_pop(SIGTERM);
+ sigchain_pop(SIGHUP);
+ sigchain_pop(SIGINT);
+}
diff --git a/sigchain.h b/sigchain.h
index 618083b..138b20f 100644
--- a/sigchain.h
+++ b/sigchain.h
@@ -7,5 +7,6 @@ int sigchain_push(int sig, sigchain_fun f);
int sigchain_pop(int sig);
void sigchain_push_common(sigchain_fun f);
+void sigchain_pop_common(void);
#endif /* SIGCHAIN_H */
--
2.5.0.275.gf20166c.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
[PATCHv6 0/8] fetch submodules in parallel
This replaces sb/submodule-parallel-fetch once again.
Changes are only in patch 5,6,7
(5: reverse popping, 6: see below, 7: adapt to changes of 6).
Junio wrote:
> > + if (pp->return_value(pp->data, &pp->children[i].process,
> > + &pp->children[i].err, code))
> at this point, code can be uninitialized if we took the last "is
> confused" arm of the if/elseif cascade.
It's fixed in the reroll.
sigchain_pop_common reversed the popping.
When I started an office discussion with Jonathan about how to best implement
the next step ("git submodule update" using the parallel processing machine),
I fixes some nits and also some major spots:
* The order of the arguments for the callbacks (Generally the callback cookie
comes last and is called `cb` and not `data`)
* renamed return_value_fn to task_finished_fn
* Add another callback cookie for task specific things. This will help in the
rewrite of `git submodule update` as there are steps to be done after the
some processes are done using the parallel engine. So we want to be able
to remember specific children or tag information on them instead parsing the
cp->argv.
* the main loop of the parallel processing was first adapted to Junios
suggestion,
but Jonathan pointed out more improvements. We can get rid of `no_more_task`
completely as `if (!pp->nr_processes)` as the exit condition is sufficient.
(pp->nr_processes is modified only when starting or reaping a child, so we
will
capture the whole output of each subprocess even in case of a quick shutdown)
* even more accurate documentation
Jonathan Nieder (1):
submodule.c: write "Fetching submodule " to stderr
Stefan Beller (7):
xread: poll on non blocking fds
xread_nonblock: add functionality to read from fds without blocking
strbuf: add strbuf_read_once to read without blocking
sigchain: add command to pop all common signals
run-command: add an asynchronous parallel child processor
fetch_populated_submodules: use new parallel job processing
submodules: allow parallel fetching, add tests and documentation
Documentation/fetch-options.txt | 7 +
builtin/fetch.c | 6 +-
builtin/pull.c | 6 +
git-compat-util.h | 1 +
run-command.c | 350
run-command.h | 78 +
sigchain.c | 9 ++
sigchain.h | 1 +
strbuf.c| 11 ++
strbuf.h| 9 ++
submodule.c | 129 +++
submodule.h | 2 +-
t/t0061-run-command.sh | 20 +++
t/t5526-fetch-submodules.sh | 70 +---
test-run-command.c | 25 +++
wrapper.c | 35 +++-
16 files changed, 695 insertions(+), 64 deletions(-)
diff --git a/run-command.c b/run-command.c
index df84985..28048a7 100644
--- a/run-command.c
+++ b/run-command.c
@@ -863,12 +863,13 @@ struct parallel_processes {
get_next_task_fn get_next_task;
start_failure_fn start_failure;
- return_value_fn return_value;
+ task_finished_fn task_finished;
struct {
unsigned in_use : 1;
struct child_process process;
struct strbuf err;
+ void *data;
} *children;
/*
* The struct pollfd is logically part of *children,
@@ -882,9 +883,10 @@ struct parallel_processes {
struct strbuf buffered_output; /* of finished children */
} parallel_processes_struct;
-static int default_start_failure(void *data,
-struct child_process *cp,
-struct strbuf *err)
+static int default_start_failure(struct child_process *cp,
+struct strbuf *err,
+void *pp_cb,
+void *pp_task_cb)
{
int i;
@@ -895,10 +897,11 @@ static int default_start_failure(void *data,
return 0;
}
-static int default_return_value(void *data,
- struct child_process *cp,
- struct strbuf *err,
- int result)
+static int default_task_finished(int result,
+struct child_process *cp,
+struct strbuf *err,
+void *pp_cb,
+void *pp_task_cb)
{
int i;
@@ -930,10 +933,11 @@ static void handle_children_on_signal(int signo)
raise(signo);
}
-static struct parallel_processes *pp_init(int n, void *data,
+static struct parallel_processes *pp_init(int n,
get_next_task_fn get_next_task,
start_failure_fn start_failure,
- return_
[PATCHv6 1/8] submodule.c: write "Fetching submodule " to stderr
From: Jonathan Nieder
The "Pushing submodule " progress output correctly goes to
stderr, but "Fetching submodule " is going to stdout by
mistake. Fix it to write to stderr.
Noticed while trying to implement a parallel submodule fetch. When
this particular output line went to a different file descriptor, it
was buffered separately, resulting in wrongly interleaved output if
we copied it to the terminal naively.
Signed-off-by: Jonathan Nieder
Signed-off-by: Stefan Beller
Signed-off-by: Junio C Hamano
---
submodule.c | 2 +-
t/t5526-fetch-submodules.sh | 51 +++--
2 files changed, 27 insertions(+), 26 deletions(-)
diff --git a/submodule.c b/submodule.c
index 9fcc86f..1d64e57 100644
--- a/submodule.c
+++ b/submodule.c
@@ -694,7 +694,7 @@ int fetch_populated_submodules(const struct argv_array
*options,
git_dir = submodule_git_dir.buf;
if (is_directory(git_dir)) {
if (!quiet)
- printf("Fetching submodule %s%s\n", prefix,
ce->name);
+ fprintf(stderr, "Fetching submodule %s%s\n",
prefix, ce->name);
cp.dir = submodule_path.buf;
argv_array_push(&argv, default_argv);
argv_array_push(&argv, "--submodule-prefix");
diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
index a4532b0..17759b1 100755
--- a/t/t5526-fetch-submodules.sh
+++ b/t/t5526-fetch-submodules.sh
@@ -16,7 +16,8 @@ add_upstream_commit() {
git add subfile &&
git commit -m new subfile &&
head2=$(git rev-parse --short HEAD) &&
- echo "From $pwd/submodule" > ../expect.err &&
+ echo "Fetching submodule submodule" > ../expect.err &&
+ echo "From $pwd/submodule" >> ../expect.err &&
echo " $head1..$head2 master -> origin/master" >>
../expect.err
) &&
(
@@ -27,6 +28,7 @@ add_upstream_commit() {
git add deepsubfile &&
git commit -m new deepsubfile &&
head2=$(git rev-parse --short HEAD) &&
+ echo "Fetching submodule submodule/subdir/deepsubmodule" >>
../expect.err
echo "From $pwd/deepsubmodule" >> ../expect.err &&
echo " $head1..$head2 master -> origin/master" >>
../expect.err
)
@@ -56,9 +58,7 @@ test_expect_success setup '
(
cd downstream &&
git submodule update --init --recursive
- ) &&
- echo "Fetching submodule submodule" > expect.out &&
- echo "Fetching submodule submodule/subdir/deepsubmodule" >> expect.out
+ )
'
test_expect_success "fetch --recurse-submodules recurses into submodules" '
@@ -67,7 +67,7 @@ test_expect_success "fetch --recurse-submodules recurses into
submodules" '
cd downstream &&
git fetch --recurse-submodules >../actual.out 2>../actual.err
) &&
- test_i18ncmp expect.out actual.out &&
+ test_must_be_empty actual.out &&
test_i18ncmp expect.err actual.err
'
@@ -96,7 +96,7 @@ test_expect_success "using fetchRecurseSubmodules=true in
.gitmodules recurses i
git config -f .gitmodules
submodule.submodule.fetchRecurseSubmodules true &&
git fetch >../actual.out 2>../actual.err
) &&
- test_i18ncmp expect.out actual.out &&
+ test_must_be_empty actual.out &&
test_i18ncmp expect.err actual.err
'
@@ -127,7 +127,7 @@ test_expect_success "--recurse-submodules overrides
fetchRecurseSubmodules setti
git config --unset -f .gitmodules
submodule.submodule.fetchRecurseSubmodules &&
git config --unset submodule.submodule.fetchRecurseSubmodules
) &&
- test_i18ncmp expect.out actual.out &&
+ test_must_be_empty actual.out &&
test_i18ncmp expect.err actual.err
'
@@ -146,7 +146,7 @@ test_expect_success "--dry-run propagates to submodules" '
cd downstream &&
git fetch --recurse-submodules --dry-run >../actual.out
2>../actual.err
) &&
- test_i18ncmp expect.out actual.out &&
+ test_must_be_empty actual.out &&
test_i18ncmp expect.err actual.err
'
@@ -155,7 +155,7 @@ test_expect_success "Without --dry-run propagates to
submodules" '
cd downstream &&
git fetch --recurse-submodules >../actual.out 2>../actual.err
) &&
- test_i18ncmp expect.out actual.out &&
+ test_must_be_empty actual.out &&
test_i18ncmp expect.err actual.err
'
@@ -166,7 +166,7 @@ test_expect_success "recurseSubmodules=true propagates into
submodules" '
git config fetch.recurseSubmodules true
git fetch >../actual.out 2>../actual.err
) &&
- test_i18ncmp expect.out act
[PATCHv6 2/8] xread: poll on non blocking fds
>From the man page:
EAGAIN The file descriptor fd refers to a file other than a socket
and has been marked nonblocking (O_NONBLOCK), and the read
would block.
EAGAIN or EWOULDBLOCK
The file descriptor fd refers to a socket and has been marked
nonblocking (O_NONBLOCK), and the read would block. POSIX.1-2001
allows either error to be returned for this case, and does not
require these constants to have the same value, so a portable
application should check for both possibilities.
If we get an EAGAIN or EWOULDBLOCK the fd must have set O_NONBLOCK.
As the intent of xread is to read as much as possible either until the
fd is EOF or an actual error occurs, we can ease the feeder of the fd
by not spinning the whole time, but rather wait for it politely by not
busy waiting.
We should not care if the call to poll failed, as we're in an infinite
loop and can only get out with the correct read().
Signed-off-by: Stefan Beller
Signed-off-by: Junio C Hamano
---
wrapper.c | 13 +++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/wrapper.c b/wrapper.c
index ff49807..5517928 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -201,8 +201,17 @@ ssize_t xread(int fd, void *buf, size_t len)
len = MAX_IO_SIZE;
while (1) {
nr = read(fd, buf, len);
- if ((nr < 0) && (errno == EAGAIN || errno == EINTR))
- continue;
+ if (nr < 0) {
+ if (errno == EINTR)
+ continue;
+ if (errno == EAGAIN || errno == EWOULDBLOCK) {
+ struct pollfd pfd;
+ pfd.events = POLLIN;
+ pfd.fd = fd;
+ /* We deliberately ignore the return value */
+ poll(&pfd, 1, -1);
+ }
+ }
return nr;
}
}
--
2.5.0.275.gf20166c.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
What's cooking in git.git (Sep 2015, #07; Wed, 30)
Here are the topics that have been cooking. Commits prefixed with '-' are only in 'pu' (proposed updates) while commits prefixed with '+' are in 'next'. Git 2.6.0 was released a few days ago. I'll do 2.6.1 early next week, together with updates to a few older maintenance tracks, and we'll start the next cycle after that. With somewhat reduced review bandwidth, I'd expect that the upcoming cycle would be slower than usual. At tinyurl.com/gitCal, I tentatively drew a 14-week schedule for this cycle (I plan to be offline during weeks #7-#9 myself---hopefully we'll have capable interim maintainers to take care of the list traffic in the meantime as in past years). You can find the changes described here in the integration branches of the repositories listed at http://git-blame.blogspot.com/p/git-public-repositories.html -- [New Topics] * jk/asciidoctor-section-heading-markup-fix (2015-09-25) 1 commit - Documentation: fix section header mark-up Will merge to 'next'. * jk/war-on-sprintf (2015-09-28) 68 commits - name-rev: use strip_suffix to avoid magic numbers - use strbuf_complete to conditionally append slash - fsck: use for_each_loose_file_in_objdir - Makefile: drop D_INO_IN_DIRENT build knob - fsck: drop inode-sorting code - convert strncpy to memcpy - notes: document length of fanout path with a constant - color: add color_set helper for copying raw colors - prefer memcpy to strcpy - help: clean up kfmclient munging - receive-pack: simplify keep_arg computation - avoid sprintf and strcpy with flex arrays - use alloc_ref rather than hand-allocating "struct ref" - color: add overflow checks for parsing colors - drop strcpy in favor of raw sha1_to_hex - use sha1_to_hex_r() instead of strcpy - daemon: use cld->env_array when re-spawning - stat_tracking_info: convert to argv_array - http-push: use an argv_array for setup_revisions - fetch-pack: use argv_array for index-pack / unpack-objects - diagnose_invalid_index_path: use strbuf to avoid strcpy/strcat - write_loose_object: convert to strbuf - remove_leading_path: use a strbuf for internal storage - enter_repo: convert fixed-size buffers to strbufs - merge-recursive: convert malloc / strcpy to strbuf - transport: use strbufs for status table "quickref" strings - apply: convert root string to strbuf - init: use strbufs to store paths - sha1_get_pack_name: use a strbuf - http-walker: store url in a strbuf - http-push: use strbuf instead of fwrite_buffer - remote-ext: simplify git pkt-line generation - upload-archive: convert sprintf to strbuf - resolve_ref: use strbufs for internal buffers - read_remotes_file: simplify string handling - read_branches_file: simplify string handling - mailmap: replace strcpy with xstrdup - help: drop prepend function in favor of xstrfmt - ref-filter: drop sprintf and strcpy calls - use strip_suffix and xstrfmt to replace suffix - fetch: replace static buffer with xstrfmt - config: use xstrfmt in normalize_value - replace trivial malloc + sprintf / strcpy calls with xstrfmt - receive-pack: convert strncpy to xsnprintf - http-push: replace strcat with xsnprintf - add_packed_git: convert strcpy into xsnprintf - entry.c: convert strcpy to xsnprintf - grep: use xsnprintf to format failure message - compat/hstrerror: convert sprintf to snprintf - stop_progress_msg: convert sprintf to xsnprintf - find_short_object_filename: convert sprintf to xsnprintf - use xsnprintf for generating git object headers - archive-tar: use xsnprintf for trivial formatting - convert trivial sprintf / strcpy calls to xsnprintf - compat/inet_ntop: fix off-by-one in inet_ntop4 - test-dump-cache-tree: avoid overflow of cache-tree name - progress: store throughput display in a strbuf - trace: use strbuf for quote_crnl output - mailsplit: make PATH_MAX buffers dynamic - fsck: use strbuf to generate alternate directories - add reentrant variants of sha1_to_hex and find_unique_abbrev - strbuf: make strbuf_complete_line more generic - add git_path_buf helper function - add xsnprintf helper function - fsck: don't fsck alternates for connectivity-only check - archive-tar: fix minor indentation violation - mailsplit: fix FILE* leak in split_maildir - show-branch: avoid segfault with --reflog of unborn branch Needs further tweaking ($gmane/278837) * rp/link-curl-before-ssl (2015-09-25) 3 commits - configure: make curl-config path configurable - configure.ac: detect ssl need with libcurl - Makefile: link libcurl before openssl and crypto Will merge to 'next'. * sb/http-flaky-test-fix (2015-09-25) 1 commit - t5561: get rid of racy appending to logfile Will merge to 'next'. * sb/perf-without-installed-git (2015-09-25) 1 commit - t/perf: make runner work even if Git is not installed Will merge to 'next'. * tk/typofix-connect-unknown-proto-error (2015-09-25) 1 commit - connect: fix typo in result string of prot_name()
Re: [PATCH] rebase: accept indented comments (fixes regression)
Matthieu Moy writes:
> Junio C Hamano writes:
>
>> +pick|p|drop|d|reword|r|edit|e|squash|s|fixup|f)
>> +if ! check_commit_sha "${rest%% *}" "$lineno" "$1"
>
> This does not pass my "tabs" test, as it parses the sha1 out of the line
> assuming it's separated with a space.
Yes, it was very much deliberate to match what transform_todo_ids does.
I do not think we mind loosening this SP to SP/HT at all, but if we
are going to go in that direction, I'd very much more prefer to do
"${rest%%[ ]*}"
here _and_ in transform_todo_ids so that we can keep the identical
parsing, which is the primary point of the "regression fix", rather
than doing this:
> I changed it to
>
> while read -r command sha1 rest
>
> which is a bit more lazy.
which is to invite the two parsers drift apart over time.
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
[PATCH v2] rebase-i: loosen over-eager check_bad_cmd check
804098bb (git rebase -i: add static check for commands and SHA-1,
2015-06-29) tried to check all insns before running any in the todo
list, but it did so by implementing its own parser that is a lot
stricter than necessary. We used to allow lines that are indented
(including comment lines), and we used to allow a whitespace between
the insn and the commit object name to be HT, among other things,
that are flagged as an invalid line by mistake.
Fix this by using the same tokenizer that is used to parse the todo
list file in the new check.
Whether it's a good thing to accept indented comments is
debatable (other commands like "git commit" do not accept them), but we
already accepted them in the past, and some people and scripts rely on
this behavior. Also, a line starting with space followed by a '#' cannot
have any meaning other than being a comment, hence it doesn't harm to
accept them as comments.
Largely based on patch by: Junio C Hamano
Signed-off-by: Matthieu Moy
---
Oops, missing signed-off-by added.
git-rebase--interactive.sh| 62 ---
t/t3404-rebase-interactive.sh | 15 +++
2 files changed, 44 insertions(+), 33 deletions(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index f01637b..4ebd547 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -857,7 +857,8 @@ add_exec_commands () {
# Check if the SHA-1 passed as an argument is a
# correct one, if not then print $2 in "$todo".badsha
# $1: the SHA-1 to test
-# $2: the line to display if incorrect SHA-1
+# $2: the line number of the input
+# $3: the input filename
check_commit_sha () {
badsha=0
if test -z $1
@@ -873,9 +874,10 @@ check_commit_sha () {
if test $badsha -ne 0
then
+ line="$(sed -n -e "${2}p" "$3")"
warn "Warning: the SHA-1 is missing or isn't" \
"a commit in the following line:"
- warn " - $2"
+ warn " - $line"
warn
fi
@@ -886,37 +888,31 @@ check_commit_sha () {
# from the todolist in stdin
check_bad_cmd_and_sha () {
retval=0
- git stripspace --strip-comments |
- (
- while read -r line
- do
- IFS=' '
- set -- $line
- command=$1
- sha1=$2
-
- case $command in
- ''|noop|x|"exec")
- # Doesn't expect a SHA-1
- ;;
- pick|p|drop|d|reword|r|edit|e|squash|s|fixup|f)
- if ! check_commit_sha $sha1 "$line"
- then
- retval=1
- fi
- ;;
- *)
- warn "Warning: the command isn't recognized" \
- "in the following line:"
- warn " - $line"
- warn
+ lineno=0
+ while read -r command sha1 rest
+ do
+ lineno=$(( $lineno + 1 ))
+ case $command in
+ "$comment_char"*|''|noop|x|exec)
+ # Doesn't expect a SHA-1
+ ;;
+ pick|p|drop|d|reword|r|edit|e|squash|s|fixup|f)
+ if ! check_commit_sha "$sha1" "$lineno" "$1"
+ then
retval=1
- ;;
- esac
- done
-
- return $retval
- )
+ fi
+ ;;
+ *)
+ line="$(sed -n -e "${lineno}p" "$1")"
+ warn "Warning: the command isn't recognized" \
+ "in the following line:"
+ warn " - $line"
+ warn
+ retval=1
+ ;;
+ esac
+ done <"$1"
+ return $retval
}
# Print the list of the SHA-1 of the commits
@@ -1010,7 +1006,7 @@ check_todo_list () {
;;
esac
- if ! check_bad_cmd_and_sha <"$todo"
+ if ! check_bad_cmd_and_sha "$todo"
then
raise_error=t
fi
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index d26e3f5..3da3ba3 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1227,6 +1227,21 @@ test_expect_success 'static check of bad command' '
test C = $(git cat-file commit HEAD^ | sed -ne \$p)
'
+test_expect_success 'tabs and spaces are accepted in the todolist' '
+ rebase_setup_and_clean indented-comment &&
+ write_script add-indent.sh <<-\EOF &&
+ (
+ # Turn
Re: [PATCH] rebase-i: loosen over-eager check_bad_cmd check
On Wed, Sep 30, 2015 at 4:01 PM, Matthieu Moy wrote: > 804098bb (git rebase -i: add static check for commands and SHA-1, > 2015-06-29) tried to check all insns before running any in the todo > list, but it did so by implementing its own parser that is a lot > stricter than necessary. We used to allow lines that are indented > (including comment lines), and we used to allow a whitespace between > the insn and the commit object name to be HT, among other things, > that are flagged as an invalid line by mistake. > > Fix this by using the same tokenizer that is used to parse the todo > list file in the new check. > > Whether it's a good thing to accept indented comments is > debatable (other commands like "git commit" do not accept them), but we > already accepted them in the past, and some people and scripts rely on > this behavior. Also, a line starting with space followed by a '#' cannot > have any meaning other than being a comment, hence it doesn't harm to > accept them as comments. > > Largely based on patch by: Junio C Hamano Missing sign-off. -- 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] rebase-i: loosen over-eager check_bad_cmd check
804098bb (git rebase -i: add static check for commands and SHA-1,
2015-06-29) tried to check all insns before running any in the todo
list, but it did so by implementing its own parser that is a lot
stricter than necessary. We used to allow lines that are indented
(including comment lines), and we used to allow a whitespace between
the insn and the commit object name to be HT, among other things,
that are flagged as an invalid line by mistake.
Fix this by using the same tokenizer that is used to parse the todo
list file in the new check.
Whether it's a good thing to accept indented comments is
debatable (other commands like "git commit" do not accept them), but we
already accepted them in the past, and some people and scripts rely on
this behavior. Also, a line starting with space followed by a '#' cannot
have any meaning other than being a comment, hence it doesn't harm to
accept them as comments.
Largely based on patch by: Junio C Hamano
---
I've re-added the last paragraph about whether it's good to allow
indented comments, so that someone running "git blame" on my test get
more explanation about why this is deliberate.
I'm sending this with me in the author field, but feel free to take
back the ownership, you have about as much code as I do in this patch.
git-rebase--interactive.sh| 62 ---
t/t3404-rebase-interactive.sh | 15 +++
2 files changed, 44 insertions(+), 33 deletions(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index f01637b..4ebd547 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -857,7 +857,8 @@ add_exec_commands () {
# Check if the SHA-1 passed as an argument is a
# correct one, if not then print $2 in "$todo".badsha
# $1: the SHA-1 to test
-# $2: the line to display if incorrect SHA-1
+# $2: the line number of the input
+# $3: the input filename
check_commit_sha () {
badsha=0
if test -z $1
@@ -873,9 +874,10 @@ check_commit_sha () {
if test $badsha -ne 0
then
+ line="$(sed -n -e "${2}p" "$3")"
warn "Warning: the SHA-1 is missing or isn't" \
"a commit in the following line:"
- warn " - $2"
+ warn " - $line"
warn
fi
@@ -886,37 +888,31 @@ check_commit_sha () {
# from the todolist in stdin
check_bad_cmd_and_sha () {
retval=0
- git stripspace --strip-comments |
- (
- while read -r line
- do
- IFS=' '
- set -- $line
- command=$1
- sha1=$2
-
- case $command in
- ''|noop|x|"exec")
- # Doesn't expect a SHA-1
- ;;
- pick|p|drop|d|reword|r|edit|e|squash|s|fixup|f)
- if ! check_commit_sha $sha1 "$line"
- then
- retval=1
- fi
- ;;
- *)
- warn "Warning: the command isn't recognized" \
- "in the following line:"
- warn " - $line"
- warn
+ lineno=0
+ while read -r command sha1 rest
+ do
+ lineno=$(( $lineno + 1 ))
+ case $command in
+ "$comment_char"*|''|noop|x|exec)
+ # Doesn't expect a SHA-1
+ ;;
+ pick|p|drop|d|reword|r|edit|e|squash|s|fixup|f)
+ if ! check_commit_sha "$sha1" "$lineno" "$1"
+ then
retval=1
- ;;
- esac
- done
-
- return $retval
- )
+ fi
+ ;;
+ *)
+ line="$(sed -n -e "${lineno}p" "$1")"
+ warn "Warning: the command isn't recognized" \
+ "in the following line:"
+ warn " - $line"
+ warn
+ retval=1
+ ;;
+ esac
+ done <"$1"
+ return $retval
}
# Print the list of the SHA-1 of the commits
@@ -1010,7 +1006,7 @@ check_todo_list () {
;;
esac
- if ! check_bad_cmd_and_sha <"$todo"
+ if ! check_bad_cmd_and_sha "$todo"
then
raise_error=t
fi
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index d26e3f5..3da3ba3 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1227,6 +1227,21 @@ test_expect_success 'static check of bad command' '
Re: [PATCH 41/68] init: use strbufs to store paths
Jeff King writes:
> On Tue, Sep 29, 2015 at 04:50:39PM -0700, Michael Blume wrote:
>
>> I see compile errors on my mac:
>>
>> First a whole bunch of
>>
>> ./compat/precompose_utf8.h:30:45: warning: declaration of 'struct
>> strbuf' will not be visible outside of this function [-Wvisibility]
>> void probe_utf8_pathname_composition(struct strbuf *path);
>
> Wow, my patch isn't even close to reasonable. I didn't realize because
> we do not compile this code at all for non-Mac platforms. Sorry.
Perhaps the way we completely stub out the platform specific helpers
contributes to this kind of gotchas? I am wondering how much additional
safety we would gain if we start doing something like this.
Two things to note:
* "struct strbuf" needs to be visible when the compiler sees this
part, which is an indication of the same issue shown in the above
error message, is not addressed.
* precompose_str() does not seem to be defined or used, hence
removed.
git-compat-util.h | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/git-compat-util.h b/git-compat-util.h
index 712de7f..6710ff7 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -227,9 +227,11 @@ typedef unsigned long uintptr_t;
#ifdef PRECOMPOSE_UNICODE
#include "compat/precompose_utf8.h"
#else
-#define precompose_str(in,i_nfd2nfc)
-#define precompose_argv(c,v)
-#define probe_utf8_pathname_composition(p)
+static inline void precompose_argv(int, const char **);
+static inline void probe_utf8_pathname_composition(struct strbuf *buf)
+{
+ ; /* no-op */
+}
#endif
#ifdef MKDIR_WO_TRAILING_SLASH
--
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] gitk: add missing accelerators
In d99b4b0de27a ("gitk: Accelerators for the main menu", 2015-09-09),
accelerators were added to allow efficient keyboard navigation. One
instance of the strings "Edit view..." and "Delete view" were left
without the ampersand.
Add the missing ampersand characters to unbreak our international
users.
Signed-off-by: Beat Bolli
Cc: Paul Mackerras
---
gitk-git/gitk | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/gitk-git/gitk b/gitk-git/gitk
index 2028b55..fcc606e 100755
--- a/gitk-git/gitk
+++ b/gitk-git/gitk
@@ -12452,8 +12452,8 @@ if {$cmdline_files ne {} || $revtreeargs ne {} ||
$revtreeargscmd ne {}} {
set viewchanged(1) 0
set vdatemode(1) 0
addviewmenu 1
-.bar.view entryconf [mca "Edit view..."] -state normal
-.bar.view entryconf [mca "Delete view"] -state normal
+.bar.view entryconf [mca "&Edit view..."] -state normal
+.bar.view entryconf [mca "&Delete view"] -state normal
}
if {[info exists permviews]} {
--
2.6.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: [PATCH] rebase: accept indented comments (fixes regression)
Junio C Hamano writes:
> + pick|p|drop|d|reword|r|edit|e|squash|s|fixup|f)
> + if ! check_commit_sha "${rest%% *}" "$lineno" "$1"
This does not pass my "tabs" test, as it parses the sha1 out of the line
assuming it's separated with a space. It's used in other places of the
code, but tabs still seem to work more or less by chance (they are not
parsed properly by transform_todo_ids, but then they are understood by
do_next).
I changed it to
while read -r command sha1 rest
which is a bit more lazy.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
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 v2] am: configure gpg at startup
The new builtin am ignores the user.signingkey variable: gpg is being
called with the committer details as the key ID, which may not be
correct. git_gpg_config is responsible for handling that variable and is
expected to be called on initialization by any modules that use gpg.
Perhaps git_gpg_config's functionality ought to be merged into
git_default_config, but this is simpler and in keeping with the current
practice.
Signed-off-by: Renee Margaret McConahy
---
Oops, my bad. Thanks.
builtin/am.c | 13 -
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/builtin/am.c b/builtin/am.c
index 4f77e07..3bd4fd7 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -2208,6 +2208,17 @@ enum resume_mode {
RESUME_ABORT
};
+static int git_am_config(const char *k, const char *v, void *cb)
+{
+ int status;
+
+ status = git_gpg_config(k, v, NULL);
+ if (status)
+ return status;
+
+ return git_default_config(k, v, NULL);
+}
+
int cmd_am(int argc, const char **argv, const char *prefix)
{
struct am_state state;
@@ -2308,7 +2319,7 @@ int cmd_am(int argc, const char **argv, const char
*prefix)
OPT_END()
};
- git_config(git_default_config, NULL);
+ git_config(git_am_config, NULL);
am_state_init(&state, git_path("rebase-apply"));
--
2.5.3
--
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] clone --dissociate: avoid locking pack files
Max Kirillov writes:
>> +if (option_dissociate) {
>> +struct packed_git *p;
>> +
>> +for (p = packed_git; p; p = p->next) {
>> +close_pack_windows(p);
>> +close_pack_index(p);
>> +}
>> dissociate_from_references();
>> +}
>
> This does not seem to close handles to the pack files
> themseves, does Windows still allow removing the files? I
> probably did not tried that, because I started from handles,
> and discovered mapped files only later.
This is trying to (incompletely) mimic what free_pack_by_name() in
sha1_file.c is doing but for all packs, I think. I wonder why we do
not have to do other things here that are done over there, like
clearing delta-base-cache, closing pack_fd and decrementing
pack_open_fds, etc.
The right approach may to have a helper in sha1_file.c that closes
and cleans up _all_ packs, and call it from here, instead of having
builtin/clone.c even know about implementation details such as
packed_git is a linked list, etc.
>
>> junk_mode = JUNK_LEAVE_REPO;
>> err = checkout();
>> diff --git a/t/t5700-clone-reference.sh b/t/t5700-clone-reference.sh
>> index ef1779f..2250ef4 100755
>> --- a/t/t5700-clone-reference.sh
>> +++ b/t/t5700-clone-reference.sh
>> @@ -188,5 +188,26 @@ test_expect_success 'clone and dissociate from
>> reference' '
>> test_must_fail git -C R fsck &&
>> git -C S fsck
>> '
>> +test_expect_success 'clone, dissociate from partial reference and repack' '
>> +rm -fr P Q R &&
>> +git init P &&
>> +(
>> +cd P &&
>> +test_commit one &&
>> +git repack &&
>> +test_commit two &&
>> +git repack
>> +) &&
>> +git clone --bare P Q &&
>> +(
>> +cd P &&
>> +git checkout -b second &&
>> +test_commit three &&
>> +git repack
>> +) &&
>> +git clone --bare --dissociate --reference=P Q R &&
>> +ls R/objects/pack/*.pack >packs.txt &&
>> +test_line_count = 1 packs.txt
>> +'
>
> Unless it goes very lowlevel like running lsof of readin
> proc testing this should always pass on Linux, even if the
> issue is not fixed, maybe should be a conditional for
> Windows only?
>
>> test_done
>> --
>> 2.5.3.windows.1.3.gc322723
>>
>>
>> --
>> 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
--
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] rebase: accept indented comments (fixes regression)
Matthieu Moy writes: > Sounds good, yes. I'll send a patch with this and my updated tests. Thanks. I think our mails crossed, so I'd discard my copy that lacks the new test you wrote. -- 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] clone --dissociate: avoid locking pack files
On Mon, Sep 28, 2015 at 09:44:57PM +0200, Johannes Schindelin wrote:
> When `git clone` is asked to dissociate the repository from the
> reference repository whose objects were used, it is quite possible that
> the pack files need to be repacked. In that case, the pack files need to
> be deleted that were originally hard-links to the reference repository's
> pack files.
Hello. For 1.9.* I used to have some hack for closing files
also. The case was to allow scheduled git gc to remove packs
even if I forgot to quit some less in some console.
> On platforms where a file cannot be deleted if another process still
> holds a handle on it, we therefore need to take pains to release all
> pack files and indexes before dissociating.
>
> This fixes https://github.com/git-for-windows/git/issues/446
>
> Signed-off-by: Johannes Schindelin
> ---
> builtin/clone.c| 9 -
> t/t5700-clone-reference.sh | 21 +
> 2 files changed, 29 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/clone.c b/builtin/clone.c
> index 578da85..223adc4 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -1064,8 +1064,15 @@ int cmd_clone(int argc, const char **argv, const char
> *prefix)
> transport_unlock_pack(transport);
> transport_disconnect(transport);
>
> - if (option_dissociate)
> + if (option_dissociate) {
> + struct packed_git *p;
> +
> + for (p = packed_git; p; p = p->next) {
> + close_pack_windows(p);
> + close_pack_index(p);
> + }
> dissociate_from_references();
> + }
This does not seem to close handles to the pack files
themseves, does Windows still allow removing the files? I
probably did not tried that, because I started from handles,
and discovered mapped files only later.
> junk_mode = JUNK_LEAVE_REPO;
> err = checkout();
> diff --git a/t/t5700-clone-reference.sh b/t/t5700-clone-reference.sh
> index ef1779f..2250ef4 100755
> --- a/t/t5700-clone-reference.sh
> +++ b/t/t5700-clone-reference.sh
> @@ -188,5 +188,26 @@ test_expect_success 'clone and dissociate from
> reference' '
> test_must_fail git -C R fsck &&
> git -C S fsck
> '
> +test_expect_success 'clone, dissociate from partial reference and repack' '
> + rm -fr P Q R &&
> + git init P &&
> + (
> + cd P &&
> + test_commit one &&
> + git repack &&
> + test_commit two &&
> + git repack
> + ) &&
> + git clone --bare P Q &&
> + (
> + cd P &&
> + git checkout -b second &&
> + test_commit three &&
> + git repack
> + ) &&
> + git clone --bare --dissociate --reference=P Q R &&
> + ls R/objects/pack/*.pack >packs.txt &&
> + test_line_count = 1 packs.txt
> +'
Unless it goes very lowlevel like running lsof of readin
proc testing this should always pass on Linux, even if the
issue is not fixed, maybe should be a conditional for
Windows only?
> test_done
> --
> 2.5.3.windows.1.3.gc322723
>
>
> --
> 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
--
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] am: configure gpg at startup
Renee Margaret McConahy writes:
> The new builtin am ignores the user.signingkey variable: gpg is being
> called with the committer details as the key ID, which may not be
> correct. git_gpg_config is responsible for handling that variable and is
> expected to be called on initialization by any modules that use gpg.
>
> Perhaps git_gpg_config's functionality ought to be merged into
> git_default_config, but this is simpler and in keeping with the current
> practice.
>
> Signed-off-by: Renee Margaret McConahy
> ---
Thanks.
I'd remove the unused "flags" (already pointed out by somebody, I
think), but otherwise the change looks sensible.
> builtin/am.c | 14 +-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/am.c b/builtin/am.c
> index 4f77e07..f0b0ffd 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -2208,6 +2208,18 @@ enum resume_mode {
> RESUME_ABORT
> };
>
> +static int git_am_config(const char *k, const char *v, void *cb)
> +{
> + int *flags = cb;
> + int status;
> +
> + status = git_gpg_config(k, v, NULL);
> + if (status)
> + return status;
> +
> + return git_default_config(k, v, NULL);
> +}
> +
> int cmd_am(int argc, const char **argv, const char *prefix)
> {
> struct am_state state;
> @@ -2308,7 +2320,7 @@ int cmd_am(int argc, const char **argv, const char
> *prefix)
> OPT_END()
> };
>
> - git_config(git_default_config, NULL);
> + git_config(git_am_config, NULL);
>
> am_state_init(&state, git_path("rebase-apply"));
--
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] rebase: accept indented comments (fixes regression)
Junio C Hamano writes:
> I am actually tempted to say that we should revert 804098b, which is
> the simplest fix.
>
> If we want "check everything before doing a single thing" mode, the
> right way to do it would be to base the check on the same loop as
> transform_todo_ids (one way to do so would be to give a third mode
> to that helper function, but I do not think we mind a small code
> duplication).
> ...
So here is a reroll, which is now minimally tested.
-- >8 --
Subject: [PATCH] rebase-i: loosen over-eager check_bad_cmd check
804098bb (git rebase -i: add static check for commands and SHA-1,
2015-06-29) tried to check all insns before running any in the todo
list, but it did so by implementing its own parser that is a lot
stricter than necessary. We used to allow lines that are indented
(including comment lines), and we used to allow a whitespace between
the insn and the commit object name to be HT, among other things,
that are flagged as an invalid line by mistake.
Fix this by using the same tokenizer that is used to parse the todo
list file in the new check. A new test is by Matthieu Moy.
Signed-off-by: Junio C Hamano
---
git-rebase--interactive.sh| 62 ---
t/t3404-rebase-interactive.sh | 10 +++
2 files changed, 39 insertions(+), 33 deletions(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index dcc3401..ae1806a 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -849,7 +849,8 @@ add_exec_commands () {
# Check if the SHA-1 passed as an argument is a
# correct one, if not then print $2 in "$todo".badsha
# $1: the SHA-1 to test
-# $2: the line to display if incorrect SHA-1
+# $2: the line number of the input
+# $3: the input filename
check_commit_sha () {
badsha=0
if test -z $1
@@ -865,9 +866,10 @@ check_commit_sha () {
if test $badsha -ne 0
then
+ line="$(sed -n -e "${2}p" "$3")"
warn "Warning: the SHA-1 is missing or isn't" \
"a commit in the following line:"
- warn " - $2"
+ warn " - $line"
warn
fi
@@ -878,37 +880,31 @@ check_commit_sha () {
# from the todolist in stdin
check_bad_cmd_and_sha () {
retval=0
- git stripspace --strip-comments |
- (
- while read -r line
- do
- IFS=' '
- set -- $line
- command=$1
- sha1=$2
-
- case $command in
- ''|noop|x|"exec")
- # Doesn't expect a SHA-1
- ;;
- pick|p|drop|d|reword|r|edit|e|squash|s|fixup|f)
- if ! check_commit_sha $sha1 "$line"
- then
- retval=1
- fi
- ;;
- *)
- warn "Warning: the command isn't recognized" \
- "in the following line:"
- warn " - $line"
- warn
+ lineno=0
+ while read -r command rest
+ do
+ lineno=$(( $lineno + 1 ))
+ case $command in
+ "$comment_char"*|''|noop|x|exec)
+ # Doesn't expect a SHA-1
+ ;;
+ pick|p|drop|d|reword|r|edit|e|squash|s|fixup|f)
+ if ! check_commit_sha "${rest%% *}" "$lineno" "$1"
+ then
retval=1
- ;;
- esac
- done
-
- return $retval
- )
+ fi
+ ;;
+ *)
+ line="$(sed -n -e "${lineno}p" "$1")"
+ warn "Warning: the command isn't recognized" \
+ "in the following line:"
+ warn " - $line"
+ warn
+ retval=1
+ ;;
+ esac
+ done <"$1"
+ return $retval
}
# Print the list of the SHA-1 of the commits
@@ -1002,7 +998,7 @@ check_todo_list () {
;;
esac
- if ! check_bad_cmd_and_sha <"$todo"
+ if ! check_bad_cmd_and_sha "$todo"
then
raise_error=t
fi
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index ebdab4b..2437a3c 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1206,6 +1206,16 @@ test_expect_success 'static check of bad command' '
test C = $(git cat-file commit HEAD^ | sed -ne \$p)
'
+test_expect_success 'indented comments are accepted' '
+ rebase_set
Re: [PATCH] rebase: accept indented comments (fixes regression)
Junio C Hamano writes: > Matthieu Moy writes: > >> With Git <2.0.6, 'git rebase' used to accept lines starting with >> whitespaces followed with '#' as a comment. This was broken by >> 804098b (git rebase -i: add static check for commands and SHA-1, >> 2015-06-29), which introduced additional checks on the TODO-list using >> "git stripspaces" which only strips comments starting at the first >> column. > > I cannot help thinking that this is sidestepping the real issue. > > The real issue, I think, is that the new check tokenises the input > differently from how the expand_todo_ids -> transform_todo_ids > callchain parses it. The problem Nazri Ramliy noticed about the new > check that does not ignore the indentation is merely one aspect of > it. Right. > Stripping leading whilespaces with sed may ignore indented anything > and help Nazri's script, but 804098b tightened checks to disallow > other things that we historically allowed, e.g. if you replace SP > between "pick" and the commit object name with an HT, the new check > will not notice that HT is also a perfectly good token separator > character and barfs. Indeed. I'm adding a test for that too. > I am actually tempted to say that we should revert 804098b, which is > the simplest fix. I think the commit has value, and reverting it makes the "drop" command essentially useless. > As far as I can tell, the hand-rolled parsing is there only in oder > to report the incoming $line as-is. Indeed, I remember finding the parsing code weird when I reviewed it, and the reason was to provide the exact line. > It is much easier to just identify with which line number the location > of the problem, and show it when it is necessary from the original > source, and we do not care about performance in the error codepath. Agreed. > Perhaps something along these lines instead, with your new tests > added in? Sounds good, yes. I'll send a patch with this and my updated tests. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- 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] Provide a dirname() function when NO_LIBGEN_H=YesPlease
Hi Johannes,
On 30/09/15 15:50, Johannes Schindelin wrote:
> When there is no `libgen.h` to our disposal, we miss the `dirname()`
> function.
>
> So far, we only had one user of that function: credential-cache--daemon
> (which was only compiled when Unix sockets are available, anyway). But
> now we also have `builtin/am.c` as user, so we need it.
Yes, many moons ago (on my old 32-bit laptop) when I was still 'working'
with MinGW I noticed this same thing while looking into providing a win32
emulation of unix sockets. So, I had to look into this at the same time.
Since this didn't progress, I didn't mention the libgen issue.
Anyway, I still have a 'test-libgen.c' file (attached) from back then that
contains some tests. I don't quite recall what the final state of this
code was, but it was intended to test _existing_ libgen implementations
as well as provide a 'git' version which would work on MinGW, cygwin and
linux. Note that some of the existing implementations didn't all agree on
what the tests should report! I don't remember if I looked at the POSIX
spec or not.
So, I don't know how useful it will be - if nothing else, there are some
tests! :-D
HTH
Ramsay Jones
>
> Since `dirname()` is a sibling of `basename()`, we simply put our very
> own `gitdirname()` implementation next to `gitbasename()` and use it
> if `NO_LIBGEN_H` has been set.
>
> Signed-off-by: Johannes Schindelin
> ---
>
> I stumbled over the compile warning when upgrading Git for Windows
> to 2.6.0. There was a left-over NO_LIBGEN_H=YesPlease (which we
> no longer need in Git for Windows 2.x), but it did point to the
> fact that we use `dirname()` in builtin/am.c now, so we better
> have a fall-back implementation for platforms without libgen.h.
>
> I tested this implementation a bit, but I still would appreciate
> a few eye-balls to go over it.
>
> compat/basename.c | 26 ++
> git-compat-util.h | 2 ++
> 2 files changed, 28 insertions(+)
>
> diff --git a/compat/basename.c b/compat/basename.c
> index d8f8a3c..10dba38 100644
> --- a/compat/basename.c
> +++ b/compat/basename.c
> @@ -13,3 +13,29 @@ char *gitbasename (char *path)
> }
> return (char *)base;
> }
> +
> +char *gitdirname(char *path)
> +{
> + char *p = path, *slash, c;
> +
> + /* Skip over the disk name in MSDOS pathnames. */
> + if (has_dos_drive_prefix(p))
> + p += 2;
> + /* POSIX.1-2001 says dirname("/") should return "/" */
> + slash = is_dir_sep(*p) ? ++p : NULL;
> + while ((c = *(p++)))
> + if (is_dir_sep(c)) {
> + char *tentative = p - 1;
> +
> + /* POSIX.1-2001 says to ignore trailing slashes */
> + while (is_dir_sep(*p))
> + p++;
> + if (*p)
> + slash = tentative;
> + }
> +
> + if (!slash)
> + return ".";
> + *slash = '\0';
> + return path;
> +}
> diff --git a/git-compat-util.h b/git-compat-util.h
> index f649e81..8b01aa5 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -253,6 +253,8 @@ struct itimerval {
> #else
> #define basename gitbasename
> extern char *gitbasename(char *);
> +#define dirname gitdirname
> +extern char *gitdirname(char *);
> #endif
>
> #ifndef NO_ICONV
>
#include
#include
#include
#ifndef NO_LIBGEN_H
# include
#endif
struct test_data {
char *from; /* input: transform from this ... */
char *to;/* output: ... to this.*/
};
#ifdef NO_LIBGEN_H
#if defined(__MINGW32__) || defined(_MSC_VER)
#define has_dos_drive_prefix(path) (isalpha(*(path)) && (path)[1] == ':')
#define is_dir_sep(c) ((c) == '/' || (c) == '\\')
#else
#define has_dos_drive_prefix(path) 0
#define is_dir_sep(c) ((c) == '/')
#endif
#define basename gitbasename
#define dirname gitdirname
char *gitbasename (char *path)
{
char *p;
if (!path || !*path)
return ".";
/* skip drive designator, if any */
if (has_dos_drive_prefix(path))
path += 2;
if (!*path)
return ".";
/* trim trailing directory separators */
p = path + strlen(path) - 1;
while (is_dir_sep(*p)) {
if (p == path)
return path;
*p-- = '\0';
}
/* find begining of last path component */
while (p >= path && !is_dir_sep(*p))
p--;
return p + 1;
}
char *gitdirname(char *path)
{
char *p, *start;
if (!path || !*path)
return ".";
start = path;
/* skip drive designator, if any */
if (has_dos_drive_prefix(path))
start += 2;
/* check for // */
if (strcmp(start, "//") == 0)
return path;
/* check for \\ */
if (is_dir_sep('\\') && strcmp(start, "") == 0)
return path;
/* trim trailing directory separators */
p = path + strlen(path) - 1;
while (is_dir_sep(*p)) {
if (p == start)
return path;
*p-- = '\0';
}
/* find begining of last path component */
while (p >= start && !is_dir_sep(*p))
p--;
/* terminate dirname */
if (p < start) {
Re: [PATCH] am: configure gpg at startup
On Wed, Sep 30, 2015 at 1:49 PM, Renee Margaret McConahy
wrote:
> The new builtin am ignores the user.signingkey variable: gpg is being
> called with the committer details as the key ID, which may not be
> correct. git_gpg_config is responsible for handling that variable and is
> expected to be called on initialization by any modules that use gpg.
>
> Perhaps git_gpg_config's functionality ought to be merged into
> git_default_config, but this is simpler and in keeping with the current
> practice.
>
> Signed-off-by: Renee Margaret McConahy
> ---
> diff --git a/builtin/am.c b/builtin/am.c
> index 4f77e07..f0b0ffd 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -2208,6 +2208,18 @@ enum resume_mode {
> RESUME_ABORT
> };
>
> +static int git_am_config(const char *k, const char *v, void *cb)
> +{
> + int *flags = cb;
'flags' seems to be unused.
> + int status;
> +
> + status = git_gpg_config(k, v, NULL);
> + if (status)
> + return status;
> +
> + return git_default_config(k, v, NULL);
> +}
> +
> int cmd_am(int argc, const char **argv, const char *prefix)
> {
> struct am_state state;
> @@ -2308,7 +2320,7 @@ int cmd_am(int argc, const char **argv, const char
> *prefix)
> OPT_END()
> };
>
> - git_config(git_default_config, NULL);
> + git_config(git_am_config, NULL);
>
> am_state_init(&state, git_path("rebase-apply"));
>
> --
> 2.6.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
[PATCH] am: configure gpg at startup
The new builtin am ignores the user.signingkey variable: gpg is being
called with the committer details as the key ID, which may not be
correct. git_gpg_config is responsible for handling that variable and is
expected to be called on initialization by any modules that use gpg.
Perhaps git_gpg_config's functionality ought to be merged into
git_default_config, but this is simpler and in keeping with the current
practice.
Signed-off-by: Renee Margaret McConahy
---
builtin/am.c | 14 +-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/builtin/am.c b/builtin/am.c
index 4f77e07..f0b0ffd 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -2208,6 +2208,18 @@ enum resume_mode {
RESUME_ABORT
};
+static int git_am_config(const char *k, const char *v, void *cb)
+{
+ int *flags = cb;
+ int status;
+
+ status = git_gpg_config(k, v, NULL);
+ if (status)
+ return status;
+
+ return git_default_config(k, v, NULL);
+}
+
int cmd_am(int argc, const char **argv, const char *prefix)
{
struct am_state state;
@@ -2308,7 +2320,7 @@ int cmd_am(int argc, const char **argv, const char
*prefix)
OPT_END()
};
- git_config(git_default_config, NULL);
+ git_config(git_am_config, NULL);
am_state_init(&state, git_path("rebase-apply"));
--
2.6.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: [PATCH 6/8] run-command: add an asynchronous parallel child processor
Junio C Hamano writes: > I may have comments on other parts of this patch, but I noticed this > a bit hard to read while reading the end result. > ... I finished reading the remainder. Other than the above all look sensible. Will replace what had been queued. 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 6/8] run-command: add an asynchronous parallel child processor
On Tue, Sep 29, 2015 at 8:12 PM, Junio C Hamano wrote:
> Stefan Beller writes:
>
>> + while (1) {
>> + int i;
>> + int output_timeout = 100;
>> + int spawn_cap = 4;
>> +
>> + if (!no_more_task) {
>> + for (i = 0; i < spawn_cap; i++) {
>> + int code;
>> + if (pp->nr_processes == pp->max_processes)
>> + break;
>> +
>> + code = pp_start_one(pp);
>> + if (!code)
>> + continue;
>> + if (code < 0) {
>> + pp->shutdown = 1;
>> + kill_children(pp, SIGTERM);
>> + }
>> + no_more_task = 1;
>> + break;
>> + }
>> + }
>> + if (no_more_task && !pp->nr_processes)
>> + break;
>
> I may have comments on other parts of this patch, but I noticed this
> a bit hard to read while reading the end result.
>
> Losing the outer "if (!no_more_task)" and replacing the above with
>
> for (no_more_task = 0, i = 0;
> !no_more_task && i < spawn_cap;
> i++) {
> ... do things that may or may not set
> ... no_more_task
> }
> if (no_more_task && ...)
> break;
>
> would make it clear that regardless of spawn_cap, no_more_task is
> honored.
>
> Also I think that having the outer "if (!no_more_task)" and not
> having "no_more_task = 0" after each iteration is buggy. Even when
> next_task() told start_one() that it does not have more tasks for
> now, as long as there are running processes, it is entirely plausible
> that next call to next_task() can return "now we have some more task
> to do".
>
> Although I think it would make it unsightly, if you want to have the
> large indentation that protects the spawn_cap loop from getting
> entered, the condition would be
>
> if (!pp->shutdown) {
> for (... spawn_cap loop ...) {
> ...
> }
> }
>
> That structure could make sense. But even then I would probably
> write it more like
>
> ...
> int spawn_cap = 4;
>
> pp = pp_init(...);
> while (1) {
> int no_more_task = 0;
>
> for (i = 0;
> !no_more_task && !pp->shutdown && i < spawn_cap;
> i++) {
> ...
> code = start_one();
> ... set no_more_task to 1 as needed
> ... set pp->shutdown to 1 as needed
> }
> if (no_more_task && !pp->nr_processes)
> break;
> buffer_stderr(...);
> output(...);
> collect(...);
> }
That is indeed better to read.
Though if we reset `no_more_task` each iteration of the loop by
having its declaration inside the loop, the condition for exiting the
loop needs to be:
if ((no_more_task || pp->shutdown) && !pp->nr_processes)
break;
for correctness.
When looking at that condition, I realized that I implicitly assumed
the workflow, where get_next_task returns 0 intermittently, to be a
second class citizen. I reworded the documentation as well there.
>
> That is, you need to have two independent conditions that tell you
> not to spawn any new task:
>
> (1) You called start_one() repeatedly and next_task() said "nothing
> more for now", so you know calling start_one() one more time
> without changing other conditions (like draining output from
> running processes and culling finished ones) will not help.
>
> Letting other parts of the application that uses this scheduler
> loop (i.e. drain output, cull finished process, etc.) may
> change the situation and you _do_ need to call start_one() when
> the next_task() merely said "nothing more for now".
>
> That is what no_more_task controls.
>
> (2) The application said "I want the system to be gracefully shut
> down". next_task() may also have said "nothing more for now"
> and you may have set no_more_task in response to it, but unlike
> (1) above, draining and culling must be done only to shut the
> system down, the application does not want new processes to be
> added. You do not want to enter the spawn_cap loop when it
> happens.
>
> That is what pp->shutdown controls.
>
--
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] Provide a dirname() function when NO_LIBGEN_H=YesPlease
Johannes Schindelin writes:
> I stumbled over the compile warning when upgrading Git for Windows
> to 2.6.0. There was a left-over NO_LIBGEN_H=YesPlease (which we
> no longer need in Git for Windows 2.x), but it did point to the
> fact that we use `dirname()` in builtin/am.c now, so we better
> have a fall-back implementation for platforms without libgen.h.
Thanks for being careful.
>
> I tested this implementation a bit, but I still would appreciate
> a few eye-balls to go over it.
>
> compat/basename.c | 26 ++
> git-compat-util.h | 2 ++
> 2 files changed, 28 insertions(+)
>
> diff --git a/compat/basename.c b/compat/basename.c
> index d8f8a3c..10dba38 100644
> --- a/compat/basename.c
> +++ b/compat/basename.c
> @@ -13,3 +13,29 @@ char *gitbasename (char *path)
> }
> return (char *)base;
> }
> +
> +char *gitdirname(char *path)
> +{
> + char *p = path, *slash, c;
> +
> + /* Skip over the disk name in MSDOS pathnames. */
> + if (has_dos_drive_prefix(p))
> + p += 2;
Not a new problem, but many callers of has_dos_drive_prefix()
hardcodes that "2" in various forms. I wonder if this is something
we should relieve callers of by tweaking the semantics of it, e.g.
by returning 2 (or howmanyever bytes should be skipped) from the
function, changing it to skip_dos_drive_prefix(&p), etc.
> + /* POSIX.1-2001 says dirname("/") should return "/" */
> + slash = is_dir_sep(*p) ? ++p : NULL;
> + while ((c = *(p++)))
I am confused by this. What is the invariant on 'p' at the
beginning of the body of this while loop in each iteration?
Inside the body, p skips over dir-sep characters, so p must point at
the byte past the last run of slashes?
If that is the invariant, upon entry, shouldn't the initialization
of "slash" be skipping over all slashes, not just the first one,
when the input is "///foo", for example? Instead the above skips '/'
and sets slash to the byte past the first '/' (which is OK because
you want to NUL-terminate to remove "//foo" from the input) but does
not move p to 'f', so the invariant is not "p must point at the byte
past the last run of slashes".
> + if (is_dir_sep(c)) {
> + char *tentative = p - 1;
> +
> + /* POSIX.1-2001 says to ignore trailing slashes */
> + while (is_dir_sep(*p))
> + p++;
> + if (*p)
> + slash = tentative;
> + }
I would have expected the function to scan from the end/right/tail.
> + if (!slash)
> + return ".";
> + *slash = '\0';
> + return path;
> +}
> diff --git a/git-compat-util.h b/git-compat-util.h
> index f649e81..8b01aa5 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -253,6 +253,8 @@ struct itimerval {
> #else
> #define basename gitbasename
> extern char *gitbasename(char *);
> +#define dirname gitdirname
> +extern char *gitdirname(char *);
> #endif
>
> #ifndef NO_ICONV
--
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] rebase: accept indented comments (fixes regression)
Matthieu Moy writes:
> With Git <2.0.6, 'git rebase' used to accept lines starting with
> whitespaces followed with '#' as a comment. This was broken by
> 804098b (git rebase -i: add static check for commands and SHA-1,
> 2015-06-29), which introduced additional checks on the TODO-list using
> "git stripspaces" which only strips comments starting at the first
> column.
I cannot help thinking that this is sidestepping the real issue.
The real issue, I think, is that the new check tokenises the input
differently from how the expand_todo_ids -> transform_todo_ids
callchain parses it. The problem Nazri Ramliy noticed about the new
check that does not ignore the indentation is merely one aspect of
it.
Stripping leading whilespaces with sed may ignore indented anything
and help Nazri's script, but 804098b tightened checks to disallow
other things that we historically allowed, e.g. if you replace SP
between "pick" and the commit object name with an HT, the new check
will not notice that HT is also a perfectly good token separator
character and barfs.
I am actually tempted to say that we should revert 804098b, which is
the simplest fix.
If we want "check everything before doing a single thing" mode, the
right way to do it would be to base the check on the same loop as
transform_todo_ids (one way to do so would be to give a third mode
to that helper function, but I do not think we mind a small code
duplication).
As far as I can tell, the hand-rolled parsing is there only in oder
to report the incoming $line as-is. It is much easier to just
identify with which line number the location of the problem, and
show it when it is necessary from the original source, and we do not
care about performance in the error codepath.
Perhaps something along these lines instead, with your new tests
added in?
git-rebase--interactive.sh | 62 ++
1 file changed, 29 insertions(+), 33 deletions(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index dcc3401..ae1806a 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -849,7 +849,8 @@ add_exec_commands () {
# Check if the SHA-1 passed as an argument is a
# correct one, if not then print $2 in "$todo".badsha
# $1: the SHA-1 to test
-# $2: the line to display if incorrect SHA-1
+# $2: the line number of the input
+# $3: the input filename
check_commit_sha () {
badsha=0
if test -z $1
@@ -865,9 +866,10 @@ check_commit_sha () {
if test $badsha -ne 0
then
+ line="$(sed -n -e "${2}p" "$3")"
warn "Warning: the SHA-1 is missing or isn't" \
"a commit in the following line:"
- warn " - $2"
+ warn " - $line"
warn
fi
@@ -878,37 +880,31 @@ check_commit_sha () {
# from the todolist in stdin
check_bad_cmd_and_sha () {
retval=0
- git stripspace --strip-comments |
- (
- while read -r line
- do
- IFS=' '
- set -- $line
- command=$1
- sha1=$2
-
- case $command in
- ''|noop|x|"exec")
- # Doesn't expect a SHA-1
- ;;
- pick|p|drop|d|reword|r|edit|e|squash|s|fixup|f)
- if ! check_commit_sha $sha1 "$line"
- then
- retval=1
- fi
- ;;
- *)
- warn "Warning: the command isn't recognized" \
- "in the following line:"
- warn " - $line"
- warn
+ lineno=0
+ while read -r command rest
+ do
+ lineno=$(( $lineno + 1 ))
+ case $command in
+ "$comment_char"*|''|noop|x|exec)
+ # Doesn't expect a SHA-1
+ ;;
+ pick|p|drop|d|reword|r|edit|e|squash|s|fixup|f)
+ if ! check_commit_sha "${rest%% *}" "$lineno" "$1"
+ then
retval=1
- ;;
- esac
- done
-
- return $retval
- )
+ fi
+ ;;
+ *)
+ line="$(sed -n -e "${lineno}p" "$1")"
+ warn "Warning: the command isn't recognized" \
+ "in the following line:"
+ warn " - $line"
+ warn
+ retval=1
+ ;;
+ esac
+ done <"$1"
+ return $retval
}
# Print the list of the SHA-1 o
Re: [PATCH] git-send-email.perl: Fixed sending of many/huge changes/patches
Lars Wendler writes:
> It seems to me that there is a size limit, after cutting down the patch
> to ~16K, sending started to work. I cut it twice, once by removing lines
> from the head and once from the bottom, in both cases at the size of
> around 16K I could send the patch.
>
> See also original report:
> http://permalink.gmane.org/gmane.comp.version-control.git/274569
>
> Reported-by: Juston Li
> Tested-by: Markos Chandras
> Signed-off-by: Lars Wendler
> ---
> git-send-email.perl | 6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/git-send-email.perl b/git-send-email.perl
> index e3ff44b..e907e0ea 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -1365,7 +1365,11 @@ Message-Id: $message_id
> $smtp->mail( $raw_from ) or die $smtp->message;
> $smtp->to( @recipients ) or die $smtp->message;
> $smtp->data or die $smtp->message;
> - $smtp->datasend("$header\n$message") or die $smtp->message;
> + $smtp->datasend("$header\n") or die $smtp->message;
> + my @lines = split /^/, $message;
> + foreach my $line (@lines) {
> + $smtp->datasend("$line") or die $smtp->message;
> + }
Thanks. One and a half comments.
* If 16k is the limit, and smtp payload line limit is much much
shorter than that, is it sensible to send data line by line?
* Has this been reported to Net::Cmd::datasend() upstream?
> $smtp->dataend() or die $smtp->message;
> $smtp->code =~ /250|200/ or die "Failed to send
> $subject\n".$smtp->message;
> }
--
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] Provide a dirname() function when NO_LIBGEN_H=YesPlease
When there is no `libgen.h` to our disposal, we miss the `dirname()`
function.
So far, we only had one user of that function: credential-cache--daemon
(which was only compiled when Unix sockets are available, anyway). But
now we also have `builtin/am.c` as user, so we need it.
Since `dirname()` is a sibling of `basename()`, we simply put our very
own `gitdirname()` implementation next to `gitbasename()` and use it
if `NO_LIBGEN_H` has been set.
Signed-off-by: Johannes Schindelin
---
I stumbled over the compile warning when upgrading Git for Windows
to 2.6.0. There was a left-over NO_LIBGEN_H=YesPlease (which we
no longer need in Git for Windows 2.x), but it did point to the
fact that we use `dirname()` in builtin/am.c now, so we better
have a fall-back implementation for platforms without libgen.h.
I tested this implementation a bit, but I still would appreciate
a few eye-balls to go over it.
compat/basename.c | 26 ++
git-compat-util.h | 2 ++
2 files changed, 28 insertions(+)
diff --git a/compat/basename.c b/compat/basename.c
index d8f8a3c..10dba38 100644
--- a/compat/basename.c
+++ b/compat/basename.c
@@ -13,3 +13,29 @@ char *gitbasename (char *path)
}
return (char *)base;
}
+
+char *gitdirname(char *path)
+{
+ char *p = path, *slash, c;
+
+ /* Skip over the disk name in MSDOS pathnames. */
+ if (has_dos_drive_prefix(p))
+ p += 2;
+ /* POSIX.1-2001 says dirname("/") should return "/" */
+ slash = is_dir_sep(*p) ? ++p : NULL;
+ while ((c = *(p++)))
+ if (is_dir_sep(c)) {
+ char *tentative = p - 1;
+
+ /* POSIX.1-2001 says to ignore trailing slashes */
+ while (is_dir_sep(*p))
+ p++;
+ if (*p)
+ slash = tentative;
+ }
+
+ if (!slash)
+ return ".";
+ *slash = '\0';
+ return path;
+}
diff --git a/git-compat-util.h b/git-compat-util.h
index f649e81..8b01aa5 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -253,6 +253,8 @@ struct itimerval {
#else
#define basename gitbasename
extern char *gitbasename(char *);
+#define dirname gitdirname
+extern char *gitdirname(char *);
#endif
#ifndef NO_ICONV
--
2.5.3.windows.1.3.gc322723
--
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: Gitk cannot start in the latest version when using --all
Konstantin Khomoutov 007spb.ru> writes:
>
> On Tue, 29 Sep 2015 15:51:46 +0200
> Christophe COEVOET notk.org> wrote:
>
> > >> I'm installing git and gitk from the Ubuntu PPA maintained by the
> > >> Git team. I received the Git 2.6 update today.
> > >> Since this update, I'm unable to launch gitk with --all. I'm
> > >> receiving the following error message:
> > >>
> > >> Error in startup script: bad menu entry index "Éditer la vue..."
> > >> while executing
> > >> ".bar.view entryconf [mca "Edit view..."] -state normal"
> > >> invoked from within
> > >> "if {$cmdline_files ne {} || $revtreeargs ne {} || $revtreeargscmd
> > >> ne {}} {
> > >> # create a view for the files/dirs specified on the command
> > >> # line
> > >> se..."
> > >> (file "/usr/bin/gitk" line 12442)
> [...]
> > > Does it start if you run it via
> > >
> > > $ LANG=C gitk --all
> > >
> > > or
> > >
> > > $ LANG=en_US.UTF-8 gitk --all
> > >
> > > ?
> > Yeah, both of these commands are working fine.
>
> OK, so the problem is that some menu entry has the title "Edit view..."
> which is for some reason not translated for your current locale,
> and then some code tries to locate it using the original title
> translated to French -- I fathom the command [mca] being called here
> stands for "Message Catalog A-something"
>
Hello, I notice the same problem (updated to gitk 2.6.0 on openSUSE)
It is not only a problem with the German translation but also with others, I
additionally tried French and Japanese.
But even stranger, gitk starts correctly, when no command line arguments are
given in these locales (and the German locale), but with any command line
argument I tried (e.g. only 'gitk .') it crashes with the error message
mentioned above (they differ in the text 'bad menu entry index "Éditer la
vue..."', there the translated text of the locale is shown)
Furthermore I checked the translation files, there seems to be an
appropriate entry for "Edit view..." for all languages.
N�r��yb�X��ǧv�^�){.n�+ا���ܨ}���Ơz�&j:+v���zZ+��+zf���h���~i���z� �w���?�&�)ߢf
Re: gitk crashes with german translation
On Wed, 30 Sep 2015 09:58:14 + (UTC) Peter Vasil wrote: > When I try to run "gitk --all" on Mac with German language settings I > get the following error: > Error in startup script: bad menu entry index "Ansicht bearbeiten ..." > while executing > ".bar.view entryconf [mca "Edit view..."] -state normal" [...] http://thread.gmane.org/gmane.comp.version-control.git/278820 -- 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
gitk crashes with german translation
Hi list,
When I try to run "gitk --all" on Mac with German language settings I get
the following error:
Error in startup script: bad menu entry index "Ansicht bearbeiten ..."
while executing
".bar.view entryconf [mca "Edit view..."] -state normal"
invoked from within
"if {$cmdline_files ne {} || $revtreeargs ne {} || $revtreeargscmd ne {}} {
# create a view for the files/dirs specified on the command line
se..."
(file "/usr/local/bin/gitk" line 12442)
I use git version 2.6.0
If I remove the folder with translations in order to use the english version,
gitk does not crash.
Cheers,
Peter
--
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] rebase: accept indented comments (fixes regression)
Matthieu Moy writes:
> With Git <2.0.6, 'git rebase' used to accept lines starting with
> whitespaces followed with '#' as a comment. This was broken by
> 804098b (git rebase -i: add static check for commands and SHA-1,
> 2015-06-29), which introduced additional checks on the TODO-list using
> "git stripspaces" which only strips comments starting at the first
> column.
>
> Whether it's a good thing to accept indented comments is
> debatable (other commands like "git commit" do not accept them), but we
> already accepted them in the past, and some people and scripts rely on
> this behavior. Also, a line starting with space followed by a '#' cannot
> have any meaning other than being a comment, hence it doesn't harm to
> accept them as comments.
>
> Signed-off-by: Matthieu Moy
Thank you for the patch, and sorry for the introduced regression.
Rémi
> ---
> Junio C Hamano writes:
>
> > Junio C Hamano writes:
> >
> >> I know you alluded to preprocess what is fed to stripspace, but I
> >> wonder if we can remove the misguided call to stripspace in the
> >> first place and do something like the attached instead.
> >>
> >> git-rebase--interactive.sh | 3 +--
> >> 1 file changed, 1 insertion(+), 2 deletions(-)
> >>
> >> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> >> index f01637b..a64f77a 100644
> >> --- a/git-rebase--interactive.sh
> >> +++ b/git-rebase--interactive.sh
> >> @@ -886,7 +886,6 @@ check_commit_sha () {
> >> # from the todolist in stdin
> >> check_bad_cmd_and_sha () {
> >> retval=0
> >> - git stripspace --strip-comments |
> >> (
> >> while read -r line
> >> do
> >> @@ -896,7 +895,7 @@ check_bad_cmd_and_sha () {
> >> sha1=$2
> >>
> >> case $command in
> >> - ''|noop|x|"exec")
> >> + '#'*|''|noop|x|"exec")
> >> # Doesn't expect a SHA-1
> >> ;;
> >> pick|p|drop|d|reword|r|edit|e|squash|s|fixup|f)
> >
> > Nah, that would not work, as I misread the "split only at SP" manual
> > parsing of $line.
>
> OK, let's go for the solution I seem to be able to get right even with
> low cafeine ;-).
>
> git-rebase--interactive.sh | 3 +++
> t/t3404-rebase-interactive.sh | 10 ++
> 2 files changed, 13 insertions(+)
>
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index f01637b..55adf78 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -886,6 +886,9 @@ check_commit_sha () {
> # from the todolist in stdin
> check_bad_cmd_and_sha () {
> retval=0
> + # git rebase -i accepts comments preceeded by spaces, while
> + # stripspace does not.
> + sed 's/^[[:space:]]*//' |
> git stripspace --strip-comments |
> (
> while read -r line
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index d26e3f5..ac5bac3 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -1227,6 +1227,16 @@ test_expect_success 'static check of bad command' '
> test C = $(git cat-file commit HEAD^ | sed -ne \$p)
> '
>
> +test_expect_success 'indented comments are accepted' '
> + rebase_setup_and_clean indented-comment &&
> + write_script add-indent.sh <<-\EOF &&
> + printf "\n \t # comment\n" >>$1
> + EOF
> + test_set_editor "$(pwd)/add-indent.sh" &&
> + git rebase -i HEAD^ &&
> + test E = $(git cat-file commit HEAD | sed -ne \$p)
> +'
> +
> cat >expect < Warning: the SHA-1 is missing or isn't a commit in the following line:
> - edit XXX False commit
> --
> 2.6.0.rc2.24.g231a9a1.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: [PATCH] git-send-email.perl: Fixed sending of many/huge changes/patches
Hi Lars, On 2015-09-30 09:26, Lars Wendler wrote: > From: Stefan Agner > > Sometimes sending huge patches/commits fail with > > [Net::SMTP::SSL] Connection closed at /usr/lib/git-core/git-send-email > line 1320. > > Running the command with --smtp-debug=1 yields to > > Net::SMTP::SSL: Net::Cmd::datasend(): unexpected EOF on command channel: > at /usr/lib/git-core/git-send-email line 1320. > [Net::SMTP::SSL] Connection closed at /usr/lib/git-core/git-send-email > line 1320. > > Stefan described it in his mail like this: > > It seems to me that there is a size limit, after cutting down the patch > to ~16K, sending started to work. I cut it twice, once by removing lines > from the head and once from the bottom, in both cases at the size of > around 16K I could send the patch. > > See also original report: > http://permalink.gmane.org/gmane.comp.version-control.git/274569 > > Reported-by: Juston Li > Tested-by: Markos Chandras > Signed-off-by: Lars Wendler > --- Very nice! Thank you, Dscho -- 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] rebase: accept indented comments (fixes regression)
With Git <2.0.6, 'git rebase' used to accept lines starting with
whitespaces followed with '#' as a comment. This was broken by
804098b (git rebase -i: add static check for commands and SHA-1,
2015-06-29), which introduced additional checks on the TODO-list using
"git stripspaces" which only strips comments starting at the first
column.
Whether it's a good thing to accept indented comments is
debatable (other commands like "git commit" do not accept them), but we
already accepted them in the past, and some people and scripts rely on
this behavior. Also, a line starting with space followed by a '#' cannot
have any meaning other than being a comment, hence it doesn't harm to
accept them as comments.
Signed-off-by: Matthieu Moy
---
Junio C Hamano writes:
> Junio C Hamano writes:
>
>> I know you alluded to preprocess what is fed to stripspace, but I
>> wonder if we can remove the misguided call to stripspace in the
>> first place and do something like the attached instead.
>>
>> git-rebase--interactive.sh | 3 +--
>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
>> index f01637b..a64f77a 100644
>> --- a/git-rebase--interactive.sh
>> +++ b/git-rebase--interactive.sh
>> @@ -886,7 +886,6 @@ check_commit_sha () {
>> # from the todolist in stdin
>> check_bad_cmd_and_sha () {
>> retval=0
>> -git stripspace --strip-comments |
>> (
>> while read -r line
>> do
>> @@ -896,7 +895,7 @@ check_bad_cmd_and_sha () {
>> sha1=$2
>>
>> case $command in
>> -''|noop|x|"exec")
>> +'#'*|''|noop|x|"exec")
>> # Doesn't expect a SHA-1
>> ;;
>> pick|p|drop|d|reword|r|edit|e|squash|s|fixup|f)
>
> Nah, that would not work, as I misread the "split only at SP" manual
> parsing of $line.
OK, let's go for the solution I seem to be able to get right even with
low cafeine ;-).
git-rebase--interactive.sh| 3 +++
t/t3404-rebase-interactive.sh | 10 ++
2 files changed, 13 insertions(+)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index f01637b..55adf78 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -886,6 +886,9 @@ check_commit_sha () {
# from the todolist in stdin
check_bad_cmd_and_sha () {
retval=0
+ # git rebase -i accepts comments preceeded by spaces, while
+ # stripspace does not.
+ sed 's/^[[:space:]]*//' |
git stripspace --strip-comments |
(
while read -r line
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index d26e3f5..ac5bac3 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1227,6 +1227,16 @@ test_expect_success 'static check of bad command' '
test C = $(git cat-file commit HEAD^ | sed -ne \$p)
'
+test_expect_success 'indented comments are accepted' '
+ rebase_setup_and_clean indented-comment &&
+ write_script add-indent.sh <<-\EOF &&
+ printf "\n \t # comment\n" >>$1
+ EOF
+ test_set_editor "$(pwd)/add-indent.sh" &&
+ git rebase -i HEAD^ &&
+ test E = $(git cat-file commit HEAD | sed -ne \$p)
+'
+
cat >expect
[PATCH] git-send-email.perl: Fixed sending of many/huge changes/patches
From: Stefan Agner
Sometimes sending huge patches/commits fail with
[Net::SMTP::SSL] Connection closed at /usr/lib/git-core/git-send-email
line 1320.
Running the command with --smtp-debug=1 yields to
Net::SMTP::SSL: Net::Cmd::datasend(): unexpected EOF on command channel:
at /usr/lib/git-core/git-send-email line 1320.
[Net::SMTP::SSL] Connection closed at /usr/lib/git-core/git-send-email
line 1320.
Stefan described it in his mail like this:
It seems to me that there is a size limit, after cutting down the patch
to ~16K, sending started to work. I cut it twice, once by removing lines
from the head and once from the bottom, in both cases at the size of
around 16K I could send the patch.
See also original report:
http://permalink.gmane.org/gmane.comp.version-control.git/274569
Reported-by: Juston Li
Tested-by: Markos Chandras
Signed-off-by: Lars Wendler
---
git-send-email.perl | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/git-send-email.perl b/git-send-email.perl
index e3ff44b..e907e0ea 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1365,7 +1365,11 @@ Message-Id: $message_id
$smtp->mail( $raw_from ) or die $smtp->message;
$smtp->to( @recipients ) or die $smtp->message;
$smtp->data or die $smtp->message;
- $smtp->datasend("$header\n$message") or die $smtp->message;
+ $smtp->datasend("$header\n") or die $smtp->message;
+ my @lines = split /^/, $message;
+ foreach my $line (@lines) {
+ $smtp->datasend("$line") or die $smtp->message;
+ }
$smtp->dataend() or die $smtp->message;
$smtp->code =~ /250|200/ or die "Failed to send
$subject\n".$smtp->message;
}
--
2.6.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
