* 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
* [PATCH 2/3] push: introduce REJECT_FETCH_FIRST and REJECT_NEEDS_FORCE
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>
When pushing update an existing ref, we wouldn't even know if we are
fast-forwarding the ref on the other end if:
* we do not have the object currently at the tip of remote;
* the object currently at the tip of remote is not a committish; or
* the object we are pushing is not a committish.
In such a case, the push has been rejected on the client end, but we
used the same error and advice messages as the ones used when
rejecting a non-fast-forward push, i.e. pull from there and
integrate before pushing again. This did not make much sense.
Introduce two error classes and suggest fetching from the other side
first and evaluate the situation, if we do not have the current
object, or just tell the user the update needs --force when we do
have the current object and either it or the object we are trying to
push is not a committish, in which case it can never fast-forward
and we know there is no point suggesting to merge.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
advice.c | 4 ++++
advice.h | 2 ++
builtin/push.c | 25 +++++++++++++++++++++++++
builtin/send-pack.c | 10 ++++++++++
cache.h | 2 ++
remote.c | 22 ++++++++++++++++------
send-pack.c | 2 ++
transport-helper.c | 10 ++++++++++
transport.c | 12 ++++++++++++
transport.h | 2 ++
10 files changed, 85 insertions(+), 6 deletions(-)
diff --git a/advice.c b/advice.c
index d287927..780f58d 100644
--- a/advice.c
+++ b/advice.c
@@ -5,6 +5,8 @@ int advice_push_non_ff_current = 1;
int advice_push_non_ff_default = 1;
int advice_push_non_ff_matching = 1;
int advice_push_already_exists = 1;
+int advice_push_fetch_first = 1;
+int advice_push_needs_force = 1;
int advice_status_hints = 1;
int advice_commit_before_merge = 1;
int advice_resolve_conflict = 1;
@@ -20,6 +22,8 @@ static struct {
{ "pushnonffdefault", &advice_push_non_ff_default },
{ "pushnonffmatching", &advice_push_non_ff_matching },
{ "pushalreadyexists", &advice_push_already_exists },
+ { "pushfetchfirst", &advice_push_fetch_first },
+ { "pushneedsforce", &advice_push_needs_force },
{ "statushints", &advice_status_hints },
{ "commitbeforemerge", &advice_commit_before_merge },
{ "resolveconflict", &advice_resolve_conflict },
diff --git a/advice.h b/advice.h
index 8bf6356..fad36df 100644
--- a/advice.h
+++ b/advice.h
@@ -8,6 +8,8 @@ extern int advice_push_non_ff_current;
extern int advice_push_non_ff_default;
extern int advice_push_non_ff_matching;
extern int advice_push_already_exists;
+extern int advice_push_fetch_first;
+extern int advice_push_needs_force;
extern int advice_status_hints;
extern int advice_commit_before_merge;
extern int advice_resolve_conflict;
diff --git a/builtin/push.c b/builtin/push.c
index 8491e43..da928fa 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -224,6 +224,13 @@ static const char message_advice_ref_already_exists[] =
N_("Updates were rejected because the destination reference already exists\n"
"in the remote.");
+static const char message_advice_ref_fetch_first[] =
+ N_("Updates were rejected; you need to fetch the destination reference\n"
+ "to decide what to do.\n");
+
+static const char message_advice_ref_needs_force[] =
+ N_("Updates were rejected; you need to force update.\n");
+
static void advise_pull_before_push(void)
{
if (!advice_push_non_ff_current || !advice_push_update_rejected)
@@ -252,6 +259,20 @@ static void advise_ref_already_exists(void)
advise(_(message_advice_ref_already_exists));
}
+static void advise_ref_fetch_first(void)
+{
+ if (!advice_push_fetch_first || !advice_push_update_rejected)
+ return;
+ advise(_(message_advice_ref_fetch_first));
+}
+
+static void advise_ref_needs_force(void)
+{
+ if (!advice_push_needs_force || !advice_push_update_rejected)
+ return;
+ advise(_(message_advice_ref_needs_force));
+}
+
static int push_with_options(struct transport *transport, int flags)
{
int err;
@@ -285,6 +306,10 @@ static int push_with_options(struct transport *transport, int flags)
advise_checkout_pull_push();
} else if (reject_reasons & REJECT_ALREADY_EXISTS) {
advise_ref_already_exists();
+ } else if (reject_reasons & REJECT_FETCH_FIRST) {
+ advise_ref_fetch_first();
+ } else if (reject_reasons & REJECT_NEEDS_FORCE) {
+ advise_ref_needs_force();
}
return 1;
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index f849e0a..57a46b2 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -44,6 +44,16 @@ static void print_helper_status(struct ref *ref)
msg = "non-fast forward";
break;
+ case REF_STATUS_REJECT_FETCH_FIRST:
+ res = "error";
+ msg = "fetch first";
+ break;
+
+ case REF_STATUS_REJECT_NEEDS_FORCE:
+ res = "error";
+ msg = "needs force";
+ break;
+
case REF_STATUS_REJECT_ALREADY_EXISTS:
res = "error";
msg = "already exists";
diff --git a/cache.h b/cache.h
index baa47b4..360bba5 100644
--- a/cache.h
+++ b/cache.h
@@ -1011,6 +1011,8 @@ struct ref {
REF_STATUS_REJECT_NONFASTFORWARD,
REF_STATUS_REJECT_ALREADY_EXISTS,
REF_STATUS_REJECT_NODELETE,
+ REF_STATUS_REJECT_FETCH_FIRST,
+ REF_STATUS_REJECT_NEEDS_FORCE,
REF_STATUS_UPTODATE,
REF_STATUS_REMOTE_REJECT,
REF_STATUS_EXPECTING_REPORT
diff --git a/remote.c b/remote.c
index 875296c..689dcf7 100644
--- a/remote.c
+++ b/remote.c
@@ -1322,17 +1322,26 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
!is_null_sha1(ref->old_sha1);
if (ref->update) {
- int nonfastforward =
- !has_sha1_file(ref->old_sha1)
- || !ref_newer(ref->new_sha1, ref->old_sha1);
-
if (!prefixcmp(ref->name, "refs/tags/")) {
if (!force_ref_update) {
ref->status = REF_STATUS_REJECT_ALREADY_EXISTS;
continue;
}
ref->forced_update = 1;
- } else if (nonfastforward) {
+ } 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;
@@ -1521,7 +1530,8 @@ int ref_newer(const unsigned char *new_sha1, const unsigned char *old_sha1)
struct commit_list *list, *used;
int found = 0;
- /* Both new and old must be commit-ish and new is descendant of
+ /*
+ * Both new and old must be commit-ish and new is descendant of
* old. Otherwise we require --force.
*/
o = deref_tag(parse_object(old_sha1), NULL, 0);
diff --git a/send-pack.c b/send-pack.c
index 1c375f0..97ab336 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -230,6 +230,8 @@ int send_pack(struct send_pack_args *args,
switch (ref->status) {
case REF_STATUS_REJECT_NONFASTFORWARD:
case REF_STATUS_REJECT_ALREADY_EXISTS:
+ case REF_STATUS_REJECT_FETCH_FIRST:
+ case REF_STATUS_REJECT_NEEDS_FORCE:
case REF_STATUS_UPTODATE:
continue;
default:
diff --git a/transport-helper.c b/transport-helper.c
index 965b778..cb3ef7d 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -666,6 +666,16 @@ static void push_update_ref_status(struct strbuf *buf,
free(msg);
msg = NULL;
}
+ else if (!strcmp(msg, "fetch first")) {
+ status = REF_STATUS_REJECT_FETCH_FIRST;
+ free(msg);
+ msg = NULL;
+ }
+ else if (!strcmp(msg, "needs force")) {
+ status = REF_STATUS_REJECT_NEEDS_FORCE;
+ free(msg);
+ msg = NULL;
+ }
}
if (*ref)
diff --git a/transport.c b/transport.c
index 585ebcd..5105562 100644
--- a/transport.c
+++ b/transport.c
@@ -699,6 +699,14 @@ static int print_one_push_status(struct ref *ref, const char *dest, int count, i
print_ref_status('!', "[rejected]", ref, ref->peer_ref,
"already exists", porcelain);
break;
+ case REF_STATUS_REJECT_FETCH_FIRST:
+ print_ref_status('!', "[rejected]", ref, ref->peer_ref,
+ "fetch first", porcelain);
+ break;
+ case REF_STATUS_REJECT_NEEDS_FORCE:
+ print_ref_status('!', "[rejected]", ref, ref->peer_ref,
+ "needs force", porcelain);
+ break;
case REF_STATUS_REMOTE_REJECT:
print_ref_status('!', "[remote rejected]", ref,
ref->deletion ? NULL : ref->peer_ref,
@@ -750,6 +758,10 @@ void transport_print_push_status(const char *dest, struct ref *refs,
*reject_reasons |= REJECT_NON_FF_OTHER;
} else if (ref->status == REF_STATUS_REJECT_ALREADY_EXISTS) {
*reject_reasons |= REJECT_ALREADY_EXISTS;
+ } else if (ref->status == REF_STATUS_REJECT_FETCH_FIRST) {
+ *reject_reasons |= REJECT_FETCH_FIRST;
+ } else if (ref->status == REF_STATUS_REJECT_NEEDS_FORCE) {
+ *reject_reasons |= REJECT_NEEDS_FORCE;
}
}
}
diff --git a/transport.h b/transport.h
index bfd2df5..c818763 100644
--- a/transport.h
+++ b/transport.h
@@ -143,6 +143,8 @@ void transport_set_verbosity(struct transport *transport, int verbosity,
#define REJECT_NON_FF_HEAD 0x01
#define REJECT_NON_FF_OTHER 0x02
#define REJECT_ALREADY_EXISTS 0x04
+#define REJECT_FETCH_FIRST 0x08
+#define REJECT_NEEDS_FORCE 0x10
int transport_push(struct transport *connection,
int refspec_nr, const char **refspec, int flags,
--
1.8.1.1.498.gfdee8be
^ permalink raw reply related
* Re: [PATCH 2/3] push: introduce REJECT_FETCH_FIRST and REJECT_NEEDS_FORCE
From: Junio C Hamano @ 2013-01-22 6:04 UTC (permalink / raw)
To: git; +Cc: Jeff King, Chris Rorvick
In-Reply-To: <1358834027-32039-3-git-send-email-gitster@pobox.com>
This one has a logic flaw. The logic outlined in the cover letter
is correct, and the one described in the log message of this one is
not.
We should say "fetch first" only when we do not have old_sha1.
diff --git a/remote.c b/remote.c
index 248910f..8c39ea2 100644
--- a/remote.c
+++ b/remote.c
@@ -1325,10 +1325,10 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
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))
+ else if (!has_sha1_file(ref->old_sha1))
ref->status = REF_STATUS_REJECT_FETCH_FIRST;
- else if (!lookup_commit_reference_gently(ref->new_sha1, 1))
+ else if (!lookup_commit_reference_gently(ref->new_sha1, 1) ||
+ !lookup_commit_reference_gently(ref->old_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;
^ permalink raw reply related
* Re: [PATCH 3/3] push: further reduce "struct ref" and simplify the logic
From: Junio C Hamano @ 2013-01-22 6:21 UTC (permalink / raw)
To: git; +Cc: Jeff King, Chris Rorvick
In-Reply-To: <1358834027-32039-4-git-send-email-gitster@pobox.com>
Junio C Hamano <gitster@pobox.com> writes:
> 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).
Actually, the latter is a bad idea; it changes the semantics and
mark a push that was done with an unnecessary --force option.
I'm rerolling these three patches (the "update" removal will also be
moved to the preparatory clean-up patch).
^ permalink raw reply
* [PATCH 0/3] Finishing touches to "push" advises
From: Junio C Hamano @ 2013-01-22 6:30 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 series applies on top of 256b9d7 (push: fix "refs/tags/
hierarchy cannot be updated without --force", 2013-01-16).
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 simplify the logic to assign rejection status
advice.c | 4 ++++
advice.h | 2 ++
builtin/push.c | 25 +++++++++++++++++++++++++
builtin/send-pack.c | 10 ++++++++++
cache.h | 6 +++---
remote.c | 42 +++++++++++++++++++-----------------------
send-pack.c | 2 ++
transport-helper.c | 10 ++++++++++
transport.c | 14 +++++++++++++-
transport.h | 2 ++
10 files changed, 90 insertions(+), 27 deletions(-)
--
1.8.1.1.498.gfdee8be
^ permalink raw reply
* [PATCH v2 1/3] push: further clean up fields of "struct ref"
From: Junio C Hamano @ 2013-01-22 6:30 UTC (permalink / raw)
To: git; +Cc: Jeff King, Chris Rorvick
In-Reply-To: <1358836230-9197-1-git-send-email-gitster@pobox.com>
The "nonfastforward" and "update" fields are only used while
deciding what value to assign to the "status" locally in a single
function. Remove them from the "struct ref".
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>
---
* The "update" removal in v1 has been moved to this.
cache.h | 4 +---
remote.c | 16 ++++++----------
transport.c | 2 +-
3 files changed, 8 insertions(+), 14 deletions(-)
diff --git a/cache.h b/cache.h
index a942bbd..12631a1 100644
--- a/cache.h
+++ b/cache.h
@@ -1001,10 +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 {
REF_STATUS_NONE = 0,
diff --git a/remote.c b/remote.c
index d3a1ca2..3375914 100644
--- a/remote.c
+++ b/remote.c
@@ -1317,27 +1317,23 @@ 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) {
- ref->nonfastforward =
+ if (!ref->deletion && !is_null_sha1(ref->old_sha1)) {
+ 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 v2 2/3] push: introduce REJECT_FETCH_FIRST and REJECT_NEEDS_FORCE
From: Junio C Hamano @ 2013-01-22 6:30 UTC (permalink / raw)
To: git; +Cc: Jeff King, Chris Rorvick
In-Reply-To: <1358836230-9197-1-git-send-email-gitster@pobox.com>
When we push to update an existing ref, if:
* we do not have the object at the tip of the remote; or
* 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.
In these cases, the push was locally rejected on the client end, but
we used the same error and advice messages as the ones used when
rejecting a non-fast-forward push, i.e. pull from there and
integrate before pushing again. This did not make much sense.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* Updated log message and fixed the logic to decide "fetch first";
we should say "fetch first" only when we do not have the current
tip of the remote end.
send-pack.c has style violation that "else" is not on the same
line as closing brace of its corresponding "if", but I followed
the existing style of surrounding code. Cleaning them up is for
a separate topic.
advice.c | 4 ++++
advice.h | 2 ++
builtin/push.c | 25 +++++++++++++++++++++++++
builtin/send-pack.c | 10 ++++++++++
cache.h | 2 ++
remote.c | 22 ++++++++++++++++------
send-pack.c | 2 ++
transport-helper.c | 10 ++++++++++
transport.c | 12 ++++++++++++
transport.h | 2 ++
10 files changed, 85 insertions(+), 6 deletions(-)
diff --git a/advice.c b/advice.c
index d287927..780f58d 100644
--- a/advice.c
+++ b/advice.c
@@ -5,6 +5,8 @@ int advice_push_non_ff_current = 1;
int advice_push_non_ff_default = 1;
int advice_push_non_ff_matching = 1;
int advice_push_already_exists = 1;
+int advice_push_fetch_first = 1;
+int advice_push_needs_force = 1;
int advice_status_hints = 1;
int advice_commit_before_merge = 1;
int advice_resolve_conflict = 1;
@@ -20,6 +22,8 @@ static struct {
{ "pushnonffdefault", &advice_push_non_ff_default },
{ "pushnonffmatching", &advice_push_non_ff_matching },
{ "pushalreadyexists", &advice_push_already_exists },
+ { "pushfetchfirst", &advice_push_fetch_first },
+ { "pushneedsforce", &advice_push_needs_force },
{ "statushints", &advice_status_hints },
{ "commitbeforemerge", &advice_commit_before_merge },
{ "resolveconflict", &advice_resolve_conflict },
diff --git a/advice.h b/advice.h
index 8bf6356..fad36df 100644
--- a/advice.h
+++ b/advice.h
@@ -8,6 +8,8 @@ extern int advice_push_non_ff_current;
extern int advice_push_non_ff_default;
extern int advice_push_non_ff_matching;
extern int advice_push_already_exists;
+extern int advice_push_fetch_first;
+extern int advice_push_needs_force;
extern int advice_status_hints;
extern int advice_commit_before_merge;
extern int advice_resolve_conflict;
diff --git a/builtin/push.c b/builtin/push.c
index 8491e43..da928fa 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -224,6 +224,13 @@ static const char message_advice_ref_already_exists[] =
N_("Updates were rejected because the destination reference already exists\n"
"in the remote.");
+static const char message_advice_ref_fetch_first[] =
+ N_("Updates were rejected; you need to fetch the destination reference\n"
+ "to decide what to do.\n");
+
+static const char message_advice_ref_needs_force[] =
+ N_("Updates were rejected; you need to force update.\n");
+
static void advise_pull_before_push(void)
{
if (!advice_push_non_ff_current || !advice_push_update_rejected)
@@ -252,6 +259,20 @@ static void advise_ref_already_exists(void)
advise(_(message_advice_ref_already_exists));
}
+static void advise_ref_fetch_first(void)
+{
+ if (!advice_push_fetch_first || !advice_push_update_rejected)
+ return;
+ advise(_(message_advice_ref_fetch_first));
+}
+
+static void advise_ref_needs_force(void)
+{
+ if (!advice_push_needs_force || !advice_push_update_rejected)
+ return;
+ advise(_(message_advice_ref_needs_force));
+}
+
static int push_with_options(struct transport *transport, int flags)
{
int err;
@@ -285,6 +306,10 @@ static int push_with_options(struct transport *transport, int flags)
advise_checkout_pull_push();
} else if (reject_reasons & REJECT_ALREADY_EXISTS) {
advise_ref_already_exists();
+ } else if (reject_reasons & REJECT_FETCH_FIRST) {
+ advise_ref_fetch_first();
+ } else if (reject_reasons & REJECT_NEEDS_FORCE) {
+ advise_ref_needs_force();
}
return 1;
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index f849e0a..57a46b2 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -44,6 +44,16 @@ static void print_helper_status(struct ref *ref)
msg = "non-fast forward";
break;
+ case REF_STATUS_REJECT_FETCH_FIRST:
+ res = "error";
+ msg = "fetch first";
+ break;
+
+ case REF_STATUS_REJECT_NEEDS_FORCE:
+ res = "error";
+ msg = "needs force";
+ break;
+
case REF_STATUS_REJECT_ALREADY_EXISTS:
res = "error";
msg = "already exists";
diff --git a/cache.h b/cache.h
index 12631a1..377a3df 100644
--- a/cache.h
+++ b/cache.h
@@ -1010,6 +1010,8 @@ struct ref {
REF_STATUS_REJECT_NONFASTFORWARD,
REF_STATUS_REJECT_ALREADY_EXISTS,
REF_STATUS_REJECT_NODELETE,
+ REF_STATUS_REJECT_FETCH_FIRST,
+ REF_STATUS_REJECT_NEEDS_FORCE,
REF_STATUS_UPTODATE,
REF_STATUS_REMOTE_REJECT,
REF_STATUS_EXPECTING_REPORT
diff --git a/remote.c b/remote.c
index 3375914..c991915 100644
--- a/remote.c
+++ b/remote.c
@@ -1318,17 +1318,26 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
*/
if (!ref->deletion && !is_null_sha1(ref->old_sha1)) {
- int nonfastforward =
- !has_sha1_file(ref->old_sha1)
- || !ref_newer(ref->new_sha1, ref->old_sha1);
-
if (!prefixcmp(ref->name, "refs/tags/")) {
if (!force_ref_update) {
ref->status = REF_STATUS_REJECT_ALREADY_EXISTS;
continue;
}
ref->forced_update = 1;
- } else if (nonfastforward) {
+ } else if (!has_sha1_file(ref->old_sha1)) {
+ if (!force_ref_update) {
+ ref->status = REF_STATUS_REJECT_FETCH_FIRST;
+ continue;
+ }
+ ref->forced_update = 1;
+ } else if (!lookup_commit_reference_gently(ref->old_sha1, 1) ||
+ !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;
@@ -1517,7 +1526,8 @@ int ref_newer(const unsigned char *new_sha1, const unsigned char *old_sha1)
struct commit_list *list, *used;
int found = 0;
- /* Both new and old must be commit-ish and new is descendant of
+ /*
+ * Both new and old must be commit-ish and new is descendant of
* old. Otherwise we require --force.
*/
o = deref_tag(parse_object(old_sha1), NULL, 0);
diff --git a/send-pack.c b/send-pack.c
index 1c375f0..97ab336 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -230,6 +230,8 @@ int send_pack(struct send_pack_args *args,
switch (ref->status) {
case REF_STATUS_REJECT_NONFASTFORWARD:
case REF_STATUS_REJECT_ALREADY_EXISTS:
+ case REF_STATUS_REJECT_FETCH_FIRST:
+ case REF_STATUS_REJECT_NEEDS_FORCE:
case REF_STATUS_UPTODATE:
continue;
default:
diff --git a/transport-helper.c b/transport-helper.c
index 965b778..cb3ef7d 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -666,6 +666,16 @@ static void push_update_ref_status(struct strbuf *buf,
free(msg);
msg = NULL;
}
+ else if (!strcmp(msg, "fetch first")) {
+ status = REF_STATUS_REJECT_FETCH_FIRST;
+ free(msg);
+ msg = NULL;
+ }
+ else if (!strcmp(msg, "needs force")) {
+ status = REF_STATUS_REJECT_NEEDS_FORCE;
+ free(msg);
+ msg = NULL;
+ }
}
if (*ref)
diff --git a/transport.c b/transport.c
index 585ebcd..5105562 100644
--- a/transport.c
+++ b/transport.c
@@ -699,6 +699,14 @@ static int print_one_push_status(struct ref *ref, const char *dest, int count, i
print_ref_status('!', "[rejected]", ref, ref->peer_ref,
"already exists", porcelain);
break;
+ case REF_STATUS_REJECT_FETCH_FIRST:
+ print_ref_status('!', "[rejected]", ref, ref->peer_ref,
+ "fetch first", porcelain);
+ break;
+ case REF_STATUS_REJECT_NEEDS_FORCE:
+ print_ref_status('!', "[rejected]", ref, ref->peer_ref,
+ "needs force", porcelain);
+ break;
case REF_STATUS_REMOTE_REJECT:
print_ref_status('!', "[remote rejected]", ref,
ref->deletion ? NULL : ref->peer_ref,
@@ -750,6 +758,10 @@ void transport_print_push_status(const char *dest, struct ref *refs,
*reject_reasons |= REJECT_NON_FF_OTHER;
} else if (ref->status == REF_STATUS_REJECT_ALREADY_EXISTS) {
*reject_reasons |= REJECT_ALREADY_EXISTS;
+ } else if (ref->status == REF_STATUS_REJECT_FETCH_FIRST) {
+ *reject_reasons |= REJECT_FETCH_FIRST;
+ } else if (ref->status == REF_STATUS_REJECT_NEEDS_FORCE) {
+ *reject_reasons |= REJECT_NEEDS_FORCE;
}
}
}
diff --git a/transport.h b/transport.h
index bfd2df5..c818763 100644
--- a/transport.h
+++ b/transport.h
@@ -143,6 +143,8 @@ void transport_set_verbosity(struct transport *transport, int verbosity,
#define REJECT_NON_FF_HEAD 0x01
#define REJECT_NON_FF_OTHER 0x02
#define REJECT_ALREADY_EXISTS 0x04
+#define REJECT_FETCH_FIRST 0x08
+#define REJECT_NEEDS_FORCE 0x10
int transport_push(struct transport *connection,
int refspec_nr, const char **refspec, int flags,
--
1.8.1.1.498.gfdee8be
^ permalink raw reply related
* [PATCH v2 3/3] push: further simplify the logic to assign rejection status
From: Junio C Hamano @ 2013-01-22 6:30 UTC (permalink / raw)
To: git; +Cc: Jeff King, Chris Rorvick
In-Reply-To: <1358836230-9197-1-git-send-email-gitster@pobox.com>
Instead of using deeply nested if/else statements, first decide what
rejection status we would get if this push weren't forced, and then
assign the rejection reason to the ref->status field and flip the
ref->forced_update field when we forced a push for a ref that indeed
required forcing.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* The first one mistakenly changed the semantics and reported a
forced push even when the push was done with useless and
unnecessary --force option (e.g. the update was properly
fast-forwarding but --force was given from the command line).
This fixes it.
remote.c | 40 +++++++++++++++-------------------------
1 file changed, 15 insertions(+), 25 deletions(-)
diff --git a/remote.c b/remote.c
index c991915..af2136d 100644
--- a/remote.c
+++ b/remote.c
@@ -1318,32 +1318,22 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
*/
if (!ref->deletion && !is_null_sha1(ref->old_sha1)) {
- if (!prefixcmp(ref->name, "refs/tags/")) {
- if (!force_ref_update) {
- ref->status = REF_STATUS_REJECT_ALREADY_EXISTS;
- continue;
- }
+ int status = 0;
+
+ if (!prefixcmp(ref->name, "refs/tags/"))
+ status = REF_STATUS_REJECT_ALREADY_EXISTS;
+ else if (!has_sha1_file(ref->old_sha1))
+ status = REF_STATUS_REJECT_FETCH_FIRST;
+ else if (!lookup_commit_reference_gently(ref->old_sha1, 1) ||
+ !lookup_commit_reference_gently(ref->new_sha1, 1))
+ status = REF_STATUS_REJECT_NEEDS_FORCE;
+ else if (!ref_newer(ref->new_sha1, ref->old_sha1))
+ status = REF_STATUS_REJECT_NONFASTFORWARD;
+
+ if (!force_ref_update)
+ ref->status = status;
+ else if (status)
ref->forced_update = 1;
- } else if (!has_sha1_file(ref->old_sha1)) {
- if (!force_ref_update) {
- ref->status = REF_STATUS_REJECT_FETCH_FIRST;
- continue;
- }
- ref->forced_update = 1;
- } else if (!lookup_commit_reference_gently(ref->old_sha1, 1) ||
- !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;
- }
- ref->forced_update = 1;
- }
}
}
}
--
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