Re: Bisect limited to Merge Commits
On Wed, Apr 27, 2016 at 11:19 PM, Hagen Paul Pfeifer wrote: > * Johannes Sixt | 2016-04-27 23:33:53 [+0200]: > > Hey Junio, hey Hannes, > >> git bisect start >> git rev-list --first-parent --boundary origin..origin/pu | >> sed -ne s/-//p | xargs git bisect good >> git bisect bad origin/pu >> >>and it starts bisecting among the 50-something first-parent commits between >>origin and origin/pu. > > just for clarification: contributors rebase their work before pushing it on > master. The integrator simple merges --no-ff the individual branches. Just a > regular workflow, nothing special - except that many contributor commits will > not build. ;( > > The idea is just to skip the contributor commits during bisect and focus on > the merge commits (the ones with more than one ancestors) because they are > likely build and testable. > > One possible approach is probably to sort out all non-merge commits before > bisecting and bisect only on a this set of commits. The advantage is that the > first bad commit is the merge commit introduced the regression. Mmmh, any > comments? > I suspect doing something akin to the idea of "bisect --first-parent" would work for this use case and be more flexible in general. Your idea is pretty much what i think bisect --first-parent would do, except that it would also work for non-merge commits which happen to be in the "mainline" history. Thanks, Jake -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 0/3] git-p4: fix Git LFS pointer parsing
On Thu, Apr 28, 2016 at 8:26 AM, wrote: > diff to v3: > * fix missing assignment of pointerFile variable > ($gmane/292454, thanks Sebastian for making me aware) > * fix s/brake/break/ in commit message > ($gmane/292451, thanks Eric) The series looks good to me now. Regards, Sebastian -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 2/3] travis-ci: express Linux/OS X dependency versions more clearly
From: Lars Schneider The Git Travis CI OSX build always installs the latest versions of Git LFS and Perforce via brew and the Linux build installs fixed versions. Consequently new LFS/Perforce versions can break the OS X build even if there is no change in Git. Signed-off-by: Lars Schneider --- .travis.yml | 17 ++--- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/.travis.yml b/.travis.yml index 4acf617..1fdcec8 100644 --- a/.travis.yml +++ b/.travis.yml @@ -22,8 +22,11 @@ addons: env: global: - DEVELOPER=1 -- P4_VERSION="16.1" -- GIT_LFS_VERSION="1.2.0" +# The Linux build installs the defined dependency versions below. +# The OS X build installs the latest available versions. Keep that +# in mind when you encounter a broken OS X build! +- LINUX_P4_VERSION="16.1" +- LINUX_GIT_LFS_VERSION="1.2.0" - DEFAULT_TEST_TARGET=prove - GIT_PROVE_OPTS="--timer --jobs 3 --state=failed,slow,save" - GIT_TEST_OPTS="--verbose --tee" @@ -38,17 +41,17 @@ before_install: linux) mkdir --parents custom/p4 pushd custom/p4 -wget --quiet http://filehost.perforce.com/perforce/r$P4_VERSION/bin.linux26x86_64/p4d -wget --quiet http://filehost.perforce.com/perforce/r$P4_VERSION/bin.linux26x86_64/p4 +wget --quiet http://filehost.perforce.com/perforce/r$LINUX_P4_VERSION/bin.linux26x86_64/p4d +wget --quiet http://filehost.perforce.com/perforce/r$LINUX_P4_VERSION/bin.linux26x86_64/p4 chmod u+x p4d chmod u+x p4 export PATH="$(pwd):$PATH" popd mkdir --parents custom/git-lfs pushd custom/git-lfs -wget --quiet https://github.com/github/git-lfs/releases/download/v$GIT_LFS_VERSION/git-lfs-linux-amd64-$GIT_LFS_VERSION.tar.gz -tar --extract --gunzip --file "git-lfs-linux-amd64-$GIT_LFS_VERSION.tar.gz" -cp git-lfs-$GIT_LFS_VERSION/git-lfs . +wget --quiet https://github.com/github/git-lfs/releases/download/v$LINUX_GIT_LFS_VERSION/git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz +tar --extract --gunzip --file "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz" +cp git-lfs-$LINUX_GIT_LFS_VERSION/git-lfs . export PATH="$(pwd):$PATH" popd ;; -- 2.5.1 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 3/3] git-p4: fix Git LFS pointer parsing
From: Lars Schneider
Git LFS 1.2.0 removed a preamble from the output of the 'git lfs pointer'
command [1] which broke the parsing of this output. Adjust the parser
to support the old and the new format.
Please note that this patch slightly changes the second return parameter
from a list of LF terminated strings to a single string that contains
a number of LF characters.
[1]
https://github.com/github/git-lfs/commit/da2935d9a739592bc775c98d8ef4df9c72ea3b43
Signed-off-by: Lars Schneider
Helped-by: Sebastian Schuberth
Helped-by: Ben Woosley
Signed-off-by: Lars Schneider
---
git-p4.py | 13 ++---
t/t9824-git-p4-git-lfs.sh | 4
2 files changed, 14 insertions(+), 3 deletions(-)
diff --git a/git-p4.py b/git-p4.py
index 527d44b..8ab48f2 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1064,8 +1064,15 @@ class GitLFS(LargeFileSystem):
if pointerProcess.wait():
os.remove(contentFile)
die('git-lfs pointer command failed. Did you install the
extension?')
-pointerContents = [i+'\n' for i in pointerFile.split('\n')[2:][:-1]]
-oid = pointerContents[1].split(' ')[1].split(':')[1][:-1]
+
+# Git LFS removed the preamble in the output of the 'pointer' command
+# starting from version 1.2.0. Check for the preamble here to support
+# earlier versions.
+# c.f.
https://github.com/github/git-lfs/commit/da2935d9a739592bc775c98d8ef4df9c72ea3b43
+if pointerFile.startswith('Git LFS pointer for'):
+pointerFile = re.sub(r'Git LFS pointer for.*\n\n', '', pointerFile)
+
+oid = re.search(r'^oid \w+:(\w+)', pointerFile, re.MULTILINE).group(1)
localLargeFile = os.path.join(
os.getcwd(),
'.git', 'lfs', 'objects', oid[:2], oid[2:4],
@@ -1073,7 +1080,7 @@ class GitLFS(LargeFileSystem):
)
# LFS Spec states that pointer files should not have the executable
bit set.
gitMode = '100644'
-return (gitMode, pointerContents, localLargeFile)
+return (gitMode, pointerFile, localLargeFile)
def pushFile(self, localLargeFile):
uploadProcess = subprocess.Popen(
diff --git a/t/t9824-git-p4-git-lfs.sh b/t/t9824-git-p4-git-lfs.sh
index 0b664a3..ca93ac8 100755
--- a/t/t9824-git-p4-git-lfs.sh
+++ b/t/t9824-git-p4-git-lfs.sh
@@ -13,6 +13,10 @@ test_file_in_lfs () {
FILE="$1" &&
SIZE="$2" &&
EXPECTED_CONTENT="$3" &&
+ sed -n '1,1 p' "$FILE" | grep "^version " &&
+ sed -n '2,2 p' "$FILE" | grep "^oid " &&
+ sed -n '3,3 p' "$FILE" | grep "^size " &&
+ test_line_count = 3 "$FILE" &&
cat "$FILE" | grep "size $SIZE" &&
HASH=$(cat "$FILE" | grep "oid sha256:" | sed -e "s/oid sha256://g") &&
LFS_FILE=".git/lfs/objects/$(echo "$HASH" | cut -c1-2)/$(echo "$HASH" |
cut -c3-4)/$HASH" &&
--
2.5.1
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 1/3] travis-ci: update Git-LFS and P4 to the latest version
From: Lars Schneider Signed-off-by: Lars Schneider --- .travis.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.travis.yml b/.travis.yml index 78e433b..4acf617 100644 --- a/.travis.yml +++ b/.travis.yml @@ -22,8 +22,8 @@ addons: env: global: - DEVELOPER=1 -- P4_VERSION="15.2" -- GIT_LFS_VERSION="1.1.0" +- P4_VERSION="16.1" +- GIT_LFS_VERSION="1.2.0" - DEFAULT_TEST_TARGET=prove - GIT_PROVE_OPTS="--timer --jobs 3 --state=failed,slow,save" - GIT_TEST_OPTS="--verbose --tee" -- 2.5.1 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 0/3] git-p4: fix Git LFS pointer parsing
From: Lars Schneider v1: $gmane/291917 v2: $gmane/291991 v3: $gmane/292425 diff to v3: * fix missing assignment of pointerFile variable ($gmane/292454, thanks Sebastian for making me aware) * fix s/brake/break/ in commit message ($gmane/292451, thanks Eric) Thanks, Lars Lars Schneider (3): travis-ci: update Git-LFS and P4 to the latest version travis-ci: express Linux/OS X dependency versions more clearly git-p4: fix Git LFS pointer parsing .travis.yml | 17 ++--- git-p4.py | 13 ++--- t/t9824-git-p4-git-lfs.sh | 4 3 files changed, 24 insertions(+), 10 deletions(-) -- 2.5.1 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Bisect limited to Merge Commits
* Johannes Sixt | 2016-04-27 23:33:53 [+0200]: Hey Junio, hey Hannes, > git bisect start > git rev-list --first-parent --boundary origin..origin/pu | > sed -ne s/-//p | xargs git bisect good > git bisect bad origin/pu > >and it starts bisecting among the 50-something first-parent commits between >origin and origin/pu. just for clarification: contributors rebase their work before pushing it on master. The integrator simple merges --no-ff the individual branches. Just a regular workflow, nothing special - except that many contributor commits will not build. ;( The idea is just to skip the contributor commits during bisect and focus on the merge commits (the ones with more than one ancestors) because they are likely build and testable. One possible approach is probably to sort out all non-merge commits before bisecting and bisect only on a this set of commits. The advantage is that the first bad commit is the merge commit introduced the regression. Mmmh, any comments? hgn -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Strangeness with git-add and nested repositories
Hi Stefan, On Wed, Apr 27, 2016 at 9:08 AM, Stefan Beller wrote: > I think (pure speculation), that it the error is in the context > (repository) switching logic. > What happens if you alter the order, i.e. give testfile first and then > the files in the nested > repos? Interestingly, reversing the order produces the same result (the testfile is added, the nested files are not). I've also noticed that running something like 'git status testfile nestedfiles' results in the nested files being omitted from the git status output; I'd expect them to be printed by git-status as untracked files. So it appears the problem is not isolated to git-add. To give some context, my use case is that I have a parent project that links to numerous chromium libraries, thus my parent project needs access to many of chromium's headers at build time. I wanted to make these headers available to other developers without them needing to check out all of chromium. So I add all the chromium headers to the parent project with something like: find deps/chromium/src -name "*.h" | xargs git add -- I was weirded out to find that many of the header files weren't being added, as I've already described. I ultimately worked around this by doing: find chromium/src -name "*.h" | xargs -n 1 git add Since each file gets added separately, this is quite slow. So it'd be nice if this little bug was fixed someday :) As you probably know, Chromium is comprised of many hundreds of nested repos, so that aided in manifesting this issue. Thanks, Andrew -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Update git-p4 to be compatible with git-lfs 1.2
On 26 Apr 2016, at 01:10, SZEDER Gábor wrote: > > Quoting Junio C Hamano : > >> SZEDER Gábor writes: >> >>> You can have a look at these patches at >>> >>> https://github.com/szeder/git completion-test-multiple-bash-versions >>> >>> and perhaps you could even adapt it to LFS and/or p4 somehow. >>> Plus if we want to be consistent we would need to do the same for LFS 1.0, 1.2, and for pretty much every other dependency... >>> >>> I'm not sure we should be consistent in this case, at least not solely >>> for consistency's sake and not in git.git. Taking what I did for Bash >>> and doing it for different versions of LFS, p4, etc. could perhaps >>> keep the runtime under control, but t/Makefile would surely get out >>> of control rather quickly. Putting these into a travis-ci matrix is >>> so much simpler, but the runtime makes it infeasible, of course. >> >> I took a brief look of your branch, and I like its approach. If I >> understood your approach correctly, you: >> >> * Group selected tests in t/ as "these are bash related tests I >> care about" in t/Makefile; > > Yes. > >> * Add Travis test target to build Git with specific versions of >> bash, and run the above target instead of the full test to >> exercise the version of bash you are testing. > > Not quite. > > * Add t/Makefile targets to run a Bash-related test script with a >specific Bash version, one target for each script-version pair, >and a target to run all Bash-related tests with all listed >Bash-versions. > > * Extend the travis-ci config so that, after building Git as usual >and running the full test suite as usual, it additionaly runs all >Bash-related tests will all listed Bash versions on Linux builds. > >> And I agree that the same can be done for LFS versions and P4 >> versions. Only a handful tests in t/ are about these niches. > > Luckily for me, running a test script with a specific Bash version is > as trivial as '/path/to/bash-vX.Y t9902-completion.sh'. No > modifications to the test scripts or to lib-bash.sh were necessary. > > OTOH, Git LFS and p4 tests, AFAICS, rely on git-lfs and p4 binaries > being available in $PATH, and the p4 tests need two binaries. So > there is more work that has to be done, as we would need a way to > override those binaries found in $PATH, either through an environment > variable or a command line option. Bonus points for a solution that > would work equally well with LFS, p4 and Bash: then perhaps we could > have a single unified block of Makefile metaprogramming, which could > generate all those test targets from a list of dependency-specific > tests and a list of paths to different versions of that dependency. > Then it might even be suitable for inclusion in git.git. Thanks for sharing this! It's awesome! I will try to find a generic solution based on your work that handles bash versions, LFS versions, and P4 versions! Stay tuned :-) Cheers, Lars > > >>> I think the best we can do is to keep this out of git.git and let >>> (hope?) developers interested in a particular subsystem do this >>> "multiple version compatibility" tests as they see fit. > > -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] pack-objects: warn on split packs disabling bitmaps
On Wed, Apr 27, 2016 at 09:53:24PM +, Eric Wong wrote:
> It can be tempting for a server admin to want a stable set of
> long-lived packs for dumb clients; but also want to enable
> bitmaps to serve smart clients more quickly.
>
> Unfortunately, such a configuration is impossible;
> so at least warn users of this incompatibility since
> commit 21134714787a02a37da15424d72c0119b2b8ed71
> ("pack-objects: turn off bitmaps when we split packs").
>
> Tested the warning by inspecting the output of:
>
> make -C t t5310-pack-bitmaps.sh GIT_TEST_OPTS=-v
I think the intent and code in your patch is fine; looks like doc
specifics are being discussed elsewhere.
But I did want to mention one thing, which is that long-lived split
packs are a tradeoff, even for dumb clients. The pack format cannot do
deltas between packs, so the sum of your split packs is larger than a
single pack would be. That's a good thing for somebody who cloned
earlier, and wants to only a few small packs on top. But it's much worse
for somebody who wants to do a fresh clone, and has to grab all of the
packs either way.
> Fwiw, I'm hoping to publish an ~800MB git-clone-able repo of
> our ML archives, soonish. I can serve terabytes of dumb HTTP
> traffic all day long without breaking a sweat; but smart
> packing of big repos worries me; especially when feeding
> slow clients and having to leave processes running
> (or buffering pack output to disk). So perhaps I'll teach
> my HTTP server play dumb whenever CPU/memory usage is high.
Yeah, CPU and memory load for serving large clones is a problem. Memory
especially scales with number of objects (because we keep the whole
packing list in memory for the entirety of the write). At GitHub, we
have some changes to try to serve things verbatim from the on-disk pack
without even creating an in-memory list of objects (it's just a bitmap
of which objects in the packfile to send), and that reduces CPU and
memory load quite a bit. Cleaning up and submitting those patches has
been on my todo list for a while, but I just haven't gotten to it. I'm
of course happy to share the messy state if you want to pick through it
yourself.
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] pack-objects: warn on split packs disabling bitmaps
On Wed, Apr 27, 2016 at 03:56:46PM -0700, Junio C Hamano wrote:
> Eric Wong writes:
>
> > It can be tempting for a server admin to want a stable set of
> > long-lived packs for dumb clients; but also want to enable
> > bitmaps to serve smart clients more quickly.
> >
> > Unfortunately, such a configuration is impossible;
> > so at least warn users of this incompatibility since
> > commit 21134714787a02a37da15424d72c0119b2b8ed71
> > ("pack-objects: turn off bitmaps when we split packs").
> >
> > Tested the warning by inspecting the output of:
> >
> > make -C t t5310-pack-bitmaps.sh GIT_TEST_OPTS=-v
>
> While I do not think the update in this patch is wrong, this makes
> me wonder what happens to a server admin who wants a stable set of
> long-lived objects in a pack, and other objects in another pack that
> is frequently recreated, by first packing objects needed for the
> latest released version into a single pack and marking it with .keep
> and then running "git repack" to create the second pack for the
> remainder.
>
> There is no automatic split involved, so we would get a bitmap file
> for each of these two packs. Would that pose a problem? The pack
> with the newer part of the history would not satisfy "all of the
> reachable objects are in the same pack" condition.
You will not get two bitmaps in such a case. In add_object_entry(), if
we exclude an object via want_object_in_pack(), we will issue a warning
and turn off bitmaps. That includes finding that one of the reachable
objects we would need is in a .keep pack.
You could in theory construct a broken non-closed bitmap by feeding an
arbitrary set of objects to pack-objects, but we turn off bitmap writing
entirely unless --all is used. So the test in add_object_entry() should
be sufficient for all cases there (it's actually too conservative; it's
possible that the object is not reachable from the other objects that
are going into the pack, but this is so impractical that it's not even
worth trying to catch).
The split-pack check from 211347147 had to come separately, because we
only find out about the split much later during the write phase (and
again, it is too conservative; it's _possible_ that the objects being
split aren't reachable from anything in the previous pack, but it's
exceedingly unlikely and not worth caring about).
Adding a warning as Eric's patch does is a definite improvement, and
something I should have done in the original (we _could_ just use the
same no_closure_warning, as that is technically what is going on, but I
think it is nice to mention pack-splitting, which is more likely to lead
the user in the right direction to fixing it).
> [discussion of doc fixes]
I do not mind overly if pack-splitting mentions disabling bitmaps. But I
think it may be simpler to keep the documentation in the bitmap section,
rather than trying to cross-reference in both directions. It is the
bitmap code which is not prepared to work with pack-splits (and other
things, like .keep), not the other way around.
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/15] string_list: add string_list_duplicate
On Wed, Apr 27, 2016 at 4:17 PM, Junio C Hamano wrote:
> Stefan Beller writes:
>
>> Another way to corrupt it is to change the configuration (e.g. add
>> things to the config hashmap such that it reallocates and grows).
>
> You're right. But doesn't it hint that there is a deeper problem?
>
> By making a copy and keeping it, you would hold onto a stale value
> and would not see the result of updates you yourself make to the
> system.
>
In this case the value doesn't go stale. We do not change
"submodule.defaultGroup", but only new submodule.$NAME.url
and such. The memory for accessing it goes stale, so in this case
it is okay. I don't think we want to see repeated calls to
git_config_get_value_multi
like :
if (!pathspec.nr && git_config_get_value_multi(
"submodule.defaultGroup")) {
gitmodules_config();
for (i = 0; i < list.nr; i++) {
const struct submodule *sub =
submodule_from_path(null_sha1,
list.entries[i]->name);
group =
git_config_get_value_multi("submodule.defaultGroup")
if (submodule_in_group(group, sub))
init_submodule(list.entries[i]->name,
prefix, quiet);
}
}
Maybe I am overestimating the cost of git_config_get_value_multi, so it is no
problem?
Thanks,
Stefan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/15] string_list: add string_list_duplicate
Stefan Beller writes: > Another way to corrupt it is to change the configuration (e.g. add > things to the config hashmap such that it reallocates and grows). You're right. But doesn't it hint that there is a deeper problem? By making a copy and keeping it, you would hold onto a stale value and would not see the result of updates you yourself make to the system. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 05/15] submodule-config: check if submodule a submodule is in a group
On Tue, Apr 26, 2016 at 4:17 PM, Junio C Hamano wrote: > Junio C Hamano writes: > >> I see room for bikeshedding here, but the material to bikeshed >> around is not even documented yet ;-) >> >> * a token prefixed with '*' is a label. >> * a token prefixed with './' is a path. >> * a token prefixed with ':' is a name. >> >> Hopefully I will see some description like that in later patches. >> I'll read on. > > Extending this on a bit, I would suggest tweaking the above slightly > and make the rule more like this: > > * a token prefixed with '*' is a label. > > * a token prefixed with ':' is a name. > > * everything else is a path, but "./" at the front is skipped, > which can be used to disambiguate an unfortunate path that > begins with ':' or '*'. > > A bigger thing I am wondering is if it is bettter to do _without_ > adding a new --group=X option everywhere. I am assuming that most > if not all submodule subcommands already use "module_list" aka > "submodule--helper list" that takes paths, and to them, extending > that interface to also understand the groups and names would be a > more natural way to extend the UI, no? e.g. > > $ git submodule update -- 'path1' 'path2' > $ git submodule update -- '*default' > $ git submodule update -- ':named' > > instead of > > $ git submodule update -- 'path1 'path2' > $ git submodule update --group='*default' -- > $ git submodule update --group=':named' -- > > which special-cases the way to specify a set of submodules by > listing their paths. This is indeed a better way. Currently there is no way to initialize another group as that group specified by submodule.defaultGroup. But having the possibility to use the grouping in such a way is more flexible. Thanks, Stefan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] pack-objects: warn on split packs disabling bitmaps
Eric Wong writes:
> It can be tempting for a server admin to want a stable set of
> long-lived packs for dumb clients; but also want to enable
> bitmaps to serve smart clients more quickly.
>
> Unfortunately, such a configuration is impossible;
> so at least warn users of this incompatibility since
> commit 21134714787a02a37da15424d72c0119b2b8ed71
> ("pack-objects: turn off bitmaps when we split packs").
>
> Tested the warning by inspecting the output of:
>
> make -C t t5310-pack-bitmaps.sh GIT_TEST_OPTS=-v
While I do not think the update in this patch is wrong, this makes
me wonder what happens to a server admin who wants a stable set of
long-lived objects in a pack, and other objects in another pack that
is frequently recreated, by first packing objects needed for the
latest released version into a single pack and marking it with .keep
and then running "git repack" to create the second pack for the
remainder.
There is no automatic split involved, so we would get a bitmap file
for each of these two packs. Would that pose a problem? The pack
with the newer part of the history would not satisfy "all of the
reachable objects are in the same pack" condition.
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 42d2b50..ec31170 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2156,8 +2156,10 @@ pack.packSizeLimit::
> The maximum size of a pack. This setting only affects
> packing to a file when repacking, i.e. the git:// protocol
> is unaffected. It can be overridden by the `--max-pack-size`
> - option of linkgit:git-repack[1]. The minimum size allowed is
> - limited to 1 MiB. The default is unlimited.
> + option of linkgit:git-repack[1]. Reaching this limit prevents
> + pack bitmaps from being written when `repack.writeBitmaps`
> + is configured. The minimum size allowed is limited to 1 MiB.
> + The default is unlimited.
This is not wrong per-se, but I find the additional text overly
verbose. The resulting text has at least three issues:
- This sets maximum. It does not say what happens when the maximum
is reached ("packing fails" is a valid expectation). We should
spell out that when the maximum is reached, we will create
multiple packfiles.
- It is not "reaching this limit" that prevents bitmaps from being
written. It is "we will create multiple packfiles" that does.
- Even when repack.writeBitmaps is not configured, but bitmap is
requested via the command line option "repack -b", bitmap
generation is disabled once we end up creating multiple
packfiles.
> @@ -2557,8 +2559,9 @@ repack.writeBitmaps::
> objects to disk (e.g., when `git repack -a` is run). This
> index can speed up the "counting objects" phase of subsequent
> packs created for clones and fetches, at the cost of some disk
> - space and extra time spent on the initial repack. Defaults to
> - false.
> + space and extra time spent on the initial repack. This has
> + no effect if `pack.packSizeLimit` is configured and reached.
> + Defaults to false.
This has the opposite issue as the third point above. We have to
ignore repack.writeBitmaps when we end up splitting a pack,
regardless of the reason why we split was pack.packSizeLimit or by
an explicit request from the command line.
Also to future-proof these two paragraphs (and those that are
touched by later parts of this patch), it may even be a good idea to
omit the mention of packsizelimit altogether. We may invent a new
and different reason why we end up producing multiple packfiles,
other than packsizelimit. What this patch wants to make readers
aware is that when multiple packs are generated, bitmap generation
is disabled. Other details (e.g. how the user asks us to create
multiple packs, either via command line or configuration, or how the
use asks us to generate bitmaps) are of lessor importance.
> diff --git a/Documentation/git-pack-objects.txt
> b/Documentation/git-pack-objects.txt
> index bbea529..19cdcd0 100644
> --- a/Documentation/git-pack-objects.txt
> +++ b/Documentation/git-pack-objects.txt
> @@ -110,7 +110,8 @@ base-name::
> --max-pack-size=::
> Maximum size of each output pack file. The size can be suffixed with
> "k", "m", or "g". The minimum size allowed is limited to 1 MiB.
> - If specified, multiple packfiles may be created.
> + If specified, multiple packfiles may be created, which also
> + prevents the creation of a bitmap index.
This is a good update, judging with the yardstick I set in the
previous paragraph in this review.
> The default is unlimited, unless the config variable
> `pack.packSizeLimit` is set.
>
> diff --git a/Documentation/git-repack.txt b/Documentation/git-repack.txt
> index af230d0..2065812 100644
> --- a/Documentation/git-repack.txt
> +++ b/Documentation/git-repack.txt
> @@ -106,7 +106,8 @@ other objects in that pack they al
Announcing git-cinnabar 0.3.2
Git-cinnabar is a git remote helper to interact with mercurial repositories. It allows to clone, pull and push from/to mercurial remote repositories, using git. Code on https://github.com/glandium/git-cinnabar [ Previous announcements: http://marc.info/?l=git&m=145294370431454 http://marc.info/?l=git&m=145284823007354 http://marc.info/?l=git&m=142837367709781 (...)] What's new since 0.3.1? - Fixed a performance regression when cloning big repositories on OSX. - git configuration items with line breaks are now supported. - Fixed a number of issues with corner cases in mercurial data (such as, but not limited to nodes with no first parent, malformed .hgtags, etc.) - Fixed a stack overflow, a buffer overflow and a use-after free in cinnabar-helper. - Better work with git worktrees, or when called from subdirectories. - Updated git to 2.7.4 for cinnabar-helper. - Properly remove all refs meant to be removed when using git version lower than 2.1. Mike -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: make test Unexpected passes
On Wed, Apr 27, 2016 at 3:05 PM, Junio C Hamano wrote: > Isn't what the test expects bogus in the first place? I'd suggest > removing the test as "pointless waste of resource". > > Comments? > > -- >8 -- Yes, toss it; I find your arguments below compelling. > Manual merge resolution by users fundamentally depends on being able > to tell what is the tracked contents and what is the separator lines > added by "git merge" to tell users which block of lines came from > where. You can deliberately craft a file with lines that resemble > conflict marker lines to make it impossible for the user (here, the > outer merge of merge-recursive also counts as a user of the internal > merge) to tell which part is which, and write a test to demonstrate > that with such a file that "git merge" fundamentally cannot work > with and has to fail on. It however is pointless and waste of time > and resource to run such a test that asserts the obvious. > > In real life, people who do need to track files with such lines that > have as their prefixes set the conflict-marker-size > attribute to make sure they will be able to tell between the tracked > lines that happen to begin with these (confusing) prefixes and the > marker lines that are added by "git merge". -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Bisect limited to Merge Commits
Johannes Sixt writes: > Am 27.04.2016 um 22:56 schrieb Junio C Hamano: >> So being able to stop at only commits on the first-parent chain is a >> valid and useful tool. "git bisect --first-parent" is one of the >> things that are sometimes asked for. > > With origin pointing to git.git, I attempted this: > > git bisect start > git rev-list --first-parent --boundary origin..origin/pu | >sed -ne s/-//p | xargs git bisect good > git bisect bad origin/pu > > and it starts bisecting among the 50-something first-parent commits > between origin and origin/pu. That is a cute idea but I wonder if it would work well in a history full of pointless no-ff merges. Here is a sample topology (output from "git log --oneline --graph") * 84ef1bb pointless |\ | * 4ae9f68 third |/ * aec8732 pointless |\ | * fd4ed0d second |/ * 696b5c1 initial where the tip of 'master' was initial, a side branch built 'second' on it and then 'master' made a no-ff 'pointless' merge, another side branch built 'third' on it and then 'master' again made a no-ff 'pointless' merge. $ git rev-list --first-parent --boundary initial..pointless would give us 'third' and 'second' as boundaries, but by marking 'third' as GOOD, we declare to 'bisect' machinery that it and all of its ancestors are GOOD. So upon seeing 'bisect bad HEAD', we would immediately see that HEAD is the first bad commit, wouldn't we? Actually, "pointless no-ff merges" is a red herring. Any history with side branch that branched from the first-parent chain after the commit at the bottom of the bisection range (in this example, the side branch that built 'third' was forked off of the first 'pointless' commit on the first-parent chain of 'master', which was made after the bottom of the range 'initial') would have the same problem, I would imagine. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 05/29] refname_is_safe(): insist that the refname already be normalized
On Wed, Apr 27, 2016 at 04:37:28PM -0400, Jeff King wrote: > > > But anything writing a _new_ refname (whether the actual ref, or > > > referencing it via a symref) should be using check_refname_format() > > > before writing. > > > > Unfortunately, neither check is lesser -- refname_is_safe allows > > refs/heads//foo but not a/b while check_refname_format allows a/b but > > not refs/heads//foo. So sometimes we need both, while other times we > > just need one :( > > IMHO, that sounds like a bug. check_refname_format() should > conceptually[1] be a superset of refname_is_safe(). Is there a case > where we would want to _allow_ a refname that is not safe to look at on > disk? BTW, if there isn't a superset relationship here, then I suspect I may have introduced some loosening or inconsistencies in 03afcbe (read_packed_refs: avoid double-checking sane refs, 2015-04-16). So any tightening now may simply be fixing that (which doesn't change anything with respect to correctness, but may give people more confidence that the tightening is not breaking something people have been depending on). -Peff PS Reading over that commit message, I think it mis-states "refname_is_safe() can only be true if check_refname_format() also failed". It should be "!refname_is_safe() ...". I.e., the condition can only trigger if... -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: make test Unexpected passes
Elijah Newren writes: > Yeah, the t6036 testcase 'git detects conflict w/ > criss-cross+contrived resolution' could be made to pass by tweaking > the conflict markers. In fact, any tweak would make it appear to > pass, but the test could be updated to still fail by updating the > contrived resolution of a previous merge to use the newer conflict > marker style as well. Isn't what the test expects bogus in the first place? I'd suggest removing the test as "pointless waste of resource". Comments? -- >8 -- Subject: [PATCH] t6036: remove pointless test that expects failure One test in t6036 prepares a file whose contents contain these lines: <<< Temporary merge branch 1 C === B >>> Temporary merge branch 2 and uses recursive merge strategy to run criss-cross merge with it. Manual merge resolution by users fundamentally depends on being able to tell what is the tracked contents and what is the separator lines added by "git merge" to tell users which block of lines came from where. You can deliberately craft a file with lines that resemble conflict marker lines to make it impossible for the user (here, the outer merge of merge-recursive also counts as a user of the internal merge) to tell which part is which, and write a test to demonstrate that with such a file that "git merge" fundamentally cannot work with and has to fail on. It however is pointless and waste of time and resource to run such a test that asserts the obvious. In real life, people who do need to track files with such lines that have as their prefixes set the conflict-marker-size attribute to make sure they will be able to tell between the tracked lines that happen to begin with these (confusing) prefixes and the marker lines that are added by "git merge". Remove the test as pointless waste of resource. Signed-off-by: Junio C Hamano --- t/t6036-recursive-corner-cases.sh | 83 --- 1 file changed, 83 deletions(-) diff --git a/t/t6036-recursive-corner-cases.sh b/t/t6036-recursive-corner-cases.sh index cc1ee6a..c7b0ae6 100755 --- a/t/t6036-recursive-corner-cases.sh +++ b/t/t6036-recursive-corner-cases.sh @@ -304,89 +304,6 @@ test_expect_success 'git detects conflict merging criss-cross+modify/delete, rev test $(git rev-parse :3:file) = $(git rev-parse B:file) ' -# -# criss-cross + modify/modify with very contrived file contents: -# -# B D -# o---o -# / \ / \ -# A o X ? F -# \ / \ / -# o---o -# C E -# -# Commit A: file with contents 'A\n' -# Commit B: file with contents 'B\n' -# Commit C: file with contents 'C\n' -# Commit D: file with contents 'D\n' -# Commit E: file with contents: -# <<< Temporary merge branch 1 -# C -# === -# B -# >>> Temporary merge branch 2 -# -# Now, when we merge commits D & E, does git detect the conflict? - -test_expect_success 'setup differently handled merges of content conflict' ' - git clean -fdqx && - rm -rf .git && - git init && - - echo A >file && - git add file && - test_tick && - git commit -m A && - - git branch B && - git checkout -b C && - echo C >file && - git add file && - test_tick && - git commit -m C && - - git checkout B && - echo B >file && - git add file && - test_tick && - git commit -m B && - - git checkout B^0 && - test_must_fail git merge C && - echo D >file && - git add file && - test_tick && - git commit -m D && - git tag D && - - git checkout C^0 && - test_must_fail git merge B && - cat>> Temporary merge branch 2 -EOF - git add file && - test_tick && - git commit -m E && - git tag E -' - -test_expect_failure 'git detects conflict w/ criss-cross+contrived resolution' ' - git checkout D^0 && - - test_must_fail git merge -s recursive E^0 && - - test 3 -eq $(git ls-files -s | wc -l) && - test 3 -eq $(git ls-files -u | wc -l) && - test 0 -eq $(git ls-files -o | wc -l) && - - test $(git rev-parse :2:file) = $(git rev-parse D:file) && - test $(git rev-parse :3:file) = $(git rev-parse E:file) -' - # # criss-cross + d/f conflict via add/add: # Commit A: Neither file 'a' nor directory 'a/' exists. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] pack-objects: warn on split packs disabling bitmaps
It can be tempting for a server admin to want a stable set of
long-lived packs for dumb clients; but also want to enable
bitmaps to serve smart clients more quickly.
Unfortunately, such a configuration is impossible;
so at least warn users of this incompatibility since
commit 21134714787a02a37da15424d72c0119b2b8ed71
("pack-objects: turn off bitmaps when we split packs").
Tested the warning by inspecting the output of:
make -C t t5310-pack-bitmaps.sh GIT_TEST_OPTS=-v
Signed-off-by: Eric Wong
---
Fwiw, I'm hoping to publish an ~800MB git-clone-able repo of
our ML archives, soonish. I can serve terabytes of dumb HTTP
traffic all day long without breaking a sweat; but smart
packing of big repos worries me; especially when feeding
slow clients and having to leave processes running
(or buffering pack output to disk). So perhaps I'll teach
my HTTP server play dumb whenever CPU/memory usage is high.
Documentation/config.txt | 11 +++
Documentation/git-pack-objects.txt | 3 ++-
Documentation/git-repack.txt | 9 ++---
builtin/pack-objects.c | 9 -
4 files changed, 23 insertions(+), 9 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 42d2b50..ec31170 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2156,8 +2156,10 @@ pack.packSizeLimit::
The maximum size of a pack. This setting only affects
packing to a file when repacking, i.e. the git:// protocol
is unaffected. It can be overridden by the `--max-pack-size`
- option of linkgit:git-repack[1]. The minimum size allowed is
- limited to 1 MiB. The default is unlimited.
+ option of linkgit:git-repack[1]. Reaching this limit prevents
+ pack bitmaps from being written when `repack.writeBitmaps`
+ is configured. The minimum size allowed is limited to 1 MiB.
+ The default is unlimited.
Common unit suffixes of 'k', 'm', or 'g' are
supported.
@@ -2557,8 +2559,9 @@ repack.writeBitmaps::
objects to disk (e.g., when `git repack -a` is run). This
index can speed up the "counting objects" phase of subsequent
packs created for clones and fetches, at the cost of some disk
- space and extra time spent on the initial repack. Defaults to
- false.
+ space and extra time spent on the initial repack. This has
+ no effect if `pack.packSizeLimit` is configured and reached.
+ Defaults to false.
rerere.autoUpdate::
When set to true, `git-rerere` updates the index with the
diff --git a/Documentation/git-pack-objects.txt
b/Documentation/git-pack-objects.txt
index bbea529..19cdcd0 100644
--- a/Documentation/git-pack-objects.txt
+++ b/Documentation/git-pack-objects.txt
@@ -110,7 +110,8 @@ base-name::
--max-pack-size=::
Maximum size of each output pack file. The size can be suffixed with
"k", "m", or "g". The minimum size allowed is limited to 1 MiB.
- If specified, multiple packfiles may be created.
+ If specified, multiple packfiles may be created, which also
+ prevents the creation of a bitmap index.
The default is unlimited, unless the config variable
`pack.packSizeLimit` is set.
diff --git a/Documentation/git-repack.txt b/Documentation/git-repack.txt
index af230d0..2065812 100644
--- a/Documentation/git-repack.txt
+++ b/Documentation/git-repack.txt
@@ -106,7 +106,8 @@ other objects in that pack they already have locally.
--max-pack-size=::
Maximum size of each output pack file. The size can be suffixed with
"k", "m", or "g". The minimum size allowed is limited to 1 MiB.
- If specified, multiple packfiles may be created.
+ If specified, multiple packfiles may be created, causing
+ `--write-bitmap-index` and `repack.writeBitmaps` to be ignored.
The default is unlimited, unless the config variable
`pack.packSizeLimit` is set.
@@ -115,7 +116,9 @@ other objects in that pack they already have locally.
Write a reachability bitmap index as part of the repack. This
only makes sense when used with `-a` or `-A`, as the bitmaps
must be able to refer to all reachable objects. This option
- overrides the setting of `pack.writeBitmaps`.
+ overrides the setting of `repack.writeBitmaps`. This option
+ has no effect if a multiple packfiles are created due to
+ reaching `pack.packSizeLimit` or `--max-pack-size`.
--pack-kept-objects::
Include objects in `.keep` files when repacking. Note that we
@@ -123,7 +126,7 @@ other objects in that pack they already have locally.
This means that we may duplicate objects, but this makes the
option safe to use when there are concurrent pushes or fetches.
This option is generally only useful if you are writing bitmaps
- with `-b` or `pack.writeBitmaps`, as it ensures that the
+ with `-b` or `repack
Re: Bisect limited to Merge Commits
Am 27.04.2016 um 22:56 schrieb Junio C Hamano: So being able to stop at only commits on the first-parent chain is a valid and useful tool. "git bisect --first-parent" is one of the things that are sometimes asked for. With origin pointing to git.git, I attempted this: git bisect start git rev-list --first-parent --boundary origin..origin/pu | sed -ne s/-//p | xargs git bisect good git bisect bad origin/pu and it starts bisecting among the 50-something first-parent commits between origin and origin/pu. -- Hannes -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] config.c: drop local variable
As `ret` is not used for anything except determining an early return, we don't need a variable for that. Drop it. Signed-off-by: Stefan Beller --- While reading config code, I found 2 nits. Both improve readability, no bugfix or feature. As it is generally discouraged to have cleanup patch churn, I checked master..pu to see any possible topics in flight conflicting on config.c or submodule-config.c and there is nothing close by. Thanks, Stefan config.c | 5 + 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/config.c b/config.c index 10b5c95..262d8d7 100644 --- a/config.c +++ b/config.c @@ -1309,14 +1309,11 @@ static struct config_set_element *configset_find_element(struct config_set *cs, struct config_set_element k; struct config_set_element *found_entry; char *normalized_key; - int ret; /* * `key` may come from the user, so normalize it before using it * for querying entries from the hashmap. */ - ret = git_config_parse_key(key, &normalized_key, NULL); - - if (ret) + if (git_config_parse_key(key, &normalized_key, NULL)) return NULL; hashmap_entry_init(&k, strhash(normalized_key)); -- 2.8.0.41.g0ac0153.dirty -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] submodule-config: don't shadow `cache`
Lots of internal functions in submodule-confic.c have a first parameter
`struct submodule_cache *cache`, which currently always refers to the
global variable `cache` in the file. To avoid confusion rename the
global `cache` variable.
Signed-off-by: Stefan Beller
---
submodule-config.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/submodule-config.c b/submodule-config.c
index 8ac5031..debab29 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -30,7 +30,7 @@ enum lookup_type {
lookup_path
};
-static struct submodule_cache cache;
+static struct submodule_cache the_submodule_cache;
static int is_cache_init;
static int config_path_cmp(const struct submodule_entry *a,
@@ -470,14 +470,14 @@ static void ensure_cache_init(void)
if (is_cache_init)
return;
- cache_init(&cache);
+ cache_init(&the_submodule_cache);
is_cache_init = 1;
}
int parse_submodule_config_option(const char *var, const char *value)
{
struct parse_config_parameter parameter;
- parameter.cache = &cache;
+ parameter.cache = &the_submodule_cache;
parameter.commit_sha1 = NULL;
parameter.gitmodules_sha1 = null_sha1;
parameter.overwrite = 1;
@@ -490,18 +490,18 @@ const struct submodule *submodule_from_name(const
unsigned char *commit_sha1,
const char *name)
{
ensure_cache_init();
- return config_from_name(&cache, commit_sha1, name);
+ return config_from_name(&the_submodule_cache, commit_sha1, name);
}
const struct submodule *submodule_from_path(const unsigned char *commit_sha1,
const char *path)
{
ensure_cache_init();
- return config_from_path(&cache, commit_sha1, path);
+ return config_from_path(&the_submodule_cache, commit_sha1, path);
}
void submodule_free(void)
{
- cache_free(&cache);
+ cache_free(&the_submodule_cache);
is_cache_init = 0;
}
--
2.8.0.41.g0ac0153.dirty
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/15] string_list: add string_list_duplicate
On Wed, Apr 27, 2016 at 2:11 PM, Junio C Hamano wrote:
> Stefan Beller writes:
>
>> When not duplicating git_config_get_value_multi("submodule.defaultGroup");
>> I run into
>>
>> Program received signal SIGSEGV, Segmentation fault.
>> ...
>> So the string list seems to be corrupted here.
>> Someone stomping over our memory? How long is the result
>> of git_config_get_value_multi deemed to be stable and usable?
>
> That call goes to
>
> git_config_get_value_multi()
> -> git_configset_get_value_multi()
>-> configset_find_element()
>
> the returned value from there would be either NULL or the list of
> values that belong to the config cache layer, i.e. a caller of the
> API can peek but is not allowed to modify it.
>
> So if you are modifying the value you obtain from the API, you must
> make a copy; otherwise you will "stomp over" memory that belongs to
> the config cache layer, not to you.
Yes, we do not modify the string_list (the return of git_config_get_value_multi)
Another way to corrupt it is to change the configuration (e.g. add
things to the config hashmap such that it reallocates and grows).
The problem arises after a call to submodule_from_path(...);
which may change the cache for submodules. I assumed that was
completely different from the regular config cache, but somehow there is
a relation, which I have not tracked down yet.
Thanks,
Stefan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 15/29] ref_transaction_create(): disallow recursive pruning
Junio C Hamano writes: > If a casual reader sees this code: > > ref_transaction_delete(transaction, r->name, r->sha1, > REF_ISPRUNING | REF_NODEREF, NULL, &err) > > it gives an incorrect impression that there may also be a valid case > to make a "delete" call with ISPRUNING alone without NODEREF, in > other codepaths and under certain conditions, and write an incorrect > > ref_transaction_delete(transaction, refname, sha1, > REF_ISPRUNING, NULL, &err) > > in her new code. Or a careless programmer and reviewer may not even > memorize and remember what the new world order is when they see such > a code and let it pass. > > As I understand that we declare that "to prune a ref from set of > loose refs is to prune the named one, never following a symbolic > ref" is the new world order with this patch, making sure that > ISPRUNING automatically and always mean NODEREF will eliminate the > possibility that any new code makes an incorrect call to "delete", > which I think is much better. ... but my understanding of the point of this patch may be flawed, in which case I of course am willing to be enlightened ;-) -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/15] string_list: add string_list_duplicate
Stefan Beller writes:
> When not duplicating git_config_get_value_multi("submodule.defaultGroup");
> I run into
>
> Program received signal SIGSEGV, Segmentation fault.
> ...
> So the string list seems to be corrupted here.
> Someone stomping over our memory? How long is the result
> of git_config_get_value_multi deemed to be stable and usable?
That call goes to
git_config_get_value_multi()
-> git_configset_get_value_multi()
-> configset_find_element()
the returned value from there would be either NULL or the list of
values that belong to the config cache layer, i.e. a caller of the
API can peek but is not allowed to modify it.
So if you are modifying the value you obtain from the API, you must
make a copy; otherwise you will "stomp over" memory that belongs to
the config cache layer, not to you.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4] http: support sending custom HTTP headers
Jeff King writes: > On Wed, Apr 27, 2016 at 02:20:37PM +0200, Johannes Schindelin wrote: > >> The only change vs v3 is that I replaced my flimsical test by Peff's (with >> *one* change: I realized that we need to group the Require statements in a >> block when I tried to verify that the test fails when I >> modify the first header). > > Whoops, I didn't actually test that case. Thanks for catching (as you > might guess, I wanted to make sure we handle multiple values correctly). > >> Documentation/config.txt| 6 ++ >> http-push.c | 10 +- >> http.c | 35 --- >> http.h | 1 + >> remote-curl.c | 4 ++-- >> t/lib-httpd/apache.conf | 8 >> t/t5551-http-fetch-smart.sh | 7 +++ >> 7 files changed, 61 insertions(+), 10 deletions(-) > > This version looks good to me. > > -Peff Thanks, both. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Bisect limited to Merge Commits
Hagen Paul Pfeifer writes: > Imagine a "rebase feature branch" style of development. All features are > developed on separate features branch which are rebased on master and > immediately merged into the upstream master. I do not want to imagine such ;-) The only semi-sensible reason why people might want to always rebase on top of 'master' is to keep the history completely linear, so with such a workflow, there won't be a merge commit. But I think "rebase" part of your description is a red herring. If a development goes by always doing a new feature in a side branch and then merge the branch only after it is done to the 'master' branch, then bisecting only the commits on first-parent chain would often be a quick first-pass to find the topic whose merge into the 'master' branch introduced a breakage. From there you can dig down to each individual commit on the side branch. And for that, it is immaterial that the side branch gets rebased on 'master' and forced to become a merge with "--no-ff", of the side branch was developed on older upstream but in a careless way full of "oops, previous one I broke the entire world and it does not even compile; this commit fixes it" commits. So being able to stop at only commits on the first-parent chain is a valid and useful tool. "git bisect --first-parent" is one of the things that are sometimes asked for. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: BUG on OSX `git p4 submit` can fail when the workspace root doesn't exist locally.
On Wed, Apr 27, 2016 at 11:06 AM, Jacob Smith wrote: > I debugged the issue using the script here: > https://github.com/git/git/blob/master/git-p4.py > I'm not sure how close to the main repo that is. > > On Wed, Apr 27, 2016 at 11:28 AM, Stefan Beller wrote: >> On Wed, Apr 27, 2016 at 9:15 AM, Jacob Smith wrote: >>> On OS X, >> >> Do you use the Git as provided from OS X? In that case you better report the >> bug >> to Apple, as their version of Git is slightly different (not close on >> upstream, by both >> having additional patches as well as not following the upstream closely >> IIUC). In that case, I have cc'd Luke and Lars, who work on p4 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Bisect limited to Merge Commits
Hey, are there any plans to add an option to skip all non-merge commits via bisecting? git bisect start --merges-only Imagine a "rebase feature branch" style of development. All features are developed on separate features branch which are rebased on master and immediately merged into the upstream master. Now think about lazy programmers who miss to check the build process at each commit - leading to many unbuildable commits. The only guaranteed buildable commit is often the merge commit. Bisecting only merge commits will reduce the amount of skip commands (think about many unbuildable commits) and reduce the number of tests. Any comments on this idea? What about extending bisect.c:do_find_bisection() hgn -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 08/29] ref_transaction_commit(): remove local variable n
Junio C Hamano writes: > I expect that somewhere in this series transaction->nr will not stay s/series/& it is documented that/ > constant even if the client code of ref-transaction API makes no > direct call that adds a new update[] element, though, even if it is > not done in this patch. > > Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 15/29] ref_transaction_create(): disallow recursive pruning
David Turner writes: > On Wed, 2016-04-27 at 11:47 -0700, Junio C Hamano wrote: >> Michael Haggerty writes: >> >> > It is nonsensical (and a little bit dangerous) to use REF_ISPRUNING >> > without REF_NODEREF. Forbid it explicitly. Change the one >> > REF_ISPRUNING >> > caller to pass REF_NODEREF too. >> > >> > Signed-off-by: Michael Haggerty >> > --- >> > This also makes later patches a bit clearer. >> >> I wonder if it is more future-proof to solve this by doing >> >> -#define REF_ISPRUNING 0x04 >> +#define REF_ISPRUNING (0x04 | REF_NODEREF) >> >> instead. It makes the intention clear that pruning is always about >> the single level (i.e. no-deref). > > I think the approach in Michael's patch might be clearer than yours, > since someone reading the code doesn't have to look at the definition > of REF_ISPRUNING to understand what is going on. I have to strongly disagree, assuming that my understanding of the point of this patch correctly. If a casual reader sees this code: ref_transaction_delete(transaction, r->name, r->sha1, REF_ISPRUNING | REF_NODEREF, NULL, &err) it gives an incorrect impression that there may also be a valid case to make a "delete" call with ISPRUNING alone without NODEREF, in other codepaths and under certain conditions, and write an incorrect ref_transaction_delete(transaction, refname, sha1, REF_ISPRUNING, NULL, &err) in her new code. Or a careless programmer and reviewer may not even memorize and remember what the new world order is when they see such a code and let it pass. As I understand that we declare that "to prune a ref from set of loose refs is to prune the named one, never following a symbolic ref" is the new world order with this patch, making sure that ISPRUNING automatically and always mean NODEREF will eliminate the possibility that any new code makes an incorrect call to "delete", which I think is much better. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 05/29] refname_is_safe(): insist that the refname already be normalized
On Wed, Apr 27, 2016 at 04:34:53PM -0400, David Turner wrote: > > I thought the point is that one is a lesser check than the other, and > > we > > need different rules for different situations. So we might allow > > deletion on a refname that does not pass check_refname_format(), but > > we > > must make sure it is not going to cause any mischief (e.g., escaping > > ".git" and deleting random files). > > > > But anything writing a _new_ refname (whether the actual ref, or > > referencing it via a symref) should be using check_refname_format() > > before writing. > > Unfortunately, neither check is lesser -- refname_is_safe allows > refs/heads//foo but not a/b while check_refname_format allows a/b but > not refs/heads//foo. So sometimes we need both, while other times we > just need one :( IMHO, that sounds like a bug. check_refname_format() should conceptually[1] be a superset of refname_is_safe(). Is there a case where we would want to _allow_ a refname that is not safe to look at on disk? -Peff [1] The implementation can be a direct call, or can simply implement a superset of the rules, if that's more efficient. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 05/29] refname_is_safe(): insist that the refname already be normalized
On Wed, 2016-04-27 at 16:15 -0400, Jeff King wrote: > On Wed, Apr 27, 2016 at 04:10:32PM -0400, David Turner wrote: > > > On Wed, 2016-04-27 at 10:59 -0700, Junio C Hamano wrote: > > > > > There is another call to refname_is_safe() in > > > resolve_ref_unsafe(), > > > which applies the sanity check to the string from a symref. We > > > seem > > > to allow > > > > > > $ git symbolic-ref refs/heads/SSS refs/heads//master > > > > > > and we end up storing "ref: refs/heads//master" (or make a > > > symbolic > > > link with doubled slashes), but the current code considers the > > > resulting symbolic link as "dangling". Again, this change moves > > > the > > > rejection a bit earlier in the codepath, without changing the end > > > result, I'd think. > > > > I think we should disallow that -- refname_is_safe should probably > > call > > (or be replaced with calls to) check_refname_format. > > I thought the point is that one is a lesser check than the other, and > we > need different rules for different situations. So we might allow > deletion on a refname that does not pass check_refname_format(), but > we > must make sure it is not going to cause any mischief (e.g., escaping > ".git" and deleting random files). > > But anything writing a _new_ refname (whether the actual ref, or > referencing it via a symref) should be using check_refname_format() > before writing. Unfortunately, neither check is lesser -- refname_is_safe allows refs/heads//foo but not a/b while check_refname_format allows a/b but not refs/heads//foo. So sometimes we need both, while other times we just need one :( -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] trailer: load config to handle core.commentChar
On Wed, Apr 27, 2016 at 4:31 PM, Junio C Hamano wrote: >> On Wed, Apr 27, 2016 at 9:24 PM, Rafal Klys wrote: >>> Add call to git_config(git_default_config, NULL) to update the >>> comment_char_line from default '#' to possible different value set in >>> core.commentChar. >> >>> Signed-off-by: Rafal Klys >>> --- >>> trailer.c | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/trailer.c b/trailer.c >>> index 8e48a5c..a3700b4 100644 >>> --- a/trailer.c >>> +++ b/trailer.c >>> @@ -888,6 +888,9 @@ void process_trailers(const char *file, int in_place, >>> int trim_empty, struct str >>> git_config(git_trailer_default_config, NULL); >>> git_config(git_trailer_config, NULL); >>> >>> + /* for core.commentChar */ >>> + git_config(git_default_config, NULL); >>> + > > I can sort-of see why the original (logically) reads the > configuration files twice by making two separate calls to > git_config(), but I do not think we should add a third round like > this patch does. Shouldn't git-trailer-default-config have a > fallthru call to git-default-config instead? Thanks, I was going to say the same thing. Also, would it be possible to add a test? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] trailer: load config to handle core.commentChar
Christian Couder writes: > On Wed, Apr 27, 2016 at 9:24 PM, Rafal Klys wrote: >> Add call to git_config(git_default_config, NULL) to update the >> comment_char_line from default '#' to possible different value set in >> core.commentChar. > > It is "comment_line_char" not "comment_char_line", but otherwise you > can add "Reviewed-by: Christian Couder ". > > Thanks! > >> Signed-off-by: Rafal Klys >> --- >> trailer.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/trailer.c b/trailer.c >> index 8e48a5c..a3700b4 100644 >> --- a/trailer.c >> +++ b/trailer.c >> @@ -888,6 +888,9 @@ void process_trailers(const char *file, int in_place, >> int trim_empty, struct str >> git_config(git_trailer_default_config, NULL); >> git_config(git_trailer_config, NULL); >> >> + /* for core.commentChar */ >> + git_config(git_default_config, NULL); >> + I can sort-of see why the original (logically) reads the configuration files twice by making two separate calls to git_config(), but I do not think we should add a third round like this patch does. Shouldn't git-trailer-default-config have a fallthru call to git-default-config instead? >> lines = read_input_file(file); >> >> if (in_place) >> -- >> 2.8.1.68.g625efa9.dirty >> >> -- >> To unsubscribe from this list: send the line "unsubscribe git" in >> the body of a message to majord...@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Makefile: remove dependency on git.spec
Dennis Kaarsemaker writes:
> ab21433 dropped support for rpmbuild using our own specfile by removing
> git.spec.in, but forgot to remove the dependency of dist on git.spec.
>
> Signed-off-by: Dennis Kaarsemaker
> ---
Thanks.
> Makefile | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Makefile b/Makefile
> index 23182bc..8083b10 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2396,7 +2396,7 @@ quick-install-html:
> ### Maintainer's dist rules
>
> GIT_TARNAME = git-$(GIT_VERSION)
> -dist: git.spec git-archive$(X) configure
> +dist: git-archive$(X) configure
> ./git-archive --format=tar \
> --prefix=$(GIT_TARNAME)/ HEAD^{tree} > $(GIT_TARNAME).tar
> @mkdir -p $(GIT_TARNAME)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 15/29] ref_transaction_create(): disallow recursive pruning
On Wed, 2016-04-27 at 11:47 -0700, Junio C Hamano wrote: > Michael Haggerty writes: > > > It is nonsensical (and a little bit dangerous) to use REF_ISPRUNING > > without REF_NODEREF. Forbid it explicitly. Change the one > > REF_ISPRUNING > > caller to pass REF_NODEREF too. > > > > Signed-off-by: Michael Haggerty > > --- > > This also makes later patches a bit clearer. > > I wonder if it is more future-proof to solve this by doing > > -#define REF_ISPRUNING0x04 > +#define REF_ISPRUNING(0x04 | REF_NODEREF) > > instead. It makes the intention clear that pruning is always about > the single level (i.e. no-deref). I think the approach in Michael's patch might be clearer than yours, since someone reading the code doesn't have to look at the definition of REF_ISPRUNING to understand what is going on. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 05/29] refname_is_safe(): insist that the refname already be normalized
On Wed, Apr 27, 2016 at 04:10:32PM -0400, David Turner wrote: > On Wed, 2016-04-27 at 10:59 -0700, Junio C Hamano wrote: > > > There is another call to refname_is_safe() in resolve_ref_unsafe(), > > which applies the sanity check to the string from a symref. We seem > > to allow > > > > $ git symbolic-ref refs/heads/SSS refs/heads//master > > > > and we end up storing "ref: refs/heads//master" (or make a symbolic > > link with doubled slashes), but the current code considers the > > resulting symbolic link as "dangling". Again, this change moves the > > rejection a bit earlier in the codepath, without changing the end > > result, I'd think. > > I think we should disallow that -- refname_is_safe should probably call > (or be replaced with calls to) check_refname_format. I thought the point is that one is a lesser check than the other, and we need different rules for different situations. So we might allow deletion on a refname that does not pass check_refname_format(), but we must make sure it is not going to cause any mischief (e.g., escaping ".git" and deleting random files). But anything writing a _new_ refname (whether the actual ref, or referencing it via a symref) should be using check_refname_format() before writing. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 24/29] ref_transaction_update(): check refname_is_safe() at a minimum
Michael Haggerty writes:
> If the user has asked that a new value be set for a reference, we use
> check_refname_format() to verify that the reference name satisfies all
> of the rules. But in other cases, at least check that refname_is_safe().
It isn't clear to me what "in other cases" exactly refers to. A
request to delete a ref would obviously one of those that do not
"ask that a new value be set", but are there other cases?
> Signed-off-by: Michael Haggerty
> ---
> There are remaining problems in this area of the code. For example,
> check_refname_format() is *less* strict than refname_is_safe(). It
> allows almost arbitrary top-level reference names like "foo" and
> "refs". (The latter is especially fun because if you manage to create
> a reference called "refs", Git stops recognizing the directory as a
> Git repository.) On the other hand, some callers call
> check_refname_format() with incomplete reference names (e.g., branch
> names like "master"), so the functions can't be made stricter until
> those callers are changed.
>
> I'll address these problems separately if I find the time.
Thanks.
> refs.c | 5 +++--
> t/t1400-update-ref.sh | 2 +-
> t/t1430-bad-ref-name.sh | 2 +-
> 3 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index 858b6d7..41eb9e2 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -802,8 +802,9 @@ int ref_transaction_update(struct ref_transaction
> *transaction,
> if ((flags & REF_ISPRUNING) && !(flags & REF_NODEREF))
> die("BUG: REF_ISPRUNING set without REF_NODEREF");
>
> - if (new_sha1 && !is_null_sha1(new_sha1) &&
> - check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
> + if ((new_sha1 && !is_null_sha1(new_sha1)) ?
> + check_refname_format(refname, REFNAME_ALLOW_ONELEVEL) :
> + !refname_is_safe(refname)) {
> strbuf_addf(err, "refusing to update ref with bad name '%s'",
> refname);
> return -1;
> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
> index 40b0cce..08bd8fd 100755
> --- a/t/t1400-update-ref.sh
> +++ b/t/t1400-update-ref.sh
> @@ -23,7 +23,7 @@ test_expect_success setup '
> m=refs/heads/master
> n_dir=refs/heads/gu
> n=$n_dir/fixes
> -outside=foo
> +outside=refs/foo
>
> test_expect_success \
> "create $m" \
> diff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh
> index 25ddab4..8937e25 100755
> --- a/t/t1430-bad-ref-name.sh
> +++ b/t/t1430-bad-ref-name.sh
> @@ -285,7 +285,7 @@ test_expect_success 'update-ref -d cannot delete non-ref
> in .git dir' '
> echo precious >expect &&
> test_must_fail git update-ref -d my-private-file >output 2>error &&
> test_must_be_empty output &&
> - test_i18ngrep -e "cannot lock .*: unable to resolve reference" error &&
> + test_i18ngrep -e "refusing to update ref with bad name" error &&
> test_cmp expect .git/my-private-file
> '
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] trailer: load config to handle core.commentChar
On Wed, Apr 27, 2016 at 9:24 PM, Rafal Klys wrote: > Add call to git_config(git_default_config, NULL) to update the > comment_char_line from default '#' to possible different value set in > core.commentChar. It is "comment_line_char" not "comment_char_line", but otherwise you can add "Reviewed-by: Christian Couder ". Thanks! > Signed-off-by: Rafal Klys > --- > trailer.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/trailer.c b/trailer.c > index 8e48a5c..a3700b4 100644 > --- a/trailer.c > +++ b/trailer.c > @@ -888,6 +888,9 @@ void process_trailers(const char *file, int in_place, int > trim_empty, struct str > git_config(git_trailer_default_config, NULL); > git_config(git_trailer_config, NULL); > > + /* for core.commentChar */ > + git_config(git_default_config, NULL); > + > lines = read_input_file(file); > > if (in_place) > -- > 2.8.1.68.g625efa9.dirty > > -- > To unsubscribe from this list: send the line "unsubscribe git" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 05/29] refname_is_safe(): insist that the refname already be normalized
On Wed, 2016-04-27 at 10:59 -0700, Junio C Hamano wrote: > There is another call to refname_is_safe() in resolve_ref_unsafe(), > which applies the sanity check to the string from a symref. We seem > to allow > > $ git symbolic-ref refs/heads/SSS refs/heads//master > > and we end up storing "ref: refs/heads//master" (or make a symbolic > link with doubled slashes), but the current code considers the > resulting symbolic link as "dangling". Again, this change moves the > rejection a bit earlier in the codepath, without changing the end > result, I'd think. I think we should disallow that -- refname_is_safe should probably call (or be replaced with calls to) check_refname_format. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v6 00/19] index-helper/watchman
What's new: 1. configs for automatically populating the untracked-cache and watchman extensions. 2. index-helper errors go to index-helper.log (index-helper redirects stdout, stderr) 3. Duy's "Add tracing to measure where most of the time is spent" 4. index-helper sends/receives watchman status (OK/NAK) 5. use packet_read infrastructure for communication with index-helper. 6. rearranged a tiny bit of code so two instances match up I decided not to do the weird thing where we pass fds over the socket, because the mailing list consensus was that it was maybe too weird. David Turner (8): index-helper: log warnings unpack-trees: preserve index extensions watchman: add a config option to enable the extension index-helper: kill mode index-helper: don't run if already running index-helper: autorun mode index-helper: optionally automatically run untracked-cache: config option Nguyễn Thái Ngọc Duy (11): read-cache.c: fix constness of verify_hdr() read-cache: allow to keep mmap'd memory after reading index-helper: new daemon for caching index and related stuff index-helper: add --strict daemonize(): set a flag before exiting the main process index-helper: add --detach read-cache: add watchman 'WAMA' extension Add watchman support to reduce index refresh cost index-helper: use watchman to avoid refreshing index with lstat() update-index: enable/disable watchman support Add tracing to measure where most of the time is spent .gitignore | 2 + Documentation/config.txt | 12 + Documentation/git-index-helper.txt | 78 + Documentation/git-update-index.txt | 6 + Documentation/technical/index-format.txt | 22 ++ Makefile | 18 ++ builtin/gc.c | 2 +- builtin/update-index.c | 16 + cache.h | 16 +- config.c | 5 + configure.ac | 8 + daemon.c | 2 +- diff-lib.c | 4 + dir.c| 25 +- dir.h| 6 + environment.c| 3 + git-compat-util.h| 1 + index-helper.c | 511 + name-hash.c | 2 + preload-index.c | 2 + read-cache.c | 533 ++- refs/files-backend.c | 2 + setup.c | 4 +- t/t1701-watchman-extension.sh| 37 +++ t/t7063-status-untracked-cache.sh| 22 ++ t/t7900-index-helper.sh | 68 t/test-lib-functions.sh | 4 + test-dump-watchman.c | 16 + unpack-trees.c | 1 + watchman-support.c | 135 watchman-support.h | 7 + 31 files changed, 1549 insertions(+), 21 deletions(-) create mode 100644 Documentation/git-index-helper.txt create mode 100644 index-helper.c create mode 100755 t/t1701-watchman-extension.sh create mode 100755 t/t7900-index-helper.sh create mode 100644 test-dump-watchman.c create mode 100644 watchman-support.c create mode 100644 watchman-support.h -- 2.4.2.767.g62658d5-twtrsrc -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v6 05/19] index-helper: log warnings
Instead of writing warnings to stderr, write them to a log. Later, we'll
probably be daemonized, so writing to stderr will be pointless.
Signed-off-by: David Turner
---
Documentation/git-index-helper.txt | 3 +++
index-helper.c | 29 ++---
2 files changed, 29 insertions(+), 3 deletions(-)
diff --git a/Documentation/git-index-helper.txt
b/Documentation/git-index-helper.txt
index a5c058f..4eb3d95 100644
--- a/Documentation/git-index-helper.txt
+++ b/Documentation/git-index-helper.txt
@@ -51,6 +51,9 @@ command. The following commands are used to control the
daemon:
All commands and replies are terminated by a 0 byte.
+In the event of an error, messages may be written to
+$GIT_DIR/index-helper.log.
+
GIT
---
Part of the linkgit:git[1] suite
diff --git a/index-helper.c b/index-helper.c
index 5cadd0d..7400d68 100644
--- a/index-helper.c
+++ b/index-helper.c
@@ -19,6 +19,29 @@ static struct shm shm_index;
static struct shm shm_base_index;
static int to_verify = 1;
+static void log_warning(const char *warning, ...)
+{
+ va_list ap;
+ FILE *fp;
+
+ fp = fopen(git_path("index-helper.log"), "a");
+ if (!fp)
+ /*
+* Probably nobody will see this, but it's the best
+* we can do.
+*/
+ die("Failed to open log for warnings");
+
+ fprintf(fp, "%"PRIuMAX"\t", (uintmax_t)time(NULL));
+
+ va_start(ap, warning);
+ vfprintf(fp, warning, ap);
+ va_end(ap);
+
+ fputc('\n', fp);
+ fclose(fp);
+}
+
static void release_index_shm(struct shm *is)
{
if (!is->shm)
@@ -132,7 +155,7 @@ static int verify_shm(void)
ce = istate.cache[i];
if (ce->ce_namelen != base->ce_namelen ||
strcmp(ce->name, base->name)) {
- warning("mismatch at entry %d", i);
+ log_warning("mismatch at entry %d", i);
break;
}
ce_flags = ce->ce_flags;
@@ -146,7 +169,7 @@ static int verify_shm(void)
ce->ce_flags = ce_flags;
base->ce_flags = base_flags;
if (ret) {
- warning("mismatch at entry %d", i);
+ log_warning("mismatch at entry %d", i);
break;
}
}
@@ -263,7 +286,7 @@ static void loop(int fd, int idle_in_seconds)
* alive, nothing to do.
*/
} else {
- warning("BUG: Bogus command %s", buf);
+ log_warning("BUG: Bogus command %s", buf);
}
} else {
/*
--
2.4.2.767.g62658d5-twtrsrc
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v6 08/19] read-cache: add watchman 'WAMA' extension
From: Nguyễn Thái Ngọc Duy
The extension contains a bitmap, one bit for each entry in the
index. If the n-th bit is zero, the n-th entry is considered
unchanged, we can ce_mark_uptodate() it without refreshing. If the bit
is non-zero and we found out the corresponding file is clean after
refresh, we can clear the bit.
In addition, there's a list of directories in the untracked-cache
to invalidate (because they have new or modified entries).
The 'skipping refresh' bit is not in this patch yet as we would need
watchman. More details in later patches.
Signed-off-by: Nguyễn Thái Ngọc Duy
Signed-off-by: David Turner
---
Documentation/technical/index-format.txt | 22 ++
cache.h | 4 ++
dir.h| 3 +
read-cache.c | 117 ++-
4 files changed, 144 insertions(+), 2 deletions(-)
diff --git a/Documentation/technical/index-format.txt
b/Documentation/technical/index-format.txt
index ade0b0c..86ed3a6 100644
--- a/Documentation/technical/index-format.txt
+++ b/Documentation/technical/index-format.txt
@@ -295,3 +295,25 @@ The remaining data of each directory block is grouped by
type:
in the previous ewah bitmap.
- One NUL.
+
+== Watchman cache
+
+ The watchman cache tracks files for which watchman has told us about
+ changes. The signature for this extension is { 'W', 'A', 'M', 'A' }.
+
+ The extension starts with
+
+ - A NUL-terminated string: the watchman vector clock at the last
+time we heard from watchman.
+
+ - 32-bit bitmap size: the size of the CE_WATCHMAN_DIRTY bitmap
+
+ - 32-bit untracked cache entry count: the number of dirty untracked
+cache entries
+
+ - An ewah bitmap, the n-th bit indicates whether the n-th index entry
+is CE_WATCHMAN_DIRTY.
+
+ - a list of N NUL-terminated strings. Each is a directory that should
+be marked dirty in the untracked cache because watchman has told us
+about an update to a file in it.
diff --git a/cache.h b/cache.h
index 0aeb994..f4f7eef 100644
--- a/cache.h
+++ b/cache.h
@@ -182,6 +182,8 @@ struct cache_entry {
#define CE_VALID (0x8000)
#define CE_STAGESHIFT 12
+#define CE_WATCHMAN_DIRTY (0x0001)
+
/*
* Range 0x0FFF in ce_flags is divided into
* two parts: in-memory flags and on-disk ones.
@@ -320,6 +322,7 @@ static inline unsigned int canon_mode(unsigned int mode)
#define CACHE_TREE_CHANGED (1 << 5)
#define SPLIT_INDEX_ORDERED(1 << 6)
#define UNTRACKED_CHANGED (1 << 7)
+#define WATCHMAN_CHANGED (1 << 8)
struct split_index;
struct untracked_cache;
@@ -344,6 +347,7 @@ struct index_state {
struct untracked_cache *untracked;
void *mmap;
size_t mmap_size;
+ char *last_update;
};
extern struct index_state the_index;
diff --git a/dir.h b/dir.h
index 3ec3fb0..3d540de 100644
--- a/dir.h
+++ b/dir.h
@@ -142,6 +142,9 @@ struct untracked_cache {
int gitignore_invalidated;
int dir_invalidated;
int dir_opened;
+ /* watchman invalidation data */
+ unsigned int use_watchman : 1;
+ struct string_list invalid_untracked;
};
struct dir_struct {
diff --git a/read-cache.c b/read-cache.c
index 9eedf03..512f9f0 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -20,6 +20,7 @@
#include "utf8.h"
#include "unix-socket.h"
#include "pkt-line.h"
+#include "ewah/ewok.h"
static struct cache_entry *refresh_cache_entry(struct cache_entry *ce,
unsigned int options);
@@ -42,11 +43,13 @@ static struct cache_entry *refresh_cache_entry(struct
cache_entry *ce,
#define CACHE_EXT_RESOLVE_UNDO 0x52455543 /* "REUC" */
#define CACHE_EXT_LINK 0x6c696e6b/* "link" */
#define CACHE_EXT_UNTRACKED 0x554E5452 /* "UNTR" */
+#define CACHE_EXT_WATCHMAN 0x57414D41/* "WAMA" */
/* changes that can be kept in $GIT_DIR/index (basically all extensions) */
#define EXTMASK (RESOLVE_UNDO_CHANGED | CACHE_TREE_CHANGED | \
CE_ENTRY_ADDED | CE_ENTRY_REMOVED | CE_ENTRY_CHANGED | \
-SPLIT_INDEX_ORDERED | UNTRACKED_CHANGED)
+SPLIT_INDEX_ORDERED | UNTRACKED_CHANGED | \
+WATCHMAN_CHANGED)
struct index_state the_index;
static const char *alternate_index_output;
@@ -1221,8 +1224,13 @@ int refresh_index(struct index_state *istate, unsigned
int flags,
continue;
new = refresh_cache_ent(istate, ce, options, &cache_errno,
&changed);
- if (new == ce)
+ if (new == ce) {
+ if (ce->ce_flags & CE_WATCHMAN_DIRTY) {
+ ce->ce_flags &= ~CE_WATCHMAN_DIRTY;
+ istate->cache_changed |= WATCHMAN_CHANGED;
+ }
continue;
+ }
if (!new) {
const char *fmt;
@@ -1366,6 +1374,9
[PATCH v6 06/19] daemonize(): set a flag before exiting the main process
From: Nguyễn Thái Ngọc Duy
This allows signal handlers and atexit functions to realize this
situation and not clean up.
Signed-off-by: Nguyễn Thái Ngọc Duy
Signed-off-by: David Turner
---
builtin/gc.c | 2 +-
cache.h | 2 +-
daemon.c | 2 +-
setup.c | 4 +++-
4 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/builtin/gc.c b/builtin/gc.c
index c583aad..37180de 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -385,7 +385,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
* failure to daemonize is ok, we'll continue
* in foreground
*/
- daemonized = !daemonize();
+ daemonized = !daemonize(NULL);
}
} else
add_repack_all_option();
diff --git a/cache.h b/cache.h
index 4b678e9..0aeb994 100644
--- a/cache.h
+++ b/cache.h
@@ -530,7 +530,7 @@ extern int set_git_dir_init(const char *git_dir, const char
*real_git_dir, int);
extern int init_db(const char *template_dir, unsigned int flags);
extern void sanitize_stdfds(void);
-extern int daemonize(void);
+extern int daemonize(int *);
#define alloc_nr(x) (((x)+16)*3/2)
diff --git a/daemon.c b/daemon.c
index 8d45c33..a5cf954 100644
--- a/daemon.c
+++ b/daemon.c
@@ -1365,7 +1365,7 @@ int main(int argc, char **argv)
return execute();
if (detach) {
- if (daemonize())
+ if (daemonize(NULL))
die("--detach not supported on this platform");
} else
sanitize_stdfds();
diff --git a/setup.c b/setup.c
index de1a2a7..9adf13f 100644
--- a/setup.c
+++ b/setup.c
@@ -1017,7 +1017,7 @@ void sanitize_stdfds(void)
close(fd);
}
-int daemonize(void)
+int daemonize(int *daemonized)
{
#ifdef NO_POSIX_GOODIES
errno = ENOSYS;
@@ -1029,6 +1029,8 @@ int daemonize(void)
case -1:
die_errno("fork failed");
default:
+ if (daemonized)
+ *daemonized = 1;
exit(0);
}
if (setsid() == -1)
--
2.4.2.767.g62658d5-twtrsrc
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v6 13/19] watchman: add a config option to enable the extension
For installations that have centrally-managed configuration, it's
easier to set a config once than to run update-index on every
repository.
Signed-off-by: David Turner
---
.gitignore| 1 +
Documentation/config.txt | 4
Makefile | 1 +
read-cache.c | 6 ++
t/t1701-watchman-extension.sh | 37 +
test-dump-watchman.c | 16
6 files changed, 65 insertions(+)
create mode 100755 t/t1701-watchman-extension.sh
create mode 100644 test-dump-watchman.c
diff --git a/.gitignore b/.gitignore
index b92f122..e6a5b2c 100644
--- a/.gitignore
+++ b/.gitignore
@@ -188,6 +188,7 @@
/test-dump-cache-tree
/test-dump-split-index
/test-dump-untracked-cache
+/test-dump-watchman
/test-fake-ssh
/test-scrap-cache-tree
/test-genrandom
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 2cd6bdd..15001ce 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1848,6 +1848,10 @@ imap::
The configuration variables in the 'imap' section are described
in linkgit:git-imap-send[1].
+index.addwatchmanextension::
+ Automatically add the watchman extension to the index whenever
+ it is written.
+
index.version::
Specify the version with which new index files should be
initialized. This does not affect existing repositories.
diff --git a/Makefile b/Makefile
index 65ab0f4..5f0a954 100644
--- a/Makefile
+++ b/Makefile
@@ -599,6 +599,7 @@ TEST_PROGRAMS_NEED_X += test-delta
TEST_PROGRAMS_NEED_X += test-dump-cache-tree
TEST_PROGRAMS_NEED_X += test-dump-split-index
TEST_PROGRAMS_NEED_X += test-dump-untracked-cache
+TEST_PROGRAMS_NEED_X += test-dump-watchman
TEST_PROGRAMS_NEED_X += test-fake-ssh
TEST_PROGRAMS_NEED_X += test-genrandom
TEST_PROGRAMS_NEED_X += test-hashmap
diff --git a/read-cache.c b/read-cache.c
index 470a27d..252299d 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2427,6 +2427,7 @@ static int do_write_index(struct index_state *istate, int
newfd,
int entries = istate->cache_nr;
struct stat st;
struct strbuf previous_name_buf = STRBUF_INIT, *previous_name;
+ int watchman = 0;
for (i = removed = extended = 0; i < entries; i++) {
if (cache[i]->ce_flags & CE_REMOVE)
@@ -2450,6 +2451,11 @@ static int do_write_index(struct index_state *istate,
int newfd,
if (istate->version == 3 || istate->version == 2)
istate->version = extended ? 3 : 2;
+ if (!git_config_get_bool("index.addwatchmanextension", &watchman) &&
+ watchman &&
+ !the_index.last_update)
+ the_index.last_update = xstrdup("");
+
hdr_version = istate->version;
hdr.hdr_signature = htonl(CACHE_SIGNATURE);
diff --git a/t/t1701-watchman-extension.sh b/t/t1701-watchman-extension.sh
new file mode 100755
index 000..71f1d46
--- /dev/null
+++ b/t/t1701-watchman-extension.sh
@@ -0,0 +1,37 @@
+#!/bin/sh
+
+test_description='watchman extension smoke tests'
+
+# These don't actually test watchman interaction -- just the
+# index extension
+
+. ./test-lib.sh
+
+test_expect_success 'enable watchman' '
+ test_commit a &&
+ test-dump-watchman .git/index >actual &&
+ echo "last_update: (null)" >expect &&
+ test_cmp expect actual &&
+ git update-index --watchman &&
+ test-dump-watchman .git/index >actual &&
+ echo "last_update: " >expect &&
+ test_cmp expect actual
+'
+
+test_expect_success 'disable watchman' '
+ git update-index --no-watchman &&
+ test-dump-watchman .git/index >actual &&
+ echo "last_update: (null)" >expect &&
+ test_cmp expect actual
+'
+
+test_expect_success 'auto-enable watchman' '
+ test_config index.addwatchmanextension true &&
+ test_commit c &&
+ test-dump-watchman .git/index >actual &&
+ echo "last_update: " >expect &&
+ test_cmp expect actual
+'
+
+
+test_done
diff --git a/test-dump-watchman.c b/test-dump-watchman.c
new file mode 100644
index 000..0314fa5
--- /dev/null
+++ b/test-dump-watchman.c
@@ -0,0 +1,16 @@
+#include "cache.h"
+#include "ewah/ewok.h"
+
+int main(int argc, char **argv)
+{
+ do_read_index(&the_index, argv[1], 1);
+ printf("last_update: %s\n", the_index.last_update ?
+ the_index.last_update : "(null)");
+
+ /*
+* For now, we just dump last_update, since it is not reasonable
+* to populate the extension itself in tests.
+*/
+
+ return 0;
+}
--
2.4.2.767.g62658d5-twtrsrc
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v6 17/19] index-helper: optionally automatically run
Introduce a new config option, indexhelper.autorun, to automatically
run git index-helper before starting up a builtin git command. This
enables users to keep index-helper running without manual
intervention.
Signed-off-by: David Turner
---
Documentation/config.txt | 4
read-cache.c | 37 +++--
t/t7900-index-helper.sh | 19 +++
3 files changed, 58 insertions(+), 2 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 15001ce..385ea66 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1856,6 +1856,10 @@ index.version::
Specify the version with which new index files should be
initialized. This does not affect existing repositories.
+indexhelper.autorun::
+ Automatically run git index-helper when any builtin git
+ command is run inside a repository.
+
init.templateDir::
Specify the directory from which templates will be copied.
(See the "TEMPLATE DIRECTORY" section of linkgit:git-init[1].)
diff --git a/read-cache.c b/read-cache.c
index 252299d..9329967 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -21,6 +21,7 @@
#include "unix-socket.h"
#include "pkt-line.h"
#include "ewah/ewok.h"
+#include "run-command.h"
static struct cache_entry *refresh_cache_entry(struct cache_entry *ce,
unsigned int options);
@@ -1723,6 +1724,33 @@ static void post_read_index_from(struct index_state
*istate)
tweak_untracked_cache(istate);
}
+static int want_auto_index_helper(void)
+{
+ int want = -1;
+ const char *value = NULL;
+ const char *key = "indexhelper.autorun";
+
+ if (git_config_key_is_valid(key) &&
+ !git_config_get_value(key, &value)) {
+ int b = git_config_maybe_bool(key, value);
+ want = b >= 0 ? b : 0;
+ }
+ return want;
+}
+
+static void autorun_index_helper(void)
+{
+ const char *argv[] = {"git-index-helper", "--detach", "--autorun",
NULL};
+ if (want_auto_index_helper() <= 0)
+ return;
+
+ trace_argv_printf(argv, "trace: auto index-helper:");
+
+ if (run_command_v_opt(argv,
+ RUN_SILENT_EXEC_FAILURE | RUN_CLEAN_ON_EXIT))
+ warning(_("You specified indexhelper.autorun, but running
git-index-helper failed."));
+}
+
static int poke_and_wait_for_reply(int fd)
{
struct strbuf buf = STRBUF_INIT;
@@ -1797,6 +1825,7 @@ static int poke_daemon(struct index_state *istate,
if (fd < 0) {
warning("Failed to connect to index-helper socket");
unlink(git_path("index-helper.sock"));
+ autorun_index_helper();
return -1;
}
@@ -1834,9 +1863,13 @@ static int try_shm(struct index_state *istate)
int fd = -1;
if (!is_main_index(istate) ||
- old_size <= 20 ||
- stat(git_path("index-helper.sock"), &st))
+ old_size <= 20)
return -1;
+
+ if (stat(git_path("index-helper.sock"), &st)) {
+ autorun_index_helper();
+ return -1;
+ }
if (poke_daemon(istate, &st, 0))
return -1;
sha1 = (unsigned char *)istate->mmap + old_size - 20;
diff --git a/t/t7900-index-helper.sh b/t/t7900-index-helper.sh
index aa6e5fc..2d3ce3c 100755
--- a/t/t7900-index-helper.sh
+++ b/t/t7900-index-helper.sh
@@ -16,6 +16,9 @@ test -n "$NO_MMAP" && {
}
test_expect_success 'index-helper smoke test' '
+ # We need an existing commit so that the index exists (otherwise,
+ # the index-helper will not be autostarted)
+ test_commit x &&
git index-helper --exit-after 1 &&
test_path_is_missing .git/index-helper.sock
'
@@ -46,4 +49,20 @@ test_expect_success 'index-helper is quiet with --autorun' '
git index-helper --autorun
'
+test_expect_success 'index-helper autorun works' '
+ rm -f .git/index-helper.sock &&
+ git status &&
+ test_path_is_missing .git/index-helper.sock &&
+ test_config indexhelper.autorun true &&
+ git status &&
+ test -S .git/index-helper.sock &&
+ git status 2>err &&
+ test -S .git/index-helper.sock &&
+ ! grep -q . err &&
+ git index-helper --kill &&
+ test_config indexhelper.autorun false &&
+ git status &&
+ test_path_is_missing .git/index-helper.sock
+'
+
test_done
--
2.4.2.767.g62658d5-twtrsrc
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v6 02/19] read-cache: allow to keep mmap'd memory after reading
From: Nguyễn Thái Ngọc Duy
Later, we will introduce git index-helper to share this memory with
other git processes.
We only unmap it when we discard the index (although the kernel may of
course choose to page it out).
Signed-off-by: Nguyễn Thái Ngọc Duy
Signed-off-by: David Turner
---
cache.h | 3 +++
read-cache.c | 13 -
2 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/cache.h b/cache.h
index b829410..4180e2b 100644
--- a/cache.h
+++ b/cache.h
@@ -333,11 +333,14 @@ struct index_state {
struct split_index *split_index;
struct cache_time timestamp;
unsigned name_hash_initialized : 1,
+keep_mmap : 1,
initialized : 1;
struct hashmap name_hash;
struct hashmap dir_hash;
unsigned char sha1[20];
struct untracked_cache *untracked;
+ void *mmap;
+ size_t mmap_size;
};
extern struct index_state the_index;
diff --git a/read-cache.c b/read-cache.c
index 16cc487..3cb0ec3 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1574,6 +1574,10 @@ int do_read_index(struct index_state *istate, const char
*path, int must_exist)
mmap = xmmap(NULL, mmap_size, PROT_READ, MAP_PRIVATE, fd, 0);
if (mmap == MAP_FAILED)
die_errno("unable to map index file");
+ if (istate->keep_mmap) {
+ istate->mmap = mmap;
+ istate->mmap_size = mmap_size;
+ }
close(fd);
hdr = mmap;
@@ -1626,11 +1630,13 @@ int do_read_index(struct index_state *istate, const
char *path, int must_exist)
src_offset += 8;
src_offset += extsize;
}
- munmap(mmap, mmap_size);
+ if (!istate->keep_mmap)
+ munmap(mmap, mmap_size);
return istate->cache_nr;
unmap:
munmap(mmap, mmap_size);
+ istate->mmap = NULL;
die("index file corrupt");
}
@@ -1655,6 +1661,7 @@ int read_index_from(struct index_state *istate, const
char *path)
discard_index(split_index->base);
else
split_index->base = xcalloc(1, sizeof(*split_index->base));
+ split_index->base->keep_mmap = istate->keep_mmap;
ret = do_read_index(split_index->base,
git_path("sharedindex.%s",
sha1_to_hex(split_index->base_sha1)), 1);
@@ -1698,6 +1705,10 @@ int discard_index(struct index_state *istate)
free(istate->cache);
istate->cache = NULL;
istate->cache_alloc = 0;
+ if (istate->keep_mmap && istate->mmap) {
+ munmap(istate->mmap, istate->mmap_size);
+ istate->mmap = NULL;
+ }
discard_split_index(istate);
free_untracked_cache(istate->untracked);
istate->untracked = NULL;
--
2.4.2.767.g62658d5-twtrsrc
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v6 11/19] update-index: enable/disable watchman support
From: Nguyễn Thái Ngọc Duy
Signed-off-by: Nguyễn Thái Ngọc Duy
Signed-off-by: David Turner
---
Documentation/git-index-helper.txt | 3 +++
Documentation/git-update-index.txt | 6 ++
builtin/update-index.c | 16
3 files changed, 25 insertions(+)
diff --git a/Documentation/git-index-helper.txt
b/Documentation/git-index-helper.txt
index 647b796..d8b8efb 100644
--- a/Documentation/git-index-helper.txt
+++ b/Documentation/git-index-helper.txt
@@ -15,6 +15,9 @@ DESCRIPTION
Keep the index file in memory for faster access. This daemon is per
repository.
+If you want the index-helper to accelerate untracked file checking,
+run git update-index --watchman before using it.
+
OPTIONS
---
diff --git a/Documentation/git-update-index.txt
b/Documentation/git-update-index.txt
index c6cbed1..6736487 100644
--- a/Documentation/git-update-index.txt
+++ b/Documentation/git-update-index.txt
@@ -19,6 +19,7 @@ SYNOPSIS
[--ignore-submodules]
[--[no-]split-index]
[--[no-|test-|force-]untracked-cache]
+[--[no-]watchman]
[--really-refresh] [--unresolve] [--again | -g]
[--info-only] [--index-info]
[-z] [--stdin] [--index-version ]
@@ -176,6 +177,11 @@ may not support it yet.
--no-untracked-cache::
Enable or disable untracked cache feature. Please use
`--test-untracked-cache` before enabling it.
+
+--watchman::
+--no-watchman::
+ Enable or disable watchman support. This is, at present,
+ only useful with git index-helper.
+
These options take effect whatever the value of the `core.untrackedCache`
configuration variable (see linkgit:git-config[1]). But a warning is
diff --git a/builtin/update-index.c b/builtin/update-index.c
index 1c94ca5..a3b4b5d 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -914,6 +914,7 @@ int cmd_update_index(int argc, const char **argv, const
char *prefix)
{
int newfd, entries, has_errors = 0, nul_term_line = 0;
enum uc_mode untracked_cache = UC_UNSPECIFIED;
+ int use_watchman = -1;
int read_from_stdin = 0;
int prefix_length = prefix ? strlen(prefix) : 0;
int preferred_index_format = 0;
@@ -1012,6 +1013,8 @@ int cmd_update_index(int argc, const char **argv, const
char *prefix)
N_("test if the filesystem supports untracked
cache"), UC_TEST),
OPT_SET_INT(0, "force-untracked-cache", &untracked_cache,
N_("enable untracked cache without testing the
filesystem"), UC_FORCE),
+ OPT_BOOL(0, "watchman", &use_watchman,
+ N_("use or not use watchman to reduce refresh cost")),
OPT_END()
};
@@ -1149,6 +1152,19 @@ int cmd_update_index(int argc, const char **argv, const
char *prefix)
die("Bug: bad untracked_cache value: %d", untracked_cache);
}
+ if (use_watchman > 0) {
+ the_index.last_update= xstrdup("");
+ the_index.cache_changed |= WATCHMAN_CHANGED;
+#ifndef USE_WATCHMAN
+ warning("git was built without watchman support -- I'm "
+ "adding the extension here, but it probably won't "
+ "do you any good.");
+#endif
+ } else if (!use_watchman) {
+ the_index.last_update= NULL;
+ the_index.cache_changed |= WATCHMAN_CHANGED;
+ }
+
if (active_cache_changed) {
if (newfd < 0) {
if (refresh_args.flags & REFRESH_QUIET)
--
2.4.2.767.g62658d5-twtrsrc
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v6 18/19] Add tracing to measure where most of the time is spent
From: Nguyễn Thái Ngọc Duy
All the known heavy code blocks are measured (except object database
access). This should help identify if an optimization is effective or
not. An unoptimized git-status would give something like below (92% of
time is accounted). To sum up the effort of making git scale better:
- read cache line is being addressed by index-helper
- preload/refresh index by watchman
- read packed refs by lmdb backend
- diff-index by rebuilding cache-tree
- read directory by untracked cache and watchman
- write index by split index
- name hash potientially by index-helper
read-cache.c:2075 performance: 0.004058570 s: read cache .../index
preload-index.c:104 performance: 0.004419864 s: preload index
read-cache.c:1265 performance: 0.000185224 s: refresh index
refs/files-backend.c:1100 performance: 0.001935788 s: read packed refs
diff-lib.c:240performance: 0.000144132 s: diff-files
diff-lib.c:506performance: 0.013592000 s: diff-index
name-hash.c:128 performance: 0.000614177 s: initialize name hash
dir.c:2030performance: 0.015814103 s: read directory
read-cache.c:2565 performance: 0.004052343 s: write index, changed mask
= 2
trace.c:420 performance: 0.048365509 s: git command: './git'
'status'
Signed-off-by: Nguyễn Thái Ngọc Duy
Signed-off-by: David Turner
---
diff-lib.c | 4
dir.c| 2 ++
name-hash.c | 2 ++
preload-index.c | 2 ++
read-cache.c | 11 +++
refs/files-backend.c | 2 ++
6 files changed, 23 insertions(+)
diff --git a/diff-lib.c b/diff-lib.c
index bc49c70..7af7f9a 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -90,6 +90,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
int diff_unmerged_stage = revs->max_count;
unsigned ce_option = ((option & DIFF_RACY_IS_MODIFIED)
? CE_MATCH_RACY_IS_DIRTY : 0);
+ uint64_t start = getnanotime();
diff_set_mnemonic_prefix(&revs->diffopt, "i/", "w/");
@@ -236,6 +237,7 @@ int run_diff_files(struct rev_info *revs, unsigned int
option)
}
diffcore_std(&revs->diffopt);
diff_flush(&revs->diffopt);
+ trace_performance_since(start, "diff-files");
return 0;
}
@@ -491,6 +493,7 @@ static int diff_cache(struct rev_info *revs,
int run_diff_index(struct rev_info *revs, int cached)
{
struct object_array_entry *ent;
+ uint64_t start = getnanotime();
ent = revs->pending.objects;
if (diff_cache(revs, ent->item->oid.hash, ent->name, cached))
@@ -500,6 +503,7 @@ int run_diff_index(struct rev_info *revs, int cached)
diffcore_fix_diff_index(&revs->diffopt);
diffcore_std(&revs->diffopt);
diff_flush(&revs->diffopt);
+ trace_performance_since(start, "diff-index");
return 0;
}
diff --git a/dir.c b/dir.c
index 5058b29..c56a8b9 100644
--- a/dir.c
+++ b/dir.c
@@ -2183,6 +2183,7 @@ int read_directory(struct dir_struct *dir, const char
*path, int len, const stru
{
struct path_simplify *simplify;
struct untracked_cache_dir *untracked;
+ uint64_t start = getnanotime();
/*
* Check out create_simplify()
@@ -2224,6 +2225,7 @@ int read_directory(struct dir_struct *dir, const char
*path, int len, const stru
free_simplify(simplify);
qsort(dir->entries, dir->nr, sizeof(struct dir_entry *), cmp_name);
qsort(dir->ignored, dir->ignored_nr, sizeof(struct dir_entry *),
cmp_name);
+ trace_performance_since(start, "read directory %.*s", len, path);
if (dir->untracked) {
static struct trace_key trace_untracked_stats =
TRACE_KEY_INIT(UNTRACKED_STATS);
trace_printf_key(&trace_untracked_stats,
diff --git a/name-hash.c b/name-hash.c
index 6d9f23e..b3966d8 100644
--- a/name-hash.c
+++ b/name-hash.c
@@ -115,6 +115,7 @@ static int cache_entry_cmp(const struct cache_entry *ce1,
static void lazy_init_name_hash(struct index_state *istate)
{
int nr;
+ uint64_t start = getnanotime();
if (istate->name_hash_initialized)
return;
@@ -124,6 +125,7 @@ static void lazy_init_name_hash(struct index_state *istate)
for (nr = 0; nr < istate->cache_nr; nr++)
hash_index_entry(istate, istate->cache[nr]);
istate->name_hash_initialized = 1;
+ trace_performance_since(start, "initialize name hash");
}
void add_name_hash(struct index_state *istate, struct cache_entry *ce)
diff --git a/preload-index.c b/preload-index.c
index c1fe3a3..7bb4809 100644
--- a/preload-index.c
+++ b/preload-index.c
@@ -72,6 +72,7 @@ static void preload_index(struct index_state *index,
{
int threads, i, work, offset;
struct thread_data data[MAX_PARALLEL];
+ uint64_t start = getnanotime();
if (!core_preload_index)
return;
@@ -100,6 +101,7 @@ static void p
[PATCH v6 19/19] untracked-cache: config option
Add a config option to populate the untracked cache.
For installations that have centrally-managed configuration, it's
easier to set a config once than to run update-index on every
repository.
Signed-off-by: David Turner
---
Documentation/config.txt | 4
read-cache.c | 7 ++-
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 385ea66..c7b76ef 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1848,6 +1848,10 @@ imap::
The configuration variables in the 'imap' section are described
in linkgit:git-imap-send[1].
+index.adduntrackedcache::
+ Automatically populate the untracked cache whenever the index
+ is written.
+
index.addwatchmanextension::
Automatically add the watchman extension to the index whenever
it is written.
diff --git a/read-cache.c b/read-cache.c
index e56a644..40869fe 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2469,7 +2469,7 @@ static int do_write_index(struct index_state *istate, int
newfd,
int entries = istate->cache_nr;
struct stat st;
struct strbuf previous_name_buf = STRBUF_INIT, *previous_name;
- int watchman = 0;
+ int watchman = 0, untracked = 0;
uint64_t start = getnanotime();
for (i = removed = extended = 0; i < entries; i++) {
@@ -2499,6 +2499,11 @@ static int do_write_index(struct index_state *istate,
int newfd,
!the_index.last_update)
the_index.last_update = xstrdup("");
+ if (!git_config_get_bool("index.adduntrackedcache", &untracked) &&
+ untracked &&
+ !istate->untracked)
+ add_untracked_cache(&the_index);
+
hdr_version = istate->version;
hdr.hdr_signature = htonl(CACHE_SIGNATURE);
--
2.4.2.767.g62658d5-twtrsrc
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v6 04/19] index-helper: add --strict
From: Nguyễn Thái Ngọc Duy
There are "holes" in the index-helper approach because the shared
memory is not verified again by git. If $USER is compromised, shared
memory could be modified. But anyone who could do this could already
modify $GIT_DIR/index. A more realistic risk is some bugs in
index-helper that produce corrupt shared memory. --strict is added to
avoid that.
Strictly speaking there's still a very small gap where corrupt shared
memory could still be read by git: after we write the trailing SHA-1 in
the shared memory (thus signaling "this shm is ready") and before
verify_shm() detects an error.
Signed-off-by: Nguyễn Thái Ngọc Duy
Signed-off-by: David Turner
---
Documentation/git-index-helper.txt | 9 +++
cache.h| 1 +
index-helper.c | 48 ++
read-cache.c | 9 ---
4 files changed, 64 insertions(+), 3 deletions(-)
diff --git a/Documentation/git-index-helper.txt
b/Documentation/git-index-helper.txt
index 77687c0..a5c058f 100644
--- a/Documentation/git-index-helper.txt
+++ b/Documentation/git-index-helper.txt
@@ -22,6 +22,15 @@ OPTIONS
Exit if the cached index is not accessed for ``
seconds. Specify 0 to wait forever. Default is 600.
+--strict::
+--no-strict::
+ Strict mode makes index-helper verify the shared memory after
+ it's created. If the result does not match what's read from
+ $GIT_DIR/index, the shared memory is destroyed. This makes
+ index-helper take more than double the amount of time required
+ for reading an index, but because it will happen in the
+ background, it's not noticable. `--strict` is enabled by default.
+
NOTES
-
diff --git a/cache.h b/cache.h
index 43fb314..4b678e9 100644
--- a/cache.h
+++ b/cache.h
@@ -336,6 +336,7 @@ struct index_state {
keep_mmap : 1,
from_shm : 1,
to_shm : 1,
+always_verify_trailing_sha1 : 1,
initialized : 1;
struct hashmap name_hash;
struct hashmap dir_hash;
diff --git a/index-helper.c b/index-helper.c
index 976e913..5cadd0d 100644
--- a/index-helper.c
+++ b/index-helper.c
@@ -17,6 +17,7 @@ struct shm {
static struct shm shm_index;
static struct shm shm_base_index;
+static int to_verify = 1;
static void release_index_shm(struct shm *is)
{
@@ -111,11 +112,56 @@ static void share_index(struct index_state *istate,
struct shm *is)
hashcpy((unsigned char *)new_mmap + istate->mmap_size - 20, is->sha1);
}
+static int verify_shm(void)
+{
+ int i;
+ struct index_state istate;
+ memset(&istate, 0, sizeof(istate));
+ istate.always_verify_trailing_sha1 = 1;
+ istate.to_shm = 1;
+ i = read_index(&istate);
+ if (i != the_index.cache_nr)
+ goto done;
+ for (; i < the_index.cache_nr; i++) {
+ struct cache_entry *base, *ce;
+ /* namelen is checked separately */
+ const unsigned int ondisk_flags =
+ CE_STAGEMASK | CE_VALID | CE_EXTENDED_FLAGS;
+ unsigned int ce_flags, base_flags, ret;
+ base = the_index.cache[i];
+ ce = istate.cache[i];
+ if (ce->ce_namelen != base->ce_namelen ||
+ strcmp(ce->name, base->name)) {
+ warning("mismatch at entry %d", i);
+ break;
+ }
+ ce_flags = ce->ce_flags;
+ base_flags = base->ce_flags;
+ /* only on-disk flags matter */
+ ce->ce_flags &= ondisk_flags;
+ base->ce_flags &= ondisk_flags;
+ ret = memcmp(&ce->ce_stat_data, &base->ce_stat_data,
+offsetof(struct cache_entry, name) -
+offsetof(struct cache_entry, ce_stat_data));
+ ce->ce_flags = ce_flags;
+ base->ce_flags = base_flags;
+ if (ret) {
+ warning("mismatch at entry %d", i);
+ break;
+ }
+ }
+done:
+ discard_index(&istate);
+ return i == the_index.cache_nr;
+}
+
static void share_the_index(void)
{
if (the_index.split_index && the_index.split_index->base)
share_index(the_index.split_index->base, &shm_base_index);
share_index(&the_index, &shm_index);
+ if (to_verify && !verify_shm())
+ cleanup_shm();
discard_index(&the_index);
}
@@ -255,6 +301,8 @@ int main(int argc, char **argv)
struct option options[] = {
OPT_INTEGER(0, "exit-after", &idle_in_seconds,
N_("exit if not used after some seconds")),
+ OPT_BOOL(0, "strict", &to_verify,
+"verify shared memory after creating"),
OPT_END()
};
diff --git
[PATCH v6 12/19] unpack-trees: preserve index extensions
Make git checkout (and other unpack_tree operations) preserve the
untracked cache and watchman status. This is valuable for two reasons:
1. Often, an unpack_tree operation will not touch large parts of the
working tree, and thus most of the untracked cache will continue to be
valid.
2. Even if the untracked cache were entirely invalidated by such an
operation, the user has signaled their intention to have such a cache,
and we don't want to throw it away.
The same logic applies to the watchman state.
Signed-off-by: David Turner
---
cache.h | 1 +
read-cache.c | 8
t/t7063-status-untracked-cache.sh | 22 ++
t/test-lib-functions.sh | 4
unpack-trees.c| 1 +
5 files changed, 36 insertions(+)
diff --git a/cache.h b/cache.h
index 4cc89bb..49fa128 100644
--- a/cache.h
+++ b/cache.h
@@ -571,6 +571,7 @@ extern void write_watchman_ext(struct strbuf *sb, struct
index_state* istate);
#define CLOSE_LOCK (1 << 1)
extern int write_locked_index(struct index_state *, struct lock_file *lock,
unsigned flags);
extern int discard_index(struct index_state *);
+extern void move_index_extensions(struct index_state *dst, struct index_state
*src);
extern int unmerged_index(const struct index_state *);
extern int verify_path(const char *path);
extern int index_dir_exists(struct index_state *istate, const char *name, int
namelen);
diff --git a/read-cache.c b/read-cache.c
index a57f605..470a27d 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2768,3 +2768,11 @@ void stat_validity_update(struct stat_validity *sv, int
fd)
fill_stat_data(sv->sd, &st);
}
}
+
+void move_index_extensions(struct index_state *dst, struct index_state *src)
+{
+ dst->untracked = src->untracked;
+ src->untracked = NULL;
+ dst->last_update = src->last_update;
+ src->last_update = NULL;
+}
diff --git a/t/t7063-status-untracked-cache.sh
b/t/t7063-status-untracked-cache.sh
index a971884..083516d 100755
--- a/t/t7063-status-untracked-cache.sh
+++ b/t/t7063-status-untracked-cache.sh
@@ -646,4 +646,26 @@ test_expect_success 'test ident field is working' '
test_cmp ../expect ../err
'
+test_expect_success 'untracked cache survives a checkout' '
+ git commit --allow-empty -m empty &&
+ test-dump-untracked-cache >../before &&
+ test_when_finished "git checkout master" &&
+ git checkout -b other_branch &&
+ test-dump-untracked-cache >../after &&
+ test_cmp ../before ../after &&
+ test_commit test &&
+ test-dump-untracked-cache >../before &&
+ git checkout master &&
+ test-dump-untracked-cache >../after &&
+ test_cmp ../before ../after
+'
+
+test_expect_success 'untracked cache survives a commit' '
+ test-dump-untracked-cache >../before &&
+ git add done/two &&
+ git commit -m commit &&
+ test-dump-untracked-cache >../after &&
+ test_cmp ../before ../after
+'
+
test_done
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 8d99eb3..e974b5b 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -186,6 +186,10 @@ test_commit () {
test_tick
fi &&
git commit $signoff -m "$1" &&
+ if [ "$(git config core.bare)" = false ]
+ then
+ git update-index --force-untracked-cache
+ fi
git tag "${4:-$1}"
}
diff --git a/unpack-trees.c b/unpack-trees.c
index 9f55cc2..fc90eb3 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1215,6 +1215,7 @@ int unpack_trees(unsigned len, struct tree_desc *t,
struct unpack_trees_options
WRITE_TREE_SILENT |
WRITE_TREE_REPAIR);
}
+ move_index_extensions(&o->result, o->dst_index);
discard_index(o->dst_index);
*o->dst_index = o->result;
} else {
--
2.4.2.767.g62658d5-twtrsrc
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v6 15/19] index-helper: don't run if already running
Signed-off-by: David Turner
---
index-helper.c | 7 +++
t/t7900-index-helper.sh | 9 +
2 files changed, 16 insertions(+)
diff --git a/index-helper.c b/index-helper.c
index 60a71f2..092c814 100644
--- a/index-helper.c
+++ b/index-helper.c
@@ -463,6 +463,13 @@ int main(int argc, char **argv)
return 0;
}
+ /* check that no other copy is running */
+ fd = unix_stream_connect(git_path("index-helper.sock"));
+ if (fd > 0)
+ die(_("Already running"));
+ if (errno != ECONNREFUSED && errno != ENOENT)
+ die_errno(_("Unexpected error checking socket"));
+
atexit(cleanup);
sigchain_push_common(cleanup_on_signal);
diff --git a/t/t7900-index-helper.sh b/t/t7900-index-helper.sh
index e71b5af..7159971 100755
--- a/t/t7900-index-helper.sh
+++ b/t/t7900-index-helper.sh
@@ -29,4 +29,13 @@ test_expect_success 'index-helper creates usable path file
and can be killed' '
test_path_is_missing .git/index-helper.sock
'
+test_expect_success 'index-helper will not start if already running' '
+ test_when_finished "git index-helper --kill" &&
+ git index-helper --detach &&
+ test -S .git/index-helper.sock &&
+ test_must_fail git index-helper 2>err &&
+ test -S .git/index-helper.sock &&
+ grep "Already running" err
+'
+
test_done
--
2.4.2.767.g62658d5-twtrsrc
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v6 16/19] index-helper: autorun mode
Soon, we'll want to automatically start index-helper, so we need
a mode that silently exits if it can't start up (either because
it's not in a git dir, or because another one is already running).
Signed-off-by: David Turner
---
Documentation/git-index-helper.txt | 4
index-helper.c | 29 +++--
t/t7900-index-helper.sh| 8
3 files changed, 35 insertions(+), 6 deletions(-)
diff --git a/Documentation/git-index-helper.txt
b/Documentation/git-index-helper.txt
index d68afd7..94766ec 100644
--- a/Documentation/git-index-helper.txt
+++ b/Documentation/git-index-helper.txt
@@ -40,6 +40,10 @@ OPTIONS
--kill::
Kill any running index-helper and clean up the socket
+--autorun::
+ If index-helper is not already running, start it. Else, do
+ nothing.
+
NOTES
-
diff --git a/index-helper.c b/index-helper.c
index 092c814..334ed4b 100644
--- a/index-helper.c
+++ b/index-helper.c
@@ -432,8 +432,9 @@ static void request_kill(void)
int main(int argc, char **argv)
{
const char *prefix;
- int idle_in_seconds = 600, detach = 0, kill = 0;
+ int idle_in_seconds = 600, detach = 0, kill = 0, autorun = 0;
int fd;
+ int nongit;
struct strbuf socket_path = STRBUF_INIT;
struct option options[] = {
OPT_INTEGER(0, "exit-after", &idle_in_seconds,
@@ -442,6 +443,7 @@ int main(int argc, char **argv)
"verify shared memory after creating"),
OPT_BOOL(0, "detach", &detach, "detach the process"),
OPT_BOOL(0, "kill", &kill, "request that existing index helper
processes exit"),
+ OPT_BOOL(0, "autorun", &autorun, "this is an automatic run of
git index-helper, so certain errors can be solved by silently exiting"),
OPT_END()
};
@@ -451,7 +453,14 @@ int main(int argc, char **argv)
if (argc == 2 && !strcmp(argv[1], "-h"))
usage_with_options(usage_text, options);
- prefix = setup_git_directory();
+ prefix = setup_git_directory_gently(&nongit);
+ if (nongit) {
+ if (autorun)
+ exit(0);
+ else
+ die(_("not a git repository"));
+ }
+
if (parse_options(argc, (const char **)argv, prefix,
options, usage_text, 0))
die(_("too many arguments"));
@@ -465,10 +474,18 @@ int main(int argc, char **argv)
/* check that no other copy is running */
fd = unix_stream_connect(git_path("index-helper.sock"));
- if (fd > 0)
- die(_("Already running"));
- if (errno != ECONNREFUSED && errno != ENOENT)
- die_errno(_("Unexpected error checking socket"));
+ if (fd > 0) {
+ if (autorun)
+ exit(0);
+ else
+ die(_("Already running"));
+ }
+ if (errno != ECONNREFUSED && errno != ENOENT) {
+ if (autorun)
+ return 0;
+ else
+ die_errno(_("Unexpected error checking socket"));
+ }
atexit(cleanup);
sigchain_push_common(cleanup_on_signal);
diff --git a/t/t7900-index-helper.sh b/t/t7900-index-helper.sh
index 7159971..aa6e5fc 100755
--- a/t/t7900-index-helper.sh
+++ b/t/t7900-index-helper.sh
@@ -38,4 +38,12 @@ test_expect_success 'index-helper will not start if already
running' '
grep "Already running" err
'
+test_expect_success 'index-helper is quiet with --autorun' '
+ test_when_finished "git index-helper --kill" &&
+ git index-helper --kill &&
+ git index-helper --detach &&
+ test -S .git/index-helper.sock &&
+ git index-helper --autorun
+'
+
test_done
--
2.4.2.767.g62658d5-twtrsrc
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v6 10/19] index-helper: use watchman to avoid refreshing index with lstat()
From: Nguyễn Thái Ngọc Duy
Watchman is hidden behind index-helper. Before git tries to read the
index from shm, it notifies index-helper through the socket and waits
for index-helper to prepare a file for sharing memory (with
MAP_SHARED). index-helper then contacts watchman, updates 'WAMA'
extension and put it in a separate file and wakes git up with a reply
to git's socket.
Git uses this extension to not lstat unchanged entries. Git only
trusts the 'WAMA' extension when it's received from the separate file,
not from disk. Unmarked entries are "clean". Marked entries are dirty
from watchman point of view. If it finds out some entries are
'watchman-dirty', but are really unchanged (e.g. the file was changed,
then reverted back), then Git will clear the marking in 'WAMA' before
writing it down.
Hiding watchman behind index-helper means you need both daemons. You
can't run watchman alone. Not so good. But on the other hand, 'git'
binary is not linked to watchman/json libraries, which is good for
packaging. Core git package will run fine without watchman-related
packages. If they need watchman, they can install git-index-helper and
dependencies.
This also lets us trust anything in the untracked cache that we haven't
marked invalid, saving those stat() calls.
Another reason for tying watchman to index-helper is, when used with
untracked cache, we need to keep track of $GIT_WORK_TREE file
listing. That kind of list can be kept in index-helper.
Signed-off-by: Nguyễn Thái Ngọc Duy
Signed-off-by: David Turner
---
Documentation/git-index-helper.txt | 6 +
cache.h| 2 +
dir.c | 23 +++-
dir.h | 3 +
index-helper.c | 105 ++--
read-cache.c | 243 ++---
6 files changed, 356 insertions(+), 26 deletions(-)
diff --git a/Documentation/git-index-helper.txt
b/Documentation/git-index-helper.txt
index 18e1636..647b796 100644
--- a/Documentation/git-index-helper.txt
+++ b/Documentation/git-index-helper.txt
@@ -52,6 +52,12 @@ command. The following commands are used to control the
daemon:
Let the daemon know the index is to be read. It keeps the
daemon alive longer, unless `--exit-after=0` is used.
+"poke ":
+ Like "poke", but replies with "OK". If the index has the
+ watchman extension, index-helper queries watchman, then
+ prepares a shared memory object with the watchman index
+ extension before replying.
+
All commands and replies are terminated by a 0 byte.
In the event of an error, messages may be written to
diff --git a/cache.h b/cache.h
index 37f211b..4cc89bb 100644
--- a/cache.h
+++ b/cache.h
@@ -558,6 +558,7 @@ extern int daemonize(int *);
/* Initialize and use the cache information */
struct lock_file;
+extern int verify_index(const struct index_state *);
extern int read_index(struct index_state *);
extern int read_index_preload(struct index_state *, const struct pathspec
*pathspec);
extern int do_read_index(struct index_state *istate, const char *path,
@@ -565,6 +566,7 @@ extern int do_read_index(struct index_state *istate, const
char *path,
extern int read_index_from(struct index_state *, const char *path);
extern int is_index_unborn(struct index_state *);
extern int read_index_unmerged(struct index_state *);
+extern void write_watchman_ext(struct strbuf *sb, struct index_state* istate);
#define COMMIT_LOCK(1 << 0)
#define CLOSE_LOCK (1 << 1)
extern int write_locked_index(struct index_state *, struct lock_file *lock,
unsigned flags);
diff --git a/dir.c b/dir.c
index 69e0be6..5058b29 100644
--- a/dir.c
+++ b/dir.c
@@ -597,9 +597,9 @@ static void trim_trailing_spaces(char *buf)
*
* If "name" has the trailing slash, it'll be excluded in the search.
*/
-static struct untracked_cache_dir *lookup_untracked(struct untracked_cache *uc,
- struct untracked_cache_dir
*dir,
- const char *name, int len)
+struct untracked_cache_dir *lookup_untracked(struct untracked_cache *uc,
+struct untracked_cache_dir *dir,
+const char *name, int len)
{
int first, last;
struct untracked_cache_dir *d;
@@ -1726,6 +1726,17 @@ static int valid_cached_dir(struct dir_struct *dir,
if (!untracked)
return 0;
+ if (dir->untracked->use_watchman) {
+ /*
+* With watchman, we can trust the untracked cache's
+* valid field.
+*/
+ if (untracked->valid)
+ goto skip_stat;
+ else
+ invalidate_directory(dir->untracked, untracked);
+ }
+
if (stat(path->len ? path->buf : ".", &st)) {
invalid
[PATCH v6 09/19] Add watchman support to reduce index refresh cost
From: Nguyễn Thái Ngọc Duy
The previous patch has the logic to clear bits in 'WAMA' bitmap. This
patch has logic to set bits as told by watchman. The missing bit,
_using_ these bits, are not here yet.
A lot of this code is written by David Turner originally, mostly from
[1]. I'm just copying and polishing it a bit.
[1] http://article.gmane.org/gmane.comp.version-control.git/248006
Signed-off-by: Nguyễn Thái Ngọc Duy
Signed-off-by: David Turner
---
Makefile | 12 +
cache.h| 1 +
config.c | 5 ++
configure.ac | 8
environment.c | 3 ++
watchman-support.c | 135 +
watchman-support.h | 7 +++
7 files changed, 171 insertions(+)
create mode 100644 watchman-support.c
create mode 100644 watchman-support.h
diff --git a/Makefile b/Makefile
index c8be0e7..65ab0f4 100644
--- a/Makefile
+++ b/Makefile
@@ -451,6 +451,7 @@ MSGFMT = msgfmt
CURL_CONFIG = curl-config
PTHREAD_LIBS = -lpthread
PTHREAD_CFLAGS =
+WATCHMAN_LIBS =
GCOV = gcov
export TCL_PATH TCLTK_PATH
@@ -1416,6 +1417,13 @@ else
LIB_OBJS += thread-utils.o
endif
+ifdef USE_WATCHMAN
+ LIB_H += watchman-support.h
+ LIB_OBJS += watchman-support.o
+ WATCHMAN_LIBS = -lwatchman
+ BASIC_CFLAGS += -DUSE_WATCHMAN
+endif
+
ifdef HAVE_PATHS_H
BASIC_CFLAGS += -DHAVE_PATHS_H
endif
@@ -2025,6 +2033,9 @@ git-remote-testsvn$X: remote-testsvn.o GIT-LDFLAGS
$(GITLIBS) $(VCSSVN_LIB)
$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^)
$(LIBS) \
$(VCSSVN_LIB)
+git-index-helper$X: index-helper.o GIT-LDFLAGS $(GITLIBS)
+ $(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^)
$(LIBS) $(WATCHMAN_LIBS)
+
$(REMOTE_CURL_ALIASES): $(REMOTE_CURL_PRIMARY)
$(QUIET_LNCP)$(RM) $@ && \
ln $< $@ 2>/dev/null || \
@@ -2164,6 +2175,7 @@ GIT-BUILD-OPTIONS: FORCE
@echo NO_PYTHON=\''$(subst ','\'',$(subst ','\'',$(NO_PYTHON)))'\' >>$@+
@echo NO_UNIX_SOCKETS=\''$(subst ','\'',$(subst
','\'',$(NO_UNIX_SOCKETS)))'\' >>$@+
@echo NO_MMAP=\''$(subst ','\'',$(subst ','\'',$(NO_MMAP)))'\' >>$@+
+ @echo USE_WATCHMAN=\''$(subst ','\'',$(subst
','\'',$(USE_WATCHMAN)))'\' >>$@+
ifdef TEST_OUTPUT_DIRECTORY
@echo TEST_OUTPUT_DIRECTORY=\''$(subst ','\'',$(subst
','\'',$(TEST_OUTPUT_DIRECTORY)))'\' >>$@+
endif
diff --git a/cache.h b/cache.h
index f4f7eef..37f211b 100644
--- a/cache.h
+++ b/cache.h
@@ -687,6 +687,7 @@ extern char *git_replace_ref_base;
extern int fsync_object_files;
extern int core_preload_index;
+extern int core_watchman_sync_timeout;
extern int core_apply_sparse_checkout;
extern int precomposed_unicode;
extern int protect_hfs;
diff --git a/config.c b/config.c
index 9ba40bc..e6dc141 100644
--- a/config.c
+++ b/config.c
@@ -882,6 +882,11 @@ static int git_default_core_config(const char *var, const
char *value)
return 0;
}
+ if (!strcmp(var, "core.watchmansynctimeout")) {
+ core_watchman_sync_timeout = git_config_int(var, value);
+ return 0;
+ }
+
if (!strcmp(var, "core.createobject")) {
if (!strcmp(value, "rename"))
object_creation_mode = OBJECT_CREATION_USES_RENAMES;
diff --git a/configure.ac b/configure.ac
index 0cd9f46..334d63b 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1099,6 +1099,14 @@ AC_COMPILE_IFELSE([BSD_SYSCTL_SRC],
HAVE_BSD_SYSCTL=])
GIT_CONF_SUBST([HAVE_BSD_SYSCTL])
+#
+# Check for watchman client library
+
+AC_CHECK_LIB([watchman], [watchman_connect],
+ [USE_WATCHMAN=YesPlease],
+ [USE_WATCHMAN=])
+GIT_CONF_SUBST([USE_WATCHMAN])
+
## Other checks.
# Define USE_PIC if you need the main git objects to be built with -fPIC
# in order to build and link perl/Git.so. x86-64 seems to need this.
diff --git a/environment.c b/environment.c
index 6dec9d0..35e03c7 100644
--- a/environment.c
+++ b/environment.c
@@ -94,6 +94,9 @@ int core_preload_index = 1;
*/
int ignore_untracked_cache_config;
+int core_watchman_sync_timeout = 300;
+
+
/* This is set by setup_git_dir_gently() and/or git_default_config() */
char *git_work_tree_cfg;
static char *work_tree;
diff --git a/watchman-support.c b/watchman-support.c
new file mode 100644
index 000..b168e88
--- /dev/null
+++ b/watchman-support.c
@@ -0,0 +1,135 @@
+#include "cache.h"
+#include "watchman-support.h"
+#include "strbuf.h"
+#include "dir.h"
+#include
+
+static struct watchman_query *make_query(const char *last_update)
+{
+ struct watchman_query *query = watchman_query();
+ watchman_query_set_fields(query, WATCHMAN_FIELD_NAME |
+WATCHMAN_FIELD_EXISTS |
+WATCHMAN_FIELD_NEWER);
+ watchman_query_set_empty_on_fresh(query, 1);
+ query->sync_timeout = core_watchman_sync_timeout;
+ if (*last_upda
[PATCH v6 01/19] read-cache.c: fix constness of verify_hdr()
From: Nguyễn Thái Ngọc Duy
Signed-off-by: Nguyễn Thái Ngọc Duy
Signed-off-by: David Turner
---
read-cache.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/read-cache.c b/read-cache.c
index d9fb78b..16cc487 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1345,7 +1345,7 @@ struct ondisk_cache_entry_extended {
ondisk_cache_entry_extended_size(ce_namelen(ce)) : \
ondisk_cache_entry_size(ce_namelen(ce)))
-static int verify_hdr(struct cache_header *hdr, unsigned long size)
+static int verify_hdr(const struct cache_header *hdr, unsigned long size)
{
git_SHA_CTX c;
unsigned char sha1[20];
--
2.4.2.767.g62658d5-twtrsrc
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v6 07/19] index-helper: add --detach
From: Nguyễn Thái Ngọc Duy
We detach after creating and opening the socket, because otherwise
we might return control to the shell before index-helper is ready to
accept commands. This might lead to flaky tests.
Signed-off-by: Nguyễn Thái Ngọc Duy
Signed-off-by: David Turner
---
Documentation/git-index-helper.txt | 3 +++
index-helper.c | 15 +--
2 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/Documentation/git-index-helper.txt
b/Documentation/git-index-helper.txt
index 4eb3d95..18e1636 100644
--- a/Documentation/git-index-helper.txt
+++ b/Documentation/git-index-helper.txt
@@ -31,6 +31,9 @@ OPTIONS
for reading an index, but because it will happen in the
background, it's not noticable. `--strict` is enabled by default.
+--detach::
+ Detach from the shell.
+
NOTES
-
diff --git a/index-helper.c b/index-helper.c
index 7400d68..4273773 100644
--- a/index-helper.c
+++ b/index-helper.c
@@ -17,7 +17,7 @@ struct shm {
static struct shm shm_index;
static struct shm shm_base_index;
-static int to_verify = 1;
+static int daemonized, to_verify = 1;
static void log_warning(const char *warning, ...)
{
@@ -59,6 +59,8 @@ static void cleanup_shm(void)
static void cleanup(void)
{
+ if (daemonized)
+ return;
unlink(git_path("index-helper.sock"));
cleanup_shm();
}
@@ -318,7 +320,7 @@ static const char * const usage_text[] = {
int main(int argc, char **argv)
{
const char *prefix;
- int idle_in_seconds = 600;
+ int idle_in_seconds = 600, detach = 0;
int fd;
struct strbuf socket_path = STRBUF_INIT;
struct option options[] = {
@@ -326,6 +328,7 @@ int main(int argc, char **argv)
N_("exit if not used after some seconds")),
OPT_BOOL(0, "strict", &to_verify,
"verify shared memory after creating"),
+ OPT_BOOL(0, "detach", &detach, "detach the process"),
OPT_END()
};
@@ -349,6 +352,14 @@ int main(int argc, char **argv)
if (fd < 0)
die_errno(_("could not set up index-helper socket"));
+ if (!freopen("/dev/null", "w", stderr))
+ die_errno("failed to redirect stderr to /dev/null");
+
+ if (!freopen("/dev/null", "w", stdout))
+ die_errno("failed to redirect stdout to /dev/null");
+
+ if (detach && daemonize(&daemonized))
+ die_errno(_("unable to detach"));
loop(fd, idle_in_seconds);
close(fd);
--
2.4.2.767.g62658d5-twtrsrc
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v6 03/19] index-helper: new daemon for caching index and related stuff
From: Nguyễn Thái Ngọc Duy Instead of reading the index from disk and worrying about disk corruption, the index is cached in memory (memory bit-flips happen too, but hopefully less often). The result is faster read. Read time is reduced by 70%. The biggest gain is not having to verify the trailing SHA-1, which takes lots of time especially on large index files. But this also opens doors for further optimiztions: - we could create an in-memory format that's essentially the memory dump of the index to eliminate most of parsing/allocation overhead. The mmap'd memory can be used straight away. Experiment [1] shows we could reduce read time by 88%. - we could cache non-index info such as name hash Shared memory is done by storing files in a per-repository temporary directory. This is more portable than shm (which requires posix-realtime and has various quirks on OS X). It might even work on Windows, although this has not been tested. The shared memory file's name follows the template "shm--" where is the trailing SHA-1 of the index file. is "index" for cached index files (and might later be "name-hash" for name-hash cache). If such shared memory exists, it contains the same index content as on disk. The content is already validated by the daemon and git won't validate it again (except comparing the trailing SHA-1s). We keep this daemon's logic as thin as possible. The "brain" stays in git. So the daemon can read and validate stuff, but that's all it's allowed to do. It does not add/create new information. It doesn't even accept direct updates from git. Git can poke the daemon via unix domain sockets to tell it to refresh the index cache, or to keep it alive some more minutes. It can't give any real index data directly to the daemon. Real data goes to disk first, then the daemon reads and verifies it from there. Poking only happens for $GIT_DIR/index, not temporary index files. $GIT_DIR/index-helper.sock is the socket for the daemon process. The daemon reads from the socket and executes commands. Named pipes were considered for portability reasons, but then commands that need replies from the daemon would have open their own pipes, since a named pipe should only have one reader. Unix domain sockets don't have this problem. On webkit.git with index format v2, duplicating 8 times to 1.5m entries and 236MB in size: (vanilla) 0.50 s: read_index_from .git/index (index-helper) 0.18 s: read_index_from .git/index Interestingly with index v4, we get less out of index-helper. It makes sense as v4 requires more processing after loading the index: (vanilla) 0.37 s: read_index_from .git/index (index-helper) 0.22 s: read_index_from .git/index [1] http://thread.gmane.org/gmane.comp.version-control.git/247268/focus=248771 Signed-off-by: Nguyễn Thái Ngọc Duy Signed-off-by: David Turner Signed-off-by: Ramsay Jones --- .gitignore | 1 + Documentation/git-index-helper.txt | 47 ++ Makefile | 5 + cache.h| 2 + git-compat-util.h | 1 + index-helper.c | 285 + read-cache.c | 122 ++-- t/t7900-index-helper.sh| 23 +++ 8 files changed, 477 insertions(+), 9 deletions(-) create mode 100644 Documentation/git-index-helper.txt create mode 100644 index-helper.c create mode 100755 t/t7900-index-helper.sh diff --git a/.gitignore b/.gitignore index 5087ce1..b92f122 100644 --- a/.gitignore +++ b/.gitignore @@ -71,6 +71,7 @@ /git-http-fetch /git-http-push /git-imap-send +/git-index-helper /git-index-pack /git-init /git-init-db diff --git a/Documentation/git-index-helper.txt b/Documentation/git-index-helper.txt new file mode 100644 index 000..77687c0 --- /dev/null +++ b/Documentation/git-index-helper.txt @@ -0,0 +1,47 @@ +git-index-helper(1) +=== + +NAME + +git-index-helper - A simple cache daemon for speeding up index file access + +SYNOPSIS + +[verse] +'git index-helper' [options] + +DESCRIPTION +--- +Keep the index file in memory for faster access. This daemon is per +repository. + +OPTIONS +--- + +--exit-after=:: + Exit if the cached index is not accessed for `` + seconds. Specify 0 to wait forever. Default is 600. + +NOTES +- + +$GIT_DIR/index-helper.sock a Unix domain socket that the daemon reads +commands from. The directory will also contain files named +"shm-index-". These are used as backing stores for shared +memory. Normally the daemon will clean up these files when it exits +or when they are no longer relevant. But if it crashes, some objects +could remain there and they can be safely deleted with "rm" +command. The following commands are used to control the daemon: + +"refresh":: + Reread the index. + +"poke": + Let the daemon know the index is to be read. It keeps the + daemon alive longer
[PATCH v6 14/19] index-helper: kill mode
Add a new command (and command-line arg) to allow index-helpers to
exit cleanly.
This is mainly useful for tests.
Signed-off-by: David Turner
---
Documentation/git-index-helper.txt | 3 +++
index-helper.c | 31 ++-
t/t7900-index-helper.sh| 9 +
3 files changed, 42 insertions(+), 1 deletion(-)
diff --git a/Documentation/git-index-helper.txt
b/Documentation/git-index-helper.txt
index d8b8efb..d68afd7 100644
--- a/Documentation/git-index-helper.txt
+++ b/Documentation/git-index-helper.txt
@@ -37,6 +37,9 @@ OPTIONS
--detach::
Detach from the shell.
+--kill::
+ Kill any running index-helper and clean up the socket
+
NOTES
-
diff --git a/index-helper.c b/index-helper.c
index a3c0221..60a71f2 100644
--- a/index-helper.c
+++ b/index-helper.c
@@ -378,6 +378,8 @@ static void loop(int fd, int idle_in_seconds)
* alive, nothing to do.
*/
}
+ } else if (!strcmp(buf, "die")) {
+ break;
} else {
log_warning("BUG: Bogus command %s", buf);
}
@@ -408,10 +410,29 @@ static const char * const usage_text[] = {
NULL
};
+static void request_kill(void)
+{
+ int fd = unix_stream_connect(git_path("index-helper.sock"));
+
+ if (fd >= 0) {
+ write_in_full(fd, "die", 4);
+ close(fd);
+ }
+
+ /*
+* The child will try to do this anyway, but we want to be
+* ready to launch a new daemon immediately after this command
+* returns.
+*/
+
+ unlink(git_path("index-helper.sock"));
+ return;
+}
+
int main(int argc, char **argv)
{
const char *prefix;
- int idle_in_seconds = 600, detach = 0;
+ int idle_in_seconds = 600, detach = 0, kill = 0;
int fd;
struct strbuf socket_path = STRBUF_INIT;
struct option options[] = {
@@ -420,6 +441,7 @@ int main(int argc, char **argv)
OPT_BOOL(0, "strict", &to_verify,
"verify shared memory after creating"),
OPT_BOOL(0, "detach", &detach, "detach the process"),
+ OPT_BOOL(0, "kill", &kill, "request that existing index helper
processes exit"),
OPT_END()
};
@@ -434,6 +456,13 @@ int main(int argc, char **argv)
options, usage_text, 0))
die(_("too many arguments"));
+ if (kill) {
+ if (detach)
+ die(_("--kill doesn't want any other options"));
+ request_kill();
+ return 0;
+ }
+
atexit(cleanup);
sigchain_push_common(cleanup_on_signal);
diff --git a/t/t7900-index-helper.sh b/t/t7900-index-helper.sh
index 114c112..e71b5af 100755
--- a/t/t7900-index-helper.sh
+++ b/t/t7900-index-helper.sh
@@ -20,4 +20,13 @@ test_expect_success 'index-helper smoke test' '
test_path_is_missing .git/index-helper.sock
'
+test_expect_success 'index-helper creates usable path file and can be killed' '
+ test_when_finished "git index-helper --kill" &&
+ test_path_is_missing .git/index-helper.sock &&
+ git index-helper --detach &&
+ test -S .git/index-helper.sock &&
+ git index-helper --kill &&
+ test_path_is_missing .git/index-helper.sock
+'
+
test_done
--
2.4.2.767.g62658d5-twtrsrc
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] trailer: load config to handle core.commentChar
Add call to git_config(git_default_config, NULL) to update the comment_char_line from default '#' to possible different value set in core.commentChar. Signed-off-by: Rafal Klys --- trailer.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/trailer.c b/trailer.c index 8e48a5c..a3700b4 100644 --- a/trailer.c +++ b/trailer.c @@ -888,6 +888,9 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct str git_config(git_trailer_default_config, NULL); git_config(git_trailer_config, NULL); + /* for core.commentChar */ + git_config(git_default_config, NULL); + lines = read_input_file(file); if (in_place) -- 2.8.1.68.g625efa9.dirty -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4] http: support sending custom HTTP headers
On Wed, Apr 27, 2016 at 02:20:37PM +0200, Johannes Schindelin wrote: > The only change vs v3 is that I replaced my flimsical test by Peff's (with > *one* change: I realized that we need to group the Require statements in a > block when I tried to verify that the test fails when I > modify the first header). Whoops, I didn't actually test that case. Thanks for catching (as you might guess, I wanted to make sure we handle multiple values correctly). > Documentation/config.txt| 6 ++ > http-push.c | 10 +- > http.c | 35 --- > http.h | 1 + > remote-curl.c | 4 ++-- > t/lib-httpd/apache.conf | 8 > t/t5551-http-fetch-smart.sh | 7 +++ > 7 files changed, 61 insertions(+), 10 deletions(-) This version looks good to me. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 19/29] refs: don't dereference on rename
Michael Haggerty writes:
> @@ -2380,8 +2381,8 @@ int rename_ref(const char *oldrefname, const char
> *newrefname, const char *logms
> goto rollback;
> }
>
> - if (!read_ref_full(newrefname, RESOLVE_REF_READING, sha1, NULL) &&
> - delete_ref(newrefname, sha1, REF_NODEREF)) {
> + if (!read_ref_full(newrefname, resolve_flags, sha1, NULL) &&
> + delete_ref(newrefname, NULL, REF_NODEREF)) {
Could you explain s/sha1/NULL/ here in the proposed log message?
> if (errno==EISDIR) {
> struct strbuf path = STRBUF_INIT;
> int result;
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] mmap(win32): avoid copy-on-write when it is unnecessary
Am 27.04.2016 um 08:43 schrieb Johannes Schindelin:
On Tue, 26 Apr 2016, Johannes Sixt wrote:
Should we insert a check for MAP_PRIVATE to catch
unexpected use-cases (think of the index-helper daemon effort)?
I agree, we should have such a check. The line above the `die("Invalid
usage ...")` that you can read as first line in above-quoted hunk reads:
if (!(flags & MAP_PRIVATE))
So I think we're fine :-)
Oh, well... I thought I had checked the code before I wrote my question,
but it seems I was blind... ;)
Thanks,
-- Hannes
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 15/29] ref_transaction_create(): disallow recursive pruning
Michael Haggerty writes:
> It is nonsensical (and a little bit dangerous) to use REF_ISPRUNING
> without REF_NODEREF. Forbid it explicitly. Change the one REF_ISPRUNING
> caller to pass REF_NODEREF too.
>
> Signed-off-by: Michael Haggerty
> ---
> This also makes later patches a bit clearer.
I wonder if it is more future-proof to solve this by doing
-#define REF_ISPRUNING 0x04
+#define REF_ISPRUNING (0x04 | REF_NODEREF)
instead. It makes the intention clear that pruning is always about
the single level (i.e. no-deref).
> refs.c | 3 +++
> refs/files-backend.c | 2 +-
> 2 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/refs.c b/refs.c
> index ba14105..5dc2473 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -790,6 +790,9 @@ int ref_transaction_update(struct ref_transaction
> *transaction,
> if (transaction->state != REF_TRANSACTION_OPEN)
> die("BUG: update called for transaction that is not open");
>
> + if ((flags & REF_ISPRUNING) && !(flags & REF_NODEREF))
> + die("BUG: REF_ISPRUNING set without REF_NODEREF");
> +
> if (new_sha1 && !is_null_sha1(new_sha1) &&
> check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
> strbuf_addf(err, "refusing to update ref with bad name '%s'",
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 9faf17c..8fcbd7d 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -2116,7 +2116,7 @@ static void prune_ref(struct ref_to_prune *r)
> transaction = ref_transaction_begin(&err);
> if (!transaction ||
> ref_transaction_delete(transaction, r->name, r->sha1,
> -REF_ISPRUNING, NULL, &err) ||
> +REF_ISPRUNING | REF_NODEREF, NULL, &err) ||
> ref_transaction_commit(transaction, &err)) {
> ref_transaction_free(transaction);
> error("%s", err.buf);
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 12/29] read_raw_ref(): improve docstring
Michael Haggerty writes: > * Backend-specific flags might be set in type as well, regardless of > * outcome. > * > - * sb_path is workspace: the caller should allocate and free it. All made sense. A welcome bonus is the removal of this stale comment that 42a38cf7 (read_raw_ref(): manage own scratch space, 2016-04-07) forgot to remove. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v14 3/6] t0040-parse-options: improve test coverage
On Wed, Apr 27, 2016 at 11:25 PM, Eric Sunshine wrote: > On Mon, Apr 25, 2016 at 2:40 PM, Pranit Bauva wrote: >> On Wed, Apr 13, 2016 at 10:57 PM, Eric Sunshine >> wrote: >>> Each of these patches should have a single conceptual purpose. It >>> seems, from the above explanation, that you're mixing and mismatching >>> bits of such changes between patches. >>> >>> * The two new tests for --no-verbose and --no-quiet should be together >>> and check that they correctly reverse --verbose and --quiet, >>> respectively. >>> >>> * The test you describe above which ensures that --no-quiet leaves >>> 'quiet' at 0 should be bundled with the change that might break that >>> behavior, namely, the OPT__COUNTUP() change. >> >> I am planning to re-roll this. >> So, I am just confirming whether I understood properly. >> >> * I will add the tests for check for '-q --no-quiet' instead of just >> '--no-quiet' sets to 0 and '-v --no-verbose' sets to 0 in the patch >> which improves test coverage which will be before the OPT_COUNTUP() >> change. > > These tests would be even a bit more interesting if you tested "-q -q > --no-quiet" and "-v -v --no-verbose", respectively, to ensure that the > "no" options actually reset to 0 rather than merely decrementing by 1. This seems a better choice. > I recall also suggesting adding a new test checking that "-q -q" > increments the quiet count to 2 (which could be done in the patch > which makes 'quiet' print as a number instead of a boolean or in the > same "improve test coverage" patch). Will include this. >> * I will then add the test for '--no-quiet' sets to 0 in the separate >> patch after OPT_COUNTUP() change. > > No, this and "--no-verbose sets to 0" are logically related to the > OPT__COUNTUP() change, thus would be incorporated into that patch. > Alternately, these two tests could just be part of "improve test > coverage" patch, establishing base behavior which the OPT__COUNTUP() > patch shouldn't break. I would prefer including it in "improve test coverage" patch to establish the base behavior. This seems more natural to me. You can expect this series from me within 2 days. Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 08/29] ref_transaction_commit(): remove local variable n
Michael Haggerty writes: > This microoptimization doesn't make a significant difference in speed. > And it causes problems if somebody ever wants to modify the function to > add updates to a transaction as part of processing it, as will happen > shortly. > > Make the same change in initial_ref_transaction_commit(). > > Signed-off-by: Michael Haggerty > --- This particular change also makes the end result easier to read. I have no objection to officially declaring that we do support "adding" new transaction update while we are committing (and we do not support other futzing like "removing" or "reordering"), either. I expect that somewhere in this series transaction->nr will not stay constant even if the client code of ref-transaction API makes no direct call that adds a new update[] element, though, even if it is not done in this patch. Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 53/83] builtin/apply: make find_header() return -1 instead of die()ing
On Mon, Apr 25, 2016 at 9:18 AM, Duy Nguyen wrote: > On Sun, Apr 24, 2016 at 8:33 PM, Christian Couder > wrote: >> To be compatible with the rest of the error handling in builtin/apply.c, >> find_header() should return -1 instead of calling die(). >> >> Unfortunately find_header() already returns -1 when no header is found, >> so let's make it return -2 instead in this case. > > I don't think this is a good way to go. Too many magic numbers. I > don't have a better option though. Maybe returning names instead of > numbers would help a bit. I suppose 'hdrsize' could signal this extra condition. For instance, always return -1 on error, and set hdrsize to 0 for header not found (unless 0 is a valid size?), and -1 for any other error. But perhaps that's getting too clever... -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 53/83] builtin/apply: make find_header() return -1 instead of die()ing
On Sun, Apr 24, 2016 at 9:33 AM, Christian Couder
wrote:
> To be compatible with the rest of the error handling in builtin/apply.c,
> find_header() should return -1 instead of calling die().
>
> Unfortunately find_header() already returns -1 when no header is found,
> so let's make it return -2 instead in this case.
>
> Signed-off-by: Christian Couder
> ---
> diff --git a/builtin/apply.c b/builtin/apply.c
> @@ -1588,18 +1596,18 @@ static int find_header(struct apply_state *state,
> continue;
> if (!patch->old_name && !patch->new_name) {
> if (!patch->def_name)
> - die(Q_("git diff header lacks
> filename information when removing "
> - "%d leading pathname component
> (line %d)",
> - "git diff header lacks
> filename information when removing "
> - "%d leading pathname
> components (line %d)",
> - state->p_value),
> - state->p_value, state->linenr);
> + return error(Q_("git diff header
> lacks filename information when removing "
> + "%d leading pathname
> component (line %d)",
> + "git diff header
> lacks filename information when removing "
> + "%d leading pathname
> components (line %d)",
> + state->p_value),
> +state->p_value,
> state->linenr);
> patch->old_name = xstrdup(patch->def_name);
> patch->new_name = xstrdup(patch->def_name);
> }
> if (!patch->is_delete && !patch->new_name)
> - die("git diff header lacks filename
> information "
> - "(line %d)", state->linenr);
> + return error("git diff header lacks filename
> information "
> +"(line %d)", state->linenr);
I realize that the caller in this patch currently just die()'s, and I
haven't looked at subsequent patches yet, but is this new 'return'
going to cause the caller to start leaking patch->old_name and
patch->new_name which are xstrdup()'d just above?
> patch->is_toplevel_relative = 1;
> *hdrsize = git_hdr_len;
> return offset;
> @@ -2115,6 +2123,9 @@ static int parse_chunk(struct apply_state *state, char
> *buffer, unsigned long si
> int hdrsize, patchsize;
> int offset = find_header(state, buffer, size, &hdrsize, patch);
>
> + if (offset == -1)
> + exit(1);
> +
> if (offset < 0)
> return offset;
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: BUG on OSX `git p4 submit` can fail when the workspace root doesn't exist locally.
I debugged the issue using the script here: https://github.com/git/git/blob/master/git-p4.py I'm not sure how close to the main repo that is. On Wed, Apr 27, 2016 at 11:28 AM, Stefan Beller wrote: > On Wed, Apr 27, 2016 at 9:15 AM, Jacob Smith wrote: >> On OS X, > > Do you use the Git as provided from OS X? In that case you better report the > bug > to Apple, as their version of Git is slightly different (not close on > upstream, by both > having additional patches as well as not following the upstream closely IIUC). -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/15] string_list: add string_list_duplicate
On Tue, Apr 26, 2016 at 3:37 PM, Junio C Hamano wrote:
> Stefan Beller writes:
>
>> The result of git_config_get_value_multi do not seem to be stable and
>> may get overwritten. Have an easy way to preserve the results of that
>> query.
>>
>> Signed-off-by: Stefan Beller
>> ---
>
> This morning I reviewed a patch that adds something whose name ends
> with _copy(), and this may want to follow suit.
ok, in case we need this patch, I'll rename.
>
> Should strdup_strings() be a separate parameter, or should it follow
> what src->strdup_strings has?
>
> "Do not seem to be stable" does not build confidence in the code,
> making it sound as if the developer is basing his work on guess not
> analysis, and makes it hard to tell if this is a wrong workaround to
> a valid issue (e.g. it could be "making the result stable" is what
> we want in the longer term) or a valid solution to a problem that
> would be common across callers of that API.
When not duplicating git_config_get_value_multi("submodule.defaultGroup");
I run into
Program received signal SIGSEGV, Segmentation fault.
0x00579097 in get_entry_index (list=0x876848, string=0x8721e0
"./submodule1", exact_match=0x7fffd6bc) at string-list.c:38
38 int compare = cmp(string, list->items[middle].string);
(gdb) bt
#0 0x00579097 in get_entry_index (list=0x876848,
string=0x8721e0 "./submodule1", exact_match=0x7fffd6bc) at
string-list.c:38
#1 0x005792d5 in string_list_has_string (list=0x876848,
string=0x8721e0 "./submodule1") at string-list.c:91
#2 0x0057e78c in submodule_in_group (group=0x876848,
sub=0x878510) at submodule-config.c:558
#3 0x00497cf9 in module_init (argc=0, argv=0x7fffdb28,
prefix=0x0) at builtin/submodule--helper.c:441
#4 0x004993f6 in cmd_submodule__helper (argc=2,
argv=0x7fffdb20, prefix=0x0) at builtin/submodule--helper.c:927
#5 0x0040582e in run_builtin (p=0x83c0c0 ,
argc=2, argv=0x7fffdb20) at git.c:353
#6 0x00405a1d in handle_builtin (argc=2, argv=0x7fffdb20)
at git.c:540
#7 0x00405b3f in run_argv (argcp=0x7fffd9fc,
argv=0x7fffda10) at git.c:594
#8 0x00405d32 in main (argc=2, av=0x7fffdb18) at git.c:701
(gdb) print list->items[middle].string
Cannot access memory at address 0x746c006fd40bd151
So the string list seems to be corrupted here.
Someone stomping over our memory? How long is the result
of git_config_get_value_multi deemed to be stable and usable?
I did not find out myself how to properly answer these.
So it was symptom based programming (copying that stuff makes the
error go away).
This only happens in one code path, which is
group = NULL;
if (!pathspec.nr)
group = //string_list_duplicate(
(struct string_list*)
git_config_get_value_multi("submodule.defaultGroup");//, 1);
if (group) {
gitmodules_config();
for (i = 0; i < list.nr; i++) {
const struct submodule *sub =
submodule_from_path(null_sha1,
list.entries[i]->name);
if (submodule_in_group(group, sub))
init_submodule(list.entries[i]->name,
prefix, quiet);
}
}
... // group is not further used
Maybe gitmodules_config needs to be called before git_config_get_value_multi,
as it changes that?
I dunno, will debug more later.
Thanks,
Stefan
>
>> string-list.c | 18 ++
>> string-list.h | 2 ++
>> 2 files changed, 20 insertions(+)
>>
>> diff --git a/string-list.c b/string-list.c
>> index 2a32a3f..f981c8a 100644
>> --- a/string-list.c
>> +++ b/string-list.c
>> @@ -7,6 +7,24 @@ void string_list_init(struct string_list *list, int
>> strdup_strings)
>> list->strdup_strings = strdup_strings;
>> }
>>
>> +struct string_list *string_list_duplicate(const struct string_list *src,
>> + int strdup_strings)
>> +{
>> + struct string_list *list;
>> + struct string_list_item *item;
>> +
>> + if (!src)
>> + return NULL;
>> +
>> + list = xmalloc(sizeof(*list));
>> + string_list_init(list, strdup_strings);
>> + for_each_string_list_item(item, src)
>> + string_list_append(list, item->string);
>> +
>> + return list;
>> +}
>> +
>> +
>> /* if there is no exact match, point to the index where the entry could be
>> * inserted */
>> static int get_entry_index(const struct string_list *list, const char
>> *string,
>> diff --git a/string-list.h b/string-list.h
>> index d3809a1..1a5612f 100644
>> --- a/string-list.h
>> +++ b/string-list.h
>> @@ -19,6 +19,8 @@ struct string_list {
>> #define STRING_LIST_INIT_DUP { NULL, 0, 0, 1, NULL }
>>
>> void string_list_init(struct string_list *list, int strdup_strings);
>> +struct string_list *string_list_duplicate(const struct string_li
Re: [PATCH 05/29] refname_is_safe(): insist that the refname already be normalized
Michael Haggerty writes: > Does anybody have a use case for allowing un-normalized reference > names like "refs/foo/../bar///baz"? I'm pretty certain they would not > be handled correctly anyway, especially if they are not stored as > loose references. I wondered what codepath this follows: $ git rev-parse mhagger/wip//split-under-lock get_sha1_basic() calls to dwim_ref() to learn real_ref and its value. dwim_ref() repeatedly gives the string to a known dwim rule, and the "refs/remotes/%.*s" rule is expected to find that the user meant to name "refs/remotes/mhagger/wip//split-under-lock". This, still with doubled slash, is passed to resolve_ref_unsafe(), which has a call to !refname_is_safe(refname) to reject the request. So I think this will move the rejection early in the codepath than how we reject the ref with doubled slash in the current code, but the end result would be the same for this example. The same is true for heads//master or refs/heads//master. There is another call to refname_is_safe() in resolve_ref_unsafe(), which applies the sanity check to the string from a symref. We seem to allow $ git symbolic-ref refs/heads/SSS refs/heads//master and we end up storing "ref: refs/heads//master" (or make a symbolic link with doubled slashes), but the current code considers the resulting symbolic link as "dangling". Again, this change moves the rejection a bit earlier in the codepath, without changing the end result, I'd think. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v14 3/6] t0040-parse-options: improve test coverage
On Mon, Apr 25, 2016 at 2:40 PM, Pranit Bauva wrote: > On Wed, Apr 13, 2016 at 10:57 PM, Eric Sunshine > wrote: >> Each of these patches should have a single conceptual purpose. It >> seems, from the above explanation, that you're mixing and mismatching >> bits of such changes between patches. >> >> * The two new tests for --no-verbose and --no-quiet should be together >> and check that they correctly reverse --verbose and --quiet, >> respectively. >> >> * The test you describe above which ensures that --no-quiet leaves >> 'quiet' at 0 should be bundled with the change that might break that >> behavior, namely, the OPT__COUNTUP() change. > > I am planning to re-roll this. > So, I am just confirming whether I understood properly. > > * I will add the tests for check for '-q --no-quiet' instead of just > '--no-quiet' sets to 0 and '-v --no-verbose' sets to 0 in the patch > which improves test coverage which will be before the OPT_COUNTUP() > change. These tests would be even a bit more interesting if you tested "-q -q --no-quiet" and "-v -v --no-verbose", respectively, to ensure that the "no" options actually reset to 0 rather than merely decrementing by 1. I recall also suggesting adding a new test checking that "-q -q" increments the quiet count to 2 (which could be done in the patch which makes 'quiet' print as a number instead of a boolean or in the same "improve test coverage" patch). > * I will then add the test for '--no-quiet' sets to 0 in the separate > patch after OPT_COUNTUP() change. No, this and "--no-verbose sets to 0" are logically related to the OPT__COUNTUP() change, thus would be incorporated into that patch. Alternately, these two tests could just be part of "improve test coverage" patch, establishing base behavior which the OPT__COUNTUP() patch shouldn't break. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] Makefile: remove dependency on git.spec
ab21433 dropped support for rpmbuild using our own specfile by removing
git.spec.in, but forgot to remove the dependency of dist on git.spec.
Signed-off-by: Dennis Kaarsemaker
---
Makefile | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Makefile b/Makefile
index 23182bc..8083b10 100644
--- a/Makefile
+++ b/Makefile
@@ -2396,7 +2396,7 @@ quick-install-html:
### Maintainer's dist rules
GIT_TARNAME = git-$(GIT_VERSION)
-dist: git.spec git-archive$(X) configure
+dist: git-archive$(X) configure
./git-archive --format=tar \
--prefix=$(GIT_TARNAME)/ HEAD^{tree} > $(GIT_TARNAME).tar
@mkdir -p $(GIT_TARNAME)
--
2.8.1-387-gd7fd66b
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/83] builtin/apply: avoid parameter shadowing 'linenr' global
On Wed, Apr 27, 2016 at 6:27 PM, Junio C Hamano wrote: > > I think 02/83 that renamed the global-to-be-moved-to-state to > state_p_value was brilliant, and this should follow suit; you would > be moving linenr into the state eventually in later steps, right? Yeah, ok, I will do the same thing to this patch as I did to 02/83. Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 10/29] read_raw_ref(): clear *type at start of function
This is more convenient and less error-prone for callers. Signed-off-by: Michael Haggerty --- refs/files-backend.c | 1 + 1 file changed, 1 insertion(+) diff --git a/refs/files-backend.c b/refs/files-backend.c index 303c43b..f10c80f 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -1430,6 +1430,7 @@ int read_raw_ref(const char *refname, unsigned char *sha1, int ret = -1; int save_errno; + *type = 0; strbuf_reset(&sb_path); strbuf_git_path(&sb_path, "%s", refname); path = sb_path.buf; -- 2.8.1 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 09/29] read_raw_ref(): rename flags argument to type
This will hopefully reduce confusion with the "flags" arguments that are
used in many functions in this module as an input parameter to choose
how the function should operate.
Signed-off-by: Michael Haggerty
---
refs/files-backend.c | 16
refs/refs-internal.h | 2 +-
2 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 05797cb..303c43b 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1395,18 +1395,18 @@ static int resolve_missing_loose_ref(const char
*refname,
*
* If the ref is symbolic, fill in *symref with the referrent
* (e.g. "refs/heads/master") and return 0. The caller is responsible
- * for validating the referrent. Set REF_ISSYMREF in flags.
+ * for validating the referrent. Set REF_ISSYMREF in type.
*
* If the ref doesn't exist, set errno to ENOENT and return -1.
*
* If the ref exists but is neither a symbolic ref nor a sha1, it is
- * broken. Set REF_ISBROKEN in flags, set errno to EINVAL, and return
+ * broken. Set REF_ISBROKEN in type, set errno to EINVAL, and return
* -1.
*
* If there is another error reading the ref, set errno appropriately and
* return -1.
*
- * Backend-specific flags might be set in flags as well, regardless of
+ * Backend-specific flags might be set in type as well, regardless of
* outcome.
*
* sb_path is workspace: the caller should allocate and free it.
@@ -1419,7 +1419,7 @@ static int resolve_missing_loose_ref(const char *refname,
* refname will still be valid and unchanged.
*/
int read_raw_ref(const char *refname, unsigned char *sha1,
-struct strbuf *symref, unsigned int *flags)
+struct strbuf *symref, unsigned int *type)
{
struct strbuf sb_contents = STRBUF_INIT;
struct strbuf sb_path = STRBUF_INIT;
@@ -1448,7 +1448,7 @@ stat_ref:
if (lstat(path, &st) < 0) {
if (errno != ENOENT)
goto out;
- if (resolve_missing_loose_ref(refname, sha1, flags)) {
+ if (resolve_missing_loose_ref(refname, sha1, type)) {
errno = ENOENT;
goto out;
}
@@ -1469,7 +1469,7 @@ stat_ref:
if (starts_with(sb_contents.buf, "refs/") &&
!check_refname_format(sb_contents.buf, 0)) {
strbuf_swap(&sb_contents, symref);
- *flags |= REF_ISSYMREF;
+ *type |= REF_ISSYMREF;
ret = 0;
goto out;
}
@@ -1510,7 +1510,7 @@ stat_ref:
strbuf_reset(symref);
strbuf_addstr(symref, buf);
- *flags |= REF_ISSYMREF;
+ *type |= REF_ISSYMREF;
ret = 0;
goto out;
}
@@ -1521,7 +1521,7 @@ stat_ref:
*/
if (get_sha1_hex(buf, sha1) ||
(buf[40] != '\0' && !isspace(buf[40]))) {
- *flags |= REF_ISBROKEN;
+ *type |= REF_ISBROKEN;
errno = EINVAL;
goto out;
}
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 3a4f634..0b047f6 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -210,6 +210,6 @@ int do_for_each_ref(const char *submodule, const char *base,
each_ref_fn fn, int trim, int flags, void *cb_data);
int read_raw_ref(const char *refname, unsigned char *sha1,
-struct strbuf *symref, unsigned int *flags);
+struct strbuf *symref, unsigned int *type);
#endif /* REFS_REFS_INTERNAL_H */
--
2.8.1
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 22/29] lock_ref_for_update(): new function
Extract a new function, lock_ref_for_update(), from
ref_transaction_commit().
Signed-off-by: Michael Haggerty
---
refs/files-backend.c | 152 ---
1 file changed, 85 insertions(+), 67 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index e0d9fa3..546656a 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3048,6 +3048,88 @@ static int ref_update_reject_duplicates(struct
string_list *refnames,
return 0;
}
+/*
+ * Acquire all locks, verify old values if provided, check
+ * that new values are valid, and write new values to the
+ * lockfiles, ready to be activated. Only keep one lockfile
+ * open at a time to avoid running out of file descriptors.
+ */
+static int lock_ref_for_update(struct ref_update *update,
+ struct ref_transaction *transaction,
+ struct string_list *affected_refnames,
+ struct strbuf *err)
+{
+ int ret;
+
+ if ((update->flags & REF_HAVE_NEW) &&
+ is_null_sha1(update->new_sha1))
+ update->flags |= REF_DELETING;
+ update->lock = lock_ref_sha1_basic(
+ update->refname,
+ ((update->flags & REF_HAVE_OLD) ?
+update->old_sha1 : NULL),
+ affected_refnames, NULL,
+ update->flags,
+ &update->type,
+ err);
+ if (!update->lock) {
+ char *reason;
+
+ ret = (errno == ENOTDIR)
+ ? TRANSACTION_NAME_CONFLICT
+ : TRANSACTION_GENERIC_ERROR;
+ reason = strbuf_detach(err, NULL);
+ strbuf_addf(err, "cannot lock ref '%s': %s",
+ update->refname, reason);
+ free(reason);
+ return ret;
+ }
+ if ((update->flags & REF_HAVE_NEW) &&
+ !(update->flags & REF_DELETING) &&
+ !(update->flags & REF_LOG_ONLY)) {
+ int overwriting_symref = ((update->type & REF_ISSYMREF) &&
+ (update->flags & REF_NODEREF));
+
+ if (!overwriting_symref &&
+ !hashcmp(update->lock->old_oid.hash, update->new_sha1)) {
+ /*
+* The reference already has the desired
+* value, so we don't need to write it.
+*/
+ } else if (write_ref_to_lockfile(update->lock,
+update->new_sha1,
+err)) {
+ char *write_err = strbuf_detach(err, NULL);
+
+ /*
+* The lock was freed upon failure of
+* write_ref_to_lockfile():
+*/
+ update->lock = NULL;
+ strbuf_addf(err,
+ "cannot update the ref '%s': %s",
+ update->refname, write_err);
+ free(write_err);
+ return TRANSACTION_GENERIC_ERROR;
+ } else {
+ update->flags |= REF_NEEDS_COMMIT;
+ }
+ }
+ if (!(update->flags & REF_NEEDS_COMMIT)) {
+ /*
+* We didn't call write_ref_to_lockfile(), so
+* the lockfile is still open. Close it to
+* free up the file descriptor:
+*/
+ if (close_ref(update->lock)) {
+ strbuf_addf(err, "couldn't close '%s.lock'",
+ update->refname);
+ return TRANSACTION_GENERIC_ERROR;
+ }
+ }
+ return 0;
+}
+
int ref_transaction_commit(struct ref_transaction *transaction,
struct strbuf *err)
{
@@ -3085,74 +3167,10 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
for (i = 0; i < transaction->nr; i++) {
struct ref_update *update = updates[i];
- if ((update->flags & REF_HAVE_NEW) &&
- is_null_sha1(update->new_sha1))
- update->flags |= REF_DELETING;
- update->lock = lock_ref_sha1_basic(
- update->refname,
- ((update->flags & REF_HAVE_OLD) ?
-update->old_sha1 : NULL),
- &affected_refnames, NULL,
- update->flags,
- &update->type,
- err);
- if (!update->lock) {
- char *reason;
-
- ret = (errno == ENOTDIR)
- ? TRANS
[PATCH 00/29] Yet more preparation for reference backends
This started as a modest attempt to move the last couple of patches mentioned here [1] to before the vtable patches. I wanted to do that because having real changes mixed with the vtable refactoring was making rebasing and stuff awkward. But then it snowballed. A lot of what's new is pretty trivial (though in some cases fixes minor bugs). But a few of the later patches are pretty major. The two main problems addressed by this series are: 1. Reference transactions will, in the future, sometimes span two completely different reference backends. For example, we already have worktree-specific refs like `HEAD` and `refs/bisect/*`, which are stored within the worktree, plus shared refs, which live in the main repository. It is even possible for a symref in one reference backend to point at a reference in another reference backend. Thus we need to be able to split one main transaction into one transaction per backend. 2. Currently, reference transactions that involve symbolic refs are not atomic: the symref is not locked at all, even when its reflog is being updated. This is a no-no. It also means that its referent can change between the time that the symref is resolved to find out its old_oid and the time that the referent is locked. These aren't super serious races because symrefs are usually not modified very often (especially on Git servers, which is mostly where concurrent modifications are an issue). But still... The approach taken to solve these problems is inspired by David Turner's patch [2], whose approach was first discussed here [3]. David wrote a separate function, dereference_symrefs(transaction, ...), which scanned through the whole transaction and split up any symref updates into one symref update with the REF_LOG_ONLY option (to update the symrefs reflog), plus a second update that changes the underlying reference. This approach solves problem 1 but not problem 2. This patch series takes a slightly different approach. For each reference update during the "lock references" part of ref_transaction_commit(), it 1. Locks the named reference. (If it is a symref, lock *the symref*, not its referent!) 2. Reads the reference non-recursively 3. If it is a symref, change the update to REF_LOG_ONLY and add a second update to the transaction for the underlying reference. 4. If it is not a symref but was derived from a symref update, record the reference's old_oid in the ref_update structure for the original symref. In this way, each reference in a symref chain is locked down *before* we read it and the lock is held throughout the transaction. As a bonus, we don't have to use resolve_ref_full() to find old_oid for the references; it is enough to read the references *once*, because we do it under lock. The three patches starting with "refs: resolve symbolic refs first" involve a lot of new code in an area that is pretty intricate. I've reviewed them a few times, but it's quite possible I've made some mistakes (especially in the error-handling code). Careful review here would be much appreciated. It's possible that people will think this is too much new code for fixing a relatively unlikely race. I'm not absolutely convinced myself that it's not overengineered. But splitting ref_updates is definitely needed for supporting multiple backends, and I think the approach of locking then following one reference at a time is more or less a prerequisite for making symref locking work correctly with multiple reference backends. So (I think) our choices are to accept this code or something like it, or to give up the hope of correct atomicity of transactions that span backends. This patch series is also available from my GitHub repository [4] as branch "split-under-lock". It applies to Junio's current master. Incidentally, a draft of the *next* patch series, which adds a ref_store vtable abstraction for managing reference backends, is available as branch "ref-store" in my GitHub repo. That branch passes all tests but is not yet carefully reviewed. Following is a table of contents for this patch series... Chapter 1: Prologue This chapter consists of small cleanups that I noticed while working on the code. * safe_create_leading_directories(): improve docstring * remove_dir_recursively(): add docstring * refname_is_safe(): use skip_prefix() This patch fixes a bug in refname_is_safe(): * refname_is_safe(): don't allow the empty string This one makes refname_is_safe() a little stricter...it shouldn't allow refnames like "refs/foo/../bar///baz" because the downstream code isn't smart enough to handle them anyway. (At the latest if the reference has to be sought in packed-refs, things would fail): * refname_is_safe(): insist that the refname already be normalized Chapter 2: Character development Make sure error output goes the right place: * commit_ref_update(): write error message to *err, not stderr Tighten up some cod
[PATCH 07/29] rename_ref(): remove unneeded local variable
Signed-off-by: Michael Haggerty
---
refs/files-backend.c | 9 +++--
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index de38517..0ade681 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2351,20 +2351,17 @@ int rename_ref(const char *oldrefname, const char
*newrefname, const char *logms
struct ref_lock *lock;
struct stat loginfo;
int log = !lstat(git_path("logs/%s", oldrefname), &loginfo);
- const char *symref = NULL;
struct strbuf err = STRBUF_INIT;
if (log && S_ISLNK(loginfo.st_mode))
return error("reflog for %s is a symlink", oldrefname);
- symref = resolve_ref_unsafe(oldrefname, RESOLVE_REF_READING,
- orig_sha1, &flag);
+ if (!resolve_ref_unsafe(oldrefname, RESOLVE_REF_READING, orig_sha1,
&flag))
+ return error("refname %s not found", oldrefname);
+
if (flag & REF_ISSYMREF)
return error("refname %s is a symbolic ref, renaming it is not
supported",
oldrefname);
- if (!symref)
- return error("refname %s not found", oldrefname);
-
if (!rename_ref_available(oldrefname, newrefname))
return 1;
--
2.8.1
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 24/29] ref_transaction_update(): check refname_is_safe() at a minimum
If the user has asked that a new value be set for a reference, we use
check_refname_format() to verify that the reference name satisfies all
of the rules. But in other cases, at least check that refname_is_safe().
Signed-off-by: Michael Haggerty
---
There are remaining problems in this area of the code. For example,
check_refname_format() is *less* strict than refname_is_safe(). It
allows almost arbitrary top-level reference names like "foo" and
"refs". (The latter is especially fun because if you manage to create
a reference called "refs", Git stops recognizing the directory as a
Git repository.) On the other hand, some callers call
check_refname_format() with incomplete reference names (e.g., branch
names like "master"), so the functions can't be made stricter until
those callers are changed.
I'll address these problems separately if I find the time.
refs.c | 5 +++--
t/t1400-update-ref.sh | 2 +-
t/t1430-bad-ref-name.sh | 2 +-
3 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/refs.c b/refs.c
index 858b6d7..41eb9e2 100644
--- a/refs.c
+++ b/refs.c
@@ -802,8 +802,9 @@ int ref_transaction_update(struct ref_transaction
*transaction,
if ((flags & REF_ISPRUNING) && !(flags & REF_NODEREF))
die("BUG: REF_ISPRUNING set without REF_NODEREF");
- if (new_sha1 && !is_null_sha1(new_sha1) &&
- check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
+ if ((new_sha1 && !is_null_sha1(new_sha1)) ?
+ check_refname_format(refname, REFNAME_ALLOW_ONELEVEL) :
+ !refname_is_safe(refname)) {
strbuf_addf(err, "refusing to update ref with bad name '%s'",
refname);
return -1;
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 40b0cce..08bd8fd 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -23,7 +23,7 @@ test_expect_success setup '
m=refs/heads/master
n_dir=refs/heads/gu
n=$n_dir/fixes
-outside=foo
+outside=refs/foo
test_expect_success \
"create $m" \
diff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh
index 25ddab4..8937e25 100755
--- a/t/t1430-bad-ref-name.sh
+++ b/t/t1430-bad-ref-name.sh
@@ -285,7 +285,7 @@ test_expect_success 'update-ref -d cannot delete non-ref in
.git dir' '
echo precious >expect &&
test_must_fail git update-ref -d my-private-file >output 2>error &&
test_must_be_empty output &&
- test_i18ngrep -e "cannot lock .*: unable to resolve reference" error &&
+ test_i18ngrep -e "refusing to update ref with bad name" error &&
test_cmp expect .git/my-private-file
'
--
2.8.1
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 17/29] delete_branches(): use resolve_refdup()
The return value of resolve_ref_unsafe() is not guaranteed to stay
around as long as we need it, so use resolve_refdup() instead.
Signed-off-by: Michael Haggerty
---
builtin/branch.c | 19 +++
1 file changed, 11 insertions(+), 8 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index 0adba62..ae55688 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -212,7 +212,7 @@ static int delete_branches(int argc, const char **argv, int
force, int kinds,
die(_("Couldn't look up commit object for HEAD"));
}
for (i = 0; i < argc; i++, strbuf_release(&bname)) {
- const char *target;
+ char *target = NULL;
int flags = 0;
strbuf_branchname(&bname, argv[i]);
@@ -231,11 +231,11 @@ static int delete_branches(int argc, const char **argv,
int force, int kinds,
}
}
- target = resolve_ref_unsafe(name,
- RESOLVE_REF_READING
- | RESOLVE_REF_NO_RECURSE
- | RESOLVE_REF_ALLOW_BAD_NAME,
- sha1, &flags);
+ target = resolve_refdup(name,
+ RESOLVE_REF_READING
+ | RESOLVE_REF_NO_RECURSE
+ | RESOLVE_REF_ALLOW_BAD_NAME,
+ sha1, &flags);
if (!target) {
error(remote_branch
? _("remote-tracking branch '%s' not found.")
@@ -248,7 +248,7 @@ static int delete_branches(int argc, const char **argv, int
force, int kinds,
check_branch_commit(bname.buf, name, sha1, head_rev, kinds,
force)) {
ret = 1;
- continue;
+ goto next;
}
if (delete_ref(name, is_null_sha1(sha1) ? NULL : sha1,
@@ -258,7 +258,7 @@ static int delete_branches(int argc, const char **argv, int
force, int kinds,
: _("Error deleting branch '%s'"),
bname.buf);
ret = 1;
- continue;
+ goto next;
}
if (!quiet) {
printf(remote_branch
@@ -270,6 +270,9 @@ static int delete_branches(int argc, const char **argv, int
force, int kinds,
: find_unique_abbrev(sha1, DEFAULT_ABBREV));
}
delete_branch_config(bname.buf);
+
+ next:
+ free(target);
}
free(name);
--
2.8.1
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 28/29] commit_ref_update(): remove the flags parameter
commit_ref_update() is now only called with flags=0. So remove the flags
parameter entirely.
Signed-off-by: Michael Haggerty
---
refs/files-backend.c | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 40ed157..938871b 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2568,7 +2568,7 @@ static int write_ref_to_lockfile(struct ref_lock *lock,
const unsigned char *sha1, struct strbuf *err);
static int commit_ref_update(struct ref_lock *lock,
const unsigned char *sha1, const char *logmsg,
-int flags, struct strbuf *err);
+struct strbuf *err);
int rename_ref(const char *oldrefname, const char *newrefname, const char
*logmsg)
{
@@ -2636,7 +2636,7 @@ int rename_ref(const char *oldrefname, const char
*newrefname, const char *logms
hashcpy(lock->old_oid.hash, orig_sha1);
if (write_ref_to_lockfile(lock, orig_sha1, &err) ||
- commit_ref_update(lock, orig_sha1, logmsg, 0, &err)) {
+ commit_ref_update(lock, orig_sha1, logmsg, &err)) {
error("unable to write current sha1 into %s: %s", newrefname,
err.buf);
strbuf_release(&err);
goto rollback;
@@ -2656,7 +2656,7 @@ int rename_ref(const char *oldrefname, const char
*newrefname, const char *logms
flag = log_all_ref_updates;
log_all_ref_updates = 0;
if (write_ref_to_lockfile(lock, orig_sha1, &err) ||
- commit_ref_update(lock, orig_sha1, NULL, 0, &err)) {
+ commit_ref_update(lock, orig_sha1, NULL, &err)) {
error("unable to write current sha1 into %s: %s", oldrefname,
err.buf);
strbuf_release(&err);
}
@@ -2870,12 +2870,12 @@ static int write_ref_to_lockfile(struct ref_lock *lock,
*/
static int commit_ref_update(struct ref_lock *lock,
const unsigned char *sha1, const char *logmsg,
-int flags, struct strbuf *err)
+struct strbuf *err)
{
clear_loose_ref_cache(&ref_cache);
- if (log_ref_write(lock->ref_name, lock->old_oid.hash, sha1, logmsg,
flags, err) < 0 ||
+ if (log_ref_write(lock->ref_name, lock->old_oid.hash, sha1, logmsg, 0,
err) < 0 ||
(strcmp(lock->ref_name, lock->orig_ref_name) &&
-log_ref_write(lock->orig_ref_name, lock->old_oid.hash, sha1,
logmsg, flags, err) < 0)) {
+log_ref_write(lock->orig_ref_name, lock->old_oid.hash, sha1,
logmsg, 0, err) < 0)) {
char *old_msg = strbuf_detach(err, NULL);
strbuf_addf(err, "cannot update the ref '%s': %s",
lock->ref_name, old_msg);
@@ -2911,7 +2911,7 @@ static int commit_ref_update(struct ref_lock *lock,
}
}
}
- if (!(flags & REF_LOG_ONLY) && commit_ref(lock)) {
+ if (commit_ref(lock)) {
strbuf_addf(err, "couldn't set '%s'", lock->ref_name);
unlock_ref(lock);
return -1;
--
2.8.1
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 25/29] refs: resolve symbolic refs first
Before committing ref updates, split symbolic ref updates into two
parts: an update to the underlying ref, and a log-only update to the
symbolic ref. This ensures that both references are locked correctly
during the transaction, including while their reflogs are updated.
Similarly, if the reference pointed to by HEAD is modified directly, add
a separate log-only update to HEAD, rather than leaving the job of
updating HEAD's reflog to commit_ref_update(). This change ensures that
HEAD is locked correctly while its reflog is being modified, as well as
being cheaper (HEAD only needs to be resolved once).
This makes use of a new function, lock_ref_raw(), which is analogous to
read_ref_raw(), but acquires a lock on the reference before reading it.
This change still has two problems:
* There are redundant read_ref_full() reference lookups.
* It is still possible to get incorrect reflogs for symbolic references
if there is a concurrent update by another process, since the old_oid
of a symref is determined before the lock on the pointed-to ref is
held.
Both problems will soon be fixed.
Signed-off-by: David Turner
Signed-off-by: Junio C Hamano
Signed-off-by: Michael Haggerty
---
refs/files-backend.c | 513 +-
refs/refs-internal.h | 11 +-
t/t1400-update-ref.sh | 35
3 files changed, 514 insertions(+), 45 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 8f2a795..d0e932f 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1556,6 +1556,226 @@ static void unlock_ref(struct ref_lock *lock)
}
/*
+ * Lock refname, without following symrefs, and set lock to point at a
+ * newly-allocated lock object. Fill in lock->old_oid, referent, and
+ * type similarly to read_raw_ref().
+ *
+ * The caller must verify that refname is a "safe" reference name (in
+ * the sense of refname_is_safe()) before calling this function.
+ *
+ * If the reference doesn't already exist, verify that refname doesn't
+ * have a D/F conflict with any existing references. extras and skip
+ * are passed to verify_refname_available_dir() for this check.
+ *
+ * If mustexist is not set and the reference is not found or is
+ * broken, lock the reference anyway but clear sha1.
+ *
+ * Return 0 on success. On failure, write an error message to err and
+ * return TRANSACTION_NAME_CONFLICT or TRANSACTION_GENERIC_ERROR.
+ *
+ * Implementation note: This function is basically
+ *
+ * lock reference
+ * read_raw_ref()
+ *
+ * but it includes a lot more code to
+ * - Deal with possible races with other processes
+ * - Avoid calling verify_refname_available_dir() when it can be
+ * avoided, namely if we were successfully able to read the ref
+ * - Generate informative error messages in the case of failure
+ */
+static int lock_raw_ref(const char *refname, int deleting, int mustexist,
+ const struct string_list *extras,
+ const struct string_list *skip,
+ struct ref_lock **lock_p,
+ struct strbuf *referent,
+ unsigned int *type,
+ struct strbuf *err)
+{
+ struct ref_lock *lock;
+ struct strbuf ref_file = STRBUF_INIT;
+ int attempts_remaining = 3;
+ int ret = TRANSACTION_GENERIC_ERROR;
+
+ assert(err);
+ *type = 0;
+
+ /* First lock the file so it can't change out from under us. */
+
+ *lock_p = lock = xcalloc(1, sizeof(*lock));
+
+ lock->ref_name = xstrdup(refname);
+ lock->orig_ref_name = xstrdup(refname);
+ strbuf_git_path(&ref_file, "%s", refname);
+
+retry:
+ switch (safe_create_leading_directories(ref_file.buf)) {
+ case SCLD_OK:
+ break; /* success */
+ case SCLD_EXISTS:
+ /*
+* Suppose refname is "refs/foo/bar". We just failed
+* to create the containing directory, "refs/foo",
+* because there was a non-directory in the way. This
+* indicates a D/F conflict, probably because of
+* another reference such as "refs/foo". There is no
+* reason to expect this error to be transitory.
+*/
+ if (verify_refname_available(refname, extras, skip, err)) {
+ if (mustexist) {
+ /*
+* To the user the relevant error is
+* that the "mustexist" reference is
+* missing:
+*/
+ strbuf_reset(err);
+ strbuf_addf(err, "unable to resolve reference
'%s'",
+ refname);
+ } else {
+ /*
+* The error message set by
+
[PATCH 19/29] refs: don't dereference on rename
From: David Turner
When renaming refs, don't dereference either the origin or the destination
before renaming.
The origin does not need to be dereferenced because it is presently
forbidden to rename symbolic refs.
Not dereferencing the destination fixes a bug where renaming on top of
a broken symref would use the pointed-to ref name for the moved
reflog.
Add a test for the reflog bug.
Signed-off-by: David Turner
Signed-off-by: Junio C Hamano
Signed-off-by: Michael Haggerty
---
refs/files-backend.c | 13 -
t/t3200-branch.sh| 9 +
2 files changed, 17 insertions(+), 5 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 91416db..e4cdd5a 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2358,11 +2358,12 @@ int rename_ref(const char *oldrefname, const char
*newrefname, const char *logms
struct stat loginfo;
int log = !lstat(git_path("logs/%s", oldrefname), &loginfo);
struct strbuf err = STRBUF_INIT;
+ const int resolve_flags = RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE;
if (log && S_ISLNK(loginfo.st_mode))
return error("reflog for %s is a symlink", oldrefname);
- if (!resolve_ref_unsafe(oldrefname, RESOLVE_REF_READING, orig_sha1,
&flag))
+ if (!resolve_ref_unsafe(oldrefname, resolve_flags, orig_sha1, &flag))
return error("refname %s not found", oldrefname);
if (flag & REF_ISSYMREF)
@@ -2380,8 +2381,8 @@ int rename_ref(const char *oldrefname, const char
*newrefname, const char *logms
goto rollback;
}
- if (!read_ref_full(newrefname, RESOLVE_REF_READING, sha1, NULL) &&
- delete_ref(newrefname, sha1, REF_NODEREF)) {
+ if (!read_ref_full(newrefname, resolve_flags, sha1, NULL) &&
+ delete_ref(newrefname, NULL, REF_NODEREF)) {
if (errno==EISDIR) {
struct strbuf path = STRBUF_INIT;
int result;
@@ -2405,7 +2406,8 @@ int rename_ref(const char *oldrefname, const char
*newrefname, const char *logms
logmoved = log;
- lock = lock_ref_sha1_basic(newrefname, NULL, NULL, NULL, 0, NULL, &err);
+ lock = lock_ref_sha1_basic(newrefname, NULL, NULL, NULL, REF_NODEREF,
+ NULL, &err);
if (!lock) {
error("unable to rename '%s' to '%s': %s", oldrefname,
newrefname, err.buf);
strbuf_release(&err);
@@ -2423,7 +2425,8 @@ int rename_ref(const char *oldrefname, const char
*newrefname, const char *logms
return 0;
rollback:
- lock = lock_ref_sha1_basic(oldrefname, NULL, NULL, NULL, 0, NULL, &err);
+ lock = lock_ref_sha1_basic(oldrefname, NULL, NULL, NULL, REF_NODEREF,
+ NULL, &err);
if (!lock) {
error("unable to lock %s for rollback: %s", oldrefname,
err.buf);
strbuf_release(&err);
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index f3e3b6c..4281160 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -79,6 +79,15 @@ test_expect_success 'git branch -m dumps usage' '
test_i18ngrep "branch name required" err
'
+test_expect_success 'git branch -m m broken_symref should work' '
+ test_when_finished "git branch -D broken_symref" &&
+ git branch -l m &&
+ git symbolic-ref refs/heads/broken_symref refs/heads/i_am_broken &&
+ git branch -m m broken_symref &&
+ git reflog exists refs/heads/broken_symref &&
+ test_must_fail git reflog exists refs/heads/i_am_broken
+'
+
test_expect_success 'git branch -m m m/m should work' '
git branch -l m &&
git branch -m m m/m &&
--
2.8.1
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 27/29] lock_ref_for_update(): don't resolve symrefs
If a transaction includes a non-NODEREF update to a symbolic reference,
we don't have to look it up in lock_ref_for_update(). The reference will
be dereferenced anyway when the split-off update is processed.
This change requires that we store a backpointer from the split-off
update to its parent update, for two reasons:
* We still want to report the original reference name in error messages.
So if an error occurs when checking the split-off update's old_sha1,
walk the parent_update pointers back to find the original reference
name, and report that one.
* We still need to write the old_sha1 of the symref to its reflog. So
after we read the split-off update's reference value, walk the
parent_update pointers back and fill in their old_sha1 fields.
Aside from eliminating unnecessary reads, this change fixes a
subtle (though not very serious) race condition: in the old code, the
old_sha1 of the symref was resolved before the reference that it pointed
at was locked. So it was possible that the old_sha1 value logged to the
symref's reflog could be wrong if another process changed the downstream
reference before it was locked.
Signed-off-by: Michael Haggerty
---
There is one remaining race that I know of: the value of HEAD is read
at the start of the transaction to determine whether its referent is
changed in the transaction. If so, the update is also logged in HEAD's
reflog.
The problem is that HEAD is read without acquiring its lock. So in
principle HEAD could change during the time that the transaction is
being processed, leading to inconsistencies in the reflogs.
This problem could be fixed, but it would require HEAD to be locked
for every reference transaction, probably in some kind of an
additional fake ref_update. I don't know that it's worth the effort,
but if it is it can be done within the same framework that I
implemented here.
refs/files-backend.c | 108 +--
refs/refs-internal.h | 17
2 files changed, 95 insertions(+), 30 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 66ceb83..40ed157 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3366,8 +3366,15 @@ static int split_symref_update(struct ref_update *update,
update->new_sha1, update->old_sha1,
update->msg);
- /* Change the symbolic ref update to log only: */
+ new_update->parent_update = update;
+
+ /*
+* Change the symbolic ref update to log only. Also, it
+* doesn't need to check its old SHA-1 value, as that will be
+* done when new_update is processed.
+*/
update->flags |= REF_LOG_ONLY | REF_NODEREF;
+ update->flags &= ~REF_HAVE_OLD;
item->util = new_update;
@@ -3375,6 +3382,17 @@ static int split_symref_update(struct ref_update *update,
}
/*
+ * Return the refname under which update was originally requested.
+ */
+static const char *original_update_refname(struct ref_update *update)
+{
+ while (update->parent_update)
+ update = update->parent_update;
+
+ return update->refname;
+}
+
+/*
* Prepare for carrying out update:
* - Lock the reference referred to by update.
* - Read the reference under lock.
@@ -3428,44 +3446,74 @@ static int lock_ref_for_update(struct ref_update
*update,
lock = update->lock;
if (update->type & REF_ISSYMREF) {
- if (read_ref_full(update->refname,
- mustexist ? RESOLVE_REF_READING : 0,
- lock->old_oid.hash, NULL)) {
- if (update->flags & REF_HAVE_OLD) {
- strbuf_addf(err, "cannot lock ref '%s': can't
resolve old value",
- update->refname);
+ if (update->flags & REF_NODEREF) {
+ /*
+* We won't be reading the referent as part of
+* the transaction, so we have to read it here
+* to record and possibly check old_sha1:
+*/
+ if (read_ref_full(update->refname,
+ mustexist ? RESOLVE_REF_READING : 0,
+ lock->old_oid.hash, NULL)) {
+ if (update->flags & REF_HAVE_OLD) {
+ strbuf_addf(err, "cannot lock ref '%s':
"
+ "can't resolve old value",
+ update->refname);
+ return TRANSACTION_GENERIC_ERROR;
+ } else {
+ hashclr(lock->old_oid.hash);
+ }
+ }
+ if ((update->flags & REF_HAVE_OLD) &&
+
[PATCH 01/29] safe_create_leading_directories(): improve docstring
Document the difference between this function and
safe_create_leading_directories_const(), and that the former restores
path before returning.
Signed-off-by: Michael Haggerty
---
cache.h | 5 +
1 file changed, 5 insertions(+)
diff --git a/cache.h b/cache.h
index 2711048..4134f64 100644
--- a/cache.h
+++ b/cache.h
@@ -993,6 +993,11 @@ int adjust_shared_perm(const char *path);
* directory while we were working. To be robust against this kind of
* race, callers might want to try invoking the function again when it
* returns SCLD_VANISHED.
+ *
+ * safe_create_leading_directories() temporarily changes path while it
+ * is working but restores it before returning.
+ * safe_create_leading_directories_const() doesn't modify path, even
+ * temporarily.
*/
enum scld_error {
SCLD_OK = 0,
--
2.8.1
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 14/29] refs: make error messages more consistent
* Always start error messages with a lower-case letter.
* Always enclose reference names in single quotes.
Signed-off-by: Michael Haggerty
---
This change is not strictly needed, but I wanted to fix the old error
messages before I started adding new ones (otherwise, should the new
error messages be made to look like the old ones or should they be
corrected?) By the way, should these be internationalized?
refs.c| 8
refs/files-backend.c | 32
t/t1400-update-ref.sh | 4 ++--
3 files changed, 22 insertions(+), 22 deletions(-)
diff --git a/refs.c b/refs.c
index b18d995..ba14105 100644
--- a/refs.c
+++ b/refs.c
@@ -504,7 +504,7 @@ static int write_pseudoref(const char *pseudoref, const
unsigned char *sha1,
filename = git_path("%s", pseudoref);
fd = hold_lock_file_for_update(&lock, filename, LOCK_DIE_ON_ERROR);
if (fd < 0) {
- strbuf_addf(err, "Could not open '%s' for writing: %s",
+ strbuf_addf(err, "could not open '%s' for writing: %s",
filename, strerror(errno));
return -1;
}
@@ -515,14 +515,14 @@ static int write_pseudoref(const char *pseudoref, const
unsigned char *sha1,
if (read_ref(pseudoref, actual_old_sha1))
die("could not read ref '%s'", pseudoref);
if (hashcmp(actual_old_sha1, old_sha1)) {
- strbuf_addf(err, "Unexpected sha1 when writing %s",
pseudoref);
+ strbuf_addf(err, "unexpected sha1 when writing '%s'",
pseudoref);
rollback_lock_file(&lock);
goto done;
}
}
if (write_in_full(fd, buf.buf, buf.len) != buf.len) {
- strbuf_addf(err, "Could not write to '%s'", filename);
+ strbuf_addf(err, "could not write to '%s'", filename);
rollback_lock_file(&lock);
goto done;
}
@@ -792,7 +792,7 @@ int ref_transaction_update(struct ref_transaction
*transaction,
if (new_sha1 && !is_null_sha1(new_sha1) &&
check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
- strbuf_addf(err, "refusing to update ref with bad name %s",
+ strbuf_addf(err, "refusing to update ref with bad name '%s'",
refname);
return -1;
}
diff --git a/refs/files-backend.c b/refs/files-backend.c
index b8c1779..9faf17c 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1730,7 +1730,7 @@ static int verify_lock(struct ref_lock *lock,
lock->old_oid.hash, NULL)) {
if (old_sha1) {
int save_errno = errno;
- strbuf_addf(err, "can't verify ref %s", lock->ref_name);
+ strbuf_addf(err, "can't verify ref '%s'",
lock->ref_name);
errno = save_errno;
return -1;
} else {
@@ -1739,7 +1739,7 @@ static int verify_lock(struct ref_lock *lock,
}
}
if (old_sha1 && hashcmp(lock->old_oid.hash, old_sha1)) {
- strbuf_addf(err, "ref %s is at %s but expected %s",
+ strbuf_addf(err, "ref '%s' is at %s but expected %s",
lock->ref_name,
sha1_to_hex(lock->old_oid.hash),
sha1_to_hex(old_sha1));
@@ -1819,7 +1819,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char
*refname,
if (last_errno != ENOTDIR ||
!verify_refname_available_dir(orig_refname, extras, skip,
get_loose_refs(&ref_cache),
err))
- strbuf_addf(err, "unable to resolve reference %s: %s",
+ strbuf_addf(err, "unable to resolve reference '%s': %s",
orig_refname, strerror(last_errno));
goto error_return;
@@ -1857,7 +1857,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char
*refname,
/* fall through */
default:
last_errno = errno;
- strbuf_addf(err, "unable to create directory for %s",
+ strbuf_addf(err, "unable to create directory for '%s'",
ref_file.buf);
goto error_return;
}
@@ -2478,7 +2478,7 @@ static int log_ref_setup(const char *refname, struct
strbuf *logfile, struct str
strbuf_git_path(logfile, "logs/%s", refname);
if (force_create || should_autocreate_reflog(refname)) {
if (safe_create_leading_directories(logfile->buf) < 0) {
- strbuf_addf(err, "unable to create directory for %s: "
+ strbuf_addf(err, "unable to create directory for '%s': "
"%s", l
[PATCH 02/29] remove_dir_recursively(): add docstring
Add a docstring for the remove_dir_recursively() function and the REMOVE_DIR_* flags that can be passed to it. Signed-off-by: Michael Haggerty --- dir.h | 23 +++ 1 file changed, 23 insertions(+) diff --git a/dir.h b/dir.h index 301b737..5f19acc 100644 --- a/dir.h +++ b/dir.h @@ -262,9 +262,32 @@ extern int is_empty_dir(const char *dir); extern void setup_standard_excludes(struct dir_struct *dir); + +/* Constants for remove_dir_recursively: */ + +/* + * If a non-directory is found within path, stop and return an error. + * (In this case some empty directories might already have been + * removed.) + */ #define REMOVE_DIR_EMPTY_ONLY 01 + +/* + * If any Git work trees are found within path, skip them without + * considering it an error. + */ #define REMOVE_DIR_KEEP_NESTED_GIT 02 + +/* Remove the contents of path, but leave path itself. */ #define REMOVE_DIR_KEEP_TOPLEVEL 04 + +/* + * Remove path and its contents, recursively. flags is a combination + * of the above REMOVE_DIR_* constants. Return 0 on success. + * + * This function uses path as temporary scratch space, but restores it + * before returning. + */ extern int remove_dir_recursively(struct strbuf *path, int flag); /* tries to remove the path with empty directories along it, ignores ENOENT */ -- 2.8.1 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 15/29] ref_transaction_create(): disallow recursive pruning
It is nonsensical (and a little bit dangerous) to use REF_ISPRUNING
without REF_NODEREF. Forbid it explicitly. Change the one REF_ISPRUNING
caller to pass REF_NODEREF too.
Signed-off-by: Michael Haggerty
---
This also makes later patches a bit clearer.
refs.c | 3 +++
refs/files-backend.c | 2 +-
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/refs.c b/refs.c
index ba14105..5dc2473 100644
--- a/refs.c
+++ b/refs.c
@@ -790,6 +790,9 @@ int ref_transaction_update(struct ref_transaction
*transaction,
if (transaction->state != REF_TRANSACTION_OPEN)
die("BUG: update called for transaction that is not open");
+ if ((flags & REF_ISPRUNING) && !(flags & REF_NODEREF))
+ die("BUG: REF_ISPRUNING set without REF_NODEREF");
+
if (new_sha1 && !is_null_sha1(new_sha1) &&
check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
strbuf_addf(err, "refusing to update ref with bad name '%s'",
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 9faf17c..8fcbd7d 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2116,7 +2116,7 @@ static void prune_ref(struct ref_to_prune *r)
transaction = ref_transaction_begin(&err);
if (!transaction ||
ref_transaction_delete(transaction, r->name, r->sha1,
- REF_ISPRUNING, NULL, &err) ||
+ REF_ISPRUNING | REF_NODEREF, NULL, &err) ||
ref_transaction_commit(transaction, &err)) {
ref_transaction_free(transaction);
error("%s", err.buf);
--
2.8.1
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 29/29] lock_ref_sha1_basic(): only handle REF_NODEREF mode
Now lock_ref_sha1_basic() is only called with flags==REF_NODEREF. So we
don't have to handle other cases anymore.
This enables several simplifications, the most interesting of which come
from the fact that ref_lock::orig_ref_name is now always the same as
ref_lock::ref_name:
* Remove ref_lock::orig_ref_name
* Remove local variable orig_refname from lock_ref_sha1_basic()
* commit_ref_update() never has to write to the reflog for
lock->orig_ref_name
Signed-off-by: Michael Haggerty
---
refs/files-backend.c | 54
1 file changed, 21 insertions(+), 33 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 938871b..5dca352 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -7,7 +7,6 @@
struct ref_lock {
char *ref_name;
- char *orig_ref_name;
struct lock_file *lk;
struct object_id old_oid;
};
@@ -1551,7 +1550,6 @@ static void unlock_ref(struct ref_lock *lock)
if (lock->lk)
rollback_lock_file(lock->lk);
free(lock->ref_name);
- free(lock->orig_ref_name);
free(lock);
}
@@ -1605,7 +1603,6 @@ static int lock_raw_ref(const char *refname, int
deleting, int mustexist,
*lock_p = lock = xcalloc(1, sizeof(*lock));
lock->ref_name = xstrdup(refname);
- lock->orig_ref_name = xstrdup(refname);
strbuf_git_path(&ref_file, "%s", refname);
retry:
@@ -1991,14 +1988,13 @@ static struct ref_lock *lock_ref_sha1_basic(const char
*refname,
struct strbuf *err)
{
struct strbuf ref_file = STRBUF_INIT;
- struct strbuf orig_ref_file = STRBUF_INIT;
- const char *orig_refname = refname;
struct ref_lock *lock;
int last_errno = 0;
- int lflags = 0;
+ int lflags = LOCK_NO_DEREF;
int mustexist = (old_sha1 && !is_null_sha1(old_sha1));
- int resolve_flags = 0;
+ int resolve_flags = RESOLVE_REF_NO_RECURSE;
int attempts_remaining = 3;
+ int resolved;
assert(err);
@@ -2008,46 +2004,39 @@ static struct ref_lock *lock_ref_sha1_basic(const char
*refname,
resolve_flags |= RESOLVE_REF_READING;
if (flags & REF_DELETING)
resolve_flags |= RESOLVE_REF_ALLOW_BAD_NAME;
- if (flags & REF_NODEREF) {
- resolve_flags |= RESOLVE_REF_NO_RECURSE;
- lflags |= LOCK_NO_DEREF;
- }
- refname = resolve_ref_unsafe(refname, resolve_flags,
-lock->old_oid.hash, type);
- if (!refname && errno == EISDIR) {
+ resolved = !!resolve_ref_unsafe(refname, resolve_flags,
+ lock->old_oid.hash, type);
+ if (!resolved && errno == EISDIR) {
/*
* we are trying to lock foo but we used to
* have foo/bar which now does not exist;
* it is normal for the empty directory 'foo'
* to remain.
*/
- strbuf_git_path(&orig_ref_file, "%s", orig_refname);
- if (remove_empty_directories(&orig_ref_file)) {
+ strbuf_git_path(&ref_file, "%s", refname);
+ if (remove_empty_directories(&ref_file)) {
last_errno = errno;
- if (!verify_refname_available_dir(orig_refname, extras,
skip,
+ if (!verify_refname_available_dir(refname, extras, skip,
get_loose_refs(&ref_cache), err))
strbuf_addf(err, "there are still refs under
'%s'",
- orig_refname);
+ refname);
goto error_return;
}
- refname = resolve_ref_unsafe(orig_refname, resolve_flags,
-lock->old_oid.hash, type);
+ resolved = !!resolve_ref_unsafe(refname, resolve_flags,
+ lock->old_oid.hash, type);
}
- if (!refname) {
+ if (!resolved) {
last_errno = errno;
if (last_errno != ENOTDIR ||
- !verify_refname_available_dir(orig_refname, extras, skip,
+ !verify_refname_available_dir(refname, extras, skip,
get_loose_refs(&ref_cache),
err))
strbuf_addf(err, "unable to resolve reference '%s': %s",
- orig_refname, strerror(last_errno));
+ refname, strerror(last_errno));
goto error_return;
}
- if (flags & REF_NODEREF)
- refname = orig_refname;
-
/*
* If the ref did not exist and we are creating it, make sure
* t
[PATCH 04/29] refname_is_safe(): don't allow the empty string
Signed-off-by: Michael Haggerty
---
This fixes a coding error from the original implementation.
refs.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/refs.c b/refs.c
index 5789152..ca0280f 100644
--- a/refs.c
+++ b/refs.c
@@ -136,11 +136,12 @@ int refname_is_safe(const char *refname)
free(buf);
return result;
}
- while (*refname) {
+
+ do {
if (!isupper(*refname) && *refname != '_')
return 0;
refname++;
- }
+ } while (*refname);
return 1;
}
--
2.8.1
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 08/29] ref_transaction_commit(): remove local variable n
This microoptimization doesn't make a significant difference in speed.
And it causes problems if somebody ever wants to modify the function to
add updates to a transaction as part of processing it, as will happen
shortly.
Make the same change in initial_ref_transaction_commit().
Signed-off-by: Michael Haggerty
---
refs/files-backend.c | 20 +---
1 file changed, 9 insertions(+), 11 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 0ade681..05797cb 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3043,7 +3043,6 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
struct strbuf *err)
{
int ret = 0, i;
- int n = transaction->nr;
struct ref_update **updates = transaction->updates;
struct string_list refs_to_delete = STRING_LIST_INIT_NODUP;
struct string_list_item *ref_to_delete;
@@ -3054,13 +3053,13 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
if (transaction->state != REF_TRANSACTION_OPEN)
die("BUG: commit called for transaction that is not open");
- if (!n) {
+ if (!transaction->nr) {
transaction->state = REF_TRANSACTION_CLOSED;
return 0;
}
/* Fail if a refname appears more than once in the transaction: */
- for (i = 0; i < n; i++)
+ for (i = 0; i < transaction->nr; i++)
string_list_append(&affected_refnames, updates[i]->refname);
string_list_sort(&affected_refnames);
if (ref_update_reject_duplicates(&affected_refnames, err)) {
@@ -3074,7 +3073,7 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
* lockfiles, ready to be activated. Only keep one lockfile
* open at a time to avoid running out of file descriptors.
*/
- for (i = 0; i < n; i++) {
+ for (i = 0; i < transaction->nr; i++) {
struct ref_update *update = updates[i];
if ((update->flags & REF_HAVE_NEW) &&
@@ -3145,7 +3144,7 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
}
/* Perform updates first so live commits remain referenced */
- for (i = 0; i < n; i++) {
+ for (i = 0; i < transaction->nr; i++) {
struct ref_update *update = updates[i];
if (update->flags & REF_NEEDS_COMMIT) {
@@ -3164,7 +3163,7 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
}
/* Perform deletes now that updates are safely completed */
- for (i = 0; i < n; i++) {
+ for (i = 0; i < transaction->nr; i++) {
struct ref_update *update = updates[i];
if (update->flags & REF_DELETING) {
@@ -3190,7 +3189,7 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
cleanup:
transaction->state = REF_TRANSACTION_CLOSED;
- for (i = 0; i < n; i++)
+ for (i = 0; i < transaction->nr; i++)
if (updates[i]->lock)
unlock_ref(updates[i]->lock);
string_list_clear(&refs_to_delete, 0);
@@ -3210,7 +3209,6 @@ int initial_ref_transaction_commit(struct ref_transaction
*transaction,
struct strbuf *err)
{
int ret = 0, i;
- int n = transaction->nr;
struct ref_update **updates = transaction->updates;
struct string_list affected_refnames = STRING_LIST_INIT_NODUP;
@@ -3220,7 +3218,7 @@ int initial_ref_transaction_commit(struct ref_transaction
*transaction,
die("BUG: commit called for transaction that is not open");
/* Fail if a refname appears more than once in the transaction: */
- for (i = 0; i < n; i++)
+ for (i = 0; i < transaction->nr; i++)
string_list_append(&affected_refnames, updates[i]->refname);
string_list_sort(&affected_refnames);
if (ref_update_reject_duplicates(&affected_refnames, err)) {
@@ -3243,7 +3241,7 @@ int initial_ref_transaction_commit(struct ref_transaction
*transaction,
if (for_each_rawref(ref_present, &affected_refnames))
die("BUG: initial ref transaction called with existing refs");
- for (i = 0; i < n; i++) {
+ for (i = 0; i < transaction->nr; i++) {
struct ref_update *update = updates[i];
if ((update->flags & REF_HAVE_OLD) &&
@@ -3264,7 +3262,7 @@ int initial_ref_transaction_commit(struct ref_transaction
*transaction,
goto cleanup;
}
- for (i = 0; i < n; i++) {
+ for (i = 0; i < transaction->nr; i++) {
struct ref_update *update = updates[i];
if ((update->flags & REF_HAVE_NEW) &&
--
2.8.1
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 03/29] refname_is_safe(): use skip_prefix()
Signed-off-by: Michael Haggerty
---
refs.c | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/refs.c b/refs.c
index 87dc82f..5789152 100644
--- a/refs.c
+++ b/refs.c
@@ -120,17 +120,19 @@ int check_refname_format(const char *refname, int flags)
int refname_is_safe(const char *refname)
{
- if (starts_with(refname, "refs/")) {
+ const char *rest;
+
+ if (skip_prefix(refname, "refs/", &rest)) {
char *buf;
int result;
- buf = xmallocz(strlen(refname));
/*
* Does the refname try to escape refs/?
* For example: refs/foo/../bar is safe but refs/foo/../../bar
* is not.
*/
- result = !normalize_path_copy(buf, refname + strlen("refs/"));
+ buf = xmallocz(strlen(rest));
+ result = !normalize_path_copy(buf, rest);
free(buf);
return result;
}
--
2.8.1
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 23/29] unlock_ref(): move definition higher in the file
This avoids the need for a forward declaration in the next patch.
Signed-off-by: Michael Haggerty
---
refs/files-backend.c | 20 ++--
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 546656a..8f2a795 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1545,6 +1545,16 @@ out:
return ret;
}
+static void unlock_ref(struct ref_lock *lock)
+{
+ /* Do not free lock->lk -- atexit() still looks at them */
+ if (lock->lk)
+ rollback_lock_file(lock->lk);
+ free(lock->ref_name);
+ free(lock->orig_ref_name);
+ free(lock);
+}
+
/*
* Peel the entry (if possible) and return its new peel_status. If
* repeel is true, re-peel the entry even if there is an old peeled
@@ -1703,16 +1713,6 @@ int do_for_each_ref(const char *submodule, const char
*base,
return do_for_each_entry(refs, base, do_one_ref, &data);
}
-static void unlock_ref(struct ref_lock *lock)
-{
- /* Do not free lock->lk -- atexit() still looks at them */
- if (lock->lk)
- rollback_lock_file(lock->lk);
- free(lock->ref_name);
- free(lock->orig_ref_name);
- free(lock);
-}
-
/*
* Verify that the reference locked by lock has the value old_sha1.
* Fail if the reference doesn't exist and mustexist is set. Return 0
--
2.8.1
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 11/29] read_raw_ref(): rename symref argument to referent
After all, it doesn't hold the symbolic reference, but rather the
reference referred to.
Signed-off-by: Michael Haggerty
---
refs/files-backend.c | 21 +++--
refs/refs-internal.h | 2 +-
2 files changed, 12 insertions(+), 11 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index f10c80f..364425c 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1393,9 +1393,10 @@ static int resolve_missing_loose_ref(const char *refname,
*
* If the ref is a sha1, fill in sha1 and return 0.
*
- * If the ref is symbolic, fill in *symref with the referrent
- * (e.g. "refs/heads/master") and return 0. The caller is responsible
- * for validating the referrent. Set REF_ISSYMREF in type.
+ * If the ref is symbolic, fill in *referent with the name of the
+ * branch to which it refers (e.g. "refs/heads/master") and return 0.
+ * The caller is responsible for validating the referent. Set
+ * REF_ISSYMREF in type.
*
* If the ref doesn't exist, set errno to ENOENT and return -1.
*
@@ -1411,15 +1412,15 @@ static int resolve_missing_loose_ref(const char
*refname,
*
* sb_path is workspace: the caller should allocate and free it.
*
- * It is OK for refname to point into symref. In this case:
- * - if the function succeeds with REF_ISSYMREF, symref will be
+ * It is OK for refname to point into referent. In this case:
+ * - if the function succeeds with REF_ISSYMREF, referent will be
* overwritten and the memory pointed to by refname might be changed
* or even freed.
- * - in all other cases, symref will be untouched, and therefore
+ * - in all other cases, referent will be untouched, and therefore
* refname will still be valid and unchanged.
*/
int read_raw_ref(const char *refname, unsigned char *sha1,
-struct strbuf *symref, unsigned int *type)
+struct strbuf *referent, unsigned int *type)
{
struct strbuf sb_contents = STRBUF_INIT;
struct strbuf sb_path = STRBUF_INIT;
@@ -1469,7 +1470,7 @@ stat_ref:
}
if (starts_with(sb_contents.buf, "refs/") &&
!check_refname_format(sb_contents.buf, 0)) {
- strbuf_swap(&sb_contents, symref);
+ strbuf_swap(&sb_contents, referent);
*type |= REF_ISSYMREF;
ret = 0;
goto out;
@@ -1509,8 +1510,8 @@ stat_ref:
while (isspace(*buf))
buf++;
- strbuf_reset(symref);
- strbuf_addstr(symref, buf);
+ strbuf_reset(referent);
+ strbuf_addstr(referent, buf);
*type |= REF_ISSYMREF;
ret = 0;
goto out;
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 0b047f6..37a1a37 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -210,6 +210,6 @@ int do_for_each_ref(const char *submodule, const char *base,
each_ref_fn fn, int trim, int flags, void *cb_data);
int read_raw_ref(const char *refname, unsigned char *sha1,
-struct strbuf *symref, unsigned int *type);
+struct strbuf *referent, unsigned int *type);
#endif /* REFS_REFS_INTERNAL_H */
--
2.8.1
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
