Git development
 help / color / mirror / Atom feed
* A typo in RelNotes/1.8.2 (v1.8.0.1-347-gf94c325)
From: Horst H. von Brand @ 2012-11-30 13:15 UTC (permalink / raw)
  To: gitster; +Cc: git

-- 
Dr. Horst H. von Brand                   User #22616 counter.li.org
Departamento de Informatica                    Fono: +56 32 2654431
Universidad Tecnica Federico Santa Maria             +56 32 2654239
Casilla 110-V, Valparaiso, Chile 2340000       Fax:  +56 32 2797513

^ permalink raw reply

* Re: [PATCH 6/8] imap-send: change msg_data from storing (char *, len) to storing strbuf
From: Michael Haggerty @ 2012-11-30 13:36 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Jeremy White, Johannes Schindelin, Jeff King
In-Reply-To: <7vboegp04x.fsf@alter.siamese.dyndns.org>

On 11/29/2012 10:30 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> struct msg_data stored (char *, len) of the data to be included in a
> 
> That (<type>, <varname>) is a bit funny notation, even though it is
> understandable.

I understand that it is funny, but it seems like the clearest way to
express what is meant in a way that fits in the summary line.  Feel free
to change it if you like.

>> message, kept the character data NUL-terminated, etc., much like a
>> strbuf would do.  So change it to use a struct strbuf.  This makes the
>> code clearer and reduces copying a little bit.
>>
>> A side effect of this change is that the memory for each message is
>> freed after it is used rather than leaked, though that detail is
>> unimportant given that imap-send is a top-level command.
>>
>> --
> 
> ?

If by "?" you are wondering where the memory leak was, it was:

* The while loop in main() called split_msg()

  * split_msg() cleared the msg_data structure using
    memset(msg, 0, sizeof *msg)

  * split_msg() copied the first message out of all_msgs using
    xmemdupz() and stored the result to msg->data

* The msg_data was passed to imap_store_msg().  Its contents were
  copied to cb.data (which will be freed in the imap functions) but
  the original was left unfreed.

* The next time through the loop, split_msg() zeroed the msg_data
  structure again, thus discarding the pointer to the xmemdupz()ed
  memory.

The leak caused more memory than necessary to be allocated (worst case:
nearly the total size of all_msgs).  But (a) all_msgs is already stored
in memory, so the wastage is at most a factor of 2; and (b) this all
happens in main() shortly before program exit erases all sins.

I didn't bother documenting this in the commit message because the patch
changes the code anyway, but feel free to add the above explanation to
the commit message if you think it is useful.

>> For some reason, there is a bunch of infrastructure in this file for
>> dealing with IMAP flags, although there is nothing in the code that
>> actually allows any flags to be set.  If there is no plan to add
>> support for flags in the future, a bunch of code could be ripped out
>> and "struct msg_data" could be completely replaced with strbuf.
> 
> Yeah, after all these years we have kept the unused flags field
> there and nobody needed anything out of it.  I am OK with a removal
> if it is done at the very end of the series.

I don't think the removal of flags needs to be part of the same series.
 I suggest a separate patch series dedicated to deleting *all* the extra
imap infrastructure at once.  That being said, I'm not committing to do
so.  (We could add it to an "straightforward projects for aspiring git
developers" list, if we had such a thing.)

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

^ permalink raw reply

* Re: [PATCH 8/8] wrap_in_html(): process message in bulk rather than line-by-line
From: Michael Haggerty @ 2012-11-30 13:40 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Jeremy White, Johannes Schindelin, Jeff King
In-Reply-To: <7v7gp4p00u.fsf@alter.siamese.dyndns.org>

[-- Attachment #1: Type: text/plain, Size: 1456 bytes --]

On 11/29/2012 10:33 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> Now that we can xml-quote an arbitrary string in O(N), there is no
>> reason to process the message line by line.  This change saves lots of
>> memory allocations and copying.
>>
>> The old code would have created invalid output for a malformed input
>> message (one that does not contain a blank line separating the header
>> from the body).  The new code die()s in this situation.
> 
> Given that imap-send is about sending a patch the distinction would
> not matter in practice, but isn't the difference between the two
> that the new version would not allow sending a header-only message
> without a body, while the old one allowed it?

I was thinking that the end-of-header line is a required part of an
RFC2282 email message, but I was wrong.  If you squash the attached
patch onto this commit, it will handle emails without bodies correctly.

Nevertheless, the old code was even *more* broken because it added a
"</pre>" regardless of whether the separator line had been seen, and
therefore a message without an end-of-header line would come out like

    Header1: foo
    Header2: bar
    </pre>

with no content_type line, no pre_open, and </pre> appended to the
header without a blank line in between.  This is the "invalid output"
that I was referring to.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

[-- Attachment #2: addstr-xml-quoted-addendum.diff --]
[-- Type: text/x-patch, Size: 337 bytes --]

diff --git a/imap-send.c b/imap-send.c
index eec9e35..e521e2f 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -1348,7 +1348,7 @@ static void wrap_in_html(struct strbuf *msg)
 	const char *body = strstr(msg->buf, "\n\n");
 
 	if (!body)
-		die("malformed message");
+		return; /* Headers but no body; no wrapping needed */
 
 	body += 2;
 

^ permalink raw reply related

* Re: [Query] Can we ignore case for commiters name in shortlog?
From: Viresh Kumar @ 2012-11-30 15:05 UTC (permalink / raw)
  To: Max Horn; +Cc: Nicolas Pitre, Junio C Hamano, git
In-Reply-To: <C9C7BAF7-D552-41CF-B45D-413B9B69C054@quendi.de>

On 30 November 2012 16:49, Max Horn <max@quendi.de> wrote:
> I don't see how wrong case is different from any other form of misspelling. And mailmap is there precisely to handle such problems. Now, if these case issues were for some reasons very frequent, it might be worth adding dedicated support for it. But this seems dubious to me -- do you have any evidence for this? Indeed, do you have more than just the one example?

I don't have another example, but i have seen it many times. This happens
when people use different repo's to send patches and by mistake have mentioned
names in different case in them in their local .git/config files.

--
viresh

^ permalink raw reply

* Re: Operations on unborn branch
From: Martin von Zweigbergk @ 2012-11-30 16:39 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <7vd2yyi4l1.fsf@alter.siamese.dyndns.org>

On Tue, Nov 27, 2012 at 11:12 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> You have to special case the edges whichever way you go.  [...]

If I understand you correctly, you're saying that revision walking
would need a different special case. This is the most obvious
difference, it seems. "git show" would also need different
special-casing. But rebase wouldn't need --root, diff-tree wouldn't
need --root, any operations on an unborn branch would just work (incl
reset, cherry-pick).

Again, this is hypothetical, so I'll stop the complaining now :-)

^ permalink raw reply

* Re: [RFC/PATCH 1/2] reset: learn to reset to tree
From: Martin von Zweigbergk @ 2012-11-30 16:45 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <CANiSa6hWYsfm0t+s_q7=CcD78yNfpuduxkRc35xW8qDOy97W3g@mail.gmail.com>

On Thu, Nov 29, 2012 at 2:00 PM, Martin von Zweigbergk
<martinvonz@gmail.com> wrote:
> Slightly off topic, but another difference (or somehow another aspect
> of the same difference?) that has tripped me up a few times is that
> "git checkout $rev ." only affects added and modified files (in $rev
> compared to HEAD), but "git reset $rev ." would also delete deleted
> files from the index.

Actually, what is the reasoning behind this difference? It almost
seems like a bug. I think I have just thought it was too obvious to be
a bug before, but thinking more about it, I can't see any reason why
"git checkout $rev" should delete files, but "git checkout $rev ."
shouldn't. I hope I'm just hallucinating or missing something. Can
someone shed some light on this?

^ permalink raw reply

* [RFC] remove/deprecate 'submodule init' and 'sync'
From: W. Trevor King @ 2012-11-30 17:53 UTC (permalink / raw)
  To: Jens Lehmann, Phil Hord
  Cc: Git, Heiko Voigt, Junio C Hamano, Jeff King, Shawn Pearce, Nahor
In-Reply-To: <50B54A68.60309@web.de>

On Wed, Nov 28, 2012 at 12:19:04AM +0100, Jens Lehmann wrote:
> Am 26.11.2012 22:00, schrieb W. Trevor King:
> > From: "W. Trevor King" <wking@tremily.us>
> > 
> > This allows users to override the .gitmodules value with a
> > per-repository value.
> 
> Your intentions makes lots of sense, but your patch does more than
> that. Copying the branch setting into .git/config sets the initial
> branch setting into stone. That makes it impossible to have a branch
> "foo" in the superproject using a branch "bar" in a submodule and
> another superproject branch "frotz" using branch "nitfol" for the
> same submodule. You should use the branch setting from .git/config
> if present and fall back to the branch setting from .gitmodules if
> not, which would enable the user to have her own setting if she
> doesn't like what upstream provides but would still enable others
> to follow different submodule branches in different superproject
> branches.

I've mulling this over, and when I started coding support for
submodule.<name>.remote, I had an idea.

On Thu, Nov 29, 2012 at 10:27:19PM -0500, W. Trevor King wrote:
> On Thu, Nov 29, 2012 at 08:11:20PM -0500, Phil Hord wrote:
> > I've always felt that the "origin" defaults are broken and are simply
> > being ignored because most users do not trip over them.  But ISTR that
> > submodule commands use the remote indicated by the superproject's
> > current remote-tracking configuration, with a fallback to 'origin' if
> > there is none.  Sort of a "best effort" algorithm, I think.  Am I
> > remembering that wrong?
> 
> The current code uses a bare "git-fetch".  I'm not sure what that
> defaults to if you're on a detached head.  If it bothers you, I'm fine
> adding the submodule.<name>.remote option in v6.

In my v5 patch, I check for submodule.<name>.remote first in the usual
`git config` files.  If I don't find what I'm looking for I fall back
on .gitmodules (basically Jens' suggestion).  However, my initial
copying-to-.git/config approach was mostly done to mimic existing
configuration handling in git-submodule.sh.  Since I agree with Jens
on configuration precendence, and I now had two options to read
(.branch and .remote), I thought I'd pull the logic out into its own
function (code included at the end).  While I was shifting the
existing submodule config handling over to my new function, I noticed
that with this logic, `submodule init` doesn't really do anything
important anymore.  Likewise for `submodule sync`, which seems to be
quite similar to `init`.

What to do about this?  `init` has been around for a while, so we
can't just remove it (maybe in 2.0?).  Leaving it in place is not
really a problem though, it just means that the user is locking in the
current .gitmodules configuration (as Jens pointed out with respect to
.branch).

I may be way off base here, as I'm fairly new to submodules in general
and these two commands in particular, but I thought I'd float the
idea.

Cheers,
Trevor

---
#
# Print a submodule configuration setting
#
# $1 = submodule name
# $2 = option name
# $3 = default value
#
# Checks in the usual git-config places first (for overrides),
# otherwise it falls back on .gitmodules.  This allows you to
# distribute project-wide defaults in .gitmodules, while still
# customizing individual repositories if necessary.  If the option is
# not in .gitmodules either, print a default value.
#
get_submodule_config()
{
	name="$1"
	option="$2"
	default="$3"
	value=$(git config submodule."$name"."$option")
	if test -z "$value"
	then
		value=$(git config -f .gitmodules submodule."$name"."$option")
	fi
	printf '%s' "${value:-$default}"
}

-- 
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy

^ permalink raw reply

* Re: [PATCH/RFC 1/5] mingw: make fgetc raise SIGINT if apropriate
From: Johannes Schindelin @ 2012-11-30 17:58 UTC (permalink / raw)
  To: Erik Faye-Lund; +Cc: git, msysgit, peff
In-Reply-To: <1352815447-8824-2-git-send-email-kusmabite@gmail.com>

Hi,

On Tue, 13 Nov 2012, Erik Faye-Lund wrote:

> Set a control-handler to prevent the process from terminating, and
> simulate SIGINT so it can be handled by a signal-handler as usual.

One thing you might want to mention is that the fgetc() handling is not
thread-safe, and intentionally so: if two threads read from the same
console, we are in trouble anyway.

BTW I like the new mingw_raise() very much!

Ciao,
Dscho

-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

^ permalink raw reply

* Re: [PATCH/RFC 2/5] compat/terminal: factor out echo-disabling
From: Johannes Schindelin @ 2012-11-30 17:59 UTC (permalink / raw)
  To: Erik Faye-Lund; +Cc: git, msysgit, peff
In-Reply-To: <1352815447-8824-3-git-send-email-kusmabite@gmail.com>

Hi,

On Tue, 13 Nov 2012, Erik Faye-Lund wrote:

> By moving the echo-disabling code to a separate function, we can
> implement OS-specific versions of it for non-POSIX platforms.
> 
> Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
> ---
>  compat/terminal.c | 43 +++++++++++++++++++++++++------------------
>  1 file changed, 25 insertions(+), 18 deletions(-)
> 
> diff --git a/compat/terminal.c b/compat/terminal.c
> index bbb038d..3217838 100644
> --- a/compat/terminal.c
> +++ b/compat/terminal.c
> @@ -14,6 +14,7 @@ static void restore_term(void)
>  		return;
>  
>  	tcsetattr(term_fd, TCSAFLUSH, &old_term);
> +	close(term_fd);
>  	term_fd = -1;
>  }

That looks like an independent resource leak fix... correct?

Rest looks awsomely fine.
Dscho

-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

^ permalink raw reply

* Re: [PATCH/RFC 4/5] mingw: reuse tty-version of git_terminal_prompt
From: Johannes Schindelin @ 2012-11-30 18:05 UTC (permalink / raw)
  To: Erik Faye-Lund; +Cc: git, msysgit, peff
In-Reply-To: <1352815447-8824-5-git-send-email-kusmabite@gmail.com>

Hi kusma,

On Tue, 13 Nov 2012, Erik Faye-Lund wrote:

> The getpass-implementation we use on Windows isn't at all ideal;
> it works in raw-mode (as opposed to cooked mode), and as a result
> does not deal correcly with deletion, arrow-keys etc.
> 
> Instead, use cooked mode to read a line at the time, allowing the
> C run-time to process the input properly.

Awesome!

The patch itself has a couple Windows-specific things in compat/terminal.c
that I would have loved to see in compat/mingw.c instead, but it looks as
if we have no choice: restore_term() and disable_echo() need to be
substantially different enough from the implementation in compat/mingw.c.

(Read: I am fine with this patch.)

Ciao,
Dscho

-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

^ permalink raw reply

* Re: [PATCH/RFC 5/5] mingw: get rid of getpass implementation
From: Johannes Schindelin @ 2012-11-30 18:06 UTC (permalink / raw)
  To: Erik Faye-Lund; +Cc: git, msysgit, peff
In-Reply-To: <1352815447-8824-6-git-send-email-kusmabite@gmail.com>

Hi kusma,

On Tue, 13 Nov 2012, Erik Faye-Lund wrote:

> There's no remaining call-sites, and as pointed out in the
> previous commit message, it's not quite ideal. So let's just
> lose it.

Awesome!
Dscho

-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

^ permalink raw reply

* Re: [msysGit] [PATCH/RFC 1/5] mingw: make fgetc raise SIGINT if apropriate
From: Jeff King @ 2012-11-30 18:11 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Erik Faye-Lund, git, msysgit
In-Reply-To: <alpine.DEB.1.00.1211301857170.31987@s15462909.onlinehome-server.info>

On Fri, Nov 30, 2012 at 06:58:11PM +0100, Johannes Schindelin wrote:

> Hi,
> 
> On Tue, 13 Nov 2012, Erik Faye-Lund wrote:
> 
> > Set a control-handler to prevent the process from terminating, and
> > simulate SIGINT so it can be handled by a signal-handler as usual.
> 
> One thing you might want to mention is that the fgetc() handling is not
> thread-safe, and intentionally so: if two threads read from the same
> console, we are in trouble anyway.

That makes sense to me, but I'm confused why it is part of mingw_fgetc,
which could in theory read from arbitrary streams, no? It it is not
necessarily a console operation at all. I feel like I'm probably missing
something subtle here...

-Peff

^ permalink raw reply

* Re: [RFC] remove/deprecate 'submodule init' and 'sync'
From: W. Trevor King @ 2012-11-30 18:17 UTC (permalink / raw)
  To: Jens Lehmann, Phil Hord
  Cc: Git, Heiko Voigt, Junio C Hamano, Jeff King, Shawn Pearce, Nahor
In-Reply-To: <20121130175309.GA718@odin.tremily.us>

On Fri, Nov 30, 2012 at 12:53:09PM -0500, W. Trevor King wrote:
> Likewise for `submodule sync`, which seems to be
> quite similar to `init`.

Ah, I'd remove the part of `sync` that touches the superproject's
.git/config, but keep the part that stores the superproject-reorded
URL in the submodule's config:

    url=$(get_submodule_config "$name" url)
    up_path=$(get_up_path "$sm_path")
    url=$(resolve_relative_url "$url" "$up_path") &&
    if test -n "$url"
    then
      if test -e "$sm_path"/.git
      then
      (
        clear_local_git_env
        cd "$sm_path"
        remote=$(get_default_remote)
        git config remote."$remote".url "$url"
      )
      fi
    fi

I should probably also tweak sync to do similar things with
submodule.<name>.branch and .remote as part of my `--update remote`
series.

Cheers,
Trevor

-- 
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy

^ permalink raw reply

* Re: [PATCH/RFC 2/5] compat/terminal: factor out echo-disabling
From: Jeff King @ 2012-11-30 18:19 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Erik Faye-Lund, git, msysgit
In-Reply-To: <alpine.DEB.1.00.1211301858570.31987@s15462909.onlinehome-server.info>

On Fri, Nov 30, 2012 at 06:59:30PM +0100, Johannes Schindelin wrote:

> > diff --git a/compat/terminal.c b/compat/terminal.c
> > index bbb038d..3217838 100644
> > --- a/compat/terminal.c
> > +++ b/compat/terminal.c
> > @@ -14,6 +14,7 @@ static void restore_term(void)
> >  		return;
> >  
> >  	tcsetattr(term_fd, TCSAFLUSH, &old_term);
> > +	close(term_fd);
> >  	term_fd = -1;
> >  }
> 
> That looks like an independent resource leak fix... correct?

I don't think so. In the current code, term_fd does not take ownership
of the fd. It is fundamentally part of the FILE* in git_terminal_prompt,
and is closed when we fclose() that. But in Erik's refactor, we actually
open a _second_ descriptor to /dev/tty, which needs to be closed.

I don't think there is any reason that should not work (the terminal
characteristics should be per-tty, not per-descriptor), though it does
feel a little hacky to have to open /dev/tty twice.

-Peff

-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

^ permalink raw reply

* Re: [PATCH/RFC 3/5] compat/terminal: separate input and output handles
From: Jeff King @ 2012-11-30 18:22 UTC (permalink / raw)
  To: Erik Faye-Lund; +Cc: git, msysgit
In-Reply-To: <1352815447-8824-4-git-send-email-kusmabite@gmail.com>

On Tue, Nov 13, 2012 at 03:04:05PM +0100, Erik Faye-Lund wrote:

> On Windows, the terminal cannot be opened in read-write mode, so
> we need distinct pairs for reading and writing. Since this works
> fine on other platforms as well, always open them in pairs.

Looks OK. We're now opening /dev/tty three separate times in the no-echo
case, but it's not like this is in a performance critical loop.

-Peff

-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

^ permalink raw reply

* Re: [PATCH/RFC 4/5] mingw: reuse tty-version of git_terminal_prompt
From: Jeff King @ 2012-11-30 18:27 UTC (permalink / raw)
  To: Erik Faye-Lund; +Cc: git, msysgit
In-Reply-To: <1352815447-8824-5-git-send-email-kusmabite@gmail.com>

On Tue, Nov 13, 2012 at 03:04:06PM +0100, Erik Faye-Lund wrote:

> The getpass-implementation we use on Windows isn't at all ideal;
> it works in raw-mode (as opposed to cooked mode), and as a result
> does not deal correcly with deletion, arrow-keys etc.
> 
> Instead, use cooked mode to read a line at the time, allowing the
> C run-time to process the input properly.
> 
> Since we set files to be opened in binary-mode by default on
> Windows, introduce a FORCE_TEXT macro that expands to the "t"
> modifier that forces the terminal to be opened in text-mode so we
> do not have to deal with CRLF issues.

I think this is OK. I had originally envisioned just having a separate
"#ifdef WIN32" and not really sharing code at all with /dev/tty because
I was worried that the conditionals would end up making it hard to read.
This is a little more complex than I would have liked, but I do not see
how the code sharing could be simplified any more than what you have
done, and it is nice to avoid repeating ourselves.

-Peff

-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

^ permalink raw reply

* Re: [PATCH/RFC 5/5] mingw: get rid of getpass implementation
From: Jeff King @ 2012-11-30 18:27 UTC (permalink / raw)
  To: Erik Faye-Lund; +Cc: git, msysgit
In-Reply-To: <1352815447-8824-6-git-send-email-kusmabite@gmail.com>

On Tue, Nov 13, 2012 at 03:04:07PM +0100, Erik Faye-Lund wrote:

> There's no remaining call-sites, and as pointed out in the
> previous commit message, it's not quite ideal. So let's just
> lose it.
> 
> Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
> ---
>  compat/mingw.c | 15 ---------------
>  compat/mingw.h |  2 --
>  2 files changed, 17 deletions(-)

Yay!

-Peff

-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

^ permalink raw reply

* Re: [PATCH/RFC 0/5] win32: support echo for terminal-prompt
From: Jeff King @ 2012-11-30 18:30 UTC (permalink / raw)
  To: Erik Faye-Lund; +Cc: git, msysgit
In-Reply-To: <CABPQNSZ7YRgvS1=SR=Nx0wcn+NXCVwqjVShk-ug3=yX62RH-oQ@mail.gmail.com>

On Fri, Nov 30, 2012 at 11:16:59AM +0100, Erik Faye-Lund wrote:

> Ping?

Thanks for the reminder; your initial series came while I was traveling.

I think it looks good. The compat/terminal code ends up a little uglier,
but I think you overall did a good job of balancing code reuse across
platforms with readability.

-Peff

-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

^ permalink raw reply

* Re: [PATCH] fsck: warn about ".git" in trees
From: Torsten Bögershausen @ 2012-11-30 19:50 UTC (permalink / raw)
  To: Jeff King; +Cc: Junio C Hamano, git
In-Reply-To: <20121128213529.GA16518@sigill.intra.peff.net>

> Having a ".git" entry inside a tree can cause confusing
> results on checkout. At the top-level, you could not
> checkout such a tree, as it would complain about overwriting
> the real ".git" directory. In a subdirectory, you might
> check it out, but performing operations in the subdirectory
> would confusingly consider the in-tree ".git" directory as
> the repository.
[snip]
> +	int has_dotgit = 0;

Name like "." or ".." are handled as directories by the OS.

".git" could be a file or a directory, at least in theory,
and from the OS point of view,
but we want to have this as a reserved name.

Looking at bad directory names, which gives trouble when checking out:

Should we check for "/" or "../blabla" as well?

^ permalink raw reply

* Re: [PATCH] fsck: warn about ".git" in trees
From: Jeff King @ 2012-11-30 19:55 UTC (permalink / raw)
  To: Torsten Bögershausen; +Cc: Junio C Hamano, git
In-Reply-To: <50B90E11.8090501@web.de>

On Fri, Nov 30, 2012 at 08:50:41PM +0100, Torsten Bögershausen wrote:

> >Having a ".git" entry inside a tree can cause confusing
> >results on checkout. At the top-level, you could not
> >checkout such a tree, as it would complain about overwriting
> >the real ".git" directory. In a subdirectory, you might
> >check it out, but performing operations in the subdirectory
> >would confusingly consider the in-tree ".git" directory as
> >the repository.
> [snip]
> >+	int has_dotgit = 0;
> 
> Name like "." or ".." are handled as directories by the OS.

Right. In theory git could run on a system that does not treat them
specially, but in practice they are going to be problematic on most
systems.

> ".git" could be a file or a directory, at least in theory, and from
> the OS point of view, but we want to have this as a reserved name.

Exactly.

> Looking at bad directory names, which gives trouble when checking out:
> 
> Should we check for "/" or "../blabla" as well?

We do already (the error is "contains full pathnames"). We also cover
empty pathnames and some other cases.

-Peff

^ permalink raw reply

* Re: [PATCH 5/5] launch_editor: propagate SIGINT from editor to git
From: Jeff King @ 2012-11-30 20:24 UTC (permalink / raw)
  To: Johannes Sixt; +Cc: Kalle Olavi Niemitalo, Paul Fox, git
In-Reply-To: <50A00116.8060604@kdbg.org>

On Sun, Nov 11, 2012 at 08:48:38PM +0100, Johannes Sixt wrote:

> Am 11.11.2012 17:57, schrieb Jeff King:
> > @@ -51,6 +51,8 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en
> >  		sigchain_push(SIGINT, SIG_IGN);
> >  		ret = finish_command(&p);
> >  		sigchain_pop(SIGINT);
> > +		if (WIFSIGNALED(ret) && WTERMSIG(ret) == SIGINT)
> > +			raise(SIGINT);
> 
> The return value of finish_command() is already a digested version of
> waitpid's status value. According to
> Documentation/technical/api-run-command.txt:
> 
> . If the program terminated due to a signal, then the return value is
> the signal number - 128, ...
> 
> the correct condition would be
> 
> 		if (ret == SIGINT - 128)

Yeah, that is the same thing as WTERMSIG (which uses "ret & 0x7f") for
the range of -127..-1. I do not mind changing it to match run-command's
stated output, but I am curious whether there are systems where WTERMSIG
is not defined in the same way, and the code would break.

-Peff

^ permalink raw reply

* Bug report: probiem with across-branch git-diff
From: Tomas Neme @ 2012-11-30 21:13 UTC (permalink / raw)
  To: git

(env)lacrymology@Roller:boilerplate$ git rev-parse
test_project:boilerplate/apps/custom_zinnia/models/blog.py
9e93aa98ad2aeb8094dd3206b02e7bd835ce6899

lacrymology@Roller:boilerplate$ git diff
boilerplate/apps/custom_zinnia/abstract_models.py
test_project:boilerplate/apps/custom_zinnia/models/blog.py
fatal: Path 'boilerplate/apps/custom_zinnia/models/blog.py' exists,
but not 'boilerplate/apps/custom_zinnia/models/blog.py'.
Did you mean 'test_project:boilerplate/apps/custom_zinnia/models/blog.py'
aka 'test_project:./boilerplate/apps/custom_zinnia/models/blog.py'?

blog.py doesn't exist in the current branch. Current branch descends
from test_project, and abstract_models.py it's actually the same file
`git mv`'ed

--
"The whole of Japan is pure invention. There is no such country, there
are no such people" --Oscar Wilde

|_|0|_|
|_|_|0|
|0|0|0|

(\__/)
(='.'=)This is Bunny. Copy and paste bunny
(")_(") to help him gain world domination.

^ permalink raw reply

* [PATCH] gitk: add a checkbox to control the visibility of tags
From: Łukasz Stelmach @ 2012-11-30 21:08 UTC (permalink / raw)
  To: git, gitster; +Cc: Łukasz Stelmach

Enable hiding of tags displayed in the tree as yellow labels.
If a repository is used together with a system like Gerrit
there may be quite a lot of tags used to control building
and there may be hardly any place left for commit subjects.

Signed-off-by: Łukasz Stelmach <stlman@poczta.fm>
---
 gitk-git/gitk |   23 +++++++++++++++--------
 1 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/gitk-git/gitk b/gitk-git/gitk
index d93bd99..274b46b 100755
--- a/gitk-git/gitk
+++ b/gitk-git/gitk
@@ -1754,7 +1754,7 @@ proc readrefs {} {
     global tagids idtags headids idheads tagobjid
     global otherrefids idotherrefs mainhead mainheadid
     global selecthead selectheadid
-    global hideremotes
+    global hideremotes hidetags
 
     foreach v {tagids idtags headids idheads otherrefids idotherrefs} {
 	catch {unset $v}
@@ -1776,6 +1776,7 @@ proc readrefs {} {
 	    set headids($name) $id
 	    lappend idheads($id) $name
 	} elseif {[string match "tags/*" $name]} {
+	    if {$hidetags} continue
 	    # this lets refs/tags/foo^{} overwrite refs/tags/foo,
 	    # which is what we want since the former is the commit ID
 	    set name [string range $name 5 end]
@@ -2702,7 +2703,7 @@ proc savestuff {w} {
     global cmitmode wrapcomment datetimeformat limitdiffs
     global colors uicolor bgcolor fgcolor diffcolors diffcontext selectbgcolor
     global autoselect autosellen extdifftool perfile_attrs markbgcolor use_ttk
-    global hideremotes want_ttk
+    global hideremotes hidetags want_ttk
 
     if {$stuffsaved} return
     if {![winfo viewable .]} return
@@ -2725,6 +2726,7 @@ proc savestuff {w} {
 	puts $f [list set autosellen $autosellen]
 	puts $f [list set showneartags $showneartags]
 	puts $f [list set hideremotes $hideremotes]
+	puts $f [list set hidetags $hidetags]
 	puts $f [list set showlocalchanges $showlocalchanges]
 	puts $f [list set datetimeformat $datetimeformat]
 	puts $f [list set limitdiffs $limitdiffs]
@@ -10864,7 +10866,7 @@ proc create_prefs_page {w} {
 proc prefspage_general {notebook} {
     global NS maxwidth maxgraphpct showneartags showlocalchanges
     global tabstop limitdiffs autoselect autosellen extdifftool perfile_attrs
-    global hideremotes want_ttk have_ttk
+    global hideremotes hidetags want_ttk have_ttk
 
     set page [create_prefs_page $notebook.general]
 
@@ -10887,6 +10889,9 @@ proc prefspage_general {notebook} {
     ${NS}::checkbutton $page.hideremotes -text [mc "Hide remote refs"] \
 	-variable hideremotes
     grid x $page.hideremotes -sticky w
+    ${NS}::checkbutton $page.hidetags -text [mc "Hide tag labels"] \
+	-variable hidetags
+    grid x $page.hidetags -sticky w
 
     ${NS}::label $page.ddisp -text [mc "Diff display options"]
     grid $page.ddisp - -sticky w -pady 10
@@ -10988,7 +10993,7 @@ proc doprefs {} {
     global oldprefs prefstop showneartags showlocalchanges
     global uicolor bgcolor fgcolor ctext diffcolors selectbgcolor markbgcolor
     global tabstop limitdiffs autoselect autosellen extdifftool perfile_attrs
-    global hideremotes want_ttk have_ttk
+    global hideremotes hidetags want_ttk have_ttk
 
     set top .gitkprefs
     set prefstop $top
@@ -10997,7 +11002,7 @@ proc doprefs {} {
 	return
     }
     foreach v {maxwidth maxgraphpct showneartags showlocalchanges \
-		   limitdiffs tabstop perfile_attrs hideremotes want_ttk} {
+		   limitdiffs tabstop perfile_attrs hideremotes hidetags want_ttk} {
 	set oldprefs($v) [set $v]
     }
     ttk_toplevel $top
@@ -11117,7 +11122,7 @@ proc prefscan {} {
     global oldprefs prefstop
 
     foreach v {maxwidth maxgraphpct showneartags showlocalchanges \
-		   limitdiffs tabstop perfile_attrs hideremotes want_ttk} {
+		   limitdiffs tabstop perfile_attrs hideremotes hidetags want_ttk} {
 	global $v
 	set $v $oldprefs($v)
     }
@@ -11131,7 +11136,7 @@ proc prefsok {} {
     global oldprefs prefstop showneartags showlocalchanges
     global fontpref mainfont textfont uifont
     global limitdiffs treediffs perfile_attrs
-    global hideremotes
+    global hideremotes hidetags
 
     catch {destroy $prefstop}
     unset prefstop
@@ -11177,7 +11182,8 @@ proc prefsok {} {
 	  $limitdiffs != $oldprefs(limitdiffs)} {
 	reselectline
     }
-    if {$hideremotes != $oldprefs(hideremotes)} {
+    if {$hideremotes != $oldprefs(hideremotes) ||
+        $hidetags != $oldprefs(hidetags)} {
 	rereadrefs
     }
 }
@@ -11601,6 +11607,7 @@ set cmitmode "patch"
 set wrapcomment "none"
 set showneartags 1
 set hideremotes 0
+set hidetags 0
 set maxrefs 20
 set maxlinelen 200
 set showlocalchanges 1
-- 
1.7.8.6

^ permalink raw reply related

* [PATCH 0/5] ignore SIG{INT,QUIT} when launching editor
From: Jeff King @ 2012-11-30 22:39 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Paul Fox, Krzysztof Mazur

This is a re-roll of the pf/editor-ignore-sigint series.

There are two changes from the original:

  1. We ignore both SIGINT and SIGQUIT for "least surprise" compared to
     system(3).

  2. We now use "code + 128" to look for signal death (instead of
     WTERMSIG), as per run-command's documentation on how it munges the
     code.

People mentioned some buggy editors which go into an infinite EIO loop
when their parent dies due to SIGQUIT. That should be a non-issue now,
as we will be ignoring SIGQUIT. And even if you could replicate it
(e.g., with another signal) those programs should be (and reportedly
have been) fixed. It is not git's job to babysit its child processes.

The patches are:

  [1/5]: run-command: drop silent_exec_failure arg from wait_or_whine
  [2/5]: launch_editor: refactor to use start/finish_command
  [3/5]: launch_editor: ignore terminal signals while editor has control
  [4/5]: run-command: do not warn about child death from terminal
  [5/5]: launch_editor: propagate signals from editor to git

Since this can be thought of as "act more like system(3)", I wondered
whether the signal-ignore logic should be moved into run-command, or
even used by default for blocking calls to run_command (which are
basically our version of system(3)). But it is detrimental in the common
case that the child is not taking control of the terminal, and is just
an implementation detail (e.g., we call "git update-ref" behind the
scenes, but the user does not know or care). If they hit ^C during such
a run and we are ignoring SIGINT, then either:

  1. we will notice the child died by signal and report an
     error in the subprocess rather than just dying; the end result is
     similar, but the error is unnecessarily confusing

  2. we do not bother to check the child's return code (because we do
     not care whether the child succeeded or not, like a "gc --auto");
     we end up totally ignoring the user's request to abort the
     operation

So I do not think we care about this behavior except for launching the
editor. And the signal-propagation behavior of 5/5 is really so weirdly
editor-specific (because it is about behaving well whether the child
blocks signals or not).

-Peff

^ permalink raw reply

* [PATCH 1/5] run-command: drop silent_exec_failure arg from wait_or_whine
From: Jeff King @ 2012-11-30 22:40 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Paul Fox, Krzysztof Mazur
In-Reply-To: <20121130223943.GA27120@sigill.intra.peff.net>

We do not actually use this parameter; instead we complain
from the child itself (for fork/exec) or from start_command
(if we are using spawn on Windows).

Signed-off-by: Jeff King <peff@peff.net>
---
 run-command.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/run-command.c b/run-command.c
index 3b982e4..3aae270 100644
--- a/run-command.c
+++ b/run-command.c
@@ -226,7 +226,7 @@ static inline void set_cloexec(int fd)
 		fcntl(fd, F_SETFD, flags | FD_CLOEXEC);
 }
 
-static int wait_or_whine(pid_t pid, const char *argv0, int silent_exec_failure)
+static int wait_or_whine(pid_t pid, const char *argv0)
 {
 	int status, code = -1;
 	pid_t waiting;
@@ -432,8 +432,7 @@ fail_pipe:
 		 * At this point we know that fork() succeeded, but execvp()
 		 * failed. Errors have been reported to our stderr.
 		 */
-		wait_or_whine(cmd->pid, cmd->argv[0],
-			      cmd->silent_exec_failure);
+		wait_or_whine(cmd->pid, cmd->argv[0]);
 		failed_errno = errno;
 		cmd->pid = -1;
 	}
@@ -538,7 +537,7 @@ int finish_command(struct child_process *cmd)
 
 int finish_command(struct child_process *cmd)
 {
-	return wait_or_whine(cmd->pid, cmd->argv[0], cmd->silent_exec_failure);
+	return wait_or_whine(cmd->pid, cmd->argv[0]);
 }
 
 int run_command(struct child_process *cmd)
-- 
1.8.0.1.620.g558b0aa

^ permalink raw reply related


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