[PATCH] strbuf_read_file(): preserve errno across close() call

[Jeff King]

On Fri, Feb 23, 2018 at 01:49:52AM -0500, Jeff King wrote:

> > +static ssize_t strbuf_read_file_or_whine(struct strbuf *sb, const char 
> > *path)
> > +{
> > +   int fd;
> > +   ssize_t len;
> > +
> > +   fd = open(path, O_RDONLY);
> > +   if (fd < 0)
> > +   return error_errno(_("could not open '%s'"), path);
> > +   len = strbuf_read(sb, fd, 0);
> > +   close(fd);
> > +   if (len < 0)
> > +   return error(_("could not read '%s'."), path);
> > +   return len;
> > +}
> 
> If we were to use error_errno() in the second conditional here, we
> should take care not to clobber errno during the close(). I think
> strbuf_read_file() actually has the same problem, which might be worth
> fixing.

Here's a patch, while I'm thinking about it.

I notice that quite a few strbuf error paths may call strbuf_release(),
too.  Technically free() may clobber errno, too. I don't know if it's
worth protecting against (IIRC POSIX is being amended to disallow this,
but I have no idea how common it is in existing platforms).

-- >8 --
Subject: [PATCH] strbuf_read_file(): preserve errno across close() call

If we encounter a read error, the user may want to report it
by looking at errno. However, our close() call may clobber
errno, leading to confusing results. Let's save and restore
it in the error case.

Signed-off-by: Jeff King 
---
 strbuf.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/strbuf.c b/strbuf.c
index 1df674e919..5f138ed3c8 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -612,14 +612,18 @@ ssize_t strbuf_read_file(struct strbuf *sb, const char 
*path, size_t hint)
 {
int fd;
ssize_t len;
+   int saved_errno;
 
fd = open(path, O_RDONLY);
if (fd < 0)
return -1;
len = strbuf_read(sb, fd, hint);
+   saved_errno = errno;
close(fd);
-   if (len < 0)
+   if (len < 0) {
+   errno = saved_errno;
return -1;
+   }
 
return len;
 }
-- 
2.16.2.580.g96c83ce8ea

Re: [PATCH] sequencer: factor out strbuf_read_file_or_whine()

[Jeff King]

On Thu, Feb 22, 2018 at 08:29:25PM +0100, René Scharfe wrote:

> Reduce code duplication by factoring out a function that reads an entire
> file into a strbuf, or reports errors on stderr if something goes wrong.
> 
> Signed-off-by: Rene Scharfe 
> ---
> The difference to using strbuf_read_file() is more detailed error
> messages for open(2) failures.  But I don't know if we need them -- or
> under which circumstances reading todo files could fail anyway.  When
> doing multiple rebases in parallel perhaps?

I'm fine with this patch, but FWIW I think reporting the result of
strbuf_read_file with error_errno() would actually be an improvement.
The errno values are generally sufficient to tell if the problem was in
opening or reading, and then we'd get more information in the case of a
failed read() call.

Thought note...

> diff --git a/sequencer.c b/sequencer.c
> index e9baaf59bd..e34334f0ef 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -1869,22 +1869,31 @@ static int count_commands(struct todo_list *todo_list)
>   return count;
>  }
>  
> +static ssize_t strbuf_read_file_or_whine(struct strbuf *sb, const char *path)
> +{
> + int fd;
> + ssize_t len;
> +
> + fd = open(path, O_RDONLY);
> + if (fd < 0)
> + return error_errno(_("could not open '%s'"), path);
> + len = strbuf_read(sb, fd, 0);
> + close(fd);
> + if (len < 0)
> + return error(_("could not read '%s'."), path);
> + return len;
> +}

If we were to use error_errno() in the second conditional here, we
should take care not to clobber errno during the close(). I think
strbuf_read_file() actually has the same problem, which might be worth
fixing.

-Peff

Re: [BUG] [git 2.16.1] yeeek ... my files are gone .. by git pull

[Jeff King]

On Fri, Feb 23, 2018 at 06:29:55AM +0100, "Marcel 'childNo͡.de' Trautwein" 
wrote:

> shows me a quite different behavior, so solely rebase not seems the full 
> problem
> BUT
> `--rebase=preserve` will .. o’man , really, is this intended?

Yeah, the bug seems to be in --preserve-merges. Here's an easier
reproduction:

  git init
  >one && git add one && git commit -m one
  git checkout --orphan other
  git mv one two && git commit -m two

  git rebase --preserve-merges master

at which point we've dropped commit "two" and its files entirely.

I don't know much about how preserve-merges works. It looks like in the
loop around git-rebase--interactive.sh:931 that we mark commit "two"
with preserve=t, and so we _don't_ add it to the list of commits to
pick.

I think that stems from the fact that it has no parent marked to be
rewritten.

So something like this helps:

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 81c5b42875..71e6cbb388 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -921,15 +921,20 @@ else
 
if test -z "$rebase_root"
then
preserve=t
+   p=
for p in $(git rev-list --parents -1 $sha1 | cut -d' ' 
-s -f2-)
do
if test -f "$rewritten"/$p
then
preserve=f
fi
done
+   if test -z "$p"
+   then
+   preserve=f
+   fi
else
preserve=f
fi
if test f = "$preserve"

Because it at least adds "two" to the list of commits to pick. But
oddly, it picks it directly as a root commit again. Whereas a rebase
without --preserve-merges (and even "-i") picks it on top of commit
"one" (which is what I'd expect).

+cc Dscho, as the --preserve-merges guru.

-Peff

Re: RFC: git squash

[Junio C Hamano]

Julius Musseau  writes:

> git squash []
>
> Squashes ..HEAD into a single commit. Replaces HEAD with the
> result.  If not specified,  defaults to the current branch's
> upstream (a.k.a. @{upstream}).
>
> Rationale:
>
> This command provides an intuitive mechanism for in-place squash that
> doesn't drop dirty merge results.
>
> We call this an in-place squash because the state of all files and
> directories at HEAD does not change. Only the ancestory of HEAD
> changes: its (only) parent becomes the merge-base of  and
> HEAD, removing all intermediate commits.

So is it essentially the same as

git reset --soft $(git merge-base $commit HEAD)
git commit

with some icing for coming up with a default log message?  The above
won't touch the working tree at all.

Re: [BUG] [git 2.16.1] yeeek ... my files are gone .. by git pull

[Marcel 'childNo͡.de' Trautwein]

> Am 23.02.2018 um 00:20 schrieb Jonathan Nieder :
> 
> Hi Marcel,
> 
> …
> Sorry, this is not the most helpful reply but:
> 
> Can you describe a reproduction recipe so that I can experience the
> same thing?
> 
> That is:
> 
> 1. steps to reproduce
> 2. expected result
> 3. actual result
> 4. the difference and why it was unexpected
> 

1. steps to reproduce
=
```
Last login: Fri Feb 23 00:33:11 on ttys001
~ PATH variable not enhanced, no applications found in ~/Applications/*-latest


-bash:/Users/marcel:$ mkdir /tmp/$$
change to new directory '/tmp/2608'? [Y/n] 


-bash:/tmp/2608:$ mkdir a.git
change to new directory 'a.git'? [Y/n] 


-bash:/tmp/2608/a.git:$ git init
Initialized empty Git repository in /private/tmp/2608/a.git/.git/


-bash:/tmp/2608/a.git:$ touch foo


-bash:/tmp/2608/a.git:$ git add foo


-bash:/tmp/2608/a.git:$ git commit -m "foo" foo
[master (root-commit) ed191c4] foo
 1 file changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 foo


-bash:/tmp/2608/a.git:$ cd -
/tmp/2608


-bash:/tmp/2608:$ mkdir b.git
change to new directory 'b.git'? [Y/n] 


-bash:/tmp/2608/b.git:$ git init
Initialized empty Git repository in /private/tmp/2608/b.git/.git/


-bash:/tmp/2608/b.git:$ touch bar


-bash:/tmp/2608/b.git:$ git add bar


-bash:/tmp/2608/b.git:$ git commit -m "bar" bar
[master (root-commit) 80b0355] bar
 1 file changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 bar


-bash:/tmp/2608/b.git:$ cd -
/tmp/2608


-bash:/tmp/2608:$ git clone a.git c
Cloning into 'c'...
done.


-bash:/tmp/2608:$ cd c


-bash:/tmp/2608/c:$ ll
total 0
drwxr-xr-x  12 marcel  wheel   384B 23 Feb 05:47 .git
-rw-r--r--   1 marcel  wheel 0B 23 Feb 05:47 foo


-bash:/tmp/2608/c:$ git pull ../b.git/
warning: no common commits
remote: Counting objects: 3, done.
remote: Total 3 (delta 0), reused 0 (delta 0)
Unpacking objects: 100% (3/3), done.
From ../b
 * branchHEAD   -> FETCH_HEAD
Successfully rebased and updated refs/heads/master.


-bash:/tmp/2608/c:$ ll
total 0
drwxr-xr-x  14 marcel  wheel   448B 23 Feb 05:47 .git
-rw-r--r--   1 marcel  wheel 0B 23 Feb 05:47 bar


-bash:/tmp/2608/c:$ git reflog
80b0355 (HEAD -> master) HEAD@{0}: pull ../b.git/: checkout 
80b03552466bc526b1130ce5ca4a991ba31a0546: returning to refs/heads/master
80b0355 (HEAD -> master) HEAD@{1}: pull ../b.git/: checkout 
80b03552466bc526b1130ce5ca4a991ba31a0546
ed191c4 (origin/master, origin/HEAD) HEAD@{2}: clone: from /tmp/2608/a.git


-bash:/tmp/2608/c:$ git remote -v
origin  /tmp/2608/a.git (fetch)
origin  /tmp/2608/a.git (push)


-bash:/tmp/2608/c:$  git log --all --graph --decorate --oneline 
--simplify-by-decoration
* 80b0355 (HEAD -> master) bar
* ed191c4 (origin/master, origin/HEAD) foo
```

2. expected result
==
just an error in case the too trees have no common ancestors

```
-bash:/tmp/2608/c:$ git pull ../b.git/
warning: no common commits
remote: Counting objects: 3, done.
remote: Total 3 (delta 0), reused 0 (delta 0)
Unpacking objects: 100% (3/3), done.
From ../b
 * branchHEAD   -> FETCH_HEAD
fatal: refusing to merge unrelated histories
```

3. actual result

pulls out, removes all files from the first tree

4. the difference and why it was unexpected
===
I can’t find words on it … it should not work but it did? somehow … with 
unexpected results to my local repository

it somehow seems to be an issue of my config, because resetting it, will not 
allow the pull as expected

```
-bash:/tmp/2608/c:$ GIT_CONFIG_NOSYSTEM=1 HOME=. git config -l
core.repositoryformatversion=0
core.filemode=true
core.bare=false
core.logallrefupdates=true
core.ignorecase=true
core.precomposeunicode=true
remote.origin.url=/tmp/2608/a.git
remote.origin.fetch=+refs/heads/*:refs/remotes/origin/*
branch.master.remote=origin
branch.master.merge=refs/heads/master


-bash:/tmp/2608/c:$ GIT_CONFIG_NOSYSTEM=1 HOME=. git pull ../b.git/
warning: no common commits
remote: Counting objects: 3, done.
remote: Total 3 (delta 0), reused 0 (delta 0)
Unpacking objects: 100% (3/3), done.
From ../b
 * branchHEAD   -> FETCH_HEAD
fatal: refusing to merge unrelated histories


-bash:/tmp/2608/c:$ git pull ../b.git/
From ../b
 * branchHEAD   -> FETCH_HEAD
Successfully rebased and updated refs/heads/master.
```

the logs tells me he rebases ...
```
-bash:/tmp/2608/c:$ git config -l | grep merge
diff.tool=p4merge
merge.tool=p4merge
merge.branchdesc=true
merge.log=true
branch.autosetupmerge=true
branch.master.merge=refs/heads/master


-bash:/tmp/2608/c:$ git config -l | grep pull
pull.rebase=preserve


-bash:/tmp/2608/c:$ git config -l | grep fetch
fetch.recursesubmodules=true
remote.origin.fetch=+refs/heads/*:refs/remotes/origin/*
```


> I suspect that this information is in your message, somewhere, but it
> is (understandably) unfocussed and I am having trouble pulling it out.
> 

I’m sorry, 

F.LLI PISTOLESI Snc Company

[.F.LLI PISTOLESI]

Hello , 
 


I am looking for a reliable supplier /manufacturer of products for sell in 
Europe.

I came across your listing and wanted to get some information regarding minimum 
Order Quantities, FOB pricing and also the possibility of packaging including 
payments terms.

So could you please get back to be with the above informations as soon as 
possible .

My email is :tm6428...@gmail.com

Many thanks and i looking forward to hearing from you and hopefully placing an 
order with you company.

Best Regards
Lorenzo Delleani.

F.LLI PISTOLESI Snc Company P.O. box 205
2740 AE Waddinxveen
The Netherlands

FUSE with git

[Duane Knesek]

Disclaimer:  I am not a git developer, nor have I ever written
anything FUSE. So I apologize if the following is idiotic:

I've been looking for a virtual file system (in Linux) that works with
git to make huge working directories fast.  I have found that
Microsoft has written GVFS in Windows for this purpose.  I read in a
forum a discussion where they said using a FUSE implementation was too
slow and that they had to write a full file system at the kernel level
to be fast enough.  Their web page also boasts that a checkout takes
30 seconds rather than 3 hours.  My question is why?

If a FUSE was implemented in a way where a git checkout would do
nothing more than copy the snapshot manifest file locally, wouldn't
that basically be instantaneous?  The file system could then fetch
files by the hashes within that manifest whenever one needed to be
read.  Files would only need to be stored locally if it they were
modified.  Since the file system would know exactly what files were
modified, then it seems that git status and commit would be fast as
well.

Perhaps MS implemented GVFS that way because building a big tree from
scratch would be slow if it had to go into user space over and over
again?  If so, then what if a build system like Bazel (from Google)
was used to always build everything incrementally? It too could be
modified (maybe via plugin) to interact with the file system to know
exactly what files changed without reading everything.  The file
system could also use Google's hashes and remote caching to provide
unmodified binary content just like it would use git's SHA1 to provide
unmodified source content from git.  So when a user did a checkout, it
would appear that all the binaries were committed along the source
code.  Bazel would build new binaries from the source that was
modified, and only those new binaries would be written locally. To
execute those binaries, most of them would be read from the cache
while new ones would be read locally.

Perhaps that runtime part is the issue?  Executing the resulting code
was too slow due to the slower file access?  I would think that hit
would not be too bad, and only be during init, but perhaps I'm wrong.


Microsoft is full of really smart guys.  So clearly I am missing
something.  What is it?


(Sorry if I'm wasting your time)

RFC: git squash

[Julius Musseau]

Hi, Git Developers,

Thanks for your help regarding my earlier email (trying to break git
pull --rebase).

I just wanted to warn you all that my first attempt at a patch is
imminent.  I'm working on a "git squash" command.  Here's a quick
summary:

--
git squash []

Squashes ..HEAD into a single commit. Replaces HEAD with the
result.  If not specified,  defaults to the current branch's
upstream (a.k.a. @{upstream}).

Rationale:

This command provides an intuitive mechanism for in-place squash that
doesn't drop dirty merge results.

We call this an in-place squash because the state of all files and
directories at HEAD does not change. Only the ancestory of HEAD
changes: its (only) parent becomes the merge-base of  and
HEAD, removing all intermediate commits.

Alternatives:

- "git merge --squash master" correctly preserves dirty merge results,
but it's tricky to achieve an in-place squash with this command, since
it requires the following sequence of commands (these assume the
current branch is "feature" and we want to squash it relative to
"master"):

git checkout $(git merge-base HEAD master)
git merge --squash feature
git commit
git branch -f feature
git checkout feature

- "git rebase --interactive HEAD~N" with commits set to "squash" (or
"fixup") is very popular in industry.  But it has some tricky edge
cases: drops dirty merge results, can run into conflicts, and can be
confusing if HEAD~N spans any merges (including clean merges).
-


I expect I'll have the patch finished this weekend, and ready for
everyone to look at by Monday (Feb 26th).

Note:  I'm not 100% sure "git rebase --interactive" would drop dirty
merge results (e.g., the dirty parts from conflict-resolving merges).
That's speculation on my part.  I'll confirm this before I finish and
submit the patch.


yours sincerely,

Julius Musseau

Re: [PATCH v3 13/35] ls-refs: introduce ls-refs server command

[Brandon Williams]

On 02/22, Jeff King wrote:
> On Tue, Feb 06, 2018 at 05:12:50PM -0800, Brandon Williams wrote:
> 
> > +ls-refs takes in the following parameters wrapped in packet-lines:
> > +
> > +symrefs
> > +   In addition to the object pointed by it, show the underlying ref
> > +   pointed by it when showing a symbolic ref.
> > +peel
> > +   Show peeled tags.
> > +ref-pattern 
> > +   When specified, only references matching the one of the provided
> > +   patterns are displayed.
> 
> How do we match those patterns? That's probably an important thing to
> include in the spec.

Yeah I thought about it when I first wrote it and was hoping that
someone who nudge me in the right direction :)

> 
> Looking at the code, I see:
> 
> > +/*
> > + * Check if one of the patterns matches the tail part of the ref.
> > + * If no patterns were provided, all refs match.
> > + */
> > +static int ref_match(const struct argv_array *patterns, const char 
> > *refname)
> 
> This kind of tail matching can't quite implement all of the current
> behavior. Because we actually do the normal dwim_ref() matching, which
> includes stuff like "refs/remotes/%s/HEAD".
> 
> The other problem with tail-matching is that it's inefficient on the
> server. Ideally we could get a request for "master" and only look up
> refs/heads/master, refs/tags/master, etc. And if there are 50,000 refs
> in refs/pull, we wouldn't have to process those at all. Of course this
> is no worse than the current code, which not only looks at each ref but
> actually _sends_ it. But it would be nice if we could fix this.
> 
> There's some more discussion in this old thread:
> 
>   
> https://public-inbox.org/git/20161024132932.i42rqn2vlpocq...@sigill.intra.peff.net/

Thanks for the pointer.  I was told to be wary a while about about
performance implications on the server but no discussion ensued till now
about it :)

We always have the ability to extend the patterns accepted via a feature
(or capability) to ls-refs, so maybe the best thing to do now would only
support a few patterns with specific semantics.  Something like if you
say "master" only match against refs/heads/ and refs/tags/ and if you
want something else you would need to specify "refs/pull/master"?

That way we could only support globs at the end "master*" where * can
match anything (including slashes)

> 
> > +{
> > +   char *pathbuf;
> > +   int i;
> > +
> > +   if (!patterns->argc)
> > +   return 1; /* no restriction */
> > +
> > +   pathbuf = xstrfmt("/%s", refname);
> > +   for (i = 0; i < patterns->argc; i++) {
> > +   if (!wildmatch(patterns->argv[i], pathbuf, 0)) {
> > +   free(pathbuf);
> > +   return 1;
> > +   }
> > +   }
> > +   free(pathbuf);
> > +   return 0;
> > +}
> 
> Does the client have to be aware that we're using wildmatch? I think
> they'd need "refs/heads/**" to actually implement what we usually
> specify in refspecs as "refs/heads/*". Or does the lack of WM_PATHNAME
> make this work with just "*"?
> 
> Do we anticipate that the client would left-anchor the refspec like
> "/refs/heads/*" so that in theory the server could avoid looking outside
> of /refs/heads/?

Yeah we may want to anchor it by providing the leading '/' instead of
just "refs/".

> 
> -Peff

I need to read over the discussion you linked to more but what sort of
ref patterns do you believe we should support as part of the initial
release of v2?  It seems like you wanted this at some point in the past
so I assume you have an idea of what sort of filtering would be
beneficial.

-- 
Brandon Williams

RE: removed submodules shown as untracked when switching branches

[Mike Friedrich]

Thank You. This is interesting. There seems to be also a config option 
submodule.recurse. I did not know that these options have such an effect on the 
checkout command.

Best Regards, Mike

-Original Message-
From: Stefan Beller [mailto:sbel...@google.com]
Sent: Thursday, February 22, 2018 6:53 PM
To: Mike Friedrich 
Cc: git@vger.kernel.org
Subject: Re: removed submodules shown as untracked when switching branches

On Thu, Feb 22, 2018 at 12:26 PM, Mike Friedrich  wrote:

> git submodule add ../submodule sub
> git add sub
> git commit -m "submodule added"
>
> git checkout master

The original behavior of checkout is to ignore submodules, hence it will be 
left alone.
Can you retry this recipe with --recurse-submodules given to checkout.
(Or rather run "git config submodule.recurse true" once)

Thanks,
Stefan



This email is non-binding, is subject to contract, and neither Kulicke and 
Soffa Industries, Inc. nor its subsidiaries (each and collectively “K”) shall 
have any obligation to you to consummate the transactions herein or to enter 
into any agreement, other than in accordance with the terms and conditions of a 
definitive agreement if and when negotiated, finalized and executed between the 
parties. This email and all its contents are protected by International and 
United States copyright laws. Any reproduction or use of all or any part of 
this email without the express written consent of K is prohibited.

Re: removed submodules shown as untracked when switching branches

[Stefan Beller]

On Thu, Feb 22, 2018 at 12:26 PM, Mike Friedrich  wrote:

> git submodule add ../submodule sub
> git add sub
> git commit -m "submodule added"
>
> git checkout master

The original behavior of checkout is to ignore submodules, hence it
will be left alone.
Can you retry this recipe with --recurse-submodules given to checkout.
(Or rather run "git config submodule.recurse true" once)

Thanks,
Stefan

Re: Git should preserve modification times at least on request

[Derek Fawcus]

On Wed, Feb 21, 2018 at 11:44:13PM +0100, Ævar Arnfjörð Bjarmason wrote:
> On Wed, Feb 21 2018, Peter Backes jotted:
> > On Wed, Feb 21, 2018 at 10:33:05PM +0100, Ævar Arnfjörð Bjarmason wrote:
> >> This sounds like a sensible job for a git import tool, i.e. import a
> >> target directory into git, and instead of 'git add'-ing the whole thing
> >> it would look at the mtimes, sort files by mtime, then add them in order
> >> and only commit those files that had the same mtime in the same commit
> >> (or within some boundary).
> >
> > I think that this would be The Wrong Thing to do.

Agreed, but probably for a different reason.

> I'm merely pointing out that if you have the use-case Derek Fawcus
> describes you can get per-file mtimes via something similar to the the
> hook method Theodore Ts'o described today with a simple import tool with
> no changes to git or its object format required.

Actually, I was not proposing any change to the git objects.
I was simply suggesting a case where I'd have found a optional mechanism
for mtime restoration useful.

What would be useful is a better version of the hook based scheme which
Ted mentioned.  The import could be via a wrapper script, but checkouts
would have to be via a hook such that the original timestamps could then
be applied; and those stamps would have to be part of the tar-file commit.

The idea of automatically generating a bunch of commits in time order
would be the wrong thing here. That is because one file could well
contain changes from more than one logical commit (as guided by the
Changelog), and that one logical commit can be spread across a few
files with diffrent mode time, one has to manually tease those apart.

So here the purpose behind restoring the timestamps is as an aid in
guiding the examination of files to find the changes referenced in
the Changelog.

Git is quite useful for this sort of effort, as once a sensible commit
has been synthsized, rebase of the next tar-file commit then helps
reveal the next set of changes.

So what I'm thinking of is for stuff like this: 
https://github.com/DoctorWkt/unix-jun72
(and the other repros there), where one wishes to figure out and
regenerate a history of changes.  Since git is quite useful for
representing the end result, it is just that other scripting
may make it easier to use for such cases.

DF

Re: [PATCH v3 04/35] upload-pack: convert to a builtin

[Jeff King]

On Thu, Feb 22, 2018 at 06:05:15PM -0500, Jeff King wrote:

> On Thu, Feb 22, 2018 at 02:42:35PM -0800, Jonathan Nieder wrote:
> 
> > > I couldn't quite get it to work, but I think it's because I'm doing
> > > something wrong with the submodules. But I also think this attack would
> > > _have_ to be done over ssh, because on a local system the submodule
> > > clone would a hard-link rather than a real fetch.
> > 
> > What happens if the submodule URL starts with file://?
> 
> Ah, that would do it. Or I guess any follow-up fetch.
> 
> I'm still having trouble convincing submodules to fetch _just_ the
> desired sha1, though. It always just fetches everything. I know there's
> a way that this kicks in (that's why we have things like
> allowReachableSHA1InWant), but I'm not sufficiently well-versed in
> submodules to know how to trigger it.

 This won't work anyway. I was right when I said that we don't
redirect stderr for rev-list, but of course it's stdout that determines
the pager behavior. So I don't think you could get rev-list to trigger a
pager here.

I don't think there's currently any vulnerability, but it's more to do
with luck than any amount of carefulness on our part.

-Peff

Re: [BUG] [git 2.16.1] yeeek ... my files are gone .. by git pull

[Jonathan Nieder]

Hi Marcel,

Marcel 'childNo͡.de' Trautwein" wrote:

> I think we have a problem … or at least I had
> and I’m not quite sure if this is „working as designed“
> but I’m sure it „should not work as it did“.
[...]
> I wanted to clone another repository … but yeah … it’s late for me today and 
> I put
> in s.th. `git pull 
> g...@private.gitlab.instance.example.com:aGroup/repository.git`
>
> next … all committed files are zapped and the repository given has
> been checked out in my home directory 勞
>
> what? Shouldn’t this just fail? Why can I pass another remote to pull?

Sorry, this is not the most helpful reply but:

Can you describe a reproduction recipe so that I can experience the
same thing?

That is:

 1. steps to reproduce
 2. expected result
 3. actual result
 4. the difference and why it was unexpected

I suspect that this information is in your message, somewhere, but it
is (understandably) unfocussed and I am having trouble pulling it out.

[...]
> trying to fix this up by doing another pull failed:
> ```
> -bash:$ git remote -v
> origing...@bitbucket.org:childnode/marcel.git (fetch)
> origing...@bitbucket.org:childnode/marcel.git (push)
>
> -bash:$ git pull
> fatal: refusing to merge unrelated histories

Ok, this part is something I might be able to help shed some light on.

Searching for 'unrelated' in "git help pull" finds:

   --allow-unrelated-histories
   By default, git merge command refuses to merge histories that do not
   share a common ancestor. This option can be used to override this
   safety when merging histories of two projects that started their
   lives independently. As that is a very rare occasion, no
   configuration variable to enable this by default exists and will not
   be added.

So that explains the "what" of that error message.

The "why" is a separate question.  Could you share output from

  git log --all --graph --decorate --oneline --simplify-by-decoration

and

  git status

to help us understand your current state?

Also, suggestions for improvements to the 'refusing to merge' message
would be very welcome.

Thanks and hope that helps,
Jonathan

Re: [PATCH v3 04/35] upload-pack: convert to a builtin

[Jeff King]

On Thu, Feb 22, 2018 at 02:42:35PM -0800, Jonathan Nieder wrote:

> > I couldn't quite get it to work, but I think it's because I'm doing
> > something wrong with the submodules. But I also think this attack would
> > _have_ to be done over ssh, because on a local system the submodule
> > clone would a hard-link rather than a real fetch.
> 
> What happens if the submodule URL starts with file://?

Ah, that would do it. Or I guess any follow-up fetch.

I'm still having trouble convincing submodules to fetch _just_ the
desired sha1, though. It always just fetches everything. I know there's
a way that this kicks in (that's why we have things like
allowReachableSHA1InWant), but I'm not sufficiently well-versed in
submodules to know how to trigger it.

-Peff

[BUG] [git 2.16.1] yeeek ... my files are gone .. by git pull

[Marcel 'childNo͡.de' Trautwein]

Hi,

I think we have a problem … or at least I had
and I’m not quite sure if this is „working as designed“
but I’m sure it „should not work as it did“.

Because? It pruned a lot of files and even the local repository.
by pull
by giving another repository URL instead of a known remote

While working in a subpath of my homedir
(that is a git repository itself, without any changes in worktree or index: 
https://bitbucket.org/childnode/marcel/ )
I wanted to clone another repository … but yeah … it’s late for me today and I 
put
in s.th. `git pull 
g...@private.gitlab.instance.example.com:aGroup/repository.git`

next … all committed files are zapped and the repository given has been checked 
out in my home directory 勞

what? Shouldn’t this just fail? Why can I pass another remote to pull?

 god any untracked / ignored files are still alive

a, yeh, I’m on a mac 
(for any git configuration … have a look in my repository 
https://bitbucket.org/childnode/marcel/src/88ff8d0c28bb90dfde3aea9e6c39bb551bea8ca8/.gitconfig?at=master=file-view-default

the console out was:
```
-bash:$ git pull g...@private.gitlab.instance.example.com:aGroup/repository.git
Warning: Permanently added the ECDSA host key for IP address '10.1.2.3' to the 
list of known hosts.
warning: no common commits
remote: Counting objects: 2301, done.
remote: Compressing objects: 100% (710/710), done.
remote: Total 2301 (delta 1040), reused 2239 (delta 1004)
Receiving objects: 100% (2301/2301), 405.41 KiB | 635.00 KiB/s, done.
Resolving deltas: 100% (1040/1040), done.
From private.gitlab.instance.example.com:aGroup/repository
 * branchHEAD   -> FETCH_HEAD
Fetching submodule .shapps/willgit
Fetching submodule .vim
Fetching submodule .vim/autoload/pathogen
warning: redirecting to https://github.com/tpope/vim-pathogen.git/
Fetching submodule .vim/bundle/ack
warning: redirecting to https://github.com/mileszs/ack.vim.git/
Fetching submodule .vim/bundle/colors-solarized
warning: redirecting to https://github.com/altercation/vim-colors-solarized.git/
Fetching submodule .vim/bundle/flake8
Fetching submodule .vim/bundle/fugitive
warning: redirecting to https://github.com/tpope/vim-fugitive.git/
Fetching submodule .vim/bundle/kwbdi
warning: redirecting to https://github.com/vim-scripts/kwbdi.vim.git/
Fetching submodule .vim/bundle/markdown
warning: redirecting to https://github.com/tpope/vim-markdown.git/
Fetching submodule .vim/bundle/nerdtree
warning: redirecting to https://github.com/scrooloose/nerdtree.git/
Fetching submodule .vim/bundle/unimpaired
warning: redirecting to https://github.com/tpope/vim-unimpaired.git/
Fetching submodule 
gists/bitbucket/childnode/2015-06-16_G4pLy_prevent-empty-version-comment-in.git
Fetching submodule 
gists/bitbucket/childnode/2015-06-21_kyAAM_plasticscm-addcurrentworkdir-batch-task
Fetching submodule gists/github/childnode/18de20f4448692257aa3e99c8319b70d
Fetching submodule gists/github/childnode/295dbd6e_hasSize.regex
Fetching submodule gists/github/childnode/4a0de936_gradle_buildSrc_dogfood
Fetching submodule gists/github/childnode/66d4b982_git.rebaseAllBranches
Fetching submodule 
gists/github/childnode/6df6d14c_ideaGradleProjectSetupForAdditionalSourceSets
Fetching submodule gists/github/childnode/81ae6468_build_jar_manifest.gradle
Fetching submodule gists/github/childnode/85958ff8_extendedHelp.gradle_secret
Fetching submodule 
gists/github/childnode/88304258_git_deleteAllRemoteBranches.sh
Fetching submodule gists/github/childnode/8f100f90_dockerSaveAllImages.sh
Fetching submodule 
gists/github/childnode/9741c4d1_idea.warnGenerateWorkspace.gradle_secret
Fetching submodule gists/github/childnode/a175d954_ext.props.gradle_secret
Fetching submodule gists/github/childnode/d15cd5e9_atlassian-confluence-config
Fetching submodule gists/github/childnode/d35cf810dd28775ac5c0e491107215fd
Fetching submodule gists/github/childnode/da08e8a6f989ce0f94077ae1a6b1573b
Fetching submodule gists/github/childnode/e7ef876c_html2ical_secret
Fetching submodule gists/github/childnode/eb3199790f2f82785f62c3150f352ede
Successfully rebased and updated refs/heads/master.

```

trying to fix this up by doing another pull failed:
```
-bash:$ git remote -v
origin  g...@bitbucket.org:childnode/marcel.git (fetch)
origin  g...@bitbucket.org:childnode/marcel.git (push)

-bash:$ git pull
fatal: refusing to merge unrelated histories

-bash:$ git pull g...@bitbucket.org:childnode/marcel.git
From bitbucket.org:childnode/marcel
 * branchHEAD   -> FETCH_HEAD
fatal: refusing to merge unrelated histories

```

these messages and the fact that it doesn’t work backward let me think I ran 
into
a collision? really?

revlog looks a bit strange too
```
04f3066 (HEAD -> master) HEAD@{0}: pull 
g...@private.gitlab.instance.example.com:aGroup/repository.git: checkout 
04f3066d03e09323c7341c472be4c45ea5e3a4ff: returning to refs/heads/master
04f3066 (HEAD -> master) HEAD@{1}: pull 

Re: [PATCH v3 04/35] upload-pack: convert to a builtin

[Jonathan Nieder]

Jeff King wrote:

> All of that said, I think the current code is quite dangerous already,
> and maybe even broken.  upload-pack may run sub-commands like rev-list
> or pack-objects, which are themselves builtins.

Sounds like more commands to set the IGNORE_PAGER_CONFIG flag for in
git.c.

Thanks for looking this over thoughtfully.

[...]
> I couldn't quite get it to work, but I think it's because I'm doing
> something wrong with the submodules. But I also think this attack would
> _have_ to be done over ssh, because on a local system the submodule
> clone would a hard-link rather than a real fetch.

What happens if the submodule URL starts with file://?

Thanks,
Jonathan

DEAR FRIEND.

[Mr Daouda Ali]

Dear Friend,
   I am Mr.Daouda Ali the head of file department of Bank of
Africa(B.O.A) here in Burkina Faso / Ouagadougou. In my department we
discover an abandoned sum of (US$18 million US Dollars) in an account
that belongs to one of our foreign customer who died along with his
family in plane crash. It is therefore upon this discovery that I now
decided to make this business proposal to you and release the money to
you as the next of kin or relation to the deceased for the safety and
subsequent disbursement since nobody is coming for it. I agree that
40% of this money will be for you, while 60% would be for me. Then
after the money is been transferred into your account, I will visit
your country for an investment under your kind control.

You have to contact my Bank directly as the real next of kin of this
deceased account with next of kin application form. You have to send
me those your information below to enable me use it and get you next
of kin application form from bank, so that you will contact Bank for
the transfer of this money into your account.

Your Full Name___
Your Home Address
Your Age ___
Your Handset Number
Your Occupation ___

I am waiting for your urgent respond to enable us proceed further for
the transfer through my private e-mail(daoudaali...@gmail.com)

Yours faithfully,
Mr.Daouda Ali

Re: [PATCH 1/1] git-p4: add unshelve command

[Luke Diamand]

On 22 February 2018 at 21:39, Miguel Torroja  wrote:
> Hi Luke,
>
> I really like the idea of creating a branch based on a shelved CL (We
> particularly use shelves all the time), I tested your change and I
> have some comments.
>
>  - I have some concerns about having the same "[git-p4...change =
> .]" as if it were a real submitted CL.
> One use case I foresee of the new implementation could be to
> cherry-pick that change on another branch (or current branch) prior to
> a git p4 submit.

OK, I think we could just not add that in the case of an unshelved commit.

>
>  - I see that the new p4/unshelve... branch is based on the tip of
> p4/master by default. what if we set the default to the current HEAD?

There's a "--origin" option you can use to set it to whatever you want.

I started out with HEAD as the default, but then found that to get a
sensible diff you have to both sync and rebase, which can be quite
annoying.

In my case, in my early testing, I ended up with a git commit which
included both the creation of a file, and a subsequent change, even
though I had only unshelved the subsequent change. That was because
HEAD didn't include the file creation change (but p4/master _did_).

>
>  - Shelved CLs can be updated and you might have to run "git p4
> unshelve" a second time to get the latest updates. if we call it a
> second time it fails as it's trying to update p4/unshelve... rather
> than discarding previous one and creating a new one.

OK, that should also be fixable.

>
>
> Thanks,

Thanks for the feedback, very useful! I'll reroll.
Luke

Re: [PATCH v3 04/35] upload-pack: convert to a builtin

[Jeff King]

On Thu, Feb 22, 2018 at 04:44:02PM -0500, Jeff King wrote:

> But I don't think it _is_ an accident waiting to happen for the rest of
> the commands. upload-pack is special. The point is that people may touch
> git.c thinking they are adding a nice new feature (like pager config, or
> aliases, or default options, or whatever). And it _would_ be a nice new
> feature for most commands, but not for upload-pack, because its
> requirements are different.
> 
> So thinking about security in the git wrapper is just a burden for those
> other commands.

All of that said, I think the current code is quite dangerous already,
and maybe even broken.  upload-pack may run sub-commands like rev-list
or pack-objects, which are themselves builtins.

For example:

  git init --bare evil.git
  git -C evil.git --work-tree=. commit --allow-empty -m foo
  git -C evil.git config pager.pack-objects 'echo >&2 oops'
  git clone --no-local evil.git victim

That doesn't _quite_ work, because we route pack-objects' stderr into a
pipe, which suppresses the pager. But we don't for rev-list, which we
call when checking reachability. It's a bit tricky to get a client to
trigger those for a vanilla fetch, though. Here's the best I could come
up with:

  git init --bare evil.git
  git -C evil.git --work-tree=. commit --allow-empty -m one
  git -C evil.git config pager.rev-list 'echo >&2 oops'

  git init super
  (
cd super
# obviously use host:path if you're attacking somebody over ssh
git submodule add ../evil.git evil
git commit -am 'add evil submodule'
  )
  git -C evil.git config uploadpack.allowReachableSHA1InWant true
  git -C evil.git update-ref -d refs/heads/master

  git clone --recurse-submodules super victim

I couldn't quite get it to work, but I think it's because I'm doing
something wrong with the submodules. But I also think this attack would
_have_ to be done over ssh, because on a local system the submodule
clone would a hard-link rather than a real fetch.

-Peff

Re: [PATCH v3 28/35] transport-helper: introduce stateless-connect

[Jonathan Tan]

On Thu, 22 Feb 2018 10:53:53 -0800
Brandon Williams  wrote:

> > > @@ -612,6 +615,11 @@ static int process_connect_service(struct transport 
> > > *transport,
> > >   if (data->connect) {
> > >   strbuf_addf(, "connect %s\n", name);
> > >   ret = run_connect(transport, );
> > > + } else if (data->stateless_connect) {
> > > + strbuf_addf(, "stateless-connect %s\n", name);
> > > + ret = run_connect(transport, );
> > > + if (ret)
> > > + transport->stateless_rpc = 1;
> > 
> > Why is process_connect_service() falling back to stateless_connect if
> > connect doesn't work? I don't think this fallback would work, as a
> > client that needs "connect" might need its full capabilities.
> 
> Right now there isn't really a notion of "needing" connect since if
> connect fails then you need to fallback to doing the dumb thing.  Also
> note that there isn't all fallback from connect to stateless-connect
> here.  If the remote helper advertises connect, only connect will be
> tried even if stateless-connect is advertised.  So this only really
> works in the case where stateless-connect is advertised and connect
> isn't, as is with our http remote-helper.

After some in-office discussion, I think I understand how this works.
Assuming a HTTP server that supports protocol v2 (at least for
ls-refs/fetch):

 1. Fetch, which supports protocol v2, will (indirectly) call
process_connect_service. If it learns that it supports v2, it must
know that what's returned may not be a fully bidirectional channel,
but may only be a stateless-connect channel (and it does know).
 2. Archive/upload-archive, which does not support protocol v2, will
(indirectly) call process_connect_service. stateless_connect checks
info/refs and observes that the server supports protocol v2, so it
returns a stateless-connect channel. The user, being unaware of
protocol versions, tries to use it, and it doesn't work. (This is a
slight regression in that previously, it would fail more quickly -
archive/upload-archive has always not supported HTTP because HTTP
doesn't support connect.)

I still think that it's too confusing for process_connect_service() to
attempt to fallback to stateless-connect, at least because the user must
remember that process_connect_service() returns such a channel if
protocol v2 is used (and existing code must be updated to know this).
It's probably better to have a new API that can return either a connect
channel or a stateless-connect channel, and the user will always use it
as if it was a stateless-connect channel. The old API then can be
separately deprecated and removed, if desired.

Re: [PATCH v3 04/35] upload-pack: convert to a builtin

[Jeff King]

On Thu, Feb 22, 2018 at 01:26:34PM -0800, Jonathan Nieder wrote:

> Keep in mind that git upload-archive (a read-only command, just like
> git upload-pack) also already has the same issues.

Yuck. I don't think we've ever made a historical promise about that. But
then, I don't think the promise about upload-pack has ever really been
documented, except in mailing list discussions.

-Peff

Re: [PATCH v3 04/35] upload-pack: convert to a builtin

[Jeff King]

On Thu, Feb 22, 2018 at 01:24:02PM -0800, Jonathan Nieder wrote:

> >  But my greater concern is that people who
> > work on git.c should not have to worry about accidentally violating this
> > principle when they add a new feature or config option.
> 
> That sounds like a combination of (6) and insufficient documentation
> or tests.  Ideas for how we can help prevent such accidents?

I don't think it's insufficient tests. How can we test against some
problem in the untrusted repository when the feature that would trigger
it has not been written yet?

E.g., imagine we begin to allow alias.* to override command names in
git.c. Now suddenly setting "alias.upload-pack" is a vulnerability.
Should we add a test for that _now_ as a precaution? I don't think so,
because we can't guess what new features are going to be added. So we'd
be lucky if such a test actually did anything useful.

A comment could help, but it seems quite likely that whatever feature
somebody is adding might not be right next to that comment (and thus not
seen). I think we're mostly relying on institutional knowledge during
review to catch these things. Which is not great, but I'm not sure where
we'd document that knowledge that people would actually see it at the
right time.

> > In other words, it seems like an accident waiting to happen. I'd be more
> > amenable to it if there was some compelling reason for it to be a
> > builtin, but I don't see one listed in the commit message. I see only
> > "let's make it easier to share the code", which AFAICT is equally served
> > by just lib-ifying the code and calling it from the standalone
> > upload-pack.c.
> 
> If we have so little control of the common code used by git commands
> that could be invoked by a remote user, I think we're in trouble
> already.  I don't think being a builtin vs not makes that
> significantly different, since there are plenty of builtins that can
> be triggered by remote users.  Further, if we have so little control
> over the security properties of git.c, what hope do we have of making
> the rest of libgit.a usable in secure code?

I agree that the situation is already pretty dicey. But I also think
that using the git wrapper is more risky than the rest of libgit.a.
There's tons of dangerous code in libgit.a, but upload-pack is smart
enough not to call it. And people modifying upload-pack have a greater
chance of thinking about the security implications, because they know
they're working with upload-pack. Whereas people are likely to touch
git.c without considering upload-pack at all.

The big danger in libgit.a is from modifying some low-level code called
by upload-pack in a way that trusts the on-disk contents more than it
should. My gut says that's less likely, though certainly not impossible
(a likely candidate would perhaps be a ref backend config that opens up
holes; e.g., if you could point a database backend at some random path).

> In other words, having to pay more attention to the git wrapper from a
> security pov actually feels to me like a *good* thing.  The git
> wrapper is the entry point to almost all git commands.  If it is an
> accident waiting to happen, then anything that calls git commands is
> already an accident waiting to happen.  So how can we make it not an
> accident waiting to happen? :)

But I don't think it _is_ an accident waiting to happen for the rest of
the commands. upload-pack is special. The point is that people may touch
git.c thinking they are adding a nice new feature (like pager config, or
aliases, or default options, or whatever). And it _would_ be a nice new
feature for most commands, but not for upload-pack, because its
requirements are different.

So thinking about security in the git wrapper is just a burden for those
other commands.

> > There's not much point for receive-pack. It respects hooks, so any
> > security ship has already sailed there.
> 
> Yet there are plenty of cases where people who can push are not
> supposed to have root privilege.  I am not worried about hooks
> specifically (although the changes described at [1] might help and I
> still plan to work on those) but I am worried about e.g. commandline
> injection issues.  I don't think we can treat receive-pack as out of
> scope.
> 
> And to be clear, I don't think you were saying receive-pack *is* out
> of scope.  But you seem to be trying to draw some boundary, where I
> only see something fuzzier (e.g. if a bug only applies to
> receive-pack, then that certainly decreases its impact, but it doesn't
> make the impact go away).

Right, I think command-line injection is a separate issue. My concern is
_just_ about "can we be run against on-disk repo contents". And nothing
matters for receive-pack there, because you can already execute
arbitrary code with hooks.

-Peff

Re: [PATCH 1/1] git-p4: add unshelve command

[Miguel Torroja]

Hi Luke,

I really like the idea of creating a branch based on a shelved CL (We
particularly use shelves all the time), I tested your change and I
have some comments.

 - I have some concerns about having the same "[git-p4...change =
.]" as if it were a real submitted CL.
One use case I foresee of the new implementation could be to
cherry-pick that change on another branch (or current branch) prior to
a git p4 submit.

 - I see that the new p4/unshelve... branch is based on the tip of
p4/master by default. what if we set the default to the current HEAD?

 - Shelved CLs can be updated and you might have to run "git p4
unshelve" a second time to get the latest updates. if we call it a
second time it fails as it's trying to update p4/unshelve... rather
than discarding previous one and creating a new one.


Thanks,


On Thu, Feb 22, 2018 at 10:50 AM, Luke Diamand  wrote:
> This can be used to "unshelve" a shelved P4 commit into
> a git commit.
>
> For example:
>
>   $ git p4 unshelve 12345
>
> The resulting commit ends up in the branch:
>refs/remotes/p4/unshelved/12345
>
> Signed-off-by: Luke Diamand 
> ---
>  Documentation/git-p4.txt |  22 
>  git-p4.py| 128 
> +++
>  t/t9832-unshelve.sh  |  67 +
>  3 files changed, 186 insertions(+), 31 deletions(-)
>  create mode 100755 t/t9832-unshelve.sh
>
> diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
> index d8c8f11c9f..910ae63a1c 100644
> --- a/Documentation/git-p4.txt
> +++ b/Documentation/git-p4.txt
> @@ -164,6 +164,21 @@ $ git p4 submit --shelve
>  $ git p4 submit --update-shelve 1234 --update-shelve 2345
>  
>
> +
> +Unshelve
> +
> +Unshelving will take a shelved P4 changelist, and produce the equivalent git 
> commit
> +in the branch refs/remotes/p4/unshelved/.
> +
> +The git commit is created relative to the current p4/master branch, so if 
> this
> +branch is behind Perforce itself, it may include more changes than you 
> expected.
> +
> +
> +$ git p4 sync
> +$ git p4 unshelve 12345
> +$ git show refs/remotes/p4/unshelved/12345
> +
> +
>  OPTIONS
>  ---
>
> @@ -337,6 +352,13 @@ These options can be used to modify 'git p4 rebase' 
> behavior.
>  --import-labels::
> Import p4 labels.
>
> +Unshelve options
> +
> +
> +--origin::
> +Sets the git refspec against which the shelved P4 changelist is compared.
> +Defaults to p4/master.
> +
>  DEPOT PATH SYNTAX
>  -
>  The p4 depot path argument to 'git p4 sync' and 'git p4 clone' can
> diff --git a/git-p4.py b/git-p4.py
> index 7bb9cadc69..59bd4ff64f 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -316,12 +316,17 @@ def p4_last_change():
>  results = p4CmdList(["changes", "-m", "1"], skip_info=True)
>  return int(results[0]['change'])
>
> -def p4_describe(change):
> +def p4_describe(change, shelved=False):
>  """Make sure it returns a valid result by checking for
> the presence of field "time".  Return a dict of the
> results."""
>
> -ds = p4CmdList(["describe", "-s", str(change)], skip_info=True)
> +cmd = ["describe", "-s"]
> +if shelved:
> +cmd += ["-S"]
> +cmd += [str(change)]
> +
> +ds = p4CmdList(cmd, skip_info=True)
>  if len(ds) != 1:
>  die("p4 describe -s %d did not return 1 result: %s" % (change, 
> str(ds)))
>
> @@ -2421,6 +2426,18 @@ class P4Sync(Command, P4UserMap):
>  if gitConfig("git-p4.syncFromOrigin") == "false":
>  self.syncWithOrigin = False
>
> +self.depotPaths = []
> +self.changeRange = ""
> +self.previousDepotPaths = []
> +self.hasOrigin = False
> +
> +# map from branch depot path to parent branch
> +self.knownBranches = {}
> +self.initialParents = {}
> +
> +self.tz = "%+03d%02d" % (- time.timezone / 3600, ((- time.timezone % 
> 3600) / 60))
> +self.labels = {}
> +
>  # Force a checkpoint in fast-import and wait for it to finish
>  def checkpoint(self):
>  self.gitStream.write("checkpoint\n\n")
> @@ -2429,7 +2446,7 @@ class P4Sync(Command, P4UserMap):
>  if self.verbose:
>  print "checkpoint finished: " + out
>
> -def extractFilesFromCommit(self, commit):
> +def extractFilesFromCommit(self, commit, shelved=False, shelved_cl = 0):
>  self.cloneExclude = [re.sub(r"\.\.\.$", "", path)
>   for path in self.cloneExclude]
>  files = []
> @@ -2452,6 +2469,9 @@ class P4Sync(Command, P4UserMap):
>  file["rev"] = commit["rev%s" % fnum]
>  file["action"] = commit["action%s" % fnum]
>  file["type"] = commit["type%s" % fnum]
> +if shelved:
> +file["shelved_cl"] = int(shelved_cl)
> +
>  files.append(file)
>  fnum = fnum + 1
>  return files
> 

Re: [PATCH v3 04/35] upload-pack: convert to a builtin

[Jonathan Nieder]

Jeff King wrote:

> The current property is that it's safe to fetch from an
> untrusted repository, even over ssh. If we're keeping that for protocol
> v1, we'd want it to apply to protocol v2, as well.

Ah, this is what I had been missing (the non-ssh case).

I see your point.  I think we need to fix the pager config issue and add
some clarifying documentation to git.c so that people know what to look
out for.

Keep in mind that git upload-archive (a read-only command, just like
git upload-pack) also already has the same issues.

Thanks,
Jonathan

Re: [PATCH v3 04/35] upload-pack: convert to a builtin

[Jonathan Nieder]

Hi,

Jeff King wrote:
> On Thu, Feb 22, 2018 at 11:38:14AM -0800, Jonathan Nieder wrote:

>> To be clear, which of the following are you (most) worried about?
>>
>>  1. being invoked with --help and spawning a pager
>>  2. receiving and acting on options between 'git' and 'upload-pack'
>>  3. repository discovery
>>  4. pager config
>>  5. alias discovery
>>  6. increased code surface / unknown threats
>
> My immediate concern is (4).

Thanks for clarifying.

>  But my greater concern is that people who
> work on git.c should not have to worry about accidentally violating this
> principle when they add a new feature or config option.

That sounds like a combination of (6) and insufficient documentation
or tests.  Ideas for how we can help prevent such accidents?

> In other words, it seems like an accident waiting to happen. I'd be more
> amenable to it if there was some compelling reason for it to be a
> builtin, but I don't see one listed in the commit message. I see only
> "let's make it easier to share the code", which AFAICT is equally served
> by just lib-ifying the code and calling it from the standalone
> upload-pack.c.

If we have so little control of the common code used by git commands
that could be invoked by a remote user, I think we're in trouble
already.  I don't think being a builtin vs not makes that
significantly different, since there are plenty of builtins that can
be triggered by remote users.  Further, if we have so little control
over the security properties of git.c, what hope do we have of making
the rest of libgit.a usable in secure code?

In other words, having to pay more attention to the git wrapper from a
security pov actually feels to me like a *good* thing.  The git
wrapper is the entry point to almost all git commands.  If it is an
accident waiting to happen, then anything that calls git commands is
already an accident waiting to happen.  So how can we make it not an
accident waiting to happen? :)

>> Although in most setups the user does not control the config files on
>> a server, item (4) looks like a real issue worth solving.  I think we
>> should introduce a flag to skip looking for pager config.  We could
>> use it for receive-pack, too.
>
> There's not much point for receive-pack. It respects hooks, so any
> security ship has already sailed there.

Yet there are plenty of cases where people who can push are not
supposed to have root privilege.  I am not worried about hooks
specifically (although the changes described at [1] might help and I
still plan to work on those) but I am worried about e.g. commandline
injection issues.  I don't think we can treat receive-pack as out of
scope.

And to be clear, I don't think you were saying receive-pack *is* out
of scope.  But you seem to be trying to draw some boundary, where I
only see something fuzzier (e.g. if a bug only applies to
receive-pack, then that certainly decreases its impact, but it doesn't
make the impact go away).

Thanks,
Jonathan

[1] 
https://public-inbox.org/git/20171002234517.gv19...@aiede.mtv.corp.google.com/

Re: [PATCH] sequencer: factor out strbuf_read_file_or_whine()

[Junio C Hamano]

René Scharfe  writes:

> Reduce code duplication by factoring out a function that reads an entire
> file into a strbuf, or reports errors on stderr if something goes wrong.
>
> Signed-off-by: Rene Scharfe 
> ---
> The difference to using strbuf_read_file() is more detailed error
> messages for open(2) failures.  But I don't know if we need them -- or
> under which circumstances reading todo files could fail anyway.  When
> doing multiple rebases in parallel perhaps?
>
>  sequencer.c | 74 
> +++--
>  1 file changed, 28 insertions(+), 46 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index e9baaf59bd..e34334f0ef 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -1869,22 +1869,31 @@ static int count_commands(struct todo_list *todo_list)
>   return count;
>  }
>  
> +static ssize_t strbuf_read_file_or_whine(struct strbuf *sb, const char *path)
> +{
> + int fd;
> + ssize_t len;
> +
> + fd = open(path, O_RDONLY);
> + if (fd < 0)
> + return error_errno(_("could not open '%s'"), path);
> + len = strbuf_read(sb, fd, 0);
> + close(fd);
> + if (len < 0)
> + return error(_("could not read '%s'."), path);
> + return len;
> +}
> +

This looks like a good granularity of a unit of independent work.
The original we see below looks like it was written with scissors
and glue ;-)

It appears to me that no topic in flight introduce more instances
that need to be converted with a quick trial merge to 'pu', so I'll
queue this forked at the tip of 'master'.

Thanks.

>  static int read_populate_todo(struct todo_list *todo_list,
>   struct replay_opts *opts)
>  {
>   struct stat st;
>   const char *todo_file = get_todo_path(opts);
> - int fd, res;
> + int res;
>  
>   strbuf_reset(_list->buf);
> - fd = open(todo_file, O_RDONLY);
> - if (fd < 0)
> - return error_errno(_("could not open '%s'"), todo_file);
> - if (strbuf_read(_list->buf, fd, 0) < 0) {
> - close(fd);
> - return error(_("could not read '%s'."), todo_file);
> - }
> - close(fd);
> + if (strbuf_read_file_or_whine(_list->buf, todo_file) < 0)
> + return -1;
>  
>   res = stat(todo_file, );
>   if (res)
> @@ -3151,20 +3160,13 @@ int check_todo_list(void)
>   struct strbuf todo_file = STRBUF_INIT;
>   struct todo_list todo_list = TODO_LIST_INIT;
>   struct strbuf missing = STRBUF_INIT;
> - int advise_to_edit_todo = 0, res = 0, fd, i;
> + int advise_to_edit_todo = 0, res = 0, i;
>  
>   strbuf_addstr(_file, rebase_path_todo());
> - fd = open(todo_file.buf, O_RDONLY);
> - if (fd < 0) {
> - res = error_errno(_("could not open '%s'"), todo_file.buf);
> - goto leave_check;
> - }
> - if (strbuf_read(_list.buf, fd, 0) < 0) {
> - close(fd);
> - res = error(_("could not read '%s'."), todo_file.buf);
> + if (strbuf_read_file_or_whine(_list.buf, todo_file.buf) < 0) {
> + res = -1;
>   goto leave_check;
>   }
> - close(fd);
>   advise_to_edit_todo = res =
>   parse_insn_buffer(todo_list.buf.buf, _list);
>  
> @@ -3180,17 +3182,10 @@ int check_todo_list(void)
>  
>   todo_list_release(_list);
>   strbuf_addstr(_file, ".backup");
> - fd = open(todo_file.buf, O_RDONLY);
> - if (fd < 0) {
> - res = error_errno(_("could not open '%s'"), todo_file.buf);
> - goto leave_check;
> - }
> - if (strbuf_read(_list.buf, fd, 0) < 0) {
> - close(fd);
> - res = error(_("could not read '%s'."), todo_file.buf);
> + if (strbuf_read_file_or_whine(_list.buf, todo_file.buf) < 0) {
> + res = -1;
>   goto leave_check;
>   }
> - close(fd);
>   strbuf_release(_file);
>   res = !!parse_insn_buffer(todo_list.buf.buf, _list);
>  
> @@ -3271,15 +3266,8 @@ int skip_unnecessary_picks(void)
>   }
>   strbuf_release();
>  
> - fd = open(todo_file, O_RDONLY);
> - if (fd < 0) {
> - return error_errno(_("could not open '%s'"), todo_file);
> - }
> - if (strbuf_read(_list.buf, fd, 0) < 0) {
> - close(fd);
> - return error(_("could not read '%s'."), todo_file);
> - }
> - close(fd);
> + if (strbuf_read_file_or_whine(_list.buf, todo_file) < 0)
> + return -1;
>   if (parse_insn_buffer(todo_list.buf.buf, _list) < 0) {
>   todo_list_release(_list);
>   return -1;
> @@ -3370,17 +3358,11 @@ int rearrange_squash(void)
>   const char *todo_file = rebase_path_todo();
>   struct todo_list todo_list = TODO_LIST_INIT;
>   struct hashmap subject2item;
> - int res = 0, rearranged = 0, *next, *tail, fd, i;
> + int res = 0, rearranged = 0, *next, *tail, i;
>   char **subjects;
>  
> -

removed submodules shown as untracked when switching branches

[Mike Friedrich]

Hi,

When switching clean branches I see untracked files appearing where I expect to 
see "nothing to commit, working tree clean".
This happens when submodules get removed on one branch but its present in 
another.
I expect git to either not mark the submodule in git status as untracked or git 
to remove the submodule as it would for ordinary tracked files which do not 
exist on a branch anymore.

Tested on Windows with: git version 2.15.1.windows.2
Tested on Ubuntu Linux with same output: git version 2.14.1

Test:
git init test
git init submodule
cd submodule
touch file.txt
git add file.txt
git commit -m "test"
cd ../test
touch initial.txt
git add initial.txt
git commit -m "initial"
git checkout -b develop
git status
#On branch develop
#nothing to commit, working tree clean

git submodule add ../submodule sub
git add sub
git commit -m "submodule added"
git status
#On branch develop
#nothing to commit, working tree clean

git checkout master
git status
#On branch master
#Untracked files:
#  (use "git add ..." to include in what will be committed)
#
#sub/
#
#nothing added to commit but untracked files present (use "git add" to 
track)
# expected: nothing to commit, working tree clean

git submodule update
# (no output)
git submodule
# (no output)
git status
#On branch master
#Untracked files:
#  (use "git add ..." to include in what will be committed)
#
#sub/
#
#nothing added to commit but untracked files present (use "git add" to 
track)
# expected: nothing to commit, working tree clean

git clean -dfx
#Skipping repository sub/

Best Regards,
Mike Friedrich




This email is non-binding, is subject to contract, and neither Kulicke and 
Soffa Industries, Inc. nor its subsidiaries (each and collectively “K”) shall 
have any obligation to you to consummate the transactions herein or to enter 
into any agreement, other than in accordance with the terms and conditions of a 
definitive agreement if and when negotiated, finalized and executed between the 
parties. This email and all its contents are protected by International and 
United States copyright laws. Any reproduction or use of all or any part of 
this email without the express written consent of K is prohibited.

Re: [PATCH v3 1/2] Fix misconversion of gitsubmodule.txt

[Junio C Hamano]

Stefan Beller  writes:

> On Thu, Feb 22, 2018 at 12:20 PM, Junio C Hamano  wrote:
>
>>
>> Forgot to say something of your own?  Perhaps wanted to (1) show a
>> sample of a better log message, (2) say "Acked-by", (3) re-raise the
>> point that the same "error" already appears in the same file and it
>> is better to clean them up all at once, or (4) something else?
>
> (2) Acked-by and (4) I-am-sorry-for.

OK, I think the two patches are better squashed into one; in the
larger picture, it really does not matter where the problem
originated from.

Here is what I tentatively queued with copyedited log message.

-- >8 --
From: Motoki Seki 
Date: Thu, 22 Feb 2018 08:52:25 +
Subject: [PATCH] Documentation/gitsubmodules.txt: avoid non-ASCII apostrophes

In gitsubmodules.txt, a few non-ASCII apostrophes are used to spell
possessive, e.g. "submodule's".  These unfortunately are not
rendered at https://git-scm.com/docs/gitsubmodules correctly by the
renderer used there.

Use ASCII apostrophes instead to work around the problem.  It also
is good to be consistent, as there are possessives spelled with
ASCII apostrophes.

Signed-off-by: Motoki Seki 
Signed-off-by: Junio C Hamano 
---
 Documentation/gitsubmodules.txt | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/Documentation/gitsubmodules.txt b/Documentation/gitsubmodules.txt
index 46cf120f66..030c974c25 100644
--- a/Documentation/gitsubmodules.txt
+++ b/Documentation/gitsubmodules.txt
@@ -24,7 +24,7 @@ On the filesystem, a submodule usually (but not always - see 
FORMS below)
 consists of (i) a Git directory located under the `$GIT_DIR/modules/`
 directory of its superproject, (ii) a working directory inside the
 superproject's working directory, and a `.git` file at the root of
-the submodule’s working directory pointing to (i).
+the submodule's working directory pointing to (i).
 
 Assuming the submodule has a Git directory at `$GIT_DIR/modules/foo/`
 and a working directory at `path/to/bar/`, the superproject tracks the
@@ -33,7 +33,7 @@ in its `.gitmodules` file (see linkgit:gitmodules[5]) of the 
form
 `submodule.foo.path = path/to/bar`.
 
 The `gitlink` entry contains the object name of the commit that the
-superproject expects the submodule’s working directory to be at.
+superproject expects the submodule's working directory to be at.
 
 The section `submodule.foo.*` in the `.gitmodules` file gives additional
 hints to Gits porcelain layer such as where to obtain the submodule via
@@ -132,27 +132,27 @@ using older versions of Git.
 +
 It is possible to construct these old form repositories manually.
 +
-When deinitialized or deleted (see below), the submodule’s Git
+When deinitialized or deleted (see below), the submodule's Git
 directory is automatically moved to `$GIT_DIR/modules//`
 of the superproject.
 
  * Deinitialized submodule: A `gitlink`, and a `.gitmodules` entry,
-but no submodule working directory. The submodule’s git directory
+but no submodule working directory. The submodule's git directory
 may be there as after deinitializing the git directory is kept around.
 The directory which is supposed to be the working directory is empty instead.
 +
 A submodule can be deinitialized by running `git submodule deinit`.
 Besides emptying the working directory, this command only modifies
-the superproject’s `$GIT_DIR/config` file, so the superproject’s history
+the superproject's `$GIT_DIR/config` file, so the superproject's history
 is not affected. This can be undone using `git submodule init`.
 
  * Deleted submodule: A submodule can be deleted by running
 `git rm  && git commit`. This can be undone
 using `git revert`.
 +
-The deletion removes the superproject’s tracking data, which are
+The deletion removes the superproject's tracking data, which are
 both the `gitlink` entry and the section in the `.gitmodules` file.
-The submodule’s working directory is removed from the file
+The submodule's working directory is removed from the file
 system, but the Git directory is kept around as it to make it
 possible to checkout past commits without requiring fetching
 from another repository.
-- 
2.16.2-264-ge3a80781f5

Re: [PATCH v3 11/35] test-pkt-line: introduce a packet-line test helper

[Stefan Beller]

On Tue, Feb 6, 2018 at 5:12 PM, Brandon Williams  wrote:

> +static void pack_line(const char *line)
> +{
> +   if (!strcmp(line, "") || !strcmp(line, "\n"))

>From our in-office discussion:
v1/v0 packs pktlines twice in http, which is not possible to
construct using this test helper when using the same string
for the packed and unpacked representation of flush and delim packets,
i.e. test-pkt-line --pack $(test-pkt-line --pack ) would produce
'' instead of '0009\n'.
To fix it we'd have to replace the unpacked versions of these pkts to
something else such as "FLUSH" "DELIM".

However as we do not anticipate the test helper to be used in further
tests for v0, this ought to be no big issue.
Maybe someone else cares though?

Re: [PATCH v3 1/2] Fix misconversion of gitsubmodule.txt

[Stefan Beller]

On Thu, Feb 22, 2018 at 12:20 PM, Junio C Hamano  wrote:

>
> Forgot to say something of your own?  Perhaps wanted to (1) show a
> sample of a better log message, (2) say "Acked-by", (3) re-raise the
> point that the same "error" already appears in the same file and it
> is better to clean them up all at once, or (4) something else?

(2) Acked-by and (4) I-am-sorry-for.

Thanks,
Stefan

Re: [PATCH v3 2/2] Replace non-ASCII apostrophes to ASCII single quotes in gitsubmodules.txt

[Junio C Hamano]

marmot1123  writes:

> Before this patch, there are several non-ASCII apostrophes in
> gitsubmodules.txt, and misconverged at the 
> https://git-scm.com/docs/gitsubmodules/ .
> To make codes consistent, these non-ASCII apostrophes are replaced
> with ASCII single quotes.  This patch also makes the document readable
> on the website.
>
> Signed-off-by: Motoki Seki 
> ---

Interesting.  I didn't know the AsciiDoc renderer used there
mishandled these non-ASCII ticks.  Thanks for writing it down in the
log message.

When we say "there are ..." in a proposed commit log message
(i.e. in the present tense), it is the norm to talk about the
current state of affairs, so "Before this patch," is redundant.  I'd
drop it myself while queuing, and possibly applying other typofixes.

Thanks.


>  Documentation/gitsubmodules.txt | 10 +-
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/gitsubmodules.txt b/Documentation/gitsubmodules.txt
> index 0d59ab4cdfb1c..030c974c25606 100644
> --- a/Documentation/gitsubmodules.txt
> +++ b/Documentation/gitsubmodules.txt
> @@ -132,27 +132,27 @@ using older versions of Git.
>  +
>  It is possible to construct these old form repositories manually.
>  +
> -When deinitialized or deleted (see below), the submodule’s Git
> +When deinitialized or deleted (see below), the submodule's Git
>  directory is automatically moved to `$GIT_DIR/modules//`
>  of the superproject.
>  
>   * Deinitialized submodule: A `gitlink`, and a `.gitmodules` entry,
> -but no submodule working directory. The submodule’s git directory
> +but no submodule working directory. The submodule's git directory
>  may be there as after deinitializing the git directory is kept around.
>  The directory which is supposed to be the working directory is empty instead.
>  +
>  A submodule can be deinitialized by running `git submodule deinit`.
>  Besides emptying the working directory, this command only modifies
> -the superproject’s `$GIT_DIR/config` file, so the superproject’s history
> +the superproject's `$GIT_DIR/config` file, so the superproject's history
>  is not affected. This can be undone using `git submodule init`.
>  
>   * Deleted submodule: A submodule can be deleted by running
>  `git rm  && git commit`. This can be undone
>  using `git revert`.
>  +
> -The deletion removes the superproject’s tracking data, which are
> +The deletion removes the superproject's tracking data, which are
>  both the `gitlink` entry and the section in the `.gitmodules` file.
> -The submodule’s working directory is removed from the file
> +The submodule's working directory is removed from the file
>  system, but the Git directory is kept around as it to make it
>  possible to checkout past commits without requiring fetching
>  from another repository.
>
> --
> https://github.com/git/git/pull/459

Re: [PATCH v3 04/35] upload-pack: convert to a builtin

[Jeff King]

On Thu, Feb 22, 2018 at 03:19:40PM -0500, Jeff King wrote:

> > To be clear, which of the following are you (most) worried about?
> > 
> >  1. being invoked with --help and spawning a pager
> >  2. receiving and acting on options between 'git' and 'upload-pack'
> >  3. repository discovery
> >  4. pager config
> >  5. alias discovery
> >  6. increased code surface / unknown threats
> 
> My immediate concern is (4). But my greater concern is that people who
> work on git.c should not have to worry about accidentally violating this
> principle when they add a new feature or config option.
> 
> In other words, it seems like an accident waiting to happen. I'd be more
> amenable to it if there was some compelling reason for it to be a
> builtin, but I don't see one listed in the commit message. I see only
> "let's make it easier to share the code", which AFAICT is equally served
> by just lib-ifying the code and calling it from the standalone
> upload-pack.c.

By the way, any decision here would presumably need to be extended to
git-serve, etc. The current property is that it's safe to fetch from an
untrusted repository, even over ssh. If we're keeping that for protocol
v1, we'd want it to apply to protocol v2, as well.

-Peff

Re: [PATCH v3 1/2] Fix misconversion of gitsubmodule.txt

[Junio C Hamano]

Stefan Beller  writes:

> On Thu, Feb 22, 2018 at 12:52 AM, marmot1123  wrote:
>> In the 2nd and 4th paragraph of DESCRIPTION, there ware misconversions 
>> `submodule’s`.
>> It seems non-ASCII apostrophes, so I rewrite ASCII apostrophes.
>>
>> Signed-off-by: Motoki Seki 
>> ---
>>  Documentation/gitsubmodules.txt | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/gitsubmodules.txt 
>> b/Documentation/gitsubmodules.txt
>> index 46cf120f666df..0d59ab4cdfb1c 100644
>> --- a/Documentation/gitsubmodules.txt
>> +++ b/Documentation/gitsubmodules.txt
>> @@ -24,7 +24,7 @@ On the filesystem, a submodule usually (but not always - 
>> see FORMS below)
>>  consists of (i) a Git directory located under the `$GIT_DIR/modules/`
>>  directory of its superproject, (ii) a working directory inside the
>>  superproject's working directory, and a `.git` file at the root of
>> -the submodule’s working directory pointing to (i).
>> +the submodule's working directory pointing to (i).
>>
>>  Assuming the submodule has a Git directory at `$GIT_DIR/modules/foo/`
>>  and a working directory at `path/to/bar/`, the superproject tracks the
>> @@ -33,7 +33,7 @@ in its `.gitmodules` file (see linkgit:gitmodules[5]) of 
>> the form
>>  `submodule.foo.path = path/to/bar`.
>>
>>  The `gitlink` entry contains the object name of the commit that the
>> -superproject expects the submodule’s working directory to be at.
>> +superproject expects the submodule's working directory to be at.
>>
>>  The section `submodule.foo.*` in the `.gitmodules` file gives additional
>>  hints to Gits porcelain layer such as where to obtain the submodule via
>>
>> --
>> https://github.com/git/git/pull/459

Forgot to say something of your own?  Perhaps wanted to (1) show a
sample of a better log message, (2) say "Acked-by", (3) re-raise the
point that the same "error" already appears in the same file and it
is better to clean them up all at once, or (4) something else?

Re: [PATCH v3 04/35] upload-pack: convert to a builtin

[Jeff King]

On Thu, Feb 22, 2018 at 11:38:14AM -0800, Jonathan Nieder wrote:

> >>> And possibly respecting pager.upload-pack, which would violate our rule
> >>> that it is safe to run upload-pack in untrusted repositories.
> >>
> >> And this isn't an issue with receive-pack because this same guarantee
> >> doesn't exist?
> >
> > Yes, exactly (which is confusing and weird, yes, but that's how it is).
> 
> To be clear, which of the following are you (most) worried about?
> 
>  1. being invoked with --help and spawning a pager
>  2. receiving and acting on options between 'git' and 'upload-pack'
>  3. repository discovery
>  4. pager config
>  5. alias discovery
>  6. increased code surface / unknown threats

My immediate concern is (4). But my greater concern is that people who
work on git.c should not have to worry about accidentally violating this
principle when they add a new feature or config option.

In other words, it seems like an accident waiting to happen. I'd be more
amenable to it if there was some compelling reason for it to be a
builtin, but I don't see one listed in the commit message. I see only
"let's make it easier to share the code", which AFAICT is equally served
by just lib-ifying the code and calling it from the standalone
upload-pack.c.

> Although in most setups the user does not control the config files on
> a server, item (4) looks like a real issue worth solving.  I think we
> should introduce a flag to skip looking for pager config.  We could
> use it for receive-pack, too.

There's not much point for receive-pack. It respects hooks, so any
security ship has already sailed there.

-Peff

Re: [PATCH] t: send verbose test-helper output to fd 4

[Junio C Hamano]

Jeff King  writes:

> This is a repost of the two patches from:
>
>   https://public-inbox.org/git/20180209185710.ga23...@sigill.intra.peff.net/
>
> (now just one patch, since sg/test-i18ngrep graduated and we can do it
> all in one step). The idea got positive feedback, but nobody commented
> on patches and I didn't see them in "What's cooking".

Thanks for clearly explaining the change.  Will queue.

Re: bug in HTTP protocol spec

[Dorian Taylor]

> On Feb 22, 2018, at 12:02 PM, Junio C Hamano  wrote:
> 
> I saw somewhere "Apple-Mail" and a phrase "repaste".  So perhaps
> copy on the client is involved in the whitespace damage (of
> course the original could be broken, but I somehow doubt it).

https://doriantaylor.com/file/well-ill-be-damned.png

> For what it's worth, I am slightly negative on this addition.  It
> makes it inconsistent if we only mention it about _this_ flush and
> not any other flush.  It even gets in the way of learning the
> protocol by new people reading it, by giving an impression that
> somehow LF is outside and in between packet line.
> 
> Thanks.

This patch exists because I was asked to write it. I don’t know squat about 
this protocol other than the fact that I followed the spec and it didn’t work. 
I traced a known-good protocol endpoint and found it contained content that 
didn’t agree with the spec. I then obliged the request to submit a patch with 
*what I knew to be true* about the sample that actually worked. I then followed 
the recommendations *advertised on GitHub* for submitting the patch.

You’re welcome.

--
Dorian Taylor
Make things. Make sense.
https://doriantaylor.com



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH v7 0/7] convert: add support for different encodings

[Jeff King]

On Thu, Feb 22, 2018 at 09:00:45PM +0100, Lars Schneider wrote:

> > If it was only about a diff of UTF-16 files, I may suggest a patch.
> > I simply copy-paste it here for review, if someone thinks that it may
> > be useful, I can send it as a real patch/RFC.
> 
> That's a nice idea but I see two potential problems:
> 
> (1) Git hosting services (GitLab, BitBucket, GitHub, ...) would still
> show these files as binary. That's a huge problem for my users as
> they interact more with these services than the Git command line.
> That's the main reason why I implemented the "UTF-8 as canonical
> form" approach in my series.

I can't speak for the other services, but I can tell you that GitHub
would be pretty eager to enable such a feature if it existed.

I suspect most services providing human-readable diffs would want to do
the same. Though there are still cases where you'd see a binary patch
(e.g., format-patch in emails, or GitHub's .patch endpoint, since those
are meant to be applied and must contain the "real" data).

-Peff

Re: [PATCH v5 00/17] document & test fetch pruning & add fetch.pruneTags

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  writes:

> On Thu, Feb 22 2018, Junio C. Hamano jotted:
>
>> Ævar Arnfjörð Bjarmason   writes:
>>
>>> Here's a v5 (correct subject line this time!). Many thanks to Eric for
>>> a thorough review.
>>
>> We haven't seen any comments on this round.  Is everybody happy?
>>
>> I do not have a strong opinion on the new feature, either for or
>> against.  I didn't find anything majorly questionable in the
>> execution, though, so...
>
> I've been running that here on thousands of boxes (that are actively
> using it) for 2 weeks now without issue. Would be great to have it
> merged down & in 2.17.

If those thousands of boxes are all employing one specific workflow
that is helped by these changes, and the workflow is that other
people do not care about (or even worse, actively do not want to let
their junior project members to use without thinking), then a
data-point from the original author does not amount to much ;-)

Let's see how others find it useful and/or if the changed code gets
in the way of others (I am not absolutely sure if the changes are
free of regression to existing users who do not use the new
feature).

Thanks.

Re: [PATCH v3 07/35] connect: convert get_remote_heads to use struct packet_reader

[Stefan Beller]

> +static enum protocol_version discover_version(struct packet_reader *reader)
> +{
...
> +
> +   /* Maybe process capabilities here, at least for v2 */
> +   switch (version) {
> +   case protocol_v1:
> +   /* Read the peeked version line */
> +   packet_reader_read(reader);
> +   break;
> +   case protocol_v0:
> +   break;
> +   case protocol_unknown_version:
> +   die("unknown protocol version: '%s'\n", reader->line);

The following patches introduce more of the switch(version) cases.
And there it actually is a
BUG("protocol version unknown? should have been set in discover_version")
but here it is a mere
  die (_("The server uses a different protocol version than we can
speak: %s\n"),
  reader->line);
so I would think here it is reasonable to add _(translation).

Re: bug in HTTP protocol spec

[Junio C Hamano]

Jeff King  writes:

> This indentation is funny. But I suspect it is because your whole patch
> seems to have been whitespace-damaged (see the section on gmail in
> "git help git-format-patch").

I saw somewhere "Apple-Mail" and a phrase "repaste".  So perhaps
copy on the client is involved in the whitespace damage (of
course the original could be broken, but I somehow doubt it).

>> The client may send Extra Parameters (see
>> Documentation/technical/pack-protocol.txt) as a colon-separated string
>> -in the Git-Protocol HTTP header.
>> +in the Git-Protocol HTTP header. Note as well that there is *no* newline
>> +after the ``.
>
> I guess I'm not opposed to calling that out, but this is normal for
> pktline (the flush packet has no data; in the other lines the newline is
> not a syntactic part of the pktline stream, but is actually data
> contained inside each of those pktlines).

For what it's worth, I am slightly negative on this addition.  It
makes it inconsistent if we only mention it about _this_ flush and
not any other flush.  It even gets in the way of learning the
protocol by new people reading it, by giving an impression that
somehow LF is outside and in between packet line.

Thanks.

Re: [PATCH v7 0/7] convert: add support for different encodings

[Lars Schneider]

> On 16 Feb 2018, at 17:58, Torsten Bögershausen  wrote:
> 
> On Fri, Feb 16, 2018 at 03:42:35PM +0100, Lars Schneider wrote:
> []
>> 
>> Agreed. However, people using ShiftJIS are not my target audience.
>> My target audience are:
>> 
>> (1) People that have to encode their text files in UTF-16 (for 
>>whatever reason - usually because of legacy processes or tools).
>> 
>> (2) People that want to see textual diffs of their UTF-16 encoded 
>>files in their Git tools without special adjustments (on the 
>>command line, on the web, etc).
>> 
>> That was my primary motivation. The fact that w-t-e supports any
>> other encoding too is just a nice side effect. I don't foresee people
>> using other w-t-encodings other than UTF-16 in my organization.
>> 
>> I have the suspicion that the feature could be useful for the Git
>> community at large. Consider this Stack Overflow question:
>> https://stackoverflow.com/questions/777949/can-i-make-git-recognize-a-utf-16-file-as-text
>> 
>> This question was viewed 42k times and there is no good solution.
>> I believe w-t-e could be a good solution.
>> 
> 
> If it was only about a diff of UTF-16 files, I may suggest a patch.
> I simply copy-paste it here for review, if someone thinks that it may
> be useful, I can send it as a real patch/RFC.

That's a nice idea but I see two potential problems:

(1) Git hosting services (GitLab, BitBucket, GitHub, ...) would still
show these files as binary. That's a huge problem for my users as
they interact more with these services than the Git command line.
That's the main reason why I implemented the "UTF-8 as canonical
form" approach in my series.

(2) You can only detect a BOM if the encoding is UTF-16. UTF-16BE and
UTF-16LE must not have a BOM and therefore cannot be easily
detected. Plus, even if you detect an UTF-16 BOM then it would be 
just a hint that the file is likely UTF-16 encoded as the sequence
could be there by chance. 

I still think it would be nice to see diffs for arbitrary encodings.
Would it be an option to read the `encoding` attribute and use it in
`git diff`?

- Lars


> 
> git show HEAD
> 
> 
> commit 9f7d43f29eaf6017b7b16261ce91d8ef182cf415
> Author: Torsten Bögershausen 
> Date:   Fri Feb 2 15:35:23 2018 +0100
> 
>Auto diff of UTF-16 files in UTF-8
> 
>When an UTF-16 file is commited and later changed, `git diff` shows
>"Binary files XX and YY differ".
> 
>When the user wants a diff in UTF-8, a textconv needs to be specified
>in .gitattributes and the textconv must be configured.
> 
>A more user-friendly diff can be produced for UTF-16 if
>- the user did not use `git diff --binary`
>- the blob is identified as binary
>- the blob has an UTF-16 BOM
>- the blob can be converted into UTF-8
> 
>Enhance the diff machinery to auto-detect UTF-16 blobs and show them
>as UTF-8, unless the user specifies `git diff --binary` which creates
>a binary diff.
> 
> diff --git a/diff.c b/diff.c
> index fb22b19f09..51831ee94d 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -3192,6 +3192,10 @@ static void builtin_diff(const char *name_a,
>   strbuf_reset();
>   }
> 
> + if (one && one->reencoded_from_utf16)
> + strbuf_addf(, "a is converted to UTF-8 from 
> UTF-16\n");
> + if (two && two->reencoded_from_utf16)
> + strbuf_addf(, "b is converted to UTF-8 from 
> UTF-16\n");
>   mf1.size = fill_textconv(textconv_one, one, );
>   mf2.size = fill_textconv(textconv_two, two, );
> 
> @@ -3611,8 +3615,25 @@ int diff_populate_filespec(struct diff_filespec *s, 
> unsigned int flags)
>   s->size = size;
>   s->should_free = 1;
>   }
> - }
> - else {
> + if (!s->binary && buffer_is_binary(s->data, s->size) &&
> + buffer_has_utf16_bom(s->data, s->size)) {
> + int outsz = 0;
> + char *outbuf;
> + outbuf = reencode_string_len(s->data, (int)s->size,
> +  "UTF-8", "UTF-16", );
> + if (outbuf) {
> + if (s->should_free)
> + free(s->data);
> + if (s->should_munmap)
> + munmap(s->data, s->size);
> + s->should_munmap = 0;
> + s->data = outbuf;
> + s->size = outsz;
> + s->reencoded_from_utf16 = 1;
> + s->should_free = 1;
> + }
> + }
> + } else {
>   enum object_type type;
>   if (size_only || (flags & CHECK_BINARY)) {
>   type = sha1_object_info(s->oid.hash, 

Re: [PATCH v3 07/35] connect: convert get_remote_heads to use struct packet_reader

[Stefan Beller]

On Tue, Feb 6, 2018 at 5:12 PM, Brandon Williams  wrote:

> @@ -56,6 +62,41 @@ static void die_initial_contact(int unexpected)
>   "and the repository exists."));
>  }
>
> +static enum protocol_version discover_version(struct packet_reader *reader)
> +{
> +   enum protocol_version version = protocol_unknown_version;
> +
> +   /*
> +* Peek the first line of the server's response to
> +* determine the protocol version the server is speaking.
> +*/
> +   switch (packet_reader_peek(reader)) {
> +   case PACKET_READ_EOF:
> +   die_initial_contact(0);
> +   case PACKET_READ_FLUSH:
> +   case PACKET_READ_DELIM:
> +   version = protocol_v0;
> +   break;
> +   case PACKET_READ_NORMAL:
> +   version = determine_protocol_version_client(reader->line);
> +   break;
> +   }
> +
> +   /* Maybe process capabilities here, at least for v2 */

We do not (yet) react to v2, so this comment only makes
sense after a later patch? If so please include it later,
as this is confusing for now.


> +   switch (version) {
> +   case protocol_v1:
> +   /* Read the peeked version line */
> +   packet_reader_read(reader);

I wonder if we want to assign version to v0 here,
as now all v1 is done and we could treat the remaining
communication as a v0. Not sure if that helps with some
switch/cases, but as we'd give all cases to have the compiler
not yell at us, this would be no big deal. So I guess we can keep
it v1.

With or without the comment nit, this patch is
Reviewed-by: Stefan Beller 

Re: [PATCH v3 16/35] transport: convert transport_get_remote_refs to take a list of ref patterns

[Brandon Williams]

On 02/22, Jonathan Tan wrote:
> On Thu, 22 Feb 2018 10:26:47 -0800
> Brandon Williams  wrote:
> 
> > On 02/21, Jonathan Tan wrote:
> > > On Tue,  6 Feb 2018 17:12:53 -0800
> > > Brandon Williams  wrote:
> > > 
> > > > -const struct ref *transport_get_remote_refs(struct transport 
> > > > *transport)
> > > > +const struct ref *transport_get_remote_refs(struct transport 
> > > > *transport,
> > > > +   const struct argv_array 
> > > > *ref_patterns)
> > > >  {
> > > > if (!transport->got_remote_refs) {
> > > > -   transport->remote_refs = 
> > > > transport->vtable->get_refs_list(transport, 0, NULL);
> > > > +   transport->remote_refs =
> > > > +   transport->vtable->get_refs_list(transport, 0,
> > > > +ref_patterns);
> > > > transport->got_remote_refs = 1;
> > > > }
> > > 
> > > Should we do our own client-side filtering if the server side cannot do
> > > it for us (because it doesn't support protocol v2)? Either way, this
> > > decision should be mentioned in the commit message.
> > 
> > If someone wants to add this in the future they can, but that is outside
> > the scope of this series.
> 
> In that case, also document that this function is allowed to return refs
> that do not match the ref patterns.
> 
> Unlike in patch 15 (which deals with the interface between the transport
> code and transport vtables, which can be changed as long as the
> transport code is aware of it, as I wrote in [1]), this may result in
> user-visible differences depending on which protocol is used. But after
> more thinking, I don't think we're in a situation yet where having extra
> refs shown/written are harmful, and if it comes to that, we can tighten
> this code later without backwards incompatibility. So, OK, this is fine.
> 
> [1] 
> https://public-inbox.org/git/20180221145639.c6cf2409ce2120109bdd1...@google.com/

I'll add the documentation.

-- 
Brandon Williams

Re: [PATCH v3 05/35] upload-pack: factor out processing lines

[Brandon Williams]

On 02/22, Stefan Beller wrote:
> On Tue, Feb 6, 2018 at 5:12 PM, Brandon Williams  wrote:
> > Factor out the logic for processing shallow, deepen, deepen_since, and
> > deepen_not lines into their own functions to simplify the
> > 'receive_needs()' function in addition to making it easier to reuse some
> > of this logic when implementing protocol_v2.
> >
> > Signed-off-by: Brandon Williams 
> 
> Reviewed-by: Stefan Beller 
> for the stated purpose of just refactoring existing code for better reuse 
> later.
> 
> I do have a few comments on the code in general,
> which might be out of scope for this series.

Yeah you mentioned some comments in a previous round based on style
preference.  I'm going to refrain from changing the style of this patch
since it is a matter of preference.

> 
> A close review would have been fastest if we had some sort of
> https://public-inbox.org/git/20171025224620.27657-1-sbel...@google.com/
> which I might revive soon for this purpose. (it showed that I would need it)
> 
> 
> > +   *depth = (int)strtol(arg, , 0);
> 
> strtol is not used quite correctly here IMHO, as we do not
> inspect errno for ERANGE
> 
> Thanks,
> Stefan

-- 
Brandon Williams

Re: [PATCH v3 04/35] upload-pack: convert to a builtin

[Jonathan Nieder]

Hi,

Jeff King wrote:
> On Thu, Feb 22, 2018 at 10:07:15AM -0800, Brandon Williams wrote:
>> On 02/22, Jeff King wrote:
>>> On Wed, Feb 21, 2018 at 01:44:22PM -0800, Jonathan Tan wrote:
 On Tue,  6 Feb 2018 17:12:41 -0800
 Brandon Williams  wrote:

> In order to allow for code sharing with the server-side of fetch in
> protocol-v2 convert upload-pack to be a builtin.
[...]
 As Stefan mentioned in [1], also mention in the commit message that this
 means that the "git-upload-pack" invocation gains additional
 capabilities (for example, invoking a pager for --help).
>>>
>>> And possibly respecting pager.upload-pack, which would violate our rule
>>> that it is safe to run upload-pack in untrusted repositories.
>>
>> And this isn't an issue with receive-pack because this same guarantee
>> doesn't exist?
>
> Yes, exactly (which is confusing and weird, yes, but that's how it is).

To be clear, which of the following are you (most) worried about?

 1. being invoked with --help and spawning a pager
 2. receiving and acting on options between 'git' and 'upload-pack'
 3. repository discovery
 4. pager config
 5. alias discovery
 6. increased code surface / unknown threats

For (1), "--help" has to be the first argument.  "git daemon" passes
--strict so it doesn't happen there.  "git http-backend" passes
--stateless-rpc so it doesn't happen there.  "git shell" sanitizes
input to avoid it happening there.

A custom setup could provide their own entry point that doesn't do
such sanitization.  I think that in some sense it's out of our hands,
but it would be nice to harden as described upthread.

For (2), I am having trouble imagining a setup where it would happen.

upload-pack doesn't have the RUN_SETUP or RUN_SETUP_GENTLY flag set,
so (3) doesn't apply.

Although in most setups the user does not control the config files on
a server, item (4) looks like a real issue worth solving.  I think we
should introduce a flag to skip looking for pager config.  We could
use it for receive-pack, too.

Builtins are handled before aliases, so (5) doesn't apply.

(6) is a real issue: it is why "git shell" is not a builtin, for
example.  But I would rather that we use appropriate sanitization
before upload-pack is invoked than live in fear.  git upload-pack is
sufficiently complicated that I don't think the git.c wrapper
increases the complexity by a significant amount.

That leaves me with a personal answer of only being worried about (4)
and not the rest.  What do you think?  Is one of the other items I
listed above worrisome, or is there another item I missed?

Thanks,
Jonathan

Re: [PATCH v3 03/35] pkt-line: add delim packet support

[Brandon Williams]

On 02/22, Stefan Beller wrote:
> On Tue, Feb 6, 2018 at 5:12 PM, Brandon Williams  wrote:
> > One of the design goals of protocol-v2 is to improve the semantics of
> > flush packets.  Currently in protocol-v1, flush packets are used both to
> > indicate a break in a list of packet lines as well as an indication that
> > one side has finished speaking.  This makes it particularly difficult
> > to implement proxies as a proxy would need to completely understand git
> > protocol instead of simply looking for a flush packet.
> >
> > To do this, introduce the special deliminator packet '0001'.  A delim
> > packet can then be used as a deliminator between lists of packet lines
> > while flush packets can be reserved to indicate the end of a response.
> 
> Please mention where this can be found in the documentation.
> (Defer to later patch?)

Yeah the documentation does get added in a future patch, I'll make a
comment to that effect.

> As the commit message states, this is only to be used for v2,
> in v0 it is still an illegal pkt.
> 
> >
> > Signed-off-by: Brandon Williams 
> 
> The code is
> Reviewed-by: Stefan Beller 

-- 
Brandon Williams

Re: [PATCH v3 16/35] transport: convert transport_get_remote_refs to take a list of ref patterns

[Jonathan Tan]

On Thu, 22 Feb 2018 10:26:47 -0800
Brandon Williams  wrote:

> On 02/21, Jonathan Tan wrote:
> > On Tue,  6 Feb 2018 17:12:53 -0800
> > Brandon Williams  wrote:
> > 
> > > -const struct ref *transport_get_remote_refs(struct transport *transport)
> > > +const struct ref *transport_get_remote_refs(struct transport *transport,
> > > + const struct argv_array 
> > > *ref_patterns)
> > >  {
> > >   if (!transport->got_remote_refs) {
> > > - transport->remote_refs = 
> > > transport->vtable->get_refs_list(transport, 0, NULL);
> > > + transport->remote_refs =
> > > + transport->vtable->get_refs_list(transport, 0,
> > > +  ref_patterns);
> > >   transport->got_remote_refs = 1;
> > >   }
> > 
> > Should we do our own client-side filtering if the server side cannot do
> > it for us (because it doesn't support protocol v2)? Either way, this
> > decision should be mentioned in the commit message.
> 
> If someone wants to add this in the future they can, but that is outside
> the scope of this series.

In that case, also document that this function is allowed to return refs
that do not match the ref patterns.

Unlike in patch 15 (which deals with the interface between the transport
code and transport vtables, which can be changed as long as the
transport code is aware of it, as I wrote in [1]), this may result in
user-visible differences depending on which protocol is used. But after
more thinking, I don't think we're in a situation yet where having extra
refs shown/written are harmful, and if it comes to that, we can tighten
this code later without backwards incompatibility. So, OK, this is fine.

[1] 
https://public-inbox.org/git/20180221145639.c6cf2409ce2120109bdd1...@google.com/

Re: [PATCH v3 05/35] upload-pack: factor out processing lines

[Stefan Beller]

On Tue, Feb 6, 2018 at 5:12 PM, Brandon Williams  wrote:
> Factor out the logic for processing shallow, deepen, deepen_since, and
> deepen_not lines into their own functions to simplify the
> 'receive_needs()' function in addition to making it easier to reuse some
> of this logic when implementing protocol_v2.
>
> Signed-off-by: Brandon Williams 

Reviewed-by: Stefan Beller 
for the stated purpose of just refactoring existing code for better reuse later.

I do have a few comments on the code in general,
which might be out of scope for this series.

A close review would have been fastest if we had some sort of
https://public-inbox.org/git/20171025224620.27657-1-sbel...@google.com/
which I might revive soon for this purpose. (it showed that I would need it)


> +   *depth = (int)strtol(arg, , 0);

strtol is not used quite correctly here IMHO, as we do not
inspect errno for ERANGE

Thanks,
Stefan

Re: Bug: git log: boundary commits do not respect order (e.g. date-order, topo-order) 2

[Derrick Stolee]

On 2/21/2018 6:57 PM, Josh Tepper wrote:

When using git log, boundary commits (ie, those commits added by
specifying --boundary) do not respect the order (e.g., --date-order,
--topo-order).  Consider the following commit history, where number
indicates the order of the commit timestamps:


0125  <--A
\ \
  346  <--B


Executing the following command:

$ git log --boundary --date-order ^A B

Should produce the following order (boundary commits shown with dashes):
6 -5 4 3 -1

However, it in fact produces:
6 4 3 -5 -1

Please advise.



Hi Josh,

Looking at the docs [1], I don't see any specifics on how the boundary 
commits should be ordered.


Clearly, the implementation specifies that the boundary is written after 
all other commits. For a full discussion of this, see the commit message 
for 86ab4906a7c "revision walker: Fix --boundary when limited". Here is 
an excerpt:


 - After get_revision() finishes giving out all the positive
   commits, if we are doing the boundary processing, we look at
   the parents that we marked as potential boundaries earlier,
   see if they are really boundaries, and give them out.

The boundary commits are correctly sorted by topo-order among themselves 
as of commit 4603ec0f960 "get_revision(): honor the topo_order flag for 
boundary commits".


So, I'm not sure that this is a bug (it is working "as designed") but it 
certainly is non-obvious behavior.


In what use case do you need these boundary commits to appear earlier?

Thanks,
-Stolee

[1] https://git-scm.com/docs/git-log

[PATCH] sequencer: factor out strbuf_read_file_or_whine()

[René Scharfe]

Reduce code duplication by factoring out a function that reads an entire
file into a strbuf, or reports errors on stderr if something goes wrong.

Signed-off-by: Rene Scharfe 
---
The difference to using strbuf_read_file() is more detailed error
messages for open(2) failures.  But I don't know if we need them -- or
under which circumstances reading todo files could fail anyway.  When
doing multiple rebases in parallel perhaps?

 sequencer.c | 74 +++--
 1 file changed, 28 insertions(+), 46 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index e9baaf59bd..e34334f0ef 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1869,22 +1869,31 @@ static int count_commands(struct todo_list *todo_list)
return count;
 }
 
+static ssize_t strbuf_read_file_or_whine(struct strbuf *sb, const char *path)
+{
+   int fd;
+   ssize_t len;
+
+   fd = open(path, O_RDONLY);
+   if (fd < 0)
+   return error_errno(_("could not open '%s'"), path);
+   len = strbuf_read(sb, fd, 0);
+   close(fd);
+   if (len < 0)
+   return error(_("could not read '%s'."), path);
+   return len;
+}
+
 static int read_populate_todo(struct todo_list *todo_list,
struct replay_opts *opts)
 {
struct stat st;
const char *todo_file = get_todo_path(opts);
-   int fd, res;
+   int res;
 
strbuf_reset(_list->buf);
-   fd = open(todo_file, O_RDONLY);
-   if (fd < 0)
-   return error_errno(_("could not open '%s'"), todo_file);
-   if (strbuf_read(_list->buf, fd, 0) < 0) {
-   close(fd);
-   return error(_("could not read '%s'."), todo_file);
-   }
-   close(fd);
+   if (strbuf_read_file_or_whine(_list->buf, todo_file) < 0)
+   return -1;
 
res = stat(todo_file, );
if (res)
@@ -3151,20 +3160,13 @@ int check_todo_list(void)
struct strbuf todo_file = STRBUF_INIT;
struct todo_list todo_list = TODO_LIST_INIT;
struct strbuf missing = STRBUF_INIT;
-   int advise_to_edit_todo = 0, res = 0, fd, i;
+   int advise_to_edit_todo = 0, res = 0, i;
 
strbuf_addstr(_file, rebase_path_todo());
-   fd = open(todo_file.buf, O_RDONLY);
-   if (fd < 0) {
-   res = error_errno(_("could not open '%s'"), todo_file.buf);
-   goto leave_check;
-   }
-   if (strbuf_read(_list.buf, fd, 0) < 0) {
-   close(fd);
-   res = error(_("could not read '%s'."), todo_file.buf);
+   if (strbuf_read_file_or_whine(_list.buf, todo_file.buf) < 0) {
+   res = -1;
goto leave_check;
}
-   close(fd);
advise_to_edit_todo = res =
parse_insn_buffer(todo_list.buf.buf, _list);
 
@@ -3180,17 +3182,10 @@ int check_todo_list(void)
 
todo_list_release(_list);
strbuf_addstr(_file, ".backup");
-   fd = open(todo_file.buf, O_RDONLY);
-   if (fd < 0) {
-   res = error_errno(_("could not open '%s'"), todo_file.buf);
-   goto leave_check;
-   }
-   if (strbuf_read(_list.buf, fd, 0) < 0) {
-   close(fd);
-   res = error(_("could not read '%s'."), todo_file.buf);
+   if (strbuf_read_file_or_whine(_list.buf, todo_file.buf) < 0) {
+   res = -1;
goto leave_check;
}
-   close(fd);
strbuf_release(_file);
res = !!parse_insn_buffer(todo_list.buf.buf, _list);
 
@@ -3271,15 +3266,8 @@ int skip_unnecessary_picks(void)
}
strbuf_release();
 
-   fd = open(todo_file, O_RDONLY);
-   if (fd < 0) {
-   return error_errno(_("could not open '%s'"), todo_file);
-   }
-   if (strbuf_read(_list.buf, fd, 0) < 0) {
-   close(fd);
-   return error(_("could not read '%s'."), todo_file);
-   }
-   close(fd);
+   if (strbuf_read_file_or_whine(_list.buf, todo_file) < 0)
+   return -1;
if (parse_insn_buffer(todo_list.buf.buf, _list) < 0) {
todo_list_release(_list);
return -1;
@@ -3370,17 +3358,11 @@ int rearrange_squash(void)
const char *todo_file = rebase_path_todo();
struct todo_list todo_list = TODO_LIST_INIT;
struct hashmap subject2item;
-   int res = 0, rearranged = 0, *next, *tail, fd, i;
+   int res = 0, rearranged = 0, *next, *tail, i;
char **subjects;
 
-   fd = open(todo_file, O_RDONLY);
-   if (fd < 0)
-   return error_errno(_("could not open '%s'"), todo_file);
-   if (strbuf_read(_list.buf, fd, 0) < 0) {
-   close(fd);
-   return error(_("could not read '%s'."), todo_file);
-   }
-   close(fd);
+   if (strbuf_read_file_or_whine(_list.buf, todo_file) < 0)
+   return -1;
if (parse_insn_buffer(todo_list.buf.buf, _list) 

Re: [PATCH v3 14/35] connect: request remote refs using v2

[Jonathan Tan]

On Thu, 22 Feb 2018 13:26:58 -0500
Jeff King  wrote:

> On Thu, Feb 22, 2018 at 10:19:22AM -0800, Brandon Williams wrote:
> 
> > On 02/21, Jonathan Tan wrote:
> > > On Tue,  6 Feb 2018 17:12:51 -0800
> > > Brandon Williams  wrote:
> > > 
> > > > +extern struct ref **get_remote_refs(int fd_out, struct packet_reader 
> > > > *reader,
> > > > +   struct ref **list, int for_push,
> > > > +   const struct argv_array 
> > > > *ref_patterns);
> > > 
> > > I haven't looked at the rest of this patch in detail, but the type of
> > > ref_patterns is probably better as struct string_list, since this is not
> > > a true argument array (e.g. with flags starting with --). Same comment
> > > for the next few patches that deal with ref patterns.
> > 
> > Its just a list of strings which don't require having a util pointer
> > hanging around so actually using an argv_array would be more memory
> > efficient than a string_list.  But either way I don't think it matters
> > much.
> 
> I agree that it shouldn't matter much here. But if the name argv_array
> is standing in the way of using it, I think we should consider giving it
> a more general name. I picked that not to evoke "this must be arguments"
> but "this is terminated by a single NULL".
> 
> In general I think it should be the preferred structure for string
> lists, just because it actually converts for free to the "other" common
> format (whereas you can never pass string_list.items to a function that
> doesn't know about string lists).

This sounds reasonable - I withdraw my comment about using struct
string_list.

Re: [PATCH v3 08/35] connect: discover protocol version outside of get_remote_heads

[Jonathan Tan]

On Thu, 22 Feb 2018 10:17:39 -0800
Brandon Williams  wrote:

> > > diff --git a/remote.h b/remote.h
> > > index 1f6611be2..2016461df 100644
> > > --- a/remote.h
> > > +++ b/remote.h
> > > @@ -150,10 +150,11 @@ int check_ref_type(const struct ref *ref, int 
> > > flags);
> > >  void free_refs(struct ref *ref);
> > >  
> > >  struct oid_array;
> > > -extern struct ref **get_remote_heads(int in, char *src_buf, size_t 
> > > src_len,
> > > +struct packet_reader;
> > > +extern struct ref **get_remote_heads(struct packet_reader *reader,
> > >struct ref **list, unsigned int flags,
> > >struct oid_array *extra_have,
> > > -  struct oid_array *shallow);
> > > +  struct oid_array *shallow_points);
> > 
> > This change probably does not belong in this patch, especially since
> > remote.c is unchanged.
> 
> Yes this hunk is needed, the signature of get_remote_heads changes.  It
> may be difficult to see that due to the fact that we don't really have a
> clear story on how header files are divided up within the project.

Thanks - I indeed didn't notice that the implementation of
get_remote_heads() is modified too in this patch.

My initial comment was about just the renaming of "shallow" to
"shallow_points", but yes, you're right - I see in the implementation
that it is indeed named "shallow_points" there, so this change is
justified, especially since you're already changing the signature of
this function.

Re: [PATCH v3 03/35] pkt-line: add delim packet support

[Stefan Beller]

On Tue, Feb 6, 2018 at 5:12 PM, Brandon Williams  wrote:
> One of the design goals of protocol-v2 is to improve the semantics of
> flush packets.  Currently in protocol-v1, flush packets are used both to
> indicate a break in a list of packet lines as well as an indication that
> one side has finished speaking.  This makes it particularly difficult
> to implement proxies as a proxy would need to completely understand git
> protocol instead of simply looking for a flush packet.
>
> To do this, introduce the special deliminator packet '0001'.  A delim
> packet can then be used as a deliminator between lists of packet lines
> while flush packets can be reserved to indicate the end of a response.

Please mention where this can be found in the documentation.
(Defer to later patch?)
As the commit message states, this is only to be used for v2,
in v0 it is still an illegal pkt.

>
> Signed-off-by: Brandon Williams 

The code is
Reviewed-by: Stefan Beller 

Re: [PATCH v3 35/35] remote-curl: don't request v2 when pushing

[Brandon Williams]

On 02/22, Brandon Williams wrote:
> On 02/21, Jonathan Tan wrote:
> > On Tue,  6 Feb 2018 17:13:12 -0800
> > Brandon Williams  wrote:
> > 
> > > +test_expect_success 'push with http:// and a config of v2 does not 
> > > request v2' '
> > > + # Till v2 for push is designed, make sure that if a client has
> > > + # protocol.version configured to use v2, that the client instead falls
> > > + # back and uses v0.
> > > +
> > > + test_commit -C http_child three &&
> > > +
> > > + # Push to another branch, as the target repository has the
> > > + # master branch checked out and we cannot push into it.
> > > + GIT_TRACE_PACKET=1 git -C http_child -c protocol.version=1 \
> > > + push origin HEAD:client_branch && 2>log &&
> > 
> > Should it be protocol.version=2? Also, two double ampersands?
> > 
> > Also, optionally, it might be better to do
> > GIT_TRACE_PACKET="$(pwd)/log", so that it does not get mixed with other
> > stderr output.
> 
> Wow thanks for catching that, let me fix that.

I like setting the log via "$(pwd)/log" but it turns out that this
appends to the file if it already exists, which means the previous tests
need to do some cleanup.  This is actually probably preferable anyway.

> 
> -- 
> Brandon Williams

-- 
Brandon Williams

Your Consent

[Mr.Lee]

Important details to share with you, kindly email me for info: 
"peter.waddell...@gmail.com" Peter

Re: [PATCH v3 35/35] remote-curl: don't request v2 when pushing

[Brandon Williams]

On 02/21, Jonathan Tan wrote:
> On Tue,  6 Feb 2018 17:13:12 -0800
> Brandon Williams  wrote:
> 
> > +test_expect_success 'push with http:// and a config of v2 does not request 
> > v2' '
> > +   # Till v2 for push is designed, make sure that if a client has
> > +   # protocol.version configured to use v2, that the client instead falls
> > +   # back and uses v0.
> > +
> > +   test_commit -C http_child three &&
> > +
> > +   # Push to another branch, as the target repository has the
> > +   # master branch checked out and we cannot push into it.
> > +   GIT_TRACE_PACKET=1 git -C http_child -c protocol.version=1 \
> > +   push origin HEAD:client_branch && 2>log &&
> 
> Should it be protocol.version=2? Also, two double ampersands?
> 
> Also, optionally, it might be better to do
> GIT_TRACE_PACKET="$(pwd)/log", so that it does not get mixed with other
> stderr output.

Wow thanks for catching that, let me fix that.

-- 
Brandon Williams

Re: [PATCH v3 32/35] http: allow providing extra headers for http requests

[Brandon Williams]

On 02/21, Jonathan Tan wrote:
> On Tue,  6 Feb 2018 17:13:09 -0800
> Brandon Williams  wrote:
> 
> > @@ -172,6 +172,8 @@ struct http_get_options {
> >  * for details.
> >  */
> > struct strbuf *base_url;
> > +
> > +   struct string_list *extra_headers;
> 
> Document this? For example:
> 
>   If not NULL, additional HTTP headers to be sent with the request. The
>   strings in the list must not be freed until after the request.

I'll add that.

-- 
Brandon Williams

Re: [PATCH v3 30/35] remote-curl: create copy of the service name

[Brandon Williams]

On 02/21, Jonathan Tan wrote:
> On Tue,  6 Feb 2018 17:13:07 -0800
> Brandon Williams  wrote:
> 
> > Make a copy of the service name being requested instead of relying on
> > the buffer pointed to by the passed in 'const char *' to remain
> > unchanged.
> > 
> > Signed-off-by: Brandon Williams 
> 
> Probably worth mentioning in the commit message:
> 
>   Currently, all service names are string constants, but a subsequent
>   patch will introduce service names from external sources.
> 
> Other than that,
> 
> Reviewed-by: Jonathan Tan 

I'll add that.

-- 
Brandon Williams

Re: [PATCH v3 20/35] upload-pack: introduce fetch server command

[Brandon Williams]

On 02/21, Jonathan Tan wrote:
> On Tue,  6 Feb 2018 17:12:57 -0800
> Brandon Williams  wrote:
> 
> > +want 
> > +   Indicates to the server an object which the client wants to
> > +   retrieve.
> 
> Mention that the client can "want" anything even if not advertised by
> the server (like uploadpack.allowanysha1inwant).

Will do.

> 
> > +output = *section
> > +section = (acknowledgments | packfile)
> > + (flush-pkt | delim-pkt)
> > +
> > +acknowledgments = PKT-LINE("acknowledgments" LF)
> > + *(ready | nak | ack)
> 
> Can this part be described more precisely in the BNF section? I see that
> you describe later that there can be multiple ACKs or one NAK (but not
> both), and "ready" can be sent regardless of whether ACKs or a NAK is
> sent.

Yep I'll fix that.

> 
> > +ready = PKT-LINE("ready" LF)
> > +nak = PKT-LINE("NAK" LF)
> > +ack = PKT-LINE("ACK" SP obj-id LF)
> > +
> > +packfile = PKT-LINE("packfile" LF)
> > +  [PACKFILE]
> > +
> > +
> > +acknowledgments section
> > +   * Always begins with the section header "acknowledgments"
> > +
> > +   * The server will respond with "NAK" if none of the object ids sent
> > + as have lines were common.
> > +
> > +   * The server will respond with "ACK obj-id" for all of the
> > + object ids sent as have lines which are common.
> > +
> > +   * A response cannot have both "ACK" lines as well as a "NAK"
> > + line.
> > +
> > +   * The server will respond with a "ready" line indicating that
> > + the server has found an acceptable common base and is ready to
> > + make and send a packfile (which will be found in the packfile
> > + section of the same response)
> > +
> > +   * If the client determines that it is finished with negotiations
> > + by sending a "done" line, the acknowledgments sections can be
> > + omitted from the server's response as an optimization.
> 
> Should this be changed to "must"? The current implementation does not
> support it (on the client side).

This is actually a great question and one which may need to be thought
about in terms of its application to future extensions to the fetch
command.  Since fetch's response is now broken up into sections we may
want the client to cope with sections being in any order and maybe even
skipping sections it doesn't know about.  Not sure if its necessary but
its an idea.

> 
> > +#define UPLOAD_PACK_DATA_INIT { OBJECT_ARRAY_INIT, OID_ARRAY_INIT, 0, 0, 
> > 0, 0, 0, 0 }
> 
> Optional: the trailing zeroes can be omitted. (That's shorter, and also
> easier to maintain when we add new fields.)
> 
> > +int upload_pack_v2(struct repository *r, struct argv_array *keys,
> > +  struct argv_array *args)
> > +{
> > +   enum fetch_state state = FETCH_PROCESS_ARGS;
> > +   struct upload_pack_data data = UPLOAD_PACK_DATA_INIT;
> > +   use_sideband = LARGE_PACKET_MAX;
> > +
> > +   while (state != FETCH_DONE) {
> > +   switch (state) {
> > +   case FETCH_PROCESS_ARGS:
> > +   process_args(args, );
> > +
> > +   if (!want_obj.nr) {
> > +   /*
> > +* Request didn't contain any 'want' lines,
> > +* guess they didn't want anything.
> > +*/
> > +   state = FETCH_DONE;
> > +   } else if (data.haves.nr) {
> > +   /*
> > +* Request had 'have' lines, so lets ACK them.
> > +*/
> > +   state = FETCH_SEND_ACKS;
> > +   } else {
> > +   /*
> > +* Request had 'want's but no 'have's so we can
> > +* immedietly go to construct and send a pack.
> > +*/
> > +   state = FETCH_SEND_PACK;
> > +   }
> > +   break;
> > +   case FETCH_READ_HAVES:
> > +   read_haves();
> > +   state = FETCH_SEND_ACKS;
> > +   break;
> 
> This branch seems to never be taken?

Must be left over from another version, I'll remove it.

> 
> > +   case FETCH_SEND_ACKS:
> > +   if (process_haves_and_send_acks())
> > +   state = FETCH_SEND_PACK;
> > +   else
> > +   state = FETCH_DONE;
> > +   break;
> > +   case FETCH_SEND_PACK:
> > +   packet_write_fmt(1, "packfile\n");
> > +   create_pack_file();
> > +   state = FETCH_DONE;
> > +   break;
> > +   case FETCH_DONE:
> > +   continue;
> > +   }
> > +   }
> > +
> > +   upload_pack_data_clear();
> > +   return 0;
> > +}

-- 
Brandon Williams

Re: [PATCH v3 28/35] transport-helper: introduce stateless-connect

[Brandon Williams]

On 02/21, Jonathan Tan wrote:
> On Tue,  6 Feb 2018 17:13:05 -0800
> Brandon Williams  wrote:
> 
> > Introduce the transport-helper capability 'stateless-connect'.  This
> > capability indicates that the transport-helper can be requested to run
> > the 'stateless-connect' command which should attempt to make a
> > stateless connection with a remote end.  Once established, the
> > connection can be used by the git client to communicate with
> > the remote end natively in a stateless-rpc manner as supported by
> > protocol v2.  This means that the client must send everything the server
> > needs in a single request as the client must not assume any
> > state-storing on the part of the server or transport.
> 
> Maybe it's worth mentioning that support in the actual remote helpers
> will be added in a subsequent patch.

I can mention that.

> 
> > If a stateless connection cannot be established then the remote-helper
> > will respond in the same manner as the 'connect' command indicating that
> > the client should fallback to using the dumb remote-helper commands.
> 
> This makes sense, but there doesn't seem to be any code in this patch
> that implements this.
> 
> > @@ -612,6 +615,11 @@ static int process_connect_service(struct transport 
> > *transport,
> > if (data->connect) {
> > strbuf_addf(, "connect %s\n", name);
> > ret = run_connect(transport, );
> > +   } else if (data->stateless_connect) {
> > +   strbuf_addf(, "stateless-connect %s\n", name);
> > +   ret = run_connect(transport, );
> > +   if (ret)
> > +   transport->stateless_rpc = 1;
> 
> Why is process_connect_service() falling back to stateless_connect if
> connect doesn't work? I don't think this fallback would work, as a
> client that needs "connect" might need its full capabilities.

Right now there isn't really a notion of "needing" connect since if
connect fails then you need to fallback to doing the dumb thing.  Also
note that there isn't all fallback from connect to stateless-connect
here.  If the remote helper advertises connect, only connect will be
tried even if stateless-connect is advertised.  So this only really
works in the case where stateless-connect is advertised and connect
isn't, as is with our http remote-helper.

-- 
Brandon Williams

Re: [PATCH v4 08/13] commit-graph: implement --delete-expired

[Junio C Hamano]

Derrick Stolee  writes:

> Teach git-commit-graph to delete the .graph files that are siblings of a
> newly-written graph file, except for the file referenced by 'graph-latest'
> at the beginning of the process and the newly-written file. If we fail to
> delete a graph file, only report a warning because another git process may
> be using that file. In a multi-process environment, we expect the previoius
> graph file to be used by a concurrent process, so we do not delete it to
> avoid race conditions.

I do not understand the later part of the above.  On some operating
systems, you actually can remove a file that is open by another
process without any ill effect.  There are systems that do not allow
removing a file that is in use, and an attempt to unlink it may
fail.  The need to handle such a failure gracefully is not limited
to the case of removing a commit graph file---we need to deal with
it when removing file of _any_ type.

Especially the last sentence "we do not delete it to avoid race
conditions" I find problematic.  If a system does not allow removing
a file in use and we detect a failure after an attempt to do so, it
is not "we do not delete it" --- even if you do, you won't succeed
anyway, so there is no point saying that.  And on systems that do
allow safe removal of a file in use (i.e. they allow an open file to
be used by processes that have open filehandles to it after its
removal), there is no point refraining to delete it "to avoid race
conditions", either---in fact it is unlikely that you would even know
somebody else had it open and was using it.

In any case, I do not think '--delete-expired' option that can be
given only when you are writing makes much sense as an API.  An
'expire' command, just like 'set-latest' command, that is a separate
command from 'write',  may make sense, though.

Re: [PATCH] subtree: hide GPG signatures in calls to log

[Stefan Beller]

On Thu, Feb 22, 2018 at 5:37 AM, Stephen R Guglielmo
 wrote:
> On Feb 15, 2018 10:34 PM, "Stephen R Guglielmo" 
> wrote:
>
> This fixes `add` and `pull` for GPG signed objects.
>
> Signed-off-by: Stephen R Guglielmo 

Yay! Thanks for a patch!
I had to go back to the discussion
https://public-inbox.org/git/CADfK3RV1qo_jP=wd6zf2u9bh2xf+gjwbc9t4a3yk+c08o0o...@mail.gmail.com/
to really understand what is happening here. Can you give a summary
and explanation in the commit message?
(What is the current bug, how is it triggered, and why this is the
best way to fix it? That would be essentially repeating
https://public-inbox.org/git/cadfk3rwacb0m+m_u51jla9tnyru_7xesfy55i5euskh98jg...@mail.gmail.com/)

Now that I read the discussion, I think the code is fine.

Reviewed-by: Stefan Beller 

> ---
>  contrib/subtree/git-subtree.sh | 12 ++--
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
> index dec085a23..9594ca4b5 100755
> --- a/contrib/subtree/git-subtree.sh
> +++ b/contrib/subtree/git-subtree.sh
> @@ -297,7 +297,7 @@ find_latest_squash () {
>  main=
>  sub=
>  git log --grep="^git-subtree-dir: $dir/*\$" \
> ---pretty=format:'START %H%n%s%n%n%b%nEND%n' HEAD |
> +--no-show-signature --pretty=format:'START %H%n%s%n%n%b%nEND%n'
> HEAD |
>  while read a b junk
>  do
>  debug "$a $b $junk"
> @@ -341,7 +341,7 @@ find_existing_splits () {
>  main=
>  sub=
>  git log --grep="^git-subtree-dir: $dir/*\$" \
> ---pretty=format:'START %H%n%s%n%n%b%nEND%n' $revs |
> +--no-show-signature --pretty=format:'START %H%n%s%n%n%b%nEND%n'
> $revs |
>  while read a b junk
>  do
>  case "$a" in
> @@ -382,7 +382,7 @@ copy_commit () {
>  # We're going to set some environment vars here, so
>  # do it in a subshell to get rid of them safely later
>  debug copy_commit "{$1}" "{$2}" "{$3}"
> -git log -1 --pretty=format:'%an%n%ae%n%aD%n%cn%n%ce%n%cD%n%B' "$1" |
> +git log --no-show-signature -1
> --pretty=format:'%an%n%ae%n%aD%n%cn%n%ce%n%cD%n%B' "$1" |
>  (
>  read GIT_AUTHOR_NAME
>  read GIT_AUTHOR_EMAIL
> @@ -462,8 +462,8 @@ squash_msg () {
>  oldsub_short=$(git rev-parse --short "$oldsub")
>  echo "Squashed '$dir/' changes from $oldsub_short..$newsub_short"
>  echo
> -git log --pretty=tformat:'%h %s' "$oldsub..$newsub"
> -git log --pretty=tformat:'REVERT: %h %s' "$newsub..$oldsub"
> +git log --no-show-signature --pretty=tformat:'%h %s'
> "$oldsub..$newsub"
> +git log --no-show-signature --pretty=tformat:'REVERT: %h %s'
> "$newsub..$oldsub"
>  else
>  echo "Squashed '$dir/' content from commit $newsub_short"
>  fi
> @@ -475,7 +475,7 @@ squash_msg () {
>
>  toptree_for_commit () {
>  commit="$1"
> -git log -1 --pretty=format:'%T' "$commit" -- || exit $?
> +git rev-parse --verify "$commit^{tree}" || exit $?
>  }
>
>  subtree_for_commit () {
> --
> 2.16.1
>
>
>
> Hi all, just following up on this as I haven't heard any feedback.
>
> Thanks,
> Steve

Re: [PATCH v3 1/2] Fix misconversion of gitsubmodule.txt

[Stefan Beller]

On Thu, Feb 22, 2018 at 12:52 AM, marmot1123  wrote:
> In the 2nd and 4th paragraph of DESCRIPTION, there ware misconversions 
> `submodule’s`.
> It seems non-ASCII apostrophes, so I rewrite ASCII apostrophes.
>
> Signed-off-by: Motoki Seki 
> ---
>  Documentation/gitsubmodules.txt | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/gitsubmodules.txt b/Documentation/gitsubmodules.txt
> index 46cf120f666df..0d59ab4cdfb1c 100644
> --- a/Documentation/gitsubmodules.txt
> +++ b/Documentation/gitsubmodules.txt
> @@ -24,7 +24,7 @@ On the filesystem, a submodule usually (but not always - 
> see FORMS below)
>  consists of (i) a Git directory located under the `$GIT_DIR/modules/`
>  directory of its superproject, (ii) a working directory inside the
>  superproject's working directory, and a `.git` file at the root of
> -the submodule’s working directory pointing to (i).
> +the submodule's working directory pointing to (i).
>
>  Assuming the submodule has a Git directory at `$GIT_DIR/modules/foo/`
>  and a working directory at `path/to/bar/`, the superproject tracks the
> @@ -33,7 +33,7 @@ in its `.gitmodules` file (see linkgit:gitmodules[5]) of 
> the form
>  `submodule.foo.path = path/to/bar`.
>
>  The `gitlink` entry contains the object name of the commit that the
> -superproject expects the submodule’s working directory to be at.
> +superproject expects the submodule's working directory to be at.
>
>  The section `submodule.foo.*` in the `.gitmodules` file gives additional
>  hints to Gits porcelain layer such as where to obtain the submodule via
>
> --
> https://github.com/git/git/pull/459

Re: [PATCH v4 07/13] commit-graph: implement --set-latest

[Junio C Hamano]

Derrick Stolee  writes:

>  static struct opts_commit_graph {
>   const char *obj_dir;
>   const char *graph_file;
> + int set_latest;
>  } opts;
> ...
> @@ -89,6 +106,8 @@ static int graph_write(int argc, const char **argv)
>   { OPTION_STRING, 'o', "object-dir", _dir,
>   N_("dir"),
>   N_("The object directory to store the graph") },
> + OPT_BOOL('u', "set-latest", _latest,
> + N_("update graph-head to written graph file")),
>   OPT_END(),
>   };
>  
> @@ -102,6 +121,9 @@ static int graph_write(int argc, const char **argv)
>   graph_name = write_commit_graph(opts.obj_dir);
>  
>   if (graph_name) {
> + if (opts.set_latest)
> + set_latest_file(opts.obj_dir, graph_name);
> +

This feels like a very strange API from potential caller's point of
view.  Because you have to decide that you are going to mark it as
the latest one upfront before actually writing the graph file, if
you forget to pass --set-latest, you have to know how to manually
mark the file as latest anyway.  I would understand if it were one
of the following:

 (1) whenever a new commit graph file is written in the
 objects/info/ directory, always mark it as the latest (drop
 --set-latest option altogether); or

 (2) make set-latest command that takes a name of an existing graph
 file in the objects/info/ directory, and sets the latest
 pointer to point at it (make it separate from 'write' command).

though.

Git "branch properties"-- thoughts?

[Paul Smith]

Hi all.  I'm wondering if anyone has any thoughts about the best, or
even any, way to have "branch properties": some extra information which
is attached to a branch (that is, the actual branch name not the commit
it currently points to).

My requirements are that the information needs to be pushed to the
server, so that lets out branch descriptions for example.  Ideally the
information would also be easily updated and available in all clones
during normal fetch operations but this isn't a hard requirement: I
could script it.

My immediate desire is to find a way to mark a branch as "frozen", that
will control which pushes are allowed.  I use gitolite on my server and
I can easily write a server hook that will handle the checking,
rejecting, etc.  I already have such an infrastructure.  What I need is
a way to know which branches are in that state, so my hook can see that
and DTRT.  There are other "branch properties" I could envision, too,
but don't have a real need right now.

Of course I could embed the frozen state into the gitolite repository
configuration.  Indeed, I have already implemented "locks" for obsolete
branches.  But "frozen" is a more ephemeral state and requiring access
to the gitolite repository to manage it is just not what I want; it's a
separate repository so the state is not visible, requires privileges I
really don't want to hand out to everyone, and is generally difficult. 
I want some users to be able to manage frozen branches relatively
easily, and all users to be able to see the state of which branches are
frozen, etc.

So then I thought about creating a "frozen" tag, like "frozen/v1.0" or
something.  This is slightly weird because it is applied to a commit,
which is not really right, but whatever: it's just a marker so I would
just be checking to see if it exists or not.  The other problem is that
Git tags are not intended to be transient/moveable.  While you CAN
delete them and move them, when someone pulls the repository they won't
get that update by default.  Since the hook is server-side the fact
that the local repository has the wrong information doesn't matter for
behavior, but it's confusing for people.  So, it's not ideal.

I thought about creating a branch, like "frozen/v1.0", rather than a
tag.  I don't need a branch here, and no one would push to that branch
(I'd have to disallow that in my hooks), and the commit associated with
the branch would not be relevant most likely.  I would only check to
see if the branch existed, or not.  Branches are nice because creating
and deleting them is handled automatically (if you use prune
consistently, which we do because we have tons of transient branches).

Then I looked into using notes, and they look interesting, but they're
associated with a specific commit as well and I don't want that: a
frozen branch can still have new commits pushed to it they just have
meet certain criteria.  This makes them hard to translate into a branch
name.

So far, using a special branch name seems the most "reasonable".  But,
I wonder if I missed some cool aspect if Git that would work better, or
if anyone else has other suggestions.

Cheers!

Re: [PATCH v3 14/35] connect: request remote refs using v2

[Jeff King]

On Thu, Feb 22, 2018 at 10:19:22AM -0800, Brandon Williams wrote:

> On 02/21, Jonathan Tan wrote:
> > On Tue,  6 Feb 2018 17:12:51 -0800
> > Brandon Williams  wrote:
> > 
> > > +extern struct ref **get_remote_refs(int fd_out, struct packet_reader 
> > > *reader,
> > > + struct ref **list, int for_push,
> > > + const struct argv_array *ref_patterns);
> > 
> > I haven't looked at the rest of this patch in detail, but the type of
> > ref_patterns is probably better as struct string_list, since this is not
> > a true argument array (e.g. with flags starting with --). Same comment
> > for the next few patches that deal with ref patterns.
> 
> Its just a list of strings which don't require having a util pointer
> hanging around so actually using an argv_array would be more memory
> efficient than a string_list.  But either way I don't think it matters
> much.

I agree that it shouldn't matter much here. But if the name argv_array
is standing in the way of using it, I think we should consider giving it
a more general name. I picked that not to evoke "this must be arguments"
but "this is terminated by a single NULL".

In general I think it should be the preferred structure for string
lists, just because it actually converts for free to the "other" common
format (whereas you can never pass string_list.items to a function that
doesn't know about string lists).

-Peff

Re: [PATCH v3 16/35] transport: convert transport_get_remote_refs to take a list of ref patterns

[Brandon Williams]

On 02/21, Jonathan Tan wrote:
> On Tue,  6 Feb 2018 17:12:53 -0800
> Brandon Williams  wrote:
> 
> > -const struct ref *transport_get_remote_refs(struct transport *transport)
> > +const struct ref *transport_get_remote_refs(struct transport *transport,
> > +   const struct argv_array 
> > *ref_patterns)
> >  {
> > if (!transport->got_remote_refs) {
> > -   transport->remote_refs = 
> > transport->vtable->get_refs_list(transport, 0, NULL);
> > +   transport->remote_refs =
> > +   transport->vtable->get_refs_list(transport, 0,
> > +ref_patterns);
> > transport->got_remote_refs = 1;
> > }
> 
> Should we do our own client-side filtering if the server side cannot do
> it for us (because it doesn't support protocol v2)? Either way, this
> decision should be mentioned in the commit message.

If someone wants to add this in the future they can, but that is outside
the scope of this series.

-- 
Brandon Williams

Re: [PATCH v3 15/35] transport: convert get_refs_list to take a list of ref patterns

[Brandon Williams]

On 02/21, Jonathan Tan wrote:
> On Tue,  6 Feb 2018 17:12:52 -0800
> Brandon Williams  wrote:
> 
> > @@ -21,7 +22,8 @@ struct transport_vtable {
> >  * the ref without a huge amount of effort, it should store it
> >  * in the ref's old_sha1 field; otherwise it should be all 0.
> >  **/
> > -   struct ref *(*get_refs_list)(struct transport *transport, int for_push);
> > +   struct ref *(*get_refs_list)(struct transport *transport, int for_push,
> > +const struct argv_array *ref_patterns);
> 
> Also mention in the documentation that this function is allowed to
> return refs that do not match the ref patterns.

I'll add a comment.

-- 
Brandon Williams

Re: [PATCH v4 06/13] commit-graph: implement git commit-graph read

[Junio C Hamano]

Junio C Hamano  writes:

> Derrick Stolee  writes:
>
>> +'read'::
>> +
>> +Read a graph file given by the graph-head file and output basic
>> +details about the graph file.
>> ++
>> +With `--file=` option, consider the graph stored in the file at
>> +the path  /info/.
>> +
>
> A sample reader confusion after reading the above twice:
>
> What is "the graph-head file" and how does the user specify it?  Is
> it given by  the value for the "--file=" command line option?

This confusion is somewhat lightened with s/graph-head/graph-latest/
(I just saw 07/13 to realize that the file is renamed).

Perhaps describe it as "Read the graph file currently active
(i.e. the one pointed at by graph-latest file in the object/info
directory) and output blah" + "With --file parameter, read the
graph file specified with that parameter instead"?

Re: [PATCH v3 14/35] connect: request remote refs using v2

[Brandon Williams]

On 02/21, Jonathan Tan wrote:
> On Tue,  6 Feb 2018 17:12:51 -0800
> Brandon Williams  wrote:
> 
> > +extern struct ref **get_remote_refs(int fd_out, struct packet_reader 
> > *reader,
> > +   struct ref **list, int for_push,
> > +   const struct argv_array *ref_patterns);
> 
> I haven't looked at the rest of this patch in detail, but the type of
> ref_patterns is probably better as struct string_list, since this is not
> a true argument array (e.g. with flags starting with --). Same comment
> for the next few patches that deal with ref patterns.

Its just a list of strings which don't require having a util pointer
hanging around so actually using an argv_array would be more memory
efficient than a string_list.  But either way I don't think it matters
much.

-- 
Brandon Williams

Re: [PATCH v3 06/42] completion: use __gitcomp_builtin in _git_am

[Junio C Hamano]

Duy Nguyen  writes:

> Now that you mention it, the only command that completes
> --rerere-autoupdate is git-merge. Since this is "auto" I don't think
> people want to type manually.

Sorry, but I do not quite get the connection between "since this is
'auto'" and the rest of the sentence.  Is it just it is so lengthy
that people do not want to type and are likely to use completion?

> Maybe I should separate these changes
> _and_ remove --rerere-autoupdate from _git_merge() too? At least that
> it will be consistent that way.

H.  Why not complete this option?  Is it because the current
completion script does not and we are trying to preserve the
behaviour?  I do not have a strong opinion either way, but just
trying to understand the reasoning behind the choice.

>>> diff --git a/rerere.h b/rerere.h
>>> index c2961feaaa..5e5a312e4c 100644
>>> --- a/rerere.h
>>> +++ b/rerere.h
>>> @@ -37,6 +37,7 @@ extern void rerere_clear(struct string_list *);
>>>  extern void rerere_gc(struct string_list *);
>>>
>>>  #define OPT_RERERE_AUTOUPDATE(v) OPT_UYN(0, "rerere-autoupdate", (v), \
>>> -   N_("update the index with reused conflict resolution if possible"))
>>> +   N_("update the index with reused conflict resolution if possible"), 
>>> \
>>> +   PARSE_OPT_NOCOMPLETE)
>>>
>>>  #endif
>>> --
>>> 2.16.1.207.gedba492059
>>>

Re: Bug Report: git status triggers a file change event

[Stefan Beller]

On Wed, Feb 21, 2018 at 9:22 PM, Jonathan Nieder  wrote:
> +git-for-windows
> Hi,
>
> Raining Chain wrote:
>
>> On Windows 10, git version 2.16.2.windows.1, running the command
>>
>> git status
>>
>> will trigger a file change event to file C:\myPath\.git  "Attributes 
>> changed."
>>
>> This causes problems when using scripts that detect file changes such
>> as tsc -w (Typescript compiler) and using softwares that regularly
>> call git status such as VisualStudioCode.
>>
>> Thanks.

Does
https://github.com/git/git/commit/27344d6a6c8056664966e11acf674e5da6dd7ee3
help?

>
> Can you say more about how "tsc -w" reacts to the file change?  Is there
> a way to tell it to exclude particular files from the files it watches?
>
> Thanks,
> Jonathan

Re: [PATCH v3 08/35] connect: discover protocol version outside of get_remote_heads

[Brandon Williams]

On 02/21, Jonathan Tan wrote:
> On Tue,  6 Feb 2018 17:12:45 -0800
> Brandon Williams  wrote:
> 
> > -   get_remote_heads(fd[0], NULL, 0, , 0, NULL, );
> > +
> > +   packet_reader_init(, fd[0], NULL, 0,
> > +  PACKET_READ_CHOMP_NEWLINE |
> > +  PACKET_READ_GENTLE_ON_EOF);
> > +
> > +   switch (discover_version()) {
> > +   case protocol_v1:
> > +   case protocol_v0:
> > +   get_remote_heads(, , 0, NULL, );
> > +   break;
> > +   case protocol_unknown_version:
> > +   BUG("unknown protocol version");
> > +   }
> 
> This inlining is repeated a few times, which raises the question: if the
> intention is to keep the v0/1 logic separately from v2, why not have a
> single function that wraps them all? Looking at the end result (after
> all the patches in this patch set are applied), it seems that the v2
> version does not have extra_have or shallow parameters, which is a good
> enough reason for me (I don't think functions that take in many
> arguments and then selectively use them is a good idea). I think that
> other reviewers will have this question too, so maybe discuss this in
> the commit message.

Yes this sort of switch statement appears a few times but really there
isn't a good way to "have one function to wrap it all" with the current
state of the code. That sort of change would take tons of refactoring to
get into a state where we could do that, and is outside the scope of
this series.

> 
> > diff --git a/remote.h b/remote.h
> > index 1f6611be2..2016461df 100644
> > --- a/remote.h
> > +++ b/remote.h
> > @@ -150,10 +150,11 @@ int check_ref_type(const struct ref *ref, int flags);
> >  void free_refs(struct ref *ref);
> >  
> >  struct oid_array;
> > -extern struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
> > +struct packet_reader;
> > +extern struct ref **get_remote_heads(struct packet_reader *reader,
> >  struct ref **list, unsigned int flags,
> >  struct oid_array *extra_have,
> > -struct oid_array *shallow);
> > +struct oid_array *shallow_points);
> 
> This change probably does not belong in this patch, especially since
> remote.c is unchanged.

Yes this hunk is needed, the signature of get_remote_heads changes.  It
may be difficult to see that due to the fact that we don't really have a
clear story on how header files are divided up within the project.

-- 
Brandon Williams

Re: [PATCH v3 04/35] upload-pack: convert to a builtin

[Jeff King]

On Thu, Feb 22, 2018 at 10:07:15AM -0800, Brandon Williams wrote:

> On 02/22, Jeff King wrote:
> > On Wed, Feb 21, 2018 at 01:44:22PM -0800, Jonathan Tan wrote:
> > 
> > > On Tue,  6 Feb 2018 17:12:41 -0800
> > > Brandon Williams  wrote:
> > > 
> > > > In order to allow for code sharing with the server-side of fetch in
> > > > protocol-v2 convert upload-pack to be a builtin.
> > > > 
> > > > Signed-off-by: Brandon Williams 
> > > 
> > > As Stefan mentioned in [1], also mention in the commit message that this
> > > means that the "git-upload-pack" invocation gains additional
> > > capabilities (for example, invoking a pager for --help).
> > 
> > And possibly respecting pager.upload-pack, which would violate our rule
> > that it is safe to run upload-pack in untrusted repositories.
> 
> And this isn't an issue with receive-pack because this same guarantee
> doesn't exist?

Yes, exactly (which is confusing and weird, yes, but that's how it is).

-Peff

Re: [PATCH v3 04/35] upload-pack: convert to a builtin

[Brandon Williams]

On 02/22, Jeff King wrote:
> On Wed, Feb 21, 2018 at 01:44:22PM -0800, Jonathan Tan wrote:
> 
> > On Tue,  6 Feb 2018 17:12:41 -0800
> > Brandon Williams  wrote:
> > 
> > > In order to allow for code sharing with the server-side of fetch in
> > > protocol-v2 convert upload-pack to be a builtin.
> > > 
> > > Signed-off-by: Brandon Williams 
> > 
> > As Stefan mentioned in [1], also mention in the commit message that this
> > means that the "git-upload-pack" invocation gains additional
> > capabilities (for example, invoking a pager for --help).
> 
> And possibly respecting pager.upload-pack, which would violate our rule
> that it is safe to run upload-pack in untrusted repositories.

And this isn't an issue with receive-pack because this same guarantee
doesn't exist?

> 
> (This actually doesn't work right now because pager.* is broken for
> builtins that don't specify RUN_SETUP; but I think with the fixes last
> year to the config code, we can now drop that restriction).
> 
> Obviously we can work around this with an extra RUN_NO_PAGER_CONFIG
> flag. But I think it points to a general danger in making upload-pack a
> builtin. I'm not sure what other features it would want to avoid (or
> what might grow in the future).
> 
> > Having said that, the main purpose of this patch seems to be to libify
> > upload-pack, and the move to builtin is just a way of putting the
> > program somewhere - we could have easily renamed upload-pack.c and
> > created a new upload-pack.c containing the main(), preserving the
> > non-builtin-ness of upload-pack, while still gaining the benefits of
> > libifying upload-pack.
> 
> Yeah, this seems like a better route to me.
> 
> -Peff

-- 
Brandon Williams

Re: What's cooking in git.git (Feb 2018, #03; Wed, 21)

[Junio C Hamano]

Brandon Williams  writes:

>>  Is the 'fixup!' cleanly squashable to the problematic one, or does
>>  this series require another reroll to get it in a good enough shape?
>
> Yeah the fixup patch looks good to me.  I don't think there was anything
> else that needed attention so it should be in good shape with the fixup
> patch.

OK, let me squash it down and mark the topic to be merged to 'next'
after giving it a quick final look.  Thanks.

Re: bug in HTTP protocol spec

[Brandon Williams]

On 02/21, Dorian Taylor wrote:
> 
> > On Feb 21, 2018, at 9:37 PM, Jonathan Nieder  wrote:
> > 
> > Thanks for writing it.
> > 
> > Do you mind if we forge your sign-off?  (See Documentation/SubmittingPatches
> > item '(5) Certify your work' for details about what this means.)
> 
> Sure, or I can just re-paste:
> 
> Signed-off-by: Dorian Taylor 
> 
> ---
> Documentation/technical/http-protocol.txt | 10 +++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/technical/http-protocol.txt 
> b/Documentation/technical/http-protocol.txt
> index a0e45f2889e6e..19d73f7efb338 100644
> --- a/Documentation/technical/http-protocol.txt
> +++ b/Documentation/technical/http-protocol.txt
> @@ -214,14 +214,17 @@ smart server reply:
>   S: Cache-Control: no-cache
>   S:
>   S: 001e# service=git-upload-pack\n
> +   S: 
>   S: 004895dcfa3633004da0049d3d0fa03f80589cbcaf31 
> refs/heads/maint\0multi_ack\n
>   S: 0042d049f6c27a2244e12041955e262a404c7faba355 refs/heads/master\n
>   S: 003c2cb58b79488a98d2721cea644875a8dd0026b115 refs/tags/v1.0\n
>   S: 003fa3c2e2402b99163d1d59756e5f207ae21cccba4c refs/tags/v1.0^{}\n
> +   S: 
> 
> The client may send Extra Parameters (see
> Documentation/technical/pack-protocol.txt) as a colon-separated string
> -in the Git-Protocol HTTP header.
> +in the Git-Protocol HTTP header. Note as well that there is *no* newline
> +after the ``.
> 
> Dumb Server Response
> 
> @@ -264,8 +267,8 @@ Servers MUST set $servicename to be the request parameter 
> value.
> Servers SHOULD include an LF at the end of this line.
> Clients MUST ignore an LF at the end of the line.
> 
> -Servers MUST terminate the response with the magic `` end
> -pkt-line marker.
> +Servers MUST follow the first pkt-line, as well as terminate the
> +response, with the magic `` end pkt-line marker.
> 
> The returned response is a pkt-line stream describing each ref and
> its known value.  The stream SHOULD be sorted by name according to
> @@ -278,6 +281,7 @@ Extra Parameter.
> 
>  smart_reply =  PKT-LINE("# service=$servicename" LF)
>*1("version 1")
> +  ""
>ref_list
>""
>  ref_list=  empty_list / non_empty_list
> 
> ---
> 
> > 
> >> Note I am not sure what the story is behind that `version 1`
> >> element, whether it's supposed to go before or after the null packet
> >> or if there should be another null packet or what. Perhaps somebody
> >> more fluent with the smart protocol can advise.
> > 
> > I believe the 'version 1' goes after the flush-packet.
> 
> I took a traipse through the code and couldn’t determine it one way or 
> another, but my money is on that looking something like `000aversion 1\n` on 
> the wire.

Yes the version string goes along with the ref_list in v1 like so:

  # service=
  
  version 1
  ref_list
  

This is because it is part of the payload which is actually delivered to
the git fetch/push binary where as the "# service" bit is used by the
remote helper to identify smart vs not smart servers.

> 
> --
> Dorian Taylor
> Make things. Make sense.
> https://doriantaylor.com
> 



-- 
Brandon Williams

Re: bug in HTTP protocol spec

[Dorian Taylor]

> On Feb 22, 2018, at 2:08 AM, Jeff King  wrote:
> 
> 
> This indentation is funny. But I suspect it is because your whole patch
> seems to have been whitespace-damaged (see the section on gmail in
> "git help git-format-patch").

That is, bit-for-bit, what came out of that submitGit thing that is advertised 
when you try to open a pull request on the git repository on Github. I was a 
bit surprised myself.

--
Dorian Taylor
Make things. Make sense.
https://doriantaylor.com



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH v5 00/17] document & test fetch pruning & add fetch.pruneTags

[Ævar Arnfjörð Bjarmason]

On Thu, Feb 22 2018, Junio C. Hamano jotted:

> Ævar Arnfjörð Bjarmason   writes:
>
>> Here's a v5 (correct subject line this time!). Many thanks to Eric for
>> a thorough review.
>
> We haven't seen any comments on this round.  Is everybody happy?
>
> I do not have a strong opinion on the new feature, either for or
> against.  I didn't find anything majorly questionable in the
> execution, though, so...

I've been running that here on thousands of boxes (that are actively
using it) for 2 weeks now without issue. Would be great to have it
merged down & in 2.17.

Re: [PATCH 1/2] parse-options: expand $HOME on filename options

[Ævar Arnfjörð Bjarmason]

On Thu, Feb 22 2018, Duy Nguyen jotted:

> On Thu, Feb 15, 2018 at 5:46 AM, Ævar Arnfjörð Bjarmason
>  wrote:
>> In general I'm mildly negative on adding this, for every user like Doron
>> who'll be less confused by a hack like this, you'll have other users
>> who'll be confused about git inexplicably working with ~ in the middle
>> of strings, even though;
>>
>> $ echo git init --template ~/path
>> git init --template /home/avar/path
>> $ echo git init --template=~/path
>> git init --template=~/path
>
> If you have a directory named '~', I expect you are already used to
> prefixing it with './' because '~' will be expanded in many places
> where you might want to avoid.

Indeed. I've never had this use-case, just saying if it's being changed
it makes sense to have a small test for it somewhere.

>> I think it makes more sense to just leave such expansion to the shell,
>> and not try to magically expand it after the fact, since it's both
>> confusing (user: why does this work with git and not this other
>> program?), and as shown above changes existing semantics.
>>
>> We'll also be setting ourselves up for more disappointed users who'll
>> notice that e.g. `git clone file://~/path` doesn't work, but `git clone
>> file://$HOME/path` does, requiring more hacks to expand ~ in more
>> codepaths. Will they also expact `git log -G~` to find references to
>> their homedir in their dotfiles.git?
>>
>> I think this way lies madness, and it's better to just avoid it.
>
> Well. That's a bit extreme, I think if we add this then we handle case
> by case in future when it makes sense, not blindly expanding '~'
> everywhere.
>
> The problem I have with this --template=~/path is tab-completion
> actually completes the path, which (mis)leads me to think the command
> will accept '~/' too. But this looks like a bug in git-completion.bash
> though, it's a bit eager in completing stuff (or maybe it completes
> "--template ~/path" and "--template=~/path" the same way).

Ah I see, so you're doing "git init --template=~/".

> I don't feel strongly about this. I'm OK with dropping these patches
> if people think it's not a good idea (then I will try to fix
> git-completion.bash not to complete '~' in this case).

I don't feel strongly about it either, just mildly negative on
introducing magic that gives you different behavior than shells do by
default.

I wonder if the consistency with the tab completion wouldn't be better
done by teaching the tab completion to just expand --template=~/ to
e.g. --template=/home/duy/.

On my (Debian) system doing e.g.:

echo $HOME/bin/

Will expand to:

echo /home/avar/bin/

Maybe we could intercept that in the completion and ~ to the value of
$HOME. It would give completion that did the right thing, without the
expectation that ~ is going to be magic in some places and not others.

>> But I think that if we're going to keep it it needs some tests & docs to
>> point confused users to.

HIII2

[Lindsey]

R Hey,i am Lindsey ,How's everything with you,I have interest on you
after going through your profile I really want to have a good
friendship with you.Beside i have something very vital to tell you

Issue in switching branches with different submodules

[Christian . Zitzmann]

Hello,
I've found an issue in git when working with submodules.
My Project is set up using hundreds of components by submodules (and 
nested submodules), that are independent created in central development 
groups.
Now it occurs that the structure of the submodules is changed over time.

E.g.
Project1(OldBranch)
  - ComponentX/SubComp1 -> ssh://Server/ComponentX/SubComp1
  - ComponentX/SubComp2 -> ssh://Server/ComponentX/SubComp2
  - ComponentX/SubComp2 -> ssh://Server/ComponentX/SubComp2

Project1(Masster)
  - ComponentX-> ssh://Server/ComponentX

There is both a repository for the old subcomponents, and for the new 
Component on the server.

When trying to switch between the branches all git commands are failing.
It seems like most git commands are not creating the SubComponent 
submodules because the .git file from the Component is not deleted

A 'git submodule sync' fails with:
fatal: Not a git repository: 
D:/Project1/ComponentX/../.git/modules/ComponentX

Looking into the folders I see:
D:/Project1/.git/modules/ComponentX/SubComp1
D:/Project1/.git/modules/ComponentX/SubComp2
D:/Project1/.git/modules/ComponentX/SubComp3
D:/Project1/ComponentX/.git (file)

A 'git submodule update --init also fails with this folders
Neither a forced checkout or a hard reset is working.

Similar errors can occur when switching between branches with a different 
number of components used as submodules vs. project specific content.
As a result it happens that people are working with an incosistend state 
of the working directory.

My expectation would be that, when switching between branches/commits with 
a git checkout --recurse-submodules the branche including all submodules 
is checked out fully to exactly the state of the desired branch/commit
I.e the following usecases are an issue
- Deleted submodule
- Added submodule as replacement of local directory
- Changed remote location of submodule (One component is available from 
different providers having individual repositories)
- Restructured Component (see example)

Similar issues are existing when creating the new structure to commit it, 
but here the error is more obvious and people are manually deleting as 
much as required to get the new submodules correctly in.

Best regards

Christian Zitzmann


P HEV E SW SF PMT

Continental Automotive GmbH
Siemensstrasse 12, 93055 Regensburg, Germany

Telefon/Phone: +49 941 790-7265
E-Mail: christian.zitzm...@continental-corporation.com

Re: bug in HTTP protocol spec

[Jeff King]

On Wed, Feb 21, 2018 at 11:23:52PM -0800, Dorian Taylor wrote:

> diff --git a/Documentation/technical/http-protocol.txt 
> b/Documentation/technical/http-protocol.txt
> index a0e45f2889e6e..19d73f7efb338 100644
> --- a/Documentation/technical/http-protocol.txt
> +++ b/Documentation/technical/http-protocol.txt
> @@ -214,14 +214,17 @@ smart server reply:
>   S: Cache-Control: no-cache
>   S:
>   S: 001e# service=git-upload-pack\n
> +   S: 
>   S: 004895dcfa3633004da0049d3d0fa03f80589cbcaf31 
> refs/heads/maint\0multi_ack\n
>   S: 0042d049f6c27a2244e12041955e262a404c7faba355 refs/heads/master\n
>   S: 003c2cb58b79488a98d2721cea644875a8dd0026b115 refs/tags/v1.0\n
>   S: 003fa3c2e2402b99163d1d59756e5f207ae21cccba4c refs/tags/v1.0^{}\n
> +   S: 

This indentation is funny. But I suspect it is because your whole patch
seems to have been whitespace-damaged (see the section on gmail in
"git help git-format-patch").

> The client may send Extra Parameters (see
> Documentation/technical/pack-protocol.txt) as a colon-separated string
> -in the Git-Protocol HTTP header.
> +in the Git-Protocol HTTP header. Note as well that there is *no* newline
> +after the ``.

I guess I'm not opposed to calling that out, but this is normal for
pktline (the flush packet has no data; in the other lines the newline is
not a syntactic part of the pktline stream, but is actually data
contained inside each of those pktlines).

> Dumb Server Response
> 
> @@ -264,8 +267,8 @@ Servers MUST set $servicename to be the request parameter 
> value.
> Servers SHOULD include an LF at the end of this line.
> Clients MUST ignore an LF at the end of the line.
> 
> -Servers MUST terminate the response with the magic `` end
> -pkt-line marker.
> +Servers MUST follow the first pkt-line, as well as terminate the
> +response, with the magic `` end pkt-line marker.

In theory there can actually be one or more headers after the "service"
line. But I don't think they've ever been used (and the current client
just throws them away).

> The returned response is a pkt-line stream describing each ref and
> its known value.  The stream SHOULD be sorted by name according to
> @@ -278,6 +281,7 @@ Extra Parameter.
> 
>  smart_reply =  PKT-LINE("# service=$servicename" LF)
>*1("version 1")
> +  ""
>ref_list
>""

I think Jonathan is right that the version must go after the flush
packet (just looking at the v2 protocol patches elsewhere on the list,
the version tag is really part of the ref_list).

Not related to your patch, but I suspect it should also be
PKT-LINE("version 1").

-Peff

Re: [PATCH v3 04/35] upload-pack: convert to a builtin

[Jeff King]

On Wed, Feb 21, 2018 at 01:44:22PM -0800, Jonathan Tan wrote:

> On Tue,  6 Feb 2018 17:12:41 -0800
> Brandon Williams  wrote:
> 
> > In order to allow for code sharing with the server-side of fetch in
> > protocol-v2 convert upload-pack to be a builtin.
> > 
> > Signed-off-by: Brandon Williams 
> 
> As Stefan mentioned in [1], also mention in the commit message that this
> means that the "git-upload-pack" invocation gains additional
> capabilities (for example, invoking a pager for --help).

And possibly respecting pager.upload-pack, which would violate our rule
that it is safe to run upload-pack in untrusted repositories.

(This actually doesn't work right now because pager.* is broken for
builtins that don't specify RUN_SETUP; but I think with the fixes last
year to the config code, we can now drop that restriction).

Obviously we can work around this with an extra RUN_NO_PAGER_CONFIG
flag. But I think it points to a general danger in making upload-pack a
builtin. I'm not sure what other features it would want to avoid (or
what might grow in the future).

> Having said that, the main purpose of this patch seems to be to libify
> upload-pack, and the move to builtin is just a way of putting the
> program somewhere - we could have easily renamed upload-pack.c and
> created a new upload-pack.c containing the main(), preserving the
> non-builtin-ness of upload-pack, while still gaining the benefits of
> libifying upload-pack.

Yeah, this seems like a better route to me.

-Peff

[PATCH 1/1] git-p4: add unshelve command

[Luke Diamand]

This can be used to "unshelve" a shelved P4 commit into
a git commit.

For example:

  $ git p4 unshelve 12345

The resulting commit ends up in the branch:
   refs/remotes/p4/unshelved/12345

Signed-off-by: Luke Diamand 
---
 Documentation/git-p4.txt |  22 
 git-p4.py| 128 +++
 t/t9832-unshelve.sh  |  67 +
 3 files changed, 186 insertions(+), 31 deletions(-)
 create mode 100755 t/t9832-unshelve.sh

diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
index d8c8f11c9f..910ae63a1c 100644
--- a/Documentation/git-p4.txt
+++ b/Documentation/git-p4.txt
@@ -164,6 +164,21 @@ $ git p4 submit --shelve
 $ git p4 submit --update-shelve 1234 --update-shelve 2345
 
 
+
+Unshelve
+
+Unshelving will take a shelved P4 changelist, and produce the equivalent git 
commit
+in the branch refs/remotes/p4/unshelved/.
+
+The git commit is created relative to the current p4/master branch, so if this
+branch is behind Perforce itself, it may include more changes than you 
expected.
+
+
+$ git p4 sync
+$ git p4 unshelve 12345
+$ git show refs/remotes/p4/unshelved/12345
+
+
 OPTIONS
 ---
 
@@ -337,6 +352,13 @@ These options can be used to modify 'git p4 rebase' 
behavior.
 --import-labels::
Import p4 labels.
 
+Unshelve options
+
+
+--origin::
+Sets the git refspec against which the shelved P4 changelist is compared.
+Defaults to p4/master.
+
 DEPOT PATH SYNTAX
 -
 The p4 depot path argument to 'git p4 sync' and 'git p4 clone' can
diff --git a/git-p4.py b/git-p4.py
index 7bb9cadc69..59bd4ff64f 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -316,12 +316,17 @@ def p4_last_change():
 results = p4CmdList(["changes", "-m", "1"], skip_info=True)
 return int(results[0]['change'])
 
-def p4_describe(change):
+def p4_describe(change, shelved=False):
 """Make sure it returns a valid result by checking for
the presence of field "time".  Return a dict of the
results."""
 
-ds = p4CmdList(["describe", "-s", str(change)], skip_info=True)
+cmd = ["describe", "-s"]
+if shelved:
+cmd += ["-S"]
+cmd += [str(change)]
+
+ds = p4CmdList(cmd, skip_info=True)
 if len(ds) != 1:
 die("p4 describe -s %d did not return 1 result: %s" % (change, 
str(ds)))
 
@@ -2421,6 +2426,18 @@ class P4Sync(Command, P4UserMap):
 if gitConfig("git-p4.syncFromOrigin") == "false":
 self.syncWithOrigin = False
 
+self.depotPaths = []
+self.changeRange = ""
+self.previousDepotPaths = []
+self.hasOrigin = False
+
+# map from branch depot path to parent branch
+self.knownBranches = {}
+self.initialParents = {}
+
+self.tz = "%+03d%02d" % (- time.timezone / 3600, ((- time.timezone % 
3600) / 60))
+self.labels = {}
+
 # Force a checkpoint in fast-import and wait for it to finish
 def checkpoint(self):
 self.gitStream.write("checkpoint\n\n")
@@ -2429,7 +2446,7 @@ class P4Sync(Command, P4UserMap):
 if self.verbose:
 print "checkpoint finished: " + out
 
-def extractFilesFromCommit(self, commit):
+def extractFilesFromCommit(self, commit, shelved=False, shelved_cl = 0):
 self.cloneExclude = [re.sub(r"\.\.\.$", "", path)
  for path in self.cloneExclude]
 files = []
@@ -2452,6 +2469,9 @@ class P4Sync(Command, P4UserMap):
 file["rev"] = commit["rev%s" % fnum]
 file["action"] = commit["action%s" % fnum]
 file["type"] = commit["type%s" % fnum]
+if shelved:
+file["shelved_cl"] = int(shelved_cl)
+
 files.append(file)
 fnum = fnum + 1
 return files
@@ -2743,7 +2763,16 @@ class P4Sync(Command, P4UserMap):
 def streamP4FilesCbSelf(entry):
 self.streamP4FilesCb(entry)
 
-fileArgs = ['%s#%s' % (f['path'], f['rev']) for f in filesToRead]
+fileArgs = []
+for f in filesToRead:
+if 'shelved_cl' in f:
+# Handle shelved CLs using the "p4 print file@=N" syntax 
to print
+# the contents
+fileArg = '%s@=%d' % (f['path'], f['shelved_cl'])
+else:
+fileArg = '%s#%s' % (f['path'], f['rev'])
+
+fileArgs.append(fileArg)
 
 p4CmdList(["-x", "-", "print"],
   stdin=fileArgs,
@@ -3162,10 +3191,10 @@ class P4Sync(Command, P4UserMap):
 else:
 return None
 
-def importChanges(self, changes):
+def importChanges(self, changes, shelved=False):
 cnt = 1
 for change in changes:
-description = p4_describe(change)
+description = p4_describe(change, shelved)
 self.updateOptionDict(description)
 
  

[PATCH 0/1] git-p4: add unshelve command

[Luke Diamand]

This is an initial attempt at adding an unshelve command to
git-p4.

For those not familiar with it, p4 shelve creates a "pending"
changelist, which isn't committed into the central repo but is
nonetheless visible to other develoeprs. The "unshelve" command
takes one of these pending changelists and applies it to your repo.
It is used quite a lot for code review.

git-p4 learned about shelving changelists recently; this completes
the picture by letting you unshelve them as well.

This was inspired by the stackoverflow answer here:


https://stackoverflow.com/questions/41841917/git-p4-how-to-fetch-a-changelist

The secret is to use the "p4 print file@=N" syntax to get the
contents of a shelved changelist, which has long perplexed me.

I haven't used this a great deal, so it may still have a few rough
edges.

In particular, it currently puts the unshelved commit into
refs/remotes/p4/unshelved/

where  is the changelist being unshelved. That might not be
the best way to do this.


Luke Diamand (1):
  git-p4: add unshelve command

 Documentation/git-p4.txt |  22 
 git-p4.py| 128 +++
 t/t9832-unshelve.sh  |  67 +
 3 files changed, 186 insertions(+), 31 deletions(-)
 create mode 100755 t/t9832-unshelve.sh

-- 
2.15.1.272.gc310869385

Re: [PATCH v3 13/35] ls-refs: introduce ls-refs server command

[Jeff King]

On Tue, Feb 06, 2018 at 05:12:50PM -0800, Brandon Williams wrote:

> +ls-refs takes in the following parameters wrapped in packet-lines:
> +
> +symrefs
> + In addition to the object pointed by it, show the underlying ref
> + pointed by it when showing a symbolic ref.
> +peel
> + Show peeled tags.
> +ref-pattern 
> + When specified, only references matching the one of the provided
> + patterns are displayed.

How do we match those patterns? That's probably an important thing to
include in the spec.

Looking at the code, I see:

> +/*
> + * Check if one of the patterns matches the tail part of the ref.
> + * If no patterns were provided, all refs match.
> + */
> +static int ref_match(const struct argv_array *patterns, const char *refname)

This kind of tail matching can't quite implement all of the current
behavior. Because we actually do the normal dwim_ref() matching, which
includes stuff like "refs/remotes/%s/HEAD".

The other problem with tail-matching is that it's inefficient on the
server. Ideally we could get a request for "master" and only look up
refs/heads/master, refs/tags/master, etc. And if there are 50,000 refs
in refs/pull, we wouldn't have to process those at all. Of course this
is no worse than the current code, which not only looks at each ref but
actually _sends_ it. But it would be nice if we could fix this.

There's some more discussion in this old thread:

  
https://public-inbox.org/git/20161024132932.i42rqn2vlpocq...@sigill.intra.peff.net/

> +{
> + char *pathbuf;
> + int i;
> +
> + if (!patterns->argc)
> + return 1; /* no restriction */
> +
> + pathbuf = xstrfmt("/%s", refname);
> + for (i = 0; i < patterns->argc; i++) {
> + if (!wildmatch(patterns->argv[i], pathbuf, 0)) {
> + free(pathbuf);
> + return 1;
> + }
> + }
> + free(pathbuf);
> + return 0;
> +}

Does the client have to be aware that we're using wildmatch? I think
they'd need "refs/heads/**" to actually implement what we usually
specify in refspecs as "refs/heads/*". Or does the lack of WM_PATHNAME
make this work with just "*"?

Do we anticipate that the client would left-anchor the refspec like
"/refs/heads/*" so that in theory the server could avoid looking outside
of /refs/heads/?

-Peff

Re: [PATCH v3 12/35] serve: introduce git-serve

[Jeff King]

On Tue, Feb 06, 2018 at 05:12:49PM -0800, Brandon Williams wrote:

> +In protocol v2 communication is command oriented.  When first contacting a
> +server a list of capabilities will advertised.  Some of these capabilities
> +will be commands which a client can request be executed.  Once a command
> +has completed, a client can reuse the connection and request that other
> +commands be executed.

If I understand this correctly, we'll potentially have a lot more
round-trips between the client and server (one per "command"). And for
git-over-http, each one will be its own HTTP request?

We've traditionally tried to minimize HTTP requests, but I guess it's
not too bad if we can keep the connection open in most cases. Then we
just suffer some extra framing bytes, but we don't have to re-establish
the TCP connection each time.

I do wonder if the extra round trips will be noticeable in high-latency
conditions. E.g., if I'm 200ms away, converting the current
ref-advertisement spew to "capabilities, then the client asks for refs,
then we spew the refs" is going to cost an extra 200ms, even if the
fetch just ends up being a noop. I'm not sure how bad that is in the
grand scheme of things (after all, the TCP handshake involves some
round-trips, too).

> + Capability Advertisement
> +--
> +
> +A server which decides to communicate (based on a request from a client)
> +using protocol version 2, notifies the client by sending a version string
> +in its initial response followed by an advertisement of its capabilities.
> +Each capability is a key with an optional value.  Clients must ignore all
> +unknown keys.  Semantics of unknown values are left to the definition of
> +each key.  Some capabilities will describe commands which can be requested
> +to be executed by the client.
> +
> +capability-advertisement = protocol-version
> +capability-list
> +flush-pkt
> +
> +protocol-version = PKT-LINE("version 2" LF)
> +capability-list = *capability
> +capability = PKT-LINE(key[=value] LF)
> +
> +key = 1*CHAR
> +value = 1*CHAR
> +CHAR = 1*(ALPHA / DIGIT / "-" / "_")
> +
> +A client then responds to select the command it wants with any particular
> +capabilities or arguments.  There is then an optional section where the
> +client can provide any command specific parameters or queries.
> +
> +command-request = command
> +   capability-list
> +   (command-args)
> +   flush-pkt
> +command = PKT-LINE("command=" key LF)
> +command-args = delim-pkt
> +*arg
> +arg = 1*CHAR

For a single stateful TCP connection like git:// or git-over-ssh, the
client would get the capabilities once and then issue a series of
commands. For git-over-http, how does it work?

The client speaks first in HTTP, so we'd first make a request to get
just the capabilities from the server? And then proceed from there with
a series of requests, assuming that the capabilities for each server we
subsequently contact are the same? That's probably reasonable (and
certainly the existing http protocol makes that capabilities
assumption).

I don't see any documentation on how this all works with http. But
reading patch 34, it looks like we just do the usual
service=git-upload-pack request (with the magic request for v2), and
then the server would send us capabilities. Which follows my line of
thinking in the paragraph above.

-Peff

Re: [PATCH 1/2] parse-options: expand $HOME on filename options

[Duy Nguyen]

On Thu, Feb 15, 2018 at 5:46 AM, Ævar Arnfjörð Bjarmason
 wrote:
> In general I'm mildly negative on adding this, for every user like Doron
> who'll be less confused by a hack like this, you'll have other users
> who'll be confused about git inexplicably working with ~ in the middle
> of strings, even though;
>
> $ echo git init --template ~/path
> git init --template /home/avar/path
> $ echo git init --template=~/path
> git init --template=~/path

If you have a directory named '~', I expect you are already used to
prefixing it with './' because '~' will be expanded in many places
where you might want to avoid.

> I think it makes more sense to just leave such expansion to the shell,
> and not try to magically expand it after the fact, since it's both
> confusing (user: why does this work with git and not this other
> program?), and as shown above changes existing semantics.
>
> We'll also be setting ourselves up for more disappointed users who'll
> notice that e.g. `git clone file://~/path` doesn't work, but `git clone
> file://$HOME/path` does, requiring more hacks to expand ~ in more
> codepaths. Will they also expact `git log -G~` to find references to
> their homedir in their dotfiles.git?
>
> I think this way lies madness, and it's better to just avoid it.

Well. That's a bit extreme, I think if we add this then we handle case
by case in future when it makes sense, not blindly expanding '~'
everywhere.

The problem I have with this --template=~/path is tab-completion
actually completes the path, which (mis)leads me to think the command
will accept '~/' too. But this looks like a bug in git-completion.bash
though, it's a bit eager in completing stuff (or maybe it completes
"--template ~/path" and "--template=~/path" the same way).

I don't feel strongly about this. I'm OK with dropping these patches
if people think it's not a good idea (then I will try to fix
git-completion.bash not to complete '~' in this case).

> But I think that if we're going to keep it it needs some tests & docs to
> point confused users to.
-- 
Duy

Re: [PATCH v3 06/42] completion: use __gitcomp_builtin in _git_am

[Duy Nguyen]

On Wed, Feb 14, 2018 at 7:53 PM, SZEDER Gábor  wrote:
> On Fri, Feb 9, 2018 at 12:01 PM, Nguyễn Thái Ngọc Duy  
> wrote:
>> The new completable options are:
>>
>> --directory
>> --exclude
>> --gpg-sign
>> --include
>> --keep-cr
>> --keep-non-patch
>> --message-id
>> --no-keep-cr
>> --patch-format
>> --quiet
>> --reject
>> --resolvemsg=
>>
>> In-progress options like --continue will be part of --git-completion-helper
>> then filtered out by _git_am() unless the operation is in progress. This
>> helps keep marking of these operations in just one place.
>>
>> Signed-off-by: Nguyễn Thái Ngọc Duy 
>> ---
>>  contrib/completion/git-completion.bash | 11 ---
>>  parse-options.h|  4 ++--
>>  rerere.h   |  3 ++-
>>  3 files changed, 8 insertions(+), 10 deletions(-)
>>
>> diff --git a/contrib/completion/git-completion.bash 
>> b/contrib/completion/git-completion.bash
>> index 1e0bd835fe..eba482eb9c 100644
>> --- a/contrib/completion/git-completion.bash
>> +++ b/contrib/completion/git-completion.bash
>> @@ -1105,12 +1105,13 @@ __git_count_arguments ()
>>  }
>>
>>  __git_whitespacelist="nowarn warn error error-all fix"
>> +__git_am_inprogress_options="--skip --continue --resolved --abort"
>>
>>  _git_am ()
>>  {
>> __git_find_repo_path
>> if [ -d "$__git_repo_path"/rebase-apply ]; then
>> -   __gitcomp "--skip --continue --resolved --abort"
>> +   __gitcomp "$__git_am_inprogress_options"
>> return
>> fi
>> case "$cur" in
>> @@ -1119,12 +1120,8 @@ _git_am ()
>> return
>> ;;
>> --*)
>> -   __gitcomp "
>> -   --3way --committer-date-is-author-date --ignore-date
>> -   --ignore-whitespace --ignore-space-change
>> -   --interactive --keep --no-utf8 --signoff --utf8
>> -   --whitespace= --scissors
>> -   "
>> +   __gitcomp_builtin am "--no-utf8" \
>> +   "$__git_am_inprogress_options"
>> return
>> esac
>>  }
>> diff --git a/parse-options.h b/parse-options.h
>> index 3c32401736..009cd863e5 100644
>> --- a/parse-options.h
>> +++ b/parse-options.h
>> @@ -144,8 +144,8 @@ struct option {
>>  #define OPT_STRING_LIST(s, l, v, a, h) \
>> { OPTION_CALLBACK, (s), (l), (v), (a), \
>>   (h), 0, _opt_string_list }
>> -#define OPT_UYN(s, l, v, h) { OPTION_CALLBACK, (s), (l), (v), NULL, 
>> \
>> - (h), PARSE_OPT_NOARG, 
>> _opt_tertiary }
>> +#define OPT_UYN(s, l, v, h, f)  { OPTION_CALLBACK, (s), (l), (v), NULL, 
>> \
>> + (h), PARSE_OPT_NOARG|(f), 
>> _opt_tertiary }
>>  #define OPT_DATE(s, l, v, h) \
>> { OPTION_CALLBACK, (s), (l), (v), N_("time"),(h), 0,\
>>   parse_opt_approxidate_cb }
>
> Shouldn't this hunk go into a commit of its own?  Or at least it would
> deserve a mention in the commit message.

It's not a standalone change. It is used by the OPT_RERERE_AUTOUPDATE
below, which in turn is used by git-add. Together, --rerere-autoupdate
is removed from the completion list of git-add (and also a few more
commands).

Now that you mention it, the only command that completes
--rerere-autoupdate is git-merge. Since this is "auto" I don't think
people want to type manually. Maybe I should separate these changes
_and_ remove --rerere-autoupdate from _git_merge() too? At least that
it will be consistent that way.

>> diff --git a/rerere.h b/rerere.h
>> index c2961feaaa..5e5a312e4c 100644
>> --- a/rerere.h
>> +++ b/rerere.h
>> @@ -37,6 +37,7 @@ extern void rerere_clear(struct string_list *);
>>  extern void rerere_gc(struct string_list *);
>>
>>  #define OPT_RERERE_AUTOUPDATE(v) OPT_UYN(0, "rerere-autoupdate", (v), \
>> -   N_("update the index with reused conflict resolution if possible"))
>> +   N_("update the index with reused conflict resolution if possible"), \
>> +   PARSE_OPT_NOCOMPLETE)
>>
>>  #endif
>> --
>> 2.16.1.207.gedba492059
>>



-- 
Duy

[PATCH v3 1/2] Fix misconversion of gitsubmodule.txt

[marmot1123]

In the 2nd and 4th paragraph of DESCRIPTION, there ware misconversions 
`submodule’s`.
It seems non-ASCII apostrophes, so I rewrite ASCII apostrophes.

Signed-off-by: Motoki Seki 
---
 Documentation/gitsubmodules.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/gitsubmodules.txt b/Documentation/gitsubmodules.txt
index 46cf120f666df..0d59ab4cdfb1c 100644
--- a/Documentation/gitsubmodules.txt
+++ b/Documentation/gitsubmodules.txt
@@ -24,7 +24,7 @@ On the filesystem, a submodule usually (but not always - see 
FORMS below)
 consists of (i) a Git directory located under the `$GIT_DIR/modules/`
 directory of its superproject, (ii) a working directory inside the
 superproject's working directory, and a `.git` file at the root of
-the submodule’s working directory pointing to (i).
+the submodule's working directory pointing to (i).
 
 Assuming the submodule has a Git directory at `$GIT_DIR/modules/foo/`
 and a working directory at `path/to/bar/`, the superproject tracks the
@@ -33,7 +33,7 @@ in its `.gitmodules` file (see linkgit:gitmodules[5]) of the 
form
 `submodule.foo.path = path/to/bar`.
 
 The `gitlink` entry contains the object name of the commit that the
-superproject expects the submodule’s working directory to be at.
+superproject expects the submodule's working directory to be at.
 
 The section `submodule.foo.*` in the `.gitmodules` file gives additional
 hints to Gits porcelain layer such as where to obtain the submodule via

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

[PATCH v3 2/2] Replace non-ASCII apostrophes to ASCII single quotes in gitsubmodules.txt

[marmot1123]

Before this patch, there are several non-ASCII apostrophes in
gitsubmodules.txt, and misconverged at the 
https://git-scm.com/docs/gitsubmodules/ .
To make codes consistent, these non-ASCII apostrophes are replaced
with ASCII single quotes.  This patch also makes the document readable
on the website.

Signed-off-by: Motoki Seki 
---
 Documentation/gitsubmodules.txt | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/Documentation/gitsubmodules.txt b/Documentation/gitsubmodules.txt
index 0d59ab4cdfb1c..030c974c25606 100644
--- a/Documentation/gitsubmodules.txt
+++ b/Documentation/gitsubmodules.txt
@@ -132,27 +132,27 @@ using older versions of Git.
 +
 It is possible to construct these old form repositories manually.
 +
-When deinitialized or deleted (see below), the submodule’s Git
+When deinitialized or deleted (see below), the submodule's Git
 directory is automatically moved to `$GIT_DIR/modules//`
 of the superproject.
 
  * Deinitialized submodule: A `gitlink`, and a `.gitmodules` entry,
-but no submodule working directory. The submodule’s git directory
+but no submodule working directory. The submodule's git directory
 may be there as after deinitializing the git directory is kept around.
 The directory which is supposed to be the working directory is empty instead.
 +
 A submodule can be deinitialized by running `git submodule deinit`.
 Besides emptying the working directory, this command only modifies
-the superproject’s `$GIT_DIR/config` file, so the superproject’s history
+the superproject's `$GIT_DIR/config` file, so the superproject's history
 is not affected. This can be undone using `git submodule init`.
 
  * Deleted submodule: A submodule can be deleted by running
 `git rm  && git commit`. This can be undone
 using `git revert`.
 +
-The deletion removes the superproject’s tracking data, which are
+The deletion removes the superproject's tracking data, which are
 both the `gitlink` entry and the section in the `.gitmodules` file.
-The submodule’s working directory is removed from the file
+The submodule's working directory is removed from the file
 system, but the Git directory is kept around as it to make it
 possible to checkout past commits without requiring fetching
 from another repository.

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