* [JGIT PATCH 1/4] Make the cleanup less verbose when it fails to delete temporary stuff.
From: Robin Rosenberg @ 2008-11-27 21:15 UTC (permalink / raw)
To: spearce; +Cc: git, fonseca, Robin Rosenberg
In-Reply-To: <1227820535-9785-1-git-send-email-robin.rosenberg@dewire.com>
Pass the test case name for easier tracking of the test case that
causes problems.
Signed-off-by: Robin Rosenberg <robin.rosenberg@dewire.com>
---
.../org/spearce/jgit/lib/RepositoryTestCase.java | 59 ++++++++++++++-----
1 files changed, 43 insertions(+), 16 deletions(-)
diff --git a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RepositoryTestCase.java b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RepositoryTestCase.java
index 9d7d133..9c272f6 100644
--- a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RepositoryTestCase.java
+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RepositoryTestCase.java
@@ -66,22 +66,43 @@
jcommitter = new PersonIdent("J. Committer", "jcommitter@example.com");
}
- protected static void recursiveDelete(final File dir) {
+ protected static boolean recursiveDelete(final File dir) {
+ return recursiveDelete(dir, false, null);
+ }
+
+ protected static boolean recursiveDelete(final File dir, boolean silent,
+ final String name) {
+ if (!dir.exists())
+ return silent;
final File[] ls = dir.listFiles();
if (ls != null) {
for (int k = 0; k < ls.length; k++) {
final File e = ls[k];
if (e.isDirectory()) {
- recursiveDelete(e);
+ silent = recursiveDelete(e, silent, name);
} else {
- e.delete();
+ if (!e.delete()) {
+ if (!silent) {
+ String msg = "Warning: Failed to delete " + e;
+ if (name != null)
+ msg += " in " + name;
+ System.out.println(msg);
+ }
+ silent = true;
+ }
}
}
}
- dir.delete();
- if (dir.exists()) {
- System.out.println("Warning: Failed to delete " + dir);
+ if (!dir.delete()) {
+ if (!silent) {
+ String msg = "Warning: Failed to delete " + dir;
+ if (name != null)
+ msg += " in " + name;
+ System.out.println(msg);
+ }
+ silent = true;
}
+ return silent;
}
protected static void copyFile(final File src, final File dst)
@@ -121,19 +142,25 @@ protected static void checkFile(File f, final String checkData)
protected Repository db;
+ private static int testcount;
+ private static Thread shutdownhook;
+
public void setUp() throws Exception {
super.setUp();
- recursiveDelete(trashParent);
- trash = new File(trashParent,"trash"+System.currentTimeMillis());
+ System.gc();
+ trash = new File(trashParent,"trash"+System.currentTimeMillis()+"."+(testcount++));
+ final String name = getClass().getName() + "." + getName();
+ recursiveDelete(trashParent, true, name);
trash_git = new File(trash, ".git");
-
- Runtime.getRuntime().addShutdownHook(new Thread() {
- @Override
- public void run() {
- recursiveDelete(trashParent);
- }
- });
-
+ if (shutdownhook == null) {
+ shutdownhook = new Thread() {
+ @Override
+ public void run() {
+ recursiveDelete(trashParent, false, name);
+ }
+ };
+ Runtime.getRuntime().addShutdownHook(shutdownhook);
+ }
db = new Repository(trash_git);
db.create();
--
1.6.0.3.640.g6331a
^ permalink raw reply related
* [JGIT PATCH 3/4] Cleanup malformed test cases
From: Robin Rosenberg @ 2008-11-27 21:15 UTC (permalink / raw)
To: spearce; +Cc: git, fonseca, Robin Rosenberg
In-Reply-To: <1227820535-9785-3-git-send-email-robin.rosenberg@dewire.com>
---
.../tst/org/spearce/jgit/lib/PackWriterTest.java | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/PackWriterTest.java b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/PackWriterTest.java
index e5bce4d..3c02955 100644
--- a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/PackWriterTest.java
+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/PackWriterTest.java
@@ -309,6 +309,7 @@ public void testWritePack4ThinPack() throws IOException {
public void testWritePack2SizeDeltasVsNoDeltas() throws Exception {
testWritePack2();
final int sizePack2NoDeltas = cos.getCount();
+ tearDown();
setUp();
testWritePack2DeltasReuseRefs();
final int sizePack2DeltasRefs = cos.getCount();
@@ -327,6 +328,7 @@ public void testWritePack2SizeDeltasVsNoDeltas() throws Exception {
public void testWritePack2SizeOffsetsVsRefs() throws Exception {
testWritePack2DeltasReuseRefs();
final int sizePack2DeltasRefs = cos.getCount();
+ tearDown();
setUp();
testWritePack2DeltasReuseOffsets();
final int sizePack2DeltasOffsets = cos.getCount();
@@ -344,6 +346,7 @@ public void testWritePack2SizeOffsetsVsRefs() throws Exception {
public void testWritePack4SizeThinVsNoThin() throws Exception {
testWritePack4();
final int sizePack4 = cos.getCount();
+ tearDown();
setUp();
testWritePack4ThinPack();
final int sizePack4Thin = cos.getCount();
--
1.6.0.3.640.g6331a
^ permalink raw reply related
* [JGIT PATCH 2/4] Add shutdown hooks to try to clean up after unit tests anyway
From: Robin Rosenberg @ 2008-11-27 21:15 UTC (permalink / raw)
To: spearce; +Cc: git, fonseca, Robin Rosenberg
In-Reply-To: <1227820535-9785-2-git-send-email-robin.rosenberg@dewire.com>
Signed-off-by: Robin Rosenberg <robin.rosenberg@dewire.com>
---
.../org/spearce/jgit/lib/RepositoryTestCase.java | 18 +++++++++++++++---
1 files changed, 15 insertions(+), 3 deletions(-)
diff --git a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RepositoryTestCase.java b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RepositoryTestCase.java
index 9c272f6..ef4fd1b 100644
--- a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RepositoryTestCase.java
+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RepositoryTestCase.java
@@ -66,8 +66,8 @@
jcommitter = new PersonIdent("J. Committer", "jcommitter@example.com");
}
- protected static boolean recursiveDelete(final File dir) {
- return recursiveDelete(dir, false, null);
+ protected boolean recursiveDelete(final File dir) {
+ return recursiveDelete(dir, false, getClass().getName() + "." + getName());
}
protected static boolean recursiveDelete(final File dir, boolean silent,
@@ -161,6 +161,12 @@ public void run() {
};
Runtime.getRuntime().addShutdownHook(shutdownhook);
}
+ Runtime.getRuntime().addShutdownHook(new Thread() {
+ @Override
+ public void run() {
+ recursiveDelete(trash);
+ }
+ });
db = new Repository(trash_git);
db.create();
@@ -197,12 +203,18 @@ protected void tearDown() throws Exception {
* @throws IOException
*/
protected Repository createNewEmptyRepo() throws IOException {
- File newTestRepo = new File(trashParent, "new"+System.currentTimeMillis()+"/.git");
+ final File newTestRepo = new File(trashParent, "new"+System.currentTimeMillis()+"/.git");
assertFalse(newTestRepo.exists());
File unusedDir = new File(trashParent, "tmp"+System.currentTimeMillis());
assertTrue(unusedDir.mkdirs());
final Repository newRepo = new Repository(newTestRepo);
newRepo.create();
+ Runtime.getRuntime().addShutdownHook(new Thread() {
+ @Override
+ public void run() {
+ recursiveDelete(newTestRepo);
+ }
+ });
return newRepo;
}
--
1.6.0.3.640.g6331a
^ permalink raw reply related
* [JGIT PATCH 4/4] Automatically clean up any repositories created by the test cases
From: Robin Rosenberg @ 2008-11-27 21:15 UTC (permalink / raw)
To: spearce; +Cc: git, fonseca, Robin Rosenberg
In-Reply-To: <1227820535-9785-4-git-send-email-robin.rosenberg@dewire.com>
Signed-off-by: Robin Rosenberg <robin.rosenberg@dewire.com>
---
.../org/spearce/jgit/lib/RepositoryTestCase.java | 21 +++++++++++++++++++-
1 files changed, 20 insertions(+), 1 deletions(-)
diff --git a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RepositoryTestCase.java b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RepositoryTestCase.java
index ef4fd1b..cab65a0 100644
--- a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RepositoryTestCase.java
+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RepositoryTestCase.java
@@ -45,6 +45,7 @@
import java.io.InputStreamReader;
import java.io.OutputStreamWriter;
import java.io.Reader;
+import java.util.ArrayList;
import junit.framework.TestCase;
import org.spearce.jgit.util.JGitTestUtil;
@@ -145,6 +146,8 @@ protected static void checkFile(File f, final String checkData)
private static int testcount;
private static Thread shutdownhook;
+ private ArrayList<Repository> repositoriesToClose = new ArrayList<Repository>();
+
public void setUp() throws Exception {
super.setUp();
System.gc();
@@ -192,6 +195,20 @@ copyFile(JGitTestUtil.getTestResourceFile(packs[k] + ".idx"), new File(packDir,
protected void tearDown() throws Exception {
db.close();
+ for (Repository r : repositoriesToClose) {
+ r.close();
+ }
+ // Since memory mapping is controlled by the GC we need to
+ // tell it this is a good time to clean up and unlock
+ // mmemory mapped files.
+ System.gc();
+
+ recursiveDelete(trash, false, getName());
+ for (Repository r : repositoriesToClose) {
+ recursiveDelete(r.getWorkDir(), false, getName());
+ }
+ repositoriesToClose.clear();
+
super.tearDown();
}
@@ -209,12 +226,14 @@ protected Repository createNewEmptyRepo() throws IOException {
assertTrue(unusedDir.mkdirs());
final Repository newRepo = new Repository(newTestRepo);
newRepo.create();
+ final String name = getClass().getName() + "." + getName();
Runtime.getRuntime().addShutdownHook(new Thread() {
@Override
public void run() {
- recursiveDelete(newTestRepo);
+ recursiveDelete(newTestRepo, false, name);
}
});
+ repositoriesToClose.add(newRepo);
return newRepo;
}
--
1.6.0.3.640.g6331a
^ permalink raw reply related
* Re: [PATCH] Fix typos in the documentation.
From: Ralf Wildenhues @ 2008-11-27 21:16 UTC (permalink / raw)
To: Andreas Ericsson, Junio C Hamano; +Cc: git
In-Reply-To: <492E6415.6080101@op5.se>
Hello Andreas, Junio,
* Andreas Ericsson wrote on Thu, Nov 27, 2008 at 10:10:45AM CET:
> Ralf Wildenhues wrote:
>>
>> This patch is against pu.
> 'pu' is a moving target in git. For future reference, it's
> better to send patches against 'master'.
* Junio C Hamano wrote on Thu, Nov 27, 2008 at 10:48:56AM CET:
>
> While I appreciate your fix, it would have been nicer to have at least
> three patches in this case, organized like this:
Both obvious improvements. Thanks for teaching me again how to properly
behave on this list, I've not been following it for a while and forgot.
Cheers, and keep up the good work,
Ralf
^ permalink raw reply
* Re: [PATCH] bash: remove dashed command leftovers
From: Shawn O. Pearce @ 2008-11-27 21:24 UTC (permalink / raw)
To: SZEDER GGGbor; +Cc: git
In-Reply-To: <1227792938-4006-1-git-send-email-szeder@ira.uka.de>
SZEDER GGGbor <szeder@ira.uka.de> wrote:
> Commit 5a625b07 (bash: remove fetch, push, pull dashed form leftovers,
> 2008-10-03) did that already, but there were still some git-cmd left
> here and there.
>
> Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
Acked-by: Shawn O. Pearce <spearce@spearce.org>
I really appreciate you taking the time to clean up existing code.
Its never fun, but it really helps to keep it from rotting.
> ---
> contrib/completion/git-completion.bash | 41 ++++++-------------------------
> 1 files changed, 8 insertions(+), 33 deletions(-)
--
Shawn.
^ permalink raw reply
* Re: summaries in git add --patch
From: Jakub Narebski @ 2008-11-27 21:27 UTC (permalink / raw)
To: William Pursell; +Cc: git
In-Reply-To: <492F0CAD.3010101@gmail.com>
William Pursell <bill.pursell@gmail.com> writes:
> I just implemented a command to give a brief summary
> of the patches in the current file.
Good idea.
Side note: you should signoff your patches, following
Documentation/SubmittingPatches. And it would be better
if the patches itself were either threaded, or all be
replies to this cover letter ([PATCH 0/5]) email.
> Please note that
> I am just rediscovering perl after having abandoned it
> years ago, so any criticism is appreciated. 5 patches
> to follow. (From aa14a0c3f)
>
> Here's a screen shot:
>
>
> Stage this hunk [y,n,a,l,d,k,K,j,J,e,?]? l
> '*' indicates current hunk. '+' stage, '-' don't stage
> 0+: @@ -8,9 +8,9 @@ Aani
> 1 : @@ -48,7 +48,7 @@ abandonable
> *2 : @@ -88,7 +88,7 @@ abaton
I hope that contrary to this 'screenshot' actual output is aligned
properly...
> 3 : @@ -128,7 +128,7 @@ abdest
> 4-: @@ -81192,9 +81192,9 @@ gyrous
> 5 : @@ -234925,7 +234925,7 @@ zymotic
That of course assumes that summary catches right thing; still
it is better than nothing.
> @@ -88,7 +88,7 @@ abaton
> abator
> abattoir
> Abatua
> -abature
> +agature
> abave
> abaxial
> abaxile
Same here.
--
Jakub Narebski
Poland
ShadeHawk on #git
^ permalink raw reply
* Re: [PATCH] bash: complete full refs
From: Shawn O. Pearce @ 2008-11-27 21:29 UTC (permalink / raw)
To: SZEDER GGGbor; +Cc: git
In-Reply-To: <1227792947-4055-1-git-send-email-szeder@ira.uka.de>
SZEDER GGGbor <szeder@ira.uka.de> wrote:
> Sometimes it's handy to complete full refs, [...]
>
> To do that, we check whether the ref to be completed starts with
> 'refs'.
...
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 0ee071b..39bf18b 100755
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -188,11 +188,22 @@ __git_tags ()
>
> __git_refs ()
> {
> - local cmd i is_hash=y dir="$(__gitdir "$1")"
> + local i is_hash=y dir="$(__gitdir "$1")"
> + local cur="${COMP_WORDS[COMP_CWORD]}" format refs
> if [ -d "$dir" ]; then
> - if [ -e "$dir/HEAD" ]; then echo HEAD; fi
> - git --git-dir="$dir" for-each-ref --format='%(refname:short)' \
> - refs/tags refs/heads refs/remotes
> + case "$cur" in
> + refs*)
I wonder if the pattern shouldn't be:
refs|refs/*)
to reduce the risk of matching "refs-" and trying to do a full ref
match instead of a short ref match.
Otherwise,
Acked-by: Shawn O. Pearce <spearce@spearce.org>
--
Shawn.
^ permalink raw reply
* Re: [PATCH] bash: offer refs instead of filenames for 'git revert'
From: Shawn O. Pearce @ 2008-11-27 21:30 UTC (permalink / raw)
To: SZEDER GGGbor; +Cc: git
In-Reply-To: <1227792953-4095-1-git-send-email-szeder@ira.uka.de>
SZEDER GGGbor <szeder@ira.uka.de> wrote:
> The completion script for 'git revert' currently offers options and
> filenames. However, 'git revert' doesn't take any filenames from the
> command line, but a single commit. Therefore, it's more sane to offer
> refs instead.
>
> Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
Oh, good catch.
Acked-by: Shawn O. Pearce <spearce@spearce.org>
> ---
> contrib/completion/git-completion.bash | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 39bf18b..7e668d5 100755
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -1348,7 +1348,7 @@ _git_revert ()
> return
> ;;
> esac
> - COMPREPLY=()
> + __gitcomp "$(__git_refs)"
> }
>
> _git_rm ()
> --
> 1.6.0.4.814.gfe502
>
--
Shawn.
^ permalink raw reply
* Re: [JGIT PATCH 0/4] RepositoryTestCase cleanups
From: Shawn O. Pearce @ 2008-11-27 21:49 UTC (permalink / raw)
To: Robin Rosenberg; +Cc: git, fonseca
In-Reply-To: <1227820410-9621-1-git-send-email-robin.rosenberg@dewire.com>
Robin Rosenberg <robin.rosenberg@dewire.com> wrote:
> Ok, so here is an attempt to improve the ability of the JGit's unit
> tests to delete temporary repositories. This has probably been seen
> by many, but Jonas Fonseca raised the issue.
Hmpph. This takes 19 seconds to run the suite, where it used to be
only 2 seconds on the same system. The slower run isn't something
I'm too happy about, actually I'd like to make the run even faster
than 2 seconds.
> The background is that on Windows you cannot delete files that are
> open and mmapped files are open until they get unmapped, which in
> Java is beyond explicit programmer control. You can only free the
> resources and pray that the GC does the work. Fortunately it usually
> does. It turned out our testcases weren't even trying to clean up
> properly.
If the issue is mmap'd files, why don't we instead disable mmap
on Windows during JUnit tests, and use the non-mmap variant of
pack access? At least do that for the bulk of the tests, and
then have a single test case which tests the mmap code path but
has careful System.gc calls in place to try and ensure we can
actually clean up the temporary files.
Another option is to refactor the Repository class a little so we
can replace local filesystem IO with something else, like say an
in-core repository. E.g. a pack file and/or pack index stored in
a byte[], and refs stored in a HashMap. We'd still need a couple
of tests to verify local disk IO, but many of the tests can be
validated against such a pure in-memory Repository concept.
I'd actually like to get that Repository refactoring done soon,
someone else was asking about it for the RefDatabase (to store the
refs in a SQL database so JGit ties into JTA) but I may also want
it for Gerrit 2 - I'm looking at doing something that would put
200,000 refs per year into a repository. That's so large that
most operations can't afford to scan the entire ref database,
and it really cannot be loose. ;-)
Refactoring repository is a fair chunk of work, disabling the mmap
feature under Windows in JUnit may be easier. Hmm, according to
WindowCache's <clinit> its default by false. Why is it enabling
on Windows? The only code that calls WindowCache.reconfigure()
is in the Eclipse plugin, so pure JGit unit tests shouldn't be
turning on mmap code *at all*.
Which also points out a gap in our tests. Nothing new, we have
lots of gaps. *sigh*
--
Shawn.
^ permalink raw reply
* Re: [StGit] Import file(s) problem
From: Catalin Marinas @ 2008-11-27 22:06 UTC (permalink / raw)
To: Shinya Kuribayashi; +Cc: kha, git
In-Reply-To: <492EC5F5.2050807@ruby.dti.ne.jp>
2008/11/27 Shinya Kuribayashi <skuribay@ruby.dti.ne.jp>:
> Today I encountered a problem when importing a patch. I don't know this
> is a known issue or not. If there are good workarounds for this, I'd
> like to know that. Any comments are appreciated.
>
>
> Steps to reproduce
> -------------------
>
> 1. Prepare a patch, say stg-test.patch
>
> 2. Rename it to have '..' extension
The '..' construct has special meaning in both Git and StGit meaning
an interval of commits or patches. We'll need to reject patch names
with '..' to avoid such errors.
--
Catalin
^ permalink raw reply
* Re: What's cooking in git.git (Nov 2008, #06; Wed, 26)
From: Johannes Schindelin @ 2008-11-27 22:49 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7v7i6qc8r0.fsf@gitster.siamese.dyndns.org>
Hi,
On Wed, 26 Nov 2008, Junio C Hamano wrote:
> ----------------------------------------------------------------
> [Will merge to "master" soon]
>
> [...]
>
> * nd/narrow (Tue Nov 18 06:33:16 2008 -0500) 10 commits
> + t2104: touch portability fix
> + grep: skip files outside sparse checkout area
> + checkout_entry(): CE_NO_CHECKOUT on checked out entries.
> + Prevent diff machinery from examining worktree outside sparse
> checkout
> + ls-files: Add tests for --sparse and friends
> + update-index: add --checkout/--no-checkout to update
> CE_NO_CHECKOUT bit
> + update-index: refactor mark_valid() in preparation for new options
> + ls-files: add options to support sparse checkout
> + Introduce CE_NO_CHECKOUT bit
> + Extend index to save more flags
I have a strong suspicion that the narrow stuff will make the worktree
mess pale in comparison.
Note that I do not have time to review this myself (which is not helped at
all by it being no longer a trivial single patch, but a full 10 patches!),
but I really have a bad feeling about this. IMO it is substantially
under-reviewed.
Ciao,
Dscho
^ permalink raw reply
* Re: git-svn clone behaves non-deterministic
From: Josef Wolf @ 2008-11-27 22:40 UTC (permalink / raw)
To: git
In-Reply-To: <20081127182947.GB12716@raven.wolf.lan>
On Thu, Nov 27, 2008 at 07:29:47PM +0100, Josef Wolf wrote:
> - "HEAD points to unborn branch" confirms that the import failed
> - there are exactly 2000 "Checking reflog" lines. Since the last
> imported revision was 2008, I would have expected 2008 (or 2007
> or 2009) such lines. My first thought was this might be related
> to the --repack option defaulting to 1000. But with --repack=570
> it also stops after r2008.
I've done lots of trial-and-error and I think I've finally found the
point where the script stops working. Fortunately, git-svn is a perl
script :-). So I've found in the sub gs_do_update, the
statement
$reporter->finish_report($pool);
is the last executed function. Here is what Data::Dumper says about
the $pool parameter:
$VAR1 = bless(
do {
\(
my $o = bless(
do{
\(my $o = 148559280)
},
'_p_apr_pool_t'
)
)
},
'SVN::Pool'
);
I have tried to format the Data::Dumper output more readable.
Any hints how to get closer to the problem?
^ permalink raw reply
* Re: [PATCH 2/3] Add / command in add --patch
From: Junio C Hamano @ 2008-11-27 22:50 UTC (permalink / raw)
To: William Pursell; +Cc: git
In-Reply-To: <492E1D1D.5090101@gmail.com>
William Pursell <bill.pursell@gmail.com> writes:
> This command allows the user to skip hunks that don't
> match the specified regex.
>
> BUG: if the user enters an invalid regex, perl will abort.
> For example: /+\s*foo will abort with:
> Quantifier follows nothing in regex
I think that is a lessor bug that can be fixed more easily. I think the
bigger problem with your patch is that it breaks the code structure.
If you look at the existing code, you would notice that the loop is
structured in such a way that we show the hunk we currently have focus,
get a command from the user, and the command decides what to do with the
hunk we have focus (no-op for many of them, flip {USE} bit for some) and
where to move the focus (many increments $ix, some decrements $ix). The
"find" command is about not doing anything to {USE} bit and moving the
focus to the hunk that has the text, so you have your additional code
touching wrong section of the code.
I'd suggest redoing [2/3] like this.
git-add--interactive.perl | 25 ++++++++++++++++++++++++-
1 files changed, 24 insertions(+), 1 deletions(-)
diff --git c/git-add--interactive.perl w/git-add--interactive.perl
index f20b880..17724d1 100755
--- c/git-add--interactive.perl
+++ w/git-add--interactive.perl
@@ -800,6 +800,7 @@ y - stage this hunk
n - do not stage this hunk
a - stage this and all the remaining hunks in the file
d - do not stage this hunk nor any of the remaining hunks in the file
+/ - search for a hunk matching the given regex
j - leave this hunk undecided, see next undecided hunk
J - leave this hunk undecided, see next hunk
k - leave this hunk undecided, see previous undecided hunk
@@ -919,7 +920,7 @@ sub patch_update_file {
for (@{$hunk[$ix]{DISPLAY}}) {
print;
}
- print colored $prompt_color, "Stage this hunk [y,n,a,d$other,?]? ";
+ print colored $prompt_color, "Stage this hunk [y,n,a,d,/$other,?]? ";
my $line = <STDIN>;
if ($line) {
if ($line =~ /^y/i) {
@@ -946,6 +947,28 @@ sub patch_update_file {
}
next;
}
+ elsif ($line =~ m|^/(.*)|) {
+ my $search_string;
+ eval {
+ $search_string = qr{$1}m;
+ };
+ if ($@) {
+ my ($err,$exp) = ($@, $1);
+ $err =~ s/ at .*git-add--interactive line \d+, <STDIN> line \d+.*$//;
+ print STDERR "Malformed search regexp $exp: $err\n";
+ next;
+ }
+ my $iy = $ix;
+ while (1) {
+ my $text = join ("", @{$hunk[$iy]{TEXT}});
+ last if ($text =~ $search_string);
+ $iy++;
+ $iy = 0 if ($iy >= $num);
+ last if ($ix == $iy);
+ }
+ $ix = $iy;
+ next;
+ }
elsif ($other =~ /K/ && $line =~ /^K/) {
$ix--;
next;
^ permalink raw reply related
* Re: [RFC PATCH 0/4] Teach git fetch to verify signed tags automatically
From: Deskin Miller @ 2008-11-28 0:09 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vmyfpn10v.fsf@gitster.siamese.dyndns.org>
On Sun, Nov 23, 2008 at 09:30:56PM -0800, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> > Deskin Miller <deskinm@umich.edu> writes:
> >
> >> It struck me a while back when I fetched a new tagged release from git.git that
> >> if I wanted to verify the tag's signature, I'd have to issue another command to
> >> do so. Shouldn't git be able to do that for me automatically, when it fetches
> >> signed tags? Now it does. Also, 'git remote update' gets this for free.
> >
> > I think this should be done inside your own hook. Not interested at all
> > in a solution to touch builtin-fetch.c, unless if the patch is about
> > adding a new hook so that people with other needs can use it as well.
>
> ... or a much stronger case can be made why this shouldn't be done in a
> hook.
>
> I realize "not interested at all" was a bit too strong, so I am trying to
> rephrase it here. The cycle that begins with an RFC that leads to
> discussion and review is about clarifying the rationale and design
> incrementally, so please do not get offended by my no, and sorry for using
> unnecessarily strong wording.
>
> What I meant was more like "The justification as given in the message does
> not interest me in the patch at all as it stands. I do not understand why
> this has to be done as a patch to git-fetch itself, not in a hook script,
> or why doing it inside git-fetch is a better approach than doing it in a
> hook (if there already is a hook mechanism to do this)".
Let's try this then:
-----
Despite core git's built-in support of cryptographic authentication and
integrity verification through the use of signed tags, git still fails
to provide a good first line of defense against malicious entities
hijacking repositories and disseminating arbitrary code, since git does
not try to verify signed tags at the time they are fetched. If such a
compromise occurred, the prospect of even one individual who did not
verify the newly-fetched tag prior to use gives this a large potential
value to attackers, and represents a commensurate risk to the git-using
community.
This patch series mitigates this risk by trying to verify each signed
tag when it is first fetched. Since, however, not everyone is concerned
with the security of signed tags, this feature tries to be conservative
insofar as signatures with public keys which are missing from the user's
keyring do not cause anything to be said about the tag's validity;
furthermore, a configuration variable exists to disable these checks
entirely, if desired.
-----
*the RFC patch series v1 does not include such a configuration variable.
I appreciate that such verification could be accomplished by the
as-yet-nonexistent post-fetch hook, and if that hook existed, I probably
would have done this in that hook. With that said, I do feel like this
feature merits consideration for inclusion in the builtin fetch code.
First, I very much agree with what Dscho said in his review of patch 1,
that hooks represent a rather more advanced feature of git than most
users are willing to investigate.
So the question, then, is whether this feature is important enough to
include in core git. I of course think that it is important enough.
Given that we have git-tag installed by default when one builds git,
there is a certain commitment to supporting the use of signed tags; and
I see verifying them when first fetched as a logical continuation of
this support. As such, a hook seems an unsuitable approach to provide
support for this new use of signed tags.
I'm happy to ask what suggestions you have for someone intending to
implement hooks around fetch; I've not looked at the code in this light,
but someone's got to do it sooner or later.
Deskin Miller
^ permalink raw reply
* Re: [RFC PATCH 0/4] Teach git fetch to verify signed tags automatically
From: Deskin Miller @ 2008-11-28 0:18 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
In-Reply-To: <alpine.DEB.1.00.0811241140280.30769@pacific.mpi-cbg.de>
On Mon, Nov 24, 2008 at 11:41:27AM +0100, Johannes Schindelin wrote:
> On Sun, 23 Nov 2008, Deskin Miller wrote:
>
> > -What to do if a tag is found to have a bad signature?
>
> Or even worse: if the public key was not found? In dubio pro reo, they
> say, but OTOH you asked to verify the signatures...
I don't see how not finding the public key is `worse' than a bad
signature. Compared to what the user learns currently when they run git
fetch and receive new signed tags, the case of not having the required
public key leaves them in exactly the same state: the user does not know
whether the signature is valid or not.
The user didn't ask to verify, as I see it; rather, they asked git to
*try* to verify. If that fails in a way they don't expect, they're free
to investigate further with git tag -v for situations like not having
the right public key.
Deskin Miller
^ permalink raw reply
* Re: [RFC PATCH 1/4] Refactor builtin-verify-tag.c
From: Deskin Miller @ 2008-11-28 0:18 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
In-Reply-To: <alpine.DEB.1.00.0811241147490.30769@pacific.mpi-cbg.de>
On Mon, Nov 24, 2008 at 12:04:59PM +0100, Johannes Schindelin wrote:
> Hi,
>
> On Sun, 23 Nov 2008, Deskin Miller wrote:
>
> > builtin-verify-tag.c didn't expose any of its functionality to be used
> > internally. Refactor some of it into new verify-tag.c and expose
> > verify_tag_sha1 able to be called from elsewhere in git.
> >
> > Signed-off-by: Deskin Miller <deskinm@umich.edu>
> > ---
> > Makefile | 2 +
> > builtin-verify-tag.c | 61 ++-------------------------------------
> > verify-tag.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++++++
> > verify-tag.h | 10 ++++++
> > 4 files changed, 93 insertions(+), 57 deletions(-)
> > create mode 100644 verify-tag.c
> > create mode 100644 verify-tag.h
>
> I'll comment on the output of "format-patch -n -C -C" instead, as that
> makes it much easier to see what you actually did:
Didn't realise -C -C was the magic incantation; I'll remember it for the
future.
> > Makefile | 2 +
> > builtin-verify-tag.c | 61 ++-------------------------------
> > builtin-verify-tag.c => verify-tag.c | 48 ++++-----------------------
> > verify-tag.h | 10 +++++
> > 4 files changed, 23 insertions(+), 98 deletions(-)
> > copy builtin-verify-tag.c => verify-tag.c (56%)
> > create mode 100644 verify-tag.h
> >
> > [...]
> > diff --git a/builtin-verify-tag.c b/verify-tag.c
> > similarity index 56%
> > copy from builtin-verify-tag.c
> > copy to verify-tag.c
> > index 729a159..c9be331 100644
> > --- a/builtin-verify-tag.c
> > +++ b/verify-tag.c
> > @@ -1,18 +1,12 @@
> > /*
> > - * Builtin "git verify-tag"
> > + * Internals for "git verify-tag"
>
> Agree.
>
> > *
> > - * Copyright (c) 2007 Carlos Rica <jasampler@gmail.com>
> > + * Copyright (c) 2008 Deskin Miller <deskinm@umich.edu>
>
> Disagree.
>
> Even if Carlos seemed to stop his work on Git entirely, which I find
> disappointing, you are _not_ free to pretend his work is yours. And given
> this diff:
> [...]
> I think pretty much all you did was deleting (and thereby you do not gain
> any copyright).
I realised my mistake in altering the copyright information just after
sending out these patches. I think I'd written the header first in
verify-tag.c before copying the code in; though I couldn't say what I
thought I'd be writing that would end up protected by copyright. At any
rate, it was an honest mistake, and I apologise, Carlos, for my
unintended plagarism; I'll be sure to restore the proper copyright
notice for any subsequent versions.
> Except for one change: why on earth did you think it a good idea to
> suppress telling the user the _name_ of the tag when an error occurs?
>
> I, for one, would find it way less than helpful to read
>
> Cannot verify a non-tag object of type blob.
>
> than to read
>
> refs/tags/dscho-key: cannot verify a non-tag object of type blob.
>
> Besides, I do not see where you warn that "tag <name> not found." Changes
> like this one need to be justified (by saying in the commit message where
> the warning is already issued, and not letting the reviewer/reader leave
> wondering).
The verify_tag_sha1 function is newly exposed to the rest of git, and
has a different signature from verify_tag, which could take a ref while
verify_tag_sha1 takes a sha1. verify_tag still includes both the checks
you refer to before calling verify_tag_sha1, so the error output is
identical in all cases before and after applying this patch.
The OBJ_TAG check, however, is duplicated so that internal git calls to
verify_tag_sha1 can't pass in e.g. a blob sha1 which just happens to
contain the same contents as a signed tag.
Actually, I initially did not leave the OBJ_TAG check in verify_tag, but
relied on it checking the return value of verify_tag_sha1 to see if an
error occurred, and printing 'Failed to verify <name>' in that case, for
precisely the reason you point out, that the ref name is very useful in
this failure case. However, I ultimately decided to duplicate the check
so that the error output would match up exactly.
> Please, next time you submit a patch like this, do the -C -C yourself.
> Letting all the reviewers do it looks lousy on the overall time balance
> sheet, and it may also lead to a potential reviewer preferring to do
> something else instead.
Will do; thanks for reviewing in spite of my shortcomings.
> Now, Junio already said that he is not (yet) convinced that this change
> should be in Git proper, rather than a hook, so it is up to you to decide
> if you deem it important enough to try harder to convince people.
>
> I, for one, would think that it may be a good change: AFAIK only hard-core
> gits use hooks, everybody else avoids them. So if we deem verifying
> signatures important enough, we might want to have better support for it
> than some example hooks.
>
> So color me half-convinced.
>
> Ciao,
> Dscho
Deskin Miller
^ permalink raw reply
* Re: [RFC PATCH 4/4] Make git fetch verify signed tags
From: Deskin Miller @ 2008-11-28 0:19 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
In-Reply-To: <alpine.DEB.1.00.0811241143410.30769@pacific.mpi-cbg.de>
On Mon, Nov 24, 2008 at 11:44:40AM +0100, Johannes Schindelin wrote:
> Hi,
>
> On Sun, 23 Nov 2008, Deskin Miller wrote:
>
> > When git fetch downloads signed tag objects, make it verify them right
> > then. This extends the output summary of fetch to include "(good
> > signature)" for valid tags and "(BAD SIGNATURE)" for invalid tags. If
> > the user does not have the correct key in the gpg keyring, gpg returns
> > 2, verify_tag_sha1 returns -2 and nothing additional is output about the
> > tag's validity.
>
> This must be turned off by default, IMO. You cannot expect each and every
> developer to have gpg _and_ all those public keys installed.
Adding a configuration variable to control this makes sense, and is on
my TODO list for v2 (core.autoVerifyTags?). However, I don't see a
compelling reason to make it off by default, as if gpg isn't found, or a
particular public key isn't in the keyring, the output is no different
from what fetch prints now.
Deskin Miller
^ permalink raw reply
* Re: summaries in git add --patch
From: Junio C Hamano @ 2008-11-28 0:34 UTC (permalink / raw)
To: William Pursell; +Cc: git
In-Reply-To: <492F0CAD.3010101@gmail.com>
William Pursell <bill.pursell@gmail.com> writes:
> Stage this hunk [y,n,a,l,d,k,K,j,J,e,?]? l
> '*' indicates current hunk. '+' stage, '-' don't stage
> 0+: @@ -8,9 +8,9 @@ Aani
> 1 : @@ -48,7 +48,7 @@ abandonable
> *2 : @@ -88,7 +88,7 @@ abaton
> 3 : @@ -128,7 +128,7 @@ abdest
> 4-: @@ -81192,9 +81192,9 @@ gyrous
> 5 : @@ -234925,7 +234925,7 @@ zymotic
> @@ -88,7 +88,7 @@ abaton
> abator
> abattoir
> Abatua
> -abature
> +agature
> abave
> abaxial
> abaxile
Machines count from zero but humans count from one.
What is your plans to limit the output of this when there are dozens of
hunks?
A hunk can and often is quite long which would make this list scroll off
the screen. Together with the previous point, I suspect it would be
better to make this not part of the "Stage this one?" question, but an
action that (1) does not do anything to the hunk we have currently focus
on, and (2) does not move the focus after it does its thing. In other
words, a new "status" action. I think 'S' is not taken yet although 's'
is taken for 'split'.
^ permalink raw reply
* Re: [PATCH] bash: complete full refs
From: Shawn O. Pearce @ 2008-11-28 0:51 UTC (permalink / raw)
To: SZEDER GGGbor; +Cc: git
In-Reply-To: <20081128004638.GA6854@neumann>
SZEDER GGGbor <szeder@ira.uka.de> wrote:
> Sometimes it's handy to complete full refs, e.g. the user has some
> refs outside of refs/{heads,remotes,tags} or the user wants to
> complete some git command's special refs (like 'git show
> refs/bisect/bad').
...
> On Thu, Nov 27, 2008 at 01:29:38PM -0800, Shawn O. Pearce wrote:
> > I wonder if the pattern shouldn't be:
> >
> > refs|refs/*)
> >
> > to reduce the risk of matching "refs-" and trying to do a full ref
> > match instead of a short ref match.
> Good point. Adjusted the code and commit message accordingly.
Thanks.
Acked-by: Shawn O. Pearce <spearce@spearce.org>
> contrib/completion/git-completion.bash | 19 +++++++++++++++----
> 1 files changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 0ee071b..244ed41 100755
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -188,11 +188,22 @@ __git_tags ()
>
> __git_refs ()
> {
> - local cmd i is_hash=y dir="$(__gitdir "$1")"
> + local i is_hash=y dir="$(__gitdir "$1")"
> + local cur="${COMP_WORDS[COMP_CWORD]}" format refs
> if [ -d "$dir" ]; then
> - if [ -e "$dir/HEAD" ]; then echo HEAD; fi
> - git --git-dir="$dir" for-each-ref --format='%(refname:short)' \
> - refs/tags refs/heads refs/remotes
> + case "$cur" in
> + refs|refs/*)
> + format="refname"
> + refs="${cur%/*}"
> + ;;
> + *)
> + if [ -e "$dir/HEAD" ]; then echo HEAD; fi
> + format="refname:short"
> + refs="refs/tags refs/heads refs/remotes"
> + ;;
> + esac
> + git --git-dir="$dir" for-each-ref --format="%($format)" \
> + $refs
> return
> fi
> for i in $(git ls-remote "$dir" 2>/dev/null); do
> --
> 1.6.0.4.814.gfe502
>
--
Shawn.
^ permalink raw reply
* [PATCH] bash: complete full refs
From: SZEDER Gábor @ 2008-11-28 0:46 UTC (permalink / raw)
To: Shawn O. Pearce; +Cc: git
In-Reply-To: <20081127212938.GB23984@spearce.org>
Sometimes it's handy to complete full refs, e.g. the user has some
refs outside of refs/{heads,remotes,tags} or the user wants to
complete some git command's special refs (like 'git show
refs/bisect/bad').
To do that, we check whether the ref to be completed starts with
'refs/' or is 'refs' (to reduce the risk of matching 'refs-'). If it
does, then we offer full refs for completion; otherwise everything
works as usual.
This way the impact on the common case is fairly small (hopefully not
many users have branches or tags starting with 'refs'), and in the
special case the cost of typing out 'refs' is bearable.
While at it, also remove the unused 'cmd' variable from '__git_refs'.
Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
---
On Thu, Nov 27, 2008 at 01:29:38PM -0800, Shawn O. Pearce wrote:
> I wonder if the pattern shouldn't be:
>
> refs|refs/*)
>
> to reduce the risk of matching "refs-" and trying to do a full ref
> match instead of a short ref match.
Good point. Adjusted the code and commit message accordingly.
contrib/completion/git-completion.bash | 19 +++++++++++++++----
1 files changed, 15 insertions(+), 4 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 0ee071b..244ed41 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -188,11 +188,22 @@ __git_tags ()
__git_refs ()
{
- local cmd i is_hash=y dir="$(__gitdir "$1")"
+ local i is_hash=y dir="$(__gitdir "$1")"
+ local cur="${COMP_WORDS[COMP_CWORD]}" format refs
if [ -d "$dir" ]; then
- if [ -e "$dir/HEAD" ]; then echo HEAD; fi
- git --git-dir="$dir" for-each-ref --format='%(refname:short)' \
- refs/tags refs/heads refs/remotes
+ case "$cur" in
+ refs|refs/*)
+ format="refname"
+ refs="${cur%/*}"
+ ;;
+ *)
+ if [ -e "$dir/HEAD" ]; then echo HEAD; fi
+ format="refname:short"
+ refs="refs/tags refs/heads refs/remotes"
+ ;;
+ esac
+ git --git-dir="$dir" for-each-ref --format="%($format)" \
+ $refs
return
fi
for i in $(git ls-remote "$dir" 2>/dev/null); do
--
1.6.0.4.814.gfe502
^ permalink raw reply related
* Re: [RFC PATCH 0/4] Teach git fetch to verify signed tags automatically
From: Johannes Schindelin @ 2008-11-28 1:18 UTC (permalink / raw)
To: Deskin Miller; +Cc: Junio C Hamano, git
In-Reply-To: <20081128000606.GB2759@euler>
Hi,
On Thu, 27 Nov 2008, Deskin Miller wrote:
> This patch series mitigates this risk by trying to verify each signed
> tag when it is first fetched. Since, however, not everyone is concerned
> with the security of signed tags, this feature tries to be conservative
> insofar as signatures with public keys which are missing from the user's
> keyring do not cause anything to be said about the tag's validity;
Now, in the context of security, this is not conservative. Conservative
would be to fail as soon as a signature could not be verified, be it that
there is no key to match against, or that the signature is corrupt.
Your notion to fail silently if the necessary keys were not found makes
your patch series rather useless, no?
After all, the whole idea is to let Git check if every signature is
correct, and when Git does not fail, rely on them being valid.
So I think that the _only_ thing that would make sense is to fail _unless_
all the signatures were verified to be correct.
_That_ is why I want this feature to be off by default.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH 2/5] Name change: other -> commands.
From: Junio C Hamano @ 2008-11-28 1:27 UTC (permalink / raw)
To: William Pursell; +Cc: git
In-Reply-To: <492F0CB8.2080808@gmail.com>
I see this needless churn. Some commands are always available, so these
are indeed other commands.
^ permalink raw reply
* Re: [PATCH 3/5] Removed unused variables prev and next.
From: Junio C Hamano @ 2008-11-28 1:28 UTC (permalink / raw)
To: William Pursell; +Cc: git
In-Reply-To: <492F0CBF.3010202@gmail.com>
Hmm, I was hoping that use of these instead of $other =~ /,k/ might be
more efficient some day, but Ok.
^ permalink raw reply
* Re: [RFC PATCH 0/4] Teach git fetch to verify signed tags automatically
From: Junio C Hamano @ 2008-11-28 1:43 UTC (permalink / raw)
To: Deskin Miller; +Cc: Johannes Schindelin, git
In-Reply-To: <20081128001825.GA29662@euler>
Deskin Miller <deskinm@umich.edu> writes:
> The user didn't ask to verify, as I see it; rather, they asked git to
> *try* to verify.
If that is your argument, I really do not see any point in your patch.
They asked git to fetch, and did not say anything about trying anything
else.
^ 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