git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jakub Narebski <jnareb@gmail.com>
To: Derrick Stolee <dstolee@microsoft.com>
Cc: "git\@vger.kernel.org" <git@vger.kernel.org>,
	"gitster\@pobox.com" <gitster@pobox.com>,
	"stolee\@gmail.com" <stolee@gmail.com>,
	"avarab\@gmail.com" <avarab@gmail.com>,
	"marten.agren\@gmail.com" <marten.agren@gmail.com>,
	"peff\@peff.net" <peff@peff.net>
Subject: Re: [PATCH v3 19/20] gc: automatically write commit-graph files
Date: Sat, 02 Jun 2018 20:03:18 +0200	[thread overview]
Message-ID: <866031rrq1.fsf@gmail.com> (raw)
In-Reply-To: <20180524162504.158394-20-dstolee@microsoft.com> (Derrick Stolee's message of "Thu, 24 May 2018 16:26:09 +0000")

Derrick Stolee <dstolee@microsoft.com> writes:

> The commit-graph file is a very helpful feature for speeding up git
> operations. In order to make it more useful, write the commit-graph file
> by default during standard garbage collection operations.

I think you meant here "make it possible to write the commit-graph file
during standard garbage collection operations." (i.e. add "make it
possible" because it hides behind new config option, and remove "by
default" because currently it is not turned on by default).

>
> Add a 'gc.commitGraph' config setting that triggers writing a
> commit-graph file after any non-trivial 'git gc' command. Defaults to
> false while the commit-graph feature matures. We specifically do not
> want to turn this on by default until the commit-graph feature is fully

s/turn this on/have this on/  I think.

> integrated with history-modifying features like shallow clones.

Two things.

First, shallow clones, replacement mechanims (git-replace) and grafts
are not "history-modifying" features; this name is in my opinion
reserved for history-rewriting features such as interactive rebase, the
`git filter-branch` feature or out-of-tree BFG Repo Cleaner or
reposurgeon tools.  They alter the _view_ of history; they should be
IMVHO named "history-view-altering" features -- though I agree this is
mouthful.

Second, shouldn't we, as Martin Ågren said, warn about the issue in the
manpage for git-commit-graph?

>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  Documentation/config.txt |  6 ++++++
>  Documentation/git-gc.txt |  4 ++++
>  builtin/gc.c             |  6 ++++++
>  t/t5318-commit-graph.sh  | 14 ++++++++++++++
>  4 files changed, 30 insertions(+)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 11f027194e..9a3abd87e7 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1553,6 +1553,12 @@ gc.autoDetach::
>  	Make `git gc --auto` return immediately and run in background
>  	if the system supports it. Default is true.
>  
> +gc.commitGraph::
> +	If true, then gc will rewrite the commit-graph file after any
> +	change to the object database. If '--auto' is used, then the
> +	commit-graph will not be updated unless the threshold is met.

What threshold?  Ah, thresholds defined for `git gc --auto` (gc.auto,
gc.autoPackLimit, gc.logExpiry,...).

> +	See linkgit:git-commit-graph[1] for details.

You missed declaring the default value for this config option.

> +
>  gc.logExpiry::
>  	If the file gc.log exists, then `git gc --auto` won't run
>  	unless that file is more than 'gc.logExpiry' old.  Default is
> diff --git a/Documentation/git-gc.txt b/Documentation/git-gc.txt
> index 571b5a7e3c..17dd654a59 100644
> --- a/Documentation/git-gc.txt
> +++ b/Documentation/git-gc.txt
> @@ -119,6 +119,10 @@ The optional configuration variable `gc.packRefs` determines if
>  it within all non-bare repos or it can be set to a boolean value.
>  This defaults to true.
>  
> +The optional configuration variable 'gc.commitGraph' determines if
> +'git gc' runs 'git commit-graph write'. This can be set to a boolean

Should it be "runs" or "should run"?

> +value. This defaults to false.

Should it be '...' or `...`?  Below we have `gc.aggresiveWindow`, above
we have 'gc.commitGraph', for example.

> +
>  The optional configuration variable `gc.aggressiveWindow` controls how
>  much time is spent optimizing the delta compression of the objects in
>  the repository when the --aggressive option is specified.  The larger
> diff --git a/builtin/gc.c b/builtin/gc.c
> index 77fa720bd0..efd214a59f 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -20,6 +20,7 @@
>  #include "argv-array.h"
>  #include "commit.h"
>  #include "packfile.h"
> +#include "commit-graph.h"
>  
>  #define FAILED_RUN "failed to run %s"
>  
> @@ -34,6 +35,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_commit_graph = 0;
>  static int detach_auto = 1;
>  static timestamp_t gc_log_expire_time;
>  static const char *gc_log_expire = "1.day.ago";
> @@ -121,6 +123,7 @@ 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.commitgraph", &gc_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);
> @@ -480,6 +483,9 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
>  	if (pack_garbage.nr > 0)
>  		clean_pack_garbage();
>  
> +	if (gc_commit_graph)
> +		write_commit_graph_reachable(get_object_directory(), 0);
> +

Nice.

Though now I wonder when appending should be used...

>  	if (auto_gc && too_many_loose_objects())
>  		warning(_("There are too many unreachable loose objects; "
>  			"run 'git prune' to remove them."));
> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
> index a659620332..d20b17586f 100755
> --- a/t/t5318-commit-graph.sh
> +++ b/t/t5318-commit-graph.sh
> @@ -245,6 +245,20 @@ test_expect_success 'perform fast-forward merge in full repo' '
>  	test_cmp expect output
>  '
>  
> +test_expect_success 'check that gc clears commit-graph' '

I wouldn't use the word "clears" here...

> +	cd "$TRASH_DIRECTORY/full" &&
> +	git commit --allow-empty -m "blank" &&
> +	git commit-graph write --reachable &&
> +	cp $objdir/info/commit-graph commit-graph-before-gc &&
> +	git reset --hard HEAD~1 &&
> +	git config gc.commitGraph true &&
> +	git gc &&
> +	cp $objdir/info/commit-graph commit-graph-after-gc &&
> +	! test_cmp commit-graph-before-gc commit-graph-after-gc &&
> +	git commit-graph write --reachable &&
> +	test_cmp commit-graph-after-gc $objdir/info/commit-graph
> +'

...but otherwise, nice test: it checks that git-gc after rewriting
history changes commit-graph file, and that the changed file is what we
expect it to be (note: here we compare commit-graph files directly, and
not just check the features via 'git commit-graph read').

> +
>  # the verify tests below expect the commit-graph to contain
>  # exactly the commits reachable from the commits/8 branch.
>  # If the file changes the set of commits in the list, then the

  reply	other threads:[~2018-06-02 18:03 UTC|newest]

Thread overview: 149+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-04-17 18:10 [RFC PATCH 00/12] Integrate commit-graph into 'fsck' and 'gc' Derrick Stolee
2018-04-17 18:10 ` [RFC PATCH 01/12] fixup! commit-graph: always load commit-graph information Derrick Stolee
2018-04-17 18:10 ` [RFC PATCH 03/12] commit-graph: check file header information Derrick Stolee
2018-04-19 15:58   ` Jakub Narebski
2018-04-17 18:10 ` [RFC PATCH 02/12] commit-graph: add 'check' subcommand Derrick Stolee
2018-04-19 13:24   ` Jakub Narebski
2018-04-17 18:10 ` [RFC PATCH 04/12] commit-graph: parse commit from chosen graph Derrick Stolee
2018-04-19 17:21   ` Jakub Narebski
2018-04-17 18:10 ` [RFC PATCH 06/12] commit: force commit to parse from object database Derrick Stolee
2018-04-20 12:13   ` Jakub Narebski
2018-04-17 18:10 ` [RFC PATCH 05/12] commit-graph: check fanout and lookup table Derrick Stolee
2018-04-20  7:27   ` Jakub Narebski
2018-04-17 18:10 ` [RFC PATCH 07/12] commit-graph: load a root tree from specific graph Derrick Stolee
2018-04-20 12:18   ` Jakub Narebski
2018-04-17 18:10 ` [RFC PATCH 08/12] commit-graph: verify commit contents against odb Derrick Stolee
2018-04-20 16:47   ` Jakub Narebski
2018-04-17 18:10 ` [RFC PATCH 09/12] fsck: check commit-graph Derrick Stolee
2018-04-20 16:59   ` Jakub Narebski
2018-04-17 18:10 ` [RFC PATCH 10/12] commit-graph: add '--reachable' option Derrick Stolee
2018-04-20 17:17   ` Jakub Narebski
2018-04-17 18:10 ` [RFC PATCH 11/12] gc: automatically write commit-graph files Derrick Stolee
2018-04-20 17:34   ` Jakub Narebski
2018-04-20 18:33     ` Ævar Arnfjörð Bjarmason
2018-04-17 18:10 ` [RFC PATCH 12/12] commit-graph: update design document Derrick Stolee
2018-04-20 19:10   ` Jakub Narebski
2018-04-17 18:50 ` [RFC PATCH 00/12] Integrate commit-graph into 'fsck' and 'gc' Derrick Stolee
2018-05-10 17:34 ` [PATCH 00/12] Integrate commit-graph into fsck, gc, and fetch Derrick Stolee
2018-05-10 17:34   ` [PATCH 01/12] commit-graph: add 'verify' subcommand Derrick Stolee
2018-05-10 18:15     ` Martin Ågren
2018-05-10 17:34   ` [PATCH 02/12] commit-graph: verify file header information Derrick Stolee
2018-05-10 18:21     ` Martin Ågren
2018-05-10 17:34   ` [PATCH 03/12] commit-graph: parse commit from chosen graph Derrick Stolee
2018-05-10 17:34   ` [PATCH 04/12] commit-graph: verify fanout and lookup table Derrick Stolee
2018-05-10 18:29     ` Martin Ågren
2018-05-11 15:17       ` Derrick Stolee
2018-05-10 17:34   ` [PATCH 05/12] commit: force commit to parse from object database Derrick Stolee
2018-05-10 17:34   ` [PATCH 06/12] commit-graph: load a root tree from specific graph Derrick Stolee
2018-05-10 17:34   ` [PATCH 07/12] commit-graph: verify commit contents against odb Derrick Stolee
2018-05-10 17:34   ` [PATCH 08/12] fsck: verify commit-graph Derrick Stolee
2018-05-10 17:34   ` [PATCH 09/12] commit-graph: add '--reachable' option Derrick Stolee
2018-05-10 17:34   ` [PATCH 10/12] gc: automatically write commit-graph files Derrick Stolee
2018-05-10 17:34   ` [PATCH 11/12] fetch: compute commit-graph by default Derrick Stolee
2018-05-10 17:34   ` [PATCH 12/12] commit-graph: update design document Derrick Stolee
2018-05-10 19:05   ` [PATCH 00/12] Integrate commit-graph into fsck, gc, and fetch Martin Ågren
2018-05-10 19:22     ` Stefan Beller
2018-05-11 17:23       ` Derrick Stolee
2018-05-11 17:30         ` Martin Ågren
2018-05-10 19:17   ` Ævar Arnfjörð Bjarmason
2018-05-11 17:23     ` Derrick Stolee
2018-05-11 21:15   ` [PATCH v2 00/12] Integrate commit-graph into fsck and gc Derrick Stolee
2018-05-11 21:15     ` [PATCH v2 01/12] commit-graph: add 'verify' subcommand Derrick Stolee
2018-05-12 13:31       ` Martin Ågren
2018-05-14 13:27         ` Derrick Stolee
2018-05-20 12:10       ` Jakub Narebski
2018-05-11 21:15     ` [PATCH v2 02/12] commit-graph: verify file header information Derrick Stolee
2018-05-12 13:35       ` Martin Ågren
2018-05-14 13:31         ` Derrick Stolee
2018-05-20 20:00       ` Jakub Narebski
2018-05-11 21:15     ` [PATCH v2 03/12] commit-graph: test that 'verify' finds corruption Derrick Stolee
2018-05-12 13:43       ` Martin Ågren
2018-05-21 18:53       ` Jakub Narebski
2018-05-24 16:28         ` Derrick Stolee
2018-05-11 21:15     ` [PATCH v2 04/12] commit-graph: parse commit from chosen graph Derrick Stolee
2018-05-12 20:50       ` Martin Ågren
2018-05-11 21:15     ` [PATCH v2 05/12] commit-graph: verify fanout and lookup table Derrick Stolee
2018-05-11 21:15     ` [PATCH v2 06/12] commit: force commit to parse from object database Derrick Stolee
2018-05-12 20:54       ` Martin Ågren
2018-05-11 21:15     ` [PATCH v2 07/12] commit-graph: load a root tree from specific graph Derrick Stolee
2018-05-12 20:55       ` Martin Ågren
2018-05-11 21:15     ` [PATCH v2 08/12] commit-graph: verify commit contents against odb Derrick Stolee
2018-05-12 21:17       ` Martin Ågren
2018-05-14 13:44         ` Derrick Stolee
2018-05-15 21:12       ` Martin Ågren
2018-05-11 21:15     ` [PATCH v2 09/12] fsck: verify commit-graph Derrick Stolee
2018-05-17 18:13       ` Martin Ågren
2018-05-11 21:15     ` [PATCH v2 10/12] commit-graph: add '--reachable' option Derrick Stolee
2018-05-17 18:16       ` Martin Ågren
2018-05-11 21:15     ` [PATCH v2 11/12] gc: automatically write commit-graph files Derrick Stolee
2018-05-17 18:20       ` Martin Ågren
2018-05-11 21:15     ` [PATCH v2 12/12] commit-graph: update design document Derrick Stolee
2018-05-24 16:25     ` [PATCH v3 00/20] Integrate commit-graph into 'fsck' and 'gc' Derrick Stolee
2018-05-24 16:25       ` [PATCH v3 01/20] commit-graph: UNLEAK before die() Derrick Stolee
2018-05-24 22:47         ` Stefan Beller
2018-05-25  0:08           ` Derrick Stolee
2018-05-24 16:25       ` [PATCH v3 02/20] commit-graph: fix GRAPH_MIN_SIZE Derrick Stolee
2018-05-26 18:46         ` Jakub Narebski
2018-05-26 20:30           ` brian m. carlson
2018-06-02 19:43             ` Jakub Narebski
2018-05-24 16:25       ` [PATCH v3 03/20] commit-graph: parse commit from chosen graph Derrick Stolee
2018-05-27 10:23         ` Jakub Narebski
2018-05-29 12:31           ` Derrick Stolee
2018-05-24 16:25       ` [PATCH v3 04/20] commit: force commit to parse from object database Derrick Stolee
2018-05-27 18:04         ` Jakub Narebski
2018-05-24 16:25       ` [PATCH v3 05/20] commit-graph: load a root tree from specific graph Derrick Stolee
2018-05-27 19:12         ` Jakub Narebski
2018-05-24 16:25       ` [PATCH v3 06/20] commit-graph: add 'verify' subcommand Derrick Stolee
2018-05-27 22:55         ` Jakub Narebski
2018-05-30 16:07           ` Derrick Stolee
2018-06-02 21:19             ` Jakub Narebski
2018-06-04 11:30               ` Derrick Stolee
2018-05-24 16:25       ` [PATCH v3 07/20] commit-graph: verify catches corrupt signature Derrick Stolee
2018-05-28 14:05         ` Jakub Narebski
2018-05-29 12:43           ` Derrick Stolee
2018-06-02 22:30             ` Jakub Narebski
2018-05-24 16:25       ` [PATCH v3 08/20] commit-graph: verify required chunks are present Derrick Stolee
2018-05-28 17:11         ` Jakub Narebski
2018-05-24 16:25       ` [PATCH v3 09/20] commit-graph: verify corrupt OID fanout and lookup Derrick Stolee
2018-05-30 13:34         ` Jakub Narebski
2018-05-30 16:18           ` Derrick Stolee
2018-06-02  4:38         ` Duy Nguyen
2018-06-04 11:32           ` Derrick Stolee
2018-06-04 14:42             ` Duy Nguyen
2018-05-24 16:25       ` [PATCH v3 10/20] commit-graph: verify objects exist Derrick Stolee
2018-05-30 19:22         ` Jakub Narebski
2018-05-31 12:53           ` Derrick Stolee
2018-05-24 16:25       ` [PATCH v3 11/20] commit-graph: verify root tree OIDs Derrick Stolee
2018-05-30 22:24         ` Jakub Narebski
2018-05-31 13:16           ` Derrick Stolee
2018-06-02 22:50             ` Jakub Narebski
2018-05-24 16:25       ` [PATCH v3 12/20] commit-graph: verify parent list Derrick Stolee
2018-06-01 23:21         ` Jakub Narebski
2018-05-24 16:25       ` [PATCH v3 13/20] commit-graph: verify generation number Derrick Stolee
2018-06-02 12:23         ` Jakub Narebski
2018-06-04 11:47           ` Derrick Stolee
2018-05-24 16:25       ` [PATCH v3 14/20] commit-graph: verify commit date Derrick Stolee
2018-06-02 12:29         ` Jakub Narebski
2018-05-24 16:25       ` [PATCH v3 15/20] commit-graph: test for corrupted octopus edge Derrick Stolee
2018-06-02 12:39         ` Jakub Narebski
2018-06-04 13:08           ` Derrick Stolee
2018-05-24 16:26       ` [PATCH v3 16/20] commit-graph: verify contents match checksum Derrick Stolee
2018-05-30 12:35         ` SZEDER Gábor
2018-06-02 15:52         ` Jakub Narebski
2018-06-04 11:55           ` Derrick Stolee
2018-05-24 16:26       ` [PATCH v3 17/20] fsck: verify commit-graph Derrick Stolee
2018-06-02 16:17         ` Jakub Narebski
2018-06-04 11:59           ` Derrick Stolee
2018-05-24 16:26       ` [PATCH v3 18/20] commit-graph: add '--reachable' option Derrick Stolee
2018-06-02 17:34         ` Jakub Narebski
2018-06-04 12:44           ` Derrick Stolee
2018-05-24 16:26       ` [PATCH v3 19/20] gc: automatically write commit-graph files Derrick Stolee
2018-06-02 18:03         ` Jakub Narebski [this message]
2018-06-04 12:51           ` Derrick Stolee
2018-05-24 16:26       ` [PATCH v3 20/20] commit-graph: update design document Derrick Stolee
2018-06-02 18:27         ` Jakub Narebski
2018-05-24 21:15       ` [PATCH v3 00/20] Integrate commit-graph into 'fsck' and 'gc' Ævar Arnfjörð Bjarmason
2018-05-25  4:11       ` Junio C Hamano
2018-05-29  4:27       ` Junio C Hamano
2018-05-29 12:37         ` Derrick Stolee
2018-05-29 13:41           ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=866031rrq1.fsf@gmail.com \
    --to=jnareb@gmail.com \
    --cc=avarab@gmail.com \
    --cc=dstolee@microsoft.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=marten.agren@gmail.com \
    --cc=peff@peff.net \
    --cc=stolee@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).