* [PATCH/RFC 2/5] compat/terminal: factor out echo-disabling
From: Erik Faye-Lund @ 2012-11-13 14:04 UTC (permalink / raw)
To: git, msysgit; +Cc: peff
In-Reply-To: <1352815447-8824-1-git-send-email-kusmabite@gmail.com>
By moving the echo-disabling code to a separate function, we can
implement OS-specific versions of it for non-POSIX platforms.
Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
---
compat/terminal.c | 43 +++++++++++++++++++++++++------------------
1 file changed, 25 insertions(+), 18 deletions(-)
diff --git a/compat/terminal.c b/compat/terminal.c
index bbb038d..3217838 100644
--- a/compat/terminal.c
+++ b/compat/terminal.c
@@ -14,6 +14,7 @@ static void restore_term(void)
return;
tcsetattr(term_fd, TCSAFLUSH, &old_term);
+ close(term_fd);
term_fd = -1;
}
@@ -24,6 +25,27 @@ static void restore_term_on_signal(int sig)
raise(sig);
}
+static int disable_echo()
+{
+ struct termios t;
+
+ term_fd = open("/dev/tty", O_RDWR);
+ if (tcgetattr(term_fd, &t) < 0)
+ goto error;
+
+ old_term = t;
+ sigchain_push_common(restore_term_on_signal);
+
+ t.c_lflag &= ~ECHO;
+ if (!tcsetattr(term_fd, TCSAFLUSH, &t))
+ return 0;
+
+error:
+ close(term_fd);
+ term_fd = -1;
+ return -1;
+}
+
char *git_terminal_prompt(const char *prompt, int echo)
{
static struct strbuf buf = STRBUF_INIT;
@@ -34,24 +56,9 @@ char *git_terminal_prompt(const char *prompt, int echo)
if (!fh)
return NULL;
- if (!echo) {
- struct termios t;
-
- if (tcgetattr(fileno(fh), &t) < 0) {
- fclose(fh);
- return NULL;
- }
-
- old_term = t;
- term_fd = fileno(fh);
- sigchain_push_common(restore_term_on_signal);
-
- t.c_lflag &= ~ECHO;
- if (tcsetattr(fileno(fh), TCSAFLUSH, &t) < 0) {
- term_fd = -1;
- fclose(fh);
- return NULL;
- }
+ if (!echo && disable_echo()) {
+ fclose(fh);
+ return NULL;
}
fputs(prompt, fh);
--
1.8.0.7.gbeffeda
--
*** 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 related
* [PATCH/RFC 1/5] mingw: make fgetc raise SIGINT if apropriate
From: Erik Faye-Lund @ 2012-11-13 14:04 UTC (permalink / raw)
To: git, msysgit; +Cc: peff
In-Reply-To: <1352815447-8824-1-git-send-email-kusmabite@gmail.com>
Set a control-handler to prevent the process from terminating, and
simulate SIGINT so it can be handled by a signal-handler as usual.
Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
---
compat/mingw.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++--------
compat/mingw.h | 6 +++++
2 files changed, 72 insertions(+), 10 deletions(-)
diff --git a/compat/mingw.c b/compat/mingw.c
index 78e8f54..33ddfdf 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -319,6 +319,31 @@ ssize_t mingw_write(int fd, const void *buf, size_t count)
return write(fd, buf, min(count, 31 * 1024 * 1024));
}
+static BOOL WINAPI ctrl_ignore(DWORD type)
+{
+ return TRUE;
+}
+
+#undef fgetc
+int mingw_fgetc(FILE *stream)
+{
+ int ch;
+ if (!isatty(_fileno(stream)))
+ return fgetc(stream);
+
+ SetConsoleCtrlHandler(ctrl_ignore, TRUE);
+ while (1) {
+ ch = fgetc(stream);
+ if (ch != EOF || GetLastError() != ERROR_OPERATION_ABORTED)
+ break;
+
+ /* Ctrl+C was pressed, simulate SIGINT and retry */
+ mingw_raise(SIGINT);
+ }
+ SetConsoleCtrlHandler(ctrl_ignore, FALSE);
+ return ch;
+}
+
#undef fopen
FILE *mingw_fopen (const char *filename, const char *otype)
{
@@ -1524,7 +1549,7 @@ static HANDLE timer_event;
static HANDLE timer_thread;
static int timer_interval;
static int one_shot;
-static sig_handler_t timer_fn = SIG_DFL;
+static sig_handler_t timer_fn = SIG_DFL, sigint_fn = SIG_DFL;
/* The timer works like this:
* The thread, ticktack(), is a trivial routine that most of the time
@@ -1538,13 +1563,7 @@ static sig_handler_t timer_fn = SIG_DFL;
static unsigned __stdcall ticktack(void *dummy)
{
while (WaitForSingleObject(timer_event, timer_interval) == WAIT_TIMEOUT) {
- if (timer_fn == SIG_DFL) {
- if (isatty(STDERR_FILENO))
- fputs("Alarm clock\n", stderr);
- exit(128 + SIGALRM);
- }
- if (timer_fn != SIG_IGN)
- timer_fn(SIGALRM);
+ mingw_raise(SIGALRM);
if (one_shot)
break;
}
@@ -1635,12 +1654,49 @@ int sigaction(int sig, struct sigaction *in, struct sigaction *out)
sig_handler_t mingw_signal(int sig, sig_handler_t handler)
{
sig_handler_t old = timer_fn;
- if (sig != SIGALRM)
+
+ switch (sig) {
+ case SIGALRM:
+ timer_fn = handler;
+ break;
+
+ case SIGINT:
+ sigint_fn = handler;
+ break;
+
+ default:
return signal(sig, handler);
- timer_fn = handler;
+ }
+
return old;
}
+#undef raise
+int mingw_raise(int sig)
+{
+ switch (sig) {
+ case SIGALRM:
+ if (timer_fn == SIG_DFL) {
+ if (isatty(STDERR_FILENO))
+ fputs("Alarm clock\n", stderr);
+ exit(128 + SIGALRM);
+ } else if (timer_fn != SIG_IGN)
+ timer_fn(SIGALRM);
+ return 0;
+
+ case SIGINT:
+ if (sigint_fn == SIG_DFL)
+ exit(128 + SIGINT);
+ else if (sigint_fn != SIG_IGN)
+ sigint_fn(SIGINT);
+ return 0;
+
+ default:
+ return raise(sig);
+ }
+}
+
+
static const char *make_backslash_path(const char *path)
{
static char buf[PATH_MAX + 1];
diff --git a/compat/mingw.h b/compat/mingw.h
index 61a6521..6b9e69a 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -179,6 +179,9 @@ int mingw_open (const char *filename, int oflags, ...);
ssize_t mingw_write(int fd, const void *buf, size_t count);
#define write mingw_write
+int mingw_fgetc(FILE *stream);
+#define fgetc mingw_fgetc
+
FILE *mingw_fopen (const char *filename, const char *otype);
#define fopen mingw_fopen
@@ -287,6 +290,9 @@ static inline unsigned int git_ntohl(unsigned int x)
sig_handler_t mingw_signal(int sig, sig_handler_t handler);
#define signal mingw_signal
+int mingw_raise(int sig);
+#define raise mingw_raise
+
/*
* ANSI emulation wrappers
*/
--
1.8.0.7.gbeffeda
--
*** 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 related
* [PATCH/RFC 0/5] win32: support echo for terminal-prompt
From: Erik Faye-Lund @ 2012-11-13 14:04 UTC (permalink / raw)
To: git, msysgit; +Cc: peff
We currently only support getpass, which does not echo at all, for
git_terminal_prompt on Windows. The Windows console is perfectly
capable of doing this, so let's make it so.
This implementation tries to reuse the /dev/tty-code as much as
possible.
The big reason that this becomes a bit hairy is that Ctrl+C needs
to be handled correctly, so we don't leak the console state to a
non-echoing setting when a user aborts.
Windows makes this bit a little bit tricky, in that we need to
implement SIGINT for fgetc. However, I suspect that this is a good
thing to do in the first place.
An earlier iteration was also breifly discussed here:
http://mid.gmane.org/CABPQNSaUCEDU4+2N63n0k_XwSXOP_iFZG3GEYSPSBPcSVV8wRQ@mail.gmail.com
The series can also be found here, only with an extra patch that
makes the (interactive) testing a bit easier:
https://github.com/kusma/git/tree/work/terminal-cleanup
Erik Faye-Lund (5):
mingw: make fgetc raise SIGINT if apropriate
compat/terminal: factor out echo-disabling
compat/terminal: separate input and output handles
mingw: reuse tty-version of git_terminal_prompt
mingw: get rid of getpass implementation
compat/mingw.c | 91 +++++++++++++++++++++++++++-----------
compat/mingw.h | 8 +++-
compat/terminal.c | 129 ++++++++++++++++++++++++++++++++++++++++--------------
3 files changed, 169 insertions(+), 59 deletions(-)
--
1.8.0.7.gbeffeda
--
*** 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
* [PATCH 0/5] win32: support echo for terminal-prompt
From: Erik Faye-Lund @ 2012-11-13 14:01 UTC (permalink / raw)
To: git, msysgit; +Cc: peff
We currently only support getpass, which does not echo at all, for
git_terminal_prompt on Windows. The Windows console is perfectly
capable of doing this, so let's make it so.
This implementation tries to reuse the /dev/tty-code as much as
possible.
The big reason that this becomes a bit hairy is that Ctrl+C needs
to be handled correctly, so we don't leak the console state to a
non-echoing setting when a user aborts.
Windows makes this bit a little bit tricky, in that we need to
implement SIGINT for fgetc. However, I suspect that this is a good
thing to do in the first place.
An earlier iteration was also breifly discussed here:
http://mid.gmane.org/CABPQNSaUCEDU4+2N63n0k_XwSXOP_iFZG3GEYSPSBPcSVV8wRQ@mail.gmail.com
The series can also be found here, only with an extra patch that
makes the (interactive) testing a bit easier:
https://github.com/kusma/git/tree/work/terminal-cleanup
Erik Faye-Lund (5):
mingw: make fgetc raise SIGINT if apropriate
compat/terminal: factor out echo-disabling
compat/terminal: separate input and output handles
mingw: reuse tty-version of git_terminal_prompt
mingw: get rid of getpass implementation
compat/mingw.c | 91 +++++++++++++++++++++++++++-----------
compat/mingw.h | 8 +++-
compat/terminal.c | 129 ++++++++++++++++++++++++++++++++++++++++--------------
3 files changed, 169 insertions(+), 59 deletions(-)
--
1.8.0.7.gbeffeda
--
*** 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: [PATCHv4] replace: parse revision argument for -d
From: Jeff King @ 2012-11-13 13:37 UTC (permalink / raw)
To: Michael J Gruber; +Cc: git, Junio C Hamano
In-Reply-To: <848bc8f48885acd63b4f6d29eea0543e73d86451.1352802239.git.git@drmicha.warpmail.net>
On Tue, Nov 13, 2012 at 11:34:11AM +0100, Michael J Gruber wrote:
> '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:
> v4 names the aux variable more concisely and does away with a superfluous
> assignment.
Thanks. I agree the name "full_hex" is much better. Patch looks good to
me at this point.
-Peff
^ permalink raw reply
* Re: Reviews on mailing-list
From: Nguyen Thai Ngoc Duy @ 2012-11-13 13:38 UTC (permalink / raw)
To: David Lang
Cc: Krzysztof Mazur, Thiago Farina, Felipe Contreras,
Deniz Türkoglu, git, Junio C Hamano, Shawn Pearce
In-Reply-To: <alpine.DEB.2.02.1211111313140.19687@nftneq.ynat.uz>
On Mon, Nov 12, 2012 at 4:15 AM, David Lang <david@lang.hm> wrote:
> Using a web browser requires connectivity at the time you are doing the
> review.
>
> Mailing list based reviews can be done at times when you don't have
> connectivity.
I am not against email-based reviews but I'd like to point out that
with Google Gears (and HTML5 Storage?) Gerrit can be made work offline
too. I don't know how much work required though.
--
Duy
^ permalink raw reply
* Re: Notes in format-patch (was: Re: [PATCHv3] replace: parse revision argument for -d)
From: Jeff King @ 2012-11-13 13:38 UTC (permalink / raw)
To: Michael J Gruber; +Cc: git, Junio C Hamano
In-Reply-To: <50A2213B.4060505@drmicha.warpmail.net>
On Tue, Nov 13, 2012 at 11:30:19AM +0100, Michael J Gruber wrote:
> 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 (and omit the one after the ---) but may have goofed up a rebase:
I do not know was intended, but the above quoted output is hard to read,
and your suggested change looks way better.
-Peff
^ permalink raw reply
* Re: Simple question ? [Git branch]
From: Ramkumar Ramachandra @ 2012-11-13 12:47 UTC (permalink / raw)
To: agatte; +Cc: git
In-Reply-To: <1352810777584-7571115.post@n2.nabble.com>
agatte wrote:
> I don't know what I could change branches ?
It's called "checkout".
Ram
^ permalink raw reply
* Simple question ? [Git branch]
From: agatte @ 2012-11-13 12:46 UTC (permalink / raw)
To: git
Hi All Users,
I am beginner in git. I am doing my first steps with this tool.
Now, I used git gui on linux OS.
I don't know what I could change branches ?
I need to change current working branch to do a commit.
I can see in the menu branch :
Create
checkout
rebase
Reset
--
Could anyone help me please ?
I would appreciate for any help.
agatte
--
View this message in context: http://git.661346.n2.nabble.com/Simple-question-Git-branch-tp7571115.html
Sent from the git mailing list archive at Nabble.com.
^ permalink raw reply
* Re: Fwd: [PATCH] Add tcsh-completion support to contrib by using git-completion.bash
From: SZEDER Gábor @ 2012-11-13 11:14 UTC (permalink / raw)
To: Marc Khouzam; +Cc: git
In-Reply-To: <CAFj1UpFd9X8Jq5o7B4m35i=merBDvOo4NOtwth=UnG2S5X_rGw@mail.gmail.com>
Hi,
On Mon, Nov 12, 2012 at 03:07:46PM -0500, Marc Khouzam wrote:
> Hi,
[...]
> Signed-off-by: Marc Khouzam <marc.khouzam@gmail.com>
[...]
> Thanks
>
> Marc
>
> ---
> contrib/completion/git-completion.bash | 53 +++++++++++++++++++++++++++++++-
> contrib/completion/git-completion.tcsh | 34 ++++++++++++++++++++
> 2 files changed, 86 insertions(+), 1 deletions(-)
> create mode 100755 contrib/completion/git-completion.tcsh
Please have a look at Documentation/SubmittingPatches to see how to
properly format the commit message, i.e. no greeting and sign-off in
the commit message part, and the S-o-b line should be the last before
the '---'.
Your patch seems to be severely line-wrapped. That document also
contains a few MUA-specific tips to help avoid that.
Other than that, it's a good description of the changes and
considerations. I agree that this approach seems to be the best from
the three.
> diff --git a/contrib/completion/git-completion.bash
> b/contrib/completion/git-completion.bash
> index be800e0..6d4b57a 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -1,4 +1,6 @@
> -#!bash
> +#!/bin/bash
> +# The above line is important as this script can be executed when used
> +# with another shell such as tcsh
See comment near the end.
> #
> # bash/zsh completion support for core Git.
> #
> @@ -2481,3 +2483,52 @@ __git_complete gitk __gitk_main
> if [ Cygwin = "$(uname -o 2>/dev/null)" ]; then
> __git_complete git.exe __git_main
> fi
> +
> +# Method that will output the result of the completion done by
> +# the bash completion script, so that it can be re-used in another
> +# context than the bash complete command.
> +# It accepts 1 to 2 arguments:
> +# 1: The command-line to complete
> +# 2: The index of the word within argument #1 in which the cursor is
> +# located (optional). If parameter 2 is not provided, it will be
> +# determined as best possible using parameter 1.
> +_git_complete_with_output ()
> +{
> + # Set COMP_WORDS to the command-line as bash would.
> + COMP_WORDS=($1)
That comment is only true for older Bash versions. Since v4 Bash
splits the command line at characters that the readline library treats
as word separators when performing word completion. But the
completion script has functions to deal with both, so this shouldn't
be a problem.
> +
> + # Set COMP_CWORD to the cursor location as bash would.
> + if [ -n "$2" ]; then
> + COMP_CWORD=$2
> + else
> + # Assume the cursor is at the end of parameter #1.
> + # We must check for a space as the last character which will
> + # tell us that the previous word is complete and the cursor
> + # is on the next word.
> + if [ "${1: -1}" == " " ]; then
> + # The last character is a space, so our
> location is at the end
> + # of the command-line array
> + COMP_CWORD=${#COMP_WORDS[@]}
> + else
> + # The last character is not a space, so our
> location is on the
> + # last word of the command-line array, so we
> must decrement the
> + # count by 1
> + COMP_CWORD=$((${#COMP_WORDS[@]}-1))
> + fi
> + fi
> +
> + # Call _git() or _gitk() of the bash script, based on the first
> + # element of the command-line
> + _${COMP_WORDS[0]}
> +
> + # Print the result that is stored in the bash variable ${COMPREPLY}
Really? ;)
I like the above comments about setting COMP_CWORD, because they
explain why you do what you do, which would be otherwise difficult to
figure out. But telling that an echo in a for loop over an array
prints that array is, well, probably not necessary.
> + for i in ${COMPREPLY[@]}; do
> + echo "$i"
> + done
There is no need for the loop here to print the array one element per
line:
local IFS=$'\n'
echo "${COMPREPLY[*]}"
> +}
> +
> +if [ -n "$1" ] ; then
> + # If there is an argument, we know the script is being executed
> + # so go ahead and run the _git_complete_with_output function
> + _git_complete_with_output "$1" "$2"
Where does the second argument come from? Below you run this script
as '${__git_tcsh_completion_script} "${COMMAND_LINE}"', i.e. $2 is
never set. Am I missing something?
> +fi
> diff --git a/contrib/completion/git-completion.tcsh
> b/contrib/completion/git-completion.tcsh
> new file mode 100755
> index 0000000..7b7baea
> --- /dev/null
> +++ b/contrib/completion/git-completion.tcsh
> @@ -0,0 +1,34 @@
> +#!tcsh
> +#
> +# tcsh completion support for core Git.
> +#
> +# Copyright (C) 2012 Marc Khouzam <marc.khouzam@gmail.com>
> +# Distributed under the GNU General Public License, version 2.0.
> +#
> +# This script makes use of the git-completion.bash script to
> +# determine the proper completion for git commands under tcsh.
> +#
> +# To use this completion script:
> +#
> +# 1) Copy both this file and the bash completion script to your
> ${HOME} directory
> +# using the names ${HOME}/.git-completion.tcsh and
> ${HOME}/.git-completion.bash.
> +# 2) Add the following line to your .tcshrc/.cshrc:
> +# source ${HOME}/.git-completion.tcsh
> +
> +# One can change the below line to use a different location
> +set __git_tcsh_completion_script = ${HOME}/.git-completion.bash
> +
> +# Check that the user put the script in the right place
> +if ( ! -e ${__git_tcsh_completion_script} ) then
> + echo "ERROR in git-completion.tcsh script. Cannot find:
> ${__git_tcsh_completion_script}. Git completion will not work."
> + exit
> +endif
> +
> +# Make the script executable if it is not
> +if ( ! -x ${__git_tcsh_completion_script} ) then
> + chmod u+x ${__git_tcsh_completion_script}
> +endif
Not sure about this. If I source a script to provide completion for a
command, then I definitely don't expect it to change file permissions.
However, even if the git completion script is not executable, you can
still run it with 'bash ${__git_tcsh_completion_script}'. This way
neither the user would need to set permissions, not the script would
need to set it behind the users back. Furthermore, this would also
make changing the shebang line unnecessary.
> +
> +complete git 'p/*/`${__git_tcsh_completion_script} "${COMMAND_LINE}"
> | sort | uniq`/'
> +complete gitk 'p/*/`${__git_tcsh_completion_script} "${COMMAND_LINE}"
> | sort | uniq`/'
Is the 'sort | uniq' really necessary? After the completion function
returns Bash automatically sorts the elements in COMPREPLY and removes
any duplicates. Doesn't tcsh do the same? I have no idea about tcsh
completion.
Does the git completion script returns any duplicates at all?
Ambigious refs come to mind, but I just checked that refs completion,
or rather 'git for-each-ref' (the command driving refs completion), is
kind enough to make any ambigious ref names unique (i.e. a branch and
a tag with the same name is listed as 'heads/name' and 'tags/name').
Thanks,
Gábor
^ permalink raw reply
* [PATCH nd/wildmatch] Correct Git's version of isprint and isspace
From: Nguyễn Thái Ngọc Duy @ 2012-11-13 10:46 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, schnhrr, rene.scharfe,
Nguyễn Thái Ngọc Duy
In-Reply-To: <507E9FDE.7080706@cs.tu-berlin.de>
Git's ispace does not include 11 and 12. Git's isprint includes
control space characters (10-13). According to glibc-2.14.1 on C
locale on Linux, this is wrong. This patch fixes it.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
I wrote a small C program to compare the result of all is* functions
that Git replaces against the libc version. These are the only ones that
differ. Which matches what Jan Schönherr commented.
ctype.c | 6 +++---
git-compat-util.h | 11 ++++++-----
2 files changed, 9 insertions(+), 8 deletions(-)
diff --git a/ctype.c b/ctype.c
index 0bfebb4..71311a3 100644
--- a/ctype.c
+++ b/ctype.c
@@ -14,11 +14,11 @@ enum {
P = GIT_PATHSPEC_MAGIC, /* other non-alnum, except for ] and } */
X = GIT_CNTRL,
U = GIT_PUNCT,
- Z = GIT_CNTRL | GIT_SPACE
+ Z = GIT_CNTRL_SPACE
};
-const unsigned char sane_ctype[256] = {
- X, X, X, X, X, X, X, X, X, Z, Z, X, X, Z, X, X, /* 0.. 15 */
+const unsigned int sane_ctype[256] = {
+ X, X, X, X, X, X, X, X, X, Z, Z, Z, Z, Z, X, X, /* 0.. 15 */
X, X, X, X, X, X, X, X, X, X, X, X, X, X, X, X, /* 16.. 31 */
S, P, P, P, R, P, P, P, R, R, G, R, P, P, R, P, /* 32.. 47 */
D, D, D, D, D, D, D, D, D, D, P, P, P, P, P, G, /* 48.. 63 */
diff --git a/git-compat-util.h b/git-compat-util.h
index 02f48f6..4ed3f94 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -474,8 +474,8 @@ extern const char tolower_trans_tbl[256];
#undef ispunct
#undef isxdigit
#undef isprint
-extern const unsigned char sane_ctype[256];
-#define GIT_SPACE 0x01
+extern const unsigned int sane_ctype[256];
+#define GIT_CNTRL_SPACE 0x01
#define GIT_DIGIT 0x02
#define GIT_ALPHA 0x04
#define GIT_GLOB_SPECIAL 0x08
@@ -483,9 +483,10 @@ extern const unsigned char sane_ctype[256];
#define GIT_PATHSPEC_MAGIC 0x20
#define GIT_CNTRL 0x40
#define GIT_PUNCT 0x80
-#define sane_istest(x,mask) ((sane_ctype[(unsigned char)(x)] & (mask)) != 0)
+#define GIT_SPACE 0x100
+#define sane_istest(x,mask) ((sane_ctype[(unsigned int)(x)] & (mask)) != 0)
#define isascii(x) (((x) & ~0x7f) == 0)
-#define isspace(x) sane_istest(x,GIT_SPACE)
+#define isspace(x) sane_istest(x,GIT_SPACE | GIT_CNTRL_SPACE)
#define isdigit(x) sane_istest(x,GIT_DIGIT)
#define isalpha(x) sane_istest(x,GIT_ALPHA)
#define isalnum(x) sane_istest(x,GIT_ALPHA | GIT_DIGIT)
@@ -493,7 +494,7 @@ extern const unsigned char sane_ctype[256];
#define isupper(x) sane_iscase(x, 0)
#define is_glob_special(x) sane_istest(x,GIT_GLOB_SPECIAL)
#define is_regex_special(x) sane_istest(x,GIT_GLOB_SPECIAL | GIT_REGEX_SPECIAL)
-#define iscntrl(x) (sane_istest(x,GIT_CNTRL))
+#define iscntrl(x) (sane_istest(x,GIT_CNTRL | GIT_CNTRL_SPACE))
#define ispunct(x) sane_istest(x, GIT_PUNCT | GIT_REGEX_SPECIAL | \
GIT_GLOB_SPECIAL | GIT_PATHSPEC_MAGIC)
#define isxdigit(x) (hexval_table[x] != -1)
--
1.8.0.rc2.23.g1fb49df
^ permalink raw reply related
* [PATCHv4] replace: parse revision argument for -d
From: Michael J Gruber @ 2012-11-13 10:34 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano
In-Reply-To: <50A22026.60506@drmicha.warpmail.net>
'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:
v4 names the aux variable more concisely and does away with a superfluous
assignment.
builtin/replace.c | 15 +++++++++------
t/t6050-replace.sh | 11 +++++++++++
2 files changed, 20 insertions(+), 6 deletions(-)
diff --git a/builtin/replace.c b/builtin/replace.c
index e3aaf70..398ccd5 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -46,24 +46,27 @@ typedef int (*each_replace_name_fn)(const char *name, const char *ref,
static int for_each_replace_name(const char **argv, each_replace_name_fn fn)
{
- const char **p;
+ const char **p, *full_hex;
char ref[PATH_MAX];
int had_error = 0;
unsigned char sha1[20];
for (p = argv; *p; p++) {
- if (snprintf(ref, sizeof(ref), "refs/replace/%s", *p)
- >= sizeof(ref)) {
- error("replace ref name too long: %.*s...", 50, *p);
+ if (get_sha1(*p, sha1)) {
+ error("Failed to resolve '%s' as a valid ref.", *p);
had_error = 1;
continue;
}
+ full_hex = sha1_to_hex(sha1);
+ snprintf(ref, sizeof(ref), "refs/replace/%s", full_hex);
+ /* read_ref() may reuse the buffer */
+ full_hex = ref + strlen("refs/replace/");
if (read_ref(ref, sha1)) {
- error("replace ref '%s' not found.", *p);
+ error("replace ref '%s' not found.", full_hex);
had_error = 1;
continue;
}
- if (fn(*p, ref, sha1))
+ if (fn(full_hex, ref, sha1))
had_error = 1;
}
return had_error;
diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
index 5c87f28..decdc33 100755
--- a/t/t6050-replace.sh
+++ b/t/t6050-replace.sh
@@ -140,6 +140,17 @@ test_expect_success '"git replace" replacing' '
test "$HASH2" = "$(git replace)"
'
+test_expect_success '"git replace" resolves sha1' '
+ SHORTHASH2=$(git rev-parse --short=8 $HASH2) &&
+ git replace -d $SHORTHASH2 &&
+ git replace $SHORTHASH2 $R &&
+ git show $HASH2 | grep "O Thor" &&
+ test_must_fail git replace $HASH2 $R &&
+ git replace -f $HASH2 $R &&
+ test_must_fail git replace -f &&
+ test "$HASH2" = "$(git replace)"
+'
+
# This creates a side branch where the bug in H2
# does not appear because P2 is created by applying
# H2 and squashing H5 into it.
--
1.8.0.311.gdd08018
^ permalink raw reply related
* Notes in format-patch (was: Re: [PATCHv3] replace: parse revision argument for -d)
From: Michael J Gruber @ 2012-11-13 10:30 UTC (permalink / raw)
To: Michael J Gruber; +Cc: git, Jeff King, Junio C Hamano
In-Reply-To: <a35a8f44b908bded0b475b02e7917011fb3bf90b.1352728712.git.git@drmicha.warpmail.net>
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 (and omit the one after the ---) but may have goofed up a rebase:
...
Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
Notes:
v3 safeguards the hex buffer against reuse
builtin/replace.c | 16 ++++++++++------
...
^ permalink raw reply
* Re: [PATCHv3] replace: parse revision argument for -d
From: Michael J Gruber @ 2012-11-13 10:25 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano
In-Reply-To: <20121112204254.GH4623@sigill.intra.peff.net>
Jeff King venit, vidit, dixit 12.11.2012 21:42:
> On Mon, Nov 12, 2012 at 03:18:02PM +0100, Michael J Gruber wrote:
>
>> '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
>
> Thanks, I don't see any other functional problems.
>
>> diff --git a/builtin/replace.c b/builtin/replace.c
>> index e3aaf70..33e6ec3 100644
>> --- a/builtin/replace.c
>> +++ b/builtin/replace.c
>> @@ -46,24 +46,28 @@ typedef int (*each_replace_name_fn)(const char *name, const char *ref,
>>
>> static int for_each_replace_name(const char **argv, each_replace_name_fn fn)
>> {
>> - const char **p;
>> + const char **p, *q;
>
> I find this readable today, but I wonder if in six months we will wonder
> what in the world "q" means. Maybe "short_refname" or something would be
> appropriate?
That would be sooo inappropriate! ;)
Maybe "full_hex"?
I should also do away with the first replacement which really made sense
in v1 only.
v4 to follow.
Michael
^ permalink raw reply
* Re: RFD: fast-import is picky with author names (and maybe it should - but how much so?)
From: Michael J Gruber @ 2012-11-13 10:15 UTC (permalink / raw)
To: Felipe Contreras
Cc: Jeff King, A Large Angry SCM, Git Mailing List, Junio C Hamano
In-Reply-To: <CAMP44s1gA1P-Lr1M=7RDRqFQmvQAtNnB+yAJfKC1gk3XUjbfCQ@mail.gmail.com>
Felipe Contreras venit, vidit, dixit 12.11.2012 23:47:
> On Mon, Nov 12, 2012 at 10:41 PM, Jeff King <peff@peff.net> wrote:
>> On Sun, Nov 11, 2012 at 07:48:14PM +0100, Felipe Contreras wrote:
>>
>>>> 3. Exporters should not use it if they have any broken-down
>>>> representation at all. Even knowing that the first half is a human
>>>> name and the second half is something else would give it a better
>>>> shot at cleaning than fast-import would get.
>>>
>>> I'm not sure what you mean by this. If they have name and email, then
>>> sure, it's easy.
>>
>> But not as easy as just printing it. What if you have this:
>>
>> name="Peff <angle brackets> King"
>> email="<peff@peff.net>"
>>
>> Concatenating them does not produce a valid git author name. Sending the
>> concatenation through fast-import's cleanup function would lose
>> information (namely, the location of the boundary between name and
>> email).
>
> Right. Unfortunately I'm not aware of any DSCM that does that.
>
>> Similarly, one might have other structured data (e.g., CVS username)
>> where the structure is a useful hint, but some conversion to name+email
>> is still necessary.
>
> CVS might be the only one that has such structured data. I think in
> subversion the username has no meaning. A 'felipec' subversion
> username is as bad as a mercurial 'felipec' username.
In subversion, the username has the clearly defined meaning of being a
username on the subversion host. If the host is, e.g., a sourceforge
site then I can easily look up the user profile and convert the username
into a valid e-mail address (<username>@users.sf.net). That is the
advantage that the exporter (together with user knowledge) has over the
importer.
If the initial clone process aborts after every single "unknown" user
it's no fun, of course. On the other hand, if an incremental clone
(fetch) let's commits with unknown author sneak in it's no fun either
(because I may want to fetch in crontab and publish that converted beast
automatically). That is why I proposed neither approach.
Most conveniently, the export side of a remote helper would
- do "obvious" automatic lossless transformations
- use an author map for other names
- For names not covered by the above (or having an empty map entry):
Stop exporting commits but continue parsing commits and amend the author
map with any unknown usernames (empty entry), and warn the user.
(crontab script can notify me based on the return code.)
If the cloning involves a "foreign clone" (like the hg clone behind the
scene) then the runtime of the second pass should be much smaller. In
principle, one could even store all blobs and trees on the first run and
skip that step on the second, but that would rely on immutability on the
foreign side, so I dunno. (And to check the sha1, we have to get the
blob anyways.)
As for the format for incomplete entries (foo <some@where>), a technical
guideline should suffice for those that follow guidelines.
Michael
^ permalink raw reply
* [PATCH 14/13] test-wildmatch: avoid Windows path mangling
From: Nguyễn Thái Ngọc Duy @ 2012-11-13 10:06 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Johannes Sixt,
Nguyễn Thái Ngọc Duy
In-Reply-To: <7vvcdco1pf.fsf@alter.siamese.dyndns.org>
On Windows, arguments starting with a forward slash is mangled as if
it were full pathname. This causes the patterns beginning with a slash
not to be passed to test-wildmatch correctly. Avoid mangling by never
accepting patterns starting with a slash. Those arguments must be
rewritten with a leading "XXX" (e.g. "/abc" becomes "XXX/abc"), which
will be removed by test-wildmatch itself before feeding the patterns
to wildmatch() or fnmatch().
Reported-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
On Sun, Nov 11, 2012 at 5:47 PM, Junio C Hamano <gitster@pobox.com> wrote:
> The title taken together with the above explanation makes it sound
> as if wildmatch code does not work with the pattern /foo on Windows
> at all and to avoid the issue (instead of fixing the breakage) this
> patch removes such tests....
OK how about this?
t/t3070-wildmatch.sh | 10 +++++-----
test-wildmatch.c | 8 ++++++++
2 files changed, 13 insertions(+), 5 deletions(-)
diff --git a/t/t3070-wildmatch.sh b/t/t3070-wildmatch.sh
index e6ad6f4..3155eab 100755
--- a/t/t3070-wildmatch.sh
+++ b/t/t3070-wildmatch.sh
@@ -74,7 +74,7 @@ match 0 0 'foo/bar' 'foo[/]bar'
match 0 0 'foo/bar' 'f[^eiu][^eiu][^eiu][^eiu][^eiu]r'
match 1 1 'foo-bar' 'f[^eiu][^eiu][^eiu][^eiu][^eiu]r'
match 1 0 'foo' '**/foo'
-match 1 x '/foo' '**/foo'
+match 1 x 'XXX/foo' '**/foo'
match 1 0 'bar/baz/foo' '**/foo'
match 0 0 'bar/baz/foo' '*/foo'
match 0 0 'foo/bar/baz' '**/bar*'
@@ -95,8 +95,8 @@ match 0 0 ']' '[!]-]'
match 1 x 'a' '[!]-]'
match 0 0 '' '\'
match 0 x '\' '\'
-match 0 x '/\' '*/\'
-match 1 x '/\' '*/\\'
+match 0 x 'XXX/\' '*/\'
+match 1 x 'XXX/\' '*/\\'
match 1 1 'foo' 'foo'
match 1 1 '@foo' '@foo'
match 0 0 'foo' '@foo'
@@ -187,8 +187,8 @@ match 0 0 '-' '[[-\]]'
match 1 1 '-adobe-courier-bold-o-normal--12-120-75-75-m-70-iso8859-1' '-*-*-*-*-*-*-12-*-*-*-m-*-*-*'
match 0 0 '-adobe-courier-bold-o-normal--12-120-75-75-X-70-iso8859-1' '-*-*-*-*-*-*-12-*-*-*-m-*-*-*'
match 0 0 '-adobe-courier-bold-o-normal--12-120-75-75-/-70-iso8859-1' '-*-*-*-*-*-*-12-*-*-*-m-*-*-*'
-match 1 1 '/adobe/courier/bold/o/normal//12/120/75/75/m/70/iso8859/1' '/*/*/*/*/*/*/12/*/*/*/m/*/*/*'
-match 0 0 '/adobe/courier/bold/o/normal//12/120/75/75/X/70/iso8859/1' '/*/*/*/*/*/*/12/*/*/*/m/*/*/*'
+match 1 1 'XXX/adobe/courier/bold/o/normal//12/120/75/75/m/70/iso8859/1' 'XXX/*/*/*/*/*/*/12/*/*/*/m/*/*/*'
+match 0 0 'XXX/adobe/courier/bold/o/normal//12/120/75/75/X/70/iso8859/1' 'XXX/*/*/*/*/*/*/12/*/*/*/m/*/*/*'
match 1 0 'abcd/abcdefg/abcdefghijk/abcdefghijklmnop.txt' '**/*a*b*g*n*t'
match 0 0 'abcd/abcdefg/abcdefghijk/abcdefghijklmnop.txtz' '**/*a*b*g*n*t'
diff --git a/test-wildmatch.c b/test-wildmatch.c
index 74c0864..e384c8e 100644
--- a/test-wildmatch.c
+++ b/test-wildmatch.c
@@ -3,6 +3,14 @@
int main(int argc, char **argv)
{
+ int i;
+ for (i = 2; i < argc; i++) {
+ if (argv[i][0] == '/')
+ die("Forward slash is not allowed at the beginning of the\n"
+ "pattern because Windows does not like it. Use `XXX/' instead.");
+ else if (!strncmp(argv[i], "XXX/", 4))
+ argv[i] += 3;
+ }
if (!strcmp(argv[1], "wildmatch"))
return !!wildmatch(argv[3], argv[2], 0);
else if (!strcmp(argv[1], "iwildmatch"))
--
1.8.0.rc2.23.g1fb49df
^ permalink raw reply related
* Re: [PATCH] send-email: add proper default sender
From: Felipe Contreras @ 2012-11-13 9:06 UTC (permalink / raw)
To: Jeff King; +Cc: git, Thomas Rast, Junio C Hamano, Jonathan Nieder
In-Reply-To: <20121113074720.GA18746@sigill.intra.peff.net>
On Tue, Nov 13, 2012 at 8:47 AM, Jeff King <peff@peff.net> wrote:
> But I still don't see how that has anything to do with what send-email
> does or should do. That is why I said "strawman" above. You seem to
> think I am saying that send-email should use the system that generated
> those broken names, when I am saying the opposite.
No, I'm saying none should use that system, and that in fact 'git
commit' should be stricter... both should be stricter.
> 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.
>> I'm not talking about git send-email, I'm talking about your comment
>> 'it has always been the case that you can use git without setting
>> user.*', which has caused issues with wrong author/commmitter names in
>> commits, and will probably continue to do so.
>
> The second half of that sentence that you quoted above is "...instead
> only using the environment." As in, environment variables like
> GIT_AUTHOR_EMAIL, GIT_COMMITTER_EMAIL, and EMAIL. _Not_ implicit
> generation of the email from the username and hostname.
That sentence was *not* about 'git send-email', it was about git in
general, and 'git commit' is perfectly happy with implicit generation
of the email from the username and hostname.
I don't thin 'git commit' should do that, and I don't think 'git
send-email' should do that. I'm criticizing the whole approach.
> I am tempted to fault myself for not communicating well, but I feel like
> I have made that point at least 3 times in this conversation now. Is
> that the source of the confusion?
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.
> And you will survive if upstream git (whether it is me today or Junio
> tomorrow) does not pick up your patch.
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.
> I remember writing you a long
> email recently about how one of the responsibilities of the maintainer
> is to balance features versus regressions. I'll not bother repeating
> myself here.
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.
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?
I prefer to concentrate on real issues, but that's just me.
> 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.
> 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)
> 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".
But that's not going to happen. And in the unlikely event that it
does, it's not going to be a major issue.
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?
>> But let's look at the current situation closely:
>>
>> PERL5LIB=~/dev/git/perl ./git-send-email.perl --confirm=always -1
>>
>> 1) No information at all
>>
>> fatal: empty ident name (for <felipec@nysa.(none)>) not allowed
>
> That is dependent on your system. If you have a non-empty name in your
> GECOS field, and if your machine has a FQDN, it will currently work (and
> prompt).
Yes, that's point 2).
>> 2) Full Name + full hostname
>>
>> Who should the emails appear to be from? [Felipe Contreras
>> <felipec@nysa.felipec.org>]
>>
>> That's right, ident doesn't fail, and that's not the mail address I
>> specified, it's *implicit*.
>
> Right. I never said it did. I said it currently rejected obviously bogus
> stuff (like the ".(none)" above) due to IDENT_STRICT, but currently
> allowed implicit definitions. And I also said that if we get rid of the
> prompt, we should disallow implicit definitions like this, because the
> prompt is the safety valve on sending out mails with broken from
> addresses. I even wrote a patch that let you find out whether the ident
> was generated implicitly.
Correct.
>> 3) Full Name + EMAIL
>>
>> Who should the emails appear to be from? [Felipe Contreras
>> <felipe.contreras@gmail.com>]
>
> Which sounds fine to me. EMAIL is considered explicit, and I have not
> seen any evidence that people are putting bogus values in their EMAIL
> variable and complaining that it is git's fault for respecting it.
Agreed.
>> 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).
Yeah, but clearly the intention of the code was to use the committer
if the author wasn't available, which is the case here.
>> 5.1) GIT_COMMITER without anything else
>>
>> fatal: empty ident name (for <felipec@nysa.(none)>) not allowed
>> var GIT_AUTHOR_IDENT: command returned error: 128
>
> Right. Same bug as above.
No, this is a different bug.
The bug above 5) is here:
$sender = $repoauthor || $repocommitter || '';
$repoauthor will always evaluate to true.
This one 5.1) is there:
($repoauthor) = Git::ident_person(@repo, 'author');
($repocommitter) = Git::ident_person(@repo, 'committer'); <-
>> So $repoauthor || $repocommiter is pointless.
>
> Agreed.
Good.
>> 6) GIT_AUTHOR
>>
>> Who should the emails appear to be from? [Felipe Contreras 4th
>> <felipe.contreras+4@gmail.com>]
>
> Right, that's what I'd expect.
You mean without the input question?
>> 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):
($repoauthor) = Git::ident_person(@repo, 'author'); <- here
($repocommitter) = Git::ident_person(@repo, 'committer');
>> 4) config user
>>
>> From: Felipe Contreras 2nd <felipe.contreras+2@gmail.com>
>
> OK.
>
>> 5) GIT_COMMITTER
>>
>> From: Felipe Contreras 2nd <felipe.contreras+2@gmail.com>
>
> OK.
>
>> 6) GIT_AUTHOR
>>
>> From: Felipe Contreras 2nd <felipe.contreras+2@gmail.com>
>
> Doesn't that seem like a regression? It used to use a different address,
> and in every other git program, the environment takes precedence over
> config.
Yes, it is a regression (that won't affect anybody).
>> 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().
>> 6.1) GIT_AUTHOR without anything else
>>
>> Even if the previous problem was solved:
>>
>> export GIT_AUTHOR_NAME='Felipe Contreras 4th'; export
>> GIT_AUTHOR_EMAIL='felipe.contreras+4@gmail.com'
>> ./git var GIT_EXPLICIT_IDENT
>> 0
>>
>> No explicit ident? This is most certainly not what the user would expect.
>
> Yes, it looks like we do not set up the explicit ident flags when
> parsing the author. So my patch is insufficient.
Indeed.
>> 5.2) GIT_COMMITTER with Full Name and full hostname
>>
>> export GIT_COMMITTER_NAME='Felipe Contreras 3nd'; export
>> GIT_COMMITTER_EMAIL='felipe.contreras+3@gmail.com'
>> ./git var GIT_EXPLICIT_IDENT
>> 1
>>
>> From: Felipe Contreras <felipec@nysa.felipec.org>
>>
>> It is explicit, yeah, but 'git send-email' would not be picking the
>> committer, it would pick the author.
>
> Yep.
>
> 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).
I think strictess should be tied to explicitness, and 'git var' should
error out, and not die.
>> > I tried to help you by pointing you in the right direction and even
>> > providing a sample "git var" patch.
>>
>> Are you 100% sure that was the right direction?
>
> 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.
>> I think the right approach is more along these lines:
>
> I think that is moving in the right direction, but...
>
>> --- a/ident.c
>> +++ b/ident.c
>> @@ -291,9 +291,9 @@ const char *fmt_ident(const char *name, const char *email,
>> }
>>
>> if (strict && email == git_default_email.buf &&
>> - strstr(email, "(none)")) {
>> + !(user_ident_explicitly_given & IDENT_MAIL_GIVEN)) {
>> fputs(env_hint, stderr);
>> - die("unable to auto-detect email address (got '%s')", email);
>> + die("no explicit email address");
>
> I think this needs to be optional, otherwise you are breaking callers
> who use IDENT_STRICT but are OK with the implicit ident (e.g.,
> commit, format-patch with threading).
>
> You can argue whether "git commit" should disallow such addresses, but
> that is a separate topic from how send-email should behave.
Yes, that's exactly what I would argue.
>> Not only will this fix 'git send-email', but it will also fix 'git
>> commit' so that we don't end up with authors such as 'Felipe Contreras
>> <felipec@nysa.felipec.org>' ever again.
>
> 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?
No, but it can also be considered a bug... do we have any data on how
many people are being affected by this? If the '(none)' commits are
any indication of it, probably a lot. At least the ones that do have a
fqdn.
>> > But it is not my itch to scratch.
>>
>> Suit yourself, it's only git users that would get hurt. I can always
>> use my own 'git send-email' (as I am doing right now).
>
> 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.
> 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.
I'm just trying to be pragmatic. I don't see the point in wasting my
time for people that don't exist. As I said, I don't think anybody
would be hit by this.
> And now I have wasted a large chunk of the evening responding to you,
> neither accomplishing my other tasks nor polishing this topic. I do not
> mind reviewing patches or responding to discussions, nor do I consider
> them time wasted; they are an important part of the development process.
> 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".
Fixing a regression that nobody would notice is not my itch either,
yet the patch I sent above does it, and it even fixes 'git commit'
(IMO). But it's also not good enough.
I scratched my itch with the original patch, anything after that is to
help other people.
I think it would be much easier to just remove the question input. The
only "regression" would be the people that have a fqdn and full name
_and_ expect the question. But Junio suggested to just die in those
cases, and trying to send an email that would probably fail is not
that different.
Fixing all the var and ident infrastructure seems way, *way* far from
what I intended to do.
Cheers.
--
Felipe Contreras
^ permalink raw reply
* [PATCH 3/3] fix corner case for relative submodule path calculation
From: Heiko Voigt @ 2012-11-13 8:35 UTC (permalink / raw)
To: Jeff King; +Cc: Jeffrey S. Haemer, Jens Lehmann, Git Issues
In-Reply-To: <20121113083233.GA38188@book.hvoigt.net>
A trailing /. for the superprojects origin is treated as
a full path component. This is wrong. Lets add a test and
fix this.
Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
git-submodule.sh | 22 ++++++++++++++++++++++
t/t7400-submodule-basic.sh | 44 ++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 66 insertions(+)
diff --git a/git-submodule.sh b/git-submodule.sh
index ab6b110..9f61a9c 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -69,6 +69,28 @@ resolve_relative_url ()
;;
esac
+ # strip one dot path components
+ tempurl="$remoteurl"
+ remoteurl=
+ sep=
+ while test -n "$tempurl"
+ do
+ case "$tempurl" in
+ */.)
+ tempurl="${tempurl%/.}"
+ ;;
+ ?*/*)
+ remoteurl="${tempurl##*/}$sep$remoteurl"
+ tempurl="${tempurl%/*}"
+ sep=/
+ ;;
+ *)
+ remoteurl="$tempurl$sep$remoteurl"
+ tempurl=
+ ;;
+ esac
+ done
+
while test -n "$url"
do
case "$url" in
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 3c2afa6..1b4cc00 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -518,6 +518,50 @@ test_expect_success 'subrepo is NOT considered a relative path"' '
)
'
+test_expect_success '../subrepo works with absolute local path - "$submodurl/repo/."' '
+ (
+ cd reltest &&
+ cp pristine-.git-config .git/config &&
+ cp pristine-.gitmodules .gitmodules &&
+ git config remote.origin.url "$submodurl/repo/." &&
+ git submodule init &&
+ test "$(git config submodule.sub.url)" = "$submodurl/subrepo"
+ )
+'
+
+test_expect_success '../subrepo works with absolute local path - "$submodurl/repo/./"' '
+ (
+ cd reltest &&
+ cp pristine-.git-config .git/config &&
+ cp pristine-.gitmodules .gitmodules &&
+ git config remote.origin.url "$submodurl/repo/./" &&
+ git submodule init &&
+ test "$(git config submodule.sub.url)" = "$submodurl/subrepo"
+ )
+'
+
+test_expect_success '../subrepo works with absolute local path - "$submodurl/./repo/."' '
+ (
+ cd reltest &&
+ cp pristine-.git-config .git/config &&
+ cp pristine-.gitmodules .gitmodules &&
+ git config remote.origin.url "$submodurl/./repo/." &&
+ git submodule init &&
+ test "$(git config submodule.sub.url)" = "$submodurl/subrepo"
+ )
+'
+
+test_expect_success '../subrepo works with absolute local path - "$submodurl/././repo/."' '
+ (
+ cd reltest &&
+ cp pristine-.git-config .git/config &&
+ cp pristine-.gitmodules .gitmodules &&
+ git config remote.origin.url "$submodurl/././repo/." &&
+ git submodule init &&
+ test "$(git config submodule.sub.url)" = "$submodurl/subrepo"
+ )
+'
+
test_expect_success '../subrepo works with URL - ssh://hostname/repo' '
(
cd reltest &&
--
1.8.0.3.gaed4666
^ permalink raw reply related
* [PATCH 2/3] ensure that relative submodule url needs ./ or ../
From: Heiko Voigt @ 2012-11-13 8:35 UTC (permalink / raw)
To: Jeff King; +Cc: Jeffrey S. Haemer, Jens Lehmann, Git Issues
In-Reply-To: <20121113083233.GA38188@book.hvoigt.net>
Even though a relative path can be without them the
documentation explicitely talks about them. Lets ensure
that behavior with a test.
Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
t/t7400-submodule-basic.sh | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 5397037..3c2afa6 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -506,6 +506,18 @@ test_expect_success 'set up for relative path tests' '
)
'
+test_expect_success 'subrepo is NOT considered a relative path"' '
+ (
+ cd reltest &&
+ cp pristine-.git-config .git/config &&
+ cp pristine-.gitmodules .gitmodules &&
+ git config -f .gitmodules submodule.sub.url "subrepo" &&
+ git config remote.origin.url "$submodurl" &&
+ git submodule init &&
+ test "$(git config submodule.sub.url)" = subrepo
+ )
+'
+
test_expect_success '../subrepo works with URL - ssh://hostname/repo' '
(
cd reltest &&
--
1.8.0.3.gaed4666
^ permalink raw reply related
* [PATCH 1/3] Fix relative submodule setup of submodule tests
From: Heiko Voigt @ 2012-11-13 8:34 UTC (permalink / raw)
To: Jeff King; +Cc: Jeffrey S. Haemer, Jens Lehmann, Git Issues
In-Reply-To: <20121113083233.GA38188@book.hvoigt.net>
If a remote is configured in a superproject relative submodule urls
should be relative to that remote. Since we have a bug in relative
path calculation for superproject paths that contain a "/." using
../submodule was accepted here. We are going to fix this behavior so
we first need to correct these tests.
Later tests expect the submodules origin to be in a directory underneath
the tests root. Lets remove the origin from super (which points directly
at the tests root directory) to keep these tests expectations.
Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
t/t7403-submodule-sync.sh | 2 ++
t/t7406-submodule-update.sh | 2 ++
t/t7407-submodule-foreach.sh | 2 ++
t/t7506-status-submodule.sh | 2 ++
4 files changed, 8 insertions(+)
diff --git a/t/t7403-submodule-sync.sh b/t/t7403-submodule-sync.sh
index 524d5c1..b310a58 100755
--- a/t/t7403-submodule-sync.sh
+++ b/t/t7403-submodule-sync.sh
@@ -18,6 +18,8 @@ test_expect_success setup '
git clone . super &&
git clone super submodule &&
(cd super &&
+ # relative submodule urls relate to this folder not the remotes
+ git remote rm origin &&
git submodule add ../submodule submodule &&
test_tick &&
git commit -m "submodule"
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 1542653..f3628c9 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -32,6 +32,8 @@ test_expect_success 'setup a submodule tree' '
git clone super merging &&
git clone super none &&
(cd super &&
+ # relative submodule urls relate to this folder not the remotes
+ git remote rm origin &&
git submodule add ../submodule submodule &&
test_tick &&
git commit -m "submodule" &&
diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
index 9b69fe2..99956a6 100755
--- a/t/t7407-submodule-foreach.sh
+++ b/t/t7407-submodule-foreach.sh
@@ -21,6 +21,8 @@ test_expect_success 'setup a submodule tree' '
git clone super submodule &&
(
cd super &&
+ # relative submodule urls relate to this folder not the remotes
+ git remote rm origin &&
git submodule add ../submodule sub1 &&
git submodule add ../submodule sub2 &&
git submodule add ../submodule sub3 &&
diff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh
index d31b34d..9021b1a 100755
--- a/t/t7506-status-submodule.sh
+++ b/t/t7506-status-submodule.sh
@@ -203,6 +203,8 @@ test_expect_success 'status with merge conflict in .gitmodules' '
test_create_repo_with_commit sub2 &&
(
cd super &&
+ # relative submodule urls relate to this folder not the remotes
+ git remote rm origin &&
prev=$(git rev-parse HEAD) &&
git checkout -b add_sub1 &&
git submodule add ../sub1 &&
--
1.8.0.3.gaed4666
^ permalink raw reply related
* [PATCH 0/3] fix cloning superprojects from "."
From: Heiko Voigt @ 2012-11-13 8:32 UTC (permalink / raw)
To: Jeff King; +Cc: Jeffrey S. Haemer, Jens Lehmann, Git Issues
In-Reply-To: <20121109184225.GA1190@book.hvoigt.net>
Hi,
On Fri, Nov 09, 2012 at 07:42:26PM +0100, Heiko Voigt wrote:
> Since this is a change in behaviour I would like to further think about
> the implications this brings if we fix this. Not sure how many people
> clone from ".". The correct behavior (as documented) is the one you
> introduce with your patch. If we decide to fix this we should also correct
> the path calculation in git-submodule.sh.
Ok I think this corner case is not that commonly used since most people
work with remote remotes which you can not cd into to clone from ".".
Here is a patch series to clean this handling up.
Cheers Heiko
Heiko Voigt (3):
Fix relative submodule setup of submodule tests
ensure that relative submodule url needs ./ or ../
fix corner case for relative submodule path calculation
git-submodule.sh | 22 +++++++++++++++++
t/t7400-submodule-basic.sh | 56 ++++++++++++++++++++++++++++++++++++++++++++
t/t7403-submodule-sync.sh | 2 ++
t/t7406-submodule-update.sh | 2 ++
t/t7407-submodule-foreach.sh | 2 ++
t/t7506-status-submodule.sh | 2 ++
6 files changed, 86 insertions(+)
--
1.8.0.3.gaed4666
^ permalink raw reply
* RE: [BUG] gitweb: XSS vulnerability of RSS feed
From: Pyeron, Jason J CTR (US) @ 2012-11-13 8:31 UTC (permalink / raw)
To: git@vger.kernel.org
In-Reply-To: <CAM9Z-n=6xsC7yiKJ+NU-CxNPxEXWmJzvXLUocgZgWPQnuK6G4Q@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1644 bytes --]
> -----Original Message-----
> From: Drew Northup
> Sent: Monday, November 12, 2012 1:56 PM
>
> On Sun, Nov 11, 2012 at 6:28 PM, glpk xypron <xypron.glpk@gmx.de>
> wrote:
> > Gitweb can be used to generate an RSS feed.
> >
> > Arbitrary tags can be inserted into the XML document describing
> > the RSS feed by careful construction of the URL.
> >
> > Example
> >
> http://server/?p=project.git&a=rss&f=</title><script>alert(document.coo
> kie)</script><title>
> >
> > The generated XML contains
> > <script>alert(document.cookie)</script>
This is just an example.
> >
> > Depending on the system used to render the XML this might lead
> > to the execution of javascript in the security context of the
> > gitweb server pages.
> >
> > Please, escape all URL parameters.
We should look for the general entry points, not the script tag.
> >
> > Version tested:
> > gitweb v.1.8.0.dirty with git 1.7.2.5
> >
> > Best regards
> >> Heinrich Schuchardt
>
> Something like this may be useful to defuse the "file" parameter, but
> I presume a more definitive fix is in order...
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 10ed9e5..af93e65 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -1447,6 +1447,10 @@ sub validate_pathname {
> if ($input =~ m!\0!) {
> return undef;
> }
> + # No XSS <script></script> inclusions
### not real perl...
foreach $xml in ( <, >, &, ...)
{
$input=~s/$xml/xmlescape{$xml}/g;
}
### "<" => "<"
> + if ($input =~ m!(<script>)(.*)(</script>)!){
> + return undef;
> + }
> return $input;
> }
>
[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5615 bytes --]
^ permalink raw reply
* Re: [PATCH] send-email: add proper default sender
From: Jeff King @ 2012-11-13 7:47 UTC (permalink / raw)
To: Felipe Contreras; +Cc: git, Thomas Rast, Junio C Hamano, Jonathan Nieder
In-Reply-To: <CAMP44s1w3oZhEUM-cnO=kECH2bhdOTGVuKy8JS4uhWFbA_oi3w@mail.gmail.com>
On Tue, Nov 13, 2012 at 07:42:58AM +0100, Felipe Contreras wrote:
> >> > No, it's not. Those broken names do not come from the environment, but
> >> > from our last-resort guess of the hostname.
> >>
> >> That depends how you define environment, but fine, the point is that
> >> it happens.
> >
> > If you have a strawman definition that does not have anything to do with
> > what I said in my original email, then yes, it could happen.
>
> It happens, I've seen commits with (none) not that long ago.
There was a bug that caused the check to fail in some cases. I fixed it
in f20f387 (this July).
But I still don't see how that has anything to do with what send-email
does or should do. That is why I said "strawman" above. You seem to
think I am saying that send-email should use the system that generated
those broken names, when I am saying the opposite.
> > But as I said already, "git var" uses IDENT_STRICT and will not
> > allow such broken names.
>
> Since 1.7.11, sure. But not everyone is using such a recent version of
> git, and people with fully qualified domains would still get unwanted
> behavior.
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.
> > Did you read my email? I explicitly proposed that we would _not_ allow
> > send-email to use implicit email addresses constructed in that way.
>
> I'm not talking about git send-email, I'm talking about your comment
> 'it has always been the case that you can use git without setting
> user.*', which has caused issues with wrong author/commmitter names in
> commits, and will probably continue to do so.
The second half of that sentence that you quoted above is "...instead
only using the environment." As in, environment variables like
GIT_AUTHOR_EMAIL, GIT_COMMITTER_EMAIL, and EMAIL. _Not_ implicit
generation of the email from the username and hostname.
I am tempted to fault myself for not communicating well, but I feel like
I have made that point at least 3 times in this conversation now. Is
that the source of the confusion?
> > Sorry, but that is not how things work on this project. You do not get
> > to cause regressions because you are too lazy to implement the feature
> > _you_ want in a way that does not break other people.
>
> That doesn't change the fact that they would survive, and the fact
> that those users don't actually exist.
And you will survive if upstream git (whether it is me today or Junio
tomorrow) does not pick up your patch. I remember writing you a long
email recently about how one of the responsibilities of the maintainer
is to balance features versus regressions. I'll not bother repeating
myself here.
As for whether they exist, what data do you have? Are you aware that the
test suite, for example, relies on setting GIT_AUTHOR_NAME but not
having any user.* config? 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?
> But let's look at the current situation closely:
>
> PERL5LIB=~/dev/git/perl ./git-send-email.perl --confirm=always -1
>
> 1) No information at all
>
> fatal: empty ident name (for <felipec@nysa.(none)>) not allowed
That is dependent on your system. If you have a non-empty name in your
GECOS field, and if your machine has a FQDN, it will currently work (and
prompt).
> 2) Full Name + full hostname
>
> Who should the emails appear to be from? [Felipe Contreras
> <felipec@nysa.felipec.org>]
>
> That's right, ident doesn't fail, and that's not the mail address I
> specified, it's *implicit*.
Right. I never said it did. I said it currently rejected obviously bogus
stuff (like the ".(none)" above) due to IDENT_STRICT, but currently
allowed implicit definitions. And I also said that if we get rid of the
prompt, we should disallow implicit definitions like this, because the
prompt is the safety valve on sending out mails with broken from
addresses. I even wrote a patch that let you find out whether the ident
was generated implicitly.
> 3) Full Name + EMAIL
>
> Who should the emails appear to be from? [Felipe Contreras
> <felipe.contreras@gmail.com>]
Which sounds fine to me. EMAIL is considered explicit, and I have not
seen any evidence that people are putting bogus values in their EMAIL
variable and complaining that it is git's fault for respecting it.
> 4) config user
>
> Who should the emails appear to be from? [Felipe Contreras 2nd
> <felipe.contreras+2@gmail.com>]
OK.
> 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).
> 5.1) GIT_COMMITER without anything else
>
> fatal: empty ident name (for <felipec@nysa.(none)>) not allowed
> var GIT_AUTHOR_IDENT: command returned error: 128
Right. Same bug as above.
> So $repoauthor || $repocommiter is pointless.
Agreed.
> 6) GIT_AUTHOR
>
> Who should the emails appear to be from? [Felipe Contreras 4th
> <felipe.contreras+4@gmail.com>]
Right, that's what I'd expect.
> 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.
> 4) config user
>
> From: Felipe Contreras 2nd <felipe.contreras+2@gmail.com>
OK.
> 5) GIT_COMMITTER
>
> From: Felipe Contreras 2nd <felipe.contreras+2@gmail.com>
OK.
> 6) GIT_AUTHOR
>
> From: Felipe Contreras 2nd <felipe.contreras+2@gmail.com>
Doesn't that seem like a regression? It used to use a different address,
and in every other git program, the environment takes precedence over
config.
> 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.
> 6.1) GIT_AUTHOR without anything else
>
> Even if the previous problem was solved:
>
> export GIT_AUTHOR_NAME='Felipe Contreras 4th'; export
> GIT_AUTHOR_EMAIL='felipe.contreras+4@gmail.com'
> ./git var GIT_EXPLICIT_IDENT
> 0
>
> No explicit ident? This is most certainly not what the user would expect.
Yes, it looks like we do not set up the explicit ident flags when
parsing the author. So my patch is insufficient.
> 5.2) GIT_COMMITTER with Full Name and full hostname
>
> export GIT_COMMITTER_NAME='Felipe Contreras 3nd'; export
> GIT_COMMITTER_EMAIL='felipe.contreras+3@gmail.com'
> ./git var GIT_EXPLICIT_IDENT
> 1
>
> From: Felipe Contreras <felipec@nysa.felipec.org>
>
> It is explicit, yeah, but 'git send-email' would not be picking the
> committer, it would pick the author.
Yep.
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).
> > I tried to help you by pointing you in the right direction and even
> > providing a sample "git var" patch.
>
> Are you 100% sure that was the right direction?
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 the right approach is more along these lines:
I think that is moving in the right direction, but...
> --- a/ident.c
> +++ b/ident.c
> @@ -291,9 +291,9 @@ const char *fmt_ident(const char *name, const char *email,
> }
>
> if (strict && email == git_default_email.buf &&
> - strstr(email, "(none)")) {
> + !(user_ident_explicitly_given & IDENT_MAIL_GIVEN)) {
> fputs(env_hint, stderr);
> - die("unable to auto-detect email address (got '%s')", email);
> + die("no explicit email address");
I think this needs to be optional, otherwise you are breaking callers
who use IDENT_STRICT but are OK with the implicit ident (e.g.,
commit, format-patch with threading).
You can argue whether "git commit" should disallow such addresses, but
that is a separate topic from how send-email should behave.
> Not only will this fix 'git send-email', but it will also fix 'git
> commit' so that we don't end up with authors such as 'Felipe Contreras
> <felipec@nysa.felipec.org>' ever again.
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?
> > But it is not my itch to scratch.
>
> Suit yourself, it's only git users that would get hurt. I can always
> use my own 'git send-email' (as I am doing right now).
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.
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.
And now I have wasted a large chunk of the evening responding to you,
neither accomplishing my other tasks nor polishing this topic. I do not
mind reviewing patches or responding to discussions, nor do I consider
them time wasted; they are an important part of the development process.
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".
-Peff
^ permalink raw reply
* Re: [PATCH] send-email: add proper default sender
From: Felipe Contreras @ 2012-11-13 7:18 UTC (permalink / raw)
To: Jeff King; +Cc: git, Thomas Rast, Junio C Hamano, Jonathan Nieder
In-Reply-To: <CAMP44s1w3oZhEUM-cnO=kECH2bhdOTGVuKy8JS4uhWFbA_oi3w@mail.gmail.com>
On Tue, Nov 13, 2012 at 7:42 AM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> 6) GIT_AUTHOR
>
> Who should the emails appear to be from? [Felipe Contreras 4th
> <felipe.contreras+4@gmail.com>]
>
> 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
This was supposed to be above (before my change).
--
Felipe Contreras
^ permalink raw reply
* Re: [PATCH 1/4] remote-hg: add missing config for basic tests
From: Felipe Contreras @ 2012-11-13 7:12 UTC (permalink / raw)
To: Jeff King; +Cc: git, Ramkumar Ramachandra
In-Reply-To: <20121113054826.GC10995@sigill.intra.peff.net>
On Tue, Nov 13, 2012 at 6:48 AM, Jeff King <peff@peff.net> wrote:
> Any objection to me marking it up as I apply?
Nope.
--
Felipe Contreras
^ 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