Re: [PATCH] DO-NOT-MERGE: write and read commit-graph always

2018-07-31 Thread Elijah Newren
On Wed, Jul 18, 2018 at 8:22 AM, Derrick Stolee  wrote:
> The following test fails because the repo has ambiguous merge-bases, and
> the commit-graph changes the walk order so we select a different one.
> This alters the resulting merge from the expected result.
>
> t6024-recursive-merge.sh, Test 4
>
> The tests above are made to pass by deleting the commit-graph file
> before the necessary steps.

I know you meant for these to not be merged, but perhaps the test in
t6024 could be made to be less stringent about order of merge bases.
In particular, instead of expecting a certain sha1 to be at stage 2
and a different one to be at stage 3, it could just check that both
shas appear in the `git ls-files --stage` output.

> diff --git a/t/t6024-recursive-merge.sh b/t/t6024-recursive-merge.sh
> index 3f59e58dfb..cec10983cd 100755
> --- a/t/t6024-recursive-merge.sh
> +++ b/t/t6024-recursive-merge.sh
> @@ -61,6 +61,7 @@ GIT_AUTHOR_DATE="2006-12-12 23:00:08" git commit -m F
>  '
>
>  test_expect_success "combined merge conflicts" "
> +   rm -rf .git/objects/info/commit-graph &&
> test_must_fail git merge -m final G
>  "
>
> --
> 2.18.0.118.gd4f65b8d14


Re: [PATCH] DO-NOT-MERGE: write and read commit-graph always

2018-07-31 Thread Jakub Narebski
Stefan Beller  writes:

>> I wonder though if all those changes to the testsuite shouldn't be
>> merged.
>
> I think Stolee doesn't want this to be merged after rereading
> subject and the commit message.

Yes, I understand that, and for the most part I agree with it.  This
commit main purpose is to thoroughly exercise the new feature.

But I think that changes to the testsuite could have been extracted into
separate commit, and merged (and only those changes).  It could serve as
note about the intent of the test.  Well, perhaps after encapsulating it
in some function with a good name... ;-)

Anyway, this is a minor insignificant thing.
-- 
Jakub Narębski


Re: [PATCH] DO-NOT-MERGE: write and read commit-graph always

2018-07-30 Thread Stefan Beller
> I wonder though if all those changes to the testsuite shouldn't be
> merged.

I think Stolee doesn't want this to be merged after rereading
subject and the commit message.

Thanks,
Stefan


Re: [PATCH] DO-NOT-MERGE: write and read commit-graph always

2018-07-30 Thread Jakub Narebski
Derrick Stolee  writes:

> This commit is not intended to be merged, but is only to create a test
> environment to see what works with the commit-graph feature and what
> does not. The tests that fail are primarily related to corrupting the
> object store to remove a commit from visibility and testing that
> rev-list fails -- except it doesn't when there is a commit-graph that
> prevents parsing from the object database. The following tests will fail
> with this commit, but are not "real" bugs:
>
> t0410-partial-clone.sh, Test 9
> t5307-pack-missing-commit.sh, Tests 3-4
> t6011-rev-list-with-bad-commit.sh, Test 4
>
> The following test fails because the repo has ambiguous merge-bases, and
> the commit-graph changes the walk order so we select a different one.
> This alters the resulting merge from the expected result.
>
> t6024-recursive-merge.sh, Test 4
>
> The tests above are made to pass by deleting the commit-graph file
> before the necessary steps.
>
> Signed-off-by: Derrick Stolee 
> ---
>  builtin/commit.c|  2 ++
>  builtin/gc.c|  3 +--
>  commit-graph.c  | 11 ---
>  t/t0410-partial-clone.sh|  1 +
>  t/t5307-pack-missing-commit.sh  |  2 ++
>  t/t6011-rev-list-with-bad-commit.sh |  1 +
>  t/t6024-recursive-merge.sh  |  1 +
>  7 files changed, 8 insertions(+), 13 deletions(-)
>
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 158e3f843a..acc31252a9 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -33,6 +33,7 @@
>  #include "sequencer.h"
>  #include "mailmap.h"
>  #include "help.h"
> +#include "commit-graph.h"
>  
>  static const char * const builtin_commit_usage[] = {
>   N_("git commit [] [--] ..."),
> @@ -1652,6 +1653,7 @@ int cmd_commit(int argc, const char **argv, const char 
> *prefix)
>"not exceeded, and then \"git reset HEAD\" to recover."));
>  
>   rerere(0);
> + write_commit_graph_reachable(get_object_directory(), 1);
>   run_command_v_opt(argv_gc_auto, RUN_GIT_CMD);
>   run_commit_hook(use_editor, get_index_file(), "post-commit", NULL);
>   if (amend && !no_post_rewrite) {

This is certainly not for merging.

> diff --git a/builtin/gc.c b/builtin/gc.c
> index e103f0f85d..60ab773087 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -41,7 +41,7 @@ static int aggressive_depth = 50;
>  static int aggressive_window = 250;
>  static int gc_auto_threshold = 6700;
>  static int gc_auto_pack_limit = 50;
> -static int gc_write_commit_graph;
> +static int gc_write_commit_graph = 1;
>  static int detach_auto = 1;
>  static timestamp_t gc_log_expire_time;
>  static const char *gc_log_expire = "1.day.ago";

This is switching from default to off to default to on.  I think with
this feature we would default to off for a long time (wishful thinking:
maybe automatically enabling it for large repositories, or if
commit-graph file exists already?).

> @@ -131,7 +131,6 @@ static void gc_config(void)
>   git_config_get_int("gc.aggressivedepth", &aggressive_depth);
>   git_config_get_int("gc.auto", &gc_auto_threshold);
>   git_config_get_int("gc.autopacklimit", &gc_auto_pack_limit);
> - git_config_get_bool("gc.writecommitgraph", &gc_write_commit_graph);
>   git_config_get_bool("gc.autodetach", &detach_auto);
>   git_config_get_expiry("gc.pruneexpire", &prune_expire);
>   git_config_get_expiry("gc.worktreepruneexpire", 
> &prune_worktrees_expire);

This is certainly not for merging, as it disables gc.writeCommitGraph
config option entirely.

> diff --git a/commit-graph.c b/commit-graph.c
> index 237d4e7d1b..ed0d27c12e 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -227,22 +227,11 @@ static int prepare_commit_graph(struct repository *r)
>  {
>   struct alternate_object_database *alt;
>   char *obj_dir;
> - int config_value;
>  
>   if (r->objects->commit_graph_attempted)
>   return !!r->objects->commit_graph;
>   r->objects->commit_graph_attempted = 1;
>  
> - if (repo_config_get_bool(r, "core.commitgraph", &config_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;
> -
>   if (!commit_graph_compatible(r))
>   return 0;
>

This is certainly not for merging, as it disables core.commitGraph
config option entirely.

> diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
> index 4984ca583d..c235672b03 100755
> --- a/t/t0410-partial-clone.sh
> +++ b/t/t0410-partial-clone.sh
> @@ -181,6 +181,7 @@ test_expect_success 'rev-list stops traversal at missing 
> and promised commit' '
>  
>   git -C repo config core.repositoryformatversion 1 &&
>