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

On Thu, Jan 04, 2024 at 03:33:55PM +0100, Tamino Bauknecht wrote:

Nice. This looks like a useful feature that folks working with more than one remote may want to take advantage of. Not that typing "git fetch --all" is all that hard, but I can see the utility of something like this for a repository with >1 remotes where the individual wants to keep all remotes up-to-date.

Instead of "can be overridden ...", how about:

    This behavior can be overridden by explicitly specifying one or more
    remote(s) to fetch from.

To minimize the diff, let's leave the existing conditional as is, so the end state would look like:

    if (all) {
      if (argc == 1)
        die(_("fetch --all does not take a repository argument"));
      else if (argc > 1)
        die(_("fetch --all does not make sense with refspecs"));
    }

    if (all || (config.all > 0 && !argc))
      (void) for_each_remote(get_one_remote_for_fetch, &list);

I don't feel particularly strongly about whether or not you reorganize these if-statements, but we should change "argc == 0" to "!argc", which matches the conventions of the rest of the project.

This should be `cat >expect <<-\EOF` (without the space between the redirect and heredoc, as well as indicating that the heredoc does not need any shell expansions).

Same note here about the space between the redirect.

It looks like these tests match the existing style of the test suite, but they are somewhat out of date with respect to our more modern standards. I would probably write this like:

    test_expect_success 'git fetch --all (works with fetch.all = true)' '
      git clone one test10 &&
      test_config -C test10 fetch.all true &&
      for r in one two three
      do
        git -C test10 remote add $r ../$r || return 1
      done &&
      git -C test10 fetch --all &&
      git -C test10 branch -r >actual &&
      test_cmp expect actual
    '

While reviewing, I thought that the tests using the test9 and test10 clones were duplicates, but they are not: the earlier one uses a "git fetch", and the latter uses a "git fetch --all".

If we take just the test10 and test11 tests, I think there is some opportunity to consolidate these two, like so:

    for v in true false
    do
        test_expect_success "git fetch --all (works with fetch.all=$v)" '
          git clone one test10 &&
          test_config -C test10 fetch.all $v &&
          for r in one two three
          do
            git -C test10 remote add $r ../$r || return 1
          done &&
          git -C test10 fetch --all &&
          git -C test10 branch -r >actual &&
          test_cmp expect actual
        '
    done

Same note(s) about cleaning up this heredoc.

I suspect that you could go further with a "setup" function that gives you a fresh clone (with fetch.all set to a specified value), and then test test would continue on from the line "git fetch one". But I think it's worth not getting too carried away with refactoring these tests ;-).

Thanks, Taylor