git/builtin/push.c, branch v2.46.2

builtin/push: call set_refspecs after validating remote

2024-07-12T16:14:11Z

When an end-user runs "git push" with an empty string for the remote repository name, e.g. $ git push '' main "git push" fails with a BUG(). Even though this is a nonsense request that we want to fail, we shouldn't hit a BUG(). Instead we want to give a sensible error message, e.g., 'bad repository'". This is because since 9badf97c42 (remote: allow resetting url list, 2024-06-14), we reset the remote URL if the provided URL is empty. When a user of 'remotes_remote_get' tries to fetch a remote with an empty repo name, the function initializes the remote via 'make_remote'. But the remote is still not a valid remote, since the URL is empty, so it tries to add the URL alias using 'add_url_alias'. This in-turn will call 'add_url', but since the URL is empty we call 'strvec_clear' on the `remote->url`. Back in 'remotes_remote_get', we again check if the remote is valid, which fails, so we return 'NULL' for the 'struct remote *' value. The 'builtin/push.c' code, calls 'set_refspecs' before validating the remote. This worked with empty repo names earlier since we would get a remote, albeit with an empty URL. With the new changes, we get a 'NULL' remote value, this causes the check for remote to fail and raises the BUG in 'set_refspecs'. Do a simple fix by doing remote validation first. Also add a test to validate the bug fix. With this, we can also now directly pass remote to 'set_refspecs' instead of it trying to lazily obtain it. Helped-by: Jeff King Helped-by: Junio C Hamano Signed-off-by: Karthik Nayak Signed-off-by: Junio C Hamano

remote: drop checks for zero-url case

2024-06-14T16:34:39Z

Now that the previous commit removed the possibility that a "struct remote" will ever have zero url fields, we can drop a number of redundant checks and untriggerable code paths. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano

remote: simplify url/pushurl selection

2024-06-14T16:34:38Z

When we want to know the push urls for a remote, there is some simple logic: - if the user configured any remote.*.pushurl keys, then those make the complete set of push urls - otherwise we push to all urls in remote.*.url Many spots implement this with a level of indirection, assigning to a local url/url_nr pair. But since both arrays are now strvecs, we can just use a pointer to select the appropriate strvec, shortening the code a bit. Even though this is now a one-liner, since it is application logic that is present in so many places, it's worth abstracting a helper function. In fact, we already have such a function, but it's local to builtin/push.c. So we'll just make it available everywhere via remote.h. There are two spots to pay special attention to here: 1. in builtin/remote.c's get_url(), we are selecting first based on push_mode and then falling back to "url" when we're in push_mode but no pushurl is defined. The updated code makes that much more clear, compared to the original which had an "else" fall-through. 2. likewise in that file's set_url(), we _only_ respect push_mode, sine the point is that we are adding to pushurl in that case (whether it is empty or not). And thus it does not use our helper function. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano

remote: use strvecs to store remote url/pushurl

2024-06-14T16:34:38Z

Now that the url/pushurl fields of "struct remote" own their strings, we can switch from bare arrays to strvecs. This has a few advantages: - push/clear are now one-liners - likewise the free+assigns in alias_all_urls() can use strvec_replace() - we now use size_t for storage, avoiding possible overflow - this will enable some further cleanups in future patches There's quite a bit of fallout in the code that reads these fields, as it tends to access these arrays directly. But it's mostly a mechanical replacement of "url_nr" with "url.nr", and "url[i]" with "url.v[i]", with a few variations (e.g. "*url" could become "*url.v", but I used "url.v[0]" for consistency). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano

Merge branch 'en/header-cleanup'

2024-01-08T22:05:15Z

Remove unused header "#include". * en/header-cleanup: treewide: remove unnecessary includes in source files treewide: add direct includes currently only pulled in transitively trace2/tr2_tls.h: remove unnecessary include submodule-config.h: remove unnecessary include pkt-line.h: remove unnecessary include line-log.h: remove unnecessary include http.h: remove unnecessary include fsmonitor--daemon.h: remove unnecessary includes blame.h: remove unnecessary includes archive.h: remove unnecessary include treewide: remove unnecessary includes in source files treewide: remove unnecessary includes from header files

Merge branch 'jc/retire-cas-opt-name-constant'

2024-01-02T21:51:29Z

Code clean-up. * jc/retire-cas-opt-name-constant: remote.h: retire CAS_OPT_NAME

treewide: remove unnecessary includes in source files

2023-12-26T20:04:31Z

Each of these were checked with gcc -E -I. ${SOURCE_FILE} | grep ${HEADER_FILE} to ensure that removing the direct inclusion of the header actually resulted in that header no longer being included at all (i.e. that no other header pulled it in transitively). ...except for a few cases where we verified that although the header was brought in transitively, nothing from it was directly used in that source file. These cases were: * builtin/credential-cache.c * builtin/pull.c * builtin/send-pack.c Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano

Merge branch 'jk/config-cleanup'

2023-12-20T18:14:55Z

Code clean-up around use of configuration variables. * jk/config-cleanup: sequencer: simplify away extra git_config_string() call gpg-interface: drop pointless config_error_nonbool() checks push: drop confusing configset/callback redundancy config: use git_config_string() for core.checkRoundTripEncoding diff: give more detailed messages for bogus diff.* config config: use config_error_nonbool() instead of custom messages imap-send: don't use git_die_config() inside callback git_xmerge_config(): prefer error() to die() config: reject bogus values for core.checkstat

remote.h: retire CAS_OPT_NAME

2023-12-19T19:27:04Z

When the "--force-with-lease" option was introduced in 28f5d176 (remote.c: add command line option parser for "--force-with-lease", 2013-07-08), the design discussion revolved around the concept of "compare-and-swap", and it can still be seen in the name used for variables and helper functions. The end-user facing option name ended up to be a bit different, so during the development iteration of the feature, we used this C preprocessor macro to make it easier to rename it later. All of that happened more than 10 years ago, and the flexibility afforded by the CAS_OPT_NAME macro outlived its usefulness. Inline the constant string for the option name, like all other option names in the code. Signed-off-by: Junio C Hamano

push: drop confusing configset/callback redundancy

2023-12-08T23:26:22Z

We parse push config by calling git_config() with our git_push_config() callback. But inside that callback, when we see "push.gpgsign", we ignore the value passed into the callback and instead make a new call to git_config_get_value(). This is unnecessary at best, and slightly wrong at worst (if there are multiple instances, get_value() only returns one; both methods end up with last-one-wins, but we'd fail to report errors if earlier incarnations were bogus). The call was added by 68c757f219 (push: add a config option push.gpgSign for default signed pushes, 2015-08-19). That commit doesn't give any reason to deviate from the usual strategy here; it was probably just somebody unfamiliar with our config API and its conventions. It also added identical code to builtin/send-pack.c, which also handles push.gpgsign. And then the same issue spread to its neighbor in b33a15b081 (push: add recurseSubmodules config option, 2015-11-17), presumably via cargo-culting. This patch fixes all three to just directly use the value provided to the callback. While I was adjusting the code to do so, I noticed that push.gpgsign is overly careful about a NULL value. After git_parse_maybe_bool() has returned anything besides 1, we know that the value cannot be NULL (if it were, it would be an implicit "true", and many callers of maybe_bool rely on that). Here that lets us shorten "if (v && !strcasecmp(v, ...))" to just "if (!strcasecmp(v, ...))". Signed-off-by: Jeff King Signed-off-by: Junio C Hamano