* [PATCH] Add explicit --src/dst-prefix to git-formt-patch in git-rebase.sh for the case of "diff.noprefix" in git-config
@ 2010-09-08 13:54 Oded Shimon
2010-09-08 20:31 ` ods15
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Oded Shimon @ 2010-09-08 13:54 UTC (permalink / raw)
To: git
---
git-rebase.sh | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/git-rebase.sh b/git-rebase.sh
index 7508463..e83a0cf 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -565,7 +565,7 @@ fi
if test -z "$do_merge"
then
- git format-patch -k --stdout --full-index --ignore-if-in-upstream \
+ git format-patch -k --stdout --full-index --ignore-if-in-upstream --src-prefix=a/ --dst-prefix=b/ \
--no-renames $root_flag "$revisions" |
git am $git_am_opt --rebasing --resolvemsg="$RESOLVEMSG" &&
move_to_original_branch
--
1.6.4.4
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] Add explicit --src/dst-prefix to git-formt-patch in git-rebase.sh for the case of "diff.noprefix" in git-config
2010-09-08 13:54 [PATCH] Add explicit --src/dst-prefix to git-formt-patch in git-rebase.sh for the case of "diff.noprefix" in git-config Oded Shimon
@ 2010-09-08 20:31 ` ods15
2010-09-08 21:07 ` Jan Krüger
2010-09-09 8:07 ` [PATCH] Add --src/dst-prefix to git-formt-patch in git-rebase.sh Oded Shimon
2 siblings, 0 replies; 11+ messages in thread
From: ods15 @ 2010-09-08 20:31 UTC (permalink / raw)
To: git
On Wed, Sep 08, 2010 at 04:54:05PM +0300, Oded Shimon wrote:
> ---
> git-rebase.sh | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/git-rebase.sh b/git-rebase.sh
> index 7508463..e83a0cf 100755
> --- a/git-rebase.sh
> +++ b/git-rebase.sh
> @@ -565,7 +565,7 @@ fi
>
> if test -z "$do_merge"
> then
> - git format-patch -k --stdout --full-index --ignore-if-in-upstream \
> + git format-patch -k --stdout --full-index --ignore-if-in-upstream --src-prefix=a/ --dst-prefix=b/ \
> --no-renames $root_flag "$revisions" |
> git am $git_am_opt --rebasing --resolvemsg="$RESOLVEMSG" &&
> move_to_original_branch
Hi.
I may have rudely just sent a patch without explaining anything, I thought
the patch was self explanitory. I'm not sure on the expected procedure
here for sending patches.
When using "git rebase", when "diff.noprefix" is set to true in
git-config, then all the rebases mess up because they get the directory
path wrong.
As far as I can tell, my patch fixes this, with no side-effects that I can
think of.
Can anyone comment on the patch? Can it be pushed upstream?
- ods15
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] Add explicit --src/dst-prefix to git-formt-patch in git-rebase.sh for the case of "diff.noprefix" in git-config
2010-09-08 13:54 [PATCH] Add explicit --src/dst-prefix to git-formt-patch in git-rebase.sh for the case of "diff.noprefix" in git-config Oded Shimon
2010-09-08 20:31 ` ods15
@ 2010-09-08 21:07 ` Jan Krüger
2010-09-09 8:07 ` [PATCH] Add --src/dst-prefix to git-formt-patch in git-rebase.sh Oded Shimon
2 siblings, 0 replies; 11+ messages in thread
From: Jan Krüger @ 2010-09-08 21:07 UTC (permalink / raw)
To: Oded Shimon; +Cc: git
Oded Shimon <ods15@ods15.dyndns.org> wrote:
> ---
A shorter summary (subject) and more of a commit message body might be a
good idea. People tend to like patch summaries that fit into one line
on a terminal, and shorter is even better for those tools that output
additional fields on the same line.
Signoff is missing, please see Documentation/SubmittingPatches for
details (including the reason why we need one in the first place).
I can't comment on the patch itself since I'm not familiar with the
whole diff machinery nor the innards of rebase.
> git-rebase.sh | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/git-rebase.sh b/git-rebase.sh
> index 7508463..e83a0cf 100755
> --- a/git-rebase.sh
> +++ b/git-rebase.sh
> @@ -565,7 +565,7 @@ fi
>
> if test -z "$do_merge"
> then
> - git format-patch -k --stdout --full-index
> --ignore-if-in-upstream \
> + git format-patch -k --stdout --full-index
> --ignore-if-in-upstream --src-prefix=a/ --dst-prefix=b/ \
> --no-renames $root_flag "$revisions" | git am $git_am_opt --rebasing
> --resolvemsg="$RESOLVEMSG" && move_to_original_branch
-Jan
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] Add --src/dst-prefix to git-formt-patch in git-rebase.sh
2010-09-08 13:54 [PATCH] Add explicit --src/dst-prefix to git-formt-patch in git-rebase.sh for the case of "diff.noprefix" in git-config Oded Shimon
2010-09-08 20:31 ` ods15
2010-09-08 21:07 ` Jan Krüger
@ 2010-09-09 8:07 ` Oded Shimon
2010-09-09 8:36 ` [Alt. PATCH] format-patch: do not use diff UI config Thomas Rast
2010-09-09 18:35 ` [PATCH] Add --src/dst-prefix to git-formt-patch in git-rebase.sh Junio C Hamano
2 siblings, 2 replies; 11+ messages in thread
From: Oded Shimon @ 2010-09-09 8:07 UTC (permalink / raw)
To: git
For the case of "diff.noprefix" in git-config, git-format-patch should
still output diff with standard prefixes for git-am
Signed-off-by: Oded Shimon <ods15@ods15.dyndns.org>
---
git-rebase.sh | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/git-rebase.sh b/git-rebase.sh
index 7508463..e83a0cf 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -565,7 +565,7 @@ fi
if test -z "$do_merge"
then
- git format-patch -k --stdout --full-index --ignore-if-in-upstream \
+ git format-patch -k --stdout --full-index --ignore-if-in-upstream --src-prefix=a/ --dst-prefix=b/ \
--no-renames $root_flag "$revisions" |
git am $git_am_opt --rebasing --resolvemsg="$RESOLVEMSG" &&
move_to_original_branch
--
1.6.4.4
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [Alt. PATCH] format-patch: do not use diff UI config
2010-09-09 8:07 ` [PATCH] Add --src/dst-prefix to git-formt-patch in git-rebase.sh Oded Shimon
@ 2010-09-09 8:36 ` Thomas Rast
2010-09-09 19:13 ` Sverre Rabbelier
` (2 more replies)
2010-09-09 18:35 ` [PATCH] Add --src/dst-prefix to git-formt-patch in git-rebase.sh Junio C Hamano
1 sibling, 3 replies; 11+ messages in thread
From: Thomas Rast @ 2010-09-09 8:36 UTC (permalink / raw)
To: Oded Shimon; +Cc: Junio C Hamano, Jan Krüger, git
format-patch read and used the diff UI config, such as diff.renames,
diff.noprefix and diff.mnemnoicprefix. These have a history of
breaking rebase and patch application in general; cf. 840b3ca (rebase:
protect against diff.renames configuration, 2008-11-10).
Instead of continually putting more options inside git-rebase to avoid
these issues, this patch takes the stance that output from
format-patch is intended primarily for git-am and only as a side
effect also for human consumption. Hence, ignore the diff UI config
entirely when coming from format-patch.
Note that all existing calls to git_log_config except for the one in
git_format_config use a NULL callback.
Reported-by: Oded Shimon <ods15@ods15.dyndns.org>
Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
This is a bolder approach that just outright ignores the backwards
compatibility complaints Junio had in 840b3ca. Among the variables
parsed in git_diff_ui_config, namely
color.diff (and its legacy alias diff.color)
diff.renames
diff.autorefreshindex
diff.mnemonicprefix
diff.noprefix
diff.external
diff.wordregex
diff.ignoresubmodules
arguably only diff.renames (and perhaps diff.ignoresubmodules, I don't
use them) should affect format-patch. Everything else undermines the
guarantee (by having a consistent format) that format-patch|am works.
So now I'm not so sure about diff.renames. Perhaps it needs to be
retained, but that requires a special case since we cannot move it to
git_diff_basic_config() (which affects diff-* plumbing too).
In any case I also made a test. If you decide to go for the original
patch, please feel free to "steal" it.
builtin/log.c | 20 ++++++++++++++++++--
t/t3400-rebase.sh | 7 +++++++
2 files changed, 25 insertions(+), 2 deletions(-)
diff --git a/builtin/log.c b/builtin/log.c
index f2d9d61..a1079fe 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -385,8 +385,19 @@ static int cmd_log_walk(struct rev_info *rev)
return diff_result_code(&rev->diffopt, 0);
}
+struct log_config_cb_data
+{
+ /*
+ * If no_diff_ui_config is set, we use diff_basic_config
+ * instead, ignoring the plumbing-specific UI settings.
+ */
+ int no_diff_ui_config;
+};
+
static int git_log_config(const char *var, const char *value, void *cb)
{
+ struct log_config_cb_data *cb_data = cb;
+
if (!strcmp(var, "format.pretty"))
return git_config_string(&fmt_pretty, var, value);
if (!strcmp(var, "format.subjectprefix"))
@@ -406,7 +417,10 @@ static int git_log_config(const char *var, const char *value, void *cb)
if (!prefixcmp(var, "color.decorate."))
return parse_decorate_color_config(var, 15, value);
- return git_diff_ui_config(var, value, cb);
+ if (!cb_data || !cb_data->no_diff_ui_config)
+ return git_diff_ui_config(var, value, cb);
+ else
+ return git_diff_basic_config(var, value, cb);
}
int cmd_whatchanged(int argc, const char **argv, const char *prefix)
@@ -1099,6 +1113,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
char *add_signoff = NULL;
struct strbuf buf = STRBUF_INIT;
int use_patch_format = 0;
+ struct log_config_cb_data config_cb_data;
const struct option builtin_format_patch_options[] = {
{ OPTION_CALLBACK, 'n', "numbered", &numbered, NULL,
"use [PATCH n/m] even with a single patch",
@@ -1160,7 +1175,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
extra_hdr.strdup_strings = 1;
extra_to.strdup_strings = 1;
extra_cc.strdup_strings = 1;
- git_config(git_format_config, NULL);
+ config_cb_data.no_diff_ui_config = 1;
+ git_config(git_format_config, &config_cb_data);
init_revisions(&rev, prefix);
rev.commit_format = CMIT_FMT_EMAIL;
rev.verbose_header = 1;
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index 349eebd..0e2fe71 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -144,6 +144,13 @@ test_expect_success 'rebase is not broken by diff.renames' '
GIT_TRACE=1 git rebase force-3way
'
+test_expect_success 'rebase is not broken by diff.noprefix' '
+ git config diff.noprefix true &&
+ test_when_finished "git config --unset diff.noprefix" &&
+ git checkout -b noprefix side &&
+ GIT_TRACE=1 git rebase master
+'
+
test_expect_success 'setup: recover' '
test_might_fail git rebase --abort &&
git reset --hard &&
--
1.7.3.rc0.289.gcd076
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] Add --src/dst-prefix to git-formt-patch in git-rebase.sh
2010-09-09 8:07 ` [PATCH] Add --src/dst-prefix to git-formt-patch in git-rebase.sh Oded Shimon
2010-09-09 8:36 ` [Alt. PATCH] format-patch: do not use diff UI config Thomas Rast
@ 2010-09-09 18:35 ` Junio C Hamano
2010-09-09 18:49 ` ods15
2010-09-09 18:49 ` Oded Shimon
1 sibling, 2 replies; 11+ messages in thread
From: Junio C Hamano @ 2010-09-09 18:35 UTC (permalink / raw)
To: Oded Shimon; +Cc: git
Oded Shimon <ods15@ods15.dyndns.org> writes:
> For the case of "diff.noprefix" in git-config, git-format-patch should
> still output diff with standard prefixes for git-am
>
> Signed-off-by: Oded Shimon <ods15@ods15.dyndns.org>
Hmm.
> diff --git a/git-rebase.sh b/git-rebase.sh
> index 7508463..e83a0cf 100755
> --- a/git-rebase.sh
> +++ b/git-rebase.sh
> @@ -565,7 +565,7 @@ fi
>
> if test -z "$do_merge"
> then
> - git format-patch -k --stdout --full-index --ignore-if-in-upstream \
> + git format-patch -k --stdout --full-index --ignore-if-in-upstream --src-prefix=a/ --dst-prefix=b/ \
> --no-renames $root_flag "$revisions" |
> git am $git_am_opt --rebasing --resolvemsg="$RESOLVEMSG" &&
As the format-patch invocation is already multi-line, you probably would
want to use a continuation line with "\" to keep the line length shorter.
We need to protect ourselves from crazy people, so regrettably something
like this patch is unavoidable, albeit unsightly.
Thanks.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] Add --src/dst-prefix to git-formt-patch in git-rebase.sh
2010-09-09 18:35 ` [PATCH] Add --src/dst-prefix to git-formt-patch in git-rebase.sh Junio C Hamano
@ 2010-09-09 18:49 ` ods15
2010-09-09 18:49 ` Oded Shimon
1 sibling, 0 replies; 11+ messages in thread
From: ods15 @ 2010-09-09 18:49 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
On Thu, Sep 09, 2010 at 11:35:19AM -0700, Junio C Hamano wrote:
> Oded Shimon <ods15@ods15.dyndns.org> writes:
>
> > For the case of "diff.noprefix" in git-config, git-format-patch should
> > still output diff with standard prefixes for git-am
> >
> > Signed-off-by: Oded Shimon <ods15@ods15.dyndns.org>
>
> Hmm.
Anything wrong?
> > diff --git a/git-rebase.sh b/git-rebase.sh
> > index 7508463..e83a0cf 100755
> > --- a/git-rebase.sh
> > +++ b/git-rebase.sh
> > @@ -565,7 +565,7 @@ fi
> >
> > if test -z "$do_merge"
> > then
> > - git format-patch -k --stdout --full-index --ignore-if-in-upstream \
> > + git format-patch -k --stdout --full-index --ignore-if-in-upstream --src-prefix=a/ --dst-prefix=b/ \
> > --no-renames $root_flag "$revisions" |
> > git am $git_am_opt --rebasing --resolvemsg="$RESOLVEMSG" &&
>
> As the format-patch invocation is already multi-line, you probably would
> want to use a continuation line with "\" to keep the line length shorter.
Will do.
> We need to protect ourselves from crazy people, so regrettably something
> like this patch is unavoidable, albeit unsightly.
I am one of those crazy people (hence noticing the bug). I constantly copy
paste the filenames from diffs in order to write them in command line,
with mouse double-click which grabs the entire path/filename...
- ods15
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] Add --src/dst-prefix to git-formt-patch in git-rebase.sh
2010-09-09 18:35 ` [PATCH] Add --src/dst-prefix to git-formt-patch in git-rebase.sh Junio C Hamano
2010-09-09 18:49 ` ods15
@ 2010-09-09 18:49 ` Oded Shimon
1 sibling, 0 replies; 11+ messages in thread
From: Oded Shimon @ 2010-09-09 18:49 UTC (permalink / raw)
To: git
For the case of "diff.noprefix" in git-config, git-format-patch should
still output diff with standard prefixes for git-am
Signed-off-by: Oded Shimon <ods15@ods15.dyndns.org>
---
git-rebase.sh | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/git-rebase.sh b/git-rebase.sh
index 7508463..3335cee 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -566,6 +566,7 @@ fi
if test -z "$do_merge"
then
git format-patch -k --stdout --full-index --ignore-if-in-upstream \
+ --src-prefix=a/ --dst-prefix=b/ \
--no-renames $root_flag "$revisions" |
git am $git_am_opt --rebasing --resolvemsg="$RESOLVEMSG" &&
move_to_original_branch
--
1.6.4.4
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [Alt. PATCH] format-patch: do not use diff UI config
2010-09-09 8:36 ` [Alt. PATCH] format-patch: do not use diff UI config Thomas Rast
@ 2010-09-09 19:13 ` Sverre Rabbelier
2010-09-09 19:43 ` Jeff King
2010-09-10 16:21 ` Junio C Hamano
2 siblings, 0 replies; 11+ messages in thread
From: Sverre Rabbelier @ 2010-09-09 19:13 UTC (permalink / raw)
To: Thomas Rast; +Cc: Oded Shimon, Junio C Hamano, Jan Krüger, git
Heya,
On Thu, Sep 9, 2010 at 03:36, Thomas Rast <trast@student.ethz.ch> wrote:
> +test_expect_success 'rebase is not broken by diff.noprefix' '
> + git config diff.noprefix true &&
> + test_when_finished "git config --unset diff.noprefix" &&
> + git checkout -b noprefix side &&
> + GIT_TRACE=1 git rebase master
> +'
> +
Can we have one for 'git config diff.color true' too?
--
Cheers,
Sverre Rabbelier
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Alt. PATCH] format-patch: do not use diff UI config
2010-09-09 8:36 ` [Alt. PATCH] format-patch: do not use diff UI config Thomas Rast
2010-09-09 19:13 ` Sverre Rabbelier
@ 2010-09-09 19:43 ` Jeff King
2010-09-10 16:21 ` Junio C Hamano
2 siblings, 0 replies; 11+ messages in thread
From: Jeff King @ 2010-09-09 19:43 UTC (permalink / raw)
To: Thomas Rast; +Cc: Oded Shimon, Junio C Hamano, Jan Krüger, git
On Thu, Sep 09, 2010 at 10:36:54AM +0200, Thomas Rast wrote:
> format-patch read and used the diff UI config, such as diff.renames,
> diff.noprefix and diff.mnemnoicprefix. These have a history of
> breaking rebase and patch application in general; cf. 840b3ca (rebase:
> protect against diff.renames configuration, 2008-11-10).
>
> Instead of continually putting more options inside git-rebase to avoid
> these issues, this patch takes the stance that output from
> format-patch is intended primarily for git-am and only as a side
> effect also for human consumption. Hence, ignore the diff UI config
> entirely when coming from format-patch.
>
> Note that all existing calls to git_log_config except for the one in
> git_format_config use a NULL callback.
This was my first thought upon reading Oded's patch, too. We would want
to cut out anything that will cause format-patch to create a patch that
could not be applied. So from your list:
> This is a bolder approach that just outright ignores the backwards
> compatibility complaints Junio had in 840b3ca. Among the variables
> parsed in git_diff_ui_config, namely
>
> color.diff (and its legacy alias diff.color)
> diff.renames
> diff.autorefreshindex
> diff.mnemonicprefix
> diff.noprefix
> diff.external
> diff.wordregex
> diff.ignoresubmodules
>
> arguably only diff.renames (and perhaps diff.ignoresubmodules, I don't
> use them) should affect format-patch. Everything else undermines the
> guarantee (by having a consistent format) that format-patch|am works.
I would agree that diff.renames should probably be the only thing we
want to allow (because it is not about making a broken diff, but because
the receiver may or may not support it, and we already know that
git-rebase will handle it).
diff.external is debatable. If your external diff is producing real,
applicable diffs, then it is fine to use it. I have to wonder why you
would use an external diff, then. I guess because it's faster, or maybe
has an algorithm that produces equivalent but easier-to-read results
(e.g., patience before we had --patience)?
> So now I'm not so sure about diff.renames. Perhaps it needs to be
> retained, but that requires a special case since we cannot move it to
> git_diff_basic_config() (which affects diff-* plumbing too).
I think it is reasonable to just move an explicit "diff.renames" check
into format_patch, and then set the diff_options appropriately. It
requires special case code because it _is_ a special case.
-Peff
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Alt. PATCH] format-patch: do not use diff UI config
2010-09-09 8:36 ` [Alt. PATCH] format-patch: do not use diff UI config Thomas Rast
2010-09-09 19:13 ` Sverre Rabbelier
2010-09-09 19:43 ` Jeff King
@ 2010-09-10 16:21 ` Junio C Hamano
2 siblings, 0 replies; 11+ messages in thread
From: Junio C Hamano @ 2010-09-10 16:21 UTC (permalink / raw)
To: Thomas Rast; +Cc: Oded Shimon, Jan Krüger, git
Thomas Rast <trast@student.ethz.ch> writes:
> ...
> arguably only diff.renames (and perhaps diff.ignoresubmodules, I don't
> use them) should affect format-patch. Everything else undermines the
> guarantee (by having a consistent format) that format-patch|am works.
We need to be a bit careful here.
Each user must be able to find a combination of ($opts1, $opts2) to make
"format-patch $opts1 | am $opts2" run correctly with his funny settings
(e.g. diff.noprefix). We must guarantee that [*1*].
I however don't think we need to guarantee that the pipeline always works
for empty opts1/2, and certainly we shouldn't insist what flows in that
pipe must be the bog-standard -p1 with a/ b/ prefix patch. For example,
in circles under svn influence, people may prefer opts1=--no-prefix, and
as long as the recipient understands that is the community norm around
there, he can run his "am" with -p0 and everything should work. It is not
unreasonable for the sender to have diff.noprefix in the repository config
in such a setup, don't you think?
There is no way to easily affect what options the "format-patch | am"
pipeline uses inside rebase. It may make sense to introduce --rebasing
option to format-patch to cause it to ignore any funny setting the user
might have, so that we don't have to keep adding options to the command
invocation. "am" has --rebasing already, and it may be beneficial to
teach the codepath to defeat some configuration variables in a similar
way.
[Footnote]
*1* ... within reason. For example, I don't think there is no opts2 if
you had opts1="--src-prefix=a/ --dst-prefix=b/c/" that makes the pipeline
work reasonably.
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2010-09-10 16:21 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-09-08 13:54 [PATCH] Add explicit --src/dst-prefix to git-formt-patch in git-rebase.sh for the case of "diff.noprefix" in git-config Oded Shimon
2010-09-08 20:31 ` ods15
2010-09-08 21:07 ` Jan Krüger
2010-09-09 8:07 ` [PATCH] Add --src/dst-prefix to git-formt-patch in git-rebase.sh Oded Shimon
2010-09-09 8:36 ` [Alt. PATCH] format-patch: do not use diff UI config Thomas Rast
2010-09-09 19:13 ` Sverre Rabbelier
2010-09-09 19:43 ` Jeff King
2010-09-10 16:21 ` Junio C Hamano
2010-09-09 18:35 ` [PATCH] Add --src/dst-prefix to git-formt-patch in git-rebase.sh Junio C Hamano
2010-09-09 18:49 ` ods15
2010-09-09 18:49 ` Oded Shimon
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.