* Re: [PATCH 00/20] [GIT PULL][v3.2] tracing: queued updates
From: Ingo Molnar @ 2011-10-12 16:29 UTC (permalink / raw)
To: Jeff King
Cc: Valdis.Kletnieks, git, Steven Rostedt, linux-kernel,
Andrew Morton, Thomas Gleixner, Frederic Weisbecker
In-Reply-To: <20111012141939.GA25085@sigill.intra.peff.net>
* Jeff King <peff@peff.net> wrote:
> > It's really useful for a painless UI flow to disambiguate failure
> > messages into clearly actionable variants.
>
> I agree. I think some people are concerned with leaking information
> about which repos exist and how they are configured. That is
> probably not a big problem for a public site like kernel.org,
> though.
Well, a gitconfig option could be provided to not leak that small
amount of info - but it would otherwise be weird for an OSS SCM to
default to a behavior that only makes sense with closed source,
right? :-)
Thanks,
Ingo
^ permalink raw reply
* Re: [PATCH] Documentation: update [section.subsection] to reflect what git does
From: Jeff King @ 2011-10-12 16:29 UTC (permalink / raw)
To: Carlos Martín Nieto; +Cc: git, Junio C Hamano
In-Reply-To: <1318434726-5556-1-git-send-email-cmn@elego.de>
On Wed, Oct 12, 2011 at 05:52:06PM +0200, Carlos Martín Nieto wrote:
> -There is also a case insensitive alternative `[section.subsection]` syntax.
> -In this syntax, subsection names follow the same restrictions as for section
> -names.
> +There is also a deprecated `[section.subsection]` syntax. With this
> +syntax, the subsection name is converted to lower-case and is also
> +compared case sensitively. These subsection names follow the same
> +restrictions as section names.
Hmm. While technically more correct, I think it is a little more
confusing to read. The lower-case canonicalization thing is actually
used for the other case-insensitive parts, too. So maybe it makes sense
to describe that in detail, and then just note that
"[section.subsection]" uses the same mechanism.
The patch below does this, and then the original text in the section you
tweaked above hopefully makes more sense in the new context.
The explanation matches what we do now, but it did end up a bit longer
than I had hoped. We could make it a lot shorter by:
1. Canonicalizing the section and key names that the caller gives to
git-config.
2. Not mentioning the "section.foo" syntax. We can't canonicalize the
subsection in (1) because of this syntax. But we can at least gloss
over the detail, and then maybe just mention it much later in the
file format. Or even deprecate it.
-- >8 --
Subject: [PATCH] docs/config: explain case-insensitive matching
We generally think of key matching as case-insensitive, but
it's not exactly. It's about canonicalizing one side, and
comparing it byte-wise with the canonical key name given to
git-config.
Signed-off-by: Jeff King <peff@peff.net>
---
Documentation/git-config.txt | 50 +++++++++++++++++++++++++++++++++++++++++-
1 files changed, 49 insertions(+), 1 deletions(-)
diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index e7ecf5d..e92aee9 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -28,7 +28,7 @@ DESCRIPTION
-----------
You can query/set/replace/unset options with this command. The name is
actually the section and the key separated by a dot, and the value will be
-escaped.
+escaped. See the section on name matching below.
Multiple lines can be added to an option by using the '--add' option.
If you want to update or unset an option which can occur on multiple
@@ -178,6 +178,54 @@ See also <<FILES>>.
Opens an editor to modify the specified config file; either
'--system', '--global', or repository (default).
+
+NAME MATCHING
+-------------
+
+Configuration key names are matched using an algorithm that allows for
+partial case sensitivity. Section and key names are read from the config
+files, canonicalized according to the rules below, and then compared
+case-sensitively with the input given to git-config. Therefore any
+callers to git-config should request the canonicalized version of the
+name. This typically means lowercasing the section and key names, and
+leaving the subsection (if any) intact. For example, ask for
+`git config core.eol`, not `git config CoRe.EOL`.
+
+The canonicalization rules are:
+
+1. Lowercase the section and key names.
+
+2. If a literal subsection (like `[section "foo"]`) is used, leave it
+ intact.
+
+3. If a non-literal subsection (like `[section.foo]`) is used, lowercase
+ the subsection.
+
+4. Concatenate the resulting section, subsection, and key, separated by
+ a dot ('.').
+
+For example, this configuration file:
+
+-----------------------------------------------
+[CORE]
+eol = true
+
+[branch "Foo"]
+REMOTE = origin
+
+[color.DIFF]
+new = blue
+-----------------------------------------------
+
+would yield the following three canonicalized names:
+
+-----------------------------------------------
+core.eol
+branch.Foo.remote
+color.diff.new
+-----------------------------------------------
+
+
[[FILES]]
FILES
-----
--
1.7.7.rc2.21.gb9948
^ permalink raw reply related
* [PATCH] Documentation: update [section.subsection] to reflect what git does
From: Carlos Martín Nieto @ 2011-10-12 15:52 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano
Using the [section.subsection] syntax, the subsection is transformed
to lower-case and is matched case sensitively. Say so in the
documentation and mention that you shouldn't be using it anyway.
Signed-off-by: Carlos Martín Nieto <cmn@elego.de>
---
This bit me recently when I was creating a parser. See Jeff's
explanation here:
http://thread.gmane.org/gmane.comp.version-control.git/179569/focus=180290
Documentation/config.txt | 7 ++++---
1 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 0658ffb..1212c47 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -45,9 +45,10 @@ lines. Variables may belong directly to a section or to a given subsection.
You can have `[section]` if you have `[section "subsection"]`, but you
don't need to.
-There is also a case insensitive alternative `[section.subsection]` syntax.
-In this syntax, subsection names follow the same restrictions as for section
-names.
+There is also a deprecated `[section.subsection]` syntax. With this
+syntax, the subsection name is converted to lower-case and is also
+compared case sensitively. These subsection names follow the same
+restrictions as section names.
All the other lines (and the remainder of the line after the section
header) are recognized as setting variables, in the form
--
1.7.6.557.gcee4
^ permalink raw reply related
* Re: [PATCH v3] gitk: Teach gitk to respect log.showroot
From: Marcus Karlsson @ 2011-10-12 14:36 UTC (permalink / raw)
To: Paul Mackerras; +Cc: zbyszek, gitster, git
In-Reply-To: <20111008064704.GA27056@bloggs.ozlabs.ibm.com>
On Sat, Oct 08, 2011 at 05:47:04PM +1100, Paul Mackerras wrote:
> On Tue, Oct 04, 2011 at 10:08:13PM +0200, Marcus Karlsson wrote:
> > Teach gitk to respect log.showroot.
>
> Sounds reasonable, ...
>
> > - set cmd [concat | git diff-tree -r $flags $ids]
> > + set cmd [concat | git diff-tree -r]
> > + if {$log_showroot eq true} {
> > + set cmd [concat $cmd --root]
> > + }
> > + set cmd [concat $cmd $flags $ids]
>
> but is there any reason not to do it like this?
>
> if {$log_showroot} {
> lappend flags --root
> }
> set cmd [concat | git diff-tree -r $flags $ids]
>
> I.e., do you particularly want the --root before the other flags?
>
> Paul.
Not really, that would work very well.
Marcus
^ permalink raw reply
* Re: [PATCH] fix "git apply --index ..." not to deref NULL
From: Jim Meyering @ 2011-10-12 14:33 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20111012142750.GB25085@sigill.intra.peff.net>
Jeff King wrote:
> On Wed, Oct 12, 2011 at 10:18:01AM +0200, Jim Meyering wrote:
>
>> I noticed this when "git am CORRUPTED" unexpectedly failed with an
>> odd diagnostic, and even removed one of the files it was supposed
>> to have patched.
>>
>> Reproduce with any valid old/new patch from which you have removed
>> the "+++ b/FILE" line. You'll see a diagnostic like this
>>
>> fatal: unable to write file '(null)' mode 100644: Bad address
>>
>> and you'll find that FILE has been removed.
>
> Yikes. Your fix looks right to me.
>
>> builtin/apply.c | 3 +++
>> t/t4254-am-corrupt.sh | 43 +++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 46 insertions(+), 0 deletions(-)
>> create mode 100644 t/t4254-am-corrupt.sh
>
> Missing executable bit on the new test.
Thanks.
Fixed with this:
-- >8 --
Subject: [PATCH] fix "git apply --index ..." not to deref NULL
I noticed this when "git am CORRUPTED" unexpectedly failed with an
odd diagnostic, and even removed one of the files it was supposed
to have patched.
Reproduce with any valid old/new patch from which you have removed
the "+++ b/FILE" line. You'll see a diagnostic like this
fatal: unable to write file '(null)' mode 100644: Bad address
and you'll find that FILE has been removed.
The above is on glibc-based systems. On other systems, rather than
getting "null", you may provoke a segfault as git tries to
dereference the NULL file name.
Signed-off-by: Jim Meyering <meyering@redhat.com>
---
builtin/apply.c | 3 +++
t/t4254-am-corrupt.sh | 43 +++++++++++++++++++++++++++++++++++++++++++
2 files changed, 46 insertions(+), 0 deletions(-)
create mode 100755 t/t4254-am-corrupt.sh
diff --git a/builtin/apply.c b/builtin/apply.c
index f2edc52..aaa39fe 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -1407,6 +1407,9 @@ static int find_header(char *line, unsigned long size, int *hdrsize, struct patc
"%d leading pathname components (line %d)" , p_value, linenr);
patch->old_name = patch->new_name = patch->def_name;
}
+ if (!patch->is_delete && !patch->new_name)
+ die("git diff header lacks filename information "
+ "(line %d)", linenr);
patch->is_toplevel_relative = 1;
*hdrsize = git_hdr_len;
return offset;
diff --git a/t/t4254-am-corrupt.sh b/t/t4254-am-corrupt.sh
new file mode 100755
index 0000000..b7da95f
--- /dev/null
+++ b/t/t4254-am-corrupt.sh
@@ -0,0 +1,43 @@
+#!/bin/sh
+
+test_description='git am with corrupt input'
+. ./test-lib.sh
+
+# Note the missing "+++" line:
+cat > bad-patch.diff <<'EOF'
+From: A U Thor <au.thor@example.com>
+diff --git a/f b/f
+index 7898192..6178079 100644
+--- a/f
+@@ -1 +1 @@
+-a
++b
+EOF
+
+test_expect_success setup '
+ test $? = 0 &&
+ echo a > f &&
+ git add f &&
+ test_tick &&
+ git commit -m initial
+'
+
+# This used to fail before, too, but with a different diagnostic.
+# fatal: unable to write file '(null)' mode 100644: Bad address
+# Also, it had the unwanted side-effect of deleting f.
+test_expect_success 'try to apply corrupted patch' '
+ git am bad-patch.diff 2> actual
+ test $? = 1
+'
+
+cat > expected <<EOF
+fatal: git diff header lacks filename information (line 4)
+EOF
+
+test_expect_success 'compare diagnostic; ensure file is still here' '
+ test $? = 0 &&
+ test -f f &&
+ test_cmp expected actual
+'
+
+test_done
--
1.7.7
^ permalink raw reply related
* Re: [PATCH] fix "git apply --index ..." not to deref NULL
From: Jeff King @ 2011-10-12 14:27 UTC (permalink / raw)
To: Jim Meyering; +Cc: git
In-Reply-To: <87lisq8vye.fsf@rho.meyering.net>
On Wed, Oct 12, 2011 at 10:18:01AM +0200, Jim Meyering wrote:
> I noticed this when "git am CORRUPTED" unexpectedly failed with an
> odd diagnostic, and even removed one of the files it was supposed
> to have patched.
>
> Reproduce with any valid old/new patch from which you have removed
> the "+++ b/FILE" line. You'll see a diagnostic like this
>
> fatal: unable to write file '(null)' mode 100644: Bad address
>
> and you'll find that FILE has been removed.
Yikes. Your fix looks right to me.
> builtin/apply.c | 3 +++
> t/t4254-am-corrupt.sh | 43 +++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 46 insertions(+), 0 deletions(-)
> create mode 100644 t/t4254-am-corrupt.sh
Missing executable bit on the new test.
-Peff
^ permalink raw reply
* Re: [PATCH 00/20] [GIT PULL][v3.2] tracing: queued updates
From: Jeff King @ 2011-10-12 14:19 UTC (permalink / raw)
To: Ingo Molnar
Cc: Valdis.Kletnieks, git, Steven Rostedt, linux-kernel,
Andrew Morton, Thomas Gleixner, Frederic Weisbecker
In-Reply-To: <20111012080711.GM18618@elte.hu>
On Wed, Oct 12, 2011 at 10:07:14AM +0200, Ingo Molnar wrote:
> > On Tue, 11 Oct 2011 07:50:17 +0200, Ingo Molnar said:
> >
> > > $ git pull git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-2.6-trace.git tip/perf/core
> > > fatal: The remote end hung up unexpectedly
> >
> > Is it possible to get 'git' to say something more informative than
> > "hung up unexpectedly"? "Tree not found, check URL" or similar
> > would be nice...
It's not possible for the client to say anything more. The server sees
that the request isn't valid and hangs up without saying anything. So
the server needs to be changed to output better responses.
> Firstly, arguably, typoing something is not 'fatal' really - it's
> just a resource that was not found on the server.
>
> Secondly, and more importantly, the reason for the failed pull is
> indeed important to know, if you want to resolve the problem with a
> minimum fuss:
>
> - Was it the tree that didnt exist?
> - Or the branch?
> - Or was there some other problem [such as a truly unexpectedly
> closed transport socket]?
>
> It's really useful for a painless UI flow to disambiguate failure
> messages into clearly actionable variants.
I agree. I think some people are concerned with leaking information
about which repos exist and how they are configured. That is probably
not a big problem for a public site like kernel.org, though.
You might find this thread interesting:
http://thread.gmane.org/gmane.comp.version-control.git/182529/focus=182642
It seems to have resulted in a patch that will at least say "access
denied" for every error. Which is a step up from "the remote end hung up
unexpectedly", but I do think most users would appreciate it being more
specific.
Perhaps we just need a config option to turn on more verbose messages,
if the site decides that there's no security implications to doing so.
-Peff
^ permalink raw reply
* Re: [PATCH 1/3] t5403.1: simplify commit creation
From: Johannes Sixt @ 2011-10-12 14:14 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, Junio C Hamano
In-Reply-To: <1318412105-13595-1-git-send-email-pclouds@gmail.com>
Am 10/12/2011 11:35, schrieb Nguyễn Thái Ngọc Duy:
> test_expect_success setup '
> echo Data for commit0. >a &&
> echo Data for commit0. >b &&
> - git update-index --add a &&
> - git update-index --add b &&
> - tree0=$(git write-tree) &&
> - commit0=$(echo setup | git commit-tree $tree0) &&
> - git update-ref refs/heads/master $commit0 &&
> + git add a b &&
> + git commit -m setup &&
> git clone ./. clone1 &&
> git clone ./. clone2 &&
> GIT_DIR=clone2/.git git branch new2 &&
I don't think this change is necessary. It doesn't hurt to use plumbing
commands here and there in the test suite to exercise them to a degree
that they deserve.
-- Hannes
^ permalink raw reply
* Re: [RFC] teach --edit to git rebase
From: Jean Privat @ 2011-10-12 13:57 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vvcrubyz1.fsf@alter.siamese.dyndns.org>
> The only thing you can do with this new option is "update one commit
> buried in the history, and rebase everything that build on top of it",
It is a good summary indeed.
> as
> far as I can tell. It feels a shame to waste the generic word "--edit" for
> such a narrow option.
I'm bad at bikesheeding. The 'edit' come from the 'edit' command in
rebase interactive. I am open to other names. Note that in the
following I keep the --edit name, not because I am suborn but because
I do not have better to propose yet.
> At the UI level, "git commit --amend HEAD~4" might be a more natural way
> to invoke such an operation, I would think.
As I say in the original email the point of the 'rebase --edit
some-commit' is to temporally checkout some-commit so that edits are
done in the context of the commit and not in the context of the head
of the branch.
One has to do a rebase --edit prior to modification (although we can
imagine a possibility to bring back the index or the content of the
working directory with us either automatically as with a branch
checkout or manually with the help of stash).
Unless I misinterpret the 'git commit --amend HEAD~4' you suggest, it
means that you have to prepare the commit in the head of the branch.
It may be difficult if what was in HEAD~4 is altered by HEAD~2.
My argument is that if preparing a patch to HEAD~4 in HEAD is easy, a
git commit --fixup will do the tick.
If the preparation is difficult because I have to work on (or more
insidious, near) change that occurs between HEAD~4 and HEAD, I need
something like my proposal. For example I added a line in HEAD~2 but I
prefer now to have this line to appears in HEAD~4.
The workflow I propose is :
$ # we are on master
$ git rebase --edit HEAD~4 # workdir is a detached master~4 like with a
# git checkout master~4
$ hack hack hack; git add files
$ git commit --amend
$ git rebase --continue # conflict is detected with master~2, resolve it
# manually
$ git rebase --continue # workdir is now a rebased master
I do no see what is the workflow with an extend git commit --amend
Do you mean something like the following ?
$ git checkout HEAD~4 -- . # bring back the content of master~4 but
# HEAD still points master
$ hack hack hack; git add files # And try to now be disturbed by the fact
# that diff and status are polluted
# with things related to master
$ git commit --amend HEAD~4 # conflict is detected with master~2, resolve it
$ git rebase --continue # Do we really want using "git rebase"?
This last workflow seems so awkward to me that I might miss something.
-- Jean Privat
^ permalink raw reply
* Re: [PATCH] Make is_gitfile a non-static generic function
From: Phil Hord @ 2011-10-12 13:29 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Phil Hord, git@vger.kernel.org
In-Reply-To: <7vipnvccso.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> wrote:
> Phil Hord <hordp@cisco.com> writes:
>
>> The new is_gitfile is an amalgam of similar functional checks
>> from different places in the code....
>
>
> After looking at this patch and the way the other caller in transport.c
> uses it, I am more and more convinced that "is_gitfile()" is a stupid and
> horrible mistake.
I think it's a simple and low-impact change that fixes a bug with a
minimum of disruption. But I also think it is lazy.
> The caller in transport.c says "I am about to read from a regular file,
> and usually I would treat it as a bundle, but I want to avoid that
> codepath if that regular file is not a bundle. So I use the codepath only
> when that file is not a gitfile".
>
> It should be saying "Is it a bundle? Then I'd use the codepath to read
> from the bundle" to begin with. Otherwise the code will break when we add
> yet another regular file we can fetch from that is not a bundle nor a
> gitfile.
Yes, and this is part of the kind of distraction that held back my
update over the weekend.
When we do add another file type we'll wind up with a half-dozen
places that get affected in slightly different ways again. Wouldn't
it be nice to have a function to tell us what kind of thing it is
we've been asked to look at? Something like git_type(url) that
returns GIT_BUNDLE, GIT_DIRECTORY or GIT_FILE, maybe.
Except I didn't see many examples in the code using this sort of
enumerated decision function.
> I think the hand-crafted check in builtin/clone.c you removed originated
> from laziness to avoid teaching read_gitfile() to read potential gitfile
> silently (and signal errors by simply returning NULL).
I made a read_gitfile(... , gently) function, but I didn't like it
much. When !gently, I think it should be rather explicit about the
type of failure. This makes the code look like 20% of it is repeated
"if (!gently) die... ;\n return;" sequences. It's almost enough to
lead me to macros.
And what about when fopen() fails and we are running silently. Do we
just shrug and say "Not a gitfile"? I don't think it's good enough.
We need to be able to say all of these:
It's a gitfile, here's the internal path.
It's not a gitfile, it is something else.
It looked like a gitfile until I ran into E_ACCES or some other error.
Making the one function run silently or not complicates the code further.
I tried to find a similar style to mimic elsewhere in the code, but I
didn't find any consistency. Pointers to clean examples would be
welcome.
I started working on more of an API. But it's still very ugly and not
ready for even a strawman discussion.
But I don't know how much time I have for a full writeup atm. Without
something, though, I cannot easily fetch from a submodule, because
submodules all use gitfiles now, and git:master does not know how to
fetch from them.
And that's the itch I had to scratch.
> I also suspect the
> codepath may become simpler if we had a way to ask "Is this a bundle?".
>
> I think read_bundle_header() in bundle.c can be refactored to a silent
> interface that allows us to ask "Is this a bundle?" question properly.
I'll take a look at it. But I won't have much time for it this week.
Thanks,
Phil
^ permalink raw reply
* [PATCH 3/3] t5403: do not use access repos with GIT_DIR when worktree is involved
From: Nguyễn Thái Ngọc Duy @ 2011-10-12 9:35 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy
In-Reply-To: <1318412105-13595-1-git-send-email-pclouds@gmail.com>
Setting GIT_DIR alone means worktree is current directory for legacy
reasons. Avoid using that, instead go to the worktree and execute
commands there.
The troublesome command is "GIT_DIR=clone2/.git git add clone2/b". The
real worktree is clone2, but that command tells git worktree is $(pwd).
What does user expect to add then? Should the new entry in index be "b"
or "clone2/b"?
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
t/t5403-post-checkout-hook.sh | 17 ++++++++++-------
1 files changed, 10 insertions(+), 7 deletions(-)
diff --git a/t/t5403-post-checkout-hook.sh b/t/t5403-post-checkout-hook.sh
index 6643f32..3459539 100755
--- a/t/t5403-post-checkout-hook.sh
+++ b/t/t5403-post-checkout-hook.sh
@@ -13,10 +13,13 @@ test_expect_success setup '
git commit -m setup &&
git clone ./. clone1 &&
git clone ./. clone2 &&
- GIT_DIR=clone2/.git git branch new2 &&
- echo Data for commit1. >clone2/b &&
- GIT_DIR=clone2/.git git add clone2/b &&
- GIT_DIR=clone2/.git git commit -m new2
+ (
+ cd clone2 &&
+ git branch new2 &&
+ echo Data for commit1. >b &&
+ git add b &&
+ git commit -m new2
+ )
'
for clone in 1 2; do
@@ -45,7 +48,7 @@ test_expect_success 'post-checkout runs as expected ' '
'
test_expect_success 'post-checkout args are correct with git checkout -b ' '
- GIT_DIR=clone1/.git git checkout -b new1 &&
+ ( cd clone1 && git checkout -b new1 ) &&
old=$(awk "{print \$1}" clone1/.git/post-checkout.args) &&
new=$(awk "{print \$2}" clone1/.git/post-checkout.args) &&
flag=$(awk "{print \$3}" clone1/.git/post-checkout.args) &&
@@ -53,7 +56,7 @@ test_expect_success 'post-checkout args are correct with git checkout -b ' '
'
test_expect_success 'post-checkout receives the right args with HEAD changed ' '
- GIT_DIR=clone2/.git git checkout new2 &&
+ ( cd clone2 && git checkout new2 ) &&
old=$(awk "{print \$1}" clone2/.git/post-checkout.args) &&
new=$(awk "{print \$2}" clone2/.git/post-checkout.args) &&
flag=$(awk "{print \$3}" clone2/.git/post-checkout.args) &&
@@ -61,7 +64,7 @@ test_expect_success 'post-checkout receives the right args with HEAD changed ' '
'
test_expect_success 'post-checkout receives the right args when not switching branches ' '
- GIT_DIR=clone2/.git git checkout master b &&
+ ( cd clone2 && git checkout master b ) &&
old=$(awk "{print \$1}" clone2/.git/post-checkout.args) &&
new=$(awk "{print \$2}" clone2/.git/post-checkout.args) &&
flag=$(awk "{print \$3}" clone2/.git/post-checkout.args) &&
--
1.7.3.1.256.g2539c.dirty
^ permalink raw reply related
* [PATCH 2/3] t5403: convert leading spaces to tabs
From: Nguyễn Thái Ngọc Duy @ 2011-10-12 9:35 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy
In-Reply-To: <1318412105-13595-1-git-send-email-pclouds@gmail.com>
The first and last tests use tabs. The rest uses spaces. Convert all
to tabs.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
t/t5403-post-checkout-hook.sh | 46 ++++++++++++++++++++--------------------
1 files changed, 23 insertions(+), 23 deletions(-)
diff --git a/t/t5403-post-checkout-hook.sh b/t/t5403-post-checkout-hook.sh
index 0c126d7..6643f32 100755
--- a/t/t5403-post-checkout-hook.sh
+++ b/t/t5403-post-checkout-hook.sh
@@ -28,44 +28,44 @@ EOF
done
test_expect_success 'post-checkout runs as expected ' '
- GIT_DIR=clone1/.git git checkout master &&
- test -e clone1/.git/post-checkout.args
+ GIT_DIR=clone1/.git git checkout master &&
+ test -e clone1/.git/post-checkout.args
'
test_expect_success 'post-checkout receives the right arguments with HEAD unchanged ' '
- old=$(awk "{print \$1}" clone1/.git/post-checkout.args) &&
- new=$(awk "{print \$2}" clone1/.git/post-checkout.args) &&
- flag=$(awk "{print \$3}" clone1/.git/post-checkout.args) &&
- test $old = $new -a $flag = 1
+ old=$(awk "{print \$1}" clone1/.git/post-checkout.args) &&
+ new=$(awk "{print \$2}" clone1/.git/post-checkout.args) &&
+ flag=$(awk "{print \$3}" clone1/.git/post-checkout.args) &&
+ test $old = $new -a $flag = 1
'
test_expect_success 'post-checkout runs as expected ' '
- GIT_DIR=clone1/.git git checkout master &&
- test -e clone1/.git/post-checkout.args
+ GIT_DIR=clone1/.git git checkout master &&
+ test -e clone1/.git/post-checkout.args
'
test_expect_success 'post-checkout args are correct with git checkout -b ' '
- GIT_DIR=clone1/.git git checkout -b new1 &&
- old=$(awk "{print \$1}" clone1/.git/post-checkout.args) &&
- new=$(awk "{print \$2}" clone1/.git/post-checkout.args) &&
- flag=$(awk "{print \$3}" clone1/.git/post-checkout.args) &&
- test $old = $new -a $flag = 1
+ GIT_DIR=clone1/.git git checkout -b new1 &&
+ old=$(awk "{print \$1}" clone1/.git/post-checkout.args) &&
+ new=$(awk "{print \$2}" clone1/.git/post-checkout.args) &&
+ flag=$(awk "{print \$3}" clone1/.git/post-checkout.args) &&
+ test $old = $new -a $flag = 1
'
test_expect_success 'post-checkout receives the right args with HEAD changed ' '
- GIT_DIR=clone2/.git git checkout new2 &&
- old=$(awk "{print \$1}" clone2/.git/post-checkout.args) &&
- new=$(awk "{print \$2}" clone2/.git/post-checkout.args) &&
- flag=$(awk "{print \$3}" clone2/.git/post-checkout.args) &&
- test $old != $new -a $flag = 1
+ GIT_DIR=clone2/.git git checkout new2 &&
+ old=$(awk "{print \$1}" clone2/.git/post-checkout.args) &&
+ new=$(awk "{print \$2}" clone2/.git/post-checkout.args) &&
+ flag=$(awk "{print \$3}" clone2/.git/post-checkout.args) &&
+ test $old != $new -a $flag = 1
'
test_expect_success 'post-checkout receives the right args when not switching branches ' '
- GIT_DIR=clone2/.git git checkout master b &&
- old=$(awk "{print \$1}" clone2/.git/post-checkout.args) &&
- new=$(awk "{print \$2}" clone2/.git/post-checkout.args) &&
- flag=$(awk "{print \$3}" clone2/.git/post-checkout.args) &&
- test $old = $new -a $flag = 0
+ GIT_DIR=clone2/.git git checkout master b &&
+ old=$(awk "{print \$1}" clone2/.git/post-checkout.args) &&
+ new=$(awk "{print \$2}" clone2/.git/post-checkout.args) &&
+ flag=$(awk "{print \$3}" clone2/.git/post-checkout.args) &&
+ test $old = $new -a $flag = 0
'
if test "$(git config --bool core.filemode)" = true; then
--
1.7.3.1.256.g2539c.dirty
^ permalink raw reply related
* [PATCH 1/3] t5403.1: simplify commit creation
From: Nguyễn Thái Ngọc Duy @ 2011-10-12 9:35 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
t/t5403-post-checkout-hook.sh | 7 ++-----
1 files changed, 2 insertions(+), 5 deletions(-)
diff --git a/t/t5403-post-checkout-hook.sh b/t/t5403-post-checkout-hook.sh
index d05a913..0c126d7 100755
--- a/t/t5403-post-checkout-hook.sh
+++ b/t/t5403-post-checkout-hook.sh
@@ -9,11 +9,8 @@ test_description='Test the post-checkout hook.'
test_expect_success setup '
echo Data for commit0. >a &&
echo Data for commit0. >b &&
- git update-index --add a &&
- git update-index --add b &&
- tree0=$(git write-tree) &&
- commit0=$(echo setup | git commit-tree $tree0) &&
- git update-ref refs/heads/master $commit0 &&
+ git add a b &&
+ git commit -m setup &&
git clone ./. clone1 &&
git clone ./. clone2 &&
GIT_DIR=clone2/.git git branch new2 &&
--
1.7.3.1.256.g2539c.dirty
^ permalink raw reply related
* Re: [RESEND PATCH v3] Configurable hyperlinking in gitk
From: Chris Packham @ 2011-10-12 9:07 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jeff Epler, Paul Mackerras, Jakub Narebski, git, Marc Branchaud
In-Reply-To: <7vfwizdvnn.fsf@alter.siamese.dyndns.org>
On 12/10/11 11:13, Junio C Hamano wrote:
> Jeff Epler <jepler@unpythonic.net> writes:
>
>> I'm aware of no problems with this patch, and a number of people have
>> commented that it is useful to them.
>
> Hmmm, "didn't generate any discussion" does not mesh very well with "a
> number of people are happy". Which one should I trust?
>
For what it's worth I've (just) tested v3 and it works well for me.
^ permalink raw reply
* [PATCH] Makefile: add a knob to turn off hardlinks within same directory
From: Jonathan Nieder @ 2011-10-12 8:38 UTC (permalink / raw)
To: git; +Cc: Bastian Blank, Cedric Staniewski
The git builtins in $(gitexecdir) are implemented as hard links to a
single git binary by default, so even the overhead of symlink
resolution is not needed to run them. However, the trick can be
harmful, in two cases:
- on Windows, some tools to estimate directory size hugely
overestimate the size of git (each hardlink counts as taking up the
same amount of space as git.exe)
- various filesystems have limits on the number of hardlinks that
can be made to a particular file --- Linux's LINK_MAX is 127,
_POSIX_LINK_MAX is 8, and btrfs has a limit of 4096 /
($len_filename + 8) or so links to a given inode in a single
directory.
Normally that second case is not a problem (when ln fails, "make
install" falls back to "ln -s"), but if git is tar'ed up on one
filesystem and then extracted on a more limited one, it can result in
"Too many links" errors.
Nowadays people are encouraged to (and typically do) run builtins
using the "git" command name directly rather than those dashed forms
in scripts, making the use of hardlinks for the dashed forms a
somewhat pointless optimization. Introduce a new knob to allow people
to turn it off with a simple "make install NO_HARDLINKS=YesPlease".
Typically someone using this setting would want to set
NO_CROSS_DIRECTORY_HARDLINKS, too, but that is not enforced, so you
can make $(bindir)/git and $(gitexecdir)/git into hardlinks to the
same inode and still make sure your tarball avoids the btrfs limits if
you want.
Requested-by: Bastian Blank <waldi@debian.org>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Hi,
See <http://bugs.debian.org/642603> for context. Sane?
Makefile | 7 +++++++
1 files changed, 7 insertions(+), 0 deletions(-)
diff --git a/Makefile b/Makefile
index 9afdcf2a..ab64ff4c 100644
--- a/Makefile
+++ b/Makefile
@@ -226,6 +226,10 @@ all::
# Define NO_CROSS_DIRECTORY_HARDLINKS if you plan to distribute the installed
# programs as a tar, where bin/ and libexec/ might be on different file systems.
#
+# Define NO_HARDLINKS if you plan to distribute the installed programs as a tar
+# that might be extracted on a filesystem like btrfs that does not cope well
+# with many links to one inode in one directory.
+#
# Define USE_NED_ALLOCATOR if you want to replace the platforms default
# memory allocators with the nedmalloc allocator written by Niall Douglas.
#
@@ -2326,12 +2330,14 @@ endif
} && \
for p in $(filter $(install_bindir_programs),$(BUILT_INS)); do \
$(RM) "$$bindir/$$p" && \
+ test -z "$(NO_HARDLINKS)" && \
ln "$$bindir/git$X" "$$bindir/$$p" 2>/dev/null || \
ln -s "git$X" "$$bindir/$$p" 2>/dev/null || \
cp "$$bindir/git$X" "$$bindir/$$p" || exit; \
done && \
for p in $(BUILT_INS); do \
$(RM) "$$execdir/$$p" && \
+ test -z "$(NO_HARDLINKS)" && \
ln "$$execdir/git$X" "$$execdir/$$p" 2>/dev/null || \
ln -s "git$X" "$$execdir/$$p" 2>/dev/null || \
cp "$$execdir/git$X" "$$execdir/$$p" || exit; \
@@ -2339,6 +2345,7 @@ endif
remote_curl_aliases="$(REMOTE_CURL_ALIASES)" && \
for p in $$remote_curl_aliases; do \
$(RM) "$$execdir/$$p" && \
+ test -z "$(NO_HARDLINKS)" && \
ln "$$execdir/git-remote-http$X" "$$execdir/$$p" 2>/dev/null || \
ln -s "git-remote-http$X" "$$execdir/$$p" 2>/dev/null || \
cp "$$execdir/git-remote-http$X" "$$execdir/$$p" || exit; \
--
1.7.7
^ permalink raw reply related
* [PATCH] fix "git apply --index ..." not to deref NULL
From: Jim Meyering @ 2011-10-12 8:18 UTC (permalink / raw)
To: git
I noticed this when "git am CORRUPTED" unexpectedly failed with an
odd diagnostic, and even removed one of the files it was supposed
to have patched.
Reproduce with any valid old/new patch from which you have removed
the "+++ b/FILE" line. You'll see a diagnostic like this
fatal: unable to write file '(null)' mode 100644: Bad address
and you'll find that FILE has been removed.
The above is on glibc-based systems. On other systems, rather than
getting "null" in parentheses, you'll probably provoke a segfault,
as git tries to dereference the NULL file name.
Signed-off-by: Jim Meyering <meyering@redhat.com>
---
builtin/apply.c | 3 +++
t/t4254-am-corrupt.sh | 43 +++++++++++++++++++++++++++++++++++++++++++
2 files changed, 46 insertions(+), 0 deletions(-)
create mode 100644 t/t4254-am-corrupt.sh
diff --git a/builtin/apply.c b/builtin/apply.c
index f2edc52..aaa39fe 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -1407,6 +1407,9 @@ static int find_header(char *line, unsigned long size, int *hdrsize, struct patc
"%d leading pathname components (line %d)" , p_value, linenr);
patch->old_name = patch->new_name = patch->def_name;
}
+ if (!patch->is_delete && !patch->new_name)
+ die("git diff header lacks filename information "
+ "(line %d)", linenr);
patch->is_toplevel_relative = 1;
*hdrsize = git_hdr_len;
return offset;
diff --git a/t/t4254-am-corrupt.sh b/t/t4254-am-corrupt.sh
new file mode 100644
index 0000000..b7da95f
--- /dev/null
+++ b/t/t4254-am-corrupt.sh
@@ -0,0 +1,43 @@
+#!/bin/sh
+
+test_description='git am with corrupt input'
+. ./test-lib.sh
+
+# Note the missing "+++" line:
+cat > bad-patch.diff <<'EOF'
+From: A U Thor <au.thor@example.com>
+diff --git a/f b/f
+index 7898192..6178079 100644
+--- a/f
+@@ -1 +1 @@
+-a
++b
+EOF
+
+test_expect_success setup '
+ test $? = 0 &&
+ echo a > f &&
+ git add f &&
+ test_tick &&
+ git commit -m initial
+'
+
+# This used to fail before, too, but with a different diagnostic.
+# fatal: unable to write file '(null)' mode 100644: Bad address
+# Also, it had the unwanted side-effect of deleting f.
+test_expect_success 'try to apply corrupted patch' '
+ git am bad-patch.diff 2> actual
+ test $? = 1
+'
+
+cat > expected <<EOF
+fatal: git diff header lacks filename information (line 4)
+EOF
+
+test_expect_success 'compare diagnostic; ensure file is still here' '
+ test $? = 0 &&
+ test -f f &&
+ test_cmp expected actual
+'
+
+test_done
--
1.7.7
^ permalink raw reply related
* Re: [PATCH 00/20] [GIT PULL][v3.2] tracing: queued updates
From: Ingo Molnar @ 2011-10-12 8:07 UTC (permalink / raw)
To: Valdis.Kletnieks, git
Cc: Steven Rostedt, linux-kernel, Andrew Morton, Thomas Gleixner,
Frederic Weisbecker
In-Reply-To: <20002.1318367320@turing-police.cc.vt.edu>
* Valdis.Kletnieks@vt.edu <Valdis.Kletnieks@vt.edu> wrote:
> On Tue, 11 Oct 2011 07:50:17 +0200, Ingo Molnar said:
>
> > $ git pull git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-2.6-trace.git tip/perf/core
> > fatal: The remote end hung up unexpectedly
>
> Is it possible to get 'git' to say something more informative than
> "hung up unexpectedly"? "Tree not found, check URL" or similar
> would be nice...
I've Cc:-ed the Git development list which has lots of very
responsive gents on it eager to improve Git.
I see this message dozens of times per year and i am always confused
about the message.
Firstly, arguably, typoing something is not 'fatal' really - it's
just a resource that was not found on the server.
Secondly, and more importantly, the reason for the failed pull is
indeed important to know, if you want to resolve the problem with a
minimum fuss:
- Was it the tree that didnt exist?
- Or the branch?
- Or was there some other problem [such as a truly unexpectedly
closed transport socket]?
It's really useful for a painless UI flow to disambiguate failure
messages into clearly actionable variants.
Thanks,
Ingo
^ permalink raw reply
* Re: "trying out" gitolite with minimum impact on a system
From: Sitaram Chamarty @ 2011-10-12 7:52 UTC (permalink / raw)
To: Philippe Vaucher; +Cc: Git Mailing List
In-Reply-To: <CAGK7Mr6cnP6QQwGswWwQYiGR2_BUjMPz+VsygQXb0Voehm+akg@mail.gmail.com>
On Wed, Oct 12, 2011 at 11:45 AM, Philippe Vaucher
<philippe.vaucher@gmail.com> wrote:
>> After that, entirely within that user, you have an admin user and six
>> normal users, to do with as you please. You emulate different users
>> simply by using a different username in the URL, like "git clone
>> u1:reponame" versus "git clone u2:reponame".
>
> Hum, except if I missed something the classic way to use gitolite is
> to always clone using the same user (git@host:repository.git), and the
> "real" identification is done by the ssh keys (which means that
> contrary to plain ssh you lose the ability to have two users with the
> same ssh key, which should never happen anyway).
>
> But maybe you're refering to an alternate authentification mechanism
> within gitolite I'm unaware of.
you should read "entirely within that user" as "entirely within that
new, possibly throw-away, Unix userid you created". This is meant to
fit with the subject line's "minimum impact on a system" phrase.
regards
sitaram
^ permalink raw reply
* Re: [PATCH] Improving performance with pthreads in refresh_index().
From: Simon Klinkert @ 2011-10-12 7:21 UTC (permalink / raw)
To: Michael J Gruber; +Cc: git
In-Reply-To: <4E942D76.7030908@drmicha.warpmail.net>
Okay. It seems like my idea was already implemented by Linus. I wasn't aware of that fact.
Anyway, I've learned a bit more about git's bowels. Thanks a lot.
Simon
On 11.10.2011, at 13:50, Michael J Gruber wrote:
> klinkert@webgods.de venit, vidit, dixit 11.10.2011 11:32:
>> Git performs for every file in a repository at least one (with a cold cache)
>> lstat(). In larger repositories operations like git status take a
>> long time. In case your local repository is located on a remote server
>> (e. g. mounted via nfs) it ends up in an *incredible* slow git.
>>
>> With this patch you're able to determine a number of threads (maxthreads)
>> in your config file to run these tons of lstats in threads. There
>> won't be created any pthreads if you haven't set maxthreads. In my
>> test cases a git status with this patch performs enormously faster (over
>> two minutes before and approximately 25 seconds now). Of course, it
>> has a positive impact on other git commands, too.
>
> Can you specify under which circumstances one should get a speedup? Our
> NFS isn't slow enough... but on a dead slow sshfs work tree I get the
> following for "git status -s":
>
> maxthreads: 0, preloadindex: false, time: 14.73
> maxthreads: 1, preloadindex: false, time: 14.25
> maxthreads: 2, preloadindex: false, time: 13.32
> maxthreads: 3, preloadindex: false, time: 12.40
> maxthreads: 4, preloadindex: false, time: 12.65
> maxthreads: 5, preloadindex: false, time: 12.16
> maxthreads: 8, preloadindex: false, time: 12.32
> maxthreads: 10, preloadindex: false, time: 11.98
> maxthreads: 15, preloadindex: false, time: 12.31
> maxthreads: 20, preloadindex: false, time: 12.00
> maxthreads: 0, preloadindex: true, time: 12.17
> maxthreads: 1, preloadindex: true, time: 11.98
> maxthreads: 2, preloadindex: true, time: 12.21
> maxthreads: 3, preloadindex: true, time: 11.99
> maxthreads: 4, preloadindex: true, time: 12.14
> maxthreads: 5, preloadindex: true, time: 12.21
> maxthreads: 8, preloadindex: true, time: 12.14
> maxthreads: 10, preloadindex: true, time: 12.08
> maxthreads: 15, preloadindex: true, time: 12.16
> maxthreads: 20, preloadindex: true, time: 11.96
>
> So it seams it gives me what preloadindex does, which is not much.
>
> Note: I'm not saying the patch is bad. I'm just wondering whether that
> is expected.
>
> Michael
> P.S.: It's actually sshfs with ssh to an NFSv3 client (server restricts
> exports) :(
^ permalink raw reply
* Re: "trying out" gitolite with minimum impact on a system
From: Philippe Vaucher @ 2011-10-12 6:15 UTC (permalink / raw)
To: Sitaram Chamarty; +Cc: Git Mailing List
In-Reply-To: <CAMK1S_g5CnP+vrE71cqMgcjpj8ocE+wdtA2vPjeaXGCRNt25Dw@mail.gmail.com>
> After that, entirely within that user, you have an admin user and six
> normal users, to do with as you please. You emulate different users
> simply by using a different username in the URL, like "git clone
> u1:reponame" versus "git clone u2:reponame".
Hum, except if I missed something the classic way to use gitolite is
to always clone using the same user (git@host:repository.git), and the
"real" identification is done by the ssh keys (which means that
contrary to plain ssh you lose the ability to have two users with the
same ssh key, which should never happen anyway).
But maybe you're refering to an alternate authentification mechanism
within gitolite I'm unaware of.
Philippe
^ permalink raw reply
* Re: [RFC/PATCH]: reverse bisect v 2.0
From: Junio C Hamano @ 2011-10-12 4:57 UTC (permalink / raw)
To: Andrew Ardill
Cc: Christian Couder, Jeff King, Michal Vyskocil, git,
Sverre Rabbelier, Johannes Sixt
In-Reply-To: <CAH5451kUf=vPfgOOusmJjfbiyueX9VByJLzZ9WbyqLd0z78wxA@mail.gmail.com>
Andrew Ardill <andrew.ardill@gmail.com> writes:
> Examples.
> Search for a feature add:
> $ git bisect start --introduced='feature: git frotz says xyzzy' v0.99 master
> Bisecting: 171 revisions left to test after this (roughly 8 steps)
> $ ... build and then test ...
> $ git bisect tested
> Does this snapshot have 'feature: git frotz says xyzzy' [y/n]? yes
> # already added, look backwards
>
> Search for a feature regression:
> $ git bisect start --removed='feature: git frotz says xyzzy' v0.99 master
> Bisecting: 171 revisions left to test after this (roughly 8 steps)
> $ ... build and then test ...
> $ git bisect tested
> Does this snapshot have 'feature: git frotz says xyzzy' [y/n]? yes
> # not removed yet, look forwards
With an obvious addition of non-interactive short-cut subcommands "git
bisect yes" and "git bisect no", I think --removed= is a much better
wording than --used-to= I suggested in the discussion.
I however am still worried about the flipping of the mapping between
<good,bad> and <yes,no> this design requires. What are we going to do to
the labels of low-level machinery (i.e $GIT_DIR/refs/bisect/bad and
$GIT/refs/bisect/good)? They appear in "bisect visualize" and I was hoping
that it would be simpler in the code if we do not have to change them in
such a way that depends on this introduced/removed switch, and that was
the reason why I was trying to see if we can solve this without the
switchable mapping between <good,bad> and <yes,no>.
More specifically, I was hoping that we can rename "good" to "old" and
"bad" to "new" unconditionally and be done with it. We would ask the user
"What did the code used to do in the olden days?" and "Does this version
behave the same as it used to?". The possible answers the user can give
are "git bisect old" (it behaves the same as the older versions) and "git
bisect new" (it behaves the same as the newer versions). Then we do not
have to worry about having to flip the meaning of <yes> and <no> at the UI
level.
^ permalink raw reply
* Re: Re* [PATCH v3 19/22] resolve_ref(): emit warnings for improperly-formatted references
From: Jeff King @ 2011-10-12 4:50 UTC (permalink / raw)
To: Junio C Hamano
Cc: Michael Haggerty, git, cmn, A Large Angry SCM, Daniel Barkalow,
Sverre Rabbelier
In-Reply-To: <7v39eyddoc.fsf@alter.siamese.dyndns.org>
On Tue, Oct 11, 2011 at 09:41:39PM -0700, Junio C Hamano wrote:
> Whether we remove the warning or not, I think it would be an improvement
> not to look at random files directly underneath $GIT_DIR/. I am not sure
> how we can be confident that we caught everything, though.
>
> In other words, is shorten-unambiguous-refs the last one that needs
> fixing? How would we know for certain?
My assumption was that the set of rules is defined by
ref_rev_parse_rules, so grepping for functions that access that would be
sufficient.
It looks like there is also ref_fetch_rules, which serves a similar
purpose. Uses of that would also need to be audited.
I _think_ that should be enough for arbitrary lookup. I'm sure other
callsites directly say things like 'git_path("MERGE_HEAD")', but that's
not a problem. They would be doing so with a well-defined top-level
refname. This is really just about the ref_rev_parse_rules lookups.
-Peff
^ permalink raw reply
* Re: [RFC] teach --edit to git rebase
From: Junio C Hamano @ 2011-10-12 4:44 UTC (permalink / raw)
To: Jean Privat; +Cc: git
In-Reply-To: <CAMQw0oOBEjW3yS2+wcktXDuEuUiHKjfbK2qDzKvBOiwxo7Zkow@mail.gmail.com>
Jean Privat <jean.privat@gmail.com> writes:
> Yes I know "show me the code" but because I am lazy I prefer ask 1-if
> the proposal makes sense, and 2-if the following way of doing it makes
> sense.
No, no, and no we do not necessarily hate talking to lazy people but only
as long as what they propose makes some sense.
The only thing you can do with this new option is "update one commit
buried in the history, and rebase everything that build on top of it", as
far as I can tell. It feels a shame to waste the generic word "--edit" for
such a narrow option.
At the UI level, "git commit --amend HEAD~4" might be a more natural way
to invoke such an operation, I would think.
^ permalink raw reply
* Re: Re* [PATCH v3 19/22] resolve_ref(): emit warnings for improperly-formatted references
From: Junio C Hamano @ 2011-10-12 4:41 UTC (permalink / raw)
To: Jeff King
Cc: Michael Haggerty, git, cmn, A Large Angry SCM, Daniel Barkalow,
Sverre Rabbelier
In-Reply-To: <20111012021128.GA32149@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> At any rate, I think the changes should be all or nothing. If the
> warning goes away, fine. But if the warning stays, and dwim_ref is going
> to have special rules for looking in the top-level $GIT_DIR, then things
> like shorten_unambiguous_ref need to respect those rules, or we've just
> created a new bug.
Whether we remove the warning or not, I think it would be an improvement
not to look at random files directly underneath $GIT_DIR/. I am not sure
how we can be confident that we caught everything, though.
In other words, is shorten-unambiguous-refs the last one that needs
fixing? How would we know for certain?
Also I tend to think Michael's "only warn in refs/" is probably not the
right solution. When a caller asks to resolve_ref(MERGE_HEAD), one of
these things can be true:
- A file $GIT_DIR/MERGE_HEAD does not exist; this is not inherently an
error unless we were supposed to be in the middle of a conflicted
merge.
- A file $GIT_DIR/MERGE_HEAD exists, and records a correct 40-hexadecimal
get_sha1_hex() can grok. This is perfectly normal.
- A file $GIT_DIR/MERGE_HEAD exists, but get_sha1_hex() does not grok
it. Michael warns against this twice, and I think it is a wrong thing
to pass this unnoticed.
Once we tighten all the "too loose accesses to $GIT_DIR/$random_filename",
we might even want to have an option to cause the caller to die() in the
error case, and the logic is the same for refs under $GIT_DIR/refs/, not
just the ref-like-things directly under $GIT_DIR.
A regular ref, can also appear in $GIT_DIR/packed-refs, but a corruption
of an entry in the file will be caught when the file is read and outside
the scope of this discussion, I think.
^ permalink raw reply
* Re: Re* [PATCH v3 19/22] resolve_ref(): emit warnings for improperly-formatted references
From: Michael Haggerty @ 2011-10-12 2:56 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jeff King, git, cmn, A Large Angry SCM, Daniel Barkalow,
Sverre Rabbelier
In-Reply-To: <7vehyjcckp.fsf@alter.siamese.dyndns.org>
On 10/12/2011 01:50 AM, Junio C Hamano wrote:
> It starts sounding like that the ill-thought-out warning should be ripped
> out regardless of what other things we do.
ISTM that the warning is proving its worth already by illuminating some
questionable practices (treating any file in $GIT_DIR as a potential
reference) :-)
However, it might be that fixing 100% of the questionable practices is
too much work. In that case, I suggest that the warning not be ripped
out altogether, but rather that the warning only be emitted for invalid
references whose names start with "refs/".
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
^ 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