* 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: 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] 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: [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: [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] 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: [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: 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 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 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] 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: [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
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