Re: [PATCH] Drop some dashes from built-in invocations in scripts

[Michael Forney]

On 8/5/17, Junio C Hamano  wrote:
> Have you made sure that all of these scripts, before calling
> 'git-foo' in the current code, update their PATH so that these found
> in the bog standard place (i.e. GIT_EXEC_PATH)?
>
> The reason I ask is because we can rest assured these changes will
> be a no-regression improvement if you did so.  I do not offhand
> think of a reason why these scripts wouldn't be doing so, but it
> never hurts to make sure.

I just checked and all the scripts make some other call to a built-in
with `git foo`, so I think it should be safe.

Re: [PATCH] Drop some dashes from built-in invocations in scripts

[Michael Forney]

On 8/5/17, Junio C Hamano <gits...@pobox.com> wrote:
> Michael Forney <mfor...@mforney.org> writes:
>> On 8/5/17, Junio C Hamano <gits...@pobox.com> wrote:
>>> Have you made sure that all of these scripts, before calling
>>> 'git-foo' in the current code, update their PATH so that these found
>>> in the bog standard place (i.e. GIT_EXEC_PATH)?
>>>
>>> The reason I ask is because we can rest assured these changes will
>>> be a no-regression improvement if you did so.  I do not offhand
>>> think of a reason why these scripts wouldn't be doing so, but it
>>> never hurts to make sure.
>>
>> I just checked and all the scripts make some other call to a built-in
>> with `git foo`, so I think it should be safe.
>
> As long as they are the same "foo"'s, then the check you did is
> perfectly fine.  The (unlikely I would think) case that can lead to
> a regression is if these script deliberately used `git-foo` to find
> them on $PATH, which can be different from 'git foo' that is found
> by 'git' in its own binary (as all of them are built-ins), and that
> is why I asked.

Ah. Well, it looks like all but git-merge-resolve.sh run `.
git-sh-setup`, so we know that GIT_EXEC_PATH must in their PATH (and
at the front unless the script was invoked directly).

git-merge-resolve.sh does not do this, so I suppose if the user ran
$GIT_EXEC_PATH/git-merge-resolve directly, and also had a custom
git-merge-index executable in their PATH, that would switch to running
the git merge-index built-in instead.

[PATCH] Drop some dashes from built-in invocations in scripts

[Michael Forney]

This way, they still work even if the built-in symlinks aren't
installed.

Signed-off-by: Michael Forney <mfor...@mforney.org>
---
It looks like there was an effort to do this a number of years ago (through
`make remove-dashes`). These are just a few I noticed were still left in the
.sh scripts.

 git-merge-octopus.sh  | 2 +-
 git-merge-one-file.sh | 8 
 git-merge-resolve.sh  | 2 +-
 git-stash.sh  | 2 +-
 git-submodule.sh  | 6 +++---
 5 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/git-merge-octopus.sh b/git-merge-octopus.sh
index bcf0d92ec..6c390d6c2 100755
--- a/git-merge-octopus.sh
+++ b/git-merge-octopus.sh
@@ -100,7 +100,7 @@ do
if test $? -ne 0
then
gettextln "Simple merge did not work, trying automatic merge."
-   git-merge-index -o git-merge-one-file -a ||
+   git merge-index -o git-merge-one-file -a ||
OCTOPUS_FAILURE=1
next=$(git write-tree 2>/dev/null)
fi
diff --git a/git-merge-one-file.sh b/git-merge-one-file.sh
index 424b034e3..9879c5939 100755
--- a/git-merge-one-file.sh
+++ b/git-merge-one-file.sh
@@ -115,16 +115,16 @@ case "${1:-.}${2:-.}${3:-.}" in
;;
esac
 
-   src1=$(git-unpack-file $2)
-   src2=$(git-unpack-file $3)
+   src1=$(git unpack-file $2)
+   src2=$(git unpack-file $3)
case "$1" in
'')
echo "Added $4 in both, but differently."
-   orig=$(git-unpack-file e69de29bb2d1d6434b8b29ae775ad8c2e48c5391)
+   orig=$(git unpack-file e69de29bb2d1d6434b8b29ae775ad8c2e48c5391)
;;
*)
echo "Auto-merging $4"
-   orig=$(git-unpack-file $1)
+   orig=$(git unpack-file $1)
;;
esac
 
diff --git a/git-merge-resolve.sh b/git-merge-resolve.sh
index c9da747fc..343fe7bcc 100755
--- a/git-merge-resolve.sh
+++ b/git-merge-resolve.sh
@@ -45,7 +45,7 @@ then
exit 0
 else
echo "Simple merge failed, trying Automatic merge."
-   if git-merge-index -o git-merge-one-file -a
+   if git merge-index -o git-merge-one-file -a
then
exit 0
else
diff --git a/git-stash.sh b/git-stash.sh
index 9b6c2da7b..9aa09c3a3 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -573,7 +573,7 @@ apply_stash () {
 
if test -n "$u_tree"
then
-   GIT_INDEX_FILE="$TMPindex" git-read-tree "$u_tree" &&
+   GIT_INDEX_FILE="$TMPindex" git read-tree "$u_tree" &&
GIT_INDEX_FILE="$TMPindex" git checkout-index --all &&
rm -f "$TMPindex" ||
die "$(gettext "Could not restore untracked files from stash 
entry")"
diff --git a/git-submodule.sh b/git-submodule.sh
index e131760ee..ffa2d6648 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -864,7 +864,7 @@ cmd_summary() {
test $status != A && test $ignore_config = all 
&& continue
fi
# Also show added or modified modules which are checked 
out
-   GIT_DIR="$sm_path/.git" git-rev-parse --git-dir 
>/dev/null 2>&1 &&
+   GIT_DIR="$sm_path/.git" git rev-parse --git-dir 
>/dev/null 2>&1 &&
printf '%s\n' "$sm_path"
done
)
@@ -898,11 +898,11 @@ cmd_summary() {
missing_dst=
 
test $mod_src = 16 &&
-   ! GIT_DIR="$name/.git" git-rev-parse -q --verify $sha1_src^0 
>/dev/null &&
+   ! GIT_DIR="$name/.git" git rev-parse -q --verify $sha1_src^0 
>/dev/null &&
missing_src=t
 
test $mod_dst = 16 &&
-   ! GIT_DIR="$name/.git" git-rev-parse -q --verify $sha1_dst^0 
>/dev/null &&
+   ! GIT_DIR="$name/.git" git rev-parse -q --verify $sha1_dst^0 
>/dev/null &&
missing_dst=t
 
display_name=$(git submodule--helper relative-path "$name" 
"$wt_prefix")
-- 
2.13.3

Re: [PATCH] Drop some dashes from built-in invocations in scripts

[Michael Forney]

On 8/7/17, Junio C Hamano  wrote:
> Just to avoid possible confusion, the above is not to say "once it
> is decided, you are not allowed to bring fresh arguments to the
> discussion".  As Peff said [*2*] in that old discussion thread, the
> circumstances may have changed over 9 years, and it may benefit to
> revisit some old decisions.
>
> So in that sense, I do not mind somebody makes a fresh proposal,
> which would probably be similar to what I did back then in [*3*],
> which is at the beginning of that old thread.  But I do not think I
> would be doing so myself, and I suspect that I would not be very
> supportive for such a proposal, because my gut feeling is that the
> upside is not big enough compared to downsides.
>
> The obvious upside is that you do not have to have many built-in
> commands on the filesystem, either as a hardlink, a copy, or a
> symbolic link.  But we will be breaking people's scripts by breaking
> the 11-year old promise that we will keep their "git-foo" working as
> long as they prepend $GIT_EXEC_PATH to their $PATH we we did so.
> Another downside is that we now will expose which subcommands are
> built-in and which are not, which is unnecessarily implementation
> detail we'd want end-users to rely on.
>
> The "'git co' may stop working" I mentioned in my previous message
> is not counted as a downside---if the upside is large enough, I
> think that "we respawn git-foo as dashed built-in when running an
> alias that expands to 'foo'" can be fixed to respawn "git foo"
> instead of "git-foo".  But there may be other downside that we may
> not be able to fix, and I suspect that "we'd be breaking the 11-year
> old promise" is something we would not be able to fix.  That is why
> I doubt that I would be advocating the removal of "git-foo" from the
> filesystem myself.

Thanks for the history and explanation, Junio. I agree with your analysis.

I didn't know that git aliases invoke the `git-foo` path for built-ins
(I don't use them much myself, so didn't notice).

I also didn't know that it was supported to add GIT_EXEC_DIR to your
PATH to be able to call `git-foo`. I generally think of /libexec as
implementation-specific executables that a tool may call internally
(in that sense, whether or not the commands are built-ins would remain
an implementation-detail).

However, I still think the patch should be applied for
self-consistency at least (git-submodule.sh currently calls both `git
rev-parse` and `git-rev-parse`). Also, based on Johannes' reply, it
may still be useful for git-for-windows.

>
> [References]
>
> *1*
> https://public-inbox.org/git/alpine.lfd.1.10.0808261114070.3...@nehalem.linux-foundation.org/
>
> *2*
> https://public-inbox.org/git/20080826145719.gb5...@coredump.intra.peff.net/
>
> *3* https://public-inbox.org/git/7vprnzt7d5@gitster.siamese.dyndns.org/

Confusing behavior with ignored submodules and `git commit -a`

[Michael Forney]

Hi,

In the past few months have noticed some confusing behavior with
ignored submodules. I finally got around to bisecting this to commit
5556808690ea245708fb80383be5c1afee2fb3eb (add, reset: ensure
submodules can be added or reset).

Here is a demonstration of the problem:

First some repository initialization with a submodule marked as ignored.

$ git init inner && git -C inner commit --allow-empty -m 'inner 1'
Initialized empty Git repository in /tmp/inner/.git/
[master (root-commit) ef55bed] inner 1
$ git init outer && cd outer
Initialized empty Git repository in /tmp/outer/.git/
$ git submodule add ../inner
Cloning into '/tmp/outer/inner'...
done.
$ echo 1 > foo.txt && git add foo.txt
$ git commit -m 'outer 1'
[master (root-commit) efeb85c] outer 1
 3 files changed, 5 insertions(+)
 create mode 100644 .gitmodules
 create mode 100644 foo.txt
 create mode 16 inner
$ git config submodule.inner.ignore all
$ git -C inner commit --allow-empty -m 'inner 2'
[master 7b7f0fa] inner 2
$ git status
On branch master
nothing to commit, working tree clean
$

Up to here is all expected. However, if I go to update `foo.txt` and
commit with `git commit -a`, changes to inner get recorded
unexpectedly. What's worse is the shortstat output of `git commit -a`,
and the diff output of `git show` give no indication that the
submodule was changed.

$ echo 2 > foo.txt
$ git status
On branch master
Changes not staged for commit:
  (use "git add ..." to update what will be committed)
  (use "git checkout -- ..." to discard changes in working directory)

modified:   foo.txt

no changes added to commit (use "git add" and/or "git commit -a")
$ git commit -a -m 'update foo.txt'
[master 6ec564c] update foo.txt
 1 file changed, 1 insertion(+), 1 deletion(-)
$ git show
commit 6ec564c15ddae099c71f01750b4c434557525653 (HEAD -> master)
Author: Michael Forney <mfor...@mforney.org>
Date:   Fri Mar 16 20:18:37 2018 -0700

update foo.txt

diff --git a/foo.txt b/foo.txt
index d00491f..0cfbf08 100644
--- a/foo.txt
+++ b/foo.txt
@@ -1 +1 @@
-1
+2
$

There have been a couple occasions where I accidentally pushed local
changes to ignored submodules because of this. Since they don't show
up in the log output, it is difficult to figure out what actually has
gone wrong.

Anyway, since the bisected commit (555680869) only mentions add and
reset, I'm assuming that this is a regression and not a deliberate
behavior change. The documentation for submodule..ignore states
that the setting should only affect `git status` and the diff family.
In terms of my expectations, I would go further and say it should only
affect `git status` and diffs against the working tree.

I took a brief look through the relevant sources, and it wasn't clear
to me how to fix this without accidentally changing the behavior of
other subcommands.

Any help with this issue is appreciated!

Re: Confusing behavior with ignored submodules and `git commit -a`

[Michael Forney]

On 2018-03-16, Michael Forney  wrote:
> Hi,
>
> In the past few months have noticed some confusing behavior with
> ignored submodules. I finally got around to bisecting this to commit
> 5556808690ea245708fb80383be5c1afee2fb3eb (add, reset: ensure
> submodules can be added or reset).
>
> Here is a demonstration of the problem:
>
> First some repository initialization with a submodule marked as ignored.
>
> $ git init inner && git -C inner commit --allow-empty -m 'inner 1'
> Initialized empty Git repository in /tmp/inner/.git/
> [master (root-commit) ef55bed] inner 1
> $ git init outer && cd outer
> Initialized empty Git repository in /tmp/outer/.git/
> $ git submodule add ../inner
> Cloning into '/tmp/outer/inner'...
> done.
> $ echo 1 > foo.txt && git add foo.txt
> $ git commit -m 'outer 1'
> [master (root-commit) efeb85c] outer 1
>  3 files changed, 5 insertions(+)
>  create mode 100644 .gitmodules
>  create mode 100644 foo.txt
>  create mode 16 inner
> $ git config submodule.inner.ignore all
> $ git -C inner commit --allow-empty -m 'inner 2'
> [master 7b7f0fa] inner 2
> $ git status
> On branch master
> nothing to commit, working tree clean
> $
>
> Up to here is all expected. However, if I go to update `foo.txt` and
> commit with `git commit -a`, changes to inner get recorded
> unexpectedly. What's worse is the shortstat output of `git commit -a`,
> and the diff output of `git show` give no indication that the
> submodule was changed.
>
> $ echo 2 > foo.txt
> $ git status
> On branch master
> Changes not staged for commit:
>   (use "git add ..." to update what will be committed)
>   (use "git checkout -- ..." to discard changes in working directory)
>
> modified:   foo.txt
>
> no changes added to commit (use "git add" and/or "git commit -a")
> $ git commit -a -m 'update foo.txt'
> [master 6ec564c] update foo.txt
>  1 file changed, 1 insertion(+), 1 deletion(-)
> $ git show
> commit 6ec564c15ddae099c71f01750b4c434557525653 (HEAD -> master)
> Author: Michael Forney 
> Date:   Fri Mar 16 20:18:37 2018 -0700
>
> update foo.txt
>
> diff --git a/foo.txt b/foo.txt
> index d00491f..0cfbf08 100644
> --- a/foo.txt
> +++ b/foo.txt
> @@ -1 +1 @@
> -1
> +2
> $
>
> There have been a couple occasions where I accidentally pushed local
> changes to ignored submodules because of this. Since they don't show
> up in the log output, it is difficult to figure out what actually has
> gone wrong.
>
> Anyway, since the bisected commit (555680869) only mentions add and
> reset, I'm assuming that this is a regression and not a deliberate
> behavior change. The documentation for submodule..ignore states
> that the setting should only affect `git status` and the diff family.
> In terms of my expectations, I would go further and say it should only
> affect `git status` and diffs against the working tree.
>
> I took a brief look through the relevant sources, and it wasn't clear
> to me how to fix this without accidentally changing the behavior of
> other subcommands.
>
> Any help with this issue is appreciated!

I accidentally pushed local changes to ignored submodules again due to this.

Can anyone confirm whether this is the intended behavior of ignore? If
it is, then at least the documentation needs an update saying that
`commit -a` will commit all submodule changes, even if they are
ignored.

Re: Confusing behavior with ignored submodules and `git commit -a`

[Michael Forney]

On 2018-11-15, Stefan Beller  wrote:
> On Wed, Nov 14, 2018 at 10:05 PM Michael Forney 
> wrote:
>> Looking at ff6f1f564c, I don't really see anything that might be
>> related to git-add, git-reset, or git-diff, so I'm guessing that this
>> only worked before because the submodule config wasn't getting loaded
>> during `git add` or `git reset`. Now that the config is loaded
>> automatically, submodule..ignore started taking effect where it
>> shouldn't.
>>
>> Unfortunately, this doesn't really get me much closer to finding a fix.
>
> Maybe selectively unloading or overwriting the config?
>
> Or we can change is_submodule_ignored() in diff.c
> to be only applied selectively whether we are running the
> right command? For this approach we'd have to figure out the
> set of commands to which the ignore config should apply or
> not (and come up with a more concise documentation then)
>
> This approach sounds appealing to me as it would cover
> new commands as well and we'd only have a central point
> where the decision for ignoring is made.

Well, currently the submodule config can be disabled in diff_flags by
setting override_submodule_config=1. However, I'm thinking it may be
simpler to selectively *enable* the submodule config in diff_flags
where it is needed instead of disabling it everywhere else (i.e.
use_submodule_config instead of override_submodule_config).

I'm also starting to see why this is tricky. The only difference that
diff.c:run_diff_files sees between `git add inner` and `git add --all`
is whether the index entry matched the pathspec exactly or not.

Here is a work-in-progress diff that seems to have the correct
behavior in all cases I tried. Can you think of any cases that it
breaks? I'm not quite sure of the consequences of having diff_change
and diff_addremove always ignore the submodule config; git-diff and
git-status still seem to work correctly.

diff --git a/builtin/add.c b/builtin/add.c
index f65c17229..9902f7742 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -117,7 +117,6 @@ int add_files_to_cache(const char *prefix,
rev.diffopt.output_format = DIFF_FORMAT_CALLBACK;
rev.diffopt.format_callback = update_callback;
rev.diffopt.format_callback_data = 
-   rev.diffopt.flags.override_submodule_config = 1;
rev.max_count = 0; /* do not compare unmerged paths with stage #2 */
run_diff_files(, DIFF_RACY_IS_MODIFIED);
clear_pathspec(_data);
diff --git a/diff-lib.c b/diff-lib.c
index 83fce5151..fbb048cca 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -68,12 +68,13 @@ static int check_removed(const struct cache_entry
*ce, struct stat *st)
 static int match_stat_with_submodule(struct diff_options *diffopt,
 const struct cache_entry *ce,
 struct stat *st, unsigned ce_option,
-unsigned *dirty_submodule)
+unsigned *dirty_submodule,
+int exact)
 {
int changed = ie_match_stat(diffopt->repo->index, ce, st, ce_option);
if (S_ISGITLINK(ce->ce_mode)) {
struct diff_flags orig_flags = diffopt->flags;
-   if (!diffopt->flags.override_submodule_config)
+   if (!diffopt->flags.override_submodule_config && !exact)
set_diffopt_flags_from_submodule_config(diffopt, 
ce->name);
if (diffopt->flags.ignore_submodules)
changed = 0;
@@ -88,7 +89,7 @@ static int match_stat_with_submodule(struct
diff_options *diffopt,

 int run_diff_files(struct rev_info *revs, unsigned int option)
 {
-   int entries, i;
+   int entries, i, matched;
int diff_unmerged_stage = revs->max_count;
unsigned ce_option = ((option & DIFF_RACY_IS_MODIFIED)
  ? CE_MATCH_RACY_IS_DIRTY : 0);
@@ -110,7 +111,8 @@ int run_diff_files(struct rev_info *revs, unsigned
int option)
if (diff_can_quit_early(>diffopt))
break;

-   if (!ce_path_match(istate, ce, >prune_data, NULL))
+   matched = ce_path_match(istate, ce, >prune_data, NULL);
+   if (!matched)
continue;

if (ce_stage(ce)) {
@@ -226,7 +228,8 @@ int run_diff_files(struct rev_info *revs, unsigned
int option)
}

changed = match_stat_with_submodule(>diffopt, ce, 
,
-   ce_option, 
_submodule);
+   ce_option, 
_submodule,
+   matched == 
MATCHED_EXACTLY);
newmode = ce_mode_from_stat(ce,

Re: Confusing behavior with ignored submodules and `git commit -a`

[Michael Forney]

On 2018-11-15, Michael Forney  wrote:
> Here is a work-in-progress diff that seems to have the correct
> behavior in all cases I tried.

I was hoping that gmail wouldn't mess with the whitespace, but apparently
it has, sorry about that. Let me try again.

---
 builtin/add.c |  1 -
 diff-lib.c| 15 +--
 diff.c| 22 ++
 3 files changed, 11 insertions(+), 27 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index f65c17229..9902f7742 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -117,7 +117,6 @@ int add_files_to_cache(const char *prefix,
rev.diffopt.output_format = DIFF_FORMAT_CALLBACK;
rev.diffopt.format_callback = update_callback;
rev.diffopt.format_callback_data = 
-   rev.diffopt.flags.override_submodule_config = 1;
rev.max_count = 0; /* do not compare unmerged paths with stage #2 */
run_diff_files(, DIFF_RACY_IS_MODIFIED);
clear_pathspec(_data);
diff --git a/diff-lib.c b/diff-lib.c
index 83fce5151..fbb048cca 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -68,12 +68,13 @@ static int check_removed(const struct cache_entry *ce, 
struct stat *st)
 static int match_stat_with_submodule(struct diff_options *diffopt,
 const struct cache_entry *ce,
 struct stat *st, unsigned ce_option,
-unsigned *dirty_submodule)
+unsigned *dirty_submodule,
+int exact)
 {
int changed = ie_match_stat(diffopt->repo->index, ce, st, ce_option);
if (S_ISGITLINK(ce->ce_mode)) {
struct diff_flags orig_flags = diffopt->flags;
-   if (!diffopt->flags.override_submodule_config)
+   if (!diffopt->flags.override_submodule_config && !exact)
set_diffopt_flags_from_submodule_config(diffopt, 
ce->name);
if (diffopt->flags.ignore_submodules)
changed = 0;
@@ -88,7 +89,7 @@ static int match_stat_with_submodule(struct diff_options 
*diffopt,
 
 int run_diff_files(struct rev_info *revs, unsigned int option)
 {
-   int entries, i;
+   int entries, i, matched;
int diff_unmerged_stage = revs->max_count;
unsigned ce_option = ((option & DIFF_RACY_IS_MODIFIED)
  ? CE_MATCH_RACY_IS_DIRTY : 0);
@@ -110,7 +111,8 @@ int run_diff_files(struct rev_info *revs, unsigned int 
option)
if (diff_can_quit_early(>diffopt))
break;
 
-   if (!ce_path_match(istate, ce, >prune_data, NULL))
+   matched = ce_path_match(istate, ce, >prune_data, NULL);
+   if (!matched)
continue;
 
if (ce_stage(ce)) {
@@ -226,7 +228,8 @@ int run_diff_files(struct rev_info *revs, unsigned int 
option)
}
 
changed = match_stat_with_submodule(>diffopt, ce, 
,
-   ce_option, 
_submodule);
+   ce_option, 
_submodule,
+   matched == 
MATCHED_EXACTLY);
newmode = ce_mode_from_stat(ce, st.st_mode);
}
 
@@ -292,7 +295,7 @@ static int get_stat_data(const struct cache_entry *ce,
return -1;
}
changed = match_stat_with_submodule(diffopt, ce, ,
-   0, dirty_submodule);
+   0, dirty_submodule, 0);
if (changed) {
mode = ce_mode_from_stat(ce, st.st_mode);
oid = _oid;
diff --git a/diff.c b/diff.c
index e38d1ecaf..73dc75286 100644
--- a/diff.c
+++ b/diff.c
@@ -6209,24 +6209,6 @@ int diff_can_quit_early(struct diff_options *opt)
opt->flags.has_changes);
 }
 
-/*
- * Shall changes to this submodule be ignored?
- *
- * Submodule changes can be configured to be ignored separately for each path,
- * but that configuration can be overridden from the command line.
- */
-static int is_submodule_ignored(const char *path, struct diff_options *options)
-{
-   int ignored = 0;
-   struct diff_flags orig_flags = options->flags;
-   if (!options->flags.override_submodule_config)
-   set_diffopt_flags_from_submodule_config(options, path);
-   if (options->flags.ignore_submodules)
-   ignored = 1;
-   options->flags = orig_flags;
-   return ignored;
-}
-
 void diff_addremove(struct diff_options *options,
int addremove, unsigned mode,
const struct object_id *oid,
@@ -6235,7 +6217,7 @@ void diff_addremove(struct diff_options *option

Re: Confusing behavior with ignored submodules and `git commit -a`

[Michael Forney]

On 2018-11-15, Stefan Beller  wrote:
> On Thu, Nov 15, 2018 at 1:33 PM Michael Forney  wrote:
>> Well, currently the submodule config can be disabled in diff_flags by
>> setting override_submodule_config=1. However, I'm thinking it may be
>> simpler to selectively *enable* the submodule config in diff_flags
>> where it is needed instead of disabling it everywhere else (i.e.
>> use_submodule_config instead of override_submodule_config).
>
> This sounds like undoing the good(?) part of the series that introduced
> this regression, as before that we selectively loaded the submodule
> config, which lead to confusion when you forgot it. Selectively *enabling*
> the submodule config sounds like that state before?
>
> Or do we *only* talk about enabling the ignore flag, while loading the
> rest of the submodule config automatic?

Yes, that is what I meant. I believe the automatic loading of
submodule config is the right thing to do, it just uncovered cases
where the current handling of override_submodule_config is not quite
sufficient.

My suggestion of replacing override_submodule_config with
use_submodule_config is because it seems like there are fewer places
where we want to apply the ignore config than not. I think it should
only apply in diffs against the working tree and when staging changes
to the index (unless specified explicitly). The documentation just
mentions the "diff family", but all but one of the possible values for
submodule..ignore ("all") don't make sense unless comparing with
the working tree. This is also how show/log -p behaved in git <2.15.
So I think that clarifying that it is about modifications *to the
working tree* would be a good idea.

>> I'm also starting to see why this is tricky. The only difference that
>> diff.c:run_diff_files sees between `git add inner` and `git add --all`
>> is whether the index entry matched the pathspec exactly or not.
>
> Unrelated to the trickiness, I think we'd need to document the behavior
> of the -a flag in git-add and git-commit better as adding the diff below
> will depart from the "all" rule again, which I thought was a strong
> motivator for Brandons series (IIRC).

Can you explain what you mean by the "all" rule?

>> Here is a work-in-progress diff that seems to have the correct
>> behavior in all cases I tried. Can you think of any cases that it
>> breaks? I'm not quite sure of the consequences of having diff_change
>> and diff_addremove always ignore the submodule config; git-diff and
>> git-status still seem to work correctly.
>>
>> diff --git a/builtin/add.c b/builtin/add.c
>> index f65c17229..9902f7742 100644
>> --- a/builtin/add.c
>> +++ b/builtin/add.c
>> @@ -117,7 +117,6 @@ int add_files_to_cache(const char *prefix,
>> rev.diffopt.output_format = DIFF_FORMAT_CALLBACK;
>> rev.diffopt.format_callback = update_callback;
>> rev.diffopt.format_callback_data = 
>> -   rev.diffopt.flags.override_submodule_config = 1;
>
> This line partially reverts 5556808, taking 02f2f56bc377c28
> into account.

Correct. The problem with 55568086 is that add_files_to_cache is used
by both git-add and git-commit, regardless of whether --all was
specified. So, while it made it possible to stage ignored submodules,
it also made it so the submodule ignore config was overridden in all
uses of git-add and git-commit.

So, this diff attempts to make it so the ignore config is only applied
when the pathspec matches exactly rather than just always overriding
the ignore config.

>> diff --git a/diff-lib.c b/diff-lib.c
>> index 83fce5151..fbb048cca 100644
>> --- a/diff-lib.c
>> +++ b/diff-lib.c
>> @@ -68,12 +68,13 @@ static int check_removed(const struct cache_entry
>> *ce, struct stat *st)
>>  static int match_stat_with_submodule(struct diff_options *diffopt,
>>  const struct cache_entry *ce,
>>  struct stat *st, unsigned ce_option,
>> -unsigned *dirty_submodule)
>> +unsigned *dirty_submodule,
>> +int exact)
>> [...];
>
> This is an interesting take so far as it is all about *detecting* change
> here via stat information and not like the previous (before the regression)
> where it was about correcting output.
>
> match_stat_with_submodule would grow its documentation to be
> slightly more complicated as a result.

Yes, this is one part I'm not quite happy with. I wonder if instead
match_stat_with_submodule could be made simpler by moving the
ie_match_stat call up to the two call sites, and then it could be
selectively called by run_diff_fi

Re: Confusing behavior with ignored submodules and `git commit -a`

[Michael Forney]

+bmwill

On 2018-11-14, Michael Forney  wrote:
> On 2018-10-25, Stefan Beller  wrote:
>> I guess reverting that commit is not a good idea now, as
>> I would expect something to break.
>>
>> Maybe looking through the series 614ea03a71
>> (Merge branch 'bw/submodule-config-cleanup', 2017-08-26)
>> to understand why it happened in the context would be a good start.
>
> Thanks, that's a good idea. I'll take a look through that series.

Interesting. If I build git from master after reverting 55568086, I do
indeed observe the issue it claims to fix (unable to add ignored
submodules). However, if I build from 9ef23f91fc (the immediate parent
of 55568086), I do not see the issue.

Investigating this further, it seems that 55568086 addresses an issue
that does not appear until later on in the series in ff6f1f564c
(submodule-config: lazy-load a repository's .gitmodules file). Perhaps
this was a result of reordering commits during a rebase. In other
words, I get correct behavior until 55568086, and in
55568086..ff6f1f564c^ if I revert 55568086.

Looking at ff6f1f564c, I don't really see anything that might be
related to git-add, git-reset, or git-diff, so I'm guessing that this
only worked before because the submodule config wasn't getting loaded
during `git add` or `git reset`. Now that the config is loaded
automatically, submodule..ignore started taking effect where it
shouldn't.

Unfortunately, this doesn't really get me much closer to finding a fix.

Re: Confusing behavior with ignored submodules and `git commit -a`

[Michael Forney]

On 2018-10-25, Stefan Beller  wrote:
> On Thu, Oct 25, 2018 at 11:03 AM Michael Forney 
> wrote:
>>
>> On 2018-03-16, Michael Forney  wrote:
>> > Hi,
>> >
>> > In the past few months have noticed some confusing behavior with
>> > ignored submodules. I finally got around to bisecting this to commit
>> > 5556808690ea245708fb80383be5c1afee2fb3eb (add, reset: ensure
>> > submodules can be added or reset).
>
> Uh. :(
>
> See the discussion starting at
> https://public-inbox.org/git/20170725213928.125998-4-bmw...@google.com/
> specifically
> https://public-inbox.org/git/xmqqinieq49v@gitster.mtv.corp.google.com/

Thanks for the links. Let me explain how I'm using
submodule..ignore. Maybe there's a better mechanism in git to
deal with this (if .ignore is a misfeature).

I have a git repository which contains a number of submodules that
refer to external repositories. Some of these repositories need to
patched in some way, so patches are stored alongside the submodules,
and are applied when building. This mostly works fine, but causes
submodules to show up as modified in `git status` and get updated with
`git commit -a`. To resolve this, I've added `ignore = all` to
.gitmodules for all the submodules that need patches applied. This
way, I can explicitly `git add` the submodule when I want to update
the base commit, but otherwise pretend that they are clean. This has
worked pretty well for me, but less so since git 2.15 when this issue
was introduced.

Of course, I could maintain and publish forks of those repositories
and use those as the remote for the submodules. However in many cases
these patches are just temporary until they get applied upstream and a
new release is made, and I don't really want to keep mirrors
unnecessarily, or keep switching the submodule URL between upstream
and my fork.

>> > However, if I go to update `foo.txt` and
>> > commit with `git commit -a`, changes to inner get recorded
>> > unexpectedly. What's worse is the shortstat output of `git commit -a`,
>> > and the diff output of `git show` give no indication that the
>> > submodule was changed.
>
> This is really bad. git-status and git-commit share some code,
> and we'll populate the commit message with a status output.
> So it seems reasonable to expect the status and the commit to match,
> i.e. if status tells me there is no change, then commit should not record
> the submodule update.

I just checked and if I don't specify a message on the command-line,
the status output in the message template *does* mention that `inner`
is getting updated.

>> > There have been a couple occasions where I accidentally pushed local
>> > changes to ignored submodules because of this. Since they don't show
>> > up in the log output, it is difficult to figure out what actually has
>> > gone wrong.
>
> How was it prevented before? Just by git commit -a not picking up the
> submodule change?

Yes. Previously, `git commit -a` would not pick up the change (unless
I added it explicitly with `git add`), and `git log` would still show
changes to ignored submodules (which is the behavior I want).

> I guess reverting that commit is not a good idea now, as
> I would expect something to break.
>
> Maybe looking through the series 614ea03a71
> (Merge branch 'bw/submodule-config-cleanup', 2017-08-26)
> to understand why it happened in the context would be a good start.

Thanks, that's a good idea. I'll take a look through that series.

>> I accidentally pushed local changes to ignored submodules again due to
>> this.
>>
>> Can anyone confirm whether this is the intended behavior of ignore? If
>> it is, then at least the documentation needs an update saying that
>> `commit -a` will commit all submodule changes, even if they are
>> ignored.
>
> The docs say "(but it will nonetheless show up in the output of
> status and commit when it has been staged)" as well, so that commit
> sounds like a regression?

I just came across someone else affected by this issue:
https://github.com/git/git/commit/55568086#commitcomment-27137460