Page MenuHomeFreeBSD

Enhance patch(1) to search for file being patched, from the current directory.
AcceptedPublic

Authored by hselasky on May 7 2021, 1:42 PM.
Tags
None
Referenced Files
Unknown Object (File)
Dec 28 2022, 1:07 PM
Unknown Object (File)
Dec 27 2022, 5:47 AM

Details

Summary

When working with code integration, it may frequently happen that directory
layouts are different, but file names are identical. This patch adds a
new feature to patch(1) where it will automatically and recursivly search for a
matching file by filename, if the user input is "*" when asked to enter a
filename.

MFC after: 1 week
Sponsored by: NVIDIA Networking

Test Plan
|diff --git a/usr.bin/patch/pch.c b/usr.bin/patch/pch.c
|index 70051640cf0..94d3f24d43e 100644
|--- a/usr.bin/patch/pch.c
|+++ b/usr.bin/patch/pch.c
--------------------------
File to patch (enter '/' to search): /
Select this file, ./test/pch.c [y]:

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

hselasky created this revision.
hselasky edited the test plan for this revision. (Show Details)

A command line argument to do this without the prompt (non-interactively) would be nice too.

@swills : Yes, but how to avoid problem with other OS'es implementation? Do you have any suggestion for which command line arguments are free?

@swills : Yes, but how to avoid problem with other OS'es implementation? Do you have any suggestion for which command line arguments are free?

May I humbly suggest -S (capitol S)?
Thanks for doing this. I think it's a great idea. My only concern might be that several files, even just more than one might cause complications/confusion.
Maybe cause a break to require a Y/N to confirm which one when such a situation is encountered?

I never needed such thing, and cannot think about situation where it would be useful to me.

That said, I believe that your file hierarchy iteration is too naive. Look at fts(3) and options it accepts, esp. about symlink following.
I agree that if such feature is added, it must be guarded by a command line option to patch(1).

I have seen the use of '**' (two asterisks) to mean "search through all directories at any depth" ...

  • Use "/" for searching
  • Added new option -S to patch

I'm not at all sure how I feel about this.

However -S is unused in gnu patch upstream.

usr.bin/patch/util.c
431

Maybe fts(3) would make this code much simpler?

- Pass bestguess to basename.

@imp : I just wanted to keep things simple. Are there any examples for fts ?

I've been using this patch for some time now on my local machine with great success. Any strong objections against this patch?

@imp : You suggested using fts(3). Can we keep the code as is, simple and readable. There is a need to generate the full path for open(3), so I don't know how that will work with fts(3): Also there is no need for performance here, from my point of view.

usr.bin/patch/patch.c
547

gnu patch does not have a -S documented, so that's good.

usr.bin/patch/pch.c
215

where is presult freed? The one below is freed, but this one isn't.

usr.bin/patch/util.c
431

I've checked, and for this, I think fts would be about a wash.

457

d_type isn't documented and is non-standard.
This might be a problem should we ever have to bootstrap patch on a non-freebsd system.
However, the only reason to do that is so we can apply patches that gnu-patch on the host system doesn't grok or gets wrong.
patch isn't a bootstrap thing now (though it is a nxb thing, but that's a FreeBSD only feature).
So on the whole, it's likely OK, and I'll just not it for posterity.

To be clear, it's ok not to use fts since I don't think the code would be that much smaller (if at all).

My gut reaction when I saw this last year was that I'm not a fan, but it is hidden behind an option, at least... I don't really have any strong objection, though it might be nice if it had slightly more intelligence to it. e.g., if I have a patch against a foo/bar/file.c, search for a file.c but if you find: x/bar/file.c and x/pineapples/file.c, prefer the former not just because it's the first discovered but because it has the most in common.

FTSENT has an fts_path in addition to fts_name, so the path isn't an issue.

I don't really have any strong opinion either for or against, though this feature is not something I'd likely use...

Thanks for all your comments so far. I really appreciate it.

usr.bin/patch/pch.c
218

presult is swapped with bestguess, freeing the previous value just above.

usr.bin/patch/util.c
457

Is there any requirement for bootstrapping on other OS'es. What API's do they need to support? Probably I could ifdef the whole feature.

So maybe I would need to use fstat() instead to figure out directory vs files?

My gut reaction when I saw this last year was that I'm not a fan, but it is hidden behind an option, at least... I don't really have any strong objection, though it might be nice if it had slightly more intelligence to it. e.g., if I have a patch against a foo/bar/file.c, search for a file.c but if you find: x/bar/file.c and x/pineapples/file.c, prefer the former not just because it's the first discovered but because it has the most in common.

In the manual mode it will ask the user if 'path/file.xxx' is OK, so you will have the chance to select the right file or step all the possibilities.

I found it useful when doing debugging. For example when getting patches from people off bugzilla, In FreeBSD all kernel files are uniqe, due to the way the kernel is linked. There cannot be two xxx.o with same xxx !

FTSENT has an fts_path in addition to fts_name, so the path isn't an issue.

I see. Is fts protable enough to bootstrap on other OS'es?

I don't really have any strong opinion either for or against, though this feature is not something I'd likely use...

I might just try and back it out if someone complains about it.

My gut reaction when I saw this last year was that I'm not a fan, but it is hidden behind an option, at least... I don't really have any strong objection, though it might be nice if it had slightly more intelligence to it. e.g., if I have a patch against a foo/bar/file.c, search for a file.c but if you find: x/bar/file.c and x/pineapples/file.c, prefer the former not just because it's the first discovered but because it has the most in common.

In the manual mode it will ask the user if 'path/file.xxx' is OK, so you will have the chance to select the right file or step all the possibilities.

I found it useful when doing debugging. For example when getting patches from people off bugzilla, In FreeBSD all kernel files are uniqe, due to the way the kernel is linked. There cannot be two xxx.o with same xxx !

FTSENT has an fts_path in addition to fts_name, so the path isn't an issue.

I see. Is fts protable enough to bootstrap on other OS'es?

I wouldn't worry about it, this is light enough and not essential for bootstrap; we could just patch it out for bootstrap versions if it came to that. Don't feel pressured to use it, though, I just find it a little easier to follow along personally.

I don't really have any strong opinion either for or against, though this feature is not something I'd likely use...

I might just try and back it out if someone complains about it.

Add checks for __BSD_VISIBLE == 0, to avoid breaking bootstrap on non-BSD systems.

@imp : Any further comments?

hselasky retitled this revision from Enhance patch(1) to search for file being patched, from current directory. to Enhance patch(1) to search for file being patched, from the current directory..Mar 8 2022, 11:24 AM
hselasky edited the summary of this revision. (Show Details)
hselasky removed a reviewer: kib.

I plan on submitting this soon. Please shout if you have more change requests.

Thank you!

This revision is now accepted and ready to land.Mar 10 2022, 9:25 AM