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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 7, 2012, 05:50 UTC
Message-ID: <7vaa4v42fi.fsf@alter.siamese.dyndns.org>
In-Reply-To: <alpine.DEB.1.00.1202070504340.24259@s15462909.onlinehome-server.info>

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);