* Re: [PATCH] rev-parse --git-path: fix output when running in a subdirectory
From: Johannes Schindelin @ 2017-02-10 15:44 UTC (permalink / raw)
To: Mike Rappazzo; +Cc: Junio C Hamano, Duy Nguyen, Git Mailing List
In-Reply-To: <CANoM8SV7oJ6YmKM-n63620EkODxD562BZnLZB6OvX8O6BmDT1A@mail.gmail.com>
Hi Mike,
On Thu, 9 Feb 2017, Mike Rappazzo wrote:
> On Thu, Feb 9, 2017 at 5:54 PM, Junio C Hamano <gitster@pobox.com> wrote:
> >
> > That leaves what the right single-step behaviour change should be. As
> > I recall Duy said something about --common-dir and other things Mike's
> > earlier change also covered, I'd prefer to leave it to three of you to
> > figure out what the final patch should be.
> >
>
> The tests which I covered in my previous patch [1] addressed the places
> where we identified similar problems. We should try to include some
> form of those tests. As far as implementation goes in rev-parse, the
> version in this thread is probably better that what I had, but it would
> need to also be applied to --git-common-dir and --shared-index-path.
Thank you so much for pointing out that git-common-dir and
shared-index-path have the same problem.
I looked a little further, and it seems that the show_file() function may
have the exact same problem... but then, it only prefixes filenames if the
--prefix=<prefix> option has been passed, and it could be argued that
those prefixed filenames are *not* meant to be relative to the cwd but to
the top-level directory.
Anways, v2 was just sent out, and with Peff's acknowledgement that this
fixes a real bug and that hypothetical scripts relying on the buggy
behavior were broken beyond repair even without worktrees anyway, I am
hopeful that we'll get somewhere.
Ciao,
Johannes
^ permalink raw reply
* [PATCH v2 2/2] rev-parse: fix several options when running in a subdirectory
From: Johannes Schindelin @ 2017-02-10 15:33 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy,
Michael Rappazzo
In-Reply-To: <cover.1486740772.git.johannes.schindelin@gmx.de>
In addition to making git_path() aware of certain file names that need
to be handled differently e.g. when running in worktrees, the commit
557bd833bb (git_path(): be aware of file relocation in $GIT_DIR,
2014-11-30) also snuck in a new option for `git rev-parse`:
`--git-path`.
On the face of it, there is no obvious bug in that commit's diff: it
faithfully calls git_path() on the argument and prints it out, i.e. `git
rev-parse --git-path <filename>` has the same precise behavior as
calling `git_path("<filename>")` in C.
The problem lies deeper, much deeper. In hindsight (which is always
unfair), implementing the .git/ directory discovery in
`setup_git_directory()` by changing the working directory may have
allowed us to avoid passing around a struct that contains information
about the current repository, but it bought us many, many problems.
In this case, when being called in a subdirectory, `git rev-parse`
changes the working directory to the top-level directory before calling
`git_path()`. In the new working directory, the result is correct. But
in the working directory of the calling script, it is incorrect.
Example: when calling `git rev-parse --git-path HEAD` in, say, the
Documentation/ subdirectory of Git's own source code, the string
`.git/HEAD` is printed.
Side note: that bug is hidden when running in a subdirectory of a
worktree that was added by the `git worktree` command: in that case, the
(correct) absolute path of the `HEAD` file is printed.
In the interest of time, this patch does not go the "correct" route to
introduce a struct with repository information (and removing global
state in the process), instead this patch chooses to detect when the
command was called in a subdirectory and forces the result to be an
absolute path.
While at it, we are also fixing the output of --git-common-dir and
--shared-index-path.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/rev-parse.c | 15 +++++++++++----
t/t1500-rev-parse.sh | 4 ++--
t/t1700-split-index.sh | 2 +-
t/t2027-worktree-list.sh | 4 ++--
4 files changed, 16 insertions(+), 9 deletions(-)
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index ff13e59e1db..84af2802f6f 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -545,6 +545,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
unsigned int flags = 0;
const char *name = NULL;
struct object_context unused;
+ struct strbuf buf = STRBUF_INIT;
if (argc > 1 && !strcmp("--parseopt", argv[1]))
return cmd_parseopt(argc - 1, argv + 1, prefix);
@@ -599,7 +600,9 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
if (!strcmp(arg, "--git-path")) {
if (!argv[i + 1])
die("--git-path requires an argument");
- puts(git_path("%s", argv[i + 1]));
+ strbuf_reset(&buf);
+ puts(relative_path(git_path("%s", argv[i + 1]),
+ prefix, &buf));
i++;
continue;
}
@@ -821,8 +824,9 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
continue;
}
if (!strcmp(arg, "--git-common-dir")) {
- const char *pfx = prefix ? prefix : "";
- puts(prefix_filename(pfx, strlen(pfx), get_git_common_dir()));
+ strbuf_reset(&buf);
+ puts(relative_path(get_git_common_dir(),
+ prefix, &buf));
continue;
}
if (!strcmp(arg, "--is-inside-git-dir")) {
@@ -845,7 +849,9 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
die(_("Could not read the index"));
if (the_index.split_index) {
const unsigned char *sha1 = the_index.split_index->base_sha1;
- puts(git_path("sharedindex.%s", sha1_to_hex(sha1)));
+ const char *path = git_path("sharedindex.%s", sha1_to_hex(sha1));
+ strbuf_reset(&buf);
+ puts(relative_path(path, prefix, &buf));
}
continue;
}
@@ -906,5 +912,6 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
die_no_single_rev(quiet);
} else
show_default();
+ strbuf_release(&buf);
return 0;
}
diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index f39f783f2db..d74f09ad93e 100755
--- a/t/t1500-rev-parse.sh
+++ b/t/t1500-rev-parse.sh
@@ -93,7 +93,7 @@ test_expect_success 'git-common-dir from worktree root' '
test_cmp expect actual
'
-test_expect_failure 'git-common-dir inside sub-dir' '
+test_expect_success 'git-common-dir inside sub-dir' '
mkdir -p path/to/child &&
test_when_finished "rm -rf path" &&
echo "$(git -C path/to/child rev-parse --show-cdup).git" >expect &&
@@ -107,7 +107,7 @@ test_expect_success 'git-path from worktree root' '
test_cmp expect actual
'
-test_expect_failure 'git-path inside sub-dir' '
+test_expect_success 'git-path inside sub-dir' '
mkdir -p path/to/child &&
test_when_finished "rm -rf path" &&
echo "$(git -C path/to/child rev-parse --show-cdup).git/objects" >expect &&
diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
index 1d6e27a09d8..446ff34f966 100755
--- a/t/t1700-split-index.sh
+++ b/t/t1700-split-index.sh
@@ -200,7 +200,7 @@ EOF
test_cmp expect actual
'
-test_expect_failure 'rev-parse --shared-index-path' '
+test_expect_success 'rev-parse --shared-index-path' '
rm -rf .git &&
test_create_repo . &&
git update-index --split-index &&
diff --git a/t/t2027-worktree-list.sh b/t/t2027-worktree-list.sh
index c1a072348e7..848da5f3684 100755
--- a/t/t2027-worktree-list.sh
+++ b/t/t2027-worktree-list.sh
@@ -8,7 +8,7 @@ test_expect_success 'setup' '
test_commit init
'
-test_expect_failure 'rev-parse --git-common-dir on main worktree' '
+test_expect_success 'rev-parse --git-common-dir on main worktree' '
git rev-parse --git-common-dir >actual &&
echo .git >expected &&
test_cmp expected actual &&
@@ -18,7 +18,7 @@ test_expect_failure 'rev-parse --git-common-dir on main worktree' '
test_cmp expected2 actual2
'
-test_expect_failure 'rev-parse --git-path objects linked worktree' '
+test_expect_success 'rev-parse --git-path objects linked worktree' '
echo "$(git rev-parse --show-toplevel)/.git/objects" >expect &&
test_when_finished "rm -rf linked-tree && git worktree prune" &&
git worktree add --detach linked-tree master &&
--
2.11.1.windows.1
^ permalink raw reply related
* [PATCH v2 0/2] Fix bugs in rev-parse's output when run in a subdirectory
From: Johannes Schindelin @ 2017-02-10 15:33 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy,
Michael Rappazzo
In-Reply-To: <50fe3ea3302c40f4c96eaa5a568837e3334f9dc4.1486555851.git.johannes.schindelin@gmx.de>
The bug that bit me (hard!) and that triggered not only a long series of
curses but also my writing a patch and sending it to the list was that
`git rev-parse --git-path HEAD` would give *incorrect* output when run
in a subdirectory of a regular checkout, but *correct* output when run
in a subdirectory of an associated *worktree*.
I had tested the script in question quite a bit, but in a worktree. And
in production, it quietly did exactly the wrong thing.
Relative to v1 of the then-single patch, this changed:
- the patch now covers also --git-common-dir and --shared-index-path
- the output is now a relative path when git_dir is relative
- I plucked the tests from Mike's patch series and dropped my ad-hoc
test from t0060.
- While at it, I fixed Mike's test that assumed that the objects/
directory lives in .git/worktrees/<name>/objects/.
Johannes Schindelin (1):
rev-parse: fix several options when running in a subdirectory
Michael Rappazzo (1):
rev-parse tests: add tests executed from a subdirectory
builtin/rev-parse.c | 15 +++++++++++----
t/t1500-rev-parse.sh | 28 ++++++++++++++++++++++++++++
t/t1700-split-index.sh | 17 +++++++++++++++++
t/t2027-worktree-list.sh | 10 +++++++++-
4 files changed, 65 insertions(+), 5 deletions(-)
base-commit: 6e3a7b3398559305c7a239a42e447c21a8f39ff8
Published-As: https://github.com/dscho/git/releases/tag/git-path-in-subdir-v2
Fetch-It-Via: git fetch https://github.com/dscho/git git-path-in-subdir-v2
Interdiff vs v1:
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index f9d5762bf2b..84af2802f6f 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -545,6 +545,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
unsigned int flags = 0;
const char *name = NULL;
struct object_context unused;
+ struct strbuf buf = STRBUF_INIT;
if (argc > 1 && !strcmp("--parseopt", argv[1]))
return cmd_parseopt(argc - 1, argv + 1, prefix);
@@ -597,13 +598,11 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
}
if (!strcmp(arg, "--git-path")) {
- const char *path;
if (!argv[i + 1])
die("--git-path requires an argument");
- path = git_path("%s", argv[i + 1]);
- if (prefix && !is_absolute_path(path))
- path = real_path(path);
- puts(path);
+ strbuf_reset(&buf);
+ puts(relative_path(git_path("%s", argv[i + 1]),
+ prefix, &buf));
i++;
continue;
}
@@ -825,8 +824,9 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
continue;
}
if (!strcmp(arg, "--git-common-dir")) {
- const char *pfx = prefix ? prefix : "";
- puts(prefix_filename(pfx, strlen(pfx), get_git_common_dir()));
+ strbuf_reset(&buf);
+ puts(relative_path(get_git_common_dir(),
+ prefix, &buf));
continue;
}
if (!strcmp(arg, "--is-inside-git-dir")) {
@@ -849,7 +849,9 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
die(_("Could not read the index"));
if (the_index.split_index) {
const unsigned char *sha1 = the_index.split_index->base_sha1;
- puts(git_path("sharedindex.%s", sha1_to_hex(sha1)));
+ const char *path = git_path("sharedindex.%s", sha1_to_hex(sha1));
+ strbuf_reset(&buf);
+ puts(relative_path(path, prefix, &buf));
}
continue;
}
@@ -910,5 +912,6 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
die_no_single_rev(quiet);
} else
show_default();
+ strbuf_release(&buf);
return 0;
}
diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index 790584fcc54..444b5a4df80 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -271,8 +271,6 @@ relative_path "<null>" "<empty>" ./
relative_path "<null>" "<null>" ./
relative_path "<null>" /foo/a/b ./
-test_git_path "mkdir sub && cd sub && test_when_finished cd .. &&" \
- foo "$(pwd)/.git/foo"
test_git_path A=B info/grafts .git/info/grafts
test_git_path GIT_GRAFT_FILE=foo info/grafts foo
test_git_path GIT_GRAFT_FILE=foo info/////grafts foo
diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index 038e24c4014..d74f09ad93e 100755
--- a/t/t1500-rev-parse.sh
+++ b/t/t1500-rev-parse.sh
@@ -87,4 +87,32 @@ test_rev_parse -C work -g ../repo.git -b t 'GIT_DIR=../repo.git, core.bare = tru
test_rev_parse -C work -g ../repo.git -b u 'GIT_DIR=../repo.git, core.bare undefined' false false true ''
+test_expect_success 'git-common-dir from worktree root' '
+ echo .git >expect &&
+ git rev-parse --git-common-dir >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'git-common-dir inside sub-dir' '
+ mkdir -p path/to/child &&
+ test_when_finished "rm -rf path" &&
+ echo "$(git -C path/to/child rev-parse --show-cdup).git" >expect &&
+ git -C path/to/child rev-parse --git-common-dir >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'git-path from worktree root' '
+ echo .git/objects >expect &&
+ git rev-parse --git-path objects >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'git-path inside sub-dir' '
+ mkdir -p path/to/child &&
+ test_when_finished "rm -rf path" &&
+ echo "$(git -C path/to/child rev-parse --show-cdup).git/objects" >expect &&
+ git -C path/to/child rev-parse --git-path objects >actual &&
+ test_cmp expect actual
+'
+
test_done
diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
index 292a0720fcc..446ff34f966 100755
--- a/t/t1700-split-index.sh
+++ b/t/t1700-split-index.sh
@@ -200,4 +200,21 @@ EOF
test_cmp expect actual
'
+test_expect_success 'rev-parse --shared-index-path' '
+ rm -rf .git &&
+ test_create_repo . &&
+ git update-index --split-index &&
+ ls -t .git/sharedindex* | tail -n 1 >expect &&
+ git rev-parse --shared-index-path >actual &&
+ test_cmp expect actual &&
+ mkdir work &&
+ test_when_finished "rm -rf work" &&
+ (
+ cd work &&
+ ls -t ../.git/sharedindex* | tail -n 1 >expect &&
+ git rev-parse --shared-index-path >actual &&
+ test_cmp expect actual
+ )
+'
+
test_done
diff --git a/t/t2027-worktree-list.sh b/t/t2027-worktree-list.sh
index 465eeeacd3d..848da5f3684 100755
--- a/t/t2027-worktree-list.sh
+++ b/t/t2027-worktree-list.sh
@@ -14,10 +14,18 @@ test_expect_success 'rev-parse --git-common-dir on main worktree' '
test_cmp expected actual &&
mkdir sub &&
git -C sub rev-parse --git-common-dir >actual2 &&
- echo sub/.git >expected2 &&
+ echo ../.git >expected2 &&
test_cmp expected2 actual2
'
+test_expect_success 'rev-parse --git-path objects linked worktree' '
+ echo "$(git rev-parse --show-toplevel)/.git/objects" >expect &&
+ test_when_finished "rm -rf linked-tree && git worktree prune" &&
+ git worktree add --detach linked-tree master &&
+ git -C linked-tree rev-parse --git-path objects >actual &&
+ test_cmp expect actual
+'
+
test_expect_success '"list" all worktrees from main' '
echo "$(git rev-parse --show-toplevel) $(git rev-parse --short HEAD) [$(git symbolic-ref --short HEAD)]" >expect &&
test_when_finished "rm -rf here && git worktree prune" &&
--
2.11.1.windows.1
^ permalink raw reply
* [PATCH] preload-index: avoid lstat for skip-worktree items
From: Johannes Schindelin @ 2017-02-10 15:10 UTC (permalink / raw)
To: git; +Cc: Jeff Hostetler, Junio C Hamano
From: Jeff Hostetler <jeffhost@microsoft.com>
Teach preload-index to avoid lstat() calls for index-entries
with skip-worktree bit set. This is a performance optimization.
During a sparse-checkout, the skip-worktree bit is set on items
that were not populated and therefore are not present in the
worktree. The per-thread preload-index loop performs a series
of tests on each index-entry as it attempts to compare the
worktree version with the index and mark them up-to-date.
This patch short-cuts that work.
On a Windows 10 system with a very large repo (450MB index)
and various levels of sparseness, performance was improved
in the {preloadindex=true, fscache=false} case by 80% and
in the {preloadindex=true, fscache=true} case by 20% for various
commands.
Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Published-As: https://github.com/dscho/git/releases/tag/preload-index-sparse-v1
Fetch-It-Via: git fetch https://github.com/dscho/git preload-index-sparse-v1
preload-index.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/preload-index.c b/preload-index.c
index c1fe3a3ef9c..70a4c808783 100644
--- a/preload-index.c
+++ b/preload-index.c
@@ -53,6 +53,8 @@ static void *preload_thread(void *_data)
continue;
if (ce_uptodate(ce))
continue;
+ if (ce_skip_worktree(ce))
+ continue;
if (!ce_path_match(ce, &p->pathspec, NULL))
continue;
if (threaded_has_symlink_leading_path(&cache, ce->name, ce_namelen(ce)))
base-commit: 6e3a7b3398559305c7a239a42e447c21a8f39ff8
--
2.11.1.windows.1
^ permalink raw reply related
* [PATCH] fixup! bisect--helper: `bisect_next_check` & bisect_voc shell function in C
From: Johannes Schindelin @ 2017-02-10 14:20 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Pranit Bauva
It is curious that only MacOSX builds trigger an error about this, both
GCC and Clang, but not Linux GCC nor Clang (see
https://travis-ci.org/git/git/jobs/200182819#L1152 for details):
builtin/bisect--helper.c:299:6: error: variable 'good_syn' is used
uninitialized whenever 'if' condition is true
[-Werror,-Wsometimes-uninitialized]
if (missing_good && !missing_bad && current_term &&
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
builtin/bisect--helper.c:350:7: note: uninitialized use occurs here
if (!good_syn)
^~~~~~~~
If you "re-roll" (or, as pointed out at the Contributors' Summit, better
put: if you send another iteration of the patch series), please squash
this fix in.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Based-On: pu at https://github.com/dscho/git
Fetch-Base-Via: git fetch https://github.com/dscho/git pu
Published-As: https://github.com/dscho/git/releases/tag/bisect--helper-fixup-v1
Fetch-It-Via: git fetch https://github.com/dscho/git bisect--helper-fixup-v1
builtin/bisect--helper.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 8cd6527bd1..614a85ffb5 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -280,7 +280,7 @@ static int bisect_next_check(const struct bisect_terms *terms,
int missing_good = 1, missing_bad = 1, retval = 0;
char *bad_ref = xstrfmt("refs/bisect/%s", terms->term_bad);
char *good_glob = xstrfmt("%s-*", terms->term_good);
- char *bad_syn, *good_syn;
+ char *bad_syn = NULL, *good_syn = NULL;
if (ref_exists(bad_ref))
missing_bad = 0;
base-commit: 6fa4b393c01a84c9adf2e2435fba6de13227eabf
--
2.11.1.windows.1
^ permalink raw reply related
* Re: Bug with fixup and autosquash
From: Johannes Schindelin @ 2017-02-10 14:02 UTC (permalink / raw)
To: Philip Oakley
Cc: Junio C Hamano, Ashutosh Bapat, git, Michael Haggerty,
Michael J Gruber, Matthieu Moy
In-Reply-To: <454E7D934160418EB4C4871ED209CBCA@PhilipOakley>
Hi Philip,
On Thu, 9 Feb 2017, Philip Oakley wrote:
> From: "Johannes Schindelin" <Johannes.Schindelin@gmx.de>
> Sent: Thursday, February 09, 2017 8:55 PM
>
> > The rebase--helper code (specifically, the patch moving autosquash
> > logic into it: https://github.com/dscho/git/commit/7d0831637f) tries
> > to match exact onelines first,
>
> While I think this is an improvement, and will strongly support the `git
> commit --fixup=<commit>` option which will, if the sha1/oid is given,
> create the exact commit subject line.
That is already the case (with the exception that it is not the "exact
commit subject line" but the oneline, i.e. unwrapped first paragraph).
> However it would also be useful if the actual commit subject line could
> have a similar format option, so that those who use say the git gui
> (rather than the cli) for the commit message, could easily create the
> `!fixup <commit>` message which would allow a broader range of ways of
> spelling the commit (e.g. giving a sha1(min length) that is within the
> rebase todo list).
It is already the case that `fixup! <sha1>` is accepted, see the code
replaced by above-mentioned commit:
https://github.com/dscho/git/commit/7d0831637f#diff-0f15aff45d5dd346465c35597a5f274eL780
... and its replacement code in C:
https://github.com/dscho/git/commit/7d0831637f#diff-79231f0693f84f3951daeea17065aad9R2800
Note that both preimage and postimage code try to match onelines first,
with the new code changing behavior ever so slightly: it tries to match
the exact oneline first, then a commit SHA-1 of an already-seen `pick`
line, and only then falls back to the (expensive) prefix match.
Ciao,
Dscho
^ permalink raw reply
* Continuous Testing of Git on Windows
From: Johannes Schindelin @ 2017-02-10 12:24 UTC (permalink / raw)
To: git-for-windows; +Cc: git
Hi team,
at the GitMerge conference, I heard a couple of times "I do not bother
reporting bugs in `pu` or `next` on MacOSX and Windows anymore, only Linux
seems to be truly supported" or some variation thereof.
This is a strong indicator that we have some room for improvement here.
Active readers of the Git mailing list will not be surprised to read that
I think we have to react to build/test failures quicker, that it is not
enough to declare it okay for those integration branches to fail the test
suite or even to fail to build[*1*].
In that vein, I worked quite a bit on "Continuous Integration", or more
appropriately: Continuous Testing. That is, I created an ensemble of jobs
that build & test the four integration tests of upstream Git (`maint`,
`master`, `next` and `pu`) to verify the *basic* validity of their
respective current revisions.
Most CI integrations these days require custom configuration files to be
committed, and certain knobs to be twisted on GitHub (which I cannot turn
because I have no special privileges on git/git). After struggling with
making it work *somehow* anyway (even trying to get in touch with Travis,
but they have not bothered to reply to my requests in over a year...), I
decided to go with the Visual Studio Team Services (or short, VSTS; it
does come in handy that it is developed by distant colleagues of mine, so
they *have* to reply to my requests) where the CI configuration can be
separated from the source code.
The entire setup is a little bit more complex than your grandfather's CI
setup: it has to orchestrate five separate Git repositories, two of them
generated and updated from live 32/64-bit Git for Windows SDKs, using a
custom pool of build agents due to high resource demands, using three
separate Git for Windows installations to support 32/64-bit as well as
updating git.exe via `git pull`[*2*].
There is currently only one downside to that setup: the ability to have
publicly accessible build logs on VSTS is still in development.
This is not *such* a big downside: if the MacOSX/Linux CI based on
Travis[*3*] is any indicator, few people, if any, give a flying,
fornicating fly about public build logs.
However, we should strive to improve our software development practices,
and one such well-known Best Practice is to use Continuous Testing more
effectively, i.e. *not* to ignore it.
That is why I taught the Git for Windows CI job that tests the four
upstream Git integration branches to *also* bisect test breakages and then
upload comments to the identified commit on GitHub. See an example here
(the identified breakage seems to have disappeared in the meantime):
https://github.com/git/git/commit/5a12b3d76973#commitcomment-20802488
The code that generates this comment can be seen here:
https://github.com/git-for-windows/build-extra/blob/50c392c7d107/please.sh#L1648-L1665
So here is hoping to a quicker turnaround from breakage to fix in the
future!
Ciao,
Johannes
P.S.: I realize that these commit comments may *still* be ignored, but I
simply was not yet ready to annoy everybody by having automated mails sent
out.
Footnote *1*: It would be kind of okay if, say, `pu` would simply pick up
*all* patches so that they would not be forgotten. But that is not the
case. Even worse: it was stated recently that the expectation is that the
*submitters* of patches find bugs in their code, that the patches should
essentially be bug-free by the time they were submitted. This reasoning
falls flat on its face considering the very real failures, of course,
demonstrating our dear need for Continuous Testing.
Footnote *2*: I will describe the entire setup, including links to the
involved repositories, in a separate mail at a later stage.
Footnote *3*: Look at https://travis-ci.org/git/git/builds, and be happy
if you have a red/green deficiency so you cannot see all that red.
^ permalink raw reply
* [PATCH v2 5/9] refs: store submodule ref stores in a hashmap
From: Michael Haggerty @ 2017-02-10 11:16 UTC (permalink / raw)
To: Junio C Hamano
Cc: Nguyễn Thái Ngọc Duy, Stefan Beller,
Johannes Schindelin, David Turner, Jeff King, git,
Michael Haggerty
In-Reply-To: <cover.1486724698.git.mhagger@alum.mit.edu>
Aside from scaling better, this means that the submodule name needn't be
stored in the ref_store instance anymore (which will be changed in a
moment). This, in turn, will help loosen the strict 1:1 relationship
between ref_stores and submodules.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
refs.c | 58 ++++++++++++++++++++++++++++++++++++++++------------
refs/refs-internal.h | 6 ------
2 files changed, 45 insertions(+), 19 deletions(-)
diff --git a/refs.c b/refs.c
index d7158b6..5121c57 100644
--- a/refs.c
+++ b/refs.c
@@ -3,6 +3,7 @@
*/
#include "cache.h"
+#include "hashmap.h"
#include "lockfile.h"
#include "refs.h"
#include "refs/refs-internal.h"
@@ -1352,11 +1353,41 @@ int resolve_gitlink_ref(const char *submodule, const char *refname,
return 0;
}
+struct submodule_hash_entry
+{
+ struct hashmap_entry ent; /* must be the first member! */
+
+ struct ref_store *refs;
+
+ /* NUL-terminated name of submodule: */
+ char submodule[FLEX_ARRAY];
+};
+
+static int submodule_hash_cmp(const void *entry, const void *entry_or_key,
+ const void *keydata)
+{
+ const struct submodule_hash_entry *e1 = entry, *e2 = entry_or_key;
+ const char *submodule = keydata ? keydata : e2->submodule;
+
+ return strcmp(e1->submodule, submodule);
+}
+
+static struct submodule_hash_entry *alloc_submodule_hash_entry(
+ const char *submodule, struct ref_store *refs)
+{
+ struct submodule_hash_entry *entry;
+
+ FLEX_ALLOC_STR(entry, submodule, submodule);
+ hashmap_entry_init(entry, strhash(submodule));
+ entry->refs = refs;
+ return entry;
+}
+
/* A pointer to the ref_store for the main repository: */
static struct ref_store *main_ref_store;
-/* A linked list of ref_stores for submodules: */
-static struct ref_store *submodule_ref_stores;
+/* A hashmap of ref_stores, stored by submodule name: */
+static struct hashmap submodule_ref_stores;
/*
* Return the ref_store instance for the specified submodule (or the
@@ -1365,17 +1396,18 @@ static struct ref_store *submodule_ref_stores;
*/
static struct ref_store *lookup_ref_store(const char *submodule)
{
- struct ref_store *refs;
+ struct submodule_hash_entry *entry;
if (!submodule)
return main_ref_store;
- for (refs = submodule_ref_stores; refs; refs = refs->next) {
- if (!strcmp(submodule, refs->submodule))
- return refs;
- }
+ if (!submodule_ref_stores.tablesize)
+ /* It's initialized on demand in register_ref_store(). */
+ return NULL;
- return NULL;
+ entry = hashmap_get_from_hash(&submodule_ref_stores,
+ strhash(submodule), submodule);
+ return entry ? entry->refs : NULL;
}
/*
@@ -1389,15 +1421,15 @@ static void register_ref_store(struct ref_store *refs, const char *submodule)
if (main_ref_store)
die("BUG: main_ref_store initialized twice");
- refs->next = NULL;
main_ref_store = refs;
} else {
- if (lookup_ref_store(submodule))
+ if (!submodule_ref_stores.tablesize)
+ hashmap_init(&submodule_ref_stores, submodule_hash_cmp, 0);
+
+ if (hashmap_put(&submodule_ref_stores,
+ alloc_submodule_hash_entry(submodule, refs)))
die("BUG: ref_store for submodule '%s' initialized twice",
submodule);
-
- refs->next = submodule_ref_stores;
- submodule_ref_stores = refs;
}
}
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index d8a7eb1..07fd208 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -636,12 +636,6 @@ struct ref_store {
* reference store:
*/
const char *submodule;
-
- /*
- * Submodule reference store instances are stored in a linked
- * list using this pointer.
- */
- struct ref_store *next;
};
/*
--
2.9.3
^ permalink raw reply related
* [PATCH v2 1/9] refs: reorder some function definitions
From: Michael Haggerty @ 2017-02-10 11:16 UTC (permalink / raw)
To: Junio C Hamano
Cc: Nguyễn Thái Ngọc Duy, Stefan Beller,
Johannes Schindelin, David Turner, Jeff King, git,
Michael Haggerty
In-Reply-To: <cover.1486724698.git.mhagger@alum.mit.edu>
This avoids the need to add forward declarations in the next step.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
refs.c | 64 ++++++++++++++++++++++++++++++++--------------------------------
1 file changed, 32 insertions(+), 32 deletions(-)
diff --git a/refs.c b/refs.c
index 9bd0bc1..707092f 100644
--- a/refs.c
+++ b/refs.c
@@ -1358,27 +1358,19 @@ static struct ref_store *main_ref_store;
/* A linked list of ref_stores for submodules: */
static struct ref_store *submodule_ref_stores;
-void base_ref_store_init(struct ref_store *refs,
- const struct ref_storage_be *be,
- const char *submodule)
+struct ref_store *lookup_ref_store(const char *submodule)
{
- refs->be = be;
- if (!submodule) {
- if (main_ref_store)
- die("BUG: main_ref_store initialized twice");
+ struct ref_store *refs;
- refs->submodule = "";
- refs->next = NULL;
- main_ref_store = refs;
- } else {
- if (lookup_ref_store(submodule))
- die("BUG: ref_store for submodule '%s' initialized twice",
- submodule);
+ if (!submodule || !*submodule)
+ return main_ref_store;
- refs->submodule = xstrdup(submodule);
- refs->next = submodule_ref_stores;
- submodule_ref_stores = refs;
+ for (refs = submodule_ref_stores; refs; refs = refs->next) {
+ if (!strcmp(submodule, refs->submodule))
+ return refs;
}
+
+ return NULL;
}
struct ref_store *ref_store_init(const char *submodule)
@@ -1395,21 +1387,6 @@ struct ref_store *ref_store_init(const char *submodule)
return be->init(submodule);
}
-struct ref_store *lookup_ref_store(const char *submodule)
-{
- struct ref_store *refs;
-
- if (!submodule || !*submodule)
- return main_ref_store;
-
- for (refs = submodule_ref_stores; refs; refs = refs->next) {
- if (!strcmp(submodule, refs->submodule))
- return refs;
- }
-
- return NULL;
-}
-
struct ref_store *get_ref_store(const char *submodule)
{
struct ref_store *refs;
@@ -1435,6 +1412,29 @@ struct ref_store *get_ref_store(const char *submodule)
return refs;
}
+void base_ref_store_init(struct ref_store *refs,
+ const struct ref_storage_be *be,
+ const char *submodule)
+{
+ refs->be = be;
+ if (!submodule) {
+ if (main_ref_store)
+ die("BUG: main_ref_store initialized twice");
+
+ refs->submodule = "";
+ refs->next = NULL;
+ main_ref_store = refs;
+ } else {
+ if (lookup_ref_store(submodule))
+ die("BUG: ref_store for submodule '%s' initialized twice",
+ submodule);
+
+ refs->submodule = xstrdup(submodule);
+ refs->next = submodule_ref_stores;
+ submodule_ref_stores = refs;
+ }
+}
+
void assert_main_repository(struct ref_store *refs, const char *caller)
{
if (*refs->submodule)
--
2.9.3
^ permalink raw reply related
* [PATCH v2 3/9] refs: remove some unnecessary handling of submodule == ""
From: Michael Haggerty @ 2017-02-10 11:16 UTC (permalink / raw)
To: Junio C Hamano
Cc: Nguyễn Thái Ngọc Duy, Stefan Beller,
Johannes Schindelin, David Turner, Jeff King, git,
Michael Haggerty
In-Reply-To: <cover.1486724698.git.mhagger@alum.mit.edu>
The only external entry point to the ref_store lookup functions is
get_ref_store(), which ensures that submodule == "" is passed along as
NULL. So ref_store_init() and lookup_ref_store() don't have to handle
submodule being specified as the empty string.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
refs.c | 13 ++-----------
1 file changed, 2 insertions(+), 11 deletions(-)
diff --git a/refs.c b/refs.c
index d7265cc..6348414 100644
--- a/refs.c
+++ b/refs.c
@@ -1362,15 +1362,12 @@ static struct ref_store *submodule_ref_stores;
* Return the ref_store instance for the specified submodule (or the
* main repository if submodule is NULL). If that ref_store hasn't
* been initialized yet, return NULL.
- *
- * For backwards compatibility, submodule=="" is treated the same as
- * submodule==NULL.
*/
static struct ref_store *lookup_ref_store(const char *submodule)
{
struct ref_store *refs;
- if (!submodule || !*submodule)
+ if (!submodule)
return main_ref_store;
for (refs = submodule_ref_stores; refs; refs = refs->next) {
@@ -1384,9 +1381,6 @@ static struct ref_store *lookup_ref_store(const char *submodule)
/*
* Create, record, and return a ref_store instance for the specified
* submodule (or the main repository if submodule is NULL).
- *
- * For backwards compatibility, submodule=="" is treated the same as
- * submodule==NULL.
*/
static struct ref_store *ref_store_init(const char *submodule)
{
@@ -1396,10 +1390,7 @@ static struct ref_store *ref_store_init(const char *submodule)
if (!be)
die("BUG: reference backend %s is unknown", be_name);
- if (!submodule || !*submodule)
- return be->init(NULL);
- else
- return be->init(submodule);
+ return be->init(submodule);
}
struct ref_store *get_ref_store(const char *submodule)
--
2.9.3
^ permalink raw reply related
* [PATCH v2 8/9] files_ref_store::submodule: use NULL for the main repository
From: Michael Haggerty @ 2017-02-10 11:16 UTC (permalink / raw)
To: Junio C Hamano
Cc: Nguyễn Thái Ngọc Duy, Stefan Beller,
Johannes Schindelin, David Turner, Jeff King, git,
Michael Haggerty
In-Reply-To: <cover.1486724698.git.mhagger@alum.mit.edu>
The old practice of storing the empty string in this member for the main
repository was a holdover from before 00eebe3 (refs: create a base class
"ref_store" for files_ref_store, 2016-09-04), when the submodule was
stored in a flex array at the end of `struct files_ref_store`. Storing
NULL for this case is more idiomatic and a tiny bit less code.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
refs/files-backend.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index c9d2d28..4fe92f0 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -915,8 +915,8 @@ struct files_ref_store {
/*
* The name of the submodule represented by this object, or
- * the empty string if it represents the main repository's
- * reference store:
+ * NULL if it represents the main repository's reference
+ * store:
*/
const char *submodule;
@@ -982,7 +982,7 @@ static struct ref_store *files_ref_store_create(const char *submodule)
base_ref_store_init(ref_store, &refs_be_files);
- refs->submodule = submodule ? xstrdup(submodule) : "";
+ refs->submodule = xstrdup_or_null(submodule);
return ref_store;
}
@@ -994,7 +994,7 @@ static struct ref_store *files_ref_store_create(const char *submodule)
static void files_assert_main_repository(struct files_ref_store *refs,
const char *caller)
{
- if (*refs->submodule)
+ if (refs->submodule)
die("BUG: %s called for a submodule", caller);
}
@@ -1158,7 +1158,7 @@ static struct packed_ref_cache *get_packed_ref_cache(struct files_ref_store *ref
{
char *packed_refs_file;
- if (*refs->submodule)
+ if (refs->submodule)
packed_refs_file = git_pathdup_submodule(refs->submodule,
"packed-refs");
else
@@ -1228,7 +1228,7 @@ static void read_loose_refs(const char *dirname, struct ref_dir *dir)
size_t path_baselen;
int err = 0;
- if (*refs->submodule)
+ if (refs->submodule)
err = strbuf_git_path_submodule(&path, refs->submodule, "%s", dirname);
else
strbuf_git_path(&path, "%s", dirname);
@@ -1269,7 +1269,7 @@ static void read_loose_refs(const char *dirname, struct ref_dir *dir)
} else {
int read_ok;
- if (*refs->submodule) {
+ if (refs->submodule) {
hashclr(sha1);
flag = 0;
read_ok = !resolve_gitlink_ref(refs->submodule,
@@ -1383,7 +1383,7 @@ static int files_read_raw_ref(struct ref_store *ref_store,
*type = 0;
strbuf_reset(&sb_path);
- if (*refs->submodule)
+ if (refs->submodule)
strbuf_git_path_submodule(&sb_path, refs->submodule, "%s", refname);
else
strbuf_git_path(&sb_path, "%s", refname);
--
2.9.3
^ permalink raw reply related
* [PATCH v2 4/9] register_ref_store(): new function
From: Michael Haggerty @ 2017-02-10 11:16 UTC (permalink / raw)
To: Junio C Hamano
Cc: Nguyễn Thái Ngọc Duy, Stefan Beller,
Johannes Schindelin, David Turner, Jeff King, git,
Michael Haggerty
In-Reply-To: <cover.1486724698.git.mhagger@alum.mit.edu>
Move the responsibility for registering the ref_store for a submodule
from base_ref_store_init() to a new function, register_ref_store(). Call
the latter from ref_store_init().
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
refs.c | 43 +++++++++++++++++++++++++++++--------------
1 file changed, 29 insertions(+), 14 deletions(-)
diff --git a/refs.c b/refs.c
index 6348414..d7158b6 100644
--- a/refs.c
+++ b/refs.c
@@ -1379,6 +1379,29 @@ static struct ref_store *lookup_ref_store(const char *submodule)
}
/*
+ * Register the specified ref_store to be the one that should be used
+ * for submodule (or the main repository if submodule is NULL). It is
+ * a fatal error to call this function twice for the same submodule.
+ */
+static void register_ref_store(struct ref_store *refs, const char *submodule)
+{
+ if (!submodule) {
+ if (main_ref_store)
+ die("BUG: main_ref_store initialized twice");
+
+ refs->next = NULL;
+ main_ref_store = refs;
+ } else {
+ if (lookup_ref_store(submodule))
+ die("BUG: ref_store for submodule '%s' initialized twice",
+ submodule);
+
+ refs->next = submodule_ref_stores;
+ submodule_ref_stores = refs;
+ }
+}
+
+/*
* Create, record, and return a ref_store instance for the specified
* submodule (or the main repository if submodule is NULL).
*/
@@ -1386,11 +1409,14 @@ static struct ref_store *ref_store_init(const char *submodule)
{
const char *be_name = "files";
struct ref_storage_be *be = find_ref_storage_backend(be_name);
+ struct ref_store *refs;
if (!be)
die("BUG: reference backend %s is unknown", be_name);
- return be->init(submodule);
+ refs = be->init(submodule);
+ register_ref_store(refs, submodule);
+ return refs;
}
struct ref_store *get_ref_store(const char *submodule)
@@ -1423,22 +1449,11 @@ void base_ref_store_init(struct ref_store *refs,
const char *submodule)
{
refs->be = be;
- if (!submodule) {
- if (main_ref_store)
- die("BUG: main_ref_store initialized twice");
+ if (!submodule)
refs->submodule = "";
- refs->next = NULL;
- main_ref_store = refs;
- } else {
- if (lookup_ref_store(submodule))
- die("BUG: ref_store for submodule '%s' initialized twice",
- submodule);
-
+ else
refs->submodule = xstrdup(submodule);
- refs->next = submodule_ref_stores;
- submodule_ref_stores = refs;
- }
}
void assert_main_repository(struct ref_store *refs, const char *caller)
--
2.9.3
^ permalink raw reply related
* [PATCH v2 6/9] refs: push the submodule attribute down
From: Michael Haggerty @ 2017-02-10 11:16 UTC (permalink / raw)
To: Junio C Hamano
Cc: Nguyễn Thái Ngọc Duy, Stefan Beller,
Johannes Schindelin, David Turner, Jeff King, git,
Michael Haggerty
In-Reply-To: <cover.1486724698.git.mhagger@alum.mit.edu>
Push the submodule attribute down from ref_store to files_ref_store.
This is another step towards loosening the 1:1 connection between
ref_stores and submodules.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
refs.c | 11 ----------
refs/files-backend.c | 61 ++++++++++++++++++++++++++++++++++++----------------
refs/refs-internal.h | 13 -----------
3 files changed, 43 insertions(+), 42 deletions(-)
diff --git a/refs.c b/refs.c
index 5121c57..07959ff 100644
--- a/refs.c
+++ b/refs.c
@@ -1481,17 +1481,6 @@ void base_ref_store_init(struct ref_store *refs,
const char *submodule)
{
refs->be = be;
-
- if (!submodule)
- refs->submodule = "";
- else
- refs->submodule = xstrdup(submodule);
-}
-
-void assert_main_repository(struct ref_store *refs, const char *caller)
-{
- if (*refs->submodule)
- die("BUG: %s called for a submodule", caller);
}
/* backend functions */
diff --git a/refs/files-backend.c b/refs/files-backend.c
index f902393..5e135a4 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -912,6 +912,14 @@ struct packed_ref_cache {
*/
struct files_ref_store {
struct ref_store base;
+
+ /*
+ * The name of the submodule represented by this object, or
+ * the empty string if it represents the main repository's
+ * reference store:
+ */
+ const char *submodule;
+
struct ref_entry *loose;
struct packed_ref_cache *packed;
};
@@ -974,10 +982,23 @@ static struct ref_store *files_ref_store_create(const char *submodule)
base_ref_store_init(ref_store, &refs_be_files, submodule);
+ refs->submodule = submodule ? xstrdup(submodule) : "";
+
return ref_store;
}
/*
+ * Die if refs is for a submodule (i.e., not for the main repository).
+ * caller is used in any necessary error messages.
+ */
+static void files_assert_main_repository(struct files_ref_store *refs,
+ const char *caller)
+{
+ if (*refs->submodule)
+ die("BUG: %s called for a submodule", caller);
+}
+
+/*
* Downcast ref_store to files_ref_store. Die if ref_store is not a
* files_ref_store. If submodule_allowed is not true, then also die if
* files_ref_store is for a submodule (i.e., not for the main
@@ -987,14 +1008,18 @@ static struct files_ref_store *files_downcast(
struct ref_store *ref_store, int submodule_allowed,
const char *caller)
{
+ struct files_ref_store *refs;
+
if (ref_store->be != &refs_be_files)
die("BUG: ref_store is type \"%s\" not \"files\" in %s",
ref_store->be->name, caller);
+ refs = (struct files_ref_store *)ref_store;
+
if (!submodule_allowed)
- assert_main_repository(ref_store, caller);
+ files_assert_main_repository(refs, caller);
- return (struct files_ref_store *)ref_store;
+ return refs;
}
/* The length of a peeled reference line in packed-refs, including EOL: */
@@ -1133,8 +1158,8 @@ static struct packed_ref_cache *get_packed_ref_cache(struct files_ref_store *ref
{
char *packed_refs_file;
- if (*refs->base.submodule)
- packed_refs_file = git_pathdup_submodule(refs->base.submodule,
+ if (*refs->submodule)
+ packed_refs_file = git_pathdup_submodule(refs->submodule,
"packed-refs");
else
packed_refs_file = git_pathdup("packed-refs");
@@ -1203,8 +1228,8 @@ static void read_loose_refs(const char *dirname, struct ref_dir *dir)
size_t path_baselen;
int err = 0;
- if (*refs->base.submodule)
- err = strbuf_git_path_submodule(&path, refs->base.submodule, "%s", dirname);
+ if (*refs->submodule)
+ err = strbuf_git_path_submodule(&path, refs->submodule, "%s", dirname);
else
strbuf_git_path(&path, "%s", dirname);
path_baselen = path.len;
@@ -1244,10 +1269,10 @@ static void read_loose_refs(const char *dirname, struct ref_dir *dir)
} else {
int read_ok;
- if (*refs->base.submodule) {
+ if (*refs->submodule) {
hashclr(sha1);
flag = 0;
- read_ok = !resolve_gitlink_ref(refs->base.submodule,
+ read_ok = !resolve_gitlink_ref(refs->submodule,
refname.buf, sha1);
} else {
read_ok = !read_ref_full(refname.buf,
@@ -1358,8 +1383,8 @@ static int files_read_raw_ref(struct ref_store *ref_store,
*type = 0;
strbuf_reset(&sb_path);
- if (*refs->base.submodule)
- strbuf_git_path_submodule(&sb_path, refs->base.submodule, "%s", refname);
+ if (*refs->submodule)
+ strbuf_git_path_submodule(&sb_path, refs->submodule, "%s", refname);
else
strbuf_git_path(&sb_path, "%s", refname);
@@ -1540,7 +1565,7 @@ static int lock_raw_ref(struct files_ref_store *refs,
int ret = TRANSACTION_GENERIC_ERROR;
assert(err);
- assert_main_repository(&refs->base, "lock_raw_ref");
+ files_assert_main_repository(refs, "lock_raw_ref");
*type = 0;
@@ -2006,7 +2031,7 @@ static struct ref_lock *lock_ref_sha1_basic(struct files_ref_store *refs,
int attempts_remaining = 3;
int resolved;
- assert_main_repository(&refs->base, "lock_ref_sha1_basic");
+ files_assert_main_repository(refs, "lock_ref_sha1_basic");
assert(err);
lock = xcalloc(1, sizeof(struct ref_lock));
@@ -2152,7 +2177,7 @@ static int lock_packed_refs(struct files_ref_store *refs, int flags)
static int timeout_value = 1000;
struct packed_ref_cache *packed_ref_cache;
- assert_main_repository(&refs->base, "lock_packed_refs");
+ files_assert_main_repository(refs, "lock_packed_refs");
if (!timeout_configured) {
git_config_get_int("core.packedrefstimeout", &timeout_value);
@@ -2190,7 +2215,7 @@ static int commit_packed_refs(struct files_ref_store *refs)
int save_errno = 0;
FILE *out;
- assert_main_repository(&refs->base, "commit_packed_refs");
+ files_assert_main_repository(refs, "commit_packed_refs");
if (!packed_ref_cache->lock)
die("internal error: packed-refs not locked");
@@ -2223,7 +2248,7 @@ static void rollback_packed_refs(struct files_ref_store *refs)
struct packed_ref_cache *packed_ref_cache =
get_packed_ref_cache(refs);
- assert_main_repository(&refs->base, "rollback_packed_refs");
+ files_assert_main_repository(refs, "rollback_packed_refs");
if (!packed_ref_cache->lock)
die("internal error: packed-refs not locked");
@@ -2397,7 +2422,7 @@ static int repack_without_refs(struct files_ref_store *refs,
struct string_list_item *refname;
int ret, needs_repacking = 0, removed = 0;
- assert_main_repository(&refs->base, "repack_without_refs");
+ files_assert_main_repository(refs, "repack_without_refs");
assert(err);
/* Look for a packed ref */
@@ -2930,7 +2955,7 @@ static int commit_ref_update(struct files_ref_store *refs,
const unsigned char *sha1, const char *logmsg,
struct strbuf *err)
{
- assert_main_repository(&refs->base, "commit_ref_update");
+ files_assert_main_repository(refs, "commit_ref_update");
clear_loose_ref_cache(refs);
if (log_ref_write(lock->ref_name, lock->old_oid.hash, sha1, logmsg, 0, err)) {
@@ -3560,7 +3585,7 @@ static int lock_ref_for_update(struct files_ref_store *refs,
int ret;
struct ref_lock *lock;
- assert_main_repository(&refs->base, "lock_ref_for_update");
+ files_assert_main_repository(refs, "lock_ref_for_update");
if ((update->flags & REF_HAVE_NEW) && is_null_sha1(update->new_sha1))
update->flags |= REF_DELETING;
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 07fd208..008822d 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -629,13 +629,6 @@ extern struct ref_storage_be refs_be_files;
struct ref_store {
/* The backend describing this ref_store's storage scheme: */
const struct ref_storage_be *be;
-
- /*
- * The name of the submodule represented by this object, or
- * the empty string if it represents the main repository's
- * reference store:
- */
- const char *submodule;
};
/*
@@ -658,10 +651,4 @@ void base_ref_store_init(struct ref_store *refs,
*/
struct ref_store *get_ref_store(const char *submodule);
-/*
- * Die if refs is for a submodule (i.e., not for the main repository).
- * caller is used in any necessary error messages.
- */
-void assert_main_repository(struct ref_store *refs, const char *caller);
-
#endif /* REFS_REFS_INTERNAL_H */
--
2.9.3
^ permalink raw reply related
* [PATCH v2 9/9] read_loose_refs(): read refs using resolve_ref_recursively()
From: Michael Haggerty @ 2017-02-10 11:16 UTC (permalink / raw)
To: Junio C Hamano
Cc: Nguyễn Thái Ngọc Duy, Stefan Beller,
Johannes Schindelin, David Turner, Jeff King, git,
Michael Haggerty
In-Reply-To: <cover.1486724698.git.mhagger@alum.mit.edu>
There is no need to call read_ref_full() or resolve_gitlink_ref() from
read_loose_refs(), because we already have a ref_store object in hand.
So we can call resolve_ref_recursively() ourselves. Happily, this
unifies the code for the submodule vs. non-submodule cases.
This requires resolve_ref_recursively() to be exposed to the refs
subsystem, though not to non-refs code.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
refs.c | 50 +++++++++++++++++++++++++-------------------------
refs/files-backend.c | 18 ++++--------------
refs/refs-internal.h | 5 +++++
3 files changed, 34 insertions(+), 39 deletions(-)
diff --git a/refs.c b/refs.c
index 05af56b..f03dcf5 100644
--- a/refs.c
+++ b/refs.c
@@ -1230,10 +1230,10 @@ int for_each_rawref(each_ref_fn fn, void *cb_data)
}
/* This function needs to return a meaningful errno on failure */
-static const char *resolve_ref_recursively(struct ref_store *refs,
- const char *refname,
- int resolve_flags,
- unsigned char *sha1, int *flags)
+const char *resolve_ref_recursively(struct ref_store *refs,
+ const char *refname,
+ int resolve_flags,
+ unsigned char *sha1, int *flags)
{
static struct strbuf sb_refname = STRBUF_INIT;
int unused_flags;
@@ -1390,27 +1390,6 @@ static struct ref_store *main_ref_store;
static struct hashmap submodule_ref_stores;
/*
- * Return the ref_store instance for the specified submodule (or the
- * main repository if submodule is NULL). If that ref_store hasn't
- * been initialized yet, return NULL.
- */
-static struct ref_store *lookup_ref_store(const char *submodule)
-{
- struct submodule_hash_entry *entry;
-
- if (!submodule)
- return main_ref_store;
-
- if (!submodule_ref_stores.tablesize)
- /* It's initialized on demand in register_ref_store(). */
- return NULL;
-
- entry = hashmap_get_from_hash(&submodule_ref_stores,
- strhash(submodule), submodule);
- return entry ? entry->refs : NULL;
-}
-
-/*
* Register the specified ref_store to be the one that should be used
* for submodule (or the main repository if submodule is NULL). It is
* a fatal error to call this function twice for the same submodule.
@@ -1451,6 +1430,27 @@ static struct ref_store *ref_store_init(const char *submodule)
return refs;
}
+/*
+ * Return the ref_store instance for the specified submodule (or the
+ * main repository if submodule is NULL). If that ref_store hasn't
+ * been initialized yet, return NULL.
+ */
+static struct ref_store *lookup_ref_store(const char *submodule)
+{
+ struct submodule_hash_entry *entry;
+
+ if (!submodule)
+ return main_ref_store;
+
+ if (!submodule_ref_stores.tablesize)
+ /* It's initialized on demand in register_ref_store(). */
+ return NULL;
+
+ entry = hashmap_get_from_hash(&submodule_ref_stores,
+ strhash(submodule), submodule);
+ return entry ? entry->refs : NULL;
+}
+
struct ref_store *get_ref_store(const char *submodule)
{
struct ref_store *refs;
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 4fe92f0..cdb6b8f 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1267,20 +1267,10 @@ static void read_loose_refs(const char *dirname, struct ref_dir *dir)
create_dir_entry(refs, refname.buf,
refname.len, 1));
} else {
- int read_ok;
-
- if (refs->submodule) {
- hashclr(sha1);
- flag = 0;
- read_ok = !resolve_gitlink_ref(refs->submodule,
- refname.buf, sha1);
- } else {
- read_ok = !read_ref_full(refname.buf,
- RESOLVE_REF_READING,
- sha1, &flag);
- }
-
- if (!read_ok) {
+ if (!resolve_ref_recursively(&refs->base,
+ refname.buf,
+ RESOLVE_REF_READING,
+ sha1, &flag)) {
hashclr(sha1);
flag |= REF_ISBROKEN;
} else if (is_null_sha1(sha1)) {
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 793c850..33adbf9 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -650,4 +650,9 @@ void base_ref_store_init(struct ref_store *refs,
*/
struct ref_store *get_ref_store(const char *submodule);
+const char *resolve_ref_recursively(struct ref_store *refs,
+ const char *refname,
+ int resolve_flags,
+ unsigned char *sha1, int *flags);
+
#endif /* REFS_REFS_INTERNAL_H */
--
2.9.3
^ permalink raw reply related
* [PATCH v2 7/9] base_ref_store_init(): remove submodule argument
From: Michael Haggerty @ 2017-02-10 11:16 UTC (permalink / raw)
To: Junio C Hamano
Cc: Nguyễn Thái Ngọc Duy, Stefan Beller,
Johannes Schindelin, David Turner, Jeff King, git,
Michael Haggerty
In-Reply-To: <cover.1486724698.git.mhagger@alum.mit.edu>
This is another step towards weakening the 1:1 relationship between
ref_stores and submodules.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
refs.c | 3 +--
refs/files-backend.c | 2 +-
refs/refs-internal.h | 7 +++----
3 files changed, 5 insertions(+), 7 deletions(-)
diff --git a/refs.c b/refs.c
index 07959ff..05af56b 100644
--- a/refs.c
+++ b/refs.c
@@ -1477,8 +1477,7 @@ struct ref_store *get_ref_store(const char *submodule)
}
void base_ref_store_init(struct ref_store *refs,
- const struct ref_storage_be *be,
- const char *submodule)
+ const struct ref_storage_be *be)
{
refs->be = be;
}
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 5e135a4..c9d2d28 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -980,7 +980,7 @@ static struct ref_store *files_ref_store_create(const char *submodule)
struct files_ref_store *refs = xcalloc(1, sizeof(*refs));
struct ref_store *ref_store = (struct ref_store *)refs;
- base_ref_store_init(ref_store, &refs_be_files, submodule);
+ base_ref_store_init(ref_store, &refs_be_files);
refs->submodule = submodule ? xstrdup(submodule) : "";
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 008822d..793c850 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -632,12 +632,11 @@ struct ref_store {
};
/*
- * Fill in the generic part of refs for the specified submodule and
- * add it to our collection of reference stores.
+ * Fill in the generic part of refs and add it to our collection of
+ * reference stores.
*/
void base_ref_store_init(struct ref_store *refs,
- const struct ref_storage_be *be,
- const char *submodule);
+ const struct ref_storage_be *be);
/*
* Return the ref_store instance for the specified submodule. For the
--
2.9.3
^ permalink raw reply related
* [PATCH v2 0/9] Store submodules in a hash, not a linked list
From: Michael Haggerty @ 2017-02-10 11:16 UTC (permalink / raw)
To: Junio C Hamano
Cc: Nguyễn Thái Ngọc Duy, Stefan Beller,
Johannes Schindelin, David Turner, Jeff King, git,
Michael Haggerty
This is v2 of the patch series, considerably reorganized but not that
different codewise. Thanks to Stefan, Junio, and Peff for their
feedback about v1 [1]. I think I have addressed all of your comments.
Changes since v1:
* Rebase from `master` onto `maint` (to match Junio).
* Reorder some commits to make the presentation more logical.
* Added an explicit preparatory commit that just reorders some
function definitions, because it makes the diff for the subsequent
commit a lot easier to read.
* Make some preexisting functions private:
* lookup_ref_store()
* ref_store_init()
* Remove some unnecessary handling of `submodule == ""` when it is
already known to have been converted to `NULL`. (Some of the
purported handling also happened to be broken.)
* Introduce function `register_ref_store()` in a separate step, before
switching to hashmaps.
* Don't initialize hashmap in `lookup_ref_store()`. (Just return
`NULL`; the hashmap will be initialized in `register_ref_store()` a
moment later.)
* Make code in `submodule_hash_cmp()` clearer.
* Use `FLEX_ALLOC_STR()` in `alloc_submodule_hash_entry()`.
* Don't specify an initial size for the submodule hashmap (the default
is OK).
This patch series is also available from my fork on GitHub [2] as
branch "submodule-hash".
Michael
[1] http://public-inbox.org/git/cover.1486629195.git.mhagger@alum.mit.edu/T/#u
[2] https://github.com/mhagger/git
Michael Haggerty (9):
refs: reorder some function definitions
refs: make some ref_store lookup functions private
refs: remove some unnecessary handling of submodule == ""
register_ref_store(): new function
refs: store submodule ref stores in a hashmap
refs: push the submodule attribute down
base_ref_store_init(): remove submodule argument
files_ref_store::submodule: use NULL for the main repository
read_loose_refs(): read refs using resolve_ref_recursively()
refs.c | 107 +++++++++++++++++++++++++++++++++++----------------
refs/files-backend.c | 77 +++++++++++++++++++++---------------
refs/refs-internal.h | 48 ++++-------------------
3 files changed, 127 insertions(+), 105 deletions(-)
--
2.9.3
^ permalink raw reply
* [PATCH v2 2/9] refs: make some ref_store lookup functions private
From: Michael Haggerty @ 2017-02-10 11:16 UTC (permalink / raw)
To: Junio C Hamano
Cc: Nguyễn Thái Ngọc Duy, Stefan Beller,
Johannes Schindelin, David Turner, Jeff King, git,
Michael Haggerty
In-Reply-To: <cover.1486724698.git.mhagger@alum.mit.edu>
The following functions currently don't need to be exposed:
* ref_store_init()
* lookup_ref_store()
That might change in the future, but for now make them private.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
refs.c | 19 +++++++++++++++++--
refs/refs-internal.h | 19 -------------------
2 files changed, 17 insertions(+), 21 deletions(-)
diff --git a/refs.c b/refs.c
index 707092f..d7265cc 100644
--- a/refs.c
+++ b/refs.c
@@ -1358,7 +1358,15 @@ static struct ref_store *main_ref_store;
/* A linked list of ref_stores for submodules: */
static struct ref_store *submodule_ref_stores;
-struct ref_store *lookup_ref_store(const char *submodule)
+/*
+ * Return the ref_store instance for the specified submodule (or the
+ * main repository if submodule is NULL). If that ref_store hasn't
+ * been initialized yet, return NULL.
+ *
+ * For backwards compatibility, submodule=="" is treated the same as
+ * submodule==NULL.
+ */
+static struct ref_store *lookup_ref_store(const char *submodule)
{
struct ref_store *refs;
@@ -1373,7 +1381,14 @@ struct ref_store *lookup_ref_store(const char *submodule)
return NULL;
}
-struct ref_store *ref_store_init(const char *submodule)
+/*
+ * Create, record, and return a ref_store instance for the specified
+ * submodule (or the main repository if submodule is NULL).
+ *
+ * For backwards compatibility, submodule=="" is treated the same as
+ * submodule==NULL.
+ */
+static struct ref_store *ref_store_init(const char *submodule)
{
const char *be_name = "files";
struct ref_storage_be *be = find_ref_storage_backend(be_name);
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 708b260..d8a7eb1 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -653,25 +653,6 @@ void base_ref_store_init(struct ref_store *refs,
const char *submodule);
/*
- * Create, record, and return a ref_store instance for the specified
- * submodule (or the main repository if submodule is NULL).
- *
- * For backwards compatibility, submodule=="" is treated the same as
- * submodule==NULL.
- */
-struct ref_store *ref_store_init(const char *submodule);
-
-/*
- * Return the ref_store instance for the specified submodule (or the
- * main repository if submodule is NULL). If that ref_store hasn't
- * been initialized yet, return NULL.
- *
- * For backwards compatibility, submodule=="" is treated the same as
- * submodule==NULL.
- */
-struct ref_store *lookup_ref_store(const char *submodule);
-
-/*
* Return the ref_store instance for the specified submodule. For the
* main repository, use submodule==NULL; such a call cannot fail. For
* a submodule, the submodule must exist and be a nonbare repository,
--
2.9.3
^ permalink raw reply related
* Re: [PATCH 0/5] Store submodules in a hash, not a linked list
From: Michael Haggerty @ 2017-02-10 10:27 UTC (permalink / raw)
To: Jeff King
Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy,
Stefan Beller, Johannes Schindelin, David Turner, git
In-Reply-To: <20170210004033.cgqmovhvoylad5cf@sigill.intra.peff.net>
On 02/10/2017 01:40 AM, Jeff King wrote:
> On Thu, Feb 09, 2017 at 10:23:35PM +0100, Michael Haggerty wrote:
>
>>>> So push the submodule attribute down to the `files_ref_store` class
>>>> (but continue to let the `ref_store`s be looked up by submodule).
>>>
>>> I'm not sure I understand all of the ramifications here. It _sounds_ like
>>> pushing this down into the files-backend code would make it harder to
>>> have mixed ref-backends for different submodules. Or is this just
>>> pushing down an implementation detail of the files backend, and future
>>> code is free to have as many different ref_stores as it likes?
>>
>> I don't understand how this would make it harder, aside from the fact
>> that a new backend class might also need a path member and have to
>> maintain its own copy rather than using one that the base class provides.
>
> Probably the answer is "I'm really confused".
>
> But here's how my line of reasoning went:
>
> Right now we have a main ref-store that points to the submodule
> ref-stores. I don't know the current state of it, but in theory those
> could all use different backends.
>
> This seems like it's pushing that submodule linkage down into the
> backend.
>
> But I think from your response that the answer is no, the thing that is
> being pushed down is not the right way for the main ref store and the
> submodules to be linked. In fact, there is no reason at all for the
> main ref store to know or care about submodules. Anybody who wants to
> know about a submodule's refs should ask the hashmap.
That's correct; the main ref store and submodule ref stores know nothing
of each other.
Michael
^ permalink raw reply
* Re: [PATCH 1/5] refs: store submodule ref stores in a hashmap
From: Michael Haggerty @ 2017-02-10 10:23 UTC (permalink / raw)
To: Junio C Hamano
Cc: Nguyễn Thái Ngọc Duy, Stefan Beller,
Johannes Schindelin, David Turner, git, Jeff King
In-Reply-To: <xmqqh943p0hv.fsf@gitster.mtv.corp.google.com>
On 02/09/2017 09:34 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
>> [...]
>> +static int submodule_hash_cmp(const void *entry, const void *entry_or_key,
>> + const void *keydata)
>> +{
>> + const struct submodule_hash_entry *e1 = entry, *e2 = entry_or_key;
>> + const char *submodule = keydata;
>> +
>> + return strcmp(e1->submodule, submodule ? submodule : e2->submodule);
>
> I would have found it more readable if it were like so:
>
> const char *submodule = keydata ? keydata : e2->submodule;
>
> return strcmp(e1->submodule, submodule);
>
> but I suspect the difference is not that huge.
Yes, that's better. I'll change it.
On 02/10/2017 05:04 AM, Jeff King wrote:
> On Thu, Feb 09, 2017 at 12:34:04PM -0800, Junio C Hamano wrote:
>
>>> +static struct submodule_hash_entry *alloc_submodule_hash_entry(
>>> + const char *submodule, struct ref_store *refs)
>>> +{
>>> + size_t len = strlen(submodule);
>>> + struct submodule_hash_entry *entry = malloc(sizeof(*entry) + len + 1);
>>
>> I think this (and the later memcpy) is what FLEX_ALLOC_MEM() was
>> invented for.
>
> Yes, it was. Though since the length comes from a strlen() call, it can
> actually use the _STR variant, like:
>
> FLEX_ALLOC_STR(entry, submodule, submodule);
>
> Besides being shorter, this does integer-overflow checks on the final
> length.
Nice. TIL. Will fix.
>>> @@ -1373,16 +1405,17 @@ void base_ref_store_init(struct ref_store *refs,
>>> die("BUG: main_ref_store initialized twice");
>>>
>>> refs->submodule = "";
>>> - refs->next = NULL;
>>> main_ref_store = refs;
>>> } else {
>>> - if (lookup_ref_store(submodule))
>>> + refs->submodule = xstrdup(submodule);
>>> +
>>> + if (!submodule_ref_stores.tablesize)
>>> + hashmap_init(&submodule_ref_stores, submodule_hash_cmp, 20);
>>
>> Makes me wonder what "20" stands for. Perhaps the caller should be
>> allowed to say "I do not quite care what initial size is" by passing
>> 0 or some equally but more clealy meaningless value (which of course
>> would be outside the scope of this series).
>
> I think this is what "0" already does (grep for HASHMAP_INITIAL_SIZE).
> In fact, that constant is 64. The 20 we pass in goes through some magic
> load-factor computation and ends up as 25. That being smaller than the
> INITIAL_SIZE constant, I believe that we end up allocating 64 entries
> either way (that's just from reading the code, though; I didn't run it
> to double check).
I guess I might as well change it to zero, then.
Thanks for the feedback!
Michael
^ permalink raw reply
* Re: [PATCH v2] gc: ignore old gc.log files
From: Junio C Hamano @ 2017-02-10 9:16 UTC (permalink / raw)
To: David Turner; +Cc: git, peff, pclouds
In-Reply-To: <20170209191724.3987-1-dturner@twosigma.com>
David Turner <dturner@twosigma.com> writes:
> The intent of automatic gc is to have a git repository be relatively
> low-maintenance from a server-operator perspective.
This is diametrically opposite from how I recall the auto-gc came
about in Sep 2007. The primary purpose was to help desktop clients
that never runs repack.
By pointing this out, I do not mean that we shouldn't make auto-gc
work well in the server settings. I however do not want our log
messages to distort history in order to justify a change that is
worth making, and I do not think this change needs to do that.
For example, a paragraph like this:
It would be really nice if the auto gc mechanism can be used to
help server operators, even though the original purpose it was
introduced was primarily to help desktop clients that never
repacks.
followed by a description of what makes it not exactly helpful for
server operators in the current behaviour (iow, "what is it that you
are fixing?"), would be a useful justification that is faithful to
the history. Of course, ", even though..." part is irrelevant
and/or unnecessary in that description of the motivation, and if you
omit it, I wouldn't call that is distorting the history.
> Of course, large
> operators like GitHub will need a more complicated management strategy,
> but for ordinary usage, git should just work.
True. "should just work" may want to be replaced by what exactly
are the things it currently does that you view as its problems.
Once you say that, "git learns to do x and y" in the next paragraph,
i.e. the description of the solution to the problem, starts making
sense.
>
> In this commit, git learns to ignore gc.log files which are older than
> (by default) one day old. It also learns about a config, gc.logExpiry
> to manage this. There is also some cleanup: a successful manual gc,
> or a warning-free auto gc with an old log file, will remove any old
> gc.log files.
>
> So git should never get itself into a state where it refuses to do any
> maintenance, just because at some point some piece of the maintenance
> didn't make progress. That might still happen (e.g. because the repo
> is corrupt), but at the very least it won't be because Git is too dumb
> to try again.
IOW, what you wrote in this last paragraph can come earlier to
explain what you perceive as problems the current behaviour has.
> Signed-off-by: David Turner <dturner@twosigma.com>
> Helped-by: Jeff King <peff@peff.net>
> ---
A v2 patch is unfriendly to reviewers unless it is sent with summary
of what got changed since v1, taking input from the discussion on
the previous round, and here before the diffstat is a good place to
do so.
> +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
> + "1.day". See `gc.pruneExpire` for more possible values.
> +
Micronit. Perhaps you meant by "more possible values" "more ways to
specify its values", IOW, you didn't mean to say "instead of 1.day,
you can say 2.days".
> diff --git a/builtin/gc.c b/builtin/gc.c
> index 331f21926..46edcff30 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -33,6 +33,7 @@ static int aggressive_window = 250;
> static int gc_auto_threshold = 6700;
> static int gc_auto_pack_limit = 50;
> static int detach_auto = 1;
> +static unsigned long gc_log_expire_time;
> static const char *prune_expire = "2.weeks.ago";
> static const char *prune_worktrees_expire = "3.months.ago";
>
> @@ -76,10 +77,12 @@ static void git_config_date_string(const char *key, const char **output)
> static void process_log_file(void)
> {
> struct stat st;
> - if (!fstat(get_lock_file_fd(&log_lock), &st) && st.st_size)
> + if (!fstat(get_lock_file_fd(&log_lock), &st) && st.st_size) {
> commit_lock_file(&log_lock);
> - else
> + } else {
> + unlink(git_path("gc.log"));
> rollback_lock_file(&log_lock);
After we grab a lock by creating gc.log.lock, if we fail to fstat(2),
we remove gc.log? That does not sound quite right, as the failure
to fstat(2) sounds like a log-worthy event. Removing the log after
noticing that we didn't write anything (i.e. st.st_size being 0) is
quite sensible, though.
> @@ -111,6 +114,11 @@ static void gc_config(void)
> git_config_get_int("gc.auto", &gc_auto_threshold);
> git_config_get_int("gc.autopacklimit", &gc_auto_pack_limit);
> git_config_get_bool("gc.autodetach", &detach_auto);
> +
> + if (!git_config_get_value("gc.logexpiry", &value)) {
> + parse_expiry_date(value, &gc_log_expire_time);
> + }
Drop {}?
> @@ -290,19 +298,34 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
> static int report_last_gc_error(void)
> {
> struct strbuf sb = STRBUF_INIT;
> - int ret;
> + int ret = 0;
> + struct stat st;
> + char *gc_log_path = git_pathdup("gc.log");
>
> - ret = strbuf_read_file(&sb, git_path("gc.log"), 0);
> + if (stat(gc_log_path, &st)) {
> + if (errno == ENOENT)
> + goto done;
> +
> + ret = error(_("Can't read %s"), gc_log_path);
You probably want to use error_errno() instead here. This is not
"can't read"; your stat() noticed there is something wrong and you
gave up before you even attempted to read.
> + goto done;
> + }
> +
> + if (st.st_mtime < gc_log_expire_time)
> + goto done;
OK.
> + ret = strbuf_read_file(&sb, gc_log_path, 0);
> if (ret > 0)
> - return error(_("The last gc run reported the following. "
> + ret = error(_("The last gc run reported the following. "
> "Please correct the root cause\n"
> "and remove %s.\n"
> "Automatic cleanup will not be performed "
> "until the file is removed.\n\n"
> "%s"),
> - git_path("gc.log"), sb.buf);
> + gc_log_path, sb.buf);
> strbuf_release(&sb);
> - return 0;
> +done:
> + free(gc_log_path);
> + return ret;
> }
OK.
> @@ -349,6 +372,8 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
> argv_array_pushl(&prune_worktrees, "worktree", "prune", "--expire", NULL);
> argv_array_pushl(&rerere, "rerere", "gc", NULL);
>
> + /* default expiry time, overwritten in gc_config */
> + parse_expiry_date("1.day", &gc_log_expire_time);
Alternatively, we can mimick the way in which prune_expire and
prune_worktrees_expire are set up (i.e. they are kept as strings,
configuration overwrites the string), and then turn the final string
into value after gc_config() returns. I think what you wrote here
may be simpler. Nice.
> gc_config();
>
> if (pack_refs < 0)
> @@ -448,5 +473,8 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
> warning(_("There are too many unreachable loose objects; "
> "run 'git prune' to remove them."));
>
> + if (!daemonized)
> + unlink(git_path("gc.log"));
> +
OK. We want to remove "gc.log" after running a successful
foreground gc and this does exactly that.
^ permalink raw reply
* [PATCH v2] gc: ignore old gc.log files
From: David Turner @ 2017-02-09 19:17 UTC (permalink / raw)
To: git; +Cc: peff, pclouds, David Turner
The intent of automatic gc is to have a git repository be relatively
low-maintenance from a server-operator perspective. Of course, large
operators like GitHub will need a more complicated management strategy,
but for ordinary usage, git should just work.
In this commit, git learns to ignore gc.log files which are older than
(by default) one day old. It also learns about a config, gc.logExpiry
to manage this. There is also some cleanup: a successful manual gc,
or a warning-free auto gc with an old log file, will remove any old
gc.log files.
So git should never get itself into a state where it refuses to do any
maintenance, just because at some point some piece of the maintenance
didn't make progress. That might still happen (e.g. because the repo
is corrupt), but at the very least it won't be because Git is too dumb
to try again.
Signed-off-by: David Turner <dturner@twosigma.com>
Helped-by: Jeff King <peff@peff.net>
---
Documentation/config.txt | 5 +++++
builtin/gc.c | 42 +++++++++++++++++++++++++++++++++++-------
t/t6500-gc.sh | 13 +++++++++++++
3 files changed, 53 insertions(+), 7 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index fc5a28a32..2c2c9c75c 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1402,6 +1402,11 @@ gc.autoDetach::
Make `git gc --auto` return immediately and run in background
if the system supports it. Default is true.
+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
+ "1.day". See `gc.pruneExpire` for more possible values.
+
gc.packRefs::
Running `git pack-refs` in a repository renders it
unclonable by Git versions prior to 1.5.1.2 over dumb
diff --git a/builtin/gc.c b/builtin/gc.c
index 331f21926..46edcff30 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -33,6 +33,7 @@ static int aggressive_window = 250;
static int gc_auto_threshold = 6700;
static int gc_auto_pack_limit = 50;
static int detach_auto = 1;
+static unsigned long gc_log_expire_time;
static const char *prune_expire = "2.weeks.ago";
static const char *prune_worktrees_expire = "3.months.ago";
@@ -76,10 +77,12 @@ static void git_config_date_string(const char *key, const char **output)
static void process_log_file(void)
{
struct stat st;
- if (!fstat(get_lock_file_fd(&log_lock), &st) && st.st_size)
+ if (!fstat(get_lock_file_fd(&log_lock), &st) && st.st_size) {
commit_lock_file(&log_lock);
- else
+ } else {
+ unlink(git_path("gc.log"));
rollback_lock_file(&log_lock);
+ }
}
static void process_log_file_at_exit(void)
@@ -111,6 +114,11 @@ static void gc_config(void)
git_config_get_int("gc.auto", &gc_auto_threshold);
git_config_get_int("gc.autopacklimit", &gc_auto_pack_limit);
git_config_get_bool("gc.autodetach", &detach_auto);
+
+ if (!git_config_get_value("gc.logexpiry", &value)) {
+ parse_expiry_date(value, &gc_log_expire_time);
+ }
+
git_config_date_string("gc.pruneexpire", &prune_expire);
git_config_date_string("gc.worktreepruneexpire", &prune_worktrees_expire);
git_config(git_default_config, NULL);
@@ -290,19 +298,34 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
static int report_last_gc_error(void)
{
struct strbuf sb = STRBUF_INIT;
- int ret;
+ int ret = 0;
+ struct stat st;
+ char *gc_log_path = git_pathdup("gc.log");
- ret = strbuf_read_file(&sb, git_path("gc.log"), 0);
+ if (stat(gc_log_path, &st)) {
+ if (errno == ENOENT)
+ goto done;
+
+ ret = error(_("Can't read %s"), gc_log_path);
+ goto done;
+ }
+
+ if (st.st_mtime < gc_log_expire_time)
+ goto done;
+
+ ret = strbuf_read_file(&sb, gc_log_path, 0);
if (ret > 0)
- return error(_("The last gc run reported the following. "
+ ret = error(_("The last gc run reported the following. "
"Please correct the root cause\n"
"and remove %s.\n"
"Automatic cleanup will not be performed "
"until the file is removed.\n\n"
"%s"),
- git_path("gc.log"), sb.buf);
+ gc_log_path, sb.buf);
strbuf_release(&sb);
- return 0;
+done:
+ free(gc_log_path);
+ return ret;
}
static int gc_before_repack(void)
@@ -349,6 +372,8 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
argv_array_pushl(&prune_worktrees, "worktree", "prune", "--expire", NULL);
argv_array_pushl(&rerere, "rerere", "gc", NULL);
+ /* default expiry time, overwritten in gc_config */
+ parse_expiry_date("1.day", &gc_log_expire_time);
gc_config();
if (pack_refs < 0)
@@ -448,5 +473,8 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
warning(_("There are too many unreachable loose objects; "
"run 'git prune' to remove them."));
+ if (!daemonized)
+ unlink(git_path("gc.log"));
+
return 0;
}
diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh
index 1762dfa6a..84ad07eb2 100755
--- a/t/t6500-gc.sh
+++ b/t/t6500-gc.sh
@@ -67,5 +67,18 @@ test_expect_success 'auto gc with too many loose objects does not attempt to cre
test_line_count = 2 new # There is one new pack and its .idx
'
+test_expect_success 'background auto gc does not run if gc.log is present and recent but does if it is old' '
+ keep=$(ls .git/objects/pack/*.pack|head -1|sed -e "s/pack$/keep/") &&
+ test_commit foo &&
+ test_commit bar &&
+ git repack &&
+ test_config gc.autopacklimit 1 &&
+ test_config gc.autodetach true &&
+ echo fleem >.git/gc.log &&
+ test_must_fail git gc --auto 2>err &&
+ test_i18ngrep "^error:" err &&
+ test-chmtime =-86401 .git/gc.log &&
+ git gc --auto
+'
test_done
--
2.11.GIT
^ permalink raw reply related
* git subtree add fails in new repository
From: Daurnimator @ 2017-02-10 6:03 UTC (permalink / raw)
To: Git Mailing List
I'm trying to make a series of repository that only contain subtrees
for various other projects.
However git subtree does not like being on a newly created branch:
$ git init
$ git subtree add --prefix=git https://github.com/git/git.git master
fatal: ambiguous argument 'HEAD': unknown revision or path not in the
working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'
Working tree has modifications. Cannot add.
^ permalink raw reply
* Re: [PATCH] mingw: use OpenSSL's SHA-1 routines
From: Jeff King @ 2017-02-10 5:02 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Jeff Hostetler, Junio C Hamano
In-Reply-To: <6a29f8c60d315a24292c1fa9f5e84df4dfdbf813.1486679254.git.johannes.schindelin@gmx.de>
On Thu, Feb 09, 2017 at 11:27:49PM +0100, Johannes Schindelin wrote:
> From: Jeff Hostetler <jeffhost@microsoft.com>
>
> Use OpenSSL's SHA-1 routines rather than builtin block-sha1 routines.
> This improves performance on SHA1 operations on Intel processors.
>
> OpenSSL 1.0.2 has made considerable performance improvements and
> support the Intel hardware acceleration features. See:
> https://software.intel.com/en-us/articles/improving-openssl-performance
> https://software.intel.com/en-us/articles/intel-sha-extensions
>
> To test this I added/staged a single file in a gigantic
> repository having a 450MB index file. The code in read-cache.c
> verifies the header SHA as it reads the index and computes a new
> header SHA as it writes out the new index. Therefore, in this test
> the SHA code must process 900MB of data. Testing was done on an
> Intel I7-4770 CPU @ 3.40GHz (Intel64, Family 6, Model 60) CPU.
I think this is only half the story. A heavy-sha1 workload is faster,
which is good. But one of the original reasons to prefer blk-sha1 (at
least on Linux) is that resolving libcrypto.so symbols takes a
non-trivial amount of time. I just timed it again, and it seems to be
consistently 1ms slower to run "git rev-parse --git-dir" on my machine
(from the top-level of a repo).
1ms is mostly irrelevant, but it adds up on scripted workloads that
start a lot of git processes. Whether it's a net win or not depends on
how much sha1 computation you do in your workload versus how many
processes you start.
I don't know what that means for Windows, though. My impression is that
process startup is so painfully slow there that the link time may just
be lost in the noise. It may just always be a win there. So not really
an objection to your patch, but something you may want to consider.
(Of course, it would in theory be possible to have the best of both
worlds either by static-linking openssl, or by teaching block-sha1 the
same optimizations, but both of those are obviously much more complex).
-Peff
^ permalink raw reply
* Re: [PATCH 1/5] refs: store submodule ref stores in a hashmap
From: Jeff King @ 2017-02-10 4:04 UTC (permalink / raw)
To: Junio C Hamano
Cc: Michael Haggerty, Nguyễn Thái Ngọc Duy,
Stefan Beller, Johannes Schindelin, David Turner, git
In-Reply-To: <xmqqh943p0hv.fsf@gitster.mtv.corp.google.com>
On Thu, Feb 09, 2017 at 12:34:04PM -0800, Junio C Hamano wrote:
> > +static struct submodule_hash_entry *alloc_submodule_hash_entry(
> > + const char *submodule, struct ref_store *refs)
> > +{
> > + size_t len = strlen(submodule);
> > + struct submodule_hash_entry *entry = malloc(sizeof(*entry) + len + 1);
>
> I think this (and the later memcpy) is what FLEX_ALLOC_MEM() was
> invented for.
Yes, it was. Though since the length comes from a strlen() call, it can
actually use the _STR variant, like:
FLEX_ALLOC_STR(entry, submodule, submodule);
Besides being shorter, this does integer-overflow checks on the final
length.
> > @@ -1373,16 +1405,17 @@ void base_ref_store_init(struct ref_store *refs,
> > die("BUG: main_ref_store initialized twice");
> >
> > refs->submodule = "";
> > - refs->next = NULL;
> > main_ref_store = refs;
> > } else {
> > - if (lookup_ref_store(submodule))
> > + refs->submodule = xstrdup(submodule);
> > +
> > + if (!submodule_ref_stores.tablesize)
> > + hashmap_init(&submodule_ref_stores, submodule_hash_cmp, 20);
>
> Makes me wonder what "20" stands for. Perhaps the caller should be
> allowed to say "I do not quite care what initial size is" by passing
> 0 or some equally but more clealy meaningless value (which of course
> would be outside the scope of this series).
I think this is what "0" already does (grep for HASHMAP_INITIAL_SIZE).
In fact, that constant is 64. The 20 we pass in goes through some magic
load-factor computation and ends up as 25. That being smaller than the
INITIAL_SIZE constant, I believe that we end up allocating 64 entries
either way (that's just from reading the code, though; I didn't run it
to double check).
-Peff
^ permalink raw reply
* Re: [PATCH] rev-parse --git-path: fix output when running in a subdirectory
From: Jeff King @ 2017-02-10 4:21 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johannes Schindelin, git, Nguyễn Thái Ngọc Duy
In-Reply-To: <xmqqo9ybnie0.fsf@gitster.mtv.corp.google.com>
On Thu, Feb 09, 2017 at 01:50:31PM -0800, Junio C Hamano wrote:
> > There is just no way you can "fix" this otherwise. As an occasional shell
> > scripter, you may be tempted to use `$(git rev-parse --show-cdup)$(git
> > rev-parse --git-path filename)`, but of course that breaks in worktrees
> > and if you do not use worktrees you would not even know that your
> > workaround introduced another bug.
>
> In case it is not clear, I understand all of the above.
>
> I was just worried about the people who do *NOT* use worktrees and
> did the obvious "concatenate --cdup with --git-path" and thought
> their script were working happily and well. By prepending the path
> to the (real) location of the .git in the updated --git-path output
> ourselves, they will complain, our update broke their script.
That concatenating approach is broken in other ways, too. For example,
if you have $GIT_DIR set to an absolute path, then --git-path will use
that. I don't think we have ever promised that the output of --git-path
(or --git-dir) would ever be absolute or relative (in fact, the
--git-dir documentation implies that you may get either).
So yes, there could be somebody who is doing this concatenation
workaround, but only ever calls their script in a certain way that never
triggers the absolute-path variant. They're happy now, and may not be
after Dscho's patch. But I really think they are relying on a bogus
assumption, and their scripts are already buggy.
-Peff
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox