[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Wget-dev] wget2 | Fix for -p -nc Bug (!416)
From: |
Darshit Shah |
Subject: |
Re: [Wget-dev] wget2 | Fix for -p -nc Bug (!416) |
Date: |
Fri, 22 Mar 2019 07:29:53 +0000 |
Merge request https://gitlab.com/gnuwget/wget2/merge_requests/416 was reviewed
by Darshit Shah
--
Darshit Shah started a new discussion on tests/test-p-nc.c:
> +/*
> + * Copyright(c) 2013 Tim Ruehsen
> + * Copyright(c) 2015-2019 Free Software Foundation, Inc.
The copyrights for this file only exist in 2019. Not anytime before that.
Change this to:
`Copyright(c) 2019 Free Software Foundation, Inc.`
--
Darshit Shah started a new discussion on tests/test-p-nc.c:
> + *
> + *
> + * Testing -p -nc
A better description would be nice here
--
Darshit Shah started a new discussion on tests/test-p-nc.c:
> + *
> + * Changelog
> + * 27.05.2014 Tim Ruehsen created
I'm pretty sure you created this file in 2019, not Tim in 2014.
--
Darshit Shah started a new discussion on tests/test-p-nc.c:
> + wget_test(
> +// WGET_TEST_KEEP_TMPFILES, 1,
> +// WGET_TEST_EXECUTABLE, "wget",
Please remove the commented code
--
Darshit Shah started a new discussion on tests/test-p-nc.c:
> + { urls[7].name + 1, urls[7].body },
> + { urls[9].name + 1, urls[9].body }, // test1.css
> + { urls[10].name + 1, urls[10].body }, // test2.css
The comment isn't aligned on this one
--
Darshit Shah started a new discussion on tests/test-p-nc.c:
> + { urls[4].name + 1, "modified image" },
> + { NULL } },
> + WGET_TEST_EXPECTED_FILES, &(wget_test_file_t []) {
Could you please explain here why `WGET_TEST_EXPECTED_FILES` here does not
match the list in the previous test?
--
Darshit Shah started a new discussion on tests/test-p-nc.c:
> +
> + exit(0);
> +}
This is a well written test.
Just a couple of nit picks here and there, but it's perfectly ready for a merge.
One small thing though, I would really prefer if this weren't in a separate
commit. The failing test will make `git bisect` significantly more complex in
the future. Please put the test in the commit that fixes the issue.
--
Darshit Shah started a new discussion on src/wget.c:
> } else if (!config.clobber || (config.recursive && config.directories))
> {
> - if (oflag == O_TRUNC && !(config.recursive &&
> config.directories))
> + if (oflag == O_TRUNC && (!(config.recursive &&
> config.directories) || (config.page_requisites && !config.clobber))) {
This would be clearer when you split it across multiple lines:
```
if (oflag == O_TRUNC &&
(!(config.recursive && config.directories) ||
(config.page_requisites && !config.clobber))) {
```
In general, try and keep the line lengths short. At around 80 chars per line.
This is not a hard rule, but it generally keeps the code more readable.
--
Reply to this email directly or view it on GitLab:
https://gitlab.com/gnuwget/wget2/merge_requests/416
You're receiving this email because of your account on gitlab.com.
- [Wget-dev] wget2 | Fix for -p -nc Bug (!416), Kumar Mallikarjuna, 2019/03/20
- Re: [Wget-dev] wget2 | Fix for -p -nc Bug (!416), Tim Rühsen, 2019/03/21
- Re: [Wget-dev] wget2 | Fix for -p -nc Bug (!416), Tim Rühsen, 2019/03/21
- Re: [Wget-dev] wget2 | Fix for -p -nc Bug (!416), Tim Rühsen, 2019/03/21
- Re: [Wget-dev] wget2 | Fix for -p -nc Bug (!416), Kumar Mallikarjuna, 2019/03/21
- Re: [Wget-dev] wget2 | Fix for -p -nc Bug (!416), Kumar Mallikarjuna, 2019/03/21
- Re: [Wget-dev] wget2 | Fix for -p -nc Bug (!416), Tim Rühsen, 2019/03/21
- Re: [Wget-dev] wget2 | Fix for -p -nc Bug (!416), Kumar Mallikarjuna, 2019/03/22
- Re: [Wget-dev] wget2 | Fix for -p -nc Bug (!416),
Darshit Shah <=
- Re: [Wget-dev] wget2 | Fix for -p -nc Bug (!416), Darshit Shah, 2019/03/22
- Message not available
- Message not available
- Message not available
- Re: [Wget-dev] wget2 | Fix for -p -nc Bug (!416), Kumar Mallikarjuna, 2019/03/22
- Message not available
- Message not available
- Message not available