bugQuilt - Bugs: bug #27111, bash completion of patch names...

 
 

bug #27111: bash completion of patch names fails

Submitted by:  Martin von Gagern <gagern>
Submitted on:  Sun 26 Jul 2009 08:55:55 AM UTC  
 
Category: NoneSeverity: 3 - Normal
Item Group: NoneStatus: Fixed
Privacy: PublicAssigned to: Daniel Lovasko <dlovasko>
Open/Closed: Closed

Add a New Comment (Rich MarkupRich Markup):
   

You are not logged in

Please log in, so followups can be emailed to you.

 

Tue 07 Jan 2014 04:00:14 PM UTC, comment #5:

Fixed:

bash_completion: Fix completion of patch names

http://git.savannah.gnu.org/cgit/quilt.git/commit/?id=f535e6f731668035a3fa2472628ba82146ea7262

Jean Delvare <khali>
Project Administrator
Wed 18 Dec 2013 05:12:56 PM UTC, comment #4:

Thanks for the fast and complete feedback Martin, this is very appreciated.

You are correct that the fact that QUILT_PC can be overridden isn't documented. I'm not sure it should be. While we use this mechanism in the build process (to generate the manual page from each command's help), I don't really expect real users to use it. I can't remember anyone ever asking about it.

Since quilt version 0.50, we do remember the name of the patches directory and the series file for every working tree. They are stored in .pc/.quilt_patches and .pc/.quilt_series respectively. This avoids problems if the values for these are changed in .quiltrc after a project has been started. That's what makes "--quiltrc -" relatively safe. This also makes the .pc directory the key to get the environment right and assumes that QUILT_PC never changes.

If anyone really wants to override QUILT_PC and keep bash completion working then I believe they should export QUILT_PC in their account profile rather than .quiltrc. Or maybe we could grep for QUILT_PC in .quiltrc and execute that statement at the beginning of the bash completion script. But honestly I'd wait for someone to complain first.

Creating a dedicated quiltrc file is an interesting option, thanks for suggesting it. I'll think about it; it shouldn't be too difficult to implement.

My own alternative idea was to add a dedicated command line option to (at least) the series, applied and unapplied commands: --no-patches-prefix, which would simply unset QUILT_PATCHES_PREFIX. This too should be easy enough to implement, and adding "--no-patches-prefix" to the command lines in the bash completion script is as easy as adding "--quiltrc -" as I just proposed. I'm just not sure if it would really be more robust than my initial proposal, as other settings in .quiltrc could cause similar problems. For example, if I set QUILT_SERIES_ARGS="--color" in my .quiltrc, this breaks bash completion too, because "quilt series" will then surround patch names with color codes. There may be other similar ways to break it, and at least "--quiltrc -" works around that.

WRT to /etc/quilt.quiltrc, I don't like it either. I'm not sure the file serves a purpose, and even if it does, I would prefer that it is all commented out by default. This creates two sets of default settings, the one from quilt itself and the one from the default quilt.quiltrc file, and just creating an empty ~/.quiltrc makes the user switch from the latter to the former; very confusing. But I guess there are reasons for that, historical or otherwise, so I don't really expect to be able to get rid of it.

Jean Delvare <khali>
Project Administrator
Wed 18 Dec 2013 03:40:22 PM UTC, comment #3:

Nice to see some activity here. Skipping quiltrc seems like a reasonable approach except for one case: if no .pc directory exists yet, and the user has customized QUILT_PATCHES in his or her ~/.quiltrc. Since my own patches won't deal with that situation either, I consider it the best suggestion we have so far.

Reading the sources, I found that one can also override QUILT_PC on the command line, and from the RC file as well. This does not appear to be documented. If someone set QUILT_PC in ~/.quiltrc, then not reading quiltrc would lead to completely incorrect completions. But I guess this should be a pretty rare situation. The only case I can imagine where setting QUILT_PC makes sense would be stacking multiple patch sets, in which case I'd use an environment variable and not the ~/.quiltrc to set things.

The only alternative I could imagine would be creating a quiltrc file for bash completions only, which loads the original quiltrc (finding it the same way quilt does), then unsets QUILT_PATCHES_PREFIX. This would mean some duplicate work, so I wouldn't suggest implementing this, just wanted to note it in case it is ever needed.

On the whole, I have to wonder about /etc/quilt.quiltrc. Firstly, why does that file contain settings that the quilt devs don't use? Is it backwards compatibility?
Secondly, I would much prefer environment variables overriding those from quiltrc, although I must confess that I know no simple syntax how to achieve this. In particular not one which also works for unsetting variables, as is required here.

On the whole, I'd like to see your patch committed.

Martin von Gagern <gagern>
Wed 18 Dec 2013 02:56:51 PM UTC, comment #2:

Sorry for the delay. I looked into this today, both your patches and the one proposed by Daniel Lovasko on the quilt-dev list.

While these patches indeed solve the problem, they add complexity and come with a performance hit.

The whole problem comes from QUILT_PATCHES_PREFIX=yes in /etc/quilt.quiltrc. Most users (including me) have a ~/.quiltrc which overrides /etc/quilt.quiltrc, without QUILT_PATCHES_PREFIX=yes in it, so we were not affected by the bug. Without QUILT_PATCHES_PREFIX, quilt series and other similar commands return absolute patch names, instead of relative to the working directory.

This explains why this bug could stay open for so long without being looking into: I believe a significant share of users never hit it, including the core quilt developers / maintainers.

I think that a better solution to this problem is to disable QUILT_PATCHES_PREFIX during bash completion. This can be done easily by adding "--quiltrc -" to the command line of affected quilt commands (series, applied and unapplied as far as I know) in the bash completion script. This comes with no performance hit, in fact it may even make things slightly faster.

Jean Delvare <khali>
Project Administrator
Wed 23 Nov 2011 10:25:46 AM UTC, comment #1:

Over two years without so much as a comment. Ping?

Martin von Gagern <gagern>
Sun 26 Jul 2009 08:55:55 AM UTC, original submission:

The bash completion from quilt-0.48 or current git fails to complete patch names with app-bash-4.0. This is due to the fact that the completion function uses "quilt series", "quilt applied" and similar internally, which in turn output the paths of the patch files relative to the current working directory, instead of simply the file names as are usually used.

So the possible completions start with "patches/", "../../patches/" or something similar, depending on how deep in the tree you currently are. If you changed $QUILT_PATCHES, that directory will of course change as well. Completion using these full paths will work, but isn't what users would expect. It also isn't what the list of possible completions prints; it seems that this list does strip directory names from its output. So a listed completion doesn't work when typed, which is very irritating.

Steps to reproduce:
1. mkdir foo
2. unset QUILT_PATCHES
3. cd foo
4. mkdir patches
5. quilt new foo.patch
6. quilt new bar.diff
7. quilt header <press tab twice here>
-a -h -r
--backup bar.diff --strip-diffstat
-e foo.patch --strip-trailing-whitespace
8. quilt header f<press tab here>
<no completion at all!!!>
9. quilt series
patches/foo.patch
patches/bar.diff

I guess the solution would be to pipe the output of all those internall quilt patch listing commands through something that strips directory names, like
sed -e 's:.*/::'
The attached gentoo278641a.patch implements this.

Of course this doesn't work when using subdirectories inside the $QUILT_PATCHES directory. To work with those as well, the sed would have to specifically look for a sequence of zero or more "../" followed by $QUILT_PATCHES, possibly followed by "/". A matching sed command would be something like the following one:
sed -e "s:^\\(\\.\\./\\)*${QUILT_PATCHES:-patches}/\\?::"
The attached gentoo278641b.patch implements this.

Strictly speaking, the QUILT_PATCHES directory would have to be escaped, to avoid special interpretation of any regular expression symbols. But I don't know how to express this easily, and I doubt that's worth the effort, as most QUILT_PATCHES directories should be fairly simple, and special treatment of a dot shouldn't hurt.

Also strictly speaking there should be some handling of ~/.quiltrc, the --quiltrc command line option, and so on. So finding the correct patch names can be almost arbitrarily complex. I won't expect a perfect fix, but a mostly working fix.

I originally reported this issue as http://bugs.gentoo.org/278641

Martin von Gagern <gagern>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach File(s):
   
   
Comment:
   

Attached Files
file #30052:  completion-fix-prefix.patch added by khali (6kB - text/x-patch - bash_completion: Fix completion of patch names (v2))
file #18478:  gentoo278641a.patch added by gagern (5kB - application/octet-stream - Two patches addressing the issue with different levels of complexity)
file #18479:  gentoo278641b.patch added by gagern (5kB - application/octet-stream - Two patches addressing the issue with different levels of complexity)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by khali (Updated the item)
  • -unavailable- added by gagern (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 8 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Tue 07 Jan 2014 04:00:36 PM UTCkhaliOpen/ClosedOpen=>Closed
    Tue 07 Jan 2014 04:00:14 PM UTCkhaliStatusConfirmed=>Fixed
    Fri 20 Dec 2013 10:22:51 AM UTCkhaliAttached File-=>Added completion-fix-prefix.patch, #30052
    Wed 18 Dec 2013 03:06:44 PM UTCkhaliAttached File-=>Added completion-fix-prefix.patch, #30035
    Wed 18 Dec 2013 02:56:51 PM UTCkhaliStatusNone=>Confirmed
    Tue 15 Jan 2013 05:37:53 PM UTCkhaliAssigned toNone=>dlovasko
    Sun 26 Jul 2009 08:55:55 AM UTCgagernAttached File-=>Added gentoo278641a.patch, #18478
      Attached File-=>Added gentoo278641b.patch, #18479

    Back to the top


    Powered by Savane 3.1-cleanup