[PATCH] builtin-commit.c: Not add duplicate S-o-b when commit
Scan the whole rfc2822 footer for duplicate S-o-b, not just the last
line of the commit message.
A commit may have multiple S-o-bs, or other tags, such as:
some commit log...
Signed-off-by: C O Mitter commit...@example.com
Reported-by: R E Porter repor...@example.com
Because the S-o-b is not located at the last line in the above commit,
when the above commit is amended by the original committer, a
duplicated S-o-b may appended by accident. New commit log may looks
like:
some commit log...
Signed-off-by: C O Mitter commit...@example.com
Reported-by: R E Porter repor...@example.com
Signed-off-by: C O Mitter commit...@example.com
This hack scans the whole rfc2822 footer for duplicate S-o-b, and only
append a S-o-b when necessary. Also add testcases in 't/t7502-commit.sh'
for this.
Signed-off-by: Jiang Xin worldhello@gmail.com
---
builtin/commit.c | 28
t/t7502-commit.sh | 19 +++
2 files changed, 43 insertions(+), 4 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index 20cef..1a3da 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -704,15 +704,35 @@ static int prepare_to_commit(const char *index_file,
const char *prefix,
if (signoff) {
struct strbuf sob = STRBUF_INIT;
int i;
+ int hit_footer = 0;
+ int hit_sob = 0;
strbuf_addstr(sob, sign_off_header);
strbuf_addstr(sob, fmt_name(getenv(GIT_COMMITTER_NAME),
getenv(GIT_COMMITTER_EMAIL)));
strbuf_addch(sob, '\n');
- for (i = sb.len - 1; i 0 sb.buf[i - 1] != '\n'; i--)
- ; /* do nothing */
- if (prefixcmp(sb.buf + i, sob.buf)) {
- if (!i || !ends_rfc2822_footer(sb))
+ for (i = sb.len - 1; i 0; i--) {
+ if (hit_footer sb.buf[i] == '\n') {
+ hit_footer = 2;
+ i += 2;
+ break;
+ }
+ hit_footer = (sb.buf[i] == '\n');
+ }
+ hit_footer = (2 == hit_footer);
+ if (hit_footer) {
+ while (i sb.len)
+ {
+ if (!prefixcmp(sb.buf + i, sob.buf)) {
+ hit_sob = 1;
+ break;
+ }
+ while (i sb.len sb.buf[i++] != '\n')
+ ; /* do nothing */
+ }
+ }
+ if (!hit_sob) {
+ if (!hit_footer || !ends_rfc2822_footer(sb))
strbuf_addch(sb, '\n');
strbuf_addbuf(sb, sob);
}
diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
index 18145..8198f 100755
--- a/t/t7502-commit.sh
+++ b/t/t7502-commit.sh
@@ -336,7 +336,26 @@ test_expect_success 'A single-liner subject with a token
plus colon is not a foo
git commit -s -m hello: kitty --allow-empty
git cat-file commit HEAD | sed -e 1,/^$/d actual
test_line_count = 3 actual
+'
+
+cat expect EOF
+Footer-like: commit log
+
+Signed-off-by: C O Mitter commit...@example.com
+EOF
+
+test_expect_success 'S-o-b after footer-like commit message' '
+ head -1 expect | git commit -s --allow-empty -F -
+ git cat-file commit HEAD | sed 1,/^\$/d output
+ test_cmp expect output
+'
+
+echo Reported-by: R E Porter repor...@example.com expect
+test_expect_success 'no duplicate S-o-b when signoff' '
+ cat expect | git commit -s --allow-empty -F -
+ git cat-file commit HEAD | sed 1,/^\$/d output
+ test_cmp expect output
'
cat .git/FAKE_EDITOR EOF
--
1.7.12.rc0.28.g8ecd8a5.dirty
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] difftool: Handle compare() returning -1
Keep the temporary directory around when compare()
cannot read its input files, which is indicated by -1.
Defer tempdir creation to allow an early exit in setup_dir_diff().
Wrap the rest of the entry points in an exit_cleanup() function
to handle removing temporary files and error reporting.
Print the temporary files' location so that the user can
recover them.
Signed-off-by: David Aguilar dav...@gmail.com
---
Replaces difftool: Check all return codes from compare(),
which was the original permutation of this patch.
This ended up touching much more than the original patch
since it now handles every exit point, but it's more thorough
and complete.
git-difftool.perl | 100 +-
1 file changed, 68 insertions(+), 32 deletions(-)
diff --git a/git-difftool.perl b/git-difftool.perl
index 8e51238..5ed0b3a 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -18,7 +18,7 @@ use File::Copy;
use File::Compare;
use File::Find;
use File::stat;
-use File::Path qw(mkpath);
+use File::Path qw(mkpath rmtree);
use File::Temp qw(tempdir);
use Getopt::Long qw(:config pass_through);
use Git;
@@ -112,6 +112,17 @@ EOF
exit(0);
}
+sub exit_cleanup
+{
+ my ($tmpdir, $status) = @_;
+ rmtree($tmpdir);
+ if ($status and $!) {
+ my ($package, $file, $line) = caller();
+ warn $file line $line: $!\n;
+ }
+ exit($status | ($status 8));
+}
+
sub setup_dir_diff
{
my ($repo, $workdir, $symlinks) = @_;
@@ -128,13 +139,6 @@ sub setup_dir_diff
my $diffrtn = $diffrepo-command_oneline(@gitargs);
exit(0) if (length($diffrtn) == 0);
- # Setup temp directories
- my $tmpdir = tempdir('git-difftool.X', CLEANUP = 1, TMPDIR = 1);
- my $ldir = $tmpdir/left;
- my $rdir = $tmpdir/right;
- mkpath($ldir) or die $!;
- mkpath($rdir) or die $!;
-
# Build index info for left and right sides of the diff
my $submodule_mode = '16';
my $symlink_mode = '12';
@@ -203,6 +207,13 @@ EOF
}
}
+ # Setup temp directories
+ my $tmpdir = tempdir('git-difftool.X', CLEANUP = 0, TMPDIR = 1);
+ my $ldir = $tmpdir/left;
+ my $rdir = $tmpdir/right;
+ mkpath($ldir) or exit_cleanup($tmpdir, 1);
+ mkpath($rdir) or exit_cleanup($tmpdir, 1);
+
# If $GIT_DIR is not set prior to calling 'git update-index' and
# 'git checkout-index', then those commands will fail if difftool
# is called from a directory other than the repo root.
@@ -219,16 +230,18 @@ EOF
$repo-command_input_pipe(qw(update-index -z --index-info));
print($inpipe $lindex);
$repo-command_close_pipe($inpipe, $ctx);
+
my $rc = system('git', 'checkout-index', '--all', --prefix=$ldir/);
- exit($rc | ($rc 8)) if ($rc != 0);
+ exit_cleanup($tmpdir, $rc) if $rc != 0;
$ENV{GIT_INDEX_FILE} = $tmpdir/rindex;
($inpipe, $ctx) =
$repo-command_input_pipe(qw(update-index -z --index-info));
print($inpipe $rindex);
$repo-command_close_pipe($inpipe, $ctx);
+
$rc = system('git', 'checkout-index', '--all', --prefix=$rdir/);
- exit($rc | ($rc 8)) if ($rc != 0);
+ exit_cleanup($tmpdir, $rc) if $rc != 0;
# If $GIT_DIR was explicitly set just for the update/checkout
# commands, then it should be unset before continuing.
@@ -240,14 +253,19 @@ EOF
for my $file (@working_tree) {
my $dir = dirname($file);
unless (-d $rdir/$dir) {
- mkpath($rdir/$dir) or die $!;
+ mkpath($rdir/$dir) or
+ exit_cleanup($tmpdir, 1);
}
if ($symlinks) {
- symlink($workdir/$file, $rdir/$file) or die $!;
+ symlink($workdir/$file, $rdir/$file) or
+ exit_cleanup($tmpdir, 1);
} else {
- copy($workdir/$file, $rdir/$file) or die $!;
+ copy($workdir/$file, $rdir/$file) or
+ exit_cleanup($tmpdir, 1);
+
my $mode = stat($workdir/$file)-mode;
- chmod($mode, $rdir/$file) or die $!;
+ chmod($mode, $rdir/$file) or
+ exit_cleanup($tmpdir, 1);
}
}
@@ -255,27 +273,35 @@ EOF
# temporary file to both the left and right directories to show the
# change in the recorded SHA1 for the submodule.
for my $path (keys %submodule) {
+ my $ok;
if (defined($submodule{$path}{left})) {
- write_to_file($ldir/$path, Subproject commit
$submodule{$path}{left});
+ $ok = write_to_file($ldir/$path,
+ Subproject commit
Test t/t7502-commit.sh failed
Test t/t7502-commit.sh failed. I guess it's commit v1.7.9.7-1-gf20f387 which breaks it. $ git log -1 --oneline --stat v1.7.9.7-1-gf20f387 f20f commit: check committer identity more strictly builtin/commit.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) Result of t/t7502-commit.sh: not ok - 21 committer is automatic # # # echo negative # ( # sane_unset GIT_COMMITTER_EMAIL # sane_unset GIT_COMMITTER_NAME # # must fail because there is no change # test_must_fail git commit -e -m sample # ) # head -n 8 .git/COMMIT_EDITMSG | \ # sed s/^# Committer: .*/# Committer:/ actual # test_i18ncmp expect actual # Contents of file expect: sample # Please enter the commit message for your changes. Lines starting # with '#' will be ignored, and an empty message aborts the commit. # # Author:A U Thor aut...@example.com # Committer: # Contents of file actual: sample -- Jiang Xin -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] builtin-commit.c: Not add duplicate S-o-b when commit
Jiang Xin worldhello@gmail.com writes: Scan the whole rfc2822 footer for duplicate S-o-b, not just the last line of the commit message. A commit may have multiple S-o-bs, or other tags, such as: some commit log... Signed-off-by: C O Mitter commit...@example.com Reported-by: R E Porter repor...@example.com Because the S-o-b is not located at the last line in the above commit, when the above commit is amended by the original committer, a duplicated S-o-b may appended by accident. New commit log may looks like: some commit log... Signed-off-by: C O Mitter commit...@example.com Reported-by: R E Porter repor...@example.com Signed-off-by: C O Mitter commit...@example.com After stating the observation like the above, please make it a habit to say which is bad because..., if you think it is a bad behaviour and the patch is about fixing it. Because a chain of S-o-b is used to record the flow of a patch, it is entirely normal if developer A writes the patch (she signs it off), reviewer B picks it up and sends it back with a minor fix-up to the list, and developer A again picks it up from the list and forwards it to the uplevel maintainer, in which case you may see S-o-b by A, then B (it may be S-o-b or something else, e.g. Reviewed-by) and then S-o-b by A again. The above observation is correct (a commit log may look like so), but your untold conclusion (it is a bad thing because there are S-o-b from the same person twice) is not necessarily correct. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] builtin-commit.c: Not add duplicate S-o-b when commit
2012/7/26 Junio C Hamano gits...@pobox.com: After stating the observation like the above, please make it a habit to say which is bad because..., if you think it is a bad behaviour and the patch is about fixing it. Indead before I start, I examine git-commit and git-am, and find the behaviours of the two commands are different. git commit -s checks the last line of the footer, while git -am checks the last S-o-b. E.g. original commit X: commit log... Signed-off-by: A Signed-off-by: B Reported-by: C When user B amend the commit, the amended commit Y looks like: commit log... Signed-off-by: A Signed-off-by: B Reported-by: C Signed-off-by: B While if the original commit X send to user B by patch, and user B run command git am -s, the commit would be: Signed-off-by: A Signed-off-by: B Reported-by: C So I guess duplicate S-o-b is not intentional. I use an alias for commit: git config --global alias.ci commit -s And will encounter duplicate S-o-b issues frequently, especially format-patch/send-email workflow. -- Jiang Xin -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 01/16] Implement a remote helper for svn in C.
On Thursday 26 July 2012 02:46:07 Jonathan Nieder wrote: Hi, Florian Achleitner wrote: --- /dev/null +++ b/contrib/svn-fe/remote-svn.c @@ -0,0 +1,219 @@ + +#include cache.h +#include remote.h +#include strbuf.h +#include url.h +#include exec_cmd.h +#include run-command.h +#include svndump.h +#include argv-array.h + +static int debug; + +static inline void printd(const char *fmt, ...) I remember reviewing this before, and mentioning that this could be replaced with trace_printf() and that would simplify some code and improve the functionality. I think I also remember giving some other suggestions, but I don't have it in front of me so I can't be sure (should have more time this weekend). Did you look over that review? Did you have any questions about it, or was it just full of bad ideas, or something else? It's silly and vain of me, but I'm not motivated by the idea of spending more time looking over this without anything coming of it. (Rejecting suggestions is fine, but sending feedback when doing so is important because otherwise reviewers get demotivated.) Yes, I incorporated your review in the new version, as far as applicable. But I didn't send you an answer on the detailed points. I will send an answer to the previous review .. Hope that helps, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 1/4 v2] Implement a basic remote helper for svn in C.
Hi!
Most of this review went into the new version..
For the remaining points, some comments follow.
On Monday 02 July 2012 06:07:41 Jonathan Nieder wrote:
Hi,
Florian Achleitner wrote:
--- /dev/null
+++ b/contrib/svn-fe/remote-svn.c
@@ -0,0 +1,207 @@
+
+#include stdlib.h
+#include string.h
+#include stdio.h
git-compat-util.h (or some header that includes it) must be the first
header included so the appropriate feature test macros can be defined.
See Documentation/CodingGuidelines for more on that.
check.
+#include cache.h
+#include remote.h
+#include strbuf.h
+#include url.h
+#include exec_cmd.h
+#include run-command.h
+#include svndump.h
+
+static int debug = 0;
Small nit: please drop the redundant = 0 here. Or:
check.
+
+static inline void printd(const char* fmt, ...)
+{
+ if(debug) {
+ va_list vargs;
+ va_start(vargs, fmt);
+ fprintf(stderr, rhsvn debug: );
+ vfprintf(stderr, fmt, vargs);
+ fprintf(stderr, \n);
+ va_end(vargs);
+ }
+}
Why not use trace_printf and avoid the complication?
Hm.. I tried. It wasn't exactly what I wanted. When I use trace_printf, it's
activated together with all other traces. I can use trace_vprintf and specify
a key, but I would always have to print the header rhsvn debug: and the key
by hand. So I could replace vfprintf in this function by trace_vprintf to do
that. But then there's not much simplification. (?)
+
+enum cmd_result cmd_capabilities(struct strbuf* line);
+enum cmd_result cmd_import(struct strbuf* line);
+enum cmd_result cmd_list(struct strbuf* line);
What's a cmd_result? '*' sticks to variable name.
+
+enum cmd_result { SUCCESS, NOT_HANDLED, ERROR };
Oh, that's what a cmd_result is. :) Why not define the type before
using it to avoid keeping the reader in suspense?
What does each result represent? If this is a convention like
1: handled
0: not handled
-1: error, callee takes care of printing the error message
then please document it in a comment near the caller so the reader can
understand what is happening without too much confusion. Given such a
comment, does the enum add clarity?
Hm.. the enum now has SUCCESS, NOT_HANDLED, TERMINATE.
It gives the numbers a name, thats it.
+typedef enum cmd_result (*command)(struct strbuf*);
When I first read this, I wonder what is being commanded. Are these
commands passed on the remote helper's standard input, commands passed
on its output, or commands run at some point in the process? What is
the effect and return value of associated function? Does the function
always return some success/failure value, or does it sometimes exit?
Maybe a more specific type name would be clearer?
I renamed it to input_command_handler. Unfortunately the remote-helper spec
calls what is sent to the helper a 'command'.
[...]
+
+const command command_list[] = {
+ cmd_capabilities, cmd_import, cmd_list, NULL
+};
First association is to functions like cmd_fetch() which implement git
subcommands. So I thought these were going to implement subcommands
like git remote-svn capabilities, git remote-svn import and would
use the same cmd_foo(argc, argv, prefix) calling convention that git
subcommands do. Maybe a different naming convention could avoid
confusion.
Ok.. same as above, they are kind of commands. Of course I can change the
names. For me it's not too confusing, because I don't know the git subcommands
convention very well. You can choose a name.
[...]
+enum cmd_result cmd_capabilities(struct strbuf* line)
+{
+ if(strcmp(line-buf, capabilities))
+ return NOT_HANDLED;
Style: missing SP after keyword.
+
+ printf(import\n);
+ printf(\n);
+ fflush(stdout);
+ return SUCCESS;
+}
Why the multiple printf? Is the flush needed?
Excess printf gone.
Flush is needed. Otherwise it doesn't flush and the other end waits forever.
Don't know exactly why. Some pipe-buffer ..
+
+ /* opening a fifo for usually reading blocks until a writer has opened
it too. +* Therefore, we open with RDWR.
+*/
+ report_fd = open(back_pipe_env, O_RDWR);
+ if(report_fd 0) {
+ die(Unable to open fast-import back-pipe! %s,
strerror(errno));
+ }
Is this necessary? Why shouldn't we fork the writer first and wait
for it here?
Yes, necessary. Blocking on this open call prevents fast-import as well as the
remote helper from reading and writing on their normal command streams.
This leads to deadlocks.
E.g. If there's have nothing to import, the helper sends only 'done' to fast-
import and quits. That might happen before fast-import opened this pipe.
Then it waits forever because the reader has already closed it.
+
+ code = start_command(svndump_proc);
+ if(code)
+ die(Unable to start %s, code %d,
Re: [RFC 01/16] Implement a remote helper for svn in C.
On Thursday 26 July 2012 03:14:43 Jonathan Nieder wrote: Florian Achleitner wrote: Yes, I incorporated your review in the new version, as far as applicable. But I didn't send you an answer on the detailed points. I will send an answer to the previous review .. Thanks. Now that I check, I see that you did make lots of important changes and probably lost the one I noticed just now in the noise. Another way to keep reviewers happy is to describe what changed since the last revision under the triple-dash for each patch when sending out a new set of patches. That way, they can see that there was progress and there is less frustration when one specific change didn't make it. See http://thread.gmane.org/gmane.comp.version-control.git/176203 for example. Yeah, that makes sense. In this reroll, I really changed a lot, order and scope of patches is very different. Many haven't hit the list yet. I wanted to write a new more useful history. The first patch, this one, consists of many enhancement commits I made after each other, finally integrated into one. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 1/4 v2] Implement a basic remote helper for svn in C.
Florian Achleitner wrote:
Most of this review went into the new version..
For the remaining points, some comments follow.
Thanks for this.
On Monday 02 July 2012 06:07:41 Jonathan Nieder wrote:
[...]
+
+static inline void printd(const char* fmt, ...)
[...]
Why not use trace_printf and avoid the complication?
Hm.. I tried. It wasn't exactly what I wanted. When I use trace_printf, it's
activated together with all other traces. I can use trace_vprintf and specify
a key, but I would always have to print the header rhsvn debug: and the
key
by hand. So I could replace vfprintf in this function by trace_vprintf to do
that. But then there's not much simplification. (?)
Hmm. There's no trace_printf_with_key() but that's presumably because
no one has needed it. If it existed, you could use
#define printd(msg) trace_printf_with_key(GIT_TRACE_REMOTE_SVN, %s,
msg)
But now that I check, I don't see how the current printd() calls would
be useful to other people. Why announce these moments and not others?
They're just temporary debugging cruft, right?
For that, plain trace_printf() works great.
[...]
+
+enum cmd_result { SUCCESS, NOT_HANDLED, ERROR };
[...]
Hm.. the enum now has SUCCESS, NOT_HANDLED, TERMINATE.
Much nicer.
I think this tristate return value could be avoided entirely because...
[continued at (*) below]
[...]
+
+ printf(import\n);
+ printf(\n);
+ fflush(stdout);
+ return SUCCESS;
+}
Why the multiple printf? Is the flush needed?
Excess printf gone.
Flush is needed. Otherwise it doesn't flush and the other end waits forever.
Ah, fast-import is ready, remote helper is ready, no one initiates
pumping of data between them. Maybe the purpose of the flush would
be more obvious if it were moved to the caller.
[...]
+ /* opening a fifo for usually reading blocks until a writer has opened
it too. +* Therefore, we open with RDWR.
+*/
+ report_fd = open(back_pipe_env, O_RDWR);
+ if(report_fd 0) {
+ die(Unable to open fast-import back-pipe! %s,
strerror(errno));
+ }
Is this necessary? Why shouldn't we fork the writer first and wait
for it here?
Yes, necessary.
Oh, dear. I hope not. E.g., Cygwin doesn't support opening fifos
RDWR (out of scope for the gsoc project, but still).
[...]
E.g. If there's have nothing to import, the helper sends only 'done' to fast-
import and quits.
Won't the writer open the pipe and wait for us to open our end before
doing that?
[...]
+
+ code = start_command(svndump_proc);
+ if(code)
+ die(Unable to start %s, code %d, svndump_proc.argv[0], code);
start_command() is supposed to have printed a message already when it
fails, unless errno == ENOENT and silent_exec_failure was set.
Yes, but it doesn't die, right?
You can exit without writing a message with exit(), e.g. like so:
if (code)
exit(code);
or like so:
if (code)
exit(128);
[...]
+
+ close(svndump_proc.out);
Important? Wouldn't finish_command do this?
As far as I understood it, it doesn't close extra created pipes. Probably I
just didn't find it in the code ..
So this is to work around a bug in the run-command interface?
[...]
+ close(report_fd);
What is the purpose of this step?
Close the back-report pipe end of the remote-helper.
That's just repeating the question. :) Perhaps it's supposed to
trigger some action on the other end of the pipe? It would just be
useful to add a comment documenting why one shouldn't remove this
close() call, or else will probably end up removing it and needlessly
suffering.
[...]
+
+ code = finish_command(svndump_proc);
+ if(code)
+ warning(Something went wrong with termination of %s, code %d,
svndump_proc.argv[0], code);
finish_command() is supposed to print a message when it fails.
I changed the message text. It should tell us if svnrdump exited with non-
zero.
I'd suggest looking at other finish_command() callers for examples.
[...]
+enum cmd_result do_command(struct strbuf* line)
+{
+ const command* p = command_list;
+ enum cmd_result ret;
+ printd(command line '%s', line-buf);
+ while(*p) {
+ ret = (*p)(line);
+ if(ret != NOT_HANDLED)
+ return ret;
+ p++;
+ }
If possible, matching commands by name (like git.c does) would make
the behavior easier to predict.
There is some usecase for this. The intention was, that command handlers
should be able to process more than one 'name'. E.g. an import batch is
terminated by a newline. This newline is handled by the import handler if it
is a batch. (This feature wasn't implemented in the version reviewed here.)
So I decided to let the handler functions tell if they handle this line.
[continued from (*) above]
... it isn't needed at the moment.
See http://c2.com/xp/YouArentGonnaNeedIt.html
[...]
+ free((void*)url);
+
Re: [RFC 1/4 v2] Implement a basic remote helper for svn in C.
On Jul 2, 2012, at 4:07 AM, Jonathan Nieder jrnie...@gmail.com wrote: [...] diff: Use fifo instead of pipe: Retrieve the name of the pipe from env and open it for svndump. I'd prefer to avoid this if possible, since it means having to decide where the pipe goes on the filesystem. Can you summarize the discussion in the commit message so future readers understand why we're doing it? Crazy thought here but would a socket not be a bad choice here? Imagine being able to ssh tunnel into the SVN server and run the helper with filesystem access to the SVN repo. Akin to the pushy project use case. http://packages.python.org/pushy/ SSH into the machine, copy the required components to the machine, and use the RPC. Nothing needed but SSH and python. In this case SSH, SVN, and the helper would be needed. This also would work just fine with the local host too. Steve Note: Resent, Sorry it was signed, and rejected before.-- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] i18n: leave \n out of translated diffstat
GETTEXT_POISON scrapes everything in translated strings, including \n.
t4205.12 however needs this \n in matching the end result. Keep this
\n out of translation to make t4205.12 happy.
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
I haven't followed recent i18n patches closely. Jiang may have
already fixed this in one of his patches. Anyway just in case
everybody does miss this..
Should I resend parseopt i18n marking series now or wait until rc
period is over?
diff.c |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/diff.c b/diff.c
index 62cbe14..95706a5 100644
--- a/diff.c
+++ b/diff.c
@@ -1397,7 +1397,7 @@ int print_stat_summary(FILE *fp, int files, int
insertions, int deletions)
if (!files) {
assert(insertions == 0 deletions == 0);
- return fputs(_( 0 files changed\n), fp);
+ return fprintf(fp, %s\n, _( 0 files changed));
}
strbuf_addf(sb,
--
1.7.8
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Test t/t7502-commit.sh failed
On Thu, Jul 26, 2012 at 02:27:52PM +0800, Jiang Xin wrote: Test t/t7502-commit.sh failed. I guess it's commit v1.7.9.7-1-gf20f387 which breaks it. $ git log -1 --oneline --stat v1.7.9.7-1-gf20f387 f20f commit: check committer identity more strictly builtin/commit.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) Result of t/t7502-commit.sh: not ok - 21 committer is automatic # # # echo negative # ( # sane_unset GIT_COMMITTER_EMAIL # sane_unset GIT_COMMITTER_NAME # # must fail because there is no change # test_must_fail git commit -e -m sample # ) # head -n 8 .git/COMMIT_EDITMSG | \ # sed s/^# Committer: .*/# Committer:/ actual # test_i18ncmp expect actual # Hmm. It doesn't fail here, but that is probably because I have my name set properly in /etc/passwd. I think the test is bogus, though. From the results you report: Contents of file expect: sample # Please enter the commit message for your changes. Lines starting # with '#' will be ignored, and an empty message aborts the commit. # # Author:A U Thor aut...@example.com # Committer: # Contents of file actual: sample The test is expecting us to write out the commit template for the user to edit. But the whole point of f20f387 is to fail early, before we ask the user to edit the template. So the test is trying to check that we wrote _something_ into the Committer field, even though that something would not necessarily be used to make the commit object (because the code path for the commit object is going to be much stricter). I am not sure that the test is really all that useful. The point seems to be that we fall back to some kind of system-based ident, but that is not portable. On some systems we can, and on some we can't, depending on things like how /etc/passwd and the hostname are configured. I'll see if I can make it more robust, but I think the right solution may simply be to rip out the test. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: False positive from orphaned_commit_warning() ?
On 12-07-25 05:52 PM, Junio C Hamano wrote:
Paul Gortmaker paul.gortma...@windriver.com writes:
Has anyone else noticed false positives coming from the
orphan check?
Thanks. This should fix it.
Indeed it does. Thanks for the fix (and git in general).
Paul.
--
builtin/checkout.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 6acca75..d812219 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -606,7 +606,7 @@ static int add_pending_uninteresting_ref(const char
*refname,
const unsigned char *sha1,
int flags, void *cb_data)
{
- add_pending_sha1(cb_data, refname, sha1, flags | UNINTERESTING);
+ add_pending_sha1(cb_data, refname, sha1, UNINTERESTING);
return 0;
}
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/5] test-lib: filesystem prerequisites
This mini series provides and makes use of test prerequisites for case insensitivity, symlinks and unicode conversion. SYMLINKS existed before but was not used in t0050. CASE_INSENSITIVE_FS was defined in t0003 rather than test-lib (and redone in t0050). UTF8_NFD_TO_NFC did not exist but was redone in two ways in two tests. After this series, all 3 are defined in test-lib and used in the various tests. Michael J Gruber (5): test-lib: provide case insensitivity as a prerequisite t0050: use the CASE_INSENSITIVE_FS test prereq t0050: use the SYMLINKS test prereq test-lib: provide UTF8 behaviour as a prerequisite t3910: use the SYMLINKS test prereq t/README | 9 ++ t/t0003-attributes.sh| 10 -- t/t0050-filesystem.sh| 64 -- t/t3910-mac-os-precompose.sh | 281 +-- t/test-lib.sh| 24 5 files changed, 189 insertions(+), 199 deletions(-) Really (-w), it is this besides the tab removals: t/README | 9 + t/t0003-attributes.sh| 10 -- t/t0050-filesystem.sh| 62 -- t/t3910-mac-os-precompose.sh | 25 +++-- t/test-lib.sh| 24 5 files changed, 60 insertions(+), 70 deletions(-) -- 1.7.12.rc0.198.gd66b616 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 4/5] test-lib: provide UTF8 behaviour as a prerequisite
UTF8 behaviour of the filesystem (conversion from nfd to nfc) plays a role in several tests and is tested in several tests. Therefore, move the test from t0050 into the test lib and use the prerequisite in t0050. Signed-off-by: Michael J Gruber g...@drmicha.warpmail.net --- t/README | 5 + t/t0050-filesystem.sh | 24 +++- t/test-lib.sh | 14 ++ 3 files changed, 26 insertions(+), 17 deletions(-) diff --git a/t/README b/t/README index 5725607..e4128e5 100644 --- a/t/README +++ b/t/README @@ -629,6 +629,11 @@ use these, and test_set_prereq for how to define your own. Test is run on a case insensitive file system. + - UTF8_NFD_TO_NFC + + Test is run on a filesystem which converts decomposed utf-8 (nfd) + to precomposed utf-8 (nfc). + Tips for Writing Tests -- diff --git a/t/t0050-filesystem.sh b/t/t0050-filesystem.sh index b46ae72..78816d9 100755 --- a/t/t0050-filesystem.sh +++ b/t/t0050-filesystem.sh @@ -7,22 +7,6 @@ test_description='Various filesystem issues' auml=$(printf '\303\244') aumlcdiar=$(printf '\141\314\210') -unibad= -test_expect_success 'see what we expect' ' - - test_unicode=test_expect_success - mkdir junk - junk/$auml - case $(cd junk echo *) in - $aumlcdiar) - test_unicode=test_expect_failure - unibad=t - ;; - *) ;; - esac - rm -fr junk -' - if test_have_prereq CASE_INSENSITIVE_FS then say will test on a case insensitive filesystem @@ -31,8 +15,14 @@ else test_case=test_expect_success fi -test $unibad +if test_have_prereq UTF8_NFD_TO_NFC +then say will test on a unicode corrupting filesystem + test_unicode=test_expect_failure +else + test_unicode=test_expect_success +fi + test_have_prereq SYMLINKS || say will test on a filesystem lacking symbolic links diff --git a/t/test-lib.sh b/t/test-lib.sh index 57fc1f2..057ac1d 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -676,3 +676,17 @@ then test_set_prereq CASE_INSENSITIVE_FS fi rm -rf junk + +# check whether FS converts nfd unicode to nfc +auml=$(printf '\303\244') +aumlcdiar=$(printf '\141\314\210') + +mkdir junk +junk/$auml +case $(cd junk echo *) in +$aumlcdiar) + test_set_prereq UTF8_NFD_TO_NFC + ;; +*) ;; +esac +rm -fr junk -- 1.7.12.rc0.198.gd66b616 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/5] t0050: use the CASE_INSENSITIVE_FS test prereq
Signed-off-by: Michael J Gruber g...@drmicha.warpmail.net --- t/t0050-filesystem.sh | 21 - 1 file changed, 8 insertions(+), 13 deletions(-) diff --git a/t/t0050-filesystem.sh b/t/t0050-filesystem.sh index 1542cf6..df9498b 100755 --- a/t/t0050-filesystem.sh +++ b/t/t0050-filesystem.sh @@ -7,23 +7,12 @@ test_description='Various filesystem issues' auml=$(printf '\303\244') aumlcdiar=$(printf '\141\314\210') -case_insensitive= unibad= no_symlinks= test_expect_success 'see what we expect' ' - test_case=test_expect_success test_unicode=test_expect_success mkdir junk - echo good junk/CamelCase - echo bad junk/camelcase - if test $(cat junk/CamelCase) != good - then - test_case=test_expect_failure - case_insensitive=t - fi - rm -fr junk - mkdir junk junk/$auml case $(cd junk echo *) in $aumlcdiar) @@ -41,14 +30,20 @@ test_expect_success 'see what we expect' ' } ' -test $case_insensitive +if test_have_prereq CASE_INSENSITIVE_FS +then say will test on a case insensitive filesystem + test_case=test_expect_failure +else + test_case=test_expect_success +fi + test $unibad say will test on a unicode corrupting filesystem test $no_symlinks say will test on a filesystem lacking symbolic links -if test $case_insensitive +if test_have_prereq CASE_INSENSITIVE_FS then test_expect_success detection of case insensitive filesystem during repo init ' -- 1.7.12.rc0.198.gd66b616 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 1/4 v2] Implement a basic remote helper for svn in C.
(cc-ing Ram since he's also knowledgeable about remote-helper protocol) Florian Achleitner wrote: On Thursday 26 July 2012 06:40:39 Jonathan Nieder wrote: Though I still think the way forward is to keep using plain pipes internally for now and to make the bidirectional communication optional, since it wouldn't close any doors to whatever is most convenient on each platform. Hopefully I'll hear more from Florian about this in time. Would you like to see a new pipe patch? Since the svn remote helper relies on this, it seems worth working on, yeah. As for how to spend your time (and whether to beg someone else to work on it instead :)): I'm not sure what's on your plate or where you are with respect to the original plan for the summer at the moment, so it would be hard for me to give useful advice about how to balance things. What did you think of the suggestion of adding a new bidi-import capability and command to the remote helper protocol? I think this would be clean and avoid causing a regression on Windows, but it's easily possible I am missing something fundamental. Thanks, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 1/4 v2] Implement a basic remote helper for svn in C.
On Thursday 26 July 2012 04:08:42 Jonathan Nieder wrote:
Florian Achleitner wrote:
On Monday 02 July 2012 06:07:41 Jonathan Nieder wrote:
[...]
+
+static inline void printd(const char* fmt, ...)
[...]
Why not use trace_printf and avoid the complication?
Hm.. I tried. It wasn't exactly what I wanted. When I use trace_printf,
it's activated together with all other traces. I can use trace_vprintf
and specify a key, but I would always have to print the header rhsvn
debug: and the key by hand. So I could replace vfprintf in this
function by trace_vprintf to do that. But then there's not much
simplification. (?)
Hmm. There's no trace_printf_with_key() but that's presumably because
no one has needed it. If it existed, you could use
#define printd(msg) trace_printf_with_key(GIT_TRACE_REMOTE_SVN, %s,
msg)
But now that I check, I don't see how the current printd() calls would
be useful to other people. Why announce these moments and not others?
They're just temporary debugging cruft, right?
For that, plain trace_printf() works great.
Yes, it's for debugging only, I could just delete it all. It's inspired by
transport-helper.c. The env var GIT_TRANSPORT_HELPER_DEBUG enables it. While
transport-helper has a lot of if (debug) fprintf(..), I encapsulated it in
printd.
So I should kick printd out?
+
+ printf(import\n);
+ printf(\n);
+ fflush(stdout);
+ return SUCCESS;
+}
Why the multiple printf? Is the flush needed?
Excess printf gone.
Flush is needed. Otherwise it doesn't flush and the other end waits
forever.
Ah, fast-import is ready, remote helper is ready, no one initiates
pumping of data between them. Maybe the purpose of the flush would
be more obvious if it were moved to the caller.
Acutally this goes to the git parent process (not fast-import), waiting for a
reply to the command. I think I have to call flush on this side of the pipe.
Can you flush it from the reader? This wouldn't have the desired effect, it
drops buffered data.
[...]
+ /* opening a fifo for usually reading blocks until a writer has opened
it too. + * Therefore, we open with RDWR.
+ */
+ report_fd = open(back_pipe_env, O_RDWR);
+ if(report_fd 0) {
+ die(Unable to open fast-import back-pipe! %s,
strerror(errno));
+ }
Is this necessary? Why shouldn't we fork the writer first and wait
for it here?
Yes, necessary.
Oh, dear. I hope not. E.g., Cygwin doesn't support opening fifos
RDWR (out of scope for the gsoc project, but still).
I believe it can be solved using RDONLY and WRONLY too. Probably we solve it
by not using the fifo at all.
Currently the blocking comes from the fact, that fast-import doesn't parse
it's command line at startup. It rather reads an input line first and decides
whether to parse the argv after reading the first input line or at the end of
the input. (don't know why)
remote-svn opens the pipe before sending the first command to fast-import and
blocks on the open, while fast-import waits for input -- deadlock.
with remote-svn: RDWR, fast-import: WRONLY, this works.
Other scenario: Nothing to import, remote-svn only sends 'done' and closes the
pipe again. After fast-import reads the first line it parses it's command line
and tries to open the fifo which is already closed on the other side --
blocks.
This is solved by using RDWR on both sides.
If we change the points where the pipes are openend and closed, this could be
circumvented.
[...]
E.g. If there's have nothing to import, the helper sends only 'done' to
fast- import and quits.
Won't the writer open the pipe and wait for us to open our end before
doing that?
[...]
+
+ code = start_command(svndump_proc);
+ if(code)
+ die(Unable to start %s, code %d, svndump_proc.argv[0], code);
start_command() is supposed to have printed a message already when it
fails, unless errno == ENOENT and silent_exec_failure was set.
Yes, but it doesn't die, right?
You can exit without writing a message with exit(), e.g. like so:
if (code)
exit(code);
or like so:
if (code)
exit(128);
ok, why not..
[...]
+
+ close(svndump_proc.out);
Important? Wouldn't finish_command do this?
As far as I understood it, it doesn't close extra created pipes. Probably
I
just didn't find it in the code ..
So this is to work around a bug in the run-command interface?
Good question.
[...]
+ close(report_fd);
What is the purpose of this step?
Close the back-report pipe end of the remote-helper.
That's just repeating the question. :) Perhaps it's supposed to
trigger some action on the other end of the pipe? It would just be
useful to add a comment documenting why one shouldn't remove this
close() call, or else will probably end up removing it and needlessly
suffering.
It's just closing files I've openend
Re: [PATCH 1/3] retain reflogs for deleted refs
On 26 Jul 2012, at 14:47, Nguyen Thai Ngoc Duy wrote: So we haven't found any way to present both branches foo and foo/bar on file system at the same time. How about when we a new branch introduces such a conflict, we push the new branch directly to packed-refs? If we need either of them on a separate file, for fast update for example, then we unpack just one and repack all refs that conflict with it. Attempting to update two conflict branches in parallel may impact performance, but I don't think that happens often. -- Duy How about simply deprecating / in branch name? -Alexey. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Test t/t7502-commit.sh failed
Jeff King p...@peff.net writes: On Thu, Jul 26, 2012 at 02:27:52PM +0800, Jiang Xin wrote: ... not ok - 21 committer is automatic # # # echo negative # ( # sane_unset GIT_COMMITTER_EMAIL # sane_unset GIT_COMMITTER_NAME # # must fail because there is no change # test_must_fail git commit -e -m sample # ) # head -n 8 .git/COMMIT_EDITMSG | \ # sed s/^# Committer: .*/# Committer:/ actual # test_i18ncmp expect actual # Hmm. It doesn't fail here, but that is probably because I have my name set properly in /etc/passwd. I think the test is bogus, though. From the results you report: Contents of file expect: sample # Please enter the commit message for your changes. Lines starting # with '#' will be ignored, and an empty message aborts the commit. # # Author:A U Thor aut...@example.com # Committer: # Contents of file actual: sample The test is expecting us to write out the commit template for the user to edit. But the whole point of f20f387 is to fail early, before we ask the user to edit the template. So the test is trying to check that we wrote _something_ into the Committer field, even though that something would not necessarily be used to make the commit object (because the code path for the commit object is going to be much stricter). I am not sure that the test is really all that useful. The point seems to be that we fall back to some kind of system-based ident, but that is not portable. I think the point is to make sure that the # Committer: line is given to the reader to remind that we took the codepath that comes up with a committer ident by using untrustworthy heuristics. You are correct that the usefulness of the value of system-based ident varies between systems (that is why it is stripped out with sed), though. You earlier gave a reason why f20f387 (commit: check committer identity more strictly, 2012-07-23) does not have a test for it; I think the same reason applies why this test is unworkable. A related tangent; all the test vectors in this script seems to be too wide, and we probably would want to narrow them for what each test wants to see. For example, the test in question only wants to see # Committer: some system based ident and it does not matter if the template was rewritten in future versions of Git so that it does not begin with # Please enter Similarly, the one previous only wants to see # Author: different from committer. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Test t/t7502-commit.sh failed
On Thu, Jul 26, 2012 at 09:34:27AM -0700, Junio C Hamano wrote: not ok - 21 committer is automatic [...] I am not sure that the test is really all that useful. The point seems to be that we fall back to some kind of system-based ident, but that is not portable. I think the point is to make sure that the # Committer: line is given to the reader to remind that we took the codepath that comes up with a committer ident by using untrustworthy heuristics. You are correct that the usefulness of the value of system-based ident varies between systems (that is why it is stripped out with sed), though. Ah, right. I was led astray by the crappy test title. When viewed with the test immediately prior (which checks that Author: is shown in the template), it makes more sense. You earlier gave a reason why f20f387 (commit: check committer identity more strictly, 2012-07-23) does not have a test for it; I think the same reason applies why this test is unworkable. Right. You can check this only when git var GIT_COMMITTER_IDENT works, and you can check the f20f387 behavior only when it does _not_ work. So we could do something like: (sane_unset GIT_COMMITTER_NAME sane_unset GIT_COMMITTER_EMAIL git var GIT_COMMITTER_IDENT /dev/null) test_set_prereq AUTOIDENT || test_set_prereq NOAUTOIDENT test_expect_success AUTOIDENT \ 'mention auto ident in commit template' '...' test_expect_success NOAUTOIDENT \ 'git rejects bogus ident before starting editor' '...' But it is somewhat unsatisfying to only get random test coverage depending on how your system happens to be configured. I guess we somewhat have that already with the case-insensitivity tests. Do we want to go that route, or just drop this test completely? A related tangent; all the test vectors in this script seems to be too wide, and we probably would want to narrow them for what each test wants to see. For example, the test in question only wants to see # Committer: some system based ident and it does not matter if the template was rewritten in future versions of Git so that it does not begin with # Please enter Similarly, the one previous only wants to see # Author: different from committer. Agreed. They should probably just i18ngrep for ^# Committer: or similar. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] retain reflogs for deleted refs
On 26 Jul 2012, at 18:59, Jeff King wrote: Not to mention git itself, as it splits up the refs/remotes hierarchy into subdirectories. I think deprecating / is out of the question. -Peff Ok, i guess you know better than me, my vision of Git is probably still too simplistic. -Alexey.-- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/4] allow recovery from command name typos
On Fri, Jul 27, 2012 at 01:08:34AM +0800, Tay Ray Chuan wrote: Perhaps we should audit isatty() calls and replace them with a helper function that does this kind of thing consistently in a more robust way (my recent favorite is Linus's somewhat anal logic used in builtin/merge.c::default_edit_option()). Any specific callers to isatty() you have in mind? A quick grep shows that a significant portion of the offenders are isatty(2) calls to determine whether to display progress, I think those are ok. Yeah, those are probably fine. Grep reveals that besides isatty(2) and the merge default_edit_option check, we have: - isatty(1) for checking auto-output munging, including auto-colors, auto-columns, and the pager. These are all fine, as they are not about interactivity, but specifically about whether stdout is a tty. - isatty(0) in commit.c to print a message when reading -F - from stdin. OK. - isatty(0) in pack-redundant to avoid reading stdin when it is a terminal (a questionable choice, perhaps, but not really something that would want a full interactivity check). - isatty(0) check in cmd_revert to set opts.edit automatically. This one should match merge's behavior. - isatty(0) in shortlog; this is a compatibility hack as shortlog traditionally accepted log output on stdin, but can now be used stand-alone. OK. So I think the only one that could be improved is the one in cmd_revert. The credential helper has some prompting functionality that is close to what I intend to do here, but I think it can make some assumptions about stdin/stdout that we can't, as you have pointed out. So that leaves merge-edit and this patch as the beneficiaries of a builtin/merge.c::default_edit_option() refactor. That's just off the top of my head. The credential code uses git_terminal_prompt, which actually opens /dev/tty directly. So it is probably sane to use for your new prompt, but it does not (and should not) rely on isatty. Perhaps the helper function could be named git_can_prompt() and placed in prompt.c? Please don't. The isatty() checks have nothing to do with whether git_prompt can run. The only thing such a git_can_prompt function should do is see if we can open /dev/tty. The isatty check in merge.c is more about are we interactive, so that it is sane to run $EDITOR. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 1/4 v2] Implement a basic remote helper for svn in C.
Jonathan Nieder jrnie...@gmail.com writes:
[...]
+
+enum cmd_result { SUCCESS, NOT_HANDLED, ERROR };
[...]
Hm.. the enum now has SUCCESS, NOT_HANDLED, TERMINATE.
Much nicer.
I think this tristate return value could be avoided entirely because...
... it isn't needed at the moment.
I am not sure what you mean by that.
The command dispatcher loop in [Patch v2 1/16] seems to call every
possible input handler with the first line of the input and expect
them to answer This is not for me, so NOT_HANDLED is needed.
An alternative dispatcher could be written in such a way that the
dispatcher inspects the first line and decide what to call, and in
such a scheme, you do not need NOT_HANDLED. My intuition tells me
that such an arrangement is in general a better organization.
Looking at what cmd_import() does, however, I think the approach the
patch takes might make sense for this application. Unlike other
handlers like capabilities that do not want to handle anything
other than capabilities, it wants to handle two:
- import that starts an import batch;
- (an empty line), but only when an import batch is in effect.
A centralized dispatcher that does not use NOT_HANDLED could be
written for such an input stream, but then the state information
(i.e. are we in an import batch?) needs to be global, which may or
may not be desirable (I haven't thought things through on this).
In any case, if you are going to use dispatching based on
NOT_HANDLED, the result may have to be (at least) quadri-state. In
addition to I am done successfully, please go back and dispatch
another command (SUCCESS), This is not for me (NOT_HANDLED), and
I am done successfully, and there is no need to dispatch and
process another command further (TERMINATE), you may want to be
able to say This was for me, but I found an error (ERROR).
Of course, if the dispatch loop has to be rewritten so that a
central dispatcher decides what to call, individual input handlers
do not need to say NOT_HANDLED nor TERMINATE, as the central
dispatcher should keep track of the overall state of the system, and
the usual 0 on success, negative on error may be sufficient.
One thing I wondered was how an input capability (or list)
should be handled after import was issued (hence batch_active
becomes true). The dispatcher loop in the patch based on
NOT_HANDLED convention will happily call cmd_capabilities(), which
does not have any notion of the batch_active state (because it is a
function scope static inside cmd_import()), and will say Ah, that
is mine, and let me do my thing. If we want to diagnose such an
input stream as an error, the dispatch loop needs to become aware of
the overall state of the system _anyway_, so that may be an argument
against the NOT_HANDLED based dispatch system the patch series uses.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] difftool: Disable --symlinks on cygwin
On Thu, Jul 26, 2012 at 4:31 AM, Erik Faye-Lund kusmab...@gmail.com wrote: On Wed, Jul 25, 2012 at 5:14 AM, David Aguilar dav...@gmail.com wrote: Symlinks are not ubiquitous on Windows so make --no-symlinks the default. Signed-off-by: David Aguilar dav...@gmail.com --- I don't have cygwin so I can't verify this one myself. Is 'cygwin' really the value of $^O there? git-difftool.perl | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/git-difftool.perl b/git-difftool.perl index 591ee75..10d3d97 100755 --- a/git-difftool.perl +++ b/git-difftool.perl @@ -291,7 +291,8 @@ sub main gui = undef, help = undef, prompt = undef, - symlinks = $^O ne 'MSWin32' $^O ne 'msys', + symlinks = $^O ne 'cygwin' + $^O ne 'MSWin32' $^O ne 'msys', I thought Cygwin supported (their own version of) symlinks? What's the rationale for not using it by default there? I am not a Cygwin user so I cannot verify whether it is a good or bad idea. I have a few questions regarding symlinks on Cygwin: Do the symlinks work consistently with the Perl symlink() function? Can we always rely on this capability being available? Do all win32 filesystems support it? Do all builds of cygwin perl support it? If any of these answers are no or maybe, then an improvement beyond this patch would be to perhaps support a `difftool.symlinks` configuration variable so that the user can tell us what to use as the default. -- David -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i18n: leave \n out of translated diffstat
Nguyễn Thái Ngọc Duy pclo...@gmail.com writes:
diff --git a/diff.c b/diff.c
index 62cbe14..95706a5 100644
--- a/diff.c
+++ b/diff.c
@@ -1397,7 +1397,7 @@ int print_stat_summary(FILE *fp, int files, int
insertions, int deletions)
if (!files) {
assert(insertions == 0 deletions == 0);
- return fputs(_( 0 files changed\n), fp);
+ return fprintf(fp, %s\n, _( 0 files changed));
}
Good.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] retain reflogs for deleted refs
On Thu, Jul 26, 2012 at 10:46:01AM -0700, Junio C Hamano wrote: Bleh. It seems that we did too good a job in coming up with a list of disallowed ref characters; they really are things you don't want in your filenames at all. :) Why do no need to even worry about ~ vs : vs whatever in the first place? With a flag-day per repository core.repositoryformatversion = 1, you do not have to worry about mixture of old-style refs and new ones, so refs/heads/next-d/log could be a topic branch 'next/log' that is based on an integration branch 'next' branch that physically resides at refs/heads/next-f or an entry refs/heads/next in packed refs. Only the API functions in refs.c should care, no? I think the point was that Michael wanted to select a standard that could be used for graveyard reflogs _now_, but which would eventually match the format we use for active refs. And that requires a character that is not valid in a refname. Given that the change of format for actives refs would require a flag day, keeping the graveyard scheme mixable with the current ref rules may not be worth caring about, though. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/4] allow recovery from command name typos
Tay Ray Chuan rcta...@gmail.com writes: On Thu, Jul 26, 2012 at 1:57 AM, Junio C Hamano gits...@pobox.com wrote: Tay Ray Chuan rcta...@gmail.com writes: If suggestions are available (based on Levenshtein distance) and if the terminal isatty(), present a prompt to the user to select one of the computed suggestions. The way to determine If the terminal is a tty used in this patch looks overly dangerous, given that we do not know what kind of git command we may be invoking at this point. Indeed, it should also have considered stdin's tty-ness. Not necessarily. As long as you call git_prompt(), which opens /dev/tty on its own and does not break even if its standard input is coming from elsewhere, you should be OK. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/4] allow recovery from command name typos
Jeff King p...@peff.net writes: - isatty(0) check in cmd_revert to set opts.edit automatically. This one should match merge's behavior. ... So I think the only one that could be improved is the one in cmd_revert. Yeah, that matches the result of my grep. Thanks for sanity checking. The credential code uses git_terminal_prompt, which actually opens /dev/tty directly. So it is probably sane to use for your new prompt, but it does not (and should not) rely on isatty. I think using git_terminal_prompt() after doing a looser does the user sit at a terminal and is capable of answering interactive prompt check with isatty(2) is OK, as long as we know that all implementations of git_terminal_prompt() never read from whatever happens to be connected to the standard input. The function falls back to getpass() on platforms without DEV_TTY, and if getpass() on some platforms reads from the standard input, that would be a disaster. I wasn't sure about that part. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/5] test-lib: filesystem prerequisites
Michael J Gruber g...@drmicha.warpmail.net writes: This mini series provides and makes use of test prerequisites for case insensitivity, symlinks and unicode conversion. SYMLINKS existed before but was not used in t0050. CASE_INSENSITIVE_FS was defined in t0003 rather than test-lib (and redone in t0050). UTF8_NFD_TO_NFC did not exist but was redone in two ways in two tests. After this series, all 3 are defined in test-lib and used in the various tests. Consolidating the logic to set necessary prerequisites used in various scripts is very good, but I am not sure adding them to test-lib and run them unconditionally is a good idea. SYMLINKS is used by 47 among 595 tests, which is common enough, but the new ones are not common enough. I do not think we want to create a temporary junk dir, two temporary camelcase files, read and compare them, when nobody in the script cares. We do not have to split them into separate include files, though, in order to avoid such waste. Instead, you can make the logic to set prerequisite conditional inside test-lib.sh and update the users. Something like: (in t/test-lib.sh) case ,$TEST_WANT_PREREQ, in *,CASE_INSENSITIVE_FS,*) mkdir junk echo good junk/CamelCase echo bad junk/camelcase test $(cat junk/CamelCase) == good || test_set_prereq CASE_INSENSITIVE_FS rm -fr junk esac (at the beginning of t/t0003-attributes.sh) #!/bin/sh test_description=gitattributes TEST_WANT_PREREQ=CASE_INSENSITIVE_FS,SYMLINKS . ./test-lib.sh Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/4] allow recovery from command name typos
On Thu, Jul 26, 2012 at 10:59:51AM -0700, Junio C Hamano wrote: The credential code uses git_terminal_prompt, which actually opens /dev/tty directly. So it is probably sane to use for your new prompt, but it does not (and should not) rely on isatty. I think using git_terminal_prompt() after doing a looser does the user sit at a terminal and is capable of answering interactive prompt check with isatty(2) is OK, as long as we know that all implementations of git_terminal_prompt() never read from whatever happens to be connected to the standard input. I don't think isatty(2) is correct, though. It would yield false negatives when the user has redirected stderr but /dev/tty is still available. I don't know if it possible for getpass to fallback to stdin when stderr is a tty (it would mean that opening /dev/tty failed, which would mean that we have no controlling terminal _but_ our stderr is still connected to some terminal. That might be bizarre enough not to care about). I think the right answer would be a real is_prompt_available() that checked /dev/tty when HAVE_DEV_TTY was set, and otherwise checked isatty(2) (or whatever was appropriate for the platform). The function falls back to getpass() on platforms without DEV_TTY, and if getpass() on some platforms reads from the standard input, that would be a disaster. I wasn't sure about that part. Yeah, although it is already a disaster in those cases, as the main caller of git_terminal_prompt is remote-curl, whose stdin is connected to git via the remote-helper protocol. Which isn't to say this wouldn't make things worse. It would, but the real solution is to implement a sane git_terminal_prompt for those platforms. Erik had a patch for Windows to use their magical CONIN$, but I think it is temporarily stalled. I don't know if there are any other platforms that do not have /dev/tty (I know we do not set HAVE_DEV_TTY by default, but that is only because I was being conservative and waiting for people on particular platforms to confirm that it works before tweaking our Makefile defaults). -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/5] test-lib: filesystem prerequisites
On Thu, Jul 26, 2012 at 11:16:45AM -0700, Junio C Hamano wrote: Consolidating the logic to set necessary prerequisites used in various scripts is very good, but I am not sure adding them to test-lib and run them unconditionally is a good idea. SYMLINKS is used by 47 among 595 tests, which is common enough, but the new ones are not common enough. I do not think we want to create a temporary junk dir, two temporary camelcase files, read and compare them, when nobody in the script cares. We do not have to split them into separate include files, though, in order to avoid such waste. Instead, you can make the logic to set prerequisite conditional inside test-lib.sh and update the users. Something like: [...] (at the beginning of t/t0003-attributes.sh) #!/bin/sh test_description=gitattributes TEST_WANT_PREREQ=CASE_INSENSITIVE_FS,SYMLINKS . ./test-lib.sh That looks like a maintenance annoyance. Can't we just have the prerequisite-checker lazily perform the test on demand and cache the result? It should be OK as long as: 1. The prereq is careful about its pre- and post- conditions. We already make sure to clean up after those tests so as not to taint later tests. We would probably want to also make them more careful about preconditions like which directory they are in (so, for example, refer to $TRASH_DIRECTORY/junk and not junk). 2. The prereq test does not accidentally munge any existing test state from previous tests. That should not be a big deal as long as we avoid names like junk in favor of more unique names like check-case-sensitivity-prereq. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 1/2] difftool: Wrap long lines for readability
Keep everything within 80 columns. Wrap the user-facing messages too.
Signed-off-by: David Aguilar dav...@gmail.com
---
Unchanged since last time -- resending for patch 2/2
git-difftool.perl | 46 --
1 file changed, 32 insertions(+), 14 deletions(-)
diff --git a/git-difftool.perl b/git-difftool.perl
index 10d3d97..8e51238 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -93,15 +93,22 @@ sub print_tool_help
}
}
- print 'git difftool --tool=tool' may be set to one of the
following:\n;
+ print 'EOF';
+'git difftool --tool=tool' may be set to one of the following:
+EOF
print \t$_\n for (sort(@found));
- print \nThe following tools are valid, but not currently available:\n;
+ print 'EOF';
+
+The following tools are valid, but not currently available:
+EOF
print \t$_\n for (sort(@notfound));
- print \nNOTE: Some of the tools listed above only work in a
windowed\n;
- print environment. If run in a terminal-only session, they will
fail.\n;
+ print 'EOF';
+NOTE: Some of the tools listed above only work in a windowed
+environment. If run in a terminal-only session, they will fail.
+EOF
exit(0);
}
@@ -114,8 +121,11 @@ sub setup_dir_diff
# if $GIT_DIR and $GIT_WORK_TREE are set in ENV, they are actually used
# by Git-repository-command*.
my $repo_path = $repo-repo_path();
- my $diffrepo = Git-repository(Repository = $repo_path, WorkingCopy =
$workdir);
- my $diffrtn = $diffrepo-command_oneline('diff', '--raw',
'--no-abbrev', '-z', @ARGV);
+ my %repo_args = (Repository = $repo_path, WorkingCopy = $workdir);
+ my $diffrepo = Git-repository(%repo_args);
+
+ my @gitargs = ('diff', '--raw', '--no-abbrev', '-z', @ARGV);
+ my $diffrtn = $diffrepo-command_oneline(@gitargs);
exit(0) if (length($diffrtn) == 0);
# Setup temp directories
@@ -140,11 +150,15 @@ sub setup_dir_diff
my $i = 0;
while ($i $#rawdiff) {
if ($rawdiff[$i] =~ /^::/) {
- print Combined diff formats ('-c' and '--cc') are not
supported in directory diff mode.\n;
+ warn 'EOF';
+Combined diff formats ('-c' and '--cc') are not supported in
+directory diff mode ('-d' and '--dir-diff').
+EOF
exit(1);
}
- my ($lmode, $rmode, $lsha1, $rsha1, $status) = split(' ',
substr($rawdiff[$i], 1));
+ my ($lmode, $rmode, $lsha1, $rsha1, $status) =
+ split(' ', substr($rawdiff[$i], 1));
my $src_path = $rawdiff[$i + 1];
my $dst_path;
@@ -156,7 +170,7 @@ sub setup_dir_diff
$i += 2;
}
- if (($lmode eq $submodule_mode) or ($rmode eq $submodule_mode))
{
+ if ($lmode eq $submodule_mode or $rmode eq $submodule_mode) {
$submodule{$src_path}{left} = $lsha1;
if ($lsha1 ne $rsha1) {
$submodule{$dst_path}{right} = $rsha1;
@@ -167,14 +181,16 @@ sub setup_dir_diff
}
if ($lmode eq $symlink_mode) {
- $symlink{$src_path}{left} =
$diffrepo-command_oneline('show', $lsha1);
+ $symlink{$src_path}{left} =
+ $diffrepo-command_oneline('show', $lsha1);
}
if ($rmode eq $symlink_mode) {
- $symlink{$dst_path}{right} =
$diffrepo-command_oneline('show', $rsha1);
+ $symlink{$dst_path}{right} =
+ $diffrepo-command_oneline('show', $rsha1);
}
- if (($lmode ne $null_mode) and ($status !~ /^C/)) {
+ if ($lmode ne $null_mode and $status !~ /^C/) {
$lindex .= $lmode $lsha1\t$src_path\0;
}
@@ -199,14 +215,16 @@ sub setup_dir_diff
# Populate the left and right directories based on each index file
my ($inpipe, $ctx);
$ENV{GIT_INDEX_FILE} = $tmpdir/lindex;
- ($inpipe, $ctx) = $repo-command_input_pipe(qw/update-index -z
--index-info/);
+ ($inpipe, $ctx) =
+ $repo-command_input_pipe(qw(update-index -z --index-info));
print($inpipe $lindex);
$repo-command_close_pipe($inpipe, $ctx);
my $rc = system('git', 'checkout-index', '--all', --prefix=$ldir/);
exit($rc | ($rc 8)) if ($rc != 0);
$ENV{GIT_INDEX_FILE} = $tmpdir/rindex;
- ($inpipe, $ctx) = $repo-command_input_pipe(qw/update-index -z
--index-info/);
+ ($inpipe, $ctx) =
+ $repo-command_input_pipe(qw(update-index -z --index-info));
print($inpipe $rindex);
$repo-command_close_pipe($inpipe, $ctx);
$rc = system('git', 'checkout-index', '--all',
Re: Test t/t7502-commit.sh failed
Jeff King p...@peff.net writes: Yes, and they are given that chance. This is not about the behavior of git, but about stupid assumptions by the test. OK. I'm close to finished with a series that I think will at least make it better. Stay tuned. ;-) -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/5] test-lib: filesystem prerequisites
Jeff King p...@peff.net writes: That looks like a maintenance annoyance. Can't we just have the prerequisite-checker lazily perform the test on demand and cache the result? It should be OK as long as: 1. The prereq is careful about its pre- and post- conditions. We already make sure to clean up after those tests so as not to taint later tests. We would probably want to also make them more careful about preconditions like which directory they are in (so, for example, refer to $TRASH_DIRECTORY/junk and not junk). 2. The prereq test does not accidentally munge any existing test state from previous tests. That should not be a big deal as long as we avoid names like junk in favor of more unique names like check-case-sensitivity-prereq. Yeah, it is very desirable if we could lazy-eval, and we _should_ be able to arrange the above. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/6] t7502 fixes
On Thu, Jul 26, 2012 at 03:33:59PM -0400, Jeff King wrote: I'm close to finished with a series that I think will at least make it better. Stay tuned. Here it is. I was able to replicate the original problem by munging my /etc/passwd, and I've confirmed that this series fixes it by skipping the related test. I also added in a test for the early-quit behavior introduced by f20f387. It can't run everywhere, of course, but it seems we have at least one tester whose system will run it (and I did confirm that it works with my munged /etc/passwd). One other option is that we could introduce a GIT_PRETEND_THIS_IS_MY_GECOS_NAME variable. That would let us wrong both sides of the test on all systems. I just hate to add an interface that will be carried around in production code just for the sake of a test. [1/6]: t7502: clean up fake_editor tests [2/6]: t7502: properly quote GIT_EDITOR [3/6]: t7502: narrow checks for author/committer name in template [4/6]: t7502: drop confusing test_might_fail call [5/6]: t7502: handle systems where auto-identity is broken [6/6]: t7502: test early quit from commit with bad ident -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/6] t7502: properly quote GIT_EDITOR
One of the tests tries to ensure that editor is not run due to an early failure. However, it needs to quote the pathname of the trash directory used in $GIT_EDITOR, since git will pass it along to the shell. In other words, the test would pass whether the code was correct or not, since the unquoted editor specification would never run. We never noticed the problem because the code is indeed correct, so git-commit never even tried to run the editor. Signed-off-by: Jeff King p...@peff.net --- t/t7502-commit.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh index ddce53a..3f9fb55 100755 --- a/t/t7502-commit.sh +++ b/t/t7502-commit.sh @@ -290,7 +290,7 @@ test_expect_success 'do not fire editor in the presence of conflicts' ' test_must_fail git cherry-pick -n master echo editor not started .git/result ( - GIT_EDITOR=$(pwd)/.git/FAKE_EDITOR + GIT_EDITOR=\$(pwd)/.git/FAKE_EDITOR\ export GIT_EDITOR test_must_fail git commit ) -- 1.7.11.3.8.ge78f547 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/6] t7502: narrow checks for author/committer name in template
t7502.20 and t7502.21 check that the author and committer name are mentioned in the commit message template under certain circumstances. However, they end up checking a much larger and unnecessary portion of the template. Let's narrow their checks to the specific lines. While we're at it, let's give these tests more descriptive names, so their purposes are more obvious. Signed-off-by: Jeff King p...@peff.net --- The test just above still checks the Please write your... part of the message. But it is purely about using -F -e --cleanup=strip, and is testing whether we correctly include the instructions. I guess we could limit it to just checking for ^# or something if we didn't want to depend on the actual text. I'm inclined to just leave it for now. t/t7502-commit.sh | 25 - 1 file changed, 8 insertions(+), 17 deletions(-) diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh index 3f9fb55..efecb06 100755 --- a/t/t7502-commit.sh +++ b/t/t7502-commit.sh @@ -235,24 +235,15 @@ test_expect_success 'cleanup commit messages (strip,-F,-e): output' ' test_i18ncmp expect actual ' -echo # -# Author:$GIT_AUTHOR_NAME $GIT_AUTHOR_EMAIL -# expect - -test_expect_success 'author different from committer' ' +test_expect_success 'message shows author when it is not equal to committer' ' echo negative test_might_fail git commit -e -m sample - head -n 7 .git/COMMIT_EDITMSG actual - test_i18ncmp expect actual + test_i18ngrep \ + ^# Author: *A U Thor aut...@example.com\$ \ + .git/COMMIT_EDITMSG ' -mv expect expect.tmp -sed '$d' expect.tmp expect -rm -f expect.tmp -echo # Committer: -# expect - -test_expect_success 'committer is automatic' ' +test_expect_success 'message shows committer when it is automatic' ' echo negative ( @@ -261,9 +252,9 @@ test_expect_success 'committer is automatic' ' # must fail because there is no change test_must_fail git commit -e -m sample ) - head -n 8 .git/COMMIT_EDITMSG | \ - sed s/^# Committer: .*/# Committer:/ actual - test_i18ncmp expect actual + # the ident is calculated from the system, so we cannot + # check the actual value, only that it is there + test_i18ngrep ^# Committer: .git/COMMIT_EDITMSG ' write_script .git/FAKE_EDITOR EOF -- 1.7.11.3.8.ge78f547 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 4/6] t7502: drop confusing test_might_fail call
In t7502.20, we run git commit and check that it warns us that the author and committer identity are not the same (this is always the case in the test environment, since we set up the idents differently). Instead of actually making a commit, we have a clean index, so the git commit we run will fail. This is marked as might_fail, which is not really correct; it will always fail since there is nothing to commit. However, the only reason not to do a complete commit would be to see the intermediate state of the COMMIT_EDITMSG file when the commit is not completed. We don't need to care about this, though; even a complete commit will leave COMMIT_EDITMSG for us to view. By doing a real commit and dropping the might_fail, we are more robust against other unforeseen failures of git commit that might influence our test result. It might seem less robust to depend on the fact that git commit leaves COMMIT_EDITMSG in place after a successful commit. However, that brings this test in line with others parts of the script, which make the same assumption. Furthermore, if that ever does change, the right solution is not to prevent commit from completing, but to set EDITOR to a script that will record the contents we see. After all, the point of these tests is to check what the user sees in their EDITOR, so that would be the most direct test. For now, though, we can continue to use the shortcut that COMMIT_EDITMSG is left intact. Signed-off-by: Jeff King p...@peff.net --- Sorry for the long explanation. It took me a long time to figure out why this test_might_fail is there at all, so I wanted to present my full analysis as to why it's OK to remove. t/t7502-commit.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh index efecb06..d261b82 100755 --- a/t/t7502-commit.sh +++ b/t/t7502-commit.sh @@ -237,7 +237,7 @@ test_expect_success 'cleanup commit messages (strip,-F,-e): output' ' test_expect_success 'message shows author when it is not equal to committer' ' echo negative - test_might_fail git commit -e -m sample + git commit -e -m sample -a test_i18ngrep \ ^# Author: *A U Thor aut...@example.com\$ \ .git/COMMIT_EDITMSG -- 1.7.11.3.8.ge78f547 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 5/6] t7502: handle systems where auto-identity is broken
Test t7502.21 checks whether we write the committer name into COMMIT_EDITMSG when it has been automatically determined. However, not all systems can produce valid automatic identities. Prior to f20f387 (commit: check committer identity more strictly), this test worked even when we did not have a valid automatic identity, since it did not run the strict test until after we had generated the template. That commit tightened the check to fail early (since we would fail later, anyway), meaning that systems without a valid GECOS name or hostname would fail the test. We cannot just work around this, because it depends on configuration outside the control of the test script. Therefore we introduce a new test_prerequisite to run this test only on systems where automatic ident works at all. As a result, we can drop the confusing test_must_fail bit from the test. The intent was that by giving git commit invalid input (namely, nothing to commit), that it would stop at a predictable point, whether we had a valid identity or not, from which we could view the contents of COMMIT_EDITMSG. Since that assumption no longer holds, and we can only run the test when we have a valid identity, there is no reason not to let commit run to completion. That lets us be more robust to other unforeseen failures. Signed-off-by: Jeff King p...@peff.net --- This should fix the test failure that started the thread. Please let me know if it doesn't. t/t7502-commit.sh | 13 ++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh index d261b82..c444812 100755 --- a/t/t7502-commit.sh +++ b/t/t7502-commit.sh @@ -243,14 +243,21 @@ test_expect_success 'message shows author when it is not equal to committer' ' .git/COMMIT_EDITMSG ' -test_expect_success 'message shows committer when it is automatic' ' +test_expect_success 'setup auto-ident prerequisite' ' + if (sane_unset GIT_COMMITTER_EMAIL + sane_unset GIT_COMMITTER_NAME + git var GIT_COMMITTER_IDENT); then + test_set_prereq AUTOIDENT + fi +' + +test_expect_success AUTOIDENT 'message shows committer when it is automatic' ' echo negative ( sane_unset GIT_COMMITTER_EMAIL sane_unset GIT_COMMITTER_NAME - # must fail because there is no change - test_must_fail git commit -e -m sample + git commit -e -m sample -a ) # the ident is calculated from the system, so we cannot # check the actual value, only that it is there -- 1.7.11.3.8.ge78f547 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 6/6] t7502: test early quit from commit with bad ident
In commit f20f387, git commit notices and dies much earlier when we have a bogus commit identity. That commit did not add a test because we cannot do so reliably (namely, we can only trigger the behavior on a system where the automatically generated identity is bogus). However, now that we have a prerequisite check for this feature, we can add a test that will at least run on systems that produce such a bogus identity. Signed-off-by: Jeff King p...@peff.net --- t/t7502-commit.sh | 17 + 1 file changed, 17 insertions(+) diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh index c444812..deb187e 100755 --- a/t/t7502-commit.sh +++ b/t/t7502-commit.sh @@ -248,6 +248,8 @@ test_expect_success 'setup auto-ident prerequisite' ' sane_unset GIT_COMMITTER_NAME git var GIT_COMMITTER_IDENT); then test_set_prereq AUTOIDENT + else + test_set_prereq NOAUTOIDENT fi ' @@ -269,6 +271,21 @@ echo editor started $(pwd)/.git/result exit 0 EOF +test_expect_success NOAUTOIDENT 'do not fire editor when committer is bogus' ' + .git/result + expect + + echo negative + ( + sane_unset GIT_COMMITTER_EMAIL + sane_unset GIT_COMMITTER_NAME + GIT_EDITOR=\$(pwd)/.git/FAKE_EDITOR\ + export GIT_EDITOR + test_must_fail git commit -e -m sample -a + ) + test_cmp expect .git/result +' + test_expect_success 'do not fire editor in the presence of conflicts' ' git clean -f -- 1.7.11.3.8.ge78f547 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9] git on Mac OS and precomposed unicode
Just a couple of nitpicks.
Torsten Bögershausen skrev 2012-07-08 15.50:
diff --git a/compat/precompose_utf8.c b/compat/precompose_utf8.c
[...]
+static size_t has_utf8(const char *s, size_t maxlen, size_t *strlen_c)
+{
+ const uint8_t *utf8p = (const uint8_t*) s;
+ size_t strlen_chars = 0;
+ size_t ret = 0;
+
+ if ((!utf8p) || (!*utf8p)) {
Style: Drop the extra parentheses
+ return 0;
+ }
+
+ while((*utf8p) maxlen) {
Style: Drop the extra parentheses
[...]
+void probe_utf8_pathname_composition(char *path, int len)
+{
+ const static char *auml_nfc = \xc3\xa4;
+ const static char *auml_nfd = \x61\xcc\x88;
+ int output_fd;
+ if (precomposed_unicode != -1)
+ return; /* We found it defined in the global config, respect it
*/
+ path[len] = 0;
Not needed, will be overwritten by strcpy
+ strcpy(path + len, auml_nfc);
+ output_fd = open(path, O_CREAT|O_EXCL|O_RDWR, 0600);
+ if (output_fd =0) {
+ close(output_fd);
+ path[len] = 0;
Not needed, will be overwritten by strcpy
+ strcpy(path + len, auml_nfd);
+ /* Indicate to the user, that we can configure it to true */
+ if (0 == access(path, R_OK))
+ git_config_set(core.precomposeunicode, false);
+ /* To be backward compatible, set precomposed_unicode
to 0 */
+ precomposed_unicode = 0;
+ path[len] = 0;
Not needed, will be overwritten by strcpy
+ strcpy(path + len, auml_nfc);
+ unlink(path);
Err out if path cannot be deleted?
-- robin
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/6] t7502 fixes
All patches look very sensible. Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
The GitTogether
For the last few years, there has been a gathering of Gitty people in Mountain View directly following the GSoC Mentor Summit that is referred to as a GitTogether: https://git.wiki.kernel.org/index.php/GitTogether A few of us have been talking about what we would like to do this year and thinking about the gatherings the past few years and how we could get the most out of it. I would like to see two different gatherings this year - one that would be user-centric to gather people that use Git together with some of the developers and talk about Git from a user's perspective. The other event I would like to see would be a gathering of many of the core Git developers in a sort of hacker summit. GitHub would like to volunteer to organize and pay for these events this year. I would like to hold the developer-centric one in Berlin in early October (a few weeks before the Mentor Summit this time) and the user one in January or February of next year. The general idea of the developer one in October would be to get 30-40 people who work directly on Git core, JGit and libgit2 (or closely related projects) together to discuss core issues, new features, etc. GitHub can help with travel and lodging for participants who need it, but attendance would be limited to people actually working on Git the most. Similar to some of the earlier GitTogethers. The user conference early next year would be held in San Francisco or nearby and would be a chance for people using Git to share how they're using it, what they would like to see, etc. I would expect to host far more people at this - closer to 100, something like the last GitTogether. I'm working on putting together websites for the two events for registration, schedule and to gather topics that should be discussed. I am planning on having the talks recorded and put online as well. I wanted to get some general feedback from the ML about what they think about this plan before I finalized everything though. For those of you who *have* been to a GitTogether, what did you find useful and/or useless about it? What did you get out of it and would like to see again? For those of you who have never been, what do you think would be useful? I was thinking for both of them to have a combination of short prepared talks, lightning/unconference style talks and general discussion / breakout sessions. Finally, is there any feedback on the times and places - especially the Berlin one. If nobody can agree on a better specific time, I'll push forward with early October in Berlin, but if there is a concensus around a different time, I'm fine moving it. Scott -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9] git on Mac OS and precomposed unicode
Robin Rosenberg robin.rosenb...@dewire.com writes: Just a couple of nitpicks. Polishing is always good and better late than never, but for a topic that has long been graduated to 'master' already, it would be easier to review and discuss if it came as a patch form relative to the codebase _after_ the topic has been applied. Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/2] test-lib: provide case insensitivity as a prerequisite
And on top, Michael's [1/5] would become like this, and [2/5] would apply unchanged. -- 8 -- From: Michael J Gruber g...@drmicha.warpmail.net Date: Thu, 26 Jul 2012 15:39:53 +0200 Case insensitivity plays a role in several tests and is tested in several tests. Therefore, move the test from t003 into the test lib and use the prerequisite in t0003. Signed-off-by: Michael J Gruber g...@drmicha.warpmail.net Signed-off-by: Junio C Hamano gits...@pobox.com --- t/README | 4 t/t0003-attributes.sh | 10 -- t/test-lib.sh | 6 ++ 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/t/README b/t/README index 4c3ea25..5725607 100644 --- a/t/README +++ b/t/README @@ -625,6 +625,10 @@ use these, and test_set_prereq for how to define your own. Git was compiled with USE_LIBPCRE=YesPlease. Wrap any tests that use git-grep --perl-regexp or git-grep -P in these. + - CASE_INSENSITIVE_FS + + Test is run on a case insensitive file system. + Tips for Writing Tests -- diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh index 51f3045..febc45c 100755 --- a/t/t0003-attributes.sh +++ b/t/t0003-attributes.sh @@ -123,16 +123,6 @@ test_expect_success 'attribute matching is case insensitive when core.ignorecase ' -test_expect_success 'check whether FS is case-insensitive' ' - mkdir junk - echo good junk/CamelCase - echo bad junk/camelcase - if test $(cat junk/CamelCase) != good - then - test_set_prereq CASE_INSENSITIVE_FS - fi -' - test_expect_success CASE_INSENSITIVE_FS 'additional case insensitivity tests' ' test_must_fail attr_check a/B/D/g a/b/d/* -c core.ignorecase=0 test_must_fail attr_check A/B/D/NO a/b/d/* -c core.ignorecase=0 diff --git a/t/test-lib.sh b/t/test-lib.sh index 878d000..52cb32a 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -664,6 +664,12 @@ test_lazy_prereq SYMLINKS ' ln -s x y 2/dev/null test -h y 2/dev/null ' +test_lazy_prereq CASE_INSENSITIVE_FS ' + echo good CamelCase + echo bad camelcase + test $(cat CamelCase) != good +' + # When the tests are run as root, permission tests will report that # things are writable when they shouldn't be. test -w / || test_set_prereq SANITY -- 1.7.12.rc0.44.gc69a8ad -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Extract Git::SVN from git-svn, take 2.
Same as before, now with tab indentation in the new Perl tests. As before, patch #3 is 132k and will be rejected by some of the lists. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/4] Extract some utilities from git-svn to allow extracting Git::SVN.
From: Michael G. Schwern schw...@pobox.com
Put them in a new module called Git::SVN::Utils. Yeah, not terribly
original and it will be a dumping ground. But its better than having
them in the main git-svn program. At least they can be documented
and tested.
* fatal() is used by many classes.
* Change the $can_compress lexical into a function.
This should be enough to extract Git::SVN.
Signed-off-by: Michael G. Schwern schw...@pobox.com
---
git-svn.perl | 34 +---
perl/Git/SVN/Utils.pm | 59 ++
perl/Makefile | 1 +
t/Git-SVN/00compile.t | 8 ++
t/Git-SVN/Utils/can_compress.t | 11
t/Git-SVN/Utils/fatal.t| 34
6 files changed, 132 insertions(+), 15 deletions(-)
create mode 100644 perl/Git/SVN/Utils.pm
create mode 100644 t/Git-SVN/00compile.t
create mode 100644 t/Git-SVN/Utils/can_compress.t
create mode 100644 t/Git-SVN/Utils/fatal.t
diff --git a/git-svn.perl b/git-svn.perl
index 0b074c4..79fe4a4 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -10,6 +10,8 @@ use vars qw/ $AUTHOR $VERSION
$AUTHOR = 'Eric Wong normalper...@yhbt.net';
$VERSION = '@@GIT_VERSION@@';
+use Git::SVN::Utils qw(fatal can_compress);
+
# From which subdir have we been invoked?
my $cmd_dir_prefix = eval {
command_oneline([qw/rev-parse --show-prefix/], STDERR = 0)
@@ -35,8 +37,6 @@ $Git::SVN::Log::TZ = $ENV{TZ};
$ENV{TZ} = 'UTC';
$| = 1; # unbuffer STDOUT
-sub fatal (@) { print STDERR @_\n; exit 1 }
-
# All SVN commands do it. Otherwise we may die on SIGPIPE when the remote
# repository decides to close the connection which we expect to be kept alive.
$SIG{PIPE} = 'IGNORE';
@@ -66,7 +66,7 @@ sub _req_svn {
fatal Need SVN::Core 1.1.0 or better (got
$SVN::Core::VERSION);
}
}
-my $can_compress = eval { require Compress::Zlib; 1};
+
use Carp qw/croak/;
use Digest::MD5;
use IO::File qw//;
@@ -1578,7 +1578,7 @@ sub cmd_reset {
}
sub cmd_gc {
- if (!$can_compress) {
+ if (!can_compress()) {
warn Compress::Zlib could not be found; unhandled.log .
files will not be compressed.\n;
}
@@ -2014,13 +2014,13 @@ sub md5sum {
} elsif (!$ref) {
$md5-add($arg) or croak $!;
} else {
- ::fatal Can't provide MD5 hash for unknown ref type: ', $ref,
';
+ fatal Can't provide MD5 hash for unknown ref type: ', $ref,
';
}
return $md5-hexdigest();
}
sub gc_directory {
- if ($can_compress -f $_ basename($_) eq unhandled.log) {
+ if (can_compress() -f $_ basename($_) eq unhandled.log) {
my $out_filename = $_ . .gz;
open my $in_fh, , $_ or die Unable to open $_: $!\n;
binmode $in_fh;
@@ -2055,6 +2055,9 @@ use Time::Local;
use Memoize; # core since 5.8.0, Jul 2002
use Memoize::Storable;
use POSIX qw(:signal_h);
+
+use Git::SVN::Utils qw(fatal can_compress);
+
my $can_use_yaml;
BEGIN {
$can_use_yaml = eval { require Git::SVN::Memoize::YAML; 1};
@@ -2880,8 +2883,8 @@ sub assert_index_clean {
command_noisy('read-tree', $treeish);
$x = command_oneline('write-tree');
if ($y ne $x) {
- ::fatal trees ($treeish) $y != $x\n,
- Something is seriously wrong...;
+ fatal trees ($treeish) $y != $x\n,
+ Something is seriously wrong...;
}
});
}
@@ -3236,7 +3239,7 @@ sub mkemptydirs {
my %empty_dirs = ();
my $gz_file = $self-{dir}/unhandled.log.gz;
if (-f $gz_file) {
- if (!$can_compress) {
+ if (!can_compress()) {
warn Compress::Zlib could not be found; ,
empty directories in $gz_file will not be read\n;
} else {
@@ -3919,7 +3922,7 @@ sub set_tree {
my ($self, $tree) = (shift, shift);
my $log_entry = ::get_commit_entry($tree);
unless ($self-{last_rev}) {
- ::fatal(Must have an existing revision to commit);
+ fatal(Must have an existing revision to commit);
}
my %ed_opts = ( r = $self-{last_rev},
log = $log_entry-{log},
@@ -4348,6 +4351,7 @@ sub remove_username {
package Git::SVN::Log;
use strict;
use warnings;
+use Git::SVN::Utils qw(fatal);
use POSIX qw/strftime/;
use constant commit_log_separator = ('-' x 72) . \n;
use vars qw/$TZ $limit $color $pager $non_recursive $verbose $oneline
@@ -4446,15 +4450,15 @@ sub config_pager {
sub run_pager {
return unless defined $pager;
pipe my ($rfd, $wfd) or return;
- defined(my $pid = fork) or ::fatal Can't fork: $!;
+ defined(my $pid = fork) or fatal Can't fork: $!;
if (!$pid) {
[PATCH 4/4] Move initialization of Git::SVN variables into Git::SVN.
From: Michael G. Schwern schw...@pobox.com
Also it can compile on its own now, yay!
---
git-svn.perl | 4
perl/Git/SVN.pm | 9 +++--
t/Git-SVN/00compile.t | 3 ++-
3 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/git-svn.perl b/git-svn.perl
index 4c77f69..ef10f6f 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -20,10 +20,7 @@ my $cmd_dir_prefix = eval {
my $git_dir_user_set = 1 if defined $ENV{GIT_DIR};
$ENV{GIT_DIR} ||= '.git';
-$Git::SVN::default_repo_id = 'svn';
-$Git::SVN::default_ref_id = $ENV{GIT_SVN_ID} || 'git-svn';
$Git::SVN::Ra::_log_window_size = 100;
-$Git::SVN::_minimize_url = 'unset';
if (! exists $ENV{SVN_SSH} exists $ENV{GIT_SSH}) {
$ENV{SVN_SSH} = $ENV{GIT_SSH};
@@ -114,7 +111,6 @@ my ($_stdin, $_help, $_edit,
# This is a refactoring artifact so Git::SVN can get at this git-svn switch.
sub opt_prefix { return $_prefix || '' }
-$Git::SVN::_follow_parent = 1;
$Git::SVN::Fetcher::_placeholder_filename = .gitignore;
$_q ||= 0;
my %remote_opts = ( 'username=s' = \$Git::SVN::Prompt::_username,
diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm
index c71c041..2e0d7f0 100644
--- a/perl/Git/SVN.pm
+++ b/perl/Git/SVN.pm
@@ -3,9 +3,9 @@ use strict;
use warnings;
use Fcntl qw/:DEFAULT :seek/;
use constant rev_map_fmt = 'NH40';
-use vars qw/$default_repo_id $default_ref_id $_no_metadata $_follow_parent
+use vars qw/$_no_metadata
$_repack $_repack_flags $_use_svm_props $_head
-$_use_svnsync_props $no_reuse_existing $_minimize_url
+$_use_svnsync_props $no_reuse_existing
$_use_log_author $_add_author_from $_localtime/;
use Carp qw/croak/;
use File::Path qw/mkpath/;
@@ -30,6 +30,11 @@ BEGIN {
$can_use_yaml = eval { require Git::SVN::Memoize::YAML; 1};
}
+our $_follow_parent = 1;
+our $_minimize_url = 'unset';
+our $default_repo_id = 'svn';
+our $default_ref_id = $ENV{GIT_SVN_ID} || 'git-svn';
+
my ($_gc_nr, $_gc_period);
# properties that we do not log:
diff --git a/t/Git-SVN/00compile.t b/t/Git-SVN/00compile.t
index a7aa85a..97475d9 100644
--- a/t/Git-SVN/00compile.t
+++ b/t/Git-SVN/00compile.t
@@ -3,6 +3,7 @@
use strict;
use warnings;
-use Test::More tests = 1;
+use Test::More tests = 2;
require_ok 'Git::SVN::Utils';
+require_ok 'Git::SVN';
--
1.7.11.1
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 4/2] test-lib: provide UTF8 behaviour as a prerequisite
And Michael's [4/5] would become like this (again, [3/5] does not need any change). -- 8 -- From: Michael J Gruber g...@drmicha.warpmail.net Date: Thu, 26 Jul 2012 15:39:56 +0200 UTF8 behaviour of the filesystem (conversion from nfd to nfc) plays a role in several tests and is tested in several tests. Therefore, move the test from t0050 into the test lib and use the prerequisite in t0050. Signed-off-by: Michael J Gruber g...@drmicha.warpmail.net Signed-off-by: Junio C Hamano gits...@pobox.com --- t/README | 5 + t/t0050-filesystem.sh | 24 +++- t/test-lib.sh | 13 + 3 files changed, 25 insertions(+), 17 deletions(-) diff --git a/t/README b/t/README index 5725607..e4128e5 100644 --- a/t/README +++ b/t/README @@ -629,6 +629,11 @@ use these, and test_set_prereq for how to define your own. Test is run on a case insensitive file system. + - UTF8_NFD_TO_NFC + + Test is run on a filesystem which converts decomposed utf-8 (nfd) + to precomposed utf-8 (nfc). + Tips for Writing Tests -- diff --git a/t/t0050-filesystem.sh b/t/t0050-filesystem.sh index b46ae72..78816d9 100755 --- a/t/t0050-filesystem.sh +++ b/t/t0050-filesystem.sh @@ -7,22 +7,6 @@ test_description='Various filesystem issues' auml=$(printf '\303\244') aumlcdiar=$(printf '\141\314\210') -unibad= -test_expect_success 'see what we expect' ' - - test_unicode=test_expect_success - mkdir junk - junk/$auml - case $(cd junk echo *) in - $aumlcdiar) - test_unicode=test_expect_failure - unibad=t - ;; - *) ;; - esac - rm -fr junk -' - if test_have_prereq CASE_INSENSITIVE_FS then say will test on a case insensitive filesystem @@ -31,8 +15,14 @@ else test_case=test_expect_success fi -test $unibad +if test_have_prereq UTF8_NFD_TO_NFC +then say will test on a unicode corrupting filesystem + test_unicode=test_expect_failure +else + test_unicode=test_expect_success +fi + test_have_prereq SYMLINKS || say will test on a filesystem lacking symbolic links diff --git a/t/test-lib.sh b/t/test-lib.sh index 52cb32a..95c966e 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -670,6 +670,19 @@ test_lazy_prereq CASE_INSENSITIVE_FS ' test $(cat CamelCase) != good ' +test_lazy_prereq UTF8_NFD_TO_NFC ' + # check whether FS converts nfd unicode to nfc + auml=$(printf \303\244) + aumlcdiar=$(printf \141\314\210) + $auml + case $(echo *) in + $aumlcdiar) + true ;; + *) + false ;; + esac +' + # When the tests are run as root, permission tests will report that # things are writable when they shouldn't be. test -w / || test_set_prereq SANITY -- 1.7.12.rc0.44.gc69a8ad -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Extract remaining classes from git-svn
This series of patches extracts the remaining classes from git-svn. They're all simple extractions and functionally have no change. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/8] Prepare Git::SVN::Log for extraction from git-svn.
From: Michael G. Schwern schw...@pobox.com
* Load Git command functions itself.
* Can't access the git-svn switch lexical any more, but its only used by
Git::SVN::Log so turn it into a Git::SVN::Log global.
* Load Git::SVN as needed. No need to load it always, its only used twice.
* Moved a state variable to the routine it's used for. (Drive by refactoring)
---
git-svn.perl | 15 +++
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/git-svn.perl b/git-svn.perl
index ef10f6f..e16475b 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -87,7 +87,7 @@ BEGIN {
foreach (qw/command command_oneline command_noisy command_output_pipe
command_input_pipe command_close_pipe
command_bidi_pipe command_close_bidi_pipe/) {
- for my $package ( qw(Git::SVN::Migration Git::SVN::Log),
+ for my $package ( qw(Git::SVN::Migration),
__PACKAGE__) {
*{${package}::$_} = \{Git::$_};
}
@@ -106,7 +106,7 @@ my ($_stdin, $_help, $_edit,
$_version, $_fetch_all, $_no_rebase, $_fetch_parent,
$_merge, $_strategy, $_preserve_merges, $_dry_run, $_local,
$_prefix, $_no_checkout, $_url, $_verbose,
- $_git_format, $_commit_url, $_tag, $_merge_info, $_interactive);
+ $_commit_url, $_tag, $_merge_info, $_interactive);
# This is a refactoring artifact so Git::SVN can get at this git-svn switch.
sub opt_prefix { return $_prefix || '' }
@@ -270,7 +270,7 @@ my %cmd = (
{ 'url' = \$_url, } ],
'blame' = [ \Git::SVN::Log::cmd_blame,
Show what revision and author last modified each line of a
file,
- { 'git-format' = \$_git_format } ],
+ { 'git-format' = \$Git::SVN::Log::_git_format } ],
'reset' = [ \cmd_reset,
Undo fetches back to the specified SVN revision,
{ 'revision|r=s' = \$_revision,
@@ -2044,11 +2044,14 @@ package Git::SVN::Log;
use strict;
use warnings;
use Git::SVN::Utils qw(fatal);
+use Git qw(command command_oneline command_output_pipe command_close_pipe);
use POSIX qw/strftime/;
use constant commit_log_separator = ('-' x 72) . \n;
use vars qw/$TZ $limit $color $pager $non_recursive $verbose $oneline
%rusers $show_commit $incremental/;
-my $l_fmt;
+
+# Option set in git-svn
+our $_git_format;
sub cmt_showable {
my ($c) = @_;
@@ -2094,6 +2097,8 @@ sub git_svn_log_cmd {
}
my ($url, $rev, $uuid, $gs) = ::working_head_info($head);
+
+ require Git::SVN;
$gs ||= Git::SVN-_new;
my @cmd = (qw/log --abbrev-commit --pretty=raw --default/,
$gs-refname);
@@ -2155,6 +2160,7 @@ sub run_pager {
sub format_svn_date {
my $t = shift || time;
+ require Git::SVN;
my $gmoff = Git::SVN::get_tz($t);
return strftime(%Y-%m-%d %H:%M:%S $gmoff (%a, %d %b %Y),
localtime($t));
}
@@ -2225,6 +2231,7 @@ sub process_commit {
return 1;
}
+my $l_fmt;
sub show_commit {
my $c = shift;
if ($oneline) {
--
1.7.11.1
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/8] Extract Git::SVN::Log from git-svn.
From: Michael G. Schwern schw...@pobox.com
Straight cut paste.
Also noticed Git::SVN::Ra wasn't in the compile test. It is now.
---
git-svn.perl | 395 +-
perl/Git/SVN/Log.pm | 395 ++
perl/Makefile | 1 +
t/Git-SVN/00compile.t | 6 +-
4 files changed, 401 insertions(+), 396 deletions(-)
create mode 100644 perl/Git/SVN/Log.pm
diff --git a/git-svn.perl b/git-svn.perl
index e16475b..7c8da44 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -11,6 +11,7 @@ $AUTHOR = 'Eric Wong normalper...@yhbt.net';
$VERSION = '@@GIT_VERSION@@';
use Git::SVN;
+use Git::SVN::Log;
use Git::SVN::Utils qw(fatal can_compress);
# From which subdir have we been invoked?
@@ -2040,400 +2041,6 @@ sub gc_directory {
}
-package Git::SVN::Log;
-use strict;
-use warnings;
-use Git::SVN::Utils qw(fatal);
-use Git qw(command command_oneline command_output_pipe command_close_pipe);
-use POSIX qw/strftime/;
-use constant commit_log_separator = ('-' x 72) . \n;
-use vars qw/$TZ $limit $color $pager $non_recursive $verbose $oneline
-%rusers $show_commit $incremental/;
-
-# Option set in git-svn
-our $_git_format;
-
-sub cmt_showable {
- my ($c) = @_;
- return 1 if defined $c-{r};
-
- # big commit message got truncated by the 16k pretty buffer in rev-list
- if ($c-{l} $c-{l}-[-1] eq ...\n
- $c-{a_raw} =~ /\@([a-f\d\-]+)$/) {
- @{$c-{l}} = ();
- my @log = command(qw/cat-file commit/, $c-{c});
-
- # shift off the headers
- shift @log while ($log[0] ne '');
- shift @log;
-
- # TODO: make $c-{l} not have a trailing newline in the future
- @{$c-{l}} = map { $_\n } grep !/^git-svn-id: /, @log;
-
- (undef, $c-{r}, undef) = ::extract_metadata(
- (grep(/^git-svn-id: /, @log))[-1]);
- }
- return defined $c-{r};
-}
-
-sub log_use_color {
- return $color || Git-repository-get_colorbool('color.diff');
-}
-
-sub git_svn_log_cmd {
- my ($r_min, $r_max, @args) = @_;
- my $head = 'HEAD';
- my (@files, @log_opts);
- foreach my $x (@args) {
- if ($x eq '--' || @files) {
- push @files, $x;
- } else {
- if (::verify_ref($x^0)) {
- $head = $x;
- } else {
- push @log_opts, $x;
- }
- }
- }
-
- my ($url, $rev, $uuid, $gs) = ::working_head_info($head);
-
- require Git::SVN;
- $gs ||= Git::SVN-_new;
- my @cmd = (qw/log --abbrev-commit --pretty=raw --default/,
- $gs-refname);
- push @cmd, '-r' unless $non_recursive;
- push @cmd, qw/--raw --name-status/ if $verbose;
- push @cmd, '--color' if log_use_color();
- push @cmd, @log_opts;
- if (defined $r_max $r_max == $r_min) {
- push @cmd, '--max-count=1';
- if (my $c = $gs-rev_map_get($r_max)) {
- push @cmd, $c;
- }
- } elsif (defined $r_max) {
- if ($r_max $r_min) {
- ($r_min, $r_max) = ($r_max, $r_min);
- }
- my (undef, $c_max) = $gs-find_rev_before($r_max, 1, $r_min);
- my (undef, $c_min) = $gs-find_rev_after($r_min, 1, $r_max);
- # If there are no commits in the range, both $c_max and $c_min
- # will be undefined. If there is at least 1 commit in the
- # range, both will be defined.
- return () if !defined $c_min || !defined $c_max;
- if ($c_min eq $c_max) {
- push @cmd, '--max-count=1', $c_min;
- } else {
- push @cmd, '--boundary', $c_min..$c_max;
- }
- }
- return (@cmd, @files);
-}
-
-# adapted from pager.c
-sub config_pager {
- if (! -t *STDOUT) {
- $ENV{GIT_PAGER_IN_USE} = 'false';
- $pager = undef;
- return;
- }
- chomp($pager = command_oneline(qw(var GIT_PAGER)));
- if ($pager eq 'cat') {
- $pager = undef;
- }
- $ENV{GIT_PAGER_IN_USE} = defined($pager);
-}
-
-sub run_pager {
- return unless defined $pager;
- pipe my ($rfd, $wfd) or return;
- defined(my $pid = fork) or fatal Can't fork: $!;
- if (!$pid) {
- open STDOUT, '', $wfd or
-fatal Can't redirect to stdout: $!;
- return;
- }
- open STDIN, '', $rfd or fatal Can't redirect stdin: $!;
- $ENV{LESS} ||= 'FRSX';
- exec $pager or fatal Can't run pager: $! ($pager);
-}
-
-sub format_svn_date {
- my $t = shift || time;
-
[PATCH 8/8] Fix indents to match style.
From: Michael G. Schwern schw...@pobox.com
---
git-svn.perl | 16
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/git-svn.perl b/git-svn.perl
index 584e93a..4d173d4 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -31,14 +31,14 @@ use Git::SVN::Migration;
use Git::SVN::Utils qw(fatal can_compress);
use Git qw(
-git_cmd_try
-command
-command_oneline
-command_noisy
-command_output_pipe
-command_close_pipe
-command_bidi_pipe
-command_close_bidi_pipe
+ git_cmd_try
+ command
+ command_oneline
+ command_noisy
+ command_output_pipe
+ command_close_pipe
+ command_bidi_pipe
+ command_close_bidi_pipe
);
BEGIN {
--
1.7.11.1
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 7/8] Extract Git::SVN::GlobSpec from git-svn.
From: Michael G. Schwern schw...@pobox.com
Straight cut paste. That's the last class.
* Make Git::SVN load it on its own, its the only thing that needs it.
---
git-svn.perl | 59
perl/Git/SVN.pm | 2 ++
perl/Git/SVN/GlobSpec.pm | 59
perl/Makefile| 1 +
t/Git-SVN/00compile.t| 3 ++-
5 files changed, 64 insertions(+), 60 deletions(-)
create mode 100644 perl/Git/SVN/GlobSpec.pm
diff --git a/git-svn.perl b/git-svn.perl
index 0856a77..584e93a 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -2040,65 +2040,6 @@ sub gc_directory {
}
}
-
-package Git::SVN::GlobSpec;
-use strict;
-use warnings;
-
-sub new {
- my ($class, $glob, $pattern_ok) = @_;
- my $re = $glob;
- $re =~ s!/+$!!g; # no need for trailing slashes
- my (@left, @right, @patterns);
- my $state = left;
- my $die_msg = Only one set of wildcard directories .
- (e.g. '*' or '*/*/*') is supported: '$glob'\n;
- for my $part (split(m|/|, $glob)) {
- if ($part =~ /\*/ $part ne *) {
- die Invalid pattern in '$glob': $part\n;
- } elsif ($pattern_ok $part =~ /[{}]/
-$part !~ /^\{[^{}]+\}/) {
- die Invalid pattern in '$glob': $part\n;
- }
- if ($part eq *) {
- die $die_msg if $state eq right;
- $state = pattern;
- push(@patterns, [^/]*);
- } elsif ($pattern_ok $part =~ /^\{(.*)\}$/) {
- die $die_msg if $state eq right;
- $state = pattern;
- my $p = quotemeta($1);
- $p =~ s/\\,/|/g;
- push(@patterns, (?:$p));
- } else {
- if ($state eq left) {
- push(@left, $part);
- } else {
- push(@right, $part);
- $state = right;
- }
- }
- }
- my $depth = @patterns;
- if ($depth == 0) {
- die One '*' is needed in glob: '$glob'\n;
- }
- my $left = join('/', @left);
- my $right = join('/', @right);
- $re = join('/', @patterns);
- $re = join('\/',
- grep(length, quotemeta($left), ($re), quotemeta($right)));
- my $left_re = qr/^\/\Q$left\E(\/|$)/;
- bless { left = $left, right = $right, left_regex = $left_re,
- regex = qr/$re/, glob = $glob, depth = $depth }, $class;
-}
-
-sub full_path {
- my ($self, $path) = @_;
- return (length $self-{left} ? $self-{left}/ : '') .
- $path . (length $self-{right} ? /$self-{right} : '');
-}
-
__END__
Data structures:
diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm
index 2e0d7f0..b8b3474 100644
--- a/perl/Git/SVN.pm
+++ b/perl/Git/SVN.pm
@@ -207,6 +207,8 @@ sub read_all_remotes {
. must start with 'refs/'\n)
unless $remote_ref =~ m{^refs/};
$local_ref = uri_decode($local_ref);
+
+ require Git::SVN::GlobSpec;
my $rs = {
t = $t,
remote = $remote,
diff --git a/perl/Git/SVN/GlobSpec.pm b/perl/Git/SVN/GlobSpec.pm
new file mode 100644
index 000..96cfd98
--- /dev/null
+++ b/perl/Git/SVN/GlobSpec.pm
@@ -0,0 +1,59 @@
+package Git::SVN::GlobSpec;
+use strict;
+use warnings;
+
+sub new {
+ my ($class, $glob, $pattern_ok) = @_;
+ my $re = $glob;
+ $re =~ s!/+$!!g; # no need for trailing slashes
+ my (@left, @right, @patterns);
+ my $state = left;
+ my $die_msg = Only one set of wildcard directories .
+ (e.g. '*' or '*/*/*') is supported: '$glob'\n;
+ for my $part (split(m|/|, $glob)) {
+ if ($part =~ /\*/ $part ne *) {
+ die Invalid pattern in '$glob': $part\n;
+ } elsif ($pattern_ok $part =~ /[{}]/
+$part !~ /^\{[^{}]+\}/) {
+ die Invalid pattern in '$glob': $part\n;
+ }
+ if ($part eq *) {
+ die $die_msg if $state eq right;
+ $state = pattern;
+ push(@patterns, [^/]*);
+ } elsif ($pattern_ok $part =~ /^\{(.*)\}$/) {
+ die $die_msg if $state eq right;
+ $state = pattern;
+ my $p = quotemeta($1);
+ $p =~ s/\\,/|/g;
+ push(@patterns, (?:$p));
+ } else {
+ if ($state eq left) {
+
[PATCH 4/8] Extract Git::SVN::Migration from git-svn.
From: Michael G. Schwern schw...@pobox.com
Straight cut paste.
---
git-svn.perl | 258 +-
perl/Git/SVN/Migration.pm | 258 ++
perl/Makefile | 1 +
t/Git-SVN/00compile.t | 3 +-
4 files changed, 262 insertions(+), 258 deletions(-)
create mode 100644 perl/Git/SVN/Migration.pm
diff --git a/git-svn.perl b/git-svn.perl
index db60984..3741e2e 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -12,6 +12,7 @@ $VERSION = '@@GIT_VERSION@@';
use Git::SVN;
use Git::SVN::Log;
+use Git::SVN::Migration;
use Git::SVN::Utils qw(fatal can_compress);
use Git qw(
@@ -2042,263 +2043,6 @@ sub gc_directory {
}
-package Git::SVN::Migration;
-# these version numbers do NOT correspond to actual version numbers
-# of git nor git-svn. They are just relative.
-#
-# v0 layout: .git/$id/info/url, refs/heads/$id-HEAD
-#
-# v1 layout: .git/$id/info/url, refs/remotes/$id
-#
-# v2 layout: .git/svn/$id/info/url, refs/remotes/$id
-#
-# v3 layout: .git/svn/$id, refs/remotes/$id
-#- info/url may remain for backwards compatibility
-#- this is what we migrate up to this layout automatically,
-#- this will be used by git svn init on single branches
-# v3.1 layout (auto migrated):
-#- .rev_db = .rev_db.$UUID, .rev_db will remain as a symlink
-# for backwards compatibility
-#
-# v4 layout: .git/svn/$repo_id/$id, refs/remotes/$repo_id/$id
-#- this is only created for newly multi-init-ed
-# repositories. Similar in spirit to the
-# --use-separate-remotes option in git-clone (now default)
-#- we do not automatically migrate to this (following
-# the example set by core git)
-#
-# v5 layout: .rev_db.$UUID = .rev_map.$UUID
-#- newer, more-efficient format that uses 24-bytes per record
-# with no filler space.
-#- use xxd -c24 .rev_map.$UUID to view and debug
-#- This is a one-way migration, repositories updated to the
-# new format will not be able to use old git-svn without
-# rebuilding the .rev_db. Rebuilding the rev_db is not
-# possible if noMetadata or useSvmProps are set; but should
-# be no problem for users that use the (sensible) defaults.
-use strict;
-use warnings;
-use Carp qw/croak/;
-use File::Path qw/mkpath/;
-use File::Basename qw/dirname basename/;
-
-our $_minimize;
-use Git qw(
- command
- command_noisy
- command_output_pipe
- command_close_pipe
-);
-
-sub migrate_from_v0 {
- my $git_dir = $ENV{GIT_DIR};
- return undef unless -d $git_dir;
- my ($fh, $ctx) = command_output_pipe(qw/rev-parse --symbolic --all/);
- my $migrated = 0;
- while ($fh) {
- chomp;
- my ($id, $orig_ref) = ($_, $_);
- next unless $id =~ s#^refs/heads/(.+)-HEAD$#$1#;
- next unless -f $git_dir/$id/info/url;
- my $new_ref = refs/remotes/$id;
- if (::verify_ref($new_ref^0)) {
- print STDERR W: $orig_ref is probably an old ,
-branch used by an ancient version of ,
-git-svn.\n,
-However, $new_ref also exists.\n,
-We will not be able ,
-to use this branch until this ,
-ambiguity is resolved.\n;
- next;
- }
- print STDERR Migrating from v0 layout...\n if !$migrated;
- print STDERR Renaming ref: $orig_ref = $new_ref\n;
- command_noisy('update-ref', $new_ref, $orig_ref);
- command_noisy('update-ref', '-d', $orig_ref, $orig_ref);
- $migrated++;
- }
- command_close_pipe($fh, $ctx);
- print STDERR Done migrating from v0 layout...\n if $migrated;
- $migrated;
-}
-
-sub migrate_from_v1 {
- my $git_dir = $ENV{GIT_DIR};
- my $migrated = 0;
- return $migrated unless -d $git_dir;
- my $svn_dir = $git_dir/svn;
-
- # just in case somebody used 'svn' as their $id at some point...
- return $migrated if -d $svn_dir ! -f $svn_dir/info/url;
-
- print STDERR Migrating from a git-svn v1 layout...\n;
- mkpath([$svn_dir]);
- print STDERR Data from a previous version of git-svn exists, but\n\t,
-$svn_dir\n\t(required for this version ,
-($::VERSION) of git-svn) does not exist.\n;
- my ($fh, $ctx) = command_output_pipe(qw/rev-parse --symbolic --all/);
- while ($fh) {
- my $x = $_;
- next unless $x =~ s#^refs/remotes/##;
- chomp $x;
- next unless -f $git_dir/$x/info/url;
-
[PATCH 5/8] Load all the modules in one place and before running code.
From: Michael G. Schwern schw...@pobox.com
Just makes the code easier to follow. No functional change.
Also eliminate an unused lexical $SVN.
---
git-svn.perl | 44 +---
1 file changed, 21 insertions(+), 23 deletions(-)
diff --git a/git-svn.perl b/git-svn.perl
index 3741e2e..fc49ad6 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -10,11 +10,26 @@ use vars qw/$AUTHOR $VERSION
$AUTHOR = 'Eric Wong normalper...@yhbt.net';
$VERSION = '@@GIT_VERSION@@';
+use Carp qw/croak/;
+use Digest::MD5;
+use IO::File qw//;
+use File::Basename qw/dirname basename/;
+use File::Path qw/mkpath/;
+use File::Spec;
+use File::Find;
+use Getopt::Long qw/:config gnu_getopt no_ignore_case auto_abbrev/;
+use IPC::Open3;
+use Memoize;
+
use Git::SVN;
+use Git::SVN::Editor;
+use Git::SVN::Fetcher;
+use Git::SVN::Ra;
+use Git::SVN::Prompt;
use Git::SVN::Log;
use Git::SVN::Migration;
-use Git::SVN::Utils qw(fatal can_compress);
+use Git::SVN::Utils qw(fatal can_compress);
use Git qw(
git_cmd_try
command
@@ -26,6 +41,11 @@ use Git qw(
command_close_bidi_pipe
);
+BEGIN {
+ Memoize::memoize 'Git::config';
+ Memoize::memoize 'Git::config_bool';
+}
+
# From which subdir have we been invoked?
my $cmd_dir_prefix = eval {
@@ -79,28 +99,6 @@ sub _req_svn {
}
}
-use Carp qw/croak/;
-use Digest::MD5;
-use IO::File qw//;
-use File::Basename qw/dirname basename/;
-use File::Path qw/mkpath/;
-use File::Spec;
-use File::Find;
-use Getopt::Long qw/:config gnu_getopt no_ignore_case auto_abbrev/;
-use IPC::Open3;
-use Git::SVN::Editor qw//;
-use Git::SVN::Fetcher qw//;
-use Git::SVN::Ra qw//;
-use Git::SVN::Prompt qw//;
-use Memoize; # core since 5.8.0, Jul 2002
-
-BEGIN {
- Memoize::memoize 'Git::config';
- Memoize::memoize 'Git::config_bool';
-}
-
-my ($SVN);
-
$sha1 = qr/[a-f\d]{40}/;
$sha1_short = qr/[a-f\d]{4,40}/;
my ($_stdin, $_help, $_edit,
--
1.7.11.1
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] builtin-commit.c: Not add duplicate S-o-b when commit
2012/7/27 Junio C Hamano gits...@pobox.com: Signed-off-by: A Signed-off-by: B Reported-by: C So I guess duplicate S-o-b is not intentional. I think the two commands are doing randomly different things on garbage input. The order in the input (i.e. your original) does not make sense. C is not the person who handled the patch the last. I am clear, thanks. Please reword the commit 3c58c1a in branch jx/i18n-1.7.11. Change the tags from: Signed-off-by: Jiang Xin worldhello@gmail.com Signed-off-by: Ævar Arnfjörð Bjarmason ava...@gmail.com Reported-by: Vincent van Ravesteijn v...@lyx.org Reviewed-by: Stefano Lattarini stefano.lattar...@gmail.com Signed-off-by: Junio C Hamano gits...@pobox.com to Reported-by: Vincent van Ravesteijn v...@lyx.org Signed-off-by: Jiang Xin worldhello@gmail.com Signed-off-by: Ævar Arnfjörð Bjarmason ava...@gmail.com Reviewed-by: Stefano Lattarini stefano.lattar...@gmail.com Signed-off-by: Junio C Hamano gits...@pobox.com -- Jiang Xin -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/4] Prepare Git::SVN for extraction into its own file.
Michael G. Schwern schw...@pobox.com writes:
From: Michael G. Schwern schw...@pobox.com
This means it should be able to load without git-svn being loaded.
* Load Git.pm on its own and all the needed command functions.
* It needs to grab at a git-svn lexical $_prefix representing the --prefix
option. Provide opt_prefix() for that. This is a refactoring artifact.
The prefix should really be passed into Git::SVN-new.
I agree that the prefix is part of SVN-new arguments in the final
state after applying the whole series (not just these four but also
with the follow-up patches).
* Unqualify unnecessarily fully qualified globals like
$Git::SVN::default_repo_id.
* Lexically isolate the class just to make sure nothing is leaking out.
---
Forgot to sign-off, or are you still unsure about this step?
diff --git a/git-svn.perl b/git-svn.perl
index 79fe4a4..9cdf6fc 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -109,6 +109,10 @@ my ($_stdin, $_help, $_edit,
$_merge, $_strategy, $_preserve_merges, $_dry_run, $_local,
$_prefix, $_no_checkout, $_url, $_verbose,
$_git_format, $_commit_url, $_tag, $_merge_info, $_interactive);
+
+# This is a refactoring artifact so Git::SVN can get at this git-svn switch.
+sub opt_prefix { return $_prefix || '' }
+
$Git::SVN::_follow_parent = 1;
$Git::SVN::Fetcher::_placeholder_filename = .gitignore;
$_q ||= 0;
@@ -4280,12 +4292,13 @@ sub find_rev_after {
sub _new {
my ($class, $repo_id, $ref_id, $path) = @_;
unless (defined $repo_id length $repo_id) {
- $repo_id = $Git::SVN::default_repo_id;
+ $repo_id = $default_repo_id;
}
unless (defined $ref_id length $ref_id) {
- $_prefix = '' unless defined($_prefix);
+ # Access the prefix option from the git-svn main program if
it's loaded.
+ my $prefix = defined ::opt_prefix ? ::opt_prefix() : ;
Again, I agree with you that passing $prefix as one of the arguments
to -new is the right thing to do in the final state after applying
the whole series. I don't know if later steps in your patch series
will do so, but it _might_ make more sense to update -new and its
callers to do so without doing anything else first, so that you do
not have to call out to the ::opt_prefix() when you split things
out.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] Extract some utilities from git-svn to allow extracting Git::SVN.
Michael G. Schwern schw...@pobox.com writes:
From: Michael G. Schwern schw...@pobox.com
Put them in a new module called Git::SVN::Utils. Yeah, not terribly
original and it will be a dumping ground. But its better than having
them in the main git-svn program. At least they can be documented
and tested.
* fatal() is used by many classes.
* Change the $can_compress lexical into a function.
This should be enough to extract Git::SVN.
Signed-off-by: Michael G. Schwern schw...@pobox.com
---
Looks good.
diff --git a/perl/Git/SVN/Utils.pm b/perl/Git/SVN/Utils.pm
new file mode 100644
index 000..3d0bfa4
--- /dev/null
+++ b/perl/Git/SVN/Utils.pm
@@ -0,0 +1,59 @@
...
+=head1 FUNCTIONS
+
+All functions can be imported only on request.
+
+=head3 fatal
+
+fatal(@message);
+
+Display a message and exit with a fatal error code.
+
+=cut
+
+# Note: not certain why this is in use instead of die. Probably because
+# the exit code of die is 255? Doesn't appear to be used consistently.
+sub fatal (@) { print STDERR @_\n; exit 1 }
Very true. Also I do not think the line-noise prototype buys us
anything (other than making the code look mysterious to non Perl
programmers); we are not emulating any Perl's builtin with this
function, and I do not see a reason why we want to force list
context to its arguments, either. But removal of it is not part of
this step anyway, so I wouldn't complain.
+=head3 can_compress
+
+my $can_compress = can_compress;
+
+Returns true if Compress::Zlib is available, false otherwise.
+
+=cut
+
+my $can_compress;
+sub can_compress {
+return $can_compress if defined $can_compress;
+
+return $can_compress = eval { require Compress::Zlib; } ? 1 : 0;
+}
The original said eval { require Compress::Zlib; 1; }; presumably,
when require does succeed, the value inside is the 1; that has to
be at the end of Compress::Zlib, so the difference should not matter.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/4] Move initialization of Git::SVN variables into Git::SVN.
Michael G. Schwern schw...@pobox.com writes:
From: Michael G. Schwern schw...@pobox.com
Also it can compile on its own now, yay!
Hmmm.
If you swap the order of steps 3/4 and 4/4 by creating Git/SVN.pm
that only has these variable definitions (i.e. our $X and use
vars $X) and make git-svn.perl use them from Git::SVN in the first
step, and then do the bulk-moving (equivalent of your 3/4) in the
second step, would it free you from having to say it's doubtful it
will compile by itself?
In short:
- I didn't see anything questionable in 1/4;
- Calling up ::opt_prefix() from module in 2/4 looked ugly to me
but I suspect it should be easy to fix;
- 3/4 was a straight move and I didn't see anything questionable in
it, but I think it would be nicer if intermediate steps can be
made to still work by making 4/4 come first or something
similarly simple.
If the issues in 2/4 and 3/4 are easily fixable by going the route I
handwaved above, the result of doing so based on this round is ready
to be applied, I think.
Eric, Jonathan, what do you think?
---
git-svn.perl | 4
perl/Git/SVN.pm | 9 +++--
t/Git-SVN/00compile.t | 3 ++-
3 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/git-svn.perl b/git-svn.perl
index 4c77f69..ef10f6f 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -20,10 +20,7 @@ my $cmd_dir_prefix = eval {
my $git_dir_user_set = 1 if defined $ENV{GIT_DIR};
$ENV{GIT_DIR} ||= '.git';
-$Git::SVN::default_repo_id = 'svn';
-$Git::SVN::default_ref_id = $ENV{GIT_SVN_ID} || 'git-svn';
$Git::SVN::Ra::_log_window_size = 100;
-$Git::SVN::_minimize_url = 'unset';
if (! exists $ENV{SVN_SSH} exists $ENV{GIT_SSH}) {
$ENV{SVN_SSH} = $ENV{GIT_SSH};
@@ -114,7 +111,6 @@ my ($_stdin, $_help, $_edit,
# This is a refactoring artifact so Git::SVN can get at this git-svn switch.
sub opt_prefix { return $_prefix || '' }
-$Git::SVN::_follow_parent = 1;
$Git::SVN::Fetcher::_placeholder_filename = .gitignore;
$_q ||= 0;
my %remote_opts = ( 'username=s' = \$Git::SVN::Prompt::_username,
diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm
index c71c041..2e0d7f0 100644
--- a/perl/Git/SVN.pm
+++ b/perl/Git/SVN.pm
@@ -3,9 +3,9 @@ use strict;
use warnings;
use Fcntl qw/:DEFAULT :seek/;
use constant rev_map_fmt = 'NH40';
-use vars qw/$default_repo_id $default_ref_id $_no_metadata $_follow_parent
+use vars qw/$_no_metadata
$_repack $_repack_flags $_use_svm_props $_head
-$_use_svnsync_props $no_reuse_existing $_minimize_url
+$_use_svnsync_props $no_reuse_existing
$_use_log_author $_add_author_from $_localtime/;
use Carp qw/croak/;
use File::Path qw/mkpath/;
@@ -30,6 +30,11 @@ BEGIN {
$can_use_yaml = eval { require Git::SVN::Memoize::YAML; 1};
}
+our $_follow_parent = 1;
+our $_minimize_url = 'unset';
+our $default_repo_id = 'svn';
+our $default_ref_id = $ENV{GIT_SVN_ID} || 'git-svn';
+
my ($_gc_nr, $_gc_period);
# properties that we do not log:
diff --git a/t/Git-SVN/00compile.t b/t/Git-SVN/00compile.t
index a7aa85a..97475d9 100644
--- a/t/Git-SVN/00compile.t
+++ b/t/Git-SVN/00compile.t
@@ -3,6 +3,7 @@
use strict;
use warnings;
-use Test::More tests = 1;
+use Test::More tests = 2;
require_ok 'Git::SVN::Utils';
+require_ok 'Git::SVN';
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/4] Prepare Git::SVN for extraction into its own file.
Junio C Hamano gits...@pobox.com writes: Michael G. Schwern schw...@pobox.com writes: From: Michael G. Schwern schw...@pobox.com This means it should be able to load without git-svn being loaded. * Load Git.pm on its own and all the needed command functions. * It needs to grab at a git-svn lexical $_prefix representing the --prefix option. Provide opt_prefix() for that. This is a refactoring artifact. The prefix should really be passed into Git::SVN-new. I agree that the prefix is part of SVN-new arguments in the final s/is/should be/; sorry for the noise. state after applying the whole series (not just these four but also with the follow-up patches). ... Again, I agree with you that passing $prefix as one of the arguments to -new is the right thing to do in the final state after applying the whole series. I don't know if later steps in your patch series will do so, but it _might_ make more sense to update -new and its callers to do so without doing anything else first, so that you do not have to call out to the ::opt_prefix() when you split things out. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/4] Move initialization of Git::SVN variables into Git::SVN.
Junio C Hamano wrote: Michael G. Schwern schw...@pobox.com writes: Also it can compile on its own now, yay! Hmmm. I agree with Michael's yay and also think it's fine that after patch 3 it isn't there yet. That's because git-svn.perl doesn't use Git::SVN on its own but helps it out a little. So even if we only applied patches 1-3, git-svn would still work (maybe it's worth testing perl -MGit::SVN by hand to avoid the it's doubtful about whether Git::SVN is self-contained and replace it with a more certain statement?), and patch 4 just makes it even better. [...] In short: - I didn't see anything questionable in 1/4; - Calling up ::opt_prefix() from module in 2/4 looked ugly to me but I suspect it should be easy to fix; - 3/4 was a straight move and I didn't see anything questionable in it, but I think it would be nicer if intermediate steps can be made to still work by making 4/4 come first or something similarly simple. If the issues in 2/4 and 3/4 are easily fixable by going the route I handwaved above, the result of doing so based on this round is ready to be applied, I think. Eric, Jonathan, what do you think? I think this is pretty good already, though I also like your suggestion re 2/4. I haven't reviewed the tests these introduce and assume Eric has that covered. Thanks, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
