[PATCH] add -e: ignore dirty submodules

[PATCH] add -e: ignore dirty submodules

[Johannes Schindelin]

We cannot add untracked/modified files in submodules anyway.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	This patch is actually from Oct 23, 2010.

 builtin/add.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index 1c42900..b79336d 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -280,6 +280,7 @@ static int edit_patch(int argc, const char **argv, const char *prefix)
 
 	argc = setup_revisions(argc, argv, &rev, NULL);
 	rev.diffopt.output_format = DIFF_FORMAT_PATCH;
+	DIFF_OPT_SET(&rev.diffopt, IGNORE_DIRTY_SUBMODULES);
 	out = open(file, O_CREAT | O_WRONLY, 0644);
 	if (out < 0)
 		die (_("Could not open '%s' for writing."), file);
-- 
1.7.9.msysgit.0.27.ge92cd

Re: [PATCH] add -e: ignore dirty submodules

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> We cannot add untracked/modified files in submodules anyway.

I can see the updated code would not break "git apply" that will be run on
this output, but the above cannot be the whole story. It is unclear to me
what it is trying to achieve (in other words, "this patch does not break
the system" is not the whole purpose of the patch).

When a submodule is updated and is dirty, we would get:

    diff --git a/submodule b/submodule
    @@ -1,+1 @@
    -Subproject commit XXXX...
    +Subproject commit YYYY...-dirty

and leaving this diff in the edited patch adds YYYY... for submodule, even
though "-dirty" suffix is there.  So it is not fixing "the user tries to
update but fails because we do not filter dirty submodules" bug, or somesuch.
Besides, showing -dirty may be a good reminder that submodule has further
changes on top of what is going to be committed in this case.

When a submodule is only dirty, we would see:

    diff --git a/submodule b/submodule
    @@ -1,+1 @@
    -Subproject commit XXXX...
    +Subproject commit XXXX...-dirty

and leaving this diff in the edited patch keeps the submodule at XXXX...,
again without failing, so it is not fixing "the user gets unnecessary
error message" bug, or somesuch.  In this case, leaving this diff will be
a no-op so it is wasteful and distracting to the user who edits the patch.

Is that what this patch is about?  "For a submodule that is unchanged but
is dirty, submodule diff whose difference is only the '-dirty' suffix is
given but the user cannot update the index with such a diff anyway, so it
is a waste of space", or something like that?

That is the best guess I arrived at, but I suspect that it cannot be it,
as that discards the "-dirty" clue from the output when the submodule path
does have difference, as we saw in the earlier example. So there must be
something I am missing.

So I am out of ideas guessing what this patch is trying to achieve.  The
commit log shouldn't force the readers of the history to _guess_ like the
above.

>  builtin/add.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/builtin/add.c b/builtin/add.c
> index 1c42900..b79336d 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -280,6 +280,7 @@ static int edit_patch(int argc, const char **argv, const char *prefix)
>  
>  	argc = setup_revisions(argc, argv, &rev, NULL);
>  	rev.diffopt.output_format = DIFF_FORMAT_PATCH;
> +	DIFF_OPT_SET(&rev.diffopt, IGNORE_DIRTY_SUBMODULES);
>  	out = open(file, O_CREAT | O_WRONLY, 0644);
>  	if (out < 0)
>  		die (_("Could not open '%s' for writing."), file);

Re: [PATCH] add -e: ignore dirty submodules

[Johannes Schindelin]

Dear Junio C Hamano,

On Mon, 6 Feb 2012, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > We cannot add untracked/modified files in submodules anyway.

The purpose of "add -e" is to stage changes. It does so by presenting a
diff which the user can edit (and applying it after recounting the numbers
in the hunk headers).

If the diff does not apply, it does not make sense to present it to the
user.

Offering to add changes represented by a diff like

diff --git <blub>
<header>
-deadbeef...
+deadbeef...-dirty

does not make sense.

Even a diff like

diff --git <blah>
<header>
-deadbeef...
-coffeebabe...-dirty

would be refused by git-apply.

Indeed, due to the design of the submodules, it is impossible to stage
dirty files in a submodule and a supercommit at the same time. Oh, and this
discussion is not the place to wish for a feature like that, just in case
you want to ask me to implement that in order to be allowed to have my puny
little patch applied. (I guess this is the reason why I waited so long
before I dared to submit the patch to the mailing list.)

Now, due to these concerns, even stripping out the -dirty part can lead to
a comically non-sensical diff like

diff --git <blergh>
<header>
-deadbeef...
+deadbeef...

So keeping the diff generation as-is but preparing the patch by munching
away the -dirty suffix would not fix the problem.

Also, it would be wrong to assume that the user asked to get a status
update. git add -e != git status. If the user wanted to know what changes
are in the worktree, including the worktrees of the submodules, but only
those that have been checked out, git status would be the command to call
(even if it was touted as a git commit --dry-run once upon a time, which
is kind of wrong, see above, you cannot commit the untracked or dirty
files in a submodule, yet git status shows them).

So: showing the fact that a submodule has untracked or dirty files in the
patch that the user wants to edit with git add -e is wrong, wrong, wrong.

The only submodule-related thing we should ever present to the user who
called git add -e is a diff like

diff --git <narf>
<header>
-deadcad...
+beda551ed...

because that is a stageable change.

Alas, salvation is nigh! Yes, just a little line which asks for the level
"dirty" (which implies "untracked", as detailed by diff-options.txt, uhm,
sorry, I was asked to be precise, Documentation/diff-options.txt) fixes
that!

With this flag, there are no more "-dirty" lines in the submodule diffs.
None! Only diffs between different submodule commits are shown, and even
if the worktree of the submodule is dirty, or has untracked files, the
-dirty suffix is omitted. Just what you need for git-apply to work! Yay!

As a plus, it makes the diff generation faster because what cannot be
staged anyway is not even discovered! Super-yay!

I actually enjoyed writing this text so much, may I respectfully ask to
include it verbatim in the commit message?

Thank you very much,
Johannes Schindelin

Re: [PATCH] add -e: ignore dirty submodules

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Offering to add changes represented by a diff like
>
> diff --git <blub>
> <header>
> -deadbeef...
> +deadbeef...-dirty
>
> does not make sense.

Now you are being much clearer.  If you had the above from the beginning,
I wouldn't have had to ask.

So after all, this is a noise reduction patch, and I think that it is a
good change.

> Even a diff like
>
> diff --git <blah>
> <header>
> -deadbeef...
> -coffeebabe...-dirty
>
> would be refused by git-apply.

That would be refused, but if you replace '-coffeebabe' with '+coffeebabe',
which is what you would get from "git diff" without ignore-dirty-submodule,
I think it would be accepted by "git apply" just fine.

Have you tried it?

> I actually enjoyed writing this text so much, may I respectfully ask to
> include it verbatim in the commit message?

With mostly irrelevant mumblings and all?  Of course not.

I was asking what you meant to achieve with the patch, because you did not
even include relevant bits of information.

But now I think I learned what you meant by this patch, so let me try to
see if I understood it correctly by rephrasing it.

Like this?

-- >8 --
Subject: add -e: do not show difference in a submodule that is merely dirty
From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Date: Tue, 7 Feb 2012 05:05:48 +0100 (CET)

When the HEAD of the submodule matches what is recorded in the index of
the superproject, and it has local changes or untracked files, the patch
offered by "git add -e" for editing shows a diff like this:

    diff --git a/submodule b/submodule
    <header>
    -deadbeef...
    +deadbeef...-dirty

Because applying such a patch has no effect to the index, this is a
useless noise.  Generate the patch with IGNORE_DIRTY_SUBMODULES flag to
prevent such a change from getting reported.

This patch also loses the "-dirty" suffix from the output when the HEAD of
the submodule is different from what is in the index of the superproject.
As such dirtiness expressed by the suffix does not affect the result of
the patch application at all, there is no information lost if we remove
it. The user could still run "git status" before "git add -e" if s/he
cares about the dirtiness.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

 builtin/add.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index 1c42900..b79336d 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -280,6 +280,7 @@ static int edit_patch(int argc, const char **argv, const char *prefix)
 
 	argc = setup_revisions(argc, argv, &rev, NULL);
 	rev.diffopt.output_format = DIFF_FORMAT_PATCH;
+	DIFF_OPT_SET(&rev.diffopt, IGNORE_DIRTY_SUBMODULES);
 	out = open(file, O_CREAT | O_WRONLY, 0644);
 	if (out < 0)
 		die (_("Could not open '%s' for writing."), file);

Re: [PATCH] add -e: ignore dirty submodules

[Jens Lehmann]

Am 07.02.2012 09:21, schrieb Junio C Hamano:
> So after all, this is a noise reduction patch, and I think that it is a
> good change.

I agree. While my first thought was that it might make sense to honor
the diff.ignoreSubmodules setting here too to produce the same
information "git status" gives (so you notice you might have forgotten
to commit something in the submodule), I now agree that doesn't make
sense in the context of add -e.