threads / patch / 29564

patchadd -e: ignore dirty submodules

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

## tl;dr

5 messages between Feb 7, 2012 and Feb 7, 2012. Diffs are folded; open one to read it.

replies: 4people: 3as markdown or json

Johannes Schindelin· Feb 7, 2012, 04:05 UTC · lore
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(-)
Show changes to builtin/add.c +1 −0
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
Junio C Hamano· Feb 7, 2012, 05:50 UTC · re: Johannes Schindelin · lore

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

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.

Show 15 quoted lines
>  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);
Johannes Schindelin· Feb 7, 2012, 07:45 UTC · re: Junio C Hamano · lore

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

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
Show changes to diff +3 −6
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
Junio C Hamano· Feb 7, 2012, 08:21 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 8 quoted lines
> 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.

Show 8 quoted lines
> 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(-)
Show changes to builtin/add.c +1 −0
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);
Jens Lehmann· Feb 7, 2012, 20:43 UTC · re: Junio C Hamano · lore

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

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.

← back to recent threads