* [PATCH] am: Allow passing exclude and include args to apply @ 2010-12-19 16:17 maximilian attems 2010-12-19 16:56 ` Thiago Farina 0 siblings, 1 reply; 7+ messages in thread From: maximilian attems @ 2010-12-19 16:17 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, klibc, maximilian attems When porting patches from dash git to klibc git, where dash has a different directory structure those switches are handy: Exported with format-patch on dash side and used am as import for klibc side. Signed-off-by: maximilian attems <max@stro.at> --- Documentation/git-am.txt | 5 ++++- git-am.sh | 4 +++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/Documentation/git-am.txt b/Documentation/git-am.txt index 51297d0..4c65dba 100644 --- a/Documentation/git-am.txt +++ b/Documentation/git-am.txt @@ -13,7 +13,8 @@ SYNOPSIS [--3way] [--interactive] [--committer-date-is-author-date] [--ignore-date] [--ignore-space-change | --ignore-whitespace] [--whitespace=<option>] [-C<n>] [-p<n>] [--directory=<dir>] - [--reject] [-q | --quiet] [--scissors | --no-scissors] + [--exclude=PATH] [--include=PATH] [--reject] [-q | --quiet] + [--scissors | --no-scissors] [(<mbox> | <Maildir>)...] 'git am' (--continue | --skip | --abort) @@ -87,6 +88,8 @@ default. You can use `--no-utf8` to override this. -C<n>:: -p<n>:: --directory=<dir>:: +--exclude=<path-pattern>:: +--include=<path-pattern>:: --reject:: These flags are passed to the 'git apply' (see linkgit:git-apply[1]) program that applies diff --git a/git-am.sh b/git-am.sh index df09b42..174f6a2 100755 --- a/git-am.sh +++ b/git-am.sh @@ -22,6 +22,8 @@ whitespace= pass it through git-apply ignore-space-change pass it through git-apply ignore-whitespace pass it through git-apply directory= pass it through git-apply +exclude= pass it through git-apply +include= pass it through git-apply C= pass it through git-apply p= pass it through git-apply patch-format= format the patch(es) are in @@ -340,7 +342,7 @@ do ;; --resolvemsg) shift; resolvemsg=$1 ;; - --whitespace|--directory) + --whitespace|--directory|--exclude|--include) git_apply_opt="$git_apply_opt $(sq "$1=$2")"; shift ;; -C|-p) git_apply_opt="$git_apply_opt $(sq "$1$2")"; shift ;; -- 1.7.2.3 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] am: Allow passing exclude and include args to apply 2010-12-19 16:17 [PATCH] am: Allow passing exclude and include args to apply maximilian attems @ 2010-12-19 16:56 ` Thiago Farina 2010-12-19 17:13 ` maximilian attems 0 siblings, 1 reply; 7+ messages in thread From: Thiago Farina @ 2010-12-19 16:56 UTC (permalink / raw) To: maximilian attems; +Cc: git, Junio C Hamano, klibc On Sun, Dec 19, 2010 at 2:17 PM, maximilian attems <max@stro.at> wrote: > When porting patches from dash git to klibc git, > where dash has a different directory structure those > switches are handy: > Exported with format-patch on dash side and used am > as import for klibc side. > I don't know, but this commit message doesn't look good. Can you be more specific about what the patch does, and what it's trying to fix. Describing a use case is good, but in this case it isn't help me much (maybe for others the issue here is clear, but for my taste no). > Signed-off-by: maximilian attems <max@stro.at> > --- > Documentation/git-am.txt | 5 ++++- > git-am.sh | 4 +++- > 2 files changed, 7 insertions(+), 2 deletions(-) > > diff --git a/Documentation/git-am.txt b/Documentation/git-am.txt > index 51297d0..4c65dba 100644 > --- a/Documentation/git-am.txt > +++ b/Documentation/git-am.txt > @@ -13,7 +13,8 @@ SYNOPSIS > [--3way] [--interactive] [--committer-date-is-author-date] > [--ignore-date] [--ignore-space-change | --ignore-whitespace] > [--whitespace=<option>] [-C<n>] [-p<n>] [--directory=<dir>] > - [--reject] [-q | --quiet] [--scissors | --no-scissors] > + [--exclude=PATH] [--include=PATH] [--reject] [-q | --quiet] > + [--scissors | --no-scissors] > [(<mbox> | <Maildir>)...] > 'git am' (--continue | --skip | --abort) > > @@ -87,6 +88,8 @@ default. You can use `--no-utf8` to override this. > -C<n>:: > -p<n>:: > --directory=<dir>:: > +--exclude=<path-pattern>:: > +--include=<path-pattern>:: > --reject:: > These flags are passed to the 'git apply' (see linkgit:git-apply[1]) > program that applies > diff --git a/git-am.sh b/git-am.sh > index df09b42..174f6a2 100755 > --- a/git-am.sh > +++ b/git-am.sh > @@ -22,6 +22,8 @@ whitespace= pass it through git-apply > ignore-space-change pass it through git-apply > ignore-whitespace pass it through git-apply > directory= pass it through git-apply > +exclude= pass it through git-apply > +include= pass it through git-apply > C= pass it through git-apply > p= pass it through git-apply > patch-format= format the patch(es) are in > @@ -340,7 +342,7 @@ do > ;; > --resolvemsg) > shift; resolvemsg=$1 ;; > - --whitespace|--directory) > + --whitespace|--directory|--exclude|--include) > git_apply_opt="$git_apply_opt $(sq "$1=$2")"; shift ;; > -C|-p) > git_apply_opt="$git_apply_opt $(sq "$1$2")"; shift ;; > -- > 1.7.2.3 > > -- > To unsubscribe from this list: send the line "unsubscribe git" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] am: Allow passing exclude and include args to apply 2010-12-19 16:56 ` Thiago Farina @ 2010-12-19 17:13 ` maximilian attems 2010-12-19 17:26 ` [klibc] " Sam Ravnborg 2010-12-19 20:13 ` Junio C Hamano 0 siblings, 2 replies; 7+ messages in thread From: maximilian attems @ 2010-12-19 17:13 UTC (permalink / raw) To: Thiago Farina; +Cc: git, Junio C Hamano, klibc On Sun, Dec 19, 2010 at 02:56:58PM -0200, Thiago Farina wrote: > On Sun, Dec 19, 2010 at 2:17 PM, maximilian attems <max@stro.at> wrote: > > When porting patches from dash git to klibc git, > > where dash has a different directory structure those > > switches are handy: > > Exported with format-patch on dash side and used am > > as import for klibc side. > > > > I don't know, but this commit message doesn't look good. Can you be > more specific about what the patch does, and what it's trying to fix. hmm, it does what the subject says. (: have you ever used `git am'? > Describing a use case is good, but in this case it isn't help me much > (maybe for others the issue here is clear, but for my taste no). when one wants to promote a specific new feature, it is much better to come up with it's use case, as burden is on Maintainer to keep it working. Please be more specific on what's missing? Your personal taste is unknown to me and not of importance. thank you. -- maks ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [klibc] [PATCH] am: Allow passing exclude and include args to apply 2010-12-19 17:13 ` maximilian attems @ 2010-12-19 17:26 ` Sam Ravnborg 2010-12-19 18:27 ` maximilian attems 2010-12-19 20:13 ` Junio C Hamano 1 sibling, 1 reply; 7+ messages in thread From: Sam Ravnborg @ 2010-12-19 17:26 UTC (permalink / raw) To: maximilian attems; +Cc: Thiago Farina, klibc, Junio C Hamano, git On Sun, Dec 19, 2010 at 05:13:13PM +0000, maximilian attems wrote: > On Sun, Dec 19, 2010 at 02:56:58PM -0200, Thiago Farina wrote: > > On Sun, Dec 19, 2010 at 2:17 PM, maximilian attems <max@stro.at> wrote: > > > When porting patches from dash git to klibc git, > > > where dash has a different directory structure those > > > switches are handy: > > > Exported with format-patch on dash side and used am > > > as import for klibc side. > > > > > > > I don't know, but this commit message doesn't look good. Can you be > > more specific about what the patch does, and what it's trying to fix. > > hmm, it does what the subject says. (: > have you ever used `git am'? > ... > Please be more specific on what's missing? Your personal taste is > unknown to me and not of importance. I had to read it twice to get it.. Most likely because I read the changelog without reading the title first. Sam ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [klibc] [PATCH] am: Allow passing exclude and include args to apply 2010-12-19 17:26 ` [klibc] " Sam Ravnborg @ 2010-12-19 18:27 ` maximilian attems 0 siblings, 0 replies; 7+ messages in thread From: maximilian attems @ 2010-12-19 18:27 UTC (permalink / raw) To: Sam Ravnborg; +Cc: Thiago Farina, klibc, Junio C Hamano, git On Sun, Dec 19, 2010 at 06:26:57PM +0100, Sam Ravnborg wrote: > > I had to read it twice to get it.. > Most likely because I read the changelog without reading the title first. > > Sam thanks, fixed in v2. -- maks ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] am: Allow passing exclude and include args to apply 2010-12-19 17:13 ` maximilian attems 2010-12-19 17:26 ` [klibc] " Sam Ravnborg @ 2010-12-19 20:13 ` Junio C Hamano 2011-01-29 19:31 ` maximilian attems 1 sibling, 1 reply; 7+ messages in thread From: Junio C Hamano @ 2010-12-19 20:13 UTC (permalink / raw) To: maximilian attems; +Cc: Thiago Farina, git, klibc maximilian attems <max@stro.at> writes: > On Sun, Dec 19, 2010 at 02:56:58PM -0200, Thiago Farina wrote: >> On Sun, Dec 19, 2010 at 2:17 PM, maximilian attems <max@stro.at> wrote: >> > When porting patches from dash git to klibc git, >> > where dash has a different directory structure those >> > switches are handy: >> > Exported with format-patch on dash side and used am >> > as import for klibc side. >> > > ... > when one wants to promote a specific new feature, it is much better to > come up with it's use case, as burden is on Maintainer to keep it working. You need to do that with test suite, not with the log message. Otherwise you are adding undue burden on the Maintainer to download klibc and dash just to run regression testing whenever somebody else makes changes to "am/apply" callchain down the road. While I love patches that are backed by a strong "here is a real-world problem we needed to solve, and this change made our life much easier by doing so-and-so" statement, I also tend to think twice before considering a change that could potentially encourage a bad version control discipline. Your use case description in the log message however lacks crucial information to be useful when judging that aspect. You said that the directory structure is "different", but didn't say they are different in what way. In order to skip one mail exchange turnaround, I'd speculate. If dash repository keeps (perhaps slightly stale version of) the same files as klibc repository in its libc/ subdirectory, a patch to dash that fixes its libc part may all have its pathnames prefixed with libc/. In order to apply such a patch to the klibc tree, you would need to give -p2 to strip one extra level (if you are going the other way, you would instead give --directory=libc/ to deepen it). But then I do not see a need for --exclude to remove parts from the patch that touch outside of libc/ tree. If the dash patch you needed to deal with touched both inside libc/ and outside, and if you are taking only libc/ part and discarding everything else, I see two issues with respect to promoting pottentially bad version control disciplines. - Should you be reusing the information in the commit without editing? I am not worried about Signed-off-by which is about asserting the origin, and origin of the libc/ part is the same as the origin of the whole. But what about reviewers' and tester's assertion at the end? Also the description of the change itself may need to be adjusted to the new context you are reusing the change for. - Why does the patch touch two unrelated parts in the first place, if its libc/ part can stand on its own? This is not about the discipline of the user of "am", but of the originating project. Another thing that came to my mind around the vague "different directory structure" is this question: what if directory A/ in "dash" corresponded to directory B/ in "klibc" and you saw a patch to A/ (and some others) for "dash" that you wanted to reuse in "klibc"? Do we need more changes to make it work, or do we already have enough support for this combination? I would imagine that "git am --directory=B/ -p2 --exclude=\* --include=A/" or something like that should work, but I didn't think it through nor I didn't check the command line syntax, either. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] am: Allow passing exclude and include args to apply 2010-12-19 20:13 ` Junio C Hamano @ 2011-01-29 19:31 ` maximilian attems 0 siblings, 0 replies; 7+ messages in thread From: maximilian attems @ 2011-01-29 19:31 UTC (permalink / raw) To: Junio C Hamano; +Cc: Thiago Farina, git, klibc hello, sorry for the time it took to reply, but "things" went crazy around christmas. On Sun, 19 Dec 2010, Junio C Hamano wrote: > maximilian attems <max@stro.at> writes: > > > ... > > when one wants to promote a specific new feature, it is much better to > > come up with it's use case, as burden is on Maintainer to keep it working. > > You need to do that with test suite, not with the log message. Otherwise > you are adding undue burden on the Maintainer to download klibc and dash > just to run regression testing whenever somebody else makes changes to > "am/apply" callchain down the road. ok thanks for the point. > While I love patches that are backed by a strong "here is a real-world > problem we needed to solve, and this change made our life much easier by > doing so-and-so" statement, I also tend to think twice before considering > a change that could potentially encourage a bad version control > discipline. sure, one sees your careful planning. > Your use case description in the log message however lacks crucial > information to be useful when judging that aspect. You said that the > directory structure is "different", but didn't say they are different in > what way. In order to skip one mail exchange turnaround, I'd speculate. sorry about that, will answer below. > If dash repository keeps (perhaps slightly stale version of) the same > files as klibc repository in its libc/ subdirectory, a patch to dash that > fixes its libc part may all have its pathnames prefixed with libc/. In > order to apply such a patch to the klibc tree, you would need to give -p2 > to strip one extra level (if you are going the other way, you would > instead give --directory=libc/ to deepen it). But then I do not see a > need for --exclude to remove parts from the patch that touch outside of > libc/ tree. > > If the dash patch you needed to deal with touched both inside libc/ and > outside, and if you are taking only libc/ part and discarding everything > else, I see two issues with respect to promoting pottentially bad version > control disciplines. > > - Should you be reusing the information in the commit without editing? I > am not worried about Signed-off-by which is about asserting the origin, > and origin of the libc/ part is the same as the origin of the whole. > But what about reviewers' and tester's assertion at the end? Also the > description of the change itself may need to be adjusted to the new > context you are reusing the change for. > > - Why does the patch touch two unrelated parts in the first place, if its > libc/ part can stand on its own? This is not about the discipline of > the user of "am", but of the originating project. Life is simpler in effect, considering this usecase. So the code lives in dash repo and from to time is ported to klibc. So dash has no klibc code on it's own it's only klibc, which is carrying a copy of dash in order to have an useful shell for early userspace. It is not a full copy of dash and some things are not their, that's the usage case for the exclude argument on the git am call. > Another thing that came to my mind around the vague "different directory > structure" is this question: what if directory A/ in "dash" corresponded > to directory B/ in "klibc" and you saw a patch to A/ (and some others) for > "dash" that you wanted to reuse in "klibc"? Do we need more changes to > make it work, or do we already have enough support for this combination? > > I would imagine that "git am --directory=B/ -p2 --exclude=\* --include=A/" > or something like that should work, but I didn't think it through nor I > didn't check the command line syntax, either. well mostly the current workflow looks like this: 1) Generate patches to import on dash git repository: git format-patch --keep-subject <changeset>.. Path fixup: perl -i -pe 's#^([-+]{3} [ab]/)src/#$1#g' 00*patch 2) Import patches on by one git am --directory="usr/dash" --exclude="usr/dash/configure.ac" \ --exclude="usr/dash/ChangeLog" --exclude="usr/dash/dash.1" \ --exclude="usr/dash/Makefile.am" --exclude="usr/dash/mksignames.c" \ -i -s ../dash/000X-foo.patch the path fixup with perl is needed as dash code lives in "src/", but later it lands in "usr/dash/". I do not have a use case for --include, I only added it out of completness. Unless objection I'd repost the patch with exclude only and relevant testcase. Of course if you'd know of a trick that makes the perl path mangling useless I'd be more than happy to hear. thank you. -- maks ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2011-01-29 19:31 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2010-12-19 16:17 [PATCH] am: Allow passing exclude and include args to apply maximilian attems 2010-12-19 16:56 ` Thiago Farina 2010-12-19 17:13 ` maximilian attems 2010-12-19 17:26 ` [klibc] " Sam Ravnborg 2010-12-19 18:27 ` maximilian attems 2010-12-19 20:13 ` Junio C Hamano 2011-01-29 19:31 ` maximilian attems
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.