* [PATCH 08/36] attr.c: tighten constness around "git_attr" structure
From: Stefan Beller @ 2016-10-22 23:31 UTC (permalink / raw)
To: gitster; +Cc: git, bmwill, pclouds, Stefan Beller
In-Reply-To: <20161022233225.8883-1-sbeller@google.com>
From: Junio C Hamano <gitster@pobox.com>
It holds an interned string, and git_attr_name() is a way to peek
into it. Make sure the involved pointer types are pointer-to-const.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
attr.c | 2 +-
attr.h | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/attr.c b/attr.c
index 7bfeef3..5c35d42 100644
--- a/attr.c
+++ b/attr.c
@@ -43,7 +43,7 @@ static int cannot_trust_maybe_real;
static struct git_attr_check *check_all_attr;
static struct git_attr *(git_attr_hash[HASHSIZE]);
-char *git_attr_name(struct git_attr *attr)
+const char *git_attr_name(const struct git_attr *attr)
{
return attr->name;
}
diff --git a/attr.h b/attr.h
index 8b08d33..00d7a66 100644
--- a/attr.h
+++ b/attr.h
@@ -25,7 +25,7 @@ extern const char git_attr__false[];
* Unset one is returned as NULL.
*/
struct git_attr_check {
- struct git_attr *attr;
+ const struct git_attr *attr;
const char *value;
};
@@ -34,7 +34,7 @@ struct git_attr_check {
* return value is a pointer to a null-delimited string that is part
* of the internal data structure; it should not be modified or freed.
*/
-char *git_attr_name(struct git_attr *);
+extern const char *git_attr_name(const struct git_attr *);
int git_check_attr(const char *path, int, struct git_attr_check *);
--
2.10.1.508.g6572022
^ permalink raw reply related
* [PATCH 10/36] attr: rename function and struct related to checking attributes
From: Stefan Beller @ 2016-10-22 23:31 UTC (permalink / raw)
To: gitster; +Cc: git, bmwill, pclouds, Stefan Beller
In-Reply-To: <20161022233225.8883-1-sbeller@google.com>
From: Junio C Hamano <gitster@pobox.com>
The traditional API to check attributes is to prepare an N-element
array of "struct git_attr_check" and pass N and the array to the
function "git_check_attr()" as arguments.
In preparation to revamp the API to pass a single structure, in
which these N elements are held, rename the type used for these
individual array elements to "struct git_attr_check_elem" and rename
the function to "git_check_attrs()".
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
archive.c | 6 +++---
attr.c | 12 ++++++------
attr.h | 8 ++++----
builtin/check-attr.c | 19 ++++++++++---------
builtin/pack-objects.c | 6 +++---
convert.c | 12 ++++++------
ll-merge.c | 10 +++++-----
userdiff.c | 4 ++--
ws.c | 6 +++---
9 files changed, 42 insertions(+), 41 deletions(-)
diff --git a/archive.c b/archive.c
index dde1ab4..2dc8d6c 100644
--- a/archive.c
+++ b/archive.c
@@ -87,7 +87,7 @@ void *sha1_file_to_archive(const struct archiver_args *args,
return buffer;
}
-static void setup_archive_check(struct git_attr_check *check)
+static void setup_archive_check(struct git_attr_check_elem *check)
{
static struct git_attr *attr_export_ignore;
static struct git_attr *attr_export_subst;
@@ -123,7 +123,7 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,
struct archiver_context *c = context;
struct archiver_args *args = c->args;
write_archive_entry_fn_t write_entry = c->write_entry;
- struct git_attr_check check[2];
+ struct git_attr_check_elem check[2];
const char *path_without_prefix;
int err;
@@ -138,7 +138,7 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,
path_without_prefix = path.buf + args->baselen;
setup_archive_check(check);
- if (!git_check_attr(path_without_prefix, ARRAY_SIZE(check), check)) {
+ if (!git_check_attrs(path_without_prefix, ARRAY_SIZE(check), check)) {
if (ATTR_TRUE(check[0].value))
return 0;
args->convert = ATTR_TRUE(check[1].value);
diff --git a/attr.c b/attr.c
index 1877f7a..c99e23a 100644
--- a/attr.c
+++ b/attr.c
@@ -40,7 +40,7 @@ struct git_attr {
static int attr_nr;
static int cannot_trust_maybe_real;
-static struct git_attr_check *check_all_attr;
+static struct git_attr_check_elem *check_all_attr;
static struct git_attr *(git_attr_hash[HASHSIZE]);
const char *git_attr_name(const struct git_attr *attr)
@@ -665,7 +665,7 @@ static int macroexpand_one(int attr_nr, int rem);
static int fill_one(const char *what, struct match_attr *a, int rem)
{
- struct git_attr_check *check = check_all_attr;
+ struct git_attr_check_elem *check = check_all_attr;
int i;
for (i = a->num_attr - 1; 0 < rem && 0 <= i; i--) {
@@ -730,7 +730,7 @@ static int macroexpand_one(int nr, int rem)
* collected. Otherwise all attributes are collected.
*/
static void collect_some_attrs(const char *path, int num,
- struct git_attr_check *check)
+ struct git_attr_check_elem *check)
{
struct attr_stack *stk;
@@ -758,7 +758,7 @@ static void collect_some_attrs(const char *path, int num,
rem = 0;
for (i = 0; i < num; i++) {
if (!check[i].attr->maybe_real) {
- struct git_attr_check *c;
+ struct git_attr_check_elem *c;
c = check_all_attr + check[i].attr->attr_nr;
c->value = ATTR__UNSET;
rem++;
@@ -773,7 +773,7 @@ static void collect_some_attrs(const char *path, int num,
rem = fill(path, pathlen, basename_offset, stk, rem);
}
-int git_check_attr(const char *path, int num, struct git_attr_check *check)
+int git_check_attrs(const char *path, int num, struct git_attr_check_elem *check)
{
int i;
@@ -789,7 +789,7 @@ int git_check_attr(const char *path, int num, struct git_attr_check *check)
return 0;
}
-int git_all_attrs(const char *path, int *num, struct git_attr_check **check)
+int git_all_attrs(const char *path, int *num, struct git_attr_check_elem **check)
{
int i, count, j;
diff --git a/attr.h b/attr.h
index 00d7a66..dd3c4a3 100644
--- a/attr.h
+++ b/attr.h
@@ -20,11 +20,11 @@ extern const char git_attr__false[];
#define ATTR_UNSET(v) ((v) == NULL)
/*
- * Send one or more git_attr_check to git_check_attr(), and
+ * Send one or more git_attr_check to git_check_attrs(), and
* each 'value' member tells what its value is.
* Unset one is returned as NULL.
*/
-struct git_attr_check {
+struct git_attr_check_elem {
const struct git_attr *attr;
const char *value;
};
@@ -36,7 +36,7 @@ struct git_attr_check {
*/
extern const char *git_attr_name(const struct git_attr *);
-int git_check_attr(const char *path, int, struct git_attr_check *);
+int git_check_attrs(const char *path, int, struct git_attr_check_elem *);
/*
* Retrieve all attributes that apply to the specified path. *num
@@ -45,7 +45,7 @@ int git_check_attr(const char *path, int, struct git_attr_check *);
* objects describing the attributes and their values. *check must be
* free()ed by the caller.
*/
-int git_all_attrs(const char *path, int *num, struct git_attr_check **check);
+int git_all_attrs(const char *path, int *num, struct git_attr_check_elem **check);
enum git_attr_direction {
GIT_ATTR_CHECKIN,
diff --git a/builtin/check-attr.c b/builtin/check-attr.c
index 53a5a18..97e3837 100644
--- a/builtin/check-attr.c
+++ b/builtin/check-attr.c
@@ -24,8 +24,8 @@ static const struct option check_attr_options[] = {
OPT_END()
};
-static void output_attr(int cnt, struct git_attr_check *check,
- const char *file)
+static void output_attr(int cnt, struct git_attr_check_elem *check,
+ const char *file)
{
int j;
for (j = 0; j < cnt; j++) {
@@ -51,14 +51,15 @@ static void output_attr(int cnt, struct git_attr_check *check,
}
}
-static void check_attr(const char *prefix, int cnt,
- struct git_attr_check *check, const char *file)
+static void check_attr(const char *prefix,
+ int cnt, struct git_attr_check_elem *check,
+ const char *file)
{
char *full_path =
prefix_path(prefix, prefix ? strlen(prefix) : 0, file);
if (check != NULL) {
- if (git_check_attr(full_path, cnt, check))
- die("git_check_attr died");
+ if (git_check_attrs(full_path, cnt, check))
+ die("git_check_attrs died");
output_attr(cnt, check, file);
} else {
if (git_all_attrs(full_path, &cnt, &check))
@@ -69,8 +70,8 @@ static void check_attr(const char *prefix, int cnt,
free(full_path);
}
-static void check_attr_stdin_paths(const char *prefix, int cnt,
- struct git_attr_check *check)
+static void check_attr_stdin_paths(const char *prefix,
+ int cnt, struct git_attr_check_elem *check)
{
struct strbuf buf = STRBUF_INIT;
struct strbuf unquoted = STRBUF_INIT;
@@ -99,7 +100,7 @@ static NORETURN void error_with_usage(const char *msg)
int cmd_check_attr(int argc, const char **argv, const char *prefix)
{
- struct git_attr_check *check;
+ struct git_attr_check_elem *check;
int cnt, i, doubledash, filei;
if (!is_bare_repository())
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 1e7c2a9..3cb38ed 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -896,7 +896,7 @@ static void write_pack_file(void)
written, nr_result);
}
-static void setup_delta_attr_check(struct git_attr_check *check)
+static void setup_delta_attr_check(struct git_attr_check_elem *check)
{
static struct git_attr *attr_delta;
@@ -908,10 +908,10 @@ static void setup_delta_attr_check(struct git_attr_check *check)
static int no_try_delta(const char *path)
{
- struct git_attr_check check[1];
+ struct git_attr_check_elem check[1];
setup_delta_attr_check(check);
- if (git_check_attr(path, ARRAY_SIZE(check), check))
+ if (git_check_attrs(path, ARRAY_SIZE(check), check))
return 0;
if (ATTR_FALSE(check->value))
return 1;
diff --git a/convert.c b/convert.c
index 077f5e6..c95ae71 100644
--- a/convert.c
+++ b/convert.c
@@ -718,7 +718,7 @@ static int ident_to_worktree(const char *path, const char *src, size_t len,
return 1;
}
-static enum crlf_action git_path_check_crlf(struct git_attr_check *check)
+static enum crlf_action git_path_check_crlf(struct git_attr_check_elem *check)
{
const char *value = check->value;
@@ -735,7 +735,7 @@ static enum crlf_action git_path_check_crlf(struct git_attr_check *check)
return CRLF_UNDEFINED;
}
-static enum eol git_path_check_eol(struct git_attr_check *check)
+static enum eol git_path_check_eol(struct git_attr_check_elem *check)
{
const char *value = check->value;
@@ -748,7 +748,7 @@ static enum eol git_path_check_eol(struct git_attr_check *check)
return EOL_UNSET;
}
-static struct convert_driver *git_path_check_convert(struct git_attr_check *check)
+static struct convert_driver *git_path_check_convert(struct git_attr_check_elem *check)
{
const char *value = check->value;
struct convert_driver *drv;
@@ -761,7 +761,7 @@ static struct convert_driver *git_path_check_convert(struct git_attr_check *chec
return NULL;
}
-static int git_path_check_ident(struct git_attr_check *check)
+static int git_path_check_ident(struct git_attr_check_elem *check)
{
const char *value = check->value;
@@ -783,7 +783,7 @@ static const char *conv_attr_name[] = {
static void convert_attrs(struct conv_attrs *ca, const char *path)
{
int i;
- static struct git_attr_check ccheck[NUM_CONV_ATTRS];
+ static struct git_attr_check_elem ccheck[NUM_CONV_ATTRS];
if (!ccheck[0].attr) {
for (i = 0; i < NUM_CONV_ATTRS; i++)
@@ -792,7 +792,7 @@ static void convert_attrs(struct conv_attrs *ca, const char *path)
git_config(read_convert_config, NULL);
}
- if (!git_check_attr(path, NUM_CONV_ATTRS, ccheck)) {
+ if (!git_check_attrs(path, NUM_CONV_ATTRS, ccheck)) {
ca->crlf_action = git_path_check_crlf(ccheck + 4);
if (ca->crlf_action == CRLF_UNDEFINED)
ca->crlf_action = git_path_check_crlf(ccheck + 0);
diff --git a/ll-merge.c b/ll-merge.c
index ad8be42..eb2c37e 100644
--- a/ll-merge.c
+++ b/ll-merge.c
@@ -336,13 +336,13 @@ static const struct ll_merge_driver *find_ll_merge_driver(const char *merge_attr
return &ll_merge_drv[LL_TEXT_MERGE];
}
-static int git_path_check_merge(const char *path, struct git_attr_check check[2])
+static int git_path_check_merge(const char *path, struct git_attr_check_elem check[2])
{
if (!check[0].attr) {
check[0].attr = git_attr("merge");
check[1].attr = git_attr("conflict-marker-size");
}
- return git_check_attr(path, 2, check);
+ return git_check_attrs(path, 2, check);
}
static void normalize_file(mmfile_t *mm, const char *path)
@@ -362,7 +362,7 @@ int ll_merge(mmbuffer_t *result_buf,
mmfile_t *theirs, const char *their_label,
const struct ll_merge_options *opts)
{
- static struct git_attr_check check[2];
+ static struct git_attr_check_elem check[2];
static const struct ll_merge_options default_opts;
const char *ll_driver_name = NULL;
int marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
@@ -398,12 +398,12 @@ int ll_merge(mmbuffer_t *result_buf,
int ll_merge_marker_size(const char *path)
{
- static struct git_attr_check check;
+ static struct git_attr_check_elem check;
int marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
if (!check.attr)
check.attr = git_attr("conflict-marker-size");
- if (!git_check_attr(path, 1, &check) && check.value) {
+ if (!git_check_attrs(path, 1, &check) && check.value) {
marker_size = atoi(check.value);
if (marker_size <= 0)
marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
diff --git a/userdiff.c b/userdiff.c
index 2125d6d..4de3289 100644
--- a/userdiff.c
+++ b/userdiff.c
@@ -263,7 +263,7 @@ struct userdiff_driver *userdiff_find_by_name(const char *name) {
struct userdiff_driver *userdiff_find_by_path(const char *path)
{
static struct git_attr *attr;
- struct git_attr_check check;
+ struct git_attr_check_elem check;
if (!attr)
attr = git_attr("diff");
@@ -271,7 +271,7 @@ struct userdiff_driver *userdiff_find_by_path(const char *path)
if (!path)
return NULL;
- if (git_check_attr(path, 1, &check))
+ if (git_check_attrs(path, 1, &check))
return NULL;
if (ATTR_TRUE(check.value))
diff --git a/ws.c b/ws.c
index ea4b2b1..7350905 100644
--- a/ws.c
+++ b/ws.c
@@ -71,7 +71,7 @@ unsigned parse_whitespace_rule(const char *string)
return rule;
}
-static void setup_whitespace_attr_check(struct git_attr_check *check)
+static void setup_whitespace_attr_check(struct git_attr_check_elem *check)
{
static struct git_attr *attr_whitespace;
@@ -82,10 +82,10 @@ static void setup_whitespace_attr_check(struct git_attr_check *check)
unsigned whitespace_rule(const char *pathname)
{
- struct git_attr_check attr_whitespace_rule;
+ struct git_attr_check_elem attr_whitespace_rule;
setup_whitespace_attr_check(&attr_whitespace_rule);
- if (!git_check_attr(pathname, 1, &attr_whitespace_rule)) {
+ if (!git_check_attrs(pathname, 1, &attr_whitespace_rule)) {
const char *value;
value = attr_whitespace_rule.value;
--
2.10.1.508.g6572022
^ permalink raw reply related
* [PATCH 09/36] attr.c: plug small leak in parse_attr_line()
From: Stefan Beller @ 2016-10-22 23:31 UTC (permalink / raw)
To: gitster; +Cc: git, bmwill, pclouds, Stefan Beller
In-Reply-To: <20161022233225.8883-1-sbeller@google.com>
From: Junio C Hamano <gitster@pobox.com>
If any error is noticed after the match_attr structure is allocated,
we shouldn't just return NULL from this function.
Add a fail_return label that frees the allocated structure and
returns NULL, and consistently jump there when we want to return
NULL after cleaning up.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
attr.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/attr.c b/attr.c
index 5c35d42..1877f7a 100644
--- a/attr.c
+++ b/attr.c
@@ -223,7 +223,7 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,
if (!macro_ok) {
fprintf(stderr, "%s not allowed: %s:%d\n",
name, src, lineno);
- return NULL;
+ goto fail_return;
}
is_macro = 1;
name += strlen(ATTRIBUTE_MACRO_PREFIX);
@@ -233,7 +233,7 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,
fprintf(stderr,
"%.*s is not a valid attribute name: %s:%d\n",
namelen, name, src, lineno);
- return NULL;
+ goto fail_return;
}
}
else
@@ -246,7 +246,7 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,
for (cp = states, num_attr = 0; *cp; num_attr++) {
cp = parse_attr(src, lineno, cp, NULL);
if (!cp)
- return NULL;
+ goto fail_return;
}
res = xcalloc(1,
@@ -267,7 +267,7 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,
if (res->u.pat.flags & EXC_FLAG_NEGATIVE) {
warning(_("Negative patterns are ignored in git attributes\n"
"Use '\\!' for literal leading exclamation."));
- return NULL;
+ goto fail_return;
}
}
res->is_macro = is_macro;
@@ -283,6 +283,10 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,
}
return res;
+
+fail_return:
+ free(res);
+ return NULL;
}
/*
--
2.10.1.508.g6572022
^ permalink raw reply related
* [PATCH 07/36] attr.c: simplify macroexpand_one()
From: Stefan Beller @ 2016-10-22 23:31 UTC (permalink / raw)
To: gitster; +Cc: git, bmwill, pclouds, Stefan Beller
In-Reply-To: <20161022233225.8883-1-sbeller@google.com>
From: Junio C Hamano <gitster@pobox.com>
The double-loop wants to do an early return immediately when one
matching macro is found. Eliminate the extra variable 'a' used for
that purpose and rewrite the "assign the found item to 'a' to make
it non-NULL and force the loop(s) to terminate" with a direct return
from there.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
attr.c | 11 ++++-------
1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/attr.c b/attr.c
index 95416d3..7bfeef3 100644
--- a/attr.c
+++ b/attr.c
@@ -701,24 +701,21 @@ static int fill(const char *path, int pathlen, int basename_offset,
static int macroexpand_one(int nr, int rem)
{
struct attr_stack *stk;
- struct match_attr *a = NULL;
int i;
if (check_all_attr[nr].value != ATTR__TRUE ||
!check_all_attr[nr].attr->maybe_macro)
return rem;
- for (stk = attr_stack; !a && stk; stk = stk->prev)
- for (i = stk->num_matches - 1; !a && 0 <= i; i--) {
+ for (stk = attr_stack; stk; stk = stk->prev) {
+ for (i = stk->num_matches - 1; 0 <= i; i--) {
struct match_attr *ma = stk->attrs[i];
if (!ma->is_macro)
continue;
if (ma->u.attr->attr_nr == nr)
- a = ma;
+ return fill_one("expand", ma, rem);
}
-
- if (a)
- rem = fill_one("expand", a, rem);
+ }
return rem;
}
--
2.10.1.508.g6572022
^ permalink raw reply related
* [PATCH 03/36] attr.c: update a stale comment on "struct match_attr"
From: Stefan Beller @ 2016-10-22 23:31 UTC (permalink / raw)
To: gitster; +Cc: git, bmwill, pclouds, Stefan Beller
In-Reply-To: <20161022233225.8883-1-sbeller@google.com>
From: Junio C Hamano <gitster@pobox.com>
When 82dce998 (attr: more matching optimizations from .gitignore,
2012-10-15) changed a pointer to a string "*pattern" into an
embedded "struct pattern" in struct match_attr, it forgot to update
the comment that describes the structure.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
attr.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/attr.c b/attr.c
index 45aec1b..4ae7801 100644
--- a/attr.c
+++ b/attr.c
@@ -131,9 +131,8 @@ struct pattern {
* If is_macro is true, then u.attr is a pointer to the git_attr being
* defined.
*
- * If is_macro is false, then u.pattern points at the filename pattern
- * to which the rule applies. (The memory pointed to is part of the
- * memory block allocated for the match_attr instance.)
+ * If is_macro is false, then u.pat is the filename pattern to which the
+ * rule applies.
*
* In either case, num_attr is the number of attributes affected by
* this rule, and state is an array listing them. The attributes are
--
2.10.1.508.g6572022
^ permalink raw reply related
* [PATCH 05/36] attr.c: complete a sentence in a comment
From: Stefan Beller @ 2016-10-22 23:31 UTC (permalink / raw)
To: gitster; +Cc: git, bmwill, pclouds, Stefan Beller
In-Reply-To: <20161022233225.8883-1-sbeller@google.com>
From: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
attr.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/attr.c b/attr.c
index 05db667..a7f2c3f 100644
--- a/attr.c
+++ b/attr.c
@@ -300,7 +300,7 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,
* directory (again, reading the file from top to bottom) down to the
* current directory, and then scan the list backwards to find the first match.
* This is exactly the same as what is_excluded() does in dir.c to deal with
- * .gitignore
+ * .gitignore file and info/excludes file as a fallback.
*/
static struct attr_stack {
--
2.10.1.508.g6572022
^ permalink raw reply related
* [PATCH 06/36] attr.c: mark where #if DEBUG ends more clearly
From: Stefan Beller @ 2016-10-22 23:31 UTC (permalink / raw)
To: gitster; +Cc: git, bmwill, pclouds, Stefan Beller
In-Reply-To: <20161022233225.8883-1-sbeller@google.com>
From: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
attr.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/attr.c b/attr.c
index a7f2c3f..95416d3 100644
--- a/attr.c
+++ b/attr.c
@@ -469,7 +469,7 @@ static void debug_set(const char *what, const char *match, struct git_attr *attr
#define debug_push(a) do { ; } while (0)
#define debug_pop(a) do { ; } while (0)
#define debug_set(a,b,c,d) do { ; } while (0)
-#endif
+#endif /* DEBUG_ATTR */
static void drop_attr_stack(void)
{
--
2.10.1.508.g6572022
^ permalink raw reply related
* [PATCH 04/36] attr.c: explain the lack of attr-name syntax check in parse_attr()
From: Stefan Beller @ 2016-10-22 23:31 UTC (permalink / raw)
To: gitster; +Cc: git, bmwill, pclouds, Stefan Beller
In-Reply-To: <20161022233225.8883-1-sbeller@google.com>
From: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
attr.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/attr.c b/attr.c
index 4ae7801..05db667 100644
--- a/attr.c
+++ b/attr.c
@@ -183,6 +183,12 @@ static const char *parse_attr(const char *src, int lineno, const char *cp,
return NULL;
}
} else {
+ /*
+ * As this function is always called twice, once with
+ * e == NULL in the first pass and then e != NULL in
+ * the second pass, no need for invalid_attr_name()
+ * check here.
+ */
if (*cp == '-' || *cp == '!') {
e->setto = (*cp == '-') ? ATTR__FALSE : ATTR__UNSET;
cp++;
--
2.10.1.508.g6572022
^ permalink raw reply related
* [PATCH 02/36] attr.c: use strchrnul() to scan for one line
From: Stefan Beller @ 2016-10-22 23:31 UTC (permalink / raw)
To: gitster; +Cc: git, bmwill, pclouds, Stefan Beller
In-Reply-To: <20161022233225.8883-1-sbeller@google.com>
From: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
attr.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/attr.c b/attr.c
index eec5d7d..45aec1b 100644
--- a/attr.c
+++ b/attr.c
@@ -402,8 +402,8 @@ static struct attr_stack *read_attr_from_index(const char *path, int macro_ok)
for (sp = buf; *sp; ) {
char *ep;
int more;
- for (ep = sp; *ep && *ep != '\n'; ep++)
- ;
+
+ ep = strchrnul(sp, '\n');
more = (*ep == '\n');
*ep = '\0';
handle_attr_line(res, sp, path, ++lineno, macro_ok);
--
2.10.1.508.g6572022
^ permalink raw reply related
* [PATCH 01/36] commit.c: use strchrnul() to scan for one line
From: Stefan Beller @ 2016-10-22 23:31 UTC (permalink / raw)
To: gitster; +Cc: git, bmwill, pclouds, Stefan Beller
In-Reply-To: <20161022233225.8883-1-sbeller@google.com>
From: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
commit.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/commit.c b/commit.c
index 856fd4a..41b2fdd 100644
--- a/commit.c
+++ b/commit.c
@@ -415,8 +415,7 @@ int find_commit_subject(const char *commit_buffer, const char **subject)
p++;
if (*p) {
p = skip_blank_lines(p + 2);
- for (eol = p; *eol && *eol != '\n'; eol++)
- ; /* do nothing */
+ eol = strchrnul(p, '\n');
} else
eol = p;
--
2.10.1.508.g6572022
^ permalink raw reply related
* [PATCHv2 00/36] Revamping the attr subsystem!
From: Stefan Beller @ 2016-10-22 23:31 UTC (permalink / raw)
To: gitster; +Cc: git, bmwill, pclouds, Stefan Beller
previous discussion:
http://public-inbox.org/git/20161012224109.23410-1-sbeller@google.com
http://public-inbox.org/git/20161011002115.23312-1-sbeller@google.com/
This implements the discarded series':
jc/attr
jc/attr-more
sb/pathspec-label
sb/submodule-default-paths
* I rebase to origin master (no merge conflicts)
* I implemented the thread safe attr API in patch 27 (attr: convert to new threadsafe API)
* patch 28 (attr: keep attr stack for each check) makes it actually possible
to run in a multithreaded environment.
* I added a test for the multithreaded when it is introduced in patch 32
(pathspec: allow querying for attributes)
Thanks,
Stefan
Junio C Hamano (24):
commit.c: use strchrnul() to scan for one line
attr.c: use strchrnul() to scan for one line
attr.c: update a stale comment on "struct match_attr"
attr.c: explain the lack of attr-name syntax check in parse_attr()
attr.c: complete a sentence in a comment
attr.c: mark where #if DEBUG ends more clearly
attr.c: simplify macroexpand_one()
attr.c: tighten constness around "git_attr" structure
attr.c: plug small leak in parse_attr_line()
attr: rename function and struct related to checking attributes
attr: (re)introduce git_check_attr() and struct git_attr_check
attr: convert git_all_attrs() to use "struct git_attr_check"
attr: convert git_check_attrs() callers to use the new API
attr: retire git_check_attrs() API
attr: add counted string version of git_check_attr()
attr: add counted string version of git_attr()
attr: expose validity check for attribute names
attr.c: add push_stack() helper
attr.c: pass struct git_attr_check down the callchain
attr.c: rename a local variable check
attr.c: correct ugly hack for git_all_attrs()
attr.c: introduce empty_attr_check_elems()
attr.c: always pass check[] to collect_some_attrs()
attr.c: outline the future plans by heavily commenting
Nguyễn Thái Ngọc Duy (1):
attr: support quoting pathname patterns in C style
Stefan Beller (11):
attr: make git_check_attr_counted static
attr: convert to new threadsafe API
attr: keep attr stack for each check
Documentation: fix a typo
pathspec: move long magic parsing out of prefix_pathspec
pathspec: move prefix check out of the inner loop
pathspec: allow querying for attributes
pathspec: allow escaped query values
submodule update: add `--init-default-path` switch
clone: add --init-submodule=<pathspec> switch
completion: clone can initialize specific submodules
Documentation/config.txt | 5 +
Documentation/git-clone.txt | 23 +-
Documentation/git-submodule.txt | 17 +-
Documentation/gitattributes.txt | 10 +-
Documentation/glossary-content.txt | 20 +
Documentation/technical/api-gitattributes.txt | 117 ++++--
archive.c | 26 +-
attr.c | 530 ++++++++++++++++++--------
attr.h | 74 +++-
builtin/check-attr.c | 65 ++--
builtin/clone.c | 36 +-
builtin/pack-objects.c | 27 +-
commit.c | 3 +-
contrib/completion/git-completion.bash | 1 +
convert.c | 45 +--
dir.c | 35 ++
git-submodule.sh | 21 +-
hashmap.h | 2 +
ll-merge.c | 36 +-
pathspec.c | 225 +++++++++--
pathspec.h | 16 +
t/t0003-attributes.sh | 26 ++
t/t6134-pathspec-with-labels.sh | 180 +++++++++
t/t7400-submodule-basic.sh | 134 +++++++
userdiff.c | 21 +-
ws.c | 26 +-
26 files changed, 1313 insertions(+), 408 deletions(-)
create mode 100755 t/t6134-pathspec-with-labels.sh
--
2.10.1.508.g6572022
^ permalink raw reply
* Stash pop/apply conflict and --theirs and --ours
From: Sven Strickroth @ 2016-10-22 22:58 UTC (permalink / raw)
To: Git List, Junio C Hamano, Jeff King
Hi,
I regularly experience that beginners have problems unterstanding that
--ours and --theirs are swapped when a conflict occurrs on git stash
apply or stash pop.
From the HCI perspective this is really counter intuitive.
So, I'd like to propose that on git shash pop/apply theirs and ours
should be swapped in git index, so that git checkout --theirs and --ours
work as expected.
PS: I'm sorry if this was already discussed, I haven't found any discussion.
--
Best regards,
Sven Strickroth
PGP key id F5A9D4C4 @ any key-server
^ permalink raw reply
* Re: [PATCH 2/3] submodule tests: replace cloning from . by "$(pwd)"
From: Stefan Beller @ 2016-10-22 20:46 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johannes Sixt, Johannes Schindelin, git@vger.kernel.org, Karl A.,
Dennis Kaarsemaker, Jonathan Nieder
In-Reply-To: <xmqqh984aldl.fsf@gitster.mtv.corp.google.com>
On Sat, Oct 22, 2016 at 12:33 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Johannes Sixt <j6t@kdbg.org> writes:
>
>>> The logic to construct the relative urls is not smart enough to
>>> detect that the ending /. is referring to the directory itself
>>> but rather treats it like any other relative path, i.e.
>>>
>>> path/to/dir/. + ../relative/path/to/submodule
>>>
>>> would result in
>>>
>>> path/to/dir/relative/path/to/submodule
>>>
>>> and not omit the "dir" as you may expect.
>>>
>>> As in a later patch we'll normalize the remote url before the
>>> computation of relative urls takes place, we need to first get our
>>> test suite in a shape with normalized urls only, which is why we should
>>> refrain from cloning from '.'
>>
>> But you are removing a valid use case from the tests. Aren't you
>> sweeping something under the rug with this patch?
>
> I share the same reaction.
Oh I see. I agree.
I reverted the lines that replace . by "$(pwd)" and it still works, but it
still works because the code path regarding local paths was not broken
(both . as well as "$(pwd)" are working without the fix)
>
> If the primary problem being solved is that the combination of a
> relative URL ../sub and the base URL for the superproject which is
> set to /path/to/dir/. (due to "clone .") were incorrectly resolved
> as /path/to/dir/sub (because the buggy relative path logic did not
> know that removing "/." at the end does not take you to one level
> up), and a topic that fixes the bug would make that relative URL
> ../sub to be resolved as /path/to/sub, of course. Otherwise, the
> topic did not fix the bug.
>
> Now if a test that wanted to have a clone of the superproject by
> "clone .", which results in the base URL of /path/to/dir/., actually
> wants to refer in its .gitmodules to /path/to/dir/sub (which after
> all was where the submodule the test created with or without the
> bugfix), I would think the right adjustment for the test that used
> to rely on the buggy behaviour would be to stop using ../sub and
> instead use ./sub as the relative URL, no? After all, the bug forced
> the original test writer to write ../sub but the submodule in this
> case actually is at ./sub relative to its superproject, and that is
> what the original test writer would have written if the bug weren't
> there in the first place, no?
True.
I have looked into it again, and by now I think the bug is a feature,
actually.
Consider this:
git clone . super
git -C super submodule add ../submodule
# we thought the previous line is buggy
git clone super super-clone
Now in the super-clone the ../submodule is the correct
relative url, because the url where we cloned from doesn't
end in /.
If we were to fix the bug with the /. ending, then we would need the
following:
git clone . super
git -C super submodule add ./submodule
# correct relative URL above!
git clone super super-clone
cd superclone && sed s|url =./|url = ../|
# make sure we fix the relative URLs to account for a different remote.
And this doesn't seem right to me as it burdens the user of the super-clone.
>
> Another thing I do not quite understand is why this step comes
> before the fix. If the "clone ." is adjusted to avoid triggering
> the buggy behaviour, i.e. making the base URL to /path/to/dir
> (instead of /path/to/dir/.), wouldn't the relative URL ../sub that
> was written to work around the bug that hasn't been fixed yet in
> this step need to be adjusted anyway? It would end up referring to
> /path/to/sub and not /path/to/dir/sub until the fix is put in place.
>
> Is the removal of remote.origin.url a wrong workaround for that
> breakage, I wonder... I do not understand that change at all, and I
> do not think it was explained in the log message.
I think it is wrong, because it is sidestepping the actual issue.
Continuing from above:
git clone super-clone super-clone2
# this works again, as the remote change doesn't matter.
mkdir test && git -C test clone ../ .
# submodule urls need to be "undone again to work:
cd test && sed s|url =../|url = ./|
So I think keeping the /. around as it currently is, is a pretty
useful bug.
>
> If we really wanted to update the test before fixing the bug, I
> would understand if this step changed ../sub (or whatever relative
> URL that has extra ../ only because the base URL has extra /. at the
> end to compensate for the buggy resolution) to ./sub in the tests
> and marked them to expect failure, and then a later step that fixes
> the bug turns them to expect success without make any other change.
I'll think about this further, but currently I am inclined to declare
it a nonbug
and as it eases testing a lot. Also if someone wants to clone a repository
with broken relative urls, they can work around that by e.g.
git clone <url>/.
to fix one level of indentation, though it is not documented and seems
to be brittle.
Thanks,
Stefan
^ permalink raw reply
* Re: [PATCH v5 00/27] Prepare the sequencer for the upcoming rebase -i patches
From: Junio C Hamano @ 2016-10-22 17:11 UTC (permalink / raw)
To: Johannes Schindelin
Cc: git, Stefan Beller, Jeff King, Jakub Narębski, Johannes Sixt,
Ramsay Jones
In-Reply-To: <cover.1477052405.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> This patch series marks the '4' in the countdown to speed up rebase -i
> by implementing large parts in C (read: there will be three more patch
> series after that before the full benefit hits git.git: sequencer-i,
> rebase--helper and rebase-i-extra).
> ...
> It would be *really* nice if we could get this patch series at least into
> `next` soon, as it gets late and later for the rest of the patches to make
> it into `master` in time for v2.11 (and it is not for lack of trying on my
> end...).
This "countdown 4" step can affect cherry-pick and revert, even
though we were careful to review changes to the sequencer.c code. I
prefer to cook it in 'next' sufficiently long to ensure that we hear
feedbacks from non-Windows users if there is any unexpected breakage.
There isn't enough time to include this topic in the upcoming
release within the current https://tinyurl.com/gitCal calendar,
however, which places the final on Nov 11th.
I am wondering if it makes sense to delay 2.11 by moving the final
by 4 weeks to Dec 9th.
Thoughts?
Speaking of what to and not to include in the upcoming release, we
do want to include Stefan's off-by-one fix to the submodule-helper,
but that is blocked on Windows end due to the test. I think
everybody agreed that a longer time "right thing to do" fix is to
address the "when base is /path/to/dir/., where is ../sub relative
to it?" issue, but if we are to do so, it would need a longer
gestation period once it hits 'next', as it can affect the current
users and we may even need B/C notes in the release notes for the
change. Giving ourselves a few more weeks of breathing room would
help us to make sure the fix to relative URL issue is sound, too.
As to "countdown 3" and below steps, I am guessing that some of them
can start cooking in 'next' before 2.11, but even with lengthened
schedule, it is likely that they need to cook there beyond the end
of this cycle, unless they are truly trivial changes that do not
even need any reviews.
Thanks.
^ permalink raw reply
* Re: [PATCH] daemon: detect and reject too-long paths
From: Junio C Hamano @ 2016-10-22 16:37 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20161022045938.h3xa3yapzlg427vy@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> When we are checking the path via path_ok(), we use some
> fixed PATH_MAX buffers. We write into them via snprintf(),
> so there's no possibility of overflow, but it does mean we
> may silently truncate the path, leading to potentially
> confusing errors when the partial path does not exist.
>
> We're better off to reject the path explicitly.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
Sounds sensible.
> Another option would be to switch to strbufs here. That potentially
> introduces cases where a client can convince us to just keep allocating
> memory, but I don't think so in practice; the paths and interpolated
> data items all have to come in 64K pkt-lines, which places a hard
> limit. This is a much more minimal change, though, and I don't hear
> anybody complaining about the inability to use large paths.
The alternative version did not look bad, either; in fact, the end
result may even be conceptually simpler.
But I agree that this one with the same hard-limit we always had is
a much more minimal change and is sufficient.
Thanks.
> daemon.c | 25 +++++++++++++++++++++----
> 1 file changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/daemon.c b/daemon.c
> index 425aad0507..ff0fa583b0 100644
> --- a/daemon.c
> +++ b/daemon.c
> @@ -160,6 +160,7 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)
> {
> static char rpath[PATH_MAX];
> static char interp_path[PATH_MAX];
> + size_t rlen;
> const char *path;
> const char *dir;
>
> @@ -187,8 +188,12 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)
> namlen = slash - dir;
> restlen -= namlen;
> loginfo("userpath <%s>, request <%s>, namlen %d, restlen %d, slash <%s>", user_path, dir, namlen, restlen, slash);
> - snprintf(rpath, PATH_MAX, "%.*s/%s%.*s",
> - namlen, dir, user_path, restlen, slash);
> + rlen = snprintf(rpath, sizeof(rpath), "%.*s/%s%.*s",
> + namlen, dir, user_path, restlen, slash);
> + if (rlen >= sizeof(rpath)) {
> + logerror("user-path too large: %s", rpath);
> + return NULL;
> + }
> dir = rpath;
> }
> }
> @@ -207,7 +212,15 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)
>
> strbuf_expand(&expanded_path, interpolated_path,
> expand_path, &context);
> - strlcpy(interp_path, expanded_path.buf, PATH_MAX);
> +
> + rlen = strlcpy(interp_path, expanded_path.buf,
> + sizeof(interp_path));
> + if (rlen >= sizeof(interp_path)) {
> + logerror("interpolated path too large: %s",
> + interp_path);
> + return NULL;
> + }
> +
> strbuf_release(&expanded_path);
> loginfo("Interpolated dir '%s'", interp_path);
>
> @@ -219,7 +232,11 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)
> logerror("'%s': Non-absolute path denied (base-path active)", dir);
> return NULL;
> }
> - snprintf(rpath, PATH_MAX, "%s%s", base_path, dir);
> + rlen = snprintf(rpath, sizeof(rpath), "%s%s", base_path, dir);
> + if (rlen >= sizeof(rpath)) {
> + logerror("base-path too large: %s", rpath);
> + return NULL;
> + }
> dir = rpath;
> }
^ permalink raw reply
* Re: [PATCH 3/3] transport: allow summary-width to be computed dynamically
From: Junio C Hamano @ 2016-10-22 16:25 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20161022050426.t2fifjqrldc6mebc@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Fri, Oct 21, 2016 at 09:39:45PM -0700, Junio C Hamano wrote:
>
>> And this is the final one.
>>
>> -- >8 --
>> From: Junio C Hamano <gitster@pobox.com>
>> Date: Fri, 21 Oct 2016 21:33:06 -0700
>> Subject: [PATCH] transport: compute summary-width dynamically
>>
>> Now all that is left to do is to actually iterate over the refs
>> and measure the display width needed to show their abbreviation.
>
> I think we crossed emails. :) This is obviously correct, if we don't
> mind paying the find_unique_abbrev cost twice for each sha1.
Indeed we did. I do not think the cost matters that much in the
codepath to produce the final summary output.
> This is a minor style nit, but I think it's better to avoid mixing
> unrelated bits between the initialization, condition, and iteration bits
> of a for loop.
Yeah, you're right.
^ permalink raw reply
* Re: [PATCH v4 5/8] trailer: clarify failure modes in parse_trailer
From: Junio C Hamano @ 2016-10-22 16:19 UTC (permalink / raw)
To: Christian Couder
Cc: Jonathan Tan, Stefan Beller, git@vger.kernel.org, Ramsay Jones
In-Reply-To: <CAP8UFD0XvNx6Brio9muDhOj5+dr=HRbhK_Lmr+k4LUPKrAVC9Q@mail.gmail.com>
Christian Couder <christian.couder@gmail.com> writes:
> On Fri, Oct 21, 2016 at 2:18 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> If I were guiding a topic that introduce this feature from scratch
>> today, I would probably suggest a pattern based approach, e.g. a
>> built-in "[-A-Za-z0-9]+:" [*1*] may be the default prefix that is
>> used to recognize the beginning of a trailer, and a user or a
>> project that wants "Bug #538" would be allowed to add an additional
>> pattern, e.g. "Bug *#", that recognises a custom trailer line that
>> is used by the project.
>
> When we designed the separator mechanism, we had the following discussions:
>
> https://public-inbox.org/git/xmqqa9a1d6xn.fsf@gitster.dls.corp.google.com/
> https://public-inbox.org/git/xmqqmwcuzyqx.fsf@gitster.dls.corp.google.com/
>
> They made me think that you were against too much flexibility, so I
> removed functionality that allowed to put separators into the ".key"
> config options, and now you are saying that we botched the thing and
> that you would like more flexibility of this kind back.
Correct. Pay attention to the fact that I said _we_ botched.
If an initial design made by a topic author is crappy, that may be
author's botch. Once a topic goes through a review cycle by getting
reviewed, rerolled, re-reviewed, ... to the point that those
involved accept the result, and we later realize that it was not
good, the botch no longer is author's alone. If it is shipped as
part of a release, then it is not just the authors and the reviewers
but everybody. We collectively stopped at a place that was not
ideal and share the blame ;-).
> Anyway I think it is still possible to add back such kind of
> functionality in a backward compatible way for example by adding
> ".extendedKey" config options.
Yup, or with trailer.keyPattern that is multi-values, or with any
number of alternatives.
^ permalink raw reply
* Re: [PATCH v4 5/8] trailer: clarify failure modes in parse_trailer
From: Christian Couder @ 2016-10-22 13:07 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jonathan Tan, Stefan Beller, git@vger.kernel.org, Ramsay Jones
In-Reply-To: <xmqqeg3aeeqe.fsf@gitster.mtv.corp.google.com>
On Fri, Oct 21, 2016 at 2:18 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Jonathan Tan <jonathantanmy@google.com> writes:
>
>> That is true - I think we can take the allowed separators as an
>> argument (meaning that we can have different behavior for file parsing
>> and command line parsing), and since we already have that string, we
>> can use strcspn. I'll try this out in the next reroll.
>
> Sounds good. Thanks.
>
>
> The following is a tangent that I think this topic should ignore,
> but we may want to revisit it sometime later.
>
> I think the design of the "separator" mechanism is one of the things
> we botched in the current system. If I recall correctly, this was
> introduced to allow people write "Bug# 538" in the trailer section
> and get it recognised as a valid trailer.
>
> When I say that this was a botched design, I do not mean to say that
> we should have instead forced projects to adopt "Bug: 538" format.
> The design is botched because the users' wish to allow "Bug# 538" or
> "Bug #538" by setting separators to ":#" from the built-in ":" does
> not mean that they would want "Signed-off-by# me <my@addre.ss>" to
> be accepted.
>
> If I were guiding a topic that introduce this feature from scratch
> today, I would probably suggest a pattern based approach, e.g. a
> built-in "[-A-Za-z0-9]+:" [*1*] may be the default prefix that is
> used to recognize the beginning of a trailer, and a user or a
> project that wants "Bug #538" would be allowed to add an additional
> pattern, e.g. "Bug *#", that recognises a custom trailer line that
> is used by the project.
When we designed the separator mechanism, we had the following discussions:
https://public-inbox.org/git/xmqqa9a1d6xn.fsf@gitster.dls.corp.google.com/
https://public-inbox.org/git/xmqqmwcuzyqx.fsf@gitster.dls.corp.google.com/
They made me think that you were against too much flexibility, so I
removed functionality that allowed to put separators into the ".key"
config options, and now you are saying that we botched the thing and
that you would like more flexibility of this kind back.
Anyway I think it is still possible to add back such kind of
functionality in a backward compatible way for example by adding
".extendedKey" config options.
^ permalink raw reply
* Re: [PATCH v4 5/8] trailer: clarify failure modes in parse_trailer
From: Christian Couder @ 2016-10-22 9:29 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jonathan Tan, Stefan Beller, git@vger.kernel.org, Ramsay Jones
In-Reply-To: <xmqqr37aej26.fsf@gitster.mtv.corp.google.com>
On Fri, Oct 21, 2016 at 12:45 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Jonathan Tan <jonathantanmy@google.com> writes:
>
>> If we do that, there is also the necessity of creating a string that
>> combines the separators and '=' (I guess '\n' is not necessary now,
>> since all the lines are null terminated). I'm OK either way.
>>
>> (We could cache that string, although I would think that if we did
>> that, we might as well write the loop manually, like in this patch.)
>
> I wonder if there is a legit reason to look for '=' in the first
> place. "Signed-off-by= Jonathan Tan <jt@my.home>" does not look
> like a valid trailer line to me.
>
> Isn't that a remnant of lazy coding in the original that tried to
> share a single parser for contents and command line options or
> something?
I think the relevant discussion was this one:
https://public-inbox.org/git/20140915.080429.1739849931027469667.chriscool@tuxfamily.org/
^ permalink raw reply
* Re: tools for easily "uncommitting" parts of a patch I just commited?
From: Lukas Fleischer @ 2016-10-22 9:19 UTC (permalink / raw)
To: git; +Cc: Jeff King, Git mailing list
In-Reply-To: <CA+P7+xq25LcdmtzmBNChiGhGratcdp7m0EOsQuEh68=gJQ9HNQ@mail.gmail.com>
On Thu, 20 Oct 2016 at 19:27:58, Jacob Keller wrote:
> [...]
> I still think we're misunderstanding. I want git commit to complain
> *only* under the following circumstance:
>
> I run "git add -p" and put a partial change into the index in <file>.
> There are still other parts which were not added to the index yet.
> Thus, the index version of the file and the actual file differ.
>
> Then, I (accidentally) run "git commit <file>"
> [...]
This reminded me of something that bothered me for a while. It's not
100% on-topic but still quite related so I thought I'd bring it up.
When working on a feature, I usually try to make atomic changes from the
beginning and use `git commit -a` to commit them one after another. This
works fine most of the time. Sometimes I notice only after making some
changes that it might be better to split the working tree changes into
several commits.
In that case, I git-add the relevant hunks and then, unfortunately, I
often run `git commit -a` instead of `git commit` (muscle memory bites
me), so I need to do all the splitting work again.
It's not much of an issue but would it be worthwhile to add an optional
feature (configurable) that warns you when using --all with staged
changes (which are not new files)? Are there others having the same
issue? Do you think this should be implemented as part of an alias
instead?
Regards,
Lukas
^ permalink raw reply
* Re: [PATCH 2/3] submodule tests: replace cloning from . by "$(pwd)"
From: Junio C Hamano @ 2016-10-22 7:33 UTC (permalink / raw)
To: Johannes Sixt
Cc: Stefan Beller, Johannes.Schindelin, git, venv21, dennis, jrnieder
In-Reply-To: <c6c5ce05-3511-a992-e079-316f0ce90ecd@kdbg.org>
Johannes Sixt <j6t@kdbg.org> writes:
>> The logic to construct the relative urls is not smart enough to
>> detect that the ending /. is referring to the directory itself
>> but rather treats it like any other relative path, i.e.
>>
>> path/to/dir/. + ../relative/path/to/submodule
>>
>> would result in
>>
>> path/to/dir/relative/path/to/submodule
>>
>> and not omit the "dir" as you may expect.
>>
>> As in a later patch we'll normalize the remote url before the
>> computation of relative urls takes place, we need to first get our
>> test suite in a shape with normalized urls only, which is why we should
>> refrain from cloning from '.'
>
> But you are removing a valid use case from the tests. Aren't you
> sweeping something under the rug with this patch?
I share the same reaction.
If the primary problem being solved is that the combination of a
relative URL ../sub and the base URL for the superproject which is
set to /path/to/dir/. (due to "clone .") were incorrectly resolved
as /path/to/dir/sub (because the buggy relative path logic did not
know that removing "/." at the end does not take you to one level
up), and a topic that fixes the bug would make that relative URL
../sub to be resolved as /path/to/sub, of course. Otherwise, the
topic did not fix the bug.
Now if a test that wanted to have a clone of the superproject by
"clone .", which results in the base URL of /path/to/dir/., actually
wants to refer in its .gitmodules to /path/to/dir/sub (which after
all was where the submodule the test created with or without the
bugfix), I would think the right adjustment for the test that used
to rely on the buggy behaviour would be to stop using ../sub and
instead use ./sub as the relative URL, no? After all, the bug forced
the original test writer to write ../sub but the submodule in this
case actually is at ./sub relative to its superproject, and that is
what the original test writer would have written if the bug weren't
there in the first place, no?
Another thing I do not quite understand is why this step comes
before the fix. If the "clone ." is adjusted to avoid triggering
the buggy behaviour, i.e. making the base URL to /path/to/dir
(instead of /path/to/dir/.), wouldn't the relative URL ../sub that
was written to work around the bug that hasn't been fixed yet in
this step need to be adjusted anyway? It would end up referring to
/path/to/sub and not /path/to/dir/sub until the fix is put in place.
Is the removal of remote.origin.url a wrong workaround for that
breakage, I wonder... I do not understand that change at all, and I
do not think it was explained in the log message.
If we really wanted to update the test before fixing the bug, I
would understand if this step changed ../sub (or whatever relative
URL that has extra ../ only because the base URL has extra /. at the
end to compensate for the buggy resolution) to ./sub in the tests
and marked them to expect failure, and then a later step that fixes
the bug turns them to expect success without make any other change.
^ permalink raw reply
* Re: [PATCH 2/3] submodule tests: replace cloning from . by "$(pwd)"
From: Johannes Sixt @ 2016-10-22 7:09 UTC (permalink / raw)
To: Stefan Beller; +Cc: gitster, Johannes.Schindelin, git, venv21, dennis, jrnieder
In-Reply-To: <20161021235939.20792-3-sbeller@google.com>
Am 22.10.2016 um 01:59 schrieb Stefan Beller:
> When adding a submodule via "git submodule add <relative url>",
> the relative url applies to the superprojects remote. When the
> superproject was cloned via "git clone . super", the remote url
> is ending with '/.'.
>
> The logic to construct the relative urls is not smart enough to
> detect that the ending /. is referring to the directory itself
> but rather treats it like any other relative path, i.e.
>
> path/to/dir/. + ../relative/path/to/submodule
>
> would result in
>
> path/to/dir/relative/path/to/submodule
>
> and not omit the "dir" as you may expect.
>
> As in a later patch we'll normalize the remote url before the
> computation of relative urls takes place, we need to first get our
> test suite in a shape with normalized urls only, which is why we should
> refrain from cloning from '.'
But you are removing a valid use case from the tests. Aren't you
sweeping something under the rug with this patch?
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> t/t7064-wtstatus-pv2.sh | 9 ++++++---
> t/t7403-submodule-sync.sh | 3 ++-
> t/t7406-submodule-update.sh | 6 ++++--
> t/t7407-submodule-foreach.sh | 3 ++-
> t/t7506-status-submodule.sh | 3 ++-
> 5 files changed, 16 insertions(+), 8 deletions(-)
>
> diff --git a/t/t7064-wtstatus-pv2.sh b/t/t7064-wtstatus-pv2.sh
> index 3012a4d..95514bb 100755
> --- a/t/t7064-wtstatus-pv2.sh
> +++ b/t/t7064-wtstatus-pv2.sh
> @@ -330,7 +330,8 @@ test_expect_success 'verify UU (edit-edit) conflict' '
> test_expect_success 'verify upstream fields in branch header' '
> git checkout master &&
> test_when_finished "rm -rf sub_repo" &&
> - git clone . sub_repo &&
> + git clone "$(pwd)" sub_repo &&
> + git -C sub_repo config --unset remote.origin.url &&
Why is it necessary to remove this configuration? Is it because when it
is present, the submodule does not construct its own reference? And if
so, should it not be sufficient to only remove the configuration
(without changing 'git clone' command), but move this patch after the
patch that fixes the /. treatment?
> (
> ## Confirm local master tracks remote master.
> cd sub_repo &&
...
^ permalink raw reply
* Re: [PATCH] doc: fix merge-base ASCII art tab spacing
From: Philip Oakley @ 2016-10-22 5:45 UTC (permalink / raw)
To: Junio C Hamano; +Cc: GitList, Jeff King, Johannes Schindelin
In-Reply-To: <xmqq1sz9b9ex.fsf@gitster.mtv.corp.google.com>
From: "Junio C Hamano" <gitster@pobox.com>
> "Philip Oakley" <philipoakley@iee.org> writes:
>
>> It appears that acciidoctor sees the art as being a separated
>> mono-spaced block, with border/background as locally
>> appropriate. While the asciidoc looks to simply change to mono-spaced
>> text.
>
> FWIW, I changed my mind and your patch is now queued on 'next'.
>
> Thanks.
>
Many thanks
Philip
^ permalink raw reply
* Re: [PATCH] daemon: detect and reject too-long paths
From: Jeff King @ 2016-10-22 5:26 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <20161022045938.h3xa3yapzlg427vy@sigill.intra.peff.net>
On Sat, Oct 22, 2016 at 12:59:38AM -0400, Jeff King wrote:
> When we are checking the path via path_ok(), we use some
> fixed PATH_MAX buffers. We write into them via snprintf(),
> so there's no possibility of overflow, but it does mean we
> may silently truncate the path, leading to potentially
> confusing errors when the partial path does not exist.
>
> We're better off to reject the path explicitly.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> Another option would be to switch to strbufs here. That potentially
> introduces cases where a client can convince us to just keep allocating
> memory, but I don't think so in practice; the paths and interpolated
> data items all have to come in 64K pkt-lines, which places a hard
> limit. This is a much more minimal change, though, and I don't hear
> anybody complaining about the inability to use large paths.
For reference, the switch to dynamic memory looks something like this.
We don't even need strbufs, and we can get rid of the static variables
entirely (they weren't about buffer reuse, but just about extending the
lifetime past the return value).
Though we do have to add some free()s to avoid leaking error cases, this
looks simpler to me (the return value _is_ leaked in the success case,
because the caller doesn't know if we returned the original value or a
newly allocated one. In practice it doesn't matter because we call this
function once per process; compare to the 8K of BSS being wasted in the
original).
diff --git a/daemon.c b/daemon.c
index 425aad0507..4575ce5 100644
--- a/daemon.c
+++ b/daemon.c
@@ -158,8 +158,7 @@ static size_t expand_path(struct strbuf *sb, const char *placeholder, void *ctx)
static const char *path_ok(const char *directory, struct hostinfo *hi)
{
- static char rpath[PATH_MAX];
- static char interp_path[PATH_MAX];
+ char *to_free = NULL;
const char *path;
const char *dir;
@@ -187,9 +186,9 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)
namlen = slash - dir;
restlen -= namlen;
loginfo("userpath <%s>, request <%s>, namlen %d, restlen %d, slash <%s>", user_path, dir, namlen, restlen, slash);
- snprintf(rpath, PATH_MAX, "%.*s/%s%.*s",
- namlen, dir, user_path, restlen, slash);
- dir = rpath;
+ dir = to_free = xstrfmt("%.*s/%s%.*s",
+ namlen, dir, user_path,
+ restlen, slash);
}
}
else if (interpolated_path && hi->saw_extended_args) {
@@ -207,11 +206,8 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)
strbuf_expand(&expanded_path, interpolated_path,
expand_path, &context);
- strlcpy(interp_path, expanded_path.buf, PATH_MAX);
- strbuf_release(&expanded_path);
- loginfo("Interpolated dir '%s'", interp_path);
-
- dir = interp_path;
+ dir = to_free = strbuf_detach(&expanded_path, NULL);
+ loginfo("Interpolated dir '%s'", dir);
}
else if (base_path) {
if (*dir != '/') {
@@ -219,8 +215,7 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)
logerror("'%s': Non-absolute path denied (base-path active)", dir);
return NULL;
}
- snprintf(rpath, PATH_MAX, "%s%s", base_path, dir);
- dir = rpath;
+ dir = to_free = xstrfmt("%s%s", base_path, dir);
}
path = enter_repo(dir, strict_paths);
@@ -229,12 +224,15 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)
* if we fail and base_path_relaxed is enabled, try without
* prefixing the base path
*/
+ free(to_free);
+ to_free = NULL;
dir = directory;
path = enter_repo(dir, strict_paths);
}
if (!path) {
logerror("'%s' does not appear to be a git repository", dir);
+ free(to_free);
return NULL;
}
@@ -265,6 +263,7 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)
}
logerror("'%s': not in whitelist", path);
+ free(to_free);
return NULL; /* Fallthrough. Deny by default */
}
^ permalink raw reply related
* Re: [PATCH 4/3] test-lib: bail out when "-v" used under "prove"
From: Jacob Keller @ 2016-10-22 5:25 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Stefan Beller, Lars Schneider, git
In-Reply-To: <20161022044506.vba6g2q25yxa2air@sigill.intra.peff.net>
On Fri, Oct 21, 2016 at 9:45 PM, Jeff King <peff@peff.net> wrote:
> I thought I'd just knock this out in 5 minutes before I forgot about it.
> But as with so many things, getting it right proved slightly harder than
> I thought.
Always seems to be that way, doesn't it?
> But I did learn about TAP's "Bail out!" directive. And
> apparently you can pass it back arbitrary YAML (!). And the "--verbose"
> output really is violating the spec, and they claim that Test::Harness
> will eventually be tightened to complain (though that was in 2007, and
> it still hasn't happened, so...).
>
> Anyway. Here is the patch I came up with (on top of the others).
>
Nice.
> -- >8 --
> Subject: test-lib: bail out when "-v" used under "prove"
>
> When there is a TAP harness consuming the output of our test
> scripts, the "--verbose" breaks the output by mingling
> test command output with TAP. Because the TAP::Harness
> module used by "prove" is fairly lenient, this _usually_
> works, but it violates the spec, and things get very
> confusing if the commands happen to output a line that looks
> like TAP (e.g., the word "ok" on its own line).
>
> Let's detect this situation and complain. Just calling
> error() isn't great, though; prove will tell us that the
> script failed, but the message doesn't make it through to
> the user. Instead, we can use the special TAP signal "Bail
> out!". This not only shows the message to the user, but
> instructs the harness to stop running the tests entirely.
> This is exactly what we want here, as the problem is in the
> command-line options, and every test script would produce
> the same error.
>
> The result looks like this (the first "Bailout called" line
> is in red if prove uses color on your terminal):
>
> $ make GIT_TEST_OPTS='--verbose --tee'
> rm -f -r 'test-results'
> *** prove ***
> Bailout called. Further testing stopped: verbose mode forbidden under TAP harness; try --verbose-log
> FAILED--Further testing stopped: verbose mode forbidden under TAP harness; try --verbose-log
> Makefile:39: recipe for target 'prove' failed
> make: *** [prove] Error 255
>
Nice that makes sense.
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> t/test-lib.sh | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 85946ec40d..b859db61ac 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -321,6 +321,16 @@ say () {
> say_color info "$*"
> }
>
> +if test -n "$HARNESS_ACTIVE"
> +then
> + if test "$verbose" = t || test -n "$verbose_only"
> + then
> + printf 'Bail out! %s\n' \
> + 'verbose mode forbidden under TAP harness; try --verbose-log'
> + exit 1
> + fi
> +fi
> +
Not too much code, so that's good. I like it.
Thanks,
Jake
> test "${test_description}" != "" ||
> error "Test script did not set test_description."
>
> --
> 2.10.1.776.ge0e381e
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox