* Re: git svn clone just stops
From: Pascal Obry @ 2009-12-20 9:44 UTC (permalink / raw)
To: Eric Wong; +Cc: Mark Jerkovic, git
In-Reply-To: <20091220085731.GA11535@dcvr.yhbt.net>
Hi Eric,
> --revision may be passed to fetch, but its optional. fetch should
> always know where to pickup after it left off.
Ok, thanks for the info.
--
--|------------------------------------------------------
--| Pascal Obry Team-Ada Member
--| 45, rue Gabriel Peri - 78114 Magny Les Hameaux FRANCE
--|------------------------------------------------------
--| http://www.obry.net - http://v2p.fr.eu.org
--| "The best way to travel is by means of imagination"
--|
--| gpg --keyserver keys.gnupg.net --recv-key F949BD3B
^ permalink raw reply
* Re: [msysGit] [PATCH] git-gui: use themed tk widgets with Tk 8.5
From: Johannes Schindelin @ 2009-12-20 13:16 UTC (permalink / raw)
To: Pat Thoyts; +Cc: git, Shawn O. Pearce, msysgit
In-Reply-To: <87my1ev4gi.fsf@users.sourceforge.net>
Hi,
On Sun, 20 Dec 2009, Pat Thoyts wrote:
> This patch enables the use of themed Tk widgets with Tk 8.5 and above.
> These make a significant difference on Windows in making the application
> appear native. The gui.usettk git config variable may be set to disable
> this if the user prefers the classic Tk look.
>
> On Windows and MacOSX ttk defaults to the native look as much as possible.
> On X11 the user may select a theme using the TkTheme XRDB resource class.
Maybe you could describe a quick way to set that resource?
> Some support is included for Tk 8.6 features (themed spinbox and native
> font chooser for MacOSX and Windows).
>
> Signed-off-by: Pat Thoyts <patthoyts@users.sourceforge.net>
>
> There is a screenshot of this running on Windows 7 at
> http://www.patthoyts.tk/screenshots/git-gui-win32.png
> It has been tested on Linux but not on MacOSX yet.
Please put this paragraph after the "---". Oh, and could you also include
a screenshot without ttk?
As to the patch: it is huge. I suspect that it would be easier to read
and verify if it was done in two patches: the first one introducing helper
procs that replace the existing repeated code, and a second patch that
modifies only the helper procs to handle ttk if desired. You did that
with pad_label, but that proc is not defined in a central place, but
limited to about.tcl.
Hmm?
Thanks,
Dscho
^ permalink raw reply
* Re: [msysGit] [PATCH] git-gui: use themed tk widgets with Tk 8.5
From: Pat Thoyts @ 2009-12-20 15:23 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Pat Thoyts, git, Shawn O. Pearce, msysgit
In-Reply-To: <alpine.DEB.1.00.0912201412420.4985@pacific.mpi-cbg.de>
2009/12/20 Johannes Schindelin <Johannes.Schindelin@gmx.de>:
>>
>> On Windows and MacOSX ttk defaults to the native look as much as possible.
>> On X11 the user may select a theme using the TkTheme XRDB resource class.
>
> Maybe you could describe a quick way to set that resource?
echo '*TkTheme: clam' | xrdb -merge -
Or edit ~/.Xdefaults or maybe ~/.Xresources to add a line containing
*TkTheme: clam
Used to be the X resource database was the way to customize X11. The
builtin themes are:
default: thin borders, kind of windows98ish
classic: motif style
alt: more windowsy still.
clam: based on some version of the XFCE look a few years ago.
On Windows you also get winnative, xpnative or vista but the intention
is that Tk just makes everything look native by selecting the correct
theme when it starts up. MacOSX gets an aqua theme. There is some work
in progress to get the Qt or Gtk styling engines to draw the ttk
elements so that it can pick up the current GNOME/KDE desktop -
effectively making it look native on such systems there too. However,
on Windows is where this stuff makes the most difference and works
best.
You can of course just run git-gui in any git repository to see how it
looks at the moment. However I put up another screenshot at
http://www.patthoyts.tk/screenshots/git-gui-old.png which is the
unmodified git-gui code with tcl8.5. The fonts look worse with 8.4
(even on windows).
> As to the patch: it is huge. I suspect that it would be easier to read
> and verify if it was done in two patches: the first one introducing helper
> procs that replace the existing repeated code, and a second patch that
> modifies only the helper procs to handle ttk if desired. You did that
> with pad_label, but that proc is not defined in a central place, but
> limited to about.tcl.
I could split it up but it is quite hard to validate without switching
themes to ensure you got everything. I toggle through some themes
looking for unchanged widgets or rogue borders.
I did find a bug in the choose_rev conversion when I rechecked with
tcl 8.4 so I will reissue this.
Pat Thoyts
^ permalink raw reply
* Re: [PATCH] rebase -i: print the editor name if it fails to launch
From: Junio C Hamano @ 2009-12-20 18:44 UTC (permalink / raw)
To: Björn Gustavsson; +Cc: git
In-Reply-To: <4B2DE132.1080700@gmail.com>
Björn Gustavsson <bgustavsson@gmail.com> writes:
> I am not sure whether "git am" also should be modified
> to call git_editor_error_string. Currently it ignores
> any errors from the call to git_editor. Should it abort
> if there is an error or should it print a warning like this
>
> echo "Warning: $(git_editor_error_string)"
>
> if the editor fails to launch?
It probably is better, even though the lack of check there is not as
harmful compared to the one in "rebase -i" that annoyed you (which I also
see occasionally after freshly rebooting my environment, as I use
emacsclient like you do). You will be taken back to the "what now?"
interactive loop and have a chance to \C-c out of it. So such a patch
would help not in the sense that it would prevent the command from doing
what you didn't intend to (which is your "rebase -i" patch is about), but
in the sense that it hints what needs to be fixed once you break out of
the command.
> - git_editor "$TODO" ||
> - die_abort "Could not execute editor"
> + git_editor "$TODO" || die_abort "$(git_editor_error_string)"
Isn't this too elaborate? git_editor() has already run and when it
attempted to launch the editor it assigned to GIT_EDITOR in order to use
it as an eval string.
git_editor "$TODO" ||
die_abort "Failed to run '${GIT_EDITOR:-your editor}'"
would suffice, no?
The alternative string is for a case where "git var" gave an empty string,
or GIT_EDITOR was an empty string from the beginning.
^ permalink raw reply
* Re: [PATCH] git-gui: use themed tk widgets with Tk 8.5
From: Junio C Hamano @ 2009-12-20 19:04 UTC (permalink / raw)
To: Pat Thoyts; +Cc: Johannes Schindelin, Pat Thoyts, git, Shawn O. Pearce, msysgit
In-Reply-To: <a5b261830912200723s131f0b93w2f5ab94f19faf86a@mail.gmail.com>
Pat Thoyts <patthoyts@googlemail.com> writes:
> 2009/12/20 Johannes Schindelin <Johannes.Schindelin@gmx.de>:
>>>
>>> On Windows and MacOSX ttk defaults to the native look as much as possible.
>>> On X11 the user may select a theme using the TkTheme XRDB resource class.
>>
>> Maybe you could describe a quick way to set that resource?
>
> echo '*TkTheme: clam' | xrdb -merge -
>
> Or edit ~/.Xdefaults or maybe ~/.Xresources to add a line containing
> *TkTheme: clam
> Used to be the X resource database was the way to customize X11. The
> builtin themes are:
> default: thin borders, kind of windows98ish
> classic: motif style
> alt: more windowsy still.
> clam: based on some version of the XFCE look a few years ago.
On X, does the "default: thin borders, kind of windows98ish" look very
different from the non-themed classic Tk style?
What I am trying to get at is to gain enough information so that I can
suggest a rewrite of these two lines (quoted at the beginning by Dscho)
like this:
Ttk defaults to the native look as much as possible on Windows and
MacOSX; the default on X is the _________ look.
The user may choose a theme by:
- on Windows, doing ________;
- on MacOSX, doing _________;
- on X, selecting a theme using the TkTheme XRDB resource class;
which I think would be much easier to read.
^ permalink raw reply
* Re: [msysGit] [PATCH] git-gui: use themed tk widgets with Tk 8.5
From: Pat Thoyts @ 2009-12-20 20:25 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johannes Schindelin, Pat Thoyts, git, Shawn O. Pearce, msysgit
In-Reply-To: <7vws0ha2mt.fsf@alter.siamese.dyndns.org>
2009/12/20 Junio C Hamano <gitster@pobox.com>:
>
> On X, does the "default: thin borders, kind of windows98ish" look very
> different from the non-themed classic Tk style?
>
Here is a big set of screenshots then:
Unmodified or with Tk 8.4:
original windows: http://www.patthoyts.tk/screenshots/git-gui-old.png
original unix: http://www.patthoyts.tk/screenshots/git-gui-x11.png
With Tk 8.5 and using themed widgets:
unix default: http://www.patthoyts.tk/screenshots/git-gui-default.png
unix alt: http://www.patthoyts.tk/screenshots/git-gui-alt.png
unix classic: http://www.patthoyts.tk/screenshots/git-gui-classic.png
unix clam: http://www.patthoyts.tk/screenshots/git-gui-clam.png
unix tileqt: http://www.patthoyts.tk/screenshots/git-gui-tileqt.png
(experimental)
windows7: http://www.patthoyts.tk/screenshots/git-gui-win32.png
> What I am trying to get at is to gain enough information so that I can
> suggest a rewrite of these two lines (quoted at the beginning by Dscho)
> like this:
>
> Ttk defaults to the native look as much as possible on Windows and
> MacOSX; the default on X is the _________ look.
>
> The user may choose a theme by:
>
> - on Windows, doing ________;
> - on MacOSX, doing _________;
> - on X, selecting a theme using the TkTheme XRDB resource class;
>
> which I think would be much easier to read.
You write better than I do. The theme used globally by themed Tk can
be selected on at least Ubuntu's GNOME desktop by adding the following
line into ~/.Xresources:
*TkTheme: clam
It is also possible to add a Theme menu or selection combobox onto the
options dialog and record a theme selection along with the other
git-gui saved options. Then a user could use 'git config gui.theme
clam'. The set of available themes can be extended with packages and
is somewhat platform dependent (ie: xpnative can only be present on
Windows XP and above).
Pat Thoyts.
^ permalink raw reply
* Re: [msysGit] [PATCH] git-gui: use themed tk widgets with Tk 8.5
From: Junio C Hamano @ 2009-12-20 21:05 UTC (permalink / raw)
To: Pat Thoyts; +Cc: Johannes Schindelin, Pat Thoyts, git, Shawn O. Pearce, msysgit
In-Reply-To: <a5b261830912201225q7d596b55qb54676922301954e@mail.gmail.com>
Pat Thoyts <patthoyts@googlemail.com> writes:
> 2009/12/20 Junio C Hamano <gitster@pobox.com>:
>>
>> On X, does the "default: thin borders, kind of windows98ish" look very
>> different from the non-themed classic Tk style?
>>
>
> Here is a big set of screenshots then:
Which is probably very helpful to others. Thanks.
On the other hand, if you wanted to give me just a quick answer, you
simply could have said "The default X11 theme looks like the good old
classic Tk that draws diamond shaped radio-buttons". (which is the
impression I got after running the current git-gui installation on my box
and comparing it with your "Tk 8.5 themed with unix default").
^ permalink raw reply
* Re: Efficiency and correctness patches for git-svn mergeinfo support
From: Sam Vilain @ 2009-12-20 21:07 UTC (permalink / raw)
To: Andrew Myrick; +Cc: git, Eric Wong
In-Reply-To: <6b2f9b1d0912191415n560a5a58xbe6390b1fcade854@mail.gmail.com>
On Sat, 2009-12-19 at 14:15 -0800, Andrew Myrick wrote:
> I tried cloning from a fairly recent revision that I knew was after
> our switchover to svn 1.5, and I received a number of these errors:
>
> Couldn't find revmap for [branch]
> Exiting subroutine via next at /Users/adm/libexec/git-core/git-svn line 2983.
> Exiting subroutine via next at /Users/adm/libexec/git-core/git-svn line 2983.
> Exiting subroutine via next at /Users/adm/libexec/git-core/git-svn line 2983.
>
> I'm not sure if this is expected, since I didn't clone from the whole
> repo, but it did cause a lot of spew. I'm starting a fresh clone now,
> but it takes a few days to get through the whole repository. I'm
> fairly new to git, so I would welcome any tips on how I can test this
> more quickly.
Whoops, no, not expected, I'll post a minor correction. That means that
the branch which was merged in does not have git-svn metadata; ie, it's
not being tracked explicitly. If people are doing merging of things
which aren't roots of branches you would expect this. SVN, like
Perforce, supports a confusing amount of flexibility in its merge
tracking. If [branch] is a real branch, then you'll want to see why it
doesn't have metadata yet. Is it really a sub-tree of a real branch?
You could fetch it independently using a separate git-svn remote, or you
could ignore the warning; it should be relatively self-evident what
happened from the merge message and the contents of the changeset.
Note if your repository was significantly re-organized at any point, it
will pay to treat each section of history as a separate import project,
and stitch the results together afterwards using grafts and
filter-branch.
This version should be *significantly* faster than the old one. ie, it
should not take a minute per commit while importing the heavily
merged-into integration branch. Possibly a few seconds at most.
Sam
^ permalink raw reply
* Re: [PATCH 2/5] git-svn: memoize conversion of SVN merge ticket info to git commit ranges
From: Sam Vilain @ 2009-12-20 21:24 UTC (permalink / raw)
To: git; +Cc: Eric Wong
In-Reply-To: <1261240435-8948-3-git-send-email-sam@vilain.net>
On Sun, 2009-12-20 at 05:33 +1300, Sam Vilain wrote:
> Each time the svn mergeinfo ticket changes, we look it up in the rev_map;
> when there are a lot of merged branches, this will result in many repeated
> lookups of the same information for subsequent commits. Arrange the slow
> part of the function so that it may be memoized, and memoize it. The more
> expensive revision walking operation can be memoized separately.
>
> Signed-off-by: Sam Vilain <sam@vilain.net>
> ---
> git-svn.perl | 91 ++++++++++++++++++++++++++++++++++-----------------------
> 1 files changed, 54 insertions(+), 37 deletions(-)
>
> diff --git a/git-svn.perl b/git-svn.perl
> index a4b052c..3b17a83 100755
> --- a/git-svn.perl
> +++ b/git-svn.perl
> @@ -1634,6 +1634,7 @@ use Carp qw/croak/;
> use File::Path qw/mkpath/;
> use File::Copy qw/copy/;
> use IPC::Open3;
> +use Memoize; # core since 5.8.0, Jul 2002
>
> my ($_gc_nr, $_gc_period);
>
> @@ -2967,6 +2968,55 @@ sub find_extra_svk_parents {
> }
> }
>
> +sub lookup_svn_merge {
> + my $uuid = shift;
> + my $url = shift;
> + my $merge = shift;
> +
> + my ($source, $revs) = split ":", $merge;
> + my $path = $source;
> + $path =~ s{^/}{};
> + my $gs = Git::SVN->find_by_url($url.$source, $url, $path);
> + if ( !$gs ) {
> + warn "Couldn't find revmap for $url$source\n";
> + next;
> + }
As mentioned in the other thread, that 'next' should now be 'return'.
Sam
^ permalink raw reply
* Re: Efficiency and correctness patches for git-svn mergeinfo support
From: Andrew Myrick @ 2009-12-20 22:03 UTC (permalink / raw)
To: Sam Vilain; +Cc: git, Eric Wong
In-Reply-To: <1261343240.20752.20.camel@denix>
On Sun, Dec 20, 2009 at 1:07 PM, Sam Vilain <sam@vilain.net> wrote:
> On Sat, 2009-12-19 at 14:15 -0800, Andrew Myrick wrote:
>> I tried cloning from a fairly recent revision that I knew was after
>> our switchover to svn 1.5, and I received a number of these errors:
>>
>> Couldn't find revmap for [branch]
>> Exiting subroutine via next at /Users/adm/libexec/git-core/git-svn line 2983.
>> Exiting subroutine via next at /Users/adm/libexec/git-core/git-svn line 2983.
>> Exiting subroutine via next at /Users/adm/libexec/git-core/git-svn line 2983.
>>
>> I'm not sure if this is expected, since I didn't clone from the whole
>> repo, but it did cause a lot of spew. I'm starting a fresh clone now,
>> but it takes a few days to get through the whole repository. I'm
>> fairly new to git, so I would welcome any tips on how I can test this
>> more quickly.
>
> Whoops, no, not expected, I'll post a minor correction. That means that
> the branch which was merged in does not have git-svn metadata; ie, it's
> not being tracked explicitly. If people are doing merging of things
> which aren't roots of branches you would expect this. SVN, like
> Perforce, supports a confusing amount of flexibility in its merge
> tracking. If [branch] is a real branch, then you'll want to see why it
> doesn't have metadata yet. Is it really a sub-tree of a real branch?
> You could fetch it independently using a separate git-svn remote, or you
> could ignore the warning; it should be relatively self-evident what
> happened from the merge message and the contents of the changeset.
I think the problem is that I started fetching from a later revision
than when the branch was created and reintegrated. There would be no
metadata for the branch, so a lookup for it would fail. Fetching the
branch explicitly, or fetching from r1 as my current test is doing,
should not experience this problem.
> Note if your repository was significantly re-organized at any point, it
> will pay to treat each section of history as a separate import project,
> and stitch the results together afterwards using grafts and
> filter-branch.
I don't believe it was, but I will keep that in mind if I run into
trouble. The import from r1 went fine with git v1.6.5.*, and it's
proceeding well now.
> This version should be *significantly* faster than the old one. ie, it
> should not take a minute per commit while importing the heavily
> merged-into integration branch. Possibly a few seconds at most.
So far, most commits are fetched in under 3s with your new version.
Thanks for your help, Sam.
-Andrew
^ permalink raw reply
* [ANNOUNCE] Git 1.6.6.rc4
From: Junio C Hamano @ 2009-12-20 22:59 UTC (permalink / raw)
To: git
A release candidate Git 1.6.6.rc4 is available at the usual places
for final testing:
http://www.kernel.org/pub/software/scm/git/
git-1.6.6.rc4.tar.{gz,bz2} (source tarball)
git-htmldocs-1.6.6.rc4.tar.{gz,bz2} (preformatted docs)
git-manpages-1.6.6.rc4.tar.{gz,bz2} (preformatted docs)
The RPM binary packages for a few architectures are found in:
testing/git-*-1.6.6.rc4-1.fc11.$arch.rpm (RPM)
Hopefully I'll do the final this Wednesday to make 1.6.6 a holiday gift to
everybody.
I would very much prefer news outlets like kernelpodcast.org and lwn.net
_not_ to say "... is released; it comes with many _fixes_". Fixes to
released versions have indeed been included in the 'master' branch, but
they all appear in the maintenance release. The main _point_ of using a
new feature release like 1.6.6 is to get new _features_, so it is more
appropriate to say "it comes with many new features."
Thanks for all contributors who have worked hard to whip this release into
shape.
----------------------------------------------------------------
Changes since v1.6.6-rc3 are as follows:
Björn Gustavsson (1):
rebase -i: abort cleanly if the editor fails to launch
Eric Wong (2):
git svn: make empty directory creation gc-aware
t9146: use 'svn_cmd' wrapper
Junio C Hamano (1):
Git 1.6.6-rc4
Stephen Boyd (2):
api-strbuf.txt: fix typos and document launch_editor()
technical-docs: document hash API
^ permalink raw reply
* Re: [RFC PATCH] Record a single transaction for conflicting push operations
From: Catalin Marinas @ 2009-12-20 23:21 UTC (permalink / raw)
To: Karl Wiberg; +Cc: git, Gustav Hållberg
In-Reply-To: <b8197bcb0912191550u300a9c20o351eba66c85292bb@mail.gmail.com>
2009/12/19 Karl Wiberg <kha@treskal.com>:
> On Fri, Dec 18, 2009 at 4:49 PM, Catalin Marinas
> <catalin.marinas@gmail.com> wrote:
>
>> @@ -371,12 +369,10 @@ class StackTransaction(object):
>> # We've just caused conflicts, so we must allow them in
>> # the final checkout.
>> self.__allow_conflicts = lambda trans: True
>> -
>> - # Save this update so that we can run it a little later.
>> - self.__conflicting_push = update
>> + self.__patches = _TransPatchMap(self.__stack)
>> + update()
>> self.__halt("%d merge conflict(s)" % len(self.__conflicts))
>> else:
>> - # Update immediately.
>> update()
>>
>> def push_tree(self, pn):
>
> Better. But couldn't you remove the update function completely and
> just inline the code in it, since it's called immediately?
Of course, I tried, but couldn't get it to work. I get HEAD and top
not equal unless I call update() between _TransPatchMap and
self.__halt(). For the non-conflicting case we need to call update
before or after this "if merge_conflict".
One solution is to split the "if merge_conflict" in two but maybe you
have a better idea.
Thanks,
--
Catalin
^ permalink raw reply
* possible code error at run_command.c
From: Frank Li @ 2009-12-21 6:46 UTC (permalink / raw)
To: git
int start_command(struct child_process *cmd)
{
int need_in, need_out, need_err;
int fdin[2], fdout[2], fderr[2];
int failed_errno = failed_errno;
I have not found failed_errno as global variable.
failed_errno = failed_errno means nothing.
It is possible coding error and should be
int failed_errno= 0 or
failed_errno=errno.
^ permalink raw reply
* Re: [RFC PATCH] Record a single transaction for conflicting push operations
From: Karl Wiberg @ 2009-12-21 7:08 UTC (permalink / raw)
To: Catalin Marinas; +Cc: git, Gustav Hållberg
In-Reply-To: <b0943d9e0912201521k73bdcb5fl333e845028954050@mail.gmail.com>
On Mon, Dec 21, 2009 at 12:21 AM, Catalin Marinas
<catalin.marinas@gmail.com> wrote:
> 2009/12/19 Karl Wiberg <kha@treskal.com>:
>
>> Better. But couldn't you remove the update function completely and
>> just inline the code in it, since it's called immediately?
>
> Of course, I tried, but couldn't get it to work. I get HEAD and top
> not equal unless I call update() between _TransPatchMap and
> self.__halt(). For the non-conflicting case we need to call update
> before or after this "if merge_conflict".
>
> One solution is to split the "if merge_conflict" in two but maybe
> you have a better idea.
Yes, duplicating the conditional was what I had in mind. But if you
don't find it to improve the readability of the code (as compared to
having a function), I certainly won't insist.
Thanks for working on this.
By the way, you do realize there's another command that requires two
steps to undo completely: refresh? And that one is harder to get out
of---undoing it all in one step would mean throwing away the updates
to the patch.
--
Karl Wiberg, kha@treskal.com
subrabbit.wordpress.com
www.treskal.com/kalle
^ permalink raw reply
* Re: possible code error at run_command.c
From: Johannes Sixt @ 2009-12-21 7:18 UTC (permalink / raw)
To: Frank Li; +Cc: git
In-Reply-To: <1976ea660912202246k45732bf2p111bbeb78047693e@mail.gmail.com>
Frank Li schrieb:
> int start_command(struct child_process *cmd)
> {
> int need_in, need_out, need_err;
> int fdin[2], fdout[2], fderr[2];
> int failed_errno = failed_errno;
>
> I have not found failed_errno as global variable.
> failed_errno = failed_errno means nothing.
This is a commonly used idiom to avoid an (incorrect) compiler warning
about an uninitialized variable.
Strictly speaking, I think that you are right by saying "means nothing"
because the use of the uninitialized variable invokes undefined behavior
(and for this reason, I dislike this construct), but in practice it will
not make a difference.
-- Hannes
^ permalink raw reply
* Re: possible code error at run_command.c
From: Wincent Colaiuta @ 2009-12-21 7:09 UTC (permalink / raw)
To: Frank Li; +Cc: git
In-Reply-To: <1976ea660912202246k45732bf2p111bbeb78047693e@mail.gmail.com>
El 21/12/2009, a las 07:46, Frank Li escribió:
> int start_command(struct child_process *cmd)
> {
> int need_in, need_out, need_err;
> int fdin[2], fdout[2], fderr[2];
> int failed_errno = failed_errno;
>
> I have not found failed_errno as global variable.
> failed_errno = failed_errno means nothing.
>
> It is possible coding error and should be
> int failed_errno= 0 or
> failed_errno=errno.
Via "git blame":
commit 5a7a3671b74c043216549b94a718da04cc3ffcd6
Author: David Soria Parra <dsp@php.net>
Date: Tue Aug 4 11:28:40 2009 +0200
run-command.c: squelch a "use before assignment" warning
i686-apple-darwin9-gcc-4.0.1 (GCC) 4.0.1 (Apple Inc. build 5490)
compiler
(and probably others) mistakenly thinks variable failed_errno is
used
before assigned. Work it around by giving it a fake
initialization.
Signed-off-by: David Soria Parra <dsp@php.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Cheers,
Wincent
^ permalink raw reply
* Re: possible code error at run_command.c
From: Frank Li @ 2009-12-21 7:29 UTC (permalink / raw)
To: Johannes Sixt; +Cc: git
In-Reply-To: <4B2F214D.2020905@viscovery.net>
> This is a commonly used idiom to avoid an (incorrect) compiler warning
> about an uninitialized variable.
>
> Strictly speaking, I think that you are right by saying "means nothing"
> because the use of the uninitialized variable invokes undefined behavior
> (and for this reason, I dislike this construct), but in practice it will
> not make a difference.
>
This error is captured at MSVC environment by run time varible check.
I prefer change it to
int failed_errno = errno;
^ permalink raw reply
* Re: possible code error at run_command.c
From: Johannes Sixt @ 2009-12-21 7:43 UTC (permalink / raw)
To: Frank Li; +Cc: git
In-Reply-To: <1976ea660912202329x42f6add3j9175867e8723a4bd@mail.gmail.com>
Frank Li schrieb:
>> This is a commonly used idiom to avoid an (incorrect) compiler warning
>> about an uninitialized variable.
>>
>> Strictly speaking, I think that you are right by saying "means nothing"
>> because the use of the uninitialized variable invokes undefined behavior
>> (and for this reason, I dislike this construct), but in practice it will
>> not make a difference.
>>
>
> This error is captured at MSVC environment by run time varible check.
Disable this check - it just takes away performance. :-)
(If you don't disable the check, then keep the required changes private.)
> I prefer change it to
> int failed_errno = errno;
You don't need to initialize the variable at all because it is always
initialized elsewhere before it is used. Perhaps MSVC is clever enough to
see it?
-- Hannes
^ permalink raw reply
* Re: [PATCH] rebase -i: print the editor name if it fails to launch
From: Björn Gustavsson @ 2009-12-21 7:54 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7v4onlbi3q.fsf@alter.siamese.dyndns.org>
2009/12/20 Junio C Hamano <gitster@pobox.com>:
> Isn't this too elaborate? git_editor() has already run and when it
> attempted to launch the editor it assigned to GIT_EDITOR in order to use
> it as an eval string.
I wanted to hide the details about the inner working of git_editor(),
but since it is only called in one place, yes, it is too elaborate.
>
> git_editor "$TODO" ||
> die_abort "Failed to run '${GIT_EDITOR:-your editor}'"
>
> would suffice, no?
Yes. Much better. I will rewrite my patch when I'll get home from
work later today.
--
Björn Gustavsson, Erlang/OTP, Ericsson AB
^ permalink raw reply
* Re: possible code error at run_command.c
From: Frank Li @ 2009-12-21 8:18 UTC (permalink / raw)
To: Johannes Sixt; +Cc: git
In-Reply-To: <4B2F2727.4060405@viscovery.net>
>
> Disable this check - it just takes away performance. :-)
Release version will disable this.
Debug version enable this by default is just for find out code error.
>
> (If you don't disable the check, then keep the required changes private.)
>
>> I prefer change it to
>> int failed_errno = errno;
>
> You don't need to initialize the variable at all because it is always
> initialized elsewhere before it is used. Perhaps MSVC is clever enough to
> see it?
>
Maybe some excute path miss initialized it. Otherwise compiler will
not report warning.
^ permalink raw reply
* Re: possible code error at run_command.c
From: Johannes Sixt @ 2009-12-21 8:30 UTC (permalink / raw)
To: Frank Li; +Cc: git
In-Reply-To: <1976ea660912210018y15acfe32o78841d5e0968f793@mail.gmail.com>
Frank Li schrieb:
> Maybe some excute path miss initialized it. Otherwise compiler will
> not report warning.
LOOK AT THE CODE. There is no such code path.
-- Hannes
^ permalink raw reply
* Re: possible code error at run_command.c
From: Erik Faye-Lund @ 2009-12-21 8:41 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Frank Li, git
In-Reply-To: <4B2F320A.6010908@viscovery.net>
On Mon, Dec 21, 2009 at 9:30 AM, Johannes Sixt <j.sixt@viscovery.net> wrote:
> Frank Li schrieb:
>> Maybe some excute path miss initialized it. Otherwise compiler will
>> not report warning.
>
> LOOK AT THE CODE. There is no such code path.
>
That's odd. I agree, there isn't such a code path. But this is the
first time I've ever seen this MSVC-feature turn up false positives,
which puzzles me.
Perhaps Frank has had to modify start_command for MSVC, and somehow
introduced such an error? I dunno.
--
Erik "kusma" Faye-Lund
^ permalink raw reply
* Re: possible code error at run_command.c
From: Johannes Sixt @ 2009-12-21 8:52 UTC (permalink / raw)
To: kusmabite; +Cc: Frank Li, git
In-Reply-To: <40aa078e0912210041g1fd94c77g6cf9f1b236b6ecd7@mail.gmail.com>
Erik Faye-Lund schrieb:
> On Mon, Dec 21, 2009 at 9:30 AM, Johannes Sixt <j.sixt@viscovery.net> wrote:
>> Frank Li schrieb:
>>> Maybe some excute path miss initialized it. Otherwise compiler will
>>> not report warning.
>> LOOK AT THE CODE. There is no such code path.
>>
>
> That's odd.
Only if Frank did the homework and removed the initialization from
int failed_errno = failed_errno;
> I agree, there isn't such a code path. But this is the
> first time I've ever seen this MSVC-feature turn up false positives,
> which puzzles me.
This line will trigger the check. It initializes failed_errno with itself,
which is still uninitialized at this time.
Note that we have more definitions of this kind:
$ git grep -E ' ([a-zA-Z_][a-zA-Z_0-9]*) = \1[;,]' *.c
builtin-rev-list.c: int reaches = reaches, all = all;
match-trees.c: unsigned mode1 = mode1;
match-trees.c: unsigned mode2 = mode2;
run-command.c: int failed_errno = failed_errno;
transport.c: int cmp = cmp, len;
wt-status.c: int status = status;
-- Hannes
^ permalink raw reply
* Re: possible code error at run_command.c
From: Erik Faye-Lund @ 2009-12-21 9:08 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Frank Li, git
In-Reply-To: <4B2F3732.6030903@viscovery.net>
On Mon, Dec 21, 2009 at 9:52 AM, Johannes Sixt <j.sixt@viscovery.net> wrote:
> Erik Faye-Lund schrieb:
>> On Mon, Dec 21, 2009 at 9:30 AM, Johannes Sixt <j.sixt@viscovery.net> wrote:
>>> Frank Li schrieb:
>>>> Maybe some excute path miss initialized it. Otherwise compiler will
>>>> not report warning.
>>> LOOK AT THE CODE. There is no such code path.
>>>
>>
>> That's odd.
>
> Only if Frank did the homework and removed the initialization from
>
> int failed_errno = failed_errno;
>
>> I agree, there isn't such a code path. But this is the
>> first time I've ever seen this MSVC-feature turn up false positives,
>> which puzzles me.
>
> This line will trigger the check. It initializes failed_errno with itself,
> which is still uninitialized at this time.
>
> Note that we have more definitions of this kind:
>
> $ git grep -E ' ([a-zA-Z_][a-zA-Z_0-9]*) = \1[;,]' *.c
> builtin-rev-list.c: int reaches = reaches, all = all;
> match-trees.c: unsigned mode1 = mode1;
> match-trees.c: unsigned mode2 = mode2;
> run-command.c: int failed_errno = failed_errno;
> transport.c: int cmp = cmp, len;
> wt-status.c: int status = status;
>
> -- Hannes
>
>
Right, I didn't think of that. Since that is the case, I'd say
disabling run-time checks is perfectly sane.
That being said, it might make sense for MSVC people to be able to
have this feature turned on so they can easier find REAL
programmer-errors. But I guess removing the self-assignments makes
trouble for non-MSVC people.
Perhaps some of these warnings could be avoided in a "safer" way? IMO,
assigning variables to themselves like this is a bit fishy, as it
effectively hides uninitialized-use warnings for the entire lifetime
of that variable. Sure, the programmer who did that probably knows
what he or she does -- but they can't possibly know what people will
do in the future. If we could somehow turn these (really few)
occasions into something that produces reliable warnings on
uninitialized variables, I think we might be able to catch some bugs
earlier.
--
Erik "kusma" Faye-Lund
^ permalink raw reply
* Re: possible code error at run_command.c
From: Frank Li @ 2009-12-21 9:13 UTC (permalink / raw)
To: Johannes Sixt; +Cc: kusmabite, git
In-Reply-To: <4B2F3732.6030903@viscovery.net>
> This line will trigger the check. It initializes failed_errno with itself,
> which is still uninitialized at this time.
>
I see.
I always use release version at finial production.
This is not important. I read code again. There are not path.
Thank you take care.
^ 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