Re: [PATCH 3/2] notes-merge: Don't remove .git/NOTES_MERGE_WORKTREE; it may be the user's cwd

From: Johan Herland <johan@herland.net>
Date: Mar 15, 2012, 07:39 UTC
Message-ID: <CALKQrgfpM-y=9O=h33jxirVoOO8dHHO8tWCR9RatxTottpRXFA@mail.gmail.com>
In-Reply-To: <7v7gyml4g7.fsf@alter.siamese.dyndns.org>

On Thu, Mar 15, 2012 at 08:16, Junio C Hamano <gitster@pobox.com> wrote:

Show 10 quoted lines

> Junio C Hamano <gitster@pobox.com> writes:
>> Johan Herland <johan@herland.net> writes:
>>> I'm torn about the new remove_everything_inside_dir(). Obviously it's a
>>> copy-paste-modify of dir.c:remove_dir_recursively(), and could instead be
>>> implemented by adding an extra flag to remove_dir_recursively(). However,
>>> adding a "#define REMOVE_DIR_CONTENTS_BUT_NOT_DIR_ITSELF 04" seemed even
>>> uglier to me...
>>
>> Hmm, what ugliness am I missing when viewing the attached patch?  It looks
>> simple and straightforward enough, at least to me.

Agreed, you found a much more palatable name than I did. The patch below looks good to me, and should become patch #3 in this series, with my "3/2" patch being adjusted accordingly and becoming patch #4. Do you want me to send the whole series again, or is it easier for you to simply fix it up yourself?

Show 29 quoted lines

>>  dir.c |   14 ++++++++++----
>>  dir.h |    1 +
>>  2 files changed, 11 insertions(+), 4 deletions(-)
>>
>> diff --git a/dir.c b/dir.c
>> index 0a78d00..6432728 100644
>> --- a/dir.c
>> +++ b/dir.c
>> @@ -1178,6 +1178,7 @@ int remove_dir_recursively(struct strbuf *path, int flag)
>>       struct dirent *e;
>>       int ret = 0, original_len = path->len, len;
>>       int only_empty = (flag & REMOVE_DIR_EMPTY_ONLY);
>> +     int keep_toplevel = (flag & REMOVE_DIR_KEEP_TOPLEVEL);
>>       unsigned char submodule_head[20];
>>
>>       if ((flag & REMOVE_DIR_KEEP_NESTED_GIT) &&
>> @@ -1185,9 +1186,14 @@ int remove_dir_recursively(struct strbuf *path, int flag)
>>               /* Do not descend and nuke a nested git work tree. */
>>               return 0;
>>
>> +     flag &= ~REMOVE_DIR_KEEP_TOPLEVEL;
>
> Nit. This needs to drop REMOVE_DIR_KEEP_NESTED_GIT as well in order to
> preserve the current behaviour.
>
> I actually suspect that the passing of "only_empty" in the original may be
> a bug in a0f4afb (clean: require double -f options to nuke nested git
> repository and work tree, 2009-06-30), and this patch might be a fix to
> the bug, but I didn't think things through, and it is getting late, so...

I noticed the same while looking at this function, and I think your analysis is correct. As it stands, REMOVE_DIR_KEEP_NESTED_GIT only applies to .git folders located directly in the toplevel dir, and not inside a subdirectory. That strikes me as odd given the name of the flag.

Have fun! :)

...Johan

Show 21 quoted lines

>>       dir = opendir(path->buf);
>> -     if (!dir)
>> -             return rmdir(path->buf);
>> +     if (!dir) {
>> +             if (!keep_toplevel)
>> +                     return rmdir(path->buf);
>> +             else
>> +                     return -1;
>> +     }
>>       if (path->buf[original_len - 1] != '/')
>>               strbuf_addch(path, '/');
>>
>> @@ -1202,7 +1208,7 @@ int remove_dir_recursively(struct strbuf *path, int flag)
>>               if (lstat(path->buf, &st))
>>                       ; /* fall thru */
>>               else if (S_ISDIR(st.st_mode)) {
>> -                     if (!remove_dir_recursively(path, only_empty))
>> +                     if (!remove_dir_recursively(path, flag))
>>                               continue; /* happy */
>>               } else if (!only_empty && !unlink(path->buf))
>>                       continue; /* happy, too */

-- 
Johan Herland, <johan@herland.net>
www.herland.net