wget-dev
[Top][All Lists]
Advanced

[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.




reply via email to

[Prev in Thread] Current Thread [Next in Thread]