Re: [PATCH] doc: filter-branch does not require re-export of vars

[Jeff King]

On Mon, May 29, 2017 at 01:27:45PM +0900, Junio C Hamano wrote:

> Samuel Lijin  writes:
> 
> >> However, I think POSIX mandates the behavior you'd expect. And the only
> >> shell I know that misbehaves in this way is Solaris /bin/sh, which we
> >> have already declared too broken to support.
> >
> > Off-topic, but where is this explicitly documented?
> 
> One link I had readily available was
> 
>   https://public-inbox.org/git/7vei5qtnc5@alter.siamese.dyndns.org/
> 
> but there may be older discussions that were the actual process of
> our officially having "written its /bin/sh off as broken and
> unusable" if you dig further in the list archive.

Ah, I had taken Samuel's question as "where is the POSIX behavior
documented". :)

The link above is a good explanation of the $() problem on Solaris. It
also doesn't do ${parameter#word}. I couldn't find a specific moment of
outlawing that /bin/sh, but there are several mentions of problems with
it going back to the beginning. Here's one from 2007:

  http://public-inbox.org/git/7vabt9sasl@assigned-by-dhcp.cox.net/

We had already given up on it by then.

I don't think there's anything in CodingGuidelines (though t/README does
call it "broken"), though it explicitly endorses $(), which rules out
Solaris /bin/sh.

I think we found in the last year or two that there are some old
versions of ksh that don't like our scripts (definitely ksh88, and maybe
ksh93?). Some light reading for the curious:

  
http://public-inbox.org/git/calr6jehvik9kzxr6r6xzkz5eao-rjwj3xyah_dosdxhejys...@mail.gmail.com/

  
http://public-inbox.org/git/calr6jejwjja0x2qxsxqobqc_yxrgx87lyf8cmj0mmjff6pk...@mail.gmail.com/

  http://public-inbox.org/git/xmqqinxt3kwq@gitster.mtv.corp.google.com/

The problems are around backslash-escaping, signal exit codes, and
doubled parentheses. There are some patches, but I think we decided not
to pursue it (I couldn't find that exact decision, but it's referenced
later on).

-Peff

Re: [PATCH] doc: filter-branch does not require re-export of vars

[Junio C Hamano]

Samuel Lijin  writes:

>> However, I think POSIX mandates the behavior you'd expect. And the only
>> shell I know that misbehaves in this way is Solaris /bin/sh, which we
>> have already declared too broken to support.
>
> Off-topic, but where is this explicitly documented?

One link I had readily available was

  https://public-inbox.org/git/7vei5qtnc5@alter.siamese.dyndns.org/

but there may be older discussions that were the actual process of
our officially having "written its /bin/sh off as broken and
unusable" if you dig further in the list archive.

Re: git push recurse.submodules behavior changed in 2.13

[Stefan Beller]

On Sun, May 28, 2017 at 7:44 PM, Junio C Hamano  wrote:
> John Shahid  writes:
>
>> It looks like the git push recurse-submodules behavior has changed.
>> Currently with 2.13 you cannot run "git push
>> --recurse-submodules=on-demand" if the parent repo is on a different
>> branch than the sub repos, e.g. parent repo is on "develop" and
>> sub-repo on "master". I created a test that can be found here [1].
>>
>> A bisect shows that the change to propagate refspec [2] to the
>> submodules is the culprit. imho this is an undesired change in
>> behavior. I looked at the code but couldn't see an easy way to fix
>> this issue without breaking the feature mentioned above. The only
>> option I can think of is to control the refspec propagation behavior
>> using a flag, e.g. "--propagate-refspecs" or add another
>> recurse-submodules option, e.g. "--recurse-submodules=propagate"
>>
>> What do you all think ?
>>
>> [1] https://gist.github.com/jvshahid/b778702cc3d825c6887d2707e866a9c8
>> [2] 
>> https://github.com/git/git/commit/06bf4ad1db92c32af38e16d9b7f928edbd647780
>
> Brandon?  I cannot quite tell from the report what "has changed"
> refers to, what failures "you cannot run" gets, and if that is a
> desirable thing to do (i.e. if letting the user run it in such a
> configuration would somehow break things, actively erroring out may
> be a deliberate change) or not (i.e. an unintended regression).
>

Before the refspec was passed down into the submodules,
we'd just invoke "git push" in the submodule assuming the user
setup a remote tracking branch and a push strategy such that
"git push" would do the right thing.
And because the submodule is configured independently, it
doesn't matter which branch you're on in the superproject.

Looking at the test[1], you run "git push --recurse-submodules"
without any remote/branch that was called out in the commit
message[2] to not have changed. Is that understanding correct?

Looking at the test cases of [2] we did not test for explicit
"still works with no args given", though one could have expected
we'd have a test for that already. :/

Thanks,
Stefan

Re: [PATCH] doc: filter-branch does not require re-export of vars

[Jeff King]

On Sun, May 28, 2017 at 09:35:30PM -0400, Samuel Lijin wrote:

> > However, I think POSIX mandates the behavior you'd expect. And the only
> > shell I know that misbehaves in this way is Solaris /bin/sh, which we
> > have already declared too broken to support.
> 
> Off-topic, but where is this explicitly documented?

I couldn't find a place that mentioned it explicitly, but POSIX defines
the concept as "the export attribute" of the variables. Which implies to
me that the bit is tied to the variable itself, not its value.

It also says that the flag is cleared when a variable is unset, so:

  foo=one
  export foo
  sh -c 'echo $foo should be one'
  unset foo
  foo=two
  sh -c 'echo $foo is not exported'

That could potentially affect somebody writing a filter-branch snippet,
but presumably if they are using "unset" they know what they're doing.

-Peff

Re: 2.13.0-rc1 is broken on SPARC

[Junio C Hamano]

"Liam R. Howlett"  writes:

> My SPARC build does not function and seg bus terminates on any command.

Sorry, known issue in the released version 2.13 (we would have
appreciated if a bug report for -rc1 came way before the decision to
tag the final release).

A fix exists on 'pu' (you can merge or cherry-pick a0103914c2); it
was unfortunately held up with an unrelated enthusiasm around an
attempt to use submodule to manage this codebase in our project.

I'll try to untangle the fix proper and quickly merge down to the
maintenance track.

Thanks for a report.

Respond as soon as you receive this massage

[Telex office]

#104 Lambert avenue 144/150 Adoglita
Cotonou Republic Du Benin.
Tel: 229. 5654.56 77

Attention:Sir/Madam

Your overdue payment sum of $3,500,000.00 USD has been released today from 
Federal High Court Benin and we are hereby to let you know that the first 
payment of $5000 with the MTCN: (156-673-) have been transferred to your 
Name for you to pick up but you have to pay the sum of $79 usd to activate it 
before you can pick up the $5000 .contact now so that we can give you the 
Receivers name to send the activation fee and collect your $5000 immediately 
within 45 minutes. Again we give you 12hrs to respond and after 12hrs you did 
not respond we can cancel your payment file and return the total fund to 
Government Treasury Account kindly contact Rev BAR C.F Wilson for your payment,

Contact information:
E-mail: un...@fastservice.com
Name: Rev BAR C.F Wilson ,
Tele: +229 6277 1872

Respond as soon as you receive this massage we are waiting,

Regard

MR.RICHARD MARK,
Director Federal Ministry Of,
Finance Benin Republic
Federal Ministry Of Finance Benin Republic

Re: git push recurse.submodules behavior changed in 2.13

[Junio C Hamano]

John Shahid  writes:

> It looks like the git push recurse-submodules behavior has changed.
> Currently with 2.13 you cannot run "git push
> --recurse-submodules=on-demand" if the parent repo is on a different
> branch than the sub repos, e.g. parent repo is on "develop" and
> sub-repo on "master". I created a test that can be found here [1].
>
> A bisect shows that the change to propagate refspec [2] to the
> submodules is the culprit. imho this is an undesired change in
> behavior. I looked at the code but couldn't see an easy way to fix
> this issue without breaking the feature mentioned above. The only
> option I can think of is to control the refspec propagation behavior
> using a flag, e.g. "--propagate-refspecs" or add another
> recurse-submodules option, e.g. "--recurse-submodules=propagate"
>
> What do you all think ?
>
> [1] https://gist.github.com/jvshahid/b778702cc3d825c6887d2707e866a9c8
> [2] https://github.com/git/git/commit/06bf4ad1db92c32af38e16d9b7f928edbd647780

Brandon?  I cannot quite tell from the report what "has changed"
refers to, what failures "you cannot run" gets, and if that is a
desirable thing to do (i.e. if letting the user run it in such a
configuration would somehow break things, actively erroring out may
be a deliberate change) or not (i.e. an unintended regression).

2.13.0-rc1 is broken on SPARC

[Liam R. Howlett]

Hello,

My SPARC build does not function and seg bus terminates on any command.
I have worked out the commit is the one that switched the default SHA1
code to sha1dc - commit e6b07da2780f349c29809bd75d3eca6ad3c35d19.  I
managed to get it working by following the instructions on updating the
library in commit 28dc98e343ca4eb370a29ceec4c19beac9b5c01e.

Should I generate & send a patch for this?  I would normally do just
that, but with an external library, the resulting patch required
conflict resolution on the merging so format-patch won't work with the
given instructions.

I should note that following the build instructions in commit
e6b07da2780f349c29809bd75d3eca6ad3c35d19 to switch back to openssl's
SHA1 implementation works fine.

Thanks,
Liam

Re: [PATCH] pull: ff --rebase --autostash works in dirty repo

[Junio C Hamano]

Tyler Brazier  writes:

> pull --rebase --autostash was failing on a fast-forward in a dirty repo
> since we shortcut to run_merge(), which does not know how to autostash.
> The shortcut is a performance optimization, and since rebase was
> rewritten in C, it seemed okay to just bypass the shortcut if we
> autostash.
> ---

Please clarify "was failing".  I suspect that

When we can fast-forward to the updated upstream, "git pull
--rebase --autostash" in a dirty repository did not auto-stash
and failed.  This was due to a short-cut to avoid running rebase
when we can fast-forward, but the autostash option was ignored
in that codepath.

or something like that was what you meant.

"rebase" was not rewritten in C.  Not the part that matters in
making "pull --rebase" work anyway.  Unlike the one in the earlier
discussion, you are not removing the short-cut unconditionally; I do
not think you need to justify the "bypassing" based on performance.
If we need to take run_rebase() codepath when "--autostash" is in
effect, we need to do so even if the result were somewhat slower for
correctness (and it would not hurt to mention that actual measurement
showed that it is dubious it is slower in the first place).

Missing sign-off.

>  builtin/pull.c  |  5 +++--
>  t/t5520-pull.sh | 18 ++
>  2 files changed, 21 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/pull.c b/builtin/pull.c
> index dd1a4a94e41e..609e594d3f28 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -772,6 +772,7 @@ int cmd_pull(int argc, const char **argv, const char 
> *prefix)
>   struct oid_array merge_heads = OID_ARRAY_INIT;
>   struct object_id orig_head, curr_head;
>   struct object_id rebase_fork_point;
> + int autostash;
>  
>   if (!getenv("GIT_REFLOG_ACTION"))
>   set_reflog_message(argc, argv);
> @@ -800,8 +801,8 @@ int cmd_pull(int argc, const char **argv, const char 
> *prefix)
>   if (!opt_rebase && opt_autostash != -1)
>   die(_("--[no-]autostash option is only valid with --rebase."));
>  
> + autostash = config_autostash;
>   if (opt_rebase) {
> - int autostash = config_autostash;
>   if (opt_autostash != -1)
>   autostash = opt_autostash;
>  
> @@ -868,7 +869,7 @@ int cmd_pull(int argc, const char **argv, const char 
> *prefix)
>   head = lookup_commit_reference(orig_head.hash);
>   commit_list_insert(head, );
>   merge_head = lookup_commit_reference(merge_heads.oid[0].hash);
> - if (is_descendant_of(merge_head, list)) {
> + if (!autostash && is_descendant_of(merge_head, list)) {
>   /* we can fast-forward this without invoking rebase */
>   opt_ff = "--ff-only";
>   return run_merge();

The scope of the "autostash" feels a bit unfortunate, but that is a
direct consequence of having two "if (opt_rebase)" separated in the
control flow, so it's not your fault.

When autostsash is in effect, you _know_ you do not have to compute
is-descendant-of and you do not have to prepare merge_head or list
here.  I do not like deeply indented code, but perhaps like this one
on top of your patch?

 builtin/pull.c | 22 --
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index 609e594d3f..42f0560252 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -863,16 +863,18 @@ int cmd_pull(int argc, const char **argv, const char 
*prefix)
die(_("Cannot rebase onto multiple branches."));
 
if (opt_rebase) {
-   struct commit_list *list = NULL;
-   struct commit *merge_head, *head;
-
-   head = lookup_commit_reference(orig_head.hash);
-   commit_list_insert(head, );
-   merge_head = lookup_commit_reference(merge_heads.oid[0].hash);
-   if (!autostash && is_descendant_of(merge_head, list)) {
-   /* we can fast-forward this without invoking rebase */
-   opt_ff = "--ff-only";
-   return run_merge();
+   if (!autostash) {
+   struct commit_list *list = NULL;
+   struct commit *merge_head, *head;
+
+   head = lookup_commit_reference(orig_head.hash);
+   commit_list_insert(head, );
+   merge_head = 
lookup_commit_reference(merge_heads.oid[0].hash);
+   if (is_descendant_of(merge_head, list)) {
+   /* we can fast-forward this without invoking 
rebase */
+   opt_ff = "--ff-only";
+   return run_merge();
+   }
}
return run_rebase(_head, merge_heads.oid, 
_fork_point);
} else {



> diff 

Re: [PATCH/RFC] branch: add tests for new copy branch feature

[Junio C Hamano]

Sahil Dua  writes:

> New feature - copying a branch along with its config section.

That's an unusual non-sentence (without a verb) in our commit message.

> Aim is to have an option -c for copying a branch just like -m option for
> renaming a branch.

What should it copy literally, and what should it copy with
adjustment (and adjusting what), and what should it refrain from
copying?  

For example, when you create a branch topic-2 by copying from a
branch topic-1, do you copy the reflog in such a way that topic-2's
reflog contains all the entries of topic-1 before the copy happens,
capped with a new (and not shared with topic-1) entry that says
"Copied from topic-1"?  When topic-1 is set to pull from a custom
upstream 'upstream' (i.e. not "origin")'s 'trunk' branch, by setting
'branch.topic-2.remote' to "upstream", i.e. the same as the value of
'branch.topic-1.remote' and 'branch.topic-2.merge' to "trunk",
i.e. the same as 'branch.topic-1.merge'?  Should a "git push" while
topic-2 is checked out push to the same remote as a "git push" would
push to when topic-1 is checked out?  Should they update the same
branch at the remote?  Why and/or why not? [*1*]

> This commit adds a few basic tests for getting any suggestions/feedback
> about expected behavior for this new feature.

Writing tests is a good way to prepare for an implementation, which
must be done according to the design, but that happens after the
design is polished and judged to be a good one.  While soliciting
comments on the design from others, tests are a bit too low level to
express what you are aiming at.  It is a bit unhelpful to those who
may want to help to figure out answers to questions like the ones in
the previos paragraph (the one that begins with "For example,...")
by telling them to "read my intention from these tests", and when
you want as wide an input as possible, that is counterproductive for
your objective ;-).

> Signed-off-by: Sahil Dua 
> ---
>  t/t3200-branch.sh | 53 +
>  1 file changed, 53 insertions(+)
>
> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
> index fe62e7c775da..2c95ed6ebf3c 100755
> --- a/t/t3200-branch.sh
> +++ b/t/t3200-branch.sh
> @@ -341,6 +341,59 @@ test_expect_success 'config information was renamed, 
> too' '
>   test_must_fail git config branch.s/s/dummy
>  '
>  
> +test_expect_success 'git branch -c dumps usage' '
> + test_expect_code 128 git branch -c 2>err &&
> + test_i18ngrep "branch name required" err
> +'

OK.  Do we want to support a single-name format (i.e. copy from the
current)?

> +git config branch.d.dummy Hello

Write your tests to do as little outside test_expct_success block as
possible, and do a set-up close to where it matters.

> +test_expect_success 'git branch -c d e should work' '
> + git branch -l d &&
> + git reflog exists refs/heads/d &&
> + git branch -c d e &&
> + git reflog exists refs/heads/d &&
> + git reflog exists refs/heads/e
> +'

A reasonable design of "copy" is for d's log to be left intact, and
e's log to begin with a single entry "created by copying from d".
Another possible design is to copy the log (making them identical),
and then add one entry to e's log "created by copying from d".

The above test would succeed with either implementation and does not
answer "should reflog be copied?" (see above about why "here are the
tests that shows my thinking--read them and comment" is a bad
approach).

"move" that makes the source of the movement disappear after the
operation has a stronger justification of moving the log under the
new name (the only alternative is to lose the history of the source
forever), it is debatable if "copy" wants to retain a copy of the
history of an otherwise unrelated branch, which has its own history
retained after the operation.

> +
> +test_expect_success 'config information was copied, too' '
> + test $(git config branch.e.dummy) = Hello &&
> + test $(git config branch.d.dummy) = Hello
> +'

The expectation is that a configuration like 'dummy' that does not
have any meaning to Git itself will all blindly be copied, which is
similar to what "move" does.

> +git config branch.f/f.dummy Hello
> +
> +test_expect_success 'git branch -c f/f g/g should work' '
> + git branch -l f/f &&
> + git reflog exists refs/heads/f/f &&
> + git branch -c f/f g/g &&
> + git reflog exists refs/heads/f/f &&
> + git reflog exists refs/heads/g/g
> +'

Hmph.  What's the reason to expect this not to work?

> +test_expect_success 'config information was copied, too' '
> + test $(git config branch.f/f.dummy) = Hello &&
> + test $(git config branch.g/g.dummy) = Hello
> +'

Isn't this part of the "should work" test above?  The definition of
working is not just reflog is created for the new branch without the
old branch losing its reflog, right?

> +test_expect_success 'git branch -c m2 m2 should 

Re: [PATCH] doc: filter-branch does not require re-export of vars

[Samuel Lijin]

On Fri, May 26, 2017 at 2:37 PM, Jeff King  wrote:
>
> On Fri, May 26, 2017 at 07:36:54PM +0200, Andreas Heiduk wrote:
>
> > The function `set_ident` in `filter-branch` exported the variables
> > GIT_(AUTHOR|COMMITTER)_(NAME|EMAIL|DATE) at least since 6f6826c52b in 2007.
> > Therefore the filter scripts don't need to re-eport them again.
>
> Some old shells keep separate values for the internal and exporter
> versions of variables. I.e., this:
>
>   foo=one
>   export foo
>   foo=two
>
> would continue to export $foo as "one", even though it is "two" inside
> the script.
>
> However, I think POSIX mandates the behavior you'd expect. And the only
> shell I know that misbehaves in this way is Solaris /bin/sh, which we
> have already declared too broken to support.

Off-topic, but where is this explicitly documented?

> According to
>
>   
> https://www.gnu.org/savannah-checkouts/gnu/autoconf/manual/autoconf-2.69/html_node/Limitations-of-Builtins.html#export
>
> it sounds like there are some other antique shells which may do the
> same (it doesn't cover this case explicitly, but the "coexist" cases it
> mentions are likely to behave in this way).
>
> At this point, I'd be inclined to remove the text as you suggest and
> either make a small note at the bottom of the page, or just omit it
> entirely and assume that anybody on an old non-POSIX shell can fend for
> themselves.
>
> -Peff

Re: [PATCH] branch test: fix invalid config key access

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  writes:

> On Sun, May 28, 2017 at 7:12 PM, Sahil Dua  wrote:
>> Fixes the test by changing "branch.s/s/dummy" to "branch.s/s.dummy" which is
>> the right way of accessing config key "branch.s/s.dummy". Purpose of
>> this test is to confirm that this key doesn't exist after the branch
>> "s/s" has been renamed to "s".
>>
>> Earlier it was trying to access invalid config key and hence was getting
>> an error. However, this wasn't caught because we were expecting the
>> command to fail for other reason as mentioned above.
>
> Looks obviously correct to me. Added by Johannes in commit dc81c58cd6
> ("git-branch: rename config vars branch..*, too", 2006-12-16),
> CC-ing him in case he has anything to say about it, but just looks
> like a typo back in 2006.

Yeah, it does look like a simple typo.  Sorry for not spotting it
back then.


>> Signed-off-by: Sahil Dua 
>> ---
>>  t/t3200-branch.sh | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
>> index fe62e7c..10f8f02 100755
>> --- a/t/t3200-branch.sh
>> +++ b/t/t3200-branch.sh
>> @@ -338,7 +338,7 @@ test_expect_success 'git branch -m s/s s should work 
>> when s/t is deleted' '
>>
>>  test_expect_success 'config information was renamed, too' '
>> test $(git config branch.s.dummy) = Hello &&
>> -   test_must_fail git config branch.s/s/dummy
>> +   test_must_fail git config branch.s/s.dummy
>>  '
>>
>>  test_expect_success 'deleting a symref' '
>> --
>> 2.7.4 (Apple Git-66)
>>

Re: [RFC/PATCH] WIP: add deprecation & experimental process/interface

[Junio C Hamano]

Ævar Arnfjörð Bjarmason   writes:

> diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN
> index 4f94fc7574..c76bbedf86 100755
> --- a/GIT-VERSION-GEN
> +++ b/GIT-VERSION-GEN
> @@ -37,4 +37,5 @@ fi
>  test "$VN" = "$VC" || {
>   echo >&2 "GIT_VERSION = $VN"
>   echo "GIT_VERSION = $VN" >$GVF
> + echo "GIT_VERSION_INT = $(echo $VN | sed -e 
> 's/^\([0-9]*\)\.\([0-9]*\)\..*/\1\2/')" >>$GVF
>  }

Unlike Perl's v1.2.3.4 notation, this forces us worry when we go
from v2.99.0 to v2.100.0 and eventually to v3.0, no?

> + } else if (1) {
> + /*
> +  * TODO: Instead of `if 1` we should check a
> +  * core.version variable here.
> +  *
> +  * I.e. if set to core.version=2.13 the user is opting
> +  * in to get deprecations set at dep_at right away,
> +  * and also perhaps experimental features from a
> +  * sister experimental() interface.
> +  */

This essentially forces us to always read _some_ configuration.
Some commands are meant to work outside repositories, so those who
want to affect them needs to write core.version in their global
configuration.  Some low-level plumbing commands may want to do
absolute minimum without configurablity.

I am not saying that it is absolutely a bad design decision to force
us to read some configuration (yet); it's just that it is something
that we have to keep in mind and always think about the
ramifications of.

> + die(_("Early bird deprecation error: %s"), message);
> + }
> +}

Re: [PATCH] change 'commands' to comments and improve wording

[Junio C Hamano]

Adrian  writes: (nothing)

Imagine a reader who finds the title of this commit in 3 weeks from
now among 200 other commits.  Do you think the reader can guess that
this is a documentation fix?  Can the reader tell this is about
"stash"?

Subject: stash doc: write explanation as comments, not as commands

or something, perhaps.  This commit is simple enough that it may not
need the body but explaining why would not hurt.  Perhaps like this:


In the illustration of workflows in "git stash" documentation,
where it shows sequence of 'git' commands, there are a few steps
that are not literal commands (i.e. "here, you would use your
editor").  Clarify that these are not something the user can
blindly cut-and-paste by turning them into comments.

would be clear enough.

> ---

And please sign-off (Documentation/SubmittingPatches) your patch.

Thanks.


>  Documentation/git-stash.txt | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
> index 70191d06b69e..3899d36b2bab 100644
> --- a/Documentation/git-stash.txt
> +++ b/Documentation/git-stash.txt
> @@ -233,7 +233,7 @@ return to your original branch to make the emergency fix, 
> like this:
>  $ git checkout -b my_wip
>  $ git commit -a -m "WIP"
>  $ git checkout master
> -$ edit emergency fix
> +# ... edit emergency fix ...
>  $ git commit -a -m "Fix in a hurry"
>  $ git checkout my_wip
>  $ git reset --soft HEAD^
> @@ -245,7 +245,7 @@ You can use 'git stash' to simplify the above, like this:
>  
>  # ... hack hack hack ...
>  $ git stash
> -$ edit emergency fix
> +# ... edit emergency fix ...
>  $ git commit -a -m "Fix in a hurry"
>  $ git stash pop
>  # ... continue hacking ...
> @@ -261,11 +261,11 @@ each change before committing:
>  # ... hack hack hack ...
>  $ git add --patch foo# add just first part to the index
>  $ git stash save --keep-index# save all other changes to the stash
> -$ edit/build/test first part
> +# ... edit, build and test first part ...
>  $ git commit -m 'First part' # commit fully tested change
>  $ git stash pop  # prepare to work on all other changes
>  # ... repeat above five steps until one commit remains ...
> -$ edit/build/test remaining parts
> +# ... edit, build and test remaining parts ...
>  $ git commit foo -m 'Remaining parts'
>  
>  
>
> --
> https://github.com/git/git/pull/361

Re: [PATCH] wildmatch test: remove redundant duplicate test

[Junio C Hamano]

Thanks.  Did you run "sort | uniq -c" on it or something ;-)?

Will apply.

Re: git-2.13.0: log --date=format:%z not working

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  writes:

>>
>> Here are some links to past explorations:
>>
>>   http://public-inbox.org/git/20160208152858.ga17...@sigill.intra.peff.net/
>>
>>   http://public-inbox.org/git/87vb2d37ea@web.de/
>
> There's a third and possibly least shitty option that isn't covered in
> those threads; We could just make a pass over the strftime format
> ourselves and replace %z and %Z with the timezone (as done for
> DATE_ISO8601_STRICT in date.c), then hand the rest off to strftime().

I do not know about %Z but certainly for %z that sounds the least
bad.

In a nearby message René seems to have come up with the same idea,
too ;-)

Re: [PATCH] doc: filter-branch does not require re-export of vars

[Junio C Hamano]

Jeff King  writes:

> On Fri, May 26, 2017 at 07:36:54PM +0200, Andreas Heiduk wrote:
>
>> The function `set_ident` in `filter-branch` exported the variables
>> GIT_(AUTHOR|COMMITTER)_(NAME|EMAIL|DATE) at least since 6f6826c52b in 2007.
>> Therefore the filter scripts don't need to re-eport them again.
>
> Some old shells keep separate values for the internal and exporter
> versions of variables. I.e., this:
>
>   foo=one
>   export foo
>   foo=two
>
> would continue to export $foo as "one", even though it is "two" inside
> the script.

Yup, that sounds like a grandfather's war story at this point,
though ;-)

> However, I think POSIX mandates the behavior you'd expect. ...
> ...
> At this point, I'd be inclined to remove the text as you suggest and
> either make a small note at the bottom of the page, or just omit it
> entirely and assume that anybody on an old non-POSIX shell can fend for
> themselves.

Sounds good to me.  Thanks.

[PATCH] pull: ff --rebase --autostash works in dirty repo

[Tyler Brazier]

pull --rebase --autostash was failing on a fast-forward in a dirty repo
since we shortcut to run_merge(), which does not know how to autostash.
The shortcut is a performance optimization, and since rebase was
rewritten in C, it seemed okay to just bypass the shortcut if we
autostash.
---
 builtin/pull.c  |  5 +++--
 t/t5520-pull.sh | 18 ++
 2 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index dd1a4a94e41e..609e594d3f28 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -772,6 +772,7 @@ int cmd_pull(int argc, const char **argv, const char 
*prefix)
struct oid_array merge_heads = OID_ARRAY_INIT;
struct object_id orig_head, curr_head;
struct object_id rebase_fork_point;
+   int autostash;
 
if (!getenv("GIT_REFLOG_ACTION"))
set_reflog_message(argc, argv);
@@ -800,8 +801,8 @@ int cmd_pull(int argc, const char **argv, const char 
*prefix)
if (!opt_rebase && opt_autostash != -1)
die(_("--[no-]autostash option is only valid with --rebase."));
 
+   autostash = config_autostash;
if (opt_rebase) {
-   int autostash = config_autostash;
if (opt_autostash != -1)
autostash = opt_autostash;
 
@@ -868,7 +869,7 @@ int cmd_pull(int argc, const char **argv, const char 
*prefix)
head = lookup_commit_reference(orig_head.hash);
commit_list_insert(head, );
merge_head = lookup_commit_reference(merge_heads.oid[0].hash);
-   if (is_descendant_of(merge_head, list)) {
+   if (!autostash && is_descendant_of(merge_head, list)) {
/* we can fast-forward this without invoking rebase */
opt_ff = "--ff-only";
return run_merge();
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 17f4d0fe4e72..4c85be0417cf 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -272,6 +272,24 @@ test_expect_success '--rebase fast forward' '
test_cmp reflog.expected reflog.fuzzy
 '
 
+test_expect_success '--rebase --autostash fast forward' '
+   test_when_finished "
+   git reset --hard;
+   git checkout to-rebase;
+   git branch -D to-rebase-ff;
+   git branch -D behind" &&
+   git branch behind &&
+   git checkout -b to-rebase-ff &&
+   echo another modification >>file &&
+   git add file &&
+   git commit -m mod &&
+
+   git checkout behind &&
+   echo dirty >file &&
+   git pull --rebase --autostash . to-rebase-ff &&
+   test "$(git rev-parse HEAD)" = "$(git rev-parse to-rebase-ff)"
+'
+
 test_expect_success '--rebase with conflicts shows advice' '
test_when_finished "git rebase --abort; git checkout -f to-rebase" &&
git checkout -b seq &&

--
https://github.com/git/git/pull/365

Re: [PATCH/RFC] branch: add tests for new copy branch feature

[Ævar Arnfjörð Bjarmason]

On Mon, May 29, 2017 at 12:56 AM, Sahil Dua  wrote:
> New feature - copying a branch along with its config section.
>
> Aim is to have an option -c for copying a branch just like -m option for
> renaming a branch.
>
> This commit adds a few basic tests for getting any suggestions/feedback
> about expected behavior for this new feature.
>
> Signed-off-by: Sahil Dua 
> ---
>  t/t3200-branch.sh | 53 +
>  1 file changed, 53 insertions(+)
>
> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
> index fe62e7c775da..2c95ed6ebf3c 100755
> --- a/t/t3200-branch.sh
> +++ b/t/t3200-branch.sh
> @@ -341,6 +341,59 @@ test_expect_success 'config information was renamed, 
> too' '
> test_must_fail git config branch.s/s/dummy
>  '
>
> +test_expect_success 'git branch -c dumps usage' '
> +   test_expect_code 128 git branch -c 2>err &&
> +   test_i18ngrep "branch name required" err
> +'
> +
> +git config branch.d.dummy Hello
> +
> +test_expect_success 'git branch -c d e should work' '
> +   git branch -l d &&
> +   git reflog exists refs/heads/d &&
> +   git branch -c d e &&
> +   git reflog exists refs/heads/d &&
> +   git reflog exists refs/heads/e
> +'
> +
> +test_expect_success 'config information was copied, too' '
> +   test $(git config branch.e.dummy) = Hello &&
> +   test $(git config branch.d.dummy) = Hello
> +'
> +
> +git config branch.f/f.dummy Hello
> +
> +test_expect_success 'git branch -c f/f g/g should work' '
> +   git branch -l f/f &&
> +   git reflog exists refs/heads/f/f &&
> +   git branch -c f/f g/g &&
> +   git reflog exists refs/heads/f/f &&
> +   git reflog exists refs/heads/g/g
> +'
> +
> +test_expect_success 'config information was copied, too' '
> +   test $(git config branch.f/f.dummy) = Hello &&
> +   test $(git config branch.g/g.dummy) = Hello
> +'
> +
> +test_expect_success 'git branch -c m2 m2 should work' '
> +   git branch -l m2 &&
> +   git reflog exists refs/heads/m2 &&
> +   git branch -c m2 m2 &&
> +   git reflog exists refs/heads/m2
> +'
> +
> +test_expect_success 'git branch -c a a/a should fail' '
> +   git branch -l a &&
> +   git reflog exists refs/heads/a &&
> +   test_must_fail git branch -c a a/a
> +'
> +
> +test_expect_success 'git branch -c b/b b should fail' '
> +   git branch -l b/b &&
> +   test_must_fail git branch -c b/b b
> +'
> +
>  test_expect_success 'deleting a symref' '
> git branch target &&
> git symbolic-ref refs/heads/symref refs/heads/target &&
>

This looks good to me. Comments, in no particular order:

* There should be a test for `git branch -c `, i.e. that
should implicitly copy from HEAD just like `git branch -m `
does. However, when looking at this I can see there's actually no test
for one-argument `git branch -m`, i.e. if you apply this:

--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -699,8 +699,6 @@ int cmd_branch(int argc, const char **argv, const
char *prefix)
} else if (rename) {
if (!argc)
die(_("branch name required"));
-   else if (argc == 1)
-   rename_branch(head, argv[0], rename > 1);
else if (argc == 2)
rename_branch(argv[0], argv[1], rename > 1);
else

The only test that fails is a `git branch -M master`, i.e.
one-argument -M is tested for, but not -m, same codepath though, but
while you're at it a patch/series like this could start by adding a
one-arg -m test, then follow-up by copying that for the new `-c`.

* We should have a -C to force -c just like -M forces -m, i.e. a test
where one-arg `git branch -C alreadyexists` clobbers alreadyexists,
and `git branch -C source alreadyexists` does the same for two-arg.

* I know this is just something you're copying, but this `git branch
-l ` use gets me every time "wait how does listing it help isn't
that always succeeding ... damnit it's --create-reflog not --list, got
me again" :)

Just noting it in case it confuses other reviewers who are skimming
this. Might be worth it to just use --create-reflog for new tests
instead of the non-obvious -l whatever the existing tests in the file
do, or maybe I'm the only one confused by this :)

* When you use `git branch -m` it adds a note to the reflog, your
patch should have a test like the existing "git branch -M baz bam
should add entries to .git/logs/HEAD" test in this file except
"Branch: copied ..." instead of "Branch: renamed...".

* Should there be tests for `git branch -c master master` like we have
for `git branch -m master master` (see 3f59481e33 ("branch: allow a
no-op "branch -M  HEAD"", 2011-11-25)). Allowing this
for -m smells like a bend-over-backwards workaround for some script
Jonathan had, should we be doing this for -c too? I don't know.

[PATCH/RFC] branch: add tests for new copy branch feature

[Sahil Dua]

New feature - copying a branch along with its config section.

Aim is to have an option -c for copying a branch just like -m option for
renaming a branch.

This commit adds a few basic tests for getting any suggestions/feedback
about expected behavior for this new feature.

Signed-off-by: Sahil Dua 
---
 t/t3200-branch.sh | 53 +
 1 file changed, 53 insertions(+)

diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index fe62e7c775da..2c95ed6ebf3c 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -341,6 +341,59 @@ test_expect_success 'config information was renamed, too' '
test_must_fail git config branch.s/s/dummy
 '
 
+test_expect_success 'git branch -c dumps usage' '
+   test_expect_code 128 git branch -c 2>err &&
+   test_i18ngrep "branch name required" err
+'
+
+git config branch.d.dummy Hello
+
+test_expect_success 'git branch -c d e should work' '
+   git branch -l d &&
+   git reflog exists refs/heads/d &&
+   git branch -c d e &&
+   git reflog exists refs/heads/d &&
+   git reflog exists refs/heads/e
+'
+
+test_expect_success 'config information was copied, too' '
+   test $(git config branch.e.dummy) = Hello &&
+   test $(git config branch.d.dummy) = Hello
+'
+
+git config branch.f/f.dummy Hello
+
+test_expect_success 'git branch -c f/f g/g should work' '
+   git branch -l f/f &&
+   git reflog exists refs/heads/f/f &&
+   git branch -c f/f g/g &&
+   git reflog exists refs/heads/f/f &&
+   git reflog exists refs/heads/g/g
+'
+
+test_expect_success 'config information was copied, too' '
+   test $(git config branch.f/f.dummy) = Hello &&
+   test $(git config branch.g/g.dummy) = Hello
+'
+
+test_expect_success 'git branch -c m2 m2 should work' '
+   git branch -l m2 &&
+   git reflog exists refs/heads/m2 &&
+   git branch -c m2 m2 &&
+   git reflog exists refs/heads/m2
+'
+
+test_expect_success 'git branch -c a a/a should fail' '
+   git branch -l a &&
+   git reflog exists refs/heads/a &&
+   test_must_fail git branch -c a a/a
+'
+
+test_expect_success 'git branch -c b/b b should fail' '
+   git branch -l b/b &&
+   test_must_fail git branch -c b/b b
+'
+
 test_expect_success 'deleting a symref' '
git branch target &&
git symbolic-ref refs/heads/symref refs/heads/target &&

--
https://github.com/git/git/pull/363

Re: [PATCH v3 4/4] stash: implement builtin stash

[Jeff King]

On Sun, May 28, 2017 at 11:31:48AM -0700, Joel Teichroeb wrote:

> >> +   /* TODO: Improve this logic */
> >> +   strbuf_addf(, "%s", REV);
> >> +   str = strstr(symbolic.buf, "@");
> >
> > Could you elaborate on how this should be improved?
> 
> I just figured there would be a builtin function that could help here,
> but hadn't had the chance to look into it. It's something easy to do
> in bash, but more complicated in C.

There's no strbuf function for "truncate at this character". But:

  - you can use strchr for a single-character match, which is more
efficient; i.e.:

  str = strchr(symbolic.buf, '@');

  - instead of inserting a '\0' into the strbuf, use strbuf_setlen(),
which also updates the symbolic.len; i.e.:

  strbuf_setlen(, str - symbolic.buf);

  - it looks like you copy into the strbuf just to truncate, so you
could actually get the final size before inserting into the strbuf
using strchrnul. Like:

  end_of_rev = strchrnul(REV, '@');
  strbuf_add(, REV, end_of_rev - REV);

-Peff

Re: [PATCH v3 4/4] stash: implement builtin stash

[Jeff King]

On Sun, May 28, 2017 at 08:51:07PM +0200, Ævar Arnfjörð Bjarmason wrote:

> On Sun, May 28, 2017 at 6:56 PM, Joel Teichroeb  wrote:
> > Implement all git stash functionality as a builtin command
> >
> > Signed-off-by: Joel Teichroeb 
> > ---
> 
> General note on this that I missed in my first E-Mail, you have ~20
> calls to argv_array_init() but none to argv_array_clear(). So you're
> leaking memory, and it obscures potential other issues with valgrind.

I haven't looked carefully at the patches, but note that the argv array
in a child_process is handled automatically by start/finish_command.

So:

> @@ -1107,9 +,9 @@ static int list_stash(int argc, const char
> **argv, const char *prefix)
> argv_array_pushv(, argv);
> argv_array_push(, ref_stash);
> if (cmd_log(args.argc, args.argv, prefix))
> -   return 1;
> -
> -   return 0;
> +   ret = 1;
> +   argv_array_clear();
> +   return ret;
>  }

This one does need a clear, because it's calling an internal function.
But...

> @@ -741,13 +740,18 @@ static int do_push_stash(const char *prefix,
> const char *message,
> argv_array_push(, "--");
> argv_array_pushv(, argv);
> pipe_command(, NULL, 0, , 0, NULL, 0);
> +   argv_array_clear();

This one does not, because the array will have been cleared by calling
pipe_command (because it does the start/finish block itself; and yes,
before you go checking, start_command() clears it even when it returns
error).

> +   child_process_clear();

This should not be necessary for the same reason.

> -   cp2.git_cmd = 1;
> -   argv_array_push(, "checkout-index");
> -   argv_array_push(, "-z");
> -   argv_array_push(, "--force");
> -   argv_array_push(, "--stdin");
> -   pipe_command(, out.buf, out.len, NULL, 0, NULL, 
> 0);
> +   child_process_init();
> +   cp.git_cmd = 1;
> +   argv_array_push(, "checkout-index");
> +   argv_array_push(, "-z");
> +   argv_array_push(, "--force");
> +   argv_array_push(, "--stdin");
> +   pipe_command(, out.buf, out.len, NULL, 0, NULL, 0);
> +   argv_array_clear();
> +   child_process_clear();

And ditto here.

I'd also encourage using CHILD_PROCESS_INIT and ARGV_ARRAY_INIT constant
initializers instead of their function-call counterparts, as that
matches our usual style.

-Peff

Re: [PATCH v3 0/4] Implement git stash as a builtin command

[Ævar Arnfjörð Bjarmason]

On Sun, May 28, 2017 at 6:56 PM, Joel Teichroeb  wrote:
> I've rewritten git stash as a builtin c command. All tests pass,
> and I've added two new tests. Test coverage is around 95% with the
> only things missing coverage being error handlers.

Worth noting, with your patches the best of 3 run of all the stash tests:

$ for i in {1..3}; do (time prove -j1 t*-stash*.sh) 2>&1 | grep ^real;
done  |awk '{print $2}' | sort -n | head -n 1
0m3.293s

Without:

$ for i in {1..3}; do (time prove -j1 t*-stash*.sh) 2>&1 | grep ^real;
done  |awk '{print $2}' | sort -n | head -n 1
0m7.619s

Of course some individual invocations are much faster than that, this
includes all the shell overhead of the tests themselves.

Re: [PATCH v3 4/4] stash: implement builtin stash

[Ævar Arnfjörð Bjarmason]

On Sun, May 28, 2017 at 6:56 PM, Joel Teichroeb  wrote:
> Implement all git stash functionality as a builtin command
>
> Signed-off-by: Joel Teichroeb 
> ---

General note on this that I missed in my first E-Mail, you have ~20
calls to argv_array_init() but none to argv_array_clear(). So you're
leaking memory, and it obscures potential other issues with valgrind.

A lot of that's easy to solve, but sometimes requires a temporary
variable since the code is now returning directly, e.g:

@@ -1091,6 +1094,7 @@ static int list_stash(int argc, const char
**argv, const char *prefix)
struct object_id obj;
struct object_context unused;
struct argv_array args;
+   int ret = 0;

argc = parse_options(argc, argv, prefix, options,
 git_stash_list_usage, PARSE_OPT_KEEP_UNKNOWN);
@@ -1107,9 +,9 @@ static int list_stash(int argc, const char
**argv, const char *prefix)
argv_array_pushv(, argv);
argv_array_push(, ref_stash);
if (cmd_log(args.argc, args.argv, prefix))
-   return 1;
-
-   return 0;
+   ret = 1;
+   argv_array_clear();
+   return ret;
 }

But more generally this goes a long way to resolving the issue where
you have variables like out1, out2 or cp1, cp2 etc. which Christian
pointed out. I.e. you're not freeing/clearing strbufs either, instead
just creating new ones that also aren't freed, or not clearing
child_process structs, e.g. this on top allows you to re-use the same
variable and stops leaking memory:

diff --git a/builtin/stash.c b/builtin/stash.c
index bf36ff8f9b..4e7344501a 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -729,7 +729,6 @@ static int do_push_stash(const char *prefix, const
char *message,

if (keep_index) {
struct child_process cp = CHILD_PROCESS_INIT;
-   struct child_process cp2 = CHILD_PROCESS_INIT;
struct strbuf out = STRBUF_INIT;

reset_tree(info.i_tree, 0, 1);
@@ -741,13 +740,18 @@ static int do_push_stash(const char *prefix,
const char *message,
argv_array_push(, "--");
argv_array_pushv(, argv);
pipe_command(, NULL, 0, , 0, NULL, 0);
+   argv_array_clear();
+   child_process_clear();

-   cp2.git_cmd = 1;
-   argv_array_push(, "checkout-index");
-   argv_array_push(, "-z");
-   argv_array_push(, "--force");
-   argv_array_push(, "--stdin");
-   pipe_command(, out.buf, out.len, NULL, 0, NULL, 0);
+   child_process_init();
+   cp.git_cmd = 1;
+   argv_array_push(, "checkout-index");
+   argv_array_push(, "-z");
+   argv_array_push(, "--force");
+   argv_array_push(, "--stdin");
+   pipe_command(, out.buf, out.len, NULL, 0, NULL, 0);
+   argv_array_clear();
+   child_process_clear();
}
} else {
struct child_process cp2 = CHILD_PROCESS_INIT;

Re: [PATCH v3 4/4] stash: implement builtin stash

[Joel Teichroeb]

On Sun, May 28, 2017 at 11:26 AM, Ævar Arnfjörð Bjarmason
 wrote:
> On Sun, May 28, 2017 at 6:56 PM, Joel Teichroeb  wrote:
>> Implement all git stash functionality as a builtin command
>
> First thanks for working on this, it's great. Applied it locally,
> passes all tests for me. A couple of comments Christian didn't cover
>
>> +   info->has_u = get_sha1_with_context(u_commit_rev.buf, 0, 
>> info->u_commit.hash, ) == 0 &&
>> +   get_sha1_with_context(u_tree_rev.buf, 0, info->u_tree.hash, 
>> ) == 0;
>> +
>> +
>> +   /* TODO: Improve this logic */
>> +   strbuf_addf(, "%s", REV);
>> +   str = strstr(symbolic.buf, "@");
>
> Could you elaborate on how this should be improved?
>

I just figured there would be a builtin function that could help here,
but hadn't had the chance to look into it. It's something easy to do
in bash, but more complicated in C.

>
>> +static int patch_working_tree(struct stash_info *info, const char *prefix,
>> +   const char **argv)
>> +{
>> +   const char *stash_index_path = ".git/foocache2";
>
> This foocache path isn't created by the shell code, if it's a new
> thing that's needed (and I haven't followed this code in detail, don'n
> know what it's for) shouldn't we give it a more descriptive name so
> that if git crashes it's obvious what it is?
>

Opps, I had cleaned that part up locally, but I forgot to push it when
switching computers. It'll be better in the next patch set.

>> +   const char *message = NULL;
>> +   const char *commit = NULL;
>> +   struct object_id obj;
>> +   struct option options[] = {
>> +   OPT_STRING('m', "message", , N_("message"),
>> +N_("stash commit message")),
>> +   OPT__QUIET(, N_("be quiet, only report errors")),
>> +   OPT_END()
>> +   };
>> +   argc = parse_options(argc, argv, prefix, options,
>> +git_stash_store_usage, 0);
>
> Nit: In general in this patch the 2nd line of parse_options doesn't
> align with a tabwidth of 8. Ditto for indenting function arguments
> (e.g. for untracked_files).

I'll fix my tab width. Forgot that long lines would change, haha.

Re: [PATCH v3 4/4] stash: implement builtin stash

[Ævar Arnfjörð Bjarmason]

On Sun, May 28, 2017 at 6:56 PM, Joel Teichroeb  wrote:
> Implement all git stash functionality as a builtin command

First thanks for working on this, it's great. Applied it locally,
passes all tests for me. A couple of comments Christian didn't cover

> +   info->has_u = get_sha1_with_context(u_commit_rev.buf, 0, 
> info->u_commit.hash, ) == 0 &&
> +   get_sha1_with_context(u_tree_rev.buf, 0, info->u_tree.hash, 
> ) == 0;
> +
> +
> +   /* TODO: Improve this logic */
> +   strbuf_addf(, "%s", REV);
> +   str = strstr(symbolic.buf, "@");

Could you elaborate on how this should be improved?


> +static int patch_working_tree(struct stash_info *info, const char *prefix,
> +   const char **argv)
> +{
> +   const char *stash_index_path = ".git/foocache2";

This foocache path isn't created by the shell code, if it's a new
thing that's needed (and I haven't followed this code in detail, don'n
know what it's for) shouldn't we give it a more descriptive name so
that if git crashes it's obvious what it is?

> +   const char *message = NULL;
> +   const char *commit = NULL;
> +   struct object_id obj;
> +   struct option options[] = {
> +   OPT_STRING('m', "message", , N_("message"),
> +N_("stash commit message")),
> +   OPT__QUIET(, N_("be quiet, only report errors")),
> +   OPT_END()
> +   };
> +   argc = parse_options(argc, argv, prefix, options,
> +git_stash_store_usage, 0);

Nit: In general in this patch the 2nd line of parse_options doesn't
align with a tabwidth of 8. Ditto for indenting function arguments
(e.g. for untracked_files).

Re: [PATCH v3 2/4] stash: add test for stashing in a detached state

[Ævar Arnfjörð Bjarmason]

On Sun, May 28, 2017 at 6:56 PM, Joel Teichroeb  wrote:
> Signed-off-by: Joel Teichroeb 
> ---
>  t/t3903-stash.sh | 11 +++
>  1 file changed, 11 insertions(+)
>
> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index aaae221304..b86851ef46 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -808,6 +808,17 @@ test_expect_success 'create with multiple arguments for 
> the message' '
> test_cmp expect actual
>  '
>
> +test_expect_success 'create in a detached state' '
> +   test_when_finished "git checkout master" &&
> +   git checkout HEAD~1 &&
> +   >foo &&
> +   git add foo &&
> +   STASH_ID=$(git stash create) &&
> +   echo "WIP on (no branch): 47d5e0e initial" >expect &&
> +   git show --pretty=%s -s ${STASH_ID} >actual &&
> +   test_cmp expect actual

I thought this needed test_i18ncmp, turns out it didn't, neither the
original stash code nor your replacement translates this. That's fine,
just a note to other reviewers...

> +'
> +
>  test_expect_success 'stash --  stashes and restores the file' '
> >foo &&
> >bar &&
> --
> 2.13.0
>

Re: [PATCH v3 4/4] stash: implement builtin stash

[Christian Couder]

On Sun, May 28, 2017 at 6:56 PM, Joel Teichroeb  wrote:

[...]

> +int untracked_files(struct strbuf *out, int include_untracked,
> +   const char **argv)
> +{
> +   struct child_process cp = CHILD_PROCESS_INIT;
> +   cp.git_cmd = 1;
> +   argv_array_push(, "ls-files");
> +   argv_array_push(, "-o");
> +   argv_array_push(, "-z");

You might want to use argv_array_pushl(), for example:

   argv_array_pushl(, "ls-files", "-o", "-z", NULL);

> +   if (include_untracked != 2)
> +   argv_array_push(, "--exclude-standard");
> +   argv_array_push(, "--");
> +   if (argv)
> +   argv_array_pushv(, argv);
> +   return pipe_command(, NULL, 0, out, 0, NULL, 0);
> +}
> +
> +static int check_no_changes(const char *prefix, int include_untracked,
> +   const char **argv)
> +{
> +   struct argv_array args1;
> +   struct argv_array args2;
> +   struct strbuf out = STRBUF_INIT;
> +
> +   argv_array_init();
> +   argv_array_push(, "diff-index");
> +   argv_array_push(, "--quiet");
> +   argv_array_push(, "--cached");
> +   argv_array_push(, "HEAD");
> +   argv_array_push(, "--ignore-submodules");
> +   argv_array_push(, "--");

Here and in other places also you could use argv_array_pushl().

> +   if (argv)
> +   argv_array_pushv(, argv);
> +   argv_array_init();
> +   argv_array_push(, "diff-files");
> +   argv_array_push(, "--quiet");
> +   argv_array_push(, "--ignore-submodules");
> +   argv_array_push(, "--");
> +   if (argv)
> +   argv_array_pushv(, argv);
> +   if (include_untracked)
> +   untracked_files(, include_untracked, argv);
> +   return cmd_diff_index(args1.argc, args1.argv, prefix) == 0 &&
> +   cmd_diff_files(args2.argc, args2.argv, prefix) == 0 &&
> +   (!include_untracked || out.len == 0);
> +}
> +
> +static int get_stash_info(struct stash_info *info, const char *commit)
> +{
> +   struct strbuf w_commit_rev = STRBUF_INIT;
> +   struct strbuf b_commit_rev = STRBUF_INIT;
> +   struct strbuf i_commit_rev = STRBUF_INIT;
> +   struct strbuf u_commit_rev = STRBUF_INIT;
> +   struct strbuf w_tree_rev = STRBUF_INIT;
> +   struct strbuf b_tree_rev = STRBUF_INIT;
> +   struct strbuf i_tree_rev = STRBUF_INIT;
> +   struct strbuf u_tree_rev = STRBUF_INIT;
> +   struct strbuf commit_buf = STRBUF_INIT;
> +   struct strbuf symbolic = STRBUF_INIT;
> +   struct strbuf out = STRBUF_INIT;
> +   struct object_context unused;
> +   char *str;
> +   int ret;
> +   const char *REV = commit;

We use lower case variable names.

> +   struct child_process cp = CHILD_PROCESS_INIT;
> +   info->is_stash_ref = 0;
> +
> +   if (commit == NULL) {
> +   strbuf_addf(_buf, "%s@{0}", ref_stash);
> +   REV = commit_buf.buf;
> +   } else if (strlen(commit) < 3) {
> +   strbuf_addf(_buf, "%s@{%s}", ref_stash, commit);
> +   REV = commit_buf.buf;
> +   }
> +   info->REV = REV;

Also the "REV" member of struct stash_info could be lower cased.

> +   strbuf_addf(_commit_rev, "%s", REV);
> +   strbuf_addf(_commit_rev, "%s^1", REV);
> +   strbuf_addf(_commit_rev, "%s^2", REV);
> +   strbuf_addf(_commit_rev, "%s^3", REV);
> +   strbuf_addf(_tree_rev, "%s:", REV);
> +   strbuf_addf(_tree_rev, "%s^1:", REV);
> +   strbuf_addf(_tree_rev, "%s^2:", REV);
> +   strbuf_addf(_tree_rev, "%s^3:", REV);
> +
> +

Spurious new line above.

> +   ret = (
> +   get_sha1_with_context(w_commit_rev.buf, 0, 
> info->w_commit.hash, ) == 0 &&
> +   get_sha1_with_context(b_commit_rev.buf, 0, 
> info->b_commit.hash, ) == 0 &&
> +   get_sha1_with_context(i_commit_rev.buf, 0, 
> info->i_commit.hash, ) == 0 &&
> +   get_sha1_with_context(w_tree_rev.buf, 0, info->w_tree.hash, 
> ) == 0 &&
> +   get_sha1_with_context(b_tree_rev.buf, 0, info->b_tree.hash, 
> ) == 0 &&
> +   get_sha1_with_context(i_tree_rev.buf, 0, info->i_tree.hash, 
> ) == 0);
> +
> +   if (!ret) {
> +   fprintf_ln(stderr, _("%s is not a valid reference"), REV);
> +   return 1;

Maybe use "return error(_("%s is not a valid reference"), REV);"

> +   }
> +
> +

Spurious new lines above.

> +
> +static void stash_create_callback(struct diff_queue_struct *q,
> +   struct diff_options *opt, void *cbdata)
> +{
> +   int i;
> +
> +   for (i = 0; i < q->nr; i++) {
> +   struct diff_filepair *p = q->queue[i];
> +   const char *path = p->one->path;
> +   struct stat st;
> +   remove_file_from_index(_index, path);
> +   if (!lstat(path, ))
> +   add_to_index(_index, path, , 0);
> +

Re: [PATCH v3 3/4] close the index lock when not writing the new index

[Ævar Arnfjörð Bjarmason]

On Sun, May 28, 2017 at 6:56 PM, Joel Teichroeb  wrote:
> Signed-off-by: Joel Teichroeb 
> ---
>  builtin/add.c | 3 ++-
>  builtin/mv.c  | 8 +---
>  builtin/rm.c  | 3 ++-
>  merge-recursive.c | 8 +---
>  4 files changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/builtin/add.c b/builtin/add.c
> index 9f53f020d0..6b04eb2c71 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -461,7 +461,8 @@ int cmd_add(int argc, const char **argv, const char 
> *prefix)
> if (active_cache_changed) {
> if (write_locked_index(_index, _file, COMMIT_LOCK))
> die(_("Unable to write new index file"));
> -   }
> +   } else
> +   rollback_lock_file(_file);

>From Documentation/CodingGuidelines: "When there are multiple arms to
a conditional and some of them[...]".

>
> return exit_status;
>  }
> diff --git a/builtin/mv.c b/builtin/mv.c
> index 61d20037ad..ccf21de17f 100644
> --- a/builtin/mv.c
> +++ b/builtin/mv.c
> @@ -293,9 +293,11 @@ int cmd_mv(int argc, const char **argv, const char 
> *prefix)
> if (gitmodules_modified)
> stage_updated_gitmodules();
>
> -   if (active_cache_changed &&
> -   write_locked_index(_index, _file, COMMIT_LOCK))
> -   die(_("Unable to write new index file"));
> +   if (active_cache_changed) {
> +   if (write_locked_index(_index, _file, COMMIT_LOCK))
> +   die(_("Unable to write new index file"));
> +   } else
> +   rollback_lock_file(_file);
>
> return 0;
>  }
> diff --git a/builtin/rm.c b/builtin/rm.c
> index fb79dcab18..4c7a91888b 100644
> --- a/builtin/rm.c
> +++ b/builtin/rm.c
> @@ -389,7 +389,8 @@ int cmd_rm(int argc, const char **argv, const char 
> *prefix)
> if (active_cache_changed) {
> if (write_locked_index(_index, _file, COMMIT_LOCK))
> die(_("Unable to write new index file"));
> -   }
> +   } else
> +   rollback_lock_file(_file);
>
> return 0;
>  }
> diff --git a/merge-recursive.c b/merge-recursive.c
> index 62decd51cc..db841c0d38 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -2145,9 +2145,11 @@ int merge_recursive_generic(struct merge_options *o,
> if (clean < 0)
> return clean;
>
> -   if (active_cache_changed &&
> -   write_locked_index(_index, lock, COMMIT_LOCK))
> -   return err(o, _("Unable to write index."));
> +   if (active_cache_changed) {
> +   if (write_locked_index(_index, lock, COMMIT_LOCK))
> +   return err(o, _("Unable to write index."));
> +   } else
> +   rollback_lock_file(lock);
>
> return clean ? 0 : 1;
>  }
> --
> 2.13.0
>

Re: [PATCH v3 1/4] stash: add test for stash create with no files

[Ævar Arnfjörð Bjarmason]

On Sun, May 28, 2017 at 6:56 PM, Joel Teichroeb  wrote:
> Ensure the command gives the correct return code
>
> Signed-off-by: Joel Teichroeb 
> ---
>  t/t3903-stash.sh | 8 
>  1 file changed, 8 insertions(+)
>
> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index 3b4bed5c9a..aaae221304 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -444,6 +444,14 @@ test_expect_failure 'stash file to directory' '
> test foo = "$(cat file/file)"
>  '
>
> +test_expect_success 'stash create - no changes' '
> +   git stash clear &&
> +   test_when_finished "git reset --hard HEAD" &&
> +   git reset --hard &&
> +   git stash create > actual &&
> +   test $(cat actual | wc -l) -eq 0

Looks fine like this, I don't think there's any actual portability
concern (as with some wrappers), but FWIW you can do this more briefly
with the test framework as:

test_line_count = 0 actual

Although I wonder in this case whether you don't actually mean:

[...]>actual &&
! test -s actual

I.e. isn't the test that there should be no output, not that there
shouldn't be a \n there? Or alternatively:

 test $(cat actual | wc -c) -eq 0

> +'
> +
>  test_expect_success 'stash branch - no stashes on stack, stash-like 
> argument' '
> git stash clear &&
> test_when_finished "git reset --hard HEAD" &&
> --
> 2.13.0
>

Re: [PATCH] branch test: fix invalid config key access

[Ævar Arnfjörð Bjarmason]

On Sun, May 28, 2017 at 7:12 PM, Sahil Dua  wrote:
> Fixes the test by changing "branch.s/s/dummy" to "branch.s/s.dummy" which is
> the right way of accessing config key "branch.s/s.dummy". Purpose of
> this test is to confirm that this key doesn't exist after the branch
> "s/s" has been renamed to "s".
>
> Earlier it was trying to access invalid config key and hence was getting
> an error. However, this wasn't caught because we were expecting the
> command to fail for other reason as mentioned above.

Looks obviously correct to me. Added by Johannes in commit dc81c58cd6
("git-branch: rename config vars branch..*, too", 2006-12-16),
CC-ing him in case he has anything to say about it, but just looks
like a typo back in 2006.

> Signed-off-by: Sahil Dua 
> ---
>  t/t3200-branch.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
> index fe62e7c..10f8f02 100755
> --- a/t/t3200-branch.sh
> +++ b/t/t3200-branch.sh
> @@ -338,7 +338,7 @@ test_expect_success 'git branch -m s/s s should work when 
> s/t is deleted' '
>
>  test_expect_success 'config information was renamed, too' '
> test $(git config branch.s.dummy) = Hello &&
> -   test_must_fail git config branch.s/s/dummy
> +   test_must_fail git config branch.s/s.dummy
>  '
>
>  test_expect_success 'deleting a symref' '
> --
> 2.7.4 (Apple Git-66)
>

[PATCH] branch test: fix invalid config key access

[Sahil Dua]

Fixes the test by changing "branch.s/s/dummy" to "branch.s/s.dummy" which is
the right way of accessing config key "branch.s/s.dummy". Purpose of
this test is to confirm that this key doesn't exist after the branch
"s/s" has been renamed to "s".

Earlier it was trying to access invalid config key and hence was getting
an error. However, this wasn't caught because we were expecting the
command to fail for other reason as mentioned above.

Signed-off-by: Sahil Dua 
---
 t/t3200-branch.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index fe62e7c..10f8f02 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -338,7 +338,7 @@ test_expect_success 'git branch -m s/s s should work when 
s/t is deleted' '
 
 test_expect_success 'config information was renamed, too' '
test $(git config branch.s.dummy) = Hello &&
-   test_must_fail git config branch.s/s/dummy
+   test_must_fail git config branch.s/s.dummy
 '
 
 test_expect_success 'deleting a symref' '
-- 
2.7.4 (Apple Git-66)

[PATCH v3 3/4] close the index lock when not writing the new index

[Joel Teichroeb]

Signed-off-by: Joel Teichroeb 
---
 builtin/add.c | 3 ++-
 builtin/mv.c  | 8 +---
 builtin/rm.c  | 3 ++-
 merge-recursive.c | 8 +---
 4 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index 9f53f020d0..6b04eb2c71 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -461,7 +461,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)
if (active_cache_changed) {
if (write_locked_index(_index, _file, COMMIT_LOCK))
die(_("Unable to write new index file"));
-   }
+   } else
+   rollback_lock_file(_file);
 
return exit_status;
 }
diff --git a/builtin/mv.c b/builtin/mv.c
index 61d20037ad..ccf21de17f 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -293,9 +293,11 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
if (gitmodules_modified)
stage_updated_gitmodules();
 
-   if (active_cache_changed &&
-   write_locked_index(_index, _file, COMMIT_LOCK))
-   die(_("Unable to write new index file"));
+   if (active_cache_changed) {
+   if (write_locked_index(_index, _file, COMMIT_LOCK))
+   die(_("Unable to write new index file"));
+   } else
+   rollback_lock_file(_file);
 
return 0;
 }
diff --git a/builtin/rm.c b/builtin/rm.c
index fb79dcab18..4c7a91888b 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -389,7 +389,8 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
if (active_cache_changed) {
if (write_locked_index(_index, _file, COMMIT_LOCK))
die(_("Unable to write new index file"));
-   }
+   } else
+   rollback_lock_file(_file);
 
return 0;
 }
diff --git a/merge-recursive.c b/merge-recursive.c
index 62decd51cc..db841c0d38 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -2145,9 +2145,11 @@ int merge_recursive_generic(struct merge_options *o,
if (clean < 0)
return clean;
 
-   if (active_cache_changed &&
-   write_locked_index(_index, lock, COMMIT_LOCK))
-   return err(o, _("Unable to write index."));
+   if (active_cache_changed) {
+   if (write_locked_index(_index, lock, COMMIT_LOCK))
+   return err(o, _("Unable to write index."));
+   } else
+   rollback_lock_file(lock);
 
return clean ? 0 : 1;
 }
-- 
2.13.0

[PATCH v3 2/4] stash: add test for stashing in a detached state

[Joel Teichroeb]

Signed-off-by: Joel Teichroeb 
---
 t/t3903-stash.sh | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index aaae221304..b86851ef46 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -808,6 +808,17 @@ test_expect_success 'create with multiple arguments for 
the message' '
test_cmp expect actual
 '
 
+test_expect_success 'create in a detached state' '
+   test_when_finished "git checkout master" &&
+   git checkout HEAD~1 &&
+   >foo &&
+   git add foo &&
+   STASH_ID=$(git stash create) &&
+   echo "WIP on (no branch): 47d5e0e initial" >expect &&
+   git show --pretty=%s -s ${STASH_ID} >actual &&
+   test_cmp expect actual
+'
+
 test_expect_success 'stash --  stashes and restores the file' '
>foo &&
>bar &&
-- 
2.13.0

[PATCH v3 4/4] stash: implement builtin stash

[Joel Teichroeb]

Implement all git stash functionality as a builtin command

Signed-off-by: Joel Teichroeb 
---
 Makefile  |2 +-
 builtin.h |1 +
 builtin/stash.c   | 1280 +
 git-stash.sh => contrib/examples/git-stash.sh |0
 git.c |1 +
 5 files changed, 1283 insertions(+), 1 deletion(-)
 create mode 100644 builtin/stash.c
 rename git-stash.sh => contrib/examples/git-stash.sh (100%)

diff --git a/Makefile b/Makefile
index e35542e631..4af4ac41c7 100644
--- a/Makefile
+++ b/Makefile
@@ -523,7 +523,6 @@ SCRIPT_SH += git-quiltimport.sh
 SCRIPT_SH += git-rebase.sh
 SCRIPT_SH += git-remote-testgit.sh
 SCRIPT_SH += git-request-pull.sh
-SCRIPT_SH += git-stash.sh
 SCRIPT_SH += git-submodule.sh
 SCRIPT_SH += git-web--browse.sh
 
@@ -961,6 +960,7 @@ BUILTIN_OBJS += builtin/send-pack.o
 BUILTIN_OBJS += builtin/shortlog.o
 BUILTIN_OBJS += builtin/show-branch.o
 BUILTIN_OBJS += builtin/show-ref.o
+BUILTIN_OBJS += builtin/stash.o
 BUILTIN_OBJS += builtin/stripspace.o
 BUILTIN_OBJS += builtin/submodule--helper.o
 BUILTIN_OBJS += builtin/symbolic-ref.o
diff --git a/builtin.h b/builtin.h
index 9e4a89816d..16e8a437d4 100644
--- a/builtin.h
+++ b/builtin.h
@@ -121,6 +121,7 @@ extern int cmd_shortlog(int argc, const char **argv, const 
char *prefix);
 extern int cmd_show(int argc, const char **argv, const char *prefix);
 extern int cmd_show_branch(int argc, const char **argv, const char *prefix);
 extern int cmd_status(int argc, const char **argv, const char *prefix);
+extern int cmd_stash(int argc, const char **argv, const char *prefix);
 extern int cmd_stripspace(int argc, const char **argv, const char *prefix);
 extern int cmd_submodule__helper(int argc, const char **argv, const char 
*prefix);
 extern int cmd_symbolic_ref(int argc, const char **argv, const char *prefix);
diff --git a/builtin/stash.c b/builtin/stash.c
new file mode 100644
index 00..bf36ff8f9b
--- /dev/null
+++ b/builtin/stash.c
@@ -0,0 +1,1280 @@
+#include "builtin.h"
+#include "parse-options.h"
+#include "refs.h"
+#include "tree.h"
+#include "lockfile.h"
+#include "object.h"
+#include "tree-walk.h"
+#include "cache-tree.h"
+#include "unpack-trees.h"
+#include "diff.h"
+#include "revision.h"
+#include "commit.h"
+#include "diffcore.h"
+#include "merge-recursive.h"
+#include "argv-array.h"
+#include "run-command.h"
+
+static const char * const git_stash_usage[] = {
+   N_("git stash list []"),
+   N_("git stash show []"),
+   N_("git stash drop [-q|--quiet] []"),
+   N_("git stash ( pop | apply ) [--index] [-q|--quiet] []"),
+   N_("git stash branch  []"),
+   N_("git stash [save [--patch] [-k|--[no-]keep-index] [-q|--quiet]"),
+   N_("[-u|--include-untracked] [-a|--all] []]"),
+   N_("git stash clear"),
+   N_("git stash create []"),
+   N_("git stash store [-m|--message ] [-q|--quiet] "),
+   NULL
+};
+
+static const char * const git_stash_list_usage[] = {
+   N_("git stash list []"),
+   NULL
+};
+
+static const char * const git_stash_show_usage[] = {
+   N_("git stash show []"),
+   NULL
+};
+
+static const char * const git_stash_drop_usage[] = {
+   N_("git stash drop [-q|--quiet] []"),
+   NULL
+};
+
+static const char * const git_stash_pop_usage[] = {
+   N_("git stash pop [--index] [-q|--quiet] []"),
+   NULL
+};
+
+static const char * const git_stash_apply_usage[] = {
+   N_("git stash apply [--index] [-q|--quiet] []"),
+   NULL
+};
+
+static const char * const git_stash_branch_usage[] = {
+   N_("git stash branch  []"),
+   NULL
+};
+
+static const char * const git_stash_save_usage[] = {
+   N_("git stash [save [--patch] [-k|--[no-]keep-index] [-q|--quiet]"),
+   N_("[-u|--include-untracked] [-a|--all] []]"),
+   NULL
+};
+
+static const char * const git_stash_clear_usage[] = {
+   N_("git stash clear"),
+   NULL
+};
+
+static const char * const git_stash_create_usage[] = {
+   N_("git stash create []"),
+   NULL
+};
+
+static const char * const git_stash_store_usage[] = {
+   N_("git stash store [-m|--message ] [-q|--quiet] "),
+   NULL
+};
+
+static const char *ref_stash = "refs/stash";
+static int quiet = 0;
+static struct lock_file lock_file;
+static char stash_index_path[64];
+
+struct stash_info {
+   struct object_id w_commit;
+   struct object_id b_commit;
+   struct object_id i_commit;
+   struct object_id u_commit;
+   struct object_id w_tree;
+   struct object_id b_tree;
+   struct object_id i_tree;
+   struct object_id u_tree;
+   const char *message;
+   const char *REV;
+   int is_stash_ref;
+   int has_u;
+   const char *patch;
+};
+
+int untracked_files(struct strbuf *out, int include_untracked,
+   const char **argv)
+{
+ 

[PATCH v3 1/4] stash: add test for stash create with no files

[Joel Teichroeb]

Ensure the command gives the correct return code

Signed-off-by: Joel Teichroeb 
---
 t/t3903-stash.sh | 8 
 1 file changed, 8 insertions(+)

diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 3b4bed5c9a..aaae221304 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -444,6 +444,14 @@ test_expect_failure 'stash file to directory' '
test foo = "$(cat file/file)"
 '
 
+test_expect_success 'stash create - no changes' '
+   git stash clear &&
+   test_when_finished "git reset --hard HEAD" &&
+   git reset --hard &&
+   git stash create > actual &&
+   test $(cat actual | wc -l) -eq 0
+'
+
 test_expect_success 'stash branch - no stashes on stack, stash-like argument' '
git stash clear &&
test_when_finished "git reset --hard HEAD" &&
-- 
2.13.0

[PATCH v3 0/4] Implement git stash as a builtin command

[Joel Teichroeb]

I've rewritten git stash as a builtin c command. All tests pass,
and I've added two new tests. Test coverage is around 95% with the
only things missing coverage being error handlers.

Joel Teichroeb (4):
  stash: add test for stash create with no files
  stash: add test for stashing in a detached state
  close the index lock when not writing the new index
  stash: implement builtin stash

 Makefile  |2 +-
 builtin.h |1 +
 builtin/add.c |3 +-
 builtin/mv.c  |8 +-
 builtin/rm.c  |3 +-
 builtin/stash.c   | 1280 +
 git-stash.sh => contrib/examples/git-stash.sh |0
 git.c |1 +
 merge-recursive.c |8 +-
 t/t3903-stash.sh  |   19 +
 10 files changed, 1316 insertions(+), 9 deletions(-)
 create mode 100644 builtin/stash.c
 rename git-stash.sh => contrib/examples/git-stash.sh (100%)

-- 
2.13.0

[PATCH] doc: Improve description for rev-parse --short

[Andreas Heiduk]

First: `git rev-parse --short` without a number does use a fixed default but
`core.abbrev` which in turn uses `find_unique_abbrev` internally.

Second: `--short` implies `--verify` since the beginning (d50125085a), so
it cannot be used for bulk-shortening ids unfortunately.

Signed-off-by: Andreas Heiduk 
---
 Documentation/config.txt| 1 +
 Documentation/git-rev-parse.txt | 4 +++-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index e0b9fd0bc..158cb588b 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -862,6 +862,7 @@ core.abbrev::
computed based on the approximate number of packed objects
in your repository, which hopefully is enough for
abbreviated object names to stay unique for some time.
+   The minimum length is 4.
 
 add.ignoreErrors::
 add.ignore-errors (deprecated)::
diff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt
index c40c47044..7a7421c8e 100644
--- a/Documentation/git-rev-parse.txt
+++ b/Documentation/git-rev-parse.txt
@@ -140,7 +140,9 @@ can be used.
 --short=number::
Instead of outputting the full SHA-1 values of object names try to
abbreviate them to a shorter unique name. When no length is specified
-   7 is used. The minimum length is 4.
+   the effective value of the configuration variable `core.abbrev` (see
+   linkgit:git-config[1]) is used.  The minimum length is 4.  The length
+   may be exceeded to ensure unique object names.  Implies `--verify`.
 
 --symbolic::
Usually the object names are output in SHA-1 form (with
-- 
2.13.0

Re: Missing: Consistency of clean state output of "git add -i"

[Kaartic Sivaraam]

On Sun, 2017-05-28 at 11:39 +0100, Philip Oakley wrote:
> I wouldn't let that stop you. We were all ignorant once.
> I know little of tcl (gitk/git-gui), but I've still managed to fix a
> couple 
> of issues, with the help of others on the list (and the search
> engines and 
> their results;-)
> 
Thanks for the motivative reply. 

> Perl is *that* hard, is it?
I don't have any idea about Perl. I'll see if I could do something when
I find time. That said, I wouldn't like to prevent anyone who like to
take up this task. If anyone's interested, please take it up.

--
Regards,
Kaartic

Re: mergetool: what to do about deleting precious files?

[Junio C Hamano]

"Philip Oakley"  writes:

>> So I do not think this is not limited to "new file".  Anything that
>> a tree-level three-way merge would resolve cleanly without having to
>> consult the content-level three-way merge will complete without
>> consulting the merge.ours.driver; per-file content-level three-way
>> merge driver (which is what merge= mechanism lets you
>> specify via the attributes mechanism) is not something you would
>> want to use for this kind of thing.  It is purely for resolving the
>> actual content-level conflicts.
>>
> That (that Git knows best) sounds just wrong.

Don't twist my words.  I never said Git knows best.  

The user-level merge driver is a mechanism to affect conflict level
three-way merges.  The interface to the content level three-way
merge driver feeds three versions of blobs and the driver is
expected to give a merged result.  The interface as designed is
incapable of passing "here is the common ancestor", "our side is
missing" and "their side is this content".

So if we want a mechanism that can affect the outcome of tree-level
three-way merge, we need a _new_ mechanism.  The existing merge
drivers that are written by end users (at least the ones written
correctly to the spec, anyway) are not expecting to be called with
"in our tree, there is no blob here", and trying to piggyback on it
will break existing users.

Re: [Non-Bug] cloning a repository with a default MASTER branch tries to check out the master branch

[Junio C Hamano]

"Philip Oakley"  writes:

> From: "Junio C Hamano" 
>> "Philip Oakley"  writes:
>>
>>> However given the discussion about an unborn HEAD, the option here
>>> would be to also pass the NULL sha for the symref and then add the
>>> annotation 'HEAD' after an extra \0, in the same way that an active
>>> symref could be annotated with the '\0HEAD'. This would kill two birds
>>> with one stone!
>>
>> Are you aware of the symref capability that is already advertised in
>> the initial upload-pack response?  Right now, we do so only when
>> HEAD actually points at something, and the earlier suggestion by
>> Peff is to do so unconditionally, even when HEAD is dangling.
>
> The suggestion is the otherway around. I would argue (as a viewpoint)
> that what we advertise are object IDs and their associated refs,
> sorted by ref name. (I'm thinking of the
> git/Documentation/technical/pack-protocol.txt here). My suggestion was

That's not the part of the protocol I explained Peff's suggestion to
you about.  That's ref advertisement proper, and its first line has
a trailing NUL followed by "protocol capability" list.  There is one
"capability" that tells the receiver specifically about HEAD symref
(if and only if HEAD is a symref).  There are two reasons why the
current code does not help even though that necessary protocol bits
are *already* there (i.e. you do not need any protocol extension).
One is that existing servers do not use the symref capability for
HEAD if HEAD is pointing at an unborn branch (i.e. dangling). The
other is that the existing code sitting on the receiving end is not
prepared to handle one, even if the server end sent one.

[no subject]

[Lasek László]

Hello, $850,000 has been donate to you,
kindly get back to us via:   maureendav...@outlook.com












ü Mielőtt kinyomtatja ezt az e-mailt, gondoljon a környezetre. P Please 
consider the environment before printing this email.
***

Ezt az emailt a Websense ESG ellenőrizte a BKV Zrt. biztonsági szabályzata 
alapján. Nem található benne vírus.

[PATCH] completion: Add completions for git config commit

[Rikard Falkeborn]

Add missing completions for git config:

* commit.cleanup
* commit.gpgSign
* commit.verbose

Signed-off-by: Rikard Falkeborn 
---
 contrib/completion/git-completion.bash | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 1ed0a09fe..90dfd7681 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2395,8 +2395,11 @@ _git_config ()
color.status.untracked
color.status.updated
color.ui
+   commit.cleanup
+   commit.gpgSign
commit.status
commit.template
+   commit.verbose
core.abbrev
core.askpass
core.attributesfile
-- 
2.13.0

Re: git-2.13.0: log --date=format:%z not working

[René Scharfe]

Am 27.05.2017 um 23:46 schrieb Jeff King:
> On Sat, May 27, 2017 at 06:57:08PM +0200, Ævar Arnfjörð Bjarmason wrote:
> 
>> There's another test which breaks if we just s/gmtime/localtime/g. As
>> far as I can tell to make the non-local case work we'd need to do a
>> whole dance where we set the TZ variable to e.g. UTC$offset, then call
>> strftime(), then call it again. Maybe there's some way to just specify
>> the tz offset, but I didn't find any in a quick skimming of time.h.
> 
> There isn't.
Right.  We could handle %z internally, though.  %Z would be harder (left
as an exercise for readers..).

First we'd have to undo 0a0416a3 (strbuf_expand: convert "%%" to "%"),
though, in order to give full control back to strbuf_expand callbacks.

2-pack patch:

--- >8 ---
 builtin/cat-file.c |  5 +
 convert.c  |  1 +
 daemon.c   |  3 +++
 date.c |  2 +-
 ll-merge.c |  5 +++--
 pretty.c   |  3 +++
 strbuf.c   | 39 ++-
 strbuf.h   | 11 +--
 t/t6006-rev-list-format.sh | 12 
 9 files changed, 63 insertions(+), 18 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 1890d7a639..9cf2e4291d 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -252,6 +252,11 @@ static size_t expand_format(struct strbuf *sb, const char 
*start, void *data)
 {
const char *end;
 
+   if (*start == '%') {
+   strbuf_addch(sb, '%');
+   return 1;
+   }
+
if (*start != '(')
return 0;
end = strchr(start + 1, ')');
diff --git a/convert.c b/convert.c
index 8d652bf27c..8336fff694 100644
--- a/convert.c
+++ b/convert.c
@@ -399,6 +399,7 @@ static int filter_buffer_or_fd(int in, int out, void *data)
struct strbuf path = STRBUF_INIT;
struct strbuf_expand_dict_entry dict[] = {
{ "f", NULL, },
+   { "%", "%" },
{ NULL, NULL, },
};
 
diff --git a/daemon.c b/daemon.c
index ac7181a483..191fac2716 100644
--- a/daemon.c
+++ b/daemon.c
@@ -148,6 +148,9 @@ static size_t expand_path(struct strbuf *sb, const char 
*placeholder, void *ctx)
case 'D':
strbuf_addstr(sb, context->directory);
return 1;
+   case '%':
+   strbuf_addch(sb, '%');
+   return 1;
}
return 0;
 }
diff --git a/date.c b/date.c
index 63fa99685e..d16a88eea5 100644
--- a/date.c
+++ b/date.c
@@ -246,7 +246,7 @@ const char *show_date(timestamp_t time, int tz, const 
struct date_mode *mode)
month_names[tm->tm_mon], tm->tm_year + 1900,
tm->tm_hour, tm->tm_min, tm->tm_sec, tz);
else if (mode->type == DATE_STRFTIME)
-   strbuf_addftime(, mode->strftime_fmt, tm);
+   strbuf_addftime(, mode->strftime_fmt, tm, tz);
else
strbuf_addf(, "%.3s %.3s %d %02d:%02d:%02d %d%c%+05d",
weekday_names[tm->tm_wday],
diff --git a/ll-merge.c b/ll-merge.c
index ac0d4a5d78..e6202c7397 100644
--- a/ll-merge.c
+++ b/ll-merge.c
@@ -172,7 +172,7 @@ static int ll_ext_merge(const struct ll_merge_driver *fn,
 {
char temp[4][50];
struct strbuf cmd = STRBUF_INIT;
-   struct strbuf_expand_dict_entry dict[6];
+   struct strbuf_expand_dict_entry dict[7];
struct strbuf path_sq = STRBUF_INIT;
const char *args[] = { NULL, NULL };
int status, fd, i;
@@ -185,7 +185,8 @@ static int ll_ext_merge(const struct ll_merge_driver *fn,
dict[2].placeholder = "B"; dict[2].value = temp[2];
dict[3].placeholder = "L"; dict[3].value = temp[3];
dict[4].placeholder = "P"; dict[4].value = path_sq.buf;
-   dict[5].placeholder = NULL; dict[5].value = NULL;
+   dict[5].placeholder = "%"; dict[5].value = "%";
+   dict[6].placeholder = NULL; dict[6].value = NULL;
 
if (fn->cmdline == NULL)
die("custom merge driver %s lacks command line.", fn->name);
diff --git a/pretty.c b/pretty.c
index 587d48371b..56872bfa25 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1428,6 +1428,9 @@ static size_t format_commit_item(struct strbuf *sb, /* in 
UTF-8 */
} magic = NO_MAGIC;
 
switch (placeholder[0]) {
+   case '%':
+   strbuf_addch(sb, '%');
+   return 1;
case '-':
magic = DEL_LF_BEFORE_EMPTY;
break;
diff --git a/strbuf.c b/strbuf.c
index 00457940cf..206dff6037 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -309,12 +309,6 @@ void strbuf_expand(struct strbuf *sb, const char *format, 
expand_fn_t fn,
break;
format = percent + 1;
 
-   if (*format == '%') {
-   strbuf_addch(sb, '%');
-   format++;
-   continue;
-   }
-
  

Re: [Non-Bug] cloning a repository with a default MASTER branch tries to check out the master branch

[Philip Oakley]

From: "Junio C Hamano" 

"Philip Oakley"  writes:


However given the discussion about an unborn HEAD, the option here
would be to also pass the NULL sha for the symref and then add the
annotation 'HEAD' after an extra \0, in the same way that an active
symref could be annotated with the '\0HEAD'. This would kill two birds
with one stone!


Are you aware of the symref capability that is already advertised in
the initial upload-pack response?  Right now, we do so only when
HEAD actually points at something, and the earlier suggestion by
Peff is to do so unconditionally, even when HEAD is dangling.


The suggestion is the otherway around. I would argue (as a viewpoint) that 
what we advertise are object IDs and their associated refs, sorted by ref 
name. (I'm thinking of the git/Documentation/technical/pack-protocol.txt 
here). My suggestion was that when we get to the sorted ref that HEAD points 
to (including the unborn oid) that we annotate that ref.


I didn't quite follow Peff's suggestion as to where the list change went and 
if that was a protocol change.


There are two current fault scenarios.
a. The currently reported case where HEAD has an unborn symref
b. The multiple ref HEAD case, where the HEAD oid matches multiple 
advertised refs, and the correct symref is not the first listed (which is 
the case I had looked at a few years ago, a prompted me to respond).



With the above discussions, we would have both a NULL oid for the unborn 
(sym)ref sent (if needed), and the (sym)ref for HEAD would have the extra 
annotation. That would at least not break the protocol rule that "If HEAD is 
not a valid ref, HEAD MUST NOT appear in the advertisement list at all" (it 
is now an annotation to another valid ref [or the unborn symref]).


Existing clients that are symref aware do not do anything (good or
bad) when a HEAD that is dangling [*1*] is advertised, so such a
change will not hurt (but it would not help by itself either).
Ancient clients that are not even aware of the symref are not
affected.

Then new clients _could_ start paying attention to the advertised
HEAD that is dangling.


My main step was that for case b, so that we don't need to 
guess_remote_head(). The annotation would have already told us.




The bundle transport is a different beast.  I do not think it
advertises where HEAD is pointing at, whether it is dangling or
not.


My understanding was that the Bundle was a tiny wrapper and that it has that 
same protocol header, which was then decoded using the same code. Hence the 
hope that it could fix both the bundle and remote clone problems. I'd just 
avoided stepping into remote clone arena because of other implementations.



[Footnote]

*1* A HEAD symref that is advertised in the upload-pack response is
   dangling when its pointee does not appear in the set of refs
   that are advertised.  Félix's case would have shown HEAD
   pointing at refs/heads/master in the symref capability extension,
   but the list of refs and their values would not have included
   that ref (there was only refs/heads/MASTER "for joke reasons").


I hope I haven't confused the different parts of the negotiation and 
transfer, which can be confusing.


Philip 

Re: Missing: Consistency of clean state output of "git add -i"

[Philip Oakley]

From: "Kaartic Sivaraam" 

I guess I'll take back my note in the previous email that says, I could
help. I saw the implementation and found that I couldn't help as I
don't have experience with PERL. My bad.


I wouldn't let that stop you. We were all ignorant once.
I know little of tcl (gitk/git-gui), but I've still managed to fix a couple 
of issues, with the help of others on the list (and the search engines and 
their results;-)


Perl is *that* hard, is it?

regards
Philip 

Re: git-2.13.0: log --date=format:%z not working

[Ævar Arnfjörð Bjarmason]

On Sat, May 27, 2017 at 11:46 PM, Jeff King  wrote:
> On Sat, May 27, 2017 at 06:57:08PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> There's another test which breaks if we just s/gmtime/localtime/g. As
>> far as I can tell to make the non-local case work we'd need to do a
>> whole dance where we set the TZ variable to e.g. UTC$offset, then call
>> strftime(), then call it again. Maybe there's some way to just specify
>> the tz offset, but I didn't find any in a quick skimming of time.h.
>
> There isn't. At least on _some_ platforms, the zone information is
> embedded in "struct tm" and stored by gmtime() and localtime(), but the
> fields aren't publicly accessible. Which is why your patch worked for
> format-local (it swaps out gmtime() for localtime() which sets those
> fields behind the scenes). But:
>
>   - I'm not sure that's guaranteed by the standard; strftime() might get
> its zone information elsewhere (if it needs to reliably distinguish
> between gmtime() and localtime() results it has to at least set a
> bit in the "struct tm", but that bit may not be the full zone info).
>
>   - Even if it does work, you're stuck with only the local timezone. In
> theory you could temporarily tweak the process's timezone, call
> localtime(), and then tweak it back. I was never able to get that to
> work (links below).
>
> On glibc, at least, you can access the zone fields in "struct tm" by
> compiling with _DEFAULT_SOURCE.
>
> So I think the best we could do is probably to have a feature macro like
> TM_HAS_GMTOFF, and set tm->tm_gmtoff and tm->tm_zone on platforms that
> support it. I'm not sure what we'd put in the latter, though; we don't
> actually have the timezone name at all (we just have "+0200" or whatever
> we parsed from the git object, but that would be better than nothing).
>
> That leaves other platforms still broken, but like I said, I don't think
> there's a portable solution.
>
> Here are some links to past explorations:
>
>   http://public-inbox.org/git/20160208152858.ga17...@sigill.intra.peff.net/
>
>   http://public-inbox.org/git/87vb2d37ea@web.de/

There's a third and possibly least shitty option that isn't covered in
those threads; We could just make a pass over the strftime format
ourselves and replace %z and %Z with the timezone (as done for
DATE_ISO8601_STRICT in date.c), then hand the rest off to strftime().

I'm not going to pursue it though, Ulrich, are you maybe interested in
hacking on that?

Re: mergetool: what to do about deleting precious files?

[Philip Oakley]

From: "Junio C Hamano" 

"Philip Oakley"  writes:

The git book [1] and a few blog posts [2] show how to preserve files 
which

are in the current branch against changes that are on the branch being
merged in.

e.g. (from [2])

echo ' merge=ours' >> .gitattributes && # commit
git config --global merge.ours.driver true

(test) $ git checkout demo
(demo) $ git merge -
#  contents are not merged and the original retained.



However what is not covered (at least in the documentation ) is the case
where the file to be ignored is not present on the current branch, but is
present on the branch to be merged in.


Hmph.  Per-path 'ours' and 'theirs' kick in only after we decide to
perform the content level three-way merge.  I wonder what would (not
"should", but "would with the current code") happen, with the same
attribute setting, if the file being merged were not changed by ours
but modified by the side branch?  I suspect that we'd take the change
made by the side branch.


Here the 'ours' strategy is defined by the user's config file merge driver 
list.


I'd understood it that once it was decided there was a merge to be performed 
(the repective blob oid's in the repo/index are different) that the problem 
of merging is then handed off to the declared merge driver.





Normal expectations would be that in such a case the new file from the
second parent branch would be added to the current branch.




The git-scm and blog posts suggest that the original is left in place at the 
%P path, the merge driver run, and its return values used to decide if the 
user has to go and resolve conflicts. By setting the driver to 'true', the 
result is then said to be that the current 'blob' (i.e. file) is accepted 
unchanged (in %P), so anything from the second parent blob was completely 
ignored.


However if we have the addition of a new file, I can't tell from the docs 
what should happen? Is this still a merge such that the merge driver is 
called, or is the added file accepted without recourse to its .gitattributes 
setting (surely that would be a bug).


Then assuming we have reached an external driver, and it wants to not add 
that very file that was added in the second parent branch, what does the %P 
path point to (/dev/null?) - in particular, shouldn't the docs say? (I've 
not tested, and one test is not proof)


It maybe that the user wants a merge driver that says "If I ever see a 
secret key or password, then remove the whole file", which (removing the 
file from the merge) is a currently undocumented process (if even possible).



So I do not think this is not limited to "new file".  Anything that
a tree-level three-way merge would resolve cleanly without having to
consult the content-level three-way merge will complete without
consulting the merge.ours.driver; per-file content-level three-way
merge driver (which is what merge= mechanism lets you
specify via the attributes mechanism) is not something you would
want to use for this kind of thing.  It is purely for resolving the
actual content-level conflicts.

That (that Git knows best) sounds just wrong. If the user has set a file 
attribute strategy, why would we ignore it? We already have different 
internal strategies anyway, so how do we even know that the potential merge 
was conflict free if we have haven't checked its attribute type. Maybe I'm 
missing something.

--
Philip 

Re: Missing: Consistency of clean state output of "git add -i"

[Kaartic Sivaraam]

I guess I'll take back my note in the previous email that says, I could
help. I saw the implementation and found that I couldn't help as I
don't have experience with PERL. My bad.

Missing: Consistency of clean state output of "git add -i"

[Kaartic Sivaraam]

Hello all,
When the "git add -i" command is triggered with a clean working
directory and index, the outputs of the various options don't seem to
be giving consistent results. A few of the distinct outputs are,

1. No output, the options are displayed
2. A single blank line and the options are displayed
3. The "add untracked" option, prints "No untracked files." and adds a
empty line and prints the options
4. The "patch" option, prints "No changes" and prints the options

It seems that the clean state output should be improved for the sake of
a consistent user interface. 

Note: I could possibly help if I were pointed to the implementation.

Quote: "We hate most in others that which we fail to see in ourselves."
- Anil Dash
--
Regards,
Kaartic