* Re: [PATCH] winansi: support ESC [ K (erase in line)
From: Johannes Schindelin @ 2009-03-10 11:31 UTC (permalink / raw)
To: Johannes Sixt; +Cc: git, gitster, Peter Harris, Sebastian Schuberth
In-Reply-To: <49B61377.90103@viscovery.net>
Hi,
On Tue, 10 Mar 2009, Johannes Sixt wrote:
> Johannes Schindelin schrieb:
> > This fixes the last bit of msysGit issue 124 for me:
> >
> > http://code.google.com/p/msysgit/issues/detail?id=124
> >
> > which annoyed me one time to many today.
> >
> > I had an earlier version which was smaller, but pretty hacky, in
> > that it checked if fprintf is #define'd in xwrite(), and had
> > special handling for that case.
> >
> > This patch is only slightly hacky, in that it assumes that you do
> > not try to output something that ends in an incomplete ESC [
> > sequence.
>
> Good that I read mail before I start hacking. I was about to do something
> about this in a moment. ;)
Heh...
> > To make use of it during a fetch, write() needs to be overridden, too.
>
> No, that's not necessary with the patch that I'm about to send in a
> moment. To replace write() for ANSI emulation really goes too far.
See my response to your patch...
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH/RFC] recv_sideband: Band #2 always goes to stderr
From: Johannes Schindelin @ 2009-03-10 11:39 UTC (permalink / raw)
To: Johannes Sixt
Cc: git, gitster, Peter Harris, Sebastian Schuberth, Nicolas Pitre
In-Reply-To: <49B64C3A.50909@viscovery.net>
Hi,
FWIW GitTorrent may be implemented as part of git-daemon, if Sam's ideas
become reality. And then, sideband transport is _the_ means to do
asyncrounous communication while pushing bytes.
On Tue, 10 Mar 2009, Johannes Sixt wrote:
> Johannes Sixt schrieb:
> > All data producers and data consumers *in git* use band #2 to
> > transport error messages and progress report. GitTorrent cannot not
> > talk to upload-pack or upload-archive and expect to get arbitrary
> > binary data over band #2.
> >
> > For use-cases that you have in mind in GitTorrent, the *protocol* may
> > be a good choice, but the current implementation is definitely a
> > special case.
>
> And it really is: Did you notice that stuff that recv_sideband sends over
> the channel named 'err' (before my patch) has "remote: " prepended on
> every line? That's certainly not an implementation that you want if you
> send binary data over that band!
Yes, that is unfortunate, but can be fixed easily.
However, I disagree to "fix" something that is working, even if it might
be more complicated than currently necessary.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH 1/2] grep Added --blame so that grep can show result tagged with blame entries
From: Matthieu Moy @ 2009-03-10 11:46 UTC (permalink / raw)
To: pi.songs; +Cc: gitster, git, rene.scharfe
In-Reply-To: <1b29507a0903100429v5b4a66d7sa9e3b32010fae2e0@mail.gmail.com>
pi song <pi.songs@gmail.com> writes:
> OK, I'll move my work to "next" branch. BTW, can anyone tell me what
> the "next" and "pu" branch are for ?
git/Documentation/gitworkflows.txt
In short : next = things that _should_ enter master in the future.
pu = proposed updates = things that _may_ enter master.
--
Matthieu
^ permalink raw reply
* Re: [PATCH/RFC] recv_sideband: Band #2 always goes to stderr
From: Johannes Sixt @ 2009-03-10 12:14 UTC (permalink / raw)
To: Johannes Schindelin
Cc: git, gitster, Peter Harris, Sebastian Schuberth, Nicolas Pitre
In-Reply-To: <alpine.DEB.1.00.0903101236480.14295@intel-tinevez-2-302>
Johannes Schindelin schrieb:
> FWIW GitTorrent may be implemented as part of git-daemon, if Sam's ideas
> become reality. And then, sideband transport is _the_ means to do
> asyncrounous communication while pushing bytes.
I do not see how recv_sideband() in its current form could be helpful here
(assuming that you really are thinking of sending binary data over band #2).
> On Tue, 10 Mar 2009, Johannes Sixt wrote:
>> Johannes Sixt schrieb:
>>> For use-cases that you have in mind in GitTorrent, the *protocol* may
>>> be a good choice, but the current implementation is definitely a
>>> special case.
>> And it really is: Did you notice that stuff that recv_sideband sends over
>> the channel named 'err' (before my patch) has "remote: " prepended on
>> every line? That's certainly not an implementation that you want if you
>> send binary data over that band!
>
> Yes, that is unfortunate, but can be fixed easily.
I don't believe this. Every treatment of "remote: " that you take away
from recv_sideband() you must insert somewhere else. Perhaps easy, but
certainly not as trivial as my patch.
Just a reminder: You proposed to override write() on Windows in a
non-trivial way, and we are discussing the topic above because I think
that is not a good idea. The reasons are:
- write() is a fundamental operation, and we should not mess with it out
of caution.
- Your proposal is not a catch-all. For example, combine-diff.c uses
puts() in dump_quoted_path(). If your goal was to not touch code outside
of compat/ then you need to override at least puts(), too.
- All code that writes ANSI escapes should use fprintf() anyway.
(Currently that is not the case, but all cases I'm aware of can be fixed
trivially.)
-- Hannes
^ permalink raw reply
* Re: [PATCH] winansi: support ESC [ K (erase in line)
From: Peter Harris @ 2009-03-10 12:29 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, gitster, Johannes Sixt, Sebastian Schuberth
In-Reply-To: <e2b19f6c7c50e5b0a652c40b0d8e4947134ed669.1236639280u.git.johannes.schindelin@gmx.de>
On Mon, Mar 9, 2009 at 8:41 PM, Johannes Schindelin wrote:
> To make use of it during a fetch, write() needs to be overridden, too.
Thanks for looking at this.
Sorry for leaving this part TODO for so long. Your version is
considerably less hacky than the version I had in my head, too.
> +int winansi_write(int fd, const void *buf, size_t len)
> +{
> + if (isatty(fd)) {
> + init();
> + if (console)
> + return ansi_emulate((const char *)buf, len,
> + fd == 2 ? stderr : stdout);
> + }
> + return write(fd, buf, len);
> +}
Switching an unbuffered write to a buffered fwrite makes me a little
nervous. In practice, all writes probably go through this function, so
it doesn't matter. And if the write is going somewhere it matters, it
likely fails isatty anyway. But I would still be less nervous with an
fflush() after ansi_emulate.
Peter Harris
^ permalink raw reply
* Re: Google Summer of Code 2009 - Organization Application
From: Jakub Narebski @ 2009-03-10 12:42 UTC (permalink / raw)
To: Shawn O. Pearce; +Cc: git
In-Reply-To: <20090310001613.GL11989@spearce.org>
"Shawn O. Pearce" <spearce@spearce.org> writes:
> Folks, its that time of year again. We need to apply to be a
> mentoring organization for Google Summer of Code 2009.
>
> Deadline is this Friday around noon PST.
>
> I have updated the application document on the wiki with the
> current questions and more up-to-date answers:
>
> http://git.or.cz/gitwiki/SoC2009Application
>
> Please make any comments in the next day or so, so that we can
> address them and have time to upload the completed application
> before their cut-off deadline.
Thank you very much for your efforts wrt Google Summers of Code.
About SoC2009Application: why it refers to SoC2008Template?
--
Jakub Narebski
Poland
ShadeHawk on #git
^ permalink raw reply
* Re: [PATCH/RFC] recv_sideband: Band #2 always goes to stderr
From: Johannes Schindelin @ 2009-03-10 12:52 UTC (permalink / raw)
To: Johannes Sixt
Cc: git, gitster, Peter Harris, Sebastian Schuberth, Nicolas Pitre
In-Reply-To: <49B659B4.5000705@viscovery.net>
Hi,
On Tue, 10 Mar 2009, Johannes Sixt wrote:
> Johannes Schindelin schrieb:
> > FWIW GitTorrent may be implemented as part of git-daemon, if Sam's
> > ideas become reality. And then, sideband transport is _the_ means to
> > do asyncrounous communication while pushing bytes.
>
> I do not see how recv_sideband() in its current form could be helpful
> here (assuming that you really are thinking of sending binary data over
> band #2).
I think it is a safe bet that the side band would be a good way to
exchange updates to the mirror list as well as the refs list.
> > On Tue, 10 Mar 2009, Johannes Sixt wrote:
> >> Johannes Sixt schrieb:
> >>> For use-cases that you have in mind in GitTorrent, the *protocol*
> >>> may be a good choice, but the current implementation is definitely a
> >>> special case.
> >>
> >> And it really is: Did you notice that stuff that recv_sideband sends
> >> over the channel named 'err' (before my patch) has "remote: "
> >> prepended on every line? That's certainly not an implementation that
> >> you want if you send binary data over that band!
> >
> > Yes, that is unfortunate, but can be fixed easily.
>
> I don't believe this. Every treatment of "remote: " that you take away
> from recv_sideband() you must insert somewhere else. Perhaps easy, but
> certainly not as trivial as my patch.
AFAICT it would be a matter of
unsigned pf = isatty(err) ? strlen(PREFIX) : 0;
> Just a reminder: You proposed to override write() on Windows in a
> non-trivial way, and we are discussing the topic above because I think
> that is not a good idea. The reasons are:
>
> - write() is a fundamental operation, and we should not mess with it out
> of caution.
But we do not mess with it! We ask explicitely if we are talking about a
tty.
> - Your proposal is not a catch-all. For example, combine-diff.c uses
> puts() in dump_quoted_path(). If your goal was to not touch code
> outside of compat/ then you need to override at least puts(), too.
>From compat/mingw.h:
-- snip --
/*
* ANSI emulation wrappers
*/
int winansi_fputs(const char *str, FILE *stream);
[...]
#define fputs winansi_fputs
-- snap --
... added in c09df8a, SOBbed by yourself ;-)
> - All code that writes ANSI escapes should use fprintf() anyway.
> (Currently that is not the case, but all cases I'm aware of can be
> fixed trivially.)
I disagree that all ANSI escapes have to go through fprintf(). Sometimes
you have a buffer, and I do not like doing extra work with %.*s there.
BTW I hope that you are not annoyed by the discussion; I think it is
necessary and important. I am certainly not married to my current POV; so
far, I am still in favor of it, though.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH] winansi: support ESC [ K (erase in line)
From: Johannes Schindelin @ 2009-03-10 12:54 UTC (permalink / raw)
To: Peter Harris; +Cc: git, gitster, Johannes Sixt, Sebastian Schuberth
In-Reply-To: <eaa105840903100529lc29664hd6e6fde29e06844a@mail.gmail.com>
[-- Attachment #1: Type: TEXT/PLAIN, Size: 1068 bytes --]
Hi,
On Tue, 10 Mar 2009, Peter Harris wrote:
> On Mon, Mar 9, 2009 at 8:41 PM, Johannes Schindelin wrote:
>
> > +int winansi_write(int fd, const void *buf, size_t len)
> > +{
> > + if (isatty(fd)) {
> > + init();
> > + if (console)
> > + return ansi_emulate((const char *)buf, len,
> > + fd == 2 ? stderr : stdout);
> > + }
> > + return write(fd, buf, len);
> > +}
>
> Switching an unbuffered write to a buffered fwrite makes me a little
> nervous. In practice, all writes probably go through this function, so
> it doesn't matter. And if the write is going somewhere it matters, it
> likely fails isatty anyway. But I would still be less nervous with an
> fflush() after ansi_emulate.
That would make the code substantially more messy, though.
Besides, half of it is already there: when encountering an ANSI sequence,
your code already fflush()es the fd before setting the new attributes.
And stderr does not need the fflush() anyway.
Ciao,
Dscho
^ permalink raw reply
* [PATCH] builtin-tag.c: remove global variable to use the callback data of git-config.
From: Carlos Rica @ 2009-03-10 13:03 UTC (permalink / raw)
To: git, johannes.schindelin, gitster
Signed-off-by: Carlos Rica <jasampler@yahoo.es>
---
This way the data flow is much clearer.
Here I declare a struct to wrap the new local array along with its size.
An alternative to this is strbuf, would it be preferable?
builtin-tag.c | 43 ++++++++++++++++++++++++++-----------------
1 files changed, 26 insertions(+), 17 deletions(-)
diff --git a/builtin-tag.c b/builtin-tag.c
index 01e7374..2b2d728 100644
--- a/builtin-tag.c
+++ b/builtin-tag.c
@@ -21,8 +21,6 @@ static const char * const git_tag_usage[] = {
NULL
};
-static char signingkey[1000];
-
struct tag_filter {
const char *pattern;
int lines;
@@ -156,7 +154,12 @@ static int verify_tag(const char *name, const char
*ref,
return 0;
}
-static int do_sign(struct strbuf *buffer)
+struct char_array {
+ char *buf;
+ size_t size;
+};
+
+static int do_sign(struct char_array *signingkey, struct strbuf
*buffer)
{
struct child_process gpg;
const char *args[4];
@@ -164,11 +167,12 @@ static int do_sign(struct strbuf *buffer)
int len;
int i, j;
- if (!*signingkey) {
- if (strlcpy(signingkey, git_committer_info(IDENT_ERROR_ON_NO_NAME),
- sizeof(signingkey)) > sizeof(signingkey) - 1)
+ if (!signingkey->buf[0]) {
+ if (strlcpy(signingkey->buf,
+ git_committer_info(IDENT_ERROR_ON_NO_NAME),
+ signingkey->size) > signingkey->size - 1)
return error("committer info too long.");
- bracket = strchr(signingkey, '>');
+ bracket = strchr(signingkey->buf, '>');
if (bracket)
bracket[1] = '\0';
}
@@ -183,7 +187,7 @@ static int do_sign(struct strbuf *buffer)
gpg.out = -1;
args[0] = "gpg";
args[1] = "-bsau";
- args[2] = signingkey;
+ args[2] = signingkey->buf;
args[3] = NULL;
if (start_command(&gpg))
@@ -220,9 +224,10 @@ static const char tag_template[] =
"# Write a tag message\n"
"#\n";
-static void set_signingkey(const char *value)
+static void set_signingkey(struct char_array *signingkey, const char
*value)
{
- if (strlcpy(signingkey, value, sizeof(signingkey)) >=
sizeof(signingkey))
+ if (strlcpy(signingkey->buf, value, signingkey->size)
+ >= signingkey->size)
die("signing key value too long (%.10s...)", value);
}
@@ -231,7 +236,7 @@ static int git_tag_config(const char *var, const
char *value, void *cb)
if (!strcmp(var, "user.signingkey")) {
if (!value)
return config_error_nonbool(var);
- set_signingkey(value);
+ set_signingkey((struct char_array *) cb, value);
return 0;
}
@@ -266,9 +271,10 @@ static void write_tag_body(int fd, const unsigned
char *sha1)
free(buf);
}
-static int build_tag_object(struct strbuf *buf, int sign, unsigned char
*result)
+static int build_tag_object(struct strbuf *buf, int sign,
+ struct char_array *signingkey, unsigned char *result)
{
- if (sign && do_sign(buf) < 0)
+ if (sign && do_sign(signingkey, buf) < 0)
return error("unable to sign the tag");
if (write_sha1_file(buf->buf, buf->len, tag_type, result) < 0)
return error("unable to write tag file");
@@ -277,6 +283,7 @@ static int build_tag_object(struct strbuf *buf, int
sign, unsigned char *result)
static void create_tag(const unsigned char *object, const char *tag,
struct strbuf *buf, int message, int sign,
+ struct char_array *signingkey,
unsigned char *prev, unsigned char *result)
{
enum object_type type;
@@ -331,7 +338,7 @@ static void create_tag(const unsigned char *object,
const char *tag,
strbuf_insert(buf, 0, header_buf, header_len);
- if (build_tag_object(buf, sign, result) < 0) {
+ if (build_tag_object(buf, sign, signingkey, result) < 0) {
if (path)
fprintf(stderr, "The tag message has been left in %s\n",
path);
@@ -374,6 +381,8 @@ int cmd_tag(int argc, const char **argv, const char
*prefix)
const char *msgfile = NULL, *keyid = NULL;
struct msg_arg msg = { 0, STRBUF_INIT };
struct commit_list *with_commit = NULL;
+ char keyarr[1000] = {'\0'};
+ struct char_array signingkey = { keyarr, sizeof(keyarr) };
struct option options[] = {
OPT_BOOLEAN('l', NULL, &list, "list tag names"),
{ OPTION_INTEGER, 'n', NULL, &lines, NULL,
@@ -403,14 +412,14 @@ int cmd_tag(int argc, const char **argv, const
char *prefix)
OPT_END()
};
- git_config(git_tag_config, NULL);
+ git_config(git_tag_config, &signingkey);
argc = parse_options(argc, argv, options, git_tag_usage, 0);
msgfile = parse_options_fix_filename(prefix, msgfile);
if (keyid) {
sign = 1;
- set_signingkey(keyid);
+ set_signingkey(&signingkey, keyid);
}
if (sign)
annotate = 1;
@@ -474,7 +483,7 @@ int cmd_tag(int argc, const char **argv, const char
*prefix)
if (annotate)
create_tag(object, tag, &buf, msg.given || msgfile,
- sign, prev, object);
+ sign, &signingkey, prev, object);
lock = lock_any_ref_for_update(ref, prev, 0);
if (!lock)
--
1.5.4.3
^ permalink raw reply related
* Re: [PATCH] builtin-tag.c: remove global variable to use the callback data of git-config.
From: Johannes Schindelin @ 2009-03-10 13:34 UTC (permalink / raw)
To: Carlos Rica; +Cc: git, gitster
In-Reply-To: <1236690219.20402.28.camel@luis-desktop>
Hi,
On Tue, 10 Mar 2009, Carlos Rica wrote:
>
> Signed-off-by: Carlos Rica <jasampler@yahoo.es>
Good to see you again!
BTW do you want to be recorded with a different email address in the
author line than in the S-O-B?
> -static int do_sign(struct strbuf *buffer)
> +struct char_array {
> + char *buf;
> + size_t size;
> +};
That looks very much like you want a struct strbuf, no?
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH] builtin-tag.c: remove global variable to use the callback data of git-config.
From: Carlos Rica @ 2009-03-10 14:00 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, gitster
In-Reply-To: <alpine.DEB.1.00.0903101432010.14295@intel-tinevez-2-302>
Please, don't even try to apply this patch, since long lines are wrapped.
I will send it fixed, sorry by the inconvenience.
On Tue, Mar 10, 2009 at 2:34 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
> On Tue, 10 Mar 2009, Carlos Rica wrote:
>
>>
>> Signed-off-by: Carlos Rica <jasampler@yahoo.es>
>
> Good to see you again!
Thanks!
>
> BTW do you want to be recorded with a different email address in the
> author line than in the S-O-B?
Thank you for notify it, Johannes! That's was another mistake here,
I wanted to use the gmail account for this, I will change it when I
send this fixed.
>> -static int do_sign(struct strbuf *buffer)
>> +struct char_array {
>> + char *buf;
>> + size_t size;
>> +};
>
> That looks very much like you want a struct strbuf, no?
I was asking exactly that, in the hidden lines of the patch,
so if everybody prefers the strbuf solution I will use it, and then,
we should choose if there must be a limit for the signing key id max length
(now 1000) since by using dynamic memory it would not be required.
> Ciao,
> Dscho
>
>
^ permalink raw reply
* Re: Google Summer of Code 2009 - Organization Application
From: Shawn O. Pearce @ 2009-03-10 14:24 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git
In-Reply-To: <m31vt5im3j.fsf@localhost.localdomain>
Jakub Narebski <jnareb@gmail.com> wrote:
> "Shawn O. Pearce" <spearce@spearce.org> writes:
>
> > http://git.or.cz/gitwiki/SoC2009Application
>
> About SoC2009Application: why it refers to SoC2008Template?
Because the template hasn't changed for 2009?
--
Shawn.
^ permalink raw reply
* Re: Git BoF @ EclipseCon
From: Shawn O. Pearce @ 2009-03-10 14:25 UTC (permalink / raw)
To: Manuel Woelker; +Cc: git
In-Reply-To: <3d045c7e0903100110i45fb1874s5e0dd3517e60695a@mail.gmail.com>
Manuel Woelker <manuel.woelker@gmail.com> wrote:
> Hi git list!
>
> Just a quick heads-up for all interested parties: There is a git BoF
> scheduled for EclipseCon 2009 (
> http://www.eclipsecon.org/2009/sessions?id=776 ). The main focus is
> going to be to discuss how we can make git (probably via jgit/egit) a
> viable option in the Eclipse ecosystem. A little more info here:
> http://eclipsesource.com/blogs/2009/03/02/git-bof-eclipsecon/
>
> I think it would be great to have some of you experts present there
> and I hope we can make some progress. It also looks like there will be
> another VCS BoF on Tuesday, to discuss more general issues
> (IP, infrastructure, work flows, etc).
I'm on the panel for the VCS dicussion on Tuesday, and plan on
going to this Wednesday Git BoF.
--
Shawn.
^ permalink raw reply
* Re: [PATCH/RFC] recv_sideband: Band #2 always goes to stderr
From: Johannes Sixt @ 2009-03-10 14:26 UTC (permalink / raw)
To: Johannes Schindelin
Cc: git, gitster, Peter Harris, Sebastian Schuberth, Nicolas Pitre
In-Reply-To: <alpine.DEB.1.00.0903101343530.14295@intel-tinevez-2-302>
Johannes Schindelin schrieb:
> Hi,
>
> On Tue, 10 Mar 2009, Johannes Sixt wrote:
>
>> Johannes Schindelin schrieb:
>>> FWIW GitTorrent may be implemented as part of git-daemon, if Sam's
>>> ideas become reality. And then, sideband transport is _the_ means to
>>> do asyncrounous communication while pushing bytes.
>> I do not see how recv_sideband() in its current form could be helpful
>> here (assuming that you really are thinking of sending binary data over
>> band #2).
>
> I think it is a safe bet that the side band would be a good way to
> exchange updates to the mirror list as well as the refs list.
Binary or not - the purpose and suitability of the sideband *protocol* for
this task are undisputed.
But you don't want to have "remote:" thrown in at seemingly random places
in the demultiplexed stream that comes fromt he current implementation of
recv_sideband().
>>> On Tue, 10 Mar 2009, Johannes Sixt wrote:
>>>> And it really is: Did you notice that stuff that recv_sideband sends
>>>> over the channel named 'err' (before my patch) has "remote: "
>>>> prepended on every line? That's certainly not an implementation that
>>>> you want if you send binary data over that band!
>>> Yes, that is unfortunate, but can be fixed easily.
>> I don't believe this. Every treatment of "remote: " that you take away
>> from recv_sideband() you must insert somewhere else. Perhaps easy, but
>> certainly not as trivial as my patch.
>
> AFAICT it would be a matter of
>
> unsigned pf = isatty(err) ? strlen(PREFIX) : 0;
But don't you see that are mixing a high-level concept of "terminal" into
the low-level function that you want it to be? In its current form,
recv_sideband() is *not* a low-level utility, it's already at a high level
that knows about the line-oriented nature of band #2. What you need for
GitTorrent is a different function that *only* demultiplexes the sideband
protocol data into different streams without munging them. That's a
totally different function that *maybe* can share some code with the
current recv_sideband().
>> Just a reminder: You proposed to override write() on Windows in a
>> non-trivial way, and we are discussing the topic above because I think
>> that is not a good idea. The reasons are:
>>
>> - write() is a fundamental operation, and we should not mess with it out
>> of caution.
>
> But we do not mess with it! We ask explicitely if we are talking about a
> tty.
With reference to Peter's reply, I'm not the only one who gets nervous if
write() is replaced in a non-trivial way.
After all, you are sneaking the high-level concept "terminal emulation"
into the low-level write() function.
>> - Your proposal is not a catch-all. For example, combine-diff.c uses
>> puts() in dump_quoted_path(). If your goal was to not touch code
>> outside of compat/ then you need to override at least puts(), too.
>
>>From compat/mingw.h:
>
> -- snip --
> /*
> * ANSI emulation wrappers
> */
>
> int winansi_fputs(const char *str, FILE *stream);
> [...]
> #define fputs winansi_fputs
> -- snap --
>
> ... added in c09df8a, SOBbed by yourself ;-)
My point was that you cannot get away without modifying code outside of
compat/ (if that was your motivation to override write()). I don't care
whether we change this instance to fputs() or fprintf(). But we already
*have* something, and don't need *yet another* override.
>> - All code that writes ANSI escapes should use fprintf() anyway.
>> (Currently that is not the case, but all cases I'm aware of can be
>> fixed trivially.)
>
> I disagree that all ANSI escapes have to go through fprintf(). Sometimes
> you have a buffer, and I do not like doing extra work with %.*s there.
But on the other hand you risk breaking write() semantics and give us a
colorful mix of concepts.
I don't insist in that ANSI escapes must go through fprintf(), but they
should really not go through a level that is lower than stdio. Basic file
IO should really not be muddied with terminal emulation.
> BTW I hope that you are not annoyed by the discussion; I think it is
> necessary and important. I am certainly not married to my current POV; so
> far, I am still in favor of it, though.
I'm absolutely not annoyed. And I am as married to my POV as you are to
yours. ;) In this case we perhaps need a tie-breaker.
-- Hannes
^ permalink raw reply
* Re: [PATCH/RFC] recv_sideband: Band #2 always goes to stderr
From: Shawn O. Pearce @ 2009-03-10 14:38 UTC (permalink / raw)
To: Johannes Sixt
Cc: Johannes Schindelin, git, gitster, Peter Harris,
Sebastian Schuberth, Nicolas Pitre
In-Reply-To: <49B6788F.2080609@viscovery.net>
Johannes Sixt <j.sixt@viscovery.net> wrote:
> But don't you see that are mixing a high-level concept of "terminal" into
> the low-level function that you want it to be? In its current form,
> recv_sideband() is *not* a low-level utility, it's already at a high level
> that knows about the line-oriented nature of band #2. What you need for
> GitTorrent is a different function that *only* demultiplexes the sideband
> protocol data into different streams without munging them. That's a
> totally different function that *maybe* can share some code with the
> current recv_sideband().
ACK.
The definition of the streams in the current sideband protocol
are rather well defined for the one protocol that uses it,
fetch-pack/receive-pack:
stream #1: pack data
stream #2: stderr messages, progress, meant for tty
stream #3: oh-sh*t abort message, remote is dead, goodbye!
The stream number is encoded as a byte. Anyone trying to reuse the
sideband protocol within the fetch-pack/receive-pack protocol to
carry *extra* data should use new channel numbers. We have another
252 remaining. I don't think we're lacking on description space.
> With reference to Peter's reply, I'm not the only one who gets nervous if
> write() is replaced in a non-trivial way.
>
> After all, you are sneaking the high-level concept "terminal emulation"
> into the low-level write() function.
Oh god. Please do not do this. I usually ignore the Win32 port
stuff. But I agree with you Hannes, please do not do this.
> I'm absolutely not annoyed. And I am as married to my POV as you are to
> yours. ;) In this case we perhaps need a tie-breaker.
I cast my vote in with you Hannes. Your original RFC patch made
sense as a cleanup in the code.
I agree with you that the way the code is currently structured
and used, sideband #2 should always dump to the stderr channel
of the process. We also assume in a lot of other places that
fprintf(stderr, ...) is a good way to report progress to the
end-user. This is no different, its just progress data from the
remote system rather than the local system.
Huge refactorings would be necessary to split sideband #2 into
something other than stderr. You would also want to replace nearly
all references to stderr. We're not going down that road.
FWIW, JGit takes the meanings of the streams as I described them
above. Stream #1 goes to the pack processing, stream #2 gets parsed
and converted into updates calls to our ProgressMonitor API (which in
turn goes to GUI progress meters in the host IDE), stream #3, if it
ever shows up, turns into the message of an exception being thrown
up the stack. That is the definition of this particular protocol.
Accept it, and move it. :-)
The sideband protocol is fairly simple. If we ever needed to use
it for other data, we could probably refactor some of the header
formatting/parsing out a bit and create a more generalized split
function, under a different name. But that's all in the future
and something we just don't have an immediate need to worry about.
So don't worry about it.
--
Shawn.
^ permalink raw reply
* BUG: git can't handle certain kinds of renames in merges
From: Caleb Cushing @ 2009-03-10 14:45 UTC (permalink / raw)
To: git
git actually sucks at some merges 101 :P. I'm going to have to play
with my test repository further because eventually I should be able to
get git-write-tree failures making any kind of 'merge' impossible.
git://github.com/xenoterracide/git-test-case.git
clone that. checkout branch 1. then git merge master to see my end failure.
steps to create
add a file in a branch with a line (or more).
create a new branch based on this branch and check it out.
in the new branch mv the file into a directory with the same name as
the file was. add -u and add the file so git sees the rename.
checkout the original branch add some lines. checkout the new branch
merge. the merge will go fine.
remove a line from the new branches file.
checkout master. add another line to that file.
checkout new branch and attempt to merge.
you should now see the point that may test case is at.
--
Caleb Cushing
http://xenoterracide.blogspot.com
^ permalink raw reply
* Re: [PATCH/RFC] recv_sideband: Band #2 always goes to stderr
From: Johannes Schindelin @ 2009-03-10 14:46 UTC (permalink / raw)
To: Shawn O. Pearce
Cc: Johannes Sixt, git, gitster, Peter Harris, Sebastian Schuberth,
Nicolas Pitre
In-Reply-To: <20090310143851.GP11989@spearce.org>
Hi,
On Tue, 10 Mar 2009, Shawn O. Pearce wrote:
> Johannes Sixt <j.sixt@viscovery.net> wrote:
> > But don't you see that are mixing a high-level concept of "terminal"
> > into the low-level function that you want it to be? In its current
> > form, recv_sideband() is *not* a low-level utility, it's already at a
> > high level that knows about the line-oriented nature of band #2. What
> > you need for GitTorrent is a different function that *only*
> > demultiplexes the sideband protocol data into different streams
> > without munging them. That's a totally different function that *maybe*
> > can share some code with the current recv_sideband().
>
> ACK.
>
> The definition of the streams in the current sideband protocol are
> rather well defined for the one protocol that uses it,
> fetch-pack/receive-pack:
>
> stream #1: pack data
> stream #2: stderr messages, progress, meant for tty
> stream #3: oh-sh*t abort message, remote is dead, goodbye!
>
> The stream number is encoded as a byte. Anyone trying to reuse the
> sideband protocol within the fetch-pack/receive-pack protocol to carry
> *extra* data should use new channel numbers. We have another 252
> remaining. I don't think we're lacking on description space.
Fair enough, the tie-breaker hath spoken.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH/RFC] recv_sideband: Band #2 always goes to stderr
From: Shawn O. Pearce @ 2009-03-10 14:46 UTC (permalink / raw)
To: Johannes Sixt
Cc: Johannes Schindelin, git, gitster, Peter Harris,
Sebastian Schuberth, Nicolas Pitre
In-Reply-To: <49B61703.8030602@viscovery.net>
Johannes Sixt <j.sixt@viscovery.net> wrote:
> From: Johannes Sixt <j6t@kdbg.org>
>
> This removes the last parameter of recv_sideband, by which the callers
> told which channel band #2 data should be written to. Since both callers
> of the function passed 2 for the parameter, we hereby remove the
> parameter and send band #2 to stderr explicitly using fprintf.
>
> This has the nice side-effect that the band #2 data (most importantly
> progress reports during a fetch operation) passes through our ANSI
> emulation layer on Windows.
>
> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
Looks right to me.
> diff --git a/sideband.c b/sideband.c
> index cca3360..a706ac8 100644
> --- a/sideband.c
> +++ b/sideband.c
> @@ -50,8 +49,8 @@ int recv_sideband(const char *me, int in_stream, int out, int err)
> switch (band) {
> case 3:
> buf[pf] = ' ';
> - buf[pf+1+len] = '\n';
> - safe_write(err, buf, pf+1+len+1);
> + buf[pf+1+len] = '\0';
> + fprintf(stderr, "%s\n", buf);
Can't you instead do:
fprintf(stderr, "%.*s\n", buf, pf + len);
like you do...
> @@ -95,12 +94,13 @@ int recv_sideband(const char *me, int in_stream, int out, int err)
> memcpy(save, b + brk, sf);
> b[brk + sf - 1] = b[brk - 1];
> memcpy(b + brk - 1, suffix, sf);
> - safe_write(err, b, brk + sf);
> + fprintf(stderr, "%.*s", brk + sf, b);
> memcpy(b + brk, save, sf);
> len -= brk;
> } else {
> int l = brk ? brk : len;
> - safe_write(err, b, l);
> + if (l > 0)
> + fprintf(stderr, "%.*s", l, b);
here?
--
Shawn.
^ permalink raw reply
* Re: [PATCH/RFC] recv_sideband: Band #2 always goes to stderr
From: Johannes Sixt @ 2009-03-10 15:02 UTC (permalink / raw)
To: Shawn O. Pearce
Cc: Johannes Schindelin, git, gitster, Peter Harris,
Sebastian Schuberth, Nicolas Pitre
In-Reply-To: <20090310144646.GQ11989@spearce.org>
Shawn O. Pearce schrieb:
> Johannes Sixt <j.sixt@viscovery.net> wrote:
>> diff --git a/sideband.c b/sideband.c
>> index cca3360..a706ac8 100644
>> --- a/sideband.c
>> +++ b/sideband.c
>> @@ -50,8 +49,8 @@ int recv_sideband(const char *me, int in_stream, int out, int err)
>> switch (band) {
>> case 3:
>> buf[pf] = ' ';
>> - buf[pf+1+len] = '\n';
>> - safe_write(err, buf, pf+1+len+1);
>> + buf[pf+1+len] = '\0';
>> + fprintf(stderr, "%s\n", buf);
>
> Can't you instead do:
>
> fprintf(stderr, "%.*s\n", buf, pf + len);
>
> like you do...
>
>> @@ -95,12 +94,13 @@ int recv_sideband(const char *me, int in_stream, int out, int err)
>> memcpy(save, b + brk, sf);
>> b[brk + sf - 1] = b[brk - 1];
>> memcpy(b + brk - 1, suffix, sf);
>> - safe_write(err, b, brk + sf);
>> + fprintf(stderr, "%.*s", brk + sf, b);
>> memcpy(b + brk, save, sf);
>> len -= brk;
>> } else {
>> int l = brk ? brk : len;
>> - safe_write(err, b, l);
>> + if (l > 0)
>> + fprintf(stderr, "%.*s", l, b);
>
> here?
I deliberatly avoided "%.*s" in the former hunk (1) because of the posts
that we had yesterday about potentially misbehaved fprintf in the case
where the precision is 0; and (2) because it was so easy to avoid it. I
don't think we need ultimate performance in this case, and I also consider
the plain "%s\n" more readable.
That said, the second hunk is really only the minimal change and I'd like
to rewrite it to get rid of the memcpy stuff. It is really not needed once
fprintf is in the game. But that's a separate patch.
-- Hannes
^ permalink raw reply
* [PATCH] Typo and language fixes for git-checkout.txt
From: Michael J Gruber @ 2009-03-10 15:06 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
Documentation/git-checkout.txt | 33 ++++++++++++++++-----------------
1 files changed, 16 insertions(+), 17 deletions(-)
A few fixes for typos and speakos. I tried to be lenient, not enforcing
a different writing style esp. w.r.t. punctuation.
In the hunk about --track guess work without -b I removed the
unfortunate term "remote system" which isn't used anywhere else (and is
incorrect).
diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
index 3bccffa..125d8f3 100644
--- a/Documentation/git-checkout.txt
+++ b/Documentation/git-checkout.txt
@@ -8,7 +8,7 @@ git-checkout - Checkout a branch or paths to the working tree
SYNOPSIS
--------
[verse]
-'git checkout' [-q] [-f] [--track | --no-track] [-b <new_branch> [-l]] [-m] [<branch>]
+'git checkout' [-q] [-f] [-t | --track | --no-track] [-b <new_branch> [-l]] [-m] [<branch>]
'git checkout' [-f|--ours|--theirs|-m|--conflict=<style>] [<tree-ish>] [--] <paths>...
DESCRIPTION
@@ -21,15 +21,15 @@ specified, <new_branch>. Using -b will cause <new_branch> to
be created; in this case you can use the --track or --no-track
options, which will be passed to `git branch`.
-As a convenience, --track will default to create a branch whose
+As a convenience, --track will default to creating a branch whose
name is constructed from the specified branch name by stripping
the first namespace level.
When <paths> are given, this command does *not* switch
branches. It updates the named paths in the working tree from
the index file, or from a named <tree-ish> (most often a commit). In
-this case, the `-b` options is meaningless and giving
-either of them results in an error. <tree-ish> argument can be
+this case, the `-b` and `--track` options are meaningless and giving
+either of them results in an error. The <tree-ish> argument can be
used to specify a specific tree-ish (i.e. commit, tag or tree)
to update the index for the given paths before updating the
working tree.
@@ -75,14 +75,13 @@ entries; instead, unmerged entries are ignored.
<repository> <refspec>" explicitly. This behavior is the default
when the start point is a remote branch. Set the
branch.autosetupmerge configuration variable to `false` if you want
- 'git-checkout' and 'git-branch' to always behave as if '--no-track' were
+ 'git checkout' and 'git branch' to always behave as if '--no-track' were
given. Set it to `always` if you want this behavior when the
- start-point is either a local or remote branch.
+ start point is either a local or remote branch.
+
-If no '-b' option was given, the name of the new branch will be
-derived from the remote branch, by attempting to guess the name
-of the branch on remote system. If "remotes/" or "refs/remotes/"
-are prefixed, it is stripped away, and then the part up to the
+If no '-b' option is given, the name of the new branch will be
+derived from the remote branch. If "remotes/" or "refs/remotes/"
+is prefixed it is stripped away, and then the part up to the
next slash (which would be the nickname of the remote) is removed.
This would tell us to use "hack" as the local branch when branching
off of "origin/hack" (or "remotes/origin/hack", or even
@@ -152,12 +151,12 @@ $ git checkout v2.6.18
------------
Earlier versions of git did not allow this and asked you to
-create a temporary branch using `-b` option, but starting from
+create a temporary branch using the `-b` option, but starting from
version 1.5.0, the above command 'detaches' your HEAD from the
-current branch and directly point at the commit named by the tag
-(`v2.6.18` in the above example).
+current branch and directly points at the commit named by the tag
+(`v2.6.18` in the example above).
-You can use usual git commands while in this state. You can use
+You can use all git commands while in this state. You can use
`git reset --hard $othercommit` to further move around, for
example. You can make changes and create a new commit on top of
a detached HEAD. You can even create a merge by using `git
@@ -191,7 +190,7 @@ $ git checkout hello.c <3>
------------
+
<1> switch branch
-<2> take out a file out of other commit
+<2> take a file out of another commit
<3> restore hello.c from HEAD of current branch
+
If you have an unfortunate branch that is named `hello.c`, this
@@ -202,7 +201,7 @@ You should instead write:
$ git checkout -- hello.c
------------
-. After working in a wrong branch, switching to the correct
+. After working in the wrong branch, switching to the correct
branch would be done using:
+
------------
@@ -210,7 +209,7 @@ $ git checkout mytopic
------------
+
However, your "wrong" branch and correct "mytopic" branch may
-differ in files that you have locally modified, in which case,
+differ in files that you have modified locally, in which case
the above checkout would fail like this:
+
------------
--
1.6.2
^ permalink raw reply related
* Re: [PATCH/RFC] recv_sideband: Band #2 always goes to stderr
From: Johannes Schindelin @ 2009-03-10 15:07 UTC (permalink / raw)
To: Johannes Sixt
Cc: Shawn O. Pearce, git, gitster, Peter Harris, Sebastian Schuberth,
Nicolas Pitre
In-Reply-To: <49B680F7.4040103@viscovery.net>
Hi,
On Tue, 10 Mar 2009, Johannes Sixt wrote:
> Shawn O. Pearce schrieb:
> > Johannes Sixt <j.sixt@viscovery.net> wrote:
> >> diff --git a/sideband.c b/sideband.c
> >> index cca3360..a706ac8 100644
> >> --- a/sideband.c
> >> +++ b/sideband.c
> >> @@ -50,8 +49,8 @@ int recv_sideband(const char *me, int in_stream, int out, int err)
> >> switch (band) {
> >> case 3:
> >> buf[pf] = ' ';
> >> - buf[pf+1+len] = '\n';
> >> - safe_write(err, buf, pf+1+len+1);
> >> + buf[pf+1+len] = '\0';
> >> + fprintf(stderr, "%s\n", buf);
> >
> > Can't you instead do:
> >
> > fprintf(stderr, "%.*s\n", buf, pf + len);
> >
> > like you do...
> >
> >> @@ -95,12 +94,13 @@ int recv_sideband(const char *me, int in_stream, int out, int err)
> >> memcpy(save, b + brk, sf);
> >> b[brk + sf - 1] = b[brk - 1];
> >> memcpy(b + brk - 1, suffix, sf);
> >> - safe_write(err, b, brk + sf);
> >> + fprintf(stderr, "%.*s", brk + sf, b);
> >> memcpy(b + brk, save, sf);
> >> len -= brk;
> >> } else {
> >> int l = brk ? brk : len;
> >> - safe_write(err, b, l);
> >> + if (l > 0)
> >> + fprintf(stderr, "%.*s", l, b);
> >
> > here?
>
> I deliberatly avoided "%.*s" in the former hunk (1) because of the posts
> that we had yesterday about potentially misbehaved fprintf in the case
> where the precision is 0;
Didn't that turn out to be a false alarm?
> and (2) because it was so easy to avoid it. I don't think we need
> ultimate performance in this case, and I also consider the plain "%s\n"
> more readable.
>
> That said, the second hunk is really only the minimal change and I'd
> like to rewrite it to get rid of the memcpy stuff. It is really not
> needed once fprintf is in the game. But that's a separate patch.
I think, indeed, that you can avoid the memcpy() by using %.*s. The
private buffer is only used to make sure that the text is written in one
go anyway (i.e. that two sidebands messages are not written to the same
line because they use multiple calls to fprintf()/fwrite() per line),
right?
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH/RFC] recv_sideband: Band #2 always goes to stderr
From: Johannes Sixt @ 2009-03-10 15:14 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Shawn O. Pearce, git, gitster, Peter Harris, Sebastian Schuberth,
Nicolas Pitre
In-Reply-To: <alpine.DEB.1.00.0903101605460.14295@intel-tinevez-2-302>
Johannes Schindelin schrieb:
> On Tue, 10 Mar 2009, Johannes Sixt wrote:
>> I deliberatly avoided "%.*s" in the former hunk (1) because of the posts
>> that we had yesterday about potentially misbehaved fprintf in the case
>> where the precision is 0;
>
> Didn't that turn out to be a false alarm?
Even better. I didn't follow the thread very closely.
> I think, indeed, that you can avoid the memcpy() by using %.*s. The
> private buffer is only used to make sure that the text is written in one
> go anyway (i.e. that two sidebands messages are not written to the same
> line because they use multiple calls to fprintf()/fwrite() per line),
> right?
It must be something like that, yes. ;)
-- Hannes
^ permalink raw reply
* Re: [PATCH][v2] http authentication via prompts (with correct line lengths)
From: Mike Gaffney @ 2009-03-10 15:33 UTC (permalink / raw)
To: Johannes Schindelin, git
In-Reply-To: <alpine.DEB.1.00.0903101132360.14295@intel-tinevez-2-302>
Johannes,
Your points make sense, thank you for clarifying, it helps
me understand what the underlying concerns are. The attitude doesn't really
help.
Does Junio's counter solution where git would prompt for the
password if the username contained a url solve your concerns with unsecure
config variables? The patch would be just a bugfix to current functionality.
Also, libcurl does not warn you that you have an insecure netrc file.
Just tried it with 7.19.4 on cygwin and FC9.
Thanks for the feedback,
Mike
^ permalink raw reply
* Re: BUG: git can't handle certain kinds of renames in merges
From: Pieter de Bie @ 2009-03-10 16:02 UTC (permalink / raw)
To: Caleb Cushing; +Cc: git
In-Reply-To: <81bfc67a0903100745m3a425337h3f4f7cdbde6b5cfe@mail.gmail.com>
On Mar 10, 2009, at 2:45 PM, Caleb Cushing wrote:
> git://github.com/xenoterracide/git-test-case.git
>
> clone that. checkout branch 1. then git merge master to see my end
> failure.
>
> steps to create
>
> add a file in a branch with a line (or more).
> create a new branch based on this branch and check it out.
> in the new branch mv the file into a directory with the same name as
> the file was. add -u and add the file so git sees the rename.
> checkout the original branch add some lines. checkout the new branch
> merge. the merge will go fine.
> remove a line from the new branches file.
> checkout master. add another line to that file.
> checkout new branch and attempt to merge.
>
> you should now see the point that may test case is at.
Yes, this is because automatic renaming detection fails
with this kind of toy examples. Git can't infer the file
was renamed because almost nothing is similar enough. Take
a look at the attached script and run it with 'sh test.sh'
and 'sh test.sh real_test', and look at the difference.
- Pieter
#!/usr/bin/bish
if test x$1 = x
then
EXTRA_LINES=""
else
EXTRA_LINES="line2\nline3\nline4\nline5"
fi
FILE1="a\n$EXTRA_LINES"
FILE2="a\n$EXTRA_LINES\nb"
FILE3="$EXTRA_LINES\nb"
FILE4="a\n$EXTRA_LINES\nb\nc"
echo -e $FILE2
rm -rf test_dir
mkdir test_dir
cd test_dir
git init
echo -e $FILE1 > file
git add file
git commit -am "Initial"
git checkout -b branch
git mv file a
mkdir file
git mv a file/file
git commit -m "Move"
git checkout master
echo -e $FILE2 > file
git commit -am "Add a line"
git checkout branch
git merge master
echo -e $FILE3 > file/file
git commit -am "Remove line"
git checkout master
echo -e $FILE4 > file
git commit -am "Add another line"
git checkout branch
git merge master
^ permalink raw reply
* Re: [RFC/PATCH] git push usability improvements and default change
From: Jay Soffian @ 2009-03-10 16:20 UTC (permalink / raw)
To: Finn Arne Gangstad; +Cc: Junio C Hamano, git
In-Reply-To: <20090310100400.GC11448@pvv.org>
On Tue, Mar 10, 2009 at 6:04 AM, Finn Arne Gangstad <finnag@pvv.org> wrote:
> You are thinking of something like this in .gitconfig?
> [remote "*"]
> push = __something__
>
> Previously you indicated that there is no way to specify the current
> matching rule in a remote push line I think?
No, you can. "push = :" works fine.
However, there's no way to specify "push = nothing" currently.
FWIW, I don't care for using remote.*.push for this use case. I think
push.default is clearer here.
j.
^ 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