* [PATCH 1/2] format-patch: document and test --reroll-count
From: Junio C Hamano @ 2013-01-02 22:42 UTC (permalink / raw)
To: git
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* This comes on top of the 7 patches that have been cooking on 'pu'
(http://thread.gmane.org/gmane.comp.version-control.git/212036),
and I am planning to squash this to 7/7 that adds --reroll-count
option to the code. This is sent as a separate patch to keep
reviewing easier.
Documentation/git-format-patch.txt | 10 +++++++++-
t/t4014-format-patch.sh | 8 ++++++++
2 files changed, 17 insertions(+), 1 deletion(-)
diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
index 6d43f56..736d8bf 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -18,7 +18,7 @@ SYNOPSIS
[--start-number <n>] [--numbered-files]
[--in-reply-to=Message-Id] [--suffix=.<sfx>]
[--ignore-if-in-upstream]
- [--subject-prefix=Subject-Prefix]
+ [--subject-prefix=Subject-Prefix] [--reroll-count <n>]
[--to=<email>] [--cc=<email>]
[--cover-letter] [--quiet]
[<common diff options>]
@@ -166,6 +166,14 @@ will want to ensure that threading is disabled for `git send-email`.
allows for useful naming of a patch series, and can be
combined with the `--numbered` option.
+--reroll-count=<n>::
+ Mark the series as the <n>-th iteration of the topic. The
+ output filenames have `v<n>` pretended to them, and the
+ subject prefix ("PATCH" by default, but configurable via the
+ `--subject-prefix` option) has ` v<n>` appended to it. E.g.
+ `--reroll-count=4` may produce `v4-0001-add-makefile.patch`
+ file that has "Subject: [PATCH v4 1/20] Add makefile" in it.
+
--to=<email>::
Add a `To:` header to the email headers. This is in addition
to any configured headers, and may be used multiple times.
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 959aa26..0ff9958 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -237,6 +237,14 @@ test_expect_success 'multiple files' '
ls patches/0001-Side-changes-1.patch patches/0002-Side-changes-2.patch patches/0003-Side-changes-3-with-n-backslash-n-in-it.patch
'
+test_expect_success 'reroll count' '
+ rm -fr patches &&
+ git format-patch -o patches --cover-letter --reroll-count 4 master..side >list &&
+ ! grep -v "^patches/v4-000[0-3]-" list &&
+ sed -n -e "/^Subject: /p" $(cat list) >subjects &&
+ ! grep -v "^Subject: \[PATCH v4 [0-3]/3\] " subjects
+'
+
check_threading () {
expect="$1" &&
shift &&
--
1.8.0.9.g5e84801
^ permalink raw reply related
* [PATCH 2/2] format-patch: give --reroll-count a short synonym -v
From: Junio C Hamano @ 2013-01-02 22:42 UTC (permalink / raw)
To: git
In-Reply-To: <1357166525-12188-1-git-send-email-gitster@pobox.com>
Accept "-v" as a synonym to "--reroll-count", so that users can say
"git format-patch -v4 master", instead of having to fully spell it
out as "git format-patch --reroll-count=4 master".
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* As I do not think of a reason why users would want to tell the
command to be "verbose", I think it may be OK to squat on the
short and sweet single letter option, but I do not mind dropping
it.
Documentation/git-format-patch.txt | 3 ++-
builtin/log.c | 2 +-
t/t4014-format-patch.sh | 8 ++++++++
3 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
index 736d8bf..ae3212e 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -18,7 +18,7 @@ SYNOPSIS
[--start-number <n>] [--numbered-files]
[--in-reply-to=Message-Id] [--suffix=.<sfx>]
[--ignore-if-in-upstream]
- [--subject-prefix=Subject-Prefix] [--reroll-count <n>]
+ [--subject-prefix=Subject-Prefix] [(--reroll-count|-v) <n>]
[--to=<email>] [--cc=<email>]
[--cover-letter] [--quiet]
[<common diff options>]
@@ -166,6 +166,7 @@ will want to ensure that threading is disabled for `git send-email`.
allows for useful naming of a patch series, and can be
combined with the `--numbered` option.
+-v <n>::
--reroll-count=<n>::
Mark the series as the <n>-th iteration of the topic. The
output filenames have `v<n>` pretended to them, and the
diff --git a/builtin/log.c b/builtin/log.c
index e101498..08e8a9d 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1081,7 +1081,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
N_("use <sfx> instead of '.patch'")),
OPT_INTEGER(0, "start-number", &start_number,
N_("start numbering patches at <n> instead of 1")),
- OPT_INTEGER(0, "reroll-count", &reroll_count,
+ OPT_INTEGER('v', "reroll-count", &reroll_count,
N_("mark the series as Nth re-roll")),
{ OPTION_CALLBACK, 0, "subject-prefix", &rev, N_("prefix"),
N_("Use [<prefix>] instead of [PATCH]"),
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 0ff9958..03b8e51 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -245,6 +245,14 @@ test_expect_success 'reroll count' '
! grep -v "^Subject: \[PATCH v4 [0-3]/3\] " subjects
'
+test_expect_success 'reroll count (-v)' '
+ rm -fr patches &&
+ git format-patch -o patches --cover-letter -v 4 master..side >list &&
+ ! grep -v "^patches/v4-000[0-3]-" list &&
+ sed -n -e "/^Subject: /p" $(cat list) >subjects &&
+ ! grep -v "^Subject: \[PATCH v4 [0-3]/3\] " subjects
+'
+
check_threading () {
expect="$1" &&
shift &&
--
1.8.0.9.g5e84801
^ permalink raw reply related
* Re: Missing Refs after Garbage Collection
From: Martin Fick @ 2013-01-02 22:43 UTC (permalink / raw)
To: Earl Gresh; +Cc: git
In-Reply-To: <C0A16EC8-D05A-41D0-BF2A-34BF3B1B839E@codeaurora.org>
On Friday, December 21, 2012 06:41:43 pm Earl Gresh wrote:
> Hi-
>
> I have observed that after running GC, one particular git
> repository ended up with some missing refs in the
> refs/changes/* namespace the Gerrit uses for storing
> patch sets. The refs were valid and should not have been
> pruned. Concerned about loosing data, GC is still enabled
> but ref packing is turned off. Now the number of refs has
> grown to the point that it's causing performance problems
> when cloning the project.
>
> Is anyone familiar with git gc deleting valid references?
> I'm running git version 1.7.8. Have there been any
> patches in later git releases that might address this
> issue ( if it is a git problem )?
When Earl was testing ref-packing a few months ago, that the
refs in question where reported invalid by git show-ref:
git show-ref 2>&1 |grep refs/changes/45/129345/1
error: refs/changes/45/129345/1 does not point to a valid
object!
But we could trace the refs manually to git show-object just
fine. But oddly enough, when using git show-ref with the -v,
the error above would not be spit out.
So, my guess is that something during the repack was
following the same code path that git show-ref (without the -
v) was following and determining that the ref was invalid and
therefor it was not able to add it to the new packfile, but
yet perhaps it was still being added to the prune-list and
thus getting pruned? Is this possible somehow?
Looking at handle_one_ref() I can't see how. The fprintf()
happens before the ref is added to the prune list and is
unconditional. I am grasping here, but what if the sha1
passed into handle_one_ref() somehow gets set incorrectly to
000...? Would it then basically get written to the packed-
ref file as 000... (deleted), but then still get added to the
prune list? You might say "but then it wouldn't get pruned
since the loose ref doesn't match 000..., but if the logic
which checks this matching makes the same error reading the
sha1 and thinks it is 000... it might then get pruned?
-Martin
^ permalink raw reply
* Re: [PATCH V2] t9810: Do not use sed -i
From: Pete Wyckoff @ 2013-01-02 22:44 UTC (permalink / raw)
To: Torsten Bögershausen; +Cc: git
In-Reply-To: <201301020020.38535.tboegi@web.de>
tboegi@web.de wrote on Wed, 02 Jan 2013 00:20 +0100:
> sed -i is not portable on all systems.
> Use sed with different input and output files.
> Utilize a tmp file whenever needed
>
> Added missing && at 2 places
>
> Signed-off-by: Torsten Bögershausen <tboegi@web.de>
One more teensy fix is needed in this hunk, following Junio's
comment about redirections going at the end:
> @@ -139,7 +140,8 @@ test_expect_success 'scrub ko files differently' '
> cd "$git" &&
> git config git-p4.skipSubmitEdit true &&
> git config git-p4.attemptRCSCleanup true &&
> - sed -i "s/^line4/line4 edit/" fileko &&
> + sed <fileko "s/^line4/line4 edit/" >fileko.tmp &&
> + mv -f fileko.tmp fileko &&
> git commit -m "fileko line4 edit" fileko &&
> git p4 submit &&
> scrub_ko_check fileko &&
I checked that the test still works with your changes.
Thanks for the cleanup!
Acked-by: Pete Wyckoff <pw@padd.com>
-- Pete
^ permalink raw reply
* Re: [PATCH V2] t9810: Do not use sed -i
From: Junio C Hamano @ 2013-01-02 23:06 UTC (permalink / raw)
To: Pete Wyckoff; +Cc: Torsten Bögershausen, git
In-Reply-To: <20130102224446.GA12363@padd.com>
Pete Wyckoff <pw@padd.com> writes:
> tboegi@web.de wrote on Wed, 02 Jan 2013 00:20 +0100:
>> sed -i is not portable on all systems.
>> Use sed with different input and output files.
>> Utilize a tmp file whenever needed
>>
>> Added missing && at 2 places
>>
>> Signed-off-by: Torsten Bögershausen <tboegi@web.de>
>
> One more teensy fix is needed in this hunk, following Junio's
> comment about redirections going at the end:
I've already pushed out the previous one on 'pu' after fixing it up
like this.
Thanks. Anything I missed?
Date: Tue, 1 Jan 2013 22:40:37 +0100
Subject: [PATCH] t9810: Do not use sed -i
sed -i is not portable on all systems. Use sed with different input
and output files. Utilize a tmp file whenever needed.
Signed-off-by: Torsten Bögershausen <tboegi@web.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
t/t9810-git-p4-rcs.sh | 24 ++++++++++++++----------
1 file changed, 14 insertions(+), 10 deletions(-)
diff --git a/t/t9810-git-p4-rcs.sh b/t/t9810-git-p4-rcs.sh
index 0c2fc3e..34fbc90 100755
--- a/t/t9810-git-p4-rcs.sh
+++ b/t/t9810-git-p4-rcs.sh
@@ -26,10 +26,8 @@ test_expect_success 'init depot' '
line7
line8
EOF
- cp filek fileko &&
- sed -i "s/Revision/Revision: do not scrub me/" fileko
- cp fileko file_text &&
- sed -i "s/Id/Id: do not scrub me/" file_text
+ sed "s/Revision/Revision: do not scrub me/" <filek >fileko &&
+ sed "s/Id/Id: do not scrub me/" <fileko >file_text &&
p4 add -t text+k filek &&
p4 submit -d "filek" &&
p4 add -t text+ko fileko &&
@@ -88,7 +86,8 @@ test_expect_success 'edit far away from RCS lines' '
(
cd "$git" &&
git config git-p4.skipSubmitEdit true &&
- sed -i "s/^line7/line7 edit/" filek &&
+ sed "s/^line7/line7 edit/" <filek >filek.tmp &&
+ mv -f filek.tmp filek &&
git commit -m "filek line7 edit" filek &&
git p4 submit &&
scrub_k_check filek
@@ -105,7 +104,8 @@ test_expect_success 'edit near RCS lines' '
cd "$git" &&
git config git-p4.skipSubmitEdit true &&
git config git-p4.attemptRCSCleanup true &&
- sed -i "s/^line4/line4 edit/" filek &&
+ sed "s/^line4/line4 edit/" <filek >filek.tmp &&
+ mv -f filek.tmp filek &&
git commit -m "filek line4 edit" filek &&
git p4 submit &&
scrub_k_check filek
@@ -122,7 +122,8 @@ test_expect_success 'edit keyword lines' '
cd "$git" &&
git config git-p4.skipSubmitEdit true &&
git config git-p4.attemptRCSCleanup true &&
- sed -i "/Revision/d" filek &&
+ sed "/Revision/d" <filek >filek.tmp &&
+ mv -f filek.tmp filek &&
git commit -m "filek remove Revision line" filek &&
git p4 submit &&
scrub_k_check filek
@@ -139,7 +140,8 @@ test_expect_success 'scrub ko files differently' '
cd "$git" &&
git config git-p4.skipSubmitEdit true &&
git config git-p4.attemptRCSCleanup true &&
- sed -i "s/^line4/line4 edit/" fileko &&
+ sed "s/^line4/line4 edit/" <fileko >fileko.tmp &&
+ mv -f fileko.tmp fileko &&
git commit -m "fileko line4 edit" fileko &&
git p4 submit &&
scrub_ko_check fileko &&
@@ -189,12 +191,14 @@ test_expect_success 'do not scrub plain text' '
cd "$git" &&
git config git-p4.skipSubmitEdit true &&
git config git-p4.attemptRCSCleanup true &&
- sed -i "s/^line4/line4 edit/" file_text &&
+ sed "s/^line4/line4 edit/" <file_text >file_text.tmp &&
+ mv -f file_text.tmp file_text &&
git commit -m "file_text line4 edit" file_text &&
(
cd "$cli" &&
p4 open file_text &&
- sed -i "s/^line5/line5 p4 edit/" file_text &&
+ sed "s/^line5/line5 p4 edit/" <file_text >file_text.tmp &&
+ mv -f file_text.tmp file_text &&
p4 submit -d "file5 p4 edit"
) &&
echo s | test_expect_code 1 git p4 submit &&
--
1.8.1.203.gc241474
^ permalink raw reply related
* [RFH] NetBSD 6?
From: Junio C Hamano @ 2013-01-02 23:11 UTC (permalink / raw)
To: git
I would appreciate if somebody with more familiarlity with the
platform can suggest a better alternative than applying the
following patch to our Makefile. Right now I have an equivalent of
this change in config.mak locally when building on the said
platform.
The "2.7" bit certainly looks fishy, as users should be able to
choose between "2.6" and "2.7" (and possibly "3.0"), IIUC.
Makefile | 3 +++
1 file changed, 3 insertions(+)
diff --git a/Makefile b/Makefile
index b0df41b..2ca6047 100644
--- a/Makefile
+++ b/Makefile
@@ -1163,8 +1163,11 @@ ifeq ($(uname_S),NetBSD)
ifeq ($(shell expr "$(uname_R)" : '[01]\.'),2)
NEEDS_LIBICONV = YesPlease
endif
+ PYTHON_PATH = /usr/pkg/bin/python2.7
+ PERL_PATH = /usr/pkg/bin/perl
BASIC_CFLAGS += -I/usr/pkg/include
BASIC_LDFLAGS += -L/usr/pkg/lib $(CC_LD_DYNPATH)/usr/pkg/lib
+ OLD_ICONV = YesPlease
USE_ST_TIMESPEC = YesPlease
NO_MKSTEMPS = YesPlease
HAVE_PATHS_H = YesPlease
^ permalink raw reply related
* Re: [PATCH 1/4] test: Add target test-lint-shell-syntax
From: Torsten Bögershausen @ 2013-01-02 23:14 UTC (permalink / raw)
To: Jeff King; +Cc: Torsten Bögershausen, git
In-Reply-To: <20130102094635.GD9328@sigill.intra.peff.net>
On 02.01.13 10:46, Jeff King wrote:> On Tue, Jan 01, 2013 at 10:40:08PM +0100, Torsten Bögershausen wrote:
>
>> Add the perl script "check-non-portable-shell.pl" to detect non-portable
>> shell syntax
>
> Cool. Thanks for adding more test-lint. But...
>
>> diff --git a/t/Makefile b/t/Makefile
>> index 88e289f..7b0c4dc 100644
>> --- a/t/Makefile
>> +++ b/t/Makefile
>> @@ -23,7 +23,7 @@ TGITWEB = $(sort $(wildcard t95[0-9][0-9]-*.sh))
>>
>> all: $(DEFAULT_TEST_TARGET)
>>
>> -test: pre-clean $(TEST_LINT)
>> +test: pre-clean test-lint-shell-syntax $(TEST_LINT)
>> $(MAKE) aggregate-results-and-cleanup
>
> I do not think it should be a hard-coded dependency of "test", as then
> there is no way to turn it off. It would make more sense to me to set a
> default TEST_LINT that includes it, but could be overridden by the user.
>
>> prove: pre-clean $(TEST_LINT)
>> @@ -43,7 +43,7 @@ clean-except-prove-cache:
>> clean: clean-except-prove-cache
>> $(RM) .prove
>>
>> -test-lint: test-lint-duplicates test-lint-executable
>> +test-lint: test-lint-duplicates test-lint-executable test-lint-shell-syntax
>
> This, however, is right. The point of "test-lint" is "use all the lint",
> so adding it here makes sense (and anyone who has set "TEST_LINT =
> test-lint" will get the new check).
>
>> +test-lint-shell-syntax:
>> + $(PERL_PATH) check-non-portable-shell.pl $(T)
>
> This is wrong if $(PERL_PATH) contains spaces, no? Doing "$(PERL_PATH)"
> is also wrong, because the expansion happens in 'make', and a
> $(PERL_PATH) with double-quotes would fool the shell. Since we export
> $PERL_PATH, I think doing:
>
> "$$PERL_PATH"" check-non-portable-shell.pl $(T)
Thanks, but:
- The double "" after PERL_PATH makes the string un-terminated.
- Using "$$PERL_PATH" expands from make into "$PERL_PATH" on the command line
- If the Makefile looks like this:
PERL_PATH = "/Users/tb/projects/git/tb/pe rl"
[snip]
$(PERL_PATH) check-non-portable-shell.pl $(T)
The command line will look like this:
"/Users/tb/projects/git/tb/pe rl" check-non-portable-shell.pl t0000-basic.sh ...
So I think that PERL_PATH should be quoted when it is defined in the Makefile.
[snip]
Peff, many thanks. Please see V2 patch comming soon
^ permalink raw reply
* Re: git filter-branch doesn't dereference annotated tags
From: Junio C Hamano @ 2013-01-02 23:19 UTC (permalink / raw)
To: Grégory Pakosz; +Cc: git, Johannes Sixt
In-Reply-To: <CAC_01E2iHgNvh5PnBh3TcNKr2pLazZwRojVK9ksaE3x0a1QHmQ@mail.gmail.com>
Grégory Pakosz <gpakosz@visionobjects.com> writes:
> So we have an annotated tag that points to a commit that is rewritten
> to nothing as the result of the filtering. What should happen?
If the user asked to filter that tag itself, it may make sense to
remove it, rather than keeping it pointing at the original commit,
because the commit it used to point at no longer exists in the
alternate history being created by filter-branch.
> It's basically the same problem. In my opinion, lines 447-466 should
> take into account $new_sha1 is empty.
Yeah, I think that is a sensible observation.
Having said that, I welcome comments from others, of course. My
involvement in this script has been very limited.
^ permalink raw reply
* [PATCH V2] test: Add check-non-portable-shell.pl
From: Torsten Bögershausen @ 2013-01-02 23:20 UTC (permalink / raw)
To: peff, gitster, git; +Cc: tboegi
Add the perl script "check-non-portable-shell.pl" to detect non-portable
shell syntax
"echo -n" is an example of a shell command working on Linux,
but not on Mac OS X.
These shell commands are checked and reported as error:
- "echo -n" (printf should be used)
- "sed -i" (Use a temp file)
- arrays in shell scripts (declare statement)
- "which" (type should be used)
- "==" (bash style for =)
"make test-lint-shell-syntax" can be used to run only the check.
"make" will run check-non-portable-shell.pl followed by t0000.sh -- t9999.sh
"TEST_LINT= make" will only run t0000.sh -- t9999.sh
Helped-By: Jeff King <peff@peff.net>
Signed-off-by: Torsten Bögershausen <tboegi@web.de>
---
Thanks for reviewing & suggestions
Changes since v1:
Makefile:
- "$TEST_LINT= make" will disable the check
check-non-portable-shell.pl:
- Much more perl style (instead of C-Code in perl language)
Hopefuly better commit message
t/Makefile | 6 +++++-
t/check-non-portable-shell.pl | 27 +++++++++++++++++++++++++++
2 files changed, 32 insertions(+), 1 deletion(-)
create mode 100755 t/check-non-portable-shell.pl
diff --git a/t/Makefile b/t/Makefile
index 88e289f..fd239cb 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -13,6 +13,7 @@ TAR ?= $(TAR)
RM ?= rm -f
PROVE ?= prove
DEFAULT_TEST_TARGET ?= test
+TEST_LINT ?= test-lint-shell-syntax
# Shell quote;
SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH))
@@ -43,7 +44,7 @@ clean-except-prove-cache:
clean: clean-except-prove-cache
$(RM) .prove
-test-lint: test-lint-duplicates test-lint-executable
+test-lint: test-lint-duplicates test-lint-executable test-lint-shell-syntax
test-lint-duplicates:
@dups=`echo $(T) | tr ' ' '\n' | sed 's/-.*//' | sort | uniq -d` && \
@@ -55,6 +56,9 @@ test-lint-executable:
test -z "$$bad" || { \
echo >&2 "non-executable tests:" $$bad; exit 1; }
+test-lint-shell-syntax:
+ $(PERL_PATH) check-non-portable-shell.pl $(T)
+
aggregate-results-and-cleanup: $(T)
$(MAKE) aggregate-results
$(MAKE) clean
diff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl
new file mode 100755
index 0000000..49d7291
--- /dev/null
+++ b/t/check-non-portable-shell.pl
@@ -0,0 +1,27 @@
+#!/usr/bin/perl
+
+# Test t0000..t9999.sh for non portable shell scripts
+# This script can be called with one or more filenames as parameters
+
+use strict;
+use warnings;
+
+my $exit_code=0;
+
+sub err {
+ my $msg = shift;
+ print "$ARGV:$.: error: $msg: $_\n";
+ $exit_code = 1;
+}
+
+while (<>) {
+ chomp;
+ /^\s*sed\s+-i/ and err 'sed -i is not portable';
+ /^\s*echo\s+-n/ and err 'echo -n is not portable (please use printf)';
+ /^\s*declare\s+/ and err 'arrays/declare not portable';
+ /^\s*[^#]\s*which\s/ and err 'which is not portable (please use type)';
+ /test\s+[^=]*==/ and err '== is not portable (please use =)';
+ # this resets our $. for each file
+ close ARGV if eof;
+}
+exit $exit_code;
--
1.8.0.197.g5a90748
^ permalink raw reply related
* Re: [PATCH 1/4] test: Add target test-lint-shell-syntax
From: Jeff King @ 2013-01-02 23:22 UTC (permalink / raw)
To: Torsten Bögershausen; +Cc: git
In-Reply-To: <50E4BF58.4090808@web.de>
On Thu, Jan 03, 2013 at 12:14:32AM +0100, Torsten Bögershausen wrote:
> > This is wrong if $(PERL_PATH) contains spaces, no? Doing "$(PERL_PATH)"
> > is also wrong, because the expansion happens in 'make', and a
> > $(PERL_PATH) with double-quotes would fool the shell. Since we export
> > $PERL_PATH, I think doing:
> >
> > "$$PERL_PATH"" check-non-portable-shell.pl $(T)
> Thanks, but:
> - The double "" after PERL_PATH makes the string un-terminated.
Yeah, sorry, typo on my part.
> - Using "$$PERL_PATH" expands from make into "$PERL_PATH" on the command line
Right. That's what I intended.
> - If the Makefile looks like this:
> PERL_PATH = "/Users/tb/projects/git/tb/pe rl"
> [snip]
> $(PERL_PATH) check-non-portable-shell.pl $(T)
> The command line will look like this:
> "/Users/tb/projects/git/tb/pe rl" check-non-portable-shell.pl t0000-basic.sh ...
>
> So I think that PERL_PATH should be quoted when it is defined in the Makefile.
Does a $PERL_PATH with quotes actually work?
Usually in our runtime environment, commands that are handed to git are
assumed to be passed directly to the shell, and you need to quote. E.g.,
setting diff.external to:
[diff]
external = "foo --bar"
will let the shell split the argument out; if you have a space, you
would want to set it like:
[diff]
external = "'command with space'"
This is the most flexible way to do it.
However, for Makefile variables, I think we do not (and cannot) follow
the same rule. Notice that all of the uses of $PERL_PATH in the test
suite enclose it in quotes. Having extra quotes would break those
invocations. And the value of $PERL_PATH will be put on the #!-line,
which cannot not be quoted.
-Peff
^ permalink raw reply
* Re: [PATCH] Replace git-cvsimport with a rewrite that fixes major bugs.
From: Martin Langhoff @ 2013-01-02 23:44 UTC (permalink / raw)
To: Eric Raymond; +Cc: Jonathan Nieder, Junio C Hamano, Git Mailing List
In-Reply-To: <20130102222849.GA21105@thyrsus.com>
On Wed, Jan 2, 2013 at 5:28 PM, Eric S. Raymond <esr@thyrsus.com> wrote:
> Martin Langhoff <martin.langhoff@gmail.com>:
>> I dealt with enough CVS repos to see that the branch point could be
>> ambiguous, and that some cases were incurably ugly and ambiguous.
>
> You are quite right, but you have misintepreted the subject of my
> confidence. I am under no illusion that the new cvsimport/cvsps
> pair is a perfect solution to the CVS-lifting problem, nor even that
> such a solution is possible.
Thanks. That is a much more reassuring stance.
m
--
martin.langhoff@gmail.com
martin@laptop.org -- Software Architect - OLPC
- ask interesting questions
- don't get distracted with shiny stuff - working code first
- http://wiki.laptop.org/go/User:Martinlanghoff
^ permalink raw reply
* Re: [PATCH 1/4] test: Add target test-lint-shell-syntax
From: Torsten Bögershausen @ 2013-01-02 23:58 UTC (permalink / raw)
To: Jeff King; +Cc: Torsten Bögershausen, git
In-Reply-To: <20130102232239.GA27952@sigill.intra.peff.net>
On 03.01.13 00:22, Jeff King wrote:
> On Thu, Jan 03, 2013 at 12:14:32AM +0100, Torsten Bögershausen wrote:
>
>>> This is wrong if $(PERL_PATH) contains spaces, no? Doing "$(PERL_PATH)"
>>> is also wrong, because the expansion happens in 'make', and a
>>> $(PERL_PATH) with double-quotes would fool the shell. Since we export
>>> $PERL_PATH, I think doing:
>>>
>>> "$$PERL_PATH"" check-non-portable-shell.pl $(T)
>> Thanks, but:
>> - The double "" after PERL_PATH makes the string un-terminated.
>
> Yeah, sorry, typo on my part.
>
>> - Using "$$PERL_PATH" expands from make into "$PERL_PATH" on the command line
>
> Right. That's what I intended.
>
>> - If the Makefile looks like this:
>> PERL_PATH = "/Users/tb/projects/git/tb/pe rl"
>> [snip]
>> $(PERL_PATH) check-non-portable-shell.pl $(T)
>> The command line will look like this:
>> "/Users/tb/projects/git/tb/pe rl" check-non-portable-shell.pl t0000-basic.sh ...
>>
>> So I think that PERL_PATH should be quoted when it is defined in the Makefile.
>
> Does a $PERL_PATH with quotes actually work?
At least on my system the following combination works:
git diff
diff --git a/t/Makefile b/t/Makefile
index f8f8c54..391a5ca 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -8,7 +8,7 @@
#GIT_TEST_OPTS = --verbose --debug
SHELL_PATH ?= $(SHELL)
-PERL_PATH ?= /usr/bin/perl
+PERL_PATH = "/Users/tb/projects/git/tb/pe rl"
TAR ?= $(TAR)
RM ?= rm -f
PROVE ?= prove
~/projects/git/tb/t> ls -l "/Users/tb/projects/git/tb/pe rl"
lrwxr-xr-x 1 tb staff 13 Jan 3 00:33 /Users/tb/projects/git/tb/pe rl -> /usr/bin/perl
====================================================
And this works as well, is that what you intended?
Note: "single Dollar"
====================================================
git diff
diff --git a/t/Makefile b/t/Makefile
index f8f8c54..f624f95 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -8,7 +8,7 @@
#GIT_TEST_OPTS = --verbose --debug
SHELL_PATH ?= $(SHELL)
-PERL_PATH ?= /usr/bin/perl
+PERL_PATH = /Users/tb/projects/git/tb/pe rl
TAR ?= $(TAR)
RM ?= rm -f
PROVE ?= prove
@@ -57,7 +57,7 @@ test-lint-executable:
echo >&2 "non-executable tests:" $$bad; exit 1; }
test-lint-shell-syntax:
- $(PERL_PATH) check-non-portable-shell.pl $(T)
+ "$(PERL_PATH)" check-non-portable-shell.pl $(T)
> Usually in our runtime environment, commands that are handed to git are
> assumed to be passed directly to the shell, and you need to quote. E.g.,
> setting diff.external to:
>
> [diff]
> external = "foo --bar"
>
> will let the shell split the argument out; if you have a space, you
> would want to set it like:
>
> [diff]
> external = "'command with space'"
>
> This is the most flexible way to do it.
>
> However, for Makefile variables, I think we do not (and cannot) follow
> the same rule. Notice that all of the uses of $PERL_PATH in the test
> suite enclose it in quotes. Having extra quotes would break those
> invocations. And the value of $PERL_PATH will be put on the #!-line,
> which cannot not be quoted.
>
> -Peff
I followed these lines as an example:
test-results/git-smoke.tar.gz: test-results
$(PERL_PATH) ./harness \
--archive="test-results/git-smoke.tar.gz" \
$(T)
(and make smoke did not work, as we don't have ./harness :-(
Do we need some cleanup/improvements here as well?
/Torsten
^ permalink raw reply related
* Re: [PATCH 1/4] test: Add target test-lint-shell-syntax
From: Junio C Hamano @ 2013-01-03 0:01 UTC (permalink / raw)
To: Jeff King; +Cc: Torsten Bögershausen, git
In-Reply-To: <20130102094635.GD9328@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Tue, Jan 01, 2013 at 10:40:08PM +0100, Torsten Bögershausen wrote:
>
>> Add the perl script "check-non-portable-shell.pl" to detect non-portable
>> shell syntax
>
> Cool. Thanks for adding more test-lint. But...
>
>> diff --git a/t/Makefile b/t/Makefile
>> index 88e289f..7b0c4dc 100644
>> --- a/t/Makefile
>> +++ b/t/Makefile
>> @@ -23,7 +23,7 @@ TGITWEB = $(sort $(wildcard t95[0-9][0-9]-*.sh))
>>
>> all: $(DEFAULT_TEST_TARGET)
>>
>> -test: pre-clean $(TEST_LINT)
>> +test: pre-clean test-lint-shell-syntax $(TEST_LINT)
>> $(MAKE) aggregate-results-and-cleanup
>
> I do not think it should be a hard-coded dependency of "test", as then
> there is no way to turn it off. It would make more sense to me to set a
> default TEST_LINT that includes it, but could be overridden by the user.
I would actually not add this to TEST_LINT by default, especially
when "duplicates" and "executable" that are much simpler and less
likely to hit false positives are not on by default.
At least, a change to add this to TEST_LINT by default must wait to
be merged to any integration branch until all the fix-up topics that
this test triggers in the current codebase graduate to the branch.
>> +test-lint-shell-syntax:
>> + $(PERL_PATH) check-non-portable-shell.pl $(T)
>
> This is wrong if $(PERL_PATH) contains spaces, no?
You are correct; "harness" thing in the same Makefile gets this
wrong, too. I think the right invocation is:
@'$(PERL_PATH_SQ)' check-non-portable.shell.pl $(T)
although I do not offhand know if that symbol is already exported by
the top-level Makefile.
^ permalink raw reply
* Re: [PATCH 1/4] test: Add target test-lint-shell-syntax
From: Junio C Hamano @ 2013-01-03 0:08 UTC (permalink / raw)
To: Jeff King; +Cc: Torsten Bögershausen, git
In-Reply-To: <7vtxqzyw0g.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> I would actually not add this to TEST_LINT by default, especially
> when "duplicates" and "executable" that are much simpler and less
> likely to hit false positives are not on by default.
>
> At least, a change to add this to TEST_LINT by default must wait to
> be merged to any integration branch until all the fix-up topics that
> this test triggers in the current codebase graduate to the branch.
>
>>> +test-lint-shell-syntax:
>>> + $(PERL_PATH) check-non-portable-shell.pl $(T)
>>
>> This is wrong if $(PERL_PATH) contains spaces, no?
>
> You are correct; "harness" thing in the same Makefile gets this
> wrong, too. I think the right invocation is:
>
> @'$(PERL_PATH_SQ)' check-non-portable.shell.pl $(T)
>
> although I do not offhand know if that symbol is already exported by
> the top-level Makefile.
I'll tentatively queue this instead. The log message has also been
cleaned up a bit.
-- >8 --
From: Torsten Bögershausen <tboegi@web.de>
Date: Thu, 3 Jan 2013 00:20:19 +0100
Subject: [PATCH] test: Add check-non-portable-shell.pl
Add the perl script "check-non-portable-shell.pl" to detect
non-portable shell syntax.
"echo -n" is an example of a shell command working on Linux, but not
on Mac OS X.
These shell commands are checked and reported as error:
- "echo -n" (printf should be used)
- "sed -i" (GNUism; use a temp file instead)
- "declare" (bashism, often used with arrays)
- "which" (unreliable exit status and output; use type instead)
- "test a == b" (bashism for "test a = b")
"make test-lint-shell-syntax" can be used to run only the check.
Helped-By: Jeff King <peff@peff.net>
Signed-off-by: Torsten Bögershausen <tboegi@web.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
t/Makefile | 8 ++++++--
t/check-non-portable-shell.pl | 27 +++++++++++++++++++++++++++
2 files changed, 33 insertions(+), 2 deletions(-)
create mode 100755 t/check-non-portable-shell.pl
diff --git a/t/Makefile b/t/Makefile
index 3025418..a43becb 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -16,6 +16,7 @@ DEFAULT_TEST_TARGET ?= test
# Shell quote;
SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH))
+PERL_PATH_SQ = $(subst ','\'',$(PERL_PATH))
T = $(sort $(wildcard t[0-9][0-9][0-9][0-9]-*.sh))
TSVN = $(sort $(wildcard t91[0-9][0-9]-*.sh))
@@ -43,7 +44,7 @@ clean-except-prove-cache:
clean: clean-except-prove-cache
$(RM) .prove
-test-lint: test-lint-duplicates test-lint-executable
+test-lint: test-lint-duplicates test-lint-executable test-lint-shell-syntax
test-lint-duplicates:
@dups=`echo $(T) | tr ' ' '\n' | sed 's/-.*//' | sort | uniq -d` && \
@@ -55,6 +56,9 @@ test-lint-executable:
test -z "$$bad" || { \
echo >&2 "non-executable tests:" $$bad; exit 1; }
+test-lint-shell-syntax:
+ @'$(PERL_PATH_SQ)' check-non-portable-shell.pl $(T)
+
aggregate-results-and-cleanup: $(T)
$(MAKE) aggregate-results
$(MAKE) clean
@@ -87,7 +91,7 @@ test-results:
mkdir -p test-results
test-results/git-smoke.tar.gz: test-results
- $(PERL_PATH) ./harness \
+ '$(PERL_PATH_SQ)' ./harness \
--archive="test-results/git-smoke.tar.gz" \
$(T)
diff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl
new file mode 100755
index 0000000..8b5a71d
--- /dev/null
+++ b/t/check-non-portable-shell.pl
@@ -0,0 +1,27 @@
+#!/usr/bin/perl
+
+# Test t0000..t9999.sh for non portable shell scripts
+# This script can be called with one or more filenames as parameters
+
+use strict;
+use warnings;
+
+my $exit_code=0;
+
+sub err {
+ my $msg = shift;
+ print "$ARGV:$.: error: $msg: $_\n";
+ $exit_code = 1;
+}
+
+while (<>) {
+ chomp;
+ /^\s*sed\s+-i/ and err 'sed -i is not portable';
+ /^\s*echo\s+-n/ and err 'echo -n is not portable (please use printf)';
+ /^\s*declare\s+/ and err 'arrays/declare not portable';
+ /^\s*[^#]\s*which\s/ and err 'which is not portable (please use type)';
+ /test\s+[^=]*==/ and err '"test a == b" is not portable (please use =)';
+ # this resets our $. for each file
+ close ARGV if eof;
+}
+exit $exit_code;
--
1.8.1.203.gc241474
^ permalink raw reply related
* Re: [PATCH 1/4] test: Add target test-lint-shell-syntax
From: Junio C Hamano @ 2013-01-03 0:16 UTC (permalink / raw)
To: Torsten Bögershausen; +Cc: Jeff King, git
In-Reply-To: <50E4C9B5.8070308@web.de>
Torsten Bögershausen <tboegi@web.de> writes:
> At least on my system the following combination works:
>
> git diff
> diff --git a/t/Makefile b/t/Makefile
> index f8f8c54..391a5ca 100644
> --- a/t/Makefile
> +++ b/t/Makefile
> @@ -8,7 +8,7 @@
>
> #GIT_TEST_OPTS = --verbose --debug
> SHELL_PATH ?= $(SHELL)
> -PERL_PATH ?= /usr/bin/perl
> +PERL_PATH = "/Users/tb/projects/git/tb/pe rl"
I do not think that will fly. Having that in the main Makefile
where the existing users of the symbol relies on it without any
surrounding quotes, e.g.
$(patsubst %.perl,%,$(SCRIPT_PERL)): % : %.perl GIT-VERSION-FILE
$(QUIET_GEN)$(RM) $@ $@+ && \
INSTLIBDIR=`MAKEFLAGS= $(MAKE) -C perl -s --no-print-directory instlibdir` && \
sed -e '1{' \
-e ' s|#!.*perl|#!$(PERL_PATH_SQ)|' \
-e ' h' \
-e ' s=.*=use lib (split(/$(pathsep)/, $$ENV{GITPERLLIB} || "'"$$INSTLIBDIR"'"));=' \
-e ' H' \
-e ' x' \
-e '}' \
-e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \
$@.perl >$@+ && \
chmod +x $@+ && \
mv $@+ $@
where $(PERL_PATH_SQ) is defined to replace each ' in $(PERL_PATH)
with '\'' so that '$(PERL_PATH_SQ)' becomes a shell-safe way to
quote the value of PERL_PATH without quotes, your definition will
look for a relative path that is inside a directory named '"'
(that's a single double-quote).
^ permalink raw reply
* Re: [PATCH 1/4] test: Add target test-lint-shell-syntax
From: Torsten Bögershausen @ 2013-01-03 0:23 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Torsten Bögershausen, Jeff King, git
In-Reply-To: <7vlicbyvc2.fsf@alter.siamese.dyndns.org>
On 03.01.13 01:16, Junio C Hamano wrote:
> Torsten Bögershausen <tboegi@web.de> writes:
>
>> At least on my system the following combination works:
>>
>> git diff
>> diff --git a/t/Makefile b/t/Makefile
>> index f8f8c54..391a5ca 100644
>> --- a/t/Makefile
>> +++ b/t/Makefile
>> @@ -8,7 +8,7 @@
>>
>> #GIT_TEST_OPTS = --verbose --debug
>> SHELL_PATH ?= $(SHELL)
>> -PERL_PATH ?= /usr/bin/perl
>> +PERL_PATH = "/Users/tb/projects/git/tb/pe rl"
>
> I do not think that will fly. Having that in the main Makefile
> where the existing users of the symbol relies on it without any
> surrounding quotes, e.g.
>
> $(patsubst %.perl,%,$(SCRIPT_PERL)): % : %.perl GIT-VERSION-FILE
> $(QUIET_GEN)$(RM) $@ $@+ && \
> INSTLIBDIR=`MAKEFLAGS= $(MAKE) -C perl -s --no-print-directory instlibdir` && \
> sed -e '1{' \
> -e ' s|#!.*perl|#!$(PERL_PATH_SQ)|' \
> -e ' h' \
> -e ' s=.*=use lib (split(/$(pathsep)/, $$ENV{GITPERLLIB} || "'"$$INSTLIBDIR"'"));=' \
> -e ' H' \
> -e ' x' \
> -e '}' \
> -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \
> $@.perl >$@+ && \
> chmod +x $@+ && \
> mv $@+ $@
>
> where $(PERL_PATH_SQ) is defined to replace each ' in $(PERL_PATH)
> with '\'' so that '$(PERL_PATH_SQ)' becomes a shell-safe way to
> quote the value of PERL_PATH without quotes, your definition will
> look for a relative path that is inside a directory named '"'
> (that's a single double-quote).
Thanks to all for the explanations, fixing up and queing.
And good news:
pu today is "clean",there where no problems found:
commit d69ea46220647c048d332c471a184446cce17627
Merge: e552539 fcf30b3
Author: Junio C Hamano <gitster@pobox.com>
Date: Wed Jan 2 12:44:33 2013 -0800
When the dust has settled, we can either enable the check always, or mention
"make test-lint-shell-syntax" in the Documentation.
^ permalink raw reply
* Re: [RFH] NetBSD 6?
From: Greg Troxel @ 2013-01-03 0:25 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vd2xn18p5.fsf@alter.siamese.dyndns.org>
[-- Attachment #1: Type: text/plain, Size: 2656 bytes --]
Junio C Hamano <gitster@pobox.com> writes:
> [query about NetBSD-6]
> The "2.7" bit certainly looks fishy, as users should be able to
> choose between "2.6" and "2.7" (and possibly "3.0"), IIUC.
>
> + PYTHON_PATH = /usr/pkg/bin/python2.7
> + PERL_PATH = /usr/pkg/bin/perl
(I am one of the people who maintain the git package in pkgsrc.)
(Strictly, this is not really about NetBSD, but about all systems where
the standard approach to get python is via pkgsrc. So that include
DragonflyBSD as well. (pkgsrc runs on many other systems, but it isn't
the standard approach, so from the git viewpoint that's irrelevant.))
You are entirely right that on e.g. NetBSD 6 the view is that users
should be able to choose the python version.
pkgsrc can install multiple versions of python at the same time, to cope
with python-using packages that need different versions. pkgsrc chooses
not to have a 'python' program, because that would result in installed
packages changing their binding of which python version to use when the
default was changed. The default python version is currently 2.7, so
/usr/pkg/bin/python2.7 is the best guess for finding python on a NetBSD
system, if you're only allowed one guess. A user can set a
PYTHON_VERSION_DEFAULT variable to choose the version they want; each
package expresses which versions will work.
This isn't relevant for git, not being a pure python library, but pkgsrc
supports installing multiple versions of some packages, so one can have
two versions installed at once:
py27-expat-0nb6 Python interface to expat
py26-expat-0nb6 Python interface to expat
The git package just depends on one version; by default the git package
depends on python (but one can tell it not to).
The python.m4 macro that comes with automake seems to find one of the
various pythonX.Y binaries in $PATH just fine.
pkgsrc has an entry for git (at 1.8.0.1).
The key line for handling python is:
MAKE_FLAGS+= PYTHON_PATH=${PYTHONBIN}
and there PYTHONBIN is set up by pkgsrc infrastructure for the right
prefix (99.9% but not always /usr/pkg) and version. After this,
everything seems to come out right:
> head -1 /usr/pkg/libexec/git-core/git-p4
#!/usr/pkg/bin/python2.7
So I'd say that if PYTHON_PATH is set in the environment to configure,
it should behave as it does now. And if not, it would be nice if the
highest pythonX.Y found (that is known to work with git) is used.
> + PYTHON_PATH = /usr/pkg/bin/python2.7
> + PERL_PATH = /usr/pkg/bin/perl
So it would be nice to make these work as ?=, letting an environment
variable win if set.
[-- Attachment #2: Type: application/pgp-signature, Size: 194 bytes --]
^ permalink raw reply
* Re: [RFH] NetBSD 6?
From: Greg Troxel @ 2013-01-03 0:30 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vd2xn18p5.fsf@alter.siamese.dyndns.org>
[-- Attachment #1: Type: text/plain, Size: 1001 bytes --]
Junio C Hamano <gitster@pobox.com> writes:
> I would appreciate if somebody with more familiarlity with the
> platform can suggest a better alternative than applying the
> following patch to our Makefile. Right now I have an equivalent of
> this change in config.mak locally when building on the said
> platform.
I realized after sending my previous reply that you are probably trying
to have a way to build and run tests on NetBSD-6 from a git checkout as
part of development testing.
One approach I've taken with other programs is to have a README.NetBSD
file which is actually an executable /bin/sh script with comments,
explaining the prereqs in terms of pkgsrc and invoking configure to get
dependencies from pkgsrc (-I/usr/pkg/include plus -L/-R).
So in the git case, this could set PYTHON_PATH in the environment.
I realize a README.foo file for N different systems could be clutter,
but having these checked in would provide the concise help that people
on any of those platforms need.
[-- Attachment #2: Type: application/pgp-signature, Size: 194 bytes --]
^ permalink raw reply
* Re: [PATCH 1/4] test: Add target test-lint-shell-syntax
From: Junio C Hamano @ 2013-01-03 2:02 UTC (permalink / raw)
To: Torsten Bögershausen; +Cc: Jeff King, git
In-Reply-To: <50E4CF7E.9090302@web.de>
Torsten Bögershausen <tboegi@web.de> writes:
> When the dust has settled, we can either enable the check always,
> or mention "make test-lint-shell-syntax" in the Documentation.
In the longer term, I'm pretty much in favor of enabling all the
checks that are cheap by default, as that would help people catch
easy mistakes while preparing their patches. People do not tend to
enable any check if it were optional.
^ permalink raw reply
* Re: [RFH] NetBSD 6?
From: Junio C Hamano @ 2013-01-03 2:15 UTC (permalink / raw)
To: Greg Troxel; +Cc: git
In-Reply-To: <rmipq1numzj.fsf@fnord.ir.bbn.com>
Greg Troxel <gdt@ir.bbn.com> writes:
> I realize a README.foo file for N different systems could be clutter,
> but having these checked in would provide the concise help that people
> on any of those platforms need.
Our Makefile documents knobs people on various platforms can tweak
(PYTHON_PATH and OLD_ICONV are two examples of them), sets default
values for them based on $(uname -S), then includes config.mak file
the user can optionally create to override these platform defaults.
This infrastructure is used across platforms, not just for NetBSD.
The part shown in the patch was to update the platform default for
NetBSD. The setting we have been shipping in our Makefile seemed to
be different from what I needed on my NetBSD 6 install, and I was
wondering if we have no real users of Git on the platorm (which
would explain why we didn't get any complaints or patches to update
this part). Or there are some optional packages all real NetBSD
users install, but being a NetBSD newbie I didn't, that makes the
values I showed in the patch inappropriate for them (e.g. Perhaps
there is a mechanism other than pkgsrc that installs perl and python
under /usr/bin? Perhaps an optional libi18n package that gives
iconv(3) with new function signature?), in which case I definitely
should not apply that patch to my tree, as it would only be an
improvement for one person and a regression for existing users at
the same time.
^ permalink raw reply
* Re: [PATCH V2] t9810: Do not use sed -i
From: Pete Wyckoff @ 2013-01-03 3:08 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Torsten Bögershausen, git
In-Reply-To: <7vhamz18y3.fsf@alter.siamese.dyndns.org>
gitster@pobox.com wrote on Wed, 02 Jan 2013 15:06 -0800:
> Pete Wyckoff <pw@padd.com> writes:
>
> > tboegi@web.de wrote on Wed, 02 Jan 2013 00:20 +0100:
> >> sed -i is not portable on all systems.
> >> Use sed with different input and output files.
> >> Utilize a tmp file whenever needed
> >>
> >> Added missing && at 2 places
> >>
> >> Signed-off-by: Torsten Bögershausen <tboegi@web.de>
> >
> > One more teensy fix is needed in this hunk, following Junio's
> > comment about redirections going at the end:
>
> I've already pushed out the previous one on 'pu' after fixing it up
> like this.
>
> Thanks. Anything I missed?
I should have guessed that you would just silently fix it.
Thanks!
-- Pete
^ permalink raw reply
* Re: [PATCH] Replace git-cvsimport with a rewrite that fixes major bugs.
From: Chris Rorvick @ 2013-01-03 6:34 UTC (permalink / raw)
To: esr; +Cc: git
In-Reply-To: <20130101172645.GA5506@thyrsus.com>
On Tue, Jan 1, 2013 at 11:26 AM, Eric S. Raymond <esr@thyrsus.com> wrote:
> diff --git a/git-cvsimport.py b/git-cvsimport.py
> new file mode 100755
> index 0000000..6407e8a
> --- /dev/null
> +++ b/git-cvsimport.py
> @@ -0,0 +1,342 @@
> +#!/usr/bin/env python
> +#
> +# Import CVS history into git
> +#
> +# Intended to be a near-workalike of Matthias Urlichs's Perl implementation.
> +#
> +# By Eric S. Raymond <esr@thyrsus.com>, December 2012
> +# May be redistributed under the license of the git project.
> +
> +import sys
> +
> +if sys.hexversion < 0x02060000:
> + sys.stderr.write("git cvsimport: requires Python 2.6 or later.\n")
> + sys.exit(1)
> +
> +import os, getopt, subprocess, tempfile, shutil
> +
> +DEBUG_COMMANDS = 1
> +
> +class Fatal(Exception):
> + "Unrecoverable error."
> + def __init__(self, msg):
> + Exception.__init__(self)
> + self.msg = msg
> +
> +def do_or_die(dcmd, legend=""):
> + "Either execute a command or raise a fatal exception."
> + if legend:
> + legend = " " + legend
> + if verbose >= DEBUG_COMMANDS:
> + sys.stdout.write("git cvsimport: executing '%s'%s\n" % (dcmd, legend))
> + try:
> + retcode = subprocess.call(dcmd, shell=True)
> + if retcode < 0:
> + raise Fatal("git cvsimport: child was terminated by signal %d." % -retcode)
> + elif retcode != 0:
> + raise Fatal("git cvsimport: child returned %d." % retcode)
> + except (OSError, IOError) as e:
> + raise Fatal("git cvsimport: execution of %s%s failed: %s" % (dcmd, legend, e))
> +
> +def capture_or_die(dcmd, legend=""):
> + "Either execute a command and capture its output or die."
> + if legend:
> + legend = " " + legend
> + if verbose >= DEBUG_COMMANDS:
> + sys.stdout.write("git cvsimport: executing '%s'%s\n" % (dcmd, legend))
> + try:
> + return subprocess.check_output(dcmd, shell=True)
> + except subprocess.CalledProcessError as e:
> + if e.returncode < 0:
> + sys.stderr.write("git cvsimport: child was terminated by signal %d." % -e.returncode)
> + elif e.returncode != 0:
> + sys.stderr.write("git cvsimport: child returned %d." % e.returncode)
> + sys.exit(1)
> +
> +class cvsps:
> + "Method class for cvsps back end."
> + def __init__(self):
> + self.opts = ""
> + self.revmap = None
> + def set_repo(self, val):
> + "Set the repository root option."
> + if not val.startswith(":"):
> + if not val.startswith(os.sep):
> + val = os.path.abspath(val)
> + val = ":local:" + val
> + self.opts += " --root '%s'" % val
> + def set_authormap(self, val):
> + "Set the author-map file."
> + self.opts += " -A '%s'" % val
> + def set_fuzz(self, val):
> + "Set the commit-similarity window."
> + self.opts += " -z %s" % val
> + def set_nokeywords(self):
> + "Suppress CVS keyword expansion."
> + self.opts += " -k"
> + def add_opts(self, val):
> + "Add options to the engine command line."
> + self.opts += " " + val
> + def set_exclusion(self, val):
> + "Set a file exclusion regexp."
> + self.opts += " -n -f '%s'" % val
> + def set_after(self, val):
> + "Set a date threshold for incremental import."
> + self.opts += " -d '%s'" % val
> + def set_revmap(self, val):
> + "Set the file to which the engine should dump a reference map."
> + self.revmap = val
> + self.opts += " -R '%s'" % self.revmap
> + def set_module(self, val):
> + "Set the module to query."
> + self.opts += " " + val
> + def command(self):
> + "Emit the command implied by all previous options."
> + return "cvsps --fast-export " + self.opts
> +
> +class cvs2git:
> + "Method class for cvs2git back end."
> + def __init__(self):
> + self.opts = ""
> + self.modulepath = "."
> + def set_authormap(self, _val):
> + "Set the author-map file."
> + sys.stderr.write("git cvsimport: author maping is not supported with cvs2git.\n")
> + sys.exit(1)
> + def set_repo(self, _val):
> + "Set the repository root option."
> + sys.stderr.write("git cvsimport: cvs2git must run within a repository checkout directory.\n")
> + sys.exit(1)
> + def set_fuzz(self, _val):
> + "Set the commit-similarity window."
> + sys.stderr.write("git cvsimport: fuzz setting is not supported with cvs2git.\n")
> + sys.exit(1)
> + def set_nokeywords(self):
> + "Suppress CVS keyword expansion."
> + self.opts += " --keywords-off"
> + def add_opts(self, val):
> + "Add options to the engine command line."
> + self.opts += " " + val
> + def set_exclusion(self, val):
> + "Set a file exclusion regexp."
> + self.opts += " --exclude='%s'" % val
> + def set_after(self, _val):
> + "Set a date threshold for incremental import."
> + sys.stderr.write("git cvsimport: incremental import is not supported with cvs2git.\n")
> + sys.exit(1)
> + def set_revmap(self, _val):
> + "Set the file to which the engine should dump a reference map."
> + sys.stderr.write("git cvsimport: can't get a reference map from cvs2git.\n")
> + sys.exit(1)
> + def set_module(self, val):
> + "Set the module to query."
> + self.modulepath = " " + val
> + def command(self):
> + "Emit the command implied by all previous options."
> + return "(cvs2git --username=git-cvsimport --quiet --quiet --blobfile={0} --dumpfile={1} {2} {3} && cat {0} {1} && rm {0} {1})".format(tempfile.mkstemp()[1], tempfile.mkstemp()[1], self.opts, self.modulepath)
> +
> +class filesource:
> + "Method class for file-source back end."
> + def __init__(self, filename):
> + self.filename = filename
> + def __complain(self, legend):
> + sys.stderr.write("git cvsimport: %s with file source.\n" % legend)
> + sys.exit(1)
> + def set_repo(self, _val):
> + "Set the repository root option."
> + self.__complain("repository can't be set")
> + def set_authormap(self, _val):
> + "Set the author-map file."
> + sys.stderr.write("git cvsimport: author maping is not supported with filesource.\n")
> + sys.exit(1)
> + def set_fuzz(self, _val):
> + "Set the commit-similarity window."
> + self.__complain("fuzz can't be set")
> + def set_nokeywords(self, _val):
> + "Suppress CVS keyword expansion."
> + self.__complain("keyword suppression can't be set")
> + def add_opts(self, _val):
> + "Add options to the engine command line."
> + self.__complain("other options can't be set")
> + def set_exclusion(self, _val):
> + "Set a file exclusion regexp."
> + self.__complain("exclusions can't be set")
> + def set_after(self, _val):
> + "Set a date threshold for incremental import."
> + pass
> + def set_revmap(self, _val):
> + "Set the file to which the engine should dump a reference map."
> + sys.stderr.write("git cvsimport: can't get a reference map from cvs2git.\n")
> + sys.exit(1)
> + def set_module(self, _val):
> + "Set the module to query."
> + self.__complain("module can't be set")
> + def command(self):
> + "Emit the command implied by all previous options."
> + return "cat " + self.filename
> +
> +if __name__ == '__main__':
> + if sys.hexversion < 0x02060000:
> + sys.stderr.write("git cvsimport: requires Python 2.6 or later.\n")
> + sys.exit(1)
> + (options, arguments) = getopt.getopt(sys.argv[1:], "vbe:d:C:r:o:ikus:p:z:P:S:aL:A:Rh")
> + verbose = 0
> + bare = False
> + root = None
> + outdir = os.getcwd()
> + remotize = False
> + import_only = False
> + underscore_to_dot = False
> + slashsubst = None
> + authormap = None
> + revisionmap = False
> + backend = cvsps()
> + for (opt, val) in options:
> + if opt == '-v':
> + verbose += 1
> + elif opt == '-b':
> + bare = True
> + elif opt == '-e':
> + for cls in (cvsps, cvs2git):
> + if cls.__name__ == val:
> + backend = cls()
> + break
> + else:
> + sys.stderr.write("git cvsimport: unknown engine %s.\n" % val)
> + sys.exit(1)
> + elif opt == '-d':
> + backend.set_repo(val)
> + elif opt == '-C':
> + outdir = val
> + elif opt == '-r':
> + remotize = True
> + elif opt == '-o':
> + sys.stderr.write("git cvsimport: -o is no longer supported.\n")
> + sys.exit(1)
> + elif opt == '-i':
> + import_only = True
> + elif opt == '-k':
> + backend.set_nokeywords()
> + elif opt == '-u':
> + underscore_to_dot = True
> + elif opt == '-s':
> + slashsubst = val
> + elif opt == '-p':
> + backend.add_opts(val.replace(",", " "))
> + elif opt == '-z':
> + backend.set_fuzz(val)
> + elif opt == '-P':
> + backend = filesource(val)
> + sys.exit(1)
> + elif opt in ('-m', '-M'):
> + sys.stderr.write("git cvsimport: -m and -M are no longer supported: use reposurgeon instead.\n")
> + sys.exit(1)
> + elif opt == '-S':
> + backend.set_exclusion(val)
> + elif opt == '-a':
> + sys.stderr.write("git cvsimport: -a is no longer supported.\n")
> + sys.exit(1)
> + elif opt == '-L':
> + sys.stderr.write("git cvsimport: -L is no longer supported.\n")
> + sys.exit(1)
> + elif opt == '-A':
> + authormap = os.path.abspath(val)
> + elif opt == '-R':
> + revisionmap = True
> + else:
> + print """\
> +git cvsimport [-A <author-conv-file>] [-C <git_repository>] [-b] [-d <CVSROOT>]
> + [-e engine] [-h] [-i] [-k] [-p <options-for-cvsps>] [-P <source-file>]
> + [-r <remote>] [-R] [-s <subst>] [-S <regex>] [-u] [-v] [-z <fuzz>]
> + [<CVS_module>]
> +"""
> +
> + def metadata(fn):
> + if bare:
> + return fn
> + else:
> + return os.path.join(".git", fn)
> + try:
> + if outdir:
> + try:
> + # If the output directory does not exist, create it
> + # and initialize it as a git repository.
> + os.mkdir(outdir)
> + do_or_die("git init --quiet " + outdir)
> + except OSError:
> + # Otherwise, assume user wants incremental import.
> + if not bare and not os.path.exists(os.path.join(outdir, ".git")):
> + raise Fatal("output directory is not a git repository")
> + threshold = capture_or_die("git log -1 --format=%ct").strip()
> + backend.set_after(threshold)
> + if revisionmap:
> + backend.set_revmap(tempfile.mkstemp()[1])
> + markmap = tempfile.mkstemp()[1]
> + if arguments:
> + backend.set_module(arguments[0])
> + gitopts = ""
> + if bare:
> + gitopts += " --bare"
> + if revisionmap:
> + gitopts += " --export-marks='%s'" % markmap
> + if authormap:
> + shutil.copyfile(authormap, metadata("cvs_authors"))
> + if os.path.exists(metadata("cvs-authors")):
> + backend.set_authormap(metadata("cvs-authors"))
> + do_or_die("%s | (cd %s >/dev/null; git fast-import --quiet %s)" \
> + % (backend.command(), outdir, gitopts))
outdir needs to be quoted in the formatted string, i.e.:
"%s | (cd '%s' >/dev/null ..."
Also, I noticed the generated cvs-revisions file now maps cvs
revisions to blobs instead of commits. Was this change intentional?
Thanks,
Chris
^ permalink raw reply
* [RFD] annnotating a pair of commit objects?
From: Junio C Hamano @ 2013-01-03 7:03 UTC (permalink / raw)
To: git
I'd like a datastore that maps a pair of commit object names to
another object name, such that:
* When looking at two commits A and B, efficiently query all data
associated with a pair of commits <X,Y> where X is contained in
the range A..B and not in B..A, and Y is contained in the range
B..A and not in A..B.
* When <X,Y> is registered in the datastore, and X is rewritten to
X' and/or Y is rewritten to Y', the mapping is updated so that it
can be queried with <X',Y'> as a new key, similar to the way a
notes tree that maps object X can be updated to map object X'
when such a rewrite happens.
The intended use case is to "go beyond rerere". Given a history of
this shape:
o---o---o---I mainline
/
O---o---X---o---A topic A
\
o---Y---o---o---B topic B
Suppose in the original O we had a function "distimmed_doshes()" to
tell if doshes are already distimmed, with some call sites. On the
branch leading to A, at commit X, this function was renamed to
"doshes_are_distimmed()", and all existing call sites were adjusted.
On the side branch leading to B, however, at commit Y, a new call
site to the old function was added in a file that was not touched
between O..A at all.
When merging either the topic A or the topic B (but not both) to the
integration branch that did not touch this function or use of it, no
special care needs to be taken, but when merging the second topic
after merging the other one, we need to resolve a semantic conflict.
Namely, the callsite to "distimmed_doshes()" introduced by commit Y
needs to be adjusted to call "doshes_are_distimmed()" instead.
The first step is to recognize the potential issue. When queuing
the topic that contains X and the other topic that contains Y,
suppose I could register <X,Y> to the datastore I am dreaming. When
I merge A to the integration branch, I can notice that there is no
such pair <M,N> in the datastore that:
* M is in A..I and not in I..A
* N is in I..A and not in A..I
and can create a merge J without semantic adjustment.
o---o---o---I---J mainline
/ /
O---o---X---o---A topic A
\
o---Y---o---o---B topic B
When I later merge topic B to the integration branch, however, there
is <X,Y> in the datastore such that:
* X is in B..J and not in J..B
* Y is in J..B and not in B..J
to notice that we need to be careful when creating the merge K:
o---o---o---I---J---K mainline
/ / /
O---o---X---o---A / topic A
\ /
o---Y---o---o---B topic B
Of course, the next step is to store not just one bit "<X,Y> exists
in the datastore--be careful", but what semantic adjustment needs to
be applied [*1*]
Obviously, with O(cnt(A..B))*O(cnt(B..A)) complexity, this can be
trivially coded, by trying all pairs in symmetric difference.
But I am hoping we can do better than that.
Ideas?
[Footnote]
*1* We could do it in multiple ways and the details are not
interesting. A blob object that records a patch that can be applied
with "git apply -3", or a commit object that represents necessary
"evil" change that can be cherry-pick'ed, would be two possible
implementations.
^ permalink raw reply
* Re: [PATCH] Replace git-cvsimport with a rewrite that fixes major bugs.
From: Junio C Hamano @ 2013-01-03 7:08 UTC (permalink / raw)
To: Chris Rorvick; +Cc: esr, git
In-Reply-To: <CAEUsAPYwinmbDkSVu71WJRgUjLfBeNdKDFt6O1f8-Ti9evn6Hw@mail.gmail.com>
Chris Rorvick <chris@rorvick.com> writes:
> outdir needs to be quoted in the formatted string, i.e.:
>
> "%s | (cd '%s' >/dev/null ..."
The issue is real, but I am afraid that the above is not sufficient
because outdir can contain single quotes. I think other places that
call out to external processes share the same issue of being careless
about quoting in general.
Doesn't Python come with a standard subprocess module that lets you
spawn external programs safely, similar to the way Perl's list form
open(), e.g. "open($fh, "-|", 'git', @args)", works?
^ permalink raw reply
* [PATCH] tests: turn on test-lint by default
From: Jeff King @ 2013-01-03 7:17 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Torsten Bögershausen, git
In-Reply-To: <7vhamzyqev.fsf@alter.siamese.dyndns.org>
On Wed, Jan 02, 2013 at 06:02:48PM -0800, Junio C Hamano wrote:
> Torsten Bögershausen <tboegi@web.de> writes:
>
> > When the dust has settled, we can either enable the check always,
> > or mention "make test-lint-shell-syntax" in the Documentation.
>
> In the longer term, I'm pretty much in favor of enabling all the
> checks that are cheap by default, as that would help people catch
> easy mistakes while preparing their patches. People do not tend to
> enable any check if it were optional.
That is fine with me, and I always intended that we turn the lint on by
default at some point (I'm not really sure why we didn't -- looking at
the list archives, I think I did not push it because it seemed like
nobody was really that interested).
Certainly the two existing checks are cheap and do not produce false
positives, and should be safe to turn on. Like this:
-- >8 --
Subject: [PATCH] tests: turn on test-lint by default
The test Makefile knows about a few "lint" checks for common
errors. However, they are not enabled as part of "make test"
by default, which means that many people do not bother
running them. Since they are both quick to run and accurate
(i.e., no false positives), there should be no harm in
turning them on and helping submitters catch errors earlier.
We could just set:
TEST_LINT = test-lint
to enable all tests. But that would be unnecessarily
annoying later on if we add slower or less accurate tests
that should not be part of the default. Instead, we name the
tests individually.
Signed-off-by: Jeff King <peff@peff.net>
---
t/Makefile | 1 +
1 file changed, 1 insertion(+)
diff --git a/t/Makefile b/t/Makefile
index 3025418..5c6de81 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -13,6 +13,7 @@ DEFAULT_TEST_TARGET ?= test
RM ?= rm -f
PROVE ?= prove
DEFAULT_TEST_TARGET ?= test
+TEST_LINT ?= test-lint-duplicates test-lint-executable
# Shell quote;
SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH))
--
1.8.1.rc3.4.gf3a2f57
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox