All of lore.kernel.org
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: maximilian attems <max@stro.at>
Cc: Thiago Farina <tfransosi@gmail.com>,
	git@vger.kernel.org, klibc@zytor.com
Subject: Re: [PATCH] am: Allow passing exclude and include args to apply
Date: Sun, 19 Dec 2010 12:13:16 -0800	[thread overview]
Message-ID: <7v4oa9frmr.fsf@alter.siamese.dyndns.org> (raw)
In-Reply-To: <20101219171313.GI17034@vostochny.stro.at> (maximilian attems's message of "Sun\, 19 Dec 2010 17\:13\:13 +0000")

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.

  parent reply	other threads:[~2010-12-19 20:15 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
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 [this message]
2011-01-29 19:31       ` maximilian attems

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=7v4oa9frmr.fsf@alter.siamese.dyndns.org \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=klibc@zytor.com \
    --cc=max@stro.at \
    --cc=tfransosi@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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.