* Re: Git in a Nutshell guide
From: jhud7196 @ 2007-11-22 16:37 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git
In-Reply-To: <200711212232.33791.jnareb@gmail.com>
> Dnia środa 21. listopada 2007 20:45, Jan Hudec napisał:
>> On Wed, Nov 21, 2007 at 00:57:28 +0100, Jakub Narebski wrote:
>
>>> 2. Git has some historical cruft, including the fact that it began as
>>> series of low level tools (plumbing) and became proper SCM
>>> (acquired
>>> proper porcelain) later, what can be even now seen in
>>> documentation...
>
> What I meant here that some of important documentation can be only
> found in plumbing commands man pages.
Yes, that's true. And does indeed make /writing/ the book harder, because
the information is harder to compile. Fortunately it should not make it
so much harder to read.
>>> 3. Explanation of some features (like object model) would be much
>>> easier
>>> with some graphics (diagrams etc.), but chosen documentation
>>> format,
>>> AsciiDoc, doesn't make it easy...
>>
>> I dare to disagree here. Asciidoc supports generating image tags for
>> respective output formats and it really does not look hard. We could
>> have
>> PNGs and if we wanted higher press quality even SVG or EPS and convert
>> them
>> to PNGs for the HTML version (and use EPS for latex output).
>
> The problem is that 1) language used to generate images must be easy
> to understand and readable in text form (so sources are enough to
> understand); 2) one of the main formats is manpage (although that would
> not matter for "Git Guide" / "The Git Book").
Ad 1), I don't think we have to generate the images. For most images
it is easier to just draw them in a vector image editor.
As you say, 2) does not matter for "Git Guide"/"The Git Book". The man
pages will have to do without graphics or with Ascii-art replacements.
> Should we choose PIC? Or perhaps DOT? I can write little MetaPost, but
> I don't think that would be good format for git diagrams, even if it
> can be converted to PDF, SVG and PNG.
I'd use DOT for things that can be done with it (ie. visualizing
histories) and just hand-draw the rest in SVG (prefered) or PNG
(for illustrations).
--
Jan Hudec <bulb@ucw.cz>
^ permalink raw reply
* Re: Git in a Nutshell guide
From: jhud7196 @ 2007-11-22 16:57 UTC (permalink / raw)
To: Jonas Juselius; +Cc: Jakub Narebski, git
In-Reply-To: <1195737303.19260.27.camel@localhost>
> I'm planning to write a section in the "Nutshell guide" on rewriting
> history and rebasing. I have a question related to rewriting history. As
> usual, I'll assume that the part of the history I'm mucking with has not
> been pushed or pulled by anyone.
> Suppose I have been working on some topic branch for a while and been
> overly trigger happy, i.e. I have produced a ridiculous number of
> commits along the way. At some point when I'm done I want to publish my
> changes, but doing so would create an insanely obese history full of
> near nonsense commits. What I want to do is to slim down the commit log
> into pieces that actually makes sense. What is the preferred (or best,
> most convenient) way of doing this? The way I have done this previously
> is essentially:
>
> 1. git branch -m mytopic tmp_mytopic # rename
> 2. git branch mytopci tmp_mytpoic~42 # go back in history
> Loop:
> 3.1 git log; git diff; git annotate...
> 3.2 git diff tmp_mytopic~42..tmp_mytopic~33 | git-apply
> 3.3 git commit -m "sane commit message" -a
> 4. git branch -d tmp_mytopic
>
> If I need to reorder commits I can first use git-rebase -i to get
> everything streamlined. There must be a better way of doing this, right?
IIRC rebase -i is also capable of merging the commits. So you can use it
for the whole thing.
--
Jan Hudec <bulb@ucw.cz>
^ permalink raw reply
* Re: [PATCH 3/3] Replace setenv(GIT_DIR_ENVIRONMENT, ...) with set_git_dir()
From: Steffen Prohaska @ 2007-11-22 17:56 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Junio C Hamano, Johannes Schindelin, git, Dmitry Kakurin
In-Reply-To: <474552D6.5060305@viscovery.net>
On Nov 22, 2007, at 10:58 AM, Johannes Sixt wrote:
> Steffen Prohaska schrieb:
>> On Nov 22, 2007, at 8:52 AM, Junio C Hamano wrote:
>>> I suspect that if there are even earlier callers than these
>>> early parts in the codepaths (handle_options, enter_repo, and
>>> setup_git_directory_gently), maybe these earlier callers are
>>> doing something wrong. Logically, if you are somewhere very
>>> early in the codepath that you can still change the value of
>>> GIT_DIR, you shouldn't have assumed the unknown value of GIT_DIR
>>> and cached locations relative to that directory, no? What are
>>> the problematic callers? What values do they access and why?
>> I thought about these questions, too. But only very briefly.
>> I did not analyze the code path that lead to calls of getenv().
>> I'm not sure if it's really necessary. Calling set_git_dir()
>> looks more sensible too me than the old code. I believe using
>> set_git_dir() is the safer choice, and should not do any harm.
>> So I stopped analyzing too much, and instead proposed to use
>> set_git_dir().
>
> Junio's point is this: If we stumble over a dangling pointer that
> getenv() produced, then this has obviously happened before setenv
> (GIT_DIR), and caching that pointer is probably the wrong thing to
> do anyway (because it refers to the wrong GIT_DIR) and needs to be
> fixed.
I see your point. It is probably more important to investigate
this than I recognized at a first glance.
> So the task is to find those traps. Dmitry obviously stumbled over
> one case, but I haven't ever encountered any problems with the
> current code. But then this might be sheer luck. And I'm not a
> heavy user of export GIT_DIR=foo, either. Do *you* know a
> problematic case?
No. I only stumbled over the code, when I reviewed differences
between msysgit and mingw. I rarely use GIT_DIR=foo. Actually,
I can't remember the last time I did.
>> Interesting, though, is to find out if we have other potentially
>> dangerous calls to getenv() that are not removed by this patch.
>
> Side note for other readers: This is a Windows specific problem for
> the moment because its getenv() does not behave well.
Yes, and apparently even nobody knows how to trigger the problem
on Windows. At this point, we only know that caching getenv()
calls is unsafe, while on Unix it is safe (at least for BSD
it's documented to be safe).
Steffen
^ permalink raw reply
* Re: Adding push configuration to .git/config
From: Steffen Prohaska @ 2007-11-22 18:22 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Junio C Hamano, Nico -telmich- Schottelius, git
In-Reply-To: <Pine.LNX.4.64.0711221120300.27959@racer.site>
On Nov 22, 2007, at 12:23 PM, Johannes Schindelin wrote:
> I have to say that I slowly grow an antipathy for "git push" without
> parameters. _All_ of the confusions with push that I saw stem from
> being
> too lazy to say where and what you want to push. (Okay, there is this
> other thing where people say "git push origin master:master" and I
> still
> do not know where they got _that_ from.)
I don't agree. "git push" should support good defaults. At
some point you make the decision to publish, and you use
"git push" for this. If you have a reasonably stable environment
there will be a good default, what publishing means.
Obviously users should think before they "git push". They know
that they are publishing, and they (hopefully) understand
that they can't easily undo this operation. But I believe
people who used any version control system before know that.
It's the same for every system. If you publish, others can
get your source, and this can't easily be reverted.
But people are lazy, and having a good default is what they
expect from a good tool. The default should be safe, and
reliably do something useful. A good default can safe you
from typing errors. But the default should not do too much,
and should not do unexpected things; at least not until you
explicitly configured.
You know that I believe the current default is not such a
choice. Pushing matching branches sooner or later triggers
annoying errors if used with a shared remote repository.
But this is how the default is; and I'll live with it.
At least it saves me time. I learnt to ignore the error
messages. And we now have '--dry-run'. So I can quickly
check what will happen.
But, I strongly believe that a tool that expects users to
type the current branch and the default remote each time;
just for publishing some changes to the default location,
is not what users want.
Steffen
^ permalink raw reply
* Re: [PATCH v2] builtin-commit: Include the diff in the commit message when verbose.
From: Junio C Hamano @ 2007-11-22 19:14 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Kristian Høgsberg, gitster, git
In-Reply-To: <Pine.LNX.4.64.0711221049350.27959@racer.site>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> On Wed, 21 Nov 2007, Kristian Høgsberg wrote:
>
>> +
>> + /* Truncate the message just before the diff, if any. */
>> + p = strstr(sb.buf, "\ndiff --git a/");
>> + if (p != NULL)
>> + strbuf_setlen(&sb, p - sb.buf);
>> +
>
> Is this related to the change in wt_status? If so, wouldn't we want to
> suppress the diff, instead of generating it, and then killing it later?
This corresponds to the sed script near l.545 in git-commit.sh.
I've been wondering if it would be better not to have this logic
but instead "git commit -v" to show the diff text prefixed with
'# ' to make it a proper comment, by the way.
> Besides, you'd want to leave the \n there: strbuf_setlen(&sb, p + 1 -
> sb.buf);
Yup.
>> + /* Sigh, the entire diff machinery is hardcoded to output to
>> + * stdout. Do the dup-dance...*/
>
> I wonder how much effort it would be to change that. Not that it would
> help too much, since we want the output in a strbuf anyway.
The codepath is preparing COMMIT_EDITMSG file for the user to
edit (or to the standard output, if this is "git status -v").
I do not know about "strbuf" part; resulting patch text could be
large and in most cases the callers would rather flush it to
outside (either pipe or file) as soon as possible than having to
keep it all in core.
I was trying to do the stdout -> FILE *fp conversion of diff.c
last night but dropped it halfway, after finding one puts()
misconversion and fixing it. The changes should mostly be
straightforward but the result felt ugly.
^ permalink raw reply
* [PATCH] git-svn: Remove unnecessary Git::SVN::Util package
From: David D. Kilzer @ 2007-11-22 19:18 UTC (permalink / raw)
To: ask, Eric Wong; +Cc: git, David D. Kilzer
In-Reply-To: <2CB65B86-3BEF-46C2-86F8-EFAB2AE6D353@develooper.com>
Digest::MD5 is loaded regardless of the package in which it's
declared, so move its 'use' statement and the md5sum() function
into the main package.
Signed-off-by: David D. Kilzer <ddkilzer@kilzer.net>
---
Ask Bjørn Hansen <ask@develooper.com> wrote:
> On Nov 21, 2007, at 11:57, David D. Kilzer wrote:
> > Created new Git::SVN::Util package with an md5sum() function. A
> > new package was created so that Digest::MD5 did not have to be
> > loaded in the main package.
> Huh? It's all in the same file anyway, so what difference does it
> make?
None!
git-svn.perl | 23 +++++++++--------------
1 files changed, 9 insertions(+), 14 deletions(-)
diff --git a/git-svn.perl b/git-svn.perl
index 62801c8..17d3020 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -35,6 +35,7 @@ push @Git::SVN::Ra::ISA, 'SVN::Ra';
push @SVN::Git::Editor::ISA, 'SVN::Delta::Editor';
push @SVN::Git::Fetcher::ISA, 'SVN::Delta::Editor';
use Carp qw/croak/;
+use Digest::MD5;
use IO::File qw//;
use File::Basename qw/dirname basename/;
use File::Path qw/mkpath/;
@@ -48,8 +49,7 @@ BEGIN {
foreach (qw/command command_oneline command_noisy command_output_pipe
command_input_pipe command_close_pipe/) {
for my $package ( qw(SVN::Git::Editor SVN::Git::Fetcher
- Git::SVN::Migration Git::SVN::Log Git::SVN
- Git::SVN::Util),
+ Git::SVN::Migration Git::SVN::Log Git::SVN),
__PACKAGE__) {
*{"${package}::$_"} = \&{"Git::$_"};
}
@@ -835,19 +835,19 @@ sub cmd_info {
command_output_pipe(qw(cat-file blob), "HEAD:$path");
if ($file_type eq "link") {
my $file_name = <$fh>;
- $checksum = Git::SVN::Util::md5sum("link $file_name");
+ $checksum = md5sum("link $file_name");
} else {
- $checksum = Git::SVN::Util::md5sum($fh);
+ $checksum = md5sum($fh);
}
command_close_pipe($fh, $ctx);
} elsif ($file_type eq "link") {
my $file_name =
command(qw(cat-file blob), "HEAD:$path");
$checksum =
- Git::SVN::Util::md5sum("link " . $file_name);
+ md5sum("link " . $file_name);
} else {
open FILE, "<", $path or die $!;
- $checksum = Git::SVN::Util::md5sum(\*FILE);
+ $checksum = md5sum(\*FILE);
close FILE or die $!;
}
$result .= "Checksum: " . $checksum . "\n";
@@ -1187,11 +1187,6 @@ sub find_file_type_and_diff_status {
return ("file", $diff_status);
}
-package Git::SVN::Util;
-use strict;
-use warnings;
-use Digest::MD5;
-
sub md5sum {
my $arg = shift;
my $ref = ref $arg;
@@ -2926,7 +2921,7 @@ sub apply_textdelta {
if (defined $exp) {
seek $base, 0, 0 or croak $!;
- my $got = Git::SVN::Util::md5sum($base);
+ my $got = ::md5sum($base);
die "Checksum mismatch: $fb->{path} $fb->{blob}\n",
"expected: $exp\n",
" got: $got\n" if ($got ne $exp);
@@ -2945,7 +2940,7 @@ sub close_file {
if (my $fh = $fb->{fh}) {
if (defined $exp) {
seek($fh, 0, 0) or croak $!;
- my $got = Git::SVN::Util::md5sum($fh);
+ my $got = ::md5sum($fh);
if ($got ne $exp) {
die "Checksum mismatch: $path\n",
"expected: $exp\n got: $got\n";
@@ -3300,7 +3295,7 @@ sub chg_file {
$fh->flush == 0 or croak $!;
seek $fh, 0, 0 or croak $!;
- my $exp = Git::SVN::Util::md5sum($fh);
+ my $exp = ::md5sum($fh);
seek $fh, 0, 0 or croak $!;
my $pool = SVN::Pool->new;
--
1.5.3.4
^ permalink raw reply related
* Re: [PATCH 0/5] Colors for git-add--interactive
From: Junio C Hamano @ 2007-11-22 19:20 UTC (permalink / raw)
To: Dan Zwell
Cc: Jeff King, Shawn O. Pearce, Wincent Colaiuta, Git Mailing List,
Jonathan del Strother, Johannes Schindelin, Frank Lichtenheld,
Jakub Narebski
In-Reply-To: <20071122045437.46ee4638@paradox.zwell.net>
Dan Zwell <dzwell@zwell.net> writes:
> My impression is that diff --cc is called any time
> that HEAD has two parents. Is this correct?
You get combined output when your index is unmerged.
Showing combined output to the user to examine may make sense,
but I think you would want to have the user pick from diff
between stage#2 and the work tree for an unmerged entry, if you
allow to pick hunks during a conflicted merge.
^ permalink raw reply
* Re: Reordering lines in "git-rebase -i" task sheet
From: David Brown @ 2007-11-22 19:25 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin, git
In-Reply-To: <7vlk8q7hzg.fsf@gitster.siamese.dyndns.org>
On Thu, Nov 22, 2007 at 01:21:55AM -0800, Junio C Hamano wrote:
>That is the reason why git commit leaves the first line empty in
>its initial template. It is to allow the user to immediately
>start typing.
Unless of course your editor tries to be "helpful" and remember what line
you were on previously in a file.
David
^ permalink raw reply
* Re: [PATCH 2/4] Teach git-add--interactive to accept a file path to patch
From: Junio C Hamano @ 2007-11-22 19:29 UTC (permalink / raw)
To: Wincent Colaiuta; +Cc: Jeff King, git
In-Reply-To: <FDAFAD8C-8FF5-4DD7-AC5E-BA39790281A4@wincent.com>
Wincent Colaiuta <win@wincent.com> writes:
> El 22/11/2007, a las 12:29, Junio C Hamano escribió:
>
>> By the way, the arguments on the command line to git commands
>> after "--" are generally pathspecs, iow, patterns to specify
>> groups of files. Please do not introduce unnecessary
>> inconsistencies to the UI by requiring them to be exact pathname
>> only in this particular mode of the command and nowhere else.
>
> Well, I it wasn't my intention to introduce any such requirement. The
> path parameters get passed in and eventually handed over unmodified to:
>
> git diff-files -p --
What I was referring to was this in your original:
>> + my @tracked = grep {
>> + defined run_cmd_pipe(qw(git ls-files
>> + --exclude-standard --), $_)
>> + } @ARGV;
It picks elements from @ARGV that ls-files says "Ok" back, not
the expanded result from ls-files, and @tracked elements are
used later as filenames. Which means you are expecting @ARGV to
contain only concrete filenames, not pathspec.
> Or if we add some kind of "--patch" switch, instead do:
>
> git add -i --patch foo
You do not need both "-i" and "--patch", do you? Plain git-add
does not take --patch anyway so it can pass --patch to the
underlying interactive one.
^ permalink raw reply
* Re: [PATCH] Avoid recalculating filename string pointer.
From: Mike Hommey @ 2007-11-22 19:54 UTC (permalink / raw)
To: André Goddard Rosa; +Cc: gitster, Git Mailing List
In-Reply-To: <b8bf37780711211659i65a99493te3e3d5cee008ae7d@mail.gmail.com>
On Wed, Nov 21, 2007 at 10:59:41PM -0200, André Goddard Rosa wrote:
> --- a/fast-import.c
> +++ b/fast-import.c
> @@ -2304,11 +2304,13 @@ int main(int argc, const char **argv)
> else if (!prefixcmp(a, "--export-marks="))
> mark_file = a + 15;
> else if (!prefixcmp(a, "--export-pack-edges=")) {
> + char *filename = a + 20;
> +
> if (pack_edges)
> fclose(pack_edges);
> - pack_edges = fopen(a + 20, "a");
> + pack_edges = fopen(filename, "a");
> if (!pack_edges)
> - die("Cannot open %s: %s", a + 20, strerror(errno));
> + die("Cannot open %s: %s", filename, strerror(errno));
> } else if (!strcmp(a, "--force"))
> force_update = 1;
> else if (!strcmp(a, "--quiet"))
Normally, the compiler takes care of such optimizations. It actually
takes care of it much better than you can do yourself, and doing it
yourself can even sometimes generate less optimized code because it
gets in the compiler optimizations'way.
Mike
^ permalink raw reply
* Re: [PATCH] Simplify the code and avoid an attribution.
From: Mike Hommey @ 2007-11-22 20:01 UTC (permalink / raw)
To: André Goddard Rosa; +Cc: gitster, Git Mailing List
In-Reply-To: <b8bf37780711211700m1aa32006t6302f134f9c3fb7e@mail.gmail.com>
On Wed, Nov 21, 2007 at 11:00:02PM -0200, André Goddard Rosa wrote:
> --- a/config.c
> +++ b/config.c
> @@ -447,15 +447,16 @@ int git_config_from_file(config_fn_t fn, const
> char *filename)
> int ret;
> FILE *f = fopen(filename, "r");
>
> - ret = -1;
> - if (f) {
> - config_file = f;
> - config_file_name = filename;
> - config_linenr = 1;
> - ret = git_parse_file(fn);
> - fclose(f);
> - config_file_name = NULL;
> - }
> + if (!f)
> + return -1;
> +
> + config_file = f;
> + config_file_name = filename;
> + config_linenr = 1;
> + ret = git_parse_file(fn);
> + fclose(f);
> + config_file_name = NULL;
> +
> return ret;
> }
Actually, since it is more likely that the file has been opened, the
original code is more optimal because it doesn't generate a jump in most
cases. And if you're worried about the ret variable, don't worry, it's
most likely stripped out by the compiler optimizations.
> - if (f == NULL)
> + if (!f)
> - if ((f = fopen(filename, "rb")) == NULL)
> + if (!(f = fopen(filename, "rb")))
It's a matter of taste
Mike
^ permalink raw reply
* [PATCH] Converted git-merge-ours.sh -> builtin-merge-ours.c
From: Thomas Harning @ 2007-11-22 20:19 UTC (permalink / raw)
To: gitster; +Cc: git, thomas.harning
Here's a simple patch to make git-merge-ours.sh into a builtin.
I figure this would be a simple way of getting in the git-development flow.
Signed-off-by: Thomas Harning Jr <harningt@gmail.com>
---
Makefile | 3 ++-
builtin-merge-ours.c | 32 ++++++++++++++++++++++++++++++++
builtin.h | 1 +
git-merge-ours.sh | 14 --------------
git.c | 1 +
5 files changed, 36 insertions(+), 15 deletions(-)
create mode 100644 builtin-merge-ours.c
delete mode 100755 git-merge-ours.sh
diff --git a/Makefile b/Makefile
index cabde81..7a0ee78 100644
--- a/Makefile
+++ b/Makefile
@@ -221,7 +221,7 @@ SCRIPT_SH = \
git-sh-setup.sh \
git-am.sh \
git-merge.sh git-merge-stupid.sh git-merge-octopus.sh \
- git-merge-resolve.sh git-merge-ours.sh \
+ git-merge-resolve.sh \
git-lost-found.sh git-quiltimport.sh git-submodule.sh \
git-filter-branch.sh \
git-stash.sh
@@ -353,6 +353,7 @@ BUILTIN_OBJS = \
builtin-mailsplit.o \
builtin-merge-base.o \
builtin-merge-file.o \
+ builtin-merge-ours.o \
builtin-mv.o \
builtin-name-rev.o \
builtin-pack-objects.o \
diff --git a/builtin-merge-ours.c b/builtin-merge-ours.c
new file mode 100644
index 0000000..fbfe183
--- /dev/null
+++ b/builtin-merge-ours.c
@@ -0,0 +1,32 @@
+/*
+ * Implementation of git-merge-ours.sh as builtin
+ *
+ * Copyright (c) 2007 Thomas Harning Jr
+ * Original:
+ * Original Copyright (c) 2005 Junio C Hamano
+ *
+ * Pretend we resolved the heads, but declare our tree trumps everybody else.
+ */
+#include "git-compat-util.h"
+#include "builtin.h"
+
+int cmd_merge_ours(int argc, const char **argv, const char *prefix)
+{
+ const char *nargv[] = {
+ "diff-index",
+ "--quiet",
+ "--cached",
+ "HEAD",
+ NULL
+ };
+ int i;
+
+ int ret = cmd_diff_index(4, nargv, prefix);
+ printf("GOT: %i\n", ret);
+ /* We need to exit with 2 if the index does not match our HEAD tree,
+ * because the current index is what we will be committing as the
+ * merge result.
+ */
+ if(ret) ret = 2;
+ return ret;
+}
diff --git a/builtin.h b/builtin.h
index 9a6213a..bcb54aa 100644
--- a/builtin.h
+++ b/builtin.h
@@ -51,6 +51,7 @@ extern int cmd_ls_tree(int argc, const char **argv, const char *prefix);
extern int cmd_mailinfo(int argc, const char **argv, const char *prefix);
extern int cmd_mailsplit(int argc, const char **argv, const char *prefix);
extern int cmd_merge_base(int argc, const char **argv, const char *prefix);
+extern int cmd_merge_ours(int argc, const char **argv, const char *prefix);
extern int cmd_merge_file(int argc, const char **argv, const char *prefix);
extern int cmd_mv(int argc, const char **argv, const char *prefix);
extern int cmd_name_rev(int argc, const char **argv, const char *prefix);
diff --git a/git-merge-ours.sh b/git-merge-ours.sh
deleted file mode 100755
index c81a790..0000000
--- a/git-merge-ours.sh
+++ /dev/null
@@ -1,14 +0,0 @@
-#!/bin/sh
-#
-# Copyright (c) 2005 Junio C Hamano
-#
-# Pretend we resolved the heads, but declare our tree trumps everybody else.
-#
-
-# We need to exit with 2 if the index does not match our HEAD tree,
-# because the current index is what we will be committing as the
-# merge result.
-
-git diff-index --quiet --cached HEAD || exit 2
-
-exit 0
diff --git a/git.c b/git.c
index 7604319..80c2f14 100644
--- a/git.c
+++ b/git.c
@@ -325,6 +325,7 @@ static void handle_internal_command(int argc, const char **argv)
{ "mailsplit", cmd_mailsplit },
{ "merge-base", cmd_merge_base, RUN_SETUP },
{ "merge-file", cmd_merge_file },
+ { "merge-ours", cmd_merge_ours, RUN_SETUP },
{ "mv", cmd_mv, RUN_SETUP | NEED_WORK_TREE },
{ "name-rev", cmd_name_rev, RUN_SETUP },
{ "pack-objects", cmd_pack_objects, RUN_SETUP },
--
1.5.3.6.861.gd794
^ permalink raw reply related
* Re: [PATCH] Avoid recalculating filename string pointer.
From: Junio C Hamano @ 2007-11-22 20:21 UTC (permalink / raw)
To: Mike Hommey; +Cc: André Goddard Rosa, Git Mailing List
In-Reply-To: <20071122195457.GB19675@glandium.org>
Mike Hommey <mh@glandium.org> writes:
> On Wed, Nov 21, 2007 at 10:59:41PM -0200, André Goddard Rosa wrote:
>> --- a/fast-import.c
>> +++ b/fast-import.c
>> @@ -2304,11 +2304,13 @@ int main(int argc, const char **argv)
>> else if (!prefixcmp(a, "--export-marks="))
>> mark_file = a + 15;
>> else if (!prefixcmp(a, "--export-pack-edges=")) {
>> + char *filename = a + 20;
>> +
>> if (pack_edges)
>> fclose(pack_edges);
>> - pack_edges = fopen(a + 20, "a");
>> + pack_edges = fopen(filename, "a");
>> if (!pack_edges)
>> - die("Cannot open %s: %s", a + 20, strerror(errno));
>> + die("Cannot open %s: %s", filename, strerror(errno));
>> } else if (!strcmp(a, "--force"))
>> force_update = 1;
>> else if (!strcmp(a, "--quiet"))
>
> Normally, the compiler takes care of such optimizations. It actually
> takes care of it much better than you can do yourself, and doing it
> yourself can even sometimes generate less optimized code because it
> gets in the compiler optimizations'way.
True, but I think another point of the patch is to address the
risk of two instances of "+ 20" going out of sync if/when the
option parsing is updated.
Not that I think André meant the patch as defensive coding (the
subject suggests it was meant to be a micro-optimization), nor
this is the good way to address that risk factor (parse-options
may be a better match for it).
^ permalink raw reply
* Re: [PATCH] Simplify the code and avoid an attribution.
From: Junio C Hamano @ 2007-11-22 20:29 UTC (permalink / raw)
To: Mike Hommey; +Cc: André Goddard Rosa, Git Mailing List
In-Reply-To: <20071122200157.GC19675@glandium.org>
Mike Hommey <mh@glandium.org> writes:
> On Wed, Nov 21, 2007 at 11:00:02PM -0200, André Goddard Rosa wrote:
>> --- a/config.c
>> +++ b/config.c
>> @@ -447,15 +447,16 @@ int git_config_from_file(config_fn_t fn, const
>> char *filename)
>> int ret;
>> FILE *f = fopen(filename, "r");
>>
>> - ret = -1;
>> - if (f) {
>> - config_file = f;
>> - config_file_name = filename;
>> - config_linenr = 1;
>> - ret = git_parse_file(fn);
>> - fclose(f);
>> - config_file_name = NULL;
>> - }
>> + if (!f)
>> + return -1;
>> +
>> + config_file = f;
>> + config_file_name = filename;
>> + config_linenr = 1;
>> + ret = git_parse_file(fn);
>> + fclose(f);
>> + config_file_name = NULL;
>> +
>> return ret;
>> }
>
> Actually, since it is more likely that the file has been opened, the
> original code is more optimal because it doesn't generate a jump in most
> cases. And if you're worried about the ret variable, don't worry, it's
> most likely stripped out by the compiler optimizations.
I did not comment on this patch partly because I did not know
what "attribution" meant. I think it is a good change from
readability, not micro-optimization, point of view.
If you structure your code this way,
do this
if (there is an error)
return error code;
do that
do the rest
it is much easier to read than
do this
set error code pessimistically
if (it was successful) {
do that
update error code
do the rest
}
return error code
especially the more likely part that runs the real processing
("do that...do the rest") tends to grow over time, and even
acquire other error checks.
^ permalink raw reply
* Re: gitk's copy pasteboard doesn't persist after it quits
From: Shawn O. Pearce @ 2007-11-22 21:01 UTC (permalink / raw)
To: Jonathan del Strother; +Cc: git
In-Reply-To: <DFBFC631-01BF-4D2D-BCF3-3FC376479CB2@steelskies.com>
Jonathan del Strother <maillist@steelskies.com> wrote:
> On OS X, if I load gitk, copy a sha1, then quit, the sha1 isn't put
> into the system-wide pasteboard. It's definitely copied - I can paste
> it back into the sha1 field - but it seems to be some sort of local
> pasteboard that's specific to gitk
>
> If I switch to another app, the sha1 is stored in the pasteboard
> correctly, and I can then quit gitk and still have it available. I'm
> guessing that gitk (or Tcl/Tk) is syncing with the system-wide
> pasteboard on focus change, but not on quit.
I've seen this not just on Mac OS X but also on Windows. It seems
like Tk wipes the clipboard clean on exit, so I've just learned to
not exit a Tk application until I'm done with the clipboard...
--
Shawn.
^ permalink raw reply
* Re: [PATCH 2/5] Don't return 'undef' in case called in a vector context.
From: Junio C Hamano @ 2007-11-22 21:17 UTC (permalink / raw)
To: Dan Zwell
Cc: Git Mailing List, Jeff King, Shawn O. Pearce, Wincent Colaiuta,
Jonathan del Strother, Johannes Schindelin, Frank Lichtenheld,
Jakub Narebski
In-Reply-To: <20071122045534.435f01bb@paradox.zwell.net>
Dan Zwell <dzwell@zwell.net> writes:
> diff --git a/perl/Git.pm b/perl/Git.pm
> index dca92c8..6603762 100644
> --- a/perl/Git.pm
> +++ b/perl/Git.pm
> @@ -508,7 +508,7 @@ sub config {
> my $E = shift;
> if ($E->value() == 1) {
> # Key not found.
> - return undef;
> + return;
> } else {
> throw $E;
> }
Shouldn't the same fix made to config_bool as well?
^ permalink raw reply
* [StGIT PATCH] stg-gitk: allow passing args to gitk; add --help.
From: Yann Dirson @ 2007-11-22 21:23 UTC (permalink / raw)
To: Catalin Marinas; +Cc: git
Quite handy to further limit the commits to be shown.
---
contrib/stg-gitk | 30 +++++++++++++++++++++---------
1 files changed, 21 insertions(+), 9 deletions(-)
diff --git a/contrib/stg-gitk b/contrib/stg-gitk
index 6ddcfb1..cb264e5 100755
--- a/contrib/stg-gitk
+++ b/contrib/stg-gitk
@@ -12,25 +12,31 @@ set -e
# Copyright (c) 2007 Yann Dirson <ydirson@altern.org>
# Subject to the GNU GPL, version 2.
+helptext="Usage: $(basename $0) [--refs] [<branches>|--all] [-- <gitk args>]"
+
usage()
{
- echo "Usage: $(basename $0) [<branches>|--all]"
+ echo >&2 "$helptext"
exit 1
}
allbranches=0
refsonly=0
+branches=''
while [ "$#" -gt 0 ]; do
case "$1" in
--refs) refsonly=1 ;;
--all) allbranches=1 ;;
+ --help) echo "$helptext"; exit 0 ;;
+ --) shift; break ;;
--*) usage ;;
- *) break ;;
+ *) branches="$branches $1" ;;
esac
shift
done
+# Now any remaining stuff in $@ are additional options for gitk
-if [ $allbranches = 1 ] && [ "$#" -gt 0 ]; then
+if [ $allbranches = 1 ] && [ "$branches" != "" ]; then
usage
fi
@@ -41,11 +47,17 @@ refdirs=''
if [ $allbranches = 1 ]; then
refdirs="$GIT_DIR/refs"
else
- if [ "$#" = 0 ]; then
- set -- "$(stg branch)"
+ # default to current branch
+ if [ "$branches" == "" ]; then
+ branches="$(stg branch)"
+ fi
+ if [ "$branches" == "" ]; then
+ echo >&2 "ERROR: cannot find current branch."
+ exit 1
fi
- for b in "$@"; do
+ # expand patches for each named branch
+ for b in $branches; do
if [ -e "$GIT_DIR/refs/patches/$b" ]; then
# StGIT branch: show all patches
refdirs="$refdirs $GIT_DIR/refs/heads/$b $GIT_DIR/refs/patches/$b"
@@ -73,11 +85,11 @@ elif grep -q -- --argscmd $(which gitk); then
# This gitk supports --argscmd.
# Let's use a hack to pass --all, which was consumed during command-line parsing
if [ $allbranches = 1 ]; then
- gitk --argscmd="$0 --refs --all"
+ gitk --argscmd="$0 --refs --all" "$@"
else
- gitk --argscmd="$0 --refs $*"
+ gitk --argscmd="$0 --refs $branches" "$@"
fi
else
# This gitk does not support --argscmd, just compute refs onces
- gitk $(printrefs)
+ gitk $(printrefs) "$@"
fi
^ permalink raw reply related
* Re: [PATCH 4/5] Let git-add--interactive read colors from configuration
From: Junio C Hamano @ 2007-11-22 21:28 UTC (permalink / raw)
To: Jeff King
Cc: Dan Zwell, Git Mailing List, Shawn O. Pearce, Wincent Colaiuta,
Jonathan del Strother, Johannes Schindelin, Frank Lichtenheld,
Jakub Narebski
In-Reply-To: <20071122121836.GG12913@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Thu, Nov 22, 2007 at 04:56:06AM -0600, Dan Zwell wrote:
>
>> + # Grab the 3 main colors in git color string format, with sane
>> + # (visible) defaults:
>> + $prompt_color = Git::color_to_ansi_code(
>> + scalar $repo->config_default('color.interactive.prompt',
>> + 'bold blue'));
>
> And by the same token as the last message, given that config_* take only
> two arguments, is there a reason not to extend them so that
>
> $repo->config_bool('my.key', 0);
>
> handles the default. Then I think you could simplify this to just:
>
> $repo->config_color('color.interactive.prompt', 'bold blue');
>
> and hide the color_to_ansi_code messiness from the script altogether.
I like the config_color() method.
I think the "config_bool with default" also makes sense but it
needs to be coded a bit carefully. Issues to consider:
(1) Non default form "$r->config_bool('key')" should keep the
original semantics; missing key in the configuration is the
same as false (i.e. "undef" in scalar, () in list context).
(2) What should be the second parameter in the form to default
to true? '1'? 'true'? Any kind of "true" value in Perl
should be accepted?
(3) Same question as (2) but for defaulting to false. Any kind
of "false"?
^ permalink raw reply
* Re: gitk's copy pasteboard doesn't persist after it quits
From: Jean-François Veillette @ 2007-11-22 21:29 UTC (permalink / raw)
To: Shawn O. Pearce; +Cc: Jonathan del Strother, git
In-Reply-To: <20071122210105.GY14735@spearce.org>
Le 07-11-22 à 16:01, Shawn O. Pearce a écrit :
> Jonathan del Strother <maillist@steelskies.com> wrote:
>> On OS X, if I load gitk, copy a sha1, then quit, the sha1 isn't put
>> into the system-wide pasteboard. It's definitely copied - I can
>> paste
>> it back into the sha1 field - but it seems to be some sort of local
>> pasteboard that's specific to gitk
I don't know the specific of Tcl/Tk on OSX, but for Cocoa apps you
can choose how you want it pasteboard to behave. Imagine you copy a
1 gig video object, should you have another copy of it in the 'shared
space' ? no, that's why you can decide that you make it available on
demand only (if someone ask to paste it the system will ask your app
for the full content). But if you (as an application) have put
something in the pasteboard 'on-demand-only', then when you quit, you
should store the real value in there, nut just a pointer to the
values. I'm guessing the TclTk implementation doen't answer that
kind of notification from the system and quit without filling the
real pasteboard content.
- jfv
^ permalink raw reply
* Re: [PATCH 5/5] Added diff hunk coloring to git-add--interactive
From: Junio C Hamano @ 2007-11-22 21:37 UTC (permalink / raw)
To: Jeff King
Cc: Dan Zwell, Git Mailing List, Shawn O. Pearce, Wincent Colaiuta,
Jonathan del Strother, Johannes Schindelin, Frank Lichtenheld,
Jakub Narebski
In-Reply-To: <20071122122540.GH12913@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
>> + if ($diff_use_color) {
>> + $new_color = get_color('color.diff.new', 'green');
>> + $old_color = get_color('color.diff.old', 'red');
>> + $fraginfo_color = get_color('color.diff.frag', 'cyan');
>> + $metainfo_color = get_color('color.diff.meta', 'bold');
>> + $normal_color = Git::color_to_ansi_code('normal');
>> + # Not implemented:
>> + #$whitespace_color = get_color('color.diff.whitespace',
>> + #'normal red');
>
> Unfortunately, there is a historical wart that probably still needs
> supporting, which is that the original names were diff.color.*. Or have
> we officially removed support for that yet?
Neither officially or unofficially yet, but we can start the
process of making it official with an early announcement. I do
not think we would hurt people as long as a long enough advance
notice is given.
I however am wondering if we need to have so many "enable color
support" switches. color.status, color.diff, and now yet
another color.interactive? Who sets color.status and/or
color.interactive to auto without setting color.diff to auto as
well?
It may be good that they _can_ be individually controlled, but I
strongly suspect that most people would just want to set a
single variable color.ui to "auto", and have it give the default
value for all the color.$cmd configuration variable that are not
explicitly defined.
^ permalink raw reply
* Re: [PATCH 2/4] Teach git-add--interactive to accept a file path to patch
From: Wincent Colaiuta @ 2007-11-22 21:55 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git
In-Reply-To: <7vsl2y3wph.fsf@gitster.siamese.dyndns.org>
El 22/11/2007, a las 20:29, Junio C Hamano escribió:
> Wincent Colaiuta <win@wincent.com> writes:
>
>> Or if we add some kind of "--patch" switch, instead do:
>>
>> git add -i --patch foo
>
> You do not need both "-i" and "--patch", do you? Plain git-add
> does not take --patch anyway so it can pass --patch to the
> underlying interactive one.
Good point.
Cheers,
Wincent
^ permalink raw reply
* Re: [PATCH 3/3] Replace setenv(GIT_DIR_ENVIRONMENT, ...) with set_git_dir()
From: Johannes Schindelin @ 2007-11-22 22:09 UTC (permalink / raw)
To: Steffen Prohaska; +Cc: Johannes Sixt, Junio C Hamano, git, Dmitry Kakurin
In-Reply-To: <3B6B19E6-255F-4D8F-B6A3-255A9E8E0AB0@zib.de>
Hi,
On Thu, 22 Nov 2007, Steffen Prohaska wrote:
> Yes, and apparently even nobody knows how to trigger the problem on
> Windows.
A quick and easy way would be to instrument getenv(), unsetenv() and
setenv(), which would trigger an error. Something like this (but you
will have to put in a few "extern called_getenv; called_getenv = 0;",
since already a simple git-init fails because of setup_path()):
-- snipsnap --
[PATCH] Instrument getenv(), setenv(), unsetenv() and putenv()
... for finding places where a pointer obtained by getenv() could
be invalidated later.
---
environment.c | 1 +
git-compat-util.h | 31 +++++++++++++++++++++++++++++++
2 files changed, 32 insertions(+), 0 deletions(-)
diff --git a/environment.c b/environment.c
index ce75e98..027340e 100644
--- a/environment.c
+++ b/environment.c
@@ -9,6 +9,7 @@
*/
#include "cache.h"
+int called_setenv, called_getenv;
char git_default_email[MAX_GITNAME];
char git_default_name[MAX_GITNAME];
int trust_executable_bit = 1;
diff --git a/git-compat-util.h b/git-compat-util.h
index 79eb10e..a41469b 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -427,4 +427,35 @@ static inline int strtol_i(char const *s, int base, int *result)
return 0;
}
+extern int called_setenv, called_getenv;
+static inline char *test_getenv(const char *name)
+{
+ if (!called_setenv)
+ warning ("called test_getenv %s", name);
+ called_getenv = 1;
+ return getenv(name);
+}
+static inline int test_setenv(const char *name, const char *value, int overwrite)
+{
+ if (!called_setenv && called_getenv)
+ die ("getenv was called before setenv(%s, %s, %d)",
+ name, value, overwrite);
+ return setenv(name, value, overwrite);
+}
+static inline int test_unsetenv(const char *name)
+{
+ if (!called_setenv && called_getenv)
+ die ("getenv was called before unsetenv(%s)", name);
+ return unsetenv(name);
+}
+static inline int test_putenv(char *string)
+{
+ if (!called_setenv && called_getenv)
+ die ("getenv was called before putenv(%s)", string);
+ return putenv(string);
+}
+#define getenv test_getenv
+#define setenv test_setenv
+#define unsetenv test_unsetenv
+
#endif
^ permalink raw reply related
* Re: Reordering lines in "git-rebase -i" task sheet
From: Johannes Schindelin @ 2007-11-22 22:11 UTC (permalink / raw)
To: David Brown; +Cc: Junio C Hamano, git
In-Reply-To: <20071122192556.GA14662@old.davidb.org>
Hi,
On Thu, 22 Nov 2007, David Brown wrote:
> On Thu, Nov 22, 2007 at 01:21:55AM -0800, Junio C Hamano wrote:
>
> > That is the reason why git commit leaves the first line empty in its
> > initial template. It is to allow the user to immediately start
> > typing.
>
> Unless of course your editor tries to be "helpful" and remember what
> line you were on previously in a file.
IMHO this objection is irrelevant, since these helpful editors do not have
any preference as to where the help text should be.
Hth,
Dscho
^ permalink raw reply
* Re: [PATCH] Fix segmentation fault when user doesn't have access permission to the repository.
From: André Goddard Rosa @ 2007-11-22 22:27 UTC (permalink / raw)
To: Alex Riesen; +Cc: gitster, Git Mailing List
In-Reply-To: <20071122160959.GA3411@steel.home>
[-- Attachment #1: Type: text/plain, Size: 4501 bytes --]
On Nov 22, 2007 2:09 PM, Alex Riesen <raa.lkml@gmail.com> wrote:
> André Goddard Rosa, Thu, Nov 22, 2007 01:59:00 +0100:
> > @@ -487,8 +490,13 @@ static int do_fetch(struct transport *transport,
> > die("Don't know how to fetch from %s", transport->url);
> >
> > /* if not appending, truncate FETCH_HEAD */
> > - if (!append)
> > - fclose(fopen(git_path("FETCH_HEAD"), "w"));
> > + if (!append) {
> > + char *filename = git_path("FETCH_HEAD");
> > + int fd = fopen(filename, "w");
>
> This should have been "FILE *fp", not "int fd".
>
Hi, Alex!
Many thanks, you're right.
I tested it here before posting but luckly (or not, as I didn't catch
this when compiling) it worked,
as a pointer have the sizeof(int) in my x86 platform. >:|
Would you please comment on the attached patch and see if it's ok?
From dbadc5213b9957fb575c6da8528e5dd7a3f1f43e Mon Sep 17 00:00:00 2001
From: =?utf-8?q?Andr=C3=A9=20Goddard=20Rosa?= <andre.goddard@gmail.com>
Date: Thu, 22 Nov 2007 20:22:23 -0200
Subject: [PATCH] Fix segmentation fault when user doesn't have access
permission to the repository.
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit
Signed-off-by: André Goddard Rosa <andre.goddard@gmail.com>
---
builtin-fetch--tool.c | 12 ++++++++++--
builtin-fetch.c | 21 ++++++++++++++++-----
2 files changed, 26 insertions(+), 7 deletions(-)
diff --git a/builtin-fetch--tool.c b/builtin-fetch--tool.c
index ed60847..7460ab7 100644
--- a/builtin-fetch--tool.c
+++ b/builtin-fetch--tool.c
@@ -511,10 +511,14 @@ int cmd_fetch__tool(int argc, const char **argv,
const char *prefix)
if (!strcmp("append-fetch-head", argv[1])) {
int result;
FILE *fp;
+ char *filename;
if (argc != 8)
return error("append-fetch-head takes 6 args");
- fp = fopen(git_path("FETCH_HEAD"), "a");
+ filename = git_path("FETCH_HEAD");
+ fp = fopen(filename, "a");
+ if (!fp)
+ return error("cannot open %s: %s\n", filename, strerror(errno));
result = append_fetch_head(fp, argv[2], argv[3],
argv[4], argv[5],
argv[6], !!argv[7][0],
@@ -525,10 +529,14 @@ int cmd_fetch__tool(int argc, const char **argv,
const char *prefix)
if (!strcmp("native-store", argv[1])) {
int result;
FILE *fp;
+ char *filename;
if (argc != 5)
return error("fetch-native-store takes 3 args");
- fp = fopen(git_path("FETCH_HEAD"), "a");
+ filename = git_path("FETCH_HEAD");
+ fp = fopen(filename, "a");
+ if (!fp)
+ return error("cannot open %s: %s\n", filename, strerror(errno));
result = fetch_native_store(fp, argv[2], argv[3], argv[4],
verbose, force);
fclose(fp);
diff --git a/builtin-fetch.c b/builtin-fetch.c
index be9e3ea..84c8ed4 100644
--- a/builtin-fetch.c
+++ b/builtin-fetch.c
@@ -255,7 +255,7 @@ static int update_local_ref(struct ref *ref,
}
}
-static void store_updated_refs(const char *url, struct ref *ref_map)
+static int store_updated_refs(const char *url, struct ref *ref_map)
{
FILE *fp;
struct commit *commit;
@@ -263,8 +263,13 @@ static void store_updated_refs(const char *url,
struct ref *ref_map)
char note[1024];
const char *what, *kind;
struct ref *rm;
+ char *filename = git_path("FETCH_HEAD");
- fp = fopen(git_path("FETCH_HEAD"), "a");
+ fp = fopen(filename, "a");
+ if (!fp) {
+ error("cannot open %s: %s\n", filename, strerror(errno));
+ return 1;
+ }
for (rm = ref_map; rm; rm = rm->next) {
struct ref *ref = NULL;
@@ -335,6 +340,7 @@ static void store_updated_refs(const char *url,
struct ref *ref_map)
}
}
fclose(fp);
+ return 0;
}
/*
@@ -404,7 +410,7 @@ static int fetch_refs(struct transport *transport,
struct ref *ref_map)
if (ret)
ret = transport_fetch_refs(transport, ref_map);
if (!ret)
- store_updated_refs(transport->url, ref_map);
+ ret |= store_updated_refs(transport->url, ref_map);
transport_unlock_pack(transport);
return ret;
}
@@ -487,8 +493,13 @@ static int do_fetch(struct transport *transport,
die("Don't know how to fetch from %s", transport->url);
/* if not appending, truncate FETCH_HEAD */
- if (!append)
- fclose(fopen(git_path("FETCH_HEAD"), "w"));
+ if (!append) {
+ char *filename = git_path("FETCH_HEAD");
+ FILE *fp = fopen(filename, "w");
+ if (!fp)
+ return error("cannot open %s: %s\n", filename, strerror(errno));
+ fclose(fp);
+ }
ref_map = get_ref_map(transport, refs, ref_count, tags, &autotags);
--
1.5.3.6.861.gd794-dirty
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-segmentation-fault-when-user-doesn-t-have-access.patch --]
[-- Type: text/x-patch; name=0001-Fix-segmentation-fault-when-user-doesn-t-have-access.patch, Size: 3642 bytes --]
From dbadc5213b9957fb575c6da8528e5dd7a3f1f43e Mon Sep 17 00:00:00 2001
From: =?utf-8?q?Andr=C3=A9=20Goddard=20Rosa?= <andre.goddard@gmail.com>
Date: Thu, 22 Nov 2007 20:22:23 -0200
Subject: [PATCH] Fix segmentation fault when user doesn't have access
permission to the repository.
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit
Signed-off-by: André Goddard Rosa <andre.goddard@gmail.com>
---
builtin-fetch--tool.c | 12 ++++++++++--
builtin-fetch.c | 21 ++++++++++++++++-----
2 files changed, 26 insertions(+), 7 deletions(-)
diff --git a/builtin-fetch--tool.c b/builtin-fetch--tool.c
index ed60847..7460ab7 100644
--- a/builtin-fetch--tool.c
+++ b/builtin-fetch--tool.c
@@ -511,10 +511,14 @@ int cmd_fetch__tool(int argc, const char **argv, const char *prefix)
if (!strcmp("append-fetch-head", argv[1])) {
int result;
FILE *fp;
+ char *filename;
if (argc != 8)
return error("append-fetch-head takes 6 args");
- fp = fopen(git_path("FETCH_HEAD"), "a");
+ filename = git_path("FETCH_HEAD");
+ fp = fopen(filename, "a");
+ if (!fp)
+ return error("cannot open %s: %s\n", filename, strerror(errno));
result = append_fetch_head(fp, argv[2], argv[3],
argv[4], argv[5],
argv[6], !!argv[7][0],
@@ -525,10 +529,14 @@ int cmd_fetch__tool(int argc, const char **argv, const char *prefix)
if (!strcmp("native-store", argv[1])) {
int result;
FILE *fp;
+ char *filename;
if (argc != 5)
return error("fetch-native-store takes 3 args");
- fp = fopen(git_path("FETCH_HEAD"), "a");
+ filename = git_path("FETCH_HEAD");
+ fp = fopen(filename, "a");
+ if (!fp)
+ return error("cannot open %s: %s\n", filename, strerror(errno));
result = fetch_native_store(fp, argv[2], argv[3], argv[4],
verbose, force);
fclose(fp);
diff --git a/builtin-fetch.c b/builtin-fetch.c
index be9e3ea..84c8ed4 100644
--- a/builtin-fetch.c
+++ b/builtin-fetch.c
@@ -255,7 +255,7 @@ static int update_local_ref(struct ref *ref,
}
}
-static void store_updated_refs(const char *url, struct ref *ref_map)
+static int store_updated_refs(const char *url, struct ref *ref_map)
{
FILE *fp;
struct commit *commit;
@@ -263,8 +263,13 @@ static void store_updated_refs(const char *url, struct ref *ref_map)
char note[1024];
const char *what, *kind;
struct ref *rm;
+ char *filename = git_path("FETCH_HEAD");
- fp = fopen(git_path("FETCH_HEAD"), "a");
+ fp = fopen(filename, "a");
+ if (!fp) {
+ error("cannot open %s: %s\n", filename, strerror(errno));
+ return 1;
+ }
for (rm = ref_map; rm; rm = rm->next) {
struct ref *ref = NULL;
@@ -335,6 +340,7 @@ static void store_updated_refs(const char *url, struct ref *ref_map)
}
}
fclose(fp);
+ return 0;
}
/*
@@ -404,7 +410,7 @@ static int fetch_refs(struct transport *transport, struct ref *ref_map)
if (ret)
ret = transport_fetch_refs(transport, ref_map);
if (!ret)
- store_updated_refs(transport->url, ref_map);
+ ret |= store_updated_refs(transport->url, ref_map);
transport_unlock_pack(transport);
return ret;
}
@@ -487,8 +493,13 @@ static int do_fetch(struct transport *transport,
die("Don't know how to fetch from %s", transport->url);
/* if not appending, truncate FETCH_HEAD */
- if (!append)
- fclose(fopen(git_path("FETCH_HEAD"), "w"));
+ if (!append) {
+ char *filename = git_path("FETCH_HEAD");
+ FILE *fp = fopen(filename, "w");
+ if (!fp)
+ return error("cannot open %s: %s\n", filename, strerror(errno));
+ fclose(fp);
+ }
ref_map = get_ref_map(transport, refs, ref_count, tags, &autotags);
--
1.5.3.6.861.gd794-dirty
^ permalink raw reply related
* Re: [PATCH 4/5] Let git-add--interactive read colors from configuration
From: Jeff King @ 2007-11-22 22:30 UTC (permalink / raw)
To: Junio C Hamano
Cc: Dan Zwell, Git Mailing List, Shawn O. Pearce, Wincent Colaiuta,
Jonathan del Strother, Johannes Schindelin, Frank Lichtenheld,
Jakub Narebski
In-Reply-To: <7v63zu3r7h.fsf@gitster.siamese.dyndns.org>
On Thu, Nov 22, 2007 at 01:28:34PM -0800, Junio C Hamano wrote:
> I think the "config_bool with default" also makes sense but it
> needs to be coded a bit carefully. Issues to consider:
Yes. It is not strictly necessary for this patch series, but I think it
is nice to stake out a claim on the third argument of config_* functions
for consistency sake. But perhaps in the name of avoiding regression, it
should come later, when somebody actually wants to use it.
> (1) Non default form "$r->config_bool('key')" should keep the
> original semantics; missing key in the configuration is the
> same as false (i.e. "undef" in scalar, () in list context).
Yes, this is obviously the most important thing.
> (2) What should be the second parameter in the form to default
> to true? '1'? 'true'? Any kind of "true" value in Perl
> should be accepted?
>
> (3) Same question as (2) but for defaulting to false. Any kind
> of "false"?
Hmm. I am tempted to say "yes, any true or any false value" in that the
point of config_* is to convert git config values to native perl
representations. OTOH, the moral equivalent of
config_color('my.key', 'bold red');
is probably more appropriately
config_bool('my.key', 'true');
so I am fine doing it that way, as well (though I think it makes us
duplicate the "translate these strings into bools" code into perl).
-Peff
^ 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