* [PATCH 5/6] Git.pm: teach "ident" to query explicitness
From: Jeff King @ 2012-11-13 16:53 UTC (permalink / raw)
To: Felipe Contreras; +Cc: git, Thomas Rast, Junio C Hamano, Jonathan Nieder
In-Reply-To: <20121113164845.GD20361@sigill.intra.peff.net>
"git var" recently learned to report on whether an ident we
fetch from it was configured explicitly or implicitly. Let's
make that information available to callers of the ident
function.
Because evaluating "ident" in an array versus scalar context
already has a meaning, we cannot return our extra value in a
backwards compatible way. Instead, we require the caller to
add an extra "explicit" flag to request the information.
The ident_person function, on the other hand, always returns
a scalar, so we are free to overload it in an array context.
Signed-off-by: Jeff King <peff@peff.net>
---
perl/Git.pm | 28 ++++++++++++++++++++--------
1 file changed, 20 insertions(+), 8 deletions(-)
diff --git a/perl/Git.pm b/perl/Git.pm
index 497f420..1994ec1 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -737,7 +737,7 @@ sub remote_refs {
}
-=item ident ( TYPE | IDENTSTR )
+=item ident ( TYPE | IDENTSTR [, options] )
=item ident_person ( TYPE | IDENTSTR | IDENTARRAY )
@@ -750,6 +750,10 @@ and either returns it as a scalar string or as an array with the fields parsed.
Alternatively, it can take a prepared ident string (e.g. from the commit
object) and just parse it.
+If the C<explicit> option is set to 1, the returned array will contain an
+additional boolean specifying whether the ident was configure explicitly by the
+user.
+
C<ident_person> returns the person part of the ident - name and email;
it can take the same arguments as C<ident> or the array returned by C<ident>.
@@ -763,17 +767,22 @@ The synopsis is like:
=cut
sub ident {
- my ($self, $type) = _maybe_self(@_);
- my $identstr;
+ my ($self, $type, %options) = _maybe_self(@_);
+ my ($identstr, $explicit);
if (lc $type eq lc 'committer' or lc $type eq lc 'author') {
- my @cmd = ('var', 'GIT_'.uc($type).'_IDENT');
+ my $uc = uc($type);
+ my @cmd = ('var', "GIT_${uc}_IDENT", "GIT_${uc}_EXPLICIT");
unshift @cmd, $self if $self;
- $identstr = command_oneline(@cmd);
+ ($identstr, $explicit) = command(@cmd);
} else {
$identstr = $type;
}
if (wantarray) {
- return $identstr =~ /^(.*) <(.*)> (\d+ [+-]\d{4})$/;
+ my @ret = $identstr =~ /^(.*) <(.*)> (\d+ [+-]\d{4})$/;
+ if ($options{explicit} && defined $explicit) {
+ push @ret, $explicit if defined $explicit;
+ }
+ return @ret;
} else {
return $identstr;
}
@@ -781,8 +790,11 @@ sub ident {
sub ident_person {
my ($self, @ident) = _maybe_self(@_);
- $#ident == 0 and @ident = $self ? $self->ident($ident[0]) : ident($ident[0]);
- return "$ident[0] <$ident[1]>";
+ $#ident == 0 and @ident = $self ?
+ $self->ident($ident[0], explicit => 1) :
+ ident($ident[0], explicit => 1);
+ my $ret = "$ident[0] <$ident[1]>";
+ return wantarray ? ($ret, @ident[3]) : $ret;
}
--
1.8.0.207.gdf2154c
^ permalink raw reply related
* [PATCH 6/6] send-email: do not prompt for explicit repo ident
From: Jeff King @ 2012-11-13 16:53 UTC (permalink / raw)
To: Felipe Contreras; +Cc: git, Thomas Rast, Junio C Hamano, Jonathan Nieder
In-Reply-To: <20121113164845.GD20361@sigill.intra.peff.net>
If git-send-email is configured with sendemail.from, we will
not prompt the user for the "From" address of the emails.
If it is not configured, we prompt the user, but provide the
repo author or committer as a default. Even though we
probably have a sensible value for the default, the prompt
is a safety check in case git generated an incorrect
implicit ident string.
Now that Git.pm will tell us whether the ident is implicit or
explicit, we can stop prompting in the explicit case, saving
most users from having to see the prompt at all.
The test scripts need to be adjusted to not expect a prompt
for the sender, since they always have the author explicitly
defined in the environment. Unfortunately, we cannot
reliably test that prompting still happens in the implicit
case, as send-email will produce inconsistent results
depending on the machine config (if we cannot find a FQDN,
"git var" will barf, causing us to exit early; if we can,
send-email will continue but prompt).
Signed-off-by: Jeff King <peff@peff.net>
---
git-send-email.perl | 22 +++++++++++++---------
t/t9001-send-email.sh | 5 ++---
2 files changed, 15 insertions(+), 12 deletions(-)
diff --git a/git-send-email.perl b/git-send-email.perl
index 5a7c29d..0c49b32 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -436,9 +436,8 @@ if (0) {
}
}
-my ($repoauthor, $repocommitter);
-($repoauthor) = Git::ident_person(@repo, 'author');
-($repocommitter) = Git::ident_person(@repo, 'committer');
+my ($repoauthor, $author_explicit) = Git::ident_person(@repo, 'author');
+my ($repocommitter, $committer_explicit) = Git::ident_person(@repo, 'committer');
# Verify the user input
@@ -755,12 +754,17 @@ if (!$force) {
my $prompting = 0;
if (!defined $sender) {
- $sender = $repoauthor || $repocommitter || '';
- $sender = ask("Who should the emails appear to be from? [$sender] ",
- default => $sender,
- valid_re => qr/\@.*\./, confirm_only => 1);
- print "Emails will be sent from: ", $sender, "\n";
- $prompting++;
+ ($sender, my $explicit) =
+ defined $repoauthor ? ($repoauthor, $author_explicit) :
+ defined $repocommitter ? ($repocommitter, $committer_explicit) :
+ ('', 0);
+ if (!$explicit) {
+ $sender = ask("Who should the emails appear to be from? [$sender] ",
+ default => $sender,
+ valid_re => qr/\@.*\./, confirm_only => 1);
+ print "Emails will be sent from: ", $sender, "\n";
+ $prompting++;
+ }
}
if (!@initial_to && !defined $to_cmd) {
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 6c6af7d..c5d66cf 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -191,14 +191,13 @@ test_expect_success $PREREQ 'Show all headers' '
test_expect_success $PREREQ 'Prompting works' '
clean_fake_sendmail &&
- (echo "Example <from@example.com>"
- echo "to@example.com"
+ (echo "to@example.com"
echo ""
) | GIT_SEND_EMAIL_NOTTY=1 git send-email \
--smtp-server="$(pwd)/fake.sendmail" \
$patches \
2>errors &&
- grep "^From: Example <from@example.com>\$" msgtxt1 &&
+ grep "^From: A U Thor <author@example.com>\$" msgtxt1 &&
grep "^To: to@example.com\$" msgtxt1
'
--
1.8.0.207.gdf2154c
^ permalink raw reply related
* [PATCH 4/6] var: provide explicit/implicit ident information
From: Jeff King @ 2012-11-13 16:53 UTC (permalink / raw)
To: Felipe Contreras; +Cc: git, Thomas Rast, Junio C Hamano, Jonathan Nieder
In-Reply-To: <20121113164845.GD20361@sigill.intra.peff.net>
Internally, we keep track of whether the author or committer
ident information was provided by the user, or whether it
was implicitly determined by the system. However, there is
currently no way for external programs or scripts to get
this information without re-implementing the ident logic
themselves. Let's provide a way for them to do so.
Signed-off-by: Jeff King <peff@peff.net>
---
Documentation/git-var.txt | 8 ++++++++
builtin/var.c | 27 +++++++++++++++++++++++++++
t/t0007-git-var.sh | 36 ++++++++++++++++++++++++++++++++++++
3 files changed, 71 insertions(+)
diff --git a/Documentation/git-var.txt b/Documentation/git-var.txt
index 53abba5..963b8d4 100644
--- a/Documentation/git-var.txt
+++ b/Documentation/git-var.txt
@@ -39,9 +39,17 @@ VARIABLES
GIT_AUTHOR_IDENT::
The author of a piece of code.
+GIT_AUTHOR_EXPLICIT::
+ Outputs "1" if the author identity was provided explicitly by the
+ user (in the config or environment), or "0" if it was determined
+ implicitly from the system.
+
GIT_COMMITTER_IDENT::
The person who put a piece of code into git.
+GIT_COMMITTER_EXPLICIT::
+ Like GIT_AUTHOR_EXPLICIT, but for the committer ident.
+
GIT_EDITOR::
Text editor for use by git commands. The value is meant to be
interpreted by the shell when it is used. Examples: `~/bin/vi`,
diff --git a/builtin/var.c b/builtin/var.c
index 49b48f5..ce28101 100644
--- a/builtin/var.c
+++ b/builtin/var.c
@@ -26,13 +26,40 @@ static const char *pager(int flag)
return pgm;
}
+static const char *explicit_ident(const char * (*get_ident)(int),
+ int (*check_ident)(void))
+{
+ /*
+ * Prime the "explicit" flag by getting the identity.
+ * We do not want to be strict here, because we would not want
+ * to die on bogus implicit values, but instead just report that they
+ * are not explicit.
+ */
+ get_ident(0);
+ return check_ident() ? "1" : "0";
+}
+
+static const char *committer_explicit(int flag)
+{
+ return explicit_ident(git_committer_info,
+ committer_ident_sufficiently_given);
+}
+
+static const char *author_explicit(int flag)
+{
+ return explicit_ident(git_author_info,
+ author_ident_sufficiently_given);
+}
+
struct git_var {
const char *name;
const char *(*read)(int);
};
static struct git_var git_vars[] = {
{ "GIT_COMMITTER_IDENT", git_committer_info },
+ { "GIT_COMMITTER_EXPLICIT", committer_explicit },
{ "GIT_AUTHOR_IDENT", git_author_info },
+ { "GIT_AUTHOR_EXPLICIT", author_explicit },
{ "GIT_EDITOR", editor },
{ "GIT_PAGER", pager },
{ "", NULL },
diff --git a/t/t0007-git-var.sh b/t/t0007-git-var.sh
index 45a5f66..66b9810 100755
--- a/t/t0007-git-var.sh
+++ b/t/t0007-git-var.sh
@@ -26,4 +26,40 @@ test_expect_success 'git var can show multiple values' '
test_cmp expect actual
'
+for type in AUTHOR COMMITTER; do
+ test_expect_success "$type ident can detect implicit values" "
+ echo 0 >expect &&
+ (
+ sane_unset GIT_${type}_NAME &&
+ sane_unset GIT_${type}_EMAIL &&
+ sane_unset EMAIL &&
+ git var GIT_${type}_EXPLICIT >actual
+ ) &&
+ test_cmp expect actual
+ "
+
+ test_expect_success "$type ident is explicit via environment" "
+ echo 1 >expect &&
+ (
+ GIT_${type}_NAME='A Name' &&
+ export GIT_${type}_NAME &&
+ GIT_${type}_EMAIL='name@example.com' &&
+ export GIT_${type}_EMAIL &&
+ git var GIT_${type}_EXPLICIT >actual
+ ) &&
+ test_cmp expect actual
+ "
+
+ test_expect_success "$type ident is explicit via config" "
+ echo 1 >expect &&
+ test_config user.name 'A Name' &&
+ test_config user.email 'name@example.com' &&
+ (
+ sane_unset GIT_${type}_NAME &&
+ sane_unset GIT_${type}_EMAIL &&
+ git var GIT_${type}_EXPLICIT >actual
+ )
+ "
+done
+
test_done
--
1.8.0.207.gdf2154c
^ permalink raw reply related
* [PATCH 3/6] var: accept multiple variables on the command line
From: Jeff King @ 2012-11-13 16:52 UTC (permalink / raw)
To: Felipe Contreras; +Cc: git, Thomas Rast, Junio C Hamano, Jonathan Nieder
In-Reply-To: <20121113164845.GD20361@sigill.intra.peff.net>
Git-var currently only accepts a single value to print. This
is inefficient if the caller is interested in finding
multiple values, as they must invoke git-var multiple times.
This patch lets callers specify multiple variables, and
prints one per line.
Signed-off-by: Jeff King <peff@peff.net>
---
This will later let us get the "explicit" flag for free.
Documentation/git-var.txt | 9 +++++++--
builtin/var.c | 13 +++++++------
t/t0007-git-var.sh | 29 +++++++++++++++++++++++++++++
3 files changed, 43 insertions(+), 8 deletions(-)
create mode 100755 t/t0007-git-var.sh
diff --git a/Documentation/git-var.txt b/Documentation/git-var.txt
index 67edf58..53abba5 100644
--- a/Documentation/git-var.txt
+++ b/Documentation/git-var.txt
@@ -9,11 +9,16 @@ git-var - Show a git logical variable
SYNOPSIS
--------
[verse]
-'git var' ( -l | <variable> )
+'git var' ( -l | <variable>... )
DESCRIPTION
-----------
-Prints a git logical variable.
+Prints one or more git logical variables, separated by newlines.
+
+Note that some variables may contain newlines themselves (e.g.,
+`GIT_EDITOR`), and it is therefore possible to receive ambiguous output
+when requesting multiple variables. This can be mitigated by putting any
+such variables at the end of the list.
OPTIONS
-------
diff --git a/builtin/var.c b/builtin/var.c
index aedbb53..49b48f5 100644
--- a/builtin/var.c
+++ b/builtin/var.c
@@ -73,8 +73,7 @@ static int show_config(const char *var, const char *value, void *cb)
int cmd_var(int argc, const char **argv, const char *prefix)
{
- const char *val = NULL;
- if (argc != 2)
+ if (argc < 2)
usage(var_usage);
if (strcmp(argv[1], "-l") == 0) {
@@ -83,11 +82,13 @@ int cmd_var(int argc, const char **argv, const char *prefix)
return 0;
}
git_config(git_default_config, NULL);
- val = read_var(argv[1]);
- if (!val)
- usage(var_usage);
- printf("%s\n", val);
+ while (*++argv) {
+ const char *val = read_var(*argv);
+ if (!val)
+ usage(var_usage);
+ printf("%s\n", val);
+ }
return 0;
}
diff --git a/t/t0007-git-var.sh b/t/t0007-git-var.sh
new file mode 100755
index 0000000..45a5f66
--- /dev/null
+++ b/t/t0007-git-var.sh
@@ -0,0 +1,29 @@
+#!/bin/sh
+
+test_description='basic sanity checks for git var'
+. ./test-lib.sh
+
+test_expect_success 'get GIT_AUTHOR_IDENT' '
+ test_tick &&
+ echo "A U Thor <author@example.com> 1112911993 -0700" >expect &&
+ git var GIT_AUTHOR_IDENT >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'get GIT_COMMITTER_IDENT' '
+ test_tick &&
+ echo "C O Mitter <committer@example.com> 1112912053 -0700" >expect &&
+ git var GIT_COMMITTER_IDENT >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'git var can show multiple values' '
+ cat >expect <<-\EOF &&
+ A U Thor <author@example.com> 1112912053 -0700
+ C O Mitter <committer@example.com> 1112912053 -0700
+ EOF
+ git var GIT_AUTHOR_IDENT GIT_COMMITTER_IDENT >actual &&
+ test_cmp expect actual
+'
+
+test_done
--
1.8.0.207.gdf2154c
^ permalink raw reply related
* [PATCH 2/6] ident: keep separate "explicit" flags for author and committer
From: Jeff King @ 2012-11-13 16:52 UTC (permalink / raw)
To: Felipe Contreras; +Cc: git, Thomas Rast, Junio C Hamano, Jonathan Nieder
In-Reply-To: <20121113164845.GD20361@sigill.intra.peff.net>
We keep track of whether the user ident was given to us
explicitly, or if we guessed at it from system parameters
like username and hostname. However, we kept only a single
variable. This covers the common cases (because the author
and committer will usually come from the same explicit
source), but can miss two cases:
1. GIT_COMMITTER_* is set explicitly, but we fallback for
GIT_AUTHOR. We claim the ident is explicit, even though
the author is not.
2. GIT_AUTHOR_* is set and we ask for author ident, but
not committer ident. We will claim the ident is
implicit, even though it is explicit.
This patch uses two variables instead of one, updates both
when we set the "fallback" values, and updates them
individually when we read from the environment.
Rather than keep user_ident_sufficiently_given as a
compatibility wrapper, we update the only two callers to
check the committer_ident, which matches their intent and
what was happening already.
Signed-off-by: Jeff King <peff@peff.net>
---
Case 1 made me initially think might need to check
author_ident_sufficiently_given when deciding whether to show the
"Author:" line during commit. But I don't think it is necessary, as we
already check !strcmp(author, committer); if the committer is explicit
and the author is identical, even if it is implicit, there is no point
in telling the user.
builtin/commit.c | 4 ++--
cache.h | 3 ++-
ident.c | 32 +++++++++++++++++++++++++-------
3 files changed, 29 insertions(+), 10 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index 1dd2ec5..d6dd3df 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -755,7 +755,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
ident_shown++ ? "" : "\n",
author_ident->buf);
- if (!user_ident_sufficiently_given())
+ if (!committer_ident_sufficiently_given())
status_printf_ln(s, GIT_COLOR_NORMAL,
_("%s"
"Committer: %s"),
@@ -1265,7 +1265,7 @@ static void print_summary(const char *prefix, const unsigned char *sha1,
strbuf_addstr(&format, "\n Author: ");
strbuf_addbuf_percentquote(&format, &author_ident);
}
- if (!user_ident_sufficiently_given()) {
+ if (!committer_ident_sufficiently_given()) {
strbuf_addstr(&format, "\n Committer: ");
strbuf_addbuf_percentquote(&format, &committer_ident);
if (advice_implicit_identity) {
diff --git a/cache.h b/cache.h
index 50d9eea..18fdd18 100644
--- a/cache.h
+++ b/cache.h
@@ -1149,7 +1149,8 @@ struct config_include_data {
#define CONFIG_INCLUDE_INIT { 0 }
extern int git_config_include(const char *name, const char *value, void *data);
-extern int user_ident_sufficiently_given(void);
+extern int committer_ident_sufficiently_given(void);
+extern int author_ident_sufficiently_given(void);
extern const char *git_commit_encoding;
extern const char *git_log_output_encoding;
diff --git a/ident.c b/ident.c
index 733d69d..ac9672f 100644
--- a/ident.c
+++ b/ident.c
@@ -14,7 +14,8 @@ static char git_default_date[50];
#define IDENT_NAME_GIVEN 01
#define IDENT_MAIL_GIVEN 02
#define IDENT_ALL_GIVEN (IDENT_NAME_GIVEN|IDENT_MAIL_GIVEN)
-static int user_ident_explicitly_given;
+static int committer_ident_explicitly_given;
+static int author_ident_explicitly_given;
#ifdef NO_GECOS_IN_PWENT
#define get_gecos(ignored) "&"
@@ -113,7 +114,8 @@ const char *ident_default_email(void)
if (email && email[0]) {
strbuf_addstr(&git_default_email, email);
- user_ident_explicitly_given |= IDENT_MAIL_GIVEN;
+ committer_ident_explicitly_given |= IDENT_MAIL_GIVEN;
+ author_ident_explicitly_given |= IDENT_MAIL_GIVEN;
} else
copy_email(xgetpwuid_self(), &git_default_email);
strbuf_trim(&git_default_email);
@@ -331,6 +333,10 @@ const char *fmt_name(const char *name, const char *email)
const char *git_author_info(int flag)
{
+ if (getenv("GIT_AUTHOR_NAME"))
+ author_ident_explicitly_given |= IDENT_NAME_GIVEN;
+ if (getenv("GIT_AUTHOR_EMAIL"))
+ author_ident_explicitly_given |= IDENT_MAIL_GIVEN;
return fmt_ident(getenv("GIT_AUTHOR_NAME"),
getenv("GIT_AUTHOR_EMAIL"),
getenv("GIT_AUTHOR_DATE"),
@@ -340,16 +346,16 @@ const char *git_author_info(int flag)
const char *git_committer_info(int flag)
{
if (getenv("GIT_COMMITTER_NAME"))
- user_ident_explicitly_given |= IDENT_NAME_GIVEN;
+ committer_ident_explicitly_given |= IDENT_NAME_GIVEN;
if (getenv("GIT_COMMITTER_EMAIL"))
- user_ident_explicitly_given |= IDENT_MAIL_GIVEN;
+ committer_ident_explicitly_given |= IDENT_MAIL_GIVEN;
return fmt_ident(getenv("GIT_COMMITTER_NAME"),
getenv("GIT_COMMITTER_EMAIL"),
getenv("GIT_COMMITTER_DATE"),
flag);
}
-int user_ident_sufficiently_given(void)
+static int ident_is_sufficient(int user_ident_explicitly_given)
{
#ifndef WINDOWS
return (user_ident_explicitly_given & IDENT_MAIL_GIVEN);
@@ -358,6 +364,16 @@ int user_ident_sufficiently_given(void)
#endif
}
+int committer_ident_sufficiently_given(void)
+{
+ return ident_is_sufficient(committer_ident_explicitly_given);
+}
+
+int author_ident_sufficiently_given(void)
+{
+ return ident_is_sufficient(author_ident_explicitly_given);
+}
+
int git_ident_config(const char *var, const char *value, void *data)
{
if (!strcmp(var, "user.name")) {
@@ -365,7 +381,8 @@ int git_ident_config(const char *var, const char *value, void *data)
return config_error_nonbool(var);
strbuf_reset(&git_default_name);
strbuf_addstr(&git_default_name, value);
- user_ident_explicitly_given |= IDENT_NAME_GIVEN;
+ committer_ident_explicitly_given |= IDENT_NAME_GIVEN;
+ author_ident_explicitly_given |= IDENT_NAME_GIVEN;
return 0;
}
@@ -374,7 +391,8 @@ int git_ident_config(const char *var, const char *value, void *data)
return config_error_nonbool(var);
strbuf_reset(&git_default_email);
strbuf_addstr(&git_default_email, value);
- user_ident_explicitly_given |= IDENT_MAIL_GIVEN;
+ committer_ident_explicitly_given |= IDENT_MAIL_GIVEN;
+ author_ident_explicitly_given |= IDENT_MAIL_GIVEN;
return 0;
}
--
1.8.0.207.gdf2154c
^ permalink raw reply related
* [PATCH 1/6] ident: make user_ident_explicitly_given private
From: Jeff King @ 2012-11-13 16:49 UTC (permalink / raw)
To: Felipe Contreras; +Cc: git, Thomas Rast, Junio C Hamano, Jonathan Nieder
In-Reply-To: <20121113164845.GD20361@sigill.intra.peff.net>
There are no users of this global variable, as queriers
go through the user_ident_sufficiently_given accessor.
Let's make it private, which will enable further
refactoring.
Signed-off-by: Jeff King <peff@peff.net>
---
cache.h | 4 ----
ident.c | 6 +++++-
2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/cache.h b/cache.h
index dbd8018..50d9eea 100644
--- a/cache.h
+++ b/cache.h
@@ -1149,10 +1149,6 @@ struct config_include_data {
#define CONFIG_INCLUDE_INIT { 0 }
extern int git_config_include(const char *name, const char *value, void *data);
-#define IDENT_NAME_GIVEN 01
-#define IDENT_MAIL_GIVEN 02
-#define IDENT_ALL_GIVEN (IDENT_NAME_GIVEN|IDENT_MAIL_GIVEN)
-extern int user_ident_explicitly_given;
extern int user_ident_sufficiently_given(void);
extern const char *git_commit_encoding;
diff --git a/ident.c b/ident.c
index a4bf206..733d69d 100644
--- a/ident.c
+++ b/ident.c
@@ -10,7 +10,11 @@
static struct strbuf git_default_name = STRBUF_INIT;
static struct strbuf git_default_email = STRBUF_INIT;
static char git_default_date[50];
-int user_ident_explicitly_given;
+
+#define IDENT_NAME_GIVEN 01
+#define IDENT_MAIL_GIVEN 02
+#define IDENT_ALL_GIVEN (IDENT_NAME_GIVEN|IDENT_MAIL_GIVEN)
+static int user_ident_explicitly_given;
#ifdef NO_GECOS_IN_PWENT
#define get_gecos(ignored) "&"
--
1.8.0.207.gdf2154c
^ permalink raw reply related
* Re: [PATCH] send-email: add proper default sender
From: Jeff King @ 2012-11-13 16:48 UTC (permalink / raw)
To: Felipe Contreras; +Cc: git, Thomas Rast, Junio C Hamano, Jonathan Nieder
In-Reply-To: <CAMP44s1NdK9mw3Qz_sk1Zvg0gS6E+V0BuCfDutz8-8YD_App=Q@mail.gmail.com>
On Tue, Nov 13, 2012 at 10:06:26AM +0100, Felipe Contreras wrote:
> > Those people would also not be using a new version of git-send-email,
> > and it will always prompt. I thought we were talking about what
> > send-email should do in future versions. Namely, loosening that safety
> > valve (the prompt) because it is inconvenient, but tightening the checks
> > so that losing the safety valve is not a problem.
>
> Yeah, but all I'm saying is that the issue happens, you seemed to
> suggest that it doesn't.
What is "it"?
If it is bad guesses like "user@host.(none)" on unconfigured systems,
then I did not suggest it doesn't happen. I said that it happened with
older versions of git, but that it has now been fixed. Of course it will
continue to happen for people on older versions of git until they
upgrade. I cannot go back in time and fix released versions.
If it is guessing at all to end up with "user@host.domain" for a host
that does not receive mail, yes, that happens, and I have been very
clear that it does. The safety valve is showing the ident and a warning
to the user during the commit process.
I do not see any point in discussing the former when considering future
changes to git. It is already disallowed by current versions of git, and
we are just waiting for the whole world to upgrade to a fixed version.
I can see the argument for tightening the check to disallow the latter.
But it would also hurt real people who are relying on the feature.
Perhaps it would make sense to adopt the implicit_ident_advice in other
code paths besides "git commit" (e.g., via "git var).
> I think you are the one that is not understanding what I'm saying. But
> I don't think it matters.
>
> This is what I'm saying; the current situation with 'git commit' is
> not OK, _both_ 'git commit' and 'git send-email' should change.
Perhaps I am being dense, but this is the first time it became apparent
to me that you are arguing for a change in "git commit".
> Indeed I would, but there's other people that would benefit from this
> patch. I'm sure I'm not the only person that doesn't have
> sendmail.from configured, but does have user.name/user.email, and is
> constantly typing enter.
>
> And the difference is that I'm _real_, the hypothetical user that
> sends patches with GIT_AUTHOR_NAME/EMAIL is not. I would be convinced
> otherwise if some evidence was presented that such a user is real
> though.
Sadly we cannot poll the configuration of every user, nor expect them
all to pay attention to this discussion on the list. So we cannot take
the absence of comment from such users as evidence that they do not
exist. Instead we must use our judgement about what is being changed,
and we tend to err on the side of keeping the status quo, since that is
what the silent majority is busy _not_ complaining about.
We use the same judgement on the other side, too. Right now your
evidence is "1 user wants this, 0 users do not". But we can guess that
there are other people who would like the intent of your patch, but did
not care enough or are not active enough on the list to write the patch
themselves or comment on this thread.
It is also very easy to me accept the status quo when it is not in
fundamental conflict with the proposed improvement, but simply a matter
of implementation (which it is the case for send-email stopping
prompting in some cases, though it is not for changing the behavior of
git-commit).
> And to balance you need to *measure*, and that means taking into
> consideration who actually uses the features, if there's any. And it
> looks to me this is a feature nobody uses.
You said "measure" and then "it looks to me like". What did you measure?
Did you consider systematic bias in your measurement, like the fact that
people who are using the feature have no reason to come on the list and
announce it?
> But listen closely to what you said:
>
> > I actually think it would make more sense to drop the prompt entirely and just die when the user has not given us a usable ident.
>
> Suppose somebody has a full name, and a fully qualified domain name,
> and he can receive mails to it directly. Such a user would not need a
> git configuration, and would not need $EMAIL, or anything.
>
> Currently 'git send-email' will throw 'Felipe Contreras
> <felipec@felipec.org>' which would actually work, but is not explicit.
>
> You are suggesting to break that use-case. You are introducing a
> regression. And this case is realistic, unlike the
> GIT_AUTHOR_NAME/EMAIL. Isn't it?
Yes, dying would be a regression, in that you would have to configure
your name via the environment and re-run rather than type it at the
prompt. You raise a good point that for people who _could_ take the
implicit default, hitting "enter" is working fine now, and we would lose
that. I'd be fine with also just continuing to prompt in the implicit
case.
But that is a much smaller issue to me than having send-email fail to
respect environment variables and silently use user.*, which is what
started this whole discussion. And I agree it is worth considering as a
regression we should avoid.
> I prefer to concentrate on real issues, but that's just me.
To be honest, I am confused at this point what you actually want. Do you
think we should take your original patch? I think its regression is too
great. And I am not sure if you agree or not. You seem to be arguing
that the regression is not important, yet you simultaneously argue that
we should be making all ident more strict, which would mean that we
should indeed take my suggestion and use "git var" instead of preferring
the config.
> > As for whether they exist, what data do you have?
>
> What data do _you_ have?
>
> When there's no evidence either way, the rational response is to don't
> believe. That's the default position.
This is not religion. It is a software project. In the absence of data,
the sane thing is not to break existing users. The burden is on you to
argue that there is no such breakage.
> > Are you aware that the
> > test suite, for example, relies on setting GIT_AUTHOR_NAME but not
> > having any user.* config?
>
> What tests? My patch doesn't seem to break anything there:
> % make -C t t9001-send-email.sh
> # passed all 96 test(s)
My point was that there is at least one known setup that uses only
environment variables and not a config file, and that the rest of git is
intended to work with that. It is not a test failure, but t9001.18
reveals the fact that your change does not handle this situation; we
continue to prompt under the test suite's configuration. In the proper
fix, the test needs to be adjusted.
You don't see any test failures with your patch because we do not cover
the case that you regress (GIT_AUTHOR_EMAIL and user.email both set).
> > When somebody comes on the list and asks why
> > every git program in the entire system respects GIT_* environment
> > variables as an override to user.* configuration _except_ for
> > send-email, what should I say?
>
> The same thing you say when somebody comes reporting a bug: "yeah, we
> should probably fix that".
It is a larger hassle for both the developer and the user to fix the bug
after the fact, ship a new version, and then have the user upgrade their
git. Why not just not introduce the bug in the first place?
> It's all about proportion. Is it possible that we all are going to die
> tomorrow because of an asteroid? Sure... but what's the point of
> worrying about it if it's not likely?
If you want to talk about risk assessment, then the right computation is
to compare the cost of fixing it now versus the cost of fixing it later
times the probability of it happening. The development cost is probably
a little higher later (because we have to refresh ourselves on the
issue), but the deployment cost is much higher (e.g., users whose
distros ship a broken version, or who have IT policy that does not let
them upgrade git as soon as the bug-fix ships).
> >> That's right, AUTHOR_IDENT would fall back to the default email and full name.
> >
> > Yeah, I find that somewhat questionable in the current behavior, and I'd
> > consider it a bug. Typically we prefer the committer ident when given a
> > choice (e.g., for writing reflog entries).
>
> Yeah, but clearly the intention of the code was to use the committer
> if the author wasn't available, which is the case here.
Yes. It would make sense to me to respect an explicit committer over an
implicit author. Though the fact that author is preferred over committer
is inconsistent with most of the rest of git. I'm undecided whether that
should be changed.
> >> What about after my change?
> >>
> >> 6.1) GIT_AUTHOR without anything else
> >>
> >> fatal: empty ident name (for <felipec@nysa.(none)>) not allowed
> >> var GIT_COMMITTER_IDENT: command returned error: 128
> >
> > Doesn't that seem like a regression? It used to work.
>
> No, this is *before* my change.
>
> I's the same bug as 5.1):
Ah, I see. We are loading both, whether or not they end up being used,
and barfing prematurely on errors. That seems like a bug.
> >> And what about your proposed change?
> >
> > Let me be clear that I sent you a "something like this" patch to try to
> > point you in the right direction. If it has a bug or is incomplete, that
> > does not mean the direction is wrong, but only that I did not spend very
> > much time on the patch.
>
> It doesn't matter, the idea was to use user_ident_sufficiently_given().
Right. Which it sounds like now agree with me on?
> > I think that respecting the usual ident lookup but disallowing implicit
> > identities (either totally, or causing them to fallback to prompting) is
> > the right direction. I agree my patch was not a complete solution. I'm
> > sorry if it led you astray in terms of implementation, but I also think
> > I've been very clear in my text about what the behavior should be.
>
> I think that is orthogonal to what I'm trying accomplish.
What is it you are trying to accomplish? Eliminating the prompt in some
cases, as in your original patch? Dealing with implicit identities may
be orthogonal to your goal, but your original patch is not sufficient,
as it reverses the precedence of config and environment variables.
So you can either:
1. Reimplement the environment variable lookup that ident.c does,
leaving implicit ident logic out completely.
2. Modify ident.c and "git var" to let send-email reuse the logic in
ident.c, but avoid dropping the prompt when an implicit ident is
used.
Doing (2) sounds a lot more maintainable to me in the long run.
> > Don't get me wrong. I think the spirit of your patch is correct, and it
> > helps some git users. But it also hurts others. And it is not that hard
> > to do it right.
>
> And I disagree, I think it hurts nobody, and I think it's hard to do it right.
I am tired of talking about this. The patch series below took me less
time to write than I have spent arguing with you. It was not that hard.
[1/6]: ident: make user_ident_explicitly_given private
[2/6]: ident: keep separate "explicit" flags for author and committer
[3/6]: var: accept multiple variables on the command line
[4/6]: var: provide explicit/implicit ident information
[5/6]: Git.pm: teach "ident" to query explicitness
[6/6]: send-email: do not prompt for explicit repo ident
> Fixing a regression that nobody would notice is not my itch either,
Sorry, but it is part of your itch. The git project is at state A. With
your patch, we are at a regressed state B. If you then fix it, we are at
state C. From your perspective, it may be "I do not want to make the fix
to go from B to C". But from the project's perspective, it is "we do not
want to go from state A to state B; state C would be acceptable". So
from the perspective of people who do not care about your feature but do
care about the regression in state B, it is inextricably linked to your
itch.
> yet the patch I sent above does it, and it even fixes 'git commit'
> (IMO). But it's also not good enough.
I do not necessarily agree on "git commit". Moreover, I feel like it is
a separate issue. My series above _just_ implements the "do not prompt
when explicit" behavior. It does not deal with git-commit at all, nor
does it address the author/committer fallback questions. Those can
easily go on top.
-Peff
^ permalink raw reply
* Re: [PATCH] update-index/diff-index: use core.preloadindex to improve performance
From: Junio C Hamano @ 2012-11-13 16:46 UTC (permalink / raw)
To: karsten.blees; +Cc: Jeff King, git, msysgit, pro-logic
In-Reply-To: <OF27D0F811.18A373FB-ONC1257AB5.0055809D-C1257AB5.0055B47D@dcon.de>
karsten.blees@dcon.de writes:
> Jeff King <peff@peff.net> wrote on 02.11.2012 16:38:00:
>
>> On Fri, Nov 02, 2012 at 11:26:16AM -0400, Jeff King wrote:
>>
>> > Still, I don't think we need to worry about performance regressions,
>> > because people who don't have a setup suitable for it will not turn on
>> > core.preloadindex in the first place. And if they have it on, the more
>> > places we use it, probably the better.
>>
>> BTW, your patch was badly damaged in transit (wrapped, and tabs
>> converted to spaces). I was able to fix it up, but please check your
>> mailer's settings.
>>
>
> Yes, I feared as much, that's why I included the pull URL (the company MTA
> only accepts outbound mail from Notes clients, sorry).
>
> Is there a policy for people with broken mailers (send patch as
> attachment, add pull URL more prominently, don't include plaintext patch
> at all...)?
If anything, "fix your mailer" probably is the policy you are
looking for, I think.
--
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.
You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en
^ permalink raw reply
* Re: checkout from neighbour branch undeletes a path?
From: Junio C Hamano @ 2012-11-13 16:43 UTC (permalink / raw)
To: Peter Vereshagin; +Cc: git
In-Reply-To: <20121113152341.GC6561@external.screwed.box>
Peter Vereshagin <peter@vereshagin.org> writes:
> Am wondering if 'checkout branch path' undeletes the files?
"git checkout branch path" (by the way, "branch" does not have to be
a branch name; any commit object name would do, like "git checkout
HEAD^^ hello.c") is a way to check out named path(s) out of the
named commit.
If the commit "branch" has "path" in it, its contents are checked
out to "path" in your current working tree (and the entry in the
index updated to match it).
If you happen to have removed "path" in your current working tree
before running that command, it might look as if there is some
undelete going on, but that is a wrong way to look at things. The
version of "path" in the "branch" may or may not be similar to what
you have removed earlier.
^ permalink raw reply
* Re: Commit message problem of reverting multiple commits
From: Junio C Hamano @ 2012-11-13 16:34 UTC (permalink / raw)
To: 乙酸鋰; +Cc: git
In-Reply-To: <CAHtLG6Qn68TFVnd_8LSf6OMqHZduAFgk0Hd46E3vKgFHCjpksQ@mail.gmail.com>
乙酸鋰 <ch3cooli@gmail.com> writes:
> I ran git 1.8.0 command line
>
> git revert --no-commit rev1 rev2
>
> I see a prepared commit message like
>
> Revert "<description from one commit>"
> This reverts commit <SHA1 of one commit>.
>
>
> The actual revert content is correct - it is all the relevant commits
> that were selected. I expect the message to reflect this:
>
> Revert "<description from commit1>", "<description from commit2>"
> This reverts commits <SHA1 of commit1>, <SHA1 of commit2>.
Hrmph. I actually think the revert-content is not correct.
I think the command should not take more than one commit on the
command line when --no-commit is in use in the first place (the same
thing can be said for cherry-pick). After all, "git revert rev1
rev2" is to revert rev1 and then rev2 independently, so unless that
option is spelled "--squash", the resulting history should have two
commits that reverts rev1 and rev2 independently.
^ permalink raw reply
* Re: Notes in format-patch
From: Junio C Hamano @ 2012-11-13 16:29 UTC (permalink / raw)
To: Michael J Gruber; +Cc: git, Jeff King
In-Reply-To: <50A2213B.4060505@drmicha.warpmail.net>
Michael J Gruber <git@drmicha.warpmail.net> writes:
> Michael J Gruber venit, vidit, dixit 12.11.2012 15:18:
>> 'git replace' parses the revision arguments when it creates replacements
>> (so that a sha1 can be abbreviated, e.g.) but not when deleting
>> replacements.
>>
>> Make it parse the argument to 'replace -d' in the same way.
>>
>> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
>> ---
>>
>> Notes:
>> v3 safeguards the hex buffer against reuse
>> builtin/replace.c | 16 ++++++++++------
>> t/t6050-replace.sh | 11 +++++++++++
>> 2 files changed, 21 insertions(+), 6 deletions(-)
>>
>> diff --git a/builtin/replace.c b/builtin/replace.c
>
> By the way - Junio, is that the intented outcome of "format-patch
> --notes"? I would rather put the newline between the note and the
> diffstat...
I do not mind (actually I personally would prefer to see) a blank
line between the three-dash and "Notes:", but I agree that we should
have a blank line before the diffstat block.
^ permalink raw reply
* Re: [PATCH 2/3] diff: introduce diff.submodule configuration variable
From: Junio C Hamano @ 2012-11-13 16:27 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Jeff King, Git List, Jens Lehmann
In-Reply-To: <CALkWK0nJwznx36yoAUKXRnyA+32143tBVJHnnrosz-Ht7VhwHw@mail.gmail.com>
Ramkumar Ramachandra <artagnon@gmail.com> writes:
> Jeff King wrote:
>> On Sun, Nov 11, 2012 at 10:29:05PM +0530, Ramkumar Ramachandra wrote:
>>> @@ -223,6 +238,15 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)
>>> return 0;
>>> }
>>>
>>> + if (!strcmp(var, "diff.submodule")) {
>>
>> Shouldn't this be in git_diff_ui_config so it does not affect scripts
>> calling plumbing?
>
> I honestly didn't understand the difference between
> git_diff_basic_config and git_diff_ui_config. The latter function
> calls the former function at the end of its body. Why are they two
> separate functions in the first place?
In case you meant s/didn't/don't/, git_diff_ui_config() should be
called only by human-facing Porcelain commands where their
behaviours can and should be affected by end user configuration
variables.
When a configuration variable should not affect output from plumbing
commands like diff-files, diff-index, and diff-tree, it must not be
read in git_diff_basic_config(), but in git_diff_ui_config().
The output from "git format-patch" is consumed by "git apply" that
expects "Subproject commit %s\n" with fully spelled object name, so
your configuration must not affect the output of format-patch,
either.
^ permalink raw reply
* Re: [PATCH] send-email: add proper default sender
From: Junio C Hamano @ 2012-11-13 16:13 UTC (permalink / raw)
To: Jeff King; +Cc: Felipe Contreras, git, Thomas Rast, Jonathan Nieder
In-Reply-To: <20121113074720.GA18746@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Tue, Nov 13, 2012 at 07:42:58AM +0100, Felipe Contreras wrote:
> ...
>> 5) GIT_COMMITTER
>>
>> Who should the emails appear to be from? [Felipe Contreras 2nd
>> <felipe.contreras+2@gmail.com>]
>>
>> Whoa, what happened there?
>>
>> Well:
>>
>> $sender = $repoauthor || $repocommitter || '';
>> ($repoauthor) = Git::ident_person(@repo, 'author');
>> % ./git var GIT_AUTHOR_IDENT
>> Felipe Contreras 2nd <felipe.contreras+2@gmail.com> 1352783223 +0100
>>
>> That's right, AUTHOR_IDENT would fall back to the default email and full name.
>
> Yeah, I find that somewhat questionable in the current behavior, and I'd
> consider it a bug. Typically we prefer the committer ident when given a
> choice (e.g., for writing reflog entries).
Just to make sure I follow the discussion correctly, do you mean
that the bug is that we pick a fuzzy AUTHOR when COMMITTER is
specified more concretely and we usually use COMMIITTER for this
kind of thing in the first place but send-email does not in this
case (I do not see "git var GIT_AUTHOR_IDENT" returning value from
the implicit logic as a bug in this case---just making sure).
>> 6.1) GIT_AUTHOR without anything else
>>
>> fatal: empty ident name (for <felipec@nysa.(none)>) not allowed
>> var GIT_COMMITTER_IDENT: command returned error: 128
>
> Doesn't that seem like a regression? It used to work.
I do not think we mind a change to favor GIT_COMMITTER setting over
GIT_AUTHOR to be consistent with others too much, but that indeed is
a big change. People who are used to the inconsistency that
send-email favors AUTHOR over COMMITTER will certainly find it as a
regression.
> The explicitness needs to be tied to the specific ident we grabbed.
> Probably adding a "git var GIT_AUTHOR_EXPLICIT" would be enough, or
> alternatively, adding a flag to "git var" to error out rather than
> return a non-explicit ident (this may need to adjust the error
> handling of the "git var" calls from send-email).
Yeah, something like that would be necessary for "git var" users to
make this kind of decision.
> While simultaneously breaking "git commit" for people who are happily
> using the implicit generation. I can see the appeal of doing so; I was
> tempted to suggest it when I cleaned up IDENT_STRICT a few months back.
> But do we have any data on how many people are currently using that
> feature that would be annoyed by it?
As we didn't break "git commit" when you worked on IDENT_STRICT, I
do not think we have any data on it ;-)
> ...
> It may be something I would work on myself in the future, but I have
> other things to work on at the moment, and since you are interested in
> the topic, I thought you would be a good candidate to polish it enough
> to be suitable upstream. But instead I see a lot of push-back on what I
> considered to be a fairly straightforward technical comment on a
> regression.
> ...
> But I feel like I am fighting an uphill battle just to convince you that
> regressions are bad, and that I am having to make the same points
> repeatedly. That makes me frustrated and less excited about reviewing
> your patches; and when I say "it is not my itch", that is my most polite
> way of saying "If that is going to be your attitude, then I do not feel
> like dealing with you anymore on this topic".
For a change with low benefit/cost ratio like this, the best course
of action often is to stop paying attention to the thread that has
become unproductive and venomous, and resurrect the topic after
people forgot about it and doing it right yourself ;-).
^ permalink raw reply
* Re: [BUG] gitweb: XSS vulnerability of RSS feed
From: Jakub Narębski @ 2012-11-13 15:57 UTC (permalink / raw)
To: Kevin
Cc: Drew Northup, Jeff King, glpk xypron, git, Junio C Hamano,
Jason J Pyeron CTR (US), Andreas Schwab
In-Reply-To: <CAO54GHCzeWv41Bu5By0JOzbBHGuzXV=krdDr0U=QsMBun7PF7A@mail.gmail.com>
On Tue, Nov 13, 2012 at 4:45 PM, Kevin <ikke@ikke.info> wrote:
> The problem with input filtering is that you can only filter for one
> output scenario. What if the the input is going to be output in a wiki
> like environment, or to pdf, or whatever? Then you have to unescape
> the data again, and maybe apply filtering/escaping for those
> environments.
>
> You only know how to escape data when you are going to output it, so
> then is the the best moment to escape it.
Also there are so many ways to evade XSS filtering
https://www.owasp.org/index.php/XSS_Filter_Evasion_Cheat_Sheet
If you can and should escape data (like in our case), it cannot not work;
not possible to evade it.
--
Jakub Narebski
^ permalink raw reply
* Re: [BUG] gitweb: XSS vulnerability of RSS feed
From: Kevin @ 2012-11-13 15:45 UTC (permalink / raw)
To: Drew Northup
Cc: Jeff King, glpk xypron, git, Jakub Narębski, Junio C Hamano,
Jason J Pyeron CTR (US), Andreas Schwab
In-Reply-To: <CAM9Z-nkuHj8MWLfWsvY=EqHXCUS+Pk5Ezv6m5J+cnh7cQHNc_g@mail.gmail.com>
The problem with input filtering is that you can only filter for one
output scenario. What if the the input is going to be output in a wiki
like environment, or to pdf, or whatever? Then you have to unescape
the data again, and maybe apply filtering/escaping for those
environments.
You only know how to escape data when you are going to output it, so
then is the the best moment to escape it.
^ permalink raw reply
* Re: [PATCH 2/3] diff: introduce diff.submodule configuration variable
From: Ramkumar Ramachandra @ 2012-11-13 15:45 UTC (permalink / raw)
To: Jeff King; +Cc: Git List, Jens Lehmann
In-Reply-To: <20121113053336.GA10995@sigill.intra.peff.net>
Jeff King wrote:
> On Sun, Nov 11, 2012 at 10:29:05PM +0530, Ramkumar Ramachandra wrote:
>> @@ -223,6 +238,15 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)
>> return 0;
>> }
>>
>> + if (!strcmp(var, "diff.submodule")) {
>
> Shouldn't this be in git_diff_ui_config so it does not affect scripts
> calling plumbing?
I honestly didn't understand the difference between
git_diff_basic_config and git_diff_ui_config. The latter function
calls the former function at the end of its body. Why are they two
separate functions in the first place?
Btw, I just posted a follow-up.
Ram
^ permalink raw reply
* [PATCH v4 4/4] submodule: display summary header in bold
From: Ramkumar Ramachandra @ 2012-11-13 15:42 UTC (permalink / raw)
To: Git List
In-Reply-To: <1352821367-3611-1-git-send-email-artagnon@gmail.com>
Currently, 'git diff --submodule' displays output with a bold diff
header for non-submodules. So this part is in bold:
diff --git a/file1 b/file1
index 30b2f6c..2638038 100644
--- a/file1
+++ b/file1
For submodules, the header looks like this:
Submodule submodule1 012b072..248d0fd:
Unfortunately, it's easy to miss in the output because it's not bold.
Change this.
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
diff.c | 2 +-
submodule.c | 8 ++++----
submodule.h | 2 +-
3 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/diff.c b/diff.c
index ffaed72..1065978 100644
--- a/diff.c
+++ b/diff.c
@@ -2261,7 +2261,7 @@ static void builtin_diff(const char *name_a,
const char *add = diff_get_color_opt(o, DIFF_FILE_NEW);
show_submodule_summary(o->file, one ? one->path : two->path,
one->sha1, two->sha1, two->dirty_submodule,
- del, add, reset);
+ meta, del, add, reset);
return;
}
diff --git a/submodule.c b/submodule.c
index e3e0b45..2f55436 100644
--- a/submodule.c
+++ b/submodule.c
@@ -258,7 +258,7 @@ int parse_fetch_recurse_submodules_arg(const char *opt, const char *arg)
void show_submodule_summary(FILE *f, const char *path,
unsigned char one[20], unsigned char two[20],
- unsigned dirty_submodule,
+ unsigned dirty_submodule, const char *meta,
const char *del, const char *add, const char *reset)
{
struct rev_info rev;
@@ -292,15 +292,15 @@ void show_submodule_summary(FILE *f, const char *path,
return;
}
- strbuf_addf(&sb, "Submodule %s %s..", path,
+ strbuf_addf(&sb, "%sSubmodule %s %s..", meta, path,
find_unique_abbrev(one, DEFAULT_ABBREV));
if (!fast_backward && !fast_forward)
strbuf_addch(&sb, '.');
strbuf_addf(&sb, "%s", find_unique_abbrev(two, DEFAULT_ABBREV));
if (message)
- strbuf_addf(&sb, " %s\n", message);
+ strbuf_addf(&sb, " %s%s\n", message, reset);
else
- strbuf_addf(&sb, "%s:\n", fast_backward ? " (rewind)" : "");
+ strbuf_addf(&sb, "%s:%s\n", fast_backward ? " (rewind)" : "", reset);
fwrite(sb.buf, sb.len, 1, f);
if (!message) {
diff --git a/submodule.h b/submodule.h
index f2e8271..3dc1b3f 100644
--- a/submodule.h
+++ b/submodule.h
@@ -20,7 +20,7 @@ void handle_ignore_submodules_arg(struct diff_options *diffopt, const char *);
int parse_fetch_recurse_submodules_arg(const char *opt, const char *arg);
void show_submodule_summary(FILE *f, const char *path,
unsigned char one[20], unsigned char two[20],
- unsigned dirty_submodule,
+ unsigned dirty_submodule, const char *meta,
const char *del, const char *add, const char *reset);
void set_config_fetch_recurse_submodules(int value);
void check_for_new_submodule_commits(unsigned char new_sha1[20]);
--
1.7.8.1.362.g5d6df.dirty
^ permalink raw reply related
* [PATCH v4 2/4] diff: introduce diff.submodule configuration variable
From: Ramkumar Ramachandra @ 2012-11-13 15:42 UTC (permalink / raw)
To: Git List
In-Reply-To: <1352821367-3611-1-git-send-email-artagnon@gmail.com>
Introduce a diff.submodule configuration variable corresponding to the
'--submodule' command-line option of 'git diff'.
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
Documentation/diff-config.txt | 7 +++++++
Documentation/diff-options.txt | 3 ++-
diff.c | 32 ++++++++++++++++++++++++++++----
t/t4041-diff-submodule-option.sh | 30 +++++++++++++++++++++++++++++-
4 files changed, 66 insertions(+), 6 deletions(-)
diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
index decd370..89dd634 100644
--- a/Documentation/diff-config.txt
+++ b/Documentation/diff-config.txt
@@ -107,6 +107,13 @@ diff.suppressBlankEmpty::
A boolean to inhibit the standard behavior of printing a space
before each empty output line. Defaults to false.
+diff.submodule::
+ Specify the format in which differences in submodules are
+ shown. The "log" format lists the commits in the range like
+ linkgit:git-submodule[1] `summary` does. The "short" format
+ format just shows the names of the commits at the beginning
+ and end of the range. Defaults to short.
+
diff.wordRegex::
A POSIX Extended Regular Expression used to determine what is a "word"
when performing word-by-word difference calculations. Character
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index cf4b216..f4f7e25 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -170,7 +170,8 @@ any of those replacements occurred.
the commits in the range like linkgit:git-submodule[1] `summary` does.
Omitting the `--submodule` option or specifying `--submodule=short`,
uses the 'short' format. This format just shows the names of the commits
- at the beginning and end of the range.
+ at the beginning and end of the range. Can be tweaked via the
+ `diff.submodule` configuration variable.
--color[=<when>]::
Show colored diff.
diff --git a/diff.c b/diff.c
index e89a201..7f2a255 100644
--- a/diff.c
+++ b/diff.c
@@ -123,6 +123,17 @@ static int parse_dirstat_params(struct diff_options *options, const char *params
return ret;
}
+static int parse_submodule_params(struct diff_options *options, const char *value)
+{
+ if (!strcmp(value, "log"))
+ DIFF_OPT_SET(options, SUBMODULE_LOG);
+ else if (!strcmp(value, "short"))
+ DIFF_OPT_CLR(options, SUBMODULE_LOG);
+ else
+ return -1;
+ return 0;
+}
+
static int git_config_rename(const char *var, const char *value)
{
if (!value)
@@ -178,6 +189,13 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)
if (!strcmp(var, "diff.ignoresubmodules"))
handle_ignore_submodules_arg(&default_diff_options, value);
+ if (!strcmp(var, "diff.submodule")) {
+ if (parse_submodule_params(&default_diff_options, value))
+ warning(_("Unknown value for 'diff.submodule' config variable: '%s'"),
+ value);
+ return 0;
+ }
+
if (git_color_config(var, value, cb) < 0)
return -1;
@@ -3475,6 +3493,14 @@ static int parse_dirstat_opt(struct diff_options *options, const char *params)
return 1;
}
+static int parse_submodule_opt(struct diff_options *options, const char *value)
+{
+ if (parse_submodule_params(options, value))
+ die(_("Failed to parse --submodule option parameter: '%s'"),
+ value);
+ return 1;
+}
+
int diff_opt_parse(struct diff_options *options, const char **av, int ac)
{
const char *arg = av[0];
@@ -3655,10 +3681,8 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
handle_ignore_submodules_arg(options, arg + 20);
} else if (!strcmp(arg, "--submodule"))
DIFF_OPT_SET(options, SUBMODULE_LOG);
- else if (!prefixcmp(arg, "--submodule=")) {
- if (!strcmp(arg + 12, "log"))
- DIFF_OPT_SET(options, SUBMODULE_LOG);
- }
+ else if (!prefixcmp(arg, "--submodule="))
+ return parse_submodule_opt(options, arg + 12);
/* misc options */
else if (!strcmp(arg, "-z"))
diff --git a/t/t4041-diff-submodule-option.sh b/t/t4041-diff-submodule-option.sh
index 6c01d0c..e401814 100755
--- a/t/t4041-diff-submodule-option.sh
+++ b/t/t4041-diff-submodule-option.sh
@@ -33,6 +33,7 @@ test_create_repo sm1 &&
add_file . foo >/dev/null
head1=$(add_file sm1 foo1 foo2)
+fullhead1=$(cd sm1; git rev-list --max-count=1 $head1)
test_expect_success 'added submodule' "
git add sm1 &&
@@ -43,6 +44,34 @@ EOF
test_cmp expected actual
"
+test_expect_success 'added submodule, set diff.submodule' "
+ git config diff.submodule log &&
+ git add sm1 &&
+ git diff --cached >actual &&
+ cat >expected <<-EOF &&
+Submodule sm1 0000000...$head1 (new submodule)
+EOF
+ git config --unset diff.submodule &&
+ test_cmp expected actual
+"
+
+test_expect_success '--submodule=short overrides diff.submodule' "
+ git config diff.submodule log &&
+ git add sm1 &&
+ git diff --submodule=short --cached >actual &&
+ cat >expected <<-EOF &&
+diff --git a/sm1 b/sm1
+new file mode 160000
+index 0000000..a2c4dab
+--- /dev/null
++++ b/sm1
+@@ -0,0 +1 @@
++Subproject commit $fullhead1
+EOF
+ git config --unset diff.submodule &&
+ test_cmp expected actual
+"
+
commit_file sm1 &&
head2=$(add_file sm1 foo3)
@@ -73,7 +102,6 @@ EOF
test_cmp expected actual
"
-fullhead1=$(cd sm1; git rev-list --max-count=1 $head1)
fullhead2=$(cd sm1; git rev-list --max-count=1 $head2)
test_expect_success 'modified submodule(forward) --submodule=short' "
git diff --submodule=short >actual &&
--
1.7.8.1.362.g5d6df.dirty
^ permalink raw reply related
* [PATCH v4 3/4] diff: rename "set" variable
From: Ramkumar Ramachandra @ 2012-11-13 15:42 UTC (permalink / raw)
To: Git List
In-Reply-To: <1352821367-3611-1-git-send-email-artagnon@gmail.com>
From: Jeff King <peff@peff.net>
Once upon a time the builtin_diff function used one color, and the color
variables were called "set" and "reset". Nowadays it is a much longer
function and we use several colors (e.g., "add", "del"). Rename "set" to
"meta" to show that it is the color for showing diff meta-info (it still
does not indicate that it is a "color", but at least it matches the
scheme of the other color variables).
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
diff.c | 12 ++++++------
1 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/diff.c b/diff.c
index 7f2a255..ffaed72 100644
--- a/diff.c
+++ b/diff.c
@@ -2240,7 +2240,7 @@ static void builtin_diff(const char *name_a,
mmfile_t mf1, mf2;
const char *lbl[2];
char *a_one, *b_two;
- const char *set = diff_get_color_opt(o, DIFF_METAINFO);
+ const char *meta = diff_get_color_opt(o, DIFF_METAINFO);
const char *reset = diff_get_color_opt(o, DIFF_RESET);
const char *a_prefix, *b_prefix;
struct userdiff_driver *textconv_one = NULL;
@@ -2287,24 +2287,24 @@ static void builtin_diff(const char *name_a,
b_two = quote_two(b_prefix, name_b + (*name_b == '/'));
lbl[0] = DIFF_FILE_VALID(one) ? a_one : "/dev/null";
lbl[1] = DIFF_FILE_VALID(two) ? b_two : "/dev/null";
- strbuf_addf(&header, "%s%sdiff --git %s %s%s\n", line_prefix, set, a_one, b_two, reset);
+ strbuf_addf(&header, "%s%sdiff --git %s %s%s\n", line_prefix, meta, a_one, b_two, reset);
if (lbl[0][0] == '/') {
/* /dev/null */
- strbuf_addf(&header, "%s%snew file mode %06o%s\n", line_prefix, set, two->mode, reset);
+ strbuf_addf(&header, "%s%snew file mode %06o%s\n", line_prefix, meta, two->mode, reset);
if (xfrm_msg)
strbuf_addstr(&header, xfrm_msg);
must_show_header = 1;
}
else if (lbl[1][0] == '/') {
- strbuf_addf(&header, "%s%sdeleted file mode %06o%s\n", line_prefix, set, one->mode, reset);
+ strbuf_addf(&header, "%s%sdeleted file mode %06o%s\n", line_prefix, meta, one->mode, reset);
if (xfrm_msg)
strbuf_addstr(&header, xfrm_msg);
must_show_header = 1;
}
else {
if (one->mode != two->mode) {
- strbuf_addf(&header, "%s%sold mode %06o%s\n", line_prefix, set, one->mode, reset);
- strbuf_addf(&header, "%s%snew mode %06o%s\n", line_prefix, set, two->mode, reset);
+ strbuf_addf(&header, "%s%sold mode %06o%s\n", line_prefix, meta, one->mode, reset);
+ strbuf_addf(&header, "%s%snew mode %06o%s\n", line_prefix, meta, two->mode, reset);
must_show_header = 1;
}
if (xfrm_msg)
--
1.7.8.1.362.g5d6df.dirty
^ permalink raw reply related
* [PATCH v4 1/4] Documentation: move diff.wordRegex from config.txt to diff-config.txt
From: Ramkumar Ramachandra @ 2012-11-13 15:42 UTC (permalink / raw)
To: Git List
In-Reply-To: <1352821367-3611-1-git-send-email-artagnon@gmail.com>
19299a8 (Documentation: Move diff.<driver>.* from config.txt to
diff-config.txt, 2011-04-07) moved the diff configuration options to
diff-config.txt, but forgot about diff.wordRegex, which was left
behind in config.txt. Fix this.
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
Documentation/config.txt | 6 ------
Documentation/diff-config.txt | 6 ++++++
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 9a0544c..e70216d 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -962,12 +962,6 @@ difftool.<tool>.cmd::
difftool.prompt::
Prompt before each invocation of the diff tool.
-diff.wordRegex::
- A POSIX Extended Regular Expression used to determine what is a "word"
- when performing word-by-word difference calculations. Character
- sequences that match the regular expression are "words", all other
- characters are *ignorable* whitespace.
-
fetch.recurseSubmodules::
This option can be either set to a boolean value or to 'on-demand'.
Setting it to a boolean changes the behavior of fetch and pull to
diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
index 75ab8a5..decd370 100644
--- a/Documentation/diff-config.txt
+++ b/Documentation/diff-config.txt
@@ -107,6 +107,12 @@ diff.suppressBlankEmpty::
A boolean to inhibit the standard behavior of printing a space
before each empty output line. Defaults to false.
+diff.wordRegex::
+ A POSIX Extended Regular Expression used to determine what is a "word"
+ when performing word-by-word difference calculations. Character
+ sequences that match the regular expression are "words", all other
+ characters are *ignorable* whitespace.
+
diff.<driver>.command::
The custom diff driver command. See linkgit:gitattributes[5]
for details.
--
1.7.8.1.362.g5d6df.dirty
^ permalink raw reply related
* [PATCH v4 0/4] Introduce diff.submodule
From: Ramkumar Ramachandra @ 2012-11-13 15:42 UTC (permalink / raw)
To: Git List
In-Reply-To: <http://mid.gmane.org/1352653146-3932-1-git-send-email-artagnon@gmail.com>
v1 is here: http://mid.gmane.org/1349196670-2844-1-git-send-email-artagnon@gmail.com
v2 is here: http://mid.gmane.org/1351766630-4837-1-git-send-email-artagnon@gmail.com
v3 is here: http://mid.gmane.org/1352653146-3932-1-git-send-email-artagnon@gmail.com
This version was prepared in response to Peff's review of v3.
What changed:
* Functional code simplified and moved to git_diff_ui_config.
* Peff contributed one additional patch to the series.
Thanks.
Ram
Jeff King (1):
diff: rename "set" variable
Ramkumar Ramachandra (3):
Documentation: move diff.wordRegex from config.txt to diff-config.txt
diff: introduce diff.submodule configuration variable
submodule: display summary header in bold
Documentation/config.txt | 6 -----
Documentation/diff-config.txt | 13 ++++++++++
Documentation/diff-options.txt | 3 +-
diff.c | 46 ++++++++++++++++++++++++++++---------
submodule.c | 8 +++---
submodule.h | 2 +-
t/t4041-diff-submodule-option.sh | 30 ++++++++++++++++++++++++-
7 files changed, 84 insertions(+), 24 deletions(-)
--
1.7.8.1.362.g5d6df.dirty
^ permalink raw reply
* Re: [PATCH] update-index/diff-index: use core.preloadindex to improve performance
From: karsten.blees @ 2012-11-13 15:36 UTC (permalink / raw)
To: Jeff King; +Cc: git, msysgit, pro-logic
In-Reply-To: <20121102153800.GE11170@sigill.intra.peff.net>
Jeff King <peff@peff.net> wrote on 02.11.2012 16:38:00:
> On Fri, Nov 02, 2012 at 11:26:16AM -0400, Jeff King wrote:
>
> > Still, I don't think we need to worry about performance regressions,
> > because people who don't have a setup suitable for it will not turn on
> > core.preloadindex in the first place. And if they have it on, the more
> > places we use it, probably the better.
>
> BTW, your patch was badly damaged in transit (wrapped, and tabs
> converted to spaces). I was able to fix it up, but please check your
> mailer's settings.
>
Yes, I feared as much, that's why I included the pull URL (the company MTA
only accepts outbound mail from Notes clients, sorry).
Is there a policy for people with broken mailers (send patch as
attachment, add pull URL more prominently, don't include plaintext patch
at all...)?
^ permalink raw reply
* Re: [PATCH] update-index/diff-index: use core.preloadindex to improve performance
From: karsten.blees @ 2012-11-13 15:33 UTC (permalink / raw)
To: Jeff King; +Cc: git, msysgit, pro-logic
In-Reply-To: <20121102152616.GD11170@sigill.intra.peff.net>
Jeff King <peff@peff.net> wrote on 02.11.2012 16:26:16:
> On Tue, Oct 30, 2012 at 10:50:42AM +0100, karsten.blees@dcon.de wrote:
>
> > 'update-index --refresh' and 'diff-index' (without --cached) don't
honor
> > the core.preloadindex setting yet. Porcelain commands using these
(such as
> > git [svn] rebase) suffer from this, especially on Windows.
> >
> > Use read_cache_preload to improve performance.
> >
> > Additionally, in builtin/diff.c, don't preload index status if we
don't
> > access the working copy (--cached).
> >
> > Results with msysgit on WebKit repo (2GB in 200k files):
> >
> > | update-index | diff-index | rebase
> > ----------------+--------------+------------+---------
> > msysgit-v1.8.0 | 9.157s | 10.536s | 42.791s
> > + preloadindex | 9.157s | 10.536s | 28.725s
> > + this patch | 2.329s | 2.752s | 15.152s
> > + fscache [1] | 0.731s | 1.171s | 8.877s
>
> Cool numbers. On my quad-core SSD Linux box, I saw a few speedups, too.
> Here are the numbers for "update-index --refresh" on the WebKit repo
> (all are wall clock time, best-of-five):
>
> | before | after
> -----------+--------+--------
> cold cache | 4.513s | 2.059s
> warm cache | 0.252s | 0.164s
>
Great, and thanks for testing on Linux (I only have Linux VMs for testing,
and I couldn't get meaningful performance data from those).
> Not as dramatic, but still nice. I wonder how a spinning disk would fare
> on the cold-cache case, though. I also tried it with all but one CPU
> disabled, and the warm cache case was a little bit slower.
>
Unfortunately, with a 'real' disk, cold cache times increase with the
number of threads. I've played around with '#define MAX_PARALLEL 20' in
preload-index.c, with the following results ('git update-index --refresh'
on WebKit repo, Win7 x64, Core i7 860 (2.8GHz 4 Core HT), WD VelociRaptor
(300G 10krpm 8ms), msysgit 1.8.0 + preload-index patch + fscache patch):
MAX_PARALLEL | cold cache | warm cache
-------------+------------+------------
no preload | 49.938 | 9.204 (*)
1 | 45.412 | 1.622
2 | 55.334 | 1.123
3 | 65.973 | 0.982
4 | 67.579 | 0.889
5 | 76.159 | 0.827
6 | 81.510 | 0.811
7 | 86.269 | 0.858
8 | 85.862 | 0.827
...
10 | 87.953 | 0.717
...
20 | 176.251 | 0.749
(*) core.preloadindex currently also disables fscache, thus the 9s
With more threads, Windows resource monitor also shows increasing disk
queue length and response times.
It seems clear that more concurrent disk seeks hurt cold cache performance
pretty badly. On the other hand, warm cache improvements for #threads >
#cores are only about 10 - 20%.
I don't know if Linux is better at caching / prefetching directory
listings (might depend on file system, too), but perhaps MAX_PARALLEL
should be set to a more reasonable value, or be made configurable (e.g.
core.preloadIndexThreads)?
Karsten
^ permalink raw reply
* checkout from neighbour branch undeletes a path?
From: Peter Vereshagin @ 2012-11-13 15:23 UTC (permalink / raw)
To: git
Hello.
Am wondering if 'checkout branch path' undeletes the files? For the example
below I'd like the 'file00.txt' to be deleted and never checked out from the
previous branch... How can I do that?
$ git init
Initialized empty Git repository in /tmp/repo00/.git/
$ mkdir pathdir
$ echo test00 > pathdir/file00.txt
$ git add pathdir
$ git commit -am 'added file00.txt'
[master (root-commit) d4f7c70] added file00.txt
1 files changed, 1 insertions(+), 0 deletions(-)
create mode 100644 pathdir/file00.txt
$ git branch -m master branch00
$ git branch branch01
$ rm pathdir/file00.txt
$ echo test01 > pathdir/file01.txt
$ git add pathdir
$ git status
$ git commit -am 'added file01.txt; removed file00.txt'
[branch00 c3e78ff] added file01.txt; removed file00.txt
2 files changed, 1 insertions(+), 1 deletions(-)
delete mode 100644 pathdir/file00.txt
create mode 100644 pathdir/file01.txt
$ git checkout branch01
Switched to branch 'branch01'
$ rm -r pathdir
$ git checkout branch00 pathdir
$ find pathdir/
pathdir/
pathdir/file00.txt
pathdir/file01.txt
$
I know about 'merge' and it's not the what I need: to import only the
particular subdirectory from the previous branch.
Thank you.
--
Peter Vereshagin <peter@vereshagin.org> (http://vereshagin.org) pgp: A0E26627
^ permalink raw reply
* Re: [BUG] gitweb: XSS vulnerability of RSS feed
From: Jakub Narębski @ 2012-11-13 15:19 UTC (permalink / raw)
To: Drew Northup
Cc: Jeff King, glpk xypron, git, Junio C Hamano,
Jason J Pyeron CTR (US), Andreas Schwab
In-Reply-To: <CAM9Z-nkuHj8MWLfWsvY=EqHXCUS+Pk5Ezv6m5J+cnh7cQHNc_g@mail.gmail.com>
On Tue, Nov 13, 2012 at 3:44 PM, Drew Northup <n1xim.email@gmail.com> wrote:
> On Mon, Nov 12, 2012 at 3:24 PM, Jeff King <peff@peff.net> wrote:
>> On Mon, Nov 12, 2012 at 01:55:46PM -0500, Drew Northup wrote:
>>> + # No XSS <script></script> inclusions
>>> + if ($input =~ m!(<script>)(.*)(</script>)!){
>>> + return undef;
>>> + }
>> This is the wrong fix for a few reasons:
>>
>> 1. It is on the input-validation side, whereas the real problem is on
>> the output-quoting side. Your patch means I could not access a file
>> called "<script>foo</script>". What we really want is to have the
>> unquoted name internally, but then make sure we quote it when
>> outputting as part of an HTML (or XML) file.
>
> I don't buy the argument that we don't need to clean up the input as
> well. There are scant few of us that are going to name a file
> "<script>alert("Something Awful")</script>" in this world (I am
> probably one of them). Input validation is key to keeping problems
> like this from coming up repeatedly as those writing the guts of
> programs are typically more interested in getting the "assigned task"
> done and reporting the output to the user in a safe manner.
Input cleanup or blacklisting *does not* prevent code injection (XSS
in this case). This is a myth.
Input validation has its place, and is done by gitweb when possible
(see e.g. evaluate_and_validate_params, validate_project, etc.).
But the proposed solution is not input validation.
'<script>alert("Something Awful")</script>' is a perfectly valid filename.
As is more realistic "<<create>>.uml" or "File > Open screenshot.png".
And last and most important you have to escape output anyway;
filename is not HTML. Without escaping it would be rendered incorrectly.
And HTML escaping prevents XSS.
>> I think the right answer is going to be a well-placed call to esc_html.
>> This already happens automatically when we go through the CGI
>> element-building functions, but obviously we failed to make the call
>> when building the output manually. This is a great reason why template
>> languages which default to safe expansion should always be used.
>> Unfortunately, gitweb is living in 1995 in terms of web frameworks.
>
> Escaping the output protects the user, but it DOES NOT protect the
> server. We MUST handle both possibilities.
Errr, what?
If you are thinking about shell injection, we are covered.
Gitweb uses list form of open which is for shell what prepared
statements are for SQL. In one or two cases where we need to
use pipe we do shell escaping.
> Besides, inserting one call to esc_html only fixes one attack path. I
> didn't look to see if all others were already covered.
They should be covered. This case slipped.
--
Jakub Narebski
^ 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