* Re: [RFD] Notes are independent: proposal for new notes implementation
From: Avery Pennarun @ 2010-02-09 21:37 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git, Johan Herland, Junio C Hamano, Jon Seymour
In-Reply-To: <201002092155.29304.jnareb@gmail.com>
On Tue, Feb 9, 2010 at 3:55 PM, Jakub Narebski <jnareb@gmail.com> wrote:
> On Tue, 9 Feb 2010, Avery Pennarun wrote:
>> 2010/2/9 Jakub Narebski <jnareb@gmail.com>:
>> > But
>> > what if the answer was to change implementation, decoupling history of
>> > notes from each other, and keeping history of each note separate.
>>
>> Congratulations, you've re-invented CVS! :)
>>
>> Seriously though, I'm not sure what problems this solved. Notes that
>> are related to each other can (and perhaps should) be in the same
>> notes commit history; notes that are not related to each other can
>> exist in separate histories with their own ref.
>
> The problem is (as I see it) that notes are _not_ (in almost all cases)
> related to each other, just like files in $HOME or in /etc are not
> related to each other.
As a side note, I didn't find this example compelling at all. I
*absolutely* want to manage all my files in /etc as a single repo.
"The configuration of my computer" is an ongoing project where the
configuration of my smtp daemon depends on /etc/hosts and /etc/passwd
and /etc/group. If I set up another server, I want to be able to fork
my basic configuration and apply some patches. If I set up some
clever aliases in /etc/profile, I want to send that patch "upstream"
to the /etc project on my other servers.
Similarly with $HOME; the evolution of my home directory over time is
a thing I can talk about as a sensible whole, and of course I want
rename tracking and deltas and so on.
Combining /etc and $HOME into a single repo would be harder to
justify. But that sounds to me like the "kind (flavor)" distinction
you're talking about; system config files and personal files are two
different kinds of files.
>> > This means for example that if in repository A somebody annotated
>> > commits foo and bar creating notes in this order, and in repository B
>> > somebody annotated bar and foo (creating notes in reverse order), then
>> > merging those changes would require generating merge commit even if
>> > those notes are identical.
>>
>> That's a feature; now you have the true history of your notes, which
>> is good for all the same reasons it's good in git.
>
> No, you are introducing artificial ordering in something that is a bag,
> unordered collection.
I would put it another way: you're recording the true ordering of
something where the ordering *may* not be important. It is easy to
ignore that ordering.
However, it's very hard to unignore ordering that you didn't record in
the first place. That's why CVS's model of recording
one-history-per-file is so nasty. Yet it seemed so clever when they
invented it.
What is a real use case where the "artificial ordering" causes a problem?
Here's a use case where having a single history would be a clear
benefit: say you're running an autobuilder such as gitbuilder[1].
Something goes wrong with your autobuild environment, like the disk
fills up, and all the build results since yesterday at noon are
invalid. If the build results are stored as a single history of git
notes[2], I can just rewind the history to yesterday at noon and
discard the entire sequence of bad results across all commits. If
they each had their own history, it would be more complex to
implement.
Have fun,
Avery
[1] http://github.com/apenwarr/gitbuilder
[2] gitbuilder doesn't currently use git-notes, it just uses plain files.
^ permalink raw reply
* [PATCH] test for add with non-existent pathspec
From: Chris Packham @ 2010-02-09 21:31 UTC (permalink / raw)
To: git; +Cc: peff, szeder, Chris Packham
In-Reply-To: <a038bef51002090643g23b35bb6xaa765ff13b80f5bb@mail.gmail.com>
Add a test for 'git add -u pathspec' and 'git add pathspec' where
pathspec does not exist. The expected result is that git add exits with
an error message and an appropriate exit code.
This adds 1 expected failure to t/t2200-add-update.sh.
Signed-off-by: Chris Packham <judge.packham@gmail.com>
---
If anyone feels like pursuing this heres a patch to two simple tests around
this behaviour. I made an initial attempt to change builtin-add.c to print an
error message and set the exit code to a non-zero value but my naive attempt
broke the normal usage of 'git add pathspec'.
t/t2200-add-update.sh | 5 +++++
t/t3700-add.sh | 5 +++++
2 files changed, 10 insertions(+), 0 deletions(-)
diff --git a/t/t2200-add-update.sh b/t/t2200-add-update.sh
index 9120750..dbabc3c 100755
--- a/t/t2200-add-update.sh
+++ b/t/t2200-add-update.sh
@@ -176,4 +176,9 @@ test_expect_success 'add -u resolves unmerged paths' '
'
+test_expect_failure 'error out when attempting to add -u non-existent pathspec' '
+ test_must_fail git add -u non-existent &&
+ ! (git ls-files | grep "non-existent")
+'
+
test_done
diff --git a/t/t3700-add.sh b/t/t3700-add.sh
index 85eb0fb..c77bb71 100755
--- a/t/t3700-add.sh
+++ b/t/t3700-add.sh
@@ -255,4 +255,9 @@ test_expect_success 'git add to resolve conflicts on otherwise ignored path' '
git add track-this
'
+test_expect_success 'error out when attempting to add non-existent pathspec' '
+ test_must_fail git add non-existent &&
+ ! (git ls-files | grep "non-existent")
+'
+
test_done
--
1.6.4.2
^ permalink raw reply related
* Re: Better cooperation between checkouts and stashing
From: Markus Elfring @ 2010-02-09 21:01 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vbpfy9nt4.fsf@alter.siamese.dyndns.org>
>
> But I am at loss guessing what dependencies among what "subfunctions"
> you are talking about.
>
I propose further clarifications and improvements between the git
functions "checkout" and "stash".
Regards,
Markus
^ permalink raw reply
* Re: [RFD] Notes are independent: proposal for new notes implementation
From: Jakub Narebski @ 2010-02-09 20:55 UTC (permalink / raw)
To: Avery Pennarun; +Cc: git, Johan Herland, Junio C Hamano, Jon Seymour
In-Reply-To: <32541b131002091226p58b40237j40d3cd6cfaa91df5@mail.gmail.com>
On Tue, 9 Feb 2010, Avery Pennarun wrote:
> 2010/2/9 Jakub Narebski <jnareb@gmail.com>:
> > But
> > what if the answer was to change implementation, decoupling history of
> > notes from each other, and keeping history of each note separate.
>
> Congratulations, you've re-invented CVS! :)
>
> Seriously though, I'm not sure what problems this solved. Notes that
> are related to each other can (and perhaps should) be in the same
> notes commit history; notes that are not related to each other can
> exist in separate histories with their own ref.
The problem is (as I see it) that notes are _not_ (in almost all cases)
related to each other, just like files in $HOME or in /etc are not
related to each other. Separate notes refs for separate histories
are not IMHO a good solution: refs namespaces are about *kind* (flavor)
of notes: commits annotations, bisect, git-svn, apply-email, bugs / tickets,
etc. and each flavor (kind) of notes contain many independent notes.
This is opposed to workspace history, where history (in almost all cases)
makes sense only of all files, history of a project as a whole.
And of course we would have atomic commits, merge tracking, support for
renames etc., something like Zit[1]
[1]: http://git.wiki.kernel.org/index.php/InterfacesFrontendsAndTools#Zit
> > This means for example that if in repository A somebody annotated
> > commits foo and bar creating notes in this order, and in repository B
> > somebody annotated bar and foo (creating notes in reverse order), then
> > merging those changes would require generating merge commit even if
> > those notes are identical.
>
> That's a feature; now you have the true history of your notes, which
> is good for all the same reasons it's good in git.
No, you are introducing artificial ordering in something that is a bag,
unordered collection.
> Of course this whole line of reasoning could lead to questions like
> "can I rebase my notes history?" and "what about rebase -i" and "can I
> maintain a notes patch queue" and so on.
--
Jakub Narebski
Poland
^ permalink raw reply
* Re: [RFD] Notes are independent: proposal for new notes implementation
From: Avery Pennarun @ 2010-02-09 20:26 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git, Johan Herland, Junio C Hamano, Jon Seymour
In-Reply-To: <201002092105.25636.jnareb@gmail.com>
2010/2/9 Jakub Narebski <jnareb@gmail.com>:
> But
> what if the answer was to change implementation, decoupling history of
> notes from each other, and keeping history of each note separate.
Congratulations, you've re-invented CVS! :)
Seriously though, I'm not sure what problems this solved. Notes that
are related to each other can (and perhaps should) be in the same
notes commit history; notes that are not related to each other can
exist in separate histories with their own ref.
> This means for example that if in repository A somebody annotated
> commits foo and bar creating notes in this order, and in repository B
> somebody annotated bar and foo (creating notes in reverse order), then
> merging those changes would require generating merge commit even if
> those notes are identical.
That's a feature; now you have the true history of your notes, which
is good for all the same reasons it's good in git.
Of course this whole line of reasoning could lead to questions like
"can I rebase my notes history?" and "what about rebase -i" and "can I
maintain a notes patch queue" and so on.
Have fun,
Avery
^ permalink raw reply
* Re: Better cooperation between checkouts and stashing
From: Junio C Hamano @ 2010-02-09 20:06 UTC (permalink / raw)
To: Markus Elfring; +Cc: Junio C Hamano, git
In-Reply-To: <4B71B574.3010609@web.de>
Markus Elfring <Markus.Elfring@web.de> writes:
>>
>> We _can_ start experimenting with an option (similar to "checkout -m"),
>> which does:
>>
>
> Does the documentation need any improvements on the dependencies between
> the mentioned subfunctions?
Sorry, you lost me. I did say and still think that it would be good to
have in the documentation a separate section that teaches the users what
happens to their uncommitted changes when checking out a different branch
(or an unnamed branch). But I am at loss guessing what dependencies among
what "subfunctions" you are talking about.
^ permalink raw reply
* [RFD] Notes are independent: proposal for new notes implementation
From: Jakub Narebski @ 2010-02-09 20:05 UTC (permalink / raw)
To: git; +Cc: Johan Herland, Junio C Hamano, Jon Seymour
Junio have noticed in one of threads about notes implementation in
git[*1*] that current notes implementation has (conceptual) problems:
[1] "Re: A generalization of git notes from blobs to trees - git metadata?"
Message-ID: <7v8wb4aj4m.fsf@alter.siamese.dyndns.org>
http://permalink.gmane.org/gmane.comp.version-control.git/139252
(its one of threads that IIRC started with implementing hand-rolled
support for notes in gitweb by Giuseppe Bilotta)
Junio C Hamano <gitster@pobox.com> writes:
JH>
JH> It's [current notes implementation] like "keeping track of /etc" (or
JH> "your home directory"). It is a misguided thing to do because you
JH> are throwing in records of the states of totally unrelated things
JH> into a single history (e.g. "Why does it matter I added new user
JH> frotz to /etc/passwd before I futzed with my sendmail configuration?
JH> ---It shouldn't matter; there shouldn't be ancestry relationships
JH> between these two changes"). I somehow feel that keeping track of
JH> the "growth of the bag of annotations to any and all commits" in a
JH> single history may be making the same mistake.
The proposed solution was to use custom merge strategy for notes. But
what if the answer was to change implementation, decoupling history of
notes from each other, and keeping history of each note separate.
Let's simplify situation, and talk for now about single notes namespace
(refs/notes/commits), no fanout scheme, and plain blob notes.
In CURRENT notes implementation the notes ref (e.g. refs/notes/commits)
point to a commit object: the tip of history of all notes. This commit
stores information about last change to any note; it's commit message is
"Annotate <SHA-1>". It's tree contains mapping between notes and
annotated object: notes are stored as leafs in the tree, and their
pathnames are (representing) objects they are annotating.
This means for example that if in repository A somebody annotated
commits foo and bar creating notes in this order, and in repository B
somebody annotated bar and foo (creating notes in reverse order), then
merging those changes would require generating merge commit even if
those notes are identical.
tree: <-- Annotate bar <-- refs/notes/commits
<foo note SHA-1> <foo SHA-1> |
<bar note SHA-1> <bar SHA-1> | (parent)
|
v
tree: <-- Annotate foo
<foo note SHA-1> <foo SHA-1> (no parent)
The PROPOSED solution (with admittedly larger overhead) is to have notes
history stored in submodule-like fashion. The notes ref would point to
the tree object. In this tree each leaf would point to a *commit*
representing tip of history for a given note (like for submodules).
Each commit would contain tree, which would map note to annotated object
(it is extra level of indirection, needed because commit cannot point to
blob directly... unless multiple notes for the same commit in tree
structure got implemented, or tree annotations got implemented.)
This way history of each note is in kind of a separate branch, and notes
refs point to tree object representing branch hierarchy.
Merge conflict would appear only if notes for the same object would have
different contents or/and different history.
tree: <-- refs/notes/commits
/------------ <foo hist SHA-1> <foo SHA-1>
| /-- <bar hist SHA-1> <bar SHA-1>
| |
| v
| Annotate bar --> tree:
| (no parent) <bar note SHA-1> <bar SHA-1>
v
Annotate foo ----------> tree
(no parent) <foo note SHA-1> <foo SHA-1>
One thing that would need to be addressed is converting from older notes
implementation, but this should be doable. The problem would be in
supporting both implementations in one repository; it might be not
possible. Also this would break compatibility: older git versions
supporting notes wouldn't be able, I guess, to access new (proposed)
format.
There are probably numerous issues with proposed implementation, beside
breaking backward compatibility...
P.S. This shows why git tools (such as gitweb) should not access notes
directly, but use git-notes, %N / %N(<ref>) format specifier, and proposed
<object>^@{} / object^@{<ref>} or <object>^{notes} / <object>^{notes:<ref>}
--
Jakub Narebski
Poland
^ permalink raw reply
* Re: t5401-update-hooks test failure
From: Shawn O. Pearce @ 2010-02-09 19:26 UTC (permalink / raw)
To: Nicolas Pitre; +Cc: Junio C Hamano, Larry D'Anna, Jeff King, git
In-Reply-To: <alpine.LFD.2.00.1002091337421.1681@xanadu.home>
Nicolas Pitre <nico@fluxnic.net> wrote:
> On Tue, 9 Feb 2010, Shawn O. Pearce wrote:
> >
> > builtin-send-pack.c clearly isn't stopping early while processing
> > the stream, since we see later messages from the post-receive and
> > post-update hooks just fine.
> >
> > So I think the only code that is in question is the case 2 arm of
> > recv_sideband(). But to be honest, I can't find any fault with it.
>
> Note that strict order of messages passed through the sideband can't be
> relied upon. Often you have sideband 1 connected to stdin and sideband
> 2 connected to stderr,
Oh. Sure. But that isn't really the case here.
The messages are all coming down side band #2, before we write
anything down side band #1. The missing message in question
should have appeared somewhere in the middle of that side band
#2 stream that we are seeing in the test output. Given that its
all serialized down into a single stream by the parent process
receive-pack, we really shouldn't see the messages out of order.
> and they are linked with pipes, and various
> factors such as stdio buffering or even printf implementation in the C
> lib
The only way I can see this missing message happening is if the C
library isn't flushing the stdio buffer before the hook process
exits. Given that the hook process is a /bin/sh shell script,
and its using echo to print its messages... I'm at a loss for how
to fix that in Git.
Unless its the recv_sideband() somehow skipping a line. But I
can't see it doing that.
--
Shawn.
^ permalink raw reply
* Re: [PATCH 3/4] Support working directory located at root
From: Johannes Sixt @ 2010-02-09 19:19 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy
Cc: git, João Carlos Mendes Luís, Junio C Hamano
In-Reply-To: <1265734950-15145-3-git-send-email-pclouds@gmail.com>
On Dienstag, 9. Februar 2010, Nguyễn Thái Ngọc Duy wrote:
> I said I would have code change for DOS drive too. But I take it back.
> Supporting GIT_DIR=C:\.git might be easy, GIT_DIR=C:.git is not.
One does not set GIT_DIR=C:.git; it would be insane because it means ".git in
an unpredictable directory somewhere on drive C". It would be great to
support GIT_DIR=C:/.git
-- Hannes
^ permalink raw reply
* Re: t5401-update-hooks test failure
From: Nicolas Pitre @ 2010-02-09 19:20 UTC (permalink / raw)
To: Shawn O. Pearce; +Cc: Junio C Hamano, Larry D'Anna, Jeff King, git
In-Reply-To: <20100209175139.GA28936@spearce.org>
On Tue, 9 Feb 2010, Shawn O. Pearce wrote:
> Junio C Hamano <gitster@pobox.com> wrote:
> > $ while sh t5401-*.sh -i; do :; done
> > ... wait for a while ...
> > * FAIL 12: send-pack stderr contains hook messages
> >
> > grep ^remote: send.err | sed "s/ *\$//" >actual &&
> > test_cmp - actual <expect
> >
> > $ t/(364643e...); cat trash\ directory.t5401-update-hooks/actual
> > remote: STDOUT pre-receive
> > remote: STDERR pre-receive
> > remote: STDOUT update refs/heads/master
> > remote: STDERR update refs/heads/master
> > remote: STDOUT update refs/heads/tofail
> > remote: STDOUT post-receive
> > remote: STDERR post-receive
> > remote: STDOUT post-update
> > remote: STDERR post-update
> > $ t/(364643e...); cat trash\ directory.t5401-update-hooks/expect
> > remote: STDOUT pre-receive
> > remote: STDERR pre-receive
> > remote: STDOUT update refs/heads/master
> > remote: STDERR update refs/heads/master
> > remote: STDOUT update refs/heads/tofail
> > remote: STDERR update refs/heads/tofail
> > remote: STDOUT post-receive
> > remote: STDERR post-receive
> > remote: STDOUT post-update
> > remote: STDERR post-update
>
> A quick visual inspection shows that only the STDERR tofail message
> is missing here. That sounds to me like a race condition in the
> recv_sideband decoder. Or, a race condition in the hook code in
> builtin-receive-pack.c.
>
> I doubt its in receive-pack. run_update_hook() directly calls the
> copy_to_sideband() function, and that reads until EOF on the hook's
> stderr stream before it returns and waits for the hook's exit status.
> So we should be pulling everything and dumping it into the sideband.
>
> builtin-send-pack.c clearly isn't stopping early while processing
> the stream, since we see later messages from the post-receive and
> post-update hooks just fine.
>
> So I think the only code that is in question is the case 2 arm of
> recv_sideband(). But to be honest, I can't find any fault with it.
Note that strict order of messages passed through the sideband can't be
relied upon. Often you have sideband 1 connected to stdin and sideband
2 connected to stderr, and they are linked with pipes, and various
factors such as stdio buffering or even printf implementation in the C
lib, pipe buffers in the OS, random scheduling between the processes
involved, and other factors such as disk or network speed vs CPU speed,
are all adding to a certain degree of randomness affecting how the data
end up in various channels wrt each other.
See my efforts to reduce that issue so far with those commits:
6b59f51b312f06d9420d34c09fa408c658aac6d2
6b9c42b4daabe3d0471d9f90d2ae472c9d994e1c
d048a96ee9bec968be0bdc9c43ffce61169545be
ebe8fa738dcf6911fe520adce0cfa0cb26dee5e2
Nicolas
^ permalink raw reply
* Re: Better cooperation between checkouts and stashing
From: Markus Elfring @ 2010-02-09 19:20 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vhbq0wuy6.fsf@alter.siamese.dyndns.org>
>
> We _can_ start experimenting with an option (similar to "checkout -m"),
> which does:
>
Does the documentation need any improvements on the dependencies between
the mentioned subfunctions?
Would we like to collect further ideas on the suggested issue?
Regards,
Markus
^ permalink raw reply
* Re: [PATCH 2/4] rev-parse: make --git-dir return /.git instead of //.git
From: Johannes Sixt @ 2010-02-09 19:18 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy
Cc: git, João Carlos Mendes Luís, Junio C Hamano
In-Reply-To: <1265734950-15145-2-git-send-email-pclouds@gmail.com>
On Dienstag, 9. Februar 2010, Nguyễn Thái Ngọc Duy wrote:
> @@ -647,7 +647,7 @@ int cmd_rev_parse(int argc, const char **argv, const
> char *prefix) }
> if (!getcwd(cwd, PATH_MAX))
> die_errno("unable to get current working directory");
> - printf("%s/.git\n", cwd);
> + printf("%s%s.git\n", cwd, *cwd == '/' && cwd[1] == '\0' ? "" : "/");
On Windows, when you are in the root of a drive, then cwd is "C:/", i.e. there
is a trailing slash just as in the Unix root directory. But you do not take
care of this situation. That is, you would print "C://".
How about:
static inline int is_root_path(const char *path)
{
if (has_dos_drive_prefix(path))
path += 2;
while (is_dir_sep(*path))
path++;
return !*path;
}
and use it though-out your series?
(Simplify the loop to 'return is_dir_sep(*path) && !path[1];' if you can
assume that paths are nomalized.)
-- Hannes
^ permalink raw reply
* Re: [PATCH 1/4] make_absolute_path(): Do not append redundant slash
From: Johannes Sixt @ 2010-02-09 19:10 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy
Cc: git, João Carlos Mendes Luís, Junio C Hamano
In-Reply-To: <1265734950-15145-1-git-send-email-pclouds@gmail.com>
On Dienstag, 9. Februar 2010, Nguyễn Thái Ngọc Duy wrote:
> @@ -54,8 +54,9 @@ const char *make_absolute_path(const char *path)
> if (len + strlen(last_elem) + 2 > PATH_MAX)
> die ("Too long path name: '%s/%s'",
> buf, last_elem);
> - buf[len] = '/';
> - strcpy(buf + len + 1, last_elem);
> + if (*buf != '/' || buf[1] != '\0')
> + buf[len++] = '/';
Huh? You are adding a slash unless buf is exactly "/". That is, when buf
is "/foo/" you still add a slash? That's not exactly avoiding redundancy.
(Disclaimer: I didn't analyze the rest of the function whether my claim is
true.)
> + strcpy(buf + len, last_elem);
> free(last_elem);
> last_elem = NULL;
> }
-- Hannes
^ permalink raw reply
* Re: [PATCH 0/4] Some improvements for git-imap-send
From: Jeff King @ 2010-02-09 18:54 UTC (permalink / raw)
To: kusmabite; +Cc: Hitoshi Mitake, gitster, git, jwhite, robertshearman
In-Reply-To: <40aa078e1002091037j226eb911v215a5564cba42142@mail.gmail.com>
On Tue, Feb 09, 2010 at 07:37:44PM +0100, Erik Faye-Lund wrote:
> > Did you mean "SASL-support that is needed for CRAM-MD5"? The SASL needed
> > for that is pretty simple. Hitoshi's patch 3/4 does all of that already
> > in less than 100 lines. Using a "real" sasl library might get us more
> > authentication methods than CRAM-MD5, but I don't know that anyone
> > necessarily cares about them.
>
> No, that's not what I meant. I agree that CRAM-MD5 should be
> sufficient, but to be honest I'd already thought that once you have an
> SSL connection, plaintext would also be sufficient. So I'm thinking of
> this addition as a "hmpf, some server requires stuff that is really
> over the top - perhaps we'll have this problem later with other
> servers, and we'd be better off just using some well-tested
> implementation". But that's kinda philosophical.
Ah, I see. Yes, it's possible that we may want to support other
authentication methods later. In my experience, CRAM-MD5 is the only
common non-plain IMAP mechanism used by IMAP, but I admit it has been
quite a number of years since I actively paid attention to such things.
I'd leave that choice to whoever feels like implementing it. :)
-Peff
^ permalink raw reply
* Re: [PATCH 0/4] Some improvements for git-imap-send
From: Erik Faye-Lund @ 2010-02-09 18:37 UTC (permalink / raw)
To: Jeff King; +Cc: Hitoshi Mitake, gitster, git, jwhite, robertshearman
In-Reply-To: <20100209165745.GA21135@coredump.intra.peff.net>
On Tue, Feb 9, 2010 at 5:57 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Feb 09, 2010 at 04:13:26PM +0100, Erik Faye-Lund wrote:
>
>> On Tue, Feb 9, 2010 at 4:06 PM, Jeff King <peff@peff.net> wrote:
>> > On Tue, Feb 09, 2010 at 09:09:01PM +0900, Hitoshi Mitake wrote:
>> >
>> >> base64.c | 122 ++++++++
>> >> base64.h | 36 +++
>> >> md5.c | 600 +++++++++++++++++++++++++++++++++++++++
>> >> md5.h | 61 ++++
>> >> md5_hmac.c | 137 +++++++++
>> >> md5_hmac.h | 36 +++
>> >
>> > That's a lot of extra code. Doesn't imap-send already conditionally
>> > compile against openssl for starttls support? Can't we just get all
>> > three of these algorithms from openssl?
>> >
>>
>> I don't think OpenSSL includes SASL-support that is needed for
>> STARTTLS. But it might make sense to use something like GSASL[1]
>> instead of rolling all the SASL-mechanisms ourselves.
>
> Did you mean "SASL-support that is needed for CRAM-MD5"? The SASL needed
> for that is pretty simple. Hitoshi's patch 3/4 does all of that already
> in less than 100 lines. Using a "real" sasl library might get us more
> authentication methods than CRAM-MD5, but I don't know that anyone
> necessarily cares about them.
>
No, that's not what I meant. I agree that CRAM-MD5 should be
sufficient, but to be honest I'd already thought that once you have an
SSL connection, plaintext would also be sufficient. So I'm thinking of
this addition as a "hmpf, some server requires stuff that is really
over the top - perhaps we'll have this problem later with other
servers, and we'd be better off just using some well-tested
implementation". But that's kinda philosophical.
> But using openssl to replace the low-level routines in patches 1+2 would
> drop almost 1000 lines, and not significantly change his 3/4.
>
> Personally, I don't care either way about using a SASL library. It's an
> extra dependency, but one that is optional for this feature. But
> somebody will have to do the work to integrate it, whereas I think using
> openssl is only a few lines of change. If somebody wants to do that
> work, then great.
>
I agree.
--
Erik "kusma" Faye-Lund
^ permalink raw reply
* Re: [PATCH 4/4] git-imap-send: Add method to convert from LF to CRLF
From: Junio C Hamano @ 2010-02-09 18:20 UTC (permalink / raw)
To: Linus Torvalds
Cc: Hitoshi Mitake, gitster, git, Jeremy White, Robert Shearman
In-Reply-To: <alpine.LFD.2.00.1002090918180.3829@localhost.localdomain>
Linus Torvalds <torvalds@linux-foundation.org> writes:
> So I suspect that CRLF should be unconditional.
That matches my reading of the RFC. Thanks.
Hitoshi, can we have a modified version of 4/4 as a separate patch, so
that we can apply it independently from the rest of the series?
As to the MD5 implementation, I am somewhat torn.
Even if your md5 implementation were vastly superiour and faster than
OpenSSL one (I don't know), the use of MD5 is not performance critical
like SHA-1 is (for which we uniformly use Linus's block SHA-1 these days);
the only thing it would be buying us to have our own implementation is one
less dependency for people who do want to use imap-send with CRAM-MD5 but
without SSL support. How common is that combination?
^ permalink raw reply
* Re: [PATCH] blame: prevent a segv when -L given start > EOF
From: Junio C Hamano @ 2010-02-09 18:06 UTC (permalink / raw)
To: Jay Soffian; +Cc: Johannes Sixt, git
In-Reply-To: <76718491002090955g66eca03ar7438e57811616267@mail.gmail.com>
Sorry for knowing but forgetting about the issue while applying.
I'll queue this fix-up.
-- >8 --
t8003: check exit code of command and error message separately
Shell reports exit status only from the most downstream command
in a pipeline. In these tests, we want to make sure that the
command fails in a controlled way, and produces a correct error
message.
This issue was known by Jay who submitted the patch, and also was
pointed out by Hannes during the review process, but I forgot to
fix it up before applying. Sorry about that.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
t/t8003-blame.sh | 6 ++++--
1 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/t/t8003-blame.sh b/t/t8003-blame.sh
index 4a8db74..3bbddd0 100755
--- a/t/t8003-blame.sh
+++ b/t/t8003-blame.sh
@@ -158,11 +158,13 @@ EOF
'
test_expect_success 'blame -L with invalid start' '
- test_must_fail git blame -L5 tres 2>&1 | grep "has only 2 lines"
+ test_must_fail git blame -L5 tres 2>errors &&
+ grep "has only 2 lines" errors
'
test_expect_success 'blame -L with invalid end' '
- git blame -L1,5 tres 2>&1 | grep "has only 2 lines"
+ test_must_fail git blame -L1,5 tres 2>errors &&
+ grep "has only 2 lines" errors
'
test_done
^ permalink raw reply related
* Re: Fix signal handler
From: Markus Elfring @ 2010-02-09 18:01 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20100202205849.GA14385@sigill.intra.peff.net>
>
> If you can demonstrate a practical problem and provide a patch, then I
> am sure people would be happy to read it.
>
I need a few further clarifications on this issue to choose a potential fix.
I have noticed that the variable "show_early_output" gets a value
assigned only at a few places in the source code. I wonder that the set
pointer is only used by the function "limit_list" to call the function
"log_show_early" on demand.
http://git.kernel.org/?p=git/git.git;a=blob;f=revision.c;h=3ba6d991f6e9789949c314c2981dfc6b208a6f66;hb=HEAD#l683
I find that a simple flag would be sufficient. I see no need to handle
different function pointers here. Do any objections exist to achieve the
same effect with the data type "sig_atomic_t"?
Regards,
Markus
^ permalink raw reply
* [PATCH v2] blame: prevent a segv when -L given start > EOF
From: Jay Soffian @ 2010-02-09 17:59 UTC (permalink / raw)
To: git, Junio C Hamano; +Cc: Jay Soffian, Johannes Sixt
In-Reply-To: <4B71122F.3040809@viscovery.net>
blame would segv if given -L <lineno> with <lineno> past the end of the file.
While we're fixing the bug, add test cases for an invalid <start> when called
as -L <start>,<end> or -L<start>.
Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
Modified the tests per Hannes recommendations.
builtin-blame.c | 2 +-
t/t8003-blame.sh | 10 ++++++++++
2 files changed, 11 insertions(+), 1 deletions(-)
diff --git a/builtin-blame.c b/builtin-blame.c
index 6408ec8..10f7eac 100644
--- a/builtin-blame.c
+++ b/builtin-blame.c
@@ -2433,7 +2433,7 @@ parse_done:
if (top < 1)
top = lno;
bottom--;
- if (lno < top)
+ if (lno < top || lno < bottom)
die("file %s has only %lu lines", path, lno);
ent = xcalloc(1, sizeof(*ent));
diff --git a/t/t8003-blame.sh b/t/t8003-blame.sh
index ad834f2..be648b2 100755
--- a/t/t8003-blame.sh
+++ b/t/t8003-blame.sh
@@ -157,4 +157,14 @@ EOF
git --no-pager blame $COMMIT -- uno >/dev/null
'
+test_expect_success 'blame -L with invalid start' '
+ test_must_fail git blame -L5 tres >output 2>&1 &&
+ grep "has only 2 lines" output
+'
+
+test_expect_success 'blame -L with invalid end' '
+ test_must_fail git blame -L1,5 tres >output 2>&1 &&
+ grep "has only 2 lines" output
+'
+
test_done
--
1.7.0.rc1.200.g9c1f9
^ permalink raw reply related
* Re: git + davfs2, is it safe?
From: Shawn O. Pearce @ 2010-02-09 17:57 UTC (permalink / raw)
To: G?bor Farkas; +Cc: git
In-Reply-To: <2c173a351002090839v45d01d5dt17e4b4af37e84615@mail.gmail.com>
G?bor Farkas <gabor@nekomancer.net> wrote:
> for various reasons our git repositories are hosted using https (webdav),
> which is http-auth and client-certificate authenticated.
>
> (linux on both the server and the client)
>
> it's possible to somehow persuade git to push/fetch from such repositories,
> but it's very annoying because either you have to enter your
> username+password+cert_password
> on every fetch and twice on every push, or you have to write them down
> in $HOME/.netrc .
This is why I prefer SSH. You have ssh-agent around to retain your
key for you.
A more modern Git (1.6.6 or later IIRC) should avoid prompting for
the password twice, because the HTTP client should actually reuse
the connection for the subsequent requests. But I haven't tested it.
You might also want to consider upgrading your server to post-1.6.6
and stop using WebDAV... but instead use smart HTTP for push.
That should only need one authentication cycle to perform the push.
It sounds like its time for a proper curl-agent for libcurl though.
Users and applications shouldn't have to keep implementing their
password stores as text files...
--
Shawn.
^ permalink raw reply
* Re: [PATCH] blame: prevent a segv when -L given start > EOF
From: Jay Soffian @ 2010-02-09 17:55 UTC (permalink / raw)
To: Johannes Sixt; +Cc: git, Junio C Hamano
In-Reply-To: <4B71122F.3040809@viscovery.net>
On Tue, Feb 9, 2010 at 2:43 AM, Johannes Sixt <j.sixt@viscovery.net> wrote:
> Jay Soffian schrieb:
>> +test_expect_success 'blame -L with invalid start' '
>> + test_must_fail git blame -L5 tres 2>&1 | grep "has only 2 lines"
>
> Please write this as
>
> test_must_fail git blame -L5 tres >output 2>&1 &&
> grep "has only 2 lines" output
>
>> +'
>> +
>> +test_expect_success 'blame -L with invalid end' '
>> + git blame -L1,5 tres 2>&1 | grep "has only 2 lines"
>
> test_must_fail git blame -L1,5 tres >output 2>&1 &&
> grep "has only 2 lines" output
>
> because shells look only at the exit code of the last command in a pipeline.
Thanks, I knew that. I'd left in test_must_fail accidentally because
initially I wasn't bothering to grep the output. I then added the grep
and forgot to remove test_must_fail. Isn't this an adequate test:
test_expect_success 'blame -L with invalid start' '
git blame -L5 tres 2>&1 | grep "has only 2 lines"
As it seems unlikely git would crash and still output the message
correctly in this case.
?
j.
^ permalink raw reply
* Re: t5401-update-hooks test failure
From: Shawn O. Pearce @ 2010-02-09 17:51 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Larry D'Anna, Jeff King, git
In-Reply-To: <7v4olqlva7.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> wrote:
> $ while sh t5401-*.sh -i; do :; done
> ... wait for a while ...
> * FAIL 12: send-pack stderr contains hook messages
>
> grep ^remote: send.err | sed "s/ *\$//" >actual &&
> test_cmp - actual <expect
>
> $ t/(364643e...); cat trash\ directory.t5401-update-hooks/actual
> remote: STDOUT pre-receive
> remote: STDERR pre-receive
> remote: STDOUT update refs/heads/master
> remote: STDERR update refs/heads/master
> remote: STDOUT update refs/heads/tofail
> remote: STDOUT post-receive
> remote: STDERR post-receive
> remote: STDOUT post-update
> remote: STDERR post-update
> $ t/(364643e...); cat trash\ directory.t5401-update-hooks/expect
> remote: STDOUT pre-receive
> remote: STDERR pre-receive
> remote: STDOUT update refs/heads/master
> remote: STDERR update refs/heads/master
> remote: STDOUT update refs/heads/tofail
> remote: STDERR update refs/heads/tofail
> remote: STDOUT post-receive
> remote: STDERR post-receive
> remote: STDOUT post-update
> remote: STDERR post-update
A quick visual inspection shows that only the STDERR tofail message
is missing here. That sounds to me like a race condition in the
recv_sideband decoder. Or, a race condition in the hook code in
builtin-receive-pack.c.
I doubt its in receive-pack. run_update_hook() directly calls the
copy_to_sideband() function, and that reads until EOF on the hook's
stderr stream before it returns and waits for the hook's exit status.
So we should be pulling everything and dumping it into the sideband.
builtin-send-pack.c clearly isn't stopping early while processing
the stream, since we see later messages from the post-receive and
post-update hooks just fine.
So I think the only code that is in question is the case 2 arm of
recv_sideband(). But to be honest, I can't find any fault with it.
I've been running this test in a while loop for a while now, and
I can't make it trigger this failure. Maybe its possible that
its the update hook itself, not flushing its stderr buffer before
it terminates?
--
Shawn.
^ permalink raw reply
* Re: [PATCH v2 6/6] receive-pack: Send hook output over side band #2
From: Larry D'Anna @ 2010-02-09 17:41 UTC (permalink / raw)
To: Shawn O. Pearce; +Cc: git
In-Reply-To: <20100209173307.GA1115@cthulhu>
* Larry D'Anna (larry@elder-gods.org) [100209 12:33]:
> * Shawn O. Pearce (spearce@spearce.org) [100209 12:20]:
> > Larry D'Anna <larry@elder-gods.org> wrote:
> > >
> > > This breaks t5401. See <7v4olqlva7.fsf@alter.siamese.dyndns.org> in another thread.
> >
> > No. Your patch causes t5401 to break. If you apply this series on
> > top of maint, its fine. If you merge this series into master, and
> > correctly fix the Win32 merge conflict in run-command.c, its fine.
>
> Well, the version of this commit that's in pu is defiantly failing t5401 on my
> machine. Just tested it again with a clean build. My series isn't an ancestor
> of it either. Did you notice the failure is intermittent? You have to run it a
> few times before it fails.
I also just tried applying the series on top of maint myself, same result.
--larry
^ permalink raw reply
* Re: [PATCH v2 6/6] receive-pack: Send hook output over side band #2
From: Larry D'Anna @ 2010-02-09 17:33 UTC (permalink / raw)
To: Shawn O. Pearce; +Cc: git
In-Reply-To: <20100209172027.GC26964@spearce.org>
* Shawn O. Pearce (spearce@spearce.org) [100209 12:20]:
> Larry D'Anna <larry@elder-gods.org> wrote:
> >
> > This breaks t5401. See <7v4olqlva7.fsf@alter.siamese.dyndns.org> in another thread.
>
> No. Your patch causes t5401 to break. If you apply this series on
> top of maint, its fine. If you merge this series into master, and
> correctly fix the Win32 merge conflict in run-command.c, its fine.
Well, the version of this commit that's in pu is defiantly failing t5401 on my
machine. Just tested it again with a clean build. My series isn't an ancestor
of it either. Did you notice the failure is intermittent? You have to run it a
few times before it fails.
--larry
^ permalink raw reply
* Re: [PATCH 4/4] git-imap-send: Add method to convert from LF to CRLF
From: Linus Torvalds @ 2010-02-09 17:24 UTC (permalink / raw)
To: Hitoshi Mitake; +Cc: gitster, git, Jeremy White, Robert Shearman
In-Reply-To: <1265717345-2118-5-git-send-email-mitake@dcl.info.waseda.ac.jp>
On Tue, 9 Feb 2010, Hitoshi Mitake wrote:
>
> Some strict IMAP servers (e.g. Cyrus) don't
> allow "bare newlines ('\n')" in messages.
> So I added new boolean option "lf-to-crlf" to imap section.
> If this option enabled, git-imap-send converts LF to CRLF("\r\n").
>
> If you want to use it, add line:
> lf-to-crlf
> to [imap] section of your .gitconfig .
Hmm. Should this even be an option? Maybe we should _always_ do CRLF. That
does seem to be the technically correct thing to do for SMTP and IMAP.
rfc2822 (smtp) is pretty clear that CRLF is the line ending, and neither
CR nor LF must ever be sent individually. That's true both for headers and
the body of the email. The same goes for rfc3501 (imap).
So I suspect that CRLF should be unconditional.
Linus
^ 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