* 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: [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: Ambiguous reference weirdness
From: Jeff King @ 2012-02-22 7:00 UTC (permalink / raw)
To: Phil Hord; +Cc: git, Ramkumar Ramachandra, Junio C Hamano, Jonathan Nieder
In-Reply-To: <CABURp0oAw7cvU7cwCZOtvqZ_oa0hDPsE_0Lm3kR1ctdNuxU3hg@mail.gmail.com>
On Tue, Feb 21, 2012 at 08:46:24PM -0500, Phil Hord wrote:
> 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
What is 1147? Is it supposed to be a partial sha1, or is it a ref you
have?
Have you looked at the object that it resolves to? I suspect it is the
partial sha1 of a non-commit object. E.g.:
$ git cat-file -t HEAD^{tree}
tree
$ git cherry-pick HEAD^{tree}
fatal: BUG: expected exactly one commit from walk
$ git log HEAD^{tree} | wc -l
0
Both cases have a similar source: they feed the arguments to the
revision walking machinery, which of course finds no actual revisions to
walk.
In the cherry-pick case, the code is checking the right thing, but the
message is horrible. It is not a bug, but merely unexpected input, and
it should provide a usage message.
In the log case, we totally ignore any pending non-revision arguments.
So it is correct to produce no output (there is nothing to show, which
is not unusual in itself; many queries end up producing empty output).
But we should probably notice that there are pending objects left over
and produce some kind of diagnostic.
I've reordered some of your example commands below to fit the flow of my
explanation better.
> $git log 114
> fatal: ambiguous argument '114': unknown revision or path not in the
> working tree.
> Use '--' to separate paths from revisions
Right. I think we require at least 4 characters in a partial sha1, so we
don't treat that as a possibility. So we are left guessing whether you
mean to do:
git log 114 --
or
git log HEAD -- 114
since it exists as neither a revision nor a path, and the error message
reflects that (the first one is an error, as there is no such revision.
The second is a correct query, though one that does not produce any
results).
> $ git checkout 114
> error: pathspec '114' did not match any file(s) known to git.
I think checkout has the same "is this a path or a revision" ambiguity
to resolve. But rather than be explicit that you might have meant "114"
as a tree, the error message assumes you meant a path. That might be
worth improving, similar to the above example.
Again, you can disambiguate with:
$ git checkout -- 1147
error: pathspec '1147' did not match any file(s) known to git.
$ git checkout 1147 --
fatal: reference is not a tree: 1147
> $ git checkout 1147
> fatal: reference is not a tree: 1147
In this case the name does resolve to an object, so we try to use it as
such (even though we later find that it is useless for the operation).
We _could_ realize that it is not a tree and disambiguate to:
$ git checkout -- 1147
but the current rule is at least consistent and simple.
> $ git checkout 1146
> error: short SHA1 1146 is ambiguous.
> error: pathspec '1146' did not match any file(s) known to git.
The sha1 is ambiguous, and therefore it does not resolve to anything. So
you get the same case as "git checkout 1147", but with the extra
ambiguity warning.
> $ git merge 114
> fatal: '114' does not point to a commit
It might be nice for this error message to be split into two cases:
1. the name does not resolve _at all_ (i.e., you made a typo)
2. the name does resolve to something, but it is not a commit
In the latter case, we actually do get an extra error message from
elsewhere in the code:
> $ git merge 1147
> error: 1147: expected commit type, but the object dereferences to blob type
> fatal: '1147' does not point to a commit
But in case 1, it's not clear which is which (maybe even rewording it as
"114 cannot be resolved as a commit" would be less confusing).
> $ git cherry-pick 114
> fatal: ambiguous argument '114': unknown revision or path not in the
> working tree.
> Use '--' to separate paths from revisions
> [...]
> $ git cherry-pick 1147
> fatal: BUG: expected exactly one commit from walk
This is the "does not resolve" versus "is not actually a commit". In the
first case, though, I wonder if the error message is accurate. I'm not
sure if you can do "git cherry-pick <rev> -- <paths>", so the error
message is misleading (if anything, I would expect it to limit the
revision walk, but trying "git cherry-pick HEAD -- 114" seems to still
complain about the absence of 114).
> [more examples]
These are all variants that hopefully make sense in light of the
explanations above.
> 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?
I think the outcomes are all working as intended, but the error messages
could stand to be improved.
-Peff
^ permalink raw reply
* Re: [RFC] pre-rebase: Refuse to rewrite commits that are reachable from upstream
From: Jeff King @ 2012-02-22 7:09 UTC (permalink / raw)
To: Dave Zarzycki; +Cc: Junio C Hamano, Johan Herland, git, jnareb, philipoakley
In-Reply-To: <1AD297DA-6E85-4808-94F8-907BA890E7F6@apple.com>
On Tue, Feb 21, 2012 at 06:59:38PM -0500, Dave Zarzycki wrote:
> > I think that question should be "warn before pushing out a commit that the
> > user may later regret to have pushed out" ;-)
>
> Why limit this proposal to just the commits that are reachable from
> upstream? What if somebody pulls from your repo?
>
> In other words, wouldn't it be better to have a git track "unshared"
> commits and only let those be rewritten? The theory being that if the
> given commits haven't been pushed or pulled anywhere, then they are
> safe to rewrite.
You don't necessarily know who has read from you. Depending on your
setup, the user running git code may not have write access to the
repository (e.g., Alice runs "git pull ~bob/project.git"). Where would
Alice write the list of commits she pulled so that when Bob later runs
git, he knows that she has pulled them?
There is also the issue of "dumb" transports in which no git code is
running on the remote repo at all (e.g., Alice fetches from Bob via dumb
http; Bob's server doesn't even have git at all).
There may be clever or complex ideas to tackle those problems, but I
suspect that handling push would cover most practical cases (e.g., in
the dumb http case, Bob's commits probably ended up on the server via
push). So perhaps it is a good place to start.
-Peff
^ permalink raw reply
* Re: how do you review auto-resolved files
From: Jeff King @ 2012-02-22 7:28 UTC (permalink / raw)
To: Neal Kreitzinger; +Cc: Junio C Hamano, Neal Kreitzinger, git
In-Reply-To: <4F442721.4080107@gmail.com>
On Tue, Feb 21, 2012 at 05:22:09PM -0600, Neal Kreitzinger wrote:
> My definition for "auto-resolve": "During a merge, the working tree
> files are updated to reflect the result of the merge... When both
> sides made changes to different areas of the same file, git picks
> both sides automatically, and leaves its up to you to make sure you
> review those merge results for correctness after git has made the
> merge commit."
Once the merge commit is made, you can review these with:
$ git show --raw
which will give you the list of paths that were touched on both sides,
and then you can examine them manually.
You can also use:
$ git show -c
to get the combined diff, showing hunks that were changed on both sides
(but only in files that would have been listed above). Annoyingly, I
don't think there is a way to get the same multi-way diff information
before the commit is created (i.e., when you still have some conflicts
in the index and working tree left to resolve).
But even both of those are not sufficient to find merge errors. Even
though there is no textual conflict, there may be semantic conflicts
that cross file boundaries (e.g., function foo() changes in foo.c, but a
caller in bar.c is introduced on a side branch). There is no replacement
for actually looking at the full result (though for the lazy, compiling
and running the test suite can often catch the low-hanging fruit).
-Peff
^ permalink raw reply
* Re: [RFC] pre-rebase: Refuse to rewrite commits that are reachable from upstream
From: Dave Zarzycki @ 2012-02-22 8:00 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Johan Herland, git, jnareb, philipoakley
In-Reply-To: <20120222070957.GB17015@sigill.intra.peff.net>
On Feb 22, 2012, at 2:09 AM, Jeff King <peff@peff.net> wrote:
> On Tue, Feb 21, 2012 at 06:59:38PM -0500, Dave Zarzycki wrote:
>
>>> I think that question should be "warn before pushing out a commit that the
>>> user may later regret to have pushed out" ;-)
>>
>> Why limit this proposal to just the commits that are reachable from
>> upstream? What if somebody pulls from your repo?
>>
>> In other words, wouldn't it be better to have a git track "unshared"
>> commits and only let those be rewritten? The theory being that if the
>> given commits haven't been pushed or pulled anywhere, then they are
>> safe to rewrite.
>
> You don't necessarily know who has read from you. Depending on your
> setup, the user running git code may not have write access to the
> repository (e.g., Alice runs "git pull ~bob/project.git"). Where would
> Alice write the list of commits she pulled so that when Bob later runs
> git, he knows that she has pulled them?
>
> There is also the issue of "dumb" transports in which no git code is
> running on the remote repo at all (e.g., Alice fetches from Bob via dumb
> http; Bob's server doesn't even have git at all).
>
> There may be clever or complex ideas to tackle those problems, but I
> suspect that handling push would cover most practical cases (e.g., in
> the dumb http case, Bob's commits probably ended up on the server via
> push). So perhaps it is a good place to start.
Fair points. Honestly, I was thinking more about a developer pulling changes between locations within his or her control, say a laptop and a desktop, or simply between multiple clones on the same machine. In this scenario, it would be useful to warn the developer that: "[some of] the commits you are about to rewrite, while not in the upstream repository, have been replicated into other repositories within your control. These other repositories will not be rewritten. Are you sure you want to continue?"
davez
^ permalink raw reply
* [PATCH v3] completion: remote set-* <name> and <branch>
From: Philip Jägenstedt @ 2012-02-22 8:58 UTC (permalink / raw)
To: git; +Cc: SZEDER Gábor, Felipe Contreras, Teemu Likonen
In-Reply-To: <20120222001737.GB2228@goldbirke>
Hope this works better. Is it possible to use format-patch or send-email
to get subjects like [PATCH 1/4 v3], as opposed to what I am sending?
^ permalink raw reply
* [PATCH 1/4] completion: remote set-* <name> and <branch>
From: Philip Jägenstedt @ 2012-02-22 8:58 UTC (permalink / raw)
To: git
Cc: SZEDER Gábor, Felipe Contreras, Teemu Likonen,
Philip Jägenstedt
In-Reply-To: <1329901093-24106-1-git-send-email-philip@foolip.org>
Complete <name> only for set-url. For set-branches and
set-head, complete <name> and <branch> over the network,
like e.g. git pull already does.
Signed-off-by: Philip Jägenstedt <philip@foolip.org>
---
contrib/completion/git-completion.bash | 12 +++++++++---
1 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index ab24310..c63a408 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -738,6 +738,9 @@ __git_complete_remote_or_refspec ()
{
local cur_="$cur" cmd="${words[1]}"
local i c=2 remote="" pfx="" lhs=1 no_complete_refspec=0
+ if [ "$cmd" = "remote" ]; then
+ c=$((++c))
+ fi
while [ $c -lt $cword ]; do
i="${words[c]}"
case "$i" in
@@ -788,7 +791,7 @@ __git_complete_remote_or_refspec ()
__gitcomp_nl "$(__git_refs)" "$pfx" "$cur_"
fi
;;
- pull)
+ pull|remote)
if [ $lhs = 1 ]; then
__gitcomp_nl "$(__git_refs "$remote")" "$pfx" "$cur_"
else
@@ -2289,7 +2292,7 @@ _git_config ()
_git_remote ()
{
- local subcommands="add rename rm show prune update set-head"
+ local subcommands="add rename rm set-head set-branches set-url show prune update"
local subcommand="$(__git_find_on_cmdline "$subcommands")"
if [ -z "$subcommand" ]; then
__gitcomp "$subcommands"
@@ -2297,9 +2300,12 @@ _git_remote ()
fi
case "$subcommand" in
- rename|rm|show|prune)
+ rename|rm|set-url|show|prune)
__gitcomp_nl "$(__git_remotes)"
;;
+ set-head|set-branches)
+ __git_complete_remote_or_refspec
+ ;;
update)
local i c='' IFS=$'\n'
for i in $(git --git-dir="$(__gitdir)" config --get-regexp "remotes\..*" 2>/dev/null); do
--
1.7.5.4
^ permalink raw reply related
* [PATCH 2/4] completion: normalize increment/decrement style
From: Philip Jägenstedt @ 2012-02-22 8:58 UTC (permalink / raw)
To: git
Cc: SZEDER Gábor, Felipe Contreras, Teemu Likonen,
Philip Jägenstedt
In-Reply-To: <1329901093-24106-1-git-send-email-philip@foolip.org>
The style used for incrementing and decrementing variables was fairly
inconsistenty and was normalized to use x++, or ((x++)) in contexts
where the former would otherwise be interpreted as a command. This is a
bash-ism, but for obvious reasons this script is already bash-specific.
Signed-off-by: Philip Jägenstedt <philip@foolip.org>
---
contrib/completion/git-completion.bash | 22 ++++++++++------------
1 files changed, 10 insertions(+), 12 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index c63a408..1903bc9 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -149,7 +149,7 @@ __git_ps1_show_upstream ()
svn_upstream=${svn_upstream[ ${#svn_upstream[@]} - 2 ]}
svn_upstream=${svn_upstream%@*}
local n_stop="${#svn_remote[@]}"
- for ((n=1; n <= n_stop; ++n)); do
+ for ((n=1; n <= n_stop; n++)); do
svn_upstream=${svn_upstream#${svn_remote[$n]}}
done
@@ -178,10 +178,8 @@ __git_ps1_show_upstream ()
for commit in $commits
do
case "$commit" in
- "<"*) let ++behind
- ;;
- *) let ++ahead
- ;;
+ "<"*) ((behind++)) ;;
+ *) ((ahead++)) ;;
esac
done
count="$behind $ahead"
@@ -739,7 +737,7 @@ __git_complete_remote_or_refspec ()
local cur_="$cur" cmd="${words[1]}"
local i c=2 remote="" pfx="" lhs=1 no_complete_refspec=0
if [ "$cmd" = "remote" ]; then
- c=$((++c))
+ ((c++))
fi
while [ $c -lt $cword ]; do
i="${words[c]}"
@@ -758,7 +756,7 @@ __git_complete_remote_or_refspec ()
-*) ;;
*) remote="$i"; break ;;
esac
- c=$((++c))
+ ((c++))
done
if [ -z "$remote" ]; then
__gitcomp_nl "$(__git_remotes)"
@@ -998,7 +996,7 @@ __git_find_on_cmdline ()
return
fi
done
- c=$((++c))
+ ((c++))
done
}
@@ -1009,7 +1007,7 @@ __git_has_doubledash ()
if [ "--" = "${words[c]}" ]; then
return 0
fi
- c=$((++c))
+ ((c++))
done
return 1
}
@@ -1132,7 +1130,7 @@ _git_branch ()
-d|-m) only_local_ref="y" ;;
-r) has_r="y" ;;
esac
- c=$((++c))
+ ((c++))
done
case "$cur" in
@@ -2586,7 +2584,7 @@ _git_tag ()
f=1
;;
esac
- c=$((++c))
+ ((c++))
done
case "$prev" in
@@ -2639,7 +2637,7 @@ _git ()
--help) command="help"; break ;;
*) command="$i"; break ;;
esac
- c=$((++c))
+ ((c++))
done
if [ -z "$command" ]; then
--
1.7.5.4
^ permalink raw reply related
* [PATCH 3/4] completion: remove stale "to submit patches" documentation
From: Philip Jägenstedt @ 2012-02-22 8:58 UTC (permalink / raw)
To: git
Cc: SZEDER Gábor, Felipe Contreras, Teemu Likonen,
Philip Jägenstedt
In-Reply-To: <1329901093-24106-1-git-send-email-philip@foolip.org>
It was out-of-sync with the reality of who works on this
script. Defer (silently) to Documentation/SubmittingPatches
like all other code.
Signed-off-by: Philip Jägenstedt <philip@foolip.org>
---
contrib/completion/git-completion.bash | 12 ------------
1 files changed, 0 insertions(+), 12 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 1903bc9..48237c8 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -60,18 +60,6 @@
# per-repository basis by setting the bash.showUpstream config
# variable.
#
-#
-# To submit patches:
-#
-# *) Read Documentation/SubmittingPatches
-# *) Send all patches to the current maintainer:
-#
-# "Shawn O. Pearce" <spearce@spearce.org>
-#
-# *) Always CC the Git mailing list:
-#
-# git@vger.kernel.org
-#
if [[ -n ${ZSH_VERSION-} ]]; then
autoload -U +X bashcompinit && bashcompinit
--
1.7.5.4
^ permalink raw reply related
* [PATCH 4/4] completion: use tabs for indentation
From: Philip Jägenstedt @ 2012-02-22 8:58 UTC (permalink / raw)
To: git
Cc: SZEDER Gábor, Felipe Contreras, Teemu Likonen,
Philip Jägenstedt
In-Reply-To: <1329901093-24106-1-git-send-email-philip@foolip.org>
CodingGuidlines confidently declares "We use tabs for indentation."
It would be a shame if it were caught lying.
Signed-off-by: Philip Jägenstedt <philip@foolip.org>
---
contrib/completion/git-completion.bash | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 48237c8..03f0b8c 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -284,13 +284,13 @@ __git_ps1 ()
fi
fi
if [ -n "${GIT_PS1_SHOWSTASHSTATE-}" ]; then
- git rev-parse --verify refs/stash >/dev/null 2>&1 && s="$"
+ git rev-parse --verify refs/stash >/dev/null 2>&1 && s="$"
fi
if [ -n "${GIT_PS1_SHOWUNTRACKEDFILES-}" ]; then
- if [ -n "$(git ls-files --others --exclude-standard)" ]; then
- u="%"
- fi
+ if [ -n "$(git ls-files --others --exclude-standard)" ]; then
+ u="%"
+ fi
fi
if [ -n "${GIT_PS1_SHOWUPSTREAM-}" ]; then
--
1.7.5.4
^ permalink raw reply related
* Re: [PATCH] contrib: added git-diffall
From: David Aguilar @ 2012-02-22 9:15 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Tim Henigan, git
In-Reply-To: <7vbooregj6.fsf@alter.siamese.dyndns.org>
On Tue, Feb 21, 2012 at 6:41 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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?
I don't see any bash-isms there myself, either. We should keep this
stuff without bash-isms.
I haven't had time to read these patches in depth yet but will try to
read the re-roll.
Can we ask the github user to elaborate on what exactly was erroring out?
Does dash not handle || inside $()? We can only make wild guesses
without their help.
The only hint from the pull request is "silent exit with no results".
Do we do that?
There are a few code paths where we do "exit 1" but that's only under
error conditions.
We haven't had any reports about git-mergetool/difftool, which use
these functions...
are we certain the problem was not some other bash-isms in the script?
--
David
^ permalink raw reply
* Re: [PATCH v3] completion: remote set-* <name> and <branch>
From: Thomas Rast @ 2012-02-22 9:52 UTC (permalink / raw)
To: Philip Jägenstedt
Cc: git, SZEDER Gábor, Felipe Contreras, Teemu Likonen
In-Reply-To: <1329901093-24106-1-git-send-email-philip@foolip.org>
Philip Jägenstedt <philip@foolip.org> writes:
> Hope this works better. Is it possible to use format-patch or send-email
> to get subjects like [PATCH 1/4 v3], as opposed to what I am sending?
'git format-patch --subject-prefix="PATCH v3"' gives you [PATCH v3 1/4]
which I think is what everyone does around here...
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply
* Re: [PATCH] contrib: added git-diffall
From: Stefano Lattarini @ 2012-02-22 10:05 UTC (permalink / raw)
To: Tim Henigan; +Cc: git, gitster
In-Reply-To: <1329785969-828-1-git-send-email-tim.henigan@gmail.com>
On 02/21/2012 01:59 AM, Tim Henigan wrote:
> test -z $(which mktemp 2>/dev/null)
>
This is wrong: if mktemp is not avilable, the expression above will
become, after command substitution and word splitting have taken pace,
equivalent to:
test -z
which, per POSIX, must return 0 (and does with at least bash 4.1.5 and
dash 0.5.5.1). You should just use this instead:
which mktemp 2>/dev/null
OK, technically you could also fix your idiom above a little and use:
test -z "$(which mktemp 2>/dev/null)"
but seems like a useless use of indirections to me.
And all of this is naturally render moot by Junio's advice of not using
which(1) in the first place ;-)
Regards,
Stefano
^ permalink raw reply
* Re: [PATCH] remote-curl: Fix push status report when all branches fail
From: Jeff King @ 2012-02-22 10:13 UTC (permalink / raw)
To: Shawn O. Pearce; +Cc: Junio C Hamano, git
In-Reply-To: <1327079011-24788-1-git-send-email-spearce@spearce.org>
On Fri, Jan 20, 2012 at 09:03:31AM -0800, Shawn O. Pearce wrote:
> diff --git a/remote-curl.c b/remote-curl.c
> index 48c20b8..25c1af7 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -822,12 +822,13 @@ static void parse_push(struct strbuf *buf)
> break;
> } while (1);
>
> - if (push(nr_spec, specs))
> - exit(128); /* error already reported */
> -
> + ret = push(nr_spec, specs);
> printf("\n");
> fflush(stdout);
>
> + if (ret)
> + exit(128); /* error already reported */
> +
This hunk is causing intermittent failures of t5541 for me, especially
when the system is under heavy load (e.g., make -j32 test). Before your
patch, this is what happened:
1. remote-curl relays the status lines from send-pack, then sees that
send-pack reported error, and it exits
2. push reads the status lines, looking for a blank line to terminate
them. It sees EOF instead of the blank line and exits(128) itself.
After your patch, this happens:
1. remote-curl relays the status lines, alway appends the blank line
terminator, and then exits
2. push reads the status lines, including the blank line terminator,
and reports them to the user.
3. push then disconnects the remote-curl helper by writing a blank
line to it (to signal end-of-input), followed by finish_command().
The latter propagates the error code from the exit in step 1, and
we use that to signal failure from "git push".
There's a race condition now in step 3. The push process may write to
the pipe going to remote-curl after it has exited, causing it to receive
SIGPIPE and die. We can block SIGPIPE, but that's not sufficient; we'll
still notice that our write() returns EPIPE and die.
Obviously we can't not print the post-push "\n" in remote-curl, for the
reasons you outlined in the commit message of this patch. We also can't
not exit from remote-curl on error. Even though in the test in t5541 we
have signaled error via the ref statuses, we might have received an
error that does not come through a ref status (e.g., if we couldn't run
send-pack at all).
We can't not write the "\n" to signal end-of-input to remote-curl,
because we don't actually know yet that there's an error (we find out
when we wait() on the process). Barring any asynchronous SIGCHLD
handling, of course, but I don't think we want to get into that.
So it's kind of a bug in the remote helper protocol. The helpers can
signal failure only by dying, but we can find out about that failure
only after disconnecting, which involves writing to them. It would be
much more sane if the helpers returned an overall text status from each
command (e.g., printed "error push failed" instead of dying).
But that would involve changing the protocol, of course. I think our
best option is to work around it by considering the final blank line we
send before disconnect as "best effort". That is, it is a courtesy to
the remote helper to tell it we are hanging up cleanly, and if it does
not arrive, then we can ignore the problem and proceed with closing the
pipe. I.e., something like:
diff --git a/transport-helper.c b/transport-helper.c
index 6f227e2..f6b3b1f 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -9,6 +9,7 @@
#include "remote.h"
#include "string-list.h"
#include "thread-utils.h"
+#include "sigchain.h"
static int debug;
@@ -220,15 +221,21 @@ static struct child_process *get_helper(struct transport *transport)
static int disconnect_helper(struct transport *transport)
{
struct helper_data *data = transport->data;
- struct strbuf buf = STRBUF_INIT;
int res = 0;
if (data->helper) {
if (debug)
fprintf(stderr, "Debug: Disconnecting.\n");
if (!data->no_disconnect_req) {
- strbuf_addf(&buf, "\n");
- sendline(data, &buf);
+ /*
+ * Ignore write errors; there's nothing we can do,
+ * since we're about to close the pipe anyway. And the
+ * most likely error is EPIPE due to the helper dying
+ * to report an error itself.
+ */
+ sigchain_push(SIGPIPE, SIG_IGN);
+ xwrite(data->helper->in, "\n", 1);
+ sigchain_pop(SIGPIPE);
}
close(data->helper->in);
close(data->helper->out);
which makes the t5541 failures go away for me. What do you think?
-Peff
^ permalink raw reply related
* [PATCHv4] git-p4: RCS keyword handling
From: Luke Diamand @ 2012-02-22 10:15 UTC (permalink / raw)
To: git; +Cc: Pete Wyckoff, Eric Scouten, Junio C Hamano, Luke Diamand
Updated patch incorporating comments from Pete and Junio to
handle RCS keyword expansion in git-p4.
Note: The current patch fails to to handle the case where a user
adds an expanded RCS keyword in *git* (e.g. via cut-n-paste).
I'll try to address that separately. There's a failing test case
for this.
This version:
- uses "p4 fstat" to get the filetype
- uses Junio's suggested regexp to match $Keyword:$
- uses the sets of added/deleted files rather than parsing diff output
- various other small fixes spotted by Pete
- adds additional test cases
Luke Diamand (1):
git-p4: add initial support for RCS keywords
Documentation/git-p4.txt | 5 +
contrib/fast-import/git-p4 | 118 ++++++++++++--
t/t9810-git-p4-rcs.sh | 388 ++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 501 insertions(+), 10 deletions(-)
create mode 100755 t/t9810-git-p4-rcs.sh
--
1.7.9.259.ga92e
^ permalink raw reply
* [PATCHv4] git-p4: add initial support for RCS keywords
From: Luke Diamand @ 2012-02-22 10:15 UTC (permalink / raw)
To: git; +Cc: Pete Wyckoff, Eric Scouten, Junio C Hamano, Luke Diamand
In-Reply-To: <1329905741-2092-1-git-send-email-luke@diamand.org>
RCS keywords cause problems for git-p4 as perforce always
expands them (if +k is set) and so when applying the patch,
git reports that the files have been modified by both sides,
when in fact they haven't.
This change means that when git-p4 detects a problem applying
a patch, it will check to see if keyword expansion could be
the culprit. If it is, it strips the keywords in the p4
repository so that they match what git is expecting. It then
has another go at applying the patch.
This behaviour is enabled with a new git-p4 configuration
option and is off by default.
Improved-by: Pete Wyckoff <pw@padd.com>
Signed-off-by: Luke Diamand <luke@diamand.org>
---
Documentation/git-p4.txt | 5 +
contrib/fast-import/git-p4 | 118 ++++++++++++--
t/t9810-git-p4-rcs.sh | 388 ++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 501 insertions(+), 10 deletions(-)
create mode 100755 t/t9810-git-p4-rcs.sh
diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
index 8b92cc0..3fecefa 100644
--- a/Documentation/git-p4.txt
+++ b/Documentation/git-p4.txt
@@ -483,6 +483,11 @@ git-p4.skipUserNameCheck::
user map, 'git p4' exits. This option can be used to force
submission regardless.
+git-p4.attemptRCSCleanup:
+ If enabled, 'git p4 submit' will attempt to cleanup RCS keywords
+ ($Header$, etc). These would otherwise cause merge conflicts and prevent
+ the submit going ahead. This option should be considered experimental at
+ present.
IMPLEMENTATION DETAILS
----------------------
diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index a78d9c5..c8b6c8a 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -10,7 +10,7 @@
import optparse, sys, os, marshal, subprocess, shelve
import tempfile, getopt, os.path, time, platform
-import re
+import re, shutil
verbose = False
@@ -186,6 +186,47 @@ def split_p4_type(p4type):
mods = s[1]
return (base, mods)
+#
+# return the raw p4 type of a file (text, text+ko, etc)
+#
+def p4_type(file):
+ results = p4CmdList(["fstat", "-T", "headType", file])
+ return results[0]['headType']
+
+#
+# Given a type base and modifier, return a regexp matching
+# the keywords that can be expanded in the file
+#
+def p4_keywords_regexp_for_type(base, type_mods):
+ if base in ("text", "unicode", "binary"):
+ kwords = None
+ if "ko" in type_mods:
+ kwords = 'Id|Header'
+ elif "k" in type_mods:
+ kwords = 'Id|Header|Author|Date|DateTime|Change|File|Revision'
+ else:
+ return None
+ pattern = r"""
+ \$ # Starts with a dollar, followed by...
+ (%s) # one of the keywords, followed by...
+ (:[^$]+)? # possibly an old expansion, followed by...
+ \$ # another dollar
+ """ % kwords
+ return pattern
+ else:
+ return None
+
+#
+# Given a file, return a regexp matching the possible
+# RCS keywords that will be expanded, or None for files
+# with kw expansion turned off.
+#
+def p4_keywords_regexp_for_file(file):
+ if not os.path.exists(file):
+ return None
+ else:
+ (type_base, type_mods) = split_p4_type(p4_type(file))
+ return p4_keywords_regexp_for_type(type_base, type_mods)
def setP4ExecBit(file, mode):
# Reopens an already open file and changes the execute bit to match
@@ -753,6 +794,29 @@ class P4Submit(Command, P4UserMap):
return result
+ def patchRCSKeywords(self, file, pattern):
+ # Attempt to zap the RCS keywords in a p4 controlled file matching the given pattern
+ (handle, outFileName) = tempfile.mkstemp(dir='.')
+ try:
+ outFile = os.fdopen(handle, "w+")
+ inFile = open(file, "r")
+ regexp = re.compile(pattern, re.VERBOSE)
+ for line in inFile.readlines():
+ line = regexp.sub(r'$\1$', line)
+ outFile.write(line)
+ inFile.close()
+ outFile.close()
+ # Forcibly overwrite the original file
+ os.unlink(file)
+ shutil.move(outFileName, file)
+ except:
+ # cleanup our temporary file
+ os.unlink(outFileName)
+ print "Failed to strip RCS keywords in %s" % file
+ raise
+
+ print "Patched up RCS keywords in %s" % file
+
def p4UserForCommit(self,id):
# Return the tuple (perforce user,git email) for a given git commit id
self.getUserMapFromPerforceServer()
@@ -918,6 +982,7 @@ class P4Submit(Command, P4UserMap):
filesToDelete = set()
editedFiles = set()
filesToChangeExecBit = {}
+
for line in diff:
diff = parseDiffTreeEntry(line)
modifier = diff['status']
@@ -964,9 +1029,45 @@ class P4Submit(Command, P4UserMap):
patchcmd = diffcmd + " | git apply "
tryPatchCmd = patchcmd + "--check -"
applyPatchCmd = patchcmd + "--check --apply -"
+ patch_succeeded = True
if os.system(tryPatchCmd) != 0:
+ fixed_rcs_keywords = False
+ patch_succeeded = False
print "Unfortunately applying the change failed!"
+
+ # Patch failed, maybe it's just RCS keyword woes. Look through
+ # the patch to see if that's possible.
+ if gitConfig("git-p4.attemptRCSCleanup","--bool") == "true":
+ file = None
+ pattern = None
+ kwfiles = {}
+ for file in editedFiles | filesToDelete:
+ # did this file's delta contain RCS keywords?
+ pattern = p4_keywords_regexp_for_file(file)
+
+ if pattern:
+ # this file is a possibility...look for RCS keywords.
+ regexp = re.compile(pattern, re.VERBOSE)
+ for line in read_pipe_lines(["git", "diff", "%s^..%s" % (id, id), file]):
+ if regexp.search(line):
+ if verbose:
+ print "got keyword match on %s in %s in %s" % (pattern, line, file)
+ kwfiles[file] = pattern
+ break
+
+ for file in kwfiles:
+ if verbose:
+ print "zapping %s with %s" % (line,pattern)
+ self.patchRCSKeywords(file, kwfiles[file])
+ fixed_rcs_keywords = True
+
+ if fixed_rcs_keywords:
+ print "Retrying the patch with RCS keywords cleaned up"
+ if os.system(tryPatchCmd) == 0:
+ patch_succeeded = True
+
+ if not patch_succeeded:
print "What do you want to do?"
response = "x"
while response != "s" and response != "a" and response != "w":
@@ -1585,15 +1686,12 @@ class P4Sync(Command, P4UserMap):
# Note that we do not try to de-mangle keywords on utf16 files,
# even though in theory somebody may want that.
- if type_base in ("text", "unicode", "binary"):
- if "ko" in type_mods:
- text = ''.join(contents)
- text = re.sub(r'\$(Id|Header):[^$]*\$', r'$\1$', text)
- contents = [ text ]
- elif "k" in type_mods:
- text = ''.join(contents)
- text = re.sub(r'\$(Id|Header|Author|Date|DateTime|Change|File|Revision):[^$]*\$', r'$\1$', text)
- contents = [ text ]
+ pattern = p4_keywords_regexp_for_type(type_base, type_mods)
+ if pattern:
+ regexp = re.compile(pattern, re.VERBOSE)
+ text = ''.join(contents)
+ text = regexp.sub(r'$\1$', text)
+ contents = [ text ]
self.gitStream.write("M %s inline %s\n" % (git_mode, relPath))
diff --git a/t/t9810-git-p4-rcs.sh b/t/t9810-git-p4-rcs.sh
new file mode 100755
index 0000000..9e84b5a
--- /dev/null
+++ b/t/t9810-git-p4-rcs.sh
@@ -0,0 +1,388 @@
+#!/bin/sh
+
+test_description='git-p4 rcs keywords'
+
+. ./lib-git-p4.sh
+
+test_expect_success 'start p4d' '
+ start_p4d
+'
+
+#
+# Make one file with keyword lines at the top, and
+# enough plain text to be able to test modifications
+# far away from the keywords.
+#
+test_expect_success 'init depot' '
+ (
+ cd "$cli" &&
+ cat <<-\EOF >filek &&
+ $Id$
+ /* $Revision$ */
+ # $Change$
+ line4
+ line5
+ line6
+ line7
+ line8
+ EOF
+ cp filek fileko &&
+ sed -i "s/Revision/Revision: do not scrub me/" fileko
+ cp fileko file_text &&
+ sed -i "s/Id/Id: do not scrub me/" file_text
+ p4 add -t text+k filek &&
+ p4 submit -d "filek" &&
+ p4 add -t text+ko fileko &&
+ p4 submit -d "fileko" &&
+ p4 add -t text file_text &&
+ p4 submit -d "file_text"
+ )
+'
+
+#
+# Generate these in a function to make it easy to use single quote marks.
+#
+write_scrub_scripts() {
+ cat >"$TRASH_DIRECTORY/scrub_k.py" <<-\EOF &&
+ import re, sys
+ sys.stdout.write(re.sub(r'(?i)\$(Id|Header|Author|Date|DateTime|Change|File|Revision):[^$]*\$', r'$\1$', sys.stdin.read()))
+ EOF
+ cat >"$TRASH_DIRECTORY/scrub_ko.py" <<-\EOF
+ import re, sys
+ sys.stdout.write(re.sub(r'(?i)\$(Id|Header):[^$]*\$', r'$\1$', sys.stdin.read()))
+ EOF
+}
+
+test_expect_success 'scrub scripts' '
+ write_scrub_scripts
+'
+
+#
+# Compare $cli/file to its scrubbed version, should be different.
+# Compare scrubbed $cli/file to $git/file, should be same.
+#
+scrub_k_check() {
+ file=$1 &&
+ scrub="$TRASH_DIRECTORY/$file" &&
+ "$PYTHON_PATH" "$TRASH_DIRECTORY/scrub_k.py" <"$git/$file" >"$scrub" &&
+ ! test_cmp "$cli/$file" "$scrub" &&
+ test_cmp "$git/$file" "$scrub" &&
+ rm "$scrub"
+}
+scrub_ko_check() {
+ file=$1 &&
+ scrub="$TRASH_DIRECTORY/$file" &&
+ "$PYTHON_PATH" "$TRASH_DIRECTORY/scrub_ko.py" <"$git/$file" >"$scrub" &&
+ ! test_cmp "$cli/$file" "$scrub" &&
+ test_cmp "$git/$file" "$scrub" &&
+ rm "$scrub"
+}
+
+#
+# Modify far away from keywords. If no RCS lines show up
+# in the diff, there is no conflict.
+#
+test_expect_success 'edit far away from RCS lines' '
+ test_when_finished cleanup_git &&
+ "$GITP4" clone --dest="$git" //depot &&
+ (
+ cd "$git" &&
+ git config git-p4.skipSubmitEdit true &&
+ sed -i "s/^line7/line7 edit/" filek &&
+ git commit -m "filek line7 edit" filek &&
+ "$GITP4" submit &&
+ scrub_k_check filek
+ )
+'
+
+#
+# Modify near the keywords. This will require RCS scrubbing.
+#
+test_expect_success 'edit near RCS lines' '
+ test_when_finished cleanup_git &&
+ "$GITP4" clone --dest="$git" //depot &&
+ (
+ cd "$git" &&
+ git config git-p4.skipSubmitEdit true &&
+ git config git-p4.attemptRCSCleanup true &&
+ sed -i "s/^line4/line4 edit/" filek &&
+ git commit -m "filek line4 edit" filek &&
+ "$GITP4" submit &&
+ scrub_k_check filek
+ )
+'
+
+#
+# Modify the keywords themselves. This also will require RCS scrubbing.
+#
+test_expect_success 'edit keyword lines' '
+ test_when_finished cleanup_git &&
+ "$GITP4" clone --dest="$git" //depot &&
+ (
+ cd "$git" &&
+ git config git-p4.skipSubmitEdit true &&
+ git config git-p4.attemptRCSCleanup true &&
+ sed -i "/Revision/d" filek &&
+ git commit -m "filek remove Revision line" filek &&
+ "$GITP4" submit &&
+ scrub_k_check filek
+ )
+'
+
+#
+# Scrubbing text+ko files should not alter all keywords, just Id, Header.
+#
+test_expect_success 'scrub ko files differently' '
+ test_when_finished cleanup_git &&
+ "$GITP4" clone --dest="$git" //depot &&
+ (
+ cd "$git" &&
+ git config git-p4.skipSubmitEdit true &&
+ git config git-p4.attemptRCSCleanup true &&
+ sed -i "s/^line4/line4 edit/" fileko &&
+ git commit -m "fileko line4 edit" fileko &&
+ "$GITP4" submit &&
+ scrub_ko_check fileko &&
+ ! scrub_k_check fileko
+ )
+'
+
+# hack; git-p4 submit should do it on its own
+test_expect_success 'cleanup after failure' '
+ (
+ cd "$cli" &&
+ p4 revert ...
+ )
+'
+
+#
+# Do not scrub anything but +k or +ko files. Sneak a change into
+# the cli file so that submit will get a conflict. Make sure that
+# scrubbing doesn't make a mess of things.
+#
+# Assumes that git-p4 exits leaving the p4 file open, with the
+# conflict-generating patch unapplied.
+#
+# This might happen only if the git repo is behind the p4 repo at
+# submit time, and there is a conflict.
+#
+test_expect_success 'do not scrub plain text' '
+ test_when_finished cleanup_git &&
+ "$GITP4" clone --dest="$git" //depot &&
+ (
+ cd "$git" &&
+ git config git-p4.skipSubmitEdit true &&
+ git config git-p4.attemptRCSCleanup true &&
+ sed -i "s/^line4/line4 edit/" file_text &&
+ git commit -m "file_text line4 edit" file_text &&
+ (
+ cd "$cli" &&
+ p4 open file_text &&
+ sed -i "s/^line5/line5 p4 edit/" file_text &&
+ p4 submit -d "file5 p4 edit"
+ ) &&
+ ! "$GITP4" submit &&
+ (
+ # exepct something like:
+ # file_text - file(s) not opened on this client
+ # but not copious diff output
+ cd "$cli" &&
+ p4 diff file_text >wc &&
+ test_line_count = 1 wc
+ )
+ )
+'
+
+# hack; git-p4 submit should do it on its own
+test_expect_success 'cleanup after failure 2' '
+ (
+ cd "$cli" &&
+ p4 revert ...
+ )
+'
+
+create_kw_file() {
+ cat <<\EOF >"$1"
+/* A file
+ Id: $Id$
+ Revision: $Revision$
+ File: $File$
+ */
+int main(int argc, const char **argv) {
+ return 0;
+}
+EOF
+}
+
+test_expect_success 'add kwfile' '
+ (
+ cd "$cli" &&
+ echo file1 >file1 &&
+ p4 add file1 &&
+ p4 submit -d "file 1" &&
+ create_kw_file kwfile1.c &&
+ p4 add kwfile1.c &&
+ p4 submit -d "Add rcw kw file" kwfile1.c
+ )
+'
+
+p4_append_to_file() {
+ f=$1 &&
+ p4 edit -t ktext "$f" &&
+ echo "/* $(date) */" >>"$f" &&
+ p4 submit -d "appending a line in p4"
+}
+
+# Create some files with RCS keywords. If they get modified
+# elsewhere then the version number gets bumped which then
+# results in a merge conflict if we touch the RCS kw lines,
+# even though the change itself would otherwise apply cleanly.
+test_expect_success 'cope with rcs keyword expansion damage' '
+ test_when_finished cleanup_git &&
+ "$GITP4" clone --dest="$git" //depot &&
+ (
+ cd "$git" &&
+ git config git-p4.skipSubmitEdit true &&
+ git config git-p4.attemptRCSCleanup true &&
+ (cd ../cli && p4_append_to_file kwfile1.c) &&
+ old_lines=$(wc -l <kwfile1.c) &&
+ perl -n -i -e "print unless m/Revision:/" kwfile1.c &&
+ new_lines=$(wc -l <kwfile1.c) &&
+ expr $new_lines == $old_lines - 1 &&
+
+ git add kwfile1.c &&
+ git commit -m "Zap an RCS kw line" &&
+ "$GITP4" submit &&
+ "$GITP4" rebase &&
+ git diff p4/master &&
+ "$GITP4" commit &&
+ echo "try modifying in both" &&
+ cd "$cli" &&
+ p4 edit kwfile1.c &&
+ echo "line from p4" >>kwfile1.c &&
+ p4 submit -d "add a line in p4" kwfile1.c &&
+ cd "$git" &&
+ echo "line from git at the top" | cat - kwfile1.c >kwfile1.c.new &&
+ mv kwfile1.c.new kwfile1.c &&
+ git commit -m "Add line in git at the top" kwfile1.c &&
+ "$GITP4" rebase &&
+ "$GITP4" submit
+ )
+'
+
+test_expect_success 'cope with rcs keyword file deletion' '
+ test_when_finished cleanup_git &&
+ (
+ cd "$cli" &&
+ echo "\$Revision\$" >kwdelfile.c &&
+ p4 add -t ktext kwdelfile.c &&
+ p4 submit -d "Add file to be deleted" &&
+ cat kwdelfile.c &&
+ grep -q 1 kwdelfile.c
+ ) &&
+ "$GITP4" clone --dest="$git" //depot &&
+ (
+ cd "$git" &&
+ grep -q Revision kwdelfile.c &&
+ git rm -f kwdelfile.c &&
+ git commit -m "Delete a file containing RCS keywords" &&
+ git config git-p4.skipSubmitEdit true &&
+ git config git-p4.attemptRCSCleanup true &&
+ "$GITP4" submit
+ ) &&
+ (
+ cd "$cli" &&
+ p4 sync &&
+ [ ! -f kwdelfile.c ]
+ )
+'
+
+# If you add keywords in git of the form $Header$ then everything should
+# work fine without any special handling.
+test_expect_success 'Add keywords in git which match the default p4 values' '
+ test_when_finished cleanup_git &&
+ "$GITP4" clone --dest="$git" //depot &&
+ (
+ cd "$git" &&
+ echo "NewKW: \$Revision\$" >>kwfile1.c &&
+ git add kwfile1.c &&
+ git commit -m "Adding RCS keywords in git" &&
+ git config git-p4.skipSubmitEdit true &&
+ git config git-p4.attemptRCSCleanup true &&
+ "$GITP4" submit
+ ) &&
+ (
+ cd "$cli" &&
+ p4 sync &&
+ [ -f kwfile1.c ] &&
+ grep 'NewKW.*Revision.*[0-9]' kwfile1.c
+
+ )
+'
+
+# If you add keywords in git of the form $Header:#1$ then things will fail
+# unless git-p4 takes steps to scrub the *git* commit.
+#
+test_expect_failure 'Add keywords in git which do not match the default p4 values' '
+ test_when_finished cleanup_git &&
+ "$GITP4" clone --dest="$git" //depot &&
+ (
+ cd "$git" &&
+ echo "NewKW2: \$Revision:1\$" >>kwfile1.c &&
+ git add kwfile1.c &&
+ git commit -m "Adding RCS keywords in git" &&
+ git config git-p4.skipSubmitEdit true &&
+ git config git-p4.attemptRCSCleanup true &&
+ "$GITP4" submit
+ ) &&
+ (
+ cd "$cli" &&
+ p4 sync &&
+ grep 'NewKW2.*Revision.*[0-9]' kwfile1.c
+
+ )
+'
+
+# Check that the existing merge conflict handling still works.
+# Modify kwfile1.c in git, and delete in p4. We should be able
+# to skip the git commit.
+#
+test_expect_success 'merge conflict handling still works' '
+ test_when_finished cleanup_git &&
+ (
+ cd "$cli" &&
+ echo "Hello:\$Id\$" >merge2.c &&
+ echo "World" >>merge2.c &&
+ p4 add -t ktext merge2.c &&
+ p4 submit -d "add merge test file"
+ ) &&
+ "$GITP4" clone --dest="$git" //depot &&
+ (
+ cd "$git" &&
+ sed -e "/Hello/d" merge2.c >merge2.c.tmp &&
+ mv merge2.c.tmp merge2.c &&
+ git add merge2.c &&
+ git commit -m "Modifying merge2.c"
+ ) &&
+ (
+ cd "$cli" &&
+ p4 delete merge2.c &&
+ p4 submit -d "remove merge test file"
+ ) &&
+ (
+ cd "$git" &&
+ [ -f merge2.c ] &&
+ git config git-p4.skipSubmitEdit true &&
+ git config git-p4.attemptRCSCleanup true &&
+ !(echo "s" | "$GITP4" submit) &&
+ git rebase --skip &&
+ [ ! -f merge2.c ]
+ )
+'
+
+
+test_expect_success 'kill p4d' '
+ kill_p4d
+'
+
+test_done
--
1.7.9.259.ga92e
^ permalink raw reply related
* Re: git status: small difference between stating whole repository and small subdirectory
From: Nguyen Thai Ngoc Duy @ 2012-02-22 10:34 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 Tue, Feb 21, 2012 at 11:16:37AM -0800, Junio C Hamano wrote:
> Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
>
> > I'm aware that Jeff's tackling at lower level, which retains
> > cache-tree for many more cases.
> >
> > But this patch seems simple and safe
> > to me, and in my experience this case happens quite often (or maybe I
> > tend to keep my index clean). Junio, any chance this patch may get in?
>
> I do not think we are talking about a duplicated effort here.
>
> By definition, the change to hook into unpack_trees() and making sure we
> invalidate all the necessary subtrees in the cache cannot give you a cache
> tree that is more populated than what you started with. And the train of
> thought in Peff's message is to improve this invalidation---we currently
> invalidate everything ;-)
>
> Somebody has to populate the cache tree fully when we _know_ the index
> matches a certain tree, and adding a call to prime_cache_tree() in
> strategic places is a way to do so. The most obvious is write-tree, but
> there are a few other existing codepaths that do so.
>
> 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)".
It's tradeoff. As you said prime_cache_tree() is expensive.
cache_tree_update is supposed to be cheap. But cache_tree_update() when
empty is even more expensive than prime_cache_tree(). So it boils down
how much cache-tree we have after unpack_trees() and whether it's worth
repopulate cache-tree again.
> 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
> ;-)
I timed prime_cache_tree() and cache_tree_update() while switching branch
on linux-2.6, between v2.6.32 and a quite recent version. prime_cache_tree()
took ~55ms while cache_tree_update() 150ms or 90ms (depending on final tree).
It depends on your view, whether 55ms is expensive on such a reasonably large
repository. I took several seconds for me to complete the checkout though.
Checking out with "-q" prime_cache_tree() took 145ms and 80ms respectively,
as expensive as cache_tree_update()
If cache-tree is only used at commit time, I think we could delay
prime_cache_tree() until absolutely needed. We could add an optional index
extension recording the fact that index matches certain tree.
On the first cache_tree_invalidate_path(), if cache-tree is still
empty, we prime cache-tree, then invalidate the requested path.
It would then add no cost to a quick branch switch.
But if diff-cached also takes advantage of cache-tree, it's a different story.
Anyway, I think this patch does better than my last one
-- 8< --
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 6b9061f..e7eaeec 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -387,6 +387,7 @@ static int merge_working_tree(struct checkout_opts *opts,
int ret;
struct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));
int newfd = hold_locked_index(lock_file, 1);
+ int head_index_mismatch = 1;
if (read_cache_preload(NULL) < 0)
return error(_("corrupt index file"));
@@ -396,6 +397,7 @@ static int merge_working_tree(struct checkout_opts *opts,
ret = reset_tree(new->commit->tree, opts, 1);
if (ret)
return ret;
+ head_index_mismatch = 0;
} else {
struct tree_desc trees[2];
struct tree *tree;
@@ -490,7 +492,27 @@ static int merge_working_tree(struct checkout_opts *opts,
ret = reset_tree(new->commit->tree, opts, 0);
if (ret)
return ret;
- }
+ } else
+ head_index_mismatch = topts.head_index_mismatch;
+ }
+
+ /*
+ * Currently cache-tree is always destroyed after
+ * unpack_trees(). It's probably a good idea to repopulate
+ * cache-tree. If the user makes a few modifications and
+ * commits, tree generation woulda be cheap. If they switch
+ * away again, not so cheap.
+ *
+ * When unpack_trees() learns to retains as much cache-tree as
+ * possible, this code probably does not help much on tree
+ * generation, unless the tree difference between to heads are
+ * too far, little cache-tree can be kept.
+ */
+ if (!head_index_mismatch &&
+ !cache_tree_fully_valid(active_cache_tree)) {
+ if (!new->commit->tree->object.parsed)
+ parse_tree(new->commit->tree);
+ prime_cache_tree(&active_cache_tree, new->commit->tree);
}
if (write_cache(newfd, active_cache, active_nr) ||
diff --git a/unpack-trees.c b/unpack-trees.c
index 7c9ecf6..f2c518f 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1022,6 +1022,8 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
o->result.timestamp.nsec = o->src_index->timestamp.nsec;
o->merge_size = len;
mark_all_ce_unused(o->src_index);
+ if (o->fn != twoway_merge)
+ o->head_index_mismatch = 1;
/*
* Sparse checkout loop #1: set NEW_SKIP_WORKTREE on existing entries
@@ -1736,6 +1738,8 @@ int twoway_merge(struct cache_entry **src, struct unpack_trees_options *o)
(oldtree && newtree &&
!same(oldtree, newtree) && /* 18 and 19 */
same(current, newtree))) {
+ if (!newtree || (newtree && !same(current, newtree)))
+ o->head_index_mismatch = 1;
return keep_entry(current, o);
}
else if (oldtree && !newtree && same(current, oldtree)) {
diff --git a/unpack-trees.h b/unpack-trees.h
index 5e432f5..b75b64e 100644
--- a/unpack-trees.h
+++ b/unpack-trees.h
@@ -48,7 +48,8 @@ struct unpack_trees_options {
gently,
exiting_early,
show_all_errors,
- dry_run;
+ dry_run,
+ head_index_mismatch;
const char *prefix;
int cache_bottom;
struct dir_struct *dir;
-- 8< --
^ permalink raw reply related
* Re: [PATCH 0/8 v6] diff --stat: use the full terminal width
From: Zbigniew Jędrzejewski-Szmek @ 2012-02-22 11:24 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Michael J Gruber, pclouds, j.sixt
In-Reply-To: <7v1upogd6w.fsf@alter.siamese.dyndns.org>
On 02/21/2012 09:10 PM, Junio C Hamano wrote:
> Zbigniew Jędrzejewski-Szmek<zbyszek@in.waw.pl> writes:
>
>> This seem overly complex. A nice property to have would be
>> "if the window is wide enough so there's enough space for full
>> filenames, the graph part scales monotonically with the change count".
>> (If there's filename truncation, than there just isn't enough space
>> for everything and the graph may be compressed. But otherwise, if we
>> have two graphs which do not end at the edge of the screen, and the
>> second one is wider than the first one, then without looking at the
>> change counts we know that the second one has more changes).
>>
>> For this property to be satisfied, the graph_width limit would have to
>> be independent of the filename width.
>>
>> So maybe it should be ...
>
> Sorry, the desired property I would understand, but that does not click
> with your "have to be independent" conclusion, so I do not have comment on
> the "maybe it should be..." part.
Hi,
by "scales monotonically with the change count" I meant with two
different commits. Image that there are two commits
a | 300 ++++++++++++++++++++++
and
a/a/a/b | 300 ++++++++++++++++++++++
Both commits have the same change count, but filenames of different
length. If the filename length can influence the number of "+" in the
graph, then the scaling is not monotonic. There would always be cases
when a bigger change with longer filenames has a narrower graph.
> The resolution requirement may want to set a "desired lower limit" for the
> width of the graph, but it is only "desired" because it is possible that
> you have to bust the limit if you have three files with 1, 9999 and 10000
> changed lines and your terminal is only 200 columns wide.
>
> The current code caps name part to 50/80, but allows the graph to use more
> when you have only shorter names. Perhaps you can follow the same logic
> in the first part of your [7/8] (which needs to be separated to at least
> in two pieces, as it conflates the "lift 50-column cap from the name width
> and make it proportional to the term_width()" part and "but cap the graph
> part to 40-column" part, that are separate topics)? Then we can try
> different heuristics to find a better way to cap the length of the graph
> on top?
Sure. I'll be replying to this mail with patches
[7.1/8] use a maximum of 5/8 for the filename part
[7.2/8] add a test for output with COLUMNS=40
[7.3/8] limit graph part to 40 columns
--
Zbyszek
^ permalink raw reply
* contrib/git-move-refs.py not exists in cvs2svn-2.3.0
From: supadhyay @ 2012-02-22 11:25 UTC (permalink / raw)
To: git
Hi All,
I have run the cvs2git tool to migrate my CVS repository to git. I am able
to finish successfully. Now as the last recommended steps from
http://cvs2svn.tigris.org/cvs2git.html , I found to run
"contrib/git-move-refs.py" but in my cvs2svn-2.3.0 I can not see any file
name like "git-move-refs.ph", I have one file "git-move-tags.py". Is this
is the same file ?
Can anyone please update me?
thanks.
--
View this message in context: http://git.661346.n2.nabble.com/contrib-git-move-refs-py-not-exists-in-cvs2svn-2-3-0-tp7308023p7308023.html
Sent from the git mailing list archive at Nabble.com.
^ permalink raw reply
* [PATCH 7.3/8] diff --stat: limit graph part to 40 columns
From: Zbigniew Jędrzejewski-Szmek @ 2012-02-22 11:51 UTC (permalink / raw)
To: git, gitster
Cc: Michael J Gruber, pclouds, j.sixt,
Zbigniew Jędrzejewski-Szmek
In-Reply-To: <1329911507-10587-1-git-send-email-zbyszek@in.waw.pl>
The way that available columns are divided between the filename part
and the graph part is modified to use as many columns as necessary for
the filenames and up to 40 columns for the graph.
If commits changing a lot of lines are displayed in a wide terminal
window (200 or more columns), and the +- graph would use the full
width, the output would look bad. Messages wrapped to about 80 columns
would be interspersed with very long +- lines. It makes sense to limit
the width of the graph part to a fixed value, even if more columns are
available. This fixed value is subjectively hard-coded to be 40
columns, which seems to work well for git.git and linux-2.6.git and
some other repositories.
Signed-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
---
Documentation/diff-options.txt | 2 +-
diff.c | 8 ++++++--
t/t4052-stat-output.sh | 16 ++++++----------
3 files changed, 13 insertions(+), 13 deletions(-)
diff --git Documentation/diff-options.txt Documentation/diff-options.txt
index 29647e5..e4d0e3e 100644
--- Documentation/diff-options.txt
+++ Documentation/diff-options.txt
@@ -54,7 +54,7 @@ endif::git-format-patch[]
--stat[=<width>[,<name-width>[,<count>]]]::
Generate a diffstat. By default, as much space as necessary
- will be used for the filename part, and the rest for
+ will be used for the filename part, and up to 40 columns for
the graph part. Maximum width defaults to terminal width,
or 80 columns if not connected to a terminal, and can be
overriden by `<width>`. The width of the filename part can be
diff --git diff.c diff.c
index f1c278f..8a9a387 100644
--- diff.c
+++ diff.c
@@ -1421,13 +1421,15 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
/*
* We have width = stat_width or term_columns() columns total.
* We want a maximum of min(max_len, stat_name_width) for the name part.
+ * We want a maximum of min(max_change, 40) for the +- part.
* We also need 1 for " " and 4 + decimal_width(max_change)
* for " | NNNN " and one the empty column at the end, altogether
* 6 + decimal_width(max_change).
*
* If there's not enough space, we will use the smaller of
* stat_name_width (if set) and 5/8*width for the filename,
- * and the rest for constant elements + graph part.
+ * and the rest for constant elements + graph part, but no more
+ * than 40 for the graph part.
* (5/8 gives 50 for filename and 30 for the constant parts + graph
* for the standard terminal size).
*
@@ -1452,7 +1454,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
/*
* First assign sizes that are wanted, ignoring available width.
*/
- graph_width = max_change;
+ graph_width = max_change < 40 ? max_change : 40;
name_width = (options->stat_name_width > 0 &&
options->stat_name_width < max_len) ?
options->stat_name_width : max_len;
@@ -1463,6 +1465,8 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
if (name_width + number_width + 6 + graph_width > width) {
if (graph_width > width * 3/8 - number_width - 6)
graph_width = width * 3/8 - number_width - 6;
+ if (graph_width > 40)
+ graph_width = 40;
if (name_width > width - number_width - 6 - graph_width)
name_width = width - number_width - 6 - graph_width;
else
diff --git t/t4052-stat-output.sh t/t4052-stat-output.sh
index 84be8bd..1b237b7 100755
--- t/t4052-stat-output.sh
+++ t/t4052-stat-output.sh
@@ -78,11 +78,7 @@ test_expect_success 'preparation for big change tests' '
'
cat >expect80 <<'EOF'
- abcd | 1000 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
-EOF
-
-cat >expect200 <<'EOF'
- abcd | 1000 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
+ abcd | 1000 ++++++++++++++++++++++++++++++++++++++++
EOF
while read verb expect cmd args
@@ -94,9 +90,9 @@ do
'
done <<\EOF
ignores expect80 format-patch -1 --stdout
-respects expect200 diff HEAD^ HEAD --stat
-respects expect200 show --stat
-respects expect200 log -1 --stat
+respects expect80 diff HEAD^ HEAD --stat
+respects expect80 show --stat
+respects expect80 log -1 --stat
EOF
cat >expect40 <<'EOF'
@@ -170,7 +166,7 @@ cat >expect80 <<'EOF'
...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 ++++++++++++++++++++
EOF
cat >expect200 <<'EOF'
- aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
+ aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 ++++++++++++++++++++++++++++++++++++++++
EOF
while read verb expect cmd args
do
@@ -187,7 +183,7 @@ respects expect200 log -1 --stat
EOF
cat >expect <<'EOF'
- abcd | 1000 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
+ abcd | 1000 ++++++++++++++++++++++++++++++++++++++++
EOF
test_expect_success 'merge --stat respects COLUMNS (big change)' '
git checkout -b branch HEAD^^ &&
--
1.7.9.1.355.ge8a9f
^ permalink raw reply related
* [PATCH 7.2/8] diff --stat: add a test for output with COLUMNS=40
From: Zbigniew Jędrzejewski-Szmek @ 2012-02-22 11:51 UTC (permalink / raw)
To: git, gitster
Cc: Michael J Gruber, pclouds, j.sixt,
Zbigniew Jędrzejewski-Szmek
In-Reply-To: <1329911507-10587-1-git-send-email-zbyszek@in.waw.pl>
In preparation for the introduction on the limit of the width of the
graph part, a new test with COLUMNS=40 is added to check that the
environment variable influences diff, show, log, but not format-patch.
A new test is added because limiting the graph part makes COLUMNS=200
stop influencing diff --stat behaviour, which isn't wide enough now.
The old test with COLUMNS=200 is retained to check for regressions.
Signed-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
---
t/t4052-stat-output.sh | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git t/t4052-stat-output.sh t/t4052-stat-output.sh
index c250744..84be8bd 100755
--- t/t4052-stat-output.sh
+++ t/t4052-stat-output.sh
@@ -99,6 +99,25 @@ respects expect200 show --stat
respects expect200 log -1 --stat
EOF
+cat >expect40 <<'EOF'
+ abcd | 1000 ++++++++++++++++++++++++++
+EOF
+
+while read verb expect cmd args
+do
+ test_expect_success "$cmd $verb not enough COLUMNS (big change)" '
+ COLUMNS=40 git $cmd $args >output
+ grep " | " output >actual &&
+ test_cmp "$expect" actual
+ '
+done <<\EOF
+ignores expect80 format-patch -1 --stdout
+respects expect40 diff HEAD^ HEAD --stat
+respects expect40 show --stat
+respects expect40 log -1 --stat
+EOF
+
+
cat >expect <<'EOF'
abcd | 1000 ++++++++++++++++++++++++++
EOF
--
1.7.9.1.355.ge8a9f
^ permalink raw reply related
* [PATCH 7.1/8] diff --stat: use a maximum of 5/8 for the filename part
From: Zbigniew Jędrzejewski-Szmek @ 2012-02-22 11:51 UTC (permalink / raw)
To: git, gitster
Cc: Michael J Gruber, pclouds, j.sixt,
Zbigniew Jędrzejewski-Szmek
In-Reply-To: <4F44D084.7030308@in.waw.pl>
The way that available columns are divided between the filename part
and the graph part is modified to use as many columns as necessary for
the filenames and the rest for the graph.
If there isn't enough columns to print both the filename and the
graph, at least 5/8 of available space is devoted to filenames. On a
standard 80 column terminal, or if not connected to a terminal and
using the default of 80 columns, this gives the same partition as
before.
The effect of this change is visible in the patch to t4052 that fixes a
few tests marked with test_expect_failure.
Signed-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
---
Documentation/diff-options.txt | 14 ++++---
diff.c | 90 ++++++++++++++++++++++++++--------------
t/t4052-stat-output.sh | 14 +++----
3 files changed, 75 insertions(+), 43 deletions(-)
diff --git Documentation/diff-options.txt Documentation/diff-options.txt
index 9ed78c9..29647e5 100644
--- Documentation/diff-options.txt
+++ Documentation/diff-options.txt
@@ -53,13 +53,15 @@ endif::git-format-patch[]
Generate a diff using the "patience diff" algorithm.
--stat[=<width>[,<name-width>[,<count>]]]::
- Generate a diffstat. You can override the default
- output width for 80-column terminal by `--stat=<width>`.
- The width of the filename part can be controlled by
- giving another width to it separated by a comma.
+ Generate a diffstat. By default, as much space as necessary
+ will be used for the filename part, and the rest for
+ the graph part. Maximum width defaults to terminal width,
+ or 80 columns if not connected to a terminal, and can be
+ overriden by `<width>`. The width of the filename part can be
+ limited by giving another width `<name-width>` after a comma.
By giving a third parameter `<count>`, you can limit the
- output to the first `<count>` lines, followed by
- `...` if there are more.
+ output to the first `<count>` lines, followed by `...` if
+ there are more.
+
These parameters can also be set individually with `--stat-width=<width>`,
`--stat-name-width=<name-width>` and `--stat-count=<count>`.
diff --git diff.c diff.c
index 1db46a4..f1c278f 100644
--- diff.c
+++ diff.c
@@ -1375,7 +1375,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
int i, len, add, del, adds = 0, dels = 0;
uintmax_t max_change = 0, max_len = 0;
int total_files = data->nr;
- int width, name_width, count;
+ int width, name_width, graph_width, number_width = 4, count;
const char *reset, *add_c, *del_c;
const char *line_prefix = "";
int extra_shown = 0;
@@ -1389,28 +1389,15 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
line_prefix = msg->buf;
}
- if (options->stat_width == -1)
- width = term_columns();
- else
- width = options->stat_width ? options->stat_width : 80;
- name_width = options->stat_name_width ? options->stat_name_width : 50;
count = options->stat_count ? options->stat_count : data->nr;
- /* Sanity: give at least 5 columns to the graph,
- * but leave at least 10 columns for the name.
- */
- if (width < 25)
- width = 25;
- if (name_width < 10)
- name_width = 10;
- else if (width < name_width + 15)
- name_width = width - 15;
-
- /* Find the longest filename and max number of changes */
reset = diff_get_color_opt(options, DIFF_RESET);
add_c = diff_get_color_opt(options, DIFF_FILE_NEW);
del_c = diff_get_color_opt(options, DIFF_FILE_OLD);
+ /*
+ * Find the longest filename and max number of changes
+ */
for (i = 0; (i < count) && (i < data->nr); i++) {
struct diffstat_file *file = data->files[i];
uintmax_t change = file->added + file->deleted;
@@ -1431,19 +1418,62 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
}
count = i; /* min(count, data->nr) */
- /* Compute the width of the graph part;
- * 10 is for one blank at the beginning of the line plus
- * " | count " between the name and the graph.
+ /*
+ * We have width = stat_width or term_columns() columns total.
+ * We want a maximum of min(max_len, stat_name_width) for the name part.
+ * We also need 1 for " " and 4 + decimal_width(max_change)
+ * for " | NNNN " and one the empty column at the end, altogether
+ * 6 + decimal_width(max_change).
+ *
+ * If there's not enough space, we will use the smaller of
+ * stat_name_width (if set) and 5/8*width for the filename,
+ * and the rest for constant elements + graph part.
+ * (5/8 gives 50 for filename and 30 for the constant parts + graph
+ * for the standard terminal size).
*
- * From here on, name_width is the width of the name area,
- * and width is the width of the graph area.
+ * In other words: stat_width limits the maximum width, and
+ * stat_name_width fixes the maximum width of the filename,
+ * and is also used to divide available columns if there
+ * aren't enough.
*/
- name_width = (name_width < max_len) ? name_width : max_len;
- if (width < (name_width + 10) + max_change)
- width = width - (name_width + 10);
+
+ if (options->stat_width == -1)
+ width = term_columns();
else
- width = max_change;
+ width = options->stat_width ? options->stat_width : 80;
+ /*
+ * Guarantee 3/8*16==6 for the graph part
+ * and 5/8*16==10 for the filename part
+ */
+ if (width < 16 + 6 + number_width)
+ width = 16 + 6 + number_width;
+
+ /*
+ * First assign sizes that are wanted, ignoring available width.
+ */
+ graph_width = max_change;
+ name_width = (options->stat_name_width > 0 &&
+ options->stat_name_width < max_len) ?
+ options->stat_name_width : max_len;
+
+ /*
+ * Adjust adjustable widths not to exceed maximum width
+ */
+ if (name_width + number_width + 6 + graph_width > width) {
+ if (graph_width > width * 3/8 - number_width - 6)
+ graph_width = width * 3/8 - number_width - 6;
+ if (name_width > width - number_width - 6 - graph_width)
+ name_width = width - number_width - 6 - graph_width;
+ else
+ graph_width = width - number_width - 6 - name_width;
+ }
+
+ /*
+ * From here name_width is the width of the name area,
+ * and graph_width is the width of the graph area.
+ * max_change is used to scale graph properly.
+ */
for (i = 0; i < count; i++) {
const char *prefix = "";
char *name = data->files[i]->print_name;
@@ -1499,18 +1529,18 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
adds += add;
dels += del;
- if (width <= max_change) {
+ if (graph_width <= max_change) {
int total = add + del;
- total = scale_linear(add + del, width, max_change);
+ total = scale_linear(add + del, graph_width, max_change);
if (total < 2 && add && del)
/* width >= 2 due to the sanity check */
total = 2;
if (add < del) {
- add = scale_linear(add, width, max_change);
+ add = scale_linear(add, graph_width, max_change);
del = total - add;
} else {
- del = scale_linear(del, width, max_change);
+ del = scale_linear(del, graph_width, max_change);
add = total - del;
}
}
diff --git t/t4052-stat-output.sh t/t4052-stat-output.sh
index 2b4510c..c250744 100755
--- t/t4052-stat-output.sh
+++ t/t4052-stat-output.sh
@@ -28,19 +28,19 @@ EOF
while read cmd args
do
- test_expect_failure "$cmd graph width defaults to 80 columns" '
+ test_expect_success "$cmd graph width defaults to 80 columns" '
git $cmd $args >output &&
grep " | " output >actual &&
test_cmp expect80 actual
'
- test_expect_failure "$cmd --stat=width with long name" '
+ test_expect_success "$cmd --stat=width with long name" '
git $cmd $args --stat=40 >output &&
grep " | " output >actual &&
test_cmp expect40 actual
'
- test_expect_failure "$cmd --stat-width=width with long name" '
+ test_expect_success "$cmd --stat-width=width with long name" '
git $cmd $args --stat-width=40 >output &&
grep " | " output >actual &&
test_cmp expect40 actual
@@ -87,7 +87,7 @@ EOF
while read verb expect cmd args
do
- test_expect_success "$cmd $verb COLUMNS (big change)" '
+ test_expect_success "$cmd $verb too many COLUMNS (big change)" '
COLUMNS=200 git $cmd $args >output
grep " | " output >actual &&
test_cmp "$expect" actual
@@ -130,7 +130,7 @@ test_expect_success 'preparation for long filename tests' '
'
cat >expect <<'EOF'
- ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 +++++
+ ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 ++++++++++++
EOF
while read cmd args
@@ -151,7 +151,7 @@ cat >expect80 <<'EOF'
...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 ++++++++++++++++++++
EOF
cat >expect200 <<'EOF'
- ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
+ aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
EOF
while read verb expect cmd args
do
@@ -178,7 +178,7 @@ test_expect_success 'merge --stat respects COLUMNS (big change)' '
'
cat >expect <<'EOF'
- ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 ++++++++++++++++++++++++++++++++++++++++
+ aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 +++++++++++++++++++++++++++++++++++++++
EOF
test_expect_success 'merge --stat respects COLUMNS (long filename)' '
COLUMNS=100 git merge --stat --no-ff master >output &&
--
1.7.9.1.355.ge8a9f
^ permalink raw reply related
* Re: [PATCHv4] git-p4: add initial support for RCS keywords
From: Pete Wyckoff @ 2012-02-22 12:53 UTC (permalink / raw)
To: Luke Diamand; +Cc: git, Eric Scouten, Junio C Hamano
In-Reply-To: <1329905741-2092-2-git-send-email-luke@diamand.org>
luke@diamand.org wrote on Wed, 22 Feb 2012 10:15 +0000:
> RCS keywords cause problems for git-p4 as perforce always
> expands them (if +k is set) and so when applying the patch,
> git reports that the files have been modified by both sides,
> when in fact they haven't.
>
> This change means that when git-p4 detects a problem applying
> a patch, it will check to see if keyword expansion could be
> the culprit. If it is, it strips the keywords in the p4
> repository so that they match what git is expecting. It then
> has another go at applying the patch.
>
> This behaviour is enabled with a new git-p4 configuration
> option and is off by default.
>
> Improved-by: Pete Wyckoff <pw@padd.com>
> Signed-off-by: Luke Diamand <luke@diamand.org>
Looks brilliant. Ack. Thanks for suffering through N rounds of
review. :)
-- Pete
^ permalink raw reply
* Re: git status: small difference between stating whole repository and small subdirectory
From: Nguyen Thai Ngoc Duy @ 2012-02-22 12:54 UTC (permalink / raw)
To: Junio C Hamano
Cc: Thomas Rast, Jeff King, Piotr Krukowiecki, Git Mailing List
In-Reply-To: <7v7gzfefw1.fsf@alter.siamese.dyndns.org>
On Wed, Feb 22, 2012 at 9:55 AM, Junio C Hamano <gitster@pobox.com> wrote:
> 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?
Simple. You wrote it and I missed it.
On Sat, Feb 18, 2012 at 5:25 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
>
>> That being said, we do have an index extension to store the tree sha1 of
>> whole directories (i.e., we populate it when we write a whole tree or
>> subtree into the index from the object db, and it becomes invalidated
>> when a file becomes modified). This optimization is used by things like
>> "git commit" to avoid having to recreate the same sub-trees over and
>> over when creating tree objects from the index. But we could also use it
>> here to avoid having to even read the sub-tree objects from the object
>> db.
>
> Like b65982b (Optimize "diff-index --cached" using cache-tree, 2009-05-20)
> perhaps?
This optimizes the case when a cached tree matches entirely.I wonder
whether it's faster if we switch to tree-tree diff whenever we find
valid cached trees. If cache-tree is fully valid, "git diff --cached
foo" would be equivalent to "git diff HEAD foo".
I tried "git diff --raw HEAD HEAD~100" (where HEAD was
v3.1-rc1-272-g73e0881 on linux-2.6) and "git diff --cached --raw
HEAD~100" with no cache-tree. The former is a little bit faster than
the latter (177ms vs 275ms). On gentoo-x86, 70k worktree files, it's
4.33s vs 4.45s. But in tree-tree diff we pay high in cold cache case
for loading trees from "HEAD". So no, probably not worth more code
changes. Your optimization is good enough.
--
Duy
^ 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