* 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: 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
* [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: 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
* 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
* [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 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
* 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: 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
* [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
* [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
* Re: [PATCH] Replace git-cvsimport with a rewrite that fixes major bugs.
From: Eric S. Raymond @ 2013-01-02 22:28 UTC (permalink / raw)
To: Martin Langhoff; +Cc: Jonathan Nieder, Junio C Hamano, Git Mailing List
In-Reply-To: <CACPiFCKNkpaf6CgU=5rn1dyUSG2KV43oeTKJgRsSh9-Rhtq3Kw@mail.gmail.com>
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.
> My best guess is that you haven't dealt with enough ugly CVS repos. I
> used to have the old original X.org repos, but no more. Surely
> Mozilla's fugly old CVS repos are up somewhere, and may be
> therapeutic.
Thanks, but since I wrote reposurgeon in 2010 I've done more conversions
of messy CVS and Subversion repositories than I can easily remember (the
Subversion ones being relevant because they often have truly nasty CVS
artifacts in their early history). Just off the top of my head there's
been gpsd, the Network Utility Tools, Roundup, SSTK2000, the Hercules
project, and robotfindskitten. And a raft of smaller projects - I sought
them out as torture tests for reposurgeon.
I am therefore intimately, painfully familiar with how bad CVS repos
can get. I take it as given that there are still boojums that will
perplex my tools lurking out there in the unexplored jungle.
In fact, this very kind of prior experience had been a major
motivation for reposurgeon. I became convinced several years back
that the batchy design philosophy of conventional repo-conversion
tools was flawed, not flexible enough to deal with the real-world
messes out there. So I wrote reposurgeon to amplify human judgment
rather than try to replace it.
An example of the batchiness mistake close to home is the -m and -M
options in the old version of cvsimport. It takes human judgment
looking at the whole commit DAG in gitspace to decide what merge
points would best express the (as you say, sometimes ambiguous) CVS
history - what's needed is a scalpel and sutures in a surgeon's hand,
not a regexp hammer.
For extended discussion, see my blog post "Repositories In
Translation" at http://esr.ibiblio.org/?p=3859 in which I argue that
the process has much more in common with the ambiguity of literary
translation than is normally understood.
No, what I am very confident about is the performance and stability of
the new cvsps/cvsimport code on *the cases the old code handled* - and
a fairly well-defined larger group of many more cases.
My confidence is derived from having built a test suite that
incorporates and improves on the git-tree tests. I don't have to merely
guess or hope that the new code works better, I can exhibit tests
that demonstrate it.
Among my near-term to-do items are applying those tests to cvs2git and
parsecvs. But I first need to get parsecvs working again; presently, as I've
inherited it, it does not correctly create a HEAD reference in the
translated git repo.
--
<a href="http://www.catb.org/~esr/">Eric S. Raymond</a>
^ permalink raw reply
* Re: git filter-branch doesn't dereference annotated tags
From: Grégory Pakosz @ 2013-01-02 22:03 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Johannes Sixt
In-Reply-To: <7vk3rwaa3r.fsf@alter.siamese.dyndns.org>
[-- Attachment #1: Type: text/plain, Size: 2221 bytes --]
> I was wondering if it should be
>
> sha1=$(git rev-parse --verify "$ref")
>
> or something that does not dereference a tag at all.
>
> The way I read what that loop seems to want to do is:
>
> Read each refname that was given originally from the file
> $tempdir/heads, find out the object it used to refer to and
> have it in $sha1, find out what new object the object was
> rewritten to and have it in $rewritten, and:
>
> (1) if the rewrite left the object unchanged, do nothing but
> warn users just in case this was a mistake;
> (2) if the rewrite told us to remove it, then delete the
> ref; or
> (3) if the rewrite gave us a new object, replace the ref to
> point to that new one.
>
> And in the latter two cases, save the original one in
> $orig_namespace so that the user can choose to recover if
> this filter-branch was done by mistake.
>
> So I do not think unwraping the ref at that point makes any sense,
> unless it is not prepared to handle annotated tags at all by
> unwrapping tags too early.
>
> What am I missing?
>
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?
My initial questions and patching suggestions are based on git
1.7.10.4 behavior.
However, playing with git HEAD exhibits a slightly different behavior:
it breaks when invoking git mktag line 459 (introduced by
1bf6551e42c79a594689a356a9b14759d55f3cf5):
error: char7: could not get SHA1 hash
fatal: invalid tag signature file
Could not create new tag object for tag-a
It's basically the same problem. In my opinion, lines 447-466 should
take into account $new_sha1 is empty.
Please forgive me again for not having configured my mailer yet :(
When I'm ready to provide a patch that implements a solution we all
agree with I'll use git send-email.
In the mean time, I would like to pursue the discussion in this mail
thread so please find attached a patch that deletes a tag instead of
invoking the tag-name-filter when it detects $new_sha1 is empty.
I tested the patch doesn't break t7003. What do you think?
Gregory
[-- Attachment #2: 0001-git-filter-branch-allow-deletion-of-tags-when-refere.patch --]
[-- Type: application/octet-stream, Size: 2591 bytes --]
From 2cb9d5bc605cc2f2d8a6603b6a06657516959aa6 Mon Sep 17 00:00:00 2001
From: Gregory Pakosz <gpakosz@visionobjects.com>
Date: Wed, 2 Jan 2013 23:02:03 +0100
Subject: [PATCH] git-filter-branch: allow deletion of tags when referenced
commit gets rewritten to nothing
---
git-filter-branch.sh | 61 ++++++++++++++++++++++++++++------------------------
1 file changed, 33 insertions(+), 28 deletions(-)
diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index 5314249..2e8569c 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -436,36 +436,41 @@ if [ "$filter_tag_name" ]; then
[ -f "../map/$sha1" ] || continue
new_sha1="$(cat "../map/$sha1")"
- GIT_COMMIT="$sha1"
- export GIT_COMMIT
- new_ref="$(echo "$ref" | eval "$filter_tag_name")" ||
- die "tag name filter failed: $filter_tag_name"
-
- echo "$ref -> $new_ref ($sha1 -> $new_sha1)"
-
- if [ "$type" = "tag" ]; then
- new_sha1=$( ( printf 'object %s\ntype commit\ntag %s\n' \
- "$new_sha1" "$new_ref"
- git cat-file tag "$ref" |
- sed -n \
- -e '1,/^$/{
- /^object /d
- /^type /d
- /^tag /d
- }' \
- -e '/^-----BEGIN PGP SIGNATURE-----/q' \
- -e 'p' ) |
- git mktag) ||
- die "Could not create new tag object for $ref"
- if git cat-file tag "$ref" | \
- sane_grep '^-----BEGIN PGP SIGNATURE-----' >/dev/null 2>&1
- then
- warn "gpg signature stripped from tag object $sha1t"
+ if [ -n "$new_sha1" ]; then
+ GIT_COMMIT="$sha1"
+ export GIT_COMMIT
+ new_ref="$(echo "$ref" | eval "$filter_tag_name")" ||
+ die "tag name filter failed: $filter_tag_name"
+
+ echo "$ref -> $new_ref ($sha1 -> $new_sha1)"
+
+ if [ "$type" = "tag" ]; then
+ new_sha1=$( ( printf 'object %s\ntype commit\ntag %s\n' \
+ "$new_sha1" "$new_ref"
+ git cat-file tag "$ref" |
+ sed -n \
+ -e '1,/^$/{
+ /^object /d
+ /^type /d
+ /^tag /d
+ }' \
+ -e '/^-----BEGIN PGP SIGNATURE-----/q' \
+ -e 'p' ) |
+ git mktag) ||
+ die "Could not create new tag object for $ref"
+ if git cat-file tag "$ref" | \
+ sane_grep '^-----BEGIN PGP SIGNATURE-----' >/dev/null 2>&1
+ then
+ warn "gpg signature stripped from tag object $sha1t"
+ fi
fi
- fi
- git update-ref "refs/tags/$new_ref" "$new_sha1" ||
- die "Could not write tag $new_ref"
+ git update-ref "refs/tags/$new_ref" "$new_sha1" ||
+ die "Could not write tag $new_ref"
+ else
+ git update-ref -d "refs/tags/$ref" "$sha1t" ||
+ die "Could not delete tag $ref"
+ fi
done
fi
--
1.8.0.1
^ permalink raw reply related
* Re: Test failures with python versions when building git 1.8.1
From: Dan McGee @ 2013-01-02 21:25 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jeff King, GIT Mailing-list, Florian Achleitner,
David Michael Barr, Eric S. Raymond
In-Reply-To: <7vehi34k5s.fsf@alter.siamese.dyndns.org>
On Wed, Jan 2, 2013 at 10:35 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Dan McGee <dan@archlinux.org> writes:
>
>> This works great now, thanks! I ran it through our package build
>> scripts and all tests now pass as expected.
>
> If you have a chance, could you try tip of the 'next' branch without
> this patch applied? We had an equivalent patch cooking there for
> some time by now.
Yeah, this worked great as well. Thanks again everyone.
-Dan
^ permalink raw reply
* Re: [PATCH] merge: Honor prepare-commit-msg return code
From: Junio C Hamano @ 2013-01-02 21:21 UTC (permalink / raw)
To: Antoine Pelisse; +Cc: git, Jay Soffian
In-Reply-To: <CALWbr2wWjwUnHFq1icMRuW=vjQDhTO1e_chffqUvDWY5za1Kiw@mail.gmail.com>
Antoine Pelisse <apelisse@gmail.com> writes:
>>> prepare-commit-msg hook is run when committing to prepare the log
>>> message. If the exit-status is non-zero, the commit should be aborted.
>>
>> I was scratching my head why you CC'ed Jay, until I dug up 65969d4
>> (merge: honor prepare-commit-msg hook, 2011-02-14).
>
> I did as suggested in "SubmittingPatches" :)
Oh, that wasn't meant as a complaint. I am tempted to rewrite the
log message like so, though:
65969d4 (merge: honor prepare-commit-msg hook, 2011-02-14) tried to
make "git commit" and "git merge" consistent, because a merge that
required user assistance has to be concluded with "git commit", but
only "git commit" triggered prepare-commit-msg hook. When it added
a call to run the prepare-commit-msg hook, however, it forgot to
check the exit code from the hook like "git commit" does, and ended
up replacing one inconsistency with another.
>> diff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh
>> index bc497bc..3573751 100755
>> --- a/t/t7505-prepare-commit-msg-hook.sh
>> +++ b/t/t7505-prepare-commit-msg-hook.sh
>> @@ -172,11 +172,12 @@ test_expect_success 'with failing hook (merge)' '
>> git checkout -B other HEAD@{1} &&
>> echo "more" >> file &&
>> git add file &&
>> - chmod -x $HOOK &&
>> + rm -f "$HOOK" &&
>> git commit -m other &&
>> - chmod +x $HOOK &&
>> + write_script "$HOOK" <<-EOF
>> + exit 1
>> + EOF
>> git checkout - &&
>> - head=`git rev-parse HEAD` &&
>> test_must_fail git merge other
>>
>> '
>
> What about moving the hook file then ? Not very important to me, just
> a suggestion as it would keep the shebang.
Strictly speaking, the way $HOOK is prepared in the original is
wrong. The script is always run under "#!/bin/sh" instead of the
shell the user told us to use with $SHELL_PATH. For a simple one
liner that only exits with 1, it does not matter, though.
Many test scripts got this wrong and that was the reason we later
added write_script helper function to the test suite.
^ permalink raw reply
* Re: [PATCH] Replace git-cvsimport with a rewrite that fixes major bugs.
From: Martin Langhoff @ 2013-01-02 21:15 UTC (permalink / raw)
To: Eric Raymond; +Cc: Jonathan Nieder, Junio C Hamano, Git Mailing List
In-Reply-To: <20130102164107.GA19006@thyrsus.com>
On Wed, Jan 2, 2013 at 11:41 AM, Eric S. Raymond <esr@thyrsus.com> wrote:
> Martin Langhoff <martin.langhoff@gmail.com>:
>> Replacement with something more solid is welcome, but until you are
>> extremely confident of its handling of legacy setups... I would still
>> provide the old cvsimport, perhaps in contrib.
>
> I am extremely confident. I built a test suite so I could be.
This is rather off-putting. Really.
I dealt with enough CVS repos to see that the branch point could be
ambiguous, and that some cases were incurably ugly and ambiguous.
Off the top of my head I can recall
- Files created on a branch appear on HEAD (if the cvs client was
well behaved, in HEAD's attic, if the cvs client was buggy... )
- Files tagged with the branch at a much later time. Scenario is a
developer opening/tagging a new branch mindlessly on a partial
checkout; then trying to "fix" the problem later.
My best guess is that you haven't dealt with enough ugly CVS repos. I
used to have the old original X.org repos, but no more. Surely
Mozilla's fugly old CVS repos are up somewhere, and may be
therapeutic.
cheers,
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 v3] git-clean: Display more accurate delete messages
From: Junio C Hamano @ 2013-01-02 21:10 UTC (permalink / raw)
To: Zoltan Klinger; +Cc: git
In-Reply-To: <7vfw2j2vlp.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Zoltan Klinger <zoltan.klinger@gmail.com> writes:
>
>> +static const char* MSG_REMOVE = "Removing %s\n";
>> +static const char* MSG_WOULD_REMOVE = "Would remove %s\n";
>> +static const char* MSG_WOULD_NOT_REMOVE = "Would not remove %s\n";
I also noticed that this message is not used, which mwans that the
program used to say "Would not remove" for some paths but the
updated one will never do so.
^ permalink raw reply
* Re: [PATCH] merge: Honor prepare-commit-msg return code
From: Antoine Pelisse @ 2013-01-02 21:02 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jay Soffian
In-Reply-To: <7v623f2uam.fsf@alter.siamese.dyndns.org>
>> prepare-commit-msg hook is run when committing to prepare the log
>> message. If the exit-status is non-zero, the commit should be aborted.
>
> I was scratching my head why you CC'ed Jay, until I dug up 65969d4
> (merge: honor prepare-commit-msg hook, 2011-02-14).
I did as suggested in "SubmittingPatches" :)
>> +test_expect_success 'with failing hook (merge)' '
>> +
>> + git checkout -B other HEAD@{1} &&
>> + echo "more" >> file &&
>> + git add file &&
>> + chmod -x $HOOK &&
>
> I have a feeling that this will break folks without POSIXPERM
> prerequisite.
It felt wrong when I did it, but kept it consistent within the file.
It indeed looks older than other test files I've seen so far but I
don't feel like I know the test general-style-and-policy enough to fix
it myself either.
> diff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh
> index bc497bc..3573751 100755
> --- a/t/t7505-prepare-commit-msg-hook.sh
> +++ b/t/t7505-prepare-commit-msg-hook.sh
> @@ -172,11 +172,12 @@ test_expect_success 'with failing hook (merge)' '
> git checkout -B other HEAD@{1} &&
> echo "more" >> file &&
> git add file &&
> - chmod -x $HOOK &&
> + rm -f "$HOOK" &&
> git commit -m other &&
> - chmod +x $HOOK &&
> + write_script "$HOOK" <<-EOF
> + exit 1
> + EOF
> git checkout - &&
> - head=`git rev-parse HEAD` &&
> test_must_fail git merge other
>
> '
What about moving the hook file then ? Not very important to me, just
a suggestion as it would keep the shebang.
Cheers,
^ permalink raw reply
* Re: Test failures with python versions when building git 1.8.1
From: Jeff King @ 2013-01-02 20:54 UTC (permalink / raw)
To: Junio C Hamano
Cc: Dan McGee, GIT Mailing-list, Florian Achleitner,
David Michael Barr, Eric S. Raymond
In-Reply-To: <7vip7f4k7x.fsf@alter.siamese.dyndns.org>
On Wed, Jan 02, 2013 at 08:34:42AM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > Whether we end up doing something with contrib and tests or not, the
> > patch below gives a minimal fix in the meantime.
>
> Replacing the symbolic link with write_script that uses exported
> variables looks like a familiar pattern. I like it.
>
> Oh, wait. That pattern is of course familiar, because 5a02966
> (t9020: use configured Python to run the test helper, 2012-12-18)
> has been in 'next', and is planned for the first batch.
Great minds think alike, I guess?
Would have been nice to mention that you had done a patch when you
pointed to the unproductive thread. :P But I think you can take the
similarity of our patches and commit messages as my endorsement of
5a02966. :)
-Peff
^ permalink raw reply
* Re: [PATCH v2] build: do not automatically reconfigure unless configure.ac changed
From: Stefano Lattarini @ 2013-01-02 20:53 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jonathan Nieder, Jeff King, Martin von Zweigbergk, git
In-Reply-To: <7vbod72uze.fsf@alter.siamese.dyndns.org>
On 01/02/2013 09:25 PM, Junio C Hamano wrote:
> Stefano Lattarini <stefano.lattarini@gmail.com> writes:
>
>>> ifdef AUTOCONFIGURED
>>> -config.status: configure
>>> - $(QUIET_GEN)if test -f config.status; then \
>>> +# We avoid depending on 'configure' here, because it gets rebuilt
>>> +# every time GIT-VERSION-FILE is modified, only to update the embedded
>>> +# version number string, which config.status does not care about.
>>>
>> Alas, config.status *do* care about it, in that the '@PACKAGE_VERSION@',
>> '@PACKAGE_STRING@' and '@DEFS@' substitutions are affected by what is
>> hard-coded in configure as the version number [1]. But if we do not
>> use those substitutions in any of our files (and I believe we don't),
>> then *we* can happily not care about the configure embedded version
>> number string, and thus avoid the extra configure runs. Phew.
>>
>> [1] Yes, this is a mess. We know. Sorry!
>
> Heh. Should we warn against the use of these symbols somewhere in
> configure.ac, perhaps, then?
>
Actually, they should be checked against in files processed by
'config.status', i.e., files listed in AC_CONFIG_FILES calls in
'configure.ac'. But I honestly believe that would be overkill;
I say we simply adjust your comment to read something like:
# We avoid depending on 'configure' here, because it gets rebuilt
# every time GIT-VERSION-FILE is modified, only to update the
# embedded version number string, which we however do not
# substitute in any file processed by config.status.
Thanks,
Stefano
^ permalink raw reply
* Re: [PATCH] merge: Honor prepare-commit-msg return code
From: Junio C Hamano @ 2013-01-02 20:40 UTC (permalink / raw)
To: Antoine Pelisse; +Cc: git, Jay Soffian
In-Reply-To: <1357152170-5511-1-git-send-email-apelisse@gmail.com>
Antoine Pelisse <apelisse@gmail.com> writes:
> prepare-commit-msg hook is run when committing to prepare the log
> message. If the exit-status is non-zero, the commit should be aborted.
I was scratching my head why you CC'ed Jay, until I dug up 65969d4
(merge: honor prepare-commit-msg hook, 2011-02-14).
> +test_expect_success 'with failing hook (merge)' '
> +
> + git checkout -B other HEAD@{1} &&
> + echo "more" >> file &&
> + git add file &&
> + chmod -x $HOOK &&
I have a feeling that this will break folks without POSIXPERM
prerequisite.
How about doing it this way instead? This old test script seems to
want a lot of style clean-ups, but I refrained from doing any in
this fixlet.
Thanks.
t/t7505-prepare-commit-msg-hook.sh | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh
index bc497bc..3573751 100755
--- a/t/t7505-prepare-commit-msg-hook.sh
+++ b/t/t7505-prepare-commit-msg-hook.sh
@@ -172,11 +172,12 @@ test_expect_success 'with failing hook (merge)' '
git checkout -B other HEAD@{1} &&
echo "more" >> file &&
git add file &&
- chmod -x $HOOK &&
+ rm -f "$HOOK" &&
git commit -m other &&
- chmod +x $HOOK &&
+ write_script "$HOOK" <<-EOF
+ exit 1
+ EOF
git checkout - &&
- head=`git rev-parse HEAD` &&
test_must_fail git merge other
'
^ permalink raw reply related
* Re: [PATCH v2] build: do not automatically reconfigure unless configure.ac changed
From: Junio C Hamano @ 2013-01-02 20:25 UTC (permalink / raw)
To: Stefano Lattarini; +Cc: Jonathan Nieder, Jeff King, Martin von Zweigbergk, git
In-Reply-To: <50E48BF6.2020900@gmail.com>
Stefano Lattarini <stefano.lattarini@gmail.com> writes:
>> ifdef AUTOCONFIGURED
>> -config.status: configure
>> - $(QUIET_GEN)if test -f config.status; then \
>> +# We avoid depending on 'configure' here, because it gets rebuilt
>> +# every time GIT-VERSION-FILE is modified, only to update the embedded
>> +# version number string, which config.status does not care about.
>>
> Alas, config.status *do* care about it, in that the '@PACKAGE_VERSION@',
> '@PACKAGE_STRING@' and '@DEFS@' substitutions are affected by what is
> hard-coded in configure as the version number [1]. But if we do not
> use those substitutions in any of our files (and I believe we don't),
> then *we* can happily not care about the configure embedded version
> number string, and thus avoid the extra configure runs. Phew.
>
> [1] Yes, this is a mess. We know. Sorry!
Heh. Should we warn against the use of these symbols somewhere in
configure.ac, perhaps, then?
^ permalink raw reply
* Re: [PATCH v3] git-clean: Display more accurate delete messages
From: Junio C Hamano @ 2013-01-02 20:11 UTC (permalink / raw)
To: Zoltan Klinger; +Cc: git
In-Reply-To: <1357091159-22080-1-git-send-email-zoltan.klinger@gmail.com>
Zoltan Klinger <zoltan.klinger@gmail.com> writes:
> +static const char* MSG_REMOVE = "Removing %s\n";
> +static const char* MSG_WOULD_REMOVE = "Would remove %s\n";
> +static const char* MSG_WOULD_NOT_REMOVE = "Would not remove %s\n";
> +static const char* MSG_WOULD_IGNORE_GIT_DIR = "Would ignore untracked git repository %s\n";
> +static const char* MSG_WARN_GIT_DIR_IGNORE = "ignoring untracked git repository %s";
> +static const char* MSG_WARN_REMOVE_FAILED = "failed to remove %s";
"foo* bar" should be "foo *bar". Also I personally find these
upcased message constants somewhat hard to read in the sites of use
in the code below.
Also gettext machinery needs to be told that these strings may be
asked to be replaced with their translations using _() elsewhere in
the code. I.e.
static const char *msg_remove = N_("Removing %s\n");
Aren't WOULD_IGNORE_GIT_DIR and WARN_GIT_DIR_IGNORE named somewhat
inconsistently? Perhaps the latter is WARN_IGNORED_GIT_DIR or
something?
> @@ -34,11 +42,109 @@ static int exclude_cb(const struct option *opt, const char *arg, int unset)
> return 0;
> }
>
> +static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,
> + int dry_run, int quiet, int *dir_gone)
> +{
> + DIR *dir;
> + struct strbuf quoted = STRBUF_INIT;
> + struct dirent *e;
> + int res = 0, ret = 0, gone = 1, original_len = path->len, len, i;
> + unsigned char submodule_head[20];
> + struct string_list dels = STRING_LIST_INIT_DUP;
> +
> + *dir_gone = 1;
> +
> + quote_path_relative(path->buf, strlen(path->buf), "ed, prefix);
Shouldn't this be inside the next if() statement body? I also think
you could even omit this call when (!dry_run && quiet). The same
comment applies to all uses of quote_path_relative() in this patch,
including the ones that were kept from the original in clean.c,
which made sense because they were used in all if/else bodies that
followed them but this patch makes it no longer true.
> + if ((force_flag & REMOVE_DIR_KEEP_NESTED_GIT) &&
> + !resolve_gitlink_ref(path->buf, "HEAD", submodule_head)) {
> + if (dry_run && !quiet)
> + printf(_(MSG_WOULD_IGNORE_GIT_DIR), quoted.buf);
> + else if (!dry_run)
> + warning(_(MSG_WARN_GIT_DIR_IGNORE), quoted.buf);
> +
> + *dir_gone = 0;
> + return 0;
> + }
> +
> + dir = opendir(path->buf);
> + if (!dir) {
> + /* an empty dir could be removed even if it is unreadble */
> + res = dry_run ? 0 : rmdir(path->buf);
> + if (res) {
> + warning(_(MSG_WARN_REMOVE_FAILED), quoted.buf);
> + *dir_gone = 0;
> + }
> + return res;
> + }
> +
> + if (path->buf[original_len - 1] != '/')
> + strbuf_addch(path, '/');
> +
> + len = path->len;
> + while ((e = readdir(dir)) != NULL) {
> + struct stat st;
> + if (is_dot_or_dotdot(e->d_name))
> + continue;
> +
> + strbuf_setlen(path, len);
> + strbuf_addstr(path, e->d_name);
> + quote_path_relative(path->buf, strlen(path->buf), "ed, prefix);
> + if (lstat(path->buf, &st))
> + ; /* fall thru */
> + else if (S_ISDIR(st.st_mode)) {
> + if (remove_dirs(path, prefix, force_flag, dry_run, quiet, &gone))
> + ret = 1;
> + if (gone)
> + string_list_append(&dels, quoted.buf);
> + else
> + *dir_gone = 0;
> + continue;
> + } else {
> + res = dry_run ? 0 : unlink(path->buf);
> + if (!res)
> + string_list_append(&dels, quoted.buf);
> + else {
> + warning(_(MSG_WARN_REMOVE_FAILED), quoted.buf);
> + *dir_gone = 0;
> + ret = 1;
> + }
> + continue;
> + }
> +
> + /* path too long, stat fails, or non-directory still exists */
> + *dir_gone = 0;
> + ret = 1;
> + break;
> + }
> + closedir(dir);
> +
> + strbuf_setlen(path, original_len);
> + quote_path_relative(path->buf, strlen(path->buf), "ed, prefix);
> + if (*dir_gone) {
> + res = dry_run ? 0 : rmdir(path->buf);
> + if (!res)
> + *dir_gone = 1;
> + else {
> + warning(_(MSG_WARN_REMOVE_FAILED), quoted.buf);
> + *dir_gone = 0;
> + ret = 1;
> + }
> + }
> +
> + if (!*dir_gone && !quiet) {
> + for (i = 0; i < dels.nr; i++)
> + printf(dry_run ? _(MSG_WOULD_REMOVE) : _(MSG_REMOVE), dels.items[i].string);
> + }
> + string_list_clear(&dels, 0);
> + return ret;
> +}
> int cmd_clean(int argc, const char **argv, const char *prefix)
> {
> - int i;
> - int show_only = 0, remove_directories = 0, quiet = 0, ignored = 0;
> - int ignored_only = 0, config_set = 0, errors = 0;
> + int i, res;
> + int dry_run = 0, remove_directories = 0, quiet = 0, ignored = 0;
> + int ignored_only = 0, config_set = 0, errors = 0, gone = 1;
> int rm_flags = REMOVE_DIR_KEEP_NESTED_GIT;
> struct strbuf directory = STRBUF_INIT;
> struct dir_struct dir;
> @@ -49,7 +155,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
> char *seen = NULL;
> struct option options[] = {
> OPT__QUIET(&quiet, N_("do not print names of files removed")),
> - OPT__DRY_RUN(&show_only, N_("dry run")),
> + OPT__DRY_RUN(&dry_run, N_("dry run")),
> OPT__FORCE(&force, N_("force")),
> OPT_BOOLEAN('d', NULL, &remove_directories,
> N_("remove whole directories")),
> @@ -77,7 +183,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
> if (ignored && ignored_only)
> die(_("-x and -X cannot be used together"));
>
> - if (!show_only && !force) {
> + if (!dry_run && !force) {
> if (config_set)
> die(_("clean.requireForce set to true and neither -n nor -f given; "
> "refusing to clean"));
> @@ -150,38 +256,23 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
> if (S_ISDIR(st.st_mode)) {
> strbuf_addstr(&directory, ent->name);
> qname = quote_path_relative(directory.buf, directory.len, &buf, prefix);
> + if (remove_directories || (matches == MATCHED_EXACTLY)) {
> + if (remove_dirs(&directory, prefix, rm_flags, dry_run, quiet, &gone))
> errors++;
> + if (gone && !quiet)
> + printf(dry_run ? _(MSG_WOULD_REMOVE) : _(MSG_REMOVE), qname);
> }
> strbuf_reset(&directory);
> } else {
> if (pathspec && !matches)
> continue;
> qname = quote_path_relative(ent->name, -1, &buf, prefix);
> + res = dry_run ? 0 : unlink(ent->name);
> + if (res) {
> + warning(_(MSG_WARN_REMOVE_FAILED), qname);
> errors++;
> - }
> + } else if (!quiet)
> + printf(dry_run ? _(MSG_WOULD_REMOVE) :_(MSG_REMOVE), qname);
spaces required around that ':' (ctx:WxV)
#313: FILE: builtin/clean.c:275:
+ printf(dry_run ? _(MSG_WOULD_REMOVE) :_(MSG_REMOVE), qname);
> }
> }
> free(seen);
The updated code structure is much nicer than the previous round,
but I am somewhat puzzled how return value of remove_dirs() and
&gone relate to each other. Surely when gone is set to zero,
remove_dirs() is reporting that the directory it was asked to remove
recursively did not go away, so it must report failure, no? Having
the &gone flag looks redundant and checking for gone in some places
while checking for the return value for others feels like an
invitation for future bugs.
Also the remove_dirs() function seems to replace the use of
remove_dir_recurse() from dir.c by copying large part of it, with
error message sprinkled. Does remove_dir_recurse() still get used
by other codepaths? If so, do the remaining callsites benefit from
using this updated version?
Thanks; will replace what has been sitting on the 'pu' branch with
this copy.
^ permalink raw reply
* Re: [PATCH v2] build: do not automatically reconfigure unless configure.ac changed
From: Stefano Lattarini @ 2013-01-02 19:35 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jonathan Nieder, Jeff King, Martin von Zweigbergk, git
In-Reply-To: <7va9sr4jgu.fsf@alter.siamese.dyndns.org>
On 01/02/2013 05:50 PM, Junio C Hamano wrote:
> Stefano Lattarini <stefano.lattarini@gmail.com> writes:
>
>> On 01/02/2013 09:48 AM, Jonathan Nieder wrote:
>>> Jeff King wrote:
>>>
>>>> It seems I am late to the party. But FWIW, this looks the most sane to
>>>> me of the patches posted in this thread.
>>> ...
>> FYI, this seems a sane approach to me....
>> The only nit I have to offer is that I'd like to see more comments in
>> the git Makefile about why this "semi-hack" is needed.
>
> Thanks, everybody.
>
> Please eyeball the below for (hopefully) the last time, to be
> eventually merged to maint-1.7.12, maint-1.8.0 and maint (aka
> maint-1.8.1) branches.
>
> -- >8 --
> From: Jonathan Nieder <jrnieder@gmail.com>
> Date: Wed, 2 Jan 2013 00:25:44 -0800
> Subject: [PATCH] build: do not automatically reconfigure unless configure.ac changed
>
> Starting with v1.7.12-rc0~4^2 (build: reconfigure automatically if
> configure.ac changes, 2012-07-19), "config.status --recheck" is
> automatically run every time the "configure" script changes. In
> particular, that means the configuration procedure repeats whenever
> the version number changes (since the configure script changes to
> support "./configure --version" and "./configure --help"), making
> bisecting painfully slow.
>
> The intent was to make the reconfiguration process only trigger for
> changes to configure.ac's logic. Tweak the Makefile rule to match
> that intent by depending on configure.ac instead of configure.
>
> Reported-by: Martin von Zweigbergk <martinvonz@gmail.com>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> Reviewed-by: Jeff King <peff@peff.net>
> Reviewed-by: Stefano Lattarini <stefano.lattarini@gmail.com>
> ---
> Makefile | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 26b697d..2f5e2ab 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2167,8 +2167,14 @@ configure: configure.ac GIT-VERSION-FILE
> $(RM) $<+
>
> ifdef AUTOCONFIGURED
> -config.status: configure
> - $(QUIET_GEN)if test -f config.status; then \
> +# We avoid depending on 'configure' here, because it gets rebuilt
> +# every time GIT-VERSION-FILE is modified, only to update the embedded
> +# version number string, which config.status does not care about.
>
Alas, config.status *do* care about it, in that the '@PACKAGE_VERSION@',
'@PACKAGE_STRING@' and '@DEFS@' substitutions are affected by what is
hard-coded in configure as the version number [1]. But if we do not
use those substitutions in any of our files (and I believe we don't),
then *we* can happily not care about the configure embedded version
number string, and thus avoid the extra configure runs. Phew.
[1] Yes, this is a mess. We know. Sorry!
> +# We
> +# do want to recheck when the platform/environment detection logic
> +# changes, hence this depends on configure.ac.
> +config.status: configure.ac
> + $(QUIET_GEN)$(MAKE) configure && \
> + if test -f config.status; then \
> ./config.status --recheck; \
> else \
> ./configure; \
HTH,
Stefano
^ permalink raw reply
* Re: [PATCH] Replace git-cvsimport with a rewrite that fixes major bugs.
From: Junio C Hamano @ 2013-01-02 19:07 UTC (permalink / raw)
To: esr; +Cc: git
In-Reply-To: <20130102183710.GB19006@thyrsus.com>
"Eric S. Raymond" <esr@thyrsus.com> writes:
> Junio C Hamano <gitster@pobox.com>:
>> As your version already knows how to detect the case where cvsps is
>> too old to operate with it, I imagine it to be straight-forward to
>> ship the old cvsimport under obscure name, "git cvsimport--old" or
>> something, and spawn it from your version when necessary, perhaps
>> after issuing a warning "cvsps 3.0 not found; switching to an old
>> and unmaintained version of cvsimport..."
>
> This can be done. As this may not be the last case in which it comes up,
> perhaps we should have an 'obsolete' directory distinct from 'contrib'.
>
> I'll ship another patch.
Alright; thanks.
Don't forget to sign-off 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