* Re: [PATCH v2 1/5] trailer: be stricter in parsing separators
From: Junio C Hamano @ 2016-11-01 20:32 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, christian.couder
In-Reply-To: <c7db0aafb543845382e1835e3704273d3596e6bb.1478028700.git.jonathantanmy@google.com>
Jonathan Tan <jonathantanmy@google.com> writes:
> Currently, a line is interpreted to be a trailer line if it contains a
> separator. Make parsing stricter by requiring the text on the left of
> the separator, if not the empty string, to be of the "<token><optional
> whitespace>" form.
Hmph. The optional whitespace is to allow for what kind of line?
It is not for "Signed off by:" that is a misspelt "Signed-off-by:";
it may not hurt but I do not think of a case that would be useful
offhand.
> (The find_separator function distinguishes the no-separator case from
> the separator-starts-line case because some callers of this function
> need such a distinction.)
>
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
> trailer.c | 23 +++++++++++++++++------
> 1 file changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/trailer.c b/trailer.c
> index f0ecde2..0ee634f 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -563,15 +563,26 @@ static int token_matches_item(const char *tok, struct arg_item *item, int tok_le
> }
>
> /*
> - * Return the location of the first separator in line, or -1 if there is no
> - * separator.
> + * If the given line is of the form
> + * "<token><optional whitespace><separator>..." or "<separator>...", return the
> + * location of the separator. Otherwise, return -1.
> */
> static int find_separator(const char *line, const char *separators)
> {
> - int loc = strcspn(line, separators);
> - if (!line[loc])
> - return -1;
> - return loc;
> + int whitespace_found = 0;
> + const char *c;
> + for (c = line; *c; c++) {
> + if (strchr(separators, *c))
> + return c - line;
> + if (!whitespace_found && (isalnum(*c) || *c == '-'))
> + continue;
> + if (c != line && (*c == ' ' || *c == '\t')) {
> + whitespace_found = 1;
> + continue;
> + }
> + break;
> + }
> + return -1;
> }
>
> /*
^ permalink raw reply
* Re: [PATCH v2 4/6] grep: optionally recurse into submodules
From: Brandon Williams @ 2016-11-01 20:25 UTC (permalink / raw)
To: Stefan Beller; +Cc: git@vger.kernel.org
In-Reply-To: <CAGZ79kaPSCJo4jBh5pha6_u4pe-7zXoYQi3bD1L14nwUmdD-Hg@mail.gmail.com>
On 11/01, Stefan Beller wrote:
> On Mon, Oct 31, 2016 at 3:38 PM, Brandon Williams <bmwill@google.com> wrote:
>
> >
> > +--recurse-submodules::
> > + Recursively search in each submodule that has been initialized and
> > + checked out in the repository.
> > +
>
> and warn otherwise.
I've been going back and forth on whether to warn the user...maybe
`grep` isn't really the right place for the warning?
> > +
> > + /*
> > + * Capture output to output buffer and check the return code from the
> > + * child process. A '0' indicates a hit, a '1' indicates no hit and
> > + * anything else is an error.
> > + */
> > + status = capture_command(&cp, &w->out, 0);
> > + if (status && (status != 1))
>
> Does the user have enough information what went wrong?
> Is the child verbose enough, such that we do not need to give a
> die[_errno]("submodule processs failed") ?
good point...the output from the child is stored in a buffer and won't
actually get printed if this fails out. Perhaps we should flush the
buffer and then die?
> > + if (S_ISREG(ce->ce_mode) &&
> > + match_pathspec(pathspec, name.buf, name.len, 0, NULL,
> > + S_ISDIR(ce->ce_mode) ||
> > + S_ISGITLINK(ce->ce_mode))) {
>
> Why do we have to pass the ISDIR and ISGITLINK here for the regular file
> case? ce_path_match and match_pathspec are doing the same thing?
I was simply doing what ce_path_match was doing. And I needed to switch
to using match_pathspec instead because ce_path_match doesn't allow for
checking the super_prefix as part of the pathspec logic...Perhaps a
refactor (in the future) in the pathspec logic could do that via a flag?
> > + submodule_path_match(pathspec, name.buf, NULL)) {
> > + hit |= grep_submodule(opt, NULL, ce->name, ce->name);
>
> What is the difference between the last two parameters?
Path and file name, in the cached case they are the same.
> > + * filename: name of the submodule including tree name of parent
> > + * path: location of the submodule
>
> That sounds the same to me.
So they are similar. path should be used as the directory to
chdir for the child process and it doesn't have the tree name prefixed
to it.
--
Brandon Williams
^ permalink raw reply
* [PATCH v2 4/5] trailer: have function to describe trailer layout
From: Jonathan Tan @ 2016-11-01 20:08 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, gitster, christian.couder
In-Reply-To: <cover.1478028700.git.jonathantanmy@google.com>
Create a function that, taking a string, describes the position of its
trailer block (if available) and the contents thereof, and make trailer
use it. This makes it easier for other Git components, in the future, to
interpret trailer blocks in the same way as trailer.
In a subsequent patch, another component will be made to use this.
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
trailer.c | 118 +++++++++++++++++++++++++++++++++++++++++++-------------------
trailer.h | 25 +++++++++++++
2 files changed, 107 insertions(+), 36 deletions(-)
diff --git a/trailer.c b/trailer.c
index f5427ec..7265a50 100644
--- a/trailer.c
+++ b/trailer.c
@@ -46,6 +46,8 @@ static LIST_HEAD(conf_head);
static char *separators = ":";
+static int configured;
+
#define TRAILER_ARG_STRING "$ARG"
static const char *git_generated_prefixes[] = {
@@ -546,6 +548,17 @@ static int git_trailer_config(const char *conf_key, const char *value, void *cb)
return 0;
}
+static void ensure_configured(void)
+{
+ if (configured)
+ return;
+
+ /* Default config must be setup first */
+ git_config(git_trailer_default_config, NULL);
+ git_config(git_trailer_config, NULL);
+ configured = 1;
+}
+
static const char *token_from_item(struct arg_item *item, char *tok)
{
if (item->conf.key)
@@ -870,59 +883,43 @@ static int process_input_file(FILE *outfile,
const char *str,
struct list_head *head)
{
- int patch_start, trailer_start, trailer_end;
+ struct trailer_info info;
struct strbuf tok = STRBUF_INIT;
struct strbuf val = STRBUF_INIT;
- struct trailer_item *last = NULL;
- struct strbuf *trailer, **trailer_lines, **ptr;
+ int i;
- patch_start = find_patch_start(str);
- trailer_end = find_trailer_end(str, patch_start);
- trailer_start = find_trailer_start(str, trailer_end);
+ trailer_info_get(&info, str);
/* Print lines before the trailers as is */
- fwrite(str, 1, trailer_start, outfile);
+ fwrite(str, 1, info.trailer_start - str, outfile);
- if (!ends_with_blank_line(str, trailer_start))
+ if (!info.blank_line_before_trailer)
fprintf(outfile, "\n");
- /* Parse trailer lines */
- trailer_lines = strbuf_split_buf(str + trailer_start,
- trailer_end - trailer_start,
- '\n',
- 0);
- for (ptr = trailer_lines; *ptr; ptr++) {
+ for (i = 0; i < info.trailer_nr; i++) {
int separator_pos;
- trailer = *ptr;
- if (trailer->buf[0] == comment_line_char)
- continue;
- if (last && isspace(trailer->buf[0])) {
- struct strbuf sb = STRBUF_INIT;
- strbuf_addf(&sb, "%s\n%s", last->value, trailer->buf);
- strbuf_strip_suffix(&sb, "\n");
- free(last->value);
- last->value = strbuf_detach(&sb, NULL);
+ char *trailer = info.trailers[i];
+ if (trailer[0] == comment_line_char)
continue;
- }
- separator_pos = find_separator(trailer->buf, separators);
+ separator_pos = find_separator(trailer, separators);
if (separator_pos >= 1) {
- parse_trailer(&tok, &val, NULL, trailer->buf,
+ parse_trailer(&tok, &val, NULL, trailer,
separator_pos);
- last = add_trailer_item(head,
- strbuf_detach(&tok, NULL),
- strbuf_detach(&val, NULL));
+ add_trailer_item(head,
+ strbuf_detach(&tok, NULL),
+ strbuf_detach(&val, NULL));
} else {
- strbuf_addbuf(&val, trailer);
+ strbuf_addstr(&val, trailer);
strbuf_strip_suffix(&val, "\n");
add_trailer_item(head,
NULL,
strbuf_detach(&val, NULL));
- last = NULL;
}
}
- strbuf_list_free(trailer_lines);
- return trailer_end;
+ trailer_info_release(&info);
+
+ return info.trailer_end - str;
}
static void free_all(struct list_head *head)
@@ -973,9 +970,7 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct str
int trailer_end;
FILE *outfile = stdout;
- /* Default config must be setup first */
- git_config(git_trailer_default_config, NULL);
- git_config(git_trailer_config, NULL);
+ ensure_configured();
read_input_file(&sb, file);
@@ -1002,3 +997,54 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct str
strbuf_release(&sb);
}
+
+void trailer_info_get(struct trailer_info *info, const char *str)
+{
+ int patch_start, trailer_end, trailer_start;
+ struct strbuf **trailer_lines, **ptr;
+ char **trailer_strings = NULL;
+ size_t nr = 0, alloc = 0;
+ char **last = NULL;
+
+ ensure_configured();
+
+ patch_start = find_patch_start(str);
+ trailer_end = find_trailer_end(str, patch_start);
+ trailer_start = find_trailer_start(str, trailer_end);
+
+ trailer_lines = strbuf_split_buf(str + trailer_start,
+ trailer_end - trailer_start,
+ '\n',
+ 0);
+ for (ptr = trailer_lines; *ptr; ptr++) {
+ if (last && isspace((*ptr)->buf[0])) {
+ struct strbuf sb = STRBUF_INIT;
+ strbuf_attach(&sb, *last, strlen(*last), strlen(*last));
+ strbuf_addbuf(&sb, *ptr);
+ *last = strbuf_detach(&sb, NULL);
+ continue;
+ }
+ ALLOC_GROW(trailer_strings, nr + 1, alloc);
+ trailer_strings[nr] = strbuf_detach(*ptr, NULL);
+ last = find_separator(trailer_strings[nr], separators) >= 1
+ ? &trailer_strings[nr]
+ : NULL;
+ nr++;
+ }
+ strbuf_list_free(trailer_lines);
+
+ info->blank_line_before_trailer = ends_with_blank_line(str,
+ trailer_start);
+ info->trailer_start = str + trailer_start;
+ info->trailer_end = str + trailer_end;
+ info->trailers = trailer_strings;
+ info->trailer_nr = nr;
+}
+
+void trailer_info_release(struct trailer_info *info)
+{
+ int i;
+ for (i = 0; i < info->trailer_nr; i++)
+ free(info->trailers[i]);
+ free(info->trailers);
+}
diff --git a/trailer.h b/trailer.h
index 36b40b8..65cc5d7 100644
--- a/trailer.h
+++ b/trailer.h
@@ -1,7 +1,32 @@
#ifndef TRAILER_H
#define TRAILER_H
+struct trailer_info {
+ /*
+ * True if there is a blank line before the location pointed to by
+ * trailer_start.
+ */
+ int blank_line_before_trailer;
+
+ /*
+ * Pointers to the start and end of the trailer block found. If there
+ * is no trailer block found, these 2 pointers point to the end of the
+ * input string.
+ */
+ const char *trailer_start, *trailer_end;
+
+ /*
+ * Array of trailers found.
+ */
+ char **trailers;
+ size_t trailer_nr;
+};
+
void process_trailers(const char *file, int in_place, int trim_empty,
struct string_list *trailers);
+void trailer_info_get(struct trailer_info *info, const char *str);
+
+void trailer_info_release(struct trailer_info *info);
+
#endif /* TRAILER_H */
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH v2 5/5] sequencer: use trailer's trailer layout
From: Jonathan Tan @ 2016-11-01 20:08 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, gitster, christian.couder
In-Reply-To: <cover.1478028700.git.jonathantanmy@google.com>
Make sequencer use trailer.c's trailer layout definition, as opposed to
parsing the footer by itself. This makes "commit -s", "cherry-pick -x",
and "format-patch --signoff" consistent with trailer, allowing
non-trailer lines and multiple-line trailers in trailer blocks under
certain conditions, and therefore suppressing the extra newline in those
cases.
Consistency with trailer extends to respecting trailer configs. Tests
have been included to show that.
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
sequencer.c | 75 +++++++++---------------------------------------
t/t3511-cherry-pick-x.sh | 16 +++++++++--
t/t4014-format-patch.sh | 37 ++++++++++++++++++++----
t/t7501-commit.sh | 36 +++++++++++++++++++++++
4 files changed, 95 insertions(+), 69 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 5fd75f3..d64c973 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -16,6 +16,7 @@
#include "refs.h"
#include "argv-array.h"
#include "quote.h"
+#include "trailer.h"
#define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
@@ -56,30 +57,6 @@ static const char *get_todo_path(const struct replay_opts *opts)
return git_path_todo_file();
}
-static int is_rfc2822_line(const char *buf, int len)
-{
- int i;
-
- for (i = 0; i < len; i++) {
- int ch = buf[i];
- if (ch == ':')
- return 1;
- if (!isalnum(ch) && ch != '-')
- break;
- }
-
- return 0;
-}
-
-static int is_cherry_picked_from_line(const char *buf, int len)
-{
- /*
- * We only care that it looks roughly like (cherry picked from ...)
- */
- return len > strlen(cherry_picked_prefix) + 1 &&
- starts_with(buf, cherry_picked_prefix) && buf[len - 1] == ')';
-}
-
/*
* Returns 0 for non-conforming footer
* Returns 1 for conforming footer
@@ -89,49 +66,25 @@ static int is_cherry_picked_from_line(const char *buf, int len)
static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob,
int ignore_footer)
{
- char prev;
- int i, k;
- int len = sb->len - ignore_footer;
- const char *buf = sb->buf;
- int found_sob = 0;
-
- /* footer must end with newline */
- if (!len || buf[len - 1] != '\n')
- return 0;
+ struct trailer_info info;
+ int i;
+ int found_sob = 0, found_sob_last = 0;
- prev = '\0';
- for (i = len - 1; i > 0; i--) {
- char ch = buf[i];
- if (prev == '\n' && ch == '\n') /* paragraph break */
- break;
- prev = ch;
- }
+ trailer_info_get(&info, sb->buf);
- /* require at least one blank line */
- if (prev != '\n' || buf[i] != '\n')
+ if (info.trailer_start == info.trailer_end)
return 0;
- /* advance to start of last paragraph */
- while (i < len - 1 && buf[i] == '\n')
- i++;
-
- for (; i < len; i = k) {
- int found_rfc2822;
-
- for (k = i; k < len && buf[k] != '\n'; k++)
- ; /* do nothing */
- k++;
+ for (i = 0; i < info.trailer_nr; i++)
+ if (sob && !strncmp(info.trailers[i], sob->buf, sob->len)) {
+ found_sob = 1;
+ if (i == info.trailer_nr - 1)
+ found_sob_last = 1;
+ }
- found_rfc2822 = is_rfc2822_line(buf + i, k - i - 1);
- if (found_rfc2822 && sob &&
- !strncmp(buf + i, sob->buf, sob->len))
- found_sob = k;
+ trailer_info_release(&info);
- if (!(found_rfc2822 ||
- is_cherry_picked_from_line(buf + i, k - i - 1)))
- return 0;
- }
- if (found_sob == i)
+ if (found_sob_last)
return 3;
if (found_sob)
return 2;
diff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh
index 9cce5ae..bf0a5c9 100755
--- a/t/t3511-cherry-pick-x.sh
+++ b/t/t3511-cherry-pick-x.sh
@@ -25,9 +25,8 @@ Signed-off-by: B.U. Thor <buthor@example.com>"
mesg_broken_footer="$mesg_no_footer
-The signed-off-by string should begin with the words Signed-off-by followed
-by a colon and space, and then the signers name and email address. e.g.
-Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>"
+This is not recognized as a footer because Myfooter is not a recognized token.
+Myfooter: A.U. Thor <author@example.com>"
mesg_with_footer_sob="$mesg_with_footer
Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>"
@@ -112,6 +111,17 @@ test_expect_success 'cherry-pick -s inserts blank line after non-conforming foot
test_cmp expect actual
'
+test_expect_success 'cherry-pick -s recognizes trailer config' '
+ pristine_detach initial &&
+ git -c "trailer.Myfooter.ifexists=add" cherry-pick -s mesg-broken-footer &&
+ cat <<-EOF >expect &&
+ $mesg_broken_footer
+ Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
+ EOF
+ git log -1 --pretty=format:%B >actual &&
+ test_cmp expect actual
+'
+
test_expect_success 'cherry-pick -x inserts blank line when conforming footer not found' '
pristine_detach initial &&
sha1=$(git rev-parse mesg-no-footer^0) &&
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index ba4902d..482112c 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -1294,8 +1294,7 @@ EOF
4:Subject: [PATCH] subject
8:
10:Signed-off-by: example happens to be wrapped here.
-11:
-12:Signed-off-by: C O Mitter <committer@example.com>
+11:Signed-off-by: C O Mitter <committer@example.com>
EOF
test_cmp expected actual
'
@@ -1368,7 +1367,7 @@ EOF
test_cmp expected actual
'
-test_expect_success 'signoff: detect garbage in non-conforming footer' '
+test_expect_success 'signoff: tolerate garbage in conforming footer' '
append_signoff <<\EOF >actual &&
subject
@@ -1383,8 +1382,36 @@ EOF
8:
10:
13:Signed-off-by: C O Mitter <committer@example.com>
-14:
-15:Signed-off-by: C O Mitter <committer@example.com>
+EOF
+ test_cmp expected actual
+'
+
+test_expect_success 'signoff: respect trailer config' '
+ append_signoff <<\EOF >actual &&
+subject
+
+Myfooter: x
+Some Trash
+EOF
+ cat >expected <<\EOF &&
+4:Subject: [PATCH] subject
+8:
+11:
+12:Signed-off-by: C O Mitter <committer@example.com>
+EOF
+ test_cmp expected actual &&
+
+ test_config trailer.Myfooter.ifexists add &&
+ append_signoff <<\EOF >actual &&
+subject
+
+Myfooter: x
+Some Trash
+EOF
+ cat >expected <<\EOF &&
+4:Subject: [PATCH] subject
+8:
+11:Signed-off-by: C O Mitter <committer@example.com>
EOF
test_cmp expected actual
'
diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index d84897a..4003a27 100755
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh
@@ -460,6 +460,42 @@ $alt" &&
test_cmp expected actual
'
+test_expect_success 'signoff respects trailer config' '
+
+ echo 5 >positive &&
+ git add positive &&
+ git commit -s -m "subject
+
+non-trailer line
+Myfooter: x" &&
+ git cat-file commit HEAD | sed -e "1,/^\$/d" > actual &&
+ (
+ echo subject
+ echo
+ echo non-trailer line
+ echo Myfooter: x
+ echo
+ echo "Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>"
+ ) >expected &&
+ test_cmp expected actual &&
+
+ echo 6 >positive &&
+ git add positive &&
+ git -c "trailer.Myfooter.ifexists=add" commit -s -m "subject
+
+non-trailer line
+Myfooter: x" &&
+ git cat-file commit HEAD | sed -e "1,/^\$/d" > actual &&
+ (
+ echo subject
+ echo
+ echo non-trailer line
+ echo Myfooter: x
+ echo "Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>"
+ ) >expected &&
+ test_cmp expected actual
+'
+
test_expect_success 'multiple -m' '
>negative &&
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH v2 3/5] trailer: avoid unnecessary splitting on lines
From: Jonathan Tan @ 2016-11-01 20:08 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, gitster, christian.couder
In-Reply-To: <cover.1478028700.git.jonathantanmy@google.com>
trailer.c currently splits lines while processing a buffer (and also
rejoins lines when needing to invoke ignore_non_trailer).
Avoid such line splitting, except when generating the strings
corresponding to trailers (for ease of use by clients - a subsequent
patch will allow other components to obtain the layout of a trailer
block in a buffer, including the trailers themselves). The main purpose
of this is to make it easy to return pointers into the original buffer
(for a subsequent patch), but this also significantly reduces the number
of memory allocations required.
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
trailer.c | 195 ++++++++++++++++++++++++++++++++------------------------------
1 file changed, 101 insertions(+), 94 deletions(-)
diff --git a/trailer.c b/trailer.c
index 04edab2..f5427ec 100644
--- a/trailer.c
+++ b/trailer.c
@@ -102,12 +102,12 @@ static int same_trailer(struct trailer_item *a, struct arg_item *b)
return same_token(a, b) && same_value(a, b);
}
-static inline int contains_only_spaces(const char *str)
+static inline int is_blank_line(const char *str)
{
const char *s = str;
- while (*s && isspace(*s))
+ while (*s && *s != '\n' && isspace(*s))
s++;
- return !*s;
+ return !*s || *s == '\n';
}
static inline void strbuf_replace(struct strbuf *sb, const char *a, const char *b)
@@ -696,51 +696,71 @@ static void process_command_line_args(struct list_head *arg_head,
free(cl_separators);
}
-static struct strbuf **read_input_file(const char *file)
+static void read_input_file(struct strbuf *sb, const char *file)
{
- struct strbuf **lines;
- struct strbuf sb = STRBUF_INIT;
-
if (file) {
- if (strbuf_read_file(&sb, file, 0) < 0)
+ if (strbuf_read_file(sb, file, 0) < 0)
die_errno(_("could not read input file '%s'"), file);
} else {
- if (strbuf_read(&sb, fileno(stdin), 0) < 0)
+ if (strbuf_read(sb, fileno(stdin), 0) < 0)
die_errno(_("could not read from stdin"));
}
+}
- lines = strbuf_split(&sb, '\n');
+static const char *next_line(const char *str)
+{
+ const char *nl = strchrnul(str, '\n');
+ return nl + !!*nl;
+}
- strbuf_release(&sb);
+/*
+ * Return the position of the start of the last line. If len is 0, return -1.
+ */
+static int last_line(const char *buf, size_t len)
+{
+ int i;
+ if (len == 0)
+ return -1;
+ if (len == 1)
+ return 0;
+ /*
+ * Skip the last character (in addition to the null terminator),
+ * because if the last character is a newline, it is considered as part
+ * of the last line anyway.
+ */
+ i = len - 2;
- return lines;
+ for (; i >= 0; i--) {
+ if (buf[i] == '\n')
+ return i + 1;
+ }
+ return 0;
}
/*
- * Return the (0 based) index of the start of the patch or the line
- * count if there is no patch in the message.
+ * Return the position of the start of the patch or the length of str if there
+ * is no patch in the message.
*/
-static int find_patch_start(struct strbuf **lines, int count)
+static int find_patch_start(const char *str)
{
- int i;
+ const char *s;
- /* Get the start of the patch part if any */
- for (i = 0; i < count; i++) {
- if (starts_with(lines[i]->buf, "---"))
- return i;
+ for (s = str; *s; s = next_line(s)) {
+ if (starts_with(s, "---"))
+ return s - str;
}
- return count;
+ return s - str;
}
/*
- * Return the (0 based) index of the first trailer line or count if
- * there are no trailers. Trailers are searched only in the lines from
- * index (count - 1) down to index 0.
+ * Return the position of the first trailer line or len if there are no
+ * trailers.
*/
-static int find_trailer_start(struct strbuf **lines, int count)
+static int find_trailer_start(const char *buf, size_t len)
{
- int start, end_of_title, only_spaces = 1;
+ const char *s;
+ int end_of_title, l, only_spaces = 1;
int recognized_prefix = 0, trailer_lines = 0, non_trailer_lines = 0;
/*
* Number of possible continuation lines encountered. This will be
@@ -750,15 +770,16 @@ static int find_trailer_start(struct strbuf **lines, int count)
* are to be considered non-trailers).
*/
int possible_continuation_lines = 0;
+ int ret;
/* The first paragraph is the title and cannot be trailers */
- for (start = 0; start < count; start++) {
- if (lines[start]->buf[0] == comment_line_char)
+ for (s = buf; s < buf + len; s = next_line(s)) {
+ if (s[0] == comment_line_char)
continue;
- if (contains_only_spaces(lines[start]->buf))
+ if (is_blank_line(s))
break;
}
- end_of_title = start;
+ end_of_title = s - buf;
/*
* Get the start of the trailers by looking starting from the end for a
@@ -766,30 +787,33 @@ static int find_trailer_start(struct strbuf **lines, int count)
* trailers, or (ii) contains at least one Git-generated trailer and
* consists of at least 25% trailers.
*/
- for (start = count - 1; start >= end_of_title; start--) {
+ for (l = last_line(buf, len);
+ l >= end_of_title;
+ l = last_line(buf, l)) {
+ const char *bol = buf + l;
const char **p;
int separator_pos;
- if (lines[start]->buf[0] == comment_line_char) {
+ if (bol[0] == comment_line_char) {
non_trailer_lines += possible_continuation_lines;
possible_continuation_lines = 0;
continue;
}
- if (contains_only_spaces(lines[start]->buf)) {
+ if (is_blank_line(bol)) {
if (only_spaces)
continue;
non_trailer_lines += possible_continuation_lines;
if (recognized_prefix &&
trailer_lines * 3 >= non_trailer_lines)
- return start + 1;
- if (trailer_lines && !non_trailer_lines)
- return start + 1;
- return count;
+ return next_line(bol) - buf;
+ else if (trailer_lines && !non_trailer_lines)
+ return next_line(bol) - buf;
+ return len;
}
only_spaces = 0;
for (p = git_generated_prefixes; *p; p++) {
- if (starts_with(lines[start]->buf, *p)) {
+ if (starts_with(bol, *p)) {
trailer_lines++;
possible_continuation_lines = 0;
recognized_prefix = 1;
@@ -797,8 +821,8 @@ static int find_trailer_start(struct strbuf **lines, int count)
}
}
- separator_pos = find_separator(lines[start]->buf, separators);
- if (separator_pos >= 1 && !isspace(lines[start]->buf[0])) {
+ separator_pos = find_separator(bol, separators);
+ if (separator_pos >= 1 && !isspace(bol[0])) {
struct list_head *pos;
trailer_lines++;
@@ -808,13 +832,13 @@ static int find_trailer_start(struct strbuf **lines, int count)
list_for_each(pos, &conf_head) {
struct arg_item *item;
item = list_entry(pos, struct arg_item, list);
- if (token_matches_item(lines[start]->buf, item,
+ if (token_matches_item(bol, item,
separator_pos)) {
recognized_prefix = 1;
break;
}
}
- } else if (isspace(lines[start]->buf[0]))
+ } else if (isspace(bol[0]))
possible_continuation_lines++;
else {
non_trailer_lines++;
@@ -825,88 +849,70 @@ static int find_trailer_start(struct strbuf **lines, int count)
;
}
- return count;
-}
-
-/* Get the index of the end of the trailers */
-static int find_trailer_end(struct strbuf **lines, int patch_start)
-{
- struct strbuf sb = STRBUF_INIT;
- int i, ignore_bytes;
-
- for (i = 0; i < patch_start; i++)
- strbuf_addbuf(&sb, lines[i]);
- ignore_bytes = ignore_non_trailer(sb.buf, sb.len);
- strbuf_release(&sb);
- for (i = patch_start - 1; i >= 0 && ignore_bytes > 0; i--)
- ignore_bytes -= lines[i]->len;
-
- return i + 1;
+ return len;
}
-static int has_blank_line_before(struct strbuf **lines, int start)
+/* Return the position of the end of the trailers. */
+static int find_trailer_end(const char *buf, size_t len)
{
- for (;start >= 0; start--) {
- if (lines[start]->buf[0] == comment_line_char)
- continue;
- return contains_only_spaces(lines[start]->buf);
- }
- return 0;
+ return len - ignore_non_trailer(buf, len);
}
-static void print_lines(FILE *outfile, struct strbuf **lines, int start, int end)
+static int ends_with_blank_line(const char *buf, size_t len)
{
- int i;
- for (i = start; lines[i] && i < end; i++)
- fprintf(outfile, "%s", lines[i]->buf);
+ int ll = last_line(buf, len);
+ if (ll < 0)
+ return 0;
+ return is_blank_line(buf + ll);
}
static int process_input_file(FILE *outfile,
- struct strbuf **lines,
+ const char *str,
struct list_head *head)
{
- int count = 0;
- int patch_start, trailer_start, trailer_end, i;
+ int patch_start, trailer_start, trailer_end;
struct strbuf tok = STRBUF_INIT;
struct strbuf val = STRBUF_INIT;
struct trailer_item *last = NULL;
+ struct strbuf *trailer, **trailer_lines, **ptr;
- /* Get the line count */
- while (lines[count])
- count++;
-
- patch_start = find_patch_start(lines, count);
- trailer_end = find_trailer_end(lines, patch_start);
- trailer_start = find_trailer_start(lines, trailer_end);
+ patch_start = find_patch_start(str);
+ trailer_end = find_trailer_end(str, patch_start);
+ trailer_start = find_trailer_start(str, trailer_end);
/* Print lines before the trailers as is */
- print_lines(outfile, lines, 0, trailer_start);
+ fwrite(str, 1, trailer_start, outfile);
- if (!has_blank_line_before(lines, trailer_start - 1))
+ if (!ends_with_blank_line(str, trailer_start))
fprintf(outfile, "\n");
/* Parse trailer lines */
- for (i = trailer_start; i < trailer_end; i++) {
+ trailer_lines = strbuf_split_buf(str + trailer_start,
+ trailer_end - trailer_start,
+ '\n',
+ 0);
+ for (ptr = trailer_lines; *ptr; ptr++) {
int separator_pos;
- if (lines[i]->buf[0] == comment_line_char)
+ trailer = *ptr;
+ if (trailer->buf[0] == comment_line_char)
continue;
- if (last && isspace(lines[i]->buf[0])) {
+ if (last && isspace(trailer->buf[0])) {
struct strbuf sb = STRBUF_INIT;
- strbuf_addf(&sb, "%s\n%s", last->value, lines[i]->buf);
+ strbuf_addf(&sb, "%s\n%s", last->value, trailer->buf);
strbuf_strip_suffix(&sb, "\n");
free(last->value);
last->value = strbuf_detach(&sb, NULL);
continue;
}
- separator_pos = find_separator(lines[i]->buf, separators);
+ separator_pos = find_separator(trailer->buf, separators);
if (separator_pos >= 1) {
- parse_trailer(&tok, &val, NULL, lines[i]->buf,
+ parse_trailer(&tok, &val, NULL, trailer->buf,
separator_pos);
last = add_trailer_item(head,
strbuf_detach(&tok, NULL),
strbuf_detach(&val, NULL));
} else {
- strbuf_addbuf(&val, lines[i]);
+ strbuf_addbuf(&val, trailer);
strbuf_strip_suffix(&val, "\n");
add_trailer_item(head,
NULL,
@@ -914,6 +920,7 @@ static int process_input_file(FILE *outfile,
last = NULL;
}
}
+ strbuf_list_free(trailer_lines);
return trailer_end;
}
@@ -962,7 +969,7 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct str
{
LIST_HEAD(head);
LIST_HEAD(arg_head);
- struct strbuf **lines;
+ struct strbuf sb = STRBUF_INIT;
int trailer_end;
FILE *outfile = stdout;
@@ -970,13 +977,13 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct str
git_config(git_trailer_default_config, NULL);
git_config(git_trailer_config, NULL);
- lines = read_input_file(file);
+ read_input_file(&sb, file);
if (in_place)
outfile = create_in_place_tempfile(file);
/* Print the lines before the trailers */
- trailer_end = process_input_file(outfile, lines, &head);
+ trailer_end = process_input_file(outfile, sb.buf, &head);
process_command_line_args(&arg_head, trailers);
@@ -987,11 +994,11 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct str
free_all(&head);
/* Print the lines after the trailers as is */
- print_lines(outfile, lines, trailer_end, INT_MAX);
+ fwrite(sb.buf + trailer_end, 1, sb.len - trailer_end, outfile);
if (in_place)
if (rename_tempfile(&trailers_tempfile, file))
die_errno(_("could not rename temporary file to %s"), file);
- strbuf_list_free(lines);
+ strbuf_release(&sb);
}
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH v2 2/5] commit: make ignore_non_trailer take buf/len
From: Jonathan Tan @ 2016-11-01 20:08 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, gitster, christian.couder
In-Reply-To: <cover.1478028700.git.jonathantanmy@google.com>
Make ignore_non_trailer take a buf/len pair instead of struct strbuf.
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
builtin/commit.c | 2 +-
commit.c | 22 +++++++++++-----------
commit.h | 2 +-
trailer.c | 2 +-
4 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index 8976c3d..887ccc7 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -790,7 +790,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
strbuf_stripspace(&sb, 0);
if (signoff)
- append_signoff(&sb, ignore_non_trailer(&sb), 0);
+ append_signoff(&sb, ignore_non_trailer(sb.buf, sb.len), 0);
if (fwrite(sb.buf, 1, sb.len, s->fp) < sb.len)
die_errno(_("could not write commit template"));
diff --git a/commit.c b/commit.c
index 856fd4a..2cf8515 100644
--- a/commit.c
+++ b/commit.c
@@ -1649,7 +1649,7 @@ const char *find_commit_header(const char *msg, const char *key, size_t *out_len
}
/*
- * Inspect sb and determine the true "end" of the log message, in
+ * Inspect the given string and determine the true "end" of the log message, in
* order to find where to put a new Signed-off-by: line. Ignored are
* trailing comment lines and blank lines, and also the traditional
* "Conflicts:" block that is not commented out, so that we can use
@@ -1659,37 +1659,37 @@ const char *find_commit_header(const char *msg, const char *key, size_t *out_len
* Returns the number of bytes from the tail to ignore, to be fed as
* the second parameter to append_signoff().
*/
-int ignore_non_trailer(struct strbuf *sb)
+int ignore_non_trailer(const char *buf, size_t len)
{
int boc = 0;
int bol = 0;
int in_old_conflicts_block = 0;
- while (bol < sb->len) {
- char *next_line;
+ while (bol < len) {
+ const char *next_line = memchr(buf + bol, '\n', len - bol);
- if (!(next_line = memchr(sb->buf + bol, '\n', sb->len - bol)))
- next_line = sb->buf + sb->len;
+ if (!next_line)
+ next_line = buf + len;
else
next_line++;
- if (sb->buf[bol] == comment_line_char || sb->buf[bol] == '\n') {
+ if (buf[bol] == comment_line_char || buf[bol] == '\n') {
/* is this the first of the run of comments? */
if (!boc)
boc = bol;
/* otherwise, it is just continuing */
- } else if (starts_with(sb->buf + bol, "Conflicts:\n")) {
+ } else if (starts_with(buf + bol, "Conflicts:\n")) {
in_old_conflicts_block = 1;
if (!boc)
boc = bol;
- } else if (in_old_conflicts_block && sb->buf[bol] == '\t') {
+ } else if (in_old_conflicts_block && buf[bol] == '\t') {
; /* a pathname in the conflicts block */
} else if (boc) {
/* the previous was not trailing comment */
boc = 0;
in_old_conflicts_block = 0;
}
- bol = next_line - sb->buf;
+ bol = next_line - buf;
}
- return boc ? sb->len - boc : 0;
+ return boc ? len - boc : 0;
}
diff --git a/commit.h b/commit.h
index afd14f3..9c12abb 100644
--- a/commit.h
+++ b/commit.h
@@ -355,7 +355,7 @@ extern const char *find_commit_header(const char *msg, const char *key,
size_t *out_len);
/* Find the end of the log message, the right place for a new trailer. */
-extern int ignore_non_trailer(struct strbuf *sb);
+extern int ignore_non_trailer(const char *buf, size_t len);
typedef void (*each_mergetag_fn)(struct commit *commit, struct commit_extra_header *extra,
void *cb_data);
diff --git a/trailer.c b/trailer.c
index 0ee634f..04edab2 100644
--- a/trailer.c
+++ b/trailer.c
@@ -836,7 +836,7 @@ static int find_trailer_end(struct strbuf **lines, int patch_start)
for (i = 0; i < patch_start; i++)
strbuf_addbuf(&sb, lines[i]);
- ignore_bytes = ignore_non_trailer(&sb);
+ ignore_bytes = ignore_non_trailer(sb.buf, sb.len);
strbuf_release(&sb);
for (i = patch_start - 1; i >= 0 && ignore_bytes > 0; i--)
ignore_bytes -= lines[i]->len;
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH v2 1/5] trailer: be stricter in parsing separators
From: Jonathan Tan @ 2016-11-01 20:08 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, gitster, christian.couder
In-Reply-To: <cover.1478028700.git.jonathantanmy@google.com>
Currently, a line is interpreted to be a trailer line if it contains a
separator. Make parsing stricter by requiring the text on the left of
the separator, if not the empty string, to be of the "<token><optional
whitespace>" form.
(The find_separator function distinguishes the no-separator case from
the separator-starts-line case because some callers of this function
need such a distinction.)
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
trailer.c | 23 +++++++++++++++++------
1 file changed, 17 insertions(+), 6 deletions(-)
diff --git a/trailer.c b/trailer.c
index f0ecde2..0ee634f 100644
--- a/trailer.c
+++ b/trailer.c
@@ -563,15 +563,26 @@ static int token_matches_item(const char *tok, struct arg_item *item, int tok_le
}
/*
- * Return the location of the first separator in line, or -1 if there is no
- * separator.
+ * If the given line is of the form
+ * "<token><optional whitespace><separator>..." or "<separator>...", return the
+ * location of the separator. Otherwise, return -1.
*/
static int find_separator(const char *line, const char *separators)
{
- int loc = strcspn(line, separators);
- if (!line[loc])
- return -1;
- return loc;
+ int whitespace_found = 0;
+ const char *c;
+ for (c = line; *c; c++) {
+ if (strchr(separators, *c))
+ return c - line;
+ if (!whitespace_found && (isalnum(*c) || *c == '-'))
+ continue;
+ if (c != line && (*c == ' ' || *c == '\t')) {
+ whitespace_found = 1;
+ continue;
+ }
+ break;
+ }
+ return -1;
}
/*
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH v2 0/5] Make other git commands use trailer layout
From: Jonathan Tan @ 2016-11-01 20:08 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, gitster, christian.couder
In-Reply-To: <cover.1477698917.git.jonathantanmy@google.com>
Thanks for all your comments.
This patch set is now built off master (since jt/trailer-with-cruft is
merged).
I couldn't think of an easy way to clearly decide if a token with spaces
should be considered a token, so I've tightened the restrictions. One
benefit is that we no longer need to create temporary strings that
include '\n' to be passed into the find_separator method.
In 2/4 (now 3/5), I've also changed some variable names as requested
(e.g. sb -> input, and un-did some others).
Jonathan Tan (5):
trailer: be stricter in parsing separators
commit: make ignore_non_trailer take buf/len
trailer: avoid unnecessary splitting on lines
trailer: have function to describe trailer layout
sequencer: use trailer's trailer layout
builtin/commit.c | 2 +-
commit.c | 22 ++--
commit.h | 2 +-
sequencer.c | 75 +++---------
t/t3511-cherry-pick-x.sh | 16 ++-
t/t4014-format-patch.sh | 37 +++++-
t/t7501-commit.sh | 36 ++++++
trailer.c | 296 ++++++++++++++++++++++++++++-------------------
trailer.h | 25 ++++
9 files changed, 313 insertions(+), 198 deletions(-)
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply
* Re: [PATCH v1 15/19] config: add git_config_get_date_string() from gc.c
From: Junio C Hamano @ 2016-11-01 19:28 UTC (permalink / raw)
To: Christian Couder
Cc: git, Nguyen Thai Ngoc Duy, Ævar Arnfjörð Bjarmason,
Christian Couder
In-Reply-To: <20161023092648.12086-16-chriscool@tuxfamily.org>
Christian Couder <christian.couder@gmail.com> writes:
> This function will be used in a following commit to get the expiration
> time of the shared index files from the config, and it is generic
> enough to be put in "config.c".
Is it generic enough that a helper that sounds as if it can get any
date string dies if it is given a future date? I somehow doubt it.
At the minimum, it must be made clear that there is an artificial
limitation that the current set of callers find useful in cache.h as
a one-liner comment next to the added declaration. Then people with
the same need (i.e. they want to reject future timestamps) can
decide to use it, while others would stay away from it.
If you can come up with a better word to use to encode that
artificial limitation in its name, renaming it is even better.
^ permalink raw reply
* Re: [PATCH v1 14/19] read-cache: touch shared index files when used
From: Junio C Hamano @ 2016-11-01 19:23 UTC (permalink / raw)
To: Duy Nguyen
Cc: Christian Couder, Git Mailing List,
Ævar Arnfjörð Bjarmason, Christian Couder
In-Reply-To: <CACsJy8As2o-ZDXMRWeebpXiWUrDMLaXC2H1R+OMbhAMmM8V_wg@mail.gmail.com>
Duy Nguyen <pclouds@gmail.com> writes:
> On Sun, Oct 23, 2016 at 4:26 PM, Christian Couder
> <christian.couder@gmail.com> wrote:
>> @@ -2268,6 +2268,12 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,
>
> Doing this in read_index_from() would keep the shared file even more
> "fresher" since read happens a lot more often than write. But I think
> our main concern is not the temporary index files created by the user
> scripts, but $GIT_DIR/index.lock (make sure we don't accidentally
> delete its shared file before it gets renamed to $GIT_DIR/index). For
> this case, I think refreshing in write_locked_index is enough.
Also warning() is unwarranted.
You may be accessing somebody else's repository to help diagnose the
issue without having any write access. Treat the utime() like the
opportunistic index refresh done by "git status"---if we can write,
great, but it is not a problem if we can't.
>
>> int ret = write_shared_index(istate, lock, flags);
>> if (ret)
>> return ret;
>> + } else {
>> + /* Signal that the shared index is used */
>> + const char *shared_index = git_path("sharedindex.%s",
>> + sha1_to_hex(si->base_sha1));
>> + if (!check_and_freshen_file(shared_index, 1))
>> + warning("could not freshen '%s'", shared_index);
>
> _()
^ permalink raw reply
* Re: [PATCH v1 12/19] Documentation/config: add splitIndex.maxPercentChange
From: Junio C Hamano @ 2016-11-01 19:19 UTC (permalink / raw)
To: Christian Couder
Cc: git, Nguyen Thai Ngoc Duy, Ævar Arnfjörð Bjarmason,
Christian Couder
In-Reply-To: <20161023092648.12086-13-chriscool@tuxfamily.org>
Christian Couder <christian.couder@gmail.com> writes:
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
> Documentation/config.txt | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 96521a4..380eeb8 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2763,6 +2763,19 @@ showbranch.default::
> The default set of branches for linkgit:git-show-branch[1].
> See linkgit:git-show-branch[1].
>
> +splitIndex.maxPercentChange::
> + When the split index feature is used, this specifies the
> + percent of entries the split index can contain compared to the
> + whole number of entries in both the split index and the shared
> + index before a new shared index is written.
> + The value should be between 0 and 100. If the value is 0 then
> + a new shared index is always written, if it is 100 a new
> + shared index is never written.
Hmph. The early part of the description implies this will kick in
only when some other conditions (i.e. the bit in the index or the
other configuration) are met, but if this disables the split index
when it is set to 0, would we even need the other configuration
variable? IOW, perhaps we can do without core.splitIndex?
> + By default the value is 20, so a new shared index is written
> + if the number of entries in the split index would be greater
> + than 20 percent of the total number of entries.
> + See linkgit:git-update-index[1].
^ permalink raw reply
* Re: [PATCH v1 11/19] t1700: add tests for splitIndex.maxPercentChange
From: Junio C Hamano @ 2016-11-01 19:15 UTC (permalink / raw)
To: Christian Couder
Cc: git, Nguyen Thai Ngoc Duy, Ævar Arnfjörð Bjarmason,
Christian Couder
In-Reply-To: <20161023092648.12086-12-chriscool@tuxfamily.org>
Christian Couder <christian.couder@gmail.com> writes:
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
> t/t1700-split-index.sh | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 72 insertions(+)
>
> diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
> index 507a1dd..f03addf 100755
> --- a/t/t1700-split-index.sh
> +++ b/t/t1700-split-index.sh
> @@ -238,4 +238,76 @@ EOF
> test_cmp expect actual
> '
>
> +test_expect_success 'set core.splitIndex config variable to true' '
> + git config core.splitIndex true &&
> + : >three &&
> + git update-index --add three &&
> + BASE=$(test-dump-split-index .git/index | grep "^base") &&
> + test-dump-split-index .git/index | sed "/^own/d" >actual &&
> + cat >expect <<EOF &&
> +$BASE
> +replacements:
> +deletions:
> +EOF
Using <<-EOF lets us indent the above four lines with a horizontal
tab to align with the remainder of this test_expect_success block,
so let's do that.
^ permalink raw reply
* Re: [PATCH v1 09/19] config: add git_config_get_max_percent_split_change()
From: Junio C Hamano @ 2016-11-01 19:13 UTC (permalink / raw)
To: Duy Nguyen
Cc: Christian Couder, Git Mailing List,
Ævar Arnfjörð Bjarmason, Christian Couder
In-Reply-To: <CACsJy8A0djR6=s0AY0tzVehYY5b1-o11uRsFdGtOUCeu4Z6Xjw@mail.gmail.com>
Duy Nguyen <pclouds@gmail.com> writes:
> On Sun, Oct 23, 2016 at 4:26 PM, Christian Couder
> <christian.couder@gmail.com> wrote:
>> This new function will be used in a following commit to get the
>> +int git_config_get_max_percent_split_change(void)
>> +{
>> + int val = -1;
>> +
>> + if (!git_config_get_int("splitindex.maxpercentchange", &val)) {
>> + if (0 <= val && val <= 100)
>> + return val;
>> +
>> + error("splitindex.maxpercentchange value '%d' "
>
> We should keep camelCase form for easy reading. And wrap this string with _().
>
>> + "should be between 0 and 100", val);
>
> I wonder if anybody would try to put 12.3 here and confused by the
> error message, because 0 <= 12.3 <= 100, but it's not an integer..
> Ah.. never mind, die_bad_number() would be called first in this case
> with a loud and clear complaint.
OK.
>
>> + return -1;
Perhaps do the usual
return error(_("..."));
here?
>> + }
>> +
>> + return -1; /* default value */
^ permalink raw reply
* Re: [PATCH v1 05/19] update-index: warn in case of split-index incoherency
From: Junio C Hamano @ 2016-11-01 19:05 UTC (permalink / raw)
To: Duy Nguyen
Cc: Christian Couder, Git Mailing List,
Ævar Arnfjörð Bjarmason, Christian Couder
In-Reply-To: <CACsJy8Br2q0aadTFjkNgb=oN8nSzbkWJEK7bCCgr7v-oOZtrSA@mail.gmail.com>
Duy Nguyen <pclouds@gmail.com> writes:
>> diff --git a/builtin/update-index.c b/builtin/update-index.c
>> index b75ea03..a14dbf2 100644
>> --- a/builtin/update-index.c
>> +++ b/builtin/update-index.c
>> @@ -1098,12 +1098,21 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
>> }
>>
>> if (split_index > 0) {
>> + if (git_config_get_split_index() == 0)
>> + warning("core.splitIndex is set to false; "
>> + "remove or change it, if you really want to "
>> + "enable split index");
>
> Wrap this string and the one below with _() so they can be translated.
True.
I further wonder if a natural reaction from users after seeing this
message is "I do want to--what else would I use that option to run
you for? Just do as you are told, instead of telling me what to
do!". Is this warning really a good idea, or shouldn't these places
be setting the configuration?
>> if (the_index.split_index)
>> the_index.cache_changed |= SPLIT_INDEX_ORDERED;
>> else
>> add_split_index(&the_index);
>> - } else if (!split_index)
>> + } else if (!split_index) {
>> + if (git_config_get_split_index() == 1)
>> + warning("core.splitIndex is set to true; "
>> + "remove or change it, if you really want to "
>> + "disable split index");
>> remove_split_index(&the_index);
>> + }
>>
>> switch (untracked_cache) {
>> case UC_UNSPECIFIED:
>> --
>> 2.10.1.462.g7e1e03a
^ permalink raw reply
* Re: [PATCH 4/4] sequencer: use trailer's trailer layout
From: Junio C Hamano @ 2016-11-01 18:16 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git
In-Reply-To: <a416ab9b-ff1f-9a71-3e58-60fd4f8a6b8e@google.com>
Jonathan Tan <jonathantanmy@google.com> writes:
>>> 9:I want to mention about Signed-off-by: here.
>> ...
>> This seems a bit weird.
>
> This is because the "I want to mention" block has 100% trailer lines
> (since its only line contains a colon). We could forbid spaces in
> trailer field names, but as you said [1], it might be better to allow
> them since users might include them.
That merely means that the implementation of the wish expressed in
[1] was overly loose and needs a bit of tightening, isn't it?
> The original sequencer.c interpreted this block as not a trailer
> block, because it only accepted alphanumeric characters or '-' before
> the colon (and no spaces) - hence the difference in behavior.
That sounds more sensible to me. Would there be an easy way to
still allow misspelled "Thanks to:" but not be fooled by an obvious
nonsense like this example, without going deep into natural language
processing? If not, we may want to tighten it back.
>
> [1] <xmqqbmyhr4vt.fsf@gitster.mtv.corp.google.com>
^ permalink raw reply
* Re: [ANNOUNCE] Git v2.10.2
From: Johannes Schindelin @ 2016-11-01 18:01 UTC (permalink / raw)
To: git-for-windows, Junio C Hamano; +Cc: git
In-Reply-To: <alpine.DEB.2.20.1610291031250.3264@virtualbox>
Hi all,
On Sat, 29 Oct 2016, Johannes Schindelin wrote:
> On Fri, 28 Oct 2016, Junio C Hamano wrote:
>
> > The latest maintenance release Git v2.10.2 is now available at
> > the usual places.
>
> The corresponding Git for Windows version will be hopefully out on
> Tuesday: https://github.com/git-for-windows/git/milestone/5
As of time of writing, cURL has not been released. Git for Windows v2.10.2
will have to wait for tomorrow, or whenever cURL 7.51.0 will be released.
Ciao,
Johannes
^ permalink raw reply
* Re: Git issue
From: Junio C Hamano @ 2016-11-01 18:11 UTC (permalink / raw)
To: Jeff King; +Cc: Halde, Faiz, git@vger.kernel.org
In-Reply-To: <20161101174526.e2tilsriz2fqaru3@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Tue, Nov 01, 2016 at 10:28:57AM +0000, Halde, Faiz wrote:
>
>> I frequently use the following command to ignore changes done in a file
>>
>> git update-index --assume-unchanged somefile
>>
>> Now when I do a pull from my remote branch and say the file 'somefile'
>> was changed locally and in remote, git will abort the merge saying I
>> need to commit my changes of 'somefile'.
>>
>> But isn't the whole point of the above command to ignore the changes
>> within the file?
>
> No. The purpose of --assume-unchanged is to promise git that you will
> not change the file, so that it may skip checking the file contents in
> some cases as an optimization.
That's correct.
The next anticipated question is "then how would I tell Git to
ignore changes done to a file locally by me?", whose short answer is
"You don't", of course.
People may however wonder, if Git can make things more automatic if
the user is willing to tell her intention of what should happen to
"somefile" in the example above when an operation requested cannot
proceed while ignoring the local changes. For example, "ignore my
change and overwrite as needed" could be such an instruction (and it
is obvious what should happen in that case when "git pull" was
done--just clobber it with the version from the other side).
As I do not think of other sensible alternative behaviour, and I do
not think Git should make it easy to lose local changes when the
user is doing things like "pull" [*1*], it leads to the longer
answer to the question, which is again "You don't" ;-).
[Footnote]
*1* Things like "git checkout [<tree>] [--] <path>", "git rm -f" and
"git reset --hard" are ways to explicit request nuking the local
changes, and presence of these commands do not contradict with
"do not make it easy to lose local changes", of course.
^ permalink raw reply
* Re: Git issue
From: Jeff King @ 2016-11-01 17:45 UTC (permalink / raw)
To: Halde, Faiz; +Cc: git@vger.kernel.org
In-Reply-To: <BY2PR0601MB16400EAC3E9683841907F4B2A2A10@BY2PR0601MB1640.namprd06.prod.outlook.com>
On Tue, Nov 01, 2016 at 10:28:57AM +0000, Halde, Faiz wrote:
> I frequently use the following command to ignore changes done in a file
>
> git update-index --assume-unchanged somefile
>
> Now when I do a pull from my remote branch and say the file 'somefile'
> was changed locally and in remote, git will abort the merge saying I
> need to commit my changes of 'somefile'.
>
> But isn't the whole point of the above command to ignore the changes
> within the file?
No. The purpose of --assume-unchanged is to promise git that you will
not change the file, so that it may skip checking the file contents in
some cases as an optimization.
From "git help update-index":
--[no-]assume-unchanged
When this flag is specified, the object names recorded for
the paths are not updated. Instead, this option sets/unsets
the "assume unchanged" bit for the paths. When the "assume
unchanged" bit is on, the user promises not to change the
file and allows Git to assume that the working tree file
matches what is recorded in the index. If you want to change
the working tree file, you need to unset the bit to tell Git.
This is sometimes helpful when working with a big project on
a filesystem that has very slow lstat(2) system call (e.g.
cifs).
Git will fail (gracefully) in case it needs to modify this
file in the index e.g. when merging in a commit; thus, in
case the assumed-untracked file is changed upstream, you will
need to handle the situation manually.
-Peff
^ permalink raw reply
* Re: [PATCH 4/4] sequencer: use trailer's trailer layout
From: Jonathan Tan @ 2016-11-01 17:38 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqeg2wqa1e.fsf@gitster.mtv.corp.google.com>
On 10/31/2016 06:11 PM, Junio C Hamano wrote:
> Jonathan Tan <jonathantanmy@google.com> writes:
>> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
>> index ba4902d..635b394 100755
>> --- a/t/t4014-format-patch.sh
>> +++ b/t/t4014-format-patch.sh
>> @@ -1277,8 +1277,7 @@ EOF
>> 4:Subject: [PATCH] subject
>> 8:
>> 9:I want to mention about Signed-off-by: here.
>> -10:
>> -11:Signed-off-by: C O Mitter <committer@example.com>
>> +10:Signed-off-by: C O Mitter <committer@example.com>
>> EOF
>> test_cmp expected actual
>> '
>
> The original log message is a single-liner subject line, blank, "I
> want to mention..." and when asked to append S-o-b:, we would want
> to see a blank before the added S-o-b, no?
>
> This seems a bit weird.
This is because the "I want to mention" block has 100% trailer lines
(since its only line contains a colon). We could forbid spaces in
trailer field names, but as you said [1], it might be better to allow
them since users might include them.
The original sequencer.c interpreted this block as not a trailer block,
because it only accepted alphanumeric characters or '-' before the colon
(and no spaces) - hence the difference in behavior.
[1] <xmqqbmyhr4vt.fsf@gitster.mtv.corp.google.com>
^ permalink raw reply
* Re: [PATCH v2 1/6] submodules: add helper functions to determine presence of submodules
From: Stefan Beller @ 2016-11-01 17:31 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Brandon Williams, git@vger.kernel.org
In-Reply-To: <xmqq7f8nqfqc.fsf@gitster.mtv.corp.google.com>
On Tue, Nov 1, 2016 at 10:20 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
> Maybe I am old fashioned, but I'd feel better to see these with
> explicit "extern" in front (check the older header files like
> cache.h when you are in doubt what the project convention has been).
I did check the other files and saw them, so I was very unsure what to
suggest here. I only saw the extern keyword used in headers that were
there when Git was really young, so I assumed it's a style nit by kernel
developers. Thanks for clarifying!
I think we'll want to have some consistency though, so we
maybe want to coordinate a cleanup of submodule.h as well as
submodule-config.h to mark all the functions extern.
This doesn't need to be a all-at-once thing, but we'd keep it in mind
for future declarations in the header.
Thanks,
Stefan
^ permalink raw reply
* Re: [PATCH v2 3/6] grep: add submodules as a grep source type
From: Junio C Hamano @ 2016-11-01 17:31 UTC (permalink / raw)
To: Brandon Williams; +Cc: git, sbeller
In-Reply-To: <1477953496-103596-4-git-send-email-bmwill@google.com>
Brandon Williams <bmwill@google.com> writes:
> Add `GREP_SOURCE_SUBMODULE` as a grep_source type and cases for this new
> type in the various switch statements in grep.c.
>
> When initializing a grep_source with type `GREP_SOURCE_SUBMODULE` the
> identifier can either be NULL (to indicate that the working tree will be
> used) or a SHA1 (the REV of the submodule to be grep'd). If the
> identifier is a SHA1 then we want to fall through to the
> `GREP_SOURCE_SHA1` case to handle the copying of the SHA1.
>
> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---
Conceptually, it somehow feels strange to have SUBMODULE in this
set.
Source being SHA1 means we are doing a recursive grep in a tree
structure that is stored in the object store, being FILE means we
are reading from the filesystem, being BUF means we are fed in-core
buffer (e.g. to implement the "log --grep='string in message'"). It
is unclear how SUBMODULE fits in that picture, as we do not have a
caller that uses the type at this step yet. Hopefully it will
become obvious why this new type belongs to that set as the series
progresses ;-)
> grep.c | 16 +++++++++++++++-
> grep.h | 1 +
> 2 files changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/grep.c b/grep.c
> index 1194d35..0dbdc1d 100644
> --- a/grep.c
> +++ b/grep.c
> @@ -1735,12 +1735,23 @@ void grep_source_init(struct grep_source *gs, enum grep_source_type type,
> case GREP_SOURCE_FILE:
> gs->identifier = xstrdup(identifier);
> break;
> + case GREP_SOURCE_SUBMODULE:
> + if (!identifier) {
> + gs->identifier = NULL;
> + break;
> + }
> + /*
> + * FALL THROUGH
> + * If the identifier is non-NULL (in the submodule case) it
> + * will be a SHA1 that needs to be copied.
> + */
> case GREP_SOURCE_SHA1:
> gs->identifier = xmalloc(20);
> hashcpy(gs->identifier, identifier);
> break;
> case GREP_SOURCE_BUF:
> gs->identifier = NULL;
> + break;
> }
> }
>
> @@ -1760,6 +1771,7 @@ void grep_source_clear_data(struct grep_source *gs)
> switch (gs->type) {
> case GREP_SOURCE_FILE:
> case GREP_SOURCE_SHA1:
> + case GREP_SOURCE_SUBMODULE:
> free(gs->buf);
> gs->buf = NULL;
> gs->size = 0;
> @@ -1831,8 +1843,10 @@ static int grep_source_load(struct grep_source *gs)
> return grep_source_load_sha1(gs);
> case GREP_SOURCE_BUF:
> return gs->buf ? 0 : -1;
> + case GREP_SOURCE_SUBMODULE:
> + break;
> }
> - die("BUG: invalid grep_source type");
> + die("BUG: invalid grep_source type to load");
> }
>
> void grep_source_load_driver(struct grep_source *gs)
> diff --git a/grep.h b/grep.h
> index 5856a23..267534c 100644
> --- a/grep.h
> +++ b/grep.h
> @@ -161,6 +161,7 @@ struct grep_source {
> GREP_SOURCE_SHA1,
> GREP_SOURCE_FILE,
> GREP_SOURCE_BUF,
> + GREP_SOURCE_SUBMODULE,
> } type;
> void *identifier;
^ permalink raw reply
* Re: [PATCH v2 4/6] grep: optionally recurse into submodules
From: Stefan Beller @ 2016-11-01 17:26 UTC (permalink / raw)
To: Brandon Williams; +Cc: git@vger.kernel.org
In-Reply-To: <1477953496-103596-5-git-send-email-bmwill@google.com>
On Mon, Oct 31, 2016 at 3:38 PM, Brandon Williams <bmwill@google.com> wrote:
>
> +--recurse-submodules::
> + Recursively search in each submodule that has been initialized and
> + checked out in the repository.
> +
and warn otherwise.
> +
> + /*
> + * Limit number of threads for child process to use.
> + * This is to prevent potential fork-bomb behavior of git-grep as each
> + * submodule process has its own thread pool.
> + */
> + if (num_threads)
> + argv_array_pushf(&submodule_options, "--threads=%d",
> + (num_threads + 1) / 2);
Just like in the run_parallel machinery this seems like an approximate
workaround. I'm ok with that for now.
Ideally the parent/child can send each other signals to hand
over threads. (SIGUSR1/SIGUSR2 would be enough to do that,
though I wonder if that is as portable as I would hope. Or we'd look at
"make" and see how they handle recursive calls.
> +
> + /*
> + * Capture output to output buffer and check the return code from the
> + * child process. A '0' indicates a hit, a '1' indicates no hit and
> + * anything else is an error.
> + */
> + status = capture_command(&cp, &w->out, 0);
> + if (status && (status != 1))
Does the user have enough information what went wrong?
Is the child verbose enough, such that we do not need to give a
die[_errno]("submodule processs failed") ?
> +static int grep_submodule(struct grep_opt *opt, const unsigned char *sha1,
> + const char *filename, const char *path)
> +{
> + if (!(is_submodule_initialized(path) &&
If it is not initialized, the user "obviously" doesn't care, so maybe
we only need to warn
if init, but not checked out?
> + is_submodule_checked_out(path))) {
> + warning("skiping submodule '%s%s' since it is not initialized and checked out",
> + super_prefix ? super_prefix : "",
> + path);
> + return 0;
> + }
> +
> +#ifndef NO_PTHREADS
> + if (num_threads) {
> + add_work(opt, GREP_SOURCE_SUBMODULE, filename, path, sha1);
> + return 0;
> + } else
> +#endif
> + {
> + struct work_item w;
> + int hit;
> +
> + grep_source_init(&w.source, GREP_SOURCE_SUBMODULE,
> + filename, path, sha1);
> + strbuf_init(&w.out, 0);
> + opt->output_priv = &w;
> + hit = grep_submodule_launch(opt, &w.source);
> +
> + write_or_die(1, w.out.buf, w.out.len);
> +
> + grep_source_clear(&w.source);
> + strbuf_release(&w.out);
> + return hit;
> + }
> +}
> +
> +static int grep_cache(struct grep_opt *opt, const struct pathspec *pathspec,
> + int cached)
> {
> int hit = 0;
> int nr;
> + struct strbuf name = STRBUF_INIT;
> + int name_base_len = 0;
> + if (super_prefix) {
> + name_base_len = strlen(super_prefix);
> + strbuf_addstr(&name, super_prefix);
> + }
> +
> read_cache();
>
> for (nr = 0; nr < active_nr; nr++) {
> const struct cache_entry *ce = active_cache[nr];
> - if (!S_ISREG(ce->ce_mode))
> - continue;
> - if (!ce_path_match(ce, pathspec, NULL))
> - continue;
> - /*
> - * If CE_VALID is on, we assume worktree file and its cache entry
> - * are identical, even if worktree file has been modified, so use
> - * cache version instead
> - */
> - if (cached || (ce->ce_flags & CE_VALID) || ce_skip_worktree(ce)) {
> - if (ce_stage(ce) || ce_intent_to_add(ce))
> - continue;
> - hit |= grep_sha1(opt, ce->oid.hash, ce->name, 0,
> - ce->name);
> + strbuf_setlen(&name, name_base_len);
> + strbuf_addstr(&name, ce->name);
> +
> + if (S_ISREG(ce->ce_mode) &&
> + match_pathspec(pathspec, name.buf, name.len, 0, NULL,
> + S_ISDIR(ce->ce_mode) ||
> + S_ISGITLINK(ce->ce_mode))) {
Why do we have to pass the ISDIR and ISGITLINK here for the regular file
case? ce_path_match and match_pathspec are doing the same thing?
> + /*
> + * If CE_VALID is on, we assume worktree file and its
> + * cache entry are identical, even if worktree file has
> + * been modified, so use cache version instead
> + */
> + if (cached || (ce->ce_flags & CE_VALID) ||
> + ce_skip_worktree(ce)) {
> + if (ce_stage(ce) || ce_intent_to_add(ce))
> + continue;
> + hit |= grep_sha1(opt, ce->oid.hash, ce->name,
> + 0, ce->name);
> + } else {
> + hit |= grep_file(opt, ce->name);
> + }
> + } else if (recurse_submodules && S_ISGITLINK(ce->ce_mode) &&
> + submodule_path_match(pathspec, name.buf, NULL)) {
> + hit |= grep_submodule(opt, NULL, ce->name, ce->name);
What is the difference between the last two parameters?
> + * filename: name of the submodule including tree name of parent
> + * path: location of the submodule
That sounds the same to me.
> }
>
> + if (recurse_submodules && (!use_index || untracked || list.nr))
> + die(_("option not supported with --recurse-submodules."));
The user asks: Which option?
> +
> +test_expect_success 'grep and nested submodules' '
> + git init submodule/sub &&
> + echo "foobar" >submodule/sub/a &&
> + git -C submodule/sub add a &&
> + git -C submodule/sub commit -m "add a" &&
> + git -C submodule submodule add ./sub &&
> + git -C submodule add sub &&
> + git -C submodule commit -m "added sub" &&
> + git add submodule &&
> + git commit -m "updated submodule" &&
Both in this test as well as in the setup, we setup a repository
with submodules, that have clean working dirs.
What should happen with dirty working dirs. dirty in the sense:
* file untracked in the submodule
* file added in the submodule, but not committed
* file committed in the submodule, that commit is
untracked in the superproject
* file committed in the submodule, that commit is
added to the index in the superproject
* (last case is just as above:) file committed in submodule,
that commit was committed into the superproject.
^ permalink raw reply
* Re: [PATCH v2 1/6] submodules: add helper functions to determine presence of submodules
From: Brandon Williams @ 2016-11-01 17:24 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Stefan Beller, git@vger.kernel.org
In-Reply-To: <xmqq7f8nqfqc.fsf@gitster.mtv.corp.google.com>
On 11/01, Junio C Hamano wrote:
> Stefan Beller <sbeller@google.com> writes:
>
> Overall the suggestions from you in this review is good and please
> consider anything I did not mention I agree with you. Thanks.
>
> >> +extern int is_submodule_initialized(const char *path);
> >> +extern int is_submodule_checked_out(const char *path);
> >
> > no need to put extern for function names. (no other functions in this
> > header are extern. so local consistency maybe? I'd also claim that
> > all other extern functions in headers ought to be declared without
> > being extern)
>
> Maybe I am old fashioned, but I'd feel better to see these with
> explicit "extern" in front (check the older header files like
> cache.h when you are in doubt what the project convention has been).
I wouldn't consider that old fashion as I'm fairly new to all this and
I also prefer the explicit "extern" :P
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH v2 1/6] submodules: add helper functions to determine presence of submodules
From: Brandon Williams @ 2016-11-01 17:23 UTC (permalink / raw)
To: Stefan Beller; +Cc: git@vger.kernel.org
In-Reply-To: <CAGZ79kamzSPyM65k9ugS0dAJCfGnGvk3m2p+XtCEozCvoZ5+OA@mail.gmail.com>
On 10/31, Stefan Beller wrote:
> On Mon, Oct 31, 2016 at 3:38 PM, Brandon Williams <bmwill@google.com> wrote:
> > +int is_submodule_checked_out(const char *path)
> > +{
> > + int ret = 0;
> > + struct strbuf buf = STRBUF_INIT;
> > +
> > + strbuf_addf(&buf, "%s/.git", path);
> > + ret = file_exists(buf.buf);
>
> I think we can be more tight here; instead of checking
> if the file or directory exists, we should be checking if
> it is a valid git directory, i.e. s/file_exists/resolve_gitdir/
> which returns a path to the actual git dir (in case of a .gitlink)
> or NULL when nothing is found that looks like a git directory or
> pointer to it.
Sounds good.
> > +
> > + strbuf_release(&buf);
> > + return ret;
> > +}
> > +
> > int parse_submodule_update_strategy(const char *value,
> > struct submodule_update_strategy *dst)
> > {
> > diff --git a/submodule.h b/submodule.h
> > index d9e197a..bd039ca 100644
> > --- a/submodule.h
> > +++ b/submodule.h
> > @@ -37,6 +37,8 @@ void set_diffopt_flags_from_submodule_config(struct diff_options *diffopt,
> > const char *path);
> > int submodule_config(const char *var, const char *value, void *cb);
> > void gitmodules_config(void);
> > +extern int is_submodule_initialized(const char *path);
> > +extern int is_submodule_checked_out(const char *path);
>
> no need to put extern for function names. (no other functions in this
> header are extern. so local consistency maybe? I'd also claim that
> all other extern functions in headers ought to be declared without
> being extern)
From looking around at other sections of the code it seems like the
extern keyword is used for functions declared in header files. What's
the style guideline for the project say about this?
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH v2 1/6] submodules: add helper functions to determine presence of submodules
From: Junio C Hamano @ 2016-11-01 17:20 UTC (permalink / raw)
To: Stefan Beller; +Cc: Brandon Williams, git@vger.kernel.org
In-Reply-To: <CAGZ79kamzSPyM65k9ugS0dAJCfGnGvk3m2p+XtCEozCvoZ5+OA@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
Overall the suggestions from you in this review is good and please
consider anything I did not mention I agree with you. Thanks.
>> +extern int is_submodule_initialized(const char *path);
>> +extern int is_submodule_checked_out(const char *path);
>
> no need to put extern for function names. (no other functions in this
> header are extern. so local consistency maybe? I'd also claim that
> all other extern functions in headers ought to be declared without
> being extern)
Maybe I am old fashioned, but I'd feel better to see these with
explicit "extern" in front (check the older header files like
cache.h when you are in doubt what the project convention has been).
^ 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