Git development
 help / color / mirror / Atom feed
* Re: jgit and ignore
From: Tor Arne Vestbø @ 2009-03-01 14:21 UTC (permalink / raw)
  To: Jon Smirl; +Cc: Ferry Huberts (Pelagic), Shawn O. Pearce, Git Mailing List
In-Reply-To: <9e4733910903010608u1777f0d4j843f12551154f962@mail.gmail.com>

Jon Smirl wrote:
> I have a C/C++ perspective open right now. Navigator is on the left,
> editor in the middle, outline on the right. In the navigator there are
> several files that should be ignored. I'd like to right click on them
> and select team/ignore. When I do that they will be ignored in git and
> also disappear from my navigator.

That would be Team->Add to .gitignore, as Eclipse does not have a way to
add files to the global ignore from context menus, but I agree, we
should have something like that eventually.

If we were to also remove ignored files from the views we would have to
either add a View Filter for .git-ignored files, or for globally ignored
files. Good idea for enhancement, please report in the issue tracker:

    http://code.google.com/p/egit/issues/

> I also don't like how I have a global .git for all of my projects
> instead of a .git for each project individually. (Did I select that
> when I first installed egit and didn't know what I was doing?) Now I'm
> in a mess and can't publish individual projects.

That's a result of choosing "Create repository in project's parent
directory" when you shared the project in Eclipse.

Perhaps we should try to detect if the project is a Java (/JDT) project,
or otherwise likely to be a "child"-project, where it makes sense to
have the repository in the parent directory?

Tor Arne

^ permalink raw reply

* Re: jgit and ignore
From: Jon Smirl @ 2009-03-01 14:08 UTC (permalink / raw)
  To: Ferry Huberts (Pelagic); +Cc: Shawn O. Pearce, Git Mailing List
In-Reply-To: <49AA884D.1050806@pelagic.nl>

On Sun, Mar 1, 2009 at 8:06 AM, Ferry Huberts (Pelagic)
<ferry.huberts@pelagic.nl> wrote:
> Jon Smirl wrote:
>> On Sun, Mar 1, 2009 at 5:11 AM, Ferry Huberts (Pelagic)
>> <ferry.huberts@pelagic.nl> wrote:
>>> Shawn O. Pearce wrote:
>>>> Jon Smirl <jonsmirl@gmail.com> wrote:
>>>>> I'm using jgit in eclipse. Works great for me.
>>>> Yay!
>>>>
>>>>> I have a couple of generated files in my working directory. There
>>>>> doesn't seem to be any UI for ignoring them. Is it there and I just
>>>>> can't find it?
>>>> EGit doesn't (yet) honor the .gitignore files like it should. Someone
>>>> (Ferry i-forget-the-rest-of-his-name) is working on adding ignore
>>>> support and has patches in flight for at least some of it.
>>>>
>>> Ferry i-do-remember-my-name Huberts is working on it :-)
>>>
>>> I have most of it working in a basic form already but am currently
>>> refactoring things to take care of some nasty little details.
>>> Expect something to arrive within (my best guesstimate) about 3 to 4
>>> weeks. after next week I'll be skiing for a week, so no coding then :-)
>>>
>>> For the new functionality:
>>> You don't really need a UI: just add a .gitignore file with a pattern
>>> and the plugin will pick it up and show you what is ignored by means of
>>> a nice little decoration.
>>
>> I expected it to work by right clicking the file and picking
>> team/ignore. This would add the file name .gitignore and automatically
>> add .gitignore to my commit. It would also alter the eclipse filter to
>> make the file disappear in the eclipse browser.
>>
>
> that'll come later, first we need to ignore the same files as git :-)
>
> which eclipse browser?

I have a C/C++ perspective open right now. Navigator is on the left,
editor in the middle, outline on the right. In the navigator there are
several files that should be ignored. I'd like to right click on them
and select team/ignore. When I do that they will be ignored in git and
also disappear from my navigator.

I also don't like how I have a global .git for all of my projects
instead of a .git for each project individually. (Did I select that
when I first installed egit and didn't know what I was doing?) Now I'm
in a mess and can't publish individual projects.

>
> BTW. the plugin will not look at the eclipse filter for ignore files
> once I've finished the implementation. that's the only way to make sure
> that we ignore resources in exactly the same way as git does: git only
> ignores on the basis of .gitignore files, command line options (we do
> not have those for the plugin), and the info/exclude file in the repository.
>



-- 
Jon Smirl
jonsmirl@gmail.com

^ permalink raw reply

* Re: [PATCH] send-email: add --confirm option
From: Jay Soffian @ 2009-03-01 14:05 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Nanako Shiraishi, Paul Gortmaker
In-Reply-To: <7vhc2d8vjk.fsf@gitster.siamese.dyndns.org>

On Sun, Mar 1, 2009 at 4:03 AM, Junio C Hamano <gitster@pobox.com> wrote:
> It is hard to judge if this is a good thing to do _at this point_.  Those
> who complained may welcome it but that is hardly the point.  You need to
> convince those who stayed silent (because they thought the default won't
> change and were not paying attention) that it is a good change.

No, I won't. This is not my itch, I don't know who those users are,
and I think I provided any such users several easy ways out:

1) They can type "a" after the first prompt.
2) They can git config --global sendemail.confirm never
3) They won't even notice if they're using any suppresscc option, such
as yourself.

I know you're sensitive to existing users after getting burned by the
dashless-git issue, but I think you're over applying that lesson in
*this* case.

> Especially the changes (not additions) to existing tests worries me; each
> change to the existing one is a demonstration of breaking existing users.

There were two classes of changes:

1) Many of the tests Cc'd w/o warning. Those now prompt. Hence the
"git config sendemail.suppress never" at the top.

2) Since I added (1), the "echo y |" in the other tests was redundant.
I believe had a no-confirm option been available in the past, that the
test authors would not have used "echo y". And I thought it confusing
to leave the "echo y|" as someone might ask "if it's not supposed to
confirm, why is there an echo y here?"

> You cannot retrofit safety by disallowing things once you used to allow,
> without upsetting existing users.

And you hamper your ability to improve the product by not allowing
even minor backward incompatible changes.

But I anticipated this objection and I thought very much about how an
existing user might hypothetically be upset, while accommodating the
very real concern of a new user.

Aside, when I upgrade a product, I expect it to change in some ways.
If I really don't want change, I don't upgrade.

On balance, I think this change will be silently appreciated by many
more users than the few, if any, vocal objectors.

>> @@ -1094,13 +1119,10 @@ foreach my $t (@files) {
>>       $message_id = undef;
>>  }
>>
>> -if ($compose) {
>> -     cleanup_compose_files();
>> -}
>> +cleanup_compose_files();
>>
>>  sub cleanup_compose_files() {
>> -     unlink($compose_filename, $compose_filename . ".final");
>> -
>> +     unlink($compose_filename, $compose_filename . ".final") if $compose;
>>  }
>
> Does this hunk have anything to do with this topic, or is it just a churn
> that does not change any behaviour?

The former. You'll notice there is a "quit" option in the prompt (in
addition to yes/no/all). The quit option needs to call
cleanup_compose_files(), so it now has two callers.

Instead of both callers having to do this:

  if ($compose) {
       cleanup_compose_files();
  }

I just moved the if compose test into the function so each caller can
simply ask:

  cleanup_compose_files();


>> @@ -175,15 +180,13 @@ test_set_editor "$(pwd)/fake-editor"
>>
>>  test_expect_success '--compose works' '
>>       clean_fake_sendmail &&
>> -     echo y | \
>> -             GIT_SEND_EMAIL_NOTTY=1 \
>> -             git send-email \
>> -             --compose --subject foo \
>> -             --from="Example <nobody@example.com>" \
>> -             --to=nobody@example.com \
>> -             --smtp-server="$(pwd)/fake.sendmail" \
>> -             $patches \
>> -             2>errors
>> +     git send-email \
>> +     --compose --subject foo \
>> +     --from="Example <nobody@example.com>" \
>> +     --to=nobody@example.com \
>> +     --smtp-server="$(pwd)/fake.sendmail" \
>> +     $patches \
>> +     2>errors
>>  '
>
> How would test this break without this hunk?  "echo" dies of sigpipe, or
> something?
>
> I am not objecting to this particular change; just asking why this change
> is here.  "It does not break, but the command shouldn't ask for
> confirmation, and giving 'y' into it is unnecessary" is a perfectly
> acceptable answer, but if that is the case you probably would want to
> verify that the command indeed does go ahead without asking.

The hunk is there because the "echo y" is redundant with confirm=never
which is now set early.

> These all test you would get a prompt when you as the author expect the
> code to give one.  Do you also need tests that verify you do not ask
> needless confirmation when the code shouldn't?

Yes, I couldn't think how to test that last night for some reason, but
it is obvious to me now. I can send a followup.

>>  test_expect_success '--compose adds MIME for utf8 body' '
>>       clean_fake_sendmail &&
>>       (echo "#!$SHELL_PATH" &&
>>        echo "echo utf8 body: àéìöú >>\"\$1\""
>>       ) >fake-editor-utf8 &&
>>       chmod +x fake-editor-utf8 &&
>> -     echo y | \
>>         GIT_EDITOR="\"$(pwd)/fake-editor-utf8\"" \
>> -       GIT_SEND_EMAIL_NOTTY=1 \
>>         git send-email \
>>         --compose --subject foo \
>>         --from="Example <nobody@example.com>" \
>
> If the change you made to git-send-email.perl is later broken and the
> command (incorrectly) starts asking for confirmation with these command
> line options, what does this test do?  Does it get stuck, forbidding the
> test suite to be run unattended?

Many of the tests (because they automatically cc) will hang waiting on
input if git-send-email.perl is later broken in such a way as to start
ignoring --compose.

I could not think of a good way to avoid this. We could possibly "echo
y|" redundantly to every test, but even that does not guarantee that
one of the other prompts cannot cause a hang. So this potential to
hang, I think, already exists.

The best I could think of was something like (sleep 60 && kill $$) &;
sleep_pid=$!; ... kill $sleep_pid; wrapping each test, or even the
entire script. Suggestions here appreciated.

But, I think this can largely be addressed by testing that the change
does *not* prompt when it shouldn't, and moving that test before any
of the existing tests which could prompt.

j.

^ permalink raw reply

* Re: jgit and ignore
From: Ferry Huberts (Pelagic) @ 2009-03-01 13:47 UTC (permalink / raw)
  To: Tor Arne Vestbø; +Cc: Jon Smirl, Shawn O. Pearce, Git Mailing List
In-Reply-To: <49AA8ECD.4090302@gmail.com>

Tor Arne Vestbø wrote:
> Ferry Huberts (Pelagic) wrote:
>> BTW. the plugin will not look at the eclipse filter for ignore files
>> once I've finished the implementation. that's the only way to make sure
>> that we ignore resources in exactly the same way as git does: git only
>> ignores on the basis of .gitignore files, command line options (we do
>> not have those for the plugin), and the info/exclude file in the repository.
> 
> You're missing one: the file specified by the configuration variable
> core.excludesfile, which can be though of as a global ignore filter.
> 
> The manual says:
> 
> "Patterns which a user wants git to ignore in all situations (e.g.,
> backup or temporary files generated by the user's editor of choice)
> generally go into a file specified by core.excludesfile in the user's
> ~/.gitconfig."
> 
> This is _exactly_ what Eclipse's Team->Ignored Resources is for.
> 
> I see two groups of users: a) those with a global ignore specified in
> core.excludesfile b) and those without (they may not even have cgit
> installed, if EGit is bundled with Eclipse in the future).
> 
> In my opinion, EGit should default to using Eclipse's built in ignores,
> but then detect the presence of a global core.excludesfile, in which
> case it would notify the user ("I see you have a core.excludesfile") and
> let the user switch to using that one instead.
> 
> In other words, whether or not to use core.excludesfile or Eclipse's
> global team ignores should be optional, and we should provide heuristics
> for deciding when to switch.
> 
> I think that would cover both groups of users without causing confusion.
> 
> Tor Arne

Ok, missed that one.
I'll have to think a bit on how to incorporate that one but I think it's
doable in a relatively easy way.

I do not agree with your propasal however.
We then would have different behaviour between how 'git' behaves within
Eclipse (by means of the plugin) and how 'git' behaves within the
command line. That alone can cause much more confusion.

Your proposal can also be implemented differently by (for example)
building a UI that allows the user to edit the global ignore file.
The Eclipse global team ignores simply are too broad: think of the
situation where I also have non-git project (like svn) in my workspace.

^ permalink raw reply

* Re: jgit and ignore
From: Tor Arne Vestbø @ 2009-03-01 13:34 UTC (permalink / raw)
  To: Ferry Huberts (Pelagic); +Cc: Jon Smirl, Shawn O. Pearce, Git Mailing List
In-Reply-To: <49AA884D.1050806@pelagic.nl>

Ferry Huberts (Pelagic) wrote:
> BTW. the plugin will not look at the eclipse filter for ignore files
> once I've finished the implementation. that's the only way to make sure
> that we ignore resources in exactly the same way as git does: git only
> ignores on the basis of .gitignore files, command line options (we do
> not have those for the plugin), and the info/exclude file in the repository.

You're missing one: the file specified by the configuration variable
core.excludesfile, which can be though of as a global ignore filter.

The manual says:

"Patterns which a user wants git to ignore in all situations (e.g.,
backup or temporary files generated by the user's editor of choice)
generally go into a file specified by core.excludesfile in the user's
~/.gitconfig."

This is _exactly_ what Eclipse's Team->Ignored Resources is for.

I see two groups of users: a) those with a global ignore specified in
core.excludesfile b) and those without (they may not even have cgit
installed, if EGit is bundled with Eclipse in the future).

In my opinion, EGit should default to using Eclipse's built in ignores,
but then detect the presence of a global core.excludesfile, in which
case it would notify the user ("I see you have a core.excludesfile") and
let the user switch to using that one instead.

In other words, whether or not to use core.excludesfile or Eclipse's
global team ignores should be optional, and we should provide heuristics
for deciding when to switch.

I think that would cover both groups of users without causing confusion.

Tor Arne

^ permalink raw reply

* Re: jgit and ignore
From: Ferry Huberts (Pelagic) @ 2009-03-01 13:06 UTC (permalink / raw)
  To: Jon Smirl; +Cc: Shawn O. Pearce, Git Mailing List
In-Reply-To: <9e4733910903010454u662eb5afob45f608321660500@mail.gmail.com>

Jon Smirl wrote:
> On Sun, Mar 1, 2009 at 5:11 AM, Ferry Huberts (Pelagic)
> <ferry.huberts@pelagic.nl> wrote:
>> Shawn O. Pearce wrote:
>>> Jon Smirl <jonsmirl@gmail.com> wrote:
>>>> I'm using jgit in eclipse. Works great for me.
>>> Yay!
>>>
>>>> I have a couple of generated files in my working directory. There
>>>> doesn't seem to be any UI for ignoring them. Is it there and I just
>>>> can't find it?
>>> EGit doesn't (yet) honor the .gitignore files like it should. Someone
>>> (Ferry i-forget-the-rest-of-his-name) is working on adding ignore
>>> support and has patches in flight for at least some of it.
>>>
>> Ferry i-do-remember-my-name Huberts is working on it :-)
>>
>> I have most of it working in a basic form already but am currently
>> refactoring things to take care of some nasty little details.
>> Expect something to arrive within (my best guesstimate) about 3 to 4
>> weeks. after next week I'll be skiing for a week, so no coding then :-)
>>
>> For the new functionality:
>> You don't really need a UI: just add a .gitignore file with a pattern
>> and the plugin will pick it up and show you what is ignored by means of
>> a nice little decoration.
> 
> I expected it to work by right clicking the file and picking
> team/ignore. This would add the file name .gitignore and automatically
> add .gitignore to my commit. It would also alter the eclipse filter to
> make the file disappear in the eclipse browser.
> 

that'll come later, first we need to ignore the same files as git :-)

which eclipse browser?

BTW. the plugin will not look at the eclipse filter for ignore files
once I've finished the implementation. that's the only way to make sure
that we ignore resources in exactly the same way as git does: git only
ignores on the basis of .gitignore files, command line options (we do
not have those for the plugin), and the info/exclude file in the repository.

^ permalink raw reply

* Re: jgit and ignore
From: Jon Smirl @ 2009-03-01 12:54 UTC (permalink / raw)
  To: Ferry Huberts (Pelagic); +Cc: Shawn O. Pearce, Git Mailing List
In-Reply-To: <49AA5F64.6070207@pelagic.nl>

On Sun, Mar 1, 2009 at 5:11 AM, Ferry Huberts (Pelagic)
<ferry.huberts@pelagic.nl> wrote:
> Shawn O. Pearce wrote:
>> Jon Smirl <jonsmirl@gmail.com> wrote:
>>> I'm using jgit in eclipse. Works great for me.
>>
>> Yay!
>>
>>> I have a couple of generated files in my working directory. There
>>> doesn't seem to be any UI for ignoring them. Is it there and I just
>>> can't find it?
>>
>> EGit doesn't (yet) honor the .gitignore files like it should. Someone
>> (Ferry i-forget-the-rest-of-his-name) is working on adding ignore
>> support and has patches in flight for at least some of it.
>>
> Ferry i-do-remember-my-name Huberts is working on it :-)
>
> I have most of it working in a basic form already but am currently
> refactoring things to take care of some nasty little details.
> Expect something to arrive within (my best guesstimate) about 3 to 4
> weeks. after next week I'll be skiing for a week, so no coding then :-)
>
> For the new functionality:
> You don't really need a UI: just add a .gitignore file with a pattern
> and the plugin will pick it up and show you what is ignored by means of
> a nice little decoration.

I expected it to work by right clicking the file and picking
team/ignore. This would add the file name .gitignore and automatically
add .gitignore to my commit. It would also alter the eclipse filter to
make the file disappear in the eclipse browser.


>
> Ferry
>



-- 
Jon Smirl
jonsmirl@gmail.com

^ permalink raw reply

* Re: [PATCH 1/3] t3400-rebase: Move detached HEAD check earlier
From: Johannes Sixt @ 2009-03-01 11:21 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <1235902803-32528-1-git-send-email-j6t@kdbg.org>

Hmpf. There won't be 2/3 or 3/3 today :-(

I've set format.numbered to false now.

-- Hannes

^ permalink raw reply

* Re: [PATCH] import memmem() with linear complexity from Gnulib
From: René Scharfe @ 2009-03-01 11:15 UTC (permalink / raw)
  To: Jeff King; +Cc: Mike Hommey, Junio C Hamano, git
In-Reply-To: <20090301034123.GC30384@coredump.intra.peff.net>

Jeff King schrieb:
> On Sat, Feb 28, 2009 at 11:44:01PM +0100, Mike Hommey wrote:
> 
>>> ---
>>>  Makefile             |    1 +
>>>  compat/memmem.c      |  103 +++++++++----
>>>  compat/str-two-way.h |  429 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  3 files changed, 504 insertions(+), 29 deletions(-)
>> Seeing how much memmem is being used in the codebase, is it really worth?
> 
> See earlier in the thread, where "git log -Stoken" is substantially
> faster on Linux versus Windows (but some exact numbers before and after
> on Windows would be nice to have in the commit message).

Yes, and also please see the other numbers I just posted.  It's more of
an update -- we took the current memmem() fall-back from glibc, too, and
they switched to this shiny new implementation to avoid the quadratic
complexity of the old one in the meantime.

I was going to say that with a fast memmem() we could convert some
strstr() calls, especially those where we know the lengths of the
strings anyway -- intuitively, memmem() should be faster than strstr()
in that case.  However, the following patch on top of the Gnulib import
makes "git grep grep v1.6.1" take 10% *more* time for me (on Windows).
I wonder why.


diff --git a/grep.c b/grep.c
index 062b2b6..66ef171 100644
--- a/grep.c
+++ b/grep.c
@@ -39,6 +39,8 @@ static void compile_regexp(struct grep_pat *p, struct grep_opt *opt)
 {
 	int err;
 
+	p->pattern_len = strlen(p->pattern);
+
 	if (opt->fixed || is_fixed(p->pattern))
 		p->fixed = 1;
 	if (opt->regflags & REG_ICASE)
@@ -268,16 +270,17 @@ static void show_name(struct grep_opt *opt, const char *name)
 	printf("%s%c", name, opt->null_following_name ? '\0' : '\n');
 }
 
-static int fixmatch(const char *pattern, char *line, regmatch_t *match)
+static int fixmatch(const char *pattern, size_t pattern_len, const char *bol,
+		    const char *eol, regmatch_t *match)
 {
-	char *hit = strstr(line, pattern);
+	char *hit = memmem(bol, eol - bol, pattern, pattern_len);
 	if (!hit) {
 		match->rm_so = match->rm_eo = -1;
 		return REG_NOMATCH;
 	}
 	else {
-		match->rm_so = hit - line;
-		match->rm_eo = match->rm_so + strlen(pattern);
+		match->rm_so = hit - bol;
+		match->rm_eo = match->rm_so + pattern_len;
 		return 0;
 	}
 }
@@ -335,7 +338,7 @@ static int match_one_pattern(struct grep_opt *opt, struct grep_pat *p, char *bol
 			       pmatch, 0);
 	}
 	else {
-		hit = !fixmatch(p->pattern, bol, pmatch);
+		hit = !fixmatch(p->pattern, p->pattern_len, bol, eol, pmatch);
 	}
 
 	if (hit && opt->word_regexp) {
diff --git a/grep.h b/grep.h
index 5102ce3..2e22ab2 100644
--- a/grep.h
+++ b/grep.h
@@ -28,6 +28,7 @@ struct grep_pat {
 	int no;
 	enum grep_pat_token token;
 	const char *pattern;
+	size_t pattern_len;
 	enum grep_header_field field;
 	regex_t regexp;
 	unsigned fixed:1;

^ permalink raw reply related

* Re: [PATCH 3/4] diffcore-pickaxe: further refactor count_match()
From: René Scharfe @ 2009-03-01 10:53 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <7viqmtaec4.fsf@gitster.siamese.dyndns.org>

Junio C Hamano schrieb:
> René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
> 
>> I get this (Ubuntu 8.10 x64, Fedora 10 x64 using the same Linux repo,
>> Windows Vista x64 using a different Linux repo with the same HEAD on
>> NTFS and msysgit, numbers are the elapsed time in seconds, best of five
>> runs):
>>
>>                            Ubuntu  Fedora  Windows
>>    v1.6.2-rc2                8.14    8.16    9.236
>>    v1.6.2-rc2+[1-4]          2.43    2.45    2.995
>>    v1.6.2-rc2+[1-4]+memmem   1.31    1.25    2.917
>>    v1.6.2-rc2+[1-3]+memmem   1.51    1.16    8.455
>>
>> Ubuntu has glibc 2.8, while Fedora 10 has glibc 2.9, with a new and more
>> efficient memmem() implementation.  On Windows, we use our own naive
>> memmem() implementation.

Correction: On Windows, we use the previous, quadratic, naive memmem()
implementation from glibc, not anything we came up with.

>> So using memmem() is worthwhile.  And providing a better fall-back
>> version in compat/ can speed up this particular case to the point where
>> the fourth patch becomes moot.
> 
> Are these numbers telling me that with a good memmem() implementation,
> patch 4/4 is not just moot but actively detrimental?
> 
> With a long enough needle, it entirely is possible that scanning the whole
> image with sublinear string search algorithm would perform much better
> than the preprocessing patch 4/4 does which has to scan all the bytes in
> the common parts.

Yes, patch 4 makes it go slower than using memmem() alone, in this test.

Here are a few more numbers, all measured on Windows.  The topmost four
times in the column "long" should be the same as in the Windows column
above, yet they are slightly bigger.  Some background process must've
decided to do some work.

  git log -S"$STRING" HEAD -- kernel/sched.c >/dev/null

  short: STRING='e'
  long:  STRING='Ensure that the real time constraints are schedulable.'

                                  short  long
  v1.6.2-rc2                      9.120  9.266
  v1.6.2-rc2+[1-4]                3.037  3.048
  v1.6.2-rc2+[1-4]+memmem         2.994  2.964
  v1.6.2-rc2+[1-3]+memmem         8.514  8.528
  v1.6.2-rc2+[1-4]+memmem+linear  1.939  1.759
  v1.6.2-rc2+[1-3]+memmem+linear  2.315  1.559

So patch 4 helps significantly with short needles, while it hurts a bit
with long needles.  Linear memmem() is faster than the quadratic one we
currently have in compat/ in all cases.

René

^ permalink raw reply

* [PATCH 1/3] t3400-rebase: Move detached HEAD check earlier
From: Johannes Sixt @ 2009-03-01 10:20 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Johannes Sixt

Short story: There is a section in t3400 that tests fundamental rebase
properties.  3ec7371f (Add two extra tests for git rebase, 2009-02-09)
added a check that rebase works on a detached HEAD, but the test was put
near the end of the file.  This moves it to a more suitable place.

Long story: The test that preceded the one in question tests that a
rebased commit degrades from a content change with mode change to a
mere mode change.  But on Windows, where we have core.filemode=false,
the original commit did not record the mode change, and so the rebase
operation did not rebase anything.  This caused the subsequent detached
HEAD test to fail.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 t/t3400-rebase.sh |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index 8c0c5f5..be7ae5a 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -48,6 +48,10 @@ test_expect_success \
     'the rebase operation should not have destroyed author information' \
     '! (git log | grep "Author:" | grep "<>")'
 
+test_expect_success 'HEAD was detached during rebase' '
+     test $(git rev-parse HEAD@{1}) != $(git rev-parse my-topic-branch@{1})
+'
+
 test_expect_success 'rebase after merge master' '
      git reset --hard topic &&
      git merge master &&
@@ -85,10 +89,6 @@ test_expect_success 'rebase a single mode change' '
      GIT_TRACE=1 git rebase master
 '
 
-test_expect_success 'HEAD was detached during rebase' '
-     test $(git rev-parse HEAD@{1}) != $(git rev-parse modechange@{1})
-'
-
 test_expect_success 'Show verbose error when HEAD could not be detached' '
      : > B &&
      test_must_fail git rebase topic 2> output.err > output.out &&
-- 
1.6.2.rc1.27.g29e57

^ permalink raw reply related

* smudge filter problem
From: bill lam @ 2009-03-01 10:18 UTC (permalink / raw)
  To: git

I track xls by using filter to convert it to gnumeric format. Inside gitconfig
[filter "excelbiff"]
  clean = ssconvert --export-type=Gnumeric_XmlIO:sax fd://0 fd://1 | zcat -f
  smudge = ssconvert --export-type=Gnumeric_Excel:excel_biff8 fd://0 fd://1
and xls set inside gitattribute

While the clean filter works, the smudge filter does not. When git-checkout it said seek error 
 git ckeckout foo.xls

However git-show works as pipe to ssconvert
 git show HEAD:foo.xls | ssconvert --export-type=Gnumeric_Excel:excel_biff8 fd://0  newfoo.xls

I guess ssconvert was mistaken to believe that all data has been flushed too soon.

I tested with an xls that converted to gnumeric uncompress file size about 10MB and libgsf 1.14.11

PS. for older libgsf it needs to using a temp file to buffer data for ssconvert instead of using fd://0

-- 
regards,
====================================================
GPG key 1024D/4434BAB3 2008-08-24
gpg --keyserver subkeys.pgp.net --recv-keys 4434BAB3
唐詩019 孟浩然  夏日南亭懷辛大
    山光忽西落  池月漸東上  散髮乘夜涼  開軒臥閑敞  荷風送香氣  竹露滴清響
    欲取鳴琴彈  恨無知音賞  感此懷故人  中宵勞夢想

^ permalink raw reply

* Re: jgit and ignore
From: Ferry Huberts (Pelagic) @ 2009-03-01 10:11 UTC (permalink / raw)
  To: Shawn O. Pearce; +Cc: Jon Smirl, Git Mailing List
In-Reply-To: <20090228172622.GC26689@spearce.org>

Shawn O. Pearce wrote:
> Jon Smirl <jonsmirl@gmail.com> wrote:
>> I'm using jgit in eclipse. Works great for me.
>  
> Yay!
> 
>> I have a couple of generated files in my working directory. There
>> doesn't seem to be any UI for ignoring them. Is it there and I just
>> can't find it?
> 
> EGit doesn't (yet) honor the .gitignore files like it should. Someone
> (Ferry i-forget-the-rest-of-his-name) is working on adding ignore
> support and has patches in flight for at least some of it.
> 
Ferry i-do-remember-my-name Huberts is working on it :-)

I have most of it working in a basic form already but am currently
refactoring things to take care of some nasty little details.
Expect something to arrive within (my best guesstimate) about 3 to 4
weeks. after next week I'll be skiing for a week, so no coding then :-)

For the new functionality:
You don't really need a UI: just add a .gitignore file with a pattern
and the plugin will pick it up and show you what is ignored by means of
a nice little decoration.

Ferry

^ permalink raw reply

* Re: [PATCH 3/4] Add init-serve, the remote side of "git init --remote=host:path"
From: Jeff King @ 2009-03-01 10:00 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <7v63itbxe7.fsf@gitster.siamese.dyndns.org>

On Sat, Feb 28, 2009 at 09:54:56PM -0800, Junio C Hamano wrote:

> One issue I did not describe in the message was to what extent we would
> want to allow operations other than the creating of a new repository
> itself.
> 
> "Other than the creation" includes things like these:

Hmph. I am not too excited by this list. What is the advantage of doing
them over the git protocol versus some out-of-band method?

It seems to me there are two main cases for dealing with a remote in
this way:

  1. You have shell access and a uid on the remote, but it is
     inconvenient to ssh across, find the repo (which may already be
     known locally by remote.*.url config), and then execute some
     commands.

     In this scenario, there really are no security concerns; you could
     have logged in and done all of this anyway. So it seems like a more
     natural fit would be a _client_ program that figures out what shell
     snippet you want to execute, connects to the remote, and does it.

  2. Your access on the remote is very restricted, you may not have your
     own uid, and hooks may be enforcing arbitrary security policy.

     In this case, even something as simple as chgrp seems like it could
     be a problem, depending on the remote's setup (e.g., because all
     users connect as one uid, but group permissions are somehow
     meaningful to the system; this implies that connecting users should
     not be able to arbitrarily chgrp their repos, even if chgrp itself
     allows it).

     Furthermore, in the case of many providers (e.g., github,
     repo.or.cz), there is already a separate out-of-band interface for
     doing "meta" stuff, like managing user accounts and repos. Isn't it
     more natural for them to integrate these features with their
     existing interfaces?

But let's say that there really is some value in setting up this
channel (because we want a uniform way of doing these things so we can
add more automated tool support from the client side). Then I think it
makes sense to look at what the people in (2) above are doing already.
That is, what sorts of things can you already do (and not do) in
github's or repo.or.cz's interface? On top of that, it probably makes
sense to ask them if they are interested in such a feature, as they
would be primary users. And if they are, what do they want out of it?

-Peff

^ permalink raw reply

* Re: coding style: #ifdef blocks and real C blocks
From: Tay Ray Chuan @ 2009-03-01  9:33 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <7vbpsl8v7r.fsf@gitster.siamese.dyndns.org>

On Sun, Mar 1, 2009 at 5:10 PM, Junio C Hamano <gitster@pobox.com> wrote:
>    #ifdef USE_CURL_MULTI
>    #define active_slot_get get_active_multi_slot
>    #else
>    #define active_slot_get get_active_slot
>    #endif
>
> so that the code itself would not have to have any #ifdef?

On further thought, wouldn't it be a better idea to make this
uppercase, ie. GET_ACTIVE_SLOT, to make it more obvious this is a
macro? Then we wouldn't have to jumble the name into "active_slot_get"
to differentiate it from the two slot functions.

-- 
Cheers,
Ray Chuan

^ permalink raw reply

* Re: coding style: #ifdef blocks and real C blocks
From: Tay Ray Chuan @ 2009-03-01  9:27 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <7vbpsl8v7r.fsf@gitster.siamese.dyndns.org>

Hi,

On Sun, Mar 1, 2009 at 5:10 PM, Junio C Hamano <gitster@pobox.com> wrote:
> How about doing something like this:
>
>    #ifdef USE_CURL_MULTI
>    #define active_slot_get get_active_multi_slot
>    #else
>    #define active_slot_get get_active_slot
>    #endif
>

Nice.

> Similarly, with something like this:
>
>    #ifdef USE_CURL_MULTI
>    slot active_persistent_slot() {
>            return persistent_connection ? get_active_slot() : get_active_multi_slot();
>    }
>    #else
>    slot active_persistent_slot() {
>            return get_active_slot();
>    }
>    #endif
>
> the call site can be #ifdef free, no?
>

Hmm, so I just do "slot = active_persistent_slot()" ?

-- 
Cheers,
Ray Chuan

^ permalink raw reply

* Re: coding style: #ifdef blocks and real C blocks
From: Junio C Hamano @ 2009-03-01  9:10 UTC (permalink / raw)
  To: Tay Ray Chuan; +Cc: git
In-Reply-To: <be6fef0d0903010052t50551f3w74352b69afdee620@mail.gmail.com>

Tay Ray Chuan <rctay89@gmail.com> writes:

> #ifdef USE_CURL_MULTI
> 	slot = get_active_multi_slot();
> #else
> 	slot = get_active_slot();
> #endif
> 	slot->callback_func = process_response;
> 	slot->callback_data = request;
> 	request->slot = slot;

How about doing something like this:

    #ifdef USE_CURL_MULTI
    #define active_slot_get get_active_multi_slot
    #else
    #define active_slot_get get_active_slot
    #endif

so that the code itself would not have to have any #ifdef?

    slot = active_slot_get()
    slot->callback_func = process_response;
    slot->callback_data = request;
    request->slot = slot;
    
> #ifdef USE_CURL_MULTI
> 	if (!persistent_connection)
> 		slot = get_active_multi_slot();
> 	else
> 		slot = get_active_slot();
> #else
> 	slot = get_active_slot();
> #endif

Similarly, with something like this:

    #ifdef USE_CURL_MULTI
    slot active_persistent_slot() {
            return persistent_connection ? get_active_slot() : get_active_multi_slot();
    }
    #else
    slot active_persistent_slot() {
            return get_active_slot();
    }
    #endif

the call site can be #ifdef free, no?

^ permalink raw reply

* Re: [PATCH] send-email: add --confirm option
From: Junio C Hamano @ 2009-03-01  9:03 UTC (permalink / raw)
  To: Jay Soffian; +Cc: git, Nanako Shiraishi, Paul Gortmaker
In-Reply-To: <1235895801-80414-1-git-send-email-jaysoffian@gmail.com>

Jay Soffian <jaysoffian@gmail.com> writes:

> send-email violates the principle of least surprise by automatically
> cc'ing additional recipients without confirming this with the user.
>
> This patch teaches send-email a --confirm option. It takes the
> following values:
>
>  --confirm=always   always confirm before sending
>  --confirm=never    never confirm before sending
>  --confirm=cc       confirm before sending when send-email has
>                     automatically added addresses from the patch to
>                     the Cc list
>  --confirm=compose  confirm before sending the first message when
>                     using --compose. (Needed to maintain backwards
>                     compatibility with existing behavior.)
>  --confirm=auto     'cc' + 'compose'
>
> The option defaults to 'compose' if any suppress Cc related options have
> been used, otherwise it defaults to 'auto'.

It is hard to judge if this is a good thing to do _at this point_.  Those
who complained may welcome it but that is hardly the point.  You need to
convince those who stayed silent (because they thought the default won't
change and were not paying attention) that it is a good change.

Especially the changes (not additions) to existing tests worries me; each
change to the existing one is a demonstration of breaking existing users.

You cannot retrofit safety by disallowing things once you used to allow,
without upsetting existing users.

> @@ -837,6 +834,25 @@ X-Mailer: git-send-email $gitversion
>  	unshift (@sendmail_parameters,
>  			'-f', $raw_from) if(defined $envelope_sender);
>  
> +	if ($needs_confirm && !$dry_run) {
> +		print "\n$header\n";
> +		while (1) {
> +			chomp ($_ = $term->readline(
> +				"Send this email? ([y]es|[n]o|[q]uit|[a]ll): "
> +			));
> +			last if /^(?:yes|y|no|n|quit|q|all|a)/i;
> +			print "\n";
> +		}
> +		if (/^n/i) {
> +			return;
> +		} elsif (/^q/i) {
> +			cleanup_compose_files();
> +			exit(0);
> +		} elsif (/^a/i) {
> +			$confirm = 'never';
> +		}
> +	}

I think "[a]ll" would make it a bit less objectionable to people who hate
unsolicited confirmation dialogs.  Nice touch.

> @@ -1094,13 +1119,10 @@ foreach my $t (@files) {
>  	$message_id = undef;
>  }
>  
> -if ($compose) {
> -	cleanup_compose_files();
> -}
> +cleanup_compose_files();
>  
>  sub cleanup_compose_files() {
> -	unlink($compose_filename, $compose_filename . ".final");
> -
> +	unlink($compose_filename, $compose_filename . ".final") if $compose;
>  }

Does this hunk have anything to do with this topic, or is it just a churn
that does not change any behaviour?

> @@ -175,15 +180,13 @@ test_set_editor "$(pwd)/fake-editor"
>  
>  test_expect_success '--compose works' '
>  	clean_fake_sendmail &&
> -	echo y | \
> -		GIT_SEND_EMAIL_NOTTY=1 \
> -		git send-email \
> -		--compose --subject foo \
> -		--from="Example <nobody@example.com>" \
> -		--to=nobody@example.com \
> -		--smtp-server="$(pwd)/fake.sendmail" \
> -		$patches \
> -		2>errors
> +	git send-email \
> +	--compose --subject foo \
> +	--from="Example <nobody@example.com>" \
> +	--to=nobody@example.com \
> +	--smtp-server="$(pwd)/fake.sendmail" \
> +	$patches \
> +	2>errors
>  '

How would test this break without this hunk?  "echo" dies of sigpipe, or
something?

I am not objecting to this particular change; just asking why this change
is here.  "It does not break, but the command shouldn't ask for
confirmation, and giving 'y' into it is unnecessary" is a perfectly
acceptable answer, but if that is the case you probably would want to
verify that the command indeed does go ahead without asking.

> @@ -375,15 +378,56 @@ test_expect_success '--suppress-cc=cc' '
>  	test_suppression cc
>  '
>  
> +test_confirm () {
> +	echo y | \
> +		GIT_SEND_EMAIL_NOTTY=1 \
> +		git send-email \
> +		--from="Example <nobody@example.com>" \
> +		--to=nobody@example.com \
> +		--smtp-server="$(pwd)/fake.sendmail" \
> +		$@ \
> +		$patches | grep "Send this email"
> +}
> +
> +test_expect_success '--confirm=always' '
> +	test_confirm --confirm=always --suppress-cc=all
> +'
> + ...
> +test_expect_success 'confirm by default (due to --compose)' '
> +	CONFIRM=$(git config --get sendemail.confirm) &&
> +	git config --unset sendemail.confirm &&
> +	test_confirm --suppress-cc=all --compose
> +	ret="$?"
> +	git config sendemail.confirm ${CONFIRM:-never}
> +	test $ret = "0"
> +'

These all test you would get a prompt when you as the author expect the
code to give one.  Do you also need tests that verify you do not ask
needless confirmation when the code shouldn't?

>  test_expect_success '--compose adds MIME for utf8 body' '
>  	clean_fake_sendmail &&
>  	(echo "#!$SHELL_PATH" &&
>  	 echo "echo utf8 body: àéìöú >>\"\$1\""
>  	) >fake-editor-utf8 &&
>  	chmod +x fake-editor-utf8 &&
> -	echo y | \
>  	  GIT_EDITOR="\"$(pwd)/fake-editor-utf8\"" \
> -	  GIT_SEND_EMAIL_NOTTY=1 \
>  	  git send-email \
>  	  --compose --subject foo \
>  	  --from="Example <nobody@example.com>" \

If the change you made to git-send-email.perl is later broken and the
command (incorrectly) starts asking for confirmation with these command
line options, what does this test do?  Does it get stuck, forbidding the
test suite to be run unattended?

^ permalink raw reply

* coding style: #ifdef blocks and real C blocks
From: Tay Ray Chuan @ 2009-03-01  8:52 UTC (permalink / raw)
  To: git

Hi,

I couldn't find any mention of #ifdef usage nor any example of how I'm
thinking of using #ifdef, so I'm posing this here.

Right now here's what I have on my local branch:

---
#ifdef USE_CURL_MULTI
	slot = get_active_multi_slot();
#else
	slot = get_active_slot();
#endif
	slot->callback_func = process_response;
	slot->callback_data = request;
	request->slot = slot;
---

I want to implement an option that's stored in the variable
"persistent_connection", which basically makes the code behave as if
USE_CURL_MULTI isn't defined. (This would make git open/close less
connections throughout the lifespan of its execution, which would
remove/minimize the number of credential prompting if authentication
is required, among other advantages.)

Here's what I thought of:

---
#ifdef USE_CURL_MULTI
	if (!persistent_connection)
		slot = get_active_multi_slot();
	else
		slot = get_active_slot();
#else
	slot = get_active_slot();
#endif
---

I thought of shortening this further to

---
#ifdef USE_CURL_MULTI
if (!persistent_connection)
	slot = get_active_multi_slot();
else
#else
	slot = get_active_slot();
#endif
	slot->callback_func = process_response;
	slot->callback_data = request;
	request->slot = slot;
---

What would you suggest?

-- 
Cheers,
Ray Chuan

^ permalink raw reply

* Re: [PATCH] rebase -i: avoid 'git reset' when possible
From: Junio C Hamano @ 2009-03-01  8:38 UTC (permalink / raw)
  To: Johannes Schindelin
  Cc: Sverre Rabbelier, Stephen Haberman, Shawn O. Pearce, Thomas Rast,
	Git Mailing List, Stephan Beyer, Christian Couder,
	Daniel Barkalow
In-Reply-To: <alpine.DEB.1.00.0902271354130.6600@intel-tinevez-2-302>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 3dc659d..a9617a2 100755
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -442,6 +442,27 @@ do_rest () {
>  	done
>  }
>  
> +# skip picking commits whose parents are unchanged
> +skip_unnecessary_picks () {
> +	current=$ONTO
> +	i=0
> +	while read command sha1 rest
> +	do
> +		test pick = "$command" &&
> +		test $current = "$(git rev-parse $sha1^)" ||
> +		break
> +		current=$(git rev-parse $sha1)
> +		i=$(($i+1))
> +	done < "$TODO"
> +	test $i = 0 || {
> +		sed -e "${i}q" < "$TODO" >> "$DONE" &&
> +		sed -e "1,${i}d" < "$TODO" >> "$TODO".new &&
> +		mv -f "$TODO".new "$TODO" &&
> +		ONTO=$current ||
> +		die "Could not skip $i pick commands"
> +	}
> +}

I do not think you have to count and then run two sed.

	this=$ONTO
	skip_pick=t
	while read command sha1 rest
	do
		sha1=$(git rev-parse "$sha1")
        	case "$skip_pick,$command" in
                t,pick | t,p)
			if test "$this" = "$sha1"
                        then
				echo >&3 "$command $sha1 $rest"
                        	this="$sha1"
				continue
			fi
		esac                        
		skip_pick=f
                echo "$command $sha1 $rest"
	done <"$TODO" >"$TODO.new" 3>>"$DONE" &&
	...

^ permalink raw reply

* [PATCH] send-email: add --confirm option
From: Jay Soffian @ 2009-03-01  8:23 UTC (permalink / raw)
  To: git; +Cc: Jay Soffian, Nanako Shiraishi, Junio C Hamano, Paul Gortmaker

send-email violates the principle of least surprise by automatically
cc'ing additional recipients without confirming this with the user.

This patch teaches send-email a --confirm option. It takes the
following values:

 --confirm=always   always confirm before sending
 --confirm=never    never confirm before sending
 --confirm=cc       confirm before sending when send-email has
                    automatically added addresses from the patch to
                    the Cc list
 --confirm=compose  confirm before sending the first message when
                    using --compose. (Needed to maintain backwards
                    compatibility with existing behavior.)
 --confirm=auto     'cc' + 'compose'

The option defaults to 'compose' if any suppress Cc related options have
been used, otherwise it defaults to 'auto'.

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
Junio,

I based this on top of js/send-email.

j.

 Documentation/git-send-email.txt |   16 ++++++++
 git-send-email.perl              |   68 +++++++++++++++++++++++------------
 t/t9001-send-email.sh            |   72 +++++++++++++++++++++++++++++--------
 3 files changed, 117 insertions(+), 39 deletions(-)

diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index 164d149..bcf7ff1 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -199,6 +199,22 @@ specified, as well as 'body' if --no-signed-off-cc is specified.
 Administering
 ~~~~~~~~~~~~~
 
+--confirm::
+	Confirm just before sending:
++
+--
+- 'always' will always confirm before sending
+- 'never' will never confirm before sending
+- 'cc' will confirm before sending when send-email has automatically
+  added addresses from the patch to the Cc list
+- 'compose' will confirm before sending the first message when using --compose.
+- 'auto' is equivalent to 'cc' + 'compose'
+--
++
+Default is the value of 'sendemail.confirm' configuration value; if that
+is unspecified, default to 'auto' unless any of the suppress options
+have been specified, in which case default to 'compose'.
+
 --dry-run::
 	Do everything except actually send the emails.
 
diff --git a/git-send-email.perl b/git-send-email.perl
index adf7ecb..a777e3f 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -75,6 +75,8 @@ git send-email [options] <file | directory | rev-list options >
     --[no-]thread                  * Use In-Reply-To: field. Default on.
 
   Administering:
+    --confirm               <str>  * Confirm recipients before sending;
+                                     auto, cc, compose, always, or never.
     --quiet                        * Output one line of info per email.
     --dry-run                      * Don't actually send the emails.
     --[no-]validate                * Perform patch sanity checks. Default on.
@@ -181,7 +183,7 @@ sub do_edit {
 my ($thread, $chain_reply_to, $suppress_from, $signed_off_by_cc, $cc_cmd);
 my ($smtp_server, $smtp_server_port, $smtp_authuser, $smtp_encryption);
 my ($identity, $aliasfiletype, @alias_files, @smtp_host_parts);
-my ($validate);
+my ($validate, $confirm);
 my (@suppress_cc);
 
 my %config_bool_settings = (
@@ -207,6 +209,7 @@ my %config_settings = (
     "suppresscc" => \@suppress_cc,
     "envelopesender" => \$envelope_sender,
     "multiedit" => \$multiedit,
+    "confirm"   => \$confirm,
 );
 
 # Handle Uncouth Termination
@@ -258,6 +261,7 @@ my $rc = GetOptions("sender|from=s" => \$sender,
 		    "suppress-from!" => \$suppress_from,
 		    "suppress-cc=s" => \@suppress_cc,
 		    "signed-off-cc|signed-off-by-cc!" => \$signed_off_by_cc,
+		    "confirm=s" => \$confirm,
 		    "dry-run" => \$dry_run,
 		    "envelope-sender=s" => \$envelope_sender,
 		    "thread!" => \$thread,
@@ -346,6 +350,11 @@ if ($suppress_cc{'body'}) {
 	delete $suppress_cc{'body'};
 }
 
+# Set confirm
+if (!defined $confirm) {
+	$confirm = scalar %suppress_cc ? 'compose' : 'auto';
+};
+
 # Debugging, print out the suppressions.
 if (0) {
 	print "suppressions:\n";
@@ -663,25 +672,13 @@ if (!defined $smtp_server) {
 	$smtp_server ||= 'localhost'; # could be 127.0.0.1, too... *shrug*
 }
 
-if ($compose) {
-	while (1) {
-		$_ = $term->readline("Send this email? (y|n) ");
-		last if defined $_;
-		print "\n";
-	}
-
-	if (uc substr($_,0,1) ne 'Y') {
-		cleanup_compose_files();
-		exit(0);
-	}
-
-	if ($compose > 0) {
-		@files = ($compose_filename . ".final", @files);
-	}
+if ($compose && $compose > 0) {
+	@files = ($compose_filename . ".final", @files);
 }
 
 # Variables we set as part of the loop over files
-our ($message_id, %mail, $subject, $reply_to, $references, $message);
+our ($message_id, %mail, $subject, $reply_to, $references, $message,
+	$needs_confirm, $message_num);
 
 sub extract_valid_address {
 	my $address = shift;
@@ -837,6 +834,25 @@ X-Mailer: git-send-email $gitversion
 	unshift (@sendmail_parameters,
 			'-f', $raw_from) if(defined $envelope_sender);
 
+	if ($needs_confirm && !$dry_run) {
+		print "\n$header\n";
+		while (1) {
+			chomp ($_ = $term->readline(
+				"Send this email? ([y]es|[n]o|[q]uit|[a]ll): "
+			));
+			last if /^(?:yes|y|no|n|quit|q|all|a)/i;
+			print "\n";
+		}
+		if (/^n/i) {
+			return;
+		} elsif (/^q/i) {
+			cleanup_compose_files();
+			exit(0);
+		} elsif (/^a/i) {
+			$confirm = 'never';
+		}
+	}
+
 	if ($dry_run) {
 		# We don't want to send the email.
 	} elsif ($smtp_server =~ m#^/#) {
@@ -935,6 +951,7 @@ X-Mailer: git-send-email $gitversion
 $reply_to = $initial_reply_to;
 $references = $initial_reply_to || '';
 $subject = $initial_subject;
+$message_num = 0;
 
 foreach my $t (@files) {
 	open(F,"<",$t) or die "can't open file $t";
@@ -943,11 +960,12 @@ foreach my $t (@files) {
 	my $author_encoding;
 	my $has_content_type;
 	my $body_encoding;
-	@cc = @initial_cc;
+	@cc = ();
 	@xh = ();
 	my $input_format = undef;
 	my @header = ();
 	$message = "";
+	$message_num++;
 	# First unfold multiline header fields
 	while(<F>) {
 		last if /^\s*$/;
@@ -1080,6 +1098,13 @@ foreach my $t (@files) {
 		}
 	}
 
+	$needs_confirm = (
+		$confirm eq "always" or
+		($confirm =~ /^(?:auto|cc)$/ && @cc) or
+		($confirm =~ /^(?:auto|compose)$/ && $compose && $message_num == 1));
+
+	@cc = (@initial_cc, @cc);
+
 	send_message();
 
 	# set up for the next message
@@ -1094,13 +1119,10 @@ foreach my $t (@files) {
 	$message_id = undef;
 }
 
-if ($compose) {
-	cleanup_compose_files();
-}
+cleanup_compose_files();
 
 sub cleanup_compose_files() {
-	unlink($compose_filename, $compose_filename . ".final");
-
+	unlink($compose_filename, $compose_filename . ".final") if $compose;
 }
 
 $smtp->quit if $smtp;
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 4df4f96..2f4a654 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -31,6 +31,11 @@ clean_fake_sendmail() {
 	rm -f commandline* msgtxt*
 }
 
+# Prevent any tests from hanging
+test_expect_success 'Set sendemail.confirm=never' '
+	git config sendemail.confirm never
+'
+
 test_expect_success 'Extract patches' '
     patches=`git format-patch -s --cc="One <one@example.com>" --cc=two@example.com -n HEAD^1`
 '
@@ -175,15 +180,13 @@ test_set_editor "$(pwd)/fake-editor"
 
 test_expect_success '--compose works' '
 	clean_fake_sendmail &&
-	echo y | \
-		GIT_SEND_EMAIL_NOTTY=1 \
-		git send-email \
-		--compose --subject foo \
-		--from="Example <nobody@example.com>" \
-		--to=nobody@example.com \
-		--smtp-server="$(pwd)/fake.sendmail" \
-		$patches \
-		2>errors
+	git send-email \
+	--compose --subject foo \
+	--from="Example <nobody@example.com>" \
+	--to=nobody@example.com \
+	--smtp-server="$(pwd)/fake.sendmail" \
+	$patches \
+	2>errors
 '
 
 test_expect_success 'first message is compose text' '
@@ -375,15 +378,56 @@ test_expect_success '--suppress-cc=cc' '
 	test_suppression cc
 '
 
+test_confirm () {
+	echo y | \
+		GIT_SEND_EMAIL_NOTTY=1 \
+		git send-email \
+		--from="Example <nobody@example.com>" \
+		--to=nobody@example.com \
+		--smtp-server="$(pwd)/fake.sendmail" \
+		$@ \
+		$patches | grep "Send this email"
+}
+
+test_expect_success '--confirm=always' '
+	test_confirm --confirm=always --suppress-cc=all
+'
+
+test_expect_success '--confirm=auto' '
+	test_confirm --confirm=auto
+'
+
+test_expect_success '--confirm=cc' '
+	test_confirm --confirm=cc
+'
+
+test_expect_success '--confirm=compose' '
+	test_confirm --confirm=compose --compose
+'
+
+test_expect_success 'confirm by default (due to cc)' '
+	CONFIRM=$(git config --get sendemail.confirm) &&
+	git config --unset sendemail.confirm &&
+	test_confirm &&
+	git config sendemail.confirm $CONFIRM
+'
+
+test_expect_success 'confirm by default (due to --compose)' '
+	CONFIRM=$(git config --get sendemail.confirm) &&
+	git config --unset sendemail.confirm &&
+	test_confirm --suppress-cc=all --compose
+	ret="$?"
+	git config sendemail.confirm ${CONFIRM:-never}
+	test $ret = "0"
+'
+
 test_expect_success '--compose adds MIME for utf8 body' '
 	clean_fake_sendmail &&
 	(echo "#!$SHELL_PATH" &&
 	 echo "echo utf8 body: àéìöú >>\"\$1\""
 	) >fake-editor-utf8 &&
 	chmod +x fake-editor-utf8 &&
-	echo y | \
 	  GIT_EDITOR="\"$(pwd)/fake-editor-utf8\"" \
-	  GIT_SEND_EMAIL_NOTTY=1 \
 	  git send-email \
 	  --compose --subject foo \
 	  --from="Example <nobody@example.com>" \
@@ -405,9 +449,7 @@ test_expect_success '--compose respects user mime type' '
 	 echo " echo utf8 body: àéìöú) >\"\$1\""
 	) >fake-editor-utf8-mime &&
 	chmod +x fake-editor-utf8-mime &&
-	echo y | \
 	  GIT_EDITOR="\"$(pwd)/fake-editor-utf8-mime\"" \
-	  GIT_SEND_EMAIL_NOTTY=1 \
 	  git send-email \
 	  --compose --subject foo \
 	  --from="Example <nobody@example.com>" \
@@ -421,9 +463,7 @@ test_expect_success '--compose respects user mime type' '
 
 test_expect_success '--compose adds MIME for utf8 subject' '
 	clean_fake_sendmail &&
-	echo y | \
 	  GIT_EDITOR="\"$(pwd)/fake-editor\"" \
-	  GIT_SEND_EMAIL_NOTTY=1 \
 	  git send-email \
 	  --compose --subject utf8-sübjëct \
 	  --from="Example <nobody@example.com>" \
@@ -445,7 +485,7 @@ test_expect_success 'detects ambiguous reference/file conflict' '
 test_expect_success 'feed two files' '
 	rm -fr outdir &&
 	git format-patch -2 -o outdir &&
-	GIT_SEND_EMAIL_NOTTY=1 git send-email \
+	git send-email \
 	--dry-run \
 	--from="Example <nobody@example.com>" \
 	--to=nobody@example.com \
-- 
1.6.2.rc1.309.g5f417

^ permalink raw reply related

* Re: [PATCH 3/4] diffcore-pickaxe: further refactor count_match()
From: Junio C Hamano @ 2009-03-01  7:31 UTC (permalink / raw)
  To: René Scharfe; +Cc: git
In-Reply-To: <49A937B8.1030205@lsrfire.ath.cx>

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:

> I get this (Ubuntu 8.10 x64, Fedora 10 x64 using the same Linux repo,
> Windows Vista x64 using a different Linux repo with the same HEAD on
> NTFS and msysgit, numbers are the elapsed time in seconds, best of five
> runs):
>
>                            Ubuntu  Fedora  Windows
>    v1.6.2-rc2                8.14    8.16    9.236
>    v1.6.2-rc2+[1-4]          2.43    2.45    2.995
>    v1.6.2-rc2+[1-4]+memmem   1.31    1.25    2.917
>    v1.6.2-rc2+[1-3]+memmem   1.51    1.16    8.455
>
> Ubuntu has glibc 2.8, while Fedora 10 has glibc 2.9, with a new and more
> efficient memmem() implementation.  On Windows, we use our own naive
> memmem() implementation.

Shoot, I was not being careful enough.

> So using memmem() is worthwhile.  And providing a better fall-back
> version in compat/ can speed up this particular case to the point where
> the fourth patch becomes moot.

Are these numbers telling me that with a good memmem() implementation,
patch 4/4 is not just moot but actively detrimental?

With a long enough needle, it entirely is possible that scanning the whole
image with sublinear string search algorithm would perform much better
than the preprocessing patch 4/4 does which has to scan all the bytes in
the common parts.

^ permalink raw reply

* Re: Changing the defaults for send-email / suppress-cc ?
From: Jay Soffian @ 2009-03-01  7:05 UTC (permalink / raw)
  To: Nanako Shiraishi; +Cc: Junio C Hamano, Paul Gortmaker, git
In-Reply-To: <76718490902282259q39da4267r34d169ec200704ba@mail.gmail.com>

On Sun, Mar 1, 2009 at 1:59 AM, Jay Soffian <jaysoffian@gmail.com> wrote:
> fun, so I was going to see if it was reasonable to link format-patch
> against msmtp. OTOH, I don't want to bloat the git binary, so it may

And msmtp is GPLv3, so nevermind anyway.

j.

^ permalink raw reply

* Re: Changing the defaults for send-email / suppress-cc ?
From: Jay Soffian @ 2009-03-01  7:01 UTC (permalink / raw)
  To: Nanako Shiraishi; +Cc: Junio C Hamano, Paul Gortmaker, git
In-Reply-To: <76718490902282259q39da4267r34d169ec200704ba@mail.gmail.com>

On Sun, Mar 1, 2009 at 1:59 AM, Jay Soffian <jaysoffian@gmail.com> wrote:
> I don't have any illusion that writing my own SMTP client in C is much
> fun, so I was going to see if it was reasonable to link format-patch
> against msmtp. OTOH, I don't want to bloat the git binary, so it may
> just be best to have format-patch be able to run msmtp automagically
> when it is called with "--email".
>
> Or something. I haven't thought real hard about it yet. :-)

OTOH, I like having format-patch spit out the patches so I can give
them a final look-over before sending them. So I dunno if it is worth
this. Perhaps format-patch should be able to invoke an SMTP server
automatically, in the same way that send-email can, but then again,
perhaps not.

Maybe I should stop this late night thinking out loud before I annoy
one too many maintainers. :-)

j.

^ permalink raw reply

* Re: Changing the defaults for send-email / suppress-cc ?
From: Jay Soffian @ 2009-03-01  6:59 UTC (permalink / raw)
  To: Nanako Shiraishi; +Cc: Junio C Hamano, Paul Gortmaker, git
In-Reply-To: <20090301153000.6117@nanako3.lavabit.com>

On Sun, Mar 1, 2009 at 1:30 AM, Nanako Shiraishi <nanako3@lavabit.com> wrote:
> Isn't it an option to introduce a new program, say 'git send', that
> reimplements what 'git send-email' is meant to be used for, but has a
> better implementation and a better default setting?

I would, at some point, like to unify format-patch, send-email, and
possible imap-send. I envision being able to "git format-patch
--email" (and possibly "git format-patch --imap").

I don't have any illusion that writing my own SMTP client in C is much
fun, so I was going to see if it was reasonable to link format-patch
against msmtp. OTOH, I don't want to bloat the git binary, so it may
just be best to have format-patch be able to run msmtp automagically
when it is called with "--email".

Or something. I haven't thought real hard about it yet. :-)

j.

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox