Bug report: git clone with dest

Bug report: git clone with dest

[Isaac Shabtay]

Hello,
Target directory is deleted on clone failures.

Steps to reproduce, for example on Windows:

cd /d %TEMP%
mkdir dest
git clone https://some-fake-url/whatever-makes-git-clone-fail dest

Of course, the clone will fail as it should. But looks like the Git
client also ends up deleting the "dest" directory.

This shouldn't happen.

Re: Bug report: git clone with dest

[Jeff King]

On Wed, Jan 03, 2018 at 12:59:48PM -0800, Isaac Shabtay wrote:

> Target directory is deleted on clone failures.
> 
> Steps to reproduce, for example on Windows:
> 
> cd /d %TEMP%
> mkdir dest
> git clone https://some-fake-url/whatever-makes-git-clone-fail dest
> 
> Of course, the clone will fail as it should. But looks like the Git
> client also ends up deleting the "dest" directory.

Interesting. AFAICT Git has behaved this way for almost 9 years, and now
we have two reports in two days. Serendipity, or did something else
change? :)

Anyway, you might be interested in the patch series I posted yesterday:

  https://public-inbox.org/git/20180102210753.GA10430@sigill.intra.peff.net/

-Peff

Re: Bug report: git clone with dest

[Isaac Shabtay]

Indeed interesting... this one's for the books...
Thanks for the patches. Any idea when these are going to make it to
the official Git client builds? (specifically the Windows one)

On 3 January 2018 at 14:28, Jeff King <peff@peff.net> wrote:
> On Wed, Jan 03, 2018 at 12:59:48PM -0800, Isaac Shabtay wrote:
>
>> Target directory is deleted on clone failures.
>>
>> Steps to reproduce, for example on Windows:
>>
>> cd /d %TEMP%
>> mkdir dest
>> git clone https://some-fake-url/whatever-makes-git-clone-fail dest
>>
>> Of course, the clone will fail as it should. But looks like the Git
>> client also ends up deleting the "dest" directory.
>
> Interesting. AFAICT Git has behaved this way for almost 9 years, and now
> we have two reports in two days. Serendipity, or did something else
> change? :)
>
> Anyway, you might be interested in the patch series I posted yesterday:
>
>   https://public-inbox.org/git/20180102210753.GA10430@sigill.intra.peff.net/
>
> -Peff

Re: Bug report: git clone with dest

[Johannes Schindelin]

Hi Isaac,

On Wed, 3 Jan 2018, Isaac Shabtay wrote:

> Indeed interesting... this one's for the books...
> Thanks for the patches. Any idea when these are going to make it to
> the official Git client builds? (specifically the Windows one)

If you help them getting reviewed, tested, and validated, I could be
talked into taking them into Git for Windows early so that they'll be in
Git for Windows v2.16.0. It will require your effort, though.

Ciao,
Johannes

Re: Bug report: git clone with dest

[Isaac Shabtay]

Hello Johannes, Jeff,

I cloned git's codebase, applied the four patches on master, built and
tested on Ubuntu Trusty. (After verifying that master indeed exhibits
this behaviour on Linux as well. Just checking).
Seems to work fine.
I also looked at the code. Most of the patched lines relate to tests,
and the one for clone.c seems reasonable to me. Added tests seem to
have very good coverage too.

I qualify everything I had written above by saying that it's my first
time ever looking at Git's source code.

On 4 January 2018 at 15:20, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi Isaac,
>
> On Wed, 3 Jan 2018, Isaac Shabtay wrote:
>
>> Indeed interesting... this one's for the books...
>> Thanks for the patches. Any idea when these are going to make it to
>> the official Git client builds? (specifically the Windows one)
>
> If you help them getting reviewed, tested, and validated, I could be
> talked into taking them into Git for Windows early so that they'll be in
> Git for Windows v2.16.0. It will require your effort, though.
>
> Ciao,
> Johannes

Re: Bug report: git clone with dest

[Johannes Schindelin]

Hi Isaac,

On Thu, 4 Jan 2018, Isaac Shabtay wrote:

> I cloned git's codebase, applied the four patches on master, built and
> tested on Ubuntu Trusty. (After verifying that master indeed exhibits
> this behaviour on Linux as well. Just checking).
> Seems to work fine.
> I also looked at the code. Most of the patched lines relate to tests,
> and the one for clone.c seems reasonable to me. Added tests seem to
> have very good coverage too.

Thanks.

> I qualify everything I had written above by saying that it's my first
> time ever looking at Git's source code.

We all started at some point.

Now, if you want to make this easier for me, could you please apply those
patches on top of Git for Windows' master branch and open a Pull Request
at https://github.com/git-for-windows/git?

Thank you,
Johannes

Re: Bug report: git clone with dest

[Isaac Shabtay]

Done: https://github.com/git-for-windows/git/pull/1421

I added credit to Jeff in the PR's description.

Note that I tried compiling master, but failed due to a reason
unrelated to this patch:

builtin/checkout.c:24:10: fatal error: fscache.h: No such file or directory

Maybe I wasn't building it right.



On 5 January 2018 at 02:33, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi Isaac,
>
> On Thu, 4 Jan 2018, Isaac Shabtay wrote:
>
>> I cloned git's codebase, applied the four patches on master, built and
>> tested on Ubuntu Trusty. (After verifying that master indeed exhibits
>> this behaviour on Linux as well. Just checking).
>> Seems to work fine.
>> I also looked at the code. Most of the patched lines relate to tests,
>> and the one for clone.c seems reasonable to me. Added tests seem to
>> have very good coverage too.
>
> Thanks.
>
>> I qualify everything I had written above by saying that it's my first
>> time ever looking at Git's source code.
>
> We all started at some point.
>
> Now, if you want to make this easier for me, could you please apply those
> patches on top of Git for Windows' master branch and open a Pull Request
> at https://github.com/git-for-windows/git?
>
> Thank you,
> Johannes

Re: Bug report: git clone with dest

[Johannes Schindelin]

Hi Isaac,

On Fri, 5 Jan 2018, Isaac Shabtay wrote:

> Done: https://github.com/git-for-windows/git/pull/1421
> 
> I added credit to Jeff in the PR's description.

Sadly, the PR's description won't make it into the commit history, and the
authorship really should have been retained.

I found Peff's topic branch in his fork and force-pushed, to demonstrate
what I wanted to have. Currently the test suite is running (I test 64-bit
builds of the three major platforms Windows, macOS and Linux), and once
that is done and passed, I will merge the Pull Request.

> Note that I tried compiling master, but failed due to a reason
> unrelated to this patch:
> 
> builtin/checkout.c:24:10: fatal error: fscache.h: No such file or directory

That was an oversight in a previously-merged Pull Request. I have fixed
that locally and will soon push it out onto `master`.

Ciao,
Johannes

Re: Bug report: git clone with dest

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Hi Isaac,
>
> On Fri, 5 Jan 2018, Isaac Shabtay wrote:
>
>> Done: https://github.com/git-for-windows/git/pull/1421
>> 
>> I added credit to Jeff in the PR's description.
>
> Sadly, the PR's description won't make it into the commit history, and the
> authorship really should have been retained.
>
> I found Peff's topic branch in his fork and force-pushed, to demonstrate
> what I wanted to have. Currently the test suite is running (I test 64-bit
> builds of the three major platforms Windows, macOS and Linux), and once
> that is done and passed, I will merge the Pull Request.
>
>> Note that I tried compiling master, but failed due to a reason
>> unrelated to this patch:
>> 
>> builtin/checkout.c:24:10: fatal error: fscache.h: No such file or directory
>
> That was an oversight in a previously-merged Pull Request. I have fixed
> that locally and will soon push it out onto `master`.
>
> Ciao,
> Johannes

FWIW, I do not mind including this as part of -rc2 if not -rc1 for
the upcoming release.

Re: Bug report: git clone with dest

[Jeff King]

On Fri, Jan 05, 2018 at 09:22:07PM +0100, Johannes Schindelin wrote:

> On Fri, 5 Jan 2018, Isaac Shabtay wrote:
> 
> > Done: https://github.com/git-for-windows/git/pull/1421
> > 
> > I added credit to Jeff in the PR's description.
> 
> Sadly, the PR's description won't make it into the commit history, and the
> authorship really should have been retained.
> 
> I found Peff's topic branch in his fork and force-pushed, to demonstrate
> what I wanted to have. Currently the test suite is running (I test 64-bit
> builds of the three major platforms Windows, macOS and Linux), and once
> that is done and passed, I will merge the Pull Request.

I think the discussion has ended at "don't do anything else", but note
that Junio and I were musing on whether to update the series around the
dir_exists() function. Which would then create headaches for you later
when you try to merge a subtly-different series that makes it upstream.

Like I said, I think we've resolved not to do anything, but I wanted to
point out a potential pitfall with this kind of "pick up a topic early"
strategy (I'm intimately familiar with this pitfall because I do it all
the time for the fork we run on our servers at GitHub).

-Peff

Re: Bug report: git clone with dest

[Johannes Schindelin]

Hi Peff,

On Fri, 5 Jan 2018, Jeff King wrote:

> On Fri, Jan 05, 2018 at 09:22:07PM +0100, Johannes Schindelin wrote:
> 
> > On Fri, 5 Jan 2018, Isaac Shabtay wrote:
> > 
> > > Done: https://github.com/git-for-windows/git/pull/1421
> > > 
> > > I added credit to Jeff in the PR's description.
> > 
> > Sadly, the PR's description won't make it into the commit history, and the
> > authorship really should have been retained.
> > 
> > I found Peff's topic branch in his fork and force-pushed, to demonstrate
> > what I wanted to have. Currently the test suite is running (I test 64-bit
> > builds of the three major platforms Windows, macOS and Linux), and once
> > that is done and passed, I will merge the Pull Request.
> 
> I think the discussion has ended at "don't do anything else", but note
> that Junio and I were musing on whether to update the series around the
> dir_exists() function.

I briefly looked over this discussion and got the same impression.

> Which would then create headaches for you later when you try to merge a
> subtly-different series that makes it upstream.

Subtly-different is not a big problem. It is typically solved by `git
rebase --skip` ;-)

> Like I said, I think we've resolved not to do anything, but I wanted to
> point out a potential pitfall with this kind of "pick up a topic early"
> strategy (I'm intimately familiar with this pitfall because I do it all
> the time for the fork we run on our servers at GitHub).

Thanks for your concern.

And not to worry, I have plenty of expertise, won over the years, in
dealing with subtly different variants of patches having been accepted
upstream and conflicting with patches that were carried in Git for
Windows.

Ciao,
Dscho

Re: Bug report: git clone with dest

[Jeff King]

On Wed, Jan 03, 2018 at 02:42:51PM -0800, Isaac Shabtay wrote:

> Indeed interesting... this one's for the books...
> Thanks for the patches. Any idea when these are going to make it to the
> official Git client builds? (specifically the Windows one)

They haven't even been reviewed yet. If they get good feedback, then the
maintainer will pick them up, then merge them to 'next', and then
eventually to 'master', after which they'd become part of the next
major release. For a pure bug-fix, it may instead go to 'maint' and
become part of the next minor release.

Right now we're entering release freeze for v2.16.0. We'd still take
fixes for recent breakages there, but given the age of the problem I
doubt it will make the cutoff. But as this is a bug-fix, it might make
it into v2.16.1.

-Peff

Re: Bug report: git clone with dest

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Jan 03, 2018 at 02:42:51PM -0800, Isaac Shabtay wrote:
>
>> Indeed interesting... this one's for the books...
>> Thanks for the patches. Any idea when these are going to make it to the
>> official Git client builds? (specifically the Windows one)
>
> They haven't even been reviewed yet. If they get good feedback, then the
> maintainer will pick them up, then merge them to 'next', and then
> eventually to 'master', after which they'd become part of the next
> major release. For a pure bug-fix, it may instead go to 'maint' and
> become part of the next minor release.

Even a pure bug-fix, unless it is something no longer needed on the
'master' front, goes thru 'pu'->'next'->'master' avenue first, and
is recorded in the RelNotes with the notes like "(merge d45420c1c8
jk/abort-clone-with-existing-dest later to maint)" when it happens.

	side note: in fact "grep -e 'later to maint' RelNotes" is
	how I remind myself what to merge down to 'maint'; the
	actual procedure is a bit more involved (those interested in
	the details can find the 'ML' script on the 'todo' branch;
	its name stands for 'merge later')

Later, after not hearing from people that the "fix" breaks things,
the topic is also mreged to 'maint' and becomes part of the next
minor release.

> Right now we're entering release freeze for v2.16.0. We'd still take
> fixes for recent breakages there, but given the age of the problem I
> doubt it will make the cutoff. But as this is a bug-fix, it might make
> it into v2.16.1.

Yup.  From the usual timeline, I'd expect this to be part of
'master' working towards 2.17 and to become a part of 2.16.x
series.

Thanks.

Re: Bug report: git clone with dest

[Jeff King]

On Fri, Jan 05, 2018 at 11:53:50AM -0800, Junio C Hamano wrote:

> > They haven't even been reviewed yet. If they get good feedback, then the
> > maintainer will pick them up, then merge them to 'next', and then
> > eventually to 'master', after which they'd become part of the next
> > major release. For a pure bug-fix, it may instead go to 'maint' and
> > become part of the next minor release.
> 
> Even a pure bug-fix, unless it is something no longer needed on the
> 'master' front, goes thru 'pu'->'next'->'master' avenue first, and
> is recorded in the RelNotes with the notes like "(merge d45420c1c8
> jk/abort-clone-with-existing-dest later to maint)" when it happens.
> 
> 	side note: in fact "grep -e 'later to maint' RelNotes" is
> 	how I remind myself what to merge down to 'maint'; the
> 	actual procedure is a bit more involved (those interested in
> 	the details can find the 'ML' script on the 'todo' branch;
> 	its name stands for 'merge later')
> 
> Later, after not hearing from people that the "fix" breaks things,
> the topic is also mreged to 'maint' and becomes part of the next
> minor release.

Out of curiosity, did this change at some point? I thought the process
used to be to merge to maint, and then pick up topics in master by
merging maint to master.

-Peff

Re: Bug report: git clone with dest

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Out of curiosity, did this change at some point? I thought the process
> used to be to merge to maint, and then pick up topics in master by
> merging maint to master.

If you look at "Sync with maint" merges made to 'master', you'd
notice that most of them are only updating Documentation/RelNotes/*
and otherwise no-effect merges, simply because when such an up-merge
is made, everything in 'maint' is already in 'master' because topics
are merged to the latter first.  Security fixes that go through
embargoes are excempt for obvious reasons ;-)

I do not recall how it was before 2012.

Re: Bug report: git clone with dest

[Jeff King]

On Fri, Jan 05, 2018 at 12:45:03PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > Out of curiosity, did this change at some point? I thought the process
> > used to be to merge to maint, and then pick up topics in master by
> > merging maint to master.
> 
> If you look at "Sync with maint" merges made to 'master', you'd
> notice that most of them are only updating Documentation/RelNotes/*
> and otherwise no-effect merges, simply because when such an up-merge
> is made, everything in 'maint' is already in 'master' because topics
> are merged to the latter first.  Security fixes that go through
> embargoes are excempt for obvious reasons ;-)

OK, that makes sense. Pretty sure I did it wrong when I was interim
maintainer back in the day, then. :)

-Peff

Re: Bug report: git clone with dest

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Jan 05, 2018 at 12:45:03PM -0800, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> > Out of curiosity, did this change at some point? I thought the process
>> > used to be to merge to maint, and then pick up topics in master by
>> > merging maint to master.
>> 
>> If you look at "Sync with maint" merges made to 'master', you'd
>> notice that most of them are only updating Documentation/RelNotes/*
>> and otherwise no-effect merges, simply because when such an up-merge
>> is made, everything in 'maint' is already in 'master' because topics
>> are merged to the latter first.  Security fixes that go through
>> embargoes are excempt for obvious reasons ;-)
>
> OK, that makes sense. Pretty sure I did it wrong when I was interim
> maintainer back in the day, then. :)

Well, MaintNotes on the 'todo' branch needs a bit of updating, as it
says something somewhat misleading.

-- >8 --
Subject: MaintNotes: clarify the purpose of maint->master upmerge

Even though the paragraph before this one is pretty clear that
topics are first merged to 'master' and then to 'maint', it was
misleading to say 'maint' is merged to 'master' to propagate fixes
forward, as most of the time, such an upmerge is a noop because
topics merged to 'maint' are usually merged to 'master' already.

These up-merges are done primarily to make sure that the tip of
'master' has updated release notes from all the maintenance tracks,
so be explicit about that to avoid confusion.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 MaintNotes | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/MaintNotes b/MaintNotes
index 3a70b88..393d81f 100644
--- a/MaintNotes
+++ b/MaintNotes
@@ -173,8 +173,8 @@ feature release).  These days, maintenance releases are named by
 incrementing the last digit of three-dotted decimal name (e.g. "2.12.1"
 was the first maintenance release for the "2.12" series).
 
-New features never go to the 'maint' branch.  This branch is also
-merged into "master" to propagate the fixes forward as needed.
+New features never go to the 'maint' branch.  It is merged into "master"
+primarily to propagate the description in the release notes forward.
 
 A new development does not usually happen on "master". When you send a
 series of patches, after review on the mailing list, a separate topic

Re: Bug report: git clone with dest

[Jeff King]

On Fri, Jan 05, 2018 at 02:37:25PM -0800, Junio C Hamano wrote:

> Well, MaintNotes on the 'todo' branch needs a bit of updating, as it
> says something somewhat misleading.
> 
> -- >8 --
> Subject: MaintNotes: clarify the purpose of maint->master upmerge

Yup, this makes sense. Thanks for clarifying.

-Peff

Re: Bug report: git clone with dest

[Johannes Schindelin]

Hi,


On Fri, 5 Jan 2018, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > On Wed, Jan 03, 2018 at 02:42:51PM -0800, Isaac Shabtay wrote:
> >
> >> Indeed interesting... this one's for the books...
> >> Thanks for the patches. Any idea when these are going to make it to the
> >> official Git client builds? (specifically the Windows one)
> >
> > They haven't even been reviewed yet.

FWIW I went through them in that PR that Isaac opened (and which I updated
with Peff's patches, rebased), and I found nothing obviously wrong with
them. They seem to be straight-forward enough.

Just s/m(y temporarily removing its objects)/b\1/ and s/(call stat
directly)( the same scratch)/\1 using\2/

Ciao,
Dscho