git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Prathamesh Chavan <pc44800@gmail.com>
Cc: git@vger.kernel.org, christian.couder@gmail.com, sbeller@google.com
Subject: Re: [PATCH v2 2/2] submodule: port submodule subcommand 'deinit' from shell to C
Date: Thu, 11 Jan 2018 12:48:11 -0800	[thread overview]
Message-ID: <xmqqh8rsxgtw.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <20180111201721.25930-3-pc44800@gmail.com> (Prathamesh Chavan's message of "Fri, 12 Jan 2018 01:47:21 +0530")

Prathamesh Chavan <pc44800@gmail.com> writes:

> +	/* remove the submodule work tree (unless the user already did it) */
> +	if (is_directory(path)) {
> +		struct strbuf sb_rm = STRBUF_INIT;
> +		const char *format;
> +
> +		/*
> +		 * protect submodules containing a .git directory
> +		 * NEEDSWORK: instead of dying, automatically call
> +		 * absorbgitdirs and (possibly) warn.
> +		 */
> +		if (is_directory(sub_git_dir))
> +			die(_("Submodule work tree '%s' contains a .git "
> +			      "directory (use 'rm -rf' if you really want "
> +			      "to remove it including all of its history)"),
> +			    displaypath);
> +
> +		if (!(flags & OPT_FORCE)) {
> +			struct child_process cp_rm = CHILD_PROCESS_INIT;
> +			cp_rm.git_cmd = 1;
> +			argv_array_pushl(&cp_rm.args, "rm", "-qn",
> +					 path, NULL);
> +
> +			if (run_command(&cp_rm))
> +				die(_("Submodule work tree '%s' contains local "
> +				      "modifications; use '-f' to discard them"),
> +				      displaypath);
> +		}
> +
> +		strbuf_addstr(&sb_rm, path);
> +
> +		if (!remove_dir_recursively(&sb_rm, 0))
> +			format = _("Cleared directory '%s'\n");
> +		else
> +			format = _("Could not remove submodule work tree '%s'\n");
> +
> +		if (!(flags & OPT_QUIET))
> +			printf(format, displaypath);
> +
> +		strbuf_release(&sb_rm);
> +	}
> +
> +	if (mkdir(path, 0777))
> +		die_errno(_("could not create empty submodule directory %s"),
> +		      displaypath);

If path was a directory (which presumably is the normal case) and
recursive removal fails (i.e. when the code says "Could not remove"),
this mkdir() would also fail with EEXIST.

In such a case, the original code did not die and instead continued
to remove the entries for the submodule from the configuration.
This "rewritten" version dies, leaving the stale configuration for
the submodule we failed to get rid of from the working tree.

I offhand do not know which one of these error case behaviours is
more useful; the user needs to do something (e.g. loosening the perm
in some paths in the submodule that prevented "rm -rf" from working
with "chmod u+w sub/some/path" and removing it manually) to recover
in either case, and cleaning as much as possible by removing the
configuration entries even when this mkdir() fails would probably be
a better behaviour, as long as the command as a whole exits with non
zero status to signal an error.

  reply	other threads:[~2018-01-11 20:48 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-01-09 17:57 [PATCH v1 0/2] Incremental rewrite of git-submodules Prathamesh Chavan
2018-01-09 17:57 ` [PATCH v1 1/2] submodule: port submodule subcommand 'sync' from shell to C Prathamesh Chavan
2018-01-09 20:57   ` Junio C Hamano
2018-01-09 17:57 ` [PATCH v1 2/2] submodule: port submodule subcommand 'deinit' " Prathamesh Chavan
2018-01-09 21:24   ` Junio C Hamano
2018-01-10 20:22     ` Prathamesh Chavan
2018-01-10 21:47       ` Junio C Hamano
2018-01-09 19:25 ` [PATCH v1 0/2] Incremental rewrite of git-submodules Stefan Beller
2018-01-09 20:06 ` Brandon Williams
2018-01-11 20:17 ` [PATCH v2 " Prathamesh Chavan
2018-01-11 20:17   ` [PATCH v2 1/2] submodule: port submodule subcommand 'sync' from shell to C Prathamesh Chavan
2018-01-11 20:31     ` Junio C Hamano
2018-01-11 20:17   ` [PATCH v2 2/2] submodule: port submodule subcommand 'deinit' " Prathamesh Chavan
2018-01-11 20:48     ` Junio C Hamano [this message]
2018-01-11 20:37   ` [PATCH v2 0/2] Incremental rewrite of git-submodules Junio C Hamano
2018-01-14 21:15   ` [PATCH v3 " Prathamesh Chavan
2018-01-14 21:15     ` [PATCH v3 1/2] submodule: port submodule subcommand 'sync' from shell to C Prathamesh Chavan
2018-01-14 21:15     ` [PATCH v3 2/2] submodule: port submodule subcommand 'deinit' " Prathamesh Chavan
2018-01-16 19:32     ` [PATCH v3 0/2] Incremental rewrite of git-submodules Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqh8rsxgtw.fsf@gitster.mtv.corp.google.com \
    --to=gitster@pobox.com \
    --cc=christian.couder@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=pc44800@gmail.com \
    --cc=sbeller@google.com \
    --subject='Re: [PATCH v2 2/2] submodule: port submodule subcommand '\''deinit'\'' from shell to C' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).