public inbox for cygwin-apps@cygwin.com
 help / color / mirror / Atom feed
* [PATCH cygport] git.cygclass: Suppress the depth option
@ 2023-11-19  2:11 Daisuke Fujimura
  2023-11-20 14:23 ` Jon Turney
  0 siblings, 1 reply; 11+ messages in thread
From: Daisuke Fujimura @ 2023-11-19  2:11 UTC (permalink / raw)
  To: cygwin-apps

Some git providers do not support smart transport, so specifying the
depth option will result in an error.

```
Cloning into 'xxxx'...
fatal: dumb http transport does not support shallow capabilities
```

Therefore, I suggest adding a variable to suppress the depth option.
(Variable names should be changed to something appropriate according
to the naming convention.)

diff --git a/cygclass/git.cygclass b/cygclass/git.cygclass
index e53a7985..0aa97a09 100644
--- a/cygclass/git.cygclass
+++ b/cygclass/git.cygclass
@@ -75,7 +75,12 @@ git_fetch() {
# shallow fetch a ref (master, branch or tag) with --depth=1
# (not allowed for a hash, unless remote is configured to permit
# it with allow*SHA1InWant).
- _depth="--depth 1"
+ _depth=""
+ # git provider does not support smart transport
+ if ! defined GIT_PROVIDER_NOT_SUPPORT_SMART_TRANSPORT
+ then
+ _depth="--depth 1"
+ fi
if defined GIT_TAG
then
_depth+=" --branch ${GIT_TAG}"

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH cygport] git.cygclass: Suppress the depth option
  2023-11-19  2:11 [PATCH cygport] git.cygclass: Suppress the depth option Daisuke Fujimura
@ 2023-11-20 14:23 ` Jon Turney
  2023-11-20 16:42   ` ASSI
  2023-11-30 12:17   ` Daisuke Fujimura
  0 siblings, 2 replies; 11+ messages in thread
From: Jon Turney @ 2023-11-20 14:23 UTC (permalink / raw)
  To: Daisuke Fujimura, cygwin-apps

On 19/11/2023 02:11, Daisuke Fujimura via Cygwin-apps wrote:
> Some git providers do not support smart transport, so specifying the
> depth option will result in an error.

Right. This is a bug and needs fixing.

Thanks for the patch.

> ```
> Cloning into 'xxxx'...
> fatal: dumb http transport does not support shallow capabilities
> ```
> 
> Therefore, I suggest adding a variable to suppress the depth option.
> (Variable names should be changed to something appropriate according
> to the naming convention.)
> 
> diff --git a/cygclass/git.cygclass b/cygclass/git.cygclass
> index e53a7985..0aa97a09 100644
> --- a/cygclass/git.cygclass
> +++ b/cygclass/git.cygclass
> @@ -75,7 +75,12 @@ git_fetch() {
> # shallow fetch a ref (master, branch or tag) with --depth=1
> # (not allowed for a hash, unless remote is configured to permit
> # it with allow*SHA1InWant).
> - _depth="--depth 1"
> + _depth=""
> + # git provider does not support smart transport
> + if ! defined GIT_PROVIDER_NOT_SUPPORT_SMART_TRANSPORT

If you're going to add a variable which changes the behaviour of cygport 
like this, it should be documented (by adding an appropriate robodoc 
comment)

This could just be named something a little shorter, like 
"GIT_URI_NO_SMART_TRANSPORT", since it's really a property of the URI's 
host?

But I wonder if wouldn't just be better to try with --depth and then 
fallback to without it, if that fails (especially if we can tell it 
failed for that reason).

(Looking at [1], that seems a better approach than trying to probe the 
URI for smart transport support, which seems problematic)

[1] 
https://stackoverflow.com/questions/9270488/is-it-possible-to-detect-whether-a-http-git-remote-is-smart-or-dumb

What do you think?

> + then
> + _depth="--depth 1"
> + fi
> if defined GIT_TAG
> then
> _depth+=" --branch ${GIT_TAG}"
> 


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH cygport] git.cygclass: Suppress the depth option
  2023-11-20 14:23 ` Jon Turney
@ 2023-11-20 16:42   ` ASSI
  2023-11-30 12:17   ` Daisuke Fujimura
  1 sibling, 0 replies; 11+ messages in thread
From: ASSI @ 2023-11-20 16:42 UTC (permalink / raw)
  To: cygwin-apps

Jon Turney via Cygwin-apps writes:
> This could just be named something a little shorter, like
> "GIT_URI_NO_SMART_TRANSPORT", since it's really a property of the
> URI's host?
>
> But I wonder if wouldn't just be better to try with --depth and then
> fallback to without it, if that fails (especially if we can tell it
> failed for that reason).

I was about to say the same thing…


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

Wavetables for the Waldorf Blofeld:
http://Synth.Stromeko.net/Downloads.html#BlofeldUserWavetables

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH cygport] git.cygclass: Suppress the depth option
  2023-11-20 14:23 ` Jon Turney
  2023-11-20 16:42   ` ASSI
@ 2023-11-30 12:17   ` Daisuke Fujimura
  2023-12-03 20:34     ` Jon Turney
  1 sibling, 1 reply; 11+ messages in thread
From: Daisuke Fujimura @ 2023-11-30 12:17 UTC (permalink / raw)
  To: cygwin-apps

Implementations that conditionally branch on variables are simple.

The proposed retry implementation complicates git.cygclass, but I
think it reduces the maintainer's effort.

I have created a patch for a retry implementation.
Could you review it?

diff --git a/cygclass/git.cygclass b/cygclass/git.cygclass
index e53a7985..1e26ab37 100644
--- a/cygclass/git.cygclass
+++ b/cygclass/git.cygclass
@@ -76,19 +76,33 @@ git_fetch() {
  # (not allowed for a hash, unless remote is configured to permit
  # it with allow*SHA1InWant).
  _depth="--depth 1"
+ _branch=""
  if defined GIT_TAG
  then
- _depth+=" --branch ${GIT_TAG}"
+ _depth=" --branch ${GIT_TAG}"
  elif defined GIT_BRANCH
  then
- _depth+=" --branch ${GIT_BRANCH}"
+ _depth=" --branch ${GIT_BRANCH}"
  fi
  fi

  # T likely doesn't exist at this point, so create it first
  mkdir -p ${T}
  cd ${T}
- verbose git clone ${_depth} --no-checkout ${GIT_URI} ${GIT_MODULE}
|| error "git clone failed"
+ _gitlog=${T}/git.$$.log
+ verbose git clone ${_depth} ${_branch} --no-checkout ${GIT_URI}
${GIT_MODULE} |& tee ${_gitlog}
+ if [ ${PIPESTATUS[0]} != 0 ]
+ then
+ grep "fatal: dumb http transport does not support shallow
capabilities" ${_gitlog} >& /dev/null
+ if [ $? = 0 ]
+ then
+ warning "git clone failed, retry without --depth option"
+ verbose git clone ${_branch} --no-checkout ${GIT_URI} ${GIT_MODULE}
|| error "git clone failed"
+ else
+ error "git clone failed"
+ fi
+ fi
  cd ${T}/${GIT_MODULE}

 #****v* git.cygclass/GIT_BRANCH


On Mon, Nov 20, 2023 at 11:23 PM Jon Turney <jon.turney@dronecode.org.uk> wrote:
>
> On 19/11/2023 02:11, Daisuke Fujimura via Cygwin-apps wrote:
> > Some git providers do not support smart transport, so specifying the
> > depth option will result in an error.
>
> Right. This is a bug and needs fixing.
>
> Thanks for the patch.
>
> > ```
> > Cloning into 'xxxx'...
> > fatal: dumb http transport does not support shallow capabilities
> > ```
> >
> > Therefore, I suggest adding a variable to suppress the depth option.
> > (Variable names should be changed to something appropriate according
> > to the naming convention.)
> >
> > diff --git a/cygclass/git.cygclass b/cygclass/git.cygclass
> > index e53a7985..0aa97a09 100644
> > --- a/cygclass/git.cygclass
> > +++ b/cygclass/git.cygclass
> > @@ -75,7 +75,12 @@ git_fetch() {
> > # shallow fetch a ref (master, branch or tag) with --depth=1
> > # (not allowed for a hash, unless remote is configured to permit
> > # it with allow*SHA1InWant).
> > - _depth="--depth 1"
> > + _depth=""
> > + # git provider does not support smart transport
> > + if ! defined GIT_PROVIDER_NOT_SUPPORT_SMART_TRANSPORT
>
> If you're going to add a variable which changes the behaviour of cygport
> like this, it should be documented (by adding an appropriate robodoc
> comment)
>
> This could just be named something a little shorter, like
> "GIT_URI_NO_SMART_TRANSPORT", since it's really a property of the URI's
> host?
>
> But I wonder if wouldn't just be better to try with --depth and then
> fallback to without it, if that fails (especially if we can tell it
> failed for that reason).
>
> (Looking at [1], that seems a better approach than trying to probe the
> URI for smart transport support, which seems problematic)
>
> [1]
> https://stackoverflow.com/questions/9270488/is-it-possible-to-detect-whether-a-http-git-remote-is-smart-or-dumb
>
> What do you think?
>
> > + then
> > + _depth="--depth 1"
> > + fi
> > if defined GIT_TAG
> > then
> > _depth+=" --branch ${GIT_TAG}"
> >
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH cygport] git.cygclass: Suppress the depth option
  2023-11-30 12:17   ` Daisuke Fujimura
@ 2023-12-03 20:34     ` Jon Turney
  2023-12-03 21:53       ` Brian Inglis
  0 siblings, 1 reply; 11+ messages in thread
From: Jon Turney @ 2023-12-03 20:34 UTC (permalink / raw)
  To: Daisuke Fujimura, cygwin-apps

[-- Attachment #1: Type: text/plain, Size: 3071 bytes --]

On 30/11/2023 12:17, Daisuke Fujimura via Cygwin-apps wrote:
> Implementations that conditionally branch on variables are simple.
> 
> The proposed retry implementation complicates git.cygclass, but I
> think it reduces the maintainer's effort.
> 
> I have created a patch for a retry implementation.
> Could you review it?

Thanks very much.

Sure.

> diff --git a/cygclass/git.cygclass b/cygclass/git.cygclass
> index e53a7985..1e26ab37 100644
> --- a/cygclass/git.cygclass
> +++ b/cygclass/git.cygclass
> @@ -76,19 +76,33 @@ git_fetch() {
>    # (not allowed for a hash, unless remote is configured to permit
>    # it with allow*SHA1InWant).
>    _depth="--depth 1"
> + _branch=""

I think this is not necessary, as expanding an undefined variable is 
permitted and has the equivalent empty value.

>    if defined GIT_TAG
>    then
> - _depth+=" --branch ${GIT_TAG}"
> + _depth=" --branch ${GIT_TAG}"

I think this wants to be _branch, as otherwise that used but never defined?

>    elif defined GIT_BRANCH
>    then
> - _depth+=" --branch ${GIT_BRANCH}"
> + _depth=" --branch ${GIT_BRANCH}"

Likewise.

>    fi
>    fi
> 
>    # T likely doesn't exist at this point, so create it first
>    mkdir -p ${T}
>    cd ${T}
> - verbose git clone ${_depth} --no-checkout ${GIT_URI} ${GIT_MODULE}
> || error "git clone failed"
> + _gitlog=${T}/git.$$.log
> + verbose git clone ${_depth} ${_branch} --no-checkout ${GIT_URI}
> ${GIT_MODULE} |& tee ${_gitlog}
> + if [ ${PIPESTATUS[0]} != 0 ]
> + then
> + grep "fatal: dumb http transport does not support shallow
> capabilities" ${_gitlog} >& /dev/null

Can't this just use 'grep -q' ?

I wonder if there's a locale issue here (i.e. will git produce a 
localized error message if LANG etc. is defined?)

Maybe depending on the precise string is too fragile, and we should just 
unconditionally retry to see if things get better?

> + if [ $? = 0 ]
> + then
> + warning "git clone failed, retry without --depth option"
> + verbose git clone ${_branch} --no-checkout ${GIT_URI} ${GIT_MODULE}
> || error "git clone failed"
> + else

In this case, the clone failed for a different reason, but we've eaten 
the output from git, so maybe there's no indication given as to why?

Do we want to do something like "cat ${_gitlog}" here?

> + error "git clone failed"
> + fi
> + fi
>    cd ${T}/${GIT_MODULE}
> 
>   #****v* git.cygclass/GIT_BRANCH
> 
> 
> On Mon, Nov 20, 2023 at 11:23 PM Jon Turney <jon.turney@dronecode.org.uk> wrote:
>>
>> On 19/11/2023 02:11, Daisuke Fujimura via Cygwin-apps wrote:
>>> Some git providers do not support smart transport, so specifying the
>>> depth option will result in an error.
>>
>> Right. This is a bug and needs fixing.
>>
>> Thanks for the patch.
>>
>>> ```
>>> Cloning into 'xxxx'...
>>> fatal: dumb http transport does not support shallow capabilities
>>> ```
>>>
[...]
>>
>> But I wonder if wouldn't just be better to try with --depth and then
>> fallback to without it, if that fails (especially if we can tell it
>> failed for that reason).
>>

Attached is the patch after my edits.

[-- Attachment #2: 0001-git.cygclass-Retry-without-the-depth-option.patch --]
[-- Type: text/plain, Size: 1920 bytes --]

From 825353de17d828cd3e6a2a2863f4ea661136c6bb Mon Sep 17 00:00:00 2001
From: Daisuke Fujimura <booleanlabel@gmail.com>
Date: Sun, 3 Dec 2023 18:44:05 +0000
Subject: [PATCH cygport] git.cygclass: Retry without the depth option

Some git providers do not support smart transport, so specifying the
'depth' option will result in an error.

```
Cloning into 'xxxx'...
fatal: dumb http transport does not support shallow capabilities
```

Retry without it in those cases
---
 cygclass/git.cygclass | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/cygclass/git.cygclass b/cygclass/git.cygclass
index e53a7985..3cb2c0d5 100644
--- a/cygclass/git.cygclass
+++ b/cygclass/git.cygclass
@@ -78,17 +78,35 @@ git_fetch() {
 		_depth="--depth 1"
 		if defined GIT_TAG
 		then
-			_depth+=" --branch ${GIT_TAG}"
+			_branch="--branch ${GIT_TAG}"
 		elif defined GIT_BRANCH
 		then
-			_depth+=" --branch ${GIT_BRANCH}"
+			_branch="--branch ${GIT_BRANCH}"
 		fi
 	fi
 
 	# T likely doesn't exist at this point, so create it first
 	mkdir -p ${T}
 	cd ${T}
-	verbose git clone ${_depth} --no-checkout ${GIT_URI} ${GIT_MODULE} || error "git clone failed"
+
+	# Try to clone with the depth option (if appropriate), but retry if that
+	# fails due to lack of capabilities of the host of the specified
+	# GIT_URI.
+	_gitlog=${T}/git.$$.log
+	verbose git clone ${_depth} ${_branch} --no-checkout ${GIT_URI} ${GIT_MODULE} |& tee ${_gitlog}
+	if [ ${PIPESTATUS[0]} != 0 ]
+	then
+		grep -q "fatal: dumb http transport does not support shallow capabilities" ${_gitlog}
+		if [ $? = 0 ]
+		then
+			warning "git clone failed, retrying without --depth option"
+			verbose git clone ${_branch} --no-checkout ${GIT_URI} ${GIT_MODULE} || error "git clone failed"
+		else
+			cat ${_gitlog}
+			error "git clone failed"
+		fi
+	fi
+
 	cd ${T}/${GIT_MODULE}
 
 #****v* git.cygclass/GIT_BRANCH
-- 
2.42.1


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH cygport] git.cygclass: Suppress the depth option
  2023-12-03 20:34     ` Jon Turney
@ 2023-12-03 21:53       ` Brian Inglis
  2023-12-16 15:38         ` Daisuke Fujimura
  2024-02-11 17:09         ` Jon Turney
  0 siblings, 2 replies; 11+ messages in thread
From: Brian Inglis @ 2023-12-03 21:53 UTC (permalink / raw)
  To: cygwin-apps

On 2023-12-03 13:34, Jon Turney via Cygwin-apps wrote:
> On 30/11/2023 12:17, Daisuke Fujimura via Cygwin-apps wrote:
>> Implementations that conditionally branch on variables are simple.
>>
>> The proposed retry implementation complicates git.cygclass, but I
>> think it reduces the maintainer's effort.
>>
>> I have created a patch for a retry implementation.
>> Could you review it?
> 
> Thanks very much.
> 
> Sure.
> 
>> diff --git a/cygclass/git.cygclass b/cygclass/git.cygclass
>> index e53a7985..1e26ab37 100644
>> --- a/cygclass/git.cygclass
>> +++ b/cygclass/git.cygclass
>> @@ -76,19 +76,33 @@ git_fetch() {
>>    # (not allowed for a hash, unless remote is configured to permit
>>    # it with allow*SHA1InWant).
>>    _depth="--depth 1"
>> + _branch=""
> 
> I think this is not necessary, as expanding an undefined variable is permitted 
> and has the equivalent empty value.
> 
>>    if defined GIT_TAG
>>    then
>> - _depth+=" --branch ${GIT_TAG}"
>> + _depth=" --branch ${GIT_TAG}"
> 
> I think this wants to be _branch, as otherwise that used but never defined?
> 
>>    elif defined GIT_BRANCH
>>    then
>> - _depth+=" --branch ${GIT_BRANCH}"
>> + _depth=" --branch ${GIT_BRANCH}"
> 
> Likewise.
> 
>>    fi
>>    fi
>>
>>    # T likely doesn't exist at this point, so create it first
>>    mkdir -p ${T}
>>    cd ${T}
>> - verbose git clone ${_depth} --no-checkout ${GIT_URI} ${GIT_MODULE}
>> || error "git clone failed"
>> + _gitlog=${T}/git.$$.log
>> + verbose git clone ${_depth} ${_branch} --no-checkout ${GIT_URI}
>> ${GIT_MODULE} |& tee ${_gitlog}
>> + if [ ${PIPESTATUS[0]} != 0 ]
>> + then
>> + grep "fatal: dumb http transport does not support shallow
>> capabilities" ${_gitlog} >& /dev/null
> 
> Can't this just use 'grep -q' ?
> 
> I wonder if there's a locale issue here (i.e. will git produce a localized error 
> message if LANG etc. is defined?)
> 
> Maybe depending on the precise string is too fragile, and we should just 
> unconditionally retry to see if things get better?
> 
>> + if [ $? = 0 ]
>> + then
>> + warning "git clone failed, retry without --depth option"
>> + verbose git clone ${_branch} --no-checkout ${GIT_URI} ${GIT_MODULE}
>> || error "git clone failed"
>> + else
> 
> In this case, the clone failed for a different reason, but we've eaten the 
> output from git, so maybe there's no indication given as to why?
> 
> Do we want to do something like "cat ${_gitlog}" here?
> 
>> + error "git clone failed"
>> + fi
>> + fi
>>    cd ${T}/${GIT_MODULE}
>>
>>   #****v* git.cygclass/GIT_BRANCH
>>
>>
>> On Mon, Nov 20, 2023 at 11:23 PM Jon Turney <jon.turney@dronecode.org.uk> wrote:
>>>
>>> On 19/11/2023 02:11, Daisuke Fujimura via Cygwin-apps wrote:
>>>> Some git providers do not support smart transport, so specifying the
>>>> depth option will result in an error.
>>>
>>> Right. This is a bug and needs fixing.
>>>
>>> Thanks for the patch.
>>>
>>>> ```
>>>> Cloning into 'xxxx'...
>>>> fatal: dumb http transport does not support shallow capabilities
>>>> ```
>>>>
> [...]
>>>
>>> But I wonder if wouldn't just be better to try with --depth and then
>>> fallback to without it, if that fails (especially if we can tell it
>>> failed for that reason).
>>>
> 
> Attached is the patch after my edits.

Looks like straight curl HEAD -I tells you about smart transport if you want a 
quick check rather than a dry run:

$ time curl -ILSs https://repo.or.cz/r/git.git/info/refs?service=git-upload-pack 
| grep -qi '^content-type:\sapplication/x-git-upload-pack'; echo $?

real    0m0.630s
user    0m0.077s
sys     0m0.123s
0
$ time curl -ILSs 
https://github.com/BrianInglis/apt-cyg.git/info/refs?service=git-upload-pack  | 
grep -qi '^content-type:\sapplication/x-git-upload-pack'; echo $?

real    0m0.440s
user    0m0.061s
sys     0m0.184s
1

-- 
Take care. Thanks, Brian Inglis              Calgary, Alberta, Canada

La perfection est atteinte                   Perfection is achieved
non pas lorsqu'il n'y a plus rien à ajouter  not when there is no more to add
mais lorsqu'il n'y a plus rien à retirer     but when there is no more to cut
                                 -- Antoine de Saint-Exupéry

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH cygport] git.cygclass: Suppress the depth option
  2023-12-03 21:53       ` Brian Inglis
@ 2023-12-16 15:38         ` Daisuke Fujimura
  2024-01-13 14:49           ` Jon Turney
  2024-02-11 17:09         ` Jon Turney
  1 sibling, 1 reply; 11+ messages in thread
From: Daisuke Fujimura @ 2023-12-16 15:38 UTC (permalink / raw)
  To: cygwin-apps

I have implemented a curl-based smart transport check. How about this one?

diff --git a/cygclass/git.cygclass b/cygclass/git.cygclass
index e53a7985..f3ed343e 100644
--- a/cygclass/git.cygclass
+++ b/cygclass/git.cygclass
@@ -67,6 +67,7 @@ SRC_DIR="${GIT_MODULE}${GIT_SUBDIR+/}${GIT_SUBDIR}"

 git_fetch() {
  local _depth
+ local _branch

  check_prog_req git

@@ -78,17 +79,21 @@ git_fetch() {
  _depth="--depth 1"
  if defined GIT_TAG
  then
- _depth+=" --branch ${GIT_TAG}"
+ _branch="--branch ${GIT_TAG}"
  elif defined GIT_BRANCH
  then
- _depth+=" --branch ${GIT_BRANCH}"
+ _branch="--branch ${GIT_BRANCH}"
  fi
  fi

+ check_prog_req curl
+ curl -is ${GIT_URI}/info/refs?service=git-upload-pack | grep
--binary-files=text -i '^content-type:\sapplication/x-git-upload-pack'
>& /dev/null && _branch="${_depth} ${_branch}"
+
  # T likely doesn't exist at this point, so create it first
  mkdir -p ${T}
  cd ${T}
- verbose git clone ${_depth} --no-checkout ${GIT_URI} ${GIT_MODULE}
|| error "git clone failed"
+ verbose git clone ${_branch} --no-checkout ${GIT_URI} ${GIT_MODULE}
|| error "git clone failed"
+
  cd ${T}/${GIT_MODULE}

 #****v* git.cygclass/GIT_BRANCH


On Mon, Dec 4, 2023 at 6:53 AM Brian Inglis via Cygwin-apps
<cygwin-apps@cygwin.com> wrote:
>
> On 2023-12-03 13:34, Jon Turney via Cygwin-apps wrote:
> > On 30/11/2023 12:17, Daisuke Fujimura via Cygwin-apps wrote:
> >> Implementations that conditionally branch on variables are simple.
> >>
> >> The proposed retry implementation complicates git.cygclass, but I
> >> think it reduces the maintainer's effort.
> >>
> >> I have created a patch for a retry implementation.
> >> Could you review it?
> >
> > Thanks very much.
> >
> > Sure.
> >
> >> diff --git a/cygclass/git.cygclass b/cygclass/git.cygclass
> >> index e53a7985..1e26ab37 100644
> >> --- a/cygclass/git.cygclass
> >> +++ b/cygclass/git.cygclass
> >> @@ -76,19 +76,33 @@ git_fetch() {
> >>    # (not allowed for a hash, unless remote is configured to permit
> >>    # it with allow*SHA1InWant).
> >>    _depth="--depth 1"
> >> + _branch=""
> >
> > I think this is not necessary, as expanding an undefined variable is permitted
> > and has the equivalent empty value.
> >
> >>    if defined GIT_TAG
> >>    then
> >> - _depth+=" --branch ${GIT_TAG}"
> >> + _depth=" --branch ${GIT_TAG}"
> >
> > I think this wants to be _branch, as otherwise that used but never defined?
> >
> >>    elif defined GIT_BRANCH
> >>    then
> >> - _depth+=" --branch ${GIT_BRANCH}"
> >> + _depth=" --branch ${GIT_BRANCH}"
> >
> > Likewise.
> >
> >>    fi
> >>    fi
> >>
> >>    # T likely doesn't exist at this point, so create it first
> >>    mkdir -p ${T}
> >>    cd ${T}
> >> - verbose git clone ${_depth} --no-checkout ${GIT_URI} ${GIT_MODULE}
> >> || error "git clone failed"
> >> + _gitlog=${T}/git.$$.log
> >> + verbose git clone ${_depth} ${_branch} --no-checkout ${GIT_URI}
> >> ${GIT_MODULE} |& tee ${_gitlog}
> >> + if [ ${PIPESTATUS[0]} != 0 ]
> >> + then
> >> + grep "fatal: dumb http transport does not support shallow
> >> capabilities" ${_gitlog} >& /dev/null
> >
> > Can't this just use 'grep -q' ?
> >
> > I wonder if there's a locale issue here (i.e. will git produce a localized error
> > message if LANG etc. is defined?)
> >
> > Maybe depending on the precise string is too fragile, and we should just
> > unconditionally retry to see if things get better?
> >
> >> + if [ $? = 0 ]
> >> + then
> >> + warning "git clone failed, retry without --depth option"
> >> + verbose git clone ${_branch} --no-checkout ${GIT_URI} ${GIT_MODULE}
> >> || error "git clone failed"
> >> + else
> >
> > In this case, the clone failed for a different reason, but we've eaten the
> > output from git, so maybe there's no indication given as to why?
> >
> > Do we want to do something like "cat ${_gitlog}" here?
> >
> >> + error "git clone failed"
> >> + fi
> >> + fi
> >>    cd ${T}/${GIT_MODULE}
> >>
> >>   #****v* git.cygclass/GIT_BRANCH
> >>
> >>
> >> On Mon, Nov 20, 2023 at 11:23 PM Jon Turney <jon.turney@dronecode.org.uk> wrote:
> >>>
> >>> On 19/11/2023 02:11, Daisuke Fujimura via Cygwin-apps wrote:
> >>>> Some git providers do not support smart transport, so specifying the
> >>>> depth option will result in an error.
> >>>
> >>> Right. This is a bug and needs fixing.
> >>>
> >>> Thanks for the patch.
> >>>
> >>>> ```
> >>>> Cloning into 'xxxx'...
> >>>> fatal: dumb http transport does not support shallow capabilities
> >>>> ```
> >>>>
> > [...]
> >>>
> >>> But I wonder if wouldn't just be better to try with --depth and then
> >>> fallback to without it, if that fails (especially if we can tell it
> >>> failed for that reason).
> >>>
> >
> > Attached is the patch after my edits.
>
> Looks like straight curl HEAD -I tells you about smart transport if you want a
> quick check rather than a dry run:
>
> $ time curl -ILSs https://repo.or.cz/r/git.git/info/refs?service=git-upload-pack
> | grep -qi '^content-type:\sapplication/x-git-upload-pack'; echo $?
>
> real    0m0.630s
> user    0m0.077s
> sys     0m0.123s
> 0
> $ time curl -ILSs
> https://github.com/BrianInglis/apt-cyg.git/info/refs?service=git-upload-pack  |
> grep -qi '^content-type:\sapplication/x-git-upload-pack'; echo $?
>
> real    0m0.440s
> user    0m0.061s
> sys     0m0.184s
> 1
>
> --
> Take care. Thanks, Brian Inglis              Calgary, Alberta, Canada
>
> La perfection est atteinte                   Perfection is achieved
> non pas lorsqu'il n'y a plus rien à ajouter  not when there is no more to add
> mais lorsqu'il n'y a plus rien à retirer     but when there is no more to cut
>                                  -- Antoine de Saint-Exupéry

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH cygport] git.cygclass: Suppress the depth option
  2023-12-16 15:38         ` Daisuke Fujimura
@ 2024-01-13 14:49           ` Jon Turney
  0 siblings, 0 replies; 11+ messages in thread
From: Jon Turney @ 2024-01-13 14:49 UTC (permalink / raw)
  To: Daisuke Fujimura; +Cc: cygwin-apps

On 16/12/2023 15:38, Daisuke Fujimura via Cygwin-apps wrote:
> I have implemented a curl-based smart transport check. How about this one?

Thanks.

Again, sorry about my terrible slowness in reviewing this.

> diff --git a/cygclass/git.cygclass b/cygclass/git.cygclass
> index e53a7985..f3ed343e 100644
> --- a/cygclass/git.cygclass
> +++ b/cygclass/git.cygclas
[...]
> 
> + check_prog_req curl
> + curl -is ${GIT_URI}/info/refs?service=git-upload-pack | grep
> --binary-files=text -i '^content-type:\sapplication/x-git-upload-pack'
>> & /dev/null && _branch="${_depth} ${_branch}"
> +

It seems like this is going to give a false negative on 'git://' 
transport URLs?

Maybe we should check if GIT_URI starts 'http(|s)://' ?


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH cygport] git.cygclass: Suppress the depth option
  2023-12-03 21:53       ` Brian Inglis
  2023-12-16 15:38         ` Daisuke Fujimura
@ 2024-02-11 17:09         ` Jon Turney
  2024-02-16 11:59           ` Daisuke Fujimura
  1 sibling, 1 reply; 11+ messages in thread
From: Jon Turney @ 2024-02-11 17:09 UTC (permalink / raw)
  Cc: cygwin-apps

On 03/12/2023 21:53, Brian Inglis via Cygwin-apps wrote:
> On 2023-12-03 13:34, Jon Turney via Cygwin-apps wrote:
>> On 30/11/2023 12:17, Daisuke Fujimura via Cygwin-apps wrote:
>>> Implementations that conditionally branch on variables are simple.
>>>
>>> The proposed retry implementation complicates git.cygclass, but I
>>> think it reduces the maintainer's effort.
>>>
>>> I have created a patch for a retry implementation.
>>> Could you review it?
>>

Thanks very much to Fujimura-san for all his work on this.

>> Attached is the patch after my edits.

I've applied a reheated version of this patch. Hopefully that works well 
enough, but obviously can be further refined if needed.

> Looks like straight curl HEAD -I tells you about smart transport if you 
> want a quick check rather than a dry run:
> 
> $ time curl -ILSs 
> https://repo.or.cz/r/git.git/info/refs?service=git-upload-pack | grep 
> -qi '^content-type:\sapplication/x-git-upload-pack'; echo $?
> 
> real    0m0.630s
> user    0m0.077s
> sys     0m0.123s
> 0
> $ time curl -ILSs 
> https://github.com/BrianInglis/apt-cyg.git/info/refs?service=git-upload-pack  | grep -qi '^content-type:\sapplication/x-git-upload-pack'; echo $?
> 
> real    0m0.440s
> user    0m0.061s
> sys     0m0.184s
> 1

Thanks for this.

Uh, but it seems like 'git clone --depth 1' works with both of these 
URLs, so... um... I'm not sure what's going on.


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH cygport] git.cygclass: Suppress the depth option
  2024-02-11 17:09         ` Jon Turney
@ 2024-02-16 11:59           ` Daisuke Fujimura
  2024-02-18 15:10             ` Jon Turney
  0 siblings, 1 reply; 11+ messages in thread
From: Daisuke Fujimura @ 2024-02-16 11:59 UTC (permalink / raw)
  To: cygwin-apps

Thank you for merging.

I have confirmed that this modification has resulted in the intended behaviour.

```
$  cygport --version
cygport 0.36.8
Copyright (C) 2020 Cygport authors

This program comes with NO WARRANTY, to the extent permitted by law.

You may redistribute copies of this program under the terms of
the GNU General Public License as published by the Free Software
Foundation, either version 3 of the License, or (at your option) any
later version.

For more information about these matters, see the file named COPYING.

Written for the Cygwin project <https://cygwin.com/>.


$ head -3 agbsum-15-1bl1.cygport
HOMEPAGE="https://mandelbrot.dk/${PN}"
GIT_URI="https://mandelbrot.dk/${PN}"
GIT_TAG="${PV}"

$ cygport agbsum-15-1bl1.cygport fetch
*** Info: Trying to enable case sensitivity on /tmp/agbsum/agbsum-15-1bl1.x86_64
git clone --depth 1 --branch 15 --no-checkout
https://mandelbrot.dk/agbsum agbsum
Cloning into 'agbsum'...
fatal: dumb http transport does not support shallow capabilities
*** Warning: git clone failed, retrying without --depth option
git clone --branch 15 --no-checkout https://mandelbrot.dk/agbsum agbsum
Cloning into 'agbsum'...
Fetching objects: 251, done.
git checkout tags/15
HEAD is now at bef1780 Rename source directory: 'src' => 'source';
Update naming convention; Update copyright holder name; Update code
style;

```

On Mon, Feb 12, 2024 at 2:09 AM Jon Turney via Cygwin-apps
<cygwin-apps@cygwin.com> wrote:
>
> On 03/12/2023 21:53, Brian Inglis via Cygwin-apps wrote:
> > On 2023-12-03 13:34, Jon Turney via Cygwin-apps wrote:
> >> On 30/11/2023 12:17, Daisuke Fujimura via Cygwin-apps wrote:
> >>> Implementations that conditionally branch on variables are simple.
> >>>
> >>> The proposed retry implementation complicates git.cygclass, but I
> >>> think it reduces the maintainer's effort.
> >>>
> >>> I have created a patch for a retry implementation.
> >>> Could you review it?
> >>
>
> Thanks very much to Fujimura-san for all his work on this.
>
> >> Attached is the patch after my edits.
>
> I've applied a reheated version of this patch. Hopefully that works well
> enough, but obviously can be further refined if needed.
>
> > Looks like straight curl HEAD -I tells you about smart transport if you
> > want a quick check rather than a dry run:
> >
> > $ time curl -ILSs
> > https://repo.or.cz/r/git.git/info/refs?service=git-upload-pack | grep
> > -qi '^content-type:\sapplication/x-git-upload-pack'; echo $?
> >
> > real    0m0.630s
> > user    0m0.077s
> > sys     0m0.123s
> > 0
> > $ time curl -ILSs
> > https://github.com/BrianInglis/apt-cyg.git/info/refs?service=git-upload-pack  | grep -qi '^content-type:\sapplication/x-git-upload-pack'; echo $?
> >
> > real    0m0.440s
> > user    0m0.061s
> > sys     0m0.184s
> > 1
>
> Thanks for this.
>
> Uh, but it seems like 'git clone --depth 1' works with both of these
> URLs, so... um... I'm not sure what's going on.
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH cygport] git.cygclass: Suppress the depth option
  2024-02-16 11:59           ` Daisuke Fujimura
@ 2024-02-18 15:10             ` Jon Turney
  0 siblings, 0 replies; 11+ messages in thread
From: Jon Turney @ 2024-02-18 15:10 UTC (permalink / raw)
  To: Daisuke Fujimura; +Cc: cygwin-apps

On 16/02/2024 11:59, Daisuke Fujimura via Cygwin-apps wrote:
> Thank you for merging.
> 
> I have confirmed that this modification has resulted in the intended behaviour.
> 
[...]
> 
> $ head -3 agbsum-15-1bl1.cygport
> HOMEPAGE="https://mandelbrot.dk/${PN}"
> GIT_URI="https://mandelbrot.dk/${PN}"
> GIT_TAG="${PV}"
> 
> $ cygport agbsum-15-1bl1.cygport fetch
> *** Info: Trying to enable case sensitivity on /tmp/agbsum/agbsum-15-1bl1.x86_64
> git clone --depth 1 --branch 15 --no-checkout
> https://mandelbrot.dk/agbsum agbsum
> Cloning into 'agbsum'...
> fatal: dumb http transport does not support shallow capabilities
> *** Warning: git clone failed, retrying without --depth option
> git clone --branch 15 --no-checkout https://mandelbrot.dk/agbsum agbsum
> Cloning into 'agbsum'...
> Fetching objects: 251, done.
> git checkout tags/15
> HEAD is now at bef1780 Rename source directory: 'src' => 'source';
> Update naming convention; Update copyright holder name; Update code
> style;
> 
> ```

Thank you very much for testing!


^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2024-02-18 15:10 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-11-19  2:11 [PATCH cygport] git.cygclass: Suppress the depth option Daisuke Fujimura
2023-11-20 14:23 ` Jon Turney
2023-11-20 16:42   ` ASSI
2023-11-30 12:17   ` Daisuke Fujimura
2023-12-03 20:34     ` Jon Turney
2023-12-03 21:53       ` Brian Inglis
2023-12-16 15:38         ` Daisuke Fujimura
2024-01-13 14:49           ` Jon Turney
2024-02-11 17:09         ` Jon Turney
2024-02-16 11:59           ` Daisuke Fujimura
2024-02-18 15:10             ` Jon Turney

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).