From: Omar Polo <op@omarpolo.com>
Subject: Re: got diff vs .gitignore
To: Stefan Sperling <stsp@stsp.name>
Cc: gameoftrees@openbsd.org
Date: Thu, 10 Feb 2022 16:59:38 +0100

Stefan Sperling <stsp@stsp.name> writes:

> On Thu, Feb 10, 2022 at 03:58:15PM +0100, Omar Polo wrote:
>> Hello,
>> 
>> I noticed that `got diff' and .gitignore have a strange relationship.
>> 
>> I was working in an autotools project, my .gitignore had "**/Makefile"
>> in it; one sub-directory thought has plain Makefile that was properly
>> added to the repository.  `got diff' showed the diff for it, but `got
>> diff path/to/Makefile' didn't!
>> 
>> Patch below adds a test case and fixes the isuse for me.  Regress is
>> passing, but I'm not 100% sure that the patch is correct.
>> got_worktree_status takes a flag `no_ignores'; i'm merely switching it
>> to don't skip ignored files.
>
> A big problem with your fix is that the status crawl will no longer
> skip ignored subdirs. For instance, if you are inside a /usr/ports
> work tree with a pobj/ dir that contains lots of stuff, you don't
> want got status to crawl the ignored pobj/ dir while diffing things.
> Crawling pobj/ would just be a waste of time for no gain.
>
> The patch below seems like a better fix to me.
> The bug is that report_single_file_status() matches paths against ignore
> patterns before doing anything else. This forgets to take into account
> that only unversioned files should be affected by ignores. Any already
> versioned file should be processed as usual even if it happens to match
> an ignore pattern. Yet this function skips such files.
>
> This patch makes your test pass and does not break any others.

I haven't thought about that situation, your diff is certainly better
and it does fix the problem from me too.

just a curiosity: there's a reason to delay the ignore check after the
lstat and not just after got_fileindex_entry_get?  My reasoning is that
if got_fileindex_entry_get doesn't find anything then the file is not in
the repository and we can honour the `no_ignore' flag without even
caring to stat it.  (I'm not familiar enough with how the worktree is
managed thought, so I'm probably missing something)

thanks!

> ok?
>
> diff 5a20d08d6546fa260cd5ce2ed01e438f5ec35f41 bb10ea6a88e34392660ce4081898b737955d702e
> blob - 610c0eb85518f221f5f8ae1907397ddd07bea422
> blob + 0b4571401cfdddef043be11cce960c58beae2802
> --- lib/worktree.c
> +++ lib/worktree.c
> @@ -3560,9 +3560,6 @@ report_single_file_status(const char *path, const char
>  	struct got_fileindex_entry *ie;
>  	struct stat sb;
>  
> -	if (!no_ignores && match_ignores(ignores, path))
> -		return NULL;
> -
>  	ie = got_fileindex_entry_get(fileindex, path, strlen(path));
>  	if (ie)
>  		return report_file_status(ie, ondisk_path, -1, NULL,
> @@ -3575,6 +3572,9 @@ report_single_file_status(const char *path, const char
>  		    GOT_STATUS_NO_CHANGE, path, NULL, NULL, NULL, -1, NULL);
>  	}
>  
> +	if (!no_ignores && match_ignores(ignores, path))
> +		return NULL;
> +
>  	if (S_ISREG(sb.st_mode) || S_ISLNK(sb.st_mode))
>  		return (*status_cb)(status_arg, GOT_STATUS_UNVERSIONED,
>  		    GOT_STATUS_NO_CHANGE, path, NULL, NULL, NULL, -1, NULL);