Skip to content

Commitgraph: add new config for changed-paths and recommend it in scalar #1983

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+79 −0
Open

Commitgraph: add new config for changed-paths and recommend it in scalar #1983

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions Documentation/config/commitgraph.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,14 @@ commitGraph.maxNewFilters::
Specifies the default value for the `--max-new-filters` option of `git
commit-graph write` (c.f., linkgit:git-commit-graph[1]).

commitGraph.changedPaths::
If true, then `git commit-graph write` will compute and write
changed-path Bloom filters by default, equivalent to passing
`--changed-paths`. If false or unset, changed-path Bloom filters
will only be written when explicitly requested via `--changed-paths`.
Command-line options always take precedence over this configuration.
Defaults to unset.

commitGraph.readChangedPaths::
Deprecated. Equivalent to commitGraph.changedPathsVersion=-1 if true, and
commitGraph.changedPathsVersion=0 if false. (If commitGraph.changedPathVersion
Expand Down
5 changes: 5 additions & 0 deletions builtin/commit-graph.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,11 @@ static int git_commit_graph_write_config(const char *var, const char *value,
{
if (!strcmp(var, "commitgraph.maxnewfilters"))
write_opts.max_new_filters = git_config_int(var, value, ctx->kvi);
else if (!strcmp(var, "commitgraph.changedpaths")) {
/* Only set to 1 if config is true and not already overridden by command line */
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this config is loaded before the command-line arguments, we don't want to do this check for opts.enable_changed_paths == -1. As we parse the config, we may have duplicate key-value pairs in system, global, or local config and we need to use a "last one wins" approach.

We should consider the ability for global config to have it set to true and the local config set to false. Right now, the false value is ignored. Would opts.enable_changed_paths = git_config_bool(var, value) ? 1 : -1; give us the right behavior (false disables the previous true but doesn't actually erase changed-path filters that may otherwise exist).

if (opts.enable_changed_paths == -1 && git_config_bool(var, value))
opts.enable_changed_paths = 1;
}
/*
* No need to fall-back to 'git_default_config', since this was already
* called in 'cmd_commit_graph()'.
Expand Down
1 change: 1 addition & 0 deletions scalar.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,7 @@ static int set_recommended_config(int reconfigure)
#endif
/* Optional */
{ "status.aheadBehind", "false" },
{ "commitGraph.changedPaths", "true" },
{ "commitGraph.generationVersion", "1" },
{ "core.autoCRLF", "false" },
{ "core.safeCRLF", "false" },
Expand Down
65 changes: 65 additions & 0 deletions t/t5318-commit-graph.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -946,4 +946,69 @@ test_expect_success 'stale commit cannot be parsed when traversing graph' '
)
'

test_expect_success 'commitGraph.changedPaths=true acts like --changed-paths' '
git init config-changed-paths-true &&
(
cd config-changed-paths-true &&
test_commit initial &&
test_commit second &&
test_commit third &&
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's a case that we should add:

Global config has true, local config has false. (Alternatively: use git config --add to have multiple values in the same config file, true then false.)

These tests seem complete and correct, and now I'm providing feedback that is more a matter of "taste" and a preference for succinct tests (especially around config options).

Test setup ideas: we only need a single "new" commit to trigger new changed-path filter computations in git commit-graph write. We can use one test to toggle config values (and the state of the commit-graph having filters or not).

I'm thinking through a sequence of events that will help (consider a test_commit before each case):

  1. Write commit-graph without --changed-paths. See no progress for them.
  2. Config to true. Write without --changed-paths. See progress.
  3. Add config as false on top of true. Write without --changed-paths. See progress (due to existing filters).
  4. Keep config as in previous test, but delete commit-graph and write without --changed-paths. Don't see progress.
  5. Clear config and set to false. Write with --changed-paths and see progress (due to CLI).
  6. Keep false config, write without --changed-paths and see progress (due to existing filters).

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests seem complete and correct, and now I'm providing feedback that is more a matter of "taste" and a preference for succinct tests (especially around config options).

Another minor piece of style/taste feedback (and please defer to @derrickstolee and @dscho who are familiar with the preferred style in Git tests).

I noticed that all 3 tests are following the same basic approach:

  1. Configure commitGraph.changedPaths
  2. Confirm bloom filter behavior works as expected based on config value setting
  3. Confirm that --changed-paths and --no-changed-paths work as expected, and take precedence over commitGraph.changedPaths

However, the test titles are currently a bit inconsistent:

  • 'commitGraph.changedPaths=true acts like --changed-paths'
  • 'commitGraph.changedPaths=false respects command line options'
  • 'commitGraph.changedPaths=unset respects command line options'

If we stick with three separate tests, I recommend using the same name scheme across all three. When I first read the titles, I mistakenly thought that we might need another test to validate that commitGraph.changedPaths=true respects command line options.

# set commitGraph.changedPaths to true and it should write bloom filters
git config commitGraph.changedPaths true &&
GIT_PROGRESS_DELAY=0 git commit-graph write --reachable --progress 2>error &&
grep -i "bloom filters" error &&
# --no-changed-paths should take precedence over the config and erase bloom filters
GIT_PROGRESS_DELAY=0 git commit-graph write --no-changed-paths --reachable --progress 2>error &&
! grep -i "bloom filters" error &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently there is some white-space on this otherwise empty line:

--- 61c1ad542c Commitgraph: add new config for changed-paths and recommend it in scalar
t/t5318-commit-graph.sh:965: trailing whitespace.
+

Would you mind deleting that, amending the commit, and force-pushing?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or: git rebase --whitespace=fix HEAD~1 should work for this single patch.

# --changed-paths should take precedence over the config and add bloom filters
GIT_PROGRESS_DELAY=0 git commit-graph write --changed-paths --reachable --progress 2>error &&
grep -i "bloom filters" error
)
'

test_expect_success 'commitGraph.changedPaths=false respects command line options' '
git init config-changed-paths-false &&
(
cd config-changed-paths-false &&
test_commit initial &&
test_commit second &&
test_commit third &&
# set commitGraph.changedPaths to false and it should not write bloom filters
git config commitGraph.changedPaths false &&
GIT_PROGRESS_DELAY=0 git commit-graph write --reachable --progress 2>error &&
! grep -i "bloom filters" error &&
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a test_grep helper that will help with this, I think:

test_grep ! "bloom filters" error &&

or drop the ! when you expect the match.

# --changed-paths should take precedence over the config and add bloom filters
GIT_PROGRESS_DELAY=0 git commit-graph write --changed-paths --reachable --progress 2>error &&
grep -i "bloom filters" error &&
# --no-changed-paths should take precedence over the config and erase bloom filters
GIT_PROGRESS_DELAY=0 git commit-graph write --no-changed-paths --reachable --progress 2>error &&
! grep -i "bloom filters" error
)
'

test_expect_success 'commitGraph.changedPaths=unset respects command line options' '
git init config-changed-paths-unset &&
(
cd config-changed-paths-unset &&
test_commit initial &&
test_commit second &&
test_commit third &&
# commitGraph.changedPaths is not set and it should always respect command line options
GIT_PROGRESS_DELAY=0 git commit-graph write --reachable --progress 2>error &&
! grep -i "bloom filters" error &&
GIT_PROGRESS_DELAY=0 git commit-graph write --changed-paths --reachable --progress 2>error &&
grep -i "bloom filters" error &&
GIT_PROGRESS_DELAY=0 git commit-graph write --no-changed-paths --reachable --progress 2>error &&
! grep -i "bloom filters" error
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another super minor piece of style feedback: I noticed that the guts of this test are doing the same as the two new tests above, but the spacing & comments here are inconsistent with the tests above.

)
'

test_done
Loading