* Re: [PATCH v2] unpack-trees: do not abort when overwriting an existing file with the same content
From: Jeff King @ 2013-01-21 23:15 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, Junio C Hamano
In-Reply-To: <1358768433-26096-1-git-send-email-pclouds@gmail.com>
On Mon, Jan 21, 2013 at 06:40:33PM +0700, Nguyen Thai Ngoc Duy wrote:
> + /*
> + * If it has the same content that we are going to overwrite,
> + * there's no point in complaining. We still overwrite it in
> + * the end though.
> + */
> + if (ce &&
> + S_ISREG(st->st_mode) && S_ISREG(ce->ce_mode) &&
> + (!trust_executable_bit ||
> + (0100 & (ce->ce_mode ^ st->st_mode)) == 0) &&
> + st->st_size < SAME_CONTENT_SIZE_LIMIT &&
> + sha1_object_info(ce->sha1, &ce_size) == OBJ_BLOB &&
> + ce_size == st->st_size) {
> + void *buffer = NULL;
> + unsigned long size;
> + enum object_type type;
> + struct strbuf sb = STRBUF_INIT;
> + int matched =
> + strbuf_read_file(&sb, ce->name, ce_size) == ce_size &&
> + (buffer = read_sha1_file(ce->sha1, &type, &size)) != NULL &&
> + type == OBJ_BLOB &&
> + size == ce_size &&
> + !memcmp(buffer, sb.buf, size);
> + free(buffer);
> + strbuf_release(&sb);
> + if (matched)
> + return 0;
> + }
Can you elaborate on when this code is triggered?
In the general case, shouldn't we already know the sha1 of what's on
disk in the index, and be able to just compare the hashes? And if we
don't, because the entry is start-dirty, should we be updating it
(possibly earlier, so we do not even get into the "need to write" code
path) instead of doing this ad-hoc byte comparison?
Confused...
-Peff
^ permalink raw reply
* Re: [RFC] git rm -u
From: Junio C Hamano @ 2013-01-21 23:18 UTC (permalink / raw)
To: Matthieu Moy
Cc: Piotr Krukowiecki, Jonathan Nieder, Eric James Michael Ritz,
Git Mailing List, Tomas Carnecky
In-Reply-To: <vpqhamapal1.fsf@grenoble-inp.fr>
Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> But v1.5.2.5~1 (git-add -u paths... now works from subdirectory,
>> 2007-08-16) changed the semantics to limit the operation to the
>> working tree.
>
> Not really. It fixed "git add -u path", not plain "git add -u". A quick
> test checking out and compiling v1.5.2.5~1^ shows that "git add -u ."
> from a subdirectory was adding everything from the root.
>
> My interpretation is that v1.5.2.5~1 fixed an actual bug, without
> thinking about what would happen when "git add -u" was called without
> path, so the behavior is "what happens to be the most natural to
> implement".
I guess at this point it does not matter that much if that was an
unintended consequence of a buggy fix, or a new behaviour by design.
We initially were tree-wide but later limited the operation to the
current directory.
I think your "Check 'git diff' then run 'git add -u'" example may be
a good enough argument that it is a good idea to restore the
originally intended "tree-wide" behaviour in any case.
^ permalink raw reply
* Re: [RFC] Instruct git-completion.bash that we are in test mode
From: Junio C Hamano @ 2013-01-21 23:32 UTC (permalink / raw)
To: Jean-Noël AVILA; +Cc: git
In-Reply-To: <201301212330.10824.jn.avila@free.fr>
"Jean-Noël AVILA" <jn.avila@free.fr> writes:
> At least, "it works for me".
I suspect that your approach will still not fix the case in which
you build a branch with a new command git-check-ignore, and then
check out another branch that does not yet have that command without
first running "make clean".
Does the following really pass with your patch?
git checkout origin/next
make
git checkout origin/maint
git apply your_patch.mbox
make
cd t && sh ./t9902-completion.sh
> + __git_cmdlist () { git help -a| egrep -m 1 -B1000 PATH | egrep '^ [a-zA-Z0-9]'; }
'egrep' is not even in POSIX in the first place but grep -E ought to
be a replacement for it, so I'll let it pass, but "-m1 -B1000"?
Please stay within portable options.
git help -a |
sed -n -e '/^ [a-z]/p' -e '/^git commands available from elsewhere/q'/'
might be a good enough substitute, I think, if we were to take your
approach, but I suspect it needs a lot more to limit the output in
the test mode.
^ permalink raw reply
* Re: [PATCH 0/2] Hiding some refs in ls-remote
From: Junio C Hamano @ 2013-01-21 23:33 UTC (permalink / raw)
To: Jeff King; +Cc: git, spearce, mfick
In-Reply-To: <20130121230108.GB17156@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
>> It may even make sense to have "git push" honor it, excluding them
>> from "git push --mirror" (or "git push --all" if some of the
>> branches are hidden); I haven't thought things through, though.
>
> That is harder, as that is something that happens on the client. How
> does the client learn about the transfer.hiderefs setting on the remote?
No, I was talking about running "git push" from a repository that
has the transfer.hiderefs set, emulating a fetch from a client by
pushing out in the reverse direction.
^ permalink raw reply
* Re: [PATCH v6 0/8] push: update remote tags only with force
From: Jeff King @ 2013-01-21 23:40 UTC (permalink / raw)
To: Chris Rorvick
Cc: Junio C Hamano, Max Horn, git, Angelo Borsotti, Drew Northup,
Michael Haggerty, Philip Oakley, Johannes Sixt, Kacper Kornet,
Felipe Contreras
In-Reply-To: <CAEUsAPZr+bNNA-pqrQbBGvku4T3h58Ub66mK2zLeHqghEKw5Aw@mail.gmail.com>
On Thu, Jan 17, 2013 at 09:18:50PM -0600, Chris Rorvick wrote:
> On Thu, Jan 17, 2013 at 7:06 PM, Jeff King <peff@peff.net> wrote:
> > However, if instead of the rule being
> > "blobs on the remote side cannot be replaced", if it becomes "the old
> > value on the remote side must be referenced by what we replace it with",
> > that _is_ something we can calculate reliably on the sending side.
>
> Interesting. I would have thought knowing reachability implied having
> the old object in the sending repository.
No, because if you do not have it, then you know it is not reachable
from your refs (or your repository is corrupted). If you do have it, it
_might_ be reachable. For commits, checking is cheap (merge-base) and we
already do it. For trees and blobs, it is much more expensive, as you
have to walk the whole object graph. While it might be "more correct"
in some sense to say "it's OK to replace a tree with a commit that
points to it", in practice I doubt anyone cares, so you can probably
just punt on those ones and say "no, it's not a fast forward".
> > And
> > that is logically an extension of the fast-forward rule, which is why I
> > suggested placing it with ref_newer (but the latter should probably be
> > extended to not suggest merging if we _know_ it is a non-commit object).
>
> Sounds great, especially if it is not dependent on the sender actually
> having the old object. Until this is implemented, though, I don't
> understand what was wrong with doing the checks in the
> is_forwardable() helper function (of course after fixing the
> regression/bug.)
I don't think it is wrong per se; I just think that the check would go
more naturally where we are checking whether the object does indeed
fast-forward. Because is_forwardable in some cases must say "I don't
know; I don't have the object to check its type, so maybe it is
forwardable, and maybe it is not". Whereas when we do the actual
reachability check, we can say definitely "this is not reachable because
I don't have it, or this is not reachable because it is a commit and I
checked, or this might be reachable but I don't care to check because it
has a funny type".
I think looking at it as the latter makes it more obvious how to handle
the "maybe" situation (e.g., the bug in is_forwardable was hard to see).
Anyway, I do not care that much where it goes. To me, the important
thing is the error message. I do think the error "already exists" is a
reasonable one for refs/tags (we do not allow non-force pushes of
existing tags), but not necessarily for other cases, like trying to push
a blob over a blob. The problem there is not "already exists" but rather
"a blob is not something that can fast-forward". Using the existing
REJECT_NONFASTFORWARD is insufficient (because later code will recommend
pull-then-push, which is wrong). So I'd be in favor of creating a new
error status for it.
-Peff
^ permalink raw reply
* Re: [PATCH 0/2] Hiding some refs in ls-remote
From: Jeff King @ 2013-01-21 23:45 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, spearce, mfick
In-Reply-To: <7v38xuf6w5.fsf@alter.siamese.dyndns.org>
On Mon, Jan 21, 2013 at 03:33:46PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> >> It may even make sense to have "git push" honor it, excluding them
> >> from "git push --mirror" (or "git push --all" if some of the
> >> branches are hidden); I haven't thought things through, though.
> >
> > That is harder, as that is something that happens on the client. How
> > does the client learn about the transfer.hiderefs setting on the remote?
>
> No, I was talking about running "git push" from a repository that
> has the transfer.hiderefs set, emulating a fetch from a client by
> pushing out in the reverse direction.
Ah. That seems a bit more questionable to me, as you are assuming that
the face that the repository shows to network clients is the same as it
would show when it is the client itself. That wouldn't be true, for
example, when pushing to a backup repository which would expect to get
everything.
Of course the same problem comes from a backup repository which wants to
fetch from you. Which again comes down to the fact that I think this
ref-hiding is really in the eye of the receiver, not the sender.
-Peff
^ permalink raw reply
* Re: [PATCH v6 0/8] push: update remote tags only with force
From: Junio C Hamano @ 2013-01-21 23:53 UTC (permalink / raw)
To: Jeff King
Cc: Chris Rorvick, Max Horn, git, Angelo Borsotti, Drew Northup,
Michael Haggerty, Philip Oakley, Johannes Sixt, Kacper Kornet,
Felipe Contreras
In-Reply-To: <20130121234002.GE17156@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> ... The problem there is not "already exists" but rather
> "a blob is not something that can fast-forward". Using the existing
> REJECT_NONFASTFORWARD is insufficient (because later code will recommend
> pull-then-push, which is wrong). So I'd be in favor of creating a new
> error status for it.
Very well said.
Please make it so ;-) or should I?
^ permalink raw reply
* Re: [PATCH v3 1/6] Change old system name 'GIT' to 'Git'
From: Junio C Hamano @ 2013-01-22 0:39 UTC (permalink / raw)
To: Thomas Ackermann; +Cc: davvid, git
In-Reply-To: <1335904329.632749.1358795780375.JavaMail.ngmail@webmail20.arcor-online.net>
Thomas Ackermann <th.acker@arcor.de> writes:
> Git changed its 'official' system name from 'GIT' to 'Git' in v1.6.5.3
> (as can be seen in the corresponding release note where 'GIT' was
> changed to 'Git' in the header line). So change every occurrence
> of 'GIT" in the documention to 'Git'.
>
> Signed-off-by: Thomas Ackermann <th.acker@arcor.de>
> ---
This is more about "stop using poor-man's small caps".
I think it misses "GIT - the stupid content tracker" in README, but
probably it is OK (it is not an end-user facing documentation).
I noticed that these two places still use poor-man's small caps
after this patch.
* Documentation/SubmittingPatches:
that are being emailed around. Although core GIT is a lot
* Documentation/git-credential.txt:
TYPICAL USE OF GIT CREDENTIAL
With these two updated, I think it is a sensible change and the
patch is timed reasonably well. It applies to 'maint', and other
topics in flight do not seem to add new uses of GIT.
Not commenting on other 5 patches in the series yet, but if they
interact with other topics in flight, they may have to be separated
out. We'll see.
Thanks.
^ permalink raw reply
* Re: [RFC] Instruct git-completion.bash that we are in test mode
From: Jeff King @ 2013-01-22 0:39 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jean-Noël AVILA, git
In-Reply-To: <7v7gn6f6ya.fsf@alter.siamese.dyndns.org>
On Mon, Jan 21, 2013 at 03:32:29PM -0800, Junio C Hamano wrote:
> "Jean-Noël AVILA" <jn.avila@free.fr> writes:
>
> > At least, "it works for me".
>
> I suspect that your approach will still not fix the case in which
> you build a branch with a new command git-check-ignore, and then
> check out another branch that does not yet have that command without
> first running "make clean".
>
> Does the following really pass with your patch?
>
> git checkout origin/next
> make
> git checkout origin/maint
> git apply your_patch.mbox
> make
> cd t && sh ./t9902-completion.sh
I really hate to suggest this, but should it be more like:
if test -z "$FAKE_COMMAND_LIST"; then
__git_cmdlist() {
git help -a | egrep '^ [a-zA-Z0-9]'
}
else
__git_cmdlist() {
printf '%s' "$FAKE_COMMAND_LIST"
}
fi
That gives us a nice predictable starting point for actually testing the
completion code. The downside is that it doesn't let us test that we
remain compatible with the output of "help -a". But we could potentially
add a single, more liberal test (without $FAKE_COMMAND_LIST, but ready
to expect extra output) that checks that.
> > + __git_cmdlist () { git help -a| egrep -m 1 -B1000 PATH | egrep '^ [a-zA-Z0-9]'; }
>
> 'egrep' is not even in POSIX in the first place but grep -E ought to
> be a replacement for it, so I'll let it pass, but "-m1 -B1000"?
> Please stay within portable options.
If I recall correctly, egrep is actually more portable than "grep -E"
(and it is already in use, so I think we are OK). I agree on the rest,
though. :)
-Peff
^ permalink raw reply
* Re: [PATCH v3 2/6] Change 'git' to 'Git' whenever the whole system is referred to #1
From: Junio C Hamano @ 2013-01-22 0:41 UTC (permalink / raw)
To: Thomas Ackermann; +Cc: davvid, git
In-Reply-To: <1430594044.632790.1358795873467.JavaMail.ngmail@webmail20.arcor-online.net>
Thomas Ackermann <th.acker@arcor.de> writes:
> Signed-off-by: Thomas Ackermann <th.acker@arcor.de>
> ---
>
Forgot --stat?
It helps to check the integrity of patch application and also helps
anticipating possible interaction with other topics in flight.
Please don't omit it.
^ permalink raw reply
* Re: [PATCH v2] unpack-trees: do not abort when overwriting an existing file with the same content
From: Duy Nguyen @ 2013-01-22 0:59 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano
In-Reply-To: <20130121231515.GD17156@sigill.intra.peff.net>
On Tue, Jan 22, 2013 at 6:15 AM, Jeff King <peff@peff.net> wrote:
> Can you elaborate on when this code is triggered?
>
> In the general case, shouldn't we already know the sha1 of what's on
> disk in the index, and be able to just compare the hashes? And if we
> don't, because the entry is start-dirty, should we be updating it
> (possibly earlier, so we do not even get into the "need to write" code
> path) instead of doing this ad-hoc byte comparison?
>
> Confused...
git reset HEAD~10
# blah that was a mistake, undo it
git checkout HEAD@{1}
I hit it a few times, probably not with the exact recipe above though.
--
Duy
^ permalink raw reply
* Re: [RFC/PATCH] add: warn when -u or -A is used without filepattern
From: Duy Nguyen @ 2013-01-22 1:10 UTC (permalink / raw)
To: Matthieu Moy
Cc: git, gitster, Jonathan Nieder, Eric James Michael Ritz,
Tomas Carnecky
In-Reply-To: <1358769611-3625-1-git-send-email-Matthieu.Moy@imag.fr>
On Mon, Jan 21, 2013 at 7:00 PM, Matthieu Moy <Matthieu.Moy@imag.fr> wrote:
> Most git commands that can be used with our without a filepattern are
> tree-wide by default, the filepattern being used to restrict their scope.
> A few exceptions are: 'git grep', 'git clean', 'git add -u' and 'git add -A'.
>
> The inconsistancy of 'git add -u' and 'git add -A' are particularly
> problematic since other 'git add' subcommands (namely 'git add -p' and
> 'git add -e') are tree-wide by default.
>
> Flipping the default now is unacceptable, so this patch starts training
> users to type explicitely 'git add -u|-A :/' or 'git add -u|-A .', to prepare
> for the next steps:
>
> * forbid 'git add -u|-A' without filepattern (like 'git add' without
> option)
>
> * much later, maybe, re-allow 'git add -u|-A' without filepattern, with a
> tree-wide scope.
What about 'grep' and 'clean'? I think at least 'clean' should go
tree-wide default too. I don't mind grep go the same way either but I
think people voiced preference in current behavior..
--
Duy
^ permalink raw reply
* Re: [PATCH v2] unpack-trees: do not abort when overwriting an existing file with the same content
From: Junio C Hamano @ 2013-01-22 1:45 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Jeff King, git
In-Reply-To: <CACsJy8AFKXYkHbUf4aqBpCg+v06oFsvHq_zxQFE4BOCzTDAqtg@mail.gmail.com>
Duy Nguyen <pclouds@gmail.com> writes:
> On Tue, Jan 22, 2013 at 6:15 AM, Jeff King <peff@peff.net> wrote:
>> Can you elaborate on when this code is triggered?
>>
>> In the general case, shouldn't we already know the sha1 of what's on
>> disk in the index, and be able to just compare the hashes? And if we
>> don't, because the entry is start-dirty, should we be updating it
>> (possibly earlier, so we do not even get into the "need to write" code
>> path) instead of doing this ad-hoc byte comparison?
If the index knows what is in the working tree, I think I agree.
>> Confused...
>
> git reset HEAD~10
> # blah that was a mistake, undo it
> git checkout HEAD@{1}
>
> I hit it a few times, probably not with the exact recipe above though.
I've seen myself doing "git reset HEAD~10" by mistake (I wanted "--hard"),
but the recovery is to do "git reset --hard @{1}" and then come back
with "git reset --hard HEAD~10", so it hasn't been a real problem
for me.
A case similar to this is already covered by a special case of
two-tree read-tree where the index already matches the tree we are
checking out even though it is different from HEAD; in other words,
if you did this:
git init
date >file
git add file; git commit -m 'has a file'
git checkout -b side
git rm file; git commit -m 'does not have the file'
git checkout master file
: now index has the file from 'master' and worktree is clean
git checkout master
you shouldn't get any complaint, I think.
If you did "git rm --cached file" to lose it from the index,
immediately after "git checkout master file", then we wouldn't know
what we may be losing. If the file in the working tree happens to
match the index and the HEAD after checking out the other branch
('master' in this case), it is _not_ losing information, so we might
be able to treat it as an extension of the existing special case.
I haven't thought things through to see if in a more general case
that this codepath triggers we might be losing a local changes that
we should be carrying forward while checking out a new branch,
though.
^ permalink raw reply
* Re: [RFC/PATCH] add: warn when -u or -A is used without filepattern
From: Junio C Hamano @ 2013-01-22 1:51 UTC (permalink / raw)
To: Duy Nguyen
Cc: Matthieu Moy, git, Jonathan Nieder, Eric James Michael Ritz,
Tomas Carnecky
In-Reply-To: <CACsJy8B1=3gMfGUf3kyea9TyZmr1J7dbM1_+huMNrep24hwuiQ@mail.gmail.com>
Duy Nguyen <pclouds@gmail.com> writes:
> What about 'grep' and 'clean'? I think at least 'clean' should go
> tree-wide default too. I don't mind grep go the same way either but I
> think people voiced preference in current behavior..
I think the major argument for "git grep" to be the way it is is
because people expect it to be extension of running "grep -r" in the
same directory ("git grep" excludes untracked ones so you do not
have to suffer from --exclude=.git and such unpleasantries).
Shouldn't "git clean" be an extension of running "rm -r" in the same
directory ("git clean" does not lose tracked ones but otherwise it
is a recursive removal)?
^ permalink raw reply
* Re: [PATCH v2] unpack-trees: do not abort when overwriting an existing file with the same content
From: Duy Nguyen @ 2013-01-22 1:55 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git
In-Reply-To: <7vwqv6c7oe.fsf@alter.siamese.dyndns.org>
On Tue, Jan 22, 2013 at 8:45 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Duy Nguyen <pclouds@gmail.com> writes:
>
>> On Tue, Jan 22, 2013 at 6:15 AM, Jeff King <peff@peff.net> wrote:
>>> Can you elaborate on when this code is triggered?
>>>
>>> In the general case, shouldn't we already know the sha1 of what's on
>>> disk in the index, and be able to just compare the hashes? And if we
>>> don't, because the entry is start-dirty, should we be updating it
>>> (possibly earlier, so we do not even get into the "need to write" code
>>> path) instead of doing this ad-hoc byte comparison?
>
> If the index knows what is in the working tree, I think I agree.
>
>>> Confused...
>>
>> git reset HEAD~10
>> # blah that was a mistake, undo it
>> git checkout HEAD@{1}
>>
>> I hit it a few times, probably not with the exact recipe above though.
>
> I've seen myself doing "git reset HEAD~10" by mistake (I wanted "--hard"),
> but the recovery is to do "git reset --hard @{1}" and then come back
> with "git reset --hard HEAD~10", so it hasn't been a real problem
> for me.
Except when you already make some changes elsewhere and you don't want --hard.
--
Duy
^ permalink raw reply
* Re: [PATCH 1/2] Update :/abc ambiguity check
From: Duy Nguyen @ 2013-01-22 2:46 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vsj5ufia6.fsf@alter.siamese.dyndns.org>
On Tue, Jan 22, 2013 at 2:27 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
>
>> :/abc may mean two things:
>>
>> - as a revision, it means the revision that has "abc" in commit
>> message.
>>
>> - as a pathpec, it means "abc" from root.
>>
>> Currently we see ":/abc" as a rev (most of the time), but never see it
>> as a pathspec even if "abc" exists and "git log :/abc" will gladly
>> take ":/abc" as rev even it's ambiguous. This patch makes it:
>>
>> - ambiguous when "abc" exists on worktree
>> - a rev if abc does not exist on worktree
>> - a path if abc is not found in any commits (although better use
>
> The "any commits" above sounds very scary. Are you really going to
> check against all the commits?
If I remember correctly :/ will search through commit chains until it
finds a commit that matches. So :/non-existent-string definitely
searches through all commits.
>> "--" to avoid ambiguation because searching through commit DAG is
>> expensive)
>>
>> A plus from this patch is, because ":/" never matches anything as a
>> rev, it is never considered a valid rev and because root directory
>> always exists, ":/" is always unambiguously seen as a pathspec.
>
> That is the primary plus in practice, I think, and it is a big one.
>
> When naming a directory that belongs to a different subdirectory
> hierarchy, typing ":/that/directory/name" is not any easier than
> having your shell help you complete "../../that/directory/name"; I
> suspect nobody uses the relative-to-root notation to name anything
> but the root in real life.
As I noted in the patch comment, I do copy/paste repo-absolute paths
from a diff quite often (just skip the "a" and "b" prefix). Sometimes
I hope "git diff" has an option to produce relative paths..
--
Duy
^ permalink raw reply
* Re: [PATCH v3 08/31] parse_pathspec: add PATHSPEC_EMPTY_MATCH_ALL
From: Duy Nguyen @ 2013-01-22 2:46 UTC (permalink / raw)
To: Martin von Zweigbergk; +Cc: git, Junio C Hamano, Matthieu Moy
In-Reply-To: <CANiSa6gfTxOWzMg7V19PntDBhU4kW6qpa+K2XjnWgzNRXDKRSw@mail.gmail.com>
On Tue, Jan 22, 2013 at 6:12 AM, Martin von Zweigbergk
<martinvonz@gmail.com> wrote:
> Hi,
>
> I was tempted to ask this before, and the recent thread regarding "add
> -u/A" [1] convinced me to.
>
> On Sun, Jan 13, 2013 at 4:35 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
>> We have two ways of dealing with empty pathspec:
>>
>> 1. limit it to current prefix
>> 2. match the entire working directory
>>
>> Some commands go with #1, some with #2. get_pathspec() and
>> parse_pathspec() only supports #1. Make it support #2 too via
>> PATHSPEC_EMPTY_MATCH_ALL flag.
>
> If #2 is indeed the direction we want to go, then maybe we should make
> that the default behavior from parse_pathspec()? I.e. rename the flag
> "PATHSPEC_EMPTY_MATCH_PREFIX" (or something). Makes sense?
No problem with me. Will do unless someone objects.
> Btw, Matthieu was asking where we use #1. If you do invert the name
> and meaning of the flag, then the answer to that question should be
> (mostly?) obvious from a re-roll of your series (i.e. all the places
> where PATHSPEC_EMPTY_MATCH_PREFIX is used).
>
> Martin
>
> [1] http://thread.gmane.org/gmane.comp.version-control.git/213988/focus=214113
--
Duy
^ permalink raw reply
* Re: [PATCH 1/2] Update :/abc ambiguity check
From: Junio C Hamano @ 2013-01-22 4:10 UTC (permalink / raw)
To: Duy Nguyen; +Cc: git
In-Reply-To: <CACsJy8B1uxbJvP+0ZEx3br9_Qr9ZX7num8bcgd5sFS7XnvGNpw@mail.gmail.com>
Duy Nguyen <pclouds@gmail.com> writes:
>>> ... take ":/abc" as rev even it's ambiguous. This patch makes it:
>>>
>>> - ambiguous when "abc" exists on worktree
>>> - a rev if abc does not exist on worktree
>>> - a path if abc is not found in any commits (although better use
>>
>> The "any commits" above sounds very scary. Are you really going to
>> check against all the commits?
>
> If I remember correctly :/ will search through commit chains until it
> finds a commit that matches. So :/non-existent-string definitely
> searches through all commits.
That is the real work the user asked us to do, so it is not a wasted
latency. The description looked as if you were doing extra work
only for disambiguation, which triggered my "Huh?" meter.
^ permalink raw reply
* Re: Lockless Refs? (Was [PATCH] refs: do not use cached refs in repack_without_ref)
From: Drew Northup @ 2013-01-22 4:31 UTC (permalink / raw)
To: Jeff King
Cc: Martin Fick, Michael Haggerty, Shawn Pearce, git, Junio C Hamano
In-Reply-To: <20130105161215.GA24900@sigill.intra.peff.net>
On Sat, Jan 5, 2013 at 11:12 AM, Jeff King <peff@peff.net> wrote:
> On Mon, Dec 31, 2012 at 03:30:53AM -0700, Martin Fick wrote:
>
>> The general approach is to setup a transaction and either
>> commit or abort it. A transaction can be setup by renaming
>> an appropriately setup directory to the "ref.lock" name. If
>> the rename succeeds, the transaction is begun. Any actor can
>> abort the transaction (up until it is committed) by simply
>> deleting the "ref.lock" directory, so it is not at risk of
>> going stale.
>
> Deleting a directory is not atomic, as you first have to remove the
> contents, putting it into a potentially inconsistent state. I'll assume
> you deal with that later...
I know I'm a bit late to the dance here, but FWIW the apparent atomicy
(odd conjugation there) of directory deletion depends largely upon the
filesystem and VFS api in use. It is not unheard of that a delete
operation actually consist of moving the reference to the item's own
allocation marker into a "trashcan" to be cleaned up after later.
In other words, I'd not advise planning on directory deletes always
being atomic nor always not being atomic.
--
-Drew Northup
--------------------------------------------------------------
"As opposed to vegetable or mineral error?"
-John Pescatore, SANS NewsBites Vol. 12 Num. 59
^ permalink raw reply
* Re: [RFC] Instruct git-completion.bash that we are in test mode
From: Junio C Hamano @ 2013-01-22 4:31 UTC (permalink / raw)
To: Jeff King; +Cc: Jean-Noël AVILA, git
In-Reply-To: <20130122003954.GA23297@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> I really hate to suggest this, but should it be more like:
>
> if test -z "$FAKE_COMMAND_LIST"; then
> __git_cmdlist() {
> git help -a | egrep '^ [a-zA-Z0-9]'
> }
> else
> __git_cmdlist() {
> printf '%s' "$FAKE_COMMAND_LIST"
> }
> fi
>
> That gives us a nice predictable starting point for actually testing the
> completion code. The downside is that it doesn't let us test that we
> remain compatible with the output of "help -a".
Yeah, I think this is simpler and more to the point for the test in
t9902. If we really want to test something that is the same as, or
at least any closer than this approach (or my "help --standard"), to
what the real users use, the test has to become inherently flaky, so
I think we should go for the simplicity of this patch shows.
^ permalink raw reply
* Re: [PATCH v6 0/8] push: update remote tags only with force
From: Chris Rorvick @ 2013-01-22 4:59 UTC (permalink / raw)
To: Jeff King
Cc: Junio C Hamano, Max Horn, git, Angelo Borsotti, Drew Northup,
Michael Haggerty, Philip Oakley, Johannes Sixt, Kacper Kornet,
Felipe Contreras
In-Reply-To: <20130121234002.GE17156@sigill.intra.peff.net>
On Mon, Jan 21, 2013 at 5:40 PM, Jeff King <peff@peff.net> wrote:
> On Thu, Jan 17, 2013 at 09:18:50PM -0600, Chris Rorvick wrote:
>
>> On Thu, Jan 17, 2013 at 7:06 PM, Jeff King <peff@peff.net> wrote:
>> > However, if instead of the rule being
>> > "blobs on the remote side cannot be replaced", if it becomes "the old
>> > value on the remote side must be referenced by what we replace it with",
>> > that _is_ something we can calculate reliably on the sending side.
>>
>> Interesting. I would have thought knowing reachability implied having
>> the old object in the sending repository.
>
> No, because if you do not have it, then you know it is not reachable
> from your refs (or your repository is corrupted). If you do have it, it
> _might_ be reachable. For commits, checking is cheap (merge-base) and we
> already do it. For trees and blobs, it is much more expensive, as you
> have to walk the whole object graph. While it might be "more correct"
> in some sense to say "it's OK to replace a tree with a commit that
> points to it", in practice I doubt anyone cares, so you can probably
> just punt on those ones and say "no, it's not a fast forward".
Thanks for explaining this further. I'm not exactly sure what I was
thinking when I wrote the above other than I didn't fully grasp you
point and responded in a confused state. Clear on all fronts now.
>> > And
>> > that is logically an extension of the fast-forward rule, which is why I
>> > suggested placing it with ref_newer (but the latter should probably be
>> > extended to not suggest merging if we _know_ it is a non-commit object).
>>
>> Sounds great, especially if it is not dependent on the sender actually
>> having the old object. Until this is implemented, though, I don't
>> understand what was wrong with doing the checks in the
>> is_forwardable() helper function (of course after fixing the
>> regression/bug.)
>
> I don't think it is wrong per se; I just think that the check would go
> more naturally where we are checking whether the object does indeed
> fast-forward. Because is_forwardable in some cases must say "I don't
> know; I don't have the object to check its type, so maybe it is
> forwardable, and maybe it is not". Whereas when we do the actual
> reachability check, we can say definitely "this is not reachable because
> I don't have it, or this is not reachable because it is a commit and I
> checked, or this might be reachable but I don't care to check because it
> has a funny type".
>
> I think looking at it as the latter makes it more obvious how to handle
> the "maybe" situation (e.g., the bug in is_forwardable was hard to see).
>
> Anyway, I do not care that much where it goes. To me, the important
> thing is the error message. I do think the error "already exists" is a
> reasonable one for refs/tags (we do not allow non-force pushes of
> existing tags), but not necessarily for other cases, like trying to push
> a blob over a blob. The problem there is not "already exists" but rather
> "a blob is not something that can fast-forward". Using the existing
> REJECT_NONFASTFORWARD is insufficient (because later code will recommend
> pull-then-push, which is wrong). So I'd be in favor of creating a new
> error status for it.
I agree with everything above. I just don't understand why reverting
the "already exists" behavior for non-commit-ish objects was a
prerequisite to fixing this. Despite the flaws (I am not referring to
the buggy behavior) you and Junio have pointed out, this still seems
like an improvement over the previous (and soon-to-be current)
behavior. Saying the remote reference already exists is true, and it
implies that removing it might solve the problem which is also true.
Adding another error status will allow the error message to be made
clearer in both cases (i.e., I avoided the word "tag" specifically so
that it would apply to other cases, or so I thought.)
Chris
^ permalink raw reply
* Build broken for contrib/remote-helpers...
From: John Szakmeister @ 2013-01-22 5:49 UTC (permalink / raw)
To: git
I tried running make in contrib/remote-helpers and it died with:
:: make
make -e -C ../../t test
rm -f -r test-results
duplicate test numbers: /Users/jszakmeister/sources/git
make[1]: *** [test-lint-duplicates] Error 1
make: *** [test] Error 2
The path shown is not quite correct. I have the sources extracted to
/Users/jszakmeister/sources/git-1.8.1.1. It appears that the Makefile
in contrib/remote-helpers is exporting T, which is causing the
duplicate test detection to fail.
Thought you'd like to know!
-John
^ permalink raw reply
* [PATCH 0/3] Finishing touches to "push" advises
From: Junio C Hamano @ 2013-01-22 5:53 UTC (permalink / raw)
To: git; +Cc: Jeff King, Chris Rorvick
In-Reply-To: <20130121234002.GE17156@sigill.intra.peff.net>
This builds on Chris Rorvick's earlier effort to forbid unforced
updates to refs/tags/ hierarchy and giving sensible error and advise
messages for that case (we are not rejecting such a push due to fast
forwardness, and suggesting to fetch and integrate before pushing
again does not make sense).
The main change is in the second patch. When we
* do not have the object at the tip of the remote;
* the object at the tip of the remote is not a commit; or
* the object we are pushing is not a commit,
there is no point suggesting to fetch, integrate and push again.
If we do not have the current object at the tip of the remote, we
should tell the user to fetch first and evaluate the situation
before deciding what to do next.
Otherwise, if the current object is not a commit, or if we are
trying to push an object that is not a commit, then the user does
not have to fetch first (we already have the object), but it still
does not make sense to suggest to integrate and re-push. Just tell
them that such a push requires a force in such a case.
Junio C Hamano (3):
push: further clean up fields of "struct ref"
push: introduce REJECT_FETCH_FIRST and REJECT_NEEDS_FORCE
push: further reduce "struct ref" and simplify the logic
advice.c | 4 ++++
advice.h | 2 ++
builtin/push.c | 25 +++++++++++++++++++++++++
builtin/send-pack.c | 10 ++++++++++
cache.h | 6 +++---
remote.c | 38 ++++++++++++++++----------------------
send-pack.c | 2 ++
transport-helper.c | 10 ++++++++++
transport.c | 14 +++++++++++++-
transport.h | 2 ++
10 files changed, 87 insertions(+), 26 deletions(-)
--
1.8.1.1.498.gfdee8be
^ permalink raw reply
* [PATCH 1/3] push: further clean up fields of "struct ref"
From: Junio C Hamano @ 2013-01-22 5:53 UTC (permalink / raw)
To: git; +Cc: Jeff King, Chris Rorvick
In-Reply-To: <1358834027-32039-1-git-send-email-gitster@pobox.com>
The "nonfastforward" field is only used to decide what value to
assign to the "status" locally in a single function. Remove it from
the "struct ref" and make it into a local variable.
The "requires_force" field is not used to decide if the proposed
update requires a --force option to succeed, or to record such a
decision made elsewhere. It is used by status reporting code that
the particular update was "forced". Rename it to "forced_udpate",
and move the code to assign to it around to further clarify how it
is used and what it is used for.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
cache.h | 3 +--
remote.c | 10 +++++-----
transport.c | 2 +-
3 files changed, 7 insertions(+), 8 deletions(-)
diff --git a/cache.h b/cache.h
index a942bbd..baa47b4 100644
--- a/cache.h
+++ b/cache.h
@@ -1001,9 +1001,8 @@ struct ref {
char *symref;
unsigned int
force:1,
- requires_force:1,
+ forced_update:1,
merge:1,
- nonfastforward:1,
update:1,
deletion:1;
enum {
diff --git a/remote.c b/remote.c
index d3a1ca2..875296c 100644
--- a/remote.c
+++ b/remote.c
@@ -1322,22 +1322,22 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
!is_null_sha1(ref->old_sha1);
if (ref->update) {
- ref->nonfastforward =
+ int nonfastforward =
!has_sha1_file(ref->old_sha1)
- || !ref_newer(ref->new_sha1, ref->old_sha1);
+ || !ref_newer(ref->new_sha1, ref->old_sha1);
if (!prefixcmp(ref->name, "refs/tags/")) {
- ref->requires_force = 1;
if (!force_ref_update) {
ref->status = REF_STATUS_REJECT_ALREADY_EXISTS;
continue;
}
- } else if (ref->nonfastforward) {
- ref->requires_force = 1;
+ ref->forced_update = 1;
+ } else if (nonfastforward) {
if (!force_ref_update) {
ref->status = REF_STATUS_REJECT_NONFASTFORWARD;
continue;
}
+ ref->forced_update = 1;
}
}
}
diff --git a/transport.c b/transport.c
index 2673d27..585ebcd 100644
--- a/transport.c
+++ b/transport.c
@@ -659,7 +659,7 @@ static void print_ok_ref_status(struct ref *ref, int porcelain)
const char *msg;
strcpy(quickref, status_abbrev(ref->old_sha1));
- if (ref->requires_force) {
+ if (ref->forced_update) {
strcat(quickref, "...");
type = '+';
msg = "forced update";
--
1.8.1.1.498.gfdee8be
^ permalink raw reply related
* [PATCH 3/3] push: further reduce "struct ref" and simplify the logic
From: Junio C Hamano @ 2013-01-22 5:53 UTC (permalink / raw)
To: git; +Cc: Jeff King, Chris Rorvick
In-Reply-To: <1358834027-32039-1-git-send-email-gitster@pobox.com>
The "update" field in "struct ref" is only used in a very narrow
scope in a single function. Remove it.
Also simplify the code that rejects an attempted push by first
checking if the proposed update is forced (in which case we do not
need any check on our end).
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
cache.h | 1 -
remote.c | 42 +++++++++++++-----------------------------
2 files changed, 13 insertions(+), 30 deletions(-)
diff --git a/cache.h b/cache.h
index 360bba5..377a3df 100644
--- a/cache.h
+++ b/cache.h
@@ -1003,7 +1003,6 @@ struct ref {
force:1,
forced_update:1,
merge:1,
- update:1,
deletion:1;
enum {
REF_STATUS_NONE = 0,
diff --git a/remote.c b/remote.c
index 689dcf7..248910f 100644
--- a/remote.c
+++ b/remote.c
@@ -1317,37 +1317,21 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
* passing the --force argument
*/
- ref->update =
- !ref->deletion &&
- !is_null_sha1(ref->old_sha1);
-
- if (ref->update) {
- if (!prefixcmp(ref->name, "refs/tags/")) {
- if (!force_ref_update) {
- ref->status = REF_STATUS_REJECT_ALREADY_EXISTS;
- continue;
- }
- ref->forced_update = 1;
- } else if (!has_sha1_file(ref->old_sha1) ||
- !lookup_commit_reference_gently(ref->old_sha1, 1)) {
- if (!force_ref_update) {
- ref->status = REF_STATUS_REJECT_FETCH_FIRST;
- continue;
- }
- ref->forced_update = 1;
- } else if (!lookup_commit_reference_gently(ref->new_sha1, 1)) {
- if (!force_ref_update) {
- ref->status = REF_STATUS_REJECT_NEEDS_FORCE;
- continue;
- }
- ref->forced_update = 1;
- } else if (!ref_newer(ref->new_sha1, ref->old_sha1)) {
- if (!force_ref_update) {
- ref->status = REF_STATUS_REJECT_NONFASTFORWARD;
- continue;
- }
+ if (!ref->deletion && !is_null_sha1(ref->old_sha1)) {
+ if (force_ref_update) {
ref->forced_update = 1;
+ continue;
}
+
+ if (!prefixcmp(ref->name, "refs/tags/"))
+ ref->status = REF_STATUS_REJECT_ALREADY_EXISTS;
+ else if (!has_sha1_file(ref->old_sha1) ||
+ !lookup_commit_reference_gently(ref->old_sha1, 1))
+ ref->status = REF_STATUS_REJECT_FETCH_FIRST;
+ else if (!lookup_commit_reference_gently(ref->new_sha1, 1))
+ ref->status = REF_STATUS_REJECT_NEEDS_FORCE;
+ else if (!ref_newer(ref->new_sha1, ref->old_sha1))
+ ref->status = REF_STATUS_REJECT_NONFASTFORWARD;
}
}
}
--
1.8.1.1.498.gfdee8be
^ 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