-
-
Notifications
You must be signed in to change notification settings - Fork 143
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update gitignore when switching subdirectories between normal and generated #492
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great overall! I don't have a problem with remove_tup_gitignore() staying in parser.c since it is still used there as well. (I think generally, updater.c depends on parser.c and less so the other way around, but that's probably not universal).
src/tup/create_name_file.c
Outdated
if(tup_db_select_tent(dtent, ".gitignore", &gitignore_tent) < 0) | ||
return -1; | ||
if(gitignore_tent && gitignore_tent->type == TUP_NODE_GENERATED) { | ||
tup_db_add_create_list(gitignore_tent->dt); // HACK -- ideally, tup_db_add_modify_list here would be sufficient, but as it is, we need to reparse the directory's Tupfile to update the .gitignore. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The create_list is a bit misnamed these days. Originally "The UPdater" code (aka: tup) was supposed to take a list of files that were created, deleted, or modified since the last update from the file monitor. These became the create_list, delete_list, and modify_list. Tupfiles needed to be re-parsed when files were created or deleted (or if the Tupfile itself was modified). Over time the delete_list was removed, and the create_list became a list of directories that need to be re-parsed rather than a list of newly created files. So adding the directory tupid to the create_list is the correct thing to do here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to confirm: I should switch this to be dtent->dt
instead of gitignore_tent->dt
? (:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the confusion -- dt
is the tupid for the directory containing the file, so gitignore_tent->dt
is correct. The tupid for the gitignore file itself would be gitignore_tent->tnode.tupid
(the awkward construction there is because all of the tupids are in a tree in src/tup/entry.c). So dtent->tnode.tupid
would equal gitignore_tent->dt
here. dtent->dt
is the parent directory of the directory containing the .gitignore file, so if we had foo/sub/.gitignore
, then dtent->dt
would be the directory foo
, while we want sub
.
Now it's probably more confusing :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah... okay, yeah, that makes sense. Gotta love unintentionally being right; I for sure thought I was adding the gitignore as a created file there XDXD
@@ -1130,7 +1129,7 @@ int import(struct tupfile *tf, const char *cmdline, const char **retvar, const c | |||
/* If a .gitignore directive is removed, we need to either revert back to the | |||
* user's explicit .gitignore file, or remove it entirely. | |||
*/ | |||
static int remove_tup_gitignore(struct tupfile *tf, struct tup_entry *tent) | |||
int remove_tup_gitignore(struct graph *g, struct tup_entry *tent) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably still take a struct tupfile, since this function should be printing errors to tf->f
instead of stderr
. The former will print the error messages under the banner of the directory being parsed, rather than immediately, so it is more obvious which directory actually failed. I can fix this in a separate commit if you like.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Err in retrospect, keeping the struct tupfile would make moving it into updater.c more difficult. I am making a change now to make this a FILE *err input instead, so the updater.c usage of it can pass in stderr and the parser.c usage can pass in tf->f.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yeah, I was wondering if we still had a valid tupfile in updater.c for that 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, I left this one for you, since you said you already had a patch waiting; but I fixed the other issues. I think it might be good to go, but you'll be the judge of that XD
b2064a7
to
c35c364
Compare
Merged (with minor changes from the remove_tup_gitignore differences) - thanks so much! |
Ah.. we lost the Verified badge there sadly; welp XD Thanks! |
Sorry, what happened? Did my rebasing this cause an issue? |
Mmh, yeah; GitHub doesn't trust that your unsigned commit is really authored by me (relevant GH docs link). But that's fine; if anyone asks questions, just direct them to the PR 😂 |
Doh, sorry. I wasn't aware that was a thing on github. |
Fixes #491.
Current version is a bit hacky;
remove_tup_gitignore
probably doesn't belong inparser.c
after these changes, and I'm usingtup_db_add_create_list
instead oftup_db_add_modify_list
because it just-so happens to work. If you like the overall shape this is taking, just let me know and I'll fix those rough spots; otherwise, you probably have a better idea of what needs to be done 😅