* Re: [PATCH] tests: turn on test-lint-shell-syntax by default
From: Junio C Hamano @ 2013-02-05 20:52 UTC (permalink / raw)
To: Torsten Bögershausen; +Cc: Jonathan Nieder, git, kraai
In-Reply-To: <51116D3E.3070409@web.de>
Torsten Bögershausen <tboegi@web.de> writes:
> Thanks for the detailed suggestion.
> Instead of using a file for putting out non portable syntax,
> can we can use a similar logic as test_failure ?
Your test_bad_syntax_ function can be called from a subshell, and
its "exit 1" will not exit, no?
test_expect_success 'prepare a blob with incomplete line' '
(
echo first line
echo second line
echo -n final and incomplete line
) >incomplete.txt
'
^ permalink raw reply
* Re: [PATCH] t4038: add tests for "diff --cc --raw <trees>"
From: Junio C Hamano @ 2013-02-05 20:48 UTC (permalink / raw)
To: John Keeping; +Cc: git, Antoine Pelisse
In-Reply-To: <20130205202558.GX1342@serenity.lan>
John Keeping <john@keeping.me.uk> writes:
> Signed-off-by: John Keeping <john@keeping.me.uk>
> ---
> ...
> diff --git a/t/t4038-diff-combined.sh b/t/t4038-diff-combined.sh
> index 40277c7..a0701bc 100755
> --- a/t/t4038-diff-combined.sh
> +++ b/t/t4038-diff-combined.sh
> @@ -89,4 +89,33 @@ test_expect_success 'diagnose truncated file' '
> grep "diff --cc file" out
> '
>
> +test_expect_success 'setup for --cc --raw' '
> + blob=$(echo file |git hash-object --stdin -w) &&
> + base_tree=$(echo "100644 blob $blob file" | git mktree) &&
> + trees= &&
> + for i in `test_seq 1 40`
> + do
> + blob=$(echo file$i |git hash-object --stdin -w) &&
> + trees="$trees $(echo "100644 blob $blob file" |git mktree)"
Please have a SP after each of these '|' pipes.
If you collect trees this way:
trees="$trees$(echo ... | git mktree)$LF"
then ...
> + done
> +'
> +
> +test_expect_success 'check --cc --raw with four trees' '
> + four_trees=$(echo "$trees" |awk -e "{
> + print \$1
> + print \$2
> + print \$3
> + print \$4
> + }") &&
(What's "awk -e"?)
... you can do
echo "$trees" | sed -e 4q
which is less repetitive.
Thanks.
^ permalink raw reply
* Re: [PATCHv4] Add contrib/credentials/netrc with GPG support
From: Ted Zlatanov @ 2013-02-05 20:47 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git
In-Reply-To: <7vhalqsfkf.fsf@alter.siamese.dyndns.org>
On Tue, 05 Feb 2013 11:53:20 -0800 Junio C Hamano <gitster@pobox.com> wrote:
JCH> Ted Zlatanov <tzz@lifelogs.com> writes:
>> Changes since PATCHv3:
>>
>> - simple tests in Makefile
>> - support multiple files, code refactored
>> - documentation and comments updated
>> - fix IO::File for GPG pipe
>> - exit peacefully in almost every situation, die on bad invocation or query
>> - use log_verbose() and -v for logging for the user
>> - use log_debug() and -d for logging for the developer
>> - use Net::Netrc parser and `man netrc' to improve parsing
>> - ignore 'default' and 'macdef' netrc entries
>> - require 'machine' token in netrc lines
>> - ignore netrc files with bad permissions or owner (from Net::Netrc)
JCH> Please place the above _after_ the three-dashes.
JCH> The space here, above "---", is to justify why this change is a good
JCH> idea to people who see this patch for the first time who never saw
JCH> the earlier rounds of this patch, e.g. reading "git log" output 6
JCH> months down the road (see Documentation/SubmittingPatches "(2)
JCH> Describe your changes well").
Will do in PATCHv5.
JCH> Avoid starting an argument to "echo" with a dash; some
JCH> implementations choke with "unknown option".
Nice, thanks. It's purely decorative so I use '=>' now.
>> +my %options = (
>> + help => 0,
>> + debug => 0,
>> + verbose => 0,
>> + file => [],
JCH> Looks like there is some funny indentation going on here.
Fixed.
>> + -f|--file AUTHFILE : specify netrc-style files. Files with the .gpg extension
>> + will be decrypted by GPG before parsing. Multiple -f
>> + arguments are OK, and the order is respected.
JCH> Saying "order is respected" without mentioning the collision
JCH> resolution rules is not helpful to the users when deciding in what
JCH> order they should give these files. First one wins, or last one
JCH> wins? Later you say "looks for the first entry", but it will be
JCH> much easier to read the above to mention it here as well.
Right. Reworded.
>> +Thus, when we get this query on STDIN:
>> +
>> +protocol=https
>> +username=tzz
>> +
>> +this credential helper will look for the first entry in every AUTHFILE that
>> +matches
>> +
>> +port https login tzz
>> +
>> +OR
>> +
>> +protocol https login tzz
>> +
>> +OR... etc. acceptable tokens as listed above. Any unknown tokens are
>> +simply ignored.
>> +
>> +Then, the helper will print out whatever tokens it got from the entry, including
>> +"password" tokens, mapping back to Git's helper protocol; e.g. "port" is mapped
>> +back to "protocol".
JCH> Isn't "hostname" typically what users expect to see? It is somewhat
JCH> unnerving to see an example that throws the same password back to
JCH> any host you happen to have an accoutn "tzz" on, even though that is
JCH> not technically an invalid way to use this helper.
Yeah, I changed it to show "machine" in the query (which would be more typical).
>> + if ($stat[2] & 077) {
>> + log_verbose("Insecure $file (mode=%04o); skipping it",
>> + $stat[2] & 07777);
JCH> Nice touch, although I am not sure rejecting world or group readable
JCH> encrypted file is absolutely necessary.
Right. Fixed.
>> + if ($stat[4] != $<) {
>> + log_verbose("Not owner of $file; skipping it");
>> + next FILE;
JCH> OK. A group of local users may share the same account at the
JCH> remote, but that would be unusual.
I added --insecure/-k to override this check.
>> + if ($mode & 077)
JCH> Again? Didn't you just do this?
Damn, sorry.
JCH> I think the prevalent style is to
JCH> if (condition) {
JCH> do this;
JCH> } elsif (another condition) {
JCH> do that
JCH> } else {
JCH> do that other thing;
JCH> }
JCH> (this comment applies to all if/elsif/else cascades in this patch).
Yup. I was working with Net::Netrc code and forgot to reformat it. Sorry.
>> +
>> + next FILE;
JCH> Isn't this outermost loop, by the way? What the motivation to have
JCH> an explicit label everywhere (not complaining---it could be your own
JCH> discipline thing---just wondering).
I think it's more readable with large loops, and it actually makes sense
when you read the code. Not a big deal to me either, I just felt for
this particular script it was OK.
>> + if ($file =~ m/\.gpg$/) {
>> + log_verbose("Using GPG to open $file");
>> + # GPG doesn't work well with 2- or 3-argument open
JCH> If that is the case, please quote $file properly against shell
JCH> munging it.
Ahhh that gets ugly. OK, quoted.
JCH> The only thing you do on $io is to read from it via "while (<$io>)",
JCH> so I would personally have written this part like this without
JCH> having to use IO::File(), though:
JCH> $io = open("-|", qw(gpg --decrypt), $file);
That doesn't work for me, unfortunately. I'm trying to avoid the IPC::*
modules and such. Please test it yourself with GPG. I'm on Perl
5.14.2.
I think it's OK with the quoting, as you'll see in PATCHv5.
JCH> Similarly for the plain file:
JCH> $io = open("<", $file);
You mean "open $io, '<', $file". open() returns nonzero on success and
undef on failure. OK, I'll use open() instead of IO::File.
>> + my ($mach, $macdef, $tok, @tok) = (0, 0);
JCH> I think you meant to use $mach as a reference to a hash and $macdef
JCH> as a reference to an array; do you want to initialize them to
JCH> numeric zeros?
That actually came from Net::Netrc. The $mach default is OK either way;
I left it undefined so it's clearer. I think the $macdef initial value
is a bug (which, I guess, shows macros are very rare); I just made it a
boolean for our purposes.
Ted
^ permalink raw reply
* Re: [PATCH] tests: turn on test-lint-shell-syntax by default
From: Torsten Bögershausen @ 2013-02-05 20:36 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Torsten Bögershausen, Jonathan Nieder, git, kraai
In-Reply-To: <7v4ni2y1fm.fsf@alter.siamese.dyndns.org>
On 27.01.13 18:34, Junio C Hamano wrote:
> Torsten Bögershausen <tboegi@web.de> writes:
>
>> Back to the which:
>> ...
>> and running "make test" gives the following, at least in my system:
>> ...
> I think everybody involved in this discussion already knows that;
> the point is that it can easily give false negative, without the
> scripts working very hard to do so.
>
> If we did not care about incurring runtime performance cost, we
> could arrange:
>
> - the test framework to define a variable $TEST_ABORT that has a
> full path to a file that is in somewhere test authors cannot
> touch unless they really try hard to (i.e. preferrably outside
> $TRASH_DIRECTORY, as it is not uncommon for to tests to do "rm *"
> there). This location should be per $(basename "$0" .sh) to allow
> running multiple tests in paralell;
>
> - the test framework to "rm -f $TEST_ABORT" at the beginning of
> test_expect_success/failure;
>
> - test_expect_success/failure to check $TEST_ABORT and if it
> exists, abort the execution, showing the contents of the file as
> an error message.
>
> Then you can wrap commands whose use we want to limit, perhaps like
> this, in the test framework:
>
> which () {
> cat >"$TEST_ABORT" <<-\EOF
> Do not use unportable 'which' in the test script.
> "if type $cmd" is a good way to see if $cmd exists.
> EOF
> }
>
> sed () {
> saw_wantarg= must_abort=
> for arg
> do
> if test -n "$saw_wantarg"
> then
> saw_wantarg=
> continue
> fi
> case "$arg" in
> --) break ;; # end of options
> -i) echo >"$TEST_ABORT" "Do not use 'sed -i'"
> must_abort=yes
> break ;;
> -e) saw_wantarg=yes ;; # skip next arg
> -*) continue ;; # options without arg
> *) break ;; # filename
> esac
> done
> if test -z "$must_abort"
> sed "$@"
> fi
> }
>
> Then you can check that TEST_ABORT does not appear in test scripts
> (ensuring that they do not attempt to circumvent the mechanis) and
> catch use of unwanted commands or unwanted extended features of
> commands at runtime.
>
> But this will incur runtime performace hit, so I am not sure it
> would be worth it.
Thanks for the detailed suggestion.
Instead of using a file for putting out non portable syntax,
can we can use a similar logic as test_failure ?
I did some benchmarking, the test suite on a Laptop is 37..38 minutes,
including make clean && make both on next, pu, master or with the patch below.
I couldn't find a measurable impact on the execution time.
What do we think about a patch like this
(Not sure if this cut-and-paste data applies, it's for review)
[PATCH] test-lib: which, echo -n and sed -i are not portable
The posix version of sed command supports options -n -e -f
The gnu extension -i to edit a file in place is not
available on all systems.
To catch the usage of non-posix options with sed a
wrapper function is added in test-lib.sh.
The wrapper checks that only -n -e -f are used.
The short form "-ne" for "-n -e" is allowed as well.
echo -n is not portable and not available on all systems,
printf can be used instead.
Add a wrapper to catch echo -n
which is not portable, the output differs between different
implementations, and the return code may not be reliable.
Add a function test_bad_syntax_ in test-lib.sh, which increments
the variable test_bad_syntax and works similar to test_failure_
Signed-off-by: Torsten Bögershausen <tboegi@web.de>
---
t/test-lib.sh | 46 ++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 46 insertions(+)
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 1a6c4ab..248ed34 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -266,6 +266,7 @@ else
exec 4>/dev/null 3>/dev/null
fi
+test_bad_syntax=0
test_failure=0
test_count=0
test_fixed=0
@@ -300,6 +301,12 @@ test_ok_ () {
say_color "" "ok $test_count - $@"
}
+test_bad_syntax_ () {
+ test_bad_syntax=$(($test_bad_syntax + 1))
+ say_color error "$@"
+ test "$immediate" = "" || { GIT_EXIT_OK=t; exit 1; }
+}
+
test_failure_ () {
test_failure=$(($test_failure + 1))
say_color error "not ok $test_count - $1"
@@ -402,10 +409,15 @@ test_done () {
fixed $test_fixed
broken $test_broken
failed $test_failure
+ bad_syntax $test_bad_syntax
EOF
fi
+ if test "$test_bad_syntax" != 0
+ then
+ say_color error "# $test_bad_syntax non portable shell syntax"
+ fi
if test "$test_fixed" != 0
then
say_color error "# $test_fixed known breakage(s) vanished; please update test(s)"
@@ -645,6 +657,40 @@ yes () {
done
}
+
+# which is not portable
+which () {
+ test_bad_syntax_ "Do not use unportable 'which' in the test script." \
+ "'if type $1' is a good way to see if '$1' exists."
+ return 1
+}
+
+# catch non-portable usage of sed
+sed () {
+ for arg
+ do
+ case "$arg" in
+ -[efn]) continue ;; # allowed posix options
+ -ne) continue ;; # tolerated
+ -*)test_bad_syntax_ "Do not use 'sed "$arg"'. Valid options for 'sed' are -n -e -f"
+ return 1
+ ;;
+ *) continue ;;
+ esac
+ done
+ command sed "$@"
+}
+
+# catch non portable echo -n
+echo () {
+ if test "$1" = -n
+ then
+ test_bad_syntax_ "Do not use 'echo -n'. Use printf instead"
+ else
+ command echo "$@"
+ fi
+}
+
# Fix some commands on Windows
case $(uname -s) in
*MINGW*)
--
1.8.1.1
^ permalink raw reply related
* [PATCH] t4038: add tests for "diff --cc --raw <trees>"
From: John Keeping @ 2013-02-05 20:25 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Antoine Pelisse
In-Reply-To: <7vip696i3v.fsf@alter.siamese.dyndns.org>
Signed-off-by: John Keeping <john@keeping.me.uk>
---
On Sun, Feb 03, 2013 at 04:24:52PM -0800, Junio C Hamano wrote:
> Ideally it should also have test cases
> to show "git diff --cc --raw blob1 blob2...blob$n" for n=4 and n=40
> (or any two values clearly below and above the old hardcoded limit)
> behave sensibly, exposing the old breakage, which I'll leave as a
> LHF (low-hanging-fruit). Hint, hint...
Hint taken ;-)
git-diff uses a different code path for blobs, so I've had to use trees
to trigger this. The last test fails without
jc/combine-diff-many-parents and passes with it.
t/t4038-diff-combined.sh | 29 +++++++++++++++++++++++++++++
1 file changed, 29 insertions(+)
diff --git a/t/t4038-diff-combined.sh b/t/t4038-diff-combined.sh
index 40277c7..a0701bc 100755
--- a/t/t4038-diff-combined.sh
+++ b/t/t4038-diff-combined.sh
@@ -89,4 +89,33 @@ test_expect_success 'diagnose truncated file' '
grep "diff --cc file" out
'
+test_expect_success 'setup for --cc --raw' '
+ blob=$(echo file |git hash-object --stdin -w) &&
+ base_tree=$(echo "100644 blob $blob file" | git mktree) &&
+ trees= &&
+ for i in `test_seq 1 40`
+ do
+ blob=$(echo file$i |git hash-object --stdin -w) &&
+ trees="$trees $(echo "100644 blob $blob file" |git mktree)"
+ done
+'
+
+test_expect_success 'check --cc --raw with four trees' '
+ four_trees=$(echo "$trees" |awk -e "{
+ print \$1
+ print \$2
+ print \$3
+ print \$4
+ }") &&
+ git diff --cc --raw $four_trees $base_tree >out &&
+ # Check for four leading colons in the output:
+ grep "^::::[^:]" out
+'
+
+test_expect_success 'check --cc --raw with forty trees' '
+ git diff --cc --raw $trees $base_tree >out &&
+ # Check for forty leading colons in the output:
+ grep "^::::::::::::::::::::::::::::::::::::::::[^:]" out
+'
+
test_done
--
1.8.1.2
^ permalink raw reply related
* Re: [PATCH] Add contrib/credentials/netrc with GPG support, try #2
From: Junio C Hamano @ 2013-02-05 20:23 UTC (permalink / raw)
To: Ted Zlatanov; +Cc: Jeff King, git
In-Reply-To: <87bobyr0ju.fsf@lifelogs.com>
Ted Zlatanov <tzz@lifelogs.com> writes:
> JCH> You still need to parse a file that has a "default" entry correctly;
> JCH> otherwise the users won't be able to share existing .netrc files
> JCH> with other applications e.g. ftp, which is the whole point of this
> JCH> series. Not using values from the "default" entry is probably fine,
> JCH> though.
>
> OK; done in PATCHv4.
Hmph.
Didn't you remove that from your version of net_netrc_loader when
you borrowed the bulk of the code from Net::Netrc::_readrc? I see
"default" token handled at the beginning of "TOKEN: while (@tok)"
loop in the original but not in your version I see in v4.
^ permalink raw reply
* Re: [WIP/RFH/RFD/PATCH] grep: allow to use textconv filters
From: Jeff King @ 2013-02-05 20:11 UTC (permalink / raw)
To: Michael J Gruber; +Cc: Junio C Hamano, git
In-Reply-To: <5111317E.8060906@drmicha.warpmail.net>
On Tue, Feb 05, 2013 at 05:21:18PM +0100, Michael J Gruber wrote:
> Thanks Jeff, that helps a lot! It covers "grep expr" and "grep expr rev
> -- path" just fine. I'll look into "grep expr rev:path" which does not
> work yet because of an empty driver.
>
> I also have "show --textconv" covered and a suggestion for "cat-file
> --textconv" (to work without a textconv filter).
>
> Expect a mini-series soon :)
Cool, I'm glad it helped. It would be great if diff_filespec and
grep_source could grow together into a unified object. One of the gross
things about the patch I posted is that we will now sometimes read the
file/blob data via grep_source_load, and sometimes via
diff_populate_filespec. They _should_ be equivalent, but in an ideal
world, they would be the same code path.
That may be too much to tackle for your series, though (I wanted to do
it when I factored out grep_source, but backed off for the same reason).
The "grep expr rev:path" fix should look something like this:
diff --git a/builtin/grep.c b/builtin/grep.c
index 915c8ef..cdc7d32 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -820,13 +820,17 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
for (i = 0; i < argc; i++) {
const char *arg = argv[i];
unsigned char sha1[20];
+ struct object_context oc;
/* Is it a rev? */
- if (!get_sha1(arg, sha1)) {
+ if (!get_sha1_with_context(arg, sha1, &oc)) {
struct object *object = parse_object(sha1);
if (!object)
die(_("bad object %s"), arg);
if (!seen_dashdash)
verify_non_filename(prefix, arg);
+ /* oops, we need something that will remember oc.path
+ * here, so that we can pass it along to
+ * grep_source_init */
add_object_array(object, arg, &list);
continue;
}
But you'll have to replace the object_array with something more
featureful, I think.
-Peff
^ permalink raw reply related
* Re: Is anyone working on a next-gen Git protocol (Re: [PATCH v3 0/8] Hiding refs)
From: Ævar Arnfjörð Bjarmason @ 2013-02-05 20:07 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King, Shawn Pearce, Johannes Schindelin
In-Reply-To: <7vfw1avjcw.fsf@alter.siamese.dyndns.org>
On Tue, Feb 5, 2013 at 5:03 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> Do you have any plans for something that *does* have the reduction of
>> network bandwidth as a primary goal?
>
> Uncluttering gives reduction of bandwidth anyway, so I do not see
> much point in the distinction you seem to be making.
Doing this work wouldn't only give us a way to specify which refs we
want, but if done correctly would future-proof the protocol in case we
want to add any other extensions down the line in a
backwards-compatible fashion without having the server first spew all
his refs at us.
Anyway, an implementation that allows a client to say "I want X" is
simpler than an implementation where a server has to anticipate in
advance which X the clients will ask for.
>> Is this what you've been working on? Because if so I misunderstood you
>> thinking you were going to work on something that gave clients the
>> ability specify what they wanted before the initial ref advertisement.
>> ...
>> 4. http://thread.gmane.org/gmane.comp.version-control.git/207190
>
> "Who speaks first" mentioned in 4. above, was primarily about
> "delaying ref advertisement", which would be a larger protocol
> change. Nobody seems to have attacked it since it was discussed,
> and I was tired of hearing nothing but complaints and whines. This
> "hiding refs" series was done as a cheaper way to solve a related
> issue, without having to wait for the solution of "delaying
> advertisement", which is an orthogonal issue.
Oh sure. I just wanted to know if you were working on delaying ref
advertisement to avoid duplicating efforts. I had the impression you
were given your earlier E-Mail, but obviously we had a
misunderstanding.
^ permalink raw reply
* Re: [PATCH] Add contrib/credentials/netrc with GPG support, try #2
From: Ted Zlatanov @ 2013-02-05 20:03 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git
In-Reply-To: <7vip66sftf.fsf@alter.siamese.dyndns.org>
On Tue, 05 Feb 2013 11:47:56 -0800 Junio C Hamano <gitster@pobox.com> wrote:
JCH> Ted Zlatanov <tzz@lifelogs.com> writes:
JCH> Oh, another thing. 'default' is like 'machine' followed by any
JCH> machine name, so the above while loop that reads two tokens
JCH> pair-wise needs to be aware that 'default' is not followed by a
JCH> value. I think the loop will fail to parse this:
>>
JCH> default login anonymous password me@home
JCH> machine k.org login me password mysecret
>>
>> I'd prefer to ignore "default" because it should not be used for the Git
>> credential helpers (its only use case is for anonymous services AFAIK).
>> So I'll add a case to ignore it in PATCHv4, if that's OK.
JCH> You still need to parse a file that has a "default" entry correctly;
JCH> otherwise the users won't be able to share existing .netrc files
JCH> with other applications e.g. ftp, which is the whole point of this
JCH> series. Not using values from the "default" entry is probably fine,
JCH> though.
OK; done in PATCHv4.
Ted
^ permalink raw reply
* Re: [PATCHv4] Add contrib/credentials/netrc with GPG support
From: Junio C Hamano @ 2013-02-05 19:53 UTC (permalink / raw)
To: Ted Zlatanov; +Cc: Jeff King, git
In-Reply-To: <87fw1ar3og.fsf_-_@lifelogs.com>
Ted Zlatanov <tzz@lifelogs.com> writes:
> Changes since PATCHv3:
>
> - simple tests in Makefile
> - support multiple files, code refactored
> - documentation and comments updated
> - fix IO::File for GPG pipe
> - exit peacefully in almost every situation, die on bad invocation or query
> - use log_verbose() and -v for logging for the user
> - use log_debug() and -d for logging for the developer
> - use Net::Netrc parser and `man netrc' to improve parsing
> - ignore 'default' and 'macdef' netrc entries
> - require 'machine' token in netrc lines
> - ignore netrc files with bad permissions or owner (from Net::Netrc)
Please place the above _after_ the three-dashes.
The space here, above "---", is to justify why this change is a good
idea to people who see this patch for the first time who never saw
the earlier rounds of this patch, e.g. reading "git log" output 6
months down the road (see Documentation/SubmittingPatches "(2)
Describe your changes well").
>
> Signed-off-by: Ted Zlatanov <tzz@lifelogs.com>
> ---
> contrib/credential/netrc/Makefile | 10 +
> contrib/credential/netrc/git-credential-netrc | 423 +++++++++++++++++++++++++
> 2 files changed, 433 insertions(+), 0 deletions(-)
> create mode 100644 contrib/credential/netrc/Makefile
> create mode 100755 contrib/credential/netrc/git-credential-netrc
>
> diff --git a/contrib/credential/netrc/Makefile b/contrib/credential/netrc/Makefile
> new file mode 100644
> index 0000000..ee8c5f0
> --- /dev/null
> +++ b/contrib/credential/netrc/Makefile
> @@ -0,0 +1,10 @@
> +test_netrc:
> + @(echo "bad data" | ./git-credential-netrc -f A -d -v) || echo "Bad invocation test, ignoring failure"
> + @echo "-> Silent invocation... nothing should show up here with a missing file"
Avoid starting an argument to "echo" with a dash; some
implementations choke with "unknown option".
> + @echo "bad data" | ./git-credential-netrc -f A get
> + @echo "-> Back to noisy: -v and -d used below, missing file"
> + echo "bad data" | ./git-credential-netrc -f A -d -v get
> + @echo "-> Look for any entry in the default file set"
> + echo "" | ./git-credential-netrc -d -v get
> + @echo "-> Look for github.com in the default file set"
> + echo "host=google.com" | ./git-credential-netrc -d -v get
> diff --git a/contrib/credential/netrc/git-credential-netrc b/contrib/credential/netrc/git-credential-netrc
> new file mode 100755
> index 0000000..6946217
> --- /dev/null
> +++ b/contrib/credential/netrc/git-credential-netrc
> @@ -0,0 +1,423 @@
> +#!/usr/bin/perl
> +
> +use strict;
> +use warnings;
> +
> +use Getopt::Long;
> +use File::Basename;
> +
> +my $VERSION = "0.1";
> +
> +my %options = (
> + help => 0,
> + debug => 0,
> + verbose => 0,
> + file => [],
Looks like there is some funny indentation going on here.
> +
> + # identical token maps, e.g. host -> host, will be inserted later
> + tmap => {
> + port => 'protocol',
> + machine => 'host',
> + path => 'path',
> + login => 'username',
> + user => 'username',
> + password => 'password',
> + }
> + );
> +
> +# Map each credential protocol token to itself on the netrc side.
> +foreach (values %{$options{tmap}}) {
> + $options{tmap}->{$_} = $_;
> +}
> +
> +# Now, $options{tmap} has a mapping from the netrc format to the Git credential
> +# helper protocol.
> +
> +# Next, we build the reverse token map.
> +
> +# When $rmap{foo} contains 'bar', that means that what the Git credential helper
> +# protocol calls 'bar' is found as 'foo' in the netrc/authinfo file. Keys in
> +# %rmap are what we expect to read from the netrc/authinfo file.
> +
> +my %rmap;
> +foreach my $k (keys %{$options{tmap}}) {
> + push @{$rmap{$options{tmap}->{$k}}}, $k;
> +}
> +
> +Getopt::Long::Configure("bundling");
> +
> +# TODO: maybe allow the token map $options{tmap} to be configurable.
> +GetOptions(\%options,
> + "help|h",
> + "debug|d",
> + "verbose|v",
> + "file|f=s@",
> + );
> +
> +if ($options{help}) {
> + my $shortname = basename($0);
> + $shortname =~ s/git-credential-//;
> +
> + print <<EOHIPPUS;
> +
> +$0 [-f AUTHFILE1] [-f AUTHFILEN] [-d] [-v] get
> +
> +Version $VERSION by tzz\@lifelogs.com. License: BSD.
> +
> +Options:
> +
> + -f|--file AUTHFILE : specify netrc-style files. Files with the .gpg extension
> + will be decrypted by GPG before parsing. Multiple -f
> + arguments are OK, and the order is respected.
Saying "order is respected" without mentioning the collision
resolution rules is not helpful to the users when deciding in what
order they should give these files. First one wins, or last one
wins? Later you say "looks for the first entry", but it will be
much easier to read the above to mention it here as well.
> + -d|--debug : turn on debugging (developer info)
> +
> + -v|--verbose : be more verbose (show files and information found)
> +
> +To enable this credential helper:
> +
> + git config credential.helper '$shortname -f AUTHFILE1 -f AUTHFILE2'
> +
> +(Note that Git will prepend "git-credential-" to the helper name and look for it
> +in the path.)
> +
> +...and if you want lots of debugging info:
> +
> + git config credential.helper '$shortname -f AUTHFILE -d'
> +
> +...or to see the files opened and data found:
> +
> + git config credential.helper '$shortname -f AUTHFILE -v'
> +
> +Only "get" mode is supported by this credential helper. It opens every AUTHFILE
> +and looks for the first entry that matches the requested search criteria:
> +
> + 'port|protocol':
> + The protocol that will be used (e.g., https). (protocol=X)
> +
> + 'machine|host':
> + The remote hostname for a network credential. (host=X)
> +
> + 'path':
> + The path with which the credential will be used. (path=X)
> +
> + 'login|user|username':
> + The credential’s username, if we already have one. (username=X)
> +
> +Thus, when we get this query on STDIN:
> +
> +protocol=https
> +username=tzz
> +
> +this credential helper will look for the first entry in every AUTHFILE that
> +matches
> +
> +port https login tzz
> +
> +OR
> +
> +protocol https login tzz
> +
> +OR... etc. acceptable tokens as listed above. Any unknown tokens are
> +simply ignored.
> +
> +Then, the helper will print out whatever tokens it got from the entry, including
> +"password" tokens, mapping back to Git's helper protocol; e.g. "port" is mapped
> +back to "protocol".
Isn't "hostname" typically what users expect to see? It is somewhat
unnerving to see an example that throws the same password back to
any host you happen to have an accoutn "tzz" on, even though that is
not technically an invalid way to use this helper.
> +Again, note that the first matching entry from all the AUTHFILEs is used.
> +
> +Tokens can be quoted as 'STRING' or "STRING".
> +
> +No caching is performed by this credential helper.
> +
> +EOHIPPUS
Otherwise, nice write-up.
> +my $mode = shift @ARGV;
> +
> +# Credentials must get a parameter, so die if it's missing.
> +die "Syntax: $0 [-f AUTHFILE1] [-f AUTHFILEN] [-d] get" unless defined $mode;
> +
> +# Only support 'get' mode; with any other unsupported ones we just exit.
> +exit 0 unless $mode eq 'get';
> +
> +my $files = $options{file};
> +
> +# if no files were given, use a predefined list.
> +# note that .gpg files come first
> +unless (scalar @$files)
> +{
> + my @candidates = qw[
> + ~/.authinfo.gpg
> + ~/.netrc.gpg
> + ~/.authinfo
> + ~/.netrc
> + ];
> +
> + $files = $options{file} = [ map { glob $_ } @candidates ];
> +}
> +
> +my $query = read_credential_data_from_stdin();
> +
> +FILE:
> +foreach my $file (@$files)
> +{
> + unless (-r $file)
> + {
> + log_verbose("Unable to read $file; skipping it");
> + next FILE;
> + }
> +
> + # the following check is copied from Net::Netrc
> + # OS/2 and Win32 do not handle stat in a way compatable with this check :-(
> + unless ($^O eq 'os2'
> + || $^O eq 'MSWin32'
> + || $^O eq 'MacOS'
> + || $^O =~ /^cygwin/)
> + {
> + my @stat = stat($file);
> +
> + if (@stat) {
> + if ($stat[2] & 077) {
> + log_verbose("Insecure $file (mode=%04o); skipping it",
> + $stat[2] & 07777);
Nice touch, although I am not sure rejecting world or group readable
encrypted file is absolutely necessary.
> + next FILE;
> + }
> + if ($stat[4] != $<) {
> + log_verbose("Not owner of $file; skipping it");
> + next FILE;
OK. A group of local users may share the same account at the
remote, but that would be unusual.
> + }
> + }
> + }
> +
> + my $mode = (stat($file))[2];
> + if ($mode & 077)
> + {
> + log_verbose("Insecure $file (mode=%04o); skipping it",
> + $mode & 07777);
Again? Didn't you just do this?
> + next FILE;
> + }
> +
> + my @entries = load_netrc($file);
> +
> + unless (scalar @entries)
> + {
> + if ($!)
> + {
> + log_verbose("Unable to open $file: $!");
> + }
> + else
> + {
> + log_verbose("No netrc entries found in $file");
> + }
I think the prevalent style is to
if (condition) {
do this;
} elsif (another condition) {
do that
} else {
do that other thing;
}
(this comment applies to all if/elsif/else cascades in this patch).
> +
> + next FILE;
Isn't this outermost loop, by the way? What the motivation to have
an explicit label everywhere (not complaining---it could be your own
discipline thing---just wondering).
> + }
> +
> + my $entry = find_netrc_entry($query, @entries);
> + if ($entry)
> + {
> + print_credential_data($entry, $query);
> + # we're done!
> + last FILE;
> + }
> +}
> +
> +exit 0;
> +
> +sub load_netrc
> +{
> + my $file = shift @_;
> +
> + my $io;
> + if ($file =~ m/\.gpg$/) {
> + log_verbose("Using GPG to open $file");
> + # GPG doesn't work well with 2- or 3-argument open
If that is the case, please quote $file properly against shell
munging it.
The only thing you do on $io is to read from it via "while (<$io>)",
so I would personally have written this part like this without
having to use IO::File(), though:
$io = open("-|", qw(gpg --decrypt), $file);
Similarly for the plain file:
$io = open("<", $file);
> + $io = new IO::File("gpg --decrypt $file|");
> + }
> + else {
> + log_verbose("Opening $file...");
> + $io = new IO::File($file, '<');
> + }
> +
> + # nothing to do if the open failed (we log the error later)
> + return unless $io;
> +
> + # Net::Netrc does this, but the functionality is merged with the file
> + # detection logic, so we have to extract just the part we need
> + my @netrc_entries = net_netrc_loader($io);
> +
> + # these entries will use the credential helper protocol token names
> + my @entries;
> +
> + foreach my $nentry (@netrc_entries) {
> + my %entry;
> + my $num_port;
> +
> + if (defined $nentry->{port} && $nentry->{port} =~ m/^\d+$/) {
> + $num_port = $nentry->{port};
> + delete $nentry->{port};
> + }
> +
> + # create the new entry for the credential helper protocol
> + $entry{$options{tmap}->{$_}} = $nentry->{$_} foreach keys %$nentry;
> +
> + # for "host X port Y" where Y is an integer (captured by
> + # $num_port above), set the host to "X:Y"
> + if (defined $entry{host} && defined $num_port) {
> + $entry{host} = join(':', $entry{host}, $num_port);
> + }
> +
> + push @entries, \%entry;
> + }
> +
> + return @entries;
> +}
> +
> +sub net_netrc_loader
> +{
> + my $fh = shift @_;
> + my @entries;
> + my ($mach, $macdef, $tok, @tok) = (0, 0);
I think you meant to use $mach as a reference to a hash and $macdef
as a reference to an array; do you want to initialize them to
numeric zeros?
(The remainder of the patch unsnipped for others' reference).
Thanks.
> + LINE:
> + while (<$fh>) {
> + undef $macdef if /\A\n\Z/;
> +
> + if ($macdef) {
> + push(@$macdef, $_);
> + next LINE;
> + }
> +
> + s/^\s*//;
> + chomp;
> +
> + while (length && s/^("((?:[^"]+|\\.)*)"|((?:[^\\\s]+|\\.)*))\s*//) {
> + (my $tok = $+) =~ s/\\(.)/$1/g;
> + push(@tok, $tok);
> + }
> +
> + TOKEN:
> + while (@tok) {
> + $tok = shift(@tok);
> +
> + if ($tok eq "machine") {
> + my $host = shift @tok;
> + $mach = { machine => $host };
> + push @entries, $mach;
> + }
> + elsif (exists $options{tmap}->{$tok}) {
> + unless ($mach) {
> + log_debug("Skipping token $tok because no machine was given");
> + next TOKEN;
> + }
> +
> + my $value = shift @tok;
> + unless (defined $value) {
> + log_debug("Token $tok had no value, skipping it.");
> + next TOKEN;
> + }
> +
> + # Following line added by rmerrell to remove '/' escape char in .netrc
> + $value =~ s/\/\\/\\/g;
> + $mach->{$tok} = $value;
> + }
> + elsif ($tok eq "macdef") { # we ignore macros
> + next TOKEN unless $mach;
> + my $value = shift @tok;
> + $mach->{macdef} = {} unless exists $mach->{macdef};
> + $macdef = $mach->{machdef}{$value} = [];
> + }
> + }
> + }
> +
> + return @entries;
> +}
> +
> +sub read_credential_data_from_stdin
> +{
> + # the query: start with every token with no value
> + my %q = map { $_ => undef } values(%{$options{tmap}});
> +
> + while (<STDIN>) {
> + next unless m/^([^=]+)=(.+)/;
> +
> + my ($token, $value) = ($1, $2);
> + die "Unknown search token $token" unless exists $q{$token};
> + $q{$token} = $value;
> + log_debug("We were given search token $token and value $value");
> + }
> +
> + foreach (sort keys %q) {
> + log_debug("Searching for %s = %s", $_, $q{$_} || '(any value)');
> + }
> +
> + return \%q;
> +}
> +
> +# takes the search tokens and then a list of entries
> +# each entry is a hash reference
> +sub find_netrc_entry
> +{
> + my $query = shift @_;
> +
> + ENTRY:
> + foreach my $entry (@_)
> + {
> + my $entry_text = join ', ', map { "$_=$entry->{$_}" } keys %$entry;
> + foreach my $check (sort keys %$query) {
> + if (defined $query->{$check}) {
> + log_debug("compare %s [%s] to [%s] (entry: %s)",
> + $check,
> + $entry->{$check},
> + $query->{$check},
> + $entry_text);
> + unless ($query->{$check} eq $entry->{$check}) {
> + next ENTRY;
> + }
> + }
> + else {
> + log_debug("OK: any value satisfies check $check");
> + }
> + }
> +
> + return $entry;
> + }
> +
> + # nothing was found
> + return;
> +}
> +
> +sub print_credential_data
> +{
> + my $entry = shift @_;
> + my $query = shift @_;
> +
> + log_debug("entry has passed all the search checks");
> + TOKEN:
> + foreach my $git_token (sort keys %$entry) {
> + log_debug("looking for useful token $git_token");
> + # don't print unknown (to the credential helper protocol) tokens
> + next TOKEN unless exists $query->{$git_token};
> +
> + # don't print things asked in the query (the entry matches them)
> + next TOKEN if defined $query->{$git_token};
> +
> + log_debug("FOUND: $git_token=$entry->{$git_token}");
> + printf "%s=%s\n", $git_token, $entry->{$git_token};
> + }
> +}
> +sub log_verbose {
> + return unless $options{verbose};
> + printf STDERR @_;
> + printf STDERR "\n";
> +}
> +
> +sub log_debug {
> + return unless $options{debug};
> + printf STDERR @_;
> + printf STDERR "\n";
> +}
^ permalink raw reply
* Re: [PATCH] Add contrib/credentials/netrc with GPG support, try #2
From: Junio C Hamano @ 2013-02-05 19:47 UTC (permalink / raw)
To: Ted Zlatanov; +Cc: Jeff King, git
In-Reply-To: <87k3qmr8yc.fsf@lifelogs.com>
Ted Zlatanov <tzz@lifelogs.com> writes:
> JCH> Oh, another thing. 'default' is like 'machine' followed by any
> JCH> machine name, so the above while loop that reads two tokens
> JCH> pair-wise needs to be aware that 'default' is not followed by a
> JCH> value. I think the loop will fail to parse this:
>
> JCH> default login anonymous password me@home
> JCH> machine k.org login me password mysecret
>
> I'd prefer to ignore "default" because it should not be used for the Git
> credential helpers (its only use case is for anonymous services AFAIK).
> So I'll add a case to ignore it in PATCHv4, if that's OK.
You still need to parse a file that has a "default" entry correctly;
otherwise the users won't be able to share existing .netrc files
with other applications e.g. ftp, which is the whole point of this
series. Not using values from the "default" entry is probably fine,
though.
Thanks.
^ permalink raw reply
* Re* [PATCH 3/3] apply: diagnose incomplete submodule object name better
From: Junio C Hamano @ 2013-02-05 19:19 UTC (permalink / raw)
To: git; +Cc: Heiko Voigt, Martin von Zweigbergk
In-Reply-To: <1359693125-22357-4-git-send-email-gitster@pobox.com>
Junio C Hamano <gitster@pobox.com> writes:
> We could read from the payload part of the patch to learn the full
> object name of the commit, but the primary user "git rebase" has
> been fixed to give us a full object name, so this should suffice
> for now.
And the patch on top to do so looks like this. With this patch in
place, we could drop 1/3 of this series, but there is no strong
reason to do so. After all, 1/3 is all about internal implementation
details.
-- >8 --
Subject: [PATCH 4/3] apply: verify submodule commit object name better
A textual patch also records the submodule commit object name in
full. Make the parsing more robust by reading from there and
verifying the (possibly abbreviated) name on the index line matches.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin/apply.c | 40 ++++++++++++++++++++++++++++++++++++++--
1 file changed, 38 insertions(+), 2 deletions(-)
diff --git a/builtin/apply.c b/builtin/apply.c
index 1f78e2c..e0f1474 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -3587,6 +3587,40 @@ static int get_current_sha1(const char *path, unsigned char *sha1)
return 0;
}
+static int preimage_sha1_in_gitlink_patch(struct patch *p, unsigned char sha1[20])
+{
+ /*
+ * A usable gitlink patch has only one fragment (hunk) that looks like:
+ * @@ -1 +1 @@
+ * -Subproject commit <old sha1>
+ * +Subproject commit <new sha1>
+ * or
+ * @@ -1 +0,0 @@
+ * -Subproject commit <old sha1>
+ * for a removal patch.
+ */
+ struct fragment *hunk = p->fragments;
+ static const char heading[] = "-Subproject commit ";
+ char *preimage;
+
+ if (/* does the patch have only one hunk? */
+ hunk && !hunk->next &&
+ /* is its preimage one line? */
+ hunk->oldpos == 1 && hunk->oldlines == 1 &&
+ /* does preimage begin with the heading? */
+ (preimage = memchr(hunk->patch, '\n', hunk->size)) != NULL &&
+ !prefixcmp(++preimage, heading) &&
+ /* does it record full SHA-1? */
+ !get_sha1_hex(preimage + sizeof(heading) - 1, sha1) &&
+ preimage[sizeof(heading) + 40 - 1] == '\n' &&
+ /* does the abbreviated name on the index line agree with it? */
+ !prefixcmp(preimage + sizeof(heading) - 1, p->old_sha1_prefix))
+ return 0; /* it all looks fine */
+
+ /* we may have full object name on the index line */
+ return get_sha1_hex(p->old_sha1_prefix, sha1);
+}
+
/* Build an index that contains the just the files needed for a 3way merge */
static void build_fake_ancestor(struct patch *list, const char *filename)
{
@@ -3607,8 +3641,10 @@ static void build_fake_ancestor(struct patch *list, const char *filename)
continue;
if (S_ISGITLINK(patch->old_mode)) {
- if (get_sha1_hex(patch->old_sha1_prefix, sha1))
- die("submoule change for %s without full index name",
+ if (!preimage_sha1_in_gitlink_patch(patch, sha1))
+ ; /* ok, the textual part looks sane */
+ else
+ die("sha1 information is lacking or useless for submoule %s",
name);
} else if (!get_sha1_blob(patch->old_sha1_prefix, sha1)) {
; /* ok */
--
1.8.1.2.639.g9428735
^ permalink raw reply related
* [PATCHv4] Add contrib/credentials/netrc with GPG support
From: Ted Zlatanov @ 2013-02-05 18:55 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git
In-Reply-To: <87k3qmr8yc.fsf@lifelogs.com>
Changes since PATCHv3:
- simple tests in Makefile
- support multiple files, code refactored
- documentation and comments updated
- fix IO::File for GPG pipe
- exit peacefully in almost every situation, die on bad invocation or query
- use log_verbose() and -v for logging for the user
- use log_debug() and -d for logging for the developer
- use Net::Netrc parser and `man netrc' to improve parsing
- ignore 'default' and 'macdef' netrc entries
- require 'machine' token in netrc lines
- ignore netrc files with bad permissions or owner (from Net::Netrc)
Signed-off-by: Ted Zlatanov <tzz@lifelogs.com>
---
contrib/credential/netrc/Makefile | 10 +
contrib/credential/netrc/git-credential-netrc | 423 +++++++++++++++++++++++++
2 files changed, 433 insertions(+), 0 deletions(-)
create mode 100644 contrib/credential/netrc/Makefile
create mode 100755 contrib/credential/netrc/git-credential-netrc
diff --git a/contrib/credential/netrc/Makefile b/contrib/credential/netrc/Makefile
new file mode 100644
index 0000000..ee8c5f0
--- /dev/null
+++ b/contrib/credential/netrc/Makefile
@@ -0,0 +1,10 @@
+test_netrc:
+ @(echo "bad data" | ./git-credential-netrc -f A -d -v) || echo "Bad invocation test, ignoring failure"
+ @echo "-> Silent invocation... nothing should show up here with a missing file"
+ @echo "bad data" | ./git-credential-netrc -f A get
+ @echo "-> Back to noisy: -v and -d used below, missing file"
+ echo "bad data" | ./git-credential-netrc -f A -d -v get
+ @echo "-> Look for any entry in the default file set"
+ echo "" | ./git-credential-netrc -d -v get
+ @echo "-> Look for github.com in the default file set"
+ echo "host=google.com" | ./git-credential-netrc -d -v get
diff --git a/contrib/credential/netrc/git-credential-netrc b/contrib/credential/netrc/git-credential-netrc
new file mode 100755
index 0000000..6946217
--- /dev/null
+++ b/contrib/credential/netrc/git-credential-netrc
@@ -0,0 +1,423 @@
+#!/usr/bin/perl
+
+use strict;
+use warnings;
+
+use Getopt::Long;
+use File::Basename;
+
+my $VERSION = "0.1";
+
+my %options = (
+ help => 0,
+ debug => 0,
+ verbose => 0,
+ file => [],
+
+ # identical token maps, e.g. host -> host, will be inserted later
+ tmap => {
+ port => 'protocol',
+ machine => 'host',
+ path => 'path',
+ login => 'username',
+ user => 'username',
+ password => 'password',
+ }
+ );
+
+# Map each credential protocol token to itself on the netrc side.
+foreach (values %{$options{tmap}}) {
+ $options{tmap}->{$_} = $_;
+}
+
+# Now, $options{tmap} has a mapping from the netrc format to the Git credential
+# helper protocol.
+
+# Next, we build the reverse token map.
+
+# When $rmap{foo} contains 'bar', that means that what the Git credential helper
+# protocol calls 'bar' is found as 'foo' in the netrc/authinfo file. Keys in
+# %rmap are what we expect to read from the netrc/authinfo file.
+
+my %rmap;
+foreach my $k (keys %{$options{tmap}}) {
+ push @{$rmap{$options{tmap}->{$k}}}, $k;
+}
+
+Getopt::Long::Configure("bundling");
+
+# TODO: maybe allow the token map $options{tmap} to be configurable.
+GetOptions(\%options,
+ "help|h",
+ "debug|d",
+ "verbose|v",
+ "file|f=s@",
+ );
+
+if ($options{help}) {
+ my $shortname = basename($0);
+ $shortname =~ s/git-credential-//;
+
+ print <<EOHIPPUS;
+
+$0 [-f AUTHFILE1] [-f AUTHFILEN] [-d] [-v] get
+
+Version $VERSION by tzz\@lifelogs.com. License: BSD.
+
+Options:
+
+ -f|--file AUTHFILE : specify netrc-style files. Files with the .gpg extension
+ will be decrypted by GPG before parsing. Multiple -f
+ arguments are OK, and the order is respected.
+
+ -d|--debug : turn on debugging (developer info)
+
+ -v|--verbose : be more verbose (show files and information found)
+
+To enable this credential helper:
+
+ git config credential.helper '$shortname -f AUTHFILE1 -f AUTHFILE2'
+
+(Note that Git will prepend "git-credential-" to the helper name and look for it
+in the path.)
+
+...and if you want lots of debugging info:
+
+ git config credential.helper '$shortname -f AUTHFILE -d'
+
+...or to see the files opened and data found:
+
+ git config credential.helper '$shortname -f AUTHFILE -v'
+
+Only "get" mode is supported by this credential helper. It opens every AUTHFILE
+and looks for the first entry that matches the requested search criteria:
+
+ 'port|protocol':
+ The protocol that will be used (e.g., https). (protocol=X)
+
+ 'machine|host':
+ The remote hostname for a network credential. (host=X)
+
+ 'path':
+ The path with which the credential will be used. (path=X)
+
+ 'login|user|username':
+ The credential’s username, if we already have one. (username=X)
+
+Thus, when we get this query on STDIN:
+
+protocol=https
+username=tzz
+
+this credential helper will look for the first entry in every AUTHFILE that
+matches
+
+port https login tzz
+
+OR
+
+protocol https login tzz
+
+OR... etc. acceptable tokens as listed above. Any unknown tokens are
+simply ignored.
+
+Then, the helper will print out whatever tokens it got from the entry, including
+"password" tokens, mapping back to Git's helper protocol; e.g. "port" is mapped
+back to "protocol".
+
+Again, note that the first matching entry from all the AUTHFILEs is used.
+
+Tokens can be quoted as 'STRING' or "STRING".
+
+No caching is performed by this credential helper.
+
+EOHIPPUS
+
+ exit 0;
+}
+
+my $mode = shift @ARGV;
+
+# Credentials must get a parameter, so die if it's missing.
+die "Syntax: $0 [-f AUTHFILE1] [-f AUTHFILEN] [-d] get" unless defined $mode;
+
+# Only support 'get' mode; with any other unsupported ones we just exit.
+exit 0 unless $mode eq 'get';
+
+my $files = $options{file};
+
+# if no files were given, use a predefined list.
+# note that .gpg files come first
+unless (scalar @$files)
+{
+ my @candidates = qw[
+ ~/.authinfo.gpg
+ ~/.netrc.gpg
+ ~/.authinfo
+ ~/.netrc
+ ];
+
+ $files = $options{file} = [ map { glob $_ } @candidates ];
+}
+
+my $query = read_credential_data_from_stdin();
+
+FILE:
+foreach my $file (@$files)
+{
+ unless (-r $file)
+ {
+ log_verbose("Unable to read $file; skipping it");
+ next FILE;
+ }
+
+ # the following check is copied from Net::Netrc
+ # OS/2 and Win32 do not handle stat in a way compatable with this check :-(
+ unless ($^O eq 'os2'
+ || $^O eq 'MSWin32'
+ || $^O eq 'MacOS'
+ || $^O =~ /^cygwin/)
+ {
+ my @stat = stat($file);
+
+ if (@stat) {
+ if ($stat[2] & 077) {
+ log_verbose("Insecure $file (mode=%04o); skipping it",
+ $stat[2] & 07777);
+ next FILE;
+ }
+ if ($stat[4] != $<) {
+ log_verbose("Not owner of $file; skipping it");
+ next FILE;
+ }
+ }
+ }
+
+ my $mode = (stat($file))[2];
+ if ($mode & 077)
+ {
+ log_verbose("Insecure $file (mode=%04o); skipping it",
+ $mode & 07777);
+ next FILE;
+ }
+
+ my @entries = load_netrc($file);
+
+ unless (scalar @entries)
+ {
+ if ($!)
+ {
+ log_verbose("Unable to open $file: $!");
+ }
+ else
+ {
+ log_verbose("No netrc entries found in $file");
+ }
+
+ next FILE;
+ }
+
+ my $entry = find_netrc_entry($query, @entries);
+ if ($entry)
+ {
+ print_credential_data($entry, $query);
+ # we're done!
+ last FILE;
+ }
+}
+
+exit 0;
+
+sub load_netrc
+{
+ my $file = shift @_;
+
+ my $io;
+ if ($file =~ m/\.gpg$/) {
+ log_verbose("Using GPG to open $file");
+ # GPG doesn't work well with 2- or 3-argument open
+ $io = new IO::File("gpg --decrypt $file|");
+ }
+ else {
+ log_verbose("Opening $file...");
+ $io = new IO::File($file, '<');
+ }
+
+ # nothing to do if the open failed (we log the error later)
+ return unless $io;
+
+ # Net::Netrc does this, but the functionality is merged with the file
+ # detection logic, so we have to extract just the part we need
+ my @netrc_entries = net_netrc_loader($io);
+
+ # these entries will use the credential helper protocol token names
+ my @entries;
+
+ foreach my $nentry (@netrc_entries) {
+ my %entry;
+ my $num_port;
+
+ if (defined $nentry->{port} && $nentry->{port} =~ m/^\d+$/) {
+ $num_port = $nentry->{port};
+ delete $nentry->{port};
+ }
+
+ # create the new entry for the credential helper protocol
+ $entry{$options{tmap}->{$_}} = $nentry->{$_} foreach keys %$nentry;
+
+ # for "host X port Y" where Y is an integer (captured by
+ # $num_port above), set the host to "X:Y"
+ if (defined $entry{host} && defined $num_port) {
+ $entry{host} = join(':', $entry{host}, $num_port);
+ }
+
+ push @entries, \%entry;
+ }
+
+ return @entries;
+}
+
+sub net_netrc_loader
+{
+ my $fh = shift @_;
+ my @entries;
+ my ($mach, $macdef, $tok, @tok) = (0, 0);
+
+ LINE:
+ while (<$fh>) {
+ undef $macdef if /\A\n\Z/;
+
+ if ($macdef) {
+ push(@$macdef, $_);
+ next LINE;
+ }
+
+ s/^\s*//;
+ chomp;
+
+ while (length && s/^("((?:[^"]+|\\.)*)"|((?:[^\\\s]+|\\.)*))\s*//) {
+ (my $tok = $+) =~ s/\\(.)/$1/g;
+ push(@tok, $tok);
+ }
+
+ TOKEN:
+ while (@tok) {
+ $tok = shift(@tok);
+
+ if ($tok eq "machine") {
+ my $host = shift @tok;
+ $mach = { machine => $host };
+ push @entries, $mach;
+ }
+ elsif (exists $options{tmap}->{$tok}) {
+ unless ($mach) {
+ log_debug("Skipping token $tok because no machine was given");
+ next TOKEN;
+ }
+
+ my $value = shift @tok;
+ unless (defined $value) {
+ log_debug("Token $tok had no value, skipping it.");
+ next TOKEN;
+ }
+
+ # Following line added by rmerrell to remove '/' escape char in .netrc
+ $value =~ s/\/\\/\\/g;
+ $mach->{$tok} = $value;
+ }
+ elsif ($tok eq "macdef") { # we ignore macros
+ next TOKEN unless $mach;
+ my $value = shift @tok;
+ $mach->{macdef} = {} unless exists $mach->{macdef};
+ $macdef = $mach->{machdef}{$value} = [];
+ }
+ }
+ }
+
+ return @entries;
+}
+
+sub read_credential_data_from_stdin
+{
+ # the query: start with every token with no value
+ my %q = map { $_ => undef } values(%{$options{tmap}});
+
+ while (<STDIN>) {
+ next unless m/^([^=]+)=(.+)/;
+
+ my ($token, $value) = ($1, $2);
+ die "Unknown search token $token" unless exists $q{$token};
+ $q{$token} = $value;
+ log_debug("We were given search token $token and value $value");
+ }
+
+ foreach (sort keys %q) {
+ log_debug("Searching for %s = %s", $_, $q{$_} || '(any value)');
+ }
+
+ return \%q;
+}
+
+# takes the search tokens and then a list of entries
+# each entry is a hash reference
+sub find_netrc_entry
+{
+ my $query = shift @_;
+
+ ENTRY:
+ foreach my $entry (@_)
+ {
+ my $entry_text = join ', ', map { "$_=$entry->{$_}" } keys %$entry;
+ foreach my $check (sort keys %$query) {
+ if (defined $query->{$check}) {
+ log_debug("compare %s [%s] to [%s] (entry: %s)",
+ $check,
+ $entry->{$check},
+ $query->{$check},
+ $entry_text);
+ unless ($query->{$check} eq $entry->{$check}) {
+ next ENTRY;
+ }
+ }
+ else {
+ log_debug("OK: any value satisfies check $check");
+ }
+ }
+
+ return $entry;
+ }
+
+ # nothing was found
+ return;
+}
+
+sub print_credential_data
+{
+ my $entry = shift @_;
+ my $query = shift @_;
+
+ log_debug("entry has passed all the search checks");
+ TOKEN:
+ foreach my $git_token (sort keys %$entry) {
+ log_debug("looking for useful token $git_token");
+ # don't print unknown (to the credential helper protocol) tokens
+ next TOKEN unless exists $query->{$git_token};
+
+ # don't print things asked in the query (the entry matches them)
+ next TOKEN if defined $query->{$git_token};
+
+ log_debug("FOUND: $git_token=$entry->{$git_token}");
+ printf "%s=%s\n", $git_token, $entry->{$git_token};
+ }
+}
+sub log_verbose {
+ return unless $options{verbose};
+ printf STDERR @_;
+ printf STDERR "\n";
+}
+
+sub log_debug {
+ return unless $options{debug};
+ printf STDERR @_;
+ printf STDERR "\n";
+}
--
1.7.9.rc2
^ permalink raw reply related
* Re: Bug in "git log --graph -p -m" (version 1.7.7.6)
From: Junio C Hamano @ 2013-02-05 17:40 UTC (permalink / raw)
To: Dale R. Worley; +Cc: git
In-Reply-To: <201302051700.r15H0GXx031004@freeze.ariadne.com>
worley@alum.mit.edu (Dale R. Worley) writes:
> I have found a situation where "git log" produces (apparently)
> endless output. Presumably this is a bug. Following is a (Linux)
> script that reliably reproduces the error for me (on Fedora 16):
Wasn't this fixed at v1.8.1.1~13 or is this a different issue?
^ permalink raw reply
* Re: [PATCH v3 0/8] Hiding refs
From: Junio C Hamano @ 2013-02-05 17:38 UTC (permalink / raw)
To: Michael Haggerty; +Cc: Jonathan Nieder, git, Jeff King, Shawn Pearce
In-Reply-To: <5110DF1D.8010505@alum.mit.edu>
Michael Haggerty <mhagger@alum.mit.edu> writes:
> On 02/05/2013 09:33 AM, Jonathan Nieder wrote:
>> Michael Haggerty wrote:
>>
>>> I would again like to express my discomfort about this feature, which is
>>> already listed as "will merge to next". Frankly, I have the feeling
>>> that this feature is being steamrolled in before a community consensus
>>> has been reached and indeed before many valid points raised by other
>>> members of the community have even been addressed. For example:
>>
>> In $dayjob I work with Gerrit, so I think I can start to answer some
>> of these questions.
>>
>>> * I didn't see a response to Peff's convincing arguments that this
>>> should be a client-side feature rather than a server-side feature [1].
>>
>> The client can't control the size of the ref advertisement. That is
>> the main motivation if I understood correctly.
>
> Not according to Junio [4]:
>
> Look at this as a mechanism for the repository owner to control the
> clutter in what is shown to the intended audience of what s/he
> publishes in the repository. Network bandwidth reduction of
> advertisement is a side effect of clutter reduction, and not
> necessarily the primary goal.
See my response to Jonathan.
> Hiderefs creates a "dark" corner of a remote git repo that can hold
> arbitrary content that is impossible for anybody to discover but
> nevertheless possible for anybody to download (if they know the name of
> a hidden reference).
That is why allow-tip-sha1-in-want is a separate opt-in feature only
the server side controls.
^ permalink raw reply
* Re: [PATCH v3 0/8] Hiding refs
From: Junio C Hamano @ 2013-02-05 17:36 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Michael Haggerty, git, Jeff King, Shawn Pearce
In-Reply-To: <20130205083327.GA4931@elie.Belkin>
Jonathan Nieder <jrnieder@gmail.com> writes:
>> * I didn't see a response to Peff's convincing arguments that this
>> should be a client-side feature rather than a server-side feature [1].
>
> The client can't control the size of the ref advertisement. That is
> the main motivation if I understood correctly.
The answer to this question is more nuanced.
With the current protocol, it is upload-pack who speaks first, so
there is no way for the requestor to say "I am from an updated Git
suite and understand how to tell you to give me limited set of
refs", before upload-pack blasts 4MB of ref advertisement to it.
If the side that fetches is potentially interested in finding out
any and all refs, then an alternative solution would be to break
the current protocol, open a separate port and have upload-pack-2
listen to it, sit silently to let the requestor speak first when it
gets connection to that port.
But if the primary thing you are interested in is to hide the
references that:
(1) the server side needs to keep track of for its own use; but
(2) the requestors do not have to learn about from upload-pack,
we can do so without breaking older requestors. That is what the
early part of this series is about. We can view the last patch to
add the allow-tip-sha1-in-want as an icing on the cake.
It has the side effect of reducing the transfer overhead, because by
hiding the internal refs, the server side will stop blasting 4MB of
ref advertisements the requestors are not interested in, and that
would be the primary observable outcome from the end-user's point of
view (i.e. your "git pull --ff-only" will become a lot faster).
^ permalink raw reply
* Re: [PATCH v3 0/8] Hiding refs
From: Junio C Hamano @ 2013-02-05 17:27 UTC (permalink / raw)
To: Michael Haggerty; +Cc: git, Jeff King, Shawn Pearce
In-Reply-To: <5110BD18.3080608@alum.mit.edu>
Michael Haggerty <mhagger@alum.mit.edu> writes:
> I would again like to express my discomfort about this feature, which is
> already listed as "will merge to next".
Do not take "will merge to next" too literally. One major purpose
of marking a topic as such is exactly to solicit comments like this
;-)
> * I didn't see a response to Peff's convincing arguments that this
> should be a client-side feature rather than a server-side feature [1].
Uncluttering is not about a choice client should make. "delayed
advertisement" is an orthogonal issue and requires a larger protocol
update (it needs to make "git fetch" speak first instead of the
current protocol in which "upload-pack" speaks first).
> * I didn't see an answer to Duy's question [2] about what is different
> between the proposed feature and gitnamespaces.
I think Jonathan addressed this already.
> * I didn't see a response to my worries that this feature could be
> abused [3].
You can choose not to advertise allow-tip-sha1-in-want capability; I
do not think it is making things worse than the status quo.
> * Why should a repository have exactly one setting for what refs should
> be hidden? Wouldn't it make more sense to allow multiple "views" to be
> defined?:
You are welcome to extend to have different views, but how would
your clients express which view they would want?
Giving a single view that the serving end decides gives us an
immediate benefit of showing an uncluttered set of refs of server's
choice, without making the problem space larger than necessary.
> * Is it enough to support only reference exclusion (as opposed to
> exclusion and inclusion rules)?
Again, I do not think you cannot extend it to do positive and
negative filtering "exclude these, but include those even though
they match the 'exclude these' patterns I gave you earlier".
> * Why should this feature only be available remotely?
The whole point is to give the server side a choice to show selected
refs, so that it can use hidden portion for its own use. These refs
should not be hidden from local operations like "gc".
I appreciate the comments, but I do not think any point you raised
in this message is very much relevant as objections.
^ permalink raw reply
* Re: [PATCH v2 2/2] i18n: mark OPTION_NUMBER (-NUM) for translation
From: Junio C Hamano @ 2013-02-05 17:07 UTC (permalink / raw)
To: Jiang Xin; +Cc: Duy Nguyen, Git List
In-Reply-To: <8d6d4d869ea58e0a26b3bb6377fc102728948997.1360080194.git.worldhello.net@gmail.com>
Jiang Xin <worldhello.net@gmail.com> writes:
> Signed-off-by: Jiang Xin <worldhello.net@gmail.com>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> parse-options.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/parse-options.c b/parse-options.c
> index cd029f..be916 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -497,6 +497,8 @@ static int usage_with_options_internal(struct parse_opt_ctx_t *ctx,
> const struct option *opts, int full, int err)
> {
> FILE *outfile = err ? stderr : stdout;
> + const char *opt_num_buff = _("-NUM");
> + int opt_num_size = utf8_strwidth(opt_num_buff);
>
> if (!usagestr)
> return PARSE_OPT_HELP;
> @@ -544,8 +546,10 @@ static int usage_with_options_internal(struct parse_opt_ctx_t *ctx,
> pos += fprintf(outfile, ", ");
> if (opts->long_name)
> pos += fprintf(outfile, "--%s", opts->long_name);
> - if (opts->type == OPTION_NUMBER)
> - pos += fprintf(outfile, "-NUM");
> + if (opts->type == OPTION_NUMBER) {
> + fputs(opt_num_buff, outfile);
> + pos += opt_num_size;
> + }
I somehow suspect that this is going in a direction that makes this
piece of code much less maintainable.
Look at the entire function and see how many places you do fprintf
on strings that are marked with _(). short_name and long_name are
not likely to be translated, but everything else is, especially
multiple places that show _(opts->help) neither of these patches
touch.
I wonder if it makes more sense to add a helper function that
returns the number of column positions (not bytes) with a signature
similar to fprintf() and use that throughout the function instead.
^ permalink raw reply
* Bug in "git log --graph -p -m" (version 1.7.7.6)
From: Dale R. Worley @ 2013-02-05 17:00 UTC (permalink / raw)
To: git
I have found a situation where "git log" produces (apparently)
endless output. Presumably this is a bug. Following is a (Linux)
script that reliably reproduces the error for me (on Fedora 16):
----------
set -ve
# Print the git version.
git --version
# Create respository.
rm -rf .git
git init
# Initial commit.
( echo 1 ; echo 2 ; echo 3 ) >file
git add file
git commit -m 'Commit P'
git branch B HEAD
# Next commit on master adds line "1a".
( echo 1 ; echo 1a ; echo 2 ; echo 3 ) >file
git add file
git commit -m 'Commit Q'
git checkout B
# Next commit on B adds line "2a".
( echo 1 ; echo 2 ; echo 2a ; echo 3 ) >file
git add file
git commit -m 'Commit R'
# Merge the two commits, but add line "3a" to the commit as well.
git checkout master
git merge --no-commit B
# Show what the merge produces.
cat file
# Add line "3a".
( echo 1 ; echo 1a ; echo 2 ; echo 2a ; echo 3 ; echo 3a ) >file
git commit -m 'Commit S'
# These log commands work.
git log
git log --graph
git log --graph -p
# This log command produces infinite output.
git log --graph -p -m
----------
Dale
^ permalink raw reply
* Re: [PATCH] Add contrib/credentials/netrc with GPG support, try #2
From: Ted Zlatanov @ 2013-02-05 17:01 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git
In-Reply-To: <7vvca6u47f.fsf@alter.siamese.dyndns.org>
On Tue, 05 Feb 2013 08:15:48 -0800 Junio C Hamano <gitster@pobox.com> wrote:
JCH> Ted Zlatanov <tzz@lifelogs.com> writes:
>> +# build reverse token map
>> +my %rmap;
>> +foreach my $k (keys %{$options{tmap}}) {
>> + push @{$rmap{$options{tmap}->{$k}}}, $k;
>> +}
JCH> Mental note: "$rmap{foo} -eq 'bar'" means that what Git calls 'bar'
JCH> is found as 'foo' in the netrc/authinfo file. Keys in %rmap are
JCH> what we expect to read from the netrc/authinfo file.
I'll document that better in PATCHv4.
JCH> So you grabbed one line of input, split them into token pairs, and
JCH> built %tokens = ('key Git may want to see' => 'value read from file')
JCH> mapping.
This will be fixed with PATCHv4 to do multiple lines (as defined by the
netrc manpage, etc.).
>> + # for "host X port Y" where Y is an integer (captured by
>> + # $num_port above), set the host to "X:Y"
>> + $tokens{host} = join(':', $tokens{host}, $num_port)
>> + if defined $tokens{host} && defined $num_port;
JCH> What happens when 'host' does not exist? netrc/authinfo should be a
JCH> stream of SP/HT/LF delimited tokens and 'machine' token (or
JCH> 'default') begins a new entry, so it would mean the input file is
JCH> corrupt if we do not have $tokens{host} when we get here, I think.
Yes. I'll make the host/machine token required, which will avoid this
issue.
JCH> Oh, another thing. 'default' is like 'machine' followed by any
JCH> machine name, so the above while loop that reads two tokens
JCH> pair-wise needs to be aware that 'default' is not followed by a
JCH> value. I think the loop will fail to parse this:
JCH> default login anonymous password me@home
JCH> machine k.org login me password mysecret
I'd prefer to ignore "default" because it should not be used for the Git
credential helpers (its only use case is for anonymous services AFAIK).
So I'll add a case to ignore it in PATCHv4, if that's OK.
JCH> Hmph, aren't you checking what you read a bit too early? This is a
JCH> valid input:
JCH> default
JCH> login anonymous
JCH> password me@home
JCH> machine k.org
JCH> login me
JCH> password mysecret
JCH> but does this loop gives mysecret back to me when asked for
JCH> host=k.org and user=me?
To be fixed in PATCHv4, which will require the host/machine, use it as
the primary key, and only examine entries with it.
JCH> I would probably structure this part like this: [...]
I will do it like that, thank you for the suggestion.
I'll also add a simple testing Makefile for my own use, and you can
consider adding tests to the general framework later.
Ted
^ permalink raw reply
* Re: [PATCH v3] status: show the branch name if possible in in-progress info
From: Junio C Hamano @ 2013-02-05 16:28 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Jonathan Nieder, git, Matthieu Moy
In-Reply-To: <CACsJy8D_kSrJYagxo1LWt=P8XKj9F9_yAwH3vzQWQOi+yvP8kg@mail.gmail.com>
Duy Nguyen <pclouds@gmail.com> writes:
> On Tue, Feb 5, 2013 at 1:38 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>> Missing description. Stealing from the link you sent:
>>
>> The typical use-case is starting a rebase, do something else, come back
>> the day after, run "git status" or make a new commit and wonder what
>> in the world's going on. Which branch is being rebased is probably the
>> most useful tidbit to help, but the target may help too.
>>
>> Ideally, I would have loved to see "rebasing master on origin/master",
>> but the target ref name is not stored during rebase, so this patch
>> writes "rebasing master on a78c8c98b" as a half-measure to remind
>> future users of that potential improvement.
>>
>> Signed-off-by: <...>
>
> Looking good. Junio, do you need a new patch mail or can you just
> amend the commit message?
I'd like to see you either
- send a reroll, making it clear it is a reroll, or
- tell me to amend.
instead of asking which one I would prefer ;-) One less message I
have to respond to that way.
"commit --amend" done.
^ permalink raw reply
* Re: [WIP/RFH/RFD/PATCH] grep: allow to use textconv filters
From: Michael J Gruber @ 2013-02-05 16:21 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git
In-Reply-To: <20130205111353.GD24973@sigill.intra.peff.net>
Jeff King venit, vidit, dixit 05.02.2013 12:13:
> On Mon, Feb 04, 2013 at 04:27:31PM +0100, Michael J Gruber wrote:
>
>> Recently and not so recently, we made sure that log/grep type operations
>> use textconv filters when a userfacing diff would do the same:
>>
>> ef90ab6 (pickaxe: use textconv for -S counting, 2012-10-28)
>> b1c2f57 (diff_grep: use textconv buffers for add/deleted files, 2012-10-28)
>> 0508fe5 (combine-diff: respect textconv attributes, 2011-05-23)
>>
>> "git grep" currently does not use textconv filters at all, that is
>> neither for displaying the match and context nor for the actual grepping.
>>
>> Introduce a binary mode "--textconv" (in addition to "--text" and "-I")
>> which makes git grep use any configured textconv filters for grepping
>> and output purposes.
>
> Sounds like a reasonable goal.
>
>> The difficulty is in getting the different cases (blob/sha1 vs.
>> worktree) right, and in making the changes minimally invasive. It seems
>> that some more refactoring could help: "git show --textconv" does not
>> use textconv filters when used on blobs either. (It does for diffs, of
>> course.) Most existing helper functions are tailored for diffs.
>
> I think "git show" with blobs originally did not because we have no
> filename with which to look up the attributes. IIRC, the patches to
> support "cat-file --textconv" taught get_sha1_with_context to report the
> path at which we found a blob. I suspect it is mostly a matter of
> plumbing that information from the revision parser through to
> show_blob_object.
>
>> Nota bene: --textconv does not affect "diff --stat" either...
>
> Yeah, though I wonder if it should be on by default. The diffstat for a
> binary file, unlike the diff, is already useful. The diffstat of the
> textconv'd data may _also_ be useful, of course.
>
>> @@ -659,6 +659,9 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
>> OPT_SET_INT('I', NULL, &opt.binary,
>> N_("don't match patterns in binary files"),
>> GREP_BINARY_NOMATCH),
>> + OPT_SET_INT(0, "textconv", &opt.binary,
>> + N_("process binary files with textconv filters"),
>> + GREP_BINARY_TEXTCONV),
>
> Is this really a new form of GREP_BINARY_*? What happens when a file
> does not have a textconv filter?
>
> I would expect this to be more like diff's "--textconv" flag, which is
> really "allow textconv to be respected". Then you could do:
>
> git grep -I --textconv foo
>
> to grep in the text version of files which support it, and ignore the
> rest.
>
>> -static int grep_source_load(struct grep_source *gs);
>> -static int grep_source_is_binary(struct grep_source *gs);
>> +static int grep_source_load(struct grep_source *gs, struct grep_opt *opt);
>> +static int grep_source_is_binary(struct grep_source *gs, struct grep_opt *opt);
>
> Hmm. grep_source_load is more or less the analogue of
> diff_populate_filespec, which does not know about textconv at all. So I
> feel like this might be going in at the wrong layer...
>
>> @@ -1354,14 +1356,15 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle
>>
>> switch (opt->binary) {
>> case GREP_BINARY_DEFAULT:
>> - if (grep_source_is_binary(gs))
>> + if (grep_source_is_binary(gs, opt))
>> binary_match_only = 1;
>> break;
>> case GREP_BINARY_NOMATCH:
>> - if (grep_source_is_binary(gs))
>> + if (grep_source_is_binary(gs, opt))
>> return 0; /* Assume unmatch */
>> break;
>
> The is_binary function learned about "opt" so that it could pass it to
> grep_source_load, which might do the textconv for us. But we _know_ that
> we will not here, because we see that we have other GREP_BINARY flags.
>
> And when we do have _TEXTCONV:
>
>> case GREP_BINARY_TEXT:
>> + case GREP_BINARY_TEXTCONV:
>> break;
>
> We do not call is_binary. :)
>
>> -static int grep_source_load_sha1(struct grep_source *gs)
>> +static int grep_source_load_sha1(struct grep_source *gs, struct grep_opt *opt)
>> {
>> enum object_type type;
>> -
>> grep_read_lock();
>> - gs->buf = read_sha1_file(gs->identifier, &type, &gs->size);
>> + if (opt->binary == GREP_BINARY_TEXTCONV) {
>> + struct diff_filespec *df = alloc_filespec(gs->name);
>> + gs->size = fill_textconv(gs->driver, df, &gs->buf);
>> + free_filespec(df);
>> + } else {
>> + gs->buf = read_sha1_file(gs->identifier, &type, &gs->size);
>> + }
>
> So here we do the textconv for the sha1 case. But what about file
> sources?
>
> This is why I think the layer is wrong; you want the fill_textconv
> function to call your abstract _load function (in the diff_filespec
> world, it is diff_filespec_populate, but it is the moral equivalent).
> And you want it to hold off as long as possible in case we can pull the
> value from cache, or feed the working tree version of a file straight to
> the filter.
>
>> -void grep_source_load_driver(struct grep_source *gs)
>> +void grep_source_load_driver(struct grep_source *gs, struct grep_opt *opt)
>> {
>> if (gs->driver)
>> return;
>>
>> - grep_attr_lock();
>> + grep_attr_lock(); //TODO
>> + printf("Looking up userdiff driver for: %s", gs->path);
>> if (gs->path)
>> gs->driver = userdiff_find_by_path(gs->path);
>> if (!gs->driver)
>> gs->driver = userdiff_find_by_name("default");
>> + if (opt->binary == GREP_BINARY_TEXTCONV)
>> + gs->driver = userdiff_get_textconv(gs->driver);
>> grep_attr_unlock();
>> }
>
> This is wrong. The point of userdiff_get_textconv is that it will return
> NULL when we are not doing textconv for this path. So you can use it
> like:
>
> struct userdiff_driver *textconv = userdiff_get_textconv(gs->driver);
>
> if (textconv) {
> /* ok, we are doing textconv. Call our fill_textconv
> * equivalent. */
> }
> else {
> /* nope, plain old file. */
> }
>
> But by assigning it on top of gs->driver, you're going to end up with a
> NULL driver sometimes. And the post-condition of the load_driver
> function is that gs->driver always points to a valid driver (even if it
> is the default one). I wouldn't be surprised if this causes segfaults.
>
>
> So I would do it more like the patch below. Only lightly tested by me.
> There are some refactoring opportunities if you want to bring
> grep_source and diff_filespec closer together.
>
> ---
> diff --git a/builtin/grep.c b/builtin/grep.c
> index 8025964..915c8ef 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -659,6 +659,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
> OPT_SET_INT('I', NULL, &opt.binary,
> N_("don't match patterns in binary files"),
> GREP_BINARY_NOMATCH),
> + OPT_BOOL(0, "textconv", &opt.allow_textconv,
> + N_("process binary files with textconv filters")),
> { OPTION_INTEGER, 0, "max-depth", &opt.max_depth, N_("depth"),
> N_("descend at most <depth> levels"), PARSE_OPT_NONEG,
> NULL, 1 },
> diff --git a/grep.c b/grep.c
> index 4bd1b8b..3880d64 100644
> --- a/grep.c
> +++ b/grep.c
> @@ -2,6 +2,8 @@
> #include "grep.h"
> #include "userdiff.h"
> #include "xdiff-interface.h"
> +#include "diff.h"
> +#include "diffcore.h"
>
> static int grep_source_load(struct grep_source *gs);
> static int grep_source_is_binary(struct grep_source *gs);
> @@ -1321,6 +1323,58 @@ static void std_output(struct grep_opt *opt, const void *buf, size_t size)
> fwrite(buf, size, 1, stdout);
> }
>
> +static int fill_textconv_grep(struct userdiff_driver *driver,
> + struct grep_source *gs)
> +{
> + struct diff_filespec *df;
> + char *buf;
> + size_t size;
> +
> + if (!driver || !driver->textconv)
> + return grep_source_load(gs);
> +
> + /*
> + * The textconv interface is intimately tied to diff_filespecs, so we
> + * have to pretend to be one. If we could unify the grep_source
> + * and diff_filespec structs, this mess could just go away.
> + */
> + df = alloc_filespec(gs->path);
> + switch (gs->type) {
> + case GREP_SOURCE_SHA1:
> + fill_filespec(df, gs->identifier, 1, 0100644);
> + break;
> + case GREP_SOURCE_FILE:
> + fill_filespec(df, null_sha1, 0, 0100644);
> + break;
> + default:
> + die("BUG: attempt to textconv something without a path?");
> + }
> +
> + /*
> + * fill_textconv is not remotely thread-safe; it may load objects
> + * behind the scenes, and it modifies the global diff tempfile
> + * structure.
> + */
> + grep_read_lock();
> + size = fill_textconv(driver, df, &buf);
> + grep_read_unlock();
> + free_filespec(df);
> +
> + /*
> + * The normal fill_textconv usage by the diff machinery would just keep
> + * the textconv'd buf separate from the diff_filespec. But much of the
> + * grep code passes around a grep_source and assumes that its "buf"
> + * pointer is the beginning of the thing we are searching. So let's
> + * install our textconv'd version into the grep_source, taking care not
> + * to leak any existing buffer.
> + */
> + grep_source_clear_data(gs);
> + gs->buf = buf;
> + gs->size = size;
> +
> + return 0;
> +}
> +
> static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int collect_hits)
> {
> char *bol;
> @@ -1331,6 +1385,7 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle
> unsigned count = 0;
> int try_lookahead = 0;
> int show_function = 0;
> + struct userdiff_driver *textconv = NULL;
> enum grep_context ctx = GREP_CONTEXT_HEAD;
> xdemitconf_t xecfg;
>
> @@ -1352,19 +1407,36 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle
> }
> opt->last_shown = 0;
>
> - switch (opt->binary) {
> - case GREP_BINARY_DEFAULT:
> - if (grep_source_is_binary(gs))
> - binary_match_only = 1;
> - break;
> - case GREP_BINARY_NOMATCH:
> - if (grep_source_is_binary(gs))
> - return 0; /* Assume unmatch */
> - break;
> - case GREP_BINARY_TEXT:
> - break;
> - default:
> - die("bug: unknown binary handling mode");
> + if (opt->allow_textconv) {
> + grep_source_load_driver(gs);
> + /*
> + * We might set up the shared textconv cache data here, which
> + * is not thread-safe.
> + */
> + grep_attr_lock();
> + textconv = userdiff_get_textconv(gs->driver);
> + grep_attr_unlock();
> + }
> +
> + /*
> + * We know the result of a textconv is text, so we only have to care
> + * about binary handling if we are not using it.
> + */
> + if (!textconv) {
> + switch (opt->binary) {
> + case GREP_BINARY_DEFAULT:
> + if (grep_source_is_binary(gs))
> + binary_match_only = 1;
> + break;
> + case GREP_BINARY_NOMATCH:
> + if (grep_source_is_binary(gs))
> + return 0; /* Assume unmatch */
> + break;
> + case GREP_BINARY_TEXT:
> + break;
> + default:
> + die("bug: unknown binary handling mode");
> + }
> }
>
> memset(&xecfg, 0, sizeof(xecfg));
> @@ -1372,7 +1444,7 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle
>
> try_lookahead = should_lookahead(opt);
>
> - if (grep_source_load(gs) < 0)
> + if (fill_textconv_grep(textconv, gs) < 0)
> return 0;
>
> bol = gs->buf;
> diff --git a/grep.h b/grep.h
> index 8fc854f..94a7ac2 100644
> --- a/grep.h
> +++ b/grep.h
> @@ -106,6 +106,7 @@ struct grep_opt {
> #define GREP_BINARY_NOMATCH 1
> #define GREP_BINARY_TEXT 2
> int binary;
> + int allow_textconv;
> int extended;
> int use_reflog_filter;
> int pcre;
>
Thanks Jeff, that helps a lot! It covers "grep expr" and "grep expr rev
-- path" just fine. I'll look into "grep expr rev:path" which does not
work yet because of an empty driver.
I also have "show --textconv" covered and a suggestion for "cat-file
--textconv" (to work without a textconv filter).
Expect a mini-series soon :)
Michael
^ permalink raw reply
* [PATCH v2 2/2] i18n: mark OPTION_NUMBER (-NUM) for translation
From: Jiang Xin @ 2013-02-05 16:16 UTC (permalink / raw)
To: Junio C Hamano, Duy Nguyen; +Cc: Git List, Jiang Xin
In-Reply-To: <883efc2358a0deb48bee48134d45ddd528a732d3.1360080194.git.worldhello.net@gmail.com>
Signed-off-by: Jiang Xin <worldhello.net@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
parse-options.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/parse-options.c b/parse-options.c
index cd029f..be916 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -497,6 +497,8 @@ static int usage_with_options_internal(struct parse_opt_ctx_t *ctx,
const struct option *opts, int full, int err)
{
FILE *outfile = err ? stderr : stdout;
+ const char *opt_num_buff = _("-NUM");
+ int opt_num_size = utf8_strwidth(opt_num_buff);
if (!usagestr)
return PARSE_OPT_HELP;
@@ -544,8 +546,10 @@ static int usage_with_options_internal(struct parse_opt_ctx_t *ctx,
pos += fprintf(outfile, ", ");
if (opts->long_name)
pos += fprintf(outfile, "--%s", opts->long_name);
- if (opts->type == OPTION_NUMBER)
- pos += fprintf(outfile, "-NUM");
+ if (opts->type == OPTION_NUMBER) {
+ fputs(opt_num_buff, outfile);
+ pos += opt_num_size;
+ }
if ((opts->flags & PARSE_OPT_LITERAL_ARGHELP) ||
!(opts->flags & PARSE_OPT_NOARG))
--
1.8.1.1.368.g6034fad.dirty
^ permalink raw reply
* [PATCH v2 1/2] Get correct column with for options in command usage
From: Jiang Xin @ 2013-02-05 16:16 UTC (permalink / raw)
To: Junio C Hamano, Duy Nguyen; +Cc: Git List, Jiang Xin
In-Reply-To: <20130205121552.GA16601@lanh>
Command usage would not align well if command options are translated,
especially to CJK. Call utf8_strwidth in function usage_argh, so that
the caller will get correct column width.
Signed-off-by: Jiang Xin <worldhello.net@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
parse-options.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/parse-options.c b/parse-options.c
index 67e98..cd029f 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -3,6 +3,7 @@
#include "cache.h"
#include "commit.h"
#include "color.h"
+#include "utf8.h"
static int parse_options_usage(struct parse_opt_ctx_t *ctx,
const char * const *usagestr,
@@ -473,7 +474,7 @@ int parse_options(int argc, const char **argv, const char *prefix,
static int usage_argh(const struct option *opts, FILE *outfile)
{
- const char *s;
+ const char *s, *p;
int literal = (opts->flags & PARSE_OPT_LITERAL_ARGHELP) || !opts->argh;
if (opts->flags & PARSE_OPT_OPTARG)
if (opts->long_name)
@@ -482,7 +483,10 @@ static int usage_argh(const struct option *opts, FILE *outfile)
s = literal ? "[%s]" : "[<%s>]";
else
s = literal ? " %s" : " <%s>";
- return fprintf(outfile, s, opts->argh ? _(opts->argh) : _("..."));
+ p = opts->argh ? _(opts->argh) : _("...");
+ fprintf(outfile, s, p);
+ /* Remove extra 2 chars ("%s" in s) to get column width of utf8 string */
+ return utf8_strwidth(p) + strlen(s) - 2;
}
#define USAGE_OPTS_WIDTH 24
--
1.8.1.1.368.g6034fad.dirty
^ permalink raw reply
* Re: [PATCH] Add contrib/credentials/netrc with GPG support, try #2
From: Junio C Hamano @ 2013-02-05 16:18 UTC (permalink / raw)
To: Ted Zlatanov; +Cc: Jeff King, git
In-Reply-To: <7v7gmmvj5p.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> I thought I've given a more concrete outline than "I'll read
> Net::Netrc and do whatever I think it does" in a separate message.
And it turns out that the message was sitting in my outbox. Sorry.
I just told the outbox to send it out.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox