Re: Re: Git issues with submodules
On Mon, Nov 25, 2013 at 12:53:45PM -0800, Junio C Hamano wrote: > Heiko Voigt writes: > > > What I think needs fixing here first is that the ignore setting should not > > apply to any diffs between HEAD and index. IMO, it should only apply > > to the diff between worktree and index. > > Hmph. How about "git diff $commit", the diff between the worktree and > a named commit (which may or may not be HEAD)? Thats an interesting question. My first thought was that I would expect it to not show submodules since it involves the worktree, but then I could also argue that it should only show differences between whats in the index and the given commit. That would make matters more complicated but I image the use case (floating submodules) involves not caring about submodules except for some integration points when submodule sha1's are explicitly recorded. I would expect to only see diffs between these integration points. But then I am not a big user (none at all at the moment) of the floating model. Cheers Heiko -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Re: Re: Git issues with submodules
On Mon, Nov 25, 2013 at 11:57:45PM +0600, Sergey Sharybin wrote: > Heiko, yeah sure see what you mean. Changing existing behavior is pretty PITA. > > Just one more question for now, are you referencing to the patch "[RFC > PATCH] disable complete ignorance of submodules for index <-> HEAD > diff"? Coz i tested it and seems it doesn't change behavior of > add/commit. Yep, that was just an RFC for status and diff. I think teaching add and commit to skip submodules if ignored are a separate topic and thus will be in a separate patch. I have to add tests and probably some more commands. The logic of ignoring submodules is implemented quite deep in the diff code. So changing it can affect quite some commands so we have to check quite carefully what will be affected and if we can change it without to much fallout. > Also, i'm around to test the all patches which are related on submodules :) Thanks, good to know. Stay tuned! Cheers Heiko -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Re: Git issues with submodules
Heiko, yeah sure see what you mean. Changing existing behavior is pretty PITA. Just one more question for now, are you referencing to the patch "[RFC PATCH] disable complete ignorance of submodules for index <-> HEAD diff"? Coz i tested it and seems it doesn't change behavior of add/commit. Also, i'm around to test the all patches which are related on submodules :) On Mon, Nov 25, 2013 at 11:49 PM, Heiko Voigt wrote: > On Mon, Nov 25, 2013 at 03:02:51PM +0600, Sergey Sharybin wrote: >> Am i right the intention is to make it so `git add .` and `git commit >> .` doesn't include changes to submodule hash unless -f argument is >> provided? > > Yes thats the goal. My patch currently only disables it when ignore is > set to all. I will add another patch that implements the -f and > --submodule-ignore option to both of them so the user has an easy way to > bypass that. But having said that we changing existing behavior here so > we have to investigate carefully whether we are not breaking peoples > expectations (and script). That also applies to the other patch > that enables showing them in diff and friends again. > > Cheers Heiko -- With best regards, Sergey Sharybin -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Re: Git issues with submodules
On Mon, Nov 25, 2013 at 03:02:51PM +0600, Sergey Sharybin wrote: > Am i right the intention is to make it so `git add .` and `git commit > .` doesn't include changes to submodule hash unless -f argument is > provided? Yes thats the goal. My patch currently only disables it when ignore is set to all. I will add another patch that implements the -f and --submodule-ignore option to both of them so the user has an easy way to bypass that. But having said that we changing existing behavior here so we have to investigate carefully whether we are not breaking peoples expectations (and script). That also applies to the other patch that enables showing them in diff and friends again. Cheers Heiko -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Re: Git issues with submodules
On Sat, Nov 23, 2013 at 09:32:45PM +0100, Jens Lehmann wrote: > Am 22.11.2013 22:54, schrieb Heiko Voigt: > > What I think needs fixing here first is that the ignore setting should not > > apply to any diffs between HEAD and index. IMO, it should only apply > > to the diff between worktree and index. > > Not only that. It should also apply to diffs between commits/trees > and work tree but not between commits/trees. The reason the ignore > setting was added three years ago was to avoid expensive work tree > operations when it was clear that either the information wasn't > wanted or it took too much time to determine that. And I doubt you > want to see modifications to submodules in your work tree when > diffing against HEAD but not when diffing against the index. > > And this behavior happens to be just what the floating branch model > needs too. I'm not sure there isn't a use case out there that also > needs to silence diff & friends regarding submodule changes between > commits/trees and/or index too (even though I cannot come up with > one at the moment). So I propose to add "worktree" as another value > for the ignore option - which ignores submodule modifications in > the work tree - and leave "all" as it is. I am not so sure about that. Only finding out what has changed (commit wise) in a submodule is expensive. Just finding out whether a submodule sha1 has changed is not expensive. Maybe we should completely stop respecting the ignore=all setting for history and diff between index and HEAD. AFAIK, we do not have any other setting that instruct git to ignore specific parts of the history unless explicitly asked for by specifying a pathspec. And I think a user should never miss by accident that something has changed in the repository. Cheers Heiko -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Re: Git issues with submodules
Hi,
On Sat, Nov 23, 2013 at 09:10:44PM +0100, Jens Lehmann wrote:
> Am 22.11.2013 23:09, schrieb Jonathan Nieder:
> > Heiko Voigt wrote:
> >
> >> After that we can discuss whether add should add submodules that are
> >> tracked but not shown. How about commit -a ? Should it also ignore the
> >> change? I am undecided here. There does not seem to be any good
> >> decision. From the users point of view we should probably not add it
> >> since its not visible in status. What do others think?
> >
> > I agree --- it should not add.
>
> I concur: adding a change that is hidden from the user during
> the process is not a good idea.
Here is a patch achieving that. Still missing a test which I will add.
Cheers Heiko
---8<
Subject: [PATCH] fix 'git add' to skip submodules configured as ignored
If submodules are configured as ignore=all they are not shown by status.
Lets also ignore them when adding files to the index. This avoids that
users accidentially add ignored submodules with: git add .
We achieve this by reading the submodule config and thus correctly
initializing the infrastructure to take the ignore decision.
Signed-off-by: Heiko Voigt
---
builtin/add.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/builtin/add.c b/builtin/add.c
index 226f758..2d0d2ef 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -15,6 +15,7 @@
#include "diffcore.h"
#include "revision.h"
#include "bulk-checkin.h"
+#include "submodule.h"
static const char * const builtin_add_usage[] = {
N_("git add [options] [--] ..."),
@@ -378,6 +379,10 @@ static int add_config(const char *var, const char *value,
void *cb)
ignore_add_errors = git_config_bool(var, value);
return 0;
}
+
+ if (!prefixcmp(var, "submodule."))
+ return parse_submodule_config_option(var, value);
+
return git_default_config(var, value, cb);
}
@@ -415,6 +420,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
int implicit_dot = 0;
struct update_callback_data update_data;
+ gitmodules_config();
git_config(add_config, NULL);
argc = parse_options(argc, argv, prefix, builtin_add_options,
--
1.8.5.rc3.1.gbe2a8c7
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Re: Git issues with submodules
Heiko Voigt wrote: > What I think needs fixing here first is that the ignore setting should not > apply to any diffs between HEAD and index. IMO, it should only apply > to the diff between worktree and index. > > When we have that the user does not see the submodule changed when > normally working. But after doing git add . the change to the submodule > should be shown in status and diff regardless of the configuration. Yeah, I think this is a good direction. > After that we can discuss whether add should add submodules that are > tracked but not shown. How about commit -a ? Should it also ignore the > change? Here, I think ignored submodules should behave like files matched by .gitignore: add should not add (`add -f` would be a good way to force it), and `commit -a` should also exclude it. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Re: Git issues with submodules
Heiko Voigt wrote: > After that we can discuss whether add should add submodules that are > tracked but not shown. How about commit -a ? Should it also ignore the > change? I am undecided here. There does not seem to be any good > decision. From the users point of view we should probably not add it > since its not visible in status. What do others think? I agree --- it should not add. That leaves the question of how to add explicitly. "git add -f"? "git add --ignore-submodules=none"? Thanks, Jonathan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Re: Git issues with submodules
Hi,
On Fri, Nov 22, 2013 at 10:01:44PM +0100, Jens Lehmann wrote:
> Hmm, looks like git show also needs to be fixed to honor the
> ignore setting from .gitmodules. It already does that for
> diff.ignoreSubmodules from either .git/config or git -c and
> also supports the --ignore-submodules command line option.
> The following fixes this inconsistency for me:
>
> -->8---
> diff --git a/builtin/log.c b/builtin/log.c
> index b708517..ca97cfb 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -25,6 +25,7 @@
> #include "version.h"
> #include "mailmap.h"
> #include "gpg-interface.h"
> +#include "submodule.h"
>
> /* Set a default date-time format for git log ("log.date" config variable) */
> static const char *default_date_mode = NULL;
> @@ -521,6 +522,7 @@ int cmd_show(int argc, const char **argv, const char
> *prefix
> int i, count, ret = 0;
>
> init_grep_defaults();
> + gitmodules_config();
> git_config(git_log_config, NULL);
>
> memset(&match_all, 0, sizeof(match_all));
> -->8---
>
> But the question is if that is the right thing to do: should
> diff.ignoreSubmodules and submodule..ignore only affect
> the diff family or also git log & friends? That would make
> users blind for submodule history (which they already are
> when using diff & friends, so that might be ok here too).
>
> > For some reason, the
> > `git add .` is adding the ignored submodule to the index.
>
> The ignore setting is documented to only affect diff output
> (including what checkout, commit and status show as modified).
> While I agree that this behavior is confusing for Sergey and
> not optimal for the floating branch model he uses, git is
> currently doing exactly what it should. And for people using
> the ignore setting to not having to stat submodules with huge
> and/or many files that behavior is what they want: don't bother
> me with what changed, but commit what I did change on purpose.
> We may have to rethink what should happen for users of the
> floating branch model though.
This gets more nasty. When using 'git add .' you secretly add the
submodule to the index. But it is neither shown in status nor diff
--cached. commit actually complains there is nothing to add. But then
once you add a local file to the index you can commit and secretly take
the submodule change with you.
What I think needs fixing here first is that the ignore setting should not
apply to any diffs between HEAD and index. IMO, it should only apply
to the diff between worktree and index.
When we have that the user does not see the submodule changed when
normally working. But after doing git add . the change to the submodule
should be shown in status and diff regardless of the configuration.
I will have a look at that.
After that we can discuss whether add should add submodules that are
tracked but not shown. How about commit -a ? Should it also ignore the
change? I am undecided here. There does not seem to be any good
decision. From the users point of view we should probably not add it
since its not visible in status. What do others think?
Cheers Heiko
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
