Skip to content

read-cache: use index state repository for trace2 logging

Jayesh Daga via GitGitGadget git 2 versions – New

Get series with b4
b4 am [email protected]

Versions newest first · linked by author and title

  1. v3 Latest2 patches 0 repliesNew
  2. v11 patch 3 repliesSuperseded

Changes in v3 from the cover letter

* Apply the change consistently across read-cache.c
* Drop unnecessary intermediate variable
* Remove obsolete TODO comments
* Update commit message

Patches in v3 No applies recorded yet

#Subject and filesSizeRepliesApplied
1/2repo: add paths.git_dir repo info keyDocumentation/git-repo.adoc builtin/repo.c t/t1900-repo-info.sh+22 -00 replies
2/2read-cache: use istate->repo for trace2 loggingread-cache.c+8 -240 replies

Reviews trailers per patch

No Reviewed-by, Acked-by or Tested-by trailers for v3 yet.

Read the v3 thread

Cover letter [PATCH v3 0/2] [GSoC] read-cache: use index state repository for trace2 logging

trace2 calls in read-cache.c use the global 'the_repository', even though the relevant index_state provides an explicit repository pointer via 'istate->repo'.

Using the global repository can result in incorrect trace2 output when multiple repository instances are in use, as events may be attributed to the wrong repository.

Use 'istate->repo' instead in these call sites to ensure correct repository attribution.

Changes since v2:

* Apply the change consistently across read-cache.c
* Drop unnecessary intermediate variable
* Remove obsolete TODO comments
* Update commit message

Jayesh Daga (1):
read-cache: use istate->repo for trace2 logging

jayesh0104 (1):
repo: add paths.git_dir repo info key

 Documentation/git-repo.adoc |  5 +++++
 builtin/repo.c              |  7 +++++++
 read-cache.c                | 32 ++++++++------------------------
 t/t1900-repo-info.sh        | 10 ++++++++++
 4 files changed, 30 insertions(+), 24 deletions(-)

base-commit: ca1db8a0f7dc0dbea892e99f5b37c5fe5861be71
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2253%2Fjayesh0104%2Ftrace2-istate-repo-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2253/jayesh0104/trace2-istate-repo-v3
Pull-Request: https://github.com/git/git/pull/2253

Range-diff vs v2:

 -:  ---------- > 1:  5a8165b05d repo: add paths.git_dir repo info key
 1:  7fdfaaef9b ! 2:  9bb6d0fa01 read-cache: use istate->repo for trace2 logging

@@ Metadata
## Commit message ##
read-cache: use istate->repo for trace2 logging

- trace2_data_intmax() calls in do_read_index() currently use the
- global 'the_repository' instance, even though the index_state
- already carries an explicit repository pointer (istate->repo).
+ trace2 calls in read-cache.c use the global 'the_repository',
+ even though the relevant index_state provides an explicit
+ repository pointer via 'istate->repo'.

- index_state instances are initialized via INDEX_STATE_INIT(r),
- which sets istate->repo. Using the_repository here is therefore
- redundant and obscures the actual repository context associated
- with the index being read.
+ Using the global repository can result in incorrect trace2
+ output when multiple repository instances are in use, as
+ events may be attributed to the wrong repository.

- In particular, using the global repository can lead to misleading
- trace2 output in scenarios where multiple repository instances are
- in use (such as tests or future refactoring toward better library
- boundaries), as events may be attributed to the wrong repository.
-
- Switch these calls to use istate->repo directly, making the
- association between the index and its repository explicit.
-
- Since istate->repo is expected to be non-NULL, enforce this
- assumption with a BUG() check so that any violation of this
- invariant is caught early.
-
- Also remove the now-obsolete TODO comment.
+ Use 'istate->repo' instead to ensure correct repository
+ attribution.

## read-cache.c ##
-@@ read-cache.c: int do_read_index(struct index_state *istate, const char *path, int must_exist)
- size_t extension_offset = 0;
- int nr_threads, cpus;
- struct index_entry_offset_table *ieot = NULL;
-+ struct repository *r;
-
- if (istate->initialized)
- return istate->cache_nr;
@@ read-cache.c: int do_read_index(struct index_state *istate, const char *path, int must_exist)
}
munmap((void *)mmap, mmap_size);
@@ read-cache.c: int do_read_index(struct index_state *istate, const char *path, in
- * that is associated with the given "istate".
- */
- trace2_data_intmax("index", the_repository, "read/version",
-+ r=istate->repo;
-+ if (!r)
-+ BUG("istate->repo is NULL in do_read_index");
-+ trace2_data_intmax("index", r, "read/version",
++ trace2_data_intmax("index", istate->repo, "read/version",
istate->version);
- trace2_data_intmax("index", the_repository, "read/cache_nr",
-+ trace2_data_intmax("index", r, "read/cache_nr",
++ trace2_data_intmax("index", istate->repo, "read/cache_nr",
istate->cache_nr);

/*
+@@ read-cache.c: int read_index_from(struct index_state *istate, const char *path,
+ if (istate->initialized)
+ return istate->cache_nr;
+
+- /*
+- * TODO trace2: replace "the_repository" with the actual repo instance
+- * that is associated with the given "istate".
+- */
+- trace2_region_enter_printf("index", "do_read_index", the_repository,
++ trace2_region_enter_printf("index", "do_read_index", istate->repo,
+ "%s", path);
+ trace_performance_enter();
+ ret = do_read_index(istate, path, 0);
+ trace_performance_leave("read cache %s", path);
+- trace2_region_leave_printf("index", "do_read_index", the_repository,
++ trace2_region_leave_printf("index", "do_read_index", istate->repo,
+ "%s", path);
+
+ split_index = istate->split_index;
+@@ read-cache.c: static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
+ istate->timestamp.nsec = ST_MTIME_NSEC(st);
+ trace_performance_since(start, "write index, changed mask = %x", istate->cache_changed);
+
+- /*
+- * TODO trace2: replace "the_repository" with the actual repo instance
+- * that is associated with the given "istate".
+- */
+- trace2_data_intmax("index", the_repository, "write/version",
++ trace2_data_intmax("index", istate->repo, "write/version",
+ istate->version);
+- trace2_data_intmax("index", the_repository, "write/cache_nr",
++ trace2_data_intmax("index", istate->repo, "write/cache_nr",
+ istate->cache_nr);
+
+ ret = 0;
+@@ read-cache.c: static int do_write_locked_index(struct index_state *istate,
+ return ret;
+ }
+
+- /*
+- * TODO trace2: replace "the_repository" with the actual repo instance
+- * that is associated with the given "istate".
+- */
+- trace2_region_enter_printf("index", "do_write_index", the_repository,
++ trace2_region_enter_printf("index", "do_write_index", istate->repo,
+ "%s", get_lock_file_path(lock));
+ ret = do_write_index(istate, lock->tempfile, write_extensions, flags);
+- trace2_region_leave_printf("index", "do_write_index", the_repository,
++ trace2_region_leave_printf("index", "do_write_index", istate->repo,
+ "%s", get_lock_file_path(lock));
+
+ if (was_full)

Signature · 1 line
gitgitgadget