* [PATCH 2/2] cvsimport: handle the parsing of uppercase config options
From: Michael J Gruber @ 2010-12-29 21:55 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1293659350.git.git@drmicha.warpmail.net>
The current code leads to
fatal: bad config value for 'cvsimport.r' in .git/config
for a standard use case with cvsimport.r set:
cvsimport sets internal variables by checking the config for each
possible command line option. The problem is that config items are case
insensitive, so config.r and config.R are the same. The ugly error is
due to that fact that cvsimport expects a bool for -R (and thus
config.R) but a remote name for -r (and thus config.r).
Fix this by making cvsimport expect long names for uppercase options.
config options for cvsimport have been undocumented so far, though
present in the code and advertised in several tutorials. So one may read
"enhance" for "fix". Similarly, the names for the options are
"documented" in the code, waitiing for their lowercase equivalents to be
transformed into long config options, as well.
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
git-cvsimport.perl | 19 ++++++++++++++++++-
t/t9600-cvsimport.sh | 7 +++++--
2 files changed, 23 insertions(+), 3 deletions(-)
diff --git a/git-cvsimport.perl b/git-cvsimport.perl
index 7888b77..8e683e5 100755
--- a/git-cvsimport.perl
+++ b/git-cvsimport.perl
@@ -90,6 +90,14 @@ sub write_author_info($) {
}
# convert getopts specs for use by git config
+my %longmap = (
+ 'A:' => 'authors-file',
+ 'M:' => 'merge-regex',
+ 'P:' => undef,
+ 'R' => 'track-revisions',
+ 'S:' => 'ignore-paths',
+);
+
sub read_repo_config {
# Split the string between characters, unless there is a ':'
# So "abc:de" becomes ["a", "b", "c:", "d", "e"]
@@ -99,8 +107,17 @@ sub read_repo_config {
$key =~ s/://g;
my $arg = 'git config';
$arg .= ' --bool' if ($o !~ /:$/);
-
- chomp(my $tmp = `$arg --get cvsimport.$key`);
+ my $ckey = $key;
+
+ if (exists $longmap{$o}) {
+ # An uppercase option like -R cannot be
+ # expressed in the configuration, as the
+ # variable names are downcased.
+ $ckey = $longmap{$o};
+ next if (! defined $ckey);
+ $ckey =~ s/-//g;
+ }
+ chomp(my $tmp = `$arg --get cvsimport.$ckey`);
if ($tmp && !($arg =~ /--bool/ && $tmp eq 'false')) {
no strict 'refs';
my $opt_name = "opt_" . $key;
diff --git a/t/t9600-cvsimport.sh b/t/t9600-cvsimport.sh
index 432b82e..4c384ff 100755
--- a/t/t9600-cvsimport.sh
+++ b/t/t9600-cvsimport.sh
@@ -89,7 +89,8 @@ EOF
test_expect_success PERL 'update git module' '
(cd module-git &&
- git cvsimport -a -R -z 0 module &&
+ git config cvsimport.trackRevisions true &&
+ git cvsimport -a -z 0 module &&
git merge origin
) &&
test_cmp module-cvs/o_fortuna module-git/o_fortuna
@@ -117,7 +118,8 @@ test_expect_success PERL 'cvsimport.module config works' '
(cd module-git &&
git config cvsimport.module module &&
- git cvsimport -a -R -z0 &&
+ git config cvsimport.trackRevisions true &&
+ git cvsimport -a -z0 &&
git merge origin
) &&
test_cmp module-cvs/tick module-git/tick
@@ -137,6 +139,7 @@ test_expect_success PERL 'import from a CVS working tree' '
$CVS co -d import-from-wt module &&
(cd import-from-wt &&
+ git config cvsimport.trackRevisions false &&
git cvsimport -a -z0 &&
echo 1 >expect &&
git log -1 --pretty=format:%s%n >actual &&
--
1.7.3.4.865.ga2bc4
^ permalink raw reply related
* [PATCH 1/2] cvsimport: partial whitespace cleanup
From: Michael J Gruber @ 2010-12-29 21:55 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1293659350.git.git@drmicha.warpmail.net>
in preparation of the config parse patch
Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
git-cvsimport.perl | 16 ++++++++--------
1 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/git-cvsimport.perl b/git-cvsimport.perl
index d27abfe..7888b77 100755
--- a/git-cvsimport.perl
+++ b/git-cvsimport.perl
@@ -91,8 +91,8 @@ sub write_author_info($) {
# convert getopts specs for use by git config
sub read_repo_config {
- # Split the string between characters, unless there is a ':'
- # So "abc:de" becomes ["a", "b", "c:", "d", "e"]
+ # Split the string between characters, unless there is a ':'
+ # So "abc:de" becomes ["a", "b", "c:", "d", "e"]
my @opts = split(/ *(?!:)/, shift);
foreach my $o (@opts) {
my $key = $o;
@@ -100,13 +100,13 @@ sub read_repo_config {
my $arg = 'git config';
$arg .= ' --bool' if ($o !~ /:$/);
- chomp(my $tmp = `$arg --get cvsimport.$key`);
+ chomp(my $tmp = `$arg --get cvsimport.$key`);
if ($tmp && !($arg =~ /--bool/ && $tmp eq 'false')) {
- no strict 'refs';
- my $opt_name = "opt_" . $key;
- if (!$$opt_name) {
- $$opt_name = $tmp;
- }
+ no strict 'refs';
+ my $opt_name = "opt_" . $key;
+ if (!$$opt_name) {
+ $$opt_name = $tmp;
+ }
}
}
}
--
1.7.3.4.865.ga2bc4
^ permalink raw reply related
* [PATCH 0/2] Handle cvsimport config for uppercase options
From: Michael J Gruber @ 2010-12-29 21:55 UTC (permalink / raw)
To: git
Sorry for rushing this out (no in-reply-to etc.) , I'm mostly offline
these days and pressed by the rc phase.
This is the version (n+1) as suggested by Junio, so 2/2 is mostly by JC
(only that his patch didn't apply, it probably was for maint).
I dropped the documentation patch, since the discussion resulted in short
config option names being frowned upon, so we should leave them undocumented
and return to rewrite them one day.
Michael J Gruber (2):
cvsimport: partial whitespace cleanup
cvsimport: handle the parsing of uppercase config options
git-cvsimport.perl | 33 +++++++++++++++++++++++++--------
t/t9600-cvsimport.sh | 7 +++++--
2 files changed, 30 insertions(+), 10 deletions(-)
--
1.7.3.4.865.ga2bc4
^ permalink raw reply
* Re: [PATCH] t9001: Fix test prerequisites
From: Junio C Hamano @ 2010-12-29 21:38 UTC (permalink / raw)
To: Robin H. Johnson; +Cc: git, Ævar Arnfjörð Bjarmason
In-Reply-To: <1293656551-5463-1-git-send-email-robbat2@gentoo.org>
"Robin H. Johnson" <robbat2@gentoo.org> writes:
> Add in missing Perl prerequisites for new tests of send-email.
>
> Signed-off-by: Robin H. Johnson <robbat2@gentoo.org>
> ---
> t/t9001-send-email.sh | 4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
Thanks.
> diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
> index 1dc4a92..3271426 100755
> --- a/t/t9001-send-email.sh
> +++ b/t/t9001-send-email.sh
> @@ -1135,7 +1135,7 @@ test_expect_success $PREREQ '--8bit-encoding also treats subject' '
> # Note that the patches in this test are deliberately out of order; we
> # want to make sure it works even if the cover-letter is not in the
> # first mail.
> -test_expect_success 'refusing to send cover letter template' '
> +test_expect_success $PREREQ 'refusing to send cover letter template' '
> clean_fake_sendmail &&
> rm -fr outdir &&
> git format-patch --cover-letter -2 -o outdir &&
> @@ -1151,7 +1151,7 @@ test_expect_success 'refusing to send cover letter template' '
> test -z "$(ls msgtxt*)"
> '
>
> -test_expect_success '--force sends cover letter template anyway' '
> +test_expect_success $PREREQ '--force sends cover letter template anyway' '
> clean_fake_sendmail &&
> rm -fr outdir &&
> git format-patch --cover-letter -2 -o outdir &&
This however makes me wonder (Robin, the following is primarily meant for
Ævar to whom 57cd35e (t/t9001-send-email.sh: change from skip_all=* to
prereq skip, 2010-08-13) is credited, and not a complaint to your patch at
all, but you are welcome to comment on it if you feel like).
Everything in this test seem to require $PREREQ now (test_expect_success
always takes the 3-parameter form). Does it suggest that we might want to
allow tests to define a "global prerequisite", e.g.
GIT_TESTS_PREREQ="PERL"
export GIT_TESTS_PREREQ
and make the traditional 2-parameter test_expect_success without an
explicit prerequisite take notice? Would it let us not have to worry
about this kind of breakages? Or is 9001 a very tiny minority oddball
that must have $PREREQ everywhere and such a test framework feature would
be an overkill?
More importantly, 9001 is all about send-email and we know upfront that we
want to skip everything when PERL prerequisite is not met. Why not a
simple
if ! test_have_prereq PERL
then
test_done
fi
insufficient?
Who cares "skipped statistics"? When one does not care about testing
send-email at all, why should one care how many tests on that program are
skipped? I personally do not think this is worth the trouble, and am very
close (showing two fingers almost touching) to suggest reverting 57cd35e.
Please convince me otherwise.
^ permalink raw reply
* Re: [PATCH 18/31] rebase: extract merge code to new source file
From: Johannes Sixt @ 2010-12-29 21:31 UTC (permalink / raw)
To: Martin von Zweigbergk
Cc: git, Junio C Hamano, Johannes Schindelin, Christian Couder
In-Reply-To: <1293528648-21873-19-git-send-email-martin.von.zweigbergk@gmail.com>
On Dienstag, 28. Dezember 2010, Martin von Zweigbergk wrote:
> -run_interactive_rebase () {
> +run_specific_rebase () {
> if [ "$interactive_rebase" = implied ]; then
> GIT_EDITOR=:
> export GIT_EDITOR
> fi
> export onto autosquash strategy strategy_opts verbose rebase_root \
> - force_rebase action preserve_merges upstream switch_to head_name
> - exec git-rebase--interactive
> + force_rebase action preserve_merges upstream switch_to head_name \
> + state_dir orig_head onto_name GIT_QUIET revisions RESOLVEMSG \
> + allow_rerere_autoupdate
> + export -f move_to_original_branch
Is export -f portable?
> + test "$type" != am && exec git-rebase--$type
I'd prefer to dispatch to the final rebase type using
. git-rebase--$type
This way, you can avoid to export the huge list of helper variables and the
function. (And it might be faster by a millisecond - or a few dozens on
Windows.)
-- Hannes
^ permalink raw reply
* [PATCH] t9001: Fix test prerequisites
From: Robin H. Johnson @ 2010-12-29 21:02 UTC (permalink / raw)
To: git; +Cc: Robin H. Johnson
Add in missing Perl prerequisites for new tests of send-email.
Signed-off-by: Robin H. Johnson <robbat2@gentoo.org>
---
t/t9001-send-email.sh | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 1dc4a92..3271426 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -1135,7 +1135,7 @@ test_expect_success $PREREQ '--8bit-encoding also treats subject' '
# Note that the patches in this test are deliberately out of order; we
# want to make sure it works even if the cover-letter is not in the
# first mail.
-test_expect_success 'refusing to send cover letter template' '
+test_expect_success $PREREQ 'refusing to send cover letter template' '
clean_fake_sendmail &&
rm -fr outdir &&
git format-patch --cover-letter -2 -o outdir &&
@@ -1151,7 +1151,7 @@ test_expect_success 'refusing to send cover letter template' '
test -z "$(ls msgtxt*)"
'
-test_expect_success '--force sends cover letter template anyway' '
+test_expect_success $PREREQ '--force sends cover letter template anyway' '
clean_fake_sendmail &&
rm -fr outdir &&
git format-patch --cover-letter -2 -o outdir &&
--
1.7.3.2
^ permalink raw reply related
* Re* [RFC/PATCH] Re: git submodule -b ... of current HEAD fails
From: Junio C Hamano @ 2010-12-29 20:53 UTC (permalink / raw)
To: Jens Lehmann; +Cc: Jonathan Nieder, git, Klaus Ethgen, Sven Verdoolaege
In-Reply-To: <4D1AF989.3000105@web.de>
Jens Lehmann <Jens.Lehmann@web.de> writes:
> $ git checkout -f -q -b master origin/master
> fatal: git checkout: branch master already exists
> $ git checkout -f -q -B master origin/master
> fatal: Cannot force update the current branch.
>
> So maybe the real problem here is that "git checkout -B" barfs when it
> doesn't have anything to do instead of silently doing nothing?
>
> So we maybe want to fix this issue in "git checkout"? Then the patch
> will start working (and the test for it can be added in a later patch).
Let me think aloud, as it may be a good time to summarize the current
semantics of what "checking out a branch" means.
0. "git checkout $branch" to check out a branch is to start preparing to
commit on that branch while keeping the tentative changes you have in
your working tree and your index.
1. You might not have the $branch yet to check out in such a way. You
could do
$ git branch $branch $start && git checkout $branch
and
$ git checkout -b $branch $start
is conceptually a short-hand of the above two command sequence.
3. When $branch exists, 2. should fail, as it involves resetting the tip
of $branch to $start, potentially losing commits near its old tip.
If you really mean it, you can force it by
$ git checkout -B $branch $start
which is conceptually a short-hand for:
$ git branch -f $branch $start && git checkout $branch
4. "git checkout $branch" refuses to discard your work when the
tentative changes you made conflict with the difference between your
current HEAD and $branch. You can give "-m" to force it to attempt a
three-way merge with possible conflicts, or you can give "-f" to force
it to discard all tentative changes.
5. The logical consequence of all of the above is that "git checkout [-f]
-B $branch $start" should conceptually be a short-hand for:
$ git branch -f $branch $start && git checkout [-f] $branch
And "branch -f $branch" refuses to reset the tip of the current branch.
An end-user who runs "git checkout -f -B $branch $start" is telling us
the following things:
- The $branch may or may not exist;
- The tip it currently may point at is immaterial and the user is willing
to lose it (this is what capital-ness of -B means);
- The tentative changes in the working tree and the index is immaterial
(this is what -f given to checkout means).
It seems to me that "checkout -f -B $branch $start" when $branch is the
current one ought to do an equivalent of "reset --hard $start" and nothing
else.
What happens if -f is not given? The user wants to keep the tentative
changes, and that is done by comparing a commit in HEAD (i.e. the current
branch) and the target $branch. The two step approach that can be
illustrated by the short-hand definition of "checkout -B" however breaks
down in this case, even if you change the variant of "git branch -f" it
internally runs:
$ git branch --allow-updating-current -f $branch $start &&
git checkout $branch
The first step already resets $branch to its new value, and the second
step does not have a way to know that your tentative changes are based on
the previous value.
BUT ;-)
I think it is Ok after all. The actual implementation of "checkout -[b|B]"
is more like:
$ git read-tree -m -u HEAD $start
$ git update-ref -f refs/heads/$branch $start
$ git symbolic-ref HEAD refs/heads/$branch
That is, when we update the working tree and the index, we still haven't
updated the HEAD yet (switch_branches() calls merge_working_tree() to run
this two-way merge, and then calls update_refs_for_switch() which does the
update-ref part including the branch creation).
The next question is if we want to enable "git branch -f $branch $start"
for the current branch from the end user, which presumably would look as
if the user ran "git reset $start". I am actually indifferent about this,
but as the first step we probably should be conservative to forbid it as
we do now.
So in conclusion, here is a patch that is not even compile tested ;-)
This was made on top of 'maint' only because that was the branch I
happened to have checked out in my development working tree.
Note that the test script does not even test the case I was initially
worried about (i.e. keeping local changes). I'll leave it to interested
others ;-)
branch.c | 12 ++++++++----
branch.h | 23 +++++++++++++++--------
builtin/branch.c | 3 ++-
builtin/checkout.c | 4 +++-
t/t2018-checkout-branch.sh | 13 +++++++++++++
5 files changed, 41 insertions(+), 14 deletions(-)
diff --git a/branch.c b/branch.c
index 93dc866..74eb866 100644
--- a/branch.c
+++ b/branch.c
@@ -137,7 +137,7 @@ static int setup_tracking(const char *new_ref, const char *orig_ref,
void create_branch(const char *head,
const char *name, const char *start_name,
- int force, int reflog, enum branch_track track)
+ int flags, int reflog, enum branch_track track)
{
struct ref_lock *lock = NULL;
struct commit *commit;
@@ -147,6 +147,9 @@ void create_branch(const char *head,
int forcing = 0;
int dont_change_ref = 0;
int explicit_tracking = 0;
+ int ok_to_update = flags & (CREATE_BRANCH_UPDATE_OK |
+ CREATE_BRANCH_UPDATE_CURRENT_OK);
+ int ok_to_update_current = flags & CREATE_BRANCH_UPDATE_CURRENT_OK;
if (track == BRANCH_TRACK_EXPLICIT || track == BRANCH_TRACK_OVERRIDE)
explicit_tracking = 1;
@@ -155,11 +158,12 @@ void create_branch(const char *head,
die("'%s' is not a valid branch name.", name);
if (resolve_ref(ref.buf, sha1, 1, NULL)) {
- if (!force && track == BRANCH_TRACK_OVERRIDE)
+ if (!ok_to_update && track == BRANCH_TRACK_OVERRIDE)
dont_change_ref = 1;
- else if (!force)
+ else if (!ok_to_update)
die("A branch named '%s' already exists.", name);
- else if (!is_bare_repository() && head && !strcmp(head, name))
+ else if (!ok_to_update_current &&
+ !is_bare_repository() && head && !strcmp(head, name))
die("Cannot force update the current branch.");
forcing = 1;
}
diff --git a/branch.h b/branch.h
index eed817a..0afbeae 100644
--- a/branch.h
+++ b/branch.h
@@ -4,16 +4,23 @@
/* Functions for acting on the information about branches. */
/*
- * Creates a new branch, where head is the branch currently checked
- * out, name is the new branch name, start_name is the name of the
- * existing branch that the new branch should start from, force
- * enables overwriting an existing (non-head) branch, reflog creates a
- * reflog for the branch, and track causes the new branch to be
- * configured to merge the remote branch that start_name is a tracking
- * branch for (if any).
+ * Creates a new branch, where:
+ *
+ * - head is the branch currently checked out;
+ * - name is the new branch name;
+ * - start_name is the name of a commit that the new branch should start at
+ * (could be another branch or a remote-tracking branch, in which case
+ * track---see below---may also trigger);
+ * - flags indicates overwriting an existing branch and/or overwriting the
+ * current branch is allowed;
+ * - reflog creates a reflog for the branch; and
+ * - track causes the new branch to be configured to merge the remote branch
+ * that start_name is a tracking branch for (if any).
*/
+#define CREATE_BRANCH_UPDATE_OK 01
+#define CREATE_BRANCH_UPDATE_CURRENT_OK 02
void create_branch(const char *head, const char *name, const char *start_name,
- int force, int reflog, enum branch_track track);
+ int flags, int reflog, enum branch_track track);
/*
* Remove information about the state of working on the current
diff --git a/builtin/branch.c b/builtin/branch.c
index 87976f0..ea5b411 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -651,7 +651,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
OPT_BIT('m', NULL, &rename, "move/rename a branch and its reflog", 1),
OPT_BIT('M', NULL, &rename, "move/rename a branch, even if target exists", 2),
OPT_BOOLEAN('l', NULL, &reflog, "create the branch's reflog"),
- OPT_BOOLEAN('f', "force", &force_create, "force creation (when already exists)"),
+ OPT_SET_INT('f', "force", &force_create, "force creation (when already exists)",
+ CREATE_BRANCH_UPDATE_OK),
{
OPTION_CALLBACK, 0, "no-merged", &merge_filter_ref,
"commit", "print only not merged branches",
diff --git a/builtin/checkout.c b/builtin/checkout.c
index a54583b..bf5b43a 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -529,7 +529,9 @@ static void update_refs_for_switch(struct checkout_opts *opts,
}
else
create_branch(old->name, opts->new_branch, new->name,
- opts->new_branch_force ? 1 : 0,
+ opts->new_branch_force
+ ? CREATE_BRANCH_UPDATE_CURRENT_OK
+ : 0,
opts->new_branch_log, opts->track);
new->name = opts->new_branch;
setup_branch_path(new);
diff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh
index fa69016..37e4fdb 100755
--- a/t/t2018-checkout-branch.sh
+++ b/t/t2018-checkout-branch.sh
@@ -169,4 +169,17 @@ test_expect_success 'checkout -f -B to an existing branch with mergeable changes
test_must_fail test_dirty_mergeable
'
+test_expect_success 'checkout -b/B the current branch' '
+ git reset --hard &&
+ git checkout branch1 &&
+ it=$(git rev-parse --verify HEAD) &&
+ test_must_fail git checkout -b branch1 HEAD &&
+ git checkout -B branch1 $it &&
+ test "$it" = "$(git rev-parse --verify HEAD)" &&
+ test "refs/heads/branch1" = "$(git symbolic-ref HEAD)" &&
+ git checkout -B branch1 HEAD &&
+ test "$it" = "$(git rev-parse --verify HEAD)" &&
+ test "refs/heads/branch1" = "$(git symbolic-ref HEAD)"
+'
+
test_done
^ permalink raw reply related
* [PATCH 3/3] gitweb: add css class to remote url titles
From: Sylvain Rabot @ 2010-12-29 19:33 UTC (permalink / raw)
To: git; +Cc: Sylvain Rabot
In-Reply-To: <1293651215-4924-1-git-send-email-sylvain@abstraction.fr>
Signed-off-by: Sylvain Rabot <sylvain@abstraction.fr>
---
gitweb/gitweb.perl | 8 ++++----
gitweb/static/gitweb.css | 5 +++++
2 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index eae75ac..cb169c7 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -5146,13 +5146,13 @@ sub git_remote_block {
if (defined $fetch) {
if ($fetch eq $push) {
- $urls_table .= format_repo_url("URL", $fetch);
+ $urls_table .= format_repo_url("<span class=\"metadata_remote_fetch_url\">URL</span>", $fetch);
} else {
- $urls_table .= format_repo_url("Fetch URL", $fetch);
- $urls_table .= format_repo_url("Push URL", $push) if defined $push;
+ $urls_table .= format_repo_url("<span class=\"metadata_remote_fetch_url\">Fetch URL</span>", $fetch);
+ $urls_table .= format_repo_url("<span class=\"metadata_remote_push_url\">Push URL</span>", $push) if defined $push;
}
} elsif (defined $push) {
- $urls_table .= format_repo_url("Push URL", $push);
+ $urls_table .= format_repo_url("<span class=\"metadata_remote_push_url\">Push URL</span>", $push);
} else {
$urls_table .= format_repo_url("", "No remote URL");
}
diff --git a/gitweb/static/gitweb.css b/gitweb/static/gitweb.css
index 79d7eeb..631b20d 100644
--- a/gitweb/static/gitweb.css
+++ b/gitweb/static/gitweb.css
@@ -579,6 +579,11 @@ div.remote {
display: inline-block;
}
+.metadata_remote_fetch_url,
+.metadata_remote_push_url {
+ font-weight: bold;
+}
+
/* Style definition generated by highlight 2.4.5, http://www.andre-simon.de/ */
/* Highlighting theme definition: */
--
1.7.3.4.523.g72f0d.dirty
^ permalink raw reply related
* [PATCH 2/3] gitweb: remove test when closing file descriptor
From: Sylvain Rabot @ 2010-12-29 19:33 UTC (permalink / raw)
To: git; +Cc: Sylvain Rabot
In-Reply-To: <1293651215-4924-1-git-send-email-sylvain@abstraction.fr>
it happens that closing file descriptor fails whereas
the blob is perfectly readable. According to perlman
the reasons could be:
If the file handle came from a piped open, "close" will additionally
return false if one of the other system calls involved fails, or if the
program exits with non-zero status. (If the only problem was that the
program exited non-zero, $! will be set to 0.) Closing a pipe also waits
for the process executing on the pipe to complete, in case you want to
look at the output of the pipe afterwards, and implicitly puts the exit
status value of that command into $?.
Prematurely closing the read end of a pipe (i.e. before the process writ-
ing to it at the other end has closed it) will result in a SIGPIPE being
delivered to the writer. If the other end can't handle that, be sure to
read all the data before closing the pipe.
In this case we don't mind that close fails.
Signed-off-by: Sylvain Rabot <sylvain@abstraction.fr>
---
gitweb/gitweb.perl | 3 +--
1 files changed, 1 insertions(+), 2 deletions(-)
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index ea984b9..eae75ac 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3465,8 +3465,7 @@ sub run_highlighter {
my ($fd, $highlight, $syntax) = @_;
return $fd unless ($highlight && defined $syntax);
- close $fd
- or die_error(404, "Reading blob failed");
+ close $fd;
open $fd, quote_command(git_cmd(), "cat-file", "blob", $hash)." | ".
quote_command($highlight_bin).
" --xhtml --fragment --syntax $syntax |"
--
1.7.3.4.523.g72f0d.dirty
^ permalink raw reply related
* [PATCH 1/3] gitweb: add extensions to highlight feature
From: Sylvain Rabot @ 2010-12-29 19:33 UTC (permalink / raw)
To: git; +Cc: Sylvain Rabot
In-Reply-To: <1293651215-4924-1-git-send-email-sylvain@abstraction.fr>
added: sql, php5, phps, bash, zsh, ksh, mk, make
Signed-off-by: Sylvain Rabot <sylvain@abstraction.fr>
---
gitweb/gitweb.perl | 7 ++++---
1 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 4779618..ea984b9 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -250,13 +250,14 @@ our %highlight_ext = (
# main extensions, defining name of syntax;
# see files in /usr/share/highlight/langDefs/ directory
map { $_ => $_ }
- qw(py c cpp rb java css php sh pl js tex bib xml awk bat ini spec tcl),
+ qw(py c cpp rb java css php sh pl js tex bib xml awk bat ini spec tcl sql make),
# alternate extensions, see /etc/highlight/filetypes.conf
'h' => 'c',
+ map { $_ => 'sh' } qw(bash zsh ksh),
map { $_ => 'cpp' } qw(cxx c++ cc),
- map { $_ => 'php' } qw(php3 php4),
+ map { $_ => 'php' } qw(php3 php4 php5 phps),
map { $_ => 'pl' } qw(perl pm), # perhaps also 'cgi'
- 'mak' => 'make',
+ map { $_ => 'make'} qw(mak mk),
map { $_ => 'xml' } qw(xhtml html htm),
);
--
1.7.3.4.523.g72f0d.dirty
^ permalink raw reply related
* [PATCH 0/3 v3] minor gitweb modifications
From: Sylvain Rabot @ 2010-12-29 19:33 UTC (permalink / raw)
To: git; +Cc: Sylvain Rabot
here a three patch serie with minor updates for gitweb based on master.
This serie has been improved regarding the comments of :
- Jakub Narebski <jnareb@gmail.com>
- Jonathan Nieder <jrnieder@gmail.com>
Regards.
Sylvain Rabot (3):
gitweb: add extensions to highlight feature
gitweb: remove test when closing file descriptor
gitweb: add css class to remote url titles
gitweb/gitweb.perl | 18 +++++++++---------
gitweb/static/gitweb.css | 5 +++++
2 files changed, 14 insertions(+), 9 deletions(-)
--
1.7.3.4.523.g72f0d.dirty
^ permalink raw reply
* RE: help for a git newbie please - git hooks
From: Marlene Cote @ 2010-12-29 19:26 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git@vger.kernel.org
In-Reply-To: <m3k4iscsq2.fsf@localhost.localdomain>
Jakub,
Thank-you for your reply and your advice about the subject. I apologize for the vagueness of the previous one.
I see the templates directory now. Thanks again.
--------------------------
Regards,
Marlene Cote
Affirmed Networks
978-268-0821
->-----Original Message-----
->From: Jakub Narebski [mailto:jnareb@gmail.com]
->Sent: Wednesday, December 29, 2010 1:58 PM
->To: Marlene Cote
->Cc: git@vger.kernel.org
->Subject: Re: help for a git newbie please
->
->Marlene Cote <Marlene_Cote@affirmednetworks.com> writes:
->
->> I don't understand the docs when they talk about hooks. I have
->> tried making my .git/hooks samples executable and they don't have
->> any suffix to remove, so they should just run. However, every time
->> I make a new clone, the changes I made to the hooks are gone. Just
->> the samples get put into the clone again. How do I put a hook in
->> place that every developer will get and execute? If I should be
->> using server side hooks, where exactly would those go? Should I
->> modify the hooks under my repositories directory?
->
->First, for the future, could you please use more specific subject?
->"Help for a git newbie please" doesn't tell us _anything_ about what
->problem do you have with git on first glance.
->
->Second, hooks are not versioned and not under version control. They
->are not transferred on clone either. There are reasons for that,
->including the safety (running code under control of other side), and
->the fact that hooks are usually configured to local needs, so they
->might not make sense for other people.
->
->
->Now, if you either use some kind of networked filesystem, or you can
->configure it so each developers machine has the same install, you can
->make use of git templates mechanism. This is the way git includes
->sample hooks in newly created repositories (git init, git clone, or
->even IIRC "git init" on existing repository).
->
->Those template files are by default (on Linux) installed in
->/usr/share/git-core/templates. So what you can do is to put hooks you
->want each developer to have (either as executable hook, or as hook
->sample) in /usr/share/git-core/templates/hooks/ directory (or its
->equivalent in your installation).
->
->I hope that helps.
->
->--
->Jakub Narebski
->Poland
->ShadeHawk on #git
^ permalink raw reply
* shared hooks (Re: help for a git newbie please)
From: Jonathan Nieder @ 2010-12-29 19:17 UTC (permalink / raw)
To: Jakub Narebski; +Cc: Marlene Cote, git
In-Reply-To: <m3k4iscsq2.fsf@localhost.localdomain>
Jakub Narebski wrote:
> Now, if you either use some kind of networked filesystem, or you can
> configure it so each developers machine has the same install, you can
> make use of git templates mechanism.
To expand on this: if you are in the "controlled environment"
situation, you may find the discussion pointed to by [1] interesting.
If you do not control developer machines, life is even simpler.
One approach[2] is to track a git-templates/ directory in the working
tree and add a setup-hooks.sh script that installs those hooks in
.git/hooks.
[1] http://thread.gmane.org/gmane.linux.debian.devel.bugs.general/776932
Summary: it is easier to change hooks later if you keep hooks in some
directory outside the template dir (e.g., in /etc/hooks) and put a
symlink to that directory in the template.
[2] e.g., used by the libreoffice repository iirc
^ permalink raw reply
* Re: [PATCH] unpack_trees(): skip trees that are the same in all input
From: Junio C Hamano @ 2010-12-29 19:15 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: git, Linus Torvalds
In-Reply-To: <AANLkTinm=k_84Nh4YakbkdNNLO4-yeVxGF+p_rR5TFB=@mail.gmail.com>
Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
> On Thu, Dec 23, 2010 at 5:13 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> @@ -447,6 +451,11 @@ static int traverse_trees_recursive(int n, unsigned long dirmask,
>> struct traverse_info newinfo;
>> struct name_entry *p;
>>
>> + if (!df_conflicts) {
>> + int status = fast_forward_merge(n, dirmask, names, info);
>> + if (status)
>> + return status;
>> + }
>
> Also skip the optimization when sparse checkout is active
> (info->data->skip_sparse_checkout == 0). People may need to just
> update skip-worktree bits and add/remove worktree files along the
> line.
Sounds sensible and safer to special case that one, I would agree.
By the way, I think info->data->skip_sparse_checkout should be fixed by
renaming it to info->data->sparse_checkout. The flag controls a special
case logic that should not be in effect unless explicitly asked, and
forcing normal codepath to say "If skip_sparse_checkout is not false, do
this" in double-negative is unnice than "If sparse_checkout is in effect,
please run this special case" when reading the code.
^ permalink raw reply
* Re: help for a git newbie please
From: Jakub Narebski @ 2010-12-29 18:58 UTC (permalink / raw)
To: Marlene Cote; +Cc: git
In-Reply-To: <20053D7ED46E664F8B9A4D5E5B31937407197534@MBX021-W4-CA-1.exch021.domain.local>
Marlene Cote <Marlene_Cote@affirmednetworks.com> writes:
> I don't understand the docs when they talk about hooks. I have
> tried making my .git/hooks samples executable and they don't have
> any suffix to remove, so they should just run. However, every time
> I make a new clone, the changes I made to the hooks are gone. Just
> the samples get put into the clone again. How do I put a hook in
> place that every developer will get and execute? If I should be
> using server side hooks, where exactly would those go? Should I
> modify the hooks under my repositories directory?
First, for the future, could you please use more specific subject?
"Help for a git newbie please" doesn't tell us _anything_ about what
problem do you have with git on first glance.
Second, hooks are not versioned and not under version control. They
are not transferred on clone either. There are reasons for that,
including the safety (running code under control of other side), and
the fact that hooks are usually configured to local needs, so they
might not make sense for other people.
Now, if you either use some kind of networked filesystem, or you can
configure it so each developers machine has the same install, you can
make use of git templates mechanism. This is the way git includes
sample hooks in newly created repositories (git init, git clone, or
even IIRC "git init" on existing repository).
Those template files are by default (on Linux) installed in
/usr/share/git-core/templates. So what you can do is to put hooks you
want each developer to have (either as executable hook, or as hook
sample) in /usr/share/git-core/templates/hooks/ directory (or its
equivalent in your installation).
I hope that helps.
--
Jakub Narebski
Poland
ShadeHawk on #git
^ permalink raw reply
* help for a git newbie please
From: Marlene Cote @ 2010-12-29 17:43 UTC (permalink / raw)
To: git@vger.kernel.org
I don't understand the docs when they talk about hooks. I have tried making my .git/hooks samples executable and they don't have any suffix to remove, so they should just run. However, every time I make a new clone, the changes I made to the hooks are gone. Just the samples get put into the clone again. How do I put a hook in place that every developer will get and execute? If I should be using server side hooks, where exactly would those go? Should I modify the hooks under my repositories directory?
--------------------------
Regards,
Marlene Cote
Affirmed Networks
978-268-0821
^ permalink raw reply
* [PATCH 4/4] git-instaweb: Use "git-instaweb" as sitename (for page titles)
From: Jakub Narebski @ 2010-12-29 16:50 UTC (permalink / raw)
To: git; +Cc: Eric Wong
In-Reply-To: <201012291743.41213.jnareb@gmail.com>
The default is to use "$ENV{'SERVER_NAME'} Git" or "Untitled Git";
use "git-instaweb" instead.
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
This is new feature, and doesn't need to be in 1.7.4
It would be an RFC, if not for the fact that it is such trivial change.
git-instaweb.sh | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/git-instaweb.sh b/git-instaweb.sh
index 94f8b07..935515d 100755
--- a/git-instaweb.sh
+++ b/git-instaweb.sh
@@ -583,6 +583,7 @@ gitweb_conf() {
our \$projectroot = "$(dirname "$fqgitdir")";
our \$git_temp = "$fqgitdir/gitweb/tmp";
our \$projects_list = \$projectroot;
+our \$site_name = "git-instaweb";
our @stylesheets = ("static/".basename(\$stylesheets[0]));
our \$logo = "static/git-logo.png";
--
1.7.3
^ permalink raw reply related
* [PATCH 3/4] git-instaweb: Add checks that web server can be started and is started
From: Jakub Narebski @ 2010-12-29 16:49 UTC (permalink / raw)
To: git; +Cc: Eric Wong
In-Reply-To: <201012291743.41213.jnareb@gmail.com>
Better to check that "$root/gitweb.cgi" exists, and that web server
started, rather than have (for example) the following error message
$ git instaweb --httpd=plackup --browser=mozilla
Waiting for 'plackup' to start .../usr/share/gitweb/gitweb.cgi:
No such file or directory at <somewhere>/perl5/CGI/Compile.pm line 97.
........
and keep waiting in httpd_is_ready for gitweb.cgi which would not start.
Note that the first check is not strictly necessary, but checking
separately that gitweb.cgi script can be found at expected place leads
to better error message.
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
It can be considered bugfix, or it can be considered hardening of
git-instaweb (making it more resilent).
git-instaweb.sh | 5 ++++-
1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/git-instaweb.sh b/git-instaweb.sh
index da569f2..94f8b07 100755
--- a/git-instaweb.sh
+++ b/git-instaweb.sh
@@ -41,6 +41,9 @@ test -z "$root" && root='@@GITWEBDIR@@'
# any untaken local port will do...
test -z "$port" && port=1234
+# Sanity check
+test -e "$root/gitweb.cgi" || die "gitweb.cgi not found at '$root'"
+
resolve_full_httpd () {
case "$httpd" in
*apache2*|*lighttpd*|*httpd*)
@@ -617,7 +620,7 @@ webrick)
;;
esac
-start_httpd
+start_httpd || die "Could not start '$httpd'"
url=http://127.0.0.1:$port
if test -n "$browser"; then
--
1.7.3
^ permalink raw reply related
* [PATCH 2/4] git-instaweb: Static files are under "static/" in gitweb_conf
From: Jakub Narebski @ 2010-12-29 16:48 UTC (permalink / raw)
To: git; +Cc: Tadeusz Sosnierz, Eric Wong
In-Reply-To: <201012291743.41213.jnareb@gmail.com>
The variables used to compose links for gitweb static files,
i.e. @stylesheets / $stylesheet, $logo, etc. are configurable via
GITWEB_CSS, GITWEB_LOGO, etc. gitweb build configuration variables.
The installation directory of those static files is currently always
set to $(gitwebdir)/static. Thus git-instaweb could rely on the place
where those files are installed, but not on $logo, $favicon
etc. variables.
Set @stylesheets, $logo, $favicon and $javascript to their default
values in gitweb config file for git-instaweb (in gitweb_conf
function), so that web servers invoked by git-instaweb (for example
Plack::Middleware::Static component for 'plackup' web server) can
correctly serve links to those static files.
We assume here that git-instaweb uses modern gitweb with names of
gitweb config variables as mentioned above.
Note that we had to take care that possible optional minimizations of
stylesheet and of javascript is correctly taken into account.
This is continuation of fixup for issue noticed by Tadeusz Sośnierz
with --httpd=plackup and gitweb stylesheet (CSS).
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
The subject (commit description) is not the best, but that is what I
was able to come up with.
This doesn't matter for default configuration and setup, but it allows
me to use my custom gitweb build configuration without any changes in
result.
git-instaweb.sh | 5 +++++
1 files changed, 5 insertions(+), 0 deletions(-)
diff --git a/git-instaweb.sh b/git-instaweb.sh
index bb57d81..da569f2 100755
--- a/git-instaweb.sh
+++ b/git-instaweb.sh
@@ -581,6 +581,11 @@ our \$projectroot = "$(dirname "$fqgitdir")";
our \$git_temp = "$fqgitdir/gitweb/tmp";
our \$projects_list = \$projectroot;
+our @stylesheets = ("static/".basename(\$stylesheets[0]));
+our \$logo = "static/git-logo.png";
+our \$favicon = "static/git-favicon.png";
+our \$javascript = "static/".basename(\$javascript);
+
\$feature{'remote_heads'}{'default'} = [1];
EOF
}
--
1.7.3
^ permalink raw reply related
* [BUGFIX PATCH 1/4] git-instaweb: Fix issue with static files for 'plackup' server
From: Jakub Narebski @ 2010-12-29 16:47 UTC (permalink / raw)
To: git; +Cc: Tadeusz Sosnierz, Eric Wong
In-Reply-To: <201012291743.41213.jnareb@gmail.com>
The default (in gitweb/Makefile) is to use relative paths for gitweb
static files, e.g. "static/gitweb.css" for GITWEB_CSS. But the
configuration for Plack::Middleware::Static in plackup_conf assumed
that static files must be absolute paths starting with "/gitweb/"
prefix which had to be stripped, e.g. "/gitweb/static/gitweb.css".
This in turn caused web server run by "git instaweb --httpd=plackup"
to not access static files (e.g. CSS) correctly.
This is a minimal fixup, making 'plackup' web server in git-instaweb
work with default gitweb build configuration.
Reported-by: Tadeusz Sośnierz <tadzikes@gmail.com>
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
The regexp is probably too strict: qr{^/static/} should be enough,
but I didn't want to change too much at once.
This bug was not noticed because we don't have any test for
git-instaweb, not mentioning tests for all web servers supported. And
the fact that I was checking "git instaweb -httpd=plackup" against
gitweb.cgi built with custom configuration (including the fact that
GITWEB_CSS="/gitweb/static/gitweb.css").
tadzik, does that fix the issue you noticed?
Junio, could you apply this before 1.7.4? Thanks in advance.
git-instaweb.sh | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/git-instaweb.sh b/git-instaweb.sh
index 10fcebb..bb57d81 100755
--- a/git-instaweb.sh
+++ b/git-instaweb.sh
@@ -549,7 +549,7 @@ my \$app = builder {
};
# serve static files, i.e. stylesheet, images, script
enable 'Static',
- path => sub { m!\.(js|css|png)\$! && s!^/gitweb/!! },
+ path => qr{^/static/.*(?:js|css|png)\$},
root => "$root/",
encoding => 'utf-8'; # encoding for 'text/plain' files
# convert CGI application to PSGI app
--
1.7.3
^ permalink raw reply related
* [PATCH 0/4] git-instaweb: Three fixes and one improvement
From: Jakub Narebski @ 2010-12-29 16:43 UTC (permalink / raw)
To: git; +Cc: Tadeusz Sosnierz, Eric Wong
Actually only the first patch is a proper bugfix for issue noticed by
Tadeusz Sośnierz ('tadzik' on github, https://github.com/tadzik) in
GitHub private message; second and third are hardening git-instaweb
against differences in configuration and environment.
Junio, could you at least apply the first one before 1.7.4? This is a
fix for a real bug, if for not often used "git instaweb
--httpd=plackup".
tadzik, does the first patch fixes the issue you noticed?
Jakub Narebski (4):
git-instaweb: Fix issue with static files for 'plackup' server
git-instaweb: Static files are under "static/" in gitweb_conf
git-instaweb: Add checks that web server can be started and is
started
git-instaweb: Use "git-instaweb" as sitename (for page titles)
git-instaweb.sh | 13 +++++++++++--
1 files changed, 11 insertions(+), 2 deletions(-)
--
Jakub Narębski
Poland
^ permalink raw reply
* Re: [PATCH] unpack_trees(): skip trees that are the same in all input
From: Nguyen Thai Ngoc Duy @ 2010-12-29 14:43 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Linus Torvalds
In-Reply-To: <7vr5d94fs4.fsf@alter.siamese.dyndns.org>
On Thu, Dec 23, 2010 at 5:13 AM, Junio C Hamano <gitster@pobox.com> wrote:
> @@ -447,6 +451,11 @@ static int traverse_trees_recursive(int n, unsigned long dirmask,
> struct traverse_info newinfo;
> struct name_entry *p;
>
> + if (!df_conflicts) {
> + int status = fast_forward_merge(n, dirmask, names, info);
> + if (status)
> + return status;
> + }
Also skip the optimization when sparse checkout is active
(info->data->skip_sparse_checkout == 0). People may need to just
update skip-worktree bits and add/remove worktree files along the
line.
Or you could make another step ahead and make fast_forward_merge aware
of sparse checkout too ;-)
--
Duy
^ permalink raw reply
* [minor BUG] cherry-pick -x doesn't work if a conflict occurs
From: Uwe Kleine-König @ 2010-12-29 14:16 UTC (permalink / raw)
To: git
Hello,
when hitting a conflict cherry-pick suggests using
git commit -c $sha1
but the resulting (suggested) commit log doesn't have the extra
reference requested by -x.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* Re: [PATCH 02/31] rebase: refactor reading of state
From: Martin von Zweigbergk @ 2010-12-29 8:09 UTC (permalink / raw)
To: Junio C Hamano
Cc: Martin von Zweigbergk, git, Johannes Schindelin, Johannes Sixt,
Christian Couder
In-Reply-To: <7v1v51v6l2.fsf@alter.siamese.dyndns.org>
On Tue, 28 Dec 2010, Junio C Hamano wrote:
> Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> writes:
>
> > +read_state () {
> > + if test -d "$merge_dir"
> > + then
> > + state_dir="$merge_dir"
> > + prev_head=$(cat "$merge_dir"/prev_head) &&
> > + end=$(cat "$merge_dir"/end) &&
> > + msgnum=$(cat "$merge_dir"/msgnum)
> > + else
> > + state_dir="$apply_dir"
> > + fi &&
> > + head_name=$(cat "$state_dir"/head-name) &&
> > + onto=$(cat "$state_dir"/onto) &&
> > + orig_head=$(cat "$state_dir"/orig-head) &&
> > + GIT_QUIET=$(cat "$state_dir"/quiet)
> > +}
> > +
> > continue_merge () {
> > test -n "$prev_head" || die "prev_head must be defined"
> > test -d "$merge_dir" || die "$merge_dir directory does not exist"
> > @@ -138,10 +154,6 @@ call_merge () {
> > }
> >
> > move_to_original_branch () {
> > - test -z "$head_name" &&
> > - head_name="$(cat "$merge_dir"/head-name)" &&
> > - onto="$(cat "$merge_dir"/onto)" &&
> > - orig_head="$(cat "$merge_dir"/orig-head)"
>
> It used to be safe to call this without head_name and friends set, because
> the function took care of reading these itself. Nobody calls this without
> head_name set anymore?
>
> I am not complaining nor suggesting to add an unnecessary "read_state"
> here only to slow things down---I am making sure that you removed this
> because you know it is unnecessary.
Correct. It used to be called without head_name set from
finish_rb_merge, which would in turn be called from the --continue and
--skip arms. onto would already have been set in these cases, but
would then be re-read.
> > @@ -220,13 +232,9 @@ do
> > echo "mark them as resolved using git add"
> > exit 1
> > }
> > + read_state
> > if test -d "$merge_dir"
> > then
> > - prev_head=$(cat "$merge_dir/prev_head")
> > - end=$(cat "$merge_dir/end")
> > - msgnum=$(cat "$merge_dir/msgnum")
> > - onto=$(cat "$merge_dir/onto")
> > - GIT_QUIET=$(cat "$merge_dir/quiet")
>
> Even though this patch may make the code shorter, it starts to read
> head_name and orig_head that we previously did not open and change the
> values of variables with what are read from them. Does this change affect
> the behaviour in any way (either in performance or in correctness)?
True. Previously, head_name and orig_head were lazily read only when
the rebase had been finished (in the finish_rb_merge, as I mentioned
above). Now these values are read unnecessarily when the rebase is
resumed, but a later patch fails. The am-based rebase is not affacted
by this patch as it already read head_name and orig_head eagerly.
Good point about the correctness. I looked into the correctness issue
arising if a file could not be found and I think I concluded that all
of the files would always be written (do we need to care about the
case where a user deletes e.g. .git/rebase-apply/onto?). However, I
did not think about the possibility of overwriting variables. It seems
fine, though, as neither continue_merge nor call_merge uses either of
these variables.
Regarding performance, I would say there is definitely a cost
associated with this patch. How big this cost is, though, I don't dare
to speculate. I will leave that up to the rest of you.
It should be noted that read_state is only called when a rebase is
resumed with --continue or --skip, or when it is aborted. There are no
changes to the code in the call_merge-continue_merge loop.
The performance hit is probably biggest in the --abort case, which
previously only read head_name and orig_head. It now ends up reading
_at least_ two more values.
>
> > @@ -236,10 +244,6 @@ do
> > finish_rb_merge
> > exit
> > fi
> > - head_name=$(cat "$apply_dir"/head-name) &&
> > - onto=$(cat "$apply_dir"/onto) &&
> > - orig_head=$(cat "$apply_dir"/orig-head) &&
> > - GIT_QUIET=$(cat "$apply_dir"/quiet)
> > git am --resolved --3way --resolvemsg="$RESOLVEMSG" &&
> > move_to_original_branch
>
> Earlier move-to-original-branch was Ok to be called without head_name, and
> the old code here read from the file anyway, so it didn't matter, but now
> it seems that the first check and assignment you removed from the function
> may matter because this caller does not even read from head_name. Are you
> sure about this change?
If I understand your question correctly, then yes, it is ok, because
of the previous hunk that calls read_state. That call is made
before/outside the if block for merge-based rebase, so the variables
are already set when this code is reached.
> > @@ -279,18 +275,15 @@ do
> > die "No rebase in progress?"
> >
> > git rerere clear
> > -
> > - test -d "$merge_dir" || merge_dir="$apply_dir"
>
> My heartbeat skipped when I first saw this. Thanks to the previous
> commit, it was exposed that somebody reused $dotest that was only to be
> used when using merge machinery because the things left to be done in this
> codepath are common between the merge and apply, which is kind of sloppy,
> but that sloppiness is now gone ;-).
>
> Are there places that read from individual files for states left after
> this patch, or read_state is the only interface to get to the states? If
> the latter that would be a great news, and also would suggest that we may
> want to have a corresponding write_state function (and we may even want to
> make the state into a single file to reduce I/O---but that is a separate
> issue that can be done at the very end of the series if it turns out to be
> beneficial).
There are still a few other places where state is read, mainly in
call_merge. It reads things that vary from iteration to iteration,
such as a counter. I forgot to say in the commit message, but I tried
to extract only the code that reads the initial state.
The write_state function actually is there, but it comes pretty late,
in patch 24. I don't remember why I added it so much later. I could
possibly move it closer to the beginning of this series.
> Of course it is fine if introduction of read_state is an attempt to catch
> most common cases, but it would reduce chances of mistake if the coverage
> were 100% (as opposed to 99.9%) hence this question.
Do you mean if all the state was read in the read_state function? I
should say that the pieces of state that are read in read_state are
not read anywhere else. But overall, the coverage is more like 60% or
so.
Thanks for a thorough review. Many of these things should have been in
the commit message. I need to get better at writing those...
Thanks,
Martin
^ permalink raw reply
* Re: [PATCH 13/14] t4135-*.sh: Skip the "backslash" tests on cygwin
From: Nguyen Thai Ngoc Duy @ 2010-12-29 13:56 UTC (permalink / raw)
To: Ramsay Jones; +Cc: Johannes Sixt, Junio C Hamano, GIT Mailing-list, jrnieder
In-Reply-To: <4D1A2827.6070503@ramsay1.demon.co.uk>
On Wed, Dec 29, 2010 at 1:10 AM, Ramsay Jones
<ramsay@ramsay1.demon.co.uk> wrote:
> Nguyen Thai Ngoc Duy wrote:
>> On Wed, Dec 22, 2010 at 2:31 AM, Ramsay Jones
>> <ramsay@ramsay1.demon.co.uk> wrote:
>>> The problem boils down to the call to strncmp_icase() suppressing the call to
>>> fnmatch() when the pattern contains glob chars, but the (remaining) string is
>>> equal to the name; thus returning an exact match (MATCHED_EXACTLY) rather than
>>> calling fnmatch (and returning either no-match or MATCHED_FNMATCH).
>>
>> I think that's expected behavior. Wildcard pathspecs are fixed
>> pathspecs will additional wildcard matching support and can match both
>> ways. See 186d604 (glossary: define pathspec)
>
> Really? Hmm, that seems ... odd! (To be clear: you are saying that an exact
> match, *even if the pattern contains glob chars*, takes precedence over the
> glob match! - again *odd*)
not exactly taking precedence. I would say it's "normal pathspecs with
extra capability", so it can match more
> Well, if you are happy with that ...
Well, we have two options here: either that, or declare it a day
(near) zero bug [1] with potentially massive impact. Personally I'd go
with which ever way that is less work :) as long as it does not annoy
me (much).
[1] Exerpt from 56fc510 ([PATCH] git-ls-files: generalized pathspecs -
2005-08-21): "the "matching" criteria is a combination of "exact path
component match" (the same as the git-diff-* family), and
"fnmatch()"." Git makes digging history fun.
--
Duy
^ 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