Re: [PATCH v2 9/9] read_loose_refs(): read refs using resolve_ref_recursively()

[Michael Haggerty]

On 02/10/2017 08:22 PM, Junio C Hamano wrote:
> Michael Haggerty  writes:
>> [...]
> 
> OK, but one thing puzzles me...
> 
>> @@ -1390,27 +1390,6 @@ static struct ref_store *main_ref_store;
>>  static struct hashmap submodule_ref_stores;
>>  
>>  /*
>> - * Return the ref_store instance for the specified submodule (or the
>> - * main repository if submodule is NULL). If that ref_store hasn't
>> - * been initialized yet, return NULL.
>> - */
>> -static struct ref_store *lookup_ref_store(const char *submodule)
>> -{
>> -struct submodule_hash_entry *entry;
>> -
>> -if (!submodule)
>> -return main_ref_store;
>> -
>> -if (!submodule_ref_stores.tablesize)
>> -/* It's initialized on demand in register_ref_store(). */
>> -return NULL;
>> -
>> -entry = hashmap_get_from_hash(_ref_stores,
>> -  strhash(submodule), submodule);
>> -return entry ? entry->refs : NULL;
>> -}
>> -
>> -/*
>>   * Register the specified ref_store to be the one that should be used
>>   * for submodule (or the main repository if submodule is NULL). It is
>>   * a fatal error to call this function twice for the same submodule.
>> @@ -1451,6 +1430,27 @@ static struct ref_store *ref_store_init(const char 
>> *submodule)
>>  return refs;
>>  }
>>  
>> +/*
>> + * Return the ref_store instance for the specified submodule (or the
>> + * main repository if submodule is NULL). If that ref_store hasn't
>> + * been initialized yet, return NULL.
>> + */
>> +static struct ref_store *lookup_ref_store(const char *submodule)
>> +{
>> +struct submodule_hash_entry *entry;
>> +
>> +if (!submodule)
>> +return main_ref_store;
>> +
>> +if (!submodule_ref_stores.tablesize)
>> +/* It's initialized on demand in register_ref_store(). */
>> +return NULL;
>> +
>> +entry = hashmap_get_from_hash(_ref_stores,
>> +  strhash(submodule), submodule);
>> +return entry ? entry->refs : NULL;
>> +}
>> +
> 
> I somehow thought that we had an early "reorder the code" step to
> avoid hunks like these?  Am I missing some subtle changes made while
> moving the function down?

You are quite right; thanks for noticing. I forgot to un-move this
function when re-rolling. These two hunks can be discarded (the function
text is unchanged).

I pushed the fixed commit to branch `submodule-hash` in my fork [1]. If
you'd like me to send it to the mailing list again, please let me know.

Michael

[1] https://github.com/mhagger/git

Re: Git status performance on PS (command prompt)

[Christian Couder]

On Sun, Feb 12, 2017 at 5:46 PM, brian m. carlson
 wrote:
> On Sun, Feb 12, 2017 at 04:53:47PM +0100, Mark Gaiser wrote:

[...]

>> I did a bit of profiling in git to figure out where this slowdown comes from.
>> Callgrind tells me that "read_directory_recursive" takes up ~62% of the time.
>> Within that call the function "last_exclude_matching_from_list" takes
>> up 49% of the time it takes to run "git status --porcelain -b".

The core.untrackedCache config option may help.

Re: Working with public-inbox.org [Was: [PATCH] rev-parse: respect core.hooksPath in --git-path]

[Arif Khokar]

On 02/10/2017 11:10 AM, Johannes Schindelin wrote:

Hi Arif,

On Wed, 24 Aug 2016, Johannes Schindelin wrote:



I recently adapted an old script I had to apply an entire patch series
given the GMane link to its cover letter:

https://github.com/git-for-windows/build-extra/blob/master/apply-from-gmane.sh

Maybe you find it in you to adapt that to work with public-inbox.org?


Oh well. That would have been too easy a task, right?

As it happens, I needed this functionality myself (when reworking my
git-path-in-subdir patch to include Mike Rappazzo's previous patch series
that tried to fix the same bug).

I copy-edited the script to work with public-inbox.org, it accepts a
Message-ID or URL or GMane URL and will try to apply the patch (or patch
series) on top of the current revision:

https://github.com/git-for-windows/build-extra/blob/2268850552c7/apply-from-public-inbox.sh


Thanks for the link.  One thing that comes to mind that is that it may 
be better to just download the patches and then manually apply them 
afterwords rather than doing it in the script itself.  Or at least add 
an option to the script to not automatically invoke git am.


Getting back to the point I made when this thread was still active, I 
still think it would be better to be able to list the message-id values 
in the header or body of the cover letter message of a patch series 
(preferably the former) in order to facilitate downloading the patches 
via NNTP from gmane or public-inbox.org.  That would make it easier 
compared to the different, ad-hoc, methods that exist for each email client.


Alternatively, or perhaps in addition to the list of message-ids, a list 
of URLs to public-inbox.org or gmane messages could also be provided for 
those who prefer to download patches via HTTP.

Re: [PATCH v2 0/9] Store submodules in a hash, not a linked list

[David Turner]

On Fri, 2017-02-10 at 12:16 +0100, Michael Haggerty wrote:
> This is v2 of the patch series, considerably reorganized but not that
> different codewise. Thanks to Stefan, Junio, and Peff for their
> feedback about v1 [1]. I think I have addressed all of your comments.

LGTM.

Incorrect pipe for git log graph when using --name-status option

[hIpPy]

The `git log` command with `graph` and pretty format works correctly
as expected.

$ git log --graph --pretty=format:'%h' -2
* 714a14e
* 87dce5f


However, with `--name-status` option added, there is a pipe
incorrectly placed after the commit hash (example below).

$ git log --graph --pretty=format:'%h' -2 --name-status
* 714a14e|
| M README.md
| A rm.Extensions/BitSet.cs
| M rm.Extensions/Properties/AssemblyInfo.cs
| M rm.Extensions/rm.Extensions.csproj
| A rm.ExtensionsTest/BitSetTest.cs
| M rm.ExtensionsTest/rm.ExtensionsTest.csproj

* 87dce5f|
| M rm.Extensions/GraphExtension.cs
| M rm.Extensions/Wrapped.cs
| M rm.Extensions/WrappedExtension.cs
| M rm.Extensions/rm.Extensions.csproj


IMHO, I think this is a bug. I think the correct output should be
below.

$ git log --graph --pretty=format:'%h' -2 --name-status
* 714a14e
| M README.md
| A rm.Extensions/BitSet.cs
| M rm.Extensions/Properties/AssemblyInfo.cs
| M rm.Extensions/rm.Extensions.csproj

| A rm.ExtensionsTest/BitSetTest.cs
| M rm.ExtensionsTest/rm.ExtensionsTest.csproj
|
* 87dce5f
| M rm.Extensions/GraphExtension.cs
| M rm.Extensions/Wrapped.cs
| M rm.Extensions/WrappedExtension.cs

| M rm.Extensions/rm.Extensions.csproj

I'm using this:

git version 2.11.0.windows.1
GNU bash, version 4.3.46(2)-release (x86_64-pc-msys)
Windows 8.1 64-bit


Thanks,
RM

Re: [PATCH 0/5] Store submodules in a hash, not a linked list

[Duy Nguyen]

On Thu, Feb 09, 2017 at 07:40:33PM -0500, Jeff King wrote:
> On Thu, Feb 09, 2017 at 10:23:35PM +0100, Michael Haggerty wrote:
> 
> > >> So push the submodule attribute down to the `files_ref_store` class
> > >> (but continue to let the `ref_store`s be looked up by submodule).
> > > 
> > > I'm not sure I understand all of the ramifications here. It _sounds_ like
> > > pushing this down into the files-backend code would make it harder to
> > > have mixed ref-backends for different submodules. Or is this just
> > > pushing down an implementation detail of the files backend, and future
> > > code is free to have as many different ref_stores as it likes?
> > 
> > I don't understand how this would make it harder, aside from the fact
> > that a new backend class might also need a path member and have to
> > maintain its own copy rather than using one that the base class provides.
> 
> Probably the answer is "I'm really confused".
> 
> But here's how my line of reasoning went:
> 
>   Right now we have a main ref-store that points to the submodule
>   ref-stores. I don't know the current state of it, but in theory those
>   could all use different backends.
> 
>   This seems like it's pushing that submodule linkage down into the
>   backend.
> 
> But I think from your response that the answer is no, the thing that is
> being pushed down is not the right way for the main ref store and the
> submodules to be linked.

I think it's more about "pushing out" than "pushing down". Once files
backend takes a path to .git directory, we could have a submodule
ref_store that resolves submodule path to that .git directory,
files-backend will not need to know anything about submodules.

I imagine in future lookup_ref_store() will take a .git path instead
of a submodule path, then iterate through all backends and call the
backend-specific "probe" function to let the backend figure out if
it's the right backend and whatever parameters it needs (e.g. IP
address or path). There would be submodule_lookup_ref_store() wrapper
that converts submodule path to .git path for lookup_ref_store() to
consume.
--
Duy

[PATCH] completion: teach to complete git-subtree

[cornelius . weig]

From: Cornelius Weig 

Git-subtree is a contrib-command without completion, making it
cumbersome to use.

Teach bash completion to complete the subcommands of subtree (add,
merge, pull, push, split) and options thereof. For subcommands
supporting it (add, push, pull) also complete remote names and refspec.

Signed-off-by: Cornelius Weig 
---
 contrib/completion/git-completion.bash | 35 ++
 1 file changed, 31 insertions(+), 4 deletions(-)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 6c6e1c7..430bfed 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -535,9 +535,9 @@ __git_complete_remote_or_refspec ()
 {
local cur_="$cur" cmd="${words[1]}"
local i c=2 remote="" pfx="" lhs=1 no_complete_refspec=0
-   if [ "$cmd" = "remote" ]; then
-   ((c++))
-   fi
+   case "$cmd" in
+   remote|subtree) ((c++)) ;;
+   esac
while [ $c -lt $cword ]; do
i="${words[c]}"
case "$i" in
@@ -586,7 +586,7 @@ __git_complete_remote_or_refspec ()
__gitcomp_nl "$(__git_refs)" "$pfx" "$cur_"
fi
;;
-   pull|remote)
+   pull|remote|subtree)
if [ $lhs = 1 ]; then
__gitcomp_nl "$(__git_refs "$remote")" "$pfx" "$cur_"
else
@@ -2569,6 +2569,33 @@ _git_submodule ()
fi
 }
 
+_git_subtree ()
+{
+   local subcommands="add merge pull push split"
+   local subcommand="$(__git_find_on_cmdline "$subcommands")"
+   if [ -z "$subcommand" ]; then
+   __gitcomp "$subcommands"
+   return
+   fi
+   case "$subcommand,$cur" in
+   add,--*|merge,--*|pull,--*)
+   __gitcomp "--prefix= --squash --message="
+   ;;
+   push,--*)
+   __gitcomp "--prefix="
+   ;;
+   split,--*)
+   __gitcomp "
+   --annotate= --branch= --ignore-joins
+   --onto= --prefix= --rejoin
+   "
+   ;;
+   add,*|push,*|pull,*)
+   __git_complete_remote_or_refspec
+   ;;
+   esac
+}
+
 _git_svn ()
 {
local subcommands="
-- 
2.10.2

Re: [PATCH] fetch: print an error when declining to request an unadvertised object

[Junio C Hamano]

Matt McCutchen  writes:

> What do you think?  Do you not care about having a more specific error,
> in which case I can copy the code from builtin/fetch-pack.c to
> fetch_refs_via_pack?  Or shall I add code to filter_refs to set a flag
> and add code to builtin/fetch-pack.c and fetch_refs_via_pack to check
> the flag?  Or what?

The fact that we have the above two choices tells me that a two-step
approach may be an appropriate approach.

The first step is to teach fetch_refs_via_pack() that it should not
ignore the information returned in sought[].  It would add new code
similar to what cmd_fetch_pack() uses to notice and report errors
[*1*] to the function.  It would be a sensible first step, but would
not let the user know which of multiple causes of "not matched" we
noticed.

By "a more specific error", I think you are envisioning that the
current boolean "matched" is made into an enum that allows the
caller to tell how each request did not match [*2*].  That can be
the topic of the second patch and would have to touch filter_refs()
[*3*], cmd_fetch_pack() and fetch_refs_via_pack().

I do not have strong preference myself offhand between stopping at
the first step or completing both.

Even if you did only the first step, as long as the second step can
be done without reverting what the first step did [*4*] by somebody
who cares the "specific error" deeply enough, I am OK with that.  Of
course if you did both steps, that is fine by me as well ;-)


[Footnote]

*1* While I know that it is not right to die() in filter_refs(), and
fetch_refs_via_pack() is a better place to notice errors, I do
not offhand know if it is the right place to report errors, or a
caller higher in the callchain may want the callee to be silent
and wants to show its own error message (in which case the error
may have to percolate upwards in the callchain).

*2* e.g. "was it a ref but they did not advertise?  Did it request
an explicit object name and they did not allow it?"  We may want
to support other "more specific" errors that can be detected in
the future.

*3* The current code flips the sought[i]->matched bit on for matched
ones (relying on the initial state of the bit being false), but
it now needs to stuff different kind of "not matched" to the
field to allow the caller to act on it.

*4* IOW, I am OK with an initial "small" improvement, but I'd want
to make sure that such an initial step does not make future
enhancements by others harder.

Re: [git-gui] Amending doesn't preserve timestamp

[Igor Djordjevic BugA]

On 12/02/2017 22:40, Juraj wrote:
> Hi Igor,
> 
> I forgot to write the version I'm using. It's on Ubuntu 16.04, git-gui
> package version 1:2.7.4-0ubuntu1 (--version: git-gui version 0.20.0),
> git version 2.7.4, tcl and tk 8.6.0+9. Perhaps it got fixed in a newer
> version, in that case, my bad for not checking before posting.
> 
> Thanks,
> Juraj

Hi Juraj,

Indeed, if I`m reading it correctly, it seems to be addressed in git-gui
version 0.21.0[1], introduced in git version 2.11.0[2] on 2016-11-29
("git-gui: Do not reset author details on amend", 2016-04-11[3],
referencing an old bug report[4]).

Regards,
BugA

[1] https://public-inbox.org/git/878ttji701@red.patthoyts.tk/
[2]
https://public-inbox.org/git/xmqqmvgidlsg@gitster.mtv.corp.google.com/
[3]
https://public-inbox.org/git/1462458182-4488-1-git-send-email-org...@gmail.com/
[4]
https://public-inbox.org/git/CAGHpTB+35j0njmCZ0uCgBVroe=ma7hlnn6fdty8yebkwgem...@mail.gmail.com/

Re: What's cooking in git.git (Feb 2017, #03; Fri, 10)

[René Scharfe]

Am 12.02.2017 um 19:32 schrieb Vegard Nossum:

I said I would resubmit the patches with more config options and more
command-line arguments (to avoid potentially breaking backwards
compatibility), but IIRC the response seemed to be "preceding blank line
heuristic is good enough" and "why bother", so I ended up not not
resubmitting anything.


I was (and still am) looking forward to your patches.  The current 
heuristic is simplistic, the patches you already sent improve the output 
in certain scenarios, my proposed changes on top aimed to limit 
drawbacks in other scenarios, but together they still have shortcomings.


Avoiding new switches would be nice, though (if possible).  I feel we 
need a lot more tests to nail down our expectations.


René

Re: [PATCH v4 0/7] stash: support pathspec argument

[Thomas Gummerer]

On 02/12, Thomas Gummerer wrote:
> Thanks Peff and Junio for the review of the last round.
>

Sorry it seems like I messed up the In-Reply-To header.  Previous rounds were 
at:
v1: http://public-inbox.org/git/20170121200804.19009-1-t.gumme...@gmail.com/
v2: http://public-inbox.org/git/20170129201604.30445-1-t.gumme...@gmail.com/
v3: http://public-inbox.org/git/20170205202642.14216-1-t.gumme...@gmail.com/

[PATCH v4 3/7] stash: add test for the create command line arguments

[Thomas Gummerer]

Currently there is no test showing the expected behaviour of git stash
create's command line arguments.  Add a test for that to show the
current expected behaviour and to make sure future refactorings don't
break those expectations.

Signed-off-by: Thomas Gummerer 
---
 t/t3903-stash.sh | 18 ++
 1 file changed, 18 insertions(+)

diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 3577115807..ffe3549ea5 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -784,4 +784,22 @@ test_expect_success 'push -m shows right message' '
test_cmp expect actual
 '
 
+test_expect_success 'create stores correct message' '
+   >foo &&
+   git add foo &&
+   STASH_ID=$(git stash create "create test message") &&
+   echo "On master: create test message" >expect &&
+   git show --pretty=%s -s ${STASH_ID} >actual &&
+   test_cmp expect actual
+'
+
+test_expect_success 'create with multiple arguments for the message' '
+   >foo &&
+   git add foo &&
+   STASH_ID=$(git stash create test untracked) &&
+   echo "On master: test untracked" >expect &&
+   git show --pretty=%s -s ${STASH_ID} >actual &&
+   test_cmp expect actual
+'
+
 test_done
-- 
2.11.0.301.g86e6ecc671.dirty

[PATCH v4 4/7] stash: introduce new format create

[Thomas Gummerer]

git stash create currently supports a positional argument for adding a
message.  This is not quite in line with how git commands usually take
comments (using a -m flag).

Add a new syntax for adding a message to git stash create using a -m
flag.  This is with the goal of deprecating the old style git stash
create with positional arguments.

This also adds a -u argument, for untracked files.  This is already used
internally as another positional argument, but can now be used from the
command line.

This introduces a slight regression, when git stash create -m works is
used.  Before this change, it created a stash with the message
"-m works", but now it creates a stash with the message "-m".

Signed-off-by: Thomas Gummerer 
---
 Documentation/git-stash.txt |  1 +
 git-stash.sh| 52 +
 t/t3903-stash.sh|  9 
 3 files changed, 58 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 2e9e344cd7..a138ed6a24 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -17,6 +17,7 @@ SYNOPSIS
 [-u|--include-untracked] [-a|--all] []]
 'git stash' clear
 'git stash' create []
+'git stash' create [-m ] [-u|--include-untracked ]
 'git stash' store [-m|--message ] [-q|--quiet] 
 
 DESCRIPTION
diff --git a/git-stash.sh b/git-stash.sh
index 8365ebba2a..6d629fc43f 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -58,8 +58,52 @@ clear_stash () {
 }
 
 create_stash () {
-   stash_msg="$1"
-   untracked="$2"
+   stash_msg=
+   untracked=
+   new_style=
+   while test $# != 0
+   do
+   case "$1" in
+   -m|--message)
+   shift
+   test -z ${1+x} && usage
+   stash_msg="$1"
+   new_style=t
+   ;;
+   -u|--include-untracked)
+   shift
+   test -z ${1+x} && usage
+   untracked="$1"
+   new_style=t
+   ;;
+   *)
+   if test -n "$new_style"
+   then
+   echo "invalid argument"
+   option="$1"
+   # TRANSLATORS: $option is an invalid option, 
like
+   # `--blah-blah'. The 7 spaces at the beginning 
of the
+   # second line correspond to "error: ". So you 
should line
+   # up the second line with however many 
characters the
+   # translation of "error: " takes in your 
language. E.g. in
+   # English this is:
+   #
+   #$ git stash save --blah-blah 2>&1 | head 
-n 2
+   #error: unknown option for 'stash save': 
--blah-blah
+   #   To provide a message, use git stash 
save -- '--blah-blah'
+   eval_gettextln "error: unknown option for 
'stash create': \$option"
+   usage
+   fi
+   break
+   ;;
+   esac
+   shift
+   done
+
+   if test -z "$new_style"
+   then
+   stash_msg="$*"
+   fi
 
git update-index -q --refresh
if no_changes
@@ -268,7 +312,7 @@ push_stash () {
git reflog exists $ref_stash ||
clear_stash || die "$(gettext "Cannot initialize stash")"
 
-   create_stash "$stash_msg" $untracked
+   create_stash -m "$stash_msg" -u "$untracked"
store_stash -m "$stash_msg" -q $w_commit ||
die "$(gettext "Cannot save the current status")"
say "$(eval_gettext "Saved working directory and index state 
\$stash_msg")"
@@ -667,7 +711,7 @@ clear)
;;
 create)
shift
-   create_stash "$*" && echo "$w_commit"
+   create_stash "$@" && echo "$w_commit"
;;
 store)
shift
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index ffe3549ea5..812d0f7a40 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -802,4 +802,13 @@ test_expect_success 'create with multiple arguments for 
the message' '
test_cmp expect actual
 '
 
+test_expect_success 'new style stash create stores correct message' '
+   >foo &&
+   git add foo &&
+   STASH_ID=$(git stash create -m "create test message new style") &&
+   echo "On master: create test message new style" >expect &&
+   git show --pretty=%s -s ${STASH_ID} >actual &&
+   test_cmp expect actual
+'
+
 test_done
-- 
2.11.0.301.g86e6ecc671.dirty

[PATCH v4 5/7] stash: teach 'push' (and 'create') to honor pathspec

[Thomas Gummerer]

While working on a repository, it's often helpful to stash the changes
of a single or multiple files, and leave others alone.  Unfortunately
git currently offers no such option.  git stash -p can be used to work
around this, but it's often impractical when there are a lot of changes
over multiple files.

Allow 'git stash push' to take pathspec to specify which paths to stash.

Helped-by: Junio C Hamano 
Signed-off-by: Thomas Gummerer 
---
 Documentation/git-stash.txt| 11 ++
 git-stash.sh   | 50 --
 t/t3903-stash.sh   | 72 ++
 t/t3905-stash-include-untracked.sh | 26 ++
 4 files changed, 148 insertions(+), 11 deletions(-)

diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index a138ed6a24..f462f393b0 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -15,9 +15,13 @@ SYNOPSIS
 'git stash' branch  []
 'git stash' [save [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]
 [-u|--include-untracked] [-a|--all] []]
+'git stash' push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]
+[-u|--include-untracked] [-a|--all] [-m|--message ]]
+[--] [...]
 'git stash' clear
 'git stash' create []
 'git stash' create [-m ] [-u|--include-untracked ]
+[-- ...]
 'git stash' store [-m|--message ] [-q|--quiet] 
 
 DESCRIPTION
@@ -47,6 +51,7 @@ OPTIONS
 ---
 
 save [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] 
[-q|--quiet] []::
+push [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] 
[-q|--quiet] [-m|--message ] [--] [...]::
 
Save your local modifications to a new 'stash' and roll them
back to HEAD (in the working tree and in the index).
@@ -56,6 +61,12 @@ save [-p|--patch] [-k|--[no-]keep-index] 
[-u|--include-untracked] [-a|--all] [-q
only  does not trigger this action to prevent a misspelled
subcommand from making an unwanted stash.
 +
+When pathspec is given to 'git stash push', the new stash records the
+modified states only for the files that match the pathspec.  The index
+entries and working tree files are then rolled back to the state in
+HEAD only for these files, too, leaving files that do not match the
+pathspec intact.
++
 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 6d629fc43f..db56cf2c66 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -11,6 +11,7 @@ USAGE="list []
   [-u|--include-untracked] [-a|--all] []]
or: $dashless push [--patch] [-k|--[no-]keep-index] [-q|--quiet]
  [-u|--include-untracked] [-a|--all] [-m ]
+ [-- ...]
or: $dashless clear"
 
 SUBDIRECTORY_OK=Yes
@@ -35,15 +36,15 @@ else
 fi
 
 no_changes () {
-   git diff-index --quiet --cached HEAD --ignore-submodules -- &&
-   git diff-files --quiet --ignore-submodules &&
+   git diff-index --quiet --cached HEAD --ignore-submodules -- "$@" &&
+   git diff-files --quiet --ignore-submodules -- "$@" &&
(test -z "$untracked" || test -z "$(untracked_files)")
 }
 
 untracked_files () {
excl_opt=--exclude-standard
test "$untracked" = "all" && excl_opt=
-   git ls-files -o -z $excl_opt
+   git ls-files -o -z $excl_opt -- "$@"
 }
 
 clear_stash () {
@@ -76,6 +77,11 @@ create_stash () {
untracked="$1"
new_style=t
;;
+   --)
+   shift
+   new_style=t
+   break
+   ;;
*)
if test -n "$new_style"
then
@@ -103,10 +109,11 @@ create_stash () {
if test -z "$new_style"
then
stash_msg="$*"
+   set --
fi
 
git update-index -q --refresh
-   if no_changes
+   if no_changes "$@"
then
exit 0
fi
@@ -138,7 +145,7 @@ create_stash () {
# Untracked files are stored by themselves in a parentless 
commit, for
# ease of unpacking later.
u_commit=$(
-   untracked_files | (
+   untracked_files "$@" | (
GIT_INDEX_FILE="$TMPindex" &&
export GIT_INDEX_FILE &&
rm -f "$TMPindex" &&
@@ -161,7 +168,7 @@ create_stash () {
git read-tree --index-output="$TMPindex" -m $i_tree &&
GIT_INDEX_FILE="$TMPindex" &&
export GIT_INDEX_FILE &&
-   git diff-index --name-only -z HEAD -- 
>"$TMP-stagenames" &&
+   git diff-index --name-only -z 

[PATCH v4 6/7] stash: use stash_push for no verb form

[Thomas Gummerer]

Now that we have stash_push, which accepts pathspec arguments, use
it instead of stash_save in git stash without any additional verbs.

This does introduce a small regression.  Previously we allowed
git stash -- -message, with a hyphen in front of the message.  However
git stash -- message without the hyphen was not allowed.  After this
change adding a message to the stash, with or without hyphen is no
longer allowed.

While this now allows specifying a message with the -m flag, it also
disallows messages without a hyphen.  This is to prevent user errors,
where a user tries to stash something with a message, but changes their
mind, and now would like to apply a stash, but forgets to remove the -m
flag.  E.g. git stash -m mes^H^H^Happly would result in a stash with the
message apply, while the user might have intended to apply a previous
stash.

Signed-off-by: Thomas Gummerer 
---

If this kind of regression introduced here is not acceptable, I think
we could hide this change between some kind of config option, and
transition users over later after a warning period.

 Documentation/git-stash.txt |  8 
 git-stash.sh| 16 
 t/t3903-stash.sh|  4 +---
 3 files changed, 13 insertions(+), 15 deletions(-)

diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index f462f393b0..b0825f4aca 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -13,11 +13,11 @@ SYNOPSIS
 'git stash' drop [-q|--quiet] []
 'git stash' ( pop | apply ) [--index] [-q|--quiet] []
 'git stash' branch  []
-'git stash' [save [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]
-[-u|--include-untracked] [-a|--all] []]
-'git stash' push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]
+'git stash' save [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]
+[-u|--include-untracked] [-a|--all] []
+'git stash' [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]
 [-u|--include-untracked] [-a|--all] [-m|--message ]]
-[--] [...]
+[--] [...]]
 'git stash' clear
 'git stash' create []
 'git stash' create [-m ] [-u|--include-untracked ]
diff --git a/git-stash.sh b/git-stash.sh
index db56cf2c66..769cee9fd8 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -7,11 +7,11 @@ USAGE="list []
or: $dashless drop [-q|--quiet] []
or: $dashless ( pop | apply ) [--index] [-q|--quiet] []
or: $dashless branch  []
-   or: $dashless [save [--patch] [-k|--[no-]keep-index] [-q|--quiet]
-  [-u|--include-untracked] [-a|--all] []]
-   or: $dashless push [--patch] [-k|--[no-]keep-index] [-q|--quiet]
- [-u|--include-untracked] [-a|--all] [-m ]
- [-- ...]
+   or: $dashless save [--patch] [-k|--[no-]keep-index] [-q|--quiet]
+ [-u|--include-untracked] [-a|--all] []
+   or: $dashless [push [--patch] [-k|--[no-]keep-index] [-q|--quiet]
+  [-u|--include-untracked] [-a|--all] [-m ]
+  [-- ...]]
or: $dashless clear"
 
 SUBDIRECTORY_OK=Yes
@@ -699,7 +699,7 @@ apply_to_branch () {
 }
 
 PARSE_CACHE='--not-parsed'
-# The default command is "save" if nothing but options are given
+# The default command is "push" if nothing but options are given
 seen_non_option=
 for opt
 do
@@ -709,7 +709,7 @@ do
esac
 done
 
-test -n "$seen_non_option" || set "save" "$@"
+test -n "$seen_non_option" || set "push" "$@"
 
 # Main command set
 case "$1" in
@@ -760,7 +760,7 @@ branch)
 *)
case $# in
0)
-   save_stash &&
+   push_stash &&
say "$(gettext "(To restore them type \"git stash apply\")")"
;;
*)
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 8b372c35fb..d568799da9 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -274,9 +274,7 @@ test_expect_success 'stash --invalid-option' '
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 bar5,bar6 = $(cat file),$(cat file2)
 '
 
 test_expect_success 'stash an added file' '
-- 
2.11.0.301.g86e6ecc671.dirty

[PATCH v4 7/7] stash: allow pathspecs in the no verb form

[Thomas Gummerer]

Now that stash_push is used in the no verb form of stash, allow
specifying the command line for this form as well.  Always use -- to
disambiguate pathspecs from other non-option arguments.

Signed-off-by: Thomas Gummerer 
---
 git-stash.sh |  1 +
 t/t3903-stash.sh | 15 +++
 2 files changed, 16 insertions(+)

diff --git a/git-stash.sh b/git-stash.sh
index 769cee9fd8..a184b1e274 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -704,6 +704,7 @@ seen_non_option=
 for opt
 do
case "$opt" in
+   --) break ;;
-*) ;;
*) seen_non_option=t; break ;;
esac
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index d568799da9..22ac75377b 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -881,4 +881,19 @@ test_expect_success 'untracked files are left in place 
when -u is not given' '
test_path_is_file untracked
 '
 
+test_expect_success 'stash without verb with pathspec' '
+   >"foo bar" &&
+   >foo &&
+   >bar &&
+   git add foo* &&
+   git stash -- "foo b*" &&
+   test_path_is_missing "foo bar" &&
+   test_path_is_file foo &&
+   test_path_is_file bar &&
+   git stash pop &&
+   test_path_is_file "foo bar" &&
+   test_path_is_file foo &&
+   test_path_is_file bar
+'
+
 test_done
-- 
2.11.0.301.g86e6ecc671.dirty

[PATCH v4 0/7] stash: support pathspec argument

[Thomas Gummerer]

Thanks Peff and Junio for the review of the last round.

Changes since v3:

- Instead of using ${1-...} to check for missing arguments and dying
  show the usage for stash instead, being more consistent with how the
  rest of stash behaves
- Let push_stash handle the --help argument from save_stash as well,
  passing it through directly.

- Fix the tests to not pipe the output to something, and thereby
  swallowing the error codes

- Update the commit message in 4/7 to acknowledge that there is a
  small regression between the two versions

- Correctly detect when there are no changes, and only show the
  message once
- Fix the accidental deletion of untracked files when a pathspec is
  used.  Also added test cases for that.
- Add documentation for git stash push to the usage text
- Respect the -q flag and the GIT_QUIET environment variable

- This also adds two new patches, one using push_stash instead of
  save_stash for git stash without any verb.  The other allows using a
  pathspec in the verb-less option of git stash.

Interdiff below:
diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index be55e456fa..b0825f4aca 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -13,15 +13,15 @@ SYNOPSIS
 'git stash' drop [-q|--quiet] []
 'git stash' ( pop | apply ) [--index] [-q|--quiet] []
 'git stash' branch  []
-'git stash' [save [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]
-[-u|--include-untracked] [-a|--all] []]
-'git stash' push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]
+'git stash' save [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]
+[-u|--include-untracked] [-a|--all] []
+'git stash' [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]
 [-u|--include-untracked] [-a|--all] [-m|--message ]]
-[--] [...]
+[--] [...]]
 'git stash' clear
 'git stash' create []
 'git stash' create [-m ] [-u|--include-untracked ]
-[-- ...]
+[-- ...]
 'git stash' store [-m|--message ] [-q|--quiet] 
 
 DESCRIPTION
diff --git a/git-stash.sh b/git-stash.sh
index 46367d40aa..a184b1e274 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -7,8 +7,11 @@ USAGE="list []
or: $dashless drop [-q|--quiet] []
or: $dashless ( pop | apply ) [--index] [-q|--quiet] []
or: $dashless branch  []
-   or: $dashless [save [--patch] [-k|--[no-]keep-index] [-q|--quiet]
-  [-u|--include-untracked] [-a|--all] []]
+   or: $dashless save [--patch] [-k|--[no-]keep-index] [-q|--quiet]
+ [-u|--include-untracked] [-a|--all] []
+   or: $dashless [push [--patch] [-k|--[no-]keep-index] [-q|--quiet]
+  [-u|--include-untracked] [-a|--all] [-m ]
+  [-- ...]]
or: $dashless clear"
 
 SUBDIRECTORY_OK=Yes
@@ -33,8 +36,8 @@ else
 fi
 
 no_changes () {
-   git diff-index --quiet --cached HEAD --ignore-submodules -- &&
-   git diff-files --quiet --ignore-submodules &&
+   git diff-index --quiet --cached HEAD --ignore-submodules -- "$@" &&
+   git diff-files --quiet --ignore-submodules -- "$@" &&
(test -z "$untracked" || test -z "$(untracked_files)")
 }
 
@@ -64,11 +67,13 @@ create_stash () {
case "$1" in
-m|--message)
shift
-   stash_msg=${1?"-m needs an argument"}
+   test -z ${1+x} && usage
+   stash_msg="$1"
new_style=t
;;
-u|--include-untracked)
shift
+   test -z ${1+x} && usage
untracked="$1"
new_style=t
;;
@@ -104,14 +109,11 @@ create_stash () {
if test -z "$new_style"
then
stash_msg="$*"
-   while test $# != 0
-   do
-   shift
-   done
+   set --
fi
 
git update-index -q --refresh
-   if no_changes
+   if no_changes "$@"
then
exit 0
fi
@@ -270,7 +272,8 @@ push_stash () {
;;
-m|--message)
shift
-   stash_msg=${1?"-m needs an argument"}
+   test -z ${1+x} && usage
+   stash_msg=$1
;;
--help)
show_help
@@ -308,7 +311,7 @@ push_stash () {
fi
 
git update-index -q --refresh
-   if no_changes
+   if no_changes "$@"
then
say "$(gettext "No local changes to save")"
exit 0
@@ -325,9 +328,23 @@ push_stash () {
then
if test $# != 0
then
-   git ls-files -z -- "$@" | xargs -0 git reset --
-   git ls-files -z --modified -- "$@" | xargs -0 git 

[PATCH v4 2/7] stash: introduce push verb

[Thomas Gummerer]

Introduce a new git stash push verb in addition to git stash save.  The
push verb is used to transition from the current command line arguments
to a more conventional way, in which the message is given as an argument
to the -m option.

This allows us to have pathspecs at the end of the command line
arguments like other Git commands do, so that the user can say which
subset of paths to stash (and leave others behind).

Helped-by: Junio C Hamano 
Signed-off-by: Thomas Gummerer 
---
 git-stash.sh | 46 +++---
 t/t3903-stash.sh |  9 +
 2 files changed, 52 insertions(+), 3 deletions(-)

diff --git a/git-stash.sh b/git-stash.sh
index 10c284d1aa..8365ebba2a 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -9,6 +9,8 @@ USAGE="list []
or: $dashless branch  []
or: $dashless [save [--patch] [-k|--[no-]keep-index] [-q|--quiet]
   [-u|--include-untracked] [-a|--all] []]
+   or: $dashless push [--patch] [-k|--[no-]keep-index] [-q|--quiet]
+ [-u|--include-untracked] [-a|--all] [-m ]
or: $dashless clear"
 
 SUBDIRECTORY_OK=Yes
@@ -189,10 +191,11 @@ store_stash () {
return $ret
 }
 
-save_stash () {
+push_stash () {
keep_index=
patch_mode=
untracked=
+   stash_msg=
while test $# != 0
do
case "$1" in
@@ -216,6 +219,11 @@ save_stash () {
-a|--all)
untracked=all
;;
+   -m|--message)
+   shift
+   test -z ${1+x} && usage
+   stash_msg=$1
+   ;;
--help)
show_help
;;
@@ -251,8 +259,6 @@ save_stash () {
die "$(gettext "Can't use --patch and --include-untracked or 
--all at the same time")"
fi
 
-   stash_msg="$*"
-
git update-index -q --refresh
if no_changes
then
@@ -291,6 +297,36 @@ save_stash () {
fi
 }
 
+save_stash () {
+   push_options=
+   while test $# != 0
+   do
+   case "$1" in
+   --)
+   shift
+   break
+   ;;
+   -*)
+   # pass all options through to push_stash
+   push_options="$push_options $1"
+   ;;
+   *)
+   break
+   ;;
+   esac
+   shift
+   done
+
+   stash_msg="$*"
+
+   if test -z "$stash_msg"
+   then
+   push_stash $push_options
+   else
+   push_stash $push_options -m "$stash_msg"
+   fi
+}
+
 have_stash () {
git rev-parse --verify --quiet $ref_stash >/dev/null
 }
@@ -617,6 +653,10 @@ save)
shift
save_stash "$@"
;;
+push)
+   shift
+   push_stash "$@"
+   ;;
 apply)
shift
apply_stash "$@"
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 2de3e18ce6..3577115807 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -775,4 +775,13 @@ test_expect_success 'stash is not confused by partial 
renames' '
test_path_is_missing file
 '
 
+test_expect_success 'push -m shows right message' '
+   >foo &&
+   git add foo &&
+   git stash push -m "test message" &&
+   echo "stash@{0}: On master: test message" >expect &&
+   git stash list -1 >actual &&
+   test_cmp expect actual
+'
+
 test_done
-- 
2.11.0.301.g86e6ecc671.dirty

[PATCH v4 1/7] Documentation/stash: remove mention of git reset --hard

[Thomas Gummerer]

Don't mention git reset --hard in the documentation for git stash save.
It's an implementation detail that doesn't matter to the end user and
thus shouldn't be exposed to them.  In addition it's not quite true for
git stash -p, and will not be true when a filename argument to limit the
stash to a few files is introduced.

Signed-off-by: Thomas Gummerer 
---
 Documentation/git-stash.txt | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 2e9cef06e6..2e9e344cd7 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -47,8 +47,9 @@ OPTIONS
 
 save [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] 
[-q|--quiet] []::
 
-   Save your local modifications to a new 'stash', and run `git reset
-   --hard` to revert them.  The  part is optional and gives
+   Save your local modifications to a new 'stash' and roll them
+   back to HEAD (in the working tree and in the index).
+   The  part is optional and gives
the description along with the stashed state.  For quickly making
a snapshot, you can omit _both_ "save" and , but giving
only  does not trigger this action to prevent a misspelled
-- 
2.11.0.301.g86e6ecc671.dirty

Re: [git-gui] Amending doesn't preserve timestamp

[Juraj]

Hi Igor,

I forgot to write the version I'm using. It's on Ubuntu 16.04, git-gui
package version 1:2.7.4-0ubuntu1 (--version: git-gui version 0.20.0),
git version 2.7.4, tcl and tk 8.6.0+9. Perhaps it got fixed in a newer
version, in that case, my bad for not checking before posting.

Thanks,
Juraj

Re: [git-gui] Amending doesn't preserve timestamp

[Igor Djordjevic BugA]

On 12/02/2017 21:50, Juraj wrote:
> I've just noticed that amending a commit from git-gui uses the time of
> amending as the new timestamp of the commit, whereas git commit
> --amend preserves the original timestamp. Maybe the two should work
> the same, whatever it is decided to be the standard behavior.
> 
> Juraj
> 

Hi Juraj,

Just to report that it seems to work as expected on my end...? Amending
through both command line and git-gui preserves author date and updates
commiter date.

git version 2.11.1.windows.1
git-gui version 0.21.GITGUI
Tcl/Tk version 8.6.6

Regards, BugA

Re: [PATCH] fetch: print an error when declining to request an unadvertised object

[Matt McCutchen]

On Fri, 2017-02-10 at 10:36 -0800, Junio C Hamano wrote:
> > 
> There is this piece of code near the end of builtin/fetch-pack.c:
> 
> [...]
> 
> that happens before the command shows the list of fetched refs, and
> this code is prepared to inspect what happend to the requests it (in
> response to the user request) made to the underlying fetch
> machinery, and issue the error message.
> If you change your command line to "git fetch-pack REMOTE SHA1", you
> do see an error from the above.

Yes, "error: no such remote ref ", which at least makes clear that
the operation didn't work, though it would be nice to give a more
specific error message.

> This all happens in transport.c::fetch_refs_via_pack().
> I think that function is a much better place to error or die than
> filter_refs().

I confirmed that checking the sought refs there works.  However, in
filter_refs, it's easy to give a more specific error message that the
server doesn't allow requests for unadvertised objects, and that code
works for "git fetch-pack" too.  To do the same in fetch_refs_via_pack,
we'd have to duplicate a few lines of code from filter_refs and expose
the allow_unadvertised_object_request variable, or just set a flag on
the "struct ref" in filter_refs and check it in fetch_refs_via_pack.

What do you think?  Do you not care about having a more specific error,
in which case I can copy the code from builtin/fetch-pack.c to
fetch_refs_via_pack?  Or shall I add code to filter_refs to set a flag
and add code to builtin/fetch-pack.c and fetch_refs_via_pack to check
the flag?  Or what?

Matt

[git-gui] Amending doesn't preserve timestamp

[Juraj]

I've just noticed that amending a commit from git-gui uses the time of
amending as the new timestamp of the commit, whereas git commit
--amend preserves the original timestamp. Maybe the two should work
the same, whatever it is decided to be the standard behavior.

Juraj

Re: [PATCH v3 0/5] stash: support pathspec argument

[Thomas Gummerer]

[+cc Matthieu Moy as author of a patch mentioned below]

On 02/06, Jeff King wrote:
> On Sun, Feb 05, 2017 at 08:26:37PM +, Thomas Gummerer wrote:
> 
> > Thanks Junio for the review in the previous round.
> > 
> > Changes since v2:
> > 
> > - $IFS should now be supported by using "$@" everywhere instead of using
> >   a $files variable.
> > - Added a new patch showing the old behaviour of git stash create is
> >   preserved.
> > - Rephrased the documentation
> > - Simplified the option parsing in save_stash, by leaving the
> >   actual parsing to push_stash instead.
> 
> Overall, I like the direction this is heading. I raised a few issues,
> the most major of which is whether we want to allow the minor regression
> in "git stash create -m foo".
> 
> This also makes "git stash push -p " work, which is good. I
> don't think "git stash -p " does, though. I _think_ it
> would be trivial to do on top, since we already consider that case an
> error. That's somewhat outside the scope of your series, so I won't
> complain (too much :) ) if you don't dig into it, but it might just be
> trivial on top.

Hmm good idea, I think it would indeed be nice to add that.  Theres a
few things to consider though.  First, we need to switch git stash
without arguments over to use git stash push internally.  This does
introduce a slight regression as we currently allow git stash save --
-fmessage, (only messages starting with - are allowed).  I think that
regression would be acceptable however.

The other question is when we should allow the pathspec argument.
3c2eb80f, ("stash: simplify defaulting to "save" and reject unknown
options") made sure that all option arguments are treated the same.  I
think we could use the usual disambiguation of -- to allow pathspecs
after that, so stash -p wouldn't be treated specially, otherwise the
rules could get a bit confusing again.

The other question is how we would deal with the -m flag for
specifying a stash message.  I think we could special case it, but
that would allow users to do things such as git stash -m apply, which
would make the interface a bit more error prone.  So I'm leaning
towards disallowing that for now.

> A few other random bits I noticed while playing with the new code:
> 
>   $ git init
>   $ echo content >file && git add file && git commit -m file
>   $ echo change >file
> 
>   $ git stash push -p not-file
>   No changes.
>   No changes selected
> 
> Probably only one of those is necessary. :)
> 
> Let's keep trying a few things:
> 
>   $ git stash push not-file
>   Saved working directory and index state WIP on master: 5d5f951 file
>   Unstaged changes after reset:
>   M   file
>   M   file
> 
> The unstaged change is mentioned twice, which is weird. But weirder
> still is that we created a stash at all, as it's empty. Why didn't we
> hit the "no changes selected" path?
> 
> And one more:
> 
>   $ echo foo >untracked
>   $ git stash push untracked
>   Saved working directory and index state WIP on master: 5d5f951 file
>   Unstaged changes after reset:
>   M   file
>   M   file
>   Removing untracked
> 
> We removed the untracked file, even though it wasn't actually stashed! I
> thought at first this was because it was mentioned as a pathspec, but it
> seems to happen even with a different name:
> 
>   $ echo foo >untracked
>   $ git stash push does-not-exist
>   ...
>   Removing untracked
> 
> That seems dangerous.

Ouch, yeah this is clearly not good.  I'll fix this, and add tests for
these cases.

> -Peff

Re: [PATCH 1/2 v3] revision.c: args starting with "-" might be a revision

[Junio C Hamano]

Siddharth Kannan  writes:

> This "changing the order" gave me the idea to change the flow. I tried to
> implement the above steps without touching the function handle_revision_opt. 
> By
> inserting the handle_revision_arg call just before calling 
> handle_revision_opt.

Changing the order is changing the order of the function calls,
i.e. changing the flow.  So at the idea level we are on the same
page.

I was shooting for not having to duplicate calls to
handle_revision_arg().  

>> But I think the resulting code flow is much closer to the
>> above ideal.
>
> (about Junio's version of the patch): Yes, I agree with you on this. It's like
> the ideal, but the argv has already been populated, so the only remaining step
> is "left++".
>> 
>> Such a change to handle_revision_opt() unfortunately affects other
>> callers of the function, so it may not be worth it.

See the 3-patch series I just sent out.  I didn't think it through
very carefully (especially the error message the other caller
produces), but the whole thing _smells_ correct to me.

[PATCH 1/3] handle_revision_opt(): do not update argv[left++] with an unknown arg

[Junio C Hamano]

In future steps, we will make it possible for a rev or a revision
range (i.e. what is understood by handle_revision_arg() helper) to
begin with a dash.  The setup_revisions() function however currently
considers anything that begins with a dash to be:

 - an option it itself understands and handles (some take effect by
   setting fields in the revision structure, some others are left
   in the argv[left++] to be handled in later steps);
 - an option handle_revision_opt() understands and tells us to skip;
 - an option handle_revision_opt() found to be incorrect; or
 - an option handle_revision_opt() did not understand, which is
   stuffed in argv[left++].

and does not give a chance to handle_revision_arg() to inspect it.
The handle_revision_opt() function returns a positive count, a
negative count or zero to allow the caller to tell the latter three
cases apart.  A rev that begins with a dash would be thrown into the
last category.

Teach handle_revision_opt() not to touch argv[left++] in the last
case.  Because the other one among the two callers of this function
immediately errors out with the usage string when it returns zero
(i.e. the last case above), there is no negative effect to that
caller.

In setup_revisions(), which is the other caller of this function,
we need to stuff the unknown arg to argv[left++] in this case, to
preserve the current behaviour.

Signed-off-by: Junio C Hamano 
---
 revision.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/revision.c b/revision.c
index b37dbec378..4f46b8ba81 100644
--- a/revision.c
+++ b/revision.c
@@ -2016,8 +2016,6 @@ static int handle_revision_opt(struct rev_info *revs, int 
argc, const char **arg
revs->ignore_missing = 1;
} else {
int opts = diff_opt_parse(>diffopt, argv, argc, 
revs->prefix);
-   if (!opts)
-   unkv[(*unkc)++] = arg;
return opts;
}
if (revs->graph && revs->track_linear)
@@ -2234,6 +2232,8 @@ int setup_revisions(int argc, const char **argv, struct 
rev_info *revs, struct s
}
if (opts < 0)
exit(128);
+   /* arg looks like an opt but something we do not 
recognise. */
+   argv[left++] = arg;
continue;
}
 
-- 
2.12.0-rc1-212-ga9adfb24fa

[PATCH 3/3] setup_revisions(): allow a rev that begins with a dash

[Junio C Hamano]

Now all the preparatory pieces are in place, it is a matter of
handling a truly unknown option _after_ handle_revision_arg()
decides that arg is not a rev.  

Signed-off-by: Junio C Hamano 
---

 We _could_ do without a new variable maybe_opt and instead check if
 arg begins with a dash one more time, but it is cleaner to do it
 the way this patch does to avoid writing the same check twice.  We
 may be hit with a desire similar to but an opposite of the current
 topic (which wants to allow a rev that begins with a dash), to
 start allowing an option that does not begin with a dash someday.

 revision.c | 15 ---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/revision.c b/revision.c
index eccf1ab695..0f772ba73d 100644
--- a/revision.c
+++ b/revision.c
@@ -2203,6 +2203,8 @@ int setup_revisions(int argc, const char **argv, struct 
rev_info *revs, struct s
read_from_stdin = 0;
for (left = i = 1; i < argc; i++) {
const char *arg = argv[i];
+   int maybe_opt = 0;
+
if (*arg == '-') {
int opts;
 
@@ -2232,13 +2234,20 @@ int setup_revisions(int argc, const char **argv, struct 
rev_info *revs, struct s
}
if (opts < 0)
exit(128);
-   /* arg looks like an opt but something we do not 
recognise. */
-   argv[left++] = arg;
-   continue;
+   /*
+* arg looks like an opt but something we do not 
recognise.
+* It may be a rev that begins with a dash; fall 
through to
+* let handle_revision_arg() have a say in this.
+*/
+   maybe_opt = 1;
}
 
if (!handle_revision_arg(arg, revs, flags, revarg_opt)) {
got_rev_arg = 1;
+   } else if (maybe_opt) {
+   /* it turns out that it is not a rev after all */
+   argv[left++] = arg;
+   continue;
} else {
int j;
if (seen_dashdash || *arg == '^')
-- 
2.12.0-rc1-212-ga9adfb24fa

[PATCH 0/3] prepare for a rev/range that begins with a dash

[Junio C Hamano]

It turns out that telling handle_revision_opt() not to molest argv[left++]
does not have heavy fallout.

Junio C Hamano (3):
  handle_revision_opt(): do not update argv[left++] with an unknown arg
  setup_revisions(): swap if/else bodies to make the next step more readable
  setup_revisions(): allow a rev that begins with a dash

 revision.c | 22 +++---
 1 file changed, 15 insertions(+), 7 deletions(-)

-- 
2.12.0-rc1-212-ga9adfb24fa

[PATCH 2/3] setup_revisions(): swap if/else bodies to make the next step more readable

[Junio C Hamano]

Swap the condition and bodies of an "if (A) do_A else do_B" in
setup_revisions() to "if (!A) do_B else do A", to make the change in
the the next step easier to read.  

No behaviour change is intended in this step.

Signed-off-by: Junio C Hamano 
---
 revision.c | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/revision.c b/revision.c
index 4f46b8ba81..eccf1ab695 100644
--- a/revision.c
+++ b/revision.c
@@ -2237,8 +2237,9 @@ int setup_revisions(int argc, const char **argv, struct 
rev_info *revs, struct s
continue;
}
 
-
-   if (handle_revision_arg(arg, revs, flags, revarg_opt)) {
+   if (!handle_revision_arg(arg, revs, flags, revarg_opt)) {
+   got_rev_arg = 1;
+   } else {
int j;
if (seen_dashdash || *arg == '^')
die("bad revision '%s'", arg);
@@ -2255,8 +2256,6 @@ int setup_revisions(int argc, const char **argv, struct 
rev_info *revs, struct s
append_prune_data(_data, argv + i);
break;
}
-   else
-   got_rev_arg = 1;
}
 
if (prune_data.nr) {
-- 
2.12.0-rc1-212-ga9adfb24fa

Re: What's cooking in git.git (Feb 2017, #03; Fri, 10)

[Vegard Nossum]

On 11/02/2017 04:12, Junio C Hamano wrote:

René Scharfe  writes:


Am 10.02.2017 um 23:24 schrieb Junio C Hamano:

* vn/xdiff-func-context (2017-01-15) 1 commit
 - xdiff -W: relax end-of-file function detection

 "git diff -W" has been taught to handle the case where a new
 function is added at the end of the file better.

 Will hold.
 Discussion on an follow-up change to go back from the line that
 matches the funcline to show comments before the function
 definition has not resulted in an actionable conclusion.


This one is a bug fix and can be merged already IMHO.


Absolutely.  I was just waiting if the follow-up discussion would
easily and quickly lead to another patch, forgot about what exactly
I was waiting for (i.e. the gravity of not having the follow-up),
and have left it in "Will hold" status forever.


I said I would resubmit the patches with more config options and more
command-line arguments (to avoid potentially breaking backwards
compatibility), but IIRC the response seemed to be "preceding blank line
heuristic is good enough" and "why bother", so I ended up not not
resubmitting anything.



Let's merge it to 'next' and then decide if we want to also merge it
to 'master' before the final.  The above step alone is a lot less
contriversial and tricky bugfix.


Thanks,


Vegard

Re: Git status performance on PS (command prompt)

[brian m. carlson]

On Sun, Feb 12, 2017 at 04:53:47PM +0100, Mark Gaiser wrote:
> Hi,
> 
> I'm using ZSH with some fancy prompt. In that prompt is also a quick
> git status overview (some symbols indicating if the branch is up to
> date, if there are changes, etc..
> 
> The commands that are being executed to fetch the information:
> 
> For the file status:
> git status --porcelain
> 
> For the repository status:
> git status --porcelain -b

So your prompt is running git status twice?  Wouldn't it make more sense
to just run it once and do a head -n 1 to filter out the branch when you
need it?  git symbolic-ref HEAD might work as well.

> On small repositories (or even medium sized ones) this is fast, no
> problems there.
> But on larger repositories this is notably slow (i'm taking QtBase as
> an example now, but the same is true for much of the Qt repositories,
> or chromium or even the linux kernel itself). It's no problem if it's
> slow when you do "git status" on the command line. That can be
> expected to take a little while on large repositories. But in zsh
> prompts the call to git isn't asynchronous [1] so any slowdown will be
> noticed as the prompt simply doesn't completely show till after the
> command.
> 
> I did a bit of profiling in git to figure out where this slowdown comes from.
> Callgrind tells me that "read_directory_recursive" takes up ~62% of the time.
> Within that call the function "last_exclude_matching_from_list" takes
> up 49% of the time it takes to run "git status --porcelain -b".

This isn't entirely surprising.  When git status runs, it checks all the
files to see if the stat information is changed, and if so, updates the
status.  last_exclude_matching_from_list determines if the files are
ignored, which is also necessary to ensure that only the proper files
are listed.

That doesn't mean that there couldn't be some improvements there,
though.

> I don't really know how this code is supposed to work (i'm a git user,
> not a git developer), so i hope someone might be able to investigate
> this further. I can however apply patches to git locally and help out
> with testing.
> 
> Also, is there perhaps a command out there that might be better suited
> for the git status i want to show in the prompt? What i have now is
> merely from one of the oh-my-zsh themes.

I typically use the vcs_info support built into zsh[0], although that
can be very slow on my phone.  It performs adequately on most
repositories on my laptop and server machines, though, including on the
Linux kernel repo and our 2.4 and 9 GB repos at work.

I don't know that it has all the functionality that you need, as git
status provides finer-grained data, but it might.

[0] Example at 
https://github.com/bk2204/homedir/blob/master/.zsh/prompt_bmc_setup
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Confusing git messages when disk is full.

[Jáchym Barvínek]

Hello, I would like to report what I consider a bug in git, I hope I'm
doing it the right way.
I was trying to run `git pull` in  my repository and got the following
error: "git pull
Your configuration specifies to merge with the ref 'refs/heads/master'
from the remote, but no such ref was fetched."
Which was very confusing to me, I found some answers to what might be the cause
but none was the right one. The actual cause was that the filesystem
had no more free space.
When I cleaned the space, `git pull` then gave the expected answer
("Already up-to-date.").
I think the message is confusing and git should be able to report to
the user that the cause
is full disk.
Regards, Jáchym Barvínek

Git status performance on PS (command prompt)

[Mark Gaiser]

Hi,

I'm using ZSH with some fancy prompt. In that prompt is also a quick
git status overview (some symbols indicating if the branch is up to
date, if there are changes, etc..

The commands that are being executed to fetch the information:

For the file status:
git status --porcelain

For the repository status:
git status --porcelain -b

On small repositories (or even medium sized ones) this is fast, no
problems there.
But on larger repositories this is notably slow (i'm taking QtBase as
an example now, but the same is true for much of the Qt repositories,
or chromium or even the linux kernel itself). It's no problem if it's
slow when you do "git status" on the command line. That can be
expected to take a little while on large repositories. But in zsh
prompts the call to git isn't asynchronous [1] so any slowdown will be
noticed as the prompt simply doesn't completely show till after the
command.

I did a bit of profiling in git to figure out where this slowdown comes from.
Callgrind tells me that "read_directory_recursive" takes up ~62% of the time.
Within that call the function "last_exclude_matching_from_list" takes
up 49% of the time it takes to run "git status --porcelain -b".

I don't really know how this code is supposed to work (i'm a git user,
not a git developer), so i hope someone might be able to investigate
this further. I can however apply patches to git locally and help out
with testing.

Also, is there perhaps a command out there that might be better suited
for the git status i want to show in the prompt? What i have now is
merely from one of the oh-my-zsh themes.

Best regards,
Mark

p.s. please keep me in cc, i'm not subscribed to this list.

[1] Most prompts don't have async support, but there are prompts that
do provide this: https://github.com/sindresorhus/pure I guess that
would be a better solution for me in the short term. Still, having the
status being made faster would be beneficial.

Re: [PATCH 1/2 v3] revision.c: args starting with "-" might be a revision

[Siddharth Kannan]

On Sat, Feb 11, 2017 at 01:08:09PM -0800, Junio C Hamano wrote:
> Siddharth Kannan  writes:
> 
> > Um, I am sorry, but I feel that decrementing left, and incrementing it 
> > again is
> > also confusing.
> 
> Yes, but it is no more confusing than your original "left--".
> ...
> 
>  * If it is an option known to us, handle it and go to the next arg.
> 
>  * If it is an option that we do not understand, stuff it in
>argv[left++] and go to the next arg.
> 
> Because the second step currently is implemented by calling
> handle_opt(), which not just tells if it is an option we understand
> or not, but also mucks with argv[left++], you need to undo it once
> you start making it possible for a valid "rev" to begin with a dash.
> That is what your left-- was, and that is what "decrement and then
> increment when it turns out it was an unknown option after all" is.

So, our problem here is that the function handle_revision_opt is opaquely also
incrementing "left", which we need to decrement somehow.

Or: we could change the flow of the code so that this incrementing
will happen only when we have decided that the argument is not a
revision.
> 
>  * If it is an option known to us, handle it and go to the next arg.
> 
>  * If it is a rev, handle it, and note that fact in got_rev_arg
>(this change of order enables you to allow a rev that begins with
>a dash, which would have been misrecognised as a possible unknown
>option).
> 
>  * If it looks like an option (i.e. "begins with a dash"), then we
>already know that it is not something we understand, because the
>first step would have caught it already.  Stuff it in
>argv[left++] and go to the next arg.
> 
>  * If it is not a rev and we haven't seen dashdash, verify that it
>and everything that follows it are pathnames (which is an inexact
>but a cheap way to avoid ambiguity), make all them the prune_data
>and conclude.

This "changing the order" gave me the idea to change the flow. I tried to
implement the above steps without touching the function handle_revision_opt. By
inserting the handle_revision_arg call just before calling handle_revision_opt.

The decrementing then incrementing or "left--" things have now been removed.
(But there is still one thing which doesn't look good)

diff --git a/revision.c b/revision.c
index b37dbec..8c0acea 100644
--- a/revision.c
+++ b/revision.c
@@ -2203,11 +2203,11 @@ int setup_revisions(int argc, const char **argv, struct 
rev_info *revs, struct s
if (seen_dashdash)
revarg_opt |= REVARG_CANNOT_BE_FILENAME;
read_from_stdin = 0;
+
for (left = i = 1; i < argc; i++) {
const char *arg = argv[i];
+   int opts;
if (*arg == '-') {
-   int opts;
-
opts = handle_revision_pseudo_opt(submodule,
revs, argc - i, argv + i,
);
@@ -2226,7 +2226,11 @@ int setup_revisions(int argc, const char **argv, struct 
rev_info *revs, struct s
read_revisions_from_stdin(revs, _data);
continue;
}
+   }
 
+   if (!handle_revision_arg(arg, revs, flags, revarg_opt))
+   got_rev_arg = 1;
+   else if (*arg == '-') {
opts = handle_revision_opt(revs, argc - i, argv + i, 
, argv);
if (opts > 0) {
i += opts - 1;
@@ -2234,11 +2238,7 @@ int setup_revisions(int argc, const char **argv, struct 
rev_info *revs, struct s
}
if (opts < 0)
exit(128);
-   continue;
-   }
-
-
-   if (handle_revision_arg(arg, revs, flags, revarg_opt)) {
+   } else {
int j;
if (seen_dashdash || *arg == '^')
die("bad revision '%s'", arg);
@@ -2255,8 +2255,6 @@ int setup_revisions(int argc, const char **argv, struct 
rev_info *revs, struct s
append_prune_data(_data, argv + i);
break;
}
-   else
-   got_rev_arg = 1;
}
 
if (prune_data.nr) {


The "if (*arg =='-')" line is repeated. On analysing the resulting
revision.c:setup_revisions function, I feel that the codepath is still as
easily followable as it was earlier, and there is definitely no confusion
because of a mysterious decrement. Also, the repeated condition doesn't make it
any harder (it looks like a useful check because we already know that every
option would start with a "-"). But that's only my opinion, and you definitely
know better.

now the flow is very close to the ideal flow that we prefer:

1. If it is a 

Bug in 'git describe' if I have two tags on the same commit.

[Istvan Pato]

I didn't get back the latest tag by 'git describe --tags --always' if
I have two tags on the same commit.

// repository ppa:git-core/ppa

(master)⚡ % cat /etc/lsb-release
DISTRIB_ID=Ubuntu
DISTRIB_RELEASE=16.04
DISTRIB_CODENAME=xenial
DISTRIB_DESCRIPTION="Ubuntu 16.04.2 LTS"

(master)⚡ % git --version
git version 2.11.0

(master) [1] % git show-ref --tag
76c634390... refs/tags/1.0.0
b77c7cd17... refs/tags/1.1.0
b77c7cd17... refs/tags/1.2.0

(master) % git describe --tags --always
1.1.0-1-ge9e9ced

### Expected: 1.2.0

References:

https://www.kernel.org/pub/software/scm/git/docs/RelNotes-1.7.1.1.txt

* "git describe" did not tie-break tags that point at the same commit
  correctly; newer ones are preferred by paying attention to the
  tagger date now.

http://stackoverflow.com/questions/8089002/git-describe-with-two-tags-on-the-same-commit

Thanks,
Istvan Pato

Re: [PATCH 2/2 v3] sha1_name: teach get_sha1_1 "-" shorthand for "@{-1}"

[Siddharth Kannan]

Hey Matthieu,
On Sun, Feb 12, 2017 at 10:48:56AM +0100, Matthieu Moy wrote:
> Siddharth Kannan  writes:
> 
> >  sha1_name.c  |  5 
> >  t/t4214-log-shorthand.sh | 73 
> > 
> >  2 files changed, 78 insertions(+)
> >  create mode 100755 t/t4214-log-shorthand.sh
> >
> > diff --git a/sha1_name.c b/sha1_name.c
> > index 73a915f..d774e46 100644
> > --- a/sha1_name.c
> > +++ b/sha1_name.c
> > @@ -947,6 +947,11 @@ static int get_sha1_1(const char *name, int len, 
> > unsigned char *sha1, unsigned l
> > if (!ret)
> > return 0;
> >  
> > +   if (!strcmp(name, "-")) {
> > +   name = "@{-1}";
> > +   len = 5;
> > +   }
> 
> After you do that, the existing "turn - into @{-1}" pieces of code
> become useless and you should remove it (probably in a further patch).

Yeah, this is currently also implemented in checkout, apart from the
grepped list that you have supplied here. I will find all the
instances, and ensure that they work, and remove them. (This will
require some more digging into the codepath the commands, to ensure
that get_sha1_1 is called somewhere down the line)
> 
> > diff --git a/t/t4214-log-shorthand.sh b/t/t4214-log-shorthand.sh
> > ...
> > +test_expect_success 'setup' '
> > +   echo hello >world &&
> > +   git add world &&
> > +   git commit -m initial &&
> > +   echo "hello second time" >>world &&
> > ...
> 
> You may use test_commit to save a few lines of code.

Oh, yeah! I will use that. I need to work on improving the tests, as
well as adding the documentation.
> 
> > +test_expect_success 'symmetric revision range should work when one end is 
> > left empty' '
> > +   git checkout testing-2 &&
> > +   git checkout master &&
> > +   git log ...@{-1} > expect.first_empty &&
> > +   git log @{-1}... > expect.last_empty &&
> > +   git log ...- > actual.first_empty &&
> > +   git log -... > actual.last_empty &&
> 
> Nitpick: we stick the > and the filename (as you did in most places
> already).
Sorry, slipped my mind!
> 
> It may be worth adding tests for more cases like
> 
> * Check what happens with suffixes, i.e. -^, -@{yesterday} and -~.

These do not work right now. The first and last cases here are handled
by peel_onion, if I remember correctly. I have to find out why exactly
these are not working. Thanks for mentioning this!

> 
> * -..- -> to make sure you handle the presence of two - properly.
> 
> * multiple separate arguments to make sure you handle them all, e.g.
>   "git log - -", "git log HEAD -", "git log - HEAD".

Yeah, will add these tests.

> 
> The last two may be overkill, but the first one is probably important.
> 
> -- 
> Matthieu Moy
> http://www-verimag.imag.fr/~moy/

--
Regards,

Siddharth Kannan.

Re: [PATCH 2/2 v3] sha1_name: teach get_sha1_1 "-" shorthand for "@{-1}"

[Matthieu Moy]

Siddharth Kannan  writes:

>  sha1_name.c  |  5 
>  t/t4214-log-shorthand.sh | 73 
> 
>  2 files changed, 78 insertions(+)
>  create mode 100755 t/t4214-log-shorthand.sh
>
> diff --git a/sha1_name.c b/sha1_name.c
> index 73a915f..d774e46 100644
> --- a/sha1_name.c
> +++ b/sha1_name.c
> @@ -947,6 +947,11 @@ static int get_sha1_1(const char *name, int len, 
> unsigned char *sha1, unsigned l
>   if (!ret)
>   return 0;
>  
> + if (!strcmp(name, "-")) {
> + name = "@{-1}";
> + len = 5;
> + }

One drawback of this approach is that further error messages will be
given from the "@{-1}" string that the user never typed.

After you do that, the existing "turn - into @{-1}" pieces of code
become useless and you should remove it (probably in a further patch).

There are at least:

$ git grep -n -A1 'strcmp.*"-"' | grep -B 1 '@\{1\}'
builtin/checkout.c:975: if (!strcmp(arg, "-"))
builtin/checkout.c-976- arg = "@{-1}";
--
builtin/merge.c:1231:   } else if (argc == 1 && !strcmp(argv[0], "-")) {
builtin/merge.c-1232-   argv[0] = "@{-1}";
--
builtin/revert.c:158:   if (!strcmp(argv[1], "-"))
builtin/revert.c-159-   argv[1] = "@{-1}";
--
builtin/worktree.c:344: if (!strcmp(branch, "-"))
builtin/worktree.c-345- branch = "@{-1}";

In the final version, obviously the same "refactoring" (specific
command -> git-wide) should be done for documentation (it should be in
this patch to avoid letting not-up-to-date documentation even for a
single commit).

> diff --git a/t/t4214-log-shorthand.sh b/t/t4214-log-shorthand.sh
> new file mode 100755
> index 000..dec966c
> --- /dev/null
> +++ b/t/t4214-log-shorthand.sh
> @@ -0,0 +1,73 @@
> +#!/bin/sh
> +
> +test_description='log can show previous branch using shorthand - for @{-1}'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'setup' '
> + echo hello >world &&
> + git add world &&
> + git commit -m initial &&
> + echo "hello second time" >>world &&
> + git add world &&
> + git commit -m second &&
> + echo "hello other file" >>planet &&
> + git add planet &&
> + git commit -m third &&
> + echo "hello yet another file" >>city &&
> + git add city &&
> + git commit -m fourth
> +'

You may use test_commit to save a few lines of code.

> +test_expect_success '"log -" should work' '
> + git checkout -b testing-1 master^ &&
> + git checkout -b testing-2 master~2 &&
> + git checkout master &&
> +
> + git log testing-2 >expect &&
> + git log - >actual &&
> + test_cmp expect actual
> +'

I'd have split this into a "setup branches" and a '"log -" should work'
test (to actually see where "setup branches" happen in the output, and
to allow running the setup step separately if needed). Not terribly
important.

> +test_expect_success 'symmetric revision range should work when one end is 
> left empty' '
> + git checkout testing-2 &&
> + git checkout master &&
> + git log ...@{-1} > expect.first_empty &&
> + git log @{-1}... > expect.last_empty &&
> + git log ...- > actual.first_empty &&
> + git log -... > actual.last_empty &&

Nitpick: we stick the > and the filename (as you did in most places
already).

It may be worth adding tests for more cases like

* Check what happens with suffixes, i.e. -^, -@{yesterday} and -~.

* -..- -> to make sure you handle the presence of two - properly.

* multiple separate arguments to make sure you handle them all, e.g.
  "git log - -", "git log HEAD -", "git log - HEAD".

The last two may be overkill, but the first one is probably important.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/