[PATCH v2] sha1-name.c: for ":/", find detached HEAD commits
This patch broadens the set of commits matched by ":/" pathspecs to include commits reachable from HEAD but not any named ref. This avoids surprising behavior when working with a detached HEAD and trying to refer to a commit that was recently created and only exists within the detached state. If multiple worktrees exist, only the current worktree's HEAD is considered reachable. This is consistent with the existing behavior for other per-worktree refs: e.g., bisect refs are considered reachable, but only within the relevant worktree. Signed-off-by: Jeff King Signed-off-by: William Chargin Based-on-patch-by: Jeff King --- Documentation/revisions.txt | 2 +- sha1-name.c | 1 + t/t4208-log-magic-pathspec.sh | 26 ++ 3 files changed, 28 insertions(+), 1 deletion(-) diff --git a/Documentation/revisions.txt b/Documentation/revisions.txt index 7d1bd4409..aa9579eba 100644 --- a/Documentation/revisions.txt +++ b/Documentation/revisions.txt @@ -184,7 +184,7 @@ existing tag object. A colon, followed by a slash, followed by a text, names a commit whose commit message matches the specified regular expression. This name returns the youngest matching commit which is - reachable from any ref. The regular expression can match any part of the + reachable from any ref, including HEAD. The regular expression can match any part of the commit message. To match messages starting with a string, one can use e.g. ':/^foo'. The special sequence ':/!' is reserved for modifiers to what is matched. ':/!-foo' performs a negative match, while ':/!!foo' matches a diff --git a/sha1-name.c b/sha1-name.c index 60d9ef3c7..641ca12f9 100644 --- a/sha1-name.c +++ b/sha1-name.c @@ -1650,6 +1650,7 @@ static int get_oid_with_context_1(const char *name, struct commit_list *list = NULL; for_each_ref(handle_one_ref, ); + head_ref(handle_one_ref, ); commit_list_sort_by_date(); return get_oid_oneline(name + 2, oid, list); } diff --git a/t/t4208-log-magic-pathspec.sh b/t/t4208-log-magic-pathspec.sh index 62f335b2d..4c8f3b8e1 100755 --- a/t/t4208-log-magic-pathspec.sh +++ b/t/t4208-log-magic-pathspec.sh @@ -25,6 +25,32 @@ test_expect_success '"git log :/a -- " should not be ambiguous' ' git log :/a -- ' +test_expect_success '"git log :/detached -- " should find a commit only in HEAD' ' + test_when_finished "git checkout master" && + git checkout --detach && + # Must manually call `test_tick` instead of using `test_commit`, + # because the latter additionally creates a tag, which would make + # the commit reachable not only via HEAD. + test_tick && + git commit --allow-empty -m detached && + test_tick && + git commit --allow-empty -m something-else && + git log :/detached -- +' + +test_expect_success '"git log :/detached -- " should not find an orphaned commit' ' + test_must_fail git log :/detached -- +' + +test_expect_success '"git log :/detached -- " should find HEAD only of own worktree' ' + git worktree add other-tree HEAD && + git -C other-tree checkout --detach && + test_tick && + git -C other-tree commit --allow-empty -m other-detached && + git -C other-tree log :/other-detached -- && + test_must_fail git log :/other-detached -- +' + test_expect_success '"git log -- :/a" should not be ambiguous' ' git log -- :/a ' -- 2.18.0.130.g84cda7581
Re: [GSoC] GSoC with git, week 10
Hi, On Tue, Jul 10, 2018 at 10:05 PM Alban Gruin wrote: > > Hi, > > I published a new blog post: > > https://blog.pa1ch.fr/posts/2018/07/10/en/gsoc2018-week-10.html > Nice work. Sorry for late notification, my blog is up too. https://prertik.github.io/post/week-10 Cheers, Pratik
Re: [PATCH 0/2] Fix --rebase-merges with custom commentChar
Sorry for taking so long to get back to this. Hopefully the following will be acceptable to everyone. 8< - Subject: [PATCH v2] sequencer: use configured comment character Use the configured comment character when generating comments about branches in a todo list. Failure to honor this configuration causes a failure to parse the resulting todo list. Note that the comment_line_char has already been resolved by this point, even if the user has configured the comment character to be selected automatically. Signed-off-by: Aaron Schrab --- sequencer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sequencer.c b/sequencer.c index 4034c0461b..caf91af29d 100644 --- a/sequencer.c +++ b/sequencer.c @@ -3991,7 +3991,7 @@ static int make_script_with_merges(struct pretty_print_context *pp, entry = oidmap_get(, >object.oid); if (entry) - fprintf(out, "\n# Branch %s\n", entry->string); + fprintf(out, "\n%c Branch %s\n", comment_line_char, entry->string); else fprintf(out, "\n"); -- 2.18.0.419.gfe4b301394
Re: rev-parse --show-toplevel broken during exec'ed rebase?
Sorry. Forgot to include the git versions I tested with (2.13.1, 2.17.0, 2.18.0) On Wed, Jul 11, 2018 at 7:50 PM Vitali Lovich wrote: > > Typically git rev-parse --show-toplevel prints the folder containing > the .git folder regardless what subdirectory one is in. One exception > I have found is that if one is within the context of git rebase --exec > then show-toplevel always just prints the current directory it's > running from. > > Repro (starting with cwd within git project): > > (cd xdiff; git rev-parse --show-toplevel) > ... path to git repository > > git rebase -i 18404434bf406f6a6f892ed73320c5cf9cc187dd > # Stop at some commit for editing > > (cd xdiff; git rev-parse --show-toplevel) > ... path to git repository > > git rebase 18404434bf406f6a6f892ed73320c5cf9cc187dd -x "(cd xdiff; git > > rev-parse --show-toplevel)" > ... path to git repository/xdiff !!! > > This seems like incorrect behaviour to me since it's a weird > inconsistency (even with other rebase contexts) & the documentation > says "Show the absolute path of the top-level directory." with no > caveats. > > Thanks, > Vital
rev-parse --show-toplevel broken during exec'ed rebase?
Typically git rev-parse --show-toplevel prints the folder containing the .git folder regardless what subdirectory one is in. One exception I have found is that if one is within the context of git rebase --exec then show-toplevel always just prints the current directory it's running from. Repro (starting with cwd within git project): > (cd xdiff; git rev-parse --show-toplevel) ... path to git repository > git rebase -i 18404434bf406f6a6f892ed73320c5cf9cc187dd # Stop at some commit for editing > (cd xdiff; git rev-parse --show-toplevel) ... path to git repository > git rebase 18404434bf406f6a6f892ed73320c5cf9cc187dd -x "(cd xdiff; git > rev-parse --show-toplevel)" ... path to git repository/xdiff !!! This seems like incorrect behaviour to me since it's a weird inconsistency (even with other rebase contexts) & the documentation says "Show the absolute path of the top-level directory." with no caveats. Thanks, Vital
Re: [GSoC] GSoC with git, week 10
Hi Alban, On Tue, 10 Jul 2018, Alban Gruin wrote: > I published a new blog post: > > https://blog.pa1ch.fr/posts/2018/07/10/en/gsoc2018-week-10.html Very pleasant read, and awesome news about the current state of your project! Thanks, Dscho
[PATCH v3 1/6] commit-graph: refactor preparing commit graph
Two functions in the code (1) check if the repository is configured for
commit graphs, (2) call prepare_commit_graph(), and (3) check if the
graph exists. Move (1) and (3) into prepare_commit_graph(), reducing
duplication of code.
Signed-off-by: Jonathan Tan
---
commit-graph.c | 28 +---
1 file changed, 17 insertions(+), 11 deletions(-)
diff --git a/commit-graph.c b/commit-graph.c
index 41a0133ff7..1ea701ed69 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -200,15 +200,25 @@ static void prepare_commit_graph_one(const char *obj_dir)
}
static int prepare_commit_graph_run_once = 0;
-static void prepare_commit_graph(void)
+
+/*
+ * Return 1 if commit_graph is non-NULL, and 0 otherwise.
+ *
+ * On the first invocation, this function attemps to load the commit
+ * graph if the_repository is configured to have one.
+ */
+static int prepare_commit_graph(void)
{
struct alternate_object_database *alt;
char *obj_dir;
if (prepare_commit_graph_run_once)
- return;
+ return !!commit_graph;
prepare_commit_graph_run_once = 1;
+ if (!core_commit_graph)
+ return 0;
+
obj_dir = get_object_directory();
prepare_commit_graph_one(obj_dir);
prepare_alt_odb(the_repository);
@@ -216,6 +226,7 @@ static void prepare_commit_graph(void)
!commit_graph && alt;
alt = alt->next)
prepare_commit_graph_one(alt->path);
+ return !!commit_graph;
}
static void close_commit_graph(void)
@@ -337,22 +348,17 @@ static int parse_commit_in_graph_one(struct commit_graph
*g, struct commit *item
int parse_commit_in_graph(struct commit *item)
{
- if (!core_commit_graph)
+ if (!prepare_commit_graph())
return 0;
-
- prepare_commit_graph();
- if (commit_graph)
- return parse_commit_in_graph_one(commit_graph, item);
- return 0;
+ return parse_commit_in_graph_one(commit_graph, item);
}
void load_commit_graph_info(struct commit *item)
{
uint32_t pos;
- if (!core_commit_graph)
+ if (!prepare_commit_graph())
return;
- prepare_commit_graph();
- if (commit_graph && find_commit_in_graph(item, commit_graph, ))
+ if (find_commit_in_graph(item, commit_graph, ))
fill_commit_graph_info(item, commit_graph, pos);
}
--
2.18.0.203.gfac676dfb9-goog
[PATCH v3 0/6] Object store refactoring: commit graph
This is on _both_ ds/commit-graph-fsck and sb/object-store-lookup, following Stolee's suggestion. (It also seems better to build it this way to me, since both these branches are going into "next" according to the latest What's Cooking.) Junio wrote in [1]: > I've added SQUASH??? patch at the tip of each of the above, > rebuilt 'pu' with them and pushed the result out. It seems that > Travis is happier with the result. > > Please do not forget to squash them in when/if rerolling. If there > is no need to change anything else other than squashing them, you > can tell me to which commit in your series the fix needs to be > squashed in (that would save me time to figure it out, obviously). I'm rerolling because I also need to update the last patch with the new lookup_commit() function signature that Stefan's sb/object-store-lookup introduces. I have squashed the SQUASH??? patch into the corresponding patch in this patch set. Changes from v2: - now also based on sb/object-store-lookup in addition to ds/commit-graph-fsck (I rebased ds/commit-graph-fsck onto sb-object-store-lookup, then rebased this patch set onto the result) - patches 1-5 are unchanged - patch 6: - used "PRItime" instead of "ul" when printing a timestamp (the SQUASH??? patch) - updated invocations of lookup_commit() to take a repository object [1] https://public-inbox.org/git/xmqqpnzt1myi@gitster-ct.c.googlers.com/ Jonathan Tan (6): commit-graph: refactor preparing commit graph object-store: add missing include commit-graph: add missing forward declaration commit-graph: add free_commit_graph commit-graph: store graph in struct object_store commit-graph: add repo arg to graph readers Makefile | 1 + builtin/commit-graph.c | 2 + builtin/fsck.c | 2 +- cache.h| 1 - commit-graph.c | 108 + commit-graph.h | 11 ++-- commit.c | 6 +-- config.c | 5 -- environment.c | 1 - object-store.h | 6 +++ object.c | 5 ++ ref-filter.c | 2 +- t/helper/test-repository.c | 82 t/helper/test-tool.c | 1 + t/helper/test-tool.h | 1 + t/t5318-commit-graph.sh| 35 16 files changed, 207 insertions(+), 62 deletions(-) create mode 100644 t/helper/test-repository.c -- 2.18.0.203.gfac676dfb9-goog
[PATCH v3 6/6] commit-graph: add repo arg to graph readers
Add a struct repository argument to the functions in commit-graph.h that
read the commit graph. (This commit does not affect functions that write
commit graphs.)
Because the commit graph functions can now read the commit graph of any
repository, the global variable core_commit_graph has been removed.
Instead, the config option core.commitGraph is now read on the first
time in a repository that a commit is attempted to be parsed using its
commit graph.
This commit includes a test that exercises the functionality on an
arbitrary repository that is not the_repository.
Signed-off-by: Jonathan Tan
---
Makefile | 1 +
builtin/fsck.c | 2 +-
cache.h| 1 -
commit-graph.c | 60 +++-
commit-graph.h | 7 ++--
commit.c | 6 +--
config.c | 5 ---
environment.c | 1 -
ref-filter.c | 2 +-
t/helper/test-repository.c | 82 ++
t/helper/test-tool.c | 1 +
t/helper/test-tool.h | 1 +
t/t5318-commit-graph.sh| 35
13 files changed, 162 insertions(+), 42 deletions(-)
create mode 100644 t/helper/test-repository.c
diff --git a/Makefile b/Makefile
index 0cb6590f24..bb8bd67201 100644
--- a/Makefile
+++ b/Makefile
@@ -719,6 +719,7 @@ TEST_BUILTINS_OBJS += test-prio-queue.o
TEST_BUILTINS_OBJS += test-read-cache.o
TEST_BUILTINS_OBJS += test-ref-store.o
TEST_BUILTINS_OBJS += test-regex.o
+TEST_BUILTINS_OBJS += test-repository.o
TEST_BUILTINS_OBJS += test-revision-walking.o
TEST_BUILTINS_OBJS += test-run-command.o
TEST_BUILTINS_OBJS += test-scrap-cache-tree.o
diff --git a/builtin/fsck.c b/builtin/fsck.c
index ea5e2a03e6..c96f3f4fcc 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -830,7 +830,7 @@ int cmd_fsck(int argc, const char **argv, const char
*prefix)
check_connectivity();
- if (core_commit_graph) {
+ if (!git_config_get_bool("core.commitgraph", ) && i) {
struct child_process commit_graph_verify = CHILD_PROCESS_INIT;
const char *verify_argv[] = { "commit-graph", "verify", NULL,
NULL, NULL };
diff --git a/cache.h b/cache.h
index 8b447652a7..3311f4c9e2 100644
--- a/cache.h
+++ b/cache.h
@@ -813,7 +813,6 @@ extern char *git_replace_ref_base;
extern int fsync_object_files;
extern int core_preload_index;
-extern int core_commit_graph;
extern int core_apply_sparse_checkout;
extern int precomposed_unicode;
extern int protect_hfs;
diff --git a/commit-graph.c b/commit-graph.c
index b6a76a1413..b0a55ad128 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -183,15 +183,15 @@ struct commit_graph *load_commit_graph_one(const char
*graph_file)
exit(1);
}
-static void prepare_commit_graph_one(const char *obj_dir)
+static void prepare_commit_graph_one(struct repository *r, const char *obj_dir)
{
char *graph_name;
- if (the_repository->objects->commit_graph)
+ if (r->objects->commit_graph)
return;
graph_name = get_commit_graph_filename(obj_dir);
- the_repository->objects->commit_graph =
+ r->objects->commit_graph =
load_commit_graph_one(graph_name);
FREE_AND_NULL(graph_name);
@@ -203,26 +203,34 @@ static void prepare_commit_graph_one(const char *obj_dir)
* On the first invocation, this function attemps to load the commit
* graph if the_repository is configured to have one.
*/
-static int prepare_commit_graph(void)
+static int prepare_commit_graph(struct repository *r)
{
struct alternate_object_database *alt;
char *obj_dir;
+ int config_value;
- if (the_repository->objects->commit_graph_attempted)
- return !!the_repository->objects->commit_graph;
- the_repository->objects->commit_graph_attempted = 1;
+ if (r->objects->commit_graph_attempted)
+ return !!r->objects->commit_graph;
+ r->objects->commit_graph_attempted = 1;
- if (!core_commit_graph)
+ if (repo_config_get_bool(r, "core.commitgraph", _value) ||
+ !config_value)
+ /*
+* This repository is not configured to use commit graphs, so
+* do not load one. (But report commit_graph_attempted anyway
+* so that commit graph loading is not attempted again for this
+* repository.)
+*/
return 0;
- obj_dir = get_object_directory();
- prepare_commit_graph_one(obj_dir);
- prepare_alt_odb(the_repository);
- for (alt = the_repository->objects->alt_odb_list;
-!the_repository->objects->commit_graph && alt;
+ obj_dir = r->objects->objectdir;
+ prepare_commit_graph_one(r, obj_dir);
+ prepare_alt_odb(r);
+ for (alt = r->objects->alt_odb_list;
+!r->objects->commit_graph && alt;
alt = alt->next)
-
[PATCH v3 4/6] commit-graph: add free_commit_graph
Signed-off-by: Jonathan Tan
---
builtin/commit-graph.c | 2 ++
commit-graph.c | 24 ++--
commit-graph.h | 2 ++
3 files changed, 18 insertions(+), 10 deletions(-)
diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index c7d0db5ab4..0bf0c48657 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -115,6 +115,8 @@ static int graph_read(int argc, const char **argv)
printf(" large_edges");
printf("\n");
+ free_commit_graph(graph);
+
return 0;
}
diff --git a/commit-graph.c b/commit-graph.c
index 1ea701ed69..143a587581 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -231,16 +231,8 @@ static int prepare_commit_graph(void)
static void close_commit_graph(void)
{
- if (!commit_graph)
- return;
-
- if (commit_graph->graph_fd >= 0) {
- munmap((void *)commit_graph->data, commit_graph->data_len);
- commit_graph->data = NULL;
- close(commit_graph->graph_fd);
- }
-
- FREE_AND_NULL(commit_graph);
+ free_commit_graph(commit_graph);
+ commit_graph = NULL;
}
static int bsearch_graph(struct commit_graph *g, struct object_id *oid,
uint32_t *pos)
@@ -1033,3 +1025,15 @@ int verify_commit_graph(struct repository *r, struct
commit_graph *g)
return verify_commit_graph_error;
}
+
+void free_commit_graph(struct commit_graph *g)
+{
+ if (!g)
+ return;
+ if (g->graph_fd >= 0) {
+ munmap((void *)g->data, g->data_len);
+ g->data = NULL;
+ close(g->graph_fd);
+ }
+ free(g);
+}
diff --git a/commit-graph.h b/commit-graph.h
index 674052bef4..94defb04a9 100644
--- a/commit-graph.h
+++ b/commit-graph.h
@@ -58,4 +58,6 @@ void write_commit_graph(const char *obj_dir,
int verify_commit_graph(struct repository *r, struct commit_graph *g);
+void free_commit_graph(struct commit_graph *);
+
#endif
--
2.18.0.203.gfac676dfb9-goog
[PATCH v3 5/6] commit-graph: store graph in struct object_store
Instead of storing commit graphs in static variables, store them in
struct object_store. There are no changes to the signatures of existing
functions - they all still only support the_repository, and support for
other instances of struct repository will be added in a subsequent
commit.
Signed-off-by: Jonathan Tan
---
commit-graph.c | 40 +++-
object-store.h | 3 +++
object.c | 5 +
3 files changed, 27 insertions(+), 21 deletions(-)
diff --git a/commit-graph.c b/commit-graph.c
index 143a587581..b6a76a1413 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -183,24 +183,20 @@ struct commit_graph *load_commit_graph_one(const char
*graph_file)
exit(1);
}
-/* global storage */
-static struct commit_graph *commit_graph = NULL;
-
static void prepare_commit_graph_one(const char *obj_dir)
{
char *graph_name;
- if (commit_graph)
+ if (the_repository->objects->commit_graph)
return;
graph_name = get_commit_graph_filename(obj_dir);
- commit_graph = load_commit_graph_one(graph_name);
+ the_repository->objects->commit_graph =
+ load_commit_graph_one(graph_name);
FREE_AND_NULL(graph_name);
}
-static int prepare_commit_graph_run_once = 0;
-
/*
* Return 1 if commit_graph is non-NULL, and 0 otherwise.
*
@@ -212,9 +208,9 @@ static int prepare_commit_graph(void)
struct alternate_object_database *alt;
char *obj_dir;
- if (prepare_commit_graph_run_once)
- return !!commit_graph;
- prepare_commit_graph_run_once = 1;
+ if (the_repository->objects->commit_graph_attempted)
+ return !!the_repository->objects->commit_graph;
+ the_repository->objects->commit_graph_attempted = 1;
if (!core_commit_graph)
return 0;
@@ -223,16 +219,16 @@ static int prepare_commit_graph(void)
prepare_commit_graph_one(obj_dir);
prepare_alt_odb(the_repository);
for (alt = the_repository->objects->alt_odb_list;
-!commit_graph && alt;
+!the_repository->objects->commit_graph && alt;
alt = alt->next)
prepare_commit_graph_one(alt->path);
- return !!commit_graph;
+ return !!the_repository->objects->commit_graph;
}
static void close_commit_graph(void)
{
- free_commit_graph(commit_graph);
- commit_graph = NULL;
+ free_commit_graph(the_repository->objects->commit_graph);
+ the_repository->objects->commit_graph = NULL;
}
static int bsearch_graph(struct commit_graph *g, struct object_id *oid,
uint32_t *pos)
@@ -342,7 +338,7 @@ int parse_commit_in_graph(struct commit *item)
{
if (!prepare_commit_graph())
return 0;
- return parse_commit_in_graph_one(commit_graph, item);
+ return parse_commit_in_graph_one(the_repository->objects->commit_graph,
item);
}
void load_commit_graph_info(struct commit *item)
@@ -350,8 +346,8 @@ void load_commit_graph_info(struct commit *item)
uint32_t pos;
if (!prepare_commit_graph())
return;
- if (find_commit_in_graph(item, commit_graph, ))
- fill_commit_graph_info(item, commit_graph, pos);
+ if (find_commit_in_graph(item, the_repository->objects->commit_graph,
))
+ fill_commit_graph_info(item,
the_repository->objects->commit_graph, pos);
}
static struct tree *load_tree_for_commit(struct commit_graph *g, struct commit
*c)
@@ -379,7 +375,7 @@ static struct tree *get_commit_tree_in_graph_one(struct
commit_graph *g,
struct tree *get_commit_tree_in_graph(const struct commit *c)
{
- return get_commit_tree_in_graph_one(commit_graph, c);
+ return
get_commit_tree_in_graph_one(the_repository->objects->commit_graph, c);
}
static void write_graph_chunk_fanout(struct hashfile *f,
@@ -696,15 +692,17 @@ void write_commit_graph(const char *obj_dir,
if (append) {
prepare_commit_graph_one(obj_dir);
- if (commit_graph)
- oids.alloc += commit_graph->num_commits;
+ if (the_repository->objects->commit_graph)
+ oids.alloc +=
the_repository->objects->commit_graph->num_commits;
}
if (oids.alloc < 1024)
oids.alloc = 1024;
ALLOC_ARRAY(oids.list, oids.alloc);
- if (append && commit_graph) {
+ if (append && the_repository->objects->commit_graph) {
+ struct commit_graph *commit_graph =
+ the_repository->objects->commit_graph;
for (i = 0; i < commit_graph->num_commits; i++) {
const unsigned char *hash =
commit_graph->chunk_oid_lookup +
commit_graph->hash_len * i;
diff --git a/object-store.h b/object-store.h
index 0e13543bab..e481f7ad41 100644
--- a/object-store.h
+++ b/object-store.h
@@ -106,6 +106,9 @@
[PATCH v3 3/6] commit-graph: add missing forward declaration
Signed-off-by: Jonathan Tan --- commit-graph.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/commit-graph.h b/commit-graph.h index 506cb45fb1..674052bef4 100644 --- a/commit-graph.h +++ b/commit-graph.h @@ -5,6 +5,8 @@ #include "repository.h" #include "string-list.h" +struct commit; + char *get_commit_graph_filename(const char *obj_dir); /* -- 2.18.0.203.gfac676dfb9-goog
[PATCH v3 2/6] object-store: add missing include
Signed-off-by: Jonathan Tan
---
object-store.h | 3 +++
1 file changed, 3 insertions(+)
diff --git a/object-store.h b/object-store.h
index a3db17bbf5..0e13543bab 100644
--- a/object-store.h
+++ b/object-store.h
@@ -2,6 +2,9 @@
#define OBJECT_STORE_H
#include "oidmap.h"
+#include "list.h"
+#include "sha1-array.h"
+#include "strbuf.h"
struct alternate_object_database {
struct alternate_object_database *next;
--
2.18.0.203.gfac676dfb9-goog
Re: [PATCH 1/3] merge: allow reading the merge commit message from a file
"Johannes Schindelin via GitGitGadget"
writes:
> From: Johannes Schindelin
>
> This is consistent with `git commit` which, like `git merge`, supports
> passing the commit message via `-m ` and, unlike `git merge` before
> this patch, via `-F `.
>
> It is useful to allow this for scripted use, or for the upcoming patch
> to allow (re-)creating octopus merges in `git rebase --rebase-merges`.
>
> Signed-off-by: Johannes Schindelin
> ---
> Documentation/git-merge.txt | 10 +-
> builtin/merge.c | 32
> 2 files changed, 41 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
> index 6a5c00e2c..537dca7fa 100644
> --- a/Documentation/git-merge.txt
> +++ b/Documentation/git-merge.txt
> @@ -12,7 +12,7 @@ SYNOPSIS
> 'git merge' [-n] [--stat] [--no-commit] [--squash] [--[no-]edit]
> [-s ] [-X ] [-S[]]
> [--[no-]allow-unrelated-histories]
> - [--[no-]rerere-autoupdate] [-m ] [...]
> + [--[no-]rerere-autoupdate] [-m ] [-F ] [...]
> 'git merge' --abort
> 'git merge' --continue
>
> @@ -75,6 +75,14 @@ The 'git fmt-merge-msg' command can be
> used to give a good default for automated 'git merge'
> invocations. The automated message can include the branch description.
>
> +-F ::
> +--file=::
> + Read the commit message to be used for the merge commit (in
> + case one is created).
> ++
> +If `--log` is specified, a shortlog of the commits being merged
> +will be appended to the specified message.
> +
> --[no-]rerere-autoupdate::
> Allow the rerere mechanism to update the index with the
> result of auto-conflict resolution if possible.
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 4a4c09496..b0e907751 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -111,6 +111,35 @@ static int option_parse_message(const struct option *opt,
> return 0;
> }
>
> +static int option_read_message(struct parse_opt_ctx_t *ctx,
> +const struct option *opt, int unset)
> +{
> + struct strbuf *buf = opt->value;
> + const char *arg;
> +
> + if (unset)
> + BUG("-F cannot be negated");
The message "-F cannot be negated" looks as if it is pointing out a
mistake by the end user, and does not mesh well with the real reason
why this is BUG() and is not die().
I understand that this is BUG() not die() because options[] array
tells this callback not to be called with unset by having the
PARSE_OPT_NONEG bit there.
> + if (ctx->opt) {
> + arg = ctx->opt;
> + ctx->opt = NULL;
> + } else if (ctx->argc > 1) {
> + ctx->argc--;
> + arg = *++ctx->argv;
> + } else
> + return opterror(opt, "requires a value", 0);
> +
> + if (buf->len)
> + strbuf_addch(buf, '\n');
Do we assume that buf, if it is not empty, is properly terminated
with LF already? I am wondering if the real reason we do these two
lines is to make sure we have a separating blank line between what
is already there (if there already is something) and what we add, in
which case the above would want to say
if (buf->len) {
strbuf_complete_line(buf);
strbuf_addch(buf, '\n');
}
instead.
> + if (ctx->prefix && !is_absolute_path(arg))
> + arg = prefix_filename(ctx->prefix, arg);
> + if (strbuf_read_file(buf, arg, 0) < 0)
> + return error(_("could not read file '%s'"), arg);
> + have_message = 1;
A similar question is what we would want to do when the file ends
with an incomplete line. With "--log", we would be appending more
stuff to buf, and we'd want to complete such an incomplete line
before that happens, either here or in the code immediately before
"--log" is processed.
> +
> + return 0;
> +}
> +
> static struct strategy *get_strategy(const char *name)
> {
> int i;
> @@ -228,6 +257,9 @@ static struct option builtin_merge_options[] = {
> OPT_CALLBACK('m', "message", _msg, N_("message"),
> N_("merge commit message (for a non-fast-forward merge)"),
> option_parse_message),
> + { OPTION_LOWLEVEL_CALLBACK, 'F', "file", _msg, N_("path"),
> + N_("read message from file"), PARSE_OPT_NONEG,
> + (parse_opt_cb *) option_read_message },
> OPT__VERBOSITY(),
> OPT_BOOL(0, "abort", _current_merge,
> N_("abort the current in-progress merge")),
Re: [PATCH 2/3] rebase --rebase-merges: add support for octopus merges
Eric Sunshine writes: >> diff --git a/sequencer.c b/sequencer.c >> @@ -2932,7 +2966,8 @@ static int do_merge(struct commit *commit, const char >> *arg, int arg_len, >> - strbuf_addf(, "Merge branch '%.*s'", >> + strbuf_addf(, "Merge %s '%.*s'", >> + to_merge->next ? "branches" : "branch", > > Error messages in this file are already localizable. If this text ever > becomes localizable, then this "sentence lego" could be problematic > for translators. I do not think we'd want to localize these default merge messages, though. >> @@ -2956,28 +2991,76 @@ static int do_merge(struct commit *commit, const >> char *arg, int arg_len, >> + cmd.git_cmd = 1; >> + argv_array_push(, "merge"); >> + argv_array_push(, "-s"); >> + argv_array_push(, "octopus"); > > argv_array_pushl(, "-s", "octopus", NULL); > > which would make it clear that these two arguments must remain > together, and prevent someone from coming along and inserting a new > argument between them. A valid point. It is OK to break "merge" and "-s octopus" into separate push invocations, but not "-s" and "octopus". Or perhaps push it as a single "--strategy=octopus" argument, which would be a better approach anyway. >> + (GIT_AUTHOR_NAME="Hank" GIT_AUTHOR_EMAIL="hank@sea.world" \ >> +git merge -m "Tüntenfüsch" two three) && > > Unnecessary subshell? Right.
Re: [RFC PATCH v2] Add 'human' date format
On Wed, Jul 11, 2018 at 2:24 PM Ævar Arnfjörð Bjarmason wrote: > > I think that's true for the likes of linux.git & git.git, but a lot of > users of git say work in some corporate setting entirely or mostly in > the same timezone. > > In that case, knowing if some commit whose sole message was "fix"[1] was > made at 3am or in the afternoon, even if it's really old, is really > useful information, even years later. Heh. Maybe. But if you care about that kind of information, would you actually want to use the "human" date? Wouldn't you want to use the strftime thing instead, which gets you whatever field you care about, and gets it consistently regardless of how old the data is? That said, I do acknowledge that the "human" format may be a bit inflexible and ad-hoc. Of course some more generic way that allowed arbitrary rules might be better for some uses. I'll just explain the cases that made me zero in on what that last patch did: (a) I do like the "relative" date for recent stuff. Quite often, I look at how recent the commits are, for example, and then I really like seeing "2 hours ago" rather than a time with a timezone (which is a lot harder for me to mentally parse) This was the primary impetus for my original "auto" patch many years ago, that was (rightly) not merged. It really boiled down to just "default or relative, depending on how recent it was". (b) I noticed that I was distracted by dates that were *too* terse. My first patch had _just_ the time when it was today and the same timezone (but older than two hours, so the original relative logic didn't trigger). That initially sounded great to me, which is why it was that first time. But after _using_ it for a while, I actually found that it didn't have enough context for me (visually) to really trigger my date parsing at all. So "five hours ago" actually parsed better than just "9:48" to me. I didn't think it would do that, but it did. Which was why I changed the "relative" time to trigger every time if it was the exact same date (and in the past) - just to avoid the really terse model. (c) when I played around with other commands than just "git log", I also noticed that a consistent length mattered., Again, my first version was more along the lines of "if it's long ago, just use the full format, exactly like the default date". It wasn't *quite* that, because it would always skip the seconds, but it was close. And with "git log", that worked fine, because dates were fairly uniformly increasing, so the date format would slowly get longer, and that was fine. But then when I played with "git blame -C --date=human", I noticed that not only did the human date actually make sense there too, it actually made it easier for me to read - and that in particular, the "extra" info was just annoying. So now I find that shortened "only show the date" format to be really good _particularly_ for "git blame". You can see very clearly whether it's something recent or something old. Maybe my use of "git blame" is unusual, but I don't think so. I tend to do "git blame -C" when I'm looking for a bug, and then seeing something like this: ... Apr 16 2005 437) Apr 16 2005 438) Jan 14 2016 439) Apr 16 2005 440) Apr 16 2005 441) Apr 16 2005 442) Thu Jun 14 15:26 443) Thu Jun 14 15:26 444) Thu Jun 14 15:26 445) Thu Jun 14 15:26 446) Thu Jun 14 15:26 447) Thu Jun 14 15:26 448) Thu Jun 14 15:26 449) Thu Jun 14 15:26 450) Apr 16 2005 451) Jul 30 2012 452) Jul 30 2012 453) Feb 13 2012 454) Apr 16 2005 455) Apr 16 2005 456) in that date field (yeah. that happens to be "kernel/fork.c" in the current kernel - I just edited out all the other stuff than time and line number) is actually very visually easy to see what parts are old, and which ones are recent, because it changes the format pretty clearly and unambiguously, without changing the size of that field _dramatically_. (Sure, the size changes, but it's not a radical difference, it's a fairly small variation, and the variation only highlights the different time range, without making it compltely unbalanced). Anyway, enough excuses. I'l just trying to explain some of the things that I noticed simply _while_ making some of the decisions I made. Are they the "right" decisions? I don't know. But I've been running with that git config --add log.date auto in my kernel repo since I posted the patches, and so far I'm still liking it. Linus
[PATCH v2] handle lower case drive letters on Windows
On Windows, if a tool calls SetCurrentDirectory with a lower case drive
letter, the subsequent call to GetCurrentDirectory will return the same
lower case drive letter. If that happens, test-drop-caches will error out as
it does not correctly to handle lower case drive letters.
Signed-off-by: Ben Peart
---
Notes:
Base Ref: master
Web-Diff: https://github.com/benpeart/git/commit/b497c111a7
Checkout: git fetch https://github.com/benpeart/git drop-caches-v2 && git
checkout b497c111a7
### Interdiff (v1..v2):
### Patches
t/helper/test-drop-caches.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/helper/test-drop-caches.c b/t/helper/test-drop-caches.c
index d6bcfddf13..37047189c3 100644
--- a/t/helper/test-drop-caches.c
+++ b/t/helper/test-drop-caches.c
@@ -16,7 +16,7 @@ static int cmd_sync(void)
if ((0 == dwRet) || (dwRet > MAX_PATH))
return error("Error getting current directory");
- if ((Buffer[0] < 'A') || (Buffer[0] > 'Z'))
+ if ((toupper(Buffer[0]) < 'A') || (toupper(Buffer[0]) > 'Z'))
return error("Invalid drive letter '%c'", Buffer[0]);
szVolumeAccessPath[4] = Buffer[0];
base-commit: e3331758f12da22f4103eec7efe1b5304a9be5e9
--
2.17.0.gvfs.1.123.g449c066
Re: [PATCH v2 01/10] t/test-lib: teach --chain-lint to detect broken &&-chains in subshells
Eric Sunshine writes:
> The --chain-lint option detects broken &&-chains by forcing the test to
> exit early (as the very first step) with a sentinel value. If that
> sentinel is the test's overall exit code, then the &&-chain is intact;
> if not, then the chain is broken. Unfortunately, this detection does not
> extend to &&-chains within subshells even when the subshell itself is
> properly linked into the outer &&-chain.
>
> Address this shortcoming by feeding the body of the test to a
> lightweight "linter" which can peer inside subshells and identify broken
> &&-chains by pure textual inspection. Although the linter does not
> ...
> Heuristics are employed to properly identify the extent of a subshell
> formatted in the old-style since a number of legitimate constructs may
> superficially appear to close the subshell even though they don't. For
> example, it understands that neither "x=$(command)" nor "case $x in *)"
> end a subshell, despite the ")" at the end of line.
>
> Due to limitations of the tool used ('sed') and its inherent
> line-by-line processing, only subshells one level deep are handled, as
> well as one-liner subshells one level below that. Subshells deeper than
> that or multi-line subshells at level two are passed through as-is, thus
> &&-chains in their bodies are not checked.
>
> Signed-off-by: Eric Sunshine
> ---
As with the previous "transform the script and feed the result to
shell" approach, this risks to force us into writing our tests in a
subset of valid shell language, which is the primary reason why I
was not enthused when I saw the previous round. The worst part of
it is that the subset is not strictly defined based on the shell
language syntax or features (e.g. we allow this and that feature but
not that other feature) but "whatever that does not cause the linter
script to trigger false positives".
So I dunno. I haven't spent enough time to carefully look at the
actual scripts to access how serious the "problem" I perceive
actually is with this series to form a firm opinion yet. Let me
come back to the topic after doing so.
RE: [PATCH v1] handle lower case drive letters on Windows
> -Original Message- > From: Junio C Hamano On Behalf Of Junio C Hamano > Sent: Wednesday, July 11, 2018 4:11 PM > To: Ben Peart > Cc: Stefan Beller ; git > Subject: Re: [PATCH v1] handle lower case drive letters on Windows > > Ben Peart writes: > > >> -Original Message- > >> From: Stefan Beller > >> Sent: Wednesday, July 11, 2018 1:59 PM > >> To: Ben Peart > >> Cc: git ; Junio C Hamano > >> Subject: Re: [PATCH v1] handle lower case drive letters on Windows > >> > >> On Wed, Jul 11, 2018 at 10:54 AM Ben Peart > >> wrote: > >> > > >> > Teach test-drop-caches to handle lower case drive letters on Windows. > >> > >> As someone not quite familiar with Windows (and using Git there), > >> is this addressing a user visible issue, or a developer visible issue? > >> (It looks to me as the latter as it touches test code). In which way > >> does it improve the life of a developer? > >> > > > > It is a developer visible issue. On Windows, file names (including drive > > letters) are case insensitive. This patch improves the life of a Windows > > developer by making drive letters case insensitive for the test-drop-caches > > test application as well. Without this patch "test-drop-caches e" will fail > > with an error "Invalid drive letter 'e'" instead of succeeding as expected. > > I think one point of the original question was if it is common for a > developer to say "test-drop-caches e" from the command line, or the > helper is run solely by being written in some numbered test script > directly under t/. In the latter case, it would be reasonable to > expect and insist the scripts to use the more canonical form, even > if the platform is case insensitive (assuming E: is more canonical > than e:, that is) no? > > In any case, a larger point is that it would help other people who > read the patch and "git log" output, if the answer you gave Stefan > in the message I am responding to, and another one that you may give > me in a response to this message, were in the proposed log message > in the patch. > > Thanks. I apologize. My memory had faded as to the scenario that caused the issue and my earlier response was incorrect. Some months ago I ran into a situation where GetCurrentDirectory returned a lower case drive letter which caused test-drop-caches to fail. While most tools _do_ upper case the drive letter before calling SetCurrentDirectory, it isn't anything that is enforced so you _can_ get a lower case drive letter from GetCurrentDirectory and we should handle it properly. At the time, I simply patched my local copy to properly handle that case and the patch has been sitting in my "todo" backlog for a while now. I'll submit a V2 with a better commit message.
Re: What's cooking in git.git (Jul 2018, #01; Wed, 11)
Junio C Hamano writes: > * jt/commit-graph-per-object-store (2018-07-09) 6 commits > ... > Expecting a reroll, as it breaks 32-bit build. > cf. https://travis-ci.org/git/git/jobs/402422108 > * ot/ref-filter-object-info (2018-07-09) 4 commits > ... > Expecting a reroll, as it breaks compilation with uninitialized var. > cf. https://travis-ci.org/git/git/jobs/402422102 > * ds/multi-pack-index (2018-07-09) 24 commits > ... > Expecting a reroll to fix documentation build. > cf. https://travis-ci.org/git/git/jobs/402422110 I've added SQUASH??? patch at the tip of each of the above, rebuilt 'pu' with them and pushed the result out. It seems that Travis is happier with the result. Please do not forget to squash them in when/if rerolling. If there is no need to change anything else other than squashing them, you can tell me to which commit in your series the fix needs to be squashed in (that would save me time to figure it out, obviously).
Re: [RFC PATCH v2] Add 'human' date format
[ Trying to come up with crazy special cases ]
On Wed, Jul 11, 2018 at 1:49 PM Linus Torvalds
wrote:
>
> But it could be anything else invalid, of course. It could be MAX_INT
> or something like that.
That might be better. A timezone of -1 isn't actually a valid
timezone, but I guess you could create a commit by hand that had
"-0001" as the timezone.
You can't do that with something like MAX_INT, without fsck
complaining - since it has to be exactly four digits.
> The clearing of "human_tm" is done for a similar reason: the code does
>
> hide.year = tm->tm_year == human_tm->tm_year;
>
> (and then later just checks "if (human_tm->tm_year)") knowing that a
> non-zero tm_year will only ever happen for human_tz (and that 1900 is
> not a valid git date, even though I guess in theory you could do it).
Actually, the 1900 should be safe, because 'timestamp_t' is unsigned.
So a valid timestamp really can't be before 1970.
Of course, you can probably try to mess with it by giving values that
don't actually fit, because sometimes we do convert mindlessly from
'timestamp_t' to 'time_t'. In particular, if you use the
"default-local" time, it will use that
static struct tm *time_to_tm_local(timestamp_t time)
{
time_t t = time;
return localtime();
}
and not check the range of the timestamp.
But other proper time stamp functions will actually do range checking
with "date_overflow()", so in general that whole assumption of "a real
git date cannot be in the year 1900" is valid.
Linus
Re: [RFC PATCH v2] Add 'human' date format
On Sat, Jul 07 2018, Linus Torvalds wrote: I really like where this is going in general. Having a "human" format would be great. > For really recent dates (same day), use the relative date stamp, while > for old dates (year doesn't match), don't bother with time and timezone. > [...] > Once you're talking "last year" patches, you don't tend to care about time > of day or timezone. So the longest date is basically "Thu Oct 19 16:00", > because if you show the year (four characters), you don't show the time > (five characters). And the timezone (five characters) is only shown if not > showing the date (5-6 characters). Just chiming in on this part, I think it's a worthwile trade-off to always keep it relatively short, but I'd like to challenge the "you don't tend to care about time [for really old commits]". I think that's true for the likes of linux.git & git.git, but a lot of users of git say work in some corporate setting entirely or mostly in the same timezone. In that case, knowing if some commit whose sole message was "fix"[1] was made at 3am or in the afternoon, even if it's really old, is really useful information, even years later. Maybe something like v2 could be a human-lossy and v1 human-short (or better names...). I.e. (AFAICT) v1 didn't lose any information, just smartly abbreviated it, but v2 does. 1. Because let's face it, bothering to write good commit messages like git.git is the exception.
Re: [PATCH v2 6/6] commit-graph: add repo arg to graph readers
> >> Also, this will conflict with sb/object-store-lookup, won't it? I'm > >> guessing this is why you didn't touch the "git commit-graph > >> [write|verify]"code paths. > > It will conflict because of the change to lookup_commit(), but the only > > new code I'm writing is in t/helper/test-repository.c, so hopefully the > > merge won't be too tedious. The main reason why I didn't touch the > > writing/verifying part is to reduce the size of this patch set, and > > because that change is not needed to update parse_commit() and others. > > I guess my main complaint is that this won't be an actual "merge" > conflict, but the result will not compile. Since Stefan already has a > series out that changes this method, I recommend basing your series on > it (in addition to basing it on ds/commit-graph-fsck). Good point. Junio requested a reroll in his What's Cooking e-mail [1], and the same e-mail states that ds/commit-graph-fsck and sb/object-store-lookup will be merged to next, so there are a few good reasons to base it on both. I'll do that and send out an updated version soon. [1] https://public-inbox.org/git/xmqq7em138a5@gitster-ct.c.googlers.com/
Re: [RFC PATCH v2] Add 'human' date format
Andrei Rybak writes: > On Wed, 11 Jul 2018 at 22:34, Andrei Rybak wrote: >> >> Is -1 an OK initial value for timezone if local_time_tzoffset returns >> negative values as well? It looks like it doesn't matter for from functional >> > > meant to say: "It looks like it doesn't matter from the functional > point of view". As long as we do not show data in a timezone that is exactly one minute ahead (or is it behind???) of UTC, it does not cause an issue in practice.
Re: [PATCH v2] add -p: fix counting empty context lines in edited patches
Jeff Felchner writes: > Hey all, I assumed this was going to be in 2.18, but I'm still having the > same issue. What's the plan for release of this? You assumed wrong ;-) A patch written on June 11th that is already deep into pre-release freeze, unless it is about fixing a regression during the same cycle, would never be in the release tagged on 21st. It is already a part of the 'master' branch after v2.18, so v2.19 would be the first feature release that would see it (unless we discover problems in that change and need to revert it, that is).
Re: [RFC PATCH v2] Add 'human' date format
On Wed, Jul 11, 2018 at 1:34 PM Andrei Rybak wrote: > > > + int human_tz = -1; > > Is -1 an OK initial value for timezone if local_time_tzoffset returns > negative values as well? It looks like it doesn't matter for from functional The value was intentionally picked to *not* be a valid timezone value, so that the comparison of "human_tz == tz" would always fail if DATE_HUMAN is not selected. But it could be anything else invalid, of course. It could be MAX_INT or something like that. By picking something that isn't possibly a real timezone value, late code can do things like hide.tz = local || tz == human_tz; without worrying about whther it's really DATE_HUMAN or not. The clearing of "human_tm" is done for a similar reason: the code does hide.year = tm->tm_year == human_tm->tm_year; (and then later just checks "if (human_tm->tm_year)") knowing that a non-zero tm_year will only ever happen for human_tz (and that 1900 is not a valid git date, even though I guess in theory you could do it). Linus
Re: [RFC PATCH v2] Add 'human' date format
On Wed, 11 Jul 2018 at 22:34, Andrei Rybak wrote: > > Is -1 an OK initial value for timezone if local_time_tzoffset returns > negative values as well? It looks like it doesn't matter for from functional > meant to say: "It looks like it doesn't matter from the functional point of view".
Re: [RFC PATCH v2] Add 'human' date format
On 2018-07-08 00:02, Linus Torvalds wrote:
> diff --git a/date.c b/date.c
> index 49f943e25..4486c028a 100644
> --- a/date.c
> +++ b/date.c
> @@ -77,22 +77,16 @@ static struct tm *time_to_tm_local(timestamp_t time)
> }
>
> /*
> - * What value of "tz" was in effect back then at "time" in the
> - * local timezone?
> + * Fill in the localtime 'struct tm' for the supplied time,
> + * and return the local tz.
> */
> -static int local_tzoffset(timestamp_t time)
> +static int local_time_tzoffset(time_t t, struct tm *tm)
> {
> - time_t t, t_local;
> - struct tm tm;
> + time_t t_local;
> int offset, eastwest;
>
> - if (date_overflows(time))
> - die("Timestamp too large for this system: %"PRItime, time);
> -
> - t = (time_t)time;
> - localtime_r(, );
> - t_local = tm_to_time_t();
> -
> + localtime_r(, tm);
> + t_local = tm_to_time_t(tm);
> if (t_local == -1)
> return 0; /* error; just use + */
> if (t_local < t) {
> @@ -107,6 +101,20 @@ static int local_tzoffset(timestamp_t time)
> return offset * eastwest;
> }
>
[...]
> +
> const char *show_date(timestamp_t time, int tz, const struct date_mode *mode)
> {
> struct tm *tm;
> + struct tm human_tm = { 0 };
> + int human_tz = -1;
Is -1 an OK initial value for timezone if local_time_tzoffset returns
negative values as well? It looks like it doesn't matter for from functional
> static struct strbuf timebuf = STRBUF_INIT;
>
> if (mode->type == DATE_UNIX) {
> @@ -202,6 +281,15 @@ const char *show_date(timestamp_t time, int tz, const
> struct date_mode *mode)
> return timebuf.buf;
> }
>
> + if (mode->type == DATE_HUMAN) {
> + struct timeval now;
> +
> + gettimeofday(, NULL);
> +
> + /* Fill in the data for "current time" in human_tz and human_tm
> */
> + human_tz = local_time_tzoffset(now.tv_sec, _tm);
> + }
> +
> if (mode->local)
> tz = local_tzoffset(time);
>
--
Best regards, Andrei Rybak
Re: [PATCH v2] add -p: fix counting empty context lines in edited patches
Hey all, I assumed this was going to be in 2.18, but I'm still having the same
issue. What's the plan for release of this?
> On 2018 Jun 11, at 4:46, Phillip Wood wrote:
>
> From: Phillip Wood
>
> recount_edited_hunk() introduced in commit 2b8ea7f3c7 ("add -p:
> calculate offset delta for edited patches", 2018-03-05) required all
> context lines to start with a space, empty lines are not counted. This
> was intended to avoid any recounting problems if the user had
> introduced empty lines at the end when editing the patch. However this
> introduced a regression into 'git add -p' as it seems it is common for
> editors to strip the trailing whitespace from empty context lines when
> patches are edited thereby introducing empty lines that should be
> counted. 'git apply' knows how to deal with such empty lines and POSIX
> states that whether or not there is an space on an empty context line
> is implementation defined [1].
>
> Fix the regression by counting lines that consist solely of a newline
> as well as lines starting with a space as context lines and add a test
> to prevent future regressions.
>
> [1] http://pubs.opengroup.org/onlinepubs/9699919799/utilities/diff.html
>
> Reported-by: Mahmoud Al-Qudsi
> Reported-by: Oliver Joseph Ash
> Reported-by: Jeff Felchner
> Signed-off-by: Phillip Wood
> ---
>
> Thanks for the feedback, the only changes since v1 are to fix the
> commit message to match what was in pu and to change '$_' to '$mode'
> in the comparison as I think that is clearer. In the end I decided to
> leave the tests as they are.
>
> git-add--interactive.perl | 2 +-
> t/t3701-add-interactive.sh | 43 ++
> 2 files changed, 44 insertions(+), 1 deletion(-)
>
> diff --git a/git-add--interactive.perl b/git-add--interactive.perl
> index ab022ec073..8361ef45e7 100755
> --- a/git-add--interactive.perl
> +++ b/git-add--interactive.perl
> @@ -1047,7 +1047,7 @@ sub recount_edited_hunk {
> $o_cnt++;
> } elsif ($mode eq '+') {
> $n_cnt++;
> - } elsif ($mode eq ' ') {
> + } elsif ($mode eq ' ' or $mode eq "\n") {
> $o_cnt++;
> $n_cnt++;
> }
> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> index e5c66f7500..f1bb879ea4 100755
> --- a/t/t3701-add-interactive.sh
> +++ b/t/t3701-add-interactive.sh
> @@ -175,6 +175,49 @@ test_expect_success 'real edit works' '
> diff_cmp expected output
> '
>
> +test_expect_success 'setup file' '
> + test_write_lines a "" b "" c >file &&
> + git add file &&
> + test_write_lines a "" d "" c >file
> +'
> +
> +test_expect_success 'setup patch' '
> + SP=" " &&
> + NULL="" &&
> + cat >patch <<-EOF
> + @@ -1,4 +1,4 @@
> + a
> + $NULL
> + -b
> + +f
> + $SP
> + c
> + EOF
> +'
> +
> +test_expect_success 'setup expected' '
> + cat >expected <<-EOF
> + diff --git a/file b/file
> + index b5dd6c9..f910ae9 100644
> + --- a/file
> + +++ b/file
> + @@ -1,5 +1,5 @@
> + a
> + $SP
> + -f
> + +d
> + $SP
> + c
> + EOF
> +'
> +
> +test_expect_success 'edit can strip spaces from empty context lines' '
> + test_write_lines e n q | git add -p 2>error &&
> + test_must_be_empty error &&
> + git diff >output &&
> + diff_cmp expected output
> +'
> +
> test_expect_success 'skip files similarly as commit -a' '
> git reset &&
> echo file >.gitignore &&
> --
> 2.17.0
>
Re: [PATCH v1] handle lower case drive letters on Windows
Ben Peart writes: >> -Original Message- >> From: Stefan Beller >> Sent: Wednesday, July 11, 2018 1:59 PM >> To: Ben Peart >> Cc: git ; Junio C Hamano >> Subject: Re: [PATCH v1] handle lower case drive letters on Windows >> >> On Wed, Jul 11, 2018 at 10:54 AM Ben Peart >> wrote: >> > >> > Teach test-drop-caches to handle lower case drive letters on Windows. >> >> As someone not quite familiar with Windows (and using Git there), >> is this addressing a user visible issue, or a developer visible issue? >> (It looks to me as the latter as it touches test code). In which way >> does it improve the life of a developer? >> > > It is a developer visible issue. On Windows, file names (including drive > letters) are case insensitive. This patch improves the life of a Windows > developer by making drive letters case insensitive for the test-drop-caches > test application as well. Without this patch "test-drop-caches e" will fail > with an error "Invalid drive letter 'e'" instead of succeeding as expected. I think one point of the original question was if it is common for a developer to say "test-drop-caches e" from the command line, or the helper is run solely by being written in some numbered test script directly under t/. In the latter case, it would be reasonable to expect and insist the scripts to use the more canonical form, even if the platform is case insensitive (assuming E: is more canonical than e:, that is) no? In any case, a larger point is that it would help other people who read the patch and "git log" output, if the answer you gave Stefan in the message I am responding to, and another one that you may give me in a response to this message, were in the proposed log message in the patch. Thanks.
Re: [PATCH v2 6/6] commit-graph: add repo arg to graph readers
On 7/10/2018 1:31 PM, Jonathan Tan wrote:
+static void test_get_commit_tree_in_graph(const char *gitdir,
+ const char *worktree,
+ const struct object_id *commit_oid)
+{
+ struct repository r;
+ struct commit *c;
+ struct tree *tree;
+
+ /*
+* Create a commit independent of any repository.
+*/
+ c = lookup_commit(commit_oid);
Would this be more accurate to say we are creating a commit object
stored in the object cache of the_repository? How would you expect this
to work if/when lookup_commit() takes an arbitrary repository? You want
to provide , right (after initializing)?
Yes, you're right - Stefan too mentioned that this will need to be moved
below lookup_commit(). I'm not sure what the best way is to handle this
- maybe move this, and add a "needswork" stating that we need to pass r
to lookup_commit once it supports taking in a repository argument, as an
aid to the person who performs the merge. I'll do that if a reroll is
needed.
Also, this will conflict with sb/object-store-lookup, won't it? I'm
guessing this is why you didn't touch the "git commit-graph
[write|verify]"code paths.
It will conflict because of the change to lookup_commit(), but the only
new code I'm writing is in t/helper/test-repository.c, so hopefully the
merge won't be too tedious. The main reason why I didn't touch the
writing/verifying part is to reduce the size of this patch set, and
because that change is not needed to update parse_commit() and others.
I guess my main complaint is that this won't be an actual "merge"
conflict, but the result will not compile. Since Stefan already has a
series out that changes this method, I recommend basing your series on
it (in addition to basing it on ds/commit-graph-fsck).
Thanks,
-Stolee
Re: [PATCH] fsck: check skiplist for object in fsck_blob()
On 07/07/18 02:32, Jeff King wrote: [snip] >> I'm not interested in any savings - it would have to be a pretty >> wacky repo for there to be much in the way of savings! >> >> Simply, I have found (for many different reasons) that, if there >> is no good reason to execute some code, it is _far_ better to not >> do so! ;-) > > Heh. I also agree with that as a guiding principle. But I _also_ like > the principle of "if you do not need to do add this code, do not add > it". So the two are a little at odds here. :) I agree with that also! ;-) However, in this case, I can't imagine having to do less, to do nothing - if you see what I mean! So, I think "don't execute code you don't need to" trumps "don't add code you don't need to" here. [snip] > What next? I was kind of leaning towards loosening, but it sounded like > Junio thought the opposite. One thing I didn't like about the patch I > sent earlier is that it removes the option for the admin to say "no, I > really do want to enforce this". I don't know if that's of value or not. Yes, it would be good to let the admin set the policy. > In an ideal world, I think we'd detect the problem and then react > according to the receiver's receive.fsck.* config. And we could just > flip the default for receive.fsck.gitmodulesParse to "warning". But > IIRC, the fsck code in index-pack does not bother distinguishing between > warnings and errors. I think it ought to, but that's too big a change to > go on maint. > > It _might_ work to just flip the default to "ignore". That leaves the > paranoid admin with a way to re-enable it if they want, and I _think_ it > would be respected by index-pack. Ah, that would be good, if it works. > [goes and looks at the code] > > Hmm, we seem to have "info" these days, so maybe that would do what I > want. I.e., I wonder if the patch below does everything we'd want. It's > late here and I probably won't get back to this until Monday, but you > may want to play with it in the meantime. Sorry, I've been busy with other things and have not had the time to try the patch below (still trying to catch up with the mailing-list emails!). > diff --git a/fsck.c b/fsck.c > index 48e7e36869..0b0003055e 100644 > --- a/fsck.c > +++ b/fsck.c > @@ -61,7 +61,6 @@ static struct oidset gitmodules_done = OIDSET_INIT; > FUNC(ZERO_PADDED_DATE, ERROR) \ > FUNC(GITMODULES_MISSING, ERROR) \ > FUNC(GITMODULES_BLOB, ERROR) \ > - FUNC(GITMODULES_PARSE, ERROR) \ > FUNC(GITMODULES_NAME, ERROR) \ > FUNC(GITMODULES_SYMLINK, ERROR) \ > /* warnings */ \ > @@ -76,6 +75,7 @@ static struct oidset gitmodules_done = OIDSET_INIT; > FUNC(NUL_IN_COMMIT, WARN) \ > /* infos (reported as warnings, but ignored by default) */ \ > FUNC(BAD_TAG_NAME, INFO) \ > + FUNC(GITMODULES_PARSE, INFO) \ > FUNC(MISSING_TAGGER_ENTRY, INFO) > > #define MSG_ID(id, msg_type) FSCK_MSG_##id, > So, just squinting at this in my email client, if this allowed a push/fetch to succeed (along with an 'info' message), while providing an admin the means to configure it to loudly deny the push/fetch - then I think we have a winner! ;-) Sorry for not testing the patch. ATB, Ramsay Jones
Re: [PATCH v3 1/2] t3418: add testcase showing problems with rebase -i and strategy options
HI SZEDER, On Wed, Jul 11, 2018 at 3:56 AM, SZEDER Gábor wrote: > On Wed, Jun 27, 2018 at 8:28 PM SZEDER Gábor wrote: >> >> > diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh >> > index 03bf1b8a3b..11546d6e14 100755 >> > --- a/t/t3418-rebase-continue.sh >> > +++ b/t/t3418-rebase-continue.sh >> > @@ -74,6 +74,38 @@ test_expect_success 'rebase --continue remembers merge >> > strategy and options' ' >> > test -f funny.was.run >> > ' >> > >> > +test_expect_failure 'rebase -i --continue handles merge strategy and >> > options' ' >> > + rm -fr .git/rebase-* && >> > + git reset --hard commit-new-file-F2-on-topic-branch && >> > + test_commit "commit-new-file-F3-on-topic-branch-for-dash-i" F3 32 && >> > + test_when_finished "rm -fr test-bin funny.was.run funny.args" && >> > + mkdir test-bin && >> > + cat >test-bin/git-merge-funny <<-EOF && >> > + #!$SHELL_PATH >> > + echo "\$@" >>funny.args >> > + case "\$1" in --opt) ;; *) exit 2 ;; esac >> > + case "\$2" in --foo) ;; *) exit 2 ;; esac >> > + case "\$4" in --) ;; *) exit 2 ;; esac >> > + shift 2 && >> > + >funny.was.run && >> > + exec git merge-recursive "\$@" >> > + EOF >> > + chmod +x test-bin/git-merge-funny && >> >> You could use the 'write_script' helper function here. >> >> > + ( >> > + PATH=./test-bin:$PATH && >> > + test_must_fail git rebase -i -s funny -Xopt -Xfoo master >> > topic >> > + ) && >> > + test -f funny.was.run && > > And please use 'test_path_is_file' here ... > >> > + rm funny.was.run && >> > + echo "Resolved" >F2 && >> > + git add F2 && >> > + ( >> > + PATH=./test-bin:$PATH && >> > + git rebase --continue >> > + ) && >> > + test -f funny.was.run > > ... and here. > Thanks for looking over things. This particular test was a copy of another from the same file, just for rebase -i instead of rebase -m. I think it'd be nice to keep the two tests looking similar and either fix up the rebase -m case with a preparatory patch added to this series, or wait for this series to merge and let a future series clean up both testcases. For other suggested fixups to the same test, Junio suggested just taking the latter approach[1], so I'm assuming that's how I should handle these as well. [1] https://public-inbox.org/git/xmqqmuvgdods@gitster-ct.c.googlers.com/
RE: [PATCH v1] handle lower case drive letters on Windows
> -Original Message- > From: Stefan Beller > Sent: Wednesday, July 11, 2018 1:59 PM > To: Ben Peart > Cc: git ; Junio C Hamano > Subject: Re: [PATCH v1] handle lower case drive letters on Windows > > On Wed, Jul 11, 2018 at 10:54 AM Ben Peart > wrote: > > > > Teach test-drop-caches to handle lower case drive letters on Windows. > > As someone not quite familiar with Windows (and using Git there), > is this addressing a user visible issue, or a developer visible issue? > (It looks to me as the latter as it touches test code). In which way > does it improve the life of a developer? > It is a developer visible issue. On Windows, file names (including drive letters) are case insensitive. This patch improves the life of a Windows developer by making drive letters case insensitive for the test-drop-caches test application as well. Without this patch "test-drop-caches e" will fail with an error "Invalid drive letter 'e'" instead of succeeding as expected. > Thanks, > Stefan
What's cooking in git.git (Jul 2018, #01; Wed, 11)
Here are the topics that have been cooking. Commits prefixed with '-' are only in 'pu' (proposed updates) while commits prefixed with '+' are in 'next'. The ones marked with '.' do not appear in any of the integration branches, but I am still holding onto them. The tip of 'next' has been rewound and it currently has only 4 topics. Quite a many topics are cooking in 'pu' and need to be sifted into good bins (for 'next') and the remainder. Help is appreciated in that area ;-) You can find the changes described here in the integration branches of the repositories listed at http://git-blame.blogspot.com/p/git-public-repositories.html -- [New Topics] * ag/rebase-i-in-c (2018-07-10) 13 commits - rebase -i: rewrite the rest of init_revisions_and_shortrevisions in C - rebase -i: implement the logic to initialize the variable $revision in C - rebase--interactive: remove unused modes and functions - rebase--interactive: rewrite complete_action() in C - sequencer: change the way skip_unnecessary_picks() returns its result - sequencer: refactor append_todo_help() to write its message to a buffer - rebase -i: rewrite checkout_onto() in C - rebase -i: rewrite setup_reflog_action() in C - sequencer: add a new function to silence a command, except if it fails - rebase-interactive: rewrite the edit-todo functionality in C - editor: add a function to launch the sequence editor - rebase--interactive: rewrite append_todo_help() in C - sequencer: make two functions and an enum from sequencer.c public Piecemeal rewrite of the remaining "rebase -i" machinery in C. Expecting a reroll. The early parts of the series seem solidifying; perhaps with a reroll or two, they become 'next' material? * en/apply-comment-fix (2018-06-28) 1 commit - apply: fix grammar error in comment Will merge to 'next'. * en/t5407-rebase-m-fix (2018-06-28) 1 commit - t5407: fix test to cover intended arguments Will merge to 'next'. * sb/mailmap (2018-06-29) 1 commit (merged to 'next' on 2018-07-11 at d9dc53e374) + .mailmap: merge different spellings of names Will merge to 'master'. * sb/object-store-lookup (2018-06-29) 33 commits - commit.c: allow lookup_commit_reference to handle arbitrary repositories - commit.c: allow lookup_commit_reference_gently to handle arbitrary repositories - tag.c: allow deref_tag to handle arbitrary repositories - object.c: allow parse_object to handle arbitrary repositories - object.c: allow parse_object_buffer to handle arbitrary repositories - commit.c: allow get_cached_commit_buffer to handle arbitrary repositories - commit.c: allow set_commit_buffer to handle arbitrary repositories - commit.c: migrate the commit buffer to the parsed object store - commit-slabs: remove realloc counter outside of slab struct - commit.c: allow parse_commit_buffer to handle arbitrary repositories - tag: allow parse_tag_buffer to handle arbitrary repositories - tag: allow lookup_tag to handle arbitrary repositories - commit: allow lookup_commit to handle arbitrary repositories - tree: allow lookup_tree to handle arbitrary repositories - blob: allow lookup_blob to handle arbitrary repositories - object: allow lookup_object to handle arbitrary repositories - object: allow object_as_type to handle arbitrary repositories - tag: add repository argument to deref_tag - tag: add repository argument to parse_tag_buffer - tag: add repository argument to lookup_tag - commit: add repository argument to get_cached_commit_buffer - commit: add repository argument to set_commit_buffer - commit: add repository argument to parse_commit_buffer - commit: add repository argument to lookup_commit - commit: add repository argument to lookup_commit_reference - commit: add repository argument to lookup_commit_reference_gently - tree: add repository argument to lookup_tree - blob: add repository argument to lookup_blob - object: add repository argument to object_as_type - object: add repository argument to parse_object_buffer - object: add repository argument to lookup_object - object: add repository argument to parse_object - Merge branch 'sb/object-store-grafts' into sb/object-store-lookup (this branch uses sb/object-store-grafts.) lookup_commit_reference() and friends have been updated to find in-core object for a specific in-core repository instance. Will merge to 'next'. * ag/rebase-p (2018-07-06) 1 commit - git-rebase--preserve-merges: fix formatting of todo help message The help message shown in the editor to edit todo list in "rebase -p" has regressed recently, which has been corrected. Will merge to 'next'. * bb/pedantic (2018-07-09) 8 commits - utf8.c: avoid char overflow - string-list.c: avoid conversion from void * to function pointer - sequencer.c: avoid empty statements at top level - convert.c: replace "\e" escapes with "\033". - fixup! refs/refs-internal.h: avoid forward declaration of an enum -
Re: [PATCH 2/3] rebase --rebase-merges: add support for octopus merges
On Wed, Jul 11, 2018 at 8:38 AM Johannes Schindelin via GitGitGadget wrote: > Previously, we introduced the `merge` command for use in todo lists, > to allow to recreate and modify branch topology. > > For ease of implementation, and to make review easier, the initial > implementation only supported merge commits with exactly two parents. > > This patch adds support for octopus merges, making use of the > just-introduced `-F ` option for the `git merge` command: to keep > things simple, we spawn a new Git command instead of trying to call a > library function, also opening an easier door to enhance `rebase > --rebase-merges` to optionally use a merge strategy different from > `recursive` for regular merges: this feature would use the same code > path as octopus merges and simply spawn a `git merge`. > > Signed-off-by: Johannes Schindelin A few minor notes below (not a proper review)... > diff --git a/sequencer.c b/sequencer.c > @@ -2932,7 +2966,8 @@ static int do_merge(struct commit *commit, const char > *arg, int arg_len, > - strbuf_addf(, "Merge branch '%.*s'", > + strbuf_addf(, "Merge %s '%.*s'", > + to_merge->next ? "branches" : "branch", Error messages in this file are already localizable. If this text ever becomes localizable, then this "sentence lego" could be problematic for translators. > @@ -2956,28 +2991,76 @@ static int do_merge(struct commit *commit, const char > *arg, int arg_len, > + cmd.git_cmd = 1; > + argv_array_push(, "merge"); > + argv_array_push(, "-s"); > + argv_array_push(, "octopus"); argv_array_pushl(, "-s", "octopus", NULL); which would make it clear that these two arguments must remain together, and prevent someone from coming along and inserting a new argument between them. > + argv_array_push(, "--no-edit"); > + argv_array_push(, "--no-ff"); > + argv_array_push(, "--no-log"); > + argv_array_push(, "--no-stat"); > + argv_array_push(, "-F"); > + argv_array_push(, git_path_merge_msg()); Ditto: argv_array_pushl(, "-L", git_path_merge_msg(), NULL); > diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh > @@ -329,4 +329,38 @@ test_expect_success 'labels that are object IDs are > rewritten' ' > +test_expect_success 'octopus merges' ' > + git checkout -b three && > + test_commit before-octopus && > + test_commit three && > + git checkout -b two HEAD^ && > + test_commit two && > + git checkout -b one HEAD^ && > + test_commit one && > + test_tick && > + (GIT_AUTHOR_NAME="Hank" GIT_AUTHOR_EMAIL="hank@sea.world" \ > +git merge -m "Tüntenfüsch" two three) && Unnecessary subshell? > + : fast forward if possible && > + before="$(git rev-parse --verify HEAD)" && > + test_tick && > + git rebase -i -r HEAD^^ && > + test_cmp_rev HEAD $before && > + > + test_tick && > + git rebase -i --force -r HEAD^^ && > + test "Hank" = "$(git show -s --format=%an HEAD)" && > + test "$before" != $(git rev-parse HEAD) && > + test_cmp_graph HEAD^^.. <<-\EOF > + *-. Tüntenfüsch > + |\ \ > + | | * three > + | * | two > + | |/ > + * | one > + |/ > + o before-octopus > + EOF > +'
Re: [PATCH 2/2] read-cache: fix directory/file conflict handling in read_index_unmerged()
On Wed, Jul 11, 2018 at 10:03 AM, Junio C Hamano wrote: > Elijah Newren writes: > >> The _only_ reason we want to keep a previously unmerged entry in the >> index at stage #0 is so that we don't forget the fact that we have >> corresponding file in the work tree in order to be able to remove it >> when the tree we are resetting to does not have the path. >> ... >> So, that's the intended purpose of this function. The problem is that >> when directory/files conflicts are present, trying to add the file to the >> index at stage 0 fails (because there is still a directory in the way), >> and the function returns early with a -1 return code to signify the error. >> As noted above, none of the callers who want the drop-to-stage-0 behavior >> check the return status, though, so this means all remaining unmerged >> entries remain in the index and the callers proceed assuming otherwise. > > Nicely analysed and explained so far. > >> ... The temporary simultaneous appearance of the >> directory and file entries in the index will be removed by the callers >> before they attempt to write the index anywhere. > > But this part makes me feel a bit uneasy, as I find this "will be > removed" a bit hand-wavy. There are two such callers. "am --skip" > and "reset". > > The former uses am.c::fast_forward_to that calls unpack_trees() to > two-way merge (aka "switch to the other branch") and these entries > with CE_CONFLICTED flag get removed in merged/deleted_entry(). > > "reset" (all variants) call unpack_trees() on the index prepared > with read_cache_unmerged(), and the unmerged entries that are marked > with CE_CONFLICTED bit get removed the same way. > > So perhaps before "before they attempt to", saying "by calling > unpack_trees(), which excludes these unmerged entries marked with > CE_CONFLICTED flag from the resulting index," or something like that > would help uneasy readers? Makes sense, I'll include that in my re-roll after waiting a little bit for any further reviews.
Re: [PATCH 1/2] t1015: demonstrate directory/file conflict recovery failures
Hi Eric,
On Wed, Jul 11, 2018 at 2:21 AM, Eric Sunshine wrote:
> On Tue, Jul 10, 2018 at 10:18:33PM -0700, Elijah Newren wrote:
>> Several "recovery" commands outright fail or do not fully recover
>> when directory-file conflicts are present. This includes:
>> * git read-tree --reset HEAD
>> * git am --skip
>> * git am --abort
>> * git merge --abort
>> * git reset --hard
>>
>> Add testcases documenting these shortcomings.
>>
>> Signed-off-by: Elijah Newren
>> ---
>> diff --git a/t/t1015-read-index-unmerged.sh b/t/t1015-read-index-unmerged.sh
>> @@ -0,0 +1,123 @@
>> +test_expect_success 'setup modify/delete + directory/file conflict' '
>> + test_create_repo df_plus_modify_delete &&
>> + (
>> + cd df_plus_modify_delete &&
>> +
>> + printf "a\nb\nc\nd\ne\nf\ng\nh\n" >letters &&
>
> test_write_lines a b c d e f g h >letters &&
Copying and modifying an existing testcase, while forgetting to check
for anachronisms, strikes again. As always, thanks for reviewing and
catching this; I'll fix it up.
>> + git add letters &&
>> + git commit -m initial &&
>> +
>> + git checkout -b modify &&
>> + # Throw in letters.txt for sorting order fun
>> + # ("letters.txt" sorts between "letters" and "letters/file")
>> + echo i >>letters &&
>> + echo "version 2" >letters.txt &&
>> + git add letters letters.txt &&
>> + git commit -m modified &&
>> +
>> + git checkout -b delete HEAD^ &&
>> + git rm letters &&
>> + mkdir letters &&
>> + >letters/file &&
>> + echo "version 1" >letters.txt &&
>> + git add letters letters.txt &&
>> + git commit -m deleted
>> + )
>> +'
>> +
>> +test_expect_failure 'read-tree --reset cleans unmerged entries' '
>> + test_when_finished "git -C df_plus_modify_delete clean -f" &&
>> + test_when_finished "git -C df_plus_modify_delete reset --hard" &&
>> + (
>> + cd df_plus_modify_delete &&
>> + ...
>> + )
>> +'
>
> I wonder how much value these distinct repositories add over not using
> them:
In my opinion, that'd be much worse. Personally, I think we should
move in the opposite direction and try to migrate more of the
testsuite elsewhere towards clearly independent tests. A huge pet
peeve of mine is that trying to debug a test often requires working
through dozens and dozens of unrelated tests and their setup just to
understand which part of the repository state is related to the test
at hand and which parts can be ignored. It's happened enough times
that I just intentionally try to make it clear which tests of mine are
independent by making sure they have their own separate repo (and I
used to do a git reset --hard && git clean -fdqx && rm -rf .git && git
init at the beginning of tests). If folks have a better suggestion
for how to ensure test independence than using test_create_repo, I'm
all ears, but I'm strongly against just adding more files into the
repo than what previous tests did and continuing running from there.
I feel that's especially important for future readers when dealing
with weird edge and corner cases for merges, but I'd really like to
see that clean separation spread throughout the test suite.
> --- >8 ---
> #!/bin/sh
>
> test_description='Test various callers of read_index_unmerged'
> . ./test-lib.sh
>
> test_expect_success 'setup modify/delete + directory/file conflict' '
> test_write_lines a b c d e f g h >letters &&
> git add letters &&
> git commit -m initial &&
>
> git checkout -b modify &&
> # Throw in letters.txt for sorting order fun
> # ("letters.txt" sorts between "letters" and "letters/file")
> echo i >>letters &&
> echo "version 2" >letters.txt &&
> git add letters letters.txt &&
> git commit -m modified &&
>
> git checkout -b delete HEAD^ &&
> git rm letters &&
> mkdir letters &&
> >letters/file &&
> echo "version 1" >letters.txt &&
> git add letters letters.txt &&
> git commit -m deleted
> '
>
> test_expect_failure 'read-tree --reset cleans unmerged entries' '
> test_when_finished "git clean -f" &&
> test_when_finished "git reset --hard" &&
>
> git checkout delete^0 &&
> test_must_fail git merge modify &&
>
> git read-tree --reset HEAD &&
> git ls-files -u >conflicts &&
> test_must_be_empty conflicts
> '
>
> test_expect_failure 'One reset --hard cleans unmerged entries' '
> test_when_finished "git clean -f" &&
> test_when_finished "git reset --hard" &&
>
> git checkout delete^0 &&
> test_must_fail git merge modify &&
>
> git reset --hard &&
> test_path_is_missing .git/MERGE_HEAD &&
> git ls-files -u >conflicts &&
> test_must_be_empty
Re: [PATCH v1] handle lower case drive letters on Windows
On Wed, Jul 11, 2018 at 10:54 AM Ben Peart wrote: > > Teach test-drop-caches to handle lower case drive letters on Windows. As someone not quite familiar with Windows (and using Git there), is this addressing a user visible issue, or a developer visible issue? (It looks to me as the latter as it touches test code). In which way does it improve the life of a developer? Thanks, Stefan
[PATCH v1] handle lower case drive letters on Windows
Teach test-drop-caches to handle lower case drive letters on Windows.
Signed-off-by: Ben Peart
---
Notes:
Base Ref: master
Web-Diff: https://github.com/benpeart/git/commit/55b815ee73
Checkout: git fetch https://github.com/benpeart/git drop-caches-v1 && git
checkout 55b815ee73
t/helper/test-drop-caches.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/helper/test-drop-caches.c b/t/helper/test-drop-caches.c
index d6bcfddf13..37047189c3 100644
--- a/t/helper/test-drop-caches.c
+++ b/t/helper/test-drop-caches.c
@@ -16,7 +16,7 @@ static int cmd_sync(void)
if ((0 == dwRet) || (dwRet > MAX_PATH))
return error("Error getting current directory");
- if ((Buffer[0] < 'A') || (Buffer[0] > 'Z'))
+ if ((toupper(Buffer[0]) < 'A') || (toupper(Buffer[0]) > 'Z'))
return error("Invalid drive letter '%c'", Buffer[0]);
szVolumeAccessPath[4] = Buffer[0];
base-commit: e3331758f12da22f4103eec7efe1b5304a9be5e9
--
2.17.0.gvfs.1.123.g449c066
Re: [PATCH] sha1-name.c: for ":/", find detached HEAD commits
> > Does for_each_ref() iterate over per-worktree refs (like "bisect", > > perhaps)? > > No. To be clear: it iterates only over the per-worktree refs for the current worktree. So, if you mark an unreachable commit as "bad" with bisect, then that commit is reachable (and found by ":/") in the enclosing worktree only. With this patch, ":/" now behaves the same way with HEADs, which makes sense to me. Agreed with the above discussion. I can add a test for this.
Re: [PATCH 0/3] rebase -r: support octopus merges
On Wed, Jul 11, 2018 at 10:35 AM Junio C Hamano wrote: > To be honest, I am not sure if there still are people who use > octopus The latest merge with more than 2 parents in linux.git is df958569dbaa (Merge branches 'acpi-tables' and 'acpica', 2018-07-05), although looking through the log of octopusses I get the impression that mostly Rafael J. Wysocki is really keen on these. :-)
Re: [PATCH] sha1-name.c: for ":/", find detached HEAD commits
On Wed, Jul 11, 2018 at 7:20 PM Junio C Hamano wrote: > > Duy Nguyen writes: > > >> diff --git a/sha1-name.c b/sha1-name.c > >> index 60d9ef3c7..641ca12f9 100644 > >> --- a/sha1-name.c > >> +++ b/sha1-name.c > >> @@ -1650,6 +1650,7 @@ static int get_oid_with_context_1(const char *name, > >> struct commit_list *list = NULL; > >> > >> for_each_ref(handle_one_ref, ); > >> + head_ref(handle_one_ref, ); > > > > When multiple worktrees are used, should we consider all HEADs or just > > current worktree's HEAD? > > Does for_each_ref() iterate over per-worktree refs (like "bisect", > perhaps)? No. > If so, then looking in different worktree's HEADs would > make sense, and otherwise not. > > I would think that the whole point of detaching HEAD in a separate > worktree is that you can avoid exposing the work you do while > detached to other worktrees by doing so, so from that point of view, > I would probably prefer :/ not to look into other worktrees, but > that is not a very strong preference. Yeah at least for me worktrees are still quite isolated. Occasionally I need to peek in a worktree from another one (e.g. if I want to run tests on current HEAD but do not want to wait for it to finish, I'll make a new worktree, checking out the same head and run tests there) but it's really rare. So yeah maybe it's best to stick to current worktree's HEAD only. At least until someone finds a good case for it. > If peeking all over the place > is easier to implement, then a minor information leaking is not > something I'd lose sleep over. > > Thanks for bringing up an interesting issue. -- Duy
Re: [PATCH 0/3] rebase -r: support octopus merges
"Johannes Schindelin via GitGitGadget" writes: > The `--rebase-merges` option of `git rebase` generates todo lists that > include the merge commits that are to be rebased. > > To keep things simpler to review, I decided to support only regular, 2-parent > merge commits first. > > With this patch series, support is extended to cover also octopus merges. ;-) To be honest, I am not sure if there still are people who use octopus (even though I freely admit that I am guilty of inventing the term and the mechanism), given its downsides, primary one of which is that it makes bisection less efficient. Nevertheless, this *is* the right thing to do from feature completeness point of view. Thanks for following it through. > > Johannes Schindelin (3): > merge: allow reading the merge commit message from a file > rebase --rebase-merges: add support for octopus merges > rebase --rebase-merges: adjust man page for octopus support > > Documentation/git-merge.txt | 10 ++- > Documentation/git-rebase.txt | 7 +- > builtin/merge.c | 32 +++ > sequencer.c | 168 ++- > t/t3430-rebase-merges.sh | 34 +++ > 5 files changed, 204 insertions(+), 47 deletions(-) > > > base-commit: e3331758f12da22f4103eec7efe1b5304a9be5e9 > Published-As: > https://github.com/gitgitgadget/git/releases/tags/pr-8%2Fdscho%2Fsequencer-and-octopus-merges-v1 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git > pr-8/dscho/sequencer-and-octopus-merges-v1 > Pull-Request: https://github.com/gitgitgadget/git/pull/8
Re: [PATCH v2 3/3] t/lib-httpd: avoid occasional failures when checking access.log
On Wed, Jul 11, 2018 at 07:26:19PM +0200, SZEDER Gábor wrote: > > But it is really "make sure that a failed test here does not prevent us > > from doing this cleanup". So the original really should have just > > dropped that comment and added a test_when_finished. Bumping it into a > > separate test as you have here accomplishes the same thing. > > After taking a step back, I think this would fit better into the first > patch, wouldn't it? > I will send v3 shortly. Yeah, I agree it would be even nicer to clean it up there. -Peff
Re: [PATCH v2 3/3] t/lib-httpd: avoid occasional failures when checking access.log
On Wed, Jul 11, 2018 at 4:57 PM Jeff King wrote:
>
> On Wed, Jul 11, 2018 at 02:56:47PM +0200, SZEDER Gábor wrote:
>
> > +# Requires one argument: the name of a file containing the expected
> > stripped
> > +# access log entries.
> > +check_access_log() {
> > + sort "$1" >"$1".sorted &&
> > + strip_access_log >access.log.stripped &&
> > + sort access.log.stripped >access.log.sorted &&
> > + if ! test_cmp "$1".sorted access.log.sorted
> > + then
> > + test_cmp "$1" access.log.stripped
> > + fi
> > +}
>
> This will generate output showing both the unsorted and sorted
> differences. Do we want to suppress the sorted ones (e.g., by just using
> "cmp" directly)? I guess it doesn't matter unless there is an actual
> test failure, but I just wonder if it would be confusing to see both.
I have no opinion about this, at all. I tried it both ways back then,
and didn't find one any better than the other, so I just chose the
simpler-shorter one, i.e. no 2>/dev/null redirection in the condition.
> > diff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh
> > index 6cd986797d..a481e3989a 100755
> > --- a/t/t5541-http-push-smart.sh
> > +++ b/t/t5541-http-push-smart.sh
> > @@ -43,15 +43,13 @@ test_expect_success 'no empty path components' '
> > cd "$ROOT_PATH" &&
> > git clone $HTTPD_URL/smart/test_repo.git/ test_repo_clone &&
> >
> > - strip_access_log >act &&
> > + check_access_log exp
> > +'
> >
> > +test_expect_success 'clear access log' '
> > # Clear the log, so that it does not affect the "used receive-pack
> > # service" test which reads the log too.
> > - #
> > - # We do this before the actual comparison to ensure the log is
> > cleared.
> > - >"$HTTPD_ROOT_PATH"/access.log &&
> > -
> > - test_cmp exp act
> > + >"$HTTPD_ROOT_PATH"/access.log
> > '
>
> This took some head-scratching, mostly because of the original comment.
> I thought the "before" here was referring to a timing issue (maybe
> because we're talking about timing ;) ).
Heh, I had to do some head-scratching now as well... That's what I
get for updating the patch, and then waiting almost a month to finish
it up and update the commit message.
> But it is really "make sure that a failed test here does not prevent us
> from doing this cleanup". So the original really should have just
> dropped that comment and added a test_when_finished. Bumping it into a
> separate test as you have here accomplishes the same thing.
After taking a step back, I think this would fit better into the first
patch, wouldn't it?
I will send v3 shortly.
Re: [PATCH] sha1-name.c: for ":/", find detached HEAD commits
Duy Nguyen writes: >> diff --git a/sha1-name.c b/sha1-name.c >> index 60d9ef3c7..641ca12f9 100644 >> --- a/sha1-name.c >> +++ b/sha1-name.c >> @@ -1650,6 +1650,7 @@ static int get_oid_with_context_1(const char *name, >> struct commit_list *list = NULL; >> >> for_each_ref(handle_one_ref, ); >> + head_ref(handle_one_ref, ); > > When multiple worktrees are used, should we consider all HEADs or just > current worktree's HEAD? Does for_each_ref() iterate over per-worktree refs (like "bisect", perhaps)? If so, then looking in different worktree's HEADs would make sense, and otherwise not. I would think that the whole point of detaching HEAD in a separate worktree is that you can avoid exposing the work you do while detached to other worktrees by doing so, so from that point of view, I would probably prefer :/ not to look into other worktrees, but that is not a very strong preference. If peeking all over the place is easier to implement, then a minor information leaking is not something I'd lose sleep over. Thanks for bringing up an interesting issue.
Re: BUG: Segfault on "git pull" on "bad object HEAD"
Jeff King writes:
> So I feel like the right answer here is probably this:
>
> diff --git a/wt-status.c b/wt-status.c
> index d1c05145a4..5fcaa3d0f8 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -2340,7 +2340,16 @@ int has_uncommitted_changes(int ignore_submodules)
> if (ignore_submodules)
> rev_info.diffopt.flags.ignore_submodules = 1;
> rev_info.diffopt.flags.quick = 1;
> +
> add_head_to_pending(_info);
> + if (!rev_info.pending.nr) {
> + /*
> + * We have no head (or it's corrupt), but the index is not
> + * unborn; declare it as uncommitted changes.
> + */
> + return 1;
> + }
> +
> diff_setup_done(_info.diffopt);
> result = run_diff_index(_info, 1);
> return diff_result_code(_info.diffopt, result);
>
> That does quietly paper over the corruption, but it does the
> conservative thing, and a follow-up "git status" would yield "bad
> object: HEAD".
Sounds quite sensible.
Re: [PATCH 3/3] rebase --rebase-merges: adjust man page for octopus support
Hi Dscho, On Fri, Mar 9, 2018 at 8:36 AM, Johannes Schindelin via GitGitGadget wrote: > From: Johannes Schindelin > > Now that we support octopus merges in the `--rebase-merges` mode, > we should give users who actually read the manuals a chance to know > about this fact. > > Signed-off-by: Johannes Schindelin > --- > Documentation/git-rebase.txt | 7 --- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt > index 0e20a66e7..c4bcd24bb 100644 > --- a/Documentation/git-rebase.txt > +++ b/Documentation/git-rebase.txt > @@ -879,8 +879,8 @@ rescheduled immediately, with a helpful message how to > edit the todo list > (this typically happens when a `reset` command was inserted into the todo > list manually and contains a typo). > > -The `merge` command will merge the specified revision into whatever is > -HEAD at that time. With `-C `, the commit message of > +The `merge` command will merge the specified revision(s) into whatever > +is HEAD at that time. With `-C `, the commit message of > the specified merge commit will be used. When the `-C` is changed to > a lower-case `-c`, the message will be opened in an editor after a > successful merge so that the user can edit the message. > @@ -889,7 +889,8 @@ If a `merge` command fails for any reason other than > merge conflicts (i.e. > when the merge operation did not even start), it is rescheduled immediately. > > At this time, the `merge` command will *always* use the `recursive` > -merge strategy, with no way to choose a different one. To work around > +merge strategy for regular merges, and `octopus` for octopus merges, > +strategy, with no way to choose a different one. To work around The "...merges, strategy, with..." looks like an incomplete edit. Perhaps "...and `octopus` strategy for octopus merges, with no way..." ? > this, an `exec` command can be used to call `git merge` explicitly, > using the fact that the labels are worktree-local refs (the ref > `refs/rewritten/onto` would correspond to the label `onto`, for example). > -- > gitgitgadget
Re: [PATCH 2/2] read-cache: fix directory/file conflict handling in read_index_unmerged()
Elijah Newren writes:
> The _only_ reason we want to keep a previously unmerged entry in the
> index at stage #0 is so that we don't forget the fact that we have
> corresponding file in the work tree in order to be able to remove it
> when the tree we are resetting to does not have the path.
> ...
> So, that's the intended purpose of this function. The problem is that
> when directory/files conflicts are present, trying to add the file to the
> index at stage 0 fails (because there is still a directory in the way),
> and the function returns early with a -1 return code to signify the error.
> As noted above, none of the callers who want the drop-to-stage-0 behavior
> check the return status, though, so this means all remaining unmerged
> entries remain in the index and the callers proceed assuming otherwise.
Nicely analysed and explained so far.
> ... The temporary simultaneous appearance of the
> directory and file entries in the index will be removed by the callers
> before they attempt to write the index anywhere.
But this part makes me feel a bit uneasy, as I find this "will be
removed" a bit hand-wavy. There are two such callers. "am --skip"
and "reset".
The former uses am.c::fast_forward_to that calls unpack_trees() to
two-way merge (aka "switch to the other branch") and these entries
with CE_CONFLICTED flag get removed in merged/deleted_entry().
"reset" (all variants) call unpack_trees() on the index prepared
with read_cache_unmerged(), and the unmerged entries that are marked
with CE_CONFLICTED bit get removed the same way.
So perhaps before "before they attempt to", saying "by calling
unpack_trees(), which excludes these unmerged entries marked with
CE_CONFLICTED flag from the resulting index," or something like that
would help uneasy readers?
> Signed-off-by: Elijah Newren
> ---
> read-cache.c | 13 -
> t/t1015-read-index-unmerged.sh | 8
> t/t6020-merge-df.sh | 3 ---
> t/t6042-merge-rename-corner-cases.sh | 1 -
> 4 files changed, 12 insertions(+), 13 deletions(-)
>
> diff --git a/read-cache.c b/read-cache.c
> index 372588260..666d295a5 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -2632,10 +2632,13 @@ int write_locked_index(struct index_state *istate,
> struct lock_file *lock,
>
> /*
> * Read the index file that is potentially unmerged into given
> - * index_state, dropping any unmerged entries. Returns true if
> - * the index is unmerged. Callers who want to refuse to work
> - * from an unmerged state can call this and check its return value,
> - * instead of calling read_cache().
> + * index_state, dropping any unmerged entries to stage #0 (potentially
> + * resulting in a path appearing as both a file and a directory in the
> + * index; the caller is responsible to clear out the extra entries
> + * before writing the index to a tree). Returns true if the index is
> + * unmerged. Callers who want to refuse to work from an unmerged
> + * state can call this and check its return value, instead of calling
> + * read_cache().
> */
> int read_index_unmerged(struct index_state *istate)
> {
> @@ -2658,7 +2661,7 @@ int read_index_unmerged(struct index_state *istate)
> new_ce->ce_flags = create_ce_flags(0) | CE_CONFLICTED;
> new_ce->ce_namelen = len;
> new_ce->ce_mode = ce->ce_mode;
> - if (add_index_entry(istate, new_ce, 0))
> + if (add_index_entry(istate, new_ce, ADD_CACHE_SKIP_DFCHECK))
> return error("%s: cannot drop to stage #0",
>new_ce->name);
> }
> diff --git a/t/t1015-read-index-unmerged.sh b/t/t1015-read-index-unmerged.sh
> index bbd64587c..5034ed931 100755
> --- a/t/t1015-read-index-unmerged.sh
> +++ b/t/t1015-read-index-unmerged.sh
> @@ -30,7 +30,7 @@ test_expect_success 'setup modify/delete + directory/file
> conflict' '
> )
> '
>
> -test_expect_failure 'read-tree --reset cleans unmerged entries' '
> +test_expect_success 'read-tree --reset cleans unmerged entries' '
> test_when_finished "git -C df_plus_modify_delete clean -f" &&
> test_when_finished "git -C df_plus_modify_delete reset --hard" &&
> (
> @@ -45,7 +45,7 @@ test_expect_failure 'read-tree --reset cleans unmerged
> entries' '
> )
> '
>
> -test_expect_failure 'One reset --hard cleans unmerged entries' '
> +test_expect_success 'One reset --hard cleans unmerged entries' '
> test_when_finished "git -C df_plus_modify_delete clean -f" &&
> test_when_finished "git -C df_plus_modify_delete reset --hard" &&
> (
> @@ -87,7 +87,7 @@ test_expect_success 'setup directory/file conflict + simple
> edit/edit' '
> )
> '
>
> -test_expect_failure 'git merge --abort succeeds despite D/F conflict' '
> +test_expect_success 'git merge --abort succeeds despite D/F conflict' '
> test_when_finished "git -C df_plus_edit_edit clean
Re: [PATCH] sha1-name.c: for ":/", find detached HEAD commits
On Wed, Jul 11, 2018 at 05:45:09PM +0200, Duy Nguyen wrote: > > diff --git a/sha1-name.c b/sha1-name.c > > index 60d9ef3c7..641ca12f9 100644 > > --- a/sha1-name.c > > +++ b/sha1-name.c > > @@ -1650,6 +1650,7 @@ static int get_oid_with_context_1(const char *name, > > struct commit_list *list = NULL; > > > > for_each_ref(handle_one_ref, ); > > + head_ref(handle_one_ref, ); > > When multiple worktrees are used, should we consider all HEADs or just > current worktree's HEAD? > > This is the latter case, if we should go with the former (I don't > know, it's a genuine question) then you need one more call: > other_head_refs(). Oof, I didn't even think about other worktrees. I'd probably say "yes", as we consider them reachable (and really the intent of this feature seems to be "find it in any reachable commit"). -Peff
Re: [PATCH v2 6/9] gpg-interface: do not hardcode the key string len anymore
On Wed, Jul 11, 2018 at 06:15:05PM +0200, Henning Schild wrote:
> > diff --git a/gpg-interface.c b/gpg-interface.c
> > index bf8d567a4c..139b0f561e 100644
> > --- a/gpg-interface.c
> > +++ b/gpg-interface.c
> > @@ -97,7 +97,7 @@ static void parse_gpg_output(struct signature_check
> > *sigc) sigc->key = xmemdupz(found, next - found);
> > /* The ERRSIG message is not followed by
> > signer information */ if (sigc-> result != 'E') {
> > - found = next + 1;
> > + found = *next ? next + 1 : next;
>
> This would keep us in bounds of the unexpected string. But ignore the
> line instead of "complaining" or crashing.
>
> But you are right, it is easy enough and ignoring the line is probably
> the best way of dealing with it.
>
> i will change the condition to
> > if (*next && sigc-> result != 'E')
>
> also skipping the following strchrnul and xmemdupz
That sounds good to me. Thanks.
-Peff
Re: [PATCH v2 9/9] gpg-interface t: extend the existing GPG tests with GPGSM
Am Wed, 11 Jul 2018 10:33:52 -0400
schrieb Jeff King :
> On Tue, Jul 10, 2018 at 10:52:31AM +0200, Henning Schild wrote:
>
> > diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh
> > index a5d3b2cba..9dcb4e990 100755
> > --- a/t/lib-gpg.sh
> > +++ b/t/lib-gpg.sh
> > @@ -38,7 +38,14 @@ then
> > "$TEST_DIRECTORY"/lib-gpg/ownertrust &&
> > gpg --homedir "${GNUPGHOME}" /dev/null
> > 2>&1 \ --sign -u commit...@example.com &&
> > - test_set_prereq GPG
> > + test_set_prereq GPG &&
> > + echo | gpgsm --homedir "${GNUPGHOME}" -o
> > "$TEST_DIRECTORY"/lib-gpg/gpgsm.crt.user --passphrase-fd 0
> > --pinentry-mode loopback --generate-key --batch
> > "$TEST_DIRECTORY"/lib-gpg/gpgsm-gen-key.in &&
> > + gpgsm --homedir "${GNUPGHOME}" --import
> > "$TEST_DIRECTORY"/lib-gpg/gpgsm.crt.user &&
> > + gpgsm --homedir "${GNUPGHOME}" -K | grep
> > fingerprint: | cut -d" " -f4 | tr -d '\n' >
> > ${GNUPGHOME}/trustlist.txt &&
> > + echo " S relax" >> ${GNUPGHOME}/trustlist.txt &&
> > + (gpgconf --kill gpg-agent >/dev/null 2>&1 || : ) &&
> > + echo hello | gpgsm --homedir "${GNUPGHOME}" -u
> > commit...@example.com -o /dev/null --sign - 2>&1 &&
> > + test_set_prereq GPGSM
>
> By the way, I think these gpgsm invocations need some redirection of
> their stderr. This isn't inside a test_expect block, so they spew to
> the terminal even in non-verbose mode.
>
> Ideally they'd redirect to descriptor 4, which then respects "-v"
> properly (though the existing gpg invocations just go to 2).
Thanks for pointing that out. I decided to go for 2 as well, for
consistency with gpg and not having to change that.
Henning
> -Peff
Re: [PATCH] unpack-trees: do not fail reset because of unmerged skipped entry
Duy Nguyen writes: > On Tue, Jul 10, 2018 at 9:22 PM Max Kirillov wrote: >> >> On Sat, Jun 16, 2018 at 07:14:44AM +0200, Duy Nguyen wrote: >> > -- 8< -- >> > diff --git a/unpack-trees.c b/unpack-trees.c >> > index 3a85a02a77..eb544ee1b3 100644 >> > --- a/unpack-trees.c >> > +++ b/unpack-trees.c >> > @@ -1246,7 +1246,7 @@ static void mark_new_skip_worktree(struct >> > exclude_list *el, >> > if (select_flag && !(ce->ce_flags & select_flag)) >> > continue; >> > >> > - if (!ce_stage(ce)) >> > + if (!ce_stage(ce) && !(ce->ce_flags & CE_CONFLICTED)) >> > ce->ce_flags |= skip_wt_flag; >> > else >> > ce->ce_flags &= ~skip_wt_flag; >> > -- 8< -- >> >> I tried your fix and it is working. I put it instead of my original fix. >> Would you sign it off? > > Signed-off-by: Duy Nguyen Thanks, both.
Re: [PATCH] sha1-name.c: for ":/", find detached HEAD commits
Jeff King writes: > On Tue, Jul 10, 2018 at 11:18:22PM -0700, William Chargin wrote: > >> > Also, I am not sure if "or from HEAD" is even needed when we say >> > "from ANY ref" already, as we count things like HEAD as part of the >> > ref namespace. >> >> My two cents: with the docs as is, I wasn't sure whether HEAD was >> intended to count as a ref for this purpose. The gitglossary man page >> defines a ref as a "name that begins with refs/" (seemingly excluding >> HEAD), though it later says that HEAD is a "special-purpose ref". In my >> opinion, the change adds clarity without any particular downside---but >> I'm happy to revert it if you'd prefer. I'd also be happy to change the >> wording to something like "any ref, including HEAD" if we want to >> emphasize that HEAD really is a ref. > > FWIW, I think the clarification to include HEAD is helpful here, since > it took me a few minutes of thinking to decide whether the current > behavior was a bug or just a subtlety. Your "including HEAD" suggestion > seems like the best route to me. But I can live with it either way. > >> After reaching consensus on the change to the docs, should I send in a >> [PATCH v2] In-Reply-To this thread? > > Yes. > >> Peff, should I add your >> Signed-off-by to the commit message, or is that not how things are done? > > Yes, you can add in any sign-offs that have been explicitly given. It's > normal to order them chronologically, too (so mine would come first, > then yours, showing that the patch flowed through me to you; Junio will > add his at the end). Thanks, agreed 100% and I have nothing more to add.
Re: [PATCH v2 9/9] gpg-interface t: extend the existing GPG tests with GPGSM
Jeff King writes: >> While addressing 1 make 2 obvious and worse, addressing 2 is a whole >> different story and should probably be discussed outside of this >> thread. And i would not like to inherit responsibility for 2. In >> fact the whole discussion emphasizes that it was a good idea to make >> GPGSM depend on GPG, because it allows to somewhat reuse existing tests. > > IMHO there is a big difference between inheriting responsibility for > something, and not making it worse. Well said.
Re: [PATCH v2 6/9] gpg-interface: do not hardcode the key string len anymore
Am Wed, 11 Jul 2018 10:27:52 -0400
schrieb Jeff King :
> On Wed, Jul 11, 2018 at 03:46:19PM +0200, Henning Schild wrote:
>
> > > I think it's worth addressing in the near term, if only because
> > > this kind of off-by-one is quite subtle, and I don't want to
> > > forget to deal with it. Whether that happens as part of this
> > > patch, or as a cleanup before or after, I'm not picky. :)
> >
> > I get that and if anyone is willing to write that code, i will base
> > my patches on it. What i want to avoid is taking responsibility for
> > problems i did not introduce, just because i happen to work on that
> > code at the moment. Keeping track of that (not forgetting) is also
> > not for the random contributor like myself.
>
> It doesn't make sense to do a patch before your series, since it would
> just be:
>
> if (strlen(found) > 16)
> ...
Instead of randomly crashing on unexpected input, we would now silently
ignore it.
> which would get obliterated by your patch. The patch after is shown
> below. But frankly, it seems a lot easier to just handle this while
> you are rewriting the code.
>
> -- >8 --
> Subject: [PATCH] gpg-interface: handle off-by-one parsing gpg output
>
> When parsing gpg's VALIDSIG lines, we look for a space
> followed by the signer information. Because we use
> strchrnul(), though, if the space is missing we'll end up
> pointing to the trailing NUL. When we try to move past that
> space, we have to handle the NUL case separately to avoid
> accidentally stepping out of the string entirely.
True.
> Signed-off-by: Jeff King
> ---
> gpg-interface.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/gpg-interface.c b/gpg-interface.c
> index bf8d567a4c..139b0f561e 100644
> --- a/gpg-interface.c
> +++ b/gpg-interface.c
> @@ -97,7 +97,7 @@ static void parse_gpg_output(struct signature_check
> *sigc) sigc->key = xmemdupz(found, next - found);
> /* The ERRSIG message is not followed by
> signer information */ if (sigc-> result != 'E') {
> - found = next + 1;
> + found = *next ? next + 1 : next;
This would keep us in bounds of the unexpected string. But ignore the
line instead of "complaining" or crashing.
But you are right, it is easy enough and ignoring the line is probably
the best way of dealing with it.
i will change the condition to
> if (*next && sigc-> result != 'E')
also skipping the following strchrnul and xmemdupz
Henning
> next = strchrnul(found, '\n');
> sigc->signer = xmemdupz(found, next
> - found); }
Re: refs/notes/amlog problems, was Re: [PATCH v3 01/20] linear-assignment: a function to solve least-cost assignment problems
Johannes Schindelin writes: > To summarize, there are two commits recorded for that Message-Id, the > later one not mapped back, and neither is the correct commit that made it > into `master`. > > It would be nice to figure out what went wrong there, and how to fix it > for the future (and also to fix up the existing mis-mappings in `amlog`). I think what happened is that I used to have post-rewrite, but because it did not solve the real issue of multiple commits existing for the same message ID (either because of amending, or because of running "am" multiple times while looking for the best base to contruct a topic branch for the series that contains it) *and* the one that will eventually used in the final history may not be the last one (e.g. I may "am" twice to see if an older base I use in my second attempt is a better one than the base I originally used, and the patches may even apply cleanly to the older history, but may turn out to need semantic adjustment, at which point I would discard that second attempt and use the old commit from the first attempt that built on a newer base), I stopped using it. The mid-to-commit, for it to be relialble, needs to keep mapping for all the commits created from a single message, instead of being the last-one-survives mapping. I just didn't have that much interest back when I decided it was not worth and dropped the post-rewrite, I think.
Re: BUG: Segfault on "git pull" on "bad object HEAD"
On Wed, Jul 11, 2018 at 1:02 PM Ævar Arnfjörð Bjarmason
wrote:
>
> This segfaults, but should print an error instead, have a repo with a
> corrupt HEAD:
>
> (
> rm -rf /tmp/git &&
> git clone --single-branch --branch todo g...@github.com:git/git.git
> /tmp/git &&
> echo
> >/tmp/git/.git/refs/heads/todo &&
> git -C /tmp/git pull
> )
>
> On this repository e.g. "git log" will print "fatal: bad object HEAD",
> but for some reason "git pull" makes it this far:
>
> $ git pull
> Segmentation fault
>
> The immediate reason is that in run_diff_index() we have this:
>
> ent = revs->pending.objects;
>
> And that in this case that's NULL:
Probably because add_head_to_pending() in has_uncommitted_change()
does not add anything to the "pending" list because HEAD is broken.
I think if we make add_head_to_pending() return a boolean, then we can
check that if no HEAD is added, there's no point to run_diff_index and
has_uncommitted_changes() can return 0 immediately.
A new BUG() could still be added in run_diff_index() though, to check
if revs->pending.nr is non-zero before attempting to access
revs->pending.objects.
>
> (gdb) bt
> #0 0x5565993f in run_diff_index (revs=0x7fffcb90, cached=1)
> at diff-lib.c:524
> #1 0x557633da in has_uncommitted_changes (ignore_submodules=1)
> at wt-status.c:2345
> #2 0x557634c9 in require_clean_work_tree (action=0x55798f18
> "pull with rebase", hint=0x55798efb "please commit or stash them.",
> ignore_submodules=1, gently=0) at wt-status.c:2370
> #3 0x555dbdee in cmd_pull (argc=0, argv=0x7fffd868,
> prefix=0x0) at builtin/pull.c:885
> #4 0x5556c9da in run_builtin (p=0x55a2de50 ,
> argc=1, argv=0x7fffd868) at git.c:417
> #5 0x5556cce2 in handle_builtin (argc=1, argv=0x7fffd868) at
> git.c:633
> #6 0x5556ce8a in run_argv (argcp=0x7fffd71c,
> argv=0x7fffd710) at git.c:685
> #7 0x5556d03f in cmd_main (argc=1, argv=0x7fffd868) at
> git.c:762
> #8 0x55611786 in main (argc=3, argv=0x7fffd858) at
> common-main.c:45
> (gdb) p revs
> $4 = (struct rev_info *) 0x7fffcb90
> (gdb) p revs->pending
> $5 = {nr = 0, alloc = 0, objects = 0x0}
> (gdb)
>
> This has been an issue since at least v2.8.0 (didn't test back
> further). I'm not familiar with the status / diff code, so I'm not sure
> where the assertion should be added.
>
> This came up in the wild due to a user with a corrupt repo (don't know
> how it got corrupt) trying "git pull" and seeing git segfault.
--
Duy
Re: [PATCH v2 9/9] gpg-interface t: extend the existing GPG tests with GPGSM
Am Wed, 11 Jul 2018 10:35:54 -0400 schrieb Jeff King : > On Wed, Jul 11, 2018 at 03:40:19PM +0200, Henning Schild wrote: > > > > So it may be simplest to just run most of the tests twice, once > > > with gpg and once with gpgsm. I kind of wonder if all of t7510 > > > could just be bumped into a function. Or even into a sourced file > > > and run from two different scripts. See the way that t8001 and > > > t8002 use annotate-tests.sh for an example. > > > > I do not agree and would like to leave the tests as they are. > > Instead of introducing a whole lot of very similar copies, i added > > just a few. > > I'm not sure I understand why you added the ones you did, though. For > instance, "--no-show-signature overrides --show-signature x509" seems > like it has nothing to do with the gpg/gpgsm distinction. > > So I'd have expected that to be _outside_ of the shared battery of > tests. True, it took my quite some time to figure out a way to configure gpgsm non-interactively. Generate the key etc. without even a single popup of the gpg-agent... After that i just added random tests to create "coverage", without much focus. I would be happy to revisit that and drop test cases, and add some that are missing. Henning > > The original ones are even very similar between each other. > > We are again talking about two problems. 1. we need test cases for > > gpgsm if we want to merge gpgsm 2. the testsuite is very repetitive > > > > While addressing 1 make 2 obvious and worse, addressing 2 is a whole > > different story and should probably be discussed outside of this > > thread. And i would not like to inherit responsibility for 2. In > > fact the whole discussion emphasizes that it was a good idea to make > > GPGSM depend on GPG, because it allows to somewhat reuse existing > > tests. > > IMHO there is a big difference between inheriting responsibility for > something, and not making it worse. But I'm not all that interested in > fighting about it. > > -Peff
Re: [PATCH] sha1-name.c: for ":/", find detached HEAD commits
On Tue, Jul 10, 2018 at 5:44 PM William Chargin wrote:
>
> This patch broadens the set of commits matched by ":/" pathspecs to
> include commits reachable from HEAD but not any named ref. This avoids
> surprising behavior when working with a detached HEAD and trying to
> refer to a commit that was recently created and only exists within the
> detached state.
Cool!
>
> Signed-off-by: William Chargin
> Based-on-patch-by: Jeff King
> ---
> Documentation/revisions.txt | 10 +-
> sha1-name.c | 1 +
> t/t4208-log-magic-pathspec.sh | 14 ++
> 3 files changed, 20 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/revisions.txt b/Documentation/revisions.txt
> index 7d1bd4409..aa56907fd 100644
> --- a/Documentation/revisions.txt
> +++ b/Documentation/revisions.txt
> @@ -181,11 +181,11 @@ existing tag object.
>the '' before '{caret}'.
>
> ':/', e.g. ':/fix nasty bug'::
> - A colon, followed by a slash, followed by a text, names
> - a commit whose commit message matches the specified regular expression.
> - This name returns the youngest matching commit which is
> - reachable from any ref. The regular expression can match any part of the
> - commit message. To match messages starting with a string, one can use
> + A colon, followed by a slash, followed by a text, names a commit whose
> + commit message matches the specified regular expression. This name
> + returns the youngest matching commit which is reachable from any ref
> + or from HEAD. The regular expression can match any part of the commit
> + message. To match messages starting with a string, one can use
>e.g. ':/^foo'. The special sequence ':/!' is reserved for modifiers to what
>is matched. ':/!-foo' performs a negative match, while ':/!!foo' matches a
>literal '!' character, followed by 'foo'. Any other sequence beginning with
> diff --git a/sha1-name.c b/sha1-name.c
> index 60d9ef3c7..641ca12f9 100644
> --- a/sha1-name.c
> +++ b/sha1-name.c
> @@ -1650,6 +1650,7 @@ static int get_oid_with_context_1(const char *name,
> struct commit_list *list = NULL;
>
> for_each_ref(handle_one_ref, );
> + head_ref(handle_one_ref, );
When multiple worktrees are used, should we consider all HEADs or just
current worktree's HEAD?
This is the latter case, if we should go with the former (I don't
know, it's a genuine question) then you need one more call:
other_head_refs().
> commit_list_sort_by_date();
> return get_oid_oneline(name + 2, oid, list);
> }
> diff --git a/t/t4208-log-magic-pathspec.sh b/t/t4208-log-magic-pathspec.sh
> index 62f335b2d..41b9f3eba 100755
> --- a/t/t4208-log-magic-pathspec.sh
> +++ b/t/t4208-log-magic-pathspec.sh
> @@ -25,6 +25,20 @@ test_expect_success '"git log :/a -- " should not be
> ambiguous' '
> git log :/a --
> '
>
> +test_expect_success '"git log :/detached -- " should find a commit only in
> HEAD' '
> + test_when_finished "git checkout master" &&
> + git checkout --detach &&
> + test_tick &&
> + git commit --allow-empty -m detached &&
> + test_tick &&
> + git commit --allow-empty -m something-else &&
> + git log :/detached --
> +'
> +
> +test_expect_success '"git log :/detached -- " should not find an orphaned
> commit' '
> + test_must_fail git log :/detached --
> +'
> +
> test_expect_success '"git log -- :/a" should not be ambiguous' '
> git log -- :/a
> '
> --
> 2.18.0.130.g61d0c9dcf
>
--
Duy
Re: [PATCH 0/6] Compile cleanly in pedantic mode
Beat Bolli writes: > Should we add a "pedantic" flag to DEVOPTS that would simplify > building pedantically? It would also have to set > USE_PARENS_AROUND_GETTEXT_N so as to not overwhelm the developer with > too much output. That may be something worth discussing before doing; I'd prefer to wait until these 6 patches, plus the unsized static array one you did spearately, graduates to the 'master' branch.
Re: [PATCH 0/6] Add merge recursive testcases with undetected conflicts
Elijah Newren writes: > On Mon, Jul 9, 2018 at 1:22 PM, Elijah Newren wrote: >> On Mon, Jul 9, 2018 at 10:53 AM, Junio C Hamano wrote: >>> Elijah Newren writes: >>> When a merge succeeds, we expect the resulting contents to depend only upon the trees and blobs of the branches involved and of their merge base(s). Unfortunately, there are currently about half a dozen cases where the contents of a "successful" merge depend on the relative commit timestamps of the merge bases. Document these with testcases. (This series came out of looking at modifying how file collision conflict types are handled, as discussed at [1]. I discovered these issues while working on that topic.) >>> >>> I have a topic branch for this series but not merged to 'pu' as >>> test-lint gives these: >>> > ... >> >> ... here's a fixup to the topic; as you pointed out, the exact contents >> of the script being written were actually irrelevant; it was just an >> input to a merge. >> >> -- 8< -- >> Subject: [PATCH] fixup! t6036: add a failed conflict detection case: regular >> files, different modes >> > > Does a 'fixup!' commit require a Signed-off-by? Just realized that > this one didn't have it, though I don't know if it's necessary. If it > is: > > Signed-off-by: Elijah Newren Thanks. I queued it separately before running out of time Monday, but will actually squash it in to the main patch.
Re: [PATCH v2 0/2] de-confuse git cherry-pick --author
Johannes Schindelin writes: >> After poking at it a bit more, I've convinced myself that this is the >> right thing, as options like "--branches" which expand into multiple >> tips already push us into the other code path. >> >> So here's a re-roll. The first one is identical except for the typo-fix >> in the commit message. >> >> [1/2]: sequencer: handle empty-set cases consistently >> [2/2]: sequencer: don't say BUG on bogus input >> >> sequencer.c | 12 >> t/t3510-cherry-pick-sequence.sh | 7 ++- >> 2 files changed, 14 insertions(+), 5 deletions(-) > > ACK, > Dscho Thanks, both. Queued and pushed out.
Re: [PATCH 07/17] commit: increase commit message buffer size
"brian m. carlson" writes: > On Tue, Jul 10, 2018 at 02:08:28PM -0400, Ben Peart wrote: >> >> >> On 7/9/2018 7:39 PM, brian m. carlson wrote: >> > On Mon, Jul 09, 2018 at 10:45:33AM -0700, Junio C Hamano wrote: >> > > As Brandon alludes to downthread, we probably should use strbuf for >> > > things like this these days, so a preliminary clean-up to do so is >> > > probably a welcome change to sneak in and rebase this series on top >> > > of. >> > >> > Sure, I agree that would be a better change, and I'm happy to reroll >> > with that. >> > >> >> I've put together a patch to update log_ref_write_fd() to use strbuf and >> will submit it shortly. > > Excellent. I'll drop this patch in that case. OK, thanks for working well together. In the integration run I did yesterday evening, this part was resolved by taking what Ben did, dropping the s/100/200/ change from this series, and the result looked reasonable, of course.
Re: [PATCH] gc --auto: release pack files before auto packing
On Mon, Jul 9, 2018 at 11:10 PM Junio C Hamano wrote:
>
> Duy Nguyen writes:
>
> > On Sun, Jul 8, 2018 at 1:16 AM Kim Gybels wrote:
> >> Should I post a v3 that goes back to the original fix, but uses
> >> test_i18ngrep instead of grep?
> >
> > Yes please. In my comment I did write we didn't need the repo anymore
> > (or something along that line) which turns out to be wrong.
> >
> >> In addition to not breaking any tests, close_all_packs is already used
> >> in a similar way in am and fetch just before running "gc --auto".
> >>
> >> -Kim
>
> Sound good.
>
> I recall that "clear repo should treat the_repository special" was
> discussed when we saw the patch that became 74373b5f ("repository:
> fix free problem with repo_clear(the_repository)", 2018-05-10),
> instead of treating only the index portion specially. Perhaps it
> was a more correct approach after all?
I think it's good that we have a way to "shut down the repo" when we
run an external command. But what we lack is "reinitialize the repo"
after the external command is done. We could treat the_repository
special in this case so shutting down does not require
reinitialization. Then repo_clear() should work well here. We could
also add a flag in repo_clear() to say "release all the resources you
are holding, but keep the repo settings/location..., we're not done
with this repo yet" then we don't need to re-initialize the repo
afterwards and still don't make the_repository so special.
--
Duy
Re: [PATCH] unpack-trees: do not fail reset because of unmerged skipped entry
On Tue, Jul 10, 2018 at 9:22 PM Max Kirillov wrote: > > On Sat, Jun 16, 2018 at 07:14:44AM +0200, Duy Nguyen wrote: > > -- 8< -- > > diff --git a/unpack-trees.c b/unpack-trees.c > > index 3a85a02a77..eb544ee1b3 100644 > > --- a/unpack-trees.c > > +++ b/unpack-trees.c > > @@ -1246,7 +1246,7 @@ static void mark_new_skip_worktree(struct > > exclude_list *el, > > if (select_flag && !(ce->ce_flags & select_flag)) > > continue; > > > > - if (!ce_stage(ce)) > > + if (!ce_stage(ce) && !(ce->ce_flags & CE_CONFLICTED)) > > ce->ce_flags |= skip_wt_flag; > > else > > ce->ce_flags &= ~skip_wt_flag; > > -- 8< -- > > I tried your fix and it is working. I put it instead of my original fix. > Would you sign it off? Signed-off-by: Duy Nguyen -- Duy
Re: [PATCH] has_uncommitted_changes(): fall back to empty tree
On Wed, Jul 11, 2018 at 04:41:02PM +0200, Ævar Arnfjörð Bjarmason wrote: > > [From upthread]: It took me a minute to reproduce this. It needs "pull > > --rebase" if you don't have that setup in your config. > > Yeah, sorry about that. I tested without --rebase while simplifying the > testcase, but forgot that I have pull.rebase=true in my ~/.gitconfig, so > of course that didn't imply --no-rebase. In the grand scheme of things, it was still a pretty good bug report. :) Having the stack trace meant that the head-scratching was "why am I not calling the problem function", rather than "why am I not reproducing", which is much easier to track down. -Peff
Re: [PATCH v2 0/3] Fix occasional test failures in http tests
On Wed, Jul 11, 2018 at 02:56:44PM +0200, SZEDER Gábor wrote: > 't5561-http-backend.sh' is prone to occasional failures; luckily it's > not 'git-http-backend's fault, but the test script is a bit racy. I > won't go into the details here, patch 3/3's commit message discusses > it at length. > > v1 is here; it haven't been picked up by Junio: > > > https://public-inbox.org/git/20180614123107.11608-1-szeder@gmail.com/T/#u > > The first two patches are identical to those in v1, and the last patch > implements a different and simpler fix than in v1, following Peff's > suggestion. The third patch was dropped, because it's not necessary for > this simpler fix. The first two look obviously correct as before. I picked a few minor nits in the third one. But if your response is "no, I prefer it the way I wrote it", then I am perfectly happy seeing it applied as-is. Thanks for finishing this up. -Peff
Re: [PATCH v2 3/3] t/lib-httpd: avoid occasional failures when checking access.log
On Wed, Jul 11, 2018 at 02:56:47PM +0200, SZEDER Gábor wrote:
> +# Requires one argument: the name of a file containing the expected stripped
> +# access log entries.
> +check_access_log() {
> + sort "$1" >"$1".sorted &&
> + strip_access_log >access.log.stripped &&
> + sort access.log.stripped >access.log.sorted &&
> + if ! test_cmp "$1".sorted access.log.sorted
> + then
> + test_cmp "$1" access.log.stripped
> + fi
> +}
This will generate output showing both the unsorted and sorted
differences. Do we want to suppress the sorted ones (e.g., by just using
"cmp" directly)? I guess it doesn't matter unless there is an actual
test failure, but I just wonder if it would be confusing to see both.
> diff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh
> index 6cd986797d..a481e3989a 100755
> --- a/t/t5541-http-push-smart.sh
> +++ b/t/t5541-http-push-smart.sh
> @@ -43,15 +43,13 @@ test_expect_success 'no empty path components' '
> cd "$ROOT_PATH" &&
> git clone $HTTPD_URL/smart/test_repo.git/ test_repo_clone &&
>
> - strip_access_log >act &&
> + check_access_log exp
> +'
>
> +test_expect_success 'clear access log' '
> # Clear the log, so that it does not affect the "used receive-pack
> # service" test which reads the log too.
> - #
> - # We do this before the actual comparison to ensure the log is cleared.
> - >"$HTTPD_ROOT_PATH"/access.log &&
> -
> - test_cmp exp act
> + >"$HTTPD_ROOT_PATH"/access.log
> '
This took some head-scratching, mostly because of the original comment.
I thought the "before" here was referring to a timing issue (maybe
because we're talking about timing ;) ).
But it is really "make sure that a failed test here does not prevent us
from doing this cleanup". So the original really should have just
dropped that comment and added a test_when_finished. Bumping it into a
separate test as you have here accomplishes the same thing.
-Peff
Re: [RFC PATCH 4/4] t/lib-httpd: sort log based on timestamp to avoid occasional failure
On Wed, Jul 11, 2018 at 02:53:23PM +0200, SZEDER Gábor wrote: > > I.e., could we have a situation where we make a request, the client > > finishes, and then we look at the logs, but nothing has been written by > > apache yet? > > That's possible, I suppose. Highly improbable, but possible. Or the > log entries can be written only partially by the time we look at them. Digging into earlier cases I had seen on my local box, my impression was that this was the problem I was seeing (not reordering). But it's entirely possible that I was confused by reordering. I've been trying to reproduce, but after 5 minutes of t5561 running in a tight loop under high load, I haven't gotten a single failure. > Anyway, I would prefer not to do anything about it just yet, because > I'm curious :) and I'd like to see whether this actually happens in > practice. I can't reproduce for now, so I'm happy either way. :) > > What if the test script provides the in-order expectation, but we check > > only the unordered version (by sorting both actual and expected output > > on the fly)? That would give us a more relaxed check most of the time, > > but somebody digging in to a failure could run the ordered diff (or we > > could even show it automatically on failure instead of just using > > test_cmp). > > this is the fix I'll go for in v2. Great. That seems much simpler to reason about. I'll take a look. -Peff
Re: [PATCH] has_uncommitted_changes(): fall back to empty tree
On Wed, Jul 11 2018, Jeff King wrote:
> On Wed, Jul 11, 2018 at 09:34:02AM -0400, Jeff King wrote:
>
>> I do worry that other callers of run_diff_index() might have similar
>> problems, though. Grepping around, the other callers seem to fall into
>> one of three categories:
>>
>> - they resolve the object themselves and put it in the pending list
>>(and often fallback to the empty tree, which is more or less what the
>>patch above is doing)
>>
>> - they resolve the object themselves and avoid calling run_diff_index()
>>if it's not valid
>>
>> - they use setup_revisions(), which will barf on the broken object
>>
>> So I think this may be sufficient. We probably should also add an
>> assertion to run_diff_index(), since that's better than segfaulting.
>
> Here's a patch to do that. I tweaked it slightly from what I showed
> earlier to use the empty tree, which matches what other code (e.g.,
> git-diff) would do.
>
> -- >8 --
> Subject: has_uncommitted_changes(): fall back to empty tree
>
> If has_uncommitted_changes() can't resolve HEAD (e.g.,
> because it's unborn or corrupt), then we end up calling
> run_diff_index() with an empty revs.pending array. This
> causes a segfault, as run_diff_index() blindly looks at the
> first pending item.
>
> Fixing this raises a question of fault: should
> run_diff_index() handle this case, or is the caller wrong to
> pass an empty pending list?
>
> Looking at the other callers of run_diff_index(), they
> handle this in one of three ways:
>
> - they resolve the object themselves, and avoid doing the
>diff if it's not valid
>
> - they resolve the object themselves, and fall back to the
>empty tree
>
> - they use setup_revisions(), which will die() if the
>object isn't valid
>
> Since this is the only broken caller, that argues that the
> fix should go there. Falling back to the empty tree makes
> sense here, as we'd claim uncommitted changes if and only if
> the index is non-empty. This may be a little funny in the
> case of corruption (the corrupt HEAD probably _isn't_
> empty), but:
>
> - we don't actually know the reason here that HEAD didn't
> resolve (the much more likely case is that we have an
> unborn HEAD, in which case the empty tree comparison is
> the right thing)
>
> - this matches how other code, like "git diff", behaves
>
> While we're thinking about it, let's add an assertion to
> run_diff_index(). It should always be passed a single
> object, and as this bug shows, it's easy to get it wrong
> (and an assertion is easier to hunt down than a segfault, or
> a quietly ignored extra tree).
>
> Reported-by: Ævar Arnfjörð Bjarmason
> Signed-off-by: Jeff King
> ---
> diff-lib.c | 3 +++
> t/t5520-pull.sh | 12
> wt-status.c | 10 ++
> 3 files changed, 25 insertions(+)
>
> diff --git a/diff-lib.c b/diff-lib.c
> index a9f38eb5a3..732f684a49 100644
> --- a/diff-lib.c
> +++ b/diff-lib.c
> @@ -520,6 +520,9 @@ int run_diff_index(struct rev_info *revs, int cached)
> struct object_array_entry *ent;
> uint64_t start = getnanotime();
>
> + if (revs->pending.nr != 1)
> + BUG("run_diff_index must be passed exactly one tree");
> +
> ent = revs->pending.objects;
> if (diff_cache(revs, >item->oid, ent->name, cached))
> exit(128);
> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
> index 59c4b778d3..68aa5f0340 100755
> --- a/t/t5520-pull.sh
> +++ b/t/t5520-pull.sh
> @@ -618,6 +618,18 @@ test_expect_success 'pull --rebase fails on unborn
> branch with staged changes' '
> )
> '
>
> +test_expect_success 'pull --rebase fails on corrupt HEAD' '
> + test_when_finished "rm -rf corrupt" &&
> + git init corrupt &&
> + (
> + cd corrupt &&
> + test_commit one &&
> + obj=$(git rev-parse --verify HEAD | sed "s#^..#&/#") &&
> + rm -f .git/objects/$obj &&
> + test_must_fail git pull --rebase
> + )
> +'
> +
> test_expect_success 'setup for detecting upstreamed changes' '
> mkdir src &&
> (cd src &&
> diff --git a/wt-status.c b/wt-status.c
> index d1c05145a4..d89c41ba10 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -2340,7 +2340,17 @@ int has_uncommitted_changes(int ignore_submodules)
> if (ignore_submodules)
> rev_info.diffopt.flags.ignore_submodules = 1;
> rev_info.diffopt.flags.quick = 1;
> +
> add_head_to_pending(_info);
> + if (!rev_info.pending.nr) {
> + /*
> + * We have no head (or it's corrupt); use the empty tree,
> + * which will complain if the index is non-empty.
> + */
> + struct tree *tree = lookup_tree(the_hash_algo->empty_tree);
> + add_pending_object(_info, >object, "");
> + }
> +
> diff_setup_done(_info.diffopt);
> result = run_diff_index(_info, 1);
> return diff_result_code(_info.diffopt, result);
Thanks a lot.
Re: [PATCH v2 9/9] gpg-interface t: extend the existing GPG tests with GPGSM
On Wed, Jul 11, 2018 at 03:40:19PM +0200, Henning Schild wrote: > > So it may be simplest to just run most of the tests twice, once with > > gpg and once with gpgsm. I kind of wonder if all of t7510 could just > > be bumped into a function. Or even into a sourced file and run from > > two different scripts. See the way that t8001 and t8002 use > > annotate-tests.sh for an example. > > I do not agree and would like to leave the tests as they are. Instead > of introducing a whole lot of very similar copies, i added just a few. I'm not sure I understand why you added the ones you did, though. For instance, "--no-show-signature overrides --show-signature x509" seems like it has nothing to do with the gpg/gpgsm distinction. So I'd have expected that to be _outside_ of the shared battery of tests. > The original ones are even very similar between each other. > We are again talking about two problems. 1. we need test cases for > gpgsm if we want to merge gpgsm 2. the testsuite is very repetitive > > While addressing 1 make 2 obvious and worse, addressing 2 is a whole > different story and should probably be discussed outside of this > thread. And i would not like to inherit responsibility for 2. In > fact the whole discussion emphasizes that it was a good idea to make > GPGSM depend on GPG, because it allows to somewhat reuse existing tests. IMHO there is a big difference between inheriting responsibility for something, and not making it worse. But I'm not all that interested in fighting about it. -Peff
Re: [PATCH v2 9/9] gpg-interface t: extend the existing GPG tests with GPGSM
On Tue, Jul 10, 2018 at 10:52:31AM +0200, Henning Schild wrote:
> diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh
> index a5d3b2cba..9dcb4e990 100755
> --- a/t/lib-gpg.sh
> +++ b/t/lib-gpg.sh
> @@ -38,7 +38,14 @@ then
> "$TEST_DIRECTORY"/lib-gpg/ownertrust &&
> gpg --homedir "${GNUPGHOME}" /dev/null 2>&1 \
> --sign -u commit...@example.com &&
> - test_set_prereq GPG
> + test_set_prereq GPG &&
> + echo | gpgsm --homedir "${GNUPGHOME}" -o
> "$TEST_DIRECTORY"/lib-gpg/gpgsm.crt.user --passphrase-fd 0 --pinentry-mode
> loopback --generate-key --batch "$TEST_DIRECTORY"/lib-gpg/gpgsm-gen-key.in &&
> + gpgsm --homedir "${GNUPGHOME}" --import
> "$TEST_DIRECTORY"/lib-gpg/gpgsm.crt.user &&
> + gpgsm --homedir "${GNUPGHOME}" -K | grep fingerprint: | cut -d"
> " -f4 | tr -d '\n' > ${GNUPGHOME}/trustlist.txt &&
> + echo " S relax" >> ${GNUPGHOME}/trustlist.txt &&
> + (gpgconf --kill gpg-agent >/dev/null 2>&1 || : ) &&
> + echo hello | gpgsm --homedir "${GNUPGHOME}" -u
> commit...@example.com -o /dev/null --sign - 2>&1 &&
> + test_set_prereq GPGSM
By the way, I think these gpgsm invocations need some redirection of
their stderr. This isn't inside a test_expect block, so they spew to the
terminal even in non-verbose mode.
Ideally they'd redirect to descriptor 4, which then respects "-v"
properly (though the existing gpg invocations just go to 2).
-Peff
Re: [PATCH v2 6/9] gpg-interface: do not hardcode the key string len anymore
On Wed, Jul 11, 2018 at 03:46:19PM +0200, Henning Schild wrote:
> > I think it's worth addressing in the near term, if only because this
> > kind of off-by-one is quite subtle, and I don't want to forget to deal
> > with it. Whether that happens as part of this patch, or as a cleanup
> > before or after, I'm not picky. :)
>
> I get that and if anyone is willing to write that code, i will base my
> patches on it. What i want to avoid is taking responsibility for
> problems i did not introduce, just because i happen to work on that
> code at the moment. Keeping track of that (not forgetting) is also not
> for the random contributor like myself.
It doesn't make sense to do a patch before your series, since it would
just be:
if (strlen(found) > 16)
...
which would get obliterated by your patch. The patch after is shown
below. But frankly, it seems a lot easier to just handle this while you
are rewriting the code.
-- >8 --
Subject: [PATCH] gpg-interface: handle off-by-one parsing gpg output
When parsing gpg's VALIDSIG lines, we look for a space
followed by the signer information. Because we use
strchrnul(), though, if the space is missing we'll end up
pointing to the trailing NUL. When we try to move past that
space, we have to handle the NUL case separately to avoid
accidentally stepping out of the string entirely.
Signed-off-by: Jeff King
---
gpg-interface.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/gpg-interface.c b/gpg-interface.c
index bf8d567a4c..139b0f561e 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -97,7 +97,7 @@ static void parse_gpg_output(struct signature_check *sigc)
sigc->key = xmemdupz(found, next - found);
/* The ERRSIG message is not followed by signer
information */
if (sigc-> result != 'E') {
- found = next + 1;
+ found = *next ? next + 1 : next;
next = strchrnul(found, '\n');
sigc->signer = xmemdupz(found, next - found);
}
--
2.18.0.400.g702e398724
Re: [GSoC][PATCH v3 10/13] rebase--interactive: rewrite complete_action() in C
Hi Junio,
Le 11/07/2018 à 00:33, Junio C Hamano a écrit :
> Alban Gruin writes:
>> -complete_action
>> +exec git rebase--helper --complete-action "$shortrevisions"
>> "$onto_name" \
>> +"$shortonto" "$orig_head" "$cmd" $allow_empty_message \
>> +${autosquash:+--autosquash} ${keep_empty:+--keep-empty} \
>> +${verbose:+--verbose} ${force_rebase:+--no-ff}
>
> The $allow_empty_message and later options are all dashed ones.
> git-rebase.sh gives us either empty or --allow-empty-message in the
> variable for $allow_empty_message, and if you follow suit to prepare
> all the other variables that way, the ugly ${frotz+=--frotz} dance
> will all become unnecessary here.
>
Good idea.
>> +int complete_action(struct replay_opts *opts, unsigned flags,
>> +const char *shortrevisions, const char *onto_name,
>> +const char *onto, const char *orig_head, const char *cmd,
>> +unsigned autosquash, unsigned keep_empty)
>> +{
>> +const char *shortonto, *todo_file = rebase_path_todo();
>> +struct todo_list todo_list = TODO_LIST_INIT;
>> +struct strbuf *buf = &(todo_list.buf);
>> +struct object_id oid;
>> +struct stat st;
>> +
>> +get_oid(onto, );
>> +shortonto = find_unique_abbrev(, DEFAULT_ABBREV);
>> +
>> +if (!lstat(todo_file, ) && st.st_size == 0 &&
>> +write_message("noop\n", 5, todo_file, 0))
>> +return error_errno(_("could not write '%s'"), todo_file);
>
> Wait a minute... thinking back to the early "age-old ommission"
> topic, can the todo file be a non-empty file that does not have any
> command (e.g. just a single blank line, or full of comments and
> nothing else)? The original wouldn't have added "noop" and then the
> first "do we have anything to do?" check would still have been
> necessary, which would mean that ff74126c's not removing the first
> check was actually not a bug but was a cautious and sensible thing
> to do, and removal of that exact check by this commit becomes a
> regression. So,... can the todo file be a non-empty file that does
> not have any command in it at this point?
>
Hum, yes, if the commits are empty, and if rebase was invoked without
the `--keep-empty` flag. In this case, it would die with the message
“Nothing to do”, instead of dropping the commit altogether.
I will add a test case in the next iteration.
>> +if (autosquash && rearrange_squash())
>> +return 0;
>
> The original, when rearrange-squash failed, exited with failure,
> like so:
>
> - test -z "$autosquash" || git rebase--helper --rearrange-squash || exit
>
> Now this function returns success when rearrange_squash fails.
> Is that intended?
>
Yes, it is. I just saw in the man page that `exit` returns the last
exit status.
>> +if (cmd && *cmd)
>
> Shouldn't it be a BUG (programming error) if cmd == NULL? I thought
> the caller of complete_action() in this patch insisted that it got
> argc == 6 or something, so *cmd might be NUL but cmd would never be
> NULL if I understand your code correctly. IOW, shouldn't the line
> be more like:
>
> if (!cmd)
> BUG("...");
> if (*cmd)
>
> ?
>
I don’t know, it’s not really problematic if cmd is NULL. And I think
that when interactive rebase will be a builtin, it will be possible for
cmd to be NULL.
>
>> +if (strbuf_read_file(buf, todo_file, 0) < 0)
>> +return error_errno(_("could not read '%s'."), todo_file);
>> +if (parse_insn_buffer(buf->buf, _list)) {
>
> Nice that we have parse* function. We do not have to work with
> stripspace plus "wc -l" ;-).
>
>> +todo_list_release(_list);
>> +return error(_("unusable todo list: '%s'"), todo_file);
>
> When the users see this error message, is it easy for them to
> diagnose what went wrong (e.g. has parse_insn_buffer() already
> issued some message to help pinpoint which insn in the todo list is
> misspelt, for example)?
>
Yes, parse_insn_buffer() will say which line caused the error. But at
this point, the user should not have changed the todo-list, so this
error should never appear.
Perhaps this is a good use case of BUG()?
>> +}
>> +
>> +strbuf_addch(buf, '\n');
>> +strbuf_commented_addf(buf, Q_("Rebase %s onto %s (%d command)",
>> + "Rebase %s onto %s (%d commands)",
>> + todo_list.nr),
>> + shortrevisions, shortonto, todo_list.nr);
>> +append_todo_help(0, keep_empty, buf);
>> +
>> +if (write_message(buf->buf, buf->len, todo_file, 0)) {
>> +todo_list_release(_list);
>> +return error_errno(_("could not write '%s'"), todo_file);
>> +}
>> +copy_file(rebase_path_todo_backup(), todo_file, 0666);
>> +transform_todos(flags | TODO_LIST_SHORTEN_IDS);
>> +
>
> It is a bit sad that we are mostly working on the byte array
> buf->buf (or
[PATCH] has_uncommitted_changes(): fall back to empty tree
On Wed, Jul 11, 2018 at 09:34:02AM -0400, Jeff King wrote:
> I do worry that other callers of run_diff_index() might have similar
> problems, though. Grepping around, the other callers seem to fall into
> one of three categories:
>
> - they resolve the object themselves and put it in the pending list
>(and often fallback to the empty tree, which is more or less what the
>patch above is doing)
>
> - they resolve the object themselves and avoid calling run_diff_index()
>if it's not valid
>
> - they use setup_revisions(), which will barf on the broken object
>
> So I think this may be sufficient. We probably should also add an
> assertion to run_diff_index(), since that's better than segfaulting.
Here's a patch to do that. I tweaked it slightly from what I showed
earlier to use the empty tree, which matches what other code (e.g.,
git-diff) would do.
-- >8 --
Subject: has_uncommitted_changes(): fall back to empty tree
If has_uncommitted_changes() can't resolve HEAD (e.g.,
because it's unborn or corrupt), then we end up calling
run_diff_index() with an empty revs.pending array. This
causes a segfault, as run_diff_index() blindly looks at the
first pending item.
Fixing this raises a question of fault: should
run_diff_index() handle this case, or is the caller wrong to
pass an empty pending list?
Looking at the other callers of run_diff_index(), they
handle this in one of three ways:
- they resolve the object themselves, and avoid doing the
diff if it's not valid
- they resolve the object themselves, and fall back to the
empty tree
- they use setup_revisions(), which will die() if the
object isn't valid
Since this is the only broken caller, that argues that the
fix should go there. Falling back to the empty tree makes
sense here, as we'd claim uncommitted changes if and only if
the index is non-empty. This may be a little funny in the
case of corruption (the corrupt HEAD probably _isn't_
empty), but:
- we don't actually know the reason here that HEAD didn't
resolve (the much more likely case is that we have an
unborn HEAD, in which case the empty tree comparison is
the right thing)
- this matches how other code, like "git diff", behaves
While we're thinking about it, let's add an assertion to
run_diff_index(). It should always be passed a single
object, and as this bug shows, it's easy to get it wrong
(and an assertion is easier to hunt down than a segfault, or
a quietly ignored extra tree).
Reported-by: Ævar Arnfjörð Bjarmason
Signed-off-by: Jeff King
---
diff-lib.c | 3 +++
t/t5520-pull.sh | 12
wt-status.c | 10 ++
3 files changed, 25 insertions(+)
diff --git a/diff-lib.c b/diff-lib.c
index a9f38eb5a3..732f684a49 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -520,6 +520,9 @@ int run_diff_index(struct rev_info *revs, int cached)
struct object_array_entry *ent;
uint64_t start = getnanotime();
+ if (revs->pending.nr != 1)
+ BUG("run_diff_index must be passed exactly one tree");
+
ent = revs->pending.objects;
if (diff_cache(revs, >item->oid, ent->name, cached))
exit(128);
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 59c4b778d3..68aa5f0340 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -618,6 +618,18 @@ test_expect_success 'pull --rebase fails on unborn branch
with staged changes' '
)
'
+test_expect_success 'pull --rebase fails on corrupt HEAD' '
+ test_when_finished "rm -rf corrupt" &&
+ git init corrupt &&
+ (
+ cd corrupt &&
+ test_commit one &&
+ obj=$(git rev-parse --verify HEAD | sed "s#^..#&/#") &&
+ rm -f .git/objects/$obj &&
+ test_must_fail git pull --rebase
+ )
+'
+
test_expect_success 'setup for detecting upstreamed changes' '
mkdir src &&
(cd src &&
diff --git a/wt-status.c b/wt-status.c
index d1c05145a4..d89c41ba10 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -2340,7 +2340,17 @@ int has_uncommitted_changes(int ignore_submodules)
if (ignore_submodules)
rev_info.diffopt.flags.ignore_submodules = 1;
rev_info.diffopt.flags.quick = 1;
+
add_head_to_pending(_info);
+ if (!rev_info.pending.nr) {
+ /*
+* We have no head (or it's corrupt); use the empty tree,
+* which will complain if the index is non-empty.
+*/
+ struct tree *tree = lookup_tree(the_hash_algo->empty_tree);
+ add_pending_object(_info, >object, "");
+ }
+
diff_setup_done(_info.diffopt);
result = run_diff_index(_info, 1);
return diff_result_code(_info.diffopt, result);
--
2.18.0.400.g702e398724
Re: [PATCH v2 6/9] gpg-interface: do not hardcode the key string len anymore
Am Wed, 11 Jul 2018 08:34:25 -0400 schrieb Jeff King : > On Wed, Jul 11, 2018 at 10:54:59AM +0200, Henning Schild wrote: > > > > In the general case you need: > > > > > > found = *next ? next + 1 : next; > > > > > > or similar. In this case, you can actually do: > > > > > > found = next; > > > > > > because we know that it's OK to search over the literal space > > > again. But that's pretty subtle, so we're probably better off > > > just doing the conditional above. > > > > > > (And yes, looking at the existing code, I think it's even worse, > > > as there does not seem to be a guarantee that we even have 16 > > > characters in the string). > > > > The existing code works only on expected output and the same is true > > for the version after this patch. Making the parser robust against > > random input would imho be a sort of cleanup patch on top of my > > series. .. or before, in which case i would become responsible for > > making sure that still works after my modification. > > This argument is twofold. I do not really want to fix that as well > > and it might be a good idea to separate concerns anyways. > > I think it's worth addressing in the near term, if only because this > kind of off-by-one is quite subtle, and I don't want to forget to deal > with it. Whether that happens as part of this patch, or as a cleanup > before or after, I'm not picky. :) I get that and if anyone is willing to write that code, i will base my patches on it. What i want to avoid is taking responsibility for problems i did not introduce, just because i happen to work on that code at the moment. Keeping track of that (not forgetting) is also not for the random contributor like myself. Henning > -Peff
Re: [PATCH v2 9/9] gpg-interface t: extend the existing GPG tests with GPGSM
Am Wed, 11 Jul 2018 08:51:10 -0400
schrieb Jeff King :
> On Wed, Jul 11, 2018 at 12:38:24PM +0200, Henning Schild wrote:
>
> > > Can we save a dummy generated key and just import it? That's what
> > > we do for the regular gpg case.
> >
> > I will look into storing a binary and leaving notes how it was
> > generated, just like regular gpg does. The reason i did not do that
> > in the first place is that x509 certs have a validity and we
> > introduce time into the picture. But i will see if i can generate
> > epoch->infinity to get the host clock or just the future out of the
> > picture.
>
> That would be preferable. But even if we can just get something like a
> 10-year expiration, that may be enough. Somebody dealing with failing
> tests and regenerating keys in ten years is probably not the end of
> the world.
>
> It could hurt people with drastically incorrect system clocks, but I
> suspect there are other tests with similar problems (especially if
> your clock is in the past).
I now have a working version with a pregenerated certificate from
1970-3000. I would be surprised if git is still around at that time ;).
> > > We're going to have a lot of duplicated tests here. That's a
> > > maintenance burden when one of them needs fixes later. And when
> > > new tests are added, we won't automatically get them tested under
> > > each format.
> > >
> > > Can we move the battery of tests into a function that takes a few
> > > parameters (prereq name, branch to look at, etc) and then call it
> > > for both the gpg/gpgsm cases?
> >
> > I guess this is part of the earlier "allow GPGSM without GPG" and i
> > can ignore it if we agree that this is not needed?
>
> I think it's orthogonal. Even if GPGSM requires GPG, you'd still want
> to make sure that whatever exercise we give to the GPG code is also
> exercised using GPGSM.
>
> I will note, though, that _some_ tests are not really exercising
> gpg-specific bits, but more how we react to it (e.g., how we format %G
> placeholders). And it's probably OK for those to just be run once.
>
> So in an ideal world, the test script would probably look something
> like:
>
> # this function holds tests which exercise the interactions
> # with the gpg binary itself
> type_specific_tests() {
> prereq=$1
> branch=$2
>
> test_expect_success $prereq "test whatever ($prereq)" '
> some test using $branch here
> '
> }
>
> test_expect_success 'setup' '
> set up both openpgp and x509 branches here
> '
>
> type_specific_tests GPG openpgp
> type_specific_tests GPGSM x509
>
> # and now tests that generically care about getting _some_ signature
> # result (e.g., the way we format signature info)
>
> # and then probably a few tests specific to how the config is
> handled, # like your new gpg.format coverage
>
> But in practice, the type-specific bits are often muddled together
> with the type-independent ones (e.g., we happen to test the parsing
> of a failed signature from gpg by checking how %G? is formatted or
> similar).
>
> So it may be simplest to just run most of the tests twice, once with
> gpg and once with gpgsm. I kind of wonder if all of t7510 could just
> be bumped into a function. Or even into a sourced file and run from
> two different scripts. See the way that t8001 and t8002 use
> annotate-tests.sh for an example.
I do not agree and would like to leave the tests as they are. Instead
of introducing a whole lot of very similar copies, i added just a few.
The original ones are even very similar between each other.
We are again talking about two problems. 1. we need test cases for
gpgsm if we want to merge gpgsm 2. the testsuite is very repetitive
While addressing 1 make 2 obvious and worse, addressing 2 is a whole
different story and should probably be discussed outside of this
thread. And i would not like to inherit responsibility for 2. In
fact the whole discussion emphasizes that it was a good idea to make
GPGSM depend on GPG, because it allows to somewhat reuse existing tests.
Henning
> -Peff
Re: BUG: Segfault on "git pull" on "bad object HEAD"
On Wed, Jul 11, 2018 at 01:00:57PM +0200, Ævar Arnfjörð Bjarmason wrote:
> This segfaults, but should print an error instead, have a repo with a
> corrupt HEAD:
>
> (
> rm -rf /tmp/git &&
> git clone --single-branch --branch todo g...@github.com:git/git.git
> /tmp/git &&
> echo
> >/tmp/git/.git/refs/heads/todo &&
> git -C /tmp/git pull
> )
It took me a minute to reproduce this. It needs "pull --rebase" if you
don't have that setup in your config.
> The immediate reason is that in run_diff_index() we have this:
>
> ent = revs->pending.objects;
>
> And that in this case that's NULL:
>
> (gdb) bt
> #0 0x5565993f in run_diff_index (revs=0x7fffcb90, cached=1)
> at diff-lib.c:524
> #1 0x557633da in has_uncommitted_changes (ignore_submodules=1)
> at wt-status.c:2345
These two are the interesting functions. has_uncommitted_changes() calls
add_head_to_pending(). So it could realize then that there is no valid
HEAD to compare against.
But as you note, it's run_diff_index() that blindly dereferences
revs->pending.objects without seeing if it's non-empty. Normally
setup_revisions() would barf on a bad object, but the manual
add_head_to_pending() quietly returns (as it must for some cases, like
unborn branches).
So I feel like the right answer here is probably this:
diff --git a/wt-status.c b/wt-status.c
index d1c05145a4..5fcaa3d0f8 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -2340,7 +2340,16 @@ int has_uncommitted_changes(int ignore_submodules)
if (ignore_submodules)
rev_info.diffopt.flags.ignore_submodules = 1;
rev_info.diffopt.flags.quick = 1;
+
add_head_to_pending(_info);
+ if (!rev_info.pending.nr) {
+ /*
+* We have no head (or it's corrupt), but the index is not
+* unborn; declare it as uncommitted changes.
+*/
+ return 1;
+ }
+
diff_setup_done(_info.diffopt);
result = run_diff_index(_info, 1);
return diff_result_code(_info.diffopt, result);
That does quietly paper over the corruption, but it does the
conservative thing, and a follow-up "git status" would yield "bad
object: HEAD".
I do worry that other callers of run_diff_index() might have similar
problems, though. Grepping around, the other callers seem to fall into
one of three categories:
- they resolve the object themselves and put it in the pending list
(and often fallback to the empty tree, which is more or less what the
patch above is doing)
- they resolve the object themselves and avoid calling run_diff_index()
if it's not valid
- they use setup_revisions(), which will barf on the broken object
So I think this may be sufficient. We probably should also add an
assertion to run_diff_index(), since that's better than segfaulting.
-Peff
[PATCH v2 3/3] t/lib-httpd: avoid occasional failures when checking access.log
The last test of 't5561-http-backend.sh', 'server request log matches
test results' may fail occasionally, because the order of entries in
Apache's access log doesn't match the order of requests sent in the
previous tests, although all the right requests are there. I saw it
fail on Travis CI five times in the span of about half a year, when
the order of two subsequent requests was flipped, and could trigger
the failure with a modified Git. However, I was unable to trigger it
with stock Git on my machine. Three tests in
't5541-http-push-smart.sh' and 't5551-http-fetch-smart.sh' check
requests in the log the same way, so they might be prone to a similar
occasional failure as well.
When a test sends a HTTP request, it can continue execution after
'git-http-backend' fulfilled that request, but Apache writes the
corresponding access log entry only after 'git-http-backend' exited.
Some time inevitably passes between fulfilling the request and writing
the log entry, and, under unfavourable circumstances, enough time
might pass for the subsequent request to be sent and fulfilled by a
different Apache thread or process, and then Apache writes access log
entries racily.
This effect can be exacerbated by adding a bit of variable delay after
the request is fulfilled but before 'git-http-backend' exits, e.g.
like this:
diff --git a/http-backend.c b/http-backend.c
index f3dc218b2..bbf4c125b 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -709,5 +709,7 @@ int cmd_main(int argc, const char **argv)
max_request_buffer);
cmd->imp(, cmd_arg);
+ if (getpid() % 2)
+ sleep(1);
return 0;
}
This delay considerably increases the chances of log entries being
written out of order, and in turn makes t5561's last test fail almost
every time. Alas, it doesn't seem to be enough to trigger a similar
failure in t5541 and t5551.
So, since we can't just rely on the order of access log entries always
corresponding the order of requests, make checking the access log more
deterministic by sorting (simply lexicographically) both the stripped
access log entries and the expected entries before the comparison with
'test_cmp'. This way the order of log entries won't matter and
occasional out-of-order entries won't trigger a test failure, but the
comparison will still notice any unexpected or missing log entries.
OTOH, this sorting will make it harder to identify from which test an
unexpected log entry came from or which test's request went missing.
Therefore, in case of an error include the comparison of the unsorted
log enries in the test output as well.
And since all this should be performed in four tests in three test
scripts, put this into a new helper function 'check_access_log' in
't/lib-httpd.sh'.
Signed-off-by: SZEDER Gábor
---
t/lib-httpd.sh | 12
t/t5541-http-push-smart.sh | 13 +
t/t5551-http-fetch-smart.sh | 3 +--
t/t5561-http-backend.sh | 3 +--
4 files changed, 19 insertions(+), 12 deletions(-)
diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index b6788fea57..7f060aebd0 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -296,3 +296,15 @@ strip_access_log() {
s/^GET /GET /
" "$HTTPD_ROOT_PATH"/access.log
}
+
+# Requires one argument: the name of a file containing the expected stripped
+# access log entries.
+check_access_log() {
+ sort "$1" >"$1".sorted &&
+ strip_access_log >access.log.stripped &&
+ sort access.log.stripped >access.log.sorted &&
+ if ! test_cmp "$1".sorted access.log.sorted
+ then
+ test_cmp "$1" access.log.stripped
+ fi
+}
diff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh
index 6cd986797d..a481e3989a 100755
--- a/t/t5541-http-push-smart.sh
+++ b/t/t5541-http-push-smart.sh
@@ -43,15 +43,13 @@ test_expect_success 'no empty path components' '
cd "$ROOT_PATH" &&
git clone $HTTPD_URL/smart/test_repo.git/ test_repo_clone &&
- strip_access_log >act &&
+ check_access_log exp
+'
+test_expect_success 'clear access log' '
# Clear the log, so that it does not affect the "used receive-pack
# service" test which reads the log too.
- #
- # We do this before the actual comparison to ensure the log is cleared.
- >"$HTTPD_ROOT_PATH"/access.log &&
-
- test_cmp exp act
+ >"$HTTPD_ROOT_PATH"/access.log
'
test_expect_success 'clone remote repository' '
@@ -132,8 +130,7 @@ GET
/smart/test_repo.git/info/refs?service=git-receive-pack HTTP/1.1 200
POST /smart/test_repo.git/git-receive-pack HTTP/1.1 200
EOF
test_expect_success 'used receive-pack service' '
- strip_access_log >act &&
- test_cmp exp act
+ check_access_log exp
'
test_http_push_nonff "$HTTPD_DOCUMENT_ROOT_PATH"/test_repo.git \
diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index c8b6ec493b..3aab44bdcb
[PATCH v2 1/3] t5541: avoid empty line when truncating access log
The second test of 't5541-http-push-smart.sh', 'no empty path components' truncates Apache's access log by running echo >.../access.log which doesn't leave an empty file behind, like a proper truncation would, but a file with a lone newline in it. Consequently, a later test checking the log's contents must consider this improper truncation and include an empty line in the expected content. There is no need for that newline at all, so drop the 'echo' from the truncation and adjust the expected content accordingly. Signed-off-by: SZEDER Gábor --- t/t5541-http-push-smart.sh | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh index a2af693068..d623cbad97 100755 --- a/t/t5541-http-push-smart.sh +++ b/t/t5541-http-push-smart.sh @@ -54,7 +54,7 @@ test_expect_success 'no empty path components' ' # service" test which reads the log too. # # We do this before the actual comparison to ensure the log is cleared. - echo > "$HTTPD_ROOT_PATH"/access.log && + >"$HTTPD_ROOT_PATH"/access.log && test_cmp exp act ' @@ -124,7 +124,6 @@ test_expect_success 'rejected update prints status' ' rm -f "$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/hooks/update" cat >exp <
[PATCH v2 2/3] t/lib-httpd: add the strip_access_log() helper function
Four tests in three httpd-related test scripts check the contents of
Apache's 'access.log', and they all do so by running 'sed' with the
exact same script consisting of four s/// commands to strip
uninteresting log fields and to vertically align the requested URLs.
Extract this into a common helper function 'strip_access_log' in
'lib-httpd.sh', and use it in all of those tests.
Signed-off-by: SZEDER Gábor
---
t/lib-httpd.sh | 9 +
t/t5541-http-push-smart.sh | 14 ++
t/t5551-http-fetch-smart.sh | 7 +--
t/t5561-http-backend.sh | 7 +--
4 files changed, 13 insertions(+), 24 deletions(-)
diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index 435a37465a..b6788fea57 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -287,3 +287,12 @@ expect_askpass() {
test_cmp "$TRASH_DIRECTORY/askpass-expect" \
"$TRASH_DIRECTORY/askpass-query"
}
+
+strip_access_log() {
+ sed -e "
+ s/^.* \"//
+ s/\"//
+ s/ [1-9][0-9]*\$//
+ s/^GET /GET /
+ " "$HTTPD_ROOT_PATH"/access.log
+}
diff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh
index d623cbad97..6cd986797d 100755
--- a/t/t5541-http-push-smart.sh
+++ b/t/t5541-http-push-smart.sh
@@ -43,12 +43,7 @@ test_expect_success 'no empty path components' '
cd "$ROOT_PATH" &&
git clone $HTTPD_URL/smart/test_repo.git/ test_repo_clone &&
- sed -e "
- s/^.* \"//
- s/\"//
- s/ [1-9][0-9]*\$//
- s/^GET /GET /
- " >act <"$HTTPD_ROOT_PATH"/access.log &&
+ strip_access_log >act &&
# Clear the log, so that it does not affect the "used receive-pack
# service" test which reads the log too.
@@ -137,12 +132,7 @@ GET
/smart/test_repo.git/info/refs?service=git-receive-pack HTTP/1.1 200
POST /smart/test_repo.git/git-receive-pack HTTP/1.1 200
EOF
test_expect_success 'used receive-pack service' '
- sed -e "
- s/^.* \"//
- s/\"//
- s/ [1-9][0-9]*\$//
- s/^GET /GET /
- " >act <"$HTTPD_ROOT_PATH"/access.log &&
+ strip_access_log >act &&
test_cmp exp act
'
diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index 913089b144..c8b6ec493b 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -103,12 +103,7 @@ GET /smart/repo.git/info/refs?service=git-upload-pack
HTTP/1.1 200
POST /smart/repo.git/git-upload-pack HTTP/1.1 200
EOF
test_expect_success 'used upload-pack service' '
- sed -e "
- s/^.* \"//
- s/\"//
- s/ [1-9][0-9]*\$//
- s/^GET /GET /
- " >act <"$HTTPD_ROOT_PATH"/access.log &&
+ strip_access_log >act &&
test_cmp exp act
'
diff --git a/t/t5561-http-backend.sh b/t/t5561-http-backend.sh
index 84a955770a..4248b5a06d 100755
--- a/t/t5561-http-backend.sh
+++ b/t/t5561-http-backend.sh
@@ -129,12 +129,7 @@ GET /smart/repo.git/info/refs?service=git-receive-pack
HTTP/1.1 403 -
POST /smart/repo.git/git-receive-pack HTTP/1.1 403 -
EOF
test_expect_success 'server request log matches test results' '
- sed -e "
- s/^.* \"//
- s/\"//
- s/ [1-9][0-9]*\$//
- s/^GET /GET /
- " >act <"$HTTPD_ROOT_PATH"/access.log &&
+ strip_access_log >act &&
test_cmp exp act
'
--
2.18.0.273.g57f1ecce9c
[PATCH v2 0/3] Fix occasional test failures in http tests
't5561-http-backend.sh' is prone to occasional failures; luckily it's not 'git-http-backend's fault, but the test script is a bit racy. I won't go into the details here, patch 3/3's commit message discusses it at length. v1 is here; it haven't been picked up by Junio: https://public-inbox.org/git/20180614123107.11608-1-szeder@gmail.com/T/#u The first two patches are identical to those in v1, and the last patch implements a different and simpler fix than in v1, following Peff's suggestion. The third patch was dropped, because it's not necessary for this simpler fix. SZEDER Gábor (3): t5541: avoid empty line when truncating access log t/lib-httpd: add the strip_access_log() helper function t/lib-httpd: avoid occasional failures when checking access.log t/lib-httpd.sh | 21 + t/t5541-http-push-smart.sh | 24 +--- t/t5551-http-fetch-smart.sh | 8 +--- t/t5561-http-backend.sh | 8 +--- 4 files changed, 28 insertions(+), 33 deletions(-) -- 2.18.0.273.g57f1ecce9c
Re: [RFC PATCH 4/4] t/lib-httpd: sort log based on timestamp to avoid occasional failure
On Thu, Jun 14, 2018 at 7:53 PM Jeff King wrote: > > On Thu, Jun 14, 2018 at 02:31:07PM +0200, SZEDER Gábor wrote: > > > The last test of 't5561-http-backend.sh', 'server request log matches > > test results' may fail occasionally, because the order of entries in > > Apache's access log doesn't match the order of requests sent in the > > previous tests, although all the right requests are there. > I've occasionally run into these failures on my local box, too. I'm > happy somebody is looking into it (I have before, but eventually threw > up my hands in disgust). Heh, well, I found it to be one of the more entertaining debugging sessions... ;) I'm glad that somebody else saw it, too, and outside of Travis CI. > > Now, by default the timestamp of a log entry marks the beginning of > > the request processing, not when the log entry gets written. Since > > our requests are sent sequentially, sorting the log entries based on > > their timestamps would ensure that their order corresponds to the > > order of our requests. > > That's a reasonably clever solution. One thing I wonder, though: are we > always guaranteed that the log entries are written _at all_ before we > look at them? > > I.e., could we have a situation where we make a request, the client > finishes, and then we look at the logs, but nothing has been written by > apache yet? That's possible, I suppose. Highly improbable, but possible. Or the log entries can be written only partially by the time we look at them. FWIW, Apache has the 'BufferedLogs' directive, which "causes mod_log_config to store several log entries in memory and write them together to disk, rather than writing them after each request." It very sensibly defaults to off, and our test config doesn't set it. I would think that stopping Apache should flush the logs, but I don't know whether 'stop_httpd' blocks until the web server is indeed stopped, or it's all async. Perhaps a SIGHUP would suffice to flush the logs, but that's definitely async. Also note that while t5561 checks the access log at the end of the script, t5541 and t5551 do check the logs in the middle of the script, so we would have to stop Apache and then start again. Anyway, I would prefer not to do anything about it just yet, because I'm curious :) and I'd like to see whether this actually happens in practice. > > I don't really like the fix in this patch. I think an unfortunate > > clock skew during the test run could mess up the sorting added in this > > patch and cause test failure. Or if DST or even a leap second happen > > while the test is running. Do we care? Anyway, this occasional test > > failure apparently happens more often than DST and leap seconds > > combined. > > We could probably eliminate DST issues by consistently using UTC for the > timestamps. Leap seconds are probably infrequent enough not to worry > about. More likely is something like clock adjustment due to ntp. Those > adjustments are usually small enough not to matter, but if we're talking > microseconds, it could trigger. Ok, so admittedly I haven't quite thought through all these potential clock-related issues that I mentioned. Setting back the clock at the end of DST is not an issue, because Apache uses the timezone it finds set in the environment, and our test environment does set UTC, so as far as the tests and Apache's log timestamps are concerned there is no such thing as DST. Leap seconds don't go back in time, they are only added to compensate for Earth's slowing rotation, so that's not an issue, either. And I think NTP is supposed to be clever enough to not set back the clock recklessly, but only advance it slower. Of course, someone can still set back the clock manually while these tests run, but I would put that in the "If it hurts, don't do it" box. However, having said all this, ... > What if the test script provides the in-order expectation, but we check > only the unordered version (by sorting both actual and expected output > on the fly)? That would give us a more relaxed check most of the time, > but somebody digging in to a failure could run the ordered diff (or we > could even show it automatically on failure instead of just using > test_cmp). this is the fix I'll go for in v2.
Re: [PATCH v2 9/9] gpg-interface t: extend the existing GPG tests with GPGSM
On Wed, Jul 11, 2018 at 12:38:24PM +0200, Henning Schild wrote:
> > Can we save a dummy generated key and just import it? That's what we
> > do for the regular gpg case.
>
> I will look into storing a binary and leaving notes how it was
> generated, just like regular gpg does. The reason i did not do that in
> the first place is that x509 certs have a validity and we introduce
> time into the picture. But i will see if i can generate epoch->infinity
> to get the host clock or just the future out of the picture.
That would be preferable. But even if we can just get something like a
10-year expiration, that may be enough. Somebody dealing with failing
tests and regenerating keys in ten years is probably not the end of the
world.
It could hurt people with drastically incorrect system clocks, but I
suspect there are other tests with similar problems (especially if your
clock is in the past).
> > We're going to have a lot of duplicated tests here. That's a
> > maintenance burden when one of them needs fixes later. And when new
> > tests are added, we won't automatically get them tested under each
> > format.
> >
> > Can we move the battery of tests into a function that takes a few
> > parameters (prereq name, branch to look at, etc) and then call it for
> > both the gpg/gpgsm cases?
>
> I guess this is part of the earlier "allow GPGSM without GPG" and i
> can ignore it if we agree that this is not needed?
I think it's orthogonal. Even if GPGSM requires GPG, you'd still want to
make sure that whatever exercise we give to the GPG code is also
exercised using GPGSM.
I will note, though, that _some_ tests are not really exercising
gpg-specific bits, but more how we react to it (e.g., how we format %G
placeholders). And it's probably OK for those to just be run once.
So in an ideal world, the test script would probably look something
like:
# this function holds tests which exercise the interactions
# with the gpg binary itself
type_specific_tests() {
prereq=$1
branch=$2
test_expect_success $prereq "test whatever ($prereq)" '
some test using $branch here
'
}
test_expect_success 'setup' '
set up both openpgp and x509 branches here
'
type_specific_tests GPG openpgp
type_specific_tests GPGSM x509
# and now tests that generically care about getting _some_ signature
# result (e.g., the way we format signature info)
# and then probably a few tests specific to how the config is handled,
# like your new gpg.format coverage
But in practice, the type-specific bits are often muddled together with
the type-independent ones (e.g., we happen to test the parsing of a
failed signature from gpg by checking how %G? is formatted or similar).
So it may be simplest to just run most of the tests twice, once with gpg
and once with gpgsm. I kind of wonder if all of t7510 could just be
bumped into a function. Or even into a sourced file and run from two
different scripts. See the way that t8001 and t8002 use
annotate-tests.sh for an example.
-Peff
[PATCH 2/3] rebase --rebase-merges: add support for octopus merges
From: Johannes Schindelin
Previously, we introduced the `merge` command for use in todo lists,
to allow to recreate and modify branch topology.
For ease of implementation, and to make review easier, the initial
implementation only supported merge commits with exactly two parents.
This patch adds support for octopus merges, making use of the
just-introduced `-F ` option for the `git merge` command: to keep
things simple, we spawn a new Git command instead of trying to call a
library function, also opening an easier door to enhance `rebase
--rebase-merges` to optionally use a merge strategy different from
`recursive` for regular merges: this feature would use the same code
path as octopus merges and simply spawn a `git merge`.
Signed-off-by: Johannes Schindelin
---
sequencer.c | 168 +--
t/t3430-rebase-merges.sh | 34
2 files changed, 159 insertions(+), 43 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 5354d4d51..bcc43699c 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2841,6 +2841,26 @@ static int do_reset(const char *name, int len, struct
replay_opts *opts)
return ret;
}
+static struct commit *lookup_label(const char *label, int len,
+ struct strbuf *buf)
+{
+ struct commit *commit;
+
+ strbuf_reset(buf);
+ strbuf_addf(buf, "refs/rewritten/%.*s", len, label);
+ commit = lookup_commit_reference_by_name(buf->buf);
+ if (!commit) {
+ /* fall back to non-rewritten ref or commit */
+ strbuf_splice(buf, 0, strlen("refs/rewritten/"), "", 0);
+ commit = lookup_commit_reference_by_name(buf->buf);
+ }
+
+ if (!commit)
+ error(_("could not resolve '%s'"), buf->buf);
+
+ return commit;
+}
+
static int do_merge(struct commit *commit, const char *arg, int arg_len,
int flags, struct replay_opts *opts)
{
@@ -2849,8 +2869,9 @@ static int do_merge(struct commit *commit, const char
*arg, int arg_len,
struct strbuf ref_name = STRBUF_INIT;
struct commit *head_commit, *merge_commit, *i;
struct commit_list *bases, *j, *reversed = NULL;
+ struct commit_list *to_merge = NULL, **tail = _merge;
struct merge_options o;
- int merge_arg_len, oneline_offset, can_fast_forward, ret;
+ int merge_arg_len, oneline_offset, can_fast_forward, ret, k;
static struct lock_file lock;
const char *p;
@@ -2865,26 +2886,34 @@ static int do_merge(struct commit *commit, const char
*arg, int arg_len,
goto leave_merge;
}
- oneline_offset = arg_len;
- merge_arg_len = strcspn(arg, " \t\n");
- p = arg + merge_arg_len;
- p += strspn(p, " \t\n");
- if (*p == '#' && (!p[1] || isspace(p[1]))) {
- p += 1 + strspn(p + 1, " \t\n");
- oneline_offset = p - arg;
- } else if (p - arg < arg_len)
- BUG("octopus merges are not supported yet: '%s'", p);
-
- strbuf_addf(_name, "refs/rewritten/%.*s", merge_arg_len, arg);
- merge_commit = lookup_commit_reference_by_name(ref_name.buf);
- if (!merge_commit) {
- /* fall back to non-rewritten ref or commit */
- strbuf_splice(_name, 0, strlen("refs/rewritten/"), "", 0);
- merge_commit = lookup_commit_reference_by_name(ref_name.buf);
+ /*
+* For octopus merges, the arg starts with the list of revisions to be
+* merged. The list is optionally followed by '#' and the oneline.
+*/
+ merge_arg_len = oneline_offset = arg_len;
+ for (p = arg; p - arg < arg_len; p += strspn(p, " \t\n")) {
+ if (!*p)
+ break;
+ if (*p == '#' && (!p[1] || isspace(p[1]))) {
+ p += 1 + strspn(p + 1, " \t\n");
+ oneline_offset = p - arg;
+ break;
+ }
+ k = strcspn(p, " \t\n");
+ if (!k)
+ continue;
+ merge_commit = lookup_label(p, k, _name);
+ if (!merge_commit) {
+ ret = error(_("unable to parse '%.*s'"), k, p);
+ goto leave_merge;
+ }
+ tail = _list_insert(merge_commit, tail)->next;
+ p += k;
+ merge_arg_len = p - arg;
}
- if (!merge_commit) {
- ret = error(_("could not resolve '%s'"), ref_name.buf);
+ if (!to_merge) {
+ ret = error(_("nothing to merge: '%.*s'"), arg_len, arg);
goto leave_merge;
}
@@ -2895,8 +2924,13 @@ static int do_merge(struct commit *commit, const char
*arg, int arg_len,
* "[new root]", let's simply fast-forward to the merge head.
*/
rollback_lock_file();
- ret =
[PATCH 0/3] rebase -r: support octopus merges
The `--rebase-merges` option of `git rebase` generates todo lists that include the merge commits that are to be rebased. To keep things simpler to review, I decided to support only regular, 2-parent merge commits first. With this patch series, support is extended to cover also octopus merges. Johannes Schindelin (3): merge: allow reading the merge commit message from a file rebase --rebase-merges: add support for octopus merges rebase --rebase-merges: adjust man page for octopus support Documentation/git-merge.txt | 10 ++- Documentation/git-rebase.txt | 7 +- builtin/merge.c | 32 +++ sequencer.c | 168 ++- t/t3430-rebase-merges.sh | 34 +++ 5 files changed, 204 insertions(+), 47 deletions(-) base-commit: e3331758f12da22f4103eec7efe1b5304a9be5e9 Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-8%2Fdscho%2Fsequencer-and-octopus-merges-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-8/dscho/sequencer-and-octopus-merges-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/8 -- gitgitgadget
[PATCH 1/3] merge: allow reading the merge commit message from a file
From: Johannes Schindelin
This is consistent with `git commit` which, like `git merge`, supports
passing the commit message via `-m ` and, unlike `git merge` before
this patch, via `-F `.
It is useful to allow this for scripted use, or for the upcoming patch
to allow (re-)creating octopus merges in `git rebase --rebase-merges`.
Signed-off-by: Johannes Schindelin
---
Documentation/git-merge.txt | 10 +-
builtin/merge.c | 32
2 files changed, 41 insertions(+), 1 deletion(-)
diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index 6a5c00e2c..537dca7fa 100644
--- a/Documentation/git-merge.txt
+++ b/Documentation/git-merge.txt
@@ -12,7 +12,7 @@ SYNOPSIS
'git merge' [-n] [--stat] [--no-commit] [--squash] [--[no-]edit]
[-s ] [-X ] [-S[]]
[--[no-]allow-unrelated-histories]
- [--[no-]rerere-autoupdate] [-m ] [...]
+ [--[no-]rerere-autoupdate] [-m ] [-F ] [...]
'git merge' --abort
'git merge' --continue
@@ -75,6 +75,14 @@ The 'git fmt-merge-msg' command can be
used to give a good default for automated 'git merge'
invocations. The automated message can include the branch description.
+-F ::
+--file=::
+ Read the commit message to be used for the merge commit (in
+ case one is created).
++
+If `--log` is specified, a shortlog of the commits being merged
+will be appended to the specified message.
+
--[no-]rerere-autoupdate::
Allow the rerere mechanism to update the index with the
result of auto-conflict resolution if possible.
diff --git a/builtin/merge.c b/builtin/merge.c
index 4a4c09496..b0e907751 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -111,6 +111,35 @@ static int option_parse_message(const struct option *opt,
return 0;
}
+static int option_read_message(struct parse_opt_ctx_t *ctx,
+ const struct option *opt, int unset)
+{
+ struct strbuf *buf = opt->value;
+ const char *arg;
+
+ if (unset)
+ BUG("-F cannot be negated");
+
+ if (ctx->opt) {
+ arg = ctx->opt;
+ ctx->opt = NULL;
+ } else if (ctx->argc > 1) {
+ ctx->argc--;
+ arg = *++ctx->argv;
+ } else
+ return opterror(opt, "requires a value", 0);
+
+ if (buf->len)
+ strbuf_addch(buf, '\n');
+ if (ctx->prefix && !is_absolute_path(arg))
+ arg = prefix_filename(ctx->prefix, arg);
+ if (strbuf_read_file(buf, arg, 0) < 0)
+ return error(_("could not read file '%s'"), arg);
+ have_message = 1;
+
+ return 0;
+}
+
static struct strategy *get_strategy(const char *name)
{
int i;
@@ -228,6 +257,9 @@ static struct option builtin_merge_options[] = {
OPT_CALLBACK('m', "message", _msg, N_("message"),
N_("merge commit message (for a non-fast-forward merge)"),
option_parse_message),
+ { OPTION_LOWLEVEL_CALLBACK, 'F', "file", _msg, N_("path"),
+ N_("read message from file"), PARSE_OPT_NONEG,
+ (parse_opt_cb *) option_read_message },
OPT__VERBOSITY(),
OPT_BOOL(0, "abort", _current_merge,
N_("abort the current in-progress merge")),
--
gitgitgadget
[PATCH 3/3] rebase --rebase-merges: adjust man page for octopus support
From: Johannes Schindelin Now that we support octopus merges in the `--rebase-merges` mode, we should give users who actually read the manuals a chance to know about this fact. Signed-off-by: Johannes Schindelin --- Documentation/git-rebase.txt | 7 --- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt index 0e20a66e7..c4bcd24bb 100644 --- a/Documentation/git-rebase.txt +++ b/Documentation/git-rebase.txt @@ -879,8 +879,8 @@ rescheduled immediately, with a helpful message how to edit the todo list (this typically happens when a `reset` command was inserted into the todo list manually and contains a typo). -The `merge` command will merge the specified revision into whatever is -HEAD at that time. With `-C `, the commit message of +The `merge` command will merge the specified revision(s) into whatever +is HEAD at that time. With `-C `, the commit message of the specified merge commit will be used. When the `-C` is changed to a lower-case `-c`, the message will be opened in an editor after a successful merge so that the user can edit the message. @@ -889,7 +889,8 @@ If a `merge` command fails for any reason other than merge conflicts (i.e. when the merge operation did not even start), it is rescheduled immediately. At this time, the `merge` command will *always* use the `recursive` -merge strategy, with no way to choose a different one. To work around +merge strategy for regular merges, and `octopus` for octopus merges, +strategy, with no way to choose a different one. To work around this, an `exec` command can be used to call `git merge` explicitly, using the fact that the labels are worktree-local refs (the ref `refs/rewritten/onto` would correspond to the label `onto`, for example). -- gitgitgadget
Re: [PATCH v2 6/9] gpg-interface: do not hardcode the key string len anymore
On Wed, Jul 11, 2018 at 10:54:59AM +0200, Henning Schild wrote: > > In the general case you need: > > > > found = *next ? next + 1 : next; > > > > or similar. In this case, you can actually do: > > > > found = next; > > > > because we know that it's OK to search over the literal space again. > > But that's pretty subtle, so we're probably better off just doing the > > conditional above. > > > > (And yes, looking at the existing code, I think it's even worse, as > > there does not seem to be a guarantee that we even have 16 characters > > in the string). > > The existing code works only on expected output and the same is true > for the version after this patch. Making the parser robust against > random input would imho be a sort of cleanup patch on top of my > series. .. or before, in which case i would become responsible for > making sure that still works after my modification. > This argument is twofold. I do not really want to fix that as well and > it might be a good idea to separate concerns anyways. I think it's worth addressing in the near term, if only because this kind of off-by-one is quite subtle, and I don't want to forget to deal with it. Whether that happens as part of this patch, or as a cleanup before or after, I'm not picky. :) -Peff
Re: [PATCH] sha1-name.c: for ":/", find detached HEAD commits
On Tue, Jul 10, 2018 at 11:18:22PM -0700, William Chargin wrote: > > Also, I am not sure if "or from HEAD" is even needed when we say > > "from ANY ref" already, as we count things like HEAD as part of the > > ref namespace. > > My two cents: with the docs as is, I wasn't sure whether HEAD was > intended to count as a ref for this purpose. The gitglossary man page > defines a ref as a "name that begins with refs/" (seemingly excluding > HEAD), though it later says that HEAD is a "special-purpose ref". In my > opinion, the change adds clarity without any particular downside---but > I'm happy to revert it if you'd prefer. I'd also be happy to change the > wording to something like "any ref, including HEAD" if we want to > emphasize that HEAD really is a ref. FWIW, I think the clarification to include HEAD is helpful here, since it took me a few minutes of thinking to decide whether the current behavior was a bug or just a subtlety. Your "including HEAD" suggestion seems like the best route to me. But I can live with it either way. > After reaching consensus on the change to the docs, should I send in a > [PATCH v2] In-Reply-To this thread? Yes. > Peff, should I add your > Signed-off-by to the commit message, or is that not how things are done? Yes, you can add in any sign-offs that have been explicitly given. It's normal to order them chronologically, too (so mine would come first, then yours, showing that the patch flowed through me to you; Junio will add his at the end). -Peff
CONTACT MR DAVID HART FOR YOUR DELIVERY OF YOUR MASTER CARD
Congratulation To You Dear Beneficiary. Glory be to almighty God whom make everything possible to God Answer your prayer Beneficiary. This is to officially inform you that After the Board of director’s meeting held in United Nations in conjunction with Federal Ministry Of Finance Office. We have resolved in finding a solution to your compensation payments worth of $Two Million Five Hundred Thousand United States Dollars. We have arranged your payments on an ATM MasterCard. As the best and easy way to receive your fund fast Because we have perfectly concluded the deliver of your Master Card to your address. so all you need to do is to contact ATM Director Mr David Hart with your full information. The maximum daily withdraw is $5,000.00 United States Dollars everyday until your total amount of your fund deposit in your card $2.5 Million United States Dollars is completely withdrawal up to you at any nearest ATM Machine center of your choice in your country which was stamp by (IMF) International Monetary Fund Office. This is a special arrangement in collaboration with MasterCard Company USA. Here are details of your ATM Master Card Bank Name: Citibank Card Account Number:5412571234123456 Date of Issuance: July 11/7/2018 Amount Deposit in your card is ($2.5 Million United States Dollars) Shipment Code awb :33xzs ATM Master Card Registered Code No xgt442 Security Code sctc:2001dhx:567 Transaction Code 233:cstc:101:33028 Certificate deposit code: sctc:bun.xxiv:78:01 You are advice to reconfirmed your full information with the following information below (1) Your Full Name:... (2) Full residential address: (3) Phone and Fax Number: (4) Occupation:... (5) Personal Identification. Driver’s license or International Passport (6) Age:... (7) Marital Status: (8) Sex:... Contact Personal Mr David Hart with this information below Contact Personal Mr David Hart Email: davidhart...@hotmail.com The Citibank Hereby issued your our code of conduct which is ATM Registration Code of EB8527 So you have to indiccate this code when contacting the Mr David Hart on the Card center by using it as your subject. Remain Bless Yours Sincerely Mrs Jane Daniel General Secretary Office
BUG: Segfault on "git pull" on "bad object HEAD"
This segfaults, but should print an error instead, have a repo with a
corrupt HEAD:
(
rm -rf /tmp/git &&
git clone --single-branch --branch todo g...@github.com:git/git.git
/tmp/git &&
echo
>/tmp/git/.git/refs/heads/todo &&
git -C /tmp/git pull
)
On this repository e.g. "git log" will print "fatal: bad object HEAD",
but for some reason "git pull" makes it this far:
$ git pull
Segmentation fault
The immediate reason is that in run_diff_index() we have this:
ent = revs->pending.objects;
And that in this case that's NULL:
(gdb) bt
#0 0x5565993f in run_diff_index (revs=0x7fffcb90, cached=1) at
diff-lib.c:524
#1 0x557633da in has_uncommitted_changes (ignore_submodules=1) at
wt-status.c:2345
#2 0x557634c9 in require_clean_work_tree (action=0x55798f18
"pull with rebase", hint=0x55798efb "please commit or stash them.",
ignore_submodules=1, gently=0) at wt-status.c:2370
#3 0x555dbdee in cmd_pull (argc=0, argv=0x7fffd868,
prefix=0x0) at builtin/pull.c:885
#4 0x5556c9da in run_builtin (p=0x55a2de50 ,
argc=1, argv=0x7fffd868) at git.c:417
#5 0x5556cce2 in handle_builtin (argc=1, argv=0x7fffd868) at
git.c:633
#6 0x5556ce8a in run_argv (argcp=0x7fffd71c,
argv=0x7fffd710) at git.c:685
#7 0x5556d03f in cmd_main (argc=1, argv=0x7fffd868) at
git.c:762
#8 0x55611786 in main (argc=3, argv=0x7fffd858) at
common-main.c:45
(gdb) p revs
$4 = (struct rev_info *) 0x7fffcb90
(gdb) p revs->pending
$5 = {nr = 0, alloc = 0, objects = 0x0}
(gdb)
This has been an issue since at least v2.8.0 (didn't test back
further). I'm not familiar with the status / diff code, so I'm not sure
where the assertion should be added.
This came up in the wild due to a user with a corrupt repo (don't know
how it got corrupt) trying "git pull" and seeing git segfault.
Re: [PATCH v3 1/2] t3418: add testcase showing problems with rebase -i and strategy options
On Wed, Jun 27, 2018 at 8:28 PM SZEDER Gábor wrote: > > > diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh > > index 03bf1b8a3b..11546d6e14 100755 > > --- a/t/t3418-rebase-continue.sh > > +++ b/t/t3418-rebase-continue.sh > > @@ -74,6 +74,38 @@ test_expect_success 'rebase --continue remembers merge > > strategy and options' ' > > test -f funny.was.run > > ' > > > > +test_expect_failure 'rebase -i --continue handles merge strategy and > > options' ' > > + rm -fr .git/rebase-* && > > + git reset --hard commit-new-file-F2-on-topic-branch && > > + test_commit "commit-new-file-F3-on-topic-branch-for-dash-i" F3 32 && > > + test_when_finished "rm -fr test-bin funny.was.run funny.args" && > > + mkdir test-bin && > > + cat >test-bin/git-merge-funny <<-EOF && > > + #!$SHELL_PATH > > + echo "\$@" >>funny.args > > + case "\$1" in --opt) ;; *) exit 2 ;; esac > > + case "\$2" in --foo) ;; *) exit 2 ;; esac > > + case "\$4" in --) ;; *) exit 2 ;; esac > > + shift 2 && > > + >funny.was.run && > > + exec git merge-recursive "\$@" > > + EOF > > + chmod +x test-bin/git-merge-funny && > > You could use the 'write_script' helper function here. > > > + ( > > + PATH=./test-bin:$PATH && > > + test_must_fail git rebase -i -s funny -Xopt -Xfoo master topic > > + ) && > > + test -f funny.was.run && And please use 'test_path_is_file' here ... > > + rm funny.was.run && > > + echo "Resolved" >F2 && > > + git add F2 && > > + ( > > + PATH=./test-bin:$PATH && > > + git rebase --continue > > + ) && > > + test -f funny.was.run ... and here. > > +' > > + > > test_expect_success 'rebase passes merge strategy options correctly' ' > > rm -fr .git/rebase-* && > > git reset --hard commit-new-file-F3-on-topic-branch && > > -- > > 2.18.0.9.g431b2c36d5 > > > >
Re: [PATCH v2 9/9] gpg-interface t: extend the existing GPG tests with GPGSM
Am Tue, 10 Jul 2018 14:12:57 -0700 schrieb Junio C Hamano : > Henning Schild writes: > > > Add test cases to cover the new X509/gpgsm support. Most of them > > resemble existing ones. They just switch the format to x509 and set > > the signingkey when creating signatures. Validation of signatures > > does not need any configuration of git, it does need gpgsm to be > > configured to trust the key(-chain). > > We generate a self-signed key for commit...@example.com and > > configure gpgsm to trust it. > > > > Signed-off-by: Henning Schild > > --- > > t/lib-gpg.sh | 9 ++- > > t/lib-gpg/gpgsm-gen-key.in | 6 + > > t/t4202-log.sh | 66 > > ++ > > t/t5534-push-signed.sh | 52 > > t/t7003-filter-branch.sh | > > 15 +++ t/t7030-verify-tag.sh | 47 > > +++-- t/t7600-merge.sh | 31 > > ++ 7 files changed, 223 insertions(+), 3 > > deletions(-) create mode 100644 t/lib-gpg/gpgsm-gen-key.in > > I saw my post-integration tests taking forever to finish for 'pu' > and "ps x" output showed that many tests (I ran tests in parallel) > were stuck and all of them were running gpgsm. > > For now, I've ejected the topic out of 'pu', as I want to finish > today's integration run with all the other topics first before > coming back to this topic. That must have been the key generation, see my reply to one of Jeffs mails. Henning
Re: [PATCH v2 9/9] gpg-interface t: extend the existing GPG tests with GPGSM
Am Tue, 10 Jul 2018 13:09:01 -0400
schrieb Jeff King :
> On Tue, Jul 10, 2018 at 10:52:31AM +0200, Henning Schild wrote:
>
> > diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh
> > index a5d3b2cba..9dcb4e990 100755
> > --- a/t/lib-gpg.sh
> > +++ b/t/lib-gpg.sh
> > @@ -38,7 +38,14 @@ then
> > "$TEST_DIRECTORY"/lib-gpg/ownertrust &&
> > gpg --homedir "${GNUPGHOME}" /dev/null
> > 2>&1 \ --sign -u commit...@example.com &&
> > - test_set_prereq GPG
> > + test_set_prereq GPG &&
> > + echo | gpgsm --homedir "${GNUPGHOME}" -o
> > "$TEST_DIRECTORY"/lib-gpg/gpgsm.crt.user --passphrase-fd 0
> > --pinentry-mode loopback --generate-key --batch
> > "$TEST_DIRECTORY"/lib-gpg/gpgsm-gen-key.in &&
>
> Is this --generate-key going to require high-quality entropy? That
> could slow the test suite down (and maybe even stall it quite a bit
> on servers doing automated CI).
Yes, i also saw that getting "stuck" on one machine. But i blamed an
experimental kernel.
> Can we save a dummy generated key and just import it? That's what we
> do for the regular gpg case.
I will look into storing a binary and leaving notes how it was
generated, just like regular gpg does. The reason i did not do that in
the first place is that x509 certs have a validity and we introduce
time into the picture. But i will see if i can generate epoch->infinity
to get the host clock or just the future out of the picture.
> > + gpgsm --homedir "${GNUPGHOME}" --import
> > "$TEST_DIRECTORY"/lib-gpg/gpgsm.crt.user &&
> > + gpgsm --homedir "${GNUPGHOME}" -K | grep
> > fingerprint: | cut -d" " -f4 | tr -d '\n' >
> > ${GNUPGHOME}/trustlist.txt &&
> > + echo " S relax" >> ${GNUPGHOME}/trustlist.txt &&
> > + (gpgconf --kill gpg-agent >/dev/null 2>&1 || : ) &&
> > + echo hello | gpgsm --homedir "${GNUPGHOME}" -u
> > commit...@example.com -o /dev/null --sign - 2>&1 &&
> > + test_set_prereq GPGSM
>
> This &&-chain means we can't have GPGSM without GPG. In theory the two
> could be tested independently. I don't know if it's worth the trouble
> to make that work, though. I wouldn't be surprised if there are some
> subtle dependencies within the test scripts, and I'm not sure how
> common it is for somebody to have gpgsm and not gpg. So it may make
> sense to just punt on it until such a person appears.
As you found later, i already commented on that and do not think it is
worth the effort. Users will be able to just have gpgsm, testers will
need gpg to test gpgsm.
> > test_expect_success GPG 'log --graph --show-signature' '
> > git log --graph --show-signature -n1 signed >actual &&
> > grep "^| gpg: Signature made" actual &&
> > grep "^| gpg: Good signature" actual
> > '
> >
> > +test_expect_success GPGSM 'log --graph --show-signature x509' '
> > + git log --graph --show-signature -n1 signed-x509 >actual &&
> > + grep "^| gpgsm: Signature made" actual &&
> > + grep "^| gpgsm: Good signature" actual
> > +'
>
> We're going to have a lot of duplicated tests here. That's a
> maintenance burden when one of them needs fixes later. And when new
> tests are added, we won't automatically get them tested under each
> format.
>
> Can we move the battery of tests into a function that takes a few
> parameters (prereq name, branch to look at, etc) and then call it for
> both the gpg/gpgsm cases?
I guess this is part of the earlier "allow GPGSM without GPG" and i
can ignore it if we agree that this is not needed?
Henning
> -Peff
Re: [PATCH v3 01/20] linear-assignment: a function to solve least-cost assignment problems
> diff --git a/linear-assignment.c b/linear-assignment.c
> new file mode 100644
> index 0..0b0344b5f
> --- /dev/null
> +++ b/linear-assignment.c
> @@ -0,0 +1,203 @@
> +/*
> + * Based on: Jonker, R., & Volgenant, A. (1987). A shortest augmenting
> path
> + * algorithm for dense and sparse linear assignment problems. Computing,
> + * 38(4), 325-340.
> + */
> +#include "cache.h"
> +#include "linear-assignment.h"
> +
> +#define COST(column, row) cost[(column) + column_count * (row)]
> +
> +/*
> + * The parameter `cost` is the cost matrix: the cost to assign column j to
> row
> + * i is `cost[j + column_count * i].
> + */
> +void compute_assignment(int column_count, int row_count, int *cost,
> + int *column2row, int *row2column)
> +{
[...]
> +update:
> + /* updating of the column pieces */
> + for (k = 0; k < last; k++) {
> + int j1 = col[k];
> + v[j1] += d[j1] - min;
> + }
> +
> + /* augmentation */
> + do {
> + if (j < 0)
> + BUG("negative j: %d", j);
> + i = pred[j];
> + column2row[j] = i;
> + k = j;
> + j = row2column[i];
> + row2column[i] = k;
Coccinelle suggests using SWAP(j, row2column[i]) instead of the last
three lines above.
It's more idiomatic, and it avoids (ab)using the 'k' variable
(elsewhere used as loop variable) as a temporary variable.
> + } while (i1 != i);
> + }
> +
> + free(col);
> + free(pred);
> + free(d);
> + free(v);
> + free(free_row);
> +}
Re: [PATCH v3 16/24] config: create core.multiPackIndex setting
> diff --git a/Documentation/config.txt b/Documentation/config.txt > index ab641bf5a9..ab895ebb32 100644 > --- a/Documentation/config.txt > +++ b/Documentation/config.txt > @@ -908,6 +908,10 @@ core.commitGraph:: > Enable git commit graph feature. Allows reading from the > commit-graph file. > > +core.multiPackIndex:: > + Use the multi-pack-index file to track multiple packfiles using a > + single index. See linkgit:technical/multi-pack-index[1]. The 'linkgit' macro should be used to create links to other man pages, but 'technical/multi-pack-index' is not a man page and this causes 'make check-docs' to complain: LINT lint-docs ./config.txt:929: nongit link: technical/multi-pack-index[1] Makefile:456: recipe for target 'lint-docs' failed make[1]: *** [lint-docs] Error 1 > + > core.sparseCheckout:: > Enable "sparse checkout" feature. See section "Sparse checkout" in > linkgit:git-read-tree[1] for more information.
Re: [PATCH v7 06/22] commit-graph: load a root tree from specific graph
> When lazy-loading a tree for a commit, it will be important to select
> the tree from a specific struct commit_graph. Create a new method that
> specifies the commit-graph file and use that in
> get_commit_tree_in_graph().
>
> Signed-off-by: Derrick Stolee
> ---
> commit-graph.c | 12 +---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/commit-graph.c b/commit-graph.c
> index e77b19971d..9e228d3bb5 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -362,14 +362,20 @@ static struct tree *load_tree_for_commit(struct
> commit_graph *g, struct commit *
> return c->maybe_tree;
> }
>
> -struct tree *get_commit_tree_in_graph(const struct commit *c)
> +static struct tree *get_commit_tree_in_graph_one(struct commit_graph *g,
> + const struct commit *c)
> {
> if (c->maybe_tree)
> return c->maybe_tree;
Coccinelle and 'commit.cocci' complain about this change, because it
renames a function directly accessing struct commit's maybe_tree
member.
In 'commit.cocci' the regex listing the names of functions that are
allowed to access c->maybe_tree directly should be updated accordingly
as well.
> if (c->graph_pos == COMMIT_NOT_FROM_GRAPH)
> - BUG("get_commit_tree_in_graph called from non-commit-graph
> commit");
> + BUG("get_commit_tree_in_graph_one called from non-commit-graph
> commit");
> +
> + return load_tree_for_commit(g, (struct commit *)c);
> +}
>
> - return load_tree_for_commit(commit_graph, (struct commit *)c);
> +struct tree *get_commit_tree_in_graph(const struct commit *c)
> +{
> + return get_commit_tree_in_graph_one(commit_graph, c);
> }
>
> static void write_graph_chunk_fanout(struct hashfile *f,
> --
> 2.18.0.24.g1b579a2ee9
>
>
