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

Re: [PATCH v3 5/5] rm: delete .gitmodules entry of submodules removed from the work tree

From
Fredrik Gustafsson <iveqy@iveqy.com>
Date
Jul 30, 2013, 20:15 UTC
Message-ID
<20130730201511.GC19369@paksenarrion.iveqy.com>
In-Reply-To
<51F81957.1000709@web.de>
On Tue, Jul 30, 2013 at 09:51:51PM +0200, Jens Lehmann wrote:
Show 31 quoted lines
> +/*
> + * Try to remove the "submodule.<name>" section from .gitmodules where the given
> + * path is configured. Return 0 only if a .gitmodules file was found, a section
> + * with the correct path=<path> setting was found and we could remove it.
> + */
> +int remove_path_from_gitmodules(const char *path)
> +{
> +	struct strbuf sect = STRBUF_INIT;
> +	struct string_list_item *path_option;
> +
> +	if (!file_exists(".gitmodules")) /* Do nothing without .gitmodules */
> +		return -1;
> +
> +	if (gitmodules_is_unmerged)
> +		die(_("Cannot change unmerged .gitmodules, resolve merge conflicts first"));
> +
> +	path_option = unsorted_string_list_lookup(&config_name_for_path, path);
> +	if (!path_option) {
> +		warning(_("Could not find section in .gitmodules where path=%s"), path);
> +		return -1;
> +	}
> +	strbuf_addstr(&sect, "submodule.");
> +	strbuf_addstr(&sect, path_option->util);
> +	if (git_config_rename_section_in_file(".gitmodules", sect.buf, NULL) < 0) {
> +		/* Maybe the user already did that, don't error out here */
> +		warning(_("Could not remove .gitmodules entry for %s"), path);
> +		return -1;
> +	}
> +	strbuf_release(&sect);
> +	return 0;
> +}

This question applies for this function and a few more functions in this patch that has the same characteristics.

If we're in a state when we need to return non-zero, we don't do any cleaning (that is strbuf_release()). Since this file is in the part called libgit AFAIK, shouldn't we always clean after us?

Would it make sense to have different return values for different errors?

I do like the comments above the function, more functions (at least non-static ones) should follow this good style IMHO.

-- 
Med vänliga hälsningar
Fredrik Gustafsson

tel: 0733-608274
e-post: iveqy@iveqy.com
Previous: Jens LehmannNext: Jens Lehmann
Message 11 of 18 in “Teach mv to move submodules”
  1. 0/5 Teach mv to move submodulesJens Lehmann, Jul 30, 2013
  2. 1/5 Teach mv to move submodules together with their work treesJens Lehmann, Jul 30, 2013
  3. 2/5 Teach mv to move submodules using a gitfileJens Lehmann, Jul 30, 2013
  4. Fredrik GustafssonJul 31, 2013
  5. 3/5 submodule.c: add .gitmodules staging helper functionsJens Lehmann, Jul 30, 2013
  6. Junio C HamanoJul 30, 2013
  7. Jens LehmannJul 30, 2013
  8. 4/5 Teach mv to update the path entry in .gitmodules for moved submodulesJens Lehmann, Jul 30, 2013
  9. Jens LehmannAug 6, 2013
  10. 5/5 rm: delete .gitmodules entry of submodules removed from the work treeJens Lehmann, Jul 30, 2013
  11. Fredrik GustafssonJul 30, 2013
  12. Jens LehmannJul 30, 2013
  13. Jens LehmannAug 6, 2013
  14. Junio C HamanoAug 6, 2013
  15. Jens LehmannAug 7, 2013
  16. Fredrik GustafssonAug 7, 2013
  17. Jens LehmannAug 8, 2013
  18. Fredrik GustafssonAug 8, 2013

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.