* question about interactive.singlekey
From: Valentin Haenel @ 2011-09-06 20:50 UTC (permalink / raw)
To: Git-List
Hi,
i have recently installed debian on my laptop. I discovered that the
'interactive.singlekey' setting does not seem to work. On my desktop,
which has been debian for a while it does work. These are both debian
stable and have the exact same version of git v1.7.2.5, but
perhaps slightly different packages of other software installed. Does
the 'interactive.singlekey' setting perhaps need additional libraries
which were forgotten in the debian package dependencies?
Any ideas on where to look are greatly appreciated.
V-
^ permalink raw reply
* Re: [PATCH] git-svn: teach git-svn to populate svn:mergeinfo
From: Eric Wong @ 2011-09-06 20:57 UTC (permalink / raw)
To: Bryan Jacobs; +Cc: git, Sam Vilain
In-Reply-To: <20110902140702.066a4668@robyn.woti.com>
Bryan Jacobs <bjacobs@woti.com> wrote:
> +sub split_merge_info_range {
> + my ($range) = @_;
> + if ($range =~ /(\d+)-(\d+)/o) {
No need for "/o" in regexps unless you have a (constant) variable
expansion in there.
> +sub merge_commit_fail {
> + my ($gs, $linear_refs, $d) = @_;
> + #while (1) {
> + # my $cs = shift @$linear_refs or last;
> + # command_noisy(qw/cherry-pick/, $cs);
> + #}
> + #command_noisy(qw/cherry-pick -m/, '1', $d);
Huh? If there's commented-out code, it must be explained or removed.
> + fatal "Aborted after failed dcommit of merge revision";
> +}
> +++ b/t/t9160-git-svn-mergeinfo-push.sh
> @@ -0,0 +1,97 @@
> +#!/bin/sh
> +#
> +# Copyright (c) 2007, 2009 Sam Vilain
That should be: "Copyright (c) 2011 Brian Jacobs", correct?
> +test_expect_success 'check svn:mergeinfo' '
> + mergeinfo=$(svn_cmd propget svn:mergeinfo "$svnrepo"/branches/svnb1)
> + echo "$mergeinfo"
No need to echo unless you're debugging a test, right?
--
Eric Wong
^ permalink raw reply
* Re: [PATCH] git-svn: teach git-svn to populate svn:mergeinfo
From: Eric Wong @ 2011-09-06 20:45 UTC (permalink / raw)
To: Bryan Jacobs; +Cc: Sam Vilain, git
In-Reply-To: <20110906100003.4c87daba@robyn.woti.com>
Bryan Jacobs <bjacobs@woti.com> wrote:
> On Sat, 3 Sep 2011 08:49:47 +0000
> Eric Wong <normalperson@yhbt.net> wrote:
> > dcommit needs to continually rebase because it's possible somebody
> > else may make a commit to the SVN repo while a git-svn user is
> > dcommiting and cause a conflict the user would need to resolve in the
> > working tree.
> >
> > At least I think that was the reason... There is also the
> > "commit-diff" command in git-svn. It was the precursor to dcommit
> > which requires no changes to the working tree.
>
> Let me see if I've got this right.
>
> The goal here is to commit each x~..x for each x in A..B, aborting if
> the SVN tree is not in state "x~" when the diff arrives.
Yes.
> So why am I seeing files added in changes on alternate
> branches ending up in the working copy when I abort before apply_diff
> is called for the commit which merges them into the present branch?
I don't know.
In my past use of git-svn, I've _always_ stuck with linear changes and
avoided anything non-linear. SVN mergeinfo didn't exist when/where I
used SVN and my only current uses of git-svn is read-only.
Anyhow, I'm willing to accept your change since it doesn't appear to
break anything for existing users and Sam seems to approve.
--
Eric Wong
^ permalink raw reply
* Re: [PATCH] Makefile: abort on shells that do not support ${parameter%word} expansion
From: Brandon Casey @ 2011-09-06 20:30 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Naohiro Aota, git, tarmigan+git, David Barr, Brandon Casey
In-Reply-To: <2i2CfjMHrXZ7dV7ciebqx3PjO-cpw8QIplKjdcx_bGmGt8jgFr3efDXeMJMcn_I9ZH6X71aBdaO7vGiRBQuhbukGEWFJZQuvWtq079u0KYQ@cipher.nrlssc.navy.mil>
On 09/06/2011 02:32 PM, Brandon Casey wrote:
>
> FYI:
> It should be possible to test this patch on a modern system by doing
> something like:
>
> make SHELL_PATH=/bin/false
>
> and you should see something like this:
>
> make: *** [please_set_SHELL_PATH_to_a_more_modern_shell] Error 1
>
> But beware, GNU make 3.81 seems to have a bug which sends it into an
> infinite loop.
Just a clarification, I didn't mean you'd actually be able to test
the patch for correctness, but the above would at least allow you to
stress the code path.
But, with the Makefile in its current form (patch or no patch) the
above still works. Setting SHELL_PATH=/bin/false produces the desired
error message.
There still appears to be a bug in make 3.81 which is triggered when
using an ancient shell, it just manifests itself in a different way
using our current Makefile. Right now, make 3.81 will enter an
infinite loop when it tries to include the GIT-VERSION-FILE. When
something like /bin/sh on Solaris processes the GIT-VERSION-GEN
script, it produces the following incorrect string in the
GIT-VERSION-FILE:
GIT_VERSION = $(expr $(echo $(git describe --match v[0-9]* --abbrev=4 HEAD 2>/dev/null) | sed -e s/-/./g) : v*\(.*\))
which then becomes part of the Makefile when GIT-VERSION-FILE is
included on line 264. GNU make then begins to print the following
to the terminal repeatedly:
GIT_VERSION = $(expr $(echo $(git describe --match v[0-9]* --abbrev=4 HEAD 2>/dev/null) | sed -e s/-/./g) : v*\(.*\))
GIT-VERSION-FILE should really have a dependency on
shell_compatibility_test since it calls GIT-VERSION-GEN which may use
shell features that are not provided by the configured shell. If that
dependency is added so that the GIT-VERSION-FILE rule looks like this:
GIT-VERSION-FILE: shell_compatibility_test FORCE
@$(SHELL_PATH) ./GIT-VERSION-GEN
-include GIT-VERSION-FILE
_then_, we get the behavior I described originally, where
make SHELL_PATH=/bin/false
sends the make process into an infinite loop, with no output to the
terminal.
Either way, with GNU make 3.81, you get an infinite loop when you use
a shell that should trigger our error message.
-Brandon
^ permalink raw reply
* Re: [PATCH 1/2] git svn dcommit: new option --interactive.
From: Eric Wong @ 2011-09-06 20:26 UTC (permalink / raw)
To: Frédéric Heitzmann; +Cc: git, gitster, jaysoffian
In-Reply-To: <1315164113-26539-2-git-send-email-frederic.heitzmann@gmail.com>
Frédéric Heitzmann <frederic.heitzmann@gmail.com> wrote:
> Allow the user to check the patch set before it is commited to SNV. It is then
> possible to accept/discard one patch, accept all, or quit.
>
> This interactive mode is similar with 'git send email' behaviour. However,
> 'git svn dcommit' returns as soon as one patch is discarded.
>
> Part of the code was taken from git-send-email.perl
> Thanks-to: Eric Wong <normalperson@yhbt.net> for the initial idea.
> Signed-off-by: Frédéric Heitzmann <frederic.heitzmann@gmail.com>
I agree with this feature, a few comments inline.
> I would have preferred not duplicating the code snippets taken from
> git-send-email ('ask' function, Term related code, ...) but I preferred not
> to spoil Git.pm with it.
> Any comment on a better way to factor perl code would be appreciated.
We should put this into Git.pm at some point.
(Somebody should refactor git-svn.perl into separate files too... :x)
> Documentation/git-svn.txt | 8 +++++
> git-svn.perl | 71 ++++++++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 78 insertions(+), 1 deletions(-)
Tests and feature should be the same patch
> + return defined $default ? $default : undef
> + unless defined $term->IN and defined fileno($term->IN) and
> + defined $term->OUT and defined fileno($term->OUT);
Things to make life easier for (mainly) C programmers:
* Use C-style "&&" and "||" for conditionals. "and" and "or" are lower
precedence and better used for control flow (see perlop(1) manpage).
* Also, use parentheses for defined(foo) to disambiguate multiple
conditions/statements.
--
Eric Wong
^ permalink raw reply
* Re: [PATCH] date.c: Parse timezone with colon as separator
From: Jeff King @ 2011-09-06 20:24 UTC (permalink / raw)
To: Haitao Li; +Cc: git, gitster
In-Reply-To: <1315320996-1997-1-git-send-email-lihaitao@gmail.com>
On Tue, Sep 06, 2011 at 10:56:36PM +0800, Haitao Li wrote:
> Timezone designators including additional separator (`:`) are ignored.
> Actually zone designators in below formats are all valid according to
> ISO8601:2004, section 4.3.2:
> [+-]hh, [+-]hhmm, [+-]hh:mm
That seems like a sensible list to support, given that it is part of
iso8601 (though I was a little surprised after reading your subject
line, which would probably be better as "support iso8601 timezone
formats").
> ---
> date.c | 14 ++++++++++----
> 1 files changed, 10 insertions(+), 4 deletions(-)
We should probably have new tests, too. I was going to suggest squashing
in the ones below, but your patch doesn't seem to work with the first
one:
diff --git a/t/t0006-date.sh b/t/t0006-date.sh
index f87abb5..9b326cd 100755
--- a/t/t0006-date.sh
+++ b/t/t0006-date.sh
@@ -40,6 +40,8 @@ check_parse 2008-02 bad
check_parse 2008-02-14 bad
check_parse '2008-02-14 20:30:45' '2008-02-14 20:30:45 +0000'
check_parse '2008-02-14 20:30:45 -0500' '2008-02-14 20:30:45 -0500'
+check_parse '2008-02-14 20:30:45 -05' '2008-02-14 20:30:45 -0500'
+check_parse '2008-02-14 20:30:45 -05:00' '2008-02-14 20:30:45 -0500'
check_parse '2008-02-14 20:30:45' '2008-02-14 20:30:45 -0500' EST5
check_approxidate() {
If I am reading your commit message correctly, that should work, right?
-Peff
^ permalink raw reply related
* Re: [PATCH] Makefile: abort on shells that do not support ${parameter%word} expansion
From: Brandon Casey @ 2011-09-06 20:20 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Naohiro Aota, git, tarmigan+git, David Barr, Brandon Casey
In-Reply-To: <7v1uvt8ml6.fsf@alter.siamese.dyndns.org>
On 09/06/2011 03:03 PM, Junio C Hamano wrote:
> Brandon Casey <casey@nrlssc.navy.mil> writes:
>
>> diff --git a/Makefile b/Makefile
>> index 8d6d451..46d9c5d 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -1738,6 +1738,7 @@ endif
>>
>> please_set_SHELL_PATH_to_a_more_modern_shell:
>> @$$(:)
>> + @foo=bar_suffix && test bar = "$${foo%_*}"
>>
>> shell_compatibility_test: please_set_SHELL_PATH_to_a_more_modern_shell
>
> Perhaps
>
> @foo='bar?suffix' && test bar = "$${foo%\?*}"
>
> instead?
Looks right.
Naohiro, can you test? Or someone else with FreeBSD?
make should produce an error message like this:
gmake: *** [please_set_SHELL_PATH_to_a_more_modern_shell] Error 1
-Brandon
^ permalink raw reply
* Re: [PATCH] Makefile: abort on shells that do not support ${parameter%word} expansion
From: Brandon Casey @ 2011-09-06 20:09 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Naohiro Aota, git, tarmigan+git, David Barr, Brandon Casey
In-Reply-To: <7v62l58mp2.fsf@alter.siamese.dyndns.org>
On 09/06/2011 03:01 PM, Junio C Hamano wrote:
> Brandon Casey <casey@nrlssc.navy.mil> writes:
>
>> From: Brandon Casey <drafnel@gmail.com>
>>
>> Add an entry to the please_set_SHELL_PATH_to_a_more_modern_shell target
>> which tests whether the shell supports ${parameter%word} expansion. I
>> assume this one test is enough to indicate whether the shell supports the
>> entire family of prefix and suffix removal syntax:
>>
>> ${parameter%word}
>> ${parameter%%word}
>> ${parameter#word}
>> ${parameter##word}
>>
>> FreeBSD, for one, has a /bin/sh that, apparently, supports $() notation but
>> not the above prefix/suffix removal notation.
>
> My reading of the later part of the thread you are basing the above is
> somewhat different from your diagnosis. The funny seems to happen only
> when there is a backslash-quoted glob special inside double-quotes
> (e.g. "${parameter%\?*}") and the same shell does not seem to be choking
> on many prefix/suffix expansion used in other test scripts.
Ah, I didn't read through closely enough to notice that the above
syntax was not also an issue, as was mentioned in the original email.
Sorry for the noise.
-Brandon
^ permalink raw reply
* Re: [PATCH] Makefile: abort on shells that do not support ${parameter%word} expansion
From: Junio C Hamano @ 2011-09-06 20:03 UTC (permalink / raw)
To: Brandon Casey; +Cc: Naohiro Aota, git, tarmigan+git, David Barr, Brandon Casey
In-Reply-To: <rPnr5AVZRRnklxb_Yaj0gopXRTVCT-tq7iVG-1NoXjOrHWsyuLop-co4qtQjezJ98BaKc0R71r8fMcBOijq9oCOgfBF6ticVk17DwDQzV91bcC719fGSUPDsf40AuoRfgjURcxREkMk@cipher.nrlssc.navy.mil>
Brandon Casey <casey@nrlssc.navy.mil> writes:
> diff --git a/Makefile b/Makefile
> index 8d6d451..46d9c5d 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1738,6 +1738,7 @@ endif
>
> please_set_SHELL_PATH_to_a_more_modern_shell:
> @$$(:)
> + @foo=bar_suffix && test bar = "$${foo%_*}"
>
> shell_compatibility_test: please_set_SHELL_PATH_to_a_more_modern_shell
Perhaps
@foo='bar?suffix' && test bar = "$${foo%\?*}"
instead?
^ permalink raw reply
* Re: [PATCH] Makefile: abort on shells that do not support ${parameter%word} expansion
From: Junio C Hamano @ 2011-09-06 20:01 UTC (permalink / raw)
To: Brandon Casey; +Cc: Naohiro Aota, git, tarmigan+git, David Barr, Brandon Casey
In-Reply-To: <rPnr5AVZRRnklxb_Yaj0gopXRTVCT-tq7iVG-1NoXjOrHWsyuLop-co4qtQjezJ98BaKc0R71r8fMcBOijq9oCOgfBF6ticVk17DwDQzV91bcC719fGSUPDsf40AuoRfgjURcxREkMk@cipher.nrlssc.navy.mil>
Brandon Casey <casey@nrlssc.navy.mil> writes:
> From: Brandon Casey <drafnel@gmail.com>
>
> Add an entry to the please_set_SHELL_PATH_to_a_more_modern_shell target
> which tests whether the shell supports ${parameter%word} expansion. I
> assume this one test is enough to indicate whether the shell supports the
> entire family of prefix and suffix removal syntax:
>
> ${parameter%word}
> ${parameter%%word}
> ${parameter#word}
> ${parameter##word}
>
> FreeBSD, for one, has a /bin/sh that, apparently, supports $() notation but
> not the above prefix/suffix removal notation.
My reading of the later part of the thread you are basing the above is
somewhat different from your diagnosis. The funny seems to happen only
when there is a backslash-quoted glob special inside double-quotes
(e.g. "${parameter%\?*}") and the same shell does not seem to be choking
on many prefix/suffix expansion used in other test scripts.
^ permalink raw reply
* Re: [PATCH 1/3] remove prefix argument from pathspec_prefix
From: Junio C Hamano @ 2011-09-06 19:58 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Clemens Buchacher, git
In-Reply-To: <7vmxeh8pf4.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Is it because we no longer ever return "prefix" we pass in which is a
> pointer to a constant memory region to begin with?
>
> We also didn't free() in the earlier code (because we do not know if it
> can be freed) and leaking xmemdupz() if the function didn't return the
> "prefix", but now you plugged the small leak. Isn't it something you
> should advertise?
Nah, the leak is not necessarily plugged in all callers anyway, so scratch
that part. I've rewritten it like this:
commit 5879f5684cfe8a38326b4ffd078f96e35c68e640
Author: Clemens Buchacher <drizzd@aon.at>
Date: Sun Sep 4 12:41:59 2011 +0200
remove prefix argument from pathspec_prefix
Passing a prefix to a function that is supposed to find the prefix is
strange. And it's really only used if the pathspec is NULL. Make the
callers handle this case instead.
As we are always returning a fresh copy of a string (or NULL), change the
type of the returned value to non-const "char *".
Signed-off-by: Clemens Buchacher <drizzd@aon.at>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
^ permalink raw reply
* Re: [PATCH 3/3] push: old receive-pack does not understand --quiet
From: Clemens Buchacher @ 2011-09-06 19:52 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, tobiasu
In-Reply-To: <7vty8p8s8n.fsf@alter.siamese.dyndns.org>
On Tue, Sep 06, 2011 at 11:01:44AM -0700, Junio C Hamano wrote:
> Clemens Buchacher <drizzd@aon.at> writes:
>
> > Commit 90a6c7d4 (propagate --quiet to send-pack/receive-pack)
> > introduced the --quiet option to receive-pack and made send-pack
> > pass that option. Older versions of receive-pack do not recognize
> > the option, however, and terminate immediately.
> >
> > This change restores backwards compatibility by adding a 'quiet'
> > capability to receive-pack.
>
> Wouldn't this mean that there is no point in adding --quiet command line
> option to the receive-pack command? IOW, shouldn't parts of 90a6c7d
> (propagate --quiet to send-pack/receive-pack, 2011-07-30) be reverted?
>
> At this late stage in the release cycle, I would rather prefer to revert
> the whole commit and leave anything new for the next cycle.
Sure, we can do that. Only then 1.7.6.1 will be broken against all
versions except 1.7.6.1 on the server side. Right now it is only
broken against 1.7.6 and older versions.
But I am ok with it either way. I will have to check how much
resolving needs to be done after the revert.
Clemens
^ permalink raw reply
* Re: [PATCH] Makefile: abort on shells that do not support ${parameter%word} expansion
From: Brandon Casey @ 2011-09-06 19:32 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Naohiro Aota, git, tarmigan+git, David Barr, Brandon Casey
In-Reply-To: <rPnr5AVZRRnklxb_Yaj0gopXRTVCT-tq7iVG-1NoXjOrHWsyuLop-co4qtQjezJ98BaKc0R71r8fMcBOijq9oCOgfBF6ticVk17DwDQzV91bcC719fGSUPDsf40AuoRfgjURcxREkMk@cipher.nrlssc.navy.mil>
FYI:
It should be possible to test this patch on a modern system by doing
something like:
make SHELL_PATH=/bin/false
and you should see something like this:
make: *** [please_set_SHELL_PATH_to_a_more_modern_shell] Error 1
But beware, GNU make 3.81 seems to have a bug which sends it into an
infinite loop.
make 3.80 produces the desired results, as does 3.77 which I have
installed on an old machine. GNU make 3.82 seems to be the latest but
I don't have access to it. If anyone does, I'd appreciate if you
could test.
-Brandon
On 09/06/2011 02:09 PM, Brandon Casey wrote:
> From: Brandon Casey <drafnel@gmail.com>
>
> Add an entry to the please_set_SHELL_PATH_to_a_more_modern_shell target
> which tests whether the shell supports ${parameter%word} expansion. I
> assume this one test is enough to indicate whether the shell supports the
> entire family of prefix and suffix removal syntax:
>
> ${parameter%word}
> ${parameter%%word}
> ${parameter#word}
> ${parameter##word}
>
> FreeBSD, for one, has a /bin/sh that, apparently, supports $() notation but
> not the above prefix/suffix removal notation.
> ---
>
> On 09/05/2011 02:09 AM, Junio C Hamano wrote:
>> Naohiro Aota <naota@elisp.net> writes:
>>
>>> Variable expansions like "${foo#bar}" or "${foo%bar}" doesn't work on
>>> shells like FreeBSD sh and they made the test to fail.
>>
>> Sorry, I do appreciate the effort, but a patch like this takes us in the
>> wrong direction.
>>
>> While we do not allow blatant bashisms like ${parameter:offset:length}
>> (substring expansion), ${parameter/pattern/string} (pattern substitution),
>> "local" variables, "function" noiseword, and shell arrays in our shell
>> scripts, the two kinds of substitution you quoted above are purely POSIX,
>> and our coding guideline does allow them to be used in the scripts.
>
> Perhaps we should add a test for this shell feature.
>
> -Brandon
>
> Makefile | 1 +
> 1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 8d6d451..46d9c5d 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1738,6 +1738,7 @@ endif
>
> please_set_SHELL_PATH_to_a_more_modern_shell:
> @$$(:)
> + @foo=bar_suffix && test bar = "$${foo%_*}"
>
> shell_compatibility_test: please_set_SHELL_PATH_to_a_more_modern_shell
>
^ permalink raw reply
* Re: git-rebase skips automatically no more needed commits
From: Francis Moreau @ 2011-09-06 19:28 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7v1uvta97o.fsf@alter.siamese.dyndns.org>
Hello Junio,
On Tue, Sep 6, 2011 at 7:09 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> Our assumption has always been that it is a notable event that a patch
> that does not get filtered with internal "git cherry" (which culls patches
> that are textually identical to those that are already merged to the
> history you are rebasing onto) becomes totally unneeded and is safe to ask
> for human confirmation in the form of "rebase --skip" than to ignore it
> and give potentially incorrect result silently.
Ok then I think this "git cherry" filtering is not working in my case
since it seems to me that commit that I cherry-picked are not
filtered, please see below.
>
> Obviously you do not find it a notable event for some reason. We would
> need to understand why, and if the reason is sensible, it _might_ make
> sense to allow a user to say "git rebase --ignore-merged" or something
> when starting the rebase.
My use case is the following: I'm maintaining a branch from an
upstream project (the kernel one). While the upstream project follows
its development cycle (including some fixes), my branch is stuck. I
sometime want to includes (or rather backport) some commits that
happened later in the development cycle. To do that I use "git
cherry-pick".
After some period, I'm allowed to rebase to a more recent commit from
the upstream project and this rebase 'cancel' the previous 'git
cherry-pick' I did. But for some reasons, git telling me "nothing
added to commit ...", which is expected in my case, well I think,
hence my question.
Thanks.
--
Francis
^ permalink raw reply
* [PATCH] Makefile: abort on shells that do not support ${parameter%word} expansion
From: Brandon Casey @ 2011-09-06 19:09 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Naohiro Aota, git, tarmigan+git, David Barr, Brandon Casey
In-Reply-To: <7vbouzxy7g.fsf@alter.siamese.dyndns.org>
From: Brandon Casey <drafnel@gmail.com>
Add an entry to the please_set_SHELL_PATH_to_a_more_modern_shell target
which tests whether the shell supports ${parameter%word} expansion. I
assume this one test is enough to indicate whether the shell supports the
entire family of prefix and suffix removal syntax:
${parameter%word}
${parameter%%word}
${parameter#word}
${parameter##word}
FreeBSD, for one, has a /bin/sh that, apparently, supports $() notation but
not the above prefix/suffix removal notation.
---
On 09/05/2011 02:09 AM, Junio C Hamano wrote:
> Naohiro Aota <naota@elisp.net> writes:
>
>> Variable expansions like "${foo#bar}" or "${foo%bar}" doesn't work on
>> shells like FreeBSD sh and they made the test to fail.
>
> Sorry, I do appreciate the effort, but a patch like this takes us in the
> wrong direction.
>
> While we do not allow blatant bashisms like ${parameter:offset:length}
> (substring expansion), ${parameter/pattern/string} (pattern substitution),
> "local" variables, "function" noiseword, and shell arrays in our shell
> scripts, the two kinds of substitution you quoted above are purely POSIX,
> and our coding guideline does allow them to be used in the scripts.
Perhaps we should add a test for this shell feature.
-Brandon
Makefile | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/Makefile b/Makefile
index 8d6d451..46d9c5d 100644
--- a/Makefile
+++ b/Makefile
@@ -1738,6 +1738,7 @@ endif
please_set_SHELL_PATH_to_a_more_modern_shell:
@$$(:)
+ @foo=bar_suffix && test bar = "$${foo%_*}"
shell_compatibility_test: please_set_SHELL_PATH_to_a_more_modern_shell
--
1.7.6.1
^ permalink raw reply related
* Re: [PATCH 1/3] remove prefix argument from pathspec_prefix
From: Junio C Hamano @ 2011-09-06 19:02 UTC (permalink / raw)
To: Clemens Buchacher; +Cc: git
In-Reply-To: <1315132921-26949-2-git-send-email-drizzd@aon.at>
Clemens Buchacher <drizzd@aon.at> writes:
> Passing a prefix to a function that is supposed to find the prefix
> is strange. And it's really only used if the pathspec is NULL. Make
> the callers handle this case instead.
>
> Signed-off-by: Clemens Buchacher <drizzd@aon.at>
While I find the above rationale a reasonable justification for the
removal of a parameter from the function, it does not seem to justify why
the type of the returned value from the function needed to be changed.
Is it because we no longer ever return "prefix" we pass in which is a
pointer to a constant memory region to begin with?
We also didn't free() in the earlier code (because we do not know if it
can be freed) and leaking xmemdupz() if the function didn't return the
"prefix", but now you plugged the small leak. Isn't it something you
should advertise?
> diff --git a/builtin/commit.c b/builtin/commit.c
> index cbc9613..64fe501 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -255,8 +255,9 @@ static int list_paths(struct string_list *list, const char *with_tree,
> m = xcalloc(1, i);
>
> if (with_tree) {
> - const char *max_prefix = pathspec_prefix(prefix, pattern);
> - overlay_tree_on_cache(with_tree, max_prefix);
> + char *max_prefix = pathspec_prefix(pattern);
> + overlay_tree_on_cache(with_tree, max_prefix ? max_prefix : prefix);
> + free(max_prefix);
> }
>
> for (i = 0; i < active_nr; i++) {
> diff --git a/builtin/ls-files.c b/builtin/ls-files.c
> index e8a800d..a54c2a2 100644
> --- a/builtin/ls-files.c
> +++ b/builtin/ls-files.c
> @@ -545,7 +545,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
> strip_trailing_slash_from_submodules();
>
> /* Find common prefix for all pathspec's */
> - max_prefix = pathspec_prefix(prefix, pathspec);
> + max_prefix = pathspec_prefix(pathspec);
> max_prefix_len = max_prefix ? strlen(max_prefix) : 0;
>
> /* Treat unmatching pathspec elements as errors */
> diff --git a/cache.h b/cache.h
> index 607c2ea..0ccc84d 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -444,7 +444,7 @@ extern void set_git_work_tree(const char *tree);
> #define ALTERNATE_DB_ENVIRONMENT "GIT_ALTERNATE_OBJECT_DIRECTORIES"
>
> extern const char **get_pathspec(const char *prefix, const char **pathspec);
> -extern const char *pathspec_prefix(const char *prefix, const char **pathspec);
> +extern char *pathspec_prefix(const char **pathspec);
> extern void setup_work_tree(void);
> extern const char *setup_git_directory_gently(int *);
> extern const char *setup_git_directory(void);
> diff --git a/setup.c b/setup.c
> index 27c1d47..0906790 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -236,13 +236,13 @@ const char **get_pathspec(const char *prefix, const char **pathspec)
> return pathspec;
> }
>
> -const char *pathspec_prefix(const char *prefix, const char **pathspec)
> +char *pathspec_prefix(const char **pathspec)
> {
> const char **p, *n, *prev;
> unsigned long max;
>
> if (!pathspec)
> - return prefix ? xmemdupz(prefix, strlen(prefix)) : NULL;
> + return NULL;
>
> prev = NULL;
> max = PATH_MAX;
^ permalink raw reply
* Re: [PATCH 3/3] push: old receive-pack does not understand --quiet
From: Junio C Hamano @ 2011-09-06 18:01 UTC (permalink / raw)
To: Clemens Buchacher; +Cc: git, tobiasu
In-Reply-To: <1315067656-2846-4-git-send-email-drizzd@aon.at>
Clemens Buchacher <drizzd@aon.at> writes:
> Commit 90a6c7d4 (propagate --quiet to send-pack/receive-pack)
> introduced the --quiet option to receive-pack and made send-pack
> pass that option. Older versions of receive-pack do not recognize
> the option, however, and terminate immediately.
>
> This change restores backwards compatibility by adding a 'quiet'
> capability to receive-pack.
Wouldn't this mean that there is no point in adding --quiet command line
option to the receive-pack command? IOW, shouldn't parts of 90a6c7d
(propagate --quiet to send-pack/receive-pack, 2011-07-30) be reverted?
At this late stage in the release cycle, I would rather prefer to revert
the whole commit and leave anything new for the next cycle.
^ permalink raw reply
* Re: [RFC/PATCH git] http: avoid empty error messages for some curl errors
From: Daniel Stenberg @ 2011-09-06 18:00 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jonathan Nieder, git, Tay Ray Chuan
In-Reply-To: <7v62l5a9ke.fsf@alter.siamese.dyndns.org>
On Tue, 6 Sep 2011, Junio C Hamano wrote:
>>
>> For the record, in libcurl we work on providing "extra" information in
>> the error buffer when there is additional info to provide that would
>> help. In some cases we deem there isn't (or we just to provide any)
>> and then the generic error message is good enough and could indeed be
>> used like this...
>
> Sorry if I am a bit slow but are you saying:
>
> (1) we provide "extra" but your patch is not using it which is bad?
> (2) the above is Ok but there are better ways to do it?
> (3) something else?
Sorry for being unclear.
I was trying to explain why the proposed patch makes sense and that it will
continue to make sense even if in future libcurl versions it would start
returning error messages for errors where currently it returns none.
Thus, I am in favour of the general idea of the patch - I have no comment for
the exact implementation as I haven't checked the details.
--
/ daniel.haxx.se
^ permalink raw reply
* Re: [PATCH v2] Make use of git status when autocompleting git add, rm, checkout --, and reset HEAD
From: SZEDER Gábor @ 2011-09-06 17:43 UTC (permalink / raw)
To: Ron Panduwana
Cc: git, Shawn O. Pearce, Junio C Hamano, Lee Marlow, Thomas Rast
In-Reply-To: <1314740583-14567-1-git-send-email-panduwana@gmail.com>
Hi,
On Wed, Aug 31, 2011 at 04:43:03AM +0700, Ron Panduwana wrote:
> Signed-off-by: Ron Panduwana <panduwana@gmail.com>
I agree with Junio that some explanation would be necessary here.
I'm not sure this change is in general a good idea. I can imagine
that it would be useful for e.g. java projects, where you are forced
to have deep directory structures, but it will surely cause surprises
and confusion for users who trained their fingers to quickly enter
long paths with a few keystrokes and completion. Furthermore, it
seems to cause considerable performance penalty, because it will fork
three processes and a subshell to connect them with a pipe for such a
"simple" thing as file name completion. And one of those processes is
git status, which alone can take a while, especially for large
repositories, and as a side effect it refreshes the index (but that is
probably harmless).
> ---
>
> On Fri, Aug 19, 2011 at 5:10 PM, Thomas Rast <trast@student.ethz.ch> wrote:
> > Some thoughts:
> >
> > * running git-status for . has some issues: it doesn't work in the
> > Â case of
> >
> > Â Â cd subdir
> > Â Â git add ../some/file[TAB]
> >
> > Â It's also inefficient if you are at the top level and
> >
> > Â Â git add path/to/file/a/few/levels/down[TAB]
> >
> > Â since it wouldn't actually have to look for untracked files in the
> > Â entire repo.
>
> Fixed by running git-status for $cur if $cur is a directory. Otherwise run on .
That won't do. Imaginge you want to add ../some/file, and therefore
you do 'git add ../so<TAB>'. In this case $cur will hold '../so',
which is not a directory (or at least not the directory you are
looking for).
> > * -uall is not required unless you are looking for untracked files.
> > Â For the other commands you could speed up completion by passing
> > Â -uno instead.
>
> Fixed by adding second parameter to __git_files_having_status
>
>
> contrib/completion/git-completion.bash | 84 ++++++++++++++++++++-----------
> 1 files changed, 54 insertions(+), 30 deletions(-)
>
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 8648a36..9d44501 100755
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -1010,6 +1010,16 @@ __git_has_doubledash ()
> return 1
> }
>
> +# __git_files_having_status requires 2 arguments
> +__git_files_having_status ()
> +{
> + local dir="."
> + if [ -d "$cur" ]; then
> + dir="$cur"
> + fi
Please use tabs for indentation.
> + echo "$(git status $2 -s "$dir" 2>/dev/null | egrep "^$1" | cut -c4-)"
Echoing a command substitution is unnecessary.
> +}
> +
> __git_whitespacelist="nowarn warn error error-all fix"
>
> _git_am ()
> @@ -1058,17 +1068,17 @@ _git_apply ()
>
> _git_add ()
> {
> - __git_has_doubledash && return
> -
> - case "$cur" in
> - --*)
> - __gitcomp "
> - --interactive --refresh --patch --update --dry-run
> - --ignore-errors --intent-to-add
> - "
> - return
> - esac
> - COMPREPLY=()
> + if ! __git_has_doubledash; then
> + case "$cur" in
> + --*)
> + __gitcomp "
> + --interactive --refresh --patch --update --dry-run
> + --ignore-errors --intent-to-add
> + "
> + return
> + esac
> + fi
> + __gitcomp "$(__git_files_having_status "(.[MAU]|UD|\?\?)" -uall)"
Please do this and the others the other way around, as you did in
_git_checkout(), i.e.
if __git_has_doubledash ; then
__gitcomp "$(__git_files_having_status ...)"
return
fi
case "$cur" in
[...]
This will make the patch shorter and easier to review, it won't
increase the indentation unnecesarily, and it will be friendlier to
the users running 'git blame' in the future.
> }
>
> _git_archive ()
> @@ -1171,7 +1181,12 @@ _git_bundle ()
>
> _git_checkout ()
> {
> - __git_has_doubledash && return
> + if __git_has_doubledash; then
> + if [[ ${words[2]} = "--" ]]; then
> + __gitcomp "$(__git_files_having_status ".[MD]" -uno)"
> + fi
> + return
> + fi
>
> case "$cur" in
> --conflict=*)
> @@ -1469,7 +1484,7 @@ _git_help ()
> __gitcomp "$__git_all_commands $(__git_aliases)
> attributes cli core-tutorial cvs-migration
> diffcore gitk glossary hooks ignore modules
> - namespaces repository-layout tutorial tutorial-2
> + repository-layout tutorial tutorial-2
That's an independent change; I don't think you want to remove
namespaces here.
> workflows
> "
> }
> @@ -2313,14 +2328,18 @@ _git_replace ()
>
> _git_reset ()
> {
> - __git_has_doubledash && return
> -
> - case "$cur" in
> - --*)
> - __gitcomp "--merge --mixed --hard --soft --patch"
> + if ! __git_has_doubledash; then
> + case "$cur" in
> + --*)
> + __gitcomp "--merge --mixed --hard --soft --patch"
> + return
> + ;;
> + esac
> + fi
> + if [[ ${words[2]} = "HEAD" ]]; then
> + __gitcomp "$(__git_files_having_status "[ADM]." -uno)"
> return
> - ;;
> - esac
> + fi
> __gitcomp "$(__git_refs)"
> }
>
> @@ -2337,15 +2356,20 @@ _git_revert ()
>
> _git_rm ()
> {
> - __git_has_doubledash && return
> -
> - case "$cur" in
> - --*)
> - __gitcomp "--cached --dry-run --ignore-unmatch --quiet"
> - return
> - ;;
> - esac
> - COMPREPLY=()
> + if ! __git_has_doubledash; then
> + case "$cur" in
> + --*)
> + __gitcomp "--cached --dry-run --ignore-unmatch --quiet"
> + return
> + ;;
> + esac
> + fi
> + # check if --cached was specified
> + if [ "$(__git_find_on_cmdline "--cached")" ]; then
> + COMPREPLY=()
> + else
> + __gitcomp "$(__git_files_having_status "(.D|DU|UA)" -uno)"
'git rm' can be used to delete any tracked files, but this limits
completion to only those files that are already deleted from the work
tree or are unmerged.
> + fi
> }
>
> _git_shortlog ()
> @@ -2640,7 +2664,6 @@ _git ()
> --exec-path
> --html-path
> --work-tree=
> - --namespace=
Independent change.
> --help
> "
> ;;
> @@ -2737,3 +2760,4 @@ else
> shopt "$@"
> }
> fi
> +
Superfluous new line.
Best,
Gábor
^ permalink raw reply
* Re: git-rebase skips automatically no more needed commits
From: Junio C Hamano @ 2011-09-06 17:09 UTC (permalink / raw)
To: Francis Moreau; +Cc: git
In-Reply-To: <CAC9WiBg9+30NjO+NKXVdBiWjR-HU2689JQqVY7Rk5+DM7MiNBg@mail.gmail.com>
Francis Moreau <francis.moro@gmail.com> writes:
> When rebasing my current branch onto the last master one, there're
> sometimes some commits which doesn't add anything anymore.
>
> Currently git-rebase produces the following message:
>
> nothing added to commit but untracked files present (use "git add" to track)
>
> Would it be possible to add a new option to this command so it simply
> skip the unneeded commit ?
Our assumption has always been that it is a notable event that a patch
that does not get filtered with internal "git cherry" (which culls patches
that are textually identical to those that are already merged to the
history you are rebasing onto) becomes totally unneeded and is safe to ask
for human confirmation in the form of "rebase --skip" than to ignore it
and give potentially incorrect result silently.
Obviously you do not find it a notable event for some reason. We would
need to understand why, and if the reason is sensible, it _might_ make
sense to allow a user to say "git rebase --ignore-merged" or something
when starting the rebase.
^ permalink raw reply
* Re: [RFC/PATCH git] http: avoid empty error messages for some curl errors
From: Junio C Hamano @ 2011-09-06 17:02 UTC (permalink / raw)
To: Daniel Stenberg; +Cc: Jonathan Nieder, git, Tay Ray Chuan
In-Reply-To: <alpine.DEB.2.00.1109061023010.3841@tvnag.unkk.fr>
Daniel Stenberg <daniel@haxx.se> writes:
> On Mon, 5 Sep 2011, Jonathan Nieder wrote:
>
>> + if (!curl_errorstr[0])
>> + strlcpy(curl_errorstr,
>> + curl_easy_strerror(results.curl_result),
>> + sizeof(curl_errorstr));
>
> (as libcurl hacker)
>
> For the record, in libcurl we work on providing "extra" information in
> the error buffer when there is additional info to provide that would
> help. In some cases we deem there isn't (or we just to provide any)
> and then the generic error message is good enough and could indeed be
> used like this...
Sorry if I am a bit slow but are you saying:
(1) we provide "extra" but your patch is not using it which is bad?
(2) the above is Ok but there are better ways to do it?
(3) something else?
^ permalink raw reply
* Re: [PATCH v2 1/5] sha1_file cleanup: remove redundant variable check
From: Junio C Hamano @ 2011-09-06 16:59 UTC (permalink / raw)
To: Wang Hui; +Cc: git, tali
In-Reply-To: <1315304645-12009-2-git-send-email-Hui.Wang@windriver.com>
Wang Hui <Hui.Wang@windriver.com> writes:
> From: Hui Wang <Hui.Wang@windriver.com>
>
> This variable check is always true, so it is redundant and need to be
> removed.
>
> We can't remove the init value for this variable, since removing
> it will introduce building warning:
> 'base_len' may be used uninitialized in this function.
If we are into cleaning things up, we should instead notice and say "yuck"
to the repeated "is entry is absolute and relative base is given" check.
Wouldn't something like this makes things easier to follow and also avoids
the "when does the path normalized and made absolute" issue?
Completely untested and I may have off-by-one errors and such, but I think
you would get the idea...
sha1_file.c | 29 ++++++++++++-----------------
1 files changed, 12 insertions(+), 17 deletions(-)
diff --git a/sha1_file.c b/sha1_file.c
index e002056..26aa3be 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -248,27 +248,22 @@ static int link_alt_odb_entry(const char * entry, int len, const char * relative
const char *objdir = get_object_directory();
struct alternate_object_database *ent;
struct alternate_object_database *alt;
- /* 43 = 40-byte + 2 '/' + terminating NUL */
- int pfxlen = len;
- int entlen = pfxlen + 43;
- int base_len = -1;
+ int pfxlen, entlen;
+ struct strbuf pathbuf = STRBUF_INIT;
if (!is_absolute_path(entry) && relative_base) {
- /* Relative alt-odb */
- if (base_len < 0)
- base_len = strlen(relative_base) + 1;
- entlen += base_len;
- pfxlen += base_len;
+ strbuf_addstr(&pathbuf, relative_base);
+ strbuf_addch(&pathbuf, '/');
}
- ent = xmalloc(sizeof(*ent) + entlen);
+ strbuf_add(&pathbuf, entry, len);
+ normalize_path_copy(pathbuf.buf, pathbuf.buf);
+ strbuf_setlen(&pathbuf, strlen(pathbuf.buf));
- if (!is_absolute_path(entry) && relative_base) {
- memcpy(ent->base, relative_base, base_len - 1);
- ent->base[base_len - 1] = '/';
- memcpy(ent->base + base_len, entry, len);
- }
- else
- memcpy(ent->base, entry, pfxlen);
+ pfxlen = pathbuf.len;
+ entlen = pfxlen + 43; /* '/' + 2 hex + '/' + 38 hex + NUL */
+ ent = xmalloc(sizeof(*ent) + entlen);
+ memcpy(ent->base, pathbuf.buf, pfxlen);
+ strbuf_release(&pathbuf);
ent->name = ent->base + pfxlen + 1;
ent->base[pfxlen + 3] = '/';
^ permalink raw reply related
* Re: [BUG] git bisect start fails when stale bisect data is left behind
From: Junio C Hamano @ 2011-09-06 16:38 UTC (permalink / raw)
To: Christian Couder; +Cc: Joel Kaasinen, git
In-Reply-To: <CAP8UFD1h059dOyjszcP-qFauyho78c0RHBMQsGOPFgzZtp+7vg@mail.gmail.com>
Christian Couder <christian.couder@gmail.com> writes:
>> How to reproduce:
>> $ echo foo > .git/BISECT_START
>> $ git bisect start HEAD HEAD^
>>
>> Fails with "fatal: invalid reference:" on git 1.7.6.
>
> Yeah, it looks like a very old behavior.
> I'd suggest a simple improvement in the error message like this:
>
> diff --git a/git-bisect.sh b/git-bisect.sh
> index c21e33c..bd7155b 100755
> --- a/git-bisect.sh
> +++ b/git-bisect.sh
> @@ -67,7 +67,8 @@ bisect_start() {
> then
> # Reset to the rev from where we started.
> start_head=$(cat "$GIT_DIR/BISECT_START")
> - git checkout "$start_head" -- || exit
> + git checkout "$start_head" -- ||
> + die "Could not checkout previous start point
> '$start_head'. Try 'git bisect reset <branch>' first."
I do not necessarily think this is a bug to begin with --- the user had a
bad state, and bisect stopped without doing further damage.
The real question is what the sensible suggestion/advice the new message
should give. It would have to involve bisect reset in the end to get rid
of the stale bisect state, but wouldn't the user potentially lose some
other state if s/he blindly followed the die message's suggestion and ran
"bisect reset"?
^ permalink raw reply
* Re: [PATCH v2 3/5] sha1_file: improve directories comparison method
From: Junio C Hamano @ 2011-09-06 16:32 UTC (permalink / raw)
To: Wang Hui; +Cc: git, tali
In-Reply-To: <1315304645-12009-4-git-send-email-Hui.Wang@windriver.com>
Wang Hui <Hui.Wang@windriver.com> writes:
> From: Hui Wang <Hui.Wang@windriver.com>
>
> In the past, to check if two directory paths are same, we use memcmp()
> to directly compare their path strings, this method can't get an
> accurate result if paths include ".." or "." or redundant slash, e.g.
> current dir is /, "/a/b/c", "/a/b//c/d/e/../.." and "./a/b/f/../c"
> should be the same dir, but current method will identify they are
> different.
>
> Now add a global function is_same_directory() to replace the old
> memcmp() method, this function will change two input paths to real
> path first, then normalized them and compare them.
I do not like this patch _at all_. While it may result in correct result
if you _always_ make it absolute before comparing two entities, if you
will be storing the normalized result after running the comparison anyway,
and if you are comparing against the existing and supposedly already
normalized entities with a new candidate, why would anybody sane would
want to keep paying for the normalization cost at such a low level?
IOW, you are proposing to do:
given a new candidate;
for existing entities:
normalize existing
normalize candiate
compare the above two
if they are equal:
ignore
if no match found
add the normalized candidate to the list
Wouldn't it make much more sense to do this:
given a new candidate;
normalize it
for existing entities:
compare existing and normalized candidate
there is no point in normalizing the existing one!
if they are equal:
ignore
if no match found
add the normalized candidate to the list
^ permalink raw reply
* Re: [PATCH v2 2/5] sha1_file: remove a buggy value setting
From: Junio C Hamano @ 2011-09-06 16:26 UTC (permalink / raw)
To: Wang Hui; +Cc: git, tali
In-Reply-To: <1315304645-12009-3-git-send-email-Hui.Wang@windriver.com>
Wang Hui <Hui.Wang@windriver.com> writes:
> From: Hui Wang <Hui.Wang@windriver.com>
>
> The ent->base[] is a character array, it has pfxlen characters from
> position 0 to (pfxlen-1) to contain an alt object dir name, the
> position pfxlen should be the string terminating character '\0' and
> is deliberately set to '\0' at the previous code line. The position
> (pfxlen+1) is given to ent->name.
Correct. Do you understand why?
We temporarily NUL terminate the ent->base[] so that we can give it to
is_directory() to see if that is a directory, but the invariants for a
alternate_object_database instance after it is properly initialized by
this function are to have:
- the directory name followed by a slash in the base[] array;
- the name pointer pointing at one byte beyond the slash;
- name[2] filled with a slash; and
- name[41] terminated with NUL.
Later, has_loose_object_nonlocal() calls fill_sha1_path() with the name
pointer to fill name[0..1, 3..40] with the hexadecimal representation of
the object name, which would result in base[] array to have the pathname
for a loose object found in that alternate. The same thing happens in
open_sha1_file() to read from a loose object in an alternate.
And you are breaking one of the above invariants by removing that slash
after the directory name. These callers of fill_sha1_path() will see the
directory name, your NUL, two hex, slash, and 38 hex in base[].
How would the code even work with your patch?
^ 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