* Re: [PATCHv2 0/2] fixes to gitweb feature check code
From: Giuseppe Bilotta @ 2008-11-29 11:13 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git, Petr Baudis, Junio C Hamano
In-Reply-To: <200811291148.15182.jnareb@gmail.com>
2008/11/29 Jakub Narebski <jnareb@gmail.com>:
> Dnia piątek 28. listopada 2008 21:39, Giuseppe Bilotta napisał:
>> The first patch introduces git_get_feature() to clearly distinguish the
>> feature retrieval from the boolean feature check (which is kept at
>> git_check_feature()). The new function is used where appropriate.
> It is gitweb_get_feature() (for retrieval) and gitweb_check_feature()
> (for checking), not git_get_feature() and git_check_feature().
Ahem. Right. I'll resend the second patch with a fixed subject and
commit message 8-P
--
Giuseppe "Oblomov" Bilotta
^ permalink raw reply
* Re: [PATCHv2 0/2] fixes to gitweb feature check code
From: Jakub Narebski @ 2008-11-29 10:48 UTC (permalink / raw)
To: Giuseppe Bilotta; +Cc: git, Petr Baudis, Junio C Hamano
In-Reply-To: <1227904793-1821-1-git-send-email-giuseppe.bilotta@gmail.com>
Dnia piątek 28. listopada 2008 21:39, Giuseppe Bilotta napisał:
> This is v2 of the gitweb feature check fix patch, which has now been
> split into into a code patch and cleanup patch.
>
> The first patch introduces git_get_feature() to clearly distinguish the
> feature retrieval from the boolean feature check (which is kept at
> git_check_feature()). The new function is used where appropriate.
>
> The second patch cleans up use of git_check_feature(): since the
> function now returns a boolean instead of an array, the often-used
> construct
> my ($somevar) = git_check_feature('somefeat');
> although still valid, becomes a rather clumsy stylistic choice, as it
> introduces an unnecessary ambiguity. Make it clear that we're now
> dealing with scalars by using scalar assignment.
It is gitweb_get_feature() (for retrieval) and gitweb_check_feature()
(for checking), not git_get_feature() and git_check_feature().
I really like this series; making coding easier by removing need to
use special construct, and making it easier to avoid mistakes is always
a good thing.
> Giuseppe Bilotta (2):
> gitweb: fixes to gitweb feature check code
> gitweb: clean up git_check_feature() calls
>
> gitweb/gitweb.perl | 52 +++++++++++++++++++++++++++++++++++-----------------
> 1 files changed, 35 insertions(+), 17 deletions(-)
I also like the split, as it separates improvement (much easier to use
and less error prone gitweb_check_feature()) from style cleanup.
--
Jakub Narebski
Poland
^ permalink raw reply
* Re: timestamps not git-cloned
From: Thomas Rast @ 2008-11-29 10:16 UTC (permalink / raw)
To: Stephen R. van den Berg; +Cc: Chris Frey, jidanni, dhruvakm, git
In-Reply-To: <20081129092231.GA32630@cuci.nl>
[-- Attachment #1: Type: text/plain, Size: 1726 bytes --]
Stephen R. van den Berg wrote:
> Chris Frey wrote:
> >If this is the important bit, perhaps git-archive could be changed
> >to create tarballs with file timestamps based on their commit dates.
>
> Based on the principle of least surprise, I'd consider this a rather good
> idea.
Unless I'm missing something, this would make git-archive rather more
expensive than it is now: Tree objects do not record any timestamps,
so figuring out the last commit that changed a file requires a full
history walk in the worst case[*]. (This is another side-effect of
not versioning files.) On the other hand, current git-archive's
running time depends only on the size of the tree-ish given, including
all subtrees and blobs.
My unscientific guesstimates on how much work this would be, in a
random (old) linux-2.6 clone:
$ git rev-parse HEAD
e013e13bf605b9e6b702adffbe2853cfc60e7806
$ time git ls-tree -r -t $(git rev-list HEAD~5000..HEAD) >/dev/null
real 0m1.385s
user 0m1.164s
sys 0m0.220s
$ git rev-list HEAD | wc -l
117812
So reading (and dumping) all those trees and subtrees incurs a penalty
on the order of 30 seconds. Compare to the current running time of
git-archive:
$ time git archive --format=tar HEAD >/dev/null
real 0m2.790s
user 0m2.684s
sys 0m0.072s
Of course, the ratio will keep getting worse as history gets longer.
- Thomas
[*] I think to really have a "worst case" here, you need at least one
file in every leaf directory that has not changed since the root
commit, and another that changes in every commit to force the search
to really read every subtree.
--
Thomas Rast
trast@{inf,student}.ethz.ch
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]
^ permalink raw reply
* Re: timestamps not git-cloned
From: Stephen R. van den Berg @ 2008-11-29 9:22 UTC (permalink / raw)
To: Chris Frey; +Cc: jidanni, dhruvakm, git
In-Reply-To: <20081129085406.GA20428@foursquare.net>
Chris Frey wrote:
>On Fri, Nov 28, 2008 at 01:06:00PM +0800, jidanni@jidanni.org wrote:
>> I hate it when I get some latest WhizBang.tgz, only to untar it to
>> find all the files' dates the same, when in fact the README hasn't
>> been touched in seven years, but you can't tell that from ls -l. I
>> recall some content tracker was involved.
>If this is the important bit, perhaps git-archive could be changed
>to create tarballs with file timestamps based on their commit dates.
Based on the principle of least surprise, I'd consider this a rather good
idea.
--
Sincerely,
Stephen R. van den Berg.
To people that say "I could care less" - well, why don't you?
^ permalink raw reply
* Re: timestamps not git-cloned
From: Chris Frey @ 2008-11-29 8:54 UTC (permalink / raw)
To: jidanni; +Cc: dhruvakm, git
In-Reply-To: <87tz9sv3rb.fsf@jidanni.org>
On Fri, Nov 28, 2008 at 01:06:00PM +0800, jidanni@jidanni.org wrote:
> I hate it when I get some latest WhizBang.tgz, only to untar it to
> find all the files' dates the same, when in fact the README hasn't
> been touched in seven years, but you can't tell that from ls -l. I
> recall some content tracker was involved.
If this is the important bit, perhaps git-archive could be changed
to create tarballs with file timestamps based on their commit dates.
- Chris
^ permalink raw reply
* [PATCH 3/3] git add --intent-to-add: do not let an empty blob committed by accident
From: Junio C Hamano @ 2008-11-29 3:56 UTC (permalink / raw)
To: Shawn O. Pearce; +Cc: Johannes Schindelin, git
In-Reply-To: <7vvdu72nq9.fsf@gitster.siamese.dyndns.org>
Writing a tree out of an index with an "intent to add" entry is blocked.
This implies that you cannot "git commit" from such a state; however "git
commit -a" still works.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin-write-tree.c | 2 +-
cache-tree.c | 10 +++++++---
t/t3701-add-interactive.sh | 18 ++++++++++++++++++
3 files changed, 26 insertions(+), 4 deletions(-)
diff --git a/builtin-write-tree.c b/builtin-write-tree.c
index 52a3c01..9d64050 100644
--- a/builtin-write-tree.c
+++ b/builtin-write-tree.c
@@ -42,7 +42,7 @@ int cmd_write_tree(int argc, const char **argv, const char *unused_prefix)
die("%s: error reading the index", me);
break;
case WRITE_TREE_UNMERGED_INDEX:
- die("%s: error building trees; the index is unmerged?", me);
+ die("%s: error building trees", me);
break;
case WRITE_TREE_PREFIX_ERROR:
die("%s: prefix %s not found", me, prefix);
diff --git a/cache-tree.c b/cache-tree.c
index 5f8ee87..3d8f218 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -155,13 +155,17 @@ static int verify_cache(struct cache_entry **cache,
funny = 0;
for (i = 0; i < entries; i++) {
struct cache_entry *ce = cache[i];
- if (ce_stage(ce)) {
+ if (ce_stage(ce) || (ce->ce_flags & CE_INTENT_TO_ADD)) {
if (10 < ++funny) {
fprintf(stderr, "...\n");
break;
}
- fprintf(stderr, "%s: unmerged (%s)\n",
- ce->name, sha1_to_hex(ce->sha1));
+ if (ce_stage(ce))
+ fprintf(stderr, "%s: unmerged (%s)\n",
+ ce->name, sha1_to_hex(ce->sha1));
+ else
+ fprintf(stderr, "%s: not added yet\n",
+ ce->name);
}
}
if (funny)
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index e95663d..473ef85 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -133,6 +133,24 @@ test_expect_success 'real edit works' '
test_cmp expected output
'
+test_expect_success 'cannot commit with i-t-a entry' '
+ git reset --hard &&
+ echo xyzzy >rezrov &&
+ echo frotz >nitfol &&
+ git add rezrov &&
+ git add -N nitfol &&
+ test_must_fail git commit
+'
+
+test_expect_success 'can commit with an unrelated i-t-a entry in index' '
+ git reset --hard &&
+ echo xyzzy >rezrov &&
+ echo frotz >nitfol &&
+ git add rezrov &&
+ git add -N nitfol &&
+ git commit -m partial rezrov
+'
+
if test "$(git config --bool core.filemode)" = false
then
say 'skipping filemode tests (filesystem does not properly support modes)'
--
1.6.0.4.850.g6bd829
^ permalink raw reply related
* [PATCH 2/3] git add --intent-to-add: fix removal of cached emptiness
From: Junio C Hamano @ 2008-11-29 3:55 UTC (permalink / raw)
To: Shawn O. Pearce; +Cc: Johannes Schindelin, git
In-Reply-To: <7vvdu72nq9.fsf@gitster.siamese.dyndns.org>
This uses the extended index flag mechanism introduced earlier to mark
the entries added to the index via "git add -N" with CE_INTENT_TO_ADD.
The logic to detect an "intent to add" entry for the purpose of allowing
"git rm --cached $path" is tightened to check not just for a staged empty
blob, but with the CE_INTENT_TO_ADD bit. This protects an empty blob that
was explicitly added and then modified in the work tree from being dropped
with this sequence:
$ >empty
$ git add empty
$ echo "non empty" >empty
$ git rm --cached empty
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin-rm.c | 2 +-
cache.h | 3 ++-
read-cache.c | 2 ++
t/t3600-rm.sh | 4 ++--
4 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/builtin-rm.c b/builtin-rm.c
index 3d03da0..c11f455 100644
--- a/builtin-rm.c
+++ b/builtin-rm.c
@@ -115,7 +115,7 @@ static int check_local_mod(unsigned char *head, int index_only)
* "intent to add" entry.
*/
if (local_changes && staged_changes) {
- if (!index_only || !is_empty_blob_sha1(ce->sha1))
+ if (!index_only || !(ce->ce_flags & CE_INTENT_TO_ADD))
errs = error("'%s' has staged content different "
"from both the file and the HEAD\n"
"(use -f to force removal)", name);
diff --git a/cache.h b/cache.h
index ef2e7f9..f15b3fc 100644
--- a/cache.h
+++ b/cache.h
@@ -176,10 +176,11 @@ struct cache_entry {
/*
* Extended on-disk flags
*/
+#define CE_INTENT_TO_ADD 0x20000000
/* CE_EXTENDED2 is for future extension */
#define CE_EXTENDED2 0x80000000
-#define CE_EXTENDED_FLAGS (0)
+#define CE_EXTENDED_FLAGS (CE_INTENT_TO_ADD)
/*
* Safeguard to avoid saving wrong flags:
diff --git a/read-cache.c b/read-cache.c
index abc627b..fa30a0f 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -546,6 +546,8 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,
ce->ce_flags = namelen;
if (!intent_only)
fill_stat_cache_info(ce, st);
+ else
+ ce->ce_flags |= CE_INTENT_TO_ADD;
if (trust_executable_bit && has_symlinks)
ce->ce_mode = create_ce_mode(st_mode);
diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index 5b4d6f7..b7d46e5 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -187,8 +187,8 @@ test_expect_success 'but with -f it should work.' '
test_must_fail git ls-files --error-unmatch baz
'
-test_expect_failure 'refuse to remove cached empty file with modifications' '
- touch empty &&
+test_expect_success 'refuse to remove cached empty file with modifications' '
+ >empty &&
git add empty &&
echo content >empty &&
test_must_fail git rm --cached empty
--
1.6.0.4.850.g6bd829
^ permalink raw reply related
* [PATCH 1/3] builtin-rm.c: explain and clarify the "local change" logic
From: Junio C Hamano @ 2008-11-29 3:51 UTC (permalink / raw)
To: Shawn O. Pearce; +Cc: Johannes Schindelin, git
In-Reply-To: <7vk5an4cba.fsf_-_@gitster.siamese.dyndns.org>
Explain the logic to check local modification a bit more in the comment,
especially because the existing comment that talks about "git rm --cached"
was placed in a part that was not about "--cached" at all.
Also clarify "if .. else if .." structure.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* This is the first patch of a re-rolled sesries of the previous one, and
again applies to the master plus the first commit from nd/narrow.
builtin-rm.c | 53 ++++++++++++++++++++++++++++++++++++++++++-----------
1 files changed, 42 insertions(+), 11 deletions(-)
diff --git a/builtin-rm.c b/builtin-rm.c
index b7126e3..3d03da0 100644
--- a/builtin-rm.c
+++ b/builtin-rm.c
@@ -31,7 +31,8 @@ static void add_list(const char *name)
static int check_local_mod(unsigned char *head, int index_only)
{
- /* items in list are already sorted in the cache order,
+ /*
+ * Items in list are already sorted in the cache order,
* so we could do this a lot more efficiently by using
* tree_desc based traversal if we wanted to, but I am
* lazy, and who cares if removal of files is a tad
@@ -71,25 +72,55 @@ static int check_local_mod(unsigned char *head, int index_only)
*/
continue;
}
+
+ /*
+ * "rm" of a path that has changes need to be treated
+ * carefully not to allow losing local changes
+ * accidentally. A local change could be (1) file in
+ * work tree is different since the index; and/or (2)
+ * the user staged a content that is different from
+ * the current commit in the index.
+ *
+ * In such a case, you would need to --force the
+ * removal. However, "rm --cached" (remove only from
+ * the index) is safe if the index matches the file in
+ * the work tree or the HEAD commit, as it means that
+ * the content being removed is available elsewhere.
+ */
+
+ /*
+ * Is the index different from the file in the work tree?
+ */
if (ce_match_stat(ce, &st, 0))
local_changes = 1;
+
+ /*
+ * Is the index different from the HEAD commit? By
+ * definition, before the very initial commit,
+ * anything staged in the index is treated by the same
+ * way as changed from the HEAD.
+ */
if (no_head
|| get_tree_entry(head, name, sha1, &mode)
|| ce->ce_mode != create_ce_mode(mode)
|| hashcmp(ce->sha1, sha1))
staged_changes = 1;
- if (local_changes && staged_changes &&
- !(index_only && is_empty_blob_sha1(ce->sha1)))
- errs = error("'%s' has staged content different "
- "from both the file and the HEAD\n"
- "(use -f to force removal)", name);
+ /*
+ * If the index does not match the file in the work
+ * tree and if it does not match the HEAD commit
+ * either, (1) "git rm" without --cached definitely
+ * will lose information; (2) "git rm --cached" will
+ * lose information unless it is about removing an
+ * "intent to add" entry.
+ */
+ if (local_changes && staged_changes) {
+ if (!index_only || !is_empty_blob_sha1(ce->sha1))
+ errs = error("'%s' has staged content different "
+ "from both the file and the HEAD\n"
+ "(use -f to force removal)", name);
+ }
else if (!index_only) {
- /* It's not dangerous to "git rm --cached" a
- * file if the index matches the file or the
- * HEAD, since it means the deleted content is
- * still available somewhere.
- */
if (staged_changes)
errs = error("'%s' has changes staged in the index\n"
"(use --cached to keep the file, "
--
1.6.0.4.850.g6bd829
^ permalink raw reply related
* Re: What's cooking in git.git (Nov 2008, #06; Wed, 26)
From: Daniel Barkalow @ 2008-11-29 1:25 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Shawn O. Pearce, Johannes Schindelin, git
In-Reply-To: <7voczz4cfb.fsf@gitster.siamese.dyndns.org>
On Fri, 28 Nov 2008, Junio C Hamano wrote:
> "Shawn O. Pearce" <spearce@spearce.org> writes:
>
> > Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> > ...
> >> In other words, unless there is more interest in that feature, enough to
> >> generate a well-understood design before a good implementation, I'd rather
> >> see this patch series dropped.
> >
> > Ack. I agree with every remark made by Dscho, and also want to cry "wolf".
> >
> > I haven't had time to read the patch series. Its big and intrusive
> > and I just don't need the feature.
>
> Well, "me neither". Although I personally think resisting changes until
> it becomes absolutely necessary is a good discipline, we also need to
> recognise that there is a chicken-and-egg problem. When you have a
> potentially useful feature, unless people actually try using it in the
> field, you won't discover the drawbacks in either the design nor the
> implementation, let alone any improvements.
I just looked over most of it (skipping the generic index extension
portion). It looks to me like it's introducing an extra concept to avoid
actually fixing maybe-bugs in the "assume unchanged" implementation
when used with files that have been changed intentionally (with the user
intending git to overlook this change). Sparse checkout is essentially a
special case of this, where the user has changed the working directory
radically (not populating it at all) and wants git to carry on as if this
was not the case (with a certain amount of porcelain code to cause this to
happen automatically).
If there's any need for this to be distinguished from "assume unchanged",
I think it should be used with, not instead of, the CE_VALID bit; and it
could probably use some bit in the stat info section, since we don't need
stat info if we know by assumption that the entry is valid.
-Daniel
*This .sig left intentionally blank*
^ permalink raw reply
* Re: summaries in git add --patch
From: William Pursell @ 2008-11-29 0:22 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7v8wr48g98.fsf@gitster.siamese.dyndns.org>
Junio C Hamano wrote:
> William Pursell <bill.pursell@gmail.com> writes:
>
>> Here's a new patch. Instead of displaying the summary and then
>> the current hunk, it implements a 'goto' command.
>
> I take it that this is for discussion not for immediate inclusion.
>
Yes. I tend to think of all of my patches as being merely
for discussion since I'm not terribly familiar with the code
base and expect to miss many things. I'm flattered that
you would even consider them for inclusion. For that matter,
I'm flattered that you have even responded to my submissions!
>> @@ -799,6 +801,7 @@ sub help_patch_cmd {
>> y - stage this hunk
>> n - do not stage this hunk
>> a - stage this and all the remaining hunks in the file
>> +g - select a hunk to jump to
>> d - do not stage this hunk nor any of the remaining hunks in the file
>> j - leave this hunk undecided, see next undecided hunk
>> J - leave this hunk undecided, see next hunk
>
> Since you took 'g' after "go to", help text should also say "go to",
> instead of "jump to" for the mnemonics value, iow, to help people
> remember.
Agreed.
>> @@ -836,6 +839,27 @@ sub patch_update_cmd {
>> }
>> }
>>
>> +sub select_new_hunk {
>> + my $ri = shift;
>> + my @hunk = @_;
>> + my ($i, $response);
>> + print " '+' stage, '-' don't stage\n";
>> + for ( $i = 0; $i < @hunk; $i++ ) {
>> + my $status = " ";
>> + if( defined $hunk[$i]{USE} ) {
>> + $status = $hunk[$i]{USE} ? "+" : "-";
>> + }
>
> Style.
>
> (1) SP between language construct and open parenthesis, as opposed to
> no extra SP between function name and open parenthesis;
>
> (2) No extra SP around what is enclosed in parentheses.
My apologies for that. I do try to conform, but this
sort of habit is hard to change. Especially in perl,
where code so often looks like a cartoon character's
speech bubble while swearing (eg #@$!%#@@), I
like to put space inside my parens. I'm fully aware
that this is not the preferred style here, and I
am trying to conform. Is there a style validating
pre-commit hook script available?
>> + printf "%s%3d: %s",
>> + $status,
>> + $i + 1,
>> + $hunk[$i]{SUMMARY};
>> + }
>
> I think this "for ()" loop part, including the comment about +/- notation,
> should be separated into a function so that you can implement a separate
> "l"ist command like you did in the other patch, using the same function.
My thought is that 'g' would replace 'l', but as per your previous
email 'l' would be a reasonable status command, so it
makes sense to factor it out.
>
>> + printf "goto which hunk? ";
>> + $response = <STDIN>;
>> + chomp $response;
>> + $$ri = $response - 1;
>
> What happens when $response is (1) a non number, (2) outside range (both
> negative and positive), or (3) EOF?
>
> Sending ref to scalar and returning the value by assigning is a bad taste.
> Why shouldn't this function just return an integer to be assigned to $ix
> by the caller? If you want to use pass-by-ref to show off your Perl-fu, I
> think \@hunk would be what you would want to for performance reasons.
Ack. I have no Perl-fu, I'm just not familiar with the
idioms and thought this was accepted in perl. I agree
that it's poor judgement, and can only attribute my
usage of it here to laziness. ( At one point I was passing
$ix, and when I realized I wanted to modify it at the caller
it was just easier to pass by reference.)
>
>> @@ -919,7 +943,7 @@ sub patch_update_file {
>> for (@{$hunk[$ix]{DISPLAY}}) {
>> print;
>> }
>> - print colored $prompt_color, "Stage this hunk [y/n/a/d$other/?]? ";
>> + print colored $prompt_color, "Stage this hunk [y/n/a/d/g$other/?]? ";
>
> When there is only one hunk, we do not give j nor k. Should we give g in
> such a case? Why?
I would agree that g should be invalid when only one hunk is
available. I hadn't considered that case.
>
>> @@ -937,6 +961,16 @@ sub patch_update_file {
>> }
>> next;
>> }
>> + elsif ($line =~ /^g/) {
>> + chomp ($line);
>> + if ($line =~ /^g$/) {
>> + select_new_hunk (\$ix, @hunk);
>> + }
>> + else {
>> + $ix = (substr $line, 1) - 1;
>> + }
>
> The same "input validation" issue exists here. it would make sense to:
>
> - Make choose_hunk(@hunk) that calls list_hunks(@hunk) that gives the
> summary, reads one line, and returns that line;
>
> - Make the caller here to look like this:
>
> elsif ($line =~ s/^g//) {
> chomp($line);
> if ($line eq '') {
> $line = choose_hunk(@hunk);
> }
> if ($line !~ /^\d+$/) {
> print STDERR "Eh '$line', what number is that?\n";
> next;
> } elsif (0 < $line && $line <= $num) {
> $ix = $line - 1;
> } else {
> print STDERR "Sorry, you have only $num hunks\n";
> }
> }
>
>> + next;
>> + }
>> elsif ($line =~ /^d/i) {
>> while ($ix < $num) {
>> if (!defined $hunk[$ix]{USE}) {
I will try to incorporate your ideas into a workable,
includable patch. I think the '/' regex search can
become part of the choose_hunk() routine so that
an integer response means select by number while
a '/re' response means jump forward to next matching
hunk. Also, instead of storing the summary line in
the hunk, it will probably be better to generate on
the fly during the display routine.
Thanks for the feedback.
--
William Pursell
^ permalink raw reply
* [PATCH] git add --intent-to-add: fix removal of cached emptiness
From: Junio C Hamano @ 2008-11-29 0:15 UTC (permalink / raw)
To: Shawn O. Pearce; +Cc: Johannes Schindelin, git
In-Reply-To: <7voczz4cfb.fsf@gitster.siamese.dyndns.org>
This uses the extended index flag mechanism introduced earlier to mark
the entries added to the index via "git add -N" with CE_INTENT_TO_ADD.
The logic to detect an "intent to add" entry for the purpose of allowing
"git rm --cached $path" is tightened to check not just for a staged empty
blob, but with the CE_INTENT_TO_ADD bit. This protects an empty blob that
was explicitly added and then modified in the work tree from being dropped
with this sequence:
$ >empty
$ git add empty
$ echo "non empty" >empty
$ git rm --cached empty
An index an "intent to add" entry is blocked. This implies that you
cannot "git commit" from such a state; however "git commit -a" still
works.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* This applies on top of the result of merging commit 06aaaa0 (Extend
index to save more flags, 2008-10-01) from nd/narrow topic into 'master'.
builtin-rm.c | 11 ++++++-----
builtin-write-tree.c | 2 +-
cache-tree.c | 10 +++++++---
cache.h | 3 ++-
read-cache.c | 2 ++
t/t3600-rm.sh | 4 ++--
t/t3701-add-interactive.sh | 18 ++++++++++++++++++
7 files changed, 38 insertions(+), 12 deletions(-)
diff --git a/builtin-rm.c b/builtin-rm.c
index b7126e3..8debcec 100644
--- a/builtin-rm.c
+++ b/builtin-rm.c
@@ -73,14 +73,15 @@ static int check_local_mod(unsigned char *head, int index_only)
}
if (ce_match_stat(ce, &st, 0))
local_changes = 1;
- if (no_head
- || get_tree_entry(head, name, sha1, &mode)
- || ce->ce_mode != create_ce_mode(mode)
- || hashcmp(ce->sha1, sha1))
+ if (no_head ||
+ ((get_tree_entry(head, name, sha1, &mode)
+ || ce->ce_mode != create_ce_mode(mode)
+ || hashcmp(ce->sha1, sha1)) &&
+ !(ce->ce_flags & CE_INTENT_TO_ADD)))
staged_changes = 1;
if (local_changes && staged_changes &&
- !(index_only && is_empty_blob_sha1(ce->sha1)))
+ !(index_only && (ce->ce_flags & CE_INTENT_TO_ADD)))
errs = error("'%s' has staged content different "
"from both the file and the HEAD\n"
"(use -f to force removal)", name);
diff --git a/builtin-write-tree.c b/builtin-write-tree.c
index 52a3c01..9d64050 100644
--- a/builtin-write-tree.c
+++ b/builtin-write-tree.c
@@ -42,7 +42,7 @@ int cmd_write_tree(int argc, const char **argv, const char *unused_prefix)
die("%s: error reading the index", me);
break;
case WRITE_TREE_UNMERGED_INDEX:
- die("%s: error building trees; the index is unmerged?", me);
+ die("%s: error building trees", me);
break;
case WRITE_TREE_PREFIX_ERROR:
die("%s: prefix %s not found", me, prefix);
diff --git a/cache-tree.c b/cache-tree.c
index 5f8ee87..3d8f218 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -155,13 +155,17 @@ static int verify_cache(struct cache_entry **cache,
funny = 0;
for (i = 0; i < entries; i++) {
struct cache_entry *ce = cache[i];
- if (ce_stage(ce)) {
+ if (ce_stage(ce) || (ce->ce_flags & CE_INTENT_TO_ADD)) {
if (10 < ++funny) {
fprintf(stderr, "...\n");
break;
}
- fprintf(stderr, "%s: unmerged (%s)\n",
- ce->name, sha1_to_hex(ce->sha1));
+ if (ce_stage(ce))
+ fprintf(stderr, "%s: unmerged (%s)\n",
+ ce->name, sha1_to_hex(ce->sha1));
+ else
+ fprintf(stderr, "%s: not added yet\n",
+ ce->name);
}
}
if (funny)
diff --git a/cache.h b/cache.h
index ef2e7f9..f15b3fc 100644
--- a/cache.h
+++ b/cache.h
@@ -176,10 +176,11 @@ struct cache_entry {
/*
* Extended on-disk flags
*/
+#define CE_INTENT_TO_ADD 0x20000000
/* CE_EXTENDED2 is for future extension */
#define CE_EXTENDED2 0x80000000
-#define CE_EXTENDED_FLAGS (0)
+#define CE_EXTENDED_FLAGS (CE_INTENT_TO_ADD)
/*
* Safeguard to avoid saving wrong flags:
diff --git a/read-cache.c b/read-cache.c
index abc627b..fa30a0f 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -546,6 +546,8 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,
ce->ce_flags = namelen;
if (!intent_only)
fill_stat_cache_info(ce, st);
+ else
+ ce->ce_flags |= CE_INTENT_TO_ADD;
if (trust_executable_bit && has_symlinks)
ce->ce_mode = create_ce_mode(st_mode);
diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index 5b4d6f7..b7d46e5 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -187,8 +187,8 @@ test_expect_success 'but with -f it should work.' '
test_must_fail git ls-files --error-unmatch baz
'
-test_expect_failure 'refuse to remove cached empty file with modifications' '
- touch empty &&
+test_expect_success 'refuse to remove cached empty file with modifications' '
+ >empty &&
git add empty &&
echo content >empty &&
test_must_fail git rm --cached empty
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index e95663d..473ef85 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -133,6 +133,24 @@ test_expect_success 'real edit works' '
test_cmp expected output
'
+test_expect_success 'cannot commit with i-t-a entry' '
+ git reset --hard &&
+ echo xyzzy >rezrov &&
+ echo frotz >nitfol &&
+ git add rezrov &&
+ git add -N nitfol &&
+ test_must_fail git commit
+'
+
+test_expect_success 'can commit with an unrelated i-t-a entry in index' '
+ git reset --hard &&
+ echo xyzzy >rezrov &&
+ echo frotz >nitfol &&
+ git add rezrov &&
+ git add -N nitfol &&
+ git commit -m partial rezrov
+'
+
if test "$(git config --bool core.filemode)" = false
then
say 'skipping filemode tests (filesystem does not properly support modes)'
--
1.6.0.4.850.g6bd829
^ permalink raw reply related
* Re: What's cooking in git.git (Nov 2008, #06; Wed, 26)
From: Junio C Hamano @ 2008-11-29 0:13 UTC (permalink / raw)
To: Shawn O. Pearce; +Cc: Johannes Schindelin, git
In-Reply-To: <20081128192033.GF23984@spearce.org>
"Shawn O. Pearce" <spearce@spearce.org> writes:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> ...
>> In other words, unless there is more interest in that feature, enough to
>> generate a well-understood design before a good implementation, I'd rather
>> see this patch series dropped.
>
> Ack. I agree with every remark made by Dscho, and also want to cry "wolf".
>
> I haven't had time to read the patch series. Its big and intrusive
> and I just don't need the feature.
Well, "me neither". Although I personally think resisting changes until
it becomes absolutely necessary is a good discipline, we also need to
recognise that there is a chicken-and-egg problem. When you have a
potentially useful feature, unless people actually try using it in the
field, you won't discover the drawbacks in either the design nor the
implementation, let alone any improvements.
> But I feel like if it were in fact merged I'll fall over some bug
> in it sometime soon and be forced to stop and debug it.
Exactly. That is how you make progress.
Having said that, I am willing to carry it over in 'next' outside 'master'
for the 1.6.1 cycle, as three people who are most likely to be able to fix
any potential issues are not using that feature.
> Heck at
> the least I'll have to go back to JGit's index code and implement
> the new file format.
I am sorry to dissapoint you but I am planning to use the first one in the
series, which is the one that adds extended index flag bits, for the fix
to an unrelated feature.
^ permalink raw reply
* Re: remote branch checkout issue, why its *(no branch)
From: Björn Steinbrink @ 2008-11-28 23:24 UTC (permalink / raw)
To: Jakub Narebski; +Cc: Sudipta GHOSH, git
In-Reply-To: <m3k5anjxcd.fsf@localhost.localdomain>
On 2008.11.28 14:32:56 -0800, Jakub Narebski wrote:
> What you can do is to create _local_ branch, closely tied to
> remote-tracking branch 'origin/B1', by using:
>
> $ git branch -b B1 --track origin/B1
You mixed up checkout and branch there ;-)
It's either
git branch B1 origin/B1 # Just create a branch B1
or
git checkout -b B1 origin/B1 # Create and checkout B1
I omitted the "--track" as, by default, it is implied when you create a
new branch from a remote tracking branch.
Björn
^ permalink raw reply
* Re: Grafting mis-aligned trees.
From: Boyd Stephen Smith Jr. @ 2008-11-28 23:01 UTC (permalink / raw)
To: git; +Cc: Michael J Gruber
In-Reply-To: <492289B0.4010903@drmicha.warpmail.net>
[-- Attachment #1: Type: text/plain, Size: 2827 bytes --]
On Tuesday 2008 November 18 03:24, Michael J Gruber wrote:
> Boyd Stephen Smith Jr. venit, vidit, dixit 17.11.2008 23:45:
> > I haven't gotten a response from my subscription email, so please CC me
> > on any replies.
> >
> > So, I've been managaing the source I had from a client project in git and
> > have a non-linear history. Currently, two tips (production and testing)
> > but there are many feature branches that were git-merge'd in, not
> > rebased.
> >
> > Now, I've gotten the full tree. Turns out all the source code I was
> > working on was in a subdirectory "project/web". I'd like to "graft" the
> > *changes* I made onto the full tree.
> >
> > I figured this might be a job for git-filter-branch. Certainly, that did
> > the job of moving all my changes into the subdirectory. But, now I want
> > to do something that's a combination or git-rebase and git-filter-branch.
> > I want to replay the *patches/deltas* (like rebase) on top of the full
> > tree I have, but *maintain the non-liear history* (like filter-branch).
> >
> > Can anyone think of a recipe for me?
> >
> > Trees look something like this right now.
> >
> > <some history> -> FT
> >
> > TI -> <non-linear history> -> A -> <non-linear history> -> C
> > \ \ \
> > -> PI ------------------------> B ------------------------> D
> >
> > I'd like to have it look something like:
> >
> > <some history> -> FT -> <non-linear history> -> A' -> <non-linear
> > history> -> C' \ \ \
> > -> PI' ----------------------> B' -----------------------> D'
> >
> > A', B', C', and D' are different commits, but the diff (and history)
> > between FT and A' is the same as the diff (and history) between TI and A.
> >
> > Again, please CC me on any replies.
>
> [CCing is customary here anyways.]
>
> So, your base directory for TI and FT is different, right? I.e.: In the
> TI repo, your project sits at the root, whereas in the FT repo it sits
> in project/web?
Yes.
> Has FT advanced since you took the initial subdir
> snapshot for TI?
No. Well, maybe. I think the subdir diff is fairly trivial if not empty. TI
is an import from the code actually present on the testing server. FT was
the a subversion repository obtained later after some hullabaloo with the
ex-development house.
Right now this tree is effectively all mine, so I can always graft in commits
to synchronize the common subtree of FT and TI, if that makes things easier.
--
Boyd Stephen Smith Jr. ,= ,-_-. =.
bss03@volumehost.net ((_/)o o(\_))
ICQ: 514984 YM/AIM: DaTwinkDaddy `-'(. .)`-'
http://iguanasuicide.org/ \_/
[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply
* Re: Removing part of the history
From: Jakub Narebski @ 2008-11-28 22:38 UTC (permalink / raw)
To: Samuel Lucas Vaz de Mello; +Cc: git
In-Reply-To: <49306150.6010701@datacom.ind.br>
Samuel Lucas Vaz de Mello <samuellucas@datacom.ind.br> writes:
> I have a repository with code imported from CVS.
>
> I first imported from CVS using git 1.5.6 and we started to develop
> in git branches. I run cvsimport periodically and merged the
> changes from CVS (legacy developers) to our git branches. After
> some time, I realized that the import was broken, upgraded to git
> 1.6 and reimported the data. Now the imported data seems to be
> correct and I finished with a history tree like this:
>
> (our branch1) X--Y--Z
> merges> / \
> (our branch2) J--K--L--M--N--O--P
> merges> / / /
> (cvs old) ...A--B--C--D--E /
> merges> /
> (cvs new) ...A'-B'-C'-D'-E'-F'
>
>
> There is any way to get rid of the (broken) cvs-old tree?
>
> I've tried to rebase, but the commits from A' and A are different,
> it tries to reapply all history in CVS.
>
> I've tried to checkout a point before our changes start (B') and
> then, using a script, cherry-pick all commits up to HEAD that are
> not in cvs-old branch. This approach didn't handled well merges
> between our branches (K-L-M and X-Y).
>
> The history from the beginning up to the point where our changes
> started (...A--B) is quite large, so it would be nice for us to get
> rid of it.
>
> Any suggestion on how to handle this?
You can use grafts (see "Repository Layout" documentation) to prune
'cvs old' branches, by adding for each merge commit <c_i> with one
side leading to 'cvs old' branch line to .git/info/grafts
<c_i> <c_i^1>
where <c_i> is full hexadecimal object name.
Then check in gitk or some other history viewer if the history now
looks like you wanted (looks sane).
Then use "git filter-branch --all" to turn grafts into reality, check
out correctness, then remove refs/original/* and do git-gc.
HTH
--
Jakub Narebski
Poland
ShadeHawk on #git
^ permalink raw reply
* Re: remote branch checkout issue, why its *(no branch)
From: Jakub Narebski @ 2008-11-28 22:32 UTC (permalink / raw)
To: Sudipta GHOSH; +Cc: git
In-Reply-To: <9e6474ae0811281415n7b4596afq7b3ce25816d9a639@mail.gmail.com>
"Sudipta GHOSH" <sudipta.in@gmail.com> writes:
> In our git repository there are few branches like
>
> master
> origin/B1
> origin/B2
>
> After doing #git checkout origin/B1 I found that my current branch is
> *(no branch)
>
> #git branch
> *(no branch)
> master
>
> Why its showing *(no branch) why not origin/B1
>
> How I can checkout origin/B1 and after that my current branch will be
> origin/B1 not the *(no branch)
First, 'origin/B1' is so called remote-tracking branch, and its full
name is refs/remotes/origin/B1. Those branches are updated
_automatically_ by git on fetch, to follow branch 'B1' in 'origin'
repository. Therefore, to not lose _your_ changes git doesn't allow
checking out remote-tracking branches. You can checkout _state_ of
such branch into anonymous noname branch, so called in git jargon
"detached HEAD".
What you can do is to create _local_ branch, closely tied to
remote-tracking branch 'origin/B1', by using:
$ git branch -b B1 --track origin/B1
Git would set up configuration for this branch in such way that "git
pull" command given on 'B1' branch would "do the right thing"(TM)
I really recommend reading "Git User's Manual" or "The Git Community
Book" first...
--
Jakub Narebski
Poland
ShadeHawk on #git
^ permalink raw reply
* Re: [PATCH] git-p4: fix keyword-expansion regex
From: Junio C Hamano @ 2008-11-28 22:29 UTC (permalink / raw)
To: Simon Hausmann; +Cc: Pete Wyckoff, git
In-Reply-To: <200811271745.30963.simon@lst.de>
Simon Hausmann <simon@lst.de> writes:
> On Wednesday 26 November 2008 Junio C Hamano, wrote:
>> I will queue this to 'pu' and wait for an Ack from git-p4 folks. Thanks.
>
> Acked-by: Simon Hausmann <simon@lst.de>
Thanks.
^ permalink raw reply
* remote branch checkout issue, why its *(no branch)
From: Sudipta GHOSH @ 2008-11-28 22:15 UTC (permalink / raw)
To: git
Hi,
In Our git repository there are few branches like
master
orgin/B1
origin/B2
After doing #git checkout orgin/B1 I found that my current branch is
*(no branch)
#git branch
*(no branch)
master
Why its showing *(no branch) why not origin/B1
How I can checkout orgin/B1 and after that my current branch will be
orgin/B1 not the *(no branch)
Regards,
-SG
^ permalink raw reply
* Re: Removing part of the history
From: Miklos Vajna @ 2008-11-28 21:34 UTC (permalink / raw)
To: Samuel Lucas Vaz de Mello; +Cc: git
In-Reply-To: <49306150.6010701@datacom.ind.br>
[-- Attachment #1: Type: text/plain, Size: 639 bytes --]
On Fri, Nov 28, 2008 at 07:23:28PM -0200, Samuel Lucas Vaz de Mello <samuellucas@datacom.ind.br> wrote:
> There is any way to get rid of the (broken) cvs-old tree?
>
> I've tried to rebase, but the commits from A' and A are different, it
> tries to reapply all history in CVS.
Have you tried git rebase --onto?
> I've tried to checkout a point before our changes start (B') and then,
> using a script, cherry-pick all commits up to HEAD that are not in
> cvs-old branch. This approach didn't handled well merges between our
> branches (K-L-M and X-Y).
git rebase --preserve-merges is probably what you are searching for.
[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]
^ permalink raw reply
* Removing part of the history
From: Samuel Lucas Vaz de Mello @ 2008-11-28 21:23 UTC (permalink / raw)
To: git
Hi All,
I have a repository with code imported from CVS.
I first imported from CVS using git 1.5.6 and we started to develop in git branches.
I run cvsimport periodically and merged the changes from CVS (legacy developers) to our git branches.
After some time, I realized that the import was broken, upgraded to git 1.6 and reimported the data.
Now the imported data seems to be correct and I finished with a history tree like this:
(our branch1) X--Y--Z
merges> / \
(our branch2) J--K--L--M--N--O--P
merges> / / /
(cvs old) ...A--B--C--D--E /
merges> /
(cvs new) ...A'-B'-C'-D'-E'-F'
There is any way to get rid of the (broken) cvs-old tree?
I've tried to rebase, but the commits from A' and A are different, it tries to reapply all history in CVS.
I've tried to checkout a point before our changes start (B') and then, using a script, cherry-pick all commits up to HEAD that are not in cvs-old branch. This approach didn't handled well merges between our branches (K-L-M and X-Y).
The history from the beginning up to the point where our changes started (...A--B) is quite large, so it would be nice for us to get rid of it.
Any suggestion on how to handle this?
Thank you,
- Samuel
^ permalink raw reply
* [PATCHv2 2/2] gitweb: clean up git_check_feature() calls
From: Giuseppe Bilotta @ 2008-11-28 20:39 UTC (permalink / raw)
To: git; +Cc: Jakub Narebski, Petr Baudis, Junio C Hamano, Giuseppe Bilotta
In-Reply-To: <1227904793-1821-2-git-send-email-giuseppe.bilotta@gmail.com>
Since git_check_feature now returns a scalar, the parentheses around the
variables initialized from git_check_feature() are not needed.
We remove them for stylistic consistency and to prevent unnecessary
ambiguity in the code.
Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
gitweb/gitweb.perl | 26 +++++++++++++-------------
1 files changed, 13 insertions(+), 13 deletions(-)
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 128d7ad..b0d00ea 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -828,7 +828,7 @@ sub href (%) {
}
}
- my ($use_pathinfo) = gitweb_check_feature('pathinfo');
+ my $use_pathinfo = gitweb_check_feature('pathinfo');
if ($use_pathinfo) {
# try to put as many parameters as possible in PATH_INFO:
# - project name
@@ -2119,7 +2119,7 @@ sub git_get_projects_list {
$filter ||= '';
$filter =~ s/\.git$//;
- my ($check_forks) = gitweb_check_feature('forks');
+ my $check_forks = gitweb_check_feature('forks');
if (-d $projects_list) {
# search in directory
@@ -2965,7 +2965,7 @@ EOF
}
print "</div>\n";
- my ($have_search) = gitweb_check_feature('search');
+ my $have_search = gitweb_check_feature('search');
if (defined $project && $have_search) {
if (!defined $searchtext) {
$searchtext = "";
@@ -2979,7 +2979,7 @@ EOF
$search_hash = "HEAD";
}
my $action = $my_uri;
- my ($use_pathinfo) = gitweb_check_feature('pathinfo');
+ my $use_pathinfo = gitweb_check_feature('pathinfo');
if ($use_pathinfo) {
$action .= "/".esc_url($project);
}
@@ -3472,7 +3472,7 @@ sub is_patch_split {
sub git_difftree_body {
my ($difftree, $hash, @parents) = @_;
my ($parent) = $parents[0];
- my ($have_blame) = gitweb_check_feature('blame');
+ my $have_blame = gitweb_check_feature('blame');
print "<div class=\"list_head\">\n";
if ($#{$difftree} > 10) {
print(($#{$difftree} + 1) . " files changed:\n");
@@ -3986,7 +3986,7 @@ sub git_project_list_body {
# actually uses global variable $project
my ($projlist, $order, $from, $to, $extra, $no_header) = @_;
- my ($check_forks) = gitweb_check_feature('forks');
+ my $check_forks = gitweb_check_feature('forks');
my @projects = fill_project_list_info($projlist, $check_forks);
$order ||= $default_projects_order;
@@ -4446,7 +4446,7 @@ sub git_summary {
my @taglist = git_get_tags_list(16);
my @headlist = git_get_heads_list(16);
my @forklist;
- my ($check_forks) = gitweb_check_feature('forks');
+ my $check_forks = gitweb_check_feature('forks');
if ($check_forks) {
@forklist = git_get_projects_list($project);
@@ -4475,7 +4475,7 @@ sub git_summary {
}
# Tag cloud
- my $show_ctags = (gitweb_check_feature('ctags'))[0];
+ my $show_ctags = gitweb_check_feature('ctags');
if ($show_ctags) {
my $ctags = git_get_project_ctags($project);
my $cloud = git_populate_project_tagcloud($ctags);
@@ -4765,7 +4765,7 @@ sub git_blob {
$expires = "+1d";
}
- my ($have_blame) = gitweb_check_feature('blame');
+ my $have_blame = gitweb_check_feature('blame');
open my $fd, "-|", git_cmd(), "cat-file", "blob", $hash
or die_error(500, "Couldn't cat $file_name, $hash");
my $mimetype = blob_mimetype($fd, $file_name);
@@ -4858,7 +4858,7 @@ sub git_tree {
my $ref = format_ref_marker($refs, $hash_base);
git_header_html();
my $basedir = '';
- my ($have_blame) = gitweb_check_feature('blame');
+ my $have_blame = gitweb_check_feature('blame');
if (defined $hash_base && (my %co = parse_commit($hash_base))) {
my @views_nav = ();
if (defined $file_name) {
@@ -5856,7 +5856,7 @@ insensitive).</p>
<dt><b>commit</b></dt>
<dd>The commit messages and authorship information will be scanned for the given pattern.</dd>
EOT
- my ($have_grep) = gitweb_check_feature('grep');
+ my $have_grep = gitweb_check_feature('grep');
if ($have_grep) {
print <<EOT;
<dt><b>grep</b></dt>
@@ -5873,7 +5873,7 @@ EOT
<dt><b>committer</b></dt>
<dd>Name and e-mail of the committer and date of commit will be scanned for the given pattern.</dd>
EOT
- my ($have_pickaxe) = gitweb_check_feature('pickaxe');
+ my $have_pickaxe = gitweb_check_feature('pickaxe');
if ($have_pickaxe) {
print <<EOT;
<dt><b>pickaxe</b></dt>
@@ -5925,7 +5925,7 @@ sub git_shortlog {
sub git_feed {
my $format = shift || 'atom';
- my ($have_blame) = gitweb_check_feature('blame');
+ my $have_blame = gitweb_check_feature('blame');
# Atom: http://www.atomenabled.org/developers/syndication/
# RSS: http://www.notestips.com/80256B3A007F2692/1/NAMO5P9UPQ
--
1.5.6.5
^ permalink raw reply related
* [PATCHv2 1/2] gitweb: fixes to gitweb feature check code
From: Giuseppe Bilotta @ 2008-11-28 20:39 UTC (permalink / raw)
To: git; +Cc: Jakub Narebski, Petr Baudis, Junio C Hamano, Giuseppe Bilotta
In-Reply-To: <1227904793-1821-1-git-send-email-giuseppe.bilotta@gmail.com>
The gitweb_check_feature routine was being used for two different
purposes: retrieving the actual feature value (such as the list of
snapshot formats or the list of additional actions), and to check if a
feature was enabled.
For the latter use, since all features return an array, it led to either
clumsy code or subtle bugs, with disabled features appearing enabled
because (0) evaluates to 1.
We fix these bugs, and simplify the code, by separating feature (list)
value retrieval (gitweb_get_feature) from boolean feature check (i.e.
retrieving the first/only item in the feature value list). Usage of
gitweb_check_feature is replaced by gitweb_get_feature where
appropriate.
Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
gitweb/gitweb.perl | 26 ++++++++++++++++++++++----
1 files changed, 22 insertions(+), 4 deletions(-)
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 933e137..128d7ad 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -190,7 +190,9 @@ our %feature = (
# if there is no 'sub' key (no feature-sub), then feature cannot be
# overriden
#
- # use gitweb_check_feature(<feature>) to check if <feature> is enabled
+ # use gitweb_get_feature(<feature>) to retrieve the <feature> value
+ # (an array) or gitweb_check_feature(<feature>) to check if <feature>
+ # is enabled
# Enable the 'blame' blob view, showing the last commit that modified
# each line in the file. This can be very CPU-intensive.
@@ -329,7 +331,8 @@ our %feature = (
'default' => [0]},
);
-sub gitweb_check_feature {
+# retrieve the value of a given feature, as an array
+sub gitweb_get_feature {
my ($name) = @_;
return unless exists $feature{$name};
my ($sub, $override, @defaults) = (
@@ -344,6 +347,21 @@ sub gitweb_check_feature {
return $sub->(@defaults);
}
+# check if a given feature is enabled or not, returning the first (and only)
+# value of the feature. Comfort code, allowing the use of
+# my $bool_feat = gitweb_check_feature('bool_feat');
+# or
+# gitweb_check_feature('bool_feat') or somecode;
+# instead of
+# my ($bool_feat) = gitweb_git_feature('bool_feat');
+# or
+# (gitweb_check_feature('bool_feat'))[0] or somecode;
+# respectively
+sub gitweb_check_feature {
+ return (gitweb_get_feature(@_))[0];
+}
+
+
sub feature_blame {
my ($val) = git_get_project_config('blame', '--bool');
@@ -767,7 +785,7 @@ our $git_dir;
$git_dir = "$projectroot/$project" if $project;
# list of supported snapshot formats
-our @snapshot_fmts = gitweb_check_feature('snapshot');
+our @snapshot_fmts = gitweb_get_feature('snapshot');
@snapshot_fmts = filter_snapshot_fmts(@snapshot_fmts);
# dispatch
@@ -3084,7 +3102,7 @@ sub git_print_page_nav {
$arg{'tree'}{'hash'} = $treehead if defined $treehead;
$arg{'tree'}{'hash_base'} = $treebase if defined $treebase;
- my @actions = gitweb_check_feature('actions');
+ my @actions = gitweb_get_feature('actions');
my %repl = (
'%' => '%',
'n' => $project, # project name
--
1.5.6.5
^ permalink raw reply related
* [PATCHv2 0/2] fixes to gitweb feature check code
From: Giuseppe Bilotta @ 2008-11-28 20:39 UTC (permalink / raw)
To: git; +Cc: Jakub Narebski, Petr Baudis, Junio C Hamano, Giuseppe Bilotta
This is v2 of the gitweb feature check fix patch, which has now been
split into into a code patch and cleanup patch.
The first patch introduces git_get_feature() to clearly distinguish the
feature retrieval from the boolean feature check (which is kept at
git_check_feature()). The new function is used where appropriate.
The second patch cleans up use of git_check_feature(): since the
function now returns a boolean instead of an array, the often-used
construct
my ($somevar) = git_check_feature('somefeat');
although still valid, becomes a rather clumsy stylistic choice, as it
introduces an unnecessary ambiguity. Make it clear that we're now
dealing with scalars by using scalar assignment.
Giuseppe Bilotta (2):
gitweb: fixes to gitweb feature check code
gitweb: clean up git_check_feature() calls
gitweb/gitweb.perl | 52 +++++++++++++++++++++++++++++++++++-----------------
1 files changed, 35 insertions(+), 17 deletions(-)
^ permalink raw reply
* Re: What's cooking in git.git (Nov 2008, #06; Wed, 26)
From: Shawn O. Pearce @ 2008-11-28 19:20 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Junio C Hamano, git
In-Reply-To: <alpine.DEB.1.00.0811281225040.30769@pacific.mpi-cbg.de>
Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> On Thu, 27 Nov 2008, Junio C Hamano wrote:
> > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> >
> > > I have a strong suspicion that the narrow stuff will make the worktree
> > > mess pale in comparison.
> > >
> > > Note that I do not have time to review this myself (which is not
> > > helped at all by it being no longer a trivial single patch, but a full
> > > 10 patches!), but I really have a bad feeling about this. IMO it is
> > > substantially under-reviewed.
> >
> > Well, "a bad feeling" is not a convincing enough argument either, is it?
> > What kind of bad interaction are you fearing?
...
> And the worst part: I think that as with worktree, there has not been
> enough of kicking forth and back ideas how to design the beast, so I fully
> expect a subtle breakage that would require a redesign (which will be
> painful, with existing users of the feature).
>
> Maybe I am crying "wolf", but I _do_ want to caution against risking too
> much, too fast, with that feature.
>
> In other words, unless there is more interest in that feature, enough to
> generate a well-understood design before a good implementation, I'd rather
> see this patch series dropped.
Ack. I agree with every remark made by Dscho, and also want to cry "wolf".
I haven't had time to read the patch series. Its big and intrusive
and I just don't need the feature.
But I feel like if it were in fact merged I'll fall over some bug
in it sometime soon and be forced to stop and debug it. Heck at
the least I'll have to go back to JGit's index code and implement
the new file format. That shouldn't cause git.git's development to
stop, but I am whining (a little) about the file format change. ;-)
--
Shawn.
^ permalink raw reply
* [PATCH] git-cvsimport: add support for cvs pserver password scrambling.
From: Dirk Hörner @ 2008-11-28 18:06 UTC (permalink / raw)
To: git
Instead of a cleartext password, the CVS pserver expects a scrambled one
in the authentication request. With this patch it is possible to import
CVS repositories only accessible via pserver and user/password.
Signed-off-by: Dirk Hoerner <dirker@gmail.com>
---
git-cvsimport.perl | 39 ++++++++++++++++++++++++++++++++++++++-
1 files changed, 38 insertions(+), 1 deletions(-)
diff --git a/git-cvsimport.perl b/git-cvsimport.perl
index e439202..593832d 100755
--- a/git-cvsimport.perl
+++ b/git-cvsimport.perl
@@ -252,7 +252,8 @@ sub conn {
}
};
}
- $pass="A" unless $pass;
+
+ $pass = $self->_scramble($pass);
my ($s, $rep);
if ($proxyhost) {
@@ -484,6 +485,42 @@ sub _fetchfile {
return $res;
}
+sub _scramble {
+ my ($self, $pass) = @_;
+ my $scrambled = "A";
+
+ return $scrambled unless $pass;
+
+ my $pass_len = length($pass);
+ my @pass_arr = split("", $pass);
+ my $i;
+
+ # from cvs/src/scramble.c
+ my @shifts = (
+ 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15,
+ 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31,
+ 114,120, 53, 79, 96,109, 72,108, 70, 64, 76, 67,116, 74, 68, 87,
+ 111, 52, 75,119, 49, 34, 82, 81, 95, 65,112, 86,118,110,122,105,
+ 41, 57, 83, 43, 46,102, 40, 89, 38,103, 45, 50, 42,123, 91, 35,
+ 125, 55, 54, 66,124,126, 59, 47, 92, 71,115, 78, 88,107,106, 56,
+ 36,121,117,104,101,100, 69, 73, 99, 63, 94, 93, 39, 37, 61, 48,
+ 58,113, 32, 90, 44, 98, 60, 51, 33, 97, 62, 77, 84, 80, 85,223,
+ 225,216,187,166,229,189,222,188,141,249,148,200,184,136,248,190,
+ 199,170,181,204,138,232,218,183,255,234,220,247,213,203,226,193,
+ 174,172,228,252,217,201,131,230,197,211,145,238,161,179,160,212,
+ 207,221,254,173,202,146,224,151,140,196,205,130,135,133,143,246,
+ 192,159,244,239,185,168,215,144,139,165,180,157,147,186,214,176,
+ 227,231,219,169,175,156,206,198,129,164,150,210,154,177,134,127,
+ 182,128,158,208,162,132,167,209,149,241,153,251,237,236,171,195,
+ 243,233,253,240,194,250,191,155,142,137,245,235,163,242,178,152
+ );
+
+ for ($i = 0; $i < $pass_len; $i++) {
+ $scrambled .= pack("C", $shifts[ord($pass_arr[$i])]);
+ }
+
+ return $scrambled;
+}
package main;
--
1.6.0.4.837.gae258
^ permalink raw reply related
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