Re: Question about git log --merge option

[Andrey Hsiao]

Thanks Junio, I understand the option's meaning now :)

On Thu, Apr 14, 2016 at 4:37 AM, Junio C Hamano  wrote:
> Andrey Hsiao  writes:
>
>> Dear list:
>>
>> Just encountered the --merge option for git log.
>>
>> In the man page, it has the following explanation:
>> - After a failed merge, show refs that touch files having a conflict
>> and don't exist on all heads to merge.
>
> git log --merge [options] -- $paths
>
> is roughly the same as
>
> git log [options] HEAD...MERGE_HEAD -- $paths'
>
> where $paths' is $paths limited to those with conflicts.  You can
> further think of that as a rough equivalent of
>
> git log [options] ^X HEAD MERGE_HEAD -- $paths'
>
> where X is the merge base between the tips of these two branches:
>
> X---o---o---o---H
>  \
>   o---o---o---M
>
> And the commits among these ('o's, H and M in the picture), the ones
> that change any of the $paths' are shown.  If you further limit the
> output (e.g. with -n, or --since=), you may not see all of
> them, of course.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Migrating away from SHA-1?

[Theodore Ts'o]

On Tue, Apr 12, 2016 at 07:15:34PM -0400, David Turner wrote:
> 
> If SHA-1 is broken (in certain ways), someone *can* replace an
> arbitrary blob.  GPG does not help in this case, because the signature
> is over the commit object (which points to a tree, which eventually
> points to the blob), and the commit hasn't changed.  So the GPG
> signature will still verify.

The "in certain ways" is the critical bit.  The question is whether
you are trying to replace an arbitrary blob, or a blob that was
submitted under your control.

If you are trying to replace an arbitrary blob under the you need to
carry a preimage attack.  That means that given a particular hash, you
need to find another blob that has the same hash.  SHA-1 is currently
resistant against preimage attack (that is, you need to use brute
force, so the work factor is 2**159).  

If you are trying to replace an arbitrary blob which is under your
control, then all you need is a collision attack, and this is where
SHA-1 has been weakened.  It is now possible to find a collision with
a work factor of 2**69, instead of the requisite 2**80.

It was a MD5 collision which was involved with the Flame attack.
Someone (in probably the US or Isreali intelligence services)
submitted a Certificate Signing Request (CSR) to the Microsoft
Terminal Services Licensing server.  That CSR was under the control of
the attacker, and it resulted in a certificate where parts of the
certificate could be swapped out with the corresponding fields from
another CSR (which was not submitted to the Certifiying Authority)
which had the code signing bit set.

So in order to carry out this attack, not only did the (cough)
"unknown" attackers had to have come up with a collision, but the two
pieces of colliding blobs had to parsable a valid CSR's, one which had
to pass inspection by the automated CA signing authority, and the
other which had to contain the desired code signing bits set so the
attacker could sabotage an Iranian nuclear centrifuge.

OK, so how does this map to git?  First of all, from a collision
perspective, the two blobs have to map into valid C code, one of which
has to be innocuous enough such that any humans who review the patch
and/or git pull request don't notice anything wrong.  The second has
to contain whatever security backdoor the attacker is going to try to
introduce into the git tree.  Ideally this is also should pass muster
by humans who are inspecting the code, but if the attack is targetted
against a specific victim which is not likely to look at the code, it
might be okay if something like this:

#if 0  /* this is needed to make the hash collision work */
aev2Ein4Hagh8eimshood5aTeteiVo9hOhchohN6jiem6AiNEipeeR3Pie4ePaeJ
fo8eLa9ateeKie5VeG5eZuu2Sahqu1Ohai9ohGhuAevoot5OtohQuai7koo4IeTh
ohCefae4Ahkah0eiku2Efo0iuHai8ideaRooth8wVahlia0nuu1eeSh5oht1Kaer
aiJi4chunahK9oozpaiWu7viee5aiFahud6Ee2zieich1veKque6PhiaAit1shie
#endif

... was hidden in the middle of the replacement blob.  One would
*hope*, though, that if something like this appeared in a blob that
was being sent to the upstream repository, that even a sloppy github
pull request reviewer would notice.

That's because in this scenario, the attacker needs to be able to get
the first blob into the git tree first, which means they need to be
trusted enough to get the first blob in.  And so the question which
comes to mind is if you are that trusted (or if the git pull review
process is that crappy), might it not be easier to simply introduce an
obfuscated code that has a security weakness?  That is, something from
the Underhanded C contest, or an accidental buffer overrun, hopefully
one that isn't noticed by static code checkers.  If you do that, you
don't even need to figure out how to create a SHA-1 collision.

Does that mean that we shouldn't figure out how to migrate to another
hash function?  No, it's probably worth planning how to do it.  But we
probably have a fair amount of time to get this right.

Cheers,

- Ted
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Merge conflicts are reported relative to root not cwd

[Stefan Beller]

On Wed, Apr 13, 2016 at 3:41 PM, Jeff King  wrote:
> On Wed, Apr 13, 2016 at 03:18:24PM -0700, Stefan Beller wrote:
>
>> * This is your preference for whole-tree operations. What are
>>whole-tree operations? (Is there a concise definition?
>>Are submodules whole tree operations?)
>>These questions are motivated by origin/sb/submodule-path-misc-bugs
>>which a) fixes bugs and b) makes submodule handling consistent to the
>>relative-to-cwd philosophy. As most submodule commands touch all
>>submodules in the tree, we could argue it is a whole-tree operation, and
>>you'd like to see submodule paths from the root level, too.
>>
>> I'd like to avoid adding confusion here. So is there a an easy way to tell 
>> apart
>> which commands you would expect to use relative-to-cwd and which use
>> relative-to-root?
>
> I think some operations are fundamentally whole-tree. You do not merge a
> subtree, but create a new top-level commit. Similarly, even in:
>
>   cd Documentation
>   git log -p .
>
> the diffs we see still show the whole path. We are traversing the whole
> tree.

Oh I see.

cd dir-with-submodules
git submodule update .

would traverse only that dir-with-submodules/ subtree from the users
POV.

>
> If you are touching all submodules with an operation, I'd expect it to
> show full paths, not relative ones. But then I set status.relativePaths
> to "false", so maybe I am in the minority.

That would be `git submodule foreach`. Any other submodule subcommand
is similar to git log as they default to the whole tree but can do similar stuff
as "git log -- dir/" for sub trees.

Having subcommands behave differently w.r.t. path being relative or not
sounds like an inconsistency to me. Currently they are all relative,
i.e. `git submodule foreach` breaks your expectation for displaying paths.

>
> -Peff
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Ambiguous sha-1 during a rebase

[Mike Hommey]

On Wed, Apr 13, 2016 at 03:42:44PM -0700, Junio C Hamano wrote:
> Mike Hommey  writes:
> 
> > Should git-rebase use full sha-1s under the hood to avoid these type of
> > races?
> 
> It already should be doing so since Aug 2013, IIRC.

I'm using 2.8.1. Would there have been a regression?

Mike
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 0/2] Fix relative path issues in recursive submodules.

[Junio C Hamano]

Stefan Beller  writes:

> ... so what is your concern for holding instead of merging?

Nothing specific.  Remember, I may be aware of all the discussion
threads but I am certainly not reading every word in every thread.
When the participants are trustworthy enough, instead of going back
to the list archive (and risk overlooking a message that asks
"please hold off merging this, I have another last-minute update")
to see if everything known has been resolved in a satisfactory way
myself, I'd just ask, which is more efficient _and_ reliable.





--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Ambiguous sha-1 during a rebase

[Junio C Hamano]

Mike Hommey  writes:

> Should git-rebase use full sha-1s under the hood to avoid these type of
> races?

It already should be doing so since Aug 2013, IIRC.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Merge conflicts are reported relative to root not cwd

[Jeff King]

On Wed, Apr 13, 2016 at 03:18:24PM -0700, Stefan Beller wrote:

> * This is your preference for whole-tree operations. What are
>whole-tree operations? (Is there a concise definition?
>Are submodules whole tree operations?)
>These questions are motivated by origin/sb/submodule-path-misc-bugs
>which a) fixes bugs and b) makes submodule handling consistent to the
>relative-to-cwd philosophy. As most submodule commands touch all
>submodules in the tree, we could argue it is a whole-tree operation, and
>you'd like to see submodule paths from the root level, too.
> 
> I'd like to avoid adding confusion here. So is there a an easy way to tell 
> apart
> which commands you would expect to use relative-to-cwd and which use
> relative-to-root?

I think some operations are fundamentally whole-tree. You do not merge a
subtree, but create a new top-level commit. Similarly, even in:

  cd Documentation
  git log -p .

the diffs we see still show the whole path. We are traversing the whole
tree.

If you are touching all submodules with an operation, I'd expect it to
show full paths, not relative ones. But then I set status.relativePaths
to "false", so maybe I am in the minority.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Merge conflicts are reported relative to root not cwd

[Junio C Hamano]

Stefan Beller  writes:

> *  What are
>whole-tree operations?

"git merge" does not let you merge "changes just in my current
directory".  You only merge the whole tree, and you can get
conflicts from all over the tree, not just in your current
directory.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Default authentication over https?

[Jeff King]

On Mon, Apr 11, 2016 at 12:04:02PM -0400, Isaac Levy wrote:

> I use a git server which requires authentication over https. Git seems
> determined to always try an unauthenticated request first, slowing
> down operations by a couple seconds.
> 
> Is there a way to configure git to default to authenticated requests?  Thanks!

No, there isn't. Very old versions of git would ask for the password if
you provided a username, but since v1.7.8 we only do so in response to
an HTTP 401. The code is still there to do the "proactive" asking, but
it's not wired up to any particular config option.

However, I don't think even that would give you what you want. Because I
think that even if we provide a credential, curl will make an initial
request (presumably to find out which auth type it should use, but that
is just a guess). I don't know if there is a way to convince curl to
stick the credential in the first request (if my guess is right, then
perhaps by setting the auth type explicitly, or even by sticking in our
own Authorization header).

So I think the answer for now is "no", but it might be possible (and not
even that hard) to do with a patch.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 0/2] Fix relative path issues in recursive submodules.

[Stefan Beller]

On Wed, Apr 13, 2016 at 3:23 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>>
>> Ok fixed this instance here, too.
>
> ... So... should I hold sb/submodule-helper-clone-regression-fix?
> It has been marked to be merged to 'next' for some time now.

(Both things Ramsay pointed at are in submodule.c, but
sb/submodule-helper-clone-regression-fix never touches that file,
so Ramsay talks about submodule-init here? I agree
submodule-init is faulty and I am fixing/rewriting it now.)

This series (sb/submodule-helper-clone-regression-fix), has no issues
(on its own as well as playing together with any other submodule
series in flight IIUC), so what is your concern for holding instead of merging?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Ambiguous sha-1 during a rebase

[Mike Hommey]

Hi,

Something interesting happened to me. I was in the middle of an
interactive rebase, and after a --continue, I got:

error: short SHA1 e34ff55 is ambiguous.
fatal: Needed a single revision
Invalid commit name: e34ff55

One thing that happened, is that, while running that interactive rebase,
I /also/ did a `git remote update` from an other shell, which, I guess,
happened to have imported another object that made e34ff55 ambiguous.

Should git-rebase use full sha-1s under the hood to avoid these type of
races?

Relatedly, having looked up the ambiguity, it turns out the other object
that fits the same short sha1 is a tree... maybe git should be able to
disambiguate in that case, since it was looking for a commit, and
there's only one commit with that short sha1?

Mike
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: clone hang prevention / timeout?

[Jeff King]

On Mon, Apr 11, 2016 at 10:49:19PM +0100, Jason Vas Dias wrote:

> Is there any option I can specify to get the clone to timeout, or do I 
> manually
> have to strace the git process and send it a signal after a hang is detected?

Oh, one other thing you might consider, it something like "timeout" from
GNU coreutils, which puts a hard cap on the length of time a process can
run.

It's totally unaware of the state of the process, though, so if you
really do have a clone which takes an hour, it might very well kill it
at 99% complete. It has no mechanism for "gee, this process looks like
it hasn't done anything for 5 minutes".

I don't know offhand of a general tool for that.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: clone hang prevention / timeout?

[Jeff King]

On Mon, Apr 11, 2016 at 10:49:19PM +0100, Jason Vas Dias wrote:

> It appears GIT has no way of specifying a timeout for a clone operation -
> if the server decides not to complete a get request, the clone can
> hang forever -
> is this correct ?

Yes. Git's protocol has no timeouts, though each side is generally
either writing or reading at any moment, and so an interrupted
connection should cause either EPIPE or EOF, ending the process. The
exceptions I have seen are:

 - protocol / implementation bugs that cause a true deadlock. At this
   we've fixed all known cases, but that doesn't mean there aren't bugs
   lurking.

 - the network drops out in such a way that the OS doesn't realize the
   connection is gone, and the reading side is left waiting for input
   forever

I think the TCP keepalive stuff that Eric mentioned should address the
latter, though I don't know how well it works in practice. We used to
sometimes see processes hung for days on GitHub, but it's been a long
time. I don't recall if it was pre-v1.8.5 (which introduced
SO_KEEPALIVE), or if we made some other change (we have a load-balancing
layer in front that has more aggressive timeouts).

> This appears to be what I am seeing, in a script that is attempting to do many
> successive clone operations, eg. of
> git://anongit.freedesktop.org/xorg/* , the script
> occasionally hangs in a clone - I can see with netstat + strace that the TCP
> connection is open and GIT is trying to read .
> Is there any option I can specify to get the clone to timeout, or do I 
> manually
> have to strace the git process and send it a signal after a hang is detected?

There are periods where a git client may have to wait for a while in
read() while the other side is quiet (e.g., when the other side is badly
packed and needs to do a lot of up-front CPU work to prepare the
packfile). Since v1.8.4.2, the server side of a clone should generate
application-level keepalive packets, so that the client never sees
silence for more than ~5 seconds. The freedesktop servers appear to be
on v2.1.4, so a long read() as you're seeing probably is a real hang.

Note that pushing has a similar problem (the client may wait a long time
while the server chews on the uploaded packfile before reporting
status). There are no keepalives in that direction, though I have a
series there that I need to polish and submit.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 0/2] Fix relative path issues in recursive submodules.

[Junio C Hamano]

Stefan Beller  writes:

>
> Ok fixed this instance here, too.

... So... should I hold sb/submodule-helper-clone-regression-fix?
It has been marked to be merged to 'next' for some time now.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Merge conflicts are reported relative to root not cwd

[Stefan Beller]

On Wed, Apr 13, 2016 at 2:58 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> $ cd t/
>> $ git merge ...
>> ...
>> Auto-merging builtin/submodule--helper.c
>> Auto-merging builtin/fetch.c
>> CONFLICT (content): Merge conflict in builtin/fetch.c
>> Auto-merging builtin/clone.c
>> Auto-merging README.md
>> ...
>>
>> It should say ../builtin/fetch.c IMHO.
>> Any reason to keep the old behavior?
>
> I actually prefer to see the "relative to root" behaviour when it
> comes to things like this, that lets you view the things that happen
> in the whole-tree context.
>
> I would have to go insane before I start a whole-tree operation like
> "git merge" from deep in my tree, but if I happened to do that, e.g.
>
> cd perl/blib/lib/Git/SVN/Memoize
> git merge other-branch
>
> I'd rather see that the conflicted path, e.g. builtin/fetch.c,
> reported by showing it like the above output, not happening in
> ../../../../../../builtin/fetch.c which I have to count the
> up-dots to know which file it is talking about.
>

* In most trees you would still know which file is referred to, as
   there are no /$PATH/builtin/fetch.c files except for PATH=
   So I'd see that as a minor issue.

* This is your preference for whole-tree operations. What are
   whole-tree operations? (Is there a concise definition?
   Are submodules whole tree operations?)
   These questions are motivated by origin/sb/submodule-path-misc-bugs
   which a) fixes bugs and b) makes submodule handling consistent to the
   relative-to-cwd philosophy. As most submodule commands touch all
   submodules in the tree, we could argue it is a whole-tree operation, and
   you'd like to see submodule paths from the root level, too.

I'd like to avoid adding confusion here. So is there a an easy way to tell apart
which commands you would expect to use relative-to-cwd and which use
relative-to-root?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 15/16] branch: use ref-filter printing APIs

[Jeff King]

On Thu, Apr 14, 2016 at 12:31:41AM +0530, Karthik Nayak wrote:

> On Wed, Apr 13, 2016 at 8:42 PM, Junio C Hamano  wrote:
> >>> Having said that, doesn't this need to be further adjusted for
> >>> 95c38fb0 (branch: fix shortening of non-remote symrefs, 2016-04-03)?
> >>>
> >>> http://thread.gmane.org/gmane.comp.version-control.git/290622/focus=290624
> >>>
> >>
> >> That was one of the changes made in this version of the patch series :)
> >
> > But with this patch applied, it seems that the tests in Peff's fix
> > does not seem to pass.
> > If I understand correctly, "fix shortening" stops doing your
> > symref:short (which is to
> > shorten the usual "drop refs/heads, refs/remotes, etc.") and makes the
> > shortening
> > conditional. The target of a symref that is found in refs/heads/ gets
> > refs/heads and
> > nothing else stripped.
> 
> Having a look here, WRT to the patch v4 it seems the problem is that
> patch v4 doesn't support v2.6.x behavior, namely that cross-prefix symrefs 
> will
> not be shortened at all. As per the format given in the last patch
> [16/16] it shortens
> the symref irrespective of being cross-prefix symrefs.
> 
> Would it be a good idea to enforce this as in v2.6.x or change it as
> to allow shortening
> of cross-prefix symrefs.

The cross-prefix behavior I put into the test is not something I feel
strongly about; it was mostly just restoring the earlier behavior. I do
think shortening everything is fine, too, as long as there's some way to
get the fully qualified ref. E.g., if `git branch` shows %(symref:short)
or %(symref:strip=2), by default, but you can ask for just %(symref) if
you want (which I think is how you have it implemented now, though I
notice that symrefs don't get nearly as many formatting options as
things like %(upstream)).

If anyone is machine-parsing git-branch output in the first place, they
are Doing It Wrong. And doubly so if they are relying on some obscure
shortening rules that I don't think were ever carefully designed. So I
think we should be free to change it here to what serves users best.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: credentials are being erased from credential manager via git credential-wincred erase command

[Jeff King]

On Wed, Apr 13, 2016 at 01:12:06PM -0600, Bryan Turner wrote:

> From the credentials code in Git (which I could be mis-reading; I'm
> sure others on the list will correct me if I'm wrong), it appears the
> erase is done after a cached credential is rejected by the server.
> That implies that, periodically, authentication with your Stash server
> is failing and that that failed authentication results in Git clearing
> the "bad" credential. That's likely why you see this happen on
> seemingly random builds.

Yes, that's right. For HTTP, Git will erase the credential only for an
HTTP 401 (or a 407 for the proxy credential). So an intermittent failure
shouldn't cause us to erase the credential there.

But it's possible that a server whose credentials are backed by
something more complicated than a password file (e.g., LDAP or
something) may return a 401 intermittently when the backend process
fails (e.g., connection to the LDAP server fails). And I agree with your
second paragraph (that I snipped) that finding the intermittent failure
is the best first step.

We could potentially teach Git _not_ to erase credentials in such a case
(with a config option). But the downside would be that the user would
then have to manually erase and re-populate the credentials if they do
change.

IMHO, that responsibility really lies with the credential helpers
themselves. Helpers like git-credential-wincred are meant to
transparently cache and update credentials. I think for an automated
process like this, what the user really wants is more like "I'll stick
some credentials in a secure place, and I want Git _only_ to access
them, never update them".

You can hack together something like that today with:

  git config credential.helper '!f() {
case "$1" in
  get|store) git credential-wincred "$@" ;;
esac
  }; f'

and then you can populate it with:

  echo url=https://example.com |
  git credential fill |
  git credential approve

The "fill" will prompt you and generate the proper response to feed to
"approve", which will actually store it in your helper of choice. Or you
can just do a "fetch" from the repository in question, and it will
happen automatically.

If this pattern is something a lot of people want to use, I think we
could wrap that shell snippet into a "git credential-static" or
something that chained to another helper, and you'd configure it like:

  git config credential.helper 'static wincred'

or something.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Why doesn't gitk highlight commit references from git-describe?

[Stephen Kelly]

Stephen Kelly wrote:

>  cmake describe --contains


Oops, I mean git describe --contains of course.

Thanks,

Steve.


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Merge conflicts are reported relative to root not cwd

[Junio C Hamano]

Stefan Beller  writes:

> $ cd t/
> $ git merge ...
> ...
> Auto-merging builtin/submodule--helper.c
> Auto-merging builtin/fetch.c
> CONFLICT (content): Merge conflict in builtin/fetch.c
> Auto-merging builtin/clone.c
> Auto-merging README.md
> ...
>
> It should say ../builtin/fetch.c IMHO.
> Any reason to keep the old behavior?

I actually prefer to see the "relative to root" behaviour when it
comes to things like this, that lets you view the things that happen
in the whole-tree context.

I would have to go insane before I start a whole-tree operation like
"git merge" from deep in my tree, but if I happened to do that, e.g.

cd perl/blib/lib/Git/SVN/Memoize
git merge other-branch

I'd rather see that the conflicted path, e.g. builtin/fetch.c,
reported by showing it like the above output, not happening in
../../../../../../builtin/fetch.c which I have to count the
up-dots to know which file it is talking about.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Why doesn't gitk highlight commit references from git-describe?

[Stephen Kelly]

Stefan Beller wrote:

> We also want to have 4b9ab0ee0130~1^2 to work `right`, in the sense
> that not just the hexadecimals are highlighted and linking to
> 4b9ab0ee0130, but the whole expression should link to 49e863b02ae177.

Presumably the same logic which finds 4b9ab0ee0130 to link it can also see 
if it is suffixed with '~1^2' ?

Is a ref like 4b9ab0ee0130~1^2 commonly useful? In cmake we use the output 
of cmake describe --contains (when there is a following tag) to refer to 
commits, in a pattern which I've also seen in git.git occasionally:

 https://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=23f3798c

I think the only reason for using the output of cmake describe --contains is 
that it shows the reader the 'era' of the commit (and release it appears in) 
without having to look it up. I'm not really aware of another good reason to 
use it, but I think that's enough to make sense.

However I'm not sure I understand why anyone would refer to 4b9ab0ee0130~1^2 
instead of 49e863b0 (or perhaps v2.6.5~12, depending on whether the tag is 
there).

>> What does 'HEAD^' mean? If it is 'the commit before this one', then why
>> not link it?
> 
> As said I was thinking about the git development

> I do not think we would want to link HEAD to anything in that example.
> (I'd have no idea what it would link to here, so just not link it?)

Right, so if a commit message contains something like 

 Make git rebase -i HEAD ten times better

then HEAD shouldn't become a link. Makes sense to me.

Thanks,

Steve.


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Why doesn't gitk highlight commit references from git-describe?

[Junio C Hamano]

Stefan Beller  writes:

> But it should not be just tags?
>
> We also want to have 4b9ab0ee0130~1^2 to work `right`,

I'd consider that just "crazy", though.  I'd be just happy to see
4b9ab0ee0130 highlighted and lead to the named commit, i.e. as long
as ~1^2 is not part of the link, it is sufficient.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] git-p4.py: Make submit working on bare repository

[Junio C Hamano]

Amadeusz Żołnowski  writes:

>> Has anything happened to this topic after this?  I am wondering if I
>> should discard the topic az/p4-bare-no-rebase without prejudice.
>
> Sorry, I haven't got time to take a loot at this, but I'll return to
> that soon, OK? I'll prepare a patch with an option to skip rebase rather
> than do it only for bare repos.

No hurries.

With or without an option, I think the documentation needs to
clarify when it is safe to omit rebase and why.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Why doesn't gitk highlight commit references from git-describe?

[Stefan Beller]

On Wed, Apr 13, 2016 at 2:32 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> How would you know(/code) that v1.6.0-rc0~120^2 is a text worth linking?
>> "v1.6.0-rc0" is a custom string as that is how we name tags in this project.
>> It can follow any convention in other projects.
>>
>> Maybe a first approximation is if there is a `~` followed by numbers
>> or a ^ character, inspect the whole thing if it is a reference into the 
>> history?
>
> You (as a gitk process running in a repository) know what tags are
> in your repository, so you can find the above pattern and see if the
> prefix matches any of the known tag.  That way, you do not have to
> worry about having to special case HEAD etc.

Sorry for shooting from the hip here[1], that thought was

But it should not be just tags?

We also want to have 4b9ab0ee0130~1^2 to work `right`, in the sense
that not just the hexadecimals are highlighted and linking to 4b9ab0ee0130,
but the whole expression should link to 49e863b02ae177.

[1] My thinking was just like in https://xkcd.com/761/

Stephen Kelly:
> Would it be possible to implement linking for  optionally followed
> by something like that? Just tags should be links too, right?

right, just tags should work I'd expect.

> What does 'HEAD^' mean? If it is 'the commit before this one', then why not
> link it?

As said I was thinking about the git development, so see 5f3c3a4e6f11deda
for an example:

files_log_ref_write: new function

Because HEAD and stash are per-worktree, every refs backend needs to
go through the files backend to write these refs.

So create a new function, files_log_ref_write, and add it to
refs/refs-internal.h. Later, we will use this to handle reflog updates
for per-worktree symbolic refs (HEAD).

I do not think we would want to link HEAD to anything in that example.
(I'd have no idea what it would link to here, so just not link it?)

Thanks,
Stefan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Merge conflicts are reported relative to root not cwd

[Stefan Beller]

$ cd t/
$ git merge ...
...
Auto-merging builtin/submodule--helper.c
Auto-merging builtin/fetch.c
CONFLICT (content): Merge conflict in builtin/fetch.c
Auto-merging builtin/clone.c
Auto-merging README.md
...

It should say ../builtin/fetch.c IMHO.
Any reason to keep the old behavior?

Thanks,
Stefan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Why doesn't gitk highlight commit references from git-describe?

[Junio C Hamano]

Stefan Beller  writes:

> How would you know(/code) that v1.6.0-rc0~120^2 is a text worth linking?
> "v1.6.0-rc0" is a custom string as that is how we name tags in this project.
> It can follow any convention in other projects.
>
> Maybe a first approximation is if there is a `~` followed by numbers
> or a ^ character, inspect the whole thing if it is a reference into the 
> history?

You (as a gitk process running in a repository) know what tags are
in your repository, so you can find the above pattern and see if the
prefix matches any of the known tag.  That way, you do not have to
worry about having to special case HEAD etc.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Why doesn't gitk highlight commit references from git-describe?

[Stefan Beller]

On Wed, Apr 13, 2016 at 1:36 PM, Stephen Kelly  wrote:
> Hi,
>
> If I look at git commit 89ea90351dd32fbe384d0cf844640a9c55606f3b in gitk, it
> does not linkify the v1.6.0-rc0~120^2 in the commit message.
>
> Is there any reason for that, or can gitk be changed?

Sure it can be changed. Go for it.

I think it is hard though. So for example it is easy to spot sha1s
and link them (see a6ee883b8e as an example picking up ebef7e5
as a link.)

How would you know(/code) that v1.6.0-rc0~120^2 is a text worth linking?
"v1.6.0-rc0" is a custom string as that is how we name tags in this project.
It can follow any convention in other projects.

Maybe a first approximation is if there is a `~` followed by numbers
or a ^ character, inspect the whole thing if it is a reference into the history?

(Special case for git.git: Sometimes in a discussion you want to explain stuff
and may use HEAD^ or such to demonstrate the use case. Other projects would
not use that as much in descriptive text I would assume. So we'd need
to make sure
changing refs (i.e. branches, symbolic refs such as HEAD, FETCH_HEAD) are not
considered worth linkifying.)

Thanks,
Stefan

>
> Thanks,
>
> Steve.
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 0/2] Fix relative path issues in recursive submodules.

[Junio C Hamano]

Ramsay Jones  writes:

> Also, I note that t7406-submodule-update.sh test #4 is failing.
> (looks like absolute vs relative paths)

I think that is $gmane/291363

http://thread.gmane.org/gmane.comp.version-control.git/291334/focus=291363
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] git-p4.py: Make submit working on bare repository

[Amadeusz Żołnowski]

Hi,

Junio C Hamano  writes:
> Has anything happened to this topic after this?  I am wondering if I
> should discard the topic az/p4-bare-no-rebase without prejudice.

Sorry, I haven't got time to take a loot at this, but I'll return to
that soon, OK? I'll prepare a patch with an option to skip rebase rather
than do it only for bare repos.

-- 
Amadeusz Żołnowski


signature.asc
Description: PGP signature

Why doesn't gitk highlight commit references from git-describe?

[Stephen Kelly]

Hi,

If I look at git commit 89ea90351dd32fbe384d0cf844640a9c55606f3b in gitk, it 
does not linkify the v1.6.0-rc0~120^2 in the commit message. 

Is there any reason for that, or can gitk be changed?

Thanks,

Steve.


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Question about git log --merge option

[Junio C Hamano]

Andrey Hsiao  writes:

> Dear list:
>
> Just encountered the --merge option for git log.
>
> In the man page, it has the following explanation:
> - After a failed merge, show refs that touch files having a conflict
> and don't exist on all heads to merge.

git log --merge [options] -- $paths

is roughly the same as

git log [options] HEAD...MERGE_HEAD -- $paths'

where $paths' is $paths limited to those with conflicts.  You can
further think of that as a rough equivalent of

git log [options] ^X HEAD MERGE_HEAD -- $paths'

where X is the merge base between the tips of these two branches:

X---o---o---o---H
 \
  o---o---o---M

And the commits among these ('o's, H and M in the picture), the ones
that change any of the $paths' are shown.  If you further limit the
output (e.g. with -n, or --since=), you may not see all of
them, of course.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 0/2] Fix relative path issues in recursive submodules.

[Stefan Beller]

On Wed, Apr 13, 2016 at 12:21 PM, Ramsay Jones
 wrote:
>
>
> On 12/04/16 16:58, Stefan Beller wrote:
>> On Fri, Apr 1, 2016 at 7:41 AM, Ramsay Jones
>>  wrote:
>>>
> [snip[
>
 - }
 + sm_gitdir_rel = strbuf_detach(, NULL);
>>>
>>> ... this is good, but ...
>>>
 + sm_gitdir = absolute_path(sm_gitdir_rel);

   if (!is_absolute_path(path)) {
 - /*
 -  * TODO: add prefix here once we allow calling from non root
 -  * directory?
 -  */
 - strbuf_addf(, "%s/%s",
 - get_git_work_tree(),
 - path);
 + strbuf_addf(, "%s/%s", get_git_work_tree(), path);
   path = strbuf_detach(, 0);
>>>
>>> ... can you please fix this up.
>>>
>>> Thanks!
>>>
>>> ATB,
>>> Ramsay Jones
>>
>> Looking at the current code of 
>> origin/sb/submodule-helper-clone-regression-fix
>> we do not have this issue there, but I'll keep it in mind for a resend.
>
> Hmm, actually, the above change wasn't the original culprit (as I thought), 
> but
> a different instance of the same fault. :-D
>
> I've lost track of which version is now in 'pu' (currently @ 45a4edc "Merge 
> branch
> 'sb/submodule-init' into pu"), but sparse is still warning:
>
>   SP submodule.c
>   submodule.c:256:43: warning: Using plain integer as NULL pointer
>
> So, the fix looks like:
>
>   diff --git a/submodule.c b/submodule.c
>   index 5d1238a..4cc1c27 100644
>   --- a/submodule.c
>   +++ b/submodule.c
>   @@ -253,7 +253,7 @@ const char *submodule_strategy_to_string(const struct 
> submodule_update_strategy
>   return NULL;
>   case SM_UPDATE_COMMAND:
>   strbuf_addf(, "!%s", s->command);
>   -   return strbuf_detach(, 0);
>   +   return strbuf_detach(, NULL);
>   }
>   return NULL;
>}
>
> Also, I note that t7406-submodule-update.sh test #4 is failing.
> (looks like absolute vs relative paths)
>
> ATB,
> Ramsay Jones
>

Ok fixed this instance here, too.
I'll hunt down the path issue now.

>
>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 0/2] Fix relative path issues in recursive submodules.

[Ramsay Jones]

On 12/04/16 16:58, Stefan Beller wrote:
> On Fri, Apr 1, 2016 at 7:41 AM, Ramsay Jones
>  wrote:
>>
[snip[

>>> - }
>>> + sm_gitdir_rel = strbuf_detach(, NULL);
>>
>> ... this is good, but ...
>>
>>> + sm_gitdir = absolute_path(sm_gitdir_rel);
>>>
>>>   if (!is_absolute_path(path)) {
>>> - /*
>>> -  * TODO: add prefix here once we allow calling from non root
>>> -  * directory?
>>> -  */
>>> - strbuf_addf(, "%s/%s",
>>> - get_git_work_tree(),
>>> - path);
>>> + strbuf_addf(, "%s/%s", get_git_work_tree(), path);
>>>   path = strbuf_detach(, 0);
>>
>> ... can you please fix this up.
>>
>> Thanks!
>>
>> ATB,
>> Ramsay Jones
> 
> Looking at the current code of origin/sb/submodule-helper-clone-regression-fix
> we do not have this issue there, but I'll keep it in mind for a resend.

Hmm, actually, the above change wasn't the original culprit (as I thought), but
a different instance of the same fault. :-D

I've lost track of which version is now in 'pu' (currently @ 45a4edc "Merge 
branch
'sb/submodule-init' into pu"), but sparse is still warning:

  SP submodule.c
  submodule.c:256:43: warning: Using plain integer as NULL pointer

So, the fix looks like:

  diff --git a/submodule.c b/submodule.c
  index 5d1238a..4cc1c27 100644
  --- a/submodule.c
  +++ b/submodule.c
  @@ -253,7 +253,7 @@ const char *submodule_strategy_to_string(const struct 
submodule_update_strategy
  return NULL;
  case SM_UPDATE_COMMAND:
  strbuf_addf(, "!%s", s->command);
  -   return strbuf_detach(, 0);
  +   return strbuf_detach(, NULL);
  }
  return NULL;
   }

Also, I note that t7406-submodule-update.sh test #4 is failing.
(looks like absolute vs relative paths)

ATB,
Ramsay Jones



--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: credentials are being erased from credential manager via git credential-wincred erase command

[Bryan Turner]

Ken,

I'm one of the developers for Atlassian Bitbucket Server (formerly
Stash), which I believe you're running.

>From the credentials code in Git (which I could be mis-reading; I'm
sure others on the list will correct me if I'm wrong), it appears the
erase is done after a cached credential is rejected by the server.
That implies that, periodically, authentication with your Stash server
is failing and that that failed authentication results in Git clearing
the "bad" credential. That's likely why you see this happen on
seemingly random builds.

To follow up on the possibility that authentication with Stash is
periodically failing, I'd recommend opening a support request at
support.atlassian.com. My belief is that the remote Git client is
doing what it's intended to do in response to an authentication
failure. That suggests we need to look at your Stash instance to
determine why authentication is failing. If you do open a support
request, please mention me in your description so that our support
engineers can attach me to the issue!

Best regards,
Bryan Turner

On Wed, Apr 13, 2016 at 12:49 PM, O'Connell, Ken
 wrote:
> Good afternoon,
>
> My company setup wincred for management of our git/stash user credentials for 
> our automated build scripts. It works great for days, sometimes a couple 
> weeks.
> Then one day build haven't run and we discover the scripts are randomly found 
> at a user prompt awaiting stash user credentials.
>
> We look at Windows Credential Manager store in Windows and find the Stash 
> user credentials are erased from Credential Manager!
>
> We discovered via GIT_TRACE, that the command used to retrieve the 
> credentials, is being followed up by a git command to erase the credentials. 
> -not all the time, but seemingly in a random way.
> Looking at the trace logs on one server, we see the following commands:
> 'git-remote-https' 'origin' https://stash-server/bla/bla/bla.git
> 'git-credential-wincred' 'get'
> 'git credential-wincred erase'
>
> Do you have ideas on how to track down the root cause of why this command is 
> running?
>
> Info:
> Windows 7 machines
> Git version 1.9.5-msysgit, and Git 2.7.4 windows (on one machine to see if 
> upgrading helped) -it did not.
> Stash version 3.11.2
>
> Please let me know if I can get any more information to help diagnose.
> Thanks,
> -Ken
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 15/16] branch: use ref-filter printing APIs

[Karthik Nayak]

On Wed, Apr 13, 2016 at 8:42 PM, Junio C Hamano  wrote:
>>> Having said that, doesn't this need to be further adjusted for
>>> 95c38fb0 (branch: fix shortening of non-remote symrefs, 2016-04-03)?
>>>
>>> http://thread.gmane.org/gmane.comp.version-control.git/290622/focus=290624
>>>
>>
>> That was one of the changes made in this version of the patch series :)
>
> But with this patch applied, it seems that the tests in Peff's fix
> does not seem to pass.
> If I understand correctly, "fix shortening" stops doing your
> symref:short (which is to
> shorten the usual "drop refs/heads, refs/remotes, etc.") and makes the
> shortening
> conditional. The target of a symref that is found in refs/heads/ gets
> refs/heads and
> nothing else stripped.

Having a look here, WRT to the patch v4 it seems the problem is that
patch v4 doesn't support v2.6.x behavior, namely that cross-prefix symrefs will
not be shortened at all. As per the format given in the last patch
[16/16] it shortens
the symref irrespective of being cross-prefix symrefs.

Would it be a good idea to enforce this as in v2.6.x or change it as
to allow shortening
of cross-prefix symrefs.

-- 
Regards,
Karthik Nayak
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

credentials are being erased from credential manager via git credential-wincred erase command

[O'Connell, Ken]

Good afternoon,

My company setup wincred for management of our git/stash user credentials for 
our automated build scripts. It works great for days, sometimes a couple weeks. 
Then one day build haven't run and we discover the scripts are randomly found 
at a user prompt awaiting stash user credentials. 

We look at Windows Credential Manager store in Windows and find the Stash user 
credentials are erased from Credential Manager! 

We discovered via GIT_TRACE, that the command used to retrieve the credentials, 
is being followed up by a git command to erase the credentials. -not all the 
time, but seemingly in a random way.
Looking at the trace logs on one server, we see the following commands:
'git-remote-https' 'origin' https://stash-server/bla/bla/bla.git
'git-credential-wincred' 'get'
'git credential-wincred erase'

Do you have ideas on how to track down the root cause of why this command is 
running?  

Info:
Windows 7 machines
Git version 1.9.5-msysgit, and Git 2.7.4 windows (on one machine to see if 
upgrading helped) -it did not.
Stash version 3.11.2

Please let me know if I can get any more information to help diagnose.
Thanks,
-Ken



--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 03/16] index-helper: new daemon for caching index and related stuff

[David Turner]

On Wed, 2016-04-13 at 20:43 +0700, Duy Nguyen wrote:
> On Wed, Apr 13, 2016 at 7:32 AM, David Turner <
> dtur...@twopensource.com> wrote:
> > +NOTES
> > +-
> > +
> > +$GIT_DIR/index-helper.path is a symlink
> 
> In multiple worktree context, this file will be per-worktree. So we
> have one daemon per worktree. I think that's fine.
> 
> > to a directory in $TMPDIR
> > +containing a Unix domain socket called 's' that the daemon reads
> > +commands from.
> 
> Oops. I stand corrected, now it's one daemon per repository...
> Probably good to hide the socket path in $GIT_DIR though, people may
> protect it with dir permission of one of ancestor directories.

I'm not sure I understand what you're saying here.  It should be one
daemon per worktree, I think.  And as far as I know, it is.  

Socket paths must be short (less than 104 chars on Mac).  That's why I
do the weird symlink-to-tmpdir thing.

> > The directory will also contain files named
> > +"git-index-".  These are used as backing stores for shared
> > +memory.  Normally the daemon will clean up these files when it
> > exits
> > +or when they are no longer relevant.  But if it crashes, some
> > objects
> > +could remain there and they can be safely deleted with "rm"
> > +command.
> 
> Alternatively, we could store all these in $GIT_DIR/helper or
> something and clean up automatically when index-helper starts. But I
> guess at least with TMPDIR we have a chance to put them on tmpfs.
> > +#define UNIX_PATH_MAX 92
> > +#endif
> > +
> 
> This looks like dead code (or at least not used in this patch).

Yep, thanks.

> > +   fd = unix_stream_connect(socket_path);
> > +   if (refresh_cache) {
> > +   ret = write_in_full(fd, "refresh", 8) != 8;
> 
> Since we've moved to unix socket and had bidirectional communication,
> it's probably a good idea to read an "ok" back, giving index-helper
> time to prepare the cache. As I recall the last discussion with
> Johannes, missing a cache here when the index is around 300MB could
> hurt more than wait patiently once and have it ready next time.

It is somewhat slower to wait for the daemon (which requires a disk
load + a memcpy) than it is to just load it ourselves (which is just a
disk load). 

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] t1500-rev-parse: rewrite each test to run in isolation

[Jeff King]

On Wed, Apr 13, 2016 at 12:54:16AM -0400, Eric Sunshine wrote:

> > -# label is-bare is-inside-git is-inside-work prefix git-dir 
> > absolute-git-dir
> > +test_expect_success '.git/: is-inside-git-dir' '
> > +   (cd .git && test true = "$(git rev-parse --is-inside-git-dir)")
> 
> Simpler:
> 
> test true = "$(git -C .git rev-parse --is-inside-git-dir)"

While we are modernizing, I wonder if it is a good idea to drop the use
of "test" for string comparisons here. We usually save output to a file
and use test_cmp, for two reasons:

  1. It tests the exit code of the command substitution.

  2. It's easier to debug when it goes wrong (the test produces useful
 output, and the state is left in the filesystem).

It is more verbose to do so, but we could easily wrap that in:

  test_stdout "true" git -C .git rev-parse --is-inside-git-dir

or something. The other problem is that it uses an extra process to do
the test_cmp, which might make the tests slower (especially on Windows).
We could do something like:

  test_stdout () {
  expect=$1; shift
  if ! actual=$("$@")
  then
echo >&2 "test_stdout: command failed: $*"
return 1
  fi
  if test "$expect" != "$actual"
  then
  echo >&2 "test_stdout: unexpected output for $*"
  echo >&2 "test_stdout: $expect != $actual"
  return 1
  fi
  return 0
  }

which is more thorough without incurring extra processes (we lose the
nice diff output of test_cmp, but since this interface is only useful
for short outputs anyway, I don't think that's a big deal.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] gitk: Fix how remote branch names with / are drawn

[Mike Rappazzo]

On Wed, Apr 13, 2016 at 2:19 PM, David Holmer  wrote:
> I agree that this switches the issue around and that a remote with a
> '/' in the name would be miss colored in the same way a branch with a
> '/' in the name is miss colored now. However, I would guess that
> branches with '/' are MUCH MUCH more common than remotes with '/', so
> like you say "this is a better state than the present". A "complete"
> solution would take iterating through the list of remotes and matching
> the explicit whole pattern (e.g. match
> "remotes/my/remote/with/slashes/" for remote "my/remote/with/slashes")
> but I doubt that is worth it for 99.9% of people.
>
> The alternative regex that you are asking about is either using some
> syntax I am not familiar with or isn't quite correct. I'm most
> familiar with grep command line format, so perhaps tcl regex is
> different.
>
> The original code does the equivalent of this:
>
> ~$ echo "remotes/origin/dev/test1" | grep -o "remotes/.*/"
> remotes/origin/dev/
>
> The issue is that the '.*/' part is greedy in that it will match all
> the way up to and including the last /
>
> My solution was to change the . to [^/] which means "any character but
> /". This stops the match at the first / after the remote name starts:
>
> ~$ echo "remotes/origin/dev/test1" | grep -o "remotes/[^/]*/"
> remotes/origin/
>
> The alternative you suggested with '.*?/' doesn't seem to work with grep:
>
> ~$ echo "remotes/origin/dev/test1" | grep -o "remotes/.*?/"
> (no output, i.e. does not match)

`.*?` is a lazy match. I think it is an extended-regex, and your
version is probably more efficient anyway.
echo "remotes/origin/dev/test1" | grep -Eo "remotes/.*?/"

>
>
> Thank you.
>

(Most people on this list don't like "top posting"), please try to
reply inline instead.


> On Wed, Apr 13, 2016 at 7:35 AM, Mike Rappazzo  wrote:
>> On Tue, Apr 12, 2016 at 9:59 PM, David Holmer  wrote:
>>> Consider this example branch:
>>>
>>> remotes/origin/master
>>>
>>> gitk displays this branch with different background colors for each part:
>>> "remotes/origin" in orange and "master" in green. The idea is to make it
>>> visually easy to read the branch name separately from the remote name.
>>>
>>> However this fails when given this example branch:
>>>
>>> remotes/origin/foo/bar
>>>
>>> gitk displays this branch with "remotes/origin/foo" in orange and "bar" in
>>> green. This makes it hard to read the branch name "foo/bar". This is due
>>> to an inappropriately greedy regexp. This patch provides a fix so the same
>>> branch will now be displayed with "remotes/origin" in orange and "foo/bar"
>>> in green.
>>>
>>> Signed-off-by: David Holmer 
>>> ---
>>>  gitk | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/gitk b/gitk
>>> index 805a1c7..ca2392b 100755
>>> --- a/gitk
>>> +++ b/gitk
>>> @@ -6640,7 +6640,7 @@ proc drawtags {id x xt y1} {
>>> set xl [expr {$xl - $delta/2}]
>>> $canv create polygon $x $yt $xr $yt $xr $yb $x $yb \
>>> -width 1 -outline black -fill $col -tags tag.$id
>>> -   if {[regexp {^(remotes/.*/|remotes/)} $tag match remoteprefix]} 
>>> {
>>> +   if {[regexp {^(remotes/[^/]*/|remotes/)} $tag match 
>>> remoteprefix]} {
>>> set rwid [font measure mainfont $remoteprefix]
>>> set xi [expr {$x + 1}]
>>> set yti [expr {$yt + 1}]
>>> --
>>
>> This likely fixes the problem for most situations, but doesn't for a
>> remote with a '/' in the name.  Yet, I think this is a better state
>> than the present.
>>
>> Is the regex `[^/]*/` more efficient than '.*?/`?  Or do you find the
>> former more readable?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] gitk: Fix how remote branch names with / are drawn

[David Holmer]

I agree that this switches the issue around and that a remote with a
'/' in the name would be miss colored in the same way a branch with a
'/' in the name is miss colored now. However, I would guess that
branches with '/' are MUCH MUCH more common than remotes with '/', so
like you say "this is a better state than the present". A "complete"
solution would take iterating through the list of remotes and matching
the explicit whole pattern (e.g. match
"remotes/my/remote/with/slashes/" for remote "my/remote/with/slashes")
but I doubt that is worth it for 99.9% of people.

The alternative regex that you are asking about is either using some
syntax I am not familiar with or isn't quite correct. I'm most
familiar with grep command line format, so perhaps tcl regex is
different.

The original code does the equivalent of this:

~$ echo "remotes/origin/dev/test1" | grep -o "remotes/.*/"
remotes/origin/dev/

The issue is that the '.*/' part is greedy in that it will match all
the way up to and including the last /

My solution was to change the . to [^/] which means "any character but
/". This stops the match at the first / after the remote name starts:

~$ echo "remotes/origin/dev/test1" | grep -o "remotes/[^/]*/"
remotes/origin/

The alternative you suggested with '.*?/' doesn't seem to work with grep:

~$ echo "remotes/origin/dev/test1" | grep -o "remotes/.*?/"
(no output, i.e. does not match)


Thank you.

On Wed, Apr 13, 2016 at 7:35 AM, Mike Rappazzo  wrote:
> On Tue, Apr 12, 2016 at 9:59 PM, David Holmer  wrote:
>> Consider this example branch:
>>
>> remotes/origin/master
>>
>> gitk displays this branch with different background colors for each part:
>> "remotes/origin" in orange and "master" in green. The idea is to make it
>> visually easy to read the branch name separately from the remote name.
>>
>> However this fails when given this example branch:
>>
>> remotes/origin/foo/bar
>>
>> gitk displays this branch with "remotes/origin/foo" in orange and "bar" in
>> green. This makes it hard to read the branch name "foo/bar". This is due
>> to an inappropriately greedy regexp. This patch provides a fix so the same
>> branch will now be displayed with "remotes/origin" in orange and "foo/bar"
>> in green.
>>
>> Signed-off-by: David Holmer 
>> ---
>>  gitk | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/gitk b/gitk
>> index 805a1c7..ca2392b 100755
>> --- a/gitk
>> +++ b/gitk
>> @@ -6640,7 +6640,7 @@ proc drawtags {id x xt y1} {
>> set xl [expr {$xl - $delta/2}]
>> $canv create polygon $x $yt $xr $yt $xr $yb $x $yb \
>> -width 1 -outline black -fill $col -tags tag.$id
>> -   if {[regexp {^(remotes/.*/|remotes/)} $tag match remoteprefix]} {
>> +   if {[regexp {^(remotes/[^/]*/|remotes/)} $tag match 
>> remoteprefix]} {
>> set rwid [font measure mainfont $remoteprefix]
>> set xi [expr {$x + 1}]
>> set yti [expr {$yt + 1}]
>> --
>
> This likely fixes the problem for most situations, but doesn't for a
> remote with a '/' in the name.  Yet, I think this is a better state
> than the present.
>
> Is the regex `[^/]*/` more efficient than '.*?/`?  Or do you find the
> former more readable?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1] Documentation: add setup instructions for Travis CI

[Stefan Beller]

On Wed, Apr 13, 2016 at 10:39 AM, Junio C Hamano  wrote:

> here, create a "GitHub-Travis CI hints" section just before "MUA
> specific hints" section,

Somebody (I think it was you, Lars?) at GitMerge suggested to break
up the SubmittingPatches document into more than one, i.e.
the MUA hints and the Github-Travis hints could become their own documents,
and the SubmittingPatches could just contain the bare essentials.

(The file itself could also be renamed to SubmittingChanges eventually,
as the interface to the submitgit app allows you to push commits and
then transfer these to the mailing list. So while there are still patches
on the receiving end, the last contact with the change was done via
git commit/push hopefully. I dunno.)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v14 4/6] parse-options.c: make OPTION_COUNTUP respect "unspecified" values

[Eric Sunshine]

On Wed, Apr 13, 2016 at 1:43 PM, Pranit Bauva  wrote:
> On Wed, Apr 13, 2016 at 11:03 PM, Eric Sunshine  
> wrote:
>> On Wed, Apr 13, 2016 at 4:39 AM, Pranit Bauva  wrote:
>>> On Wed, Apr 13, 2016 at 11:26 AM, Eric Sunshine  
>>> wrote:
 On Tue, Apr 12, 2016 at 7:02 PM, Pranit Bauva  
 wrote:
> +test_expect_success 'OPT_COUNTUP() resets to 0 with --no- flag' '
> +   test-parse-options --no-verbose >output 2>output.err &&
> +   test_must_be_empty output.err &&
> +   test_cmp expect output
> +'

 In my v12 review, I noticed that neither --no-verbose nor --no-quiet
 was being tested by t0040 (which is conceptually independent of the
 OPT__COUNTUP change) and suggested[3] that you add a new patch to
 address that shortcoming. This idea was followed up by [1] saying that
 this test (here) could then be dropped since the case it checks would
 already be covered by the new patch. My impression was that you
 agreed[4] that that made sense, however, this test is still here. Did
 I misunderstand your response[4]?

 [1]: http://article.gmane.org/gmane.comp.version-control.git/290662
 [2]: http://article.gmane.org/gmane.comp.version-control.git/289991
 [3]: http://article.gmane.org/gmane.comp.version-control.git/290655
 [4]: http://article.gmane.org/gmane.comp.version-control.git/290787
>>>
>>> I actually did include the tests in the patch 3/6[1]. I mentioned in
>>> my response[2] that I will include the tests between 2/5 and 3/5.
>>> Since, after that no email was exchanged, I thought you agreed.
>>
>> I'm not sure that I understand what you are saying since the
>> --no-verbose test does not seem to be included in the patch you cite
>> (it is instead in the present patch under discussion).
>>
>> Perhaps there is some miscommunication and misunderstanding.
>
> Sorry for being a bit unclear.
> I will explain this. The patch 3/6 contains the test which tests the
> quiet option thus in turn testing whether the variable quiet becomes 0
> with --no flag. This patch ie. 4/6 contains the test which tests the
> verbose options thus in turn testing whether the variable verbose
> becomes 0 with no flag. Both of them test different behavior as quiet
> is initially 0 while verbose is initially -1.
>
> So finally what I wanted to achieve is that I should test --no-quiet
> in 3/6 as till then this new behavior is not yet introduced. Thus, it
> will confirm the wanted behavior which exists before 4/6.
>
> This patch introduces a test to verify whether --no-verbose makes the
> variable 0. This patch introduces a new "unspecified" behavior. Thus
> we can test this new behavior with this.
>
> I hope now it is a bit clear on what I want to do.

Sorry, it's not clearer. I understand what you're trying to do, but it
still seems to be the a less desirable (and more complex) approach
since it's mixing conceptually distinct notions and mismatching them
with changes in the wrong patches. But, perhaps my notion of
"conceptually distinct" is different from yours and vice-versa.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: 0 bot for Git

[Junio C Hamano]

Lars Schneider  writes:

> I am not sure what you mean by "fail to hit 'pu'". Maybe we talk at
> cross purposes. Here is what I think you do, please correct me:
>
> 1.) You pick the topics from the mailing list and create feature 
> branches for each one of them. E.g. one of my recent topics 
> is "ls/config-origin".

I do not do this step blindly.  The patch is studied in MUA, perhaps
applied to a new topic to view it in wider context, and tested in
isolation at this step.  In any of these steps, I may decide it is
way too premature for 'pu' and discard it.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1] Documentation: add setup instructions for Travis CI

[Junio C Hamano]

larsxschnei...@gmail.com writes:

> From: Lars Schneider 
>
> Signed-off-by: Lars Schneider 
> ---
>  Documentation/SubmittingPatches | 39 ---
>  1 file changed, 36 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
> index 98fc4cc..79e9b33 100644
> --- a/Documentation/SubmittingPatches
> +++ b/Documentation/SubmittingPatches
> @@ -63,10 +63,43 @@ t/README for guidance.
>  When adding a new feature, make sure that you have new tests to show
>  the feature triggers the new behaviour when it should, and to show the
>  feature does not trigger when it shouldn't.  Also make sure that the
> +test suite passes after your commit.

This is not a new issue, but it sounds as if you do not have to test
if you are not doing a new shiny toy.  Perhaps we should rephrase
the last sentence a bit.

After any code change, make sure that the entire test suite
passes.  After any documentation change, make sure that the
resulting documentation set formats well.

By the way, can you teach our Travis thing to check for the "make
doc" failures?

> +We recommend to use our CI infrastructure to ensure your new feature
> +passes all existing tests as well as your new tests on Linux, Mac, and
> +(hopefully soon) Windows.  Follow these steps for the initial setup:
> +
> + (1) Sign in to GitHub: https://github.com
> + Please sign up first if you haven't already, it's free.

Three issues:

 * None of the things utilized by the reader of this section looks
   like "our" infrastructure to me.

 * The above makes it sound as if we recommend everybody to become
   GitHub customer, which is not really the case.

 * This is overly long and deserves to be its own separate section,
   just like we have MUA specific hints, this is GitHub-Travis
   specific hints.

How about just saying

If you have an account at GitHub (and you can get one for
free to work on open source projects), you can use their
Travis CI integration to test your changes on Linux, Mac,
and (hopefully soon) Windows.  See GitHub-Travis CI hints
section for details.

here, create a "GitHub-Travis CI hints" section just before "MUA
specific hints" section, and move these numbered entries and the two
paragraphs that follow to the new section.  As the introductory text
for the new section itself, it may make sense to repeat a rephrased
version of the above there, e.g.

--
GitHub-Travis CI hints

With an account at GitHub (you can get one for free to work
on open source projects), you can use Travis CI to test your
changes on Linux, Mac, and (hopefully soon) Windows.

Follow these steps for the initial setup:

(1) ...


I'd mildly prefer to leave "Please sign up first" line out
of the first entry.

> + ...
> + (7) Enable Travis CI builds for your Git fork
> +
> +After the initial setup you can push your new feature branches to your
> +Git fork on GitHub and check if they pass all tests here:
> +https://travis-ci.org//git/branches
> +
> +If they don't pass then they are marked "red". If that happens then
> +click on the failing Travis CI job and scroll all the way down in the
> +log. Find the line "<-- Click here to see detailed test output!" and
> +click on the triangle next to the log line number to expand the detailed
> +test output (example: https://travis-ci.org/git/git/jobs/122676187).
> +Fix the problem and push an updated commit to your branch to ensure
> +all tests pass.
> +
> +Do not forget to update the documentation to describe the updated
> +behaviour of your new feature. It is currently a liberal mixture of US
>  and UK English norms for spelling and grammar, which is somewhat
>  unfortunate.  A huge patch that touches the files all over the place
>  only to correct the inconsistency is not welcome, though.  Potential
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v14 6/6] commit: add a commit.verbose config variable

[Eric Sunshine]

On Wed, Apr 13, 2016 at 5:15 AM, Pranit Bauva  wrote:
> On Wed, Apr 13, 2016 at 11:44 AM, Eric Sunshine  
> wrote:
>> On Tue, Apr 12, 2016 at 7:02 PM, Pranit Bauva  wrote:
>>> +test_expect_success 'status does not verbose without --verbose' '
>>> +   git status >actual &&
>>> +   ! grep "^diff --git" actual
>>> +'
>>
>> But what is this test checking?
>
> status is also a consumer of the verbose whose initial value is set to
> -1. This makes it include verbose in status output. This bug was fixed
> by explicitly initializing verbose to 0 if -1. SZEDER pointed out a
> bug[1] which broke some tests in and then when I fixed it, you
> requested me to include tests even in this patch[2] which I found
> convincing enough.
>
> [1]: http://article.gmane.org/gmane.comp.version-control.git/289730
> [2]: http://article.gmane.org/gmane.comp.version-control.git/289993

Okay, makes sense, but it's not at all obvious from the context of
this patch or its commit message. It probably would have been clearer
had the two git-status tests been added in a separate preparatory test
with a commit message explaining that the tests are to ensure that a
subsequent patch (adding commit.verbose) won't break the existing
behavior of git-status. Having a separate commit explaining that would
also help future readers of the test script who wonder what this test
is doing (since it's not obvious and it's not explained by the current
commit message) when they use git-blame to try to figure out its
purpose. If you do re-roll, you might consider breaking them out to a
new patch or, at the very least, document their purpose in the commit
message of this patch.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v14 4/6] parse-options.c: make OPTION_COUNTUP respect "unspecified" values

[Pranit Bauva]

On Wed, Apr 13, 2016 at 11:03 PM, Eric Sunshine  wrote:
> On Wed, Apr 13, 2016 at 4:39 AM, Pranit Bauva  wrote:
>> On Wed, Apr 13, 2016 at 11:26 AM, Eric Sunshine  
>> wrote:
>>> On Tue, Apr 12, 2016 at 7:02 PM, Pranit Bauva  
>>> wrote:
 +test_expect_success 'OPT_COUNTUP() resets to 0 with --no- flag' '
 +   test-parse-options --no-verbose >output 2>output.err &&
 +   test_must_be_empty output.err &&
 +   test_cmp expect output
 +'
>>>
>>> In my v12 review, I noticed that neither --no-verbose nor --no-quiet
>>> was being tested by t0040 (which is conceptually independent of the
>>> OPT__COUNTUP change) and suggested[3] that you add a new patch to
>>> address that shortcoming. This idea was followed up by [1] saying that
>>> this test (here) could then be dropped since the case it checks would
>>> already be covered by the new patch. My impression was that you
>>> agreed[4] that that made sense, however, this test is still here. Did
>>> I misunderstand your response[4]?
>>>
>>> [1]: http://article.gmane.org/gmane.comp.version-control.git/290662
>>> [2]: http://article.gmane.org/gmane.comp.version-control.git/289991
>>> [3]: http://article.gmane.org/gmane.comp.version-control.git/290655
>>> [4]: http://article.gmane.org/gmane.comp.version-control.git/290787
>>
>> I actually did include the tests in the patch 3/6[1]. I mentioned in
>> my response[2] that I will include the tests between 2/5 and 3/5.
>> Since, after that no email was exchanged, I thought you agreed.
>
> I'm not sure that I understand what you are saying since the
> --no-verbose test does not seem to be included in the patch you cite
> (it is instead in the present patch under discussion).
>
> Perhaps there is some miscommunication and misunderstanding.

Sorry for being a bit unclear.
I will explain this. The patch 3/6 contains the test which tests the
quiet option thus in turn testing whether the variable quiet becomes 0
with --no flag. This patch ie. 4/6 contains the test which tests the
verbose options thus in turn testing whether the variable verbose
becomes 0 with no flag. Both of them test different behavior as quiet
is initially 0 while verbose is initially -1.

So finally what I wanted to achieve is that I should test --no-quiet
in 3/6 as till then this new behavior is not yet introduced. Thus, it
will confirm the wanted behavior which exists before 4/6.

This patch introduces a test to verify whether --no-verbose makes the
variable 0. This patch introduces a new "unspecified" behavior. Thus
we can test this new behavior with this.

I hope now it is a bit clear on what I want to do.

>> [1]: http://article.gmane.org/gmane.comp.version-control.git/291310
>> [2]:http://article.gmane.org/gmane.comp.version-control.git/290787
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: 0 bot for Git

[Greg KH]

On Wed, Apr 13, 2016 at 10:29:57AM -0700, Stefan Beller wrote:
> 
> At Git Merge Greg said (paraphrasing here):
> 
>   We waste developers time, because we have plenty of it. Maintainers time
>   however is precious because maintainers are the bottleneck and a scare
>   resource to come by.

s/scare/scarce/

Although some people might disagree :)

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v14 4/6] parse-options.c: make OPTION_COUNTUP respect "unspecified" values

[Eric Sunshine]

On Wed, Apr 13, 2016 at 4:39 AM, Pranit Bauva  wrote:
> On Wed, Apr 13, 2016 at 11:26 AM, Eric Sunshine  
> wrote:
>> On Tue, Apr 12, 2016 at 7:02 PM, Pranit Bauva  wrote:
>>> +test_expect_success 'OPT_COUNTUP() resets to 0 with --no- flag' '
>>> +   test-parse-options --no-verbose >output 2>output.err &&
>>> +   test_must_be_empty output.err &&
>>> +   test_cmp expect output
>>> +'
>>
>> In my v12 review, I noticed that neither --no-verbose nor --no-quiet
>> was being tested by t0040 (which is conceptually independent of the
>> OPT__COUNTUP change) and suggested[3] that you add a new patch to
>> address that shortcoming. This idea was followed up by [1] saying that
>> this test (here) could then be dropped since the case it checks would
>> already be covered by the new patch. My impression was that you
>> agreed[4] that that made sense, however, this test is still here. Did
>> I misunderstand your response[4]?
>>
>> [1]: http://article.gmane.org/gmane.comp.version-control.git/290662
>> [2]: http://article.gmane.org/gmane.comp.version-control.git/289991
>> [3]: http://article.gmane.org/gmane.comp.version-control.git/290655
>> [4]: http://article.gmane.org/gmane.comp.version-control.git/290787
>
> I actually did include the tests in the patch 3/6[1]. I mentioned in
> my response[2] that I will include the tests between 2/5 and 3/5.
> Since, after that no email was exchanged, I thought you agreed.

I'm not sure that I understand what you are saying since the
--no-verbose test does not seem to be included in the patch you cite
(it is instead in the present patch under discussion).

Perhaps there is some miscommunication and misunderstanding.

> [1]: http://article.gmane.org/gmane.comp.version-control.git/291310
> [2]:http://article.gmane.org/gmane.comp.version-control.git/290787
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: 0 bot for Git

[Stefan Beller]

On Wed, Apr 13, 2016 at 10:09 AM, Lars Schneider
 wrote:
>
>> On 13 Apr 2016, at 18:27, Junio C Hamano  wrote:
>>
>> Lars Schneider  writes:
>>
>>> @Junio:
>>> If you setup Travis CI for your https://github.com/gitster/git fork
>>> then Travis CI would build all your topic branches and you (and
>>> everyone who is interested) could check
>>> https://travis-ci.org/gitster/git/branches to see which branches
>>> will break pu if you integrate them.
>>
>> I would not say such an arrangement is worthless, but it targets a
>> wrong point in the patch flow.
>>
>> The patches that result in the most wastage of my time (i.e. a
>> shared bottleneck resource the community should strive to optimize
>> for) are the ones that fail to hit 'pu'.  Ones that do not even
>> build in isolation, ones that may build but fail even the new tests
>> they bring in, ones that break existing tests, and ones that are OK
>> in isolation but do not play well with topics already in flight.
>
> I am not sure what you mean by "fail to hit 'pu'". Maybe we talk at
> cross purposes. Here is what I think you do, please correct me:
>
> 1.) You pick the topics from the mailing list and create feature
> branches for each one of them. E.g. one of my recent topics
> is "ls/config-origin".

and by You you mean Junio.

Ideally the 0bot would have sent the message as a reply to the
cover letter with the information "doesn't compile/breaks test t1234",
so Junio could ignore that series (no time wasted on his part).

At Git Merge Greg said (paraphrasing here):

  We waste developers time, because we have plenty of it. Maintainers time
  however is precious because maintainers are the bottleneck and a scare
  resource to come by.

And I think Git and the kernel have the same community design here.
(Except the kernel is bigger and has more than one maintainer)

So the idea is help Junio make a decision to drop/ignore those patches
with least amount of brain cycled spent as possible. (Not even spend 5
seconds on it).

>
> 2.) At some point you create a new pu branch based on the latest
> next branch. You merge all the new topics into the new pu.

but Junio also runs test after each(?) merge(?) of a series and once
tests fail, it takes time to sort out, what caused it. (Is that the patch series
alone or is that because 2 series interact badly with each other?)

>
> If you push the topics to github.com/gitster after step 1 then
> Travis CI could tell you if the individual topic builds clean
> and passes all tests. Then you could merge only clean topics in
> step 2 which would result in a pu that is much more likely to
> build clean.

IIRC Junio did not like granting travis access to the "blessed" repository
as travis wants so much permissions including write permission to that
repo. (We/He could have a second non advertised repo though)

Also this would incur wait time on Junios side

1) collect patches (many series over the day)
2) push
3) wait
4) do the merges

however a 0 bot would do
1) collect patches faster than Junio (0 bot is a computer after all,
working 24/7)
2) test each patch/series individually
3) send feedback without the wait time, so the contributor from a different
   time zone gets feedback quickly. (round trip is just the build and test time,
   which the developer forgot to do any way if it fails)

>
> Could that process avoid wasting your time with bad patches?
>
>> Automated testing of what is already on 'pu' does not help reduce
>> the above cost, as the culling must be done by me _without_ help
>> from automated test you propose to run on topics in 'pu'.  Ever
>> heard of chicken and egg?
>>
>> Your "You can setup your own CI" update to SubmittingPatches may
>> encourage people to test before sending.  The "Travis CI sends
>> failure notice as a response to a crappy patch" discussed by
>> Matthieu in the other subthread will be of great help.
>>
>> Thanks.
>>
>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v14 3/6] t0040-parse-options: improve test coverage

[Eric Sunshine]

On Wed, Apr 13, 2016 at 4:59 AM, Pranit Bauva  wrote:
> On Wed, Apr 13, 2016 at 10:56 AM, Eric Sunshine  
> wrote:
>> On Tue, Apr 12, 2016 at 7:02 PM, Pranit Bauva  wrote:
>>> +test_expect_success '--no-quiet sets quiet to 0' '
>>> +   test-parse-options --no-quiet >output 2>output.err &&
>>
>> Meh, as implemented, this isn't a very interesting test, is it?
>> 'quiet' started at 0, so all this shows is that --no-quiet didn't
>> disturb the 0. To really test that it resets it to 0, you'd want:
>>
>> test-parse-options --quiet --no-quiet >... 2>... &&
>>
>>> +   test_must_be_empty output.err &&
>>> +   test_cmp expect output
>>> +'
>>>  test_done
>
> This is to test whether the 0 of quiet remains 0 if --no-quiet is
> included. This test "defines" the current behavior. Then when I change
> OPT_COUNTUP(), this test will ensure that this behavior is not
> interrupted as promised by the commit message of that patch[1]. I
> guess this also describe why I choose to include these tests between
> 2/5 and 3/5 rather than 3/5 and 4/5. And also see the extended
> discussion[2] for this. If I do a re-roll then I include `--quiet`
> before `--no-quiet`

Each of these patches should have a single conceptual purpose. It
seems, from the above explanation, that you're mixing and mismatching
bits of such changes between patches.

* The two new tests for --no-verbose and --no-quiet should be together
and check that they correctly reverse --verbose and --quiet,
respectively.

* The test you describe above which ensures that --no-quiet leaves
'quiet' at 0 should be bundled with the change that might break that
behavior, namely, the OPT__COUNTUP() change.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 03/16] index-helper: new daemon for caching index and related stuff

[David Turner]

On Wed, 2016-04-13 at 18:00 +0100, Ramsay Jones wrote:
> 
> On 13/04/16 01:32, David Turner wrote:
> 
> [snip]
> 
> > diff --git a/git-compat-util.h b/git-compat-util.h
> > index c07e0c1..8b878fe 100644
> > --- a/git-compat-util.h
> > +++ b/git-compat-util.h
> > @@ -513,6 +513,7 @@ static inline int ends_with(const char *str,
> > const char *suffix)
> >  #define PROT_READ 1
> >  #define PROT_WRITE 2
> >  #define MAP_PRIVATE 1
> > +#define MAP_SHARED 2
> >  #endif
> >  
> >  #define mmap git_mmap
> > @@ -1045,4 +1046,21 @@ struct tm *git_gmtime_r(const time_t *,
> > struct tm *);
> >  #define getc_unlocked(fh) getc(fh)
> >  #endif
> >  
> > +#ifdef __linux__
> > +#define UNIX_PATH_MAX 108
> > +#elif defined(__APPLE__) || defined(BSD)
> > +#define UNIX_PATH_MAX 104
> > +#else
> > +/*
> > + * Quoth POSIX: The size of sun_path has intentionally been left
> > + * undefined. This is because different implementations use
> > different
> > + * sizes. For example, 4.3 BSD uses a size of 108, and 4.4 BSD
> > uses a
> > + * size of 104. Since most implementations originate from BSD
> > + * versions, the size is typically in the range 92 to 108.
> > + *
> > + * Thanks, POSIX!  Super-helpful!  Hope we don't overflow any
> > buffers!
> > + */
> > +#define UNIX_PATH_MAX 92
> > +#endif
> > +
> 
> It seems you forgot to delete this hunk. ;-)

Oops.  Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: 0 bot for Git

[Lars Schneider]

> On 13 Apr 2016, at 18:27, Junio C Hamano  wrote:
> 
> Lars Schneider  writes:
> 
>> @Junio:
>> If you setup Travis CI for your https://github.com/gitster/git fork
>> then Travis CI would build all your topic branches and you (and 
>> everyone who is interested) could check 
>> https://travis-ci.org/gitster/git/branches to see which branches 
>> will break pu if you integrate them.
> 
> I would not say such an arrangement is worthless, but it targets a
> wrong point in the patch flow.
> 
> The patches that result in the most wastage of my time (i.e. a
> shared bottleneck resource the community should strive to optimize
> for) are the ones that fail to hit 'pu'.  Ones that do not even
> build in isolation, ones that may build but fail even the new tests
> they bring in, ones that break existing tests, and ones that are OK
> in isolation but do not play well with topics already in flight.

I am not sure what you mean by "fail to hit 'pu'". Maybe we talk at
cross purposes. Here is what I think you do, please correct me:

1.) You pick the topics from the mailing list and create feature 
branches for each one of them. E.g. one of my recent topics 
is "ls/config-origin".

2.) At some point you create a new pu branch based on the latest
next branch. You merge all the new topics into the new pu.

If you push the topics to github.com/gitster after step 1 then
Travis CI could tell you if the individual topic builds clean 
and passes all tests. Then you could merge only clean topics in 
step 2 which would result in a pu that is much more likely to 
build clean.

Could that process avoid wasting your time with bad patches?

> Automated testing of what is already on 'pu' does not help reduce
> the above cost, as the culling must be done by me _without_ help
> from automated test you propose to run on topics in 'pu'.  Ever
> heard of chicken and egg?
> 
> Your "You can setup your own CI" update to SubmittingPatches may
> encourage people to test before sending.  The "Travis CI sends
> failure notice as a response to a crappy patch" discussed by
> Matthieu in the other subthread will be of great help.
> 
> Thanks.
> 

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 03/16] index-helper: new daemon for caching index and related stuff

[Ramsay Jones]

On 13/04/16 01:32, David Turner wrote:

[snip]

> diff --git a/git-compat-util.h b/git-compat-util.h
> index c07e0c1..8b878fe 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -513,6 +513,7 @@ static inline int ends_with(const char *str, const char 
> *suffix)
>  #define PROT_READ 1
>  #define PROT_WRITE 2
>  #define MAP_PRIVATE 1
> +#define MAP_SHARED 2
>  #endif
>  
>  #define mmap git_mmap
> @@ -1045,4 +1046,21 @@ struct tm *git_gmtime_r(const time_t *, struct tm *);
>  #define getc_unlocked(fh) getc(fh)
>  #endif
>  
> +#ifdef __linux__
> +#define UNIX_PATH_MAX 108
> +#elif defined(__APPLE__) || defined(BSD)
> +#define UNIX_PATH_MAX 104
> +#else
> +/*
> + * Quoth POSIX: The size of sun_path has intentionally been left
> + * undefined. This is because different implementations use different
> + * sizes. For example, 4.3 BSD uses a size of 108, and 4.4 BSD uses a
> + * size of 104. Since most implementations originate from BSD
> + * versions, the size is typically in the range 92 to 108.
> + *
> + * Thanks, POSIX!  Super-helpful!  Hope we don't overflow any buffers!
> + */
> +#define UNIX_PATH_MAX 92
> +#endif
> +

It seems you forgot to delete this hunk. ;-)

>  #endif

ATB,
Ramsay Jones


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Question about git log --merge option

[Andrey Hsiao]

Dear list:

Just encountered the --merge option for git log.

In the man page, it has the following explanation:
- After a failed merge, show refs that touch files having a conflict
and don't exist on all heads to merge.

I tried this option and get below results:

1. For a failed merge (with conflicts), if the conflicted file does
not exist on either side of the merge, the --merge option will return
the log from the other side.

2. If the conflicted file exists on both sides of the merge, the
--merge option will return the latest change on either side. (i.e: git
log -1 -- conflict_file / git log -1 --merge -- conflict_file may
return different results, whichever changed the latest)

I'm not sure whether above behavior is the unexpected result (cannot
find detailed explanation for --merge option online).

Thanks
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: 0 bot for Git

[Junio C Hamano]

Lars Schneider  writes:

> @Junio:
> If you setup Travis CI for your https://github.com/gitster/git fork
> then Travis CI would build all your topic branches and you (and 
> everyone who is interested) could check 
> https://travis-ci.org/gitster/git/branches to see which branches 
> will break pu if you integrate them.

I would not say such an arrangement is worthless, but it targets a
wrong point in the patch flow.

The patches that result in the most wastage of my time (i.e. a
shared bottleneck resource the community should strive to optimize
for) are the ones that fail to hit 'pu'.  Ones that do not even
build in isolation, ones that may build but fail even the new tests
they bring in, ones that break existing tests, and ones that are OK
in isolation but do not play well with topics already in flight.

Automated testing of what is already on 'pu' does not help reduce
the above cost, as the culling must be done by me _without_ help
from automated test you propose to run on topics in 'pu'.  Ever
heard of chicken and egg?

Your "You can setup your own CI" update to SubmittingPatches may
encourage people to test before sending.  The "Travis CI sends
failure notice as a response to a crappy patch" discussed by
Matthieu in the other subthread will be of great help.

Thanks.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: 0 bot for Git

[Junio C Hamano]

Matthieu Moy  writes:

 True, presumably the Travis integration already solves that part, so
 I suspect it is just the matter of setting up:
 
 - a fork of git.git and have Travis monitor any and all new
   branches;
 
 - a bot that scans the list traffic, applies each series it sees to
   a branch dedicated for that series and pushes to the above fork.
>>> 
>>> ... and to make it really useful: a way to get a notification email sent
>>> on-list or at least to the submitter as a reply to the patch series.
>> Travis CI could do this ...
> The missing part would be "as a reply to the patch series". When I start
> reviewing a series, if the patch is broken and the CI system already
> knows, I'd rather have the information attached in the same thread right
> inside my mailer.

Yeah, such a message thrown randomly at the list would be too noisy
to be useful, but if it is sent to a specific thread as a response,
it would grab attention of those who are interested in the series,
which is exactly what we want.

So with what you added, the list of what is needed is now:

 - a fork of git.git and have Travis monitor any and all new
   branches;
 
 - a bot that scans the list traffic, applies each series it sees to
   a branch dedicated for that series and pushes to the above fork;

 - a bot (which can be the same as the above or a different one, as
   long as the former and the latter has a way to associate a topic
   branch and the original message) that receives success/failure
   notice from Travis, relays it as a response to the original patch
   on the list.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: 0 bot for Git

[Lars Schneider]

> On 13 Apr 2016, at 14:30, Matthieu Moy  wrote:
> 
> Lars Schneider  writes:
> 
>>> On 13 Apr 2016, at 07:43, Matthieu Moy  wrote:
>>> 
>>> Junio C Hamano  writes:
>>> 
 Matthieu Moy  writes:
 
 True, presumably the Travis integration already solves that part, so
 I suspect it is just the matter of setting up:
 
 - a fork of git.git and have Travis monitor any and all new
  branches;
 
 - a bot that scans the list traffic, applies each series it sees to
  a branch dedicated for that series and pushes to the above fork.
>>> 
>>> ... and to make it really useful: a way to get a notification email sent
>>> on-list or at least to the submitter as a reply to the patch series.
>>> Just having a web interface somewhere that knows how broken the code is
>>> would not be that useful.
>> 
>> Travis CI could do this but I intentionally disabled it to not annoy anyone.
>> It would be easy to enable it here:
>> https://github.com/git/git/blob/7b0d47b3b6b5b64e02a5aa06b0452cadcdb18355/.travis.yml#L98-L99
> 
> The missing part would be "as a reply to the patch series". When I start
> reviewing a series, if the patch is broken and the CI system already
> knows, I'd rather have the information attached in the same thread right
> inside my mailer.
I see. How would the automation know where the email patch needs to be applied?

- Lars--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 4/4] format-patch: introduce format.base configuration

[Ye Xiaolong]

On Tue, Apr 12, 2016 at 12:47:23PM -0700, Junio C Hamano wrote:
>Xiaolong Ye  writes:
>
>> +static int config_base_commit;
>
>This variable is used as a simple boolean whose name is overly broad
>(if it were named "config_base_auto" this complaint would not
>apply).  If you envision possible future enhancements for this
>configuration variable, "int config_base_commit" might make sense
>but I don't think of anything offhand that would be happy with
>"int".
>
>> @@ -786,6 +787,12 @@ static int git_format_config(const char *var, const 
>> char *value, void *cb)
>>  }
>>  if (!strcmp(var, "format.outputdirectory"))
>>  return git_config_string(_output_directory, var, value);
>> +if (!strcmp(var, "format.base")){
>
>Style. s/)){/)) {/
>
>> +if (value && !strcasecmp(value, "auto")) {
>
>Does it make sense to allow "Auto" here?  Given that the command
>line parsing uses strcmp() to require "auto", I do not think so.
>
>> +config_base_commit = 1;
>> +return 0;
>> +}
>
>When a value other than "auto" is given, is it sane to ignore them
>without even warning?
>
>I am wondering if this wants to be a format.useAutoBase boolean
>variable.
>

Thanks for the reminder, seems boolean variable is more suitable for
this case, I'll adopt to it in next iteration.
>> @@ -1215,7 +1222,12 @@ static void prepare_bases(struct base_tree_info 
>> *bases,
>>  DIFF_OPT_SET(, RECURSIVE);
>>  diff_setup_done();
>>  
>> -if (!strcmp(base_commit, "auto")) {
>> +if (base_commit && strcmp(base_commit, "auto")) {
>> +base = lookup_commit_reference_by_name(base_commit);
>> +if (!base)
>> +die(_("Unknown commit %s"), base_commit);
>> +oidcpy(>base_commit, >object.oid);
>> +} else if ((base_commit && !strcmp(base_commit, "auto")) || 
>> config_base_commit) {
>
>It may be a poor design to teach prepare_bases() about "auto" thing.
>Doesn't it belong to the caller?  The caller used to say "If a base

Good point, as I understand your comments, we need to extract the "auto"
thing from prepare_bases() and call it early, then we could have a
concrete base before calling prepare_bases().

Thanks,
Xiaolong.
>is given, then call that function, by the way, the base must be a
>concrete one", and with the new "auto" feature, the caller loosens
>the last part of the statement and says "If a base is given, call
>that function, but if it is specified as "auto", I'd have to compute
>it for the user before doing so".
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 2/4] format-patch: add '--base' option to record base tree info

[Ye Xiaolong]

On Tue, Apr 12, 2016 at 12:08:33PM -0700, Junio C Hamano wrote:
>Xiaolong Ye  writes:
>
>> Maintainers or third party testers may want to know the exact base tree
>> the patch series applies to. Teach git format-patch a '--base' option
>> to record the base tree info and append it at the end of the_first_
>
>It probably was a good idea to add stress during the discussion to
>compare various possibilities, but there no longer is a need to
>italicise "first" like this, I think.
>
>> message(either the cover letter or the first patch in the series).
>
>Please have space before "(" (also found elsewhere in this message)
>to make this readable.
>
>>
>> The base tree info consists of the "base commit", which is a well-known
>> commit that is part of the stable part of the project history everybody
>> else works off of, and zero or more "prerequisite patches", which are
>> well-known patches in flight that is not yet part of the "base commit"
>> that need to be applied on top of "base commit" in topological order
>> before the patches can be applied.
>>
>> The "base commit" is shown as "base-commit: " followed by the 40-hex of
>> the commit object name.  A "prerequisite patch" is shown as
>> "prerequisite-patch-id: " followed by the 40-hex "patch id", which is a
>> sum of SHA-1 of the file diffs associated with a patch, with whitespace
>> and line numbers ignored, it's reasonably stable and unique.
>
>Let's be more helpful to end users.  They do not need to know the
>exact formula, especially when there is a command to generate or
>check the id for themselves:
>
>"patch id", which can be obtained by passing the patch through the
>"git patch-id --stable" command
>
>or something?  
>
>> For example, we have history where base is Z, with three prerequisites
>> X-Y-Z, before the patch series A-B-C, i.e.
>
>base is Z???
>
>   Imagine that on top of the public commit P, you applied
>   well-known patches X, Y and Z from somebody else, and then
>   built your three-patch series A, B, C.
>
>perhaps?
>
>>
>>  P---X---Y---Z---A---B---C
>>
>> We could say "git format-patch --base=P -3 C"(or variants thereof, e.g.
>> with "--cover-letter" of using "Z..C" instead of "-3 C" to specify the
>> range), then we could get base tree information block showing at the
>> end of _first_ message as below:
>
>Again, if "first" is _so_ important to stress, it probably is worth
>saying that by "first" you mean either patch 1/n or patch 0/n when
>the cover letter exists.
>
>Also "could" may have made sense while we were having discussion on
>possible design of the hypothetical feature, but with the patch
>applied, the feature becomes a reality, so you can and should stop
>living in the hypothetical world and do s/could/can/ the above.
>
>   With "git format-patch --base=P -3 C" (or variants...), the
>   base tree information block is shown at the end of the first
>   message the command outputs (either the first patch, or the
>   cover letter), like this:
>
>perhaps?
>
>I assume that the patch to the documentation has the same text I
>commented on the above, so I won't repeat my comments to them.
>

Thanks for the review,  I'll follow all the comments above and
make changes to commit log as well as documentation.
 
>>  base-commit: P
>>  prerequisite-patch-id: X
>>  prerequisite-patch-id: Y
>>  prerequisite-patch-id: Z
>>
>> Helped-by: Junio C Hamano 
>> Helped-by: Wu Fengguang 
>> Signed-off-by: Xiaolong Ye 
>> ---
>>  Documentation/git-format-patch.txt | 56 +++
>>  builtin/log.c  | 92 
>> ++
>>  t/t4014-format-patch.sh| 15 +++
>>  3 files changed, 163 insertions(+)
>> ...
>> +static void prepare_bases(struct base_tree_info *bases,
>> +  const char *base_commit,
>> +  struct commit **list,
>> +  int total)
>> +{
>> +struct commit *base = NULL, *commit;
>> +struct rev_info revs;
>> +struct diff_options diffopt;
>> +unsigned char sha1[20];
>> +int i;
>> +
>> +diff_setup();
>> +DIFF_OPT_SET(, RECURSIVE);
>> +diff_setup_done();
>> +
>> +base = lookup_commit_reference_by_name(base_commit);
>> +if (!base)
>> +die(_("Unknown commit %s"), base_commit);
>> +oidcpy(>base_commit, >object.oid);
>> +
>> +init_revisions(, NULL);
>> +revs.max_parents = 1;
>> +revs.topo_order = 1;
>> +for (i = 0; i < total; i++) {
>> +if (!in_merge_bases(base, list[i]) || base == list[i])
>> +die(_("base commit should be the ancestor of revision 
>> list"));
>
>This check looks overly expensive, but I do not think of a more
>efficient way to do this, given that "All the commits from our
>series must reach the specified base" is what you seem to want.

Yes, that's what I want to 

Re: [PATCH v2 19/21] bisect: use a bottom-up traversal to find relevant weights

[Christian Couder]

On Sun, Apr 10, 2016 at 3:19 PM, Stephan Beyer  wrote:
> The idea is to reverse the DAG and perform a traversal
> starting on all sources of the reversed DAG.
>
> We walk from the bottom commits, incrementing the weight while
> walking on a part of the graph that is single strand of pearls,
> or doing the "count the reachable ones the hard way" using
> compute_weight() when we hit a merge commit.
>
> A traversal ends when the computed weight is falling or halfway.

Yeah, it looks like it could be a good optimization to end a traversal
looking for "relevant" commits when the weight is falling.

> This way, commits with too high weight to be relevant are never
> visited (and their weights are never computed).
>
> Signed-off-by: Stephan Beyer 
> ---
>
> Notes:
> I rephrased the commit message.
>
> I renamed the functions such that they don't talk about "BFS"
> because that is irrelevant. Also use a DFS now because it is
> less code (and a little more efficient).
>
> I plugged some leaks.

That's a lot of things in just one commit.

>  bisect.c | 250 
> +--
>  1 file changed, 162 insertions(+), 88 deletions(-)

Also from the diff stats it looks like you add a lot of code in this
commit and the previous one.
I wonder why you are saying that a DFS is less code above then.

The previous patch (18/21) has the following diff stat:

> bisect.c | 116 ---
> 1 file changed, 97 insertions(+), 19 deletions(-)

And the subsequent patches don't reduce code size overall.
Diff stat for 20/21 is:

> bisect.c | 44 +++-
> 1 file changed, 19 insertions(+), 25 deletions(-)

And diff stat for 21/21 is:

> bisect.c | 18 +-
> 1 file changed, 13 insertions(+), 5 deletions(-)

So after your patches from 18/21 to 21/21 there are around 150 more
lines of code.
Maybe this is worth it, but I wonder if at least some optimizations,
like for example ending a traversal looking for "relevant" commits
when the weight is falling, could be implemented without changing the
code so much and adding so many lines.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: 0 bot for Git

[Fengguang Wu]

> > I don't know how 0 bot solves this, but the obvious issue with this
> > approach is to allow dealing with someone sending a patch like
> >
> > +++ Makefile
> > --- Makefile
> > +all:
> > +   rm -fr $(HOME); sudo rm -fr /
> >
> > to the list. One thing that Travis gives us for free is isolation:
> > malicious code in the build cannot break the bot, only the build
> > itself.

Sure, isolation is a must have for public test services like Travis or
0day. We optimize the 0day infrastructure for good behaviors and also
have ways to isolate malicious ones.

> True, presumably the Travis integration already solves that part, so
> I suspect it is just the matter of setting up:
> 
>  - a fork of git.git and have Travis monitor any and all new
>branches;
> 
>  - a bot that scans the list traffic, applies each series it sees to
>a branch dedicated for that series and pushes to the above fork.
> 
> isn't it?

Right. 0day bot could auto maintain a patch-representing git tree for
Travis to monitor and test. As how we already did for the linux kernel
project, creating one git branch per patchset posted to the lists:

https://github.com/0day-ci/linux/branches

In principle the git project should have more simple rules to decide
"which base should the robot apply a patch to". But we do need some
hints about the git community's rules in order to start the work. If
without such hints from the community, we may start with dumb rules
like "apply to latest origin/master" or "apply to latest release tag".

Thanks,
Fengguang
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 03/16] index-helper: new daemon for caching index and related stuff

[Duy Nguyen]

On Wed, Apr 13, 2016 at 7:32 AM, David Turner  wrote:
> +NOTES
> +-
> +
> +$GIT_DIR/index-helper.path is a symlink

In multiple worktree context, this file will be per-worktree. So we
have one daemon per worktree. I think that's fine.

> to a directory in $TMPDIR
> +containing a Unix domain socket called 's' that the daemon reads
> +commands from.

Oops. I stand corrected, now it's one daemon per repository...
Probably good to hide the socket path in $GIT_DIR though, people may
protect it with dir permission of one of ancestor directories.

> The directory will also contain files named
> +"git-index-".  These are used as backing stores for shared
> +memory.  Normally the daemon will clean up these files when it exits
> +or when they are no longer relevant.  But if it crashes, some objects
> +could remain there and they can be safely deleted with "rm"
> +command.

Alternatively, we could store all these in $GIT_DIR/helper or
something and clean up automatically when index-helper starts. But I
guess at least with TMPDIR we have a chance to put them on tmpfs.

> diff --git a/git-compat-util.h b/git-compat-util.h
> index c07e0c1..8b878fe 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -1045,4 +1046,21 @@ struct tm *git_gmtime_r(const time_t *, struct tm *);
>  #define getc_unlocked(fh) getc(fh)
>  #endif
>
> +#ifdef __linux__
> +#define UNIX_PATH_MAX 108
> +#elif defined(__APPLE__) || defined(BSD)
> +#define UNIX_PATH_MAX 104
> +#else
> +/*
> + * Quoth POSIX: The size of sun_path has intentionally been left
> + * undefined. This is because different implementations use different
> + * sizes. For example, 4.3 BSD uses a size of 108, and 4.4 BSD uses a
> + * size of 104. Since most implementations originate from BSD
> + * versions, the size is typically in the range 92 to 108.
> + *
> + * Thanks, POSIX!  Super-helpful!  Hope we don't overflow any buffers!
> + */
> +#define UNIX_PATH_MAX 92
> +#endif
> +

This looks like dead code (or at least not used in this patch).

> +static int poke_daemon(struct index_state *istate,
> +  const struct stat *st, int refresh_cache)
> +{
> +   int fd;
> +   int ret = 0;
> +   const char *socket_path;
> +
> +   /* if this is from index-helper, do not poke itself (recursively) */
> +   if (istate->to_shm)
> +   return 0;
> +
> +   socket_path = index_helper_path("s");
> +   if (!socket_path)
> +   return -1;
> +
> +   fd = unix_stream_connect(socket_path);
> +   if (refresh_cache) {
> +   ret = write_in_full(fd, "refresh", 8) != 8;

Since we've moved to unix socket and had bidirectional communication,
it's probably a good idea to read an "ok" back, giving index-helper
time to prepare the cache. As I recall the last discussion with
Johannes, missing a cache here when the index is around 300MB could
hurt more than wait patiently once and have it ready next time.

> +   } else {
> +   ret = write_in_full(fd, "poke", 5) != 5;
> +   }
> +
> +   close(fd);
> +   return ret;
> +}
-- 
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: CC in git cover letter vs patches (was Re: [PATCH 0/19] get rid of superfluous __GFP_REPORT)

[Michal Hocko]

On Wed 13-04-16 16:51:37, Vineet Gupta wrote:
> Trimming CC list + CC git folks
> 
> Hi Michal,
> 
> On Monday 11 April 2016 04:37 PM, Michal Hocko wrote:
> > Hi,
> > this is the second version of the patchset previously sent [1]
> 
> I have a git question if you didn't mind w.r.t. this series. Maybe there's an
> obvious answer... I'm using git 2.5.0
> 
> I was wondering how you manage to union the individual patch CC in just the 
> cover
> letter w/o bombarding everyone with everything.

I am using the following flow:

$ rm *.patch
$ for format-patch range
$ git send-email [--to resp. --cc for all patches] --cc-cmd 
./cc-cmd-only-cover.sh --compose *.patch

$ cat ./cc-cmd-only-cover.sh 
#!/bin/bash

# --compose with generate *gitsendemail.msg file
# --cover-letter expects *cover-letter* file
if [[ $1 == *gitsendemail.msg* || $1 == *cover-letter* ]]; then
grep '<.*@.*>' -h *.patch | sed 's/^.*: //' | sort | uniq
fi

it is a little bit coarse and it would be great if git had a default
option for that but this seems to be working just fine for patch-bombs
where the recipients only have to care about their parts and the cover
for the overal idea of the change.

HTH
-- 
Michal Hocko
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 17/21] bisect: rename count_distance() to compute_weight()

[Christian Couder]

On Sun, Apr 10, 2016 at 3:19 PM, Stephan Beyer  wrote:
>
> @@ -70,7 +70,7 @@ static inline int distance_direction(struct commit *commit)
> return 0;
>  }
>
> -static int count_distance(struct commit *elem)
> +static int compute_weight(struct commit *elem)
>  {
> int nr = 0;
> struct commit_list *todo = NULL;
> @@ -93,6 +93,7 @@ static int count_distance(struct commit *elem)
> }
> }
>
> +   node_data(elem)->weight = nr;
> return nr;
>  }

As the return value is not used below, I am not sure it is still worth
it to return the weight.

> @@ -241,7 +242,7 @@ static struct commit_list *best_bisection_sorted(struct 
> commit_list *list)
>   * be computed.
>   *
>   * weight = -2 means it has more than one parent and its distance is
> - * unknown.  After running count_distance() first, they will get zero
> + * unknown.  After running compute_weight() first, they will get zero
>   * or positive distance.
>   */
>  static struct commit_list *do_find_bisection(struct commit_list *list,
> @@ -286,7 +287,7 @@ static struct commit_list *do_find_bisection(struct 
> commit_list *list,
>  * If you have only one parent in the resulting set
>  * then you can reach one commit more than that parent
>  * can reach.  So we do not have to run the expensive
> -* count_distance() for single strand of pearls.
> +* compute_weight() for single strand of pearls.
>  *
>  * However, if you have more than one parent, you cannot
>  * just add their distance and one for yourself, since
> @@ -299,7 +300,7 @@ static struct commit_list *do_find_bisection(struct 
> commit_list *list,
> for (p = list; p; p = p->next) {
> if (!(p->item->object.flags & UNINTERESTING)
>  && (node_data(p->item)->weight == -2)) {
> -   node_data(p->item)->weight = count_distance(p->item);
> +   compute_weight(p->item);
>
> /* Does it happen to be at exactly half-way? */
> if (!find_all && halfway(p->item))
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 16/21] bisect: make total number of commits global

[Christian Couder]

On Sun, Apr 10, 2016 at 3:19 PM, Stephan Beyer  wrote:
> The total number of commits in a bisect process is a property of
> the bisect process. Making this property global helps to make the code
> clearer.
>
> Signed-off-by: Stephan Beyer 
> ---
>  bisect.c | 74 
> ++--
>  1 file changed, 39 insertions(+), 35 deletions(-)
>
> diff --git a/bisect.c b/bisect.c
> index f737ce7..2b415ad 100644
> --- a/bisect.c
> +++ b/bisect.c
> @@ -23,6 +23,8 @@ static const char *argv_show_branch[] = {"show-branch", 
> NULL, NULL};
>  static const char *term_bad;
>  static const char *term_good;
>
> +static int total;
> +
>  static unsigned marker;
>
>  struct node_data {
> @@ -38,7 +40,7 @@ static inline struct node_data *node_data(struct commit 
> *elem)
> return (struct node_data *)elem->util;
>  }
>
> -static inline int get_distance(struct commit *commit, int total)
> +static inline int get_distance(struct commit *commit)
>  {
> int distance = node_data(commit)->weight;
> if (total - distance < distance)
> @@ -54,7 +56,7 @@ static inline int get_distance(struct commit *commit, int 
> total)
>   * Return 0 if the distance is halfway.
>   * Return 1 if the distance is rising.
>   */
> -static inline int distance_direction(struct commit *commit, int total)
> +static inline int distance_direction(struct commit *commit)
>  {
> int doubled_diff = 2 * node_data(commit)->weight - total;
> if (doubled_diff < -1)
> @@ -107,25 +109,25 @@ static int count_interesting_parents(struct commit 
> *commit)
> return count;
>  }
>
> -static inline int halfway(struct commit *commit, int nr)
> +static inline int halfway(struct commit *commit)
>  {
> /*
>  * Don't short-cut something we are not going to return!
>  */
> if (commit->object.flags & TREESAME)
> return 0;
> -   return !distance_direction(commit, nr);
> +   return !distance_direction(commit);
>  }
>
>  #if !DEBUG_BISECT
> -#define show_list(a,b,c,d) do { ; } while (0)
> +#define show_list(a,b,c) do { ; } while (0)
>  #else
> -static void show_list(const char *debug, int counted, int nr,
> +static void show_list(const char *debug, int counted,
>   struct commit_list *list)
>  {
> struct commit_list *p;
>
> -   fprintf(stderr, "%s (%d/%d)\n", debug, counted, nr);
> +   fprintf(stderr, "%s (%d/%d)\n", debug, counted, total);
>
> for (p = list; p; p = p->next) {
> struct commit_list *pp;
> @@ -157,7 +159,7 @@ static void show_list(const char *debug, int counted, int 
> nr,
>  }
>  #endif /* DEBUG_BISECT */
>
> -static struct commit_list *best_bisection(struct commit_list *list, int nr)
> +static struct commit_list *best_bisection(struct commit_list *list)
>  {
> struct commit_list *p, *best;
> int best_distance = -1;
> @@ -169,7 +171,7 @@ static struct commit_list *best_bisection(struct 
> commit_list *list, int nr)
>
> if (flags & TREESAME)
> continue;
> -   distance = get_distance(p->item, nr);
> +   distance = get_distance(p->item);
> if (distance > best_distance) {
> best = p;
> best_distance = distance;
> @@ -195,10 +197,10 @@ static int compare_commit_dist(const void *a_, const 
> void *b_)
> return oidcmp(>commit->object.oid, >commit->object.oid);
>  }
>
> -static struct commit_list *best_bisection_sorted(struct commit_list *list, 
> int nr)
> +static struct commit_list *best_bisection_sorted(struct commit_list *list)
>  {
> struct commit_list *p;
> -   struct commit_dist *array = xcalloc(nr, sizeof(*array));
> +   struct commit_dist *array = xcalloc(total, sizeof(*array));
> int cnt, i;
>
> for (p = list, cnt = 0; p; p = p->next) {
> @@ -207,7 +209,7 @@ static struct commit_list *best_bisection_sorted(struct 
> commit_list *list, int n
>
> if (flags & TREESAME)
> continue;
> -   distance = get_distance(p->item, nr);
> +   distance = get_distance(p->item);
> array[cnt].commit = p->item;
> array[cnt].distance = distance;
> cnt++;
> @@ -243,7 +245,7 @@ static struct commit_list *best_bisection_sorted(struct 
> commit_list *list, int n
>   * or positive distance.
>   */
>  static struct commit_list *do_find_bisection(struct commit_list *list,
> -int nr, struct node_data 
> *weights,
> +struct node_data *weights,
>  int find_all)
>  {
> int n, counted;
> @@ -262,7 +264,7 @@ static struct commit_list *do_find_bisection(struct 
> commit_list *list,
> 

[PATCH] Move test-* to t/helper/ subdirectory

[Nguyễn Thái Ngọc Duy]

This keeps top dir a bit less crowded. And because these programs are
for testing purposes, it makes sense that they stay somewhere in t/

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 This patch will break any patches that add new test programs.
 Luckily, none in 'next' or 'pu' does that. I know lmdb backend adds
 test-lmdb-backend, so a manual move and some .gitignore fixup is
 required there.

 .gitignore | 33 --
 Makefile   | 24 
 t/helper/.gitignore (new)  | 33 ++
 test-chmtime.c => t/helper/test-chmtime.c  |  0
 test-config.c => t/helper/test-config.c|  0
 test-ctype.c => t/helper/test-ctype.c  |  0
 test-date.c => t/helper/test-date.c|  0
 test-delta.c => t/helper/test-delta.c  |  0
 .../helper/test-dump-cache-tree.c  |  0
 .../helper/test-dump-split-index.c |  0
 .../helper/test-dump-untracked-cache.c |  0
 test-fake-ssh.c => t/helper/test-fake-ssh.c|  0
 test-genrandom.c => t/helper/test-genrandom.c  |  0
 test-hashmap.c => t/helper/test-hashmap.c  |  0
 .../helper/test-index-version.c|  0
 test-line-buffer.c => t/helper/test-line-buffer.c  |  0
 test-match-trees.c => t/helper/test-match-trees.c  |  0
 test-mergesort.c => t/helper/test-mergesort.c  |  0
 test-mktemp.c => t/helper/test-mktemp.c|  0
 .../helper/test-parse-options.c|  0
 test-path-utils.c => t/helper/test-path-utils.c|  0
 test-prio-queue.c => t/helper/test-prio-queue.c|  0
 test-read-cache.c => t/helper/test-read-cache.c|  0
 test-regex.c => t/helper/test-regex.c  |  0
 .../helper/test-revision-walking.c |  0
 test-run-command.c => t/helper/test-run-command.c  |  0
 .../helper/test-scrap-cache-tree.c |  0
 test-sha1-array.c => t/helper/test-sha1-array.c|  0
 test-sha1.c => t/helper/test-sha1.c|  0
 test-sha1.sh => t/helper/test-sha1.sh  |  4 +--
 test-sigchain.c => t/helper/test-sigchain.c|  0
 test-string-list.c => t/helper/test-string-list.c  |  0
 .../helper/test-submodule-config.c |  0
 test-subprocess.c => t/helper/test-subprocess.c|  0
 test-svn-fe.c => t/helper/test-svn-fe.c|  0
 .../helper/test-urlmatch-normalization.c   |  0
 test-wildmatch.c => t/helper/test-wildmatch.c  |  0
 t/t5601-clone.sh   |  2 +-
 t/test-lib.sh  |  4 +--
 39 files changed, 50 insertions(+), 50 deletions(-)
 create mode 100644 t/helper/.gitignore
 rename test-chmtime.c => t/helper/test-chmtime.c (100%)
 rename test-config.c => t/helper/test-config.c (100%)
 rename test-ctype.c => t/helper/test-ctype.c (100%)
 rename test-date.c => t/helper/test-date.c (100%)
 rename test-delta.c => t/helper/test-delta.c (100%)
 rename test-dump-cache-tree.c => t/helper/test-dump-cache-tree.c (100%)
 rename test-dump-split-index.c => t/helper/test-dump-split-index.c (100%)
 rename test-dump-untracked-cache.c => t/helper/test-dump-untracked-cache.c 
(100%)
 rename test-fake-ssh.c => t/helper/test-fake-ssh.c (100%)
 rename test-genrandom.c => t/helper/test-genrandom.c (100%)
 rename test-hashmap.c => t/helper/test-hashmap.c (100%)
 rename test-index-version.c => t/helper/test-index-version.c (100%)
 rename test-line-buffer.c => t/helper/test-line-buffer.c (100%)
 rename test-match-trees.c => t/helper/test-match-trees.c (100%)
 rename test-mergesort.c => t/helper/test-mergesort.c (100%)
 rename test-mktemp.c => t/helper/test-mktemp.c (100%)
 rename test-parse-options.c => t/helper/test-parse-options.c (100%)
 rename test-path-utils.c => t/helper/test-path-utils.c (100%)
 rename test-prio-queue.c => t/helper/test-prio-queue.c (100%)
 rename test-read-cache.c => t/helper/test-read-cache.c (100%)
 rename test-regex.c => t/helper/test-regex.c (100%)
 rename test-revision-walking.c => t/helper/test-revision-walking.c (100%)
 rename test-run-command.c => t/helper/test-run-command.c (100%)
 rename test-scrap-cache-tree.c => t/helper/test-scrap-cache-tree.c (100%)
 rename test-sha1-array.c => t/helper/test-sha1-array.c (100%)
 rename test-sha1.c => t/helper/test-sha1.c (100%)
 rename test-sha1.sh => t/helper/test-sha1.sh (96%)
 rename test-sigchain.c => t/helper/test-sigchain.c (100%)
 rename test-string-list.c => t/helper/test-string-list.c (100%)
 rename test-submodule-config.c => t/helper/test-submodule-config.c (100%)
 rename test-subprocess.c => t/helper/test-subprocess.c (100%)
 rename test-svn-fe.c => t/helper/test-svn-fe.c (100%)
 rename test-urlmatch-normalization.c => t/helper/test-urlmatch-normalization.c 
(100%)
 rename test-wildmatch.c => t/helper/test-wildmatch.c (100%)

diff --git a/.gitignore b/.gitignore

[PATCH 16/25] worktree.c: add validate_worktree()

[Nguyễn Thái Ngọc Duy]

This function is later used by "worktree move" and "worktree remove"
to ensure that we have a good connection between the repository and
the worktree.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 worktree.c | 63 ++
 worktree.h |  5 +
 2 files changed, 68 insertions(+)

diff --git a/worktree.c b/worktree.c
index e878f49..28195b1 100644
--- a/worktree.c
+++ b/worktree.c
@@ -219,6 +219,69 @@ int is_main_worktree(const struct worktree *wt)
return wt && !wt->id;
 }
 
+static int report(int quiet, const char *fmt, ...)
+{
+   va_list params;
+
+   if (quiet)
+   return -1;
+
+   va_start(params, fmt);
+   vfprintf(stderr, fmt, params);
+   fputc('\n', stderr);
+   va_end(params);
+   return -1;
+}
+
+int validate_worktree(const struct worktree *wt, int quiet)
+{
+   struct strbuf sb = STRBUF_INIT;
+   const char *path;
+   int err;
+
+   if (is_main_worktree(wt)) {
+   /*
+* Main worktree using .git file to point to the
+* repository would make it impossible to know where
+* the actual worktree is if this function is executed
+* from another worktree. No .git file support for now.
+*/
+   strbuf_addf(, "%s/.git", wt->path);
+   if (!is_directory(sb.buf)) {
+   strbuf_release();
+   return report(quiet, _("'%s/.git' at main worktree is 
not the repository directory"),
+ wt->path);
+   }
+   return 0;
+   }
+
+   /*
+* Make sure "gitdir" file points to a real .git file and that
+* file points back here.
+*/
+   if (!is_absolute_path(wt->path))
+   return report(quiet, _("'%s' file does not contain absolute 
path to the worktree location"),
+ git_common_path("worktrees/%s/gitdir", wt->id));
+
+   strbuf_addf(, "%s/.git", wt->path);
+   if (!file_exists(sb.buf)) {
+   strbuf_release();
+   return report(quiet, _("'%s/.git' does not exist"), wt->path);
+   }
+
+   path = read_gitfile_gently(sb.buf, );
+   strbuf_release();
+   if (!path)
+   return report(quiet, _("'%s/.git' is not a .git file, error 
code %d"),
+ wt->path, err);
+
+   if (strcmp_icase(path, real_path(git_common_path("worktrees/%s", 
wt->id
+   return report(quiet, _("'%s' does not point back to"),
+ wt->path, git_common_path("worktrees/%s", 
wt->id));
+
+   return 0;
+}
+
 char *find_shared_symref(const char *symref, const char *target)
 {
char *existing = NULL;
diff --git a/worktree.h b/worktree.h
index c7a4d20..0d6be18 100644
--- a/worktree.h
+++ b/worktree.h
@@ -39,6 +39,11 @@ extern struct worktree *find_worktree_by_path(struct 
worktree **list,
 extern int is_main_worktree(const struct worktree *wt);
 
 /*
+ * Return zero if the worktree is in good condition.
+ */
+extern int validate_worktree(const struct worktree *wt, int quiet);
+
+/*
  * Free up the memory for worktree
  */
 extern void clear_worktree(struct worktree *);
-- 
2.8.0.rc0.210.gd302cd2

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 21/25] worktree: add "lock" command

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 Documentation/git-worktree.txt | 12 --
 builtin/worktree.c | 41 ++
 contrib/completion/git-completion.bash |  5 -
 t/t2028-worktree-move.sh (new +x)  | 34 
 4 files changed, 89 insertions(+), 3 deletions(-)
 create mode 100755 t/t2028-worktree-move.sh

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 1c9d7c1..9f0c9f0 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -11,6 +11,7 @@ SYNOPSIS
 [verse]
 'git worktree add' [-f] [--detach] [-b ]  []
 'git worktree list' [--porcelain]
+'git worktree lock' [--reason ] 
 'git worktree prune' [-n] [-v] [--expire ]
 
 DESCRIPTION
@@ -61,6 +62,12 @@ each of the linked worktrees.  The output details include if 
the worktree is
 bare, the revision currently checked out, and the branch currently checked out
 (or 'detached HEAD' if none).
 
+lock::
+
+When a worktree is locked, it cannot be pruned, moved or deleted. For
+example, if the worktree is on portable device that is not available
+when "git worktree " is executed.
+
 prune::
 
 Prune working tree information in $GIT_DIR/worktrees.
@@ -104,6 +111,9 @@ OPTIONS
 --expire ::
With `prune`, only expire unused working trees older than .
 
+--reason :
+   An explanation why the worktree is locked.
+
 DETAILS
 ---
 Each linked working tree has a private sub-directory in the repository's
@@ -220,8 +230,6 @@ performed manually, such as:
 - `remove` to remove a linked working tree and its administrative files (and
   warn if the working tree is dirty)
 - `mv` to move or rename a working tree and update its administrative files
-- `lock` to prevent automatic pruning of administrative files (for instance,
-  for a working tree on a portable device)
 
 GIT
 ---
diff --git a/builtin/worktree.c b/builtin/worktree.c
index a9e91ab..736caff 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -14,6 +14,7 @@
 static const char * const worktree_usage[] = {
N_("git worktree add []  []"),
N_("git worktree list []"),
+   N_("git worktree lock [] "),
N_("git worktree prune []"),
NULL
 };
@@ -452,6 +453,44 @@ static int list(int ac, const char **av, const char 
*prefix)
return 0;
 }
 
+static int lock_worktree(int ac, const char **av, const char *prefix)
+{
+   const char *reason = "", *old_reason;
+   struct option options[] = {
+   OPT_STRING(0, "reason", , N_("string"),
+  N_("reason for locking")),
+   OPT_END()
+   };
+   struct worktree **worktrees, *wt;
+   struct strbuf dst = STRBUF_INIT;
+
+   ac = parse_options(ac, av, prefix, options, worktree_usage, 0);
+   if (ac != 1)
+   usage_with_options(worktree_usage, options);
+
+   strbuf_addstr(, prefix_filename(prefix,
+   strlen(prefix),
+   av[0]));
+
+   worktrees = get_worktrees();
+   wt = find_worktree_by_path(worktrees, dst.buf);
+   if (!wt)
+   die(_("'%s' is not a working directory"), av[0]);
+   if (is_main_worktree(wt))
+   die(_("'%s' is a main working directory"), av[0]);
+
+   old_reason = is_worktree_locked(wt);
+   if (old_reason) {
+   if (*old_reason)
+   die(_("already locked, reason: %s"), old_reason);
+   die(_("already locked, no reason"));
+   }
+
+   write_file(git_common_path("worktrees/%s/locked", wt->id),
+  "%s", reason);
+   return 0;
+}
+
 int cmd_worktree(int ac, const char **av, const char *prefix)
 {
struct option options[] = {
@@ -468,5 +507,7 @@ int cmd_worktree(int ac, const char **av, const char 
*prefix)
return prune(ac - 1, av + 1, prefix);
if (!strcmp(av[1], "list"))
return list(ac - 1, av + 1, prefix);
+   if (!strcmp(av[1], "lock"))
+   return lock_worktree(ac - 1, av + 1, prefix);
usage_with_options(worktree_usage, options);
 }
diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index f66db2d..cdae408 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2596,7 +2596,7 @@ _git_whatchanged ()
 
 _git_worktree ()
 {
-   local subcommands="add list prune"
+   local subcommands="add list lock prune"
local subcommand="$(__git_find_on_cmdline "$subcommands")"
if [ -z "$subcommand" ]; then
__gitcomp "$subcommands"
@@ -2608,6 +2608,9 @@ _git_worktree ()
list,--*)
__gitcomp "--porcelain"
;;
+   lock,--*)
+   __gitcomp "--reason"
+   ;;

[PATCH 13/25] worktree.c: add clear_worktree()

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 worktree.c | 14 +++---
 worktree.h |  5 +
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/worktree.c b/worktree.c
index 4c38414..b4e4b57 100644
--- a/worktree.c
+++ b/worktree.c
@@ -4,14 +4,22 @@
 #include "worktree.h"
 #include "dir.h"
 
+void clear_worktree(struct worktree *wt)
+{
+   if (!wt)
+   return;
+   free(wt->path);
+   free(wt->id);
+   free(wt->head_ref);
+   memset(wt, 0, sizeof(*wt));
+}
+
 void free_worktrees(struct worktree **worktrees)
 {
int i = 0;
 
for (i = 0; worktrees[i]; i++) {
-   free(worktrees[i]->path);
-   free(worktrees[i]->id);
-   free(worktrees[i]->head_ref);
+   clear_worktree(worktrees[i]);
free(worktrees[i]);
}
free (worktrees);
diff --git a/worktree.h b/worktree.h
index e89d423..0ba07ab 100644
--- a/worktree.h
+++ b/worktree.h
@@ -28,6 +28,11 @@ extern struct worktree **get_worktrees(void);
 extern const char *get_worktree_git_dir(const struct worktree *wt);
 
 /*
+ * Free up the memory for worktree
+ */
+extern void clear_worktree(struct worktree *);
+
+/*
  * Free up the memory for worktree(s)
  */
 extern void free_worktrees(struct worktree **);
-- 
2.8.0.rc0.210.gd302cd2

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 17/25] worktree.c: add update_worktree_location()

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 worktree.c | 25 +
 worktree.h |  6 ++
 2 files changed, 31 insertions(+)

diff --git a/worktree.c b/worktree.c
index 28195b1..e526e25 100644
--- a/worktree.c
+++ b/worktree.c
@@ -282,6 +282,31 @@ int validate_worktree(const struct worktree *wt, int quiet)
return 0;
 }
 
+int update_worktree_location(struct worktree *wt, const char *path_)
+{
+   struct strbuf path = STRBUF_INIT;
+   int ret = 0;
+
+   if (is_main_worktree(wt))
+   return 0;
+
+   strbuf_add_absolute_path(, path_);
+   if (strcmp_icase(wt->path, path.buf)) {
+   if (!write_file_gently(git_common_path("worktrees/%s/gitdir",
+  wt->id),
+  "%s/.git", real_path(path.buf))) {
+   free(wt->path);
+   wt->path = strbuf_detach(, NULL);
+   ret = 0;
+   } else
+   ret = sys_error(_("failed to update '%s'"),
+   git_common_path("worktrees/%s/gitdir",
+   wt->id));
+   }
+   strbuf_release();
+   return ret;
+}
+
 char *find_shared_symref(const char *symref, const char *target)
 {
char *existing = NULL;
diff --git a/worktree.h b/worktree.h
index 0d6be18..bbe40ef 100644
--- a/worktree.h
+++ b/worktree.h
@@ -44,6 +44,12 @@ extern int is_main_worktree(const struct worktree *wt);
 extern int validate_worktree(const struct worktree *wt, int quiet);
 
 /*
+ * Update worktrees/xxx/gitdir with the new path.
+ */
+extern int update_worktree_location(struct worktree *wt,
+   const char *path_);
+
+/*
  * Free up the memory for worktree
  */
 extern void clear_worktree(struct worktree *);
-- 
2.8.0.rc0.210.gd302cd2

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 18/25] worktree.c: add is_worktree_locked()

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 worktree.c | 18 ++
 worktree.h |  6 ++
 2 files changed, 24 insertions(+)

diff --git a/worktree.c b/worktree.c
index e526e25..37eec09 100644
--- a/worktree.c
+++ b/worktree.c
@@ -219,6 +219,24 @@ int is_main_worktree(const struct worktree *wt)
return wt && !wt->id;
 }
 
+const char *is_worktree_locked(const struct worktree *wt)
+{
+   static struct strbuf sb = STRBUF_INIT;
+
+   if (!file_exists(git_common_path("worktrees/%s/locked", wt->id)))
+   return NULL;
+
+   strbuf_reset();
+   if (strbuf_read_file(,
+git_common_path("worktrees/%s/locked", wt->id),
+0) < 0)
+   die_errno(_("failed to read '%s'"),
+ git_common_path("worktrees/%s/locked", wt->id));
+
+   strbuf_rtrim();
+   return sb.buf;
+}
+
 static int report(int quiet, const char *fmt, ...)
 {
va_list params;
diff --git a/worktree.h b/worktree.h
index bbe40ef..cbd5389 100644
--- a/worktree.h
+++ b/worktree.h
@@ -39,6 +39,12 @@ extern struct worktree *find_worktree_by_path(struct 
worktree **list,
 extern int is_main_worktree(const struct worktree *wt);
 
 /*
+ * Return the reason string if the given worktree is locked. Return
+ * NULL otherwise.
+ */
+extern const char *is_worktree_locked(const struct worktree *wt);
+
+/*
  * Return zero if the worktree is in good condition.
  */
 extern int validate_worktree(const struct worktree *wt, int quiet);
-- 
2.8.0.rc0.210.gd302cd2

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 14/25] worktree.c: add find_worktree_by_path()

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 worktree.c | 16 
 worktree.h |  6 ++
 2 files changed, 22 insertions(+)

diff --git a/worktree.c b/worktree.c
index b4e4b57..e444ad1 100644
--- a/worktree.c
+++ b/worktree.c
@@ -198,6 +198,22 @@ const char *get_worktree_git_dir(const struct worktree *wt)
return get_git_common_dir();
 }
 
+struct worktree *find_worktree_by_path(struct worktree **list,
+  const char *path_)
+{
+   char *path = xstrdup(real_path(path_));
+   struct worktree *wt = NULL;
+
+   while (*list) {
+   wt = *list++;
+   if (!strcmp_icase(path, real_path(wt->path)))
+   break;
+   wt = NULL;
+   }
+   free(path);
+   return wt;
+}
+
 char *find_shared_symref(const char *symref, const char *target)
 {
char *existing = NULL;
diff --git a/worktree.h b/worktree.h
index 0ba07ab..c163b6b 100644
--- a/worktree.h
+++ b/worktree.h
@@ -28,6 +28,12 @@ extern struct worktree **get_worktrees(void);
 extern const char *get_worktree_git_dir(const struct worktree *wt);
 
 /*
+ * Search a worktree by its path. Paths are normalized internally.
+ */
+extern struct worktree *find_worktree_by_path(struct worktree **list,
+ const char *path_);
+
+/*
  * Free up the memory for worktree
  */
 extern void clear_worktree(struct worktree *);
-- 
2.8.0.rc0.210.gd302cd2

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 15/25] worktree.c: add is_main_worktree()

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 worktree.c | 5 +
 worktree.h | 5 +
 2 files changed, 10 insertions(+)

diff --git a/worktree.c b/worktree.c
index e444ad1..e878f49 100644
--- a/worktree.c
+++ b/worktree.c
@@ -214,6 +214,11 @@ struct worktree *find_worktree_by_path(struct worktree 
**list,
return wt;
 }
 
+int is_main_worktree(const struct worktree *wt)
+{
+   return wt && !wt->id;
+}
+
 char *find_shared_symref(const char *symref, const char *target)
 {
char *existing = NULL;
diff --git a/worktree.h b/worktree.h
index c163b6b..c7a4d20 100644
--- a/worktree.h
+++ b/worktree.h
@@ -34,6 +34,11 @@ extern struct worktree *find_worktree_by_path(struct 
worktree **list,
  const char *path_);
 
 /*
+ * Return true if the given worktree is the main one.
+ */
+extern int is_main_worktree(const struct worktree *wt);
+
+/*
  * Free up the memory for worktree
  */
 extern void clear_worktree(struct worktree *);
-- 
2.8.0.rc0.210.gd302cd2

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 22/25] worktree: add "unlock" command

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 Documentation/git-worktree.txt |  5 +
 builtin/worktree.c | 31 +++
 contrib/completion/git-completion.bash |  2 +-
 t/t2028-worktree-move.sh   | 14 ++
 4 files changed, 51 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 9f0c9f0..8315e5b 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -13,6 +13,7 @@ SYNOPSIS
 'git worktree list' [--porcelain]
 'git worktree lock' [--reason ] 
 'git worktree prune' [-n] [-v] [--expire ]
+'git worktree unlock' 
 
 DESCRIPTION
 ---
@@ -72,6 +73,10 @@ prune::
 
 Prune working tree information in $GIT_DIR/worktrees.
 
+unlock::
+
+Unlock a worktree, allowing it to be pruned, moved or deleted.
+
 OPTIONS
 ---
 
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 736caff..13f4083 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -16,6 +16,7 @@ static const char * const worktree_usage[] = {
N_("git worktree list []"),
N_("git worktree lock [] "),
N_("git worktree prune []"),
+   N_("git worktree unlock "),
NULL
 };
 
@@ -491,6 +492,34 @@ static int lock_worktree(int ac, const char **av, const 
char *prefix)
return 0;
 }
 
+static int unlock_worktree(int ac, const char **av, const char *prefix)
+{
+   struct option options[] = {
+   OPT_END()
+   };
+   struct worktree **worktrees, *wt;
+   struct strbuf dst = STRBUF_INIT;
+
+   ac = parse_options(ac, av, prefix, options, worktree_usage, 0);
+   if (ac != 1)
+   usage_with_options(worktree_usage, options);
+
+   strbuf_addstr(, prefix_filename(prefix,
+   strlen(prefix),
+   av[0]));
+
+   worktrees = get_worktrees();
+   wt = find_worktree_by_path(worktrees, dst.buf);
+   if (!wt)
+   die(_("'%s' is not a working directory"), av[0]);
+   if (is_main_worktree(wt))
+   die(_("'%s' is a main working directory"), av[0]);
+   if (!is_worktree_locked(wt))
+   die(_("not locked"));
+
+   return unlink_or_warn(git_common_path("worktrees/%s/locked", wt->id));
+}
+
 int cmd_worktree(int ac, const char **av, const char *prefix)
 {
struct option options[] = {
@@ -509,5 +538,7 @@ int cmd_worktree(int ac, const char **av, const char 
*prefix)
return list(ac - 1, av + 1, prefix);
if (!strcmp(av[1], "lock"))
return lock_worktree(ac - 1, av + 1, prefix);
+   if (!strcmp(av[1], "unlock"))
+   return unlock_worktree(ac - 1, av + 1, prefix);
usage_with_options(worktree_usage, options);
 }
diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index cdae408..d7d92ac 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2596,7 +2596,7 @@ _git_whatchanged ()
 
 _git_worktree ()
 {
-   local subcommands="add list lock prune"
+   local subcommands="add list lock prune unlock"
local subcommand="$(__git_find_on_cmdline "$subcommands")"
if [ -z "$subcommand" ]; then
__gitcomp "$subcommands"
diff --git a/t/t2028-worktree-move.sh b/t/t2028-worktree-move.sh
index 97434be..f4b2816 100755
--- a/t/t2028-worktree-move.sh
+++ b/t/t2028-worktree-move.sh
@@ -31,4 +31,18 @@ test_expect_success 'lock worktree twice' '
test_cmp expected .git/worktrees/source/locked
 '
 
+test_expect_success 'unlock main worktree' '
+   test_must_fail git worktree unlock .
+'
+
+test_expect_success 'unlock linked worktree' '
+   git worktree unlock source &&
+   test_path_is_missing .git/worktrees/source/locked
+'
+
+test_expect_success 'unlock worktree twice' '
+   test_must_fail git worktree unlock source &&
+   test_path_is_missing .git/worktrees/source/locked
+'
+
 test_done
-- 
2.8.0.rc0.210.gd302cd2

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 25/25] worktree: add "remove" command

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 Documentation/git-worktree.txt | 22 +
 builtin/worktree.c | 81 ++
 contrib/completion/git-completion.bash |  5 ++-
 t/t2028-worktree-move.sh   | 26 +++
 4 files changed, 124 insertions(+), 10 deletions(-)

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index a302f0a..60ad465 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -14,6 +14,7 @@ SYNOPSIS
 'git worktree lock' [--reason ] 
 'git worktree move'  
 'git worktree prune' [-n] [-v] [--expire ]
+'git worktree remove' [--force] 
 'git worktree unlock' 
 
 DESCRIPTION
@@ -78,6 +79,14 @@ prune::
 
 Prune working tree information in $GIT_DIR/worktrees.
 
+remove::
+
+Remove a worktree. Only clean worktrees, no untracked files and no
+modification in tracked files, can be removed. Unclean worktrees can
+be removed with `--force`. Main worktree cannot be removed. It needs
+to be converted to a linked worktree first by moving the repository
+away.
+
 unlock::
 
 Unlock a worktree, allowing it to be pruned, moved or deleted.
@@ -87,9 +96,10 @@ OPTIONS
 
 -f::
 --force::
-   By default, `add` refuses to create a new working tree when ``
-   is already checked out by another working tree. This option overrides
-   that safeguard.
+   By default, `add` refuses to create a new working tree when
+   `` is already checked out by another working tree and
+   `remove` refuses to remove an unclean worktree. This option
+   overrides that safeguard.
 
 -b ::
 -B ::
@@ -234,12 +244,6 @@ Multiple checkout in general is still experimental, and 
the support
 for submodules is incomplete. It is NOT recommended to make multiple
 checkouts of a superproject.
 
-git-worktree could provide more automation for tasks currently
-performed manually, such as:
-
-- `remove` to remove a linked working tree and its administrative files (and
-  warn if the working tree is dirty)
-
 GIT
 ---
 Part of the linkgit:git[1] suite
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 5402a4e..084f8fd 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -17,6 +17,7 @@ static const char * const worktree_usage[] = {
N_("git worktree lock [] "),
N_("git worktree move  "),
N_("git worktree prune []"),
+   N_("git worktree remove [] "),
N_("git worktree unlock "),
NULL
 };
@@ -595,6 +596,84 @@ static int move_worktree(int ac, const char **av, const 
char *prefix)
return update_worktree_location(wt, dst.buf);
 }
 
+static int remove_worktree(int ac, const char **av, const char *prefix)
+{
+   int force = 0;
+   struct option options[] = {
+   OPT_BOOL(0, "force", ,
+N_("force removing even if the worktree is dirty")),
+   OPT_END()
+   };
+   struct worktree **worktrees, *wt;
+   struct strbuf dst = STRBUF_INIT;
+   const char *reason;
+   int ret = 0;
+
+   ac = parse_options(ac, av, prefix, options, worktree_usage, 0);
+   if (ac != 1)
+   usage_with_options(worktree_usage, options);
+
+   strbuf_addstr(, prefix_filename(prefix,
+   strlen(prefix),
+   av[0]));
+
+   worktrees = get_worktrees();
+   wt = find_worktree_by_path(worktrees, dst.buf);
+   if (!wt)
+   die(_("'%s' is not a working directory"), av[0]);
+   if (is_main_worktree(wt))
+   die(_("'%s' is a main working directory"), av[0]);
+   if ((reason = is_worktree_locked(wt))) {
+   if (*reason)
+   die(_("already locked, reason: %s"), reason);
+   die(_("already locked, no reason"));
+   }
+   if (validate_worktree(wt, 0))
+   return -1;
+
+   if (!force) {
+   struct argv_array child_env = ARGV_ARRAY_INIT;
+   struct child_process cp;
+   char buf[1];
+
+   argv_array_pushf(_env, "%s=%s/.git",
+GIT_DIR_ENVIRONMENT, wt->path);
+   argv_array_pushf(_env, "%s=%s",
+GIT_WORK_TREE_ENVIRONMENT, wt->path);
+   memset(, 0, sizeof(cp));
+   argv_array_pushl(, "status", "--porcelain", NULL);
+   cp.env = child_env.argv;
+   cp.git_cmd = 1;
+   cp.dir = wt->path;
+   cp.out = -1;
+   ret = start_command();
+   if (ret)
+   die_errno(_("failed to run git-status on '%s', code 
%d"),
+ av[0], ret);
+   ret = xread(cp.out, buf, sizeof(buf));
+   if (ret)
+   die(_("'%s' is dirty, use --force to delete it"), 
av[0]);
+   close(cp.out);
+  

[PATCH 20/25] worktree: simplify prefixing paths

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 builtin/worktree.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index e041d7f..a9e91ab 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -330,7 +330,7 @@ static int add(int ac, const char **av, const char *prefix)
if (ac < 1 || ac > 2)
usage_with_options(worktree_usage, options);
 
-   path = prefix ? prefix_filename(prefix, strlen(prefix), av[0]) : av[0];
+   path = prefix_filename(prefix, strlen(prefix), av[0]);
branch = ac < 2 ? "HEAD" : av[1];
 
opts.force_new_branch = !!new_branch_force;
@@ -460,6 +460,8 @@ int cmd_worktree(int ac, const char **av, const char 
*prefix)
 
if (ac < 2)
usage_with_options(worktree_usage, options);
+   if (!prefix)
+   prefix = "";
if (!strcmp(av[1], "add"))
return add(ac - 1, av + 1, prefix);
if (!strcmp(av[1], "prune"))
-- 
2.8.0.rc0.210.gd302cd2

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 19/25] worktree: avoid 0{40}, too many zeroes, hard to read

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 builtin/worktree.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 7ff66fa..e041d7f 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -261,7 +261,7 @@ static int add_worktree(const char *path, const char 
*refname,
 */
strbuf_reset();
strbuf_addf(, "%s/HEAD", sb_repo.buf);
-   write_file(sb.buf, "");
+   write_file(sb.buf, sha1_to_hex(null_sha1));
strbuf_reset();
strbuf_addf(, "%s/commondir", sb_repo.buf);
write_file(sb.buf, "../..");
-- 
2.8.0.rc0.210.gd302cd2

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 23/25] worktree: add "move" commmand

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 Documentation/git-worktree.txt |  6 +++-
 builtin/worktree.c | 60 ++
 contrib/completion/git-completion.bash |  2 +-
 t/t2028-worktree-move.sh   | 29 
 4 files changed, 95 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 8315e5b..a302f0a 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -12,6 +12,7 @@ SYNOPSIS
 'git worktree add' [-f] [--detach] [-b ]  []
 'git worktree list' [--porcelain]
 'git worktree lock' [--reason ] 
+'git worktree move'  
 'git worktree prune' [-n] [-v] [--expire ]
 'git worktree unlock' 
 
@@ -69,6 +70,10 @@ When a worktree is locked, it cannot be pruned, moved or 
deleted. For
 example, if the worktree is on portable device that is not available
 when "git worktree " is executed.
 
+move::
+
+Move a worktree to a new location. Note that the main worktree cannot be moved.
+
 prune::
 
 Prune working tree information in $GIT_DIR/worktrees.
@@ -234,7 +239,6 @@ performed manually, such as:
 
 - `remove` to remove a linked working tree and its administrative files (and
   warn if the working tree is dirty)
-- `mv` to move or rename a working tree and update its administrative files
 
 GIT
 ---
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 13f4083..a988913 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -15,6 +15,7 @@ static const char * const worktree_usage[] = {
N_("git worktree add []  []"),
N_("git worktree list []"),
N_("git worktree lock [] "),
+   N_("git worktree move  "),
N_("git worktree prune []"),
N_("git worktree unlock "),
NULL
@@ -520,6 +521,63 @@ static int unlock_worktree(int ac, const char **av, const 
char *prefix)
return unlink_or_warn(git_common_path("worktrees/%s/locked", wt->id));
 }
 
+static int move_worktree(int ac, const char **av, const char *prefix)
+{
+   struct option options[] = {
+   OPT_END()
+   };
+   struct worktree **worktrees, *wt;
+   struct strbuf dst = STRBUF_INIT;
+   struct strbuf src = STRBUF_INIT;
+   const char *reason;
+
+   ac = parse_options(ac, av, prefix, options, worktree_usage, 0);
+   if (ac != 2)
+   usage_with_options(worktree_usage, options);
+
+   strbuf_addstr(, prefix_filename(prefix,
+   strlen(prefix),
+   av[1]));
+   if (file_exists(dst.buf))
+   die(_("target '%s' already exists"), av[1]);
+
+   worktrees = get_worktrees();
+   strbuf_addstr(, prefix_filename(prefix,
+strlen(prefix),
+av[0]));
+   wt = find_worktree_by_path(worktrees, src.buf);
+   if (!wt)
+   die(_("'%s' is not a working directory"), av[0]);
+   if (is_main_worktree(wt))
+   die(_("'%s' is a main working directory"), av[0]);
+   if ((reason = is_worktree_locked(wt))) {
+   if (*reason)
+   die(_("already locked, reason: %s"), reason);
+   die(_("already locked, no reason"));
+   }
+   if (validate_worktree(wt, 0))
+   return -1;
+
+   /*
+* First try. Atomically move, and probably cheaper, if both
+* source and target are on the same file system.
+*/
+   if (rename(src.buf, dst.buf) == -1) {
+   if (errno != EXDEV)
+   die_errno(_("failed to move '%s' to '%s'"),
+ src.buf, dst.buf);
+
+   /* second try.. */
+   if (copy_dir_recursively(src.buf, dst.buf))
+   die(_("failed to copy '%s' to '%s'"),
+   src.buf, dst.buf);
+   else
+   (void)remove_dir_recursively(, 0);
+   }
+
+   return update_worktree_location(wt, dst.buf);
+}
+
 int cmd_worktree(int ac, const char **av, const char *prefix)
 {
struct option options[] = {
@@ -540,5 +598,7 @@ int cmd_worktree(int ac, const char **av, const char 
*prefix)
return lock_worktree(ac - 1, av + 1, prefix);
if (!strcmp(av[1], "unlock"))
return unlock_worktree(ac - 1, av + 1, prefix);
+   if (!strcmp(av[1], "move"))
+   return move_worktree(ac - 1, av + 1, prefix);
usage_with_options(worktree_usage, options);
 }
diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index d7d92ac..022d990 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2596,7 +2596,7 @@ _git_whatchanged ()
 
 _git_worktree ()
 {
-   local subcommands="add list lock prune unlock"
+   local 

[PATCH 11/25] worktree.c: use is_dot_or_dotdot()

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 builtin/worktree.c | 2 +-
 worktree.c | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 575f899..7ff66fa 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -94,7 +94,7 @@ static void prune_worktrees(void)
if (!dir)
return;
while ((d = readdir(dir)) != NULL) {
-   if (!strcmp(d->d_name, ".") || !strcmp(d->d_name, ".."))
+   if (is_dot_or_dotdot(d->d_name))
continue;
strbuf_reset();
if (!prune_worktree(d->d_name, ))
diff --git a/worktree.c b/worktree.c
index 6181a66..ddb8cb7 100644
--- a/worktree.c
+++ b/worktree.c
@@ -2,6 +2,7 @@
 #include "refs.h"
 #include "strbuf.h"
 #include "worktree.h"
+#include "dir.h"
 
 void free_worktrees(struct worktree **worktrees)
 {
@@ -173,7 +174,7 @@ struct worktree **get_worktrees(void)
if (dir) {
while ((d = readdir(dir)) != NULL) {
struct worktree *linked = NULL;
-   if (!strcmp(d->d_name, ".") || !strcmp(d->d_name, ".."))
+   if (is_dot_or_dotdot(d->d_name))
continue;
 
if ((linked = get_linked_worktree(d->d_name))) {
-- 
2.8.0.rc0.210.gd302cd2

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 08/25] completion: support git-worktree

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 contrib/completion/git-completion.bash | 23 +++
 1 file changed, 23 insertions(+)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index e3918c8..f66db2d 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2594,6 +2594,29 @@ _git_whatchanged ()
_git_log
 }
 
+_git_worktree ()
+{
+   local subcommands="add list prune"
+   local subcommand="$(__git_find_on_cmdline "$subcommands")"
+   if [ -z "$subcommand" ]; then
+   __gitcomp "$subcommands"
+   else
+   case "$subcommand,$cur" in
+   add,--*)
+   __gitcomp "--detach --force"
+   ;;
+   list,--*)
+   __gitcomp "--porcelain"
+   ;;
+   prune,--*)
+   __gitcomp "--dry-run --expire --verbose"
+   ;;
+   *)
+   ;;
+   esac
+   fi
+}
+
 __git_main ()
 {
local i c=1 command __git_dir
-- 
2.8.0.rc0.210.gd302cd2

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 06/25] copy.c: style fix

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 copy.c | 50 +-
 1 file changed, 29 insertions(+), 21 deletions(-)

diff --git a/copy.c b/copy.c
index cdb38d5..00f8349 100644
--- a/copy.c
+++ b/copy.c
@@ -111,8 +111,10 @@ int FAST_FUNC copy_file(const char *source, const char 
*dest, int flags)
if (dest_exists) {
if (!S_ISDIR(dest_stat.st_mode))
return error(_("target '%s' is not a 
directory"), dest);
-   /* race here: user can substitute a symlink between
-* this check and actual creation of files inside dest 
*/
+   /*
+* race here: user can substitute a symlink between
+* this check and actual creation of files inside dest
+*/
} else {
/* Create DEST */
mode_t mode;
@@ -130,22 +132,24 @@ int FAST_FUNC copy_file(const char *source, const char 
*dest, int flags)
if (lstat(dest, _stat) < 0)
return sys_error(_("can't stat '%s'"), dest);
}
-   /* remember (dev,inode) of each created dir.
-* NULL: name is not remembered */
+   /*
+* remember (dev,inode) of each created dir. name is
+* not remembered
+*/
add_to_ino_dev_hashtable(_stat, NULL);
 
/* Recursively copy files in SOURCE */
dp = opendir(source);
-   if (dp == NULL) {
+   if (!dp) {
retval = -1;
goto preserve_mode_ugid_time;
}
 
-   while ((d = readdir(dp)) != NULL) {
+   while ((d = readdir(dp))) {
char *new_source, *new_dest;
 
new_source = concat_subpath_file(source, d->d_name);
-   if (new_source == NULL)
+   if (!new_source)
continue;
new_dest = concat_path_file(dest, d->d_name);
if (copy_file(new_source, new_dest, flags & 
~FILEUTILS_DEREFERENCE_L0) < 0)
@@ -155,16 +159,15 @@ int FAST_FUNC copy_file(const char *source, const char 
*dest, int flags)
}
closedir(dp);
 
-   if (!dest_exists
-&& chmod(dest, source_stat.st_mode & ~saved_umask) < 0
-   ) {
+   if (!dest_exists &&
+   chmod(dest, source_stat.st_mode & ~saved_umask) < 0) {
sys_error(_("can't preserve permissions of '%s'"), 
dest);
/* retval = -1; - WRONG! copy *WAS* made */
}
goto preserve_mode_ugid_time;
}
 
-   if (S_ISREG(source_stat.st_mode) ) { /* "cp [-opts] regular_file 
thing2" */
+   if (S_ISREG(source_stat.st_mode)) { /* "cp [-opts] regular_file thing2" 
*/
int src_fd;
int dst_fd;
mode_t new_mode;
@@ -199,7 +202,7 @@ int FAST_FUNC copy_file(const char *source, const char 
*dest, int flags)
if (!S_ISREG(source_stat.st_mode))
new_mode = 0666;
 
-   // POSIX way is a security problem versus (sym)link attacks
+   /* POSIX way is a security problem versus (sym)link attacks */
if (!ENABLE_FEATURE_NON_POSIX_CP) {
dst_fd = open(dest, O_WRONLY|O_CREAT|O_TRUNC, new_mode);
} else { /* safe way: */
@@ -226,13 +229,15 @@ int FAST_FUNC copy_file(const char *source, const char 
*dest, int flags)
retval = sys_error(_("error writing to '%s'"), dest);
/* ...but read size is already checked by bb_copyfd_eof */
close(src_fd);
-   /* "cp /dev/something new_file" should not
-* copy mode of /dev/something */
+   /*
+* "cp /dev/something new_file" should not
+* copy mode of /dev/something
+*/
if (!S_ISREG(source_stat.st_mode))
return retval;
goto preserve_mode_ugid_time;
}
- dont_cat:
+dont_cat:
 
/* Source is a symlink or a special file */
/* We are lazy here, a bit lax with races... */
@@ -252,20 +257,23 @@ int FAST_FUNC copy_file(const char *source, const char 
*dest, int flags)
if (lchown(dest, source_stat.st_uid, 
source_stat.st_gid) < 0)
sys_error(_("can't preserve %s of '%s'"), 
"ownership", dest);
}
-   /* _Not_ jumping to preserve_mode_ugid_time:
-* symlinks don't have those */
+   /*
+

[PATCH 09/25] git-worktree.txt: keep subcommand listing in alphabetical order

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 Documentation/git-worktree.txt | 10 +-
 builtin/worktree.c |  2 +-
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 62c76c1..1c9d7c1 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -10,8 +10,8 @@ SYNOPSIS
 
 [verse]
 'git worktree add' [-f] [--detach] [-b ]  []
-'git worktree prune' [-n] [-v] [--expire ]
 'git worktree list' [--porcelain]
+'git worktree prune' [-n] [-v] [--expire ]
 
 DESCRIPTION
 ---
@@ -54,10 +54,6 @@ If `` is omitted and neither `-b` nor `-B` nor 
`--detached` used,
 then, as a convenience, a new branch based at HEAD is created automatically,
 as if `-b $(basename )` was specified.
 
-prune::
-
-Prune working tree information in $GIT_DIR/worktrees.
-
 list::
 
 List details of each worktree.  The main worktree is listed first, followed by
@@ -65,6 +61,10 @@ each of the linked worktrees.  The output details include if 
the worktree is
 bare, the revision currently checked out, and the branch currently checked out
 (or 'detached HEAD' if none).
 
+prune::
+
+Prune working tree information in $GIT_DIR/worktrees.
+
 OPTIONS
 ---
 
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 38b5609..575f899 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -13,8 +13,8 @@
 
 static const char * const worktree_usage[] = {
N_("git worktree add []  []"),
-   N_("git worktree prune []"),
N_("git worktree list []"),
+   N_("git worktree prune []"),
NULL
 };
 
-- 
2.8.0.rc0.210.gd302cd2

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 10/25] path.c: add git_common_path() and strbuf_git_common_path()

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 cache.h |  3 +++
 path.c  | 29 +
 2 files changed, 32 insertions(+)

diff --git a/cache.h b/cache.h
index 213a8d3..c81f654 100644
--- a/cache.h
+++ b/cache.h
@@ -767,11 +767,14 @@ extern int check_repository_format(void);
  */
 extern const char *mkpath(const char *fmt, ...) __attribute__((format (printf, 
1, 2)));
 extern const char *git_path(const char *fmt, ...) __attribute__((format 
(printf, 1, 2)));
+extern const char *git_common_path(const char *fmt, ...) __attribute__((format 
(printf, 1, 2)));
 
 extern char *mksnpath(char *buf, size_t n, const char *fmt, ...)
__attribute__((format (printf, 3, 4)));
 extern void strbuf_git_path(struct strbuf *sb, const char *fmt, ...)
__attribute__((format (printf, 2, 3)));
+extern void strbuf_git_common_path(struct strbuf *sb, const char *fmt, ...)
+   __attribute__((format (printf, 2, 3)));
 extern char *git_path_buf(struct strbuf *buf, const char *fmt, ...)
__attribute__((format (printf, 2, 3)));
 extern void strbuf_git_path_submodule(struct strbuf *sb, const char *path,
diff --git a/path.c b/path.c
index 969b494..e315dd6 100644
--- a/path.c
+++ b/path.c
@@ -503,6 +503,35 @@ void strbuf_git_path_submodule(struct strbuf *buf, const 
char *path,
va_end(args);
 }
 
+static void do_git_common_path(struct strbuf *buf,
+  const char *fmt,
+  va_list args)
+{
+   strbuf_addstr(buf, get_git_common_dir());
+   if (buf->len && !is_dir_sep(buf->buf[buf->len - 1]))
+   strbuf_addch(buf, '/');
+   strbuf_vaddf(buf, fmt, args);
+   strbuf_cleanup_path(buf);
+}
+
+const char *git_common_path(const char *fmt, ...)
+{
+   struct strbuf *pathname = get_pathname();
+   va_list args;
+   va_start(args, fmt);
+   do_git_common_path(pathname, fmt, args);
+   va_end(args);
+   return pathname->buf;
+}
+
+void strbuf_git_common_path(struct strbuf *sb, const char *fmt, ...)
+{
+   va_list args;
+   va_start(args, fmt);
+   do_git_common_path(sb, fmt, args);
+   va_end(args);
+}
+
 int validate_headref(const char *path)
 {
struct stat st;
-- 
2.8.0.rc0.210.gd302cd2

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 04/25] copy.c: delete unused code in copy_file()

[Nguyễn Thái Ngọc Duy]

 - selinux preservation code
 - make-link code
 - delete link dereference code
 - non-recursive copy code
 - stat no preservation code
 - verbose printing code

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 copy.c | 101 +
 1 file changed, 7 insertions(+), 94 deletions(-)

diff --git a/copy.c b/copy.c
index 29e9d5b..d24a8ae 100644
--- a/copy.c
+++ b/copy.c
@@ -82,14 +82,7 @@ int FAST_FUNC copy_file(const char *source, const char 
*dest, int flags)
smallint dest_exists = 0;
smallint ovr;
 
-/* Inverse of cp -d ("cp without -d") */
-#define FLAGS_DEREF (flags & (FILEUTILS_DEREFERENCE + 
FILEUTILS_DEREFERENCE_L0))
-
-   if ((FLAGS_DEREF ? stat : lstat)(source, _stat) < 0) {
-   /* This may be a dangling symlink.
-* Making [sym]links to dangling symlinks works, so... */
-   if (flags & (FILEUTILS_MAKE_SOFTLINK|FILEUTILS_MAKE_HARDLINK))
-   goto make_links;
+   if (lstat(source, _stat) < 0) {
bb_perror_msg("can't stat '%s'", source);
return -1;
}
@@ -109,35 +102,12 @@ int FAST_FUNC copy_file(const char *source, const char 
*dest, int flags)
dest_exists = 1;
}
 
-#if ENABLE_SELINUX
-   if ((flags & FILEUTILS_PRESERVE_SECURITY_CONTEXT) && 
is_selinux_enabled() > 0) {
-   security_context_t con;
-   if (lgetfilecon(source, ) >= 0) {
-   if (setfscreatecon(con) < 0) {
-   bb_perror_msg("can't set setfscreatecon %s", 
con);
-   freecon(con);
-   return -1;
-   }
-   } else if (errno == ENOTSUP || errno == ENODATA) {
-   setfscreatecon_or_die(NULL);
-   } else {
-   bb_perror_msg("can't lgetfilecon %s", source);
-   return -1;
-   }
-   }
-#endif
-
if (S_ISDIR(source_stat.st_mode)) {
DIR *dp;
const char *tp;
struct dirent *d;
mode_t saved_umask = 0;
 
-   if (!(flags & FILEUTILS_RECUR)) {
-   bb_error_msg("omitting directory '%s'", source);
-   return -1;
-   }
-
/* Did we ever create source ourself before? */
tp = is_in_ino_dev_hashtable(_stat);
if (tp) {
@@ -160,8 +130,6 @@ int FAST_FUNC copy_file(const char *source, const char 
*dest, int flags)
saved_umask = umask(0);
 
mode = source_stat.st_mode;
-   if (!(flags & FILEUTILS_PRESERVE_STATUS))
-   mode = source_stat.st_mode & ~saved_umask;
/* Allow owner to access new dir (at least for now) */
mode |= S_IRWXU;
if (mkdir(dest, mode) < 0) {
@@ -210,45 +178,17 @@ int FAST_FUNC copy_file(const char *source, const char 
*dest, int flags)
goto preserve_mode_ugid_time;
}
 
-   if (flags & (FILEUTILS_MAKE_SOFTLINK|FILEUTILS_MAKE_HARDLINK)) {
-   int (*lf)(const char *oldpath, const char *newpath);
- make_links:
-   /* Hmm... maybe
-* if (DEREF && MAKE_SOFTLINK) source = realpath(source) ?
-* (but realpath returns NULL on dangling symlinks...) */
-   lf = (flags & FILEUTILS_MAKE_SOFTLINK) ? symlink : link;
-   if (lf(source, dest) < 0) {
-   ovr = ask_and_unlink(dest, flags);
-   if (ovr <= 0)
-   return ovr;
-   if (lf(source, dest) < 0) {
-   bb_perror_msg("can't create link '%s'", dest);
-   return -1;
-   }
-   }
-   /* _Not_ jumping to preserve_mode_ugid_time:
-* (sym)links don't have those */
-   return 0;
-   }
-
-   if (/* "cp thing1 thing2" without -R: just open and read() from thing1 
*/
-   !(flags & FILEUTILS_RECUR)
-   /* "cp [-opts] regular_file thing2" */
-|| S_ISREG(source_stat.st_mode)
-/* DEREF uses stat, which never returns S_ISLNK() == true.
- * So the below is never true: */
-/* || (FLAGS_DEREF && S_ISLNK(source_stat.st_mode)) */
-   ) {
+   if (S_ISREG(source_stat.st_mode) ) { /* "cp [-opts] regular_file 
thing2" */
int src_fd;
int dst_fd;
mode_t new_mode;
 
-   if (!FLAGS_DEREF && S_ISLNK(source_stat.st_mode)) {
+   if (S_ISLNK(source_stat.st_mode)) {
/* "cp -d symlink dst": create a link */
goto dont_cat;
}
 
-   

[PATCH 07/25] copy.c: convert copy_file() to copy_dir_recursively()

[Nguyễn Thái Ngọc Duy]

This finally enables busybox's copy_file() code under a new name
(because "copy_file" is already taken in Git code base). Because this
comes from busybox, POSIXy (or even Linuxy) behavior is expected. More
changes may be needed for Windows support.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 cache.h |   1 +
 copy.c  | 216 
 2 files changed, 179 insertions(+), 38 deletions(-)

diff --git a/cache.h b/cache.h
index 9f09540..213a8d3 100644
--- a/cache.h
+++ b/cache.h
@@ -1677,6 +1677,7 @@ extern void fprintf_or_die(FILE *, const char *fmt, ...);
 extern int copy_fd(int ifd, int ofd);
 extern int copy_file(const char *dst, const char *src, int mode);
 extern int copy_file_with_time(const char *dst, const char *src, int mode);
+extern int copy_dir_recursively(const char *source, const char *dest);
 
 extern void write_or_die(int fd, const void *buf, size_t count);
 extern int write_or_whine(int fd, const void *buf, size_t count, const char 
*msg);
diff --git a/copy.c b/copy.c
index 00f8349..f04ac87 100644
--- a/copy.c
+++ b/copy.c
@@ -1,4 +1,6 @@
 #include "cache.h"
+#include "dir.h"
+#include "hashmap.h"
 
 int copy_fd(int ifd, int ofd)
 {
@@ -66,21 +68,126 @@ int copy_file_with_time(const char *dst, const char *src, 
int mode)
return status;
 }
 
-#if 0
-/* Return:
- * -1 error, copy not made
- *  0 copy is made or user answered "no" in interactive mode
- *(failures to preserve mode/owner/times are not reported in exit code)
+struct inode_key {
+   struct hashmap_entry entry;
+   ino_t ino;
+   dev_t dev;
+   /*
+* Reportedly, on cramfs a file and a dir can have same ino.
+* Need to also remember "file/dir" bit:
+*/
+   char isdir; /* bool */
+};
+
+struct inode_value {
+   struct inode_key key;
+   char name[FLEX_ARRAY];
+};
+
+#define HASH_SIZE  311u   /* Should be prime */
+static inline unsigned hash_inode(ino_t i)
+{
+   return i % HASH_SIZE;
+}
+
+static int inode_cmp(const void *entry, const void *entry_or_key,
+const void *keydata)
+{
+   const struct inode_value *inode = entry;
+   const struct inode_key   *key   = entry_or_key;
+
+   return !(inode->key.ino   == key->ino &&
+inode->key.dev   == key->dev &&
+inode->key.isdir == key->isdir);
+}
+
+static const char *is_in_ino_dev_hashtable(const struct hashmap *map,
+  const struct stat *st)
+{
+   struct inode_key key;
+   struct inode_value *value;
+
+   key.entry.hash = hash_inode(st->st_ino);
+   key.ino= st->st_ino;
+   key.dev= st->st_dev;
+   key.isdir  = !!S_ISDIR(st->st_mode);
+   value  = hashmap_get(map, , NULL);
+   return value ? value->name : NULL;
+}
+
+static void add_to_ino_dev_hashtable(struct hashmap *map,
+const struct stat *st,
+const char *path)
+{
+   struct inode_value *v;
+   int len = strlen(path);
+
+   v = xmalloc(offsetof(struct inode_value, name) + len + 1);
+   v->key.entry.hash = hash_inode(st->st_ino);
+   v->key.ino= st->st_ino;
+   v->key.dev= st->st_dev;
+   v->key.isdir  = !!S_ISDIR(st->st_mode);
+   memcpy(v->name, path, len + 1);
+   hashmap_add(map, v);
+}
+
+/*
+ * Find out if the last character of a string matches the one given.
+ * Don't underrun the buffer if the string length is 0.
  */
-int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
+static inline char *last_char_is(const char *s, int c)
+{
+   if (s && *s) {
+   size_t sz = strlen(s) - 1;
+   s += sz;
+   if ( (unsigned char)*s == c)
+   return (char*)s;
+   }
+   return NULL;
+}
+
+static inline char *concat_path_file(const char *path, const char *filename)
+{
+   struct strbuf sb = STRBUF_INIT;
+   char *lc;
+
+   if (!path)
+   path = "";
+   lc = last_char_is(path, '/');
+   while (*filename == '/')
+   filename++;
+   strbuf_addf(, "%s%s%s", path, (lc==NULL ? "/" : ""), filename);
+   return strbuf_detach(, NULL);
+}
+
+static char *concat_subpath_file(const char *path, const char *f)
+{
+   if (f && is_dot_or_dotdot(f))
+   return NULL;
+   return concat_path_file(path, f);
+}
+
+static int do_unlink(const char *dest)
+{
+   int e = errno;
+
+   if (unlink(dest) < 0) {
+   errno = e; /* do not use errno from unlink */
+   return sys_error(_("can't create '%s'"), dest);
+   }
+   return 0;
+}
+
+static int copy_dir_1(struct hashmap *inode_map,
+ const char *source,
+ const char *dest)
 {
/* This is a recursive function, try to minimize stack usage */
-   /* 

[PATCH 05/25] copy.c: convert bb_(p)error_msg to (sys_)error

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 copy.c | 85 --
 1 file changed, 31 insertions(+), 54 deletions(-)

diff --git a/copy.c b/copy.c
index d24a8ae..cdb38d5 100644
--- a/copy.c
+++ b/copy.c
@@ -82,23 +82,16 @@ int FAST_FUNC copy_file(const char *source, const char 
*dest, int flags)
smallint dest_exists = 0;
smallint ovr;
 
-   if (lstat(source, _stat) < 0) {
-   bb_perror_msg("can't stat '%s'", source);
-   return -1;
-   }
+   if (lstat(source, _stat) < 0)
+   return sys_error(_("can't stat '%s'"), source);
 
if (lstat(dest, _stat) < 0) {
-   if (errno != ENOENT) {
-   bb_perror_msg("can't stat '%s'", dest);
-   return -1;
-   }
+   if (errno != ENOENT)
+   return sys_error(_("can't stat '%s'"), dest);
} else {
-   if (source_stat.st_dev == dest_stat.st_dev
-&& source_stat.st_ino == dest_stat.st_ino
-   ) {
-   bb_error_msg("'%s' and '%s' are the same file", source, 
dest);
-   return -1;
-   }
+   if (source_stat.st_dev == dest_stat.st_dev &&
+   source_stat.st_ino == dest_stat.st_ino)
+   return error(_("'%s' and '%s' are the same file"), 
source, dest);
dest_exists = 1;
}
 
@@ -110,18 +103,14 @@ int FAST_FUNC copy_file(const char *source, const char 
*dest, int flags)
 
/* Did we ever create source ourself before? */
tp = is_in_ino_dev_hashtable(_stat);
-   if (tp) {
+   if (tp)
/* We did! it's a recursion! man the lifeboats... */
-   bb_error_msg("recursion detected, omitting directory 
'%s'",
-   source);
-   return -1;
-   }
+   return error(_("recursion detected, omitting directory 
'%s'"),
+source);
 
if (dest_exists) {
-   if (!S_ISDIR(dest_stat.st_mode)) {
-   bb_error_msg("target '%s' is not a directory", 
dest);
-   return -1;
-   }
+   if (!S_ISDIR(dest_stat.st_mode))
+   return error(_("target '%s' is not a 
directory"), dest);
/* race here: user can substitute a symlink between
 * this check and actual creation of files inside dest 
*/
} else {
@@ -134,15 +123,12 @@ int FAST_FUNC copy_file(const char *source, const char 
*dest, int flags)
mode |= S_IRWXU;
if (mkdir(dest, mode) < 0) {
umask(saved_umask);
-   bb_perror_msg("can't create directory '%s'", 
dest);
-   return -1;
+   return sys_error(_("can't create directory 
'%s'"), dest);
}
umask(saved_umask);
/* need stat info for add_to_ino_dev_hashtable */
-   if (lstat(dest, _stat) < 0) {
-   bb_perror_msg("can't stat '%s'", dest);
-   return -1;
-   }
+   if (lstat(dest, _stat) < 0)
+   return sys_error(_("can't stat '%s'"), dest);
}
/* remember (dev,inode) of each created dir.
 * NULL: name is not remembered */
@@ -172,7 +158,7 @@ int FAST_FUNC copy_file(const char *source, const char 
*dest, int flags)
if (!dest_exists
 && chmod(dest, source_stat.st_mode & ~saved_umask) < 0
) {
-   bb_perror_msg("can't preserve %s of '%s'", 
"permissions", dest);
+   sys_error(_("can't preserve permissions of '%s'"), 
dest);
/* retval = -1; - WRONG! copy *WAS* made */
}
goto preserve_mode_ugid_time;
@@ -196,10 +182,8 @@ int FAST_FUNC copy_file(const char *source, const char 
*dest, int flags)
ovr = ask_and_unlink(dest, flags);
if (ovr <= 0)
return ovr;
-   if (link(link_target, dest) < 0) {
-   bb_perror_msg("can't create 
link '%s'", dest);
-   return -1;
-   }
+   if (link(link_target, dest) < 0)
+

[PATCH 12/25] worktree.c: store "id" instead of "git_dir"

[Nguyễn Thái Ngọc Duy]

We can reconstruct git_dir from id quite easily. It's a bit hackier to
do the reverse.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 worktree.c | 29 -
 worktree.h |  7 ++-
 2 files changed, 22 insertions(+), 14 deletions(-)

diff --git a/worktree.c b/worktree.c
index ddb8cb7..4c38414 100644
--- a/worktree.c
+++ b/worktree.c
@@ -10,7 +10,7 @@ void free_worktrees(struct worktree **worktrees)
 
for (i = 0; worktrees[i]; i++) {
free(worktrees[i]->path);
-   free(worktrees[i]->git_dir);
+   free(worktrees[i]->id);
free(worktrees[i]->head_ref);
free(worktrees[i]);
}
@@ -75,13 +75,11 @@ static struct worktree *get_main_worktree(void)
struct worktree *worktree = NULL;
struct strbuf path = STRBUF_INIT;
struct strbuf worktree_path = STRBUF_INIT;
-   struct strbuf gitdir = STRBUF_INIT;
struct strbuf head_ref = STRBUF_INIT;
int is_bare = 0;
int is_detached = 0;
 
-   strbuf_addf(, "%s", absolute_path(get_git_common_dir()));
-   strbuf_addbuf(_path, );
+   strbuf_addstr(_path, absolute_path(get_git_common_dir()));
is_bare = !strbuf_strip_suffix(_path, "/.git");
if (is_bare)
strbuf_strip_suffix(_path, "/.");
@@ -93,7 +91,7 @@ static struct worktree *get_main_worktree(void)
 
worktree = xmalloc(sizeof(struct worktree));
worktree->path = strbuf_detach(_path, NULL);
-   worktree->git_dir = strbuf_detach(, NULL);
+   worktree->id = NULL;
worktree->is_bare = is_bare;
worktree->head_ref = NULL;
worktree->is_detached = is_detached;
@@ -101,7 +99,6 @@ static struct worktree *get_main_worktree(void)
 
 done:
strbuf_release();
-   strbuf_release();
strbuf_release(_path);
strbuf_release(_ref);
return worktree;
@@ -112,16 +109,13 @@ static struct worktree *get_linked_worktree(const char 
*id)
struct worktree *worktree = NULL;
struct strbuf path = STRBUF_INIT;
struct strbuf worktree_path = STRBUF_INIT;
-   struct strbuf gitdir = STRBUF_INIT;
struct strbuf head_ref = STRBUF_INIT;
int is_detached = 0;
 
if (!id)
die("Missing linked worktree name");
 
-   strbuf_addf(, "%s/worktrees/%s",
-   absolute_path(get_git_common_dir()), id);
-   strbuf_addf(, "%s/gitdir", gitdir.buf);
+   strbuf_git_common_path(, "worktrees/%s/gitdir", id);
if (strbuf_read_file(_path, path.buf, 0) <= 0)
/* invalid gitdir file */
goto done;
@@ -141,7 +135,7 @@ static struct worktree *get_linked_worktree(const char *id)
 
worktree = xmalloc(sizeof(struct worktree));
worktree->path = strbuf_detach(_path, NULL);
-   worktree->git_dir = strbuf_detach(, NULL);
+   worktree->id = xstrdup(id);
worktree->is_bare = 0;
worktree->head_ref = NULL;
worktree->is_detached = is_detached;
@@ -149,7 +143,6 @@ static struct worktree *get_linked_worktree(const char *id)
 
 done:
strbuf_release();
-   strbuf_release();
strbuf_release(_path);
strbuf_release(_ref);
return worktree;
@@ -189,6 +182,14 @@ struct worktree **get_worktrees(void)
return list;
 }
 
+const char *get_worktree_git_dir(const struct worktree *wt)
+{
+   if (wt->id)
+   return git_common_path("worktrees/%s", wt->id);
+   else
+   return get_git_common_dir();
+}
+
 char *find_shared_symref(const char *symref, const char *target)
 {
char *existing = NULL;
@@ -200,7 +201,9 @@ char *find_shared_symref(const char *symref, const char 
*target)
for (i = 0; worktrees[i]; i++) {
strbuf_reset();
strbuf_reset();
-   strbuf_addf(, "%s/%s", worktrees[i]->git_dir, symref);
+   strbuf_addf(, "%s/%s",
+   get_worktree_git_dir(worktrees[i]),
+   symref);
 
if (parse_ref(path.buf, , NULL)) {
continue;
diff --git a/worktree.h b/worktree.h
index b4b3dda..e89d423 100644
--- a/worktree.h
+++ b/worktree.h
@@ -3,7 +3,7 @@
 
 struct worktree {
char *path;
-   char *git_dir;
+   char *id;
char *head_ref;
unsigned char head_sha1[20];
int is_detached;
@@ -23,6 +23,11 @@ struct worktree {
 extern struct worktree **get_worktrees(void);
 
 /*
+ * Return git dir of the worktree. Note that the path may be relative.
+ */
+extern const char *get_worktree_git_dir(const struct worktree *wt);
+
+/*
  * Free up the memory for worktree(s)
  */
 extern void free_worktrees(struct worktree **);
-- 
2.8.0.rc0.210.gd302cd2

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  

[PATCH 02/25] usage.c: add sys_error() that prints strerror() automatically

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 git-compat-util.h |  1 +
 usage.c   | 10 ++
 2 files changed, 11 insertions(+)

diff --git a/git-compat-util.h b/git-compat-util.h
index 4743954..e8e7765 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -412,6 +412,7 @@ extern NORETURN void usagef(const char *err, ...) 
__attribute__((format (printf,
 extern NORETURN void die(const char *err, ...) __attribute__((format (printf, 
1, 2)));
 extern NORETURN void die_errno(const char *err, ...) __attribute__((format 
(printf, 1, 2)));
 extern int error(const char *err, ...) __attribute__((format (printf, 1, 2)));
+extern int sys_error(const char *err, ...) __attribute__((format (printf, 1, 
2)));
 extern void warning(const char *err, ...) __attribute__((format (printf, 1, 
2)));
 
 #ifndef NO_OPENSSL
diff --git a/usage.c b/usage.c
index 0dba0c5..e7c37f2 100644
--- a/usage.c
+++ b/usage.c
@@ -148,6 +148,16 @@ void NORETURN die_errno(const char *fmt, ...)
va_end(params);
 }
 
+int sys_error(const char *fmt, ...)
+{
+   va_list params;
+
+   va_start(params, fmt);
+   error_routine(fmt_with_err(fmt), params);
+   va_end(params);
+   return -1;
+}
+
 #undef error
 int error(const char *err, ...)
 {
-- 
2.8.0.rc0.210.gd302cd2

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 03/25] copy.c: import copy_file() from busybox

[Nguyễn Thái Ngọc Duy]

This is busybox's unmodified copy_file() in libbb/copy_file.c from the
GPL2+ commit f2c043acfcf9dad9fd3d65821b81f89986bbe54e (busybox: fix
uninitialized memory when displaying IPv6 addresses - 2016-01-18)

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 copy.c | 331 +
 1 file changed, 331 insertions(+)

diff --git a/copy.c b/copy.c
index 574fa1f..29e9d5b 100644
--- a/copy.c
+++ b/copy.c
@@ -65,3 +65,334 @@ int copy_file_with_time(const char *dst, const char *src, 
int mode)
return copy_times(dst, src);
return status;
 }
+
+#if 0
+/* Return:
+ * -1 error, copy not made
+ *  0 copy is made or user answered "no" in interactive mode
+ *(failures to preserve mode/owner/times are not reported in exit code)
+ */
+int FAST_FUNC copy_file(const char *source, const char *dest, int flags)
+{
+   /* This is a recursive function, try to minimize stack usage */
+   /* NB: each struct stat is ~100 bytes */
+   struct stat source_stat;
+   struct stat dest_stat;
+   smallint retval = 0;
+   smallint dest_exists = 0;
+   smallint ovr;
+
+/* Inverse of cp -d ("cp without -d") */
+#define FLAGS_DEREF (flags & (FILEUTILS_DEREFERENCE + 
FILEUTILS_DEREFERENCE_L0))
+
+   if ((FLAGS_DEREF ? stat : lstat)(source, _stat) < 0) {
+   /* This may be a dangling symlink.
+* Making [sym]links to dangling symlinks works, so... */
+   if (flags & (FILEUTILS_MAKE_SOFTLINK|FILEUTILS_MAKE_HARDLINK))
+   goto make_links;
+   bb_perror_msg("can't stat '%s'", source);
+   return -1;
+   }
+
+   if (lstat(dest, _stat) < 0) {
+   if (errno != ENOENT) {
+   bb_perror_msg("can't stat '%s'", dest);
+   return -1;
+   }
+   } else {
+   if (source_stat.st_dev == dest_stat.st_dev
+&& source_stat.st_ino == dest_stat.st_ino
+   ) {
+   bb_error_msg("'%s' and '%s' are the same file", source, 
dest);
+   return -1;
+   }
+   dest_exists = 1;
+   }
+
+#if ENABLE_SELINUX
+   if ((flags & FILEUTILS_PRESERVE_SECURITY_CONTEXT) && 
is_selinux_enabled() > 0) {
+   security_context_t con;
+   if (lgetfilecon(source, ) >= 0) {
+   if (setfscreatecon(con) < 0) {
+   bb_perror_msg("can't set setfscreatecon %s", 
con);
+   freecon(con);
+   return -1;
+   }
+   } else if (errno == ENOTSUP || errno == ENODATA) {
+   setfscreatecon_or_die(NULL);
+   } else {
+   bb_perror_msg("can't lgetfilecon %s", source);
+   return -1;
+   }
+   }
+#endif
+
+   if (S_ISDIR(source_stat.st_mode)) {
+   DIR *dp;
+   const char *tp;
+   struct dirent *d;
+   mode_t saved_umask = 0;
+
+   if (!(flags & FILEUTILS_RECUR)) {
+   bb_error_msg("omitting directory '%s'", source);
+   return -1;
+   }
+
+   /* Did we ever create source ourself before? */
+   tp = is_in_ino_dev_hashtable(_stat);
+   if (tp) {
+   /* We did! it's a recursion! man the lifeboats... */
+   bb_error_msg("recursion detected, omitting directory 
'%s'",
+   source);
+   return -1;
+   }
+
+   if (dest_exists) {
+   if (!S_ISDIR(dest_stat.st_mode)) {
+   bb_error_msg("target '%s' is not a directory", 
dest);
+   return -1;
+   }
+   /* race here: user can substitute a symlink between
+* this check and actual creation of files inside dest 
*/
+   } else {
+   /* Create DEST */
+   mode_t mode;
+   saved_umask = umask(0);
+
+   mode = source_stat.st_mode;
+   if (!(flags & FILEUTILS_PRESERVE_STATUS))
+   mode = source_stat.st_mode & ~saved_umask;
+   /* Allow owner to access new dir (at least for now) */
+   mode |= S_IRWXU;
+   if (mkdir(dest, mode) < 0) {
+   umask(saved_umask);
+   bb_perror_msg("can't create directory '%s'", 
dest);
+   return -1;
+   }
+   umask(saved_umask);
+   /* need stat info for add_to_ino_dev_hashtable */
+  

[PATCH 00/25] worktree lock, move, remove and unlock

[Nguyễn Thái Ngọc Duy]

This is basically a resend from last time, which happened during rc
time. It adds 4 more commands, basically cleaning up the "TODO" list
in git-worktree.txt.

So far I've only actually used move and remove (and maybe unlock once
because worktree-add failed on me and I had to unlock it manually).
And I don't get to move worktrees a lot either so not really extensive
testing.

  [01/25] usage.c: move format processing out of die_errno()
  [02/25] usage.c: add sys_error() that prints strerror() automatically
  [03/25] copy.c: import copy_file() from busybox
  [04/25] copy.c: delete unused code in copy_file()
  [05/25] copy.c: convert bb_(p)error_msg to (sys_)error
  [06/25] copy.c: style fix
  [07/25] copy.c: convert copy_file() to copy_dir_recursively()
  [08/25] completion: support git-worktree
  [09/25] git-worktree.txt: keep subcommand listing in alphabetical order
  [10/25] path.c: add git_common_path() and strbuf_git_common_path()
  [11/25] worktree.c: use is_dot_or_dotdot()
  [12/25] worktree.c: store "id" instead of "git_dir"
  [13/25] worktree.c: add clear_worktree()
  [14/25] worktree.c: add find_worktree_by_path()
  [15/25] worktree.c: add is_main_worktree()
  [16/25] worktree.c: add validate_worktree()
  [17/25] worktree.c: add update_worktree_location()
  [18/25] worktree.c: add is_worktree_locked()
  [19/25] worktree: avoid 0{40}, too many zeroes, hard to read
  [20/25] worktree: simplify prefixing paths
  [21/25] worktree: add "lock" command
  [22/25] worktree: add "unlock" command
  [23/25] worktree: add "move" commmand
  [24/25] worktree move: accept destination as directory
  [25/25] worktree: add "remove" command

Total 11 files changed, 1028 insertions(+), 48 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 01/25] usage.c: move format processing out of die_errno()

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 usage.c | 25 +++--
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/usage.c b/usage.c
index 82ff131..0dba0c5 100644
--- a/usage.c
+++ b/usage.c
@@ -109,19 +109,12 @@ void NORETURN die(const char *err, ...)
va_end(params);
 }
 
-void NORETURN die_errno(const char *fmt, ...)
+static const char *fmt_with_err(const char *fmt)
 {
-   va_list params;
-   char fmt_with_err[1024];
+   static char fmt_with_err[1024];
char str_error[256], *err;
int i, j;
 
-   if (die_is_recursing()) {
-   fputs("fatal: recursion detected in die_errno handler\n",
-   stderr);
-   exit(128);
-   }
-
err = strerror(errno);
for (i = j = 0; err[i] && j < sizeof(str_error) - 1; ) {
if ((str_error[j++] = err[i++]) != '%')
@@ -137,9 +130,21 @@ void NORETURN die_errno(const char *fmt, ...)
}
str_error[j] = 0;
snprintf(fmt_with_err, sizeof(fmt_with_err), "%s: %s", fmt, str_error);
+   return fmt_with_err;
+}
+
+void NORETURN die_errno(const char *fmt, ...)
+{
+   va_list params;
+
+   if (die_is_recursing()) {
+   fputs("fatal: recursion detected in die_errno handler\n",
+   stderr);
+   exit(128);
+   }
 
va_start(params, fmt);
-   die_routine(fmt_with_err, params);
+   die_routine(fmt_with_err(fmt), params);
va_end(params);
 }
 
-- 
2.8.0.rc0.210.gd302cd2

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 24/26] upload-pack: split check_unreachable() in two, prep for get_reachable_list()

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 upload-pack.c | 41 ++---
 1 file changed, 26 insertions(+), 15 deletions(-)

diff --git a/upload-pack.c b/upload-pack.c
index 95a0bfb..9e4a4fa 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -452,24 +452,24 @@ static int is_our_ref(struct object *o)
return o->flags & ((allow_hidden_ref ? HIDDEN_REF : 0) | OUR_REF);
 }
 
-static int check_unreachable(struct object_array *src)
+static int do_reachable_revlist(struct child_process *cmd,
+   struct object_array *src)
 {
static const char *argv[] = {
"rev-list", "--stdin", NULL,
};
-   static struct child_process cmd = CHILD_PROCESS_INIT;
struct object *o;
char namebuf[42]; /* ^ + SHA-1 + LF */
int i;
 
-   cmd.argv = argv;
-   cmd.git_cmd = 1;
-   cmd.no_stderr = 1;
-   cmd.in = -1;
-   cmd.out = -1;
+   cmd->argv = argv;
+   cmd->git_cmd = 1;
+   cmd->no_stderr = 1;
+   cmd->in = -1;
+   cmd->out = -1;
 
-   if (start_command())
-   return 0;
+   if (start_command(cmd))
+   return -1;
 
/*
 * If rev-list --stdin encounters an unknown commit, it
@@ -487,8 +487,8 @@ static int check_unreachable(struct object_array *src)
if (!is_our_ref(o))
continue;
memcpy(namebuf + 1, oid_to_hex(>oid), GIT_SHA1_HEXSZ);
-   if (write_in_full(cmd.in, namebuf, 42) < 0)
-   return 0;
+   if (write_in_full(cmd->in, namebuf, 42) < 0)
+   return -1;
}
namebuf[40] = '\n';
for (i = 0; i < src->nr; i++) {
@@ -496,18 +496,29 @@ static int check_unreachable(struct object_array *src)
if (is_our_ref(o))
continue;
memcpy(namebuf, oid_to_hex(>oid), GIT_SHA1_HEXSZ);
-   if (write_in_full(cmd.in, namebuf, 41) < 0)
-   return 0;
+   if (write_in_full(cmd->in, namebuf, 41) < 0)
+   return -1;
}
-   close(cmd.in);
+   close(cmd->in);
 
sigchain_pop(SIGPIPE);
+   return 0;
+}
+
+static int check_unreachable(struct object_array *src)
+{
+   struct child_process cmd = CHILD_PROCESS_INIT;
+   int i, ret = do_reachable_revlist(, src);
+   char buf[1];
+
+   if (ret < 0)
+   return 0;
 
/*
 * The commits out of the rev-list are not ancestors of
 * our ref.
 */
-   i = read_in_full(cmd.out, namebuf, 1);
+   i = read_in_full(cmd.out, buf, 1);
if (i)
return 0;
close(cmd.out);
-- 
2.8.0.rc0.210.gd302cd2

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 17/26] clone: define shallow clone boundary based on time with --shallow-since

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 Documentation/git-clone.txt |  3 +++
 builtin/clone.c | 16 +---
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index b7c467a..a410409 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -193,6 +193,9 @@ objects from the source repository into a pack in the 
cloned repository.
`--no-single-branch` is given to fetch the histories near the
tips of all branches.
 
+--shallow-since=::
+   Create a shallow clone with a history after the specified time.
+
 --[no-]single-branch::
Clone only the history leading to the tip of a single branch,
either specified by the `--branch` option or the primary
diff --git a/builtin/clone.c b/builtin/clone.c
index bcba080..dc2ef4f 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -40,7 +40,8 @@ static const char * const builtin_clone_usage[] = {
 
 static int option_no_checkout, option_bare, option_mirror, 
option_single_branch = -1;
 static int option_local = -1, option_no_hardlinks, option_shared, 
option_recursive;
-static char *option_template, *option_depth;
+static int deepen;
+static char *option_template, *option_depth, *option_since;
 static char *option_origin = NULL;
 static char *option_branch = NULL;
 static const char *real_git_dir;
@@ -86,6 +87,8 @@ static struct option builtin_clone_options[] = {
   N_("path to git-upload-pack on the remote")),
OPT_STRING(0, "depth", _depth, N_("depth"),
N_("create a shallow clone of that depth")),
+   OPT_STRING(0, "shallow-since", _since, N_("time"),
+   N_("create a shallow clone since a specific time")),
OPT_BOOL(0, "single-branch", _single_branch,
N_("clone only one branch, HEAD or --branch")),
OPT_STRING(0, "separate-git-dir", _git_dir, N_("gitdir"),
@@ -849,8 +852,10 @@ int cmd_clone(int argc, const char **argv, const char 
*prefix)
usage_msg_opt(_("You must specify a repository to clone."),
builtin_clone_usage, builtin_clone_options);
 
+   if (option_depth || option_since)
+   deepen = 1;
if (option_single_branch == -1)
-   option_single_branch = option_depth ? 1 : 0;
+   option_single_branch = deepen ? 1 : 0;
 
if (option_mirror)
option_bare = 1;
@@ -976,6 +981,8 @@ int cmd_clone(int argc, const char **argv, const char 
*prefix)
if (is_local) {
if (option_depth)
warning(_("--depth is ignored in local clones; use 
file:// instead."));
+   if (option_since)
+   warning(_("--shallow-since is ignored in local clones; 
use file:// instead."));
if (!access(mkpath("%s/shallow", path), F_OK)) {
if (option_local > 0)
warning(_("source repository is shallow, 
ignoring --local"));
@@ -994,6 +1001,9 @@ int cmd_clone(int argc, const char **argv, const char 
*prefix)
if (option_depth)
transport_set_option(transport, TRANS_OPT_DEPTH,
 option_depth);
+   if (option_since)
+   transport_set_option(transport, TRANS_OPT_DEEPEN_SINCE,
+option_since);
if (option_single_branch)
transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, "1");
 
@@ -1001,7 +1011,7 @@ int cmd_clone(int argc, const char **argv, const char 
*prefix)
transport_set_option(transport, TRANS_OPT_UPLOADPACK,
 option_upload_pack);
 
-   if (transport->smart_options && !option_depth)
+   if (transport->smart_options && !deepen)
transport->smart_options->check_self_contained_and_connected = 
1;
 
refs = transport_get_remote_refs(transport);
-- 
2.8.0.rc0.210.gd302cd2

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 15/26] upload-pack: add deepen-since to cut shallow repos based on time

[Nguyễn Thái Ngọc Duy]

This should allow the user to say "create a shallow clone containing the
work from last year" (once the client side is fixed up, of course).

In theory deepen-since and deepen (aka --depth) can be used together to
draw the shallow boundary (whether it's intersection or union is up to
discussion, but if rev-list is used, it's likely intersection). However,
because deepen goes with a custom commit walker, we can't mix the two
yet.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 Documentation/technical/pack-protocol.txt |  3 +-
 Documentation/technical/protocol-capabilities.txt |  9 +
 upload-pack.c | 45 ++-
 3 files changed, 54 insertions(+), 3 deletions(-)

diff --git a/Documentation/technical/pack-protocol.txt 
b/Documentation/technical/pack-protocol.txt
index c6977bb..9251df1 100644
--- a/Documentation/technical/pack-protocol.txt
+++ b/Documentation/technical/pack-protocol.txt
@@ -219,7 +219,8 @@ out of what the server said it could do with the first 
'want' line.
 
   shallow-line  =  PKT-LINE("shallow" SP obj-id)
 
-  depth-request =  PKT-LINE("deepen" SP depth)
+  depth-request =  PKT-LINE("deepen" SP depth) /
+  PKT-LINE("deepen-since" SP timestamp)
 
   first-want=  PKT-LINE("want" SP obj-id SP capability-list)
   additional-want   =  PKT-LINE("want" SP obj-id)
diff --git a/Documentation/technical/protocol-capabilities.txt 
b/Documentation/technical/protocol-capabilities.txt
index eaab6b4..f08cc4e 100644
--- a/Documentation/technical/protocol-capabilities.txt
+++ b/Documentation/technical/protocol-capabilities.txt
@@ -179,6 +179,15 @@ This capability adds "deepen", "shallow" and "unshallow" 
commands to
 the  fetch-pack/upload-pack protocol so clients can request shallow
 clones.
 
+deepen-since
+
+
+This capability adds "deepen-since" command to fetch-pack/upload-pack
+protocol so the client can request shallow clones that are cut at a
+specific time, instead of depth. Internally it's equivalent of doing
+"rev-list --max-age=" on the server side. "deepen-since"
+cannot be used with "deepen".
+
 no-progress
 ---
 
diff --git a/upload-pack.c b/upload-pack.c
index c8dafe8..58c0936 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -14,6 +14,7 @@
 #include "sigchain.h"
 #include "version.h"
 #include "string-list.h"
+#include "argv-array.h"
 
 static const char upload_pack_usage[] = "git upload-pack [--strict] 
[--timeout=] ";
 
@@ -612,11 +613,25 @@ static void deepen(int depth, const struct object_array 
*shallows)
packet_flush(1);
 }
 
+static void deepen_by_rev_list(int ac, const char **av,
+  struct object_array *shallows)
+{
+   struct commit_list *result;
+
+   result = get_shallow_commits_by_rev_list(ac, av, SHALLOW, NOT_SHALLOW);
+   send_shallow(result);
+   free_commit_list(result);
+   send_unshallow(shallows);
+   packet_flush(1);
+}
+
 static void receive_needs(void)
 {
struct object_array shallows = OBJECT_ARRAY_INIT;
int depth = 0;
int has_non_tip = 0;
+   unsigned long deepen_since = 0;
+   int deepen_rev_list = 0;
 
shallow_nr = 0;
for (;;) {
@@ -653,6 +668,16 @@ static void receive_needs(void)
die("Invalid deepen: %s", line);
continue;
}
+   if (skip_prefix(line, "deepen-since ", )) {
+   char *end = NULL;
+   deepen_since = strtoul(arg, , 0);
+   if (!end || *end || !deepen_since ||
+   /* revisions.c's max_age -1 is special */
+   deepen_since == -1)
+   die("Invalid deepen-since: %s", line);
+   deepen_rev_list = 1;
+   continue;
+   }
if (!skip_prefix(line, "want ", ) ||
get_sha1_hex(arg, sha1_buf))
die("git upload-pack: protocol error, "
@@ -704,10 +729,26 @@ static void receive_needs(void)
if (!use_sideband && daemon_mode)
no_progress = 1;
 
-   if (depth == 0 && shallows.nr == 0)
+   if (depth == 0 && !deepen_rev_list && shallows.nr == 0)
return;
+   if (depth > 0 && deepen_rev_list)
+   die("git upload-pack: deepen and deepen-since cannot be used 
together");
if (depth > 0)
deepen(depth, );
+   else if (deepen_rev_list) {
+   struct argv_array av = ARGV_ARRAY_INIT;
+   int i;
+
+   argv_array_push(, "rev-list");
+   if (deepen_since)
+   argv_array_pushf(, "--max-age=%lu", deepen_since);
+   for (i = 0; i < want_obj.nr; i++) {
+   struct object *o = want_obj.objects[i].item;
+   argv_array_push(, 

[PATCH 22/26] clone: define shallow clone boundary with --shallow-exclude

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 Documentation/git-clone.txt |  5 +
 builtin/clone.c | 18 +-
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index a410409..5049663 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -196,6 +196,11 @@ objects from the source repository into a pack in the 
cloned repository.
 --shallow-since=::
Create a shallow clone with a history after the specified time.
 
+--shallow-exclude=::
+   Create a shallow clone with a history, excluding commits
+   reachable from a specified remote branch or tag.  This option
+   can be specified multiple times.
+
 --[no-]single-branch::
Clone only the history leading to the tip of a single branch,
either specified by the `--branch` option or the primary
diff --git a/builtin/clone.c b/builtin/clone.c
index dc2ef4f..5ccf6b7 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -44,6 +44,7 @@ static int deepen;
 static char *option_template, *option_depth, *option_since;
 static char *option_origin = NULL;
 static char *option_branch = NULL;
+static struct string_list option_not = STRING_LIST_INIT_NODUP;
 static const char *real_git_dir;
 static char *option_upload_pack = "git-upload-pack";
 static int option_verbosity;
@@ -52,6 +53,13 @@ static struct string_list option_config;
 static struct string_list option_reference;
 static int option_dissociate;
 
+static int option_parse_deepen_not(const struct option *opt,
+  const char *arg, int unset)
+{
+   string_list_append(_not, arg);
+   return 0;
+}
+
 static struct option builtin_clone_options[] = {
OPT__VERBOSITY(_verbosity),
OPT_BOOL(0, "progress", _progress,
@@ -89,6 +97,9 @@ static struct option builtin_clone_options[] = {
N_("create a shallow clone of that depth")),
OPT_STRING(0, "shallow-since", _since, N_("time"),
N_("create a shallow clone since a specific time")),
+   { OPTION_CALLBACK, 0, "shallow-exclude", NULL, N_("revision"),
+   N_("deepen history of shallow clone by excluding rev"),
+   PARSE_OPT_NONEG, option_parse_deepen_not },
OPT_BOOL(0, "single-branch", _single_branch,
N_("clone only one branch, HEAD or --branch")),
OPT_STRING(0, "separate-git-dir", _git_dir, N_("gitdir"),
@@ -852,7 +863,7 @@ int cmd_clone(int argc, const char **argv, const char 
*prefix)
usage_msg_opt(_("You must specify a repository to clone."),
builtin_clone_usage, builtin_clone_options);
 
-   if (option_depth || option_since)
+   if (option_depth || option_since || option_not.nr)
deepen = 1;
if (option_single_branch == -1)
option_single_branch = deepen ? 1 : 0;
@@ -983,6 +994,8 @@ int cmd_clone(int argc, const char **argv, const char 
*prefix)
warning(_("--depth is ignored in local clones; use 
file:// instead."));
if (option_since)
warning(_("--shallow-since is ignored in local clones; 
use file:// instead."));
+   if (option_not.nr)
+   warning(_("--shallow-exclude is ignored in local 
clones; use file:// instead."));
if (!access(mkpath("%s/shallow", path), F_OK)) {
if (option_local > 0)
warning(_("source repository is shallow, 
ignoring --local"));
@@ -1004,6 +1017,9 @@ int cmd_clone(int argc, const char **argv, const char 
*prefix)
if (option_since)
transport_set_option(transport, TRANS_OPT_DEEPEN_SINCE,
 option_since);
+   if (option_not.nr)
+   transport_set_option(transport, TRANS_OPT_DEEPEN_NOT,
+(const char *)_not);
if (option_single_branch)
transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, "1");
 
-- 
2.8.0.rc0.210.gd302cd2

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 16/26] fetch: define shallow boundary with --shallow-since

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 Documentation/fetch-options.txt |  4 
 Documentation/git-fetch-pack.txt|  4 
 Documentation/gitremote-helpers.txt |  3 +++
 builtin/fetch-pack.c|  4 
 builtin/fetch.c | 29 +++--
 fetch-pack.c| 12 +++-
 fetch-pack.h|  1 +
 remote-curl.c   | 11 +--
 transport.c |  4 
 transport.h |  4 
 10 files changed, 67 insertions(+), 9 deletions(-)

diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
index 952dfdf..8738d3d 100644
--- a/Documentation/fetch-options.txt
+++ b/Documentation/fetch-options.txt
@@ -14,6 +14,10 @@
linkgit:git-clone[1]), deepen or shorten the history to the specified
number of commits. Tags for the deepened commits are not fetched.
 
+--shallow-since=::
+   Deepen or shorten the history of a shallow repository to
+   include all reachable commits after .
+
 --unshallow::
If the source repository is complete, convert a shallow
repository to a complete one, removing all the limitations
diff --git a/Documentation/git-fetch-pack.txt b/Documentation/git-fetch-pack.txt
index 8680f45..99e6257 100644
--- a/Documentation/git-fetch-pack.txt
+++ b/Documentation/git-fetch-pack.txt
@@ -87,6 +87,10 @@ be in a separate packet, and the list must end with a flush 
packet.
'git-upload-pack' treats the special depth 2147483647 as
infinite even if there is an ancestor-chain that long.
 
+--shallow-since=::
+   Deepen or shorten the history of a shallow'repository to
+   include all reachable commits after .
+
 --no-progress::
Do not show the progress.
 
diff --git a/Documentation/gitremote-helpers.txt 
b/Documentation/gitremote-helpers.txt
index 78e0b27..9971d9a 100644
--- a/Documentation/gitremote-helpers.txt
+++ b/Documentation/gitremote-helpers.txt
@@ -415,6 +415,9 @@ set by Git if the remote helper has the 'option' capability.
 'option depth' ::
Deepens the history of a shallow repository.
 
+'option deepen-since ::
+   Deepens the history of a shallow repository based on time.
+
 'option followtags' {'true'|'false'}::
If enabled the helper should automatically fetch annotated
tag objects if the object the tag points at was transferred
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 8332d3d..0402e27 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -104,6 +104,10 @@ int cmd_fetch_pack(int argc, const char **argv, const char 
*prefix)
args.depth = strtol(arg, NULL, 0);
continue;
}
+   if (skip_prefix(arg, "--shallow-since=", )) {
+   args.deepen_since = xstrdup(arg);
+   continue;
+   }
if (!strcmp("--no-progress", arg)) {
args.no_progress = 1;
continue;
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 8e74213..283aa95 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -36,9 +36,10 @@ static int prune = -1; /* unspecified */
 
 static int all, append, dry_run, force, keep, multiple, update_head_ok, 
verbosity;
 static int progress = -1, recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
-static int tags = TAGS_DEFAULT, unshallow, update_shallow;
+static int tags = TAGS_DEFAULT, unshallow, update_shallow, deepen;
 static int max_children = 1;
 static const char *depth;
+static const char *deepen_since;
 static const char *upload_pack;
 static struct strbuf default_rla = STRBUF_INIT;
 static struct transport *gtransport;
@@ -115,6 +116,8 @@ static struct option builtin_fetch_options[] = {
OPT_BOOL(0, "progress", , N_("force progress reporting")),
OPT_STRING(0, "depth", , N_("depth"),
   N_("deepen history of shallow clone")),
+   OPT_STRING(0, "shallow-since", _since, N_("time"),
+  N_("deepen history of shallow repository based on time")),
{ OPTION_SET_INT, 0, "unshallow", , NULL,
   N_("convert to a complete repository"),
   PARSE_OPT_NONEG | PARSE_OPT_NOARG, NULL, 1 },
@@ -754,7 +757,7 @@ static int quickfetch(struct ref *ref_map)
 * really need to perform.  Claiming failure now will ensure
 * we perform the network exchange to deepen our history.
 */
-   if (depth)
+   if (deepen)
return -1;
return check_everything_connected(iterate_ref_map, 1, );
 }
@@ -859,7 +862,7 @@ static void set_option(struct transport *transport, const 
char *name, const char
name, transport->url);
 }
 
-static struct transport *prepare_transport(struct remote *remote)
+static struct transport *prepare_transport(struct remote *remote, int 

[PATCH 14/26] shallow.c: implement a generic shallow boundary finder based on rev-list

[Nguyễn Thái Ngọc Duy]

Instead of a custom commit walker like get_shallow_commits(), this new
function uses rev-list to mark NOT_SHALLOW to all reachable commits,
except borders. The definition of reachable is to be defined by the
protocol later. This makes it more flexible to define shallow boundary.

The way we find find border is paint all reachable commits NOT_SHALLOW.
Any of them that "touches" commits without NOT_SHALLOW flag are
considered shallow (e.g. zero parents via grafting mechanism). Shallow
commits and their true parents are all marked SHALLOW. Then NOT_SHALLOW
is removed from shallow commits at the end.

There is an interesting observation. With a generic walker, we can
produce all kinds of shallow cutting. In the following graph, every
commit but "x" is reachable. "b" is a parent of "a".

   x -- a -- o
  //
x -- c -- b -- o

After this function is run, "a" and "c" are both considered shallow
commits. After grafting occurs at the client side, what we see is

a -- o
/
 c -- b -- o

Notice that because of grafting, "a" has zero parents, so "b" is no
longer a parent of "a".

This is unfortunate and may be solved in two ways. The first is change
the way shallow grafting works and keep "a -- b" connection if "b"
exists and always ends at shallow commits (iow, no loose ends). This is
hard to detect, or at least not cheap to do.

The second way is mark one "x" as shallow commit instead of "a" and
produce this graph at client side:

   x -- a -- o
   //
 c -- b -- o

More commits, but simpler grafting rules.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 commit.h  |  2 ++
 shallow.c | 78 +++
 2 files changed, 80 insertions(+)

diff --git a/commit.h b/commit.h
index 5d58be0..b717be1 100644
--- a/commit.h
+++ b/commit.h
@@ -258,6 +258,8 @@ extern int for_each_commit_graft(each_commit_graft_fn, void 
*);
 extern int is_repository_shallow(void);
 extern struct commit_list *get_shallow_commits(struct object_array *heads,
int depth, int shallow_flag, int not_shallow_flag);
+extern struct commit_list *get_shallow_commits_by_rev_list(
+   int ac, const char **av, int shallow_flag, int 
not_shallow_flag);
 extern void set_alternate_shallow_file(const char *path, int override);
 extern int write_shallow_commits(struct strbuf *out, int use_pack_protocol,
 const struct sha1_array *extra);
diff --git a/shallow.c b/shallow.c
index 60f1505..40c2485 100644
--- a/shallow.c
+++ b/shallow.c
@@ -10,6 +10,8 @@
 #include "diff.h"
 #include "revision.h"
 #include "commit-slab.h"
+#include "revision.h"
+#include "list-objects.h"
 
 static int is_shallow = -1;
 static struct stat_validity shallow_stat;
@@ -137,6 +139,82 @@ struct commit_list *get_shallow_commits(struct 
object_array *heads, int depth,
return result;
 }
 
+static void show_commit(struct commit *commit, void *data)
+{
+   commit_list_insert(commit, data);
+}
+
+/*
+ * Given rev-list arguments, run rev-list. All reachable commits
+ * except border ones are marked with not_shallow_flag. Border commits
+ * are marked with shallow_flag. The list of border/shallow commits
+ * are also returned.
+ */
+struct commit_list *get_shallow_commits_by_rev_list(int ac, const char **av,
+   int shallow_flag,
+   int not_shallow_flag)
+{
+   struct commit_list *result = NULL, *p;
+   struct commit_list *not_shallow_list = NULL;
+   struct rev_info revs;
+   int both_flags = shallow_flag | not_shallow_flag;
+
+   /*
+* SHALLOW (excluded) and NOT_SHALLOW (included) should not be
+* set at this point. But better be safe than sorry.
+*/
+   clear_object_flags(both_flags);
+
+   is_repository_shallow(); /* make sure shallows are read */
+
+   init_revisions(, NULL);
+   save_commit_buffer = 0;
+   setup_revisions(ac, av, , NULL);
+
+   if (prepare_revision_walk())
+   die("revision walk setup failed");
+   traverse_commit_list(, show_commit, NULL, _shallow_list);
+
+   /* Mark all reachable commits as NOT_SHALLOW */
+   for (p = not_shallow_list; p; p = p->next)
+   p->item->object.flags |= not_shallow_flag;
+
+   /*
+* mark border commits SHALLOW + NOT_SHALLOW.
+* We cannot clear NOT_SHALLOW right now. Imagine border
+* commit A is processed first, then commit B, whose parent is
+* A, later. If NOT_SHALLOW on A is cleared at step 1, B
+* itself is considered border at step 2, which is incorrect.
+*/
+   for (p = not_shallow_list; p; p = p->next) {
+   struct commit *c = p->item;
+   struct commit_list *parent;
+
+   if (parse_commit(c))
+   

[PATCH 18/26] t5500, t5539: tests for shallow depth since a specific date

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 t/t5500-fetch-pack.sh | 24 
 t/t5539-fetch-http-shallow.sh | 26 ++
 2 files changed, 50 insertions(+)

diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index e5f83bf..26f050d 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -637,4 +637,28 @@ test_expect_success MINGW 'fetch-pack --diag-url c:repo' '
check_prot_path c:repo file c:repo
 '
 
+test_expect_success 'clone shallow since ...' '
+   test_create_repo shallow-since &&
+   (
+   cd shallow-since &&
+   GIT_COMMITTER_DATE="1 +0700" git commit --allow-empty -m one &&
+   GIT_COMMITTER_DATE="2 +0700" git commit --allow-empty -m two &&
+   GIT_COMMITTER_DATE="3 +0700" git commit --allow-empty -m three 
&&
+   git clone --shallow-since "3 +0700" "file://$(pwd)/." 
../shallow11 &&
+   git -C ../shallow11 log --pretty=tformat:%s HEAD >actual &&
+   echo three >expected &&
+   test_cmp expected actual
+   )
+'
+
+test_expect_success 'fetch shallow since ...' '
+   git -C shallow11 fetch --shallow-since "2 +0700" origin &&
+   git -C shallow11 log --pretty=tformat:%s origin/master >actual &&
+   cat >expected <<-\EOF &&
+   three
+   two
+   EOF
+   test_cmp expected actual
+'
+
 test_done
diff --git a/t/t5539-fetch-http-shallow.sh b/t/t5539-fetch-http-shallow.sh
index 37a4335..6d77ca7 100755
--- a/t/t5539-fetch-http-shallow.sh
+++ b/t/t5539-fetch-http-shallow.sh
@@ -73,5 +73,31 @@ test_expect_success 'no shallow lines after receiving ACK 
ready' '
)
 '
 
+test_expect_success 'clone shallow since ...' '
+   test_create_repo shallow-since &&
+   (
+   cd shallow-since &&
+   GIT_COMMITTER_DATE="1 +0700" git commit --allow-empty -m one &&
+   GIT_COMMITTER_DATE="2 +0700" git commit --allow-empty -m two &&
+   GIT_COMMITTER_DATE="3 +0700" git commit --allow-empty -m three 
&&
+   mv .git "$HTTPD_DOCUMENT_ROOT_PATH/shallow-since.git" &&
+   git clone --shallow-since "3 +0700" 
$HTTPD_URL/smart/shallow-since.git ../shallow11 &&
+   git -C ../shallow11 log --pretty=tformat:%s HEAD >actual &&
+   echo three >expected &&
+   test_cmp expected actual
+   )
+'
+
+test_expect_success 'fetch shallow since ...' '
+   git -C shallow11 fetch --shallow-since "2 +0700" origin &&
+   git -C shallow11 log --pretty=tformat:%s origin/master >actual &&
+   cat >expected <<-\EOF &&
+   three
+   two
+   EOF
+   test_cmp expected actual
+'
+
+
 stop_httpd
 test_done
-- 
2.8.0.rc0.210.gd302cd2

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 23/26] t5500, t5539: tests for shallow depth excluding a ref

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 t/t5500-fetch-pack.sh | 22 ++
 t/t5539-fetch-http-shallow.sh | 22 ++
 2 files changed, 44 insertions(+)

diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 26f050d..a3fe5ca 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -661,4 +661,26 @@ test_expect_success 'fetch shallow since ...' '
test_cmp expected actual
 '
 
+test_expect_success 'shallow clone exclude tag two' '
+   test_create_repo shallow-exclude &&
+   (
+   cd shallow-exclude &&
+   test_commit one &&
+   test_commit two &&
+   test_commit three &&
+   git clone --shallow-exclude two "file://$(pwd)/." ../shallow12 &&
+   git -C ../shallow12 log --pretty=tformat:%s HEAD >actual &&
+   echo three >expected &&
+   test_cmp expected actual
+   )
+'
+
+test_expect_success 'fetch exclude tag one' '
+   git -C shallow12 fetch --shallow-exclude one origin &&
+   git -C shallow12 log --pretty=tformat:%s origin/master >actual &&
+   echo three >expected &&
+   echo two  >>expected &&
+   test_cmp expected actual
+'
+
 test_done
diff --git a/t/t5539-fetch-http-shallow.sh b/t/t5539-fetch-http-shallow.sh
index 6d77ca7..f71573d 100755
--- a/t/t5539-fetch-http-shallow.sh
+++ b/t/t5539-fetch-http-shallow.sh
@@ -98,6 +98,28 @@ test_expect_success 'fetch shallow since ...' '
test_cmp expected actual
 '
 
+test_expect_success 'shallow clone exclude tag two' '
+   test_create_repo shallow-exclude &&
+   (
+   cd shallow-exclude &&
+   test_commit one &&
+   test_commit two &&
+   test_commit three &&
+   mv .git "$HTTPD_DOCUMENT_ROOT_PATH/shallow-exclude.git" &&
+   git clone --shallow-exclude two $HTTPD_URL/smart/shallow-exclude.git 
../shallow12 &&
+   git -C ../shallow12 log --pretty=tformat:%s HEAD >actual &&
+   echo three >expected &&
+   test_cmp expected actual
+   )
+'
+
+test_expect_success 'fetch exclude tag one' '
+   git -C shallow12 fetch --shallow-exclude one origin &&
+   git -C shallow12 log --pretty=tformat:%s origin/master >actual &&
+   echo three >expected &&
+   echo two  >>expected &&
+   test_cmp expected actual
+'
 
 stop_httpd
 test_done
-- 
2.8.0.rc0.210.gd302cd2

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 20/26] upload-pack: support define shallow boundary by excluding revisions

[Nguyễn Thái Ngọc Duy]

This should allow the user to say "create a shallow clone of this branch
after version ".

Short refs are accepted and expanded at the server side with expand_ref()
because we cannot expand (unknown) refs from the client side.

Like deepen-since, deepen-not cannot be used with deepen. But deepen-not
can be mixed with deepen-since. The result is exactly how you do the
command "git rev-list --since=... --not ref".

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 Documentation/technical/pack-protocol.txt |  3 ++-
 Documentation/technical/protocol-capabilities.txt |  9 +
 upload-pack.c | 23 +--
 3 files changed, 32 insertions(+), 3 deletions(-)

diff --git a/Documentation/technical/pack-protocol.txt 
b/Documentation/technical/pack-protocol.txt
index 9251df1..dee33a6 100644
--- a/Documentation/technical/pack-protocol.txt
+++ b/Documentation/technical/pack-protocol.txt
@@ -220,7 +220,8 @@ out of what the server said it could do with the first 
'want' line.
   shallow-line  =  PKT-LINE("shallow" SP obj-id)
 
   depth-request =  PKT-LINE("deepen" SP depth) /
-  PKT-LINE("deepen-since" SP timestamp)
+  PKT-LINE("deepen-since" SP timestamp) /
+  PKT-LINE("deepen-not" SP ref)
 
   first-want=  PKT-LINE("want" SP obj-id SP capability-list)
   additional-want   =  PKT-LINE("want" SP obj-id)
diff --git a/Documentation/technical/protocol-capabilities.txt 
b/Documentation/technical/protocol-capabilities.txt
index f08cc4e..0e6b57d 100644
--- a/Documentation/technical/protocol-capabilities.txt
+++ b/Documentation/technical/protocol-capabilities.txt
@@ -188,6 +188,15 @@ specific time, instead of depth. Internally it's 
equivalent of doing
 "rev-list --max-age=" on the server side. "deepen-since"
 cannot be used with "deepen".
 
+deepen-not
+--
+
+This capability adds "deepen-not" command to fetch-pack/upload-pack
+protocol so the client can request shallow clones that are cut at a
+specific revision, instead of depth. Internally it's equivalent of
+doing "rev-list --not " on the server side. "deepen-not"
+cannot be used with "deepen", but can be used with "deepen-since".
+
 no-progress
 ---
 
diff --git a/upload-pack.c b/upload-pack.c
index 58c0936..95a0bfb 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -628,6 +628,7 @@ static void deepen_by_rev_list(int ac, const char **av,
 static void receive_needs(void)
 {
struct object_array shallows = OBJECT_ARRAY_INIT;
+   struct string_list deepen_not = STRING_LIST_INIT_DUP;
int depth = 0;
int has_non_tip = 0;
unsigned long deepen_since = 0;
@@ -678,6 +679,16 @@ static void receive_needs(void)
deepen_rev_list = 1;
continue;
}
+   if (skip_prefix(line, "deepen-not ", )) {
+   char *ref = NULL;
+   unsigned char sha1[20];
+   if (expand_ref(arg, strlen(arg), sha1, ) != 1)
+   die("git upload-pack: ambiguous deepen-not: 
%s", line);
+   string_list_append(_not, ref);
+   free(ref);
+   deepen_rev_list = 1;
+   continue;
+   }
if (!skip_prefix(line, "want ", ) ||
get_sha1_hex(arg, sha1_buf))
die("git upload-pack: protocol error, "
@@ -732,7 +743,7 @@ static void receive_needs(void)
if (depth == 0 && !deepen_rev_list && shallows.nr == 0)
return;
if (depth > 0 && deepen_rev_list)
-   die("git upload-pack: deepen and deepen-since cannot be used 
together");
+   die("git upload-pack: deepen and deepen-since (or deepen-not) 
cannot be used together");
if (depth > 0)
deepen(depth, );
else if (deepen_rev_list) {
@@ -742,6 +753,14 @@ static void receive_needs(void)
argv_array_push(, "rev-list");
if (deepen_since)
argv_array_pushf(, "--max-age=%lu", deepen_since);
+   if (deepen_not.nr) {
+   argv_array_push(, "--not");
+   for (i = 0; i < deepen_not.nr; i++) {
+   struct string_list_item *s = deepen_not.items + 
i;
+   argv_array_push(, s->string);
+   }
+   argv_array_push(, "--not");
+   }
for (i = 0; i < want_obj.nr; i++) {
struct object *o = want_obj.objects[i].item;
argv_array_push(, oid_to_hex(>oid));
@@ -797,7 +816,7 @@ static int send_ref(const char *refname, const struct 
object_id *oid,
int flag, void *cb_data)
 {
static const char *capabilities = "multi_ack thin-pack 

[PATCH 25/26] upload-pack: add get_reachable_list()

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 object.h  |  2 +-
 upload-pack.c | 52 +---
 2 files changed, 50 insertions(+), 4 deletions(-)

diff --git a/object.h b/object.h
index f8b6442..614a006 100644
--- a/object.h
+++ b/object.h
@@ -31,7 +31,7 @@ struct object_array {
  * revision.h:  0-1026
  * fetch-pack.c:0---4
  * walker.c:0-2
- * upload-pack.c:   1119
+ * upload-pack.c:   4   1119
  * builtin/blame.c:   12-13
  * bisect.c:   16
  * bundle.c:   16
diff --git a/upload-pack.c b/upload-pack.c
index 9e4a4fa..9bd590c 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -453,7 +453,8 @@ static int is_our_ref(struct object *o)
 }
 
 static int do_reachable_revlist(struct child_process *cmd,
-   struct object_array *src)
+   struct object_array *src,
+   struct object_array *reachable)
 {
static const char *argv[] = {
"rev-list", "--stdin", NULL,
@@ -484,6 +485,8 @@ static int do_reachable_revlist(struct child_process *cmd,
o = get_indexed_object(--i);
if (!o)
continue;
+   if (reachable && o->type == OBJ_COMMIT)
+   o->flags &= ~TMP_MARK;
if (!is_our_ref(o))
continue;
memcpy(namebuf + 1, oid_to_hex(>oid), GIT_SHA1_HEXSZ);
@@ -493,8 +496,13 @@ static int do_reachable_revlist(struct child_process *cmd,
namebuf[40] = '\n';
for (i = 0; i < src->nr; i++) {
o = src->objects[i].item;
-   if (is_our_ref(o))
+   if (is_our_ref(o)) {
+   if (reachable)
+   add_object_array(o, NULL, reachable);
continue;
+   }
+   if (reachable && o->type == OBJ_COMMIT)
+   o->flags |= TMP_MARK;
memcpy(namebuf, oid_to_hex(>oid), GIT_SHA1_HEXSZ);
if (write_in_full(cmd->in, namebuf, 41) < 0)
return -1;
@@ -505,10 +513,48 @@ static int do_reachable_revlist(struct child_process *cmd,
return 0;
 }
 
+static int get_reachable_list(struct object_array *src,
+ struct object_array *reachable)
+{
+   struct child_process cmd = CHILD_PROCESS_INIT;
+   int i, ret = do_reachable_revlist(, src, reachable);
+   struct object *o;
+   char namebuf[42]; /* ^ + SHA-1 + LF */
+
+   if (ret < 0)
+   return -1;
+
+   while ((i = read_in_full(cmd.out, namebuf, 41)) == 41) {
+   struct object_id sha1;
+
+   if (namebuf[40] != '\n' || get_oid_hex(namebuf, ))
+   break;
+
+   o = lookup_object(sha1.hash);
+   if (o && o->type == OBJ_COMMIT) {
+   o->flags &= ~TMP_MARK;
+   }
+   }
+   for (i = get_max_object_index(); 0 < i; i--) {
+   o = get_indexed_object(i - 1);
+   if (o && o->type == OBJ_COMMIT &&
+   (o->flags & TMP_MARK)) {
+   add_object_array(o, NULL, reachable);
+   o->flags &= ~TMP_MARK;
+   }
+   }
+   close(cmd.out);
+
+   if (finish_command())
+   return -1;
+
+   return 0;
+}
+
 static int check_unreachable(struct object_array *src)
 {
struct child_process cmd = CHILD_PROCESS_INIT;
-   int i, ret = do_reachable_revlist(, src);
+   int i, ret = do_reachable_revlist(, src, NULL);
char buf[1];
 
if (ret < 0)
-- 
2.8.0.rc0.210.gd302cd2

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html