Re: [RFC/PATCH v1] Add Travis CI support
Dennis Kaarsemaker writes: > On zo, 2015-10-04 at 10:46 -0700, Junio C Hamano wrote: >> One final question. Which configuration file does the CI use when >> running a PR-initiated test? The one already in the repository >> i.e. the target of the proposed pull, or the one that is possibly >> updated by the PR? >> >> I am wondering if that can be an avenue for a possible mischief. > > The latter. And it can, as it can enable notifications. OK, so an attacker can send emails (by faking one of the repository owner's identity on a commit, and then submitting a pull-request for this commit). But such attacker could already send emails via GitHub to all repository watchers (not just owners) by sending pull-requests. Or by using his mailer. Other than that, Travis-CI uses a container-based infrastructure to ensure clean and independent builds. So, an attacker could trigger a build doing "rm -fr /" or whatever without impacting other builds. -- 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 3/9] ref-filter: add support for %(path) atom
Junio C Hamano writes: > Matthieu Moy writes: > > This adds %(path) and %(path:short) atoms. The %(path) atom will print > the path of the given ref, while %(path:short) will only print the > subdirectory of the given ref. What does "path" mean in this context? How is it different from %(refname)? I found the answer below, but I could not guess from the doc and commit message. Actually, I'm not sure %(path) is the right name. If you want the "file/directory" analogy, then %(dirname) would be better. >>> >>> Noted will change. >> >> Note: I don't completely like %(dirname) either. I'm convinced it's >> better than %(path), but there may be a better option. > > Is that a derived form of the refname, just like %(refname:short) > that is 'master' for a ref whose %(refname) is 'refs/heads/master' > is a derived form of %(refname), and ":short" is what tells the > formatting machinery what kind of derivation is desired? > > If so would %(refname:dir) & %(refname:base) be more in line with > the overall structure? Yes, indeed much better. It's still about the refnames, so a specialized version of %(refname) makes much more sense than a new atom. -- 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/RFC 1/2] sha1_file: close all pack files after running
On Sun, Oct 04, 2015 at 04:53:30PM +0200, Johannes Schindelin wrote: > I guess then we would need two different patches for the > two different fixes, at least. > > So now I am unsure how to proceed: I do not want to step > on your toes, but I also want to see my use case fixed and > I want to move forward on this. At the moment, it looks as > if we are at an impasse. Just continue with your patches then. I do not hurry. -- 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 v2 02/43] refs: make repack_without_refs and is_branch public
On 09/29/2015 12:01 AM, David Turner wrote: > Signed-off-by: Ronnie Sahlberg > Signed-off-by: David Turner > --- > refs.c | 9 + > refs.h | 13 + > 2 files changed, 14 insertions(+), 8 deletions(-) > > diff --git a/refs.c b/refs.c > index ce551e9..2741cc5 100644 > --- a/refs.c > +++ b/refs.c > @@ -2826,14 +2826,7 @@ int pack_refs(unsigned int flags) > return 0; > } > > -/* > - * Rewrite the packed-refs file, omitting any refs listed in > - * 'refnames'. On error, leave packed-refs unchanged, write an error > - * message to 'err', and return a nonzero value. > - * > - * The refs in 'refnames' needn't be sorted. `err` must not be NULL. > - */ > -static int repack_without_refs(struct string_list *refnames, struct strbuf > *err) > +int repack_without_refs(struct string_list *refnames, struct strbuf *err) I looked for the corresponding change to remove `static` from is_branch(). Apparently that function already had external linkage, even though it was not listed in the header file. As a convenience to readers you might note that peculiarity in the commit message. > [...] Michael -- Michael Haggerty mhag...@alum.mit.edu -- 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 v2 01/43] refs.c: create a public version of verify_refname_available
On 09/29/2015 12:01 AM, David Turner wrote:
> From: Ronnie Sahlberg
>
> Create a public version of verify_refname_available that backends can
> provide.
>
> Signed-off-by: Ronnie Sahlberg
> Signed-off-by: David Turner
> ---
> refs.c | 51 +--
> refs.h | 17 +
> 2 files changed, 46 insertions(+), 22 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index 132eff5..ce551e9 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -279,7 +279,7 @@ struct ref_dir {
> * presence of an empty subdirectory does not block the creation of a
> * similarly-named reference. (The fact that reference names with the
> * same leading components can conflict *with each other* is a
> - * separate issue that is regulated by verify_refname_available().)
> + * separate issue that is regulated by verify_refname_available_dir().)
> *
> * Please note that the name field contains the fully-qualified
> * reference (or subdirectory) name. Space could be saved by only
> @@ -911,11 +911,11 @@ static int nonmatching_ref_fn(struct ref_entry *entry,
> void *vdata)
> *
> * extras and skip must be sorted.
> */
> -static int verify_refname_available(const char *refname,
> - const struct string_list *extras,
> - const struct string_list *skip,
> - struct ref_dir *dir,
> - struct strbuf *err)
> +static int verify_refname_available_dir(const char *refname,
> + const struct string_list *extras,
> + const struct string_list *skip,
> + struct ref_dir *dir,
> + struct strbuf *err)
> {
> const char *slash;
> int pos;
> [...]
> diff --git a/refs.h b/refs.h
> index 6d30c98..79ea220 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -218,6 +218,23 @@ extern void warn_dangling_symrefs(FILE *fp, const char
> *msg_fmt, const struct st
> int pack_refs(unsigned int flags);
>
> /*
> + * Return true iff a reference named refname could be created without
> + * conflicting with the name of an existing reference. If
> + * skip is non-NULL, ignore potential conflicts with refs in skip
> + * (e.g., because they are scheduled for deletion in the same
> + * operation).
> + *
> + * Two reference names conflict if one of them exactly matches the
> + * leading components of the other; e.g., "foo/bar" conflicts with
> + * both "foo" and with "foo/bar/baz" but not with "foo/bar" or
> + * "foo/barbados".
> + *
> + * skip must be sorted.
> + */
This comment is approximately a copy of the comment for
verify_refname_available_dir(). It seems unnecessary to keep both of
them (and is also a small maintenance burden). I suggest you shorten the
comment at verify_refname_available_dir() and make it refer to the
comment for this function for the details.
> +int verify_refname_available(const char *newname, struct string_list *extra,
> + struct string_list *skip, struct strbuf *err);
> +
> +/*
> * Flags controlling ref_transaction_update(), ref_transaction_create(), etc.
> * REF_NODEREF: act on the ref directly, instead of dereferencing
> * symbolic references.
>
Michael
--
Michael Haggerty
mhag...@alum.mit.edu
--
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 3/3] init: use strbufs to store paths
The init code predates strbufs, and uses PATH_MAX-sized
buffers along with many manual checks on intermediate sizes
(some of which make magic assumptions, such as that init
will not create a path inside .git longer than 50
characters).
We can simplify this greatly by using strbufs, which drops
some hard-to-verify strcpy calls in favor of git_path_buf.
While we're in the area, let's also convert existing calls
to git_path to the safer git_path_buf (our existing calls
were passed to pretty tame functions, and so were not a
problem, but it's easy to be consistent and safe here).
Note that we had an explicit test that "git init" rejects
long template directories. This comes from 32d1776 (init: Do
not segfault on big GIT_TEMPLATE_DIR environment variable,
2009-04-18). We can drop the test_must_fail here, as we now
accept this and need only confirm that we don't segfault,
which was the original point of the test.
Signed-off-by: Jeff King
---
Same as the original, but minus the bits that touch the precompose
probing.
builtin/init-db.c | 172 +++---
t/t0001-init.sh | 4 +-
2 files changed, 76 insertions(+), 100 deletions(-)
diff --git a/builtin/init-db.c b/builtin/init-db.c
index 89addda..f59f407 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -36,10 +36,11 @@ static void safe_create_dir(const char *dir, int share)
die(_("Could not make %s writable by group"), dir);
}
-static void copy_templates_1(char *path, int baselen,
-char *template, int template_baselen,
+static void copy_templates_1(struct strbuf *path, struct strbuf *template,
DIR *dir)
{
+ size_t path_baselen = path->len;
+ size_t template_baselen = template->len;
struct dirent *de;
/* Note: if ".git/hooks" file exists in the repository being
@@ -49,77 +50,64 @@ static void copy_templates_1(char *path, int baselen,
* with the way the namespace under .git/ is organized, should
* be really carefully chosen.
*/
- safe_create_dir(path, 1);
+ safe_create_dir(path->buf, 1);
while ((de = readdir(dir)) != NULL) {
struct stat st_git, st_template;
- int namelen;
int exists = 0;
+ strbuf_setlen(path, path_baselen);
+ strbuf_setlen(template, template_baselen);
+
if (de->d_name[0] == '.')
continue;
- namelen = strlen(de->d_name);
- if ((PATH_MAX <= baselen + namelen) ||
- (PATH_MAX <= template_baselen + namelen))
- die(_("insanely long template name %s"), de->d_name);
- memcpy(path + baselen, de->d_name, namelen+1);
- memcpy(template + template_baselen, de->d_name, namelen+1);
- if (lstat(path, &st_git)) {
+ strbuf_addstr(path, de->d_name);
+ strbuf_addstr(template, de->d_name);
+ if (lstat(path->buf, &st_git)) {
if (errno != ENOENT)
- die_errno(_("cannot stat '%s'"), path);
+ die_errno(_("cannot stat '%s'"), path->buf);
}
else
exists = 1;
- if (lstat(template, &st_template))
- die_errno(_("cannot stat template '%s'"), template);
+ if (lstat(template->buf, &st_template))
+ die_errno(_("cannot stat template '%s'"),
template->buf);
if (S_ISDIR(st_template.st_mode)) {
- DIR *subdir = opendir(template);
- int baselen_sub = baselen + namelen;
- int template_baselen_sub = template_baselen + namelen;
+ DIR *subdir = opendir(template->buf);
if (!subdir)
- die_errno(_("cannot opendir '%s'"), template);
- path[baselen_sub++] =
- template[template_baselen_sub++] = '/';
- path[baselen_sub] =
- template[template_baselen_sub] = 0;
- copy_templates_1(path, baselen_sub,
-template, template_baselen_sub,
-subdir);
+ die_errno(_("cannot opendir '%s'"),
template->buf);
+ strbuf_addch(path, '/');
+ strbuf_addch(template, '/');
+ copy_templates_1(path, template, subdir);
closedir(subdir);
}
else if (exists)
continue;
else if (S_ISLNK(st_template.st_mode)) {
- char lnk[256];
- int len;
-
[PATCH 2/3] probe_utf8_pathname_composition: use internal strbuf
When we are initializing a .git directory, we may call
probe_utf8_pathname_composition to detect utf8 mangling. We
pass in a path buffer for it to use, and it blindly
strcpy()s into it, not knowing whether the buffer is large
enough to hold the result or not.
In practice this isn't a big deal, because the buffer we
pass in already contains "$GIT_DIR/config", and we append
only a few extra bytes to it. But we can easily do the right
thing just by calling git_path_buf ourselves. Technically
this results in a different pathname (before we appended our
utf8 characters to the "config" path, and now they get their
own files in $GIT_DIR), but that should not matter for our
purposes.
Signed-off-by: Jeff King
---
I assume that "$GIT_DIR/$auml_nfc" is fine to perform this test based on
Torsten's patch showing the same simplification. If it matters, or if we
simply want to be ultra-conservative, changing the "%s" to "CoNfIg%s"
would yield identical behavior (but if it doesn't matter, I think I
prefer this as a simplification).
builtin/init-db.c| 2 +-
compat/precompose_utf8.c | 18 ++
compat/precompose_utf8.h | 2 +-
git-compat-util.h| 2 +-
4 files changed, 13 insertions(+), 11 deletions(-)
diff --git a/builtin/init-db.c b/builtin/init-db.c
index e7d0e31..89addda 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -312,7 +312,7 @@ static int create_default_files(const char *template_path)
strcpy(path + len, "CoNfIg");
if (!access(path, F_OK))
git_config_set("core.ignorecase", "true");
- probe_utf8_pathname_composition(path, len);
+ probe_utf8_pathname_composition();
}
return reinit;
diff --git a/compat/precompose_utf8.c b/compat/precompose_utf8.c
index 044c686..079070f 100644
--- a/compat/precompose_utf8.c
+++ b/compat/precompose_utf8.c
@@ -36,24 +36,26 @@ static size_t has_non_ascii(const char *s, size_t maxlen,
size_t *strlen_c)
}
-void probe_utf8_pathname_composition(char *path, int len)
+void probe_utf8_pathname_composition(void)
{
+ struct strbuf path = STRBUF_INIT;
static const char *auml_nfc = "\xc3\xa4";
static const char *auml_nfd = "\x61\xcc\x88";
int output_fd;
if (precomposed_unicode != -1)
return; /* We found it defined in the global config, respect it
*/
- strcpy(path + len, auml_nfc);
- output_fd = open(path, O_CREAT|O_EXCL|O_RDWR, 0600);
+ git_path_buf(&path, "%s", auml_nfc);
+ output_fd = open(path.buf, O_CREAT|O_EXCL|O_RDWR, 0600);
if (output_fd >= 0) {
close(output_fd);
- strcpy(path + len, auml_nfd);
- precomposed_unicode = access(path, R_OK) ? 0 : 1;
+ git_path_buf(&path, "%s", auml_nfd);
+ precomposed_unicode = access(path.buf, R_OK) ? 0 : 1;
git_config_set("core.precomposeunicode", precomposed_unicode ?
"true" : "false");
- strcpy(path + len, auml_nfc);
- if (unlink(path))
- die_errno(_("failed to unlink '%s'"), path);
+ git_path_buf(&path, "%s", auml_nfc);
+ if (unlink(path.buf))
+ die_errno(_("failed to unlink '%s'"), path.buf);
}
+ strbuf_release(&path);
}
diff --git a/compat/precompose_utf8.h b/compat/precompose_utf8.h
index 3b73585..a94e7c4 100644
--- a/compat/precompose_utf8.h
+++ b/compat/precompose_utf8.h
@@ -27,7 +27,7 @@ typedef struct {
} PREC_DIR;
void precompose_argv(int argc, const char **argv);
-void probe_utf8_pathname_composition(char *, int);
+void probe_utf8_pathname_composition(void);
PREC_DIR *precompose_utf8_opendir(const char *dirname);
struct dirent_prec_psx *precompose_utf8_readdir(PREC_DIR *dirp);
diff --git a/git-compat-util.h b/git-compat-util.h
index 348b9dc..9a3e559 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -229,7 +229,7 @@ typedef unsigned long uintptr_t;
#else
#define precompose_str(in,i_nfd2nfc)
#define precompose_argv(c,v)
-#define probe_utf8_pathname_composition(a,b)
+#define probe_utf8_pathname_composition()
#endif
#ifdef MKDIR_WO_TRAILING_SLASH
--
2.6.0.455.ga3f9923
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/3] precompose_utf8: drop unused variable
The result of iconv is assigned to a variable, but we never
use it (instead, we check errno and whether the function
consumed all bytes). Let's drop the assignment, as it
triggers gcc's -Wunused-but-set-variable.
Signed-off-by: Jeff King
---
This is obviously completely optional; I just needed it to get the
"precompose on Linux" hack to build at all with my usual "-Wall -Werror"
settings. I guess clang doesn't have a similar warning, or maybe people
on Macs are not as pedantic as I am.
compat/precompose_utf8.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/compat/precompose_utf8.c b/compat/precompose_utf8.c
index 95fe849..044c686 100644
--- a/compat/precompose_utf8.c
+++ b/compat/precompose_utf8.c
@@ -139,9 +139,8 @@ struct dirent_prec_psx *precompose_utf8_readdir(PREC_DIR
*prec_dir)
size_t inleft = namelenz;
char *outpos = &prec_dir->dirent_nfc->d_name[0];
size_t outsz =
prec_dir->dirent_nfc->max_name_len;
- size_t cnt;
errno = 0;
- cnt = iconv(prec_dir->ic_precompose, &cp,
&inleft, &outpos, &outsz);
+ iconv(prec_dir->ic_precompose, &cp, &inleft,
&outpos, &outsz);
if (errno || inleft) {
/*
* iconv() failed and errno could be
E2BIG, EILSEQ, EINVAL, EBADF
--
2.6.0.455.ga3f9923
--
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 41/68] init: use strbufs to store paths
On Sun, Oct 04, 2015 at 08:31:31AM +0200, Torsten Bögershausen wrote:
> > That is the original signature, before my sprintf series. I do not mind
> > leaving that as-is, and simply cleaning up probe_utf8_pathname_composition
> > by using a strbuf internally there. Though I have to wonder if it even
> > needs us to pass _anything_ at that point. It could just call
> > git_path_buf("config%s", auml_nfd) itself. The whole reason to pass
> > anything was to let it reuse the buffer the caller had.
> >
> > -Peff
> Makes sense, here is V2:
Yeah, I think this is much nicer.
And because it decouples the interface between init-db.c and the
precompose code, it is easy to do it as a separate patch before the
init-db one.
> diff --git a/compat/precompose_utf8.c b/compat/precompose_utf8.c
> index b4dd3c7..64b85f2 100644
> --- a/compat/precompose_utf8.c
> +++ b/compat/precompose_utf8.c
> @@ -8,6 +8,7 @@
> #include "cache.h"
> #include "utf8.h"
> #include "precompose_utf8.h"
> +#include "strbuf.h"
I think this is actually redundant; it is part of cache.h included above
(and the precompose_utf8.h header file does not need to care anymore,
since the strbuf is not part of the interface).
> -void probe_utf8_pathname_composition(struct strbuf *path)
> +void probe_utf8_pathname_composition(void)
> {
> +struct strbuf sbuf = STRBUF_INIT;
> static const char *auml_nfc = "\xc3\xa4";
> static const char *auml_nfd = "\x61\xcc\x88";
> -size_t baselen = path->len;
> +const char *path;
I don't think we need this separate "path"; we can just access the
strbuf directly (that makes the diff a little noisier, but I think the
end result is simpler).
> diff --git a/compat/precompose_utf8.h b/compat/precompose_utf8.h
> index 7fc7be5..a94e7c4 100644
> --- a/compat/precompose_utf8.h
> +++ b/compat/precompose_utf8.h
> @@ -27,7 +27,7 @@ typedef struct {
> } PREC_DIR;
>
> void precompose_argv(int argc, const char **argv);
> -void probe_utf8_pathname_composition(struct strbuf *path);
> +void probe_utf8_pathname_composition(void);
I think we need a similar fix for the compat macro to build on non-Mac
platforms.
Here's a mini-series I came up with, which I hope is polished enough for
Junio to apply as a drop-in replacement for the "init: use strbufs"
patch from my original series. I compiled-tested it on Linux, with and
without precompose_utf8.o support hacked in. I don't have access to an
OS X machine to test on, so I'd appreciate confirmation that t3910 still
passes there.
[1/3]: precompose_utf8: drop unused variable
[2/3]: probe_utf8_pathname_composition: use internal strbuf
[3/3]: init: use strbufs to store paths
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/4] git-p4: Fix t9815 git-p4-submit-fail test case on OS X
On 04.10.15 20:06, larsxschnei...@gmail.com wrote:
> From: Lars Schneider
>
> The stats command works differently on OS X compared to Linux. Detect
> OS X and execute the appropriate assertions.
>
Is there a special need to use the stat() function at all ?
That's what I read in t1301-shared-repo.sh:
modebits () {
ls -l "$1" | sed -e 's|^\(..\).*|\1|'
}
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 2/3] path: optimize common dir checking
On 09/01/2015 04:13 AM, David Turner wrote: > Instead of a linear search over common_list to check whether > a path is common, use a trie. The trie search operates on > path prefixes, and handles excludes. Here I am, coming late to the discussion as usual. Sorry for that. I dug into this code yesterday and got all nerd-tingly and started refactoring and optimizing it. But after I slept on it I realized that I'm a bit hazy on its justification. A trie is a beautiful data structure and all, but have there been any benchmarks showing (1) that this lookup is a bottleneck and (2) that the trie is an improvement on something simpler, like, say, a sorted string_list? Or is there a realistic hope that the trie might be generally useful for other purposes? The latter seems unlikely, at least with the current implementation, because it is very wasteful of space. It allocates one or two (usually two) `struct trie` for every single string that is added to it. Each `struct trie` contains an array of 256 pointers and usually needs two malloc calls to instantiate. So *each entry* stored in the trie costs something like 4 kilobytes on a 64-bit system, plus usually 4 calls to malloc. The large memory footprint, in turn, will cause access non-locality and might impact the lookup performance. So it is pretty clear that the current code would be unusable for a large number of strings. For this particular application, where we only have 19 strings to store, I suppose we could tolerate the use of approximately 64k of RAM to store 174 characters worth of strings *if* it would bring us big time savings. But I think we need some evidence of the time savings. If this lookup is really a bottleneck, I bet there are other alternatives that are just as fast as this trie and use less code, especially given that there are only 19 strings that need checking. With respect to the implementation, it looks correct to me. I would make the following three suggestions: * Please document that the `contents` field of `struct trie` is not NUL-terminated, because that would otherwise be a common assumption. (It is clearly not NUL-terminated because of the way it is initialized using xmalloc and memcpy in make_trie_node() and because of the way its length is adjusted in add_to_trie().) * But in add_to_trie(), you set `contents` using xstrdup(), which *does* NUL-terminate the string. It would be more consistent to set it using xmalloc and memcpy here. * Please use size_t instead of int for indexing into strings, at least in the trie_find() function, where the input is likely to be under the control of users. If we expected to use the trie for other purposes, then I would suggest a raft of other improvements. Ask me if you are interested. Michael -- Michael Haggerty mhag...@alum.mit.edu -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] test-path-utils.c: remove incorrect assumption
On Sun, Oct 4, 2015 at 6:21 PM, Junio C Hamano wrote: > Ray Donnelly writes: > >>> Some callers of this function in real code (i.e. not the one you are >>> removing the check) do seem to depend on that condition, e.g. the >>> codepath in clone that leads to add_to_alternates_file() wants to >>> make sure it does not add an duplicate, so it may end up not noticing >>> /foo/bar and /foo/bar/ are the same thing, no? There may be others. >> >> Enforcing that normalize_path_copy() removes any trailing '/' (apart >> from the root directory) breaks other things that assume it doesn't >> mess with trailing '/'s, for example filtering in ls-tree. Any >> suggestions for what to do about this? Would a flag be appropriate as >> to whether to do this part or not? Though I'll admit I don't like the >> idea of adding flags to modify the behavior of something that's meant >> to "normalize" something. Alternatively, I could go through all the >> breakages and try to fix them up? > > I agree with you that "normalize" should "normalize". Making sure > that all the callers expect the same kind of normalization would be > a lot of work but I do think that is the best approach in the long > run. Thanks for the ls-tree example, by the way, did you find it by > code inspection? I do not think it is reasonable to expect the test > coverage for this to be 100%, so the "try to fix them up" would have > to involve a lot of manual work both in fixing and reviewing, > unfortunately. For the ls-tree failure, I ran "make test" to see how much fell out. I'm not familiar with the code-base yet, so I figured that at least investigating the changes needed to make the test-suite pass would be a good entry point to reading the code; I will study it at the same time to try and get my bearings. > > The first step of the "best approach" would be to make a note on > normalize_path_copy() by adding a NEEDSWORK: comment to describe the > situation. > > 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 v2 3/4] git-p4: Fix t9815 git-p4-submit-fail test case on OS X
larsxschnei...@gmail.com wrote on Sun, 04 Oct 2015 11:44 -0700: > > On 04 Oct 2015, at 11:23, Junio C Hamano wrote: > > > larsxschnei...@gmail.com writes: > > > >> + if test_have_prereq CYGWIN; then > >> + : # NOOP > >> + elif test_have_prereq DARWIN; then > >> + stat -f %Sp text | egrep ^-r-- && > >> + stat -f %Sp text+x | egrep ^-r-x > >> + else > >>stat --format=%A text | egrep ^-r-- && > >>stat --format=%A text+x | egrep ^-r-x > >>fi > > > > Not a new problem but why do we need "stat" here? > > > > Shouldn't "test -r", "! test -x", and their usual friends be > > sufficient for the purpose of the test and are more portable? > > Good question. The stat call was introduced with df9c545 by Pete Wyckoff. > @Pete, @Luke: Are you aware of any particular reason for stat? I think you could do this all with test. The key is to make sure the files are readable, not writable, and either executable or not. Cygwin and darwin oddities were not on my radar 3 years ago. See also 4cea4d6 (git p4 test: use test_chmod for cygwin, 2013-01-26) for the description I wrote about what this test is trying to verify. -- Pete -- 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 3/9] ref-filter: add support for %(path) atom
On Mon, Oct 5, 2015 at 12:14 AM, Junio C Hamano wrote:
>
> Is that a derived form of the refname, just like %(refname:short)
> that is 'master' for a ref whose %(refname) is 'refs/heads/master'
> is a derived form of %(refname), and ":short" is what tells the
> formatting machinery what kind of derivation is desired?
>
> If so would %(refname:dir) & %(refname:base) be more in line with
> the overall structure?
This seems way better, I like these names more.
On Sun, Oct 4, 2015 at 11:21 PM, Matthieu Moy
wrote:
> Karthik Nayak writes:
>
>> On Sat, Oct 3, 2015 at 3:32 PM, Matthieu Moy
>> wrote:
>>> Karthik Nayak writes:
>>>
This adds %(path) and %(path:short) atoms. The %(path) atom will print
the path of the given ref, while %(path:short) will only print the
subdirectory of the given ref.
>>>
>>> What does "path" mean in this context? How is it different from
>>> %(refname)?
>>>
>>> I found the answer below, but I could not guess from the doc and commit
>>> message. Actually, I'm not sure %(path) is the right name. If you want
>>> the "file/directory" analogy, then %(dirname) would be better.
>>>
>>
>> Noted will change.
>
> Note: I don't completely like %(dirname) either. I'm convinced it's
> better than %(path), but there may be a better option.
>
I like Junio's suggestion, stick with that?
+ } else if (match_atom_name(name, "path", &valp)) {
+ const char *sp, *ep;
+
+ if (ref->kind & FILTER_REFS_DETACHED_HEAD)
+ continue;
+
+ sp = strchr(ref->refname, '/');
+ ep = strchr(++sp, '/');
>>>
>>> This assumes you have two / in the fullrefname. It is normally the case,
>>> but one can also create eg. refs/foo references. AFAIK, in this case sp
>>> will be NULL, and you're then calling strchr(++NULL) which may segfault.
>>
>> Not really,
>
> Oops, indeed, for refs/foo, sp points to / and ep is NULL. It would
> segfault for any ref without a /. Currently, the only such ref is HEAD
> and it is dealt with by the 'if' above, but that still sounds a bit
> fragile to me. Ifever we allow listing refs like FETCH_HEAD, I'm not
> sure we'll remember that and test %(path) on it.
>
> Adding something like
>
> if (!sp)
> continue;
>
> after the first strchr would make me feel safer.
>
Good point! I'll add that.
>> but you made me think of another possible issue.
>>
>> Assume refs/foo "sp = strchr(ref->refname, '/');" would set sp to point at
>> '/' and ++sp will now point at 'f'.
>>
>> The problem now is refs/foo is supposed to print just "refs" whereas it'll
>> print "refs/foo". and %(dirname:short) is supposed to print "" whereas it'll
>> print "foo". Will look into this :)
>
> I think it's worse than that because ep will be NULL, and then you call
>
> v->s = xstrndup(sp, ep - sp);
>
Ah yes, weirdly though my test didn't yield a problem.
~ git update-ref refs/foo master
~ git for-each-ref --format="%(refname) %(color:red)%(dirname:short)"
refs/foo foo
~ git for-each-ref --format="%(refname) %(color:red)%(dirname)"
refs/foo refs/foo
--
Regards,
Karthik Nayak
--
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 v2 4/4] git-p4: Disable t9819 git-p4-case-folding test on OS X
Lars Schneider writes: > By the way... what formatting should I use? > > if foo > then > bar > > or > > if foo; then > bar > > I think the latter is more used in the code base. I've allowed the latter in fringe scripts (i.e. the ones that I do not care too deeply) over time, but Documentation/CodingGuidelines is very clear about it. - Do not write control structures on a single line with semicolon. -- 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 v2 2/4] Add prereq to detect OS X
On 04 Oct 2015, at 11:28, Junio C Hamano wrote: > larsxschnei...@gmail.com writes: > >> From: Lars Schneider >> >> Some Unix tools differ between Linux and Mac OS X. Add this prereq to >> detect OS X and handle it appropriately. > > If my comment on 4/4 turns out to be to the point, then we do not > need this, I presume? In general, we really should think three > times before adding prerequisite based on the platform. A prereq > based on a specific feature of the platform is often what we want > instead. Agreed. I will remove this commit in the next roll. Thanks, Lars -- 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 3/9] ref-filter: add support for %(path) atom
Matthieu Moy writes: This adds %(path) and %(path:short) atoms. The %(path) atom will print the path of the given ref, while %(path:short) will only print the subdirectory of the given ref. >>> >>> What does "path" mean in this context? How is it different from >>> %(refname)? >>> >>> I found the answer below, but I could not guess from the doc and commit >>> message. Actually, I'm not sure %(path) is the right name. If you want >>> the "file/directory" analogy, then %(dirname) would be better. >>> >> >> Noted will change. > > Note: I don't completely like %(dirname) either. I'm convinced it's > better than %(path), but there may be a better option. Is that a derived form of the refname, just like %(refname:short) that is 'master' for a ref whose %(refname) is 'refs/heads/master' is a derived form of %(refname), and ":short" is what tells the formatting machinery what kind of derivation is desired? If so would %(refname:dir) & %(refname:base) be more in line with the overall structure? -- 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 v2 3/4] git-p4: Fix t9815 git-p4-submit-fail test case on OS X
On 04 Oct 2015, at 11:23, Junio C Hamano wrote: > larsxschnei...@gmail.com writes: > >> +if test_have_prereq CYGWIN; then >> +: # NOOP >> +elif test_have_prereq DARWIN; then >> +stat -f %Sp text | egrep ^-r-- && >> +stat -f %Sp text+x | egrep ^-r-x >> +else >> stat --format=%A text | egrep ^-r-- && >> stat --format=%A text+x | egrep ^-r-x >> fi > > Not a new problem but why do we need "stat" here? > > Shouldn't "test -r", "! test -x", and their usual friends be > sufficient for the purpose of the test and are more portable? Good question. The stat call was introduced with df9c545 by Pete Wyckoff. @Pete, @Luke: Are you aware of any particular reason for stat? Thanks, Lars -- 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 v2 4/4] git-p4: Disable t9819 git-p4-case-folding test on OS X
On 04 Oct 2015, at 11:26, Junio C Hamano wrote: > larsxschnei...@gmail.com writes: > >> From: Lars Schneider >> >> The OS X file system is case insensitive by default. Consequently this >> test does not apply. >> >> Signed-off-by: Lars Schneider >> --- >> t/t9819-git-p4-case-folding.sh | 5 + >> 1 file changed, 5 insertions(+) >> >> diff --git a/t/t9819-git-p4-case-folding.sh b/t/t9819-git-p4-case-folding.sh >> index 78f1d0f..c69ae47 100755 >> --- a/t/t9819-git-p4-case-folding.sh >> +++ b/t/t9819-git-p4-case-folding.sh >> @@ -4,6 +4,11 @@ test_description='interaction with P4 case-folding' >> >> . ./lib-git-p4.sh >> >> +if test_have_prereq DARWIN; then >> +skip_all='skipping P4 case-folding tests; OS X file system is case >> insensitive by default' >> +test_done >> +fi > > Makes one wonder what should happen on Windows, or vfat mounted on > Linux for that matter. IOW, shouldn't the prerequisite be more like > "do not run any of these tests if the filesystem does not allow us > to have two files in different cases at the same time"? > > Perhaps > >if ! test_have_prereq CASE_INSENSITIVE_FS >then >skip_all=... >test_done >fi > > instead, or something? Agreed! Although I think the “!” in the if clause is not correct. By the way... what formatting should I use? if foo then bar or if foo; then bar I think the latter is more used in the code base. - Lars-- 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 v2 2/4] Add prereq to detect OS X
larsxschnei...@gmail.com writes: > From: Lars Schneider > > Some Unix tools differ between Linux and Mac OS X. Add this prereq to > detect OS X and handle it appropriately. If my comment on 4/4 turns out to be to the point, then we do not need this, I presume? In general, we really should think three times before adding prerequisite based on the platform. A prereq based on a specific feature of the platform is often what we want instead. -- 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 v2 4/4] git-p4: Disable t9819 git-p4-case-folding test on OS X
larsxschnei...@gmail.com writes: > From: Lars Schneider > > The OS X file system is case insensitive by default. Consequently this > test does not apply. > > Signed-off-by: Lars Schneider > --- > t/t9819-git-p4-case-folding.sh | 5 + > 1 file changed, 5 insertions(+) > > diff --git a/t/t9819-git-p4-case-folding.sh b/t/t9819-git-p4-case-folding.sh > index 78f1d0f..c69ae47 100755 > --- a/t/t9819-git-p4-case-folding.sh > +++ b/t/t9819-git-p4-case-folding.sh > @@ -4,6 +4,11 @@ test_description='interaction with P4 case-folding' > > . ./lib-git-p4.sh > > +if test_have_prereq DARWIN; then > + skip_all='skipping P4 case-folding tests; OS X file system is case > insensitive by default' > + test_done > +fi Makes one wonder what should happen on Windows, or vfat mounted on Linux for that matter. IOW, shouldn't the prerequisite be more like "do not run any of these tests if the filesystem does not allow us to have two files in different cases at the same time"? Perhaps if ! test_have_prereq CASE_INSENSITIVE_FS then skip_all=... test_done fi instead, or something? -- 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 v2 3/4] git-p4: Fix t9815 git-p4-submit-fail test case on OS X
larsxschnei...@gmail.com writes: > + if test_have_prereq CYGWIN; then > + : # NOOP > + elif test_have_prereq DARWIN; then > + stat -f %Sp text | egrep ^-r-- && > + stat -f %Sp text+x | egrep ^-r-x > + else > stat --format=%A text | egrep ^-r-- && > stat --format=%A text+x | egrep ^-r-x > fi Not a new problem but why do we need "stat" here? Shouldn't "test -r", "! test -x", and their usual friends be sufficient for the purpose of the test and are more portable? -- 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 v8 0/7] git-p4: add support for large file systems
On 03 Oct 2015, at 10:23, Junio C Hamano wrote:
> Junio C Hamano writes:
>
>> Luke Diamand writes:
>>
>>> All looks good to me, Ack.
>>>
>>> One tiny thing perhaps Junio could comment on: the git commit message
>>> for 75abe9fa5b39980de27dfc33dd5b4f4b5926f34c, "git-p4: add optional
>>> type specifier to gitConfig reader" uses what looks like UTF-8 encoded
>>> left and right quotes, rather than regular ASCII quotes ("). I don't
>>> know if that matters.
>>
>> Yeah, I noticed them, too. In general, I'd prefer to avoid these
>> pretty-quotes myself, as they typically do not add much information
>> (unless nesting matters, which is usually not the case in the log
>> message, or something) and the primary effect of them is to force us
>> to step outside ASCII, which causes editors and pagers to misalign
>> for some people.
>>
>> But it is not too huge an issue that it is worth to go back and fix
>> them, either.
>
> Well, I looked at it again and it also replaced double-dash before
> option names like --bool and --int with something funny (are these
> em-dashes?), which is a more serious bogosity than pretty quotes, so
> I ended up amending the message for that commit after all.
>
Oh. This was not intentional. I wonder how that happened. I will watch out for
it in the future.
Thanks for fixing,
Lars
--
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 4/4] git-p4: Disable t9819 git-p4-case-folding test on OS X
From: Lars Schneider The OS X file system is case insensitive by default. Consequently this test does not apply. Signed-off-by: Lars Schneider --- t/t9819-git-p4-case-folding.sh | 5 + 1 file changed, 5 insertions(+) diff --git a/t/t9819-git-p4-case-folding.sh b/t/t9819-git-p4-case-folding.sh index 78f1d0f..c69ae47 100755 --- a/t/t9819-git-p4-case-folding.sh +++ b/t/t9819-git-p4-case-folding.sh @@ -4,6 +4,11 @@ test_description='interaction with P4 case-folding' . ./lib-git-p4.sh +if test_have_prereq DARWIN; then + skip_all='skipping P4 case-folding tests; OS X file system is case insensitive by default' + test_done +fi + test_expect_success 'start p4d with case folding enabled' ' start_p4d -C1 ' -- 2.5.1 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 1/4] Add Travis CI support
From: Lars Schneider
The tests are executed on "Ubuntu 12.04 LTS Server Edition 64 bit" and
on "OS X Mavericks" using gcc and clang.
Perforce and Git-LFS are installed and therefore available for the
respective tests.
Signed-off-by: Lars Schneider
---
.travis.yml | 31 +++
1 file changed, 31 insertions(+)
create mode 100644 .travis.yml
diff --git a/.travis.yml b/.travis.yml
new file mode 100644
index 000..8a29dd6
--- /dev/null
+++ b/.travis.yml
@@ -0,0 +1,31 @@
+language: c
+
+os:
+ - linux
+ - osx
+
+compiler:
+ - clang
+ - gcc
+
+before_script:
+ - >
+if [ ${TRAVIS_OS_NAME:-'linux'} = 'linux' ]; then
+ wget -q https://package.perforce.com/perforce.pubkey -O - | sudo apt-key
add -
+ echo 'deb http://package.perforce.com/apt/ubuntu precise release' | sudo
tee -a /etc/apt/sources.list
+ wget -q https://packagecloud.io/gpg.key -O - | sudo apt-key add -
+ echo 'deb https://packagecloud.io/github/git-lfs/debian/ jessie main' |
sudo tee -a /etc/apt/sources.list
+ sudo apt-get update -qq
+ sudo apt-get install -y apt-transport-https
+ sudo apt-get install perforce-server git-lfs
+elif [ ${TRAVIS_OS_NAME:-'linux'} = 'osx' ]; then
+ brew update
+ brew tap homebrew/binary
+ P4_SERVER_SHA=$(brew fetch --force perforce-server 2>&1 | grep ^SHA256:
| cut -d ' ' -f 2)
+ P4_SHA=$(brew fetch --force perforce 2>&1 | grep ^SHA256: | cut -d ' '
-f 2)
+ sed -E -i.bak "s/sha256 \"[0-9a-f]{64}\"/sha256 \"$P4_SERVER_SHA\"/g"
/usr/local/Library/Taps/homebrew/homebrew-binary/perforce-server.rb
+ sed -E -i.bak "s/sha256 \"[0-9a-f]{64}\"/sha256 \"$P4_SHA\"/g"
/usr/local/Library/Taps/homebrew/homebrew-binary/perforce.rb
+ brew install git-lfs perforce-server perforce
+fi
+
+install: make configure
--
2.5.1
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 3/4] git-p4: Fix t9815 git-p4-submit-fail test case on OS X
From: Lars Schneider The stats command works differently on OS X compared to Linux. Detect OS X and execute the appropriate assertions. Signed-off-by: Lars Schneider --- t/t9815-git-p4-submit-fail.sh | 8 ++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/t/t9815-git-p4-submit-fail.sh b/t/t9815-git-p4-submit-fail.sh index 4cff6a7..520b812 100755 --- a/t/t9815-git-p4-submit-fail.sh +++ b/t/t9815-git-p4-submit-fail.sh @@ -417,8 +417,12 @@ test_expect_success 'cleanup chmod after submit cancel' ' ! p4 fstat -T action text && test_path_is_file text+x && ! p4 fstat -T action text+x && - if test_have_prereq !CYGWIN - then + if test_have_prereq CYGWIN; then + : # NOOP + elif test_have_prereq DARWIN; then + stat -f %Sp text | egrep ^-r-- && + stat -f %Sp text+x | egrep ^-r-x + else stat --format=%A text | egrep ^-r-- && stat --format=%A text+x | egrep ^-r-x fi -- 2.5.1 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 2/4] Add prereq to detect OS X
From: Lars Schneider
Some Unix tools differ between Linux and Mac OS X. Add this prereq to
detect OS X and handle it appropriately.
Signed-off-by: Lars Schneider
---
t/test-lib.sh | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 16c4d7b..726af40 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -913,7 +913,7 @@ yes () {
done
}
-# Fix some commands on Windows
+# Fix some commands on Windows and OS X
case $(uname -s) in
*MINGW*)
# Windows has its own (incompatible) sort and find
@@ -946,6 +946,9 @@ case $(uname -s) in
test_set_prereq SED_STRIPS_CR
test_set_prereq GREP_STRIPS_CR
;;
+*Darwin*)
+ test_set_prereq DARWIN
+ ;;
*)
test_set_prereq POSIXPERM
test_set_prereq BSLASHPSPEC
--
2.5.1
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH v1] Add Travis CI support
On zo, 2015-10-04 at 10:46 -0700, Junio C Hamano wrote: > One final question. Which configuration file does the CI use when > running a PR-initiated test? The one already in the repository > i.e. the target of the proposed pull, or the one that is possibly > updated by the PR? > > I am wondering if that can be an avenue for a possible mischief. The latter. And it can, as it can enable notifications. -- Dennis Kaarsemaker www.kaarsemaker.net -- 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 0/4] Add Travis CI support
From: Lars Schneider diff to v1: * improve readabilty of Travis "before_script" section * make OS X Perforce brew robust against changing Perforce builds [1] * add a prereq to detect OS X in tests * fix t9815 git-p4-submit-fail test case on OS X * disable t9819 git-p4-case-folding test on OS X Thanks Junio for the feedback. You are right, I should have fixed the failing tests in the first place. With this patch all tests pass on Linux and OS X: https://travis-ci.org/larsxschneider/git/builds/83575208 In a subsequent patch I plan to: * add cvs to run t94?? tests * add svn to run t91?? tests * add apache to run 5539, 5550, and 5561 * investigate if it is possible to run t1509 root worktree test * investigate if it is possible to add jgit to run t5310 Plus I have the following questions: * Can you explain to me how the t7006 page tests should be executed? * Should we enable EXPENSIVE, CLONE_2GB, and USE_LIBPCRE flag? Thanks, Lars [1] This is a workaround. I am in contact about the issue with the homebrew maintainers and maybe we can make this easier soon: https://github.com/Homebrew/homebrew-binary/pull/267#issuecomment-145317114 Lars Schneider (4): Add Travis CI support Add prereq to detect OS X git-p4: Fix t9815 git-p4-submit-fail test case on OS X git-p4: Disable t9819 git-p4-case-folding test on OS X .travis.yml| 31 +++ t/t9815-git-p4-submit-fail.sh | 8 ++-- t/t9819-git-p4-case-folding.sh | 5 + t/test-lib.sh | 5 - 4 files changed, 46 insertions(+), 3 deletions(-) create mode 100644 .travis.yml -- 2.5.1 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH v1] Add Travis CI support
Matthieu Moy writes: > Currenty, to mimick this flow, we would need something like > > 1) User activates Travis-CI on his repo (each user would have to do >this, not just once) > > 2) User commits .travis.yml on top of the code to submit > > 3) User pushes to his repo > > 4) Travis-CI triggers a build > > 5) User removes the commit introducing .travis.yml, force-pushes > > 6) User submit the resulting code. I do not think it has to be so convoluted. I know this would appear to be more or less a moot point, as the long term direction would be to enable one on git/git and do PR-initiated tests, but I am writing it here because I would really prefer that the CI configuration file that will be added to my tree is a "battle tested" one. A motivated user who wants to send a patch to add it to my tree can: (1) Fork from an ancient place, e.g. v2.0.0, and add the CI configuration file. Call that branch "travis". (2) Prepare topics that he wants to test (not related to "add CI integration to Git" topic) on its own topics, branching from my 'master' or 'maint' depending on the target track. (3) Keep a branch that merges (2) with (1). This could be a set of branches, one per topics in (2). (4) Have the CI monitor (3). (5) Make sure tests pass. Send (2) out via whatever means, e.g. via SubmitGit. And keep the set-up for a few months to make sure everything looks good, before sending (1) out via SubmitGit. -- 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 3/9] ref-filter: add support for %(path) atom
Karthik Nayak writes:
> On Sat, Oct 3, 2015 at 3:32 PM, Matthieu Moy
> wrote:
>> Karthik Nayak writes:
>>
>>> This adds %(path) and %(path:short) atoms. The %(path) atom will print
>>> the path of the given ref, while %(path:short) will only print the
>>> subdirectory of the given ref.
>>
>> What does "path" mean in this context? How is it different from
>> %(refname)?
>>
>> I found the answer below, but I could not guess from the doc and commit
>> message. Actually, I'm not sure %(path) is the right name. If you want
>> the "file/directory" analogy, then %(dirname) would be better.
>>
>
> Noted will change.
Note: I don't completely like %(dirname) either. I'm convinced it's
better than %(path), but there may be a better option.
>>> + } else if (match_atom_name(name, "path", &valp)) {
>>> + const char *sp, *ep;
>>> +
>>> + if (ref->kind & FILTER_REFS_DETACHED_HEAD)
>>> + continue;
>>> +
>>> + sp = strchr(ref->refname, '/');
>>> + ep = strchr(++sp, '/');
>>
>> This assumes you have two / in the fullrefname. It is normally the case,
>> but one can also create eg. refs/foo references. AFAIK, in this case sp
>> will be NULL, and you're then calling strchr(++NULL) which may segfault.
>
> Not really,
Oops, indeed, for refs/foo, sp points to / and ep is NULL. It would
segfault for any ref without a /. Currently, the only such ref is HEAD
and it is dealt with by the 'if' above, but that still sounds a bit
fragile to me. Ifever we allow listing refs like FETCH_HEAD, I'm not
sure we'll remember that and test %(path) on it.
Adding something like
if (!sp)
continue;
after the first strchr would make me feel safer.
> but you made me think of another possible issue.
>
> Assume refs/foo "sp = strchr(ref->refname, '/');" would set sp to point at
> '/' and ++sp will now point at 'f'.
>
> The problem now is refs/foo is supposed to print just "refs" whereas it'll
> print "refs/foo". and %(dirname:short) is supposed to print "" whereas it'll
> print "foo". Will look into this :)
I think it's worse than that because ep will be NULL, and then you call
v->s = xstrndup(sp, ep - sp);
--
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: [RFC/PATCH v1] Add Travis CI support
Matthieu Moy writes: > Junio C Hamano writes: >> >> I still don't see a reason why git/git needs to be the one that is >> used, > > The very nice thing with Travis-CI is that it does not only test the > repository's branches, but also all pull-requests. OK, that is the first real argument I heard for enabling it on git/git that is worth listening to. Practically, it has little value to run CI (whose only test is to run "make test") on branches that I publish in that repository. By the time a change hits that repository, "make test" has been run on my end already, and the only thing the CI would catch is platform dependent glitches (e.g. Windows and Mac), dependency-related ones (e.g. p4), or breakages I already know about [*1*]. But we _do_ want to see tested patches submitted to the list so that reviewers do not have to waste time on obviously bogus patches reviewing (and the integrator wasting time on deconflicting). A test that is PR-initiated would give us a real value there. The repository that is used for the PR-initiated test does not have to be git/git (it only has to be a central well-known repository), but similar to arrangement for SubmitGit, I agree that git/git would be a good candidate for that "central well-known" one. There is not much point in introducing another "if you want your topics tested, throw a PR against this other repository". So,... I would not mind a patch that adds a CI configuration file (I would really prefer it to be a battle-tested one, though) to my tree, and I would not mind if CI is enabled on git/git, if Peff or somebody more security-minded than me thinks it is safe to do so. One final question. Which configuration file does the CI use when running a PR-initiated test? The one already in the repository i.e. the target of the proposed pull, or the one that is possibly updated by the PR? I am wondering if that can be an avenue for a possible mischief. Thanks. [Footnote] *1* I occasionally do push out 'pu' with known breakages (e.g. the recent 'lmdb' one) to make sure people are running the test suite so that they will work with the topic author to resolve the issue without having to wait for me to tell the topic author about it; letting CI catch that kind of breakage would not add much value, because it is already known ;-) -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/9] ref-filter: implement %(if), %(then), and %(else) atoms
Matthieu Moy writes: >> I found all the suggestions very good, except that the distinction >> between "expands to" and "is printed" bothers me a bit, as they want >> to mean exactly the same thing (imagine this whole thing were inside >> another %(if)...%(then)). > > True. Then let me try again: > > Implement %(if), %(then) and %(else) atoms. Used as > %(if)...%(then)...%(end) or %(if)...%(then)...%(else)...%(end). If the > format string between %(if) and %(then) expands to an empty string, or > to only whitespaces, then the whole %(if)...%(end) expands to the string > following %(then). Otherwise, it expands to the string following > %(else), if any. Nice. 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 1/2] test-path-utils.c: remove incorrect assumption
Ray Donnelly writes: >> Some callers of this function in real code (i.e. not the one you are >> removing the check) do seem to depend on that condition, e.g. the >> codepath in clone that leads to add_to_alternates_file() wants to >> make sure it does not add an duplicate, so it may end up not noticing >> /foo/bar and /foo/bar/ are the same thing, no? There may be others. > > Enforcing that normalize_path_copy() removes any trailing '/' (apart > from the root directory) breaks other things that assume it doesn't > mess with trailing '/'s, for example filtering in ls-tree. Any > suggestions for what to do about this? Would a flag be appropriate as > to whether to do this part or not? Though I'll admit I don't like the > idea of adding flags to modify the behavior of something that's meant > to "normalize" something. Alternatively, I could go through all the > breakages and try to fix them up? I agree with you that "normalize" should "normalize". Making sure that all the callers expect the same kind of normalization would be a lot of work but I do think that is the best approach in the long run. Thanks for the ls-tree example, by the way, did you find it by code inspection? I do not think it is reasonable to expect the test coverage for this to be 100%, so the "try to fix them up" would have to involve a lot of manual work both in fixing and reviewing, unfortunately. The first step of the "best approach" would be to make a note on normalize_path_copy() by adding a NEEDSWORK: comment to describe the situation. 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 3/9] ref-filter: add support for %(path) atom
On Sat, Oct 3, 2015 at 3:32 PM, Matthieu Moy
wrote:
> Karthik Nayak writes:
>
>> This adds %(path) and %(path:short) atoms. The %(path) atom will print
>> the path of the given ref, while %(path:short) will only print the
>> subdirectory of the given ref.
>
> What does "path" mean in this context? How is it different from
> %(refname)?
>
> I found the answer below, but I could not guess from the doc and commit
> message. Actually, I'm not sure %(path) is the right name. If you want
> the "file/directory" analogy, then %(dirname) would be better.
>
Noted will change.
>> + } else if (match_atom_name(name, "path", &valp)) {
>> + const char *sp, *ep;
>> +
>> + if (ref->kind & FILTER_REFS_DETACHED_HEAD)
>> + continue;
>> +
>> + sp = strchr(ref->refname, '/');
>> + ep = strchr(++sp, '/');
>
> This assumes you have two / in the fullrefname. It is normally the case,
> but one can also create eg. refs/foo references. AFAIK, in this case sp
> will be NULL, and you're then calling strchr(++NULL) which may segfault.
>
Not really, but you made me think of another possible issue.
Assume refs/foo "sp = strchr(ref->refname, '/');" would set sp to point at
'/' and ++sp will now point at 'f'.
The problem now is refs/foo is supposed to print just "refs" whereas it'll
print "refs/foo". and %(dirname:short) is supposed to print "" whereas it'll
print "foo". Will look into this :)
>> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
>> index d7f7a18..5557657 100755
>> --- a/t/t6302-for-each-ref-filter.sh
>> +++ b/t/t6302-for-each-ref-filter.sh
>> @@ -312,6 +312,7 @@ test_expect_success 'check %(if:equals=)' '
>> test_cmp expect actual
>> '
>>
>> +
>> test_expect_success 'check %(if:notequals=)' '
>
> Useless new blank line.
>
Will remove.
>> +test_expect_success 'check %(path)' '
>> + git for-each-ref --format="%(path)" >actual &&
>> + cat >expect <<-\EOF &&
>> + refs/heads
>
> You should add eg.
>
> git update-ref refs/foo HEAD
> git update-ref refs/foodir/bar/boz HEAD
>
> before the test to check and document the behavior for such refnames.
Yeah makes sense.
--
Regards,
Karthik Nayak
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC 1/2] sha1_file: close all pack files after running
Hi Max, On 2015-10-02 21:21, Max Kirillov wrote: > On Fri, Oct 02, 2015 at 12:13:40PM +0200, Johannes Schindelin wrote: >> On 2015-10-02 12:05, Johannes Schindelin wrote: >> >> > On 2015-10-01 05:29, Max Kirillov wrote: 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. >> >> Could you do me another favor? It seems that you want to >> work on this, so I will step back (I have to take off for >> the weekend very soon anyway, so I am really glad that you >> take care of it). But I would really love to see the line > > As I explained in other message, your case is a bit > different. I guess then we would need two different patches for the two different fixes, at least. So now I am unsure how to proceed: I do not want to step on your toes, but I also want to see my use case fixed and I want to move forward on this. At the moment, it looks as if we are at an impasse. Ciao, Johannes -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] test-path-utils.c: remove incorrect assumption
On Sat, Oct 3, 2015 at 6:13 PM, Junio C Hamano wrote: > Ray Donnelly writes: > >> In normalize_ceiling_entry(), we test that normalized paths end with >> slash, *unless* the path to be normalized was already the root >> directory. >> >> However, normalize_path_copy() does not even enforce this condition. > > Perhaps the real issue to be addressed is the above, and your patch > is killing a coalmine canary? > > Some callers of this function in real code (i.e. not the one you are > removing the check) do seem to depend on that condition, e.g. the > codepath in clone that leads to add_to_alternates_file() wants to > make sure it does not add an duplicate, so it may end up not noticing > /foo/bar and /foo/bar/ are the same thing, no? There may be others. > > Enforcing that normalize_path_copy() removes any trailing '/' (apart from the root directory) breaks other things that assume it doesn't mess with trailing '/'s, for example filtering in ls-tree. Any suggestions for what to do about this? Would a flag be appropriate as to whether to do this part or not? Though I'll admit I don't like the idea of adding flags to modify the behavior of something that's meant to "normalize" something. Alternatively, I could go through all the breakages and try to fix them up? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/9] ref-filter: implement %(if), %(then), and %(else) atoms
On Sat, Oct 3, 2015 at 3:09 PM, Matthieu Moy
wrote:
> Karthik Nayak writes:
>
>> Implement %(if), %(then) and %(else) atoms. Used as
>> %(if)..%(then)..%(end) or %(if)..%(then)..%(else)..%(end).
>
> I prefer ... to .., which often means "interval" as in HEAD^^..HEAD.
>
Seems good, will change.
>> If there is an atom with value or string literal after the %(if)
>
> I find this explanation hard to read, and ambiguous: what does "atom
> with value" mean?
>
>> then everything after the %(then) is printed, else if the %(else) atom
>> is used, then everything after %(else) is printed. If the string
>> contains only whitespaces, then it is not considered.
>
> "the string" is ambiguous again. I guess it's "what's between %(if) and
> %(then)", but it could be clearer. And it's not clear what "not
> considered" means.
>
> My take on it:
>
> Implement %(if), %(then) and %(else) atoms. Used as
> %(if)...%(then)...%(end) or %(if)...%(then)...%(else)...%(end). If the
> format string between %(if) and %(then) expands to an empty string, or
> to only whitespaces, then the string following %(then) is printed.
> Otherwise, the string following %(else), if any, is printed.
>
This (the one you sent again after Junio's suggestion, looks better),
I'll use this thanks.
>> +When a scripting language specific quoting is in effect,
>
> This may not be immediately clear to the reader. I'd add explicitly:
>
> When a scripting language specific quoting is in effect (i.e. one of
> `--shell`, `--perl`, `--python`, `--tcl` is used), ...
>
Makes sense.
>> EXAMPLES
>>
>
> This is just the context of the patch, but I read it as a hint that we
> could add some examples with complex --format usage to illustrate the
> theory above.
>
I was thinking about adding :
An example to show the usage of %(if)...%(then)...%(else)...%(end).
This prefixes the current branch with a star.
#!/bin/sh
git for-each-ref --format="%(if)%(HEAD)%(then)* %(else)
%(end)%(refname:short)" refs/heads/
An example to show the usage of %(if)...%(then)...%(end).
This adds a red color to authorname, if present
#!/bin/sh
git for-each-ref
--format="%(refname)%(if)%(authorname)%(then)%(color:red)%(end)
%(authorname)"
>> + if (if_then_else->condition_satisfied && if_then_else->else_atom) {
> // cs && else
>> + strbuf_reset(&cur->output);
>> + pop_stack_element(&cur);
>> + } else if (if_then_else->else_atom) {
> // !cs && else
>> + strbuf_swap(&cur->output, &prev->output);
>> + strbuf_reset(&cur->output);
>> + pop_stack_element(&cur);
>> + } else if (!if_then_else->condition_satisfied)
> // !cs && !else
>> + strbuf_reset(&cur->output);
>
> This if/else if/else if looks hard to read to me. I had to add the
> comments above as a note to myself to get the actual full condition for
> 3 branches.
>
> The reasoning would be clearer to me as:
>
> if (if_then_else->else_atom) {
> /*
> * There is an %(else) atom: we need to drop one state from the
> * stack, either the %(else) branch if the condition is satisfied, or
> * the %(then) branch if it isn't.
> */
> if (if_then_else->condition_satisfied) {
> strbuf_reset(&cur->output);
> pop_stack_element(&cur);
> } else {
> strbuf_swap(&cur->output, &prev->output);
> strbuf_reset(&cur->output);
> pop_stack_element(&cur);
> }
> } else if (if_then_else->condition_satisfied)
> /*
> * No %(else) atom: just drop the %(then) branch if the
> * condition is not satisfied.
> */
> strbuf_reset(&cur->output);
>
This looks neater thanks.
>> +static void if_atom_handler(struct atom_value *atomv, struct
>> ref_formatting_state *state)
>> +{
>> + struct ref_formatting_stack *new;
>> + struct if_then_else *if_then_else = xcalloc(sizeof(struct
>> if_then_else), 1);
>> +
>> + if_then_else->if_atom = 1;
>
> Do you ever use this "if_atom"? It doesn't seem so in the current patch,
> and it seems like a tautology to me: if you have a struct if_then_else,
> then you have seen the %(if).
>
Yea I'll remove that.
>> +static int is_empty(const char * s){
>
> char * s -> char *s
>
will do.
>> +static void then_atom_handler(struct atom_value *atomv, struct
>> ref_formatting_state *state)
>> +{
>> + struct ref_formatting_stack *cur = state->stack;
>> + struct if_then_else *if_then_else = (struct if_then_else
>> *)cur->at_end_data;
>> +
>> + if (!if_then_else)
>> + die(_("format: %%(then) atom used without an %%(if) atom"));
>
> You've just casted at_end_data to if_then_else. if_then_else being not
> NULL does not mean that it is properly typed. It can be the at_end_data
> of another opening atom. What happens if you use
> %(align)foo%(then)bar%(end)?
>
Nice catch, didn't see that
Re: [RFC/PATCH v1] Add Travis CI support
Hi Junio, On 2015-10-04 03:37, Junio C Hamano wrote: > Junio C Hamano writes: > >> On Sat, Oct 3, 2015 at 3:23 PM, Roberto Tyley >> wrote: >>> >>> Given this, enabling Travis CI for git/git seems pretty low risk, >>> are there any strong objections to it happening? >> >> I still don't see a reason why git/git needs to be the one that is >> used, when somebody >> so interested (and I seem to see very many of them in the thread) can >> sacrifice his or >> her own fork and enable it him or herself. > > To state it a bit differently. > > If somebody says "I've been maintaining a clone of git/git with > Travis webhooks enabled and as the result caught this many glitches > during the past two months without any ill side effect. Heh... given that Travis CI requires that .travis.yml file, nobody can really say that they have been using Travis CI *before* you add that file to `master`. If you make successful testing with Travis a *precondition* before adding that file, it is kinda asking for the impossible. Now, I like Travis, even if I have used Jenkins previously (came as part of my previous day-job). And my experience with Jenkins (in the form of BuildHive) was pretty positive: it *did* catch a couple of breakages. Even with my Git fork. But I agree with basically everybody who chimed in and said that the biggest bang for the buck would be made by enabling it on https://github.com/git/git. The only cost I see is for that `.travis.yml` file to live in Git's source code. Small price to pay, if you ask me. If you do not want to use it yourself, that is fine. But I would like to ask for it to be included so that those of us who do want to benefit from Travis' testing are not precluded from doing so [*1*]. As far as I can tell, the patch is fine as-is. Although I would put the `before_script` commands into some file inside `contrib/`. Thanks, Dscho Footnote *1*: of course it would be possible to manually rebase the patch, or to set up a scripted version of that. That is very cumbersome, though, and the benefit would obviously be substantially diminished. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH v1] Add Travis CI support
On za, 2015-10-03 at 18:37 -0700, Junio C Hamano wrote: > If somebody says "I've been maintaining a clone of git/git with > Travis webhooks enabled and as the result caught this many glitches > during the past two months without any ill side effect. I've been maintaining a clone of git/git with a different ci system enabled, and it hasn't really caught anything. Only the occasional test failure in pu like the one I mailed about yesterday. The automated testing of pull requests could be useful, but pull requests don't seem to be used much yet. -- Dennis Kaarsemaker www.kaarsemaker.net -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/9] ref-filter: implement %(if), %(then), and %(else) atoms
Junio C Hamano writes: > Matthieu Moy writes: > >> My take on it: >> >> Implement %(if), %(then) and %(else) atoms. Used as >> %(if)...%(then)...%(end) or %(if)...%(then)...%(else)...%(end). If the >> format string between %(if) and %(then) expands to an empty string, or >> to only whitespaces, then the string following %(then) is printed. >> Otherwise, the string following %(else), if any, is printed. > > I found all the suggestions very good, except that the distinction > between "expands to" and "is printed" bothers me a bit, as they want > to mean exactly the same thing (imagine this whole thing were inside > another %(if)...%(then)). True. Then let me try again: Implement %(if), %(then) and %(else) atoms. Used as %(if)...%(then)...%(end) or %(if)...%(then)...%(else)...%(end). If the format string between %(if) and %(then) expands to an empty string, or to only whitespaces, then the whole %(if)...%(end) expands to the string following %(then). Otherwise, it expands to the string following %(else), if any. -- 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: [RFC/PATCH v1] Add Travis CI support
Junio C Hamano writes: > On Sat, Oct 3, 2015 at 3:23 PM, Roberto Tyley wrote: >> >> Given this, enabling Travis CI for git/git seems pretty low risk, >> are there any strong objections to it happening? > > I still don't see a reason why git/git needs to be the one that is > used, The very nice thing with Travis-CI is that it does not only test the repository's branches, but also all pull-requests. So, if it is activated on git/git, it will become possible to have a flow like 1) User pushes to his own repo, sends a pull-request, 2) Travis-CI notices the pull-request and builds it (no action needed from anyone), 3) Once the build is finished, the user can use e.g. SubmitGit to actually submit the code. This has real benefits for the submitter (know if your code is broken early), for the reviewers (things like "you have a def-after-use" would be noticed by a computer before human beings start spending time on the review), and for you (some issues noticed before a topic enters pu). There's no extra work for the user at all compared to the standard pull-request flow (nothing to do, just submit a PR), and a one-time setup for the project. Currenty, to mimick this flow, we would need something like 1) User activates Travis-CI on his repo (each user would have to do this, not just once) 2) User commits .travis.yml on top of the code to submit 3) User pushes to his repo 4) Travis-CI triggers a build 5) User removes the commit introducing .travis.yml, force-pushes 6) User submit the resulting code. This is much more work for the user (read: nobody will do it, actually nobody do it currently) and less convenient for reviewers (who have no way to check whether the build passed). -- 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
