* Re: `Git Status`-like output for two local branches
From: Sverre Rabbelier @ 2009-09-02 8:18 UTC (permalink / raw)
To: Jeff King; +Cc: Tim Visher, Git Mailing List
In-Reply-To: <20090902075713.GA1832@coredump.intra.peff.net>
Heya,
On Wed, Sep 2, 2009 at 09:57, Jeff King<peff@peff.net> wrote:
> 2. Count the commits on each side that are not in the other.
[...]
> You can also do that by parsing the output of:
> git rev-list --left-right $a...$b --
Perhaps it is useful to introduce a --left-right-count or such?
--
Cheers,
Sverre Rabbelier
^ permalink raw reply
* Re: [BUG] 'add -u' doesn't work from untracked subdir
From: Jeff King @ 2009-09-02 8:19 UTC (permalink / raw)
To: SZEDER Gábor; +Cc: git
In-Reply-To: <20090902080305.GA11549@neumann>
On Wed, Sep 02, 2009 at 10:03:05AM +0200, SZEDER Gábor wrote:
> As the subject says, 'git add -u' does not work from an untracked
> subdir, because it doesn't add modified files to the index. The
> following script reproduces the issue:
>
> mkdir repo
> cd repo
> git init
> echo 1 >foo
> git add foo
> git commit -m first
> echo 2 >foo
> mkdir untracked_subdir
> cd untracked_subdir
> git add -u
> git diff
>
> It worked in the initial 'git add -u' implementation (dfdac5d, git-add
> -u: match the index with working tree, 2007-04-20), but 2ed2c222
> (git-add -u paths... now works from subdirectory, 2007-08-16) broke it
> later, and is broken ever since.
It is not just untracked subdirs. Try:
mkdir repo && cd repo && git init
echo 1 >foo
mkdir subdir
echo 1 >subdir/bar
git add . && git commit -m first
echo 2 >foo
echo 2 >subdir/bar
cd subdir
git add -u
git diff ;# still shows foo/1 in index
git diff --cached ;# shows subdir/bar was updated
While I have sometimes found the behavior a bit annoying[1], I always
assumed that was the intended behavior.
And indeed, in modern builtin-add.c, we find this:
if ((addremove || take_worktree_changes) && !argc) {
static const char *here[2] = { ".", NULL };
argc = 1;
argv = here;
}
which seems pretty explicit.
-Peff
[1] I would prefer "git add -u ." to add only the current directory, and
"git add -u" to touch everything. But then, I am one of the people who
turn off status.relativepaths, so I think I may be in the minority in
always wanting to think of the project as a whole.
^ permalink raw reply
* [BUG] 'add -u' doesn't work from untracked subdir
From: SZEDER Gábor @ 2009-09-02 8:03 UTC (permalink / raw)
To: git
Hi,
As the subject says, 'git add -u' does not work from an untracked
subdir, because it doesn't add modified files to the index. The
following script reproduces the issue:
mkdir repo
cd repo
git init
echo 1 >foo
git add foo
git commit -m first
echo 2 >foo
mkdir untracked_subdir
cd untracked_subdir
git add -u
git diff
It worked in the initial 'git add -u' implementation (dfdac5d, git-add
-u: match the index with working tree, 2007-04-20), but 2ed2c222
(git-add -u paths... now works from subdirectory, 2007-08-16) broke it
later, and is broken ever since.
Regards,
Gábor
^ permalink raw reply
* Re: `Git Status`-like output for two local branches
From: Jeff King @ 2009-09-02 7:57 UTC (permalink / raw)
To: Tim Visher; +Cc: Git Mailing List
In-Reply-To: <c115fd3c0908311320q46d585d2v457ccd0f411a6404@mail.gmail.com>
On Mon, Aug 31, 2009 at 04:20:47PM -0400, Tim Visher wrote:
> I'm interested in being able to get a message such as 'dev and master
> have diverged, having 1 and 2 commits different respectively' or 'dev
> is behind master by 3 commits and can be fast-forwarded', etc. I'm
> sure this is simple, but I can't figure out how to do it in the docs.
> Sorry for the noobness of the question.
No, there isn't a simple command to say "show me this status text for
these two arbitrary branches".
However, the process is relatively simple to implement in a shell
script:
1. Pick your two branches. I'm not clear on whether you want to
compare two arbitrary branches, or what.
For the regular "status", one is the current branch, and the other
is the "upstream" branch (as configured by your
branch.$current.merge variables). Calculating "upstream" can
actually be a bit tricky because it involves looking at several
configuraiton variables. However, recent versions of git allow this
shell snippet (the 'upstream' formatter was added in v1.6.3):
current=`git symbolic-ref HEAD`
upstream=`git for-each-ref --format='%(upstream)' $current`
2. Count the commits on each side that are not in the other. The
simplest way to do this is:
in_a=`git rev-list $b..$a -- | wc -l`
in_b=`git rev-list $a..$b -- | wc -l`
but the internal code actually does both traversals simultaneously,
which is slightly more efficient. You can also do that by parsing
the output of:
git rev-list --left-right $a...$b --
which will mark commits in "a" with a '<' and commits in "b" with
a '>'. I don't know whether the extra complexity is worth the
efficiency gain (in the internal code it is easier, since we
aren't actually generating output and parsing it; we just keep a
count).
3. Compare the counts. If:
a=0, b=0: they are the same commit
a=0, b>0: a is behind b by $b commits, and can be fast-forwarded
a>0, b=0: a is ahead of b by $a commits (and can be pushed, or b
can be fast-forwarded to a)
a>0, b>0: branches have diverged and have $a and $b commits
respectively
So it's easy to script, but not exactly a one-liner. We might be able to
do better if you reduce the problem space. What exactly are you trying
to accomplish?
-Peff
^ permalink raw reply
* Re: [PATCH] clone: disconnect transport after fetching
From: Sverre Rabbelier @ 2009-09-02 7:37 UTC (permalink / raw)
To: Jeff King
Cc: Junio C Hamano, git, Daniel Barkalow, Björn Steinbrink,
Matthieu Moy, Sitaram Chamarty
In-Reply-To: <20090902072638.GC31528@coredump.intra.peff.net>
Heya,
On Wed, Sep 2, 2009 at 09:26, Jeff King<peff@peff.net> wrote:
> So think of it as you exposing a long-standing bug. ;)
Ah, well in that case, you're all welcome :P.
--
Cheers,
Sverre Rabbelier
^ permalink raw reply
* Re: [PATCH] clone: disconnect transport after fetching
From: Jeff King @ 2009-09-02 7:26 UTC (permalink / raw)
To: Sverre Rabbelier
Cc: Junio C Hamano, git, Daniel Barkalow, Björn Steinbrink,
Matthieu Moy, Sitaram Chamarty
In-Reply-To: <fabb9a1e0909020009r3ee28b1fo3cba095abafec9d4@mail.gmail.com>
On Wed, Sep 02, 2009 at 09:09:19AM +0200, Sverre Rabbelier wrote:
> On Wed, Sep 2, 2009 at 08:36, Jeff King<peff@peff.net> wrote:
> > As you can see from the commit message, I did a little extra hunting to
> > make sure we are not going to impact any other code paths, and I am
> > pretty sure we are fine.
>
> Thank you for fixing my mistake :).
You're welcome, though I am not sure it is your mistake. Arguably this
is something we should have been doing all along. The point of
abstracting transports was that we didn't need to know their details at
the outer layer, but in this case we were relying on the fact that no
transports (until empty-clone-over-git) needed an explicit
transport_disconnect to cleanly hang up on the other end.
So think of it as you exposing a long-standing bug. ;)
-Peff
^ permalink raw reply
* Re: [PATCH] clone: disconnect transport after fetching
From: Sverre Rabbelier @ 2009-09-02 7:09 UTC (permalink / raw)
To: Jeff King
Cc: Junio C Hamano, git, Daniel Barkalow, Björn Steinbrink,
Matthieu Moy, Sitaram Chamarty
In-Reply-To: <20090902063647.GA29559@coredump.intra.peff.net>
Heya,
On Wed, Sep 2, 2009 at 08:36, Jeff King<peff@peff.net> wrote:
> As you can see from the commit message, I did a little extra hunting to
> make sure we are not going to impact any other code paths, and I am
> pretty sure we are fine.
Thank you for fixing my mistake :).
--
Cheers,
Sverre Rabbelier
^ permalink raw reply
* Re: stash --dwim safety
From: Johannes Schindelin @ 2009-09-02 6:48 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Matthieu Moy, git
In-Reply-To: <20090902045937.GA12046@coredump.intra.peff.net>
Hi,
On Wed, 2 Sep 2009, Jeff King wrote:
> [cc'ing Dscho, as he was the main opponent of similar proposals, and I
> suspect his silence here means he missed this discussion.
Your assumption is correct.
I am overloaded with work, and in such times it is highly unlikely that I
get back to a discussion that was less than fun.
Ciao,
Dscho
^ permalink raw reply
* [PATCH] clone: disconnect transport after fetching
From: Jeff King @ 2009-09-02 6:36 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Daniel Barkalow, Sverre Rabbelier, Björn Steinbrink,
Matthieu Moy, Sitaram Chamarty
In-Reply-To: <alpine.LNX.2.00.0909020159080.28290@iabervon.org>
The current code just leaves the transport in whatever state
it was in after performing the fetch. For a non-empty clone
over the git protocol, the transport code already
disconnects at the end of the fetch.
But for an empty clone, we leave the connection hanging, and
eventually close the socket when clone exits. This causes
the remote upload-pack to complain "the remote end hung up
unexpectedly". While this message is harmless to the clone
itself, it is unnecessarily scary for a user to see and may
pollute git-daemon logs.
This patch just explicitly calls disconnect after we are
done with the remote end, which sends a flush packet to
upload-pack and cleanly disconnects, avoiding the error
message.
Other transports are unaffected or slightly improved:
- for a non-empty repo over the git protocol, the second
disconnect is a no-op (since we are no longer connected)
- for "walker" transports (like HTTP or FTP), we actually
free some used memory (which previously just sat until
the clone process exits)
- for "rsync", disconnect is always a no-op anyway
Signed-off-by: Jeff King <peff@peff.net>
---
This was suggested by Daniel, so theoretically
Acked-by: Daniel Barkalow <barkalow@iabervon.org>
:)
As you can see from the commit message, I did a little extra hunting to
make sure we are not going to impact any other code paths, and I am
pretty sure we are fine.
builtin-clone.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/builtin-clone.c b/builtin-clone.c
index 991a7ae..0f231d8 100644
--- a/builtin-clone.c
+++ b/builtin-clone.c
@@ -580,8 +580,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
option_no_checkout = 1;
}
- if (transport)
+ if (transport) {
transport_unlock_pack(transport);
+ transport_disconnect(transport);
+ }
if (!option_no_checkout) {
struct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));
--
1.6.4.2.401.ga275f.dirty
^ permalink raw reply related
* Re: stash --dwim safety
From: Matthieu Moy @ 2009-09-02 6:26 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git
In-Reply-To: <7vy6oyj892.fsf@alter.siamese.dyndns.org>
Thanks for taking care of this,
Junio C Hamano <gitster@pobox.com> writes:
> 'git stash' [save [--patch] [-k|--[no-]keep-index] [-q|--quiet] [<message>]]
To be precise, you can change this to
> 'git stash' [save [--patch] [-k|--[no-]keep-index] [-q|--quiet] [--] [<message>]]
(added [--])
--
Matthieu
^ permalink raw reply
* Re: clong an empty repo over ssh causes (harmless) fatal
From: Daniel Barkalow @ 2009-09-02 6:02 UTC (permalink / raw)
To: Jeff King
Cc: Sverre Rabbelier, Björn Steinbrink, Matthieu Moy,
Sitaram Chamarty, git
In-Reply-To: <20090902051615.GC12046@coredump.intra.peff.net>
On Wed, 2 Sep 2009, Jeff King wrote:
> 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?
You probably should; I'm not sure when I'd get to putting together a
patch, and you did the hard part (figuring out what was going on) anyway.
-Daniel
*This .sig left intentionally blank*
^ permalink raw reply
* Re: [PATCH] git.el: Make it easy to add unmerged files
From: Martin Nordholts @ 2009-09-02 5:48 UTC (permalink / raw)
To: Alexandre Julliard; +Cc: git, Junio C Hamano
In-Reply-To: <87ws4l47k2.fsf@wine.dyndns.org>
On 08/30/2009 05:58 PM, Alexandre Julliard wrote:
> Martin Nordholts <martin@chromecode.com> writes:
>
>> (Resending as I managed to mangle the previous patch despite trying not to...)
>>
>> It is nice and easy to git-add ignored and unknown files in a
>> git-status buffer. Make it equally easy to add unmerged files which is
>> a common use case.
>
> That's not quite what adding a file means in git.el, unmerged files are
> considered added already, and marking them resolved is done through the
> git-resolve-file command. Of course that was implemented before git
> overloaded the meaning of git-add to mean git-update-index, so maybe we
> should follow the trend and use git-add-file for all index updates. In
> that case git-resolve-file should probably be removed.
Since git instructs the user to use git-add for marking unmerged files
as resolved ("After resolving the conflicts, mark the corrected paths
with 'git add <paths>' or 'git rm <paths>' and commit the result.") and
doesn't even mention git-update-index, I think we should change git.el
accordingly.
But why do we need to also remove and disable git-resolve-file from
git.el? It doesn't hurt to keep that function and the keybinding, does
it?
/ Martin
^ permalink raw reply
* 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
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