Re: [PATCH v3 4/4] submodule_needs_pushing() NEEDSWORK when we can not answer this question
From: Stefan Beller <hidden>
Date: 2016-11-15 22:39:35
On Tue, Nov 15, 2016 at 6:56 AM, Heiko Voigt [off-list ref] wrote:
quoted hunk ↗ jump to hunk
Signed-off-by: Heiko Voigt <redacted> --- submodule.c | 8 ++++++++ 1 file changed, 8 insertions(+)diff --git a/submodule.c b/submodule.c index e1196fd..29efee9 100644 --- a/submodule.c +++ b/submodule.c@@ -531,6 +531,14 @@ static int submodule_has_commits(const char *path, struct sha1_array *commits) static int submodule_needs_pushing(const char *path, struct sha1_array *commits) { if (!submodule_has_commits(path, commits)) + /* NEEDSWORK: The correct answer here is "We do not
style nit: /* * Usually we prefer comments with both the first and last line of the comment "empty". */ /* or just a one liner */ AFAICT these are the only two modes that we prefer in Git. For a discussion of all the other style, enjoy Linus' guidance. ;) http://lkml.iu.edu/hypermail/linux/kernel/1607.1/00627.html
"We do not know" ...
... because there is no way to check for us as we don't have the
submodule commits.
" We do consider it safe as no one in their sane mind would
have changed the submodule pointers without having the
submodule around. If a user did however change the submodules
without having the submodule commits around, this indicates an
expert who knows what they were doing."
We currently
+ * proceed pushing here as if the submodules commits are
+ * available on a remote. Since we can not check the
+ * remote availability for this submodule we should
+ * consider changing this behavior to: Stop here and
+ * tell the user how to skip this check if wanted.
+ */
return 0;Thanks for adding the NEEDSWORK, I just wrote the above lines to clarify my thought process, not as a suggestion for change. Overall the series looks good to me; the nits are minor IMHO. Thanks, Stefan