Re: "git add -p" versus "git add -i", followed by "p"

[Kevin Daudt]

On Sun, Dec 02, 2018 at 11:30:19AM -0500, Robert P. J. Day wrote:
> 
>   testing adding by patch for the very first time (i've just never
> needed this), and reading the "progit" book and reading the man page,
> and the impression i'm getting is that running "git add -p" (going
> straight to patch mode) is supposed to be equivalent to running "git
> add -i", then typing "p" to switch to patch mode.
> 
>   that is most emphatically not what i'm seeing. if i run "git add
> -p", then i get to what i expect -- the patch subsystem:
> 
>   $ git add -p
>   diff --git a/README.asc b/README.asc
>   index fa40bad..840e85b 100644
>   --- a/README.asc
>   +++ b/README.asc
>   @@ -1,3 +1,9 @@
>   +change 1
>   +
>   +
>   +
>   +
>   +
>= Pro Git, Second Edition
> 
>Welcome to the second edition of the Pro Git book.
>   Stage this hunk [y,n,q,a,d,j,J,g,/,e,?]?
> 
> but if i start with "git add -i", there seems to be no way to get to
> patch mode -- certainly "p" doesn't do it. am i stupidly missing
> something trivial? is the explanation misleading or inncomplete?
> 
> rday
> 
> -- 
> 

After selecting 'p', what do you get?

You should see a list of modified files. You can select the files you
want to stage by the listed numbers. After you selected those files, you
press enter, and then you will get the options you'll also see with git
add -p.

Kevin

Re: Understanding Index Header

[Kevin Daudt]

On Sat, Oct 06, 2018 at 05:13:45PM -0400, Farhan Khan wrote:
> Hi all,
> 
> I am writing a program that parses out the .git/index file. I am
> reading the git index header documentation, but I seem to be getting
> jibberish data.
> https://github.com/git/git/blob/master/Documentation/technical/index-format.txt
> 
> The first 12 bytes are the signature, version and entries. Great so far.
> 
> Afterwards, I try to read the extension signature, which the
> documentation says: "4-byte extension signature. If the first byte is
> 'A'..'Z' the
> extension is optional and can be ignored.". I am getting jibberish. Is
> this expected?
> 
> Then, the extension size comes back as 3556 (after ntohl()). That
> seems extremely high. My structure is as follows:
> 
> struct _indexfile_hdr {
> unsigned char   sig[4]; /* Always "DIRC" */
> uint32_tversion;/* Version Number */
> uint32_tentries;/* Number of extensions */
> unsigned char   extsig[4];  /* Extension signature */
> uint32_textsize;/* Size of the extension */
> uint8_t sha[8]; /* SHA1 of index before checksum */
> } __packed;
> 
> Am I doing something wrong? Is there some offset or padding that I missed?
> 
> Thanks,
> --
> Farhan Khan

Hello Farhan

Between the index header and the extensions there are index entries. So
you have 4 sections:

1. Header (fixed size)
2. Index entries (dynamic size, amount stored in the index)
3. Extensions (dynamic size)
4. hash

So you cannot capture the index with just a single struct. Also what you
call the 'entries' field is not the number of extensions, but the number
of index entries.

I'm not sure why you are seeing so many index entries. I've just tested
it myself on the git repository and I got 3419 entries when reading in
network (big-endian) order.

Hope this helps a bit.

Kevin

Re: Mailsplit

[Kevin Daudt]

On Wed, Sep 05, 2018 at 09:17:29PM -0700, Stephen & Linda Smith wrote:
> I thought I would use "git mailsplit" to split a mbox file (which downloaded 
> from public inbox) so that I could attemp to resurrect a patch series for 
> from 
> a year ago.  
> 
> The motivation is that I downloaded the series [1] and applied to a tag from 
> about the time period that the patch was sent out [2].  
> 
> The "git am -3 patch.mbox  quit 2/3 of the way though.   I resolved the fix. 
> and ran "git am --continue" which didn't apply the rest of the patches in the 
> mbox.
> 
> So two questions:
> 1)  why would git version 2.18.0 not appear to continue applying the patches. 
>   
> 
> 2) where do I find the command "git mailsplit".   The onlything in my 
> installed tree is:
> 
>   $ find  /usr/local/ -name '*mailsplit*'
>   /usr/local/share/doc/git-doc/git-mailsplit.txt
>   /usr/local/share/doc/git-doc/git-mailsplit.html
>   /usr/local/share/man/man1/git-mailsplit.1
>   /usr/local/libexec/git-core/git-mailsplit

This is the mailsplit command, and should be executed when running `git
mailsplit`. What does git --exec-dir return?

Kevin

Re: Adding nested repository with slash adds files instead of gitlink

[Kevin Daudt]

On Mon, Jun 18, 2018 at 01:19:19PM +0200, Heiko Voigt wrote:

Now with cc to the mailing list.

> Hi,
> 
> I just discovered that when you have a slash at the end of a nested
> repository, the files contained in the repository get added instead of
> the gitlink.
> 
> I found this when I was adding a submodule and wanted to commit a small
> change before that. You get the slash by using tab autocompletion.
> 
> Here is a recipe to reproduce:
> 
> mkdir test
> cd test; git init
> touch a; git add a; git commit -m a
> mkdir ../test.git; (cd ../test.git; git init --bare)
> git remote add origin ../test.git
> git push origin master
> git submodule add ../test.git submodule
> git reset
> git add submodule/
> 
> Now instead of just submodule gitlink there is an entry for submodule/a
> in the index.
> 
> I just thought I put this out there. Will have a look if I find the time
> to cook up a proper testcase and investigate.
> 
> Cheers Heiko

This has been the case as far as I can remember, and is basically lore
in the #git irc channel).

This can also be reproduced by just cloning a repo inside another repo
and running `git add path/`.

Re: Weird revision walk behaviour

[Kevin Bracey]

On 31/05/2018 08:43, Jeff King wrote:


If there are zero parents (neither relevant nor irrelevant), is it still
TREESAME? I would say in theory yes.


Not sure - I think roots are such a special case that TREESAME 
effectively doesn't matter. We always test for roots first.

  So what I was proposing would be to
rewrite the parents to the empty set.
That feels a bit radical - I believe we need to retain (some) parent 
information for modes that show it (eg the dangling unfilled circles in 
gitk). And making it a root I think could cause other problems with 
making it look like we have a disjoint history. I believe the next 
simplification step may be trying to follow down to the common root.

What next here? It looks like we have a proposed solution. Do you want
to try to work up a set of tests based on what you wrote earlier?
I was hoping Gábor would carry on, as he's made a start... I was just 
planning to back-seat drive.

I'd also love to hear from Junio as the expert in this area, but I think
he's been a bit busy with maintainer stuff recently. So maybe I should
just be patient. :)

Likewise - I have been quite deep into this, but it was a quite short 
window of investigation a long time ago, and I've not looked at it 
since. Would like input from someone with more active knowledge.


Kevin

Re: Weird revision walk behaviour

[Kevin Bracey]

On 30/05/2018 00:04, Jeff King wrote:


Do we even need to do the parent rewriting here? By definition those
parents aren't interesting, and we're TREESAME to whatever is in
treesame_parents. So conceptually it seems like we just need a flag "I
found a treesame parent", but we only convert that into a TREESAME flag
if there are no relevant parents.


I think it's necessary to make the rules consistent. To mark the commit 
as TREESAME here when it's not TREESAME to all its parents would be 
inconsistent with the definition of the TREESAME flag used everywhere else:


* Original definition: "A commit is TREESAME if it is treesame to any 
parent"

* d0af66 definition: "A commit is TREESAME if it is treesame to all parents"
* Current 4d8266 definition: "A commit is TREESAME if it is treesame to 
all relevant parents; if no relevant parents then if it is treesame to 
all (irrelevant) parents."


The current problem is that the node is not marked TREESAME, but that's 
consistent with the definition. I think we do have to rewrite the commit 
so it is TREESAME as per the definition. Not flag it as TREESAME in 
violation of it.


It's possible you *could* get away with just flagging, because we never 
recompute the TREESAME flag in simple history mode. But it would be a 
cheat, and it may have other side effects. It means this node would 
remain a special rare case for others to trip up on later.  And I don't 
think it simplifies the scan. Remembering 
"pointer-to-first-treesame-parent" (not a list) for the rewrite is no 
more complex than remembering "bool-there-was-a-treesame-parent".  (A 
bool is what earlier code did - it worked for the original TREESAME 
definition. My patch series dropped that bool without replacement - 
missing this all-irrelevant case).


In the simple history mode, the assumption is we're "simplifying away 
merges up-front" here; we won't (and can't) rewrite parents later in a 
way that needs to recompute TREESAME. In the initial scan when all 
parents are relevant and we matched one, the commit became TREESAME as 
per the new definition immediately because of the rewrite.  This applies 
the equivalent rewrite when no relevant parents, consistent with the 
general concept, and without changing the TREESAME definition.


Kevin

Re: Weird revision walk behaviour

[Kevin Bracey]

On 29/05/2018 01:06, SZEDER Gábor wrote:


So, without investing nearly enough time to understand what is going
on, I massaged the above diffs into this:

Cool.


+   treesame_parents = 
xmalloc(sizeof(*treesame_parents));
There's no need to actually record a list here. This is just for the 
simple history. We are only interested into becoming a non-merge to 1 
treesame parent, so I think we just need to record a pointer to the 
first one we see, just as this would exit immediately for the first 
relevant one. For the full-history case, we already have the full "which 
parents are treesame" recording mechanism just above, but it only kicks 
in for merge commits and only when settings require it. Adding a malloc 
here would be significant machinery overhead.

FWIW, the test suite passes with the above patch applied.
I doubt there's an existing case like this anywhere in the revision test 
suite :) . And this patch is focused enough that it *should* only be 
changing the behaviour of this very specific case. As such, it does feel 
a little like a kludge, but I think it's fine because it's aligning the 
simple-history analysis with the "analyse relevant parents if any, else 
analyse irrelevant" rule of the full-history.


And here is the small PoC test case to illustrate the issue, which
fails without but succeeds with the above patch.  Eventually it should
be part of 't6012-rev-list-simplify.sh', of course, but I haven't
looked into that yet.
It may be there's enough criss-crossy history to test here to merit 
breaking out into a second test series.


+#   B---M2   master
+#  / \ /
+# A   X
+#  \ / \
+#   C---M1   b2
+#
+# and modify 'file' in commits 'A' and 'B', so one of 'M1's parents
+# ('B') is TREESAME wrt. 'file'.

I guess we'll be wanting test cases for A..B2, B..B2 and C..B2, and some 
where the the base is "some other child of B or C".  "B..B2" is no 
longer a pure set subtraction for simplification as B is UNINTERESTING 
(ie not in the set) but RELEVANT (because you named it as a bottom 
commit), so B..B2 actually still leaves M1 with 2 relevant parents. 
You'd want test cases covering B relevant+C irrelevant and B 
irrelevant+C relevant, which means subtracting them without naming them 
- so name a child of one.


And then we need to think about whether we want it displayed in each of 
the other modes for each of those queries...


Kevin

Re: Weird revision walk behaviour

[Kevin Bracey]

On 24/05/2018 23:26, Kevin Bracey wrote:



On Wed, May 23, 2018 at 07:10:58PM +0200, SZEDER Gábor wrote:


   $ git log --oneline master..ba95710a3b -- ci/
   ea44c0a594 Merge branch 'bw/protocol-v2' into 
jt/partial-clone-proto-v2


In this case, we're hitting a merge commit which is not on master, but 
it has two parents which both are. Which, IIRC, means the merge commit 
is INTERESTING with two UNINTERESTING parents; and we are TREESAME to 
only one of them.


The commit changing the logic of TREESAME you identified believes that 
those TREESAME changes for merges which were intended to improve 
fuller history modes shouldn't affect the simple history "because 
partially TREESAME merges are turned into normal commits". Clearly 
that didn't happen here.


Haven't currently got a development environment set up here, but I've 
been looking at the code.Here's a proposal, untested, as a potential 
starting point if anyone wants to consider a proper patch.


The simplify_history first-scan logic never actually turned merges into 
simple commits unless they were TREESAME to a relevant/interesting 
parent.  Anything where the TREESAME parent was UNINTERESTING was 
retained as a merge, but had its TREESAME flag set, and that permitted 
later simplification.


With the redefinition of the TREESAME flag, this merge commit is no 
longer TREESAME, and as the decoration logic to refine TREESAME isn't 
active for simplify_history, it doesn't get cleaned up (even if it would 
be in full history?)


I think the answer may be to add an extra post-process step on the 
initial loop to handle this special case. Something like:


        case REV_TREE_SAME:
            if (!revs->simplify_history || !relevant_commit(p)) {
                /* Even if a merge with an uninteresting
                 * side branch brought the entire change
                 * we are interested in, we do not want
                 * to lose the other branches of this
                 * merge, so we just keep going.
                 */
                if (ts)
                    ts->treesame[nth_parent] = 1;
+   /* But we note it for potential later simplification */
+       if (!treesame_parent)
+    treesame_parent = p;
    continue;
 }

...

After loop:

+ if (relevant_parents == 0 && revs->simplify_history && 
treesame_parent) {
+   treesame_parent->next = NULL;// Repeats code from loop - 
share somehow?

+   commit->parents = treesame_parent;
+   commit->object.flags |= TREESAME;
+   return;
+    }

 /*
  * TREESAME is straightforward for single-parent commits. For merge

The other option would be to take off the " || !relevant_commit(p)" 
test, but I'm assuming that is still needed for other cases.


Kevin

Re: Weird revision walk behaviour

[Kevin Bracey]

On 23/05/2018 20:35, Jeff King wrote:

On Wed, May 23, 2018 at 01:32:46PM -0400, Jeff King wrote:


On Wed, May 23, 2018 at 07:10:58PM +0200, SZEDER Gábor wrote:


   $ git log --oneline master..ba95710a3b -- ci/
   ea44c0a594 Merge branch 'bw/protocol-v2' into jt/partial-clone-proto-v2


I keep some older builds around, and it does not reproduce with v1.6.6.3
(that's my usual goto for "old"). Bisecting turns up d0af663e42
(revision.c: Make --full-history consider more merges, 2013-05-16).  It
looks like an unintended change (the commit message claims that the
non-full-history case shouldn't be affected).

There's more discussion in the thread at:

   
https://public-inbox.org/git/1366658602-12254-1-git-send-email-ke...@bracey.fi/

I haven't absorbed it all yet, but I'm adding Junio to the cc.



In this case, we're hitting a merge commit which is not on master, but 
it has two parents which both are. Which, IIRC, means the merge commit 
is INTERESTING with two UNINTERESTING parents; and we are TREESAME to 
only one of them.


The commit changing the logic of TREESAME you identified believes that 
those TREESAME changes for merges which were intended to improve fuller 
history modes shouldn't affect the simple history "because partially 
TREESAME merges are turned into normal commits". Clearly that didn't 
happen here.


I think we need to look at why that isn't happening, and if it can be 
made to happen. The problem is that this commit is effectively the base 
of the graph - it's got a double-connection to the UNINTERESTING set, 
and maybe that prevented the simple history "follow 1 TREESAME" logic 
from kicking in. Maybe it won't follow 1 TREESAME to UNINTERESTING.


I know there were quite a few changes later in the series to try to 
reconcile the simple and full history, for the cases where the simple 
history takes a weird path because of its love of TREESAME parents, 
hiding evil merges. But I believe the simple history behaviour was 
supposed to remain as-is - take first TREESAME always.


Kevin

Re: Weird revision walk behaviour

[Kevin Bracey]

On 23/05/2018 20:35, Jeff King wrote:

There's more discussion in the thread at:

   
https://public-inbox.org/git/1366658602-12254-1-git-send-email-ke...@bracey.fi/

I haven't absorbed it all yet, but I'm adding Junio to the cc.


Just to ack that I've seen the discussion, but I can't identify the 
code's reasoning at the moment. My recollection is that I accepted while 
coming up with the algorithm that it might err slightly on the side of 
false positives in the display - there were some merge cases I was 
unable to fully distinguish whether or not the merge had lost a change 
it shouldn't have done, and if I was uncertain I'd rather show it than not.


The first commit was not originally intended to alter behaviour for 
anything other than --full-history, but later in the chain there was 
specific consideration into tracking the path to the specified "bottom" 
commit. It may be that's part of what's happening here.


Kevin

Re: BUG: No way to set fsck. when cloning

[Kevin Daudt]

On Thu, May 24, 2018 at 05:25:29PM +0200, Ævar Arnfjörð Bjarmason wrote:
> When I do:
> 
> git -c fetch.fsckObjects=true clone 
> g...@github.com:robbyrussell/oh-my-zsh.git
> 
> I get:
> 
> error: object 2b7227859263b6aabcc28355b0b994995b7148b6: 
> zeroPaddedFilemode: contains zero-padded file modes
> fatal: Error in object
> fatal: index-pack failed
> 
> The docs (https://git-scm.com/docs/git-config#git-config-fsckltmsg-idgt)
> say you can override this with -c fsck.zeroPaddedFilemode = ignore, but
> I see in builtin/fsck.c that just fsck_config() knows about this, and
> indeed this works *after* you clone the repo when you use 'git fsck'.
> 
> I don't have time to fix this now, but what's the best approach here?
> Make all the relevant commands copy what fsck_config() is doing, or
> should fsck_object() be lazily looking up this config by itself?

Apparently someone reported this earlier[0]. Johannes replied:

>  Well, you can apparently have your cake and eat it too (see
> https://git-scm.com/docs/git-config#git-config-receivefsckltmsg-idgt):
> 
> receive.fsck.::
> When `receive.fsckObjects` is set to true, errors can be switched
> to warnings and vice versa by configuring the `receive.fsck.`
> setting where the `` is the fsck message ID and the value
> is one of `error`, `warn` or `ignore`. For convenience, fsck prefixes
> the error/warning with the message ID, e.g. "missingEmail: invalid
> author/committer line - missing email" means that setting
> `receive.fsck.missingEmail = ignore` will hide that issue.
> 
> In your case, use receive.fsck.zeroPaddedFilemode=ignore=warn (or
> =ignore).

[0]https://public-inbox.org/git/alpine.deb.2.21.1.1801042125430...@minint-6bku6qn.europe.corp.microsoft.com/

Hope this helps, Kevin.

Re: Git push error due to hooks error

[Kevin Daudt]

On Mon, May 14, 2018 at 05:44:06PM +, Barodia, Anjali (Nokia - IN/Noida) 
wrote:
> Hi Support,
> 
> 
> I was trying to push local git to another git on gerrit, but stuck with an 
> hook error.
> This is a very large repo and while running the command "git push origin 
> --all"
> I am getting this errors:
> 
> remote: (W) 92e19d4: too many commit message lines longer than 70 characters; 
> manually wrap lines
> remote: (W) de2245b: too many commit message lines longer than 70 characters; 
> manually wrap lines
> remote: (W) dc6e982: too many commit message lines longer than 70 characters; 
> manually wrap lines
> remote: (W) d2e2efd: too many commit message lines longer than 70 characters; 
> manually wrap lines
> remote: error: internal error while processing changes To 
> ssh_url_path:8282/SI_VF.git
>  ! [remote rejected]   master -> master (Error running hook 
> /opt/gerrit/hooks/ref-update)
> error: failed to push some refs to 'ssh_user@url_path:8282/SI_VF.git'
> 
> I tried to push after deleting the .git/hooks, but still get the same error.
> Please help me in fixing this error.
> 
> Thanks with Regards,
> Anjali Barodia

Did you remove the hook from the remote repo, or the local repo? Because
this is a hook which runs on the repo you are pushing too. Something
like pre-receive or pre-update is causing this.

Kevin

Re: [PATCH 1/3] checkout.c: add strict usage of -- before file_path

[Kevin Daudt]

On Sat, May 12, 2018 at 08:23:32PM -0600, Dannier Castro L wrote:
> Currently,  is a complex command able to handle both
> branches and files without any distintion other than their names,
> taking into account that depending on the type (branch or file),
> the functionality is completely different, the easier example:
> 
> $ git checkout   # Switch from current branch to .
> $ git checkout # Restore  from HEAD, discarding
>  # changes if it's necessary.
> $ git checkout --  # The same as the last one, only with an
>  # useless '--'.
> 
> For GIT new users, this complicated versatility of  could
> be very confused, also considering that actually the flag '--' is
> completely useless (added or not, there is not any difference for
> this command), when the same program messages promote the use of
> this flag.
> 
> Regarding the 's power to overwrite any file, discarding
> changes if they exist without some way of recovering them, the
> solution propuses that the usage of '--' is strict before to
> specify the file(s) path(s) for any  command (including
> all types of flags), as a "defense barrier" to make sure about
> user's knowledge and intension running .
> 
> The solution consists in detect '--' into command args, allowing
> the discard of changes and considering the following names as
> file paths, otherwise, they are branch names.
> 
> Signed-off-by: Dannier Castro L <dannie...@gmail.com>

One data point indicating this is giving issues is that today on IRC a
user was confused why `git checkout pt` did not show any message and did
not checkout a remote branch called 'pt' as they expected. It turned out
they also had a local file/dir called 'pt', which caused git to checkout
that file/dir rather than creating a local branch based on the remote
branch.

Kevin

Re: Git enhancement request - checkout (clone) set modified dates to commit date

[Kevin Daudt]

On Sun, Apr 22, 2018 at 03:01:10PM -0400, Andrew Wolfe wrote:
> Hi Brian,
> 
> Not completely sure what you're saying.  If the files on master are
> not changed, the changed files' commit timestamps will be older than
> the branch commit timestamps.
> 
> However, if I check out master after committing to a branch, the
> modifications will necessarily disappear because they haven't been
> committed to master.  Instead, under my proposal, each will get the
> timestamp of its prior commit.
> 

Say I build the project while on a certain branch. Then I checkout a
different branch and build again. You would expect that the targets of
every source file that have changed are rebuilt.

When you would restore the timestamp back to the commit timestamp, the
timestamp will be older then the target, and make will not rebuild it,
leaving you with outdated build targets.

Re: "git branch" issue in 2.16.1

[Kevin Daudt]

On Thu, Feb 08, 2018 at 12:27:07PM +0100, Lars Schneider wrote:
> 
> > On 08 Feb 2018, at 12:13, Lars Schneider  wrote:
> > 
> > 
> >> On 08 Feb 2018, at 09:50, Jeff King  wrote:
> >> 
> >> On Wed, Feb 07, 2018 at 11:20:08PM +0100, Lars Schneider wrote:
> >> 
>  1. You have $LESS in your environment (without "F") on one platform
>    but not the other.
> >>> 
> >>> I think that's it. On my system LESS is defined to "-R".
> >>> 
> >>> This opens the pager:
> >>> 
> >>>   $ echo "TEST" | less
> >>> 
> >>> This does not open the pager:
> >>> 
> >>>   $ echo "TEST" | less -FRX
> >>> 
> >>> That means "F" works on macOS but Git doesn't set it because LESS is
> >>> already in my environment.
> >>> 
> >>> Question is, why is LESS set that way on my system? I can't find
> >>> it in .bashrc .bash_profile .zshrc and friends.
> >> 
> >> There's also /etc/bash.bashrc, /etc/profile, etc. I don't know what's
> >> normal in the mac world. You can try running:
> >> 
> >> bash -ix 2>&1  >> 
> >> to see what your startup code is doing. I don't know of a good way to
> >> correlate that with the source files, though. Or even to ask bash which
> >> startup files it's looking in.
> > 
> > Unfortunately, this command doesn't work for me.
> > 
> > I ask around and most of my coworkers have LESS="-R".
> > Only the coworker that doesn't really use his Mac and has
> > no customizations does not have $LESS defined.
> > 
> > Therefore, I think it is likely some third party component
> > that sets $LESS.
> > 
> > @Jason:
> > Do you have homebrew, iTerm2, and/or oh-my-zsh installed?
> 
> Ha. I found it it! It is indeed oh-my-zsh:
> https://github.com/robbyrussell/oh-my-zsh/blob/master/lib/misc.zsh#L23
> 
> Let's see if oh-my-zsh is willing to change that...
> 
> - Lars

I've just added unset LESS in my .zshrc, but for most users it would be
better if they don't set it at all.

Re: [BUG] git remote prune removes local tags, depending on fetch config

[Kevin Daudt]

On Tue, Jan 16, 2018 at 12:48:32PM +0100, Andreas Schwab wrote:
> On Jan 15 2018, Michael Giuffrida  wrote:
> 
> > It doesn't seem like a useful feature -- you wouldn't expect `git
> > fetch --prune` to remove your local branches that you were developing
> > on, right?
> 
> Don't mix local and remote refs.  There is a reason why remote tracking
> branches are put in a separate name space.  If you fetch the remote tags
> into a separate name space (eg. refs/remote/tags/*:refs/tags/*) then
> there is no conflict.
> 
> Andreas.

But then they are no longer considered tags, but remote tracking
branches. Tools like git tag and git describe won't consider them, and
git branch -r would show them as remote tracking branches.

> 
> -- 
> Andreas Schwab, sch...@linux-m68k.org
> GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
> "And now for something completely different."

Re: [PATCH] Add shell completion for git remote rm

[Kevin Daudt]

On Wed, Jan 17, 2018 at 07:44:19AM +0700, Duy Nguyen wrote:
> On Tue, Jan 16, 2018 at 10:57:34AM -0800, Junio C Hamano wrote:
> > Duy Nguyen  writes:
> > 
> > > On Tue, Jan 16, 2018 at 4:43 AM, Keith Smiley  wrote:
> > >> So it sounds like either we should deprecate rm, or I should update the 
> > >> patch to the suggested method where we just complete remotes, but not rm 
> > >> in the list of completions.
> > >
> > > I vote for deprecation. I could send a patch to start warning to
> > > switch from 'rm' to 'remove'. Then perhaps we can delete it after two
> > > releases. It's not classified as plumbing should we don't have worry
> > > too much about scripts relying on 'remote rm'.
> > 
> > I do not know about "two releases" part (which amounts to merely
> > 20-24 weeks), but looking at "git remote -h" output and seeing that
> > we do spell out "rename" (instead of saying "mv" or something cryptic),
> > it probably makes sense to remove "rm" some time in the future.
> > 
> > I actually do think "rm" is _already_ deprecated.  
> > 
> > "git remote --help" does not mention it in its synopsis section and
> > it merely has ", rm" after "remove" as if an afterthought.
> 
> It's actually my bad. I should have replaced 'rm' with 'remove' in
> git-remote.txt in e17dba8fe1 (remote: prefer subcommand name 'remove'
> to 'rm' - 2012-09-06)
> 
> > I am not sure if it is worth being more explicit, perhaps like this?
> 
> Looks good. If we want to be more careful, we can follow up with
> something even more annoying like this before removing 'rm'
> 
> -- 8< --
> diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt
> index 577b969c1b..0a544703e6 100644
> --- a/Documentation/git-remote.txt
> +++ b/Documentation/git-remote.txt
> @@ -90,7 +90,6 @@ In case  and  are the same, and  is a file 
> under
>  the configuration file format.
>  
>  'remove'::
> -'rm'::
>  
>  Remove the remote named . All remote-tracking branches and
>  configuration settings for the remote are removed.
> diff --git a/builtin/remote.c b/builtin/remote.c
> index d95bf904c3..774ef6931e 100644
> --- a/builtin/remote.c
> +++ b/builtin/remote.c
> @@ -1609,7 +1609,10 @@ int cmd_remote(int argc, const char **argv, const char 
> *prefix)
>   result = add(argc, argv);
>   else if (!strcmp(argv[0], "rename"))
>   result = mv(argc, argv);
> - else if (!strcmp(argv[0], "rm") || !strcmp(argv[0], "remove"))
> + else if (!strcmp(argv[0], "rm")) {
> + warning(_("'rm' is a deprecated synonym. Use 'remove' 
> instead."));
> + result = rm(argc, argv);
> + } else if (!strcmp(argv[0], "remove"))
>   result = rm(argc, argv);
>   else if (!strcmp(argv[0], "set-head"))
>   result = set_head(argc, argv);
> -- 8< --
> 
> PS. This also makes me thing about supporting subcommand aliases, so
> that people can add back 'git remote rm' if they like (or something
> like 'git r rm' when they alias 'remote' as well). Which is probably a
> good thing to do. Long command names are fine when you have completion
> support, they are a pain to type otherwise.
> 

Couldn't they just create an alias like git rrm then, if they use it so
often that it becomes an issue? Having another layer of aliases does
create a lot of complexity.

> --
> Duy

Re: [PATCH] sha1_file: remove static strbuf from sha1_file_name()

[Kevin Daudt]

On Tue, Jan 16, 2018 at 08:18:14AM +0100, Christian Couder wrote:
> Using a static buffer in sha1_file_name() is error prone
> and the performance improvements it gives are not needed
> in most of the callers.
> 
> So let's get rid of this static buffer and, if necessary
> or helpful, let's use one in the caller.

Missing sign-off

> ---
>  cache.h   |  8 +++-
>  http-walker.c |  6 --
>  http.c| 16 ++--
>  sha1_file.c   | 38 +-
>  4 files changed, 42 insertions(+), 26 deletions(-)
> 
> diff --git a/cache.h b/cache.h
> index d8b975a571..6db565408e 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -957,12 +957,10 @@ extern void check_repository_format(void);
>  #define TYPE_CHANGED0x0040
>  
>  /*
> - * Return the name of the file in the local object database that would
> - * be used to store a loose object with the specified sha1.  The
> - * return value is a pointer to a statically allocated buffer that is
> - * overwritten each time the function is called.
> + * Put in `buf` the name of the file in the local object database that
> + * would be used to store a loose object with the specified sha1.
>   */
> -extern const char *sha1_file_name(const unsigned char *sha1);
> +extern void sha1_file_name(struct strbuf *buf, const unsigned char *sha1);
>  
>  /*
>   * Return an abbreviated sha1 unique within this repository's object 
> database.
> diff --git a/http-walker.c b/http-walker.c
> index 1ae8363de2..07c2b1af82 100644
> --- a/http-walker.c
> +++ b/http-walker.c
> @@ -544,8 +544,10 @@ static int fetch_object(struct walker *walker, unsigned 
> char *sha1)
>   } else if (hashcmp(obj_req->sha1, req->real_sha1)) {
>   ret = error("File %s has bad hash", hex);
>   } else if (req->rename < 0) {
> - ret = error("unable to write sha1 filename %s",
> - sha1_file_name(req->sha1));
> + struct strbuf buf = STRBUF_INIT;
> + sha1_file_name(, req->sha1);
> + ret = error("unable to write sha1 filename %s", buf.buf);
> + strbuf_release();
>   }
>  
>   release_http_object_request(req);
> diff --git a/http.c b/http.c
> index 5977712712..5979305bc9 100644
> --- a/http.c
> +++ b/http.c
> @@ -2168,7 +2168,7 @@ struct http_object_request 
> *new_http_object_request(const char *base_url,
>   unsigned char *sha1)
>  {
>   char *hex = sha1_to_hex(sha1);
> - const char *filename;
> + struct strbuf filename = STRBUF_INIT;
>   char prevfile[PATH_MAX];
>   int prevlocal;
>   char prev_buf[PREV_BUF_SIZE];
> @@ -2180,14 +2180,15 @@ struct http_object_request 
> *new_http_object_request(const char *base_url,
>   hashcpy(freq->sha1, sha1);
>   freq->localfile = -1;
>  
> - filename = sha1_file_name(sha1);
> + sha1_file_name(, sha1);
>   snprintf(freq->tmpfile, sizeof(freq->tmpfile),
> -  "%s.temp", filename);
> +  "%s.temp", filename.buf);
>  
> - snprintf(prevfile, sizeof(prevfile), "%s.prev", filename);
> + snprintf(prevfile, sizeof(prevfile), "%s.prev", filename.buf);
>   unlink_or_warn(prevfile);
>   rename(freq->tmpfile, prevfile);
>   unlink_or_warn(freq->tmpfile);
> + strbuf_release();
>  
>   if (freq->localfile != -1)
>   error("fd leakage in start: %d", freq->localfile);
> @@ -2302,6 +2303,7 @@ void process_http_object_request(struct 
> http_object_request *freq)
>  int finish_http_object_request(struct http_object_request *freq)
>  {
>   struct stat st;
> + struct strbuf filename = STRBUF_INIT;
>  
>   close(freq->localfile);
>   freq->localfile = -1;
> @@ -2327,8 +2329,10 @@ int finish_http_object_request(struct 
> http_object_request *freq)
>   unlink_or_warn(freq->tmpfile);
>   return -1;
>   }
> - freq->rename =
> - finalize_object_file(freq->tmpfile, sha1_file_name(freq->sha1));
> +
> + sha1_file_name(, freq->sha1);
> + freq->rename = finalize_object_file(freq->tmpfile, filename.buf);
> + strbuf_release();
>  
>   return freq->rename;
>  }
> diff --git a/sha1_file.c b/sha1_file.c
> index 3da70ac650..f66c21b2da 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -321,15 +321,11 @@ static void fill_sha1_path(struct strbuf *buf, const 
> unsigned char *sha1)
>   }
>  }
>  
> -const char *sha1_file_name(const unsigned char *sha1)
> +void sha1_file_name(struct strbuf *buf, const unsigned char *sha1)
>  {
> - static struct strbuf buf = STRBUF_INIT;
> -
> - strbuf_reset();
> - strbuf_addf(, "%s/", get_object_directory());
> + strbuf_addf(buf, "%s/", get_object_directory());
>  
> - fill_sha1_path(, sha1);
> - return buf.buf;
> + fill_sha1_path(buf, sha1);
>  }
>  
>  struct strbuf *alt_scratch_buf(struct alternate_object_database *alt)
> @@ -710,7 +706,12 @@ int check_and_freshen_file(const char *fn, int freshen)
>  
>  static int 

Re: [PATCH] Add shell completion for git remote rm

[Kevin Daudt]

On Fri, Dec 29, 2017 at 02:01:00AM +, Keith Smiley wrote:
> From: Keith Smiley 
> 
> Previously git remote rm did not complete your list of removes as remove
> does.

Your signed-off-by[1] is missing, could you please add that?

[1]:
https://github.com/git/git/blob/master/Documentation/SubmittingPatches#L278

> ---
>  contrib/completion/git-completion.bash | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/contrib/completion/git-completion.bash 
> b/contrib/completion/git-completion.bash
> index 3683c772c5586..3e9044087e6ba 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -2665,7 +2665,7 @@ _git_config ()
>  _git_remote ()
>  {
>   local subcommands="
> - add rename remove set-head set-branches
> + add rename remove rm set-head set-branches
>   get-url set-url show prune update
>   "
>   local subcommand="$(__git_find_on_cmdline "$subcommands")"
> 
> --
> https://github.com/git/git/pull/448

Bug/feature request: Can’t disable fsck warnings during clone

[Kevin A. Mitchell]

Hi!

I’ve set transfer.fsckObjects to true globally, for safety.
Unfortunately, this messed up my Spacevim install.

Doing some digging, I found that some of the repos had a warning. I
can turn the warning off, but that only affects git fsck, not git
clone. Turning off transfer.fsckObjects also fixes the problem. I’d
rather have it on for my development work.

Tested with the “next” branch as well.

$ git -c transfer.fsckObjects=true -c fsck.zeroPaddedFilemode=ignore
clone https://github.com/albfan/ag.vim
Cloning into 'ag.vim'...
remote: Counting objects: 1879, done.
error: object 65e1a0027644b6625b32d30ba5ccf1c4d483480a:
zeroPaddedFilemode: contains zero-padded file modes
fatal: Error in object
fatal: index-pack failed
$ git —version
git version 2.15.1.501.g29533fb16

but this works as expected:

$ git clone https://github.com/albfan/ag.vim
Cloning into 'ag.vim'...
remote: Counting objects: 1879, done.
remote: Total 1879 (delta 0), reused 0 (delta 0), pack-reused 1879
Receiving objects: 100% (1879/1879), 1.23 MiB | 2.76 MiB/s, done.
Resolving deltas: 100% (938/938), done.
$ cd ag.vim
$ git -c transfer.fsckObjects=true -c fsck.zeroPaddedFilemode=ignore fsck
Checking object directories: 100% (256/256), done.
Checking objects: 100% (1879/1879), done.
$ git -c transfer.fsckObjects=true fsck
Checking object directories: 100% (256/256), done.
warning in tree 65e1a0027644b6625b32d30ba5ccf1c4d483480a:
zeroPaddedFilemode: contains zero-padded file modes
Checking objects: 100% (1879/1879), done.

It would be useful to be able to turn off individual warnings during
cloning. Is there something I’m missing in the config? Or, is this
something that could be fixed?

Thanks,

Kevin

-- 
Kevin A. Mitchell
http://www.kamit.com/

Re: [PATCH] setup.c: move statement under condition

[Kevin Daudt]

On Sun, Dec 24, 2017 at 12:15:35PM +0400, Vadim Petrov wrote:
> Thank you for your replay.
> 
> > I have to be honest: this commit message (including the subject) left me
> > quite puzzled as to the intent of this patch.
> 
> I still only learn English and correctly express my thoughts while somewhat 
> difficult.
> 
> > If you also have a background story that motivated you to work on this
> > patch (for example, if you hit a huge performance bottleneck with some
> > tool that fed thousands of absolute paths to Git that needed to be turned
> > into paths relative to the worktree's top-level directory), I would
> > definitely put that into the commit message, too, if I were you.
> 
> I have no such reason. I just saw it and wanted to change it.

A commit message contains the reason why this is a good change to make.
It lets others know what problems it's trying to solve or what usecase
it tries to satisfy.

The commit message basically needs to convince others why the change is
necessary / desired, now, and in the future. 

This will help others to follow your thought process and it gives you
the possiblity to communicate trade-offs you made, all which cannot
inferred from the patch.

For simple changes, the motivation can be simple too.

To make it concrete: You are talking about a condition. What condition?
And you say that the previously obtained value will not be necessary.
What do you do with that value then? Why does this change improve the
situation? 

These are things you can state in your commit message.

Hope this helps, Kevin

> > Up until recently, we encouraged dropping the curly brackets from
> > single-line statements, but apparently that changed. It is now no longer
> > clear, and often left to the taste of the contributor. But not always.
> > Sometimes we start a beautiful thread discussion the pros and cons of
> > curly brackets in the middle of patch review, and drop altogether
> > reviewing the actual patch.
> 
> I was guided by the rule from the Documentation/CodingGuidelines:
>   When there are multiple arms to a conditional and some of them
>   require braces, enclose even a single line block in braces for
>   consistency.
> And other code from setup.c:
>   from function get_common_dir:
>   if (!has_common) {
>   /* several commands */
>   } else {
>   free(candidate->work_tree);
>   }
>   from function get_common_dir_noenv:
>   if (file_exists(path.buf)) {
>   /* several commands */
>   } else {
>   strbuf_addstr(sb, gitdir);
>   }
> 
> > In short: I think your patch does the right thing, and I hope that you
> > find my suggestions to improve the patch useful.
> 
> I fixed the patch according to your suggestions.
> 
> 
> Signed-off-by: Vadim Petrov <tridro...@yandex.ru>
> ---
>  setup.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/setup.c b/setup.c
> index 8cc34186c..1a414c256 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -27,26 +27,26 @@ static int abspath_part_inside_repo(char *path)
>  {
>   size_t len;
>   size_t wtlen;
>   char *path0;
>   int off;
>   const char *work_tree = get_git_work_tree();
>  
>   if (!work_tree)
>   return -1;
>   wtlen = strlen(work_tree);
>   len = strlen(path);
> - off = offset_1st_component(path);
>  
> - /* check if work tree is already the prefix */
> - if (wtlen <= len && !strncmp(path, work_tree, wtlen)) {
> + if (wtlen > len || strncmp(path, work_tree, wtlen))
> + off = offset_1st_component(path);
> + else { /* check if work tree is already the prefix */
>   if (path[wtlen] == '/') {
>   memmove(path, path + wtlen + 1, len - wtlen);
>   return 0;
>   } else if (path[wtlen - 1] == '/' || path[wtlen] == '\0') {
>   /* work tree is the root, or the whole path */
>   memmove(path, path + wtlen, len - wtlen + 1);
>   return 0;
>   }
>   /* work tree might match beginning of a symlink to work tree */
>   off = wtlen;
>   }
> -- 
> 2.15.1.433.g936d1b989

Re: [PATCH v2 0/5] SHA1DC fixes & fully moving to a git.git submodule

[Kevin Daudt]

On Fri, Dec 08, 2017 at 10:29:56PM +, Ævar Arnfjörð Bjarmason wrote:
> Here's v2 as promised. Comments per-patch.
> 
>   sha1dc: remove in favor of using sha1collisiondetection as a submodule
> 
> Reword & expand commit message.

Is this commit missing from the mailing list because the e-mail is too
large?

Re: [PATCH 3/4] Makefile: use the sha1collisiondetection submodule by default

[Kevin Daudt]

On Sat, Dec 09, 2017 at 01:30:14PM +0100, Kevin Daudt wrote:
> On Tue, Dec 05, 2017 at 02:02:50AM -0500, Jeff King wrote:
> > On Tue, Nov 28, 2017 at 09:32:13PM +, Ævar Arnfjörð Bjarmason wrote:
> > 
> > > Change the build process so that instead of needing to supply
> > > DC_SHA1_SUBMODULE=YesPlease to use the sha1collisiondetection
> > > submodule instead of the copy of the same code shipped in the sha1dc
> > > directory, it uses the submodule by default unless
> > > NO_DC_SHA1_SUBMODULE=UnfortunatelyYes is supplied.
> > > 
> > > This reverses the logic added by me in 86cfd61e6b ("sha1dc: optionally
> > > use sha1collisiondetection as a submodule", 2017-07-01). Git has now
> > > shipped with the submodule in git.git for two major releases, if we're
> > > ever going to migrate to fully using it instead of perpetually
> > > maintaining both sha1collisiondetection and the sha1dc directory this
> > > is a logical first step.
> > > 
> > > This change removes the "auto" logic Junio added in
> > > cac87dc01d ("sha1collisiondetection: automatically enable when
> > > submodule is populated", 2017-07-01), I feel that automatically
> > > falling back to using sha1dc would defeat the point, which is to smoke
> > > out any remaining users of git.git who have issues cloning the
> > > submodule for whatever reason.
> > 
> > I'm not sure how I feel about this. I see your point that there's no
> > real value in maintaining two systems indefinitely.  At the same time, I
> > wonder how much value the submodule strategy is actually bringing us.
> > 
> > IOW, are we agreed that the path forward is to get everybody using the
> > submodule?
> > 
> > It seems like it's going to cause some minor pain for CI and packaging
> > systems that now need to care about submodules (so at least flipping the
> > switch, but maybe also dealing with having a network dependency for the
> > build that was not already there).
> > 
> > I'll admit I'm more sensitive to this than most people, since I happen
> > to maintain a fork of Git that I run through an internal CI system. And
> > that CI otherwise depends only on the locally-held fork, not any
> > external resources. But I'm probably in a fairly unique situation there.
> > 
> > -Peff
> 
> To add to this point, package systems such as Alpinelinux and Archlinux
> (and probably others) work with released tarballs, not cloned
> repositories. For them, there is no easy way to get the submodule
> contents (the release tarballs would not contain it).
> 
> Once we would switch over to submodules only (because we do not want to
> maintain 2 separate systems), it would be a lot of hassle for those
> projects to get the sha1collisiondetection contents.
> 
> That's in my opinion a bigger disadvantage of submodules, commands like
> git archive do not support it, making it harder to get self-contained
> tarballs.
> 
> Perpahs there is a good solution to that problem, but then I'd like to
> hear it.
> 
> Kevin.

I missed the v2 Ævar sent. I see that there `make dist` is adjusted to
include sha1collisiondetection, so that would at least solve this
problem.

Re: [PATCH 3/4] Makefile: use the sha1collisiondetection submodule by default

[Kevin Daudt]

On Tue, Dec 05, 2017 at 02:02:50AM -0500, Jeff King wrote:
> On Tue, Nov 28, 2017 at 09:32:13PM +, Ævar Arnfjörð Bjarmason wrote:
> 
> > Change the build process so that instead of needing to supply
> > DC_SHA1_SUBMODULE=YesPlease to use the sha1collisiondetection
> > submodule instead of the copy of the same code shipped in the sha1dc
> > directory, it uses the submodule by default unless
> > NO_DC_SHA1_SUBMODULE=UnfortunatelyYes is supplied.
> > 
> > This reverses the logic added by me in 86cfd61e6b ("sha1dc: optionally
> > use sha1collisiondetection as a submodule", 2017-07-01). Git has now
> > shipped with the submodule in git.git for two major releases, if we're
> > ever going to migrate to fully using it instead of perpetually
> > maintaining both sha1collisiondetection and the sha1dc directory this
> > is a logical first step.
> > 
> > This change removes the "auto" logic Junio added in
> > cac87dc01d ("sha1collisiondetection: automatically enable when
> > submodule is populated", 2017-07-01), I feel that automatically
> > falling back to using sha1dc would defeat the point, which is to smoke
> > out any remaining users of git.git who have issues cloning the
> > submodule for whatever reason.
> 
> I'm not sure how I feel about this. I see your point that there's no
> real value in maintaining two systems indefinitely.  At the same time, I
> wonder how much value the submodule strategy is actually bringing us.
> 
> IOW, are we agreed that the path forward is to get everybody using the
> submodule?
> 
> It seems like it's going to cause some minor pain for CI and packaging
> systems that now need to care about submodules (so at least flipping the
> switch, but maybe also dealing with having a network dependency for the
> build that was not already there).
> 
> I'll admit I'm more sensitive to this than most people, since I happen
> to maintain a fork of Git that I run through an internal CI system. And
> that CI otherwise depends only on the locally-held fork, not any
> external resources. But I'm probably in a fairly unique situation there.
> 
> -Peff

To add to this point, package systems such as Alpinelinux and Archlinux
(and probably others) work with released tarballs, not cloned
repositories. For them, there is no easy way to get the submodule
contents (the release tarballs would not contain it).

Once we would switch over to submodules only (because we do not want to
maintain 2 separate systems), it would be a lot of hassle for those
projects to get the sha1collisiondetection contents.

That's in my opinion a bigger disadvantage of submodules, commands like
git archive do not support it, making it harder to get self-contained
tarballs.

Perpahs there is a good solution to that problem, but then I'd like to
hear it.

Kevin.

Re: [PATCH 3/3] pull: add config option for verifySignatures

[Kevin Daudt]

On Sat, Dec 09, 2017 at 09:05:30AM +, Hans Jerry Illikainen wrote:
> Verify the signature of the tip commit when `pull.verifySignatures` is
> true.  This option overrides `merge.verifySignatures` on pull, and can
> be disabled with the option `--no-verify-signatures`.

Is there a reason why git pull would need a different behaviour from git
merge? Pull itself is just a convenience command for fetch +
merge/rebase.

One precedent for having a separate configuration option for pull
however is 'pull.ff', so there might be a usecase for it.

I guess your commit message could use a motivation on why you want to
set this differently from 'merge.verifySignature'.

> 
> Signed-off-by: Hans Jerry Illikainen 
> ---
>  Documentation/config.txt  |  8 
>  builtin/pull.c| 25 +
>  t/t5520-pull.sh   | 18 ++
>  t/t5573-pull-verify-signatures.sh | 32 
>  4 files changed, 83 insertions(+)
> 
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index c1598ee70..0cd2bc597 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2596,6 +2596,14 @@ pull.ff::
>   allowed (equivalent to giving the `--ff-only` option from the
>   command line). This setting overrides `merge.ff` when pulling.
>  
> +pull.verifySignatures::
> + Verify that the tip commit of the side branch being merged is
> + signed with a valid key, i.e. a key that has a valid uid: in the
> + default trust model, this means the signing key has been signed
> + by a trusted key. If the tip commit of the side branch is not
> + signed with a valid key, the merge is aborted. This setting
> + overrides `merge.verifySignatures` when pulling.
> +
>  pull.rebase::
>   When true, rebase branches on top of the fetched branch, instead
>   of merging the default branch from the default remote when "git
> diff --git a/builtin/pull.c b/builtin/pull.c
> index 166b777ed..791365915 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -300,6 +300,28 @@ static const char *config_get_ff(void)
>  }
>  
>  /**
> + * If pull.verifySignatures is unset, returns NULL. If pull.verifySignatures 
> is
> + * "true", returns "--verify-signatures". If pull.verifySignatures is 
> "false",
> + * returns "--no-verify-signatures". Otherwise, die with an error.
> + */
> +static const char *config_get_verify_signatures(void)
> +{
> + const char *value;
> +
> + if (git_config_get_value("pull.verifysignatures", ))
> + return NULL;
> +
> + switch (git_parse_maybe_bool(value)) {
> + case 0:
> + return "--no-verify-signatures";
> + case 1:
> + return "--verify-signatures";
> + default:
> + die(_("Invalid value for pull.verifysignatures: %s"), value);
> + }
> +}
> +
> +/**
>   * Returns the default configured value for --rebase. It first looks for the
>   * value of "branch.$curr_branch.rebase", where $curr_branch is the current
>   * branch, and if HEAD is detached or the configuration key does not exist,
> @@ -849,6 +871,9 @@ int cmd_pull(int argc, const char **argv, const char 
> *prefix)
>   if (!opt_ff)
>   opt_ff = xstrdup_or_null(config_get_ff());
>  
> + if (!opt_verify_signatures)
> + opt_verify_signatures = 
> xstrdup_or_null(config_get_verify_signatures());
> +
>   if (opt_rebase < 0)
>   opt_rebase = config_get_rebase();
>  
> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
> index 59c4b778d..cdf1fd213 100755
> --- a/t/t5520-pull.sh
> +++ b/t/t5520-pull.sh
> @@ -416,6 +416,15 @@ test_expect_success "pull --rebase warns on 
> --verify-signatures" '
>   test_i18ngrep "ignoring --verify-signatures for rebase" err
>  '
>  
> +test_expect_success "pull --rebase warns on pull.verifySignatures=true" '
> + test_config pull.verifySignatures true &&
> + git reset --hard before-rebase &&
> + git pull --rebase . copy 2>err &&
> + test "$(git rev-parse HEAD^)" = "$(git rev-parse copy)" &&
> + test new = "$(git show HEAD:file2)" &&
> + test_i18ngrep "ignoring --verify-signatures for rebase" err
> +'
> +
>  test_expect_success "pull --rebase does not warn on --no-verify-signatures" '
>   git reset --hard before-rebase &&
>   git pull --rebase --no-verify-signatures . copy 2>err &&
> @@ -424,6 +433,15 @@ test_expect_success "pull --rebase does not warn on 
> --no-verify-signatures" '
>   test_i18ngrep ! "verify-signatures" err
>  '
>  
> +test_expect_success "pull --rebase does not warn on 
> pull.verifySignatures=false" '
> + test_config pull.verifySignatures false &&
> + git reset --hard before-rebase &&
> + git pull --rebase . copy 2>err &&
> + test "$(git rev-parse HEAD^)" = "$(git rev-parse copy)" &&
> + test new = "$(git show HEAD:file2)" &&
> + test_i18ngrep ! "verify-signatures" err
> +'
> +
>  # 

Re: [PATCH 2/3] t: add tests for pull --verify-signatures

[Kevin Daudt]

On Sat, Dec 09, 2017 at 09:05:29AM +, Hans Jerry Illikainen wrote:
> Add tests for `pull --verify-signatures` with untrusted, bad and no
> signatures.  Previously the only test for `--verify-signatures` was to
> make sure that `pull --rebase --verify-signatures` result in a warning
> (t5520-pull.sh).

Nice!

Same thing regarding the `git checkout initial` commands counts
here too.

> 
> Signed-off-by: Hans Jerry Illikainen 
> ---
>  t/t5573-pull-verify-signatures.sh | 78 
> +++
>  1 file changed, 78 insertions(+)
>  create mode 100755 t/t5573-pull-verify-signatures.sh
> 
> diff --git a/t/t5573-pull-verify-signatures.sh 
> b/t/t5573-pull-verify-signatures.sh
> new file mode 100755
> index 0..700247910
> --- /dev/null
> +++ b/t/t5573-pull-verify-signatures.sh
> @@ -0,0 +1,78 @@
> +#!/bin/sh
> +
> +test_description='pull signature verification tests'
> +. ./test-lib.sh
> +. "$TEST_DIRECTORY/lib-gpg.sh"
> +
> +test_expect_success GPG 'create repositories with signed commits' '
> + echo 1 >a && git add a &&
> + test_tick && git commit -m initial &&
> + git tag initial &&
> +
> + git clone . signed &&
> + (
> + cd signed &&
> + echo 2 >b && git add b &&
> + test_tick && git commit -S -m "signed"
> + ) &&
> +
> + git clone . unsigned &&
> + (
> + cd unsigned &&
> + echo 3 >c && git add c &&
> + test_tick && git commit -m "unsigned"
> + ) &&
> +
> + git clone . bad &&
> + (
> + cd bad &&
> + echo 4 >d && git add d &&
> + test_tick && git commit -S -m "bad" &&
> + git cat-file commit HEAD >raw &&
> + sed -e "s/bad/forged bad/" raw >forged &&
> + git hash-object -w -t commit forged >forged.commit &&
> + git checkout $(cat forged.commit)
> + ) &&
> +
> + git clone . untrusted &&
> + (
> + cd untrusted &&
> + echo 5 >e && git add e &&
> + test_tick && git commit -SB7227189 -m "untrusted"
> + )
> +'
> +
> +test_expect_success GPG 'pull unsigned commit with --verify-signatures' '
> + test_must_fail git pull --ff-only --verify-signatures unsigned 
> 2>pullerror &&
> + test_i18ngrep "does not have a GPG signature" pullerror
> +'
> +
> +test_expect_success GPG 'pull commit with bad signature with 
> --verify-signatures' '
> + test_must_fail git pull --ff-only --verify-signatures bad 2>pullerror &&
> + test_i18ngrep "has a bad GPG signature" pullerror
> +'
> +
> +test_expect_success GPG 'pull commit with untrusted signature with 
> --verify-signatures' '
> + test_must_fail git pull --ff-only --verify-signatures untrusted 
> 2>pullerror &&
> + test_i18ngrep "has an untrusted GPG signature" pullerror
> +'
> +
> +test_expect_success GPG 'pull signed commit with --verify-signatures' '
> + git pull --verify-signatures signed >pulloutput &&
> + test_i18ngrep "has a good GPG signature" pulloutput &&
> + git checkout initial
> +'
> +
> +test_expect_success GPG 'pull commit with bad signature without 
> verification' '
> + git pull --ff-only bad 2>pullerror &&
> + git checkout initial
> +'
> +
> +test_expect_success GPG 'pull commit with bad signature with 
> --no-verify-signatures' '
> + test_config merge.verifySignatures true &&
> + test_config pull.verifySignatures true &&
> + git pull --ff-only --no-verify-signatures bad 2>pullerror &&
> + git checkout initial
> +'
> +
> +test_done
> -- 
> 2.11.0
> 

Re: [PATCH 1/3] merge: add config option for verifySignatures

[Kevin Daudt]

ergeoutput
> + test_i18ngrep "has a good GPG signature" mergeoutput &&
> + git checkout initial

This looks like a clean up step. If so, it's better to add
`test_when_finished 'git checkout initial'` at the beginning to clearly
mark it as a clean up step and make sure it's run even when the test
fails. Same counts for the other occurances.

> +'
> +
> +test_expect_success GPG 'merge signed commit with 
> merge.verifySignatures=true' '
> + test_config merge.verifySignatures true &&
> + git merge --verbose --ff-only side-signed >mergeoutput &&
> + test_i18ngrep "has a good GPG signature" mergeoutput &&
> + git checkout initial
>  '
>  
>  test_expect_success GPG 'merge commit with bad signature without 
> verification' '
> - git merge $(cat forged.commit)
> + git merge $(cat forged.commit) &&
> + git checkout initial
> +'
> +
> +test_expect_success GPG 'merge commit with bad signature with 
> merge.verifySignatures=false' '
> + test_config merge.verifySignatures false &&
> + git merge $(cat forged.commit) &&
> + git checkout initial
> +'
> +
> +test_expect_success GPG 'merge commit with bad signature with 
> merge.verifySignatures=true and --no-verify-signatures' '
> + test_config merge.verifySignatures true &&
> + git merge --no-verify-signatures $(cat forged.commit) &&
> + git checkout initial
>  '
>  
>  test_done

Kevin.

Re: [PATCH v2] gitcli: tweak "man gitcli" for clarity

[Kevin Daudt]

On Thu, Nov 23, 2017 at 08:51:55AM -0500, Jeff King wrote:
> On Thu, Nov 23, 2017 at 02:45:44AM -0500, Robert P. J. Day wrote:
> 
> > > It's pretty clear to me as it is, but maybe we can write it differently.
> > > Like:
> > >
> > >   Without a disambiguating `--`, Git makes a reasonable guess. If it
> > >   cannot guess (because your request is ambiguous), then it will error
> > >   out.
> > 
> >   ok, i'll give this another try, given that there are two independent
> > points to be made here:
> > 
> > 1) even without the "--", git can generally parse the command and do
> > the right thing (or do a *valid* thing, given its heuristics)
> > 
> > 2) occasionally, without the "--", the command is really and truly
> > ambiguous, at which point git will fail and tell you to disambiguate
> > 
> >   not the wording i will use, but can we agree that those are the two
> > points to be made here?
> 
> Yep, I think so.
> 
> -Peff

Just for completeness, as it is somewhat covered by point 1 already, but
there are cases where there is no real ambiguity but you are required to
add '--' to tell git that it should not look for the file in the working
tree:

  $ git show abc123 deleted_file.txt
  fatal: ambiguous argument 'deleted_file.txt':
  unknown revision or path not in the working tree.
  Use '--' to separate paths from revisions, like this:
  'git  [...] -- [...]'

There might be good reasons why this is, but I don't consider this to be
actually ambiguous: there is no branch called 'deleted_file.txt' and git
could know that the files exists in the mentioned commit, so it should
be pretty clear what is meant.

Might be worth documenting this.

Kevin

Re: [PATCH v2] gitcli: tweak "man gitcli" for clarity

[Kevin Daudt]

On Wed, Nov 22, 2017 at 06:19:23AM -0500, Robert P. J. Day wrote:
> On Wed, 22 Nov 2017, Junio C Hamano wrote:
> 
> > "Robert P. J. Day"  writes:
> >
> > > git repo with a file called "Gemfile", so i created a branch called
> > > "Gemfile", and when i ran:
> > >
> > >   $ git checkout Gemfile
> > >
> > > git switched to the branch. so even with the ambiguity, git
> > > obviously has some sort of precedence order it checks. so what are
> > > the rules here?
> >
> > 31b83f36 ("Merge branch 'nd/checkout-disambiguation'", 2016-09-26)
> > should have made it clear that the "checkout" command has a
> > convenience special case.
> 
>   ok, then i'm still curious about git examples that actually fail due
> to an inability to disambiguate.
> 
> rday

Here is an example with git diff

$ git init git-disambiguate
$ cd git-disambiguate
$ echo 1 >foo && git add foo && git commit -m foo
$ git branch foo
$ echo 2 >>foo && git add foo && git commit -m foo2
$ echo 3 >>foo

$ git diff foo
fatal: ambiguous argument 'foo': both revision and filename
Use '--' to separate paths from revisions, like this:
'git  [...] -- [...]'

$ git diff HEAD foo
fatal: ambiguous argument 'foo': both revision and filename
Use '--' to separate paths from revisions, like this:
'git  [...] -- [...]'

$ git diff HEAD -- foo
diff --git a/foo b/foo
index 1191247..01e79c3 100644
--- a/foo
+++ b/foo
@@ -1,2 +1,3 @@
 1
 2
+3

$ git diff HEAD foo --
diff --git a/foo b/foo
index 1191247..d00491f 100644
--- a/foo
+++ b/foo
@@ -1,2 +1 @@
 1
-2
 

Re: [PATCH v2] gitcli: tweak "man gitcli" for clarity

[Kevin Daudt]

On Tue, Nov 21, 2017 at 04:47:42PM -0500, Robert P. J. Day wrote:
> On Tue, 21 Nov 2017, Kevin Daudt wrote:
> 
> > On Tue, Nov 21, 2017 at 04:27:59PM -0500, Robert P. J. Day wrote:
> > > No major changes, just some rewording and showing some variations of
> > > general Git commands.
> > >
> > > Signed-off-by: Robert P. J. Day <rpj...@crashcourse.ca>
> > >
> > > ---
> > >
> > > diff --git a/Documentation/gitcli.txt b/Documentation/gitcli.txt
> > > index 9f13266a6..d690d1ff0 100644
> > > --- a/Documentation/gitcli.txt
> > > +++ b/Documentation/gitcli.txt
> > > @@ -13,7 +13,7 @@ gitcli
> > >  DESCRIPTION
> > >  ---
> > >
> > > -This manual describes the convention used throughout Git CLI.
> > > +This manual describes the conventions used throughout Git CLI.
> > >
> > >  Many commands take revisions (most often "commits", but sometimes
> > >  "tree-ish", depending on the context and command) and paths as their
> > > @@ -32,32 +32,35 @@ arguments.  Here are the rules:
> > > between the HEAD commit and the work tree as a whole".  You can say
> > > `git diff HEAD --` to ask for the latter.
> > >
> > > - * Without disambiguating `--`, Git makes a reasonable guess, but errors
> > > -   out and asking you to disambiguate when ambiguous.  E.g. if you have a
> > > + * Without a disambiguating `--`, Git makes a reasonable guess, but can
> > > +   error out, asking you to disambiguate when ambiguous.  E.g. if you 
> > > have a
> >
> > 'Can' error out implies that it sometimes would not error out when
> > there is ambiguity. Are there situation where git does not error out
> > in that case?
> 
>   i would say (based on my limited knowledge) that if the heuristic
> kicks in and works fine, then things will work. i think it's fair to
> say that git "can" error out if the heuristic fails.
> 
> rday

In most cases that I'm aware of, you have to be explicit. If for example
you want to refer to a file that's not in the working tree, you have to
use '--'.  Even with heuristics, it would still have to error out when
it's ambiguous what the user meant.

So the way you worded it implies that there are situations where git
knows there are multiple things the user could have meant, but it would
not error out in that case.

Kevin

Re: [PATCH v2] gitcli: tweak "man gitcli" for clarity

[Kevin Daudt]

On Tue, Nov 21, 2017 at 04:27:59PM -0500, Robert P. J. Day wrote:
> No major changes, just some rewording and showing some variations of
> general Git commands.
> 
> Signed-off-by: Robert P. J. Day 
> 
> ---
> 
> diff --git a/Documentation/gitcli.txt b/Documentation/gitcli.txt
> index 9f13266a6..d690d1ff0 100644
> --- a/Documentation/gitcli.txt
> +++ b/Documentation/gitcli.txt
> @@ -13,7 +13,7 @@ gitcli
>  DESCRIPTION
>  ---
> 
> -This manual describes the convention used throughout Git CLI.
> +This manual describes the conventions used throughout Git CLI.
> 
>  Many commands take revisions (most often "commits", but sometimes
>  "tree-ish", depending on the context and command) and paths as their
> @@ -32,32 +32,35 @@ arguments.  Here are the rules:
> between the HEAD commit and the work tree as a whole".  You can say
> `git diff HEAD --` to ask for the latter.
> 
> - * Without disambiguating `--`, Git makes a reasonable guess, but errors
> -   out and asking you to disambiguate when ambiguous.  E.g. if you have a
> + * Without a disambiguating `--`, Git makes a reasonable guess, but can
> +   error out, asking you to disambiguate when ambiguous.  E.g. if you have a

'Can' error out implies that it sometimes would not error out when there
is ambiguity. Are there situation where git does not error out in that
case?

> file called HEAD in your work tree, `git diff HEAD` is ambiguous, and
> you have to say either `git diff HEAD --` or `git diff -- HEAD` to
> disambiguate.
>  +
>  When writing a script that is expected to handle random user-input, it is
>  a good practice to make it explicit which arguments are which by placing
> -disambiguating `--` at appropriate places.
> +a disambiguating `--` at appropriate places.
> 
>   * Many commands allow wildcards in paths, but you need to protect
> -   them from getting globbed by the shell.  These two mean different
> -   things:
> +   them from getting globbed by the shell.  The following commands have
> +   two different meanings:
>  +
>  
>  $ git checkout -- *.c
> +
>  $ git checkout -- \*.c
> +$ git checkout -- "*.c"
> +$ git checkout -- '*.c'
>  
>  +
> -The former lets your shell expand the fileglob, and you are asking
> -the dot-C files in your working tree to be overwritten with the version
> -in the index.  The latter passes the `*.c` to Git, and you are asking
> -the paths in the index that match the pattern to be checked out to your
> -working tree.  After running `git add hello.c; rm hello.c`, you will _not_
> -see `hello.c` in your working tree with the former, but with the latter
> -you will.
> +The first command lets your shell expand the fileglob, and you are asking
> +the dot-C files in your working tree to be overwritten with the version in
> +the index.  The latter three variations pass the `*.c` to Git, and you are
> +asking the paths in the index that match the pattern to be checked out to
> +your working tree.  After running `git add hello.c; rm hello.c`, you will
> +_not_ see `hello.c` in your working tree with the first command, but with
> +the latter three variations, you will.
> 
>   * Just as the filesystem '.' (period) refers to the current directory,
> using a '.' as a repository name in Git (a dot-repository) is a relative
> 
> -- 
> 
> 
> Robert P. J. Day Ottawa, Ontario, CANADA
> http://crashcourse.ca
> 
> Twitter:   http://twitter.com/rpjday
> LinkedIn:   http://ca.linkedin.com/in/rpjday
> 

Re: [PATCH] doc: remove explanation of "--" from man pages

[Kevin Daudt]

On Tue, Nov 21, 2017 at 04:12:02PM -0500, Robert P. J. Day wrote:
> "man gitcli" already explains the purpose of the "--" syntax, so there
> is no value to a small subset of Git commands explaining that in their
> man pages.
> 
> Signed-off-by: Robert P. J. Day <rpj...@crashcourse.ca>
> 
> ---
> 
>   i tried this once before, and i'm going to try to push it through
> again ... it's pointless and inconsistent for less than a dozen man
> pages to explicitly explain the purpose of "--" unless all of the man
> pages do. as long as the "--" appears in the command SYNOPSIS, that
> should be more than adequate.

Although I agree that common options don't need to be explained
everytime again, this change might make '--' even more obscure. To be
honest, I didn't even know about gitcli(7), let alone most new users.

In the #git irc channel we often have to explain what '--' means and
why it's sometimes necessary.

I don't however know a better solution to it more clear.

Kevin

Re: Git on Mac - Segmentation fault:11

[Kevin]

cc: mailinglist

On Thu, Nov 16, 2017 at 9:40 PM, Frank Burkitt <fburk...@burkitt.com> wrote:
> Kevin -
>
> Thank you for getting back to me.
>
> The version of Git is 2.15.0
> I used Brew to install it
> I am not getting any segfaults from other apps
> When I do a ‘git init’ I get a Segmentation fault: 11 whether I do it in or
> outside of a virtualenv
>
> Is there any additional info that would help?
>
> Frank
>
>
> Frank Burkitt
> Mobile: 307-699-1321
> Email: fburk...@burkitt.com
>
>
> "If you can't explain it simply, you don't understand it well enough" -
> Albert Einstein
>
>
>
>
>
> On Nov 16, 2017, at 1:34 PM, Kevin Daudt <m...@ikke.info> wrote:
>
> On Wed, Nov 15, 2017 at 05:30:23PM -0700, Frank Burkitt wrote:
>
> I am using Git on a Macbook pro with MacOS High Sierra version 10.13.1
> (17B48).  I have been using it in a virtualenv with python 3.  I have
> begun to get "Segmentation fault: 11" with every git command.  I have
> been searching for a reason why this is occurring but have not been
> able to find a solution.  I have reinstalled the operating system,
> uninstalled and reinstalled Git, and a variety of other attempts at
> finding a solution.  Is this a know issue? Any suggestions would be
> appreciated.
>
>
> Hello Frank,
>
> Could you provide a bit more information?
>
> - What version of git are you using?
> - how did you install git,
> - Do you also get segfaults outside of the virtualenv?
>
> This sounds perhaps like it's a copattibility issue with a library.
>
> Kind regards, Kevin.
>
>

Re: Git on Mac - Segmentation fault:11

[Kevin Daudt]

On Wed, Nov 15, 2017 at 05:30:23PM -0700, Frank Burkitt wrote:
> I am using Git on a Macbook pro with MacOS High Sierra version 10.13.1
> (17B48).  I have been using it in a virtualenv with python 3.  I have
> begun to get "Segmentation fault: 11" with every git command.  I have
> been searching for a reason why this is occurring but have not been
> able to find a solution.  I have reinstalled the operating system,
> uninstalled and reinstalled Git, and a variety of other attempts at
> finding a solution.  Is this a know issue? Any suggestions would be
> appreciated.

Hello Frank,

Could you provide a bit more information?

- What version of git are you using?
- how did you install git,
- Do you also get segfaults outside of the virtualenv?

This sounds perhaps like it's a copattibility issue with a library.

Kind regards, Kevin.

Re: [RFC PATCH v3 4/4] builtin/branch: give more useful error messages when renaming

[Kevin Daudt]

On Mon, Nov 13, 2017 at 08:01:12AM +0530, Kaartic Sivaraam wrote:
> On Sunday 12 November 2017 11:53 PM, Kevin Daudt wrote:
> > On Thu, Nov 02, 2017 at 12:24:07PM +0530, Kaartic Sivaraam wrote:
> > > From: Kaartic Sivaraam <kaarticsivaraam91...@gmail.com>
> > > 
> > > When trying to rename an inexistent branch to with a name of a branch
> > 
> > This sentence does not read well. Probably s/with a/the/ helps.
> > 
> 
> Thanks. Seems I missed it somehow. Will fix it.
> 
> > > that already exists the rename failed specifying the new branch name
> > > exists rather than specifying that the branch trying to be renamed
> > > doesn't exist.
> > > 
> > > [..]
> > > 
> > > Note: Thanks to the strbuf API that made it possible to easily construct
> > > the composite error message strings!
> > 
> > I'm not sure this note adds a lot, since the strbuf API is not that new.
> > 
> 
> That was a little attribution I wanted make to the strbuf API as this was
> the first time I leveraged it to this extent and I was surprised by the way
> it made string manipulation easier in C. Just documented my excitation. In
> case it seems to be noise (?) which should removed, let me know.

I guess that would fit better below the the ---

Re: [RFC PATCH v3 4/4] builtin/branch: give more useful error messages when renaming

[Kevin Daudt]

On Thu, Nov 02, 2017 at 12:24:07PM +0530, Kaartic Sivaraam wrote:
> From: Kaartic Sivaraam <kaarticsivaraam91...@gmail.com>
> 
> When trying to rename an inexistent branch to with a name of a branch

This sentence does not read well. Probably s/with a/the/ helps.

> that already exists the rename failed specifying the new branch name
> exists rather than specifying that the branch trying to be renamed
> doesn't exist.
> 
> [..]
> 
> Note: Thanks to the strbuf API that made it possible to easily construct
> the composite error message strings!

I'm not sure this note adds a lot, since the strbuf API is not that new.

Kevin

Re: [PATCH] config: added --expiry-date type support

[Kevin Daudt]

+ cat >.git/config <<-\EOF &&
> + [date]
> + valid1 = "Fri Jun 4 15:46:55 2010"
> + valid2 = "2017/11/11 11:11:11PM"
> + valid3 = "2017/11/10 09:08:07 PM"
> + valid4 = "never"
> + invalid1 = "abc"
> + EOF
> + cat >expect <<-\EOF &&
> + 1275666415
> + 1510441871
> + 1510348087
> + 0
> + EOF
> + {
> + git config --expiry-date date.valid1 &&
> + git config --expiry-date date.valid2 &&
> + git config --expiry-date date.valid3 &&
> + git config --expiry-date date.valid4
> + } >actual &&
> + test_cmp expect actual &&
> + test_must_fail git config --expiry-date date.invalid1
> +'
> +
>  cat > expect << EOF
>  [quote]
>   leading = " test"
> 
> --
> https://github.com/git/git/pull/433

Welcome and thanks for your submission.

There are a couple of issues, which you can read about in the
SubmittingPatches[0] documentation.

The first and most foremost is that your signed-off-by is missing,
which is a requirement to show that you have the right to submit this
code.

The commit subject should be in the present tense, as a command to the
code base, like so:

config: add --expiry-date type support

What I'm also missing is a motivation on why you added this option,
which should be part of your commit message. As far as I know, there is
currently no config setting that expects a date format.

Kevin

[0]:https://github.com/git/git/blob/master/Documentation/SubmittingPatches

Re: [PATCH] bisect: clarify that one can select multiple good commits

[Kevin Daudt]

On Sat, Nov 11, 2017 at 08:26:00AM -0500, Robert P. J. Day wrote:
> 
> Current man page for "bisect" is inconsistent explaining the fact that
> "git bisect" takes precisely one bad commit, but one or more good
> commits, so tweak the man page in a few places to make that clear.
> 
> rday
> 
> Signed-off-by: Robert P. J. Day 
> 
> ---
> 
>   i also exercised literary license to reword an example to look for a
> commit where performance was *degraded* rather than improved, since i
> think that's the sort of thing that people would be more interested
> in.
> 
>  In fact, `git bisect` can be used to find the commit that changed
>  *any* property of your project; e.g., the commit that fixed a bug, or
> -the commit that caused a benchmark's performance to improve. To
> +the commit that caused a benchmark's performance to degrade. To
>  support this more general usage, the terms "old" and "new" can be used
>  in place of "good" and "bad", or you can choose your own terms. See
>  section "Alternate terms" below for more information.
> @@ -58,7 +58,7 @@ $ git bisect bad # Current version is bad
>  $ git bisect good v2.6.13-rc2# v2.6.13-rc2 is known to be good
>  
> 

I think this example was meant to suggest that it's not only possible to
find bad things (bugs, performance degradations), but also the opposite
(when was a bug fixed, what caused the performance to change). 

So I think it's good to keep the example like it is.

RE: cherry-pick very slow on big repository

[Kevin Willford]

Since this is happening during a merge, you might need to use merge.renameLimit
or the merge strategy option of -Xno-renames.  Although the code does fallback
to use the diff.renameLimit but there is still a lot that is done before even 
checking
the rename limit so I would first try getting renames turned off.

Thanks,
Kevin

> -Original Message-
> From: git-ow...@vger.kernel.org [mailto:git-ow...@vger.kernel.org] On Behalf
> Of Peter Krefting
> Sent: Friday, November 10, 2017 7:05 AM
> To: Derrick Stolee <sto...@gmail.com>
> Cc: Jeff King <p...@peff.net>; Git Mailing List <git@vger.kernel.org>
> Subject: Re: cherry-pick very slow on big repository
> 
> Derrick Stolee:
> 
> > Git is spending time detecting renames, which implies you probably
> > renamed a folder or added and deleted a large number of files. This
> > rename detection is quadratic (# adds times # deletes).
> 
> Yes, a couple of directories with a lot of template files have been
> renamed (and some removed, some added) between the current development
> branch and this old maintenance branch. I get the "Performing inexact
> rename detection" a lot when merging changes in the other direction.
> 
> However, none of them applies to these particular commits, which only
> touches files that are in the exact same location on both branches.
> 
> > You can remove this rename detection by running your cherry-pick
> > with `git -c diff.renameLimit=1 cherry-pick ...`
> 
> That didn't work, actually it failed to finish with this setting in
> effect, it hangs in such a way that I can't stop it with Ctrl+C
> (neither when running from the command line, nor when running inside
> gdb). It didn't finish in the 20 minutes I gave it.
> 
> I also tried with diff.renames=false, which also seemed to fail.
> 
> --
> \\// Peter -
> https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.softw
> olves.pp.se%2F=02%7C01%7Ckewillf%40microsoft.com%7C6b831a75739e4
> 0428d3808d52844106c%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636
> 459195209466999=kJtNLAs1LSoPy%2B%2BNADJkuEBPMZVcxkSkKzOEEeIG
> VpM%3D=0

Re: [PATCH 1/3] notes: move hex_to_bytes() to hex.c and export it

[Kevin Daudt]

On Sun, Nov 05, 2017 at 05:47:47PM +0100, René Scharfe wrote:
> Am 05.11.2017 um 03:56 schrieb Kevin Daudt:
> > On Tue, Oct 31, 2017 at 02:46:49PM +0100, René Scharfe wrote:
> >> Make the function for converting pairs of hexadecimal digits to binary
> >> available to other call sites.
> >>
> >> Signed-off-by: Rene Scharfe <l@web.de>
> >> ---
> >>   cache.h |  7 +++
> >>   hex.c   | 12 
> >>   notes.c | 17 -
> >>   3 files changed, 19 insertions(+), 17 deletions(-)
> >>
> >> diff --git a/cache.h b/cache.h
> >> index 6440e2bf21..f06bfbaf32 100644
> >> --- a/cache.h
> >> +++ b/cache.h
> >> @@ -1317,6 +1317,13 @@ extern int set_disambiguate_hint_config(const char 
> >> *var, const char *value);
> >>   extern int get_sha1_hex(const char *hex, unsigned char *sha1);
> >>   extern int get_oid_hex(const char *hex, struct object_id *sha1);
> >>   
> >> +/*
> >> + * Read `len` pairs of hexadecimal digits from `hex` and write the
> >> + * values to `binary` as `len` bytes. Return 0 on success, or -1 if
> > 
> > Is it correct to call the result binary? I would say that it's the value
> > that gets stored. To me, this value does not really have a base.
> 
> Here's the full context:
> 
>   /*
>* Read `len` pairs of hexadecimal digits from `hex` and write the
>* values to `binary` as `len` bytes. Return 0 on success, or -1 if
>* the input does not consist of hex digits).
>*/
>   extern int hex_to_bytes(unsigned char *binary, const char *hex, size_t len);
> 
> The patch moves the comment verbatim.  Words in backticks (`binary`,
> `hex`, `len`) are parameter names.
> 
> The function converts pairs of hexadecimal digits (base 16, ASCII
> encoded) to bytes (base 256).  A byte can be seen as an array of bits;
> thus the output is also binary (base 2) without requiring further
> conversion.
> 
> Calling the variable "binary" may seem unspecific, but makes sense in
> the context of this function.
> 
> Does any of that help?
> 
> Thanks,
> René

Thanks, I have been thinking about it more, and I agree, it does make
sense. 

I had a binary representation in mind, but this is refering to binary
data (just like you can have binary files).

Kevin

Re: [PATCH 1/3] notes: move hex_to_bytes() to hex.c and export it

[Kevin Daudt]

On Tue, Oct 31, 2017 at 02:46:49PM +0100, René Scharfe wrote:
> Make the function for converting pairs of hexadecimal digits to binary
> available to other call sites.
> 
> Signed-off-by: Rene Scharfe <l@web.de>
> ---
>  cache.h |  7 +++
>  hex.c   | 12 
>  notes.c | 17 -
>  3 files changed, 19 insertions(+), 17 deletions(-)
> 
> diff --git a/cache.h b/cache.h
> index 6440e2bf21..f06bfbaf32 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1317,6 +1317,13 @@ extern int set_disambiguate_hint_config(const char 
> *var, const char *value);
>  extern int get_sha1_hex(const char *hex, unsigned char *sha1);
>  extern int get_oid_hex(const char *hex, struct object_id *sha1);
>  
> +/*
> + * Read `len` pairs of hexadecimal digits from `hex` and write the
> + * values to `binary` as `len` bytes. Return 0 on success, or -1 if

Is it correct to call the result binary? I would say that it's the value
that gets stored. To me, this value does not really have a base.

Kevin

Re: [PATCH v1 1/2] refs: extract function to normalize partial refs

[Kevin Daudt]

On Sat, Nov 04, 2017 at 11:27:39AM +0900, Junio C Hamano wrote:
> I however notice that addition of /* to the tail is trying to be
> careful by using strbuf_complete('/'), but prefixing with "refs/"
> does not and we would end up with a double-slash if pattern begins
> with a slash.  The contract between the caller of this function (or
> its original, which is for_each_glob_ref_in()) and the callee is
> that prefix must not begin with '/', so it may be OK, but we might
> want to add "if (*pattern == '/') BUG(...)" at the beginning.
>
> I dunno.  In any case, that is totally outside the scope of this two
> patch series.

I do think it's a good idea to make future readers of the code aware of
this contract, and adding a BUG assert does that quite well. Here is a
patch that implements it.

This applies of course on top of this patch series.

-- >8 --
Subject: [PATCH] normalize_glob_ref: assert implicit contract of prefix

normalize_glob_ref has an implicit contract of expecting 'prefix' to not
start with a '/', otherwise the pattern would end up with a
double-slash.

Mark it as a BUG when the prefix argument of normalize_glob_ref starts
with a '/' so that future callers will be aware of this contract.

Signed-off-by: Kevin Daudt <m...@ikke.info>
---
 refs.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/refs.c b/refs.c
index e9ae659ae..6747981d1 100644
--- a/refs.c
+++ b/refs.c
@@ -372,6 +372,8 @@ int head_ref_namespaced(each_ref_fn fn, void *cb_data)
 void normalize_glob_ref(struct strbuf *normalized_pattern, const char *prefix,
const char *pattern, int flags)
 {
+   if (prefix && *prefix == '/') BUG("prefix cannot not start with '/'");
+
if (!prefix && !starts_with(pattern, "refs/"))
strbuf_addstr(normalized_pattern, "refs/");
else if (prefix)
-- 
2.15.0.rc2.57.g2f899857a9

Re: [PATCH 1/2] sequencer: factor out rewrite_file()

[Kevin Daudt]

On Tue, Oct 31, 2017 at 05:33:57PM +0100, Kevin Daudt wrote:
> On Tue, Oct 31, 2017 at 10:54:21AM +0100, René Scharfe wrote:
> > Reduce code duplication by extracting a function for rewriting an
> > existing file.
> > 
> > Signed-off-by: Rene Scharfe <l@web.de>
> > ---
> >  sequencer.c | 46 +-
> >  1 file changed, 17 insertions(+), 29 deletions(-)
> > 
> > diff --git a/sequencer.c b/sequencer.c > > index f2a10cc4f2..17360eb38a 
> > 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -2665,6 +2665,20 @@ int check_todo_list(void)
> > return res;
> >  }
> >  
> > +static int rewrite_file(const char *path, const char *buf, size_t len)
> > +{
> > +   int rc = 0;
> > +   int fd = open(path, O_WRONLY);
> > +   if (fd < 0)
> > +   return error_errno(_("could not open '%s' for writing"), path);
> > +   if (write_in_full(fd, buf, len) < 0)
> > +   rc = error_errno(_("could not write to '%s'"), path);
> > +   if (!rc && ftruncate(fd, len) < 0)
> > +   rc = error_errno(_("could not truncate '%s'"), path);
> > +   close(fd);
> > +   return rc;
> > +}
> > +
> >  /* skip picking commits whose parents are unchanged */
> >  int skip_unnecessary_picks(void)
> >  {
> > @@ -2737,29 +2751,11 @@ int skip_unnecessary_picks(void)
> > }
> > close(fd);
> >  
> > -   fd = open(rebase_path_todo(), O_WRONLY, 0666);
> > -   if (fd < 0) {
> > -   error_errno(_("could not open '%s' for writing"),
> > -   rebase_path_todo());
> > +   if (rewrite_file(rebase_path_todo(), todo_list.buf.buf + offset,
> > +todo_list.buf.len - offset) < 0) {
> > todo_list_release(_list);
> > return -1;
> > }
> > -   if (write_in_full(fd, todo_list.buf.buf + offset,
> > -   todo_list.buf.len - offset) < 0) {
> > -   error_errno(_("could not write to '%s'"),
> > -   rebase_path_todo());
> > -   close(fd);
> > -   todo_list_release(_list);
> 
> Is this missing on purpose in the new situation?
>

I wasn't looking at the context, only the changed lines. After reading
it again, it's clear that nothing is missing (the freeing of todo_list).

Kevin

Re: Is it possible to convert a Json file to xml file with Git

[Kevin Daudt]

On Tue, Oct 31, 2017 at 05:28:40PM +, Eyjolfur Eyjolfsson wrote:
> Hi
> 
> I have a question.
> Is it possible to convert a Json file to XML with Git
> 
> Best regards
> 
> Eyjolfur Eyjolfsson
> 
> (e) eyjolfureyjolfs...@tprg.com
> (w) tpretailgroup.com
> 

Hello Eyjolfur,

git is a version control system, which is mostly content agnostic. It
knows nothing about json or xml, let alone how to convert them.

You might want to use some kind of programming language to do the
conversion.

Could you perhaps explain more why you are asking this question?

Kevin.

Re: [PATCH 1/2] sequencer: factor out rewrite_file()

[Kevin Daudt]

On Tue, Oct 31, 2017 at 10:54:21AM +0100, René Scharfe wrote:
> Reduce code duplication by extracting a function for rewriting an
> existing file.
> 
> Signed-off-by: Rene Scharfe 
> ---
>  sequencer.c | 46 +-
>  1 file changed, 17 insertions(+), 29 deletions(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index f2a10cc4f2..17360eb38a 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2665,6 +2665,20 @@ int check_todo_list(void)
>   return res;
>  }
>  
> +static int rewrite_file(const char *path, const char *buf, size_t len)
> +{
> + int rc = 0;
> + int fd = open(path, O_WRONLY);
> + if (fd < 0)
> + return error_errno(_("could not open '%s' for writing"), path);
> + if (write_in_full(fd, buf, len) < 0)
> + rc = error_errno(_("could not write to '%s'"), path);
> + if (!rc && ftruncate(fd, len) < 0)
> + rc = error_errno(_("could not truncate '%s'"), path);
> + close(fd);
> + return rc;
> +}
> +
>  /* skip picking commits whose parents are unchanged */
>  int skip_unnecessary_picks(void)
>  {
> @@ -2737,29 +2751,11 @@ int skip_unnecessary_picks(void)
>   }
>   close(fd);
>  
> - fd = open(rebase_path_todo(), O_WRONLY, 0666);
> - if (fd < 0) {
> - error_errno(_("could not open '%s' for writing"),
> - rebase_path_todo());
> + if (rewrite_file(rebase_path_todo(), todo_list.buf.buf + offset,
> +  todo_list.buf.len - offset) < 0) {
>   todo_list_release(_list);
>   return -1;
>   }
> - if (write_in_full(fd, todo_list.buf.buf + offset,
> - todo_list.buf.len - offset) < 0) {
> - error_errno(_("could not write to '%s'"),
> - rebase_path_todo());
> - close(fd);
> - todo_list_release(_list);

Is this missing on purpose in the new situation?

> - return -1;
> - }
> - if (ftruncate(fd, todo_list.buf.len - offset) < 0) {
> - error_errno(_("could not truncate '%s'"),
> - rebase_path_todo());
> - todo_list_release(_list);
> - close(fd);
> - return -1;
> - }
> - close(fd);
>  
>   todo_list.current = i;
>   if (is_fixup(peek_command(_list, 0)))
> @@ -2944,15 +2940,7 @@ int rearrange_squash(void)
>   }
>   }
>  
> - fd = open(todo_file, O_WRONLY);
> - if (fd < 0)
> - res = error_errno(_("could not open '%s'"), todo_file);
> - else if (write(fd, buf.buf, buf.len) < 0)
> - res = error_errno(_("could not write to '%s'"), 
> todo_file);
> - else if (ftruncate(fd, buf.len) < 0)
> - res = error_errno(_("could not truncate '%s'"),
> -todo_file);
> - close(fd);
> + res = rewrite_file(todo_file, buf.buf, buf.len);
>   strbuf_release();
>   }
>  
> -- 
> 2.15.0

Except for that question, it looks good to me (as a beginner), it makes
the code better readable.

Re: [PATCH 2/2] sequencer: use O_TRUNC to truncate files

[Kevin Daudt]

On Tue, Oct 31, 2017 at 10:58:16AM +0100, René Scharfe wrote:
> Cut off any previous content of the file to be rewritten by passing the
> flag O_TRUNC to open(2) instead of calling ftruncate(2) at the end.
> That's easier and shorter.
> 
> Signed-off-by: Rene Scharfe 
> ---
>  sequencer.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index 17360eb38a..f93b60f615 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2668,13 +2668,11 @@ int check_todo_list(void)
>  static int rewrite_file(const char *path, const char *buf, size_t len)
>  {
>   int rc = 0;
> - int fd = open(path, O_WRONLY);
> + int fd = open(path, O_WRONLY | O_TRUNC);
>   if (fd < 0)
>   return error_errno(_("could not open '%s' for writing"), path);
>   if (write_in_full(fd, buf, len) < 0)
>   rc = error_errno(_("could not write to '%s'"), path);
> - if (!rc && ftruncate(fd, len) < 0)
> - rc = error_errno(_("could not truncate '%s'"), path);
>   close(fd);
>   return rc;
>  }
> -- 
> 2.15.0

Makes sense

Re: [PATCH 1/4] remote-mediawiki: add namespace support

[Kevin]

So I shared the patch some time ago (~2 years). Surprisingly its just
now getting attention. I guess some renewed interest in using mediawiki
with git. Myself, however, am no longer using mediawiki. Nor am I
completely clear on what the reasons were for using some variable or
another a couple of years ago. So... the best of luck, sorry I couldn't
be more helpful.

Eric Sunshine:
> On Sun, Oct 29, 2017 at 2:29 PM, Antoine Beaupré <anar...@debian.org> wrote:
>> On 2017-10-29 13:24:03, Eric Sunshine wrote:
>>> On Sun, Oct 29, 2017 at 12:08 PM, Antoine Beaupré <anar...@debian.org> 
>>> wrote:
>>> So, the idea is that if the input has form "something:number", then
>>> you want to look up "something" as a namespace name. Anything else
>>> (such as "something:foobar") is not considered a valid page reference.
>>> Right?
>>
>> frankly, i have no idea what's going on here.
>>
>>> The multiple 'return's are a bit messy. Perhaps collapse the entire
>>> function to something like this:
>>>
>>> sub get_mw_namespace_id_for_page {
>>> my $arg = shift;
>>> if ($arg =~ /^([^:]+):\d+$/) {
>>> return get_mw_namespace_id($1);
>>> }
>>> return undef;
>>> }
>>>
>>> In fact, it may be that the intent of the original code *was* meant to
>>> do exactly the same as shown in my example above, but that the person
>>> who wrote it accidentally typed:
>>>
>>> return get_mw_namespace_id($namespace);
>>>
>>> instead of the intended:
>>>
>>> return get_mw_namespace_id($1);
>>>
>>> So, a minimal fix would be simply to change $namespace to $1.
>>> Tightening the regex as I did in my example would be a bonus (though
>>> probably ought to be a separate patch).
>>
>> so while i'm happy to just copy-paste your code in there, that's kind of
>> a sensitive area of the code, as it was originally used only in the
>> upload procedure, which I haven't tested at all. so i'm hesitant in just
>> merging that in as is.
> 
> I don't think there's a need to copy/paste my example code. If you
> instead make the minimal suggested fix, then the resulting code will
> be effectively equivalent to my example (minus the tighter regex).
> 
>> i don't understand why or how this even works, to be honest: page names
>> don't necessarily look like numbers, in fact, they generally don't. i
>> don't understand why the patch submitted here even touches that function
>> at all, considering that the function is only used on uploads. I just
>> cargo-culted it from the original issue...
> 
> I, myself, am not familiar with or a user of Mediawiki or with the Git
> bridging, and I don't know what page names look like, but I'm pretty
> well convinced from reading both the existing code and this patch that
> the changes to get_mw_namespace_id_for_page() are really just a bug
> fix to that function. My interpretation is that the function really
> was intended to strip the ":id" portion of "name:id" before calling
> get_mw_namespace_id(); the fact that the original code neglects to do
> so seems just an oversight. The fact that the regex uses capturing
> parentheses implies strongly that it was indeed the intention to use
> $1 in the call to get_mw_namespace_id(). Unlike the "fix" in the patch
> you posted from Kevin, which is perhaps unnecessarily complicated, the
> fix I suggested above is about a minimal as possible. That is,
> changing:
> 
>  return get_mw_namespace_id($namespace);
> 
> to:
> 
>  return get_mw_namespace_id($1);
> 
> should achieve the same result. (It could be made more robust by
> tightening the regex as in my example, but that's a separate topic,
> not needed just to get the function to work as intended.)
> 

Re: [PATCH 3/3] builtin/describe: describe blobs

[Kevin Daudt]

On Sat, Oct 28, 2017 at 08:28:54PM -0700, Stefan Beller wrote:
> On Sat, Oct 28, 2017 at 10:32 AM, Johannes Schindelin
> <johannes.schinde...@gmx.de> wrote:
> > 
> > [..]
> > I wonder whether it would make sense to extend this to tree objects while
> > we are at it, but maybe that's an easy up-for-grabs.
> 
> I can look into incorporating that, too. What is the use case though?
> (Is there any error message, common enough that users want to
> identify trees?)
> 
> Thanks for the review,
> Stefan

Not sure if it's really helpfulp, but sometimes with corrup repos, git
would complain about a certain object missing or corrupt, where it might
be usefull to find out how it's referenced. Not sure though if this
would work in that case, because the repo is already corrupt.

Kevin

Re: [PATCH] cherry-pick: add --keep-existing-origin option

[Kevin Daudt]

On Sat, Oct 28, 2017 at 08:04:40PM +0800, Anthony Wong wrote:
> When cherry-picking from a commit whose commit message already
> contains the "(cherry picked from commit ...)" line, this option will
> not add another one. This is useful when you are cherry-picking from a
> bunch of commits, some are cherry-picks and already contains the
> upstream hash but some do not. Use with -x.
> 
> Signed-off-by: Anthony Wong <y...@anthonywong.net>
> ---
>  Documentation/git-cherry-pick.txt |  8 
>  builtin/revert.c  |  2 ++
>  sequencer.c   | 14 --
>  sequencer.h   |  1 +
>  4 files changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/git-cherry-pick.txt 
> b/Documentation/git-cherry-pick.txt
> index d35d771fc..7a074511f 100644
> --- a/Documentation/git-cherry-pick.txt
> +++ b/Documentation/git-cherry-pick.txt
> @@ -71,6 +71,14 @@ OPTIONS
>   development branch), adding this information can be
>   useful.
>  
> +--keep-existing-origin::
> + This option has to be used with -x to take effect. When
> + cherry-picking from a commit whose commit message already
> + contains the "(cherry picked from commit ...)" line, this
> + option will not add another one. This is useful when you are
> + cherry-picking from a bunch of commits, some are cherry-picks
> + and already contains the upstream hash but some do not.
> +
>  -r::
>   It used to be that the command defaulted to do `-x`
>   described above, and `-r` was to disable it.  Now the
> diff --git a/builtin/revert.c b/builtin/revert.c
> index b9d927eb0..a1900cc1d 100644
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -122,6 +122,7 @@ static int run_sequencer(int argc, const char **argv, 
> struct replay_opts *opts)
>   OPT_BOOL(0, "allow-empty", >allow_empty, 
> N_("preserve initially empty commits")),
>   OPT_BOOL(0, "allow-empty-message", 
> >allow_empty_message, N_("allow commits with empty messages")),
>   OPT_BOOL(0, "keep-redundant-commits", 
> >keep_redundant_commits, N_("keep redundant, empty commits")),
> + OPT_BOOL(0, "keep-existing-origin", 
> >keep_existing_origin, N_("do not add another hash if one already 
> exists, use with -x")),
>   OPT_END(),
>   };
>   options = parse_options_concat(options, cp_extra);
> @@ -157,6 +158,7 @@ static int run_sequencer(int argc, const char **argv, 
> struct replay_opts *opts)
>   "--ff", opts->allow_ff,
>   "--rerere-autoupdate", opts->allow_rerere_auto 
> == RERERE_AUTOUPDATE,
>   "--no-rerere-autoupdate", 
> opts->allow_rerere_auto == RERERE_NOAUTOUPDATE,
> + "--keep-existing-origin", 
> opts->keep_existing_origin,
>   NULL);
>   }
>  
> diff --git a/sequencer.c b/sequencer.c
> index f2a10cc4f..c96add16e 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -1050,12 +1050,14 @@ static int do_pick_commit(enum todo_command command, 
> struct commit *commit,
>   strbuf_addstr(, p);
>  
>   if (opts->record_origin) {
> - strbuf_complete_line();
> - if (!has_conforming_footer(, NULL, 0))
> - strbuf_addch(, '\n');
> - strbuf_addstr(, cherry_picked_prefix);
> - strbuf_addstr(, oid_to_hex(>object.oid));
> - strbuf_addstr(, ")\n");
> + if (!opts->keep_existing_origin || strstr(msgbuf.buf, 
> cherry_picked_prefix) == NULL) {
> + strbuf_complete_line();
> + if (!has_conforming_footer(, NULL, 0))
> + strbuf_addch(, '\n');
> + strbuf_addstr(, cherry_picked_prefix);
> + strbuf_addstr(, 
> oid_to_hex(>object.oid));
> + strbuf_addstr(, ")\n");
> + }
>   }
>   }
>  
> diff --git a/sequencer.h b/sequencer.h
> index 6f3d3df82..a907c0947 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -24,6 +24,7 @@ struct replay_opts {
>   int allow_empty;
>   int allow_empty_message;
>   int keep_redundant_commits;
> + int keep_existing_origin;
>   int verbose;
>  
>   int mainline;
> -- 
> 2.14.1
> 

I'm wondering if it isn't better to detect that there is already an
origin present and not add another one. 

Or are there situations where you do want multiple cherry-pick origins?

Kevin

Re: [PATCH] status: do not get confused by submodules in excluded directories

[Kevin Daudt]

On Tue, Oct 17, 2017 at 03:10:11PM +0200, Johannes Schindelin wrote:
> We meticulously pass the `exclude` flag to the `treat_directory()`
> function so that we can indicate that files in it are excluded rather
> than untracked when recursing.
> 
> But we did not yet treat submodules the same way.
> 
> Signed-off-by: Johannes Schindelin 
> ---
> Published-As: 
> https://github.com/dscho/git/releases/tag/submodule-in-excluded-v1
> Fetch-It-Via: git fetch https://github.com/dscho/git submodule-in-excluded-v1
>  dir.c  |  2 +-
>  t/t7061-wtstatus-ignore.sh | 14 ++
>  2 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/dir.c b/dir.c
> index 1d17b800cf3..9987011da57 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -1392,7 +1392,7 @@ static enum path_treatment treat_directory(struct 
> dir_struct *dir,
>   if (!(dir->flags & DIR_NO_GITLINKS)) {
>   unsigned char sha1[20];
>   if (resolve_gitlink_ref(dirname, "HEAD", sha1) == 0)
> - return path_untracked;
> + return exclude ? path_excluded : path_untracked;
>   }
>   return path_recurse;
>   }
> diff --git a/t/t7061-wtstatus-ignore.sh b/t/t7061-wtstatus-ignore.sh
> index fc6013ba3c8..8c849a4cd2f 100755
> --- a/t/t7061-wtstatus-ignore.sh
> +++ b/t/t7061-wtstatus-ignore.sh
> @@ -272,4 +272,18 @@ test_expect_success 'status ignored tracked directory 
> with uncommitted file in t
>   test_cmp expected actual
>  '
>  
> +cat >expected <<\EOF
> +!! tracked/submodule/
> +EOF
> +
> +test_expect_success 'status ignores submodule in excluded directory' '
> + git init tracked/submodule &&
> + (
> + cd tracked/submodule &&
> + test_commit initial
> + ) &&

Could this use test_commit -C tracked/submodule initial?

> + git status --porcelain --ignored -u tracked/submodule >actual &&
> + test_cmp expected actual
> +'
> +
>  test_done
> 
> base-commit: 111ef79afe185f8731920569450f6a65320f5d5f
> -- 
> 2.14.2.windows.3

Re: [PATCH v2] column: show auto columns when pager is active

[Kevin Daudt]

On Mon, Oct 23, 2017 at 02:52:46PM -0700, Jonathan Nieder wrote:
> Hi,
> 
> Kevin Daudt wrote:
> 
> > --- a/column.c
> > +++ b/column.c
> > @@ -5,6 +5,7 @@
> >  #include "parse-options.h"
> >  #include "run-command.h"
> >  #include "utf8.h"
> > +#include "pager.c"
> 
> Should this be pager.h?
> 
> Thanks,
> Jonathan

Thanks for catching this.

Re: [PATCH v2 1/9] perf/run: add '--config' option to the 'run' script

[Kevin Daudt]

On Sat, Sep 23, 2017 at 07:55:56PM +, Christian Couder wrote:
> It is error prone and tiring to use many long environment
> variables to give parameters to the 'run' script.
> 
> Let's make it easy to store some parameters in a config
> file and to pass them to the run script.
> 
> The GIT_PERF_CONFIG_FILE variable will be set to the
> argument of the '--config' option. This variable is not
> used yet. It will be used in a following commit.
> 
> Signed-off-by: Christian Couder 
> ---
>  t/perf/run | 7 ++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/t/perf/run b/t/perf/run
> index beb4acc0e428d..1e7c2a59e45dc 100755
> --- a/t/perf/run
> +++ b/t/perf/run
> @@ -2,9 +2,14 @@
>  
>  case "$1" in
>   --help)
> - echo "usage: $0 [other_git_tree...] [--] [test_scripts]"
> + echo "usage: $0 [--config file] [other_git_tree...] [--] 
> [test_scripts]"
>   exit 0
>   ;;
> + --config)
> + shift
> + GIT_PERF_CONFIG_FILE=$(cd "$(dirname "$1")"; pwd)/$(basename 
> "$1")

Is the idea of this construct to do some kind of normalization?
Otherwise it seems to just result in $1 again.

> + export GIT_PERF_CONFIG_FILE
> + shift ;;
>  esac
>  
>  die () {
> 
> --
> https://github.com/git/git/pull/408

Re: Multiple paths in GIT_EXEC_PATH

[Kevin Daudt]

On Tue, Oct 17, 2017 at 06:21:22PM +0300, Nikolay Yakimov wrote:
> Hello. After updating to a recent git release (2.14, I believe, but
> possibly earlier), setting GIT_EXEC_PATH to multiple directories
> stopped working. It did work before, and I believe the culprit is
> 'git-sh-setup', which uses 'git --exec-path' output directly, while
> most other git components observe PATH syntax, i.e. multiple paths
> with colon separator. Is this intended, or is it an oversight?
> 
> For why I need that is another matter. Long story short, I need git to
> look for '.gitignore' in a particular non-standard location, since I
> have multiple git repositories in the same workdir (that workdir being
> $HOME and git repositories being stores for my different configs)

The commit that changed what you described is 1073094f3 (git-sh-setup:
be explicit where to dot-source git-sh-i18n from., 2016-10-29). That
commit claims there were already scripts that assumed GIT_EXEC_PATH is
just a single entry. That commit was included in v2.11.

There was also a recent thread[0] about it that discussed this issue,
where someone stated that indeed treating GIT_EXEC_PATH with the same
semantics as PATH has been broken for a while, but it seems there are no
real plans to fix it.

[0]:https://public-inbox.org/git/20170928223134.GA30744@varnish/

Re: [PATCH] check-ref-format: require a repository for --branch

[Kevin Daudt]

On Tue, Oct 17, 2017 at 11:40:17AM +0900, Junio C Hamano wrote:
> Kevin Daudt <m...@ikke.info> writes:
> 
> >> +  setup_git_directory_gently();
> >> +
> >> +  if (!nongit)
> >> +  malformed = (strbuf_check_branch_ref(, arg) ||
> >> +   !skip_prefix(sb.buf, "refs/heads/", ));
> >> +  else
> >> +  malformed = check_branch_ref_format(arg);
> >> +
> >
> > Would it make sense to swap the logic and get rid of the double
> > negative (!nongit)?
> 
> I am trying to follow the pattern "handle the normal case that have
> been supported forever first, and then handle new exception next",
> so that it is easier to see that there is no behaviour change in the
> normal case, so I do not think it makes it easier to see to swap the
> if/else cases.

Ok, thanks for your reasoning, makes sense.
> >
> >> +  if (malformed)
> >>die("'%s' is not a valid branch name", arg);
> >> -  printf("%s\n", sb.buf + 11);
> >> +  printf("%s\n", name);
> >> +  strbuf_release();
> >>return 0;
> >>  }
> >>  

Re: [PATCH] check-ref-format: require a repository for --branch

[Kevin Daudt]

On Mon, Oct 16, 2017 at 07:45:46PM +0900, Junio C Hamano wrote:
> Junio C Hamano  writes:
> 
> [..]
> 
> diff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c
> index 1e5f9835f0..4e62852089 100644
> --- a/builtin/check-ref-format.c
> +++ b/builtin/check-ref-format.c
> @@ -38,12 +38,22 @@ static char *collapse_slashes(const char *refname)
>  
>  static int check_ref_format_branch(const char *arg)
>  {
> + int nongit, malformed;
>   struct strbuf sb = STRBUF_INIT;
> + const char *name = arg;
>  
> - setup_git_directory();
> - if (strbuf_check_branch_ref(, arg))
> + setup_git_directory_gently();
> +
> + if (!nongit)
> + malformed = (strbuf_check_branch_ref(, arg) ||
> +  !skip_prefix(sb.buf, "refs/heads/", ));
> + else
> + malformed = check_branch_ref_format(arg);
> +

Would it make sense to swap the logic and get rid of the double
negative (!nongit)?

> + if (malformed)
>   die("'%s' is not a valid branch name", arg);
> - printf("%s\n", sb.buf + 11);
> + printf("%s\n", name);
> + strbuf_release();
>   return 0;
>  }
>  

Re: [PATCH v3 00/25] object_id part 10

[Kevin Daudt]

On Mon, Oct 16, 2017 at 11:49:13PM +, brian m. carlson wrote:
> On Mon, Oct 16, 2017 at 11:15:34AM +0900, Junio C Hamano wrote:
> > With a hope that this might help other reviewers, here is the
> > interdiff between "the previous one merged with v2.15-rc1" and "this
> > round applied on v2.15-rc1 directly".  
> > 
> > The changes all looked sensible to me.  Thanks.
> 
> Is there a reasonably straightforward tool or workflow to generate
> interdiffs?  If so, I can include them in the future.
> -- 
> brian m. carlson / brian with sandals: Houston, Texas, US
> https://www.crustytoothpaste.net/~bmc | My opinion only
> OpenPGP: https://keybase.io/bk2204

tbdiff: https://github.com/trast/tbdiff

Re: [PATCH v4 03/11] protocol: introduce protocol extension mechanisms

[Kevin Daudt]

On Mon, Oct 16, 2017 at 10:55:24AM -0700, Brandon Williams wrote:
> Create protocol.{c,h} and provide functions which future servers and
> clients can use to determine which protocol to use or is being used.
> 
> Also introduce the 'GIT_PROTOCOL' environment variable which will be
> used to communicate a colon separated list of keys with optional values
> to a server.  Unknown keys and values must be tolerated.  This mechanism
> is used to communicate which version of the wire protocol a client would
> like to use with a server.
> 
> Signed-off-by: Brandon Williams 
> ---
>  Documentation/config.txt | 17 +++
>  Documentation/git.txt|  6 
>  Makefile |  1 +
>  cache.h  |  8 +
>  protocol.c   | 79 
> 
>  protocol.h   | 33 
>  6 files changed, 144 insertions(+)
>  create mode 100644 protocol.c
>  create mode 100644 protocol.h
>
> [...]
> 
> diff --git a/protocol.h b/protocol.h
> new file mode 100644
> index 0..1b2bc94a8
> --- /dev/null
> +++ b/protocol.h
> @@ -0,0 +1,33 @@
> +#ifndef PROTOCOL_H
> +#define PROTOCOL_H
> +
> +enum protocol_version {
> + protocol_unknown_version = -1,
> + protocol_v0 = 0,
> + protocol_v1 = 1,
> +};
> +
> +/*
> + * Used by a client to determine which protocol version to request be used 
> when
> + * communicating with a server, reflecting the configured value of the
> + * 'protocol.version' config.  If unconfigured, a value of 'protocol_v0' is
> + * returned.
> + */

The first sentence reads a little weird to me around 'which version to
request be used'. 

[PATCH v2] column: show auto columns when pager is active

[Kevin Daudt]

When columns are set to automatic for git tag and the output is
paginated by git, the output is a single column instead of multiple
columns.

Standard behaviour in git is to honor auto values when the pager is
active, which happens for example with commands like git log showing
colors when being paged.

Since ff1e72483 (tag: change default of `pager.tag` to "on",
2017-08-02), the pager has been enabled by default, exposing this
problem to more people.

finalize_colopts in column.c only checks whether the output is a TTY to
determine if columns should be enabled with columns set to auto. Also
check if the pager is active.

Adding a test for git column is possible but requires some care to work
around a race on stdin. See commit 18d8c2693 (test_terminal: redirect
child process' stdin to a pty, 2015-08-04). Test git tag instead, since
that does not involve stdin, and since that was the original motivation
for this patch.

Helped-by: Rafael Ascensão <rafa.al...@gmail.com>
Signed-off-by: Kevin Daudt <m...@ikke.info>
---
Changes since v1:

- Remove redundant -p flag in tests
- Explain why git tag is being tested instead of the more obvious git
  column

 column.c |  3 ++-
 t/t7006-pager.sh | 14 ++
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/column.c b/column.c
index ff7bdab1a..ded50337f 100644
--- a/column.c
+++ b/column.c
@@ -5,6 +5,7 @@
 #include "parse-options.h"
 #include "run-command.h"
 #include "utf8.h"
+#include "pager.c"
 
 #define XY2LINEAR(d, x, y) (COL_LAYOUT((d)->colopts) == COL_COLUMN ? \
(x) * (d)->rows + (y) : \
@@ -224,7 +225,7 @@ int finalize_colopts(unsigned int *colopts, int 
stdout_is_tty)
if (stdout_is_tty < 0)
stdout_is_tty = isatty(1);
*colopts &= ~COL_ENABLE_MASK;
-   if (stdout_is_tty)
+   if (stdout_is_tty || pager_in_use())
*colopts |= COL_ENABLED;
}
return 0;
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index f0f1abd1c..e985b6873 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -570,4 +570,18 @@ test_expect_success 'command with underscores does not 
complain' '
test_cmp expect actual
 '
 
+test_expect_success TTY 'git tag with auto-columns ' '
+   test_commit one &&
+   test_commit two &&
+   test_commit three &&
+   test_commit four &&
+   test_commit five &&
+   cat >expected <<\EOF &&
+initial  one  two  threefour five
+EOF
+   test_terminal env PAGER="cat >actual.tag" COLUMNS=80 \
+   git -c column.ui=auto tag --sort=authordate &&
+   test_cmp expected actual.tag
+'
+
 test_done
-- 
2.14.2

Re: Can I remove multiple stashed states at a time?

[Kevin Daudt]

On Fri, Oct 13, 2017 at 01:35:22PM -0400, Jeff King wrote:
> On Fri, Oct 13, 2017 at 11:58:12AM +0900, 小川恭史 wrote:
> 
> You can also just do:
> 
>   for i in 1 2 3; do
>  git stash drop $i
>   done
> 

Doesn't stash 2 become stash 1 after the first drop, and the same for 3,
resulting in dropping stash 1, 3 and 5?

So something like

  for i in 3 2 1; do
  git stash drop $i;
  done

Or leave $i out altogether.

Re: [PATCH v2 2/3] for-each-ref: let upstream/push optionally remote ref name

[Kevin Daudt]

On Thu, Oct 05, 2017 at 02:19:15PM +0200, Johannes Schindelin wrote:
> From: J Wyman 
> [..] 
> 
> diff --git a/Documentation/git-for-each-ref.txt 
> b/Documentation/git-for-each-ref.txt
> index 39f50bd53eb..22767025850 100644
> --- a/Documentation/git-for-each-ref.txt
> +++ b/Documentation/git-for-each-ref.txt
> @@ -142,8 +142,9 @@ upstream::
>   encountered. Append `:track,nobracket` to show tracking
>   information without brackets (i.e "ahead N, behind M").
>  +
> -Also respects `:remotename` to state the name of the *remote* instead of
> -the ref.
> +Also respects `:remotename` to state the name of the *remote* instead
> +of the ref, and `:remoteref` to state the name of the *reference* as
> +locally known by the remote.

What does "locally known by the remote" mean in this sentence?

>  +
>  Has no effect if the ref does not have tracking information associated
>  with it.  All the options apart from `nobracket` are mutually exclusive,
> @@ -152,9 +153,9 @@ but if used together the last option is selected.

Re: [PATCH] Documentation/merge-options.txt: Add -S/--gpg-sign

[Kevin Daudt]

On Thu, Oct 12, 2017 at 12:44:59PM +0200, Kevin Daudt wrote:
> On Thu, Oct 12, 2017 at 02:02:17AM -0700, W. Trevor King wrote:
> > Pull has supported these since ea230d8 (pull: add the --gpg-sign
> > option, 2014-02-10).  Insert in long-option alphabetical order
> > following 7c85d274 (Documentation/merge-options.txt: order options in
> > alphabetical groups, 2009-10-22).
> > 
> > Signed-off-by: W. Trevor King <wk...@tremily.us>
> > ---
> > This patch is based on maint.  It will have trivial conflicts with the
> > --signoff docs which landed in 14d01b4f07 (merge: add a --signoff
> > flag, 2017-07-04, v2.15.0-rc0~138^2).
> > 
> >  Documentation/git-merge.txt | 6 --
> >  Documentation/merge-options.txt | 6 ++
> >  2 files changed, 6 insertions(+), 6 deletions(-)
> > 
> > diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
> > index f90faf7aaa..1d97a17904 100644
> > --- a/Documentation/git-merge.txt
> > +++ b/Documentation/git-merge.txt
> > @@ -64,12 +64,6 @@ OPTIONS
> >  ---
> >  include::merge-options.txt[]
> >  
> > --S[]::
> > ---gpg-sign[=]::
> 
> Shouldn't the options self be removed here too, not just the
> explanation?
> 

You can ignore this, it was just my mail client that colored the diff
wrong, confusing me.

> > -   GPG-sign the resulting merge commit. The `keyid` argument is
> > -   optional and defaults to the committer identity; if specified,
> > -   it must be stuck to the option without a space.
> > -
> >  -m ::
> > Set the commit message to be used for the merge commit (in
> > case one is created).
> > diff --git a/Documentation/merge-options.txt 
> > b/Documentation/merge-options.txt
> > index 5b4a62e936..6d85a76872 100644
> > --- a/Documentation/merge-options.txt
> > +++ b/Documentation/merge-options.txt
> > @@ -42,6 +42,12 @@ set to `no` at the beginning of them.
> > current `HEAD` is already up-to-date or the merge can be
> > resolved as a fast-forward.
> >  
> > +-S[]::
> > +--gpg-sign[=]::
> > +   GPG-sign the resulting merge commit. The `keyid` argument is
> > +   optional and defaults to the committer identity; if specified,
> > +   it must be stuck to the option without a space.
> > +
> >  --log[=]::
> >  --no-log::
> > In addition to branch names, populate the log message with
> > -- 
> > 2.13.6
> > 

Re: [PATCH] Documentation/merge-options.txt: Add -S/--gpg-sign

[Kevin Daudt]

On Thu, Oct 12, 2017 at 02:02:17AM -0700, W. Trevor King wrote:
> Pull has supported these since ea230d8 (pull: add the --gpg-sign
> option, 2014-02-10).  Insert in long-option alphabetical order
> following 7c85d274 (Documentation/merge-options.txt: order options in
> alphabetical groups, 2009-10-22).
> 
> Signed-off-by: W. Trevor King 
> ---
> This patch is based on maint.  It will have trivial conflicts with the
> --signoff docs which landed in 14d01b4f07 (merge: add a --signoff
> flag, 2017-07-04, v2.15.0-rc0~138^2).
> 
>  Documentation/git-merge.txt | 6 --
>  Documentation/merge-options.txt | 6 ++
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
> index f90faf7aaa..1d97a17904 100644
> --- a/Documentation/git-merge.txt
> +++ b/Documentation/git-merge.txt
> @@ -64,12 +64,6 @@ OPTIONS
>  ---
>  include::merge-options.txt[]
>  
> --S[]::
> ---gpg-sign[=]::

Shouldn't the options self be removed here too, not just the
explanation?

> - GPG-sign the resulting merge commit. The `keyid` argument is
> - optional and defaults to the committer identity; if specified,
> - it must be stuck to the option without a space.
> -
>  -m ::
>   Set the commit message to be used for the merge commit (in
>   case one is created).
> diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
> index 5b4a62e936..6d85a76872 100644
> --- a/Documentation/merge-options.txt
> +++ b/Documentation/merge-options.txt
> @@ -42,6 +42,12 @@ set to `no` at the beginning of them.
>   current `HEAD` is already up-to-date or the merge can be
>   resolved as a fast-forward.
>  
> +-S[]::
> +--gpg-sign[=]::
> + GPG-sign the resulting merge commit. The `keyid` argument is
> + optional and defaults to the committer identity; if specified,
> + it must be stuck to the option without a space.
> +
>  --log[=]::
>  --no-log::
>   In addition to branch names, populate the log message with
> -- 
> 2.13.6
> 

Re: Unable to use --patch with git add

[Kevin Daudt]

On Wed, Oct 11, 2017 at 11:16:39PM +0530, Ayush Goel wrote:
> $ git --version
> git version 2.14.2
> 
> What more details can I provide to help debug this?
> 

Hello Ayush,

Run:

  git config --global color.ui auto
  git config --unset --local color.ui #in case it's set locally

This is a known 'breakage' because people have set color.ui to always
(which should not be used anyway).

Kind regards, Kevin

Re: [PATCH] column: show auto columns when pager is active

[Kevin Daudt]

On Wed, Oct 11, 2017 at 08:12:35PM +0200, Martin Ågren wrote:
> On 11 October 2017 at 19:23, Kevin Daudt <m...@ikke.info> wrote:
> > finalize_colopts in column.c only checks whether the output is a TTY to
> > determine if columns should be enabled with columns set to auto. Also check
> > if the pager is active.
> 
> Maybe you could say something about the difficulties of writing a test
> for `git column` proper. Something like this perhaps:
> 
>   Adding a test for git column is possible but requires some care to
>   work around a race on stdin. See commit 18d8c2693 (test_terminal:
>   redirect child process' stdin to a pty, 2015-08-04). Test git tag
>   instead, since that does not involve stdin, and since that was the
>   original motivation for this patch.

Right, makes sense.
> 
> > Helped-by: Rafael Ascensão <rafa.al...@gmail.com>
> > Signed-off-by: Kevin Daudt <m...@ikke.info>
> > ---
> >  column.c |  3 ++-
> >  t/t7006-pager.sh | 14 ++
> >  2 files changed, 16 insertions(+), 1 deletion(-)
> 
> Does the documentation on `column.ui` need to be updated? It talks about
> "if the output is to the terminal". That's similar to the documentation
> on the various `color.*`, so we should be fine, and arguably it's even
> better not to say anything since that makes it consistent.
> 
> > diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
> > index f0f1abd1c..44c2ca5d3 100755
> > --- a/t/t7006-pager.sh
> > +++ b/t/t7006-pager.sh
> > @@ -570,4 +570,18 @@ test_expect_success 'command with underscores does not 
> > complain' '
> > test_cmp expect actual
> >  '
> >
> > +test_expect_success TTY 'git tag with auto-columns ' '
> > +   test_commit one &&
> > +   test_commit two &&
> > +   test_commit three &&
> > +   test_commit four &&
> > +   test_commit five &&
> > +   cat >expected <<\EOF &&
> > +initial  one  two  threefour five
> > +EOF
> > +   test_terminal env PAGER="cat >actual.tag" COLUMNS=80 \
> > +   git -p -c column.ui=auto tag --sort=authordate &&
> > +   test_cmp expected actual.tag
> > +'
> > +
> >  test_done
> 
> Since `git tag` pages when it's listing, you don't need the `-p`. But
> it's not like it hurts to have it. Yeah, I know, you needed it with `git
> column`. :-)

Right, it was a bit of a left-over since I assumed the PAGER='cat 
>paginated.out'
from the beginning of the test was in place and I wasn't getting any
output, but it turned out PAGER wasn't set.

> I wonder if it's useful to set COLUMNS a bit lower so that this has to
> split across more than one line (but not six), i.e., to do something
> non-trivial. I suppose that might lower the chances of some weird
> breakage slipping through.

Yeah, I was doubting about that, but wouldn't this amount to testing
whether git column is working properly, instead of just testing whether
it's being done at all?

> This test uses "actual.tag" while most (all?) others in this file use
> "actual". Maybe you worry about checking the "wrong" file, e.g., in case
> the pager doesn't kick in. You could do `rm -f actual` before the
> `test_terminal`-invocation to protect against that.

Yeah, I actually ran into that, but rm-ing it is better, I agree.

> These were just the thoughts that occurred to me, not sure if any of
> them is particularly significant. Thanks for cleaning up after me.
> 

np. Just as I posted earlier, I think you did not actually cause the bug
(because this has never worked), it just made it visible to more users.

Kevin

[PATCH] column: show auto columns when pager is active

[Kevin Daudt]

When columns are set to automatic for git tag and the output is
paginated by git, the output is a single column instead of multiple
columns.

Standard behaviour in git is to honor auto values when the pager is
active, which happens for example with commands like git log showing
colors when being paged.

Since ff1e72483 (tag: change default of `pager.tag` to "on",
2017-08-02), the pager has been enabled by default, exposing this
problem to more people.

finalize_colopts in column.c only checks whether the output is a TTY to
determine if columns should be enabled with columns set to auto. Also check
if the pager is active.

Helped-by: Rafael Ascensão <rafa.al...@gmail.com>
Signed-off-by: Kevin Daudt <m...@ikke.info>
---
 column.c |  3 ++-
 t/t7006-pager.sh | 14 ++
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/column.c b/column.c
index ff7bdab1a..ded50337f 100644
--- a/column.c
+++ b/column.c
@@ -5,6 +5,7 @@
 #include "parse-options.h"
 #include "run-command.h"
 #include "utf8.h"
+#include "pager.c"
 
 #define XY2LINEAR(d, x, y) (COL_LAYOUT((d)->colopts) == COL_COLUMN ? \
(x) * (d)->rows + (y) : \
@@ -224,7 +225,7 @@ int finalize_colopts(unsigned int *colopts, int 
stdout_is_tty)
if (stdout_is_tty < 0)
stdout_is_tty = isatty(1);
*colopts &= ~COL_ENABLE_MASK;
-   if (stdout_is_tty)
+   if (stdout_is_tty || pager_in_use())
*colopts |= COL_ENABLED;
}
return 0;
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index f0f1abd1c..44c2ca5d3 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -570,4 +570,18 @@ test_expect_success 'command with underscores does not 
complain' '
test_cmp expect actual
 '
 
+test_expect_success TTY 'git tag with auto-columns ' '
+   test_commit one &&
+   test_commit two &&
+   test_commit three &&
+   test_commit four &&
+   test_commit five &&
+   cat >expected <<\EOF &&
+initial  one  two  threefour five
+EOF
+   test_terminal env PAGER="cat >actual.tag" COLUMNS=80 \
+   git -p -c column.ui=auto tag --sort=authordate &&
+   test_cmp expected actual.tag
+'
+
 test_done
-- 
2.14.2

Re: [RFC] column: show auto columns when pager is active

[Kevin Daudt]

On Tue, Oct 10, 2017 at 07:02:00PM +0200, Martin Ågren wrote:
> On 10 October 2017 at 16:01, Jeff King <p...@peff.net> wrote:
> > On Tue, Oct 10, 2017 at 12:30:49PM +0200, Martin Ågren wrote:
> >> On 9 October 2017 at 23:45, Kevin Daudt <m...@ikke.info> wrote:
> >> > Since ff1e72483 (tag: change default of `pager.tag` to "on",
> >> > 2017-08-02), the pager has been enabled by default, exposing this
> >> > problem to more people.
> >>
> >> Oh. :( I didn't know about "column" to be honest.
> >
> > Yeah, I didn't think of that with respect to the pager. This is a
> > regression in v2.14.2, I think.
> 
> Yes.

Though 2.14.2 enabled the pager by default, even before that when
someone would've enabled the pager theirselves (by setting pager.tag for
example), it would also shown just a single column.

I could reproduce it as far as 2.8.3 (I could not test further due to
libssl incompattibility).

Re: [RFC] column: show auto columns when pager is active

[Kevin Daudt]

On Tue, Oct 10, 2017 at 07:04:42PM +0200, Martin Ågren wrote:
> On 10 October 2017 at 16:29, Jeff King <p...@peff.net> wrote:
> > On Tue, Oct 10, 2017 at 10:10:19AM -0400, Jeff King wrote:
> >
> > it will randomly succeed or fail, depending on whether sed manages to
> > read the input before the stdin terminal is closed.
> >
> > I'm not sure of an easy way to fix test-terminal, but we could work
> > around it like this (which also uses "-p" to actually invoke the pager,
> > and uses a pager that makes it clear when it's being run):
> >
> > diff --git a/t/t9002-column.sh b/t/t9002-column.sh
> > index 9441145bf0..d322c3b745 100755
> > --- a/t/t9002-column.sh
> > +++ b/t/t9002-column.sh
> > @@ -180,14 +180,14 @@ EOF
> >
> >  test_expect_success TTY '20 columns, mode auto, pager' '
> > cat >expected <<\EOF &&
> > -oneseven
> > -twoeight
> > -three  nine
> > -four   ten
> > -five   eleven
> > -six
> > +paged:oneseven
> > +paged:twoeight
> > +paged:three  nine
> > +paged:four   ten
> > +paged:five   eleven
> > +paged:six
> >  EOF
> > -   test_terminal env PAGER="cat|cat" git column --mode=auto  > >actual &&
> > +   test_terminal env PAGER="sed s/^/paged:/" sh -c "git -p column 
> > --mode=auto <lista" >actual &&
> > test_cmp expected actual
> >  '
> >  test_done
> 
> Makes sense. FWIW, I don't see the flakyness with this.
> 
> > All that said, I think I'd just as soon test a real command like "git
> > tag", which doesn't care about reading from stdin.
> 
> For reference for Kevin, in case you consider testing, e.g., git tag,
> the main reason I referred to the test I posted as "hacky", was that I
> just inserted it at a more-or-less random place in t7006. So it had to
> play with `git reset` to avoid upsetting the later tests. It could
> obviously go to the end instead, but I was too lazy to move it and
> define a pager.

Thanks Jeff and Martin, I will use your tips to build a test based on
git tag instead.

[RFC] column: show auto columns when pager is active

[Kevin Daudt]

When columns are set to automatic for git tag and the output is
paginated by git, the output is a single column instead of multiple
columns.

Standard behaviour in git is to honor auto values when the pager is
active, which happens for example with commands like git log showing
colors when being paged.

Since ff1e72483 (tag: change default of `pager.tag` to "on",
2017-08-02), the pager has been enabled by default, exposing this
problem to more people.

finalize_colopts in column.c only checks whether the output is a TTY to
determine if columns should be enabled with columns set to autol. Also check
if the pager is active.

Helped-by: Rafael Ascensão <rafa.al...@gmail.com>
Signed-off-by: Kevin Daudt <m...@ikke.info>
---
This came to light when someone wondered on irc why
column.tag=[auto|always] did not work on Mac OS. Testing it myself, I
found it to work with always, but not with auto.

I could not get the test to work yet, because somehow it's not giving
any output, so feedback regarding that is welcome.


 column.c  |  3 ++-
 t/t9002-column.sh | 13 +
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/column.c b/column.c
index ff7bdab1a..ded50337f 100644
--- a/column.c
+++ b/column.c
@@ -5,6 +5,7 @@
 #include "parse-options.h"
 #include "run-command.h"
 #include "utf8.h"
+#include "pager.c"
 
 #define XY2LINEAR(d, x, y) (COL_LAYOUT((d)->colopts) == COL_COLUMN ? \
(x) * (d)->rows + (y) : \
@@ -224,7 +225,7 @@ int finalize_colopts(unsigned int *colopts, int 
stdout_is_tty)
if (stdout_is_tty < 0)
stdout_is_tty = isatty(1);
*colopts &= ~COL_ENABLE_MASK;
-   if (stdout_is_tty)
+   if (stdout_is_tty || pager_in_use())
*colopts |= COL_ENABLED;
}
return 0;
diff --git a/t/t9002-column.sh b/t/t9002-column.sh
index 89983527b..9441145bf 100755
--- a/t/t9002-column.sh
+++ b/t/t9002-column.sh
@@ -2,6 +2,7 @@
 
 test_description='git column'
 . ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-terminal.sh
 
 test_expect_success 'setup' '
cat >lista <<\EOF
@@ -177,4 +178,16 @@ EOF
test_cmp expected actual
 '
 
+test_expect_success TTY '20 columns, mode auto, pager' '
+   cat >expected <<\EOF &&
+oneseven
+twoeight
+three  nine
+four   ten
+five   eleven
+six
+EOF
+   test_terminal env PAGER="cat|cat" git column --mode=auto actual 
&&
+   test_cmp expected actual
+'
 test_done
-- 
2.14.2

Re: "git rm" seems to do recursive removal even without "-r"

[Kevin Daudt]

On Sun, Oct 08, 2017 at 05:07:12AM -0400, Robert P. J. Day wrote:
> On Sun, 8 Oct 2017, Junio C Hamano wrote:
> 
> > "Robert P. J. Day" <rpj...@crashcourse.ca> writes:
> >
> > > ... so if, in the kernel source
> > > tree, i ran:
> > >
> > >   $ git rm \*.c
> > >
> > > i would end up removing *all* 25,569 "*.c" files in the kernel source
> > > repository.
> >
> > Yes, as that is exactly what the command line asks Git to do.
> 
>   so if i wanted git to remove, say, all files named "Makefile*" from
> everywhere in the linux kernel source tree, the (dry run) command
> would be:
> 
>   $ git rm -n Makefile\*
> 
> is that it? let's try that:
> 
>   $ git rm -n Makefile\*
>   rm 'Makefile'
>   $
> 
> oops.
> 
> rday
> 

So your question is not su much  about the recursive option (delete
mentioned directories, including their contents), but globbing
(expanding the * to any files matching the pattern).

The explanation of  mentions this:

   Files to remove. Fileglobs (e.g.  *.c) can be given to remove all
   matching files.

This indicates that git itself (not your shell alone) does file
globbing.

I think the confusing part is that most people have no clear idea of the
separation between what the shell sees and interprets, and what the
program actually gets.

When you execute:

$ git rm Makefile\*

What git actually sees is this:

Makefile*

The shell intepreted the \* to mean just '*' and not interpret it
itself, and provide that to the executed program. Git, in its turn,
would start matching any file to that pattern to see which files
matches.


If you would execute:

$ git rm 'Makefile\*'

Git would see:

Makefile\*

Which does the thing you intended. So you have to deal with 2 levels of
programs interpreting the arguments, not just one.

Whether '*.c' should match just all .c files in the current dir, or all
files ending with .c depends on whether the path seperator is matched by
* or not and is a separate discussion.

GITGLOSSARY(7) under pathspec mentions this:

globGit treats the pattern as a shell glob suitable for consumption
by fnmatch(3) with the FNM_PATHNAME flag: wildcards in the
pattern will not match a / in the pathname.

So that seems to indicate '*.c' should only match .c files in the
current dir. I'm not sure why that's not the case.

I hope this clears up what's happening a bit, and perhaps can lead to
improvements to the documentation so that it's not so surprising.

Kind regards, Kevin.

Re: [bug] git add -p breaks, if color.ui is set to "always"

[Kevin Daudt]

On Fri, Oct 06, 2017 at 02:47:30PM +0200, Alexander Gehrke wrote:
> After an update to version 2.14.2 from 2.14.1 "git add --patch" stopped 
> working
> for me, just producing the same output as "git diff", but not prompting to 
> stage
> anything.
> 
> I found that unsetting the config key color.ui, which was set to "always" 
> fixed
> the problem.
> 
> From the manpage, color.ui should not have that effect and "always" should be 
> a
> legal value.
> 
> Regards
> Alexander Gehrke

Hello Alexander,

There have been a few mailing-list posts[0] about this already. While
git add -p should probably not have broken by this, setting ui.color to
always itself does not make a lot of sense either.

You are telling git to always output color, even when the target is
something that does not know what to do with the color codes. Setting it
to 'auto' would make more sense.

The thread I posted to discusses some changes that might get introduced
to improve the situation though.

Kevin.

[0}:https://public-inbox.org/git/20171003093157.gq7za2fwcqsou...@sigill.intra.peff.net/T/

Re: distinguishing between staged and unstaged content in a stash?

[Kevin Daudt]

On Wed, Oct 04, 2017 at 07:10:46AM -0400, rpj...@crashcourse.ca wrote:
> 
>   couple (admittedly trivial) questions about stashing. first, can i
> clarify that when one stashes content, a stash *always* distinguishes
> between what was staged, and what was unstaged? that is, when one is
> stashing, the "--keep-index" option relates to whether or not staged
> changes are left in the index (and, consequently, in the working
> directory as well), but that option has no effect on the final content
> of the stash, yes? even if "--keep-index" is used, staged content
> still ends up in the stash.
> 
>   also, is there a simple way to distinguish between the staged and
> unstaged contents of a stash (or, more basically, is this even a
> useful question to ask)? out of curiosity, i tried to figure out
> how to do this, and came up with the following.
> 
> to see staged portion of stash@{0}:
> 
>   $ git show stash@{0}^2
> 
> to see unstaged portion:
> 
>   $ git diff stash@{0}^2 stash@{0}
> 
> it's not like i have a pressing need to do that, i was just curious
> if there's a simpler way to do this, or if this is just not something
> people should need to do on a regular basis.
> 
> rday
> 
> 
> 

There was a recent thread about this[0]. The conclusion is that it's
seen as a good change, someone just needs to supply the patch to do
this. A possible solution was also provided (from before that thread)
here [1]

[0]:https://public-inbox.org/git/1505626069.9625.6.ca...@gmail.com/
[1]:https://public-inbox.org/git/20170317141417.g2oenl67k74nl...@sigill.intra.peff.net/

Re: git add -p stops working when setting color.ui = always

[Kevin Daudt]

On Mon, Oct 02, 2017 at 01:38:17PM +0300, Tsvi Mostovicz wrote:
> Hi,
> 
> When setting "color.ui = always" in the last few versions (not sure
> exactly when this started, but definitely exists in 2.14.1 and
> 2.14.2), git add -p stops working as expected. Instead of prompting
> the user, it skips through the prompts and doesn't allow selecting a
> hunk.
> 
> Don't know why I had color.ui = always set in my .gitconfig.
> 
> git version 2.14.2.666.gea220ee40
> 
> Thanks,
> 

Hello Tsvi,

This is being discussed just now[0]. Setting the value to auto should
fix it (setting it to always does not make much sense in your config
file).

Kevin.

[0}:https://public-inbox.org/git/20171003093157.gq7za2fwcqsou...@sigill.intra.peff.net/T/

Re: will git rebase has side effect

[Kevin Daudt]

On Mon, Oct 02, 2017 at 12:06:38AM +0800, Yubin Ruan wrote:
> 2017-10-01 22:17 GMT+08:00 Kevin Daudt <m...@ikke.info>:
> > Forgot to cc the mailing list.
> >
> > On Sun, Oct 01, 2017 at 09:23:23PM +0800, Yubin Ruan wrote:
> >> Suppose that I have such a history of commit locally:
> >>
> >> A --> B --> C --> D
> >>
> >> If I then add a few more commits locally
> >>
> >> A --> B --> C --> D --> E --> F --> G
> >>
> >> And then I do a rebase and squash F and G into one single commit H.
> >> What side effect will this rebase have? How will this affect "git push
> >> origin master"?
> >>
> >> Yubin
> >
> > Hello Yubin,
> >
> > So the situation is this:
> >
> > [origin/master]
> >   |
> > A --> B --> C --> D --> E --> F --> G
> > |
> >  [master]
> >
> > Then you squash (F' is the result of squashing F and G):
> >
> > [origin/master]
> >   |
> > A --> B --> C --> D --> E --> F'
> >   |
> >[master]
> >
> > When you want to push now, it's just as if you just created just two
> > commits in the first place, and you can just push normally (assuming no
> > one else has pushed in the mean time.
> 
> Hmm..You mean, if I do a squash, it will only affects those commits
> that has been squashed, not any other commits, and their parent-child
> relations remain the same?
> 
> Yubin

Only the commits being squashed, and the commits after it (not
applicable in your case). But not commits that come before the squash.
Remember that in git, commits point at their parent(s), not the opposite
way. So if you change commits, only the children will have to change (to
point to the new hashes), but not their parents.

Re: will git rebase has side effect

[Kevin Daudt]

Forgot to cc the mailing list.

On Sun, Oct 01, 2017 at 09:23:23PM +0800, Yubin Ruan wrote:
> Suppose that I have such a history of commit locally:
> 
> A --> B --> C --> D
> 
> If I then add a few more commits locally
> 
> A --> B --> C --> D --> E --> F --> G
> 
> And then I do a rebase and squash F and G into one single commit H.
> What side effect will this rebase have? How will this affect "git push
> origin master"?
> 
> Yubin

Hello Yubin,

So the situation is this:

[origin/master]
  |
A --> B --> C --> D --> E --> F --> G
|
 [master] 

Then you squash (F' is the result of squashing F and G):

[origin/master]
  |
A --> B --> C --> D --> E --> F'
  |
   [master] 

When you want to push now, it's just as if you just created just two
commits in the first place, and you can just push normally (assuming no
one else has pushed in the mean time.

The situation is different when you have pushed already:

  [origin/master]
|
A --> B --> C --> D --> E --> F --> G
|
 [master] 

Then after you squash, the history would look as follows:

  [origin/master]
|
A --> B --> C --> D --> E --> F --> G
 \
  --> F'
  |
   [master] 

This sitation would look to git like you created F', and someone else
had pushed F and G. It will reject the push, saying you would need to
merge these changes first (but you don't want this, because you are
merging the same changes together).

To solve this, you can use git push --force-with-lease=master origin,
which will force the push, overwriting the history the remote already
had.

Hope this helps, Kevin.

nb. --force-with-lease is a safer version of just (-f|--force), because
it will prevent you from overwriting history you don't have locally, for
example when someone else did push already.

Re: "man git-checkout", man page is inconsistent between SYNOPSIS and details

[Kevin Daudt]

On Sat, Sep 30, 2017 at 05:27:22AM -0400, Robert P. J. Day wrote:
> 
>   just noticed that in "man git-checkout", the SYNOPSIS contains the
> line:
> 
>   git checkout [-p|--patch] [] [--] [...]
> 
> implying that  is optional, but further down in the
> DESCRIPTION, one reads:
> 
>   git checkout [-p|--patch] [] [--] ...
> 
> suggesting that  is required.
> 

Hello Robert, thank you for this report.

Git checkout has 2 major modes of operating:

1. Checking out branches (and then update your working tree to match that
  commit.
2. Checking out 1 or more files from a commit.

The first four lines of the synopsis match mode nr. 1. The next two
belong to mode nr. 2.

The pathspec in the synopsis line you are quoting is required, because
that's how you tell git you want mode nr 2. That's why it's not
mentioned between [].  The last section under description explains that
mode. 

Do you feel this distinction needs to be made more clear?

Kevin.

Re: '--shallow-since' option is not available for git-pull in Git version 2.14.1

[Kevin Daudt]

On Mon, Sep 25, 2017 at 04:31:10PM +0900, Sanggyu Nam wrote:
> I’ve found that some subcommands such as git-clone, git-fetch, and
> git-pull support an option named ‘--shallow-since’, as of Git version
> 2.11. This option is documented in the man page of each subcommand. In
> Git 2.14.1, I’ve checked that the option is available for git-clone
> and git-fetch so that the history of a shallow repository is updated.
> However, running git-pull with this option shows an error as follows:
> 
> error: unknown option `shallow-since=2017-09-10T00:00:00+00:00'
> 
> usage: git pull [] [ [...]]
> ...
> 
> I found that this option is not available in Git 2.14.1 on macOS and
> Ubuntu 16.04.1. It seems the option handling of git-pull does not
> match with what’s described in the man page. Since ‘git pull’ is a
> shorthand for ‘git fetch’ followed by ‘git merge FETCH_HEAD’ in its
> default mode, we can run these two commands in this order as a
> workaround.
> 
> 

This does not only count for --shallow-since, but also --deepen
and --shallow-exclude.

Re: [PATCH] t2018: remove unwanted empty line

[Kevin Daudt]

On Sun, Sep 17, 2017 at 10:19:28AM +, Kaartic Sivaraam wrote:
> Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91...@gmail.com>
> ---
>  t/t2018-checkout-branch.sh | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh
> index 2131fb2a5682c..e0a52334a22dd 100755
> --- a/t/t2018-checkout-branch.sh
> +++ b/t/t2018-checkout-branch.sh
> @@ -192,7 +192,6 @@ test_expect_success 'checkout -b ' '
>  test_expect_success 'checkout -B to the current branch works' '
>   git checkout branch1 &&
>   git checkout -B branch1-scratch &&
> -
>   setup_dirty_mergeable &&
>   git checkout -B branch1-scratch initial &&
>   test_dirty_mergeable
> 
> --
> https://github.com/git/git/pull/403

Why is this empty line unwanted? This kind of whitespace can help
separate logical sections, just like paragraphs would.

Kevin.

RE: [PATCH 1/1] reset: fix reset when using the sparse-checkout feature.

[Kevin Willford]

From: Junio C Hamano [mailto:gits...@pobox.com]
Sent: Thursday, September 14, 2017 11:00 PM
> 
> Kevin Willford <kewi...@microsoft.com> writes:
> 
> > 1. Does this statement, "I only care about the files in this
> > sparse checkout, and do not concern me with anything else", mean
> > that git should not change files outside the sparse-checkout whether
> > that be in the working directory or in the index?  Or does that only
> > apply to the working directory and the index version of files can
> > change to whatever git the git command would do without using
> > sparse?  For example if I am doing a 'git reset HEAD~1'  should the
> > version in the index of files outside the sparse not be changed or
> > change to the HEAD~1 version with the skip-worktree bit on?
> 
> My understanding of the purpose of "sparse checkout" thing is that
> the user still wants to create correct whole-tree commit even the
> user does not have the whole-tree checkout.  The object names for
> blobs recorded in the index that are meant to be included in the
> next commit MUST be the same as those that would be in the index
> when the "sparse" feature is not in use.  "reset HEAD~1" should
> match the index entries to the tree entries in HEAD~1.  So, the
> latter, I would think, among your two alternatives.
> 
> IOW, after "git reset HEAD~", if you drop the skip-worktree bit from
> all index entries, "git diff --cached HEAD" must say "there is no
> changes".
> 
> The only difference between the "sparse" and normal would be that,
> because the "sparse" user does not intend to change anything outside
> the "sparse" area, these paths outside her "sparse" area would not
> materialize on the filesystem.  For the next "write-tree" out of the
> index to still write the correct tree out, the entries outside her
> "sparse" area in the index MUST match the tree of the commit she
> started working from.
> 

Makes sense.  And even though the reset might only change entries
outside the sparse area and the next status will report "nothing to commit,
working tree clean",  that's okay because the user hasn't changed
anything in their sparse area and intended to roll back the index to
whatever they specified in their reset command.

> > 2. How will this work with other git commands like merge, rebase,
> > cherry-pick, etc.?
> > 3. What about when there is a merge conflict with a file that is outside
> > the sparse checkout?
> 
> I would say, rather than forbidding such a merge, it should let her
> see and deal with the conflict by dropping the "this is outside the
> sparse area, so do not bother materializing it to the filesystem"
> bit, but tell her loudly what it did ("I checked out a half-merged
> file outside your sparse-checkout area because you'd need it while
> resolving the conflict").  By doing things that way, the user can
> decide if she wants to go ahead and complete the merge, even if the
> conflict is outside the area she is currently interested in, or
> postpone the merge and continue working on what she has been working
> on inside the narrowed-down area first.
> 
> I do not have a strong opinion whether the sparse-checkout
> configuration file should be adjusted to match when the command must
> tentatively bust the sparse checkout area; I'd imagine it can be
> argued either way.
> 
> Note that "sparse" is not my itch, and I would not be surprised if
> those who designed it may want it to work differently from my
> knee-jerk reaction in the previous two paragraphs, and I may even
> find such an alternative solution preferable.
> 
> But it is highly unlikely for any sensible solution would violate
> the basic premise, i.e. "the indexed contents will stay the same as
> the case without any 'sparse', so the next write-tree will do the
> right thing".

There was one other case that I thought about while implementing
this approach and it is when the user creates a file that is outside their
sparse definition.  From your explanation above I will attempt to
explain how I think it should work and please correct me if you see
it working differently.

The user creates a file that is outside the sparse area and it will show
up as untracked.  No problem here since the untracked are outside the
scope of using sparse.  Next the user adds the untracked file to the index.
The skip-worktree bit should be off and stay off since the user could make
additional changes and want to add them.  Once the user commits the
newly created file, I could see turning on the skip-worktree bit and
removing the file from the working tree after the c

Re: Git 2.14.1: t6500: error during test on musl libc

[Kevin Daudt]

On Fri, Sep 15, 2017 at 08:37:40AM +0200, Kevin Daudt wrote:
> On Thu, Sep 14, 2017 at 09:43:12PM -0500, A. Wilcox wrote:
> > -BEGIN PGP SIGNED MESSAGE-
> > Hash: SHA256
> > 
> > Hi there,
> > 
> > While bumping Git's version for our Linux distribution to 2.14.1, I've
> > run in to a new test failure in t6500-gc.sh.  This is the output of
> > the failing test with debug=t verbose=t:
> 
> This is a new test introduced by c45af94db 
> (gc: run pre-detach operations under lock, 2017-07-11) which was
> included in v2.14.0.
> 
> So it might be that this was already a problem for a longer time, only
> just recently uncovered.
> 

Adding Jeff King to CC

Re: Git 2.14.1: t6500: error during test on musl libc

[Kevin Daudt]

On Thu, Sep 14, 2017 at 09:43:12PM -0500, A. Wilcox wrote:
> -BEGIN PGP SIGNED MESSAGE-
> Hash: SHA256
> 
> Hi there,
> 
> While bumping Git's version for our Linux distribution to 2.14.1, I've
> run in to a new test failure in t6500-gc.sh.  This is the output of
> the failing test with debug=t verbose=t:

This is a new test introduced by c45af94db 
(gc: run pre-detach operations under lock, 2017-07-11) which was
included in v2.14.0.

So it might be that this was already a problem for a longer time, only
just recently uncovered.

> 
> 
> expecting success:
> # make sure we run a background auto-gc
> test_commit make-pack &&
> git repack &&
> test_config gc.autopacklimit 1 &&
> test_config gc.autodetach true &&
> 
> # create a ref whose loose presence we can use to detect a
> pack-refs run
> git update-ref refs/heads/should-be-loose HEAD &&
> test_path_is_file .git/refs/heads/should-be-loose &&
> 
> # now fake a concurrent gc that holds the lock; we can use our
> # shell pid so that it looks valid.
> hostname=$(hostname || echo unknown) &&
> printf "$$ %s" "$hostname" >.git/gc.pid &&
> 
> # our gc should exit zero without doing anything
> run_and_wait_for_auto_gc &&
> test_path_is_file .git/refs/heads/should-be-loose
> 
> [master 28ecdda] make-pack
>  Author: A U Thor 
>  1 file changed, 1 insertion(+)
>  create mode 100644 make-pack.t
> Counting objects: 3, done.
> Delta compression using up to 8 threads.
> Compressing objects: 100% (2/2), done.
> Writing objects: 100% (3/3), done.
> Total 3 (delta 0), reused 0 (delta 0)
> Auto packing the repository in background for optimum performance.
> See "git help gc" for manual housekeeping.
> File .git/refs/heads/should-be-loose doesn't exist.
> not ok 8 - background auto gc respects lock for all operations
> #
> #   # make sure we run a background auto-gc
> #   test_commit make-pack &&
> #   git repack &&
> #   test_config gc.autopacklimit 1 &&
> #   test_config gc.autodetach true &&
> #
> #   # create a ref whose loose presence we can use to
> detect a pack-refs run
> #   git update-ref refs/heads/should-be-loose HEAD &&
> #   test_path_is_file .git/refs/heads/should-be-loose &&
> #
> #   # now fake a concurrent gc that holds the lock; we can
> use our
> #   # shell pid so that it looks valid.
> #   hostname=$(hostname || echo unknown) &&
> #   printf "$$ %s" "$hostname" >.git/gc.pid &&
> #
> #   # our gc should exit zero without doing anything
> #   run_and_wait_for_auto_gc &&
> #   test_path_is_file .git/refs/heads/should-be-loose
> #
> 
> # failed 1 among 8 test(s)
> 1..8
> 
> 
> I admit I am mostly blind with the Git gc system.  Should I use strace
> on the git-gc process at the end?  How would I accomplish that?  Is
> there a better way of debugging this error further?
> 
> Core system stats:
> 
> Intel x86_64 E3-1280 v3 @ 3.60 GHz
> musl libc 1.1.16+20
> git 2.14.1, vanilla except for a patch to an earlier test due to
> musl's inability to cope with EUC-JP
> bash 4.3.48(1)-release
> 
> Thank you very much.
> 
> All the best,
> - --arw
> 
> - -- 
> A. Wilcox (awilfox)
> Project Lead, Adélie Linux
> http://adelielinux.org
> -BEGIN PGP SIGNATURE-
> Version: GnuPG v2
> 
> iQIcBAEBCAAGBQJZuz4yAAoJEMspy1GSK50UORwP/0Jxfp3xzexh27tSJlXYWS/g
> g9QK8Xmid+3A0R696Vb2GguKg2roCcTmM2anR7iD1B2f2W31sgf+8M5mnJRHyJ1p
> geEeqwrTdpCk6jQ/1Pj03L0NOftb1ftR6hcoVujBFAOph4jRlRdZDPA87fe6snrh
> q99C3LoDXQcyK6WWJwzX+t2wOplKgpGJP8wTAaZ0AHoUwVS5CLPl8tP2XaY4kLfD
> ZPPcvtp9wisVzzZ2ssE/CLGd38EbenNNZ6OJCBFJIHmlwey4G2isZ9kk6fVIHXi2
> unBJ8yVqI7hQKmQFSVQMMSFSd9azhHnDjTBO5mzWeRK9HNVMda3LZsXTtVeswnRs
> lN/ASMdt5KdfpNy/plFB7yDWLlQSQY7j1mxBMR8lL3AdVVQUbJppDM795tt+rn6a
> NCE2ESZMWd/QEULmT92AbkNJTj5ibBEoubnVTka05KMjaBLwIauhpqU5XxLFq2UH
> y3JYQU9hm0E7dQE0CLXxIm5/574T6bBUgp1cXH3CjxkeUYKR1USVKtDfBV6t/Qmt
> xlDZKPEfjKbTvL3KUF33G+eAp55wTwrJTaWlOp8A/JqooXavYghcsuFhYtCPJ8qo
> fFUa8kBZP70E/O7JkycUu8wi7p42+j1a8gR6/AnPG2u2wyoiosLCxHX+nll4gKmN
> b6BuiRn0Z9ie5xw4xcMR
> =Vf8Z
> -END PGP SIGNATURE-

RE: [PATCH 1/1] reset: fix reset when using the sparse-checkout feature.

[Kevin Willford]

> From: Junio C Hamano [mailto:gits...@pobox.com]
> Sent: Wednesday, September 13, 2017 4:18 PM
> 
> Jacob Keller <jacob.kel...@gmail.com> writes:
> 
> > By definition if you do a sparse checkout, you're telling git "I only
> > care about the files in this sparse checkout, and do not concern me
> > with anything else"... So the proposed fix is "since git cleared the
> > skip-worktree flag, we should actually also copy the file out again."
> > but I think the real problem is that we're clearing skip worktree to
> > begin with?
> 
> As this 100% agree with what I've been saying, I do not have
> anything to add to the discussion at the moment, so I'll stop
> speaking now but will continue to monitor the thread so that I may
> hear a new argument and datapoint that would make me change my mind.
> 
> Thanks for a healthy discussion.

Hi Junio,

Thanks for the feedback.  I will give this implementation a try.  I still
have the following unanswered questions, if you wouldn't mind
answering.

1. Does this statement, "I only care about the files in this
sparse checkout, and do not concern me with anything else", mean
that git should not change files outside the sparse-checkout whether
that be in the working directory or in the index?  Or does that only
apply to the working directory and the index version of files can
change to whatever git the git command would do without using
sparse?  For example if I am doing a 'git reset HEAD~1'  should the
version in the index of files outside the sparse not be changed or
change to the HEAD~1 version with the skip-worktree bit on?
2. How will this work with other git commands like merge, rebase,
cherry-pick, etc.?  
3. What about when there is a merge conflict with a file that is outside
the sparse checkout?  Should there not be a conflict because
git shouldn't be trying to merge the file since it is outside the sparse
checkout area?  Should the conflicted file get written outside the
sparse checkout so the user can resolve it?

Thanks,
Kevin

RE: [PATCH 1/1] reset: fix reset when using the sparse-checkout feature.

[Kevin Willford]

> From: Jacob Keller [mailto:jacob.kel...@gmail.com]
> Sent: Tuesday, September 12, 2017 7:39 PM
> 
> On Tue, Sep 12, 2017 at 4:30 PM, Kevin Willford <kewi...@microsoft.com> wrote:
> >
> > Sorry.  It was not in the sparse-checkout file.
> > $ git config core.sparsecheckout true
> > $ echo /i > .git/info/sparse-checkout
> > $ git checkout -f
> > was ran in the modified file case as well and "i" was the only file in the
> > working directory before reset.
> >
> 
> 
> I'm assuming then that you mean that some file "m" is modified by the
> commit, and do not mean to say that it has local modifications in the
> working tree? That is not what I had inferred from earlier, so I was
> very much confused.
> 

Yes

> In this example, the only file actually checked out is "i", as
> everything else will have the skip-worktree bit set..?
> 

Yes

> So doing git reset HEAD~1 will reset the branch back one commit, and
> now because of this reset is clearing the skip worktree flag, and
> since you never had a copy of it checked out it is notified as
> deleted, because it's no longer marked as skip-worktree?
> 

Correct

> 
> I think the real fix is to stop having reset clear skip-worktree, no?
> 
> By definition if you do a sparse checkout, you're telling git "I only
> care about the files in this sparse checkout, and do not concern me
> with anything else"... So the proposed fix is "since git cleared the
> skip-worktree flag, we should actually also copy the file out again."
> but I think the real problem is that we're clearing skip worktree to
> begin with?

This certainly is an option but I would have some questions with this
approach.  Does this statement, "I only care about the files in this
sparse checkout, and do not concern me with anything else", mean
that git should not change files outside the sparse-checkout whether
they are in the working directory or the index?  Or does that only
apply to the working directory and the index version of files can
change to whatever git decides?  So in the example above would
"m" be the HEAD~1 version of the file in the index or the HEAD
version before the reset?  Does this apply to all git commands,
merge, rebase, cherry-pick, etc?  What about when there is a merge
conflict with a file that is outside the sparse checkout?

Seems to me it is a lot more complex than only caring about the files
in the sparse checkout and no concern for anything else.  Personally
I would like to error on the side of letting the user decide what they
want to do, even if that means turning off the skip-worktree bit and
putting the working directory into an expected state.

Re: [PATCH] commit-template: change a message to be more intuitive

[Kevin Daudt]

On Tue, Sep 12, 2017 at 04:25:36PM +0530, Kaartic Sivaraam wrote:
> It's not possible to 'touch' the cut-line that is shown when the
> user requests a patch in his commit template.
> 

Touching something can also mean to disturb or change something, which
is the meaning being used here, so it is not an incorrect use of the
word.

> So, make the sentence more intuitive.

I can understand however that it might be not so clear for people with
less fluency in English.

> 
> Signed-off-by: Kaartic Sivaraam 
> ---
>  Just a small tweak. May or may not be worth the patch.
> 
>  wt-status.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/wt-status.c b/wt-status.c
> index 77c27c511..1f54b1df2 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -934,7 +934,7 @@ size_t wt_status_locate_end(const char *s, size_t len)
>  
>  void wt_status_add_cut_line(FILE *fp)
>  {
> - const char *explanation = _("Do not touch the line above.\nEverything 
> below will be removed.");
> + const char *explanation = _("Do not edit the line above.\nEverything 
> below will be removed.");
>   struct strbuf buf = STRBUF_INIT;
>  
>   fprintf(fp, "%c %s", comment_line_char, cut_line);
> -- 
> 2.14.1.1006.g90ad9a07c
> 

RE: [PATCH 1/1] reset: fix reset when using the sparse-checkout feature.

[Kevin Willford]

> From: Jacob Keller [mailto:jacob.kel...@gmail.com]
> Sent: Tuesday, September 12, 2017 4:29 PM
> 
> On Tue, Sep 12, 2017 at 1:20 PM, Kevin Willford <kewi...@microsoft.com> wrote:
> >
> > I think this is where I need to do a better job of explaining so here is a
> > simple example.
> >
> > I have a file "a" that was added in the latest commit.
> > $ git log --oneline
> > c1fa646 (HEAD -> reset, master) add file a
> > 40b342c Initial commit with file i
> >
> > Running the reset without using a sparse-checkout file
> >
> > $ git reset HEAD~1
> > $ git status
> > On branch reset
> > Untracked files:
> >   (use "git add ..." to include in what will be committed)
> >
> > a
> >
> > nothing added to commit but untracked files present (use "git add" to track)
> >
> > Turning on sparse-checkout and running checkout to make my working
> > directory sparse
> >
> > $ git config core.sparsecheckout true
> > $ echo /i > .git/info/sparse-checkout
> > $ git checkout -f
> >
> > Running reset gives me
> > $ git reset HEAD~1
> > $ git status
> > On branch reset
> > nothing to commit, working tree clean
> > $ git ls-files
> > i
> >
> > file a is gone.  Not in the index and not in the working directory.
> > Nothing to let the user know that anything changed.
> >
> > With a modified file no sparse-checkout
> > $ git log --oneline
> > 6fbd34a (HEAD -> reset, modified) modified file m
> > c734d72 Initial commit with file i and m
> > $ git reset HEAD~1
> > Unstaged changes after reset:
> > M   m
> > $ git status
> > On branch reset
> > Changes not staged for commit:
> >   (use "git add ..." to update what will be committed)
> >   (use "git checkout -- ..." to discard changes in working directory)
> >
> > modified:   m
> >
> > no changes added to commit (use "git add" and/or "git commit -a")
> >
> > With sparse-checkout
> > $ git reset HEAD~1
> > Unstaged changes after reset:
> > D   m
> > $ git status
> > On branch reset
> > Changes not staged for commit:
> >   (use "git add/rm ..." to update what will be committed)
> >   (use "git checkout -- ..." to discard changes in working directory)
> >
> > deleted:m
> >
> > no changes added to commit (use "git add" and/or "git commit -a")
> >
> 
> Wasn't "m" outside the sparse checkout? Or was it a file in the sparse
> checkout? I mean to say, the file after setting up sparse checkout was
> one of the "interesting" files that sparse checked out?
> 
> Or was it in fact a separate file which wasn't there?

Sorry.  It was not in the sparse-checkout file. 
$ git config core.sparsecheckout true
$ echo /i > .git/info/sparse-checkout
$ git checkout -f
was ran in the modified file case as well and "i" was the only file in the
working directory before reset. 

> 
> I would think that in sparse-checkout world, you should only *ever*
> have the files you list in sparse.
> 
> So files outside sparse world should be ignored, not shown and not
> show up in status, but they should absolutely not show up in the
> working tree either.

Sparse checkout uses the skip-worktree bit which has the following
documentation:
https://git-scm.com/docs/git-update-index 
"Skip-worktree bit can be defined in one (long) sentence: When reading
an entry, if it is marked as skip-worktree, then Git pretends its working
directory version is up to date and read the index version instead.

To elaborate, "reading" means checking for file existence, reading file
attributes or file content. The working directory version may be present
or absent. If present, its content may match against the index version or
not. Writing is not affected by this bit, content safety is still first 
priority.
Note that Git can update working directory file, that is marked skip-worktree,
if it is safe to do so (i.e. working directory version matches index version)"

I'm not saying that is the right behavior, just pointing out the documentation.
And from my experience git will turn on and off the skip-worktree flag
depending on the command as we see happening with reset.

> 
> You're not "changing" any commits, because the status of the file at
> HEAD~1 is exactly what HEAD~1 says it is, but you just don't have a
> checked out copy of it.

In the case of reset yes and it matches HEAD and if the sparse flag was on,
the user does not know that the file was chan

RE: [PATCH 1/1] reset: fix reset when using the sparse-checkout feature.

[Kevin Willford]

"reset HEAD~1" I should be able to run "add ." + "commit" and have
the same commit as before the reset.  If this is changed to only reset
the sparse entries, there will be staged changes after the reset because
HEAD has changed but we didn't update the index versions of the files.
If we do update the index with the "HEAD~1" version of the files and just
set the skip-worktree bit then the next commit will not have the original
changes outside the sparse-checkout for the commit.

> When we make a sparse checkout that places P outside the checked-out
> area, with P in the index and not in the working tree, what asks
> "git status" and friends not to treat P as "locally removed"?
> Shouldn't "reset HEAD~1" that resurrected P that was missing in the
> commit in the state before you did the "reset" be doing the same
> (e.g. flipping the bit in the index for path P that says "not having
> this in the working tree is not a removal---it is deliberately not
> checked out")?  If that is the right approach to solve the issue
> (which we established is a right problem to solve already), and the
> approach that the patch wants to take is quite the opposite of it,
> then my answer to the second question (are we solving the problem
> with the right approach?) is no.  And at that point, it is moot to
> ask if the code correctly and/or efficiently implements that wrong
> solution, isn't it?

I agree that we must determine if there is a problem, which I hope
we can agree that there is a valid problem to be addressed.  Determining
if I used the right approach depends on how you see reset working
correctly using the sparse feature.  Should it match a reset when not
using sparse or should it reset only sparse files?  I saw issues
if only the sparse files are the ones reset, which is the reason that
I went with this approach.

Thanks,
Kevin   

Re: [PATCH v2 1/2] doc/for-each-ref: consistently use '=' to between argument names and values

[Kevin Daudt]

On Tue, Sep 12, 2017 at 11:28:00AM +0900, Junio C Hamano wrote:
> Kevin Daudt <m...@ikke.info> writes:
> 
> > The synopsis and description inconsistently add a '=' between the
> > argument name and it's value. Make this consistent.
> >
> > Signed-off-by: Kevin Daudt <m...@ikke.info>
> > ---
> >  Documentation/git-for-each-ref.txt | 15 ---
> >  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> Good idea, and I think it is in line with an earlier suggestion by
> Jonathan (cc'ed).
> 
> Thanks.
> 

Yeah, this is his diff applied. Forgot to CC him.

[PATCH v2 2/2] doc/for-each-ref: explicitly specify option names

[Kevin Daudt]

For count, sort and format, only the argument names were listed under
OPTIONS, not the option names.

Add the option names to make it clear the options exist

Signed-off-by: Kevin Daudt <m...@ikke.info>
---
 Documentation/git-for-each-ref.txt | 18 +-
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt 
b/Documentation/git-for-each-ref.txt
index 1015c88f6..66b4e0a40 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -26,19 +26,25 @@ host language allowing their direct evaluation in that 
language.
 
 OPTIONS
 ---
-::
+...::
+   If one or more patterns are given, only refs are shown that
+   match against at least one pattern, either using fnmatch(3) or
+   literally, in the latter case matching completely or from the
+   beginning up to a slash.
+
+--count=::
By default the command shows all refs that match
``.  This option makes it stop after showing
that many refs.
 
-::
+--sort=::
A field name to sort on.  Prefix `-` to sort in
descending order of the value.  When unspecified,
`refname` is used.  You may use the --sort= option
multiple times, in which case the last key becomes the primary
key.
 
-::
+--format=::
A string that interpolates `%(fieldname)` from a ref being shown
and the object it points at.  If `fieldname`
is prefixed with an asterisk (`*`) and the ref points
@@ -51,12 +57,6 @@ OPTIONS
`xx`; for example `%00` interpolates to `\0` (NUL),
`%09` to `\t` (TAB) and `%0a` to `\n` (LF).
 
-...::
-   If one or more patterns are given, only refs are shown that
-   match against at least one pattern, either using fnmatch(3) or
-   literally, in the latter case matching completely or from the
-   beginning up to a slash.
-
 --shell::
 --perl::
 --python::
-- 
2.14.1.459.g238e487ea9

[PATCH v2 1/2] doc/for-each-ref: consistently use '=' to between argument names and values

[Kevin Daudt]

The synopsis and description inconsistently add a '=' between the
argument name and it's value. Make this consistent.

Signed-off-by: Kevin Daudt <m...@ikke.info>
---
 Documentation/git-for-each-ref.txt | 15 ---
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt 
b/Documentation/git-for-each-ref.txt
index bb370c9c7..1015c88f6 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -10,8 +10,9 @@ SYNOPSIS
 [verse]
 'git for-each-ref' [--count=] [--shell|--perl|--python|--tcl]
   [(--sort=)...] [--format=] [...]
-  [--points-at ] [(--merged | --no-merged) []]
-  [--contains []] [--no-contains []]
+  [--points-at=]
+  (--merged[=] | --no-merged[=])
+  [--contains[=]] [--no-contains[=]]
 
 DESCRIPTION
 ---
@@ -65,24 +66,24 @@ OPTIONS
the specified host language.  This is meant to produce
a scriptlet that can directly be `eval`ed.
 
---points-at ::
+--points-at=::
Only list refs which points at the given object.
 
---merged []::
+--merged[=]::
Only list refs whose tips are reachable from the
specified commit (HEAD if not specified),
incompatible with `--no-merged`.
 
---no-merged []::
+--no-merged[=]::
Only list refs whose tips are not reachable from the
specified commit (HEAD if not specified),
incompatible with `--merged`.
 
---contains []::
+--contains[=]::
Only list refs which contain the specified commit (HEAD if not
specified).
 
---no-contains []::
+--no-contains[=]::
Only list refs which don't contain the specified commit (HEAD
if not specified).
 
-- 
2.14.1.459.g238e487ea9

RE: What's cooking in git.git (Sep 2017, #02; Mon, 11)

[Kevin Willford]

> 
> * kw/write-index-reduce-alloc (2017-09-08) 2 commits
>  - read-cache: fix index corruption with index v4
>  - Add t/helper/test-write-cache to .gitignore
> 
>  Expecting a reroll.
>  cf. <CALgYhfNYgmCJqptNQLKaQpCs9mAgqZHUrDS3BVEqCv_f+WX-
> q...@mail.gmail.com>
> 
I didn't submit these patches so what would you like
me to do?

The reroll for read-cache fix was submitted here
https://public-inbox.org/git/20170907192412.8085-1-t.gumme...@gmail.com/

- Kevin

RE: [PATCH 1/1] reset: fix reset when using the sparse-checkout feature.

[Kevin Willford]

> From: Junio C Hamano [mailto:gits...@pobox.com]
> Sent: Friday, September 8, 2017 9:18 PM
> 
> Kevin Willford <kewi...@microsoft.com> writes:
> 
> > 1. reset mixed when there were files that were added
> >
> > In this case the index will no longer have the entry at all because
> > the reset is making the index look like before the file was added
> > which didn't have it. When not using the sparse-checkout this is fine
> > because the file is in the working directory and the reset didn't touch
> > it.  But using the sparse-checkout on that file and if the file was not
> > in the working directory, the index gets reset and the entry for that
> > file is gone and if we don't put the index version of the file before the
> > reset into the working directory, then we have lost the content for
> > that file
> 
> I do not quite understand this argument.  If you do
> 
>   edit $path
>   git add $path
>   rm $path
>   git reset
> 
> for a $path that is not involved in the sparse thing, the version
> that was previously indexed will be lost, but that is fine---the
> user said that version is expendable by saying "reset".
> 
> How would that be different when the $path were not to be
> materialized in the working tree due to sparseness?  Where did that
> "blob" object in the index immediately before you called "reset"
> came from, and why do you say that the user does *not* consider that
> one expendable, unlike the case for non-sparse path example above?
> 

I guess that I should have said files that were newly added, meaning they
are new files that were created and added in the previous commit.
 
I think that the difference is that the user explicitly removed the file. When
using sparse it is git that is causing the removal of the file.  For example
if I have /file in my spare-checkout file so that I am only working on the one
file.  The previous commit had new file2 added and I run a git reset HEAD~1.
I as the user do not expect that file2 just disappear, but yet that is what
happens.  So from you example above if I do.

create $path
git add $path
git commit
git checkout // where $path is not in the sparse-checkout
git reset HEAD~1

$path will be gone yet I the user did not remove it.  I guess you could
argue that the user did when they specified their sparse-checkout and
ran checkout but I wouldn't know what would go missing unless I ran
a diff before the reset to see.  There could have been X number of
files created and added in the previous commit(s) and status after the
reset would not report them and they are gone.  So I could clone,
setup sparse-checkout, checkout, reset HEAD~X and possibly lose
data I didn't expect to.

In the modified case where the previous commits have modifications
to files outside the sparse-checkout at least the status after the reset reports
the file as deleted so the user sees that something has happened to it.

I suppose the entry could stay in the index with the skip-worktree bit
on and not removed like it is now so that a git reset will only apply
to the entries in the sparse-checkout?  That seems like it would be
changing the meaning of reset.

> I suspect that a similar reasoning would apply to your 2., but I
> didn't think it through.
> 
> The possible misconception, which I perceive in both of these, is
> that you are somehow disagreeing with this basic assumption: by
> saying "git reset []", the user is telling us that the
> version in the index, even if that is different from HEAD,
> , or the file in the working tree, is *unwanted* and be
> replaced with the one in HEAD (or  when given).  Touching
> the working tree files upon "git reset" is the last thing the user
> expects to happen.
> 

I agree with this when you are not dealing with a sparse-checkout.
When using a sparse-checkout I expect git not to touch things
outside of what I have specified in my sparse-checkout file.  If it
does, it should let me know or put my working directory in a
state that is expected.  Especially when it is changing the
skip-worktree bits causing files outside the sparse-checkout to be
reported incorrectly by status.

 

RE: [PATCH 1/1] reset: fix reset when using the sparse-checkout feature.

[Kevin Willford]

From: Junio C Hamano [mailto:gits...@pobox.com]
Sent: Friday, September 8, 2017 1:02 PM
> Kevin Willford <kewi...@microsoft.com> writes:
> 
> > diff --git a/builtin/reset.c b/builtin/reset.c
> > index d72c7d1c96..1b8bb45989 100644
> > --- a/builtin/reset.c
> > +++ b/builtin/reset.c
> > @@ -24,6 +24,7 @@
> >  #include "cache-tree.h"
> >  #include "submodule.h"
> >  #include "submodule-config.h"
> > +#include "dir.h"
> >
> >  static const char * const git_reset_usage[] = {
> > N_("git reset [--mixed | --soft | --hard | --merge | --keep] [-q]
> []"),
> > @@ -124,8 +125,32 @@ static void update_index_from_diff(struct
> diff_queue_struct *q,
> >
> > for (i = 0; i < q->nr; i++) {
> > struct diff_filespec *one = q->queue[i]->one;
> > +   struct diff_filespec *two = q->queue[i]->two;
> > +   int pos;
> > int is_missing = !(one->mode && !is_null_oid(>oid));
> > +   int was_missing = !two->mode && is_null_oid(>oid);
> > struct cache_entry *ce;
> > +   struct cache_entry *ceBefore;
> > +   struct checkout state = CHECKOUT_INIT;
> 
> The five new variables are only used in the new block, so it
> probably is better to limit their scope to the "we do something
> unusual when sparse checkout is in effect" block as well.  The scope
> for the latter two can further be narrowed down to "we do need to
> force a checkout of this entry" block.
> 
> We prefer to name our variables with underscore (e.g. ce_before)
> over camelCase (e.g. ceBefore) unless there is a compelling reason
> (e.g. a platform specific code in compat/ layer to match platform
> convention).
> 

Will update.

> > +
> > +   if (core_apply_sparse_checkout && !file_exists(two->path)) {
> > +   pos = cache_name_pos(two->path, strlen(two->path));
> > +   if ((pos >= 0 && ce_skip_worktree(active_cache[pos]))
> &&
> > +   (is_missing || !was_missing))
> > +   {
> > +   state.force = 1;
> > +   state.refresh_cache = 1;
> > +   state.istate = _index;
> > +   ceBefore = make_cache_entry(two->mode,
> > +   two->oid.hash,
> > +   two->path, 0, 0);
> > +   if (!ceBefore)
> > +   die(_("make_cache_entry failed for path
> '%s'"),
> > +   two->path);
> > +
> > +   checkout_entry(ceBefore, , NULL);
> > +   }
> > +   }
> 
> Can we tell between the case where the reason why the path was not
> there in the working tree was due to the path being excluded by the
> sparse checkout and the path being removed manually by the end user?
> 
> I guess ce_skip_worktree() check is sufficient; we force checkout only
> when the path is marked to be skipped due to "sparse" thing.
> 
> Do we have to worry about the reverse case, in which file_exists(two->path)
> is true (i.e. the user created a file there manually) even though
> the path is marked to be skipped due to "sparse" setting?
> 

I don't believe so because if the user has a file there whether they modified
it or not, it is what the user did and we just leave it there and a diff with
what the index gets reset to will show how the file is different from what the
index got reset to.

> Other than that, the patch looks quite cleanly done and well explained.
> 
> Thanks.
> 

RE: [PATCH 1/1] reset: fix reset when using the sparse-checkout feature.

[Kevin Willford]

From: Junio C Hamano [mailto:gits...@pobox.com]
Sent: Friday, September 8, 2017 1:12 PM
> Kevin Willford <kewi...@microsoft.com> writes:
> 
> > When using the sparse checkout feature the git reset command will add
> > entries to the index that will have the skip-worktree bit off but will
> > leave the working directory empty.  File data is lost because the index
> > version of the files have been changed but there is nothing that is in the
> > working directory.  This will cause the next status call to show either
> > deleted for files modified or deleting or nothing for files added.
> 
> Getting rid of sparseness may of course be one way to correct the
> discrepancy, but it is unclear to me if that is the best way to do
> so.  An alternative may be to add entries to the index that does
> have the bit on for paths that are meant to be skipped due to
> "sparse" thing, no?  Am I being naive, missing some reason why that
> would give us a worse result?

There are two cases that this is trying to solve.

1. reset mixed when there were files that were added

In this case the index will no longer have the entry at all because
the reset is making the index look like before the file was added
which didn't have it. When not using the sparse-checkout this is fine
because the file is in the working directory and the reset didn't touch
it.  But using the sparse-checkout on that file and if the file was not
in the working directory, the index gets reset and the entry for that
file is gone and if we don't put the index version of the file before the
reset into the working directory, then we have lost the content for
that file

2. reset mixed when there were files modified

This case is similar but with modified files there is an entry in the index
but it is getting changed to a previous version of the file.  If we don't get
the file on disk then the version of the file that, in the non sparse-checkout
case, would be on disk is lost and cannot be recovered.  So even if we turn
on the skip-worktree bit for this entry and it doesn't show up in status
calls, we lost the previous version of the file.

[PATCH 0/1] reset: fix mixed reset when using sparse-checkout

[Kevin Willford]

Original discussion is here
https://public-inbox.org/git/20170407192357.948-4-kewi...@microsoft.com/

When running a reset mixed and using the sparse-checkout the working
directory needs to be updated so that there is not data loss when the
index is updated.  This is because the index is getting updated
potentially removing entries without changing the working directory.
When using the sparse-checkout feature the entries removed might not
be on disk and are lost.

This patch writes the before version of the file to disk if the mixed
reset is going to change the index of a file that had the skip-wortree
bit so that the file contents before the reset is preserved on disk and
status will reports the correct results.

Kevin Willford (1):
  reset: fix reset when using the sparse-checkout feature.

 builtin/reset.c  | 25 +
 t/t7114-reset-sparse-checkout.sh | 60 
 2 files changed, 85 insertions(+)
 create mode 100755 t/t7114-reset-sparse-checkout.sh

-- 
2.14.1.474.g0558484247.dirty

[PATCH 1/1] reset: fix reset when using the sparse-checkout feature.

[Kevin Willford]

When using the sparse checkout feature the git reset command will add
entries to the index that will have the skip-worktree bit off but will
leave the working directory empty.  File data is lost because the index
version of the files have been changed but there is nothing that is in the
working directory.  This will cause the next status call to show either
deleted for files modified or deleting or nothing for files added.  The
added files should be shown as untracked and modified files should be
shown as modified.

To fix this when the reset is running if there is not a file in the working
directory and if it will be missing with the new index entry or was not
missing in the previous version, we create the previous index version of
the file in the working directory so that status will report correctly
and the files will be availble for the user to deal with.

Signed-off-by: Kevin Willford <kewi...@microsoft.com>
---
 builtin/reset.c  | 25 +
 t/t7114-reset-sparse-checkout.sh | 60 
 2 files changed, 85 insertions(+)
 create mode 100755 t/t7114-reset-sparse-checkout.sh

diff --git a/builtin/reset.c b/builtin/reset.c
index d72c7d1c96..1b8bb45989 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -24,6 +24,7 @@
 #include "cache-tree.h"
 #include "submodule.h"
 #include "submodule-config.h"
+#include "dir.h"
 
 static const char * const git_reset_usage[] = {
N_("git reset [--mixed | --soft | --hard | --merge | --keep] [-q] 
[]"),
@@ -124,8 +125,32 @@ static void update_index_from_diff(struct 
diff_queue_struct *q,
 
for (i = 0; i < q->nr; i++) {
struct diff_filespec *one = q->queue[i]->one;
+   struct diff_filespec *two = q->queue[i]->two;
+   int pos;
int is_missing = !(one->mode && !is_null_oid(>oid));
+   int was_missing = !two->mode && is_null_oid(>oid);
struct cache_entry *ce;
+   struct cache_entry *ceBefore;
+   struct checkout state = CHECKOUT_INIT;
+
+   if (core_apply_sparse_checkout && !file_exists(two->path)) {
+   pos = cache_name_pos(two->path, strlen(two->path));
+   if ((pos >= 0 && ce_skip_worktree(active_cache[pos])) &&
+   (is_missing || !was_missing))
+   {
+   state.force = 1;
+   state.refresh_cache = 1;
+   state.istate = _index;
+   ceBefore = make_cache_entry(two->mode,
+   two->oid.hash,
+   two->path, 0, 0);
+   if (!ceBefore)
+   die(_("make_cache_entry failed for path 
'%s'"),
+   two->path);
+
+   checkout_entry(ceBefore, , NULL);
+   }
+   }
 
if (is_missing && !intent_to_add) {
remove_file_from_cache(one->path);
diff --git a/t/t7114-reset-sparse-checkout.sh b/t/t7114-reset-sparse-checkout.sh
new file mode 100755
index 00..f2a5426847
--- /dev/null
+++ b/t/t7114-reset-sparse-checkout.sh
@@ -0,0 +1,60 @@
+#!/bin/sh
+
+test_description='reset when using a sparse-checkout'
+
+. ./test-lib.sh
+
+# reset using a sparse-checkout file
+
+test_expect_success 'setup' '
+   test_tick &&
+   echo "checkout file" >c &&
+   echo "modify file" >m &&
+   echo "delete file" >d &&
+   git add . &&
+   git commit -m "initial commit" &&
+   echo "added file" >a &&
+   echo "modification of a file" >m &&
+   git rm d &&
+   git add . &&
+   git commit -m "second commit" &&
+   git checkout -b endCommit
+'
+
+test_expect_success 'reset when there is a sparse-checkout' '
+   echo "/c" >.git/info/sparse-checkout &&
+   test_config core.sparsecheckout true &&
+   git checkout -b resetBranch &&
+   test_path_is_missing m &&
+   test_path_is_missing a &&
+   test_path_is_missing d &&
+   git reset HEAD~1 &&
+   test "checkout file" = "$(cat c)" &&
+   test "modification of a file" = "$(cat m)" &&
+   test "added file" = "$(cat a)" &&
+   test_path_is_missing d
+'
+
+test_expect_success 'reset after 

[PATCH v3 3/3] merge-recursive: change current file dir string_lists to hashmap

[Kevin Willford]

The code was using two string_lists, one for the directories and
one for the files.  The code never checks the lists independently
so we should be able to only use one list.  The string_list also
is a O(log n) for lookup and insertion.  Switching this to use a
hashmap will give O(1) which will save some time when there are
millions of paths that will be checked.

Signed-off-by: Kevin Willford <kewi...@microsoft.com>
---
 merge-recursive.c | 56 ---
 merge-recursive.h |  3 +--
 2 files changed, 46 insertions(+), 13 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index d47f40ea81..35af3761ba 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -24,6 +24,31 @@
 #include "dir.h"
 #include "submodule.h"
 
+struct path_hashmap_entry {
+   struct hashmap_entry e;
+   char path[FLEX_ARRAY];
+};
+
+static int path_hashmap_cmp(const void *cmp_data,
+   const void *entry,
+   const void *entry_or_key,
+   const void *keydata)
+{
+   const struct path_hashmap_entry *a = entry;
+   const struct path_hashmap_entry *b = entry_or_key;
+   const char *key = keydata;
+
+   if (ignore_case)
+   return strcasecmp(a->path, key ? key : b->path);
+   else
+   return strcmp(a->path, key ? key : b->path);
+}
+
+static unsigned int path_hash(const char *path)
+{
+   return ignore_case ? strihash(path) : strhash(path);
+}
+
 static void flush_output(struct merge_options *o)
 {
if (o->buffer_output < 2 && o->obuf.len) {
@@ -314,15 +339,15 @@ static int save_files_dirs(const unsigned char *sha1,
struct strbuf *base, const char *path,
unsigned int mode, int stage, void *context)
 {
+   struct path_hashmap_entry *entry;
int baselen = base->len;
struct merge_options *o = context;
 
strbuf_addstr(base, path);
 
-   if (S_ISDIR(mode))
-   string_list_insert(>current_directory_set, base->buf);
-   else
-   string_list_insert(>current_file_set, base->buf);
+   FLEX_ALLOC_MEM(entry, path, base->buf, base->len);
+   hashmap_entry_init(entry, path_hash(entry->path));
+   hashmap_add(>current_file_dir_set, entry);
 
strbuf_setlen(base, baselen);
return (S_ISDIR(mode) ? READ_TREE_RECURSIVE : 0);
@@ -642,6 +667,7 @@ static void add_flattened_path(struct strbuf *out, const 
char *s)
 
 static char *unique_path(struct merge_options *o, const char *path, const char 
*branch)
 {
+   struct path_hashmap_entry *entry;
struct strbuf newpath = STRBUF_INIT;
int suffix = 0;
size_t base_len;
@@ -650,14 +676,16 @@ static char *unique_path(struct merge_options *o, const 
char *path, const char *
add_flattened_path(, branch);
 
base_len = newpath.len;
-   while (string_list_has_string(>current_file_set, newpath.buf) ||
-  string_list_has_string(>current_directory_set, newpath.buf) ||
+   while (hashmap_get_from_hash(>current_file_dir_set,
+path_hash(newpath.buf), newpath.buf) ||
   (!o->call_depth && file_exists(newpath.buf))) {
strbuf_setlen(, base_len);
strbuf_addf(, "_%d", suffix++);
}
 
-   string_list_insert(>current_file_set, newpath.buf);
+   FLEX_ALLOC_MEM(entry, path, newpath.buf, newpath.len);
+   hashmap_entry_init(entry, path_hash(entry->path));
+   hashmap_add(>current_file_dir_set, entry);
return strbuf_detach(, NULL);
 }
 
@@ -1941,8 +1969,14 @@ int merge_trees(struct merge_options *o,
if (unmerged_cache()) {
struct string_list *entries, *re_head, *re_merge;
int i;
-   string_list_clear(>current_file_set, 1);
-   string_list_clear(>current_directory_set, 1);
+   /*
+* Only need the hashmap while processing entries, so
+* initialize it here and free it when we are done running
+* through the entries. Keeping it in the merge_options as
+* opposed to decaring a local hashmap is for convenience
+* so that we don't have to pass it to around.
+*/
+   hashmap_init(>current_file_dir_set, path_hashmap_cmp, NULL, 
512);
get_files_dirs(o, head);
get_files_dirs(o, merge);
 
@@ -1978,6 +2012,8 @@ int merge_trees(struct merge_options *o,
string_list_clear(re_head, 0);
string_list_clear(entries, 1);
 
+   hashmap_free(>current_file_dir_set, 1);
+
free(re_merge);
free(re_head);
free(entries);
@@ -2179,8 +2215,6 @@ void init_mer

[PATCH v3 2/3] merge-recursive: remove return value from get_files_dirs

[Kevin Willford]

The return value of the get_files_dirs call is never being used.
Looking at the history of the file and it was originally only
being used for debug output statements.  Also when
read_tree_recursive return value is non zero it is changed to
zero.  This leads me to believe that it doesn't matter if
read_tree_recursive gets an error.

Since the debug output has been removed and the caller isn't
checking the return value there is no reason to keep calculating
and returning a value.

Signed-off-by: Kevin Willford <kewi...@microsoft.com>
---
 merge-recursive.c | 8 ++--
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 033d7cd406..d47f40ea81 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -328,15 +328,11 @@ static int save_files_dirs(const unsigned char *sha1,
return (S_ISDIR(mode) ? READ_TREE_RECURSIVE : 0);
 }
 
-static int get_files_dirs(struct merge_options *o, struct tree *tree)
+static void get_files_dirs(struct merge_options *o, struct tree *tree)
 {
-   int n;
struct pathspec match_all;
memset(_all, 0, sizeof(match_all));
-   if (read_tree_recursive(tree, "", 0, 0, _all, save_files_dirs, o))
-   return 0;
-   n = o->current_file_set.nr + o->current_directory_set.nr;
-   return n;
+   read_tree_recursive(tree, "", 0, 0, _all, save_files_dirs, o);
 }
 
 /*
-- 
2.14.1.329.gcdd497e120.dirty

[PATCH v3 0/3] merge-recursive: replace string_list with hashmap

[Kevin Willford]

Code was using two string_lists, one for the directories and
one for the files.  The code never checks the lists independently
so we should be able to only use one list.  The string_list also
is a O(log n) for lookup and insertion.  Switching this to use a
hashmap will give O(1) which will save some time when there are
millions of paths that will be checked.

Also cleaned up a memory leak and method where the return was not
being used.

Changes since last version:
1. Removed the function pointers and just check the ignore_case in the
compare and hash methods.
2. Added a comment about the hashmap and why it is getting initialized
and freed but not a local.
3. Use hashmap_get_from_hash and remove the dummy entry

Kevin Willford (3):
  merge-recursive: fix memory leak
  merge-recursive: remove return value from get_files_dirs
  merge-recursive: change current file dir string_lists to hashmap

 merge-recursive.c | 76 ---
 merge-recursive.h |  3 +--
 2 files changed, 57 insertions(+), 22 deletions(-)

-- 
2.14.1.329.gcdd497e120.dirty

[PATCH v3 1/3] merge-recursive: fix memory leak

[Kevin Willford]

In merge_trees if process_renames or process_entry returns less
than zero, the method will just return and not free re_merge,
re_head, or entries.

This change cleans up the allocated variables before returning
to the caller.

Signed-off-by: Kevin Willford <kewi...@microsoft.com>
---
 merge-recursive.c | 12 +---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 1494ffdb82..033d7cd406 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1956,7 +1956,7 @@ int merge_trees(struct merge_options *o,
re_merge = get_renames(o, merge, common, head, merge, entries);
clean = process_renames(o, re_head, re_merge);
if (clean < 0)
-   return clean;
+   goto cleanup;
for (i = entries->nr-1; 0 <= i; i--) {
const char *path = entries->items[i].string;
struct stage_data *e = entries->items[i].util;
@@ -1964,8 +1964,10 @@ int merge_trees(struct merge_options *o,
int ret = process_entry(o, path, e);
if (!ret)
clean = 0;
-   else if (ret < 0)
-   return ret;
+   else if (ret < 0) {
+   clean = ret;
+   goto cleanup;
+   }
}
}
for (i = 0; i < entries->nr; i++) {
@@ -1975,6 +1977,7 @@ int merge_trees(struct merge_options *o,
entries->items[i].string);
}
 
+cleanup:
string_list_clear(re_merge, 0);
string_list_clear(re_head, 0);
string_list_clear(entries, 1);
@@ -1982,6 +1985,9 @@ int merge_trees(struct merge_options *o,
free(re_merge);
free(re_head);
free(entries);
+
+   if (clean < 0)
+   return clean;
}
else
clean = 1;
-- 
2.14.1.329.gcdd497e120.dirty

[PATCH v2] merge-recursive: change current file dir string_lists to hashmap

[Kevin Willford]

The code was using two string_lists, one for the directories and
one for the files.  The code never checks the lists independently
so we should be able to only use one list.  The string_list also
is a O(log n) for lookup and insertion.  Switching this to use a
hashmap will give O(1) which will save some time when there are
millions of paths that will be checked.

Also cleaned up a memory leak and method where the return was not
being used.

Signed-off-by: Kevin Willford <kewi...@microsoft.com>
---
 merge-recursive.c | 76 ---
 merge-recursive.h |  3 +--
 2 files changed, 57 insertions(+), 22 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 1494ffdb82..ebfe01017f 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -24,6 +24,31 @@
 #include "dir.h"
 #include "submodule.h"
 
+struct path_hashmap_entry {
+   struct hashmap_entry;
+   char path[FLEX_ARRAY];
+};
+
+static int path_hashmap_cmp(const void *cmp_data,
+   const void *entry,
+   const void *entry_or_key,
+   const void *keydata)
+{
+   const struct path_hashmap_entry *a = entry;
+   const struct path_hashmap_entry *b = entry_or_key;
+   const char *key = keydata;
+
+   if (ignore_case)
+   return strcasecmp(a->path, key ? key : b->path);
+   else
+   return strcmp(a->path, key ? key : b->path);
+}
+
+static unsigned int path_hash(const char *path)
+{
+   return ignore_case ? strihash(path) : strhash(path);
+}
+
 static void flush_output(struct merge_options *o)
 {
if (o->buffer_output < 2 && o->obuf.len) {
@@ -314,29 +339,25 @@ static int save_files_dirs(const unsigned char *sha1,
struct strbuf *base, const char *path,
unsigned int mode, int stage, void *context)
 {
+   struct path_hashmap_entry *entry;
int baselen = base->len;
struct merge_options *o = context;
 
strbuf_addstr(base, path);
 
-   if (S_ISDIR(mode))
-   string_list_insert(>current_directory_set, base->buf);
-   else
-   string_list_insert(>current_file_set, base->buf);
+   FLEX_ALLOC_MEM(entry, path, base->buf, base->len);
+   hashmap_entry_init(entry, path_hash(entry->path));
+   hashmap_add(>current_file_dir_set, entry);
 
strbuf_setlen(base, baselen);
return (S_ISDIR(mode) ? READ_TREE_RECURSIVE : 0);
 }
 
-static int get_files_dirs(struct merge_options *o, struct tree *tree)
+static void get_files_dirs(struct merge_options *o, struct tree *tree)
 {
-   int n;
struct pathspec match_all;
memset(_all, 0, sizeof(match_all));
-   if (read_tree_recursive(tree, "", 0, 0, _all, save_files_dirs, o))
-   return 0;
-   n = o->current_file_set.nr + o->current_directory_set.nr;
-   return n;
+   read_tree_recursive(tree, "", 0, 0, _all, save_files_dirs, o);
 }
 
 /*
@@ -646,6 +667,7 @@ static void add_flattened_path(struct strbuf *out, const 
char *s)
 
 static char *unique_path(struct merge_options *o, const char *path, const char 
*branch)
 {
+   struct path_hashmap_entry *entry;
struct strbuf newpath = STRBUF_INIT;
int suffix = 0;
size_t base_len;
@@ -654,14 +676,16 @@ static char *unique_path(struct merge_options *o, const 
char *path, const char *
add_flattened_path(, branch);
 
base_len = newpath.len;
-   while (string_list_has_string(>current_file_set, newpath.buf) ||
-  string_list_has_string(>current_directory_set, newpath.buf) ||
+   while (hashmap_get_from_hash(>current_file_dir_set,
+path_hash(newpath.buf), newpath.buf) ||
   (!o->call_depth && file_exists(newpath.buf))) {
strbuf_setlen(, base_len);
strbuf_addf(, "_%d", suffix++);
}
 
-   string_list_insert(>current_file_set, newpath.buf);
+   FLEX_ALLOC_MEM(entry, path, newpath.buf, newpath.len);
+   hashmap_entry_init(entry, path_hash(entry->path));
+   hashmap_add(>current_file_dir_set, entry);
return strbuf_detach(, NULL);
 }
 
@@ -1945,8 +1969,14 @@ int merge_trees(struct merge_options *o,
if (unmerged_cache()) {
struct string_list *entries, *re_head, *re_merge;
int i;
-   string_list_clear(>current_file_set, 1);
-   string_list_clear(>current_directory_set, 1);
+   /*
+* Only need the hashmap while processing entries, so
+* initialize it here and free it when we are done running
+* through the entries. Keeping it in the merge_options as
+* opposed to decaring a local hashmap

RE: [PATCH] read-cache: fix index corruption with index v4

[Kevin Willford]

> From: Thomas Gummerer [mailto:t.gumme...@gmail.com]
> Sent: Monday, September 4, 2017 4:58 PM
> 
> ce012deb98 ("read-cache: avoid allocating every ondisk entry when
> writing", 2017-08-21) changed the way cache entries are written to the
> index file.  While previously it wrote the name to an struct that was
> allocated using xcalloc(), it now uses ce_write() directly.  Previously
> ce_namelen - common bytes were written to the cache entry, which would
> automatically make it nul terminated, as it was allocated using calloc.
> 
> Now we are writing ce_namelen - common + 1 bytes directly from the
> ce->name to the index.  As ce->name is however not nul terminated, and
> index-v4 needs the nul terminator to split between one index entry and
> the next, this would end up in a corrupted index.
> 
> Fix that by only writing ce_namelen - common bytes directly from
> ce->name to the index, and adding the nul terminator in an extra call to
> ce_write.
> 
> This bug was turned up by setting TEST_GIT_INDEX_VERSION = 4 in
> config.mak and running the test suite (t1700 specifically broke).
> 
> Signed-off-by: Thomas Gummerer <t.gumme...@gmail.com>
> ---
> 
> I unfortunately didn't have more time to dig so
> 
> > As ce->name is however not nul terminated
> 
> just comes from my memory and from the patch below actually fixing the
> corruption, so it's really the most likely cause.  Would be great if
> someone who can remember more about the index could confirm that this
> is indeed the case.
> 

Digging into this and ce->name IS nul terminated.  The issue comes in when
the CE_STRIP_NAME is set, which is only set when using a split index. 
This sets the ce->ce_namelen = 0 without changing the actual ce->name buffer.
When writing the entry for the split index version 4 it was using the first 
character
in the ce->name buffer because of the + 1, which obviously isn't correct.
Before
it was using a newly allocated name buffer from the ondisk struct which was
allocated based on the ce_namelen of zero.

>  read-cache.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/read-cache.c b/read-cache.c
> index 40da87ea71..80830ddcfc 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -2103,7 +2103,9 @@ static int ce_write_entry(git_SHA_CTX *c, int fd, struct
> cache_entry *ce,
>   if (!result)
>   result = ce_write(c, fd, to_remove_vi, prefix_size);
>   if (!result)
> - result = ce_write(c, fd, ce->name + common,
> ce_namelen(ce) - common + 1);
> + result = ce_write(c, fd, ce->name + common,
> ce_namelen(ce) - common);
> + if (!result)
> + result = ce_write(c, fd, "\0", 1);

You could use the padding variable here as well which is used in the < version 4
ce_write.

> 
>   strbuf_splice(previous_name, common, to_remove,
> ce->name + common, ce_namelen(ce) - common);
> --
> 2.14.1.480.gb18f417b89

While looking at the code I was wondering if we could get around the
whole setting ce->ce_namelen to zero bit but that would be much bigger
patch and possibly introduce other bugs so this seems the appropriate
fix for now.

Thanks for finding this!
Kevin