Re: Regulator updates for 3.3

Re: Regulator updates for 3.3

[Linus Torvalds]

On Tue, Jan 10, 2012 at 2:27 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
>
> Especially in the cases where the lack of the bug fix breaks the new
> code it sems sensible enough to want to do the merges so that the
> history includes things that actually work.

So I don't mind merges if they have a lear reason for existing.

This is actually one of my major gripes with the git UI, and one of
the few areas where I really think I screwed up: I made merging *too*
easy by default. I should have made it always start up an editor for a
merge message, the way it does for a commit - rather than just do a
trivial pointless merge without even asking the user for a reason for
the merge.

So looking at that almost two months of regulator history in

   gitk d52739c62e00..269d430131b6

I would not have reacted badly at all if there were one or two of
those merges, and they actually had a reason associated with them.
Sadly, due to that git UI mess-up, that's harder to do than it should
be. Oh, it's easy enough with "git merge --no-commit" followed by just
"git commit", and then you get the normal git editor window.

So right now "git merge" (and "git pull") make it too easy to make
those meaningless merge commits. If instead of seven pointless merges
you had (say) had two merges that had messages about *why* they
weren't pointless, I'd be perfectly happy.

Addid junio and git to the cc just to bring up this issue of bad UI
once again. I realize it could break old scripts to start up an editor
window, but still..

                       Linus

Re: Regulator updates for 3.3

[Mark Brown]

On Tue, Jan 10, 2012 at 02:54:27PM -0800, Linus Torvalds wrote:
> On Tue, Jan 10, 2012 at 2:27 PM, Mark Brown

> > Especially in the cases where the lack of the bug fix breaks the new
> > code it sems sensible enough to want to do the merges so that the
> > history includes things that actually work.

> So I don't mind merges if they have a lear reason for existing.

OK, good - I figured that was the case but wanted to make sure as you
were stating things rather more strongly than that.

Just to warn you there's also a whole stack of similar merges going to
come in via the sound tree too due to the same workflow, I *could* try
to rebuild the history and ask Takashi to redo his tree using that but
there's a lot of history there and it'd be hard to figure out which of
the merges was actually important.  Is it OK to leave things as they are
for this release?

> So right now "git merge" (and "git pull") make it too easy to make
> those meaningless merge commits. If instead of seven pointless merges
> you had (say) had two merges that had messages about *why* they
> weren't pointless, I'd be perfectly happy.

> Addid junio and git to the cc just to bring up this issue of bad UI
> once again. I realize it could break old scripts to start up an editor
> window, but still..

I'd use a configuration option that popped up an editor by default, even
if I did have to manually enable it.

Re: Regulator updates for 3.3

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> Addid junio and git to the cc just to bring up this issue of bad UI
> once again. I realize it could break old scripts to start up an editor
> window, but still..

It is a non-starter to unconditionally start an editor. We would need a
good way for users to conveniently say "I am doing this unusual merge that
needs to be justified, and I want an editor to write my justification".

Obviously, "git merge -e regulator/for-linus" would work and is just three
keystrokes, which can be said "convenient enough" once the user gets used
to, but I think this is still inadequate as a solution, as the real
problem is it is _too_ easy to forget to give the option.  Until the user
becomes _aware_ of the issues, it will not even occur to the user that
s/he _has_ to justify a merge (or not create a merge at all) in certain
circumstances and directions.  After all, you have been repeating the "do
not make meaningless merges" for the past five years on the list. UI tweak
alone will not fix that.

If we are to rely on user's conscious action, I think it may be something
like a set of configurations that say things like:

 - This branch is for advancing a specific topic, and not for merging
   random development that happen elsewhere;

 - This branch is for merging works by people downstream from me;

 - This remote tracking branch (and by extension that branch at that
   remote that uses this as its remote tracking branch) is my upstream and
   I should not be merging back from it; and

 - This remote tracking branch is my downstream, and I should freely merge
   it when I heard it is ready.

and depending on the combination of what is being merged into what, toggle
the --edit option by default for "git merge" when neither "--edit" nor
"--no-edit" is given, just like "git merge" defaults to "--edit" when
merging an annotated tag.

Re: Regulator updates for 3.3

[Linus Torvalds]

On Tue, Jan 10, 2012 at 6:28 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> It is a non-starter to unconditionally start an editor.

I really wonder. Because not being default will always lead to really
odd ways of saying "it should have been default, so we'll make up
these complex and arbitrary special rules" (like the ones you were
starting to outline).

So I really suspect it would be easier and more straightforward to
instead just bite the bullet, and say:

 (a) start an editor by default if both stdin/stdout matched in fstat
and were istty().

 (b) have some trivial way to disable that default behavior for people
who really want the legacy behavior. And by "trivial" I mean "set the
GIT_LEGACY_MERGE environment variable" or something.

 (c) have a "--no-editor" command line switch so that scripts and/or
users that want to make it explicit (rather than rely on the hacky
legacy workaround) can do so (and a explicit "--editor" switch to
enable people to use a GUI editor even if they aren't on a terminal -
think something IDE environment, whatever).

Where (a) is so that people will always get the editor if they aren't
aware of it, and (b) is so that existing scripting environments can
then *trivially* work around the fact that we changed semantics,
including on a site-wide basis. With (c) being for future users. Of
course, just a "git merge < /dev/null" would also do it, but sounds
ridiculously hacky (and doesn't allow the "--editor" version), so that
"--no-editor" flag sounds saner and much more powerful.

Of course, if you use "-m", no editor would fire up anyway, exactly
like with "git commit", so that's one way to avoid the issue forever
(and be backwards compatible). But if you actually *want* to get the
auto-generated message and no editor, that would need that new switch.

Yes, git has been very good about not breaking semantics. But it's
happened before too when it needed to happen. We've had much bigger
breaks (like the whole "git-xyz" to "git xyz" transition, for example,
which broke a lot of scripts).

                       Linus

Re: Regulator updates for 3.3

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Tue, Jan 10, 2012 at 6:28 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> It is a non-starter to unconditionally start an editor.
>
> I really wonder. Because not being default will always lead to really
> odd ways of saying "it should have been default, so we'll make up
> these complex and arbitrary special rules" (like the ones you were
> starting to outline).
>
> So I really suspect it would be easier and more straightforward to
> instead just bite the bullet, and say:
>
>  (a) start an editor by default if both stdin/stdout matched in fstat
> and were istty().
>
>  (b) have some trivial way to disable that default behavior for people
> who really want the legacy behavior. And by "trivial" I mean "set the
> GIT_LEGACY_MERGE environment variable" or something.
>
>  (c) have a "--no-editor" command line switch so that scripts and/or
> users that want to make it explicit (rather than rely on the hacky
> legacy workaround) can do so (and a explicit "--editor" switch to
> enable people to use a GUI editor even if they aren't on a terminal -
> think something IDE environment, whatever).

Hrm. Lack of any quoted line other than the first line from my message,
together with (c) above, makes me suspect that you did not read beyond the
first line before composing this message you are responding to.

> Yes, git has been very good about not breaking semantics. But it's
> happened before too when it needed to happen. We've had much bigger
> breaks (like the whole "git-xyz" to "git xyz" transition, for example,
> which broke a lot of scripts).

Yes, I am learning from the experience to be cautious ;-)

I dunno. You just scrapped the plan for 1.7.10; it may have to be called 2.0
instead.

Re: Regulator updates for 3.3

[Linus Torvalds]

On Tue, Jan 10, 2012 at 7:03 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> I really wonder. Because not being default will always lead to really
>> odd ways of saying "it should have been default, so we'll make up
>> these complex and arbitrary special rules" (like the ones you were
>> starting to outline).
>
> Hrm. Lack of any quoted line other than the first line from my message,
> together with (c) above, makes me suspect that you did not read beyond the
> first line before composing this message you are responding to.

No. See again. I did read your suggestion, and that's where the "we'll
make up these complex and arbitrary special rules" comment comes from.
Did I mis-understand it?

I think it's a *horrible* idea to go down the road or some
branch-specific configurations and then, and I quote:

  "depending on the combination of what is being merged into what,
toggle the --edit option by default"

THAT is the kind of design that sounds crazy.

Instead, just make editing the default. No ifs, buts, or maybe. No
configuration, no complexities - just make it act the same way our
pager logic acts (ie redirecting stdin/stdout obviously shuts down the
pager, and equally obviously needs to shut down the editor).

Then, the --edit/--no-edit flags are for future users that want to
make it explicit. But they aren't about rules, they are about just
making very explicit statements of "I don't want the editor".

The (b) thing I suggested was for "work around for people who have
legacy cases that they don't want to make explicit". I guess you could
count that as some rule, but I really think it's more of a "ok, we had
bad legacy behavior, and now we have scripts that depended on that bad
legacy".

But the notion of complex rules? That sounds really really bad. I'd
much rather get *rid* of the one complex rule we have (the "merging a
tag implies --edit"). That rule is already a hack.

                   Linus

Re: Regulator updates for 3.3

[Linus Torvalds]

On Tue, Jan 10, 2012 at 7:03 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> I dunno. You just scrapped the plan for 1.7.10; it may have to be called 2.0
> instead.

Btw, version numbers are cheap. I already argued for updating to 2.0
just because of the new signed tag pulling, which I think is a much
bigger issue.

I don't think a small change like "start the editor by default for
merge messages" is nearly as worthy of a version number. But I
wouldn't argue against it either, exactly because those major numbers
are cheap.

It took the kernel until 2.6.39 to learn that, I think git could learn
to use its major number more freely much earlier.

                   Linus

Re* Regulator updates for 3.3

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> The (b) thing I suggested was for "work around for people who have
> legacy cases that they don't want to make explicit". I guess you could
> count that as some rule, but I really think it's more of a "ok, we had
> bad legacy behavior, and now we have scripts that depended on that bad
> legacy".

I would think that would solve the issue for scripts like the one used to
rebuild the linux-next tree.  I also have such a script to rebuild 'pu' I
use three or four times a day, but admittedly the standard input of "git
merge" in that script is connected to a here document that feeds the list
of branches to be merged to the loop that drives the "git merge", so your
heuristic (a) will kick in and I wouldn't need the GIT_MERGE_NO_EDIT
environment myself.

What makes me uneasy about the idea of running the editor by default is
that many people still use Git as a better CVS/SVN. Their workflow is to
build randomly on their 'master', attempt to push and get rejected, pull
only so that they can push out, and then push the merge result out. Such
merges are done without any consideration on the cohesiveness of the
branch (the segment of the history that records their work since they last
pulled from the central repository). Having to justify the backmerge is
nothing but a nuisance for them, as they do not have any justification
better than "I am done for the day, and my commuter shuttle will leave in
15 minutes, so I tried to push what I've done so far, but it was rejected
due to non fast-forward, and I am merging random things others did so that
I can push back". They won't be saying "This merges the great work I
completed and have been testing privately for a few days to the trunk", as
the direction of their merge is backwards.

With that caveat, the patch should look like this.

-- >8 --
Subject: [PATCH] merge: use editor by default in interactive sessions

Traditionally, a cleanly resolved merge was committed by "git merge" using
the auto-generated merge commit log message with invoking the editor.

After 5 years of use in the field, it turns out that many people perform
too many unjustified backmerges of the upstream history into their topic
branches. These merges are not just useless, but they are more often than
not explained and making the end result unreadable when it gets time for
merging their history back to their upstream.

Earlier we added the "--edit" option to the command, so that people can
edit the log message to explain and justify their merge commits. Let's
take it one step further and spawn the editor by default when we are in an
interactive session (i.e. the standard input and the standard output are
pointing at the same tty device).

There may be existing scripts that leave the standard input and the
standard output of the "git merge" connected to whatever environment the
scripts were started, and such invocation might trigger the above
"interactive session" heuristics. Such scripts can export GIT_MERGE_LEGACY
environment variable set to "yes" to force the traditional behaviour.

Suggested-by: Linus Torvalds
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/merge.c |   34 ++++++++++++++++++++++++++++++----
 t/test-lib.sh   |    3 ++-
 2 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index 99f1429..6a80e1e 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -46,7 +46,7 @@ static const char * const builtin_merge_usage[] = {
 
 static int show_diffstat = 1, shortlog_len, squash;
 static int option_commit = 1, allow_fast_forward = 1;
-static int fast_forward_only, option_edit;
+static int fast_forward_only, option_edit = -1;
 static int allow_trivial = 1, have_message;
 static struct strbuf merge_msg;
 static struct commit_list *remoteheads;
@@ -189,7 +189,7 @@ static struct option builtin_merge_options[] = {
 		"create a single commit instead of doing a merge"),
 	OPT_BOOLEAN(0, "commit", &option_commit,
 		"perform a commit if the merge succeeds (default)"),
-	OPT_BOOLEAN('e', "edit", &option_edit,
+	OPT_BOOL('e', "edit", &option_edit,
 		"edit message before committing"),
 	OPT_BOOLEAN(0, "ff", &allow_fast_forward,
 		"allow fast-forward (default)"),
@@ -877,12 +877,12 @@ static void prepare_to_commit(void)
 	write_merge_msg(&msg);
 	run_hook(get_index_file(), "prepare-commit-msg",
 		 git_path("MERGE_MSG"), "merge", NULL, NULL);
-	if (option_edit) {
+	if (0 < option_edit) {
 		if (launch_editor(git_path("MERGE_MSG"), NULL, NULL))
 			abort_commit(NULL);
 	}
 	read_merge_msg(&msg);
-	stripspace(&msg, option_edit);
+	stripspace(&msg, 0 < option_edit);
 	if (!msg.len)
 		abort_commit(_("Empty commit message."));
 	strbuf_release(&merge_msg);
@@ -1076,6 +1076,29 @@ static void write_merge_state(void)
 	close(fd);
 }
 
+static int default_edit_option(void)
+{
+	static const char name[] = "GIT_MERGE_LEGACY";
+	const char *e = getenv(name);
+	struct stat st_stdin, st_stdout;
+
+	if (e) {
+		int v = git_config_maybe_bool(name, e);
+		if (v < 0)
+			die("Bad value '%s' in environment '%s'", e, name);
+		return !v;
+	}
+
+	/* Use editor if stdin and stdout are the same and is a tty */
+	return (!fstat(0, &st_stdin) &&
+		!fstat(1, &st_stdout) &&
+		isatty(0) &&
+		st_stdin.st_dev == st_stdout.st_dev &&
+		st_stdin.st_ino == st_stdout.st_ino &&
+		st_stdin.st_rdev == st_stdout.st_rdev);
+}
+
+
 int cmd_merge(int argc, const char **argv, const char *prefix)
 {
 	unsigned char result_tree[20];
@@ -1261,6 +1284,9 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		}
 	}
 
+	if (option_edit < 0)
+		option_edit = default_edit_option();
+
 	if (!use_strategies) {
 		if (!remoteheads->next)
 			add_strategies(pull_twohead, DEFAULT_TWOHEAD);
diff --git a/t/test-lib.sh b/t/test-lib.sh
index bdd9513..439f192 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -63,7 +63,8 @@ GIT_AUTHOR_NAME='A U Thor'
 GIT_COMMITTER_EMAIL=committer@example.com
 GIT_COMMITTER_NAME='C O Mitter'
 GIT_MERGE_VERBOSITY=5
-export GIT_MERGE_VERBOSITY
+GIT_MERGE_LEGACY=yes
+export GIT_MERGE_VERBOSITY GIT_MERGE_LEGACY
 export GIT_AUTHOR_EMAIL GIT_AUTHOR_NAME
 export GIT_COMMITTER_EMAIL GIT_COMMITTER_NAME
 export EDITOR
-- 
1.7.9.rc0.39.ged86b

Re: Re* Regulator updates for 3.3

[Phil Hord]

On Wed, Jan 11, 2012 at 1:59 AM, Junio C Hamano <gitster@pobox.com> wrote:
[...]
>
> With that caveat, the patch should look like this.
>
> -- >8 --
> Subject: [PATCH] merge: use editor by default in interactive sessions
>
> Traditionally, a cleanly resolved merge was committed by "git merge" using
> the auto-generated merge commit log message with invoking the editor.
>
> After 5 years of use in the field, it turns out that many people perform
> too many unjustified backmerges of the upstream history into their topic
> branches. These merges are not just useless, but they are more often than
> not explained and making the end result unreadable when it gets time for
> merging their history back to their upstream.

Typo, I think.  I believe you meant "they are more often than not not
explained", but as this is unclear, maybe you can use "they are
usually not explained" or "they more often than not go in without
explanation".

P

Re: Re* Regulator updates for 3.3

[Linus Torvalds]

On Tue, Jan 10, 2012 at 10:59 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> What makes me uneasy about the idea of running the editor by default is
> that many people still use Git as a better CVS/SVN. Their workflow is to
> build randomly on their 'master', attempt to push and get rejected, pull
> only so that they can push out, and then push the merge result out.

Sure. And I don't think we can do much about it. They'll either set
the legacy flag, or they'll just exit the editor without adding
anything useful (if you come from a CVS background in particular, you
probably never learnt to do good commit logs anyway).

So it will be a bit more work for the bad workflow, I agree - although
if it really irritates people, they can just set that GIT_MERGE_LEGACY
in their .bashrc files or something. But we can *hope* that even those
people might sometimes actually talk about what/why they are doing
things, or maybe even learn about that whole "distributed" thing.

I agree that is unlikely to ever happen, though. It's more likely that
they will change their aliases so that their "update" command just
adds the --no-edit flag. Regardless, it doesn't sound *too* onerous to
work around.

Patch looks good to me. I would personally have compared "st_mode"
instead of (or in addition to) "st_rdev", but I don't think it matters
all that much.

                                 Linus

Re: Regulator updates for 3.3

[Paul Gortmaker]

[Re: Regulator updates for 3.3] On 10/01/2012 (Tue 14:54) Linus Torvalds wrote:

[...]

> So right now "git merge" (and "git pull") make it too easy to make
> those meaningless merge commits. If instead of seven pointless merges

It looks like the editor-by-default solution is a go, but there still
might be value in increasing the visibility of the pointless merges
via. the patch below.

Paul.

> you had (say) had two merges that had messages about *why* they
> weren't pointless, I'd be perfectly happy.
> 
> Addid junio and git to the cc just to bring up this issue of bad UI
> once again. I realize it could break old scripts to start up an editor
> window, but still..
> 
>                        Linus


>From 1a548fa97b78cebcded15d2b00ee3d826f731abd Mon Sep 17 00:00:00 2001
From: Paul Gortmaker <paul.gortmaker@windriver.com>
Date: Wed, 11 Jan 2012 10:33:45 -0500
Subject: [PATCH] merge: Make merge strategy message follow the diffstat

One of the common problems I've seen with people who are
somewhat new to git is that they don't realize that a pull
is a fetch+merge.  They simply decide they want all the
latest stuff and issue a git pull without really thinking
if they are on a branch with local commits or on master,
where a fast forward can take place.

But the one line message that tells you whether you got a fast
forward or a real merge commit is usually pushed off the
screen by all the diffstat information.  So these users won't
even know that their pull has created a merge, and chances
are they will never change their workflow.

By moving the message after the diffstat, there is a better
chance that people will be aware they've done a pointless
merge commit.

Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>

diff --git a/builtin/merge.c b/builtin/merge.c
index 3a45172..9471588 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -370,12 +370,12 @@ static void finish(struct commit *head_commit,
 {
 	struct strbuf reflog_message = STRBUF_INIT;
 	const unsigned char *head = head_commit->object.sha1;
+	int automsg = 0;
 
-	if (!msg)
+	if (!msg) {
+		automsg = 1;
 		strbuf_addstr(&reflog_message, getenv("GIT_REFLOG_ACTION"));
-	else {
-		if (verbosity >= 0)
-			printf("%s\n", msg);
+	} else {
 		strbuf_addf(&reflog_message, "%s: %s",
 			getenv("GIT_REFLOG_ACTION"), msg);
 	}
@@ -409,6 +409,9 @@ static void finish(struct commit *head_commit,
 		diff_flush(&opts);
 	}
 
+	if (!automsg && verbosity >= 0)
+		printf("%s\n", msg);
+
 	/* Run a post-merge hook */
 	run_hook(NULL, "post-merge", squash ? "1" : "0", NULL);
 
-- 
1.7.4.4

Re: [PATCH] merge: Make merge strategy message follow the diffstat

[Junio C Hamano]

Paul Gortmaker <paul.gortmaker@windriver.com> writes:

> By moving the message after the diffstat, there is a better chance that
> people will be aware they've done a pointless merge commit.
>
> Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>

I think the goal of the change may be worthy, but a few points:

 - What does "automsg" mean? Is "auto" in contrast to "manual"? Even
   better, wouldn't it be far simpler to just use

	if (msg && verbosity >= 0)
		printf("%s\n", msg);

   and get rid of this mysteriously named variable altogether?

 - Wouldn't it make more sense to move "No merge message -- not updating
   HEAD" also to the end?

 - After applying this patch, does the tests still pass?

Thanks.

Re: [PATCH] merge: Make merge strategy message follow the diffstat

[Nguyen Thai Ngoc Duy]

On Sat, Jan 14, 2012 at 2:12 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Paul Gortmaker <paul.gortmaker@windriver.com> writes:
>
>> By moving the message after the diffstat, there is a better chance that
>> people will be aware they've done a pointless merge commit.
>>
>> Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
>
> I think the goal of the change may be worthy

Still, diffstat from a fetch/pull is sometimes too verbose. It'd be
better if we have something that fit in one screen (dirstat or maybe
just a first few lines from diffstat then ellipsis) then refer users
to "git diff --stat HEAD@{1}" for more detail stat.
-- 
Duy

Re: [PATCH] merge: Make merge strategy message follow the diffstat

[Linus Torvalds]

On Fri, Jan 13, 2012 at 11:27 AM, Nguyen Thai Ngoc Duy
<pclouds@gmail.com> wrote:
>
> Still, diffstat from a fetch/pull is sometimes too verbose. It'd be
> better if we have something that fit in one screen (dirstat or maybe
> just a first few lines from diffstat then ellipsis) then refer users
> to "git diff --stat HEAD@{1}" for more detail stat.

Yeah, I've wanted that. Show the beginning, the end, and the summary
line of the diffstat would be lovely.

It would be lovely in "git commit" too. Not just

    Modified: filename

but a diffstat that shows now many lines.

And what I've *really* wanted is to actually see the diff itself if it
is small. So some kind of "dynamic summary": for one-liners (or
ten-liners), show the whole diff. For medium-sized changes, show the
whole diffstat. And for really big changes, show an outline and the
"768 files changed, 179851 lines added, 7630 lines removed" stats.

IOW, whatever fits in, say, 50 lines or less.

That would be absolutely lovely if somebody were to do it.

                  Linus

Re: Re* Regulator updates for 3.3

[Pete Harlan]

On 01/10/2012 10:59 PM, Junio C Hamano wrote:
> There may be existing scripts that leave the standard input and the
> standard output of the "git merge" connected to whatever environment the
> scripts were started, and such invocation might trigger the above
> "interactive session" heuristics. Such scripts can export GIT_MERGE_LEGACY
> environment variable set to "yes" to force the traditional behaviour.

The name GIT_MERGE_LEGACY gives no clue about what flavor of legacy
merge behavior is being enabled.  Something like GIT_MERGE_LEGACY_EDIT
might be clearer, or perhaps just have GIT_MERGE_EDIT=0 to get the old
behavior without reference to whether or not that behavior is
considered legacy.

--Pete

Re: Re* Regulator updates for 3.3

[Junio C Hamano]

Pete Harlan <pgit@pcharlan.com> writes:

> On 01/10/2012 10:59 PM, Junio C Hamano wrote:
>> There may be existing scripts that leave the standard input and the
>> standard output of the "git merge" connected to whatever environment the
>> scripts were started, and such invocation might trigger the above
>> "interactive session" heuristics. Such scripts can export GIT_MERGE_LEGACY
>> environment variable set to "yes" to force the traditional behaviour.
>
> The name GIT_MERGE_LEGACY gives no clue about what flavor of legacy
> merge behavior is being enabled.  Something like GIT_MERGE_LEGACY_EDIT
> might be clearer, or perhaps just have GIT_MERGE_EDIT=0 to get the old
> behavior without reference to whether or not that behavior is
> considered legacy.

Hrm.

The only case your suggestion may make a difference would be when we find
another earlier UI mistake we would want to correct in a backward
incompatible way that affects _existing_ scripts.

With your suggestion, they need to export "GIT_MERGE_EDIT=0" today, and
they will need to update again to export "GIT_MERGE_SOMETHINGELSE=0" when
such an incompatible change comes.

With a single "GIT_MERGE_LEGACY=YesPlease", they can be future-proofed today
and will not be affected when we make another incompatible change.

So I am not sure why separating the big-red-switch into smaller pieces
would be an improvement, especially wnen the scripts that want to specify
finer-grained control of features can use "--[no-]edit" options to
explicitly ask for it.

Re: Re* Regulator updates for 3.3

[Martin Fick]

On Monday, January 16, 2012 04:33:00 pm Junio C Hamano 
wrote:
> With your suggestion, they need to export
> "GIT_MERGE_EDIT=0" today, and they will need to update
> again to export "GIT_MERGE_SOMETHINGELSE=0" when such an
> incompatible change comes.
> 
> With a single "GIT_MERGE_LEGACY=YesPlease", they can be
> future-proofed today and will not be affected when we
> make another incompatible change.
> 
> So I am not sure why separating the big-red-switch into
> smaller pieces would be an improvement, especially wnen
> the scripts that want to specify finer-grained control
> of features can use "--[no-]edit" options to explicitly
> ask for it.


Then, what would I do if I write a script which uses the new 
edit functionality (without even being aware that there was 
an old way) and you introduce a new incompatibility?  I 
can't turn on GIT_MERGE_LEGACY then since it would revert to 
behavior which my script would not expect (since it was 
written after the current incompatibility, but before the 
new one)!

-Martin

Re: Re* Regulator updates for 3.3

[Pete Harlan]

On 01/16/2012 03:33 PM, Junio C Hamano wrote:
> Pete Harlan <pgit@pcharlan.com> writes:
> 
>> On 01/10/2012 10:59 PM, Junio C Hamano wrote:
>>> There may be existing scripts that leave the standard input and the
>>> standard output of the "git merge" connected to whatever environment the
>>> scripts were started, and such invocation might trigger the above
>>> "interactive session" heuristics. Such scripts can export GIT_MERGE_LEGACY
>>> environment variable set to "yes" to force the traditional behaviour.
>>
>> The name GIT_MERGE_LEGACY gives no clue about what flavor of legacy
>> merge behavior is being enabled.  Something like GIT_MERGE_LEGACY_EDIT
>> might be clearer, or perhaps just have GIT_MERGE_EDIT=0 to get the old
>> behavior without reference to whether or not that behavior is
>> considered legacy.
> 
> Hrm.
> 
> The only case your suggestion may make a difference would be when we find
> another earlier UI mistake we would want to correct in a backward
> incompatible way that affects _existing_ scripts.
> 
> With your suggestion, they need to export "GIT_MERGE_EDIT=0" today, and
> they will need to update again to export "GIT_MERGE_SOMETHINGELSE=0" when
> such an incompatible change comes.

Which is a good thing, because maybe they started using Git after the
current proposed change (which they like), and what you see as new
becomes their "legacy" behavior.  If you change something after that,
you can't use GIT_MERGE_LEGACY=yes for that one also because which
legacy is it preserving?

In general, naming configuration variables "DO_IT_<THIS_WAY>" instead
of "DO_IT_THE_OLD_WAY" is better because it's self-documenting.  The
only time I think I'd prefer "LEGACY" is if you're planning on
deprecating and removing it eventually and you want to indicate
something to that effect in the name.

--
Pete Harlan
pgit@pcharlan.com

Re: Re* Regulator updates for 3.3

[Junio C Hamano]

Pete Harlan <pgit@pcharlan.com> writes:

> ... The
> only time I think I'd prefer "LEGACY" is if you're planning on
> deprecating and removing it eventually and you want to indicate
> something to that effect in the name.

The discussion that led to the naming of that LEGACY token needs to be
re-read, then. The kind of "LEGACY" you prefer is exactly why the
environment variable is called LEGACY in the patch you are commenting on,
written in response to Linus's suggestion to switch the default, even
though I am not 100% buying it.

Having said that, I think I am wasting my time responding to this thread
during the feature-freeze period for v1.7.9, as I am not a big fan of
switching the default without adequate warning and transition plans, after
getting burned by the "'git-foo' vs 'git foo'" flames back in the v1.6.0
release. We would likely to take a gradual and smoother migration route to
transition, e.g. v1.7.9 to introduce "merge --edit", v1.7.10 to introduce
a configuration variable merge.edit (lack of which gives a warning and an
advice message while defaulting to 'no' to preserve the traditional
behaviour), and finally v1.8.0 (or v2.0) to flip the default to 'yes'
(while the configuration still giving a warning and an advice message)
that "merge --no-edit" can still countermand.

So you have until v1.7.10 to decide a good name for the overriding
environment variable.

Re: [PATCH] merge: Make merge strategy message follow the diffstat

[Miles Bader]

So ... "--shortish-diffthingy"

-miles

-- 
Zeal, n. A certain nervous disorder afflicting the young and inexperienced.