* 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 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 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
* [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
* [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 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 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 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 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
* 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
* 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: [ANNOUNCE] Git v2.11.0-rc0
From: Jeff King @ 2016-11-01 20:36 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Michael Haggerty, Stefan Beller, Jacob Keller, git
In-Reply-To: <xmqq1sywrxxl.fsf@gitster.mtv.corp.google.com>
On Mon, Oct 31, 2016 at 02:49:42PM -0700, Junio C Hamano wrote:
> * Output from "git diff" can be made easier to read by selecting
> which lines are common and which lines are added/deleted
> intelligently when the lines before and after the changed section
> are the same. A command line option is added to help with the
> experiment to find a good heuristics.
I'm not sure we ever resolved all the discussion around options and
defaults here.
As of -rc0, we have both --indent-heuristic and --compaction-heuristic
(along with matching config), and they are mutually exclusive.
In [1], Stefan suggested just replacing the compaction heuristic
entirely with the new one (and you seemed to agree). If we're going to
do that, it makes sense to do so before the release, so that we don't
get stuck supporting --indent-heuristic forever.
We also discussed making this the default (e.g., in [2]). Did we want to
do that before the release? If so, it would have been nice to flip it on
during -rc0 to get more exposure, but perhaps -rc1 would not be too
late. That's less critical than resolving the indent/compaction thing,
though it potentially means we could rip out the config entirely and
just leave the command-line option (as an escape hatch and debugging
tool).
-Peff
[1] http://public-inbox.org/git/CAGZ79kaMSkRfkBng_Epq+2T_q--VkKQ6-m=M_jPkzeYcxuDKWA@mail.gmail.com/
[2] http://public-inbox.org/git/CA+P7+xrsz27Hhk12dwDrUEpn0L7R8F5z-XASS2JkoY6sqK7u5A@mail.gmail.com/
^ permalink raw reply
* Re: [PATCH v2 1/5] trailer: be stricter in parsing separators
From: Junio C Hamano @ 2016-11-01 20:37 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, christian.couder
In-Reply-To: <xmqqvaw7ndow.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> 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.
Other than this "Hmph" (which is not an objection---just something
that the reviewer did not understand), the rest looked good to me.
Will re-queue.
Thanks.
^ permalink raw reply
* Re: [ANNOUNCE] Git v2.11.0-rc0
From: Stefan Beller @ 2016-11-01 20:43 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git@vger.kernel.org, Linux Kernel
In-Reply-To: <xmqq1sywrxxl.fsf@gitster.mtv.corp.google.com>
> * "git clone --resurse-submodules --reference $path $URL" is a way to
"recurse"
^ permalink raw reply
* Re: Git issue
From: Philip Oakley @ 2016-11-01 20:50 UTC (permalink / raw)
To: Jeff King, Halde, Faiz; +Cc: git
In-Reply-To: <20161101174526.e2tilsriz2fqaru3@sigill.intra.peff.net>
From: "Jeff King" <peff@peff.net>
> 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.
True. However...
>
> 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.
>
The whole section (including the ones above this quote) are often confused
between the promises of the user, and the alleged promises of Git. Even in
the quote above the "Instead" probably shouldn't be there.
Given the number of misrepresentations (on the web) of what the bit does,
and the ongoing misunderstandings of users it does feel like the man page
article could be refreshed to be more assertive about the users promise, and
Git's cautions.
My quick rough working on a more assertive update..
-- >8 --
----------------------
Documentation/git-update-index.txt ----------------------
index 7386c93..4ec1711 100644
@@ -84,12 +84,12 @@ OPTIONS
Set the execute permissions on the updated files.
--[no-]assume-unchanged::
When this flag is specified, the object names recorded
- for the paths are not updated. Instead, this option
+ for the paths are not updated. 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
+ *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
@@ -300,19 +300,25 @@ $ git ls-files -s
Using ``assume unchanged'' bit
------------------------------
-Many operations in Git depend on your filesystem to have an
+Many operations in Git depend on your filesystem having a fast and
efficient `lstat(2)` implementation, so that `st_mtime`
information for working tree files can be cheaply checked to see
if the file contents have changed from the version recorded in
the index file. Unfortunately, some filesystems have
inefficient `lstat(2)`. If your filesystem is one of them, you
-can set "assume unchanged" bit to paths you have not changed to
-cause Git not to do this check. Note that setting this bit on a
-path does not mean Git will check the contents of the file to
-see if it has changed -- it makes Git to omit any checking and
-assume it has *not* changed. When you make changes to working
+can set "assume unchanged" bit to *paths you have not changed* to
+cause Git not to do this check.
+
+Note that setting this bit on a
+path does not mean Git will never check the contents of the file to
+see if it has changed. Though normally it makes Git to omit any checking to
+assume it has not changed.
+Commands which may overwrite local changes (pull/merge etc) are
+likely to check if the contents have changed
+
+If you make desired changes to working
tree files, you have to explicitly tell Git about it by dropping
"assume unchanged" bit, either before or after you modify them.
In order to set "assume unchanged" bit, use `--assume-unchanged`
---
Philip
^ permalink raw reply
* Re: [PATCH v2 1/5] trailer: be stricter in parsing separators
From: Jonathan Tan @ 2016-11-01 20:53 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, christian.couder
In-Reply-To: <xmqqr36vndgn.fsf@gitster.mtv.corp.google.com>
On 11/01/2016 01:37 PM, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> 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.
This is to allow trailers of the form "Fix #42" (mentioned in the
git-interpret-trailers documentation and also in the test).
> Other than this "Hmph" (which is not an objection---just something
> that the reviewer did not understand), the rest looked good to me.
>
> Will re-queue.
>
> Thanks.
Thanks!
^ permalink raw reply
* Re: [ANNOUNCE] Git v2.11.0-rc0
From: Junio C Hamano @ 2016-11-01 20:56 UTC (permalink / raw)
To: Jeff King; +Cc: Michael Haggerty, Stefan Beller, Jacob Keller, git
In-Reply-To: <20161101203637.3jr73wwpfal4brho@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Mon, Oct 31, 2016 at 02:49:42PM -0700, Junio C Hamano wrote:
>
>> * Output from "git diff" can be made easier to read by selecting
>> which lines are common and which lines are added/deleted
>> intelligently when the lines before and after the changed section
>> are the same. A command line option is added to help with the
>> experiment to find a good heuristics.
>
> I'm not sure we ever resolved all the discussion around options and
> defaults here.
>
> As of -rc0, we have both --indent-heuristic and --compaction-heuristic
> (along with matching config), and they are mutually exclusive.
>
> In [1], Stefan suggested just replacing the compaction heuristic
> entirely with the new one (and you seemed to agree). If we're going to
> do that, it makes sense to do so before the release, so that we don't
> get stuck supporting --indent-heuristic forever.
You meant "compaction" in the last part? I think it is probably a
good idea.
I'd vote for just removing compaction-heuristic while keeping the
indent-heuristic with experimental label and knobs and keeping it
off by default for a while.
Thanks for bringing it up.
^ permalink raw reply
* Re: [ANNOUNCE] Git v2.11.0-rc0
From: Jeff King @ 2016-11-01 20:59 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Michael Haggerty, Stefan Beller, Jacob Keller, git
In-Reply-To: <xmqqlgx2or5p.fsf@gitster.mtv.corp.google.com>
On Tue, Nov 01, 2016 at 01:56:34PM -0700, Junio C Hamano wrote:
> > As of -rc0, we have both --indent-heuristic and --compaction-heuristic
> > (along with matching config), and they are mutually exclusive.
> >
> > In [1], Stefan suggested just replacing the compaction heuristic
> > entirely with the new one (and you seemed to agree). If we're going to
> > do that, it makes sense to do so before the release, so that we don't
> > get stuck supporting --indent-heuristic forever.
>
> You meant "compaction" in the last part? I think it is probably a
> good idea.
I thought the plan mentioned in the mail I linked was to keep the indent
heuristic, but simply _call_ it the compaction heuristic. IOW, to swap
out the implementation under the hood for something we know is better.
We've already released a version with --compaction-heuristic, so we are
stuck keeping it forever either way.
> I'd vote for just removing compaction-heuristic while keeping the
> indent-heuristic with experimental label and knobs and keeping it
> off by default for a while.
So the matching variant of that plan would be to drop the internals of
compaction-heuristic, swap in the new heuristic instead, and then drop
all of the --indent-heuristic. It remains off by default, but we may
flip that in a later release.
-Peff
^ permalink raw reply
* Re: Git issue
From: Jeff King @ 2016-11-01 21:03 UTC (permalink / raw)
To: Philip Oakley; +Cc: Halde, Faiz, git
In-Reply-To: <7CE3166CFD244DAABF554451E8B0800F@PhilipOakley>
On Tue, Nov 01, 2016 at 08:50:23PM -0000, Philip Oakley wrote:
> > 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.
> >
>
> The whole section (including the ones above this quote) are often confused
> between the promises of the user, and the alleged promises of Git. Even in
> the quote above the "Instead" probably shouldn't be there.
I think the "Instead" is "we are not doing the usual update-index thing
of reading the new data from disk; instead, we are _just_ setting the
bit". Perhaps that can be spelled out more clearly, but I think just
dropping "Instead" is a step backwards.
> Given the number of misrepresentations (on the web) of what the bit does,
> and the ongoing misunderstandings of users it does feel like the man page
> article could be refreshed to be more assertive about the users promise, and
> Git's cautions.
I dunno. I know this has long been a source of confusion, but I
specifically dug in the docs to see what we had, and I thought what I
quoted above was pretty clear. That has "only" been around for about 2
years, and is fighting against other mis-advice on the Internet, though.
So I'm not sure if it is badly worded, or if people simply do not see
it.
-Peff
^ permalink raw reply
* [PATCH] doc: update-index --assume-unchanged
From: Philip Oakley @ 2016-11-01 21:04 UTC (permalink / raw)
To: git; +Cc: philipoakley, fhalde, peff, gitster
In-Reply-To: <7CE3166CFD244DAABF554451E8B0800F@PhilipOakley>
Be more assertive about the User promise, and Git's defensiveness
of file changes.
Signed-off-by: Philip Oakley <philipoakley@iee.org>
---
Here is a full patch for
https://public-inbox.org/git/7CE3166CFD244DAABF554451E8B0800F@PhilipOakley/
---
Documentation/git-update-index.txt | 22 ++++++++++++++--------
1 file changed, 14 insertions(+), 8 deletions(-)
diff --git a/Documentation/git-update-index.txt b/Documentation/git-update-index.txt
index 7386c93..4ec1711 100644
--- a/Documentation/git-update-index.txt
+++ b/Documentation/git-update-index.txt
@@ -85,10 +85,10 @@ OPTIONS
--[no-]assume-unchanged::
When this flag is specified, the object names recorded
- for the paths are not updated. Instead, this option
+ for the paths are not updated. 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
+ *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
@@ -301,17 +301,23 @@ $ git ls-files -s
Using ``assume unchanged'' bit
------------------------------
-Many operations in Git depend on your filesystem to have an
+Many operations in Git depend on your filesystem having a fast and
efficient `lstat(2)` implementation, so that `st_mtime`
information for working tree files can be cheaply checked to see
if the file contents have changed from the version recorded in
the index file. Unfortunately, some filesystems have
inefficient `lstat(2)`. If your filesystem is one of them, you
-can set "assume unchanged" bit to paths you have not changed to
-cause Git not to do this check. Note that setting this bit on a
-path does not mean Git will check the contents of the file to
-see if it has changed -- it makes Git to omit any checking and
-assume it has *not* changed. When you make changes to working
+can set "assume unchanged" bit to *paths you have not changed* to
+cause Git not to do this check.
+
+Note that setting this bit on a
+path does not mean Git will never check the contents of the file to
+see if it has changed. Though normally it makes Git to omit any checking to
+assume it has not changed.
+Commands which may overwrite local changes (pull/merge etc) are
+likely to check if the contents have changed
+
+If you make desired changes to working
tree files, you have to explicitly tell Git about it by dropping
"assume unchanged" bit, either before or after you modify them.
--
2.9.0.windows.1.323.g0305acf
^ permalink raw reply related
* Re: [ANNOUNCE] Git v2.11.0-rc0
From: Junio C Hamano @ 2016-11-01 21:05 UTC (permalink / raw)
To: Jeff King; +Cc: Michael Haggerty, Stefan Beller, Jacob Keller, git
In-Reply-To: <20161101205916.d74n6lhgp2hexpzr@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> We've already released a version with --compaction-heuristic, so we are
> stuck keeping it forever either way.
While I do not necessarily agree with this conclusion (we've already
released it labelled as experimental; can't we freely take it
away?), that may be of lessor impact. I like it better.
If it involves renaming and swapping, however, we may be talking
about a change that we cannot deliver with confidence in less than a
week before -rc1, which sounds, eh, suboptimal.
FWIW, here is how the removal of compaction without renaming looks
like.
Documentation/diff-heuristic-options.txt | 2 --
builtin/blame.c | 3 +--
diff.c | 23 +++--------------------
git-add--interactive.perl | 3 ---
xdiff/xdiff.h | 3 +--
xdiff/xdiffi.c | 15 ---------------
6 files changed, 5 insertions(+), 44 deletions(-)
diff --git a/Documentation/diff-heuristic-options.txt b/Documentation/diff-heuristic-options.txt
index 36cb549df9..d4f3d95505 100644
--- a/Documentation/diff-heuristic-options.txt
+++ b/Documentation/diff-heuristic-options.txt
@@ -1,7 +1,5 @@
--indent-heuristic::
--no-indent-heuristic::
---compaction-heuristic::
---no-compaction-heuristic::
These are to help debugging and tuning experimental heuristics
(which are off by default) that shift diff hunk boundaries to
make patches easier to read.
diff --git a/builtin/blame.c b/builtin/blame.c
index 4ddfadb71f..f45416a6c3 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2597,7 +2597,6 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
* output:
*/
{ OPTION_LOWLEVEL_CALLBACK, 0, "indent-heuristic", NULL, NULL, N_("Use an experimental indent-based heuristic to improve diffs"), PARSE_OPT_NOARG, parse_opt_unknown_cb },
- { OPTION_LOWLEVEL_CALLBACK, 0, "compaction-heuristic", NULL, NULL, N_("Use an experimental blank-line-based heuristic to improve diffs"), PARSE_OPT_NOARG, parse_opt_unknown_cb },
OPT_BIT(0, "minimal", &xdl_opts, N_("Spend extra cycles to find better match"), XDF_NEED_MINIMAL),
OPT_STRING('S', NULL, &revs_file, N_("file"), N_("Use revisions from <file> instead of calling git-rev-list")),
@@ -2645,7 +2644,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
}
parse_done:
no_whole_file_rename = !DIFF_OPT_TST(&revs.diffopt, FOLLOW_RENAMES);
- xdl_opts |= revs.diffopt.xdl_opts & (XDF_COMPACTION_HEURISTIC | XDF_INDENT_HEURISTIC);
+ xdl_opts |= revs.diffopt.xdl_opts & XDF_INDENT_HEURISTIC;
DIFF_OPT_CLR(&revs.diffopt, FOLLOW_RENAMES);
argc = parse_options_end(&ctx);
diff --git a/diff.c b/diff.c
index 8981477c43..741ce8c68d 100644
--- a/diff.c
+++ b/diff.c
@@ -28,7 +28,6 @@
static int diff_detect_rename_default;
static int diff_indent_heuristic; /* experimental */
-static int diff_compaction_heuristic; /* experimental */
static int diff_rename_limit_default = 400;
static int diff_suppress_blank_empty;
static int diff_use_color_default = -1;
@@ -223,16 +222,8 @@ void init_diff_ui_defaults(void)
int git_diff_heuristic_config(const char *var, const char *value, void *cb)
{
- if (!strcmp(var, "diff.indentheuristic")) {
+ if (!strcmp(var, "diff.indentheuristic"))
diff_indent_heuristic = git_config_bool(var, value);
- if (diff_indent_heuristic)
- diff_compaction_heuristic = 0;
- }
- if (!strcmp(var, "diff.compactionheuristic")) {
- diff_compaction_heuristic = git_config_bool(var, value);
- if (diff_compaction_heuristic)
- diff_indent_heuristic = 0;
- }
return 0;
}
@@ -3380,8 +3371,6 @@ void diff_setup(struct diff_options *options)
options->xdl_opts |= diff_algorithm;
if (diff_indent_heuristic)
DIFF_XDL_SET(options, INDENT_HEURISTIC);
- else if (diff_compaction_heuristic)
- DIFF_XDL_SET(options, COMPACTION_HEURISTIC);
options->orderfile = diff_order_file_cfg;
@@ -3876,16 +3865,10 @@ int diff_opt_parse(struct diff_options *options,
DIFF_XDL_SET(options, IGNORE_WHITESPACE_AT_EOL);
else if (!strcmp(arg, "--ignore-blank-lines"))
DIFF_XDL_SET(options, IGNORE_BLANK_LINES);
- else if (!strcmp(arg, "--indent-heuristic")) {
+ else if (!strcmp(arg, "--indent-heuristic"))
DIFF_XDL_SET(options, INDENT_HEURISTIC);
- DIFF_XDL_CLR(options, COMPACTION_HEURISTIC);
- } else if (!strcmp(arg, "--no-indent-heuristic"))
- DIFF_XDL_CLR(options, INDENT_HEURISTIC);
- else if (!strcmp(arg, "--compaction-heuristic")) {
- DIFF_XDL_SET(options, COMPACTION_HEURISTIC);
+ else if (!strcmp(arg, "--no-indent-heuristic"))
DIFF_XDL_CLR(options, INDENT_HEURISTIC);
- } else if (!strcmp(arg, "--no-compaction-heuristic"))
- DIFF_XDL_CLR(options, COMPACTION_HEURISTIC);
else if (!strcmp(arg, "--patience"))
options->xdl_opts = DIFF_WITH_ALG(options, PATIENCE_DIFF);
else if (!strcmp(arg, "--histogram"))
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index ee3d812695..5a55d80b9d 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -46,7 +46,6 @@
my $diff_algorithm = $repo->config('diff.algorithm');
my $diff_indent_heuristic = $repo->config_bool('diff.indentheuristic');
-my $diff_compaction_heuristic = $repo->config_bool('diff.compactionheuristic');
my $diff_filter = $repo->config('interactive.difffilter');
my $use_readkey = 0;
@@ -753,8 +752,6 @@ sub parse_diff {
}
if ($diff_indent_heuristic) {
splice @diff_cmd, 1, 0, "--indent-heuristic";
- } elsif ($diff_compaction_heuristic) {
- splice @diff_cmd, 1, 0, "--compaction-heuristic";
}
if (defined $patch_mode_revision) {
push @diff_cmd, get_diff_reference($patch_mode_revision);
diff --git a/xdiff/xdiff.h b/xdiff/xdiff.h
index 8db16d4ae6..b090ad8eac 100644
--- a/xdiff/xdiff.h
+++ b/xdiff/xdiff.h
@@ -41,8 +41,7 @@ extern "C" {
#define XDF_IGNORE_BLANK_LINES (1 << 7)
-#define XDF_COMPACTION_HEURISTIC (1 << 8)
-#define XDF_INDENT_HEURISTIC (1 << 9)
+#define XDF_INDENT_HEURISTIC (1 << 8)
#define XDL_EMIT_FUNCNAMES (1 << 0)
#define XDL_EMIT_FUNCCONTEXT (1 << 2)
diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c
index 760fbb6db7..f9be87fdfb 100644
--- a/xdiff/xdiffi.c
+++ b/xdiff/xdiffi.c
@@ -906,21 +906,6 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {
if (group_previous(xdfo, &go))
xdl_bug("group sync broken sliding to match");
}
- } else if ((flags & XDF_COMPACTION_HEURISTIC) && blank_lines) {
- /*
- * Compaction heuristic: if it is possible to shift the
- * group to make its bottom line a blank line, do so.
- *
- * As we already shifted the group forward as far as
- * possible in the earlier loop, we only need to handle
- * backward shifts, not forward ones.
- */
- while (!is_blank_line(xdf->recs[g.end - 1], flags)) {
- if (group_slide_up(xdf, &g, flags))
- xdl_bug("blank line disappeared");
- if (group_previous(xdfo, &go))
- xdl_bug("group sync broken sliding to blank line");
- }
} else if (flags & XDF_INDENT_HEURISTIC) {
/*
* Indent heuristic: a group of pure add/delete lines
^ permalink raw reply related
* Re: [ANNOUNCE] Git v2.11.0-rc0
From: Stefan Beller @ 2016-11-01 21:10 UTC (permalink / raw)
To: Jeff King
Cc: Junio C Hamano, Michael Haggerty, Jacob Keller,
git@vger.kernel.org
In-Reply-To: <20161101205916.d74n6lhgp2hexpzr@sigill.intra.peff.net>
On Tue, Nov 1, 2016 at 1:59 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Nov 01, 2016 at 01:56:34PM -0700, Junio C Hamano wrote:
>
>> > As of -rc0, we have both --indent-heuristic and --compaction-heuristic
>> > (along with matching config), and they are mutually exclusive.
>> >
>> > In [1], Stefan suggested just replacing the compaction heuristic
>> > entirely with the new one (and you seemed to agree). If we're going to
>> > do that, it makes sense to do so before the release, so that we don't
>> > get stuck supporting --indent-heuristic forever.
>>
>> You meant "compaction" in the last part? I think it is probably a
>> good idea.
>
> I thought the plan mentioned in the mail I linked was to keep the indent
> heuristic, but simply _call_ it the compaction heuristic. IOW, to swap
> out the implementation under the hood for something we know is better.
AFAICT Michaels design is better in every aspect than what I did initially,
so it supersedes the work I did there. I would support the swap in names.
>
> We've already released a version with --compaction-heuristic, so we are
> stuck keeping it forever either way.
IIRC the release notes specifically noted this flag to be experimental and
may be removed in future versions.
When not doing the swap of the implementation, but rather remove the
experimental feature of compaction-heuristic and introducing a *new*
experimental --indent-heuristic, this may drive the point across that
these names are actually experimental.
^ permalink raw reply
* Re: Git issue
From: Junio C Hamano @ 2016-11-01 21:23 UTC (permalink / raw)
To: Jeff King; +Cc: Philip Oakley, Halde, Faiz, git
In-Reply-To: <20161101210310.sqrhvviry7iyyjrm@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Tue, Nov 01, 2016 at 08:50:23PM -0000, Philip Oakley wrote:
>
>> > 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. ...
>
> I think the "Instead" is "we are not doing the usual update-index thing
> of reading the new data from disk; instead, we are _just_ setting the
> bit". Perhaps that can be spelled out more clearly, but I think just
> dropping "Instead" is a step backwards.
I tend to agree; the biggest problem with this part of the
description I think is that it starts by saying what it does not do,
which may help people who expect the command to do its usual thing
but otherwise is a secondary information. How about ripping it out
and moving it after the primary description, i.e.
Set or unset the "assume unchanged" bit for the paths,
without changing the object names recorded for them, in the
index.
>> Given the number of misrepresentations (on the web) of what the bit does,
>> and the ongoing misunderstandings of users it does feel like the man page
>> article could be refreshed to be more assertive about the users promise, and
>> Git's cautions.
>
> I dunno. I know this has long been a source of confusion, but I
> specifically dug in the docs to see what we had, and I thought what I
> quoted above was pretty clear. That has "only" been around for about 2
> years, and is fighting against other mis-advice on the Internet, though.
> So I'm not sure if it is badly worded, or if people simply do not see
> it.
I share the same reaction, and I find that the update in ccadb25f73
("doc: make clear --assume-unchanged's user contract", 2014-12-06)
that introduced the "the user promises not to change and allows Git
to assume" is sufficiently clear.
^ permalink raw reply
* Re: [PATCH v2 1/5] trailer: be stricter in parsing separators
From: Junio C Hamano @ 2016-11-01 21:26 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, christian.couder
In-Reply-To: <0b3e70fe-5e1f-7aaa-4c3d-88f30cb15008@google.com>
Jonathan Tan <jonathantanmy@google.com> writes:
>>> 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.
>
> This is to allow trailers of the form "Fix #42" (mentioned in the
> git-interpret-trailers documentation and also in the test).
That piece of information belongs to the in-code comment for the
function, I would think. The comment that describes what it does
(i.e. "token with optional spaces and separator") can be read from
the code easily, but the reason why the code implements that
behaviour cannot.
Thanks.
^ permalink raw reply
* Re: [ANNOUNCE] Git v2.11.0-rc0
From: Junio C Hamano @ 2016-11-01 21:38 UTC (permalink / raw)
To: Jeff King; +Cc: Michael Haggerty, Stefan Beller, Jacob Keller, git
In-Reply-To: <xmqqh97qoqq2.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> If it involves renaming and swapping, however, we may be talking
> about a change that we cannot deliver with confidence in less than a
> week before -rc1, which sounds, eh, suboptimal.
>
> FWIW, here is how the removal of compaction without renaming looks
> like.
And here is _with_ renaming. I think "compaction heuristics" is a
much better phrase to call "heuristics used during the diff hunk
compaction process" without specifying how that heuristics work
(like taking hints in the indentation levels). If we are to retire
one while keeping the other, compaction-heuristics should be the
name we give and keep for the surviving one.
I have not much confidence in the conversion result, though.
Documentation/diff-config.txt | 1 -
Documentation/diff-heuristic-options.txt | 2 --
builtin/blame.c | 3 +--
diff.c | 25 ++++-----------------
git-add--interactive.perl | 5 +----
t/t4061-diff-indent.sh | 38 ++++++++++++++++----------------
xdiff/xdiff.h | 1 -
xdiff/xdiffi.c | 17 +-------------
8 files changed, 26 insertions(+), 66 deletions(-)
diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
index 58f4bd6afa..0c79e48d9d 100644
--- a/Documentation/diff-config.txt
+++ b/Documentation/diff-config.txt
@@ -171,7 +171,6 @@ diff.tool::
include::mergetools-diff.txt[]
-diff.indentHeuristic::
diff.compactionHeuristic::
Set one of these options to `true` to enable one of two
experimental heuristics that shift diff hunk boundaries to
diff --git a/Documentation/diff-heuristic-options.txt b/Documentation/diff-heuristic-options.txt
index 36cb549df9..3cb024aa22 100644
--- a/Documentation/diff-heuristic-options.txt
+++ b/Documentation/diff-heuristic-options.txt
@@ -1,5 +1,3 @@
---indent-heuristic::
---no-indent-heuristic::
--compaction-heuristic::
--no-compaction-heuristic::
These are to help debugging and tuning experimental heuristics
diff --git a/builtin/blame.c b/builtin/blame.c
index 4ddfadb71f..395d4011fb 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2596,7 +2596,6 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
* and are only included here to get included in the "-h"
* output:
*/
- { OPTION_LOWLEVEL_CALLBACK, 0, "indent-heuristic", NULL, NULL, N_("Use an experimental indent-based heuristic to improve diffs"), PARSE_OPT_NOARG, parse_opt_unknown_cb },
{ OPTION_LOWLEVEL_CALLBACK, 0, "compaction-heuristic", NULL, NULL, N_("Use an experimental blank-line-based heuristic to improve diffs"), PARSE_OPT_NOARG, parse_opt_unknown_cb },
OPT_BIT(0, "minimal", &xdl_opts, N_("Spend extra cycles to find better match"), XDF_NEED_MINIMAL),
@@ -2645,7 +2644,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
}
parse_done:
no_whole_file_rename = !DIFF_OPT_TST(&revs.diffopt, FOLLOW_RENAMES);
- xdl_opts |= revs.diffopt.xdl_opts & (XDF_COMPACTION_HEURISTIC | XDF_INDENT_HEURISTIC);
+ xdl_opts |= revs.diffopt.xdl_opts & XDF_COMPACTION_HEURISTIC;
DIFF_OPT_CLR(&revs.diffopt, FOLLOW_RENAMES);
argc = parse_options_end(&ctx);
diff --git a/diff.c b/diff.c
index 8981477c43..f1b01f5b1e 100644
--- a/diff.c
+++ b/diff.c
@@ -27,7 +27,6 @@
#endif
static int diff_detect_rename_default;
-static int diff_indent_heuristic; /* experimental */
static int diff_compaction_heuristic; /* experimental */
static int diff_rename_limit_default = 400;
static int diff_suppress_blank_empty;
@@ -223,16 +222,8 @@ void init_diff_ui_defaults(void)
int git_diff_heuristic_config(const char *var, const char *value, void *cb)
{
- if (!strcmp(var, "diff.indentheuristic")) {
- diff_indent_heuristic = git_config_bool(var, value);
- if (diff_indent_heuristic)
- diff_compaction_heuristic = 0;
- }
- if (!strcmp(var, "diff.compactionheuristic")) {
+ if (!strcmp(var, "diff.compactionheuristic"))
diff_compaction_heuristic = git_config_bool(var, value);
- if (diff_compaction_heuristic)
- diff_indent_heuristic = 0;
- }
return 0;
}
@@ -3378,9 +3369,7 @@ void diff_setup(struct diff_options *options)
options->use_color = diff_use_color_default;
options->detect_rename = diff_detect_rename_default;
options->xdl_opts |= diff_algorithm;
- if (diff_indent_heuristic)
- DIFF_XDL_SET(options, INDENT_HEURISTIC);
- else if (diff_compaction_heuristic)
+ if (diff_compaction_heuristic)
DIFF_XDL_SET(options, COMPACTION_HEURISTIC);
options->orderfile = diff_order_file_cfg;
@@ -3876,15 +3865,9 @@ int diff_opt_parse(struct diff_options *options,
DIFF_XDL_SET(options, IGNORE_WHITESPACE_AT_EOL);
else if (!strcmp(arg, "--ignore-blank-lines"))
DIFF_XDL_SET(options, IGNORE_BLANK_LINES);
- else if (!strcmp(arg, "--indent-heuristic")) {
- DIFF_XDL_SET(options, INDENT_HEURISTIC);
- DIFF_XDL_CLR(options, COMPACTION_HEURISTIC);
- } else if (!strcmp(arg, "--no-indent-heuristic"))
- DIFF_XDL_CLR(options, INDENT_HEURISTIC);
- else if (!strcmp(arg, "--compaction-heuristic")) {
+ else if (!strcmp(arg, "--compaction-heuristic"))
DIFF_XDL_SET(options, COMPACTION_HEURISTIC);
- DIFF_XDL_CLR(options, INDENT_HEURISTIC);
- } else if (!strcmp(arg, "--no-compaction-heuristic"))
+ else if (!strcmp(arg, "--no-compaction-heuristic"))
DIFF_XDL_CLR(options, COMPACTION_HEURISTIC);
else if (!strcmp(arg, "--patience"))
options->xdl_opts = DIFF_WITH_ALG(options, PATIENCE_DIFF);
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index ee3d812695..642cce1ac6 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -45,7 +45,6 @@
my $normal_color = $repo->get_color("", "reset");
my $diff_algorithm = $repo->config('diff.algorithm');
-my $diff_indent_heuristic = $repo->config_bool('diff.indentheuristic');
my $diff_compaction_heuristic = $repo->config_bool('diff.compactionheuristic');
my $diff_filter = $repo->config('interactive.difffilter');
@@ -751,9 +750,7 @@ sub parse_diff {
if (defined $diff_algorithm) {
splice @diff_cmd, 1, 0, "--diff-algorithm=${diff_algorithm}";
}
- if ($diff_indent_heuristic) {
- splice @diff_cmd, 1, 0, "--indent-heuristic";
- } elsif ($diff_compaction_heuristic) {
+ if ($diff_compaction_heuristic) {
splice @diff_cmd, 1, 0, "--compaction-heuristic";
}
if (defined $patch_mode_revision) {
diff --git a/t/t4061-diff-indent.sh b/t/t4061-diff-indent.sh
index 556450609b..30f809d0d3 100755
--- a/t/t4061-diff-indent.sh
+++ b/t/t4061-diff-indent.sh
@@ -1,6 +1,6 @@
#!/bin/sh
-test_description='Test diff indent heuristic.
+test_description='Test diff compaction heuristic.
'
. ./test-lib.sh
@@ -157,28 +157,28 @@ test_expect_success 'diff: ugly spaces' '
compare_diff spaces-expect out
'
-test_expect_success 'diff: nice spaces with --indent-heuristic' '
- git diff --indent-heuristic old new -- spaces.txt >out-compacted &&
+test_expect_success 'diff: nice spaces with --compaction-heuristic' '
+ git diff --compaction-heuristic old new -- spaces.txt >out-compacted &&
compare_diff spaces-compacted-expect out-compacted
'
-test_expect_success 'diff: nice spaces with diff.indentHeuristic' '
- git -c diff.indentHeuristic=true diff old new -- spaces.txt >out-compacted2 &&
+test_expect_success 'diff: nice spaces with diff.compactionHeuristic' '
+ git -c diff.compactionHeuristic=true diff old new -- spaces.txt >out-compacted2 &&
compare_diff spaces-compacted-expect out-compacted2
'
-test_expect_success 'diff: --no-indent-heuristic overrides config' '
- git -c diff.indentHeuristic=true diff --no-indent-heuristic old new -- spaces.txt >out2 &&
+test_expect_success 'diff: --no-compaction-heuristic overrides config' '
+ git -c diff.compactionHeuristic=true diff --no-compaction-heuristic old new -- spaces.txt >out2 &&
compare_diff spaces-expect out2
'
-test_expect_success 'diff: --indent-heuristic with --patience' '
- git diff --indent-heuristic --patience old new -- spaces.txt >out-compacted3 &&
+test_expect_success 'diff: --compaction-heuristic with --patience' '
+ git diff --compaction-heuristic --patience old new -- spaces.txt >out-compacted3 &&
compare_diff spaces-compacted-expect out-compacted3
'
-test_expect_success 'diff: --indent-heuristic with --histogram' '
- git diff --indent-heuristic --histogram old new -- spaces.txt >out-compacted4 &&
+test_expect_success 'diff: --compaction-heuristic with --histogram' '
+ git diff --compaction-heuristic --histogram old new -- spaces.txt >out-compacted4 &&
compare_diff spaces-compacted-expect out-compacted4
'
@@ -187,8 +187,8 @@ test_expect_success 'diff: ugly functions' '
compare_diff functions-expect out
'
-test_expect_success 'diff: nice functions with --indent-heuristic' '
- git diff --indent-heuristic old new -- functions.c >out-compacted &&
+test_expect_success 'diff: nice functions with --compaction-heuristic' '
+ git diff --compaction-heuristic old new -- functions.c >out-compacted &&
compare_diff functions-compacted-expect out-compacted
'
@@ -197,18 +197,18 @@ test_expect_success 'blame: ugly spaces' '
compare_blame spaces-expect out-blame
'
-test_expect_success 'blame: nice spaces with --indent-heuristic' '
- git blame --indent-heuristic old..new -- spaces.txt >out-blame-compacted &&
+test_expect_success 'blame: nice spaces with --compaction-heuristic' '
+ git blame --compaction-heuristic old..new -- spaces.txt >out-blame-compacted &&
compare_blame spaces-compacted-expect out-blame-compacted
'
-test_expect_success 'blame: nice spaces with diff.indentHeuristic' '
- git -c diff.indentHeuristic=true blame old..new -- spaces.txt >out-blame-compacted2 &&
+test_expect_success 'blame: nice spaces with diff.compactionHeuristic' '
+ git -c diff.compactionHeuristic=true blame old..new -- spaces.txt >out-blame-compacted2 &&
compare_blame spaces-compacted-expect out-blame-compacted2
'
-test_expect_success 'blame: --no-indent-heuristic overrides config' '
- git -c diff.indentHeuristic=true blame --no-indent-heuristic old..new -- spaces.txt >out-blame2 &&
+test_expect_success 'blame: --no-compaction-heuristic overrides config' '
+ git -c diff.compactionHeuristic=true blame --no-compaction-heuristic old..new -- spaces.txt >out-blame2 &&
git blame old..new -- spaces.txt >out-blame &&
compare_blame spaces-expect out-blame2
'
diff --git a/xdiff/xdiff.h b/xdiff/xdiff.h
index 8db16d4ae6..7423f77fc8 100644
--- a/xdiff/xdiff.h
+++ b/xdiff/xdiff.h
@@ -42,7 +42,6 @@ extern "C" {
#define XDF_IGNORE_BLANK_LINES (1 << 7)
#define XDF_COMPACTION_HEURISTIC (1 << 8)
-#define XDF_INDENT_HEURISTIC (1 << 9)
#define XDL_EMIT_FUNCNAMES (1 << 0)
#define XDL_EMIT_FUNCCONTEXT (1 << 2)
diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c
index 760fbb6db7..6cb96219cb 100644
--- a/xdiff/xdiffi.c
+++ b/xdiff/xdiffi.c
@@ -906,22 +906,7 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {
if (group_previous(xdfo, &go))
xdl_bug("group sync broken sliding to match");
}
- } else if ((flags & XDF_COMPACTION_HEURISTIC) && blank_lines) {
- /*
- * Compaction heuristic: if it is possible to shift the
- * group to make its bottom line a blank line, do so.
- *
- * As we already shifted the group forward as far as
- * possible in the earlier loop, we only need to handle
- * backward shifts, not forward ones.
- */
- while (!is_blank_line(xdf->recs[g.end - 1], flags)) {
- if (group_slide_up(xdf, &g, flags))
- xdl_bug("blank line disappeared");
- if (group_previous(xdfo, &go))
- xdl_bug("group sync broken sliding to blank line");
- }
- } else if (flags & XDF_INDENT_HEURISTIC) {
+ } else if (flags & XDF_COMPACTION_HEURISTIC) {
/*
* Indent heuristic: a group of pure add/delete lines
* implies two splits, one between the end of the "before"
^ permalink raw reply related
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