* Re: What's cooking in git.git (Aug 2009, #06; Sun, 30)
From: Junio C Hamano @ 2009-09-02 5:32 UTC (permalink / raw)
To: Nick Edelen; +Cc: git
In-Reply-To: <c77435a80909011525m3a6e7917xe066d61f3863e615@mail.gmail.com>
Nick Edelen <sirnot@gmail.com> writes:
> I vaguely remember something concerning those tests when starting the
> project. I'm a bit disconnected from everything right now, but I'll
> try to get those fixed as soon as I can.
Thanks.
^ permalink raw reply
* Re: clong an empty repo over ssh causes (harmless) fatal
From: Sitaram Chamarty @ 2009-09-02 5:30 UTC (permalink / raw)
To: Jeff King; +Cc: Björn Steinbrink, Sverre Rabbelier, Matthieu Moy, git
In-Reply-To: <20090831224749.GA24190@sigill.intra.peff.net>
2009/9/1 Jeff King <peff@peff.net>:
> OK, it is definitely not about mixed versions, and it is definitely
> reproducible, even without ssh. The local clone optimization manages to
> avoid it, but you can see it with:
>
> git clone file://$PWD/a b
ok I hadn't noticed that -- good spot!
Anyway this whole thread went way over my head very quickly so just
wanted to say I appreciate you guys looking at it and fixing it.
Don't know if enough people say that :)
Sitaram
^ permalink raw reply
* Re: [PATCH v2] status: list unmerged files last
From: Jeff King @ 2009-09-02 5:28 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Sixt, bill lam, git
In-Reply-To: <7vljkxdiil.fsf@alter.siamese.dyndns.org>
On Tue, Sep 01, 2009 at 10:26:26PM -0700, Junio C Hamano wrote:
> Here is how I would justify the change (the patch is the same as Hannes's
> first version.
Makes sense to me.
Acked-by: Jeff King <peff@peff.net>
> the optimum layout..
Double period. :)
-Peff
^ permalink raw reply
* Re: [PATCH v2] status: list unmerged files last
From: Junio C Hamano @ 2009-09-02 5:26 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Sixt, bill lam, git
In-Reply-To: <20090902051248.GB12046@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Tue, Sep 01, 2009 at 09:26:26PM -0700, Junio C Hamano wrote:
>
>> But not everybody is used to such a set-up. If you rely on terminal's
>> scrollback buffer with mouse and a short terminal, I can see cutting and
>> pasting would be an issue. I do not have a good answer to "tig status",
>> but the design principle of supporting the lowest denominator is
>> important.
>
> But I'm not sure it is about "lowest common denominator". I think it is
> about different people having different preferences (as a matter of
> fact, I use an 80x25 terminal most of the time, and I think I prefer the
> content at the top. Perhaps it is simply habit, but I do think having it
> right next to "staged for commit" items makes the most sense).
> ...
Here is how I would justify the change (the patch is the same as Hannes's
first version.
From: Johannes Sixt <j6t@kdbg.org>
Date: Tue, 1 Sep 2009 22:13:53 +0200
Subject: [PATCH] status: list unmerged files much later
When resolving a conflicted merge, two lists in the status output need
more attention from the user than other parts.
- the list of updated paths is useful to review the amount of changes the
merge brings in (the user cannot do much about them other than
reviewing, though); and
- the list of unmerged paths needs the most attention from the user; the
user needs to resolve them in order to proceed.
Since the output of git status does not by default go through the pager,
the early parts of the output can scroll away at the top. It is better to
put the more important information near the bottom. During a merge, local
changes that are not in the index are minimum, and you should keep the
untracked list small in any case, so moving the unmerged list from the top
of the output to immediately after the list of updated paths would give us
the optimum layout..
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
wt-status.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
^ permalink raw reply
* Re: clong an empty repo over ssh causes (harmless) fatal
From: Jeff King @ 2009-09-02 5:16 UTC (permalink / raw)
To: Daniel Barkalow
Cc: Sverre Rabbelier, Björn Steinbrink, Matthieu Moy,
Sitaram Chamarty, git
In-Reply-To: <alpine.LNX.2.00.0909020024270.28290@iabervon.org>
On Wed, Sep 02, 2009 at 12:33:52AM -0400, Daniel Barkalow wrote:
> > The patch below seems to work for me, but I'm a little concerned how it
> > might impact other transports.
>
> Does putting a "transport_disconnect(transport);" after the
> "transport_unlock_pack(transport);" in builtin-clone.c also work for you?
> I think that's a cleaner solution, and should future-proof it in case we
> have a future transport that both doesn't disconnect itself after a fetch
> and gives an error message if the connection is dropped suddenly.
>
> It's kind of just an accident that the only transport that cares about
> disconnect very much doesn't care if you've fetched after getting the
> refs.
It does work, and I think that is a much saner solution for the reasons
you mention. Thanks. Do you want to write it up and submit it, or should
I?
-Peff
^ permalink raw reply
* Re: [PATCH v2] status: list unmerged files last
From: Jeff King @ 2009-09-02 5:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Sixt, bill lam, git
In-Reply-To: <7vmy5egefh.fsf@alter.siamese.dyndns.org>
On Tue, Sep 01, 2009 at 09:26:26PM -0700, Junio C Hamano wrote:
> But not everybody is used to such a set-up. If you rely on terminal's
> scrollback buffer with mouse and a short terminal, I can see cutting and
> pasting would be an issue. I do not have a good answer to "tig status",
> but the design principle of supporting the lowest denominator is
> important.
But I'm not sure it is about "lowest common denominator". I think it is
about different people having different preferences (as a matter of
fact, I use an 80x25 terminal most of the time, and I think I prefer the
content at the top. Perhaps it is simply habit, but I do think having it
right next to "staged for commit" items makes the most sense).
> The above suggests me that (1) we would want to have the new "unmerged"
> section next to "updated" section, (2) we would want to have it later in
> the output rather than earlier, and (3) in the traditional output, people
> are used to see unmerged paths in "changed" section, so it would be easier
> for them to transition if "unmerged" section were near "changed" section.
>
> That makes the ideal place between updated and changed, no?
Yes, I think that is fine, and makes more sense than where we have it
now. I mainly wanted to argue against sticking it at the very bottom.
> [1] It might even make sense to omit other sections and show only
> "updated" and "unmerged" in this order when the index is unmerged, but
> that is a lot more drastic change for 1.7.0.
I think that is a really bad idea. The mental model of "git status"
(versus individual diff or ls-files commands) is to see _everything_
going on in the repo. Showing a subset breaks that model and gives a
false sense of what is actually happening.
I don't know that it would matter much most of the time anyway. If you
have unmerged entries, you probably don't have any (or many) "changed
but not updated" files, too (since you are not working on a new commit
but rather a merge, they would have to be dirty state you are carrying
permanently, but not related to the merge). If you do, you probably want
to see them to be aware of what is going on.
You probably also don't have a lot of untracked files. If you have a
few, you might want to be reminded of them to make sure they were not
something you were preparing to help with a tricky merge. And if you are
the sort of person who carries around a lot of untracked files, and for
some reason you refuse to put them in your .gitignore, then you probably
have status.untracked set to "no" already (or you should consider
setting it), as they will be bugging you in other situations, as well.
-Peff
^ permalink raw reply
* Re: stash --dwim safety
From: Jeff King @ 2009-09-02 4:59 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin, Matthieu Moy, git
In-Reply-To: <7vy6oyj892.fsf@alter.siamese.dyndns.org>
[cc'ing Dscho, as he was the main opponent of similar proposals, and I
suspect his silence here means he missed this discussion. I hope this
addresses his concerns, but I think it is good to get comment from all
interested parties.
I'll just quote as appropriate below to comment, but for the whole patch
see:
http://article.gmane.org/gmane.comp.version-control.git/127574
]
On Tue, Sep 01, 2009 at 09:11:37PM -0700, Junio C Hamano wrote:
> This makes the logic of defaulting to "save" much simpler. If there is no
> non-flag arguments, it is clear that there is no command word, and we
s/is/are/ (or s/arguments/argument/)
> --- a/Documentation/git-stash.txt
> +++ b/Documentation/git-stash.txt
> [...]
> - --hard` to revert them. This is the default action when no
> - subcommand is given. The <message> part is optional and gives
> - the description along with the stashed state.
> + --hard` to revert them. The <message> part is optional and gives
> + the description along with the stashed state. For quickly making
> + a snapshot, you can omit _both_ "save" and <message>, but giving
> + only <message> does not trigger this action to prevent misspelled
> + subcommand from making an unwanted stash.
s/misspelled/a &/
-Peff
^ permalink raw reply
* Re: clong an empty repo over ssh causes (harmless) fatal
From: Daniel Barkalow @ 2009-09-02 4:33 UTC (permalink / raw)
To: Jeff King
Cc: Sverre Rabbelier, Björn Steinbrink, Matthieu Moy,
Sitaram Chamarty, git
In-Reply-To: <20090901010833.GA4033@sigill.intra.peff.net>
On Mon, 31 Aug 2009, Jeff King wrote:
> On Tue, Sep 01, 2009 at 12:50:25AM +0200, Sverre Rabbelier wrote:
>
> > 2009/9/1 Jeff King <peff@peff.net>:
> > > AFAICT, this problem goes back to v1.6.2, the first version which
> > > handled empty clones. So I blame Sverre. ;)
> >
> > Eep :(. Any idea what is going on?
>
> Yeah. We call upload-pack on the remote side, realize there are no refs,
> and then we just stop talking. Meanwhile upload-pack is waiting for a
> packet to say "these are the refs that I want". So the client really
> needs to send an extra packet saying "list of refs is finished".
>
> The patch below seems to work for me, but I'm a little concerned how it
> might impact other transports.
Does putting a "transport_disconnect(transport);" after the
"transport_unlock_pack(transport);" in builtin-clone.c also work for you?
I think that's a cleaner solution, and should future-proof it in case we
have a future transport that both doesn't disconnect itself after a fetch
and gives an error message if the connection is dropped suddenly.
It's kind of just an accident that the only transport that cares about
disconnect very much doesn't care if you've fetched after getting the
refs.
-Daniel
*This .sig left intentionally blank*
^ permalink raw reply
* Re: [PATCH v2] status: list unmerged files last
From: Junio C Hamano @ 2009-09-02 4:26 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Sixt, bill lam, git
In-Reply-To: <20090902011513.GA3874@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> I think "related things together" trumps "close to the bottom". Because
> the former is something that _always_ applies to your output, while the
> latter is catering to a particular use case and a particular screen
> setup.
>
> In other words, why is the _bottom_ reserved for more important things
> instead of the _top_? If I have a tall terminal that is long enough to
> see the output, are you potentially making the important thing less
> obvious (because I tend to read the the output from top to bottom)? If I
> use a pager (either manually, because I have seen that the output is too
> long, or automatically via the pager.status config variable)? What about
> reading status output into an interface wrapper like "tig status"?
Yes and no.
Sure, I always work in a 92x70 screen session with 10k lines of scrollback
buffer, and when I cut and paste I do not use a mouse but use screen's cut
buffer, so I would have no problem with the list at the top.
Not that I would use "git status" while resolving merges---I would use
"ls-files -u" myself, and I may perhaps start using "status -suno", so my
personal preference does not really count on this topic.
But not everybody is used to such a set-up. If you rely on terminal's
scrollback buffer with mouse and a short terminal, I can see cutting and
pasting would be an issue. I do not have a good answer to "tig status",
but the design principle of supporting the lowest denominator is
important.
J6t made a good point that this new section won't appear when committing,
which I didn't take account when I was first explained how the ordering
was chosen. After thinking about this a bit more, I think "untracked" and
"modified but not updated" sections, unlike when recording your own
commit, is mostly uninteresting while resolving a merge. You never add
files that you forgot to add to a merge; nor you would add your local
modifications to a merge. So the only sections that are interesting are
this new "unmerged" section and "updated" section to see the extent of
damage the merge causes to your history by introducing the crap other
people dumped on you ;-) [*1*].
The above suggests me that (1) we would want to have the new "unmerged"
section next to "updated" section, (2) we would want to have it later in
the output rather than earlier, and (3) in the traditional output, people
are used to see unmerged paths in "changed" section, so it would be easier
for them to transition if "unmerged" section were near "changed" section.
That makes the ideal place between updated and changed, no?
Incidentally that is where J6t's first patch was. So I would agree with
the patch (but not necessarily with its justification).
[1] It might even make sense to omit other sections and show only
"updated" and "unmerged" in this order when the index is unmerged, but
that is a lot more drastic change for 1.7.0.
^ permalink raw reply
* Re: stash --dwim safety
From: Junio C Hamano @ 2009-09-02 4:11 UTC (permalink / raw)
To: Matthieu Moy, Jeff King; +Cc: git
In-Reply-To: <20090901065716.GA5575@sigill.intra.peff.net>
From: Matthieu Moy <Matthieu.Moy@imag.fr>
Date: Tue, 18 Aug 2009 23:38:40 +0200
Subject: [PATCH] stash: simplify defaulting to "save" and reject unknown options
With the earlier DWIM patches, certain combination of options defaulted
to the "save" command correctly while certain equally valid combination
did not. For example, "git stash -k" were Ok but "git stash -q -k" did
not work.
This makes the logic of defaulting to "save" much simpler. If there is no
non-flag arguments, it is clear that there is no command word, and we
default to "save" subcommand. This rule prevents "git stash -q apply"
from quietly creating a stash with "apply" as the message.
This also teaches "git stash save" to reject an unknown option. This is
to keep a mistyped "git stash save --quite" from creating a stash with a
message "--quite", and this safety is more important with the new logic
to default to "save" with any option-looking argument without an explicit
comand word.
[jc: this is based on Matthieu's 3-patch series, and a follow-up
discussion, and he and Peff take all the credit; if I have introduced bugs
while reworking, they are mine.]
Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Jeff King <peff@peff.net> writes:
> On Tue, Sep 01, 2009 at 08:27:20AM +0200, Matthieu Moy wrote:
>
>> I was actually thinking of being a little more paranoid to prevent
>> accidental "stash save": we could refuse to create a named stash when
>> the "save" command is not given. The case I hadn't thought of was "git
>> stash -q apply", which has 99% chances of being a typo for "git stash
>> apply -q", and which would mean "create a stash named apply, quietly".
>
> I like that. I think it addresses Dscho's concern with mistakes causing
> an unexpected stash, and it is actually more consistent with the current
> rule (that named stashes need an explicit 'save'). IOW, it is actually a
> bit confusing that "git stash foo" doesn't work, but "git stash -k foo"
> does.
Ok, then here comes the final proposal.
Documentation/git-stash.txt | 9 +++++----
git-stash.sh | 27 +++++++++++++++++++++++----
t/t3903-stash.sh | 11 +++++++++++
3 files changed, 39 insertions(+), 8 deletions(-)
diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 1c4ed41..885bc97 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -14,7 +14,6 @@ SYNOPSIS
'git stash' ( pop | apply ) [--index] [-q|--quiet] [<stash>]
'git stash' branch <branchname> [<stash>]
'git stash' [save [--patch] [-k|--[no-]keep-index] [-q|--quiet] [<message>]]
-'git stash' [-p|--patch|-k|--keep-index]
'git stash' clear
'git stash' create
@@ -46,9 +45,11 @@ OPTIONS
save [--patch] [--[no-]keep-index] [-q|--quiet] [<message>]::
Save your local modifications to a new 'stash', and run `git reset
- --hard` to revert them. This is the default action when no
- subcommand is given. The <message> part is optional and gives
- the description along with the stashed state.
+ --hard` to revert them. The <message> part is optional and gives
+ the description along with the stashed state. For quickly making
+ a snapshot, you can omit _both_ "save" and <message>, but giving
+ only <message> does not trigger this action to prevent misspelled
+ subcommand from making an unwanted stash.
+
If the `--keep-index` option is used, all changes already added to the
index are left intact.
diff --git a/git-stash.sh b/git-stash.sh
index 9fd7289..f243376 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -8,7 +8,6 @@ USAGE="list [<options>]
or: $dashless ( pop | apply ) [--index] [-q|--quiet] [<stash>]
or: $dashless branch <branchname> [<stash>]
or: $dashless [save [-k|--keep-index] [-q|--quiet] [<message>]]
- or: $dashless [-k|--keep-index]
or: $dashless clear"
SUBDIRECTORY_OK=Yes
@@ -146,6 +145,14 @@ save_stash () {
-q|--quiet)
GIT_QUIET=t
;;
+ --)
+ shift
+ break
+ ;;
+ -*)
+ echo "error: unknown option for 'stash save': $1"
+ usage
+ ;;
*)
break
;;
@@ -355,6 +362,18 @@ apply_to_branch () {
drop_stash $stash
}
+# The default command is "save" if nothing but options are given
+seen_non_option=
+for opt
+do
+ case "$opt" in
+ -*) ;;
+ *) seen_non_option=t; break ;;
+ esac
+done
+
+test -n "$seen_non_option" || set "save" "$@"
+
# Main command set
case "$1" in
list)
@@ -406,9 +425,9 @@ branch)
apply_to_branch "$@"
;;
*)
- case $#,"$1","$2" in
- 0,,|1,-k,|1,--keep-index,|1,-p,|1,--patch,|2,-p,--no-keep-index|2,--patch,--no-keep-index)
- save_stash "$@" &&
+ case $# in
+ 0)
+ save_stash &&
say '(To restore them type "git stash apply")'
;;
*)
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index e16ad93..5514f74 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -208,4 +208,15 @@ test_expect_success 'stash -k' '
test bar,bar4 = $(cat file),$(cat file2)
'
+test_expect_success 'stash --invalid-option' '
+ echo bar5 > file &&
+ echo bar6 > file2 &&
+ git add file2 &&
+ test_must_fail git stash --invalid-option &&
+ test_must_fail git stash save --invalid-option &&
+ test bar5,bar6 = $(cat file),$(cat file2) &&
+ git stash -- -message-starting-with-dash &&
+ test bar,bar2 = $(cat file),$(cat file2)
+'
+
test_done
--
1.6.4.2.301.g12b4ad
^ permalink raw reply related
* Re: [PATCH v2] status: list unmerged files last
From: Jeff King @ 2009-09-02 1:15 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Sixt, bill lam, git
In-Reply-To: <7vtyzmxkpr.fsf@alter.siamese.dyndns.org>
On Tue, Sep 01, 2009 at 05:18:40PM -0700, Junio C Hamano wrote:
> The "keeping related things together" argument does mean your v1 is better
> than this patch, as you had "unmerged" next to "changed but not updated".
> I personally think the "keep related things together" argument makes much
> more sense than the "close to the bottom is easier to cut and paste"
> argument, as I tend to focus at the top of the output when looking at the
> status output and almost never cut & paste using mouse (screen for
> rectangular cutting and pasting works wonderfully), but it probably is
> just me. And remember that I am only just one of the users, nothing more.
>
> Sadly, "keep related things together" and "as close to the bottom as
> possible" are not quite compatible, and we can pick one or the other, but
> not both.
Just my two cents (and I think I have as good a track record at UI
design as Junio... ;) ):
I think "related things together" trumps "close to the bottom". Because
the former is something that _always_ applies to your output, while the
latter is catering to a particular use case and a particular screen
setup.
In other words, why is the _bottom_ reserved for more important things
instead of the _top_? If I have a tall terminal that is long enough to
see the output, are you potentially making the important thing less
obvious (because I tend to read the the output from top to bottom)? If I
use a pager (either manually, because I have seen that the output is too
long, or automatically via the pager.status config variable)? What about
reading status output into an interface wrapper like "tig status"?
So while you may be helping some users, I tend to think you may be
hurting others.
-Peff
PS I am also not entirely convinced that unmerged entries are somehow
more important to call attention to in the list than other entries. But
the above argues that even _if_ you think they are more important, it is
still not necessarily a good thing to move them to the bottom.
^ permalink raw reply
* Re: Troubles building man pages
From: Brian S. Schang @ 2009-09-02 0:52 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7v7hwjidt2.fsf@alter.siamese.dyndns.org>
Junio (et al):
Junio C Hamano wrote:
> "Brian S. Schang" <git@lists.schang.net> writes:
>
>>> ASCIIDOC git-am.xml
>>> XMLTO git-am.1
>>> I/O error : Attempt to load network entity http://www.oasis-open.org/docbook/xml/4.2/docbookx.dtd
>>> /raida/packages/git/Documentation/git-am.xml:2: warning: failed to load external entity "http://www.oasis-open.org/docbook/xml/4.2/docbookx.dtd"
>>> D DocBook XML V4.2//EN" "http://www.oasis-open.org/docbook/xml/4.2/docbookx.dtd"
> The following was an ancient experience of mine with a different distro
> but I am reasonably sure it would point you in the right direction.
>
> http://article.gmane.org/gmane.comp.version-control.git/107387
Thank you! The link you sent suggested a reinstall of DocBook. I tried
that and magically everything started to work fine.
I appreciate the help.
--
Brian Schang
^ permalink raw reply
* Re: [PATCH v2] status: list unmerged files last
From: bill lam @ 2009-09-02 0:39 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Sixt, bill lam, git
In-Reply-To: <7vtyzmxkpr.fsf@alter.siamese.dyndns.org>
On Tue, 01 Sep 2009, Junio C Hamano wrote:
> Sadly, "keep related things together" and "as close to the bottom as
> possible" are not quite compatible, and we can pick one or the other, but
> not both.
>
> If I were to pick the middle ground, I would probably move it immediately
> after the call to wt_status_print_changed(), with "keeping related things
> together" as the primary justification. It would be an incidental benefit
> that it moves the part slightly closer to the bottom and gives it a better
> chance of staying on the screen.
I can only speak of my personal experience that during rebase -i,
there is no (or very few) untracked files in the list so that the
sequence "modified, unmerged, untracked" is also a good alternative.
(I hope the mail-followup-to is correct this time)
--
regards,
====================================================
GPG key 1024D/4434BAB3 2008-08-24
gpg --keyserver subkeys.pgp.net --recv-keys 4434BAB3
^ permalink raw reply
* Re: [PATCH v2] status: list unmerged files last
From: Junio C Hamano @ 2009-09-02 0:18 UTC (permalink / raw)
To: Johannes Sixt; +Cc: bill lam, git
In-Reply-To: <200909012325.45739.j6t@kdbg.org>
Johannes Sixt <j6t@kdbg.org> writes:
> The list of unmerged files is considered rather important because after
> a conflicted merge they need attention. Since the output of git status does
> not go through the pager, the end of the output remains immediately visible
> in the terminal window. By placing unmerge entries at the end of the list,
> the user can see them immediately.
>
> Moreover, keeping the unmerge entries at the top is inconvenient if a merge
> touched many files, but only a few conflicted: After the conflicts were
> resolved, the user will conduct a 'git add' command. In order to do that
> with copy-and-paste, the user must scroll the terminal window up, and must
> do so for each individual entry (because terminal windows commonly scroll
> down automatically on the paste operation to make the cursor visible).
>
> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> On Dienstag, 1. September 2009, Junio C Hamano wrote:
>
>> I actually was expecting that you would move this at the very bottom after
>> untracked list for the above reason, and also because this part is only
>> shown while running status (that was a good point you made in the previous
>> message) and never in commit.
>
> So you would not mind a more "drastic" change?
Well, it's not really about what _I_ like or mind. It is primarily about
what the list collectively thinks. I'd like to let other eyeballs and
brains to weigh in, as I am known to pick the worst layout from the UI
point of view as you saw in this thread already ;-).
> (Originally I didn't dare to change too much and thought keeping staged
> files together would make sense.)
Yes, unmerged ones are modified and the index knows about them, but you
haven't told git what you want to commit yet, so they are in the same
category as "changed but not updated" in that sense, but unlike "changed
but not updated", you cannot leave them as they are before proceeding, so
they are worse.
The "keeping related things together" argument does mean your v1 is better
than this patch, as you had "unmerged" next to "changed but not updated".
I personally think the "keep related things together" argument makes much
more sense than the "close to the bottom is easier to cut and paste"
argument, as I tend to focus at the top of the output when looking at the
status output and almost never cut & paste using mouse (screen for
rectangular cutting and pasting works wonderfully), but it probably is
just me. And remember that I am only just one of the users, nothing more.
Sadly, "keep related things together" and "as close to the bottom as
possible" are not quite compatible, and we can pick one or the other, but
not both.
If I were to pick the middle ground, I would probably move it immediately
after the call to wt_status_print_changed(), with "keeping related things
together" as the primary justification. It would be an incidental benefit
that it moves the part slightly closer to the bottom and gives it a better
chance of staying on the screen.
But I am not a great UI designer ;-)
^ permalink raw reply
* Re: [JGIT PATCH (RESEND) 1/3] Allow RefUpdate.setExpectedOldObjectId to accept RevCommit
From: Shawn O. Pearce @ 2009-09-01 23:18 UTC (permalink / raw)
To: Robin Rosenberg; +Cc: git
In-Reply-To: <1251847010-9992-1-git-send-email-spearce@spearce.org>
"Shawn O. Pearce" <spearce@spearce.org> wrote:
> Subject: Re: [JGIT PATCH (RESEND) 1/3] Allow RefUpdate.setExpectedOldObjectId to accept RevCommit
Sorry, this is not a resend, its the first time I've sent it...
--
Shawn.
^ permalink raw reply
* Re: Problems with GIT under Windows - "not uptodate"
From: Johannes Schindelin @ 2009-09-01 23:19 UTC (permalink / raw)
To: Eric Raible; +Cc: git, david.hagood
In-Reply-To: <279b37b20909011616x60fc7bfav22daca1bc7bfc714@mail.gmail.com>
[-- Attachment #1: Type: TEXT/PLAIN, Size: 560 bytes --]
Hi,
On Tue, 1 Sep 2009, Eric Raible wrote:
> On Tue, Sep 1, 2009 at 3:56 PM, Johannes
> Schindelin<Johannes.Schindelin@gmx.de> wrote:
>
> > Eric, is there any good reason you neglect netiquette? I re-added
> > David to the Cc: list.
>
> Thanks. Frankly I (stupidly) assumed that gmane.org would handle it.
> Educate me (if you would): if I read the git list via gmane, what's the
> best way to follow up?
No idea. I read the mails in a normal mail program. You'll have to find
out yourself how to behave nicely with GMane's interface.
Ciao,
Dscho
^ permalink raw reply
* [JGIT PATCH (RESEND) 3/3] Fix DirCache.findEntry to work on an empty cache
From: Shawn O. Pearce @ 2009-09-01 23:16 UTC (permalink / raw)
To: Robin Rosenberg; +Cc: git, Shawn O. Pearce
In-Reply-To: <1251847010-9992-2-git-send-email-spearce@spearce.org>
If the cache has no entries, we want to return -1 rather than throw
ArrayIndexOutOfBoundsException. This binary search loop was stolen
from some other code which contained a test before the loop to see if
the collection was empty or not, but we failed to include that here.
Flipping the loop around to a standard while loop ensures we test
the condition properly first.
Signed-off-by: Shawn O. Pearce <sop@google.com>
Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
.../spearce/jgit/dircache/DirCacheBasicTest.java | 6 ++++++
.../src/org/spearce/jgit/dircache/DirCache.java | 6 ++----
2 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheBasicTest.java b/org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheBasicTest.java
index b3097ac..4d737c0 100644
--- a/org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheBasicTest.java
+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheBasicTest.java
@@ -39,6 +39,7 @@
import java.io.File;
+import org.spearce.jgit.lib.Constants;
import org.spearce.jgit.lib.RepositoryTestCase;
public class DirCacheBasicTest extends RepositoryTestCase {
@@ -182,4 +183,9 @@ public void testBuildThenClear() throws Exception {
assertEquals(0, dc.getEntryCount());
}
+ public void testFindOnEmpty() throws Exception {
+ final DirCache dc = DirCache.newInCore();
+ final byte[] path = Constants.encode("a");
+ assertEquals(-1, dc.findEntry(path, path.length));
+ }
}
diff --git a/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCache.java b/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCache.java
index bfb7925..9f0810a 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCache.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCache.java
@@ -583,8 +583,6 @@ public void unlock() {
* information. If < 0 the entry does not exist in the index.
*/
public int findEntry(final String path) {
- if (entryCnt == 0)
- return -1;
final byte[] p = Constants.encode(path);
return findEntry(p, p.length);
}
@@ -592,7 +590,7 @@ public int findEntry(final String path) {
int findEntry(final byte[] p, final int pLen) {
int low = 0;
int high = entryCnt;
- do {
+ while (low < high) {
int mid = (low + high) >>> 1;
final int cmp = cmp(p, pLen, sortedEntries[mid]);
if (cmp < 0)
@@ -603,7 +601,7 @@ else if (cmp == 0) {
return mid;
} else
low = mid + 1;
- } while (low < high);
+ }
return -(low + 1);
}
--
1.6.4.1.341.gf2a44
^ permalink raw reply related
* [JGIT PATCH (RESEND) 2/3] Work around Sun javac compiler error in RefUpdate
From: Shawn O. Pearce @ 2009-09-01 23:16 UTC (permalink / raw)
To: Robin Rosenberg; +Cc: git, Shawn O. Pearce
In-Reply-To: <1251847010-9992-1-git-send-email-spearce@spearce.org>
Sun's javac, version 5 and 6, apparently miscompiles the for loop
which is looking for a conflicting ref name in the existing set of
refs for this repository.
Debugging this code showed the control flow to return LOCK_FAILURE
when startsWith returned false, which is highly illogical and the
exact opposite of what we have written here.
Sun's javap tool was unable to disassemble the compiled method.
Instead it simply failed to produce anything about updateImpl.
So my remark about the code being compiled wrong is only a guess
based on how I observed the behavior, and not by actually studying
the resulting instructions.
Eclipse's JDT appears to have compiled the updateImpl method
correctly, and produces a working executable. But this is a
much less common compiler to build Java libraries with.
This refactoring to extract the name conflicting test out into
its own method appears to work around the Sun javac bug, and the
resulting class works correctly with either compiler. The code is
also more clear, so its a gain either way.
Signed-off-by: Shawn O. Pearce <sop@google.com>
Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
.../src/org/spearce/jgit/lib/RefUpdate.java | 26 +++++++++++++-------
1 files changed, 17 insertions(+), 9 deletions(-)
diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/RefUpdate.java b/org.spearce.jgit/src/org/spearce/jgit/lib/RefUpdate.java
index 8dffed2..8226e10 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/lib/RefUpdate.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/RefUpdate.java
@@ -449,15 +449,8 @@ private Result updateImpl(final RevWalk walk, final Store store)
RevObject newObj;
RevObject oldObj;
- int lastSlash = getName().lastIndexOf('/');
- if (lastSlash > 0)
- if (db.getRepository().getRef(getName().substring(0, lastSlash)) != null)
- return Result.LOCK_FAILURE;
- String rName = getName() + "/";
- for (Ref r : db.getAllRefs().values()) {
- if (r.getName().startsWith(rName))
- return Result.LOCK_FAILURE;
- }
+ if (isNameConflicting())
+ return Result.LOCK_FAILURE;
lock = new LockFile(looseFile);
if (!lock.lock())
return Result.LOCK_FAILURE;
@@ -490,6 +483,21 @@ private Result updateImpl(final RevWalk walk, final Store store)
}
}
+ private boolean isNameConflicting() throws IOException {
+ final String myName = getName();
+ final int lastSlash = myName.lastIndexOf('/');
+ if (lastSlash > 0)
+ if (db.getRepository().getRef(myName.substring(0, lastSlash)) != null)
+ return true;
+
+ final String rName = myName + "/";
+ for (Ref r : db.getAllRefs().values()) {
+ if (r.getName().startsWith(rName))
+ return true;
+ }
+ return false;
+ }
+
private static RevObject safeParse(final RevWalk rw, final AnyObjectId id)
throws IOException {
try {
--
1.6.4.1.341.gf2a44
^ permalink raw reply related
* [JGIT PATCH (RESEND) 1/3] Allow RefUpdate.setExpectedOldObjectId to accept RevCommit
From: Shawn O. Pearce @ 2009-09-01 23:16 UTC (permalink / raw)
To: Robin Rosenberg; +Cc: git, Shawn O. Pearce
RevCommit overrides .equals() such that it only implements a
reference equality test. If the expected old ObjectId was set
by the application to a RevCommit instance, it would always fail,
resulting in LOCK_FAILURE. Instead use AnyObject.equals() to compare
the value, ignoring the possibly overloaded equals in RevCommit.
Signed-off-by: Shawn O. Pearce <sop@google.com>
Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
.../tst/org/spearce/jgit/lib/RefUpdateTest.java | 52 ++++++++++++++++++++
.../src/org/spearce/jgit/lib/RefUpdate.java | 2 +-
2 files changed, 53 insertions(+), 1 deletions(-)
diff --git a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RefUpdateTest.java b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RefUpdateTest.java
index 800c0a4..a8ccf43 100644
--- a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RefUpdateTest.java
+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RefUpdateTest.java
@@ -45,6 +45,7 @@
import org.spearce.jgit.lib.RefUpdate.Result;
import org.spearce.jgit.revwalk.RevCommit;
+import org.spearce.jgit.revwalk.RevWalk;
public class RefUpdateTest extends RepositoryTestCase {
@@ -397,6 +398,57 @@ public void testUpdateRefLockFailureWrongOldValue() throws IOException {
}
/**
+ * Try modify a ref forward, fast forward, checking old value first
+ *
+ * @throws IOException
+ */
+ public void testUpdateRefForwardWithCheck1() throws IOException {
+ ObjectId ppid = db.resolve("refs/heads/master^");
+ ObjectId pid = db.resolve("refs/heads/master");
+
+ RefUpdate updateRef = db.updateRef("refs/heads/master");
+ updateRef.setNewObjectId(ppid);
+ updateRef.setForceUpdate(true);
+ Result update = updateRef.update();
+ assertEquals(Result.FORCED, update);
+ assertEquals(ppid, db.resolve("refs/heads/master"));
+
+ // real test
+ RefUpdate updateRef2 = db.updateRef("refs/heads/master");
+ updateRef2.setExpectedOldObjectId(ppid);
+ updateRef2.setNewObjectId(pid);
+ Result update2 = updateRef2.update();
+ assertEquals(Result.FAST_FORWARD, update2);
+ assertEquals(pid, db.resolve("refs/heads/master"));
+ }
+
+ /**
+ * Try modify a ref forward, fast forward, checking old commit first
+ *
+ * @throws IOException
+ */
+ public void testUpdateRefForwardWithCheck2() throws IOException {
+ ObjectId ppid = db.resolve("refs/heads/master^");
+ ObjectId pid = db.resolve("refs/heads/master");
+
+ RefUpdate updateRef = db.updateRef("refs/heads/master");
+ updateRef.setNewObjectId(ppid);
+ updateRef.setForceUpdate(true);
+ Result update = updateRef.update();
+ assertEquals(Result.FORCED, update);
+ assertEquals(ppid, db.resolve("refs/heads/master"));
+
+ // real test
+ RevCommit old = new RevWalk(db).parseCommit(ppid);
+ RefUpdate updateRef2 = db.updateRef("refs/heads/master");
+ updateRef2.setExpectedOldObjectId(old);
+ updateRef2.setNewObjectId(pid);
+ Result update2 = updateRef2.update();
+ assertEquals(Result.FAST_FORWARD, update2);
+ assertEquals(pid, db.resolve("refs/heads/master"));
+ }
+
+ /**
* Try modify a ref that is locked
*
* @throws IOException
diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/RefUpdate.java b/org.spearce.jgit/src/org/spearce/jgit/lib/RefUpdate.java
index 69399ec..8dffed2 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/lib/RefUpdate.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/RefUpdate.java
@@ -466,7 +466,7 @@ private Result updateImpl(final RevWalk walk, final Store store)
if (expValue != null) {
final ObjectId o;
o = oldValue != null ? oldValue : ObjectId.zeroId();
- if (!expValue.equals(o))
+ if (!AnyObjectId.equals(expValue, o))
return Result.LOCK_FAILURE;
}
if (oldValue == null)
--
1.6.4.1.341.gf2a44
^ permalink raw reply related
* Re: Problems with GIT under Windows - "not uptodate"
From: Eric Raible @ 2009-09-01 23:16 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, david.hagood
In-Reply-To: <alpine.DEB.1.00.0909020052550.8306@pacific.mpi-cbg.de>
On Tue, Sep 1, 2009 at 3:56 PM, Johannes
Schindelin<Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
> Eric, is there any good reason you neglect netiquette? I re-added David
> to the Cc: list.
Thanks. Frankly I (stupidly) assumed that gmane.org would handle it.
Educate me (if you would): if I read the git list via gmane, what's the
best way to follow up?
^ permalink raw reply
* Re: [JGIT PATCH] fixed error in whitespace handling of RefDatabase#readLine
From: Shawn O. Pearce @ 2009-09-01 23:15 UTC (permalink / raw)
To: Mark Struberg; +Cc: robin.rosenberg, git
In-Reply-To: <196796.47610.qm@web27808.mail.ukl.yahoo.com>
Mark Struberg <struberg@yahoo.de> wrote:
> jgit fails with "cannot checkout; no HEAD advertised by remote"
> in guessHEAD on some repositories.
...
> @@ -500,8 +500,12 @@ private static String readLine(final File file)
> int n = buf.length;
> if (n == 0)
Whitespace damage here, the patch won't apply as-is.
Also, please line wrap your commit message.
--
Shawn.
^ permalink raw reply
* Re: Problems with GIT under Windows - "not uptodate"
From: Johannes Schindelin @ 2009-09-01 22:56 UTC (permalink / raw)
To: Eric Raible; +Cc: git, david.hagood
In-Reply-To: <loom.20090901T184650-434@post.gmane.org>
Hi,
Eric, is there any good reason you neglect netiquette? I re-added David
to the Cc: list.
On Tue, 1 Sep 2009, Eric Raible wrote:
> <david.hagood <at> gmail.com> writes:
>
> > Error: Entry "Some file name" not uptodate: cannot merge.
> >
> > We've tried "git reset --hard; git pull ." We've tried "git reset --hard;
> > git checkout -f master". Neither seems to fix this.
>
> http://article.gmane.org/gmane.comp.version-control.git/122862
To summarize: the suggestion is "rm .git/index && git reset --hard".
I have to stress the same point as in "reset --hard considered harmful" a
while back, though.
Actually, I started writing a patch to provide "git checkout --fix-crlf"
some weeks ago, but I constantly run out of time to finish it.
Ciao,
Dscho
^ permalink raw reply
* Re: What's cooking in git.git (Aug 2009, #06; Sun, 30)
From: Nick Edelen @ 2009-09-01 22:25 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7viqg48nxi.fsf@alter.siamese.dyndns.org>
> * ne/rev-cache (2009-08-21) 6 commits
> . support for path name caching in rev-cache
> . full integration of rev-cache into git, completed test suite
> . administrative functions for rev-cache, start of integration into git
> . support for non-commit object caching in rev-cache
> . basic revision cache system, no integration or features
> . man page and technical discussion for rev-cache
>
> Updated but seems to break upload-pack tests when merged to 'pu'; given
> what this series touches, breakages in that area are expected.
> May discard if a working reroll comes, to give it a fresh start.
I vaguely remember something concerning those tests when starting the
project. I'm a bit disconnected from everything right now, but I'll
try to get those fixed as soon as I can.
^ permalink raw reply
* [JGIT PATCH] move the 'empty-line' check in RefDatabase#readLine down a bit to after we removed all the whitespaces.
From: Mark Struberg @ 2009-09-01 21:41 UTC (permalink / raw)
To: Shawn Spearce, robin.rosenberg; +Cc: git
This way we consistently return null regardless if the line is empty or if it does only contain whitespaces.
Signed-off-by: Mark Struberg <struberg@yahoo.de>
---
.../src/org/spearce/jgit/lib/RefDatabase.java | 7 ++++---
1 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/RefDatabase.java b/org.spearce.jgit/src/org/spearce/jgit/lib/RefDatabase.java
index 477dc62..acc835b 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/lib/RefDatabase.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/RefDatabase.java
@@ -498,14 +498,15 @@ private static String readLine(final File file)
throws FileNotFoundException, IOException {
final byte[] buf = NB.readFully(file, 4096);
int n = buf.length;
- if (n == 0)
- return null;
// remove trailing whitespaces
while (n > 0 && Character.isWhitespace(buf[n - 1])) {
n--;
}
-
+
+ if (n == 0)
+ return null;
+
return RawParseUtils.decode(buf, 0, n);
}
--
1.6.2.5
^ permalink raw reply related
* [PATCH v2] status: list unmerged files last
From: Johannes Sixt @ 2009-09-01 21:25 UTC (permalink / raw)
To: Junio C Hamano; +Cc: bill lam, git
In-Reply-To: <7vy6oy9z9r.fsf@alter.siamese.dyndns.org>
The list of unmerged files is considered rather important because after
a conflicted merge they need attention. Since the output of git status does
not go through the pager, the end of the output remains immediately visible
in the terminal window. By placing unmerge entries at the end of the list,
the user can see them immediately.
Moreover, keeping the unmerge entries at the top is inconvenient if a merge
touched many files, but only a few conflicted: After the conflicts were
resolved, the user will conduct a 'git add' command. In order to do that
with copy-and-paste, the user must scroll the terminal window up, and must
do so for each individual entry (because terminal windows commonly scroll
down automatically on the paste operation to make the cursor visible).
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
On Dienstag, 1. September 2009, Junio C Hamano wrote:
> Johannes Sixt <j6t@kdbg.org> writes:
> > Moreover, keeping the unmerge entries at the top is inconvenient if a
> > merge touched many files, but only a few conflicted: After the conflicts
> > were resolved, the user will conduct a 'git add' command. In order to do
> > that with copy-and-paste, the user must scroll the terminal window up,
> > and must do so for each individual entry (because terminal windows
> > commonly scroll down automatically on the paste operation to make the
> > cursor visible).
>
> I actually was expecting that you would move this at the very bottom after
> untracked list for the above reason, and also because this part is only
> shown while running status (that was a good point you made in the previous
> message) and never in commit.
So you would not mind a more "drastic" change?
This version 2 can be regarded as a real improvement with the argument
above, whereas version 1 would only correct something of some
sort of regression, compared to v1.6.4.
(Originally I didn't dare to change too much and thought keeping staged
files together would make sense.)
-- Hannes
wt-status.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/wt-status.c b/wt-status.c
index 3395456..60d8425 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -561,7 +561,6 @@ void wt_status_print(struct wt_status *s)
color_fprintf_ln(s->fp, color(WT_STATUS_HEADER, s), "#");
}
- wt_status_print_unmerged(s);
wt_status_print_updated(s);
wt_status_print_changed(s);
if (s->submodule_summary)
@@ -570,6 +569,7 @@ void wt_status_print(struct wt_status *s)
wt_status_print_untracked(s);
else if (s->commitable)
fprintf(s->fp, "# Untracked files not listed (use -u option to show untracked
files)\n");
+ wt_status_print_unmerged(s);
if (s->verbose)
wt_status_print_verbose(s);
--
1.6.4.2.280.gb16ab
^ 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