Switch archive URL code back to href attributes

[fnetX]

Fixes #16354

I have two commits, the first one adds back the href attribute to archive links, so some legacy scripts which use it (e.g. some Debian packages) work again + it works without JavaScript too. The preventDefault already makes sure the async function is used when JS is available.

The second commit switches the async function to use the href attribute and drops the data-url, I split this off in case you don't want to change this back (e.g. fear stupid scripts which started relying on the data-url in the meantime instead of just using the API). Drop this if you like.

Thank you for considering a merge.

[wxiaoguang]

I think it's reasonable.

[wxiaoguang]

Oh wait, have you searched the whole code base? Is there any other archive-link with data-url?

[fnetX]

Good point, there are. I checked some other files, but didn't search for them. I will change this.

[fnetX]

Sorry for the mistake. I tried to work with the original PR to change this, but was confused by some differences. I missed that it also changed other places. I can also provide a commit only adding the href attributes in case you don't want to incldue the 2nd commit.

[wxiaoguang]

Both are fine to me. I think it's good to use href with <a> without redundancy as what you do.

Let's see other's thoughts. 😊

[delvh]

If I see that correctly what these files are for, we could even use the standard way now and make this a download link.
Then no JS is needed at all.

[Gusted]

If I see that correctly what these files are for, we could even use the standard way now and make this a download link. Then no JS is needed at all.

https://developer.mozilla.org/en-US/docs/Web/HTML/Element/a#attr-download - we're already using it with the download file button.

[delvh]

Where exactly do you see the download attribute inside the changed links?

[Gusted]

Where exactly do you see the download attribute inside the changed links?

Nowhere, I mentioned that we're already using such solution to use the download attribute at the download file button

gitea/templates/repo/view_file.tmpl

Line 51 in 71ed96b

<a download href="{{$.RawFileLink}}"><span class="btn-octicon tooltip" data-content="{{.i18n.Tr "repo.download_file"}}" data-position="bottom center">{{svg "octicon-download"}}</span></a>

[wxiaoguang]

If I see that correctly what these files are for, we could even use the standard way now and make this a download link. Then no JS is needed at all.

To delvh , I think it's better to read code carefully before you make a comment. Have you ever read the code about getArchive? What does it do? Why the code was written? Why no JS is needed?

[wxiaoguang]

@fnetX Maybe we also need rel=”nofollow” to prevent crawlers from access these links.

[fnetX]

Maybe we also need rel=”nofollow”

This is not relevant to this PR, I left it untouched, and it was also not present consistently before #11296. For some reason, some links have the nofollow value set, others don't. I can see to add it when I find the time.

[wxiaoguang]

@fnetX Not for you, but for @delvh . I have seen too many non-sense comments from him. So I have to point it out.

[wxiaoguang]

Maybe we also need rel=”nofollow”

This is not relevant to this PR, I left it untouched, and it was also not present consistently before #11296. For some reason, some links have the nofollow value set, others don't. I can see to add it when I find the time.

Why I think it's better to have rel=”nofollow”:

1. Since we change from data-url back to href, then the crawler will access the link again, but we do not want that, it would waste bandwidth and cost server resources and doesn't help the crawler.
2. GitHub just does it, you can refer how GitHub does on its release pages.

[fnetX]

I agree that the nofollow is good here, but I don't see this as a blocker for this PR, because it wasn't before the changes I intended to revert. I will add it once I find the three minutes to get back to git.

[wxiaoguang]

Yep, just a suggestion, I already approved 😊

[silverwind]

Is there any point in keeping the JS around?

[zeripath]

@fnetX :

I agree that the nofollow is good here, but I don't see this as a blocker for this PR, because it wasn't before the changes I intended to revert. I will add it once I find the three minutes to get back to git.

It is a blocker - by adding the hrefs back in we absolutely must have ref="nofollow" on these links otherwise we'll be inviting search engines to crawl downloads.

@silverwind :

Is there any point in keeping the JS around?

The JS creates a nice UI to tell the user when the archive is ready for download - if we don't have it and downgrade to using a GET only we lose that.

I think it should stay.

[wxiaoguang]

The JS works for async preparing:

1. A user clicks the link, JS handles the click and sends a POST to backend to prepare the archive, and JS shows a loading to user.
2. When the archive is ready, JS triggers the download action by using GET

[silverwind]

POST to backend to prepare the archive

I guess it could be argued why this even is a POST. The backend could just on-the-fly create the .zip and stream it to HTTP, in which case it is a GET by definition and the server doesn't even need to write the archive onto disk :)

[wxiaoguang]

It would cost much time on large repository/files. And the archive written onto the disk is a kind of cache.

We can refer the PR which introduced this change.

[wxiaoguang]

This feature came from:

• [RFC] Make archival asynchronous #11296

[silverwind]

I wouldn't have approved that. Their argument about reverse proxy is weak beause if they run into a timeout on their proxy, they ought to better fix their configuration instead.

[wxiaoguang]

I would approve it.

For example, if the archive is large, and the backend needs more than 1 minute to get it ready, it's unreasonable to make a HTTP request don't response in more than 1 minute. The archive task should be a background task.

Even if there is no reverse proxy, some users may use HTTP/Socks5 proxy for their browsers, nobody can guess what are the timeout settings for these browser proxies. And I haven't seen any HTTP request (except long-polling) would last for a long time without response in real world.

And with the prepared archive, we may offer HTTP 206 Partial Content to support pause & resume (I don't know whether we have such support now, but I did see some feature request issues for HTTP 206 downloading). The on-the-fly download can not be resumed.

[wxiaoguang]

I just checked the backend code again, I think it isn't suitable for on-the-fly method :(

[silverwind]

the backend needs more than 1 minute to get it ready

If you stream the content (e.g. the golang equivalent of Node content.pipe(gzip).pipe(httpResponse)), you can deliver a instant response. That's how both GitHub and GitLab seem to also do it. The benefit here is that archives retrieval works via curl and website backup systems (like archive.org) can easily archive content.

HTTP 206 Partial Content to support pause & resume

I think the preferred way is to support the Range header, but alas, that won't work on streamed content.