git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v3] fetch: add new config option fetch.all

From
Eric Sunshine <sunshine@sunshineco.com>
Date
Jan 6, 2024, 23:32 UTC
Message-ID
<CAPig+cQ3y7rGbrNjMZB_HpMAWYOhDg8qKSoR5EF=T5+jc6rgvA@mail.gmail.com>
In-Reply-To
<20240106202352.7253-2-dev@tb6.eu>
On Sat, Jan 6, 2024 at 3:25 PM Tamino Bauknecht <dev@tb6.eu> wrote:
Show 21 quoted lines
> This commit introduces the new boolean configuration option fetch.all
> which allows to fetch all available remotes by default. The config
> option can be overridden by explicitly specifying a remote or by using
> --no-all.
> The behavior for --all is unchanged and calling git-fetch with --all and
> a remote will still result in an error.
>
> The config option was also added to the config documentation and new
> tests cover the expected behavior.
> Additionally, --no-all was added to the command-line documentation of
> git-fetch.
>
> Signed-off-by: Tamino Bauknecht <dev@tb6.eu>
> ---
> diff --git a/t/t5514-fetch-multiple.sh b/t/t5514-fetch-multiple.sh
> @@ -209,4 +218,156 @@ test_expect_success 'git fetch --multiple --jobs=0 picks a default' '
> +create_fetch_all_expect () {
> +       cat >expect <<-\EOF || return 1
> +         ...
> +       EOF
> +}

This is really minor, but the `|| return 1` is superfluous. The `cat` command itself will exit with success or failure, and since it's the last command in the function, its return value will be the value returned by the function. Thus, there is no need to use `|| return 1` to signal failure when the `cat` command itself will do so anyhow.

For such a minor issue, I would typically say "not worth a reroll", however, this sort of unnecessary code may confuse future readers into thinking that something unusual and non-obvious is going on. As such, it might be worth a reroll after all, but if you do choose to reroll, wait until others have chimed in since they might have more to add (either about this or other parts of the patch).

Previous: Tamino BauknechtNext: Eric Sunshine
Message 19 of 23 in “new config option "fetch.all"”
  1. Tamino BauknechtDec 18, 2023
  2. fetch: add new config option fetch.allTamino Bauknecht, Jan 4, 2024
  3. Taylor BlauJan 4, 2024
  4. Eric SunshineJan 4, 2024
  5. Junio C HamanoJan 4, 2024
  6. Tamino BauknechtJan 4, 2024
  7. Taylor BlauJan 4, 2024
  8. Junio C HamanoJan 4, 2024
  9. Tamino BauknechtJan 4, 2024
  10. Junio C HamanoJan 4, 2024
  11. fetch: add new config option fetch.allTamino Bauknecht, Jan 4, 2024
  12. Eric SunshineJan 4, 2024
  13. Eric SunshineJan 4, 2024
  14. 1/2 fetch: add new config option fetch.allTamino Bauknecht, Jan 4, 2024
  15. 2/2 fetch: add cli option --default-onlyTamino Bauknecht, Jan 4, 2024
  16. Eric SunshineJan 5, 2024
  17. Junio C HamanoJan 5, 2024
  18. fetch: add new config option fetch.allTamino Bauknecht, Jan 6, 2024
  19. Eric SunshineJan 6, 2024
  20. Eric SunshineJan 6, 2024
  21. Junio C HamanoJan 8, 2024
  22. fetch: add new config option fetch.allTamino Bauknecht, Jan 8, 2024
  23. Eric SunshineJan 5, 2024

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.