Rebase/cherry-picking idea

Rebase/cherry-picking idea

[Wincent Colaiuta]

In using "git-rebase --interactive" to re-order commits you  
occasionally get conflicts and will see a message like this:

	When commiting, use the option '-c %s' to retain authorship and message

I was thinking that it might be nice to stash away this commit id  
somewhere in GIT_DIR so that the user didn't have to explicitly  
remember it, and add a new switch to git-commit that could be used to  
automatically use that stashed commit id, something like:

	git commit --retain

Although I most often see this kind of message in interactive  
rebasing, the message is generated in builtin-revert.c when cherry- 
picking, so you can also see it in any other situation where you're  
cherry picking and there's a conflict.

What do people think? Would this be a nice usability improvement? Or  
is it adding clutter?

Cheers,
Wincent

Re: Rebase/cherry-picking idea

[Benoit Sigoure]

[-- Attachment #1: Type: text/plain, Size: 1175 bytes --]

On Nov 26, 2007, at 10:02 AM, Wincent Colaiuta wrote:

> In using "git-rebase --interactive" to re-order commits you  
> occasionally get conflicts and will see a message like this:
>
> 	When commiting, use the option '-c %s' to retain authorship and  
> message
>
> I was thinking that it might be nice to stash away this commit id  
> somewhere in GIT_DIR so that the user didn't have to explicitly  
> remember it, and add a new switch to git-commit that could be used  
> to automatically use that stashed commit id, something like:
>
> 	git commit --retain
>
> Although I most often see this kind of message in interactive  
> rebasing, the message is generated in builtin-revert.c when cherry- 
> picking, so you can also see it in any other situation where you're  
> cherry picking and there's a conflict.
>
> What do people think? Would this be a nice usability improvement?  
> Or is it adding clutter?


I'm not sure but I think this message is just some unwanted  
(misleading) noise, since when you rebase, once you solve the  
conflicts, you git-rebase --continue, you don't git-commit.

-- 
Benoit Sigoure aka Tsuna
EPITA Research and Development Laboratory



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 186 bytes --]

Re: Rebase/cherry-picking idea

[Wincent Colaiuta]

El 26/11/2007, a las 10:32, Benoit Sigoure escribió:

> On Nov 26, 2007, at 10:02 AM, Wincent Colaiuta wrote:
>
>> In using "git-rebase --interactive" to re-order commits you  
>> occasionally get conflicts and will see a message like this:
>>
>> 	When commiting, use the option '-c %s' to retain authorship and  
>> message
>>
>> I was thinking that it might be nice to stash away this commit id  
>> somewhere in GIT_DIR so that the user didn't have to explicitly  
>> remember it, and add a new switch to git-commit that could be used  
>> to automatically use that stashed commit id, something like:
>>
>> 	git commit --retain
>>
>> Although I most often see this kind of message in interactive  
>> rebasing, the message is generated in builtin-revert.c when cherry- 
>> picking, so you can also see it in any other situation where you're  
>> cherry picking and there's a conflict.
>>
>> What do people think? Would this be a nice usability improvement?  
>> Or is it adding clutter?
>
>
> I'm not sure but I think this message is just some unwanted  
> (misleading) noise, since when you rebase, once you solve the  
> conflicts, you git-rebase --continue, you don't git-commit.

Looks like you're right. I just did a simple test and it turns out  
that after a conflict, this:

	git commit -c ...
	git rebase --continue

Produces exactly the same history as this:

	git rebase --continue

So I think that misleading noise needs to be suppressed or reworded  
when rebasing. Will look into it.

Cheers,
Wincent

Re: Rebase/cherry-picking idea

[Wincent Colaiuta]

El 26/11/2007, a las 12:27, Wincent Colaiuta escribió:

> El 26/11/2007, a las 10:32, Benoit Sigoure escribió:
>
>> On Nov 26, 2007, at 10:02 AM, Wincent Colaiuta wrote:
>>
>>> In using "git-rebase --interactive" to re-order commits you  
>>> occasionally get conflicts and will see a message like this:
>>>
>>> 	When commiting, use the option '-c %s' to retain authorship and  
>>> message
>>>
>>> I was thinking that it might be nice to stash away this commit id  
>>> somewhere in GIT_DIR so that the user didn't have to explicitly  
>>> remember it, and add a new switch to git-commit that could be used  
>>> to automatically use that stashed commit id, something like:
>>>
>>> 	git commit --retain
>>>
>>> Although I most often see this kind of message in interactive  
>>> rebasing, the message is generated in builtin-revert.c when cherry- 
>>> picking, so you can also see it in any other situation where  
>>> you're cherry picking and there's a conflict.
>>>
>>> What do people think? Would this be a nice usability improvement?  
>>> Or is it adding clutter?
>>
>>
>> I'm not sure but I think this message is just some unwanted  
>> (misleading) noise, since when you rebase, once you solve the  
>> conflicts, you git-rebase --continue, you don't git-commit.
>
> Looks like you're right. I just did a simple test and it turns out  
> that after a conflict, this:
>
> 	git commit -c ...
> 	git rebase --continue
>
> Produces exactly the same history as this:
>
> 	git rebase --continue
>
> So I think that misleading noise needs to be suppressed or reworded  
> when rebasing. Will look into it.

How about something like this? It would obviously be nice if we could  
avoid adding another option to builtin-revert; perhaps when/if git- 
rebase becomes a builtin we can avoid that. The other alternative, and  
probably one I like I bit more, would be to auto-detect that a rebase  
is in progress by looking inside the GIT_DIR, although that would also  
alter the behaviour of manual invocations of git-revert and git-cherry- 
pick during an interactive rebase (do people actually do that?). What  
do you think?

diff --git a/builtin-revert.c b/builtin-revert.c
index a0586f9..36e36c3 100644
--- a/builtin-revert.c
+++ b/builtin-revert.c
@@ -30,7 +30,7 @@ static const char * const cherry_pick_usage[] = {
  	NULL
  };

-static int edit, no_replay, no_commit, mainline;
+static int edit, no_replay, no_commit, rebasing, mainline;
  static enum { REVERT, CHERRY_PICK } action;
  static struct commit *commit;

@@ -50,6 +50,7 @@ static void parse_args(int argc, const char **argv)
  		OPT_BOOLEAN('e', "edit", &edit, "edit the commit message"),
  		OPT_BOOLEAN('x', NULL, &no_replay, "append commit name when cherry- 
picking"),
  		OPT_BOOLEAN('r', NULL, &noop, "no-op (backward compatibility)"),
+		OPT_BOOLEAN(0, "rebasing", &rebasing, "use rebase mode"),
  		OPT_INTEGER('m', "mainline", &mainline, "parent number"),
  		OPT_END(),
  	};
@@ -352,11 +353,16 @@ static int revert_or_cherry_pick(int argc, const  
char **argv)
  		}
  		if (close(msg_fd) || commit_lock_file(&msg_file) < 0)
  			die ("Error wrapping up %s", defmsg);
+		if (rebasing)
+			message = "run 'git rebase --continue' "
+			    "or 'git rebase --abort'";
+		else
+			message = "commit the result";
  		fprintf(stderr, "Automatic %s failed.  "
  			"After resolving the conflicts,\n"
  			"mark the corrected paths with 'git add <paths>' "
-			"and commit the result.\n", me);
-		if (action == CHERRY_PICK) {
+			"and %s.\n", me, message);
+		if (action == CHERRY_PICK && !rebasing) {
  			fprintf(stderr, "When commiting, use the option "
  				"'-c %s' to retain authorship and message.\n",
  				find_unique_abbrev(commit->object.sha1,
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index bf44b6a..5afb843 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -117,7 +117,7 @@ pick_one () {
  		sha1=$(git rev-parse --short $sha1)
  		output warn Fast forward to $sha1
  	else
-		output git cherry-pick "$@"
+		output git cherry-pick --rebasing "$@"
  	fi
  }

@@ -187,7 +187,7 @@ pick_one_preserving_merges () {
  			fi
  			;;
  		*)
-			output git cherry-pick "$@" ||
+			output git cherry-pick --rebasing "$@" ||
  				die_with_patch $sha1 "Could not pick $sha1"
  			;;
  		esac

Re: Rebase/cherry-picking idea

[Benoit Sigoure]

On Nov 26, 2007, at 1:34 PM, Wincent Colaiuta wrote:

> How about something like this? It would obviously be nice if we  
> could avoid adding another option to builtin-revert; perhaps when/ 
> if git-rebase becomes a builtin we can avoid that. The other  
> alternative, and probably one I like I bit more, would be to auto- 
> detect that a rebase is in progress by looking inside the GIT_DIR,  
> although that would also alter the behaviour of manual invocations  
> of git-revert and git-cherry-pick during an interactive rebase (do  
> people actually do that?). What do you think?


Hmm yeah, I agree that it's a little bit of a dirty workaround but,  
as you pointed out, until rebase is builtinified, this looks like the  
best/easiest alternative.

-- 
Benoit Sigoure aka Tsuna
EPITA Research and Development Laboratory

Re: Rebase/cherry-picking idea

[Johannes Sixt]

Wincent Colaiuta schrieb:
> El 26/11/2007, a las 12:27, Wincent Colaiuta escribió:
>> So I think that misleading noise needs to be suppressed or reworded 
>> when rebasing. Will look into it.
> 
> How about something like this? It would obviously be nice if we could 
> avoid adding another option to builtin-revert; perhaps when/if 
> git-rebase becomes a builtin we can avoid that. The other alternative, 
> and probably one I like I bit more, would be to auto-detect that a 
> rebase is in progress by looking inside the GIT_DIR, although that would 
> also alter the behaviour of manual invocations of git-revert and 
> git-cherry-pick during an interactive rebase (do people actually do 
> that?). What do you think?

Introduce an environment variable _GIT_CHERRY_PICK_HELP (note the leading 
underscore), which git-rebase sets; if it's set, git-cherry-pick uses that 
text instead of the usual one.

-- Hannes

Re: Rebase/cherry-picking idea

[Wincent Colaiuta]

El 26/11/2007, a las 13:51, Johannes Sixt escribió:

> Wincent Colaiuta schrieb:
>> El 26/11/2007, a las 12:27, Wincent Colaiuta escribió:
>>> So I think that misleading noise needs to be suppressed or  
>>> reworded when rebasing. Will look into it.
>> How about something like this? It would obviously be nice if we  
>> could avoid adding another option to builtin-revert; perhaps when/ 
>> if git-rebase becomes a builtin we can avoid that. The other  
>> alternative, and probably one I like I bit more, would be to auto- 
>> detect that a rebase is in progress by looking inside the GIT_DIR,  
>> although that would also alter the behaviour of manual invocations  
>> of git-revert and git-cherry-pick during an interactive rebase (do  
>> people actually do that?). What do you think?
>
> Introduce an environment variable _GIT_CHERRY_PICK_HELP (note the  
> leading underscore), which git-rebase sets; if it's set, git-cherry- 
> pick uses that text instead of the usual one.

Good idea, quite a bit less cruddy:

diff --git a/builtin-revert.c b/builtin-revert.c
index a0586f9..5a57574 100644
--- a/builtin-revert.c
+++ b/builtin-revert.c
@@ -229,7 +229,7 @@ static int revert_or_cherry_pick(int argc, const  
char **argv)
  	unsigned char head[20];
  	struct commit *base, *next, *parent;
  	int i;
-	char *oneline, *reencoded_message = NULL;
+	char *oneline, *reencoded_message = NULL, *help_message;
  	const char *message, *encoding;
  	const char *defmsg = xstrdup(git_path("MERGE_MSG"));

@@ -352,11 +352,13 @@ static int revert_or_cherry_pick(int argc, const  
char **argv)
  		}
  		if (close(msg_fd) || commit_lock_file(&msg_file) < 0)
  			die ("Error wrapping up %s", defmsg);
+		help_message = getenv("_GIT_CHERRY_PICK_HELP");
  		fprintf(stderr, "Automatic %s failed.  "
  			"After resolving the conflicts,\n"
  			"mark the corrected paths with 'git add <paths>' "
-			"and commit the result.\n", me);
-		if (action == CHERRY_PICK) {
+			"and %s.\n", me,
+			help_message ? help_message : "commit the result");
+		if (action == CHERRY_PICK && !help_message) {
  			fprintf(stderr, "When commiting, use the option "
  				"'-c %s' to retain authorship and message.\n",
  				find_unique_abbrev(commit->object.sha1,
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index bf44b6a..e5f9810 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -117,6 +117,7 @@ pick_one () {
  		sha1=$(git rev-parse --short $sha1)
  		output warn Fast forward to $sha1
  	else
+		export _GIT_CHERRY_PICK_HELP="run 'git rebase --continue'"
  		output git cherry-pick "$@"
  	fi
  }
@@ -187,6 +188,7 @@ pick_one_preserving_merges () {
  			fi
  			;;
  		*)
+			export _GIT_CHERRY_PICK_HELP="run 'git rebase --continue'"
  			output git cherry-pick "$@" ||
  				die_with_patch $sha1 "Could not pick $sha1"
  			;;

Re: Rebase/cherry-picking idea

[Johannes Schindelin]

Hi,

On Mon, 26 Nov 2007, Benoit Sigoure wrote:

> On Nov 26, 2007, at 10:02 AM, Wincent Colaiuta wrote:
> 
> > In using "git-rebase --interactive" to re-order commits you occasionally get
> > conflicts and will see a message like this:
> > 
> > 	When commiting, use the option '-c %s' to retain authorship and
> > message
> > 
> > I was thinking that it might be nice to stash away this commit id somewhere
> > in GIT_DIR so that the user didn't have to explicitly remember it, and add a
> > new switch to git-commit that could be used to automatically use that
> > stashed commit id, something like:
> > 
> > 	git commit --retain
> > 
> > Although I most often see this kind of message in interactive rebasing, the
> > message is generated in builtin-revert.c when cherry-picking, so you can
> > also see it in any other situation where you're cherry picking and there's a
> > conflict.
> > 
> > What do people think? Would this be a nice usability improvement? Or is it
> > adding clutter?
> 
> 
> I'm not sure but I think this message is just some unwanted (misleading)
> noise, since when you rebase, once you solve the conflicts, you git-rebase
> --continue, you don't git-commit.

Yep.  It is on my TODO list since a long time, but I am just as glad 
somebody else is doing it.  But I have to agree with Hannes that using an 
environment variable is cleaner, more elegant and shorter.

Ciao,
Dscho

Re: Rebase/cherry-picking idea

[Johannes Schindelin]

Hi,

On Mon, 26 Nov 2007, Wincent Colaiuta wrote:

> +		help_message = getenv("_GIT_CHERRY_PICK_HELP");

Why on earth do you have a leading underscore?  No existing git 
environment variable does it that way.

Ciao,
Dscho

Re: Rebase/cherry-picking idea

[Wincent Colaiuta]

El 26/11/2007, a las 14:41, Johannes Schindelin escribió:

> Hi,
>
> On Mon, 26 Nov 2007, Wincent Colaiuta wrote:
>
>> +		help_message = getenv("_GIT_CHERRY_PICK_HELP");
>
> Why on earth do you have a leading underscore?  No existing git
> environment variable does it that way.

I was following the suggestion of Johannes Sixt:

El 26/11/2007, a las 13:51, Johannes Sixt escribió:

> Introduce an environment variable _GIT_CHERRY_PICK_HELP (note the  
> leading underscore), which git-rebase sets; if it's set, git-cherry- 
> pick uses that text instead of the usual one.


I imagine that he proposed it that way because it's an "internal use  
only" thing.

Once I get a clear idea of what kind of change is likely to actually  
get accepted I'll submit a proper patch.

Cheers,
Wincent

Re: Rebase/cherry-picking idea

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

> Introduce an environment variable _GIT_CHERRY_PICK_HELP (note the
> leading underscore), which git-rebase sets; if it's set,
> git-cherry-pick uses that text instead of the usual one.

With precedences like GIT_REFLOG_ACTION and GITHEAD_${objectname}, I
think it would be consistent without the leading underscore.

I would not object to renaming all of them to have the leading
underscore, though.  That would make it clear that they are very
different from ordinary environment variables for the user to set
(e.g. GIT_INDEX_FILE, GIT_AUTHOR_NAME).  Does any third party tool like
qgit already use GITHEAD_${objectname} and/or GIT_REFLOG_ACTION?

Re: Rebase/cherry-picking idea

[Marco Costalba]

On Nov 26, 2007 6:26 PM, Junio C Hamano <gitster@pobox.com> wrote:
> (e.g. GIT_INDEX_FILE, GIT_AUTHOR_NAME).  Does any third party tool like
> qgit already use GITHEAD_${objectname} and/or GIT_REFLOG_ACTION?
>

No, it doesn't.

Thanks for asking ;-)

Marco

Re: Rebase/cherry-picking idea

[Shawn O. Pearce]

Junio C Hamano <gitster@pobox.com> wrote:
> I would not object to renaming all of them to have the leading
> underscore, though.  That would make it clear that they are very
> different from ordinary environment variables for the user to set
> (e.g. GIT_INDEX_FILE, GIT_AUTHOR_NAME).  Does any third party tool like
> qgit already use GITHEAD_${objectname} and/or GIT_REFLOG_ACTION?

git-gui apparently doesn't use either name right now.  It avoids
needing to use GIT_REFLOG_ACTION by invoking only plumbing, except
in the case of git-merge, where it invokes git-merge and thus avoids
the need to set GITHEAD_* to get conflict markers right when the
recursive strategy gets used.

I had started to replace git-merge in Tcl and have git-gui directly
invoke merge-recursive but I haven't gotten around to really
doing that.

So I guess we could rename those two "internal" environment variables
to use a leading _ to make them different from "user level" variables,
but why change them now?  I really don't see a compelling reason to
break that part of the "API" between porcelain/plumbing.

-- 
Shawn.

Re: Rebase/cherry-picking idea

[Junio C Hamano]

"Shawn O. Pearce" <spearce@spearce.org> writes:

> So I guess we could rename those two "internal" environment variables
> to use a leading _ to make them different from "user level" variables,
> but why change them now?  I really don't see a compelling reason to
> break that part of the "API" between porcelain/plumbing.

I don't either, which means I do not see a compelling reason to have
underscore in front of that cherry-pick message environment either.

About the patch itself, I think replacing the whole message, not just
"and commit the result." part, might make more sense.

	help_message = getenv("_GIT_CHERRY_PICK_HELP");
	fprintf(stderr, "Automatic %s failed.  "
		"After resolving the conflicts,\n"
		"mark the corrected paths with 'git add <paths>' "
		"and %s.\n", me,
		help_message ? help_message : "commit the result");
	if (action == CHERRY_PICK && !help_message) {
		fprintf(stderr, "When commiting, use the option "
			"'-c %s' to retain authorship and message.\n",
			find_unique_abbrev(commit->object.sha1,
			...

Some other caller can be written to guide the user resolving and do the
"git add" part for the user, and "mark the corrected paths with 'git add
<paths>'" may not suit the need for such a caller.

Which would mean:

	help_message = getenv("GIT_CHERRY_PICK_HELP");
        if (!help_message) {
        	static char helpbuf[1024];
                help_message = helpbuf;
                sprintf(help_message,
                	"  After resolving the conflits,\n"
			"mark the corrected paths with 'git add <paths>' "
			"and commit the result.\n"
			"When commiting, use the option "
			"'-c %s' to retain authorship and message.\n",
			find_unique_abbrev(commit->object.sha1,
						DEFAULT_ABBREV));
        }
	fprintf(stderr, "Automatic %s failed.%s", help_message);
	exit(1);

But I do not care too deeply either way.

Re: Rebase/cherry-picking idea

[Junio C Hamano]

> Once I get a clear idea of what kind of change is likely to actually  
> get accepted I'll submit a proper patch.

Too often, people disappear with a patch that is basically good but has
room for improvements this way.  I really do not have time nor bandwidth
to keep bugging them for updates, and often end up cleaning up on my own
instead.

This is such a patch, and without acks or comments, it is also very
likely to be lost.

-- >8 --
revert/cherry-pick: Allow overriding the help text by the calling Porcelain

A Porcelain command that uses cherry-pick or revert may make a commit
out of resolved index itself, in which case telling the user to commit
the result is not appropriate at all.  This allows GIT_CHERRY_PICK_HELP
environment variable to be set by the calling Porcelain in order to
override the built-in help text.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * I suspect that "git rebase --continue" can do "git add -u" itself,
   just like recent "git rebase --skip" would do "git reset --hard".

   The overriding help text in this patch can lose its second line that
   tells the user to use "git add <paths>" if we decide to do so.

 builtin-revert.c           |   33 +++++++++++++++++++++++----------
 git-rebase--interactive.sh |    5 +++++
 2 files changed, 28 insertions(+), 10 deletions(-)

diff --git a/builtin-revert.c b/builtin-revert.c
index a0586f9..4f86178 100644
--- a/builtin-revert.c
+++ b/builtin-revert.c
@@ -224,6 +224,27 @@ static int merge_recursive(const char *base_sha1,
 	return run_command_v_opt(argv, RUN_COMMAND_NO_STDIN | RUN_GIT_CMD);
 }
 
+static char *help_msg(const unsigned char *sha1)
+{
+	static char helpbuf[1024];
+	char *msg = getenv("GIT_CHERRY_PICK_HELP");
+
+	if (msg)
+		return msg;
+
+	strcpy(helpbuf, "  After resolving the conflicts,\n"
+	       "mark the corrected paths with 'git add <paths>' "
+	       "and commit the result.");
+
+	if (action == CHERRY_PICK) {
+		sprintf(helpbuf + strlen(helpbuf),
+			"\nWhen commiting, use the option "
+			"'-c %s' to retain authorship and message.",
+			find_unique_abbrev(sha1, DEFAULT_ABBREV));
+	}
+	return helpbuf;
+}
+
 static int revert_or_cherry_pick(int argc, const char **argv)
 {
 	unsigned char head[20];
@@ -352,16 +373,8 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 		}
 		if (close(msg_fd) || commit_lock_file(&msg_file) < 0)
 			die ("Error wrapping up %s", defmsg);
-		fprintf(stderr, "Automatic %s failed.  "
-			"After resolving the conflicts,\n"
-			"mark the corrected paths with 'git add <paths>' "
-			"and commit the result.\n", me);
-		if (action == CHERRY_PICK) {
-			fprintf(stderr, "When commiting, use the option "
-				"'-c %s' to retain authorship and message.\n",
-				find_unique_abbrev(commit->object.sha1,
-					DEFAULT_ABBREV));
-		}
+		fprintf(stderr, "Automatic %s failed.%s\n",
+			me, help_msg(commit->object.sha1));
 		exit(1);
 	}
 	if (close(msg_fd) || commit_lock_file(&msg_file) < 0)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index bf44b6a..33a5d4b 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -30,6 +30,11 @@ test -d "$REWRITTEN" && PRESERVE_MERGES=t
 test -f "$DOTEST"/strategy && STRATEGY="$(cat "$DOTEST"/strategy)"
 test -f "$DOTEST"/verbose && VERBOSE=t
 
+GIT_CHERRY_PICK_HELP="  After resolving the conflicts,
+mark the corrected paths with 'git add <paths>', and
+run 'git rebase --continue'"
+export GIT_CHERRY_PICK_HELP
+
 warn () {
 	echo "$*" >&2
 }

Re: Rebase/cherry-picking idea

[Wincent Colaiuta]

El 28/11/2007, a las 9:06, Junio C Hamano escribió:

>> Once I get a clear idea of what kind of change is likely to actually
>> get accepted I'll submit a proper patch.
>
> Too often, people disappear with a patch that is basically good but  
> has
> room for improvements this way.  I really do not have time nor  
> bandwidth
> to keep bugging them for updates, and often end up cleaning up on my  
> own
> instead.

The problem in this case was that my patch didn't receive any  
meaningful feedback (ie. suggestions for improvement), only a lot of  
bikeshed stuff about whether the environment variable should have an  
underscore prefix or not, whether or not I should use "export FOO=..."  
or not etc. So I didn't know what was necessary in order to get it  
accepted.

> This is such a patch, and without acks or comments, it is also very
> likely to be lost.

Ok, please disregard the resend that I just posted a few minutes ago  
(hadn't seen your new patch yet).

Cheers,
Wincent

Re: Rebase/cherry-picking idea

[Junio C Hamano]

Wincent Colaiuta <win@wincent.com> writes:

> The problem in this case was that my patch didn't receive any
> meaningful feedback (ie. suggestions for improvement), only a lot of
> bikeshed stuff about whether the environment variable should have an
> underscore prefix or not, whether or not I should use "export FOO=..."
> or not etc. So I didn't know what was necessary in order to get it
> accepted.

I do not think that is the case.

I think _GIT_FOO vs GIT_FOO is an important detail, not at all a
bikeshed color, to make things consistent.  "export VAR=VAL" also is a
valid concern (you said in a separate message you only know about bash,
and I later asked people if they use shells that get affected with it
but we happily run otherwise.  I personally do not think the latter is a
problem, but since somebody already raised it as a potential issue, it
gave us a good chance to hear from people on minority platforms, if only
to build confidence in us to use that POSIX form.

Maybe it is just me, but I think my suggestion to replace not just "git
commit" part but also "git add" part is also an important design issue.

In any case, after a discussion, sending out a readily applyable patch
would help to make sure we reached a reasonable consensus; it makes it
easier for other people to try out your proposed draft of the consensus
without waiting for me or anybody else and give positive feedback.

The difference between the variant I posted and your revised one I see
are:

 * As discussed, we have precedence like GITHEAD_* and GIT_REFLOG_ACTION
   that are used purely for inter-tool communication without the leading
   underscore, so I followed them for consistency.

 * The part of the message that is overridable is made wider; that way,
   we can later choose to have "git rebase --continue" do the final "git
   add -u" step, just like we made "git rebase --skip" to run "git reset
   --hard", without changing revert/cherry-pick,as I explained in the
   comment to my patch.

 * I made the help-message creation into a separate function.  This is
   just a minor detail, but I think it is good for readability.

 * I am setting and exporting the help environment near the beginning of
   rebase--interactive just once; again I think this is more readable.

But other than that, the basic idea is the same and is all yours.

If these details (I do not think the overridability is a minor detail,
though) look like bikeshedding to you, that is somewhat sad.  You seem
to be very capable of producing usable code, but these details
(consistency and flexibility) matter for longer term stability, and I
would really want to see capable people pay attention to such details,
especially I sometimes fail to do so myself.

Re: Rebase/cherry-picking idea

[Wincent Colaiuta]

El 28/11/2007, a las 10:47, Junio C Hamano escribió:

> I think _GIT_FOO vs GIT_FOO is an important detail, not at all a
> bikeshed color, to make things consistent.  "export VAR=VAL" also is a
> valid concern (you said in a separate message you only know about  
> bash,
> and I later asked people if they use shells that get affected with it
> but we happily run otherwise.  I personally do not think the latter  
> is a
> problem, but since somebody already raised it as a potential issue, it
> gave us a good chance to hear from people on minority platforms, if  
> only
> to build confidence in us to use that POSIX form.

I'm still a little concerned that nobody commented when I pointed out  
that export VAR=VAL is used elsewhere in Git, especially in git- 
clone.sh, which is very commonly-used porcelain. Is it a problem?

> If these details (I do not think the overridability is a minor detail,
> though) look like bikeshedding to you, that is somewhat sad.  You seem
> to be very capable of producing usable code, but these details
> (consistency and flexibility) matter for longer term stability, and I
> would really want to see capable people pay attention to such details,
> especially I sometimes fail to do so myself.


I think the *key* difference between our patches was the refactoring  
that you did (extracting into a separate function and at the same time  
making the entire message customizable rather than just the end). I  
think that's easily explained by the fact that as a new contributor I  
am pretty much always going to err on the side of the most minimal  
change possible (minimum number of lines, and changes kept as local as  
possible), even if it isn't the best or most well-engineered one. Your  
patch is more invasive, but it's better too, and I think it's more  
invasive precisely because your knowledge of the codebase and your  
track record gives you the confidence to make non-minimal changes when  
you think they're better. With time I imagine I'll evolve away from  
such defensive, minimal patches as the one I sent.

Cheers,
Wincent

[PATCH] Replace instances of export VAR=VAL with VAR=VAL; export VAR

[Johannes Schindelin]

It might be POSIX, but there are shells that do not like the
expression 'export VAR=VAL'.  To be on the safe side, rewrite them
into 'VAR=VAL' and 'export VAR'.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	On Wed, 28 Nov 2007, Wincent Colaiuta wrote:

	> I'm still a little concerned that nobody commented when I 
	> pointed out that export VAR=VAL is used elsewhere in Git, 
	> especially in git-clone.sh, which is very commonly-used 
	> porcelain. Is it a problem?

	How's that for a comment?

 git-clone.sh         |    2 +-
 git-filter-branch.sh |   20 ++++++++++++--------
 git-quiltimport.sh   |   10 ++++++----
 3 files changed, 19 insertions(+), 13 deletions(-)

diff --git a/git-clone.sh b/git-clone.sh
index 24ad179..ecf9d89 100755
--- a/git-clone.sh
+++ b/git-clone.sh
@@ -229,7 +229,7 @@ cleanup() {
 trap cleanup 0
 mkdir -p "$dir" && D=$(cd "$dir" && pwd) || usage
 test -n "$GIT_WORK_TREE" && mkdir -p "$GIT_WORK_TREE" &&
-W=$(cd "$GIT_WORK_TREE" && pwd) && export GIT_WORK_TREE="$W"
+W=$(cd "$GIT_WORK_TREE" && pwd) && GIT_WORK_TREE="$W" && export GIT_WORK_TREE
 if test yes = "$bare" || test -n "$GIT_WORK_TREE"; then
 	GIT_DIR="$D"
 else
diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index 19cab5a..3afc945 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -66,17 +66,17 @@ set_ident () {
 			h
 			s/^'$lid' \([^<]*\) <[^>]*> .*$/\1/
 			s/'\''/'\''\'\'\''/g
-			s/.*/export GIT_'$uid'_NAME='\''&'\''/p
+			s/.*/GIT_'$uid'_NAME='\''&'\''\nexport GIT_'$uid'_NAME/p
 
 			g
 			s/^'$lid' [^<]* <\([^>]*\)> .*$/\1/
 			s/'\''/'\''\'\'\''/g
-			s/.*/export GIT_'$uid'_EMAIL='\''&'\''/p
+			s/.*/GIT_'$uid'_EMAIL='\''&'\''\nexport GIT_'$uid'_EMAIL/p
 
 			g
 			s/^'$lid' [^<]* <[^>]*> \(.*\)$/\1/
 			s/'\''/'\''\'\'\''/g
-			s/.*/export GIT_'$uid'_DATE='\''&'\''/p
+			s/.*/GIT_'$uid'_DATE='\''&'\''\nexport GIT_'$uid'_DATE/p
 
 			q
 		}
@@ -84,7 +84,7 @@ set_ident () {
 
 	LANG=C LC_ALL=C sed -ne "$pick_id_script"
 	# Ensure non-empty id name.
-	echo "[ -n \"\$GIT_${uid}_NAME\" ] || export GIT_${uid}_NAME=\"\${GIT_${uid}_EMAIL%%@*}\""
+	echo "case \"\$GIT_${uid}_NAME\" in \"\") GIT_${uid}_NAME=\"\${GIT_${uid}_EMAIL%%@*}\" && export GIT_${uid}_NAME;; esac"
 }
 
 USAGE="[--env-filter <command>] [--tree-filter <command>] \
@@ -206,7 +206,8 @@ done < "$tempdir"/backup-refs
 ORIG_GIT_DIR="$GIT_DIR"
 ORIG_GIT_WORK_TREE="$GIT_WORK_TREE"
 ORIG_GIT_INDEX_FILE="$GIT_INDEX_FILE"
-export GIT_DIR GIT_WORK_TREE=.
+GIT_WORK_TREE=.
+export GIT_DIR GIT_WORK_TREE
 
 # These refs should be updated if their heads were rewritten
 
@@ -231,7 +232,8 @@ done > "$tempdir"/heads
 test -s "$tempdir"/heads ||
 	die "Which ref do you want to rewrite?"
 
-export GIT_INDEX_FILE="$(pwd)/../index"
+GIT_INDEX_FILE="$(pwd)/../index"
+export GIT_INDEX_FILE
 git read-tree || die "Could not seed the index"
 
 ret=0
@@ -267,7 +269,8 @@ while read commit parents; do
 		git read-tree -i -m $commit:"$filter_subdir"
 	esac || die "Could not initialize the index"
 
-	export GIT_COMMIT=$commit
+	GIT_COMMIT=$commit
+	export GIT_COMMIT
 	git cat-file commit "$commit" >../commit ||
 		die "Cannot read commit $commit"
 
@@ -401,7 +404,8 @@ if [ "$filter_tag_name" ]; then
 
 		[ -f "../map/$sha1" ] || continue
 		new_sha1="$(cat "../map/$sha1")"
-		export GIT_COMMIT="$sha1"
+		GIT_COMMIT="$sha1"
+		export GIT_COMMIT
 		new_ref="$(echo "$ref" | eval "$filter_tag_name")" ||
 			die "tag name filter failed: $filter_tag_name"
 
diff --git a/git-quiltimport.sh b/git-quiltimport.sh
index 6b0c4d2..233e5ea 100755
--- a/git-quiltimport.sh
+++ b/git-quiltimport.sh
@@ -77,8 +77,9 @@ for patch_name in $(grep -v '^#' < "$QUILT_PATCHES/series" ); do
 	}
 
 	# Parse the author information
-	export GIT_AUTHOR_NAME=$(sed -ne 's/Author: //p' "$tmp_info")
-	export GIT_AUTHOR_EMAIL=$(sed -ne 's/Email: //p' "$tmp_info")
+	GIT_AUTHOR_NAME=$(sed -ne 's/Author: //p' "$tmp_info")
+	GIT_AUTHOR_EMAIL=$(sed -ne 's/Email: //p' "$tmp_info")
+	export GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL
 	while test -z "$GIT_AUTHOR_EMAIL" && test -z "$GIT_AUTHOR_NAME" ; do
 		if [ -n "$quilt_author" ] ; then
 			GIT_AUTHOR_NAME="$quilt_author_name";
@@ -104,8 +105,9 @@ for patch_name in $(grep -v '^#' < "$QUILT_PATCHES/series" ); do
 			GIT_AUTHOR_EMAIL="$patch_author_email"
 		fi
 	done
-	export GIT_AUTHOR_DATE=$(sed -ne 's/Date: //p' "$tmp_info")
-	export SUBJECT=$(sed -ne 's/Subject: //p' "$tmp_info")
+	GIT_AUTHOR_DATE=$(sed -ne 's/Date: //p' "$tmp_info")
+	SUBJECT=$(sed -ne 's/Subject: //p' "$tmp_info")
+	export GIT_AUTHOR_DATE SUBJECT
 	if [ -z "$SUBJECT" ] ; then
 		SUBJECT=$(echo $patch_name | sed -e 's/.patch$//')
 	fi
-- 
1.5.3.6.2064.g4e322

Re: [PATCH] Replace instances of export VAR=VAL with VAR=VAL; export VAR

[David Kastrup]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> It might be POSIX, but there are shells that do not like the
> expression 'export VAR=VAL'.  To be on the safe side, rewrite them
> into 'VAR=VAL' and 'export VAR'.

This seems like activism to me: if no supported shell has a problem with
that construct, why bother?

There probably should be a list of shells we try supporting, and in
particular a list of those where we don't bother.

And if a POSIX construct is supported by all shells we try supporting, I
don't see a point in patching it away.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: [PATCH] Replace instances of export VAR=VAL with VAR=VAL; export VAR

[Nguyen Thai Ngoc Duy]

On Nov 28, 2007 8:57 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
>
> It might be POSIX, but there are shells that do not like the
> expression 'export VAR=VAL'.  To be on the safe side, rewrite them
> into 'VAR=VAL' and 'export VAR'.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>
>         On Wed, 28 Nov 2007, Wincent Colaiuta wrote:
>
>         > I'm still a little concerned that nobody commented when I
>         > pointed out that export VAR=VAL is used elsewhere in Git,
>         > especially in git-clone.sh, which is very commonly-used
>         > porcelain. Is it a problem?
>
>         How's that for a comment?
>
>  git-clone.sh         |    2 +-
>  git-filter-branch.sh |   20 ++++++++++++--------
>  git-quiltimport.sh   |   10 ++++++----
>  3 files changed, 19 insertions(+), 13 deletions(-)

Why leave test scripts behind?

-- 
Duy

Re: [PATCH] Replace instances of export VAR=VAL with VAR=VAL; export VAR

[Johannes Sixt]

Johannes Schindelin schrieb:
> -			s/.*/export GIT_'$uid'_NAME='\''&'\''/p
> +			s/.*/GIT_'$uid'_NAME='\''&'\''\nexport GIT_'$uid'_NAME/p

Recently there was a report that \n in the substitution side of s/// is not 
supported by all seds :-(

> -	export GIT_AUTHOR_NAME=$(sed -ne 's/Author: //p' "$tmp_info")
> -	export GIT_AUTHOR_EMAIL=$(sed -ne 's/Email: //p' "$tmp_info")
> +	GIT_AUTHOR_NAME=$(sed -ne 's/Author: //p' "$tmp_info")
> +	GIT_AUTHOR_EMAIL=$(sed -ne 's/Email: //p' "$tmp_info")
> +	export GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL

This even fixes a bug: ash supports export VAR=VAL, but *does* word-split 
VAL if it is not quoted.

-- Hannes

Re: [PATCH] Replace instances of export VAR=VAL with VAR=VAL; export VAR

[David Kastrup]

"Nguyen Thai Ngoc Duy" <pclouds@gmail.com> writes:

> On Nov 28, 2007 8:57 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
>>
>> It might be POSIX, but there are shells that do not like the
>> expression 'export VAR=VAL'.  To be on the safe side, rewrite them
>> into 'VAR=VAL' and 'export VAR'.
>
> Why leave test scripts behind?

Good keyword: how about starting the basic tests by testing for shell
features that are known and accepted to be used in git?

That way, we get direct feedback when feature assumptions are
problematic with some shells.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: [PATCH] Replace instances of export VAR=VAL with VAR=VAL; export VAR

[Nguyen Thai Ngoc Duy]

On Nov 28, 2007 9:24 PM, David Kastrup <dak@gnu.org> wrote:
> "Nguyen Thai Ngoc Duy" <pclouds@gmail.com> writes:
>
> > On Nov 28, 2007 8:57 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> >>
> >> It might be POSIX, but there are shells that do not like the
> >> expression 'export VAR=VAL'.  To be on the safe side, rewrite them
> >> into 'VAR=VAL' and 'export VAR'.
> >
> > Why leave test scripts behind?
>
> Good keyword: how about starting the basic tests by testing for shell
> features that are known and accepted to be used in git?
>
> That way, we get direct feedback when feature assumptions are
> problematic with some shells.

Uh.. I meant there are "export VAL=VAL" usage in test scripts and they
should be fixed as well. Anyway your idea is nice.
-- 
Duy

Re: [PATCH] Replace instances of export VAR=VAL with VAR=VAL; export VAR

[Johannes Schindelin]

Hi,

On Wed, 28 Nov 2007, Nguyen Thai Ngoc Duy wrote:

> On Nov 28, 2007 8:57 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> >
> > It might be POSIX, but there are shells that do not like the
> > expression 'export VAR=VAL'.  To be on the safe side, rewrite them
> > into 'VAR=VAL' and 'export VAR'.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >
> >         On Wed, 28 Nov 2007, Wincent Colaiuta wrote:
> >
> >         > I'm still a little concerned that nobody commented when I
> >         > pointed out that export VAR=VAL is used elsewhere in Git,
> >         > especially in git-clone.sh, which is very commonly-used
> >         > porcelain. Is it a problem?
> >
> >         How's that for a comment?
> >
> >  git-clone.sh         |    2 +-
> >  git-filter-branch.sh |   20 ++++++++++++--------
> >  git-quiltimport.sh   |   10 ++++++----
> >  3 files changed, 19 insertions(+), 13 deletions(-)
> 
> Why leave test scripts behind?

Because I did a

	git grep 'export.*=' *.sh

instead of a

	git grep 'export.*=' -- \*.sh

But this patch is

- good of its own, and

- a comment ;-)

Ciao,
Dscho

Re: [PATCH] Replace instances of export VAR=VAL with VAR=VAL; export VAR

[Johannes Schindelin]

Hi,

On Wed, 28 Nov 2007, Johannes Sixt wrote:

> Johannes Schindelin schrieb:
> > -			s/.*/export GIT_'$uid'_NAME='\''&'\''/p
> > +			s/.*/GIT_'$uid'_NAME='\''&'\''\nexport
> > GIT_'$uid'_NAME/p
> 
> Recently there was a report that \n in the substitution side of s/// is not
> supported by all seds :-(

Okay, how about replacing the line with

+			s/.*/GIT_'$uid'_NAME='\''&'\''\
+export GIT_'$uid'_NAME/p

Hmm?  (It works here.)

Ciao,
Dscho

Re: [PATCH] Replace instances of export VAR=VAL with VAR=VAL; export VAR

[Johannes Schindelin]

On Wed, 28 Nov 2007, Nguyen Thai Ngoc Duy wrote:

> > how about starting the basic tests by testing for shell features that 
> > are known and accepted to be used in git?
> >
> > That way, we get direct feedback when feature assumptions are 
> > problematic with some shells.
> 
> Uh.. I meant there are "export VAL=VAL" usage in test scripts and they 
> should be fixed as well. Anyway your idea is nice.

I have an even better idea: how about getting rid of all shell scripts, 
making them builtins? ;-)

Ciao,
Dscho

Re: [PATCH] Replace instances of export VAR=VAL with VAR=VAL; export VAR

[Johannes Sixt]

Johannes Schindelin schrieb:
> Hi,
> 
> On Wed, 28 Nov 2007, Johannes Sixt wrote:
> 
>> Johannes Schindelin schrieb:
>>> -			s/.*/export GIT_'$uid'_NAME='\''&'\''/p
>>> +			s/.*/GIT_'$uid'_NAME='\''&'\''\nexport
>>> GIT_'$uid'_NAME/p
>> Recently there was a report that \n in the substitution side of s/// is not
>> supported by all seds :-(
> 
> Okay, how about replacing the line with
> 
> +			s/.*/GIT_'$uid'_NAME='\''&'\''\
> +export GIT_'$uid'_NAME/p
> 
> Hmm?  (It works here.)

This looks good. The other case I'm refering to was also solved in this way.

-- Hannes

Re: [PATCH] Replace instances of export VAR=VAL with VAR=VAL; export VAR

[David Kastrup]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On Wed, 28 Nov 2007, Johannes Sixt wrote:
>
>> Johannes Schindelin schrieb:
>> > -			s/.*/export GIT_'$uid'_NAME='\''&'\''/p
>> > +			s/.*/GIT_'$uid'_NAME='\''&'\''\nexport
>> > GIT_'$uid'_NAME/p
>> 
>> Recently there was a report that \n in the substitution side of s/// is not
>> supported by all seds :-(
>
> Okay, how about replacing the line with
>
> +			s/.*/GIT_'$uid'_NAME='\''&'\''\
> +export GIT_'$uid'_NAME/p
>
> Hmm?  (It works here.)

Do we really want to replace code which is not as far as I know known to
fail under any of the supported shells with something which quite
possibly relies on particular sed implementations (and we have fewer
portability guarantees or even experience about sed than about the
shell, right?)?

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

[PATCH v2] Replace instances of export VAR=VAL with VAR=VAL; export VAR

[Johannes Schindelin]

It might be POSIX, but there are shells that do not like the
expression 'export VAR=VAL'.  To be on the safe side, rewrite them
into 'VAR=VAL' and 'export VAR'.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	On Wed, 28 Nov 2007, Johannes Sixt wrote:

	> Johannes Schindelin schrieb:
	> > 
	> > On Wed, 28 Nov 2007, Johannes Sixt wrote:
	> > 
	> > > Johannes Schindelin schrieb:
	> > > > -			s/.*/export GIT_'$uid'_NAME='\''&'\''/p
	> > > > +			s/.*/GIT_'$uid'_NAME='\''&'\''\nexport
	> > > > GIT_'$uid'_NAME/p
	> > >
	> > > Recently there was a report that \n in the substitution side 
	> > > of s/// is not supported by all seds :-(
	> > 
	> > Okay, how about replacing the line with
	> > 
	> > +			s/.*/GIT_'$uid'_NAME='\''&'\''\
	> > +export GIT_'$uid'_NAME/p
	> > 
	> > Hmm?  (It works here.)
	> 
	> This looks good. The other case I'm refering to was also solved 
	> in this way.

	Done.

 git-clone.sh         |    2 +-
 git-filter-branch.sh |   23 +++++++++++++++--------
 git-quiltimport.sh   |   10 ++++++----
 3 files changed, 22 insertions(+), 13 deletions(-)

diff --git a/git-clone.sh b/git-clone.sh
index 24ad179..ecf9d89 100755
--- a/git-clone.sh
+++ b/git-clone.sh
@@ -229,7 +229,7 @@ cleanup() {
 trap cleanup 0
 mkdir -p "$dir" && D=$(cd "$dir" && pwd) || usage
 test -n "$GIT_WORK_TREE" && mkdir -p "$GIT_WORK_TREE" &&
-W=$(cd "$GIT_WORK_TREE" && pwd) && export GIT_WORK_TREE="$W"
+W=$(cd "$GIT_WORK_TREE" && pwd) && GIT_WORK_TREE="$W" && export GIT_WORK_TREE
 if test yes = "$bare" || test -n "$GIT_WORK_TREE"; then
 	GIT_DIR="$D"
 else
diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index 19cab5a..074b2c0 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -66,17 +66,20 @@ set_ident () {
 			h
 			s/^'$lid' \([^<]*\) <[^>]*> .*$/\1/
 			s/'\''/'\''\'\'\''/g
-			s/.*/export GIT_'$uid'_NAME='\''&'\''/p
+			s/.*/GIT_'$uid'_NAME='\''&'\''\
+export GIT_'$uid'_NAME/p
 
 			g
 			s/^'$lid' [^<]* <\([^>]*\)> .*$/\1/
 			s/'\''/'\''\'\'\''/g
-			s/.*/export GIT_'$uid'_EMAIL='\''&'\''/p
+			s/.*/GIT_'$uid'_EMAIL='\''&'\''\
+export GIT_'$uid'_EMAIL/p
 
 			g
 			s/^'$lid' [^<]* <[^>]*> \(.*\)$/\1/
 			s/'\''/'\''\'\'\''/g
-			s/.*/export GIT_'$uid'_DATE='\''&'\''/p
+			s/.*/GIT_'$uid'_DATE='\''&'\''\
+export GIT_'$uid'_DATE/p
 
 			q
 		}
@@ -84,7 +87,7 @@ set_ident () {
 
 	LANG=C LC_ALL=C sed -ne "$pick_id_script"
 	# Ensure non-empty id name.
-	echo "[ -n \"\$GIT_${uid}_NAME\" ] || export GIT_${uid}_NAME=\"\${GIT_${uid}_EMAIL%%@*}\""
+	echo "case \"\$GIT_${uid}_NAME\" in \"\") GIT_${uid}_NAME=\"\${GIT_${uid}_EMAIL%%@*}\" && export GIT_${uid}_NAME;; esac"
 }
 
 USAGE="[--env-filter <command>] [--tree-filter <command>] \
@@ -206,7 +209,8 @@ done < "$tempdir"/backup-refs
 ORIG_GIT_DIR="$GIT_DIR"
 ORIG_GIT_WORK_TREE="$GIT_WORK_TREE"
 ORIG_GIT_INDEX_FILE="$GIT_INDEX_FILE"
-export GIT_DIR GIT_WORK_TREE=.
+GIT_WORK_TREE=.
+export GIT_DIR GIT_WORK_TREE
 
 # These refs should be updated if their heads were rewritten
 
@@ -231,7 +235,8 @@ done > "$tempdir"/heads
 test -s "$tempdir"/heads ||
 	die "Which ref do you want to rewrite?"
 
-export GIT_INDEX_FILE="$(pwd)/../index"
+GIT_INDEX_FILE="$(pwd)/../index"
+export GIT_INDEX_FILE
 git read-tree || die "Could not seed the index"
 
 ret=0
@@ -267,7 +272,8 @@ while read commit parents; do
 		git read-tree -i -m $commit:"$filter_subdir"
 	esac || die "Could not initialize the index"
 
-	export GIT_COMMIT=$commit
+	GIT_COMMIT=$commit
+	export GIT_COMMIT
 	git cat-file commit "$commit" >../commit ||
 		die "Cannot read commit $commit"
 
@@ -401,7 +407,8 @@ if [ "$filter_tag_name" ]; then
 
 		[ -f "../map/$sha1" ] || continue
 		new_sha1="$(cat "../map/$sha1")"
-		export GIT_COMMIT="$sha1"
+		GIT_COMMIT="$sha1"
+		export GIT_COMMIT
 		new_ref="$(echo "$ref" | eval "$filter_tag_name")" ||
 			die "tag name filter failed: $filter_tag_name"
 
diff --git a/git-quiltimport.sh b/git-quiltimport.sh
index 6b0c4d2..233e5ea 100755
--- a/git-quiltimport.sh
+++ b/git-quiltimport.sh
@@ -77,8 +77,9 @@ for patch_name in $(grep -v '^#' < "$QUILT_PATCHES/series" ); do
 	}
 
 	# Parse the author information
-	export GIT_AUTHOR_NAME=$(sed -ne 's/Author: //p' "$tmp_info")
-	export GIT_AUTHOR_EMAIL=$(sed -ne 's/Email: //p' "$tmp_info")
+	GIT_AUTHOR_NAME=$(sed -ne 's/Author: //p' "$tmp_info")
+	GIT_AUTHOR_EMAIL=$(sed -ne 's/Email: //p' "$tmp_info")
+	export GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL
 	while test -z "$GIT_AUTHOR_EMAIL" && test -z "$GIT_AUTHOR_NAME" ; do
 		if [ -n "$quilt_author" ] ; then
 			GIT_AUTHOR_NAME="$quilt_author_name";
@@ -104,8 +105,9 @@ for patch_name in $(grep -v '^#' < "$QUILT_PATCHES/series" ); do
 			GIT_AUTHOR_EMAIL="$patch_author_email"
 		fi
 	done
-	export GIT_AUTHOR_DATE=$(sed -ne 's/Date: //p' "$tmp_info")
-	export SUBJECT=$(sed -ne 's/Subject: //p' "$tmp_info")
+	GIT_AUTHOR_DATE=$(sed -ne 's/Date: //p' "$tmp_info")
+	SUBJECT=$(sed -ne 's/Subject: //p' "$tmp_info")
+	export GIT_AUTHOR_DATE SUBJECT
 	if [ -z "$SUBJECT" ] ; then
 		SUBJECT=$(echo $patch_name | sed -e 's/.patch$//')
 	fi
-- 
1.5.3.6.2065.gd47ac

Re: [PATCH v2] Replace instances of export VAR=VAL with VAR=VAL; export VAR

[David Kastrup]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> It might be POSIX, but there are shells that do not like the
> expression 'export VAR=VAL'.

As long as we have no positive report about any such shell that
_otherwise_ would be usable for git, why bother?

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: Rebase/cherry-picking idea

[Junio C Hamano]

Wincent Colaiuta <win@wincent.com> writes:

> I'm still a little concerned that nobody commented when I pointed out
> that export VAR=VAL is used elsewhere in Git, especially in git-
> clone.sh, which is very commonly-used porcelain. Is it a problem?

We are waiting to find out, aren't we, after having the discussion.

Re: [PATCH] Replace instances of export VAR=VAL with VAR=VAL; export VAR

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

> Johannes Schindelin schrieb:
> ...
>>> Recently there was a report that \n in the substitution side of s/// is not
>>> supported by all seds :-(
>>
>> Okay, how about replacing the line with
>>
>> +			s/.*/GIT_'$uid'_NAME='\''&'\''\
>> +export GIT_'$uid'_NAME/p
>>
>> Hmm?  (It works here.)
>
> This looks good. The other case I'm refering to was also solved in this way.

That looks ugly to me.

Is there a particular reason to force linebreak when a semicolon would
do?

Re: [PATCH] Replace instances of export VAR=VAL with VAR=VAL; export VAR

[Johannes Schindelin]

Hi,

On Wed, 28 Nov 2007, Junio C Hamano wrote:

> Johannes Sixt <j.sixt@viscovery.net> writes:
> 
> > Johannes Schindelin schrieb:
> > ...
> >>> Recently there was a report that \n in the substitution side of s/// is not
> >>> supported by all seds :-(
> >>
> >> Okay, how about replacing the line with
> >>
> >> +			s/.*/GIT_'$uid'_NAME='\''&'\''\
> >> +export GIT_'$uid'_NAME/p
> >>
> >> Hmm?  (It works here.)
> >
> > This looks good. The other case I'm refering to was also solved in this way.
> 
> That looks ugly to me.
> 
> Is there a particular reason to force linebreak when a semicolon would
> do?

D'oh.  Of course.  You want me to resend?

Ciao,
Dscho

Re: [PATCH v2] Replace instances of export VAR=VAL with VAR=VAL; export VAR

[Junio C Hamano]

David Kastrup <dak@gnu.org> writes:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>> It might be POSIX, but there are shells that do not like the
>> expression 'export VAR=VAL'.
>
> As long as we have no positive report about any such shell that
> _otherwise_ would be usable for git, why bother?

I thought somebody already mention that ash mishandles "export VAR=VAL"
but otherwise Ok.

Re: [PATCH] Replace instances of export VAR=VAL with VAR=VAL; export VAR

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> Is there a particular reason to force linebreak when a semicolon would
>> do?
>
> D'oh.  Of course.  You want me to resend?

Semicolon I can handle but you seem to have local changes to filter
branch.

Re: [PATCH v2] Replace instances of export VAR=VAL with VAR=VAL; export VAR

[David Kastrup]

Junio C Hamano <gitster@pobox.com> writes:

> David Kastrup <dak@gnu.org> writes:
>
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>>
>>> It might be POSIX, but there are shells that do not like the
>>> expression 'export VAR=VAL'.
>>
>> As long as we have no positive report about any such shell that
>> _otherwise_ would be usable for git, why bother?
>
> I thought somebody already mention that ash mishandles "export VAR=VAL"
> but otherwise Ok.

dak@lola:~$ ash
$ export JUNK=woozle
$ sh -c 'echo $JUNK'
woozle
$ exit
dak@lola:~$ dash
$ export JUNK=woozle
$ sh -c 'echo $JUNK'
woozle
$ exit

What problem are we talking about exactly, and with what shell?

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: [PATCH v2] Replace instances of export VAR=VAL with VAR=VAL; export VAR

[Jeff King]

On Wed, Nov 28, 2007 at 11:53:13PM +0100, David Kastrup wrote:

> dak@lola:~$ ash
> $ export JUNK=woozle
> $ sh -c 'echo $JUNK'
> woozle
> $ exit
> dak@lola:~$ dash
> $ export JUNK=woozle
> $ sh -c 'echo $JUNK'
> woozle
> $ exit
> 
> What problem are we talking about exactly, and with what shell?

$ uname -sr
FreeBSD 6.1-RELEASE-p17-jc1
$ /bin/sh
$ FOO='with spaces'
$ echo $FOO
with spaces
$ OK=$FOO; export OK
$ sh -c 'echo $OK'
with spaces
$ export BAD=$FOO
$ sh -c 'echo $BAD'
with
$ echo $BAD
with

-Peff

Re: [PATCH v2] Replace instances of export VAR=VAL with VAR=VAL; export VAR

[Johannes Schindelin]

Hi,

On Wed, 28 Nov 2007, Junio C Hamano wrote:

> > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> >
> >> It might be POSIX, but there are shells that do not like the
> >> expression 'export VAR=VAL'.
> >
> > As long as we have no positive report about any such shell that
> > _otherwise_ would be usable for git, why bother?
> 
> I thought somebody already mention that ash mishandles "export VAR=VAL"
> but otherwise Ok.

I thought I read an implicit request from you.  And yes, there was an 
incidental bugfix in quiltimport.

Besides, this "why bother?" sounds awfully like "it's in POSIX and I 
ignore the experience of Junio who knows that there are/were reasons not 
to use export VAR=VAL" to me.  After all, I read this often enough on this 
list, and just forgot to fix it in filter-branch before submitting the 
patch.

Ciao,
Dscho

Re: [PATCH] Replace instances of export VAR=VAL with VAR=VAL; export VAR

[Johannes Schindelin]

Hi,

On Wed, 28 Nov 2007, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> Is there a particular reason to force linebreak when a semicolon would
> >> do?
> >
> > D'oh.  Of course.  You want me to resend?
> 
> Semicolon I can handle but you seem to have local changes to filter
> branch.

Only that patch to de-uglify the functions in the commit filter that I 
sent out earlier.

Ciao,
Dscho

Re: [PATCH v2] Replace instances of export VAR=VAL with VAR=VAL; export VAR

[David Kastrup]

Jeff King <peff@peff.net> writes:

> On Wed, Nov 28, 2007 at 11:53:13PM +0100, David Kastrup wrote:
>
>> dak@lola:~$ ash
>> $ export JUNK=woozle
>> $ sh -c 'echo $JUNK'
>> woozle
>> $ exit
>> dak@lola:~$ dash
>> $ export JUNK=woozle
>> $ sh -c 'echo $JUNK'
>> woozle
>> $ exit
>> 
>> What problem are we talking about exactly, and with what shell?
>
> $ uname -sr
> FreeBSD 6.1-RELEASE-p17-jc1
> $ /bin/sh
> $ FOO='with spaces'
> $ echo $FOO
> with spaces
> $ OK=$FOO; export OK
> $ sh -c 'echo $OK'
> with spaces
> $ export BAD=$FOO
> $ sh -c 'echo $BAD'
> with
> $ echo $BAD
> with

I'd write
export BAD="$FOO"
anyhow.  And also OK="$FOO" by the way.  The alternatives scare me.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: [PATCH v2] Replace instances of export VAR=VAL with VAR=VAL; export VAR

[Johannes Sixt]

David Kastrup schrieb:
> Junio C Hamano <gitster@pobox.com> writes:
>> I thought somebody already mention that ash mishandles "export VAR=VAL"
>> but otherwise Ok.
> 
> dak@lola:~$ ash
> $ export JUNK=woozle
> $ sh -c 'echo $JUNK'
> woozle
> $ exit
> dak@lola:~$ dash
> $ export JUNK=woozle
> $ sh -c 'echo $JUNK'
> woozle
> $ exit
> 
> What problem are we talking about exactly, and with what shell?

$ export foo="bar    baz"
$ bash
bash$ export JUNK=$foo
bash$ sh -c 'echo "$JUNK"'
bar    baz
bash$ exit
exit
$ ash
ash$ export JUNK=$foo
ash$ sh -c 'echo "$JUNK"'
bar
ash$ exit

The problem is $foo not being quoted on the RHS of the assignment of the 
export command: bash doesn't word-split, but ash does. Hence, *if*
export VAR=VAL is used, always quote VAL.

-- Hannes

Re: [PATCH v2] Replace instances of export VAR=VAL with VAR=VAL; export VAR

[David Kastrup]

Johannes Sixt <j.sixt@viscovery.net> writes:

>> What problem are we talking about exactly, and with what shell?
>
> $ export foo="bar    baz"
> $ bash
> bash$ export JUNK=$foo
> bash$ sh -c 'echo "$JUNK"'
> bar    baz
> bash$ exit
> exit
> $ ash
> ash$ export JUNK=$foo
> ash$ sh -c 'echo "$JUNK"'
> bar
> ash$ exit
>
> The problem is $foo not being quoted on the RHS of the assignment of
> the export command: bash doesn't word-split, but ash does. Hence, *if*
> export VAR=VAL is used, always quote VAL.

I much prefer the quoting solution over the quite more invasive rewrites
proposed here.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum