* Re: [PATCH] fsck: do not print dangling objects by default
From: Junio C Hamano @ 2012-02-26 22:46 UTC (permalink / raw)
To: Clemens Buchacher; +Cc: git
In-Reply-To: <7vty2ddzqj.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> I think that both the ultimate goal explained above, and the direction in
> which the documentation updates tries to move us, are good. I only gave a
> cursory look at the code changes, but what they implement seems to match
> the intention.
>
> Of course I may be missing something, so objections from others to argue
> why we shouldn't do this is very much welcomed to stop me and Clemens ;-).
Let's start with the obvious.
It is much easier for a user to use a new option on the command line when
he wants to use an improved behaviour when he runs the command manually.
Having to update scripts that run the command to act on its output, on the
other hand, is much more painful to the users.
And the intended audience for this change clearly is interactive users
that follow the user-manual to try things out.
Given that, isn't it not just sufficient but actually better to instead
add a new --no-dangling option and keep the default unchanged?
^ permalink raw reply
* Re: [PATCH] grep -P: Fix matching ^ and $
From: Junio C Hamano @ 2012-02-26 22:39 UTC (permalink / raw)
To: Michał Kiedrowicz; +Cc: git, Zbigniew Jędrzejewski-Szmek
In-Reply-To: <1330161868-7954-1-git-send-email-michal.kiedrowicz@gmail.com>
Michał Kiedrowicz <michal.kiedrowicz@gmail.com> writes:
> @@ -893,4 +900,20 @@ test_expect_success 'mimic ack-grep --group' '
> test_cmp expected actual
> '
>
> +cat >expected <<EOF
> +space: line with leading space1
> +space: line with leading space2
> +space: line with leading space3
> +EOF
> +
> +test_expect_success 'grep -E "^ "' '
> + git grep -E "^ " space >actual &&
> + test_cmp expected actual
> +'
> +
> +test_expect_success "grep -P '^ '" '
> + git grep -P "^ " space >actual &&
> + test_cmp expected actual
> +'
This test does not pass for me as I do not usually build with pcre;
shouldn't it be protected with some test prerequisite?
Otherwise the patch looks good; thanks.
^ permalink raw reply
* Re: [PATCH] Allow Overriding GIT_BUILD_DIR
From: Junio C Hamano @ 2012-02-26 22:19 UTC (permalink / raw)
To: David A. Greene; +Cc: git
In-Reply-To: <87vcmu5psm.fsf@smith.obbligato.org>
"David A. Greene" <greened@obbligato.org> writes:
> Let tests override GIT_BUILD_DIR so git will work if tests are not at
> the same directory level as standard git tests. Prior to this change,
> GIT_BUILD_DIR is hardwired to be exactly one directory above where the
> test lives. A test within contrib/, for example, can now use
> test-lib.sh and set an appropriate value for GIT_BUILD_DIR.
Ok, this is getting closer. We use GIT_BUILD_DIR to find out crucial bits
of the build environment in order to run tests, like the binaries being
tested that are in $GIT_BUILD_DIR/bin-wrappers, but we set GIT_BUILD_DIR
always to one level above where the test script being run is (because they
are typically t/t1234-name.sh). By making them able to name GIT_BUILD_DIR
directly to a place that is different from a level above "$TEST_DIRECTORY",
a test script can live anywhere.
There are two more things that worries me a bit.
One is what TEST_DIRECTORY should mean in the new world order. We use it
to find where the test-lib.sh and other lib-*.sh helper definitions are,
also we use it to find large-ish test vectors like t3900/ and t4013/. If
an external test script t1234-git-subtree.sh wants to use a separate file
to keep its own helper definitions, how should it name it? It cannot be
relative to TEST_DIRECTORY that is typically "t/". It cannot be relative
to "../" as TRASH_DIRECTORY where the script runs, as the --root option
may move it elsewhere on the filesystem (and is the reason TEST_DIRECTORY
variable exists in the first place).
And how well does an external test script work with the --root option that
moves the TEST_DIRECTORY?
> Signed-off-by: David A. Greene <greened@obbligato.org>
> ---
> t/test-lib.sh | 10 +++++++++-
> 1 files changed, 9 insertions(+), 1 deletions(-)
>
>
> ------------------
>
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index a65dfc7..4585138 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -55,6 +55,7 @@ unset $(perl -e '
> .*_TEST
> PROVE
> VALGRIND
> + BUILD_DIR
A funny indentation found here.
> ));
> @@ -901,7 +902,14 @@ then
> # itself.
> TEST_DIRECTORY=$(pwd)
> fi
> -GIT_BUILD_DIR="$TEST_DIRECTORY"/..
> +
> +if test -z "$GIT_BUILD_DIR"
> +then
> + # We allow tests to override this, in case they want to run tests
> + # outside of t/, e.g. for running tests on the test library
> + # itself.
# For in-tree test scripts, this is one level above the TEST_DIRECTORY
# (t/), but a test script that lives outside t/ can set this variable to
# point at the right place so that it can find t/ directory that house
# test helpers like lib-pager*.sh and test vectors like t4013/.
> + GIT_BUILD_DIR="$TEST_DIRECTORY"/..
> +fi
>
> if test -n "$valgrind"
> then
>
> --------------------
^ permalink raw reply
* Re: [PATCH] fsck: do not print dangling objects by default
From: Junio C Hamano @ 2012-02-26 21:57 UTC (permalink / raw)
To: Clemens Buchacher; +Cc: git
In-Reply-To: <20120226204357.GA26088@ecki>
Clemens Buchacher <drizzd@aon.at> writes:
> Every reference to git fsck that I could find in the documentation or on
> the web is followed by the some kind of assurance that dangling objects
> are "not a problem", "nothing to worry about" or "not at all
> interesting".
>
> Instead of telling the user everywhere to ignore those messages, let's
> not print them in the first place. The --dangling flag can be used to
> explicitly enable printing dangling objects.
>
> Signed-off-by: Clemens Buchacher <drizzd@aon.at>
Thanks.
I think that both the ultimate goal explained above, and the direction in
which the documentation updates tries to move us, are good. I only gave a
cursory look at the code changes, but what they implement seems to match
the intention.
Of course I may be missing something, so objections from others to argue
why we shouldn't do this is very much welcomed to stop me and Clemens ;-).
> diff --git a/Documentation/user-manual.txt b/Documentation/user-manual.txt
> index f13a846..09ffbcc 100644
> --- a/Documentation/user-manual.txt
> +++ b/Documentation/user-manual.txt
> @@ -1582,25 +1582,12 @@ Checking the repository for corruption
>
> The linkgit:git-fsck[1] command runs a number of self-consistency checks
> on the repository, and reports on any problems. This may take some
> +time.
> ...
> -Dangling objects are not a problem. At worst they may take up a little
> -extra disk space. They can sometimes provide a last-resort method for
> -recovering lost work--see <<dangling-objects>> for details.
Losing this description is not a problem and is indeed an improvement in
this section in the user manual, but let's make a mental note that the
user needs to also learn what the third sentence says elsewhere in this
document.
<< reads on >>
> @@ -1665,7 +1652,7 @@ commits in the dangling objects that `git fsck` reports. See
> <<dangling-objects>> for the details.
>
> -------------------------------------------------
> -$ git fsck
> +$ git fsck --dangling
> dangling commit 7281251ddd2a61e38657c827739c57015671a6b3
> dangling commit 2706a059f258c6b245f298dc4ff2ccd30ec21a63
> dangling commit 13472b7c4b80851a1bc551779171dcb03655e9b5
> @@ -3301,9 +3288,6 @@ broken link from tree 2d9263c6d23595e7cb2a21e5ebbb53655278dff8
> missing blob 4b9458b3786228369c63936db65827de3cc06200
> ------------------------------------------------
>
> -(Typically there will be some "dangling object" messages too, but they
> -aren't interesting.)
> -
The old sentence becomes stale as the example _explicitly_ asks the
command to show the dangling ones.
> Now you know that blob 4b9458b3 is missing, and that the tree 2d9263c6
> points to it. If you could find just one copy of that missing blob
> object, possibly in some other repository, you could move it into
And this part, together with the introductory text of the section (not
shown in the context), says what the "third sentence" above wanted to say.
So overall I think this is a vast improvement without losing any
information or clarity.
Nice.
^ permalink raw reply
* Re: sha-1 check in rev-list --verify-objects redundant?
From: Junio C Hamano @ 2012-02-26 21:37 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: Git Mailing List
In-Reply-To: <CACsJy8BUeedTZSq_ay=JmqUt3wrnm6n1eOcFt0WPkEo2B-1zwA@mail.gmail.com>
Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
> On the well-formedness, unless I'm mistaken, --verify-objects is
> _always_ used in conjunction with index-pack.
Hmm, you are making my head hurt. Is the above "always" a typo of
"never"?
The static check_everything_connected() function in builtin/fetch.c is a
direct callsite of "rev-list --verify-objects", and the function is used
in two codepaths:
* store_updated_refs() that is used after we receive and store objects
from the other end. We may or may not have run index-pack in this
codepath; in either case we need to make sure the other side did send
everything that is needed to complete the history between what we used
to have and what they claimed to supply us, to protect us from a broken
remote side.
* quickfetch() that is called even before we get any object from the
other end, to optimize the transfer when we already have what we need.
The latter is the original use to protect against unconnected island of
chain I explained in the previous message, but the former is also abot the
same protection, in a different callchain.
In both cases, the check by --verify-objects is about completeness of the
history (is everything connected to the tips of refs we have?), and is
different from integrity of individual objects (is each individual object
well formed and hash correctly?). Both kinds of sanity need to be
checked, as they are orthogonal concepts.
In order to check the history completeness, we need to read the objects
that we walk during the check. I wouldn't be surprised if the codepath to
do this is written overly defensive, taking a belt-and-suspender approach,
and check the well-formedness of an object before it reads it to find out
the other objects pointed by it.
If we _know_ that we have checked the integrity of all the necessary
individual objects before we start reading them in order to check the
completeness of the history, there is an opportunity to optimize by
teaching --verify-objects paths to optionally be looser than it currently
is, to avoid checking the object integrity twice.
^ permalink raw reply
* [PATCH] fsck: do not print dangling objects by default
From: Clemens Buchacher @ 2012-02-26 20:43 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
Every reference to git fsck that I could find in the documentation or on
the web is followed by the some kind of assurance that dangling objects
are "not a problem", "nothing to worry about" or "not at all
interesting".
Instead of telling the user everywhere to ignore those messages, let's
not print them in the first place. The --dangling flag can be used to
explicitly enable printing dangling objects.
Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---
Documentation/git-fsck.txt | 3 +++
Documentation/git-repack.txt | 2 +-
Documentation/user-manual.txt | 20 ++------------------
builtin/fsck.c | 7 +++++--
t/t1410-reflog.sh | 2 +-
t/t1450-fsck.sh | 6 +-----
t/t6050-replace.sh | 2 +-
7 files changed, 14 insertions(+), 28 deletions(-)
diff --git a/Documentation/git-fsck.txt b/Documentation/git-fsck.txt
index 6c47395..199c5a0 100644
--- a/Documentation/git-fsck.txt
+++ b/Documentation/git-fsck.txt
@@ -30,6 +30,9 @@ index file, all SHA1 references in .git/refs/*, and all reflogs (unless
Print out objects that exist but that aren't reachable from any
of the reference nodes.
+--dangling::
+ Print objects that exist but that are never 'directly' used.
+
--root::
Report root nodes.
diff --git a/Documentation/git-repack.txt b/Documentation/git-repack.txt
index 40af321..4c1aff6 100644
--- a/Documentation/git-repack.txt
+++ b/Documentation/git-repack.txt
@@ -34,7 +34,7 @@ OPTIONS
Especially useful when packing a repository that is used
for private development. Use
with '-d'. This will clean up the objects that `git prune`
- leaves behind, but `git fsck --full` shows as
+ leaves behind, but `git fsck --full --dangling` shows as
dangling.
+
Note that users fetching over dumb protocols will have to fetch the
diff --git a/Documentation/user-manual.txt b/Documentation/user-manual.txt
index f13a846..09ffbcc 100644
--- a/Documentation/user-manual.txt
+++ b/Documentation/user-manual.txt
@@ -1582,25 +1582,12 @@ Checking the repository for corruption
The linkgit:git-fsck[1] command runs a number of self-consistency checks
on the repository, and reports on any problems. This may take some
-time. The most common warning by far is about "dangling" objects:
+time.
-------------------------------------------------
$ git fsck
-dangling commit 7281251ddd2a61e38657c827739c57015671a6b3
-dangling commit 2706a059f258c6b245f298dc4ff2ccd30ec21a63
-dangling commit 13472b7c4b80851a1bc551779171dcb03655e9b5
-dangling blob 218761f9d90712d37a9c5e36f406f92202db07eb
-dangling commit bf093535a34a4d35731aa2bd90fe6b176302f14f
-dangling commit 8e4bec7f2ddaa268bef999853c25755452100f8e
-dangling tree d50bb86186bf27b681d25af89d3b5b68382e4085
-dangling tree b24c2473f1fd3d91352a624795be026d64c8841f
-...
-------------------------------------------------
-Dangling objects are not a problem. At worst they may take up a little
-extra disk space. They can sometimes provide a last-resort method for
-recovering lost work--see <<dangling-objects>> for details.
-
[[recovering-lost-changes]]
Recovering lost changes
~~~~~~~~~~~~~~~~~~~~~~~
@@ -1665,7 +1652,7 @@ commits in the dangling objects that `git fsck` reports. See
<<dangling-objects>> for the details.
-------------------------------------------------
-$ git fsck
+$ git fsck --dangling
dangling commit 7281251ddd2a61e38657c827739c57015671a6b3
dangling commit 2706a059f258c6b245f298dc4ff2ccd30ec21a63
dangling commit 13472b7c4b80851a1bc551779171dcb03655e9b5
@@ -3301,9 +3288,6 @@ broken link from tree 2d9263c6d23595e7cb2a21e5ebbb53655278dff8
missing blob 4b9458b3786228369c63936db65827de3cc06200
------------------------------------------------
-(Typically there will be some "dangling object" messages too, but they
-aren't interesting.)
-
Now you know that blob 4b9458b3 is missing, and that the tree 2d9263c6
points to it. If you could find just one copy of that missing blob
object, possibly in some other repository, you could move it into
diff --git a/builtin/fsck.c b/builtin/fsck.c
index 8c479a7..0745535 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -29,6 +29,7 @@ static int errors_found;
static int write_lost_and_found;
static int verbose;
static int show_progress = -1;
+static int show_dangling;
#define ERROR_OBJECT 01
#define ERROR_REACHABLE 02
#define ERROR_PACK 04
@@ -221,8 +222,9 @@ static void check_unreachable_object(struct object *obj)
* start looking at, for example.
*/
if (!obj->used) {
- printf("dangling %s %s\n", typename(obj->type),
- sha1_to_hex(obj->sha1));
+ if (show_dangling)
+ printf("dangling %s %s\n", typename(obj->type),
+ sha1_to_hex(obj->sha1));
if (write_lost_and_found) {
char *filename = git_path("lost-found/%s/%s",
obj->type == OBJ_COMMIT ? "commit" : "other",
@@ -614,6 +616,7 @@ static char const * const fsck_usage[] = {
static struct option fsck_opts[] = {
OPT__VERBOSE(&verbose, "be verbose"),
OPT_BOOLEAN(0, "unreachable", &show_unreachable, "show unreachable objects"),
+ OPT_BOOLEAN(0, "dangling", &show_dangling, "show dangling objects"),
OPT_BOOLEAN(0, "tags", &show_tags, "report tags"),
OPT_BOOLEAN(0, "root", &show_root, "report root nodes"),
OPT_BOOLEAN(0, "cache", &keep_cache_objects, "make index objects head nodes"),
diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh
index 252fc82..12441c5 100755
--- a/t/t1410-reflog.sh
+++ b/t/t1410-reflog.sh
@@ -20,7 +20,7 @@ check_have () {
}
check_fsck () {
- output=$(git fsck --full)
+ output=$(git fsck --full --dangling)
case "$1" in
'')
test -z "$output" ;;
diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
index 523ce9c..78bfbc3 100755
--- a/t/t1450-fsck.sh
+++ b/t/t1450-fsck.sh
@@ -27,12 +27,8 @@ test_expect_success 'loose objects borrowed from alternate are not missing' '
git init &&
echo ../../../.git/objects >.git/objects/info/alternates &&
test_commit C fileC one &&
- git fsck >../out 2>&1
+ git fsck >../actual 2>&1
) &&
- {
- grep -v dangling out >actual ||
- :
- } &&
test_cmp empty actual
'
diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
index 5c87f28..5b36ee3 100755
--- a/t/t6050-replace.sh
+++ b/t/t6050-replace.sh
@@ -95,7 +95,7 @@ test_expect_success 'tag replaced commit' '
'
test_expect_success '"git fsck" works' '
- git fsck master > fsck_master.out &&
+ git fsck --dangling master > fsck_master.out &&
grep "dangling commit $R" fsck_master.out &&
grep "dangling tag $(cat .git/refs/tags/mytag)" fsck_master.out &&
test -z "$(git fsck)"
--
1.7.9.1
^ permalink raw reply related
* Re: [PATCH 0/2] submodules: Use relative paths to gitdir and work tree
From: Jens Lehmann @ 2012-02-26 19:58 UTC (permalink / raw)
To: Johannes Sixt
Cc: Junio C Hamano, Git Mailing List, Antony Male, Phil Hord, msysGit
In-Reply-To: <4F4A6DFA.5080709@kdbg.org>
Am 26.02.2012 18:38, schrieb Johannes Sixt:
> Am 08.02.2012 23:08, schrieb Jens Lehmann:
>> This patch series replaces all absolute paths pointing from submodule work
>> trees to its gitdir and back with relative paths as discussed in $gmane/187785.
>>
>> The motivation is to make superprojects movable again. They lost this ability
>> with the move of the git directory of submodules into the .git/modules directory
>> of the superproject. While fixing that a bug which would hit when moving the
>> submodule inside the superproject was also fixed.
>>
>> Jens Lehmann (2):
>> submodules: always use a relative path to gitdir
>> submodules: always use a relative path from gitdir to work tree
>
> This series, with the tip at e3307adaba in Junio's repo causes major
> headaches on Windows.
>
> First, a check for an absolute path must be extended to take
> Windows-style paths into account.
Okay, but that check is not part of my series (it was introduced by 501770e1
"Move git-dir for submodules", which is in Git since 1.7.8), so that looks
like it would need to be fixed for msysgit even without my patches, right?
But I'm not so happy about the two code paths there anyway, so I prepared a
patch to replace them with a single code path based upon the paths computed
in the last patch of this series. Please see the always-use-relative-gitdir
branch in my github repo https://github.com/jlehmann/git-submod-enhancements
> Second, the a's and b's are filled with different forms of absolute
> paths (/c/there vs. c:/there), and as a consequence the subsequent loops
> do not find a suitable relative path.
>
> The below is a minimal hack that passes all t/*submod* tests, but it
> works only on Windows, where the pwd utility has an option -W that
> prints a Windows style absolute path.
>
> How would you have this solved? One option would be to introduce a function
>
> pwd() { builtin pwd -W "$@"; }
>
> in git-sh-setup conditionally on Windows (but that would affect other
> shell scripts, too).
I suspect other shell scripts might be less affected when non-Windows
paths are forced (at least when they aren't developed under Windows
only). What about something like this:
pwd() { builtin pwd -W "$@" | sed -e 's,^\([a-z]\):/,/\1/,'; }
> Any other ideas?
>
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 3463d6d..f37745e 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -139,8 +139,8 @@ module_clone()
> gitdir="$gitdir/modules/$path"
>
> case $gitdir in
> - /*)
> - a="$(cd_to_toplevel && pwd)/"
> + /* | [a-z]:/*)
> + a="$(cd_to_toplevel && pwd -W)/"
> b=$gitdir
> while [ "$b" ] && [ "${a%%/*}" = "${b%%/*}" ]
> do
Hmm, here the path which starts with "c:/" is returned by the "git
rev-parse --git-dir" which is used to initialize the $gitdir variable
a few lines up.
> @@ -170,8 +170,8 @@ module_clone()
>
> echo "gitdir: $rel_gitdir" >"$path/.git"
>
> - a=$(cd "$gitdir" && pwd)
> - b=$(cd "$path" && pwd)
> + a=$(cd "$gitdir" && pwd -W)
> + b=$(cd "$path" && pwd -W)
> while [ "$b" ] && [ "${a%%/*}" = "${b%%/*}" ]
> do
> a=${a#*/} b=${b#*/};
I don't understand why you need this. Does "pwd" sometimes return a
path starting with "c:/" and sometimes "/c/" depending on what form
you use when you cd into that directory? If not, does the following
help you on windows? (If that is the case, you might need the pwd
redefinition too to make that work)
-----------------8<--------------------
diff --git a/git-submodule.sh b/git-submodule.sh
index 5deabf6..5bb8109 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -134,7 +134,7 @@ module_clone()
test -n "$name" || name="$path"
base_path=$(dirname "$path")
- gitdir=$(git rev-parse --git-dir)
+ gitdir=$(git rev-parse --git-dir | sed -e 's,^\([a-z]\):/,/\1/,')
gitdir_base="$gitdir/modules/$base_path"
gitdir="$gitdir/modules/$path"
^ permalink raw reply related
* Stash during incomplete merge
From: Phil Hord @ 2012-02-26 18:36 UTC (permalink / raw)
To: git@vger.kernel.org; +Cc: Phil Hord
Hi list,
I was cherry-picking changes from an old branch recently when I ran into
unexpected behavior with git stash pop. When I git-stash-save after
resolving a merge-conflict, the subsequent git-stash-pop does not
restore my index.
I think it is the same problem being asked about here:
http://stackoverflow.com/questions/9009354/git-stash-during-a-merge-conflict
Is this expected behavior or a bug?
<http://stackoverflow.com/questions/9009354/git-stash-during-a-merge-conflict>Here's
a script the demonstrates the anomaly, but my actual encounter involved
more files, some of which I added to the index and some I did not:
# Create a sample merge-conflict
git init tmp-repo && cd tmp-repo
echo foo > foo.txt && git add foo.txt && git commit -m "foo"
git checkout -b A master && echo foo-A > foo.txt && git commit -am "foo-A"
git checkout -b B master && echo foo-B > foo.txt && git commit -am "foo-B"
git merge A
git status
# Resolve the conflict
echo foo-AB > foo.txt && git add foo.txt
git status
git stash
# test test test... Resume...
git stash pop
Here's some of the final output:
$ git merge A
Auto-merging foo.txt
CONFLICT (content): Merge conflict in foo.txt
Recorded preimage for 'foo.txt'
Automatic merge failed; fix conflicts and then commit the result.
$ git status
# On branch B
# Unmerged paths:
# (use "git add/rm <file>..." as appropriate to mark resolution)
#
# both modified: foo.txt
#
no changes added to commit (use "git add" and/or "git commit -a")
$ # Resolve the conflict
$ echo foo-AB > foo.txt && git add foo.txt
$ git status
# On branch B
# Changes to be committed:
#
# modified: foo.txt
#
$ # Now foo.txt is in my index. But I have to test something before I
commit.
$ git stash
Saved working directory and index state WIP on B: 80f2a13 foo-B
HEAD is now at 80f2a13 foo-B
$ # test test test... Resume...
$ git stash pop
# On branch B
# Changes not staged for commit:
# (use "git add <file>..." to update what will be committed)
# (use "git checkout -- <file>..." to discard changes in working
directory)
#
# modified: foo.txt
#
no changes added to commit (use "git add" and/or "git commit -a")
Dropped refs/stash@{0} (460a6d5c67a3db613fd27f1854ecc7b89eeaa207)
^ permalink raw reply
* Re: [PATCH 2/2] git-p4: remove bash-ism in t9800
From: Pete Wyckoff @ 2012-02-26 18:16 UTC (permalink / raw)
To: Johannes Sixt; +Cc: git, Vitor Antunes
In-Reply-To: <4F4A7057.2020309@kdbg.org>
j6t@kdbg.org wrote on Sun, 26 Feb 2012 18:48 +0100:
> Am 26.02.2012 16:37, schrieb Pete Wyckoff:
> > - P4EDITOR=touch P4USER=bob P4PASSWD=secret test_must_fail "$GITP4" commit --preserve-user &&
> > - test_must_fail git diff --exit-code HEAD..p4/master
> > + # dashism: test_must_fail does not propagate variables
> > + P4EDITOR=touch P4USER=bob P4PASSWD=secret &&
> > + export P4EDITOR P4USER P4PASSWD &&
> > + test_must_fail "$GITP4" commit --preserve-user &&
> > + ! git diff --exit-code HEAD..p4/master
>
> It is a bashism that variables assigned in front of a shell function are
> exported. But it is not a dashism that they are not exported; that
> (surprising?) behavior is actually conforming to POSIX.
>
> With the new code, be aware that the variables remain exported, which
> might affect subsequent tests in general, though not this one, because
> the assignments are in a sub-shell:
>
> > )
> > '
Interesting, thanks. I thought about the subshell behavior and
use, on purpose, the fact that the variables stay exported in the
second and third hunks.
-- Pete
^ permalink raw reply
* Re: [PATCH] Makefile: add thread-utils.h to LIB_H
From: Thomas Rast @ 2012-02-26 18:13 UTC (permalink / raw)
To: Dmitry V. Levin; +Cc: git, gitster
In-Reply-To: <20120224234242.GA3124@altlinux.org>
"Dmitry V. Levin" <ldv@altlinux.org> writes:
> Starting with commit v1.7.8-165-g0579f91, grep.h includes
> thread-utils.h, so the latter has to be added to LIB_H.
Oops, my bad.
Acked-by: Thomas Rast <trast@student.ethz.ch>
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply
* Re: [PATCH 2/2] git-p4: remove bash-ism in t9800
From: Johannes Sixt @ 2012-02-26 17:48 UTC (permalink / raw)
To: Pete Wyckoff; +Cc: git, Vitor Antunes
In-Reply-To: <1330270647-8817-3-git-send-email-pw@padd.com>
Am 26.02.2012 16:37, schrieb Pete Wyckoff:
> - P4EDITOR=touch P4USER=bob P4PASSWD=secret test_must_fail "$GITP4" commit --preserve-user &&
> - test_must_fail git diff --exit-code HEAD..p4/master
> + # dashism: test_must_fail does not propagate variables
> + P4EDITOR=touch P4USER=bob P4PASSWD=secret &&
> + export P4EDITOR P4USER P4PASSWD &&
> + test_must_fail "$GITP4" commit --preserve-user &&
> + ! git diff --exit-code HEAD..p4/master
It is a bashism that variables assigned in front of a shell function are
exported. But it is not a dashism that they are not exported; that
(surprising?) behavior is actually conforming to POSIX.
With the new code, be aware that the variables remain exported, which
might affect subsequent tests in general, though not this one, because
the assignments are in a sub-shell:
> )
> '
-- Hannes
^ permalink raw reply
* Re: [PATCH 0/2] submodules: Use relative paths to gitdir and work tree
From: Johannes Sixt @ 2012-02-26 17:38 UTC (permalink / raw)
To: Jens Lehmann
Cc: Junio C Hamano, Git Mailing List, Antony Male, Phil Hord, msysGit
In-Reply-To: <4F32F252.7050105@web.de>
Am 08.02.2012 23:08, schrieb Jens Lehmann:
> This patch series replaces all absolute paths pointing from submodule work
> trees to its gitdir and back with relative paths as discussed in $gmane/187785.
>
> The motivation is to make superprojects movable again. They lost this ability
> with the move of the git directory of submodules into the .git/modules directory
> of the superproject. While fixing that a bug which would hit when moving the
> submodule inside the superproject was also fixed.
>
> Jens Lehmann (2):
> submodules: always use a relative path to gitdir
> submodules: always use a relative path from gitdir to work tree
This series, with the tip at e3307adaba in Junio's repo causes major
headaches on Windows.
First, a check for an absolute path must be extended to take
Windows-style paths into account.
Second, the a's and b's are filled with different forms of absolute
paths (/c/there vs. c:/there), and as a consequence the subsequent loops
do not find a suitable relative path.
The below is a minimal hack that passes all t/*submod* tests, but it
works only on Windows, where the pwd utility has an option -W that
prints a Windows style absolute path.
How would you have this solved? One option would be to introduce a function
pwd() { builtin pwd -W "$@"; }
in git-sh-setup conditionally on Windows (but that would affect other
shell scripts, too).
Any other ideas?
diff --git a/git-submodule.sh b/git-submodule.sh
index 3463d6d..f37745e 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -139,8 +139,8 @@ module_clone()
gitdir="$gitdir/modules/$path"
case $gitdir in
- /*)
- a="$(cd_to_toplevel && pwd)/"
+ /* | [a-z]:/*)
+ a="$(cd_to_toplevel && pwd -W)/"
b=$gitdir
while [ "$b" ] && [ "${a%%/*}" = "${b%%/*}" ]
do
@@ -170,8 +170,8 @@ module_clone()
echo "gitdir: $rel_gitdir" >"$path/.git"
- a=$(cd "$gitdir" && pwd)
- b=$(cd "$path" && pwd)
+ a=$(cd "$gitdir" && pwd -W)
+ b=$(cd "$path" && pwd -W)
while [ "$b" ] && [ "${a%%/*}" = "${b%%/*}" ]
do
a=${a#*/} b=${b#*/};
--
1.7.8.216.g2e426
^ permalink raw reply related
* [gitolite] denying access by factors other than ref/path names
From: Sitaram Chamarty @ 2012-02-26 15:50 UTC (permalink / raw)
To: gitolite, Git Mailing List
[-- Attachment #1: Type: text/plain, Size: 782 bytes --]
Gitolite now allows you to deny access using data other than ref
names or path names.
For example, you can deny a push if:
* it adds more then N new files (or variations, like changed
files, binary files, new binary files...)
* it contains autogenerated files that can't be caught by
gitignore
* it fails a "no non-merge first-parents" rule
* you don't like the phase of the moon :)
Since the actual decision is done by a script you write, you can
enforce pretty much any site-specific stupi^Wstandards, (like
ensuring that junior developers push only their own commits).
http://sitaramc.github.com/gitolite/vref.html has all the gory
details for anyone interested, and contrib/VREF in the latest pu
branch has example code.
--
Sitaram
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* [PATCH 2/2] git-p4: remove bash-ism in t9800
From: Pete Wyckoff @ 2012-02-26 15:37 UTC (permalink / raw)
To: git; +Cc: Vitor Antunes
In-Reply-To: <1330270647-8817-1-git-send-email-pw@padd.com>
This works in both bash and dash:
arf$ bash -c 'VAR=1 env' | grep VAR
VAR=1
arf$ dash -c 'VAR=1 env' | grep VAR
VAR=1
But the variables are not propagated through a function
in dash:
arf$ bash -c 'f() { "$@"
}; VAR=1 f "env"' | grep VAR
VAR=1
arf$ dash -c 'f() { "$@"
}; VAR=1 f "env"' | grep VAR
Fix constructs like this, in particular, setting variables
through test_must_fail.
Based-on-patch-by: Vitor Antunes <vitor.hda@gmail.com>
Signed-off-by: Pete Wyckoff <pw@padd.com>
---
t/t9800-git-p4-basic.sh | 25 ++++++++++++++++---------
1 file changed, 16 insertions(+), 9 deletions(-)
diff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh
index 04ee20e..9f17cdb 100755
--- a/t/t9800-git-p4-basic.sh
+++ b/t/t9800-git-p4-basic.sh
@@ -234,8 +234,11 @@ test_expect_success 'refuse to preserve users without perms' '
git config git-p4.skipSubmitEditCheck true &&
echo "username-noperms: a change by alice" >>file1 &&
git commit --author "Alice <alice@localhost>" -m "perms: a change by alice" file1 &&
- P4EDITOR=touch P4USER=bob P4PASSWD=secret test_must_fail "$GITP4" commit --preserve-user &&
- test_must_fail git diff --exit-code HEAD..p4/master
+ # dashism: test_must_fail does not propagate variables
+ P4EDITOR=touch P4USER=bob P4PASSWD=secret &&
+ export P4EDITOR P4USER P4PASSWD &&
+ test_must_fail "$GITP4" commit --preserve-user &&
+ ! git diff --exit-code HEAD..p4/master
)
'
@@ -250,13 +253,15 @@ test_expect_success 'preserve user where author is unknown to p4' '
git commit --author "Bob <bob@localhost>" -m "preserve: a change by bob" file1 &&
echo "username-unknown: a change by charlie" >>file1 &&
git commit --author "Charlie <charlie@localhost>" -m "preserve: a change by charlie" file1 &&
- P4EDITOR=touch P4USER=alice P4PASSWD=secret test_must_fail "$GITP4" commit --preserve-user &&
- test_must_fail git diff --exit-code HEAD..p4/master &&
+ P4EDITOR=touch P4USER=alice P4PASSWD=secret &&
+ export P4EDITOR P4USER P4PASSWD &&
+ test_must_fail "$GITP4" commit --preserve-user &&
+ ! git diff --exit-code HEAD..p4/master &&
echo "$0: repeat with allowMissingP4Users enabled" &&
git config git-p4.allowMissingP4Users true &&
git config git-p4.preserveUser true &&
- P4EDITOR=touch P4USER=alice P4PASSWD=secret "$GITP4" commit &&
+ "$GITP4" commit &&
git diff --exit-code HEAD..p4/master &&
p4_check_commit_author file1 alice
)
@@ -275,20 +280,22 @@ test_expect_success 'not preserving user with mixed authorship' '
p4_add_user derek Derek &&
make_change_by_user usernamefile3 Derek derek@localhost &&
- P4EDITOR=cat P4USER=alice P4PASSWD=secret "$GITP4" commit |\
+ P4EDITOR=cat P4USER=alice P4PASSWD=secret &&
+ export P4EDITOR P4USER P4PASSWD &&
+ "$GITP4" commit |\
grep "git author derek@localhost does not match" &&
make_change_by_user usernamefile3 Charlie charlie@localhost &&
- P4EDITOR=cat P4USER=alice P4PASSWD=secret "$GITP4" commit |\
+ "$GITP4" commit |\
grep "git author charlie@localhost does not match" &&
make_change_by_user usernamefile3 alice alice@localhost &&
- P4EDITOR=cat P4USER=alice P4PASSWD=secret "$GITP4" |\
+ "$GITP4" commit |\
test_must_fail grep "git author.*does not match" &&
git config git-p4.skipUserNameCheck true &&
make_change_by_user usernamefile3 Charlie charlie@localhost &&
- P4EDITOR=cat P4USER=alice P4PASSWD=secret "$GITP4" commit |\
+ "$GITP4" commit |\
test_must_fail grep "git author.*does not match" &&
p4_check_commit_author usernamefile3 alice
--
1.7.9.2.288.g74b75
^ permalink raw reply related
* [PATCH 1/2] git-p4: remove bash-ism in t9809
From: Pete Wyckoff @ 2012-02-26 15:37 UTC (permalink / raw)
To: git; +Cc: Vitor Antunes
In-Reply-To: <1330270647-8817-1-git-send-email-pw@padd.com>
Plain old $# works to count the number of arguments in
either bash or dash, even if the arguments have spaces.
Based-on-patch-by: Vitor Antunes <vitor.hda@gmail.com>
Signed-off-by: Pete Wyckoff <pw@padd.com>
---
t/t9809-git-p4-client-view.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/t9809-git-p4-client-view.sh b/t/t9809-git-p4-client-view.sh
index ae9145e..18d93e4 100755
--- a/t/t9809-git-p4-client-view.sh
+++ b/t/t9809-git-p4-client-view.sh
@@ -31,7 +31,7 @@ client_view() {
#
check_files_exist() {
ok=0 &&
- num=${#@} &&
+ num=$# &&
for arg ; do
test_path_is_file "$arg" &&
ok=$(($ok + 1))
--
1.7.9.2.288.g74b75
^ permalink raw reply related
* [PATCH 0/2] git-p4: remove test bash-isms
From: Pete Wyckoff @ 2012-02-26 15:37 UTC (permalink / raw)
To: git; +Cc: Vitor Antunes
Two fixes to make the t98* tests run in dash.
Pete Wyckoff (2):
git-p4: remove bash-ism in t9809
git-p4: remove bash-ism in t9800
t/t9800-git-p4-basic.sh | 25 ++++++++++++++++---------
t/t9809-git-p4-client-view.sh | 2 +-
2 files changed, 17 insertions(+), 10 deletions(-)
--
1.7.9.2.288.g74b75
^ permalink raw reply
* Re: Question about your comment on the git parable
From: Jakub Narebski @ 2012-02-26 15:06 UTC (permalink / raw)
To: Federico Galassi; +Cc: git
In-Reply-To: <1E5ECB5A-595A-4B04-8269-6E35BF3FEA1A@gmail.com>
On Sun, 26 Feb 2012, Federico Galassi wrote:
> On 26/feb/2012, at 12:29, Jakub Narebski wrote:
>
>> Would you mind if this discussion was moved to git mailing
>> list (git@vger.kernel.org), of course always with copy directly
>> to you? There are people there that can answer your questions
>> better.
>
> No problem.
>
>> On Sun, 26 Feb 2012, Federico Galassi wrote:
>>> Hello, i think you're the author of these comments:
>>> http://news.ycombinator.com/item?id=616610
>>>
>>> I'm doing educational work on git based on the parable (talks,
>>> articles, etc..) and i'd like to improve on the real reason
>>> for a staging area.
>>>
>>> My question basically is: why is it really needed for merging?
>>> I mean, given the fictional git-like system of the parable,
>>> if I need to merge 2 snapshots i could:
>>>
>>> 1) search the commit tree for a base point
[...]
>>> 2) compare the diffs between the snapshots and the base point snapshot
>>> 3) if a conflict happens (change in the same line), just leave
>>> something in the working dir to mark the conflict. For example,
>>> keeping it simple, the system could reject a new commit until
>>> the markers of the conflict are removed from the conflicting file.
>>>
>>> Couldn't it just work this way?
>>
>> Well, it could; that is how many if not most of other version control
>> systems work.
>>
>>
>> There are (at least!) three problems with that approach. First, sometimes
>> it is not possible to "leave something in the working dir to mark the
>> conflict". Take for example case where binary file (e.g. image) was
>> changed, and textual 3-way diff file-merge algorithm wouldn't work.
>>
>> Second, what to do in the case of *tree-level* conflict, for example
>> rename/rename conflict, where one side renamed file to different
>> name (moved to different place) than the other side. There are no
>> conflict markers for this...
>>
>> Third, what about false positives with detecting conflict markers,
>> i.e. the case where "rejecting new commit until conflict markers are
>> removed", for example AsciiDoc files can be falsely detected as having
>> partial conflict markers, and of course test vectors for testing conflict
>> would have to have conflict markers in them.
>
> Ok, it's clear to me that the markers in file approach is just a little
> bit too simple. Do you see any concrete advantage in the staging area
> compared to, say, tree conflict metadata in the working dir and maybe
> a dedicated smart "resolve conflict" command?
First, for such _local_ information working directory isn't the best place.
What if you accidentally delete this? It is not and should not be
committed to repository,so there is no way to undelete it, except redoing
merge and losing all your progress so far in resolving merge conflicts.
It is much better to put such information somewhere in administrative
area[1] of repository.
Second, if we have staging area where we store information about which
files are tracked, and a bunch of per-file metadata like modification time
for better performance, why not use it also for storing information about
merge in progress?
[1]: Name taken from "Version Control by Example" (free e-book) by
Eric Sink.
There is also a thing very specific to Git, namely that "git add" adds
a current content of a file to object database of a repository (though
with modern git there is also "git add --intent-to-add" which works
like add-ing file in other version control systems)... and you have to
store reference to newly created object somewhere so that it doesn't get
garbage-collected.
>>> Can you mention other situations in which the pattern "files to be added"
>>> is either mandatory or really helpful?
>>
>> Note that any version control system must have a kind of proto-staging
>> area to know which files are to be added in next commit.
>>
>> If you do
>>
>> $ scm add file.c
>>
>> then version control system must save somewhere that 'file.c' is to be
>> tracked (to be added in next commit).
>
> Yes, the fictional vcs just tracked all the files in the working dir.
> Being selective on which file to track is of course another interesting
> feature.
IRL it is a _necessary_ feature. One of more common, if not most common
application of version control system is to manage source files for a
computer program. And there you have object files, executables and other
_generated_ files which shouldn't be put in version control, not to
mention backups created by your editor / IDE (e.g. "*~" files in Unix
world, "*.bak" files in MS Windows world).
Not to mention files which you have added to working directory, but are
not ready to be added to new commit.
--
Jakub Narebski
Poland
^ permalink raw reply
* [PATCH] git-p4: submit files with wildcards
From: Pete Wyckoff @ 2012-02-26 14:55 UTC (permalink / raw)
To: git; +Cc: Luke Diamand
There are four wildcard characters in p4. Files with these
characters can be added to p4 repos using the "-f" option. They
are stored in %xx notation, and when checked out, p4 converts
them back to normal.
When adding files with wildcards in git, the submit path must
be careful to use the encoded names in some places, and it
must use "-f" to add them.
Support for wildcards in the clone/sync path was added in
084f630 (git-p4: decode p4 wildcard characters, 2011-02-19),
but that change did not handle the submit path.
Reported-by: Luke Diamand <luke@diamand.org>
Signed-off-by: Pete Wyckoff <pw@padd.com>
---
contrib/fast-import/git-p4 | 64 ++++++++++++++++++++++++++++---------------
t/t9800-git-p4-basic.sh | 23 ++++++++++++++++
2 files changed, 65 insertions(+), 22 deletions(-)
diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index 0539553..bd89402 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -136,8 +136,8 @@ def p4_integrate(src, dest):
def p4_sync(path):
p4_system(["sync", path])
-def p4_add(f):
- p4_system(["add", f])
+def p4_add(path, *options):
+ p4_system(["add"] + list(options) + [path])
def p4_delete(f):
p4_system(["delete", f])
@@ -232,8 +232,12 @@ def setP4ExecBit(file, mode):
# Reopens an already open file and changes the execute bit to match
# the execute bit setting in the passed in mode.
- p4Type = "+x"
+ # Since this is working on already opened files, any with wildcards
+ # in the name have been encoded already, and must be referred to
+ # with the encoded name.
+ file = wildcard_encode(file)
+ p4Type = "+x"
if not isModeExec(mode):
p4Type = getP4OpenedType(file)
p4Type = re.sub('^([cku]?)x(.*)', '\\1\\2', p4Type)
@@ -596,6 +600,34 @@ def p4PathStartsWith(path, prefix):
return path.lower().startswith(prefix.lower())
return path.startswith(prefix)
+#
+# P4 wildcards are not allowed in filenames. P4 complains
+# if you simply add them, but you can force it with "-f", in
+# which case it translates them into %xx encoding internally.
+#
+def wildcard_decode(path):
+ # Search for and fix just these four characters. Do % last so
+ # that fixing it does not inadvertently create new %-escapes.
+ # Cannot have * in a filename in windows; untested as to
+ # what p4 would do in such a case.
+ if not platform.system() == "Windows":
+ path = path.replace("%2A", "*")
+ path = path.replace("%23", "#") \
+ .replace("%40", "@") \
+ .replace("%25", "%")
+ return path
+
+def wildcard_encode(path):
+ # do % first to avoid double-encoding the %s introduced here
+ path = path.replace("%", "%25") \
+ .replace("*", "%2A") \
+ .replace("#", "%23") \
+ .replace("@", "%40")
+ return path
+
+def wildcard_present(path):
+ return path.translate(None, "*#@%") != path
+
class Command:
def __init__(self):
self.usage = "usage: %prog [options]"
@@ -1099,7 +1131,12 @@ class P4Submit(Command, P4UserMap):
system(applyPatchCmd)
for f in filesToAdd:
- p4_add(f)
+ # forcibly add file names with wildcards; see also
+ # wildcard_decode() in the sync path
+ if wildcard_present(f):
+ p4_add(f, "-f")
+ else:
+ p4_add(f)
for f in filesToDelete:
p4_revert(f)
p4_delete(f)
@@ -1537,23 +1574,6 @@ class P4Sync(Command, P4UserMap):
if gitConfig("git-p4.syncFromOrigin") == "false":
self.syncWithOrigin = False
- #
- # P4 wildcards are not allowed in filenames. P4 complains
- # if you simply add them, but you can force it with "-f", in
- # which case it translates them into %xx encoding internally.
- # Search for and fix just these four characters. Do % last so
- # that fixing it does not inadvertently create new %-escapes.
- #
- def wildcard_decode(self, path):
- # Cannot have * in a filename in windows; untested as to
- # what p4 would do in such a case.
- if not self.isWindows:
- path = path.replace("%2A", "*")
- path = path.replace("%23", "#") \
- .replace("%40", "@") \
- .replace("%25", "%")
- return path
-
# Force a checkpoint in fast-import and wait for it to finish
def checkpoint(self):
self.gitStream.write("checkpoint\n\n")
@@ -1638,7 +1658,7 @@ class P4Sync(Command, P4UserMap):
def streamOneP4File(self, file, contents):
relPath = self.stripRepoPath(file['depotFile'], self.branchPrefixes)
- relPath = self.wildcard_decode(relPath)
+ relPath = wildcard_decode(relPath)
if verbose:
sys.stderr.write("%s\n" % relPath)
diff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh
index 04ee20e..22669df 100755
--- a/t/t9800-git-p4-basic.sh
+++ b/t/t9800-git-p4-basic.sh
@@ -163,6 +163,29 @@ test_expect_success 'wildcard files git-p4 clone' '
)
'
+test_expect_success 'wildcard files submit back to p4' '
+ "$GITP4" clone --dest="$git" //depot &&
+ test_when_finished cleanup_git &&
+ (
+ cd "$git" &&
+ echo git-wild-hash >git-wild#hash &&
+ echo git-wild-star >git-wild\*star &&
+ echo git-wild-at >git-wild@at &&
+ echo git-wild-percent >git-wild%percent &&
+ git add git-wild* &&
+ git commit -m "add some wildcard filenames" &&
+ git config git-p4.skipSubmitEditCheck true &&
+ "$GITP4" submit
+ ) &&
+ (
+ cd "$cli" &&
+ test_path_is_file git-wild#hash &&
+ test_path_is_file git-wild\*star &&
+ test_path_is_file git-wild@at &&
+ test_path_is_file git-wild%percent
+ )
+'
+
test_expect_success 'clone bare' '
"$GITP4" clone --dest="$git" --bare //depot &&
test_when_finished cleanup_git &&
--
1.7.9.220.g4b839
^ permalink raw reply related
* Re: Question about your comment on the git parable
From: Jakub Narebski @ 2012-02-26 14:10 UTC (permalink / raw)
To: Federico Galassi; +Cc: git
In-Reply-To: <4B4C5353-9820-4068-92DA-50665B1011E1@gmail.com>
Federico Galassi wrote:
> On 26/feb/2012, at 13:03, Jakub Narebski wrote:
>> Jakub Narebski wrote:
[...]
>>> Note also that the staging area is also a performance hack (perhaps it
>>> began as such; I am not sure about this aspect of git history). Git uses
>>> it to be able to _cheaply_ check which files were changed.
>>
>> The first name for staging area, _dircache_, hints at this.
>
> Unfortunately, i'm not into git development. Do you have a clue on why
> the index, apparently a tree referring to objects, is much faster than
> reading that stuff right from the database?
The index (at the very beginning "dircache"), or the staging area, stores
more information that are saved in object database, for example stat
information (file metadata). Most of file metadata is highly local, so
it doesn't make sense to save it in object database of repository, but
it is used to avoid a file read: usually stat-ing a file, which is much
more cheap, is enough to notice that the file did not change.
--
Jakub Narebski
Poland
^ permalink raw reply
* Re: [PATCH 2/2] index-pack: reduce memory usage when the pack has large blobs
From: Ian Kumlien @ 2012-02-26 13:28 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: git
In-Reply-To: <CACsJy8Cncs8RYiSB0N20vy9zu2NRTTHpfw3rSfmW64i-4_wxSw@mail.gmail.com>
On Sun, Feb 26, 2012 at 11:10:14AM +0700, Nguyen Thai Ngoc Duy wrote:
> On Sun, Feb 26, 2012 at 5:45 AM, Ian Kumlien <pomac@vapor.com> wrote:
> > Actually, i added a backtrace and used addr2line to confirm my
> > suspicion... which is:
> > builtin/index-pack.c:414
> >
> > ie get_data_from_pack...
>
> That function should only be called when objects are deltified, which
> should _not_ happen for large blobs. What is its caller?
Full backtrace:
for x in 0x536031 0x451b0e 0x452212 0x4523f5 0x452711 0x452799 0x452bbb
0x454344 0x4170d1 0x41726c ; do addr2line $x -e ../git/git ; done
git/wrapper.c:41
git/builtin/index-pack.c:414
git/builtin/index-pack.c:588
git/builtin/index-pack.c:625
git/builtin/index-pack.c:679
git/builtin/index-pack.c:694
git/builtin/index-pack.c:805
git/builtin/index-pack.c:1246
git/git.c:308
git/git.c:467
Which means:
xmalloc
get_data_from_pack
get_base_data -- line just after: if (!delta_nr) {
resolve_delta
find_unresolved_deltas_1
find_unresolved_deltas
parse_pack_objects
cmd_index_pack
[skipping the git.c part]
Btw, i'm running these tests on a 64 bit laptop - since i'm not at work
;) (had to manually limit xmalloc but it triggers at the same point)
^ permalink raw reply
* Re: sha-1 check in rev-list --verify-objects redundant?
From: Nguyen Thai Ngoc Duy @ 2012-02-26 11:11 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List
In-Reply-To: <7vk43af14m.fsf@alter.siamese.dyndns.org>
On Sun, Feb 26, 2012 at 3:30 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
>
>> ... I wonder if this is a
>> redundant check. --verify-objects is called to verify new packs.
>
> I do not think --verify-objects does not have anything to do with
> verifying the integrity of packs, whether new or old.
>
> The check is about the integrity of the *history* we _might_ already have
> on our side, when we find ourselves wanting to fetch up to a commit $X,
> whose reachability from the tips of our refs (i.e. the objects that are
> guaranteed to be present in our repository) is unknown, and we somehow
> already have the commit $X itself in the repository.
>
> We cannot just declare victory upon seeing commit $X and omit fetching the
> history leading to the commit, because we may or may not have its parent
> commit object, or the tree object that is recorded in it (it may be that
> we killed an HTTP walker after we fetched $X but not its parents or
> trees). We need to walk back from $X until we hit one of the tips of our
> refs, and while doing so, we also need to make sure the trees and blobs
> referenced from the walked commits are also healthy.
>
> As 5a48d24 (rev-list --verify-object, 2011-09-01) explains, we used to do
> this with --objects instead, but that check does not even make sure blobs
> exist [*1*] let alone checking to see if these blobs were healthy. The
> whole point of using --verify-objects instead of --objects is to make sure
> that we do not miss blob objects.
"rev-list --objects" does check for blob existence, in finish_object().
On the well-formedness, unless I'm mistaken, --verify-objects is
_always_ used in conjunction with index-pack. --verify-object is not
even documented. index-pack makes sure all (new) object signatures
reflect their content. Commit and tree content are validated by
rev-list walking them. So at least when --verify-objects is used with
index-pack, I don't see the point rehashing every new object _again_.
> [Footnote]
>
> *1* The --objects code reads the commits and trees in order to _list_
> objects to the blob level, so implicitly, it validates that commits and
> trees reachable from the commit $X we happened to have in our repository,
> relying on the fact that we would error out if we fail to read them.
--
Duy
^ permalink raw reply
* Re: sha-1 check in rev-list --verify-objects redundant?
From: Junio C Hamano @ 2012-02-26 9:11 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: Git Mailing List
In-Reply-To: <7vk43af14m.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
>
>> ... I wonder if this is a
>> redundant check. --verify-objects is called to verify new packs.
>
> I do not think --verify-objects does not have anything to do with
> verifying the integrity of packs, whether new or old.
A typo/grammo that should be obvious from the context, sorry. The above
should be "I do not think --verify-objects has anything to do with ..."
^ permalink raw reply
* Re: 4-way diff (base,ours,theirs,merged) to review merge results
From: Junio C Hamano @ 2012-02-26 9:05 UTC (permalink / raw)
To: Neal Kreitzinger; +Cc: git
In-Reply-To: <jicafn$gnj$1@dough.gmane.org>
"Neal Kreitzinger" <neal@rsss.com> writes:
> .... What is the
> best way to display a 4-way diff of merge-base, "ours", "theirs", and
> "merged" after a merge completes so you can review the "merged" results for
> correctness?
Ahh, sorry. While everything I wrote in my previous reply is correct with
respect to what happens _during_ a merge until you resolve it, I did not
realize that you were asking how to view a merge _after_ it is made.
For a two-parent merge $M, "git show --cc $M" runs a three-way diff
between $M (merge result), $M^1 (the first parent) and $M^2 (the other
parent) and the combined diff it shows is equivalent to:
$ git diff --cc $M $M^1 $M^2
Notice the order of parameters. Unlike a normal "diff A B" to ask the
command to explain how the state B is reached from state A, you give the
result $M and ask the command to explain how it was reached from other
states.
So in a similar way, running
$ git diff --cc $M $M^1 $M^2 $(git merge-base $M^1 $M^2)
should show a combined patch that explains the state at $M relative to the
states recorded in its parents and the merge base.
I've never tried it myself, though, as I never needed such an operation.
You can try a trivial example with 4d9e079, which merges 583c389 ec7ff5b
and has conflicts in cache.h
$ git show 4d9e079 -- cache.h
Output omitted; you can see it is the same as the next one for yourself.
$ git diff --cc 4d9e079 583c389 ec7ff5b -- cache.h
diff --cc cache.h
index 3a8e125,24732e6..422c5cf
--- a/cache.h
+++ b/cache.h
@@@ -1177,7 -1176,7 +1177,8 @@@ extern void setup_pager(void)
extern const char *pager_program;
extern int pager_in_use(void);
extern int pager_use_color;
+extern int term_columns(void);
+ extern int decimal_width(int);
extern const char *editor_program;
extern const char *askpass_program;
One side adds term_columns, the other side adds decimal_width.
$ git diff --cc 4d9e079 583c389 ec7ff5b \
$(git merge-base 583c389 ec7ff5b) -- cache.h
diff --cc cache.h
index 3a8e125,24732e6,9bd8c2d..422c5cf
--- a/cache.h
+++ b/cache.h
@@@@ -1177,7 -1176,7 -1176,6 +1177,8 @@@@ extern void setup_pager(void)
extern const char *pager_program;
extern int pager_in_use(void);
extern int pager_use_color;
++extern int term_columns(void);
+ +extern int decimal_width(int);
extern const char *editor_program;
extern const char *askpass_program;
The third column is a diff between $M and $(git merge-base $M^1 $M^2); the
resulting two new lines are indeed shown as additions against the merge
base.
^ permalink raw reply
* Re: sha-1 check in rev-list --verify-objects redundant?
From: Junio C Hamano @ 2012-02-26 8:30 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: Git Mailing List
In-Reply-To: <CACsJy8D_BdV14dGc2YsK91FrX8S=70DJOY3cU=oH3y41N2Ar0w@mail.gmail.com>
Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
> ... I wonder if this is a
> redundant check. --verify-objects is called to verify new packs.
I do not think --verify-objects does not have anything to do with
verifying the integrity of packs, whether new or old.
The check is about the integrity of the *history* we _might_ already have
on our side, when we find ourselves wanting to fetch up to a commit $X,
whose reachability from the tips of our refs (i.e. the objects that are
guaranteed to be present in our repository) is unknown, and we somehow
already have the commit $X itself in the repository.
We cannot just declare victory upon seeing commit $X and omit fetching the
history leading to the commit, because we may or may not have its parent
commit object, or the tree object that is recorded in it (it may be that
we killed an HTTP walker after we fetched $X but not its parents or
trees). We need to walk back from $X until we hit one of the tips of our
refs, and while doing so, we also need to make sure the trees and blobs
referenced from the walked commits are also healthy.
As 5a48d24 (rev-list --verify-object, 2011-09-01) explains, we used to do
this with --objects instead, but that check does not even make sure blobs
exist [*1*] let alone checking to see if these blobs were healthy. The
whole point of using --verify-objects instead of --objects is to make sure
that we do not miss blob objects.
[Footnote]
*1* The --objects code reads the commits and trees in order to _list_
objects to the blob level, so implicitly, it validates that commits and
trees reachable from the commit $X we happened to have in our repository,
relying on the fact that we would error out if we fail to read them.
^ permalink raw reply
* Re: 4-way diff (base,ours,theirs,merged) to review merge results
From: Junio C Hamano @ 2012-02-26 8:12 UTC (permalink / raw)
To: Neal Kreitzinger; +Cc: git
In-Reply-To: <jicafn$gnj$1@dough.gmane.org>
"Neal Kreitzinger" <neal@rsss.com> writes:
> (Combined diff)
> ours: has line-x
> theirs (master): does not have line-x
> merged: has line-x
> merge-base (older master): *may-or-may-not* have line-x
> conclusion: I'm not very sure if "merged" should have line-x or not...
When I need this information to resolve a merge in an area of the code
that I am not very familiar with, the first thing I do is this:
$ git merge $other
$ git diff
... yikes, that is a complex conflict!
$ git checkout --conflict=diff3 $the_path_with_difficult_conflict
$ git diff
The output will also show the lines from the merge base.
The default style of showing the conflict we use is called the "merge"
style (it originally came from the "merge" program of the RCS suite), and
it only gives the two sides without the base version. It is sufficient
when the person who is making the merge is familiar with the baseline
history of the code (e.g. in a contributor-to-integrator pull based
workflow, especially when contributors are encouraged to keep their topics
focused and short). The "diff3" style that also gives the base version is
needed less often in such a setting. That, and also the resulting output
is much shorter, is the reason why "merge" style is the default.
When the person who is making the merge is not very familiar with the
baseline history (e.g. when using Git as an improved CVS and a contributor
pulls the updated upstream into his history), however, "diff3" style may
be more often helpful---as you mentioned, "merge" style requires that you
know your code well enough to either already know or be able to guess how
the version in the merge base looked like, but by definition, pulling the
updated upstream into your work will pull more stuff (because many other
people are working on the code on the other side) than pulling one topic
from a contributor into the integrator tree, so there may be more need to
see the version from the merge base in such a workflow.
By setting the configuration variable "merge.conflictstyle" to "diff3",
you would get the base version by default whenever there is a conflict.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox