* Re: [PATCHv3 1/5] refs: add match_pattern()
From: Junio C Hamano @ 2012-02-22 6:33 UTC (permalink / raw)
To: Tom Grennan; +Cc: git, peff, jasampler, pclouds
In-Reply-To: <1329874130-16818-2-git-send-email-tmgrennan@gmail.com>
Tom Grennan <tmgrennan@gmail.com> writes:
> +static int match_path(const char *name, const char *pattern, int nlen)
> +{
> + int plen = strlen(pattern);
> +
> + return ((plen <= nlen) &&
> + !strncmp(name, pattern, plen) &&
> + (name[plen] == '\0' ||
> + name[plen] == '/' ||
> + pattern[plen-1] == '/'));
> +}
This is a counterpart to the tail match found in ls-remote, so we would
want to call it with a name that makes it clear this is a leading path
match not just "path" match. Perhaps match_leading_path() or something.
> +int match_pattern(const char *name, const char **match,
> + struct string_list *exclude, int flags)
> +{
> + int nlen = strlen(name);
> +
> + if (exclude) {
> + struct string_list_item *x;
> + for_each_string_list_item(x, exclude) {
> + if (!fnmatch(x->string, name, 0))
> + return 0;
> + }
> + }
> + if (!match || !*match)
> + return 1;
> + for (; *match; match++) {
> + if (flags == FNM_PATHNAME)
> + if (match_path(name, *match, nlen))
> + return 1;
> + if (!fnmatch(*match, name, flags))
> + return 1;
> + }
> + return 0;
> +}
As an API for a consolidated and generic function, the design needs a bit
more improving, I would think.
- The name match_pattern() was OK for a static function inside a single
file, but it is way too vague for a global function. This is to match
refnames, so I suspect there should at least be a string "ref_"
somewhere in its name.
- You pass "flags" argument, so that later we _could_ enhance the
implementation to cover needs for new callers, but alas, it uses its
full bits to express only one "do we do FNM_PATHNAME or not?" bit of
information, so essentially "flags" does not give us any expandability.
- Is it a sane assumption that a caller that asks FNM_PATHNAME will
always want match_path() semantics, too? Aren't these two logically
independent?
- Is it a sane assumption that a caller that gives an exclude list will
want neither FNM_PATHNAME semantics nor match_path() semantics?
- Positive patterns are passed in "const char **match", and negative ones
are in "struct string_list *". Doesn't the inconsistency strike you as
strange?
Perhaps like...
#define REF_MATCH_LEADING 01
#define REF_MATCH_TRAILING 02
#define REF_MATCH_FNM_PATH 04
static int match_one(const char *name, size_t namelen, const char *pattern,
unsigned flags)
{
if ((flags & REF_MATCH_LEADING) &&
match_leading_path(name, pattern, namelen))
return 1;
if ((flags & REF_MATCH_TRAILING) &&
match_trailing_path(name, pattern, namelen))
return 1;
if (!fnmatch(pattern, name,
(flags & REF_MATCH_FNM_PATH) ? FNM_PATHNAME : 0))
return 1;
return 0;
}
int ref_match_pattern(const char *name,
const char **pattern, const char **exclude, unsigned flags)
{
size_t namelen = strlen(name);
if (exclude) {
while (*exclude) {
if (match_one(name, namelen, *exclude, flags))
return 0;
exclude++;
}
}
if (!pattern || !*pattern)
return 1;
while (*pattern) {
if (match_one(name, namelen, *pattern, flags))
return 1;
pattern++;
}
return 0;
}
and then the caller could do something like
ref_match_pattern("refs/heads/master",
["maste?", NULL],
["refs/heads/", NULL],
(REF_MATCH_FNM_PATH|REF_MATCH_LEADING));
Note that the above "ref_match_pattern()" gives the same "flags" for the
call to match_one() for elements in both positive and negative array and
it is very deliberate. See review comment to [3/5] for the reasoning.
Thanks.
^ permalink raw reply
* Re: [PATCHv3 3/5] tag --exclude option
From: Junio C Hamano @ 2012-02-22 6:33 UTC (permalink / raw)
To: Tom Grennan; +Cc: git, peff, jasampler, pclouds
In-Reply-To: <1329874130-16818-4-git-send-email-tmgrennan@gmail.com>
Tom Grennan <tmgrennan@gmail.com> writes:
> Example,
> $ git tag -l --exclude "*-rc?" "v1.7.8*"
> v1.7.8
> v1.7.8.1
> v1.7.8.2
> v1.7.8.3
> v1.7.8.4
>
> Which is equivalent to,
> $ git tag -l "v1.7.8*" | grep -v \\-rc.
> v1.7.8
> v1.7.8.1
> v1.7.8.2
> v1.7.8.3
> v1.7.8.4
>
> Signed-off-by: Tom Grennan <tmgrennan@gmail.com>
Having an example is a good way to illustrate your explanation, but it is
not a substitution. Could we have at least one real sentence to describe
what the added option *does*?
This comment applies to all the patches in this series except for the
second patch.
> diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
> index 8d32b9a..470bd80 100644
> --- a/Documentation/git-tag.txt
> +++ b/Documentation/git-tag.txt
> @@ -13,7 +13,7 @@ SYNOPSIS
> <tagname> [<commit> | <object>]
> 'git tag' -d <tagname>...
> 'git tag' [-n[<num>]] -l [--contains <commit>] [--points-at <object>]
> - [<pattern>...]
> + [--exclude <pattern>] [<pattern>...]
> 'git tag' -v <tagname>...
>
> DESCRIPTION
> @@ -90,6 +90,10 @@ OPTIONS
> --points-at <object>::
> Only list tags of the given object.
>
> +--exclude <pattern>::
> + Don't list tags matching the given pattern. This has precedence
> + over any other match pattern arguments.
As you do not specify what kind of pattern matching is done to this
exclude pattern, it is important to use the same logic between positive
and negative ones to give users a consistent UI. Unfortunately we use
fnmatch without FNM_PATHNAME for positive ones, so this exclude pattern
needs to follow the same semantics to reduce confusion.
This comment applies to all the patches in this series to add this option
to existing commands that take the positive pattern.
> @@ -202,6 +206,15 @@ test_expect_success \
> '
>
> cat >expect <<EOF
> +v0.2.1
> +EOF
> +test_expect_success \
> + 'listing tags with a suffix as pattern and prefix exclusion' '
> + git tag -l --exclude "v1.*" "*.1" > actual &&
> + test_cmp expect actual
> +'
I know you are imitating the style of surrounding tests that is an older
parts of this script, but it is an eyesore. More modern tests are written
like this:
test_expect_success 'label for the test' '
cat >expect <<-EOF &&
v0.2.1
EOF
git tag -l ... >actual &&
test_cmp expect actual
'
to avoid unnecessary backslash on the first line, and have the preparation
of test vectore _inside_ test_expect_success. We would eventually want to
update the older part to the newer style for consistency.
Two possible ways to go about this are (1) have a "pure style" patch at
the beginning to update older tests to a new style and then add new code
and new test as a follow-up patch written in modern, or (2) add new code
and new test in modern, and make a mental note to update the older ones
after the dust settles. Adding new tests written in older style to a file
that already has mixed styles is the worst thing you can do.
This comment applies to all the patches in this series with tests.
Thanks.
^ permalink raw reply
* Re: [PATCH] git-svn.perl: fix a false-positive in the "already exists" test
From: Junio C Hamano @ 2012-02-22 5:08 UTC (permalink / raw)
To: Steven Walter; +Cc: normalperson, git
In-Reply-To: <CAK8d-aLXs0yMzYMXm7fKytOGDXesUEx7a8PN_Mg9gw6+Q6OTBA@mail.gmail.com>
Steven Walter <stevenrwalter@gmail.com> writes:
>>> + test -e "$SVN_TREE"/bar/zzz/yyy ' || true
>>
>> Care to explain what this " || true" is doing here, please?
>
> Ahh, good catch. I think the answer is that it shouldn't be there.
> It was originally there because of the "test_must_fail" line, I think
> (at least the other tests that use test_must_fail also have "||
> true").
Ok, that may explain the copy&paste error.
But I do not think test_must_fail followed by || true makes much sense,
either. The purpose of "test_must_fail" is to make sure the tested git
command exits with non-zero status in a controlled way (i.e. not crash)
so if the tested command that is expected to exit with non-zero status
exited with zero status, the test has detected an *error*. E.g. if you
know that the index and the working tree are different at one point in the
test sequence, you would say:
... other setup steps ... &&
test_must_fail git diff --exit-code &&
... and other tests ...
so that failure by "git diff --exit-code" to exit with non-zero status
(i.e. it did not find any difference when it should have) breaks the &&
cascade.
I just took a quick look at t9100 but I think all " || true" can be safely
removed. None of them is associated with test_must_fail in any way. For
whatever reason, these test seem to do
test_expect_success 'label of the test' '
body of the test
' || true
for no good reason.
> Do you want to just fix that up, or a new version of the original patch,
> or a fix on top of the original patches?
Eric queued the patch and then had me pull it as part of his history
already, so it is doubly too late to replace it.
Can you apply this patch and re-test?
t/t9100-git-svn-basic.sh | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/t/t9100-git-svn-basic.sh b/t/t9100-git-svn-basic.sh
index 4029f84..749b75e 100755
--- a/t/t9100-git-svn-basic.sh
+++ b/t/t9100-git-svn-basic.sh
@@ -65,7 +65,8 @@ test_expect_success "$name" "
git update-index --add dir/file/file &&
git commit -m '$name' &&
test_must_fail git svn set-tree --find-copies-harder --rmdir \
- ${remotes_git_svn}..mybranch" || true
+ ${remotes_git_svn}..mybranch
+"
name='detect node change from directory to file #1'
@@ -79,7 +80,8 @@ test_expect_success "$name" '
git update-index --add -- bar &&
git commit -m "$name" &&
test_must_fail git svn set-tree --find-copies-harder --rmdir \
- ${remotes_git_svn}..mybranch2' || true
+ ${remotes_git_svn}..mybranch2
+'
name='detect node change from file to directory #2'
@@ -96,7 +98,8 @@ test_expect_success "$name" '
${remotes_git_svn}..mybranch3 &&
svn_cmd up "$SVN_TREE" &&
test -d "$SVN_TREE"/bar/zzz &&
- test -e "$SVN_TREE"/bar/zzz/yyy ' || true
+ test -e "$SVN_TREE"/bar/zzz/yyy
+'
name='detect node change from directory to file #2'
test_expect_success "$name" '
@@ -109,7 +112,8 @@ test_expect_success "$name" '
git update-index --add -- dir &&
git commit -m "$name" &&
test_must_fail git svn set-tree --find-copies-harder --rmdir \
- ${remotes_git_svn}..mybranch4' || true
+ ${remotes_git_svn}..mybranch4
+'
name='remove executable bit from a file'
@@ -162,7 +166,7 @@ test_expect_success "$name" '
name='modify a symlink to become a file'
test_expect_success "$name" '
- echo git help > help || true &&
+ echo git help >help &&
rm exec-2.sh &&
cp help exec-2.sh &&
git update-index exec-2.sh &&
^ permalink raw reply related
* Re: git status: small difference between stating whole repository and small subdirectory
From: Junio C Hamano @ 2012-02-22 3:32 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy
Cc: Thomas Rast, Jeff King, Piotr Krukowiecki, Git Mailing List
In-Reply-To: <CACsJy8DE86qzA1=GiKZFRCt5aH8X4iMyDvfrhnqwmbq52szhHg@mail.gmail.com>
Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
>> diff --git a/builtin/checkout.c b/builtin/checkout.c
>> index 5bf96ba..c06287a 100644
>> --- a/builtin/checkout.c
>> +++ b/builtin/checkout.c
>> @@ -319,6 +319,10 @@ static void show_local_changes(struct object *head, struct diff_options *opts)
>> die(_("diff_setup_done failed"));
>> add_pending_object(&rev, head, NULL);
>> run_diff_index(&rev, 0);
>> + if (!DIFF_OPT_TST(&rev.diffopt, HAS_CHANGES)) {
>> + struct tree *tree = parse_tree_indirect(head->sha1);
>> + prime_cache_tree(&active_cache_tree, tree);
>> + }
>> }
I think this patch is wrong on at least two counts.
* The run_diff_index(&rev, 0) you reused is doing "diff HEAD" and not
"diff --cached HEAD". The added check does not say anything about the
comparison between the index and the tree at the HEAD.
* Even if we added an extra run_diff_index(&rev, 1) there, or added a
call to index_differs_from() to run "diff --cached HEAD" to check what
needs to be checked, it is still not quite right.
On the latter point, imagine what happens in the two invocations of
checkout in the following sequence:
$ git reset --hard master
$ git checkout master
$ git checkout master
The second one should notice that the cache tree is fully valid, so the
internal "diff --cached" it runs should only open the top-level tree
and scan entries in it, without recursing into any of the subtrees, and
realize that the index is in sync with "HEAD", which should be a very
cheap operation (that is the whole point of the current topic of our
discussion looking at the cache-tree). Then the new code calls
prime_cache_tree() to read _everything_?
Probably cache_tree_fully_valid() should be called before deciding that we
need to re-populate the cache tree from "HEAD".
^ permalink raw reply
* Re: [PATCH 4/4] Only re-encode certain parts in commit object, not the whole
From: Junio C Hamano @ 2012-02-22 3:14 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: Jeff King, git
In-Reply-To: <CACsJy8BZixyzf73TGPdf+_=rz59J4GWUq8B8WXuf+n97-OF=sQ@mail.gmail.com>
By the way, zj/term-columns topic has already graduated to 'master', so if
you are still interested in your earlier nd/columns topic, it would be a
good time to re-roll it.
No hurries, but pointing it out just in case you forgot.
^ permalink raw reply
* Re: git status: small difference between stating whole repository and small subdirectory
From: Junio C Hamano @ 2012-02-22 2:55 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy
Cc: Thomas Rast, Jeff King, Piotr Krukowiecki, Git Mailing List
In-Reply-To: <CACsJy8C3Myqs4=GvURWqCTxGp0R1RWotdiHGnnvBSaxyTteujw@mail.gmail.com>
Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
> That makes me think if "diff --cached" can take advantage of
> cache-tree to avoid walking down valid cached trees and do tree-tree
> diff in those cases instead. Not sure if it gains us anything but code
> complexity.
Why do I have this funny feeling that we saw that comment in this thread
already?
^ permalink raw reply
* Re: [PATCH] contrib: added git-diffall
From: Junio C Hamano @ 2012-02-22 2:41 UTC (permalink / raw)
To: Tim Henigan; +Cc: git, David Aguilar
In-Reply-To: <CAFouetiLmK3dXLRkBh+cTNA_OMPS77xo8z95WK5y4tk-o-UUog@mail.gmail.com>
Tim Henigan <tim.henigan@gmail.com> writes:
> There is no specific reason it must be bash. I changed from
> "#!/bin/sh" to "#!/bin/bash -e" due to a bug report from a user on
> Ubuntu [1]. The user reported that:
>
> "If you use /bin/sh on ubuntu you get the dash shell instead of bash shell.
> This causes git_merge_tool_path to fail. The error isn't trapped,
> so it exits
> without displaying anything and without cleaning up."
>
> Given that all the other scripts distributed with git use /bin/sh, I
> will change this script to match.
You need to dig back to that bug report deeper and find out what exactly
is causing the "failure", before blindly allowing /bin/dash to be used.
I think the above function name is a typo of get_merge_tool_path that is
borrowed from git-mergetool--lib.sh, but nothing in the function jumps as
a blatant bash-ism at me from a quick reading.
David, any idea on this?
>> The following is only after a cursory scanning, so there may be other
>> things that needs fixing, but anyway:
>>
>> - Don't use "which" in scripts. Its output is not machine parseable, and
>> exit code is not reliable, across platforms. It is only meant for
>> consumption by human who can read English (or natural language in the
>> current locale).
>
> I used "which" in two places. Both were added to support problems
> with missing standard tools on certain platforms (missing mktemp on
> msysgit and missing option from tar on Mac [2]). Is there some other
> standard way to detect the platform or if certain utils are present?
There are examples in the script you are borrowing functions from, even in
the function that allegedly fail for the dash user ;-).
> The cleanup triggers on all the platforms I have tested (Ubuntu,
> msysgit, Mac). I could change it, but for me it has "just worked".
You set "trap cleanup" for exit event, and then the control reaches the
end of the script, which is an exit event, and the cleanup function is
called. So it is natural that if you manage to get to that "trap cleanup"
line, of course cleanup will run. But if you dropped "trap" and "EXIT"
from that line, it amounts to the same thing.
A more important thing to know is that until the control reaches that
"trap cleanup EXIT" line, you do not have that trap set. So if you caused
the program to exit in an earlier part of the program (say, in the big
"while read name" loop) before the control reaches this line, you won't
see any cleanup.
Perhaps that is what you wanted, but then again writing "trap" and "EXIT"
there is pointless.
Usually people set up a clean-up task with trap before going into complex
stuff, so that no matter where in the complex code the trapped condition
(typically a signal, but an exit event is also possible) happens, they can
be sure the clean-up task is run. That is the reason behind the following
ordering.
>> If you are to set up temporary files or directories that you want to clean
>> up, a good discipline is to follow this order:
>>
>> - define variable(s) to hold the temporary locations, e.g.
>> tmpdir=$(mktemp ...)
>>
>> - set the trap before starting to use these temporary locations, e.g.
>> trap 'rm -rf "$tmpdir' 0 1 2 3 15
>>
>> - and then start populating tmpdir and do whatever you want to do.
^ permalink raw reply
* Re: [PATCH] git-svn.perl: fix a false-positive in the "already exists" test
From: Steven Walter @ 2012-02-22 2:32 UTC (permalink / raw)
To: Junio C Hamano; +Cc: normalperson, git
In-Reply-To: <7vk43feho8.fsf@alter.siamese.dyndns.org>
On Tue, Feb 21, 2012 at 9:16 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Steven Walter <stevenrwalter@gmail.com> writes:
>
>> diff --git a/t/t9100-git-svn-basic.sh b/t/t9100-git-svn-basic.sh
>> index b041516..4029f84 100755
>> --- a/t/t9100-git-svn-basic.sh
>> +++ b/t/t9100-git-svn-basic.sh
>> @@ -92,9 +92,11 @@ test_expect_success "$name" '
>> echo yyy > bar/zzz/yyy &&
>> git update-index --add bar/zzz/yyy &&
>> git commit -m "$name" &&
>> + git svn set-tree --find-copies-harder --rmdir \
>> + ${remotes_git_svn}..mybranch3 &&
>> + svn_cmd up "$SVN_TREE" &&
>> + test -d "$SVN_TREE"/bar/zzz &&
>> + test -e "$SVN_TREE"/bar/zzz/yyy ' || true
>
> Care to explain what this " || true" is doing here, please?
Ahh, good catch. I think the answer is that it shouldn't be there.
It was originally there because of the "test_must_fail" line, I think
(at least the other tests that use test_must_fail also have "||
true"). The tests all still pass with that "|| true" removed. Do you
want to just fix that up, or a new version of the original patch, or a
fix on top of the original patches?
--
-Steven Walter <stevenrwalter@gmail.com>
^ permalink raw reply
* Re: [PULL git-svn] various git svn updates
From: Junio C Hamano @ 2012-02-22 2:18 UTC (permalink / raw)
To: Eric Wong
Cc: git, Steven Walter, Steven Walter, Wei-Yin Chen,
Ævar Arnfjörð Bjarmason, Frederic Heitzmann
In-Reply-To: <20120222003857.GA1212@dcvr.yhbt.net>
Eric Wong <normalperson@yhbt.net> writes:
> The following changes since commit b3a769dc355b32c95783dc07f59e4dfebdd8bdc7:
>
> Update draft release notes to 1.7.10 (2012-02-20 00:29:40 -0800)
>
> are available in the git repository at:
>
> git://bogomips.org/git-svn master
>
> for you to fetch changes up to 379862ec5a413e636d977a6ea3d618f0b3eafceb:
>
> git-svn.perl: fix a false-positive in the "already exists" test (2012-02-21 21:37:31 +0000)
Thanks.
You didn't seem to sign off any of these changes yourself, but as I've
already pulled them, so it is a bit too late X-<.
Also there was one funny bit in the updates to t9100 I asked in a separate
message.
^ permalink raw reply
* Re: [PATCH 2/4] Do attempt pretty print in ASCII-incompatible encodings
From: Nguyen Thai Ngoc Duy @ 2012-02-22 2:17 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20120221182118.GA32668@sigill.intra.peff.net>
2012/2/22 Jeff King <peff@peff.net>:
> On Tue, Feb 21, 2012 at 09:24:50PM +0700, Nguyen Thai Ngoc Duy wrote:
>
>> We rely on ASCII everywhere. We print "\n" directly without conversion
>> for example. The end result would be a mix of some encoding and ASCII
>> if they are incompatible. Do not do that.
>>
>> In theory we could convert everything to utf-8 as intermediate medium,
>> process process process, then convert final output to the desired
>> encoding. But that's a lot of work (unless we have a pager-like
>> converter) with little real use. Users can just pipe everything to
>> iconv instead.
>
> I'm not sure why we bother checking this. Using non-ASCII-superset
> encodings is broken, yes, but are people actually doing that? I assume
> that the common one is utf-16, and anybody using it will experience
> severe breakage immediately. So are people actually doing this? Are
> there actually encodings that will cause subtle breakage that we want to
> catch?
I did :-) once actually. But that's a good point, using unsuitable
encoding leads to garbage output, but no subtle breakage there. It'd
be nice to say "your encoding is not supported" than throw garbage,
but again probably no one did it but me, and I don't feel like doing
it again.
--
Duy
^ permalink raw reply
* Re: [PATCH] git-svn.perl: fix a false-positive in the "already exists" test
From: Junio C Hamano @ 2012-02-22 2:16 UTC (permalink / raw)
To: Steven Walter; +Cc: normalperson, gitster, git
In-Reply-To: <1329747474-17976-1-git-send-email-stevenrwalter@gmail.com>
Steven Walter <stevenrwalter@gmail.com> writes:
> diff --git a/t/t9100-git-svn-basic.sh b/t/t9100-git-svn-basic.sh
> index b041516..4029f84 100755
> --- a/t/t9100-git-svn-basic.sh
> +++ b/t/t9100-git-svn-basic.sh
> @@ -92,9 +92,11 @@ test_expect_success "$name" '
> echo yyy > bar/zzz/yyy &&
> git update-index --add bar/zzz/yyy &&
> git commit -m "$name" &&
> + git svn set-tree --find-copies-harder --rmdir \
> + ${remotes_git_svn}..mybranch3 &&
> + svn_cmd up "$SVN_TREE" &&
> + test -d "$SVN_TREE"/bar/zzz &&
> + test -e "$SVN_TREE"/bar/zzz/yyy ' || true
Care to explain what this " || true" is doing here, please?
^ permalink raw reply
* Re: git status: small difference between stating whole repository and small subdirectory
From: Nguyen Thai Ngoc Duy @ 2012-02-22 2:12 UTC (permalink / raw)
To: Junio C Hamano
Cc: Thomas Rast, Jeff King, Piotr Krukowiecki, Git Mailing List
In-Reply-To: <7v8vjwgfoq.fsf@alter.siamese.dyndns.org>
On Wed, Feb 22, 2012 at 2:16 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Because prime_cache_tree() by itself is a fairly expensive operation that
> reads all the trees recursively, its benefits need to be evaluated. It
> should to happen only in an operation that is already heavy-weight, is
> likely to have read all the trees and have many of them in-core cache, and
> also relatively rarely happens compared to "git add" so that the cost can
> be amortised over time, such as "reset --(hard|mixed)".
>
> Switching branches is likely to fall into that category, but that is just
> my gut feeling. I would feel better at night if somebody did a benchmark
> ;-)
In this particular case, "git diff --cached" is run internally, so I
say all trees are read once and hopefully most of them still in OS
cache. Will run some benchmark, maybe with the coming perf test suite.
> One thing we do not currently do anywhere that _might_ be of merit is to
> make a call to cache_tree_update() instead of prime_cache_tree() when we
> already know that only a very small subpart of the cache-tree is invalid
> and it is cheaper to repair it by rehashing only a small portion of the
> index than to re-prime the entire cache tree with prime_cache_tree().
That makes me think if "diff --cached" can take advantage of
cache-tree to avoid walking down valid cached trees and do tree-tree
diff in those cases instead. Not sure if it gains us anything but code
complexity.
--
Duy
^ permalink raw reply
* Re: [PATCH] contrib: added git-diffall
From: Tim Henigan @ 2012-02-22 2:02 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vd397g8ic.fsf@alter.siamese.dyndns.org>
Thank you for taking the time to review this patch. I appreciate it.
On Tue, Feb 21, 2012 at 4:51 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Tim Henigan <tim.henigan@gmail.com> writes:
>> +#!/bin/bash -e
>
> Does this have to be bash-only (iow, infested with bash-isms beyond
> repair), or is you wrote this merely from inertia?
There is no specific reason it must be bash. I changed from
"#!/bin/sh" to "#!/bin/bash -e" due to a bug report from a user on
Ubuntu [1]. The user reported that:
"If you use /bin/sh on ubuntu you get the dash shell instead of bash shell.
This causes git_merge_tool_path to fail. The error isn't trapped,
so it exits
without displaying anything and without cleaning up."
Given that all the other scripts distributed with git use /bin/sh, I
will change this script to match.
> The following is only after a cursory scanning, so there may be other
> things that needs fixing, but anyway:
>
> - Don't use "which" in scripts. Its output is not machine parseable, and
> exit code is not reliable, across platforms. It is only meant for
> consumption by human who can read English (or natural language in the
> current locale).
I used "which" in two places. Both were added to support problems
with missing standard tools on certain platforms (missing mktemp on
msysgit and missing option from tar on Mac [2]). Is there some other
standard way to detect the platform or if certain utils are present?
>> + if test -z "$paths"
>> + then
>> + paths=$1
>> + else
>> + paths="$paths $1"
>> + fi
>
> Just a style tip; if you are going to let shell $IFS split this list
> anyway, it is customary to write the above as
>
> paths="$paths$1 "
Nice...I will change to use this format.
>> + git diff --name-only "$left"..."$right" -- $paths > "$tmp"/filelist
>
> git diff ... -- $paths >"$tmp/filelist"
... <snip>
> mkdir -p "$tmp/$left_dir" "$tmp/$right_dir"
Will change all instances.
>> + if test -n "$compare_staged"
>> + then
>> + ls_list=$(git ls-tree HEAD $name)
>> + if test -n "$ls_list"
>> + then
>> + mkdir -p "$tmp"/"$left_dir"/"$(dirname "$name")"
>> + git show HEAD:"$name" > "$tmp"/"$left_dir"/"$name"
>> + fi
>> + else
>> + mkdir -p "$tmp"/"$left_dir"/"$(dirname "$name")"
>> + git show :"$name" > "$tmp"/"$left_dir"/"$name"
>> + fi
>
> That's misleadingly indented. First I thought "in what case would we want
> to switch the LHS between HEAD:$path and :$path when doing diff --cached?"
> but the overindented four lines starting from the funny "fi" is about non
> cached case.
That is embarrassing. I will fix it.
<snip>
>> + find "$tmp/$right_dir" -type f|while read file; do
>> + cp "$file" "$git_top_dir/${file#$tmp/$right_dir/}"
>> + done
>
> Why is this loop written in such a dense way? Everything else (except for
> that misindented part) were almost to our CodingStyle and was fairly easy
> to read, though.
I missed this in my style cleanup. I will fix it.
>> + fi
>> +
>> + # Remove the tmp directory
>> + rm -rf "$tmp"
>> +}
>> +
>> +trap cleanup EXIT
>
> Does this even trigger? This is not Perl that parses and runs set-up code
> before executing everything else, so I suspect this last line amounts to
> the same thing as writing just
>
> cleanup
>
> without trap nor signal names.
The cleanup triggers on all the platforms I have tested (Ubuntu,
msysgit, Mac). I could change it, but for me it has "just worked".
> If you are to set up temporary files or directories that you want to clean
> up, a good discipline is to follow this order:
>
> - define variable(s) to hold the temporary locations, e.g.
> tmpdir=$(mktemp ...)
>
> - set the trap before starting to use these temporary locations, e.g.
> trap 'rm -rf "$tmpdir' 0 1 2 3 15
>
> - and then start populating tmpdir and do whatever you want to do.
I will review the changes needed for this before submitting v2 of my patch.
[1]: https://github.com/thenigan/git-diffall/pull/9
[2]: https://github.com/thenigan/git-diffall/pull/2#issuecomment-498472
^ permalink raw reply
* Re: [PATCH 4/4] Only re-encode certain parts in commit object, not the whole
From: Nguyen Thai Ngoc Duy @ 2012-02-22 2:01 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20120221182559.GB32668@sigill.intra.peff.net>
2012/2/22 Jeff King <peff@peff.net>:
> On Tue, Feb 21, 2012 at 09:24:52PM +0700, Nguyen Thai Ngoc Duy wrote:
>
>> Commit object has its own format, which happens to be in ascii, but
>> not really subject to re-encoding.
>>
>> There are only four areas that may be re-encoded: author line,
>> committer line, mergetag lines and commit body. Encoding of tags
>> embedded in mergetag lines is not decided by commit encoding, so leave
>> it out and consider it binary.
>
> Is this worth the effort? Yes, re-encoding the ASCII bits of the commit
> object is unnecessary. But do we actually handle encodings that are not
> ASCII supersets? IOW, I could see the point if this is making it
> possible to hold utf-16 names and messages in your commits (though why
> you would want to do so is beyond me...). But my understanding is that
> this is horribly broken anyway by other parts of the code. And even
> looking at your code below:
No, utf-16 and friends are out of question. 617/1168 supported
encodings in iconv translate chars 10,32-126 to something else, some
of them does not generate NUL. I suppose none of these are actually
used nowadays. Looking again, some don't even successfully translate
the given input. No, it's probably not worth the effort.
--
Duy
^ permalink raw reply
* Ambiguous reference weirdness
From: Phil Hord @ 2012-02-22 1:46 UTC (permalink / raw)
To: git, Ramkumar Ramachandra, Junio C Hamano, Jonathan Nieder
I accidentally ran into this today:
$ git cherry-pick 1147
fatal: BUG: expected exactly one commit from walk
git log shows no output:
$ git log 1147
Neither of these are very helpful or reassuring. I tried a few things
but I haven't looked the code yet. I found lots of inconsistencies
along the way.
$ git clone https://github.com/git/git
$ cd git
$git log 114
fatal: ambiguous argument '114': unknown revision or path not in the
working tree.
Use '--' to separate paths from revisions
$ git checkout 114
error: pathspec '114' did not match any file(s) known to git.
$ git merge 114
fatal: '114' does not point to a commit
$ git cherry-pick 114
fatal: ambiguous argument '114': unknown revision or path not in the
working tree.
Use '--' to separate paths from revisions
$ git checkout 1147
fatal: reference is not a tree: 1147
$ git merge 1147
error: 1147: expected commit type, but the object dereferences to blob type
fatal: '1147' does not point to a commit
$ git cherry-pick 1147
fatal: BUG: expected exactly one commit from walk
$ git log 1147
$ git checkout 1146
error: short SHA1 1146 is ambiguous.
error: pathspec '1146' did not match any file(s) known to git.
$ git merge 1146
error: short SHA1 1146 is ambiguous.
fatal: '1146' does not point to a commit
$ git cherry-pick 1146
error: short SHA1 1146 is ambiguous.
error: short SHA1 1146 is ambiguous.
fatal: ambiguous argument '1146': unknown revision or path not in the
working tree.
Use '--' to separate paths from revisions
$ git log 1146
error: short SHA1 1146 is ambiguous.
error: short SHA1 1146 is ambiguous.
fatal: ambiguous argument '1146': unknown revision or path not in the
working tree.
Use '--' to separate paths from revisions
I can understand some of the inconsistent error reporting (checkout
may expect filenames, but cherry-pick and merge do not). But this
seems too varied to me.
And the first two look like bugs.
Any comments or suggestions?
Phil
^ permalink raw reply
* Re: [RFC/PATCH 0/3] push: add 'prune' option
From: Nguyen Thai Ngoc Duy @ 2012-02-22 1:45 UTC (permalink / raw)
To: Jeff King; +Cc: Felipe Contreras, git, Junio C Hamano
In-Reply-To: <20120221173548.GB32376@sigill.intra.peff.net>
On Wed, Feb 22, 2012 at 12:35 AM, Jeff King <peff@peff.net> wrote:
> Huh? Don't we already have "fetch --prune"?
We do indeed. I went to git-fetch.txt, searched for prune but missed
many "include" directives in there.
--
Duy
^ permalink raw reply
* [PATCHv3 5/5] for-each-ref --exclude option
From: Tom Grennan @ 2012-02-22 1:28 UTC (permalink / raw)
To: git; +Cc: gitster, peff, jasampler, pclouds
In-Reply-To: <20120211190856.GB4903@tgrennan-laptop>
Example,
$ git for-each-ref --format="%(refname)" refs/remotes/origin
refs/remotes/origin/HEAD
refs/remotes/origin/maint
refs/remotes/origin/master
refs/remotes/origin/next
refs/remotes/origin/pu
refs/remotes/origin/todo
$ ./git-for-each-ref --format="%(refname)" --exclude "*/HEAD" refs/remotes/origin
refs/remotes/origin/maint
refs/remotes/origin/master
refs/remotes/origin/next
refs/remotes/origin/pu
refs/remotes/origin/todo
Signed-off-by: Tom Grennan <tmgrennan@gmail.com>
---
Documentation/git-for-each-ref.txt | 7 ++++++-
builtin/for-each-ref.c | 8 +++++++-
t/t6300-for-each-ref.sh | 11 +++++++++++
3 files changed, 24 insertions(+), 2 deletions(-)
diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index c872b88..5f19a8b 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -9,7 +9,8 @@ SYNOPSIS
--------
[verse]
'git for-each-ref' [--count=<count>] [--shell|--perl|--python|--tcl]
- [(--sort=<key>)...] [--format=<format>] [<pattern>...]
+ [(--sort=<key>)...] [--format=<format>]
+ [--exclude=<pattern>] [<pattern>...]
DESCRIPTION
-----------
@@ -47,6 +48,10 @@ OPTIONS
`xx`; for example `%00` interpolates to `\0` (NUL),
`%09` to `\t` (TAB) and `%0a` to `\n` (LF).
+--exclude <pattern>::
+ Ignore refs matching the given pattern. This has precedence
+ over any other pattern match arguments.
+
<pattern>...::
If one or more patterns are given, only refs are shown that
match against at least one pattern, either using fnmatch(3) or
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index bd6a114..783f59f 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -91,6 +91,9 @@ static const char **used_atom;
static cmp_type *used_atom_type;
static int used_atom_cnt, sort_atom_limit, need_tagged, need_symref;
+/* list of ref patterns and ref groups (i.e. foo/) to ignore */
+static struct string_list exclude = STRING_LIST_INIT_NODUP;
+
/*
* Used to parse format string and sort specifiers
*/
@@ -781,7 +784,7 @@ static int grab_single_ref(const char *refname, const unsigned char *sha1, int f
struct refinfo *ref;
int cnt;
- if (!match_pattern(refname, cb->grab_pattern, NULL, FNM_PATHNAME))
+ if (!match_pattern(refname, cb->grab_pattern, &exclude, FNM_PATHNAME))
return 0;
/*
@@ -985,6 +988,9 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
OPT_STRING( 0 , "format", &format, "format", "format to use for the output"),
OPT_CALLBACK(0 , "sort", sort_tail, "key",
"field name to sort on", &opt_parse_sort),
+ OPT_CALLBACK(0, "exclude", &exclude, "pattern",
+ "ignore pattern matching refs",
+ parse_opt_string_list),
OPT_END(),
};
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 1721784..26df442 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -243,6 +243,17 @@ test_expect_success 'Verify descending sort' '
'
cat >expected <<\EOF
+refs/tags/testtag
+refs/heads/master
+EOF
+
+test_expect_success 'Verify exclusion with sort' '
+ git for-each-ref --format="%(refname)" --sort=-refname \
+ --exclude "*origin*" >actual &&
+ test_cmp expected actual
+'
+
+cat >expected <<\EOF
'refs/heads/master'
'refs/remotes/origin/master'
'refs/tags/testtag'
--
1.7.8
^ permalink raw reply related
* [PATCHv3 2/5] tag --points-at option wrapper
From: Tom Grennan @ 2012-02-22 1:28 UTC (permalink / raw)
To: git; +Cc: gitster, peff, jasampler, pclouds
In-Reply-To: <20120211190856.GB4903@tgrennan-laptop>
Use the OPT_CALLBACK wrapper with "--points-at" instead of an
OPTION_CALLBACK block.
Signed-off-by: Tom Grennan <tmgrennan@gmail.com>
---
builtin/tag.c | 7 +++----
1 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/builtin/tag.c b/builtin/tag.c
index 9dcd7d2..4a016d5 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -456,10 +456,9 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
PARSE_OPT_LASTARG_DEFAULT,
parse_opt_with_commit, (intptr_t)"HEAD",
},
- {
- OPTION_CALLBACK, 0, "points-at", NULL, "object",
- "print only tags of the object", 0, parse_opt_points_at
- },
+ OPT_CALLBACK(0, "points-at", NULL, "object",
+ "print only tags of the object",
+ parse_opt_points_at),
OPT_END()
};
--
1.7.8
^ permalink raw reply related
* [PATCHv3 4/5] branch --exclude option
From: Tom Grennan @ 2012-02-22 1:28 UTC (permalink / raw)
To: git; +Cc: gitster, peff, jasampler, pclouds
In-Reply-To: <20120211190856.GB4903@tgrennan-laptop>
Example,
$ ./git-branch -r --exclude \*HEAD
origin/maint
origin/master
origin/next
origin/pu
origin/todo
Signed-off-by: Tom Grennan <tmgrennan@gmail.com>
---
Documentation/git-branch.txt | 7 ++++++-
builtin/branch.c | 18 +++++++++++++-----
t/t3200-branch.sh | 23 +++++++++++++++++++++++
3 files changed, 42 insertions(+), 6 deletions(-)
diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index 0427e80..ef08872 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -10,7 +10,8 @@ SYNOPSIS
[verse]
'git branch' [--color[=<when>] | --no-color] [-r | -a]
[--list] [-v [--abbrev=<length> | --no-abbrev]]
- [(--merged | --no-merged | --contains) [<commit>]] [<pattern>...]
+ [(--merged | --no-merged | --contains) [<commit>]]
+ [--exclude <pattern>] [<pattern>...]
'git branch' [--set-upstream | --track | --no-track] [-l] [-f] <branchname> [<start-point>]
'git branch' (-m | -M) [<oldbranch>] <newbranch>
'git branch' (-d | -D) [-r] <branchname>...
@@ -166,6 +167,10 @@ start-point is either a local or remote-tracking branch.
--contains <commit>::
Only list branches which contain the specified commit.
+--exclude <pattern>::
+ Don't list branches matching the given pattern. This has
+ precedence over other match pattern arguments.
+
--merged [<commit>]::
Only list branches whose tips are reachable from the
specified commit (HEAD if not specified).
diff --git a/builtin/branch.c b/builtin/branch.c
index e46ed58..ec06f66 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -243,6 +243,7 @@ struct ref_list {
int index, alloc, maxwidth, verbose, abbrev;
struct ref_item *list;
struct commit_list *with_commit;
+ struct string_list *exclude;
int kinds;
};
@@ -300,7 +301,7 @@ static int append_ref(const char *refname, const unsigned char *sha1, int flags,
if ((kind & ref_list->kinds) == 0)
return 0;
- if (!match_pattern(refname, cb->pattern, NULL, 0))
+ if (!match_pattern(refname, cb->pattern, ref_list->exclude, 0))
return 0;
commit = NULL;
@@ -498,7 +499,10 @@ static void show_detached(struct ref_list *ref_list)
}
}
-static int print_ref_list(int kinds, int detached, int verbose, int abbrev, struct commit_list *with_commit, const char **pattern)
+static int print_ref_list(int kinds, int detached, int verbose, int abbrev,
+ struct commit_list *with_commit,
+ struct string_list *exclude,
+ const char **pattern)
{
int i;
struct append_ref_cb cb;
@@ -509,6 +513,7 @@ static int print_ref_list(int kinds, int detached, int verbose, int abbrev, stru
ref_list.verbose = verbose;
ref_list.abbrev = abbrev;
ref_list.with_commit = with_commit;
+ ref_list.exclude = exclude;
if (merge_filter != NO_FILTER)
init_revisions(&ref_list.revs, NULL);
cb.ref_list = &ref_list;
@@ -530,7 +535,7 @@ static int print_ref_list(int kinds, int detached, int verbose, int abbrev, stru
qsort(ref_list.list, ref_list.index, sizeof(struct ref_item), ref_cmp);
detached = (detached && (kinds & REF_LOCAL_BRANCH));
- if (detached && match_pattern("HEAD", pattern, NULL, 0))
+ if (detached && match_pattern("HEAD", pattern, exclude, 0))
show_detached(&ref_list);
for (i = 0; i < ref_list.index; i++) {
@@ -665,7 +670,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
enum branch_track track;
int kinds = REF_LOCAL_BRANCH;
struct commit_list *with_commit = NULL;
-
+ struct string_list exclude = STRING_LIST_INIT_NODUP;
struct option options[] = {
OPT_GROUP("Generic options"),
OPT__VERBOSE(&verbose,
@@ -689,6 +694,9 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
PARSE_OPT_HIDDEN | PARSE_OPT_LASTARG_DEFAULT,
parse_opt_with_commit, (intptr_t) "HEAD",
},
+ OPT_CALLBACK(0, "exclude", &exclude, "pattern",
+ "ignorepattern matching branches",
+ parse_opt_string_list),
OPT__ABBREV(&abbrev),
OPT_GROUP("Specific git-branch actions:"),
@@ -753,7 +761,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
return delete_branches(argc, argv, delete > 1, kinds);
else if (list)
return print_ref_list(kinds, detached, verbose, abbrev,
- with_commit, argv);
+ with_commit, &exclude, argv);
else if (edit_description) {
const char *branch_name;
struct strbuf branch_ref = STRBUF_INIT;
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index dd1aceb..8144bc8 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -160,6 +160,29 @@ test_expect_success 'git branch --list -d t should fail' '
test_path_is_missing .git/refs/heads/t
'
+>expect
+test_expect_success \
+ 'git branch --list --exclude "t*" "t*" should be empty' '
+ git branch ta &&
+ git branch tb &&
+ git branch --list --exclude "t*" "t*" > actual &&
+ cmp expect actual
+'
+
+cat >expect <<EOF
+ ta
+EOF
+test_expect_success \
+ 'git branch --list --exclude "tb" "t*" should be "ta"' '
+ git branch --list --exclude "tb" "t*" > actual &&
+ cmp expect actual
+'
+
+test_expect_success \
+ 'git branch -d ta tb should succeed' '
+ git branch -d ta tb
+'
+
mv .git/config .git/config-saved
test_expect_success 'git branch -m q q2 without config should succeed' '
--
1.7.8
^ permalink raw reply related
* [PATCHv3 3/5] tag --exclude option
From: Tom Grennan @ 2012-02-22 1:28 UTC (permalink / raw)
To: git; +Cc: gitster, peff, jasampler, pclouds
In-Reply-To: <20120211190856.GB4903@tgrennan-laptop>
Example,
$ git tag -l --exclude "*-rc?" "v1.7.8*"
v1.7.8
v1.7.8.1
v1.7.8.2
v1.7.8.3
v1.7.8.4
Which is equivalent to,
$ git tag -l "v1.7.8*" | grep -v \\-rc.
v1.7.8
v1.7.8.1
v1.7.8.2
v1.7.8.3
v1.7.8.4
Signed-off-by: Tom Grennan <tmgrennan@gmail.com>
---
Documentation/git-tag.txt | 6 ++++-
builtin/tag.c | 17 ++++++++++---
t/t7004-tag.sh | 56 +++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 74 insertions(+), 5 deletions(-)
diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 8d32b9a..470bd80 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -13,7 +13,7 @@ SYNOPSIS
<tagname> [<commit> | <object>]
'git tag' -d <tagname>...
'git tag' [-n[<num>]] -l [--contains <commit>] [--points-at <object>]
- [<pattern>...]
+ [--exclude <pattern>] [<pattern>...]
'git tag' -v <tagname>...
DESCRIPTION
@@ -90,6 +90,10 @@ OPTIONS
--points-at <object>::
Only list tags of the given object.
+--exclude <pattern>::
+ Don't list tags matching the given pattern. This has precedence
+ over any other match pattern arguments.
+
-m <msg>::
--message=<msg>::
Use the given tag message (instead of prompting).
diff --git a/builtin/tag.c b/builtin/tag.c
index 4a016d5..547f97d 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -21,7 +21,7 @@ static const char * const git_tag_usage[] = {
"git tag [-a|-s|-u <key-id>] [-f] [-m <msg>|-F <file>] <tagname> [<head>]",
"git tag -d <tagname>...",
"git tag -l [-n[<num>]] [--contains <commit>] [--points-at <object>] "
- "\n\t\t[<pattern>...]",
+ "\n\t\t[--exclude <pattern>] [<pattern>...]",
"git tag -v <tagname>...",
NULL
};
@@ -30,6 +30,7 @@ struct tag_filter {
const char **patterns;
int lines;
struct commit_list *with_commit;
+ struct string_list *exclude;
};
static struct sha1_array points_at;
@@ -138,7 +139,7 @@ static int show_reference(const char *refname, const unsigned char *sha1,
{
struct tag_filter *filter = cb_data;
- if (match_pattern(refname, filter->patterns, NULL, 0)) {
+ if (match_pattern(refname, filter->patterns, filter->exclude, 0)) {
if (filter->with_commit) {
struct commit *commit;
@@ -165,13 +166,15 @@ static int show_reference(const char *refname, const unsigned char *sha1,
}
static int list_tags(const char **patterns, int lines,
- struct commit_list *with_commit)
+ struct commit_list *with_commit,
+ struct string_list *exclude)
{
struct tag_filter filter;
filter.patterns = patterns;
filter.lines = lines;
filter.with_commit = with_commit;
+ filter.exclude = exclude;
for_each_tag_ref(show_reference, (void *) &filter);
@@ -428,6 +431,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
const char *msgfile = NULL, *keyid = NULL;
struct msg_arg msg = { 0, STRBUF_INIT };
struct commit_list *with_commit = NULL;
+ struct string_list exclude = STRING_LIST_INIT_NODUP;
struct option options[] = {
OPT_BOOLEAN('l', "list", &list, "list tag names"),
{ OPTION_INTEGER, 'n', NULL, &lines, "n",
@@ -459,6 +463,9 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
OPT_CALLBACK(0, "points-at", NULL, "object",
"print only tags of the object",
parse_opt_points_at),
+ OPT_CALLBACK(0, "exclude", &exclude, "pattern",
+ "ignore pattern matching tags",
+ parse_opt_string_list),
OPT_END()
};
@@ -485,13 +492,15 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
usage_with_options(git_tag_usage, options);
if (list)
return list_tags(argv, lines == -1 ? 0 : lines,
- with_commit);
+ with_commit, &exclude);
if (lines != -1)
die(_("-n option is only allowed with -l."));
if (with_commit)
die(_("--contains option is only allowed with -l."));
if (points_at.nr)
die(_("--points-at option is only allowed with -l."));
+ if (exclude.nr)
+ die(_("--exclude option is only allowed with -l."));
if (delete)
return for_each_tag_name(argv, delete_tag);
if (verify)
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index f8c247a..4f1cf48 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -82,6 +82,10 @@ test_expect_success \
'listing tags using a non-matching pattern should output nothing' \
'test `git tag -l xxx | wc -l` -eq 0'
+test_expect_success \
+ 'listing tags excluding "mytag" should output nothing' \
+ 'test `git tag -l --exclude mytag | wc -l` -eq 0'
+
# special cases for creating tags:
test_expect_success \
@@ -202,6 +206,15 @@ test_expect_success \
'
cat >expect <<EOF
+v0.2.1
+EOF
+test_expect_success \
+ 'listing tags with a suffix as pattern and prefix exclusion' '
+ git tag -l --exclude "v1.*" "*.1" > actual &&
+ test_cmp expect actual
+'
+
+cat >expect <<EOF
t210
t211
EOF
@@ -212,6 +225,15 @@ test_expect_success \
'
cat >expect <<EOF
+t210
+EOF
+test_expect_success \
+ 'listing tags with a prefix as pattern and suffix exclusion' '
+ git tag -l --exclude "*1" "t21*" > actual &&
+ test_cmp expect actual
+'
+
+cat >expect <<EOF
a1
EOF
test_expect_success \
@@ -239,6 +261,15 @@ test_expect_success \
test_cmp expect actual
'
+cat >expect <<EOF
+v1.0.1
+EOF
+test_expect_success \
+ 'listing tags with ? in the pattern and exclusion' '
+ git tag -l --exclude "v1.?.3" "v1.?.?" > actual &&
+ test_cmp expect actual
+'
+
>expect
test_expect_success \
'listing tags using v.* should print nothing because none have v.' '
@@ -263,6 +294,31 @@ test_expect_success 'tag -l can accept multiple patterns' '
test_cmp expect actual
'
+test_expect_success 'tag -l can cancel exclusions' '
+ git tag -l --exclude "v*.3" --no-exclude "v1*" "v0*" >actual &&
+ test_cmp expect actual
+'
+
+cat >expect <<EOF
+v0.2.1
+v1.0.1
+EOF
+test_expect_success 'tag -l can accept multiple patterns and exclusions' '
+ git tag -l --exclude "v*.3" --exclude "v1.0" "v1*" "v0*" >actual &&
+ test_cmp expect actual
+'
+
+cat >expect <<EOF
+v0.2.1
+v1.0.1
+v1.1.3
+EOF
+test_expect_success \
+ 'tag -l can cancel then reapply exclusions' '
+ git tag -l --exclude "v*.3" --no-exclude --exclude "v1.0" \
+ "v1*" "v0*" >actual &&
+ test_cmp expect actual
+'
# creating and verifying lightweight tags:
test_expect_success \
--
1.7.8
^ permalink raw reply related
* [PATCHv3 1/5] refs: add match_pattern()
From: Tom Grennan @ 2012-02-22 1:28 UTC (permalink / raw)
To: git; +Cc: gitster, peff, jasampler, pclouds
In-Reply-To: <20120211190856.GB4903@tgrennan-laptop>
Used-by: git-branch, git-for-each-ref, git-ls-remote, and git-tag
Signed-off-by: Tom Grennan <tmgrennan@gmail.com>
---
builtin/branch.c | 16 ++--------------
builtin/for-each-ref.c | 21 ++-------------------
builtin/ls-remote.c | 12 +++---------
builtin/tag.c | 13 +------------
refs.c | 36 ++++++++++++++++++++++++++++++++++++
refs.h | 12 ++++++++++++
6 files changed, 56 insertions(+), 54 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index cb17bc3..e46ed58 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -266,18 +266,6 @@ struct append_ref_cb {
int ret;
};
-static int match_patterns(const char **pattern, const char *refname)
-{
- if (!*pattern)
- return 1; /* no pattern always matches */
- while (*pattern) {
- if (!fnmatch(*pattern, refname, 0))
- return 1;
- pattern++;
- }
- return 0;
-}
-
static int append_ref(const char *refname, const unsigned char *sha1, int flags, void *cb_data)
{
struct append_ref_cb *cb = (struct append_ref_cb *)(cb_data);
@@ -312,7 +300,7 @@ static int append_ref(const char *refname, const unsigned char *sha1, int flags,
if ((kind & ref_list->kinds) == 0)
return 0;
- if (!match_patterns(cb->pattern, refname))
+ if (!match_pattern(refname, cb->pattern, NULL, 0))
return 0;
commit = NULL;
@@ -542,7 +530,7 @@ static int print_ref_list(int kinds, int detached, int verbose, int abbrev, stru
qsort(ref_list.list, ref_list.index, sizeof(struct ref_item), ref_cmp);
detached = (detached && (kinds & REF_LOCAL_BRANCH));
- if (detached && match_patterns(pattern, "HEAD"))
+ if (detached && match_pattern("HEAD", pattern, NULL, 0))
show_detached(&ref_list);
for (i = 0; i < ref_list.index; i++) {
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index b01d76a..bd6a114 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -781,25 +781,8 @@ static int grab_single_ref(const char *refname, const unsigned char *sha1, int f
struct refinfo *ref;
int cnt;
- if (*cb->grab_pattern) {
- const char **pattern;
- int namelen = strlen(refname);
- for (pattern = cb->grab_pattern; *pattern; pattern++) {
- const char *p = *pattern;
- int plen = strlen(p);
-
- if ((plen <= namelen) &&
- !strncmp(refname, p, plen) &&
- (refname[plen] == '\0' ||
- refname[plen] == '/' ||
- p[plen-1] == '/'))
- break;
- if (!fnmatch(p, refname, FNM_PATHNAME))
- break;
- }
- if (!*pattern)
- return 0;
- }
+ if (!match_pattern(refname, cb->grab_pattern, NULL, FNM_PATHNAME))
+ return 0;
/*
* We do not open the object yet; sort may only need refname
diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
index 41c88a9..29f2b38 100644
--- a/builtin/ls-remote.c
+++ b/builtin/ls-remote.c
@@ -2,6 +2,7 @@
#include "cache.h"
#include "transport.h"
#include "remote.h"
+#include "refs.h"
static const char ls_remote_usage[] =
"git ls-remote [--heads] [--tags] [-u <exec> | --upload-pack <exec>]\n"
@@ -13,19 +14,12 @@ static const char ls_remote_usage[] =
*/
static int tail_match(const char **pattern, const char *path)
{
- const char *p;
char pathbuf[PATH_MAX];
- if (!pattern)
- return 1; /* no restriction */
-
if (snprintf(pathbuf, sizeof(pathbuf), "/%s", path) > sizeof(pathbuf))
return error("insanely long ref %.*s...", 20, path);
- while ((p = *(pattern++)) != NULL) {
- if (!fnmatch(p, pathbuf, 0))
- return 1;
- }
- return 0;
+
+ return match_pattern(pathbuf, pattern, NULL, 0);
}
int cmd_ls_remote(int argc, const char **argv, const char *prefix)
diff --git a/builtin/tag.c b/builtin/tag.c
index fe7e5e5..9dcd7d2 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -34,17 +34,6 @@ struct tag_filter {
static struct sha1_array points_at;
-static int match_pattern(const char **patterns, const char *ref)
-{
- /* no pattern means match everything */
- if (!*patterns)
- return 1;
- for (; *patterns; patterns++)
- if (!fnmatch(*patterns, ref, 0))
- return 1;
- return 0;
-}
-
static const unsigned char *match_points_at(const char *refname,
const unsigned char *sha1)
{
@@ -149,7 +138,7 @@ static int show_reference(const char *refname, const unsigned char *sha1,
{
struct tag_filter *filter = cb_data;
- if (match_pattern(filter->patterns, refname)) {
+ if (match_pattern(refname, filter->patterns, NULL, 0)) {
if (filter->with_commit) {
struct commit *commit;
diff --git a/refs.c b/refs.c
index c9f6835..0c50e81 100644
--- a/refs.c
+++ b/refs.c
@@ -3,6 +3,7 @@
#include "object.h"
#include "tag.h"
#include "dir.h"
+#include "string-list.h"
/* ISSYMREF=0x01, ISPACKED=0x02 and ISBROKEN=0x04 are public interfaces */
#define REF_KNOWS_PEELED 0x10
@@ -2127,3 +2128,38 @@ char *shorten_unambiguous_ref(const char *refname, int strict)
free(short_name);
return xstrdup(refname);
}
+
+static int match_path(const char *name, const char *pattern, int nlen)
+{
+ int plen = strlen(pattern);
+
+ return ((plen <= nlen) &&
+ !strncmp(name, pattern, plen) &&
+ (name[plen] == '\0' ||
+ name[plen] == '/' ||
+ pattern[plen-1] == '/'));
+}
+
+int match_pattern(const char *name, const char **match,
+ struct string_list *exclude, int flags)
+{
+ int nlen = strlen(name);
+
+ if (exclude) {
+ struct string_list_item *x;
+ for_each_string_list_item(x, exclude) {
+ if (!fnmatch(x->string, name, 0))
+ return 0;
+ }
+ }
+ if (!match || !*match)
+ return 1;
+ for (; *match; match++) {
+ if (flags == FNM_PATHNAME)
+ if (match_path(name, *match, nlen))
+ return 1;
+ if (!fnmatch(*match, name, flags))
+ return 1;
+ }
+ return 0;
+}
diff --git a/refs.h b/refs.h
index 33202b0..dd059d8 100644
--- a/refs.h
+++ b/refs.h
@@ -144,4 +144,16 @@ int update_ref(const char *action, const char *refname,
const unsigned char *sha1, const unsigned char *oldval,
int flags, enum action_on_err onerr);
+/*
+ * match_pattern() - compares a name with pattern match and ignore lists
+ * This returns in highest to lowest precedence:
+ * 0 if <name> fnmatch() an <exclude> pattern
+ * 1 if <match> is NULL or empty
+ * 1 if <flags> is FNM_PATHNAME and <name> is an exact match of a listed
+ * pattern upto and including a trailing '/'
+ * 1 <name> fnmatch() a <match> pattern
+ * 0 otherwise
+ */
+int match_pattern(const char *name, const char **match, struct string_list *exclude, int flags);
+
#endif /* REFS_H */
--
1.7.8
^ permalink raw reply related
* [PATCHv3 0/5] Re: tag: make list exclude !<pattern>
From: Tom Grennan @ 2012-02-22 1:28 UTC (permalink / raw)
To: git; +Cc: gitster, peff, jasampler, pclouds
In-Reply-To: <20120211190856.GB4903@tgrennan-laptop>
It has taken me a while to get back to this; the following series rebase
and rework my proposed "!<pattern>" feature. The first patch extracts
match_pattern() from builtin/tag.c and updates this to replace similar
functions in builtin/{branch,for-each-ref,ls-remote}.c
The second patch uses the OPT_CALLBACK wrapper on the recently added
"--points-at" option of git-tag. I should have implemented it this way
to begin with but I missed Jeff King's suggestion within that thread.
The remaining patches add "--exclude <pattern>" options (vs.
"!<pattern>") to: git-tag, git-branch, and git-for-each-ref.
For example,
$ git tag -l --exclude "*-rc?" "v1.7.8*"
$ git branch -r --exclude \*HEAD
$ git for-each-ref --format="%(refname)" --exclude "*HEAD"
refs/remotes/origin
Instead of,
$ git tag -l "!*-rc?" "v1.7.8*"
$ git branch -r "!*HEAD"
$ git for-each-ref --format="%(refname)" "!*HEAD" refs/remotes/origin
Note that I haven't yet added an "--exclude" feature to git-ls-remote
because I think that I should first update its option parsing.
Thanks,
Tom Grennan (5):
refs: add match_pattern()
tag --points-at option wrapper
tag --exclude option
branch --exclude option
for-each-ref --exclude option
Documentation/git-branch.txt | 7 ++++-
Documentation/git-for-each-ref.txt | 7 ++++-
Documentation/git-tag.txt | 6 +++-
builtin/branch.c | 30 ++++++++-----------
builtin/for-each-ref.c | 27 +++++------------
builtin/ls-remote.c | 12 ++------
builtin/tag.c | 35 ++++++++++------------
refs.c | 36 +++++++++++++++++++++++
refs.h | 12 ++++++++
t/t3200-branch.sh | 23 +++++++++++++++
t/t6300-for-each-ref.sh | 11 +++++++
t/t7004-tag.sh | 56 ++++++++++++++++++++++++++++++++++++
12 files changed, 195 insertions(+), 67 deletions(-)
--
1.7.8
^ permalink raw reply
* Re: git-p4 uses -h for host instead of -H
From: Pete Wyckoff @ 2012-02-22 1:23 UTC (permalink / raw)
To: Russell Myers; +Cc: git, luke, ggibbons
In-Reply-To: <CAA5tD2sYSqtGTwW1PmFMB_mP_xG24VS6hPXTLD33bJsMaj4MWg@mail.gmail.com>
mezner@russellmyers.com wrote on Tue, 21 Feb 2012 19:35 -0500:
> In attempting to use the git-p4 plugin I ran into an issue when the host
> argument was specified using the -h argument instead of the -H argument for
> the host. As a result, I found that instead of git-p4 specifying a host, it
> gets help information. Correcting this issue allowed me to clone without
> issue. I've attached a patch of what I did to fix the issue if this is
> indeed believed to be an issue.
Ack to this. Looks like the bug has been in there since the
feature of git-p4.host was introduced in abcaf07 (If the user has
configured various parameters, use them., 2008-08-10).
Thanks for the fix.
-- Pete
> From 2f6c91282c98ca0a45269524ec74655f76921ec9 Mon Sep 17 00:00:00 2001
> From: Russell Myers <mezner@russellmyers.com>
> Date: Tue, 21 Feb 2012 19:18:54 -0500
> Subject: [PATCH] Changing host argument to -H from -h. Based on
> http://www.perforce.com/perforce/doc.current/manuals/p4guide/03_using.html
> '-H' is the apporpriate flag while '-h' is a flag passed
> for help content.
>
> ---
> contrib/fast-import/git-p4 | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
> index a78d9c5..d2fd265 100755
> --- a/contrib/fast-import/git-p4
> +++ b/contrib/fast-import/git-p4
> @@ -38,7 +38,7 @@ def p4_build_cmd(cmd):
>
> host = gitConfig("git-p4.host")
> if len(host) > 0:
> - real_cmd += ["-h", host]
> + real_cmd += ["-H", host]
>
> client = gitConfig("git-p4.client")
> if len(client) > 0:
> --
> 1.7.5.4
>
^ permalink raw reply
* Re: [PATCH v2] cherry-pick: No advice to commit if --no-commit
From: Jonathan Nieder @ 2012-02-22 0:51 UTC (permalink / raw)
To: Phil Hord; +Cc: Git List, Junio C Hamano, Ramkumar Ramachandra, Phil Hord
In-Reply-To: <1329871457-12890-1-git-send-email-hordp@cisco.com>
Phil Hord wrote:
> In case of cherry-pick --no-commit, the
> hint goes too far. It tells the user to finish up with
> 'git commit'. That is not what this git-cherry-pick was
> trying to do in the first place.
Especially since if I do try to commit as it says, it will not
reuse the old commit message and timestamp like the advice made me
suspect it would. :)
[...]
> --- a/t/t3507-cherry-pick-conflict.sh
> +++ b/t/t3507-cherry-pick-conflict.sh
> @@ -59,6 +59,20 @@ test_expect_success 'advice from failed cherry-pick' "
> test_i18ncmp expected actual
> "
>
> +test_expect_success 'advice from failed cherry-pick --no-commit' "
> + pristine_detach initial &&
> +
> + picked=\$(git rev-parse --short picked) &&
The escaping here is obnoxiously tricky. Not your fault, though.
For what it's worth,
Acked-by: Jonathan Nieder <jrnieder@gmail.com>
Thanks.
^ permalink raw reply
* git-p4 uses -h for host instead of -H
From: Russell Myers @ 2012-02-22 0:49 UTC (permalink / raw)
To: git
In-Reply-To: <CAA5tD2sYSqtGTwW1PmFMB_mP_xG24VS6hPXTLD33bJsMaj4MWg@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 425 bytes --]
In attempting to use the git-p4 plugin I ran into an issue when the host
argument was specified using the -h argument instead of the -H argument
for the host. As a result, I found that instead of git-p4 specifying a
host, it gets help information. Correcting this issue allowed me to
clone without issue. I've attached a patch of what I did to fix the
issue if this is indeed believed to be an issue.
Thanks,
Russell
[-- Attachment #2: host_flag_fix.patch --]
[-- Type: text/x-patch, Size: 884 bytes --]
>From 2f6c91282c98ca0a45269524ec74655f76921ec9 Mon Sep 17 00:00:00 2001
From: Russell Myers <mezner@russellmyers.com>
Date: Tue, 21 Feb 2012 19:18:54 -0500
Subject: [PATCH] Changing host argument to -H from -h. Based on
http://www.perforce.com/perforce/doc.current/manuals/p4guide/03_using.html
'-H' is the apporpriate flag while '-h' is a flag passed
for help content.
---
contrib/fast-import/git-p4 | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index a78d9c5..d2fd265 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -38,7 +38,7 @@ def p4_build_cmd(cmd):
host = gitConfig("git-p4.host")
if len(host) > 0:
- real_cmd += ["-h", host]
+ real_cmd += ["-H", host]
client = gitConfig("git-p4.client")
if len(client) > 0:
--
1.7.5.4
^ permalink raw reply related
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